fix(memory): keep an extraction alive when it asks to leave plan mode - #1336
fix(memory): keep an extraction alive when it asks to leave plan mode#1336Poytr1 wants to merge 1 commit into
Conversation
A dream on review-bot's store died like this: the model researched, wrote a plan, called ExitPlanMode — and got back "Tool permission request failed: Error: Tool use aborted". It never wrote anything, so the run failed with nothing staged. Two causes, both ours. The permission policy refuses every request from a background extraction (correct — nobody can approve one), but it answered `cancelled`, which reaches Claude as an ABORTED tool call rather than a denial, and it abandons the run. Answer with the runtime's reject option instead, so a refused mode switch costs the request and not the dream; `cancelled` remains the fallback when no reject option is offered. The deeper cause is that #1298 turned the dream into a writing task while the session stays hard-gated to read-only/plan, so a model that respects plan mode asks to leave it before working. It never needed to: plan mode restricts the runtime's own file and shell tools, not our MCP memory tools. Both extraction prompts now say so and tell it not to ask. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Approved. Reviewed the real diff (git merge-base 3abe1ea 23138e4 = 4b2b82c) — 4 files / ~46 lines: the coordinator gate, the two extraction prompts, two tests. The diagnosis in the description matches the code, and the fix is well-scoped. No blocking findings.
What I checked
- The gate still refuses unconditionally.
memoryExtractionInFlightis evaluated before the built-in-system-tool auto-allow, so an extraction still gains no side effects — only the shape of the refusal changed.reason: 'memory_extraction'is still recorded against the pre-maskevaluationParams, andonAcpPermissionEventonly emitspermission.auto_allowedfor allow-kind options on thefallbacksource, so a selected reject can't be mislabelled as an approval in telemetry; it just becomespermission.resolvedwith anoptionId. - Blast radius is wider than dreams:
memoryExtractionCollectorsalso backs the per-turn distill pass (daemon.ts:3984) and the commit-message wand pass (daemon.ts:4094). Both get better under a rejection (they continue instead of aborting), but the doc comment atpackages/daemon/src/daemon.ts:4041still says "the collector's blanket permission cancel denies anything the runtime asks to do" — worth a one-line update while you're here. - The Codex path needs nothing: its MCP approvals arrive as
elicitation/create, and an extraction session has no pending turn there, soonAcpElicitreturnsundefinedand the host answers{action:'decline'}— already a refusal the model works past, not an abort. - The prompt claim is accurate:
writeMemoryunder the read-only/plan hard gate is the distill path's long-standing behavior — the mode restricts the runtime's own fs/shell tools, not our MCP tools. Both extraction prompts are covered (MEMORY_DREAM_SYSTEM_PROMPT,MEMORY_DISTILLATION_SYSTEM_PROMPT); the commit-message pass has no memory tools and needs no equivalent line.
Two non-blocking notes
- Silence now replaces a loud failure. Because a refused request no longer ends the run, a dream that loses some
writeMemorycalls completes with a partial staging area instead of failing.empty_proposal(dream/runner.ts:780) only catches "zero staged topics against a non-empty live store"; a partial rebuild still prunes everything it didn't write, unattended whenautoAdoptis on. That residual was already recorded on #1298 — but this PR is what makes "refused a tool, still finished" an ordinary outcome, so it's the natural place to note it. Cheap guard if you want one later: the coordinator now knows exactly when it refused an extraction request, so a refusal counter on the collector (alongside thestopReasonalready carried on the extraction result) would let the runner fail an otherwise-completed dream rather than stage it. reject_alwaysas the fallback.reject_onceis correctly preferred. A runtime that offers onlyreject_alwaysgets an "always" answer, which some runtimes persist beyond the single request — the distill pass is the one that would notice, since it runs on the warm host and reuses a cached session. Unreachable with today's claude-acp/codex option sets, and the alternative (cancelling) is the bug being fixed, so: note, not a change request.
On the unverified Opus path — rejecting ExitPlanMode doesn't by itself guarantee Opus writes instead of ending the turn with "the plan was rejected"; the prompt change is what actually addresses that. Worth stating that the failure mode if it doesn't take is a zero-write dream, which the empty_proposal fence converts into a recorded failure rather than a store wipe. Reasonable place to stop given the account limitation.
Tests: I read the two new cases (they exercise both the reject-offered and no-reject-offered branches and assert nothing is queued for a human). This review worktree has no node_modules, so I could not execute any suite — the green-suite claims in the description are taken at their word, not re-verified here.
sent by review-bot (Claude Agent · opus[1m]) · open in session
Reported by @pchsu from a real dream on review-bot's store: the model researched, produced a plan, called
ExitPlanMode, and gotIt then ended the turn with the plan instead of a proposal, so the dream failed with nothing staged. @pchsu guessed this was unrelated to the memory-format work — it isn't, and the connection is worth stating plainly.
Why it happened
Cause 1 — a refusal that reads as an abort.
resolveAcpPermissionrefuses every permission request from a background extraction, which is right: a dream must not gain side effects just because the agent runs in an auto-approval mode, and there is no human on a cron run to ask. But it answered{ outcome: 'cancelled' }, which Claude surfaces as an aborted tool call rather than a denial — and it abandons the run instead of continuing without the tool. It now selects the runtime'sreject_once/reject_alwaysoption when one is offered, so a refused request costs that request and not the whole dream.cancelledstays as the fallback for a runtime that offers no reject option.Cause 2 — a writing task under a mode that forbids writing. #1298 made the dream write its store through the memory tools, while the extraction session stays hard-gated to read-only/plan (
runDreamExtractionSession). A model that takes plan mode seriously therefore stops and asks to leave it before doing the work. It never needed to: plan mode restricts the runtime's own file and shell tools, not our MCP memory tools —mcp__agentconnect__writeMemoryhas been observed succeeding under plan mode in every local end-to-end run. Both extraction prompts now say exactly that and tell the model not to present a plan or ask for a mode switch.This is model-dependent, which is why it slipped through: Sonnet writes straight away (it notes the plan-mode reminder in its reasoning and ignores it), while Opus on a 26-file store planned first.
Verified
claude-acp: dream writes viamcp__agentconnect__writeMemory, noExitPlanMode, staged → adopted.Not verified here: the exact Opus path. This account cannot select
claude-opus-4.8, so I could not reproduce the original failure locally — the diagnosis is from the pasted tool output plus the code path, and the fix is verified on the Sonnet path only.🤖 Generated with Claude Code