Skip to content

Add REVIEW.md guidelines for AI PR reviewers #3774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
codchen merged 2 commits into from
Jul 20, 2026
+63 −0
Merged

Add REVIEW.md guidelines for AI PR reviewers #3774

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
877e48e
Add REVIEW.md guidelines for AI PR reviewers
codchen Jul 17, 2026
eef6c9e
Merge branch 'main' into claude/review-md-ai-guidelines-c4866d
codchen Jul 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions REVIEW.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Review guidelines for AI agents

Repo-specific conventions for automated PR review (Codex, Cursor, Claude, and
any other AI reviewer). The two patterns below look like bugs in isolation
but are normal consequences of how Sei cuts releases and rolls out upgrades.
Do not report them as findings unless a concrete additional signal (see the
caveats under each) makes them real.

## 1. Referencing a not-yet-created release tag is expected

Sei only creates a new release tag — and registers it in `app/tags` with a
handler in `app/upgrades.go` — *after* the code that depends on it has
already been merged to main, never before. So it is completely normal for a
PR to introduce a version-gated constant (e.g. `const FooUpgrade = "v6.7"`)
that:

- does not yet appear anywhere in `app/tags`,
- has no corresponding upgrade handler in `app/upgrades.go`, and
- therefore cannot currently be produced by `ctx.ClosestUpgradeName()` /
`ctx.LatestUpgrade()`.

Check warning on line 20 in REVIEW.md

View check run for this annotation

Claude / Claude Code Review

REVIEW.md references non-existent ctx.LatestUpgrade() method 

REVIEW.md line 20 says an unregistered upgrade tag "cannot currently be produced by `ctx.ClosestUpgradeName()` / `ctx.LatestUpgrade()`," but `ctx.LatestUpgrade()` isn't a real symbol — `LatestUpgrade` is a package-level var in the `app` package (app/upgrades.go:22), never a method on `ctx`. Since this doc is injected verbatim into AI reviewer prompts, the wrong symbol could send automated reviewers hunting for a method that doesn't exist. Fix: reference `app.LatestUpgrade` (no parens, not a ctx

@seidroid seidroid Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[nit] Minor accuracy nit: ctx.LatestUpgrade() isn't a real symbol. In this tree ClosestUpgradeName() is a Context method (sei-cosmos/types/context.go), but LatestUpgrade is a package-level variable referenced as app.LatestUpgrade (app/upgrades.go:22), not a method on ctx. Consider app.LatestUpgrade to avoid pointing reviewers at a non-existent method.

@claude claude Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 REVIEW.md line 20 says an unregistered upgrade tag "cannot currently be produced by ctx.ClosestUpgradeName() / ctx.LatestUpgrade()," but ctx.LatestUpgrade() isn't a real symbol — LatestUpgrade is a package-level var in the app package (app/upgrades.go:22), never a method on ctx. Since this doc is injected verbatim into AI reviewer prompts, the wrong symbol could send automated reviewers hunting for a method that doesn't exist. Fix: reference app.LatestUpgrade (no parens, not a ctx method).

Extended reasoning...

The bug: REVIEW.md §1 (line 20) states that an unregistered upgrade tag "cannot currently be produced by ctx.ClosestUpgradeName() / ctx.LatestUpgrade()." The first symbol is real: ClosestUpgradeName() is a genuine method on sdk.Context, defined at sei-cosmos/types/context.go:598 (func (c Context) ClosestUpgradeName() string). The second symbol, ctx.LatestUpgrade(), does not exist anywhere in the tree — there is no method by that name on Context or any other type.

What LatestUpgrade actually is: It's a package-level variable in the app package, declared at app/upgrades.go:22 (var LatestUpgrade string) and assigned at app/upgrades.go:30 (LatestUpgrade = upgradesList[len(upgradesList)-1]). Every consumer treats it as a bare package variable, not a method call: app/app.go:880-881 uses it directly as LatestUpgrade (same package), and app/app.go:2732, 2741, and 2778 use it the same way inside methods on *App. External packages read it as app.LatestUpgrade (e.g. evmrpc/tests/tracers_test.go:58). At no point is it invoked as ctx.LatestUpgrade().

Why this matters despite being doc-only: REVIEW.md's entire purpose (per the PR description) is to be injected verbatim into Codex/Cursor/Claude review prompts via the sei-protocol/uci ai-review.yml workflow. A reviewer (human or AI) reading this line could reasonably go looking for a LatestUpgrade() method on Context to understand the guidance, not find one, and either get confused about the actual mechanism or waste time searching. It doesn't break any build or test since it's prose in a markdown file, so there's no runtime impact — this is purely a documentation-accuracy issue.

Fix: Replace ctx.LatestUpgrade() with app.LatestUpgrade (no parens, since it's a variable, not a method) to match how it's actually referenced in the codebase.

Step-by-step proof:

  1. grep -n "LatestUpgrade" app/upgrades.go → line 22: var LatestUpgrade string (a package-level var, not inside any struct/method).
  2. grep -n "LatestUpgrade" app/app.go → lines 880, 881, 2732, 2741, 2778 all reference it as a bare identifier (LatestUpgrade), never as ctx.LatestUpgrade() or any receiver call.
  3. grep -n "ClosestUpgradeName" sei-cosmos/types/context.go → line 598: func (c Context) ClosestUpgradeName() string, confirming this one genuinely is a Context method, in contrast to LatestUpgrade.
  4. Searching the entire tree for a method definition matching func (\*?\w+) LatestUpgrade\(\) returns nothing — no such method has ever been defined.

This exact inaccuracy was also independently flagged by seidroid[bot] as an inline nit on REVIEW.md:20, corroborating the finding.

🔬 also observed by seidroid-inline


Do **not** flag this as "the tag/handler is never registered," "this branch
is permanently unreachable/dead," or "this constant doesn't match anything
in the tree." The tag is cut and the handler wired up in a follow-up step
after this PR lands, using the same version string, following the existing
`app/tags` naming convention.

This only becomes a real finding when:
- the PR description or diff explicitly claims *this* PR adds the tag/handler
(i.e. it's the release-cut PR) and it doesn't, or
- the referenced version string doesn't follow Sei's tag naming convention
(compare against the existing entries in `app/tags`).

## 2. Version-gated logic and block/state sync: don't assume cross-version execution

Do not flag scenarios along the lines of "new code could process a
pre-upgrade height and diverge from what was originally committed," or
"this upgrade gate can never activate because the upgrade hasn't run yet,"
as correctness bugs — unless the diff itself has a concrete logic bug in the
gate (e.g. an inverted comparison, wrong field, off-by-one on the height).

Operationally, a given binary is never used to execute or re-execute a
height range that predates its own earliest registered upgrade. Block/state
sync always proceeds version-by-version: e.g. if a node's target height
spans releases v6.0 and v6.1, the node first syncs with the v6.0 binary up
to v6.1's upgrade height, halts, switches to the v6.1 binary, and continues
syncing from there. New code never processes old, not-yet-upgraded state.

Consequences for review:
- Height/upgrade-name gates (`ctx.ClosestUpgradeName()`, `ctx.IsTracing()`,
semver comparisons against an upgrade constant, etc.) do not need extra
defenses against "a newer binary ran against pre-upgrade state" — that
situation does not occur in Sei's deployment model.
- By the time a binary is live (or tracing/replaying) at or after its own
upgrade height, that upgrade's handler has necessarily already applied on
that node, so upgrade-gated branches are reachable for those blocks. Don't
Comment on lines +38 to +56

@claude claude Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 REVIEW.md §2's consequences bullet says ctx.IsTracing() gates "do not need extra defenses against 'a newer binary ran against pre-upgrade state' — that situation does not occur in Sei's deployment model." That's backwards for the exact gate named: ctx.IsTracing() is only ever true on the RPC/trace read path (see app/app.go's RPCContextProvider/SnapshotAwareRPCContextProvider), which exists precisely so the newest binary can replay a historical, pre-upgrade height — and the many if !ctx.IsTracing() {...} else { switch semver.Compare(ctx.ClosestUpgradeName(), ...) } branches in x/evm/keeper/params.go (and sei-wasmd/x/wasm/keeper/keeper.go:1159) are exactly the "extra defenses" the doc claims aren't needed. Suggest dropping ctx.IsTracing() from that bullet or explicitly carving out the trace/RPC read path.

Extended reasoning...

The bug: REVIEW.md §2 conflates two distinct scenarios — (a) a syncing binary re-executing/committing a pre-upgrade height with post-upgrade logic (the consensus/commit path, which genuinely cannot happen because sync halts and swaps binaries at each upgrade height), and (b) the newest binary being asked, via a read-only RPC/trace query, to reproduce results as of a historical pre-upgrade height (the tracing/eth_call-at-past-block path, which happens constantly and is a first-class feature). The doc's "Consequences for review" bullet generalizes the accurate claim about (a) into a blanket statement about (b) as well, and does so by naming ctx.IsTracing() specifically — the one gate whose entire reason for existing is scenario (b).

Where this shows up in the tree: app/app.go's RPCContextProvider and SnapshotAwareRPCContextProvider take a historical height i, call app.UpgradeKeeper.GetClosestUpgrade(checkCtx, i) to find whichever upgrade was active at that old height, and build a context via ctx.WithClosestUpgradeName(closestUpgrade).WithTraceMode(true). That is, by construction, "a newer binary evaluating pre-upgrade state." Downstream, essentially every getter in x/evm/keeper/params.go (GetPriorityNormalizer, GetBaseFeePerGas, GetMaxDynamicBaseFeeUpwardAdjustment, GetMinimumFeePerGas, etc.) branches on !ctx.IsTracing() vs. tracing-with-a-semver-check against ctx.ClosestUpgradeName(), returning the pre-vX parameter set on the tracing branch. sei-wasmd/x/wasm/keeper/keeper.go:1159 does the same for gas metering. These branches are not incidental — they are the mechanism by which historical trace/RPC results stay correct, and they are exactly what the doc tells reviewers not to expect.

Why this matters (and where I think it stops short of "normal"): A verifier refutation on this bug points out that §2 already carves out "a concrete logic bug in the gate (e.g. an inverted comparison, wrong field, off-by-one on the height)" and "still report it" for a gate "genuinely broken on its own logic." That carve-out is real and does cover the case where an existing IsTracing/semver branch has an inverted comparison or wrong constant. But it doesn't cleanly cover the failure mode this bug is actually worried about: a future PR that adds a brand-new EVM/wasm param and simply omits the IsTracing/semver branch entirely. There's no existing gate to inspect for "wrong field" or "wrong constant" — the defense is just absent — and the sentence a reviewer is primed with is "that situation does not occur," which argues against even looking for a missing branch. So the carve-out narrows but doesn't fully close the gap the refutation claims closes it.

Step-by-step illustration:

  1. A future PR adds GetFooBarParam(ctx) to x/evm/keeper/params.go for a new EVM param, but forgets the if !ctx.IsTracing() {...} else {...} pre-upgrade branch that every sibling getter has.
  2. Someone calls eth_call/debug_traceCall at a block height before the upgrade that introduced FooBar. RPCContextProvider resolves ClosestUpgradeName for that historical height and sets WithTraceMode(true).
  3. GetFooBarParam ignores tracing mode entirely and returns the current param value instead of a pre-upgrade default (zero value, or whatever existed before), producing an observably wrong historical trace/eth_call result.
  4. An AI reviewer checking this diff against REVIEW.md §2 reads "Height/upgrade-name gates ... do not need extra defenses against 'a newer binary ran against pre-upgrade state' — that situation does not occur," concludes there's nothing to flag, and moves on — because there's no existing "gate" to find broken, just a missing one.

Suggested fix: Either drop ctx.IsTracing() from the "do not need extra defenses" bullet, or add an explicit carve-out stating that the trace/RPC read path is the one case where "newer binary against pre-upgrade state" is real and expected, and that reviewers should still check whether a new tracing-sensitive param/behavior has the corresponding historical branch. The §1 and block/state-sync claims are otherwise accurate; this is a narrow but real overreach in restating them for the IsTracing() gate.

Severity note: This is a documentation-only change with no runtime behavior, so nothing breaks by merging as-is — hence nit rather than normal. The risk is indirect (degraded future AI-review coverage of a real regression surface), not immediate.

call them "permanently unreachable" solely because the tag/handler isn't
registered yet at PR-review time — see §1.

If you believe a version gate is genuinely broken on its own logic (wrong
comparison direction, wrong constant, wrong context field), still report
it — this guidance only rules out the "the tag doesn't exist yet" and
"old code might run against post-upgrade state" false positives.

Check warning on line 63 in REVIEW.md

View check run for this annotation

Claude / Claude Code Review

REVIEW.md closing sentence reverses the §2 scenario it's summarizing 

REVIEW.md's closing sentence (lines 62-63) summarizes §2 as ruling out the false positive "old code might run against post-upgrade state," but §2's body (lines 36-58) only ever discusses the opposite direction — new/newer code processing pre-upgrade (old) state. The two are distinct scenarios, not paraphrases, so a reviewer skimming just the recap could misapply the exemption; suggest flipping the closing sentence to match the body (e.g. "new code might run against pre-upgrade state").
Comment on lines +60 to +63

@claude claude Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 REVIEW.md's closing sentence (lines 62-63) summarizes §2 as ruling out the false positive "old code might run against post-upgrade state," but §2's body (lines 36-58) only ever discusses the opposite direction — new/newer code processing pre-upgrade (old) state. The two are distinct scenarios, not paraphrases, so a reviewer skimming just the recap could misapply the exemption; suggest flipping the closing sentence to match the body (e.g. "new code might run against pre-upgrade state").

Extended reasoning...

§2 of REVIEW.md is internally consistent in its body but its one-line closing recap describes the reverse scenario. Every concrete example and piece of reasoning in the body frames the exempted false positive as newer code processing older (pre-upgrade) state: "new code could process a pre-upgrade height and diverge from what was originally committed" (line 36-37), "New code never processes old, not-yet-upgraded state" (line 47), and "a newer binary ran against pre-upgrade state" (line 52). The closing sentence on lines 62-63, however, names the ruled-out pattern as "old code might run against post-upgrade state" — which flips both axes at once (old vs. new code, and post- vs. pre-upgrade state) into a scenario the body never states, illustrates, or reasons about anywhere.

These are not two phrasings of the same idea. Building out the 2x2 of {code age} x {state age relative to that code}, "new code + pre-upgrade state" and "old code + post-upgrade state" are the two distinct cross-version combinations, and they correspond to different real-world code paths (e.g. a newer binary tracing/replaying a historical height vs. a stale binary somehow still being live past its own upgrade boundary). §2 builds its entire justification — the halt-and-swap sync model — around ruling out the first; it never argues that the second cannot happen, even though the closing line claims the document does so.

A refutation raised during verification argues this is not a real contradiction: the halt-and-swap deployment model described in §2 (a node syncs on binary vN up to the vN+1 upgrade height, halts, and swaps to vN+1) happens to rule out both directions simultaneously, since an old binary would also halt before reaching post-upgrade heights. That may be true as a factual matter about the deployment model, but it does not rescue the sentence as an accurate recap. The document explicitly reasons through and gives examples for exactly one direction; it never establishes, argues, or illustrates the other. A summary sentence that says "this guidance rules out X" when the surrounding text never derived X is misleading regardless of whether X happens to also be true for unstated reasons — especially since this file is injected verbatim into AI review prompts (per the PR description) specifically so reviewers can quote and rely on it as authoritative reasoning, not just a true-but-unexplained conclusion.

Concrete walk-through of the mismatch:

  1. Body claim (line 36-37): a new/upgraded binary might process a pre-upgrade (older) height and get a different result than what was originally committed. This is the scenario the whole halt-and-swap argument is built to rule out.
  2. Consequences bullet (line 52): explicitly names this as "a newer binary ran against pre-upgrade state" — new code, old state.
  3. Closing recap (lines 62-63): names the ruled-out pattern as "old code might run against post-upgrade state" — old code, new state. This is the mirror-image scenario of Node system requirements #1/Create .gitpod.yml #2, not a restatement of it.
  4. Nowhere between lines 36 and 58 does the document discuss, e.g., a stale/older binary somehow still executing blocks after a newer upgrade has already activated — so the closing line cites a "false positive it rules out" that was never demonstrated.

Impact: Since this is a docs-only file whose entire purpose is to be read (and quoted) by AI reviewers, the risk is confined to a reviewer skimming just the closing line and misapplying the exemption to the wrong directional scenario, or being confused about which pattern is actually covered. The detailed body of §2 remains correct and would set a careful reader straight, which is why this is a clarity/wording issue rather than a functional defect — no code path is affected.

Suggested fix: change line 63 to match the direction actually argued in the body, e.g. "this guidance only rules out the "the tag doesn't exist yet" and "new code might run against pre-upgrade state" false positives" (swapping "old"→"new" and "post"→"pre").

Loading