- Repo: gosharplite/tell-me-go (branch
dev@752b4785, v1.9.0) - Subject: architect — Grilled on diagnosis + plan + issue premise
- Griller: griller — 4 questions, early conclusion after Q4
- Date: 2026-08-19
- Verdict: PROCEED WITH CHANGES
SUBJECT POSITION — Issue #1406 (MCP stdio: Viper lowercases MCP_SERVERS.*.ENV map keys)
I have completed the full bootstrap (README, Makefile, domain models, environments docs, INTENTIONAL_NON_FIXES.md, issue fetch, and verification of every cited line against the live tree at /home/pos/tmp/github/gosharplite/tell-me-go, branch dev @ 752b4785). The live config evidence is also confirmed in the workspace configs/butler.yaml:117-127 (ENV: {PLUR_TOOL_PROFILE: "full"} under plur:), and my own session shows the lean-profile symptom (plur_inject_hybrid "exists but is not directly callable").
(a) Diagnosis — correct and complete. I verified every claim against the pinned dependency: go.mod:17 pins github.com/spf13/viper v1.21.0, and both mangling sites are real. insensitiviseMap ($GOMODCACHE/github.com/spf13/viper@v1.21.0/util.go:89-100) recurses into nested maps and rewrites every non-lowercase key via delete(m, key) + m[lower] = val — so the ENV map's PLUR_TOOL_PROFILE key becomes plur_tool_profile immediately after the YAML parse. getSettings (viper.go:1970-1987, lastKey := strings.ToLower(path[len(path)-1]) at ~:1985) lowercases the leaf again during Unmarshal, which in v1.21.0 runs decode(v.getSettings(v.AllKeys()), ...) (viper.go:953). The cited repo seam is exact: v.Unmarshal(cfg, ...) at internal/infrastructure/config/config.go:181, expandEnvHook at :198, and sortedEnvPairs at internal/infrastructure/mcp/stdio_client.go:354-365 passes keys verbatim with cmd.Env = append(os.Environ(), ...) at :77 (issue cites :80-90 — that's the comment block, immaterial drift). Viper is the only mangler, and Env is the only case-sensitive consumer of any config map. The other map[string]string field is LLMProvider.Headers (internal/domain/config/config.go:62), which flows to openai.WithHeaders/anthropic.WithHeaders/gemini.WithHeaders (internal/infrastructure/llm/factory.go:51,76,87) — HTTP field names are case-insensitive (RFC 9110 §5.1), and the README's own HEADERS: reasoning_effort: "max" example relies on exactly that. PROVIDERS/MODELS keys are lowercased but matched case-insensitively within the same config (identical mangling on both sides), and FAILOVER_ORDER is a value list. So ENV is uniquely broken: its keys are opaque strings handed to execve, where Linux and macOS environments are case-sensitive. One precision correction to the issue: the "decode hook" half of option 2 is infeasible — mapstructure DecodeHooks transform values (f.Kind() == reflect.String guards in expandEnvHook), never map keys; there is no hook point for key casing. Only the post-load correction pass survives as option 2.
(b) Blast radius. Confirmed: every stdio server with an ENV block is silently affected; remote (URL) servers are exempt both by protocol (no ENV — mcp_config.go validation rejects ARGS/DIR/ENV without COMMAND) and by code (Env is consumed only by NewStdioClient). It is silent — no validation, lint, or log fires; the child starts and degrades only where a server reads a case-sensitive variable. The live impact is exactly as the issue states: with MEMORY: {ENABLED: true, SERVER: "plur"} (workspace butler.yaml:155-157), the ADR-068 pipeline's plurInjector (priority 15) silently degrades to the lean surface. Two additions: (1) on Windows the bug is a no-op — env vars are case-insensitive there — so the fix is Linux/macOS-critical only, but must not regress Windows; (2) the issue's workaround is sound and I verified why: plur_tool_profile=full (config) and PLUR_TOOL_PROFILE=full (shell-exported, inherited via os.Environ()) are distinct names on case-sensitive systems, so the exported uppercase var survives — the "last-wins" comment at stdio_client.go:75-77 applies only to same-name duplicates. This is distinct from #1398/#1377 (wire protocol), agreeing with the issue.
(c) Proposed fix — the config layer is the only correct seam. The invariant "keys in MCPServerConfig.Env must reach the child byte-for-byte" cannot be enforced at the MCP layer — stdio_client.go receives a decoded struct and the original case is unrecoverable there. The raw YAML bytes exist only at load time, so the correction belongs in internal/infrastructure/config/config.go's load(): after unmarshalConfig(v, &cfg) (line ~181) and before ValidateMCPServers(), run a small helper — restoreMCPServerEnvCase(raw []byte, cfg *domain_config.Config) — that: (1) parses the raw YAML case-preservingly with gopkg.in/yaml.v3 (already a direct dependency, go.mod:33 — zero new deps); (2) walks MCP_SERVERS.<name>.ENV; (3) rebuilds each cfg.MCPServers[name].Env map as originalCaseKey → decodedValue, matching decoded keys to raw keys case-insensitively (strings.EqualFold) and falling back to the decoded key for entries absent from raw YAML (e.g. TELL_ME_* env overrides, which stay lowercase — a documented, acceptable edge). Critical subtlety: the helper must preserve the decoded values, not re-read raw values — the decoded map already passed through expandEnvHook (${VAR} expansion, config.go:198) and mapstructure's weak typing, so overwriting with raw scalar text would regress TestLoad_MCPStdio_EnvExpansion's expansion guarantee. The placement inside load() also means the watcher hot-reload path (Refresh → Loader.Load → load, ADR-029 §5) gets the fix for free. This implements the issue's recommended option 1. Belt-and-braces: leave sortedEnvPairs and the spawn path untouched (their behavior is correct); I'd add only a doc comment on MCPServerConfig.Env (internal/domain/config/mcp_config.go:45) pinning the byte-for-byte invariant, per the domain-model-context discipline.
(d) Test strategy. The primary regression test belongs in internal/infrastructure/config/config_test.go following the established TestLoad_* pattern (temp YAML → load(path)): TestLoad_MCPStdio_EnvKeyCasePreserved — YAML with ENV: {PLUR_TOOL_PROFILE: "full", LOWER_KEY: "v"} (mixed case) asserting cfg.MCPServers["plur"].Env["PLUR_TOOL_PROFILE"] == "full" and that no lowercased-only variant survives (_, ok := Env["plur_tool_profile"]; !ok). This fails on current code — proven by the fact that the existing TestLoad_MCPStdio_EnvExpansion (config_test.go:1224) contains the workaround comment "Viper lowercases nested map keys, so assert on the value rather than the key casing" and iterates values instead of asserting fs.Env["PATH"]. That test's comment is a smoking gun: the bug was known and deliberately worked around when #1396 landed. After the fix, tighten it to assert fs.Env["PATH"] == "/custom/bin" directly, converting the workaround into a true invariant pin. TestLoad_ParsedDump_MCPStdioRedaction (config_test.go:1176) stays green — it asserts on Viper's debug dump (mcp_servers::fs::env), which remains lowercase since the dump reflects Viper's view, not the corrected struct. TestSortedEnvPairs (stdio_client_test.go:228) must stay untouched — agreed, its behavior is correct. Optional secondary: an end-to-end spawn test asserting the child's captured env contains PLUR_TOOL_PROFILE=full (the integration suite already has the /proc/<pid>/environ pattern in TestStdio_LauncherTreePassThrough); I'd treat this as valuable but non-blocking, since the decode-path test guards the actual failure seam.
(e) Corrections and disagreements. (1) The decode-hook variant of option 2 is not implementable (mapstructure hooks cannot touch map keys) — the issue should be corrected to say "post-load correction pass" only. (2) The issue understates the smoking-gun evidence: TestLoad_MCPStdio_EnvExpansion's own comment proves the mangling was known and papered over at feature time — the fix should explicitly flip that test from workaround to invariant. (3) "Two independent sites" is precisely right (insensitiviseMap + getSettings); AllKeys() is downstream of the first, not a third independent mangle. (4) Blast radius should note the Windows no-op and macOS inclusion. (5) I disagree mildly with "no ADR expected": this is a deliberate anti-Viper measure that a future viper minor bump could silently re-break (the correction pass is defensive against the dependency's documented behavior). A short ADR (or an ADR-067/ADR-068 lineage note) pinning the "ENV keys reach the child byte-for-byte" invariant, plus the domain-model invariant addition, is warranted — the project's ADR index gate makes the record cheap and the protection durable. Everything else — root cause, option-1 recommendation, verification plan, workaround — I endorse as written.
Your position contains an internal contradiction that decides the entire fix shape, and I want it resolved before we walk anything downstream.
You state in (e)(1): "the 'decode hook' half of option 2 is infeasible… only the post-load correction pass survives as option 2." Then in (c) you claim your restoreMCPServerEnvCase helper — parse raw YAML, walk cfg.MCPServers, match decoded keys to raw keys via strings.EqualFold, rebuild each Env map — "implements the issue's recommended option 1." It doesn't, per the issue's own taxonomy: option 1 is the case-preserving re-decode of the MCP_SERVERS subtree (the ENV keys never pass through viper's lowercasing at all — a bypass seam), while option 2 is literally defined as "walk cfg.MCPServers and restore original-case keys from the raw YAML document" — which is precisely your mechanism. You defended rejecting option 1's naive form (raw values would bypass expandEnvHook's ${VAR} expansion and weak typing), but you never weighed the bypass seam done correctly: decode the ENV block case-preservingly from raw YAML and run os.ExpandEnv on the values yourself — the guarantee you cite as the blocker is trivially re-attachable, and the whole repair machinery disappears.
So the question is the seam, and it has a hard edge case your plan must survive: your correction pass restores case by matching decoded keys to raw keys case-insensitively — but insensitiviseMap collapses two raw keys differing only in case (e.g. Path and PATH) into one decoded key, with a nondeterministic winner (its delete-then-reinsert iterates a Go map in random order). Your helper cannot know which raw key's value won, so it restores an arbitrary one of the two original casings and the other variable silently dies — a direct violation of the "byte-for-byte" invariant you state as the goal, and it makes the fix nondeterministic for a config an operator can plausibly write.
Which seam is the actual plan — repair-after-decode (option 2) or a case-preserving bypass that keeps expansion (option 1, done right)? If you hold to repair-after-decode, reconcile the case-collision hole: what does the helper do when two raw keys map to one decoded key — pick deterministically (which rule?), reject the config, or leave it undefined?
Verdict: the plan is option 1 — the case-preserving bypass, done right — with expansion re-attached. You are correct on both counts: I mislabeled my own mechanism, and the mechanism as written is unsound.
Conceding the taxonomy point first, because it is the substantive one. My (c) helper — parse raw YAML, walk cfg.MCPServers, match decoded keys to raw keys via EqualFold, rebuild each Env map from decoded values — is literally the issue's option 2: repair-after-decode. I labeled it option 1. And in (e)(1) I rejected the bypass for the naive reason (raw values would skip ${VAR} expansion) without considering that expandEnvHook is a one-line os.ExpandEnv(data.(string)) (config.go:198-203) — re-attaching it to raw values is not "trivially possible," it is the same function call. The repair machinery was never necessary. Correct concession: my plan was option 2 mislabeled, and option 2 is not merely less clean — it is structurally incapable of satisfying the invariant, as your edge case proves.
Why the seam is forced, not preferred. insensitiviseMap (viper@v1.21.0/util.go:89-100) iterates a Go map in random order; for raw keys Path and PATH it deletes each and re-inserts under path, so exactly one value survives and which one is nondeterministic. The decoded Env map therefore holds {"path": arbitraryWinner}. My repair pass then does EqualFold("path") against the raw dictionary — which matches both raw keys — and restores an arbitrary casing onto an arbitrary value. Two independent nondeterminisms, one silent dead variable. "Pick deterministically" cannot fix this: the loser's value is already destroyed by Viper; no rule over casings can recover it. The byte-for-byte invariant therefore cannot be enforced at a repair seam by construction — enforcement must happen before the lossy transform, i.e. Viper must never see the ENV leaf keys. That is the bypass. This is not a preference between two viable seams; the invariant dictates the seam.
The bypass, concretely. internal/infrastructure/config/config.go: configureViper gains a raw []byte return (it already reads the bytes inside readConfigFile; thread them out — one read, no TOCTOU between Viper's read and the helper's). New unexported helper applyCasePreservingMCPServerEnv(raw []byte, cfg *domain_config.Config) error, called in load() immediately after unmarshalConfig(v, &cfg) (:181) and before ValidateMCPServers(). It: (1) yaml.Unmarshal(raw, &doc) with gopkg.in/yaml.v3 (already direct, go.mod:33); (2) navigates case-insensitively at the structural levels (MCP_SERVERS, per-server ENV — operators may write either case there, Viper accepts both) but byte-for-byte at the ENV leaf keys; (3) for each server in the decoded cfg.MCPServers, rebuilds Env as rawKey → os.ExpandEnv(scalarString(rawValue)), where scalarString renders non-string scalars via fmt.Sprint (FOO: 123 → "123", BAR: true → "true") to match Viper's weak-typing parity; (4) supplements: decoded keys with no raw counterpart (the exotic TELL_ME_MCP_SERVERS_*_ENV_* AutomaticEnv path) are appended with their decoded casing — raw YAML wins on conflict, deterministic, zero behavioral loss vs. today. Exact-duplicate raw keys (PATH: a, PATH: b) error in yaml.v3 exactly as they error in Viper's own parser — no behavior change. The Viper-decoded ENV map is discarded wholesale, so Viper's collapse never has a chance to matter. Cost: one extra YAML parse of the config at load — startup-only, negligible; the watcher hot-reload path re-enters load() so it inherits the fix.
Tests — the collision case is the seam-forcing regression. (1) TestLoad_MCPStdio_EnvKeyCasePreserved in internal/infrastructure/config/config_test.go (existing TestLoad_* temp-YAML pattern): fixture with ENV: {PLUR_TOOL_PROFILE: "full", Path: "a", PATH: "b"}; assert Env["PLUR_TOOL_PROFILE"]=="full" and Env["Path"]=="a" && Env["PATH"]=="b" — both survivors, byte-for-byte. This test fails on current code, fails the repair seam nondeterministically, passes the bypass deterministically. It is the test that makes the seam choice enforced rather than asserted. (2) Tighten TestLoad_MCPStdio_EnvExpansion (config_test.go:1224): replace its value-iteration workaround loop with a direct fs.Env["PATH"] == "/custom/bin" — converting the comment-documented workaround into an invariant pin. (3) TestLoad_ParsedDump_MCPStdioRedaction stays green untouched (it asserts Viper's debug dump, which remains lowercase — the dump reflects Viper's view, not the corrected struct). (4) TestSortedEnvPairs untouched — agreed, correct upstream. Optional secondary: an integration spawn asserting the child's captured env (the TestStdio_LauncherTreePassThrough /proc/<pid>/environ pattern) — valuable, non-blocking.
To your hypothetical, since you asked it straight: if the plan were held to repair-after-decode, the only invariant-preserving rule is reject the config — deterministic error naming both keys (MCP_SERVERS.<name>.ENV keys differing only in case are ambiguous: "Path", "PATH") — because "pick deterministically" is unsound (the loser's value is unrecoverable, so a deterministic casing rule still yields an arbitrary value). But reject is semantically wrong: Path and PATH are legal, distinct Linux environment variables, and a correct system passes both through — which is exactly what the bypass does and the repair cannot. That asymmetry is the argument in one line: the repair seam's only sound outcome is rejecting a config the bypass handles correctly.
One residual, stated for the record rather than hidden: the supplement merge in (4) is the only piece of the bypass that still touches the Viper-decoded map, and only for keys Viper invented (env overrides). It is deterministic and cannot reintroduce the collision — a raw key always wins over a case-colliding override key. I recommend keeping it for zero-regression; if you judge the AutomaticEnv-into-ENV path unsupportable, dropping it removes even that touch. The core seam — bypass, not repair — is not contingent on that choice.
Your concession is right, and your seam theorem — enforcement must precede the lossy transform — is now the standard the plan must meet everywhere. It doesn't yet, and the failing spot is the one you yourself flagged as residual.
Your supplement rule in (4) — "decoded keys with no raw counterpart are appended with their decoded casing; raw YAML wins on conflict" — is built on a misreading of the TELL_ME_MCP_SERVERS_*_ENV_* path, and the misreading inverts a precedence your own repo pins with tests.
The mechanics, verified: Unmarshal asks only for v.AllKeys() keys (viper.go, keys := v.AllKeys(); getSettings iterates them). AutomaticEnv alone cannot surface a key Unmarshal never asks for — your repo's own TestLoad_WrapWidth_EnvOnly comment states this verbatim: "AutomaticEnv alone cannot surface a key that Unmarshal never asks for (viper.AllKeys() omits it)" — which is why WRAP_WIDTH needs an explicit BindEnv. configureViper (config.go:136-140) registers BindEnv only for MODE/PERSON/AIMODEL/AIURL/WRAP_WIDTH — nothing under mcp_servers::*::env::*. Therefore a TELL_ME_MCP_SERVERS_FS_ENV_* variable can only take effect as a value override on a key that already exists in the YAML — the ENV subtree has no key that is "decoded with no raw counterpart." Your supplement branch is dead code; the only live manifestation of the env path is the exact conflict case where you rule "raw YAML wins."
And that rule is backwards. Viper's precedence is env-over-config (find() checks getEnv before the config map), and the repo pins it: TestLoad_ProviderMaxTokens_EnvOverride (TELL_ME_PROVIDERS_GOOGLE_MAX_TOKENS=8000 beats YAML 32000), TestLoad_WrapWidth_EnvOverride, TestLoad_ParsedDump_EnvOverriddenValues. Apply that to the ENV subtree: YAML ENV: {FOO: "yaml"} + exported TELL_ME_MCP_SERVERS_FS_ENV_FOO=env — today the child gets foo=env; under your rule it gets FOO=yaml, with the override silently dropped. That is a behavioral regression on the only path your supplement exists to serve, and it falsifies your "zero behavioral loss vs. today" claim.
So: how does the bypass preserve env-wins precedence for the ENV subtree while discarding the viper-decoded map wholesale — detect the override by comparing decoded value against raw-expanded value (which resurrects exactly the "touching the decoded map" you wanted to eliminate), or declare TELL_ME_*_ENV_* overrides unsupported and delete the supplement branch entirely (per your own "dropping it removes even that touch")? Pick one, and specify the precedence rule for the ENV subtree in the config documentation either way — because right now the plan silently changes a documented, test-pinned semantic.
Answer: neither of the two offered options — the dichotomy is false. Preserving env-wins does not require value-comparison against the decoded map, and dropping the override is unnecessary: resolve each raw ENV leaf through v.Get() and let Viper's own env precedence do the work. That keeps env-wins exactly as today, byte-for-byte keys, zero decoded-map touch, and no hand-rolled env-name construction to drift.
Your mechanics are verified, every citation: the quote is at config_test.go:1032-1033 ("AutomaticEnv alone cannot surface a key that Unmarshal never asks for (viper.AllKeys() omits it)"); TestLoad_ProviderMaxTokens_EnvOverride (:655-688) pins TELL_ME_PROVIDERS_GOOGLE_MAX_TOKENS=8000 beating YAML 32000; TestLoad_ParsedDump_EnvOverriddenValues (:1084-1088) sets TELL_ME_MCP_SERVERS_ATLASSIAN_TOKEN/URL — the MCP override family is live, and viper's find() checks env before the config map (viper.go:1227-1234) with the name uppercased via mergeWithEnvPrefix (viper.go:419-424). And your dead-code proof is airtight: Unmarshal iterates AllKeys(), which is bound-keys ∪ file-keys; configureViper (config.go:137-141) binds only MODE/PERSON/AIMODEL/AIURL/WRAP_WIDTH; nothing under mcp_servers::*::env::*; therefore every decoded ENV key has a raw counterpart, the supplement branch can never fire, and the only live env-path manifestation is the conflict case where I had wrongly ruled "raw YAML wins." Concession in full: my rule inverted viper's precedence, silently dropping a live override — a real regression, and my "zero behavioral loss" claim is falsified. The supplement branch is deleted, not repaired.
The algorithm that replaces both options. In internal/infrastructure/config/config.go, applyCasePreservingMCPServerEnv(v *viper.Viper, raw []byte, cfg *domain_config.Config) error, called in load() after unmarshalConfig (:181) and before ValidateMCPServers(): (1) yaml.v3 parse of raw (already a direct dep, go.mod:33); (2) case-insensitive structural nav to MCP_SERVERS.<name>.ENV; (3) for each raw leaf key k: resolved := v.Get("mcp_servers::" + name + "::env::" + strings.ToLower(k)) — this single call resolves TELL_ME_MCP_SERVERS_<NAME>_ENV_<K> first (viper's own name algorithm, uppercased per mergeWithEnvPrefix, so no reimplementation and no drift) and falls back to the file value; (4) env[k] = os.ExpandEnv(scalarString(resolved)) — expansion applied to whichever source won, exactly matching today's expandEnvHook (config.go:198-203), which fires on the final decoded value regardless of origin; (5) stamp cfg.MCPServers[name].Env = env. Env-wins falls out of viper's own find() — the test-pinned behavior — while the key is written byte-for-byte. The decoded Viper map is never read for ENV, so your "resurrects touching the decoded map" objection is avoided without declaring anything unsupported. One viper call per leaf; startup-only cost.
Precedence rule, specified for the docs (README MCP_SERVERS section, the Env field comment at mcp_config.go:45, and the ADR note): (a) TELL_ME_MCP_SERVERS_<NAME>_ENV_<KEY> overrides the YAML ENV value for a declared key — matching viper's global env-over-config precedence; (b) keys reach the child byte-for-byte regardless of source; (c) overrides cannot invent undeclared variables — AllKeys() omits keys Unravel never asks for (config_test.go:1032-1033), so an exported override without a YAML counterpart is inert, as it is today; (d) case-differing YAML keys (Path, PATH) are distinct child variables, each keeping its own value, and an exported override for the shared name applies to all casings of that name (both become the env value) — deterministic, documented, and the only rule that preserves both keys under env-wins. If the maintainers prefer loud failure there, reject-on-collision-with-override is the conservative alternative; my recommendation is apply-to-all-casings, because it keeps the Q1 byte-for-byte invariant intact in every combination.
Tests — the precedence gap becomes pinned, not emergent. (1) TestLoad_MCPStdio_EnvKeyCasePreserved: YAML ENV: {PLUR_TOOL_PROFILE: "full", Path: "a", PATH: "b"} → assert all three keys byte-for-byte with their own values — the seam-forcing test from Q1. (2) TestLoad_MCPStdio_EnvOverrideWins: YAML ENV: {FOO: "yaml"} + t.Setenv("TELL_ME_MCP_SERVERS_FS_ENV_FOO", "env") → assert Env["FOO"] == "env" — this pins the precedence for the ENV subtree through the real decode path, closing exactly the gap you identified (it becomes a test-pinned semantic, matching the ProviderMaxTokens_EnvOverride family). (3) Tighten TestLoad_MCPStdio_EnvExpansion (:1224) to assert fs.Env["PATH"] == "/custom/bin" directly. (4) TestLoad_ParsedDump_EnvOverriddenValues and TestSortedEnvPairs stay green untouched. If you accept (2) and it passes, the "documented, test-pinned" contract now includes the ENV subtree rather than merely implying it.
One residual honesty note: this design leans on viper's Get resolving the env name for a ::-delimited leaf path — the mechanism the repo already exercises for PROVIDERS through the identical configureViper instance, and test (2) pins it end-to-end, so any viper-internal drift fails loudly in our test rather than silently diverging. The seam question from Q1 is now fully closed: bypass on keys, viper's own precedence on values, both deterministic.
Your algorithm fails your own seam-forcing test, and the failure is the Q1 theorem re-violated on the value side.
Trace it: raw ENV: {Path: "a", PATH: "b"}. insensitiviseMap (util.go:89-100) collapses both to one key path whose value is a nondeterministic winner ("a" or "b", Go map iteration order). Your step (3) then resolves both raw keys through the identical viper path — v.Get("mcp_servers::" + name + "::env::" + strings.ToLower("Path")) and ...::env::" + strings.ToLower("PATH") are the same string — so both Path and PATH receive the same winner value. Env["Path"]=="a" && Env["PATH"]=="b" — the assertion in your own test (1), the test you built to force the seam — is unachievable under this algorithm. And it fails nondeterministically, the exact property you (correctly) refused to tolerate in Q1.
The root is structural: v.Get is a single-value lookup into a map that has already lost the distinction. It cannot return two values for one collapsed key, and it cannot tell you whether the value it returns came from the env override or the config file — so it is simultaneously (a) unable to preserve per-key values and (b) the only mechanism in your design that delivers env-wins. Your Q1 theorem — "the loser's value is already destroyed by Viper; no rule over casings can recover it; enforcement must happen before the lossy transform" — applies verbatim to your own step (3): you bypassed the transform on keys but re-admitted it as the authority on values. "Bypass on keys, viper's own precedence on values" is only half a bypass, and the half that isn't bypassed is the half that destroys data.
So the four constraints you want — (a) per-key byte-for-byte values for case-colliding keys, (b) env-wins precedence, (c) no hand-rolled env-name construction, (d) no touch of the viper-decoded map — are mutually unsatisfiable through viper's public API. One must give. The coherent resolution, for the record: take values from the raw YAML per raw key (satisfying (a)), and handle env-wins with os.LookupEnv on names constructed from your own replacer config — config.go:132 builds strings.NewReplacer("::", "_", "-", "_") and config.go:130 sets the TELL_ME_ prefix; that's app-owned configuration, not viper internals, and test (2) pins the construction end-to-end — with the override applied to all casings per your rule (d). That keeps test (1) and env-wins simultaneously, deterministically.
Which do you choose: adopt the raw-value + app-owned env-name construction (giving up (c)), or drop env-wins for the ENV subtree entirely — or do you believe v.Get can somehow return two distinct values for one collapsed key? And while you're specifying the value contract: ENV: {FOO:} (null) yields fmt.Sprint(nil) == "<nil>" under your scalarString, where today mapstructure produces "" — your "exactly matching today's expandEnvHook" is false in that corner too; what is scalarString's null rule?
I adopt the raw-value + app-owned env-name construction, and I concede (c). And to your direct question: no — v.Get cannot return two distinct values for one collapsed key. The map is collapsed before Get runs; Get is single-valued. My Q2 design was the Q1 theorem re-violated on the value side, exactly as you traced: I bypassed the transform on keys and then re-admitted the collapsed map as the authority on values — the half that wasn't bypassed is the half that destroys data. The bypass must be total: raw YAML is the sole authority for the ENV subtree's keys and values.
Your trace, verified line by line: insensitiviseMap (util.go:89-100) collapses Path/PATH to one path with a map-iteration-order winner; both my v.Get("mcp_servers::fs::env::path") calls are the same string; both keys receive the same winner; Env["Path"]=="a" && Env["PATH"]=="b" is unachievable and the failure is nondeterministic — the property I refused in Q1, now produced by my own Q2 algorithm. The structural root is exactly as you state: v.Get is a single-value lookup into a structure that has lost the distinction, and it cannot report its source (env vs file) — so it is simultaneously unable to deliver per-key values and the sole carrier of env-wins. That conjunction is the proof that (a)+(b)+(c)+(d) is unsatisfiable through viper's public API. Conceded in full.
The algorithm, with the value contract made exact. internal/infrastructure/config/config.go:
// envNameForMCPServerLeaf builds the TELL_ME_* name Viper's AutomaticEnv
// resolves for MCP_SERVERS.<name>.ENV.<leaf>, mirroring Viper's two steps —
// mergeWithEnvPrefix (ToUpper of prefix+"_"+path) then envKeyReplacer —
// from the app-owned configuration in configureViper (config.go:130,132).
// Lockstep is pinned end-to-end by TestLoad_MCPStdio_EnvOverrideWins.
func envNameForMCPServerLeaf(name, leaf string) string {
key := "TELL_ME_" + strings.ToUpper("mcp_servers::"+name+"::env::"+strings.ToLower(leaf))
return strings.NewReplacer("::", "_", "-", "_").Replace(key)
}
// scalarString renders a raw-YAML scalar as its config-string form, matching
// mapstructure's weak-typing parity: nil → "" (zero value), string → as-is,
// int/bool/float → fmt.Sprint (123 → "123", true → "true").
func scalarString(v any) string {
switch x := v.(type) {
case nil:
return ""
case string:
return x
default:
return fmt.Sprint(x)
}
}applyCasePreservingMCPServerEnv(raw []byte, cfg *domain_config.Config) error, called in load() after unmarshalConfig and before ValidateMCPServers(): yaml.v3 parse; case-insensitive structural nav to MCP_SERVERS.<name>.ENV; per raw leaf key k:
name := envNameForMCPServerLeaf(serverName, k)
if override, ok := os.LookupEnv(name); ok && override != "" {
env[k] = os.ExpandEnv(override) // env-wins; empty treated as unset (below)
} else {
env[k] = os.ExpandEnv(scalarString(rv)) // raw YAML value, byte-for-byte key
}The null rule: nil → "", not "<nil>" — your corner is conceded, fmt.Sprint(nil) was wrong; scalarString above returns "" for nil, matching mapstructure's zero-value decode, so ENV: {FOO:} yields Env["FOO"]=="" exactly as today. And the sibling corner you didn't flag, which parity also requires: viper's getEnv returns val, ok && (v.allowEmptyEnv || val != "") (viper.go:442-450) — with AllowEmptyEnv unset (config.go never calls it, default false), an empty override is treated as unset and the file value wins. My guard ok && override != "" replicates that: TELL_ME_MCP_SERVERS_FS_ENV_FOO="" does not override YAML FOO: "yaml", exactly as today. And os.ExpandEnv is applied to both branches because today's expandEnvHook fires on the final decoded value regardless of source — so an override containing ${BAR} expands today, and does under this design. That closes the "exactly matching today's expandEnvHook" claim for real: null, empty-override, and expansion all specified.
On the reframing of (c): you are right that this is not "viper internals" — config.go:130 (SetEnvPrefix("TELL_ME")) and :132 (SetEnvKeyReplacer(strings.NewReplacer("::", "_", "-", "_"))) are our declared configuration; viper merely executes it. Constructing the name from our own contract is not reaching into viper's guts — and test (2) pins the construction end-to-end through load() and viper's real Unmarshal, so if viper ever applies our config differently, the test fails loudly rather than diverging silently. I'll also add a comment at config.go:130-132 marking the prefix/replacer as a shared contract consumed by both viper and the ENV helper, so the lockstep is visible at the declaration site.
Tests. (1) TestLoad_MCPStdio_EnvKeyCasePreserved: ENV: {PLUR_TOOL_PROFILE: "full", Path: "a", PATH: "b", Null: } → assert "full", Path=="a", PATH=="b" (both byte-for-byte, deterministic — the assertion your trace proved unachievable under Q2's algorithm is now achievable), and Env["Null"]=="". Fails pre-fix, passes post-fix. (2) TestLoad_MCPStdio_EnvOverrideWins: YAML ENV: {FOO: "yaml"} + t.Setenv("TELL_ME_MCP_SERVERS_FS_ENV_FOO", "env") → Env["FOO"]=="env" — pins construction + precedence, and fails pre-fix on the key casing (foo=env today), so it is a real regression test for the combined fix. Add two subtests: empty override ("" → file value wins, viper AllowEmptyEnv parity) and case-collision + override (Path/PATH both receive the override value — rule (d), deterministic). (3) Tighten TestLoad_MCPStdio_EnvExpansion to fs.Env["PATH"]=="/custom/bin". (4) TestLoad_ParsedDump_EnvOverriddenValues, TestSortedEnvPairs untouched.
The four-constraint verdict, for the record: (a) satisfied — values come from raw YAML per raw key; (b) satisfied — os.LookupEnv on app-owned names with viper's own precedence (env-before-file) and empty-var semantics replicated; (c) conceded — but the name is built from our declared configuration, not viper internals, and is end-to-end pinned; (d) fully satisfied — the helper is viper-free; the decoded map is never read. Your unsatisfiability claim through viper's public API stands, and your resolution is the only coherent one. The governing principle, now applied without exception: for the ENV subtree, enforcement precedes the lossy transform on keys and values alike — viper never sees the ENV block at all.
Your governing principle is now stated without exception: "for the ENV subtree, enforcement precedes the lossy transform on keys and values alike — viper never sees the ENV block at all." That is true for the leaf level and the ENV block itself. It is not true for the server-name level — and the exception is exactly the nondeterminism your doctrine forbids, silently.
Consider raw MCP_SERVERS: {Plur: {COMMAND: ..., ENV: {A: 1}}, plur: {COMMAND: ..., ENV: {B: 2}}}. Viper's parse runs before your helper exists: insensitiviseMap collapses both keys to plur, one whole server block destroyed, winner nondeterministic (Go map iteration). The decoded cfg.MCPServers holds one plur with one block. Your step (2) — "case-insensitive structural nav to MCP_SERVERS.<name>.ENV" — then matches that single decoded name against two raw blocks, with no rule for which ENV map to stamp; whichever the raw map iteration visits first wins. So the fix's output for that config is nondeterministic — the precise property you refused at the leaf in Q1 and again in Q3, now reintroduced one level up. Worse, it's silent: no error, both servers' configs involved, one vanished.
The asymmetry with your leaf treatment is the tell: at the leaf, Path/PATH are representable in map[string]string, so both survive byte-for-byte; at the server level, two case-variants are not representable in map[string]MCPServerConfig (validated keys ^[a-z0-9-]{1,24}$), so "preserve both" is structurally impossible — which makes your own Q1 doctrine dictate the answer: reject deterministically. The coherent rule: for each decoded server, count raw MCP_SERVERS children that EqualFold-match its key; exactly one → proceed; zero → unreachable (skip); more than one → fail load() naming both raw keys, same error class as your leaf-level reject. Same check at the MCP_SERVERS level and the ENV key itself if you want the nav fully defined.
So: do you adopt reject-on-parent-collision as the helper's contract — and, in the same specification pass, pin these three loose ends so the Coder cannot silently diverge: (1) serverName in envNameForMCPServerLeaf and the stamp cfg.MCPServers[name].Env must be the decoded key (a raw-cased name makes the stamp a no-op on a map value copy — silent fix failure); (2) hoist the "TELL_ME" prefix and strings.NewReplacer("::", "_", "-", "_") into shared package-level vars consumed by both configureViper (config.go:130-132) and the helper, so lockstep is compile-enforced rather than comment-enforced — comment discipline is exactly what rots when a future edit changes one site; (3) non-scalar raw ENV values (maps/sequences) currently fmt.Sprint to garbage ("map[a:b]") where today mapstructure errors the load — specify reject-with-error so the helper cannot silently widen the accepted config surface.
Yes — I adopt reject-on-parent-collision as the helper's contract, with the count rule exactly as you specify, applied uniformly at every structural level the bypass navigates. And I pin all three loose ends. This closes the doctrine: the bypass is now deterministic at every level — leaf, ENV block, server name, and the MCP_SERVERS root — with no silent branch anywhere.
Reject-on-parent-collision, specified. The rule: for each decoded cfg.MCPServers key, count raw MCP_SERVERS children that EqualFold-match it — exactly one → proceed; zero → unreachable, skip (every decoded key descends from a raw key); more than one → load() fails naming both raw keys, e.g. MCP_SERVERS: keys "Plur" and "plur" differ only by case; server names must match ^[a-z0-9-]{1,24}$ — same error class and message style as ValidateMCPServers (mcp_config.go). The same count check applies at the two other structural levels the helper navigates: the top-level MCP_SERVERS key itself (raw MCP_SERVERS + mcp_servers siblings — a case where viper's collapse can silently empty the whole map, since an empty winner passes validation) and the per-server ENV key (ENV + env siblings). No EqualFold scan at any level may match more than one raw key; the nav is fully defined or the load fails. Your asymmetry diagnosis is the reason the rule is principled, not arbitrary: at the leaf, case-variants are representable in map[string]string, so Q1's doctrine says preserve both byte-for-byte; at the server level, case-variants are semantically unrepresentable (the ^[a-z0-9-]{1,24}$ validated-key contract), so the same doctrine says reject deterministically. And note what is deliberately not rejected: a single uppercase Plur with no lowercase twin loads fine, silently normalized to plur — that is viper's documented case-insensitivity for tell-me-go's own config keys (the issue's own framing: "case-insensitivity is desirable for tell-me-go's own config keys"), and it is deterministic because there is no data-destroying collision. Reject fires only on ambiguity, never on normalization. Ordering: the count checks run before any stamping, so a colliding config fails wholesale — no partial ENV application — and because the helper returns an error from load(), the watcher hot-reload keeps its prior state per ADR-029 §5, identical to every other load failure.
Loose end (1) — decoded-key iteration is the contract. The helper iterates for name := range cfg.MCPServers — the decoded map — and stamps server.Env = env; cfg.MCPServers[name] = server under that decoded name. Raw names are never used as Go map keys, and envNameForMCPServerLeaf receives the decoded name. Your hazard is real and worse than a no-op: cfg.MCPServers["Plur"] = server on an absent key inserts a phantom zero-valued server whose empty URL/COMMAND then fails validation with a misleading error — or, in the collision case, silently attaches the wrong ENV. The rule "iterate decoded, navigate raw case-insensitively, stamp decoded" makes the fix failure impossible rather than merely unlikely.
Loose end (2) — shared declarations, lockstep by construction. Hoist to package level in internal/infrastructure/config/config.go:
// envPrefix and envKeyReplacer are the app-owned environment-binding
// contract consumed by BOTH configureViper (config.go:130,132) and the
// case-preserving ENV bypass (envNameForMCPServerLeaf). Viper applies them
// to resolve TELL_ME_* overrides; the bypass reconstructs the same names.
const envPrefix = "TELL_ME"
var envKeyReplacer = strings.NewReplacer("::", "_", "-", "_")configureViper then calls v.SetEnvPrefix(envPrefix) / v.SetEnvKeyReplacer(envKeyReplacer) (replacing the inline literals at config.go:130,132), and envNameForMCPServerLeaf builds from the same two declarations. Divergence becomes impossible — there is one declaration; any future edit changes both consumers identically — and TestLoad_MCPStdio_EnvOverrideWins still pins the end-to-end contract through viper's real resolution. Your point that comment-discipline rots is exactly why this is compile-enforced, not comment-enforced.
Loose end (3) — scalar-only ENV values, reject with error. In the helper's value loop, type-switch on the raw-YAML value: string → as-is; int/uint/float64/bool → fmt.Sprint (weak-typing parity: 123→"123", true→"true"); nil → ""; map[string]any / []any → return an error naming server and key: MCP_SERVERS.<name>.ENV.<key> must be a scalar value, got map/sequence. Today mapstructure errors the load on a map or slice value inside map[string]string; without this rule the helper would silently widen the accepted config surface to "map[a:1]" garbage. The accepted surface stays identical to today: scalars and null only.
Test pins for this specification pass. (a) TestLoad_MCPStdio_StructuralCollisionRejected — table of three: server-key collision (Plur+plur), ENV-key collision (ENV+env), top-level collision (MCP_SERVERS+mcp_servers) — each asserts load() errors naming both raw keys. These tests are deterministic despite viper's random collapse winner, because the helper reads raw YAML, which preserves both keys. (b) TestLoad_MCPStdio_EnvNonScalarRejected — ENV: {FOO: {a: 1}} and ENV: {FOO: [1]} fail load. (c) TestLoad_MCPStdio_ServerKeyNormalized — single Plur: with ENV: {A: 1} loads, decoded as plur, Env["A"]=="1" — pins that we did not over-reject. (d) The Q3 suite (TestLoad_MCPStdio_EnvKeyCasePreserved with the Null key, TestLoad_MCPStdio_EnvOverrideWins with the empty-override and collision-override subtests, tightened TestLoad_MCPStdio_EnvExpansion) unchanged. TestLoad_ParsedDump_EnvOverriddenValues and TestSortedEnvPairs untouched.
The doctrine now holds without exception: for the ENV subtree, viper never sees the block at all; every level the bypass touches — leaf keys, leaf values, ENV block identity, server identity, MCP_SERVERS root — is either preserved byte-for-byte or rejected deterministically. Nondeterminism has no remaining entry point in the fix.
The design has converged — every hole I found across the round is closed, and the Q4 specification is internally consistent with everything verified in the tree. Checking the final claims: the phantom-server hazard (cfg.MCPServers["Plur"] = server inserts a zero-valued entry that then fails validation misleadingly — or, worse, attaches the wrong ENV) is real and correctly neutralized by iterate-decoded/stamp-decoded; the shared envKeyReplacer var is safe (documented concurrency-safe) and makes lockstep compile-enforced rather than comment-enforced; the scalar-only reject rule keeps the accepted config surface identical to today (mapstructure does error on map/slice into map[string]string); and test (a) is indeed deterministic despite viper's random collapse winner because the helper reads the raw document, which preserves both keys. No remaining entry point for nondeterminism in the fix.
VERDICT: Your diagnosis held up essentially intact — the two-site viper root cause, the ENV-only case-sensitive consumer, the silent-degradation blast radius, and your corrections to the issue (decode-hook infeasibility, the line-drift at stdio_client.go:77, the known-workaround smoking gun in TestLoad_MCPStdio_EnvExpansion) all verified against the live tree and viper v1.21.0. Your plan did not survive the grill in its original form — across four rounds it was corrected on the seam (repair → total bypass), on precedence (raw-wins → env-wins via app-owned env-name construction), on value authority (v.Get re-admitting the lossy transform → raw-YAML authority), and on determinism (leaf-only → reject-on-collision at every structural level) — but the final design is sound, deterministic, and test-pinned, so I recommend proceed with changes: implement applyCasePreservingMCPServerEnv per the Q4 contract with the full test suite, including the structural-collision, non-scalar-reject, and normalized-server pins. Two decisions remain the user's, not yours: whether to add the ADR + domain-model invariant you argued for against the issue's "no ADR expected" (I lean with you — cheap and durable, and the domain model exists to hold it), and whether TELL_ME_*_ENV_* overrides should be documented as supported (your choice) or rejected outright; neither affects the core seam.
What I stand by. The root-cause diagnosis was correct and remains verified against the pinned dependency, not asserted: Viper v1.21.0 lowercases the ENV map keys at two independent sites — insensitiviseMap (util.go:89-100, recursive delete-and-reinsert after YAML parse) and getSettings (viper.go:1970-1987, leaf strings.ToLower) — with v.Unmarshal (config.go:181) as the decode seam and sortedEnvPairs (stdio_client.go:354-365) passing the mangled keys through verbatim. ENV is the only config map whose keys reach a case-sensitive consumer; LLMProvider.Headers is RFC case-insensitive, and the README's own lowercase reasoning_effort example relies on that. The blast radius — every stdio server with an ENV block, silent, Linux/macOS-only (Windows is a no-op) — and the issue's option-1 family (bypass, not repair) were right. The final design stands as specified across Q1–Q4: total bypass of Viper for the ENV subtree — raw YAML as the sole authority for keys and values — with env-wins delivered via os.LookupEnv on app-owned names, expansion and weak-typing parity preserved, empty-override-as-unset matching Viper's AllowEmptyEnv default (viper.go:442-450), scalar-only values with reject, structural collision rejection at all three navigation levels, and the decode-path regression suite as the primary gate. stdio_client.go needs no change.
What the grill revised. Three of my own errors, each caught and corrected: (1) Q1 — my initial mechanism was the issue's option 2 mislabeled as option 1, and repair-after-decode is structurally incapable of the byte-for-byte invariant: the lossy collapse happens before any repair can run, so the loser's value is unrecoverable and no casing rule can be sound. The griller's seam theorem — enforcement must precede the lossy transform — became the governing doctrine. (2) Q2 — my "raw YAML wins on conflict" precedence inverted Viper's test-pinned env-wins (verified: TestLoad_ProviderMaxTokens_EnvOverride, config_test.go:655), and my supplement branch was dead code per the AllKeys proof (config_test.go:1032-1033). (3) Q3 — my v.Get-based env resolution re-admitted the collapsed map as authority on values: v.Get cannot return two values for one collapsed key, so Path/PATH both received the nondeterministic winner — the Q1 theorem re-violated one level down. The bypass became total, and env-wins moved to os.LookupEnv on names built from our own declared prefix/replacer (config.go:130,132). (4) Q4 — the same nondeterminism one level up at the server-name level, which is unrepresentable rather than preservable, and therefore reject-on-collision rather than preserve-both: a principled asymmetry, not a loophole. The three loose ends (decoded-key stamping, shared envPrefix/envKeyReplacer declarations, scalar-only reject) are pinned. The doctrine I state now is the one the grill forced me to: deterministic-or-reject at every level the bypass touches — leaf keys, leaf values, ENV identity, server identity, MCP_SERVERS root — with no silent branch anywhere.
Open questions — the two the griller flagged as the user's, with my recommendations. (1) ADR + domain-model invariant: recommend YES. The fix is a deliberate anti-Viper measure whose protection a future Viper minor bump could silently re-break; a short ADR (or an ADR-067/ADR-068 lineage note) pinning the ENV byte-for-byte invariant and the collision-rejection semantics is cheap given the ADR index gate, and the reject rules are a new config-validation behavior — precisely the class this repo records. Add the matching invariant on MCPServerConfig.Env (mcp_config.go:45) and, if the maintainers want the discipline enforced, a domain-model invariant. (2) TELL_ME_*_ENV_* overrides: recommend document-as-supported, not reject. The mechanism works today (env-wins is test-pinned generically), the final design preserves it deterministically at near-zero cost, and rejecting it would be a silent behavior change for any operator who uses it — the exact failure class this issue is about. Documenting costs one precedence paragraph in the README MCP section: env-over-file for declared keys only, overrides cannot invent undeclared variables (AllKeys semantics), empty overrides are unset, and an override applies to all casings of a name. If the maintainers judge the path too exotic to support, the reject is still cheap at this point (fail load on any exported TELL_ME_MCP_SERVERS_*_ENV_* var) — but I'd argue support is the lower-risk, lower-churn choice. Two smaller open items: whether to add the optional end-to-end spawn assertion (the /proc/<pid>/environ integration test — non-blocking, my recommendation is to include it as the belt to the decode-path suspenders), and awareness only — the fix's Windows behavior is intentionally unchanged, and the collision+override rule (apply-to-all-casings) is settled in the contract but remains the one rule a maintainer could swap for reject without touching the rest of the design. I stand by the design as grilled; the remaining decisions are documentation and support-scope choices, not design choices.