feat(configmanager): SeiConfigManager v2 body — validate-passthrough (PLT-775 PR2)#3678
feat(configmanager): SeiConfigManager v2 body — validate-passthrough (PLT-775 PR2)#3678bdchatham wants to merge 3 commits into
Conversation
PR SummaryMedium Risk Overview Dependency: pins Reviewed by Cursor Bugbot for commit 13b3a44. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3678 +/- ##
==========================================
- Coverage 59.24% 58.30% -0.94%
==========================================
Files 2272 2185 -87
Lines 188175 178428 -9747
==========================================
- Hits 111479 104039 -7440
+ Misses 66657 65149 -1508
+ Partials 10039 9240 -799
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Small, well-documented, well-tested change that fills in the v2 SeiConfigManager as a validate-passthrough behind SEI_CONFIG_MANAGER=v2; resolveHomeDir faithfully mirrors the legacy home resolution and all validation is advisory, so there are no correctness or boot-safety blockers. Only a stale doc-comment inconsistency and dependency-bump verification are worth noting.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output. Codex (codex-review.md) reported no material findings but noted it could not run targeted tests due to a Go version mismatch (sandbox 1.24.13 vs required 1.25.6), so its pass was static-only.
- go.mod/go.sum bump three deps (add sei-config v0.0.21, BurntSushi/toml 1.4.1-pseudo → 1.5.0, mapstructure v2.4.0 → v2.5.0). The toml and mapstructure bumps appear to be transitive pulls from sei-config; worth confirming
go mod tidyis clean and that the toml 1.5.0 bump doesn't alter behavior of any existing config rendering/parsing in the repo, since CI here could not be run against Go 1.25.6. - Consider a follow-up note/tracking item so the advisory validation messages printed to stderr on every v2 boot (e.g. false-positive validation issues while sei-config read fidelity is still being hardened) don't become persistent log noise before validation is promoted to fatal.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // design (bdchatham-designs designs/config-manager/DESIGN.md). | ||
| // LegacyConfigManager re-enters the unchanged legacy handler verbatim (the | ||
| // default). SeiConfigManager re-authors the existing config through the | ||
| // sei-config library and then re-enters the same reader, so both channels are |
There was a problem hiding this comment.
[nit] Stale/misleading wording: this package-level doc says v2 "re-authors the existing config through the sei-config library," but the PR's whole thesis — and the SeiConfigManager/Apply doc comments below (lines 59-65, 75-77) — is that v2 does NOT rewrite/re-author; it reads the config only to validate, then re-enters the legacy reader on the operator's original files. "re-authors" implies a write and contradicts the validate-passthrough design. Suggest rewording to something like "reads the existing config through the sei-config library to validate it, then re-enters the same legacy reader."
| } else if res := seiconfig.Validate(cfg); res.HasErrors() { | ||
| fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: validation found issues (advisory, not enforced): %v\n", res.Errors()) | ||
| } |
There was a problem hiding this comment.
🔴 v2's boot value-add is the validation pass, but the code drops all SeverityWarning diagnostics: HasErrors() returns false for warning-only results (the entire branch is skipped) and Errors() filters warnings out even when both severities are present. Operator-facing diagnostics like "test_only_dual_write must not be used in production", "unusual backend", and "EVM RPC is enabled on a validator node" — exactly the misconfigurations a validation pass exists to surface — are silently swallowed. Fix by gating on len(res.Diagnostics) > 0 (or HasErrors() || HasWarnings()) and printing res.Diagnostics rather than just res.Errors().
Extended reasoning...
The bug\n\nIn cmd/seid/cmd/configmanager/configmanager.go at lines 87-89:\n\ngo\n} else if res := seiconfig.Validate(cfg); res.HasErrors() {\n fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: validation found issues (advisory, not enforced): %v\n", res.Errors())\n}\n\n\nBoth the gate (HasErrors()) and the printed value (Errors()) filter to SeverityError only. Warnings — which sei-config explicitly defines as part of the validation diagnostics — never reach the operator.\n\n## What sei-config actually returns\n\nReading sei-config@v0.0.21/validate.go directly confirms:\n\n- The library distinguishes three severities (validate.go:11-15): SeverityError ("prevents seid from starting"), SeverityWarning ("logged but does not block startup"), SeverityInfo.\n- ValidationResult.HasErrors() (lines 54-61) returns true only if a SeverityError is present.\n- ValidationResult.Errors() (lines 63-71) returns only SeverityError diagnostics, filtering out warnings.\n- The library emits addWarning for real, operator-actionable misconfigurations:\n - storage.state_commit.write_mode: "test_only_dual_write must not be used in production" (line 264)\n - storage.state_store.backend: "unusual backend %q; expected pebbledb or rocksdb" (line 278-279)\n - evm: "EVM RPC is enabled on a validator node; this is unusual and may increase attack surface" (line 292)\n\nThe library's own contract is that warnings should be logged (just not enforced). The v2 boot code never logs them.\n\n## Step-by-step proof\n\nConsider a validator operator who sets storage.state_commit.write_mode = test_only_dual_write in sei.toml — a config that is otherwise valid (no SeverityError diagnostics).\n\n1. seiconfig.ReadConfigFromDir(home) succeeds and returns cfg.\n2. seiconfig.Validate(cfg) runs validateStorage (validate.go:249). At line 263-265, since sc.WriteMode == WriteModeTestOnlyDualWrite, it appends a single Diagnostic{SeverityWarning, "storage.state_commit.write_mode", "test_only_dual_write must not be used in production"} to res.Diagnostics. No addError is called for this field.\n3. res.HasErrors() iterates res.Diagnostics, finds the single entry has Severity == SeverityWarning, and returns false.\n4. The else if branch is skipped entirely. Nothing is printed.\n5. The operator boots a validator with a production-unsafe write mode and receives no warning from the v2 manager — exactly the failure the validation pass exists to prevent.\n\nThe same hole applies to a validator that enables EVM RPC, or to any unusual state-store backend.\n\n## Why this matters here\n\nThe PR's doc comment on SeiConfigManager explicitly says it "surfaces validation diagnostics", and the PR description frames v2's boot value-add as "the validation pass". Warnings are diagnostics by sei-config's own contract. Dropping them defeats the only feature v2 adds over legacy at boot — there is nothing else v2 does differently on the boot path (it is a passthrough otherwise).\n\n## Fix\n\nGate on the presence of any diagnostic, and print all of them:\n\ngo\n} else if res := seiconfig.Validate(cfg); len(res.Diagnostics) > 0 {\n fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: validation diagnostics (advisory, not enforced): %v\n", res.Diagnostics)\n}\n\n\nDiagnostic.String() (validate.go:37-39) already prefixes each entry with [ERROR]/[WARNING] so the operator can still tell which findings are blocking and which are advisory.
| // LegacyConfigManager re-enters the unchanged legacy handler verbatim (the | ||
| // default). SeiConfigManager re-authors the existing config through the | ||
| // sei-config library and then re-enters the same reader, so both channels are | ||
| // produced identically to legacy. See PLT-775 and the canonical design | ||
| // (bdchatham-designs designs/config-manager/DESIGN.md). |
There was a problem hiding this comment.
🟡 The package-level doc at the top of cmd/seid/cmd/configmanager/configmanager.go is stale in two places. Line 2 still calls the sei-config-backed manager (forthcoming) even though this very PR lands it, and lines 5-9 say SeiConfigManager re-authors the existing config through the sei-config library — but the actual implementation (and the type-level doc 30 lines below) explicitly says it does NOT rewrite/re-author anything; it's a validate-passthrough. Suggest replacing re-authors with something like reads and validates and dropping (forthcoming) so the package-level doc matches the rest of the file (and the PR description).
Extended reasoning...
What the bug is
The package-level doc comment that sits at the top of cmd/seid/cmd/configmanager/configmanager.go (lines 1-10) describes a version of SeiConfigManager that does not match what this PR actually ships. Two distinct stale phrases:
-
Line 2 still describes the sei-config-backed manager as
(forthcoming). That phrasing was accurate in PR1 (feat(seid): ConfigManager selection seam (PLT-775 PR1) #3671, seam only), but this PR (PLT-775 PR2) implementsSeiConfigManager.Apply— it importsseiconfig, callsReadConfigFromDir+Validate, and is wired intoSelect. The manager is no longer forthcoming; it is the active validate-passthrough. -
Lines 5-9 say:
SeiConfigManager re-authors the existing config through the sei-config library and then re-enters the same reader, so both channels are produced identically to legacy.The wordre-authorsis wrong —Applydoes not author/rewrite anything on disk. This contradicts (a) the type-level doc directly below at lines 59-65 (it does NOT rewrite them, migrate (...), or author sei.toml), (b) theConfigManagerinterface doc at lines 35-37 (v2 reads the config to validate it, then re-enters the unchanged legacy reader), and (c) the PR description itself (v2 boot is validate-passthrough... It does not rewrite, migrate, or author sei.toml at boot).
Step-by-step proof
Walk a reader through the file top-to-bottom:
- Line 2-3 — reader is told the manager is
(forthcoming). - Lines 5-9 — reader is told
SeiConfigManager re-authors the existing config through the sei-config library and then re-enters the same reader. Reader forms mental model: "v2 rewrites the legacy files via sei-config, then re-reads them." - Lines 35-37 — interface doc says
The boot path does not re-render the legacy files: v2 reads the config to validate it, then re-enters the unchanged legacy reader on the operator's existing files. Reader is confused: "Wait, does it re-render or not?" - Lines 59-65 — type-level doc on
SeiConfigManagersaysit does NOT rewrite them, migrate (...), or author sei.toml. Reader concludes: "OK, the top comment must be wrong." - Lines 78-93 —
Applybody confirms:resolveHomeDir→seiconfig.ReadConfigFromDir→seiconfig.Validate→ log advisory diagnostics →server.InterceptConfigsPreRunHandler. No writes. The type-level doc is authoritative; the package-level doc was missed when the v2 plan pivoted from "re-author the legacy files" to "validate-passthrough".
Impact and fix
Doc-only — there is no runtime impact, no operator-visible behavior change, and the authoritative type-level doc is correct. But it is the very first thing a reader of this package sees, and it presents a mental model that the rest of the file immediately contradicts. Worth fixing in one small edit:
- drop
(forthcoming)from line 2 - replace
re-authors the existing config through the sei-config library and then re-enters the same readerwith something likereads and validates the existing config through the sei-config library, then re-enters the unchanged legacy reader
Severity: nit. Two verifiers confirmed each sub-finding independently with no refutations, all agreeing this is a stale-comment cleanup rather than a functional bug.
Review-gate summary (clean)Coral /xreview slate — all RATIFY:
Findings (all [should]/[nit]) resolved: package-comment doc-drift fixed; validation-error advisory made louder; on-disk-fidelity note added; Bots: Cursor Bugbot ✅ SUCCESS; seidroid ✅ APPROVED (0 blocking). Expected, not a regressionOn a default node, v2 logs a benign advisory Merge prerequisites (separate from the review-gate)Standard CI (Test / Lint / Race / etc.) + the category label, same as the seam PR. |
…(PLT-775 PR2) Fills in the v2 ConfigManager behind SEI_CONFIG_MANAGER=v2 (seam landed in #3671). v2 boot reads the config through the sei-config unified model to VALIDATE it, then re-enters the unchanged legacy reader on the operator's ORIGINAL files — it does not rewrite, migrate, or author sei.toml at boot. The two consumed channels are produced identically to legacy by construction; v2's boot value-add is the validation pass. - SeiConfigManager.Apply: resolveHomeDir (mirrors the legacy handler's flag>SEID_env>default resolution exactly) -> ReadConfigFromDir -> Validate -> re-enter. Advisory: read/validate problems are logged, never refuse boot. - Pin sei-config v0.0.21 (merged lenient-decode fix) so a real seid config.toml can be read for validation. - Legacy-vs-v2 differential test: both managers read the same home (v2 is passthrough); serverCtx.Config + Viper.AllSettings() must match, incl. after the start.go chain-id mutation. Design captured in bdchatham-designs designs/config-manager/DESIGN.md (validate-passthrough; advisory validation; sei.toml-as-source at the generate path; explicit migration). Fatal validation is the un-defer once sei-config read fidelity is hardened (a pruning read-mapping gap is surfaced advisory). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y, home test) PR2 xreview slate (all RATIFY) follow-ups: - doc: package comment said v2 "re-authors" — stale from the old write-then- re-enter model; v2 reads to validate, does not rewrite (idiomatic-reviewer + seidroid, convergent). - advisory: distinguish validation ERRORS with a louder lead-in — they are the same SeverityError class (sc-write-mode) legacy panics on later at app.New(); surfaced earlier, not enforced (systems-engineer dissent F2). - doc: note validation runs against the on-disk files only, not the merged AppOptions, so a default node's pruning advisory is expected/benign — the template omits the key cosmos defaults in code (sei-network-specialist). - test: TestResolveHomeDir_Flag covers the --home resolution. Deferred (dissent F4, sanctioned cut): env-precedence + mixed-home rows ride the generate-path PR — the boot differential already proves passthrough parity by construction. Design MVP-scope reconciled separately (bdchatham-designs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tic findings) Addresses agentic reviewer findings on PR #3678: - lint (errcheck, red CI): the three advisory fmt.Fprintf(cmd.ErrOrStderr()) calls didn't check the return -> `_, _ =` (golangci-lint clean locally). - claude[bot] (correctness): the advisory branch fired only on HasErrors() and printed only Errors(), silently dropping SeverityWarning diagnostics. Now logs the full res.Diagnostics (each carries its [ERROR]/[WARNING]/[INFO] severity) so warnings are surfaced too; errors stay distinguished. - claude[bot] (doc): drop the stale "(forthcoming)" from the package comment — this PR lands the manager. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c18b062 to
13b3a44
Compare
What
PR2 of the ConfigManager MVP (PLT-775): fills in the v2
SeiConfigManagerbehindSEI_CONFIG_MANAGER=v2(the seam landed in #3671).v2 boot is validate-passthrough. It reads the config through the sei-config unified model to validate it, then re-enters the unchanged legacy reader on the operator's original files. It does not rewrite, migrate, or author
sei.tomlat boot. So the two consumed channels (serverCtx.Config+serverCtx.Viper) are produced identically to legacy by construction; v2's boot value-add is the validation pass.SeiConfigManager.Apply:resolveHomeDir(mirrors the legacy handler'sflag > SEID_ env > defaultresolution exactly) →ReadConfigFromDir→Validate→ re-enter. Advisory: read/validate problems are logged, never refuse boot.config.tomlparses for validation.serverCtx.Config+Viper.AllSettings()must match, incl. after thestart.gochain-id mutation.Scope / constraints
sei-cosmosfork. Diff is confined tocmd/seid/cmd/configmanager/+ the differential test +go.mod/go.sum.Acceptance-criteria drift contract
Per the canonical design (
designs/config-manager/DESIGN.md, updated with the boot-path pivot):Satisfies: Legacy untouched · v2 parity by construction (differential) · No silent fallback · Validation advisory (logged, never refuses boot; fails identically to legacy on a bad config) · Out-of-seam surface understood.
Deferred to the generate path (not MVP boot): the
SEI_collision audit, model→legacy render fidelity (the quoted-primitive / pruning write-side round-trip), and fatal validation (un-defer once sei-config read fidelity is hardened — a pruning read-mapping gap is surfaced advisory today).Lineage
bdchatham-designsdesigns/config-manager/DESIGN.md.🤖 Generated with Claude Code