test(config): pin the section-level env shadow, and make the parity comparison deterministic (PLT-775) by bdchatham · Pull Request #3837 · sei-protocol/sei-chain

bdchatham · 2026-08-01T00:15:33Z

Describe your changes and provide context

Two changes to the configuration characterization surface. Both land ahead of the [experimental] namespace work (design) so that neither rides on it, and both are reachable today without that feature existing. No production code changes.

Pin the section-level environment shadow

A non-empty SEID_<SECTION> variable makes every key beneath that section resolve to nothing, so the operator's written value in app.toml is discarded and the reader falls back to its in-code default.

This is not the environment overriding the file, which is the intended precedence. The variable's own value is never used — setting it to false gives the same answer as setting it to true, or to anything-at-all:

environment	`Get("giga_executor.enabled")`	reader resolves
(none), file says `false`	`false`	false — operator's value wins
`SEID_GIGA_EXECUTOR_ENABLED=true`	`true`	true — env over file, correct
`SEID_GIGA_EXECUTOR=true`	`nil`	true
`SEID_GIGA_EXECUTOR=false`	`nil`	true

The mechanism is viper's isPathShadowedInAutoEnv, which walks every proper prefix of a dotted key and returns before the config file is consulted. It cannot distinguish "SEID_GIGA_EXECUTOR names a scalar, so giga_executor.enabled cannot exist" from "that variable is unrelated to the key I was asked for". Enumeration is unaffected — AllKeys() still lists the shadowed keys — so a reader cannot infer "in effect" from "present".

Worth knowing what this reaches: giga_executor.enabled defaults to true, its reader is presence-guarded so a nil read keeps that default, and app.New feeds it to tmtypes.SkipLastResultsHashValidation.Store — which gates whether the node compares block.LastResultsHash against state.LastResultsHash. So an operator who writes enabled = false and has any non-empty SEID_GIGA_EXECUTOR in the process environment gets giga enabled and that comparison relaxed, with nothing in any log saying so.

These rows record the behavior and deliberately leave it alone. The legacy path has shipped, and changing how configuration resolves could silently break an operator who has come to depend on the current answer. SeiConfigManager corrects it when it owns resolution, and the divergence is then ratified against these rows rather than discovered against a production node — the pattern testutil/configtest/AGENTS.md already establishes for pinning current behavior, bugs included.

Make the legacy-vs-v2 settings comparison deterministic

TestConfigManagerLegacyVsV2Differential and its siblings compared Viper.AllSettings() in six places, and that comparison can fail on identical input. AllSettings re-nests the flat key space by splitting each key on ., so when one key is a dotted prefix of another — giga alongside giga.x — whether the scalar or the sub-tree survives depends on map iteration order. Measured at 43/157 across 200 reads of one file.

That comparison is premise three of the boot-parity argument, so a flake there quietly removes the safety net rather than failing loudly. And it is reachable today: the corpus already has an unknown-section row, and FuzzConfigManagerLegacyVsV2Parity appends arbitrary bytes to app.toml.

configtest.Settings keys on AllKeys and reads each key through Get, which is stable because Get tries longest prefixes first. It returns a flat map rather than a rendered string for two reasons:

values keep their concrete Go type, so int64(8) and "8" do not compare equal; and
a key is a map key rather than a line in a newline-joined document, so a key containing a newline — legal TOML, and reachable from the parity fuzz target — cannot make two different key sets compare equal.

DumpViper remains the right tool for a readable failure message; this is the right tool for the assertion.

Testing performed to validate your change

go test ./testutil/configtest/... ./cmd/seid/cmd/... — all three packages pass, including the six rewritten comparisons.
gofmt -s and goimports clean on all four touched files; golangci-lint run on both packages reports 0 issues.
The determinism test demonstrates the defect rather than asserting it: it reads one file 200 times and reports the distinct shapes each accessor produced. Settings is required to return exactly one; AllSettings's count is logged rather than required, because requiring instability would make the test depend on hitting both orderings within the sample — the same coin flip it documents.
The env-variable name is derived through configtest.ServerEnvPrefix/ServerEnvKey rather than built by hand. Building it by hand produced a false negative during review: the replacer runs over the whole prefixed name, so folding only the key and joining with an underscore yields SEID_GIGA.EXECUTOR, which matches nothing and shadows nothing.
Four rows cover the behavior and both of its boundaries — an empty value does not shadow, and a full-path variable delivers correctly — since only the set distinguishes the real rule from "the variable exists".

🤖 Generated with Claude Code

…omparison deterministic (PLT-775) Two changes to the configuration characterization surface, both ahead of the experimental-namespace work so that neither rides on it. Pin the section-level environment shadow. A non-empty SEID_<SECTION> variable makes every key beneath that section resolve to nothing, so the operator's written value in app.toml is discarded and the reader falls back to its in-code default. This is not the environment overriding the file, which is the intended precedence: the variable's own value is never used, and setting it to "false" gives the same answer as setting it to "true". viper's isPathShadowedInAutoEnv walks every proper prefix of a dotted key and returns before the config file is consulted, so it cannot tell "SEID_GIGA_EXECUTOR names a scalar, so giga_executor.enabled cannot exist" from "that variable is unrelated to the key I was asked for". The rows record the behavior and leave it alone. The legacy path has shipped, and changing how configuration resolves could silently break an operator who has come to depend on the current answer. SeiConfigManager corrects it when it owns resolution, and the divergence is then ratified against these rows rather than discovered against a production node. Make the legacy-vs-v2 settings comparison deterministic. The differential compared Viper.AllSettings() in six places, and that comparison can fail on identical input: AllSettings re-nests the flat key space by splitting on ".", so when one key is a dotted prefix of another, whether the scalar or the sub-tree survives depends on map iteration order. Measured at 43/157 across 200 reads of one file. The comparison is premise three of the boot-parity argument, so a flake there quietly removes the safety net rather than failing loudly. configtest.Settings keys on AllKeys and reads each key through Get, which is stable because Get tries longest prefixes first. It returns a flat map rather than a rendered string for two reasons: values keep their concrete Go type, so int64(8) and "8" do not compare equal, and a key is a map key rather than a line in a newline-joined document, so a key containing a newline — legal TOML, and reachable from the parity fuzz target — cannot make two different key sets compare equal. DumpViper remains the right tool for a readable failure message. The shape is reachable today from any hand-written app.toml section, without the experimental namespace existing, which is why this lands separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

cursor · 2026-08-01T00:16:16Z

PR Summary

Low Risk
Changes are limited to tests and harness documentation; production config behavior is recorded, not modified.

Overview
Test-only characterization for PLT-775: no production config resolution changes.

Adds configtest.Settings (flat AllKeys + Get map) with unit tests and AGENTS.md guidance to compare resolved vipers with DumpViper on failure instead of Viper.AllSettings(), which can flake on prefix collisions and drops keys where Get is nil.

Refactors configmanager_differential_test.go via requireSameChannels / requireSameSettings (nil Viper/Config guards) so all legacy-vs-v2 parity paths assert on Settings.

Introduces envshadow_config_test.go to document and lock in legacy behavior where a non-empty SEID_<SECTION> env var shadows every key under that section (file values ignored; variable value unused), including boundaries (empty env, full-key override, dashed section names) and impact on readers like giga_executor.

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

github-actions · 2026-08-01T00:17:16Z

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 1, 2026, 5:55 PM

codecov · 2026-08-01T00:19:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.40%. Comparing base (438cc04) to head (a14538c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3837      +/-   ##
==========================================
- Coverage   61.29%   60.40%   -0.90%     
==========================================
  Files        2351     2259      -92     
  Lines      197491   186873   -10618     
==========================================
- Hits       121060   112885    -8175     
+ Misses      65579    63988    -1591     
+ Partials    10852    10000     -852

Flag	Coverage Δ
sei-chain-pr	`50.18% <100.00%> (?)`
sei-db	`70.41% <ø> (+1.04%)`	⬆️
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/viper.go	`86.66% <100.00%> (+8.88%)`	⬆️

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

seidroid

Test-only PR that adds a configtest.Settings helper (stable flat-map replacement for Viper.AllSettings() in six parity assertions) and four rows pinning viper's section-level env shadow. The mechanism and the assertions check out against viper's find ordering and sei-cosmos/server/util.go; findings are limited to a stale doc comment, a small loss of failure loudness on nil vipers, and naming/independence nits.

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

Blockers

None at the file/PR level.

Non-blocking

cursor-review.md is empty (zero-byte) — the Cursor pass produced no output, so that second opinion is absent rather than clean. Codex reported no material findings but explicitly could not run the focused tests (sandbox blocked Go toolchain downloads), so neither second-opinion pass independently verified the new assertions.
cmd/seid/cmd/envshadow_config_fuzz_test.go is named *_config_fuzz_test.go but contains no Fuzz* target — every sibling in that package does (bindflags 1, chainid 2, legacy 7). Consider envshadow_config_test.go so the _fuzz_ suffix keeps meaning "has a fuzz target".
Substantively I agree with the decision to pin rather than fix the section-shadow behavior — that is exactly what testutil/configtest/AGENTS.md prescribes. Worth confirming there is a tracked follow-up for the operator-facing half the PR body calls out (enabled = false + any non-empty SEID_GIGA_EXECUTOR silently relaxes the LastResultsHash comparison "with nothing in any log saying so"), since a startup warning would be a behavior-preserving mitigation available before SeiConfigManager owns resolution.
Verification note: I could not execute go test ./testutil/configtest/ ./cmd/seid/cmd/ in this environment (command approval denied), so the pass/fail claims in the PR body are unverified here. The reasoning was checked statically: the quoted-key fixture does produce viper flat keys section.giga / section.giga.x, find returns on isPathShadowedInAutoEnv before the config file and before defaults/flag defaults (so Get→nil and IsSet→false are correct), and app.toml is only written when absent (util.go:298), so the pre-written fixture body survives repeated boots.
4 suggestion(s)/nit(s) flagged inline on specific lines.

claude

Looks good — test-only PR with no production code changes, and the one nit found (a stale doc comment on a private test helper) doesn't affect behavior.

Extended reasoning...

Overview

This PR touches only test files: two new test files (envshadow_config_fuzz_test.go, viper_settings_test.go) and edits to an existing test file and a small test-utility file (testutil/configtest/viper.go). It adds a configtest.Settings helper that reads a viper's resolved settings via AllKeys+Get (stable) instead of AllSettings (which the PR demonstrates is flaky under key-prefix collisions due to map iteration order), and swaps six existing differential/fuzz test assertions over to it. It also adds characterization tests pinning viper's section-level env-shadowing behavior (isPathShadowedInAutoEnv). No production code paths are modified — configmanager, app.New, and the actual config readers are untouched.

Security risks

None. There is no production code change; the diff is confined to test files and a test-support helper (Settings in testutil/configtest/viper.go) that is only consumed by tests. No new attack surface, auth, or crypto code is touched.

Level of scrutiny

Low. This is textbook test-hardening: it fixes a genuine flakiness source (AllSettings reorder-dependent output demonstrated at 43/157 across 200 reads) with a well-reasoned, tested replacement, and adds characterization coverage for existing (if surprising) viper behavior. The new Settings function is simple (iterate AllKeys, call Get, build a map) and its own tests cover the nil case, type-preservation, and the exact collision scenario it's meant to fix.

Other factors

The one bug found by this review run is a nit: the doc comment on the pre-existing-pattern helper newViperOver (in the newly-added viper_settings_test.go) overclaims what the function configures (env-prefix/AutomaticEnv/replacer) when it only reads a file into a bare viper. This has zero effect on any current test's behavior since none of the three consumers set env vars — it's a documentation-only issue confined to test scaffolding. Given the PR is test-only, thoroughly self-validated (described testing includes gofmt/goimports/golangci-lint clean and a 200-iteration flakiness demonstration), and the only finding is a comment nit, I'm comfortable approving without requiring a human pass.

Five review items, all non-blocking, one a real regression this branch introduced. Settings no longer tolerates a nil viper. (*viper.Viper)(nil).AllSettings() panics — the dereference is v.aliases inside AllKeys — so before this branch an unpopulated serverCtx.Viper failed loudly. Guarding it turned that into require.Equal(nil, nil), which passes: the differential's central premise reporting success on a boot that never happened. The nil branch is gone, and TestSettingsOnNilViper became TestSettingsOnNilViperPanics so the loud failure is pinned rather than merely restored. DumpViper keeps its nil tolerance, because describing a broken state is its job; Settings is the assertion, and tolerance there is what converts a broken premise into a pass. The six comparisons now route through requireSameSettings, which asserts both contexts carry a viper before comparing. A helper rather than twelve inline lines for the same reason the regression happened: an assertion a seventh call site can forget is the shape of defect being fixed. Verified by handing it two unpopulated contexts and confirming it fails naming which premise broke. Corrected the newViperOver doc comment, which claimed a SEID prefix, AutomaticEnv and the replacer that the helper never configured. The helper is deliberately file-only; the env behavior is pinned against the real server viper in the sibling file, and wiring a second weaker copy here would make four hermetic tests answerable to the developer's shell. Dropped fmt.Sprint from the full-key delivery assertion. It erased the property this change exists to preserve: the file layer resolves that key to bool(false) and the env layer to string("true"), and asserting the string records the asymmetry the coercion hides. A fresh fixture home per row, per testutil/configtest/AGENTS.md. The shared home was safe — app.toml is written only when absent — but the first boot creates config.toml, so the baseline row and the shadowed rows differed in two variables rather than one. Renamed envshadow_config_fuzz_test.go to envshadow_config_test.go. It carries no Fuzz target while all seven other *_config_fuzz_test.go files in the package do, so the suffix keeps meaning what it says. No fuzz target was added to justify the old name: isPathShadowedInAutoEnv tests only presence-and-non-emptiness, so the value space collapses to a bit that two existing rows already pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bdchatham · 2026-08-01T15:50:34Z

Thanks — all five actionable items are addressed in d0cb846. Notes on the two that involved a judgement call, and one I'm deliberately not doing.

The nil guard was a real regression, and it was the most valuable finding here. You're right about the mechanism — I verified (*viper.Viper)(nil) panics on AllKeys, Get and GetString as well as AllSettings. Fixed at both layers:

Settings no longer tolerates nil. The distinction I used: DumpViper keeps its nil tolerance because describing a broken state is its job, while Settings exists to be the assertion, and tolerance there is exactly what converts a broken premise into a pass. TestSettingsOnNilViper became TestSettingsOnNilViperPanics, so the loud failure is now pinned rather than merely restored — a future well-meaning nil guard fails a test instead of quietly reopening this.
The six comparisons route through requireSameSettings, which asserts both contexts carry a viper before comparing. A helper rather than twelve inline lines for the same reason the regression happened in the first place: an assertion a seventh call site can forget is the shape of defect being fixed.

Verified by handing it two unpopulated contexts and confirming it fails naming which premise broke, rather than only confirming it was added.

On the file name — you were right and the premise held more broadly than stated: all seven other *_config_fuzz_test.go files in the package carry at least one Fuzz* target, so this was the only violation. Renamed to envshadow_config_test.go. I considered adding a fuzz target instead and decided against it: isPathShadowedInAutoEnv tests only presence-and-non-emptiness, so the value space collapses to empty-vs-non-empty, and that pair is already pinned by two existing rows. A fuzzer would spend a full legacy boot per case re-deriving one bit.

On the tracked follow-up, and why there's no startup warning here. Your read is right that a warning would be behavior-preserving, and it's the obvious mitigation. It's deliberately out of scope for this PR, and the reasoning is the same one that made this a pinning PR rather than a fixing one: the legacy manager has shipped, and an operator may already depend on the current answer. A warning wouldn't change resolution, but it would put new output on the default path for every node — and the same class of change is what a companion design has been through three rounds of review to get right.

It is tracked, not dropped. It's recorded as a ratified legacy-vs-v2 divergence in the experimental-namespace design and in that arc's xreview ledger, with the correction landing when SeiConfigManager owns resolution rather than as a patch to the path being replaced. The rows in this PR are what that correction gets ratified against.

One thing I found while in here and left alone, since it's outside what you raised: the adjacent require.Equal(t, legacyCtx.Config, v2Ctx.Config) comparisons are vacuous on two unpopulated contexts too. That's not a regression from this branch — Config comparison never panicked on nil; pre-PR it was masked because the AllSettings panic came two lines later, and it's masked again now because requireSameSettings fires right after it. Both fields are populated by the same Apply, so the restored guard covers the scenario in practice. Flagging rather than fixing: it isn't this PR's surface, and it would want its own change if a future test ever compares only Config.

go test ./testutil/configtest/... ./cmd/seid/cmd/... passes, clean under -race, gofmt/goimports clean on all four files, golangci-lint 0 issues, and the parity fuzz target ran 15,663 execs through the new helper.

🤖 Generated with Claude Code

claude

LGTM — all prior review feedback (the nil-guard regression, stale doc comment, fmt.Sprint type erasure, shared-fixture-home, and file naming) has been addressed in d0cb846, and no new issues surfaced this round.

Extended reasoning...

Overview

Test-only PR: adds configtest.Settings (a flat, order-stable replacement for viper.AllSettings()) and routes six legacy-vs-v2 differential/fuzz comparisons through it via a new requireSameSettings helper, plus a new envshadow_config_test.go that pins the section-level env-shadow behavior in the legacy viper resolution path. No production code is touched — testutil/configtest/viper.go is a test-utility package used only by characterization suites.

Security risks

None. No production code paths, no auth/crypto/permissions surface. The env-shadow behavior being pinned is a real operational footgun (an operator's app.toml value can be silently discarded by an unrelated non-empty SEID_<SECTION> variable), but this PR only characterizes the existing legacy behavior in tests — it does not change it, and the PR description explains why that's deliberate (avoid changing config-resolution semantics as a side effect of a test PR).

Level of scrutiny

Low-to-moderate. This is dense test/documentation-style code with long comments, but the logic is straightforward: a flat-map settings comparison to avoid AllSettings() map-iteration-order flakiness, and table-driven tests booting fixture homes with various env vars set. I verified the specific regression called out in the prior review round (nil-viper guard silently passing) is fixed — Settings panics on nil, and requireSameSettings asserts both contexts have a non-nil viper before comparing.

Other factors

This is the second round on this PR; the first round produced five actionable comments (one substantive regression, four nits/docs), all of which the author fixed in commit d0cb846 with a detailed, self-critical commit message. The bug-hunting system found nothing new this round, and my own read of the diff didn't surface anything beyond what was already resolved.

seidroid

Test-only PR that pins viper's section-level env shadowing and replaces six flaky AllSettings() parity comparisons with a flat, order-stable configtest.Settings. The core reasoning checks out and I found no blockers — only doc/assertion-strength nits.

Findings: 0 blocking | 8 non-blocking | 4 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.
I could not run go test in this environment, so the PR body's claim that all three packages pass is unverified here — worth confirming CI is green on ./testutil/configtest/... and ./cmd/seid/cmd/....
The prose in envshadow_config_test.go states a fairly strong operational consequence (a shadowed giga_executor.enabled relaxes the LastResultsHash comparison via tmtypes.SkipLastResultsHashValidation.Store). Nothing in the PR asserts that end-to-end link, so it can drift silently if app.New stops wiring it. Per testutil/configtest/AGENTS.md ("Out of Scope": reads needing a running node) leaving it as prose is defensible — but consider at least asserting that app.New's read of that key sees nil under the shadow, so the load-bearing half is pinned rather than described.
requireSameSettings fails with a raw map[string]any diff over the full key space. The Settings doc comment itself points at DumpViper as "the right tool for a readable failure message" — consider including configtest.DumpViper output in the failure message so the two tools are used as the comment describes.
4 suggestion(s)/nit(s) flagged inline on specific lines.

…eview cycle Six items, two of which make the tests stronger than they were written. The AllSettings instability is now asserted rather than logged. Requiring the instability itself would depend on hitting both iteration orders within a sample, which is the coin flip being documented — but lossiness is unconditional: both orderings destroy a value, and only which one varies. Verified over 5000 reads: exactly two shapes, leaf count always 1, AllKeys always 2. Each read now asserts that flattening AllSettings yields one fewer leaf than AllKeys has keys, with a guard that the fixture still presents two colliding keys, since a degenerate fixture would satisfy a bare inequality. The consensus consequence in the shadow test's header is now pinned at the step it rests on. giga's own suite pins nil-keeps-default against a synthetic AppOptions, and app/consensus_config_fuzz_test.go pins enabled through to the atomic, but nothing asserted the join: a real boot, a real shadowing variable, the real reader. Each shadow row now calls gigaconfig.ReadConfig on the resolved viper and asserts Enabled is true against an app.toml that says false. The literal true is asserted separately from the DefaultConfig comparison, because a comparison against the default alone passes for whatever the default happens to be — the trap the harness guide describes for goldens. Settings' doc now gives both reasons it exists. Beyond flattening, viper's getSettings skips keys whose Get returns nil, which is exactly the shadow this branch pins, so a key one side enumerates and resolves to nothing is indistinguishable from a key the other side never enumerated. Only the flat form separates them. requireSameSettings reports through DumpViper on mismatch while keeping the typed map as the authority, since a rendering can collide where the maps differ. Note this does not shrink the failure — testify already produced a sorted, typed, per-key diff, and rendering grows the output about 25% — but the diff line a human reads is in the harness's own notation. bootWithAppTOML renamed to homeWithAppTOML: it built a home and returned one, and booting is applyLegacy's job. A sweep for the stale-comment class found two more, both introduced on this branch. The env-name hazard example was impossible as written — there is no dot in giga_executor, and the real hazard is the prefix, which under go test is cmd.test. And a claim that the replacer folds ".", "-" and "_" invented a third rule; there are two. That invention had already propagated from a comment dropped in the previous commit, which is why the sweep was worth running. One temp directory instead of 200: the loop exercises map iteration, not parsing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only PR that pins viper's section-level environment shadow and replaces six flaky AllSettings() parity comparisons with a deterministic configtest.Settings helper; the mechanism claims, the determinism argument, and the cross-referenced assertions all check out statically, with no blockers found. Two non-blocking suggestions: one row can pass vacuously, and the new comparison convention isn't recorded in the harness guide.

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

Blockers

None at the file/PR level.

Non-blocking

testutil/configtest/AGENTS.md is the canonical guide for this harness but doesn't mention the new rule this PR establishes — "compare two vipers with configtest.Settings, never AllSettings" (and why: prefix-collision nondeterminism plus nil-key coverage). Without a line there, the next parity assertion is likely to reintroduce AllSettings, since the only record of the rule is a doc comment on the helper.
DumpViper and Settings now independently do AllKeys + Get; DumpViper could be expressed as a sorted render of Settings(v) (keeping its <nil-viper> guard) so the two can't drift on what "the resolved key/value map" means. Purely cosmetic.
Cursor's second-opinion pass produced no output (cursor-review.md is empty); Codex reported "No material findings in the reviewed diff." So this review is the only substantive pass on the diff.
I could not run go test ./testutil/configtest/... ./cmd/seid/cmd/... in this environment (sandbox denied the command), so my review is static only. The PR body reports these passing.
1 suggestion(s)/nit(s) flagged inline on specific lines.

claude

LGTM. This commit addresses the remaining second-round nits (rename bootWithAppTOML→homeWithAppTOML, doc the AllSettings nil-drop semantic gap, make the prefix-collision test a deterministic regression guard via countLeaves, and stop allocating 200 temp dirs) and adds a gigaconfig.ReadConfig assertion tying the shadow to the actual default-fallback behavior.

Extended reasoning...

Overview

This is a follow-up commit (90fa8bb) on a test-only PR (no production code changes) that pins two viper/config characterization behaviors: the section-level env-var shadow, and a deterministic replacement for AllSettings()-based parity comparisons. The four touched files are all test/test-utility code (_test.go files plus testutil/configtest/viper.go, itself only used by tests).

Security risks

None. No production code paths are touched; the changes affect only test harnesses and characterization suites in cmd/seid/cmd and testutil/configtest.

Level of scrutiny

Low-to-moderate is appropriate: this is test infrastructure, not a runtime behavior change, and the repository's own testutil/configtest/AGENTS.md conventions are being followed carefully (fresh fixture homes per test, deterministic assertions over probabilistic ones). I verified the four specific review nits from the second round (renamed bootWithAppTOML→homeWithAppTOML, expanded the AllSettings doc comment to cover the nil-key-drop semantic in addition to re-nesting, added a deterministic countLeaves assertion that fires on every read rather than only logging, and refactored the 200-iteration loop to write the fixture file once and re-read it via newViperOverFile) are all correctly implemented in this commit, and confirmed the whole suite compiles and passes (go test ./testutil/configtest/... ./cmd/seid/cmd/..., plus gofmt -s clean).

Other factors

The new gigaconfig.ReadConfig assertion in envshadow_config_test.go correctly type-checks (*viper.Viper satisfies the servertypes.AppOptions interface ReadConfig expects) and was confirmed by running the affected tests directly. No comments remain outstanding from the second review round — this commit is exactly the response to them, and no new concerns were introduced.

…reader The dash-folding row asserted only that the key resolves to nothing under the shadow. A nil is evidence of shadowing only if the key is known to resolve without the variable set: a fixture typo or a section rename would have satisfied that assertion while the property the row exists to pin went untested. It now anchors on its own baseline home first, the same shape the section-shadow row already used. It also pins what the nil read costs, because this key's reader is shaped differently from giga_executor's. GetConfig reads it unguarded — v.GetBool("state-commit.sc-enable") at sei-cosmos/server/config/config.go:621, no presence check — so a nil read is not a fallback to an in-code default, it is GetBool(nil), which is false. The two sections therefore fail in opposite directions from one mechanism: a shadowed giga_executor.enabled keeps its true default and silently enables what the operator disabled, while a shadowed state-commit.sc-enable resolves false and silently disables what the operator enabled. Which direction a section takes depends only on whether its reader guards the read, and that is worth recording where the shadow is pinned. Both new assertions were shown to fail when they should: omitting the key from the fixture trips the baseline, and removing the shadow trips the reader assertion once the earlier nil assertion is taken out of the way so it cannot pre-empt it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only PR that pins viper's section-level env-shadow behavior and replaces six flaky AllSettings() parity comparisons with a deterministic configtest.Settings helper; I verified the viper mechanics behind both claims and found no correctness or security issues, only minor coverage/documentation suggestions.

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

Blockers

None at the file/PR level.

Non-blocking

Second-opinion passes: Codex reported "No material issues found"; cursor-review.md is empty, so the Cursor pass produced no output and contributed nothing to this synthesis.
testutil/configtest/AGENTS.md is the canonical guide for this harness but isn't updated to say that a two-viper parity comparison must go through configtest.Settings rather than Viper.AllSettings(). All six existing call sites are converted, but nothing stops the seventh from reaching for AllSettings again and reintroducing the prefix-collision flake. A couple of lines under a new heading (or beside the existing DumpViper guidance) would make the rule discoverable.
Verified independently and agree with the PR's reasoning: find() hits the auto-env shadow check before both the config and default layers, so Get returning nil for a shadowed key is correct; getEnv's ok && (allowEmptyEnv || val != "") makes the empty-value boundary row correct; and interceptConfigs only writes app.toml when absent (sei-cosmos/server/util.go:298), so a shadowed section can't be materialized back over the operator's file. No finding in any of these — noted so the next reviewer doesn't re-derive them.
2 suggestion(s)/nit(s) flagged inline on specific lines.

…lossiness failure Settings' doc argues it is stronger than AllSettings for two reasons, and only the first was pinned. The second — AllSettings omits a key whose Get returns nil while Settings records it as an explicit nil entry — had no assertion anywhere, because the only thing that produces a key which enumerates but resolves to nothing is the environment shadow, and configtest's own tests build vipers with no env layer on purpose. The shadow rows are therefore the one place it can be asserted, so they now assert it: Contains before the nil check, since indexing an absent key also yields nil and only the pair distinguishes "recorded as nil" from "not recorded". Verified by making Settings skip nil-resolving keys, which fails the new row. The lossiness assertion characterizes upstream viper rather than this repo, so a viper bump that fixes the prefix collision fails a test while nothing in sei changed. The comment said so; the failure message did not. It now tells the reader to delete the assertion rather than work around it, since Settings is correct either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only PR that pins viper's section-level env-shadow behavior and replaces six nondeterministic AllSettings() parity comparisons with a flat, Get-based configtest.Settings helper; the mechanism, helper, and every factual claim in the comments check out, leaving only minor nits.

Findings: 0 blocking | 6 non-blocking | 1 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 is the Claude pass merged with Codex's, which reported no material issues.
I independently confirmed the core claims rather than taking the comments at face value: viper's find() returns nil from isPathShadowedInAutoEnv before consulting the config layer; getEnv treats an empty value as absent without AllowEmptyEnv; the replacer runs over the whole prefixed name (mergeWithEnvPrefix then envKeyReplacer.Replace), matching configtest.ServerEnvKey; AllSettings skips keys whose Get is nil and always loses exactly one leaf on a giga/giga.x prefix collision; and Get's longest-prefix-first search resolves both. The state-commit.sc-enable unguarded GetBool is really at sei-cosmos/server/config/config.go:621, and app.TestGigaExecutorEnabledDrivesLastResultsHashValidation exists as the comment says.
Settings is a strict strengthening of the old comparison (same key source, plus explicit nil entries), and grep confirms no AllSettings() parity assertion remains anywhere outside the new characterization test — the migration is complete.
Consider a line in testutil/configtest/AGENTS.md pointing future rows at Settings for assertions vs DumpViper for failure messages; the guide is the documented entry point for adding a pin, and the distinction currently lives only in the helper's doc comment.
TestSettingsIsStableWhereAllSettingsIsNot depends on fmt.Sprint sorting map keys (Go 1.12+) for its shape fingerprint to be stable — true today and fine, but worth a word in the comment since the test's whole premise is that map ordering is untrustworthy.
1 suggestion(s)/nit(s) flagged inline on specific lines.

… in the guide require.Contains for the two membership checks. require.True on a boolean reports only "false"; Contains prints the collection, so an AllKeys failure now names the keys that were present and a prefix failure names the derived variable. That also drops the slices and strings imports, leaving the file with three. Note the key list prints in AllKeys order, which this branch's own finding says is unstable, so it varies between runs — still strictly more than a bare false. The comparison rule now lives in testutil/configtest/AGENTS.md, which is the entry point the root guide points contributors at before they change a configuration read. It said nothing about how to compare two resolved vipers, so the Settings-for-assertions and DumpViper-for-messages distinction existed only in a doc comment a contributor reaches after already choosing wrong. The new section states both, why AllSettings is neither, and why Settings panics on a nil viper. Named the shape fingerprint's dependency on fmt sorting map keys. The test's premise is that map ordering is untrustworthy, and its own mechanism relies on the ordering fmt hides — worth saying out loud in that specific test, since without the sort every read would look like a distinct shape and the counts would be meaningless rather than merely wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only PR that pins viper's section-level env-shadow behavior and replaces six order-dependent AllSettings() parity comparisons with a deterministic configtest.Settings helper. The technical claims check out against viper v1.21, sei-cosmos/server/config/config.go:621, and giga/executor/config; no blockers, only minor polish notes.

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

Blockers

None at the file/PR level.

Non-blocking

The Cursor second-opinion file (cursor-review.md) is empty — that pass produced no output. Codex reported no material issues.
requireSameSettings closes the "both vipers are nil, so parity passes vacuously" hole, but the require.Equal(t, legacyCtx.Config, v2Ctx.Config, ...) assertion that immediately precedes every one of those calls still has it: if Apply never populated the context, both *tmcfg.Config pointers are nil and the comparison reports parity on a boot that never happened. Consider folding the Config comparison into the same guarded helper (or adding matching require.NotNils) so the premise is closed on both halves rather than one.
No verification run was possible in this environment (go test was not approved), so the assessment is static. The PR body reports go test ./testutil/configtest/... ./cmd/seid/cmd/... passing and lint clean; worth confirming CI agrees, particularly TestSettingsIsStableWhereAllSettingsIsNot under -race.
Doc/comment density is very high relative to code, but it matches the existing style in testutil/configtest and cmd/seid/cmd, and the AGENTS.md "Comparing two resolved vipers" section is a genuinely useful addition — noting only that the prose duplicates itself across viper.go, AGENTS.md, and three test headers, so a future behavior change has four places to update.
2 suggestion(s)/nit(s) flagged inline on specific lines.

…ake the collision fixture reach its collision The nil-viper guard closed one half of a two-channel premise. The Config comparison beside it kept the other: Apply populates both fields, so a context it never reached carries nil in both, and require.Equal on two nil *tmcfg.Config pointers passes. It went unnoticed because AllSettings used to panic on the nil viper two lines later, so the boot failure surfaced anyway — as a dereference from inside viper rather than as a named premise. Guarding settings alone left this masked only by the accident that the two are always compared together. Both channels now go through one helper, since a test with grounds to compare either has grounds to compare both, and splitting them is what let the hole sit next to its own fix. I reported this twice as out of scope; with a reviewer asking and the fix three lines, leaving it was the wrong call. The guards also no longer discard the caller's message. A nil-viper failure from a corpus row or a fuzz execution named the broken premise but not the input that produced it, which is the half needed to reproduce it. Passing msgAndArgs straight through would have made the caller's format string consume the helper's text as an argument, so callerContext renders it as a parenthetical instead. TestSettingsKeysCannotCollideThroughRendering never reached the collision its comment described. The fixtures differed in size, so require.NotEqual held for a reason unrelated to rendering: the two documents were never equal, because DumpViper wraps the value as int64(1) while the old key text carried a bare 1. The new fixture is a single key spelled `s.a = int64(1)\ns.b` holding int64(2), which renders as exactly the two lines the other fixture's two keys produce — verified identical. The test now asserts that premise first, so the property is proven against a real collision rather than against two maps of different sizes. Sample size down from 200 to 20. Both assertions in that test are deterministic per read, so the sample fed only the informational shape count; at the measured ~4:1 split, 20 reads still show both orderings well over 99% of the time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only PR that pins viper's section-level env-shadow behavior and replaces six flaky AllSettings() parity comparisons with a deterministic configtest.Settings helper. The mechanism analysis, helper implementation, and cross-references (giga config defaults, the unguarded state-commit.sc-enable reader, SkipLastResultsHashValidation) all check out; no blocking issues, only messaging/follow-up nits.

Findings: 0 blocking | 7 non-blocking | 2 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 reflects only Claude + Codex. Codex reported no material issues; I independently reached the same conclusion on blockers.
Settings/countLeaves/AllSettings leaf-loss assertion in TestSettingsIsStableWhereAllSettingsIsNot characterizes upstream viper (v1.21.0) rather than this repo. The comment already tells a future maintainer to delete the assertion if a viper bump fixes the prefix collision, which is the right call — just be aware this is a dependency-coupled test that will need attention on the next viper upgrade.
The shadow behavior being pinned is operator-invisible and reaches a consensus-relevant gate (giga_executor.enabled → tmtypes.SkipLastResultsHashValidation). Deliberately leaving resolution alone is defensible and well argued in the PR body, but consider a cheap mitigation that does not change resolution: log a startup warning when a SEID_<SECTION>-shaped variable is set and shadows keys present in app.toml. At minimum, link a tracking issue from the test header so the SeiConfigManager fix has an anchor beyond prose.
TestSectionEnvVarShadowsItsWholeSection writes minimum-gas-prices in its fixture while the other three new tests omit it. Not a correctness problem (other tests in this package boot from equally minimal app.toml bodies), but making the fixtures uniform would remove a distracting difference between rows.
I could not execute go test ./testutil/configtest/... ./cmd/seid/cmd/... or gofmt -l in this environment (sandbox denied). The review is static; the PR body reports both clean. Worth confirming CI green before merge, particularly TestSettingsIsStableWhereAllSettingsIsNot, whose require.Equal(len(keys)-1, countLeaves(all)) depends on viper's deepSearch losing exactly one leaf on both iteration orders — I traced both orders by hand and it holds, but it is the one assertion whose validity rests on library internals.
2 suggestion(s)/nit(s) flagged inline on specific lines.

…he v2 fix Folding the Config comparison into the shared helper left the callers passing settings-flavored messages, which callerContext then appended to the Config failure — so a genuine Config mismatch reported "serverCtx.Config differs between legacy and v2 (serverCtx.Viper settings differ between legacy and v2 (unknown-section))", naming the wrong channel as the one that differed. The callers now name only the input and each helper supplies its own channel noun, so the same failure reads "serverCtx.Config differs between legacy and v2 (case \"unknown-section\")". The fuzz call site already did it this way; the other four had not been updated when the helper absorbed the second channel. callerContext's default branch printed msgAndArgs as a whole slice including index 0, so a non-string leading element would render as "([foo bar baz])" rather than a message with arguments. Unreachable from the current call sites, but it degrades gracefully now rather than oddly. The header names PLT-775 as where the v2 correction lands, so the fix has an anchor that outlives this comment rather than living only in prose. The section-shadow fixture drops minimum-gas-prices, which the other three rows never carried. It was realism the test does not use, and the difference between rows was a distraction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

seidroid

Test-only PR that pins viper's section-level env-shadow behavior and replaces six flaky AllSettings() parity comparisons with a deterministic configtest.Settings accessor; the analysis behind both changes checks out against viper's find/getSettings/deepSearch semantics and against the real readers cited in the comments. No blocking issues — only non-blocking notes, including that the Cursor pass produced no output.

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

Blockers

None at the file/PR level.

Non-blocking

Second-opinion coverage was thin: cursor-review.md is empty (that pass produced no output), and codex-review.md reports "No material findings." The findings below are from this pass alone.
Verification note: the sandbox in this environment refused go test/gofmt, so the assertions were verified by reading viper's resolution order (find returns on isPathShadowedInAutoEnv before both the config-file and defaults layers) and getSettings/deepSearch (which provably loses exactly one leaf on a prefix collision in either iteration order), not by execution. The author reports both packages passing.
The pinned defect is consensus-adjacent — a stray non-empty SEID_GIGA_EXECUTOR silently re-enables giga and relaxes LastResultsHash validation via tmtypes.SkipLastResultsHashValidation — and TestShadowFoldsSectionPunctuation shows the mirror failure for state-commit.sc-enable. Deliberately pinning rather than fixing is the right call under testutil/configtest/AGENTS.md, but a cheap interim mitigation would be a startup WARN when any SEID_<SECTION> variable names a proper prefix of an enumerated key. Worth considering on PLT-775 rather than waiting for SeiConfigManager to own resolution.
configtest.Settings compares resolved values but is still blind to env-only keys (no AllKeys entry). That is stated in the godoc and covered separately by FuzzConfigManagerEnvOnlyKeyParity, so no change needed — noting it so the coverage boundary is explicit in the review record.

Comments that couldn't be anchored to the diff

cmd/seid/cmd/configmanager_differential_test.go:49 -- [nit] In the newline-key case this require.Equal on the two dumps passes (that is exactly the collision TestSettingsKeysCannotCollideThroughRendering constructs), so the failure that actually fires is the typed one on line 50 — and its message carries no dump at all, which is the case where a human most needs one. The comment acknowledges the ordering is deliberate; consider attaching the dumps to the typed assertion instead, e.g. build a combined message once the difference is known rather than relying on the dump assertion to fire first. Purely a failure-legibility nit; correctness is unaffected either way.
testutil/configtest/viper_settings_test.go:640 -- [nit] require.Equal(t, len(keys)-1, countLeaves(all), ...) is the load-bearing assertion here and it holds for a reason worth pinning down more tightly than "one of the two colliding values is lost": with section.giga / section.giga.x, deepSearch either replaces the scalar 1 with a fresh map (order A) or overwrites the {x:2} sub-tree with 1 (order B), so the tree has exactly one leaf either way. That generalises to "one leaf lost per colliding pair", not "one leaf lost" — if someone later widens the fixture to three colliding keys this assertion silently becomes wrong rather than failing informatively. A short note that the arithmetic assumes exactly one collision pair would keep the next editor honest.
cmd/seid/cmd/envshadow_config_test.go:307 -- [nit] The baseline runs outside any t.Run, so a baseline failure is attributed to the parent test rather than to a named row while the five subtests below each get their own name. Since the header comment goes out of its way to explain that the baseline boots its own fixture home for the same reason the rows do, wrapping it in t.Run("baseline", ...) would make it read as the sixth row it effectively is.

claude

LGTM — test-only change with no production code impact, and this run found no new issues.

What was reviewed: the section-env-shadow fixtures (giga_executor, state-commit) against the real gigaconfig.ReadConfig reader and ServerEnvKey derivation; the Settings/AllSettings determinism argument against deepSearch's prefix-collision behavior; and the requireSameSettings/requireSameChannels/callerContext message-composition helpers, where the two issues I'd flagged in earlier rounds (missing msgAndArgs on the nil guards, and the wrong-channel message text) are both fixed in the current HEAD (a14538c0).

Extended reasoning...

Overview

This PR touches only test files (cmd/seid/cmd/configmanager_differential_test.go, the new envshadow_config_test.go, testutil/configtest/viper.go, its new test, and testutil/configtest/AGENTS.md). It adds configtest.Settings as a deterministic replacement for comparing viper.AllSettings() output, adds tests pinning a section-level environment-variable shadowing quirk in viper, and refactors the legacy-vs-v2 differential test's comparison helpers for consistent nil-guard and message behavior. No production code is modified.

Security risks

None. This is test-only tooling for a characterization suite; no auth, crypto, or externally-reachable code paths are touched.

Level of scrutiny

Low production risk (zero prod code changed), but the reasoning itself is subtle: it relies on non-obvious claims about viper's internal deepSearch/isPathShadowedInAutoEnv behavior and Go map-iteration nondeterminism. That subtlety was already worked through across two extensive review rounds (an automated bot and my own prior runs), each of which found real defects (a vacuous test fixture that didn't reach the rendering collision it claimed to test, missing message-forwarding on nil guards, and a wrong-channel failure message) that were fixed with verified before/after repros in each case.

Other factors

I independently spot-checked several of the PR's factual claims against the current tree (gigaconfig.ReadConfig's presence-guarded read and its default, configtest.ServerEnvKey/ServerEnvPrefix) and they hold. All prior inline review threads are marked resolved, and the two issues raised by my own previous run are addressed in the current HEAD commit. This run's bug hunt found nothing new, so there's nothing outstanding to send to a human.