feat(llm): resume the Claude Code session across a kill-chain phase - #149
Conversation
Every ReAct iteration flattens the growing transcript into a brand-new prompt and spawns a fresh `claude -p` process. Within one task that is a quadratic resend; across a mission it also means each spawn re-pays Claude Code's own CLAUDE.md/skills/MCP bootstrap tax from scratch, since T3MP3ST never told the CLI these calls belong to one session. LocalAgentAdapter now tracks the Claude Code session id from the JSON envelope and passes it back in with --resume on every later call, so the CLI carries the accumulated transcript itself and the repeated part is billed at cache-read pricing instead of resent. The session is scoped to one kill-chain phase: TempestCommand drops it for every operator right after advancePhase(), since local-agent operators are spawned once for the whole mission and would otherwise carry one session across all 7 phases. A stale or expired session id fails fast (confirmed against the real CLI: nonzero exit, no JSON on stdout) and localAgentChat retries once without --resume rather than failing the task over it. Verified against the real CLI: a resumed call recalled prior context correctly and dropped from $0.151 to $0.008 (cache_read_input_tokens absorbing the prior turn instead of cache_creation), and an invalid --resume id triggered the fallback and still succeeded. Ran live against an authorized target: the same session id held across all 4 recon tasks in one phase, then a newly spawned operator's first call in the next phase carried no --resume, confirming both the resume and the reset. Full suite: 764/764, no regressions.
jmagly
left a comment
jmagly
left a comment
There was a problem hiding this comment.
Reviewed exact head 577f0a6 against current main (afc9dad). One blocking contract defect remains.
LocalAgentAdapter.chat() always builds prompt = formatPrompt(messages, options), which serializes the complete, growing ReAct transcript. On later calls the PR sends that full transcript while also passing the prior Claude session via --resume. The resumed session already contains earlier turns, so prior context is duplicated; the implementation therefore does not achieve the stated “carry the session instead of resending the transcript” behavior and can distort context as well as cost.
Please make resumed calls send only the delta/new turn (while fresh calls retain the full prompt), and add a regression test that asserts the second CLI prompt excludes prior messages/tool results while retaining the new observation and tool contract. Also cover the stale-session fallback: its fresh retry must receive the complete transcript, not the delta.
Verification on this head:
npm run typecheck: pass- focused local-agent tests: 66/66 pass
npm test: 764/764 pass, plus ops/model/refusal gates
The current tests only assert sessionId plumbing; they do not assert prompt contents, which is why the behavioral mismatch remains green.
jmagly's PR elder-plinius#149 review caught it: LocalAgentAdapter.chat() built its prompt from the full messages array unconditionally, so a resumed call sent the whole growing transcript again on top of a session that already had it. The resend the feature exists to remove was still happening, and the resumed session's own history now grew on top of it too. Existing tests only asserted that sessionId reached the CLI, never what was actually in the prompt, so this passed green. LocalAgentAdapter now tracks the exact messages array object and how many of its entries were already sent. A later call against the SAME array (AgentLoop growing one task's transcript via push) sends only the new tail. A DIFFERENT array (a new task) still sends everything, since the resumed session has never seen that task's content even though the CLI session itself carries over. The stale-session fallback in localAgentChat now takes a separate fallbackPrompt: a fresh session has no history, so its retry gets the full transcript, never the delta sized for the failed resumed attempt. Verified against the real CLI with a synthetic multi-round exchange (~750 tokens of padding added between each call): promptTokens went 19127 -> 19625 -> 20135 -> 20645, a flat ~510 tokens per round regardless of the accumulating transcript, instead of resending or compounding. Added the regression coverage the review asked for: a same-array resumed call excludes the prior turn's content while retaining the tool contract, a new-array call still gets the full prompt, and the fallback retry receives fallbackPrompt rather than the delta, checked both at the LocalAgentAdapter call-shape level and against a real spawned CLI script. Full suite: 769/769, no regressions.
|
Confirmed, and fixed in
Added the tests you asked for: same-array resumed call excludes the prior turn while retaining the tool contract, a new-array call still gets the full prompt, and the fallback receives Also re-verified against the real CLI with a synthetic multi-round exchange (~750 tokens of padding added between each call):
|
Hosted CI caught it, not my local run: the previous commit used LLMMessage[] type annotations in two new tests without importing the type, so tsc failed with TS2304 while vitest itself stayed green (esbuild strips type annotations without checking them). I ran typecheck once before writing those tests and never reran it after, so this went out uncaught. Full suite: 769/769. npx tsc --noEmit: clean.
jmagly
left a comment
jmagly
left a comment
There was a problem hiding this comment.
Re-audited exact head a0f0d45 against main at 6d4e017. The requested delta-prompt and fresh-fallback coverage is present, and local verification passes, but the deeper provider/trust-boundary review found two blockers that matter for T3MP3ST's operating posture.
-
The Claude session survives across different tasks within a phase.
LocalAgentAdapter.chat()deliberately resumes the sameclaudeSessionIdwhen it receives a different messages array; it only changes from delta to full-prompt mode. That means a new task—and potentially a different target—still inherits the prior task's system prompt, target data, tool output, and attacker-controlled content from Claude's persisted session. T3MP3ST currently constructs a fresh message array perAgentLoop.run()and uses the PackBoard as the explicit cross-task sharing boundary. Preserve that isolation: resume within one task's ReAct loop, then reset before another task/target, unless a separately reviewed and provenance-controlled cross-task context contract is introduced. Add a regression proving a second task does not resume the first task's session. -
Claude Code is invoked with
-p --output-format jsonbut without disabling its built-in tools. The installed CLI's own help exposes--tools ""as the way to disable all tools, while the adapter contract says the model requests actions and T3MP3ST's Arsenal executes them behind scope/approval gates. A resumed coding-agent session with filesystem/Bash/MCP access creates an execution path outside Arsenal. Invoke Claude in planning-only mode with built-in tools and ambient MCP/plugin behavior disabled as appropriate, on both initial and resumed calls, and add argv-level regression coverage.
Also narrow the fallback trigger: the current .catch() starts a fresh session after every resumed-call failure, not only the documented stale-session failure. Once internal tools are denied this is mainly duplicate cost/work rather than duplicate host actions, but it should still retry fresh only for a positively identified stale-session error and propagate unrelated auth, rate-limit, timeout, and process failures.
Verification on a0f0d45:
npm ci: pass; npm reports 1 moderate and 5 high advisories, not independently attributed to this diffnpm run typecheck: passnpm test: pass, 769/769 plus ops/model/refusal gates- hosted
test: pass on the exact head - Claude Code CLI inspected locally: 2.1.215;
--resumeand explicit tool-disable controls are available
The cost optimization is useful, but session reuse must remain subordinate to task/target isolation and Arsenal as the sole execution authority.
A self-review after the last round turned up a more serious version of
what jmagly caught: TempestCommand built exactly one LLMBackbone and
handed the SAME instance to every operator's AgentLoop (and to each
OperatorAgent's own decompose-on-failure path). LocalAgentAdapter's
new session/delta state lived on that shared instance, so one
operator's Claude Code session could get resumed by a completely
different operator, with the second operator's unrelated task content
sent into the first operator's conversation. Every test for the
resume feature so far constructed independent LLMBackbone instances
per test, which is not how spawnOperator() actually wires things, so
nothing caught it.
spawnOperator() now builds a fresh LLMBackbone per operator from the
same config, used for both that operator's AgentLoop and its own
decompose fallback, so state stays consistent within one operator and
isolated from every other one. OperatorCell.spawnOperator() takes an
optional llm override to carry that through; existing callers that
don't pass one keep the cell's shared default.
Two narrower issues from the same review:
localAgentChat's stale-session fallback retried on ANY failure of a
resumed call, not just a confirmed stale session. A real network
error or auth failure would silently double latency on a retry that
could not fix it, and compounded with LLMBackbone's own retry ladder
into up to 6 CLI spawns for one failing turn. The fallback now only
fires for the exact empirically-confirmed stale-session error text
("No conversation found with session ID").
The character-estimate usage fallback (used only when the JSON
envelope fails to parse) estimated from the delta prompt instead of
the full transcript. A resumed call's real promptTokens reflects the
whole cached conversation, confirmed empirically in the last round
(19127 -> 19625 -> 20135 -> 20645), so estimating from the small delta
text instead undercounted the budget check by an order of magnitude
in that one edge case.
One finding from the review was deliberately left alone: resending
the tool contract on every delta call is real overhead, but jmagly's
review explicitly required the contract stay on every call ("retaining
... the tool contract") and the existing test already pins that. Cutting
it would contradict that requirement for a small, bounded savings.
New coverage: an isolation test that spawns two operators through a
real TempestCommand and confirms neither's LocalAgentAdapter nor its
session leaks into the other, a test proving a non-stale failure
propagates without a fallback retry, and a test proving the usage
estimate reflects the full transcript on a mid-session parse failure.
Full suite: 772/772, no regressions.
|
Ran a full self-review after the last round to check for anything else in the same shape as the transcript-duplication bug. Found three more issues, fixed in The significant one: Two narrower ones from the same pass: the stale-session fallback retried on any failure of a resumed call, not just a confirmed stale session, which could double latency on a real network/auth error and compound with the outer retry ladder into up to 6 CLI spawns for one failing turn. Narrowed to only retry on the exact confirmed stale-session error text. And the character-estimate usage fallback (used only when the JSON envelope fails to parse) estimated from the delta prompt instead of the full transcript. A resumed call's real Left one thing alone on purpose: resending the tool contract on every delta call is real overhead, but your review explicitly asked for it to stay ("retaining ... the tool contract"), and the existing test already pins that. Didn't want to quietly walk that back. New tests: an isolation test spawning two operators through a real
|
jmagly
left a comment
jmagly
left a comment
There was a problem hiding this comment.
Reviewed the author response and re-audited exact head 07108d5 against main at 6d4e017.
Thank you for the self-review. This head does resolve three real issues:
- each spawned operator now receives its own
LLMBackbone, eliminating cross-operator session leakage; - fresh-session fallback is limited to the confirmed stale-session error instead of every resumed-call failure;
- malformed-envelope usage estimation now uses the full prompt rather than the wire delta.
The new tests exercise those fixes, and both local and hosted verification are green. Two blockers from the previous review remain, however:
-
Task/target isolation is still not implemented.
LocalAgentAdapter.chat()still passes the existingclaudeSessionIdwhenever the same operator receives a different messages array. The code changes only from delta to full-prompt serialization; it does not start a fresh Claude session.AgentLoop.run()creates a new array for every task, so a reused operator can carry prior task/target/tool output and untrusted target content into a later task. The new test proves operator A does not leak into operator B, but it does not prove task 1 does not leak into task 2 on operator A.Treat a different messages-array identity as a new task boundary: clear the prior session/sent-message state before dispatch and make the first call without
--resume. Add a production-shaped regression using one operator/adapter for two sequential tasks and assert the second task has no session ID. If cross-task memory is desired later, it should travel through the explicit, provenance-controlled PackBoard/context contract—not an opaque provider session. -
Claude Code's ambient execution paths are still enabled. Both initial and resumed argv remain
-p --output-format json ...with no explicit tool denial. T3MP3ST's contract says Claude proposes tool calls and Arsenal executes them behind scope/approval/audit gates. The locally installed Claude Code 2.1.215 CLI exposes--tools ""to disable built-ins and controls for safe-mode/strict MCP configuration, but this adapter uses none of them. Project/user tools, MCP, hooks, or coding-agent capabilities must not become a parallel action path outside Arsenal.Run both fresh and resumed calls in an explicit planning-only configuration: deny built-in tools and disable ambient MCP/plugin/hook/customization execution using supported CLI controls while preserving native authentication. Add argv-level tests for both paths and a negative test proving no alternate tool path is enabled.
Provider-boundary verdict on this head:
- service reality: yes (Claude Code CLI 2.1.215 inspected locally)
- integration verification: partial
- sensitive-workload trust: conditional on the operator's Anthropic/Claude configuration; no new trust claim approved here
- integration completeness: no, because task isolation and sole-execution-authority controls remain absent
- merge ready: no
Verification on 07108d5:
npm ci: pass; npm reports 1 moderate and 5 high advisories, not independently attributed to this diffnpm run typecheck: pass- targeted local-agent/index tests: 117/117 pass
npm test: 772/772 plus ops/model/refusal gates pass- hosted
test: pass on the exact head
This is now a much narrower correction. Once these two boundaries and their regression tests are present, the next exact-head review can focus on the final diff rather than reopening the resolved items.
jmagly's third review: switching from delta to full-prompt sending on a new messages array wasn't enough, since LocalAgentAdapter.chat() still passed the OLD claudeSessionId either way. A second task on the same operator inherited the first task's persisted Claude session, carrying its system prompt, target data, and tool output (including anything attacker-controlled) across a boundary T3MP3ST already has a single sanctioned channel for: PackBoard. chat() now clears claudeSessionId whenever the incoming messages array is not the one it last saw, before building the request. A different array means AgentLoop.run() started a new task; the call after that reset carries no --resume, so it opens a genuinely fresh session instead of resuming the old one under a bigger prompt. Within one task the array is the same object across iterations (AgentLoop grows it via push), so resumption and the delta-only send from the last round are untouched. Rewrote the tests that asserted continuity across separate array literals, since that shape doesn't happen in AgentLoop and was masking this exact gap: they now grow one array via push to represent a real task, matching the pattern already used for the delta tests. Added the test jmagly asked for directly: one operator, two sequential tasks, task 2 has no session id, both at the LocalAgentAdapter level and through a real TempestCommand.spawnOperator(). Full suite: 773/773, no regressions.
|
Pushed 2ab9438 for the task-boundary issue: On the built-in-tools point, I tested I also tried I don't have a fix for this yet. Options I haven't tried: |
jmagly
left a comment
jmagly
left a comment
There was a problem hiding this comment.
Re-audited exact head 2ab943898df4793d9d9db3bfe8b0f68ec6892e44 against main at 6d4e017 and reviewed the contributor's runtime evidence.
The task-boundary blocker is resolved: a different message-array identity clears the session before dispatch, and production-shaped coverage proves sequential tasks on one operator do not resume each other. The focused local-agent/index tests pass.
One design blocker remains. The contributor's tests show that --tools "" does not provide a reliable execution boundary in the installed Claude Code CLI, while --permission-mode plan reliably blocks writes but also rejects the action-contract flow this adapter requires. That confirms the current coding-agent session cannot simultaneously satisfy both contracts: opaque provider-session reuse and Arsenal as the sole execution authority.
Please do not paper over this with prompt language or an argv assertion. The next revision needs an enforceable architecture, for example a provider/runtime mode whose API can deny ambient tools/MCP/hooks while still returning the structured action proposal, or a narrower change that excludes Claude Code session reuse until that boundary exists. If the project instead chooses to treat Claude Code as a separately trusted execution authority, that requires an explicit operator-facing trust/approval contract and a dedicated design review; it is beyond this PR's reviewed intent.
Verification:
npm ci: passnpm run typecheck: pass- focused local-agent/tool-calling/index tests excluding the known PATH-sensitive environment assertion: 88/88 pass
- hosted
test: pass on this exact head
Decision: hold/requested changes remain. This is not low-risk maintainer courtesy cleanup.
|
The merge train has reached this PR, but the remaining item is architectural rather than mechanical, so I cannot safely rewrite the branch on your behalf under the repository's contributor-takeover policy. Please choose and implement one of these reviewable outcomes:
Prompt wording or an argv assertion alone is not evidence. Your runtime results already show |
|
Maintainer takeover update (authorized): I brought this branch onto current Changes made in signed commit
Local exact-head validation before push: typecheck passed; focused local-agent tests passed (119/119); full suite and repository gates passed (787/787). I am now rerunning hosted exact-head gates before merge. |
jmagly
left a comment
jmagly
left a comment
There was a problem hiding this comment.
Exact-head re-review complete at 1e4d90fb570cf6f7b24449566d3224e13aa8eff5. The remaining session-authority blocker is resolved by default-off, explicit trusted opt-in behavior; task isolation and regression coverage are retained. Local typecheck/focused/full gates and hosted CI pass.
2 participants
Follow-up to #139/#140. That fix stopped
AgentLoop's token budget check from being blind, but the deeper cost driver is architectural: every ReAct iteration resends the whole growing transcript as a brand-newclaude -pprocess, and each of those spawns re-pays Claude Code's own CLAUDE.md/skills/MCP bootstrap tax from scratch. This uses Claude Code's--resumeto carry one session across a kill-chain phase instead, so the repeated part is billed at cache-read pricing instead of resent.Contribution Receipt
--resume) across a kill-chain phase, with a fallback to a fresh session if the resumed one is stale.npm run typecheck-> passnpm test-> pass (764/764 vitest, 11/11 ops-preflight, 19/19 model-matrix, refusal-frontier self-test)npm run doctor-> pass (30/33, 3 warnings: semgrep/promptfoo missing, API health offline with the server not running — no blockers)npm run verify-claims-> pass (27/27, no headline numbers changed)--resumerecall, the cost drop, and the stale-session failure shape; (2) a live recon-phase mission via/api/mission/startagainst an authorized external target, confirmed viapsthat the session id held across all 4 recon tasks in one phase and that a newly spawned operator's first call in the next phase carried no--resume. Both deleted/stopped after use, no logs retained.--resumefallback fires on any failure once a session id is in play, not only a confirmed-stale one; a real transient error on the resumed call pays one extra spawn before surfacing. Session scope is tied toadvancePhase()only — Codex/Hermes/OpenCode/Oh My Pi are untouched (Claude-only, same as Claude Code local-agent responses carry nousage:AgentLoop's token budget check never fires #139/fix(llm): populate usage for the Claude Code local-agent path #140). Mission-level state (the running phase/task graph) still does not survive a server restart, so a resumed session id would already be moot in that case; this PR does not change that.