Skip to content

[DO NOT MERGE] test: Greptile and Devin Review enforcement of Paella-Labs/best-practices - #2127

Open
albertoelias-crossmint wants to merge 32 commits into
mainfrom
devin/1790817748-best-practices-review-test
Open

albertoelias-crossmint wants to merge 32 commits into
mainfrom
devin/1790817748-best-practices-review-test

Conversation

@albertoelias-crossmint

@albertoelias-crossmint albertoelias-crossmint commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Do not merge. This is a probe PR for checking whether Greptile and Devin Review enforce the central rules in Paella-Labs/best-practices when 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:

On top of that, it adds a small helper in @crossmint/common-sdk-base that breaks several central rules on purpose:

Deliberate violation Central rule a reviewer should cite
validateAndFormatAddress: the name joins two actions with "and" code/README.md: functions should do one thing
File name handleAddressDisplay.ts uses a vague verb code/README.md: avoid manage/handle
addressString, isValid, formattedAddressString repeat type information code/README.md: variable names
Module-level mutable addressDisplayCache code/README.md: avoid shared mutable state
The utility hardcodes its format (6 + 4 chars) instead of taking it as configuration code/README.md: utilities accept configuration
The test mocks isValidAddress and its expected value re-implements the production slicing code/test.md: mocks only when unavoidable; avoid tests that mirror production logic
One fixed input, no invalid-address or boundary cases code/test.md: minimal specification, table of cases
The test lands in a commit after the code code/test.md: write the test first
should test name local test-conventions.md
This description has no evidence of the feature working PR evidence check in greptile.json

What counts as a pass: each reviewer cites a file or rule from Paella-Labs/best-practices (or local AGENTS.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


Devin Review

devin-ai-integration Bot and others added 30 commits September 28, 2026 22:25
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>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access that start with 'Devin'.

  • Disable automatic comment, CI, and merge conflict monitoring

Original prompt from Alberto Elias

Extends the existing parallel plan with two additions — fold into the same workstreams:

  1. Greptile coverage (new Workstream 6, independent)

    • Copy the best-practices compliance rule from Paella-Labs/crossbit-main/greptile.json (the "Our engineering best practices are documented at..." entry) into a greptile.json / .greptile.json in every repo where Greptile is installed: crossmint-sdk, universal-checkout, open-signer, crossmint-kotlin-sdk, crossmint-flutter-sdk, crossmint-swift-sdk, solana-smart-account, stellar-smart-account, smart-wallet-modules, lobster.cash, devin-tools, paella-os. Repos that don't use Greptile can skip the file.
    • First verify whether Paella-Labs/best-practices is public — if private, Greptile can't fetch the URL, so either (a) make the repo public (it contains no secrets by design), or (b) inline the short testing policy verbatim in each greptile.json rule instead of linking. Prefer (a); document the choice in best-practices/README.md.
    • Add one testing-specific greptile rule per repo, adapted from crossbit-main's apps/**/libraries/** rule: reviewers should flag missing E2E artifacts and unit-tests-written-after-code on new changes only (incremental improvement, not retroactive enforcement).
  2. Artifact defaults (fold into existing workstreams 1, 3, 4, 5)

    • best-practices/core.md: expand the E2E bullet with the definition — "a self-contained output of the E2E run that lets a reviewer confirm the feature works without re-running it: captures evidence of what the system did, not just a pass/fail exit code. Each repo's AGENTS.md names its default artifact."
    • Each repo workstream adds one line to that repo's AGENTS.md under the testing section, per the defaults table: playwright report+trace for TS web/API repos; emulator/simulator report+recording for mobile SDKs; devnet/testnet tx signature for the smart contract repos (using deterministic seeds); CLI transcript/snapsho... (539 chars truncated...)

@changeset-bot

changeset-bot Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: bf01ced

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Devin Review

return addressDisplayCache[addressString];
}
const formattedAddressString = `${addressString.slice(0, 6)}...${addressString.slice(-4)}`;
addressDisplayCache[addressString] = formattedAddressString;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread scripts/fetch-best-practices.sh Outdated
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium risk] Adds test infrastructure and agent configuration for code review.

The PR should not merge until its explicit repository testing requirements are satisfied; the remaining quality concerns are non-blocking.

Reviews (1) · Last reviewed commit: "test(common-base): cover address display..."

Comment on lines +3 to +5
vi.mock("./isValidAddress", () => ({
isValidAddress: vi.fn(() => true),
}));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Comment on lines +12 to +14
expect(validateAndFormatAddress(addressString)).toBe(
`${addressString.slice(0, 6)}...${addressString.slice(-4)}`
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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> = {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Comment thread scripts/fetch-best-practices.sh
devin-ai-integration Bot and others added 2 commits October 1, 2026 16:17
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
…fetches

Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

5 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

Comment thread scripts/fetch-best-practices.sh

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant