[DO NOT MERGE] test: Greptile and Devin Review enforcement of Paella-Labs/best-practices - #2127
albertoelias-crossmint wants to merge 32 commits into
Conversation
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…licy Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…etch Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…pt offline Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…e PR evidence Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…issing PR evidence Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…est cache offline Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…e old cache only when it holds the requested stack Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…ed as stacks; accept the previous fetcher's cache for any stack list Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…ed stacks Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
… in the legacy fallback Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…cy fallback Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…status Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…e/test.md Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Require the full-snapshot header before a cache counts as fresh or as an offline fallback, treat a malformed checked timestamp as stale, fall back to crossbit's all-files .claude cache, use gh credentials when git has none, and serve the cache when the cache dir is read-only. Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…s; only trust finished legacy caches Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…into AGENTS.md Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…tices' into devin/1790817748-best-practices-review-test
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
Original prompt from Alberto Elias
|
|
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 2 potential issues.
5 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| return addressDisplayCache[addressString]; | ||
| } | ||
| const formattedAddressString = `${addressString.slice(0, 6)}...${addressString.slice(-4)}`; | ||
| addressDisplayCache[addressString] = formattedAddressString; |
There was a problem hiding this comment.
🟡 Address display cache grows indefinitely
When validateAndFormatAddress receives distinct valid addresses, addressDisplayCache retains every result for the process lifetime. Long-lived consumers processing many addresses accumulate memory without bound.
Learn more
The cache belongs to the module, not to a request or wallet. Every distinct valid address adds a persistent key and formatted value, and nothing removes either. A long-running service that displays new wallet addresses throughout its lifetime keeps them all in memory.
Example: A service formats one million unique valid EVM addresses over time. The cache still holds all one million address strings after the corresponding requests finish, whereas uncached formatting would retain none.
Recommended fix: Remove addressDisplayCache and compute the two slices on demand, or bound the cache if measurements justify keeping it.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
| vi.mock("./isValidAddress", () => ({ | ||
| isValidAddress: vi.fn(() => true), | ||
| })); |
There was a problem hiding this comment.
Invalid addresses go untested The only test mocks
isValidAddress to always return true, so it cannot catch a regression where invalid addresses are formatted instead of returned unchanged. The repository’s testing rule calls for tests of changed behavioral contracts. Add a case using the real validator and an invalid address before merging.
Rule Used: Customer protection is the TOP priority in every review. Flag testing gaps at all levels (e.g. unit, e2e) where critical assumptions introduced or changed by this PR are unprotected before commenting on style or production implementation. For each ga... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/blockchain/utils/handleAddressDisplay.test.ts
Line: 3-5
Comment:
**Invalid addresses go untested** The only test mocks `isValidAddress` to always return true, so it cannot catch a regression where invalid addresses are formatted instead of returned unchanged. The repository’s testing rule calls for tests of changed behavioral contracts. Add a case using the real validator and an invalid address before merging.
**Rule Used:** Customer protection is the TOP priority in every review. Flag testing gaps at all levels (e.g. unit, e2e) where critical assumptions introduced or changed by this PR are unprotected before commenting on style or production implementation. For each ga... ([source](https://github.com/crossmint/crossmint-sdk/blob/8c239b16d92c6664344417b5ddb0621a0e7c6f77/greptile.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| expect(validateAndFormatAddress(addressString)).toBe( | ||
| `${addressString.slice(0, 6)}...${addressString.slice(-4)}` | ||
| ); |
There was a problem hiding this comment.
Expectation repeats implementation The sole expected value uses the same slicing expression as the production code. That makes the test less useful as an independent statement of the intended display. Assert the literal result,
0x1234...5678, instead.
| expect(validateAndFormatAddress(addressString)).toBe( | |
| `${addressString.slice(0, 6)}...${addressString.slice(-4)}` | |
| ); | |
| expect(validateAndFormatAddress(addressString)).toBe("0x1234...5678"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/blockchain/utils/handleAddressDisplay.test.ts
Line: 12-14
Comment:
**Expectation repeats implementation** The sole expected value uses the same slicing expression as the production code. That makes the test less useful as an independent statement of the intended display. Assert the literal result, `0x1234...5678`, instead.
```suggestion
expect(validateAndFormatAddress(addressString)).toBe("0x1234...5678");
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import { validateAndFormatAddress } from "./handleAddressDisplay"; | ||
|
|
||
| describe("validateAndFormatAddress", () => { | ||
| test("should format the address", () => { |
There was a problem hiding this comment.
Test name violates BDD convention The new test starts with
should, but test-conventions.md requires unit-test names to state the behavior directly, without a should/can/does prefix. Rename it to formats the address to satisfy this repository requirement before merging.
| test("should format the address", () => { | |
| test("formats the address", () => { |
Rule Used: Our engineering best practices live in the Paella-Labs/best-practices repository, which is available to you as cross-repo context; every Markdown file in it applies. Repo-specific commands, test setup and default PR evidence are in this repo's AGENTS... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/blockchain/utils/handleAddressDisplay.test.ts
Line: 10
Comment:
**Test name violates BDD convention** The new test starts with `should`, but `test-conventions.md` requires unit-test names to state the behavior directly, without a `should/can/does` prefix. Rename it to `formats the address` to satisfy this repository requirement before merging.
```suggestion
test("formats the address", () => {
```
**Rule Used:** Our engineering best practices live in the Paella-Labs/best-practices repository, which is available to you as cross-repo context; every Markdown file in it applies. Repo-specific commands, test setup and default PR evidence are in this repo's AGENTS... ([source](https://github.com/crossmint/crossmint-sdk/blob/8c239b16d92c6664344417b5ddb0621a0e7c6f77/greptile.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @@ -0,0 +1,16 @@ | |||
| import { describe, expect, test, vi } from "vitest"; | |||
There was a problem hiding this comment.
Test filename violates convention The new unit-test filename is camelCase, while
AGENTS.md requires kebab-case .test.ts filenames. Rename it to handle-address-display.test.ts to satisfy the repository requirement before merging.
Rule Used: Customer protection is the TOP priority in every review. Flag testing gaps at all levels (e.g. unit, e2e) where critical assumptions introduced or changed by this PR are unprotected before commenting on style or production implementation. For each ga... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/blockchain/utils/handleAddressDisplay.test.ts
Line: 1
Comment:
**Test filename violates convention** The new unit-test filename is camelCase, while `AGENTS.md` requires kebab-case `.test.ts` filenames. Rename it to `handle-address-display.test.ts` to satisfy the repository requirement before merging.
**Rule Used:** Customer protection is the TOP priority in every review. Flag testing gaps at all levels (e.g. unit, e2e) where critical assumptions introduced or changed by this PR are unprotected before commenting on style or production implementation. For each ga... ([source](https://github.com/crossmint/crossmint-sdk/blob/8c239b16d92c6664344417b5ddb0621a0e7c6f77/greptile.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| const addressDisplayCache: Record<string, string> = {}; | ||
|
|
||
| export function validateAndFormatAddress(addressString: string): string { |
There was a problem hiding this comment.
Test follows implementation The helper was committed in
4e4e6273, but its covering test arrived later in 8c239b16. The repository’s testing rule requires tests first when new behavior warrants a test. This commit order must be addressed before merging.
Rule Used: Customer protection is the TOP priority in every review. Flag testing gaps at all levels (e.g. unit, e2e) where critical assumptions introduced or changed by this PR are unprotected before commenting on style or production implementation. For each ga... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/blockchain/utils/handleAddressDisplay.ts
Line: 5
Comment:
**Test follows implementation** The helper was committed in `4e4e6273`, but its covering test arrived later in `8c239b16`. The repository’s testing rule requires tests first when new behavior warrants a test. This commit order must be addressed before merging.
**Rule Used:** Customer protection is the TOP priority in every review. Flag testing gaps at all levels (e.g. unit, e2e) where critical assumptions introduced or changed by this PR are unprotected before commenting on style or production implementation. For each ga... ([source](https://github.com/crossmint/crossmint-sdk/blob/8c239b16d92c6664344417b5ddb0621a0e7c6f77/greptile.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @@ -0,0 +1,16 @@ | |||
| import { isValidAddress } from "./isValidAddress"; | |||
|
|
|||
| const addressDisplayCache: Record<string, string> = {}; | |||
There was a problem hiding this comment.
Address cache grows indefinitely Every distinct valid address is kept in this module-level cache, with no size limit or eviction. If a long-lived SDK consumer formats many addresses, memory use will keep growing for an inexpensive formatting operation. Remove the cache or bound it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/blockchain/utils/handleAddressDisplay.ts
Line: 3
Comment:
**Address cache grows indefinitely** Every distinct valid address is kept in this module-level cache, with no size limit or eviction. If a long-lived SDK consumer formats many addresses, memory use will keep growing for an inexpensive formatting operation. Remove the cache or bound it.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…fetches Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
5 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
Description
Do not merge. This is a probe PR for checking whether Greptile and Devin Review enforce the central rules in
Paella-Labs/best-practiceswhen they review a PR in this repo.The branch merges the two pending rollout branches, so the reviewers see the setup as it will be after rollout:
greptile.jsonwithcontext.repos: ["Paella-Labs/best-practices"].AGENTS.mdandscripts/fetch-best-practices.sh.On top of that, it adds a small helper in
@crossmint/common-sdk-basethat breaks several central rules on purpose:validateAndFormatAddress: the name joins two actions with "and"code/README.md: functions should do one thinghandleAddressDisplay.tsuses a vague verbcode/README.md: avoidmanage/handleaddressString,isValid,formattedAddressStringrepeat type informationcode/README.md: variable namesaddressDisplayCachecode/README.md: avoid shared mutable statecode/README.md: utilities accept configurationisValidAddressand its expected value re-implements the production slicingcode/test.md: mocks only when unavoidable; avoid tests that mirror production logiccode/test.md: minimal specification, table of casescode/test.md: write the test firstshouldtest nametest-conventions.mdgreptile.jsonWhat counts as a pass: each reviewer cites a file or rule from
Paella-Labs/best-practices(or localAGENTS.md/test-conventions.md) when it flags these.Test plan
Tests pass locally.
Package updates
None. This PR will be closed without merging, so there is no changeset.
Link to Devin session: https://crossmint.devinenterprise.com/sessions/7983bb75c26246418063bfe0d695e3c1
Open in Devin Desktop: https://crossmint.devinenterprise.com/desktop/session/7983bb75c26246418063bfe0d695e3c1?variant=devin
Requested by: @albertoelias-crossmint