Skip to content

Instantly share code, notes, and snippets.

@gosharplite

gosharplite/issue-1403-grill.md

Created August 19, 2026 05:39
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save gosharplite/213ada4b83cca349afeadef12ff729f0 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save gosharplite/213ada4b83cca349afeadef12ff729f0 to your computer and use it in GitHub Desktop.
Download ZIP
Grill round transcript — tell-me-go issue #1403 (Automatic PLUR memory integration, implementation spec): Architect vs Griller, 5 questions, verdict + post-grill position
Raw
issue-1403-grill.md

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

Subject: Architect persona | Griller: Griller persona (provider: vertex-flash-3.7) | Topic: Automatic PLUR memory integration — implementation spec Questions: 5 (fixed round)

Opening Statement — Subject (Architect)

All citations verified. I have everything I need — here is my opening statement.

Opening statement — SUBJECT understanding and implementation plan for issue #1403

1. Understanding. Issue #1403 adds automatic PLUR memory integration to tell-me-go through the two extension seams the repo already ships: Seam A — a plurInjector implementing sessctx.ContextTransformer, registered in Factory.Extras at initComponents (the exact slot where agentskills.NewSkillInjector is wired today, internal/agent/agent.go), and Seam B — a plurHook implementing orchestrator.TurnHook (BeforeTurn/AfterTurn(*Turn, err)/OnPhaseTransition, internal/agent/orchestrator/engine_types.go), registered once at init. I verified both seams exist and both precedents hold: the skill injector runs at Priority 10, injects into the system message, sets Pinned and PersistHistory (internal/agent/skills/injector.go), and configRefreshHook is a real TurnHook whose OnPhaseTransition fires on Inference→Executing and whose errors are deliberately ignored per ADR-029 §5 (internal/agent/agent.go). The issue is correctly scoped: tell-me-go core only, consuming PLUR's MCP surface as-is via the tools.MCPClient domain port (internal/domain/tools/mcp_client.go) — no new transport code, preserving verify-mcp-sdk-confinement (MCP SDK confined to internal/infrastructure/mcp/).

2. The four load-bearing mechanics are all real, verified against the tree. (a) Clear-after-append: PersistenceStep.Process sets Turn.State.Response = nil after a successful AddContent and only keeps it on the persistence-failure path (internal/agent/orchestrator/engine_phases.go, #1302) — so the three-way classification keyed on the hook's err argument is correct, and GetLastModelTurn is only valid when Response == nil && err == nil. I also confirmed why Turn.State.LastError is stale on the empty-response path: on max-empty-retries, RecoveryStep returns PhasePersisting, nil — the hook receives err == nil while LastError still holds errEmptyResponse — so the design's "never key on LastError" rule is right. (b) Double-registration hazard is real: WithEngineHook appends to e.hooks (engine.go), and agent.Chat calls a.engine.ApplyOptions(orchestrator.WithEngineHook(...)) on every Chat — per-Chat hook registration would double-fire. (c) Non-stable sort: NewContextPipeline uses sort.Slice (pipeline.go), so distinct priorities are mandatory — priority 15 is distinct from every existing value (0, 3, 4, 5, 6, 10, 80, 90, 100, 105, 110, 110 — verified per-transformer), and it lands in the canonical (<100) partition, i.e. before persistChanges, so the injected block is token-counted by the gatekeeper (80) and persists when a sibling canonical transformer sets PersistHistory. (d) Eager spawn: NewStdioClient completes the handshake in the constructor (stdio_client.go), so fetch-per-turn is a warm roundtrip and the no-v1-cache decision stands.

3. Seam A plan (injection). New package internal/agent/memory (following the skills extraction pattern — note: the issue cites "ADR-030 extraction pattern", but ADR-030 is Release Branch Synchronization Policy; the skill-injector extraction is ADR-040 Complete Session Subpackage Extraction (config_watcher, skill_injector) — I will record the corrected citation in the ADR). The injector is constructed at the DI composition root with the tools.MCPClient for MEMORY.SERVER (ADR-067 pattern, internal/infrastructure/di/mcp_factory.go construction), registered in Factory.Extras alongside the skill injector in initComponents, Priority 15. Transform: marker-keyed replace-in-place on the system-message Parts using ## PLUR MEMORY / [/PLUR-MEMORY] sentinels — append only on first injection, never accumulate (unlike skills, which append; memory-single-block invariant). Fetch-per-turn via plur_inject_hybrid with {task: <current user prompt>, budget: INJECT_BUDGET} — no session_id (correct: ContextRequest carries {Turn, History, Metadata, PersistHistory, RecoveryFromOverflow} only, contracts.go; SessionID is the v1.1 additive follow-up). Fail-open with strip semantics: on any MCP error/timeout → log, strip the marker block, return unchanged; on disable mid-session → one-shot persisted strip (PersistHistory = true), then no-op. Never sets PersistHistory itself. Defensive trim to INJECT_BUDGET (pinned engrams make server size nondeterministic). Emit injected_ids via ContextMetadata.Warnings (appendable, per contracts.go) and debug log for the E2E legs.

4. Seam B plan (learning) + config. plurHook registered once at initComponents (via an AgentOption threaded into NewEngine opts — not per-Chat WithEngineHook), with belt-and-suspenders turn-scoped dedupe on last SessionID+Index. Three-way classification keyed on the hook's err argument: (i) Response != nil → episode from this turn's response + Mode/SessionID/timestamp, annotating LastError if set; (ii) Response == nil && err != nil → error episode from PreparedHistory's last user message, never GetLastModelTurn; (iii) Response == nil && err == nil → GetLastModelTurn (provably this turn's, post-clear-after-append), skip if no text parts. LEARN tiers off|capture|batch|full, mutually exclusive, default batch: batch = per-turn episodes + session-end plur_learn_batch of a bounded ~20-turn log, flushed via defer in Chat (success and error, mirroring where the session lifecycle is owned); full = gated per-turn plur_learn with frame matcher + MAX_LEARNS_PER_SESSION=3 + exact-match dedupe; plur_ingest never auto-fired. Fail-open like configRefreshHook. Write-path concurrency: documented last-write-wins + best-effort advisory flock (~/.plur/.tmg-write.lock, Unix-only, bounded acquisition, uncreatable → proceed unlocked) — consistent with the multi-persona operating model. Config: MemoryConfig as a sibling of MCPServerConfig in internal/domain/config with a validate() following the mcp_config.go:103 precedent (enabled-but-absent-server → warn + disable, parallel to the config-valid-provider invariant); ENABLED/LEARN/INJECT_BUDGET/MAX_LEARNS_PER_SESSION ride the existing watcher → configRefreshHook.OnPhaseTransition → applyConfig hot-reload chain (agent.go); SERVER session-fixed. Default disabled → zero behavior change.

5. Domain model, ADR, E2E, and flagged risks. The domain-model change is a genuine addition — the current model (docs/domain-model/tell-me-go.modelith.md) has no Memory entity; the issue's proposed entity (Config 1:1 owned / MCPServer 1:1 referenced / Context 1:n referenced), MemoryLearnTier enum, four invariants, and the teach-in-one-persona→inject-in-another scenario are coherent with existing invariants context-within-budget, context-pinned-preserved, and history-persisted-after-turn; the four new invariants must be wired into the scenario's invariants_touched and .yaml + re-rendered .md committed together for modelith-check/modelith-layers green (the Makefile gates cover all three models). New ADR-068 (index is at ADR-067, docs/adr/README.md) containing the corrections table, failure/disable matrix, trust-class note (system-prompt-positioned auto-written external text — same class as skill injection minus the install-approval gate), multi-persona concurrency stance, and v1.1 follow-ups; verify-adr-index green. Tests: hand-rolled function-field mock MCP client per ADR-021 (verify-mock-pattern is zero-tolerance), race-tested via test-race. E2E two-legged as specified — leg (i) hard (out-of-band pinned engram, since PLUR has no pin tool — documented setup step), leg (ii)(a) hard + (ii)(b) best-effort with one documented re-run allowance, never a gate failure. Risks I flag up front: (1) the ADR-030→ADR-040 citation correction above; (2) the existing 110/110 collision between HistoryPruner and finalContextValidator — pre-existing, not introduced, but worth an ADR note since the issue states distinct priorities as a hard rule; (3) the injection is system-prompt-positioned and read per-turn, so the E2E's leg (ii)(b) nondeterminism (relevance gate) must stay best-effort as specified; (4) the one-shot persisted strip on disable must coordinate with the canonical/transient partition (priority <100 persists pre-transient), which the design already satisfies at 15.

That is my understanding and plan. The griller may probe any citation above.

Q1 — Griller

Q1: In your opening statement for Seam A, you specified that plurInjector runs at Priority 15, performs marker-keyed replace-in-place using ## PLUR MEMORY / [/PLUR-MEMORY] sentinels, never sets PersistHistory on normal turns, but performs a "one-shot persisted strip (PersistHistory = true)" when memory is disabled mid-session (ENABLED: false).

How will plurInjector.Transform structure this state machine to handle:

  1. Initial insertion when no system message exists: If skillInjector matched no skills and no ecosystem intro is set, req.History[0] has Role: "user". Does plurInjector prepend a new pinned system message in memory?
  2. Persistence coupling with sibling transformers: When skillInjector (Priority 10) matches a skill and sets req.PersistHistory = true, the memory block injected at Priority 15 gets persisted to disk via persistChanges. On the next turn, loadHistory loads that persisted block from disk. How does plurInjector locate and replace the block across req.History[0].Parts?
  3. Mid-session disable detection: If plurInjector relies on stateful transition tracking (e.g. lastEnabled != currentEnabled) to fire the "one-shot" persisted strip, how does it avoid misfiring or state corruption when Prepare is invoked multiple times within a single turn (such as on RecoveryStep's WithOverflowRecovery() retry path)?

Recommendation:

  1. System message creation: If len(req.History) > 0 and req.History[0].Role != "system", prepend &llm.Content{Role: "system", Pinned: true, Parts: []*llm.Part{{Text: block}}}. If Role == "system", mutate req.History[0].
  2. Marker replacement: Scan req.History[0].Parts for the sentinel boundaries (## PLUR MEMORY to [/PLUR-MEMORY]). If found, replace the sentinel text within that Part; if not found, append a new &llm.Part{Text: block}.
  3. Stateless strip-on-disable: Avoid transformer-level mutable state (wasEnabled flags). Instead, make strip-on-disable purely content-driven: when !enabled, inspect req.History for the sentinel. If the sentinel is present, strip it and set req.PersistHistory = true; if absent, no-op. This makes disable stripping naturally idempotent and resilient to multi-pass Prepare executions.
  4. Fail-open on MCP error/timeout: Strip the sentinel block from req.History in memory (ensuring no stale recall reaches the provider), but do not set req.PersistHistory.

A1 — Subject (Architect)

Q1 answer. Verified mechanics first: Prepare reloads history from the store on every invocation — Manager.Prepare → loadHistory → History.GetWindow(ctx, 0, -1) (internal/agent/session/context/manager.go), and multi-pass per turn is real: on context overflow RecoveryStep sets RecoveryFromOverflow = true and returns PhaseRefining, which re-runs ContextRefiner.Process → Prepare again with WithOverflowRecovery() (internal/agent/orchestrator/engine_phases.go). So Transform can run twice per turn, each time on a freshly-loaded req.History. The pipeline executes canonical transformers (<100) before persistChanges, and priority 15 is canonical (internal/agent/session/context/pipeline.go).

The state machine (fully stateless in the transformer; the only fields are the injected tools.MCPClient, an atomic readable memory-config, and a logger):

Transform(ctx, req):
  if len(req.History) == 0: return nil                     // defensive, mirrors skills
  enabled := memCfg.Enabled()                              // read per-Transform (hot-reloadable)

  if !enabled:                                             // content-driven strip-on-disable
      if blk := findMemoryBlock(req.History); blk != nil:
          removeMemoryBlock(req.History, blk)              // drop the sentinel Part; if the system
                                                           //   Content is left with 0 Parts, drop the Content
          req.PersistHistory = true                        // one-shot persisted strip (canonical → persists)
      return nil                                           // absent → no-op, never sets PersistHistory

  recall, ids, err := client.CallTool(ctx, "plur_inject_hybrid",
      {task: lastUserPrompt(req.History), budget: INJECT_BUDGET, scope?})
  if err != nil:                                           // fail-open
      log("memory injection failed; stripping", err)
      if blk := findMemoryBlock(req.History); blk != nil:
          removeMemoryBlock(req.History, blk)              // in-memory only — do NOT set PersistHistory
      return nil

  block := buildMemoryBlock(trimToBudget(recall, INJECT_BUDGET))
  sys := ensureSystemMessage(req)                          // [0] system → mutate; else prepend
  replaceOrAppend(sys, block)                              // sentinel present → replace that Part's Text; else append
  recordInjectedIDs(req, ids)                              // ContextMetadata.Warnings + debug log
  return nil

1. Initial insertion with no system message. Yes — prepend, mirroring skillInjector.prependNewSystemMessage exactly: &llm.Content{Role: "system", Pinned: true, Parts: []*llm.Part{{Text: block}}} at req.History[0] (internal/agent/skills/injector.go). Two verified properties make this safe and compositional: (a) AddContent enforces first-message-is-user for new history (manager.go), so on a fresh session req.History[0].Role == "user" — but that only holds until the first SetContents persists a system message (see 2); (b) because skills (10) runs before memory (15), if skills created a system message this turn, memory sees it at [0] and mutates; if skills didn't, memory creates one. At most one system message is created per turn — the two injectors cannot both prepend on the same pass. The prepend is in-memory for that ContextRequest; it persists only if a sibling canonical transformer set PersistHistory — which is the issue's stated contract ("Never sets req.PersistHistory").

2. Locate-and-replace across turns. The block is one self-delimited Part: Text = "## PLUR MEMORY\n…recall…\n[/PLUR-MEMORY]". Because the injector controls both write and read, location is a substring scan, not an index. Round-trip is verified: when a sibling sets PersistHistory, persistChanges → SetContents(ctx, req.History) writes the entire history including the pinned system Content at [0] (manager.go executePipeline); next turn GetWindow reloads it, so req.History[0].Role == "system" with parts [skills-part…, memory-part]. replaceOrAppend: scan req.History[0].Parts for a Part whose Text contains the start sentinel → overwrite that Part's Text with the fresh block; if absent → append a new Part. Never index-based (parts arrays shift when other transformers mutate). Defensive: findMemoryBlock scans all of req.History, not just [0], and the sentinel is unique (## PLUR MEMORY cannot appear in skills content), guaranteeing memory-single-block even under history-layout drift. Note the verified corollary: when skills matches persistently, skills returns early via isAlreadyInjected (injector.go) and sets no PersistHistory that turn — so the disk copy may lag one turn, but the in-memory replacement happens at priority 15 before the gatekeeper (80), so the provider always sees fresh recall. Disk staleness is bounded at one marker-identified block, exactly as designed.

3. Mid-session disable — content-driven, zero transformer state. I agree with the griller and reject lastEnabled != currentEnabled tracking, for a reason verified against the engine: with a stateful flag, the multi-pass case either misfires or misses. Concretely: pass-1 Prepare (enabled, injected, sibling persisted the block) → overflow → pass-2 Prepare with WithOverflowRecovery() — if the config flipped to disabled between passes, the flag fires the persisted strip (correct), but if pass-1's SetContents failed, pass-2 sees no transition and never retries — the stale block stays on disk. Content-driven inverts this: the sentinel's presence, not a transition, is the trigger. Pass-1 strips and persists; pass-2 reloads the store, finds no sentinel, no-ops. If pass-1's persistence failed, pass-2 reloads the unstripped history, finds the sentinel, strips and retries — self-healing, and it fires exactly once per disable episode (after the strip persists, the sentinel is gone until a re-enable re-introduces it, which should re-fire). The "one-shot" in the issue is honored without any bookkeeping: it means "when disabled, do a persisted strip rather than a per-turn in-memory replace", and content inspection makes it naturally idempotent and multi-pass-resilient — which also satisfies the ContextTransformer contract ("safe for single-goroutine sequential use") without relying on ordering across Prepare invocations.

Refinements to the recommendation (all four points accepted): (1) correct — mirror skills' prepend verbatim, Pinned: true; (2) correct — sentinel-delimited single Part, replace-or-append; (3) correct — content-driven strip, PersistHistory = true only when a sentinel is actually found; (4) correct — error path strips in-memory but never persists. One addition to (3)/(4): after removing the memory Part, if the system Content is left with zero Parts, remove the Content from req.History — an empty system message can be rejected on the wire, and since the strip (disable path) persists, the removal is durable; on the error path it's in-memory only and reloads next turn, harmless. This keeps the invariant "inject current recall, or nothing — never stale recall" clean on both paths.

Q2 — Griller

Q2: In Seam B, plurHook implements orchestrator.TurnHook to capture episodes and learn from conversation turns. However, examining internal/agent/orchestrator/engine_types.go and engine.go reveals three concrete execution and lifecycle constraints:

  1. Missing context.Context in TurnHook: AfterTurn(turn *Turn, err error) accepts no context.Context parameter, and Turn does not store a context. When AfterTurn or FlushSession issues MCP calls (plur_capture, plur_learn, plur_learn_batch), what context and timeout does it use, and how does it behave when the turn ended due to context cancellation or timeout (err == context.Canceled / context.DeadlineExceeded)?
  2. Multi-iteration turns in Engine.Run: Within a single agent.Chat invocation, Engine.Run loops across multiple orchestrator.Turns when tool calls occur (Turn.Index = 0, 1, ...). Does AfterTurn fire plur_capture on every intermediate tool iteration, or only on turns that emit user-visible response text or terminal errors?
  3. Session buffering and lock contention: For LEARN: batch (the default), where does plurHook buffer turn records prior to FlushSession, how is that buffer pruned to prevent unbounded memory growth across long-running sessions, and how does the advisory write lock (~/.plur/.tmg-write.lock) bound acquisition time so a locked PLUR store never stalls turn completion?

Recommendation:

  1. Bounded detached context: In AfterTurn and FlushSession, construct a detached context with a short, deterministic timeout (e.g., context.WithTimeout(context.Background(), 3*time.Second)). If the turn failed due to cancellation, still record the error episode using this detached context (so user cancellations/aborts are captured), but skip plur_learn extraction.
  2. Episode filtering on tool iterations: Only emit an episode in AfterTurn when:
    • turn.State.Response (or GetLastModelTurn) contains non-empty text parts (ignoring tool-call-only turns), OR
    • The turn ended in a non-retryable error (err != nil && !IsTransient(err)). Intermediate tool-execution iterations with no model text should not generate noisy empty episodes.
  3. Bounded session ring buffer & fail-open flock:
    • Maintain a thread-safe ring buffer per SessionID capped at 20 turns in plurHook. FlushSession drains and deletes the session entry.
    • Advisory file lock on ~/.plur/.tmg-write.lock: use non-blocking syscall.Flock(LOCK_EX|LOCK_NB) with a short polling timeout (max 200–500ms). If acquisition fails, log a debug warning and proceed unlocked (fail-open), preserving turn responsiveness.

A2 — Subject (Architect)

Q2 answer. Verified the three constraints against the tree, then the state machine:

1. No ctx in TurnHook — detached bounded context is the only option, and it's on the critical path. TurnHook.AfterTurn(turn *Turn, err error) has no ctx (engine_types.go), and Turn carries no context field — engine.go's Turn holds Index/SessionID/StartTime/State/LoopDetector/CtxManager/Gateway/Executor/Registry/TokenCounter/Events/MaxToolTurns/Clock/CostTracker/ProviderName/Model/Mode/Logger/SessionTurnsAtStart/Stop, nothing context-like; ExecuteTurn's ctxWithTrace is function-local and discarded before notifyAfterTurn (engine.go). Critically, notifyAfterTurn runs synchronously inside ExecuteTurn after runPhaseLoop and before return err — so AfterTurn is on the turn's completion path, and any ctx derived from the turn's parent would already be cancelled when the turn died of cancellation. Therefore: each MCP call in AfterTurn/FlushSession uses a fresh detached context — context.WithTimeout(context.Background(), 3*time.Second) per call (or per hook invocation, shared across that invocation's calls) — bounded by min(3s, the server's EffectiveTimeout via operationContext, verified in internal/infrastructure/mcp/stdio_client.go), fail-open on error (log + ignore, the configRefreshHook/ADR-029 §5 posture). On err == context.Canceled/DeadlineExceeded: the design's branch (ii) — Response == nil && err != nil → error episode {error, prompt: last user message from PreparedHistory, mode, session, timestamp} — already covers it, and the issue explicitly says "No learning from this branch". So user aborts ARE captured as error episodes (via plur_capture, tiers capture/batch/full) using the detached ctx, and plur_learn/plur_learn_batch extraction is skipped on that branch. This is not a new decision — it is the three-way classification applied to cancellation errors, with the detached ctx being the mechanism that makes the capture possible after the turn's own ctx is dead. FlushSession (deferred in agent.Chat, success and error) uses the same detached-bounded pattern: on the error path Chat's ctx is cancelled, so it must not inherit it.

2. AfterTurn fires on every Engine.Run iteration — the text-parts filter already suppresses tool iterations. Verified: Run loops ExecuteTurn and breaks only when shouldStopRunning sees HasToolCalls false or Stop (engine.go) — a tool-executing turn is a full ExecuteTurn with its own BeforeTurn/AfterTurn. On a successful intermediate tool iteration, Turn.State.Response is nil (PersistenceStep clear-after-append), so the classification falls to branch (iii): GetLastModelTurn returns the just-appended FunctionCall-only content — no text parts → episode skipped, per the issue's own rule. The empty-response-max-retries path also lands in branch (iii) with an empty response → skipped. So no noisy empty episodes from tool iterations, and no new filtering code is needed for them — the design already handles it. One refinement I accept from the griller: add !IsTransient(err) to branch (ii). This is not cosmetic — I verified llm.IsTransient uses errors.Is (gateway.go), so after retry-exhaustion on a transient error, RecoveryStep.handleFailure returns fmt.Errorf("max retries reached: %w", err) wrapping ErrRateLimit/ErrTransient, and the terminal err reaching AfterTurn is still IsTransient-true (errors.Is unwraps). As written, the issue would capture error episodes for those; the refinement skips them (no learning value — the user prompt was never answered and repeats next turn). Genuine terminal outcomes still capture: ErrTerminal, ErrAuth (IsAuth), and ErrContextLimitExceeded after overflow recovery (IsTerminal includes it — verified gateway.go) all reach AfterTurn non-transient. I'll record this in the ADR's corrections table alongside the existing resolved-decision 1.

3. Buffer + flock. Buffer: a plurHook-owned map map[SessionID]*ringBuffer guarded by a mutex, each a bounded ring capped at 20 entries (count) plus a per-entry text/token cap (truncate episode text to a per-episode budget) — bounded in both count and bytes so a long-running session cannot grow memory unbounded. Append is O(1), oldest evicted on overflow. FlushSession: lock → drain (swap out and reset the ring) → unlock → call plur_learn_batch with the drained records in the detached bounded ctx → delete the map entry (prevents unbounded map growth across repeated Chats in one process — agent tests call Chat repeatedly). Per-session keying matters because multi-persona is the operating model and one process can host several sessions. Lock: syscall.Flock(LOCK_EX|LOCK_NB) on ~/.plur/.tmg-write.lock, polled on EWOULDBLOCK at ~25–50ms intervals up to a 200–500ms budget, then fail-open — debug log + proceed unlocked; uncreatable file → proceed unlocked; Unix-only via build tags (no-op on Windows, matching the issue's "Unix-only"). The lock is held only across the MCP write calls (capture/learn/learn_batch), released before AfterTurn returns — never across the whole turn — and it is advisory, so it serializes tell-me-go writers only (PLUR's own writes don't take it; the documented last-write-wins acceptance for genuinely concurrent writers still stands). Worst-case AfterTurn stall from lock contention is bounded by the ~500ms acquisition budget, and the whole hook invocation is capped by the detached 3s ctx — so a locked or hung PLUR store can never stall turn completion beyond that bound, and typically the lock is uncontended given the sequential-orchestration backstop.

All three recommendations accepted; the one substantive deviation from the issue text (branch (ii) !IsTransient filter) is a strengthening, verified against errors.Is unwrap behavior, and will be recorded in the ADR corrections table.

Q3 — Griller

Q3: In the configuration and DI wiring plan, MEMORY: introduces both session-fixed parameters (SERVER) and hot-reloadable limits (ENABLED, INJECT_BUDGET, LEARN, SCOPE, MAX_LEARNS_PER_SESSION). Looking at internal/infrastructure/di/container.go, internal/app/chatter.go, and internal/agent/agent.go, three wiring questions emerge:

  1. MCP Client Retrieval in DI: mcpFactory is owned by toolchainFactory and builds clients during registry initialization. Through which domain port or interface method on ports.ChatterComposer does app.NewChatter retrieve the tools.MCPClient for MEMORY.SERVER to inject into agent.NewAgent, ensuring the application layer does not import infrastructure?
  2. Enabled-but-Absent Server Fallback: If MEMORY.ENABLED: true but MEMORY.SERVER is not in MCP_SERVERS or its initialization failed in mcpFactory.Build, at what lifecycle stage is the "warn and disable" fallback executed, and what is the exact state of plurInjector and plurHook?
  3. Hot-Reload Plumbing & ADR-029 Alignment: ADR-029 enforces a strict three-step fail-fast delegate chain in agent.applyConfig: publishConfigUpdate → reconfigureEngine → reconfigureContextManager. How is MemoryConfig reloaded by ConfigWatcher, and how are updated memory settings propagated to plurInjector and plurHook without violating ADR-029's contractual delegate sequence?

Recommendation:

  1. Port Extension: Add GetMCPClient(name string) (tools.MCPClient, bool) (or expose via sessionDeps) on ports.ChatterComposer. app.NewChatter reads cfg.Memory.Server, queries deps.GetMCPClient(serverName), and passes the client (or nil if missing) via an AgentOption (e.g. agent.WithMemoryClient(client, memCfg)).
  2. Deterministic Startup Validation:
    • At config validation (Config.ValidateMemory), if ENABLED == true and SERVER is not in MCP_SERVERS, log a warning and set ENABLED = false in the resolved config (graceful fallback).
    • If the client fails construction in mcpFactory.Build, deps.GetMCPClient returns nil, false; agent.initComponents logs slog.Warn("memory_server_unavailable", "server", serverName) and constructs no-op memory components with ENABLED = false.
  3. Atomic Pointer Sharing:
    • Extend domain_config.ConfigWatcher with GetMemoryConfig() domain_config.MemoryConfig.
    • In agent.prepareRuntimeConfig(), read newCfg.Memory = a.configWatcher.GetMemoryConfig().
    • Share a single *atomic.Pointer[domain_config.MemoryConfig] (or read from the agent's atomic runtimeConfig) across agent, plurInjector, and plurHook.
    • Transform and AfterTurn read the pointer lock-free on every turn. This requires zero modifications to ADR-029's delegate ordering in applyConfig.

A3 — Subject (Architect)

Q3 answer. Verified the wiring end-to-end; the recommendation is sound with two precisions (ChatterConfig seam and the nil-client runtime guard).

1. MCP client retrieval — the port must be extended; the clients exist but are not retained today. ports.ChatterComposer (internal/domain/ports/session.go:106) has no MCP accessor — its surface is GetGateway/GetEventBus/GetPaths/GetHistoryManager/GetLogger/GetTracker/GetPricingOverrides/GetSessionProvider/GetTurnsLogger/GetSecurityManager/GetRegistry/GetSkillRepository/GetSummarizer/GetConfigWatcher/RegisterTrace. app.NewChatter (internal/app/chatter.go) imports only agent, ports, domain/skills — it must stay infrastructure-free, so the client must arrive through a domain-typed accessor, and tools.MCPClient is a domain port (internal/domain/tools/mcp_client.go). Verified where the clients actually live: mcpFactory.Build(servers) map[string]plugin.MCPServerDependency (internal/infrastructure/di/mcp_factory.go:91), owned by defaultToolchainFactory.mcpFactory (toolchain_factory.go:63,75), passed as regParams.MCPClients into tool registration (toolchain_factory.go:82,101) — and not retained after registration; only mcpFactory survives for CloseMCPClients(). MCPServerDependency wraps Client tools.MCPClient (internal/tools/integrations/plugin/plugin.go:40), so the unwrap is trivial. The concrete plan: (a) defaultToolchainFactory.BuildRegistry stashes the built mcpClients map on the factory; (b) add GetMCPClient(name string) (tools.MCPClient, bool) to ports.ChatterComposer, implemented on sessionDeps (var _ ports.ChatterComposer = (*sessionDeps)(nil) in container.go) by unwrapping dep.Client; (c) app.NewChatter calls deps.GetRegistry() first — it already does, and the registry build is what constructs the clients (wireToolRegistry → lazyRegistry → BuildRegistry, container.go) — then deps.GetMCPClient(cfg.Memory.Server) and threads it via agent.WithMemoryClient(...). One precision on the seam: ports.ChatterConfig (session.go) currently carries only ProviderName/Model/Mode/LogPath/TracePath/ConfigPath — it has no Memory field, and NewChatter cannot read GetMemoryConfig() from the watcher before the first Refresh (the watcher is only SetPaths-ed there). So extend ports.ChatterConfig with the resolved memory server key (threaded from Config.Memory.Server by the chat factory, exactly as ProviderName/Model/Mode are already threaded) — deterministic, no refresh-ordering coupling.

2. Enabled-but-absent fallback — two lifecycle stages, and the components exist but are inert. Stage 1 (static, config layer): MemoryConfig.validate() — the MCPServerConfig.validate(name) precedent at internal/domain/config/mcp_config.go:103 — checks ENABLED && SERVER ∉ MCP_SERVERS → warn + set ENABLED = false in the resolved config. This executes at config load and again on every hot-reload re-parse (the watcher's Refresh path), so it is deterministic and idempotent; it is the config-valid-provider invariant pattern (selectedProvider must reference a PROVIDERS key). Stage 2 (dynamic, construction): mcpFactory.Build skips servers whose client construction or token resolution fails — verified by TestMCPFactory_ClientInitFailureSkips and TestMCPFactory_TokenResolutionFailureSkips (mcp_factory_test.go) — so the map can lack MEMORY.SERVER even when static validation passed. Then deps.GetMCPClient returns (nil, false), and agent.initComponents logs slog.Warn("memory_server_unavailable", "server", name) and constructs the memory components with effective ENABLED = false. The exact state: plurInjector and plurHook are still constructed (stable DI shape — the injector is registered in Factory.Extras, the hook wired into NewEngine opts in initComponents) but behave as no-ops: the injector's Transform takes the enabled == false content-driven path (scan for sentinel; nothing to strip on a fresh session → return), and the hook's tiers read disabled → no episodes, no capture, no learn. One refinement: because ENABLED is hot-reloadable while the client binding is DI-fixed, the atomic can later read enabled == true with a nil client — so Transform/AfterTurn need a nil-client guard that logs and returns (fail-open no-op), i.e., "disabled by construction" is enforced at the component, not just at config resolution.

3. Hot-reload — atomic-pointer sharing, zero ADR-029 changes. Verified: ConfigWatcher (internal/domain/config/watcher.go) exposes SetPaths/Refresh(model)/SetLimits/GetLimits/GetContextWindow/ApplyLimits — extend it with GetMemoryConfig() domain_config.MemoryConfig. The interface's own doc comment pins the constraint: Refresh must stay void and the fail-fast chain is scoped to exactly SafePublish → Engine.Reconfigure → Manager.Reconfigure — so a getter is consistent; we are not adding a fourth delegate. In agent.prepareRuntimeConfig() (agent.go) — which already does a.configWatcher.Refresh(oldCfg.Model), reads GetLimits(), and stores newCfg into a.config — add newCfg.Memory = a.configWatcher.GetMemoryConfig(), and add a Memory domain_config.MemoryConfig field to runtimeConfig. Sharing: plurInjector/plurHook read memory settings lock-free per turn from the same atomic — either the agent's existing config atomic.Pointer[runtimeConfig] (a.config.Load().Memory) or a dedicated *atomic.Pointer[MemoryConfig] stored in prepareRuntimeConfig. The ADR-029 alignment is guaranteed by ordering I verified in applyConfig: cfg := a.prepareRuntimeConfig() runs before the three delegates, so the memory atomic is refreshed first, then publishConfigUpdate → reconfigureEngine → reconfigureContextManager execute unchanged, in order, fail-fast. Memory propagation is a pre-chain atomic refresh exactly like Limits already are — no contract change, no reorder.

All three recommendations accepted, with the two recorded precisions: (i) ports.ChatterConfig gains the memory server key (the deterministic seam, mirroring ProviderName/Model/Mode), and (ii) the nil-client runtime guard so a later hot-reload ENABLED=true with a DI-fixed nil client degrades to fail-open rather than panicking or half-wiring.

Q4 — Griller

Q4: Regarding the domain model updates, architectural governance (ADR-068), and repository quality gates (modelith-layers, verify-mock-pattern, verify-mcp-sdk-confinement, verify-adr-index):

  1. Domain Model & Type Alignment: When adding the Memory entity, MemoryLearnTier enum, 4 invariants, and the new scenario to docs/domain-model/tell-me-go.modelith.yaml, how will code-to-model alignment be satisfied for make modelith-layers and make modelith-drift (given that MemoryConfig lives in internal/domain/config/memory_config.go, similar to Provider → LLMProvider)?
  2. ADR-068 Governance: What is the canonical filename and index entry for the new ADR in docs/adr/README.md to satisfy verify-adr-index, and what essential sections must it codify?
  3. Quality Gate Confinement for Tests: verify-mock-pattern enforces zero tolerance for testify/mock, and verify-mcp-sdk-confinement strictly forbids importing github.com/modelcontextprotocol/go-sdk outside internal/infrastructure/mcp/. How will unit tests for internal/agent/memory (plurInjector, plurHook) and DI wiring mock the tools.MCPClient domain port?

Recommendation:

  1. Modelith Alignment:
    • Define type MemoryConfig struct and type MemoryLearnTier string in internal/domain/config/memory_config.go.
    • In scripts/modelith-layers.sh, document the Memory exception (Memory|MemoryConfig lives in config; model documents the concept), exactly matching the established precedent for Provider (Provider|LLMProvider lives in config; model documents the concept).
    • Run make modelith-render to regenerate tell-me-go.modelith.md so make modelith-check passes cleanly.
  2. ADR-068 Indexing:
    • Create docs/adr/2026-09-automatic-plur-memory-integration.md (ADR-068) and add the entry to docs/adr/README.md.
    • ADR-068 must record: (i) the Seam A / Seam B architecture, (ii) the corrections table, (iii) the failure/disable matrix, (iv) the trust-class analysis, (v) the multi-persona concurrency stance (advisory flock + sequential orchestration + last-write-wins acceptance), and (vi) the v1.1 additive follow-ups.
  3. Hand-Rolled Mocks & Pure Domain Dependencies:
    • In internal/domain/tools/toolstest/ (or package-local _test.go), provide a hand-rolled function-field mock implementing tools.MCPClient (e.g. MockMCPClient{ListToolsFunc, CallToolFunc, CloseFunc}), matching ADR-021.
    • internal/agent/memory and its tests must depend only on tools.MCPClient (domain port) and never import the third-party MCP SDK or use time.Sleep, ensuring verify-mock-pattern, verify-no-test-sleep, and verify-mcp-sdk-confinement pass without exceptions.

A4 — Subject (Architect)

Q4 answer. Verified against the scripts, the Makefile gates, and the ADR index; the recommendation is accepted with one precision the griller's list missed (the MemoryLearnTier enum is also caught by the layers script).

1. Modelith alignment — the Provider precedent is verbatim, and there's a second name to silence. scripts/modelith-layers.sh embeds the documented exceptions heredoc, which contains exactly: Provider|LLMProvider lives in config; model documents the concept — the griller's cited precedent is accurate. Add Memory|MemoryConfig lives in config; model documents the concept to that heredoc. I verified the mechanics: the script extracts 2-space PascalCase keys and greps type <Entity> struct|interface under internal/; type MemoryConfig struct in internal/domain/config/memory_config.go does not match type Memory struct (the regex requires a space after the entity name), so Memory without the exception would report "no struct/interface found" (missing). Precision: MemoryLearnTier is also extracted by the same entity regex (the enums: keys sit at the same 2-space indent), and no type MemoryLearnTier struct|interface will exist (it's a string type) — so it needs handling too. The established precedent for enums is the script's own exclusion list: grep -v '^ProviderType$' | grep -v '^LLMError$' | grep -v '^ToolCategory$' | grep -v '^APIFamily$' — add MemoryLearnTier there (mirroring the four existing enums), or a second exception entry. Both keep the report clean. Note the gate is advisory (the script exit 0s unconditionally; the Makefile marks modelith-layers "Advisory only") — so "clean" means no new warnings beyond baseline (the baseline already reports MCPServer as missing — I verified no type MCPServer struct|interface exists anywhere in internal/), and the acceptance criterion is met by adding the exception rather than by any hard gate. modelith-drift is likewise advisory (exit 0, "Never fail the build"): MemoryConfig and MemoryLearnTier are absorbed by the entity-suffix rule (*Memory* matches the modeled entity), plurInjector/plurHook are unexported (the drift grep only matches ^+type [A-Z]), and mocks are Mock*-skip-listed — so drift stays clean with zero script edits. modelith-check is the one hard gate: commit .yaml + re-rendered .md together (domainmodel-md-generated invariant), make modelith-render then make modelith-check.

2. ADR-068 governance. Canonical filename: docs/adr/2026-09-automatic-plur-memory-integration.md — I verified no 2026-09 ADR file exists in docs/adr/, and the name matches the SOP convention YYYY-MM-short-descriptive-title.md (docs/sop/standards/adr_standards.md §2). verify-adr-index (Makefile) has exactly two requirements: (a) the basename appears in docs/adr/README.md (add a table row | **ADR-068** | Automatic PLUR Memory Integration | 2026-09 | Accepted | [2026-09-automatic-plur-memory-integration.md](...) |), and (b) the # ADR-068: ... heading is unique across docs/adr/*.md (the gate greps ^# ADR-[0-9]*: and uniq -d). Required sections per the SOP §3: Status, Context, Decision, Consequences — the issue's mandated content maps into it: (i) Seam A/Seam B architecture in Context/Decision; (ii) the corrections table (the grill's 9 resolved premises, plus our two grill-round additions: the branch-(ii) !IsTransient filter from Q2 and the ports.ChatterConfig memory-server-key seam from Q3); (iii) the failure/disable matrix (available→inject latest / error+timeout→strip-no-persist / disabled→one-shot persisted strip); (iv) the trust-class analysis (system-prompt-positioned auto-written external text, same class as skill injection minus the install-approval gate, bounded by local-only store + default-off + LEARN gating + relevance gate); (v) the multi-persona concurrency stance (advisory flock + sequential-orchestration backstop + last-write-wins acceptance); (vi) the v1.1 follow-ups (additive ContextRequest.SessionID, per-persona scope separation via plur_rescope, optional pinned: true). One addition beyond the griller's list: per the SOP §5 workflow, also link ADR-068 in the main README.md "Design Decisions" section (it currently lists ADR-053..067) — verify-adr-index doesn't check README.md, but the SOP does.

3. Test confinement — hand-rolled mocks, zero gate exceptions. Verified the three gates: verify-mock-pattern greps for "github.com/stretchr/testify/mock" in internal/ files under a /[^/]*test/ path (zero tolerance) — a hand-rolled function-field mock has no such import, so it passes anywhere; verify-mcp-sdk-confinement greps for github.com/modelcontextprotocol/go-sdk outside internal/infrastructure/mcp/ (production and test files) — the memory package and its tests must import only the domain port tools.MCPClient (internal/domain/tools/mcp_client.go), never the SDK; verify-no-test-sleep forbids time.Sleep for synchronization — memory tests must use deterministic control (inject pkg/clock.Clock for any timing, including the flock acquisition poll — implement the poll as a bounded attempt count on an injected clock, not wall-clock sleeps; ready channels/poll loops for concurrency, per ADR-036). Concrete mock: MockMCPClient{ListToolsFunc, CallToolFunc, CloseFunc} implementing tools.MCPClient, package-local in internal/agent/memory/ — this matches the established precedent (testMCPClient in internal/domain/tools/mcp_client_test.go, fakeMCPClient in internal/infrastructure/di/mcp_factory_test.go, mockMCPClient in internal/tools/integrations/mcp/plugin_test.go); there is no canonical shared MCPClient mock today, so creating one is not a duplication violation. If the DI wiring test (internal/infrastructure/di) needs the same double, either reuse the existing di fakeMCPClient or export the mock from a new internal/agent/memory/memorytest/ sub-package (ADR-021 pattern — <pkg>/<pkg>test/, and the test-double dirs are already excluded from coverage); both are gate-clean. verify-testutil-convention is satisfied either way (no testutil package). No exceptions or whitelist edits to any gate are required — that is the point of routing everything through the tools.MCPClient domain port.

All three recommendations accepted; recorded precisions: (i) MemoryLearnTier must join the layers script's enum exclusion list (or get its own exception) or the "clean" report gains a missing-entity line, and (ii) ADR-068 also needs the main README Design Decisions link per the ADR SOP §5.

Q5 — Griller

Q5: For the final question, let's examine the test strategy and E2E verification across legs (i) and (ii):

  1. Offline & Network Independence in CI (make test / make check-full): In CI or restricted developer environments, npx or external network access for @plur-ai/mcp may be unavailable. How is the test suite partitioned between deterministic in-repo tests (unit, integration, and tests/e2e/) and live PLUR environment testing, ensuring make check-full is 100% self-contained and reproducible offline?
  2. Observability Contract for injected_ids: The acceptance criteria require asserting that injected_ids contains the expected engram ID for both leg (i) and leg (ii). What is the exact programmatic and observable contract for surfacing injected_ids (e.g. ContextMetadata.Warnings, TraceEvent, or debug logging) across both in-process integration tests and black-box CLI executions?
  3. Mocking & Concurrency in test-race: When running package-by-package race detection (make test-race), how will the hand-rolled mock MCP client in internal/agent/memory and tests/integration/ handle concurrent tool calls, async telemetry, and FlushSession lifecycle without race conditions?

Recommendation:

  1. Self-Contained In-Repo Tests:
    • In-repo unit and integration tests (internal/agent/memory, tests/integration/agent/, and tests/e2e/) must use local mock MCP servers (e.g. httptest.Server or hand-rolled tools.MCPClient doubles) with zero reliance on npx or internet access.
    • The live PLUR two-legged E2E (against real @plur-ai/mcp and ~/.plur/) should be documented as an environment verification procedure (with an optional -tags=e2e_live test skipped by default).
  2. Dual Observability via ContextMetadata and Events/Logs:
    • plurInjector records injected IDs in ContextRequest.Metadata.Warnings (e.g. fmt.Sprintf("injected_engrams:%s", strings.Join(ids, ","))) and emits an slog.Debug("memory_injected", "ids", ids).
    • In black-box CLI tests, telemetry/trace events or -t session logs surface the injection deterministically.
  3. Thread-Safe Mock Primitives for test-race:
    • The mock tools.MCPClient in memorytest must protect its call logs and handlers with a sync.Mutex (invoking injected handler funcs outside the mutex to prevent non-reentrant deadlocks, following the repo's ADR-036 concurrency standard).
    • FlushSession and AfterTurn tests must use synchronized channels or fake clocks (pkg/clock.FakeClock) to deterministically assert hook completions without sleeping.

A5 — Subject (Architect)

Q5 answer. Verified the test partition, the observability surfaces, and the concurrency requirements against the tree; one finding corrects the recommendation (TurnTrace carries no warnings today).

1. Offline partition — make test/make check-full is already 100% self-contained; the live legs go behind a build tag. Verified: make test = the verify-* convention gates + go test ./...; make check-full adds package-by-package -race (180s timeout per package) — neither passes build tags, so any //go:build e2e_live file is excluded by default and check-full stays offline. The existing suite proves the self-contained pattern: tests/e2e builds the real cmd/tell-me-go binary (buildE2EBinary in e2e_test.go) and drives it against local httptest.Server mock LLM providers with GEMINI_API_KEY=dummy, TELL_ME_FAST_RETRY=1, and a SELECTED_PROVIDER: "mock" temp config; the harness filters ambient TELL_ME_* env vars; assertions read stdout+stderr (runAgentStep) and the session files. The only "npx" strings in tests are IsStdio literals in mcp_config_test.go — nothing spawns npx or touches the network; MCP stdio integration tests spawn the locally-compiled testdata/helper binary. So the partition is: (a) in-repo deterministic — unit tests in internal/agent/memory with a hand-rolled MockMCPClient, integration via tests/integration/agent (established agenttest mocks), and an in-repo E2E leg that drives the real binary against a local fake plur MCP server (a httptest.Server speaking MCP JSON-RPC, or a local stdio helper binary implementing plur_inject_hybrid/plur_capture/plur_learn_batch with canned responses) — this verifies the full Seam A/Seam B plumbing offline; (b) live PLUR environment verification — the two-legged E2E from the acceptance criteria (real @plur-ai/mcp, real ~/.plur/ store and retrieval index, out-of-band pin setup), gated behind -tags=e2e_live, skipped by default, documented as an environment procedure. The build-tag mechanism has an in-repo precedent: the -tags=arch gates in the Makefile (verify-architecture, verify-transitive-gate, verify-ports-registry). Only the live legs — leg (ii)(a) "persistence through the real retrieval index" and leg (ii)(b) — require the real store; leg (i) plumbing and the injection/learning mechanics are fully exercisable against the fake.

2. Observability contract — split by surface, with one additive change required. In-process: ContextMetadata.Warnings is the primary, verified contract — it is appendable ("Consumers may append to this slice", internal/agent/session/context/contracts.go) and it survives: Manager.Prepare returns &req.Metadata, ContextRefiner stores it as Turn.State.Metadata (engine_phases.go). So plurInjector appends fmt.Sprintf("injected_engrams:%s", strings.Join(ids, ",")) and in-process integration tests assert on Turn.State.Metadata.Warnings after ExecuteTurn — no new machinery. Black-box CLI: the griller's "trace events" surface needs a correction — I verified telemetry.TurnTrace (internal/domain/telemetry/trace.go) has only StartTime/EndTime/InferenceDuration/ToolExecutions/FinalStatus; it does not carry warnings or metadata today, so injected_ids does not currently flow into TraceEvent (which ExecuteTurn publishes per turn, engine.go). The deterministic black-box surfaces available today are (a) the persisted history file (when a sibling canonical transformer sets PersistHistory, the injected block — containing the engram text — lands in history.jsonl; black-box can assert the ## PLUR MEMORY block), and (b) stdout/stderr log lines (the harness's runAgentStep asserts on combined output; the [Tool Action] list_files precedent proves stderr logs are asserted). Recommendation: make the observability contract explicit and additive — (i) append to ContextMetadata.Warnings (in-process, primary); (ii) add an additive Warnings []string (or InjectedEngrams []string) field to telemetry.TurnTrace, populated from ContextMetadata at finalizeTurnTrace, so TraceEvent consumers and session traces surface it — a contract touch-point in the trace package, gate-clean; (iii) emit the ids in the injector's log line at a level the E2E harness captures (Info, not Debug — Debug may be suppressed), so the live CLI legs can assert via stderr per the established pattern. Leg (i) asserts injected_ids via in-process metadata (integration) or the history file/trace (CLI); leg (ii)(a) asserts via the real retrieval index (live-only); leg (ii)(b) remains best-effort with the one documented re-run allowance.

3. Race-clean mocking and lifecycle. The mock must follow the repo's documented ADR-036 concurrency pattern — verified as an in-repo precedent (the tmg-issue-to-pr troubleshooting entry pins it: "lock → append → unlock; invoke user Funcs OUTSIDE the lock — non-reentrant deadlock"). So MockMCPClient{ListToolsFunc, CallToolFunc, CloseFunc} in internal/agent/memory/ (or memorytest/) guards its call log and recorded args with a sync.Mutex, appends under the lock, and invokes the injected handler functions after unlocking — so a handler that itself calls back into the mock (or asserts on the call log) cannot deadlock. The hook's own state must be equally race-safe: the per-SessionID ring buffer map guarded by its own mutex; FlushSession drains under lock then issues the MCP plur_learn_batch calls outside the lock; the memory config is the atomic.Pointer[MemoryConfig] from Q3 — lock-free reads in Transform/AfterTurn are race-clean by construction. Concurrency reality check: AfterTurn is synchronous per ExecuteTurn, and FlushSession is a defer in Chat — sequential within one Chat; the race surface is the agent's repeated/concurrent-Chat contract (agent tests), which the mutex + atomic cover. Deterministic timing: inject pkg/clock.FakeClock/FakeTicker (verified in internal/pkg/clock/clock.go:68-78) for the flock-acquisition poll budget and any timeout logic — the poll becomes a bounded attempt count on the injected clock, not wall-clock sleeps; completion assertions use ready channels, never time.Sleep (the verify-no-test-sleep gate is zero-tolerance for internal/ui/ and allow-listed elsewhere, and memory tests must use none). test-race runs package-by-package with -race — the memory package, tests/integration/agent, and the new E2E legs must all be race-clean, which the mutex/atomic/channel discipline guarantees.

All three recommendations accepted with one recorded correction: the black-box injected_ids surface is not TraceEvent as-is — TurnTrace needs the additive warnings field (or the assertion moves to the history file / captured stderr log), and I've specified both so the E2E legs have a deterministic, verified surface. That closes the round: my position stands on the two-seam design, the content-driven strip and classification mechanics, the ADR-029-compliant atomic config sharing, the modelith/ADR/gate alignment, and a fully offline make check-full with the live PLUR verification cleanly gated behind -tags=e2e_live.

Verdict — Griller

VERDICT: The subject's two-seam architecture (plurInjector ContextTransformer + plurHook TurnHook) held up exceptionally well against the repository's concrete lifecycle, concurrency, and layering constraints. The grilling tightened several critical operational edges: establishing a stateless content-driven strip on disable, bounding hook MCP calls with detached contexts and a !IsTransient error filter, adding explicit ports.ChatterComposer/ChatterConfig DI retrieval seams, and documenting Memory in the modelith layer gates. I recommend proceed with changes incorporating these agreed refinements into ADR-068 and the implementation.

Post-Grill Position — Subject (Architect)

POST-GRILL POSITION: I stand by the core architecture in full — the two-seam design (plurInjector as a priority-15 marker-keyed replace-in-place ContextTransformer registered in Factory.Extras at initComponents; plurHook as a once-at-init TurnHook with the three-way episode classification keyed on the hook's err argument, never Turn.State.LastError), fetch-per-turn with no v1 cache (the eager stdio spawn makes every call a warm roundtrip), fail-open with strip semantics so the invariant is "current recall or nothing, never stale recall", the default-disabled opt-in config surface riding the existing watcher → configRefreshHook.OnPhaseTransition → applyConfig hot-reload path with zero modification to ADR-029's contractual delegate chain, and the multi-persona concurrency stance (documented last-write-wins + advisory flock + sequential-orchestration backstop). Every load-bearing claim I made was verified against the tree during the round — clear-after-append, the non-stable sort.Slice priorities, the per-Chat WithEngineHook double-registration hazard, the empty-response-retry staleness of LastError, the canonical/transient persistence partition, and the offline self-containment of the existing test suite — and none of those verifications overturned the design.

As a result of the grill I am adopting five revisions, all recorded for the ADR-068 corrections table: (1) the injector is fully stateless — strip-on-disable is content-driven (sentinel presence, not transition tracking), which is multi-pass-Prepare-resilient and self-healing on persistence failure; (2) all hook MCP calls run on a detached bounded context (~3s) because AfterTurn is synchronously on ExecuteTurn's return path, and branch (ii) gains the !IsTransient(err) filter — a genuine strengthening, since errors.Is unwraps the "max retries reached" wrap so retry-exhaustion-on-transient errors would otherwise pollute episodes; (3) explicit DI seams: GetMCPClient(name) on ports.ChatterComposer (domain-typed, app stays infrastructure-free), the memory server key added to ports.ChatterConfig, and a nil-client runtime guard so a later hot-reload ENABLED=true with a DI-fixed nil client degrades to fail-open; (4) modelith-layers needs both Memory and MemoryLearnTier handled (exception entry + enum exclusion list), ADR-068 is docs/adr/2026-09-automatic-plur-memory-integration.md indexed in docs/adr/README.md and linked in the main README; (5) the observability contract is split: ContextMetadata.Warnings in-process, an additive warnings field on telemetry.TurnTrace for TraceEvent consumers, and an Info-level log line for the black-box CLI legs, with the live PLUR verification gated behind -tags=e2e_live so make check-full stays 100% offline.

Open questions I take into implementation rather than treating as settled: whether the !IsTransient filter should keep a debug-level record of skipped error episodes (my lean: yes, for observability parity with fail-open logging); whether TurnTrace gains a general Warnings slice or a dedicated InjectedEngrams field (a contract decision for the trace package — I lean general Warnings, populated from ContextMetadata, so future transformers reuse it); and whether the pre-existing 110/110 priority collision between HistoryPruner and finalContextValidator warrants an ADR note given the issue's "distinct priorities are mandatory" framing — it is out of scope but should be acknowledged so it is not mistaken for a new violation. None of these block the proceed-with-changes verdict; they are the three decisions I will surface to the implementer before ADR-068 is finalized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment