Skip to content

Instantly share code, notes, and snippets.

@gosharplite

gosharplite/issue-1409-grill.md

Created August 19, 2026 12:40
Show Gist options

  • Select an option

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

Select an option

Learn more about clone URLs
Save gosharplite/435e78007f76d46ea97db5dbe4363edf to your computer and use it in GitHub Desktop.
Download ZIP
Grill round on tell-me-go issue #1409 (plurHook wire contract vs @plur-ai/mcp schemas) — Architect vs Griller, 10 questions
Raw
issue-1409-grill.md

Grill Round — Issue #1409: plurHook wire contract vs real @plur-ai/mcp schemas

Round: 10 questions · Subject: Architect · Griller: Griller · Orchestrator: butler Date: 2026-08-19 · Repo: gosharplite/tell-me-go

Opening Statement — SUBJECT (Architect)

[MODEL] I have everything I need — bootstrap complete, issue fetched, implementation, tests, fake server, and wiring all verified, and the real @plur-ai/mcp schemas confirmed against the live MCP server connected to this environment. Here is my opening statement.

UNDERSTANDING — Issue #1409: plurHook wire contract does not match the real @plur-ai/mcp tool schemas

Diagnosis (confirmed against the live server schemas): The ADR-068 Seam B learn path ships payloads that fail server-side validation on every write tier. Three concrete mismatches, all in internal/agent/memory/:

  1. plur_capture (hook.go:172-196): the hook sends {agent: turn.Mode, session_id: turn.SessionID, text?, error?, prompt?} (hook.go:176-182). The real tool requires summary (no default, cannot even be invoked without it) and its optional params are agent, session_id, tags, channel — text/error/prompt are not parameters at all. Missing required summary → rejected.
  2. plur_learn_batch (hook.go:271-309): FlushSession sends {episodes: [...], session_id: sessionID} (hook.go:298-299), where each item is the episode struct whose JSON tags (buffer.go:22-27: text, error, prompt, agent, session_id, timestamp) literally define the (wrong) wire contract — the file's own comment at buffer.go:21 admits "T7's fake plur server mirrors them." The real tool requires engrams[], each with statement; there is no episodes key, no top-level session_id, and no item carries statement. Rejected on both the key name and every item.
  3. plur_learn (hook.go:248-263): maybeLearn sends {statement, agent: turn.Mode, scope?} (hook.go:249-252). The real tool has no agent parameter (it does have statement, scope, plus optional tags/source/type/commitment/domain/session_id). The unknown agent key breaks the call; and because full also appends to the ring buffer (hook.go:256-257), every full-tier session still ends with the failing plur_learn_batch flush too.

Seam A is unaffected — plur_inject_hybrid's real params (task, budget, scope) match what injector.go sends (injector.go:113-119), which is why production shows injections every turn and zero learn-path events (plur_history shows only co_injection events; plur_receipt flat at 10 engrams).

Why the tests did not catch it: every test double accepts the hook's own payload shape — a circular contract. (a) hook_test.go asserts against mockMCPClient (memory_test.go:28-60), a recorder that performs zero schema validation — TestHookCaptureBranchI even asserts the wrong keys (agent/session_id/text) as correct. (b) The fake server tests/e2e/testdata/fakeplur/main.go mirrors the same wrong contract: its capture handler reads text/error/prompt/agent/session_id (fakeplur capture()), its learnBatch reads args["episodes"], and its toolDefinitions() advertises inputSchema: {"type": "object"} with no properties — no validation exists to catch a mismatch. (c) The live leg memory_live_test.go exists but is -tags=e2e_live, best-effort log-and-pass, and only exercises injection (liveMemoryE2EConfig hardcodes LEARN: "off" at memory_live_test.go:32) — it never runs one capture or one batch flush against a real server, so the write path was never validated against ground truth before shipping.

Blast radius: all three non-off tiers are dead in practice — capture (per-turn episodes rejected), batch (session-end flush rejected; this is the default tier per ADR-068 §3, so the default-enabled config is broken), and full (gated learns rejected on the unknown agent param plus the same broken batch flush). Only off behaves as documented. It is invisible by design: fail-open (ADR-029 §5) logs memory_capture_failed / memory_learn_batch_failed / memory_learn_failed at Warn to stderr only — nothing in the UI, turns.log, or PLUR itself — so a correctly configured user sees injections working and reasonably assumes learning works.

FIX PLAN

  1. Align the wire contract with the real schemas in internal/agent/memory/:
    • capture() → plur_capture: send summary = ep.Text when non-empty, else "error: " + ep.Error (optionally appending prompt) — summary is required, so branch (ii) error episodes still produce a timeline entry; keep agent and session_id (real optional params); drop text/error/prompt.
    • FlushSession() → plur_learn_batch: send engrams: [{statement: <ep.Text>, ...}]; drop the top-level session_id; carry session identity via tags (e.g. session:<id>, mode:<mode>); skip items with empty text — error-only episodes produce no engram, consistent with ADR-068 §2(ii) "no learning from this branch" — so the buffer's count bound needs a small adjustment (evict or skip on mapping, not on raw append).
    • maybeLearn() → plur_learn: drop the agent parameter; keep statement + scope.
    • Update buffer.go's episode doc comment (the "wire contract" claim is false and is the seed of the circular test) — the struct becomes an internal model with an explicit mapping to wire shapes.
  2. Break the circular fake contract: make fakeplur's toolDefinitions() declare real inputSchema properties (summary required; engrams[] with statement required) and have handleToolCall reject missing required params — so a payload mismatch fails the offline E2E loudly instead of passing. Add schema-driven/golden unit tests in internal/agent/memory/ built from the real tool definitions (not from the hook's own args), pinning the exact key sets per call site.
  3. Add a live E2E leg behind -tags=e2e_live: one plur_capture + one FlushSession against a real npx -y @plur-ai/mcp server, asserting the episode lands on plur_timeline and engrams appear in plur_status/plur_receipt.
  4. Louder failure surface: a write-failure counter (or one-time "learning is disabled because writes are failing" notice) so a silently dead learn path cannot ship again — per the issue's fix direction #4.

Acceptance criteria: (a) unit tests assert plur_capture args = {summary, agent, session_id} exactly, plur_learn_batch args = {engrams:[{statement,...}]} exactly with no top-level session_id and no empty statements, plur_learn args = {statement, scope?} with no agent; (b) offline E2E legs still pass against the now-validating fake — they now prove the real-shaped payload persists; (c) the new live leg shows episode + engram counts increasing against a real server; (d) make check-full green (including the CC-pinned entries in INTENTIONAL_NON_FIXES.md — the hook's CC-21 AfterTurn pin and TestHookBatch/TestHookFull test pins must be re-anchored if line numbers shift, and any new mapping function must stay under the CC-10 threshold or be cataloged).

One claim I could not fully verify from the repo alone: whether the real server hard-rejects unknown params (agent on plur_learn) or silently ignores them — the issue asserts rejection and the missing-required-param failures are fatal either way; the fix is identical regardless. I am prepared to defend the empty-text-skip decision for the batch flush against the alternative (converting error episodes into statement = "error: ..." engrams), and the summary-mapping decision for capture-tier error episodes.

Q01 — GRILLER

[MODEL] Verification complete — I confirmed the full diagnosis against the code, the ADR, the fake server, and the live @plur-ai/mcp tool schemas exposed in this environment (which match the issue's table exactly: plur_capture requires summary; plur_learn_batch requires engrams[] each with statement, no top-level session_id; plur_learn has no agent param). The wiring claims also check out: hook registered once at initComponents (agent.go:178), unconditional defer flusher.FlushSession(s.ID) in Chat (agent.go:373-374).

Q1:

Your FlushSession mapping is the load-bearing change of this fix, but as written it pins only statement and defers everything else to ... — you invent tags: [session:<id>, mode:<mode>] for session identity and say nothing about scope. Against the real schema this is where your own reasoning about the wire contract stops short: plur_learn_batch items mirror plur_learn's fields, so per-item session_id and scope are native parameters — no tag convention needed for either. And the scope drop is not cosmetic: MEMORY.SCOPE is honored by Seam A (injector.go sends scope when set) and by the full tier (maybeLearn sends scope), so under your plan a user who configures SCOPE gets it applied to injection and gated learns but silently ignored by the batch flush — the default tier — writing every session's engrams to the store's default scope.

So: what is the complete per-engram mapping for plur_learn_batch — specifically, do you carry the configured MEMORY.SCOPE per engram (native field, zero invention), and on what basis do you replace the native per-item session_id with a searchable-tag convention that no other seam uses and that no consumer of the store can rely on?

A01 — SUBJECT (Architect)

[MODEL] A1 — You're right on scope, and I'll push back with evidence on session_id. The two halves of your claim have different schema support.

Scope: full concession, and it's a real bug in my plan. The plur_learn_batch item schema explicitly enumerates scope (alongside commitment, domain, pinned, rationale, source, statement, tags, type, valid_from, valid_until) — it is a native per-engram field. My plan silently dropped it, which would create exactly the inconsistency you describe: MEMORY.SCOPE applied by Seam A (injector.go:117-119 sends scope when set) and by the full tier (hook.go:252-254, maybeLearn sends scope), but ignored by the batch flush — the default tier — writing every session's engrams to the store's default scope. That's worse than the current bug in one dimension: it would appear to work while mis-scoping data. The fix: FlushSession loads cfg (h.cfg.Load(), a field it already owns but currently doesn't read) and maps scope: cfg.Scope per engram when set; omitted when unset, matching the other seams' omit-when-empty behavior. Zero invention. This also means FlushSession gains a nil-cfg guard for the scope read (fail-open, no scope), consistent with AfterTurn's existing nil-cfg path.

Session_id: the enumerated batch-item schema does not declare it. The properties I can verify on plur_learn_batch items are the eleven above — session_id is not among them (nor are locked_reason or supersedes, which plur_learn does declare). The prose "the other fields mirror plur_learn" is loose language, not a schema declaration; the enumerated properties are the contract, and the issue's own fix direction sanctions exactly my choice: "carry session identity via scope/tags or omit" (#1409, Fix direction 1). So I am not replacing a native field with a convention — the tag is the only carrier available on the batch path that the store guarantees is searchable: the tool schema states tags "contribute to BM25/embedding recall", so plur_recall on session:<id> retrieves the session's engrams — a consumer can rely on it, more reliably than an undeclared key the server may reject or silently drop. And "no other seam uses it" is a schema-constraint difference, not a choice: the injection seam cannot carry session identity at all (ADR-068 correction #8 — dropped as unobtainable at Transform time), while the full tier's plur_learn genuinely does declare native session_id — so maybeLearn should send it natively there, no tag.

Complete per-engram mapping for plur_learn_batch (final):

Field Source Basis
statement ep.Text (required; empty-text episodes skipped — ADR-068 §2(ii) "no learning from this branch") schema-required
scope cfg.Scope when set native item field
tags ["session:<ep.SessionID>", "mode:<ep.Mode>"] native searchable field; only session-identity carrier on this path
dropped text, error, prompt, timestamp, top-level session_id not in schema

One empirical hedge, and I'd fold it into the live-E2E leg: probe whether the real server's runtime validation accepts a per-item session_id despite the schema omission — if it round-trips and plur_history shows it, adopt the native field and delete the tag convention; if it rejects or drops it, the tag stays. Ship against the enumerated schema; upgrade on evidence. I'll also carry native session_id + scope on plur_learn (full tier), dropping only agent there — so the final contract is: plur_capture = {summary, agent, session_id}, plur_learn = {statement, scope?, session_id?}, plur_learn_batch = {engrams:[{statement, scope?, tags}]}.

Q02 — GRILLER

[MODEL] Concession first, verified: I challenged you on per-item session_id for the batch path based on the "mirror plur_learn" prose — you're right, and the enumerated batch-item schema is the contract. I checked it against the live tool definition: the items enumerate commitment, domain, measured_under, pinned, rationale, scope, source, statement, tags, type, valid_from, valid_until — session_id, locked_reason, and supersedes are absent (minor: your count of "eleven" omits measured_under, which is enumerated — the conclusion stands). Tags as the only session-identity carrier on that path, scope per engram, omit-when-empty: agreed.

Q2:

You've silently revised your own acceptance criterion mid-answer. Your opening pinned plur_learn args as {statement, scope?} with no agent; your final contract now adds native session_id there — and that new element is unverified on the exact dimension this issue is about. Per the tool's own semantics, plur_learn's session_id "resolves the session default scope" when no explicit scope is passed — and nothing in the hook or injector ever calls plur_session_start (I read both files in full; there is no session lifecycle anywhere in internal/agent/memory/). So on the full tier's default path (SCOPE unset — the common case), maybeLearn would send a session_id the server must resolve a scope from, for a session it never saw. Your evidence standard is inconsistent: you applied probe-then-adopt to the batch path's undeclared session_id (live leg, adopt on round-trip evidence), but you commit unconditionally to plur_learn's declared one — where the failure mode is scope mis-resolution or rejection, not silent drop. And your new live E2E leg exercises only plur_capture + FlushSession, never plur_learn, so the full tier's new param is never live-verified before shipping.

What is the evidence that an unstarted session's session_id is safe to send on plur_learn — and will you either extend the live leg to cover a full-tier plur_learn call (probe-then-adopt, matching your batch standard), or retract the session_id and keep {statement, scope?} as your opening criterion pinned? My recommendation: retract it — session_id adds a scope-resolution dependency on server state the hook cannot guarantee, for zero recall benefit on the one tier where tags would do the same job.

A02 — SUBJECT (Architect)

[MODEL] A2 — Conceded, and you've caught a genuine inconsistency with a verifiable failure mode. I verified your premise repo-wide before answering: plur_session has zero references anywhere in internal/ (and zero plur_session_start repo-wide) — the hook, the injector, and the wiring never start a server-side session, and no tell-me-go component does. So on the full tier's default path (SCOPE unset), maybeLearn sending session_id would hand the server a session it never saw, and per plur_learn's own semantics ("resolves the session default scope ... when no explicit scope is passed") the server must resolve a scope from it. That failure mode is worse than silent drop — it's scope mis-resolution, the exact class of silent-wrong-memory bug this issue is about, and it would ship on the tier where I claimed to be most careful. My probe-then-adopt standard was applied only to the batch path's undeclared key; I committed unconditionally to the declared one where the risk is higher. That was wrong.

Retracted. plur_learn pins back to {statement, scope?} — my opening criterion — plus tags for identity, which is the honest move on two counts: (1) tags is a declared, searchable field on plur_learn (same BM25/embedding recall guarantee as the batch path), so session/mode identity rides there with zero scope-resolution dependency on server state; (2) it makes the identity carrier uniform across both learn tiers — tags: ["session:<id>", "mode:<mode>"] on plur_learn and on each plur_learn_batch engram — so one convention, one consumer contract, no per-seam divergence. session_id is sent nowhere until a session lifecycle actually exists in tell-me-go (which would be a separate feature, not this fix).

And yes — I extend the live leg, both halves. The corrected evidence standard: every write tier gets live-verified before shipping, not just capture + batch. The -tags=e2e_live suite gains a full-tier leg: one CLI run with LEARN: full and a correction frame, asserting the gated plur_learn lands a real engram in the store (via plur_status/plur_receipt counts or plur_recall on the statement), and asserting no memory_learn_failed Warn in stderr. That closes the exact hole you identified — the full tier's payload never seeing a real server before shipping — and it also empirically settles the residual unknown-param question (whether the server tolerates or rejects anything extra) for the tier that previously had the most unverified surface.

Final pinned wire contract (unchanged from A1's table except the retraction):

Call Payload
plur_capture {summary, agent, session_id} (summary required; agent/session_id are declared grouping fields — no scope semantics)
plur_learn {statement, scope?} + tags: [session:<id>, mode:<mode>] — no agent, no session_id
plur_learn_batch {engrams: [{statement, scope?, tags: [session:<id>, mode:<mode>]}]} — no top-level session_id

Acceptance criteria update: unit tests assert plur_learn args contain exactly statement, scope (when set), tags — and assert session_id and agent are absent; the live suite covers all three tools (capture, learn, learn_batch) behind -tags=e2e_live, each asserting persistence, not just a handshake. make check-full green.

Q03 — GRILLER

[MODEL] Verified before asking: capture() (hook.go) builds its args directly from ep.Text/ep.Error/ep.Prompt with no size bound — truncateToBytes lives only in bufferAppend (buffer.go), which the capture tier never calls; and branch (i) text is joinTextParts(Response), the full multi-part response joined by newline.

Q3:

Your final contract pins plur_capture = {summary, agent, session_id} — exactly three keys — but never defines what summary is, and the capture path is the one write path with no bound. The asymmetry is concrete: batch statements are truncated at maxEpisodeBytes (2000) before flush, while the capture tier — the most frequently fired write call, one per turn — ships joinTextParts(Response) as an unbounded summary on branch (i), and on branch (ii) your opening plan's "optionally appending prompt" was left dangling. Since the contract now forbids a prompt key, prompt content can only survive inside summary — so which is it: is branch (ii) summary "error: " + Error alone, or "error: " + Error with the user prompt folded in? And what byte bound do you apply on the capture path — reusing maxEpisodeBytes, a separate summary cap, or none?

My recommendation: apply the same maxEpisodeBytes bound on the capture path as the batch path (one bound, one contract), and fold the prompt into the error summary when it fits — branch (ii) episodes exist precisely to record what the user was trying to do when inference failed, and the ADR's documented episode shape ({error, prompt, ...}, §2(ii)) includes it; dropping it now would silently narrow the timeline's error context. What's your call, and does the now-validating fake pin it?

A03 — SUBJECT (Architect)

[MODEL] A3 — Agreed on all three: define it, bound it, pin it. The asymmetry is real: capture() (hook.go:172-196) currently ships ep.Text = joinTextParts(turn.State.Response) (branch i) or joinTextParts(GetLastModelTurn) (branch iii) with no truncation at all — the only truncation in the codebase is sessionBuffer.append → truncateToBytes(ep.Text, maxEpisodeBytes) (buffer.go:47), which the capture tier never passes through. So the most frequently fired write (one per turn) is the only unbounded one. That's a store-bloat vector and an inconsistency with the batch path.

My call — summary definition and bound:

  • One bound, one contract: reuse maxEpisodeBytes (2000), applied at the capture call site via a small helper buildCaptureSummary(ep) that wraps the existing truncateToBytes (rune-safe, already written and tested at buffer.go:54). No separate summary cap; the constant gets a doc-comment clarifying it now bounds both write paths.
  • Branch (i)/(iii): summary = truncateToBytes(joinTextParts(...), maxEpisodeBytes) — same text, same bound as a batch engram's statement.
  • Branch (ii): error-first, prompt folded when it fits — precisely, not dangling: base = "error: " + ep.Error; if len(base) > maxEpisodeBytes → summary = truncateToBytes(base, maxEpisodeBytes) (error survives, truncated only if the error itself is pathological); else remaining = maxEpisodeBytes - len(base) - len(" | user: "); summary = base + " | user: " + truncateToBytes(ep.Prompt, remaining). So the error always survives, and the prompt lands whenever it fits — the prompt is the point of a branch-(ii) episode per ADR-068 §2(ii)'s documented {error, prompt, ...} shape: record what the user was trying to do when inference failed. Since the contract forbids a prompt key, folding it into summary is the only carrier, and "when it fits" is now a deterministic, tested rule rather than a dangling option. Your recommendation, adopted verbatim.

Does the now-validating fake pin it? Yes, at two layers, plus one knock-on I have to flag:

  1. Unit layer (pins the bound): new tests in internal/agent/memory/ assert capture()'s args contain exactly summary/agent/session_id, that summary never exceeds maxEpisodeBytes (long-response case, rune-boundary case), and the branch-(ii) matrix: prompt present in summary when it fits, absent when it doesn't, error always present.
  2. Fake layer (pins the schema): fakeplur gains real inputSchema properties and rejects missing summary on plur_capture / missing statement on plur_learn / missing engrams on plur_learn_batch — so a payload with no summary fails the offline E2E loudly.
  3. Knock-on you should know about: the fake's stored episode shape and the batch E2E assertions change with the contract. The fake's capture handler must store summary (not text/error/prompt), and its learnBatch must stop appending to the Episodes section entirely — the real plur_learn_batch creates engrams only; episodes belong to plur_capture. That means TestE2EMemoryLearning_BatchFlushAtSessionEnd (which asserts st.Episodes non-empty and episodeTexts(st) containing the model text, memory_learning_test.go:88-95) must move its assertions to st.Engrams statements — the batch flush now proves persistence via created engrams, not episodes. The episodeTexts helper and plurStoreFile.Episodes field either get repurposed or dropped. This is a deliberate contract correction, not collateral damage — the old assertion was asserting the wrong persistence surface.

Final pinned plur_capture contract: {summary: <bounded>, agent: <mode>, session_id: <sessionID>} — three keys, summary always present and always ≤ maxEpisodeBytes, branch-(ii) carrying error | user: prompt per the fit rule. Acceptance criterion added: capture-path unit tests pin the bound and the branch-(ii) fit matrix; the validating fake rejects a summary-less capture; the batch E2E asserts engrams (not episodes).

Q04 — GRILLER

[MODEL] Harness verified — and it changes the picture. setupMockLLMServer returns a canned, content-only response with no tool calls (skill_injection_test.go:32-51), so a second CLI run can never be relied on to invoke plur_status/plur_recall; the live leg has no FAKE_PLUR_STORE (the real npx server writes to the isolated HOME's ~/.plur/, whose layout is PLUR-internal); and the current live leg observes only stderr strings.

Q4:

You've now twice asserted the live legs prove persistence — "the episode lands on plur_timeline" (opening), "engrams appear in plur_status/plur_receipt counts" (A2) — but no mechanism is specified, and I've verified the existing harness cannot supply one. The mock LLM emits a canned content-only response, so a second CLI run can't be trusted to call the query tools. There's no FAKE_PLUR_STORE on the live path — the real server's ~/.plur/ layout is unverified and internal. And "no memory_learn_failed Warn in stderr" is a handshake assertion, not a persistence one — the same class of surface that let the current leg look green while the write path was dead.

So: what exactly does the live leg read to assert an episode landed and engrams were created? My recommendation: spawn a second stdio MCP client inside the test through internal/infrastructure/mcp (legal within the module — the verify-mcp-sdk-confinement gate forbids direct go-sdk imports, not the internal adapter), call plur_status/plur_timeline/plur_recall against the same npx server before and after the CLI run, and assert counts/entries moved — verifying through the same public contract the hook writes through, rather than a store-format coincidence. What is your mechanism, and does it hard-fail on mismatch when the leg is run?

A04 — SUBJECT (Architect)

[MODEL] A4 — You're right that stderr absence is a handshake assertion, not a persistence assertion — and worse, it's the exact class of surface that hid this bug: in production the memory_capture_failed Warns were firing and nobody saw them. I'm adopting your mechanism wholesale, and I verified it's mechanically available: internal/infrastructure/mcp exports NewStdioClient(cfg config.MCPServerConfig, logger *slog.Logger) (*StdioClient, error) (stdio_client.go:59), implementing the domain tools.MCPClient (ListTools/CallTool) — the same adapter class the DI factory builds for the hook's write path. Importing it from tests/e2e is legal on every gate: Go internal-visibility (same module), verify-mcp-sdk-confinement greps for github.com/modelcontextprotocol/go-sdk imports only (the adapter, not the SDK, is imported), and the tools-layer infrastructure gates explicitly exempt test files. No precedent in tests/e2e today, but no gate forbids it.

The mechanism — read the store through the same public MCP contract the hook writes through, before and after, with content-bearing assertions:

  1. Precondition probes (the only skips): exec.LookPath("npx") absent → t.Skip. Then a throwaway client handshake (ListTools or plur_status) — npx/network failure → t.Skip with a log, matching the existing leg's posture for environment unavailability.
  2. Isolated store: both the CLI run and the verification client run with HOME pointed at a fresh t.TempDir() — the real server's ~/.plur/ store lands there, so the leg never touches the developer's real memory (the existing live leg already establishes this HOME-isolation pattern, memory_live_test.go:45). Residual risk: if the real server ignores HOME and resolves the store elsewhere, the content-bearing assertions below still keep the leg correct — they identify our entries, not counts.
  3. Baseline read: client A = mcp.NewStdioClient(cfg{SERVER: {COMMAND: "npx", ARGS: ["-y", "@plur-ai/mcp"]}}, ...) — the identical config shape the live E2E config uses. Record plur_status engram/episode counts and plur_timeline entries. Close A.
  4. Drive the CLI: runMemoryCLI with LEARN: capture / LEARN: batch / LEARN: full and a unique per-run marker embedded where the test controls it: for the full-tier leg, in the user prompt itself (maybeLearn's statement is lastUserText(PreparedHistory), so the learned engram's statement literally contains marker-<runid>); for capture/batch legs, extend the mock LLM's canned response with an optional marker suffix ([marker-<runid>]) so the episode summary / batch statement carries it. I'll add that marker parameter to setupMockLLMServer — small, additive, and it makes every persistence assertion run-specific rather than count-relative.
  5. After-read (client B, same cfg, same HOME):
    • capture leg: plur_timeline (or status episode count) contains an entry whose summary contains the marker → t.Fatal on absence.
    • batch leg: plur_status engram count increased and plur_recall {query: marker} returns an engram whose statement contains the marker → t.Fatal on either failing.
    • full leg: plur_recall {query: marker} returns the gated plur_learn engram → t.Fatal on absence.
    • Tool-availability tolerance: assert on whichever of plur_status/plur_timeline/plur_recall the live server exposes, but at least one persistence surface must move, content-bearing.
  6. Hard-fail policy: once the environment probes pass, every mismatch is t.Fatal — no log-and-pass anywhere past the precondition stage. The -tags=e2e_live suite keeps its never-in-make check-full posture, but when run, a dead write path fails the leg.

This closes the loop your question exposes: the current leg's "any outcome is logged and passed" would have shipped this bug — the new legs verify through the same tools.MCPClient wire contract the hook writes through (a second StdioClient instance is a second real process speaking the same protocol), so a store-format coincidence can't fake it, and a server-side rejection can't hide behind a missing Warn. One gate caveat I'll verify during implementation rather than assume: verify-architecture's test-file scope — if the arch gate flags tests/e2e → internal/infrastructure/mcp (it shouldn't; the infrastructure gates exempt test files), the legs move to internal/infrastructure/mcp/ with the same harness. Acceptance criterion: go test -tags=e2e_live ./tests/e2e/ -run TestLivePlur -v fails loudly on a broken write path and passes with the corrected contract.

Q05 — GRILLER

[MODEL] Critical finding, verified in both adapters. Client.CallTool (client.go) and StdioClient.CallTool (stdio_client.go) implement the same three-way split: a server-side isError rejection — exactly what the real server does to a summary-less plur_capture — returns ToolResult{Text: <error text>, Error: fmt.Errorf(...)} with a nil Go error. The hook's three write calls all discard the result (_, err := CallTool(...)) and key everything on err != nil.

Q5:

Your diagnosis — and A4 — claim the dead write path was Warn-visible: "logs memory_capture_failed / memory_learn_batch_failed / memory_learn_failed at Warn", "the Warns were firing and nobody saw them". That is unverified and, per the adapter contract I just read, very likely false: a schema rejection surfaces as ToolResult.Error with nil Go error, so the hook never logged a single Warn for the actual failure class — the write path was fully silent, not Warn-silent. And that silently invalidates your fix #4 as designed: a write-failure counter keyed on the write calls' Go errors would count zero for the exact class that shipped this bug, and the live leg's "no memory_learn_failed Warn in stderr" assertion is vacuous for it. (Same blindness in Seam A: the injector checks only err != nil on plur_inject_hybrid, so an isError rejection would build the PLUR MEMORY block from the error text and inject it as recall — violating "inject current recall, or nothing".)

So: what is the failure-detection contract? Specifically — do capture/maybeLearn/FlushSession (and the injector) treat result.Error != nil as a write failure, count it, and surface it; does the now-validating fake reject via the same isError convention the real server uses (its existing unknown-tool path already does, isError: true), so an offline test can prove the counter actually fires on a rejected payload; and what does the counter surface when it fires — given that a Warn on stderr is precisely the surface that failed? My recommendation: detect on err != nil || result.Error != nil, count per session, and surface a session-end aggregate through a channel the user actually sees (turns.log or the CLI's post-turn summary), plus an offline unit test that drives the fake's rejection and asserts the count. What's your call?

A05 — SUBJECT (Architect)

[MODEL] A5 — You've caught a flat error in my diagnosis, and I verified it against the adapter before answering. The write path was fully silent, not Warn-silent — I was wrong. StdioClient.CallTool implements a documented three-way split (stdio_client.go:208-211): an MCP-level tool error (res.IsError) is surfaced as tools.ToolResult{Text, Error: fmt.Errorf(...), Metadata} with a nil Go error; only transport/JSON-RPC failures return a Go error. So the hook's if _, err := h.client.CallTool(...); err != nil never fired for the class that shipped this bug — a schema rejection arrives as result.Error != nil, err == nil, and capture/maybeLearn/FlushSession discard the result entirely (hook.go:196, 263, 309). No Warn ever logged. And you're right about the knock-on: my fix #4 as designed (counter keyed on Go errors) would count zero, and the "no memory_learn_failed Warn in stderr" live-leg assertion is vacuous for the failure class that matters.

Seam A blindness — confirmed, and it's a second real bug: Transform checks only if err != nil on plur_inject_hybrid (injector.go:120-122), then builds block := memoryHeader + "\n\n" + strings.TrimSpace(result.Text) + ... from result.Text — which on an isError rejection is the server's error message (the adapter copies text into the result on the error path). So a rejection injects the error text as system-prompt recall, violating the ADR's "inject current recall, or nothing" invariant. The fake already speaks the convention (its unknown-tool path returns isError: true, fakeplur dispatchTool default + handleToolCall), so this is testable offline today.

The failure-detection contract:

  1. Detect — all four call sites treat err != nil || result.Error != nil as failure: capture, maybeLearn, FlushSession, and Transform. For the injector, result.Error != nil takes the same strip-and-return-nil path as err != nil (injector.go:120-126) — never build a block from an error result.
  2. Count — a per-session write-failure counter on plurHook (failCounts map[string]int, under the existing h.mu), incremented on any detected failure at the three write sites; the count is the session's truth, not a stderr echo.
  3. Surface — two channels, neither of them a bare Warn: (a) in-process per-turn: append memory_write_failures:<n> to the Warnings path ADR-068 §8 already established (ContextMetadata.Warnings → Turn.State.Metadata → TurnTrace.Warnings), which reaches the UI and telemetry; (b) session-end aggregate: extend the optional-behavior seam the agent's Chat defer already uses for FlushSession — the hook exposes the session's failure count to the agent, which logs it at session end in the visible post-turn/session summary, and fires the one-time "learning is disabled because writes are failing" notice when failures == write attempts for the session. A Warn at stderr remains as a debug-level detail, explicitly not the contract.
  4. Offline proof — the now-validating fake rejects a summary-less plur_capture via isError: true (same convention as its existing unknown-tool path), and a unit test drives the hook's capture against that rejection and asserts: the counter incremented, the Warnings surface carries the failure, and no panic/no false success. This proves the counter fires on the exact class that shipped the bug — deterministically, offline, through the same wire convention the real server uses.
  5. Live-leg replacement — the vacuous stderr assertion is dropped; the A4 content-bearing persistence assertions are the signal, plus the session-end aggregate surface appearing in the CLI's visible output.

Acceptance criteria added: unit tests assert (a) each write site treats result.Error != nil as failure and increments the counter, (b) the injector strips (never injects) on an isError rejection, (c) the rejecting fake drives the counter via isError: true; E2E asserts the session-end aggregate surfaces in visible output when the write path fails. The counter design is now keyed on the domain outcome, not the transport error — so the next schema drift fires it instead of hiding for another release.

Q06 — GRILLER

[MODEL] Verified — and A5's surface claim has the same plumbing bug this whole issue is about. In ExecuteTurn (engine.go:344-350), finalizeTurnTrace runs and the TraceEvent is published before notifyAfterTurn fires; the only reader of Turn.State.Metadata.Warnings is finalizeTurnTrace itself (engine.go:381-382). An AfterTurn-time append lands in state that no consumer ever reads again — the next turn's state is freshly allocated by CreateTurn.

Q6:

Your failure-detection contract item 3(a) — the in-process per-turn surface, "append memory_write_failures:<n> to the Warnings path ADR-068 §8 already established (ContextMetadata.Warnings → Turn.State.Metadata → TurnTrace.Warnings), which reaches the UI and telemetry" — is mechanically dead for the hook. That path is Transform-time only: the injector appends during assembly, ContextRefiner copies into Turn.State.Metadata, and finalizeTurnTrace snapshots it — all before the turn ends. The hook fires in notifyAfterTurn, which runs after finalize and after the TraceEvent publish; it has no ContextRequest and no event-bus reference to inject earlier. So the per-turn failure count reaches neither telemetry nor the UI as wired.

So what is the actual per-turn surface? Either you drop it and make the session-end aggregate the single surface — in which case pin its ordering, because the FlushSession defer is registered at the top of Chat (agent.go:373-374) and any later-registered aggregate defer runs before it under LIFO, missing the flush's own failures: the aggregate must live inside FlushSession after its write attempt, or in a defer registered before line 374 — or you give the hook an event-bus reference and publish a dedicated failure event. My recommendation: drop the per-turn Warnings surface — the session-end aggregate, computed after the flush attempt, is the one surface a user can actually act on; keep the per-write debug logs as diagnostics. Which do you choose, and does the offline test that proves the counter fires also prove the surface it rides on is actually observable?

A06 — SUBJECT (Architect)

[MODEL] A6 — Verified, and the per-turn surface is dead as wired. ExecuteTurn sequence (engine.go:337-358): runPhaseLoop (ContextRefiner copies ContextRequest.Metadata.Warnings → Turn.State.Metadata during PhaseRefining) → finalizeTurnTrace (line 347 snapshots Turn.State.Metadata.Warnings into the trace) → TraceEvent publish (lines 349-354) → notifyAfterTurn (line 356 fires plurHook.AfterTurn last). The hook fires after the telemetry snapshot and after the UI-visible publish; it receives no ContextRequest, and appending to Turn.State.Metadata at that point reaches neither the already-finalized TurnTrace nor the already-published event. My item 3(a) was mechanically wrong.

My choice: drop the per-turn Warnings surface; the session-end aggregate is the single surface — and I pin its ordering exactly as you specify. The hazard is real and I verified the seam: defer flusher.FlushSession(s.ID) sits at agent.go:373-374, the first defer registered in Chat. Under LIFO, any aggregate defer registered later in Chat executes before the flush — reading a count that excludes the flush's own failure. So the aggregate lives in the same defer, flush first, read second:

if flusher, ok := a.memoryHook.(interface{ FlushSession(string) }); ok {
    defer func() {
        flusher.FlushSession(s.ID)
        if n := flusher.MemoryWriteFailures(s.ID); n > 0 {
            // surface via turnsLogger (turns.log / `tell-me-go -t`) + one-time
            // "learning is disabled because writes are failing" notice
        }
    }()
}

One defer, registered at the same top-of-Chat position — ordering is deterministic: FlushSession drains, issues plur_learn_batch, increments failCounts[sessionID] on a detected failure (err != nil || result.Error != nil), and then the accessor reads the count that includes the flush attempt. The counter map and accessor are under the existing h.mu. I explicitly reject the dedicated-event alternative even though it is mechanically open (the Turn struct carries Events, engine.go Turn field, so the hook could publish at AfterTurn): it requires a new event type plus a subscriber in the UI/telemetry path — new wiring and a new ADR-036 determinism surface — for a signal the aggregate already carries. The per-write Debug logs stay as diagnostics only.

Does the offline test prove the surface is observable? Yes, at two layers. (1) The unit test drives the rejecting fake (isError: true, missing summary) through capture and through FlushSession (the batch flush failure), then asserts: the counter incremented for each detected failure including the flush's own, and MemoryWriteFailures(sessionID) — the exact accessor the agent's defer reads — returns that count. That proves the surface's payload is correct for the shipped bug class, offline. (2) The agent-level wiring test (or the E2E with TELL_ME_DEBUG=1) asserts the defer sequence actually surfaces the aggregate line to the turnsLogger output — proving observability end-to-end, not just counter correctness. The surface rides on the accessor; both are pinned by tests.

Acceptance criteria delta: memory_write_failures:<n> Warnings append is removed from the contract; added: (a) unit test — rejecting fake, capture + flush, counter and MemoryWriteFailures accessor both asserted; (b) wiring test — the single top-of-Chat defer surfaces the aggregate after the flush attempt (LIFO-safe by construction: one defer, flush-then-read); (c) the live leg asserts the aggregate surface appears in visible output when the write path fails. The contract is now: detect on domain outcome (err != nil || result.Error != nil), count per session under mutex, surface once, at session end, after the flush attempt, through turns.log — one actionable signal, ordered, tested, and no longer relying on a Transform-time pipe the hook cannot reach.

Q07 — GRILLER

[MODEL] A6's ordering fix is verified correct — one defer, flush-then-read, registered at the same top-of-Chat position, is LIFO-safe by construction, and the engine ordering (finalize → publish → notifyAfterTurn) confirms the per-turn surface is dead. One decision the Subject flagged in its opening and has never resolved now sits at the center of the default tier's data path.

Q7:

Your mapping table (A3) skips empty-text episodes at flush time — branch (ii) error episodes become no engram. But AfterTurn still appends every classified episode to the ring buffer in the batch/full tiers (bufferAppend runs for all three branches, hook.go), and the ring is 20 slots with oldest-eviction (buffer.go). So error-only episodes occupy slots and silently evict learnable text episodes on the default tier: a session with 25 error turns then 5 text turns flushes zero engrams — the 5 learnable episodes were evicted before they could map. That is the same silent-memory-loss class this issue is about, re-introduced by the fix's own mapping.

Pin the buffer semantics: skip-at-append — don't buffer empty-text episodes in batch/full tiers at all (error episodes are the capture tier's job on the timeline, consistent with ADR-068 §2(ii) "no learning from this branch") — so the 20-slot bound applies to learnable content and the flush never maps an empty set; or skip-at-mapping — keep the raw ring as-is and accept that error episodes can evict learnable ones. And either way: when the mapped engrams array comes out empty, does FlushSession skip the plur_learn_batch call entirely (current code skips on len(episodes)==0), rather than sending engrams: [] on a pointless roundtrip? My recommendation: skip-at-append plus skip-the-call-on-empty — it makes the ring bound semantically "the last ~20 learnable turns," costs one condition in bufferAppend, and keeps TestHookBatch's buffer assertions meaningful. What's your call?

A07 — SUBJECT (Architect)

[MODEL] A7 — Your recommendation is correct, and I verified the mechanism against the code — with one arithmetic correction that strengthens rather than weakens it. bufferAppend runs for all three branches in batch/full tiers (hook.go: batch and full both call h.bufferAppend(turn.SessionID, ep)), and branch (ii) produces episode{Error, Prompt, ...} with empty Text — so error episodes do occupy ring capacity (maxBufferEpisodes = 20, oldest-eviction, buffer.go). But in your example ordering — 25 error turns then 5 text turns — the text survives: it's appended last, so it's the newest and evicts the oldest (error) entries, flushing 5 engrams. The zero-engram case is the reverse ordering: 5 text turns first, then 25 error turns — the learnable episodes are the oldest, the errors evict them one-for-one, and the flush maps an empty set. Either way your core claim stands: error-only episodes consume slots they have no right to, and on the default tier they can silently evict learnable content — the same silent-memory-loss class as the issue.

My call: skip-at-append, plus keep the skip-the-call-on-empty guard.

  1. Skip-at-append — one condition in bufferAppend (buffer.go:44): if strings.TrimSpace(ep.Text) == "" { return }. The TrimSpace matters, not just == "": joinTextParts includes a part whose text is " " (the filter is p.Text == "", so whitespace-only passes), and a whitespace statement would map to a garbage engram. The trim check covers both branch (ii) (Text == "") and the whitespace edge in one predicate. Consequence: the 20-slot bound becomes semantically "the last ~20 learnable turns"; error turns can never evict learnable content; the flush never maps an empty set from eviction. Error episodes remain the capture tier's job on the timeline — consistent with ADR-068 §2(ii) "no learning from this branch" (note: on the capture tier, branch (ii) episodes still ship as summary via A3's error-first fit rule — that path is untouched).
  2. Skip-the-call-on-empty — already in the code: FlushSession returns when len(episodes) == 0 (hook.go:283-285). With skip-at-append this guard now means "no learnable content in this session," and engrams: [] is never sent. Keep it as belt-and-suspenders — it also covers the nil/empty-buffer path independently of the append predicate. (The delete of the map entry and the drain-under-lock still happen before the guard, so a skipped flush still clears state — no behavioral regression.)
  3. Full-tier nuance preserved: skip-at-append gates only the buffer. maybeLearn runs independently on the user message (lastUserText(PreparedHistory)), so a correction frame in a user message whose turn errored still fires the gated plur_learn — the instruction was given even though inference failed. That's correct and must not be "simplified" into the buffer skip.
  4. Test impact — no existing test breaks, two added: TestHookBatch buffers only responseTurn text episodes (assertion len(buf.episodes) == 2 stands); TestHookFull's "non-frame no learn but buffer appended" subtest uses a text response (still buffers). New: (a) batch tier with a branch-(ii) error turn produces no buffer entry and a subsequent FlushSession makes zero MCP calls (asserting both skip-at-append and skip-the-call-on-empty); (b) text-then-error flooding — buffer 5 text episodes, append 20+ error episodes, assert the 5 text episodes survive and map to 5 engrams (pins the "error turns cannot evict learnable content" property).

Acceptance criterion added: the ring bound is documented as learnable-content-only; the flush never emits engrams: []; and the error-then-text / text-then-error flooding tests pin both orderings. Your recommendation, adopted as specified.

Q08 — GRILLER

[MODEL] A7 is adopted cleanly — the ordering correction is right (oldest-eviction means text-appended-last survives; the zero-engram case is the reverse order, which your flooding test pins), the TrimSpace edge is real (joinTextParts filters only p.Text == "", so whitespace-only passes), and FlushSession's existing len(episodes)==0 guard is confirmed at hook.go:283-285. But A6 left one acceptance criterion that is un-implementable as written, and it's the third "assert without mechanism" in this plan.

Q8:

A6 acceptance (c) — "the live leg asserts the aggregate surface appears in visible output when the write path fails" — cannot execute. The live leg runs the CLI against the real, now-correct server, which accepts the fixed payloads: no write failure can occur, so the aggregate never fires and the assertion is vacuous. And the aggregate can only be exercised by a rejecting server — but the now-validating fake rejects only on missing-required, which the fixed hook never sends, so even the offline E2E can't trigger it with the fake as specified. The "prove the surface end-to-end" claim therefore has no mechanism anywhere in the plan: the unit-level counter test proves detect+count, and the wiring test proves the defer reads the accessor, but nothing proves the whole chain — rejected write → counter → aggregate → visible output — through the real adapter.

What is the mechanism? My recommendation: give fakeplur an env-gated reject mode (e.g. FAKE_PLUR_REJECT_ALL=1 — it already has FAKE_PLUR_STORE/FAKE_PLUR_SEED env plumbing, so one more var fits its shape), drive a full CLI offline E2E against it (LEARN: capture, rejecting fake, read the turns.log the aggregate writes to), and assert the aggregate line appears — proving detect → count → surface through the real StdioClient (which correctly surfaces the fake's isError as ToolResult.Error) and the real Chat defer. And re-scope (c) to that offline leg, since the live leg can never produce a write failure. Reject-mode + offline E2E, unit-only, or something else — what's the call?

A08 — SUBJECT (Architect)

[MODEL] A8 — Conceded: acceptance (c) as scoped is vacuous, and the gap is real — the aggregate can only be triggered by a rejecting server, and nothing in the plan could produce one. The live leg runs against the corrected real server (writes succeed; a dead server is the skip precondition, not a failure), and the validating fake as specified rejects only missing-required, which the fixed hook never sends (summary always present, statement always present via skip-at-append, engrams never empty). So detect→count→surface was proven only in two disjoint halves: unit (detect+count) and wiring (defer reads accessor), with nothing through the real adapter — which is precisely where the failure class lives (StdioClient.CallTool maps isError → ToolResult.Error, nil Go err, stdio_client.go:208-218).

My call: reject-mode + offline E2E, your recommendation, with one refinement — scope the rejection to the write path. fakeplur gains FAKE_PLUR_REJECT_WRITES=1 (it already takes FAKE_PLUR_STORE/FAKE_PLUR_SEED, so one more env var fits its shape): plur_capture/plur_learn/plur_learn_batch return isError: true with an error text; plur_inject_hybrid/plur_recall/plur_status behave normally. Reject-writes-only deliberately reproduces the exact production symptom this issue shipped: injection healthy, writes dead — so the offline leg proves the surface fires precisely in the "looks fine, learning is broken" scenario, through the real StdioClient (isError → ToolResult.Error) and the real Chat defer, end-to-end.

The offline leg (re-scoped (c)): full CLI run, LEARN: capture, FAKE_PLUR_REJECT_WRITES=1, TELL_ME_HOME in a temp dir. Per-turn plur_capture rejections hit the fixed result.Error != nil detection → counter increments. At session end, Chat's single top-of-Chat defer runs FlushSession (capture tier: buffer empty → early return, which does not block the aggregate — the defer reads MemoryWriteFailures(s.ID) regardless of the flush's internal return) then surfaces the aggregate. Assertions: (1) the aggregate line appears in the pinned surface — the session's turns.log via the turnsLogger path, with the harness's established stderr capture (Warn level, no TELL_ME_DEBUG needed) as the secondary assertion; (2) memory_injected still appears (injection unaffected by write rejection — the bug scenario, now with a visible failure); (3) the store shows zero persisted episodes/engrams (the rejection actually happened). One honest flag for implementation: the E2E must verify the turnsLogger is wired in the minimal CLI config path the memory E2E uses — if that path doesn't enable it, the stderr Warn surface is the pinned one and the test asserts there; the mechanism (reject-mode → real adapter → real defer → visible aggregate) is identical either way.

Live leg re-scope: the live leg keeps A4's persistence-success assertions only — it can never produce a write failure, so the failure-surface chain is exclusively the offline reject leg's job. No vacuous assertion remains.

Layered acceptance criteria (final): (a) unit — rejecting fake drives detect+count per write site, injector strips on result.Error; (b) wiring — single top-of-Chat defer reads the accessor after the flush attempt (LIFO-safe); (c) offline E2E — full CLI against FAKE_PLUR_REJECT_WRITES=1, aggregate surfaces in visible output, injection still healthy, zero persistence, through the real StdioClient; (d) live leg — persistence-success assertions only. Reject-mode + offline E2E it is — the whole chain, not the halves.

Q09 — GRILLER

[MODEL] A8's reject-writes-only scoping is well-designed — injection-healthy/writes-dead reproduces the shipped symptom exactly, and the isError → ToolResult.Error path through the real StdioClient is confirmed. One unresolved contradiction remains in fix #4 itself, and it sits on the round's core theme: the trigger that decides when the user is told learning is broken.

Q9:

Your trigger condition changed between A5 and A6 and was never reconciled. A5: fire the one-time "learning is disabled because writes are failing" notice "when failures == write attempts for the session" — all-or-nothing. A6's defer snippet surfaces the aggregate and the notice on n > 0 — any failure. The two calibrations have opposite failure modes, and the all-or-nothing one contradicts your own stated goal: A5's purpose was "the next schema drift fires it instead of hiding for another release." But failures == attempts misses exactly a partial drift — if @plur-ai/mcp changes plur_learn_batch's schema while capture/learn keep working, the batch flush fails while other writes succeed: failures < attempts, no notice, and the default tier's write path hides again in the same "injection healthy, learning broken" shape this issue shipped. Meanwhile n > 0 fires on a single transient server hiccup and declares "learning is disabled" when nothing is disabled. And a single per-session counter can't express the distinction at all — it loses which tool failed.

So: pin the trigger and the counter granularity. My recommendation: key failCounts by (session, tool) and declare a tool dead when all of that tool's attempts failed — the notice names the tool ("plur_learn_batch failing — learning is disabled"), tolerates transients, and catches partial drift on any single write tool, while the aggregate stays per-session. What's the trigger — per-tool all-or-nothing, n > 0, or the A5 global failures == attempts?

A09 — SUBJECT (Architect)

[MODEL] A9 — Your per-tool calibration is correct, and the reconciliation failure is mine: A5's global failures == attempts has exactly the blind spot you name — a partial drift (plur_learn_batch schema changes, capture/learn keep working) yields failures < attempts, no notice, and the default tier hides again in the same "injection healthy, learning broken" shape. A6's n > 0 over-fires on a single transient. The single per-session counter can't express either distinction.

Pinned trigger: per-tool all-or-nothing, your recommendation.

  • Counter granularity: failCounts becomes writeStats map[string]map[string]writeStat keyed (sessionID → tool → {failures, attempts}), under the existing h.mu. Every write call site (capture→plur_capture, maybeLearn→plur_learn, FlushSession→plur_learn_batch) increments attempts unconditionally and failures on err != nil || result.Error != nil. The per-session aggregate for the summary line is derived — sum(failures) across tools — so the aggregate surface (A6) is unchanged, and the notice rides the richer structure.
  • Notice trigger: a tool is declared dead when attempts ≥ 1 && failures == attempts for that tool. The session-end defer computes the per-tool dead set and emits one notice naming the tool: "plur_learn_batch failing — learning is disabled". It fires once per session per tool (session-end computation is inherently once-per-session); per-session counters bound the blast radius — a transient flush failure declares dead for that session only, and the next session starts with fresh stats. That's the honest trade: the session-end flush has exactly one attempt per session, so a transient there produces one session's notice — but the notice is truthful ("this session's learning did not persist"), and per-turn tools (capture/learn) get real transient tolerance via attempt volume (1 failure among 10 attempts → not dead).
  • Partial drift — caught and named: batch all-failed while capture all-succeeded → only the plur_learn_batch notice fires. That's the exact scenario this issue shipped; the per-tool key is what makes it visible.
  • Injector excluded from the notice: Seam A's result.Error detection (A5) remains strip+fail-open — injection failures are visible by the absence of the memory block, not by the learning notice. The notice is write-path only.

Test matrix update — and a mechanism refinement to make partial drift testable: fakeplur's reject gate becomes FAKE_PLUR_REJECT=<comma-separated tool names> (superset of the A8 REJECT_WRITES shape — same env plumbing, per-tool precision). New unit tests: (a) all-attempts-failed per tool → dead → notice names the tool; (b) 9-fail-1-success → not dead (transient tolerance); (c) partial drift — batch all-failed, capture all-succeeded → only plur_learn_batch notice fires. Offline E2E (A8) runs FAKE_PLUR_REJECT=plur_capture with LEARN: capture and asserts the notice names plur_capture specifically; an optional second leg with FAKE_PLUR_REJECT=plur_learn_batch + LEARN: batch pins the default-tier partial-drift case through the real adapter. Acceptance criterion: the notice always names the dead tool, never fires on a merely-failed-but-partially-succeeding tool, and the aggregate line remains the per-session failure total. Per-tool all-or-nothing — pinned.

Q10 — GRILLER

[MODEL] A9 closes the trigger calibration cleanly — per-tool all-or-nothing with attempts ≥ 1 && failures == attempts, the honest transient caveat on the session-end flush, and FAKE_PLUR_REJECT=<tools> making partial drift testable. The plan is now internally consistent. One branch of the decision tree remains unwalked, and it is the seed this issue grew from.

Q10:

Your opening identified buffer.go's comment as "the seed of the circular test" and fix #1 updates it. But ADR-068 — the authoritative spec this issue itself cites — still documents the broken contract: §3 describes the capture tier as "per-turn plur_capture episodes ({agent: Mode, session_id})" — the exact wrong payload, missing summary; §2(ii) documents the episode shape the fix re-maps; the batch tier is described as "episodes + session-end plur_learn_batch" — now engrams-only; and §8's live-E2E posture ("best-effort, log-and-pass — never a hard gate", echoed in the memory_live_test.go header) is contradicted by your new hard-fail-when-run legs. The bug shipped because the documentation trail (ADR → buffer.go comment → fake mirroring the comment) was circular and wrong; the fix breaks the code-side circularity but leaves the authoritative spec documenting the wrong payload — so the next reader, or the next grill, re-derives the broken contract from the ADR.

Will the fix amend ADR-068 in the same change — §2(ii) episode shape, §3's tier table (capture payload, batch engrams-only semantics, the tags identity convention), §8's live-leg posture — plus a corrections-table row recording this issue's findings (the result.Error detection contract, the failure surface, the per-tool dead-tool trigger), following the two-round corrections-table pattern the ADR itself established; and does the acceptance criteria set gain "ADR-068 amended, verify-adr-index green"?

A10 — SUBJECT (Architect)

[MODEL] A10 — Yes. The fix amends ADR-068 in the same change — the doc trail (ADR → buffer.go comment → fake mirroring the comment) is the circularity that shipped the bug, and breaking it at only the code link leaves the next reader re-deriving the broken contract from the authoritative spec. Verified against the ADR text: §3 documents capture as "per-turn plur_capture episodes ({agent: Mode, session_id})" — the exact wrong payload, missing the required summary; §2(ii) documents the {error, prompt, ...} episode shape the fix re-maps; §3 describes batch as "episodes + session-end plur_learn_batch" — now engrams-only; §8 pins the live posture "best-effort, log-and-pass — never a hard gate", which my hard-fail-when-run legs contradict. All four get amended, plus the corrections table:

  1. §2(ii) — the three-way classification keeps the internal episode shape ({error, prompt, mode, session, timestamp}), but the ADR gains the wire mapping the old contract conflated: capture-tier summary = "error: " + Error with the prompt folded when it fits (error-first, rune-safe, maxEpisodeBytes-bounded). The internal-model vs wire-contract distinction is documented explicitly so the next reader never derives the wire from the internal struct again.
  2. §3 tier table — capture: {summary, agent, session_id} (summary required, bounded; branch-(ii) fit rule). Batch: redefined as "learnable-episode ring buffer (~20 learnable turns, skip-at-append for empty-text episodes) + session-end plur_learn_batch of engrams: [{statement, scope?, tags: [session:<id>, mode:<mode>]}]" — engrams-only (episodes belong to the capture tier's timeline), scope native per-engram, the tags identity convention named. Full: gated plur_learn {statement, scope?} + tags (no agent, no session_id), plus the same batch flush.
  3. §8 posture + observability — live legs: "behind -tags=e2e_live, never in make check-full; environment preconditions (npx present, handshake OK) → skip; once run, persistence assertions hard-fail on mismatch — no log-and-pass past the precondition stage." The memory_write_failures Warnings-append is dropped (Transform-time-only, dead for the hook — A6); replaced by the session-end aggregate + per-tool all-or-nothing dead-tool notice + per-write Debug diagnostics. The memory_live_test.go header comment gets the same posture correction.
  4. New corrections-table rows (§10, following the two-round pattern the ADR established) — Round 3 records: (a) the three payload mismatches and the fully-silent failure class (adapter isError → ToolResult.Error with nil Go err, stdio_client.go:208-218 — no Warn ever fired); (b) the detection contract err != nil || result.Error != nil at all four call sites; (c) the surface: session-end aggregate + per-tool dead-tool notice via the top-of-Chat flush-then-read defer; (d) Seam A strip-on-rejection; (e) skip-at-append buffer semantics and per-tool counter granularity.

Acceptance criteria gain: "ADR-068 amended (§2(ii), §3, §8, §10 Round-3 rows), verify-adr-index green." The filename/numbering is unchanged (no new ADR — this is a correction of an accepted one, so the index gate stays green by construction and the criterion runs it). The circular trail is broken at all three links in one change: the ADR documents the real contract, buffer.go's comment cites the corrected §3 instead of defining the contract itself, and fakeplur's header no longer claims to mirror the episode struct — it now implements the real schemas with FAKE_PLUR_REJECT gating. No domain-model change is needed — the four Memory invariants (memory-injection-fail-open, memory-injection-budgeted, memory-single-block, memory-learn-tier-exclusive) are untouched by this fix, so modelith-check stays green; the only new symbols are unexported (memory-package-internal counter structs) and trip no drift/layers gates. Amended in the same change, indexed, gate green — yes.

VERDICT — GRILLER

[MODEL] VERDICT: The core diagnosis held — all three wire-contract mismatches verified against the live @plur-ai/mcp schemas, the circular fake contract confirmed, and the default batch tier shown broken — but the plan needed nine substantive corrections, the sharpest being the flat error in the failure narrative: the write path was fully silent, not Warn-silent, because the adapter surfaces isError rejections as ToolResult.Error with nil Go error and the hook discarded the result — which would have made the original fix #4 counter count zero for the exact bug class. The final integrated plan (per-tool dead-tool detection on the domain outcome, single session-end surface via the flush-then-read defer, skip-at-append buffer semantics, FAKE_PLUR_REJECT offline E2E through the real adapter, ADR-068 amendment) is sound and internally consistent. Recommendation: proceed with changes — implement exactly the pinned contract, and treat the isError blindness and the vacuous live-leg assertion as the two lessons that must not recur.

POST-GRILL POSITION — SUBJECT (Architect)

[MODEL] POST-GRILL POSITION:

What I stand by — the diagnosis and the contract direction survived intact. The root cause was never contested because it's verified: three payload mismatches against the real @plur-ai/mcp schemas — plur_capture missing required summary and carrying non-existent text/error/prompt keys (hook.go:172-196), plur_learn_batch sending episodes/session_id instead of engrams[] each with statement (hook.go:271-309, buffer.go:22-27), and plur_learn sending an unknown agent (hook.go:248-263). The circular test contract (ADR → buffer.go comment → fake mirroring the hook's own shape → non-validating unit double) and the fact that the only live leg ran LEARN: off and never exercised a write are the reasons it shipped. The corrected wire contract is settled: plur_capture = {summary, agent, session_id}, plur_learn = {statement, scope?, tags}, plur_learn_batch = {engrams:[{statement, scope?, tags}]}, with scope native per-engram and the tags: [session:<id>, mode:<mode>] identity convention uniform across tiers.

What the grill corrected in me — ten revisions, the most important being the failure-class correction. (1) My opening claim that the write path was "Warn-visible" was flat wrong: the adapter surfaces schema rejections as ToolResult.Error with a nil Go error (stdio_client.go:208-218), so no Warn ever fired — the path was fully silent, and my fix #4 as designed (counter on Go errors) would have counted zero. The detection contract is now err != nil || result.Error != nil at all four call sites, and Seam A's blindness (injector building a memory block from error text on an isError rejection) is fixed with strip-on-rejection. (2) I dropped session_id from plur_learn — its scope-resolution semantics against an unstarted session (zero plur_session_start repo-wide) is a mis-resolution risk, not a silent-drop risk. (3) I dropped my Transform-time Warnings surface — finalizeTurnTrace (engine.go:347) and the TraceEvent publish run before notifyAfterTurn (engine.go:356), so the hook can't reach it; the session-end aggregate, computed flush-then-read in the single top-of-Chat defer (agent.go:373-374), is the one actionable surface. (4) The trigger calibration is per-tool all-or-nothing — global failures == attempts misses exactly the partial-drift scenario this issue shipped; per-(session, tool) counters with a notice naming the dead tool catch it while tolerating transients. (5) The capture summary is now defined and bounded (error-first, prompt folded when it fits, maxEpisodeBytes — one contract). (6) Buffer semantics are skip-at-append (learnable-content-only ring; the griller's eviction example had the ordering backwards — zero-engram is text-then-errors — but the class of loss was real). (7) Live-leg persistence is proven through a second StdioClient before/after reads with content-bearing markers, hard-fail-when-run. (8) The failure-surface chain is proven by an offline reject-mode E2E (FAKE_PLUR_REJECT=<tools>) through the real adapter — the live leg can never produce a write failure. (9) ADR-068 is amended in the same change (§2(ii) wire mapping, §3 tier table, §8 posture, Round-3 corrections rows), with verify-adr-index green, breaking the doc trail at all three links. (10) Scope rides natively on every batch engram — my original plan's silent scope-drop would have mis-scoped the default tier's writes.

Open questions — three honest residuals, none blocking. (1) Whether the real server's runtime validation accepts a per-item session_id on plur_learn_batch despite the schema omission — probe-then-adopt in the live leg; until evidence, the tag convention carries identity. (2) Whether the turnsLogger is enabled in the minimal CLI config path the memory E2E harness uses — the aggregate's exact channel (turns.log vs stderr Warn) needs implementation verification; the mechanism is identical either way. (3) Whether the real server hard-rejects the unknown agent on plur_learn or silently ignores it — immaterial to the fix (missing-required is fatal either way), but the live full-tier leg will settle it. Plus one accepted trade, not a question: a transient failure on the single session-end flush declares that tool dead for that session — truthful (that session's learning didn't persist) and bounded by per-session counter reset.

The integrated position: detect on domain outcome, count per (session, tool), surface once at session end through a user-visible channel, name the dead tool, and pin every layer — unit (detect+count+fit-rules), wiring (LIFO-safe defer), offline reject E2E (whole chain through the real adapter), live leg (persistence success), and the amended ADR as the authoritative spec — so the next schema drift fires the surface instead of hiding for another release. The grill's value was the corrections; the final plan is materially stronger than my opening, and I stand by it as revised.

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