Grill Round — gosharplite/tell-me-go issue #1402

Subject: Architect persona | Griller: Griller persona | Topic: Automatic PLUR memory integration (ContextTransformer injection + TurnHook learning) Questions: 9 (round declared complete by griller at Q9)

Opening Statement — Subject (Architect)

[MODEL] Bootstrap complete — I have read the README, Makefile, both domain models, the environment-management model, the Intentional Non-Fixes catalog, and verified the issue's code citations against the tree (context pipeline contracts, skill injector, TurnHook, Turn struct, configRefreshHook, MCP client port, DI factory, repo .plur.yaml). list_skills shows the local skills set. Here is my opening statement.

Opening Statement — Issue #1402: Automatic PLUR memory integration

On the diagnosis. The issue's premise is correct and its citations hold against the tree. tell-me-go exposes MCP servers only as agent-callable tools — tools.MCPClient (internal/domain/tools/mcp_client.go) is a three-method port (ListTools, CallTool, Close) with the SDK confined to internal/infrastructure/mcp/ by verify-mcp-sdk-confinement (ADR-067) — so there is genuinely no push seam for prompt injection and no automatic learning capture. The two seams the issue proposes are exactly the repo's native precedents, both verified: Seam A mirrors agentskills.NewSkillInjector (internal/agent/skills/injector.go), a sessctx.ContextTransformer (contracts.go: Transform + Priority) wired through sessctx.Factory.Extras in agent.initComponents (internal/agent/agent.go, Extras: []sessctx.ContextTransformer{agentskills.NewSkillInjector(...)}), running at Priority() = 10 "after history repair but before token gatekeeping to ensure they are counted." Seam B mirrors configRefreshHook (agent.go:379-393), a TurnHook (engine_types.go:157: BeforeTurn / AfterTurn(turn, err) / OnPhaseTransition) registered per-chat via a.engine.ApplyOptions(orchestrator.WithEngineHook(&configRefreshHook{a: a})) in Chat(), with best-effort error semantics per ADR-029 §5. Both seams consume the memory server through the existing tools.MCPClient port — plur_inject_hybrid / plur_capture / plur_learn are just CallTool invocations, so no new transport code is needed and ADR-067's SDK confinement is untouched.

Seam A plan. plurInjector is constructed at the composition root with the tools.MCPClient for the configured MEMORY.SERVER key — the mcpFactory.Build (internal/infrastructure/di/mcp_factory.go) already returns map[string]plugin.MCPServerDependency{Client: tools.MCPClient, ...} per server key, so the DI layer threads the client into the agent via an AgentOption, and initComponents appends the injector to Factory.Extras. The pipeline-ordering question the issue flags as a risk has a verified answer: the gatekeeper runs at Priority() = 80 (gatekeeper.go), emptyTurnFilter at 90, TransientMerger at priorityTransientThreshold+5 = 105, finalContextValidator at 110 (transformers.go). TransientMerger runs after the gatekeeper — transient content is merged post-budget-check and is not token-counted. Therefore the injector must append to the system message's Parts at a priority below 80 (mirroring the skill injector's priority-10, injectToExistingSystem/prependNewSystemMessage mechanics), not use TransientParts, or context-within-budget silently breaks — this is the load-bearing token-accounting decision and it is satisfiable within the existing pipeline. One deliberate divergence from the skill precedent: the skill injector sets req.PersistHistory = true when it injects; the memory injector should not (or should accept the documented caveat that it may be persisted when another canonical transformer — repair, pruning, summarisation — already set PersistHistory, since executeWithPersistence persists the fully-mutated req.History). Per-turn memory dumps should not pollute history.jsonl; each turn's assembly should re-inject fresh recall. The commenter's sharpest addition is also correct: the injector runs synchronously inside context assembly, so fail-open must include a timeout (reuse TOOL_TIMEOUT/HTTP_TIMEOUT semantics via a bounded context) and a per-session cache/TTL so memory is fetched once per session, not once per turn — the cold npx -y @plur-ai/mcp stdio spawn is a seconds-scale serial I/O hop on the turn's critical path.

Seam B plan. plurHook mirrors configRefreshHook: registered in Chat() via ApplyOptions(WithEngineHook(...)). I verified the AfterTurn timing question the issue raises: ExecuteTurn (engine.go) calls notifyAfterTurn(Turn, err) unconditionally after runPhaseLoop returns, and emergencySave runs inside runPhaseLoop before that return — so the hook fires on error paths too, but must guard for nil State.Response (a terminal error after recovery exhaustion can leave Response nil while LastError is set). The hook has everything it needs: Turn.SessionID, Turn.Mode, Turn.Logger, Turn.State.Response, Turn.State.LastError (engine.go:33). Behavior: always plur_capture an episode with agent: Turn.Mode + session_id (the commenter verified the MCP schema accepts {agent, channel, tags, session_id} — Decision 6 needs zero PLUR-side work), and under LEARN: full gate plur_learn/plur_ingest behind correction-signal detection ("don't/never/always" patterns or explicit feedback), never per-turn LLM extraction as a default. Config surface: a MEMORY: block parsed in internal/domain/config mirroring the MCPServerConfig pattern (mcp_config.go), ENABLED default false (zero behavior change for existing users), and enabled-but-absent-server → warn-and-disable mirroring config-valid-provider. ENABLED/SERVER are session-fixed (registration happens at Chat() start); LEARN/INJECT_BUDGET can ride the existing hot-reload path like other limits — but this needs an explicit decision, because the hook/injector read them at construction.

Decisions I would take now, with evidence. (1) Scope precedence: the repo's own .plur.yaml (scope: "project:tell-me-go") and the commenter's verified flat five-key parser mean MEMORY.SCOPE cannot extend .plur.yaml — it must either override it when set or defer to it; I recommend override-if-set, else .plur.yaml, else the loud "No project scope detected" warning is surfaced once. (2) Concurrency: the commenter's correction is right — it's not "two concurrent sessions", it's all Niffler personas sharing one unlocked ~/.plur/engrams.yaml (shared $HOME), so last-write-wins is a cross-persona hazard, not an edge case; v1 should document a single-writer limitation (one memory-enabled persona per TELL_ME_HOME) and treat per-persona plur_rescope store separation as the v1.1 path, not defer it silently. (3) Budget: pinned engrams bypass the relevance gate, making injection size non-deterministic from the caller's side — the injector must treat the returned ## CONSTRAINTS block as a hard ceiling and trim defensively to INJECT_BUDGET. (4) Domain model + ADR in the same change: Context/Config gain a Memory attribute (or a new Memory entity referencing MCPServer) in docs/domain-model/tell-me-go.modelith.yaml, re-rendered so modelith-check stays green; modelith-drift/modelith-layers are advisory per the Makefile, but the ADR must be indexed (verify-adr-index is a hard gate in check/check-full). The risk list I would add to the issue: the injection-persistence caveat above, double-registration of the hook if Chat() is ever called twice on one engine (WithEngineHook appends), and measuring the real per-turn latency delta of the injection hop before committing to per-turn injection.

Bottom line. The two-seam design is sound, matches the repo's precedents exactly, and every cited seam verifies against the tree. The design is achievable as specced, provided three things are nailed in the ADR: injection lands in Parts below the gatekeeper (token-counted) with timeout+cache (latency-bounded) and no history pollution; AfterTurn handles nil Response on error paths; and the single-writer/lost-update concurrency caveat is an explicit v1 documented limitation with scope-separation as the agreed next step. My plan is to implement exactly that, with the E2E before/after baseline the commenter proposes ("correction taught in one persona session is injected in another") as the acceptance proof — it is partially runnable today via the agent-mediated loop, which makes the delta (removed discipline, not added capability) measurable.

Q1 — Griller

Bootstrap complete. I've verified the subject's citations against the tree — the port, both seams, the pipeline priorities, and the engine lifecycle. Before Q1, my verification log:

Confirmed (subject's claims hold):

tools.MCPClient is a three-method port (ListTools/CallTool/Close), SDK confined to internal/infrastructure/mcp/ (mcp_client.go; Makefile verify-mcp-sdk-confinement)
ContextTransformer = Transform + Priority, ascending; priorityTransientThreshold = 100 (pipeline.go:15); gatekeeper 80, emptyTurnFilter 90, TransientMerger 105, finalContextValidator 110
executeWithPersistence runs canonical (<100) → persist → transient (≥100) (pipeline.go)
Skill injector: Priority 10, injectToExistingSystem/prependNewSystemMessage — and it sets Pinned = true on the system message and PersistHistory = true (injector.go)
TurnHook interface at engine_types.go:157; WithEngineHook appends unconditionally (engine.go:512); Chat() registers configRefreshHook per invocation (agent.go:334) — double-registration hazard real
ExecuteTurn: notifyAfterTurn(Turn, err) unconditional after runPhaseLoop; emergencySave only on err != nil && Phase == PhaseComplete (engine.go)
mcpFactory.Build → map[string]plugin.MCPServerDependency{Client, RequiresConsent, Serial} (mcp_factory.go); .plur.yaml scope confirmed

Refuted — the two findings that matter:

Turn.State.Response is nil on the success path too. PersistenceStep clears it after append (engine_phases.go:102-117, "#1302: clear-after-append"). By the time AfterTurn fires on a normal completed turn, the response text is gone. The subject's claim that the hook "has everything it needs: … Turn.State.Response" is false in the common case — only the error path keeps it. (PreparedHistory survives, and LastError/Mode/SessionID are fine.)
Seam A's "append to the system message's Parts" accumulates without bound. The skill injector pins the system message it injects into; pinned content is excluded from the gatekeeper's summarization candidates and pruning. A per-turn append into a pinned message grows every turn and can never be compressed — and when any sibling canonical transformer (skills, repair, gatekeeper summarization, emptyTurnFilter, contentCleaner) sets PersistHistory, the fully-mutated req.History — including the memory block — is written to history.jsonl and re-enters History on every later turn. "Fresh recall per turn, no history pollution" is unachievable with append mechanics. Also: both injectors at Priority() = 10 are ordered by sort.Slice (non-stable) → nondeterministic.

Q1: You state Seam A will "append to the system message's Parts at a priority below 80 (mirroring the skill injector's … mechanics)" — but the skill injector's mechanics are idempotent-per-session (one injection, guarded by isAlreadyInjected), while memory recall must be fresh per turn. Given the verified mechanics — the system message is pinned by the skill injector (pinned = never pruned, never summarised), and PersistHistory is set by sibling canonical transformers so the fully-mutated history (your injected block included) lands in history.jsonl and is reloaded into History on every subsequent turn — specify exactly how plurInjector avoids unbounded accumulation across turns and how it reconciles "each turn's assembly re-injects fresh recall" with "no history pollution." Specifically: (a) replace-in-place keyed on a marker, or append? (b) what distinct Priority() (both injectors at 10 gives non-stable sort order)? (c) what happens on turn N+1 when the block persisted on turn N is already sitting in History — is it deduped, replaced, or re-injected alongside, and does it get counted twice against the budget?

My recommendation: marker-keyed replace-in-place into the system message Parts at a distinct priority (e.g. 15, after skills) — keeping the gatekeeper's token accounting — and an explicit ADR statement that when sibling transformers set PersistHistory, the latest block may be persisted and must therefore be marker-identified and replaced on the next turn, never appended. The alternative (TransientParts) buys non-persistence but forfeits gatekeeper pressure management — and note your claim that transient content is "not token-counted" is imprecise: finalContextValidator (110, after TransientMerger at 105) does count it and hard-fails with ErrContextLimitExceeded rather than silently breaking. Which mutation semantics do you commit to?

A1 — Subject (Architect)

Both refutations verified — conceding both before answering. (1) PersistenceStep.Process (engine_phases.go: the Response block) calls Turn.CtxManager.AddContent(ctx, Turn.State.Response) then sets Turn.State.Response = nil — "#1302: clear-after-append" — so on the normal success path AfterTurn sees a nil Response; only the error path (AddContent failed, field deliberately not cleared) keeps it. (2) The skill injector pins the system message (injectToExistingSystem: first.Pinned = true) and contiguousUnpinnedSelector.IsCandidate returns !isTurnPinned(turn) (candidate_selector.go) — pinned = never a summarization candidate, and pinningPolicy protects it from pruning. Manager.loadHistory → History.GetWindow(0,-1) reloads the persisted system message every turn, and executeWithPersistence persists the fully-mutated req.History whenever any canonical transformer sets PersistHistory. And I correct my "transient is not token-counted": finalContextValidator (110) runs after TransientMerger (105), estimates over the merged Parts, and hard-fails with ErrContextLimitExceeded — transient content is counted by the final validator (hard failure, no summarization relief), it is simply invisible to the gatekeeper's pressure management at 80.

Committed semantics for plurInjector — (a) marker-keyed replace-in-place, never append.

const memoryMarker = "\n\n## PLUR MEMORY\n"        // start sentinel
const memoryEndMarker = "\n[/PLUR-MEMORY]\n"       // end sentinel

Transform(ctx, req):
  1. if memory disabled or len(req.History)==0 → return nil
  2. fresh, err := client.CallTool("plur_inject_hybrid",
       {task: <last user prompt>, budget: INJECT_BUDGET, scope?, session_id?})
     with a bounded context (TOOL_TIMEOUT semantics) and a per-session cache/TTL;
     on err → log, return nil (fail-open)
  3. block := trimToBudget(memoryMarker + fresh + memoryEndMarker)   // defensive ceiling
  4. sys := req.History[0]
     if sys.Role != "system":
         prepend new &llm.Content{Role:"system", Pinned:true, Parts:[{Text:block}]}
         // Pinned=true ONLY to protect the system prompt from HistoryPruner;
         // do NOT set req.PersistHistory
         return nil
  5. for i, p := range sys.Parts:
         if strings.Contains(p.Text, memoryMarker):
             sys.Parts[i].Text = block          // REPLACE in place — single part
             return nil
  6. sys.Parts = append(sys.Parts, &llm.Part{Text: block})   // first injection of session only
  7. NEVER set req.PersistHistory; NEVER touch Pinned on an existing message

Why this satisfies both requirements: fresh per turn — step 5 replaces the previous block with today's recall every turn; bounded — at most one memory part exists in the system message at any time; no unbounded accumulation — even when a sibling canonical transformer (repair, gatekeeper summarization, skills' first injection, cleaners) sets PersistHistory and writes the mutated history to the store, what persists is the single latest block, and on turn N+1's reload step 5 replaces it again. The persisted block is marker-identified and replaced, never appended to, exactly as you recommend — this must be an explicit ADR statement, because the alternative (silently relying on "no sibling sets PersistHistory") is not a guarantee: the skill injector sets it on its first injection, and the gatekeeper sets it on any summarization (gatekeeper.go applySummarizationResult).

(b) Distinct priority — commit to 15. Both injectors at 10 under sort.Slice (pipeline.go — non-stable) is nondeterministic ordering, which matters for test determinism (ADR-036) and for stable history bytes. Skills stay at 10 (unchanged precedent); plurInjector at 15: after skills, before gatekeeper (80) — so the block is present in req.History when the gatekeeper estimates, gets counted, and can trigger summarization of unpinned history to make room (the whole point of injecting below 80). For completeness I'll correct my opening's implied ordering: HistoryPruner is at 110 (pruner.go:137), i.e. in the transient group after persistence — so the actual order is repair(0) → cleaners(3-6) → skills(10) → plur(15) → gatekeeper(80) → emptyTurnFilter(90) → [persist] → TransientMerger(105) → HistoryPruner(110)/finalContextValidator(110). Priority 15 also means the injector runs before the pruner, but that's immaterial — the system message is pinned either way.

(c) Turn N+1 with a persisted block. The loaded req.History contains the system message with [mem-old]. Step 5 replaces it with [mem-fresh] before the gatekeeper (15 < 80) — so the gatekeeper and finalContextValidator never see both blocks. The old block is replaced, not re-injected alongside, and never double-counted: the budget sees exactly one memory block, the latest, sized ≤ INJECT_BUDGET (with pinned-engram-driven server-side oversize trimmed defensively at step 3). If a sibling sets PersistHistory this turn, the store's system message is updated to [mem-fresh]; if not, the store keeps [mem-old] and it is replaced again next turn. Either way: at most one memory block in history, in-memory or on disk, from the first injection onward. The only residue is if memory is disabled mid-session: the injector leaves the pipeline and the last persisted block sits inert in the system message — harmless text, worth one line in the ADR.

On the TransientParts alternative — rejected, and your correction is the reason: it buys non-persistence but forfeits gatekeeper pressure management (no summarization can make room for it) and converts a budget overflow from "summarize unpinned history" into a hard ErrContextLimitExceeded from finalContextValidator. Parts + replace-in-place is the only option that keeps the context-within-budget invariant graceful.

Seam B correction (same verification discipline): since Turn.State.Response is nil on the success path, the hook's response source must be: if Turn.State.Response != nil { use it } (error path — PersistenceStep deliberately left it un-cleared for retry) else { resp, _ := Turn.CtxManager.History.GetLastModelTurn(ctx) } (success path — the response was just appended by PersistenceStep before AfterTurn fires; GetLastModelTurn is on the ports.HistoryManager interface, cf. the mock at agenttest/mock_history_manager.go). One bounded history read per turn on the success path; episode content = response text + Turn.Mode + Turn.SessionID + timestamp. I commit to that sourcing and to updating the issue's Seam B text accordingly.

Q2 — Griller

Verification of A1's new claims: GetLastModelTurn(ctx) confirmed on ports.HistoryManager (history.go:87-90); HistoryPruner.Priority() = 110 confirmed (pruner.go:137) — the subject's corrected ordering table holds; contiguousUnpinnedSelector.IsCandidate = !isTurnPinned(turn) confirmed (candidate_selector.go); pinningPolicy keeps pinned turns (pruner.go:234-263). The committed replace-in-place at priority 15 is sound.

One hole in the committed pseudocode remains, and it contradicts the invariant the subject just committed to.

Q2: Your step 2 commits: on err → log, return nil (fail-open) — and your disable-mid-session note calls the leftover persisted block "harmless text, worth one line in the ADR." But with replace-in-place mechanics, return nil without replacing means the previous turn's (or previous session's) memory block stays in the system message — and since the system message is pinned, it is re-sent every turn, forever, exactly as-is. So on any MCP failure (stdio server died, timeout, network) or mid-session disable, the agent runs with stale injected memory: a snapshot of the store as of the last successful inject — which can contain retracted corrections (plur_forget/supersession exist), contradictory guidance, or a scope that no longer matches. That violates the invariant you just committed to ("fresh per turn, at most one block, the latest"), because the failure path serves yesterday's block, not the latest — and it's not budget-harmless either, since the pinned block is re-counted against context-within-budget on every turn with no path to removal.

Specify the failure/disable semantics precisely: does fail-open mean (a) proceed with no memory this turn (strip the marker block if present, no MCP call — cheap, deterministic, keeps the pipeline idempotent), or (b) proceed with the last successful block? And what is the removal path for a persisted block once memory is disabled mid-session — the injector stops running, so nothing will ever strip it; is that block's permanent residence in history.jsonl acceptable?

My recommendation: strip-on-failure, strip-on-disable — distinguish "memory unavailable" from "memory available" in the transformer: on MCP error, timeout, or disabled config, still run the marker-keyed removal (step 5 logic inverted: delete the block instead of replacing), so the invariant becomes "inject current recall, or nothing — never stale recall." Fail-open should mean memory must never break a turn, not memory must never change a turn. And the disable path needs an explicit one-shot strip (or documented acceptance that the stale block is permanent), not "one line in the ADR." Do you commit to strip semantics for both paths?

A2 — Subject (Architect)

Conceded — the hole is real and it breaks the invariant I committed to. return nil on failure leaves the previous block inside the pinned system message; that message survives pruning (pinningPolicy, pruner.go) and summarization exclusion (contiguousUnpinnedSelector), so the stale block is re-sent verbatim every turn and re-counted against context-within-budget with no removal path. Worse than budget: plur_inject_hybrid is relevance-gated per task, so the leftover block is yesterday's recall for yesterday's task — it can carry retracted corrections, superseded engrams, or a scope that no longer matches. The failure path as pseudocoded served stale memory by construction.

Commitment: strip-on-failure AND strip-on-disable. Fail-open means "memory must never break a turn" — it does not mean "memory must never change a turn." The transformer's invariant becomes: inject current recall, or nothing — never stale recall.

Failure path (MCP error / timeout, still enabled). Run the marker-keyed removal: scan the system message's Parts for the ## PLUR MEMORY marker; if present, delete that part (and if the system message is left with zero parts, remove the message from req.History); then return nil. Do NOT set req.PersistHistory. Consequences: the agent proceeds with no memory this turn — deterministic, idempotent, and the block is removed before the gatekeeper (priority 15 < 80), so it is not counted against the budget on the failure turn. The store may retain the stale block, but it never reaches the agent — it is stripped from the payload on every subsequent failure turn, and the moment the server recovers, replace-in-place overwrites it with fresh recall (and if a sibling sets PersistHistory that turn, the store copy is updated as a side effect). Self-healing, no one-shot bookkeeping needed.

Disable path (ENABLED flips false mid-session). This is the path that requires store removal, and it forces a revision of my opening claim that ENABLED is session-fixed. If the injector simply stops running, nothing strips the persisted block and it is loaded into every later payload forever — the exact permanent-stale case you flag. So: ENABLED must be hot-reloadable and read per-Transform. The injector holds a func() bool (or the agent's atomic memory-config) evaluated at the top of Transform; on the first Transform after the flip it performs the one-shot strip with req.PersistHistory = true — the removal must reach the store, because subsequent turns are no-ops and the block would otherwise return from GetWindow(0,-1) on every later load. After the strip turn: no-op while disabled. If re-enabled, injection resumes (first injection appends a fresh block — at most one block again). Mechanism: extend the existing hot-reload path — configRefreshHook.OnPhaseTransition → applyConfig already re-reads limits via the watcher on the Inference→Execution transition; the watcher's refresh set gains MEMORY.ENABLED, stored on the agent, read by the injector each Transform. This is a modest extension of the ADR-029 refresh chain, not a new mechanism. SERVER stays session-fixed — the tools.MCPClient is bound at the composition root (mcp_factory.go) and cannot be swapped mid-session without rebuilding the client; that stays a restart-level change, consistent with provider identity not being hot-swappable (domain model, config hot-reload scenario).

On "one line in the ADR" — withdrawn. The disable path is a designed behavior with a defined mechanism (one-shot persisted strip), not an accepted residue. The ADR states the full failure/disable matrix: available → inject latest (replace-in-place, ≤ INJECT_BUDGET, counted pre-gatekeeper); unavailable (error/timeout) → strip, no persist, no memory this turn; disabled → one-shot strip with persist, then no-op. The only documented residue left is the transient-window case (server down, store retains the stale block until recovery) — invisible to the agent by construction and self-cleaning.

On (a) vs (b): (a), without reservation. Last-successful-block (b) fails the freshness invariant on exactly the turns where it matters — after a plur_forget/supersession, the last successful inject may be the retracted guidance. The accepted cost of (a) is that a transient one-turn outage loses memory for that turn; that is the correct trade — memory is an enhancement, and the E2E acceptance criterion ("correction taught in one persona is injected in another") tolerates per-turn unavailability, while it would not tolerate silently serving a retracted correction. The one honest caveat I'll carry into the ADR: strip-on-failure means the injection guarantee is "best-effort fresh, else absent" — never "best-effort fresh, else last-known," which is the semantics that turns PLUR into an oracle rather than an advisor.

Q3 — Griller

A2's mechanics check out: strip-on-failure at priority 15 removes the block before the gatekeeper on the failure turn; the one-shot strip with req.PersistHistory = true runs in the canonical group (15 < 100) so executeWithPersistence persists the removal — self-cleaning across sessions; configRefreshHook.OnPhaseTransition → applyConfig re-reads via the watcher (verified agent.go) so hot-reloadable ENABLED is a plausible ADR-029 extension. One minor false premise: "provider identity not being hot-swappable" — applyConfig → reconfigureEngine → Engine.Reconfigure (agent.go) swaps ProviderName/Model mid-session by design (that's exactly why configRefreshHook exists); the real constraint is the MCP client lifecycle (mcp_factory.Build/Close at DI), not provider identity. Not load-bearing.

Now the sharpest remaining uncorrected flaw — and it's in the subject's own Seam B fix from A1:

Q3: Your A1 Seam B sourcing is: if Turn.State.Response != nil { use it } else { resp, _ := Turn.CtxManager.History.GetLastModelTurn(ctx) }. That two-way branch conflates two distinct nil-Response cases, and one of them misattributes content into the memory store:

Case 1 — success: Response was cleared by PersistenceStep's clear-after-append (engine_phases.go); GetLastModelTurn returns this turn's just-appended response. Correct.
Case 2 — persistence failure: AddContent failed, Response deliberately not cleared (engine_phases.go "#1302"); Response non-nil → the if branch. Correct.
Case 3 — turn failed before PersistenceStep (e.g., Inference LLM error, retries exhausted → RecoveryStep.handleFailure returns PhaseComplete + err without ever reaching Persisting; emergencySave no-ops because Response and ToolResponse are both nil): Response is nil and nothing was appended this turn — so GetLastModelTurn returns the previous turn's model response, which your else branch would capture as this turn's "episode summary" and write into the shared ~/.plur/engrams.yaml, where it later gets recalled and injected as if it were this turn's learning. Garbage in the memory store — the one place the product can't tolerate it, since the store is the product.

LastError is reliably set in exactly this case (executePhase sets Turn.State.LastError = err whenever a processor returns an error; the success and persistence-failure paths are distinguishable by Response != nil). Specify the full three-way classification: when Response is nil and LastError is non-nil, what does the episode contain — and is GetLastModelTurn ever a valid source on an error turn?

My recommendation: branch on the error first — (1) Response != nil → episode = response text (persistence-failure retry case); (2) Response == nil && LastError != nil → episode = {error: LastError, prompt: <last user message from PreparedHistory>, mode, session, timestamp} — never GetLastModelTurn, which would capture the prior turn's content; (3) Response == nil && LastError == nil → GetLastModelTurn (the only case it's valid). This makes the episode store trustworthy on error turns, which matters twice over given strip-on-failure makes the injection path honest — the learning path must be too. Do you commit to the three-way classification, and to never sourcing GetLastModelTurn when LastError is set?

A3 — Subject (Architect)

The classification is right in substance, with one refinement that actually makes it airtight — and the refinement comes from a path you didn't list. Verified against the tree first: executePhase sets Turn.State.LastError = err whenever a processor errors (engine.go); RecoveryStep.handleFailure returns {NextPhase: PhaseComplete}, err without reaching Persisting (engine_phases.go), and emergencySave no-ops when both Response and ToolResponse are nil (engine.go) — so Case 3 is real: AfterTurn fires with err != nil, Response == nil, and nothing appended this turn. GetLastModelTurn would return the previous turn's model response. Misattribution confirmed.

The refinement: branch on the AfterTurn err parameter, not on Turn.State.LastError. LastError is phase-level and is not cleared on a path you didn't list: the empty-response retry saga. RecoveryStep.Process on errEmptyResponse with retries exhausted returns {NextPhase: PhasePersisting}, nil (engine_phases.go) — it never clears LastError, which still holds errEmptyResponse from the failed Inference. Persisting then appends the empty response, clears it, and the turn completes normally: AfterTurn(turn, err=nil), Response == nil, but LastError != nil. Under your predicate (Response == nil && LastError != nil), that successful turn would be misclassified as an error episode — the same misattribution class you're hunting, just on the other side. The hook's err argument is the authoritative turn-level outcome (set by runPhaseLoop/ExecuteTurn); LastError is only ever episode content, never the branch predicate.

Committed — the three-way classification, keyed on the hook's err:

Response != nil (any err) → episode = this turn's response text + Mode + SessionID + timestamp; if LastError != nil, annotate it. Response is turn-scoped by construction — CreateTurn zeroes TurnState each iteration and only this turn's InferenceStep sets State.Response (per-turn Turn lifecycle, ADR-061/#1327) — so this branch is safe even on the stop-without-persist edge (processor returns Stop before Persisting with a rendered-but-unappended response; the content is still this turn's).
Response == nil && err != nil → episode = {error: LastError (fallback err), prompt: <last user message from PreparedHistory when non-nil — ContextRefiner set it this turn and the trailing user message is the current prompt by pipeline construction (emptyTurnFilter "always keep a trailing single message") — else the history manager's backward user-message scan, the accessor the --retry flow uses per the domain-model "Retry last turn" scenario, else omitted>, mode, session, timestamp}. Never GetLastModelTurn. No plur_learn/plur_ingest from this branch — an error alone is not a correction signal; the existing gate already governs learning, and this branch only feeds plur_capture's incident timeline.
Response == nil && err == nil → GetLastModelTurn — the only case it is ever valid: the just-appended response is provably this turn's because nothing can append between PersistenceStep's AddContent and notifyAfterTurn (the phase loop has exited; the ExecuteTurn tail is finalizeTurnTrace + SafePublish(TraceEvent), no history writes). Add one guard: if the retrieved response has no text parts (the empty-response success path lands here), skip the episode — writing "empty response" into the shared store is noise, and it would later be recalled as if it were a real turn.

Commitments, flat: (1) three-way classification as above; (2) GetLastModelTurn is sourced only when Response == nil && err == nil — never when the turn errored; (3) the branch predicate is the hook's err, never Turn.State.LastError, because of the verified stale-errEmptyResponse path. And I accept the Q3 premise wholesale: with strip-on-failure making the injection path honest ("current recall, or nothing"), the learning path must carry the same standard — the episode store is the product, and a prior turn's response laundered into this turn's episode is exactly the corruption that later gets injected back. One episode-worthy side note for the ADR: the empty-response success path shows the turn lifecycle does not guarantee "Response present ⇔ turn produced content" — the hook's contract must be written against err + Response, not LastError.

Q4 — Griller

Verified: sessionManager.Run creates the agent via AgentFactory and calls Chat once per session execution (session_manager.go:172) — but the agent's own tests call a.Chat multiple times on one agent (agent_chat_test.go:64,102; agent_lifecycle_test.go:412,431), so the codebase's contract allows repeated Chats per agent → the per-Chat WithEngineHook append would double-fire a plurHook (AfterTurn twice per turn = duplicate episodes). I'm holding that as a live issue; the more urgent open decision is the one you left unspecified in A1.

Q4: A1 step 2 committed to "a per-session cache/TTL" without resolving the tradeoff — and it now sits in direct tension with your Q2 commitment ("inject current recall, or nothing — never stale recall"). A cache is, by definition, bounded-stale recall: if it's fetch-once-per-session, then on turn 300 the agent is injected the store's state from session start — a correction taught to the butler at turn 3 (by you, via your own Seam B AfterTurn, or by a concurrent persona in the shared Niffler store) is invisible to injection until the next session. That breaks the feature's core value proposition within the very session where the learning happens: "teach at turn 3, rely on it at turn 5" fails. The commenter's latency evidence (cold npx stdio spawn is seconds) justifies caching the cold start, but a warm stdio roundtrip is fast — so the cache's purpose is narrower than "once per session, never per turn."

Specify the semantics: (a) what is the freshness strategy — fetch-once-per-session, TTL-bounded (what TTL, and is it configurable), or fetch-per-turn with only the cold-spawn cost amortized? (b) does a successful Seam B capture/learn invalidate the injector's cache so the next turn re-fetches — i.e., is there a plurHook → plurInjector invalidation coupling, and if not, how does a mid-session learning ever reach the prompt? (c) how is cached recall consistent with "never stale recall" — is the ADR contract "fresh within TTL / since last write, never indefinite," explicitly distinct from the unbounded failure-path staleness you rejected in Q2?

My recommendation: TTL-bounded cache (e.g. MEMORY.CACHE_TTL, default ~60s, hot-reloadable like LEARN/INJECT_BUDGET) plus explicit invalidation — plurHook.AfterTurn busts the injector's cache after any successful capture/learn, so a same-session learning is injectable at the very next turn and the cache becomes a pure latency optimization for the no-new-learning steady state. The ADR contract: "recall is fresh within TTL or since the last memory write — never indefinite," which keeps the Q2 line intact (unbounded staleness rejected; bounded staleness is an explicit, configurable tradeoff). Do you commit to TTL + write-invalidation, and to the bounded-freshness contract?

A4 — Subject (Architect)

Two commitments, one of them a pushback grounded in the evidence you yourself cited.

First, the live double-fire issue — committing to the fix, not just acknowledging it. WithEngineHook appends (engine.go:512), notifyAfterTurn iterates all hooks (engine.go), and repeated Chat per agent is real contract (agent_chat_test.go:64,102; agent_lifecycle_test.go:412,431) — so per-Chat registration double-fires, and for plurHook that's duplicate episodes in the shared store, which Q3 established is the one place corruption can't be tolerated (it gets injected back later). The production path happens to be one-Chat-per-agent (sessionManager.Run → AgentFactory), which is why the hazard is latent — but the codebase contract is the tests, so I design against it. Fix: register plurHook once at agent construction, not per-Chat. initComponents builds the engine and already has the option surface — a WithMemoryClient/WithMemoryHook AgentOption threads the client in, and orchestrator.NewEngine(..., WithEngineHook(plurHook)) registers a single instance there. The hook needs no per-Chat state: its enabled flag is read per-Transform (Q2's hot-reloadable accessor), its client is session-bound at DI, and SessionID arrives on the Turn. configRefreshHook stays per-Chat (benign — documented no-op BeforeTurn/AfterTurn, INTENTIONAL_NON_FIXES), but plurHook is engine-scoped. Defensive belt-and-suspenders: a turn-scoped dedupe in the hook (record last SessionID+Index, skip identical) makes even a future re-registration harmless — with the documented caveat that a rollback+retry replaying the same (session, index) would be skipped, which is acceptable since the episode was already captured on the first pass.

Now the cache — and here I push back on the premise. Your own evidence dissolves the tension: warm stdio roundtrips are fast; the seconds-scale cost is the cold npx spawn. And the spawn is amortized by the MCP client lifecycle, not by a response cache — the StdioClient is constructed once at DI and kept alive across turns (mcp_factory.go; Close at teardown), so the cold spawn lands on turn 1 and every later inject_hybrid is a warm roundtrip. A response cache would therefore buy only the avoidance of a fast warm call — while introducing a real correctness cost you haven't priced: plur_inject_hybrid is relevance-gated per task ({task, budget, scope?, session_id?}). A session-wide TTL cache serving one block for all tasks serves task-mismatched recall — the Q2 stale-recall problem in miniature, bounded by TTL instead of unbounded. A task-keyed cache rarely hits (consecutive turns rarely have identical prompt strings), making it a mostly-dead code path. So:

(a) Freshness strategy — fetch-per-turn, no response cache in v1. Every turn's Transform calls plur_inject_hybrid with the current task + INJECT_BUDGET; the cold-spawn cost is amortized by the persistent client (one spawn per session). The staleness bound is the warm roundtrip itself — a few hundred ms — and the response is always the live, task-relevant query result. This is strictly stronger than TTL-bounded freshness.

(b) Invalidation coupling — none needed in v1, by construction. Seam B's plur_capture/plur_learn are synchronous MCP calls inside AfterTurn; the store write is durable before the next turn's Transform runs (the phase loop has exited, the next Prepare/injection happens on the following turn). So a learning at turn 3 is injectable at turn 5 with no plurHook→plurInjector coupling at all — the coupling only exists to invalidate a cache, and there is no cache. "Teach at turn 3, rely on turn 5" holds within the same session, and cross-persona (butler learns, coder's next turn fetches) holds by the same mechanism since all personas share the store.

(c) Contract — committing to bounded-freshness, and noting v1 exceeds it. The ADR contract for v1: "recall is the live per-turn relevance-gated query result; staleness is bounded only by the query itself, never by a cache." That is the Q2 line held absolutely — no staleness window at all, which is why strip-on-failure (Q2) and fetch-per-turn are the same invariant from two directions. If the E2E in the Niffler env measures the warm roundtrip above a budget (I'd set the trigger at p95 > ~100–150ms — if the embedder's lazy model load proves to be per-query rather than per-process, that's exactly when it fires), I commit to your design as the documented fallback: MEMORY.CACHE_TTL (default 60s, hot-reloadable alongside LEARN/INJECT_BUDGET), keyed by task-fingerprint + session_id, plus write-invalidation — plurHook.AfterTurn calls recallCache.Invalidate() after any successful capture/learn, via a shared cache value injected into both hook and injector at the composition root. The fallback contract is exactly yours: "fresh within TTL or since the last memory write — never indefinite."

So: I commit to the bounded-freshness contract (c) unconditionally, and to write-invalidation as the coupling if and only if a cache exists (fallback). What I decline to commit to is a v1 response cache, because the latency evidence says the client lifecycle already solves the cold-start problem, and a per-turn fresh query is the only strategy with zero staleness window — which is what "install once, memory works" actually promises.

Q5 — Griller

Verification of A4's load-bearing claims — and here the subject's pushback refutes my Q4 premise on the code:

Eager spawn confirmed: stdio_client.go's own doc comment states "The child is spawned eagerly in NewStdioClient; the MCP handshake runs inside the constructor bounded by the server's EffectiveTimeout," and mcpFactory.Build constructs clients at DI (mcp_factory.go). The cold npx spawn happens at process startup, off the turn's critical path — turn 1's injection is a warm roundtrip. A4's "amortized by the client lifecycle" holds; fetch-per-turn with no v1 cache is the right call, and the bounded-freshness contract stands. (One micro-correction: the INTENTIONAL_NON_FIXES entry at line 844 catalogs the no-op BeforeTurn/AfterTurn stubs, not the per-Chat double-registration — the "benign" judgment is right, the citation is loose. Not load-bearing.)

That clears the seam mechanics. The remaining contradiction is in the subject's own commitments, and it's between two things the subject has explicitly signed:

Q5: The opening statement commits to both (a) "v1 should document a single-writer limitation (one memory-enabled persona per TELE_ME_HOME)" and (b) the commenter's E2E before/after baseline — "correction taught in one persona session is injected in another" — as the acceptance proof. But the E2E criterion (issue acceptance checklist: "enable in a Niffler env, verify a correction taught in one persona session is injected in another") requires at least two memory-enabled personas simultaneously: the teacher (butler, with LEARN/capture active so its AfterTurn writes) and the injector (coder, with ENABLED + plurInjector active so its next-turn Transform reads). Under your single-writer limitation, that test is untestable by construction — the exact acceptance criterion you committed to as the proof. And the commenter's own data says this isn't an edge case: all Niffler personas share one unlocked ~/.plur/engrams.yaml (shared $HOME), so multi-writer is the feature's normal operating mode, not a concurrent-session anomaly — the lost-update risk is the price of the cross-persona value proposition the issue exists to deliver.

Reconcile the two: is v1 multi-persona memory (which means accepting last-write-wins as a documented limitation, or adding a cheap cross-process write serialization such as an advisory flock around the plur_capture/plur_learn calls, or per-persona scope separation in v1 — each with its own injection implications), or is it single-writer (which forces you to rewrite the acceptance criterion)? And note the tmg deployment's own rule is "sequential only — never parallel tell-me-go calls," so the teacher-then-injector E2E is naturally serialized; the lost-update window only opens on genuinely concurrent writers (a human in two terminals, or a future parallel orchestrator) — is "accepted + mitigated by sequential orchestration + advisory lock as cheap hardening" the honest v1 stance, with "one memory-enabled persona" withdrawn?

My recommendation: multi-persona is the feature — withdraw "one memory-enabled persona per TELL_ME_HOME"; commit v1 to documented last-write-wins acceptance, mitigated by (i) the deployment's existing sequential orchestration and (ii) a best-effort cross-process advisory lock around the write path (fail-open if unavailable); keep per-persona scope separation as v1.1 with the injection-semantics implications (single-scope plur_inject_hybrid means cross-scope injection needs one call per scope) spelled out in the ADR rather than deferred silently. Do you withdraw the single-writer commitment and adopt multi-persona-with-mitigations as the v1 operating model?

A5 — Subject (Architect)

The contradiction is real and I withdraw the single-writer commitment — but let me sharpen why it was wrong before adopting the mitigation, because the correction is more precise than "the test needs two personas."

Why single-writer was wrong twice over. First, it conflated the write-side risk with the read-side feature. The lost-update hazard lives entirely on the write path (shared unlocked ~/.plur/engrams.yaml, atomic+fsync, last-write-wins — the commenter's verified data). Reads (plur_inject_hybrid) are race-free; a reader persona is not a writer. My opening's "one memory-enabled persona per TELL_ME_HOME" blurred ENABLED (injector active) with writer, which is precisely how the E2E — butler teaches, coder injects, one writer + one reader, serially — became "untestable by construction." Second, and more fundamentally: the issue's motivation is multi-persona memory ("a correction made to one persona session is invisible to the others"). Restricting v1 to one writing persona per HOME guts the feature for the Niffler deployment the issue exists to serve. Withdrawn, unconditionally.

Committed v1 operating model: multi-persona with mitigations, exactly as you recommend.

N memory-enabled personas per TELL_ME_HOME, all able to read (inject, fetch-per-turn per Q4) and write (capture/learn in Seam B). The E2E acceptance criterion stands as written — no rewrite needed: butler (ENABLED + LEARN active, AfterTurn writes) → coder (ENABLED, next-turn Transform reads). It's one writer + one reader, serialized per the deployment rule, and with fetch-per-turn (Q4) the turn-3 learning is injectable at turn 5 with no cache-invalidation coupling. It was only untestable under the wrong model.
Documented acceptance: last-write-wins on the engram store for genuinely concurrent writers — a human in two terminals, or a future parallel orchestrator. This is a documented limitation with a defined remedy, not a silent deferral: the ADR states the residual window explicitly, names the v1.1 path (per-persona scope separation), and records the injection-semantics implication of that path — plur_inject_hybrid takes a single scope, so cross-scope injection requires one call per scope per turn (N roundtrips), or a PLUR-side multi-scope call that does not exist yet. That trade is exactly why scope separation is v1.1, not v1.
Mitigation (i) — the deployment's sequential orchestration, codified. tmg-chat-ingroup rule 5 ("Sequential only — never parallel tell-me-go calls") is already the Niffler operating discipline; the ADR elevates it to the standard operating constraint for memory-enabled deployments. Within that discipline, the concurrent-writer window does not open.
Mitigation (ii) — best-effort advisory flock on the write path. Seam B's AfterTurn write calls (plur_capture, plur_learn, plur_ingest, plur_learn_batch) acquire an advisory exclusive lock on a lock file alongside the store (e.g., ~/.plur/.tmg-write.lock) for the duration of the MCP call — acquired before CallTool, released after the server's write+fsync completes — so tell-me-go writers serialize and each sees the prior write. Fail-open semantics: bounded acquisition timeout (the lock is held across a roundtrip; a stuck holder must not wedge the turn — on timeout, log and proceed unlocked); lock file uncreatable (read-only HOME) → proceed unlocked. Injection (read) never takes the lock. Two honest caveats in the ADR: syscall.Flock is Unix-only, so Windows runs in documented unlocked mode with the sequential rule as the backstop; and the lock serializes only tell-me-go writers — a raw PLUR CLI or another tool writing the same store does not honor it, which is why this is "best-effort hardening" over an "accepted" residual risk, not a solved problem.

On the residual-risk framing — yes, "accepted + mitigated by sequential orchestration + advisory lock as cheap hardening" is the honest v1 stance, and I'd phrase the ADR line exactly that way: the cross-persona value proposition is the product, last-write-wins is the accepted price of a shared unlocked store in the genuinely-concurrent edge, and the lock plus the existing serialization rule shrink that edge to near-zero in the standard deployment without pretending it's gone. And the micro-correction: accepted — the INTENTIONAL_NON_FIXES entry at that line catalogs the no-op configRefreshHook BeforeTurn/AfterTurn stubs, not the per-Chat registration; the "benign" judgment for configRefreshHook stands on the stub catalog, and the double-fire fix (registration-at-init, Q4) is what makes plurHook's case different. Not load-bearing, noted.

Q6 — Griller

A5 is a clean resolution — the reader/writer distinction (ENABLED = reader, AfterTurn = writer) dissolves the E2E contradiction without weakening the feature, and the flock mitigation is honestly scoped (Unix-only, tell-me-go-writers-only, fail-open). The v1 operating model is now coherent.

One acceptance-level gap remains, and it's at the exact point the feature's proof lives:

Q6: The acceptance criterion — "a correction taught in one persona session is injected in another" — is only deterministically satisfiable if the taught engram actually surfaces in the coder's next-turn plur_inject_hybrid response. But the design has two facts working against that: (1) plur_inject_hybrid is relevance-gated per task — a non-pinned engram is injected only when the task matches its relevance; and (2) the Seam B automatic path (plur_capture always, plur_learn/plur_ingest gated) produces non-pinned engrams — the commenter's own evidence is that only pinned engrams bypass the relevance gate "and are injected every session regardless of task." Meanwhile, the MCP surface the issue lists (plur_learn, plur_recall, plur_inject_hybrid, plur_feedback, plur_forget, plur_status, plur_capture, plur_timeline, plur_ingest, plur_sync, plur_receipt, plur_outbox, plur_rescope, plur_session_scope, plur_learn_batch) contains no pin tool — so tell-me-go cannot pin an engram through its own seams, and the commenter's "teach a pinned engram" E2E setup step requires non-MCP access (PLUR CLI or store edit). Consequence: the acceptance test as written is nondeterministic — teach a correction via Seam B in butler, start coder on an unrelated task, and the correction may not inject; you can't pin it through tell-me-go; and prompting coder with a topically-matched task edges toward circularity (you can't state the correction in the prompt and claim it was memory that injected it).

Specify the E2E's injection proof precisely: how does the test establish that the automatic (Seam B, non-pinned) learning — not a hand-pinned engram and not the prompt itself — was injected into the coder's context, deterministically? And does the acceptance criterion need a pin mechanism in the MCP surface, a documented out-of-band pinning setup step, or a re-scoped criterion (e.g., verify via plur_recall/plur_timeline that the teaching persisted, plus a relevance-aligned injection assertion checking injected_ids)?

My recommendation: split the E2E into two assertions — (i) plumbing: a pinned engram (pinned out-of-band, documented as a setup step, since the MCP surface has no pin tool) injected into a fresh persona's context, asserting injected_ids contains it; and (ii) automatic-learning: after a Seam B gated learn in butler, prompt coder with a task whose description is semantically adjacent but not self-revealing (e.g., teach "always run gofmt before committing" via a formatting-related correction; prompt coder with "review this Go file's formatting"), asserting injected_ids contains the taught engram — and accept this leg as best-effort/relevance-dependent, with plur_recall-based persistence as the hard gate. Do you commit to a two-legged E2E with the pin setup step documented, or do you re-scope the criterion?

A6 — Subject (Architect)

The nondeterminism is real, and it's structural — not a test-harness deficiency. Verified against the material: plur_inject_hybrid is relevance-gated per task; the commenter's own evidence is that only pinned: true engrams bypass the gate and inject "every session regardless of task"; Seam B's automatic path (plur_capture always, gated plur_learn/plur_ingest) produces non-pinned engrams; and the MCP surface as listed (plur_learn, plur_recall, plur_inject_hybrid, plur_feedback, plur_forget, plur_status, plur_capture, plur_timeline, plur_ingest, plur_sync, plur_receipt, plur_outbox, plur_rescope, plur_session_scope, plur_learn_batch) contains no pin tool — so a correction taught via Seam B has no deterministic injection guarantee, and the issue's out-of-scope line ("PLUR's MCP surface is consumed as-is") forecloses adding one. The criterion as written is untestable as a single deterministic assertion. I commit to the two-legged E2E, with two refinements that make the hard legs actually hard.

Leg (i) — plumbing (hard gate, deterministic). Out-of-band pinned engram, documented as a setup step (PLUR CLI or direct store edit — the only pin path available, since the surface has no pin tool; this is exactly the mechanism the commenter's own baseline used). Fresh persona (coder) on an unrelated task. Assertion: the injector's injected_ids for that turn's plur_inject_hybrid contains the pinned engram's ID. Determinism rests on the commenter's verified pinned-bypass behavior, and the assertion targets injected_ids — not the rendered block text — so it survives the injector's budget-trimming/rewrap. The injector must therefore emit injected_ids observably per turn (a debug log line, or a ContextMetadata.Warnings entry — the metadata is already surfaced in the -t trace and is the pipeline's sanctioned diagnostics channel; consumers may append to Warnings), which the E2E asserts on.

Leg (ii) — automatic learning (hard gate + best-effort, explicitly split). Butler's Seam B gated learn produces a non-pinned engram. Hard gate — persistence through the real retrieval index: plur_recall with a semantically-adjacent query returns the taught engram. This is the deterministically testable half: it proves the AfterTurn write path persisted a queryable, task-relevant learning end-to-end, without depending on injection policy. Best-effort — injection: prompt coder with the semantically-adjacent-but-not-self-revealing task (e.g., teach "always run gofmt before committing" via a formatting correction; prompt "review this Go file's formatting" — the prompt must not state the correction, so the engram is the only source of its content in context), assert injected_ids contains the taught engram. This leg is relevance-dependent by construction (threshold behavior, budget, top-N policy are server-side and not under tell-me-go's control), so it gets one documented re-run allowance (relevance nondeterminism) and is reported as best-effort — never a gate failure — while the recall-based persistence gate must pass unconditionally. The injection content assertion is on the ## CONSTRAINTS block's [ENG-ID] line appearing in coder's assembled context (via the same observable injected_ids), never on coder's model response — asserting model compliance would test the LLM, not the plumbing.

On the re-scoped criterion: yes — the acceptance checklist item is reworded in the ADR/plan to the two-legged form: (i) "a pinned engram, pinned out-of-band per the documented setup step, is injected into a fresh persona's context (assert injected_ids)" — hard; (ii) "a Seam B automatic learning persists and is task-relevant per plur_recall — hard; and injects under a semantically-adjacent task — best-effort." The original single sentence stays as the goal statement; the checklist gets the legs.

On a pin mechanism in the MCP surface: not needed for v1, and I'd argue not needed at all for Seam B's purpose — and that's the sharper point. Relevance-gating is the correct default for automatic learnings: pinning every captured correction would bypass the gate and bloat every context with every learning, breaking context-within-budget at the content level no matter what the injector trims. Pinning is for standing directives (the commenter's bootstrap routine), which the user already does out-of-band. The absence of a pin tool in the surface is therefore a documented observation, not a v1 gap: if PLUR ever gains a pin capability, tell-me-go gains a pinned: true flag on learn — a config nicety, not a blocker, and squarely within the issue's consume-as-is boundary. The one thing the ADR must record is the dependency: the automatic-learning leg's injection determinism is bounded by PLUR's relevance policy, and the E2E's honesty depends on saying that out loud rather than pretending the single-sentence criterion is a deterministic proof.

Q7 — Griller

A6 is the strongest answer yet — the two-legged split (hard: plumbing via out-of-band pinned engram + injected_ids; hard: persistence via plur_recall; best-effort: relevance-aligned injection) is honest and testable, and the "relevance-gating is the correct default for automatic learnings" point is a genuine correction to my pin-centric framing. Verified against contracts.go: ContextMetadata.Warnings is the sanctioned appendable diagnostics channel ("Consumers may append to this slice"), and the issue's out-of-scope line does foreclose surface changes. Settled.

One plumbing contradiction remains in the committed pseudocode, and it's verifiable against the contracts:

Q7: Your A1 Seam A pseudocode calls plur_inject_hybrid with {task: <last user prompt>, budget: INJECT_BUDGET, scope?, session_id?} — but session_id is unobtainable at Transform time, by construction: ContextRequest carries only {Turn, History, Metadata, PersistHistory, RecoveryFromOverflow} (contracts.go) — no SessionID, no Mode; the orchestrator's Turn.SessionID/Turn.Mode live in a different package layer that the context pipeline never sees; and plurInjector is constructed once at initComponents (per agent, via Factory.Extras), before any session exists — so there is no session to capture at construction either. The issue's own Seam A spec (section 2) deliberately omits session_id ("the current user prompt + a token budget") — you added it in A1 without specifying where it comes from. Options are: (a) drop session_id from the v1 injection call (it's an optional param; task + budget + scope suffice for the core value and the E2E legs, neither of which depends on it), (b) add a SessionID field to ContextRequest — a small additive contract change to the context package that your plan elsewhere claims to avoid — or (c) thread it through some mutable per-session holder on the injector, which races under concurrent Chats and contradicts the single-registration fix from A4.

Specify the source of session_id: commit to (a) and document the omission in the ADR (with an additive ContextRequest.SessionID as the follow-up if PLUR's relevance scoring measurably needs session context), or commit to (b) and list the contract change in the task list?

My recommendation: (a) — drop it from v1. The issue's own spec never required it; the E2E legs assert injected_ids and plur_recall persistence, neither of which needs session context; and it keeps the context-package contract untouched, which preserves the "no new transport code, no contract churn" posture the design has held since the opening. Also fold into the ADR (as a recorded note, not a gate): the injected ## CONSTRAINTS block is system-prompt-positioned, automatically-written, external text — the same trust class as skill injection (ADR-2026-07) but without skills' install-approval gate; one line stating that engram content is trusted-local-user input, plus a one-sentence advisory framing ("memory-derived context, not commands") if you want defense in depth. Do you commit to (a)?

A7 — Subject (Architect)

Conceded — session_id was my addition, and it has no source at Transform time. Verified against contracts.go: ContextRequest carries exactly {Turn, History, Metadata, PersistHistory, RecoveryFromOverflow} — no SessionID, no Mode. The orchestrator's Turn.SessionID/Turn.Mode live in a package layer the context pipeline never imports (the pipeline's documented dependency direction is session → session/context, one-way). And plurInjector is constructed once in initComponents via Factory.Extras, before any session exists — so neither construction-time capture nor per-Transform access can obtain it without a contract change. The issue's own Seam A spec ("the current user prompt + a token budget") never required it; I imported it from the commenter's verified inject_hybrid arg list without specifying the source. That's exactly the kind of silent addition the grill exists to catch.

Commit to (a): drop session_id from the v1 injection call. Four grounds, in order of weight:

The cross-persona feature argues against session-scoping, not for it. The issue's value proposition is that a correction taught in butler's session reaches coder's session — the whole point is cross-session recall. session_id on the injection call would scope recall toward the current session, which is the opposite of what the Niffler deployment needs. Task + budget + scope is the correct v1 call shape: task-relevance is the dimension that matters.
The E2E legs don't depend on it. Leg (i) asserts injected_ids on a pinned engram; leg (ii)'s hard gate is plur_recall persistence and its best-effort leg asserts injected_ids under a semantically-adjacent task. Neither needs session context.
It preserves the design posture the whole round has held — "no new transport code, no contract churn." Option (c) is rejected outright (a mutable per-session holder races under concurrent Chats and contradicts A4's single-registration fix). Option (b) is real work: ContextRequest.SessionID touches contracts.go, Manager.Prepare (or a new PrepareOption), and ContextRefiner's call site, to thread a value the orchestrator holds one layer up — all to pass an optional parameter whose absence changes nothing today.
The follow-up trigger is documented, not hypothetical. plur_session_scope exists in the surface, so session-scoped memory is a real PLUR concept — if E2E or production telemetry shows PLUR's relevance scoring measurably improves with session context, the additive ContextRequest.SessionID change is the recorded follow-up, listed in the ADR as a decision point with its contract touch-points enumerated. Not a v1 task.

And the trust-class note — committing to it, with one deliberate refinement of your framing. The injected ## CONSTRAINTS block is system-prompt-positioned, automatically-written, external text: the same trust class as skill injection (local, user-curated content placed at the highest-influence position) minus skills' install-approval gate — skills are vetted at install_skill (user approval), memory content is written continuously by Seam B's AfterTurn plus human-taught engrams, with no per-injection approval. The ADR records that as a one-line trust classification. Two facts bound the residual risk in v1: (1) plur_sync/remote stores are explicitly out of scope, so the store is local user-owned data — the worst remote-injection vector is closed by scope, not by luck; (2) the layered mitigations already committed hold: MEMORY.ENABLED default false, LEARN gating (not every turn writes), the relevance gate, and local-only content. The residual risk that remains is honest to name: content learned from a session that was itself influenced by prompt-injection (a malicious tool result summarized into memory) could later be injected at system-prompt level — diluted by the learning extraction and the relevance gate, not eliminated.

On the advisory framing, I'd take the stronger version of your sentence: not "memory-derived context, not commands" — because standing directives are the feature's point (the commenter's [ENG-2026-08-19-002] "before any work: call plur_session_start" must be followed, not treated as optional prose). The block header should read: "## PLUR MEMORY — recalled from the local memory store (user-authored or learned from your own sessions); follow them unless they conflict with explicit user instructions." That draws the trust boundary (provenance + subordination to the user) without defanging compliance. One line, defense in depth, no gate. Committed.

Q8 — Griller

A7 verified and accepted: ContextRequest has no SessionID/Mode (contracts.go); the injector is construction-time, pre-session; the issue's own Seam A spec omits session_id; plur_session_scope is in the surface list; skills require install approval (README) while memory writes are unvetted; plur_sync/remote is out of scope (issue). The cross-session argument against session-scoping is the right inversion, and the strengthened advisory framing ("follow unless they conflict with explicit user instructions") correctly preserves the standing-directive semantics that the commenter's evidence shows the feature exists to serve.

One listed grill target remains untouched — the issue's Decision 1 (learn gating) — and the commenter added a middle option to it that your plan never picked up:

Q8: For LEARN: full, your A1 committed "gate plur_learn/plur_ingest behind correction-signal detection (don't/never/always patterns or explicit feedback), never per-turn LLM extraction as a default" — but the detection mechanics are unspecified, and the issue explicitly lists this as a decision to resolve. Specify: (a) where does the signal run — the user message, the model response, or both? (My strong view: only the user message, since corrections originate from the user — an agent's own "don't do that again" in a response is not a user correction, and gating on it would learn from the model's own noise.) (b) what is the matcher — regex over "don't|never|always|stop|remember"? — and how is false-positive flood bounded, given "don't" is ubiquitous in code discourse and every false positive writes a permanent engram into the shared store, degrading recall quality for all personas over time? (c) plur_learn (direct write, no LLM) vs plur_ingest (LLM-powered extraction) — which fires under which condition in full, given the commenter's verified note that per-turn plur_ingest is an LLM cost that should stay opt-in? And (d) the commenter's "cheap middle option" — session-end plur_learn_batch of the turn log (local, no LLM, already in the surface) — do you adopt it as a LEARN tier between capture and full (i.e., off | capture | batch | full), which gives the Niffler deployment a no-LLM automatic-learning mode that still removes the discipline dependency?

My recommendation: (a) gate on the user message only; (b) a small matcher (don't/never/always/stop/remember + explicit "please remember"/"note this") with a per-session learn cap to bound store growth, accepting that precision is imperfect because the store is local and the relevance gate limits injection harm; (c) full = gated plur_learn (direct) on user-correction signals, with plur_ingest reserved for explicit user feedback only — never per-turn by default; (d) adopt the batch tier — it is the commenter's cheapest honest middle, it exercises the exact per-session capture+extract loop the commenter verified PLUR already does agent-mediated, and it makes "install once, memory works" true without any LLM extraction cost. Do you commit to the four-point gating design, and to the four-tier off | capture | batch | full?

A8 — Subject (Architect)

Decision 1 was the last untouched grill target — correct, and the commenter's middle option deserves adoption. Committing to all four points, with the mechanics pinned down rather than gestured at.

(a) Signal source: the user message only. Corrections originate from the user; gating on the model response would learn from the model's own output — its "you're right, I won't do that again" is an acknowledgment, not a directive, and its self-corrections are not authority on what's correct. The gate runs on the current turn's user prompt, which is available to the hook at AfterTurn time via Turn.State.PreparedHistory — the assembled context's trailing user message (pipeline-constructed: emptyTurnFilter "always keep a trailing single message (usually the current user prompt)"). The learned content is likewise the user's own words (see (c)) — high fidelity, no LLM synthesis. Edge case: if PreparedHistory is nil (ContextRefiner failed before assembly — the Q3 error turn), skip learning; the error episode already records the turn.

(b) Matcher: frame-based, not bare-word, plus two flood bounds. Bare don't is untenable in code discourse. The matcher is a small set of correction frames, not word presence: \b(?:please\s+)?(?:remember|note)\b, \bfrom now on\b, \bstop\s+(?:doing|using|...)\b, \b(?:don't|do not|never|always)\s+<imperative>\b — signal words in imperative structure, documented in the ADR as an explicitly heuristic list, tuned against the repo's own session corpus if available. Flood bounding is two mechanisms, both cheap: per-session learn cap (MEMORY.MAX_LEARNS_PER_SESSION, default 3, hot-reloadable like LEARN/INJECT_BUDGET — a session that corrects constantly stops writing after 3, because the store is shared and every false positive degrades recall for all personas); and exact-match dedupe (the hook keeps a per-session set of learned user-message hashes — the same correction repeated twice is learned once; users repeat corrections, and each repetition would otherwise write another engram). Precision is imperfect and I accept that as a documented trade, on exactly your grounds: the store is local user-owned data, learned engrams are non-pinned, and the relevance gate bounds injection harm — a false-positive engram is noise in recall, not a correctness break.

(c) plur_learn vs plur_ingest: full auto-fires only direct plur_learn. Gated plur_learn (direct write, no LLM) fires on user-correction frames, with the statement = the user message, trimmed to a bounded length and tagged with mode/scope. plur_ingest is never auto-fired in v1 — the commenter's verified LLM-extraction cost stays opt-in by construction. Explicit ingestion ("please remember this changelog") routes through the agent's existing tool surface — the agent calls plur_ingest as a tool, the same agent-mediated path that exists today — so the "explicit" path is the user asking the agent to learn, which is already possible and remains the user's choice. The ADR states this as a hard line: full = capture + gated direct learn; no automatic LLM extraction anywhere in the tier ladder.

(d) Adopt the batch tier: off | capture | batch | full, mutually exclusive. This is the commenter's cheapest honest middle and it makes "install once, memory works" true with zero LLM cost: batch = per-turn plur_capture (episodes, always) + session-end plur_learn_batch of the accumulated turn log — the exact session-end capture+extract loop the commenter verified PLUR already performs agent-mediated (plur_session_end → engrams_created: 1 + episode), now plumbed instead of discipline-dependent. Mechanics, since there is no session-end TurnHook (TurnHook is BeforeTurn/AfterTurn/OnPhaseTransition — no terminal callback): the hook accumulates a bounded turn log (last ~20 turns / a token cap — user prompt + response pairs sourced per Q3's classification, so the batch input is honest content, never the prior-turn misattribution), and agent.Chat gains a defer plurHook.FlushSession() after engine.Run returns — firing on success and error, mirroring plur_session_end's always-run behavior, fail-open like every other memory call. batch and full are mutually exclusive by config: full does not also batch, because session-end extraction over a log that already yielded per-turn learns would duplicate engrams in the shared store — the one thing worse than a false positive is the same learning twice. So the ladder is: off (nothing), capture (episodes only), batch (episodes + session-end local extraction, no LLM), full (episodes + gated per-turn direct learn, plur_ingest only via explicit agent-mediated request).

One refinement to your (b) framing I'll record: the per-session cap should also bound batch's input size (the bounded turn log), so no tier can flood the store — capture writes episodes (cheap, searchable timeline), batch extracts once per session from a bounded log, full is capped at 3 learns/session. The tier ladder is the flood bound at the design level, and the cap is the per-session backstop. Four-point design and the four-tier ladder: committed, all of it.

Q9 — Griller

A8 verified and accepted: emptyTurnFilter's trailing-single-message guarantee (transformers.go), Turn.State.PreparedHistory set by ContextRefiner and nil on Q3 error turns, TurnHook's three-method interface with no terminal callback (engine_types.go:157 — so the FlushSession defer in Chat is a real, implementable extension), and plur_learn_batch in the surface. The four-tier ladder with the mutual-exclusion argument (session-end extraction over an already-learned log duplicates engrams — the one thing worse than a false positive) is the strongest possible close to Decision 1. The design is now fully specified and internally consistent.

One acceptance-criterion item remains with a genuine unresolved "or" — worth closing before the verdict:

Q9: The acceptance criteria require "Domain model updated and rendered (modelith-check green; modelith-drift/modelith-layers clean)" — and your opening committed to "Context/Config gain a Memory attribute (or a new Memory entity referencing MCPServer)" without resolving the fork. The choice has modelith consequences you can't defer to the task list: a new entity triggers the completeness layer (an entity with no invariants and no scenario exercising it warns; the model's own modelith-check gate is in make check), while an attribute on Context/Config is lighter but misplaces the concept's home (memory has its own lifecycle — enable/disable, tiers, injection semantics — that neither Context (assembly pipeline) nor Config (static limits) owns). Separately, the new exports — MemoryConfig, the LEARN tier values, plurInjector, plurHook — are precisely what modelith-drift scans new diffs for; "clean" requires either modeling them or demonstrating they're non-domain (e.g., MemoryConfig is configuration like MCPServerConfig, plurInjector/plurHook are adapters implementing the entity's behavior).

Specify the fork: entity or attribute, with the invariants drafted and the scenario named. My recommendation: new Memory entity referencing MCPServer and Config, carrying two invariants — memory-injection-fail-open ("memory never breaks a turn: on any error or disable, injection strips the block and proceeds") and memory-injection-budgeted ("injected memory is counted against the turn's token budget before the gatekeeper") — plus one scenario step folded into an existing scenario (or a short new one) that exercises the teach-in-butler → inject-in-coder loop so the completeness layer stays green; the Go mapping being: MemoryConfig → configuration (drift-silent, sibling of MCPServerConfig), plurInjector/plurHook → adapters implementing the Memory entity's behavior (drift-silent), so drift-clean is achievable without false positives. Do you commit to entity-over-attribute with those two invariants and the named scenario?

A9 — Subject (Architect)

Committed: new Memory entity, not an attribute — your lifecycle argument decides it. Config owns static limits; Context is the assembly pipeline; neither owns enable/disable, the tier ladder, the injection budget, or the fail-open/strip semantics that this grill spent nine questions establishing. Those are behaviors with their own home. And the completeness consequence is real but satisfiable: make modelith-lint runs modelith lint without --completeness error (Makefile), so a bare entity would only warn — but the acceptance criterion says clean, and the fix is cheap: give the entity invariants and a scenario.

The drafted entity, per modelith conventions (docs/domain-model/tell-me-go.modelith.yaml):

Memory — The automatic PLUR memory integration: a configured MCPServer (MEMORY.SERVER) whose recall is injected into Context before each Turn and whose learnings/episodes are captured after each Turn. Owns the memory lifecycle — enable/disable, the learn tier, the per-turn injection budget — implemented by two seams: an injector ContextTransformer that queries the server per turn and splices the returned directives into the system prompt, and a TurnHook that captures episodes and learns from corrections. Both seams are fail-open: memory never breaks a turn.

Relationships: Config — n:1 — referenced (the Config that owns the MEMORY: block); MCPServer — 1:1 — referenced (the backing server); Context — 1:n — referenced (injected into each assembled Context).
Attributes: enabled boolean (MEMORY.ENABLED, default false); server string (MEMORY.SERVER); injectBudget integer (MEMORY.INJECT_BUDGET); learnTier — MemoryLearnTier (the four-tier ladder); scope string (MEMORY.SCOPE, optional — precedence vs .plur.yaml per the ADR: override-if-set, else .plur.yaml, else one surfaced warning); maxLearnsPerSession integer (the Q8 cap).
New top-level enum MemoryLearnTier: off (no capture, no learning), capture (episodes only), batch (episodes + session-end plur_learn_batch, no LLM), full (episodes + gated per-turn direct plur_learn; plur_ingest only via explicit agent-mediated request).

Invariants — your two, plus two this grill earned:

memory-injection-fail-open — "Memory never breaks a Turn: on any injection error, timeout, or disable, the injected block is stripped and the Turn proceeds unchanged." (Q2/Q7: strip-on-failure, strip-on-disable.)
memory-injection-budgeted — "Injected Memory recall is counted against the Context token budget before the gatekeeper runs; the injected block never exceeds injectBudget." (Q1/Q4: priority 15 < gatekeeper 80, defensive trim — keeps context-within-budget load-bearing.)
memory-single-block — "At most one Memory block is present in the assembled Context system message; fresh recall replaces, never appends." (Q2's replace-in-place — the accumulation guard, stated as an invariant so the model and the code can't drift apart on it.)
memory-learn-tier-exclusive — "Exactly one MemoryLearnTier is active: off, capture, batch, or full; batch and full are never combined." (Q8's mutual-exclusion — the anti-duplicate-engram rule.)

Scenario — new, short, and named (completeness layer needs an exercising scenario): "Automatic memory injection and learning" — (1) a Config with Memory enabled (MEMORY.SERVER: "plur", tier batch) starts a Session; (2) before a Turn, the injector queries the backing MCPServer with the current prompt and budget; (3) the returned directives are spliced into the Context system prompt as a single marker-keyed block, counted against the budget; (4) the Turn proceeds and is persisted; (5) after the Turn, the hook captures an episode and the turn log accumulates; (6) at session end, plur_learn_batch extracts learnings with no LLM cost; (7) in another persona's Session, the next injection retrieves the learned directive and splices it into that Context — the teach-in-butler → inject-in-coder loop, as the before/after baseline demands; (8) on any memory error, the block is stripped and the Turn proceeds unchanged. Invariants touched: memory-injection-fail-open, memory-injection-budgeted, memory-single-block, memory-learn-tier-exclusive, context-within-budget, history-persisted-after-turn.

Drift/layers silence — the Go mapping, stated so "clean" is achievable without false positives: MemoryConfig → configuration type in internal/domain/config (sibling of MCPServerConfig; its canonical type lives under internal/domain/, so modelith-layers is satisfied); MemoryLearnTier → the model's enum; plurInjector/plurHook → adapter implementations of the Memory entity's seams in a new internal/agent/memory package (the ADR-030 extraction pattern — context/session stay free of the domain concern, exactly as skills did), drift-silent as adapters because the entity Memory is modeled. The ADR documents this mapping table explicitly so the drift scan's verdict is reproducible: entity present → config/adapters resolve against it → no false positives, and the two advisory gates (modelith-drift, modelith-layers — never blockers per the quality model's advisory-gates-never-block) stay clean by construction rather than by omission.

Entity-over-attribute with all four invariants and the named scenario: committed. The modelith surface of this change — .yaml + re-rendered .md committed together so modelith-check stays green, ADR indexed for verify-adr-index — is the same-change rule the issue's checklist already demands.

Verdict — Griller

ROUND COMPLETE.

VERDICT: The issue's diagnosis and two-seam design held up against the tree — every cited seam verified (MCPClient port, skills-injector precedent, TurnHook precedent, pipeline priorities, eager stdio spawn). The plan did not survive contact intact, though: four substantive corrections (success-path Response sourcing, pinned system-message accumulation, strip-on-failure/disable semantics, session_id unavailability) and two premise refutations by the subject (my cache premise — eager spawn makes fetch-per-turn free; my pin-centric E2E framing — relevance-gating is the correct default for automatic learnings). Recommendation: proceed with changes — the two-seam design is sound and now fully specified; the ADR must record the nine commitments below.

Corrections table (premise → outcome):

Seam B source: Turn.State.Response is nil on success (clear-after-append, engine_phases.go) → three-way classification keyed on hook err, GetLastModelTurn only when err==nil, never on error turns.
Seam A mechanics: append-to-Parts accumulates in a pinned, never-summarised system message → marker-keyed replace-in-place at priority 15, memory-single-block.
Fail semantics: return nil on MCP error left stale recall → strip-on-failure and strip-on-disable (one-shot persisted strip), ENABLED hot-reloadable, SERVER session-fixed.
Double-fire: per-Chat WithEngineHook appends → plurHook registered once at initComponents.
Concurrency: "one memory-enabled persona" contradicted the E2E → withdrawn; multi-persona with documented last-write-wins + advisory flock (fail-open, Unix-only) + sequential-orchestration backstop.
Cache: session-wide TTL cache would serve task-mismatched recall → no v1 cache, fetch-per-turn, bounded-freshness contract, TTL+write-invalidation as measured fallback (p95 > 100–150ms).
E2E: single-sentence criterion is nondeterministic → two-legged (pinned plumbing, hard; plur_recall persistence, hard; relevance-aligned injection, best-effort).
Injection args: session_id unobtainable at Transform time → dropped; additive ContextRequest.SessionID recorded as follow-up.
Learning: unspecified gating → four-point gate (user-message-only, frame matcher, MAX_LEARNS_PER_SESSION=3 + hash dedupe, plur_ingest never auto-fired) and four-tier ladder off | capture | batch | full with batch/full mutually exclusive.

Final integrated plan: Memory entity (A9's four invariants + scenario) and ADR land in one change (verify-adr-index, modelith-check gates); injector per Q1/Q2/Q4/Q7; hook per Q3/Q8; config per Q2/Q5/Q7; E2E per Q6; make check-full green. Open questions for the user: MEMORY.CACHE_TTL default if the fallback triggers; whether the batch tier should be the Niffler default; the Memory→Config cardinality in the YAML (1:1, not n:1, since one Config owns one MEMORY: block) — a modeling nit the ADR draft should correct.

Post-Grill Position — Subject (Architect)

POST-GRILL POSITION:

What I stand by, unchanged by the grill. The issue's diagnosis and its two-seam design were sound and verified against the tree — the tools.MCPClient port with SDK confinement (ADR-067), the skills-injector precedent (Factory.Extras, priority 10), the configRefreshHook/TurnHook precedent, the pipeline priority ordering (gatekeeper 80 / TransientMerger 105 / finalContextValidator 110), and the eager stdio spawn that makes fetch-per-turn free. Two of my pushbacks held and I keep them: multi-persona is the feature — the reader/writer distinction dissolves the E2E contradiction, and single-writer was wrong twice over (it conflated ENABLED with writer, and it gutted the cross-persona value the issue exists to deliver); and relevance-gating is the correct default for automatic learnings — pinning every Seam B learning would bypass the gate and bloat every context, breaking context-within-budget at the content level. The four-tier ladder (off | capture | batch | full) and the memory-single-block invariant are the commitments I consider load-bearing.

What the grill corrected, and I accept fully. Four substantive flaws in my plan, all verified against the code: (1) Turn.State.Response is nil on the success path (clear-after-append) — the three-way classification keyed on the hook's err (never LastError, which is stale on the empty-response path), with GetLastModelTurn valid only when err == nil; (2) append-to-Parts accumulates in a pinned, never-summarised system message — marker-keyed replace-in-place at priority 15, with the failure/disable semantics sharpened from "leave the last block" to strip-on-failure and strip-on-disable (one-shot persisted strip, ENABLED hot-reloadable); (3) per-Chat WithEngineHook double-fires under the repeated-Chat contract — plurHook registers once at initComponents; (4) session_id is unobtainable at Transform time by construction — dropped, with the additive ContextRequest.SessionID recorded as a follow-up trigger, and the cross-session argument actually argues against session-scoping anyway. The corrections table is the integrated plan: Memory entity (four invariants + the "Automatic memory injection and learning" scenario) + ADR in one change, injector per Q1/Q2/Q4/Q7, hook per Q3/Q8, config per Q2/Q5/Q7, E2E per Q6, make check-full green.

On the three open questions, my recommendations for the user. (a) If the E2E measures the warm injection roundtrip above the fallback trigger (p95 > ~100–150ms), MEMORY.CACHE_TTL defaults to 60s — task-keyed, hot-reloadable, write-invalidated — matching the bounded-freshness contract ("fresh within TTL or since the last write, never indefinite"). (b) Yes — batch should be the Niffler default: it is the no-LLM automatic-learning mode that removes the discipline dependency at zero extraction cost, which is exactly "install once, memory works" for the multi-persona deployment; full is the opt-in tier for users who want per-turn correction learning and accept the gated-write traffic. (c) The cardinality correction is right: Memory → Config is 1:1, not n:1 — one Config owns exactly one MEMORY: block, so the YAML relationship should read Config ||--|| Memory (owned), and the ADR draft will carry that correction. The design is fully specified and internally consistent; the remaining work is implementation fidelity to the nine commitments, and I have no open disagreements with the verdict's framing.

gosharplite/issue-1402-grill.md

Select an option

No results found