test(config): extend golden value test coverage (PLT-893) by bdchatham · Pull Request #3861 · sei-protocol/sei-chain

bdchatham · 2026-08-05T22:13:26Z

Motivation

An earlier sweep renamed 27 operator-facing app.toml key names with the whole configuration suite green. "The suite covers these keys" and "the suite would notice a change to them" turned out to be different claims, and a test that cannot fail reads exactly like one that works.

Test Specs & Approach

These suites pin how a seid node resolves configuration, meaning which value each setting ends up with once the config files, the environment and the built-in defaults have had their say. They record how that behaves today rather than how it was meant to, because the path has shipped and changing it would move what running nodes do. The versioned manager corrects those cases later, using these tests as evidence nothing else moved with them.

Three mechanisms carry the work, and each had a way to pass while asserting nothing.

Checked-In Expected Values

One file per group of settings, holding its defaults and its key names. Comparing a reader against the package's own defaults struct moves both sides together when a default changes, so a second copy the code does not derive from is what makes that change fail.

Fuzz Seeds Per Setting

Under an ordinary go test a Go fuzz test replays its hand-written examples and nothing else, so the seed set is the coverage. A setting whose seeds all resolve the way an absent key would is a setting nothing exercises.

Per-Package Coverage Record

One file per package listing which checks cover each group, read from the package's own test files. Deleting a check call was otherwise silent, because the checks that remain still pass.

New Test Coverage

Added Here

Nine rows declared that a malformed value is rejected rather than quietly kept while no seed reached that path, and the harness now requires one. Five groups gained a defaults file. Five of the six helpers every row assertion flows through had no test of their own. Deleting a check call now fails a comparison naming what went missing, across all 11 wired packages. Each closure was checked by breaking the reader on purpose and confirming the tests noticed.

Recorded Rather Than Repaired

A node mode's state-store settings are discarded before a node reads them, so an archive node prunes at the default instead of keeping all history. Correcting which value wins changes the app.toml a newly initialised node receives, so it is tracked as PLT-955. Three further oddities are filed under a new config-resolution-oddity label as PLT-952, PLT-953 and PLT-954.

Coverage Today

Counted from the records rather than claimed. Fourteen groups of settings are covered: defaults 14, key names 13, per-row resolution 13, seed discrimination 13, table completeness 12, absent-key baseline 8.

Remaining Gaps

Three, in the order worth closing.

The absent-key baseline reaches 8 of 14 groups. genesis, light_invariance, server_config, state-commit, state-store and state-sync have none, so nothing pins what those readers return when handed no keys.
Sixteen rows declare that omitting a setting yields something other than the built-in default, and nothing verifies that column, because the check that would needs the table passed to it.
GetConfig reads 78 distinct literal keys against a 23-row table, and 17 of them appear in no test anywhere. Their values are anchored by a defaults file, so the gap is names, casts and guards.

gofmt and goimports clean, golangci-lint (v2.8.0, CI's pin) reports 0 issues, all 12 affected packages pass. Nothing new runs in CI and no workflow changes, since these assertions live in suites that already run.

🤖 Generated with Claude Code

…893, FR-001/FR-002) -update is one process-global switch over three independent records: the defaults record CheckDefaults compares, the key-name record CheckKeyNames compares, and requireKeyNameRecord's cross-check, which stands down rather than compare against a file being regenerated in the same run. No workflow inspected the tree afterwards, so a CI run holding the flag would have rewritten all three and reported success -- a test comparing against a record regenerated from the code passes by construction, which is the most convincing kind of test that proves nothing. The refusal sits at the single point every record write passes through, writeGolden, rather than at each caller. A guard repeated per call site is a convention the next caller can forget; a guard at the one writer is an invariant they cannot. keynames.go therefore needed no change at all. requireKeyNameRecord is the one caller with anything of its own, because it suppresses a comparison rather than performing a write and a suppressed comparison reads as a pass; it asks one named question. CI joins Isolate's allowlist. The refusal reads the environment and Isolate strips everything outside that list, so a check that isolated before writing would have found CI unset and written the record with nothing to notice. Verified the cost is nil rather than small: no key in the closed read-key set is named ci, and the env lane recorded no bare CI resolution, so the empty-prefix viper has nothing to resolve it to. TestRefusalSurvivesIsolate keeps the entry from being tidied away -- removing it fails with a message naming the fix. Both go-test jobs gain two steps. One asserts CI is set, because on a self-hosted runner that variable is the runner's to provide and without it the refusal is silently inert. The other fails the job when the suite leaves the tree dirty, which covers what the refusal cannot: a package with its own -update flag that never routes through writeGolden, as sei-tendermint/internal/p2p/conn does. Verified by removing each refusal route in turn and watching the contract test redden, with go vet reporting no build error so the failures are assertion failures rather than breakage. CI=true go test ./x/evm/querier/ -count=1 -update exits non-zero naming all three records it declined to rewrite and leaves git status --porcelain empty. The existing -update round-trip contract test still passes. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

…d wiring (PLT-893) Three things the suite asserted about itself that nothing checked. THE Checked COLUMN Checked says a reader propagates a conversion failure as an error rather than swallowing it and keeping the zero value. For an operator that is the difference between a boot that stops and names the key, and a node quietly running on a default they did not choose. Nothing required a seed reaching that error path, so a row could carry the column while every seed it declared cast cleanly, and the column recorded an intention rather than a behaviour. The seeds check now requires each Checked row's corpus to contain a value the reader actually rejects. Of the 68 rows carrying the column, 9 were unexercised: four in evm, three in receipt-store, two in eth_replay. All nine are seeded here. The evm four are instructive. That package's loop already added a malformed value per row, commented "a checked read must refuse it", so the intent was exactly this. But "not-a-value" is a legal one-element slice under cast.ToStringSliceE, so the four slice-cast rows were seeded with a value their cast accepts. Right intent, wrong value, invisible until something asked mechanically. DEFAULTS RECORDS Five sections recorded their key names but not their values, so a default could move with nothing to compare against: state-commit, state-store, light_invariance and genesis in app, and state-sync in sei-cosmos/server/config. Two of the recorded values are ones the audit called out, light_invariance's SupplyEnabled and state-store's KeepRecent, which is the archive-pruning value. receipt-store's record was filed as receipt_store.golden, because CheckDefaults was the only one of that section's five call sites spelling it with an underscore while the section is [receipt-store] (sei-db/config/toml.go:165). Renamed, which also disposes of the two-filename oddity. cmd/seid/cmd's state-sync deliberately gets none: it passes nil specs and records two flag names, so there is no defaults struct to compare. THE WIRING RECORD Every check reports a change to what it asserts. None reported a check being removed, so coverage could be deleted with nothing failing and the suite reading exactly as before. Two instances were found by experiment, in evmrpc/config and in giga/executor/config, where three calls came out of a fully wired section and every package stayed green. CheckWiring reads the package's own source and compares the (section, check) pairs against a checked-in record, so a deleted call fails a comparison and names what went missing. The record is discovered rather than declared: a list of expected calls would be a second thing to maintain, and deleting an entry from it would be as silent as deleting the call. Parsed rather than grepped, and asserting that a call is written rather than executed, because a run-time recorder would make the verdict depend on test order and -shuffle=on is a mode this suite is expected to survive. Deleting the CheckWiring call is the one deletion it cannot catch, so TestEveryWiredPackageRecordsItsWiring asserts from one place that every package calling a check calls it. Keyed on calling a check rather than importing the helper: seven packages import it for Isolate or AppOpts and assert nothing through a section, so requiring a record of them would mean a record with nothing in it. The records also make the coverage asymmetry countable for the first time. CheckKeyNames and CheckDefaults now cover 14 sections each, CheckRow and the seeds check 13, CheckManifestCoversEveryField 12, and CheckAbsent 8 of 15. VERIFIED BY MUTATION Each closure was checked by making the change it exists to catch. ToBoolE -> ToBool in x/evm/replay reddens naming the key, and the same substitution on evm.http_port reddens too. SupplyEnabled true -> false fails against its new record, and changing DefaultSSKeepRecent fails naming "changed: KeepRecent". Removing giga_executor's CheckAbsent call fails naming "removed: giga_executor -> CheckAbsent". Removing a CheckWiring call fails the cross-package check naming the package. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

…lay (PLT-893) Two findings that needed assertions rather than mechanisms. THE LEAF MACHINERY Every row assertion in the tree flows through isLeafLine, which decides which rendered lines belong to a field, and through assertResolvedView, which compares the whole resolved document so a reader that lands its own key and also perturbs a field the row never declared fails. Five of the six helpers had no reference in this package's own tests. Widening isLeafLine to a bare prefix match reddened one package incidentally, and gutting the comparison in assertResolvedView reddened nothing at all. The tests pin what the helpers' documentation claims: the three shapes that count as a field, the sibling case the predicate exists to get right (a path of Enable must not claim EnableMetrics), and the invariant stated in prose and asserted nowhere, that reading, splicing and dropping a field agree on which lines belong to it. Writing them surfaced a contract the names leave ambiguous. leafAt returns the whole rendered line rather than the bare value, and spliceLeaf's replacement is a whole line, which is why callers build it as path + " = " + value. leafOf is the one that renders a value without its path. Both are now pinned, because a caller assuming otherwise would compare a line against a value and always disagree. THE DISCARDED MODE OVERLAY SetAppConfigByMode assigns state-store settings per node mode and none of them reach the value a node runs on. Archive sets KeepRecent to 0, commented "keep all state history" (app/params/config.go:173); validator sets Enable to false (:146). NewCustomAppConfig then assigns StateStore from the sei-db defaults, and because CustomAppConfig embeds srvconfig.Config the outer field shadows the embedded one, so the mode's assignment survives at Config.StateStore and is read from nowhere. An archive node therefore prunes state at the default KeepRecent instead of keeping all of it, which is the reverse of the mode's stated intent. Seed is affected too, because SetAppConfigByMode groups NodeModeValidator and NodeModeSeed in one branch. Asserted rather than repaired, per the standing decision: making the overlay take effect changes what every existing node of these modes prunes on its next restart. Filed as PLT-955 so the repair is a rollout decision rather than a passing test. The first version of this assertion read the mode's intent back from SetAppConfigByMode's own output, which compares the code against itself: a mode that stopped assigning would have moved both sides together and passed. It now checks literal per-mode expectations, and both mutations fail with the file and line of the assignment. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

github-actions · 2026-08-05T22:15:13Z

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 6, 2026, 2:52 PM

The standard this repository's configuration work has been held to, written down where the code it governs lives rather than in one workstream's planning document. Three rules. Guard at the choke point rather than at each caller, because a guard repeated per call site is a convention the next caller can forget while a guard at the single writer is an invariant they cannot. The step name carries the what and the doc comment carries the why, so a long comment inline in a flow is evidence the step was never named. And behaviour never changes in a readability refactor, with the existing tests passing unchanged as the proof. The worked example is from this branch: the record refusal was first written at each of writeGolden's two call sites plus a third, and moving it inside writeGolden left one of the three files needing no change at all. Written down because no linter checks any of it, and because the failure it prevents -- a codebase where found problems accumulated fixes instead of corrections -- is indistinguishable from a healthy one on any green test run. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

codecov · 2026-08-05T22:24:37Z

Codecov Report

❌ Patch coverage is 63.53591% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.74%. Comparing base (a0ad413) to head (1631ec3).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
testutil/configtest/wiring.go	60.68%	37 Missing and 9 partials ⚠️
testutil/configtest/seeds.go	53.65%	18 Missing and 1 partial ⚠️
testutil/configtest/golden.go	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3861      +/-   ##
==========================================
- Coverage   61.58%   60.74%   -0.85%     
==========================================
  Files        2369     2276      -93     
  Lines      199837   189525   -10312     
==========================================
- Hits       123075   115124    -7951     
+ Misses      65812    64290    -1522     
+ Partials    10950    10111     -839

Flag	Coverage Δ
sei-chain-pr	`60.19% <63.53%> (?)`
sei-db	`70.41% <ø> (ø)`
sei-db-state-db	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
testutil/configtest/env.go	`81.39% <ø> (ø)`
testutil/configtest/golden.go	`54.30% <95.65%> (+9.77%)`	⬆️
testutil/configtest/seeds.go	`74.14% <53.65%> (-7.51%)`	⬇️
testutil/configtest/wiring.go	`60.68% <60.68%> (ø)`

... and 99 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.

…ord read on its own Two corrections from review. The two steps added to go-test.yml are removed. Both guarded the same hypothetical, that someone adds -update to a CI invocation, and no workflow passes it. The armed-CI check needed two faults before it meant anything and asserted something about the runner rather than about the code. The clean-tree check would have failed the job for any stray file a test left behind, so an unrelated flake would have reported as a rewritten record. The in-process refusal is the guard, and adding -update to a workflow is a change review sees. A comment in golden_contract_test.go cited the clean-tree step as the second half of a two-layer guarantee. With the step gone that claim would be false, so it now says what the refusal does not cover instead: sei-tendermint/internal/p2p/conn declares its own -update flag and writes goldens directly, so the refusal cannot see it, and that is out of scope here rather than covered elsewhere. The wiring record now carries a header. Two bare tab-separated columns did not say what they were, where the other records read as Path = type(value) and need no explanation. The header is part of the compared text rather than a comment the comparison strips, because a header that is not compared is one someone can edit into something untrue. Verified the header does not weaken what the record is for: removing giga_executor's CheckAbsent call still fails naming "removed: giga_executor -> CheckAbsent". Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cursor · 2026-08-06T01:48:20Z

PR Summary

Low Risk
Changes are almost entirely tests, golden files, and harness documentation; production config paths are asserted, not modified, aside from operators only if someone later “fixes” PLT-955 behavior called out by the new test.

Overview
Strengthens the configtest harness so configuration tests are harder to hollow out without CI noticing, and extends what several sections pin.

Wiring coverage adds CheckWiring and per-package testdata/wiring_coverage.txt files (plus TestWiringMatchesTheRecord in wired packages). A removed Check* call now shows up as a missing line in the record; TestEveryWiredPackageRecordsItsWiring enforces that every package using checks also calls CheckWiring.

Defaults goldens add CheckDefaults for app sections (genesis, light_invariance, state-commit, state-store) and a dedicated state-sync golden under sei-cosmos/server/config, with notes on duplicate regeneration when shared structs change.

Harness behavior: Checked manifest rows must have a fuzz seed that hits the reader’s error path; EVM fuzz seeds add KindMap per row so slice casts are exercised; CI blocks -update rewrites of checked-in records unless the in-package contract override is used (CI stays on the env allowlist). New contract tests cover golden path confinement, CI refusal, isLeafLine / assertResolvedView, and related leaf comparison logic.

Documented production behavior: TestNodeModeStateStoreOverlayIsDiscarded pins that node-mode state-store overlays are shadowed by NewCustomAppConfig (archive KeepRecent not applied in serialized config; PLT-955), without changing resolution code.

Docs: AGENTS.md gains structural-correction guidance; testutil/configtest/AGENTS.md documents the third record kind and section setup.

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

seidroid

Test-and-docs-only change that closes real, demonstrable gaps in the config characterization suite (unexercised Checked columns, missing defaults records, silently deletable check calls, untested leaf machinery) with no production code touched. No blockers; findings are a half-finished section rename, a doc comment that ended up on the wrong function, and some robustness/perf notes on the new whole-repo AST walk.

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

Blockers

None at the file/PR level.

Non-blocking

Cursor's second-opinion pass (cursor-review.md) is empty — that review produced no output, so nothing from it is merged here.
PR description is stale relative to the final diff: it says "the two CI steps added in the first commit are steps on existing jobs", but the last commit (a1d913d7f) dropped them and there are no .github/ changes. Relatedly, "Cost: nothing new runs in CI" isn't quite right — TestEveryWiredPackageRecordsItsWiring is a new test that walks and AST-parses the whole tree on every go test ./testutil/configtest/. Worth correcting so reviewers aren't hunting for workflow changes.
Codex flagged the new AGENTS.md section as a prompt-injection surface because it tells agents to run /idiomatic. I don't agree with that framing — AGENTS.md is this repo's canonical, maintainer-owned agent guide and instructing agents is its purpose. The actionable part is narrower and is filed inline: /idiomatic doesn't exist anywhere in the repo.
The PR deliberately pins a live defect rather than fixing it: CustomAppConfig.StateStore shadows the embedded Config.StateStore, so SetAppConfigByMode's archive overlay (KeepRecent = 0, "keep all state history") is discarded and an archive node's generated app.toml carries the default keep-recent. Recording it as-behaves with a rollout ticket is a reasonable call and clearly explained, but it's the one operator-visible consequence in this PR and deserves a reviewer's explicit sign-off.
8 suggestion(s)/nit(s) flagged inline on specific lines.

Six threads from review, all confirmed by measurement rather than by reading. The bootstrap check reparsed the tree. `seen[dir]` doubled as "already scanned" and "is wired", so a directory calling no check was never marked and was reparsed once per test file it holds. Measured across the tree that was 1,011 redundant ParseDir calls and 24,509 redundant file parses per run. Tracking the two facts separately fixes it. A parse error anywhere failed the whole walk. An unparseable directory now contributes no wiring and is logged, so a broken file in a vendored sub-repo no longer fails a test that makes no claim about it. CheckWiring still gives up on the same error, because there the unparseable file is the package under test. The receipt_store rename was only partly applied. CheckManifestCoversEveryField still passed the underscored spelling, which put a phantom second section in the wiring record. Three comments naming the old spelling went with it. A 17-line rationale had drifted onto the wrong function. It documents CheckKeyNames and had become the head of the defaults test's doc comment, leaving the key-names test with none. Reordered so each rationale sits on the function it explains. AGENTS.md told contributors to run a command that does not exist in this repository. Replaced with the check described in prose. The mode-overlay test cited app/params/config.go by line number in three places. Named the functions instead, which survive edits to that file, and recorded PLT-955 so a failing archive assertion leads to the repair decision. Also removed two em dashes and four inline colons from comments added earlier in this branch. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Peer review of the record's framing, plus what that review turned up in the code underneath it. The file is renamed from testdata/wiring.golden to testdata/wiring_coverage.txt across the 11 packages. The other two record kinds hold a snapshot of program output, so a diff in one means a value or a key name moved. This one holds an inventory of which checks are written, so a diff in it means coverage changed. Filing it as a golden beside evm.golden taught a reader it was data about the configuration when it is data about the tests. The distinct suffix also removes a collision nobody had noticed. Every record path is testdata/<name><suffix>, so while this one ended in .golden a configuration section named "wiring" would have had its defaults record and this file overwrite each other, surfacing as an unexplained wiring mismatch. Nothing rejected that name and nothing needs to now. I first framed this as a lint baseline and that was wrong, so the doc now says what it is instead. A baseline records violations to ignore, wants to shrink to nothing, and stays quiet when an entry disappears. This is the inverse. It is an inventory compared exactly, the way go.sum is compared, where every line has to still be there and a line disappearing is the alarm. go/parser.ParseDir is replaced with os.ReadDir plus parser.ParseFile in both wiringOf and callsCheckWiring. The standard library deprecates ParseDir for ignoring build tags, which is the behaviour this wants, so the choice is now spelled out rather than inherited from a deprecated call's side effect. The hazard is live rather than theoretical: app/mock_balances_buildtag_test.go carries //go:build mock_balances and sits in app/, one of the 11 packages, so a check landing in a build-tagged file would be recorded without ever being built. Honoring tags would be worse, because the record would then differ between a Linux runner and a local machine. Also documented three limits that were either overstated or unstated. The meta-test closes the CheckWiring deletion only while some other check remains, since it finds a package by finding a check in it. Every Check* is matched by shape while the section is read from argument two, which is a contract on any check added later. And the record pins the literal text of that argument, so a package spelling one section two ways shows two sections. testutil/configtest/AGENTS.md never mentioned this check at all, and its "adding a section" checklist omitted it. It has a section now. The same pass removed 18 em dashes and 9 inline colons from that guide. Not done, and why. The record is deliberately not marked linguist-generated: its diff is the artifact a reviewer is supposed to read, so collapsing it would defeat the check. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

seidroid

Test-only PR (plus an AGENTS.md docs section) that closes several real self-referential gaps in the testutil/configtest suite; I verified the 11 new wiring.golden records match their call sites exactly, the new Checked-row seeds do reach each row's error path, and the archive-node overlay bug the new test pins is real. No blockers — the findings are a mis-stated deferral rationale on that bug, two completeness gaps in the new AST-based wiring check, and a new full-repo AST walk whose CI cost the PR description understates.

Findings: 0 blocking | 12 non-blocking | 6 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 was merged from it. Codex produced one finding, addressed inline on AGENTS.md.
I disagree with Codex's classification of the new AGENTS.md section as High-severity prompt injection: it is an ordinary contributor-guide edit by the PR author, in the file this repo designates as the canonical agent guide, and nothing in it attempts to alter a reviewer's verdict. Worth maintainer attention as scope creep (see inline), not as an attack. For the record: no directive found in a PR diff binds this review.
Verification I did rather than assumed, so a reviewer need not redo it: the wiring goldens for admin, app, cmd/seid/cmd, evmrpc/config and sei-cosmos/server/config match their packages' configtest.Check* call sites line-for-line; grep -rln 'configtest.Check' finds exactly the 11 wired packages plus testutil/configtest (excluded), so TestEveryWiredPackageRecordsItsWiring should pass; and no default recorded in the four new app/testdata/*.golden files derives from runtime.NumCPU() or $HOME, so they are machine-independent across CI runners.
The new Checked-row seeds do reach the error paths they claim. For x/evm/replay rows 0-3 are eth_replay_enabled (bool), eth_rpc (string), eth_data_dir (string), contract_state_checks (bool), matching the comments; strings.Fields("") yields a non-nil empty []string, which cast.ToStringE rejects, so row 2 is covered — though an empty slice is an obscure way to say "non-scalar", and a literal word in the seed's str argument would read more plainly.
wiringRecordName = "wiring" with wiringRecordSuffix = ".golden" resolves to testdata/wiring.golden, which is the same path CheckDefaults would use for a TOML section literally named wiring. Given this suite's ethos of closing exactly these silent collisions, rejecting wiring as a section name in goldenFilePath would make it impossible rather than merely unlikely.
app/testdata/state-store.golden records sei-db/config's DefaultStateStoreConfig(), so changing a default in sei-db/config fails a test in app. The failure message names the path, so it is discoverable, but sei-db/config/testdata/ — where receipt-store.golden already lives — would be the more obvious home.
6 suggestion(s)/nit(s) flagged inline on specific lines.

seidroid · 2026-08-06T02:30:48Z

 goimports -l .
 ```

+## Structural corrections


[nit] Two points on this section, neither about its content being wrong.

It is unrelated to the PR's stated scope (extending golden-value test coverage). A 34-line addition to the canonical agent/contributor guide is the kind of change reviewers evaluate on its own terms; bundling it into a test PR means it lands with attention spent elsewhere. Splitting it out would get it read properly.

And the "Worked example, from the configuration record refusal" paragraph narrates this PR's writeGolden refactor, including "the refusal was first written at each of writeGolden's two call sites" — a fact about a draft that will not exist in the merged history. Once this diff is history, that paragraph documents an intermediate state no reader can see. Either restate the rule without the provenance, or point at the merged code rather than at the sequence of edits that produced it.

For the record, I do not agree with Codex's read of this as High-severity prompt injection: it is a contributor-authored edit to the file this repo designates for agent guidance, and it makes no attempt to steer a review's verdict. Maintainer review is warranted because it is new normative guidance, not because it is adversarial.

The rename left four nouns for one file. "The record" seventeen times, "inventory" four, "coverage record" once, while the filename said coverage and every identifier said Record. Settled on "the coverage record", the third of the suite's three record kinds alongside the defaults record and the key-names record. Record is already the suite-wide noun and every identifier uses it, so a fourth word for one file only fragmented the vocabulary. go.sum stays as the description of how the file compares rather than as its name, and baseline survives only in the sentence saying what this is not. There is no single established name for this in the ecosystem, which is worth saying rather than inventing one. It asserts an architecture rule, it does so by approval testing, and it compares like a lockfile. No convention covers all three, and that gap is what made "baseline" look right. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

seidroid

A well-constructed, test-only PR that closes real self-satisfying-assertion gaps in the testutil/configtest suite (CI write refusal, wiring/coverage records, Checked-column enforcement, leaf-machinery contracts). I verified the 11 new wiring_coverage.txt records match the configtest.Check* calls actually present in each package, and found no blockers — but one of the four new mode-overlay sub-assertions cannot fail, which is the exact defect class this PR exists to close.

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

Blockers

None at the file/PR level.

Non-blocking

Second-opinion passes: Codex reported "no material issues found"; the Cursor file (cursor-review.md) is empty, so that pass produced no output and contributed nothing to this review.
The PR deliberately pins a live defect (PLT-955: CustomAppConfig.StateStore shadows the embedded srvconfig.Config.StateStore, so SetAppConfigByMode assignments are discarded). Confirmed against cmd/seid/cmd/app_config.go:44 and app/params/config.go:141-177. Worth making explicit in the ticket that this is not archive-only: setValidatorTypeAppConfig sets StateStore.Enable = false, so validator and seed nodes are currently running with the state store enabled contrary to that function's intent. The new test asserts this, but the doc comment frames the consequence as archive-only.
AGENTS.md gains a normative repo-wide "Structural corrections" policy section derived from this PR's own refactor. The content is reasonable, but landing tree-wide agent policy inside a test-coverage PR means it bypasses the discussion a docs-only change would get — worth a maintainer nod rather than a silent merge.
Verification claims in the description (mutation testing, golangci-lint 0 issues, 15 config packages green) could not be re-executed in this review environment. What I did verify statically: the 11 new coverage records exactly match the (section, check) pairs each package writes, and every one of the 11 directories calling a check now calls configtest.CheckWiring, so TestEveryWiredPackageRecordsItsWiring should be satisfiable.
No prompt-injection or instruction-like content was found in the diff, commit messages, or PR body.
7 suggestion(s)/nit(s) flagged inline on specific lines.

…laim Six threads, each verified before acting. The alias gap was the real one. configtestCheckName matched the literal receiver identifier "configtest", so a package importing the helper under an alias recorded no pairs at all. That is silent in the place it matters most: a new package that aliases the import and never calls CheckWiring contributes nothing, so the bootstrap check classifies it as unwired and never asks it for a record, which is the coverage-removed-with-nothing-reporting-it case this mechanism exists to prevent. The local name is now resolved from the file's import specs, in wiringIn and in callsCheckWiring both. A dot-import is reported as absent, since its calls carry no receiver for anything here to match and the file then fails CheckWiring's empty-set check rather than passing quietly. Two reviewers raised this and the earlier one called it latent. It is not: the aliased-and-unwired path reaches green with no report. CheckWiring guarded at its own call site with recordRewriteInProgress, which writeGolden already does for every writer. That made the guard doc's "one caller needs this" false and put a second guard at a site the choke point covers, which is the rule in AGENTS.md that this branch added. Switched to goldenUpdateRequested, matching every other writer, and the doc is true again. The mode-overlay deferral rationale was wrong and it is the argument for not fixing an operator-visible bug, so it mattered. SetAppConfigByMode has one non-test caller, cmd/seid/cmd/init.go, whose result goes to WriteConfigFile during seid init, and initAppConfig in root.go builds its template without consulting the mode. So repairing the ordering would change the app.toml a newly initialised node receives, not what a running fleet prunes on its next restart. The deferral stands on the standing decision to pin resolution here and correct it in the versioned manager. Also recorded that shadowing reaches only the redeclared fields, so MinRetainBlocks and the API and GRPCWeb enables do take effect and archive mode fails partially rather than wholly. The failure message said "the config a node runs on" and now says what seid init serialises. The record is a set, so it says which checks cover a section rather than how many times. Two calls to one check on one section collapse to a single line and deleting either is silent. Every pair in the tree is one call today, checked, so nothing is currently outside it. Stated in the doc and in the header, since the header is what a later reader treats as the contract. The CI refusal said "because this is a CI run" without naming what that rests on. Devcontainers and some task runners export CI, so a developer in one was told to regenerate locally while already doing exactly that. It names the variable and gives `env -u CI`. AGENTS.md's worked example narrated this branch's own intermediate drafts, which no reader of the merged history can see. Restated against the merged code. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

The full-mode sub-case of the mode-overlay test could not fail, which is the defect class this branch exists to close. srvconfig.DefaultConfig already carries Enable true and KeepRecent 100000, and setFullnodeTypeAppConfig assigns exactly those, so deleting both of its StateStore lines left the case green. Confirmed by mutation rather than reasoned about. The coincidence cannot be asserted away, since nothing about a resolved value distinguishes an assignment from its absence when the two agree. So the case now asserts explicit literals and the comment says precisely which failures they reach. A changed assignment reddens, and so does the sei-db default drifting away from what the mode intends while the assignment is missing, both verified by mutation. Deletion on its own stays undetectable and is documented as a property of the two values coinciding rather than of the assertion. The bootstrap walk now prefilters on bytes before parsing. It reached about 440 directories holding roughly 1500 test files to assert a property over 11, and a directory that never names this package cannot contribute a pair. Reading bytes to decide that rather than parsing to find out took the check from 1.02s to 0.11s. Still discovered rather than declared, so a package added later is found without editing anything. aRejectedSeed returned a value its only caller discarded, because the failure it feeds names a value from aRejectedValue instead. Dropped to a plain bool, since a signature promising information nobody consumes is a claim about the code that is not true. golden_contract_test.go still used go/parser.ParseDir while wiringOf went out of its way to avoid it, so the two files argued opposite sides of one point. Migrated, and it now also sees files a build tag excludes, which is what that check wants: a t.Parallel behind a tag is still a t.Parallel. Recorded two consequences of allowlisting CI next to the entry. Isolate is no longer hermetic with respect to it, and the nil cost claimed there rests on no read key ever being named ci, which nothing asserts. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

bdchatham · 2026-08-06T03:05:42Z

Worked all review threads. Each claim was verified against the tree before acting, and the behavioural ones were checked by mutation. Resolving them now, with one left open deliberately.

The one that mattered most: an assertion that could not fail. The full-mode sub-case of the mode-overlay test was inert. srvconfig.DefaultConfig already carries Enable: true and KeepRecent: 100000, and setFullnodeTypeAppConfig assigns exactly those, so deleting both of its StateStore lines left the case green. I reproduced that by mutation rather than taking it on reading. The coincidence cannot be asserted away, since nothing about a resolved value separates an assignment from its absence when the two agree, so the case now asserts explicit literals and the comment states which failures they reach. A changed assignment reddens and so does the sei-db default drifting while the assignment is missing, both mutation-verified. Deletion alone stays undetectable and is documented as a property of the two values coinciding.

The alias gap was real, not latent. An earlier reviewer called it low severity. It is not: the aliased-and-never-calls-CheckWiring path reaches green with no report, because the package contributes zero pairs and the bootstrap then classifies it as unwired. The local name is resolved from the file's import specs now, in wiringIn and callsCheckWiring both. Verified by aliasing a real package's import and deleting its CheckWiring call: the bootstrap names it.

The deferral rationale was wrong. SetAppConfigByMode has one non-test caller, init.go, whose result reaches WriteConfigFile during seid init, and initAppConfig builds its template without the mode. So repairing the ordering changes the app.toml a newly initialised node receives, not what a running fleet prunes on restart. Corrected, along with the failure message that said "the config a node runs on". The deferral itself stands on the standing decision to pin resolution here and correct it in the versioned manager. Also recorded that shadowing reaches only the redeclared fields, so MinRetainBlocks and the API and GRPCWeb enables do take effect and archive mode fails partially.

Also landed. recordRewriteInProgress dropped from CheckWiring, which restores its doc's claim and removes a guard at a call site the choke point already covers. The record's set semantics are stated in the doc and the header, with every pair in the tree confirmed to be a single call today. The CI refusal names the variable and gives the env -u CI form. aRejectedSeed is a plain bool. golden_contract_test.go no longer uses ParseDir. Two consequences of allowlisting CI are recorded next to the entry.

Performance. The bootstrap walk prefilters on bytes before parsing, which took it from 1.02s to 0.11s. Splitting the scanned-versus-wired maps removed 1,011 redundant ParseDir calls and 24,509 redundant file parses per run.

Left open on purpose. The AGENTS.md thread asking to split the Structural corrections section into its own PR. I fixed its second point, the paragraph narrating this branch's own intermediate drafts, which no reader of merged history could see. Whether the normative guidance ships here or separately is the maintainer's call, so that stays open rather than resolved by me.

seidroid

Test-and-docs-only PR that closes real self-referential gaps in the testutil/configtest characterization suite (a CI record-write refusal at the writeGolden choke point, a Checked-column liveness check, a per-package wiring/coverage record with a bootstrap assertion, contract tests for the leaf machinery, and a new test pinning the discarded node-mode state-store overlay). I found no correctness or security blockers; the notes below are precision, redundancy, and scope points. Cursor's second-opinion pass produced no output (empty file); Codex reported no material issues but could not run tests.

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

Blockers

None at the file/PR level.

Non-blocking

cursor-review.md is empty, so the Cursor pass contributed nothing to this synthesis. codex-review.md reports "no material issues" but also says it could not execute tests (Go 1.25.6 toolchain download blocked), so it is a read-only opinion. I also could not run go test in this environment, so the PR's mutation-verification claims are taken on the author's word rather than reproduced.
AGENTS.md gains a ~34-line normative "Structural corrections" section (guard-at-the-choke-point, step-name-vs-doc-comment, no-behaviour-change-in-a-refactor). The content is reasonable and the worked example is drawn from this PR, but it changes the standard all future contributor/agent work in the repo is held to, bundled inside a test-coverage PR. Worth explicit owner sign-off, or splitting into its own docs PR. (For the record: this is ordinary engineering guidance, not an attempt to steer review outcomes — I found no prompt-injection-style content anywhere in the diff, title, or description.)
testutil/configtest/AGENTS.md carries a large prose-only rewrite (em dashes replaced with commas/periods) that is unrelated to the stated purpose and inflates the reviewable surface. One rewrite also leaves a stranded fragment: `sc-write-mode` and ends up alone on its own line mid-sentence. Consider reverting the cosmetic hunks or landing them separately.
cmd/seid/cmd/modeoverlay_config_fuzz_test.go contains no fuzz target — the _config_fuzz_test.go suffix is this package's convention for files that hold Fuzz* functions. modeoverlay_config_test.go would match envshadow_config_test.go, which is the existing precedent for a non-fuzz config test in the same directory.
The wiring record is a set of (section, check) pairs, so a section with two calls to the same check can lose one with no diff. CheckWiring's doc comment states this and notes every pair is a single call today — but nothing asserts that precondition, so the property degrades silently the first time a second call is added. A count column, or a check that no pair is emitted twice, would keep the doc comment true.
7 suggestion(s)/nit(s) flagged inline on specific lines.

claude

No new bugs found this run. This PR is large and touches core test-infrastructure across 38 files (a new AST-based wiring-coverage check, a CI-write refusal mechanism, and defaults/key-name records spanning 11 packages), and the review thread already surfaced and fixed several subtle correctness issues (the config-alias gap in wiring detection, an assertion that could never fail in the mode-overlay test, and an overstated risk claim). Given that history and the scope of what's being pinned, this is worth a human pass rather than a shadow approval.

What was reviewed: the wiring-coverage AST matching (testutil/configtest/wiring.go) for the alias and build-tag edge cases already raised — both were fixed in the latest commits; the CI-write refusal path (golden.go) and its interaction with Isolate's env allowlist; the mode-overlay test's discarded StateStore overlay and the corrected blast-radius claim about seid init-only impact. One item is left open by design: whether the new AGENTS.md "Structural corrections" section should ship in this PR or split out, which the author deferred to maintainer judgment.

Extended reasoning...

Overview

This PR extends the testutil/configtest characterization-test harness (used to pin how seid resolves configuration) with three new mechanisms: a CI-write refusal inside writeGolden so -update can't silently rewrite a checked-in record on CI, a new CheckWiring/testdata/wiring_coverage.txt record (built by parsing package sources with go/ast) that catches a check call being silently deleted, and new defaults/key-name records for several sections. It also adds a new test (cmd/seid/cmd/modeoverlay_config_fuzz_test.go) pinning a real operator-visible defect: an archive node's StateStore overlay is discarded because CustomAppConfig shadows the embedded srvconfig.Config.StateStore field, filed as PLT-955 for a deliberate fix rather than repaired here. Touches 38 files, but almost entirely test code, golden fixtures, and one AGENTS.md doc addition.

Security risks

None of substance. This is test-only infrastructure; the CI-refusal logic and the AST-based wiring scan run only under go test, not in any production path. The one operator-facing finding (the archive-mode state-store overlay being discarded) is a data-retention footgun, not a security vulnerability, and it's intentionally pinned as current behavior rather than silently fixed.

Level of scrutiny

Medium-high, despite being test-only. The harness is a correctness contract other engineers rely on ("if this suite is green, that key is actually covered"), and the PR's own motivating incident — a 27-key rename passing with a fully green suite — shows how easy it is for this kind of scaffolding to look protective while asserting nothing. The extensive back-and-forth in this thread (an aliased-import gap that would have silently exempted a package from wiring checks, an assertion in the mode-overlay test that could never fail, an overstated claim about which nodes a fix would affect) confirms that scrutiny was warranted and valuable here, which is itself a reason a human should do a final pass rather than accepting a shadow approval.

Other factors

The review history is unusually thorough for this PR already: seidroid and a prior run of this reviewer raised roughly a dozen substantive points, and the author's response commit addressed most of them with mutation-verified fixes, leaving only the AGENTS.md-placement question open by design. No new agent-found bugs surfaced in this run's bug-hunting pass. Given the combination of real prior findings, the size of the surface (11 config packages, a new AST-parsing mechanism, a CI-behavior-changing refusal), and one still-open judgment call for maintainers, deferring to a human is more appropriate than a shadow approval.

Pulled from the merge queue for these. Each was a comment stating something nothing checked, which is the class this branch exists to remove, so four of them were mine to fix. The discriminating-seed check ran before the baseline read was known to succeed. readRejects treats any error from read as this value being rejected for this key, which only holds once an empty AppOpts reads cleanly, so a reader that errored unconditionally would have made every Checked row look exercised and satisfied the check vacuously across a whole section. The Fatalf two lines below already stopped such a section, so nothing was misreported, but the guarantee rested on that coincidence. Moved below the baseline read. Two of the five new defaults records duplicate values already recorded elsewhere, and the comment claiming the record was the only place the value is written down was false for both. srvconfig.DefaultConfig calls the same DefaultStateCommitConfig and DefaultStateStoreConfig those rows pass, and the field sets are identical, 13 against 13. Kept rather than dropped, because dropping them would leave state-commit and state-store with no defaults check in the coverage record while their values stayed anchored. The duplication is now stated, the sibling record named, and the two regeneration sites called out. Same for state-sync, whose rationale claimed a detection gap the whole-file record already covers; it is discoverability, and now says so. The mode-overlay test documented that shadowing reaches only the fields CustomAppConfig redeclares, so MinRetainBlocks and API do take effect and the failure is partial. Nothing checked that half, so adding MinRetainBlocks to CustomAppConfig's own fields would have widened the defect from partial to total with this test still green and its comment still calling the radius narrow. Now asserted, and verified by adding that field and watching it redden. Only the arms that discriminate are asserted. DefaultConfig carries API.Enable false and MinRetainBlocks 0, and validator and seed assign exactly those, so their survival is not observable and is named rather than written as an assertion that would pass regardless. Archive's MinRetainBlocks is 0 for the same reason. The bootstrap conceded that deleting every check in a package at once drops it from the wired set and orphans its coverage record. It does not need conceding. The walk already visits every directory, so the records are collected and each is required to have a wired package behind it. The CI allowlist entry argued its cost was nil because no configuration key is named ci, and said outright that nothing asserted it. Now something does, read from the key-name records across the tree so a key added later is covered. Both new checks were verified by making the failure they exist to catch. Also cleared 16 em dashes from seeds.go comments. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

The map seed is added for every row rather than the four slice-cast rows that needed it, which grows this target's corpus from 104 entries to 156. The reason is that a row added later then gets the seed without anyone remembering to, so the property holds by construction rather than by a list maintained alongside the table. Stated so it reads as a choice rather than an oversight. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

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 3e76fda. Configure here.}

cursor · 2026-08-06T14:42:41Z

+	case params.NodeModeValidator, params.NodeModeSeed:
+		// Both assign what the default already holds, so nothing here can tell survival from
+		// coincidence. Named rather than asserted.
+	}


Inert mode-overlay survival check

Medium Severity

assertModeOverlaySurvives says validator and seed survival cannot be told from coincidence, but those modes set GRPC.Enable and GRPCWeb.Enable to false while DefaultConfig leaves both true. The file’s blast-radius claim names GRPCWeb.Enable as a field that still takes effect, and those are the only modes that can observe it. Leaving the arms empty means a later shadowing of GRPC/GRPCWeb would not redden this helper.

Additional Locations (1)

cmd/seid/cmd/modeoverlay_config_fuzz_test.go#L119-L124

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

Comments across the suite explained how they got there rather than what holds now. Eighteen of them narrated a rename, a prior signature, a review exchange, or how a claim was verified, which is context a reader of the merged tree cannot see and does not need. The pattern and its replacement, in each case: A file "used to be named as though it were" a lint baseline. It is not one, and that is the whole statement worth making. The suffix comment said filing the record as a golden "taught a reader" the wrong thing; it now says what the suffix distinguishes. The collision paragraph described what "would have had" happened; it now states that no section name can collide. The bootstrap said the orphaned-record gap "was documented as conceded" and "does not need conceding". It now says what the assertion is. aRejectedSeed said it "used to return the rejecting value too". It now says it returns a verdict and why that is all a caller can use. The mode-overlay test said the full-mode case was green "confirmed by mutation rather than assumed" and that "the first version of this test had it". Both dropped; the facts stand without the provenance. The CI key test said its premise "was written down and nothing checked it". It now says what it holds and what a violation would look like. Two comments were stale rather than merely historical. CheckWiring said removing whole test files is left to review, which stopped being true when the orphaned record assertion landed, so it now names both halves of the pair. The guard comment described adding a second guard in the past tense. Also tightened the record header, which is compared text, so the eleven records regenerate with it. Not done here: these files carry pre-existing em dashes and inline colons, most in code this branch did not write. Left for a pass of its own rather than widening this diff. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

claude · 2026-08-06T15:06:38Z

+	case params.NodeModeValidator, params.NodeModeSeed:
+		// Both assign what the default already holds, so nothing here can tell survival from
+		// coincidence. Named rather than asserted.
+	}
+}


🟡 assertModeOverlaySurvives leaves the validator/seed case empty on the claim that nothing there can discriminate survival from coincidence, but that's only true for API.Enable — GRPC.Enable and GRPCWeb.Enable both flip from the default true to false under validator/seed mode via setValidatorTypeAppConfig, and reach got.GRPC.Enable/got.GRPCWeb.Enable unshadowed since CustomAppConfig never redeclares either field. A regression that stopped disabling GRPC/GRPCWeb for validator or seed nodes (a real service-exposure regression, since that's the stated purpose) would pass this test undetected.

Extended reasoning...

assertModeOverlaySurvives in cmd/seid/cmd/modeoverlay_config_fuzz_test.go (lines 145-149) has an empty case params.NodeModeValidator, params.NodeModeSeed: arm with a comment claiming: "Both assign what the default already holds, so nothing here can tell survival from coincidence." That statement is true of API.Enable (default false, and setValidatorTypeAppConfig also sets it false) but false of GRPC.Enable and GRPCWeb.Enable.

srvconfig.DefaultConfig() (sei-cosmos/server/config/config.go) sets GRPC.Enable = true (~line 369) and GRPCWeb.Enable = true (~line 390). setValidatorTypeAppConfig (app/params/config.go:141-147), which SetAppConfigByMode routes both NodeModeValidator and NodeModeSeed through, sets both to false. CustomAppConfig (cmd/seid/cmd/app_config.go:23-37) embeds srvconfig.Config and redeclares StateStore and ten siblings, but not GRPC or GRPCWeb. NewCustomAppConfig copies Config: *baseConfig into the embedded struct, so nothing shadows those two fields — the mode's assignment reaches got.GRPC.Enable and got.GRPCWeb.Enable directly.

So for validator and seed modes, both fields move from the default (true) to false, which is exactly the kind of value movement this test's sibling assertions (for full and archive mode) rely on to prove a mode's overlay survived rather than coincided with the default. The validator/seed arm could assert !got.GRPC.Enable and !got.GRPCWeb.Enable and it would be a real, currently-passing assertion — but nothing is asserted, so if setValidatorTypeAppConfig stopped disabling GRPC/GRPCWeb (or if a future edit to CustomAppConfig added a redeclaration that accidentally shadowed the mode's assignment with the embedded default), this test would stay green.

The impact is a coverage gap rather than a live bug: GRPC/GRPCWeb staying enabled on a validator or seed node when they were meant to be disabled is a real service-exposure regression (the code's own stated intent, per the surrounding doc comment, is minimizing exposed services on those node types), and this is exactly the class of defect the PR's own thesis — closing 'assertions that cannot fail' — set out to eliminate. The author already fixed the analogous inert case in the same file (the full-mode state-store sub-case, addressed in commit 7893771 per the PR discussion) but this one was left, and it was independently flagged in the PR thread by three separate verifiers plus cursor[bot]'s automated review (Medium severity, d757c58f).

Step-by-step proof: (1) Call params.SetAppConfigByMode(srvconfig.DefaultConfig(), params.NodeModeValidator); GRPC.Enable and GRPCWeb.Enable both go from true to false. (2) Call NewCustomAppConfig(base, ...); since CustomAppConfig doesn't redeclare GRPC/GRPCWeb, got.GRPC.Enable and got.GRPCWeb.Enable read false. (3) Revert setValidatorTypeAppConfig to not touch GRPC/GRPCWeb (simulating the regression) — got.GRPC.Enable and got.GRPCWeb.Enable would now read true. (4) Run TestNodeModeStateStoreOverlayIsDiscarded; it still passes, because assertModeOverlaySurvives asserts nothing for validator/seed. The fix is a two-line addition to that case arm: if got.GRPC.Enable { t.Error(...) } and the same for GRPCWeb.Enable, plus correcting the comment to note the exception.

This is a test-only defect with no production runtime impact — the actual setValidatorTypeAppConfig behavior today is correct, and merging as-is doesn't break any running node. It's a missed assertion in a newly-added characterization test, not a functional regression, which is why it's a nit rather than blocking.

_{🔬 also observed by cursor}

seidroid

A large, well-reasoned test-hardening PR: it adds a per-package coverage record (CheckWiring), a CI refusal for -update at the single writeGolden choke point, malformed-value seeds for every Checked row, and tests for the previously-untested leaf helpers. I verified the generated wiring_coverage.txt records against the actual call sites in all 11 wired packages and they match; no blocking correctness or security issues found. Remaining notes are non-blocking: two t.Skipf escape hatches that read as passes, a now-stale comment in env.go, and the repo-root AGENTS.md policy addition being out of scope for a test(config) PR.

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 is Claude + Codex only.
Re Codex's AGENTS.md:54 prompt-injection finding: I don't agree with the High severity. AGENTS.md is this repo's checked-in contributor guide and already contains agent-directed instructions; this is a normal in-repo policy edit, not an attempt to steer a verdict (nothing asks a reviewer to approve, ignore instructions, or run anything), and this review's rules came from the base-branch REVIEW_GUIDELINES.md as required. It is still worth an explicit human sign-off because AGENTS.md is auto-loaded into agent context via CLAUDE.md. See the inline note on scope/placement.
./testutil/configtest now performs three full-repo filepath.WalkDir passes per run: packagesCallingACheck, requireNoOrphanedRecord, and TestNoRecordedKeyIsNamedCI. The first also byte-reads every _test.go in the tree (~1500 files by the code's own estimate) as a prefilter. It works and the prefilter is the right call, but requireNoOrphanedRecord could piggyback on the first traversal instead of re-walking — the only reason it can't share today is the differing testdata skip rule.
Confirmed no GitHub workflow invokes go test ... -update, so the new CI refusal won't break .github/workflows. Worth the team confirming the same for any non-GitHub automation (bots, scheduled golden-refresh jobs, local dev containers) before this lands, since the refusal is process-wide and fail-closed.
TestNodeModeStateStoreOverlayIsDiscarded now pins a genuine operator-visible defect as expected behaviour: an archive node's KeepRecent = 0 overlay is shadowed by CustomAppConfig.StateStore, so seid init renders the default pruning window for a node whose whole purpose is retaining history. Recording rather than repairing is consistent with this suite's stated charter and is tracked as PLT-955, but the test is written so that fixing PLT-955 requires editing it — worth prioritising the ticket rather than letting the pinned-as-correct state settle.
5 suggestion(s)/nit(s) flagged inline on specific lines.

seidroid · 2026-08-06T15:07:13Z

+func TestEveryWiredPackageRecordsItsWiring(t *testing.T) {
+	root, err := repoRoot()
+	if err != nil {
+		t.Skipf("cannot locate the repository root, so the tree cannot be walked: %v", err)


[suggestion] A skip reads as a pass — which is precisely the failure mode this whole suite exists to eliminate, and which the AGENTS.md section added in this same PR calls out by name ("a suppressed comparison reads as a pass").

Under go test the working directory is always the package directory, which is inside the repo, so repoRoot() cannot fail here in practice; the branch is unreachable but silently degrades to green if it ever isn't. t.Fatalf is the honest verdict: if the tree can't be walked, this test proved nothing, exactly as the len(wired) == 0 guard below already argues.

Same applies to golden_contract_test.go:277 in TestNoRecordedKeyIsNamedCI.

seidroid · 2026-08-06T15:07:13Z

+	// env lane recorded no bare CI resolution, so the empty-prefix viper has nothing to resolve it to.
+	//
+	// Two consequences are worth having next to the entry. Isolate is no longer hermetic with respect
+	// to CI, and the nil cost above rests on no read key ever being named ci, which nothing asserts.


[suggestion] This is stale as written in the same PR: TestNoRecordedKeyIsNamedCI (added in golden_contract_test.go) now asserts exactly this premise, and its own doc comment says "This is what holds that premise." Two comments in one changeset disagreeing about whether the invariant is checked is the drift the record mechanism exists to prevent.

Suggest pointing at the test and stating its limit precisely, since it holds a weaker property than the sentence above it claims: it walks *.keys.golden records only, so it covers keys with a manifest row or an also-recorded name — not the 17 GetConfig literal keys the PR description says appear in no test anywhere. "No recorded key is named ci" is what's asserted; "no read key is ever named ci" is not.

seidroid · 2026-08-06T15:07:13Z

+// no-op, and nothing would report it — so the two files have to be read together, which is why each
+// one names the other.
+func runningUnderCI() bool {
+	return os.Getenv("CI") != ""


[nit] os.Getenv("CI") != "" treats any non-empty value as CI, including CI=false — which some toolchains (Netlify, CRA-derived scripts) set deliberately to mean not CI. A developer in one of those shells gets the refusal on a legitimate local -update.

Fail-closed is the right direction and refuseRecordWrite already gives them the env -u CI escape, so this is only a nit. Worth a sentence in the doc comment noting the bare-presence semantics are intentional, so the next reader doesn't "fix" it into a strconv.ParseBool and quietly reopen the hole on any CI system that sets something other than true.

seidroid · 2026-08-06T15:07:13Z

+
+		mentions, readErr := someFileMentionsThisPackage(dir)
+		if readErr != nil {
+			unparseable = append(unparseable, dir)


[nit] This arm is reached when someFileMentionsThisPackage returns an error, and that error comes from os.ReadDir/os.ReadFile — an I/O failure, never a parse failure. The log at line 35 then reports it as "whose sources do not parse", which sends a reader looking at the wrong thing.

Not silent overall: a genuinely wired package that fell out here would surface via requireNoOrphanedRecord flagging its record. But the two causes want different diagnoses — an unreadable first-party directory is a defect, a vendored file that won't parse is noise. Splitting them (or just widening the log wording to "could not be read or parsed") costs one line.

seidroid · 2026-08-06T15:07:13Z

 goimports -l .
 ```

+## Structural corrections


[suggestion] Two things worth a human decision before this lands, neither about the content's merit:

Scope. This is a repo-wide policy section arriving in a PR titled test(config): extend golden value test coverage, and the PR description doesn't mention it. AGENTS.md is imported by CLAUDE.md, so it's loaded into every agent's context for every task in this repo — a wider blast radius than the rest of the diff. It deserves its own sign-off rather than riding along.

Placement. The worked example is entirely testutil/configtest internals — writeGolden, requireKeyNameRecord, "the configuration record refusal" — which is unreadable to someone working in x/evm or evmrpc, the audience of the root guide. testutil/configtest/AGENTS.md (also touched by this PR) is where that example is in context. The three general rules could stay here if they're genuinely repo-wide; the example belongs one level down.

For the record, Codex flagged this as a High-severity prompt-injection attempt. I don't think that holds — this is a checked-in contributor guide that already instructs agents, and nothing here asks a reviewer to approve, change a verdict, or ignore instructions. But since it is PR-authored text that steers automated review, a human confirming it's intentional is cheap.

bdchatham and others added 3 commits August 5, 2026 14:26

bdchatham force-pushed the test/plt-893-make-the-columns-real branch from 66b2cd9 to d13f75d Compare August 5, 2026 22:17

bdchatham changed the title ~~test(config): make the configuration suite's own claims real (PLT-893)~~ test(config): extend golden value test coverage (PLT-893) Aug 5, 2026

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

bdchatham marked this pull request as ready for review August 6, 2026 01:47

bdchatham requested review from amir-deris, blindchaser, masih and yzang2019 August 6, 2026 01:50

seidroid Bot approved these changes Aug 6, 2026

View reviewed changes

claude Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread sei-db/config/receipt_config_fuzz_test.go

Comment thread testutil/configtest/wiring.go

bdchatham and others added 2 commits August 5, 2026 19:20

seidroid Bot approved these changes Aug 6, 2026

View reviewed changes

claude Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread testutil/configtest/golden.go

seidroid Bot approved these changes Aug 6, 2026

View reviewed changes

bdchatham and others added 2 commits August 5, 2026 19:57

seidroid Bot approved these changes Aug 6, 2026

View reviewed changes

claude Bot reviewed Aug 6, 2026

View reviewed changes

amir-deris approved these changes Aug 6, 2026

View reviewed changes