docs: add review-facing CLAUDE.md guidance (draft for team review) - #2916
Open
piyalbasu wants to merge 6 commits into
Open
docs: add review-facing CLAUDE.md guidance (draft for team review)#2916piyalbasu wants to merge 6 commits into
piyalbasu wants to merge 6 commits into
Conversation
/code-review ingests CLAUDE.md and audits diffs against it, but this repo only had a 7-line extension/CLAUDE.md pointing at specs/ — so the review's compliance agents had nothing to check. Add specific, citable rules: - root CLAUDE.md: lead secret-leak invariant (decomposed into concrete leak channels + anchored to the background/popup boundary), backend-v2 chokepoint usage, cache/side-effect-ordering rules (from real regressions #2603/#2605), @stellar/* exact-version pinning, and a TODO(team) domain-invariants section. - extension/e2e-tests/CLAUDE.md: backend-v2 calls run in the MV3 service worker; mock with context.route/context.on, not page.route/page.on. Rules are written to be citable against a line and exclude anything CI already enforces. Domain invariants are stubbed for the team to fill — these need ratification before merge; opening as a draft. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-b96c5d743ddf9f0bc2bd (SDF collaborators only — install instructions in the release description) |
Every transaction sign/submit flow (dApp sign, send, swap, send-collectible) must attempt a Blockaid scanTx (via useScanTx) and surface the scanResult before signing; on unable-to-scan, show the explicit warning rather than silently proceeding. Grounded in the existing scanTx/useScanTx pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Network-dependent code must handle Mainnet/Testnet/Futurenet AND custom networks (isCustomNetwork); a NETWORKS switch needs a default/custom branch; unsupported networks get an explicit graceful fallback, not an unhandled case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Any code touching addresses must handle classic G accounts, contract C addresses (isContractId), muxed M accounts (isMuxedAccount), and federated addresses (isFederationAddress/isValidFederatedDomain): resolve federated before use, preserve the muxed memo id, route C addresses where an account is expected, validate with isValidStellarAddress. Completes the domain-invariants section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
All four seed invariants filled (decimals, network, signing/Blockaid, addresses); drop the "TODO(team)" from the header now that it's a live list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The repo already had a rich AGENTS.md (the file non-Claude tools read) while my new CLAUDE.md was Claude-only — fragmenting guidance. Consolidate to one source of truth: - Fold the review invariants (secret-leak, backend-v2 chokepoint, cache/ordering, @stellar pinning, and the four domain invariants) into AGENTS.md as an "Invariants — must always hold" section, next to Security-Sensitive Areas. - Replace the standalone root CLAUDE.md with a symlink -> AGENTS.md, and do the same for the e2e-tests dir (new extension/e2e-tests/AGENTS.md + symlinked CLAUDE.md). Now every path reads the same content: /code-review (via CLAUDE.md), Codex/other tools (via AGENTS.md), and Claude Code sessions (which auto-load it) — no drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates Freighter’s automated-review guidance and shares it across agent tools via symlinks.
Changes:
- Adds security, backend, caching, dependency, and domain invariants.
- Documents service-worker request interception for E2E tests.
- Links Claude guidance to the corresponding agent documents.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
AGENTS.md |
Adds review-facing invariants. |
CLAUDE.md |
Links to root agent guidance. |
extension/e2e-tests/AGENTS.md |
Documents backend-v2 E2E interception. |
extension/e2e-tests/CLAUDE.md |
Links to E2E agent guidance. |
Comments suppressed due to low confidence (1)
AGENTS.md:145
- Dedicated popup flows currently return
mnemonicPhrasefrom the background after password validation (getMnemonicPhrase.ts:49-51andshowBackupPhrase.ts:64-66). Saying only{ status, body }crosses this boundary is therefore false. Restrict that response shape to backend-v2 derivation and explicitly carve out authenticated mnemonic-display/onboarding flows while retaining the private/auth-key prohibition.
- returned across the background→popup boundary — key derivation happens in the
background service worker; only `{ status, body }` (never the mnemonic/key)
crosses back to the popup;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+139
to
+140
| - written to disk or unencrypted storage — they live only in the encrypted | ||
| session store; |
Comment on lines
+192
to
+198
| - **Address handling:** any code that accepts, parses, displays, resolves, or | ||
| routes a Stellar address must handle all four types — classic **G** accounts, | ||
| contract **C** addresses (`isContractId`), muxed **M** accounts | ||
| (`isMuxedAccount`), and **federated** addresses (`isFederationAddress` / | ||
| `isValidFederatedDomain`). Resolve federated before use; preserve/extract the | ||
| muxed memo id (don't strip or mislabel it); route C addresses appropriately | ||
| where a fundable account is expected; validate with `isValidStellarAddress`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Our automated PR reviewer (
/code-review) readsCLAUDE.mdand checks each change against it, and non-Claude tools readAGENTS.md— but the review-relevant rules were thin/split. This consolidates a set of specific, review-checkable invariants (led by the wallet's secret-handling rule) intoAGENTS.md, and symlinksCLAUDE.md → AGENTS.mdso every review path reads the same source with no drift.Draft on purpose: please ratify the rules before this lands — a wrong or over-broad rule makes reviewers (and coding agents) confidently wrong. Poke holes.
Details (for reviewers)
Why here:
/code-reviewgathersCLAUDE.md(root + modified dirs) and audits diffs against it; other agent tools readAGENTS.md; Claude Code sessions auto-load both. The repo already had a richAGENTS.mdplus a near-emptyextension/CLAUDE.md, so review rules had no real home and would fragment if added Claude-only. One file, both names, avoids that.What changed:
AGENTS.md— new "Invariants — must always hold" section (after Security-Sensitive Areas):fetchBackendV2()/the chokepoint;skipAuthfor never-gated endpoints.res.okbefore caching / fetch on empty (from fix: validate HTTP status before caching API responses #2603); validate before persisting side effects like the allow-list (from fix(grantAccess): validate queue item before setting allowlist #2605).@stellar/*exact-version pinning.isCustomNetwork), signing → always Blockaid-scanTxbefore signing, address handling (G/C/muxed/federated viaisContractId/isMuxedAccount/isFederationAddress).CLAUDE.md→AGENTS.mdsymlink (root).extension/e2e-tests/AGENTS.md(new) — backend-v2 requests run in the MV3 service worker → mock withcontext.route/context.on, notpage.route/page.on;extension/e2e-tests/CLAUDE.md→AGENTS.mdsymlink.Design constraints: every rule names the real helper/artifact so it's citable against a line; nothing a linter/typechecker/CI already enforces is included (the reviewer skips those by design).
Known caveats:
@stellar/*pinning reflects a stated team convention; I did not verify currentpackage.jsonpins them — confirm.extension/CLAUDE.md(a specs pointer) is left as-is; symlinking it to anextension/AGENTS.mdis a possible follow-up if you want that dir's guidance to reach non-Claude tools too.Provenance: came out of a benchmarking effort on whether a pre-PR review skill could beat
/code-review— it couldn't add much autonomously, but the investigation showed that feeding the reviewer goodAGENTS.md/CLAUDE.mdcontext is the higher-leverage move. This PR is that follow-through.