test(config): pin the boot-path config values (PLT-851) by bdchatham · Pull Request #3890 · sei-protocol/sei-chain

bdchatham · 2026-08-10T21:19:57Z

Motivation

PLT-775 replaces the legacy configuration path, and that replacement can only be shown to be faithful against a written record of how the current path resolves each key. This continues that record, covering the reads that happen while a node boots. None of it needs a running node: app.New installs process-global singletons, so a test binary could only boot once anyway, and the boot command binds its whole flag set into viper before those reads happen, so the resolution is reachable directly.

Test-only. No production files change.

Test Specs & Approach

Characterization, not validation. Each test records what the reader does today, including behavior nobody would choose on purpose. Where a value resolves surprisingly, the test asserts the surprise and the comment says why it is recorded rather than repaired. Changing a reader then fails a test with the old and new value in the diff, which is the review prompt.

Every assertion is checked by breaking the reader. For each new test, I changed the production code so the test should fail, ran it, and confirmed it did. Twelve such checks back this PR. That is the difference between a test that passes and a test that can fail, and an earlier pass over this suite found seven that could not fail at all.

Held against something independent. An assertion whose expected value comes from the same place as its actual value holds for any reader, so predictions are compared against a checked-in file, the reader's own empty-input result, or a declared default, never against a second call of the thing under test.

New Test Coverage

Keys only the boot path reads. Three keys resolve inside the unexported function that launches a node. Their resolution is now driven directly and their names recorded in a golden, so renaming one fails here.

One key, two live readers. min-retain-blocks is read twice with different casts: cast.ToUint64 into Tendermint's block retention, and cast.ToInt into the EVM receipt store's retention window. One number an operator sets for block retention therefore also sets receipt retention. The two casts disagree on out-of-range input and agree in effect only because both land on keep-everything, which is what this pins, along with archive mode being aligned on both sides.

A documented value that cannot boot. minimum-gas-prices is parsed by sdk.ParseDecCoins, which separates denominations with a comma and panics otherwise, and that panic is the whole boot. The flag's own help text offers a semicolon-separated example, and Config.GetMinGasPrices splits on a semicolon. The two syntaxes are disjoint, so no multi-denomination value satisfies both and the spelling an operator is shown is the one that panics. Single denominations agree, which is why this has stayed quiet.

Validation that only reports. ValidateBasic rejects an empty fee floor, and rejects pruning everything alongside state-sync snapshots. Its only caller logs the error and builds the app anyway, and the message it logs names only the fee floor, so an operator whose pruning and snapshot settings conflict is told their fee floor is empty.

Assertions that could not distinguish. telemetry.global-labels was compared by how many labels survived, which passes equally with a pair reversed; it now compares the pairs in written order. This also records the one malformed shape the reader does not report, since a two-element label holding a non-string reaches an unchecked type assertion and panics where both sibling malformed shapes return errors.

A guard on the helper every case passes through. Setting a key and a dotted child of it into viper destroys one of the two, and building that key set from a Go map lets map order pick the survivor per run. The loss is visible through the per-key reads this reader uses, so a colliding pair would resolve a wrong value on roughly half of runs and read as flaky rather than wrong. No caller collides today, and the guard sits at the single constructor they all use, including two that build their key maps dynamically where review cannot enumerate them.

Guide corrections. The eleven-key figure for keys read by two readers is per struct, so the pair is twenty-two, now stated along with why that split cannot produce a disagreement. And the claim that the manifest is what the differential enumerates is wrong and corrected: the differential compares whole resolved vipers and reads no manifest.

Verification

go test -race green on the four affected packages, golangci-lint reports 0 issues, gofmt -s and goimports clean, and the changed package is green under -shuffle=on -count=3.

cpu-profile and trace-store appeared in no test and no record anywhere in the tree, so renaming either flag moved the read site with nothing reporting it. grpc-only had one set-case assertion and no absent case. These three are the only keys startInProcess reads that no other section pins; everything else it touches is a field of the struct GetConfig produces, and that reader has a manifest. Driven through PreRunE rather than a booted node, because that is where the values arrive. start.go registers all three as flags and PreRunE binds the flag set into the viper Apply populated, so startInProcess re-reads that same viper rather than resolving anything for the first time. Both directions per key. The absent case matters most: start.go compares cpu-profile and trace-store against the empty string to decide whether to profile or trace at all, so a non-empty resolution from an absent flag starts profiling or writes a trace file on a node whose operator never asked for either. The file header claimed these keys were unreachable without an integration harness. That was true of their effects and false of their resolution, and it is the header that sent the ticket looking for a node harness, so it now draws the line between resolution and effect instead. Effects stay unpinned, deliberately and with the reason recorded. The differential the cutover rests on compares the two resolved channels after Apply, so identical resolution means startInProcess reads identical values and its effects follow. Also recorded that the inprocess package is not the tool: it calls tmnode.New directly with its own AppOptions, so it never executes startInProcess or the legacy resolver, and app.New's process-global singletons cap it at one boot per test binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

They had assertions and no record, unlike every other key in the suite. A KeyName rather than a KeySpec, because none of them resolves into a config struct a row could name a Path in: startInProcess reads them off the viper and branches. The record puts the operator-facing spelling in a checked-in file, so a rename lands in a diff rather than only reddening the assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… that means The manifest exempts PruningKeepEvery on the grounds that GetConfig does not read pruning-keep-every. Nothing held that. GetConfig could have started reading the key and the exemption would have become a false claim about a field that now resolves, with no test reporting it. The comment justifying the exemption was also misleading in a way that matters to the thing it is written for. It called the field the shape of thing a replacement manager would try to map a key onto, which reads as nothing maps a key onto it. server/pruning.go reads pruning-keep-every through appOpts and feeds the custom pruning strategy, pinned by pruning_test.go and pruning_fuzz_test.go. So the key has a reader and a consumer; what it lacks is a path through this struct. A v2 manager has to carry the key and must not expect this field to be where it lands. Writing the assertion turned up what the field actually carries. GetConfig builds BaseConfig as a struct literal and never assigns PruningKeepEvery, so it holds Go's zero value, the empty string, where DefaultConfig declares "0". That is a divergence as well as an omission, and an empty string is not a number the custom strategy can use. My first version of this test asserted the field equalled the declared default and failed, which is how the behaviour surfaced. Both halves are asserted: that this reader leaves the field empty whatever the key says, and that the declared default still differs from empty, so closing the divergence from either side lands in a diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The row records that an absent concurrency-workers resolves 0 through GetConfig against a declared 20. True, and on its own it reads as though a node runs with no workers. baseapp.New reads the same key and substitutes DefaultConcurrencyWorkers whenever it resolves 0 (baseapp.go:316-320), so the executing value is 20. Both sides were already pinned independently: GetConfig's 0 by this table, and the rescue by baseapp/config_fuzz_test.go, which covers the explicit-zero case too. What was missing was the cross-reference, so the row now says what it records and what it does not. This is the same shape as the state-sync row, where a bound flag default rescues the clobber, and it is worth expecting more of: a split can be covered from both ends and still mislead if neither end names the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A census of keys read by more than one first-party reader found 22 in state-commit and state-store alone, but the triage that matters is which splits have a live loser. Almost none do. GetConfig parses [state-commit] and [state-store] into a Config nobody hands to the store, so a disagreement there cannot reach a node, and the suite's own header already said so. min-retain-blocks is the exception. Both readers run. cmd/seid/cmd/root.go casts it with ToUint64 into baseapp's block-retention height, and app/receipt_store_config.go casts it with ToInt into the receipt store's KeepRecent. One number an operator sets for Tendermint block retention silently also sets EVM receipt retention, through two different casts. The casts diverge on operator-reachable input, measured: a negative leaves ToInt holding it while ToUint64 floors to zero, and a value past int64 floors ToInt to zero while ToUint64 keeps it. So the fan-out is a coupling for every sensible value and a divergence at the edges. The invariant the divergence rests on is that the receipt side never becomes a positive retention window. Zero or below, and the receipt store's KeepRecent>0 guard leaves receipts alone. That is what is asserted, and switching the receipt cast to ToUint64 now reddens. Two wrong versions of that assertion came first and both taught me the shape. I asserted the block side must be zero, which failed on the two past-int64 seeds because ToUint64 keeps a huge number there, harmless for a different reason: it retains more blocks than a chain reaches. Then I asserted the receipt side must be exactly zero, which failed on the negative seed. The comment now records why neither is the invariant. Also recorded that archive is aligned. PLT-955 is an archive node losing state history because its mode's state-store settings are discarded, so the fan-out was worth checking as a second history-loss path. min-retain-blocks of 0 is keep-all for blocks and leaves KeepRecent at 0, which is no pruning, so it is not one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

One reader governs a node: root.go:296 hands the key to baseapp.SetMinGasPrices, which panics unless sdk.ParseDecCoins accepts it, and ParseDecCoins separates denominations with a comma. Two artifacts document a semicolon instead, the start flag's own help text and Config.GetMinGasPrices. The syntaxes are disjoint, so no multi-denomination value satisfies both and the spelling operators are shown is the one that panics. Single-denomination values agree, which is why this has stayed quiet. Also corrects the concurrency-workers rescue note. That key is a bound start flag, so the boot path takes the flag default from GetConfig and never resolves 0; baseapp's substitution is a second net behind it, not the mechanism a node relies on. The old note also named a machine-specific 20 for a NumCPU-derived default.

start.go:308 is the only caller and it logs the error before building the app, so neither condition stops a node. The message it logs is a fixed string naming an empty minimum-gas-prices, one of the two causes, so an operator whose pruning and snapshot settings conflict is told their fee floor is empty and boots into a node that cannot serve the snapshots it advertises. The header previously claimed both conditions were refused rather than absorbed. Adds the assertion that keeps the two error strings distinguishable, since the caller's fixed message means they are the only diagnosis available.

The count in that paragraph is per struct, so a reader could take eleven as the node-boot total when StateStoreConfig splits the same way with eleven of its own. Records the pair as twenty-two, and records why the split is benign: both readers read the same viper through the same cast helpers, no state-commit or state-store key is a registered flag, and GetConfig populates both structs with no caller reading either field.

FuzzGetConfigGlobalLabels compared only how many labels survived, which passes just as well with a pair reversed or with one label's key against another's value. It now compares the resolved pairs in written order. Adds the one malformed [telemetry] shape the reader does not report. A global-labels value that is not a list, and an entry that is not a list, both return an error naming what failed; a two-element entry holding a non-string reaches an unchecked assertion at config.go:432, so global-labels = [["chain", 42]] takes a node down with an interface-conversion panic instead. Also corrects the guide's claim that the manifest is what the differential enumerates. The differential compares whole resolved vipers and reads no manifest, so it can report that two readers agree without either being pinned.

newAppViper ranges a Go map into viper.Set. Set re-nests around the dots, so a key and a dotted child of it cannot both survive, and ranging a map picks the survivor at random per run. The loss is visible through the per-key Get that GetConfig uses rather than only through AllSettings, so a colliding pair would resolve a wrong value on about half of all runs and read as flaky instead of wrong. No current call site collides. This guards the constructor every call site passes through, including the two that build their key maps dynamically, where review cannot enumerate the keys. Nesting is a dotted-segment relationship and never a textual one: pruning and pruning-keep-every are separate keys, as are state-commit.sc-write-mode and its -enable-auto sibling. The predicate carries its own test because that boundary is the part a later edit would get wrong.

github-actions · 2026-08-10T21:22:18Z

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

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`✅ passed`	`✅ passed`	`✅ passed`	Aug 12, 2026, 2:12 PM

codecov · 2026-08-10T21:31:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.31%. Comparing base (99c9c29) to head (3f70e6a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3890      +/-   ##
==========================================
- Coverage   59.47%   58.31%   -1.16%     
==========================================
  Files        2323     2225      -98     
  Lines      198389   186625   -11764     
==========================================
- Hits       117982   108835    -9147     
+ Misses      69198    67508    -1690     
+ Partials    11209    10282     -927

Flag	Coverage Δ
sei-chain-pr	`54.12% <ø> (?)`
sei-db	`70.41% <ø> (-0.22%)`	⬇️
sei-db-state-db	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.
see 174 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.

The disagreement branch could not execute. The fuzz target pinned KeepRecent to cast.ToInt(raw) and then recomputed the block side as cast.ToUint64(raw), and whenever ToInt yields a positive the value is a positive int64 that ToUint64 parses identically, so the agree-and-return path always won first. Measured: zero reachings across 400,001 integers plus every boundary and format spelling. The same recomputation meant the block side was asserted against a second call to the function under discussion, which holds for any reader. So the file claimed to pin root.go:297 and did not. Splits the two halves by what each is worth. The fuzz target keeps the receipt half, which is a real pin on app/receipt_store_config.go. A table carries both columns as literals, so the receipt column is a prediction against the reader and the block column is a recording of the cast, and the safety property is now asserted over rows that can hold a counterexample. The header now states that nothing here fails if root.go:297 changes its cast, since that argument is built inline inside newApp's app.New call and needs a node. That gap was true before this branch too; what was wrong was claiming otherwise.

Each of these asserted something about production or about the harness that is not true, which in a suite whose comments are the record is the worst kind of defect. The ValidateBasic note said start.go:308 is the only caller in the tree. There is a second at testutil/network/util.go:31, and it returns the error rather than logging it. The boot-path claim the note rests on is unchanged; the scope was wrong. The start_flags record claimed a rename lands in a diff rather than only reddening the assertions. It is the reverse. server's flagCPUProfile, flagTraceStore and flagGRPCOnly are unexported, so this package cannot spell them through the reader's constant and the record holds literals. A production rename leaves the record and the assertions both carrying the old name, and is caught one step over where setting the flag fails. The comment now states that bound. Two function doc comments had fused into one block with no separator, so godoc attached both to TestGetConfigLeavesPruningKeepEveryEmpty and left TestBaseConfigManifestNamesEveryField, which carries the exemption the other one checks, with no doc at all. Split, with each leading its own function. In AGENTS.md, the state-store paragraph had been inserted between the 62-exemption count and the sentence resolving fifty-seven of those 62, so the referent had to reach past a digression introducing its own numerals. Moved below that argument. Its claim that both readers read the same viper also could not be reconciled with the preceding sentence, which establishes one reading a flat map and the other reading viper; it now says how both reach the same viper. Also drops a redundant slice conversion.

seidroid · 2026-08-10T23:27:51Z

1. Blocking

None. This PR adds only test files (*_fuzz_test.go), golden/coverage testdata, and an AGENTS.md doc update — no production code changes, and nothing on any node's execution or consensus path is touched. I verified the new code compiles against its context: mapAppOpts is map[string]interface{} (app/seidb_test.go:91), so both the raw string and the literal 0 passed to it type-check; config_fuzz_test.go already imports fmt, reflect, and strings; CheckKeyNames(t, "start_flags", nil, keys...) matches its signature (testutil/configtest/keynames.go:98); newStartCmd's three-value return matches its call site (cmd/seid/cmd/startprerun_config_fuzz_test.go:195); and go.mod is Go 1.25.6, so the for i := range labelCount integer-range form is valid.

2. Security

None found. The changes are test-only: temp dirs via t.TempDir(), no secrets, no network or external calls, no deserialization of untrusted input reaching production, and no boundary that is loosened. I looked for injection, auth bypass, exposed secrets, unsafe deserialization, path traversal, and SSRF across the changed lines and their callees; none apply.

3. Non-blocking

sei-cosmos/server/config/config_fuzz_test.go:621 — the suite documents a real latent production defect it deliberately does not fix: a [telemetry] global-labels entry with a non-string element takes node boot down with an opaque interface-conversion panic (config.go:432) instead of the parse error its siblings return; worth a tracked follow-up.
cmd/seid/cmd/mingasprices_config_fuzz_test.go:16 — similarly records a real latent issue: the start flag's help text advertises a ;-separated multi-denomination example (start.go:208) that panics the node on boot, since the live reader parses commas; deferred, but an operator-facing footgun.
cmd/seid/cmd/mingasprices_config_fuzz_test.go:248 — under active go test -fuzz (not seed-corpus CI), the assertion that every panic message contains "invalid minimum gas prices" could flag a benign discovered input whose failure surfaces from a deeper ParseDecCoin path with a different message.
app/receipt_store_config_fuzz_test.go:72 — the fuzz assertion recomputes the expected value with the same cast.ToInt(raw) the production reader uses, so it pins the cast choice and key wiring but by construction cannot detect a value mismatch; a weak pin, as the doc comment acknowledges.

4. Summary

This is a test-and-documentation-only change: a set of config-resolution characterization/fuzz suites plus golden and coverage testdata that pin existing min-retain-blocks, minimum-gas-prices, start-flag, and [telemetry]/pruning parsing behavior, and an AGENTS.md update describing the state-store split. It introduces no production, execution, or consensus code, the new tests compile and are internally self-consistent, and the only observations are pre-existing latent behaviors the suite deliberately records rather than repairs. Nothing blocks.

{"decision": "comment",
 "summary": "Test- and docs-only PR adding config-resolution characterization/fuzz suites and testdata; no production or consensus code changes. Compiles and is self-consistent; only non-blocking notes, including two real latent production issues the suite deliberately documents rather than fixes.",
 "findings": [
   {"file": "sei-cosmos/server/config/config_fuzz_test.go", "line": 621, "severity": "low", "detail": "Test documents (does not fix) a real latent defect: a [telemetry] global-labels entry with a non-string element panics node boot with an opaque interface-conversion at config.go:432 instead of returning a parse error. Worth a follow-up to convert to an error."},
   {"file": "cmd/seid/cmd/mingasprices_config_fuzz_test.go", "line": 16, "severity": "low", "detail": "Test records a real latent operator footgun: the start flag help text (start.go:208) advertises a ';'-separated minimum-gas-prices example, but the live reader parses commas and panics the node on boot for multi-denomination values. Deferred, not fixed here."},
   {"file": "cmd/seid/cmd/mingasprices_config_fuzz_test.go", "line": 248, "severity": "low", "detail": "Under active go test -fuzz (not seed-only CI), asserting every panic contains 'invalid minimum gas prices' may spuriously fail on a discovered input whose failure comes from a deeper ParseDecCoin path with a different message."},
   {"file": "app/receipt_store_config_fuzz_test.go", "line": 72, "severity": "low", "detail": "The fuzz assertion recomputes the expected KeepRecent with the same cast.ToInt(raw) the production reader uses, so it can only detect a change of cast/key wiring, not a value error; a weak pin as its own comment acknowledges."}
 ]}

_{seidroid xreview · decision comment · session 47bc8df61940486ebfcedb655e8be0f0 · turn resp_claude_658083ad3facd7e9e0832c7bdd663ce0 · item be2565acd84e4ae983510ada047d35c5}

…anic Review noted the fuzz assertion compares the reader against the same cast.ToInt it uses, so it pins the wiring rather than the value. That is the intended division and the comment now states it: the fuzz target catches a changed cast or a reader that stops reading this key, verified by pointing the reader at a different key, and the value-level pin is the literal table in the same file. Review also predicted the panic-message assertion could fail under an active fuzz run on an input whose rejection carries a different message. I could not reproduce it: every rejection reachable from a string arrives as an error ParseDecCoins returns and SetMinGasPrices wraps, including 55-digit decimals, NaN, Inf, huge exponents and non-ASCII digits. Kept the assertion, because a different message means either the rejection moved readers or a value found a path that panics inside ParseDecCoins, and both are worth surfacing. The failure message now names both readings so whoever reaches it is not sent after the wrong one.

cursor · 2026-08-10T23:35:19Z

PR Summary

Low Risk
Test-only characterization with no production code changes. Risk is limited to brittle assertions around known surprising behavior that future intentional fixes must update deliberately.

Overview
Extends the PLT-775 config record with test-only coverage of boot-path reads that resolve without launching a node.

Pins three surprising live behaviors. min-retain-blocks is read twice with different casts (ToUint64 for Tendermint blocks, ToInt for EVM receipt KeepRecent), so full-mode's default 100000 also caps receipt retention. minimum-gas-prices accepts commas at boot while its help text and dead getter document semicolons. ValidateBasic rejects empty fee floors and pruning/state-sync conflicts, but the boot path only logs a fixed fee-floor message and continues.

Closes previously unrecorded start keys. cpu-profile, trace-store, and grpc-only now resolve through PreRunE in both absent and set directions, with their spellings held in a golden.

Hardens the GetConfig harness. Surviving telemetry.global-labels pairs are compared in written order; the non-string label panic is recorded; newAppViper refuses colliding or nested keys that would flake under map order; and the PruningKeepEvery exemption is asserted as a real omission rather than a silent claim.

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

seidroid

Test-only PR that extends the configuration characterization suite; no production code changes, and the load-bearing claims I spot-checked (root.go:296/297 casts, config.go:421/429/432, start.go:117/168/208/222/224/303/308, options.go SetMinGasPrices panicking at construction, GetMinGasPrices having no production caller) are all accurate. Remaining notes are gaps between what a few new assertions claim to pin and what they can actually detect, plus two comment-accuracy nits.

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

Blockers

None at the file/PR level.

Non-blocking

Cursor's second-opinion pass produced no output (cursor-review.md is empty), so this synthesis merges only Claude's and Codex's findings.
Codex's P1 (block-retention half not exercising root.go's live reader) is real but already addressed in the final revision: the file header explicitly states the block column is a recorded literal and that nothing fails if root.go:297 changes its cast. Keeping the note only to record that the fan-out is half-pinned by design. One cheap partial pin is available and not taken — min-retain-blocks is registered as a Uint64 flag (start.go:222), so pinning the flag's registered type would at least catch the block side changing shape underneath the recording.
The suite leans heavily on line-number citations in comments (root.go:296/297, config.go:421/429/432, baseapp.go:316-320, abci.go:758, litt_receipt_store.go:138, start.go:117/168/208/224/303/308, options.go:24-28). Every one I checked is correct today, but nothing fails when a line moves, so they decay silently. Consistent with the existing suite's convention, so noted rather than asked for.
TestMinGasPricesFlagHelpShowsASeparatorTheLiveReaderPanicsOn is the only new test in cmd/seid/cmd that does not call configtest.Isolate(t). server.StartCmd reads nothing at construction so it is currently harmless, but the inconsistency invites the question.
5 suggestion(s)/nit(s) flagged inline on specific lines.

Review found three places where the comment asserted more than the code checked, which is the same class as the dead branch fixed earlier in this branch. The archive test named archive mode and never touched it. It wrote 0 into the reader and asserted 0 came back, so it pinned cast.ToInt(0) and nothing about the mode. Moving archive off keep-all left it green, which is the regression it exists to notice. It now derives the value through params.SetAppConfigByMode and states the keep-all expectation as its own assertion. The ValidateBasic distinctness check asserted only that the pruning rejection does not read as a fee-floor problem, which any error at all satisfies, including one naming neither cause. It now also asserts the error names the snapshot conflict, which is what the comment claims it carries. The GetMinGasPrices helper collapsed panicking and reading the wrong number of denominations into one false, so the panic the comment records was invisible to the assertion. It now reports the count and the panic separately. Also corrects two things in the ValidateBasic comment: the second caller is under sei-cosmos and a root-level testutil/network exists too, so the unqualified path pointed at the wrong tree, and the parenthetical had been spliced mid-sentence. And drops a tautological assertion. With the flag default already established as empty, comparing it to DefaultMinGasPrices only asked whether the declared default is empty, and would have reported it as two values colliding.

seidroid

Test-only PR that extends the configuration characterization suite over the boot path; every behavioral claim I spot-checked against production code (root.go's two casts, the start-flag registrations, GetMinGasPrices' semicolon split, the unchecked labelsRaw[0].(string) assertion, parseSSConfigs vs GetConfig on [state-store]) is accurate. No blockers — findings are a case-sensitivity gap in the new viper-collision guard, one unstated pin gap, and small consistency/doc nits.

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

Blockers

None at the file/PR level.

Non-blocking

Cursor's second-opinion pass (cursor-review.md) is empty — that reviewer produced no output, so this review is Claude + Codex only. Codex reported no material issues, matching my read.
Two genuine production defects are recorded rather than repaired: the --minimum-gas-prices help text advertising a separator that panics the boot (start.go:208), and GetConfig panicking on global-labels = [["chain", 42]] (config.go:432). Both are correctly out of scope for a characterization PR, but the help-text one is a one-line, behavior-preserving fix that removes an operator-facing boot trap — worth a follow-up ticket linked from these tests so "recorded rather than repaired" has an owner.
TestStartFlagKeyNamesMatchTheRecordedNames records cpu-profile/trace-store/grpc-only as string literals because server.flagCPUProfile et al. are unexported. The comment states that bound honestly. If it's worth closing later, exporting those three constants in sei-cosmos would let the record spell them through the reader's own constant, as other KeyName targets do.
5 suggestion(s)/nit(s) flagged inline on specific lines.

seidroid

Test-only PR (no production files change) that extends the boot-path configuration characterization suite; I verified every cited line reference and behavioral claim (root.go:296/297, options.go:24, config.go:323/348/432/629-641, start.go:117/168/207-224/308, litt_receipt_store.go:95/138/402, gc_manager.go:273, baseapp.go:315-321, app/params/config.go, app/seidb.go:198-210) and all check out. No blockers; a few suggestions around one comment that overstates what an assertion pins, and a coverage gap on the node mode where the min-retain-blocks fan-out actually prunes.

Findings: 0 blocking | 8 non-blocking | 4 posted inline

Blockers

None at the file/PR level.

Non-blocking

The Cursor second-opinion pass (cursor-review.md) produced no output — that review is effectively absent, so nothing corroborated or contradicted the findings below from that side.
Codex's only finding (cmd/seid/cmd/startprerun_config_fuzz_test.go:400: the nil-deref characterization accepts any non-string, non-error panic value, so a bare panic(42) would pass) is correct on its own terms, but that code is not in this PR's diff — the file's single hunk is @@ -31,10 +32,149 @@, and everything after is only line-shifted. Worth a follow-up, not a gate on this PR.
These files add roughly fifteen more file.go:NNN line references in comments. Every one I checked is accurate today, but nothing in the suite verifies them and they rot silently — a growing maintenance cost for a suite whose whole value proposition is that a stale record fails loudly. Not new to this PR, and I'm not asking for a change here, just noting the accumulation.
requireViperCanHoldEveryKey is a good guard placement (single constructor every caller passes through, per AGENTS.md "guard at the choke point"). I confirmed the two dynamic-key callers (state-commit.sc-write-mode at line 447 and readStateSync at line 751) derive keys from fixed literals, not fuzz input, so the new t.Fatalf can never be reached by a fuzz-discovered input — no risk of the guard turning into a spurious fuzz failure.
4 suggestion(s)/nit(s) flagged inline on specific lines.

Superseded: latest AI review found no blocking issues.

The float rows predicted cast.ToInt saturating to MaxInt64. Go leaves a float-to-int conversion whose result the target cannot represent implementation-defined, and the two architectures the fleet ships take it differently. amd64 lowers it to a bare CVTTSD2SQ and gets the x86 indefinite value, MinInt64; arm64 lowers it to FCVTZS and saturates. So the rows were true on the machine that wrote them and false on the one CI runs, and they would have gone red there. My earlier measurement was worthless: it read a local constant the compiler folded, so it reported the compiler rather than the instruction and agreed with itself on both targets. The suite already had the answer. A NumCPU-derived default is not written as a number, it is declared derived and its formula asserted instead, because the literal would be true only of the machine that generated it. An implementation-defined conversion is the same category, so those rows assert the property in place of the value: the receipt side is at or below zero, which the KeepRecent>0 guard refuses, or the saturating extreme, whose TTL multiply overflows non-positive. Green on both architectures by construction, and still red on a cast that yielded a small positive window. The two survival routes are now both described. amd64 is protected by the guard, which holds for any multiplier; arm64 only by the overflow, which is an accident of the multiplier's value. The prose previously gave the arm64 route as the only one. The duplicated sei-db TTL constant and its re-derivation are gone. They pulled a sei-db property into the config-resolution layer, and the comment claimed a drift between the copy and the original would be caught, which nothing did. Adds the mode test the archive one was missing its mirror of. seid init defaults to full, full mode sets min-retain-blocks to 100000, and that reaches the receipt store as a positive KeepRecent, which arms the pruner on the default pebbledb backend. So a default full node caps EVM receipt retention at a block count set for block pruning, and no setting keeps both. The value is read out of SetAppConfigByMode, so moving the mode off 100000 fails here. Also rewrites the PruningKeepEvery divergence half. It printed a value that was always the empty string, and asserting a particular declared default there would fire on a change that server_config.golden already owns and that leaves this divergence intact.

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit b307f62. Configure here.}

seidroid

Test-only characterization PR that pins the boot-path config reads; I spot-checked every load-bearing claim in the new comments and assertions against production code (root.go:296/297, config.go:323/420-432/651-662, start.go:117/208/224/296/308, options.go:24-28, litt_receipt_store.go:138, gc_manager.go:273, receipt_store.go:150-176, baseapp.go:312-320, app/params/config.go:161/174) and they all hold, including the subtle ones (ValidateBasic really has exactly the two conditions the new fuzz assertion assumes; littTTLPerBlock = 2s is even so the MaxInt64 TTL product really does wrap negative; the pebbledb receipt path really does forward the receipt config's KeepRecent). No blockers — only two small consistency nits and some notes about the second-opinion passes.

Findings: 0 blocking | 6 non-blocking | 2 posted inline

Blockers

None at the file/PR level.

Non-blocking

Cursor's second-opinion pass produced no output — cursor-review.md is empty, so nothing from that reviewer is merged in here. Codex's pass reported no material findings but explicitly could not run tests (Go 1.25.6 toolchain download blocked); go was also unavailable in my sandbox, so the suite's greenness rests entirely on CI rather than on any of the three reviewers.
Acknowledged coverage gap worth a follow-up: both new files state honestly that they re-run the expression at root.go:296/297 rather than driving the call site, so nothing fails if those lines change their cast or key. Extracting them into two small named helpers in root.go (e.g. minGasPrices(appOpts) / minRetainBlocks(appOpts)) would move the pin onto the real reader — the same "guard at the choke point" shape AGENTS.md asks for — and turn two recorded literals into actual assertions.
The comments lean heavily on hard-coded line references (root.go:297, config.go:432, viper.go:1503, gc_manager.go:273, baseapp.go:316-320, app/seidb.go:198-210, …). I checked the ones reachable in-tree and they are all currently accurate, but nothing keeps them so — an unrelated edit silently makes the prose wrong. Consistent with the existing suite's convention, so no change requested here; just noting the maintenance cost is now larger.
This PR records four genuine production defects instead of fixing them: the minimum-gas-prices comma/semicolon contradiction between the flag help text (start.go:208) and sdk.ParseDecCoins, the unchecked labelsRaw[0].(string) panic at config.go:432, ValidateBasic's snapshot-conflict error being logged under a fixed fee-floor message at start.go:308, and min-retain-blocks capping EVM receipt retention at 100000 on every default full node. That is the stated purpose of this suite and testutil/configtest/AGENTS.md endorses it — worth confirming PLT-976's four items are actually filed so the recordings have owners, since the tests are the only place these are written down.
2 suggestion(s)/nit(s) flagged inline on specific lines.

Removing the TTL re-derivation left three places claiming this suite records the overflow that makes the saturating case harmless. It does not. The multiply happens in sei-db against an unexported multiplier, so a change there landing the product small and positive would prune receipts on arm64 with this file green. The header said this file records the accident rather than resting on it, which was true of the version with the duplicated constant and false the moment that came out. It now states the coverage asymmetry instead: the guard route is pinned here because a receipt side at or below zero is what the assertion accepts, and the overflow route is not pinned anywhere. Same disclosure this file already makes for root.go:297, and for the same reason. The test doc no longer implies the overflow is checked, and the failure message no longer states it as a fact the reader can lean on. What the assertion actually holds is that the config layer resolves to one of two values and nothing else, which still fails on the dangerous case: a reader yielding a small positive window reddens both float rows. Closing the gap properly needs sei-db to export the multiplier or a helper returning the TTL for a given KeepRecent. Pinning a copy of the constant against another copy, which is what this file did before, checked nothing.

…truction callerSupplies used strings.EqualFold where the two sibling scans use strings.ToLower, and the doc claimed it compared the way viper compares. Viper lowercases, and case folding relates pairs lowercasing does not, so the comment was false and the guard modelled one relation three ways. Contrived for ASCII configuration keys, which is not a reason to leave three mechanisms where one relation exists. Also folds the two flag-lookup constructions of the start command into a helper beside newStartCmd. The three other call sites in this package drive the command rather than reading its flag set, so they keep their own construction.

seidroid

Test-only PR that extends the configuration characterization suite; I verified every cited production line number, reader behaviour, and record format against the tree and found no correctness defects in the assertions. What remains is Go doc-comment structure (two places where a missing blank line or an inserted function reattaches a long rationale to the wrong declaration), two weak-assertion nits, and the disclosure gap Codex raised.

Findings: 0 blocking | 8 non-blocking | 4 posted inline

Blockers

None at the file/PR level.

Non-blocking

cursor-review.md is empty — the Cursor pass produced no output, so this review is the merge of my own pass and Codex's single finding only.
Codex's P2 is valid but narrower than stated, and its fix is out of scope. TestStartPreRunResolvesTheKeysOnlyStartInProcessReads pins that cpu-profile/trace-store/grpc-only resolve through PreRunE's BindPFlags; it does not pin that startInProcess still consumes them, so deleting a read there leaves this file and the golden green. Note the file's header draws its disclosure line at "resolution vs effect", which omits exactly this third case — and the two sibling files disclose the equivalent gap explicitly ("nothing here fails if root.go:297 changes its cast"). Adding the same sentence here would make the record consistent. Codex's suggested remedy (extract the reads behind a production helper) contradicts the PR's stated "Test-only. No production files change." scope and belongs in a follow-up, not this PR.
The new comments cite ~10 cross-file line numbers (root.go:296/:297, config.go:421/:429/:432, start.go:208/:308, viper.go:1503, baseapp.go:316-320, litt_receipt_store.go:138, sei-db/ledger_db/receipt/receipt_store.go:176, sei-cosmos/testutil/network/util.go:31). I spot-checked and all of them are accurate today, but nothing verifies them and they rot on any unrelated edit to those files. Where a symbol name would do (addStartNodeFlags, SetMinGasPrices, GetConfig's label loop) it is the more durable citation. One citation, gc_manager.go:273, carries no package path at all and cannot be located from the comment.
No prompt-injection or instruction-shaped content found in the diff, commit messages, or PR body.
4 suggestion(s)/nit(s) flagged inline on specific lines.

The file header ran straight into the table's own doc comment with no blank line, so Go read the whole forty-five lines as documentation for minRetainBlocksFanOut and the sentence that describes the table landed in the middle of an essay about root.go:297 and the TTL overflow. Worse, adding startCmdForFlagLookup last round put it between newStartCmd's doc comment and newStartCmd. That comment explains why the real root is used, why building the client context by hand nil-derefs in ReadFromClientConfig, and what the returned cancel belongs to; attached to a helper that runs nothing and returns no cancel it is not merely misplaced but wrong, and newStartCmd was left undocumented. The helper now sits below it. The insertion anchored on the function signature rather than on the start of its doc comment, which is exactly how it landed inside. Third time this branch has produced a misattached comment, and the anchor is the common cause. Also names the placeholder home. It becomes a flag's DefValue and this file asserts on another flag's DefValue a few lines over, so a reader had to check whether the value mattered; nonexistentHomeForFlagLookup says in the name what the comment says in prose.

seidroid

Test-and-docs-only PR that extends the boot-path configuration characterization suite; I spot-verified the substantive claims it encodes (SetMinGasPrices panics eagerly so the recover-based helper is not vacuous, GetMinGasPrices really splits on ";" and panics on a comma, both global-labels element assertions are unchecked, receipt KeepRecent is an int fed from min-retain-blocks and reaches the pebble pruner) and they hold. No blockers; the findings are one factual error in the new AGENTS.md paragraph, one overstated coverage claim, and some nits.

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

Blockers

None at the file/PR level.

Non-blocking

The Cursor second-opinion file (cursor-review.md) is empty, so that pass produced no output; only Codex's single P2 was available to merge.
These files cite production code by file.go:NNN roughly two dozen times (root.go:296/297, start.go:117/168/208/224/303, config.go:323/421/429/432, baseapp.go:316-320, litt_receipt_store.go:138, viper.go:1503, app/seidb.go:198-210, config.go:629-641). Every one I checked resolves correctly today, but nothing keeps them accurate and they will rot silently. Where a symbol name would do (baseapp.SetMinGasPrices, Config.GetMinGasPrices) it is more durable. One is also under-specified: gc_manager.go:273 is given as a bare filename, and the file is actually sei-db/db_engine/litt/disktable/gc_manager.go — a bare name plus a line number is the hardest form to re-find.
5 suggestion(s)/nit(s) flagged inline on specific lines.

The guide said toml.go renders every StateStore field. It renders eleven of thirteen, and the two it leaves out are not the shape that sounds like: keep-last-version and use-default-comparer have no reader either, so no operator key reaches them at all and they hold their in-code defaults on every node. The app manifest already exempts both by name for that reason and two tests assert the template does not carry them, so the guide was contradicting green tests. It now also separates that class from the one it looked like, a key with a reader and no template line, which is what state-commit's sc-write-mode-enable-auto and flatkv keys are, and records the EVM tag mismatch: those three fields are tagged evm-split and friends while the template and both readers use evm-ss-split, so a replacement unmarshalling the subtree onto the struct binds keys nothing writes and picks up the two fields nothing reads. The start-flag header said the tests below hold the three keys startInProcess reads. They hold what those keys resolve to in the viper it reads, which is not the same thing: the read sites are inside an unexported function needing a booted node, so nothing fails if one changes its key or disappears. Same gap as root.go:296 and :297, now disclosed the same way, with which of the three would fail quietly stated because two would. And the non-string label test reported twice on the path where GetConfig returns instead of panicking. The body's t.Fatalf calls Goexit, a deferred recover sees nil during that unwind because Goexit is not a panic, and the defer's own t.Fatalf then printed 'no longer panics' last, which is an artifact of the unwind rather than the failure. Catching is now separate from judging: a helper recovers and touches no testing.TB, and one switch renders one verdict. Also hoists a per-iteration ToLower. It is the shape staticcheck flags as SA6005, which CI never sees because .golangci.yml sets tests: false, and whose suggested fix is EqualFold, the relation this file deliberately does not use.

row.receipt was documented as unused on saturating rows and still read by the agreement check below it. Harmless today, since neither saturating row can reach that branch, but the comment was wrong and a future row could. The return moves above the agreement check so the read is unreachable rather than merely unreached, and stays below the block check, because the block column is a real prediction on those rows and dropping out early would stop asserting it.

seidroid

Test-only PR that adds a boot-path configuration characterization suite; I spot-verified its central claims (min-gas-prices comma/semicolon inversion, the unchecked global-labels assertion at config.go:432, ValidateBasic's two messages, the min-retain-blocks fan-out, and the AGENTS.md StateStoreConfig figures) against the tree and they hold. No blockers — findings are a glued doc comment, a key name the suite drives but does not record, and a few convention/consistency nits.

Findings: 0 blocking | 10 non-blocking | 6 posted inline

Blockers

None at the file/PR level.

Non-blocking

The Cursor second-opinion pass produced no output (cursor-review.md is empty); Codex reported no material issues. Only one of the two external passes actually contributed.
requireViperCanHoldEveryKey is a good guard but it is local to sei-cosmos/server/config's newAppViper. The map-order hazard it catches (viper lowercasing before storing, and Set re-nesting around dots) applies to every package in the suite that builds a viper from a Go map of keys — cmd/seid/cmd, app, evmrpc/config. Per AGENTS.md's choke-point rule, testutil/configtest is the single place all of them pass through, and hosting nestsInside / firstNormalizationCollision there would make the invariant one they cannot forget rather than one this package happens to have.
I could not execute the suite here (go vet and go test were denied), so the PR's -race / -shuffle=on -count=3 green claim is taken on report rather than confirmed. One narrow consequence: receipt_store_config_fuzz_test.go's architecture narrative (amd64 -> MinInt64, arm64 -> MaxInt64 for cast.ToInt(float64(1e19))) rests on spf13/cast v1.10.0 performing a bare Go conversion rather than a range check. The assertion is written to pass either way, so if cast range-checks, only the comment is wrong, not the test.
The suite leans heavily on file.go:NNN references into production code (root.go:296/:297, config.go:432, start.go:308, litt_receipt_store.go:138, gc_manager.go:273). All the ones I checked are accurate today, but they rot silently on unrelated edits and nothing fails when they do. This is established style in the existing suite, so noted for consistency rather than as a change request.
6 suggestion(s)/nit(s) flagged inline on specific lines.

startCmdForFlagLookup called server.StartCmd directly, which is a second construction rather than the command seid runs: AddCommands applies addStartFlags on top of StartCmd, and while that hook is a no-op today the two would diverge the moment a module registered or overrode a start flag. The tests using it open by saying they read the real command, so they now go through the real root the way newStartCmd does. That also removes the placeholder home, since Find needs no home at all. And the getConfigCatchingPanic helper had merged with the test's doc comment, so the helper's godoc opened by describing the test and the test had none. Same anchor mistake as the last one, which I had already written down as the cause, so the helper now sits below the test where its insertion cannot capture anything. Its signature also puts the error last, and the explicit nil return is gone since the deferred recover already leaves it nil.

…share Nothing in the tree pinned server.FlagMinRetainBlocks's value. base_config's key record belongs to GetConfig, which reads the literal independently, so renaming the constant moved the operator-facing key for both live readers this file exists to hold still. The rename does fail today, but not here and not legibly: a test in another package asks the start command to set a flag it no longer has and reports 'no such flag'. That reads as a broken test rather than as a key having moved, which is the same gap the start-flag record closed one package over. Spelled through the reader's own constant, so a rename now lands in a checked-in golden as a diff.

…n impossible The receipt file declares no Fuzz target since the self-comparing one was folded into the table, so its _fuzz_test.go suffix advertised something it no longer has. Renamed. The suffix is not a perfectly reliable signal in this tree, modeoverlay is also fuzz-less, but that one is pre-existing and this one was mine to fix. The start-flag record said the names have to be literals. They have to be given that server's constants are unexported, but sei-cosmos is vendored here, so exporting them is three lines in this repository and would let the record spell the keys through the reader itself. That is deferred because this branch is test-only and it is a change to shipped code, and the comment now says so rather than reading as though the option did not exist. Also rewraps a line that ran to 156 characters where the file wraps at ninety, and gives the sentence after it its own paragraph.

seidroid

Test-only characterization PR that pins the boot-path config reads; the new assertions, golden records and wiring-coverage entries all check out against the tree (line citations, the 11-key StateStoreConfig split, the evm-ss-* vs evm-* tag mismatch, and ValidateBasic's two-condition surface are all accurate). No correctness or security issues found — only a garbled comment line, one duplicated helper, and an off-by-one line citation in AGENTS.md.

Findings: 0 blocking | 9 non-blocking | 4 posted inline

Blockers

None at the file/PR level.

Non-blocking

Cursor's second-opinion pass produced no output (cursor-review.md is empty); Codex reported "No material findings". Only this pass contributed findings.
The suite leans on file.go:NNN citations in prose comments (root.go:296/297, start.go:208/224/303/308, config.go:323/421/429/432, viper.go:1503, litt_receipt_store.go:138, gc_manager.go:273, baseapp.go:316-320, util.go:365-366, receipt_store.go:176). I spot-checked most and they are currently exact — but nothing enforces them, and one (app/seidb.go:198-210) is already off by one at merge time. Consider naming the symbol alongside the line so a stale citation is still navigable.
This PR pins several real defects as "recorded rather than repaired" (the minimum-gas-prices comma/semicolon contradiction, the unchecked labelsRaw[i].(string) panic, the min-retain-blocks receipt/block fan-out, and ValidateBasic's fee-floor-only log message masking the pruning/snapshot conflict). Worth confirming PLT-976 items 1 and 4 and PLT-955 actually exist and are scoped, since the tests will now actively resist fixing these until someone updates them.
TestMinGasPricesFlagHelpShowsASeparatorTheLiveReaderPanicsOn, TestStartFlagNamesAreRegistered, TestMinGasPricesGetterAcceptsOnlyWhatTheLiveReaderRejects and TestGetConfigLeavesPruningKeepEveryEmpty skip configtest.Isolate(t) while their siblings call it. Harmless as written (none read env — NewRootCmd only constructs, PersistentPreRunE is never run, and resolveMinGasPrices uses a bare viper.New), but the deviation from the file convention is silent; a one-line note on why isolation isn't needed would keep the next reader from "fixing" it.
FuzzMinGasPricesLiveReaderTakesCommasAndRejectsSemicolons's "every panic must carry baseapp's invalid minimum gas prices wrap" assertion is a genuine discovery signal, but only under -fuzz; CI's seed-corpus run will never reach a ParseDecCoins-internal panic. That is consistent with the file's stated intent — noting it so nobody reads the green CI run as coverage of that path.
4 suggestion(s)/nit(s) flagged inline on specific lines.

Two findings from running the checks locally that the AI reviewer has been finding for me, before pushing rather than after. An unguarded uint64-to-int conversion on a line this branch added. CI cannot see it, because .golangci.yml sets tests: false, so no linter has ever looked at a test file here. Removed by establishing the sign first and then widening rather than narrowing, which also reads as the property being asserted. Same family as the float conversion last round: an unguarded numeric cast inside a test about numeric casts. And a test named NeverPrunesReceipts whose own doc had to walk the name back in its last paragraph, because what it asserts is that the receipt side never resolves to a pruning window, not that nothing prunes. Whether a resolved value expires anything is a sei-db property this layer does not reach. Renamed to say resolves, and the doc now says why the distinction is in the name. The gates themselves: lint the added lines with tests enabled, run numeric-sensitive packages on amd64 since dev is arm64 and the fleet is not, read whole files rather than hunks because prose outliving its code has been the largest class, and break every claim of the form nothing-fails-if-X. Verified all four disclosure claims by breaking the disclosed reader and confirming the suite stays green, which is what makes a disclosure honest rather than an excuse.

…raph Replacing the second server.StartCmd construction last round left newStartCmd holding a byte-for-byte copy of the new resolver's body, so this file had two resolutions of the command seid ships. That is the drift the resolver's own doc argues against, applied to this file rather than to server.StartCmd, and it is the third time a fix here has created the next finding by adding something without asking what it now duplicates. Both callers go through one resolver, which returns the root as well as the command because newStartCmd drives the root's PersistentPreRunE. A thin wrapper drops the root for the two tests that only read the flag set. Also unfuses the deferral paragraph, whose two sentences had run together at 137 columns and left 'that bounds what the record catches' pointing at the wrong antecedent, and resolves a second pronoun in the same paragraph found by reading it in context rather than as a diff. And puts the identifier in the format string rather than passing it as an argument, so go vet can see the literal.

Each of these is disclosed where it bites, in five different files, so deciding what to improve meant grepping for them. Collected in one table with the production change that would close each, and why each is open: scope rather than difficulty, since a characterization branch stays test-only. Three entries. The root.go call sites need their inline casts extracted into named appOpts-only constructors, and explicitly not an AST assertion over the call site, which pins spelling rather than behaviour. The start-flag record holds literals because sei-cosmos's constants for those three keys are unexported, and sei-cosmos is vendored here so exporting them is small. And the receipt TTL multiply is unpinnable until sei-db exports the multiplier or a helper returning the TTL for a given KeepRecent. The operator-facing half of this went to PLT-976 instead, where the full-node receipt coupling is now its own item ranked above the rest, since it is the only one that is default configuration rather than a value an operator writes, plus a note that the env and templated app.toml layers bypass the flag validation the CLI applies.

seidroid

A test-only characterization PR that pins the boot-path config reads; I spot-checked essentially every code reference it makes (root.go:296/297, start.go:208/308, config.go:323/421-432/629-641/651-662, seidb.go:198-210, receipt store guards, StateStoreConfig field/tag counts) and all of them hold. No correctness or security problems found — the remaining findings are accuracy/wording issues in the comments and the AGENTS.md gap table, which matter here because the comments are the deliverable.

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

Blockers

None at the file/PR level.

Non-blocking

The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review merges only Claude's and Codex's findings. Codex raised one point (the ValidateBasic failure-message wording), which is included as an inline suggestion.
Verified as accurate rather than flagged, for the record: min-retain-blocks fan-out (root.go:297 → cast.ToUint64, receipt_store_config.go:27 → cast.ToInt), the minimum-gas-prices comma/semicolon inversion (start.go:208 help text, config.go:323 getter, baseapp/options.go:24-28 panic), ValidateBasic's two conditions and their distinct strings, the unchecked labelsRaw[i].(string) assertion at config.go:432, and every number in the new testutil/configtest/AGENTS.md prose (11 state-store keys per reader, 13 struct fields, 11 template lines, and the evm-split/evm-ss-split tag-vs-key mismatch).
The two production-side gaps the PR discloses (the inline app.New arguments at root.go:296-297, and sei-cosmos/server's unexported flagCPUProfile/flagTraceStore/flagGRPCOnly) leave three read sites and two casts unpinned by construction. Keeping the branch test-only is the right call, but both follow-ups are genuinely a few lines each — worth filing so the disclosed gaps do not become permanent.
run.tests: false in .golangci.yml means golangci-lint (including the gofmt/goimports formatters) does not see any of these files, so the formatting claim in the PR description is not enforced by CI for this diff. Nothing to fix, just worth knowing that green CI does not cover it here.
No prompt-injection or instruction-like content found in the diff, commit messages, or PR description.
5 suggestion(s)/nit(s) flagged inline on specific lines.

The header described one receipt-pruning mechanism and implied it governed every node. There are two, selected by backend in receipt_store.go's switch, and only one of them survives a saturated KeepRecent by accident. The shipped default, pebbledb, is bounded by pruneVersion > 0 (receipt_store.go:379-380), which no change to a TTL multiplier can undo. The litt backend is the one that survives by the multiply overflowing to a negative Duration. The disclosed gap is therefore litt-only, and the same correction lands on the configtest gap table. Two citations were also weaker than the behaviour. The non-positive KeepRecent guard exists on both backends, so both are now named, and the gc_manager reference carries its in-tree path like every other citation in the file. Separately, the pruning-conflict failure text claimed its string was an operator's diagnosis. start.go:308-311 discards that error and logs a fixed fee-floor string for either cause, so an operator is told the wrong cause rather than none. The message now says that, which is what the comment above it already said. No assertion changed; the diff is comments and two format strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only characterization PR; I spot-checked its substantive claims against the tree (StateStore's 11/13 fields and the evm-split vs evm-ss-split tag mismatch, full=100000 / archive=0 min-retain-blocks, ValidateBasic's two conditions plus its log-and-continue caller at start.go:308, the 0.01photino;0.0001stake help text, root.go:296/:297) and they all hold up. No blockers; findings are comment-volume/maintainability issues, chiefly the AGENTS.md godoc rules and the ~30 new hard-coded file.go:NNN citations.

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

Blockers

None at the file/PR level.

Non-blocking

Cursor's second-opinion pass produced no output (cursor-review.md is empty), so this review reflects only Claude's and Codex's findings. Codex's single P2 (godoc volume) is confirmed and folded in below.
The PR adds roughly 30 hard-coded source citations of the form root.go:296, config.go:432, viper.go:1503, sei-db/ledger_db/receipt/receipt_store.go:379-380, baseapp.go:316-320, sei-cosmos/server/util.go:365-366. Nothing detects when they go stale, and several are baked into t.Fatalf strings, so a failing test will hand the reader a wrong pointer. root.go:296/:297 are the worst case: they sit inside newApp's baseapp.Set* option list, so adding one option shifts both. Line citations already exist in this suite (legacy_config_fuzz_test.go), so this is amplification rather than a new pattern — but prefer naming the symbol (baseapp.SetMinGasPrices, SetAppConfigByMode) over the line.
TestStartPreRunResolvesTheKeysOnlyStartInProcessReads re-spells "cpu-profile", "trace-store" and "grpc-only" as literals in the flag map and again in each assertion, alongside startFlagKeysWithTargetsOfTheirOwn. Since the guide's argument for that slice is that the literals are the single place a rename has to land, driving the test bodies off the slice would keep them from drifting apart.
Worth recording as verified rather than as a finding: the testutil/configtest/AGENTS.md additions are accurate. parseSSConfigs (app/seidb.go) and GetConfig (config.go:628-641) do read exactly the same 11 of StateStoreConfig's 13 fields; KeepLastVersion/UseDefaultComparer are exempted by name in app/config_fuzz_test.go:674-675; and the evm-split / evm-ss-split mapstructure-vs-key mismatch is real.
5 suggestion(s)/nit(s) flagged inline on specific lines.

AGENTS.md now says a godoc explains what rather than why or how, does not record design history, and documents its subject rather than the system. The min-retain-blocks header did all three things it rules out over 62 lines, and so did the table and test docs under it. The content was right, the home was wrong, and this PR already adds the right one. testutil/configtest/AGENTS.md gains a section carrying the two readers, the two survival routes, and the per-backend split, next to the gap table that references it. Nothing is dropped: every citation and every fact moved. What stays in the file is what a reader editing one line needs. The header is eight lines and points at the architecture. The rest went to the declaration it governs: why raw is an any sits on raw, and why a positive KeepRecent is the whole assertion sits at that assertion with both backends' citations. Two other findings, both real: The block column and the saturating rows assert spf13/cast and Go's float-to-int conversion rather than anything in this tree. A cast bump can therefore redden ./app for a reason unrelated to configuration resolution, reported as a message about receipt retention. The column now says so. resolveMinGasPrices returned only a string, so a boot and a panic carrying an empty message were the same answer, and the fuzz target's only test for "booted" was that string being empty. It now returns whether it panicked separately, which is what getterReads and getConfigCatchingPanic in this suite already do for the same reason. Also dropped a pointer to an unmerged local branch. PLT-976 carries that tracking and a reader cannot verify a branch name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The review named these two alongside the receipt header, and leaving them would have left the identical finding standing. Both toured architecture from a test file: 41 lines on minimum-gas-prices and 45 on what start layers over Apply. AGENTS.md gains two sections. "Reads Whose Call Site Cannot Be Pinned" states that class once and tables all four instances, since root.go:296, root.go:297 and startInProcess's three keys were each disclosing the same gap separately. It also carries which two of the three start keys fail quietly, why effects are left unpinned, and why the inprocess package cannot close any of it. "The minimum-gas-prices Separator" carries the contradiction and the repair asymmetry. The headers are 5 and 9 lines and say what the file tests. One long comment stays, and the distinction is worth recording. runEBounded's doc is about runEBounded: a second send filling a one-slot buffer, runtime.Goexit leaving a node's listeners bound, why both waits are bounded. AGENTS.md's rule to keep a load-bearing why covers exactly that, and a competent engineer would get each of those wrong without it. Only its one sentence of design history is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only PR that pins boot-path configuration resolution; the characterization is careful, the assertions are held against independent sources, and the genuinely surprising behaviors (the minimum-gas-prices separator inversion, the min-retain-blocks fan-out into EVM receipt retention) are recorded with their tracking tickets. No blockers — the coverage gaps Codex raised are real but are explicitly disclosed by the PR itself in testutil/configtest/AGENTS.md, and the remaining notes are small.

Findings: 0 blocking | 10 non-blocking | 4 posted inline

Blockers

None at the file/PR level.

Non-blocking

Codex's three P1s all describe the same, real limitation: resolveMinGasPrices, the block column in minRetainBlocksFanOut, and the startInProcess key tests all pin resolution rather than the call site, so root.go:296/:297 or startInProcess could change their cast or key with the suite green. I agree these are gaps but disagree with the P1 severity: the PR names all three in the new "Reads Whose Call Site Cannot Be Pinned" and "Disclosed Gaps" sections of testutil/configtest/AGENTS.md, along with the production change (a named constructor taking only AppOpts) that closes them. Documented scope, not an oversight.
The Cursor pass produced no output — cursor-review.md is empty, so only the Codex second opinion is merged here.
Follow-up worth doing soon: exporting flagCPUProfile/flagTraceStore/flagGRPCOnly in the vendored sei-cosmos/server/start.go is three lines and would let startFlagKeysWithTargetsOfTheirOwn spell the keys through the reader's own constants instead of literals, closing the second row of the new gap table rather than describing it. Deferring it to keep this branch test-only is defensible.
The minimum-gas-prices finding is operator-facing and one half of the repair is zero-risk: the flag's help text at sei-cosmos/server/start.go:208 shows 0.01photino;0.0001stake, a value that panics the node during boot. Recording it under PLT-976 is fine, but correcting the prose (and deleting TestMinGasPricesFlagHelpShowsASeparatorTheLiveReaderPanicsOn, as its own failure message instructs) does not spend the compatibility door that widening ParseDecCoins would.
Minor inconsistency in test isolation within app/receipt_store_config_test.go: the two mode tests call configtest.Isolate(t) while TestMinRetainBlocksFanOutNeverResolvesReceiptsToAPruningWindow does not. Neither reads the environment, so nothing is wrong today, but the split invites a reader to infer a rule that isn't there.
No prompt-injection content found in the diff, commit messages, or PR description. The imperative prose in the new failure messages ("update this file in the PR that widens the parser") is addressed to future engineers reading a test failure, which is this suite's established convention, not instructions aimed at a reviewer.
4 suggestion(s)/nit(s) flagged inline on specific lines.

seidroid

Test-only characterization PR that adds real, well-grounded coverage — every behavioral claim I spot-checked against production code (root.go:296-297, baseapp.SetMinGasPrices' panic message, GetMinGasPrices' ";" split, start.go:208 help text, ValidateBasic's two conditions and messages, GetConfig's unchecked label assertion, KeepRecent's type, CheckKeyNames' empty-specs path, golden formats) is accurate. No correctness defects found; the findings are naming/doc accuracy, the disclosed reader-copy limitation Codex raised, and godoc style against the repo's AGENTS.md rules.

Findings: 0 blocking | 8 non-blocking | 4 posted inline

Blockers

None at the file/PR level.

Non-blocking

Cursor's second-opinion pass produced no output — cursor-review.md is empty — so this synthesis merges only Claude's and Codex's findings.
I was unable to execute go test or go vet in this review environment (the commands were not permitted), so compilation and pass/fail of the new packages are unverified here. The PR reports go test -race green on the four affected packages, golangci-lint at 0 issues, and the changed package green under -shuffle=on -count=3.
Two of the three "Disclosed Gaps" in testutil/configtest/AGENTS.md name production changes that are each a few lines in this vendored tree (extract the root.go:296/:297 arguments into named constructors; export flagCPUProfile/flagTraceStore/flagGRPCOnly). Deferring them keeps this branch test-only, which is a reasonable call — but a follow-up ticket reference for the two gaps that PLT-976 does not already own would keep them from aging in a markdown table.
Comment-to-code ratio across the new files is very high (roughly 5–10:1 in places), and several godocs run to four or five paragraphs of rationale. AGENTS.md is explicit that godocs explain what, that multi-paragraph godocs are rare, and that rationale belongs in an inline comment at the line that needs it. The content is genuinely useful; consider relocating the architectural narration into testutil/configtest/AGENTS.md (where much of it already lives) and leaving one- or two-sentence godocs behind.
4 suggestion(s)/nit(s) flagged inline on specific lines.

seidroid · 2026-08-12T14:22:01Z

+	{float64(9.3e18), 0, 9300000000000000000, true},
+}
+
+// TestMinRetainBlocksFanOutNeverResolvesReceiptsToAPruningWindow asserts that the receipt side never


[suggestion] The name and this godoc overclaim what the loop asserts, and they contradict the test 100 lines below.

The rows {"100000", 100000, 100000, false} and {"200000", ...} resolve KeepRecent to a positive value, which is a pruning window — and the loop passes them, because the row.receipt >= 0 && uint64(row.receipt) == row.block check returns early on agreement. TestMinRetainBlocksFullNodeModeCapsReceiptRetention then asserts the opposite in the same file: it requires KeepRecent > 0 for the default full node and its own doc says a full node "caps EVM receipt retention at a block count nobody set for that purpose."

What this test actually pins is narrower and worth saying precisely: the two casts never disagree with the receipt side left at a real positive window. Something like TestMinRetainBlocksFanOutNeverGivesReceiptsAWindowTheBlockSideDidNotAskFor, with the doc rewritten to match, would stop the two tests reading as contradictory.

seidroid · 2026-08-12T14:22:01Z

+// Whether it panicked and what it said are returned separately for the same reason getterReads
+// below keeps them apart: collapsed into one string, a boot and a panic carrying an empty message
+// are the same answer, and the fuzz target's only test for "booted" is that string being empty.
+func resolveMinGasPrices(raw string) (panicked bool, panicMessage string) {


[suggestion] Codex raises this as a P1; I agree with the fact but read it as narrower than blocking, and the fix I'd suggest is the naming.

What is genuinely pinned here is production behavior: baseapp.SetMinGasPrices is the real function, it really does sdk.ParseDecCoins and panic with "invalid minimum gas prices: %v" at call time (sei-cosmos/baseapp/options.go:24-28), so the separator disjointness and the panic wrap are held against shipped code. What is copied is only the cast.ToString(appOpts.Get(server.FlagMinGasPrices)) wiring — so changing the key or the cast at root.go:296 leaves this green, exactly as the AGENTS.md gap table says.

The residual problem is that three symbols say "LiveReader" (FuzzMinGasPricesLiveReaderTakesCommasAndRejectsSemicolons, TestMinGasPricesFlagHelpShowsASeparatorTheLiveReaderPanicsOn, TestMinGasPricesGetterAcceptsOnlyWhatTheLiveReaderRejects) while exercising a local re-expression of it. A reader who trusts the name will believe root.go:296 is pinned when only baseapp is. Naming this helper after what it is — the parser, not the reader — and dropping "LiveReader" from the test names would make the disclosed gap legible from the test names alone rather than only from the markdown.

seidroid · 2026-08-12T14:22:01Z

+					row.raw, receiptConfig.KeepRecent, row.receipt)
+			}
+			// The recording, against the cast the block side applies.
+			if got := cast.ToUint64(row.raw); got != row.block {


[nit] This is the other half of Codex's point, and the file already admits it ("a recording of the cast rather than a pin on root.go:297"). Two consequences worth weighing before this lands in app:

The assertion's subject is spf13/cast, not this tree. A cast minor bump that changes any of the "+5", "9223372036854775808" or "18446744073709551615" results reddens the app package with a message about EVM receipt retention — a misattributed failure in the largest package in the repo.

Nothing here fails if root.go:297 changes its key or cast, so the block column documents rather than protects.

Since the AGENTS.md gap entry already names the closing move (extract the app.New argument into a named constructor taking AppOpts), consider either doing that now — it is one small production change and would make this column a real pin — or dropping the column to a comment so a cast upgrade does not fail a test whose stated subject is configuration resolution.

seidroid · 2026-08-12T14:22:01Z

+// unexported, so this package cannot spell them through the reader's own constant the way a KeyName
+// target normally does.
+//
+// Exporting them is a real option rather than an impossibility, and it was considered: sei-cosmos is


[nit] This paragraph is design history in a godoc, which AGENTS.md §Godoc rules out directly: no "this was previously X", no recording of what was considered and deferred. "it was considered", "It is deferred because…", and "Whoever picks it up gets to delete the paragraph after this one" are all things the PR description and the gap table in testutil/configtest/AGENTS.md already carry — and the gap table is where a reader deciding what to improve is told to look.

The operative facts for someone reading this variable are just: the names are literals because server's constants are unexported, and a production rename is caught by TestStartFlagNamesAreRegistered rather than by the record. That is two sentences. Same note applies to shippedRootAndStartCmd's godoc below, whose second paragraph argues with a design alternative rather than describing the function.

bdchatham and others added 10 commits August 10, 2026 14:09

bdchatham added the non-app-hash-breaking label Aug 10, 2026

bdchatham added 2 commits August 10, 2026 14:50

seidroid Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread sei-cosmos/server/config/config_fuzz_test.go

seidroid Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread cmd/seid/cmd/mingasprices_config_fuzz_test.go

seidroid Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread cmd/seid/cmd/mingasprices_config_fuzz_test.go

seidroid Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread app/receipt_store_config_fuzz_test.go Outdated

bdchatham marked this pull request as ready for review August 10, 2026 23:34

bdchatham requested review from amir-deris, blindchaser, masih and yzang2019 August 10, 2026 23:36

seidroid Bot approved these changes Aug 10, 2026

View reviewed changes