Uh oh!
There was an error while loading. Please reload this page.
feat(claude-agent-sdk): continue one Claude session across executions via session_key - #409
Conversation
…ia session_key Every agent execution on this provider spawned a fresh `claude` session, so an investigate -> check -> retry loop threw away its own groundwork on each pass: the agent re-read the same files and re-derived the same hypotheses. There was no way to express "keep going in the session you already have". Add a per-agent `session_key`. Executions tagged with the same key resume one underlying session, which covers both the loop-back case (an agent continuing its own work) and the hand-off case (a later agent inheriting an earlier one's conversation). Omitting it preserves today's fresh-session-per-execution behavior, so nothing changes for existing workflows. The key is deliberately a static label rather than a value produced by a workflow step. The provider keys its own `dict[session_key, session_id]` map and does the lookup internally, so no session id passes through AgentOutput or WorkflowContext and no engine plumbing is needed to move one between steps. Guard the resume rather than passing the id straight through. `--resume` on a transcript the CLI cannot find makes it abort with ProcessError *before the agent runs*, and that is reachable in ordinary use: the first execution under a key has nothing to resume, transcripts are pruned on the CLI's own schedule, and they are stored per working directory so a moved cwd loses them. `get_session_info(sid, directory=cwd)` is cwd-scoped, returns None instead of raising, and is exported at the pinned floor, so one cheap stat covers all three cases; a miss logs a warning and starts fresh. Capture opportunistically from any message carrying a `session_id` rather than only from ResultMessage — an agent that dies mid-run is precisely the one whose context is worth resuming. `fork_session` defaults to False, so a resumed session keeps its id and the map stays stable. Persist the map through checkpoints by implementing the existing duck-typed `get_session_ids` / `set_resume_session_ids` hooks that copilot.py already exposes, which the engine and CLI pick up with no engine, CLI, or checkpoint-schema change: `copilot_session_ids` is an opaque dict[str, str] and nothing downstream interprets its keys. `checkpoint_resume` flips to True accordingly. Note the engine persists only the first provider's map, so a mixed-provider workflow keeps one — pre-existing, and a foreign key-space simply misses and starts fresh. Following the `sandbox:` precedent, `session_key` validates structurally under any provider but is consumed only by this one, with no capability flag or validator gating. It is rejected on step types that have no provider session (script, human_gate, workflow, wait, set, terminate). Sharing a key across genuinely concurrent executions would interleave turns in one session and is documented as an authoring error rather than blocked.
Adversarial review of the initial session_key commit surfaced four defects
that would each have shipped as a silent failure, plus one I mis-diagnosed
in that commit message. All were reproduced before fixing.
The resume guard was wrong in both directions. `get_session_info` answers
"does this session have an extractable summary", not "does it exist": it
reads only the first 64 KiB of the transcript, so any agent whose first
prompt exceeded that — trivial under `accumulate` context mode or with
eager-injected skills — looked unresumable forever and the feature silently
never fired (proved: a 200 KB prompt returns None while the session is
plainly resumable). In the other direction it falls back to scanning sibling
git worktrees via a `git` subprocess, so it reported a session as present
from a subdirectory the CLI then refused to resume, turning the graceful
degradation this guard exists to provide into a hard abort. Replaced with an
exact check of the path the CLI actually uses,
`<config>/projects/<project key for cwd>/<id>.jsonl`, keeping the SDK lookup
only as a cwd-verified fallback for the long-path hash case. The lookup also
moves to `asyncio.to_thread`, since that subprocess was blocking the event
loop and every concurrent agent with it.
Capture trusted too many message types. Hook frames carry a `session_id` of
their own — a `SessionStart` hook emits one before the first assistant turn
— so the map could record an id with no transcript and permanently shadow
the real session, breaking exactly the mid-run-death case the opportunistic
capture was justified by. Restricted to AssistantMessage / ResultMessage,
and moved above the interrupt and timeout checks so an interrupted agent
still records its session.
The map is now keyed by (session_key, cwd). Transcripts are stored per
directory, so two agents sharing a key under different directories cannot
share a session; keying on the label alone made them overwrite each other's
id so neither ever resumed.
The mixed-provider checkpoint claim in the previous commit message was
wrong. That collision is not pre-existing: before this branch only Copilot
exposed `get_session_ids`, so the engine's collect loop was deterministic.
Adding a second implementer made it order-dependent on whichever agent ran
first, silently dropping the other provider's map — and its cwds with it,
which flips Copilot into its "pre-cwd checkpoint" branch and disables its
own working-directory guard. Nor do the key spaces merely miss each other:
`session_key: investigate` on an agent named `investigate` collides exactly.
The engine now merges every active provider's map instead of stopping at the
first, and our keys are namespaced with the cwd embedded.
Concurrent key sharing is now rejected at validate time rather than
documented. Two parallel members with one key orphan the first session and
leave two `claude` processes appending to a single transcript; it is
provider-independent and statically decidable, and `config/validator.py`
already walks these exact groups for `concurrent_safe`.
Also: `session_key` is whitespace-stripped and rejects `{{ }}`, since it is
never rendered and a template would silently become a literal key;
`fork_session=False` is explicit because a fork would strand the map on a
dead id; the optional SDK lookups moved to their own import so a sub-floor
SDK degrades the guard instead of disabling the provider; and the
`concurrent_safe` rationale no longer claims there is no shared state.
Tests: the probe is covered against real on-disk transcripts (large prompt,
wrong directory, non-UUID), and new integration tests pin the two properties
nothing else would catch — that ProviderRegistry hands back one provider
instance across a loop-back, and that the duck-typed checkpoint hop actually
connects. Schema matrix tests moved out of the wait-specific file.
One review finding was investigated and rejected: usage is NOT double
counted across a resumed session. Measured against the real CLI, a resumed
execution reports its own tokens (2/3, num_turns 1), not the session
cumulative.The Jinja2 route form needs the step-qualified path; the bare `verify.exit_code` raises TemplateError at runtime (proved by the loop-back integration test, which routes on the corrected form). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVihSFdo3stWanwWxDtnfj
`session_key` shipped ungated, on the argument that ACA's per-agent
`sandbox:` block is also ungated. That precedent does not transfer.
`sandbox:` is provider-branded and unreachable unless you have configured
`aca`, whereas `session_key` is a bare field sitting next to `model` and
`retry` on every agent — and it was a no-op on three of five providers with
no feedback at all. Six conforming precedents point the other way
(`working_dir`, `skills`, `max_session_seconds`, `mcp_tools`,
`reasoning_effort`, `structured_output` all fail validation), and the
validator's own comment names this class: silently-dropped operational
intent.
Add `session_continuity` to ProviderCapabilities, defaulted False so the
other five descriptors need no change, and check it in
`_check_agent_capabilities` alongside its siblings — which already covers
`for_each` inline agents, so both call sites come free.
Set `checkpoint_resume` back to False. A keyed agent's session genuinely is
persisted and re-applied on resume, but that flag is a blanket promise the
startup banner reads out, and it would be false for every agent that has not
opted in — which is all of them by default. Flipping it deleted the "no
checkpoint resume" line for every claude-agent-sdk user, telling the
majority the opposite of the truth. `session_continuity` carries the
granular claim instead, and under-claiming the blanket one keeps the banner
honest.
Also implement the template rejection the schema docstring already promised:
it referenced a `_validate_session_key_is_literal` method that was never
written, so `session_key: "item-{{ _key }}"` was silently accepted as one
literal key shared by every iteration — the first thing a for_each author
reaches for. It now fails validation, and the two docs that disagreed about
this (one said rejected, one said used verbatim) now match the code.…doc claims The capability-gating commit rewrote `TestCapability` with a replace that sliced from its header to end-of-file, silently deleting the four classes appended after it: `TestSessionIdProvenance`, `TestWorkingDirScoping`, `TestMapHygiene` and `TestTranscriptProbe`. Those were precisely the tests that pinned the round-1 fixes — hook frames not shadowing the real session, the interrupt path still recording, cwd keying not stomping, malformed restore entries being skipped, and the transcript probe against real on-disk transcripts. The suite stayed green throughout because deleted tests do not fail, which is exactly why this needed catching. `_Recorder.prefix_messages` and `stop_after_prefix` survived as dead scaffolding, a visible tell. With them gone `_session_transcript_exists` had zero coverage — it is stubbed in both test files — so the guard AGENTS.md calls "load-bearing" was resting on nothing. Restored, and it is the only thing exercising the real SDK path derivation. Doc corrections, all verified against the code: - The warning claim was wrong for two of the cases each doc listed. `_resolve_resume_session` returns early when the map has no entry, so the first execution under a key and an execution under a changed `working_dir` are silent by design; only a recorded session whose transcript has since gone logs. Fixed in `docs/workflow-syntax.md`, `CHANGELOG.md` and the `session_key` schema docstring. - "`get_session_info` reads only the first 64 KiB" is wrong: it reads head *and* tail and prefers a title or last-prompt record from the tail. The conclusion is unchanged — it answers "can I summarise this", not "does this exist" — but the stated trigger is narrower than a large first prompt alone. Reworded in the provider docstring and AGENTS.md. - An integration-test docstring still credited `checkpoint_resume`, which is deliberately False; the promise belongs to `session_continuity`. Verified separately against the real CLI, unmocked: two keyed executions share one session, the real probe finds the transcript the CLI just wrote, and the second run's own reasoning recalls the first turn.
…ssions across repeated resumes Round-2 review findings, all reproduced first. The concurrency validator contradicted the provider it was guarding. Sessions are keyed by (session_key, cwd) — that was the round-1 fix — but the check keyed on session_key alone, so two parallel members under different working_dir values were rejected despite provably being separate sessions, and a for_each fanning out over per-item directories was blocked outright. That is precisely the multi-worktree pattern the cwd scoping exists to enable, and `docs/workflow-syntax.md` already promised it worked. The check now compares (key, effective working_dir) for parallel members, and skips a for_each whose working_dir varies per item. A session restored from a checkpoint was dropped from the next one: `get_session_ids` exported `_session_ids` but never `_resume_session_ids`, so "continuity survives conductor resume" held for exactly one resume. If a second checkpoint was taken before the keyed agent ran again, a later loop-back silently started cold. Both maps are now exported, with this-run entries winning. An SDK below the session-lookup floor handed the id to `--resume` unverified, which aborts the CLI before the agent runs — the exact hard failure the guard exists to prevent, and the opposite of what its comment claimed. It now starts a fresh session instead. The test that asserted the old behaviour was encoding the bug, and is inverted. Corrected a false invariant I introduced in 86fcc7d: `hermes` has exposed `get_session_ids` on main all along, keyed by bare agent name like Copilot. So the collect loop's order-dependence was NOT introduced by this branch as that commit claimed — my original "pre-existing" note was closer to right — and "providers namespace their own keys so the flat map cannot collide" is false for the copilot/hermes pair. The merge is still the right fix (it ends the order-dependence for all three), but the comment and AGENTS.md now say what is actually true. Not reproducible: a one-off failure of `test_session_recorded_when_interrupted` reported under a full-suite run. Five isolated runs, three randomised provider-suite runs, and two full randomised suite runs all pass; the tree was mid-edit when it was observed. Left as-is rather than papering over it.
Upstream moved a long way in 22 commits (v0.1.27, Pydantic AI for the Claude provider, plugins, a questions step type, output constraints, native skills for claude-agent-sdk). Seven files conflicted; three needed more than a mechanical resolution. - `schema.py`: upstream added a `questions` step type. Every other non-agent type already rejects `session_key`, and a questions step invokes no provider, so it gets the same rejection — otherwise the field would have been silently accepted on exactly one step type. Documented and tested alongside the other six. - `claude_agent_sdk.py` / `capabilities.py`: `plugins` and `session_continuity` were added to the same regions independently; both kept. Our session code auto-merged into the rewritten `execute` intact. - `workflow.py`: kept our merge-all-providers session collection, took upstream's improved fail-open logging from microsoft#367. - `AGENTS.md`: upstream pinned `setting_sources=[]` (microsoft#352), so ambient user and project hooks no longer load. Our `_SESSION_ID_MESSAGES` allowlist is still required — `TaskStartedMessage`, `StreamEvent` and plugin-contributed hook frames all still carry their own `session_id` — but the rationale cited only ambient `SessionStart` hooks, so it has been corrected rather than left to read as stale. - `CHANGELOG.md`: also corrected the "providers namespace their own entries so the merged map cannot collide" line, matching the fix already made in AGENTS.md — copilot and hermes both key by bare agent name. Upstream also added a skill-injection byte budget (microsoft#350/microsoft#363) with the bundled `conductor` skill sitting 1,696 bytes under the 128 KiB default. Our `session_key` reference docs added 2,224, pushing it over and failing eleven skills tests. The two skeleton comment blocks are trimmed to the essentials with the detail left in `docs/workflow-syntax.md`, which they link to; 130,464 bytes now, back under the limit. Verified: 5777 passed. The single remaining failure, `test_subdirectory_without_skill_md_is_reported`, reproduces on a clean origin/main worktree and is not ours. `make check` exits 0 with one more `ty` unused-ignore warning than main's five — our extra guarded SDK import, the same accepted pattern as the others. All examples validate.
Review feedback: most of this branch was comments and docs, not code. Three rounds of adversarial review left prose written at reviewers — justification for choices, and arguments against alternatives — rather than at readers. Measured against the neighbours rather than trimmed by feel. The `session_key` field docstring ran 38 lines against an `AgentDef` field median of 8 (16 for `retry`, the long tier); the AGENTS.md bullet ran 984 words against a sibling median of 204; the `capabilities.py` entry 10 lines against a field median of 3; comment blocks in `claude_agent_sdk.py` averaged above the file's own median. All are now inside their local norms. The example YAML and the provider's method docstrings were already in range and were barely touched. Net: 355 lines removed, 198 added. `src/` additions drop 419 → 340, and the comment-to-code split 50% → 39%. The bundled skill loses another 83 bytes, which matters because upstream's injection budget leaves it under 700 to spare. What went: reviewer-directed justification, the `sandbox:`-precedent argument, enumerations of rejected alternatives, and the duplicated Session Continuity section in `docs/providers/experimental.md` — nothing linked to it, its table row already carries the fact, and per-provider detail belongs in the canonical `docs/workflow-syntax.md`. What stayed, compressed to a clause or a sentence: why the transcript guard exists at all (`--resume` aborts the CLI before the agent runs), why `get_session_info` is a fallback rather than the primary check, why capture is restricted to `AssistantMessage`/`ResultMessage`, why the map is keyed by `(session_key, cwd)`, why checkpoint keys are namespaced, and why `checkpoint_resume` is False while `session_continuity` is True. Two factual errors fixed on the way through. `docs/workflow-syntax.md` and `CHANGELOG.md` still claimed every provider namespaces its checkpoint entries "so the merged result cannot collide" — false, and already corrected in AGENTS.md: Copilot and Hermes both key by bare agent name. And the rejected-step-type lists omitted `questions`, which the schema does reject. Verified: no executable line changed (AST comparison with all bare string expressions stripped, so Pydantic attribute docstrings do not mask a real edit). 5777 passed with only the pre-existing `test_subdirectory_without_skill_md_is_reported` failing, `make check` exit 0 at the same 6 warnings, all examples validate, and every markdown anchor and link target still resolves.
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Reviewed this against the cached SDK builds (0.2.82 through 0.2.134) and the bundled claude binary. The design reasoning here is careful and the docstrings are more honest than most, so most of what follows is about enforcement reaching conductor run, plus a few claims that did not survive checking.
Three items have no single line to hang them on.
Both new guards only run under conductor validate.validate_workflow_config has one caller, cli/validate.py. conductor run and resume_workflow_async call load_config and nothing else. So a session_key against copilot, claude, hermes or aca is accepted at run time and quietly does nothing, and two concurrent executions sharing a key are never stopped. The repo already treats this as a known trap: _reject_unsupported_skills, _reject_unsupported_plugins and _reject_discovery_without_native_skills all exist in executor/agent.py for this exact reason. An in-flight set of (session_key, cwd) in the provider would cover the concurrency half and the case below at the same time.
Sub-workflows slip past the concurrency guard._execute_subworkflow passes registry=self._registry, so child engines share one provider instance and one _session_ids map. A for_each with max_concurrent: 4 over a type: workflow agent whose inner agent is keyed produced three overlapping resumes of one live session, and conductor validate reported nothing. A workflow agent cannot carry a session_key itself, so the parent check never sees one, and the child is validated on its own.
Usage accounting across a resumed session is untested in both directions. The comment at line 556 states that ResultMessage.usage is a cumulative session total and points at ApiUsage.apiUsage, but that docstring belongs to the context-window TypedDict rather than to ResultMessage.usage. Before this PR the distinction could not matter, because every execution opened a new session. session_key makes "a ResultMessage for a session that already reported usage" reachable for the first time, and UsageTracker.record appends one row per execution. If the comment is right, a loop-back inflates reported cost and budget_mode: enforce can abort a run over spend that never happened. Worth a two-pass run against the live CLI and a test pinning whichever answer is correct.
Smaller things: fork_session=False is never asserted (the recorder mock simulates it, so flipping the source leaves every test green), the new example appears in no validation test, and _write_checkpoint's docstring still says "for Copilot session resume".
TestTranscriptProbe deserves a mention. Using real config dirs and real transcript files, and asserting that get_session_info returns None before showing the exact-path check overrides it, pins a real upstream quirk that a mocked test would have missed entirely.
Uh oh!
There was an error while loading. Please reload this page.
| get_session_info, | ||
| project_key_for_directory, | ||
| ) | ||
| except ImportError: # pragma: no cover - SDK below the session-lookup floor |
There was a problem hiding this comment.
I checked every cached SDK build at and above the declared pin: 0.2.82 (the exact floor), 0.2.87, 0.2.128, 0.2.132, 0.2.134. All five export both symbols from __init__.py, so no supported version sits below a "session-lookup floor".
The reachable failure is a different one, and it is worse. project_key_for_directory is re-exported from _internal.session_store, so an upstream move sets both names to None. From then on _resolve_resume_session returns None on every call and logs it at DEBUG, which Conductor never prints because it installs no logging handlers. session_continuity=True is a static class attribute, so conductor validate keeps accepting session_key and the experimental banner keeps advertising support while every keyed agent starts cold.
Two suggestions beyond the wording fix below. Derive the capability from actual availability rather than declaring it unconditionally, and warn once at construction when the symbols are missing. Also worth noting that the comment on lines 51-52 promises degradation when "an SDK missing only these" symbols is present, but a single import statement binds both or neither, so that mixed state cannot occur.
| exceptImportError: # pragma: no cover - SDK below the session-lookup floor | |
| exceptImportError: # pragma: no cover - both symbols exist at our >=0.2.82 floor |
There was a problem hiding this comment.
You are right about the import: it binds both names or neither, so I fixed that comment and took your # pragma: no cover line. I also fixed the same wrong claim inside _resolve_resume_session.
On deriving the capability, I went with a one-time logger.warning at construction instead, and I would value your view on whether that is enough. Three things made me hesitate. Every capability in the codebase is a literal that describes the provider's contract rather than the current machine — working_dir stays True when the claude binary is missing, and the descriptor still reads True when the SDK is not installed at all. get_capabilities reads the class without instantiating, which is what keeps conductor validate reproducible, so deriving from import success would let the same YAML validate on CI and fail on a laptop. And your own check across 0.2.82 to 0.2.134 bounds the risk to a future upstream move, where the real harm is silence rather than the declaration. Happy to make it dynamic if you would rather have that.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| # Each call spawns an independent subprocess. The ``session_key`` map | ||
| # is shared mutable state but is only read by agents that opted in; | ||
| # concurrent executions sharing one key are rejected by the validator. |
There was a problem hiding this comment.
The rejection cited here runs only under conductor validate. validate_workflow_config is reached from cli/validate.py alone, and conductor run calls load_config without it, so nothing re-checks concurrent key sharing on the path most runs take.
That matters more than usual because this comment is the entire justification for a safety-relevant flag. The wording it replaced (no global mutable state shared across calls) was at least self-contained.
| # Each call spawns an independent subprocess. The ``session_key`` map | |
| # is shared mutable state but is only read by agents that opted in; | |
| # concurrent executions sharing one key are rejected by the validator. | |
| # Each call spawns an independent subprocess, and the ``session_key`` | |
| # map is a plain dict mutated only from the event loop. Two executions | |
| # resuming one key concurrently is the unsafe case; ``conductor | |
| # validate`` rejects it statically, but ``conductor run`` does not. |
There was a problem hiding this comment.
Gone with a slightly different wording, because conductor run now does refuse this. The provider holds an in-flight set of (session_key, cwd) and raises on a second execution for a pair already running, so the comment says that instead. Are you fine with that?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #409 +/- ##
=======================================
Coverage ? 91.39% =======================================
Files ? 108 Lines ? 17634 Branches ? 0 =======================================
Hits ? 16117 Misses ? 1517 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review on microsoft#409. The largest finding: both new guards lived in `validate_workflow_config`, which only `cli/validate.py` calls. `conductor run` and `resume` never reach it, so a `session_key` against copilot, claude, hermes or aca was accepted and quietly did nothing, and two concurrent executions sharing a key were never stopped. The repo already treats this as a known trap — `_reject_unsupported_skills`, `_reject_unsupported_plugins` and `_reject_discovery_without_native_skills` exist in `executor/agent.py` for exactly this reason — so `_reject_unsupported_session_key` joins them, first statement of `execute`. The provider now also holds an in-flight set of `(session_key, cwd)` and refuses a second execution for a pair already running. That closes a hole `conductor validate` structurally cannot see: `_execute_subworkflow` shares one registry, so a `for_each` with `max_concurrent > 1` over a `type: workflow` step whose inner agent is keyed produced overlapping resumes of one live session, and a `workflow` agent cannot carry a `session_key` itself so the parent check never fired. `execute` became a thin wrapper around `_execute_session` because `_build_output` can raise after the existing `finally` and the interrupt path returns mid-loop — a claim released only there would leak the slot. Also from review: - `set_resume_session_ids` stored outside its `try`, so the docstring's "skipped rather than raising" was false. `[["x"],["y"]]` raised TypeError; `[1, 2]`, `"ab"` and `{"a":1,"b":2}` each stored entries violating the declared type and re-exported cleanly into every later checkpoint. - The for-each loop-variable check matched `_index` and `_key` as bare substrings, so `/var/lib/search_index` and `/srv/api_key` switched the guard off for a directory every iteration shares, while `/tmp/{{- item }}` was wrongly rejected. Now parsed with `meta.find_undeclared_variables`, which this file already uses. - Dropping `break` from the checkpoint merge loop meant one raising provider sent the whole loop to the handler and lost Copilot's map too. Each provider now has its own `try`, and the warning names the one that failed. - Deleted a `runtime_working_dir` re-binding 750 lines below the existing one. Four comments corrected against evidence rather than reasoning. The CLI does NOT refuse a cross-directory resume — I created a session at the repo root and resumed it from `src/`, and the model recalled the earlier turn. It resolves `--resume` through the cwd project dir, sibling worktrees, then a global scan, so it is wider than our guard, not narrower; the guard stays because it holds the `(session_key, cwd)` contract, but the stated reason was backwards. `ResultMessage.usage` is not a cumulative session total either (a resumed run reports `input 2 / output 4 / num_turns 1`), and the comment citing `ApiUsage.apiUsage` pointed at a context-window TypedDict; the same false claim on the `usage_tracking` line is fixed and a test now pins it. `fork_session=False` had the wrong rationale — capture overwrites the map on every frame, so a fork would follow the new id rather than strand. `session_continuity` stays a static declaration. Every capability in the codebase is a literal describing the provider's contract, not the current environment, and `get_capabilities` reads them without instantiating so `conductor validate` stays reproducible. Deriving it from import success would make the same YAML validate differently on two machines. The real risk the reviewer identified — `project_key_for_directory` moving upstream would disable the feature while logging only at DEBUG, which Conductor never prints — is met with a one-time warning at construction. CHANGELOG entries moved out of the released 0.1.27 section into Unreleased. +42 tests: the runtime rejections at both layers, the malformed-entry shapes, all four rows of the loop-variable table, usage on a resumed session, per-provider checkpoint isolation, and `fork_session` asserted on the options object rather than simulated by the mock.
Luke Ellison (lukeellison)
commented
Aug 17, 2026
Thank you — this was a very useful review. Pushed as Enforcement reaching Sub-workflows. Fixed 👍 The provider now holds an in-flight set of Usage across a resumed session. You are right that the comment was wrong, and it pointed at the wrong docstring. I ran the two passes against the live CLI: the resumed execution reports Smaller ones: On the example, I think this one is already covered: One thing I left alone and want to flag: |
Resolves two conflicts and one regression that only appears on merge. CHANGELOG.md: main released 0.1.28 through 0.1.32 since this branch last merged, so the section the entries sat under is now a shipped release. Moved both under the current, empty [Unreleased] rather than letting the textual merge file them under 0.1.31. tests/test_executor/test_agent.py: both sides appended an independent provider stub plus test class at the same point. Kept both; each stub carries its own validate_connection/close, which the textual merge had collapsed onto whichever class happened to come last. claude_agent_sdk.py auto-merged, but the reconciliation was the risky one and was checked by hand: main added _read_usage (microsoft#427, stop billing cached tokens twice) inside execute's message loop, and this branch moved that loop into _execute_session. Both call sites landed in the relocated function with the cache counters and last_call_input_tokens intact, and this branch's session-id capture still runs ahead of the interrupt check. skill_injection.max_bytes: 128KB -> 160KB. This is the regression neither side could see alone. The bundled conductor skill was ~117KB when the ceiling was chosen; this branch added ~1KB of session_key documentation and main added ~1.1KB of mid-run guidance documentation (microsoft#400), which together carry the rendered content to 131,923 bytes against a 131,072 limit. Eleven tests failed, and a claude or hermes agent enabling the shipped skill would have hard-failed rather than warned -- the exact inversion the two defaults exist to prevent. warn_bytes stays at 64KB so the combination still warns. The stale ~117KB figure is corrected in AGENTS.md, docs/workflow-syntax.md and the schema docstrings, along with one dependent claim that described 260KB as twice the default. Gates on the merged tree: ruff clean, ty clean, 7559 passed / 45 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The fixture interpolates a tmp_path into a double-quoted YAML scalar, and a Windows path takes its backslashes into YAML's escape handling: the runner's C:\Users\runneradmin\... becomes an invalid \U escape and the document fails to scan before any of the test runs. Seven integration tests failed on windows-latest for that reason alone and passed everywhere else. as_posix() covers both halves of the problem -- the scalar parses, and the forward-slash path is what the `sh` in the script step wants anyway. This surfaced only now because the branch's conflicts had been blocking CI from producing a merge ref, so no Windows job had run against these tests since they were written. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Uh oh!
There was an error while loading. Please reload this page.
Sorry for not opening an issue for this - I just needed it soon so happy to work off my own branch for a while and then leave this PR for your review in case you found it useful!
Motivation
Every agent execution on the
claude-agent-sdkprovider starts a newclaudesession. An agent that runs a second time reads the same files again and
repeats work it already did.
One workflow shape shows the cost. An agent investigates, a script step checks
the result, and a failed check routes back to the agent. Each pass starts
cold. Today there is no way to say "continue the session you already have".
Approach
This PR adds an optional per-agent field,
session_key. Executions that sharea key continue one Claude session. This covers a loop-back to the same agent,
and a hand-off to a later agent that declares the same key.
The key is a static label, not a value that a workflow step produces. The
provider maps the key to the real session id internally. No session id travels
through the workflow context, so the engine needs no new plumbing.
The provider keys sessions by the label and the working directory together,
because the CLI stores transcripts per directory. Conductor writes the map to
checkpoints, so continuity survives
conductor resume.The provider confirms that the transcript is on disk before it resumes.
--resumeon a transcript the CLI cannot find stops the CLI before the agentruns. Without this guard, a pruned transcript becomes a hard failure instead
of a fresh session.
A new
session_continuitycapability gates the field. Asession_keyon aprovider that cannot honor it is a
conductor validateerror. The validatoralso rejects a key shared by executions that run at the same time, because
two
claudeprocesses cannot append to one transcript.Alternatives considered
A
session_scopeenum (workflow,agent,item,none), modelled onthe
acaprovider'sidentifier_scope. It covers more cases, but it adds aworkflow-level default and four values that no use case asked for. One key per
agent is smaller and solves the same problem.
Exposing the real session id in the workflow context, with a templated
resume_session:field. This is more flexible, because you can then resume asession from an earlier run. It also needs a new
AgentOutputfield, contextplumbing, and engine changes. It composes on top of this PR, so nothing here
blocks it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RVihSFdo3stWanwWxDtnfj