Add REVIEW.md guidelines for AI PR reviewers

[codchen]

Summary

• Adds REVIEW.md at the repo root, which the sei-protocol/uci reusable ai-review.yml workflow automatically loads (from the PR's base branch) and injects into the Codex/Cursor/Claude review prompts — no workflow change needed.
• Codifies two patterns that AI reviewers otherwise flag as bugs but are normal here, prompted by this false-positive comment:
  1. Hardcoded future release tags — Sei creates a new release tag (and its app/upgrades.go handler) after the referencing code merges, not before, so an unregistered future tag string is expected, not dead code.
  2. Version-gated logic across block/state sync — a binary never executes a height that predates its own upgrade activation, since sync halts and swaps binaries at each upgrade height. Reviewers shouldn't flag hypothetical "new code runs against pre-upgrade state" divergence.
• Both sections carve out when the finding is still real (e.g. the PR itself claims to add the missing tag, or the gate's comparison logic is actually inverted), so genuine bugs aren't suppressed.

Test plan

• Confirm the AI review bots (Codex/Cursor/Claude) pick up REVIEW.md on the next PR and stop flagging these two patterns.

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Jul 18, 2026, 5:12 AM

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.03%. Comparing base (7fee5c2) to head (eef6c9e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3774      +/-   ##
==========================================
- Coverage   60.14%   59.03%   -1.12%     
==========================================
  Files        2303     2201     -102     
  Lines      192032   180281   -11751     
==========================================
- Hits       115491   106422    -9069     
+ Misses      66230    64505    -1725     
+ Partials    10311     9354     -957     

Flag	Coverage Δ
sei-db	70.41% <ø> (ø)
sei-db-state-db	?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 138 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cursor]

PR Summary

Low Risk
Documentation-only change with no runtime, auth, or data-path impact.

Overview
Adds a root-level REVIEW.md so automated reviewers (Codex, Cursor, Claude, etc.) can load repo-specific rules via the existing sei-protocol/uci ai-review.yml workflow—no workflow changes.

The doc tells agents not to treat two Sei-specific patterns as bugs by default: (1) PRs that reference a future release tag before it exists in app/tags / app/upgrades.go, and (2) hypothetical cross-version sync issues where new code would run against pre-upgrade state. Each section lists when those findings are still valid (e.g. release-cut PR missing the handler, bad tag naming, or a broken gate comparison).

^{Reviewed by Cursor Bugbot for commit eef6c9e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[seidroid]

Docs-only PR adding a repo-root REVIEW.md that codifies two legitimate Sei review conventions (future release tags, version-gated logic under block/state sync), each with carve-outs that preserve genuine bug reporting. No code changes; content is accurate apart from one minor symbol reference, and I disagree with Codex's characterization of this as a blocking prompt-injection.

Findings: 0 blocking | 5 non-blocking | 1 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Re: Codex's "High — Prompt injection" finding. I disagree that this is a blocker. REVIEW.md is a deliberate, maintainer-authored governance file added openly (author Tony Chen, reviewer masih approved) — it is the repo's intended mechanism for supplying reviewer guidance (analogous to REVIEW_GUIDELINES.md loaded by the sei-protocol/uci workflow), not untrusted content covertly trying to alter a verdict. Both sections include explicit "this only becomes a real finding when…" carve-outs so genuine bugs (inverted gates, wrong constants, the release-cut PR itself omitting the tag) are still reported. It does not instruct reviewers to approve any specific PR or suppress arbitrary findings. Worth keeping only as a note, not a merge blocker.
• Residual governance consideration: because REVIEW.md is auto-injected from the base branch into every future AI review, it is a broad lever whose safety rests entirely on the carve-outs remaining intact. Future edits that broaden these suppressions (or add new ones) without equivalent "still report it when…" guards should be scrutinized more carefully than a normal docs change.
• Cursor's second-opinion pass (cursor-review.md) produced no output (empty file); only Codex's pass returned a finding.
• Optional: the file is a natural place to link back to the existing AGENTS.md/REVIEW_GUIDELINES.md review-guidance so the two AI-guidance surfaces stay discoverable and don't drift.
• 1 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[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]

LGTM — documentation-only change (adds REVIEW.md with two review-guidance sections), no runtime impact. One nit was flagged inline about §2 overgeneralizing to ctx.IsTracing(), but it does not undermine the PR overall.

Extended reasoning...

Overview

This PR adds a single new file, REVIEW.md, at the repo root. It contains no code changes — only prose guidance for AI PR reviewers about two Sei-specific patterns (future release tags not yet registered in app/tags/app/upgrades.go, and version-gated logic under Sei's sync model) that should not be flagged as bugs by default.

Security risks

None. This is a documentation file with no execution path, no secrets, and no interaction with auth/crypto/permission code.

Level of scrutiny

Low. The blast radius of a wrong or imprecise sentence in a reviewer-guidance doc is "a future AI review is slightly miscalibrated," not a runtime or consensus bug. I verified the one nit finding against the actual code (x/evm/keeper/params.go's GetPriorityNormalizer/GetBaseFeePerGas/etc. and app/app.go's RPCContextProvider/SnapshotAwareRPCContextProvider) and confirmed ctx.IsTracing() gates genuinely do defend against exactly the "newer binary evaluates pre-upgrade state" scenario during historical trace/eth_call replay, so REVIEW.md's §2 bullet listing ctx.IsTracing() as not needing such defenses is inaccurate. That said, it is a nit on a docs file guiding future reviews, not a functional defect in shipped code, and does not by itself warrant blocking this PR.

Other factors

Single-file diff, no tests applicable (docs), CI (buf/codecov) passed. No CODEOWNERS restriction found on REVIEW.md. No outstanding unaddressed reviewer threads beyond the one nit already captured as an inline comment.

[claude]

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

[claude]

LGTM — this is a docs-only addition to REVIEW.md with no runtime impact. The two findings from this run are wording nits in the new guidance doc (a mismatched summary sentence and a made-up symbol name), not functional bugs, so they don't block merging.

Extended reasoning...

Overview

The PR adds a single new file, REVIEW.md, at the repo root. It contains no code changes — it's a prose guidance document meant to be injected into AI PR-review prompts via the sei-protocol/uci ai-review.yml workflow, carving out two false-positive patterns (unregistered future upgrade tags, and version-gated logic assumptions across sync) that AI reviewers should not flag by default.

Security risks

None. This is a markdown documentation file with zero effect on any binary, build, or runtime behavior. It cannot introduce a vulnerability by itself; at most it could cause a future AI reviewer to under- or over-flag something, which is a review-quality concern rather than a security one.

Level of scrutiny

Low. Docs-only PRs with no code-path impact warrant light scrutiny — the main risk is that the guidance itself is unclear or self-contradictory, which is exactly what this run's findings target (a closing sentence that describes the reverse of what its section argued, and a reference to a non-existent ctx.LatestUpgrade() method that should be app.LatestUpgrade). Both are nits confined to prose accuracy/clarity in the new file; neither affects any executable code, and the doc's substantive guidance (the actual exemption logic in §1 and §2) remains sound.

Other factors

CI (Buf, Codecov) is green, and the previous review pass's IsTracing() concern from an earlier commit-instance of this PR was already surfaced as an inline comment. Since only wording nits were found this run and the change carries no execution risk, I'm comfortable approving without requiring further human sign-off.

[claude]

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

[claude]

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