From 036bf3a9afb223644d5a3ec5e21eea1dbd7ce26b Mon Sep 17 00:00:00 2001 From: daniel Date: Wed, 30 Sep 2026 20:59:43 +0100 Subject: [PATCH 1/3] feat: depth-free delegation, recoverable subagents, and resilient ChatGPT transport Delegation: - Remove the subagent depth limit and depth-dependent prompts; bound live subagents per delegation tree with shared slot files instead. - `fork` without `subagent` copies the current conversation; framing messages tell children what they inherited. Delegation guidance lives in the tool descriptions, including what forks and fresh subagents cost. - Merge `steer` into `prompt`: it accepts an ID or value and delivers into a running turn when supported, otherwise as the next turn, and returns once the turn that handled the message ends. - A failed turn, including the first, leaves a live subagent idle and promptable; the error names the subagent, its recorded cause, and how to continue it. - Forks share their parent's prompt-cache key; fresh subagents use their own. Observability: - Fatal error records carry the typed provider failure. - Forward provider retry observers through Kit's session wrappers, so retries reach loop observers and `~/.kit/errors//retries.jsonl`. - Compose spill markers name the omitted byte range and the artifact offset to read it from. ChatGPT transport: - Use agentkit-provider-openai 0.10.12 (progress-based stall detection, fresh-socket resends including after visible output with supersession, cache-key routing) and agentkit-tool-compose 0.10.12 with runlet 0.6.1. - Discover the context window in the background with retries instead of blocking session start; accept re-authentication to the same account. --- Cargo.lock | 11 +- Cargo.toml | 14 +- docs/user/subagents-and-acp-harnesses.md | 40 +- fixtures/mock-acp-v2.py | 3 + src/acp_child.rs | 52 ++ src/acp_child/prompt.rs | 2 +- src/acp_child/protocol.rs | 5 + src/compose_output.rs | 74 +- src/fatal.rs | 150 +++- src/protocols/acp.rs | 13 +- src/protocols/acp/v2.rs | 6 +- src/provider/adapter.rs | 19 +- src/provider/chatgpt.rs | 323 +++++---- src/provider/chatgpt/catalog_tests.rs | 159 +++++ src/provider/chatgpt/websocket_tests.rs | 90 ++- src/provider/chatgpt_image_tests.rs | 2 +- src/runtime.rs | 207 +++--- src/runtime/tests.rs | 101 +-- src/runtime/voice_state.rs | 31 +- src/session.rs | 57 +- src/tools/mod.rs | 6 +- src/tools/subagent.rs | 860 +++++++++++++++++------ src/tools/subagent/recovery.rs | 10 +- src/tools/subagent/tests.rs | 264 +++---- src/transcript.rs | 40 +- 25 files changed, 1752 insertions(+), 787 deletions(-) create mode 100644 src/provider/chatgpt/catalog_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 8518b383..b39984bd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -309,7 +309,8 @@ dependencies = [ [[package]] name = "agentkit-provider-openai" version = "0.10.12" -source = "git+https://git@github.com/danielkov/agentkit.git?rev=04148b4fe88ebfbe9ba79762dcf272bd5a7b8f58#04148b4fe88ebfbe9ba79762dcf272bd5a7b8f58" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59949238158745f1c53d3f31057dda3b7a643581f067b91b993075e6b3b09511" dependencies = [ "agentkit-adapter-completions", "agentkit-core", @@ -357,9 +358,9 @@ dependencies = [ [[package]] name = "agentkit-tool-compose" -version = "0.10.11" +version = "0.10.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2bd493e7dd9ed206d203ec37c4ff5c65bb97e5a77b5f0f385a366d5e75064c0c" +checksum = "207f07133cbe21cd63e2c0d46952d3bdab378d3c84bc1e11878f2f489a8b1c3b" dependencies = [ "agentkit-core", "agentkit-tools-core", @@ -4567,9 +4568,9 @@ dependencies = [ [[package]] name = "runlet" -version = "0.6.0" +version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "057e0864428c5a79a68683942d3750d05e9ffae541aa0185fde9ae1c87eca100" +checksum = "b710066183389b1bfa03d4bfd28c9558b582f9f584ca251556a639461e3efa58" dependencies = [ "hex", "regex", diff --git a/Cargo.toml b/Cargo.toml index 839c4e70..837bbe0e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,7 +47,7 @@ agentkit-plugins = "=0.10.7" agentkit-provider-openai = "=0.10.12" agentkit-provider-openrouter = "=0.10.8" agentkit-task-manager = "=0.10.7" -agentkit-tool-compose = { version = "=0.10.11", default-features = false, features = ["runlet"] } +agentkit-tool-compose = { version = "=0.10.12", default-features = false, features = ["runlet"] } agentkit-tool-skills = "=0.10.8" agentkit-tools-core = "=0.10.5" arboard = { version = "=3.6.1", default-features = false, features = ["image-data"], optional = true } @@ -81,7 +81,7 @@ rpassword = "=7.5.4" serde = { version = "=1.0.229", features = ["derive"] } serde_json = "=1.0.151" shlex = { version = "=2.0.1", optional = true } -runlet = "=0.6.0" +runlet = "=0.6.1" sha2 = "=0.11.0" subtle = "=2.6.1" tar = { version = "=0.4.46", default-features = false } @@ -125,7 +125,6 @@ jsonwebtoken = { version = "=11.0.0", default-features = false, features = ["aws tokio = { version = "=1.53.1", features = ["test-util"] } [patch.crates-io] -agentkit-provider-openai = { git = "https://git@github.com/danielkov/agentkit.git", rev = "04148b4fe88ebfbe9ba79762dcf272bd5a7b8f58" } agentkit-loop = { git = "https://github.com/danielkov/agentkit.git", rev = "bec9dcc45ee0f436d538286bc220b16dc19fd5f3" } agentkit-acp = { git = "https://github.com/daviddanialy/agentkit.git", rev = "6d519ed1e93e28e54ba1cc18889534e2e8337181" } agent-client-protocol = { git = "https://github.com/danielkov/rust-sdk.git", rev = "423ba77cd555a09f68472b93c142c8f0baaabf43" } @@ -137,15 +136,6 @@ agentkit-core = "=0.10.5" agentkit-task-manager = "=0.10.7" agentkit-tools-core = "=0.10.5" -# Keep the provider fix on a distinct public HTTPS source so its workspace -# dependencies reuse the existing registry packages and reviewed loop pin. -# The git username distinguishes Cargo source IDs; anonymous fetch still works. -[patch."https://git@github.com/danielkov/agentkit.git"] -agentkit-adapter-completions = "=0.10.9" -agentkit-core = "=0.10.5" -agentkit-http = "=0.10.7" -agentkit-loop = { git = "https://github.com/danielkov/agentkit.git", rev = "bec9dcc45ee0f436d538286bc220b16dc19fd5f3" } - # Deny also in test builds; only test-only scopes may relax ergonomics. Crate # roots additionally forbid these in production so local allows cannot evade it. [lints.clippy] diff --git a/docs/user/subagents-and-acp-harnesses.md b/docs/user/subagents-and-acp-harnesses.md index b2ade669..45161b45 100644 --- a/docs/user/subagents-and-acp-harnesses.md +++ b/docs/user/subagents-and-acp-harnesses.md @@ -90,38 +90,28 @@ return { main: second.output, alternative: branch.output } Each successful turn returns a session value with `id`, `name`, `output`, and `generation`. `subagent` creates an ID at generation 1. `prompt` keeps that ID and name while incrementing its generation. `fork` creates a different ID and uses its own preferred or fallback name; its generation is one greater than the supplied source value, and it does not advance the source session. Close a session with either `close(value)` or `close({ id: value.id })`; the latter is useful when only an ID is available. Closing an unknown ID fails with `unknown subagent session`. Kit sends ACP `session/close` when the harness advertises it. Explicit `close` also sends `session/delete` when advertised, removing the discarded branch’s persistent history after closing it. Delete failures are reported rather than silently ignored. Process shutdown and internal cleanup do not delete persistent history; this preserves completed child sessions for restart recovery. A standalone process without that capability is terminated when its handle is dropped. If native-fork siblings share a process and the harness cannot close one logical session, `close` fails rather than claiming success or disrupting the siblings. -Always pass the latest completed value back to `prompt` or `fork`. Reusing an older value fails with `stale subagent generation N; current generation is M`. This prevents two continuations from silently racing on one session. Prompt and fork calls on an individual ACP session are serialized, while separate forked sessions can be prompted concurrently. Steering injects guidance into a working turn without waiting for that turn to finish. +`prompt` accepts the subagent's ID or any value returned for it, and always targets the session's current state. `fork` copies a completed turn, so pass it the latest completed value; an older value fails with `stale subagent generation N; current generation is M`. Prompt and fork calls on an individual ACP session are serialized, while separate forked sessions can be prompted concurrently. The optional `name` argument is preferred on `subagent` and `fork`; `prompt` has no naming input and preserves the session name. The optional `harness`, `model`, and `cwd` arguments belong only on `subagent`. `harness` overrides the user's configured harness preference. `model` selects an exact model value ID advertised by that harness through its ACP session configuration, or a model alias configured for that harness. `cwd` selects the new subagent's working directory; relative paths resolve from Kit's working directory, and missing paths or non-directories fail before startup. Omit an argument to retain its configured default. `prompt` and `fork` retain the original session's harness, model, and working directory. An explicit model fails before the first prompt if the harness does not advertise a selectable `model` option or rejects the value. -## Steer a working subagent +## Message a subagent in any state -Use `steer({ id, prompt })` to inject guidance into an existing working turn, -without cancelling it or starting a new turn. While the originating `compose` -is backgrounded, call `subagents({})` in a separate compose to find the child's -immutable ID and confirm its status is `working`. Then use that ID: +`prompt({ subagent, prompt })` delivers a message however the subagent's current state allows, and returns the subagent's value once the turn that handled the message ends: + +- An idle subagent, including one whose last turn failed, starts a new turn. +- A working subagent receives the message within its current turn when its harness supports ACP v2 `steer` injection. The call then returns that turn's value. +- A working subagent without steering support, or one that is still starting or being forked, receives the message in a new turn after its current work ends. + +An `output_schema` applies to a new turn only, so a prompt with one always waits for the current turn to end. While the originating `compose` is backgrounded, `subagents({})` lists the IDs and statuses of working children: ```text -return steer({ - id: "s-…", +return prompt({ + subagent: "s-…", prompt: "Keep the change limited to the parser; do not modify the public API." }) ``` -The prompt accepts the same text or ACP content-block input as `prompt`. -Steering requires ACP v2 and a child that advertises `steer` support. Starting, -idle, retired, and fork-reserved sessions are rejected, as are unknown IDs. -Unsupported peers return an error: Kit does not fall back to cancellation or -re-prompting. Use `prompt` with the latest completed handle for an idle child. - -The returned value is the child's acceptance receipt, **not proof that the -injection was delivered, applied, or finished**. Steering does not wait for idle -or change the turn, generation, or reusable handle. The original backgrounded -compose remains responsible for returning the completed turn's output. The -child can finish or close between listing and steering, so a working listing -does not guarantee acceptance. If acknowledgement times out, delivery is unknown; -Kit does not cancel the original turn or retry the injection. Dropping the -steering caller also does not revoke an injection already sent to the child. +The prompt accepts text or ACP content blocks. Unknown and retired IDs fail. Cancelling the call stops waiting, but does not revoke a message already injected into a working turn. ## Inspect display names @@ -200,7 +190,7 @@ Persistent parent sessions record their direct children’s ACP session IDs, har Recovery reattaches the same mutable child session, not a snapshot or a new branch. It is not the generic immutable fork fallback discussed in #12. Interrupted turns are not automatically retried, and explicitly closed children are never restored—even if remote history deletion failed. Existing transcripts without child records remain readable but cannot reconstruct their old child handles. New child-state records require a reader that understands the newer transcript schema. -For v2 children, Kit waits for an idle `state_update` after prompt acceptance before returning output. Its stop reason uses the same success, cancellation, refusal, and request-limit handling as v1. Steering requires ACP v2 with advertised `steer` support. An omitted reason permits normal completion; an unknown reason reports an error rather than success. Whole-message updates replace text by message ID; omitted content preserves text, empty or null content clears it, and later chunks append. +For v2 children, Kit waits for an idle `state_update` after prompt acceptance before returning output. Its stop reason uses the same success, cancellation, refusal, and request-limit handling as v1. Delivering a `prompt` into a working turn requires ACP v2 with advertised `steer` support; otherwise the message waits for the next turn. An omitted reason permits normal completion; an unknown reason reports an error rather than success. Whole-message updates replace text by message ID; omitted content preserves text, empty or null content clears it, and later chunks append. For generic v2 children, recovery uses `session/resume` with `replayFrom: {"type": "start"}`; v1 children must advertise `loadSession` and are reattached with `session/load`. Built-in Kit children use their persistent-session launch path. Replay completes before the next prompt and never replaces the handle’s last-turn output. Kit uses only IDs recorded by the owning parent; it does not scan and adopt unrelated child sessions. If the harness was removed, cannot load sessions, or lost its history, reconnect fails without creating a replacement session. Restore the harness configuration or explicitly close the obsolete handle. Close during an in-progress reconnect reports an error without retiring the handle; retry after startup completes or is cancelled. @@ -302,11 +292,11 @@ Nested agents cannot collect interactive input. Kit answers child ACP `elicitati Cancelling an outer turn propagates to nested work. For a dispatched prompt, Kit sends ACP `session/cancel` and allows up to five seconds for the child to settle; a child that does not settle is terminated. Cancellation while starting, waiting for the session lock, prompting, or forking returns a cancelled tool result. A cancelled fork or a fork that exceeds its 30-second deadline sends `$/cancel_request` for the in-flight request. Kit still waits for a late fork response to clean up any created session before releasing source-session serialization. A `session/close` request that exceeds its five-second deadline also sends `$/cancel_request` before its response is discarded. Protocol cancellation is advisory; it does not guarantee that the child stopped or rolled back the operation. -`end_turn` and `max_tokens` are successful completed turns. In particular, a max-token response returns its partial output and remains reusable. `cancelled`, refusal (`nested agent refused the prompt`), `max_turn_requests` (`nested agent reached its turn-request limit`), protocol errors, and unknown stop reasons are failures. Once a `prompt` continuation has been dispatched and fails, Kit retires that session because its transcript may have changed; retry by starting a new subagent rather than reusing the old value. Reuse can report `unknown subagent session`, `subagent session is retired`, or `nested agent process is no longer running`. +`end_turn` and `max_tokens` are successful completed turns. In particular, a max-token response returns its partial output and remains reusable. `cancelled`, refusal (`nested agent refused the prompt`), `max_turn_requests` (`nested agent reached its turn-request limit`), protocol errors, and unknown stop reasons are failures. A failed turn, including the first turn of a new `subagent` or `fork`, leaves a live child idle with its conversation and edits intact. The error names the subagent's ID, the cause recorded in its fatal error log, and how to continue it with `prompt`. Kit retires a session only when its child process is no longer running; reuse then reports `unknown subagent session`, `subagent session is retired`, or `nested agent process is no longer running`. ## Limits and troubleshooting -Kit currently permits nesting to depth 2 and at most 120 live parent-owned subagent sessions per main session. At the maximum depth, Kit omits `subagent` and `fork` from the compose catalog because neither operation can succeed there; the runtime depth check remains as a fallback. Exceeding the depth or capacity bounds reports `subagent depth limit (2) reached` or `live subagent session limit (120) reached`. Use `subagents({})` to inspect retained sessions and `close` to release sessions that are no longer needed. Closed, failed, or explicitly closed children no longer consume capacity. +Subagents may nest to any depth. At most 120 subagent sessions may be live at once per parent and across a whole delegation tree; exceeding either reports `live subagent session limit (120) reached` or `subagent limit for this delegation tree (120) reached`. Use `subagents({})` to inspect retained sessions and `close` to release sessions that are no longer needed. Closed, failed, or explicitly closed children no longer consume capacity. ACP startup must complete within 30 seconds. Native `session/fork` must also answer within 30 seconds. Common diagnostics include: diff --git a/fixtures/mock-acp-v2.py b/fixtures/mock-acp-v2.py index 19927779..5c8ec1a8 100644 --- a/fixtures/mock-acp-v2.py +++ b/fixtures/mock-acp-v2.py @@ -158,6 +158,9 @@ def prompt(request): text = selected_models.get(session_id, model_ids[0]) if "MOCK_STRUCTURED_OUTPUT" in text: text = json.dumps({"approved": True, "reason": "mock approved"}) + if "MOCK_TURN_ERROR" in text: + send({"jsonrpc": "2.0", "method": "session/update", "params": {"sessionId": session_id, "update": {"sessionUpdate": "state_update", "state": "idle", "stopReason": "_error"}}}) + return if "MOCK_REFUSAL" in text: send({"jsonrpc": "2.0", "method": "session/update", "params": {"sessionId": session_id, "update": {"sessionUpdate": "state_update", "state": "idle", "stopReason": "refusal"}}}) return diff --git a/src/acp_child.rs b/src/acp_child.rs index 498b6425..c2b99284 100644 --- a/src/acp_child.rs +++ b/src/acp_child.rs @@ -340,6 +340,12 @@ impl AcpHarnesses { } config.credential_storage.append_cli_args(&mut command); config.telemetry.append_cli_args(&mut command); + if let Some(slots) = &config.tree_slots { + command.env(crate::tools::subagent::TREE_SLOTS_ENV, slots); + } + if let Some(key) = &config.prompt_cache_key { + command.env(crate::tools::subagent::PROMPT_CACHE_KEY_ENV, key); + } if let Some(api_key) = &config.openrouter_api_key { command.env("OPENROUTER_API_KEY", api_key.as_str()); } @@ -422,6 +428,10 @@ pub(crate) struct ChildConfig { /// Immediate owning Kit subagent, present only inside a nested Kit runtime. pub parent_id: Option, pub parent_name: Option, + /// Slot directory bounding live subagents across the whole delegation tree. + pub tree_slots: Option, + /// Prompt cache key shared by every session in one delegation tree. + pub prompt_cache_key: Option, } impl ChildConfig { @@ -441,6 +451,16 @@ impl ChildConfig { self } + pub(crate) fn with_tree_slots(mut self, directory: PathBuf) -> Self { + self.tree_slots = Some(directory); + self + } + + pub(crate) fn with_cache_lineage(mut self, key: Option) -> Self { + self.prompt_cache_key = key; + self + } + pub(crate) fn with_parent_context(mut self, id: String, name: String) -> Self { self.parent_id = Some(id); self.parent_name = Some(name); @@ -2740,6 +2760,8 @@ mod tests { default_harness: BUILTIN_HARNESS.into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; for resume in [false, true] { let command = harnesses @@ -2843,6 +2865,8 @@ mod tests { default_harness: "acp.external".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let command = harnesses.spawn("acp.external", &config, None, 1).unwrap(); assert!( @@ -3221,6 +3245,8 @@ for line in sys.stdin: default_harness: "acp.broken".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let result = ChildSession::start( config, @@ -3307,6 +3333,8 @@ for line in sys.stdin: default_harness: "acp.broken".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let result = ChildSession::start( config, @@ -3371,6 +3399,8 @@ for line in sys.stdin: default_harness: "acp.exits".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let error = match ChildSession::start( @@ -3430,6 +3460,8 @@ for line in sys.stdin: default_harness: "acp.other".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let command = harnesses.spawn("acp.other", &config, None, 0).unwrap(); assert_eq!(command.as_std().get_program(), "agent binary"); @@ -3484,6 +3516,8 @@ for line in sys.stdin: default_harness: BUILTIN_HARNESS.into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let command = harnesses .spawn("acp.kit", &config, Some(("session", true)), 2) @@ -3618,6 +3652,8 @@ for line in sys.stdin: default_harness: "acp.broken".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let starts = (0..64) .map(|_| { @@ -3686,6 +3722,8 @@ for line in sys.stdin: default_harness: "acp.mock".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let base = ChildSession::start( config, @@ -3765,6 +3803,8 @@ for line in sys.stdin: default_harness: "acp.mock".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let base = ChildSession::start( config, @@ -3929,6 +3969,8 @@ for line in sys.stdin: default_harness: "acp.mock".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; ChildSession::start( config, @@ -4250,6 +4292,8 @@ for line in sys.stdin: default_harness: "acp.mock".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let base = ChildSession::start( config.clone(), @@ -4518,6 +4562,8 @@ for line in sys.stdin: default_harness: "acp.mock".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let base = ChildSession::start( config, @@ -4648,6 +4694,8 @@ for line in sys.stdin: default_harness: "acp.mock".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }, "acp.mock".into(), None, @@ -4789,6 +4837,8 @@ for line in sys.stdin: default_harness: "acp.mock".into(), parent_id: None, parent_name: None, + tree_slots: None, + prompt_cache_key: None, }; let base = ChildSession::start( config, @@ -5245,6 +5295,8 @@ for line in sys.stdin: default_harness: BUILTIN_HARNESS.into(), parent_id: parent_id.map(str::to_owned), parent_name: parent_name.map(str::to_owned), + tree_slots: None, + prompt_cache_key: None, } } diff --git a/src/acp_child/prompt.rs b/src/acp_child/prompt.rs index 753b1df9..fb05ad4d 100644 --- a/src/acp_child/prompt.rs +++ b/src/acp_child/prompt.rs @@ -6,7 +6,7 @@ use serde_json::{Map, Value}; use super::ChildError; -#[derive(Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize)] #[serde(untagged)] pub(crate) enum ChildPrompt { Text(String), diff --git a/src/acp_child/protocol.rs b/src/acp_child/protocol.rs index 5ae60a44..13cdb296 100644 --- a/src/acp_child/protocol.rs +++ b/src/acp_child/protocol.rs @@ -475,6 +475,11 @@ pub(super) fn completion(reason: Option) -> Result v1::StopReason::MaxTurnRequests, Some(v2::StopReason::Refusal) => v1::StopReason::Refusal, Some(v2::StopReason::Cancelled) => v1::StopReason::Cancelled, + Some(v2::StopReason::Other(reason)) if reason == "_error" => { + return Err(Error::into_internal_error(std::io::Error::other( + "nested agent turn failed", + ))); + } Some(_) => { return Err(Error::into_internal_error(std::io::Error::other( "nested agent returned an unknown stop reason", diff --git a/src/compose_output.rs b/src/compose_output.rs index 84390f73..a93d983c 100644 --- a/src/compose_output.rs +++ b/src/compose_output.rs @@ -129,18 +129,22 @@ async fn guard_text( }; // Artifact storage must not turn an already-executed tool into a failed // tool call: retrying that call could duplicate its side effects. - let marker = if artifact.is_some() { - format!( - "\n...[compose output spilled: {original_bytes} bytes; read with artifact(path)]...\n" - ) - } else { - format!( - "\n...[tool completed; output truncated: {original_bytes} bytes; artifact storage failed]...\n" - ) + let stored = artifact.is_some(); + let marker = |start: usize, end: usize| { + if stored { + format!( + "\n...[compose output spilled: bytes {start}..{end} of {original_bytes} omitted here; read them with artifact({{path, offset: {start}}})]...\n" + ) + } else { + format!( + "\n...[tool completed; output truncated: bytes {start}..{end} of {original_bytes} omitted here; artifact storage failed]...\n" + ) + } }; + let marker_bytes = marker(original_bytes, original_bytes).len(); let mut preview_budget = budget; loop { - let preview = preview(&body, &marker, preview_budget); + let preview = preview(&body, marker, marker_bytes, preview_budget); let replacement = Value::Object(Map::from_iter([ ("preview".into(), Value::from(preview)), ("artifact".into(), Value::from(artifact.as_deref())), @@ -161,7 +165,7 @@ async fn guard_text( .checked_div(replacement_bytes) .unwrap_or(0) .min(preview_budget.saturating_sub(1)) - .max(marker.len()); + .max(marker_bytes); if next_budget >= preview_budget { return Err(ToolError::Internal( "compose spill metadata exceeds the model output budget".into(), @@ -171,15 +175,21 @@ async fn guard_text( } } -fn preview(value: &str, marker: &str, budget: usize) -> String { - let remaining = budget.saturating_sub(marker.len()); +/// `marker_bytes` bounds the marker for any omitted range of `value`. +fn preview( + value: &str, + marker: impl Fn(usize, usize) -> String, + marker_bytes: usize, + budget: usize, +) -> String { + let remaining = budget.saturating_sub(marker_bytes); let head_budget = remaining / 2; let tail_budget = remaining - head_budget; + let head = prefix(value, head_budget); + let tail = suffix(value, tail_budget); format!( - "{}{}{}", - prefix(value, head_budget), - marker, - suffix(value, tail_budget) + "{head}{}{tail}", + marker(head.len(), value.len() - tail.len()) ) } @@ -293,4 +303,36 @@ mod tests { assert!(serde_json::to_vec(&output).unwrap().len() <= MAX_MODEL_OUTPUT_BYTES); assert_eq!(std::fs::read_to_string(artifact).unwrap(), expected); } + + #[tokio::test] + async fn spill_marker_names_the_omitted_artifact_range() { + let directory = tempfile::tempdir().unwrap(); + let text = (0..4000) + .map(|line| format!("line {line}\n")) + .collect::(); + let expected = serde_json::to_string(&json!({ "stdout": text })).unwrap(); + + let output = guard( + directory.path(), + ToolOutput::structured(json!({ "stdout": text })), + ) + .await + .unwrap(); + let ToolOutput::Structured(output) = output else { + panic!("guard returned non-structured output"); + }; + let preview = output["preview"].as_str().unwrap(); + let (head, rest) = preview + .split_once("\n...[compose output spilled: bytes ") + .unwrap(); + let (range, rest) = rest.split_once(" of ").unwrap(); + let (start, end) = range.split_once("..").unwrap(); + let (start, end) = ( + start.parse::().unwrap(), + end.parse::().unwrap(), + ); + let tail = rest.split_once("]...\n").unwrap().1; + assert!(rest.contains(&format!("offset: {start}"))); + assert_eq!(format!("{head}{}{tail}", &expected[start..end]), expected); + } } diff --git a/src/fatal.rs b/src/fatal.rs index e84e8465..ab7c3cfe 100644 --- a/src/fatal.rs +++ b/src/fatal.rs @@ -56,6 +56,8 @@ struct FatalRecord { deserialize_with = "deserialize_span_context" )] span_context: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + failure: Option, } fn deserialize_span_context<'de, D: serde::Deserializer<'de>>( @@ -250,6 +252,10 @@ pub(crate) fn record_loop_error( let Some((kind, code, message, diagnostics)) = classify(error) else { return Ok(None); }; + let failure = match error { + LoopError::ProviderFailure(failure) => Some(failure.as_ref()), + _ => None, + }; write_default( session_id, surface, @@ -257,10 +263,50 @@ pub(crate) fn record_loop_error( code, &message, diagnostics.as_ref(), + failure, ) .map(Some) } +/// Summarizes the newest fatal record of `session_id` without prompt content. +pub(crate) fn latest_cause(session_id: &str) -> Option { + crate::session::validate_id(session_id).ok()?; + let home = std::env::var_os("HOME").filter(|home| !home.is_empty())?; + latest_cause_in(&PathBuf::from(home).join(".kit/errors"), session_id) +} + +fn latest_cause_in(base: &Path, session_id: &str) -> Option { + let directory = base.join(session_id); + let path = fs::read_dir(&directory) + .ok()? + .filter_map(Result::ok) + .map(|entry| entry.path()) + .filter(|path| { + path.extension() + .is_some_and(|extension| extension == "json") + }) + .max_by_key(|path| { + path.file_stem() + .and_then(|stem| stem.to_str()) + .and_then(|stem| stem.split('-').nth(1)) + .and_then(|millis| millis.parse::().ok()) + })?; + let record: FatalRecord = serde_json::from_slice(&fs::read(&path).ok()?).ok()?; + let mut cause = format!("{} ({})", record.message, record.code); + if let Some(failure) = record.failure { + cause.push_str(&format!("; reason {:?}", failure.reason)); + if let Some(last) = failure.last_attempt_reason { + cause.push_str(&format!(", last attempt {last:?}")); + } + if let Some(status) = failure.upstream.http_status { + cause.push_str(&format!(", HTTP {status}")); + } + cause.push_str(&format!(", {} attempts", failure.accounting.attempts)); + } + cause.push_str(&format!("; log {}", path.display())); + Some(cause) +} + pub(crate) fn record_runtime_error( session_id: &str, surface: Surface, @@ -273,6 +319,7 @@ pub(crate) fn record_runtime_error( canonical_code(code), "runtime failed before the session could continue", None, + None, ) } @@ -419,6 +466,48 @@ fn canonical_code(code: &str) -> &str { } } +/// Appends provider retry and stall events to the session's error directory. +pub(crate) struct RetryLog; + +impl agentkit_loop::LoopObserver for RetryLog { + fn handle_event(&self, event: agentkit_loop::ObservedEvent) { + let agentkit_loop::AgentEvent::ProviderRetry(retry) = &event.event else { + return; + }; + let Some(home) = std::env::var_os("HOME").filter(|home| !home.is_empty()) else { + return; + }; + let directory = PathBuf::from(home) + .join(".kit/errors") + .join(event.session_id.to_string()); + if create_private_directory(&directory).is_err() { + return; + } + #[derive(Serialize)] + struct RetryLogEntry<'a> { + at_ms: u128, + event: &'a agentkit_loop::ProviderRetryEvent, + } + let entry = RetryLogEntry { + at_ms: SystemTime::now() + .duration_since(UNIX_EPOCH) + .map_or(0, |elapsed| elapsed.as_millis()), + event: retry, + }; + let Ok(line) = serde_json::to_string(&entry) else { + return; + }; + let _ = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(directory.join("retries.jsonl")) + .and_then(|mut file| { + use std::io::Write; + writeln!(file, "{line}") + }); + } +} + fn write_default( session_id: &str, surface: Surface, @@ -426,11 +515,12 @@ fn write_default( code: &str, message: &str, diagnostics: Option<&TransportDiagnostics>, + failure: Option<&agentkit_loop::ProviderFailure>, ) -> Result { let home = std::env::var_os("HOME") .filter(|home| !home.is_empty()) .ok_or_else(|| "HOME is unset; cannot store fatal error log".to_owned())?; - write_in_with_diagnostics( + write_record( &PathBuf::from(home).join(".kit/errors"), session_id, surface, @@ -438,9 +528,11 @@ fn write_default( code, message, diagnostics, + failure, ) } +#[cfg(test)] fn write_in_with_diagnostics( base: &Path, session_id: &str, @@ -449,6 +541,29 @@ fn write_in_with_diagnostics( code: &str, message: &str, diagnostics: Option<&TransportDiagnostics>, +) -> Result { + write_record( + base, + session_id, + surface, + kind, + code, + message, + diagnostics, + None, + ) +} + +#[allow(clippy::too_many_arguments)] +fn write_record( + base: &Path, + session_id: &str, + surface: Surface, + kind: &str, + code: &str, + message: &str, + diagnostics: Option<&TransportDiagnostics>, + failure: Option<&agentkit_loop::ProviderFailure>, ) -> Result { crate::session::validate_id(session_id)?; let occurred_at_ms = SystemTime::now() @@ -472,6 +587,7 @@ fn write_in_with_diagnostics( message: bounded(message), diagnostics: diagnostics.filter(|value| value.valid()).cloned(), span_context: crate::telemetry::error_spans::snapshot(&tracing::Span::current()), + failure: failure.copied(), }; let mut bytes = serde_json::to_vec_pretty(&record) .map_err(|error| format!("could not encode fatal error log: {error}"))?; @@ -888,6 +1004,38 @@ mod tests { assert!(diagnostics.is_none()); } + #[test] + fn provider_failure_records_its_typed_cause() { + use agentkit_loop::{ProviderFailure, ProviderFailureReason, ProviderRoute}; + + let root = tempfile::tempdir().unwrap(); + let failure = ProviderFailure { + route: ProviderRoute::OpenAiResponses, + reason: ProviderFailureReason::RetryExhausted, + last_attempt_reason: Some(ProviderFailureReason::IdleTimeout), + upstream: Default::default(), + accounting: Default::default(), + }; + let path = super::write_record( + root.path(), + "session-1", + Surface::Acp, + "provider", + "provider_error", + "provider request failed", + None, + Some(&failure), + ) + .unwrap(); + let record: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(path).unwrap()).unwrap(); + assert_eq!(record["message"], "provider request failed"); + assert_eq!( + serde_json::from_value::(record["failure"].clone()).unwrap(), + failure + ); + } + #[test] fn typed_provider_failures_preserve_fatal_and_cancellation_behavior() { use agentkit_loop::{ProviderFailure, ProviderFailureReason, ProviderRoute}; diff --git a/src/protocols/acp.rs b/src/protocols/acp.rs index 3127bb97..157737c6 100644 --- a/src/protocols/acp.rs +++ b/src/protocols/acp.rs @@ -1629,7 +1629,7 @@ impl Server { let (tx, rx) = mpsc::channel(8); let voice_state = crate::runtime::voice_state::VoiceState::default(); let actor = SessionActor { - voice_monitor: voice_state.monitor(claim.is_resumed() || claim.is_fork()), + voice_monitor: voice_state.monitor(&canonical_transcript), session_id: session_id.clone(), runtime: Arc::clone(&self.runtime), integration: Arc::clone(&self.integration), @@ -2083,8 +2083,7 @@ async fn session_actor(actor: SessionActor) { reply, }) => { let result = (|| { - let mut transcript = driver.snapshot().transcript; - crate::transcript::sanitize_forked_transcript(&mut transcript); + let transcript = driver.snapshot().transcript; Ok(AcpForkState { transcript, selection: adapter.selection().map_err(AcpRuntimeError::Loop)?, @@ -5731,7 +5730,7 @@ pub(super) mod tests { let tasks = task_manager.handle(); let background_jobs = BackgroundJobs::default(); let mut skill_catalog = skill_catalog::SkillCatalogMonitor::new(&[]).unwrap(); - let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(false); + let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(&[]); let root = tempfile::tempdir().unwrap(); let runtime = Runtime::new(root.path(), "gpt-5.4").unwrap(); let response = drive_runtime_prompt( @@ -5855,7 +5854,7 @@ pub(super) mod tests { .unwrap(); let task_manager = AsyncTaskManager::new(); let tasks = task_manager.handle(); - let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(false); + let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(&[]); let response = drive_runtime_prompt( &acp_session_id, &runtime, @@ -5944,7 +5943,7 @@ pub(super) mod tests { .unwrap(); let background_jobs = BackgroundJobs::default(); let mut skill_catalog = skill_catalog::SkillCatalogMonitor::new(&[]).unwrap(); - let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(false); + let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(&[]); let request = PromptRequest::new( acp_session_id.clone(), vec![agentkit_acp::ContentBlock::Text( @@ -6238,7 +6237,7 @@ pub(super) mod tests { let runtime = Runtime::new(root.path(), "gpt-5.4").unwrap(); let skills = runtime.current_skills().await.unwrap(); let actor = tokio::spawn(session_actor(SessionActor { - voice_monitor: crate::runtime::voice_state::VoiceState::default().monitor(false), + voice_monitor: crate::runtime::voice_state::VoiceState::default().monitor(&[]), session_id: acp_session_id.clone(), runtime, integration: Arc::clone(&integration), diff --git a/src/protocols/acp/v2.rs b/src/protocols/acp/v2.rs index 091425a4..1aeed8c2 100644 --- a/src/protocols/acp/v2.rs +++ b/src/protocols/acp/v2.rs @@ -1000,7 +1000,7 @@ impl Server { let busy = Arc::new(AtomicBool::new(false)); let voice_state = crate::runtime::voice_state::VoiceState::default(); let actor = SessionActor { - voice_monitor: voice_state.monitor(claim.is_resumed() || claim.is_fork()), + voice_monitor: voice_state.monitor(&canonical_transcript), session_id: session_id.clone(), runtime: Arc::clone(&self.runtime), integration: Arc::clone(&self.integration), @@ -3561,7 +3561,7 @@ mod tests { let tasks = task_manager.handle(); let background_jobs = BackgroundJobs::default(); let mut skill_catalog = skill_catalog::SkillCatalogMonitor::new(&[]).unwrap(); - let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(false); + let mut voice_monitor = crate::runtime::voice_state::VoiceState::default().monitor(&[]); let (result, ()) = tokio::join!( prepare_prompt( &session_id, @@ -4591,7 +4591,7 @@ mod tests { )) .unwrap(); let state = crate::runtime::voice_state::VoiceState::default(); - let mut monitor = state.monitor(false); + let mut monitor = state.monitor(&[]); let turns = Arc::new(AtomicU64::new(0)); let mut driver = Agent::builder() .model(TestAdapter { diff --git a/src/provider/adapter.rs b/src/provider/adapter.rs index eff147b1..df7b761d 100644 --- a/src/provider/adapter.rs +++ b/src/provider/adapter.rs @@ -359,6 +359,7 @@ pub struct SelectableSession { config: SessionConfig, active: SessionSelection, inner: KitSession, + retry_observer: Option>, } #[async_trait] @@ -390,6 +391,7 @@ impl ModelAdapter for SelectableAdapter { config, active, inner, + retry_observer: None, }) } @@ -425,6 +427,11 @@ fn expose_background_call_ids(request: &mut TurnRequest) { impl ModelSession for SelectableSession { type Turn = KitTurn; + fn set_retry_observer(&mut self, observer: Option>) { + self.inner.set_retry_observer(observer.clone()); + self.retry_observer = observer; + } + async fn begin_turn( &mut self, mut request: TurnRequest, @@ -468,7 +475,8 @@ impl SelectableSession { selected: SessionSelection, replacement: Result, ) -> Result<(), LoopError> { - let replacement = replacement?; + let mut replacement = replacement?; + replacement.set_retry_observer(self.retry_observer.clone()); self.inner = replacement; self.active = selected; Ok(()) @@ -1103,6 +1111,14 @@ fn tool_image_traversal_error() -> LoopError { impl ModelSession for KitSession { type Turn = KitTurn; + fn set_retry_observer(&mut self, observer: Option>) { + match self { + Self::OpenAiSubscription(session) => session.set_retry_observer(observer), + Self::OpenRouter(session) => session.inner.set_retry_observer(observer), + Self::Speakeasy(session) => session.inner.set_retry_observer(observer), + } + } + async fn begin_turn( &mut self, request: TurnRequest, @@ -2063,6 +2079,7 @@ mod tests { config: SessionConfig::new("provider-identity-test"), active, inner, + retry_observer: None, } } diff --git a/src/provider/chatgpt.rs b/src/provider/chatgpt.rs index 0ddc297e..5da46fe2 100644 --- a/src/provider/chatgpt.rs +++ b/src/provider/chatgpt.rs @@ -2,7 +2,7 @@ use std::{ collections::{HashMap, HashSet}, future::Future, io::Cursor, - sync::Arc, + sync::{Arc, OnceLock}, time::Duration, }; @@ -55,6 +55,9 @@ const MAX_NORMALIZED_IMAGE_BYTES: usize = ((MAX_FIELD_BYTES - JPEG_DATA_URL_PREF const MAX_SERVER_DELAY: Duration = Duration::from_secs(10 * 60); const MAX_SUBSCRIPTION_AUTH_TIMEOUT: Duration = Duration::from_secs(30); const MODEL_CATALOG_AUTH_TIMEOUT: Duration = Duration::from_secs(5); +const MODEL_CATALOG_BACKGROUND_TIMEOUT: Duration = Duration::from_secs(30); +const MODEL_CATALOG_RETRY_DELAY: Duration = Duration::from_secs(1); +const MODEL_CATALOG_MAX_RETRY_DELAY: Duration = Duration::from_secs(60); const LEGACY_CONTINUATION_METADATA: &str = "openai.subscription.v1"; const CONTINUATION_METADATA: &str = "openai.responses.continuation.v1"; @@ -163,7 +166,7 @@ impl SubscriptionConfig { pub struct OpenAiSubscriptionAdapter { config: SubscriptionConfig, reasoning_effort: Option, - catalog_client: reqwest::Client, + catalog_client: ModelCatalogClient, responses_client: agentkit_http::Http, model_catalog: SubscriptionModelCatalogCache, } @@ -196,7 +199,10 @@ impl OpenAiSubscriptionAdapter { .user_agent(concat!("kit/", env!("CARGO_PKG_VERSION"))) .build() .map_err(|_| "could not build OpenAI subscription client".to_owned())?; - let catalog_client = client.clone(); + let catalog_client = ModelCatalogClient { + client: client.clone(), + endpoint: MODELS_ENDPOINT.to_owned(), + }; let responses_client = agentkit_http::Http::new(ChatGptRetryHintsClient(client)); Ok(Self { config, @@ -211,11 +217,10 @@ impl OpenAiSubscriptionAdapter { &self, credentials: &auth::TokenRecord, binding: &auth::CredentialBinding, + timeout: Duration, ) -> Result, LoopError> { self.model_catalog - .get_or_try_init(binding, || { - fetch_model_catalog(&self.catalog_client, credentials) - }) + .get_or_try_init(binding, || self.catalog_client.fetch(credentials, timeout)) .await } @@ -234,7 +239,8 @@ impl OpenAiSubscriptionAdapter { let binding = credentials .binding() .map_err(|error| LoopError::Provider(error.to_string()))?; - self.catalog_with_credentials(&credentials, &binding).await + self.catalog_with_credentials(&credentials, &binding, MODEL_CATALOG_AUTH_TIMEOUT) + .await } } @@ -257,14 +263,9 @@ impl ModelAdapter for OpenAiSubscriptionAdapter { .binding() .map_err(|error| LoopError::Provider(error.to_string()))?; let authentication_binding = binding_string(&binding); - // Catalog discovery stays independent and best-effort. A failed fetch is not cached. - let model_catalog = self - .catalog_with_credentials(&credentials, &binding) - .await - .unwrap_or_else(|_| Arc::new(SubscriptionModelCatalog::default())); let authentication = Authentication::new(OpenAiAuthenticationProvider { credential_storage: self.config.credential_storage.clone(), - binding, + binding: binding.clone(), timeout: auth_timeout(&resilience), }); let config = subscription_responses_config( @@ -278,10 +279,7 @@ impl ModelAdapter for OpenAiSubscriptionAdapter { .await?; Ok(OpenAiSubscriptionSession { inner, - context_window: model_catalog - .context_windows - .get(&self.config.model) - .copied(), + context_window: ContextWindowDiscovery::start(self.clone(), credentials, binding), authentication_binding, }) } @@ -311,22 +309,101 @@ fn subscription_responses_config( max_items: MAX_ITEMS, max_text_bytes: MAX_FIELD_BYTES, }) - .with_resilience(resilience); + .with_resilience(resilience) + .with_progress_timeouts( + Some(Duration::from_secs(60)), + Some(Duration::from_secs(180)), + ); if let Some(effort) = reasoning_effort { config = config.with_reasoning_effort(effort.as_str()); } config } +// One worker publishes capacity once; readers never wait for discovery. The +// worker owns only the value, not this owner, so dropping the last session/turn +// aborts pending HTTP, credential loading, or retry sleep without a cycle. +struct ContextWindowDiscovery { + value: Arc>, + worker: tokio::task::JoinHandle<()>, +} + +impl ContextWindowDiscovery { + fn start( + adapter: OpenAiSubscriptionAdapter, + mut credentials: auth::TokenRecord, + binding: auth::CredentialBinding, + ) -> Arc { + let value = Arc::new(OnceLock::new()); + let published = Arc::clone(&value); + let worker = tokio::spawn(async move { + let mut delay = MODEL_CATALOG_RETRY_DELAY; + loop { + match adapter + .catalog_with_credentials( + &credentials, + &binding, + MODEL_CATALOG_BACKGROUND_TIMEOUT, + ) + .await + { + Ok(catalog) => { + if let Some(&window) = catalog.context_windows.get(&adapter.config.model) { + let _ = published.set(window); + } + // A valid catalog may not advertise this model's capacity. + return; + } + Err(error) => { + tracing::debug!(%error, "context capacity discovery failed; retrying"); + } + } + loop { + tokio::time::sleep(delay).await; + delay = (delay * 2).min(MODEL_CATALOG_MAX_RETRY_DELAY); + match load_credentials( + adapter.config.credential_storage.clone(), + MODEL_CATALOG_AUTH_TIMEOUT, + ) + .await + { + Ok(fresh) => { + if ensure_credential_binding(&binding, &fresh).is_err() { + return; + } + credentials = fresh; + break; + } + Err(error) => { + tracing::debug!(%error, "context capacity credentials unavailable; retrying"); + } + } + } + } + }); + Arc::new(Self { value, worker }) + } +} + +impl Drop for ContextWindowDiscovery { + fn drop(&mut self) { + self.worker.abort(); + } +} + pub struct OpenAiSubscriptionSession { inner: OpenAIResponsesSession, - context_window: Option, + context_window: Arc, authentication_binding: String, } #[async_trait] impl ModelSession for OpenAiSubscriptionSession { type Turn = OpenAiSubscriptionTurn; + + fn set_retry_observer(&mut self, observer: Option>) { + self.inner.set_retry_observer(observer); + } async fn begin_turn( &mut self, mut request: TurnRequest, @@ -351,7 +428,7 @@ impl ModelSession for OpenAiSubscriptionSession { .await .map(|inner| OpenAiSubscriptionTurn { inner, - context_window: self.context_window, + context_window: Arc::clone(&self.context_window), }) } fn model_name(&self) -> Option<&str> { @@ -569,7 +646,7 @@ fn encode_image_to_budget( pub struct OpenAiSubscriptionTurn { inner: UpstreamOpenAIResponsesTurn, - context_window: Option, + context_window: Arc, } #[async_trait] @@ -583,7 +660,7 @@ impl ModelTurn for OpenAiSubscriptionTurn { cancellation: Option, ) -> Result, LoopError> { let mut event = self.inner.next_event(cancellation).await?; - if let Some(context_window) = self.context_window { + if let Some(&context_window) = self.context_window.value.get() { stamp_context_window( &mut event, context_window, @@ -947,48 +1024,62 @@ fn binding_string(binding: &auth::CredentialBinding) -> String { format!("openai-chatgpt-v1:{account_digest}:{}", binding.generation) } -async fn fetch_model_catalog( - client: &reqwest::Client, - credentials: &auth::TokenRecord, -) -> Result { - let endpoint = format!("{MODELS_ENDPOINT}?client_version={MODEL_CATALOG_CLIENT_VERSION}"); - let mut request = client - .get(endpoint) - .bearer_auth(credentials.access_token()) - .header("originator", "kit") - .header("Accept", "application/json") - .timeout(Duration::from_secs(5)); - if let Some(account_id) = credentials.account_id() { - request = request.header("ChatGPT-Account-ID", account_id); - } - let response = request - .send() - .await - .map_err(|_| LoopError::Provider("model catalog transport failed".into()))?; - if !response.status().is_success() { - return Err(LoopError::Provider(format!( - "model catalog returned {}", - response.status() - ))); - } - if response - .content_length() - .is_some_and(|length| length > MAX_MODELS_BYTES as u64) - { - return Err(protocol("model catalog exceeds 2 MiB")); - } - let mut body = Vec::new(); - let mut stream = response.bytes_stream(); - while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(|_| LoopError::Provider("model catalog body failed".into()))?; - if body.len().saturating_add(chunk.len()) > MAX_MODELS_BYTES { +#[derive(Clone)] +struct ModelCatalogClient { + client: reqwest::Client, + endpoint: String, +} + +impl ModelCatalogClient { + async fn fetch( + &self, + credentials: &auth::TokenRecord, + timeout: Duration, + ) -> Result { + let endpoint = format!( + "{}?client_version={MODEL_CATALOG_CLIENT_VERSION}", + self.endpoint + ); + let mut request = self + .client + .get(endpoint) + .bearer_auth(credentials.access_token()) + .header("originator", "kit") + .header("Accept", "application/json") + .timeout(timeout); + if let Some(account_id) = credentials.account_id() { + request = request.header("ChatGPT-Account-ID", account_id); + } + let response = request + .send() + .await + .map_err(|_| LoopError::Provider("model catalog transport failed".into()))?; + if !response.status().is_success() { + return Err(LoopError::Provider(format!( + "model catalog returned {}", + response.status() + ))); + } + if response + .content_length() + .is_some_and(|length| length > MAX_MODELS_BYTES as u64) + { return Err(protocol("model catalog exceeds 2 MiB")); } - body.extend_from_slice(&chunk); + let mut body = Vec::new(); + let mut stream = response.bytes_stream(); + while let Some(chunk) = stream.next().await { + let chunk = + chunk.map_err(|_| LoopError::Provider("model catalog body failed".into()))?; + if body.len().saturating_add(chunk.len()) > MAX_MODELS_BYTES { + return Err(protocol("model catalog exceeds 2 MiB")); + } + body.extend_from_slice(&chunk); + } + let value: Value = serde_json::from_slice(&body) + .map_err(|_| protocol("model catalog is not valid JSON"))?; + parse_model_catalog(&value) } - let value: Value = - serde_json::from_slice(&body).map_err(|_| protocol("model catalog is not valid JSON"))?; - parse_model_catalog(&value) } fn parse_model_catalog(value: &Value) -> Result { @@ -1052,6 +1143,34 @@ mod tests { use super::*; use serde_json::json; + pub(super) fn context_window(window: Option) -> Arc { + let value = Arc::new(OnceLock::new()); + if let Some(window) = window { + value.set(window).unwrap(); + } + Arc::new(ContextWindowDiscovery { + value, + worker: tokio::spawn(async {}), + }) + } + + #[test] + fn credential_binding_accepts_reauthentication_but_rejects_account_changes() { + let original = auth::test_support::token_record("old", "account-one", "generation-one"); + let expected = original.binding().unwrap(); + let fresh = auth::test_support::token_record("new", "account-one", "generation-two"); + assert!(ensure_credential_binding(&expected, &fresh).is_ok()); + assert_ne!( + binding_string(&expected), + binding_string(&fresh.binding().unwrap()) + ); + + for generation in ["generation-one", "generation-two"] { + let other = auth::test_support::token_record("other", "account-two", generation); + assert!(ensure_credential_binding(&expected, &other).is_err()); + } + } + fn image_tool_request() -> TurnRequest { use agentkit_core::{Item, SessionId, ToolResultPart, TurnId}; TurnRequest { @@ -1360,7 +1479,7 @@ mod tests { .unwrap(); let mut session = OpenAiSubscriptionSession { inner, - context_window: None, + context_window: tests::context_window(None), authentication_binding: "unused".into(), }; let mut request = image_tool_request(); @@ -1583,83 +1702,6 @@ mod tests { assert!(!format!("{attempt:?}").contains("secret-token")); } - #[test] - fn credential_binding_accepts_reauthentication_but_rejects_account_changes() { - let original = auth::test_support::token_record("old", "account-one", "generation-one"); - let expected = original.binding().unwrap(); - let fresh = auth::test_support::token_record("new", "account-one", "generation-two"); - assert!(ensure_credential_binding(&expected, &fresh).is_ok()); - assert_ne!( - binding_string(&expected), - binding_string(&fresh.binding().unwrap()) - ); - - for generation in ["generation-one", "generation-two"] { - let other = auth::test_support::token_record("other", "account-two", generation); - assert!(ensure_credential_binding(&expected, &other).is_err()); - } - } - - #[tokio::test] - async fn model_catalog_cache_is_scoped_to_account_and_generation() { - let cache = SubscriptionModelCatalogCache::default(); - let first_binding = auth::test_support::token_record("token", "account-1", "generation-1") - .binding() - .unwrap(); - let next_generation = - auth::test_support::token_record("token", "account-1", "generation-2") - .binding() - .unwrap(); - let next_account = auth::test_support::token_record("token", "account-2", "generation-1") - .binding() - .unwrap(); - - let first = cache - .get_or_try_init(&first_binding, || async { - Ok(SubscriptionModelCatalog { - context_windows: HashMap::from([("first".into(), 100)]), - visible_models: vec!["first".into()], - }) - }) - .await - .unwrap(); - let same = cache - .get_or_try_init(&first_binding, || async { - Err(protocol("cached catalog was unexpectedly reloaded")) - }) - .await - .unwrap(); - assert!(Arc::ptr_eq(&first, &same)); - - let second = cache - .get_or_try_init(&next_generation, || async { - Ok(SubscriptionModelCatalog { - context_windows: HashMap::from([("second".into(), 200)]), - visible_models: vec!["second".into()], - }) - }) - .await - .unwrap(); - assert_eq!(second.visible_models, ["second"]); - assert_eq!(second.context_windows.get("first"), None); - assert_eq!(second.context_windows.get("second"), Some(&200)); - assert!(!Arc::ptr_eq(&first, &second)); - - let third = cache - .get_or_try_init(&next_account, || async { - Ok(SubscriptionModelCatalog { - context_windows: HashMap::from([("third".into(), 300)]), - visible_models: vec!["third".into()], - }) - }) - .await - .unwrap(); - assert_eq!(third.visible_models, ["third"]); - assert_eq!(third.context_windows.get("second"), None); - assert_eq!(third.context_windows.get("third"), Some(&300)); - assert!(!Arc::ptr_eq(&second, &third)); - } - #[tokio::test] async fn model_catalog_cache_retries_after_failure() { let cache = SubscriptionModelCatalogCache::default(); @@ -1825,3 +1867,6 @@ mod image_tests; #[cfg(all(test, feature = "tui"))] mod websocket_tests; + +#[cfg(test)] +mod catalog_tests; diff --git a/src/provider/chatgpt/catalog_tests.rs b/src/provider/chatgpt/catalog_tests.rs new file mode 100644 index 00000000..094d7d4c --- /dev/null +++ b/src/provider/chatgpt/catalog_tests.rs @@ -0,0 +1,159 @@ +//! Catalog discovery through the real credential store and HTTP boundary. +#![allow( + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::disallowed_methods, + clippy::disallowed_macros +)] +use super::*; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use tokio::net::{TcpListener, TcpStream}; + +const WAIT: Duration = Duration::from_secs(5); +const CATALOG: &str = + r#"{"models":[{"slug":"gpt-5.4","context_window":272000,"visibility":"list"}]}"#; + +pub(super) async fn adapter() -> (OpenAiSubscriptionAdapter, TcpListener, tempfile::TempDir) { + let directory = tempfile::tempdir().unwrap(); + let storage = crate::credentials::CredentialStorage::Filesystem(directory.path().into()); + let record = auth::test_support::token_record("loopback-only", "account", "generation"); + storage + .entry("openai-subscription", "subscription") + .save(&serde_json::to_vec(&record).unwrap()) + .unwrap(); + let mut adapter = OpenAiSubscriptionAdapter::new( + SubscriptionConfig::new("gpt-5.4".into()) + .unwrap() + .with_credential_storage(storage), + ) + .unwrap(); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + adapter.catalog_client.endpoint = format!("http://{}/models", listener.local_addr().unwrap()); + (adapter, listener, directory) +} + +pub(super) async fn request(listener: &TcpListener) -> TcpStream { + tokio::time::timeout(WAIT, async { + let (mut stream, _) = listener.accept().await.unwrap(); + let mut header = Vec::new(); + while !header.ends_with(b"\r\n\r\n") { + header.push(stream.read_u8().await.unwrap()); + assert!(header.len() < 8192); + } + let header = String::from_utf8(header).unwrap(); + assert!( + header.starts_with("GET /models?client_version="), + "{header}" + ); + stream + }) + .await + .expect("catalog request did not arrive") +} + +pub(super) async fn respond(mut stream: TcpStream, status: &str, body: &str) { + stream + .write_all( + format!( + "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ) + .as_bytes(), + ) + .await + .unwrap(); +} + +pub(super) async fn discovered(session: &OpenAiSubscriptionSession) { + tokio::time::timeout(WAIT, async { + while !session.context_window.worker.is_finished() { + tokio::task::yield_now().await; + } + }) + .await + .expect("discovery did not finish"); + assert_eq!(session.context_window.value.get(), Some(&272_000)); +} + +#[tokio::test] +async fn context_discovery_does_not_block_start_and_retries_failed_http() { + let (adapter, listener, _directory) = adapter().await; + // The peer cannot reply until startup returns: the timeout is only a + // deadlock watchdog, not a wall-clock performance assertion. + let session = tokio::time::timeout(WAIT, adapter.start_session(SessionConfig::new("session"))) + .await + .expect("session start waited for catalog HTTP") + .unwrap(); + let first = request(&listener).await; + assert!(session.context_window.value.get().is_none()); + respond(first, "503 Service Unavailable", "").await; + let retry = request(&listener).await; + assert!(session.context_window.value.get().is_none()); + respond(retry, "200 OK", CATALOG).await; + discovered(&session).await; + // The picker and background discovery share the credential-bound result. + let catalog = adapter.model_catalog().await.unwrap(); + assert_eq!(catalog.context_windows.get("gpt-5.4"), Some(&272_000)); +} + +#[tokio::test] +async fn context_discovery_cancellation_releases_cache_initialization() { + let (adapter, listener, _directory) = adapter().await; + let session = adapter + .start_session(SessionConfig::new("first")) + .await + .unwrap(); + let mut pending = request(&listener).await; + let worker = session.context_window.worker.abort_handle(); + drop(session); + tokio::time::timeout(WAIT, async { + while !worker.is_finished() { + tokio::task::yield_now().await; + } + // The real pending HTTP request is cancelled, not detached. + let mut byte = [0]; + assert_eq!(pending.read(&mut byte).await.unwrap(), 0); + }) + .await + .unwrap(); + let next = adapter + .start_session(SessionConfig::new("next")) + .await + .unwrap(); + let retry = request(&listener).await; + respond(retry, "200 OK", CATALOG).await; + discovered(&next).await; +} + +#[tokio::test] +async fn context_discovery_stops_when_credentials_change() { + let (adapter, listener, _directory) = adapter().await; + let session = adapter + .start_session(SessionConfig::new("session")) + .await + .unwrap(); + let first = request(&listener).await; + let record = auth::test_support::token_record("new-token", "new-account", "new-generation"); + adapter + .config + .credential_storage + .entry("openai-subscription", "subscription") + .save(&serde_json::to_vec(&record).unwrap()) + .unwrap(); + respond(first, "503 Service Unavailable", "").await; + tokio::time::timeout(WAIT, async { + while !session.context_window.worker.is_finished() { + tokio::task::yield_now().await; + } + }) + .await + .unwrap(); + assert!(session.context_window.value.get().is_none()); + let next = adapter + .start_session(SessionConfig::new("next")) + .await + .unwrap(); + respond(request(&listener).await, "200 OK", CATALOG).await; + discovered(&next).await; +} diff --git a/src/provider/chatgpt/websocket_tests.rs b/src/provider/chatgpt/websocket_tests.rs index 0a600b75..d568d500 100644 --- a/src/provider/chatgpt/websocket_tests.rs +++ b/src/provider/chatgpt/websocket_tests.rs @@ -54,6 +54,10 @@ fn accept(listener: &TcpListener) -> TcpStream { } async fn session(endpoint: &str) -> OpenAiSubscriptionSession { + session_with_retries(endpoint, 0).await +} + +async fn session_with_retries(endpoint: &str, max_retries: usize) -> OpenAiSubscriptionSession { // Exercise Kit's real transport selection and request policy; override only // the destination, dummy authentication, and bounded test timeouts. let authentication = Authentication::bearer("loopback-only"); @@ -68,7 +72,7 @@ async fn session(endpoint: &str) -> OpenAiSubscriptionSession { "gpt-5.4".into(), authentication, ResilienceConfig { - max_retries: 0, + max_retries, retry_budget: WAIT, attempt_timeout: Some(WAIT), stream_idle_timeout: Some(WAIT), @@ -84,7 +88,7 @@ async fn session(endpoint: &str) -> OpenAiSubscriptionSession { .start_session(SessionConfig::new("session")) .await .unwrap(), - context_window: Some(200_000), + context_window: super::tests::context_window(Some(200_000)), authentication_binding, } } @@ -476,6 +480,28 @@ fn http(listener: &TcpListener, status: &str, body: &str) -> (String, Value) { ) } +#[tokio::test] +async fn subscription_retries_reach_the_session_retry_observer() { + let (endpoint, peer) = server(|listener| { + let mut lost = tungstenite::accept(accept(&listener)).unwrap(); + receive(&mut lost, false); + drop(lost); + let mut fresh = tungstenite::accept(accept(&listener)).unwrap(); + receive(&mut fresh, false); + success(&mut fresh, "retried", "first answer"); + }); + let mut session = session_with_retries(&endpoint, 1).await; + let retries = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let observed = Arc::clone(&retries); + session.set_retry_observer(Some(Arc::new(move |_| { + observed.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + }))); + let mut turn = begin(&mut session, false).await; + finished(&mut turn, "retried", "first answer").await; + peer.join().unwrap(); + assert!(retries.load(std::sync::atomic::Ordering::SeqCst) > 0); +} + #[tokio::test] async fn subscription_auto_426_fallback_stays_http_for_future_turns() { let (endpoint, peer) = server(|listener| { @@ -508,3 +534,63 @@ async fn subscription_auto_426_fallback_stays_http_for_future_turns() { } peer.join().unwrap(); } + +#[tokio::test] +async fn subscription_websocket_reports_capacity_discovered_after_first_turn() { + use super::catalog_tests; + let (endpoint, peer) = server(|listener| { + let mut socket = tungstenite::accept(accept(&listener)).unwrap(); + receive(&mut socket, false); + success(&mut socket, "first-response", "first answer"); + let wire = receive_wire(&mut socket); + assert_eq!(wire["previous_response_id"], "first-response"); + let input = wire["input"].as_array().unwrap(); + assert_eq!(input.len(), 1); + assert_eq!(input[0]["content"][0]["text"], "second question"); + success(&mut socket, "second-response", "second answer"); + }); + let (adapter, catalog_listener, _directory) = catalog_tests::adapter().await; + let mut deferred = adapter + .start_session(SessionConfig::new("session")) + .await + .unwrap(); + // Use the existing loopback Responses configuration; retain the real + // session-started discovery and its credential-bound cache. + deferred.inner = session(&endpoint).await.inner; + let catalog_request = catalog_tests::request(&catalog_listener).await; + let mut first = deferred.begin_turn(request(false), None).await.unwrap(); + let mut first_usage = false; + while let Some(event) = first.next_event(None).await.unwrap() { + if let ModelTurnEvent::Usage(usage) = event { + first_usage = true; + assert!(!usage.metadata.contains_key("context_window")); + } + } + assert!(first_usage); + // A completed/dropped turn must not cancel in-flight session discovery. + drop(first); + catalog_tests::respond(catalog_request, "503 Service Unavailable", "").await; + let retry = catalog_tests::request(&catalog_listener).await; + catalog_tests::respond( + retry, + "200 OK", + r#"{"models":[{"slug":"gpt-5.4","context_window":272000}]}"#, + ) + .await; + catalog_tests::discovered(&deferred).await; + let mut second = deferred.begin_turn(request(true), None).await.unwrap(); + let mut second_usage = false; + while let Some(event) = second.next_event(None).await.unwrap() { + if let ModelTurnEvent::Usage(usage) = event { + second_usage = true; + assert_eq!(usage.metadata["context_window"], 272_000); + assert_eq!( + usage.metadata["openai.subscription.context_window"], + 272_000 + ); + assert_eq!(usage.tokens.unwrap().input_tokens, 3); + } + } + assert!(second_usage); + peer.join().unwrap(); +} diff --git a/src/provider/chatgpt_image_tests.rs b/src/provider/chatgpt_image_tests.rs index f86fbaf2..30e8a0a5 100644 --- a/src/provider/chatgpt_image_tests.rs +++ b/src/provider/chatgpt_image_tests.rs @@ -96,7 +96,7 @@ async fn authenticated_continuation_replays_parallel_results_with_fallback_image .start_session(SessionConfig::new("image-replay")) .await .unwrap(), - context_window: None, + context_window: super::tests::context_window(None), // No legacy metadata is injected: the real adapter creates and validates // its current authentication-bound continuation metadata. authentication_binding: "unused-legacy-binding".into(), diff --git a/src/runtime.rs b/src/runtime.rs index 8b468917..5a5696f2 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -15,7 +15,8 @@ use agentkit_core::{ ToolOutput, ToolResultPart, }; use agentkit_loop::{ - Agent, LoopDriver, LoopError, LoopInterrupt, LoopObserver, LoopStep, SessionConfig, + Agent, LoopDriver, LoopError, LoopInterrupt, LoopObserver, LoopStep, PromptCacheRequest, + SessionConfig, }; use agentkit_task_manager::{AsyncTaskManager, RoutingDecision, TaskManager, TaskManagerHandle}; use agentkit_tool_compose::{ @@ -40,8 +41,8 @@ use crate::{ }, tools::{ A2aTool, ArtifactTool, AuthTool, CloseTool, DocsTool, EditTool, ForkTool, McpTool, - Observed, PromptTool, ReadFileTool, ShellTool, SteerTool, SubagentTool, Subagents, - SubagentsTool, ToolSchema, ToolSearch, observe_shared, + Observed, PromptTool, ReadFileTool, ShellTool, SubagentTool, Subagents, SubagentsTool, + ToolSchema, ToolSearch, observe_shared, }, }; @@ -370,10 +371,6 @@ impl SessionClaim { self.uncommitted_observer = Some(observer.clone()); } - pub(crate) fn is_resumed(&self) -> bool { - self.request.resume - } - pub(crate) fn is_fork(&self) -> bool { matches!(self.kind, SessionClaimKind::Fork) } @@ -486,9 +483,6 @@ struct McpInstallSources { configured_inherited: bool, } -pub(crate) const SUBAGENT_SYSTEM_PROMPT_MARKER: &str = - "This task was delegated to you by the primary agent."; - #[derive(Debug)] pub(crate) enum LogoutAuthenticationError { CredentialStateUnchanged(String), @@ -515,7 +509,6 @@ pub struct Runtime { openrouter_api_key: Option, ambient_openrouter_api_key: bool, telemetry: agentkit_loop::TelemetryConfig, - max_subagent_depth: usize, base_depth: usize, subagents: Subagents, /// The explicitly selected session is consumed by the first ACP session. @@ -601,28 +594,26 @@ impl Runtime { reasoning_effort, openrouter_api_key.clone(), )?; - let max_subagent_depth = 2; - let subagents = Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: root.clone(), - model: model.clone(), - provider, - reasoning_effort, - openrouter_api_key: openrouter_api_key.clone(), - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: true, - mcp_config: None, - credential_storage: credential_storage.clone(), - telemetry: Default::default(), - harnesses: AcpHarnesses::default(), - default_harness: BUILTIN_HARNESS.into(), - parent_id: None, - parent_name: None, - }, - max_subagent_depth, - ); + let subagents = Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: root.clone(), + model: model.clone(), + provider, + reasoning_effort, + openrouter_api_key: openrouter_api_key.clone(), + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: true, + mcp_config: None, + credential_storage: credential_storage.clone(), + telemetry: Default::default(), + harnesses: AcpHarnesses::default(), + default_harness: BUILTIN_HARNESS.into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }); Ok(Arc::new(Self { eval: None, root, @@ -635,7 +626,6 @@ impl Runtime { ambient_openrouter_api_key: std::env::var_os("OPENROUTER_API_KEY") .is_some_and(|value| !value.is_empty()), telemetry: Default::default(), - max_subagent_depth, base_depth: 0, subagents, session: Mutex::new(SessionSelection::default()), @@ -740,12 +730,6 @@ impl Runtime { /// Sets the inherited nesting depth for an ACP subprocess. pub fn with_depth(runtime: Arc, depth: usize) -> Result, String> { - if depth > runtime.max_subagent_depth { - return Err(format!( - "subagent depth {depth} exceeds limit {}", - runtime.max_subagent_depth - )); - } let mut runtime = Arc::try_unwrap(runtime) .map_err(|_| "could not configure runtime depth after it was shared".to_string())?; runtime.base_depth = depth; @@ -823,7 +807,7 @@ impl Runtime { Some((id, name)) => (Some(id), Some(name)), None => (None, None), }; - runtime.subagents = Subagents::new(config, runtime.max_subagent_depth); + runtime.subagents = Subagents::new(config); Ok(Arc::new(runtime)) } @@ -840,14 +824,11 @@ impl Runtime { "could not configure ACP harnesses after runtime was shared".to_string() })?; let previous = runtime.subagents.child_config(); - runtime.subagents = Subagents::new( - ChildConfig { - harnesses, - default_harness, - ..previous - }, - runtime.max_subagent_depth, - ); + runtime.subagents = Subagents::new(ChildConfig { + harnesses, + default_harness, + ..previous + }); Ok(Arc::new(runtime)) } @@ -861,13 +842,10 @@ impl Runtime { .map_err(|_| "could not configure telemetry after runtime was shared".to_string())?; runtime.telemetry = config; let previous = runtime.subagents.child_config(); - runtime.subagents = Subagents::new( - ChildConfig { - telemetry, - ..previous - }, - runtime.max_subagent_depth, - ); + runtime.subagents = Subagents::new(ChildConfig { + telemetry, + ..previous + }); Ok(Arc::new(runtime)) } @@ -1049,27 +1027,26 @@ impl Runtime { runtime.mcp = mcp; runtime.credential_storage = credential_storage.clone(); let previous = runtime.subagents.child_config(); - runtime.subagents = Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: runtime.root.clone(), - model: runtime.model.clone(), - provider: runtime.provider, - reasoning_effort: runtime.reasoning_effort, - openrouter_api_key: runtime.openrouter_api_key.clone(), - configured_mcp_config: install.configured_path, - configured_mcp_config_inherited: install.configured_inherited, - legacy_mcp_config: install.legacy, - mcp_config: install.explicit_path, - credential_storage, - telemetry: previous.telemetry, - harnesses: previous.harnesses, - default_harness: previous.default_harness, - parent_id: previous.parent_id, - parent_name: previous.parent_name, - }, - runtime.max_subagent_depth, - ); + runtime.subagents = Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: runtime.root.clone(), + model: runtime.model.clone(), + provider: runtime.provider, + reasoning_effort: runtime.reasoning_effort, + openrouter_api_key: runtime.openrouter_api_key.clone(), + configured_mcp_config: install.configured_path, + configured_mcp_config_inherited: install.configured_inherited, + legacy_mcp_config: install.legacy, + mcp_config: install.explicit_path, + credential_storage, + telemetry: previous.telemetry, + harnesses: previous.harnesses, + default_harness: previous.default_harness, + parent_id: previous.parent_id, + parent_name: previous.parent_name, + tree_slots: None, + prompt_cache_key: None, + }); Ok(Arc::new(runtime)) } @@ -1127,10 +1104,6 @@ impl Runtime { .map_err(AcpRuntimeError::Loop) } - pub const fn max_subagent_depth(&self) -> usize { - self.max_subagent_depth - } - /// Returns the depth inherited by this runtime process. pub const fn base_depth(&self) -> usize { self.base_depth @@ -1239,14 +1212,10 @@ impl Runtime { if let Some(eval) = &self.eval { children.register(Observed::new(eval.clone())); } - if depth < self.max_subagent_depth { - children - .register(Observed::new(SubagentTool::new(subagents.clone(), depth))) - .register(Observed::new(ForkTool::new(subagents.clone(), depth))); - } children + .register(Observed::new(SubagentTool::new(subagents.clone(), depth))) + .register(Observed::new(ForkTool::new(subagents.clone(), depth))) .register(Observed::new(PromptTool::new(subagents.clone()))) - .register(Observed::new(SteerTool::new(subagents.clone()))) .register(Observed::new(SubagentsTool::new(subagents.clone()))) .register(Observed::new(CloseTool::new(subagents, { let background_jobs = background_jobs.clone(); @@ -1350,7 +1319,7 @@ impl Runtime { })?; } let initial = if request.resume { - vec![Item::text(ItemKind::System, self.system_prompt(0))] + vec![Item::text(ItemKind::System, self.system_prompt())] } else { self.initial_transcript(0).await.map_err(|error| { record_runtime_failure( @@ -1404,6 +1373,7 @@ impl Runtime { let subagents = self .subagents .fresh() + .with_prompt_cache_key(crate::tools::subagent::prompt_cache_key(&session_id)) .with_observer(opened.observer.clone(), opened.children) .map_err(|error| { record_runtime_failure( @@ -1415,6 +1385,7 @@ impl Runtime { })?; let agent = Agent::builder() .cancellation(controller.handle()) + .observer(crate::fatal::RetryLog) .model(adapter.clone()) .telemetry(self.agentkit_telemetry()) .add_tool_source(self.compose_with_jobs( @@ -1438,7 +1409,12 @@ impl Runtime { ) })?; let driver = match agent - .start(SessionConfig::new(session_id.clone()).without_cache()) + .start( + SessionConfig::new(session_id.clone()).with_cache( + PromptCacheRequest::automatic() + .with_key(crate::tools::subagent::prompt_cache_key(&session_id)), + ), + ) .await { Ok(driver) => driver, @@ -1550,7 +1526,10 @@ impl Runtime { format!("compaction-{session}"), ) .map_err(LoopError::InvalidState)?; - let subagents = self.subagents.fresh(); + let subagents = self + .subagents + .fresh() + .with_prompt_cache_key(crate::tools::subagent::prompt_cache_key(&session)); let builder = Agent::builder() .model(self.adapter.clone()) .telemetry(self.agentkit_telemetry()) @@ -1567,7 +1546,12 @@ impl Runtime { let builder = builder.cancellation(controller.handle()); let mut driver = builder .build()? - .start(SessionConfig::new(session).without_cache()) + .start( + SessionConfig::new(session.clone()).with_cache( + PromptCacheRequest::automatic() + .with_key(crate::tools::subagent::prompt_cache_key(&session)), + ), + ) .await?; drive(&mut driver).await } @@ -1656,18 +1640,16 @@ impl Runtime { None => (None, None, None), }; let is_fork = forked_transcript.is_some(); - let initial = if let Some(transcript) = forked_transcript { + let initial = if let Some(mut transcript) = forked_transcript { if request.resume { return Err(AcpRuntimeError::Loop( "a forked transcript requires a new session identity".into(), )); } + crate::transcript::rebind_forked_transcript(&mut transcript, &request.id); transcript } else if request.resume { - vec![Item::text( - ItemKind::System, - self.system_prompt(self.base_depth), - )] + vec![Item::text(ItemKind::System, self.system_prompt())] } else { self.initial_transcript(self.base_depth) .await @@ -1729,16 +1711,19 @@ impl Runtime { format!("compaction-{}", crate::session::new_id()), ) .map_err(AcpRuntimeError::Loop)?; + let prompt_cache_key = crate::tools::subagent::prompt_cache_key(&session_id); let subagents = self .subagents .fresh_for_workspace(additional_directories, parent_context) + .with_prompt_cache_key(prompt_cache_key.clone()) .with_observer(opened.observer.clone(), opened.children) .map_err(AcpRuntimeError::Loop)?; let task_manager = background_task_manager(); let tasks = task_manager.handle(); let background_jobs = BackgroundJobs::default(); let canonical_transcript = opened.transcript.clone(); - let mut session_config = SessionConfig::new(session_id.clone()).without_cache(); + let mut session_config = SessionConfig::new(session_id.clone()) + .with_cache(PromptCacheRequest::automatic().with_key(prompt_cache_key)); if context.response_attempt_replacement { session_config = session_config.with_response_attempt_supersession(); } @@ -1755,6 +1740,7 @@ impl Runtime { .task_manager(task_manager) .mutator(compactor) .observer(context.integration.as_ref().clone()) + .observer(crate::fatal::RetryLog) .transcript_observer(opened.observer) .transcript(opened.transcript) .cancellation(context.cancellation) @@ -1777,7 +1763,7 @@ impl Runtime { } async fn initial_transcript(&self, depth: usize) -> Result, String> { - let mut transcript = load_initial_transcript(&self.root, self.system_prompt(depth)).await?; + let mut transcript = load_initial_transcript(&self.root, self.system_prompt()).await?; let origin = if depth > 0 { crate::session::SUBAGENT_SESSION_ORIGIN } else { @@ -1790,31 +1776,15 @@ impl Runtime { Ok(transcript) } - fn system_prompt(&self, depth: usize) -> String { - let delegation_context = if depth > 0 { - format!("{SUBAGENT_SYSTEM_PROMPT_MARKER} Investigate it and carry out the work.\n\n") - } else { - String::new() - }; + fn system_prompt(&self) -> String { format!( concat!( "You are a coding agent using Kit version {} as your harness, working in {}. This is your cwd and project context, not a filesystem boundary. ", "Make minimal changes, inspect before editing, and run the smallest useful check. ", - "Keep tool output lean: use targeted paths, ranges, filters, and bounded `head`/`tail` output. Do not dump whole trees, generated files, long successful build logs, credential files, or environment contents.\n\n", - "Use compose as a dependency graph: independent calls and `for` iterations run concurrently, including effectful calls; ", - "express required ordering with data dependencies or `after`, and use `fold` only for reductions or genuinely sequential chains. ", - "Parallelize independent work deliberately. Prefer one compose program whenever the remaining tool graph is known: keep intermediate results inside it when they can directly drive downstream work, and return only the bare minimum information necessary to plan the next turn or provide the final answer. ", - "Background long-running compose work across turn boundaries, including monitors that wait or poll for EXTERNAL events or state changes. ", - "Set the outer `background` argument to `true` to detach immediately or to a positive integer to wait that many seconds before detaching. ", - "After detaching, continue any independent work, including launching more detached work. When no independent work remains and you need background results, STOP: end your turn now. ", - "Stopping is a valid intermediate response, not completion or abandonment of the user's task. ", - "Do not issue additional tool calls to wait or poll for a background tool call to finish, or to keep the turn alive; the harness automatically resumes you when it finishes. ", - "Keep work foregrounded when the next step needs its result in the current turn, and do not treat backgrounding as durable job execution.\n\n", - "{}" + "Keep tool output lean: use targeted paths, ranges, filters, and bounded `head`/`tail` output. Do not dump whole trees, generated files, long successful build logs, credential files, or environment contents.\n\n" ), env!("CARGO_PKG_VERSION"), self.root.display(), - delegation_context ) } } @@ -2635,7 +2605,10 @@ fn background_requested(request: &ToolRequest) -> bool { ) } +const COMPOSE_GUIDANCE: &str = "\n\nPrefer one program whenever the remaining tool graph is known: keep intermediate results inside it when they can directly drive downstream work, and return only what you need to plan the next step or answer."; + fn backgroundable_spec(mut spec: ToolSpec) -> ToolSpec { + spec.description.push_str(COMPOSE_GUIDANCE); if let Some(properties) = spec .input_schema .get_mut("properties") @@ -2651,7 +2624,7 @@ fn backgroundable_spec(mut spec: ToolSpec) -> ToolSpec { properties.insert( "background".into(), Value::Object(Map::from_iter([ - ("description".into(), Value::String("Run immediately in the background when true, or move to the background after this many seconds. False keeps the call in the foreground.".into())), + ("description".into(), Value::String("Run immediately in the background when true, or move to the background after this many seconds; false keeps the call in the foreground. Use it for long-running work or for waiting on external events while you have other work to do. You are resumed automatically when it finishes: once no independent work remains, end your turn instead of polling or making calls to keep the turn alive. Ending the turn this way does not complete the task. Keep a call in the foreground when your next step needs its result.".into())), ("oneOf".into(), Value::Array(vec![ Value::Object(Map::from_iter([ ("type".into(), Value::String("boolean".into())), diff --git a/src/runtime/tests.rs b/src/runtime/tests.rs index e71e0ba0..62d7a934 100644 --- a/src/runtime/tests.rs +++ b/src/runtime/tests.rs @@ -1416,46 +1416,38 @@ async fn exact_mcp_name_cannot_bypass_the_tool_meta_dispatch() { } #[test] -fn maximum_depth_compose_omits_depth_increasing_tools() { +fn delegation_tools_are_identical_at_every_depth() { let root = tempfile::tempdir().unwrap(); let runtime = Runtime::new(root.path(), "gpt-5.4").unwrap(); - let max_depth = runtime.max_subagent_depth(); - - let below_maximum = runtime.compose(max_depth - 1); - for name in ["subagent", "fork"] { - assert!( - ToolSource::get(&below_maximum.compose, &ToolName::new(name)).is_some(), - "{name} should be available below the maximum depth" - ); - } - - let subagent_description = ToolSource::get(&below_maximum.compose, &ToolName::new("subagent")) - .unwrap() - .current_spec() - .unwrap() - .description; - assert!(subagent_description.contains( - "Use this only if you uncover independent workstreams whose parallel execution would yield quicker or better results." - )); - let fork_description = ToolSource::get(&below_maximum.compose, &ToolName::new("fork")) - .unwrap() - .current_spec() - .unwrap() - .description; - assert!(fork_description.contains( - "Use this only for an independent workstream whose parallel execution would yield quicker or better results" - )); - - let at_maximum = runtime.compose(max_depth); - for name in ["subagent", "fork"] { - assert!( - ToolSource::get(&at_maximum.compose, &ToolName::new(name)).is_none(), - "{name} should not be advertised at the maximum depth" - ); + let describe = |depth, name| { + ToolSource::get(&runtime.compose(depth).compose, &ToolName::new(name)) + .unwrap() + .current_spec() + .unwrap() + }; + for name in ["subagent", "fork", "prompt"] { + let top = describe(0, name); + for depth in [1, 5] { + let nested = describe(depth, name); + assert_eq!( + top.description, nested.description, + "{name} at depth {depth}" + ); + assert_eq!( + top.input_schema, nested.input_schema, + "{name} at depth {depth}" + ); + } } assert!( - ToolSource::get(&at_maximum.compose, &ToolName::new("prompt")).is_some(), - "non-depth-increasing session tools remain available" + describe(0, "fork") + .description + .contains("when omitted, it is your current conversation") + ); + assert!( + describe(0, "subagent") + .description + .contains("A fork carries your entire conversation") ); } @@ -2502,10 +2494,7 @@ async fn initial_transcript_records_structured_session_origin() { for (depth, expected) in [ (0, crate::session::TOP_LEVEL_SESSION_ORIGIN), (1, crate::session::SUBAGENT_SESSION_ORIGIN), - ( - runtime.max_subagent_depth(), - crate::session::SUBAGENT_SESSION_ORIGIN, - ), + (5, crate::session::SUBAGENT_SESSION_ORIGIN), ] { let transcript = runtime.initial_transcript(depth).await.unwrap(); assert_eq!( @@ -2518,36 +2507,12 @@ async fn initial_transcript_records_structured_session_origin() { } #[test] -fn system_prompt_guides_compose_and_subagent_hygiene() { +fn system_prompt_is_independent_of_delegation() { let root = tempfile::tempdir().unwrap(); let runtime = Runtime::new(root.path(), "gpt-5.4").unwrap(); - let prompt = runtime.system_prompt(0); + let prompt = runtime.system_prompt(); assert!(prompt.contains("Keep tool output lean")); assert!(prompt.contains("Do not dump whole trees")); - assert!(prompt.contains("Use compose as a dependency graph")); - assert!(prompt.contains("use `fold` only for reductions or genuinely sequential chains")); - assert!(prompt.contains("Background long-running compose work across turn boundaries")); - assert!(prompt.contains("monitors that wait or poll for EXTERNAL events or state changes")); - assert!(prompt.contains("including launching more detached work")); - assert!(prompt.contains("STOP: end your turn now.")); - assert!(prompt.contains("Stopping is a valid intermediate response")); - assert!(prompt.contains("the harness automatically resumes you when it finishes")); - assert!( - prompt.contains( - "Do not issue additional tool calls to wait or poll for a background tool call to finish, or to keep the turn alive" - ) - ); - assert!(prompt.contains("the next step needs its result in the current turn")); - assert!( - prompt.contains("Prefer one compose program whenever the remaining tool graph is known") - ); - assert!(prompt.contains("keep intermediate results inside it")); - assert!(prompt.contains("return only the bare minimum information necessary")); - let delegated_prompt = runtime.system_prompt(1); - assert!(delegated_prompt.contains( - "This task was delegated to you by the primary agent. Investigate it and carry out the work." - )); - assert!(!prompt.contains("This task was delegated to you by the primary agent.")); - let max_depth_prompt = runtime.system_prompt(runtime.max_subagent_depth()); - assert!(max_depth_prompt.contains("This task was delegated to you by the primary agent.")); + assert!(!prompt.contains("compose")); + assert!(!prompt.contains("delegat")); } diff --git a/src/runtime/voice_state.rs b/src/runtime/voice_state.rs index 60ecf8bc..05ea220b 100644 --- a/src/runtime/voice_state.rs +++ b/src/runtime/voice_state.rs @@ -5,6 +5,7 @@ use std::sync::{ atomic::{AtomicBool, Ordering}, }; +const STATUS_PREFIX: &str = "[Kit voice session status] "; const ACTIVE: &str = "[Kit voice session status] A voice session is active, including while its microphone is muted. This current status supersedes earlier voice status, including resumed or summarized context. This is internal, non-actionable context; do not acknowledge it or start work because of it. Prefer backgrounding long-running compose work to keep voice responsive. Preserve dependency ordering and keep calls foregrounded when their result is needed for the next step. When only waiting for background results and no independent work remains, end the turn; do not poll or issue keepalive calls. The harness resumes work when results arrive."; const INACTIVE: &str = "[Kit voice session status] No voice session is active on this connection. This current status supersedes any earlier voice status, including in resumed or summarized context. This is internal, non-actionable context; do not acknowledge it or start work because of it. Voice-specific responsiveness guidance no longer applies; ordinary background-work and dependency-ordering instructions still apply. Existing tasks are not cancelled."; @@ -20,16 +21,30 @@ impl VoiceState { self.0.store(active, Ordering::Relaxed); } - pub(crate) fn monitor(&self, correct_history: bool) -> VoiceMonitor { + pub(crate) fn monitor(&self, history: &[Item]) -> VoiceMonitor { VoiceMonitor { current: Arc::downgrade(&self.0), - baseline: if correct_history { None } else { Some(false) }, + baseline: Some(last_status(history) == Some(ACTIVE)), } } } -/// Actor-local baseline. Resumes/forks force a correction on the first user -/// prompt; fresh sessions start with the known inactive baseline. +fn last_status(history: &[Item]) -> Option<&str> { + history + .iter() + .rev() + .find_map(|item| match item.parts.as_slice() { + [agentkit_core::Part::Text(text)] + if item.kind == agentkit_core::ItemKind::Notification + && text.text.starts_with(STATUS_PREFIX) => + { + Some(text.text.as_str()) + } + _ => None, + }) +} + +/// Actor-local baseline, seeded from the last status recorded in history. /// Autonomous work and tool continuations must never call `submit`. pub(crate) struct VoiceMonitor { current: Weak, @@ -192,7 +207,7 @@ mod tests { #[tokio::test] async fn voice_state_idle_updates_wait_for_user_and_coalesce() { let state = VoiceState::default(); - let mut monitor = state.monitor(false); + let mut monitor = state.monitor(&[]); let (mut driver, mut requests) = driver(vec![], false).await; state.set_active(true); state.set_active(false); @@ -226,7 +241,7 @@ mod tests { #[tokio::test] async fn voice_state_resume_and_owner_drop_correct_only_next_user_prompt() { let state = VoiceState::default(); - let mut monitor = state.monitor(true); + let mut monitor = state.monitor(&[Item::notification(ACTIVE)]); let (mut driver, mut requests) = driver(vec![Item::notification(ACTIVE)], false).await; assert!(matches!( driver.next().await.unwrap(), @@ -256,7 +271,7 @@ mod tests { #[test] fn voice_state_failed_submission_preserves_pending_and_concurrent_update() { let state = VoiceState::default(); - let mut monitor = state.monitor(false); + let mut monitor = state.monitor(&[]); state.set_active(true); let failure = monitor.submit(user(), |items| { assert_eq!(statuses(&items), vec![ACTIVE]); @@ -287,7 +302,7 @@ mod tests { #[tokio::test] async fn voice_state_live_and_autonomous_work_do_not_consume_pending() { let state = VoiceState::default(); - let mut monitor = state.monitor(false); + let mut monitor = state.monitor(&[]); let (mut driver, mut requests) = driver(vec![], true).await; monitor .submit(user(), |items| driver.submit_input(items)) diff --git a/src/session.rs b/src/session.rs index 380e8b01..68d6001e 100644 --- a/src/session.rs +++ b/src/session.rs @@ -248,7 +248,7 @@ pub(crate) fn clone_completed_in( let reasoning_effort = authority.reasoning_effort; let mut transcript = authority.items; crate::transcript::repair_unanswered_tool_calls(&mut transcript); - crate::transcript::sanitize_forked_transcript(&mut transcript); + crate::transcript::rebind_forked_transcript(&mut transcript, destination); let opened = open_with_initial_timestamps_in( root, directory, @@ -269,6 +269,48 @@ pub(crate) fn clone_completed_in( Ok(()) } +/// Seeds `destination` with `source`'s history before its in-flight response. +pub fn clone_inherited(root: &Path, source: &str, destination: &str) -> Result<(), String> { + let directory = default_directory()?; + validate_id(source)?; + let authority = select_authority(&directory, &canonical_workspace(root), source)? + .ok_or_else(|| format!("session {source:?} does not exist"))?; + let reasoning_effort = authority.reasoning_effort; + let mut transcript = authority.items; + while transcript + .last() + .is_some_and(|item| item.kind == ItemKind::Assistant) + { + transcript.pop(); + } + crate::transcript::repair_unanswered_tool_calls(&mut transcript); + crate::transcript::rebind_forked_transcript(&mut transcript, destination); + if let Some(first) = transcript.first_mut() { + first.metadata.insert( + SESSION_ORIGIN_METADATA_KEY.into(), + serde_json::Value::String(SUBAGENT_SESSION_ORIGIN.into()), + ); + } + let opened = open_with_initial_timestamps_in( + root, + &directory, + destination, + false, + false, + transcript, + InitialTranscriptOptions { + stamp_items: false, + commit_creation: false, + }, + )?; + if let Some(effort) = reasoning_effort { + opened.observer.set_reasoning_effort(effort)?; + } + opened.observer.commit_creation()?; + drop(opened); + Ok(()) +} + /// Removes an abandoned lock file, but never one held by a live process. /// /// This is the last-resort cleanup path for a hosting client whose server had @@ -3603,7 +3645,7 @@ mod tests { } #[test] - fn cloning_sanitizes_session_bound_continuation_metadata() { + fn cloning_rebinds_continuation_metadata_to_the_branch() { let root = tempfile::tempdir().unwrap(); let mut metadata = MetadataMap::new(); metadata.insert( @@ -3641,10 +3683,13 @@ mod tests { .metadata .contains_key("openai.responses.continuation.v1") ); - assert!( - !branch - .metadata - .contains_key("openai.responses.continuation.v1") + assert_eq!( + source.metadata["openai.responses.continuation.v1"]["session_id"], + "source" + ); + assert_eq!( + branch.metadata["openai.responses.continuation.v1"]["session_id"], + "branch" ); assert_eq!(branch.metadata["preserved"], true); } diff --git a/src/tools/mod.rs b/src/tools/mod.rs index c37ed81f..dcea40a6 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -6,7 +6,7 @@ pub(crate) mod mcp; mod observed; mod read_file; mod shell; -mod subagent; +pub(crate) mod subagent; pub use crate::credentials::CredentialStorage; pub use a2a::A2aTool; @@ -18,9 +18,7 @@ pub use observed::Observed; pub(crate) use observed::shared as observe_shared; pub use read_file::ReadFileTool; pub use shell::ShellTool; -pub use subagent::{ - CloseTool, ForkTool, PromptTool, SteerTool, SubagentTool, Subagents, SubagentsTool, -}; +pub use subagent::{CloseTool, ForkTool, PromptTool, SubagentTool, Subagents, SubagentsTool}; mod image_gen; pub use image_gen::ImageGenTool; diff --git a/src/tools/subagent.rs b/src/tools/subagent.rs index 20d22c42..a172fc78 100644 --- a/src/tools/subagent.rs +++ b/src/tools/subagent.rs @@ -3,7 +3,7 @@ use crate::acp_child::transcript; use std::{ collections::{HashMap, HashSet}, - path::PathBuf, + path::{Path, PathBuf}, sync::{Arc, Mutex}, }; @@ -17,10 +17,83 @@ use serde::{Deserialize, Serialize}; #[cfg(test)] use serde_json::json; use serde_json::{Map, Value}; -use tokio::sync::{Mutex as AsyncMutex, OwnedSemaphorePermit, Semaphore, oneshot}; +use tokio::sync::{Mutex as AsyncMutex, Notify, OwnedSemaphorePermit, Semaphore, oneshot}; use tracing::Instrument as _; const MAX_LIVE_SUBAGENTS: usize = 120; +const MAX_TREE_SUBAGENTS: usize = MAX_LIVE_SUBAGENTS; +const DELEGATION_USAGE: &str = "Delegate work that can proceed in parallel with other work, or a self-contained piece of work whose intermediate reads, attempts and output your conversation does not need, when only its outcome matters for what comes next, such as one step of a longer sequence. A subagent parallelizes model work, not tool calls: one `compose` call already reads, edits and runs across many files at once, and each subagent adds its own sequence of requests, so delegation pays off when a piece needs substantial reasoning or writing of its own. Keep work yourself when later steps depend on its details rather than its outcome.\n\nIn the prompt, state which files or directories it owns, any interface it must honor or provide, the command that verifies its work, and what to return. Only its final message enters your conversation, so ask for the outcome you need."; +const FRESH_USAGE: &str = "A fresh subagent starts from your prompt alone: its requests carry only that prompt and what it reads itself, and it forms its own view without your conclusions. A fork carries your entire conversation in every request it makes. When what the work needs from your conversation fits in the prompt (paths, findings, constraints), or when its judgement should be independent of yours, a fresh subagent is cheaper and unbiased. The delegation criteria and brief contents are the same as for `fork`. A fresh subagent knows nothing you have learned, so also include the relevant paths, findings and constraints."; +pub(crate) const TREE_SLOTS_ENV: &str = "KIT_SUBAGENT_TREE_SLOTS"; +pub(crate) const PROMPT_CACHE_KEY_ENV: &str = "KIT_PROMPT_CACHE_KEY"; + +/// The delegation tree's shared prompt cache key: inherited, or this session's. +pub(crate) fn prompt_cache_key(session_id: &str) -> String { + std::env::var(PROMPT_CACHE_KEY_ENV) + .ok() + .filter(|key| !key.is_empty()) + .unwrap_or_else(|| session_id.to_owned()) +} + +#[derive(Debug)] +pub(crate) struct Permit { + _local: OwnedSemaphorePermit, + _slot: std::fs::File, +} + +type TreeSlots = Arc>>; + +/// Slot directory shared by every Kit process in one delegation tree. +fn tree_slot_directory(slots: &TreeSlots) -> Result<&PathBuf, ChildError> { + slots + .get_or_init(|| { + let directory = match std::env::var_os(TREE_SLOTS_ENV) { + Some(directory) => PathBuf::from(directory), + None => std::env::temp_dir().join(format!("kit-subagents-{}", session::new_id())), + }; + std::fs::create_dir_all(&directory) + .map_err(|error| format!("could not create {}: {error}", directory.display()))?; + Ok(directory) + }) + .as_ref() + .map_err(|error| ChildError::Failed(error.clone())) +} + +/// Holds one of the tree-wide slots until dropped or the process exits. +fn tree_slot(directory: &Path) -> Result { + for slot in 0..MAX_TREE_SUBAGENTS { + let file = std::fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(directory.join(format!("slot-{slot}"))) + .map_err(|error| { + ChildError::Failed(format!("could not open subagent slot: {error}")) + })?; + if file.try_lock().is_ok() { + return Ok(file); + } + } + Err(ChildError::Failed(format!( + "subagent limit for this delegation tree ({MAX_TREE_SUBAGENTS}) reached" + ))) +} + +const DELEGATION_FRAME: &str = "You are a subagent working concurrently with the agent that started you. Do only the assignment below and stay within the files it gives you, because others may be editing the rest. Your final message is returned to the parent."; +const INHERITED_FRAME: &str = "You are a subagent working concurrently with the agent that started you. The conversation above was copied from another session; background calls started in it will not reach you. Do only the assignment below and stay within the files it gives you, because others may be editing the rest. Your final message is returned to the parent."; + +fn framed(prompt: ChildPrompt, frame: &str) -> ChildPrompt { + match prompt { + ChildPrompt::Text(text) => ChildPrompt::Text(format!("{frame}\n\n{text}")), + ChildPrompt::Blocks(mut blocks) => { + blocks.insert( + 0, + agentkit_acp::ContentBlock::Text(agentkit_acp::TextContent::new(frame)), + ); + ChildPrompt::Blocks(blocks) + } + } +} const MAX_DISPLAY_NAME_LEN: usize = 32; fn normalize_display_name(candidate: &str) -> Option { @@ -81,6 +154,28 @@ fn child_error_is_terminal(error: &ChildError, child: &ChildSession) -> bool { } } +async fn wait_or_cancel( + changed: std::pin::Pin<&mut tokio::sync::futures::Notified<'_>>, + cancellation: &TurnCancellation, +) -> Result<(), ChildError> { + match select(changed, Box::pin(cancellation.cancelled())).await { + Either::Left(_) => Ok(()), + Either::Right(_) => Err(ChildError::Cancelled), + } +} + +fn failed_turn(id: &str, name: &str, error: ChildError) -> ChildError { + let ChildError::Failed(message) = error else { + return error; + }; + let cause = crate::fatal::latest_cause(id) + .map(|cause| format!(" Cause: {cause}.")) + .unwrap_or_default(); + ChildError::Failed(format!( + "subagent {id:?} ({name}) failed during its turn: {message}.{cause} It is idle and keeps its conversation and edits; continue it with prompt({{subagent: {id:?}, prompt}})." + )) +} + fn task_summary(prompt: &str) -> String { let normalized = prompt.split_whitespace().collect::>().join(" "); if normalized.is_empty() { @@ -104,12 +199,14 @@ type EventSink = Arc Result<(), ()> + Send + Sy #[derive(Clone)] pub struct Subagents { config: ChildConfig, - max_depth: usize, + tree_slots: TreeSlots, sessions: Arc>>, capacity: Arc, event_sink: EventSink, transcripts: Arc, observer: Option, + /// Signalled on every lifecycle event so deliveries can wait for a turn to end. + changed: Arc, } struct SessionEntry { @@ -137,7 +234,9 @@ struct State { child: Option, recovery: Option, forking: Option, - permit: Option, + permit: Option, + /// Prompt cache key the child process runs under; `None` uses its own id. + cache_key: Option, } impl State { @@ -255,6 +354,7 @@ struct CreateOptions { harness: Option, model: Option, cwd: Option, + inherit: Option, } struct ForkSuccess { @@ -278,19 +378,20 @@ struct ForkOperation { depth: usize, cancellation: TurnCancellation, contract: Option>, - permit: OwnedSemaphorePermit, + permit: Permit, native_fork: bool, } impl Subagents { - pub(crate) fn new(config: ChildConfig, max_depth: usize) -> Self { + pub(crate) fn new(config: ChildConfig) -> Self { Self { config, - max_depth, + tree_slots: TreeSlots::default(), sessions: Arc::default(), capacity: Arc::new(Semaphore::new(MAX_LIVE_SUBAGENTS)), observer: None, transcripts: Arc::default(), + changed: Arc::default(), event_sink: Arc::new(|event| { events::emit(event); Ok(()) @@ -298,12 +399,17 @@ impl Subagents { } } + pub(crate) fn with_prompt_cache_key(mut self, key: String) -> Self { + self.config.prompt_cache_key = Some(key); + self + } + pub(crate) fn child_config(&self) -> ChildConfig { self.config.clone() } pub(crate) fn fresh(&self) -> Self { - Self::new(self.config.clone(), self.max_depth) + Self::new(self.config.clone()) } pub(crate) fn fresh_for_workspace( @@ -316,7 +422,7 @@ impl Subagents { if let Some((id, name)) = parent { config = config.with_parent_context(id, name); } - Self::new(config, self.max_depth) + Self::new(config) } pub(crate) async fn read_transcript( @@ -333,6 +439,7 @@ impl Subagents { } fn emit_event(&self, mut event: events::RuntimeEvent) { + self.changed.notify_waiters(); if let events::RuntimeEvent::SubagentStateChanged { id, generation, @@ -395,7 +502,6 @@ impl Subagents { cancellation: TurnCancellation, contract: Option<&OutputContract>, ) -> Result { - self.check_depth(depth)?; let permit = self.reserve()?; let id = session::new_id(); let CreateOptions { @@ -403,9 +509,18 @@ impl Subagents { harness, model, cwd, + inherit, } = options; let root = self.resolve_root(cwd)?; - let harness = harness.unwrap_or_else(|| self.config.default_harness.clone()); + let cache_key = inherit + .is_some() + .then(|| self.config.prompt_cache_key.clone()) + .flatten(); + let harness = if inherit.is_some() { + crate::acp_child::BUILTIN_HARNESS.to_string() + } else { + harness.unwrap_or_else(|| self.config.default_harness.clone()) + }; if !self.config.harnesses.contains(&harness) { return Err(ChildError::Failed(format!( "unknown ACP harness {harness:?}" @@ -442,13 +557,36 @@ impl Subagents { recovery: None, forking: None, permit: Some(permit), + cache_key: cache_key.clone(), }, )?; - let persisted = kit.then(|| (id.clone(), false)); + let prompt = match (kit, inherit.is_some()) { + (false, _) => prompt, + (true, false) => framed(prompt, DELEGATION_FRAME), + (true, true) => framed(prompt, INHERITED_FRAME), + }; + let seeded = inherit.is_some(); + if let Some(source) = inherit { + let transcript_root = self.config.root.clone(); + let branch_id = id.clone(); + let cloned = tokio::task::spawn_blocking(move || { + session::clone_inherited(&transcript_root, &source, &branch_id) + }) + .await + .map_err(|error| ChildError::Failed(format!("transcript clone task failed: {error}"))) + .and_then(|result| result.map_err(ChildError::Failed)); + if let Err(error) = cloned { + self.fail_removed_and_remove(&id, &state).await; + return Err(error); + } + } + let persisted = kit.then(|| (id.clone(), seeded)); let child_config = self .config .clone() .with_root(root) + .with_tree_slots(tree_slot_directory(&self.tree_slots)?.clone()) + .with_cache_lineage(cache_key) .with_parent_context(id.clone(), state.lock().await.name.clone()); { let locked = state.lock().await; @@ -507,11 +645,7 @@ impl Subagents { .await { Ok(output) => output, - Err(error) => { - self.fail_removed_and_remove(&id, &state).await; - let _ = child.close().await; - return Err(error); - } + Err(error) => return Err(self.retain_failed_turn(&id, &state, &child, error).await), }; let (output, updates) = turn_output(output, contract); let mut locked = state.lock().await; @@ -540,10 +674,6 @@ impl Subagents { }) } - async fn steer(&self, id: &str, prompt: ChildPrompt) -> Result { - self.steer_generation(id, None, prompt).await - } - pub(crate) async fn steer_generation( &self, id: &str, @@ -593,6 +723,102 @@ impl Subagents { child.steer_generation(prompt, Some(generation)).await } + /// Delivers `prompt` by whichever method the subagent's state allows and + /// returns its value once the turn that handled the prompt has ended. + async fn deliver( + &self, + id: &str, + prompt: ChildPrompt, + cancellation: TurnCancellation, + contract: Option<&OutputContract>, + ) -> Result { + loop { + let state = self.lookup_id(id)?; + let changed = self.changed.notified(); + tokio::pin!(changed); + changed.as_mut().enable(); + let (status, forking, generation, handle_generation, child) = { + let locked = state.lock().await; + self.check_active(&locked)?; + ( + locked.status, + locked.forking.is_some(), + locked.generation, + locked.handle_generation, + locked.child.clone(), + ) + }; + if status == SubagentStatus::Idle && !forking { + let prior = SubagentValue { + id: id.to_owned(), + name: None, + output: Value::Null, + generation: handle_generation, + updates: None, + }; + return self.prompt(prior, prompt, cancellation, contract).await; + } + // An injected message has no turn of its own to carry an output contract. + if status == SubagentStatus::Working + && !forking + && contract.is_none() + && let Some(child) = child + { + match child + .steer_generation(prompt.clone(), Some(generation)) + .await + { + Ok(_) => { + return self.turn_value(id, &state, generation, &cancellation).await; + } + Err(error) if child_error_is_terminal(&error, &child) => return Err(error), + Err(_) => {} + } + } + wait_or_cancel(changed, &cancellation).await?; + } + } + + /// Waits for `generation` to end and returns what it produced. + async fn turn_value( + &self, + id: &str, + state: &Arc>, + generation: u64, + cancellation: &TurnCancellation, + ) -> Result { + loop { + let changed = self.changed.notified(); + tokio::pin!(changed); + changed.as_mut().enable(); + { + let locked = state.lock().await; + self.check_active(&locked)?; + let ended = locked.generation != generation + || matches!(locked.status, SubagentStatus::Idle) && locked.outcome.is_some(); + if ended { + if locked.generation == generation + && locked.outcome == Some(GenerationOutcome::Failed) + { + return Err(failed_turn( + id, + &locked.name, + ChildError::Failed("the turn that received this prompt failed".into()), + )); + } + return Ok(SubagentValue { + id: id.to_owned(), + name: Some(locked.name.clone()), + output: locked.output.clone(), + generation, + updates: locked.updates.clone(), + }); + } + } + wait_or_cancel(changed, cancellation).await?; + } + } + async fn prompt( &self, prior: SubagentValue, @@ -678,21 +904,21 @@ impl Subagents { Err(error) => { if child_error_is_terminal(&error, &child) { self.fail_removed_and_remove(&prior.id, &state).await; - } else { - let mut locked = state.lock().await; - if locked.status != SubagentStatus::Removed { - locked.status = SubagentStatus::Idle; - locked.outcome = Some(GenerationOutcome::Failed); - locked.generation_finished_at_unix_ms = Some(events::now_millis()); - // A failed call returns no replacement handle, so preserve the - // accepted handle generation for a retry while keeping lifecycle - // generations monotonic. - let event = locked.runtime_event(prior.id.clone()); - drop(locked); - self.emit_event(event); - } + return Err(error); } - Err(error) + let mut locked = state.lock().await; + if locked.status != SubagentStatus::Removed { + locked.status = SubagentStatus::Idle; + locked.outcome = Some(GenerationOutcome::Failed); + locked.generation_finished_at_unix_ms = Some(events::now_millis()); + // A failed call returns no replacement handle, so preserve the + // accepted handle generation for a retry while keeping lifecycle + // generations monotonic. + let event = locked.runtime_event(prior.id.clone()); + drop(locked); + self.emit_event(event); + } + Err(failed_turn(&prior.id, &name, error)) } } } @@ -706,7 +932,6 @@ impl Subagents { cancellation: TurnCancellation, contract: Option>, ) -> Result { - self.check_depth(depth)?; let permit = self.reserve()?; let source_state = self.lookup(&prior)?; self.reconnect(&prior, &source_state, &cancellation).await?; @@ -846,6 +1071,14 @@ impl Subagents { return Err(ChildError::Cancelled); } + let cache_key = Some( + source_state + .lock() + .await + .cache_key + .clone() + .unwrap_or_else(|| source_id.clone()), + ); let now = events::now_millis(); let state = self.insert_starting( id.clone(), @@ -870,6 +1103,7 @@ impl Subagents { recovery: None, forking: None, permit: None, + cache_key: cache_key.clone(), }, )?; let branch_name = state.lock().await.name.clone(); @@ -902,6 +1136,8 @@ impl Subagents { .config .clone() .with_root(root) + .with_tree_slots(tree_slot_directory(&self.tree_slots)?.clone()) + .with_cache_lineage(cache_key) .with_parent_context(id.clone(), branch_name.clone()); let child_result = if native_fork { let parent = kit.then(|| (id.clone(), branch_name)); @@ -994,18 +1230,21 @@ impl Subagents { .prompt_generation( id.clone(), generation, - structured_prompt(prompt, contract.as_deref()), + structured_prompt( + if kit { + framed(prompt, INHERITED_FRAME) + } else { + prompt + }, + contract.as_deref(), + ), cancellation, self.transcripts.get(&id, generation).ok(), ) .await { Ok(output) => output, - Err(error) => { - return Err(self - .cleanup_installed_child(&id, &state, &child, error) - .await); - } + Err(error) => return Err(self.retain_failed_turn(&id, &state, &child, error).await), }; let (output, updates) = turn_output(output, contract.as_deref()); let mut locked = state.lock().await; @@ -1108,9 +1347,7 @@ impl Subagents { )); } let reconnect_permit = if locked.child.is_none() && locked.recovery.is_some() { - Some(self.capacity.clone().try_acquire_owned().map_err(|_| { - ChildError::Failed("live subagent session limit (120) reached".into()) - })?) + Some(self.acquire_permit()?) } else { None }; @@ -1131,6 +1368,7 @@ impl Subagents { .config .clone() .with_root(record.root) + .with_tree_slots(tree_slot_directory(&self.tree_slots)?.clone()) .with_parent_context(id.into(), record.name); let cancellation = cancellation.clone(); // Tombstoned cleanup owns startup and capacity independently of @@ -1172,12 +1410,16 @@ impl Subagents { } fn lookup(&self, prior: &SubagentValue) -> Result>, ChildError> { + self.lookup_id(&prior.id) + } + + fn lookup_id(&self, id: &str) -> Result>, ChildError> { self.sessions .lock() .map_err(|_| ChildError::Failed("subagent registry lock was poisoned".into()))? - .get(&prior.id) + .get(id) .map(|entry| Arc::clone(&entry.state)) - .ok_or_else(|| ChildError::Failed(format!("unknown subagent session {:?}", prior.id))) + .ok_or_else(|| ChildError::Failed(format!("unknown subagent session {id:?}"))) } fn insert_starting( @@ -1210,7 +1452,7 @@ impl Subagents { Ok(state) } - fn reserve(&self) -> Result { + fn reserve(&self) -> Result { let mut removed_events = Vec::new(); let mut sessions = self .sessions @@ -1239,10 +1481,20 @@ impl Subagents { for event in removed_events { self.emit_event(event); } - Arc::clone(&self.capacity).try_acquire_owned().map_err(|_| { - ChildError::Failed(format!( - "live subagent session limit ({MAX_LIVE_SUBAGENTS}) reached" - )) + self.acquire_permit() + } + + fn acquire_permit(&self) -> Result { + let local = Arc::clone(&self.capacity) + .try_acquire_owned() + .map_err(|_| { + ChildError::Failed(format!( + "live subagent session limit ({MAX_LIVE_SUBAGENTS}) reached" + )) + })?; + Ok(Permit { + _local: local, + _slot: tree_slot(tree_slot_directory(&self.tree_slots)?)?, }) } @@ -1311,7 +1563,7 @@ impl Subagents { async fn cleanup_uninstalled_child( &self, child: ChildSession, - permit: Option, + permit: Option, error: ChildError, ) -> ChildError { match child.close().await { @@ -1326,6 +1578,34 @@ impl Subagents { } } + /// A live child keeps its conversation after a failed turn, so it stays + /// promptable; only a dead child is retired. + async fn retain_failed_turn( + &self, + id: &str, + state: &Arc>, + child: &ChildSession, + error: ChildError, + ) -> ChildError { + if child_error_is_terminal(&error, child) { + return self.cleanup_installed_child(id, state, child, error).await; + } + let mut locked = state.lock().await; + if locked.status == SubagentStatus::Removed { + return error; + } + locked.status = SubagentStatus::Idle; + locked.forking = None; + locked.outcome = Some(GenerationOutcome::Failed); + locked.generation_finished_at_unix_ms = Some(events::now_millis()); + let _ = self.persist_state(&locked, session::ChildLifecycle::Idle); + let name = locked.name.clone(); + let event = locked.runtime_event(id.to_string()); + drop(locked); + self.emit_event(event); + failed_turn(id, &name, error) + } + async fn cleanup_installed_child( &self, id: &str, @@ -1395,7 +1675,7 @@ impl Subagents { Self::watch_permit_until_process_exit(permit, child); } - fn watch_permit_until_process_exit(permit: Option, child: &ChildSession) { + fn watch_permit_until_process_exit(permit: Option, child: &ChildSession) { let Some(permit) = permit else { return; }; @@ -1532,16 +1812,6 @@ impl Subagents { ))) } } - fn check_depth(&self, depth: usize) -> Result<(), ChildError> { - if depth < self.max_depth { - Ok(()) - } else { - Err(ChildError::Failed(format!( - "subagent depth limit ({}) reached", - self.max_depth - ))) - } - } } #[derive(Clone)] @@ -1556,11 +1826,6 @@ pub struct PromptTool { spec: ToolSpec, } #[derive(Clone)] -pub struct SteerTool { - manager: Subagents, - spec: ToolSpec, -} -#[derive(Clone)] pub struct ForkTool { manager: Subagents, depth: usize, @@ -1741,7 +2006,25 @@ fn continuation_schema() -> serde_json::Value { ( "properties".into(), Value::Object(Map::from_iter([ - ("subagent".into(), value_schema()), + ( + "subagent".into(), + Value::Object(Map::from_iter([ + ( + "description".into(), + Value::from("The subagent's id, or a subagent value returned for it."), + ), + ( + "oneOf".into(), + Value::Array(vec![ + Value::Object(Map::from_iter([( + "type".into(), + Value::from("string"), + )])), + value_schema(), + ]), + ), + ])), + ), ("prompt".into(), crate::acp_child::prompt::schema()), ( "output_schema".into(), @@ -1811,19 +2094,8 @@ fn call_id_schema() -> serde_json::Value { impl SubagentTool { pub fn new(manager: Subagents, depth: usize) -> Self { let harnesses = manager.harness_references(); - let usage = if depth == 1 { - concat!( - "Use this only if you uncover independent workstreams whose parallel execution would yield quicker or better results. ", - "Give each subagent a focused assignment based on what you discovered, and synthesize its findings into your response. " - ) - } else { - concat!( - "Use a fresh subagent for work that changes phase or objective instead of carrying unrelated history. ", - "Keep outputs focused, pass only necessary context, reuse sessions only when continuity helps, and close subagents when no longer needed. " - ) - }; let description = format!( - "Start a parent-owned configured ACP harness, preferably assign a concise role-oriented display name, prompt it, and return its reusable session value. {usage}Omit `harness` and `model` unless the user or active workflow explicitly supplies the exact override or a configured alias. Never choose an override based on your own model, provider, publisher, familiarity, cost, or perceived quality; advertised choices indicate availability, not preference." + "Start a subagent in a fresh conversation, optionally on a configured ACP harness or model, and return its reusable session value. Prefer a short role-oriented display name. {FRESH_USAGE} Omit `harness` and `model` unless the user or active workflow explicitly supplies the exact override or a configured alias. Never choose an override based on your own model, provider, publisher, familiarity, cost, or perceived quality; advertised choices indicate availability, not preference." ); let input_schema = Value::Object(Map::from_iter([ ("type".into(), Value::from("object")), @@ -1904,45 +2176,13 @@ impl SubagentTool { } } } -impl SteerTool { - pub fn new(manager: Subagents) -> Self { - Self { - manager, - spec: ToolSpec::new( - ToolName::new("steer"), - "Inject guidance into a working ACP subagent without starting a new turn. Use an ID from subagents while its originating compose is backgrounded. Requires ACP v2 with advertised steer support. Returns acceptance, not delivery or completion; never cancels or re-prompts unsupported peers.", - Value::Object(Map::from_iter([ - ("type".into(), Value::from("object")), - ("properties".into(), Value::Object(Map::from_iter([ - ("id".into(), Value::Object(Map::from_iter([ - ("type".into(), Value::from("string")), - ]))), - ("prompt".into(), crate::acp_child::prompt::schema()), - ]))), - ("required".into(), Value::from(vec!["id", "prompt"])), - ("additionalProperties".into(), Value::Bool(false)), - ])), - ) - .with_output_schema(Value::Bool(true)) - .with_annotations(ToolAnnotations::new()), - } - } -} - -#[derive(Deserialize)] -#[serde(deny_unknown_fields)] -struct SteerInput { - id: String, - prompt: ChildPrompt, -} - impl PromptTool { pub fn new(manager: Subagents) -> Self { Self { manager, spec: ToolSpec::new( ToolName::new("prompt"), - "Re-prompt the same completed ACP subagent session using a prior subagent value.", + "Send a message to an existing subagent and return its value once the turn that handled the message ends. It works in any state: an idle subagent, including one whose last turn failed, starts a new turn; a working one receives the message within its current turn when it supports that, otherwise in a new turn after the current one ends.", continuation_schema(), ) .with_output_schema(value_schema()) @@ -1952,13 +2192,8 @@ impl PromptTool { } impl ForkTool { pub fn new(manager: Subagents, depth: usize) -> Self { - let usage = if depth == 1 { - " Use this only for an independent workstream whose parallel execution would yield quicker or better results, and synthesize its findings into your response." - } else { - "" - }; let description = format!( - "Fork a completed ACP subagent session using native capability support or the isolated Kit fallback, preferably assign the fork a concise role-oriented display name, prompt it, and return the new session value.{usage}" + "Start a subagent that works concurrently with you from a copy of an existing conversation, and return its reusable session value. The copied conversation depends on `subagent`: when omitted, it is your current conversation, including every message, tool call and tool result before this call; when given a prior subagent value, it is that subagent's completed session, which holds only what that subagent saw, not your conversation. Your prompt follows the copied conversation. Prefer a short role-oriented display name.\n\n{DELEGATION_USAGE}" ); Self { manager, @@ -1971,7 +2206,18 @@ impl ForkTool { ( "properties".into(), Value::Object(Map::from_iter([ - ("subagent".into(), value_schema()), + ("subagent".into(), { + let mut schema = value_schema(); + if let Value::Object(schema) = &mut schema { + schema.insert( + "description".into(), + Value::from( + "Completed subagent session to copy. Omit to copy your current conversation.", + ), + ); + } + schema + }), ("prompt".into(), crate::acp_child::prompt::schema()), ("name".into(), display_name_schema()), ( @@ -1992,10 +2238,7 @@ impl ForkTool { ), ])), ), - ( - "required".into(), - Value::Array(vec![Value::from("subagent"), Value::from("prompt")]), - ), + ("required".into(), Value::Array(vec![Value::from("prompt")])), ("additionalProperties".into(), Value::from(false)), ])), ) @@ -2051,15 +2294,31 @@ struct Input { #[derive(Deserialize)] #[serde(deny_unknown_fields)] struct Continuation { - subagent: SubagentValue, + subagent: SubagentRef, prompt: ChildPrompt, #[serde(default, deserialize_with = "deserialize_output_schema")] output_schema: Option, } +#[derive(Deserialize)] +#[serde(untagged)] +enum SubagentRef { + Id(String), + Value(SubagentValue), +} + +impl SubagentRef { + fn id(&self) -> &str { + match self { + Self::Id(id) => id, + Self::Value(value) => &value.id, + } + } +} + #[derive(Deserialize)] #[serde(deny_unknown_fields)] struct ForkInput { - subagent: SubagentValue, + subagent: Option, prompt: ChildPrompt, name: Option, #[serde(default, deserialize_with = "deserialize_output_schema")] @@ -2196,6 +2455,7 @@ impl Tool for SubagentTool { harness: input.harness, model: input.model, cwd: input.cwd, + inherit: None, }, self.depth, cancellation(context), @@ -2206,36 +2466,6 @@ impl Tool for SubagentTool { } } -#[async_trait] -impl Tool for SteerTool { - fn spec(&self) -> &ToolSpec { - &self.spec - } - - async fn invoke( - &self, - request: ToolRequest, - _context: &mut ToolContext<'_>, - ) -> Result { - let input: SteerInput = serde_json::from_value(request.input.clone()) - .map_err(|error| ToolError::InvalidInput(error.to_string()))?; - let receipt = - self.manager - .steer(&input.id, input.prompt) - .await - .map_err(|error| match error { - ChildError::Cancelled | ChildError::TerminalCancelled => ToolError::Cancelled, - ChildError::Failed(error) | ChildError::TerminalFailed(error) => { - ToolError::ExecutionFailed(error) - } - })?; - Ok(ToolResult::new(ToolResultPart::success( - request.call_id, - ToolOutput::structured(receipt), - ))) - } -} - #[async_trait] impl Tool for PromptTool { fn spec(&self) -> &ToolSpec { @@ -2252,8 +2482,8 @@ impl Tool for PromptTool { result( request, self.manager - .prompt( - input.subagent, + .deliver( + input.subagent.id(), input.prompt, cancellation(context), contract.as_ref(), @@ -2276,19 +2506,39 @@ impl Tool for ForkTool { let input: ForkInput = serde_json::from_value(request.input.clone()) .map_err(|e| ToolError::InvalidInput(e.to_string()))?; let contract = input.output_schema.map(OutputContract::new).transpose()?; - result( - request, - self.manager - .fork( - input.subagent, - input.prompt, - input.name, - self.depth, - cancellation(context), - contract.map(Arc::new), - ) - .await, - ) + let outcome = match input.subagent { + Some(subagent) => { + self.manager + .fork( + subagent, + input.prompt, + input.name, + self.depth, + cancellation(context), + contract.map(Arc::new), + ) + .await + } + None => { + let source = request.session_id.0.clone(); + self.manager + .create( + input.prompt, + CreateOptions { + name: input.name, + harness: None, + model: None, + cwd: None, + inherit: Some(source), + }, + self.depth, + cancellation(context), + contract.as_ref(), + ) + .await + } + }; + result(request, outcome) } } @@ -2317,27 +2567,26 @@ mod steer_tests { fn manager_with_disconnected_session( root: &Path, ) -> (Subagents, Arc>, SubagentValue) { - let manager = Subagents::new( - ChildConfig { - root: root.to_path_buf(), - additional_directories: Vec::new(), - model: "test".into(), - provider: Default::default(), - reasoning_effort: None, - openrouter_api_key: None, - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: false, - mcp_config: None, - credential_storage: Default::default(), - telemetry: Default::default(), - harnesses: Default::default(), - default_harness: crate::acp_child::BUILTIN_HARNESS.into(), - parent_id: None, - parent_name: None, - }, - 2, - ); + let manager = Subagents::new(ChildConfig { + root: root.to_path_buf(), + additional_directories: Vec::new(), + model: "test".into(), + provider: Default::default(), + reasoning_effort: None, + openrouter_api_key: None, + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: false, + mcp_config: None, + credential_storage: Default::default(), + telemetry: Default::default(), + harnesses: Default::default(), + default_harness: crate::acp_child::BUILTIN_HARNESS.into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }); let state = Arc::new(AsyncMutex::new(State { name: "Scout".into(), status: SubagentStatus::Idle, @@ -2358,7 +2607,8 @@ mod steer_tests { child: Some(ChildSession::disconnected_for_test()), recovery: None, forking: None, - permit: Some(Arc::clone(&manager.capacity).try_acquire_owned().unwrap()), + permit: Some(manager.acquire_permit().unwrap()), + cache_key: None, })); manager.sessions.lock().unwrap().insert( "source".into(), @@ -2378,26 +2628,26 @@ mod steer_tests { } #[test] - fn steer_schema_accepts_ids_and_child_prompts_only() { + fn prompt_accepts_an_id_or_a_subagent_value() { let (manager, _, _) = manager_with_disconnected_session(Path::new(".")); - let tool = SteerTool::new(manager); + let tool = PromptTool::new(manager); let validator = jsonschema::validator_for(&tool.spec.input_schema).unwrap(); for input in [ - json!({"id": "source", "prompt": "focus on tests"}), - json!({"id": "source", "prompt": [{"type": "text", "text": "focus on tests"}]}), + json!({"subagent": "source", "prompt": "focus on tests"}), + json!({"subagent": {"id": "source", "output": null, "generation": 1}, "prompt": "go"}), + json!({"subagent": "source", "prompt": [{"type": "text", "text": "focus on tests"}]}), ] { - assert!(validator.is_valid(&input)); - assert!(serde_json::from_value::(input).is_ok()); + assert!(validator.is_valid(&input), "{input}"); + let parsed = serde_json::from_value::(input).unwrap(); + assert_eq!(parsed.subagent.id(), "source"); } for input in [ - json!({"prompt": "missing ID"}), - json!({"id": "source"}), - json!({"id": 1, "prompt": "work"}), - json!({"id": "source", "prompt": "work", "generation": 1}), - json!({"id": "source", "prompt": null}), + json!({"prompt": "missing subagent"}), + json!({"subagent": "source"}), + json!({"subagent": 1, "prompt": "work"}), + json!({"subagent": "source", "prompt": null}), ] { - assert!(!validator.is_valid(&input)); - assert!(serde_json::from_value::(input).is_err()); + assert!(!validator.is_valid(&input), "{input}"); } } @@ -2428,7 +2678,7 @@ mod steer_tests { }, )])) .unwrap(); - let manager = Subagents::new(config, 2); + let manager = Subagents::new(config); let turn_manager = manager.clone(); let turn = tokio::spawn(async move { turn_manager @@ -2465,15 +2715,38 @@ mod steer_tests { locked.generation, locked.handle_generation, locked.output.clone(), - locked.permit.as_ref().unwrap().num_permits(), + locked.permit.as_ref().unwrap()._local.num_permits(), ) }; let capacity = manager.capacity.available_permits(); - let receipt = manager.steer(id, "original turn".into()).await.unwrap(); - assert_eq!(receipt, json!({"messageId": "injected-1"})); + let delivery = { + let manager = manager.clone(); + let id = id.clone(); + tokio::spawn(async move { + manager + .deliver( + &id, + "original turn".into(), + TurnCancellation::default(), + None, + ) + .await + }) + }; + loop { + let log = std::fs::read_to_string(&request_log).unwrap_or_default(); + if log + .lines() + .filter_map(|line| serde_json::from_str::(line).ok()) + .any(|request| request["method"] == "session/inject") + { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } assert!( - !turn.is_finished(), - "acceptance must not finish the gated turn" + !turn.is_finished() && !delivery.is_finished(), + "an injected prompt must not finish the gated turn" ); { let locked = state.lock().await; @@ -2481,7 +2754,10 @@ mod steer_tests { assert_eq!(locked.generation, before.0); assert_eq!(locked.handle_generation, before.1); assert_eq!(locked.output, before.2); - assert_eq!(locked.permit.as_ref().unwrap().num_permits(), before.3); + assert_eq!( + locked.permit.as_ref().unwrap()._local.num_permits(), + before.3 + ); assert_eq!(manager.capacity.available_permits(), capacity); assert!(locked.outcome.is_none()); assert!(locked.generation_finished_at_unix_ms.is_none()); @@ -2491,6 +2767,9 @@ mod steer_tests { assert_eq!(completed.id, *id); assert_eq!(completed.output, json!("original turn")); assert_eq!(completed.generation, before.0); + let delivered = delivery.await.unwrap().unwrap(); + assert_eq!(delivered.generation, completed.generation); + assert_eq!(delivered.output, completed.output); { let locked = state.lock().await; assert_eq!(locked.status, SubagentStatus::Idle); @@ -2500,12 +2779,7 @@ mod steer_tests { assert!(locked.permit.is_some()); } let continued = manager - .prompt( - completed, - "next turn".into(), - TurnCancellation::default(), - None, - ) + .deliver(id, "next turn".into(), TurnCancellation::default(), None) .await .unwrap(); assert_eq!(continued.id, *id); @@ -2556,6 +2830,146 @@ mod steer_tests { .unwrap(); } + #[tokio::test] + async fn failed_first_turn_keeps_a_live_subagent_promptable() { + use std::time::Duration; + + tokio::time::timeout(Duration::from_secs(10), async { + let root = tempfile::tempdir().unwrap(); + let mut config = manager_with_disconnected_session(root.path()) + .0 + .child_config(); + config.default_harness = "acp.mock".into(); + config.harnesses = + crate::acp_child::AcpHarnesses::new(std::collections::BTreeMap::from([( + "mock".into(), + crate::acp_child::AcpHarnessProfile { + command: "python3".into(), + args: vec![format!( + "{}/fixtures/mock-acp-v2.py", + env!("CARGO_MANIFEST_DIR") + )], + permissions: Default::default(), + }, + )])) + .unwrap(); + let manager = Subagents::new(config); + let error = manager + .create( + "MOCK_TURN_ERROR".into(), + CreateOptions { + name: Some("worker".into()), + ..Default::default() + }, + 0, + TurnCancellation::default(), + None, + ) + .await + .unwrap_err() + .to_string(); + let listing = manager.list(&TurnCancellation::default()).await.unwrap(); + assert_eq!(listing.len(), 1); + let id = listing[0].id.clone(); + assert_eq!(listing[0].status, SubagentStatus::Idle); + assert!(error.contains(&format!("{id:?} (worker)")), "{error}"); + assert!(error.contains("nested agent turn failed"), "{error}"); + assert!( + error.contains(&format!("prompt({{subagent: {id:?}")), + "{error}" + ); + + let continued = manager + .deliver(&id, "continue".into(), TurnCancellation::default(), None) + .await + .unwrap(); + assert_eq!(continued.id, id); + assert_eq!(continued.output, json!("continue")); + assert_eq!(continued.generation, 2); + manager + .close(&id, &TurnCancellation::default()) + .await + .unwrap(); + }) + .await + .unwrap(); + } + + #[tokio::test] + async fn prompt_to_a_working_subagent_without_steering_runs_after_its_turn() { + use std::time::Duration; + + tokio::time::timeout(Duration::from_secs(10), async { + let root = tempfile::tempdir().unwrap(); + let release = root.path().join("release"); + let request_log = root.path().join("requests"); + let mut config = manager_with_disconnected_session(root.path()) + .0 + .child_config(); + config.default_harness = "acp.mock".into(); + config.harnesses = + crate::acp_child::AcpHarnesses::new(std::collections::BTreeMap::from([( + "mock".into(), + crate::acp_child::AcpHarnessProfile { + command: "python3".into(), + args: vec![ + format!("{}/fixtures/mock-acp-v2.py", env!("CARGO_MANIFEST_DIR")), + format!("--prompt-release={}", release.display()), + format!("--request-log={}", request_log.display()), + ], + permissions: Default::default(), + }, + )])) + .unwrap(); + let manager = Subagents::new(config); + let turn = { + let manager = manager.clone(); + tokio::spawn(async move { + manager + .create( + "first".into(), + CreateOptions::default(), + 0, + TurnCancellation::default(), + None, + ) + .await + }) + }; + while !std::fs::read_to_string(&request_log) + .unwrap_or_default() + .contains("session/prompt") + { + tokio::time::sleep(Duration::from_millis(10)).await; + } + let id = manager.list(&TurnCancellation::default()).await.unwrap()[0] + .id + .clone(); + let delivery = { + let manager = manager.clone(); + let id = id.clone(); + tokio::spawn(async move { + manager + .deliver(&id, "second".into(), TurnCancellation::default(), None) + .await + }) + }; + tokio::time::sleep(Duration::from_millis(100)).await; + assert!(!delivery.is_finished()); + std::fs::write(&release, b"release").unwrap(); + assert_eq!(turn.await.unwrap().unwrap().output, json!("first")); + let delivered = delivery.await.unwrap().unwrap(); + assert_eq!(delivered.output, json!("second")); + assert_eq!(delivered.generation, 2); + manager + .close(&id, &TurnCancellation::default()) + .await + .unwrap(); + }) + .await + .unwrap(); + } + #[tokio::test] async fn steer_rejects_ineligible_sessions_without_changing_lifecycle() { let (manager, state, prior) = manager_with_disconnected_session(Path::new(".")); @@ -2572,7 +2986,7 @@ mod steer_tests { locked.forking = forking.map(str::to_owned); } let error = manager - .steer(&prior.id, "guidance".into()) + .steer_generation(&prior.id, None, "guidance".into()) .await .unwrap_err(); assert!(error.to_string().contains(expected), "{error}"); @@ -2587,7 +3001,7 @@ mod steer_tests { } assert!( manager - .steer("unknown", "guidance".into()) + .steer_generation("unknown", None, "guidance".into()) .await .unwrap_err() .to_string() diff --git a/src/tools/subagent/recovery.rs b/src/tools/subagent/recovery.rs index bb940c22..3a3a59c8 100644 --- a/src/tools/subagent/recovery.rs +++ b/src/tools/subagent/recovery.rs @@ -51,6 +51,7 @@ impl Subagents { recovery: Some(record.clone()), forking: None, permit: None, + cache_key: None, }; if sessions .insert( @@ -153,7 +154,6 @@ impl Subagents { record.harness ))); } - self.check_depth(record.depth.saturating_sub(1))?; let model = record .model .as_deref() @@ -161,16 +161,14 @@ impl Subagents { .transpose() .map_err(ChildError::Failed)?; // Capacity is private to this attempt until the child is installed. - let permit = self - .capacity - .clone() - .try_acquire_owned() - .map_err(|_| ChildError::Failed("maximum live subagent count reached".into()))?; + let permit = self.acquire_permit()?; locked.status = SubagentStatus::Starting; let config = self .config .clone() .with_root(record.root.clone()) + .with_tree_slots(super::tree_slot_directory(&self.tree_slots)?.clone()) + .with_cache_lineage(locked.cache_key.clone()) .with_parent_context(id.into(), locked.name.clone()); drop(locked); let manager = self.clone(); diff --git a/src/tools/subagent/tests.rs b/src/tools/subagent/tests.rs index d8f12d6f..729790e5 100644 --- a/src/tools/subagent/tests.rs +++ b/src/tools/subagent/tests.rs @@ -54,6 +54,7 @@ impl Subagents { recovery: None, forking: None, permit: Some(self.reserve().unwrap()), + cache_key: None, }, ) .unwrap(); @@ -308,27 +309,26 @@ fn text_only_values_keep_the_existing_json_shape() { fn manager_with_disconnected_session( root: &Path, ) -> (Subagents, Arc>, SubagentValue) { - let manager = Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: root.to_path_buf(), - model: "test".into(), - provider: Default::default(), - reasoning_effort: None, - openrouter_api_key: None, - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: false, - mcp_config: None, - credential_storage: Default::default(), - telemetry: Default::default(), - harnesses: Default::default(), - default_harness: crate::acp_child::BUILTIN_HARNESS.into(), - parent_id: None, - parent_name: None, - }, - 2, - ); + let manager = Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: root.to_path_buf(), + model: "test".into(), + provider: Default::default(), + reasoning_effort: None, + openrouter_api_key: None, + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: false, + mcp_config: None, + credential_storage: Default::default(), + telemetry: Default::default(), + harnesses: Default::default(), + default_harness: crate::acp_child::BUILTIN_HARNESS.into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }); let state = Arc::new(AsyncMutex::new(State { name: "Scout".into(), status: SubagentStatus::Idle, @@ -349,7 +349,8 @@ fn manager_with_disconnected_session( child: Some(ChildSession::disconnected_for_test()), recovery: None, forking: None, - permit: Some(Arc::clone(&manager.capacity).try_acquire_owned().unwrap()), + permit: Some(manager.acquire_permit().unwrap()), + cache_key: None, })); manager.sessions.lock().unwrap().insert( "source".into(), @@ -381,27 +382,26 @@ async fn close_does_not_block_listings_or_allow_stale_reuse() { }, )])) .unwrap(); - let manager = Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: root.path().to_path_buf(), - model: "unused".into(), - provider: Default::default(), - reasoning_effort: None, - openrouter_api_key: None, - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: false, - mcp_config: None, - credential_storage: Default::default(), - telemetry: Default::default(), - harnesses, - default_harness: "acp.generic".into(), - parent_id: None, - parent_name: None, - }, - 2, - ); + let manager = Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: root.path().to_path_buf(), + model: "unused".into(), + provider: Default::default(), + reasoning_effort: None, + openrouter_api_key: None, + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: false, + mcp_config: None, + credential_storage: Default::default(), + telemetry: Default::default(), + harnesses, + default_harness: "acp.generic".into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }); let handle = manager .create( "base".into(), @@ -546,27 +546,26 @@ fn manager_with_generic_harness(root: &Path, args: Vec) -> Subagents { }, )])) .unwrap(); - Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: root.to_path_buf(), - model: "unused".into(), - provider: Default::default(), - reasoning_effort: None, - openrouter_api_key: None, - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: false, - mcp_config: None, - credential_storage: Default::default(), - telemetry: Default::default(), - harnesses, - default_harness: "acp.generic".into(), - parent_id: None, - parent_name: None, - }, - 2, - ) + Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: root.to_path_buf(), + model: "unused".into(), + provider: Default::default(), + reasoning_effort: None, + openrouter_api_key: None, + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: false, + mcp_config: None, + credential_storage: Default::default(), + telemetry: Default::default(), + harnesses, + default_harness: "acp.generic".into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }) } #[test] @@ -1036,7 +1035,7 @@ mod lifecycle_events { let mut config = base.child_config(); config.parent_id = Some("s-parent".into()); config.parent_name = Some("偵察 🦀".into()); - let (manager, events) = observe_events(Subagents::new(config, 2)); + let (manager, events) = observe_events(Subagents::new(config)); manager.insert_starting_for_test().await; @@ -1101,8 +1100,10 @@ mod lifecycle_events { None, ) .await - .unwrap_err(); - assert_eq!(error.to_string(), "nested agent refused the prompt"); + .unwrap_err() + .to_string(); + assert!(error.contains("nested agent refused the prompt"), "{error}"); + assert!(error.contains(&format!("{:?}", handle.id)), "{error}"); manager .close(&handle.id, &TurnCancellation::default()) .await @@ -1776,27 +1777,26 @@ async fn reusable_prompt_failure_remains_failed_idle_and_can_be_retried() { }, )])) .unwrap(); - let manager = Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: root.path().to_path_buf(), - model: "unused".into(), - provider: Default::default(), - reasoning_effort: None, - openrouter_api_key: None, - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: false, - mcp_config: None, - credential_storage: Default::default(), - telemetry: Default::default(), - harnesses, - default_harness: "acp.generic".into(), - parent_id: None, - parent_name: None, - }, - 2, - ); + let manager = Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: root.path().to_path_buf(), + model: "unused".into(), + provider: Default::default(), + reasoning_effort: None, + openrouter_api_key: None, + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: false, + mcp_config: None, + credential_storage: Default::default(), + telemetry: Default::default(), + harnesses, + default_harness: "acp.generic".into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }); let handle = manager .create( "base".into(), @@ -1816,8 +1816,10 @@ async fn reusable_prompt_failure_remains_failed_idle_and_can_be_retried() { None, ) .await - .unwrap_err(); - assert_eq!(error.to_string(), "nested agent refused the prompt"); + .unwrap_err() + .to_string(); + assert!(error.contains("nested agent refused the prompt"), "{error}"); + assert!(error.contains(&format!("{:?}", handle.id)), "{error}"); let state = manager .lookup(&handle) .expect("live child remains reusable"); @@ -1874,27 +1876,26 @@ async fn listing_includes_named_starting_and_idle_subagents() { }, )])) .unwrap(); - let manager = Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: root.path().to_path_buf(), - model: "unused".into(), - provider: Default::default(), - reasoning_effort: None, - openrouter_api_key: None, - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: false, - mcp_config: None, - credential_storage: Default::default(), - telemetry: Default::default(), - harnesses, - default_harness: "acp.generic".into(), - parent_id: None, - parent_name: None, - }, - 2, - ); + let manager = Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: root.path().to_path_buf(), + model: "unused".into(), + provider: Default::default(), + reasoning_effort: None, + openrouter_api_key: None, + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: false, + mcp_config: None, + credential_storage: Default::default(), + telemetry: Default::default(), + harnesses, + default_harness: "acp.generic".into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }); let create_manager = manager.clone(); let create = tokio::spawn(async move { create_manager @@ -2027,27 +2028,26 @@ async fn generic_harness_without_native_fork_returns_unsupported() { }, )])) .unwrap(); - let manager = Subagents::new( - ChildConfig { - additional_directories: Vec::new(), - root: root.path().to_path_buf(), - model: "unused".into(), - provider: Default::default(), - reasoning_effort: None, - openrouter_api_key: None, - configured_mcp_config: None, - configured_mcp_config_inherited: false, - legacy_mcp_config: false, - mcp_config: None, - credential_storage: Default::default(), - telemetry: Default::default(), - harnesses, - default_harness: "acp.generic".into(), - parent_id: None, - parent_name: None, - }, - 2, - ); + let manager = Subagents::new(ChildConfig { + additional_directories: Vec::new(), + root: root.path().to_path_buf(), + model: "unused".into(), + provider: Default::default(), + reasoning_effort: None, + openrouter_api_key: None, + configured_mcp_config: None, + configured_mcp_config_inherited: false, + legacy_mcp_config: false, + mcp_config: None, + credential_storage: Default::default(), + telemetry: Default::default(), + harnesses, + default_harness: "acp.generic".into(), + parent_id: None, + parent_name: None, + tree_slots: None, + prompt_cache_key: None, + }); let prior = manager .create( "base".into(), diff --git a/src/transcript.rs b/src/transcript.rs index e845af77..c694dea2 100644 --- a/src/transcript.rs +++ b/src/transcript.rs @@ -65,12 +65,11 @@ pub fn repair_unanswered_tool_calls(transcript: &mut Vec) -> Vec { synthesized } -/// Removes provider continuation state that is bound to the source session. +/// Rebinds provider continuation state to a fork's new session identity. /// -/// A fork has a new durable identity, so replaying opaque continuation state from -/// the source would violate the provider's session binding. Generated assistant -/// images cannot be encoded without that continuation and are omitted as well. -pub(crate) fn sanitize_forked_transcript(transcript: &mut [Item]) { +/// Encrypted reasoning stays replayable under the same account binding, which the +/// provider still checks, so a fork keeps the source's request prefix intact. +pub(crate) fn rebind_forked_transcript(transcript: &mut [Item], session_id: &str) { for item in transcript { let assistant = item.kind == ItemKind::Assistant; item.parts.retain_mut(|part| { @@ -84,9 +83,19 @@ pub(crate) fn sanitize_forked_transcript(transcript: &mut [Item]) { let Some(metadata) = metadata else { return true; }; - metadata.remove(OPENAI_RESPONSES_CONTINUATION); metadata.remove(OPENAI_SUBSCRIPTION_CONTINUATION); - !(assistant && is_media) + let rebound = match metadata.get_mut(OPENAI_RESPONSES_CONTINUATION) { + Some(serde_json::Value::Object(continuation)) => { + continuation.insert("session_id".into(), session_id.into()); + true + } + Some(_) => { + metadata.remove(OPENAI_RESPONSES_CONTINUATION); + false + } + None => false, + }; + !(assistant && is_media && !rebound) }); } } @@ -202,14 +211,25 @@ mod tests { Item::new(ItemKind::User, vec![user_media.clone()]), ]; - sanitize_forked_transcript(&mut transcript); + rebind_forked_transcript(&mut transcript, "s-fork"); - assert_eq!(transcript[0].parts.len(), 1); + assert_eq!(transcript[0].parts.len(), 2); let Part::ToolCall(call) = &transcript[0].parts[0] else { panic!("expected tool call"); }; - assert_eq!(call.metadata.len(), 1); assert_eq!(call.metadata["preserved"], true); + assert!(!call.metadata.contains_key(OPENAI_SUBSCRIPTION_CONTINUATION)); + assert_eq!( + call.metadata[OPENAI_RESPONSES_CONTINUATION]["session_id"], + "s-fork" + ); + let Part::Media(media) = &transcript[0].parts[1] else { + panic!("expected continued media"); + }; + assert_eq!( + media.metadata[OPENAI_RESPONSES_CONTINUATION]["session_id"], + "s-fork" + ); assert_eq!(transcript[1].parts, vec![user_media]); } From b6c02636d3d812440dd4fd52bcbe3bd3455d6286 Mon Sep 17 00:00:00 2001 From: daniel Date: Thu, 1 Oct 2026 11:09:30 +0100 Subject: [PATCH 2/3] fix(subagents): address review of delivery, transient forks and diagnostics - Admit idle deliveries atomically: a session another caller is using, starting, forking or has advanced hands the prompt back unsent, and `prompt` re-checks state instead of failing. - Record each finished generation's outcome and output so callers whose message was injected receive that generation's result, not a later one. - Wake delivery waiters on child-exit retirement and fork-reservation release. - Fork the current conversation from in-memory `run-N` sessions: transient runs record their transcript, including compaction rewrites. - Bound the foreground model-catalog lookup as a whole, so it does not wait out background context discovery. - Report a fatal cause only when it was recorded during the failing turn. - Accept `{ id }` for `prompt`'s subagent, matching `close`. --- docs/user/subagents-and-acp-harnesses.md | 4 +- src/compaction.rs | 20 +- src/fatal.rs | 49 ++- src/provider/chatgpt.rs | 26 +- src/provider/chatgpt/catalog_tests.rs | 18 ++ src/runtime.rs | 6 +- src/session.rs | 93 +++++- src/tools/subagent.rs | 389 +++++++++++++++++++++-- src/tools/subagent/recovery.rs | 1 + src/tools/subagent/tests.rs | 2 + 10 files changed, 552 insertions(+), 56 deletions(-) diff --git a/docs/user/subagents-and-acp-harnesses.md b/docs/user/subagents-and-acp-harnesses.md index 45161b45..fdfae727 100644 --- a/docs/user/subagents-and-acp-harnesses.md +++ b/docs/user/subagents-and-acp-harnesses.md @@ -90,7 +90,7 @@ return { main: second.output, alternative: branch.output } Each successful turn returns a session value with `id`, `name`, `output`, and `generation`. `subagent` creates an ID at generation 1. `prompt` keeps that ID and name while incrementing its generation. `fork` creates a different ID and uses its own preferred or fallback name; its generation is one greater than the supplied source value, and it does not advance the source session. Close a session with either `close(value)` or `close({ id: value.id })`; the latter is useful when only an ID is available. Closing an unknown ID fails with `unknown subagent session`. Kit sends ACP `session/close` when the harness advertises it. Explicit `close` also sends `session/delete` when advertised, removing the discarded branch’s persistent history after closing it. Delete failures are reported rather than silently ignored. Process shutdown and internal cleanup do not delete persistent history; this preserves completed child sessions for restart recovery. A standalone process without that capability is terminated when its handle is dropped. If native-fork siblings share a process and the harness cannot close one logical session, `close` fails rather than claiming success or disrupting the siblings. -`prompt` accepts the subagent's ID or any value returned for it, and always targets the session's current state. `fork` copies a completed turn, so pass it the latest completed value; an older value fails with `stale subagent generation N; current generation is M`. Prompt and fork calls on an individual ACP session are serialized, while separate forked sessions can be prompted concurrently. +`prompt` accepts `{ id }`, the subagent's ID string, or any value returned for it, and always targets the session's current state. `fork` copies a completed turn, so pass it the latest completed value; an older value fails with `stale subagent generation N; current generation is M`. Prompt and fork calls on an individual ACP session are serialized, while separate forked sessions can be prompted concurrently. The optional `name` argument is preferred on `subagent` and `fork`; `prompt` has no naming input and preserves the session name. The optional `harness`, `model`, and `cwd` arguments belong only on `subagent`. `harness` overrides the user's configured harness preference. `model` selects an exact model value ID advertised by that harness through its ACP session configuration, or a model alias configured for that harness. `cwd` selects the new subagent's working directory; relative paths resolve from Kit's working directory, and missing paths or non-directories fail before startup. Omit an argument to retain its configured default. `prompt` and `fork` retain the original session's harness, model, and working directory. An explicit model fails before the first prompt if the harness does not advertise a selectable `model` option or rejects the value. @@ -106,7 +106,7 @@ An `output_schema` applies to a new turn only, so a prompt with one always waits ```text return prompt({ - subagent: "s-…", + subagent: { id: "s-…" }, prompt: "Keep the change limited to the parser; do not modify the public API." }) ``` diff --git a/src/compaction.rs b/src/compaction.rs index f2013b5d..e868f10f 100644 --- a/src/compaction.rs +++ b/src/compaction.rs @@ -395,7 +395,11 @@ where .with_strategy(SummarizeForContinuation::default()), ) .with_backend(backend); - Ok(AutomaticCompactor { inner, persistence }) + Ok(AutomaticCompactor { + inner, + persistence, + mirror: None, + }) } struct KitCompactionBackend { @@ -665,6 +669,15 @@ fn user_message_from_marker( pub struct AutomaticCompactor { inner: StrategyCompactor, persistence: Option, + mirror: Option, +} + +impl AutomaticCompactor { + /// Applies each replacement to an in-memory transcript as well. + pub(crate) fn mirroring(mut self, transcript: crate::session::TransientTranscript) -> Self { + self.mirror = Some(transcript); + self + } } #[async_trait] @@ -744,6 +757,9 @@ impl LoopMutator for AutomaticCompactor { // never the already-computed in-memory compaction. metadata.insert("persistence_error".into(), error.into()); } + if let Some(mirror) = &self.mirror { + mirror.replace(&compacted); + } metadata.insert( "replaced_items".into(), (before.saturating_sub(compacted.len()) as u64).into(), @@ -898,6 +914,7 @@ mod tests { ) .with_backend(FixedBackend), persistence: Some(opened.observer), + mirror: None, }; let mut transcript = opened.transcript; let marker = transcript.pop().unwrap(); @@ -956,6 +973,7 @@ mod tests { CompactionPipeline::new().with_strategy(EmptyStrategy), ), persistence: None, + mirror: None, }; let agent = Agent::builder() .model(NoModelTurn) diff --git a/src/fatal.rs b/src/fatal.rs index ab7c3cfe..ebeb7ddc 100644 --- a/src/fatal.rs +++ b/src/fatal.rs @@ -268,14 +268,19 @@ pub(crate) fn record_loop_error( .map(Some) } -/// Summarizes the newest fatal record of `session_id` without prompt content. -pub(crate) fn latest_cause(session_id: &str) -> Option { +/// Summarizes the newest fatal record of `session_id` written at or after +/// `since_ms`, without prompt content. +pub(crate) fn latest_cause(session_id: &str, since_ms: u64) -> Option { crate::session::validate_id(session_id).ok()?; let home = std::env::var_os("HOME").filter(|home| !home.is_empty())?; - latest_cause_in(&PathBuf::from(home).join(".kit/errors"), session_id) + latest_cause_in( + &PathBuf::from(home).join(".kit/errors"), + session_id, + since_ms, + ) } -fn latest_cause_in(base: &Path, session_id: &str) -> Option { +fn latest_cause_in(base: &Path, session_id: &str, since_ms: u64) -> Option { let directory = base.join(session_id); let path = fs::read_dir(&directory) .ok()? @@ -292,6 +297,9 @@ fn latest_cause_in(base: &Path, session_id: &str) -> Option { .and_then(|millis| millis.parse::().ok()) })?; let record: FatalRecord = serde_json::from_slice(&fs::read(&path).ok()?).ok()?; + if record.occurred_at_ms < since_ms { + return None; + } let mut cause = format!("{} ({})", record.message, record.code); if let Some(failure) = record.failure { cause.push_str(&format!("; reason {:?}", failure.reason)); @@ -1036,6 +1044,39 @@ mod tests { ); } + #[test] + fn latest_cause_ignores_records_older_than_the_failing_turn() { + use agentkit_loop::{ProviderFailure, ProviderFailureReason, ProviderRoute}; + + let root = tempfile::tempdir().unwrap(); + let failure = ProviderFailure { + route: ProviderRoute::OpenAiResponses, + reason: ProviderFailureReason::RetryExhausted, + last_attempt_reason: None, + upstream: Default::default(), + accounting: Default::default(), + }; + super::write_record( + root.path(), + "session-1", + Surface::Acp, + "provider", + "provider_error", + "provider request failed", + None, + Some(&failure), + ) + .unwrap(); + let cause = super::latest_cause_in(root.path(), "session-1", 0).unwrap(); + assert!(cause.contains("RetryExhausted"), "{cause}"); + let later = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() as u64 + + 60_000; + assert!(super::latest_cause_in(root.path(), "session-1", later).is_none()); + } + #[test] fn typed_provider_failures_preserve_fatal_and_cancellation_behavior() { use agentkit_loop::{ProviderFailure, ProviderFailureReason, ProviderRoute}; diff --git a/src/provider/chatgpt.rs b/src/provider/chatgpt.rs index 5da46fe2..879de136 100644 --- a/src/provider/chatgpt.rs +++ b/src/provider/chatgpt.rs @@ -224,23 +224,23 @@ impl OpenAiSubscriptionAdapter { .await } + /// Foreground lookup: bounded as a whole, including waiting on a background + /// discovery that is already initializing the shared cache. pub(crate) async fn model_catalog(&self) -> Result, LoopError> { - let credentials = tokio::time::timeout( - MODEL_CATALOG_AUTH_TIMEOUT, - load_credentials( + tokio::time::timeout(MODEL_CATALOG_AUTH_TIMEOUT, async { + let credentials = load_credentials( self.config.credential_storage.clone(), MODEL_CATALOG_AUTH_TIMEOUT, - ), - ) + ) + .await?; + let binding = credentials + .binding() + .map_err(|error| LoopError::Provider(error.to_string()))?; + self.catalog_with_credentials(&credentials, &binding, MODEL_CATALOG_AUTH_TIMEOUT) + .await + }) .await - .map_err(|_| { - LoopError::Provider("OpenAI model catalog credential load timed out".into()) - })??; - let binding = credentials - .binding() - .map_err(|error| LoopError::Provider(error.to_string()))?; - self.catalog_with_credentials(&credentials, &binding, MODEL_CATALOG_AUTH_TIMEOUT) - .await + .map_err(|_| LoopError::Provider("OpenAI model catalog lookup timed out".into()))? } } diff --git a/src/provider/chatgpt/catalog_tests.rs b/src/provider/chatgpt/catalog_tests.rs index 094d7d4c..90a261cc 100644 --- a/src/provider/chatgpt/catalog_tests.rs +++ b/src/provider/chatgpt/catalog_tests.rs @@ -157,3 +157,21 @@ async fn context_discovery_stops_when_credentials_change() { respond(request(&listener).await, "200 OK", CATALOG).await; discovered(&next).await; } + +#[tokio::test] +async fn foreground_catalog_lookup_does_not_wait_out_background_discovery() { + let (adapter, listener, _directory) = adapter().await; + let session = adapter + .start_session(SessionConfig::new("session")) + .await + .unwrap(); + // Background discovery now owns cache initialization and its request stalls. + let _stalled = request(&listener).await; + let started = std::time::Instant::now(); + let lookup = tokio::time::timeout(MODEL_CATALOG_BACKGROUND_TIMEOUT, adapter.model_catalog()) + .await + .expect("foreground lookup waited for background discovery"); + assert!(lookup.is_err()); + assert!(started.elapsed() < MODEL_CATALOG_AUTH_TIMEOUT * 2); + drop(session); +} diff --git a/src/runtime.rs b/src/runtime.rs index 5a5696f2..f08f51ce 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -1519,16 +1519,19 @@ impl Runtime { .await .map_err(LoopError::InvalidState)?; let skills = self.fresh_skills(); + let recorded = crate::session::TransientTranscript::new(transcript.clone()); let compactor = crate::compaction::automatic( self.adapter.clone(), self.agentkit_telemetry(), None, format!("compaction-{session}"), ) - .map_err(LoopError::InvalidState)?; + .map_err(LoopError::InvalidState)? + .mirroring(recorded.clone()); let subagents = self .subagents .fresh() + .with_transient_parent(recorded.clone()) .with_prompt_cache_key(crate::tools::subagent::prompt_cache_key(&session)); let builder = Agent::builder() .model(self.adapter.clone()) @@ -1541,6 +1544,7 @@ impl Runtime { )) .task_manager(background_task_manager()) .mutator(compactor) + .transcript_observer(recorded) .transcript(transcript) .input(vec![Item::text(ItemKind::User, prompt)]); let builder = builder.cancellation(controller.handle()); diff --git a/src/session.rs b/src/session.rs index 68d6001e..81725388 100644 --- a/src/session.rs +++ b/src/session.rs @@ -275,8 +275,31 @@ pub fn clone_inherited(root: &Path, source: &str, destination: &str) -> Result<( validate_id(source)?; let authority = select_authority(&directory, &canonical_workspace(root), source)? .ok_or_else(|| format!("session {source:?} does not exist"))?; - let reasoning_effort = authority.reasoning_effort; - let mut transcript = authority.items; + inherit_into( + root, + &directory, + destination, + authority.items, + authority.reasoning_effort, + ) +} + +/// Like [`clone_inherited`], for a parent whose conversation lives only in memory. +pub(crate) fn clone_inherited_items( + root: &Path, + transcript: Vec, + destination: &str, +) -> Result<(), String> { + inherit_into(root, &default_directory()?, destination, transcript, None) +} + +fn inherit_into( + root: &Path, + directory: &Path, + destination: &str, + mut transcript: Vec, + reasoning_effort: Option>, +) -> Result<(), String> { while transcript .last() .is_some_and(|item| item.kind == ItemKind::Assistant) @@ -293,7 +316,7 @@ pub fn clone_inherited(root: &Path, source: &str, destination: &str) -> Result<( } let opened = open_with_initial_timestamps_in( root, - &directory, + directory, destination, false, false, @@ -756,6 +779,35 @@ impl SessionObserver { } } +/// In-memory transcript of a session without durable storage, kept so its +/// conversation can still be forked. +#[derive(Clone)] +pub(crate) struct TransientTranscript(Arc>>); + +impl TransientTranscript { + pub(crate) fn new(transcript: Vec) -> Self { + Self(Arc::new(Mutex::new(transcript))) + } + + pub(crate) fn snapshot(&self) -> Vec { + self.0.lock().map(|items| items.clone()).unwrap_or_default() + } + + pub(crate) fn replace(&self, transcript: &[Item]) { + if let Ok(mut items) = self.0.lock() { + transcript.clone_into(&mut items); + } + } +} + +impl TranscriptObserver for TransientTranscript { + fn on_transcript_event(&self, event: TranscriptEvent<'_>) { + if let Ok(mut items) = self.0.lock() { + items.push(event.item.clone()); + } + } +} + impl TranscriptObserver for SessionObserver { fn on_transcript_event(&self, event: TranscriptEvent<'_>) { // Persistence must not prevent the loop from committing its in-memory @@ -2740,6 +2792,41 @@ mod tests { use agentkit_core::{ItemKind, MetadataMap, Part, ReasoningPart}; use serde_json::json; + #[test] + fn transient_transcripts_follow_the_conversation_and_can_be_forked() { + let root = tempfile::tempdir().unwrap(); + let directory = tempfile::tempdir().unwrap(); + let system = Item::text(ItemKind::System, "system"); + let transcript = TransientTranscript::new(vec![system.clone()]); + let session = agentkit_core::SessionId::new("run-1"); + for item in [ + Item::text(ItemKind::User, "question"), + Item::text(ItemKind::Assistant, "partial"), + ] { + transcript.on_transcript_event(TranscriptEvent { + session_id: &session, + item: &item, + }); + } + assert_eq!(transcript.snapshot().len(), 3); + + let destination = new_id(); + inherit_into( + root.path(), + directory.path(), + &destination, + transcript.snapshot(), + None, + ) + .unwrap(); + let forked = load_in(root.path(), directory.path(), &destination).unwrap(); + let kinds = forked.iter().map(|item| item.kind).collect::>(); + assert_eq!(kinds, vec![ItemKind::System, ItemKind::User]); + + transcript.replace(&[system, Item::text(ItemKind::User, "summary")]); + assert_eq!(transcript.snapshot().len(), 2); + } + #[test] fn reasoning_effort_absent_changes_reset_and_fork() { let root = tempfile::tempdir().unwrap(); diff --git a/src/tools/subagent.rs b/src/tools/subagent.rs index a172fc78..b4ca54ce 100644 --- a/src/tools/subagent.rs +++ b/src/tools/subagent.rs @@ -164,15 +164,17 @@ async fn wait_or_cancel( } } -fn failed_turn(id: &str, name: &str, error: ChildError) -> ChildError { +/// `started_at_ms` is when the failing turn began; older fatal records belong +/// to other turns. +fn failed_turn(id: &str, name: &str, started_at_ms: u64, error: ChildError) -> ChildError { let ChildError::Failed(message) = error else { return error; }; - let cause = crate::fatal::latest_cause(id) + let cause = crate::fatal::latest_cause(id, started_at_ms) .map(|cause| format!(" Cause: {cause}.")) .unwrap_or_default(); ChildError::Failed(format!( - "subagent {id:?} ({name}) failed during its turn: {message}.{cause} It is idle and keeps its conversation and edits; continue it with prompt({{subagent: {id:?}, prompt}})." + "subagent {id:?} ({name}) failed during its turn: {message}.{cause} It is idle and keeps its conversation and edits; continue it with prompt({{subagent: {{id: {id:?}}}, prompt}})." )) } @@ -207,6 +209,8 @@ pub struct Subagents { observer: Option, /// Signalled on every lifecycle event so deliveries can wait for a turn to end. changed: Arc, + /// The parent's conversation when it has no durable session to fork from. + transient_parent: Option, } struct SessionEntry { @@ -237,6 +241,56 @@ struct State { permit: Option, /// Prompt cache key the child process runs under; `None` uses its own id. cache_key: Option, + /// Recent finished generations, newest last, for callers waiting on one. + finished: std::collections::VecDeque, +} + +/// What one generation produced, kept for callers that injected a message into +/// it: later generations replace the session's current output and outcome. +struct FinishedTurn { + generation: u64, + started_at_ms: u64, + outcome: GenerationOutcome, + output: Value, + updates: Option, +} + +const FINISHED_TURNS: usize = 8; + +/// Whether another caller is using, starting or forking the session, or has +/// advanced it past `prior`. +fn busy(state: &State, prior: &SubagentValue) -> bool { + matches!( + state.status, + SubagentStatus::Working | SubagentStatus::Starting + ) || state.forking.is_some() + || prior.generation != state.handle_generation +} + +/// A session claimed for a new turn that has not been submitted yet. +struct Admitted { + state: Arc>, + child: ChildSession, + name: String, + generation: u64, +} + +impl State { + fn record_finished(&mut self) { + let Some(outcome) = self.outcome else { + return; + }; + self.finished.push_back(FinishedTurn { + generation: self.generation, + started_at_ms: self.generation_started_at_unix_ms, + outcome, + output: self.output.clone(), + updates: self.updates.clone(), + }); + while self.finished.len() > FINISHED_TURNS { + self.finished.pop_front(); + } + } } impl State { @@ -392,6 +446,7 @@ impl Subagents { observer: None, transcripts: Arc::default(), changed: Arc::default(), + transient_parent: None, event_sink: Arc::new(|event| { events::emit(event); Ok(()) @@ -399,6 +454,14 @@ impl Subagents { } } + pub(crate) fn with_transient_parent( + mut self, + transcript: session::TransientTranscript, + ) -> Self { + self.transient_parent = Some(transcript); + self + } + pub(crate) fn with_prompt_cache_key(mut self, key: String) -> Self { self.config.prompt_cache_key = Some(key); self @@ -558,6 +621,7 @@ impl Subagents { forking: None, permit: Some(permit), cache_key: cache_key.clone(), + finished: Default::default(), }, )?; let prompt = match (kit, inherit.is_some()) { @@ -569,8 +633,15 @@ impl Subagents { if let Some(source) = inherit { let transcript_root = self.config.root.clone(); let branch_id = id.clone(); - let cloned = tokio::task::spawn_blocking(move || { - session::clone_inherited(&transcript_root, &source, &branch_id) + let transient = self + .transient_parent + .as_ref() + .map(session::TransientTranscript::snapshot); + let cloned = tokio::task::spawn_blocking(move || match transient { + Some(transcript) => { + session::clone_inherited_items(&transcript_root, transcript, &branch_id) + } + None => session::clone_inherited(&transcript_root, &source, &branch_id), }) .await .map_err(|error| ChildError::Failed(format!("transcript clone task failed: {error}"))) @@ -655,6 +726,7 @@ impl Subagents { locked.generation_finished_at_unix_ms = Some(events::now_millis()); locked.output.clone_from(&output); locked.updates.clone_from(&updates); + locked.record_finished(); if let Err(error) = self.persist_state(&locked, session::ChildLifecycle::Idle) { drop(locked); return Err(self @@ -756,7 +828,14 @@ impl Subagents { generation: handle_generation, updates: None, }; - return self.prompt(prior, prompt, cancellation, contract).await; + // Another caller can claim the session between reading its state + // and admitting this prompt; nothing was submitted, so re-check. + if let Some(admitted) = self.admit(&prior, &prompt, &cancellation, true).await? { + return self + .run_admitted(prior.id, admitted, prompt, cancellation, contract) + .await; + } + continue; } // An injected message has no turn of its own to carry an output contract. if status == SubagentStatus::Working @@ -793,32 +872,39 @@ impl Subagents { changed.as_mut().enable(); { let locked = state.lock().await; - self.check_active(&locked)?; - let ended = locked.generation != generation - || matches!(locked.status, SubagentStatus::Idle) && locked.outcome.is_some(); - if ended { - if locked.generation == generation - && locked.outcome == Some(GenerationOutcome::Failed) - { - return Err(failed_turn( + if let Some(turn) = locked + .finished + .iter() + .find(|turn| turn.generation == generation) + { + return match turn.outcome { + GenerationOutcome::Success => Ok(SubagentValue { + id: id.to_owned(), + name: Some(locked.name.clone()), + output: turn.output.clone(), + generation, + updates: turn.updates.clone(), + }), + GenerationOutcome::Failed => Err(failed_turn( id, &locked.name, + turn.started_at_ms, ChildError::Failed("the turn that received this prompt failed".into()), - )); - } - return Ok(SubagentValue { - id: id.to_owned(), - name: Some(locked.name.clone()), - output: locked.output.clone(), - generation, - updates: locked.updates.clone(), - }); + )), + }; + } + self.check_active(&locked)?; + if locked.generation != generation { + return Err(ChildError::Failed(format!( + "subagent {id:?} finished generation {generation} without a recorded result" + ))); } } wait_or_cancel(changed, cancellation).await?; } } + #[cfg(test)] async fn prompt( &self, prior: SubagentValue, @@ -826,8 +912,30 @@ impl Subagents { cancellation: TurnCancellation, contract: Option<&OutputContract>, ) -> Result { - let state = self.lookup(&prior)?; - self.reconnect(&prior, &state, &cancellation).await?; + let Some(admitted) = self.admit(&prior, &prompt, &cancellation, false).await? else { + return Err(ChildError::Failed("subagent session is busy".into())); + }; + self.run_admitted(prior.id, admitted, prompt, cancellation, contract) + .await + } + + /// Claims an idle session for a new turn. With `yield_when_busy`, a session + /// that another caller is using or forking is reported as `None` instead of + /// an error, before anything is submitted. + async fn admit( + &self, + prior: &SubagentValue, + prompt: &ChildPrompt, + cancellation: &TurnCancellation, + yield_when_busy: bool, + ) -> Result, ChildError> { + let state = self.lookup(prior)?; + if let Err(error) = self.reconnect(prior, &state, cancellation).await { + if yield_when_busy && busy(&*state.lock().await, prior) { + return Ok(None); + } + return Err(error); + } // Lock-first ties were already allowed by the unbiased select. // The losing lock future is dropped before returning cancellation. let mut locked = @@ -838,13 +946,16 @@ impl Subagents { return Err(ChildError::Cancelled); } }; + if yield_when_busy && busy(&locked, prior) { + return Ok(None); + } self.check_ready(&locked)?; if locked.forking.is_some() { return Err(ChildError::Failed( "subagent session is being forked".into(), )); } - self.check_generation(&prior, locked.handle_generation)?; + self.check_generation(prior, locked.handle_generation)?; let generation = locked .generation .checked_add(1) @@ -864,6 +975,35 @@ impl Subagents { let event = locked.runtime_event(prior.id.clone()); drop(locked); self.emit_event(event); + Ok(Some(Admitted { + state, + child, + name, + generation, + })) + } + + async fn run_admitted( + &self, + id: String, + admitted: Admitted, + prompt: ChildPrompt, + cancellation: TurnCancellation, + contract: Option<&OutputContract>, + ) -> Result { + let Admitted { + state, + child, + name, + generation, + } = admitted; + let prior = SubagentValue { + id, + name: None, + output: Value::Null, + generation, + updates: None, + }; match child .prompt_generation( prior.id.clone(), @@ -884,6 +1024,7 @@ impl Subagents { locked.generation_finished_at_unix_ms = Some(events::now_millis()); locked.output.clone_from(&output); locked.updates.clone_from(&updates); + locked.record_finished(); if let Err(error) = self.persist_state(&locked, session::ChildLifecycle::Idle) { drop(locked); return Err(self @@ -907,10 +1048,12 @@ impl Subagents { return Err(error); } let mut locked = state.lock().await; + let started_at_ms = locked.generation_started_at_unix_ms; if locked.status != SubagentStatus::Removed { locked.status = SubagentStatus::Idle; locked.outcome = Some(GenerationOutcome::Failed); locked.generation_finished_at_unix_ms = Some(events::now_millis()); + locked.record_finished(); // A failed call returns no replacement handle, so preserve the // accepted handle generation for a retry while keeping lifecycle // generations monotonic. @@ -918,7 +1061,7 @@ impl Subagents { drop(locked); self.emit_event(event); } - Err(failed_turn(&prior.id, &name, error)) + Err(failed_turn(&prior.id, &name, started_at_ms, error)) } } } @@ -1027,6 +1170,7 @@ impl Subagents { locked.status = SubagentStatus::Idle; locked.outcome = Some(GenerationOutcome::Success); locked.generation_finished_at_unix_ms = Some(events::now_millis()); + locked.record_finished(); let event = locked.runtime_event(success.value.id.clone()); drop(locked); if success.acknowledge.send(()).is_err() { @@ -1104,6 +1248,7 @@ impl Subagents { forking: None, permit: None, cache_key: cache_key.clone(), + finished: Default::default(), }, )?; let branch_name = state.lock().await.name.clone(); @@ -1598,12 +1743,14 @@ impl Subagents { locked.forking = None; locked.outcome = Some(GenerationOutcome::Failed); locked.generation_finished_at_unix_ms = Some(events::now_millis()); + locked.record_finished(); let _ = self.persist_state(&locked, session::ChildLifecycle::Idle); let name = locked.name.clone(); + let started_at_ms = locked.generation_started_at_unix_ms; let event = locked.runtime_event(id.to_string()); drop(locked); self.emit_event(event); - failed_turn(id, &name, error) + failed_turn(id, &name, started_at_ms, error) } async fn cleanup_installed_child( @@ -1695,6 +1842,7 @@ impl Subagents { let sessions = Arc::downgrade(&self.sessions); let state = Arc::downgrade(state); let event_sink = Arc::clone(&self.event_sink); + let changed = Arc::clone(&self.changed); let transcripts = Arc::downgrade(&self.transcripts); let parent_id = self.config.parent_id.clone(); let parent_name = self.config.parent_name.clone(); @@ -1721,6 +1869,7 @@ impl Subagents { let mut event = locked.runtime_event(id.clone()); let generation = locked.generation; drop(locked); + changed.notify_waiters(); if let Some(transcripts) = transcripts.upgrade() && let Ok(transcript) = transcripts.get(&id, generation) { @@ -1769,6 +1918,8 @@ impl Subagents { let mut locked = state.lock().await; if locked.forking.as_deref() == Some(reservation) { locked.forking = None; + drop(locked); + self.changed.notify_waiters(); } } @@ -2011,11 +2162,28 @@ fn continuation_schema() -> serde_json::Value { Value::Object(Map::from_iter([ ( "description".into(), - Value::from("The subagent's id, or a subagent value returned for it."), + Value::from( + "The subagent as `{ id }`, its id string, or a subagent value returned for it.", + ), ), ( - "oneOf".into(), + "anyOf".into(), Value::Array(vec![ + Value::Object(Map::from_iter([ + ("type".into(), Value::from("object")), + ( + "properties".into(), + Value::Object(Map::from_iter([( + "id".into(), + Value::Object(Map::from_iter([( + "type".into(), + Value::from("string"), + )])), + )])), + ), + ("required".into(), Value::from(vec!["id"])), + ("additionalProperties".into(), Value::Bool(false)), + ])), Value::Object(Map::from_iter([( "type".into(), Value::from("string"), @@ -2304,12 +2472,13 @@ struct Continuation { enum SubagentRef { Id(String), Value(SubagentValue), + Handle { id: String }, } impl SubagentRef { fn id(&self) -> &str { match self { - Self::Id(id) => id, + Self::Id(id) | Self::Handle { id } => id, Self::Value(value) => &value.id, } } @@ -2609,6 +2778,7 @@ mod steer_tests { forking: None, permit: Some(manager.acquire_permit().unwrap()), cache_key: None, + finished: Default::default(), })); manager.sessions.lock().unwrap().insert( "source".into(), @@ -2633,6 +2803,7 @@ mod steer_tests { let tool = PromptTool::new(manager); let validator = jsonschema::validator_for(&tool.spec.input_schema).unwrap(); for input in [ + json!({"subagent": {"id": "source"}, "prompt": "focus on tests"}), json!({"subagent": "source", "prompt": "focus on tests"}), json!({"subagent": {"id": "source", "output": null, "generation": 1}, "prompt": "go"}), json!({"subagent": "source", "prompt": [{"type": "text", "text": "focus on tests"}]}), @@ -2875,7 +3046,7 @@ mod steer_tests { assert!(error.contains(&format!("{id:?} (worker)")), "{error}"); assert!(error.contains("nested agent turn failed"), "{error}"); assert!( - error.contains(&format!("prompt({{subagent: {id:?}")), + error.contains(&format!("prompt({{subagent: {{id: {id:?}}}")), "{error}" ); @@ -2970,6 +3141,160 @@ mod steer_tests { .unwrap(); } + #[tokio::test] + async fn injected_callers_get_their_own_generations_result() { + let (manager, state, prior) = manager_with_disconnected_session(Path::new(".")); + { + let mut locked = state.lock().await; + locked.generation = 1; + locked.outcome = Some(GenerationOutcome::Failed); + locked.output = json!("stale"); + locked.record_finished(); + locked.generation = 2; + locked.outcome = Some(GenerationOutcome::Success); + locked.output = json!("second"); + locked.record_finished(); + locked.generation = 3; + locked.status = SubagentStatus::Working; + locked.outcome = None; + locked.output = json!("third in progress"); + } + let cancellation = TurnCancellation::default(); + let failed = manager + .turn_value(&prior.id, &state, 1, &cancellation) + .await + .unwrap_err() + .to_string(); + assert!( + failed.contains("the turn that received this prompt failed"), + "{failed}" + ); + let second = manager + .turn_value(&prior.id, &state, 2, &cancellation) + .await + .unwrap(); + assert_eq!(second.output, json!("second")); + assert_eq!(second.generation, 2); + } + + #[tokio::test] + async fn releasing_a_fork_reservation_wakes_delivery_waiters() { + let (manager, state, _) = manager_with_disconnected_session(Path::new(".")); + state.lock().await.forking = Some("branch".into()); + let changed = manager.changed.notified(); + tokio::pin!(changed); + changed.as_mut().enable(); + manager.finish_forking(&state, "branch").await; + tokio::time::timeout(std::time::Duration::from_secs(1), changed) + .await + .expect("waiters were not woken"); + } + + #[tokio::test] + async fn busy_admission_yields_the_prompt_instead_of_failing() { + let (manager, state, prior) = manager_with_disconnected_session(Path::new(".")); + let cancellation = TurnCancellation::default(); + let prompt: ChildPrompt = "queued".into(); + for (status, forking, generation) in [ + (SubagentStatus::Working, None, prior.generation), + (SubagentStatus::Starting, None, prior.generation), + (SubagentStatus::Idle, Some("fork"), prior.generation), + (SubagentStatus::Idle, None, prior.generation + 1), + ] { + { + let mut locked = state.lock().await; + locked.status = status; + locked.forking = forking.map(str::to_owned); + locked.handle_generation = generation; + } + assert!( + manager + .admit(&prior, &prompt, &cancellation, true) + .await + .unwrap() + .is_none() + ); + assert!( + manager + .admit(&prior, &prompt, &cancellation, false) + .await + .is_err() + ); + let locked = state.lock().await; + assert_eq!(locked.status, status); + assert_eq!(locked.generation, prior.generation); + } + } + + #[tokio::test] + async fn concurrent_prompts_to_an_idle_subagent_run_one_after_another() { + use std::time::Duration; + + tokio::time::timeout(Duration::from_secs(10), async { + let root = tempfile::tempdir().unwrap(); + let mut config = manager_with_disconnected_session(root.path()) + .0 + .child_config(); + config.default_harness = "acp.mock".into(); + config.harnesses = + crate::acp_child::AcpHarnesses::new(std::collections::BTreeMap::from([( + "mock".into(), + crate::acp_child::AcpHarnessProfile { + command: "python3".into(), + args: vec![format!( + "{}/fixtures/mock-acp-v2.py", + env!("CARGO_MANIFEST_DIR") + )], + permissions: Default::default(), + }, + )])) + .unwrap(); + let manager = Subagents::new(config); + let first = manager + .create( + "first".into(), + CreateOptions::default(), + 0, + TurnCancellation::default(), + None, + ) + .await + .unwrap(); + let deliveries = ["a", "b"].map(|text| { + let manager = manager.clone(); + let id = first.id.clone(); + tokio::spawn(async move { + manager + .deliver(&id, text.into(), TurnCancellation::default(), None) + .await + }) + }); + let mut results = Vec::new(); + for delivery in deliveries { + let value = delivery.await.unwrap().unwrap(); + results.push((value.generation, value.output)); + } + results.sort_by_key(|(generation, _)| *generation); + let generations = results + .iter() + .map(|(generation, _)| *generation) + .collect::>(); + assert_eq!(generations, vec![2, 3]); + let mut outputs = results + .into_iter() + .map(|(_, output)| output) + .collect::>(); + outputs.sort_by_key(|output| output.to_string()); + assert_eq!(outputs, vec![json!("a"), json!("b")]); + manager + .close(&first.id, &TurnCancellation::default()) + .await + .unwrap(); + }) + .await + .unwrap(); + } + #[tokio::test] async fn steer_rejects_ineligible_sessions_without_changing_lifecycle() { let (manager, state, prior) = manager_with_disconnected_session(Path::new(".")); diff --git a/src/tools/subagent/recovery.rs b/src/tools/subagent/recovery.rs index 3a3a59c8..2da1b280 100644 --- a/src/tools/subagent/recovery.rs +++ b/src/tools/subagent/recovery.rs @@ -52,6 +52,7 @@ impl Subagents { forking: None, permit: None, cache_key: None, + finished: Default::default(), }; if sessions .insert( diff --git a/src/tools/subagent/tests.rs b/src/tools/subagent/tests.rs index 729790e5..3a6d92e6 100644 --- a/src/tools/subagent/tests.rs +++ b/src/tools/subagent/tests.rs @@ -55,6 +55,7 @@ impl Subagents { forking: None, permit: Some(self.reserve().unwrap()), cache_key: None, + finished: Default::default(), }, ) .unwrap(); @@ -351,6 +352,7 @@ fn manager_with_disconnected_session( forking: None, permit: Some(manager.acquire_permit().unwrap()), cache_key: None, + finished: Default::default(), })); manager.sessions.lock().unwrap().insert( "source".into(), From 73e168c772e753f0fd31977a797f4aa9c6f97d48 Mon Sep 17 00:00:00 2001 From: daniel Date: Thu, 1 Oct 2026 11:51:41 +0100 Subject: [PATCH 3/3] fix(subagents): limit reported fatal causes to the failing turn's run window --- src/fatal.rs | 46 ++++++++++++++++++++++--------------------- src/tools/subagent.rs | 26 +++++++++++++----------- 2 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/fatal.rs b/src/fatal.rs index ebeb7ddc..21b8e6e5 100644 --- a/src/fatal.rs +++ b/src/fatal.rs @@ -268,21 +268,17 @@ pub(crate) fn record_loop_error( .map(Some) } -/// Summarizes the newest fatal record of `session_id` written at or after -/// `since_ms`, without prompt content. -pub(crate) fn latest_cause(session_id: &str, since_ms: u64) -> Option { +/// Summarizes the newest fatal record of `session_id` written within +/// `window` (inclusive milliseconds), without prompt content. +pub(crate) fn latest_cause(session_id: &str, window: (u64, u64)) -> Option { crate::session::validate_id(session_id).ok()?; let home = std::env::var_os("HOME").filter(|home| !home.is_empty())?; - latest_cause_in( - &PathBuf::from(home).join(".kit/errors"), - session_id, - since_ms, - ) + latest_cause_in(&PathBuf::from(home).join(".kit/errors"), session_id, window) } -fn latest_cause_in(base: &Path, session_id: &str, since_ms: u64) -> Option { +fn latest_cause_in(base: &Path, session_id: &str, (since, until): (u64, u64)) -> Option { let directory = base.join(session_id); - let path = fs::read_dir(&directory) + let (_, path) = fs::read_dir(&directory) .ok()? .filter_map(Result::ok) .map(|entry| entry.path()) @@ -290,14 +286,17 @@ fn latest_cause_in(base: &Path, session_id: &str, since_ms: u64) -> Option().ok()) - })?; + .and_then(|millis| millis.parse::().ok())?; + (since..=until).contains(&millis).then_some((millis, path)) + }) + .max_by_key(|(millis, _)| *millis)?; let record: FatalRecord = serde_json::from_slice(&fs::read(&path).ok()?).ok()?; - if record.occurred_at_ms < since_ms { + if !(since..=until).contains(&record.occurred_at_ms) { return None; } let mut cause = format!("{} ({})", record.message, record.code); @@ -1045,7 +1044,7 @@ mod tests { } #[test] - fn latest_cause_ignores_records_older_than_the_failing_turn() { + fn latest_cause_is_limited_to_the_failing_turn() { use agentkit_loop::{ProviderFailure, ProviderFailureReason, ProviderRoute}; let root = tempfile::tempdir().unwrap(); @@ -1067,14 +1066,17 @@ mod tests { Some(&failure), ) .unwrap(); - let cause = super::latest_cause_in(root.path(), "session-1", 0).unwrap(); - assert!(cause.contains("RetryExhausted"), "{cause}"); - let later = std::time::SystemTime::now() + let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap() - .as_millis() as u64 - + 60_000; - assert!(super::latest_cause_in(root.path(), "session-1", later).is_none()); + .as_millis() as u64; + let cause = super::latest_cause_in(root.path(), "session-1", (0, now)).unwrap(); + assert!(cause.contains("RetryExhausted"), "{cause}"); + // A turn that started later must not inherit it. + let later = now + 60_000; + assert!(super::latest_cause_in(root.path(), "session-1", (later, u64::MAX)).is_none()); + // Nor may a turn that finished before it was written. + assert!(super::latest_cause_in(root.path(), "session-1", (0, now - 60_000)).is_none()); } #[test] diff --git a/src/tools/subagent.rs b/src/tools/subagent.rs index b4ca54ce..9be409f2 100644 --- a/src/tools/subagent.rs +++ b/src/tools/subagent.rs @@ -164,13 +164,13 @@ async fn wait_or_cancel( } } -/// `started_at_ms` is when the failing turn began; older fatal records belong -/// to other turns. -fn failed_turn(id: &str, name: &str, started_at_ms: u64, error: ChildError) -> ChildError { +/// `window` is when the failing turn ran; fatal records outside it belong to +/// other turns. +fn failed_turn(id: &str, name: &str, window: (u64, u64), error: ChildError) -> ChildError { let ChildError::Failed(message) = error else { return error; }; - let cause = crate::fatal::latest_cause(id, started_at_ms) + let cause = crate::fatal::latest_cause(id, window) .map(|cause| format!(" Cause: {cause}.")) .unwrap_or_default(); ChildError::Failed(format!( @@ -250,6 +250,7 @@ struct State { struct FinishedTurn { generation: u64, started_at_ms: u64, + finished_at_ms: u64, outcome: GenerationOutcome, output: Value, updates: Option, @@ -283,6 +284,9 @@ impl State { self.finished.push_back(FinishedTurn { generation: self.generation, started_at_ms: self.generation_started_at_unix_ms, + finished_at_ms: self + .generation_finished_at_unix_ms + .unwrap_or_else(events::now_millis), outcome, output: self.output.clone(), updates: self.updates.clone(), @@ -888,7 +892,7 @@ impl Subagents { GenerationOutcome::Failed => Err(failed_turn( id, &locked.name, - turn.started_at_ms, + (turn.started_at_ms, turn.finished_at_ms), ChildError::Failed("the turn that received this prompt failed".into()), )), }; @@ -1048,11 +1052,11 @@ impl Subagents { return Err(error); } let mut locked = state.lock().await; - let started_at_ms = locked.generation_started_at_unix_ms; + let window = (locked.generation_started_at_unix_ms, events::now_millis()); if locked.status != SubagentStatus::Removed { locked.status = SubagentStatus::Idle; locked.outcome = Some(GenerationOutcome::Failed); - locked.generation_finished_at_unix_ms = Some(events::now_millis()); + locked.generation_finished_at_unix_ms = Some(window.1); locked.record_finished(); // A failed call returns no replacement handle, so preserve the // accepted handle generation for a retry while keeping lifecycle @@ -1061,7 +1065,7 @@ impl Subagents { drop(locked); self.emit_event(event); } - Err(failed_turn(&prior.id, &name, started_at_ms, error)) + Err(failed_turn(&prior.id, &name, window, error)) } } } @@ -1742,15 +1746,15 @@ impl Subagents { locked.status = SubagentStatus::Idle; locked.forking = None; locked.outcome = Some(GenerationOutcome::Failed); - locked.generation_finished_at_unix_ms = Some(events::now_millis()); + let window = (locked.generation_started_at_unix_ms, events::now_millis()); + locked.generation_finished_at_unix_ms = Some(window.1); locked.record_finished(); let _ = self.persist_state(&locked, session::ChildLifecycle::Idle); let name = locked.name.clone(); - let started_at_ms = locked.generation_started_at_unix_ms; let event = locked.runtime_event(id.to_string()); drop(locked); self.emit_event(event); - failed_turn(id, &name, started_at_ms, error) + failed_turn(id, &name, window, error) } async fn cleanup_installed_child(