From b8e951362d0fe91d4ce07b4f436d2114f9d8a758 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Tue, 30 Jun 2026 16:29:15 -0700 Subject: [PATCH 01/34] =?UTF-8?q?feat(configmanager):=20SeiConfigManager?= =?UTF-8?q?=20v2=20body=20=E2=80=94=20validate-passthrough=20(PLT-775=20PR?= =?UTF-8?q?2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/seid/cmd/configmanager/configmanager.go | 100 ++++++++++++++---- .../cmd/configmanager/configmanager_test.go | 6 -- .../cmd/configmanager_differential_test.go | 81 ++++++++++++++ go.mod | 5 +- go.sum | 10 +- 5 files changed, 168 insertions(+), 34 deletions(-) create mode 100644 cmd/seid/cmd/configmanager_differential_test.go diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index dd32f12d14..286a9d46c6 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -2,18 +2,26 @@ // loader and the (forthcoming) sei-config-backed manager, gated by the // SEI_CONFIG_MANAGER environment variable. // -// PR1 ships the seam only: LegacyConfigManager re-enters the unchanged -// legacy handler verbatim (the default), and SeiConfigManager is a -// not-yet-implemented stub that does not import the sei-config library. -// The v2 body lands in a follow-up PR. See PLT-775 and the canonical -// 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 +// produced identically to legacy. See PLT-775 and the canonical design +// (bdchatham-designs designs/config-manager/DESIGN.md). package configmanager import ( + "errors" "fmt" + "os" + "path" + "strings" "github.com/spf13/cobra" + "github.com/spf13/viper" + seiconfig "github.com/sei-protocol/sei-config" + + "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" "github.com/sei-protocol/sei-chain/sei-cosmos/server" ) @@ -24,18 +32,17 @@ const EnvVar = "SEI_CONFIG_MANAGER" // // Load-bearing contract: an implementation must leave both channels the app // consumes — serverCtx.Config and serverCtx.Viper — populated exactly as the -// legacy path does, and must be all-or-nothing with respect to on-disk state -// (no partial config.toml/app.toml materialization on error). A v2 manager -// that authors the two files from the sei-config library must write BOTH -// atomically and then re-enter the legacy read tail so the channels are -// produced identically — it must not feed app.New from an in-memory struct. +// legacy path does. 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, so the channels are identical to legacy by +// construction. (Authoring the canonical sei.toml and rendering the legacy +// files from it is the generate path; any implementation that writes config +// must be all-or-nothing on disk.) // // The Apply signature matches server.InterceptConfigsPreRunHandler so the -// legacy implementation forwards verbatim. This is an internal, -// single-consumer contract (only root.go calls it) and is free to grow — an -// explicit home dir or a Prepare/Apply split — when the v2 write-then-re-enter -// body lands in a follow-up PR; the node dir is resolvable from cmd, so the -// signature is sufficient for PR1's seam. +// legacy implementation forwards verbatim. This is an internal, single-consumer +// contract (only root.go calls it) and is free to grow when the generate path +// lands; the node dir is resolvable from cmd. type ConfigManager interface { Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error } @@ -49,15 +56,64 @@ func (LegacyConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate str return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) } -// SeiConfigManager will resolve configuration through the sei-config library -// behind SEI_CONFIG_MANAGER=v2. PR1 ships the seam only; the real body lands -// in a follow-up PR. It intentionally does not import sei-config. +// SeiConfigManager resolves configuration through the sei-config library, +// selected by SEI_CONFIG_MANAGER=v2. It reads the config through the unified +// SeiConfig model and surfaces validation diagnostics, then re-enters the +// legacy reader on the operator's original files — it does NOT rewrite them, +// migrate (that is the explicit `seid config migrate`), or author sei.toml (the +// generate path). So the produced config is identical to legacy by +// construction; v2's boot value-add is the validation pass. +// +// Validation is ADVISORY in this MVP: issues are logged, not enforced, and a +// read/validate problem never refuses boot. sei-config's read fidelity for a +// real seid config is still being hardened, so a model gap must not break an +// otherwise-valid node. Promoting validation to fatal (the design's +// refuse-on-error criterion) is the un-defer once the read round-trips real +// fixtures. type SeiConfigManager struct{} -// Apply is not yet implemented in PR1. It returns a hard error rather than -// silently falling back to the legacy path, so a v2 invocation is observable. -func (SeiConfigManager) Apply(_ *cobra.Command, _ string, _ any) error { - return fmt.Errorf("%s=v2 not yet implemented (PR1 seam only)", EnvVar) +// Apply surfaces sei-config validation diagnostics (advisory) and re-enters the +// legacy handler on the operator's original files. It does not write to disk, +// and never refuses boot on a read or validate problem. +func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { + if home, err := resolveHomeDir(cmd); err != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not resolve home dir for validation (advisory): %v\n", err) + } else if cfg, err := seiconfig.ReadConfigFromDir(home); err != nil { + // A missing config file (fresh/partial home) is normal — the legacy + // handler creates it. Any other read error is advisory, not fatal. + if !errors.Is(err, os.ErrNotExist) { + fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not read config for validation (advisory): %v\n", err) + } + } 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()) + } + + // Re-enter the unchanged legacy reader on the operator's original files. + return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) +} + +// resolveHomeDir mirrors the legacy handler's home resolution exactly +// (sei-cosmos/server/util.go: BindPFlags over the command's flags + the SEID_ +// env prefix + AutomaticEnv, then GetString(flags.FlagHome)), so the directory +// we materialize into is the same one the re-entered handler reads from. +// Resolving this single value the same way is not reimplementing the read tail +// — the read/merge/log-level tail stays delegated to InterceptConfigsPreRunHandler. +func resolveHomeDir(cmd *cobra.Command) (string, error) { + v := viper.New() + if err := v.BindPFlags(cmd.Flags()); err != nil { + return "", err + } + if err := v.BindPFlags(cmd.PersistentFlags()); err != nil { + return "", err + } + exe, err := os.Executable() + if err != nil { + return "", err + } + v.SetEnvPrefix(path.Base(exe)) + v.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) + v.AutomaticEnv() + return v.GetString(flags.FlagHome), nil } // Select returns the ConfigManager chosen by the SEI_CONFIG_MANAGER value: diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index 7320346f67..e5ba992dba 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -33,9 +33,3 @@ func TestSelect(t *testing.T) { }) } } - -// TestSeiConfigManagerNotImplemented asserts the v2 stub fails hard rather -// than silently behaving like legacy (PR1 ships the seam only). -func TestSeiConfigManagerNotImplemented(t *testing.T) { - require.Error(t, (SeiConfigManager{}).Apply(nil, "", nil)) -} diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go new file mode 100644 index 0000000000..686ee1760f --- /dev/null +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -0,0 +1,81 @@ +package cmd + +import ( + "context" + "errors" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" +) + +// errStopPreRun aborts the command after the config-resolution PreRunE, before +// StartCmd's RunE tries to boot a node. +var errStopPreRun = errors.New("stop after prerun") + +// runConfigManager runs mgr.Apply inside a StartCmd's PreRunE against homeDir, +// using seid's real app-config template, and returns the populated server +// context (the two channels start.go/app.New consume). It mirrors the harness +// in sei-cosmos/server/util_test.go. +func runConfigManager(t *testing.T, mgr configmanager.ConfigManager, homeDir string) *server.Context { + t.Helper() + template, appCfg := initAppConfig() + + cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) + require.NoError(t, cmd.Flags().Set(flags.FlagHome, homeDir)) + cmd.PreRunE = func(c *cobra.Command, _ []string) error { + if err := mgr.Apply(c, template, appCfg); err != nil { + return err + } + return errStopPreRun + } + + serverCtx := &server.Context{} + ctx := context.WithValue(context.Background(), server.ServerContextKey, serverCtx) + require.ErrorIs(t, cmd.ExecuteContext(ctx), errStopPreRun) + return serverCtx +} + +// TestConfigManagerLegacyVsV2Differential is the core safety property: the v2 +// manager must produce the SAME consumed config as the legacy path. v2 reads +// the config (to validate it) and then re-enters the legacy reader on the +// operator's ORIGINAL files — it does not rewrite — so the two paths read the +// SAME home and any difference is a real divergence, not a path artifact. +// +// It compares parsed semantics: +// - serverCtx.Config (the *tmcfg.Config the node runs on), and +// - serverCtx.Viper.AllSettings() (the AppOptions every Sei section reads via +// appOpts.Get), both at end-of-PersistentPreRunE and after the start.go +// chain-id mutation. +// +// A realistic fixture (all 11 Sei sections) is generated by letting the legacy +// handler create the full default config once; both managers then read it. +func TestConfigManagerLegacyVsV2Differential(t *testing.T) { + home := t.TempDir() + + // Generate a complete, realistic config via the legacy creator (fresh home). + _ = runConfigManager(t, configmanager.LegacyConfigManager{}, home) + + // Both managers read the same populated home. v2 is passthrough (no rewrite), + // so RootDir and every other path-derived field match by construction. + legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) + v2Ctx := runConfigManager(t, configmanager.SeiConfigManager{}, home) + + require.Equal(t, legacyCtx.Config, v2Ctx.Config, + "serverCtx.Config differs between legacy and v2") + require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), + "serverCtx.Viper settings differ between legacy and v2") + + // The start.go chain-id mutation is identical on both vipers; assert parity + // holds after it too (covers the post-mutation snapshot). + const chainID = "differential-test-1" + legacyCtx.Viper.Set(flags.FlagChainID, chainID) + v2Ctx.Viper.Set(flags.FlagChainID, chainID) + require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), + "settings diverge after the start.go chain-id mutation") +} diff --git a/go.mod b/go.mod index 1b94a8aef9..45413d7a33 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.25.6 require ( cosmossdk.io/errors v1.0.2 github.com/99designs/keyring v1.2.1 - github.com/BurntSushi/toml v1.4.1-0.20240526193622-a339e1f7089c + github.com/BurntSushi/toml v1.5.0 github.com/adlio/schema v1.3.9 github.com/alitto/pond v1.8.3 github.com/armon/go-metrics v0.4.1 @@ -71,6 +71,7 @@ require ( github.com/sasha-s/go-deadlock v0.3.5 github.com/segmentio/kafka-go v0.4.50 github.com/sei-protocol/goutils v0.0.2 + github.com/sei-protocol/sei-config v0.0.21 github.com/sei-protocol/sei-load v0.0.0-20251007135253-78fbdc141082 github.com/sei-protocol/sei-tm-db v0.0.5 github.com/sei-protocol/seilog v0.0.3 @@ -187,7 +188,7 @@ require ( github.com/go-logr/stdr v1.2.2 // indirect github.com/go-ole/go-ole v1.3.0 // indirect github.com/go-sourcemap/sourcemap v2.1.3+incompatible // indirect - github.com/go-viper/mapstructure/v2 v2.4.0 // indirect + github.com/go-viper/mapstructure/v2 v2.5.0 // indirect github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect github.com/golang/snappy v1.0.0 // indirect diff --git a/go.sum b/go.sum index f9a82a3eff..9ace91cdd0 100644 --- a/go.sum +++ b/go.sum @@ -619,8 +619,8 @@ github.com/AzureAD/microsoft-authentication-library-for-go v1.0.0/go.mod h1:kgDm github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/BurntSushi/toml v1.2.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/BurntSushi/toml v1.4.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= -github.com/BurntSushi/toml v1.4.1-0.20240526193622-a339e1f7089c h1:pxW6RcqyfI9/kWtOwnv/G+AzdKuy2ZrqINhenH4HyNs= -github.com/BurntSushi/toml v1.4.1-0.20240526193622-a339e1f7089c/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= +github.com/BurntSushi/toml v1.5.0 h1:W5quZX/G/csjUnuI8SUYlsHs9M38FC7znL0lIO+DvMg= +github.com/BurntSushi/toml v1.5.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= github.com/CloudyKit/fastprinter v0.0.0-20200109182630-33d98a066a53/go.mod h1:+3IMCy2vIlbG1XG/0ggNQv0SvxCAIpPM5b1nCz56Xno= github.com/CloudyKit/jet/v6 v6.2.0/go.mod h1:d3ypHeIRNo2+XyqnGA8s+aphtcVpjP5hPwP/Lzo7Ro4= @@ -1083,8 +1083,8 @@ github.com/go-sql-driver/mysql v1.7.0/go.mod h1:OXbVy3sEdcQ2Doequ6Z5BW6fXNQTmx+9 github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= github.com/go-stack/stack v1.8.1/go.mod h1:dcoOX6HbPZSZptuspn9bctJ+N/CnF5gGygcUP3XYfe4= github.com/go-task/slim-sprig v0.0.0-20210107165309-348f09dbbbc0/go.mod h1:fyg7847qk6SyHyPtNmDHnmrv/HOrqktSC+C9fM+CJOE= -github.com/go-viper/mapstructure/v2 v2.4.0 h1:EBsztssimR/CONLSZZ04E8qAkxNYq4Qp9LvH92wZUgs= -github.com/go-viper/mapstructure/v2 v2.4.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM= +github.com/go-viper/mapstructure/v2 v2.5.0 h1:vM5IJoUAy3d7zRSVtIwQgBj7BiWtMPfmPEgAXnvj1Ro= +github.com/go-viper/mapstructure/v2 v2.5.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM= github.com/gobwas/httphead v0.0.0-20180130184737-2c6c146eadee/go.mod h1:L0fX3K22YWvt/FAX9NnzrNzcI4wNYi9Yku4O0LKYflo= github.com/gobwas/httphead v0.1.0 h1:exrUm0f4YX0L7EBwZHuCF4GDp8aJfVeBrlLQrs6NqWU= github.com/gobwas/httphead v0.1.0/go.mod h1:O/RXo79gxV8G+RqlR/otEwx4Q36zl9rqC5u12GKvMCM= @@ -1808,6 +1808,8 @@ github.com/sei-protocol/go-ethereum v1.15.7-sei-17 h1:jUDHZqcKAiLi8nR0isZPZn8nDZ github.com/sei-protocol/go-ethereum v1.15.7-sei-17/go.mod h1:+S9k+jFzlyVTNcYGvqFhzN/SFhI6vA+aOY4T5tLSPL0= github.com/sei-protocol/goutils v0.0.2 h1:Bfa7Sv+4CVLNM20QcpvGb81B8C5HkQC/kW1CQpIbXDA= github.com/sei-protocol/goutils v0.0.2/go.mod h1:iYE2DuJfEnM+APPehr2gOUXfuLuPsVxorcDO+Tzq9q8= +github.com/sei-protocol/sei-config v0.0.21 h1:0VXOz91v9YeM2dmpKi1XaIFy+p3EamDAJtnj02XLKIw= +github.com/sei-protocol/sei-config v0.0.21/go.mod h1:zcEdLzyIH2AyP0/QRBE3s4Y9eGn0C/qAUx1c4o4EROU= github.com/sei-protocol/sei-load v0.0.0-20251007135253-78fbdc141082 h1:f2sY8OcN60UL1/6POx+HDMZ4w04FTZtSScnrFSnGZHg= github.com/sei-protocol/sei-load v0.0.0-20251007135253-78fbdc141082/go.mod h1:V0fNURAjS6A8+sA1VllegjNeSobay3oRUW5VFZd04bA= github.com/sei-protocol/sei-tm-db v0.0.5 h1:3WONKdSXEqdZZeLuWYfK5hP37TJpfaUa13vAyAlvaQY= From b5a71b734555888acfca9d211c27063ab688e5ff Mon Sep 17 00:00:00 2001 From: bdchatham Date: Tue, 30 Jun 2026 16:40:18 -0700 Subject: [PATCH 02/34] review(configmanager): xreview fixes (doc-drift, louder error advisory, home test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/seid/cmd/configmanager/configmanager.go | 26 ++++++++++++++----- .../cmd/configmanager/configmanager_test.go | 19 ++++++++++++++ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 286a9d46c6..d387ce7f57 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -3,9 +3,10 @@ // SEI_CONFIG_MANAGER environment variable. // // 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 +// default). SeiConfigManager reads the existing config through the sei-config +// library to *validate* it, then re-enters the same reader on the operator's +// original files (it does not rewrite them), so both channels are produced +// identically to legacy. See PLT-775 and the canonical design // (bdchatham-designs designs/config-manager/DESIGN.md). package configmanager @@ -75,17 +76,30 @@ type SeiConfigManager struct{} // Apply surfaces sei-config validation diagnostics (advisory) and re-enters the // legacy handler on the operator's original files. It does not write to disk, // and never refuses boot on a read or validate problem. +// +// Validation runs against the on-disk config.toml/app.toml only (via +// ReadConfigFromDir) — NOT the fully-merged AppOptions the node boots on (no +// flag/env/in-code-default layering). So a default node logs a benign advisory +// today (e.g. `storage.pruning` empty, because the template omits the key that +// cosmos defaults in code); this is expected, not a regression. That lower +// fidelity is acceptable precisely because validation is advisory here — parity +// comes from re-entry, not from the validated struct. func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { if home, err := resolveHomeDir(cmd); err != nil { fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not resolve home dir for validation (advisory): %v\n", err) } else if cfg, err := seiconfig.ReadConfigFromDir(home); err != nil { - // A missing config file (fresh/partial home) is normal — the legacy - // handler creates it. Any other read error is advisory, not fatal. + // A missing config.toml/app.toml (fresh home, or a partial home with one + // file absent) is normal — the legacy handler creates it. Any other read + // error is advisory, not fatal. if !errors.Is(err, os.ErrNotExist) { fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not read config for validation (advisory): %v\n", err) } } 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()) + // Advisory in this MVP: the node still boots. These are SeverityError + // findings — the same class (e.g. sc-write-mode) that legacy panics on + // later at app.New(); surfacing them here is earlier warning, not + // enforcement. Fatal refuse-on-error is the un-defer. + fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: ADVISORY config validation errors (not enforced; the node will boot, but these may surface later, e.g. at app.New()): %v\n", res.Errors()) } // Re-enter the unchanged legacy reader on the operator's original files. diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index e5ba992dba..9b2ccebc56 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -3,7 +3,10 @@ package configmanager import ( "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/require" + + "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" ) // TestSelect covers the dispatch table: unset and "legacy" select the @@ -33,3 +36,19 @@ func TestSelect(t *testing.T) { }) } } + +// TestResolveHomeDir_Flag confirms resolveHomeDir reads the --home flag — the +// value v2 validates against must be the dir the re-entered handler reads. (Env +// precedence follows viper, mirrored from the legacy handler; the env case is +// awkward to exercise here because the prefix is the test binary's basename, +// not SEID_, so it rides the generate-path PR — the boot differential already +// proves passthrough parity regardless of where home resolves.) +func TestResolveHomeDir_Flag(t *testing.T) { + cmd := &cobra.Command{} + cmd.Flags().String(flags.FlagHome, "", "") + require.NoError(t, cmd.Flags().Set(flags.FlagHome, "/tmp/seid-test-home")) + + got, err := resolveHomeDir(cmd) + require.NoError(t, err) + require.Equal(t, "/tmp/seid-test-home", got) +} From 13b3a44e8c7c190d1ab61c803a052058fcfb0513 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 09:42:14 -0700 Subject: [PATCH 03/34] fix(configmanager): lint errcheck + surface validation warnings (agentic findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/seid/cmd/configmanager/configmanager.go | 22 +++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index d387ce7f57..746922aee0 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -1,6 +1,6 @@ // Package configmanager is the selection seam between the legacy config -// loader and the (forthcoming) sei-config-backed manager, gated by the -// SEI_CONFIG_MANAGER environment variable. +// loader and the sei-config-backed manager, gated by the SEI_CONFIG_MANAGER +// environment variable. // // LegacyConfigManager re-enters the unchanged legacy handler verbatim (the // default). SeiConfigManager reads the existing config through the sei-config @@ -86,20 +86,22 @@ type SeiConfigManager struct{} // comes from re-entry, not from the validated struct. func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { if home, err := resolveHomeDir(cmd); err != nil { - fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not resolve home dir for validation (advisory): %v\n", err) + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not resolve home dir for validation (advisory): %v\n", err) } else if cfg, err := seiconfig.ReadConfigFromDir(home); err != nil { // A missing config.toml/app.toml (fresh home, or a partial home with one // file absent) is normal — the legacy handler creates it. Any other read // error is advisory, not fatal. if !errors.Is(err, os.ErrNotExist) { - fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not read config for validation (advisory): %v\n", err) + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not read config for validation (advisory): %v\n", err) } - } else if res := seiconfig.Validate(cfg); res.HasErrors() { - // Advisory in this MVP: the node still boots. These are SeverityError - // findings — the same class (e.g. sc-write-mode) that legacy panics on - // later at app.New(); surfacing them here is earlier warning, not - // enforcement. Fatal refuse-on-error is the un-defer. - fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: ADVISORY config validation errors (not enforced; the node will boot, but these may surface later, e.g. at app.New()): %v\n", res.Errors()) + } else if diags := seiconfig.Validate(cfg).Diagnostics; len(diags) > 0 { + // Advisory in this MVP: the node still boots. Surface ALL diagnostics — + // each carries its own [ERROR]/[WARNING]/[INFO] severity — so warnings + // are not silently dropped alongside errors. SeverityError findings (e.g. + // sc-write-mode) are the class legacy panics on later at app.New(); + // surfacing them here is earlier warning, not enforcement. Fatal + // refuse-on-error is the un-defer. + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: ADVISORY config validation diagnostics (not enforced; the node will boot, but SeverityError items may surface later, e.g. at app.New()): %v\n", diags) } // Re-enter the unchanged legacy reader on the operator's original files. From 9756ba4a31b5e6f574ff622dfbff14cdbf27e613 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 10:30:46 -0700 Subject: [PATCH 04/34] test(configmanager): cover env-precedence home resolution (DWC-1) The flag-driven differential never exercised resolveHomeDir's SetEnvPrefix/AutomaticEnv/replacer mirror of the legacy handler. Add an env-driven twin harness + TestConfigManagerLegacyVsV2Differential_EnvHome: drive home through the environment (no --home) and assert legacy and v2 resolve the SAME home, with a non-vacuous guard that the env var actually drove resolution. Closes the silent-drift seam the advisory design cannot otherwise surface (systems-engineer review). Co-Authored-By: Claude Opus 4.8 --- .../cmd/configmanager_differential_test.go | 77 +++++++++++++++++-- 1 file changed, 71 insertions(+), 6 deletions(-) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 686ee1760f..7814c2a1a1 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -3,6 +3,9 @@ package cmd import ( "context" "errors" + "os" + "path" + "strings" "testing" "github.com/spf13/cobra" @@ -18,16 +21,45 @@ import ( // StartCmd's RunE tries to boot a node. var errStopPreRun = errors.New("stop after prerun") -// runConfigManager runs mgr.Apply inside a StartCmd's PreRunE against homeDir, -// using seid's real app-config template, and returns the populated server -// context (the two channels start.go/app.New consume). It mirrors the harness -// in sei-cosmos/server/util_test.go. +// runConfigManager runs mgr.Apply inside a StartCmd's PreRunE against homeDir +// supplied via the --home flag, using seid's real app-config template, and +// returns the populated server context (the two channels start.go/app.New +// consume). It mirrors the harness in sei-cosmos/server/util_test.go. func runConfigManager(t *testing.T, mgr configmanager.ConfigManager, homeDir string) *server.Context { t.Helper() - template, appCfg := initAppConfig() - cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) require.NoError(t, cmd.Flags().Set(flags.FlagHome, homeDir)) + return execConfigManager(t, mgr, cmd) +} + +// runConfigManagerEnvHome is runConfigManager's twin that supplies homeDir +// through the environment instead of --home, exercising the +// SetEnvPrefix/AutomaticEnv/replacer machinery that resolveHomeDir mirrors from +// the legacy handler (the flag-driven path never touches it). The env prefix is +// path.Base(os.Executable()) — the test binary — derived identically by BOTH +// the legacy handler (sei-cosmos/server/util.go:152,162-164) and v2's +// resolveHomeDir, so both resolve the same home. viper's lookup key for "home" +// is ToUpper(prefix + "_" + "home") with the ".","-" -> "_" replacer applied. +func runConfigManagerEnvHome(t *testing.T, mgr configmanager.ConfigManager, homeDir string) *server.Context { + t.Helper() + exe, err := os.Executable() + require.NoError(t, err) + envKey := strings.NewReplacer(".", "_", "-", "_").Replace( + strings.ToUpper(path.Base(exe) + "_" + flags.FlagHome)) + t.Setenv(envKey, homeDir) + + // Leave --home unset: an unchanged flag default ranks below AutomaticEnv in + // viper's precedence, so the env value is what resolves. + cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) + return execConfigManager(t, mgr, cmd) +} + +// execConfigManager runs mgr.Apply inside cmd's PreRunE (aborting before the +// node boots) and returns the populated server context. The caller configures +// how home is supplied (flag vs env) on cmd beforehand. +func execConfigManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) *server.Context { + t.Helper() + template, appCfg := initAppConfig() cmd.PreRunE = func(c *cobra.Command, _ []string) error { if err := mgr.Apply(c, template, appCfg); err != nil { return err @@ -79,3 +111,36 @@ func TestConfigManagerLegacyVsV2Differential(t *testing.T) { require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), "settings diverge after the start.go chain-id mutation") } + +// TestConfigManagerLegacyVsV2Differential_EnvHome exercises the env-precedence +// half of resolveHomeDir's mirror of the legacy handler — the flag-driven +// differential above never touches SetEnvPrefix/AutomaticEnv. When home is +// supplied via the environment (not --home), v2 must resolve the SAME home the +// legacy handler does; otherwise v2 would advisorily validate one dir while the +// re-entered legacy reader boots on another — a silent drift the advisory +// design cannot surface (no error, no diagnostic). This pins the seam so a +// future change to the legacy env-resolution can't diverge undetected. +func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { + home := t.TempDir() + + // Populate a complete realistic config in `home` via the fresh-home legacy + // creator, driven entirely through the env var (no --home). + _ = runConfigManagerEnvHome(t, configmanager.LegacyConfigManager{}, home) + + legacyCtx := runConfigManagerEnvHome(t, configmanager.LegacyConfigManager{}, home) + v2Ctx := runConfigManagerEnvHome(t, configmanager.SeiConfigManager{}, home) + + // Non-vacuous guard: the env var actually drove resolution. If the key were + // wrong, both would fall back to StartCmd's "/foobar" default (and the + // legacy creator would fail writing under it) — this asserts the env path + // resolved to the temp home, for both managers. + require.Equal(t, home, v2Ctx.Viper.GetString(flags.FlagHome), + "env-provided home did not drive v2 resolution") + require.Equal(t, home, legacyCtx.Viper.GetString(flags.FlagHome), + "env-provided home did not drive legacy resolution") + + require.Equal(t, legacyCtx.Config, v2Ctx.Config, + "serverCtx.Config differs between legacy and v2 on the env-home path") + require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), + "serverCtx.Viper settings differ between legacy and v2 on the env-home path") +} From 393578d07225bd6863d771ae5d475168c1d2450f Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 10:52:23 -0700 Subject: [PATCH 05/34] docs(configmanager): fix stale env-case comment on TestResolveHomeDir_Flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DWC-1 commit (9756ba4) added TestConfigManagerLegacyVsV2Differential_EnvHome covering the env-driven home resolution, but left the TestResolveHomeDir_Flag comment claiming the env case 'rides the generate-path PR' — now self- contradictory. Point it at the env-home differential test instead (claude[bot]). Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index 9b2ccebc56..336e1879a0 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -39,10 +39,10 @@ func TestSelect(t *testing.T) { // TestResolveHomeDir_Flag confirms resolveHomeDir reads the --home flag — the // value v2 validates against must be the dir the re-entered handler reads. (Env -// precedence follows viper, mirrored from the legacy handler; the env case is -// awkward to exercise here because the prefix is the test binary's basename, -// not SEID_, so it rides the generate-path PR — the boot differential already -// proves passthrough parity regardless of where home resolves.) +// precedence follows viper, mirrored from the legacy handler; the end-to-end +// env-driven case is exercised by TestConfigManagerLegacyVsV2Differential_EnvHome +// in the cmd package, which resolves the test-binary-basename prefix and asserts +// legacy/v2 parity.) func TestResolveHomeDir_Flag(t *testing.T) { cmd := &cobra.Command{} cmd.Flags().String(flags.FlagHome, "", "") From 120a6832a6ddc8f73013fdbb1828561b34a5ae2c Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 13:13:59 -0700 Subject: [PATCH 06/34] docs(configmanager): move narrative to doc.go, trim to human-audience form The in-file documentation was overbearing. Move the package narrative into a dedicated doc.go (single home for the why), cut each exported symbol's go-doc to its point, and drop the inline block comments in Apply so the code reads on its own. doc.go leads with the safety invariant (never rewrites, never refuses boot) per the /lingua R2 pass (prose-steward). Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager.go | 98 ++++----------------- cmd/seid/cmd/configmanager/doc.go | 19 ++++ 2 files changed, 38 insertions(+), 79 deletions(-) create mode 100644 cmd/seid/cmd/configmanager/doc.go diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 746922aee0..72129a544d 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -1,13 +1,3 @@ -// Package configmanager is the selection seam between the legacy config -// loader and the sei-config-backed manager, gated by the SEI_CONFIG_MANAGER -// environment variable. -// -// LegacyConfigManager re-enters the unchanged legacy handler verbatim (the -// default). SeiConfigManager reads the existing config through the sei-config -// library to *validate* it, then re-enters the same reader on the operator's -// original files (it does not rewrite them), so both channels are produced -// identically to legacy. See PLT-775 and the canonical design -// (bdchatham-designs designs/config-manager/DESIGN.md). package configmanager import ( @@ -26,94 +16,48 @@ import ( "github.com/sei-protocol/sei-chain/sei-cosmos/server" ) -// EnvVar is the experimental gate that selects the configuration manager. +// EnvVar gates which configuration manager seid uses. const EnvVar = "SEI_CONFIG_MANAGER" // ConfigManager resolves a seid node's configuration during PersistentPreRunE. -// -// Load-bearing contract: an implementation must leave both channels the app -// consumes — serverCtx.Config and serverCtx.Viper — populated exactly as the -// legacy path does. 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, so the channels are identical to legacy by -// construction. (Authoring the canonical sei.toml and rendering the legacy -// files from it is the generate path; any implementation that writes config -// must be all-or-nothing on disk.) -// -// The Apply signature matches server.InterceptConfigsPreRunHandler so the -// legacy implementation forwards verbatim. This is an internal, single-consumer -// contract (only root.go calls it) and is free to grow when the generate path -// lands; the node dir is resolvable from cmd. +// An implementation must leave serverCtx.Config and serverCtx.Viper populated +// exactly as the legacy path does. The Apply signature matches +// server.InterceptConfigsPreRunHandler so the legacy manager forwards verbatim. type ConfigManager interface { Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error } -// LegacyConfigManager is the default manager: it re-enters the unchanged -// legacy handler verbatim, so the legacy path stays byte-for-byte unaffected. +// LegacyConfigManager is the default manager. It forwards to the legacy handler +// unchanged, leaving the legacy path byte-for-byte unaffected. type LegacyConfigManager struct{} -// Apply delegates to the legacy interception handler unchanged. +// Apply forwards to the legacy interception handler unchanged. func (LegacyConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) } -// SeiConfigManager resolves configuration through the sei-config library, -// selected by SEI_CONFIG_MANAGER=v2. It reads the config through the unified -// SeiConfig model and surfaces validation diagnostics, then re-enters the -// legacy reader on the operator's original files — it does NOT rewrite them, -// migrate (that is the explicit `seid config migrate`), or author sei.toml (the -// generate path). So the produced config is identical to legacy by -// construction; v2's boot value-add is the validation pass. -// -// Validation is ADVISORY in this MVP: issues are logged, not enforced, and a -// read/validate problem never refuses boot. sei-config's read fidelity for a -// real seid config is still being hardened, so a model gap must not break an -// otherwise-valid node. Promoting validation to fatal (the design's -// refuse-on-error criterion) is the un-defer once the read round-trips real -// fixtures. +// SeiConfigManager validates the config through the sei-config library, then +// re-enters the legacy handler on the operator's original files. It never +// writes, migrates, or refuses boot. type SeiConfigManager struct{} -// Apply surfaces sei-config validation diagnostics (advisory) and re-enters the -// legacy handler on the operator's original files. It does not write to disk, -// and never refuses boot on a read or validate problem. -// -// Validation runs against the on-disk config.toml/app.toml only (via -// ReadConfigFromDir) — NOT the fully-merged AppOptions the node boots on (no -// flag/env/in-code-default layering). So a default node logs a benign advisory -// today (e.g. `storage.pruning` empty, because the template omits the key that -// cosmos defaults in code); this is expected, not a regression. That lower -// fidelity is acceptable precisely because validation is advisory here — parity -// comes from re-entry, not from the validated struct. +// Apply prints advisory validation diagnostics, then re-enters the legacy +// handler. A missing config file is not an error, and nothing here refuses boot. func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { if home, err := resolveHomeDir(cmd); err != nil { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not resolve home dir for validation (advisory): %v\n", err) } else if cfg, err := seiconfig.ReadConfigFromDir(home); err != nil { - // A missing config.toml/app.toml (fresh home, or a partial home with one - // file absent) is normal — the legacy handler creates it. Any other read - // error is advisory, not fatal. if !errors.Is(err, os.ErrNotExist) { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not read config for validation (advisory): %v\n", err) } } else if diags := seiconfig.Validate(cfg).Diagnostics; len(diags) > 0 { - // Advisory in this MVP: the node still boots. Surface ALL diagnostics — - // each carries its own [ERROR]/[WARNING]/[INFO] severity — so warnings - // are not silently dropped alongside errors. SeverityError findings (e.g. - // sc-write-mode) are the class legacy panics on later at app.New(); - // surfacing them here is earlier warning, not enforcement. Fatal - // refuse-on-error is the un-defer. - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: ADVISORY config validation diagnostics (not enforced; the node will boot, but SeverityError items may surface later, e.g. at app.New()): %v\n", diags) + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: advisory validation diagnostics (not enforced; node will boot): %v\n", diags) } - - // Re-enter the unchanged legacy reader on the operator's original files. return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) } -// resolveHomeDir mirrors the legacy handler's home resolution exactly -// (sei-cosmos/server/util.go: BindPFlags over the command's flags + the SEID_ -// env prefix + AutomaticEnv, then GetString(flags.FlagHome)), so the directory -// we materialize into is the same one the re-entered handler reads from. -// Resolving this single value the same way is not reimplementing the read tail -// — the read/merge/log-level tail stays delegated to InterceptConfigsPreRunHandler. +// resolveHomeDir resolves --home the same way the legacy handler does +// (sei-cosmos/server/util.go), so v2 validates the directory the handler reads. func resolveHomeDir(cmd *cobra.Command) (string, error) { v := viper.New() if err := v.BindPFlags(cmd.Flags()); err != nil { @@ -132,14 +76,10 @@ func resolveHomeDir(cmd *cobra.Command) (string, error) { return v.GetString(flags.FlagHome), nil } -// Select returns the ConfigManager chosen by the SEI_CONFIG_MANAGER value: -// unset or "legacy" -> LegacyConfigManager (the default); "v2" -> -// SeiConfigManager; any other value -> error (never a silent fallback). -// getenv is injected for testability; production callers pass os.Getenv. -// -// The value is matched exactly — no trimming or case-folding. This is -// deliberate: normalizing would broaden the gate, and the error names the -// legal tokens so an operator can self-correct. +// Select maps SEI_CONFIG_MANAGER to a manager: unset or "legacy" -> Legacy, +// "v2" -> Sei, anything else -> error. The value is matched exactly (no +// trimming or case-folding) and never falls back silently. getenv is injected +// for tests; callers pass os.Getenv. func Select(getenv func(string) string) (ConfigManager, error) { switch v := getenv(EnvVar); v { case "", "legacy": diff --git a/cmd/seid/cmd/configmanager/doc.go b/cmd/seid/cmd/configmanager/doc.go new file mode 100644 index 0000000000..fe3a5d1281 --- /dev/null +++ b/cmd/seid/cmd/configmanager/doc.go @@ -0,0 +1,19 @@ +// Package configmanager selects how seid loads its configuration, behind the +// SEI_CONFIG_MANAGER gate. Every manager boots the node identically; the only +// variable is an advisory validation pass that never rewrites a file and never +// refuses boot. +// +// SEI_CONFIG_MANAGER picks the manager: unset or "legacy" uses the legacy +// loader unchanged; "v2" uses the sei-config-backed manager. root.go calls +// Select once, during PersistentPreRunE. +// +// Both managers boot from the same two channels — serverCtx.Config and +// serverCtx.Viper — and v2 populates them exactly as legacy does: it re-enters +// the legacy reader on the operator's own files instead of rewriting them. Its +// validation pass is advisory because sei-config's read fidelity is still being +// hardened, and a gap in the model must not fail a valid node. +// +// Two things are deferred: making validation fatal, and authoring a canonical +// sei.toml to render the legacy files from (the generate path). See PLT-775 and +// the design (bdchatham-designs designs/config-manager/DESIGN.md). +package configmanager From d0bf870b3d8198bd3beab54407b739f90b9b68c2 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 13:38:18 -0700 Subject: [PATCH 07/34] =?UTF-8?q?test(configmanager):=20rich=20parity=20su?= =?UTF-8?q?rface=20=E2=80=94=20corpus,=20advisory-invariance,=20fuzz?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widen the legacy-vs-v2 proof beyond the single default fixture, all unit-tier: - Corpus differential (table-driven): default, leading comments/blanks, unknown section, quoted scalar (#36 lenient-decode) — parity holds for any on-disk shape, including ones that exercise sei-config's own reader. - Advisory-invariance: v2 never refuses boot on a valid config, and on an unreadable config it returns the SAME error legacy does (not masked, not invented) after logging the advisory read failure. - FuzzConfigManagerLegacyVsV2Parity: arbitrary app.toml suffix -> legacy and v2 reach the same outcome (identical channels, or the identical error). Runs the seed corpus deterministically in CI; open to -fuzz locally. Harness: runManager now captures stderr + the Apply error so the advisory paths are observable; execConfigManager keeps the happy-path assertion. Co-Authored-By: Claude Opus 4.8 --- .../cmd/configmanager_differential_test.go | 184 +++++++++++++++++- 1 file changed, 176 insertions(+), 8 deletions(-) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 7814c2a1a1..bdc3912155 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -1,10 +1,12 @@ package cmd import ( + "bytes" "context" "errors" "os" "path" + "path/filepath" "strings" "testing" @@ -26,10 +28,16 @@ var errStopPreRun = errors.New("stop after prerun") // returns the populated server context (the two channels start.go/app.New // consume). It mirrors the harness in sei-cosmos/server/util_test.go. func runConfigManager(t *testing.T, mgr configmanager.ConfigManager, homeDir string) *server.Context { + t.Helper() + return execConfigManager(t, mgr, startCmdForHome(t, homeDir)) +} + +// startCmdForHome builds a StartCmd with --home set to homeDir. +func startCmdForHome(t *testing.T, homeDir string) *cobra.Command { t.Helper() cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) require.NoError(t, cmd.Flags().Set(flags.FlagHome, homeDir)) - return execConfigManager(t, mgr, cmd) + return cmd } // runConfigManagerEnvHome is runConfigManager's twin that supplies homeDir @@ -54,23 +62,75 @@ func runConfigManagerEnvHome(t *testing.T, mgr configmanager.ConfigManager, home return execConfigManager(t, mgr, cmd) } -// execConfigManager runs mgr.Apply inside cmd's PreRunE (aborting before the -// node boots) and returns the populated server context. The caller configures -// how home is supplied (flag vs env) on cmd beforehand. +// execConfigManager runs mgr.Apply on the happy path (Apply succeeds; boot is +// aborted with errStopPreRun) and returns the populated server context. The +// caller configures how home is supplied (flag vs env) on cmd beforehand. func execConfigManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) *server.Context { + t.Helper() + ctx, _, err := runManager(t, mgr, cmd) + require.NoError(t, err) + return ctx +} + +// runManager runs mgr.Apply inside cmd's PreRunE, capturing what an operator +// would see and do: the populated server context, whatever Apply wrote to +// stderr (advisory diagnostics), and the error Apply returned. Apply is the +// only boot-refusing call, so on the happy path it returns nil and boot is +// aborted with errStopPreRun; on a real config error it returns that error and +// runManager surfaces it unchanged. The caller sets home on cmd beforehand. +func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) (*server.Context, string, error) { t.Helper() template, appCfg := initAppConfig() + + var stderr bytes.Buffer + cmd.SetErr(&stderr) + + var applyErr error cmd.PreRunE = func(c *cobra.Command, _ []string) error { - if err := mgr.Apply(c, template, appCfg); err != nil { - return err + if applyErr = mgr.Apply(c, template, appCfg); applyErr != nil { + return applyErr } return errStopPreRun } serverCtx := &server.Context{} ctx := context.WithValue(context.Background(), server.ServerContextKey, serverCtx) - require.ErrorIs(t, cmd.ExecuteContext(ctx), errStopPreRun) - return serverCtx + execErr := cmd.ExecuteContext(ctx) + if applyErr == nil { + require.ErrorIs(t, execErr, errStopPreRun) + } + return serverCtx, stderr.String(), applyErr +} + +// appTOMLPath and cfgTOMLPath are the two files the legacy creator writes into a +// home and both managers then read. +func appTOMLPath(home string) string { return filepath.Join(home, "config", "app.toml") } +func cfgTOMLPath(home string) string { return filepath.Join(home, "config", "config.toml") } + +// seedDefaultConfig generates a complete, realistic config (all Sei sections) by +// letting the legacy creator write into a fresh home, and returns that home. +func seedDefaultConfig(t *testing.T) string { + t.Helper() + home := t.TempDir() + _ = runConfigManager(t, configmanager.LegacyConfigManager{}, home) + return home +} + +// appendToFile appends s to the file at path (both managers must still agree on +// the result). +func appendToFile(t *testing.T, path, s string) { + t.Helper() + b, err := os.ReadFile(path) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, append(b, []byte(s)...), 0o600)) +} + +// prependToFile prepends s to the file at path. +func prependToFile(t *testing.T, path, s string) { + t.Helper() + b, err := os.ReadFile(path) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, append([]byte(s), b...), 0o600)) } // TestConfigManagerLegacyVsV2Differential is the core safety property: the v2 @@ -144,3 +204,111 @@ func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), "serverCtx.Viper settings differ between legacy and v2 on the env-home path") } + +// TestConfigManagerLegacyVsV2Differential_Corpus widens the parity proof from +// the single default fixture to a corpus of realistic on-disk shapes. Parity is +// by construction (v2 re-enters the legacy reader), so any shape an operator +// could present must produce identical channels — including shapes that exercise +// sei-config's own reader (quoted scalars, unknown keys), whose advisory read +// still must not perturb what the node boots on. +func TestConfigManagerLegacyVsV2Differential_Corpus(t *testing.T) { + cases := []struct { + name string + mutate func(t *testing.T, home string) + }{ + {"default", func(t *testing.T, home string) {}}, + {"leading-comments-and-blanks", func(t *testing.T, home string) { + prependToFile(t, appTOMLPath(home), "# corpus: a leading comment\n\n") + }}, + {"unknown-section", func(t *testing.T, home string) { + appendToFile(t, appTOMLPath(home), "\n[sei-corpus-unknown]\nkey = \"value\"\n") + }}, + {"quoted-scalar", func(t *testing.T, home string) { + // A number written as a quoted string — the sei-config lenient-decode + // case (#36). v2 reads it; the channels must still match legacy. + appendToFile(t, appTOMLPath(home), "\n[sei-corpus-scalar]\ncount = \"100000\"\n") + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + home := seedDefaultConfig(t) + tc.mutate(t, home) + + legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) + v2Ctx := runConfigManager(t, configmanager.SeiConfigManager{}, home) + + require.Equal(t, legacyCtx.Config, v2Ctx.Config, + "serverCtx.Config differs between legacy and v2 (%s)", tc.name) + require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), + "serverCtx.Viper settings differ between legacy and v2 (%s)", tc.name) + }) + } +} + +// TestConfigManagerV2AdvisoryNeverRefusesBoot pins the advisory invariant: on a +// valid config, v2 boots exactly as legacy does (Apply returns nil, both +// channels match), regardless of any diagnostics it prints. v2 adds +// observability, never a new boot outcome. +func TestConfigManagerV2AdvisoryNeverRefusesBoot(t *testing.T) { + home := seedDefaultConfig(t) + + v2Ctx, _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) + require.NoError(t, v2Err, "advisory validation must never refuse boot on a valid config") + + legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) + require.Equal(t, legacyCtx.Config, v2Ctx.Config) + require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings()) +} + +// TestConfigManagerV2AdvisoryReadErrorMatchesLegacy pins the other half of the +// invariant: when the config is unreadable, v2 must not mask the failure or +// invent a new one. It logs an advisory read error, then re-enters the legacy +// handler and returns exactly the error legacy returns. +func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { + home := seedDefaultConfig(t) + require.NoError(t, os.WriteFile(cfgTOMLPath(home), []byte("this is ] not [ valid toml"), 0o600)) + + _, _, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) + _, v2Stderr, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) + + require.Error(t, legacyErr, "corrupt config.toml should fail the legacy reader") + require.Equal(t, legacyErr.Error(), v2Err.Error(), + "v2 must return the same boot error as legacy, not mask or add one") + require.Contains(t, v2Stderr, "could not read config for validation", + "v2 should log the advisory read failure before re-entering legacy") +} + +// FuzzConfigManagerLegacyVsV2Parity is the exhaustive form of the corpus: for an +// arbitrary suffix appended to a valid app.toml, legacy and v2 must reach the +// same outcome — identical channels when both succeed, the identical error when +// both fail. Parity is by construction, so the fuzzer should never find a +// divergence. Under `go test` (no -fuzz) it runs the seed corpus, which is a +// deterministic differential in CI. +func FuzzConfigManagerLegacyVsV2Parity(f *testing.F) { + for _, seed := range []string{ + "", + "\n# a trailing comment\n", + "\n[fuzz-extra]\nk = \"v\"\n", + "\n[fuzz-scalar]\nn = \"100000\"\n", + "\nnot valid toml ][", + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, appTOMLSuffix string) { + home := seedDefaultConfig(t) + appendToFile(t, appTOMLPath(home), appTOMLSuffix) + + legacyCtx, _, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) + v2Ctx, _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) + + if (legacyErr == nil) != (v2Err == nil) { + t.Fatalf("divergent outcome for suffix %q: legacyErr=%v v2Err=%v", appTOMLSuffix, legacyErr, v2Err) + } + if legacyErr != nil { + require.Equal(t, legacyErr.Error(), v2Err.Error(), "divergent error for suffix %q", appTOMLSuffix) + return + } + require.Equal(t, legacyCtx.Config, v2Ctx.Config, "Config diverges for suffix %q", appTOMLSuffix) + require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), "settings diverge for suffix %q", appTOMLSuffix) + }) +} From 35842d92beffc879d231c8b5d639a2a635f783d7 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 13:57:53 -0700 Subject: [PATCH 08/34] refactor(configmanager): named-step Apply, shared corpus, reduction doc Adopt the systems-engineer's extensible pattern for the workstream: - Apply is now two named steps (validateAdvisory -> re-enter legacy), so the generate path can add its authoring/render step as a sibling instead of rewriting Apply. Advisory pass is a void helper; boot outcome is unchanged. - Shared configCorpus() is the single source of interesting on-disk shapes, consumed by BOTH the table-driven differential and the fuzz target (fuzz now crosses every shape with an arbitrary suffix). Kills the corpus/seed drift. - Add the cosmos-only-write-mode case: the deprecated state-commit.sc-write-mode version-skew class. sei-config still accepts it, so v2 raises no diagnostic today; the case proves read-parity now and becomes a caught case once fatal validation + a sei-config deprecation rule land. - doc.go states the reduction lemma: the node boots on the two channels, not the model, so channel-equality is the whole correctness argument. Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager.go | 28 ++++- cmd/seid/cmd/configmanager/doc.go | 4 + .../cmd/configmanager_differential_test.go | 108 ++++++++++++------ 3 files changed, 97 insertions(+), 43 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 72129a544d..0f77030b59 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -41,19 +41,35 @@ func (LegacyConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate str // writes, migrates, or refuses boot. type SeiConfigManager struct{} -// Apply prints advisory validation diagnostics, then re-enters the legacy -// handler. A missing config file is not an error, and nothing here refuses boot. +// Apply runs the advisory validation pass, then re-enters the legacy handler on +// the operator's original files. Nothing in the validation pass refuses boot. func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { - if home, err := resolveHomeDir(cmd); err != nil { + validateAdvisory(cmd) + return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) +} + +// validateAdvisory resolves the home dir, reads the on-disk config, and prints +// any validation diagnostics to stderr. Every step is advisory: a failure is +// logged and swallowed so the pass can never change what the node boots on. A +// missing config file is normal (the legacy handler creates it) and is not +// surfaced. Keeping this a distinct step from Apply is what lets the generate +// path add its authoring/render step as a sibling. +func validateAdvisory(cmd *cobra.Command) { + home, err := resolveHomeDir(cmd) + if err != nil { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not resolve home dir for validation (advisory): %v\n", err) - } else if cfg, err := seiconfig.ReadConfigFromDir(home); err != nil { + return + } + cfg, err := seiconfig.ReadConfigFromDir(home) + if err != nil { if !errors.Is(err, os.ErrNotExist) { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not read config for validation (advisory): %v\n", err) } - } else if diags := seiconfig.Validate(cfg).Diagnostics; len(diags) > 0 { + return + } + if diags := seiconfig.Validate(cfg).Diagnostics; len(diags) > 0 { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: advisory validation diagnostics (not enforced; node will boot): %v\n", diags) } - return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) } // resolveHomeDir resolves --home the same way the legacy handler does diff --git a/cmd/seid/cmd/configmanager/doc.go b/cmd/seid/cmd/configmanager/doc.go index fe3a5d1281..3db2733fe2 100644 --- a/cmd/seid/cmd/configmanager/doc.go +++ b/cmd/seid/cmd/configmanager/doc.go @@ -13,6 +13,10 @@ // validation pass is advisory because sei-config's read fidelity is still being // hardened, and a gap in the model must not fail a valid node. // +// The node boots on those two channels, never on the SeiConfig model; that is +// why differential tests suffice — proving v2's channels equal legacy's, which +// legacy is already trusted to boot on, is the whole correctness argument. +// // Two things are deferred: making validation fatal, and authoring a canonical // sei.toml to render the legacy files from (the generate path). See PLT-775 and // the design (bdchatham-designs designs/config-manager/DESIGN.md). diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index bdc3912155..5bb8b0f87f 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -133,6 +133,52 @@ func prependToFile(t *testing.T, path, s string) { require.NoError(t, os.WriteFile(path, append([]byte(s), b...), 0o600)) } +// replaceInFile replaces old with new in the file at path, asserting old was +// present so a corpus mutation can never silently become a no-op. +func replaceInFile(t *testing.T, path, old, new string) { + t.Helper() + b, err := os.ReadFile(path) + require.NoError(t, err) + require.Contains(t, string(b), old, "replace target %q not found — fixture would be vacuous", old) + require.NoError(t, os.WriteFile(path, []byte(strings.ReplaceAll(string(b), old, new)), 0o600)) +} + +// corpusCase is one realistic on-disk config shape, applied to a freshly-seeded +// default home. It is the shared unit both the table-driven differential and the +// fuzz target consume, so the set of "interesting shapes" lives in one place. +type corpusCase struct { + name string + mutate func(t *testing.T, home string) +} + +// configCorpus is the single source of the config shapes the parity proof runs +// over. Each case mutates a default home in place; parity must hold for all of +// them because v2 re-enters the legacy reader regardless of what it read. +func configCorpus() []corpusCase { + return []corpusCase{ + {"default", func(t *testing.T, home string) {}}, + {"leading-comments-and-blanks", func(t *testing.T, home string) { + prependToFile(t, appTOMLPath(home), "# corpus: a leading comment\n\n") + }}, + {"unknown-section", func(t *testing.T, home string) { + appendToFile(t, appTOMLPath(home), "\n[sei-corpus-unknown]\nkey = \"value\"\n") + }}, + {"quoted-scalar", func(t *testing.T, home string) { + // A number written as a quoted string — the sei-config lenient-decode + // case (#36). v2 reads it; the channels must still match legacy. + appendToFile(t, appTOMLPath(home), "\n[sei-corpus-scalar]\ncount = \"100000\"\n") + }}, + {"cosmos-only-write-mode", func(t *testing.T, home string) { + // The version-skew class: a config carrying the deprecated + // state-commit.sc-write-mode "cosmos_only". sei-config still accepts it + // as valid, so v2 raises no diagnostic today; the point here is that + // both managers read it identically (parity). It becomes a *caught* + // case only once fatal validation + a sei-config deprecation rule land. + replaceInFile(t, appTOMLPath(home), `sc-write-mode = "memiavl_only"`, `sc-write-mode = "cosmos_only"`) + }}, + } +} + // TestConfigManagerLegacyVsV2Differential is the core safety property: the v2 // manager must produce the SAME consumed config as the legacy path. v2 reads // the config (to validate it) and then re-enters the legacy reader on the @@ -212,24 +258,7 @@ func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { // sei-config's own reader (quoted scalars, unknown keys), whose advisory read // still must not perturb what the node boots on. func TestConfigManagerLegacyVsV2Differential_Corpus(t *testing.T) { - cases := []struct { - name string - mutate func(t *testing.T, home string) - }{ - {"default", func(t *testing.T, home string) {}}, - {"leading-comments-and-blanks", func(t *testing.T, home string) { - prependToFile(t, appTOMLPath(home), "# corpus: a leading comment\n\n") - }}, - {"unknown-section", func(t *testing.T, home string) { - appendToFile(t, appTOMLPath(home), "\n[sei-corpus-unknown]\nkey = \"value\"\n") - }}, - {"quoted-scalar", func(t *testing.T, home string) { - // A number written as a quoted string — the sei-config lenient-decode - // case (#36). v2 reads it; the channels must still match legacy. - appendToFile(t, appTOMLPath(home), "\n[sei-corpus-scalar]\ncount = \"100000\"\n") - }}, - } - for _, tc := range cases { + for _, tc := range configCorpus() { t.Run(tc.name, func(t *testing.T) { home := seedDefaultConfig(t) tc.mutate(t, home) @@ -278,37 +307,42 @@ func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { "v2 should log the advisory read failure before re-entering legacy") } -// FuzzConfigManagerLegacyVsV2Parity is the exhaustive form of the corpus: for an -// arbitrary suffix appended to a valid app.toml, legacy and v2 must reach the -// same outcome — identical channels when both succeed, the identical error when -// both fail. Parity is by construction, so the fuzzer should never find a -// divergence. Under `go test` (no -fuzz) it runs the seed corpus, which is a -// deterministic differential in CI. +// FuzzConfigManagerLegacyVsV2Parity is the exhaustive form of the corpus: it +// crosses every corpus shape with an arbitrary appended app.toml suffix, and +// asserts legacy and v2 reach the same outcome — identical channels when both +// succeed, the identical error when both fail. Parity is by construction, so the +// fuzzer should never find a divergence. Under `go test` (no -fuzz) it runs the +// seed corpus (each shape × a few suffixes), a deterministic differential in CI; +// under -fuzz it explores suffixes against every shape. func FuzzConfigManagerLegacyVsV2Parity(f *testing.F) { - for _, seed := range []string{ - "", - "\n# a trailing comment\n", - "\n[fuzz-extra]\nk = \"v\"\n", - "\n[fuzz-scalar]\nn = \"100000\"\n", - "\nnot valid toml ][", - } { - f.Add(seed) + corpus := configCorpus() + for i := range corpus { + f.Add(i, "") + f.Add(i, "\n# a trailing comment\n") + f.Add(i, "\nnot valid toml ][") } - f.Fuzz(func(t *testing.T, appTOMLSuffix string) { + + f.Fuzz(func(t *testing.T, corpusIdx int, appTOMLSuffix string) { + if corpusIdx < 0 { + corpusIdx = -corpusIdx + } + tc := corpus[corpusIdx%len(corpus)] + home := seedDefaultConfig(t) + tc.mutate(t, home) appendToFile(t, appTOMLPath(home), appTOMLSuffix) legacyCtx, _, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) v2Ctx, _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) if (legacyErr == nil) != (v2Err == nil) { - t.Fatalf("divergent outcome for suffix %q: legacyErr=%v v2Err=%v", appTOMLSuffix, legacyErr, v2Err) + t.Fatalf("divergent outcome (case %q, suffix %q): legacyErr=%v v2Err=%v", tc.name, appTOMLSuffix, legacyErr, v2Err) } if legacyErr != nil { - require.Equal(t, legacyErr.Error(), v2Err.Error(), "divergent error for suffix %q", appTOMLSuffix) + require.Equal(t, legacyErr.Error(), v2Err.Error(), "divergent error (case %q, suffix %q)", tc.name, appTOMLSuffix) return } - require.Equal(t, legacyCtx.Config, v2Ctx.Config, "Config diverges for suffix %q", appTOMLSuffix) - require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), "settings diverge for suffix %q", appTOMLSuffix) + require.Equal(t, legacyCtx.Config, v2Ctx.Config, "Config diverges (case %q, suffix %q)", tc.name, appTOMLSuffix) + require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), "settings diverge (case %q, suffix %q)", tc.name, appTOMLSuffix) }) } From bf69ff030ca75922436b3ab8a13e81f5cf49f20f Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 14:04:16 -0700 Subject: [PATCH 09/34] test(configmanager): fuzz corpusIdx as uint; de-shadow replaceInFile idiomatic-reviewer nits on the fold-in: - Fuzz index is now uint, dropping the sign guard and the math.MinInt negation edge (negating MinInt stays negative -> corpus[neg] panic; a fuzzer would hit it). Validated: 15s / 4354 execs, no panic, no divergence. - replaceInFile params renamed oldStr/newStr to stop shadowing builtin new. Co-Authored-By: Claude Opus 4.8 --- .../cmd/configmanager_differential_test.go | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 5bb8b0f87f..1126d0bb05 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -133,14 +133,14 @@ func prependToFile(t *testing.T, path, s string) { require.NoError(t, os.WriteFile(path, append([]byte(s), b...), 0o600)) } -// replaceInFile replaces old with new in the file at path, asserting old was -// present so a corpus mutation can never silently become a no-op. -func replaceInFile(t *testing.T, path, old, new string) { +// replaceInFile replaces oldStr with newStr in the file at path, asserting +// oldStr was present so a corpus mutation can never silently become a no-op. +func replaceInFile(t *testing.T, path, oldStr, newStr string) { t.Helper() b, err := os.ReadFile(path) require.NoError(t, err) - require.Contains(t, string(b), old, "replace target %q not found — fixture would be vacuous", old) - require.NoError(t, os.WriteFile(path, []byte(strings.ReplaceAll(string(b), old, new)), 0o600)) + require.Contains(t, string(b), oldStr, "replace target %q not found — fixture would be vacuous", oldStr) + require.NoError(t, os.WriteFile(path, []byte(strings.ReplaceAll(string(b), oldStr, newStr)), 0o600)) } // corpusCase is one realistic on-disk config shape, applied to a freshly-seeded @@ -317,16 +317,15 @@ func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { func FuzzConfigManagerLegacyVsV2Parity(f *testing.F) { corpus := configCorpus() for i := range corpus { - f.Add(i, "") - f.Add(i, "\n# a trailing comment\n") - f.Add(i, "\nnot valid toml ][") + f.Add(uint(i), "") + f.Add(uint(i), "\n# a trailing comment\n") + f.Add(uint(i), "\nnot valid toml ][") } - f.Fuzz(func(t *testing.T, corpusIdx int, appTOMLSuffix string) { - if corpusIdx < 0 { - corpusIdx = -corpusIdx - } - tc := corpus[corpusIdx%len(corpus)] + // corpusIdx is unsigned so a fuzzed index maps to a case with a plain modulo + // — no sign guard, no math.MinInt negation edge. + f.Fuzz(func(t *testing.T, corpusIdx uint, appTOMLSuffix string) { + tc := corpus[corpusIdx%uint(len(corpus))] home := seedDefaultConfig(t) tc.mutate(t, home) From 3606e933662dd78e5fdd12e14f97c64b36464e63 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 14:50:38 -0700 Subject: [PATCH 10/34] refactor(configmanager): log advisory diagnostics via seilog, not fmt.Fprintf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Advisory validation diagnostics now go through seilog (the seid-wide logger, already used in root.go) at Warn, instead of raw fmt.Fprintf to cmd stderr. seilog is a package-global slog logger available in PersistentPreRunE — it does not depend on the node/server-context logger (wired later inside the re-entered handler) — so logging is now structured, level-controlled, and consistent with the rest of seid. Tests: the advisory paths' behavior (I2 — v2 returns the same error as legacy, boots identically) is unchanged and still asserted. seilog fixes its output sink at package init (no per-test hook), so the log-TEXT assertion is dropped — the invariant, not the log text, is what the test protects. runManager simplified to (ctx, err). Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager.go | 16 +++++--- .../cmd/configmanager_differential_test.go | 41 +++++++++---------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 0f77030b59..d6bc8104bb 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -11,11 +11,14 @@ import ( "github.com/spf13/viper" seiconfig "github.com/sei-protocol/sei-config" + "github.com/sei-protocol/seilog" "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" "github.com/sei-protocol/sei-chain/sei-cosmos/server" ) +var logger = seilog.NewLogger("cmd", "seid", "configmanager") + // EnvVar gates which configuration manager seid uses. const EnvVar = "SEI_CONFIG_MANAGER" @@ -48,27 +51,28 @@ func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) } -// validateAdvisory resolves the home dir, reads the on-disk config, and prints -// any validation diagnostics to stderr. Every step is advisory: a failure is -// logged and swallowed so the pass can never change what the node boots on. A +// validateAdvisory resolves the home dir, reads the on-disk config, and logs any +// validation diagnostics via seilog at Warn. Every step is advisory: a failure +// is logged and swallowed so the pass can never change what the node boots on. A // missing config file is normal (the legacy handler creates it) and is not // surfaced. Keeping this a distinct step from Apply is what lets the generate // path add its authoring/render step as a sibling. func validateAdvisory(cmd *cobra.Command) { home, err := resolveHomeDir(cmd) if err != nil { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not resolve home dir for validation (advisory): %v\n", err) + logger.Warn("could not resolve home dir for config validation (advisory)", "err", err) return } cfg, err := seiconfig.ReadConfigFromDir(home) if err != nil { if !errors.Is(err, os.ErrNotExist) { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: could not read config for validation (advisory): %v\n", err) + logger.Warn("could not read config for validation (advisory)", "err", err) } return } if diags := seiconfig.Validate(cfg).Diagnostics; len(diags) > 0 { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "config-manager v2: advisory validation diagnostics (not enforced; node will boot): %v\n", diags) + logger.Warn("advisory config validation diagnostics (not enforced; node will boot)", + "count", len(diags), "diagnostics", fmt.Sprintf("%v", diags)) } } diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 1126d0bb05..29c9cce3d2 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -1,9 +1,9 @@ package cmd import ( - "bytes" "context" "errors" + "io" "os" "path" "path/filepath" @@ -67,23 +67,22 @@ func runConfigManagerEnvHome(t *testing.T, mgr configmanager.ConfigManager, home // caller configures how home is supplied (flag vs env) on cmd beforehand. func execConfigManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) *server.Context { t.Helper() - ctx, _, err := runManager(t, mgr, cmd) + ctx, err := runManager(t, mgr, cmd) require.NoError(t, err) return ctx } -// runManager runs mgr.Apply inside cmd's PreRunE, capturing what an operator -// would see and do: the populated server context, whatever Apply wrote to -// stderr (advisory diagnostics), and the error Apply returned. Apply is the -// only boot-refusing call, so on the happy path it returns nil and boot is -// aborted with errStopPreRun; on a real config error it returns that error and -// runManager surfaces it unchanged. The caller sets home on cmd beforehand. -func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) (*server.Context, string, error) { +// runManager runs mgr.Apply inside cmd's PreRunE and returns the populated +// server context and the error Apply returned. Apply is the only boot-refusing +// call, so on the happy path it returns nil and boot is aborted with +// errStopPreRun; on a real config error it returns that error and runManager +// surfaces it unchanged. Advisory diagnostics go to seilog (not cmd's stderr), +// so they are not captured here — the invariants under test are the returned +// context and error, not the log text. The caller sets home on cmd beforehand. +func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) (*server.Context, error) { t.Helper() template, appCfg := initAppConfig() - - var stderr bytes.Buffer - cmd.SetErr(&stderr) + cmd.SetErr(io.Discard) // swallow cobra's own error echo; advisory logs go to seilog var applyErr error cmd.PreRunE = func(c *cobra.Command, _ []string) error { @@ -99,7 +98,7 @@ func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Comman if applyErr == nil { require.ErrorIs(t, execErr, errStopPreRun) } - return serverCtx, stderr.String(), applyErr + return serverCtx, applyErr } // appTOMLPath and cfgTOMLPath are the two files the legacy creator writes into a @@ -281,7 +280,7 @@ func TestConfigManagerLegacyVsV2Differential_Corpus(t *testing.T) { func TestConfigManagerV2AdvisoryNeverRefusesBoot(t *testing.T) { home := seedDefaultConfig(t) - v2Ctx, _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) + v2Ctx, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) require.NoError(t, v2Err, "advisory validation must never refuse boot on a valid config") legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) @@ -291,20 +290,18 @@ func TestConfigManagerV2AdvisoryNeverRefusesBoot(t *testing.T) { // TestConfigManagerV2AdvisoryReadErrorMatchesLegacy pins the other half of the // invariant: when the config is unreadable, v2 must not mask the failure or -// invent a new one. It logs an advisory read error, then re-enters the legacy -// handler and returns exactly the error legacy returns. +// invent a new one. It logs an advisory read error (via seilog), then re-enters +// the legacy handler and returns exactly the error legacy returns. func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { home := seedDefaultConfig(t) require.NoError(t, os.WriteFile(cfgTOMLPath(home), []byte("this is ] not [ valid toml"), 0o600)) - _, _, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) - _, v2Stderr, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) + _, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) + _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) require.Error(t, legacyErr, "corrupt config.toml should fail the legacy reader") require.Equal(t, legacyErr.Error(), v2Err.Error(), "v2 must return the same boot error as legacy, not mask or add one") - require.Contains(t, v2Stderr, "could not read config for validation", - "v2 should log the advisory read failure before re-entering legacy") } // FuzzConfigManagerLegacyVsV2Parity is the exhaustive form of the corpus: it @@ -331,8 +328,8 @@ func FuzzConfigManagerLegacyVsV2Parity(f *testing.F) { tc.mutate(t, home) appendToFile(t, appTOMLPath(home), appTOMLSuffix) - legacyCtx, _, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) - v2Ctx, _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) + legacyCtx, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) + v2Ctx, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) if (legacyErr == nil) != (v2Err == nil) { t.Fatalf("divergent outcome (case %q, suffix %q): legacyErr=%v v2Err=%v", tc.name, appTOMLSuffix, legacyErr, v2Err) From 8e3384bbd6a0f59d131b6e129c43bea563ecd98d Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 14:55:01 -0700 Subject: [PATCH 11/34] refactor(configmanager): keep advisory diagnostics structured in the log idiomatic-reviewer follow-ups on the seilog switch: - Log diagnostics as a []string of Diagnostic.String() instead of fmt.Sprintf("%v", diags): structured/queryable under the JSON handler and still readable under text, where a bare []Diagnostic would struct-dump. - Use the "error" attribute key to match the sibling logger calls (ethreplay.go). Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index d6bc8104bb..deca3d7e36 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -60,19 +60,23 @@ func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string func validateAdvisory(cmd *cobra.Command) { home, err := resolveHomeDir(cmd) if err != nil { - logger.Warn("could not resolve home dir for config validation (advisory)", "err", err) + logger.Warn("could not resolve home dir for config validation (advisory)", "error", err) return } cfg, err := seiconfig.ReadConfigFromDir(home) if err != nil { if !errors.Is(err, os.ErrNotExist) { - logger.Warn("could not read config for validation (advisory)", "err", err) + logger.Warn("could not read config for validation (advisory)", "error", err) } return } if diags := seiconfig.Validate(cfg).Diagnostics; len(diags) > 0 { + msgs := make([]string, len(diags)) + for i, d := range diags { + msgs[i] = d.String() + } logger.Warn("advisory config validation diagnostics (not enforced; node will boot)", - "count", len(diags), "diagnostics", fmt.Sprintf("%v", diags)) + "count", len(diags), "diagnostics", msgs) } } From 19d724db41472eb721955214bec5caf572982a39 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 15:04:20 -0700 Subject: [PATCH 12/34] fix(configmanager): recover panics in the advisory validation pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The advisory pass's contract is 'never refuses boot,' but a panic in seiconfig.ReadConfigFromDir/Validate (read fidelity still being hardened) would propagate out of Apply and crash boot — violating advisory-invariance (I2). Guard validateAdvisory with a recover that logs the panic and lets boot proceed via the re-entered legacy reader, so the pass truly cannot change the boot outcome (seidroid). Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index deca3d7e36..b5118d2318 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -52,12 +52,19 @@ func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string } // validateAdvisory resolves the home dir, reads the on-disk config, and logs any -// validation diagnostics via seilog at Warn. Every step is advisory: a failure -// is logged and swallowed so the pass can never change what the node boots on. A -// missing config file is normal (the legacy handler creates it) and is not -// surfaced. Keeping this a distinct step from Apply is what lets the generate -// path add its authoring/render step as a sibling. +// validation diagnostics via seilog at Warn. Every step is advisory: a failure — +// or a panic in the sei-config read/validate, whose fidelity is still being +// hardened — is logged and swallowed so the pass can never change what the node +// boots on. A missing config file is normal (the legacy handler creates it) and +// is not surfaced. Keeping this a distinct step from Apply is what lets the +// generate path add its authoring/render step as a sibling. func validateAdvisory(cmd *cobra.Command) { + defer func() { + if r := recover(); r != nil { + logger.Warn("config validation panicked (advisory; recovered, node will boot)", "panic", r) + } + }() + home, err := resolveHomeDir(cmd) if err != nil { logger.Warn("could not resolve home dir for config validation (advisory)", "error", err) From 16e32f48087c4b99d2538530c113a96c8fe37a7c Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 15:16:46 -0700 Subject: [PATCH 13/34] fix(configmanager): log recovered validation panic at Error, not Warn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A recovered panic is an unexpected defect (a crash in sei-config), not a benign advisory diagnostic — Error surfaces it even when SEI_LOG_LEVEL filters Warn. The message still states the node boots. (idiomatic-reviewer) Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index b5118d2318..df40eba675 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -61,7 +61,7 @@ func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string func validateAdvisory(cmd *cobra.Command) { defer func() { if r := recover(); r != nil { - logger.Warn("config validation panicked (advisory; recovered, node will boot)", "panic", r) + logger.Error("config validation panicked (advisory; recovered, node will boot)", "panic", r) } }() From 0c2b78ef7b38439a6dc158858c33c6b5b3619ed7 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 15:28:16 -0700 Subject: [PATCH 14/34] test(configmanager): cover the fresh-home ErrNotExist advisory branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit seidroid nit: validateAdvisory's silent-skip depends on sei-config wrapping a missing config as an os.ErrNotExist-satisfying error; if that regressed, every fresh-home boot would emit a spurious advisory warning. Verified io.go:26-27 wraps toml.DecodeFile (os.Open -> PathError) with %w, and pin it: - TestReadConfigFromDirMissingIsErrNotExist: asserts ReadConfigFromDir on an empty dir returns an errors.Is(os.ErrNotExist) error — fails here, not noisily in prod, if sei-config swaps to a custom not-found error. - TestConfigManagerV2FreshHomeBoots: v2 on a truly fresh home (no config yet) must boot — the only cover of the ErrNotExist branch, the common new-node case. Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager_test.go | 13 +++++++++++++ cmd/seid/cmd/configmanager_differential_test.go | 12 ++++++++++++ 2 files changed, 25 insertions(+) diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index 336e1879a0..16f191a054 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -1,11 +1,14 @@ package configmanager import ( + "os" "testing" "github.com/spf13/cobra" "github.com/stretchr/testify/require" + seiconfig "github.com/sei-protocol/sei-config" + "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" ) @@ -52,3 +55,13 @@ func TestResolveHomeDir_Flag(t *testing.T) { require.NoError(t, err) require.Equal(t, "/tmp/seid-test-home", got) } + +// TestReadConfigFromDirMissingIsErrNotExist pins the contract validateAdvisory's +// silent-skip depends on: a missing config file must yield an error that +// errors.Is(os.ErrNotExist) recognizes, so a fresh-home boot skips the advisory +// read quietly instead of logging a spurious warning. If sei-config ever swaps +// to a custom not-found error, this fails here rather than going noisy in prod. +func TestReadConfigFromDirMissingIsErrNotExist(t *testing.T) { + _, err := seiconfig.ReadConfigFromDir(t.TempDir()) + require.ErrorIs(t, err, os.ErrNotExist) +} diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 29c9cce3d2..8d93e5e48a 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -288,6 +288,18 @@ func TestConfigManagerV2AdvisoryNeverRefusesBoot(t *testing.T) { require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings()) } +// TestConfigManagerV2FreshHomeBoots exercises the fresh-home first-boot path: v2's +// advisory read hits os.ErrNotExist (no config yet), silently skips, then +// re-enters the legacy handler, which creates the files. It must not refuse boot. +// Every other test pre-seeds the config, so this is the only cover of the +// ErrNotExist branch — the common case for a brand-new node. +func TestConfigManagerV2FreshHomeBoots(t *testing.T) { + home := t.TempDir() + v2Ctx, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) + require.NoError(t, v2Err, "v2 on a fresh home must not refuse boot on the missing-config read") + require.NotNil(t, v2Ctx.Config) +} + // TestConfigManagerV2AdvisoryReadErrorMatchesLegacy pins the other half of the // invariant: when the config is unreadable, v2 must not mask the failure or // invent a new one. It logs an advisory read error (via seilog), then re-enters From 15781a24dd16ef45a0f81e4a91af2078d3771ca8 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 1 Jul 2026 15:55:20 -0700 Subject: [PATCH 15/34] test(configmanager): make quoted-scalar corpus case real, fix overclaim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude[bot] nit: the quoted-scalar case appended an unknown [sei-corpus-scalar] section, which sei-config drops (no ErrorUnused) — behaviorally identical to the unknown-section case, and its comment overclaimed that it exercises #36's lenient decode. Retarget to a real numeric field written quoted (ss-keep-recent = "100000"), so it's a genuinely distinct shape, and correct the comment: the differential only observes channel parity (independent of v2's advisory read), so coercion correctness is verified in sei-config's own #36 tests, not here. Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager_differential_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 8d93e5e48a..6777a55ceb 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -163,9 +163,13 @@ func configCorpus() []corpusCase { appendToFile(t, appTOMLPath(home), "\n[sei-corpus-unknown]\nkey = \"value\"\n") }}, {"quoted-scalar", func(t *testing.T, home string) { - // A number written as a quoted string — the sei-config lenient-decode - // case (#36). v2 reads it; the channels must still match legacy. - appendToFile(t, appTOMLPath(home), "\n[sei-corpus-scalar]\ncount = \"100000\"\n") + // A real numeric field written as a quoted string (sei-config #36's + // lenient-decode shape). Both paths re-enter the same legacy reader, so + // the channels match regardless — parity holds because v2's read is + // advisory. Coercion of quoted primitives is verified in sei-config's + // own tests; the differential only observes channel parity, not the + // advisory read's outcome. + replaceInFile(t, appTOMLPath(home), "ss-keep-recent = 100000", `ss-keep-recent = "100000"`) }}, {"cosmos-only-write-mode", func(t *testing.T, home string) { // The version-skew class: a config carrying the deprecated From a9e2bf001fdea194238cc659187c23d8e2a6c22f Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 11:39:33 -0700 Subject: [PATCH 16/34] test(config): record that an accepted gate now boots instead of refusing Merging main brings in the characterization suite, and two of its rows fail here. Both pinned the v2 body refusing to run, which was true when v2 was a stub and is what this PR changes, so the rows are updated to record the new behavior rather than skipped or widened. FuzzPreRunManagerSelection asserted that "v2" surfaces a not-implemented error. v2 now validates advisorily and re-enters the legacy reader, so an accepted value boots and populates the channels. What the row pins is the gate, so the assertion becomes acceptance versus rejection rather than which manager ran, and the differential is what proves the two produce the same channels. TestPreRunReadsTheManagerGatePerInvocation used that same refusal as its evidence that the gate is re-read per invocation. Advisory v2 boots identically to legacy, which would have left a re-read and a cached answer indistinguishable, so the second invocation now sets a value the gate rejects. The observable belongs to the gate rather than to a manager's body, which also means it survives the resolver landing. Verified by memoizing Select across invocations: the test fails, and it passes again once the cache is removed. --- cmd/seid/cmd/rootseam_config_fuzz_test.go | 39 ++++++++++++----------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/cmd/seid/cmd/rootseam_config_fuzz_test.go b/cmd/seid/cmd/rootseam_config_fuzz_test.go index 2fc316cae4..e240fdbb1e 100644 --- a/cmd/seid/cmd/rootseam_config_fuzz_test.go +++ b/cmd/seid/cmd/rootseam_config_fuzz_test.go @@ -153,10 +153,11 @@ func TestPreRunSkipsConfigForInitPrefixedSubcommands(t *testing.T) { // a typo'd value stops the command rather than quietly booting the wrong manager, // which is what makes the flag safe to leave in a deployment. // -// Because this tree ships the v2 body as a stub, "v2" surfaces as a -// not-implemented error. That distinguishes "the gate rejected the value" from "the -// gate accepted it and the manager refused", and both are asserted so the difference -// survives the v2 body landing. +// What the gate does is all this pins. The v2 body is advisory, so it validates the +// config, logs what it finds, and re-enters the legacy reader on the operator's own +// files, which means an accepted value boots exactly as legacy does. The observable +// difference is therefore acceptance versus rejection rather than which manager ran, +// and the legacy-vs-v2 differential is what proves the two produce the same channels. func FuzzPreRunManagerSelection(f *testing.F) { f.Add("") f.Add("legacy") @@ -192,12 +193,12 @@ func FuzzPreRunManagerSelection(f *testing.F) { t.Fatal("the legacy manager must populate the boot channels") } case "v2": - if err == nil { - t.Fatal("v2 is accepted by the gate but its body is a stub, so the command must " + - "fail rather than silently falling back to legacy") + if err != nil { + t.Fatalf("%s=%q selects the v2 manager, which validates advisorily and re-enters "+ + "the legacy reader, so it must boot, got %v", configmanager.EnvVar, value, err) } - if !strings.Contains(err.Error(), configmanager.EnvVar) { - t.Fatalf("the v2 failure must name the gate, got %v", err) + if serverCtx.Viper == nil { + t.Fatal("the v2 manager must populate the boot channels") } default: if err == nil { @@ -257,21 +258,23 @@ func TestPreRunReadsTheManagerGatePerInvocation(t *testing.T) { t.Fatal("the legacy manager must populate the boot channels") } - // Second invocation in the same process, gate now set. It must reach the v2 manager, - // which this tree ships as a stub and therefore fails. Booting instead means the gate - // was answered from the first invocation rather than re-read. - if err := os.Setenv(configmanager.EnvVar, "v2"); err != nil { + // Second invocation in the same process, gate now set to a value the gate rejects. + // The rejection is the observable, and it belongs to the gate rather than to any + // manager's body: a cached answer would boot legacy cleanly here instead of failing. + // A legal "v2" cannot serve that purpose, because the v2 body is advisory and boots + // identically to legacy, so it would leave a re-read and a cached answer + // indistinguishable from outside. + if err := os.Setenv(configmanager.EnvVar, "v3"); err != nil { t.Fatalf("set %s: %v", configmanager.EnvVar, err) } second, secondErr := runRootPreRun(t, configtest.NewHome(t), "start") if secondErr == nil { - t.Fatalf("the second invocation has %s=v2 and must select the v2 manager, whose body is a "+ - "stub and fails. It booted cleanly instead, so the gate is being cached across "+ - "invocations and one process can no longer change managers between commands", - configmanager.EnvVar) + t.Fatalf("the second invocation has %s=v3, which the gate rejects, so it must fail. It "+ + "booted cleanly instead, so the gate is being cached across invocations and one "+ + "process can no longer change managers between commands", configmanager.EnvVar) } if !strings.Contains(secondErr.Error(), configmanager.EnvVar) { - t.Fatalf("the v2 failure must name the gate, got %v", secondErr) + t.Fatalf("the rejection must name the gate, got %v", secondErr) } if second.Viper != nil { t.Fatal("a rejected invocation must leave the boot channels unpopulated") From fc0c267c2056700487b6facea768f82d08aac211 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 11:52:19 -0700 Subject: [PATCH 17/34] test(configmanager): build the equality proof on the shared harness The differential was written before the characterization harness existed, so it carried its own fixtures, its own env-key arithmetic and its own file helpers. It now runs on testutil/configtest, which is the substrate the rest of the config suite already uses, so the equality proof and the recorded contract it compares against rest on the same foundation. What changes is the substrate, not the assertions. Every property the file asserted still is: channel parity on the flag path and after the chain-id mutation, the env-home path with its non-vacuous resolution guard, all five corpus shapes with the guard that keeps a mutation from silently becoming a no-op, advisory-never-refuses, the fresh-home not-exist branch as its own test, error-string parity on an unreadable config, and the fuzz target crossing shapes with an arbitrary suffix. The suffix stays ungated on purpose, since bytes that do not form a document are what makes both managers fail the same way. Two substantive gains. Every test now calls Isolate, which the file never did: the legacy reader answers to environment variables whose prefix follows the test binary's own name, so without it a variable in a developer's shell could move a result. And the env key comes from the harness rather than being spelled inline, so a change that hardcoded the seid prefix fails here instead of passing under a binary that happens to be named seid. The verdicts stay require.Equal rather than a rendered dump. For the tmcfg struct that is the stronger comparison, since DeepEqual reaches unexported fields a dump does not. resolveHomeDir gains the two cells it lacked, the environment supplying the home and a flag outranking it, keyed through the harness. Verified by hardcoding the prefix: the env case fails naming CONFIGMANAGER_TEST_HOME, which is the trap. --- .../cmd/configmanager/configmanager_test.go | 46 +++ .../cmd/configmanager_differential_test.go | 267 +++++++++--------- 2 files changed, 186 insertions(+), 127 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index 16f191a054..826c708c47 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -10,6 +10,7 @@ import ( seiconfig "github.com/sei-protocol/sei-config" "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" + "github.com/sei-protocol/sei-chain/testutil/configtest" ) // TestSelect covers the dispatch table: unset and "legacy" select the @@ -56,6 +57,51 @@ func TestResolveHomeDir_Flag(t *testing.T) { require.Equal(t, "/tmp/seid-test-home", got) } +// TestResolveHomeDirEnvAndPrecedence covers the two cells the flag case above does +// not: the home arriving through the environment, and an explicit flag outranking it. +// +// The key is derived from the running binary's basename rather than spelled as +// SEID_HOME, because that is how both resolveHomeDir and the legacy handler build it. +// A change that hardcoded "seid" would still satisfy a literal-key test while +// silently ceasing to answer the environment under any other binary name, and the +// test binary is never named seid, so deriving the key is what makes this real. +// +// This asserts resolveHomeDir alone. That it agrees with the home the legacy handler +// reads is the lockstep property, and it is asserted end to end against the real +// handler in TestConfigManagerLegacyVsV2Differential_EnvHome. +func TestResolveHomeDirEnvAndPrecedence(t *testing.T) { + prefix, err := configtest.ServerEnvPrefix() + require.NoError(t, err) + envKey := configtest.ServerEnvKey(prefix, flags.FlagHome) + + t.Run("the environment supplies the home", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(envKey, "/tmp/seid-env-home") + + cmd := &cobra.Command{} + cmd.Flags().String(flags.FlagHome, "", "") + + got, err := resolveHomeDir(cmd) + require.NoError(t, err) + require.Equal(t, "/tmp/seid-env-home", got, + "an unchanged flag default ranks below AutomaticEnv, so %s resolves the home", envKey) + }) + + t.Run("an explicit flag outranks the environment", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(envKey, "/tmp/seid-env-home") + + cmd := &cobra.Command{} + cmd.Flags().String(flags.FlagHome, "", "") + require.NoError(t, cmd.Flags().Set(flags.FlagHome, "/tmp/seid-flag-home")) + + got, err := resolveHomeDir(cmd) + require.NoError(t, err) + require.Equal(t, "/tmp/seid-flag-home", got, + "a changed flag outranks the environment in viper's precedence") + }) +} + // TestReadConfigFromDirMissingIsErrNotExist pins the contract validateAdvisory's // silent-skip depends on: a missing config file must yield an error that // errors.Is(os.ErrNotExist) recognizes, so a fresh-home boot skips the advisory diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 6777a55ceb..f4d28a0d8c 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -4,9 +4,6 @@ import ( "context" "errors" "io" - "os" - "path" - "path/filepath" "strings" "testing" @@ -17,44 +14,60 @@ import ( "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" "github.com/sei-protocol/sei-chain/sei-cosmos/server" + "github.com/sei-protocol/sei-chain/testutil/configtest" ) +// This file is the equality foundation for the two managers: everything here +// compares what legacy resolves against what v2 resolves, on one home, through the +// two channels the rest of the boot reads. +// +// It runs on the same harness as the characterization suite (testutil/configtest), +// which is what makes the comparison trustworthy. Isolate pins the process +// environment, since the legacy reader answers to environment variables whose prefix +// follows the test binary's own name, and NewHome gives a node directory the test +// controls byte for byte, since the legacy reader writes into a home while reading it. +// See testutil/configtest/AGENTS.md. +// +// The verdicts stay require.Equal rather than the harness dump helpers. For the +// *tmcfg.Config struct that is a stronger comparison, because reflect.DeepEqual +// reaches unexported fields a rendered dump does not, and a dump is the right +// instrument for a recorded golden rather than for a relative legacy-versus-v2 +// equality. + // errStopPreRun aborts the command after the config-resolution PreRunE, before // StartCmd's RunE tries to boot a node. var errStopPreRun = errors.New("stop after prerun") -// runConfigManager runs mgr.Apply inside a StartCmd's PreRunE against homeDir -// supplied via the --home flag, using seid's real app-config template, and -// returns the populated server context (the two channels start.go/app.New -// consume). It mirrors the harness in sei-cosmos/server/util_test.go. -func runConfigManager(t *testing.T, mgr configmanager.ConfigManager, homeDir string) *server.Context { +// runConfigManager runs mgr.Apply inside a StartCmd's PreRunE against home supplied +// via the --home flag, using seid's real app-config template, and returns the +// populated server context (the two channels start.go/app.New consume). +func runConfigManager(t *testing.T, mgr configmanager.ConfigManager, home *configtest.Home) *server.Context { t.Helper() - return execConfigManager(t, mgr, startCmdForHome(t, homeDir)) + return execConfigManager(t, mgr, startCmdForHome(t, home)) } -// startCmdForHome builds a StartCmd with --home set to homeDir. -func startCmdForHome(t *testing.T, homeDir string) *cobra.Command { +// startCmdForHome builds a StartCmd with --home set to the fixture home. +func startCmdForHome(t *testing.T, home *configtest.Home) *cobra.Command { t.Helper() cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) - require.NoError(t, cmd.Flags().Set(flags.FlagHome, homeDir)) + require.NoError(t, cmd.Flags().Set(flags.FlagHome, home.Root)) return cmd } -// runConfigManagerEnvHome is runConfigManager's twin that supplies homeDir -// through the environment instead of --home, exercising the -// SetEnvPrefix/AutomaticEnv/replacer machinery that resolveHomeDir mirrors from -// the legacy handler (the flag-driven path never touches it). The env prefix is -// path.Base(os.Executable()) — the test binary — derived identically by BOTH -// the legacy handler (sei-cosmos/server/util.go:152,162-164) and v2's -// resolveHomeDir, so both resolve the same home. viper's lookup key for "home" -// is ToUpper(prefix + "_" + "home") with the ".","-" -> "_" replacer applied. -func runConfigManagerEnvHome(t *testing.T, mgr configmanager.ConfigManager, homeDir string) *server.Context { +// runConfigManagerEnvHome is runConfigManager's twin that supplies the home through +// the environment instead of --home, exercising the SetEnvPrefix/AutomaticEnv +// machinery that v2's resolveHomeDir mirrors from the legacy handler. The flag-driven +// path never touches it. +// +// The key comes from the harness rather than being spelled here, because the prefix +// is the running binary's basename and both the legacy handler and resolveHomeDir +// derive it that way. Hardcoding "seid" would pass under a binary named seid and say +// nothing about the resolution actually under test. +func runConfigManagerEnvHome(t *testing.T, mgr configmanager.ConfigManager, home *configtest.Home) *server.Context { t.Helper() - exe, err := os.Executable() + prefix, err := configtest.ServerEnvPrefix() require.NoError(t, err) - envKey := strings.NewReplacer(".", "_", "-", "_").Replace( - strings.ToUpper(path.Base(exe) + "_" + flags.FlagHome)) - t.Setenv(envKey, homeDir) + t.Setenv(configtest.ServerEnvKey(prefix, flags.FlagHome), home.Root) // Leave --home unset: an unchanged flag default ranks below AutomaticEnv in // viper's precedence, so the env value is what resolves. @@ -63,8 +76,8 @@ func runConfigManagerEnvHome(t *testing.T, mgr configmanager.ConfigManager, home } // execConfigManager runs mgr.Apply on the happy path (Apply succeeds; boot is -// aborted with errStopPreRun) and returns the populated server context. The -// caller configures how home is supplied (flag vs env) on cmd beforehand. +// aborted with errStopPreRun) and returns the populated server context. The caller +// configures how home is supplied (flag vs env) on cmd beforehand. func execConfigManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) *server.Context { t.Helper() ctx, err := runManager(t, mgr, cmd) @@ -72,13 +85,12 @@ func execConfigManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra return ctx } -// runManager runs mgr.Apply inside cmd's PreRunE and returns the populated -// server context and the error Apply returned. Apply is the only boot-refusing -// call, so on the happy path it returns nil and boot is aborted with -// errStopPreRun; on a real config error it returns that error and runManager -// surfaces it unchanged. Advisory diagnostics go to seilog (not cmd's stderr), -// so they are not captured here — the invariants under test are the returned -// context and error, not the log text. The caller sets home on cmd beforehand. +// runManager runs mgr.Apply inside cmd's PreRunE and returns the populated server +// context and the error Apply returned. Apply is the only boot-refusing call, so on +// the happy path it returns nil and boot is aborted with errStopPreRun; on a real +// config error it returns that error and runManager surfaces it unchanged. Advisory +// diagnostics go to seilog (not cmd's stderr), so they are not captured here — the +// invariants under test are the returned context and error, not the log text. func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) (*server.Context, error) { t.Helper() template, appCfg := initAppConfig() @@ -101,45 +113,40 @@ func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Comman return serverCtx, applyErr } -// appTOMLPath and cfgTOMLPath are the two files the legacy creator writes into a -// home and both managers then read. -func appTOMLPath(home string) string { return filepath.Join(home, "config", "app.toml") } -func cfgTOMLPath(home string) string { return filepath.Join(home, "config", "config.toml") } - -// seedDefaultConfig generates a complete, realistic config (all Sei sections) by -// letting the legacy creator write into a fresh home, and returns that home. -func seedDefaultConfig(t *testing.T) string { +// seedDefaultConfig returns a home carrying a complete, realistic config (all Sei +// sections), generated by letting the legacy creator write into a fresh home. +func seedDefaultConfig(t *testing.T) *configtest.Home { t.Helper() - home := t.TempDir() + home := configtest.NewHome(t) _ = runConfigManager(t, configmanager.LegacyConfigManager{}, home) return home } -// appendToFile appends s to the file at path (both managers must still agree on -// the result). -func appendToFile(t *testing.T, path, s string) { +// appendToAppTOML appends s to the home's app.toml. Every corpus mutation targets +// app.toml, so these three helpers name that rather than taking a path. +func appendToAppTOML(t *testing.T, home *configtest.Home, s string) { t.Helper() - b, err := os.ReadFile(path) - require.NoError(t, err) - require.NoError(t, os.WriteFile(path, append(b, []byte(s)...), 0o600)) + b, ok := home.Read(t, "app.toml") + require.True(t, ok, "app.toml must exist before it is appended to") + home.WriteAppTOML(t, append(b, []byte(s)...)) } -// prependToFile prepends s to the file at path. -func prependToFile(t *testing.T, path, s string) { +// prependToAppTOML prepends s to the home's app.toml. +func prependToAppTOML(t *testing.T, home *configtest.Home, s string) { t.Helper() - b, err := os.ReadFile(path) - require.NoError(t, err) - require.NoError(t, os.WriteFile(path, append([]byte(s), b...), 0o600)) + b, ok := home.Read(t, "app.toml") + require.True(t, ok, "app.toml must exist before it is prepended to") + home.WriteAppTOML(t, append([]byte(s), b...)) } -// replaceInFile replaces oldStr with newStr in the file at path, asserting +// replaceInAppTOML replaces oldStr with newStr in the home's app.toml, asserting // oldStr was present so a corpus mutation can never silently become a no-op. -func replaceInFile(t *testing.T, path, oldStr, newStr string) { +func replaceInAppTOML(t *testing.T, home *configtest.Home, oldStr, newStr string) { t.Helper() - b, err := os.ReadFile(path) - require.NoError(t, err) + b, ok := home.Read(t, "app.toml") + require.True(t, ok, "app.toml must exist before it is edited") require.Contains(t, string(b), oldStr, "replace target %q not found — fixture would be vacuous", oldStr) - require.NoError(t, os.WriteFile(path, []byte(strings.ReplaceAll(string(b), oldStr, newStr)), 0o600)) + home.WriteAppTOML(t, []byte(strings.ReplaceAll(string(b), oldStr, newStr))) } // corpusCase is one realistic on-disk config shape, applied to a freshly-seeded @@ -147,63 +154,57 @@ func replaceInFile(t *testing.T, path, oldStr, newStr string) { // fuzz target consume, so the set of "interesting shapes" lives in one place. type corpusCase struct { name string - mutate func(t *testing.T, home string) + mutate func(t *testing.T, home *configtest.Home) } -// configCorpus is the single source of the config shapes the parity proof runs -// over. Each case mutates a default home in place; parity must hold for all of -// them because v2 re-enters the legacy reader regardless of what it read. +// configCorpus is the single source of the config shapes the parity proof runs over. +// Each case mutates a default home in place; parity must hold for all of them because +// v2 re-enters the legacy reader regardless of what it read. func configCorpus() []corpusCase { return []corpusCase{ - {"default", func(t *testing.T, home string) {}}, - {"leading-comments-and-blanks", func(t *testing.T, home string) { - prependToFile(t, appTOMLPath(home), "# corpus: a leading comment\n\n") + {"default", func(t *testing.T, home *configtest.Home) {}}, + {"leading-comments-and-blanks", func(t *testing.T, home *configtest.Home) { + prependToAppTOML(t, home, "# corpus: a leading comment\n\n") }}, - {"unknown-section", func(t *testing.T, home string) { - appendToFile(t, appTOMLPath(home), "\n[sei-corpus-unknown]\nkey = \"value\"\n") + {"unknown-section", func(t *testing.T, home *configtest.Home) { + appendToAppTOML(t, home, "\n[sei-corpus-unknown]\nkey = \"value\"\n") }}, - {"quoted-scalar", func(t *testing.T, home string) { + {"quoted-scalar", func(t *testing.T, home *configtest.Home) { // A real numeric field written as a quoted string (sei-config #36's // lenient-decode shape). Both paths re-enter the same legacy reader, so // the channels match regardless — parity holds because v2's read is // advisory. Coercion of quoted primitives is verified in sei-config's // own tests; the differential only observes channel parity, not the // advisory read's outcome. - replaceInFile(t, appTOMLPath(home), "ss-keep-recent = 100000", `ss-keep-recent = "100000"`) + replaceInAppTOML(t, home, "ss-keep-recent = 100000", `ss-keep-recent = "100000"`) }}, - {"cosmos-only-write-mode", func(t *testing.T, home string) { + {"cosmos-only-write-mode", func(t *testing.T, home *configtest.Home) { // The version-skew class: a config carrying the deprecated // state-commit.sc-write-mode "cosmos_only". sei-config still accepts it // as valid, so v2 raises no diagnostic today; the point here is that // both managers read it identically (parity). It becomes a *caught* // case only once fatal validation + a sei-config deprecation rule land. - replaceInFile(t, appTOMLPath(home), `sc-write-mode = "memiavl_only"`, `sc-write-mode = "cosmos_only"`) + replaceInAppTOML(t, home, `sc-write-mode = "memiavl_only"`, `sc-write-mode = "cosmos_only"`) }}, } } // TestConfigManagerLegacyVsV2Differential is the core safety property: the v2 -// manager must produce the SAME consumed config as the legacy path. v2 reads -// the config (to validate it) and then re-enters the legacy reader on the -// operator's ORIGINAL files — it does not rewrite — so the two paths read the -// SAME home and any difference is a real divergence, not a path artifact. +// manager must produce the SAME consumed config as the legacy path. v2 reads the +// config (to validate it) and then re-enters the legacy reader on the operator's +// ORIGINAL files — it does not rewrite — so the two paths read the SAME home and any +// difference is a real divergence, not a path artifact. // // It compares parsed semantics: // - serverCtx.Config (the *tmcfg.Config the node runs on), and // - serverCtx.Viper.AllSettings() (the AppOptions every Sei section reads via // appOpts.Get), both at end-of-PersistentPreRunE and after the start.go // chain-id mutation. -// -// A realistic fixture (all 11 Sei sections) is generated by letting the legacy -// handler create the full default config once; both managers then read it. func TestConfigManagerLegacyVsV2Differential(t *testing.T) { - home := t.TempDir() - - // Generate a complete, realistic config via the legacy creator (fresh home). - _ = runConfigManager(t, configmanager.LegacyConfigManager{}, home) + configtest.Isolate(t) - // Both managers read the same populated home. v2 is passthrough (no rewrite), - // so RootDir and every other path-derived field match by construction. + // Both managers read the same home, carrying a complete realistic config. + home := seedDefaultConfig(t) legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) v2Ctx := runConfigManager(t, configmanager.SeiConfigManager{}, home) @@ -221,18 +222,19 @@ func TestConfigManagerLegacyVsV2Differential(t *testing.T) { "settings diverge after the start.go chain-id mutation") } -// TestConfigManagerLegacyVsV2Differential_EnvHome exercises the env-precedence -// half of resolveHomeDir's mirror of the legacy handler — the flag-driven -// differential above never touches SetEnvPrefix/AutomaticEnv. When home is -// supplied via the environment (not --home), v2 must resolve the SAME home the -// legacy handler does; otherwise v2 would advisorily validate one dir while the -// re-entered legacy reader boots on another — a silent drift the advisory -// design cannot surface (no error, no diagnostic). This pins the seam so a -// future change to the legacy env-resolution can't diverge undetected. +// TestConfigManagerLegacyVsV2Differential_EnvHome exercises the env-precedence half +// of resolveHomeDir's mirror of the legacy handler — the flag-driven differential +// above never touches SetEnvPrefix/AutomaticEnv. When home is supplied via the +// environment (not --home), v2 must resolve the SAME home the legacy handler does; +// otherwise v2 would advisorily validate one dir while the re-entered legacy reader +// boots on another — a silent drift the advisory design cannot surface (no error, no +// diagnostic). This pins the seam so a future change to the legacy env-resolution +// can't diverge undetected. func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { - home := t.TempDir() + configtest.Isolate(t) + home := configtest.NewHome(t) - // Populate a complete realistic config in `home` via the fresh-home legacy + // Populate a complete realistic config in home via the fresh-home legacy // creator, driven entirely through the env var (no --home). _ = runConfigManagerEnvHome(t, configmanager.LegacyConfigManager{}, home) @@ -240,12 +242,12 @@ func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { v2Ctx := runConfigManagerEnvHome(t, configmanager.SeiConfigManager{}, home) // Non-vacuous guard: the env var actually drove resolution. If the key were - // wrong, both would fall back to StartCmd's "/foobar" default (and the - // legacy creator would fail writing under it) — this asserts the env path - // resolved to the temp home, for both managers. - require.Equal(t, home, v2Ctx.Viper.GetString(flags.FlagHome), + // wrong, both would fall back to StartCmd's "/foobar" default (and the legacy + // creator would fail writing under it) — this asserts the env path resolved to + // the fixture home, for both managers. + require.Equal(t, home.Root, v2Ctx.Viper.GetString(flags.FlagHome), "env-provided home did not drive v2 resolution") - require.Equal(t, home, legacyCtx.Viper.GetString(flags.FlagHome), + require.Equal(t, home.Root, legacyCtx.Viper.GetString(flags.FlagHome), "env-provided home did not drive legacy resolution") require.Equal(t, legacyCtx.Config, v2Ctx.Config, @@ -254,15 +256,16 @@ func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { "serverCtx.Viper settings differ between legacy and v2 on the env-home path") } -// TestConfigManagerLegacyVsV2Differential_Corpus widens the parity proof from -// the single default fixture to a corpus of realistic on-disk shapes. Parity is -// by construction (v2 re-enters the legacy reader), so any shape an operator -// could present must produce identical channels — including shapes that exercise -// sei-config's own reader (quoted scalars, unknown keys), whose advisory read -// still must not perturb what the node boots on. +// TestConfigManagerLegacyVsV2Differential_Corpus widens the parity proof from the +// single default fixture to a corpus of realistic on-disk shapes. Parity is by +// construction (v2 re-enters the legacy reader), so any shape an operator could +// present must produce identical channels — including shapes that exercise +// sei-config's own reader (quoted scalars, unknown keys), whose advisory read still +// must not perturb what the node boots on. func TestConfigManagerLegacyVsV2Differential_Corpus(t *testing.T) { for _, tc := range configCorpus() { t.Run(tc.name, func(t *testing.T) { + configtest.Isolate(t) home := seedDefaultConfig(t) tc.mutate(t, home) @@ -277,11 +280,12 @@ func TestConfigManagerLegacyVsV2Differential_Corpus(t *testing.T) { } } -// TestConfigManagerV2AdvisoryNeverRefusesBoot pins the advisory invariant: on a -// valid config, v2 boots exactly as legacy does (Apply returns nil, both -// channels match), regardless of any diagnostics it prints. v2 adds -// observability, never a new boot outcome. +// TestConfigManagerV2AdvisoryNeverRefusesBoot pins the advisory invariant: on a valid +// config, v2 boots exactly as legacy does (Apply returns nil, both channels match), +// regardless of any diagnostics it prints. v2 adds observability, never a new boot +// outcome. func TestConfigManagerV2AdvisoryNeverRefusesBoot(t *testing.T) { + configtest.Isolate(t) home := seedDefaultConfig(t) v2Ctx, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) @@ -293,24 +297,27 @@ func TestConfigManagerV2AdvisoryNeverRefusesBoot(t *testing.T) { } // TestConfigManagerV2FreshHomeBoots exercises the fresh-home first-boot path: v2's -// advisory read hits os.ErrNotExist (no config yet), silently skips, then -// re-enters the legacy handler, which creates the files. It must not refuse boot. -// Every other test pre-seeds the config, so this is the only cover of the -// ErrNotExist branch — the common case for a brand-new node. +// advisory read hits os.ErrNotExist (no config yet), silently skips, then re-enters +// the legacy handler, which creates the files. It must not refuse boot. Every other +// test pre-seeds the config, so this is the only cover of the ErrNotExist branch — +// the common case for a brand-new node, and the home here stays deliberately unseeded. func TestConfigManagerV2FreshHomeBoots(t *testing.T) { - home := t.TempDir() + configtest.Isolate(t) + home := configtest.NewHome(t) + v2Ctx, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) require.NoError(t, v2Err, "v2 on a fresh home must not refuse boot on the missing-config read") require.NotNil(t, v2Ctx.Config) } // TestConfigManagerV2AdvisoryReadErrorMatchesLegacy pins the other half of the -// invariant: when the config is unreadable, v2 must not mask the failure or -// invent a new one. It logs an advisory read error (via seilog), then re-enters -// the legacy handler and returns exactly the error legacy returns. +// invariant: when the config is unreadable, v2 must not mask the failure or invent a +// new one. It logs an advisory read error (via seilog), then re-enters the legacy +// handler and returns exactly the error legacy returns. func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { + configtest.Isolate(t) home := seedDefaultConfig(t) - require.NoError(t, os.WriteFile(cfgTOMLPath(home), []byte("this is ] not [ valid toml"), 0o600)) + home.WriteConfigTOML(t, []byte("this is ] not [ valid toml")) _, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) @@ -320,13 +327,18 @@ func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { "v2 must return the same boot error as legacy, not mask or add one") } -// FuzzConfigManagerLegacyVsV2Parity is the exhaustive form of the corpus: it -// crosses every corpus shape with an arbitrary appended app.toml suffix, and -// asserts legacy and v2 reach the same outcome — identical channels when both -// succeed, the identical error when both fail. Parity is by construction, so the -// fuzzer should never find a divergence. Under `go test` (no -fuzz) it runs the -// seed corpus (each shape × a few suffixes), a deterministic differential in CI; -// under -fuzz it explores suffixes against every shape. +// FuzzConfigManagerLegacyVsV2Parity is the exhaustive form of the corpus: it crosses +// every corpus shape with an arbitrary appended app.toml suffix, and asserts legacy +// and v2 reach the same outcome — identical channels when both succeed, the identical +// error when both fail. Parity is by construction, so the fuzzer should never find a +// divergence. Under `go test` (no -fuzz) it runs the seed corpus (each shape × a few +// suffixes), a deterministic differential in CI; under -fuzz it explores suffixes +// against every shape. +// +// The suffix is deliberately ungated: feeding bytes that do not form a valid TOML +// document is the point, since the property is that both managers fail the same way. +// The harness's writability guards belong to targets that build a document from a +// fuzzed value, not to one appending arbitrary bytes on purpose. func FuzzConfigManagerLegacyVsV2Parity(f *testing.F) { corpus := configCorpus() for i := range corpus { @@ -338,11 +350,12 @@ func FuzzConfigManagerLegacyVsV2Parity(f *testing.F) { // corpusIdx is unsigned so a fuzzed index maps to a case with a plain modulo // — no sign guard, no math.MinInt negation edge. f.Fuzz(func(t *testing.T, corpusIdx uint, appTOMLSuffix string) { + configtest.Isolate(t) tc := corpus[corpusIdx%uint(len(corpus))] home := seedDefaultConfig(t) tc.mutate(t, home) - appendToFile(t, appTOMLPath(home), appTOMLSuffix) + appendToAppTOML(t, home, appTOMLSuffix) legacyCtx, legacyErr := runManager(t, configmanager.LegacyConfigManager{}, startCmdForHome(t, home)) v2Ctx, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) From 3de8a132fdeb64e72b8739a8830afd49ecf57273 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 12:10:55 -0700 Subject: [PATCH 18/34] test(configmanager): assert parity on env-carried keys the settings map cannot see Every comparison in this file read AllSettings, which enumerates only what viper knows structurally: the files it read, its defaults, overrides and bound flags. A value carried solely by the environment for a key absent from app.toml has no enumerable existence, so it appears in neither AllSettings nor AllKeys and the comparison is blind to it. It is not invisible to the node, because app.New reads through appOpts.Get and AutomaticEnv resolves at Get time, so the value does reach running code. Measured on the legacy path: Get returns the environment's value for a key that AllKeys does not list. That is the class the new manager most affects, since it is the environment resolution being replaced, so it is the one the proof could least afford to miss. The new target asserts Get rather than the enumerable map, over keys absent from every file. Two guards make it a real assertion rather than agreement about nothing. The probe asserts the value actually arrived on the legacy side, so a key resolving to nil on both sides fails instead of passing. And an empty value is declined, because viper cannot tell an empty environment variable from an unset one, which the first guard caught while this was being written. Calibrated against the passthrough, where parity is structural and every row is green by construction, then checked by making v2 stop answering one env-carried key without creating an enumerable one: the new target fails and every existing comparison still passes. --- .../cmd/configmanager_differential_test.go | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index f4d28a0d8c..08f1c2302c 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -327,6 +327,72 @@ func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { "v2 must return the same boot error as legacy, not mask or add one") } +// FuzzConfigManagerEnvOnlyKeyParity closes the one class the AllSettings comparison +// cannot reach. +// +// AllSettings enumerates only what viper knows structurally, from the files it read, +// its defaults, overrides and bound flags. A value carried solely by the environment +// for a key absent from app.toml has no enumerable existence, so it appears in neither +// AllSettings nor AllKeys, and every comparison above is blind to it. It is not +// invisible to the node: app.New reads through appOpts.Get, and AutomaticEnv resolves +// at Get time, so such a value does reach running code. +// +// That makes this the sharpest row in the file today, because environment resolution +// is exactly what the new manager changes. The assertion is therefore on Get rather +// than on the enumerable map, and it carries a non-vacuity guard: the probe asserts +// the value actually arrived, so a key that silently resolves to nil on both sides +// fails here instead of passing as agreement about nothing. +func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { + // Keys absent from a generated app.toml, so the environment is their only carrier. + f.Add("sei-differential.absent-key", "from-env") + f.Add("state-store.ss-absent-probe", "17") + f.Add("evm.absent-probe", "true") + f.Add("sei-differential.dotted.deeper-probe", "nested") + + f.Fuzz(func(t *testing.T, key, value string) { + // A key has to survive the round trip into an environment variable name and + // back to be a probe at all. Anything else is out of scope rather than a + // finding, the same move the harness makes for values it cannot set. + if key == "" || !configtest.EnvValueIsSettable(key) || !configtest.EnvValueIsSettable(value) { + return + } + // An empty value is not a probe. viper's AutomaticEnv cannot tell an empty + // environment variable from an unset one, so Get returns nil either way and + // there is nothing for the parity assertion to compare. + if value == "" { + return + } + configtest.Isolate(t) + prefix, err := configtest.ServerEnvPrefix() + require.NoError(t, err) + envKey := configtest.ServerEnvKey(prefix, key) + if envKey == prefix+"_" || strings.ContainsAny(envKey, "= ") { + return // not a name the environment can carry + } + t.Setenv(envKey, value) + + home := seedDefaultConfig(t) + legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) + v2Ctx := runConfigManager(t, configmanager.SeiConfigManager{}, home) + + legacyGot := legacyCtx.Viper.Get(key) + v2Got := v2Ctx.Viper.Get(key) + + // Non-vacuity: the probe must actually be live on the legacy side, or the + // parity assertion below is two nils agreeing. AutomaticEnv resolves at Get + // time, so an env-carried value reaches Get even with the key absent from + // every file, which is the property that makes this row worth having. + require.Equal(t, value, legacyGot, + "the probe is not live: %s did not reach Get(%q) on the legacy path, so the "+ + "parity assertion below would compare nothing", envKey, key) + + require.Equal(t, legacyGot, v2Got, + "env-only key %q resolves differently between legacy and v2 (env %s=%q). This is "+ + "invisible to the AllSettings comparison, and app.New reads it through "+ + "appOpts.Get, so it reaches the running node", key, envKey, value) + }) +} + // FuzzConfigManagerLegacyVsV2Parity is the exhaustive form of the corpus: it crosses // every corpus shape with an arbitrary appended app.toml suffix, and asserts legacy // and v2 reach the same outcome — identical channels when both succeed, the identical From 199101a2aa93d7498f0849f382c30486b35fc40b Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 12:20:59 -0700 Subject: [PATCH 19/34] fix(configmanager): make the advisory pass observable, and guard three gaps Review follow-ups. The largest one is that the validation pass reported nothing, so nothing could observe whether it ran. Channel parity and never-refuses-boot both hold just as well when the read always fails or validation has quietly become a no-op, which made the pass the one claim in this PR the suite did not back. It now returns what it saw and Apply does the logging, with a test that points it at a config missing a required field and asserts it names that field. That test fails if a future sei-config bump degrades the pass, where every other assertion would stay green. An unresolved home is now declined instead of read. resolveHomeDir returns "" with no error when --home is absent, and a read rooted at "" resolves ./config relative to the process working directory, so the pass could have validated an unrelated node's files and reported diagnostics about a config this node is not booting on. The legacy reader treats an empty home the same way, so this is not a parity break, but a pass that exists to inform an operator has to decline rather than guess. A recovered panic now carries its stack, since sei-config's read and validate fidelity is still being hardened and the value alone gives no origin. The rendered diagnostic list is capped in the log line, with the count kept whole, so a config broken in many fields cannot produce one unbounded line. Two test gaps closed. The unreadable-config row dereferenced v2's error without first asserting it was non-nil, so a regression that made v2 swallow the legacy failure would have panicked instead of reporting the broken invariant. And nothing asserted that v2 writes nothing at boot, which is a headline guarantee: a v2-side write that leaves the resolved channels alone is invisible to every comparison in the file. The config directory is now snapshotted by content hash around the v2 run. On the dependency bumps, go-viper/mapstructure 2.4.0 to 2.5.0 sits on viper's decode path and so affects the legacy read for every node regardless of the gate, which is wider than this PR's stated scope. Verified against the characterization suite: all 23 packages carrying config manifests or recorded defaults pass under -race, goldens included, so the bump is behavior-preserving for everything pinned. --- cmd/seid/cmd/configmanager/configmanager.go | 107 ++++++++++++++---- .../cmd/configmanager/configmanager_test.go | 78 +++++++++++++ .../cmd/configmanager_differential_test.go | 53 +++++++++ 3 files changed, 217 insertions(+), 21 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index df40eba675..0104e8d81e 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path" + "runtime/debug" "strings" "github.com/spf13/cobra" @@ -47,44 +48,108 @@ type SeiConfigManager struct{} // Apply runs the advisory validation pass, then re-enters the legacy handler on // the operator's original files. Nothing in the validation pass refuses boot. func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { - validateAdvisory(cmd) + logAdvisory(validateAdvisory(cmd)) return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) } -// validateAdvisory resolves the home dir, reads the on-disk config, and logs any -// validation diagnostics via seilog at Warn. Every step is advisory: a failure — -// or a panic in the sei-config read/validate, whose fidelity is still being -// hardened — is logged and swallowed so the pass can never change what the node -// boots on. A missing config file is normal (the legacy handler creates it) and -// is not surfaced. Keeping this a distinct step from Apply is what lets the -// generate path add its authoring/render step as a sibling. -func validateAdvisory(cmd *cobra.Command) { +// advisoryOutcome is what the validation pass saw. The pass reports rather than logs +// so that it is observable, which nothing else here can be: a channel-parity or +// never-refuses-boot assertion holds just as well when the read always fails or the +// validation has quietly become a no-op, so something has to be able to see that the +// pass ran and what it found. Apply does the logging. +type advisoryOutcome struct { + // Home is the directory the pass read, empty when it never got that far. + Home string + // Stage names where the pass stopped, "resolve" or "read", and is empty when it + // completed. It is what keeps the two failures separately reportable. + Stage string + // Skipped records that there was nothing to validate: no home resolved, or no + // config on disk yet, which is the normal case on a fresh node. + Skipped bool + // Diagnostics are the rendered validation findings. + Diagnostics []string + // Err is a resolve or read failure, advisory like everything else here. + Err error + // Panic is a recovered panic value and Stack its origin. sei-config's read and + // validate fidelity is still being hardened, so a panic here is the case most + // likely to need debugging from a node's logs, and the value alone gives no origin. + Panic any + Stack []byte +} + +// validateAdvisory resolves the home dir, reads the on-disk config and validates it, +// reporting what it saw. Every outcome is advisory: a failure, or a panic in the +// sei-config read or validate, is captured and returned rather than propagated, so +// the pass can never change what the node boots on. Keeping this a distinct step from +// Apply is what lets the generate path add its authoring/render step as a sibling. +func validateAdvisory(cmd *cobra.Command) (out advisoryOutcome) { defer func() { if r := recover(); r != nil { - logger.Error("config validation panicked (advisory; recovered, node will boot)", "panic", r) + out.Panic, out.Stack = r, debug.Stack() } }() home, err := resolveHomeDir(cmd) if err != nil { - logger.Warn("could not resolve home dir for config validation (advisory)", "error", err) - return + out.Stage, out.Err = "resolve", err + return out + } + // An unresolved home would send the read at ./config relative to the process + // working directory, so it could validate some unrelated node's files and report + // diagnostics that have nothing to do with what this node boots on. The legacy + // reader treats an empty home the same way, so this is not a parity break, but a + // pass whose whole purpose is operator-facing diagnostics has to decline instead. + if home == "" { + out.Skipped = true + return out } + out.Home = home + cfg, err := seiconfig.ReadConfigFromDir(home) if err != nil { - if !errors.Is(err, os.ErrNotExist) { - logger.Warn("could not read config for validation (advisory)", "error", err) + // A missing config is the normal fresh-node case: the legacy handler creates it. + if errors.Is(err, os.ErrNotExist) { + out.Skipped = true + return out } + out.Stage, out.Err = "read", err + return out + } + + for _, d := range seiconfig.Validate(cfg).Diagnostics { + out.Diagnostics = append(out.Diagnostics, d.String()) + } + return out +} + +// maxLoggedDiagnostics bounds the rendered list in one log line. A badly broken +// config can produce a diagnostic per field, and count is what an operator alerts +// on, so the full set is left to be re-derived from the file rather than emitted as +// one unbounded line. +const maxLoggedDiagnostics = 10 + +// logAdvisory reports an outcome through seilog. Nothing here refuses boot. +func logAdvisory(out advisoryOutcome) { + switch { + case out.Panic != nil: + logger.Error("config validation panicked (advisory; recovered, node will boot)", + "panic", out.Panic, "stack", string(out.Stack)) + case out.Stage == "resolve": + logger.Warn("could not resolve home dir for config validation (advisory)", "error", out.Err) + case out.Stage == "read": + logger.Warn("could not read config for validation (advisory)", "error", out.Err) + } + + if len(out.Diagnostics) == 0 { return } - if diags := seiconfig.Validate(cfg).Diagnostics; len(diags) > 0 { - msgs := make([]string, len(diags)) - for i, d := range diags { - msgs[i] = d.String() - } - logger.Warn("advisory config validation diagnostics (not enforced; node will boot)", - "count", len(diags), "diagnostics", msgs) + shown, omitted := out.Diagnostics, 0 + if len(shown) > maxLoggedDiagnostics { + omitted = len(shown) - maxLoggedDiagnostics + shown = shown[:maxLoggedDiagnostics] } + logger.Warn("advisory config validation diagnostics (not enforced; node will boot)", + "count", len(out.Diagnostics), "diagnostics", shown, "omitted", omitted) } // resolveHomeDir resolves --home the same way the legacy handler does diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index 826c708c47..dd69aa7921 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -2,6 +2,8 @@ package configmanager import ( "os" + "path/filepath" + "strings" "testing" "github.com/spf13/cobra" @@ -102,6 +104,82 @@ func TestResolveHomeDirEnvAndPrecedence(t *testing.T) { }) } +// writeMinimalHome creates a node directory carrying the two files +// ReadConfigFromDir decodes, with the given contents, and returns its root. +// +// The files are written by hand rather than generated by the legacy creator, so the +// input is exactly what the test says it is. A generated config would carry +// machine-derived values (the worker counts, the hostname moniker) and template +// literals that move between releases, and an assertion about validation should not +// depend on either. +func writeMinimalHome(t *testing.T, configTOML, appTOML string) string { + t.Helper() + root := filepath.Join(t.TempDir(), "node") + cfgDir := filepath.Join(root, "config") + require.NoError(t, os.MkdirAll(cfgDir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(cfgDir, "config.toml"), []byte(configTOML), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(cfgDir, "app.toml"), []byte(appTOML), 0o600)) + return root +} + +// homeCmd returns a command whose --home is set to root. +func homeCmd(t *testing.T, root string) *cobra.Command { + t.Helper() + cmd := &cobra.Command{} + cmd.Flags().String(flags.FlagHome, "", "") + require.NoError(t, cmd.Flags().Set(flags.FlagHome, root)) + return cmd +} + +// TestValidateAdvisoryReportsWhatItFound is the positive case for the validation +// pass, and it is the one claim the parity assertions cannot make. +// +// Channel parity and never-refuses-boot both hold just as well when the pass does +// nothing at all: if the read always failed, or validation degraded to returning no +// diagnostics after a sei-config bump, every one of those assertions would still +// pass. This one fails, because it names a finding the pass has to produce. +func TestValidateAdvisoryReportsWhatItFound(t *testing.T) { + configtest.Isolate(t) + + // app.toml carries no minimum-gas-prices, which sei-config reports against + // chain.min_gas_prices. A named field is asserted rather than a count, so an + // unrelated diagnostic appearing or disappearing does not make this pass or fail + // for the wrong reason. + root := writeMinimalHome(t, "mode = \"full\"\n", "") + + out := validateAdvisory(homeCmd(t, root)) + + require.Nil(t, out.Panic, "the pass panicked: %v\n%s", out.Panic, out.Stack) + require.NoError(t, out.Err, "the pass could not read the config it was pointed at") + require.False(t, out.Skipped, "the pass skipped a home that carries both config files") + require.Equal(t, root, out.Home, "the pass validated a different directory than it was given") + + require.NotEmpty(t, out.Diagnostics, + "validation produced no findings on a config missing a required field, so the "+ + "pass has become a no-op and every parity assertion elsewhere holds vacuously") + require.Contains(t, strings.Join(out.Diagnostics, "\n"), "chain.min_gas_prices", + "validation ran but did not report the missing required field") +} + +// TestValidateAdvisorySkipsAnUnresolvedHome pins the guard on an empty home. +// +// resolveHomeDir returns "" with no error when --home is absent, and a read rooted at +// "" resolves ./config relative to the process working directory. That would validate +// whatever node happens to live there and report diagnostics about a config this node +// is not booting on, which is worse than reporting nothing from a pass that exists to +// inform an operator. +func TestValidateAdvisorySkipsAnUnresolvedHome(t *testing.T) { + configtest.Isolate(t) + + out := validateAdvisory(&cobra.Command{}) // no --home registered at all + + require.True(t, out.Skipped, "an unresolved home must be declined, not read") + require.Empty(t, out.Home, "nothing may be recorded as validated") + require.Empty(t, out.Diagnostics, "a declined pass must not report findings") + require.NoError(t, out.Err) + require.Nil(t, out.Panic) +} + // TestReadConfigFromDirMissingIsErrNotExist pins the contract validateAdvisory's // silent-skip depends on: a missing config file must yield an error that // errors.Is(os.ErrNotExist) recognizes, so a fresh-home boot skips the advisory diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 08f1c2302c..879652766c 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -2,8 +2,11 @@ package cmd import ( "context" + "crypto/sha256" "errors" + "fmt" "io" + "os" "strings" "testing" @@ -323,10 +326,60 @@ func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { _, v2Err := runManager(t, configmanager.SeiConfigManager{}, startCmdForHome(t, home)) require.Error(t, legacyErr, "corrupt config.toml should fail the legacy reader") + // Asserted before the strings are compared: were v2 to swallow the failure, the + // comparison below would panic on a nil error rather than report the invariant + // that was actually broken. + require.Error(t, v2Err, "v2 must not mask the legacy boot error") require.Equal(t, legacyErr.Error(), v2Err.Error(), "v2 must return the same boot error as legacy, not mask or add one") } +// snapshotConfigDir records the name, size and content hash of every file under the +// home's config directory, which is what a no-write claim has to be checked against. +func snapshotConfigDir(t *testing.T, home *configtest.Home) map[string]string { + t.Helper() + entries, err := os.ReadDir(home.ConfigDir()) + require.NoError(t, err) + + snap := make(map[string]string, len(entries)) + for _, e := range entries { + if e.IsDir() { + snap[e.Name()+"/"] = "dir" + continue + } + b, ok := home.Read(t, e.Name()) + require.True(t, ok, "listed file %q could not be read", e.Name()) + snap[e.Name()] = fmt.Sprintf("%d:%x", len(b), sha256.Sum256(b)) + } + return snap +} + +// TestConfigManagerV2WritesNothing pins the guarantee the rest of the file cannot +// see: v2 does not rewrite, migrate, or author anything at boot. +// +// Every other assertion here compares the resolved channels, so a v2-side write that +// leaves those channels alone passes all of them. Authoring a sei.toml, rewriting a +// file to nearly the same bytes, or adding anything to the config directory would all +// go unnoticed. This snapshots the directory around the v2 run and compares, so the +// prose guarantee becomes an enforced one. +// +// The home is pre-seeded on purpose. A fresh home is the one case where writes are +// expected, since the legacy handler v2 re-enters is what creates the files, and +// TestConfigManagerV2FreshHomeBoots covers that path. +func TestConfigManagerV2WritesNothing(t *testing.T) { + configtest.Isolate(t) + home := seedDefaultConfig(t) + + before := snapshotConfigDir(t, home) + _ = runConfigManager(t, configmanager.SeiConfigManager{}, home) + after := snapshotConfigDir(t, home) + + require.Equal(t, before, after, + "v2 changed the config directory. It re-enters the legacy reader on the operator's "+ + "own files and must not author, migrate or rewrite anything at boot, and a write "+ + "that leaves the resolved channels alone is invisible to every other assertion here") +} + // FuzzConfigManagerEnvOnlyKeyParity closes the one class the AllSettings comparison // cannot reach. // From 1ae265607c8880a8d430f8d7ff34d2af545573a0 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 12:26:37 -0700 Subject: [PATCH 20/34] test(configmanager): assert the lockstep property instead of claiming it The review is right that the lockstep property was asserted nowhere. Three doc comments said the env-home differential pinned it, and it does not: every assertion there reads the home out of the server context, which is the value the re-entered legacy handler resolved, so resolveHomeDir's own answer never appears. Since v2's channels are produced entirely by that handler, a drifted resolveHomeDir would only make the advisory read skip or warn, changing no channel and failing nothing. That is exactly the silent drift the file's own header names. TestResolveHomeDirAgreesWithTheLegacyHandler asserts it directly, for a home from the flag and a home from the environment: run the real handler on a real StartCmd, read the home it resolved, and compare resolveHomeDir against it, with a guard that the fixture drove the resolution so the comparison cannot be vacuous. Verified by making resolveHomeDir return a different path: the new test fails and the entire cmd-package differential still passes, which is the gap the review described. The three comments now point at it rather than overstating what they cover. The pre-handler ordering of the pass is now stated as deliberate, with its cost. What it reports on is the configuration a node operator authored, not what seid generated for itself, and the consequence is that a fresh node is first validated on its second start. Running the pass after re-entry as well would close that and is deliberately deferred, because sei-config currently reports an error against a freshly generated default config, so validating generated files today would have every new node logging an error about a file seid itself just wrote. That belongs with the decision to make validation fatal. --- cmd/seid/cmd/configmanager/configmanager.go | 13 ++++ .../cmd/configmanager/configmanager_test.go | 73 ++++++++++++++++--- .../cmd/configmanager_differential_test.go | 19 +++-- 3 files changed, 88 insertions(+), 17 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 0104e8d81e..edf2aa34b6 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -82,6 +82,19 @@ type advisoryOutcome struct { // sei-config read or validate, is captured and returned rather than propagated, so // the pass can never change what the node boots on. Keeping this a distinct step from // Apply is what lets the generate path add its authoring/render step as a sibling. +// +// It runs before the legacy handler, and that ordering is deliberate: what it reports +// on is the configuration a node operator authored, not the configuration seid just +// generated for itself. The consequence is that a brand-new node is not validated on +// its first boot, since there is nothing on disk yet and the handler writes the files +// afterwards, so the earliest a diagnostic can appear is the second start. +// +// Running it after re-entry as well would close that, and it is deliberately not done +// yet, because sei-config today reports an error against a freshly generated default +// config (the pruning read-mapping gap the design tracks). Validating generated files +// before that gap closes would mean every fresh node logging an error about a file +// seid itself had just written, which is worse than validating a boot late. This is +// worth revisiting when validation goes fatal, and the two decisions belong together. func validateAdvisory(cmd *cobra.Command) (out advisoryOutcome) { defer func() { if r := recover(); r != nil { diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index dd69aa7921..ca21adea22 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -1,6 +1,7 @@ package configmanager import ( + "context" "os" "path/filepath" "strings" @@ -8,10 +9,13 @@ import ( "github.com/spf13/cobra" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/sdk/trace" seiconfig "github.com/sei-protocol/sei-config" "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + serverconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" "github.com/sei-protocol/sei-chain/testutil/configtest" ) @@ -43,12 +47,9 @@ func TestSelect(t *testing.T) { } } -// TestResolveHomeDir_Flag confirms resolveHomeDir reads the --home flag — the -// value v2 validates against must be the dir the re-entered handler reads. (Env -// precedence follows viper, mirrored from the legacy handler; the end-to-end -// env-driven case is exercised by TestConfigManagerLegacyVsV2Differential_EnvHome -// in the cmd package, which resolves the test-binary-basename prefix and asserts -// legacy/v2 parity.) +// TestResolveHomeDir_Flag confirms resolveHomeDir reads the --home flag. That the +// value it returns is the same one the re-entered handler reads is a separate +// property, asserted in TestResolveHomeDirAgreesWithTheLegacyHandler. func TestResolveHomeDir_Flag(t *testing.T) { cmd := &cobra.Command{} cmd.Flags().String(flags.FlagHome, "", "") @@ -68,9 +69,9 @@ func TestResolveHomeDir_Flag(t *testing.T) { // silently ceasing to answer the environment under any other binary name, and the // test binary is never named seid, so deriving the key is what makes this real. // -// This asserts resolveHomeDir alone. That it agrees with the home the legacy handler -// reads is the lockstep property, and it is asserted end to end against the real -// handler in TestConfigManagerLegacyVsV2Differential_EnvHome. +// This asserts resolveHomeDir alone, against literal expectations. Agreement with the +// home the legacy handler reads is the lockstep property, and it is asserted against +// the real handler in TestResolveHomeDirAgreesWithTheLegacyHandler. func TestResolveHomeDirEnvAndPrecedence(t *testing.T) { prefix, err := configtest.ServerEnvPrefix() require.NoError(t, err) @@ -104,6 +105,60 @@ func TestResolveHomeDirEnvAndPrecedence(t *testing.T) { }) } +// TestResolveHomeDirAgreesWithTheLegacyHandler is the lockstep assertion: the home v2 +// validates has to be the home the handler it re-enters actually reads. +// +// No test outside this one can reach that property. v2's channels are produced +// entirely by the legacy handler, so a resolveHomeDir that drifted from the handler +// would only cause the advisory read to skip or warn, which changes no channel and +// fails no parity assertion anywhere. The result is that v2 would report diagnostics +// about one directory while the node booted on another, which is the silent drift the +// advisory design cannot surface on its own. So it is asserted directly, against the +// real handler, for both ways a home arrives. +func TestResolveHomeDirAgreesWithTheLegacyHandler(t *testing.T) { + cases := []struct { + name string + setup func(t *testing.T, cmd *cobra.Command, root string) + }{ + {"home from the flag", func(t *testing.T, cmd *cobra.Command, root string) { + require.NoError(t, cmd.Flags().Set(flags.FlagHome, root)) + }}, + {"home from the environment", func(t *testing.T, cmd *cobra.Command, root string) { + prefix, err := configtest.ServerEnvPrefix() + require.NoError(t, err) + t.Setenv(configtest.ServerEnvKey(prefix, flags.FlagHome), root) + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + configtest.Isolate(t) + root := filepath.Join(t.TempDir(), "node") + require.NoError(t, os.MkdirAll(root, 0o750)) + + // A real StartCmd, so the flag set and its defaults are the ones seid ships. + cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) + tc.setup(t, cmd, root) + + serverCtx := &server.Context{} + cmd.SetContext(context.WithValue(context.Background(), server.ServerContextKey, serverCtx)) + + // The real handler, which is what v2 re-enters and therefore what it must agree with. + require.NoError(t, LegacyConfigManager{}.Apply(cmd, + serverconfig.DefaultConfigTemplate, serverconfig.DefaultConfig())) + + handlerHome := serverCtx.Viper.GetString(flags.FlagHome) + require.Equal(t, root, handlerHome, + "the fixture did not drive the handler's resolution, so this comparison would be vacuous") + + got, err := resolveHomeDir(cmd) + require.NoError(t, err) + require.Equal(t, handlerHome, got, + "resolveHomeDir has drifted from the legacy handler: v2 would validate %q while "+ + "the node boots on %q, and no parity assertion can see it", got, handlerHome) + }) + } +} + // writeMinimalHome creates a node directory carrying the two files // ReadConfigFromDir decodes, with the given contents, and returns its root. // diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 879652766c..c753a410ab 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -225,14 +225,17 @@ func TestConfigManagerLegacyVsV2Differential(t *testing.T) { "settings diverge after the start.go chain-id mutation") } -// TestConfigManagerLegacyVsV2Differential_EnvHome exercises the env-precedence half -// of resolveHomeDir's mirror of the legacy handler — the flag-driven differential -// above never touches SetEnvPrefix/AutomaticEnv. When home is supplied via the -// environment (not --home), v2 must resolve the SAME home the legacy handler does; -// otherwise v2 would advisorily validate one dir while the re-entered legacy reader -// boots on another — a silent drift the advisory design cannot surface (no error, no -// diagnostic). This pins the seam so a future change to the legacy env-resolution -// can't diverge undetected. +// TestConfigManagerLegacyVsV2Differential_EnvHome runs the whole differential with the +// home arriving through the environment instead of --home, which the flag-driven case +// above never exercises: it is the only path here that goes through SetEnvPrefix and +// AutomaticEnv, on both managers. +// +// What it does not assert is that v2's own resolveHomeDir agrees with the handler. +// Both assertions below read the home out of the server context, which is the value +// the re-entered legacy handler resolved, so a drifted resolveHomeDir would only make +// the advisory read skip or warn and every assertion here would still pass. That +// property is asserted directly in the configmanager package, by +// TestResolveHomeDirAgreesWithTheLegacyHandler. func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { configtest.Isolate(t) home := configtest.NewHome(t) From 5493ec7dce15763f9cfa27f5f867f0115ea46378 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 12:42:24 -0700 Subject: [PATCH 21/34] test(configmanager): close three gaps the review found in the new assertions All three are cases where an assertion was weaker than the comment above it, which is the same defect this suite exists to catch, so they are worth naming. The lockstep assertion's env sub-case was not asserting lockstep. The legacy handler ends in bindFlags, which writes the env-resolved value back onto --home and marks it Changed, so a resolveHomeDir read afterwards was reading that flag rather than resolving anything, and a version with SetEnvPrefix and AutomaticEnv removed entirely would still have passed. It is now captured before the handler runs, which is also the order Apply uses, so the test matches production. The no-write snapshot listed one level of config/ and recorded subdirectories as the bare string "dir", while the claim above it was that nothing is authored at boot. A sei.toml at the home root sat outside it. It now walks the whole home. Verified by having v2 write exactly that file: the walk fails on it, and the old snapshot would not have. logAdvisory had no test, so the panic branch, the resolve-stage branch and the truncation arithmetic were unexercised. The truncation is now a separate function so the numbers can be asserted rather than read out of a log line, with a table across the cap boundary that also checks the rendered part is the first N in order and that shown plus omitted accounts for the whole list. logAdvisory itself is exercised over every outcome shape, asserting it does not panic, since a panic in the reporter would turn an advisory pass into a boot failure. --- cmd/seid/cmd/configmanager/configmanager.go | 16 ++-- .../cmd/configmanager/configmanager_test.go | 83 ++++++++++++++++++- .../cmd/configmanager_differential_test.go | 58 ++++++++----- 3 files changed, 130 insertions(+), 27 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index edf2aa34b6..958cffb1d1 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -156,15 +156,21 @@ func logAdvisory(out advisoryOutcome) { if len(out.Diagnostics) == 0 { return } - shown, omitted := out.Diagnostics, 0 - if len(shown) > maxLoggedDiagnostics { - omitted = len(shown) - maxLoggedDiagnostics - shown = shown[:maxLoggedDiagnostics] - } + shown, omitted := capDiagnostics(out.Diagnostics) logger.Warn("advisory config validation diagnostics (not enforced; node will boot)", "count", len(out.Diagnostics), "diagnostics", shown, "omitted", omitted) } +// capDiagnostics splits a diagnostic list into the part to render and the number left +// out. It is separate from logAdvisory so the arithmetic can be asserted directly: +// an off-by-one or an inverted omitted count is not visible in a log line anyone reads. +func capDiagnostics(diags []string) (shown []string, omitted int) { + if len(diags) <= maxLoggedDiagnostics { + return diags, 0 + } + return diags[:maxLoggedDiagnostics], len(diags) - maxLoggedDiagnostics +} + // resolveHomeDir resolves --home the same way the legacy handler does // (sei-cosmos/server/util.go), so v2 validates the directory the handler reads. func resolveHomeDir(cmd *cobra.Command) (string, error) { diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index ca21adea22..48f348b977 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -2,6 +2,8 @@ package configmanager import ( "context" + "errors" + "fmt" "os" "path/filepath" "strings" @@ -139,6 +141,15 @@ func TestResolveHomeDirAgreesWithTheLegacyHandler(t *testing.T) { cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) tc.setup(t, cmd, root) + // Captured before the handler runs, which is both what Apply does and what makes + // this an assertion about resolveHomeDir. The handler ends in bindFlags, which + // writes the env-resolved value back onto the --home flag and marks it Changed + // (sei-cosmos/server/util.go), so a resolveHomeDir read afterwards would be + // reading that flag rather than resolving anything, and a version that had + // dropped SetEnvPrefix and AutomaticEnv entirely would still pass. + got, err := resolveHomeDir(cmd) + require.NoError(t, err) + serverCtx := &server.Context{} cmd.SetContext(context.WithValue(context.Background(), server.ServerContextKey, serverCtx)) @@ -150,8 +161,6 @@ func TestResolveHomeDirAgreesWithTheLegacyHandler(t *testing.T) { require.Equal(t, root, handlerHome, "the fixture did not drive the handler's resolution, so this comparison would be vacuous") - got, err := resolveHomeDir(cmd) - require.NoError(t, err) require.Equal(t, handlerHome, got, "resolveHomeDir has drifted from the legacy handler: v2 would validate %q while "+ "the node boots on %q, and no parity assertion can see it", got, handlerHome) @@ -235,6 +244,76 @@ func TestValidateAdvisorySkipsAnUnresolvedHome(t *testing.T) { require.Nil(t, out.Panic) } +// TestCapDiagnostics pins the truncation arithmetic at its boundary. A miscount here +// would misreport how much was left out of a log line, which is the kind of defect +// nobody notices by reading the output. +func TestCapDiagnostics(t *testing.T) { + diags := func(n int) []string { + out := make([]string, n) + for i := range out { + out[i] = fmt.Sprintf("d%d", i) + } + return out + } + cases := []struct { + name string + in int + wantShown int + wantOmitted int + }{ + {"none", 0, 0, 0}, + {"one", 1, 1, 0}, + {"exactly at the cap", maxLoggedDiagnostics, maxLoggedDiagnostics, 0}, + {"one over the cap", maxLoggedDiagnostics + 1, maxLoggedDiagnostics, 1}, + {"far over the cap", maxLoggedDiagnostics + 15, maxLoggedDiagnostics, 15}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + in := diags(tc.in) + shown, omitted := capDiagnostics(in) + + require.Len(t, shown, tc.wantShown) + require.Equal(t, tc.wantOmitted, omitted) + // Nothing may be invented or reordered, and the two parts must account for + // the whole list, which is what makes the reported count trustworthy. + require.Equal(t, tc.in, len(shown)+omitted, "shown plus omitted must be the whole list") + for i := range shown { + require.Equal(t, in[i], shown[i], "the rendered part must be the first %d in order", tc.wantShown) + } + }) + } +} + +// TestLogAdvisoryHandlesEveryOutcome exercises the branches no other test reaches: a +// recovered panic, a failure at each stage, and a truncated diagnostic list. It +// asserts the reporting path survives each shape rather than asserting log text, since +// the text is not a contract; a panic or nil dereference in the reporter would turn an +// advisory pass into a boot failure, which is the one thing it must never do. +func TestLogAdvisoryHandlesEveryOutcome(t *testing.T) { + many := make([]string, maxLoggedDiagnostics+3) + for i := range many { + many[i] = fmt.Sprintf("[ERROR] field%d: broken", i) + } + cases := []struct { + name string + out advisoryOutcome + }{ + {"zero value", advisoryOutcome{}}, + {"skipped", advisoryOutcome{Skipped: true}}, + {"resolve failed", advisoryOutcome{Stage: "resolve", Err: errors.New("no home")}}, + {"read failed", advisoryOutcome{Stage: "read", Err: errors.New("bad toml")}}, + {"panicked", advisoryOutcome{Panic: "boom", Stack: []byte("goroutine 1 [running]:\n")}}, + {"panicked with no stack", advisoryOutcome{Panic: errors.New("boom")}}, + {"one diagnostic", advisoryOutcome{Home: "/tmp/n", Diagnostics: []string{"[ERROR] a: b"}}}, + {"more diagnostics than the cap", advisoryOutcome{Home: "/tmp/n", Diagnostics: many}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.NotPanics(t, func() { logAdvisory(tc.out) }) + }) + } +} + // TestReadConfigFromDirMissingIsErrNotExist pins the contract validateAdvisory's // silent-skip depends on: a missing config file must yield an error that // errors.Is(os.ErrNotExist) recognizes, so a fresh-home boot skips the advisory diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index c753a410ab..87524f071f 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -6,7 +6,9 @@ import ( "errors" "fmt" "io" + "io/fs" "os" + "path/filepath" "strings" "testing" @@ -337,23 +339,39 @@ func TestConfigManagerV2AdvisoryReadErrorMatchesLegacy(t *testing.T) { "v2 must return the same boot error as legacy, not mask or add one") } -// snapshotConfigDir records the name, size and content hash of every file under the -// home's config directory, which is what a no-write claim has to be checked against. -func snapshotConfigDir(t *testing.T, home *configtest.Home) map[string]string { +// snapshotHome records every path under the home, with a content hash for each file. +// +// It walks the whole home rather than listing config/, because the claim it backs is +// that nothing is authored at boot, and a sei.toml at the home root or a file dropped +// into a subdirectory of config/ would both sit outside a single-level listing of +// config/. Directories are recorded as names so an added empty one still shows up. +func snapshotHome(t *testing.T, home *configtest.Home) map[string]string { t.Helper() - entries, err := os.ReadDir(home.ConfigDir()) - require.NoError(t, err) - snap := make(map[string]string, len(entries)) - for _, e := range entries { - if e.IsDir() { - snap[e.Name()+"/"] = "dir" - continue + snap := map[string]string{} + err := filepath.WalkDir(home.Root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err } - b, ok := home.Read(t, e.Name()) - require.True(t, ok, "listed file %q could not be read", e.Name()) - snap[e.Name()] = fmt.Sprintf("%d:%x", len(b), sha256.Sum256(b)) - } + rel, err := filepath.Rel(home.Root, path) + if err != nil { + return err + } + if rel == "." { + return nil + } + if d.IsDir() { + snap[rel+"/"] = "dir" + return nil + } + b, err := os.ReadFile(path) //nolint:gosec // fixture path under the test's temp dir + if err != nil { + return err + } + snap[rel] = fmt.Sprintf("%d:%x", len(b), sha256.Sum256(b)) + return nil + }) + require.NoError(t, err) return snap } @@ -363,7 +381,7 @@ func snapshotConfigDir(t *testing.T, home *configtest.Home) map[string]string { // Every other assertion here compares the resolved channels, so a v2-side write that // leaves those channels alone passes all of them. Authoring a sei.toml, rewriting a // file to nearly the same bytes, or adding anything to the config directory would all -// go unnoticed. This snapshots the directory around the v2 run and compares, so the +// go unnoticed. This walks the whole home around the v2 run and compares, so the // prose guarantee becomes an enforced one. // // The home is pre-seeded on purpose. A fresh home is the one case where writes are @@ -373,14 +391,14 @@ func TestConfigManagerV2WritesNothing(t *testing.T) { configtest.Isolate(t) home := seedDefaultConfig(t) - before := snapshotConfigDir(t, home) + before := snapshotHome(t, home) _ = runConfigManager(t, configmanager.SeiConfigManager{}, home) - after := snapshotConfigDir(t, home) + after := snapshotHome(t, home) require.Equal(t, before, after, - "v2 changed the config directory. It re-enters the legacy reader on the operator's "+ - "own files and must not author, migrate or rewrite anything at boot, and a write "+ - "that leaves the resolved channels alone is invisible to every other assertion here") + "v2 changed the node directory. It re-enters the legacy reader on the operator's own "+ + "files and must not author, migrate or rewrite anything at boot, and a write that "+ + "leaves the resolved channels alone is invisible to every other assertion here") } // FuzzConfigManagerEnvOnlyKeyParity closes the one class the AllSettings comparison From 22620e2d5cf0ec5b8ba9e126d47bac53c04e0979 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 13:01:56 -0700 Subject: [PATCH 22/34] fix(configmanager): keep a reporting panic from refusing boot From an independent cross-review. The security lens found that the refactor which made the advisory pass observable moved the logging out from under the recover that used to cover it: every logger call used to sit inside validateAdvisory, and Apply now called logAdvisory after it returned. A panic in the reporting path therefore propagated out of Apply into PersistentPreRunE and refused the boot, which is the one thing this manager promises it cannot do. It was a regression introduced while closing an earlier review comment, and no test could have caught it, since the existing one only asserts the reporter does not panic on the shapes it is handed. reportAdvisory now contains the pass and the reporting under one recover, with a deliberately minimal handler that does not touch the value that may have caused the panic. The pass's stage discriminator is a typed constant instead of a string, so a mistyped stage is a compile error rather than a switch case that silently drops an operator warning. The fixture home in the package test now uses configtest.NewHome rather than hand-rolling the same directory, which is what the suite's own rails ask for. The differential's header no longer describes itself as a parity proof. While v2 is a passthrough both sides of every comparison run the same legacy reader, so those rows detect a side effect and cannot fail on their own; the rows that can are the ones asserting something other than equality between two runs of one reader. Saying so is more useful than the claim, and the claim becomes true when the resolver lands. --- cmd/seid/cmd/configmanager/configmanager.go | 49 +++++++++++++++---- .../cmd/configmanager/configmanager_test.go | 14 +++--- .../cmd/configmanager_differential_test.go | 21 ++++++-- 3 files changed, 62 insertions(+), 22 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 958cffb1d1..6f4b373428 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -48,21 +48,41 @@ type SeiConfigManager struct{} // Apply runs the advisory validation pass, then re-enters the legacy handler on // the operator's original files. Nothing in the validation pass refuses boot. func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { - logAdvisory(validateAdvisory(cmd)) + reportAdvisory(cmd) return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) } +// reportAdvisory runs the advisory pass and reports what it found, containing a panic +// from either step. +// +// The recover has to cover the reporting and not just the pass. Everything here is +// advisory, so the one promise this manager makes is that it cannot refuse a boot the +// legacy path would have allowed, and a panic escaping the reporter would do exactly +// that by propagating out of Apply into PersistentPreRunE. Keeping the pass's own +// recover and adding none here would leave that hole open, since the pass returns +// normally and the reporter runs after it. +func reportAdvisory(cmd *cobra.Command) { + defer func() { + if recover() != nil { + // Deliberately the smallest possible call: this runs after something already + // panicked, so it does not touch the value that may have caused it. + logger.Error("config validation reporting panicked (advisory; recovered, node will boot)") + } + }() + logAdvisory(validateAdvisory(cmd)) +} + // advisoryOutcome is what the validation pass saw. The pass reports rather than logs // so that it is observable, which nothing else here can be: a channel-parity or // never-refuses-boot assertion holds just as well when the read always fails or the // validation has quietly become a no-op, so something has to be able to see that the -// pass ran and what it found. Apply does the logging. +// pass ran and what it found. reportAdvisory does the logging. type advisoryOutcome struct { // Home is the directory the pass read, empty when it never got that far. Home string - // Stage names where the pass stopped, "resolve" or "read", and is empty when it - // completed. It is what keeps the two failures separately reportable. - Stage string + // Stage names where the pass stopped, and is stageNone when it completed. It is + // what keeps the two failures separately reportable. + Stage stage // Skipped records that there was nothing to validate: no home resolved, or no // config on disk yet, which is the normal case on a fresh node. Skipped bool @@ -77,6 +97,17 @@ type advisoryOutcome struct { Stack []byte } +// stage names how far the advisory pass got. It is a typed constant rather than a +// string because it is an internal discriminator matched by name, so a mistyped value +// should be a compile error instead of a case that silently drops an operator warning. +type stage int + +const ( + stageNone stage = iota // the pass completed + stageResolve // stopped resolving the home dir + stageRead // stopped reading the config +) + // validateAdvisory resolves the home dir, reads the on-disk config and validates it, // reporting what it saw. Every outcome is advisory: a failure, or a panic in the // sei-config read or validate, is captured and returned rather than propagated, so @@ -104,7 +135,7 @@ func validateAdvisory(cmd *cobra.Command) (out advisoryOutcome) { home, err := resolveHomeDir(cmd) if err != nil { - out.Stage, out.Err = "resolve", err + out.Stage, out.Err = stageResolve, err return out } // An unresolved home would send the read at ./config relative to the process @@ -125,7 +156,7 @@ func validateAdvisory(cmd *cobra.Command) (out advisoryOutcome) { out.Skipped = true return out } - out.Stage, out.Err = "read", err + out.Stage, out.Err = stageRead, err return out } @@ -147,9 +178,9 @@ func logAdvisory(out advisoryOutcome) { case out.Panic != nil: logger.Error("config validation panicked (advisory; recovered, node will boot)", "panic", out.Panic, "stack", string(out.Stack)) - case out.Stage == "resolve": + case out.Stage == stageResolve: logger.Warn("could not resolve home dir for config validation (advisory)", "error", out.Err) - case out.Stage == "read": + case out.Stage == stageRead: logger.Warn("could not read config for validation (advisory)", "error", out.Err) } diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index 48f348b977..a9915267cc 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -178,12 +178,10 @@ func TestResolveHomeDirAgreesWithTheLegacyHandler(t *testing.T) { // depend on either. func writeMinimalHome(t *testing.T, configTOML, appTOML string) string { t.Helper() - root := filepath.Join(t.TempDir(), "node") - cfgDir := filepath.Join(root, "config") - require.NoError(t, os.MkdirAll(cfgDir, 0o750)) - require.NoError(t, os.WriteFile(filepath.Join(cfgDir, "config.toml"), []byte(configTOML), 0o600)) - require.NoError(t, os.WriteFile(filepath.Join(cfgDir, "app.toml"), []byte(appTOML), 0o600)) - return root + h := configtest.NewHome(t) + h.WriteConfigTOML(t, []byte(configTOML)) + h.WriteAppTOML(t, []byte(appTOML)) + return h.Root } // homeCmd returns a command whose --home is set to root. @@ -300,8 +298,8 @@ func TestLogAdvisoryHandlesEveryOutcome(t *testing.T) { }{ {"zero value", advisoryOutcome{}}, {"skipped", advisoryOutcome{Skipped: true}}, - {"resolve failed", advisoryOutcome{Stage: "resolve", Err: errors.New("no home")}}, - {"read failed", advisoryOutcome{Stage: "read", Err: errors.New("bad toml")}}, + {"resolve failed", advisoryOutcome{Stage: stageResolve, Err: errors.New("no home")}}, + {"read failed", advisoryOutcome{Stage: stageRead, Err: errors.New("bad toml")}}, {"panicked", advisoryOutcome{Panic: "boom", Stack: []byte("goroutine 1 [running]:\n")}}, {"panicked with no stack", advisoryOutcome{Panic: errors.New("boom")}}, {"one diagnostic", advisoryOutcome{Home: "/tmp/n", Diagnostics: []string{"[ERROR] a: b"}}}, diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 87524f071f..bbeeca7964 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -22,9 +22,18 @@ import ( "github.com/sei-protocol/sei-chain/testutil/configtest" ) -// This file is the equality foundation for the two managers: everything here -// compares what legacy resolves against what v2 resolves, on one home, through the -// two channels the rest of the boot reads. +// This file compares what legacy resolves against what v2 resolves, on one home, +// through the two channels the rest of the boot reads. +// +// Be precise about what that currently proves. While v2 is a passthrough, both sides +// of every comparison here run the same legacy reader, so these rows cannot fail +// unless the advisory pass has a side effect. They are side-effect detectors, and the +// scaffolding the real comparison will run on, not yet a proof that two independent +// implementations agree: the second implementation does not exist yet. The rows that +// can fail on their own today are the ones that assert something other than equality +// between two runs of the same reader, namely that v2 writes nothing, that the +// advisory pass actually produced a finding, and that resolveHomeDir agrees with the +// handler. // // It runs on the same harness as the characterization suite (testutil/configtest), // which is what makes the comparison trustworthy. Isolate pins the process @@ -194,8 +203,10 @@ func configCorpus() []corpusCase { } } -// TestConfigManagerLegacyVsV2Differential is the core safety property: the v2 -// manager must produce the SAME consumed config as the legacy path. v2 reads the +// TestConfigManagerLegacyVsV2Differential is the shape the parity proof will take: the +// v2 manager must produce the SAME consumed config as the legacy path. While v2 passes +// through, it detects a side effect rather than proving agreement between two +// implementations (see the file header). v2 reads the // config (to validate it) and then re-enters the legacy reader on the operator's // ORIGINAL files — it does not rewrite — so the two paths read the SAME home and any // difference is a real divergence, not a path artifact. From 4bd83303cb674c590ce52fd2c1fec9ed07abeec7 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 13:06:19 -0700 Subject: [PATCH 23/34] fix(configmanager): report every non-completing stage, name the home, skip flag keys Three review follow-ups, one of them a defect in a test added earlier in this PR. The stage switch had a case per stage, so a stage added later without a matching case would have logged nothing and turned a failure into silence. It now reports any stage that is not the completed one, and stage carries a String so the attribute is legible. The resolved home is now in the diagnostics line. The reason the lockstep test exists is that a drifted resolveHomeDir would have these diagnostics describe a directory the node is not booting on, and without the path in the line there was no way to tell that from a log. FuzzConfigManagerEnvOnlyKeyParity asserted that its probe value arrived through the environment, but nothing stopped the fuzzer choosing a key the harness also sets as a flag. startCmdForHome marks --home Changed, and a Changed pflag outranks AutomaticEnv, so that key would have read back the fixture root and failed as a not-live probe with no divergence present. The precedence is already asserted by TestResolveHomeDirEnvAndPrecedence in this package, which is what makes the collision real rather than theoretical. Reachable by -fuzz rather than by CI, since only the seed corpus runs under plain go test. --- cmd/seid/cmd/configmanager/configmanager.go | 27 ++++++++++++++++--- .../cmd/configmanager_differential_test.go | 9 +++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 6f4b373428..f5adc4e13d 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -108,6 +108,20 @@ const ( stageRead // stopped reading the config ) +// String names the stage for a log line. +func (s stage) String() string { + switch s { + case stageNone: + return "none" + case stageResolve: + return "resolve" + case stageRead: + return "read" + default: + return fmt.Sprintf("stage(%d)", int(s)) + } +} + // validateAdvisory resolves the home dir, reads the on-disk config and validates it, // reporting what it saw. Every outcome is advisory: a failure, or a panic in the // sei-config read or validate, is captured and returned rather than propagated, so @@ -180,16 +194,23 @@ func logAdvisory(out advisoryOutcome) { "panic", out.Panic, "stack", string(out.Stack)) case out.Stage == stageResolve: logger.Warn("could not resolve home dir for config validation (advisory)", "error", out.Err) - case out.Stage == stageRead: - logger.Warn("could not read config for validation (advisory)", "error", out.Err) + // Any other non-completing stage still reports. A stage added without a case here + // would otherwise log nothing at all, turning a failure into silence, which is the + // opposite of what an advisory pass is for. + case out.Stage != stageNone: + logger.Warn("could not read config for validation (advisory)", + "stage", out.Stage, "error", out.Err) } if len(out.Diagnostics) == 0 { return } shown, omitted := capDiagnostics(out.Diagnostics) + // The home is reported because a resolveHomeDir that drifted from the legacy + // handler would have these diagnostics describe a directory the node is not + // booting on, and without the path in the line there is no way to tell from a log. logger.Warn("advisory config validation diagnostics (not enforced; node will boot)", - "count", len(out.Diagnostics), "diagnostics", shown, "omitted", omitted) + "home", out.Home, "count", len(out.Diagnostics), "diagnostics", shown, "omitted", omitted) } // capDiagnostics splits a diagnostic list into the part to render and the number left diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index bbeeca7964..640a88d0ad 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -447,6 +447,15 @@ func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { if value == "" { return } + // A key the harness sets as a flag is not an env-only key. startCmdForHome does + // cmd.Flags().Set(--home), which marks that flag Changed, and a Changed pflag + // outranks AutomaticEnv in viper's precedence, so Get would return the fixture + // root rather than the value under test and the probe would fail as not-live + // without any divergence existing. Only the seed corpus runs under plain + // `go test`, so this is reachable by `-fuzz` rather than by CI. + if key == flags.FlagHome { + return + } configtest.Isolate(t) prefix, err := configtest.ServerEnvPrefix() require.NoError(t, err) From 0179bed54911af530a915915b03117b5a1e58827 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 13:41:13 -0700 Subject: [PATCH 24/34] fix(configmanager): surface the declined pass, and guard the derived env name Two of these close holes in fixes from the previous round. The unresolved-home skip was silent. Skipped was set and logAdvisory had no case for it, so an operator who opted into v2 with no home to resolve could not tell a declined pass from a pass that ran and found nothing. It now reports at Info. Home is what separates the two skips, since it is empty only when the home never resolved, so the ordinary fresh-node case of a missing config file stays quiet. The catch-all stage arm still carried the read-specific message, so a stage added between resolve and read would have been reported as a read failure. The phrasing is neutral now, for the same reason the arm is a catch-all. The env-only fuzz guard compared the key rather than the variable it derives. ServerEnvKey upper-cases and rewrites separators, so "HOME" and "Home" derive the same variable as --home while passing an equality test against the flag name, and that flag is Changed, which outranks AutomaticEnv. Comparing the derived name closes every spelling rather than the one. Verified: all three spellings derive one variable. --- cmd/seid/cmd/configmanager/configmanager.go | 16 ++++++++++---- .../cmd/configmanager_differential_test.go | 22 +++++++++++-------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index f5adc4e13d..a70b48d344 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -194,12 +194,20 @@ func logAdvisory(out advisoryOutcome) { "panic", out.Panic, "stack", string(out.Stack)) case out.Stage == stageResolve: logger.Warn("could not resolve home dir for config validation (advisory)", "error", out.Err) - // Any other non-completing stage still reports. A stage added without a case here - // would otherwise log nothing at all, turning a failure into silence, which is the - // opposite of what an advisory pass is for. + // Any other non-completing stage still reports, so a stage added without its own + // case above logs something rather than nothing. The phrasing is neutral for the + // same reason: naming a step here would misreport a stage added later, and the + // stage attribute carries which one it actually was. case out.Stage != stageNone: - logger.Warn("could not read config for validation (advisory)", + logger.Warn("config validation stopped early (advisory)", "stage", out.Stage, "error", out.Err) + // An unresolved home is closer to a misconfiguration than to a quiet default, and + // it is reported so an operator who opted into v2 can tell a declined pass from a + // pass that ran and found nothing. Home is what separates the two skips: it is + // empty only when the home never resolved, and the missing-config skip below it + // is the ordinary fresh-node case, which stays quiet. + case out.Skipped && out.Home == "": + logger.Info("config validation skipped: no home dir resolved (advisory)") } if len(out.Diagnostics) == 0 { diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 640a88d0ad..ecfec93ef5 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -447,15 +447,6 @@ func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { if value == "" { return } - // A key the harness sets as a flag is not an env-only key. startCmdForHome does - // cmd.Flags().Set(--home), which marks that flag Changed, and a Changed pflag - // outranks AutomaticEnv in viper's precedence, so Get would return the fixture - // root rather than the value under test and the probe would fail as not-live - // without any divergence existing. Only the seed corpus runs under plain - // `go test`, so this is reachable by `-fuzz` rather than by CI. - if key == flags.FlagHome { - return - } configtest.Isolate(t) prefix, err := configtest.ServerEnvPrefix() require.NoError(t, err) @@ -463,6 +454,19 @@ func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { if envKey == prefix+"_" || strings.ContainsAny(envKey, "= ") { return // not a name the environment can carry } + // A key that derives the same variable as a flag the harness sets is not an + // env-only key. startCmdForHome does cmd.Flags().Set(--home), which marks that + // flag Changed, and a Changed pflag outranks AutomaticEnv, so Get would return + // the fixture root rather than the value under test and the probe would fail as + // not-live with no divergence present. + // + // The comparison is on the derived variable name rather than on the key, because + // ServerEnvKey upper-cases and rewrites separators: "HOME", "Home" and "home" all + // derive one variable, and a key test would let the first two through. Only the + // seed corpus runs under plain `go test`, so this is reachable by -fuzz, not CI. + if envKey == configtest.ServerEnvKey(prefix, flags.FlagHome) { + return + } t.Setenv(envKey, value) home := seedDefaultConfig(t) From 16ee3e94c6875d0fb804a1ac2d2b6dfb55240fe7 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 14:16:13 -0700 Subject: [PATCH 25/34] configmanager: guard the advisory recover handler's log call; correct the re-entry-ordering rationale --- cmd/seid/cmd/configmanager/configmanager.go | 25 ++++++++++++++------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index a70b48d344..d41da56035 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -64,8 +64,13 @@ func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string func reportAdvisory(cmd *cobra.Command) { defer func() { if recover() != nil { - // Deliberately the smallest possible call: this runs after something already - // panicked, so it does not touch the value that may have caused it. + // This runs after something already panicked, and the logger is itself a + // plausible cause (a broken handler, or a writer panicking on a closed fd), + // so guard the report: a second panic here must not escape the deferred + // func and refuse a boot the legacy path would have allowed. The message + // touches no dynamic value, so this only defends against a logger broken + // independent of its arguments. + defer func() { _ = recover() }() logger.Error("config validation reporting panicked (advisory; recovered, node will boot)") } }() @@ -134,12 +139,16 @@ func (s stage) String() string { // its first boot, since there is nothing on disk yet and the handler writes the files // afterwards, so the earliest a diagnostic can appear is the second start. // -// Running it after re-entry as well would close that, and it is deliberately not done -// yet, because sei-config today reports an error against a freshly generated default -// config (the pruning read-mapping gap the design tracks). Validating generated files -// before that gap closes would mean every fresh node logging an error about a file -// seid itself had just written, which is worse than validating a boot late. This is -// worth revisiting when validation goes fatal, and the two decisions belong together. +// Running it after re-entry as well would also validate on boot #1, and it is +// deliberately not done yet, because sei-config today reports an error against a freshly +// generated default config (the pruning read-mapping gap the design tracks). Note this +// ordering DEFERS that spurious warning by one boot rather than avoiding it: from the +// second boot on, the pre-handler pass reads the same seid-generated, operator-untouched +// config and logs the pruning-gap diagnostic until the gap closes, so a v2 node that +// never touches its config still warns on every start. Validating after re-entry too +// would only add the same warning on boot #1. Closing the pruning gap is therefore a +// prerequisite for wider v2 rollout, not just for making validation fatal; revisit the +// two together. func validateAdvisory(cmd *cobra.Command) (out advisoryOutcome) { defer func() { if r := recover(); r != nil { From 3259f249febbfe152149cbc708a5c448be4fe078 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 14:29:05 -0700 Subject: [PATCH 26/34] configmanager test: skip fuzz keys colliding with any bound flag's env var (not just --home) --- .../cmd/configmanager_differential_test.go | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index ecfec93ef5..889eab2a3c 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/sdk/trace" @@ -454,17 +455,29 @@ func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { if envKey == prefix+"_" || strings.ContainsAny(envKey, "= ") { return // not a name the environment can carry } - // A key that derives the same variable as a flag the harness sets is not an - // env-only key. startCmdForHome does cmd.Flags().Set(--home), which marks that - // flag Changed, and a Changed pflag outranks AutomaticEnv, so Get would return - // the fixture root rather than the value under test and the probe would fail as - // not-live with no divergence present. + // A key that derives the same variable as any bound flag the legacy handler + // consumes is not an env-only probe. The handler binds the StartCmd flags into + // its viper, and such a flag either outranks AutomaticEnv (the harness sets + // --home Changed, so Get returns the fixture root rather than the value under + // test — not-live) or is parsed and can reject the value outright (SEID_LOG_LEVEL + // goes through lvl.UnmarshalText and errors on an unparseable level, tripping the + // require.NoError in execConfigManager). Either way there is no divergence to + // compare, so skip when the derived variable collides with any bound flag's. // - // The comparison is on the derived variable name rather than on the key, because + // The comparison is on the derived variable name rather than the key, because // ServerEnvKey upper-cases and rewrites separators: "HOME", "Home" and "home" all // derive one variable, and a key test would let the first two through. Only the // seed corpus runs under plain `go test`, so this is reachable by -fuzz, not CI. - if envKey == configtest.ServerEnvKey(prefix, flags.FlagHome) { + probeCmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) + collidesWithFlag := false + checkFlag := func(f *pflag.Flag) { + if envKey == configtest.ServerEnvKey(prefix, f.Name) { + collidesWithFlag = true + } + } + probeCmd.Flags().VisitAll(checkFlag) + probeCmd.PersistentFlags().VisitAll(checkFlag) + if collidesWithFlag { return } t.Setenv(envKey, value) From 230292d165a841a5b1edc9ee96ebcee55f695f22 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 14:29:07 -0700 Subject: [PATCH 27/34] configmanager: TODO to extract server.ResolveHomeDir and drop the duplicated viper bootstrap --- cmd/seid/cmd/configmanager/configmanager.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index d41da56035..2f231511ab 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -242,6 +242,11 @@ func capDiagnostics(diags []string) (shown []string, omitted int) { // resolveHomeDir resolves --home the same way the legacy handler does // (sei-cosmos/server/util.go), so v2 validates the directory the handler reads. +// +// TODO: this re-implements ~15 lines of the legacy handler's viper bootstrap +// (sei-cosmos/server/util.go). Extract an exported server.ResolveHomeDir(cmd) and call +// it from both sides once the resolver lands and this is load-bearing for more than +// diagnostics; until then TestResolveHomeDirAgreesWithTheLegacyHandler guards the drift. func resolveHomeDir(cmd *cobra.Command) (string, error) { v := viper.New() if err := v.BindPFlags(cmd.Flags()); err != nil { From fa571949e0d1c3a7418c1cfb5c4ae74fcb305c82 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 14:51:31 -0700 Subject: [PATCH 28/34] configmanager: log an Info line when the advisory pass completes clean (operator can see it ran) --- cmd/seid/cmd/configmanager/configmanager.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 2f231511ab..91dd2d63d3 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -217,6 +217,11 @@ func logAdvisory(out advisoryOutcome) { // is the ordinary fresh-node case, which stays quiet. case out.Skipped && out.Home == "": logger.Info("config validation skipped: no home dir resolved (advisory)") + // The pass completed and found nothing. Report at Info so an operator who opted into + // v2 can see it ran clean, distinct from the quiet fresh-node skip (Skipped with a + // home), which stays silent because the legacy handler is about to write those files. + case !out.Skipped && out.Stage == stageNone && len(out.Diagnostics) == 0: + logger.Info("config validation passed: no advisories (node will boot)", "home", out.Home) } if len(out.Diagnostics) == 0 { From 89c81e533a2119cee87ea074a1854696355c496c Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 14:51:32 -0700 Subject: [PATCH 29/34] configmanager test: assert reportAdvisory never escapes even when the logger panics --- .../cmd/configmanager/configmanager_test.go | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index a9915267cc..b953a544bf 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "os" "path/filepath" "strings" @@ -321,3 +322,35 @@ func TestReadConfigFromDirMissingIsErrNotExist(t *testing.T) { _, err := seiconfig.ReadConfigFromDir(t.TempDir()) require.ErrorIs(t, err, os.ErrNotExist) } + +// TestReportAdvisoryNeverEscapesWhenTheLoggerPanics exercises the manager's one hard +// promise: a panic in the advisory path must never propagate out of Apply into +// PersistentPreRunE and refuse a boot the legacy path would have allowed. +// +// The logger is the subsystem this defends against. validateAdvisory recovers its own +// panics (TestValidateAdvisoryReportsWhatItFound asserts a clean pass), and logAdvisory +// is proven panic-free on every outcome (TestLogAdvisoryHandlesEveryOutcome), so the +// remaining exposure is a logger broken independent of its arguments. Swapping in a +// logger whose handler panics on every record makes both the reporting call and the +// recover handler's own log call panic; reportAdvisory must still return normally. +func TestReportAdvisoryNeverEscapesWhenTheLoggerPanics(t *testing.T) { + configtest.Isolate(t) + // A config missing a required field, so the pass produces a diagnostic and + // logAdvisory reaches a logger call rather than the quiet fresh-node skip. + root := writeMinimalHome(t, "mode = \"full\"\n", "") + + orig := logger + t.Cleanup(func() { logger = orig }) + logger = slog.New(panickingHandler{}) + + require.NotPanics(t, func() { reportAdvisory(homeCmd(t, root)) }) +} + +// panickingHandler is a slog.Handler that panics on every record, standing in for a +// logger broken independent of its arguments (a bad handler, a writer on a closed fd). +type panickingHandler struct{} + +func (panickingHandler) Enabled(context.Context, slog.Level) bool { return true } +func (panickingHandler) Handle(context.Context, slog.Record) error { panic("logger handler is broken") } +func (panickingHandler) WithAttrs([]slog.Attr) slog.Handler { return panickingHandler{} } +func (panickingHandler) WithGroup(string) slog.Handler { return panickingHandler{} } From 5e0ac4efd74b92172888246d4a740cd420a12034 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 14:59:57 -0700 Subject: [PATCH 30/34] test(configmanager): name log_level/log_format in the env-only fuzz guard The widened guard walked StartCmd's own flags to skip a fuzzed key that collides with a handler-consumed flag, on the assumption the collision hazard is always a bound flag that outranks AutomaticEnv (true for --home). log_level and log_format are a different hazard: the handler reads them from viper under AutomaticEnv whether or not any flag is bound, and they are registered on the root command, not on the bare StartCmd the harness builds, so a flag walk can never see them. A -fuzz run hitting SEID_LOG_LEVEL= would fail InterceptConfigsPreRunHandler's log-level parse and persist a spurious crasher. Name the two AutomaticEnv-read flags explicitly alongside the walk. --- .../cmd/configmanager_differential_test.go | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 889eab2a3c..7a6bf5e350 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -455,14 +455,20 @@ func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { if envKey == prefix+"_" || strings.ContainsAny(envKey, "= ") { return // not a name the environment can carry } - // A key that derives the same variable as any bound flag the legacy handler - // consumes is not an env-only probe. The handler binds the StartCmd flags into - // its viper, and such a flag either outranks AutomaticEnv (the harness sets - // --home Changed, so Get returns the fixture root rather than the value under - // test — not-live) or is parsed and can reject the value outright (SEID_LOG_LEVEL - // goes through lvl.UnmarshalText and errors on an unparseable level, tripping the - // require.NoError in execConfigManager). Either way there is no divergence to - // compare, so skip when the derived variable collides with any bound flag's. + // A key that derives the same variable as a flag the legacy handler reads is not + // an env-only probe, and there are two distinct ways that happens. A StartCmd flag + // the harness marks Changed (--home) outranks AutomaticEnv, so Get returns the + // fixture root rather than the value under test (not-live). A flag the handler + // reads straight from viper under AutomaticEnv (log_level goes through + // lvl.UnmarshalText and errors on an unparseable level, tripping the require.NoError + // in execConfigManager) rejects the value before any comparison. Either way there + // is no divergence to compare, so skip when the derived variable collides. + // + // The two need different detection. The Changed-flag case is found by walking this + // command's flags. The AutomaticEnv case cannot be: log_level and log_format are + // registered on the root command, not on the bare StartCmd the harness builds, yet + // AutomaticEnv resolves them from the environment whether or not any flag is bound, + // so a flag walk never sees them. They are named explicitly for that reason. // // The comparison is on the derived variable name rather than the key, because // ServerEnvKey upper-cases and rewrites separators: "HOME", "Home" and "home" all @@ -470,13 +476,15 @@ func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { // seed corpus runs under plain `go test`, so this is reachable by -fuzz, not CI. probeCmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) collidesWithFlag := false - checkFlag := func(f *pflag.Flag) { - if envKey == configtest.ServerEnvKey(prefix, f.Name) { + checkName := func(name string) { + if envKey == configtest.ServerEnvKey(prefix, name) { collidesWithFlag = true } } - probeCmd.Flags().VisitAll(checkFlag) - probeCmd.PersistentFlags().VisitAll(checkFlag) + probeCmd.Flags().VisitAll(func(f *pflag.Flag) { checkName(f.Name) }) + probeCmd.PersistentFlags().VisitAll(func(f *pflag.Flag) { checkName(f.Name) }) + checkName(flags.FlagLogLevel) + checkName(flags.FlagLogFormat) if collidesWithFlag { return } From c226dc92eb1c8b5f74e8ba9d5520135edf71615e Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 15:13:17 -0700 Subject: [PATCH 31/34] fix(configmanager): report advisories after re-entry so they honor the operator's log level The advisory pass emitted its lines before InterceptConfigsPreRunHandler applies the configured log level (seilog.SetDefaultLevel), so a node with log_level = "error" still got the advisory output and a higher default would have dropped the diagnostics silently, which defeats a pass whose only output is operator-facing. Split Apply: validate before re-entry (still reading the operator's authored files), report after (now governed by the applied level), and report even when the handler returns an error. The reporter's recover records the recovered panic value and stack rather than discarding them, matching what the pass already captures for a panic in validateAdvisory. --- cmd/seid/cmd/configmanager/configmanager.go | 52 +++++++++++-------- .../cmd/configmanager/configmanager_test.go | 7 ++- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 91dd2d63d3..021ed87c2b 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -45,36 +45,44 @@ func (LegacyConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate str // writes, migrates, or refuses boot. type SeiConfigManager struct{} -// Apply runs the advisory validation pass, then re-enters the legacy handler on -// the operator's original files. Nothing in the validation pass refuses boot. +// Apply validates the operator's config, re-enters the legacy handler on the original +// files, then reports what the validation found. Validation runs before re-entry so it +// reads the files the operator authored rather than the ones the handler generates. The +// reporting runs after so its lines are emitted at the log level the handler applies +// (seilog.SetDefaultLevel), not the pre-config default: without this, a node with +// log_level = "error" still gets the advisory lines and a higher default would drop them, +// which for a pass whose only output is operator-facing defeats it. The outcome is +// reported even when the handler errors, so a boot that fails still gets the advisory. +// Nothing in either step refuses a boot the legacy path would have allowed. func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { - reportAdvisory(cmd) - return server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) + out := validateAdvisory(cmd) + err := server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) + reportAdvisory(out) + return err } -// reportAdvisory runs the advisory pass and reports what it found, containing a panic -// from either step. +// reportAdvisory logs an advisory outcome, containing a panic from the logging itself. // -// The recover has to cover the reporting and not just the pass. Everything here is -// advisory, so the one promise this manager makes is that it cannot refuse a boot the -// legacy path would have allowed, and a panic escaping the reporter would do exactly -// that by propagating out of Apply into PersistentPreRunE. Keeping the pass's own -// recover and adding none here would leave that hole open, since the pass returns -// normally and the reporter runs after it. -func reportAdvisory(cmd *cobra.Command) { +// Everything here is advisory, so the one promise this manager makes is that it cannot +// refuse a boot the legacy path would have allowed, and a panic escaping this reporter +// would do exactly that by propagating out of Apply into PersistentPreRunE. The pass that +// produced out has its own recover, so the remaining exposure is the log call, which the +// deferred recover below contains. logAdvisory is proven panic-free on every outcome, so +// this only fires for a logger broken independent of its arguments. +func reportAdvisory(out advisoryOutcome) { defer func() { - if recover() != nil { - // This runs after something already panicked, and the logger is itself a - // plausible cause (a broken handler, or a writer panicking on a closed fd), - // so guard the report: a second panic here must not escape the deferred - // func and refuse a boot the legacy path would have allowed. The message - // touches no dynamic value, so this only defends against a logger broken - // independent of its arguments. + if r := recover(); r != nil { + // A second panic, from logging the first, must not escape. The nested recover + // makes recording the recovered value and stack safe, and it is worth + // recording: a reporter that swallows its own failure blind is the case least + // debuggable from a node's logs. This mirrors what the pass captures for a + // panic in validateAdvisory. defer func() { _ = recover() }() - logger.Error("config validation reporting panicked (advisory; recovered, node will boot)") + logger.Error("config validation reporting panicked (advisory; recovered, node will boot)", + "panic", r, "stack", string(debug.Stack())) } }() - logAdvisory(validateAdvisory(cmd)) + logAdvisory(out) } // advisoryOutcome is what the validation pass saw. The pass reports rather than logs diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index b953a544bf..91b156a17f 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -336,14 +336,17 @@ func TestReadConfigFromDirMissingIsErrNotExist(t *testing.T) { func TestReportAdvisoryNeverEscapesWhenTheLoggerPanics(t *testing.T) { configtest.Isolate(t) // A config missing a required field, so the pass produces a diagnostic and - // logAdvisory reaches a logger call rather than the quiet fresh-node skip. + // logAdvisory reaches a logger call rather than the quiet fresh-node skip. The pass + // runs before the logger is swapped, matching production, where it runs before the + // handler applies the operator's level and only the reporting runs after. root := writeMinimalHome(t, "mode = \"full\"\n", "") + out := validateAdvisory(homeCmd(t, root)) orig := logger t.Cleanup(func() { logger = orig }) logger = slog.New(panickingHandler{}) - require.NotPanics(t, func() { reportAdvisory(homeCmd(t, root)) }) + require.NotPanics(t, func() { reportAdvisory(out) }) } // panickingHandler is a slog.Handler that panics on every record, standing in for a From 285be962e90420131e443dd73a9cfa3f6a99ecbc Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Thu, 30 Jul 2026 15:17:01 -0700 Subject: [PATCH 32/34] test(configmanager): silence cobra usage/error echo in the differential harness runManager's injected PreRunE returns a non-nil sentinel (errStopPreRun) on the happy path, which cobra's ExecuteC treats as a command error. SetErr(io.Discard) only covers the "Error:" line (the err writer); the ~50-line start usage block goes to the out writer and was dumping to the real process stderr on every call, multiplying under -fuzz. Set SilenceUsage and SilenceErrors so the harness swallows both, matching the comment's intent. --- cmd/seid/cmd/configmanager_differential_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 7a6bf5e350..85cfb38b52 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -109,7 +109,13 @@ func execConfigManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Command) (*server.Context, error) { t.Helper() template, appCfg := initAppConfig() - cmd.SetErr(io.Discard) // swallow cobra's own error echo; advisory logs go to seilog + // The happy-path PreRunE returns a non-nil sentinel (errStopPreRun), which cobra + // treats as a command error. SetErr covers the "Error:" line, but the ~50-line start + // usage block goes to the out writer, so silence usage and errors rather than dump it + // to stderr on every call (which multiplies under -fuzz). Advisory logs go to seilog. + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetErr(io.Discard) var applyErr error cmd.PreRunE = func(c *cobra.Command, _ []string) error { From 50436922b36f421d6f545a3683aac451def60527 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Fri, 31 Jul 2026 07:31:44 -0700 Subject: [PATCH 33/34] test(configmanager): assert Apply runs the validation pass, and stop swapping the logger Nothing asserted the one claim that separates v2 from the legacy manager. Deleting the two lines in Apply that run and report the validation pass left every test green: the parity rows compare two runs of the same reader, the never-refuses-boot tests only assert that a boot succeeds, and the advisory tests call validateAdvisory and reportAdvisory directly. TestApplyRunsTheValidationPass drives the real Apply and names the diagnostic the pass has to produce, so that deletion now fails. Reporting through a logger on the manager rather than the package variable is what lets that test read what Apply reported. The panic test took the same route, so no test reassigns the package logger any more, and a later parallel test in this package cannot race the swap. The nil case is the production path, since Select builds the zero value, so the accessor resolves nil to the package logger and the every-outcome table now runs through it: without that, nothing would construct the real logger and a nil one would ship silently, panicking on use and refusing the boot it is meant to allow. TestApplyPropagatesALegacyHandlerPanic covers the other direction, that v2 cannot turn a boot the legacy path refuses into one that succeeds. It also pins why the report is not deferred: deferring it is harmless while reportAdvisory's recover sits in a nested closure, but becomes a swallowed handler panic if that recover is ever hoisted into the function body, which is the idiom validateAdvisory already uses. Verified by making both edits and watching this test fail. Co-Authored-By: Claude Opus 4.8 --- cmd/seid/cmd/configmanager/configmanager.go | 51 ++++-- .../cmd/configmanager/configmanager_test.go | 155 ++++++++++++++++-- 2 files changed, 181 insertions(+), 25 deletions(-) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 021ed87c2b..eaac137068 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -3,6 +3,7 @@ package configmanager import ( "errors" "fmt" + "log/slog" "os" "path" "runtime/debug" @@ -43,7 +44,23 @@ func (LegacyConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate str // SeiConfigManager validates the config through the sei-config library, then // re-enters the legacy handler on the operator's original files. It never // writes, migrates, or refuses boot. -type SeiConfigManager struct{} +type SeiConfigManager struct { + // logger reports the advisory outcome, and a nil one means the package logger. + // Select and every other caller build the zero value, so the nil case is the + // production path rather than a fallback: the accessor below is what keeps it + // from being a nil dereference, which in Apply would refuse a boot the legacy + // path allowed. It exists so a test can read what Apply reported without + // reassigning package state that a parallel test could race. + logger *slog.Logger +} + +// log returns the logger to report through, and never returns nil. +func (m SeiConfigManager) log() *slog.Logger { + if m.logger != nil { + return m.logger + } + return logger +} // Apply validates the operator's config, re-enters the legacy handler on the original // files, then reports what the validation found. Validation runs before re-entry so it @@ -54,10 +71,18 @@ type SeiConfigManager struct{} // which for a pass whose only output is operator-facing defeats it. The outcome is // reported even when the handler errors, so a boot that fails still gets the advisory. // Nothing in either step refuses a boot the legacy path would have allowed. -func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { +// +// The report is deliberately not deferred. Deferring it as written would be harmless, +// because reportAdvisory's recover sits in a closure one frame too deep to see a panic +// raised here. It stops being harmless the moment that recover is hoisted into +// reportAdvisory's own body, which is a plausible edit given validateAdvisory below uses +// exactly that idiom: a deferred report would then recover a panic from the legacy +// handler and return nil, turning a boot the legacy path aborts into a successful one. +// TestApplyPropagatesALegacyHandlerPanic fails on that combination. +func (m SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { out := validateAdvisory(cmd) err := server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) - reportAdvisory(out) + reportAdvisory(m.log(), out) return err } @@ -69,7 +94,7 @@ func (SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string // produced out has its own recover, so the remaining exposure is the log call, which the // deferred recover below contains. logAdvisory is proven panic-free on every outcome, so // this only fires for a logger broken independent of its arguments. -func reportAdvisory(out advisoryOutcome) { +func reportAdvisory(lg *slog.Logger, out advisoryOutcome) { defer func() { if r := recover(); r != nil { // A second panic, from logging the first, must not escape. The nested recover @@ -78,11 +103,11 @@ func reportAdvisory(out advisoryOutcome) { // debuggable from a node's logs. This mirrors what the pass captures for a // panic in validateAdvisory. defer func() { _ = recover() }() - logger.Error("config validation reporting panicked (advisory; recovered, node will boot)", + lg.Error("config validation reporting panicked (advisory; recovered, node will boot)", "panic", r, "stack", string(debug.Stack())) } }() - logAdvisory(out) + logAdvisory(lg, out) } // advisoryOutcome is what the validation pass saw. The pass reports rather than logs @@ -204,19 +229,19 @@ func validateAdvisory(cmd *cobra.Command) (out advisoryOutcome) { const maxLoggedDiagnostics = 10 // logAdvisory reports an outcome through seilog. Nothing here refuses boot. -func logAdvisory(out advisoryOutcome) { +func logAdvisory(lg *slog.Logger, out advisoryOutcome) { switch { case out.Panic != nil: - logger.Error("config validation panicked (advisory; recovered, node will boot)", + lg.Error("config validation panicked (advisory; recovered, node will boot)", "panic", out.Panic, "stack", string(out.Stack)) case out.Stage == stageResolve: - logger.Warn("could not resolve home dir for config validation (advisory)", "error", out.Err) + lg.Warn("could not resolve home dir for config validation (advisory)", "error", out.Err) // Any other non-completing stage still reports, so a stage added without its own // case above logs something rather than nothing. The phrasing is neutral for the // same reason: naming a step here would misreport a stage added later, and the // stage attribute carries which one it actually was. case out.Stage != stageNone: - logger.Warn("config validation stopped early (advisory)", + lg.Warn("config validation stopped early (advisory)", "stage", out.Stage, "error", out.Err) // An unresolved home is closer to a misconfiguration than to a quiet default, and // it is reported so an operator who opted into v2 can tell a declined pass from a @@ -224,12 +249,12 @@ func logAdvisory(out advisoryOutcome) { // empty only when the home never resolved, and the missing-config skip below it // is the ordinary fresh-node case, which stays quiet. case out.Skipped && out.Home == "": - logger.Info("config validation skipped: no home dir resolved (advisory)") + lg.Info("config validation skipped: no home dir resolved (advisory)") // The pass completed and found nothing. Report at Info so an operator who opted into // v2 can see it ran clean, distinct from the quiet fresh-node skip (Skipped with a // home), which stays silent because the legacy handler is about to write those files. case !out.Skipped && out.Stage == stageNone && len(out.Diagnostics) == 0: - logger.Info("config validation passed: no advisories (node will boot)", "home", out.Home) + lg.Info("config validation passed: no advisories (node will boot)", "home", out.Home) } if len(out.Diagnostics) == 0 { @@ -239,7 +264,7 @@ func logAdvisory(out advisoryOutcome) { // The home is reported because a resolveHomeDir that drifted from the legacy // handler would have these diagnostics describe a directory the node is not // booting on, and without the path in the line there is no way to tell from a log. - logger.Warn("advisory config validation diagnostics (not enforced; node will boot)", + lg.Warn("advisory config validation diagnostics (not enforced; node will boot)", "home", out.Home, "count", len(out.Diagnostics), "diagnostics", shown, "omitted", omitted) } diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index 91b156a17f..eef90f4741 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "github.com/spf13/cobra" @@ -288,7 +289,16 @@ func TestCapDiagnostics(t *testing.T) { // asserts the reporting path survives each shape rather than asserting log text, since // the text is not a contract; a panic or nil dereference in the reporter would turn an // advisory pass into a boot failure, which is the one thing it must never do. +// +// It reports through the production logger, which is what the zero-value manager +// resolves to, so the every-outcome table covers the logger a node actually boots with. +// The other tests here inject their own, and without this one nothing would construct +// the package logger at all: a nil or broken one would ship unnoticed, and a nil +// *slog.Logger panics on use rather than discarding, so it would refuse a boot. func TestLogAdvisoryHandlesEveryOutcome(t *testing.T) { + lg := SeiConfigManager{}.log() + require.NotNil(t, lg, "the zero-value manager must resolve to a usable logger") + many := make([]string, maxLoggedDiagnostics+3) for i := range many { many[i] = fmt.Sprintf("[ERROR] field%d: broken", i) @@ -308,7 +318,7 @@ func TestLogAdvisoryHandlesEveryOutcome(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - require.NotPanics(t, func() { logAdvisory(tc.out) }) + require.NotPanics(t, func() { logAdvisory(lg, tc.out) }) }) } } @@ -330,23 +340,21 @@ func TestReadConfigFromDirMissingIsErrNotExist(t *testing.T) { // The logger is the subsystem this defends against. validateAdvisory recovers its own // panics (TestValidateAdvisoryReportsWhatItFound asserts a clean pass), and logAdvisory // is proven panic-free on every outcome (TestLogAdvisoryHandlesEveryOutcome), so the -// remaining exposure is a logger broken independent of its arguments. Swapping in a -// logger whose handler panics on every record makes both the reporting call and the -// recover handler's own log call panic; reportAdvisory must still return normally. +// remaining exposure is a logger broken independent of its arguments. A logger whose +// handler panics on every record makes both the reporting call and the recover handler's +// own log call panic; reportAdvisory must still return normally. +// +// The broken logger is passed in rather than assigned over the package one. Reassigning +// it would leave a future parallel test in this package racing the swap, and nothing here +// needs the package logger to change: the reporter takes the logger it reports through. func TestReportAdvisoryNeverEscapesWhenTheLoggerPanics(t *testing.T) { configtest.Isolate(t) // A config missing a required field, so the pass produces a diagnostic and - // logAdvisory reaches a logger call rather than the quiet fresh-node skip. The pass - // runs before the logger is swapped, matching production, where it runs before the - // handler applies the operator's level and only the reporting runs after. + // logAdvisory reaches a logger call rather than the quiet fresh-node skip. root := writeMinimalHome(t, "mode = \"full\"\n", "") out := validateAdvisory(homeCmd(t, root)) - orig := logger - t.Cleanup(func() { logger = orig }) - logger = slog.New(panickingHandler{}) - - require.NotPanics(t, func() { reportAdvisory(out) }) + require.NotPanics(t, func() { reportAdvisory(slog.New(panickingHandler{}), out) }) } // panickingHandler is a slog.Handler that panics on every record, standing in for a @@ -357,3 +365,126 @@ func (panickingHandler) Enabled(context.Context, slog.Level) bool { return true func (panickingHandler) Handle(context.Context, slog.Record) error { panic("logger handler is broken") } func (panickingHandler) WithAttrs([]slog.Attr) slog.Handler { return panickingHandler{} } func (panickingHandler) WithGroup(string) slog.Handler { return panickingHandler{} } + +// TestApplyRunsTheValidationPass asserts the one claim that distinguishes v2 from the +// legacy manager: that Apply actually runs the validation pass and reports it. +// +// Nothing else reaches this. The parity rows compare two runs of the same reader and +// still match if the pass never runs; the never-refuses-boot tests only assert that a +// boot succeeds; and the advisory tests call validateAdvisory and reportAdvisory +// directly. Deleting the two lines in Apply that call them makes v2 byte-identical to +// the legacy manager, and every one of those tests stays green. This one fails, because +// it drives the real Apply and names a finding the pass has to produce. +func TestApplyRunsTheValidationPass(t *testing.T) { + configtest.Isolate(t) + // A config missing a required field, so validation has a finding to report. app.toml + // is written as well: without it the read fails with ErrNotExist, the pass skips + // silently, and there would be nothing to distinguish that from the wiring being gone. + root := writeMinimalHome(t, "mode = \"full\"\n", "") + + // A real StartCmd, and one whose default home is non-empty. With an empty default an + // unresolved --home reports the no-home skip instead, so an assertion that merely + // counted records would hold even on a fixture that never reached the config. + cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) + require.NoError(t, cmd.Flags().Set(flags.FlagHome, root)) + serverCtx := &server.Context{} + cmd.SetContext(context.WithValue(context.Background(), server.ServerContextKey, serverCtx)) + + capture := &capturingHandler{} + require.NoError(t, SeiConfigManager{logger: slog.New(capture)}.Apply(cmd, + serverconfig.DefaultConfigTemplate, serverconfig.DefaultConfig()), + "the fixture is meant to boot, so a failure here means the fixture, not the pass") + + // Named by diagnostic rather than by record count, and it is the same finding + // TestValidateAdvisoryReportsWhatItFound anchors on, so a sei-config change moves + // both together instead of leaving this one asserting that something was logged. + var found *slog.Record + var seen []string + for i := range capture.records { + line := renderRecord(capture.records[i]) + seen = append(seen, line) + if strings.Contains(line, "chain.min_gas_prices") { + found = &capture.records[i] + } + } + require.NotNil(t, found, + "Apply reported no diagnostic naming chain.min_gas_prices, so the validation pass "+ + "is not wired into it; records seen: %v", seen) + require.Equal(t, slog.LevelWarn, found.Level, + "the diagnostic line is the operator-facing one and has to survive a node's level") + require.Contains(t, renderRecord(*found), "home="+root, + "the line has to name the directory validated, or a drifted resolve is invisible") +} + +// TestApplyPropagatesALegacyHandlerPanic asserts the abort direction of the manager's +// promise: v2 must not turn a boot the legacy path refuses into one that succeeds. +// +// Every other test asserts the permissive direction, that a boot legacy allows still +// happens. This one exists because the advisory path holds a recover, and the reporting +// call in Apply is deliberately not deferred for that reason. A deferred report whose +// recover had been hoisted into reportAdvisory's own body would swallow the handler's +// panic and return nil, which is exactly this assertion failing. +// +// The fixture drives a real panic rather than simulating one. A home under a +// non-writable parent is absent as far as the handler's stat is concerned, so it takes +// the fresh-node branch, and the directory creation inside that branch then fails on +// permissions, which the legacy handler raises as a panic rather than an error. +func TestApplyPropagatesALegacyHandlerPanic(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root, which ignores the directory permissions this relies on") + } + configtest.Isolate(t) + + readOnly := filepath.Join(t.TempDir(), "read-only") + require.NoError(t, os.MkdirAll(readOnly, 0o500)) + // Restored so the temp-dir cleanup can remove it. + t.Cleanup(func() { _ = os.Chmod(readOnly, 0o700) }) + + cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) + require.NoError(t, cmd.Flags().Set(flags.FlagHome, filepath.Join(readOnly, "node"))) + serverCtx := &server.Context{} + cmd.SetContext(context.WithValue(context.Background(), server.ServerContextKey, serverCtx)) + + require.Panics(t, func() { + _ = SeiConfigManager{logger: slog.New(&capturingHandler{})}.Apply(cmd, + serverconfig.DefaultConfigTemplate, serverconfig.DefaultConfig()) + }, "a panic from the legacy handler must reach the caller rather than being recovered "+ + "into a successful boot") +} + +// capturingHandler records what was logged so a test can assert on it without +// reassigning the package logger. +type capturingHandler struct { + mu sync.Mutex + records []slog.Record +} + +// Enabled admits every level, so a record cannot be dropped by the level the legacy +// handler applies partway through Apply and leave an assertion looking at nothing. +func (h *capturingHandler) Enabled(context.Context, slog.Level) bool { return true } + +func (h *capturingHandler) Handle(_ context.Context, r slog.Record) error { + h.mu.Lock() + defer h.mu.Unlock() + // Cloned because slog only lends a record for the duration of the call. + h.records = append(h.records, r.Clone()) + return nil +} + +func (h *capturingHandler) WithAttrs([]slog.Attr) slog.Handler { return h } +func (h *capturingHandler) WithGroup(string) slog.Handler { return h } + +// renderRecord flattens a record's message and attributes into one line, so an +// assertion can name a value without knowing which attribute carries it. +func renderRecord(r slog.Record) string { + var b strings.Builder + b.WriteString(r.Message) + r.Attrs(func(a slog.Attr) bool { + b.WriteString(" ") + b.WriteString(a.Key) + b.WriteString("=") + b.WriteString(a.Value.String()) + return true + }) + return b.String() +} From e258712629677413acda1f1e939e22a981c41e74 Mon Sep 17 00:00:00 2001 From: FromTheRain Date: Fri, 31 Jul 2026 08:01:02 -0700 Subject: [PATCH 34/34] docs(configmanager): warn against hoisting the reporter's recover at the recover itself The reason the advisory report is not deferred lives at the call site in Apply, but the edit it guards against would be made in reportAdvisory. Put the tripwire there too: the recover has to stay in its closure, because in the function body it would see a caller's panic and a deferred report would then boot a node the legacy path refuses. --- cmd/seid/cmd/configmanager/configmanager.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index eaac137068..c6e6eec499 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -95,6 +95,11 @@ func (m SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate stri // deferred recover below contains. logAdvisory is proven panic-free on every outcome, so // this only fires for a logger broken independent of its arguments. func reportAdvisory(lg *slog.Logger, out advisoryOutcome) { + // The recover below stays inside this closure. Hoisted into reportAdvisory's own body + // it would see a panic unwinding a caller's frame whenever this function is deferred, + // so a deferred report in Apply would recover the legacy handler's panic and boot a + // node the legacy path refuses. TestApplyPropagatesALegacyHandlerPanic fails on that + // pair, and neither half fails on its own. defer func() { if r := recover(); r != nil { // A second panic, from logging the first, must not escape. The nested recover