Repository navigation
fix: await async-launched sub-agents in the investigator and service-agent drivers - #368
Conversation
|
Claude finished @mashkovd's task in 6m 19s —— View job Review complete — changes requested (0 P1, 1 P2, 3 P3)
What holds upThe per-driver audit in the PR body is accurate, and I checked it against the source rather than taking it on trust:
P2 — the pinned precondition covers one of the two builders the driver can pick
The equivalence test that would cover it by construction does not: One line fixes it: P3
Not verifiedI could not run Note: this review is pinned to |
Resolves the overlap between this branch and 19f1146 on the base. Both added a test asserting the same fact — that `hooks`, not the mctl MCP config, is what makes the SDK hold stdin open past a result frame — and two near-duplicates of a single SDK fact drift apart. Folds 19f1146's implementer test and this branch's two per-driver copies into one table-driven test over every builder that has the precondition: implementer, service-agent, incident-responder, and both issue-investigator builders. The base's stronger form is the one kept: set MCTL_TOKEN so `mcp_servers` is actually populated, assert it is non-empty, and assert none of it is `type: "sdk"`. The two copies added here lacked the setenv, so their "no sdk-type server" assertion ran against an empty dict and passed vacuously — exactly the rot 19f1146 was guarding against. Adds the matching negative, which the audit in #368 turned up and nothing executed: `build_mentor_options` passes an http mctl server but NO hooks, so `sdk_mcp_servers or hooks` is falsy and the mentor cannot be drained at all. Draining it as it stands would convert a silent loss into a guaranteed OrphanedSubagentError on every delegating run. The test exists to stop the #366 pattern being copied there on the assumption that "it has an MCP server, so it qualifies", and to make giving the mentor hooks a decision someone has to take deliberately rather than a refactor that slips through. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
86cc83a to
a6a76d0
Compare
|
Claude finished @mashkovd's task in 7m 5s —— View job Review complete — changes requested (1 P2, 4 P3)
P2 — 429 during the drain loses the
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| """ | ||
| from orchestrator.options import build_issue_investigator_options | ||
|
|
||
| options = build_issue_investigator_options( |
There was a problem hiding this comment.
P2 — the pinned precondition only covers one of the two builders _run_agent can pick.
_run_agent (run_issue_investigator.py:1320-1328) resolves options through build_issue_investigator_options_from_plan when ISSUE_INVESTIGATOR_RESOLVER_MODE=declarative, and the drain added below it runs unconditionally for both modes. This test only pins build_issue_investigator_options. The PR description says "both builders", but nothing asserts it for the _from_plan twin.
The equivalence test that would otherwise cover it by construction does not: tests/test_options.py:143-150 asserts cwd, model, allowed_tools, mcp_servers, permission_mode, max_budget_usd, add_dirs, env — and omits hooks, despite build_issue_investigator_options_from_plan's own docstring claiming "every structural field the plan does NOT declare (cwd, mcp_servers, permission_mode, add_dirs, env, hooks) ... the equivalence tests in tests/test_options.py assert this directly".
Failure scenario: someone drops hooks=_command_audit_hooks() from build_issue_investigator_options_from_plan (options.py:388) only — e.g. while making the plan own hooks. Every test stays green, including this one and the equivalence test. In declarative mode the SDK then closes stdin at the first result frame (sdk_mcp_servers or hooks is falsy, and sdk_mcp_servers is always empty here since every mctl server is type: "http"), the CLI exits, the stream ends with a live task, and drain_until_settled raises on stream exhaustion — so every delegating investigation reports harness failure — the run ended with a delegated sub-agent still live and exits 1. That is strictly worse than the pre-PR silent partial, and no test says so.
Cheapest fix is one line in the existing equivalence test (assert declarative.hooks == legacy.hooks), which also makes that docstring true; or parametrise this test over both builders.
There was a problem hiding this comment.
Half stale, half real — and the real half is a good catch.
Stale half: the driver-level test this points at was deleted in e001ab6, which replaced it with a table-driven test in tests/test_options.py covering all five builders, including build_issue_investigator_options_from_plan. Your exact failure scenario — drop hooks from the _from_plan builder alone — now fails on assert built.hooks for the issue-investigator/from_plan row. This review was pinned to 86cc83a, before that commit.
Real half, fixed in f651c45: the equivalence test omitted hooks, even though build_issue_investigator_options_from_plan's own docstring lists it among the fields "the equivalence tests in tests/test_options.py assert this directly". That claim was simply false. Added assert declarative.hooks == legacy.hooks, which makes the docstring true and puts the guard where the docstring sends a reader looking for it — worth having independently of the table-driven test.
| f"awaiting terminal status" | ||
| ) | ||
| try: | ||
| await drain_until_settled( |
There was a problem hiding this comment.
P3 — the 429 check only guards the first result frame, so a rate limit hit during the follow-up turn loses the rate_limited=True classification.
The drain reads with on_message=print and only stops on ledger.live emptying. When a delegated child settles, the SDK wakes the parent for a follow-up turn, so a second ResultMessage normally arrives on this stream — but drain_until_settled returns as soon as the task_updated/task_notification clears the ledger, and nothing inspects a ResultMessage seen during the drain.
Failure scenario: the child completes, the parent's follow-up turn (writing the triplet) hits the five_hour limit and the CLI emits is_error=True, api_error_status=429. _run_agent returns normally; investigate() then fails the triplet check and returns InvestigateResult(error="...", rate_limited=False). An operator (or run-fallback) reading rate_limited sees "agent produced no triplet" rather than "this account is out of quota", and the account-2 retry that exists for exactly this case is not taken.
Narrow, and not a regression — receive_response() also returned before any follow-up turn — so P3. If you want it closed cheaply, pass an on_message that re-applies the same 429 predicate instead of bare print.
There was a problem hiding this comment.
Superseded by the newer review's P2 on the same code, answered in full there — fixed in f651c45, with the residual ordering your suggested on_message remedy does not cover named explicitly in the test docstring and escalated to #367.
| without tearing down the sibling agents in the same task group. | ||
| """ | ||
|
|
||
| PROMPT = """\ |
There was a problem hiding this comment.
P3 — only one blank line between the end of the ServiceAgentOrphanedSubagent class body and PROMPT. PEP 8 wants two after a top-level class. select = ["E"] will not catch it (ruff implements E301-E306 in preview only), so this is a nit, not a CI failure — but every other top-level definition in this file uses two.
There was a problem hiding this comment.
Fixed in f651c45 — same finding as the newer review, same fix.
| TaskUpdatedMessage, | ||
| ) | ||
|
|
||
| from orchestrator.run_issue_investigator import ( # noqa: E402 |
There was a problem hiding this comment.
P3 — the mid-file import (and its noqa: E402) is avoidable: run_issue_investigator is already imported at the top of this file, so run_issue_investigator.InvestigatorOrphanedSubagent works without a new import statement, and the three claude_agent_sdk message classes belong in the top import block next to the existing ResultMessage import.
Also: _task_started/_task_updated/_result are now duplicated verbatim in tests/test_run_service_agent.py (and near-verbatim in the implementer/subagent-wait tests). tests/conftest.py already hosts FakeMcpClient/fake_mcp_client_factory for exactly this; a third copy is the point at which they are worth moving there, so a future SDK field addition is one edit rather than four.
There was a problem hiding this comment.
Both points fixed in f651c45.
Mid-file import: gone, along with its noqa: E402. You were right that it was pure self-inflicted noise — run_issue_investigator is already imported at the top, so run_issue_investigator.InvestigatorOrphanedSubagent works, and TaskUpdatedMessage moved into the top block next to the existing ResultMessage import.
Duplicated builders: took this too. task_started_message, task_updated_message, task_notification_message and result_message now live in tests/conftest.py beside FakeMcpClient, for exactly the reason you give — a future SDK field addition should be one edit. I switched this branch's two files over and deliberately left #367's copies (test_run_implementer_timeout.py, test_subagent_wait.py) for its author rather than editing the parent branch's tests from a stacked child; the factories are in place for them to adopt.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 3 P3.
The per-driver audit checks out against the source — I verified independently that hooks=_command_audit_hooks() is present on the service-agent, implementer, incident-responder and both investigator builders and absent from build_mentor_options/build_shepherd_options, so the decisions to convert two drivers, skip the responder and refuse to convert the mentor are all correct. tests/conftest.py already grew receive_messages() in #367, so the receive_response() -> receive_messages() switch does not silently break the existing MCP-guard fakes. run_all._safe_run_service does catch the new exception and prints type(exc).__name__, so that test asserts something real.
P2: the hooks precondition is pinned for build_issue_investigator_options only. _run_agent also reaches build_issue_investigator_options_from_plan under ISSUE_INVESTIGATOR_RESOLVER_MODE=declarative, and the legacy/declarative equivalence test in tests/test_options.py omits hooks from its field-by-field comparison despite the builder docstring claiming it covers it. One added assertion closes both gaps.
P3s: 429 misclassification if the limit is hit during the post-drain follow-up turn; PEP 8 blank line before PROMPT; avoidable mid-file test import plus a third copy of the _task_started/_task_updated helpers.
Note: this review is pinned to 86cc83a; the PR head has since moved to a6a76d0.
Resolves the overlap between this branch and 19f1146 on the base. Both added a test asserting the same fact — that `hooks`, not the mctl MCP config, is what makes the SDK hold stdin open past a result frame — and two near-duplicates of a single SDK fact drift apart. Folds 19f1146's implementer test and this branch's two per-driver copies into one table-driven test over every builder that has the precondition: implementer, service-agent, incident-responder, and both issue-investigator builders. The base's stronger form is the one kept: set MCTL_TOKEN so `mcp_servers` is actually populated, assert it is non-empty, and assert none of it is `type: "sdk"`. The two copies added here lacked the setenv, so their "no sdk-type server" assertion ran against an empty dict and passed vacuously — exactly the rot 19f1146 was guarding against. Adds the matching negative, which the audit in #368 turned up and nothing executed: `build_mentor_options` passes an http mctl server but NO hooks, so `sdk_mcp_servers or hooks` is falsy and the mentor cannot be drained at all. Draining it as it stands would convert a silent loss into a guaranteed OrphanedSubagentError on every delegating run. The test exists to stop the #366 pattern being copied there on the assumption that "it has an MCP server, so it qualifies", and to make giving the mentor hooks a decision someone has to take deliberately rather than a refactor that slips through. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
a6a76d0 to
2e93c0f
Compare
|
Claude finished @mashkovd's task in 7m 30s —— View job Review complete — approved (0 P1, 0 P2, 5 P3)Re-review pinned to
The prior P2 is closed
What I verified against the source
P3 findings
Not verified
Note: the head moved to |
| f"awaiting terminal status" | ||
| ) | ||
| try: | ||
| await drain_until_settled( |
There was a problem hiding this comment.
P2 — a 429 that arrives during the drain loses the rate_limited signal this driver is otherwise careful to preserve.
The main loop inspects only the first result frame; every later message is consumed by drain_until_settled, whose on_message defaults to print and whose only other inspection is ledger.observe — neither looks at is_error / api_error_status. So if the account's five-hour window empties mid-run while a child is live (it can roll over during a long investigation), the CLI emits a 429 ResultMessage and exits, the drain ends on stream exhaustion with the task still live, and the run reports InvestigatorOrphanedSubagent → "harness failure — ..." with rate_limited=False. run_issue_poller.poll() keys its rate_limited_failures count off exactly that flag, so an exhausted account keeps getting issues dispatched into it, and the operator reads "the platform lost the child" when the real cause was quota.
drain_until_settled already takes on_message, so the fix is a callback that prints and re-applies the same 429 check:
def _check(message: Any) -> None:
print(message)
if isinstance(message, ResultMessage) and message.is_error and message.api_error_status == 429:
raise RateLimitExhaustedError(...)plus a test feeding [_task_started(), _ok_result(), _result_message(is_error=True, api_error_status=429)] and asserting RateLimitExhaustedError, not InvestigatorOrphanedSubagent.
There was a problem hiding this comment.
Fixed in f651c45 — the scenario you describe here is closed.
The 429 predicate is now a single _note closure used both in the turn loop and as the drain's on_message, so it applies to every result frame rather than only the first. In your ordering the 429 frame arrives while the task is still live, so _note raises RateLimitExhaustedError before the drain can end on stream exhaustion — rate_limited=True survives and run_issue_poller.poll()'s rate_limited_failures counts it. The test is the one you specified: [task_started, ok_result, result(is_error=True, api_error_status=429)] asserting RateLimitExhaustedError. Verified it fails without the on_message change.
One thing worth recording, because your suggested remedy does not fully cover it: there is a second ordering where the child reaches a terminal status FIRST and the 429 lands on the parent's follow-up turn after that. No on_message can see that frame, because drain_until_settled returns the instant ledger.live empties. I wrote a test for that ordering first, watched it fail with the fix in place, and retargeted it — the residual is named in the test docstring rather than left implicit.
That residual is the same root cause as your drain-stop-condition finding on run_service_agent.py:128, and the fix is the same one line of drain_until_settled. That helper belongs to #367, the implementer depends on its current semantics, and test_drain_returns_when_last_task_settles pins the present behaviour there deliberately — so I have escalated it rather than changing a shared helper from a stacked branch. Answering on that thread too.
There was a problem hiding this comment.
Update after the final rebase onto main (#367 merged as 2c72f6c): this is now fully fixed, and so is the residual I flagged above.
The merged drain_until_settled is a loop whose stop condition is the SDK's own — a ResultMessage arriving with nothing in flight — rather than the ledger emptying. So it keeps reading past the child's terminal status and into the turn that settling woke, which is exactly where the follow-up-turn 429 lands, and on_message sees it. I verified this empirically rather than assuming: wrote the previously-impossible ordering as a test, it now passes, and it is committed as test_investigator_classifies_a_rate_limit_on_the_follow_up_turn rather than left as a happy accident of the helper's stop condition. If that condition ever reverts, that test is the driver-level consequence and will say so.
Both orderings are now classified; the docstring that described the residual has been replaced with one that describes the coverage.
| f"awaiting terminal status" | ||
| ) | ||
| try: | ||
| await drain_until_settled( |
There was a problem hiding this comment.
P3 — the drain stops at the child's terminal status, not at the result frame the SDK itself treats as end-of-run, so the parent's post-delegation work is still discarded.
Per the SDK note quoted in orchestrator/subagent_wait.py, the stdin hold exists because "each task completion wakes the parent for a follow-up turn, so a later result frame arrives with no tasks in flight and closes stdin then". drain_until_settled returns the instant ledger.live empties — i.e. exactly when that follow-up turn begins — and returning from here exits async with ClaudeSDKClient, which disconnects and tears the CLI down.
For this prompt that is the part most likely to matter: if the CLI launches step 1 (researcher) asynchronously, the parent's steps 2-4 (the ## Top-3 append, the proposals/<slug>/ triplets, the final report) all live in the woken-up turn that gets cut off. Same shape in the investigator when the parent delegates only the research and writes the triplet itself.
Not a regression — recovering the child's own work is strictly better than stopping at the first result frame — and the behaviour is pinned by test_drain_returns_when_last_task_settles on the #367 base, so it is a deliberate choice there rather than something introduced here. But it is the last variant of #366 still live, and the fix is local to drain_until_settled: keep reading until a ResultMessage arrives with ledger.live empty, which is the same condition the SDK uses to close stdin. Worth a follow-up issue if not done in this stack.
There was a problem hiding this comment.
Agreed, and I reached the same conclusion independently from the other direction — the 429 P2 has a residual ordering (child settles, then the parent's follow-up turn hits the limit) that no on_message can observe, and it is this same stop condition. Two findings, one root cause.
Not changing it here, deliberately. drain_until_settled is orchestrator/subagent_wait.py, which belongs to #367; the implementer depends on its current semantics, and test_drain_returns_when_last_task_settles pins the present behaviour there as a deliberate choice rather than an oversight. Changing a shared helper's contract from a branch stacked on top of it — where the change would be reviewed against the wrong diff and merge in the wrong order — is the kind of thing that turns one bug into two. Escalated to the branch owner with your proposed condition (keep reading until a ResultMessage arrives with ledger.live empty, which is exactly the condition the SDK itself uses to close stdin) so it lands in #367 or a follow-up issue, whichever they prefer.
Your framing is the useful part and I have passed it on verbatim: this is the last variant of #366 still live, and it is strictly narrower than what the PR fixes — recovering the child's work beats stopping at the first result frame either way — so it is a follow-up, not a blocker on this PR. The service-agent prompt is indeed where it bites hardest: steps 2-4 all live in the woken-up turn.
There was a problem hiding this comment.
Resolved upstream. #367 merged as 2c72f6c with exactly the change you proposed: drain_until_settled now stops at a ResultMessage arriving with nothing in flight — the SDK's own predicate — instead of at the ledger emptying, so the parent's post-delegation work is no longer cut off. It also loops, so a second delegation made in the turn the first child woke is awaited like the first.
That matters for this driver specifically, as you noted: steps 2-4 of its prompt live in the woken-up turn. And since its three steps are named personas in sequence, delegating twice is the normal path here rather than an edge case — so I pinned it at driver level with test_service_agent_awaits_a_second_delegation. The helper's own tests cannot cover that: it is possible to consume the relaunch in the driver's turn loop and never enter the drain at all, which is a wiring bug the helper cannot see.
This PR is now rebased onto main and no longer stacked.
| # after the `.claude/agents/{researcher,analyst,spec-writer}.md` personas that | ||
| # setting_sources=["project"] loads from cwd, so the CLI has real sub-agents to | ||
| # delegate to here. See orchestrator/subagent_wait.py. | ||
| SERVICE_AGENT_DRAIN_TIMEOUT_SECONDS = float( |
There was a problem hiding this comment.
P3 — the two new drain knobs have no test pinning their env-var names, unlike their twin.
IMPLEMENTER_DRAIN_TIMEOUT_SECONDS has both a default assertion and an env-override reload test (tests/test_options.py:243-258). SERVICE_AGENT_DRAIN_TIMEOUT_SECONDS here, and ISSUE_INVESTIGATOR_DRAIN_TIMEOUT_SECONDS in run_issue_investigator.py, have neither — the driver tests monkeypatch the module attribute, which exercises the value but never the os.getenv name. A typo in either name would be silent: the knob documents itself as tunable while doing nothing, and the moment you find out is when you raise it during an incident and nothing changes. Two asserts in the existing style close it.
There was a problem hiding this comment.
Fixed in edfe8c4. Added a default assertion plus a parametrised env-override reload test over both names, mirroring the implementer twin at tests/test_options.py. Your reasoning is the part worth pinning and I put it in the docstring: the driver tests exercise the value but never the os.getenv string, so a typo leaves the knob documenting itself as tunable while doing nothing — and you find out when you raise it during an incident and nothing changes. Verified the test earns its keep by misspelling one getenv name and watching that parametrised case go red.
One correction: ISSUE_INVESTIGATOR_DRAIN_TIMEOUT_SECONDS moved out of run_issue_investigator.py into options.py in 2e93c0f, so both knobs now live beside the implementer's and all three are registered in validate_manifest._ENV_VARS_AFFECTING_OPTIONS_DEFAULTS. The service-agent one was missing from that tuple until this branch added it.
| # cast: receive_messages() is declared AsyncIterator but is an async | ||
| # generator, so it does have aclose(); aclosing() guarantees it is | ||
| # closed on the error path below. | ||
| stream = cast("AsyncGenerator[Any, None]", client.receive_messages()) |
There was a problem hiding this comment.
P3 — this 30-line block is now byte-identical in three drivers (run_implementer.py:713-758, run_issue_investigator.py:1338-1395, here), comments included, with only the timeout constant and the orphan exception type varying. The #366 class of bug spread precisely because the async for message in client.receive_response() loop was copy-pasted per driver; this replaces it with a longer thing to copy-paste. A single subagent_wait helper — e.g. await consume_until_settled(client, timeout_s=..., orphan=ServiceAgentOrphanedSubagent, on_result=...) owning the cast/aclosing/break/drain/all_completed warn — would leave each driver with one line, keep the aclosing and single-generator invariants stated once, and is where a change to the drain's stop condition (separate comment) would land once instead of three times. The incident responder, which the PR notes has the precondition but nothing to delegate to yet, then becomes a one-liner if it ever grows a delegating prompt.
There was a problem hiding this comment.
This is the sharpest finding in the review and I think you are right on the merits: "the #366 class of bug spread precisely because the loop was copy-pasted per driver; this replaces it with a longer thing to copy-paste" is a fair description of what this PR does. Three byte-identical 30-line blocks is a worse invariant than one helper, and your point that the drain's stop condition would then be fixed once instead of three times is exactly right — those two findings compound.
Not doing it here, for the same reason as the stop-condition thread: consume_until_settled would live in orchestrator/subagent_wait.py and would require rewriting run_implementer._run_implementer_agent, both of which belong to #367, the PR this one is stacked on. Refactoring the parent branch's code from the child branch means the change is reviewed against the wrong diff, and it inverts the merge order. The right shape is: #367 (or a follow-up on top of the merged stack) extracts the helper and converts all three call sites at once, with the stop-condition change folded in, since anyone touching one will want the other.
Escalated to the branch owner along with the stop-condition finding, as one piece of work rather than two. Recording the agreement here so it is not lost if that follow-up is written by someone who only reads the thread: the duplication is a real defect in this PR, accepted knowingly, not an oversight.
There was a problem hiding this comment.
Still open and still worth doing — recording it here so it is not lost now that #367 has merged (2c72f6c) and the stop-condition half of this is done.
What changed: the two findings no longer have to move together. The stop condition landed in #367, so extracting consume_until_settled is now a standalone refactor against main rather than something that had to be coordinated across a stack. It would collapse the three byte-identical blocks in run_implementer, run_issue_investigator and run_service_agent into one call each.
Still not doing it in this PR: it would rewrite run_implementer, which this PR does not otherwise touch, and bundling an unrelated three-driver refactor into a PR whose diff is about converting two drivers makes both harder to review and to revert. Flagged to the branch owner as a follow-up issue against main, with your framing — the duplication is a real defect here, accepted knowingly.
| without tearing down the sibling agents in the same task group. | ||
| """ | ||
|
|
||
| PROMPT = """\ |
There was a problem hiding this comment.
P3 — one blank line between the new class and PROMPT; PEP 8 wants two after a top-level definition. Ruff's default rule set (E4/E7/E9 + F) does not include E305, so this is not caught by uv run ruff check, which is exactly why it is worth fixing by hand here.
There was a problem hiding this comment.
Fixed in f651c45 — two blank lines. You are right that ruff not catching it is the reason to fix it by hand rather than a reason to leave it.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2, 4 P3.
P2: in run_issue_investigator._run_agent the 429 check is applied only to the first result frame. Every message consumed by drain_until_settled goes through on_message=print and ledger.observe, neither of which looks at is_error/api_error_status, so a quota exhaustion that lands while a child is live surfaces as InvestigatorOrphanedSubagent ("harness failure") with rate_limited=False — losing the one flag run_issue_poller.poll() branches on. drain_until_settled already takes on_message, so the fix is a callback plus one test.
P3s: the drain returns when the child settles rather than at the result frame with no tasks in flight, so the parent's woken-up follow-up turn (service-agent steps 2-4) is still cut off — inherited from #367, fix belongs in drain_until_settled; no test pins the new env-var names (the implementer twin has one); the 30-line drain block is now byte-identical in three drivers; one missing blank line before PROMPT.
The per-driver precondition audit checks out against subagent_wait.py and the repo's own config: every mctl MCP server is type "http", so hooks are the whole precondition, and leaving run_mentor alone (plus the executable negative test for it) is the right call. Both headline tests assert consumption past the ResultMessage rather than mere return, which is the assertion that actually fails without the drain. I could not run the suite in this environment (dependencies are not installed), so the reported 1048 passed / ruff / mypy results are taken as stated.
… first Addresses claude[bot]'s review on #368. P3 (run_issue_investigator) — the 429 check guarded only the first result frame. The drain read with a bare `print`, so a limit verdict arriving while a delegated child was still live went unclassified: `_run_agent` returned normally, investigate() failed the triplet check, and the result carried `rate_limited=False`. An operator, or the account-2 fallback that exists for exactly this case, then read "agent produced no triplet" instead of "this account is out of quota". The predicate is now a single `_note` closure used both in the turn loop and as the drain's `on_message`, so it applies wherever a result frame appears. KNOWN RESIDUAL, escalated rather than fixed here: if the child reaches a terminal status FIRST and the 429 lands on the parent's follow-up turn after that, `drain_until_settled` has already returned -- it stops the instant `ledger.live` empties -- so no `on_message` can observe that frame. The reviewer's suggested remedy ("pass an on_message that re-applies the predicate") does not close this ordering either. Closing it requires the shared helper to keep reading to the next result frame, which is orchestrator/subagent_wait.py in #367 and is depended on by the implementer, so it is not changed from this branch. The test names the residual. P2 (tests) — the driver-level test the finding points at was already deleted in e001ab6, which replaced it with a table-driven test in tests/test_options.py covering all five builders including `build_issue_investigator_options_from_plan`. The finding's SECOND half is real and is fixed here: the legacy/declarative equivalence test omitted `hooks`, even though the builder's own docstring lists it among the fields "the equivalence tests in tests/test_options.py assert this directly". One line, and it makes that docstring true. P3 (run_service_agent) — two blank lines after the top-level class, per PEP 8. P3 (tests) — dropped the mid-file import and its `noqa: E402`: the module is already imported at the top, so `run_issue_investigator.InvestigatorOrphanedSubagent` works, and the SDK message class moved into the top import block. Also took the reviewer's second point and moved the duplicated `task_started`/`task_updated`/ `task_notification`/`result` builders into tests/conftest.py beside FakeMcpClient, so a future SDK field addition is one edit. Only this branch's two files are switched over; #367's copies are left for its author. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
|
Claude finished @mashkovd's task in 5m 19s —— View job Review complete — changes requested (0 P1, 1 P2, 2 P3)
Prior findings — closed
P2 — the containment claim covers
|
Addresses claude[bot]'s P3 on #368. The driver tests monkeypatch the module attribute, so the VALUE is exercised but the `os.getenv` string never is. A typo there would be silent in the worst way: the knob documents itself as tunable while doing nothing, and you find out when you raise it during an incident and nothing changes. Mirrors the implementer twin's existing default + env-override pair; verified by misspelling one getenv name and watching the parametrised case go red. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
Antigravity (agy) reviewModel: gemini-3.1-pro-high · commit No significant issues found. |
| without tearing down the sibling agents in the same task group. | ||
| """ | ||
|
|
||
| PROMPT = """\ |
There was a problem hiding this comment.
P3 — only one blank line between the end of the ServiceAgentOrphanedSubagent class body and PROMPT. PEP 8 wants two after a top-level class, and every other top-level definition in this file uses two. Ruff will not catch it (E301-E306 are preview-only), so it is a nit rather than a CI failure. Unchanged since the 86cc83a review.
| f"awaiting terminal status" | ||
| ) | ||
| try: | ||
| await drain_until_settled( |
There was a problem hiding this comment.
P3 — the 429 check only guards the first result frame, so a rate limit hit during the post-drain follow-up turn loses the rate_limited=True classification.
The drain reads with the default on_message=print and stops as soon as ledger.live empties. When a delegated child settles, the SDK wakes the parent for a follow-up turn, so a second ResultMessage normally arrives on this stream — but drain_until_settled returns on the task_updated/task_notification that clears the ledger, and nothing re-applies the 429 predicate to a ResultMessage seen during the drain.
Failure scenario: the child completes; the parent's follow-up turn (writing the requirements/design/tasks triplet) hits the five_hour limit and the CLI emits is_error=True, api_error_status=429. _run_agent returns normally; investigate() then fails the triplet check and returns InvestigateResult(error="...", rate_limited=False). An operator (or run-fallback) reading rate_limited sees "agent produced no triplet" rather than "this account is out of quota", and the account-2 retry that exists for exactly this case is not taken.
Narrow and not a regression — receive_response() never saw a follow-up turn either — so P3. Cheapest close is passing an on_message that re-applies the same predicate instead of the bare print default. Unchanged since the 86cc83a review.
Resolves the overlap between this branch and 19f1146 on the base. Both added a test asserting the same fact — that `hooks`, not the mctl MCP config, is what makes the SDK hold stdin open past a result frame — and two near-duplicates of a single SDK fact drift apart. Folds 19f1146's implementer test and this branch's two per-driver copies into one table-driven test over every builder that has the precondition: implementer, service-agent, incident-responder, and both issue-investigator builders. The base's stronger form is the one kept: set MCTL_TOKEN so `mcp_servers` is actually populated, assert it is non-empty, and assert none of it is `type: "sdk"`. The two copies added here lacked the setenv, so their "no sdk-type server" assertion ran against an empty dict and passed vacuously — exactly the rot 19f1146 was guarding against. Adds the matching negative, which the audit in #368 turned up and nothing executed: `build_mentor_options` passes an http mctl server but NO hooks, so `sdk_mcp_servers or hooks` is falsy and the mentor cannot be drained at all. Draining it as it stands would convert a silent loss into a guaranteed OrphanedSubagentError on every delegating run. The test exists to stop the #366 pattern being copied there on the assumption that "it has an MCP server, so it qualifies", and to make giving the mentor hooks a decision someone has to take deliberately rather than a refactor that slips through. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
… first Addresses claude[bot]'s review on #368. P3 (run_issue_investigator) — the 429 check guarded only the first result frame. The drain read with a bare `print`, so a limit verdict arriving while a delegated child was still live went unclassified: `_run_agent` returned normally, investigate() failed the triplet check, and the result carried `rate_limited=False`. An operator, or the account-2 fallback that exists for exactly this case, then read "agent produced no triplet" instead of "this account is out of quota". The predicate is now a single `_note` closure used both in the turn loop and as the drain's `on_message`, so it applies wherever a result frame appears. KNOWN RESIDUAL, escalated rather than fixed here: if the child reaches a terminal status FIRST and the 429 lands on the parent's follow-up turn after that, `drain_until_settled` has already returned -- it stops the instant `ledger.live` empties -- so no `on_message` can observe that frame. The reviewer's suggested remedy ("pass an on_message that re-applies the predicate") does not close this ordering either. Closing it requires the shared helper to keep reading to the next result frame, which is orchestrator/subagent_wait.py in #367 and is depended on by the implementer, so it is not changed from this branch. The test names the residual. P2 (tests) — the driver-level test the finding points at was already deleted in e001ab6, which replaced it with a table-driven test in tests/test_options.py covering all five builders including `build_issue_investigator_options_from_plan`. The finding's SECOND half is real and is fixed here: the legacy/declarative equivalence test omitted `hooks`, even though the builder's own docstring lists it among the fields "the equivalence tests in tests/test_options.py assert this directly". One line, and it makes that docstring true. P3 (run_service_agent) — two blank lines after the top-level class, per PEP 8. P3 (tests) — dropped the mid-file import and its `noqa: E402`: the module is already imported at the top, so `run_issue_investigator.InvestigatorOrphanedSubagent` works, and the SDK message class moved into the top import block. Also took the reviewer's second point and moved the duplicated `task_started`/`task_updated`/ `task_notification`/`result` builders into tests/conftest.py beside FakeMcpClient, so a future SDK field addition is one edit. Only this branch's two files are switched over; #367's copies are left for its author. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
edfe8c4 to
c97b422
Compare
Addresses claude[bot]'s P3 on #368. The driver tests monkeypatch the module attribute, so the VALUE is exercised but the `os.getenv` string never is. A typo there would be silent in the worst way: the knob documents itself as tunable while doing nothing, and you find out when you raise it during an incident and nothing changes. Mirrors the implementer twin's existing default + env-override pair; verified by misspelling one getenv name and watching the parametrised case go red. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
| # setting_sources=["project"] loads whatever `.claude/agents/*.md` that | ||
| # repository ships. An orphan here writes no requirements/design/tasks triplet | ||
| # at all, so the whole downstream pipeline gets nothing. | ||
| ISSUE_INVESTIGATOR_DRAIN_TIMEOUT_SECONDS = float( |
There was a problem hiding this comment.
P3 — both new sub-deadlines are nested inside a caller wall-clock bound that does not exist.
drain_until_settled states its own precondition: timeout_s "is a sub-deadline nested inside the caller's own wall-clock bound. It is not needed for liveness -- the outer bound already guarantees that" (subagent_wait.py). That holds for the implementer, whose drain sits inside anyio.fail_after(IMPLEMENTER_TIMEOUT_SECONDS) (run_implementer.py:705). It does not hold for either new caller: run_service_agent and _run_agent both open ClaudeSDKClient with no fail_after at all, and validate_manifest.py:124-129 confirms it — _TIMEOUT_CONSTANT_BY_AGENT lists only the implementer because "the other five don't bound wall-clock time in options.py".
Not a hang: move_on_after(timeout_s) bounds the drain loop itself, so liveness survives. But the consequences are worth naming: (1) the 300s is now purely additive to a run that has no in-process ceiling, so a wedged child extends every investigation/service-agent run by up to five minutes before anything notices; (2) the post-drain stream.aclose() and ClaudeSDKClient.__aexit__ on the orphan path are outside any deadline here, where the implementer's are inside one.
Cheapest close is a one-line note on these constants saying the outer bound is the Argo/Temporal deadline, not an in-process one — otherwise the next reader who follows the helper docstring will look for a fail_after that is not there.
Antigravity (agy) reviewModel: gemini-3.1-pro-high · commit An analysis of the pull request reveals a missing callback that leads to swallowed sub-agent logs during the drain phase in the service agent driver. Severity: P2 Concrete failure scenario: Proposed fix: await drain_until_settled(
stream, ledger, timeout_s=SERVICE_AGENT_DRAIN_TIMEOUT_SECONDS,
on_message=print
) |
…agent drivers A `ResultMessage` ends one TURN, not the RUN, and `ClaudeSDKClient.receive_response()` returns at the first one by contract. A driver that reads it and stops therefore abandons any sub-agent the CLI launched asynchronously (`isAsync: True`, `status: "async_launched"`) and silently throws its work away. That is mctl-agents#366, fixed for the implementer in #367; the same un-inspected loop shape was latent in every other SDK driver in `orchestrator/`. Draining only works where the SDK actually keeps the CLI subprocess alive past the result frame, which it does while `_inflight_tasks` is non-empty but ONLY when `sdk_mcp_servers or hooks` is truthy. Every mctl MCP server this repo configures is `type: "http"`, so `sdk_mcp_servers` is empty everywhere and `hooks` is the whole precondition. Audited per driver: - run_issue_investigator hooks present -> converted (highest value: an orphan here writes no requirements/design/tasks triplet at all, so the whole downstream pipeline gets nothing). `cwd` is a fresh clone of the target repository and `setting_sources=["project"]` loads whatever `.claude/agents/*.md` that repository ships, so the delegation is real. - run_service_agent hooks present -> converted. Its prompt walks four steps named after the `.claude/agents/{researcher,analyst,spec-writer}.md` personas loaded from the agent's own cwd. - run_incident_responder hooks present -> deliberately NOT converted. The precondition holds, but `cwd=agents/_incident-responder/` ships no `.claude/agents/` at all and the prompt never delegates, so there is no child to orphan. Converting would be churn, not coverage. - run_mentor NO hooks, no SDK-MCP -> CANNOT be fixed by draining. `build_mentor_options` passes neither, so the SDK closes stdin at the first result and the CLI exits: draining would wait on a child nobody is keeping alive. Giving it the precondition is a separate decision, not this change. - run_shepherd out of scope: it uses the one-shot `query()` generator with no client handle, and its failure mode is already contained (empty text -> `_fallback_bundle`, degraded rather than lost). Each converted driver gets its own failure mapping rather than a borrowed exit code. The investigator reports through `InvestigateResult.error`, so an orphan is caught explicitly ahead of the generic branch and worded as a harness failure instead of reading as "the agent broke on this issue"; `rate_limited` stays the only flag callers branch on, and a 429 result still wins over the drain. The service agent raises, which `run_all._safe_run_service` already catches, logs and drops without tearing down the sibling agents in its task group. `InvestigatorOrphanedSubagent` deliberately does not subclass the shared `OrphanedSubagentError`: `orchestrator/subagent_wait.py` imports the SDK at module scope and `run_issue_investigator` must stay importable by the long-lived Temporal worker (#149). Each driver gets a test that fails without the drain -- asserting the stream was consumed PAST the `ResultMessage`, not merely that the call returned -- plus a test pinning the `hooks` precondition its correctness rests on. Refs #366. Stacked on #367. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
Resolves the overlap between this branch and 19f1146 on the base. Both added a test asserting the same fact — that `hooks`, not the mctl MCP config, is what makes the SDK hold stdin open past a result frame — and two near-duplicates of a single SDK fact drift apart. Folds 19f1146's implementer test and this branch's two per-driver copies into one table-driven test over every builder that has the precondition: implementer, service-agent, incident-responder, and both issue-investigator builders. The base's stronger form is the one kept: set MCTL_TOKEN so `mcp_servers` is actually populated, assert it is non-empty, and assert none of it is `type: "sdk"`. The two copies added here lacked the setenv, so their "no sdk-type server" assertion ran against an empty dict and passed vacuously — exactly the rot 19f1146 was guarding against. Adds the matching negative, which the audit in #368 turned up and nothing executed: `build_mentor_options` passes an http mctl server but NO hooks, so `sdk_mcp_servers or hooks` is falsy and the mentor cannot be drained at all. Draining it as it stands would convert a silent loss into a guaranteed OrphanedSubagentError on every delegating run. The test exists to stop the #366 pattern being copied there on the assumption that "it has an MCP server, so it qualifies", and to make giving the mentor hooks a decision someone has to take deliberately rather than a refactor that slips through. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
a4c4860 made orchestrator/subagent_wait.py importable by the Temporal worker by deferring its own claude_agent_sdk imports. That removes the constraint this branch was working around, so collect the payoff here rather than leaving a workaround whose reason no longer exists. - `InvestigatorOrphanedSubagent` is now a real subclass of the shared `OrphanedSubagentError`, so `except OrphanedSubagentError` catches it like any other driver's. It kept its own name for the greppable `Investigator*` convention, exactly as `ImplementerOrphanedSubagent` does. - `subagent_wait` is imported at module scope instead of inside `_run_agent`. Verified the module still does NOT pull in the SDK: `import orchestrator.run_issue_investigator` leaves `claude_agent_sdk` out of sys.modules, and tests/test_worker_isolation.py (all three, including a4c4860's positive assertion) passes with both converted drivers in place. - `INVESTIGATOR_DRAIN_TIMEOUT_SECONDS` becomes `ISSUE_INVESTIGATOR_DRAIN_TIMEOUT_SECONDS` in orchestrator/options.py, beside `IMPLEMENTER_DRAIN_TIMEOUT_SECONDS` and the service agent's, so the three drain sub-deadlines are read and changed as one family. It only lived in the driver because a function-local import would have been unreachable from the tests that shorten it. - Both new env vars are registered in `_ENV_VARS_AFFECTING_OPTIONS_DEFAULTS` (orchestrator/validate_manifest.py), so manifest validation clears them before reading the real constants and cannot be fooled by an operator's environment, the same way the implementer's already is. The catch-and-re-raise in `_run_agent` stays: `drain_until_settled` raises the shared error by contract, so re-raising is the only way a driver gets its own type, and run_implementer does exactly this. What went away is the reason it was NOT a subclass, not the re-raise itself. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
… first Addresses claude[bot]'s review on #368. P3 (run_issue_investigator) — the 429 check guarded only the first result frame. The drain read with a bare `print`, so a limit verdict arriving while a delegated child was still live went unclassified: `_run_agent` returned normally, investigate() failed the triplet check, and the result carried `rate_limited=False`. An operator, or the account-2 fallback that exists for exactly this case, then read "agent produced no triplet" instead of "this account is out of quota". The predicate is now a single `_note` closure used both in the turn loop and as the drain's `on_message`, so it applies wherever a result frame appears. KNOWN RESIDUAL, escalated rather than fixed here: if the child reaches a terminal status FIRST and the 429 lands on the parent's follow-up turn after that, `drain_until_settled` has already returned -- it stops the instant `ledger.live` empties -- so no `on_message` can observe that frame. The reviewer's suggested remedy ("pass an on_message that re-applies the predicate") does not close this ordering either. Closing it requires the shared helper to keep reading to the next result frame, which is orchestrator/subagent_wait.py in #367 and is depended on by the implementer, so it is not changed from this branch. The test names the residual. P2 (tests) — the driver-level test the finding points at was already deleted in e001ab6, which replaced it with a table-driven test in tests/test_options.py covering all five builders including `build_issue_investigator_options_from_plan`. The finding's SECOND half is real and is fixed here: the legacy/declarative equivalence test omitted `hooks`, even though the builder's own docstring lists it among the fields "the equivalence tests in tests/test_options.py assert this directly". One line, and it makes that docstring true. P3 (run_service_agent) — two blank lines after the top-level class, per PEP 8. P3 (tests) — dropped the mid-file import and its `noqa: E402`: the module is already imported at the top, so `run_issue_investigator.InvestigatorOrphanedSubagent` works, and the SDK message class moved into the top import block. Also took the reviewer's second point and moved the duplicated `task_started`/`task_updated`/ `task_notification`/`result` builders into tests/conftest.py beside FakeMcpClient, so a future SDK field addition is one edit. Only this branch's two files are switched over; #367's copies are left for its author. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
Addresses claude[bot]'s P3 on #368. The driver tests monkeypatch the module attribute, so the VALUE is exercised but the `os.getenv` string never is. A typo there would be silent in the worst way: the knob documents itself as tunable while doing nothing, and you find out when you raise it during an incident and nothing changes. Mirrors the implementer twin's existing default + env-override pair; verified by misspelling one getenv name and watching the parametrised case go red. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
9133228 moved non-positive-timeout validation out of drain_until_settled and into a `_positive_seconds` clamp here, because raising from the helper only surfaced once a sub-agent was actually launched, got swallowed into a generic exit, and landed in the shepherd's transient arm -- which has no counter, so a typo in one env var produced an unbounded paid retry loop. The two knobs this branch added still read through a bare `float(os.getenv(...))`, which is the same defect under a different name. Both now use the clamp. The blast radius genuinely differs -- service-agent and issue-investigator do not go through the shepherd's classification, so a 0 here orphans runs rather than looping forever -- but the shape is right either way, and a knob that documents itself as tunable while silently doing nothing is bad in every mode. Parametrised the clamp and env-name tests over all three knobs instead of naming the implementer's alone, so the property is stated once for every caller of the helper and a fourth knob added with a bare getenv fails rather than reintroducing the pathology. Verified by reverting one knob to the bare form and watching only that parametrised case go red. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
The merged `drain_until_settled` is a loop whose stop condition is the SDK's own -- a ResultMessage arriving with nothing in flight -- rather than the ledger emptying. Two consequences for these drivers, both now pinned: - The residual this branch documented is CLOSED. A 429 landing on the parent's follow-up turn, AFTER the child settled, used to be unobservable because the drain had already returned; the loop now reads into the turn that settling woke, which is where that frame lands, and `_note` classifies it. Replaced the docstring claiming the residual with a test asserting the opposite, so a revert of the stop condition fails here rather than silently costing the account-2 fallback again. - A second delegation is now awaited like the first. For the service agent that is the NORMAL path, not an edge case: its prompt runs three named personas in sequence. Pinned at driver level because the helper's own tests cannot see the break-then-drain wiring -- it is possible to consume the relaunch in the turn loop and never enter the drain. Driver loop shape is unchanged and still mirrors run_implementer's. Neither driver has an outer `fail_after`, so phase 2's `current_effective_deadline()` clamp is inert for them (infinite remaining budget, no clamping) -- correct by construction rather than by accident, and noted here because it is the one part of the merged helper their tests do not exercise. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
c97b422 to
417942d
Compare
|
Claude finished @mashkovd's task in 5m 21s —— View job Review complete — changes requested (0 P1, 1 P2, 3 P3)Pinned to
P2 — the "raise rather than swallow" decision rests on a guard only one of two paths has
await run_service_agent(service) # run_all.py:133 — no guardand There an orphan turns a silent exit-0 partial into a non-zero exit, so the workflow's commit-and-push step never runs and the parent's already-written P3
What holds upChecked against source, not the description:
One earlier automated finding I could not reproduceThe claim that Not verified
|
Antigravity (agy) reviewModel: gemini-3.1-pro-high · commit Severity: P3 await drain_until_settled(
stream, ledger, timeout_s=SERVICE_AGENT_DRAIN_TIMEOUT_SECONDS,
on_message=print
) |
| `ResultMessage` throws the child's inbox entry or proposal away silently. | ||
| mctl-agents#366. | ||
|
|
||
| Raised rather than swallowed so the loss is visible: `run_all._safe_run_service` |
There was a problem hiding this comment.
P2 (repeat, still open at 417942d) — this docstring rests the whole "raise rather than swallow" decision on a guard that only one of the two run_all paths into this driver has.
_safe_run_service catches it (run_all.py:49), but that guard is on the RUN_MODE=full task group only. The other caller takes the driver bare:
async def _single_service(service: str) -> None:
...
await run_service_agent(service) # run_all.py:133 — no guardand main() runs under a bare anyio.run(main). Same for the python -m orchestrator.run_service_agent mctl-web entrypoint this module documents on line 4 — its main() has no handler either. The path is operator-reachable: docs/diagrams/archify/facts.yaml declares mctl-agents-single-service as an operation with an mctl_trigger_single_service tool.
Failure scenario: an operator triggers single-service for mctl-web; the CLI launches the researcher persona asynchronously; the child never reports a terminal status within SERVICE_AGENT_DRAIN_TIMEOUT_SECONDS; ServiceAgentOrphanedSubagent propagates out of main() and the process exits non-zero. The workflow’s commit-and-push step then does not run, so the inbox entry and any proposal the parent already wrote are discarded — where pre-PR that same run exited 0 and those files were committed (only the child’s work was silently lost). That is strictly more loss on this path, and it is the exact consequence _safe_run_service’s own docstring names.
Two defensible resolutions, but it should be a decision rather than a side effect:
- catch
ServiceAgentOrphanedSubagentin_single_servicetoo (log-and-drop, matching full mode), so on-disk work still gets committed; or - keep the non-zero exit for single-service deliberately and narrow this docstring, which today reads as an unconditional guarantee.
Either way it wants the coverage the full-mode path already has — test_orphan_does_not_tear_down_the_sibling_agents exercises _safe_run_service only, and nothing exercises _single_service with an orphan.
There was a problem hiding this comment.
You were right, and guarding is the fix — not narrowing the docstring. Fixed in 0eb1450.
I verified the claim rather than taking it: run_all._safe_run_service covers only the RUN_MODE=full task group; _single_service (run_all.py:133) and main() both called the driver bare under anyio.run. On those paths the raise converted a silent exit-0 partial into a non-zero exit, so the commit-and-push step never ran and the parent's already-written inbox entry and proposals went with it — strictly more loss than before this PR, which is the inversion of what #366 is for.
New run_service_agent_tolerating_orphans logs loudly and returns so the commit step runs, used by both previously-bare entrypoints. Same trade and rationale as _safe_run_service. Deliberately orphan-only rather than except Exception: other failures may mean zero real work happened, and swallowing those is the false green _safe_run_incident_responder's McpNotConnectedError branch exists to prevent. The orphan is the one mode where "something on disk is worth committing" is guaranteed.
Four tests, since a docstring-only fix with no test is how this came back as a repeat: the wrapper, the single-service path end to end, the orphan-only narrowness, and that main() goes through the guard.
Worth recording: the single-service test also stubs run_all's own run_service_agent binding with a tripwire. Without that, a regression to the bare call reaches a real ClaudeSDKClient and the test HANGS on a subprocess rather than failing. I hit exactly that while verifying the test catches the regression — so the guard now fails in 0.3s instead of hanging, which is the difference between a test that protects this and one nobody trusts.
| IMPLEMENTER_DRAIN_TIMEOUT_SECONDS = _positive_seconds( | ||
| "IMPLEMENTER_DRAIN_TIMEOUT_SECONDS", default=300.0 | ||
| ) | ||
| # The same sub-deadline for the other two drivers that drain (mctl-agents#368), |
There was a problem hiding this comment.
P3 — "the same sub-deadline" is the one thing these two are not: neither driver has the outer wall-clock bound the helper's contract assumes, and with the merged looping drain the total wait is now unbounded in a way the implementer's is not.
drain_until_settled states its premise explicitly: "timeout_s is a sub-deadline nested inside the caller's own wall-clock bound. It is not needed for liveness -- the outer bound already guarantees that -- but for classification." True for the implementer, which wraps everything in anyio.fail_after(IMPLEMENTER_TIMEOUT_SECONDS). Not true here: grep -n "fail_after\|move_on_after" orchestrator/run_service_agent.py orchestrator/run_issue_investigator.py is empty (the investigator is a bare anyio.run(_run_agent, ...) at run_issue_investigator.py:1538), and validate_manifest._TIMEOUT_CONSTANT_BY_AGENT says so in as many words — "Only implementer has one today ... the other five don't bound wall-clock time in options.py".
Three consequences worth stating here rather than leaving a reader to re-derive:
- For these two, 300s is the liveness guarantee for the drain, not a classification refinement inside a larger one.
- The merged helper restarts the clock on every relaunch ("WAIT FOR THE CLOSING FRAME, on a RESTARTED clock"), and
test_service_agent_awaits_a_second_delegationpins that the service agent's normal path delegates repeatedly. With no outer bound the in-process ceiling is N x 300s, not 300s — a run that keeps delegating is bounded only by the Argo/pod deadline.417942d's commit message notes the clamp is inert for these drivers but does not draw this consequence. test_the_other_two_drain_timeouts_default_to_five_minutestherefore cannot assert the nesting invariant its implementer twin does (IMPLEMENTER_DRAIN_TIMEOUT_SECONDS < IMPLEMENTER_TIMEOUT_SECONDS). As written that omission reads as an oversight rather than a fact about these two modes.
One sentence in this comment block — "unlike the implementer's, this is the only in-process bound on the wait; these drivers have no outer fail_after, and the clock restarts per delegation" — covers all three.
There was a problem hiding this comment.
Fixed in 0eb1450. The comment no longer claims parity with the implementer's knob, and now says what is actually true: neither driver has an outer fail_after, the merged helper restarts the clock on every relaunch, so the in-process ceiling is N x 300s for N delegations with nothing to trip first. The real bound is external — the Argo step deadline for issue-investigator, the CronWorkflow limit for service-agent — which is why it is acceptable rather than a leak, but it also means shortening these knobs does not shorten the worst case, and phase 2's current_effective_deadline() clamp is inert for both. All of that is in the comment now rather than in my head.
| ) | ||
| except SystemExit as e: | ||
| return InvestigateResult(service, slug, proposal_dir, error=f"SystemExit: {e}") | ||
| except InvestigatorOrphanedSubagent as e: |
There was a problem hiding this comment.
P3 — this branch can discard an already-complete triplet, and the tradeoff is stated nowhere.
_run_agent raises at step 3, so steps 4a-7 (_verify_staging, the triplet check, the publish) never run, and investigate()'s finally shutil.rmtree(staging_wrapper, ...) removes whatever the parent already wrote. The comment above ISSUE_INVESTIGATOR_DRAIN_TIMEOUT_SECONDS asserts the premise as fact — "An orphan here writes no requirements/design/tasks triplet at all" — but that is an assumption about which child was live, not a guarantee: the delegated agent comes from whatever .claude/agents/*.md the target repo ships, and can perfectly well be a supplementary reviewer launched after the parent had written all three documents.
Failure scenario: the parent writes requirements/design/tasks into staging, delegates a supplementary sub-agent, ends its turn; the CLI then exits with that child still live. drain_until_settled raises on stream exhaustion, InvestigatorOrphanedSubagent propagates, staging is rmtree'd, investigate() returns an error and the run publishes nothing. Pre-PR that same run published a usable proposal.
The stream-exhaustion sub-case is the one where the justification is weakest: the CLI is already gone, so the child is provably dead and staging is quiescent — the "something may still be writing into staging" reasoning that makes discarding correct on the deadline path does not apply there.
Defensible as written (a named harness failure beats a silently-incomplete proposal, and the cost is one re-run), hence P3 — but the reasoning belongs next to this handler, because the options.py premise reads as a fact rather than a choice.
There was a problem hiding this comment.
Fixed in 0eb1450. The orphan branch now states the trade instead of leaving it implicit: the finally rmtrees a staging directory that may hold a complete, perfectly good triplet, and we discard it anyway. The reasoning is that staging exists so a run's output is adjudicated as a whole (mctl-agents#246) — an orphan means part of the intended work never happened, so what is on disk is a coherent-looking document silently missing whatever the child was doing. Publishing that into the pipeline unmarked is worse than re-investigating: the issue stays proposed and re-running is idempotent, so discarding costs one more run while publishing costs a plausible proposal nobody knows is partial. Noted that it should be revisited only with a way to record the gap in the artifact itself.
The options.py premise is reworded too — it said an orphan writes no triplet at all, as though it were physics, when it is this choice.
| ) | ||
|
|
||
|
|
||
| def result_message(*, is_error=False, api_error_status=None, subtype="success"): |
There was a problem hiding this comment.
P3 — result_message is the one builder of the four that did not actually replace its copies, in the same PR that made the de-duplication argument.
tests/test_run_service_agent.py imports it. tests/test_run_issue_investigator.py does not: its import list takes only task_notification_message, task_started_message and task_updated_message, and the local _result_message at line 382 stays — so the new _ok_result() helper (line 3220) and all three 429 frames (lines 3327, 3383, 3419) still go through the duplicate.
Not a behaviour problem: both constructors pass the same fields. But the stated reason for moving these here is "a future SDK field addition is one edit rather than one per driver", and for the result frame specifically it is still two. The investigator's local builder is typed and keyword-only where this one is not, so if that signature is the reason to keep it, the cheaper fix is to drop result_message from conftest until a second caller actually needs it.
Related, same half-done shape: tests/test_run_service_agent.py:_StreamingClient and tests/test_run_issue_investigator.py:_FakeClient are now the same fake with the same four methods (the latter extended in this PR to accept a callable). The builders moved here; the client that yields them did not.
There was a problem hiding this comment.
Fixed in 0eb1450 — you were right that it was half-done. _result_message is gone in favour of conftest's result_message, and both local client fakes (_FakeClient in the investigator tests, _StreamingClient in the service-agent tests) are gone in favour of FakeMcpClient, which now accepts a callable message factory as well as a sequence. That was the actual reason two copies existed: the drain tests need a stream that can block, record what was consumed, or relaunch a task mid-drain, and the shared fake could only replay a list. One fake now serves both drivers, which is what makes a future SDK field addition one edit rather than four.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 3 P3. Pinned to 417942d.
P2 (repeat, still open): ServiceAgentOrphanedSubagent's docstring rests the "raise rather than swallow" decision on run_all._safe_run_service, but that guard is only on the RUN_MODE=full task group. _single_service (run_all.py:133) and the python -m orchestrator.run_service_agent <svc> entrypoint this module documents both call the driver bare under anyio.run, and mctl-agents-single-service is a declared operator-triggered operation. On that path an orphan turns a silent exit-0 partial into a non-zero exit, so the workflow's commit step never runs and the parent's already-written inbox entry and proposal are dropped — strictly more loss than pre-PR. Either guard it or narrow the docstring, and give it the test the full-mode path already has.
P3s: the options.py comment calls these "the same sub-deadline" as the implementer's, but neither driver has an outer fail_after, and the merged helper restarts the clock per relaunch, so the in-process ceiling is N x 300s with nothing to trip first; the investigator's orphan branch lets investigate()'s finally rmtree an already-complete triplet without stating the tradeoff, and the options.py premise ("an orphan here writes no triplet at all") reads as fact rather than choice; result_message was added to conftest but test_run_issue_investigator.py still uses its local _result_message, and _StreamingClient still duplicates _FakeClient.
What holds up, verified against source rather than the description. hooks=_command_audit_hooks() is on the service-agent, implementer, incident-responder and both investigator builders and absent from build_mentor_options/build_shepherd_options; every mctl server is type "http" and the SDK lifts only type "sdk" into sdk_mcp_servers, so hooks really is the whole precondition — converting two drivers, skipping the responder, and refusing to drain the mentor are all correct, and the table-driven test plus the executable mentor negative pin it. Both drains mirror #367's implementer shape (one generator, aclosing, drain then all_completed warn). The investigator's _note is used both in the turn loop and as the drain's on_message, so the 429 verdict applies to every result frame; RateLimitExhaustedError is neither a cancellation exception nor an OrphanedSubagentError, so it survives move_on_after and the wrapping except and reaches investigate() as rate_limited=True. The follow-up-turn ordering that used to be a documented residual is genuinely closed by the merged looping stop condition and is now pinned by a test. investigate()'s new except sits ahead of the generic branch and all rollback stays in the finally. All three drain knobs share _positive_seconds, are parametrised over both the clamp and env-name tests, and are registered in _ENV_VARS_AFFECTING_OPTIONS_DEFAULTS. Worker isolation holds: subagent_wait defers its own SDK imports, so the new module-scope import in run_issue_investigator is safe. Both headline tests assert consumption past the ResultMessage, and the second-delegation test covers the relaunch path the driver-level wiring could otherwise skip.
One earlier automated finding I checked and could not reproduce: run_service_agent's drain does NOT swallow messages. drain_until_settled's on_message defaults to print (subagent_wait.py), so omitting it there is equivalent to passing print — no logging is lost, and the argument is needed in the investigator only because _note also carries the 429 predicate. No change wanted on that.
Not verified: pytest, ruff and mypy could not be run here (the interpreter invocations were denied by the tool permission policy), so the reported 1048 passed / 8 skipped and clean lint and type runs are taken as stated. All findings come from reading the pinned diff and surrounding source.
…rded paths Addresses claude[bot]'s repeat P2 on #368, as a behavioural fix rather than a docstring narrowing. `run_all._safe_run_service` guards only the RUN_MODE=full task group. `_single_service` -- reached by the declared `mctl-agents-single-service` operator operation -- and `main()`'s `python -m orchestrator.run_service_agent <svc>`, which this module's own docstring documents, both called the driver bare under `anyio.run`. On those paths this branch's raise turned a silent exit-0 partial into a non-zero exit, so the workflow's commit-and-push step never ran and the parent's ALREADY-WRITTEN inbox entry and proposals were dropped: strictly more loss than the pre-#366 behaviour, which is the exact inversion of what #366 is for. Guarded rather than documented-away, because the docstring-only option leaves a known regression in a declared operator operation, and "the raise is fine, the caller handles it" was already the claim that turned out not to hold. `run_service_agent_tolerating_orphans` logs loudly and returns, so the commit step runs -- the same trade and the same rationale as `_safe_run_service` ("partial success still produces a useful digest"). Deliberately narrow: ONLY the orphan, not `except Exception`. Every other failure may mean zero real work happened, and swallowing those would report a false green to Argo for a run that produced nothing -- the bug `_safe_run_incident_responder`'s McpNotConnectedError branch exists to prevent. The orphan is the one failure mode where "there is something on disk worth committing" is guaranteed rather than hoped for. Tests cover the wrapper, the single-service path end to end, the orphan-only narrowness, and that `main()` goes through the guard. The single-service test stubs `run_all`'s own `run_service_agent` binding with a tripwire: without it a regression to the bare call reaches a real ClaudeSDKClient and the test HANGS on a subprocess instead of failing. Found that the hard way -- the first version did exactly that -- so the regression now fails in 0.3s. Also from the same review: - options.py no longer calls these knobs "the same sub-deadline" as the implementer's. Neither driver has an outer `fail_after`, and the merged helper restarts the clock per relaunch, so the in-process ceiling is N x 300s with nothing to trip first; the real bound is external (Argo step deadline, CronWorkflow limit) and phase 2's `current_effective_deadline()` clamp is inert for both. The comment now says so. - The investigator's orphan branch states the trade it makes: the `finally` rmtrees a possibly-complete triplet, and that is a choice -- publishing a proposal silently missing the child's contribution is worse than re-investigating, since the issue stays `proposed` and re-running is idempotent. The options.py premise no longer reads as physics. - Test fakes deduplicated: `_result_message` and `_FakeClient`/`_StreamingClient` are gone in favour of conftest's `result_message` and `FakeMcpClient`, which now accepts a callable message factory so one fake serves both drivers. Refs #366. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1
|
Claude finished @mashkovd's task in 5m 29s —— View job Review complete — approved (0 P1, 0 P2, 3 P3)Pinned to
The standing P2 is closed
What holds up, verified against source
P3 findings
Not verified
|
Antigravity (agy) reviewModel: gemini-3.1-pro-high · commit An independent review of the provided diff has identified one minor issue where logging would be silently lost during the drain phase of the service agent. Severity: P3 await drain_until_settled(
stream, ledger, timeout_s=SERVICE_AGENT_DRAIN_TIMEOUT_SECONDS, on_message=print
) |
|
|
||
| Raised rather than returned so the loss is visible in the log — but it must | ||
| never be allowed to reach the process exit code, which is why every | ||
| entrypoint goes through `run_service_agent_tolerating_orphans` below. |
There was a problem hiding this comment.
P3 — "every entrypoint goes through run_service_agent_tolerating_orphans below" is not true, and it is the kind of invariant a reader will act on.
Two counter-examples in the tree as of this commit:
run_all._safe_run_service(run_all.py:52) still awaits the barerun_service_agent. Behaviour there is fine — itsexcept (Exception, SystemExit)catches the orphan and logswarn: service-agent <svc> failed: ServiceAgentOrphanedSubagent: ...— but the containment comes from that broad catch, not from this wrapper.- The declared entrypoint in
agents/_manifests/service-agent/agent.yaml:12anddocs/agent-inventory.yaml:209isorchestrator.run_service_agent:run_service_agent, i.e. the bare driver. That binding is validation-only today (manifest.resolve_entrypointis called fromvalidate_manifestand nowhere else —grep -rn resolve_entrypointconfirms), so nothing dispatches through it at runtime and there is no live unguarded path. But it is the thing a reader looking for "the entrypoint" finds first.
The accurate statement is the one run_service_agent_tolerating_orphans's own docstring already makes: full mode is covered by _safe_run_service's broad catch, and this wrapper covers the two paths that guard did not. Worth making this sentence agree with it, since a future caller that trusts the stronger claim gets no guard at all.
| a false green to Argo for a run that produced nothing, which is the bug | ||
| `_safe_run_incident_responder`'s McpNotConnectedError branch exists to | ||
| prevent. The orphan is the one failure mode where "there is something on | ||
| disk worth committing" is guaranteed rather than hoped for. |
There was a problem hiding this comment.
P3 — "guaranteed rather than hoped for" is the one claim here that does not hold, and it is what the whole swallow-vs-raise decision is rested on.
The docstring reasons: only the orphan is caught, because unlike every other failure it is guaranteed to leave something on disk worth committing. But the delegated child is the work in this driver. PROMPT step 1 is the researcher persona; if the CLI launches that first delegation asynchronously and the child never settles, the parent has written nothing yet — no inbox entry, no proposal. The wrapper then logs to stderr, returns, main()/_single_service exit 0, and Argo sees a green step for a run that produced zero artifacts. That is the same "false green for a run that produced nothing" the paragraph cites _safe_run_incident_responder's McpNotConnectedError branch to rule out, arriving through the branch that was just added.
Not a regression and not a blocker: pre-#366 that identical run also exited 0, silently, so this commit strictly improves it (the loss is now loud in stderr). And the commit-step argument for not raising is sound on the partial case. What is wrong is the strength of the justification — "guaranteed" should be "the one failure mode where on-disk work is likely and where raising would demonstrably destroy it".
If you want the distinction to survive into the log rather than only into the docstring, the cheap version is to say in the warn line whether anything was written (agents/<service>/inbox, proposals/), so an operator reading a green single-service run can tell "partial" from "nothing at all".
Related, for the same paragraph: on the deadline sub-case the child is still live when this returns and ClaudeSDKClient.__aexit__ tears the CLI down, so a file it was mid-write into can be committed truncated. The investigator's orphan branch discards staging for exactly that reason; the service agent writes into the real agent directory and has no staging to discard. Also not a regression — the pre-PR driver killed the CLI sooner — but "only the delegated child's contribution is lost" is a bit cleaner than reality.
|
|
||
| source = inspect.getsource(rsa.main) | ||
| assert "run_service_agent_tolerating_orphans" in source | ||
| assert "anyio.run(run_service_agent," not in source |
There was a problem hiding this comment.
P3 — this is a source-text assertion standing in for a behavioural one, and the negative half is narrower than it looks.
assert "anyio.run(run_service_agent," not in source only catches a regression that reproduces the old line verbatim. anyio.run(run_service_agent, service) written with any other spacing, or await run_service_agent(service) inside an anyio.run-ed wrapper, or anyio.run(functools.partial(run_service_agent, service)), all pass — while the positive half also still passes as long as the string appears anywhere in the function, including in a comment.
The behavioural version is short and exercises the wiring the same way test_single_service_mode_does_not_propagate_an_orphan does for the other path:
def test_the_module_entrypoint_goes_through_the_guard(monkeypatch, capsys):
async def _orphan(service):
raise rsa.ServiceAgentOrphanedSubagent("orphaned sub-agent: t1 still live")
monkeypatch.setattr(rsa, "run_service_agent", _orphan)
monkeypatch.setattr(rsa, "ensure_auth_for_sdk", lambda: None)
monkeypatch.setattr(sys, "argv", ["run_service_agent", "mctl-agent"])
rsa.main() # must not raise, must not SystemExit
assert "warn: service-agent mctl-agent" in capsys.readouterr().errThat one goes red for every regression shape, not just the one that was there before.
There was a problem hiding this comment.
No P1/P2 findings (3 P3). Good to merge.
The P2 that stood open across the previous four reviews is closed by 0eb1450. run_service_agent_tolerating_orphans now covers both paths _safe_run_service did not — _single_service (run_all.py:141) and main() (run_service_agent.py:190) — and I checked the wiring rather than the description: the wrapper resolves run_service_agent through the module global, so test_single_service_mode_does_not_propagate_an_orphan patching rsa.run_service_agent exercises the real _single_service -> wrapper -> driver chain, and the tripwire on run_all.run_service_agent means a regression to the bare call fails rather than hangs on a real subprocess. Narrowing the catch to ServiceAgentOrphanedSubagent alone is the right call and is pinned by the parametrised non-orphan test; SystemExit for a missing agent directory still propagates, as before.
Verified against source, not the PR body:
- The precondition audit holds.
hooks=_command_audit_hooks()is on the service-agent, implementer, incident-responder and both investigator builders and absent frombuild_mentor_options/build_shepherd_options;mctl_mcp_config()emitstype: "http"and the SDK lifts onlytype: "sdk"intosdk_mcp_servers, sohooksis the whole precondition. The table-driven test over all five drainable builders,assert declarative.hooks == legacy.hooks, and the executable mentor negative pin it. - Worker isolation survives the module-scope
subagent_waitimport: that module defers its ownclaude_agent_sdkimports intoobserve()/_settle()/drain_until_settled, so nothing SDK-shaped is pulled in at import time. - The 429 verdict applies to every result frame.
_noteis the turn loop's inspector and the drain'son_message;RateLimitExhaustedErroris neither a cancellation exception nor anOrphanedSubagentError, so it survivesmove_on_afterand the wrappingexceptand reachesinvestigate()asrate_limited=True. Both orderings now have tests, including the follow-up-turn one that #367's merged stop condition made reachable. investigate()'s orphan branch sits ahead of the genericExceptionbranch and behind the_StagingReplaced/_ProposalAdvanced/SystemExitones; nothing on the_run_agentroute swallows aRuntimeErrorfirst, and all staging/clone/proposal rollback stays in thefinally. The discard-a-possibly-complete-triplet trade a prior review asked to have written down is now stated at the handler.- The options.py comment no longer claims these are the implementer's kind of sub-deadline: it says outright that neither driver has an outer
fail_after, that the clock restarts per relaunch so the in-process ceiling is N x 300s, and that the real bound is external. That matchesvalidate_manifest._TIMEOUT_CONSTANT_BY_AGENTand agrepforfail_afterover both drivers. - All three drain knobs share
_positive_seconds, are parametrised over both the clamp and the env-name tests, and are registered in_ENV_VARS_AFFECTING_OPTIONS_DEFAULTS. - The shared message builders and the client fake are now in
tests/conftest.pyand the local duplicates in both driver test files are gone, closing the half-done de-duplication flagged earlier.FakeMcpClient._iter_messagesaccepting a callable is what lets the headline tests assert consumption past theResultMessagerather than merely that the call returned. - The manifest entrypoint binding (
orchestrator.run_service_agent:run_service_agent) is validation-only —manifest.resolve_entrypointhas no runtime caller — so it is not a third unguarded path, only a naming mismatch.
P3s posted inline: the ServiceAgentOrphanedSubagent docstring claims every entrypoint goes through the wrapper, which _safe_run_service and the declared manifest entrypoint contradict; the wrapper's "there is something on disk worth committing is guaranteed" is an overclaim, since the first delegated persona is the work and an orphan there exits 0 green with nothing written; and test_the_module_entrypoint_goes_through_the_guard asserts on inspect.getsource text rather than behaviour, so it only catches a verbatim regression.
Not verified: uv run pytest / ruff / mypy were denied by the tool permission policy in this environment, so the PR's "1075 passed, 8 skipped, clean lint and types" is taken as stated. All findings come from reading the pinned diff and surrounding source.
Refs #366. Rebased onto
mainnow that #367 has merged (2c72f6c); no longer stacked.run_mentormust NOT get this treatmentIf you are here because you are copying the #366 drain into another driver, check the precondition before you do.
run_mentordoes not have it and cannot be fixed by draining.The SDK only keeps the CLI subprocess alive past a result frame when
sdk_mcp_servers or hooksis truthy (claude_agent_sdk._internal.query.wait_for_result_and_end_input).build_mentor_optionspassesmcp_servers=but no hooks — and an http MCP server does not count:_internal/client.py:141-145lifts a server intosdk_mcp_serversonly when its config saystype: "sdk", whilemctl_mcp_config()emitstype: "http"(options.py:46). So for the mentor that expression is falsy, the SDK closes stdin at the first result, the CLI exits, and the stream ends.Draining it would therefore not rescue anything — it would wait on a child nobody is keeping alive and turn a silent loss into a guaranteed
OrphanedSubagentErroron every delegating run. The intuition to distrust is "it has an MCP server, so it qualifies". It does not.Giving the mentor the audit hooks to make it drainable is a real behaviour change to a mode that does not delegate today. That is a deliberate decision, not a refactor, and it is explicitly out of scope here.
test_the_mentor_has_neither_hooks_nor_an_sdk_mcp_serverintests/test_options.pymakes this executable rather than folklore, and is the thing that must change with it if the decision is ever taken.The class of bug
A
ResultMessageends one turn, not the run.ClaudeSDKClient.receive_response()returns at the first one by contract, so a driver that reads it and stops abandons any sub-agent the CLI launched asynchronously (isAsync: True,status: "async_launched") and silently throws the child's work away. #367 fixed that for the implementer. Every other SDK driver inorchestrator/had the identical un-inspectedasync for message in client.receive_response(): print(message)shape.The precondition, checked per driver before applying anything
Because
sdk_mcp_serversis empty for every driver in this repo (all servers aretype: "http"),hooksis the entire precondition.run_issue_investigatorcwdis a fresh clone of the target repo, andsetting_sources=["project"]loads whatever.claude/agents/*.mdit shipsrun_service_agent.claude/agents/{researcher,analyst,spec-writer}.mdpersonas loaded from cwdrun_incident_responderagents/_incident-responder/ships no.claude/agents/at all, prompt never delegates, no Bashrun_mentorrun_shepherdmcp_servers={}, no hooks ❌run_incident_responderPrecondition holds, so the conversion would be sound — but there is nothing to orphan: no sub-agent definitions are reachable from its cwd and its prompt never delegates. Left as-is rather than adding the machinery gratuitously. If it ever grows a delegating prompt, the fix is a copy of what this PR does to the service agent.
run_shepherd— noted, not touchedIt uses the one-shot
query()generator with no client handle, so it cannot be drained without converting it toClaudeSDKClientfirst, and it lacks the precondition anyway. Its failure mode is also already contained (empty collected text →_try_parse_jsonreturnsNone→_fallback_bundle, i.e. degraded rather than lost). Its prompt does say "Use theshepherdsub-agent" — the cheapest fix there is almost certainly to drop that delegation instruction, since there is no per-service persona to preserve. Deliberately left as a recommendation.Failure mapping — one per driver, no invented exit codes
Investigator reports through
InvestigateResult.error, not exit codes. An orphan is caught explicitly ahead of the genericExceptionbranch and worded as a harness failure, so it does not read as "the agent broke on this issue" when what happened is that the platform discarded the child.rate_limitedstays the only flag any caller branches on. A 429ResultMessagestill raisesRateLimitExhaustedErrorand is checked before the drain — an out-of-quota account has no live child to wait for.Service agent raises.
run_all._safe_run_servicealready catchesException, logswarn: service-agent <svc> failed: …and moves on without tearing down the sibling agents sharing its task group. Covered by a test.InvestigatorOrphanedSubagentis a real subclass of the sharedOrphanedSubagentError(it keeps its own name for the greppableInvestigator*convention, exactly asImplementerOrphanedSubagentdoes), and the drain sub-deadlines for all three draining drivers live together inoptions.py, registered invalidate_manifest._ENV_VARS_AFFECTING_OPTIONS_DEFAULTS.This branch originally carried a workaround here — a non-subclassing local exception and a driver-local timeout constant — because
orchestrator/subagent_wait.pyimported the SDK at module scope, andrun_issue_investigatormust stay importable by the long-lived Temporal worker (refactor(temporal): check the Argo execution boundaries the worker reasons about #149; the worker module-level-imports it andrun_shepherd).a4c4860on the base branch removed that constraint by deferring the helper's own SDK imports, so the workaround is gone rather than shipped. Verified directly:import orchestrator.run_issue_investigatorleavesclaude_agent_sdkout ofsys.modules, and all threetests/test_worker_isolation.pytests pass with both converted drivers in place.Tests
Each converted driver gets a headline test that fails without the drain — the assertion is that the stream was consumed past the
ResultMessage(aTaskUpdatedMessagereached the consumer), not merely that the call returned. Verified by neutering the drain in place and watching both go red.Plus, per driver: no-live-tasks fast path, deadline orphan, stream-exhaustion orphan, and failed-child-is-quiescent-not-orphaned. The investigator additionally covers 429-beats-drain and the
investigate()error mapping; the service agent covers therun_alltask-group containment.The stdin-hold precondition itself is asserted once for every builder in
tests/test_options.py(second commit), folding in the implementer test from the base branch rather than leaving three near-duplicate statements of one SDK fact — together with the mentor negative described at the top.Against #367's merged drain
drain_until_settledlanded as a LOOP whose stop condition is the SDK's own — aResultMessagearriving with nothing in flight — not the ledger emptying. Both converted drivers were re-verified against it, and two behaviours are now pinned at driver level:_noteclassifies it andrate_limited=Truesurvives. Verified by writing the previously-impossible ordering as a test and watching it pass.Neither driver has an outer
fail_after, so phase 2'scurrent_effective_deadline()clamp is inert for them (infinite remaining budget). That is correct by construction, and called out because it is the one part of the merged helper their tests do not exercise.Running the suite
uv run pytest -q→ 1075 passed, 8 skipped;ruff checkandmypyclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01VmJS1T2Lw4SN85pwou8Yj1