Grill Round — Issue #1418 (Architect × Griller)

Topic: gosharplite/tell-me-go#1418 — decompose complexity hotspots (post-grill scope; supersedes #1417) Round: 6 questions (early conclusion at Q7 declaration) · Date: 2026-08-20 · Orchestrator: butler

Opening Statement — Architect (Subject)

All verification complete. I have everything needed to present the deliverable.

1. ## Understanding

#1418 is the post-grill implementation spec superseding #1417. It carries an already-adjudicated scope — A + C + B approved, D dropped, metadataIDs out — and my job is not to re-litigate that adjudication but to verify the spec against the committed tree and present an executable plan. What is binding:

Scope A: decompose (*MCPServerConfig).validate (CC=20, internal/domain/config/mcp_config.go:107) into six private sub-validators called by a thin validate(name) orchestrator, preserving exact error strings and the documented check order; sub-validator CC ≤ 10 (the canonical policy threshold, per the complexity-threshold-policy invariant in docs/domain-model/quality.modelith.md).
Scope C: decompose (*plurInjector).Transform (CC=18, internal/agent/memory/injector.go:64) per the four-site strip-ownership contract, new helpers in a new file injector_pipeline.go, Transform remaining a void orchestrator returning on every path (ADR-068 fail-open); metadataIDs (injector.go:237, CC=11) stays ACCEPTED but must be re-anchored (row + See:) in the same commit because Transform's shrink drifts its line.
Scope B: decompose AfterTurn (22), FlushSession (18), maybeLearn (11) in internal/agent/memory/hook.go per the verified helper budget, preserving ADR-068 fail-open, the #1412 claim/restore lock discipline, and the #1414 master-switch drain-and-drop; all concurrency proofs stay green.
Governance atomicity rule (binding): each hotspot's refactor commit must atomically carry the refactor + the catalog entry marked RESOLVED with a "Complexity after fix" note (per the handleActionKeys precedent) + the expectedCataloged row removals + coverage-pin re-anchors — because filterAcceptedEntries keeps only ACCEPTED entries (internal/tools/analysis/nonfix_catalog.go), so marking RESOLVED while CC>10 yields an uncataloged alert (require.Empty(t, alerts) fails), and refactor-first breaks the exact-equality require.Equal(t, expectedCataloged, cataloged) in TestVerifyNonFixCatalog.
AC amendments: sub-validator CC ≤ 10 (not ≤5); the partition gate is real_nonfix_catalog_test.go (the nonfix_catalog_partition_test.go path cited in #1417 does not exist); dual-violation ordering pins added in the A commit.

2. ## Spec Verification

I re-derived every measurable claim with the repo's own tooling (calculateComplexity in internal/tools/analysis/astutil.go: base 1 + IfStmt/ForStmt/RangeStmt/CaseClause/CommClause + &&/||; a CaseClause counts once regardless of comma-separated expressions). Results:

Measured CC — all match the issue exactly:

Symbol	Measured	Claimed
`(*MCPServerConfig).validate` @ mcp_config.go:107	20	20 ✓
`(*plurHook).AfterTurn` @ hook.go:97	22	22 ✓
`(*plurHook).FlushSession` @ hook.go:300	18	18 ✓
`(*plurHook).maybeLearn` @ hook.go:236	11	11 ✓
`(*plurInjector).Transform` @ injector.go:64	18	18 ✓
`metadataIDs` @ injector.go:237	11	11 ✓

Scope A six-validator table — re-derived from the actual validate() body, all correct:

validateTransportShape = 5: two IfStmts each with && (URL↔COMMAND mutual exclusion, URL-empty) = 4 decisions + base 1.
validateCommandFields = 5: one IfStmt with && + two || (ARGS/DIR/ENV) = 4 + base 1.
validateAuthTransportConflict = 4: one IfStmt with && + || (bearer/basic under stdio) = 3 + base 1.
validateExecutionLimits = 2: one IfStmt (TIMEOUT) = 1 + base 1.
validateAuthMode = 3: the six-mode switch has 2 CaseClauses (the five valid modes grouped, plus default) = 2 + base 1 — confirming the "CC 3, not 7" claim.
validateCredentials = 6: bearer IfStmt (&&) + basic IfStmt (&& + ||) = 5 + base 1.
Sum = 25 vs. current 20 — the +5 is the per-function base of five new functions; expected, and the issue doesn't claim otherwise. Each ≤ 10 ✓.

Scope B helper budget — these are planned targets, not measurements (the helpers don't exist yet). I checked plausibility against the current bodies: isDuplicateTurn 3 (base + IfStmt(&&) = 3 ✓), clientUnavailable 3 (base + nil-client + logger = 3 ✓), fetchLastModelTurn 5 (base + gerr||content + logger + text-empty = 5 ✓), buildEpisode 9 (base + 3-case switch + text-empty + err-annotation + IsTransient + logger + delegation !ok = 9 ✓), claimEpisodes 5 (base + master-gate + !ok + empty-drain = 5 ✓), buildEngramPayload 3 (base + range + scope-if ✓), restoreOnFailure 3, finalizeOnSuccess 3, effectiveScope 2, maybeLearn 6, claimLearnSlot 5 — all plausible. The exact AfterTurn 8 / FlushSession 7 depend on extraction shape (e.g. whether err != nil || result.Error != nil stays inline at 2 decisions); the enforceable gate is per-function ≤ 10, which the table clears with margin.

Pins and gate mechanics — all verified:

Partition gate is internal/tools/analysis/real_nonfix_catalog_test.go (TestVerifyNonFixCatalog); nonfix_catalog_partition_test.go does not exist ✓ (the issue's correction of #1417 is accurate). The gate does require.Equal(t, expectedCataloged, cataloged) + require.Empty(t, alerts), and filterAcceptedEntries in nonfix_catalog.go keeps only status-prefix accepted — so the atomicity reasoning (row-only breaks require.Equal; See:-only breaks catalogTitleFor → alert) is exactly right.
Catalog entries and their current Status/See for the five RESOLVED targets: mcp_config.go:107 (ACCEPTED 2026-08 #1396), hook.go:97 (ACCEPTED 2026-09 #1404, re-anchored #1410/#1414, CC 21→22), hook.go:236 (ACCEPTED 2026-09 #1404), hook.go:300 (ACCEPTED 2026-09 #1404, #1412/#1414 re-anchors), injector.go:64 (ACCEPTED 2026-09 #1404). metadataIDs ACCEPTED at injector.go:237. All match.
Coverage pins: hook.go:89-93 (BeforeTurn @89, OnPhaseTransition @92 — both CC=1) and hook.go:361-363 (if len(engrams) == 0 { confirmed at line 361) both exist in the catalog and in TestVerifyCoveragePinsMatchLiveCatalog's table. The 361-363 guard is provably dead (the len(episodes)==0 empty-drain returns earlier; the loop is 1:1) ✓.
Test-file anchor pins: hook_test.go:61 (TestHookCaptureBranchI CC=13), :247 (TestHookBatch CC=22), :359 (TestHookFull CC=21); injector_test.go:109 (TestInjectorEnabledInsert CC=22) — all measured and match. All five concurrency proofs exist (TestHookFlushConcurrentRestoreAndSuccessAppend @1138, TestHookBatchRetainOnFailure @842, TestHookFlushDropsStaleBufferWhenDisabledMidSession @1026, TestHookFlushNilClientDrainAndDrop @1108, TestHookFlushEmptyDrainNoop @1085). Subtest 1 of the concurrent test ("failure path restores into fresh buffer after concurrent drain") does pin the restore-into-fresh-buffer behavior ✓.
ADR-068 (docs/adr/2026-09-automatic-plur-memory-integration.md) confirms §1.5 fail-open-with-strip, §2 detached-3s-ctx, §5 ENABLED master switch with drain-and-drop, §8 observability (in-process Warnings + Info log + Debug skips + session-end dead-tool notice) — all as the issue cites.
Existing precedence pin: only stdio-conflict > missing-credentials is pinned (mcp_config.go comment at :127 "precedes the positive credential rules"; tests use errContains). The TIMEOUT-vs-auth-switch and URL-empty-vs-ARGS positions are documented-but-unpinned ✓.

One discrepancy found (minor): the issue calls injector_test.go:456 (TestMetadataIDsUnsupportedValues) an "ACCEPTED pin". It is not — measured CC=9 (below threshold), absent from expectedCataloged, and the only injector_test.go pin in the catalog is :109. Nothing pins 456. The instruction that follows (append helpers at end of file to protect :109) is still correct, and avoiding drift of the metadataIDs test cluster is sensible — but the ":456 ACCEPTED pin" framing is a drafting error, not a real pin.

3. ## Implementation Plan

Order: A → C → B, one commit per hotspot, in that sequence. A (internal/domain/config/mcp_config.go) is the most isolated hotspot — no cross-file pins, no concurrency, no shared catalog rows — and it establishes the RESOLVED-marking precedent (the handleActionKeys "Complexity after fix" format at INTENTIONAL_NON_FIXES.md) early with the lowest blast radius. C lands second because it carries the only re-anchor of a surviving ACCEPTED entry (metadataIDs injector.go:237), which is the riskiest catalog surgery (row + See: must move in lockstep and the new line is only knowable after the refactor is laid out) — get it done while the file is fresh. B lands last because it is the concurrency-sensitive one: its five proofs and the claim/restore/drain-and-drop discipline deserve the fullest review window, and its three RESOLVED entries plus two coverage-pin touches are the largest single catalog diff. Each commit must pass make verify-nonfix-catalog locally before the next begins; the final PR is the three commits plus, if the AC demands it, the ordering-pin tests folded into the A commit.

Commit A — mcp_config.go: extract the six validators exactly as the check order dictates (transport-shape → command-fields → auth-transport-conflict → execution-limits → auth-mode → credentials), each a private method on the receiver, validate(name) becoming a thin sequential caller. Error strings are copied byte-for-byte — they are pinned by the T1 table (mcp_config_test.go uses errContains on "URL must not be empty", "ARGS/DIR/ENV require COMMAND", etc., and the conflict>credentials precedence is the one documented-pinned ordering). Add the three dual-violation pins in the same commit using require.EqualError with full exact strings: {URL, Auth:"bogus", Timeout:-1} → TIMEOUT-wins; {Args:["--foo"]} no URL/Command → URL-empty-wins; {Command:"npx", Auth:"bearer", Timeout:-1} → mode-conflict-wins (note these inputs are mutually distinct, so no two pins can fire on the same input). Atomically: catalog mcp_config.go:107 → RESOLVED with "Complexity after fix" (list the six values), remove the "(*MCPServerConfig).validate" row from expectedCataloged in real_nonfix_catalog_test.go.

Commit C — injector.go + new injector_pipeline.go: move the four strip sites per the ownership contract — site 1 (disabled) stays in Transform and is the only PersistHistory site; sites 2 and 3 (transport error, result.Error) move into fetchEngramPayload; site 4 (maxBody < 0) moves into applyMemoryTransformation as a strip-without-insert signaled by the bool return (never insert an over-budget block). New helpers resolveUserPrompt, fetchEngramPayload, applyMemoryTransformation, observeInjection go in injector_pipeline.go; signatures take req/result (not the #1417 placeholder shapes). Nil-client guard stays in Transform (Warn memory_client_unavailable); success-path observability moves to observeInjection, preserving the ADR-068 §8 Info surface (memory_injected / memory_injected_no_ids) and the Warnings append. Behavior preservation: Transform returns nil on every path; exact log keys (memory_injection_failed, memory_block_overflow) and value shapes unchanged. Atomically: catalog injector.go:64 → RESOLVED with "Complexity after fix"; remove the "(*plurInjector).Transform" row; re-anchor metadataIDs — measure the new line with the complexity tool post-refactor, update BOTH the expectedCataloged row {Line: <new>} and the catalog See: injector.go:<new> in the same commit. Append new tests at the end of injector_test.go so the real pin at :109 does not drift.

Commit B — hook.go: extract per the budget — isDuplicateTurn (turn-scoped dedupe under one lock), clientUnavailable (nil-client + Warn; keeps AfterTurn at 8 not 10), fetchLastModelTurn (detached 3s ctx + GetLastModelTurn + gerr||content fail-open + text-empty skip), buildEpisode (three-way classification; branch (iii) delegates), claimEpisodes (master-switch gate + lock claim + empty-drain delete), buildEngramPayload (pure mapping, no lock/map side effects), restoreOnFailure (must re-create the map entry if a concurrent flush deleted it — pinned by TestHookFlushConcurrentRestoreAndSuccessAppend subtest 1; restore-at-front; read dropped under lock), finalizeOnSuccess (delete when empty else reset dropped), effectiveScope, claimLearnSlot (sha256 dedupe then flood bound; increment only after both pass; unlock before the MCP call). Keep the engrams-empty belt-and-suspenders in FlushSession immediately after the build call, same disposition (return, no restore, no map delete, no recordWrite); add a helper-contract pin for buildEngramPayload (nil/empty → empty; non-empty → 1:1) but do not claim a behavioral pin for the dead guard. Atomically: catalog hook.go:97/236/300 → RESOLVED with per-function "Complexity after fix" values; remove the three rows; re-anchor the coverage pin hook.go:361-363 to the guard's new line and re-verify hook.go:89-93 (BeforeTurn/OnPhaseTransition shouldn't move — they sit above AfterTurn, but the drift policy demands re-verification whenever a pinned file is touched). New tests appended at the end of hook_test.go so :61/:247/:359 stay put. The five concurrency proofs must pass unchanged.

Assembly and gates: the combined PR targets dev with three independently-green commits (A → C → B). Run make verify-nonfix-catalog after each commit (it runs TestVerifyNonFixCatalog|TestVerifyCoveragePinsMatchLiveCatalog|TestDetailedCoverageReport under -tags=arch against the live repo — the atomicity self-check), then make check and make check-full before the PR. D stays untouched — both toolstest entries remain ACCEPTED, zero catalog churn.

4. ## Risks and Open Questions

The injector_test.go:456 mislabel (Section 2) is the only factual error I found in the spec. It doesn't change the plan — appending at end-of-file is right — but the griller should know the ":456 pin" is not real; the real pin is :109 only.
The B budget table is advisory, not binding. AfterTurn 8 and FlushSession 7 are tight and depend on extraction shape (inline err != nil || result.Error != nil alone is 2 decisions). If the round wants the table itself to be the contract, the exact extraction shape must be pinned in the task spec; as written, only ≤ 10 is enforceable (by the gate) and the table clears it. I'd recommend stating this explicitly in the final plan so the coder doesn't over-fit to "8" and "7".
The metadataIDs re-anchor line is unknowable until the refactor is laid out. The issue correctly mandates row+See: in the same commit and says "re-anchor to the new measured line" — the coder must measure with the complexity tool after placing injector_pipeline.go (the new file's existence doesn't shift injector.go, but Transform's shrink does, ~237 → ~157). A guessed line will break the gate; this needs to be an explicit "measure, then write" step.
The engrams-empty guard's coverage pin is a hostage of the guard's survival. The issue keeps the guard (re-anchor, not delete) — correct, because deleting it would dangle the TestVerifyCoveragePinsMatchLiveCatalog row and require removing that row too, which the issue does not mention. The plan must keep the guard exactly as specified (orchestrator-level, after build) so the pin survives with a re-anchored line.
Ordering-pin inputs must stay mutually distinct. The three new pins use disjoint input shapes (URL+Auth+Timeout; Args-only; Command+Auth+Timeout), so no two can fire on one input — but if a future validator reorder changes TIMEOUT-vs-mode-conflict, the {Command, bearer, Timeout:-1} pin asserts conflict-wins while the documented order lists TIMEOUT before the auth switch — i.e., the documented order and the mode-conflict-wins pin are consistent today only because the conflict check precedes TIMEOUT in the current body. The plan should note that the conflict check must remain ahead of the TIMEOUT check when extracting validateAuthTransportConflict — the extraction order is itself a behavior pin.
What the previous grill could not have known: the six sub-validators sum to 25, not 20 — a reviewer might "discover" this as a regression. It isn't: it's the per-function base-1 cost of the decomposition, and the policy threshold is per-function. Flag it in the commit message to preempt the finding.
make check-full race leg: B's concurrency proofs (goroutine + channel-gated flush) run under -race in check-full; the decomposition must not introduce shared-state access outside h.mu (e.g., buildEngramPayload must stay side-effect-free, per the issue) or the race detector will catch it where the unit tests won't.

The spec is, with that single minor discrepancy, accurate against the committed tree — every CC value, every line pin, and every gate-mechanics claim I checked holds. The plan above executes it in the issue's A → C → B order with the atomicity rule enforced per hotspot.

Q1 — Griller

Bootstrap and verification complete. Here is what I independently confirmed before asking Q1:

Verified against the repo (all six CC values, lines, and gate mechanics — subject's claims hold):

validate @107 CC=20, AfterTurn @97 CC=22, FlushSession @300 CC=18, maybeLearn @236 CC=11, Transform @64 CC=18, metadataIDs @237 CC=11 — all match the tool exactly.
Six-validator decomposition (5+5+4+2+3+6=25, each ≤10) re-derived from the actual validate() body; the auth switch is 2 CaseClauses = CC 3, not 7.
Gate mechanics: filterAcceptedEntries keeps only ACCEPTED; require.Equal(t, expectedCataloged, cataloged) exact-equality; require.Empty(t, alerts); catalogTitleFor exact-line matching — so the atomicity argument (row-only breaks Equal, See-only breaks catalogTitleFor, RESOLVED-while-CC>10 breaks Empty) is exactly right.
nonfix_catalog_partition_test.go does not exist; the gate is real_nonfix_catalog_test.go. Coverage-pin rows hook.go:89-93 and hook.go:361-363 exist in both the gate table and the catalog (ACCEPTED). The 361-363 guard is provably dead. All five concurrency proofs exist at the cited lines (1138/842/1026/1108/1085).
The injector_test.go:456 "ACCEPTED pin" claim is a real drafting error — :456 is absent from expectedCataloged (only :109 is pinned), and its CC=9 is below threshold. The subject's one claimed discrepancy is genuine.

One incoherence I found that the subject inherited and compounded — the subject's "AfterTurn 8" and "the table clears ≤10 with margin" do not reconcile with the issue's own component list or the subject's own plan shape (inline tier dispatch). I'll open with that.

Q1: You state AfterTurn lands at CC 8 and that the B budget "clears [the ≤10 gate] with margin." I traced the arithmetic against the current body (hook.go:97-151) with the tool's method, and neither your plan's shape nor the issue's own component list produces 8. Your plan keeps the tier dispatch inline — capture/batch/full is 3 CaseClauses — and the retained components are: cfg gate (cfg == nil || !cfg.Enabled, if + || = 2), tier-off (1), isDuplicateTurn call (1), clientUnavailable call (1), buildEpisode's !ok (1): base 1 + 2+1+1+1+1+3 = 10 — exactly at the policy threshold, zero margin, and two over your claimed 8. If/else-if instead of switch still gives 9. The only shape that yields 8 is a fourteenth helper (a dispatchTier call = 1 decision at the call site) that appears in neither the issue's budget table nor your plan. The consequence is a hard gate break: if the coder's extraction lands AfterTurn at 11 — one extra guard, e.g. a retained if h.logger != nil for the IsTransient skip, or a (ep, err) from buildEpisode checked with two branches — verify-nonfix-catalog fails with an uncataloged alert on a hook.go function, and because the B commit RESOLVES all three hook.go entries atomically in the same commit, no ACCEPTED catalog entry remains to absorb an 11; you're in a REVISION loop with the catalog half-edited. Reconcile this: (a) what exact extraction shape gives AfterTurn 8, (b) do you retract "clears with margin" for AfterTurn (it is at 10 under your own plan shape, not below), and (c) what does the task spec actually pin so the coder cannot land at 11 — a shape-level not-to-exceed spec for AfterTurn/FlushSession, or an explicit "measure the new function CCs immediately after extraction and before touching the catalog" ordering constraint?

My recommendation: The budget table should be downgraded from "verified" to "targets," and the task spec should pin the tier-dispatch shape explicitly (either a dispatchTier helper costing 1 at the call site, or an if/else-if chain costing 2) plus the ordering rule: refactor → measure all new function CCs with the tool → only then write the RESOLVED entries and remove the rows. That makes the ≤10 guarantee mechanical instead of aspirational, and it's the same measure-then-write discipline you already apply to the metadataIDs re-anchor in C — it's just missing for the B orchestrator targets.

A1 — Architect

(a) The exact shape that yields AfterTurn = 8 — and the issue's own component list does not produce 8.

Your arithmetic is correct, and it exposes an internal inconsistency in the issue's own budget table, which my opening inherited. The issue's AfterTurn row lists six components — "cfg gate, tier-off, isDuplicateTurn, clientUnavailable, !ok, tier switch" — and that list, counted with the tool's method, is 10, not 8: base 1 + cfg gate (if + || = 2) + tier-off (1) + isDuplicateTurn call (1) + clientUnavailable call (1) + buildEpisode !ok (1) + tier switch (3 CaseClauses) = 10. The table value 8 does not follow from the table's own component list. That is a spec defect in #1418, not just in my opening.

The shapes that do give 8 (verified by decision arithmetic against the current body at hook.go:97-151):

Shape 1 — dispatchTier helper (your suggestion, and my recommendation): move the capture/batch/full switch into a helper; the call site costs 1. AfterTurn = base 1 + cfg gate 2 + tier-off 1 + isDuplicateTurn 1 + clientUnavailable 1 + !ok 1 + dispatchTier call 1 = 8. The helper itself = base 1 + 3 CaseClauses = 4. This adds a 14th row to the issue's 13-row B table — the table as filed cannot produce 8 without it.
Shape 2 — merged gate + inline switch: fold cfg == nil || !cfg.Enabled and tier == off into one shouldLearn(cfg) bool (helper = base 1 + gate 2 + tier-off 1 = 4; call site = 1), keep the switch inline (3). AfterTurn = base 1 + 1 + 1 + 1 + 1 + 3 = 8.

One correction to your option (b): an if/else-if chain does not cost 2. In Go's AST, if A {} else if B {} else if C {} is three nested IfStmt nodes — the tool counts each — so a 3-tier chain costs 3, identical to the switch's 3 CaseClauses. If/else-if only reaches 8 if it is also paired with a merged gate (Shape 2). So the realistic pinned choices are exactly Shape 1 or Shape 2; anything with the gates inline and a 3-way dispatch inline lands at 10.

(b) Retraction — "clears with margin" is wrong for AfterTurn.

Retracted, explicitly. Under the shape my opening described (inline tier switch, inline cfg gate and tier-off) AfterTurn is 10 — exactly at the policy threshold, zero margin — and the issue's own component list agrees at 10. I said the B table was "plausible targets, not measurements" and then immediately contradicted myself by claiming margin; the griller caught the inconsistency. The corrected claim: the margin holds for the leaf helpers — isDuplicateTurn 3, clientUnavailable 3, fetchLastModelTurn 5, buildEngramPayload 3, claimEpisodes 5, restoreOnFailure 3, finalizeOnSuccess 3, effectiveScope 2, maybeLearn 6, claimLearnSlot 5 — all ≤ 5, comfortably below 10. It holds for FlushSession = 7 only under the issue's exact helper decomposition: claimEpisodes absorbs the master-switch gate (its 5 = base + gate 2 + !ok + empty-drain), effectiveScope absorbs the scope read, and the inline decisions are then claim-!ok 1 + nil-client 1 + engrams-empty 1 + lock-acquire if ok 1 + err != nil || result.Error != nil 2 = base 1 + 6 = 7. If the master gate stays inline in FlushSession instead of claimEpisodes, it is 9. So FlushSession's 7 is real but contingent on gate placement; AfterTurn's 8 is contingent on a helper the issue never listed. That asymmetry — a pinned decomposition for FlushSession, none for AfterTurn — is exactly what you identified.

The gate consequence you state is also verified: TestVerifyNonFixCatalog alerts only at CC > 10 (if c.Complexity <= 10 { continue }), so an AfterTurn at 10 passes but sits at the boundary; an 11 breaks. And there is no ACCEPTED backstop for an 11: filterAcceptedEntries drops RESOLVED, so once the B commit marks hook.go:97 RESOLVED, catalogTitleFor finds nothing for line 97 — the only other hook.go ACCEPTED entries pin 89-93 (BeforeTurn/OnPhaseTransition) and 361-363 (engrams-empty), and neither interval overlaps AfterTurn's or FlushSession's post-refactor line. So the B commit is all-or-nothing exactly as you say: one stray if h.logger != nil retained for the IsTransient skip, or a (ep, err) two-branch check from buildEpisode, and you're red with a half-edited catalog.

(c) What the task spec must pin — both of your recommendations, and a correction to the AC wording.

Shape-level not-to-exceed for the two orchestrators. The spec must pick Shape 1 or Shape 2 for AfterTurn (I recommend Shape 1 — dispatchTier — because it also centralizes the per-tier action sequence including full-tier bufferAppend + maybeLearn, and it is the shape your arithmetic already names), and must pin FlushSession's gate placement (master switch inside claimEpisodes; effectiveScope extracted; engrams-empty guard immediately after the build call; err condition inline as err != nil || result.Error != nil). The budget table should be rewritten so each row's value follows from its listed components — as filed, AfterTurn's row contradicts its own component list, which is a guaranteed coder trap.
Measure-then-write ordering constraint. Refactor hook.go → run the complexity tool on the file → only then write the RESOLVED entries (with the measured "Complexity after fix" values, not the table's), remove the three expectedCataloged rows, and re-anchor the 89-93/361-363 coverage pins — all in the same commit. This is the identical discipline C already mandates for the metadataIDs re-anchor; I flagged that for C and should have flagged it for B's orchestrator targets. It converts the ≤10 guarantee from aspirational to mechanical: if the measured value is 10 you accept 10 and say so; if it is 11 you reshape before any catalog edit exists to go stale.
Downgrade the table's status from "verified" to "targets" — accepted, and it aligns with what I should have said consistently in the opening.

I also accept the AC should be reworded from "all functions CC ≤ 10" to make the orchestrator budgets shape-bound, not just threshold-bound: "AfterTurn ≤ 8 via dispatchTier (or the pinned alternative), all other B functions ≤ 10" — because as written, a coder landing AfterTurn at 10 has satisfied the AC while missing the issue's own stated budget, and the gate will not complain.

Q2 — Griller

Q1's answer was verified before I proceed: the retraction is honest, and one correction the subject made to me is correct — if A {} else if B {} else if C {} is three nested IfStmt nodes (each else if is an IfStmt in the Else slot; ast.Inspect counts each), so a 3-way chain costs 3, equal to the switch's 3 CaseClauses. I concede that point. The gate-consequence analysis (no ACCEPTED backstop for an 11; filterAcceptedEntries drops RESOLVED) is also verified against nonfix_catalog.go and the gate. But the retraction is incomplete — I traced the same counting convention one row down the table.

Q2: Your Q1 retraction says the undercount disease is AfterTurn's alone and that "the margin holds for the leaf helpers — isDuplicateTurn 3, clientUnavailable 3, fetchLastModelTurn 5, buildEngramPayload 3, claimEpisodes 5, restoreOnFailure 3, finalizeOnSuccess 3, effectiveScope 2, maybeLearn 6, claimLearnSlot 5 — all ≤ 5, comfortably below 10." That paragraph contains the same error you just retracted, one row down. Trace maybeLearn (hook.go:236-300) with the tool's method and your own Q1 convention — you counted the acquireWriteLock if ok guard as a decision in FlushSession's 7, and you counted the err-branch logger guard in fetchLastModelTurn's 5. The current body has: frame gate (1), the claimLearnSlot cluster (nil-init 1, dup 1, flood 1, flood-logger 1), scope (1), lock-acquire if ok (1), and err != nil || result.Error != nil (2) with the nested if h.logger != nil (1) — 11 total, which the tool confirms. Extract claimLearnSlot (its 5 is correct: base + nil-init + dup + flood + flood-logger), and maybeLearn retains base 1 + frame 1 + claim-call 1 + scope 1 + lock-ok 1 + err 2 + err-logger 1 = 8, not 6. The issue's own row ("frame gate, claimLearnSlot, scope, err handling") omits exactly the two decisions AfterTurn's row omitted, and your own sentence contradicts itself — "maybeLearn 6" is not "≤ 5," and under your counting it is 8, two over the table and one over your claimed margin. So the table is now shown wrong in three rows: AfterTurn 8 (needs a missing helper), FlushSession 7 (real only if the master gate sits in claimEpisodes AND the lock-acquire is counted), and maybeLearn 6 (misses lock-acquire + err-logger → 8). The consequence is milder than AfterTurn's — 8 still clears the ≤10 gate — but the plan's stated budgets are fiction in three places, and a coder "optimizing" toward 6 will either distort the shape or rediscover the 8. Where do the lock-acquire guard and the err-branch logger guard go in your plan — extracted into helpers (which rows absorb them, and do their CCs stay ≤10), or left inline with maybeLearn amended to its measured 8 — and do you extend the Q1 measure-then-write correction to all B rows, i.e. the "Complexity after fix" notes in the RESOLVED entries are written from post-extraction measurements, never from the issue's table?

My recommendation: keep the lock-acquire and the err-branch logger guard inline (behavior preservation — the Warn and the flock are mandated by ADR-068 and #1412), amend maybeLearn's row to 8 with a component list that shows the lock-ok and err-logger decisions, and make the AC's governing sentence "all B functions re-measured post-extraction, values written into the RESOLVED notes from measurement" — exactly as C's metadataIDs discipline, which you already accepted. The gate is the contract; the table is at best a target, at worst a coder trap.

A2 — Architect

Your trace is correct, and I retract the maybeLearn 6 row and my own "all ≤ 5" sentence — both were wrong under the convention I myself used for FlushSession and fetchLastModelTurn in Q1.

Verified against the committed body (maybeLearn spans hook.go:236-299; the tool measures 11, matching the decision count exactly): base 1 + frame gate 1 + nil-init 1 + dup 1 + flood 1 + flood-logger 1 + scope 1 + lock-acquire if ok 1 + err != nil || result.Error != nil 2 + err-logger 1 = 11. Extract claimLearnSlot (5 = base + nil-init + dup + flood + flood-logger — its flood-logger absorption is right because it is part of the claim cluster), and maybeLearn retains base 1 + frame 1 + claim-call 1 + scope 1 + lock-ok 1 + err 2 + err-logger 1 = 8. The issue's row "frame gate, claimLearnSlot, scope, err handling" omits exactly the lock-ok and err-logger decisions, the same two omission classes AfterTurn's row suffered. And my Q1 sentence contradicted itself — "maybeLearn 6" is not "≤ 5," and under my own counting it is 8. The table is now shown wrong in three rows, and you are right that the consequence is milder for maybeLearn (8 clears ≤ 10) but the budget is still fiction, and a coder aiming at 6 would either distort the shape or rediscover the 8.

Where the guards go — inline, with the table amended; and one FlushSession under-specification from Q1 I must correct.

For maybeLearn: the lock-acquire and the err-branch logger stay inline, and I accept your rationale as verified: the flock is ADR-068 §4 ("held only across the write MCP calls — non-blocking LOCK_EX|LOCK_NB, polled on a bounded budget, fail-open (log + proceed unlocked)") plus the #1412 unlock-before-the-MCP-call discipline, and the Warn is the ADR-068 §8 / ADR-029 §5 fail-open surface. Extracting them would move the write-lock and the failure log away from the call site for cosmetic CC reduction — the exact over-extraction the repo's own acceptance culture rejects. claimLearnSlot absorbs the claim cluster (5, ≤ 10 with margin); maybeLearn amends to 8 with the component list: base + frame 1 + claimLearnSlot call 1 + scope 1 + lock-ok 1 + err-condition 2 + err-logger 1.

For FlushSession, Q1's "7" was under-specified: I counted the lock-acquire but not the nil-client Warn's logger guard or the failure Warn's logger guard. Under my own uniform convention (every if h.logger != nil counts), FlushSession is 8, not 7, unless the shape pins two absorptions: the nil-client guard + its Warn become the shared clientUnavailable helper (already in the B table at 3, already used by AfterTurn — its 3 = base + nil + logger), and restoreOnFailure absorbs the failure Warn with its logger (its 3 = base + !exists + logger; this also puts the retained/dropped reporting under the same lock as the restore it describes, which is where it belongs). With those two absorptions and the lock-ok + err-condition inline, FlushSession = base 1 + claim-!ok 1 + clientUnavailable 1 + engrams-empty 1 + lock-ok 1 + err 2 = 7 — real under the uniform convention, but only with the shape pinned. If the task spec leaves the nil-client Warn inline, the honest number is 8. Either is ≤ 10; this is a shape choice, not a gate risk — but the spec must pick one and say so, exactly as Q1 concluded for AfterTurn's tier dispatch.

The corrected B table under the uniform convention (every logger guard counted; every row re-derived from the committed bodies):

Function	CC	Decisions beyond base
`AfterTurn`	8 (10 without `dispatchTier`)	cfg gate 2 + tier-off 1 + `isDuplicateTurn` 1 + `clientUnavailable` 1 + `!ok` 1 + dispatch-call 1
`buildEpisode`	9	3-way switch 3 + (i) text-empty 1 + (i) err-annotate 1 + (ii) IsTransient 1 + (ii) logger 1 + (iii) `!ok` 1
`fetchLastModelTurn`	5	`gerr\|\|content` 2 + logger 1 + text-empty 1
`isDuplicateTurn`	3	dup-if 1 + `&&` 1
`clientUnavailable`	3	nil 1 + logger 1
`FlushSession`	7 (8 with inline nil-client Warn)	claim-`!ok` 1 + `clientUnavailable` 1 + engrams-empty 1 + lock-ok 1 + err-condition 2
`claimEpisodes`	5	gate 2 + `!ok` 1 + empty-drain 1
`buildEngramPayload`	3	range 1 + scope 1
`restoreOnFailure`	3	`!exists` 1 + logger 1
`finalizeOnSuccess`	3	exists 1 + empty 1
`effectiveScope`	2	cfg-non-nil 1
`maybeLearn`	8	frame 1 + claim-call 1 + scope 1 + lock-ok 1 + err-condition 2 + err-logger 1
`claimLearnSlot`	5	nil-init 1 + dup 1 + flood 1 + flood-logger 1

Every row ≤ 10; the tightest are buildEpisode 9 (margin 1) and the three orchestrators at 7-8/10. My Q1 "margin" claim now stands only for the true leaves (all ≤ 5) and is withdrawn for the orchestrators.

Measure-then-write extends to all B rows — yes, and to all three hotspots, not just C. The RESOLVED "Complexity after fix" notes for hook.go:97/236/300 (and, for the same reason, the notes for mcp_config.go:107 and injector.go:64) are written from post-extraction measurements — the tool run on the refactored file — never from the issue's table, which has now been demonstrated wrong in three rows and cannot be trusted as a source of truth for note content. The gate validates only ≤ 10, not the note's numbers, so nothing but this ordering constraint keeps fiction out of the catalog. The sequencing already implied it (the row removal can't be written until the function is known ≤ 10), but the note content needs it stated explicitly, and it is the same metadataIDs discipline from C, generalized.

Recommendation accepted, with the amendments above: lock-ok + err-logger inline in maybeLearn (8), nil-client Warn and failure Warn absorbed by clientUnavailable/restoreOnFailure so FlushSession's 7 is real under the uniform convention, dispatchTier added to the table for AfterTurn's 8, and the AC's governing sentence restated as: all B functions re-measured post-extraction, RESOLVED "Complexity after fix" notes written from measurement, never from the table; the ≤ 10 gate is the contract, the budget table is a corrected target. A coder "optimizing" toward a stale 6 has no remaining excuse — the amended rows give the true targets, and the ordering rule makes the gate the enforcement point.

Q3 — Griller

ADR-068 §8 covers the observability surface (Warnings + Info log line + telemetry) but does not enumerate the per-site phase values — so the phase distinction is code-level surface, not ADR-cited. I have everything needed for Q3.

Q3: Your Q2 corrected table re-derives cleanly under the uniform convention — I re-counted all 13 rows against the committed bodies and they hold, buildEpisode 9 and validateCommandFields 5 included (no further wrong rows), and your FlushSession row "7 (8 with inline nil-client Warn)" is self-consistent. But the correction's centerpiece — sharing clientUnavailable between AfterTurn and FlushSession — has a silent-defect trap the plan does not cover, and I verified it in the committed tree. The memory_client_unavailable Warn carries a per-site phase value: "learn" at hook.go:121 (AfterTurn), "learn_batch" at hook.go:339 (FlushSession), and "inject" at injector.go:85 (Transform). Your shared helper at "3 = base + nil + logger" has no phase parameter, and a coder writing clientUnavailable() will hardcode one of the two values — silently collapsing the other site's log surface. The existing tests will not catch it: TestHookFlushNilClientDrainAndDrop asserts logger.warnValue("memory_client_unavailable", "phase") — key presence only, not the value (hook_test.go:1126) — and there is no AfterTurn nil-client Warn assertion at all (TestHookNilClient only checks no-panic). So a phase-1 hardcode passes the entire suite while the FlushSession Warn drifts from "learn_batch" to "learn" — exactly the class of un-pinned observability drift this round exists to prevent, and an asymmetry worth noting: the failure Warn you moved into restoreOnFailure is value-pinned (TestHookBatchDropCountReported asserts dropped==5/retained==20), so the coder has a guard there, but the nil-client phase has none. The C contract separately pins the injector's nil-client Warn in Transform with "inject", so the helper cannot absorb that site anyway — it is hook-only. So: does the corrected plan specify clientUnavailable(phase string) bool (parameters add no decisions — CC stays 3 — so the table row survives), and should the AC or the new tests pin the phase values — extend TestHookFlushNilClientDrainAndDrop to assert phase == "learn_batch" and add the AfterTurn-side assertion for "learn" — so the shared helper's parameterization is enforced rather than assumed?

My recommendation: specify clientUnavailable(phase string) bool in the task spec (one line, CC unchanged at 3, both call sites pass their literal), and add the two value assertions — it is a two-line test change that converts a silent drift into a pinned contract, consistent with how TestHookBatchRetainOnFailure/TestHookBatchDropCountReported already value-pin the learn_batch_failed Warn. The alternative — leaving the helper unparameterized — guarantees one of the two sites changes its logged phase, which a reviewer checking ADR-068 §8's per-site observability surface would flag post-merge.

A3 — Architect

Verified — the trap is real, and every claim you made checks out against the committed tree. The three memory_client_unavailable sites carry distinct phase literals: "learn" at hook.go:121 (AfterTurn), "learn_batch" at hook.go:339 (FlushSession), "inject" at injector.go:85 (Transform). TestHookFlushNilClientDrainAndDrop asserts warnValue("memory_client_unavailable", "phase") at hook_test.go:1126 — key presence only, value unchecked. The AfterTurn-side tests are even thinner: TestHookNilClient (hook_test.go:511) and TestHookNilClientFlush (:516) assert only "must not panic," and both use newTestHook, which wires a NoOpLogger — there is no recording logger in play, so no Warn assertion is even possible without changing the harness. Meanwhile TestHookBatchDropCountReported value-pins the failure Warn (dropped == 5, retained == 20), so the asymmetry is exactly as you describe: the restoreOnFailure Warn is guarded, the nil-client phase is not. A phase-1 hardcode in a shared helper passes the entire suite.

The corrected plan specifies clientUnavailable(phase string) bool — accepted. Parameters add no decisions under the tool's method (calculateComplexity counts only IfStmt/ForStmt/RangeStmt/CaseClause/CommClause and &&/||; a parameter is none of those), so the row survives at 3 = base 1 + if h.client == nil 1 + if h.logger != nil 1. The shape must be pinned precisely, because the two call sites have different side-effect requirements: AfterTurn's call is if h.clientUnavailable("learn") { return } (pure check + Warn), while FlushSession's is if h.clientUnavailable("learn_batch") { drain-and-drop; return } — the delete of the buffer entry stays at the FlushSession call site, never inside the helper, or the #1414 drain-and-drop semantics leak into AfterTurn. So the helper is strictly check + log + bool; callers keep their own side effects. One precision on your injector point: injector.go and hook.go are the same package (package memory), so the helper is reachable from Transform — but the C contract pins the nil-client guard in Transform with "inject", and C is adjudicated; I recommend the helper stay hook-only and Transform's guard remain untouched, exactly as you say.

Yes — pin the phase values, and the AC gains the clause. Two test changes: (1) extend TestHookFlushNilClientDrainAndDrop's final assertion from key-presence to value — if v, ok := logger.warnValue("memory_client_unavailable", "phase"); !ok || v != "learn_batch" — mirroring the dropped/retained value pattern already in TestHookBatchDropCountReported; (2) add the AfterTurn-side assertion for "learn" — which requires switching TestHookNilClient from newTestHook to newTestHookWithLogger (it currently uses a NoOpLogger, so the assertion cannot be added in place; that is a slightly larger edit than two lines but still trivial, and it is the harness change that makes the Warn observable). The AC's governing sentence gains: "the shared clientUnavailable(phase string) bool helper is parameterized, and the nil-client Warn phase values ("learn", "learn_batch") are pinned by tests" — folded into the behavior-preservation clause, with the tests as the enforcement point and the task spec carrying the one-line signature so the coder cannot invent an unparameterized variant. This converts silent drift into a pinned contract, consistent with the learn_batch_failed value-pins already in the suite.

Two incidental notes for the B commit's re-anchor sweep: TestHookFlushNilClientDrainAndDrop's doc comment cites hook.go:334-342 — a prose pin, not a gate row, but it will drift with the decomposition and should be updated in the same commit (nothing enforces it; it is exactly the class of stale reference the drift policy warns about). And the helper's logger-nil path (h.client == nil && h.logger == nil → returns false, no panic) is not exercised by either site test — an optional direct unit pin on the helper; I'd add it since the helper is new and the no-logger path is otherwise dead in tests.

Recommendation accepted in full — parameterized helper, two value assertions, hook-only scope, and the AC clause. The one amendment to your framing: the AfterTurn-side assertion is not purely additive, because TestHookNilClient's current harness cannot observe Warns; the task spec should say "switch TestHookNilClient to a recording logger and assert phase == "learn"," not merely "add an assertion."

Q4 — Griller

Q3's answer verified: the drain-and-drop-delete-stays-at-the-call-site precision is correct (it's what keeps #1414 semantics from leaking into AfterTurn), the same-package note is correct (injector.go and hook.go are both package memory), and the harness-switch amendment (TestHookNilClient uses newTestHook, whose NoOpLogger cannot observe Warns) is accurate. I also closed out two threads myself: the four strip sites in injector.go map 1:1 onto the C contract (all four are stripMemoryBlock(req); only site 1 sets PersistHistory), and the A-plan ordering question is settled — the current body runs conflict (check 5) before TIMEOUT (check 6) before the auth switch (7), which is exactly the documented order in the mcp_config.go:107 catalog rationale, so the {Command, bearer, Timeout:-1} → conflict-wins pin is consistent with both; your Risk 5's practical conclusion (extraction order is a behavior pin) holds.

Q4: You claim the combined PR delivers "three independently-green commits" and that "each commit must pass make verify-nonfix-catalog locally before the next begins." I verified what that gate actually compiles and runs — and it cannot detect a broken commit in any of the three hotspots. The Makefile target is go test -v -tags=arch -run "TestVerifyNonFixCatalog|TestVerifyCoveragePinsMatchLiveCatalog|TestDetailedCoverageReport" ./internal/tools/analysis: it builds only the analysis package and runs only those three tests. It does not compile internal/domain/config (commit A's package), does not compile internal/agent/memory (commits C and B's package), and does not execute a single config or memory test. Consequences under your own plan: a compile break in mcp_config.go — or a test regression in the validate decomposition — passes the per-commit gate (the gate never builds the file), the three new require.EqualError ordering pins you add in commit A never execute (they live in mcp_config_test.go, which the gate never runs), and the breakage surfaces only at the final make check on the assembled PR — the exact opposite of "independently green," which the issue's AC states and your plan claims to enforce. The atomicity self-check verifies the catalog partition, not the refactor's correctness. So: what is the actual per-commit gate — verify-nonfix-catalog alone, or verify-nonfix-catalog plus go build ./... and go test ./internal/domain/config/... (A) / go test ./internal/agent/memory/... (C and B) — and do you agree that "independently green" is unenforced as written, i.e. a touched-package test failure discovered at PR time is a plan-level deviation that should have been caught at the commit?

My recommendation: redefine the per-commit gate as make verify-nonfix-catalog + go build ./... + go test on the touched package (config for A; memory for C and B), and state in the task spec that a touched-package test or compile failure is a REVISION trigger before any catalog edit — so the catalog row removal and RESOLVED marking happen only after the refactored package is proven green, not merely after the analysis gate passes. This closes the gap where commit A's own new pins are the first thing a later commit's test run would catch, and it makes the issue's "independently green" AC mechanically true instead of rhetorical.

A4 — Architect

Verified — the conclusion is right, but three pieces of your evidence are factually off, and the corrections actually sharpen the recommendation. Let me state them precisely, then accept the fix.

What the gate actually compiles and runs (verified in the Makefile and the test bodies): verify-nonfix-catalog = go test -v -tags=arch -run "TestVerifyNonFixCatalog|TestVerifyCoveragePinsMatchLiveCatalog|TestDetailedCoverageReport" ./internal/tools/analysis. The gate binary statically imports neither internal/domain/config nor internal/agent/memory (confirmed: zero domain/config imports in internal/tools/analysis/*.go). But the -run filter also matches the six TestDetailedCoverageReport_* tests, and each spawns a real go test -coverprofile subprocess via toolchain.NewGoRunner — on ./internal/domain/config, ./internal/agent, ./internal/agent/session/context, ./internal/tools/integrations/skillssh, ./internal/infrastructure/llm/openai, ./internal/infrastructure/config. So the literal claims "the gate never builds the file" and "does not execute a single config test" are false:

Commit A compile break in mcp_config.go — CAUGHT. The config subprocess fails to build, writes no profile, getDetailedCoverageReport hits if err != nil && len(blocks) == 0 { return "", err } (coverage_parser.go:474-475), and require.NoError in TestDetailedCoverageReport_CatalogedGapsNotActionable fails.
Commit B/C compile break in memory's production files — CAUGHT, incidentally and asymmetrically. internal/agent/agent.go:14 imports internal/agent/memory, so the ./internal/agent coverage subprocess compiles it as a dependency. A compile break in hook.go/injector.go fails that subprocess. But a compile break in memory's test files (hook_test.go/injector_test.go) is not caught — no subprocess builds the memory test binary.
Commit A test regression — NOT caught, despite the tests executing. The config subprocess runs mcp_config_test.go including your new ordering pins, but go test -coverprofile writes the profile even on failure, so getDetailedCoverageReport returns a "⚠️ WARNING: coverage error" report with a nil error (coverage_parser.go:486-491) and require.NoError passes. A failing ordering pin is swallowed.
Commit B/C test regression — NOT caught, because memory's own tests never execute anywhere in the gate. The ./internal/agent subprocess runs the agent package's tests, not memory's.

So the asymmetry is worse than "the gate never builds the file": it incidentally compiles both production files (via two different subprocesses) but swallows every runtime test failure in config and never runs memory's tests at all. None of it is contractual — it is a side effect of coverage-pin tests, and the degrade path at coverage_parser.go:486-491 is a designed blind spot (coverage generation is best-effort). Your conclusion stands: verify-nonfix-catalog validates the catalog partition, not the refactor's correctness, and "independently green" is unenforced as written.

Accepting the recommendation in full, with the ordering made explicit:

Per-commit gate (binding): make verify-nonfix-catalog + go build ./... + go test ./internal/domain/config/... (commit A) / go test ./internal/agent/memory/... (commits C and B). The go test on the touched package is the correctness gate — it is what runs the A-commit require.EqualError ordering pins and the B-commit concurrency proofs in the commit that introduces them, closing exactly the gap where "commit A's own new pins are the first thing a later commit's test run would catch."
Ordering constraint (binding): refactor → go build ./... and go test <touched package> green → only then write the catalog edits (RESOLVED status + "Complexity after fix" notes + expectedCataloged row removals + coverage-pin re-anchors) → then make verify-nonfix-catalog to validate the partition. A touched-package compile or test failure is a REVISION trigger before any catalog edit — the catalog never carries an entry that references an unproven refactor.
Retraction: my opening's "each commit must pass make verify-nonfix-catalog locally before the next begins" is retracted as sufficient — it is necessary (partition validation) but not sufficient; the touched-package gate is the actual per-commit correctness gate. I also retract the implicit claim that the three commits were "independently green" under the gate as written — they were green under a partition check, not a correctness check.
AC amendment: "all commits independently green" becomes mechanically true by reference: each commit passes verify-nonfix-catalog + go build ./... + go test on its touched package, in that ordering — rather than rhetorical.

One consequential note the correction surfaces: because the config subprocess swallows test failures by design, the final make check is the first place a config test regression would have surfaced under the old plan — and under the new plan it is the per-commit go test ./internal/domain/config/... instead. That is the single highest-value line in the amended task spec, and it is now explicit.

Q5 — Griller

Q4's corrections verified against the committed tree — the subject is right on all three: getDetailedCoverageReport returns ("", err) only when err != nil && len(blocks) == 0 (coverage_parser.go:473-475, so a compile break with no profile → require.NoError fails), while a test failure that still writes a profile yields blocks plus the "⚠️ WARNING: coverage error" prefix with a nil error (lines ~486-491, so the require.NoError passes) — the swallow is real, and the memory-package asymmetry (agent.go imports memory as a dependency, so production compile breaks are caught incidentally; memory's own tests never run) is accurate. The accepted per-commit gate stands.

Now the C-plan seam — I read injector_test.go in full and found the ordering gap:

Q5: Both the issue's four-site table and your C plan place site 1 (disabled) and the nil-client guard "in Transform" — but neither pins their relative order, and the committed tree shows it is unpinned by any test. The current body checks disabled first (injector.go:69-75: strip + PersistHistory=true, silent) and nil-client second (injector.go:79-86: Warn memory_client_unavailable, phase "inject", no strip). I verified no test exercises the disabled+nil-client combination: TestInjectorDisabledStrip uses newTestInjector(t, mock, false) with a non-nil mock client, TestInjectorNilClient uses newTestInjector(t, nil, true) with Enabled=true — there is no {Enabled:false, client:nil} case anywhere in injector_test.go, and the disabled-path assertions (PersistHistory set, no MCP call) would still pass under a reordered Transform because the mock is non-nil. Consequence: if the coder writes the natural "guard nil first" shape — if t.client == nil { warn; return } before if !cfg.Enabled { strip; persist } — then a disabled hook with a DI-fixed nil client (the exact hot-reload scenario #1414 addresses) returns at the nil-client guard without stripping: the stale marker block survives in the Context, PersistHistory is never set, and the strip-on-disable guarantee silently breaks — a fail-open violation on the memory-single-block surface that no gate row, no coverage pin, and no existing test would catch, since the #1414 disabled-mid-session integration covers the hook (TestHookDisabledMasterSwitch_*), not the injector. So: does the task spec pin the Transform guard order — the disabled check strictly before the nil-client guard, as today — and do you accept a new pin test ({Enabled:false, client:nil, marker present} → strip + PersistHistory=true + no memory_client_unavailable Warn) as part of commit C, closing what is now the third ordering the issue's C contract leaves unspecified?

My recommendation: add one line to the C task spec — "the disabled check precedes the nil-client guard, unchanged from today; the nil-client guard is enabled-path-only" — and extend TestInjectorDisabledStrip with a client: nil subtest asserting strip + persist + no Warn. It is a two-line test addition that converts a silent corner into a pinned contract, exactly the pattern we've applied to the clientUnavailable phase values and the dual-violation pins; without it, the C refactor's correct behavior rests on the coder happening to preserve an undocumented ordering.

A5 — Architect

Verified — every claim checks out, and the ordering is load-bearing, not cosmetic. In the committed tree the guard chain is if cfg == nil @ injector.go:66 → if !cfg.Enabled @74 (strip @75-77, PersistHistory = true @76) → if t.client == nil @83 (Warn @85, phase "inject") — disabled strictly precedes nil-client. The test enumeration confirms the gap: TestInjectorDisabledStrip (injector_test.go:24) uses newTestInjector(t, mock, false) at :68 — a non-nil mock in all three table cases, so its assertions (PersistHistory, history shape, no-marker, no MCP calls) hold identically under a reordered Transform because the nil guard never fires. TestInjectorNilClient (:216) uses newTestInjector(t, nil, true) at :217 — Enabled=true. Across all 14 newTestInjector calls, the only false is :68 (with mock), the only nils are :217 and :340 (both true); no {Enabled:false, client:nil} case exists.

The consequence is real and worse than a stale block: the disabled+nil combination is a designed state, not a corner. ADR-068 §5's two-stage fallback constructs the memory components with "effective ENABLED = false" precisely when the client is unavailable — so disabled AND nil client coexist by construction, and #1414's mid-session-disable adds the runtime variant. Under nil-first, that configuration would fire a spurious memory_client_unavailable Warn (the disabled path is mandated silent — "silent, mirroring the injector's disable path") and return without stripping: the marker block survives in req.History, PersistHistory is never set, and the strip-on-disable guarantee (ADR-068 §1.3 content-driven strip; §11 matrix "Disabled: one-shot persisted strip") fails for as long as the client stays nil — the stale block keeps riding into the Context system message on every Prepare pass. No gate row, no coverage pin, no existing test touches it, and the #1414 integration covers the hook, not the injector. The issue's own C contract implies the order — "site 1 is the only persist site" and the nil guard is "no strip" only compose if the disabled strip is reachable in every disabled configuration — but never states it.

Recommendation accepted, with one refinement on the "no Warn" assertion. The task spec gains the line: "the disabled check precedes the nil-client guard, unchanged from today; the nil-client guard is enabled-path-only." I would state it as the full chain — cfg-nil → disabled → nil-client — because ADR-068 §5 literally scopes the guard to "hot-reload ENABLED=true with a DI-fixed nil client," so enabled-path-only is the ADR's own scoping, not an accident of today's body, and pinning the whole chain forecloses a future reorder of the cfg-nil check too. The subtest: {Enabled:false, client:nil, marker present} → strip + PersistHistory=true + no MCP call + no memory_client_unavailable Warn. The refinement: the "no Warn" leg needs a logger-observing harness the injector tests do not currently have — newTestInjector wires &ports.NoOpLogger{} (memory_test.go:147), recordingLogger exists only in hook_test.go (same package, so package-visible), and every injector test asserts the in-process surface (req.Metadata.Warnings), never the logger. So commit C should add a newTestInjectorWithLogger variant mirroring the hook's newTestHookWithLogger — a small harness addition, slightly more than "two lines," but the same pattern and cost class, and it also unlocks the mirror-image assertion (enabled+nil → Warn present, phase "inject") if we want the injector side of the Q3 phase contract pinned symmetrically.

This is the third ordering the issue's contracts left unspecified — the A dual-violation pins, the B clientUnavailable phase values, and now the C disabled-before-nil-client order — all the same class of silent corner that only a pin test converts into a contract. The corrected plan carries all three, and the C commit's task spec now pins the guard chain explicitly so the coder's "natural" nil-first shape is foreclosed before it is written.

Q6 — Griller

Q5's answer verified — the guard chain (cfg-nil @66 → disabled @74 → nil-client @83) matches the committed tree, the test enumeration is accurate (the only newTestInjector false is @68 with a non-nil mock; the only nils are @217 and @340, both enabled), and the ADR-068 §5 two-stage fallback (grep earlier showed "Enabled-but-absent-server → warn and disable (two stages)" at ADR lines 230-241) supports the claim that disabled+nil is a designed state, not a corner. The harness refinement (injector tests assert only req.Metadata.Warnings; recordingLogger lives in hook_test.go) is correct. Q5 is settled.

Q6: You accepted the per-commit gate — verify-nonfix-catalog + go build ./... + go test <touched package> — as "the actual per-commit correctness gate," and commit B's touched package is internal/agent/memory. But plain go test ./internal/agent/memory/... does not run the race detector, and your own risk #7 concedes the enforcement point for B's lock discipline is exactly that detector: "the decomposition must not introduce shared-state access outside h.mu … or the race detector will catch it where the unit tests won't." The two concurrency proofs in that package — TestHookFlushConcurrentRestoreAndSuccessAppend (channel-gated, two goroutines, exercising the restore-into-fresh-buffer and mid-call-append edges) and TestHookConcurrentAfterTurnAndFlush (16 goroutines + a concurrent flush) — execute deterministically without -race and validate behavior only; the data-race detection that makes them meaningful happens exclusively in the final check-full's test-race leg, which runs package-by-package over the whole repo. So under the gate you just accepted as binding, a race introduced by commit B's extraction of restoreOnFailure/finalizeOnSuccess/claimEpisodes — e.g. reading dropped or touching h.buffers outside h.mu — surfaces only at the assembled PR, a REVISION loop on the full pipeline, the exact deferral the new gate was designed to eliminate and the same "independently green" fiction we retired in Q4. The fix is a flag, not a design change: per-commit go test -race ./internal/agent/memory/... for commits C and B — the Makefile's test-race target is already package-by-package, one small package is seconds, and A's config package needs no -race. Does the corrected gate include -race for the memory package, and do you agree that as you stated it, commit B's race-correctness is still validated for the first time at PR time?

My recommendation: make the per-commit gate for C and B go test -race ./internal/agent/memory/... (A stays plain go test ./internal/domain/config/...), so the race detector runs in the commit that introduces the extraction — making the final check-full race leg a backstop rather than the first detection point, and completing the gating story: compile, unit behavior, catalog partition, and race-correctness are all validated per-commit for the one package where concurrency is the point.

A6 — Architect

Verified — the griller is right, and the correction closes a genuine self-contradiction in my own plan. The Makefile test-race target iterates go list ./... with go test -race -timeout 180s <pkg> package-by-package, so the detector runs on internal/agent/memory only in the final check-full leg, never in my accepted per-commit gate. The two named proofs are confirmed in the committed tree — TestHookFlushConcurrentRestoreAndSuccessAppend (hook_test.go:1138, channel-gated, one spawned flush goroutine racing the main goroutine's second flush, per subtest) and TestHookConcurrentAfterTurnAndFlush (16 goroutines + a concurrent FlushSession) — and under plain go test they execute deterministically and assert behavior only; the data-race detection that makes them meaningful requires -race, which my Q4-accepted gate never invoked for the memory package. And my own opening's risk #7 said exactly that: "the decomposition must not introduce shared-state access outside h.mu … or the race detector will catch it where the unit tests won't." So as stated, commit B's race-correctness was validated for the first time at PR time — a REVISION loop on the full pipeline, the precise deferral Q4 retired. Retracted as incomplete.

The corrected gate includes -race for the memory package — accepted. For commits C and B: make verify-nonfix-catalog + go build ./... + go test -race ./internal/agent/memory/...; for commit A: go test ./internal/domain/config/... stays plain (the validate decomposition introduces no concurrency — pure sequential validation on one receiver). The ordering constraint from Q4 is unchanged, with the race leg folded into "package green": refactor → go build ./... + go test -race ./internal/agent/memory/... green → only then catalog edits → make verify-nonfix-catalog. I accept -race for C as well as B, not just B: the injector is documented "fully stateless" and the new helpers touch only req/result, but the flag is one small package, seconds, and it race-validates the new injector_pipeline.go helpers in the commit that introduces them rather than a later one. The task spec gains one line per hotspot: C: go test -race ./internal/agent/memory/... and B: go test -race ./internal/agent/memory/....

Consequences, stated precisely: the final check-full race leg becomes a backstop rather than the first detection point — a race introduced by the B extraction of claimEpisodes/restoreOnFailure/finalizeOnSuccess (e.g., reading dropped or touching h.buffers outside h.mu) now fails the commit, not the assembled PR. The five concurrency/behavior proofs all execute under the race detector per-commit (the griller named the two goroutine-heavy ones; the other three — retain-on-failure, disabled drain-and-drop, nil-client drain-and-drop, empty-drain no-op — are single-goroutine behavior pins that run in the same -race invocation and gain detector coverage for free). The package's flock interaction is also covered: newTestMemoryHome (memory_test.go:150-165) makes acquireWriteLock succeed so the call-site if ok guards are live, and the detector validates the lock handoff alongside the mutex discipline. No -race incompatibility is known in this package — check-full already runs it in CI today.

The gating story is now complete and mechanical: compile (go build ./...), unit behavior (go test -race on the touched package), catalog partition (verify-nonfix-catalog), and race-correctness (-race, in the commit that introduces the extraction) are all validated per-commit for the one package where concurrency is the point; A is validated for compile + unit behavior + partition. The issue's AC "make check-full passes" survives as the assembled-PR backstop, and "all commits independently green" is now true in the strongest sense the round has demanded — including for the detector, which was the one leg my Q4-accepted gate still deferred.

ROUND COMPLETE — Griller

Q6's answer is fully verified — the Makefile test-race target is package-by-package over go list ./..., the two goroutine-heavy proofs sit at the named lines (hook_test.go:1138; the 16-goroutine concurrent test I read in full), and the newTestMemoryHome flock claim matches the harness comment I read ("pre-creates ~/.plur so the flock guard paths run for real"). The corrected gate — -race for C and B per-commit — closes the last deferral; the final check-full race leg is now a backstop, not the first detection point.

Before declaring: I independently closed out the three remaining threads the orchestrator listed, with the repo as ground truth —

A-pin exact strings: all three are byte-for-byte derivable from the current validate() body — TIMEOUT "MCP_SERVERS.<name>.TIMEOUT must be >= 0, got -1", URL-empty "…URL must not be empty (or set COMMAND for a stdio server)", conflict "…AUTH bearer/basic requires an HTTP endpoint; stdio (COMMAND) servers transmit no credentials". On require.EqualError vs the suite's errContains: they are complementary, not conflicting — the T1 fragments pin substrings, and the EqualError pins are the verification mechanism for A's own AC ("exact error strings preserved"); errContains alone could not prove byte-for-byte preservation. The choice is correct, not over-pinning.
C seams: moving sites 2/3 into fetchEngramPayload changes nothing behavioral — all four sites are the same stripMemoryBlock(req) in-memory call, only site 1 sets PersistHistory, and the Warn values (err vs result.Error) are per-site identical to today. No test pins the full Transform call sequence, and none is needed beyond the Q5 pin: the err-before-result.Error order is first-fires-returns with identical effects, and the trim structurally cannot precede the call (it consumes result.Text).
Ordering/PR shape: A→C→B holds under the complete per-commit gate — the commits touch disjoint files (config; injector.go+injector_pipeline.go; hook.go), the catalog and expectedCataloged edits are sequential with each intermediate state gate-green, and C's metadataIDs re-anchor is unaffected by B. One combined PR is right — three split PRs would collide on the shared catalog file and the shared partition table.

ROUND COMPLETE.

The round's value was in the corrections, all now folded into the plan: (1) the issue's B-table arithmetic was fiction in three rows (AfterTurn 8→10, maybeLearn 6→8, FlushSession 7→7-only-with-pinned-absorptions) — the table is retracted as targets, shapes pinned, and measure-then-write generalized to all three hotspots so the RESOLVED notes carry measured values, never table values; (2) the shared clientUnavailable helper is parameterized by phase with value pins, and the disabled-before-nil-client guard order in Transform is pinned with a test; (3) the per-commit gate is now verify-nonfix-catalog + go build ./... + go test -race on the touched package, making compile, unit behavior, partition, and race-correctness all mechanical per-commit — "independently green" is true in the strongest sense, with check-full as backstop; (4) the injector_test.go:456 mislabel stands as the issue's one confirmed drafting error (only :109 is a pin), and the NotContains label in the A scope is the second minor one (the existing conflict>credentials pin is Contains-based, as the subject's opening already noted). The diagnosis that held throughout: every CC value, line pin, catalog entry, gate mechanism, and the atomicity argument verified exactly as claimed — the spec's measurements were sound; its budget arithmetic and a few labels were not. Recommendation: proceed with the plan as corrected, with the issue's B-table amended, the three pin tests added, and the per-commit gate including -race for the memory package.

VERDICT — Griller

VERDICT: The subject's diagnosis held where it mattered most — every measurable claim (all six CC values, every line pin, the catalog entries, the gate mechanics, the atomicity argument, the dead-guard analysis, and the :456 mislabel) verified exactly against the committed tree, and the subject retracted honestly and without resistance under correction. It failed where precision mattered: the opening inherited and compounded the issue's budget-table fiction (AfterTurn 8, maybeLearn 6, FlushSession 7) while asserting "clears with margin," and the subject's own corrections introduced three gaps the round closed — the unparameterized shared clientUnavailable helper (silent phase-value drift), a per-commit gate that neither compiled nor raced the touched package, and the unpinned disabled-before-nil-client ordering in Transform. Recommendation: proceed with changes — execute the plan as corrected (amended B-table with pinned shapes and measured "Complexity after fix" notes, phase-parameterized helper with value pins, the disabled-before-nil-client pin test, and the per-commit gate of verify-nonfix-catalog + go build ./... + go test -race <touched>), and fold the round's amendments back into issue #1418's B-table and AC wording before implementation so the coder is not re-exposed to the same traps.

POST-GRILL POSITION — Architect

POST-GRILL POSITION:

What I stand by. The core diagnosis survived the round intact: every measurable claim in my opening verified exactly against the committed tree — all six CC values (validate 20@mcp_config.go:107, AfterTurn 22@97, FlushSession 18@300, maybeLearn 11@236, Transform 18@64, metadataIDs 11@237), every line pin (catalog See lines, hook_test.go:61/247/359, injector_test.go:109, coverage pins 89-93/361-363), the partition-gate mechanics (filterAcceptedEntries ACCEPTED-only, expectedCataloged exact-equality, the row-vs-See atomicity argument), the dead-guard analysis (engrams-empty is provably dead by the 1:1 builder invariant), and the one spec error I found independently (:456 mislabeled as an ACCEPTED pin — it is CC=9 and unpinned). The issue's core architecture — A→C→B order, the governance atomicity rule, error-string/check-order preservation, the metadataIDs re-anchor discipline, D untouched — is sound and executable as corrected.

What I revised under the grill, and why. The round's real yield was the correction of my own inherited optimism. Q1/Q2 forced the retraction of "clears with margin": the issue's budget table was fiction in three rows — AfterTurn 8 contradicts its own component list (10 under the tool's method), maybeLearn 6 omits the lock-acquire and err-logger decisions (measured 8 with claimLearnSlot extracted), and FlushSession 7 is real only under a pinned shape — so the table is now a corrected target with a uniform counting convention applied to all 13 rows and the ≤10 gate as the contract. Q3 pinned the shared clientUnavailable(phase string) bool helper (CC unchanged at 3) with value assertions for "learn"/"learn_batch" — the harness note that TestHookNilClient needs a recording logger was my addition. Q4 redefined the per-commit gate (verify-nonfix-catalog + go build ./... + go test ) and corrected the griller's evidence: the gate incidentally compiles both production files via coverage subprocesses (config directly; memory through agent.go's import), but swallows config test failures through getDetailedCoverageReport's warning-degrade path and never runs memory's tests — which is precisely why the explicit gate is needed. Q5 pinned the disabled-before-nil-client ordering in Transform (load-bearing per ADR-068 §5's two-stage fallback, where disabled+nil coexist by design) with a pin test. Q6 closed the last deferral: -race on the memory package per-commit for C and B, making check-full's race leg a backstop rather than the first detection point.

Open questions I leave the orchestrator and coder. First, the verdict's folding instruction is now the critical path: the amended B-table (AfterTurn 8 via a chosen shape, FlushSession 7, maybeLearn 8, buildEpisode 9), the three new pin tests, the clientUnavailable(phase string) signature, the disabled-before-nil-client line, and the per-commit gate definition must be written back into #1418's task spec and AC wording before implementation — otherwise a fresh coder session re-exposes itself to the exact traps this round closed; I recommend the amendments land as an explicit issue update, not silently. Second, the AfterTurn shape must be settled: I recommend dispatchTier (a 14th table row, call site = 1) over the merged-gate alternative — both yield 8, both are ≤10, but the spec must name one. Third, the measure-then-write discipline now binds all three hotspots: RESOLVED "Complexity after fix" notes come from post-extraction tool measurements, never from the table — the metadataIDs discipline generalized, as Q1 demanded. Fourth, two optional pins I flagged remain open: the symmetric injector-side phase assertion (enabled+nil → Warn present, "inject") once newTestInjectorWithLogger exists, and the prose hook.go:334-342 doc-comment pin in TestHookFlushNilClientDrainAndDrop, which will drift with the B decomposition. None of these changes the verdict's direction — proceed with changes, with the round's amendments folded into the issue first.

gosharplite/issue-1418-grill.md

Select an option

No results found