From b771254009f1ed2a9efd654f2adc7f4d9fec82de Mon Sep 17 00:00:00 2001 From: kina0630 Date: Tue, 5 May 2026 00:19:28 +0800 Subject: [PATCH 1/3] fix(subagent): fail stale-owner summary publish Keep generic publisher failures best-effort, but let owner-conflict errors bubble so stale owners cannot hide subagent publication races. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../src/coding_agent/tools/subagent.py | 3 + .../tests/coding_agent/tools/test_subagent.py | 162 +++++++++++++++++- .../ui/test_session_manager_owner_checks.py | 36 ++++ 3 files changed, 200 insertions(+), 1 deletion(-) diff --git a/coding-agent/src/coding_agent/tools/subagent.py b/coding-agent/src/coding_agent/tools/subagent.py index 671b950e5..08883c0c1 100644 --- a/coding-agent/src/coding_agent/tools/subagent.py +++ b/coding-agent/src/coding_agent/tools/subagent.py @@ -16,6 +16,7 @@ from coding_agent.adapter import PipelineAdapter from coding_agent.adapter_types import StopReason, TurnOutcome from coding_agent.agent_identity import effective_agent_id, legacy_agent_id_str +from coding_agent.ui.session_owner_store import SessionOwnershipConflictError from coding_agent.wire.protocol import ToolCallDelta, WireMessage @@ -154,6 +155,8 @@ async def _publish_subagent_summary( ) if isawaitable(publish_result): await publish_result + except SessionOwnershipConflictError: + raise except Exception: logger.warning( "Failed to publish subagent summary for session %s; returning summary anyway", diff --git a/coding-agent/tests/coding_agent/tools/test_subagent.py b/coding-agent/tests/coding_agent/tools/test_subagent.py index 435b6115f..d0f14e1bf 100644 --- a/coding-agent/tests/coding_agent/tools/test_subagent.py +++ b/coding-agent/tests/coding_agent/tools/test_subagent.py @@ -13,7 +13,8 @@ from agentkit.tape.tape import Tape from coding_agent.adapter_types import StopReason, TurnOutcome from coding_agent.__main__ import create_agent, create_child_pipeline -from coding_agent.environment import LocalEnvironment +from coding_agent.environment import CloudCommandResult, CloudEnvironment, LocalEnvironment +from coding_agent.ui.session_owner_store import SessionOwnershipConflictError from coding_agent.wire.protocol import StreamDelta, TurnEnd, WireMessage from coding_agent.tools.subagent import build_subagent_tool @@ -72,6 +73,44 @@ async def request_approval(self, req): ) +class CloudTraceClient: + workspace_id: str = "ws-subagent-123" + workspace_url: str = "https://workspace.example.com?token=secret" + default_cwd: str = "/workspace" + + def read_file(self, path: str) -> str: + return path + + def write_file(self, path: str, content: str) -> None: + del path, content + + def replace_file(self, path: str, old: str, new: str) -> None: + del path, old, new + + def glob_files(self, pattern: str, directory: str) -> list[str]: + del pattern, directory + return [] + + def grep_search(self, pattern: str, directory: str, include: str) -> list[str]: + del pattern, directory, include + return [] + + def apply_patch(self, path: str, patch: str) -> dict[str, object]: + del patch + return {"success": True, "path": path, "changed": False} + + def run_command( + self, + command: str, + *, + cwd: str | None, + env: dict[str, str] | None, + timeout: int, + ) -> CloudCommandResult: + del command, cwd, env, timeout + return CloudCommandResult(stdout="", stderr="", exit_code=0) + + @pytest.mark.asyncio async def test_subagent_tool_runs_real_child_pipeline_and_excludes_nested_subagent(): provider = ScriptedProvider() @@ -650,6 +689,85 @@ async def test_subagent_tool_forwards_parent_environment_to_child_builder( assert captured["environment"] is environment +@pytest.mark.asyncio +async def test_subagent_tool_forwards_cloud_environment_to_child_builder( + monkeypatch: pytest.MonkeyPatch, +): + captured: dict[str, Any] = {} + environment = CloudEnvironment(CloudTraceClient()) + + monkeypatch.setattr( + "coding_agent.tools.subagent.PipelineAdapter", _make_immediate_adapter() + ) + + parent_ctx = PipelineContext( + tape=Tape(), + session_id="parent-session", + run_context=AgentRunContext( + session_id="parent-session", + run_id="parent-run", + agent_id="parent-agent", + environment=environment, + ), + config={ + "agent_id": "parent-agent", + "subagent_timeout": 30.0, + "child_worker_coordinator": _StubCoordinator(), + }, + ) + + tool_fn = build_subagent_tool(_capture_kwargs_builder(captured)) + + await tool_fn(goal="Inspect", __pipeline_ctx__=parent_ctx) + + assert captured["environment"] is environment + + +@pytest.mark.asyncio +async def test_subagent_tool_preserves_authoritative_cloud_trace_metadata( + monkeypatch: pytest.MonkeyPatch, +): + captured: dict[str, Any] = {} + environment = CloudEnvironment(CloudTraceClient()) + + monkeypatch.setattr( + "coding_agent.tools.subagent.PipelineAdapter", _make_immediate_adapter() + ) + + parent_ctx = PipelineContext( + tape=Tape(), + session_id="parent-session", + run_context=AgentRunContext( + session_id="parent-session", + run_id="parent-run", + agent_id="parent-agent", + environment=environment, + trace_metadata={ + "request_id": "req-1", + "cloud.workspace_id": "ws-subagent-123", + }, + ), + config={ + "agent_id": "parent-agent", + "subagent_timeout": 30.0, + "child_worker_coordinator": _StubCoordinator(), + }, + ) + + tool_fn = build_subagent_tool(_capture_kwargs_builder(captured)) + + await tool_fn(goal="Inspect", __pipeline_ctx__=parent_ctx) + + assert captured["trace_metadata"] == { + "request_id": "req-1", + "cloud.workspace_id": "ws-subagent-123", + "subagent.parent_agent_id": "parent-agent", + "subagent.child_agent_id": "parent-agent.child-1", + } + assert "workspace_url" not in captured["trace_metadata"] + assert "secret" not in str(dict(captured["trace_metadata"])) + + @pytest.mark.asyncio async def test_subagent_tool_overwrites_caller_supplied_reserved_trace_keys( monkeypatch: pytest.MonkeyPatch, tmp_path @@ -693,6 +811,48 @@ async def test_subagent_tool_overwrites_caller_supplied_reserved_trace_keys( ) +@pytest.mark.asyncio +async def test_subagent_tool_does_not_swallow_stale_owner_summary_publish( + monkeypatch: pytest.MonkeyPatch, +): + child_ctx = PipelineContext(tape=Tape(), session_id="parent-session") + + async def publish_subagent_message( + session_id: str, + text: str, + *, + message_id: str | None = None, + metadata: dict[str, Any] | None = None, + ) -> bool: + del session_id, text, message_id, metadata + raise SessionOwnershipConflictError("stale owner or fencing token rejected") + + def child_pipeline_builder(**_kwargs: Any): + return cast(Pipeline, object()), child_ctx + + monkeypatch.setattr( + "coding_agent.tools.subagent.PipelineAdapter", _make_immediate_adapter() + ) + + tool_fn = build_subagent_tool(child_pipeline_builder) + parent_ctx = PipelineContext( + tape=Tape(), + session_id="parent-session", + config={ + "agent_id": "parent-agent", + "subagent_timeout": 30.0, + "child_worker_coordinator": _StubCoordinator(), + "subagent_message_publisher": publish_subagent_message, + }, + ) + + with pytest.raises( + SessionOwnershipConflictError, + match="stale owner or fencing token rejected", + ): + await tool_fn(goal="Inspect", __pipeline_ctx__=parent_ctx) + + @pytest.mark.asyncio async def test_subagent_tool_fails_fast_on_empty_parent_session_id( monkeypatch: pytest.MonkeyPatch, diff --git a/coding-agent/tests/ui/test_session_manager_owner_checks.py b/coding-agent/tests/ui/test_session_manager_owner_checks.py index e1df7db1f..5ed8b1721 100644 --- a/coding-agent/tests/ui/test_session_manager_owner_checks.py +++ b/coding-agent/tests/ui/test_session_manager_owner_checks.py @@ -5,6 +5,7 @@ import pytest from agentkit.checkpoint import CheckpointService +from agentkit.runtime import RuntimeMessageCursor, RuntimeMessageKind from coding_agent.ui.session_manager import SessionManager import types from unittest.mock import patch @@ -436,6 +437,41 @@ async def test_submit_approval_rejects_stale_owner() -> None: ) +@pytest.mark.asyncio +async def test_subagent_summary_publish_rejects_stale_owner() -> None: + owner_store = FakeOwnerStore() + manager = SessionManager( + store=InMemorySessionStore(), + checkpoint_service=CheckpointService(FakeCheckpointStore()), + owner_store=owner_store, + owner_id="owner-b", + fencing_token=2, + ) + session_id = await manager.create_session() + owner_store._owners[session_id] = SessionOwnerRecord( + owner_id="owner-a", + lease_expires_at=datetime.now(UTC) + timedelta(seconds=30), + fencing_token=1, + ) + + with pytest.raises( + SessionOwnershipConflictError, + match="stale owner or fencing token rejected", + ): + await manager.publish_subagent_message( + session_id, + "child finished", + message_id="subagent-stale-owner", + ) + + session = manager.get_session(session_id) + batch = await session.runtime_message_bus.consume_after( + RuntimeMessageCursor(), + kinds={RuntimeMessageKind.SUBAGENT_MESSAGE}, + ) + assert batch.messages == () + + @pytest.mark.asyncio async def test_create_session_acquires_owner_when_owner_store_is_configured() -> None: owner_store = FakeOwnerStore() From 4fe6e5f5bfba46ff45826ddc2bf1baca4f0a3511 Mon Sep 17 00:00:00 2001 From: kina0630 Date: Tue, 5 May 2026 00:20:01 +0800 Subject: [PATCH 2/3] docs(adr): add ADR-0018 plan Record the owner-local cloud-aware subagent decision and the PR1 task packet that gates the implementation against executable acceptance criteria. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- ...dr-0018-pr1-owner-local-subagent-parity.md | 52 ++++++ ...ocal-cloud-aware-subagent-orchestration.md | 176 ++++++++++++++++++ 2 files changed, 228 insertions(+) create mode 100644 coding-agent/.opencode/prompts/tasks/adr-0018-pr1-owner-local-subagent-parity.md create mode 100644 coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md diff --git a/coding-agent/.opencode/prompts/tasks/adr-0018-pr1-owner-local-subagent-parity.md b/coding-agent/.opencode/prompts/tasks/adr-0018-pr1-owner-local-subagent-parity.md new file mode 100644 index 000000000..c11ddb94c --- /dev/null +++ b/coding-agent/.opencode/prompts/tasks/adr-0018-pr1-owner-local-subagent-parity.md @@ -0,0 +1,52 @@ +Goal: +Codify ADR-0018 PR 1 by adding focused local/cloud parity regressions for owner-local subagent orchestration, without introducing distributed child-worker coordination. + +Scope: +- Strengthen `coding_agent.tools.subagent` tests so child runs inherit the parent `Environment`, including a cloud `CloudEnvironment` backed by a fake client. +- Add regressions proving reserved `subagent.*` metadata is authoritative while ADR-0017 cloud metadata remains authoritative and secret-free. +- Keep nested subagent delegation unavailable from child toolsets and verify the child prompt/tool-filter contract still holds. +- Keep mutating child tool deltas serialized by the owner-local `ChildWorkerCoordinator` write lease. +- Verify child summary publication still routes through the parent session runtime message bus and respects owner checks. + +Out of scope: +- Do not add PG-backed or provider-backed distributed child write leases. +- Do not run child agents in a separate process, queue worker, pod, or cloud callback handler. +- Do not add a real cloud vendor backend or credentials. +- Do not move `CloudWorkspaceClient`, `CloudEnvironment`, `ChildWorkerCoordinator`, HTTP owner routing, or approval policy into `agentkit`. +- Do not enable nested subagents. + +Context: +- ADRs: + - `docs/adr/0013-define-phase2-multi-instance-session-ownership-for-pg-http-sessions.md` + - `docs/adr/0015-enforce-owner-routed-http-event-streams.md` + - `docs/adr/0016-stage-environment-runtime-and-tool-orchestration-prs.md` + - `docs/adr/0017-cloud-workspace-execution.md` + - `docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md` +- Relevant files: + - `src/agentkit/runtime/context.py` + - `src/coding_agent/app.py` + - `src/coding_agent/tools/subagent.py` + - `src/coding_agent/subagents/coordinator.py` + - `src/coding_agent/ui/session_manager.py` + - `tests/coding_agent/tools/test_subagent.py` + - `tests/coding_agent/subagents/test_coordinator.py` + - `tests/coding_agent/environment/test_cloud_environment.py` + - `tests/coding_agent/test_bootstrap.py` + - `tests/ui/test_session_manager_public_api.py` + +Target tests: +- `uv run pytest tests/coding_agent/tools/test_subagent.py tests/coding_agent/subagents/test_coordinator.py -v` +- `uv run pytest tests/coding_agent/test_bootstrap.py tests/ui/test_session_manager_public_api.py -k "subagent or cloud" -v` + +Loop policy: +- Engineer implements the smallest correct change and runs the target tests. +- Reviewer reviews only the resulting diff and affected tests. +- Reviewer reports only P1/P2 findings. +- Engineer fixes only accepted P1/P2 findings and reruns the same target tests. +- Verifier reruns the exact target tests and reports pass/fail only. + +Stop conditions: +- At most one review/fix/retest cycle. +- Escalate architectural redirection or scope expansion to the human. +- Stop and write a new ADR if implementation needs cross-process child execution, distributed leases, or real cloud vendor transport. +- Ignore non-blocking optimization suggestions. diff --git a/coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md b/coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md new file mode 100644 index 000000000..f05c9f525 --- /dev/null +++ b/coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md @@ -0,0 +1,176 @@ +# ADR-0018: Keep cloud-aware subagent orchestration owner-local + +**Status**: Proposed +**Date**: 2026-05-04 + +## Context + +ADR-0016 staged the runtime pieces that subagents need before they can become a +first-class orchestration surface: `AgentRunContext`, environment sharing, +tool-governed execution, runtime messages, and prompt-time runtime context. +ADR-0017 then added provider-neutral cloud workspace execution and deliberately +deferred full subagent orchestration until the local/cloud tool contract was +executable. + +The current subagent implementation already has useful foundations: + +- `coding_agent.tools.subagent.build_subagent_tool` forks a child tape, derives a + child run context from the parent `AgentRunContext`, forwards the parent + `Environment` to `create_child_pipeline`, disables nested subagents through a + `tool_filter`, appends hidden child trace entries back to the parent tape, and + publishes child summaries through the parent session runtime message bus. +- `coding_agent.subagents.ChildWorkerCoordinator` allocates child agent ids and + serializes mutating child tool events with an in-process `asyncio.Lock`. +- ADR-0013 and ADR-0015 already define HTTP execution as owner-routed and + at-most-once: the parent session owner is the only process allowed to execute + runtime-sensitive work for that session. + +The open design question is whether subagent write coordination should become a +cross-process PostgreSQL-backed lease now that cloud workspaces exist. It should +not. Cloud workspaces change where tools execute; they do not, by themselves, +make child agents independently schedulable workers. Until a child can run +outside the parent session owner process, a PG-backed child lease would add +coordination complexity without protecting a real cross-process actor. + +## Decision + +Implement full subagent orchestration as **owner-local, cloud-aware execution**. + +The core invariant is: + +> A child agent is an execution-local implementation detail of the parent turn. +> Only the current parent session owner may start, run, cancel, emit tool deltas +> for, and publish summaries from child agents. + +This means: + +- Child runs must inherit the parent `Environment`. For a cloud-bound parent, + the child must use the same `CloudEnvironment` and therefore the same + provider-neutral `CloudWorkspaceClient` instance supplied by `coding_agent`. +- Child runs must derive `AgentRunContext` from the parent run: same session id, + new child run id, child agent id, parent run id, inherited context budget, and + merged trace metadata. +- Reserved `subagent.*` trace metadata is owned by the subagent dispatcher and + must overwrite caller-supplied spoofed values. +- Cloud trace metadata remains governed by ADR-0017: caller-supplied `cloud.*` + keys are stripped and the environment injects only authoritative + `cloud.workspace_id`. +- Nested subagent delegation remains disabled for child toolsets. This is a + safety property, not just a prompt optimization. +- Child summaries continue to publish through the parent session runtime message + bus, under the parent owner/fencing checks from ADR-0013 and ADR-0015. +- Mutating child tool events continue to acquire the owner-local + `ChildWorkerCoordinator` write lease. The lease serializes child writes within + one owner process; it is not a distributed lock. +- Provider-specific cloud clients, credentials, and workspace lifecycle remain in + `coding_agent`; no concrete cloud client moves into `agentkit`. + +Cross-process child orchestration is explicitly deferred. Add a new ADR before +allowing any of these behaviors: + +- scheduling child agents in a separate process, pod, queue worker, or cloud + callback handler; +- receiving child tool deltas from a process that is not the parent session + owner; +- sharing one cloud workspace across independently owned parent/child runtimes; +- replacing the owner-local child write lease with a PostgreSQL-backed or + provider-backed distributed lease. + +## Implementation Plan + +### PR 1: Codify owner-local local/cloud parity + +Goal: make the current owner-local contract executable and cloud-aware without +adding a new orchestration layer. + +Affected paths: + +- `src/coding_agent/tools/subagent.py` +- `src/coding_agent/subagents/coordinator.py` +- `src/coding_agent/app.py` +- `src/coding_agent/ui/session_manager.py` +- `tests/coding_agent/tools/test_subagent.py` +- `tests/coding_agent/subagents/test_coordinator.py` +- `tests/coding_agent/environment/test_cloud_environment.py` +- `tests/coding_agent/test_bootstrap.py` +- `tests/ui/test_session_manager_public_api.py` + +Implementation notes: + +- Prefer strengthening existing tests before adding new production types. +- Reuse the fake cloud workspace client pattern from + `tests/coding_agent/environment/test_cloud_environment.py` rather than adding a + real provider dependency. +- Add only the minimum helper code needed to make the subagent contract explicit. +- Keep `tool_filter=lambda tool_name: tool_name != "subagent"` as the nested + delegation guard unless a later ADR replaces it with a broader policy object. +- Keep `ChildWorkerCoordinator` in `coding_agent`; do not introduce a PG-backed + child lease in this PR. + +### PR 2: Harden cancellation, failure, and summary routing + +Goal: make child failures deterministic at the parent surface. + +Scope: + +- Child timeout, cancellation, and adapter close behavior remains model-visible + through the existing subagent summary path. +- Summary publication remains best-effort for child result return, but ownership + failures from the parent session manager remain fail-fast. +- Hidden child tape entries remain marked `skip_context=True` and + `subagent_child=True` so context extraction does not treat them as parent user + turns. + +### Future ADR: Distributed child workers + +Write a new ADR before this repository allows child agents to run outside the +parent session owner. That ADR must define the durable coordination mechanism, +fencing semantics, retry/idempotency model, and cloud workspace write policy. + +## Alternatives Rejected + +- Add PG-backed child write leases now — rejected because child agents are still + executed only inside the parent session owner process. A distributed lease would + not protect any current cross-process actor and would add a second coordination + model beside ADR-0013 ownership. +- Move subagent orchestration into `agentkit` now — rejected because the current + orchestration policy depends on `coding_agent` concerns: concrete + environments, provider configuration, HTTP owner routing, approval behavior, + child summary publication, and product-specific tool risk policy. +- Enable nested subagents for child runs — rejected because it complicates write + coordination, trace attribution, and parent summary semantics before the first + owner-local cloud-aware contract is tested. +- Implement a real cloud vendor child-worker backend now — rejected because + network transport, credentials, sandbox lifecycle, and distributed child + scheduling are separate concerns from proving the owner-local contract. +- Treat at-most-once HTTP execution as sufficient by itself — rejected as an + incomplete argument. The safety condition is stronger: no non-owner process may + execute a child or emit child tool deltas for the session. + +## Acceptance Criteria + +- [x] `test_subagent_tool_forwards_parent_environment_to_child_builder` +- [x] `test_subagent_tool_forwards_cloud_environment_to_child_builder` +- [x] `test_subagent_tool_preserves_authoritative_cloud_trace_metadata` +- [x] `test_subagent_tool_overwrites_caller_supplied_reserved_trace_keys` +- [x] `test_subagent_tool_child_system_prompt_explicitly_disables_nested_subagent` +- [x] `test_subagent_tool_appends_hidden_child_trace_to_parent_tape` +- [x] `test_subagent_tool_acquires_write_lease_for_mutating_child_tool_event` +- [x] `test_subagent_tool_skips_write_lease_for_read_only_child_turn` +- [x] `test_subagent_tool_publishes_completion_summary_to_parent_session` +- [x] `test_subagent_summary_publish_rejects_stale_owner` +- [x] `uv run pytest tests/coding_agent/tools/test_subagent.py tests/coding_agent/subagents/test_coordinator.py tests/coding_agent/test_bootstrap.py tests/ui/test_session_manager_public_api.py -k "subagent or cloud" -v` + +## References + +- `docs/adr/0013-define-phase2-multi-instance-session-ownership-for-pg-http-sessions.md` +- `docs/adr/0015-enforce-owner-routed-http-event-streams.md` +- `docs/adr/0016-stage-environment-runtime-and-tool-orchestration-prs.md` +- `docs/adr/0017-cloud-workspace-execution.md` +- `src/agentkit/runtime/context.py` +- `src/coding_agent/app.py` +- `src/coding_agent/tools/subagent.py` +- `src/coding_agent/subagents/coordinator.py` +- `src/coding_agent/ui/session_manager.py` +- `tests/coding_agent/tools/test_subagent.py` +- `tests/coding_agent/subagents/test_coordinator.py` From 4bb15ebbc956eab64a91e8292eec2cf4aaac992a Mon Sep 17 00:00:00 2001 From: kina0630 Date: Tue, 5 May 2026 00:33:35 +0800 Subject: [PATCH 3/3] docs(adr): include owner-check acceptance test Keep the ADR-0018 acceptance command aligned with the checklist by including the owner-check test file that exercises stale-owner summary publish rejection. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../adr/0018-owner-local-cloud-aware-subagent-orchestration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md b/coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md index f05c9f525..46cce6a47 100644 --- a/coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md +++ b/coding-agent/docs/adr/0018-owner-local-cloud-aware-subagent-orchestration.md @@ -159,7 +159,7 @@ fencing semantics, retry/idempotency model, and cloud workspace write policy. - [x] `test_subagent_tool_skips_write_lease_for_read_only_child_turn` - [x] `test_subagent_tool_publishes_completion_summary_to_parent_session` - [x] `test_subagent_summary_publish_rejects_stale_owner` -- [x] `uv run pytest tests/coding_agent/tools/test_subagent.py tests/coding_agent/subagents/test_coordinator.py tests/coding_agent/test_bootstrap.py tests/ui/test_session_manager_public_api.py -k "subagent or cloud" -v` +- [x] `uv run pytest tests/coding_agent/tools/test_subagent.py tests/coding_agent/subagents/test_coordinator.py tests/coding_agent/test_bootstrap.py tests/ui/test_session_manager_public_api.py tests/ui/test_session_manager_owner_checks.py -k "subagent or cloud" -v` ## References