Grill Round — Issue #1417 (Architect × Griller)

Topic: gosharplite/tell-me-go#1417 — decompose complexity hotspots (configuration validation, memory hooks, context injection) Round: 10 questions, early conclusion allowed · Date: 2026-08-20 · Orchestrator: butler

Opening Statement — Architect (Subject)

All verification complete. Here is my opening statement.

Understanding

Issue #1417 asks to decompose four complexity hotspots so every function lands at CC ≤ 10 (the repo's complexity-threshold-policy invariant: docs/domain-model/quality.modelith.md — CC ≤ 10 is acceptable by policy; > 10 requires a refactor or a documented IntentionalNonFix). I verified every metric against the committed tree with the repo's own complexity tooling:

A. (*MCPServerConfig).validate — internal/domain/config/mcp_config.go:107, CC=20. A monolithic validation waterfall: transport exclusivity, URL-empty, ARGS/DIR/ENV-require-COMMAND, stdio bearer/basic conflict, TIMEOUT, a 6-case auth switch, and the positive bearer/basic credential rules. Its error strings and priority ordering are pinned by TestMCPServerConfig_Validate_StdioModeConflict in mcp_config_test.go ("the most specific error wins (each check returns immediately)") plus the _AuthModes, _BasicCredentialsRequired, _BearerTokenRequired, _StdioAuth tests.
B. Memory hook — internal/agent/memory/hook.go: AfterTurn (line 97, CC=22), FlushSession (line 300, CC=18), maybeLearn (line 236, CC=11). AfterTurn is a three-way classification (response / error-episode / GetLastModelTurn) plus a 3-case LEARN-tier dispatch with nil-client and nil-config guards; every path must return fail-open per ADR-068 (docs/adr/2026-09-automatic-plur-memory-integration.md §2, §3, §5). FlushSession carries the #1412 claim/restore lock discipline (snapshot-and-remove under one lock, MCP call outside the lock, restore-at-front on failure, retained/dropped counters) and the #1414 master-switch drain-and-drop gate.
C. Context injector — internal/agent/memory/injector.go: Transform (line 64, CC=18) and metadataIDs (line 237, CC=11). Transform is a fail-open pipeline (ADR-068 §1): nil-cfg, disabled content-driven strip with PersistHistory, nil-client, err-strip, result.Error-strip, defensive rune-safe trim, marker-keyed replace-in-place, observability. metadataIDs is a 4-case type-switch.
D. internal/tools/toolstest/fake_toolchain_runner_test.go: TestFakeToolchainRunner_ZeroDefaults (line 249, CC=38) and TestFakeToolchainRunner_PresetValues (line 36, CC=28) — 12-subtest tables per test, t.Parallel(), fresh fake per subtest.

Governance — the binding constraint. All eight targets are ACCEPTED entries in docs/architect/INTENTIONAL_NON_FIXES.md (see mcp_config.go:107, hook.go:97/236/300, injector.go:64/237, fake_toolchain_runner_test.go:36/249), so this issue reverses catalogued decisions — exactly the "Drift Policy" and "Coordination rule" case: any legitimate catalog change (add/remove/re-anchor/CC re-verify) edits the partition test in the same PR. The partition gate is TestVerifyNonFixCatalog in internal/tools/analysis/real_nonfix_catalog_test.go (note: the issue's governance section cites internal/tools/analysis/nonfix_catalog_partition_test.go, which does not exist — the real file is real_nonfix_catalog_test.go). It gathers live complexity, requires every CC>10 function to be pinned by an ACCEPTED catalog entry at its exact file:line, and asserts exact equality against an expectedCataloged map that currently contains all eight hotspots with their line/CC. make verify-nonfix-catalog (wired into check/check-full) also runs TestVerifyCoveragePinsMatchLiveCatalog, which pins coverage intervals including hook.go:89-93 (BeforeTurn/OnPhaseTransition no-op stubs) and hook.go:361-363 (the FlushSession engrams-empty belt-and-suspenders) — the latter will drift under the B refactor. make check-full is the hard gate (fmt/tidy/build/lint/verify-*/modelith/vulncheck/test/dead-code/coverage/test-race).

Implementation Plan

Task 1 — A (standalone): In internal/domain/config/mcp_config.go, keep validate(name) as a thin orchestrator that calls private sub-validators in the exact current order: validateTransportShape (mutual exclusion + URL-empty), validateCommandFields (ARGS/DIR/ENV require COMMAND), validateAuthTransportConflict (stdio bearer/basic), validateExecutionLimits (TIMEOUT), validateAuthMode (the switch), validateCredentials (bearer TOKEN, basic USERNAME+TOKEN). The split between conflict/switch/credentials is forced by the pinned ordering — the TIMEOUT check fires between the stdio-auth conflict and the auth switch, so the issue's proposed four methods cannot preserve priority as-is; six helpers can. To meet the AC "each sub-validator CC ≤ 5", the 6-case switch must become a set-lookup (validAuthModes map or slice-contains) with the identical error message — a switch alone is CC=7. Every error string is copied verbatim; mcp_config_test.go's table tests stay green unchanged, proving message and ordering preservation.

Task 2 — D (independent): In fake_toolchain_runner_test.go, group the 12 subtest closures into domain-scoped helpers (e.g., testAnalysisRunnerMethods, testCoverageRunnerMethods) invoked from the two table drivers; preserve t.Parallel(), the per-subtest fresh &FakeToolchainRunner{}, assertCallLog identity checks, and the Called()-based ZeroDefaults assertions. Each helper lands at CC ≤ 10 by construction (3–4 subtests per helper). I flag this as the weakest-premise item (see Risks).

Task 3 — C (injector, same package as B): In injector.go, extract resolveUserPrompt(req) (thin wrapper over the existing lastUserText), fetchEngramPayload(ctx, task, cfg) (tools.ToolResult, bool) (args/scope build + CallTool + err-strip + result.Error-strip — strip only, never PersistHistory, preserving ADR-068 §1.5 "current recall or nothing"), and applyMemoryTransformation(req, block, budget) (defensive trim via truncateToBytes + insertMemoryBlock marker-keyed replace-in-place). Transform shrinks to the nil-cfg/disabled-strip/nil-client guards plus three calls, ~CC 7–8. For metadataIDs, extract the []interface{} conversion loop into a named helper (e.g., metadataIDsFromInterfaceSlice), dropping the switch function to CC ≤ 8. New helper tests are appended to the end of injector_test.go so the ACCEPTED pin at injector_test.go:109 (TestInjectorEnabledInsert, CC=22) does not drift.

Task 4 — B (hook, most delicate): In hook.go, decompose AfterTurn into: (1) buildEpisode(turn, err) (episode, bool) — the three-way classification with the exact rules: branch (iii) uses h.history.GetLastModelTurn under the detached 3s ctx (never Turn.State.LastError — stale on the empty-response retry path), transient skip in (ii), text-empty skips in (i)/(iii); (2) tier dispatch to handleCaptureTier / handleBatchTier / handleFullTier (or a dispatchTier), preserving capture→plur_capture, batch→bufferAppend, full→bufferAppend+maybeLearn. Keep the master-switch gate (#1414), the turn-scoped dedupe, and nil-client Warn in AfterTurn itself. For maybeLearn, extract the lock-guarded slot claim (claimLearnSlot: sha256 exact-match dedupe then flood bound, increment only after both pass, unlock before the MCP call) — preserving the session:/mode: tags convention, scope, detached ctx, fail-open log, and recordWrite. For FlushSession, extract claimEpisodes(sessionID) (master-switch drain-and-drop + lock claim + empty-drain delete), buildEngramPayload(episodes, scope) (keeping the engrams-empty belt-and-suspenders), restoreOnFailure, and finalizeOnSuccess — preserving #1412 exactly: claim snapshots and removes under one lock, MCP call outside the lock, restore-at-front on failure with retained/dropped on the Warn, success deletes the entry only when empty else resets dropped, nil-client drain-and-drop with no restore (ADR-068 §5). The concurrency proofs (TestHookFlushConcurrentRestoreAndSuccessAppend, TestHookBatchRetainOnFailure, TestHookFlushDropsStaleBufferWhenDisabledMidSession, TestHookFlushNilClientDrainAndDrop) stay green unchanged. New tests appended at the end of hook_test.go so pins at hook_test.go:61/247/359 (TestHookCaptureBranchI CC=13, TestHookBatch CC=22, TestHookFull CC=21 — all out of scope, remaining ACCEPTED) do not drift.

Governance, folded into every task's commit (coordination rule): (a) mark each of the eight hotspot entries RESOLVED in INTENTIONAL_NON_FIXES.md with a "Complexity after fix" note, following the existing browser.go handleActionKeys CC=15 → RESOLVED precedent; (b) remove the eight entries from expectedCataloged in real_nonfix_catalog_test.go (they vanish from the partition automatically once CC ≤ 10 — the require.Equal is exact, so any extracted helper that still measures > 10 fails the gate and must be re-split); (c) re-anchor the coverage pin hook.go:361-363 to wherever the engrams-empty belt-and-suspenders lands, and re-verify hook.go:89-93 and flock_unix.go:52-53 are untouched; (d) run make verify-nonfix-catalog locally after each task and make check-full at the end (generous timeout — test-race is the long pole).

Risks and Open Questions

Is CC ≤ 10 the right target for already-ACCEPTED structural code? Every target carries an ACCEPTED rationale in its own class (dispatch-structural/structural-guard for validate/AfterTurn/Transform/metadataIDs, test-complexity for the fake_toolchain_runner tests). The decompose-validate/AfterTurn/Transform work is genuinely SRP-positive (each extracted step is independently testable and the fail-open semantics become inspectable per stage), but hotspot D is metric-chasing: the catalog's own rationale says splitting "would fragment a coherent coverage matrix," and moving subtest closures into helpers relocates CC without reducing cognitive load. The issue should state why the ACCEPTED test-complexity decision is now being reversed — otherwise the catalog churn is unjustified.
The AC "each sub-validator CC ≤ 5" vs. the 6-case auth switch. A switch on six modes is CC=7 alone; hitting ≤5 requires converting it to a map/set lookup. That is a small behavioral-surface change (message identical) but a design decision the issue doesn't mention — either the AC is softened to the policy threshold (≤10) or the map conversion is mandated. Same tension in AfterTurn's tier dispatch.
Wrong partition-test path in the issue. Governance section cites internal/tools/analysis/nonfix_catalog_partition_test.go; the actual gate is real_nonfix_catalog_test.go (TestVerifyNonFixCatalog). A coder following the issue literally would edit a non-existent file. The plan must name the real file.
The lock-lifecycle trap. FlushSession's #1412 claim/restore discipline and #1414 drain-and-drop are the product of two concurrency bug-fixes. A "coordinator" extraction that reorders claim→I/O→restore, restores at the back instead of the front, or drops the nil-client no-restore rule reintroduces double-write or retention bugs. The existing concurrent tests are the safety net, but they don't cover every interleaving — the extraction must be line-faithful, not "equivalent-looking."
Coverage-pin drift beyond the obvious. TestVerifyCoveragePinsMatchLiveCatalog pins hook.go:89-93 and hook.go:361-363; the B refactor moves 361-363 and may shift 89-93 if any new method is inserted above AfterTurn. Also the memory package tests are themselves catalog-pinned at specific lines — appending tests at file end is mandatory, or re-anchor the TestHookBatch/TestHookFull/TestHookCaptureBranchI/TestInjectorEnabledInsert pins in the same commit.
Underspecified: (a) the issue proposes four validators but the pinned ordering forces six — fine, but the AC should say "modular validators," not four; (b) hotspot D's "domain-scoped helper" grouping (which of the 12 methods belong to "coverage" vs "analysis"?) is left to the implementer; (c) whether catalog entries are marked RESOLVED vs. removed — the repo precedent (handleActionKeys) is RESOLVED-with-metrics, which I recommend for auditability; (d) the issue claims 97.4% coverage — consistent with the catalog narrative but not independently re-verified here.
Hidden coupling: fake_toolchain_runner.go (the fake itself) is untouched, but its Calls log is shared-mutable state; the refactor of the tests must not introduce helpers that share a fake across subtests (each subtest must keep its fresh fake) or the TestFakeToolchainRunner_ConcurrentAppends/CallOrder invariants silently weaken.

My recommendation: proceed with A, C, B (production decompositions are sound and behavior is fully pinned by existing tests), treat D as a judgment call — either drop it with the test-complexity entries left ACCEPTED, or split it into its own commit so its catalog reversal is separately reviewable — and amend the issue's AC for the auth-switch to the map-lookup requirement explicitly.

Q1 — Griller

I now have everything verified. All eight CC values and line numbers check out; the coverage pins (hook.go:89-93, hook.go:361-363, flock_unix.go:52-53) are real; the partition gate is real_nonfix_catalog_test.go with an exact-equality expectedCataloged map containing all eight hotspots; the issue's cited nonfix_catalog_partition_test.go does not exist; and I've read the exact catalog rationales for every target — including one misquotation in the subject's own Risk #1 that I can pin.

Q1:

Q1: Your opening concedes the issue reverses eight ACCEPTED catalog decisions and that the repo's own complexity-threshold-policy makes CC>10 acceptable when documented — which every one of these is — yet you recommend executing A, B, and C anyway. Before you answer, two verification findings:

Your Risk #1 misquotes the catalog. You write that hotspot D's entries say splitting "would fragment a coherent coverage matrix." I searched the D entries in INTENTIONAL_NON_FIXES.md — TestFakeToolchainRunner_PresetValues (CC=28) and TestFakeToolchainRunner_ZeroDefaults (CC=38) contain no such phrase; their actual rationale is "subtest enumeration and assertion boilerplate, not branching business logic." The "fragment a coherent coverage matrix" line belongs to TestHydrateMediaAssets and TestToSDKSchema_EmptyType_OmitsTypeKey — a different acceptance-class cohort. Your evidence for D's "metric-chasing" verdict is therefore not verbatim, and it materially weakens your case for dropping D: the real rationale is weaker than the one you quoted.
The catalog rationales are asymmetric in exactly the way your plan ignores. The validate entry (which you recommend proceeding with) ends with "A future extraction refactor is possible but is tracked, not this PR" — an explicit carve-out you could legitimately claim #1417 fulfills. The AfterTurn, FlushSession, maybeLearn, Transform, and metadataIDs entries carry no such carve-out — they state "Extracting would fragment a coherent sequential function for cosmetic CC reduction," and for AfterTurn additionally note the fail-open structure is "mandated by ADR-068 §2." The issue itself offers no rebuttal of any of these rationales — only the CC numbers, which the policy already deems acceptable-when-documented.

So, given the catalog-authoritative and catalog-architect-curated invariants in quality.modelith.md: on what substantive basis do you overturn the ACCEPTED decisions for the five B/C functions — not "CC ≤ 10," but a reason the original architect's rationale is now wrong? Do you stand by proceeding with B and C as filed, or should the plan's first deliverable be an architect adjudication amending the catalog — a step this issue never mandates and your task list never includes?

A1 — Architect

Q1 answer

Verification finding #1 — conceded, with the required retraction. You are right and I was wrong: neither D entry contains "would fragment a coherent coverage matrix." Verbatim from INTENTIONAL_NON_FIXES.md: TestFakeToolchainRunner_PresetValues says "CC=28 comes from subtest enumeration and assertion boilerplate, not branching business logic — each subtest is a one-liner closure," and TestFakeToolchainRunner_ZeroDefaults says "CC=38 comes from subtest enumeration and assertion boilerplate, not branching business logic." The "fragment a coherent coverage matrix" sentence belongs to TestHydrateMediaAssets and TestToSDKSchema_EmptyType_OmitsTypeKey — a different cohort, as you found. I retract the attribution and withdraw my "drop D" recommendation, which rested on that misquote. The corrected rationale changes the D calculus in your direction: the catalog asserts no harm from splitting D — only that the CC is enumerative — so D is the least entrenched of the four, and the "metric-chasing" label was an over-strong characterization built on a sentence that isn't there. One refinement that actually strengthens the case for executing D: the entries classify D under test-complexity, whose defining condition is "splitting would duplicate expensive setup" — but D is twelve independent, parallel one-liner subtests with no shared state and no expensive setup to duplicate (each builds a fresh &FakeToolchainRunner{}). The class doesn't fit the test's actual structure. So D is either trivially executable or trivially leaveable — but my earlier "drop it" verdict is withdrawn.

Verification finding #2 — conceded, with one precision. The asymmetry is real and I verified it: only the validate entry (mcp_config.go:107) carries the carve-out — "A future extraction refactor is possible but is tracked, not this PR" — so #1417 literally fulfills architect-tracked work for A. None of the five B/C entries carry any such carve-out. One precision for the record: the exact sentence "Extracting would fragment a coherent sequential function for cosmetic CC reduction" is the AfterTurn entry's wording; FlushSession, maybeLearn, Transform, and metadataIDs invoke the same structural-guard/dispatch classes (renderHistory, RecoveryStep.Process, handleDomainEvent) without that sentence and without any carve-out. And AfterTurn alone adds "the fail-open guard structure … is mandated by ADR-068 §2." The substance of your finding stands: A is pre-blessed; B/C are not; the issue offers no rebuttal of any B/C rationale.

The core question — do I stand by proceeding with B/C as filed? No. I revise. The CC number alone is not a basis for overturning anything: complexity-threshold-policy explicitly deems documented CC>10 acceptable, and all five entries are documented. Executing B/C as filed would be an agent overriding catalog-architect-curated on exactly the ground the policy says is insufficient. The issue being authored by the repo owner does not cure this — the issue body is a task list, not an adjudication; it cites numbers, not reasons, and the invariants in quality.modelith.md make the catalog the authoritative answer to "why is this not fixed?" until an architect records otherwise. Your finding that my task list omitted the adjudication step is correct; that was the defect. The plan's first deliverable must be the architect adjudication: a recorded decision (maintainer issue comment or catalog amendment) that re-adjudicates the five B/C entries before any B/C code moves, with the catalog and partition updates landing in the same commit as the refactor it authorizes.

That said, the substantive bases for the adjudication to record do exist — they are reasons the original rationales are now wrong, not "CC ≤ 10":

Mandate-vs-shape conflation (AfterTurn, Transform). ADR-068 §2/§1 mandate fail-open semantics: return on every path, log and ignore memory errors. They do not mandate a 22-CC (or 18-CC) monolith. The decomposition preserves the semantics exactly — every extracted helper returns on every path, and AfterTurn/Transform remain void/orchestrating functions that return on every path. The catalog's "mandated by ADR-068 §2" clause is wrong as written: it treats a mandated behavioral property as if it mandated the monolithic shape. That is a specific, checkable error in the rationale, not a preference.
Changed circumstances since acceptance — the #1410/#1412/#1414 accretions. AfterTurn was accepted at CC=21, grew to 22; FlushSession accepted at 13, grew 16→18; and the cost has migrated into the test layer: TestHookBatch accepted at 14, re-measured 20, now 22; TestHookFull accepted at 15, now 21 — the second- and third-highest test-CC pins in the entire expectedCataloged map (after ZeroDefaults at 38). The "cosmetic" judgment was made on the earlier shape; the catalog's own drift policy re-measures CC when the referenced function changes but never re-adjudicates whether the acceptance still holds — the tests now pay the price of the monolith, which is exactly the cost the "cosmetic" verdict did not anticipate. That is the strongest basis for B, and it is a changed-fact basis, not a number.
Honest floor on C: metadataIDs is the weakest overturn in the whole issue — a 4-case type-switch at CC=11 whose rationale fits the repo's own dispatch-structural class perfectly (the same class that keeps handleDomainEvent CC=12 ACCEPTED). I would expect the architect to leave it ACCEPTED or refactor it only as a byproduct of the Transform work; if the adjudication confirms it, I will not fight it. Transform rides the mandate-vs-shape argument.

Sequencing, revised: Deliverable 0 = architect adjudication of the five B/C entries (record the supersession rationale, amend the catalog, update the partition test in the same change). Then A proceeds unconditionally (pre-blessed by the carve-out). B proceeds only if the adjudication accepts the changed-circumstances/mandate-vs-shape basis. C proceeds with Transform on the same basis; metadataIDs at the architect's call. D proceeds as a risk-free mechanical refactor or is deferred at the architect's call — my "drop it" position is withdrawn. If the architect declines to re-adjudicate any entry, that entry stays ACCEPTED and the issue narrows accordingly.

Q2 — Griller

Verified against mcp_config_test.go — and I've found the subject's Task-1 methodology claim is overstated in a way that matters for the A-ordering thread. The evidence:

TestMCPServerConfig_Validate_StdioAuth is the only test that pins a precedence: "COMMAND with bearer and empty token: mode conflict wins" asserts NotContains(err, "must not be empty when AUTH") — stdio-conflict before the credential rules.
No test constructs a dual-violation config for the pair that actually forces the subject's six-helper split: nothing combines Timeout: -1 with an invalid Auth (TIMEOUT-vs-auth-switch position), nothing combines no-URL with Args (URL-empty-vs-ARGS position), nothing combines Command+bearer with Timeout: -1. Every other test isolates a single violation and pins a message string only.

Q2: Your Task 1 says the split "is forced by the pinned ordering — the TIMEOUT check fires between the stdio-auth conflict and the auth switch" and that "mcp_config_test.go's table tests stay green unchanged, proving message and ordering preservation." Both halves need scrutiny against the file I just read:

The TIMEOUT-between-conflict-and-switch position is real in the code — but it is not pinned by any test. TestMCPServerConfig_Validate_StdioAuth pins exactly one precedence (mode-conflict over missing-credentials); _StdioModeConflict, _AuthModes, _BasicCredentialsRequired, _BearerTokenRequired each isolate a single violation and pin a message. No test feeds a config with Timeout: -1 and Auth: "bogus" — so a refactor that silently moved the auth-switch check before the TIMEOUT check would leave every existing test green. "Proving ordering preservation" is not what those tests prove; they prove message preservation and one stdio-specific precedence. The phrase "pinned ordering" in your opening ("its error strings and priority ordering are pinned by TestMCPServerConfig_Validate_StdioModeConflict") is likewise only half true — the test's comment says "the most specific error wins," but the tests don't exercise the pairs that would distinguish the order.
Consequence: your "six helpers, not the issue's four" argument rests entirely on preserving a documented-but-untested precedence. Either (a) you revise the claim to "six helpers preserve the documented order," and Task 1 adds dual-violation pinning tests (e.g., {URL, Auth:"bogus", Timeout:-1} → TIMEOUT wins; {Args:[...]} no URL → URL-empty wins; {Command, Auth:"bearer", Timeout:-1} → mode-conflict wins) so the ordering you're preserving is actually enforced — or (b) you concede the ordering isn't behaviorally pinned and the issue's four-method split is acceptable, which collapses the "six helpers forced" argument. Note (a) also touches your Risk #2: once you're adding pinning tests for the sub-validator boundaries, the CC ≤ 5 AC that forces the auth-switch → set-lookup conversion is also an unpinned behavioral surface — the identical error message can be preserved, but nothing in the current suite proves the set-lookup returns it for all six modes plus the invalid case.

Which do you stand by — and if (a), does Task 1 now include those dual-violation pinning tests in the same commit as the decomposition, as your own coordination rule would demand?

A2 — Architect

Q2 answer

Verification — both findings conceded, verified against mcp_config_test.go. (1) TestMCPServerConfig_Validate_StdioAuth is the only test asserting a precedence: the "COMMAND with bearer and empty token: mode conflict wins" case pins errContains: "requires an HTTP endpoint" plus the loop-level assert.NotContains(err, "must not be empty when AUTH") — conflict-before-credentials, exactly one pairing. (2) No test feeds a dual-violation config: _Validate isolates URL-empty and TIMEOUT on otherwise-clean configs; _StdioModeConflict isolates exclusivity/URL-empty/ARGS-DIR-ENV each on a clean base; _AuthModes/_BasicCredentialsRequired/_BearerTokenRequired isolate auth violations. Nothing combines Timeout:-1 with Auth:"bogus", nothing combines no-URL with Args, nothing combines Command+bearer with Timeout:-1. I retract my opening's "its error strings and priority ordering are pinned by TestMCPServerConfig_Validate_StdioModeConflict" and my Task-1 claim that the tests "stay green unchanged, proving message and ordering preservation." The suite proves message preservation (mostly Contains, not even exact strings — the invalid-auth case asserts only Contains("AUTH"), so the switch's full five-mode message is itself unpinned) and exactly one stdio precedence. The rest of the order is documented in code comments but not behaviorally pinned. A refactor moving the auth switch before TIMEOUT would leave every existing test green — correct.

Which do I stand by — (a), with the claim revised, and the pins added. I do not collapse to (b), but the ground shifts: six helpers is not forced by a test-pinned ordering. It is forced by the issue's own contract. The issue body says "Preserve exact error message strings and priority ordering to guarantee backwards compatibility with existing table-driven tests" — that preservation clause is the constraint, and the four-method split cannot honor it. With the issue's four methods (validateAuthMode bundling conflict+switch+credentials; validateExecutionLimits for TIMEOUT), the orchestrator must choose: auth-before-limits makes the switch fire before TIMEOUT (changes {URL, Auth:"bogus", Timeout:-1} from TIMEOUT to AUTH error), or limits-before-auth makes TIMEOUT fire before the stdio conflict (changes {Command, Auth:"bearer", Timeout:-1} from conflict to TIMEOUT error). Both preserve the one pinned pairing (conflict>credentials, which survives as long as validateAuthMode orders conflict before credentials internally), but each changes one unpinned pairing. So: the four-method split is not test-blocked — it is contract-blocked by the issue's own "preserve priority ordering" sentence, which is a softer but real constraint. Under (a) I revise the claim to: six helpers preserve the documented order, and Task 1 now makes that order enforced rather than assumed.

Task 1 amendment — the dual-violation pins land in the same commit as the decomposition (coordination rule, as you demand): {URL, Auth:"bogus", Timeout:-1} → TIMEOUT error wins; {Args:["--foo"]} with no URL and no Command → URL-empty wins; {Command:"npx", Auth:"bearer", Timeout:-1} → mode-conflict wins; the existing conflict>credentials NotContains pin retained. These should assert the full exact error strings for the ordering-sensitive cases — deliberately stronger than the current suite's Contains pattern — because the invalid-auth message (five quoted modes) is currently unpinned even in content, and the issue claims exact-string preservation. This also closes your set-lookup point: once the decomposition converts the 6-case switch to a set-lookup to meet the AC's CC ≤ 5, _AuthModes (all six valid modes + invalid, currently green) plus the new TIMEOUT-wins pin jointly pin the set-lookup's behavioral surface — the identical message is preserved and proven for every mode and the invalid case, not assumed.

One honest caveat in case the architect prefers the issue's four methods: that choice is defensible only as a deliberate, recorded behavior change — one dual-violation diagnostic will flip, the issue's "preserve priority ordering" clause must be amended or adjudicated, and the new order must be pinned in the same commit. My recommendation remains six helpers + pins, because it keeps the refactor inside the issue's own preservation contract with zero behavioral delta; but I concede the four-method split is not blocked by any test — it is blocked only by the issue's wording, which the architect can amend.

Q3 — Griller

Verified the full FlushSession state machine and its test coverage. The pinned interleavings are: disabled drain-and-drop, empty-drain delete, nil-client drain-and-drop, the !exists restore branch (TestHookFlushConcurrentRestoreAndSuccessAppend subtest 1), and success-path append-keeps-entry. What is not pinned anywhere — and what the subject's preservation enumeration omits — is the engrams-empty belt-and-suspenders disposition: after claim() has removed the episodes, that check fires and returns with no restore, no map delete, no recordWrite — the claimed episodes are dropped and the empty entry is left in the map for the next flush's empty-drain to clean up.

Q3: Task 4's FlushSession decomposition — you enumerate what it preserves (claim/remove under one lock, MCP outside the lock, restore-at-front with retained/dropped, success-delete-when-empty-else-reset, nil-client drain-and-drop no-restore) and cite the four concurrency proofs as the safety net. Two findings on the seams your four helpers create:

The engrams-empty belt-and-suspenders (hook.go:361-363) is a claim-lifecycle decision, not a payload-mapping decision — and it is the one disposition in the entire function that is neither in your preservation list nor pinned by any test. It fires after claim() has removed the episodes; its current disposition is: return, no restore, no map delete, no recordWrite — the claimed episodes are silently dropped and the empty entry survives for the next flush's empty-drain to delete. TestHookFlushEmptyDrainNoop pins the distinct episodes == 0 path, not this one, and skip-at-append makes this path unreachable, so it has zero behavioral coverage — only the catalog coverage-gap pin at 361-363, which your plan (c) re-anchors but which is a prose/interval match, not a behavioral assertion. Your plan says buildEngramPayload "keeps" the belt-and-suspenders — but if the check lives inside the payload builder, the disposition of the already-claimed episodes (restore / drop-and-leave-entry / drop-and-delete) is decided in a helper that holds neither the lock nor the map, and the orchestrator never sees the signal. Where does that disposition decision live in your four-helper split — and does Task 4 add a behavioral pin for it, or do you accept that the belt-and-suspenders survives only as an unpinned line whose disposition is whatever the orchestrator's control flow happens to produce?
For the record, the !exists restore branch (hook.go:379-382 — re-creating the map entry when a concurrent flush deleted it between claim and failure) is pinned, by TestHookFlushConcurrentRestoreAndSuccessAppend subtest 1 — your enumeration omitted it, but the test enforces it, so a restoreOnFailure extraction that assumes the entry exists will fail that test, which is the correct outcome. I'm accepting that seam as covered.

The question is #1: specify the engrams-empty disposition in the decomposition, and state whether it becomes a pinned behavior in Task 4 or remains an unpinned, coverage-re-anchored belt-and-suspenders line.

A3 — Architect

Q3 answer

Finding #2 accepted. The !exists restore branch is real and pinned: on the failure path, h.mu.Lock(); b, exists := h.buffers[sessionID]; if !exists { b = &sessionBuffer{}; h.buffers[sessionID] = b }; b.restore(episodes); dropped := b.dropped; h.mu.Unlock() — re-creating the map entry when a concurrent flush deleted it between claim and failure. My preservation enumeration omitted it; TestHookFlushConcurrentRestoreAndSuccessAppend subtest 1 enforces it. The restoreOnFailure helper contract must therefore be: acquire lock → re-create &sessionBuffer{} if absent → restore-at-front → capture dropped (read inside the lock, surfaced in the Warn after unlock). A helper assuming the entry exists fails that test, correctly.

Finding #1 — verified, and I can sharpen it. The engrams-empty belt-and-suspenders at hook.go:361-363 is not merely skip-at-append-unreachable; it is provably dead by the loop invariant: engrams := make([]engramPayload, 0, len(episodes)) and every episode appends exactly one entry (Statement: ep.Text unconditionally), so len(engrams) == len(episodes) always — and len(episodes) == 0 already returns at the empty-drain path above it. The check can never fire; it is pure defensive guard against a future edit breaking the 1:1 mapping. Its current disposition, traced: after claim() removed the episodes, return with no restore, no delete(h.buffers, sessionID), no recordWrite — claimed episodes dropped, empty entry left for a next flush's empty-drain to delete. Your characterization is exact.

Where the disposition decision lives — retracting my earlier phrasing. My Task-4 plan said buildEngramPayload "keeps the belt-and-suspenders" — wrong, and your seam is the reason. A payload-mapping helper holding neither the lock nor the map must not decide claim disposition. The split is: buildEngramPayload(episodes []episode, scope string) []engramPayload is pure (no lock, no map, no side effects); the guard moves to the orchestrator, immediately after the build call:

engrams := buildEngramPayload(episodes, scope)
if len(engrams) == 0 { // belt-and-suspenders — never send engrams: []
    return
}

The disposition decision — drop-and-leave-entry, no restore, no map delete, no recordWrite — now lives at the claim-lifecycle level in FlushSession, visibly, byte-for-byte today's behavior. The helper stays unit-testable and the orchestrator owns every claim-disposition branch (empty-drain delete, nil-client drain-and-drop, engrams-empty drop, failure restore, success finalize) in one place.

Pinning — direct answer: Task 4 does not add a behavioral pin for the disposition, because none is possible without changing behavior. Driving the branch requires breaking the 1:1 builder invariant, and restructuring the builder to filter would itself be a behavior change on a path that is provably dead. What Task 4 adds instead, in the same commit: (i) a helper-contract unit pin — buildEngramPayload(nil, scope) and buildEngramPayload(empty, scope) return empty, and a non-empty input returns 1:1 with Statement/Tags/Scope mapping — so the guard's precondition is pinned even though the guard itself cannot fire from FlushSession; and (ii) the catalog coverage-gap entry plus the TestVerifyCoveragePinsMatchLiveCatalog row re-anchored from hook.go:361-363 to the guard's new line, same commit, exactly as my plan (c) already said — it remains a documented defensive guard, consistent with the catalog's defensive-guard class. I will not claim a behavioral pin; the disposition is specified (explicit orchestrator-level return, documented, identical to today) rather than pinned — the honest maximum for a provably dead branch.

One alternative on the table: if the architect judges the future-proofing value nil, dead-code elimination is cleaner — delete the branch, remove the catalog entry and the 361-363 pin row in the same commit. My recommendation is keep-and-specify, because "never send engrams: []" is an invariant guard against future builder edits, which is precisely what the defensive-guard acceptance class exists for, and deleting it buys nothing measurable.

Q4 — Griller

The Q3 answer checks out — the loop-invariant proof is exact (len(engrams) == len(episodes) always; the episodes == 0 case already returned at the empty-drain), the disposition trace is correct, and moving the guard to the orchestrator fixes the seam. Now for C, I've re-read Transform against the subject's Task-3 helper contracts and found an internal contradiction in the fetchEngramPayload signature.

Current Transform has four strip-capable sites with distinct semantics:

Disabled path: stripMemoryBlock(req) + req.PersistHistory = true (persisted strip).
Transport error (err != nil): Warn memory_injection_failed with err as the value, then strip — no persist.
result.Error != nil: Warn memory_injection_failed with result.Error, then strip — no persist.
Overflow (maxBody < 0): Debug memory_block_overflow, then strip — no persist, and crucially it is a no-insert return.

The subject's Task 3 declares fetchEngramPayload(ctx, task, cfg) (tools.ToolResult, bool) and describes it as doing "CallTool + err-strip + result.Error-strip — strip only, never PersistHistory." But that signature takes no req (so it cannot call stripMemoryBlock) and returns no error value (so it cannot log the specific err vs result.Error that the two Warn sites today distinguish). It is the same responsibility/seam bug we just grilled out of Task 4, one question ago.

Q4: Specify the fetchEngramPayload contract so the four strip sites keep their semantics. Concretely: if the stripping happens inside fetchEngramPayload, the signature must take req — and then the "strip only, never PersistHistory" claim must be reconciled with site 1, where the disabled-path strip must set PersistHistory = true: does the disabled-path strip stay in Transform (as your Task-3 wording implies — "Transform shrinks to the nil-cfg/disabled-strip/nil-client guards"), making fetchEngramPayload's no-persist strip the only other strip site? If instead the stripping happens in Transform on ok == false, then fetchEngramPayload must return the error value (e.g., (tools.ToolResult, error, bool) or (tools.ToolResult, error)) so the memory_injection_failed Warn can still distinguish err from result.Error — the two branches today log different values under the same key, and the E2E assertion surface (ADR-068 §8, the Info-level log + dead-tool notice) may not care, but the in-process Warn contract does. And where does site 4 (the maxBody < 0 overflow strip-and-return, a no-insert fail-open) live — inside applyMemoryTransformation as a strip-without-insert, or back in Transform? Your plan describes applyMemoryTransformation(req, block, budget) as "defensive trim via truncateToBytes + insertMemoryBlock," which has no slot for a no-insert return; if it inserts unconditionally, the overflow path silently changes behavior from strip-and-return to insert-a-truncated-empty-block.

One question: pin down the exact ownership of the four strip sites across Transform / fetchEngramPayload / applyMemoryTransformation — who strips, who logs which error value, and which sites may set PersistHistory — so the plan's C contract doesn't ship the same seam bug Task 4 just had.

A4 — Architect

Q4 answer

The contradiction is real — conceded. My Task-3 fetchEngramPayload(ctx, task, cfg) (tools.ToolResult, bool) was the same seam bug we just grilled out of Task 4: no req, so it cannot strip; no error value, so it cannot distinguish the two memory_injection_failed values. The four-site inventory is verified against injector.go (site 1: !cfg.Enabled → strip + req.PersistHistory = true gated on strip's return; site 2: err != nil → Warn("memory_injection_failed", "error", err) + strip; site 3: result.Error != nil → Warn("memory_injection_failed", "error", result.Error) + strip; site 4: maxBody < 0 → Debug("memory_block_overflow", "budget", …, "block_bytes", len(block)) + strip, no insert). Site 1 is the only persist-capable site; the other three ignore strip's return value, exactly as today. Retracting the old signature; the corrected contract follows.

Ownership table — who strips, who logs, who persists:

Site	Lives in	Strips	Logs	Log value	PersistHistory
1. Disabled	`Transform`	`Transform`	none (silent)	—	YES — the only site
2. Transport err	`fetchEngramPayload`	`fetchEngramPayload`	`fetchEngramPayload`	`Warn memory_injection_failed` = `err`	NO
3. `result.Error`	`fetchEngramPayload`	`fetchEngramPayload`	`fetchEngramPayload`	`Warn memory_injection_failed` = `result.Error`	NO
4. `maxBody < 0`	`applyMemoryTransformation`	`applyMemoryTransformation`	`applyMemoryTransformation`	`Debug memory_block_overflow` (`budget`, `block_bytes`)	NO

Nil-client stays in Transform (Warn memory_client_unavailable, no strip, no persist); success-path observability moves to observeInjection (Warnings append + Info memory_injected / Debug memory_injected_no_ids), preserving ADR-068 §8's Info surface.

Contracts:

func (t *plurInjector) resolveUserPrompt(req *sessctx.ContextRequest) string          // pure, over lastUserText
func (t *plurInjector) fetchEngramPayload(ctx context.Context, req *sessctx.ContextRequest,
    cfg *config.MemoryConfig, task string) (tools.ToolResult, bool)                    // sites 2+3 inside
func (t *plurInjector) applyMemoryTransformation(req *sessctx.ContextRequest,
    result tools.ToolResult, budget int) bool                                          // site 4 inside; true = inserted
func (t *plurInjector) observeInjection(req *sessctx.ContextRequest, result tools.ToolResult, task string)

Transform becomes: cfg nil → return; !cfg.Enabled → site 1 (strip, PersistHistory on strip-true) → return; nil-client → Warn → return; task := resolveUserPrompt(req); result, ok := fetchEngramPayload(ctx, req, cfg, task) → if !ok { return nil }; if !applyMemoryTransformation(req, result, cfg.InjectBudget) { return nil }; observeInjection(req, result, task); return nil. CC ≈ 6–7, every helper ≤ 5.

Direct answers:

Site 1 stays in Transform — yes, as my Task-3 wording implied, and it is the only site that may set PersistHistory. The "strip only, never PersistHistory" claim is therefore scoped precisely to the fetch-failure sites (2, 3) — they never persist — and there is no contradiction once site 1 is owned by Transform, not fetchEngramPayload. fetchEngramPayload never touches PersistHistory.
Design X chosen (strip inside fetchEngramPayload), not Design Y. The (tools.ToolResult, bool) works because the distinct error values are consumed where they are produced: the two Warns stay co-located with their strip, same key, same values (err vs result.Error) — the in-process Warn contract is preserved verbatim. Pushing strip+log back to Transform on ok == false would force the (tools.ToolResult, error, bool) return you name, split each failure disposition across two functions, and re-inflate Transform's branch count — the same anti-pattern Task 4 just fixed. This is the consistent principle: a helper owns its failure disposition when it holds all the state it mutates — fetchEngramPayload holds req, cfg, logger, client — exactly as claimEpisodes/restoreOnFailure hold the hook's lock and map. The bool is a "proceed" signal; the orchestrator's contract is return-on-!ok, never fall through, never re-strip.
Site 4 lives inside applyMemoryTransformation as a strip-without-insert, signaled by the bool. Your catch on my Task-3 wording is correct: "defensive trim via truncateToBytes + insertMemoryBlock" had no slot for the no-insert return, and an unconditional insert would silently turn site 4's strip-and-return into an insert of an over-budget (or garbage) block. The corrected contract takes result (the trim re-derives body from result.Text, and the overflow Debug logs the original len(block)), does: build block → if over budget: maxBody; if < 0 → Debug + stripMemoryBlock(req) + return false; else truncate rune-safely and rebuild → insertMemoryBlock(req, block) → return true. The three-way overflow behavior (no-overflow insert / trimmed insert / overflow strip-and-return) is preserved exactly.

One note for the record: the issue's own sketched signatures (fetchEngramPayload(ctx, prompt) (*EngramBlock, error), applyMemoryTransformation(ctx, contents, block) []llm.Content) are placeholders that carry the same defects — no req, a []llm.Content return that the sessctx.ContextTransformer contract doesn't use. Task 3 must implement the req-threading contracts above, not the issue's sketches, or the seam bug ships regardless of which helper owns the strips.

Q5 — Griller

The Q4 answer is fully self-consistent — the four-site inventory matches injector.go exactly (site 1 persist-gated on strip's return; sites 2/3 logging distinct values; site 4 no-insert), the corrected signatures thread req and result, and the issue's own placeholder sketches indeed carry the same defects. The seam bug is fixed.

Now for D, I did the CC arithmetic on the actual test file, and the plan's "by construction" claim does not survive it. Verified counts: TestFakeToolchainRunner_ZeroDefaults = 38 = base 1 + for-loop 1 + 12 closures × 3 (each closure: if got != nil || err != nil = 2, if !f.Called(...) = 1). TestFakeToolchainRunner_PresetValues = 28 = base 1 + for-loop 1 + 26, where the RunLinter closure alone is 4 (if out != "out" + if name != ... + if err == nil || err.Error() != ... = 2).

Q5: Your Task 2 says the D refactor "lands at CC ≤ 10 by construction (3–4 subtests per helper)." That is grouping-dependent, not by construction:

A helper holding 4 ZeroDefaults closures = CC 13 (base 1 + 4×3), which is an over-threshold function that is not in the catalog → TestVerifyNonFixCatalog's require.Empty(t, alerts) fails. Even 3 per helper lands at exactly 10 — passing, but with zero margin. For PresetValues, a domain-scoped pack that includes the RunLinter closure (4 CC) plus any three 2-CC closures = 11 → also fails. So the proposed grouping (testAnalysisRunnerMethods / testCoverageRunnerMethods) is not CC-safe — and "which of the 12 methods belong to coverage vs analysis" is itself underspecified (RunTestsWithCoverage, RunTests, RunLinter, CheckGovulncheck? GetGoDoc/GetModulePath/GetModuleDir/GetPackageList?), so a coder following the issue gets a coin-flip on whether the gate fails. What grouping provably lands ≤10 — e.g., exactly 3 ZeroDefaults closures per helper, and heavy-closure-aware packing for PresetValues (RunLinter can share with at most three 2-CC closures before hitting 11) — and does Task 2 now specify per-helper subtest counts instead of "3–4 by construction"?
And is the reversal worth it at all? Your Q1 retraction argued D is "the least entrenched" because the catalog "asserts no harm from splitting" — but the reversal still churns two catalog entries to RESOLVED plus two expectedCataloged rows, for zero production-behavior change, and your own earlier characterization was "relocates CC without reducing cognitive load." Meanwhile the test-complexity class's first disjunct ("dispatch-heavy test") arguably does fit a 12-method dispatch matrix, weakening even your class-doesn't-fit argument. Given the CC risk above, does D earn its place in this issue, or does the honest architect verdict — the one you reached before the misquote swung you — remain "leave the two test entries ACCEPTED, drop D"?

One question: specify the CC-provable grouping for Task 2, or drop D — which is it?

A5 — Architect

Q5 answer

Arithmetic verified — conceded. Recount against the file: ZeroDefaults = base 1 + for-loop 1 + 36 closure-CC, where ten closures are 3 (if got != nil || err != nil = if+|| = 2, plus if !f.Called = 1), RunLinter is 4 (out != "" || name != "" || err != nil = if + 2×|| = 3, plus Called = 1), and CheckGovulncheck is 2 (if err != nil + Called). 1+1+36 = 38 ✓. PresetValues = base 1 + for-loop 1 + 26, where eleven closures are 2 (if err != nil + one value assertion) and RunLinter is 4 (if out != "out" + if name != ... + if err == nil || err.Error() != ... = if+|| = 2). 1+1+26 = 28 ✓. Your grouping arithmetic is also right: a 4-closure ZeroDefaults pack of 3-CC closures = 1+12 = 13 → over threshold, not cataloged → require.Empty(t, alerts) fails; a 3-pack = 1+9 = 10 (zero margin, and it breaks if the pack includes RunLinter: 1+4+3+3 = 11); a PresetValues pack of RunLinter + three 2-CC closures = 1+4+6 = 11 → fails. "By construction" is retracted — it is grouping-dependent, full stop.

The provable grouping, if Task 2 survives (which, see below, I recommend it does not): named per-pack helper functions (closures count toward the helper, not the driver; the driver is already 1+1+0 = 2). ZeroDefaults: 12 closures at [4, 2, 3×10] = 36 closure-CC; minimum is four helpers at exactly 9 closure-CC each — {RunLinter(4), CheckGovulncheck(2), one 3} and three × {3,3,3} — every helper lands at exactly 10, i.e. zero margin; a 5-helper pack cannot fit (5×7 < 36), so margin is impossible without changing closure CC. The only way to real headroom is extracting the shared nil-pair assertion (assertNilPair(t, got, err)) and a Called-check helper, dropping each closure to 0–1 CC — a larger test rewrite than the issue's "group subtest verifications" proposes. PresetValues: 12 closures at [4, 2×11] = 26; four helpers {RunLinter, 2, 2} = 8→9, {2×4} = 8→9, {2×3} = 6→7, {2×2} = 4→5 — provable ≤9 with margin, no rewrite needed. So Task 2 would need per-helper subtest counts and measured verification, not "3–4 by construction."

Class-fit concession. My Q1 "the class doesn't fit D" argument is withdrawn as overstated: test-complexity's first disjunct is "sequential state-mutation or dispatch-heavy test" — a 12-method dispatch matrix plausibly satisfies "dispatch-heavy" even though the second condition (expensive setup to duplicate) is absent. The fit is partial, not absent, which removes my cleanest basis for calling the acceptance misfiled.

Verdict: drop D. The honest architect position — restored on corrected grounds, not the misquote-swing — is that D does not earn its place in this issue:

Zero production-behavior change. The refactor relocates test code; no production function's CC or behavior moves. The issue's own purpose for D is satisfied only as a number.
The CC-safe grouping is not "by construction" — it is exact-pack-constrained, and for ZeroDefaults it is margin-less (every helper must land at exactly 10) or requires a bigger assertion-helper rewrite than the issue proposes. A coder following the issue's vague "domain-scoped helper functions" grouping gets exactly the coin-flip you describe.
No changed circumstances. The D entries have been stable since acceptance (2026-08, #1325/ADR-060 era) — no accretion argument exists, unlike the #1410/#1412/#1414 growth behind B. The test-complexity acceptance is not obviously misfiled once "dispatch-heavy" is conceded.
No rebuttal, same governance defect as B/C but without B/C's basis. Under the Q1 adjudication-first discipline, D would require its own architect re-adjudication, and the only justification available is the CC number the policy already deems acceptable-when-documented.

So: leave both test entries ACCEPTED, keep both expectedCataloged rows, drop D from #1417. The issue narrows to A (pre-blessed by the carve-out) and B/C (subject to the Q1 architect-adjudication deliverable). If a maintainer later wants the test CC down, it should be a separate, architect-approved task with the exact pack spec above — and, for ZeroDefaults, the assertion-helper extraction needed for real margin — not bundled into this issue, where it would churn two catalog entries and two partition rows for a number with no behavioral or cognitive-load payoff.

Q6 — Griller

Both checks are done. Verified: filterAcceptedEntries drops any entry whose Status doesn't begin with "accepted" — so catalogTitleFor/catalogTitleForRange (used by both TestVerifyNonFixCatalog and TestVerifyCoveragePinsMatchLiveCatalog) see only ACCEPTED entries. That makes the adjudication sequencing a hard constraint, not a style choice.

Q6: Two items — one retraction, one constraint.

Retraction first (SOP): your Q5 claim that for ZeroDefaults "margin is impossible without changing closure CC" is arithmetically false. A 6-helper pack — {RunLinter(4), CheckGovulncheck(2)} = 6 closure-CC, plus five pairs of 3-CC closures ({GetPackageList, GetGoDoc}, {GetModulePath, GetModuleDir}, {RunTestsWithCoverage, RunTests}, {RunBenchmarks, FormatCode}, {RunModTidy, BuildCode}) = 6 each — gives six helpers at total CC 7, three units of margin, zero assertion-helper rewrite, and the pairing is semantically natural (linter+security-check together; toolchain-query pairs). "5×7 < 36" was the wrong bound; the constraint is per-helper ≤ 8 closure-CC, and 5×8 ≥ 36 holds. Your verdict to drop D survives on your stronger grounds (zero production change, no changed circumstances, no rebuttal) — the supporting arithmetic is what's wrong. Retract or revise it.

Now the constraint: your Q1 "Deliverable 0" says the adjudication re-adjudicates B/C "before any B/C code moves" as "a recorded decision (maintainer issue comment or catalog amendment)." Those two forms are not equivalent, and one of them cannot land first:

filterAcceptedEntries (nonfix_catalog.go:63-131) keeps only entries whose Status begins with "accepted." The moment you mark AfterTurn's entry RESOLVED (or SUPERSEDED) while the function still exists at CC=22, catalogTitleFor(entries, "internal/agent/memory/hook.go", 97) returns "" — the function becomes an uncataloged alert and TestVerifyNonFixCatalog's require.Empty(t, alerts) goes red. Your own plan's RED-first discipline (Makefile: "the gate must fail against a drifted catalog — never weaken it to land green") forbids the obvious escape.
Therefore the catalog amendment and the refactor it authorizes must be atomic in one commit — "adjudication before code" can only be a logical ordering within a single commit, or the adjudication must be recorded in a gate-invisible form (maintainer/architect verdict in the issue thread or an ADR) that precedes the code, with the catalog status change folded into the refactor commit.

So the question — one decision, three parts: (1) do you commit to the gate-invisible-first / atomic-catalog-amendment sequencing, i.e., the adjudication is recorded in the issue (or an ADR) before any B/C commit, and the RESOLVED status + expectedCataloged row removal land in the same commit as the decomposition? (2) Who signs that gate-invisible verdict — the human maintainer, the architect persona, or the issue author — given catalog-architect-curated says only the Architect records catalog entries and the quality model gives no modeled path for reversing an ACCEPTED entry (RESOLVED is for "fixed," SUPERSEDED for "replaced," neither is "re-adjudicated")? And (3) if the sign-off declines B/C entirely, does the issue narrow to A alone, and is the issue body amended to record that narrowing (and the D drop) — or does the amended scope live only in this grill transcript?

A6 — Architect

Q6 answer

Retraction — conceded, and verified by recount. The 6-helper pack is arithmetically sound: ZeroDefaults closures at [RunLinter=4, CheckGovulncheck=2, 10×3] partition into {RunLinter, CheckGovulncheck} = 6 closure-CC and five pairs of 3-CC closures (each = 6) — six helpers at total CC 7, three units of margin, zero assertion-helper rewrite, and the pairing is semantically natural (linter+security together; toolchain-query pairs). My "5×7 < 36" bound was the wrong constraint, and the claims built on it — "margin is impossible without changing closure CC" and "the only way to real headroom is extracting the shared nil-pair assertion" — are both retracted. The correct bounds, for the record: 4 helpers is the minimum (all at exactly 10, zero margin); a 5-helper pack exists at ≤9 closure-CC per bin but lands three helpers at exactly 10 ({4,3,2}=9 and three ×{3,3,3}=9, with the atomicity of the 4/2/3s blocking 5 bins at ≤8); 6 helpers give uniform margin at 7. The drop-D verdict survives unchanged on the stronger grounds — zero production-behavior change, no changed circumstances, no rebuttal of the ACCEPTED rationale — and I note your natural pairing also blunts my "vague grouping" criticism, which was always secondary to the value argument.

Constraint verified. loadNonFixCatalog's doc comment states it directly — "RESOLVED, REJECTED, SUPERSEDED and DONE entries are skipped" — and filterAcceptedEntries (nonfix_catalog.go) keeps only entries whose status begins with "accepted" (lowercased at parse). catalogTitleFor/catalogTitleForRange operate on that filtered slice. So both naive orderings are red: mark AfterTurn RESOLVED while it still sits at CC=22 → catalogTitleFor(entries, "internal/agent/memory/hook.go", 97) returns "" → uncataloged alert → require.Empty(t, alerts) fails; conversely refactor-first leaves expectedCataloged carrying the {Line: 97, Complexity: 22} row the refactored function no longer produces → require.Equal fails. Only the atomic state is green. The Makefile's RED-first clause ("the gate must fail against a drifted catalog — never weaken it to land green") forbids the obvious escape. You are right: "adjudication before code" can only be a logical ordering realized as gate-invisible-first plus an atomic catalog amendment.

(1) Sequencing — committed. The adjudication is recorded in a gate-invisible form — a maintainer verdict in the issue thread, or an ADR — before any B/C commit. The catalog status change (ACCEPTED → RESOLVED), the expectedCataloged row removals, and the coverage-pin re-anchors (hook.go:361-363 engrams-empty, hook.go:89-93 stubs, plus the memory test pins if new tests drift them) land in the same commit as the decomposition, per hotspot. The coordination rule's "same PR" is the minimum bar; same-commit is the safe implementation under RED-first, and it is what I commit to.

(2) Sign-off — the human maintainer, in the architect capacity; the architect persona drafts but cannot sign. catalog-architect-curated ("Only the Architect records or removes IntentionalNonFix entries; an Agent never edits the catalog unilaterally") and the quality model's glossary both distinguish the Architect — "the maintainer who curates the NonFixCatalog and ADRs" — from an Agent (AI assistant). The architect persona is an agent role; it may prepare the adjudication draft (the mandate-vs-shape and changed-circumstances rationale, the RESOLVED note, the partition diff) and present it for sign-off — the "stop and ask the user" checkpoint — but the recorded verdict must be the human maintainer's. Conveniently that is gosharplite: issue author and repo owner are the same party, so the sign-off is attainable by the maintainer amending their own issue. On the status-modeling gap: the quality model has no "re-adjudicated" status, and it does not need one — resolved is defined as "the issue was later fixed and the entry is kept as a record," which is exactly the transition B/C undergo because the refactor is the fix, with the "Complexity after fix" note per the handleActionKeys precedent. The reversal is realized as the RESOLVED lifecycle transition, not as a standalone re-adjudication; the unmodeled path you identify — reversing without fixing — is precisely what must not happen, and the governance correctly has no path for it. If the adjudication declines, the entries stay ACCEPTED and no reversal occurs at all.

(3) If declined — the issue narrows to A, and the narrowing is recorded in the issue itself, not the transcript. The grill transcript is a session artifact (staged prompts, session history) — not a governance record future agents read. The issue is the durable task artifact the tmg-issue-to-pr pipeline and every later agent read, and catalog-authoritative makes the authoritative scope live where the work is tracked. The maintainer must amend the issue body — or add a binding scope-amendment comment — recording: the B/C adjudication outcome (approved with the recorded rationale, or declined), the D drop (both test-complexity entries remain ACCEPTED, no catalog churn), and the updated acceptance criteria (D's checkbox removed, B/C checkboxes resolved against the verdict). If the amended scope lives only in this transcript, a later agent reading the issue as filed re-opens D and executes B/C without the adjudication — the exact governance failure this round exists to prevent.

Q7 — Griller

The Q6 answer is verified and largely airtight — the 6-pack concession is correct (with a minor muddle in the 5-pack parenthetical that doesn't affect the verdict), filterAcceptedEntries' ACCEPTED-only filtering is confirmed, the human-maintainer sign-off reading matches the quality model's own glossary (Architect = "the maintainer who curates the NonFixCatalog," distinct from Agent), and the RESOLVED mapping follows the handleActionKeys precedent. The adjudication/sequencing thread is closed.

The sharpest remaining decision is metadataIDs — and I can show it's a genuine trap under the subject's own adjudication standards, with a pin-drift consequence the Task 3 plan doesn't cover.

Q7: Your Q6 commits to B/C "as a package" — does that package include metadataIDs? Under your own two established adjudication bases, it cannot ride either one:

No mandate-vs-shape basis. metadataIDs (injector.go:237, CC=11, ACCEPTED) is a 4-case type-switch with no ADR-068 mandate about its shape — the mandate-vs-shape argument exists only for AfterTurn/Transform/FlushSession, where an ADR prescribes fail-open semantics. The catalog entry classifies it dispatch-structural, "same class as handleDomainEvent (CC=12)" — the exact class whose whole point is that type-switch CC is acceptable as-is.
No changed-circumstances basis. The entry records "CC re-verified 11 (unchanged)" through the #1410 re-anchor — no accretion, unlike the 21→22 / 13→16→18 growth you marshaled for B. Your Q1 answer already called it "the weakest overturn in the whole issue" and said you'd "not fight" leaving it ACCEPTED.

So either it's out (leave ACCEPTED, no catalog churn — consistent with the adjudication discipline, and the amended issue body must drop or qualify the (Transform, metadataIDs) acceptance-criteria checkbox), or it's in, in which case you need a third basis the issue doesn't state and your two bases don't provide. Which is it?

And regardless of the answer, there's a pin-drift consequence your Task 3 doesn't address: metadataIDs is pinned in expectedCataloged at {Line: 237} and in the catalog's See: at injector.go:237. If the Transform extraction inserts fetchEngramPayload / applyMemoryTransformation / observeInjection anywhere above line 237 in the same commit, the live measurement reports the new line, require.Equal(t, expectedCataloged, cataloged) fails, and the row plus the catalog See: must be re-anchored in that commit. Your Task 3 protects the test pin (injector_test.go:109) by appending test helpers at file end — but says nothing about protecting (or re-anchoring) the metadataIDs production pin. The cheap fix is the same discipline applied to injector.go: place the three new production helpers below metadataIDs (after joinTextParts at file end) so line 237 holds and no re-anchor is needed. Specify: is metadataIDs in or out of the package, and if out, does Task 3 commit to appending the new helpers below injector.go:237 so the ACCEPTED pin survives untouched — or re-anchor row + See: in the same commit?

A7 — Architect

Q7 answer

metadataIDs is OUT. Under the two adjudication bases I committed to, it cannot ride either, and you're right that no third basis exists consistent with the discipline: the "byproduct of Transform work" option I floated in Q1 is empty — extracting fetchEngramPayload/applyMemoryTransformation/observeInjection does not touch metadataIDs' body or its CC, so there is no natural byproduct reduction; testability is already satisfied (TestMetadataIDsUnsupportedValues exists at injector_test.go:456); and "the issue's AC lists it" is a number, not an adjudication. So: leave it ACCEPTED, no catalog churn, no expectedCataloged row removal, no RESOLVED marking. This makes my Q1 statement ("the weakest overturn… I will not fight it") the committed plan default rather than a hedge. The amended issue body (per Q6 part 3) must qualify the AC checkbox to Transform-only: "Transform decomposed into pipeline helpers with CC ≤ 10; metadataIDs remains ACCEPTED (dispatch-structural, CC=11)" — never leave the checkbox as filed, or a later agent will treat metadataIDs as in-scope.

Pin-drift: your cheap fix is insufficient — and for the same class of arithmetic you've been catching. The drift source is not (only) inserting helpers above line 237; it is Transform's shrink. Transform today spans injector.go:64–~163 (~100 lines); the refactored orchestrator is ~20 lines, freeing ~60–80 lines above metadataIDs. So even if the three new production helpers go into a new file or below joinTextParts, metadataIDs shifts from 237 to roughly ~160 — the {Line: 237} row and the catalog See: injector.go:237 both go stale. The only way to hold 237 is to re-fill the vacated space above it with the extracted helpers such that the line count above 237 stays constant — coincidence-dependent, and it couples file layout to pin numbers, which the catalog's drift policy already fights. Reject that. The re-anchor is mandatory, in the same commit as the Transform refactor: update the expectedCataloged row {Line: 237} and the catalog See: injector.go:237 to the same new line, measured by the live GatherComplexities run before commit (the drift policy's prescribed behavior: "Re-verify on every PR that touches a file referenced by the catalog… update the entry when ranges do not point at the intended symbol"). Never one without the other: row-only breaks require.Equal (line mismatch), See:-only breaks catalogTitleFor (uncataloged alert). This is the same atomicity discipline as the B/C entries — the re-anchor is a catalog change, so it rides in the refactor commit, not before or after.

One containment note that makes the re-anchor the only injector.go pin change: metadataIDs is the sole surviving injector.go pin after the Transform refactor — the Transform row ({Line: 64, Complexity: 18}) and its catalog entry are removed/RESOLVED in that same commit, and TestVerifyCoveragePinsMatchLiveCatalog has no injector.go rows (its hook.go/flock_unix.go/mcp_factory.go etc. pins are unaffected by C). So the injector.go governance surface is exactly two edits: the metadataIDs row re-anchor and the metadataIDs See: re-anchor.

Placement recommendation: put the three new production helpers (fetchEngramPayload, applyMemoryTransformation, observeInjection, plus resolveUserPrompt) in a new file — internal/agent/memory/injector_pipeline.go, package memory — so injector.go's diff is confined to the Transform rewrite and the metadataIDs re-anchor is the sole pin edit. Test helpers stay appended at file end per the existing Task-3 commitment (injector_test.go:109 holds, TestMetadataIDsUnsupportedValues at 456 holds). Final scope of C as amended: Transform decomposed with the Q4 four-site ownership contract; metadataIDs untouched, ACCEPTED, re-anchored in-commit; issue body qualified accordingly.

Q8 — Griller

The Q7 answer is verified and correct on every count — the shrink-drift arithmetic checks out (Transform spans 64–~163, so a ~20-line orchestrator frees ~79 lines and metadataIDs shifts from 237 to ~158), TestMetadataIDsUnsupportedValues exists at injector_test.go:456, the coverage-pin table has no injector.go rows, and the row+See: atomicity follows from filterAcceptedEntries. The metadataIDs thread is closed cleanly.

Now the B decomposition — and the orchestrator's guess about where the CC risk lands is aimed at the wrong function. Let me verify the arithmetic against the tool's confirmed counting method (base 1 + each case 1 + each if 1 + each || +1 — this method reproduces the reported AfterTurn=22 exactly):

buildEpisode(turn, err) as Task 4 specifies it: base 1 + 3-case switch (3) + branch-(i) text == "" (1) + err != nil (1) + branch-(ii) IsTransient (1) + logger guard (1) + branch-(iii) gerr != nil || content == nil (2) + logger guard (1) + text == "" (1) = CC 12 — over threshold.

The retained AfterTurn with everything Task 4 keeps inline: base 1 + cfg == nil || !cfg.Enabled (2) + tier == off (1) + dedupe && (2) + client == nil (1) + logger guard (1) + !ok from buildEpisode (1) + 3-case tier switch (3) = CC 12 — over threshold.

Q8: Both the new helper and the retained orchestrator land at 12 under your own decomposition — and the partition gate treats a new >10 function as an uncataloged alert (require.Empty(t, alerts) fails), exactly as it would for any other drift. The tier handlers are not the risk (each is CC 1 — a single delegation), so the orchestrator's hint about handleFullTier is a red herring; the risk is the classification extraction itself — the three-way switch plus its seven guards is most of AfterTurn's 22, and lifting it verbatim into buildEpisode just relocates the CC past the threshold. Task 4 as written cannot meet either the issue's AC ("all functions at CC ≤ 10") or the gate. Specify the CC budget per extracted function and the further splits needed — at minimum (a) isDuplicateTurn(turn) bool for the turn-scoped dedupe so AfterTurn drops to ~9, and (b) a branch-(iii) fetch helper (detached-ctx GetLastModelTurn + gerr/content guard + text-empty check) so buildEpisode drops to ~8 — or state which of the two you accept staying over threshold and on what catalog basis, since neither has one. Which is it: split again until both land ≤10, or concede that the three-way classification cannot be decomposed under this AC?

A8 — Architect

Q8 answer

Arithmetic verified — conceded in full. I re-derived AfterTurn=22 with the method you state (base 1 + each case 1 + each if 1 + each ||/&& 1): 1 + cfg (nil||!Enabled) 2 + tier-off 1 + dedupe && 2 + nil-client 1 + logger guard 1 + classification switch 3 + text-empty 1 + err-annotate 1 + IsTransient 1 + logger guard 1 + gerr||content 2 + logger guard 1 + text-empty 1 + tier switch 3 = 22. Your two counts are exact: buildEpisode as Task 4 specified it = 12 (1+3+1+1+1+1+2+1+1), retained AfterTurn with dedupe inline = 12 (1+2+1+2+1+1+1+3). My Task-4 wording treated "extract the classification" as if that were the whole job — it relocates the CC, it does not reduce it; the classification (switch + seven guards) is most of AfterTurn's 22. And the handleFullTier hint is conceded as a red herring: each tier handler is a single delegation, CC 1; the risk was never there. Task 4 as written could not meet the AC or the gate — retracted.

Answer: split again until both land ≤10 — the three-way classification can be decomposed under this AC, and your (a) and (b) are exactly the missing splits. Neither function stays over threshold: there is no catalog basis for a new >10 function (it would be an uncataloged alert, and pre-cataloging a function we just wrote is self-inflicted catalog churn requiring architect adjudication for a number). The full B budget, per the confirmed counting method:

Function	CC	Composition
`AfterTurn`	8	base 1 + cfg gate 2 + tier-off 1 + `isDuplicateTurn` 0 + `clientUnavailable` 0 + `!ok` 1 + tier switch 3
`isDuplicateTurn(turn) bool`	3	base 1 + `&&` 2 (lock/check/update under one lock, defer unlock — equivalent to today's lock/check/update/unlock since it returns immediately)
`clientUnavailable() bool`	3	base 1 + nil-client 1 + logger guard 1 (same Warn key/value `memory_client_unavailable`/`nil MCP client`/`learn`)
`buildEpisode(turn, err) (episode, bool)`	9	base 1 + switch 3 + (i) text-empty 1 + err-annotate 1 + (ii) IsTransient 1 + logger guard 1 + (iii) `!ok` from fetch 1
`fetchLastModelTurn() (string, bool)`	5	base 1 + `gerr\|\|content` 2 + logger guard 1 + text-empty 1 (detached 3s ctx + `GetLastModelTurn` + Warn all inside)
Tier dispatch	inline in AfterTurn	capture/bufferAppend/maybeLearn already exist; no new handler functions needed

Note the nil-client guard must come out too: with it inline, AfterTurn is exactly 10 (zero margin — the same fragility the griller keeps catching); with clientUnavailable extracted it is 8. Two units of margin, which is where it should be.

And the rest of B, so the whole AC is budgeted, not just the buildEpisode chain: FlushSession 7 (base 1 + !ok 1 + nil-client 1 + engrams-empty 1 + err\|\|result.Error 2 + logger guard 1) with claimEpisodes 5 (base 1 + master-switch gate 2 + !ok 1 + empty-drain 1), buildEngramPayload 3, restoreOnFailure 3, finalizeOnSuccess 3, effectiveScope 2. maybeLearn 6 (base 1 + frame gate 1 + claimLearnSlot 0 + scope 1 + err\|\|result.Error 2 + logger guard 1) with claimLearnSlot 5 (base 1 + nil-init 1 + dup 1 + flood 1 + logger guard 1). Everything ≤10, most with margin; the only function at a single unit of margin is buildEpisode (9).

Semantics preserved per split, so the existing hook tests stay green: isDuplicateTurn keeps the belt-and-suspenders dedupe under the same lock; fetchLastModelTurn keeps the detached bounded ctx, the gerr||content fail-open, the Warn, and the text-empty skip — branch (iii) becomes text, ok := h.fetchLastModelTurn(); if !ok { return episode{}, false }; branch (i) keeps the err-annotation (ep.Error = err.Error()), branch (ii) keeps the transient skip before the error-episode build; the tier switch in AfterTurn keeps capture/batch/full dispatch in the same order. The net is the corrected Task-4: three new production functions in hook.go (isDuplicateTurn, clientUnavailable, fetchLastModelTurn) plus the Q3 FlushSession quartet and claimLearnSlot, all measured against the live tool before commit, with the AfterTurn/FlushSession/maybeLearn entries RESOLVED and their expectedCataloged rows removed in the same atomic commit per the Q6 sequencing.

Q9 — Griller

Q8's arithmetic is fully verified under the confirmed counting method — every budget lands correctly (AfterTurn 8, buildEpisode 9, fetchLastModelTurn 5, claimEpisodes 5, maybeLearn 6, claimLearnSlot 5, all with margin except buildEpisode at 9), the nil-client extraction restoring AfterTurn to 8 is exactly right (inline it would be 10, zero margin), and the semantics-preservation claims match the current code. The B thread is closed.

One structural gap remains: the delivery sequence and PR shape — and the commitments you've made across Q1/Q6/Q7 now constrain it in ways you haven't reconciled.

Q9: Three commitments are in tension with the delivery pipeline: (1) Q1/Q6 — the B/C adjudication is a maintainer sign-off recorded gate-invisibly, and B and C both wait on it ("C proceeds with Transform on the same basis"); (2) Q6 — every hotspot's catalog change is atomic in-commit; (3) the issue-to-PR pipeline's default is one combined PR targeting dev, and the issue body — which the Architect persona seeds its task list from — is the durable task artifact. Also note A is only "pre-blessed" modulo the amendment: as filed, the issue proposes four validators and the wrong partition-test path; your A is six helpers plus dual-violation pins, so the amended issue body must capture A's revised design too, or the Architect seeds from a spec that doesn't match what you've been grilled into.

So, specify the crisp sequence and the PR shape:

Does the maintainer's issue-body amendment (D out, metadataIDs out, C qualified to Transform-only, B conditional, A re-specified as six sub-validators + pins) precede ALL code — including A — or does A proceed while the amendment and the B/C adjudication are pending?
What is the PR shape given the adjudication sits between A/C and B: (a) one combined PR opened only after the adjudication resolves, its content = A + C + (B if approved) — meaning the pipeline stops after A and C and asks the maintainer before B is even dispatched, per the issue-to-PR failure policy; or (b) per-hotspot PRs (A, C, B-conditional) merged sequentially, each independently green against the shared expectedCataloged — which your atomic-commit discipline satisfies, but which makes the adjudication a mid-series gate between PR 2 and PR 3?

State the sequence (amendment → adjudication → A → C → B, or amendment → A → C → adjudication → B, or your actual order) and which PR shape follows from it — and who triggers the mid-pipeline stop-and-ask: the orchestrator, or the maintainer reviewing the issue thread.

A9 — Architect

Q9 answer

Premise correction first — the adjudication is not mid-series; Q1 already front-loaded it. The griller's framing "the adjudication sits between A/C and B" holds only if C proceeds before the verdict. My Q1 committed otherwise: "C proceeds with Transform on the same basis" — C's mandate-vs-shape basis (ADR-068 §1) requires the same maintainer verdict as B's. So the adjudication gates C and B together, before either, and the tension the three commitments create dissolves once it is front-loaded. A is the only pre-blessed hotspot, and even A is pre-blessed only modulo the amendment — as filed, the issue names internal/tools/analysis/nonfix_catalog_partition_test.go (nonexistent; the real gate is real_nonfix_catalog_test.go) and proposes four validators that cannot both meet the CC≤5 AC and preserve the documented order, so the Architect must not seed from the unamended body.

(1) The amendment precedes ALL code, including A. The issue body is the durable task artifact the Architect persona seeds its task list from; executing A against the unamended body means dispatching a spec that (a) names a nonexistent partition file, (b) specifies a four-validator design whose auth-switch lands at CC≥7 (failing the AC) or silently changes the TIMEOUT-vs-switch ordering (violating the preserve clause), and (c) omits the dual-violation pinning tests that make the ordering enforceable. None of that has been ratified by the maintainer. The amendment is one maintainer act that carries the entire grilled scope: A re-specified as six sub-validators (validateTransportShape, validateCommandFields, validateAuthTransportConflict, validateExecutionLimits, validateAuthMode, validateCredentials) with the set-lookup conversion, the dual-violation pinning tests, and the corrected partition-test path; D dropped (both test-complexity entries stay ACCEPTED); metadataIDs out (dispatch-structural, stays ACCEPTED, re-anchored inside C's commit); C qualified to Transform-only; B conditional on the recorded verdict; and the AC checkboxes updated to match. I do not fold the amendment and the B/C verdict into two separate acts — the maintainer signs one amendment that both ratifies the scope and records the adjudication, because the same sign-off authority and the same gate-invisible medium (issue body/thread) serve both.

Sequence: amendment+adjudication (one maintainer act) → A → C → B. All code follows the single up-front sign-off; C and B are dispatched only if the verdict accepts the two recorded bases (mandate-vs-shape for AfterTurn/FlushSession/Transform; changed circumstances via the #1410/#1412/#1414 accretion and test-layer migration for the hook). If the verdict declines, the sequence ends at A and the issue closes with the narrowed scope recorded.

(2) PR shape: one combined PR targeting dev — the pipeline default — opened after all approved work completes, content = A + C + (B if approved), A + C if declined. The atomic-commit discipline is satisfied exactly: each hotspot's commit carries its refactor + RESOLVED status + expectedCataloged row removals + re-anchors together, so every commit — and therefore the PR head — is independently green against the shared catalog/partition. Per-hotspot PRs (option b) are unnecessary: with the adjudication front-loaded there is no mid-series gate forcing separation, the three hotspots touch the shared expectedCataloged map sequentially anyway (parallel PRs would conflict), and the task loop already gives the per-increment Architect review that per-hotspot PRs would nominally add — splitting the PRs only adds merge ceremony. The "review A/C, then decide B" workflow is available only as a deliberate maintainer override of the Q1 commitment (it would re-classify C from conditional to unconditional); the committed default is the single up-front verdict.

(3) Who triggers the stop-and-ask: the orchestrator, once, up front. Per the issue-to-PR failure policy, the orchestrator stops and asks on plan-level decisions and cannot proceed past the amendment without the sign-off — the maintainer does not need to monitor proactively. The orchestrator assembles the amendment+adjudication package (the grilled scope summary above plus the RESOLVED drafts and partition diffs) and presents it before dispatching A; the maintainer signs, and the verdict is recorded in the issue body/thread — gate-invisible to filterAcceptedEntries, as Q6 requires — after which A → C → B run to the combined PR. Because the stop is front-loaded, there is no mid-pipeline stop to attribute: the orchestrator initiates the single ask; the maintainer is the signatory.

Q10 — Griller

The Q9 answer is coherent — front-loading the adjudication into one maintainer act that precedes all code dissolves the mid-series tension, the one-combined-PR shape follows, and the "pre-blessed only modulo the amendment" framing correctly reflects the issue's wrong partition-test path and four-validator spec. The ordering thread is closed.

For Q10, I re-derived the A sub-validator AC arithmetic — and the subject addressed only the auth-switch, missing two of the six. Using the confirmed counting method (reproduced exactly on AfterTurn=22 and the D closures):

validateCommandFields: if !c.IsStdio() && (len(Args) > 0 || Dir != "" || len(Env) > 0) = if(1) + &&(1) + ||(1) + ||(1) + ||(1) + base(1) = CC 6 — over the ≤5 AC.
validateCredentials: if auth == Bearer && Token == "" (2) + if auth == Basic && (Username == "" || Token == "") (4) + base(1) = CC 7 — over the ≤5 AC.
validateTransportShape and validateAuthTransportConflict land at exactly 5 (at the bound, zero margin); only validateExecutionLimits (2) and the set-lookup validateAuthMode (2) clear it comfortably.

Q10: Your Task 1 commitment to "each sub-validator CC ≤ 5" is not met by the six-validator plan as specified — you computed the auth-switch (7 → 2 via set-lookup) but not the other two, and both exceed the AC. Two coherent resolutions, and the choice determines whether the set-lookup conversion is even necessary:

Keep ≤5 and restructure further — e.g., extract hasCommandKnobs() bool (base 1 + 3×|| = 4) so validateCommandFields drops to 4, and extract a hasBasicCredentials()/hasBearerToken() pair so validateCredentials drops to 5 — two more helper functions than the six you named, and the amendment's validator list grows.
Amend the AC to the policy threshold (≤10) — complexity-threshold-policy deems ≤10 acceptable, and under it all six sub-validators pass as specified: validateCommandFields (6) and validateCredentials (7) clear it, and — critically — the 6-case auth-switch at CC 7 also clears it, which eliminates the need for the set-lookup conversion entirely. Your Q2/Q9 commitment to "the set-lookup conversion" was an implementation consequence of the stricter-than-policy ≤5 AC, not a requirement of anything in the repo; if the AC becomes ≤10, the switch stays verbatim, the message-pinning concern vanishes, and the behavioral surface you were pinning with dual-violation tests shrinks to just the ordering pins.

Which is it — the stricter ≤5 AC with the two additional restructuring helpers (and the set-lookup), or the policy-consistent ≤10 AC that keeps the switch and obviates the lookup? And whichever you choose, does the amended issue body record the choice explicitly, since the issue's AC text ("each sub-validator CC ≤ 5") as filed binds neither once the amendment supersedes it — and a coder following the unamended AC will hit the exact 6 and 7 you've now been shown?

A10 — Architect

Q10 answer

Retraction — the counting premise was wrong, and it changes the arithmetic on both sides. I verified the tool's actual counter at the source: calculateComplexity (internal/tools/analysis/astutil.go:236) counts base 1 + each *ast.IfStmt/ForStmt/RangeStmt/CaseClause/CommClause + each BinaryExpr with LAND or LOR. A CaseClause is one AST node regardless of comma-separated expressions — so the auth switch case MCPAuthAuto, MCPAuthGH, MCPAuthBearer, MCPAuthBasic, MCPAuthNone: is one case (+ the default: clause) = CC 2, not 6. The empirical proof: validate = 1 + if1(2) + if2(2) + if3(4) + if4(3) + if5(1) + switch(2) + if6(2) + if7(3) = 20, exactly the measured value; with the switch counted as 6 it would measure 24. So my Q2 "a switch alone is CC=7" — which the griller's Q10 inherited — was wrong, and the set-lookup conversion was never necessary: it was solving a CC-7 problem that does not exist. That commitment, carried through Q2/Q9, is retracted.

Corrected arithmetic for the six sub-validators (same method: base 1 + if + && + || + case-clause):

Validator	CC	vs ≤5 AC	vs ≤10 AC
`validateTransportShape`	5	at bound	✓
`validateCommandFields`	5 (if+`&&`+2×`		`, not the griller's 6)
`validateAuthTransportConflict`	4 (not 5)	✓	✓
`validateExecutionLimits`	2	✓	✓
`validateAuthMode` (switch verbatim)	3 (grouped case + default, not 7)	✓	✓
`validateCredentials`	6 (if6 2 + if7 3 + base — not 7)	✗ over by 1	✓

The griller's "exact 6 and 7" are 5 and 6 — command is at the ≤5 bound, not over; credentials is over by one, not two; the auth switch is the cheapest validator in the set. Only one of the six actually violates the ≤5 AC: validateCredentials at 6.

Which is it — the choice, now reframed. The set-lookup is obviated under both options, so the real decision is purely AC strictness. My recommendation is option 2 — amend the AC to the policy threshold (≤10) — and now on stronger grounds than the griller's framing: the switch is CC 3 and stays verbatim (exact message preserved by construction, message-pinning concern vanishes), all six sub-validators pass as specified with margin (only credentials at 6 — four units), and no extra helpers. Option 1 (keep the arbitrary ≤5) is much cheaper than the griller believed — one extraction, hasBasicCredentials() bool (base 1 + || 1 = 2), drops credentials to 5 — but it leaves validateTransportShape and validateCommandFields at exactly 5 (zero margin, the same fragility this round has caught repeatedly) and preserves an AC with no stated rationale and no repo invariant behind it (complexity-threshold-policy makes ≤10 canonical). Under ≤10, the decomposition still achieves the issue's real goals — SRP, unit-testability, ordering pins, and a genuine 20 → max-6 reduction — without the structural contortions.

The amended issue body records the choice explicitly — yes, unconditionally. The AC text as filed ("each sub-validator CC ≤ 5") binds nothing once the amendment supersedes it, and a coder following the unamended text would hit validateCredentials at 6 (or, worse, add the needless set-lookup the misread suggests). The amendment's A checkbox becomes: "(*MCPServerConfig).validate refactored into six modular validators, each at CC ≤ 10 (policy threshold); the six-mode auth switch retained verbatim as a grouped case (CC 3); exact error strings and documented check order preserved; dual-violation ordering pins added (TIMEOUT-wins, URL-empty-wins, conflict-wins); no set-lookup." The ordering pins survive regardless of the AC choice — Q2 established the TIMEOUT-vs-switch position is documented-but-unpinned, and that finding is untouched by this correction.

VERDICT — Griller

Verified at the source: calculateComplexity (astutil.go:236) increments once per *ast.CaseClause AST node regardless of comma-separated expressions, and the empirical proof holds — summing the current validate with the CaseClause counting gives 1+2+2+4+3+1+2+2+3 = 20, exactly the measured value. A10 is fully correct: the auth switch is CC 2 (+base 1 = 3 in a validator), not 6/7; my Q10 "exact 6 and 7" were 5 and 6; the subject's own Q2 "switch alone is CC=7" was equally wrong; and the set-lookup conversion was solving a problem that never existed. The dual-violation ordering pins survive untouched, and under the policy-consistent ≤10 AC all six sub-validators pass as specified with the switch verbatim.

VERDICT: The subject's diagnosis held across nine of ten threads — every CC value and line pin verified against the live tree, the gate mechanics (ACCEPTED-only partition, coverage pins, atomic-commit requirement, RED-first) read correctly, and its self-corrections (D misquote, ordering-pin overstatement, engrams-empty seam, C strip-site ownership, metadataIDs out, adjudication front-loading) consistently improved the plan. Where it did not hold was its own Q2 claim that the auth switch alone is CC 7 — a misreading of the repo's CaseClause counting that it caught and corrected in A10, verified here at the tool source, which retracts the set-lookup conversion as never necessary. Recommendation: proceed with changes — execute the one up-front maintainer amendment+adjudication (scope narrowing, AC amended to the policy threshold ≤10, set-lookup removed, six validators with the switch verbatim, dual-violation pins), then A → C (Transform-only, metadataIDs re-anchored in-commit) → B (the verified helper budget, atomic catalog commits), delivered as one combined PR to dev, with D dropped and its entries left ACCEPTED.

POST-GRILL POSITION — Architect

POST-GRILL POSITION: What I stand by, intact after ten threads: the diagnosis of the four hotspots with every CC value and line pin verified against the live tree; the governance mechanics — catalog-authoritative/catalog-architect-curated, the ACCEPTED-only partition gate (filterAcceptedEntries + require.Empty(t, alerts)), the coverage-pin surface, the atomic-commit requirement and RED-first discipline; the front-loaded sequencing (one maintainer amendment+adjudication act, gate-invisible, preceding all code, with each hotspot's RESOLVED status + expectedCataloged removals + re-anchors atomic in its commit); the one-combined-PR shape to dev; and the fail-open semantics preservation for B and C (the four-site strip contract, the claim/restore lock discipline, the engrams-empty disposition owned by the orchestrator). The final plan stands as the griller's verdict states it: amendment+adjudication → A (six validators, switch verbatim, dual-violation pins) → C (Transform-only, metadataIDs re-anchored in-commit) → B (the verified helper budget), with D dropped.

What the grill revised in me — the substantive list: the D misquote retraction, which correctly re-opened the drop-D verdict on stronger grounds (zero production change, no changed circumstances, no rebuttal); the "pinned ordering" overstatement, which produced the dual-violation pinning tests; the engrams-empty seam and C's strip-site ownership, which fixed two real design defects; metadataIDs out (dispatch-structural, no basis under my own two adjudication standards); the Q8 under-split (buildEpisode and retained AfterTurn both at 12) corrected by the verified per-function budget — isDuplicateTurn, clientUnavailable, fetchLastModelTurn, claimLearnSlot — all ≤10 with margin; and, most importantly, my own Q2 error on the auth switch: the grouped CaseClause counts once, so the switch is CC 3, the set-lookup was never necessary, and the AC amends to the policy threshold ≤10. Every retraction improved the plan; none of the original diagnosis was overturned.

Open questions for the maintainer's sign-off: (1) the amendment+adjudication act itself is the critical path — its content (scope narrowing, AC ≤10, six validators, ordering pins, corrected partition-test path) needs the single up-front ratification before any code, and the B/C verdict (mandate-vs-shape for AfterTurn/Transform; changed circumstances via the #1410/#1412/#1414 accretion for the hook) is the decision that determines whether the issue delivers A+C+B or narrows to A alone; (2) whether the D drop should be revisited as a separate future task using the verified 6-helper pack, or left ACCEPTED permanently — my recommendation is the latter; (3) acceptance of the residual standing exceptions the plan deliberately leaves in place: metadataIDs at CC 11 (one over policy, dispatch-structural class) and the two A validators at exactly the ≤5 bound now governed by ≤10 (zero-margin under the stricter reading, comfortably clear under policy). If the maintainer ratifies the amendment as specified, I consider the plan ready to dispatch: A → C → B, one combined PR, all commits independently green.

gosharplite/issue-1417-grill.md