Subject: Architect · Griller: Griller · Orchestrator: Butler · 2026-08-17 Verdict: proceed with changes · 4 questions (early conclusion)
I've completed the bootstrap (README, Makefile, domain models, ADR-067, INTENTIONAL_NON_FIXES, environments docs, the cited source files, and the test suites). Here is my opening statement.
Understanding & diagnosis. The issue asks for native Authorization: Basic base64(email:token) support in MCPServerConfig so the Atlassian Rovo MCP server (https://mcp.atlassian.com/v1/mcp) works with personal API tokens instead of requiring an org-admin-minted service-account key (the only Bearer-compatible path today). The diagnosis holds against the code. internal/infrastructure/mcp/client.go is structurally Bearer-only: NewClient(endpoint, token, timeout) accepts a single token string, and bearerTokenTransport.RoundTrip (client.go) hardcodes req.Header.Set("Authorization", "Bearer "+t.token) — the scheme is not plumbable. MCPServerConfig (internal/domain/config/mcp_config.go) has AUTH modes auto/gh/bearer/none (ADR-067 §4), and validate() rejects any other mode (TestMCPServerConfig_Validate_AuthModes even lists "basic" among invalid modes today). The DI layer (mcp_factory.go resolveServerToken) resolves exactly one bearer token. The issue's live verification is credible and transport-compatible: Rovo speaks streamable HTTP with text/event-stream framing, which is what StreamableClientTransport already does, and 401-with-WWW-Authenticate: Bearer on unauthenticated requests confirms the server is auth-gated, not protocol-incompatible.
Blast radius. (1) internal/domain/config/mcp_config.go — new MCPAuthBasic const, a Username field (yaml:"USERNAME"), validate() requiring non-empty USERNAME + TOKEN when AUTH: basic, and the AUTH doc comment; env interpolation is free because the global expandEnvHook (internal/infrastructure/config/config.go:187, os.ExpandEnv on every string field) already applies to any new string field, exactly matching the issue's requirement. (2) internal/infrastructure/mcp/client.go — generalize the constructor/transport so the Bearer path stays byte-identical; the SDK stays confined per verify-mcp-sdk-confinement (ADR-067 §2). (3) internal/infrastructure/di/mcp_factory.go — resolve Basic credentials and warn-and-skip on unresolvable, per §4 semantics. (4) Tests: mcp_config_test.go (TestMCPServerConfig_Validate_AuthModes must move "basic" from the invalid list to valid — the issue's "no regression on TestMCPServerConfig_*" is subtly wrong), client_test.go, and mcp_factory_test.go, where the injectable newClient func(endpoint, token, timeout) field signature change ripples through all ~14 tests. (5) Domain model: docs/domain-model/tell-me-go.modelith.yaml MCPServer entity — auth attribute, new username attribute, extended mcp-token-not-logged statement, optional mcp-basic-credentials-required invariant, MCP scenario extension; make modelith-render + modelith-check. (6) ADR-067 §4 amendment (auth-mode table + resolution hierarchy); verify-adr-index passes since ADR-067 is already indexed (docs/adr/README.md:75) — no new ADR number. (7) Two items the issue omits: README.md's MCP sample comment (# auto (default), gh, bearer, or none) must gain basic, and INTENTIONAL_NON_FIXES.md has coverage pins on mcp_factory.go (resolveServerToken default case :148-151, gh auth token resolver :60-65) whose lines will shift — the drift policy requires re-anchoring plus the partition-test coordination rule (verify-nonfix-catalog) in the same PR.
Proposed design. I adopt Design consideration 1 (AUTH: "basic" + credential fields) and defer Design consideration 2 (generic HEADERS map) — the issue's guardrail, and I agree: header-merge semantics with the existing transport, ad-hoc credential plumbing, and a widened config surface are not justified by a single server. Field naming: USERNAME + TOKEN-as-password, not EMAIL — Basic's userid is generically a username (Atlassian happens to use the account email; the capability "belongs in the client" per the issue, so the field should not be Atlassian-specific; a doc comment notes the Atlassian mapping). AUTH-mode interaction: EffectiveAuth() stays the single normalization point; validate() adds basic to the switch and a symmetric requirement (both USERNAME and TOKEN non-empty, mirroring the bearer branch); unknown modes remain rejected. Credential resolution: explicit-only under basic (like bearer — no gh/env fallback; auto remains GitHub-oriented and never resolves Basic), with the factory warn-and-skip path (mcp_token_resolution_skipped logs only server name + URL) preserved for defensiveness. mcp-token-not-logged: I'll extend the invariant's statement to explicitly cover Basic credentials and the derived Authorization: Basic … header — base64 is trivially reversible, so the derived header is secret-bearing and must never be logged/serialized/diagnosed; the factory and client must never log username, token, or the header. Transport: keep bearerTokenTransport untouched (byte-identical Bearer guarantee) and add a parallel Basic transport (or generalize via a small Auth{Mode, Username, Token} value type passed to a single NewClient) that sets Authorization: Basic base64(username:token) only when credentials are present — empty credentials → no header, preserving current behavior. The tools.MCPClient domain port (internal/domain/tools/mcp_client.go) is untouched — auth knowledge stays in the infrastructure adapter and DI, per ADR-067 §2/§3.
Test strategy & verification. Config: extend the table-driven AUTH tests (basic valid with creds, invalid without either field, unknown modes still rejected, USERNAME YAML binding, EffectiveAuth normalization). Transport: httptest.Server capturing the Authorization header (per existing client_test.go patterns) pinning Basic <base64(user:pass)> and, critically, a byte-identical Bearer <token> regression pin; assert the original request is not mutated (clone semantics) and empty-credential → no header. DI: TestMCPFactory_AuthBasic (explicit creds flow to newClient, gh resolver never invoked), warn-and-skip on empty creds, no regression on the existing auth-mode matrix. Domain/ADR gates: make modelith-render, modelith-check, verify-adr-index, verify-nonfix-catalog (with re-anchor), verify-architecture, verify-mcp-sdk-confinement, and full make check-full (all new functions CC ≤ 10; no time.Sleep per ADR-036; no testify/mock per ADR-021; no "testing" in production per ADR-022). E2E: a AUTH: basic config with USERNAME: "${ATLASSIAN_EMAIL}", TOKEN: "${ATLASSIAN_TOKEN}" (env-interpolated, no plaintext in committed configs), tool discovery yielding mcp_atlassian_*, and a real read call (e.g. Jira search) with the transcript + make check-full attached to the PR body — explicitly as manual/manual-evidence verification, never a live network call inside the hermetic test suite.
Risks, open questions, disagreements. (a) The issue's acceptance criterion "no regression on TestMCPServerConfig_*" is inaccurate — TestMCPServerConfig_Validate_AuthModes must change by design (basic moves invalid→valid); I'd reword the criterion to "rejects unknown modes; the basic migration of the auth-mode test is intentional." (b) The issue understates test ripple: the newClient injectable signature change in mcp_factory.go touches every factory test and the nonfix-catalog pins — both must be in the same PR. (c) I'd add the README sample-comment update to scope. (d) Open question for the griller/architect: single NewClient with an Auth value struct vs. a parallel NewBasicClient constructor — I prefer the former (one construction path, factory maps MCPServerConfig → Auth, no dual-constructor drift), but it is the one decision with real API-shape consequences. (e) Note mcp.atlassian.com/v1/mcp is not a /readonly path, so ADR-067 §8 defaults apply unchanged: RequiresConsent: true, Serial: true — correct for a mutating Jira/Confluence server, no change needed. (f) Hot-reload boundary (ADR-067 §10) is unchanged: changing Basic credentials requires a new session — document it. Overall the issue's plan is sound and minimal; my changes are scoping additions, not design disagreements.
I've completed the bootstrap and verified the subject's claims against the code. Before Q1, here's my verification ledger — everything I checked, so the round starts from shared ground:
Verified — diagnosis holds:
bearerTokenTransport.RoundTrip(client.go) hardcodes"Bearer "+t.token;NewClient(endpoint, token, timeout, opts...)is the only constructor. ✅MCPServerConfig.validate()rejects unknown modes;TestMCPServerConfig_Validate_AuthModesindeed lists"basic"in the invalid set (invalid := []string{"basic", "oauth", "gh-token", "token"}). The subject's correction of the issue's "no regression" criterion is right — that test must change by design. ✅expandEnvHook(config.go:183 — subject cited :187, off by 4) is amapstructureDecodeHook applied inunmarshalConfigvia viper, firing on source string values — so a newUSERNAMEstring field in the nestedMCP_SERVERSmap does get${ENV_VAR}expansion. Claim holds. ✅- Catalog pins exact:
resolveServerTokendefault case at mcp_factory.go:148–151,gh auth token resolverat :60–65. ✅ ADR-067 indexed at docs/adr/README.md:75 (exact). ✅mcp.atlassian.com/v1/mcpis non-/readonly→RequiresConsent: true, Serial: trueperisReadOnly(). ✅ README sample comment# auto (default), gh, bearer, or noneexists. ✅ newClientinjectable: 12 of 14 factory tests construct it (Close + IsGitHubHostname don't), so "~14" is an overcount of 2 — direction correct. Productioninfra_mcp.NewClientcallers: exactly 1 (the default closure, mcp_factory.go:68). client_test.go: 28NewClient(occurrences across 13 test functions.
Q1: You left decision (d) open — single NewClient with an Auth value struct vs. a parallel NewBasicClient — but there is a third option you didn't consider, and it dominates on churn. client.go already has a functional-options pattern: NewClient(endpoint, token, timeout, opts ...option) with withHTTPClient as the sole existing option. Add withBasicAuth(username string) — the positional token becomes the Basic password, the transport selects the scheme from an internal mode field defaulting to Bearer, so the existing Bearer branch stays byte-identical by construction — and then: (1) all 28 NewClient( occurrences in client_test.go remain source-untouched; under your Auth-struct design every one churns for zero functional gain on an internal constructor; (2) the DI injectable change is unavoidable under any design — the factory must carry username to construct the client — so the 12 newClient: constructors in mcp_factory_test.go churn regardless; (3) you still get exactly one construction path, so your "no dual-constructor drift" argument for the Auth struct applies equally to the option pattern; (4) the byte-identical Bearer guarantee is stronger with the option pattern, because the existing call sites and the existing Bearer branch are literally the unchanged code, not a re-implementation.
So the question: given that the option pattern is already the established extension point in this very file, why prefer a signature-breaking NewClient(endpoint, auth, timeout) over withBasicAuth(username), and can you name anything the Auth-struct design buys that the option pattern doesn't, to justify churning 28 test occurrences? My recommendation: extend the option pattern — keep NewClient's signature, let the DI injectable signature change be the only forced churn, and pin the Basic header with a new TestBasicAuthTransport alongside the existing TestBearerTokenTransport.
A1 — Concession, verified. You're right, and I adopt the option pattern. I verified your counts before answering: grep -c "NewClient(" internal/infrastructure/mcp/client_test.go = 28 (14 constructor calls + 14 t.Fatalf("NewClient() error = %v", err) strings), spread across 13 test functions; production callers of infra_mcp.NewClient = exactly 1 (mcp_factory.go:68); newClient: injectable closures in mcp_factory_test.go = 12 (Close + IsGitHubHostname are the non-constructing pair — your "12 of 14" is exact). My "~14" was an overcount; yours is the verified number. My expandEnvHook citation was also sloppy: the function is declared at config.go:183; the os.ExpandEnv call I cited sits at :187 inside it. Noted and corrected.
The substantive concession — the option pattern dominates, and I'm adopting it. withHTTPClient already establishes the extension point in this exact file; NewClient(endpoint, token, timeout, opts...) is the sole constructor; the positional token doubling as the Basic password requires zero client_test.go churn (28 occurrences stay source-untouched — the strongest possible form of your point (4), because bearerTokenTransport/withBearerToken and every existing call site are literally the unchanged code, not a re-implementation that happens to emit the same bytes). Your point (3) is also correct: I claimed "one construction path, no dual-constructor drift" for the Auth struct, but the option pattern has exactly one constructor too — the option is not a second constructor, so that argument transfers wholesale and then some. I retract the Auth-struct recommendation from my opening statement.
What the Auth struct bought, named honestly — and why it doesn't survive contact: exactly two things, both real but both outweighed. (1) Factory-mapping readability — Auth{Mode: Basic, Username: u, Token: t} reads more declaratively than "pass token positionally, add withBasicAuth(username)". But the readability lives in the factory's switch auth anyway, which is the single place the mapping is visible under either design; the option version is one extra argument at the call site, not a lost invariant. (2) Explicit mode state on the client — the Auth struct makes the client know it is Basic. Under the option pattern the mode is inferred from username != "". But the mode is consumed by exactly one thing (transport selection in connect()), and the mcp-token-not-logged invariant wants less visibility of credentials, not more — an internal mode field that never logs is strictly aligned with it. Neither justifies churning 28 test occurrences on an internal constructor.
The one refinement I add to your proposal — the DI seam shape. You're right that the injectable change is unavoidable (the factory must carry per-server username, so the newClient field cannot stay 3-param). I'd make the seam explicit rather than option-passing: newClient func(endpoint, username, token string, authMode string, timeout time.Duration) (tools.MCPClient, error). Why not opts ...infra_mcp.Option on the seam: option is currently unexported, exporting it to DI would force the 12 test fakes to parse/inspect options to assert the username flowed through — strictly more test complexity than recording 2 extra positional params. With explicit params, the production default closure switches on authMode: basic → NewClient(endpoint, token, timeout, withBasicAuth(username)), else the current 3-arg NewClient(endpoint, token, timeout) — so the adapter's public API is the option pattern, and the seam stays a dumb param carrier. The 12 factory fakes churn from 3→5 params regardless (unavoidable, agreed), but the churn is mechanical and the assertions get simpler (captured["…"].username).
The resulting shape, pinned: keep bearerTokenTransport, withBearerToken, and connect()'s existing Bearer call verbatim; add basicTokenTransport + a client option withBasicAuth(username string) that sets c.username; connect() selects if c.username != "" { withBasicAuthHeader(httpClient, c.username, c.token) } else { withBearerToken(httpClient, c.token) }. Empty-credential semantics match bearer: empty username or token → no header (defensive; config validation already forbids it in production). Tests: new TestBasicAuthTransport beside the untouched TestBearerTokenTransport, an end-to-end httptest.Server header-capture pin for Basic <base64(user:pass)>, and TestNewClient remains byte-identical. Every other element of my opening — config validation symmetry, USERNAME+TOKEN-as-password naming, mcp-token-not-logged extension to the derived header, catalog re-anchor duty, README comment, modelith/ADR gates — stands unchanged on top of this construction path.
Verified before Q2:
- The bearer factory path is an unconditional pass-through —
case config.MCPAuthBearer: return serverCfg.Token, true(mcp_factory.go:130). There is no skip and no warn for bearer:mcp_token_resolution_skippedexists only insideresolveTokenOrSkip, reachable solely from thegh/autoresolution-failure paths.TestMCPFactory_AuthBearerpins pass-through with zero resolver calls, and there is no factory test for bearer-with-empty-token — becausevalidate()forbids it. The transport's no-header-on-empty is the only defensive layer, and it's pinned (TestWithBearerToken). - The auth-mode comment lives in exactly 4 places — README.md:345, modelith yaml:497, modelith md:136, ADR-067:52 — all inside your stated scope.
configs/butler.yamlhas noMCP_SERVERSsection, so your scope list is complete; no gap there.
Q2: Your opening statement says the factory should "resolve Basic credentials and warn-and-skip on unresolvable, per §4 semantics," and that the mcp_token_resolution_skipped path is "preserved for defensiveness" under basic. But you also say basic "mirrors bearer" — and bearer's factory path does not skip or warn: it is an unconditional return serverCfg.Token, true. The skip-with-warn behavior is exclusively a resolution-failure mechanism for gh/auto (exec failure, missing gh, empty GITHUB_TOKEN) — under basic there is nothing to resolve, and an empty credential set is unreachable post-validate() (both USERNAME and TOKEN required, mirroring the bearer branch you yourself specced). So there are exactly two coherent positions, and your opening contradicts itself:
- (a) Pass-through, mirror bearer: empty creds flow to the client, the transport's existing no-header-on-empty defense applies, the remote server 401s with a diagnosable
WWW-Authenticate— the case the issue's own live verification documents. No new factory branch at all. - (b) Skip-with-warn under basic: a new
resolveServerTokenbranch that emitsmcp_token_resolution_skippedand silently drops the server — behavior bearer doesn't have, reachable only by bypassingvalidate(), i.e. exactly thedefensive-guardclass this repo catalogues rather than ships silently (your ownresolveServerTokendefault-case is a live nonfix entry at :148-151 with that rationale).
Which is it? And note the catalog consequence either way: if you choose (b), the empty-credential skip branch is unreachable defensive code that — per the repo's own drift policy and the nonfix-catalog convention — must be documented as a new defensive-guard entry with a partition-test edit in the same PR, expanding this PR's catalog surface; if you choose (a), your opening's "warn-and-skip preserved for defensiveness" language must be retracted to gh/auto-only, which is where it actually lives today. My recommendation: (a) pass-through — it matches bearer exactly, reuses the pinned transport defense, keeps the server's 401 (with its WWW-Authenticate header) as the diagnosable failure mode, adds zero new factory branches, and avoids new nonfix-catalog surface entirely. Can you point to any behavior basic loses by not skipping that pass-through doesn't already cover?
A2 — Concession, and I choose (a) pass-through. You caught a real incoherence in my opening: I imported the gh/auto resolution-failure mechanism into a mode that has no resolution step. The evidence is exactly as you state — case config.MCPAuthBearer: return serverCfg.Token, true (mcp_factory.go:130) is unconditional; mcp_token_resolution_skipped lives only in resolveTokenOrSkip (mcp_factory.go:157-167), reachable solely from the gh/auto failure paths; TestMCPFactory_AuthBearer pins pass-through with a resolver that errors if called; TestWithBearerToken pins the transport's no-header-on-empty as the sole defensive layer. My opening's "warn-and-skip on unresolvable, per §4 semantics" and A1's "preserved for defensiveness" are retracted — that mechanism is gh/auto-only, both today and in my design.
The decisive point that settles it — "unresolvable" is a config-load error under basic, not a factory state. Order of operations in load() (internal/infrastructure/config/config.go): unmarshalConfig runs expandEnvHook (declared :183, os.ExpandEnv at :187) before cfg.ValidateMCPServers(). os.ExpandEnv on an unset var yields "" — so TOKEN: "${ATLASSIAN_TOKEN}" with the env var unset lands in validate() as empty and is hard-rejected at load (TOKEN must not be empty when AUTH is basic). This is not speculative: the identical chain already exists for bearer today — an unset ${GITHUB_TOKEN} reference in a bearer server's TOKEN hard-fails load the same way, and there is deliberately no factory test for bearer-with-empty-token because validate() forecloses it. So for basic there is no construction-time credential state to warn-and-skip: everything is in the struct post-load, and the empty state is unreachable except by programmatic construction bypassing validation — precisely the defensive-guard class your own citation of my resolveServerToken default-case (:148-151) uses as its rationale. Shipping a skip-branch for that would be new unreachable defensive code requiring a new catalog entry + partition-test edit — expanding this PR's surface to guard a state production cannot reach.
I also name an internal tension in the issue itself, and my adjudication: the issue's Design consideration 3 ("unresolvable credentials → warn-and-skip at startup, not fatal") conflicts with its own acceptance criterion ("Config validation rejects the new mode without credentials") and plan C1 ("empty credentials rejected when the mode requires them"). These cannot both hold for basic — an env-interpolated-empty credential is either a validate() hard-error or a factory warn-and-skip, not both. The acceptance criterion and C1 win (they are mode-specific; Design consideration 3's sentence governs the resolution hierarchy), and basic mirrors bearer: validate() is the guard; the factory passes through. I'll record this adjudication explicitly in the ADR-067 §4 amendment so the inconsistency is documented rather than silently resolved.
Concrete shape under (a): resolveServerToken changes its return to the credential triple (username, token string, ok bool); the basic case is return serverCfg.Username, serverCfg.Token, true — unconditional, structurally identical to bearer. Build gains no branch at all: authMode flows to newClient as data (serverCfg.EffectiveAuth()), and the production default closure (mcp_factory.go:68) switches on it — basic → NewClient(endpoint, token, timeout, withBasicAuth(username)), else the current 3-arg call. The mcp_token_resolution_skipped warn stays exclusively in resolveTokenOrSkip (gh/auto). Catalog consequence: no new nonfix entry, no partition-test edit; the existing resolveServerToken default case pin (:148-151) needs only line re-anchoring if the signature change shifts it — maintenance, not surface, per the drift policy I already committed to.
Your closing question — does basic lose anything by not skipping? No, and I'll enumerate the trade explicitly: skip-with-warn's only added behavior is dropping the server from the registry when creds are empty post-validate. That state is (i) unreachable in production — validate() hard-fails at load naming MCP_SERVERS.<name>.USERNAME/TOKEN, which is a stronger diagnostic than a warn-and-skip that silently omits mcp_atlassian_* tools and leaves the operator grepping logs for why; and (ii) reachable only via programmatic construction, where the pinned transport no-header defense plus the server's own 401 + WWW-Authenticate is the correct, diagnosable failure — the issue's live verification documents that exact unauthenticated response. There is no third state where a skip would fire. Pass-through covers everything skip-with-warn would, with a louder failure and less dead code.
Test delta under (a): TestMCPFactory_AuthBasic mirrors TestMCPFactory_AuthBearer — explicit USERNAME+TOKEN flow to newClient, zero resolver calls, no skip assertion (there is no skip path); the empty-credential guard is tested in mcp_config_test.go (validate() rejects AUTH: basic with empty/whitespace USERNAME or TOKEN), not in the factory. Transport tests unchanged from A1: TestBasicAuthTransport beside the untouched TestBearerTokenTransport, plus the httptest header-capture pin for Basic <base64(user:pass)>.
A2's sharpest point verifies cleanly against the code: load() runs unmarshalConfig (where expandEnvHook fires) before cfg.ValidateMCPServers(), and os.ExpandEnv on an unset var yields "" — so TOKEN: "${ATLASSIAN_TOKEN}" with the env unset hard-fails at load, the identical chain bearer already has. Your adjudication (validate() wins over the issue's Design consideration 3; recording it in the ADR-067 §4 amendment) is sound. Also verified your factory mechanics: MCPAuthNone returns "", true and every non-basic mode produces an empty username in the triple by construction.
Q3: Your A1 seam carries two discriminators that cannot disagree — authMode and username. resolveServerToken is the only producer of the seam's (username, token) pair, and its switch guarantees username == "" for every non-basic mode by construction (none → "", true; bearer → "", cfg.Token, true; gh/auto → "", resolved, true; only basic returns cfg.Username). So at the seam, username != "" ⟺ authMode == "basic" always — and your own A1 pin has the client discriminating on c.username != "" in connect(), not on a mode field. The authMode param therefore carries zero information: it cannot be observed to disagree, yet it costs a 5th parameter in all 12 factory test fakes, a second switch in the production default closure, and a consistency obligation (the closure's authMode switch is only correct if the factory never changes the pairing). Dropping it gives: newClient func(endpoint, username, token string, timeout time.Duration), closure if username != "" { NewClient(endpoint, token, timeout, withBasicAuth(username)) } else { NewClient(endpoint, token, timeout) } — structurally identical to connect(), one discriminator, one switch, 4-param fakes.
The one case to nail down while you're in there: a stray USERNAME under AUTH: bearer — your validate() spec adds only the positive rule (basic requires both fields); nothing forbids USERNAME under bearer/auto/gh/none. Today, the analogous case (TOKEN under AUTH: none) is silently ignored by the factory (case config.MCPAuthNone: return "", true) and not rejected by validate(). Under your seam, a stray USERNAME is likewise silently normalized to "" by resolveServerToken — the factory never lets it reach the client. Is that silent normalization your intent (consistent with the TOKEN-under-none tolerance, no validate() change), or should validate() gain a negative rule ("USERNAME must be empty unless AUTH: basic")? My recommendation: drop authMode from the seam — discriminate on username != "" exactly as connect() does — and keep validate()'s positive rule only, letting resolveServerToken's normalization be the documented behavior, matching the existing TOKEN-under-none tolerance and keeping the factory switch the single place where mode→credentials mapping lives. A negative validate() rule would be the first strictness of its kind in MCPServerConfig and would reject configs that currently load fine. Can you name anything the authMode param preserves that username != "" loses?
A3 — Concession, both points; I adopt the 4-param seam and the positive-rule-only validation. The authMode param was redundant with my own design, and you're right to call it my inconsistency: in A1 I specced connect() discriminating on c.username != "" (no mode field on the client), then turned around and added authMode to the seam — a second discriminator that cannot disagree with the first. Since resolveServerToken is the sole producer of the (username, token) pair and its switch makes the pairing airtight — none → ("", "", true), bearer → ("", cfg.Token, true) (mcp_factory.go:129-130 today, triple under the change), gh/auto → ("", resolved, ok) via resolveTokenOrSkip, only basic → (cfg.Username, cfg.Token, true) — username != "" at the seam is exactly authMode == "basic", observable-equivalent by construction. The param's cost is precisely as you enumerate: a 5th parameter × 12 fakes, a second switch in the production closure, and a consistency obligation — the closure's authMode switch is only correct if the factory never changes the pairing, which is a rule I'd be maintaining for zero information. Dropping it gives one discriminator, one switch, and the closure if username != "" { NewClient(endpoint, token, timeout, withBasicAuth(username)) } else { NewClient(endpoint, token, timeout) } — structurally identical to connect(), so the same rule is written once and mirrored, not maintained twice.
Your closing question — what does authMode preserve that username != "" loses? Nothing functional in the current five-mode space: the mode is fully derivable from the credential shape, and the unreachable states (basic-with-empty-username, non-basic-with-username) are exactly the ones the param would exist to detect but cannot fire on — basic-with-empty-username is foreclosed by validate()'s positive rule, and non-basic-with-username is foreclosed by resolveServerToken's pairing. The only things it nominally preserves are (i) a "scheme" signal for hypothetical future modes with non-username credentials — but such a mode changes the seam's credential shape anyway, so the param would be reworked regardless, and (ii) documentation value — which the option name itself already carries: the closure's withBasicAuth(username) call is the mode declaration. Neither justifies a redundant parameter. One genuine edge worth stating for the record: a future seam consumer passing username != "" under a non-basic mode would be treated as basic by connect() — but the factory cannot produce that state, and cross-checking against authMode to catch it is precisely the consistency obligation being removed. The single rule — "non-empty username means Basic" — lives in the option semantics and is applied identically in both layers.
Stray USERNAME under non-basic modes: silent normalization, positive rule only — your recommendation (a), with evidence it matches established tolerance. validate() today contains zero negative rules — only URL-non-empty, TIMEOUT ≥ 0, AUTH-in-known-set, and bearer-requires-TOKEN (all positive). The factory already silently drops credential fields the mode doesn't use: case config.MCPAuthNone: return "", true discards a stray TOKEN under none, and no config is rejected for it — TestMCPFactory_AuthNone pins exactly that (resolver uncalled, empty token reaches the client). A negative "USERNAME must be empty unless AUTH: basic" rule would be the first strictness of its kind in MCPServerConfig and would create an asymmetry with the tolerated stray TOKEN that a user would reasonably ask about ("why is my stray USERNAME fatal when my stray TOKEN is ignored?"). And the normalization is benign in practice: under bearer, the client uses TOKEN; the stray USERNAME is dropped before it reaches the transport — the config does what its AUTH says. The misconfiguration the negative rule would catch ("USERNAME+TOKEN but AUTH: bearer, expecting basic") is self-declared by the user's own AUTH value and surfaces as a server 401 with WWW-Authenticate — the issue's documented diagnosable failure. The positive rule already catches the genuinely dangerous case: AUTH: basic with a missing field hard-fails at load naming the server and field.
Concrete deltas under the adopted shape. resolveServerToken returns (username, token string, ok bool); basic → (cfg.Username, cfg.Token, true); bearer → ("", cfg.Token, true); none → ("", "", true); gh/auto unchanged in spirit via resolveTokenOrSkip (username always ""). Build still gains no branch — mode flows as data into newClient(serverCfg.URL, username, token, serverCfg.EffectiveTimeout()); the production default closure holds the single username != "" switch. Seam: 4-param fakes. Test pins for the normalization contract: TestMCPFactory_AuthBasic asserts captured.username == cfg.Username and captured.token == cfg.Token (resolver uncalled); TestMCPFactory_AuthBearer gains a stray-USERNAME subtest asserting the factory normalizes to ("", "explicit-token") — this is the test that documents the tolerance at the factory layer, mirroring how TestMCPFactory_AuthNone documents the TOKEN-under-none drop. Config tests keep only the positive rule (basic requires non-empty USERNAME and TOKEN; unknown modes rejected). The ADR-067 §4 amendment records both the mode→credentials mapping as a single factory-owned normalization (stray fields ignored, matching the existing TOKEN-under-none precedent) and the Q2 adjudication (validate-rejects for basic, warn-and-skip remains gh/auto-resolution-only). Catalog consequence unchanged: no new nonfix entry; the existing resolveServerToken default case pin re-anchored if the signature change shifts it.
Verified the debug paths precisely: config.go:147 — slog.Debug("raw content", slog.String("content", string(data[:min(len(data), 1000)]))) — dumps the first 1000 chars of the config file; config.go:156-158 — viper parsed keys / parsed entry with slog.Any("value", v.Get(key)) — logs every key's value. Both gated by isDebug() (config.go:23), which is true only when the default logger's level is Debug.
Q4: You plan to extend mcp-token-not-logged — "Authentication tokens for MCPServers must never be logged or serialized into diagnostics" — to explicitly cover Basic credentials and the derived Authorization: Basic header, and you assert "the factory and client must never log username, token, or the header." But the codebase already has two diagnostics paths that serialize exactly this class of data, and they will serialize USERNAME the moment this PR adds the field:
config.go:147logs the raw config file content in debug mode — a literalTOKEN:/USERNAME:value in the file lands in the diagnostics dump.config.go:156-158logs every viper key plusv.Get(key)— and because viper hasAutomaticEnv()with theTELL_ME_prefix and the key replacer wired inconfigureViper,v.Getfor an env-overridden key returns the live secret (e.g.TELL_ME_MCP_SERVERS_ATLASSIAN_TOKEN's value), not just a file literal. So even the issue's recommended${ENV_VAR}pattern doesn't protect this path — the env-sourced value itself is what gets logged.
This is a real coherence gap in your plan: you're strengthening an invariant's statement (adding "and the derived header") whose plain text the existing code already violates on a reachable — debug-enabled — path, and the PR is about to widen the exposed surface (new USERNAME field) without touching those paths. Three ways out, and the choice changes the ADR wording:
- (a) Fix: redact/remove the two debug dumps in config.go — correct, but scope-creeps a hygiene change into config.go and its tests, inside an MCP auth PR that currently touches exactly zero lines of the config loader.
- (b) Document: keep the extended invariant for the new code (factory, client, transport — which genuinely never log credentials), and record in the ADR-067 §4 amendment that the config-loader debug dumps pre-date the invariant, are gated behind an explicitly debug-enabled logger, and are a known, out-of-scope exception — with the invariant's wording scoped honestly to the credential plumbing, not the general config loader.
- (c) Don't extend the invariant: leave the statement as-is, since extending it overstates what the system as a whole honors.
My recommendation: (b) — the debug dumps are general config diagnostics, pre-date ADR-067, require an explicit debug logger to fire, and are not MCP-credential plumbing; fixing them drags the config loader into a feature PR for a narrow, debug-gated exposure, exactly the kind of surface expansion the repo's small-PR and nonfix-catalog culture resists. The extended invariant should say what the new code actually does. Which do you choose, and does your answer change the invariant's wording from "never" to something scoped? (After this thread resolves, I have no further substantive angles — the construction path, factory semantics, seam shape, validation posture, and catalog duty are all settled — so I'll be ready to move to the closing when you are.)
A4 — Choice: (b) Document, with the invariant's "never" scoped to the MCP credential plumbing — and I concede the coherence gap is real. Your verification is exact: config.go:147 dumps the raw file's first 1000 chars (slog.Debug("raw content", ...)), config.go:156-158 logs every viper key with slog.Any("value", v.Get(key)), both gated by isDebug() (config.go:23, slog.Default().Enabled(ctx, slog.LevelDebug)). And your sharper point holds: with SetEnvPrefix("TELL_ME"), the ::→_ replacer, and AutomaticEnv() in configureViper, v.Get("mcpservers::atlassian::token") returns the live TELL_ME_MCP_SERVERS_ATLASSIAN_TOKEN value — so the issue's recommended ${ENV_VAR} pattern does not protect path 2; the env-sourced secret itself is what gets logged. My A1 assertion "the factory and client must never log username, token, or the header" was true of the plumbing but false of the system: I strengthened a statement the existing code already violates on a reachable path, and this PR widens the exposed surface with the new USERNAME field without touching either dump. Conceded fully.
Why (b) and not (a) or (c) — and the decisive evidence is that this is not an MCP defect at all. The viper parsed keys dump serializes every config key's value: providers::google::apikey, GITHUB_TOKEN-backed values, TELL_ME_*-overridden secrets — the entire credential surface of the system, not MCP credentials specifically. mcp-token-not-logged is an MCP-scoped invariant; binding it to the general config loader would be a category error, because the loader has no MCP-specific knowledge and fixing MCP's exposure alone would be incoherent (redact USERNAME while the dump still echoes provider API keys). Fixing all of it is a legitimate config-loader hygiene change with its own blast radius (config.go + config tests + redaction semantics for a general diagnostics path), which is exactly the scope-creep the repo's small-PR and ADR governance culture resists — and the exposure is debug-gated, requiring an explicitly debug-enabled default logger that production does not run with. That rules out (a) for this PR. (c) is also wrong: leaving the statement unextended would understate what the new code genuinely does — the factory (mcp_token_resolution_skipped logs only server name + URL), the client (stores token/username in memory, logs nothing — verified against the full client.go), and the transport (no logging) do honor the invariant today, and the PR's new Basic path will honor it too. The honest statement is scoped, not abandoned.
So the wording changes, exactly as you ask — from unqualified to scoped "never": mcp-token-not-logged becomes: "MCPServer authentication credentials — tokens, Basic-auth usernames/passwords, and derived Authorization headers — must never be logged or serialized into diagnostics by the MCP credential plumbing (DI factory, MCP client, and transport)." The known exception is recorded in the ADR-067 §4 amendment, not in the invariant's prose: the config-loader debug dumps (config.go:147, 156-158) pre-date ADR-067, serialize all config values including MCP credentials when the default logger is at Debug level, and are out of scope for this change. And to keep triage-loop-closed honest rather than silently dropped, the amendment also records the fix path: the config-loader redaction is a tracked follow-up (dedicated hygiene change, not this PR) — the ADR carries the acceptance, so no one re-discovers it as a fresh gap.
One precision on scope I want explicit: this is not a new exception created by the PR — the same two dumps already violate the invariant's current plain text for the existing TOKEN field today (pre-PR). Option (b) merely documents a pre-existing, debug-gated gap and scopes the statement to what the MCP plumbing actually guarantees; the PR adds no new violation because the new USERNAME field flows through the same plumbing that already never logs. On your three options' mechanics: no config.go edits, no config-test changes, no new nonfix-catalog entry (the ADR is the record; a catalog entry would duplicate it — and the acceptance classes don't cleanly fit a debug-gated general diagnostics path anyway). Modelith impact: invariant statement reword + MCPServer attribute descriptions extended (auth enum text, new username attribute) → make modelith-render + modelith-check green; verify-adr-index unaffected (ADR-067 already indexed).
On your closing note: I'm ready to move to the closing. Construction path (option pattern, withBasicAuth(username), byte-identical Bearer), factory semantics (pass-through under basic, warn-and-skip gh/auto-only, stray-field normalization documented), seam shape (4-param (endpoint, username, token, timeout), single username != "" discriminator mirrored in connect()), validation posture (positive rule only), catalog duty (re-anchor resolveServerToken pin, no new entries), and now the invariant scoping — all settled.
VERDICT: The subject's diagnosis held up — the Bearer-only plumbing, blast radius, and the issue's own acceptance-criterion error (TestMCPServerConfig_Validate_AuthModes must change) were all verified correct against the code, and the env-interpolation-before-validation insight was a genuine addition. The plan did not hold in four places, each corrected under interrogation: the Auth-struct constructor (→ option pattern), the "warn-and-skip for basic" claim (→ pass-through, gh/auto-only), the redundant authMode seam param (→ single username != "" discriminator), and the over-claimed invariant extension (→ scoped "never" plus the config-loader debug-dump exception recorded in the ADR amendment). Recommendation: proceed with changes — the design is now coherent, minimal, and byte-identical-Bearer-safe; the final integrated plan is the opening statement as amended by A1–A4, with the ADR-067 §4 amendment carrying both adjudications (validate-rejects for basic, stray-field normalization, warn-and-skip scope) and the tracked config-loader hygiene follow-up.
POST-GRILL POSITION:
What I stand by (verified against the code and surviving the grill): The core diagnosis — bearerTokenTransport.RoundTrip hardcodes Bearer, MCPServerConfig has no Basic mode, the DI factory resolves one bearer token, and the Rovo server's streamable-HTTP + text/event-stream framing is transport-compatible, so the blocker is purely the auth scheme. The blast radius as scoped: mcp_config.go, client.go, mcp_factory.go, all three test suites, the domain-model YAML/MD, the ADR-067 §4 amendment, the README sample comment, and the INTENTIONAL_NON_FIXES pin re-anchor duty — including the two items the issue itself omitted (README comment; catalog pins at mcp_factory.go:60-65/:148-151). The issue's acceptance-criterion error — TestMCPServerConfig_Validate_AuthModes lists "basic" in its invalid set, so the "no regression" criterion is inaccurate and that test changes by design. The env-interpolation-before-validation insight: expandEnvHook runs during unmarshal (config.go:183, os.ExpandEnv at :187) before ValidateMCPServers, so an unset ${ENV_VAR} becomes empty and hard-fails validation for credential-requiring modes — already true for bearer today, applied identically to basic. Design consideration 1 (AUTH: "basic") over the generic HEADERS map; USERNAME + TOKEN-as-password naming; mcp.atlassian.com/v1/mcp non-/readonly → RequiresConsent: true, Serial: true per ADR-067 §8, hot-reload boundary unchanged per §10.
What I revised under interrogation (four corrections, all adopted): (1) Constructor: the Auth-struct signature → the established option pattern — withBasicAuth(username), positional token as the password, Bearer byte-identical by construction with 28 client_test.go occurrences source-untouched. (2) Factory semantics: "warn-and-skip for basic" → unconditional pass-through mirroring bearer (mcp_factory.go:130); mcp_token_resolution_skipped stays gh/auto-only; "unresolvable" under basic is a config-load error, not a factory state. (3) Seam: the redundant authMode param dropped → newClient func(endpoint, username, token string, timeout time.Duration) with the single username != "" discriminator mirrored in connect(); stray USERNAME under non-basic modes silently normalized by resolveServerToken (positive-rule validation only, matching the TOKEN-under-none tolerance). (4) Invariant: the unqualified "never" → scoped "never… by the MCP credential plumbing (factory, client, transport)", with the config-loader debug dumps (config.go:147, 156-158 — a pre-existing, debug-gated, MCP-unaware gap that serializes all secrets) recorded as a known exception in the ADR-067 §4 amendment plus a tracked follow-up, keeping triage-loop-closed without scope-creeping this PR.
Open questions I'm deliberately leaving to the user/architect (with my recommendation): (1) The config-loader debug-dump hygiene follow-up — I recommend filing it as a dedicated issue now so the ADR-recorded exception is not lost; a catalog entry is unnecessary (acceptance classes don't fit, and the ADR is the record). (2) Whether to add the mcp-basic-credentials-required invariant: my recommendation is no — the model has no per-mode validation invariant for bearer's token requirement either, so adding one for basic would be asymmetric; instead extend the MCPServer.auth attribute description and the scoped mcp-token-not-logged, keeping modelith surface minimal and avoiding a completeness "invariant never touched" warning. (3) Field naming USERNAME vs EMAIL — I hold USERNAME (general capability, Atlassian maps email→username), but if the user's only real target is Atlassian, EMAIL is more discoverable; low-stakes, user preference. (4) E2E verification needs live Atlassian credentials — who supplies them, and confirmation that the transcript + make check-full output go in the PR body per the issue's plan. The integrated plan is the opening statement as amended by A1–A4, with the ADR-067 §4 amendment carrying all four adjudications (basic mode + validate-rejects, stray-field normalization, warn-and-skip scope, config-loader exception + follow-up). I stand ready to proceed on that basis.