Repository navigation
fix(core): report a background subagent's later turns to its parent - #58825
kitlangton wants to merge 3 commits into
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. The new continuation test fails on the base (no follow-up notice reaches the parent) and passes with this change; tool-subagent, subagent-lifetime, command-subagent and job tests pass. The ordering between the job's own report and the new execution-event listener looks sound: the idle marker is written when the execution event is published, before resume returns, so the tracked flag plus the stored idle ID prevent double reports.
Public API: unchanged.
Concerns (inline):
startwatches foreground children too, so a foreground child that carries on after its tool call returned will now send<subagent>messages to the parent and wake it. Is that intended? The title and description only cover background children.- The new tests use a module-level
emptyFirstflag and a 500 ms wait to prove nothing arrives.
Related: #50751
| ) | ||
|
|
||
| return Service.of({ | ||
| start: (recovery: Recovery) => { |
There was a problem hiding this comment.
start is also called for foreground subagents (tool/plugin/subagent.ts calls it before jobs.block), so every foreground child gets a watch as well. After a foreground child has returned its result through the tool call, any later busy period it starts on its own (a background shell result, a plugin continuing an empty step) now sends a <subagent …> message to the parent and wakes it. That may be what you want, but the PR is framed around background subagents. If it's not intended, only register the watch on the background paths (background, or when block returns backgrounded).
| ]) | ||
| const completionIt = testEffect(liveExecution(TestLLM.text(childText, "completion"))) | ||
| // The first model request (the child's first step) ends empty; every later request answers with text. | ||
| let emptyFirst = true |
There was a problem hiding this comment.
emptyFirst is module-level state that's never reset, so the test only works if the child's first step is the first model request this layer ever serves, and it breaks under --rerun-each or if another test is added to continuationIt. Could the empty-then-text sequence be scoped to the test, e.g. a counter created inside it or a per-test queue of responses?
| yield* sessions.prompt({ sessionID: childID, text: "One more question." }) | ||
| yield* sessions.wait(childID) | ||
| yield* sessions.wait(parent.id) | ||
| expect(Option.isNone(yield* Queue.take(notifications).pipe(Effect.timeoutOption("500 millis")))).toBe(true) |
There was a problem hiding this comment.
This assertion waits 500 ms and passes if nothing arrives, so it would also pass if a stray notification simply arrived late. Since sessions.wait(childID) and sessions.wait(parent.id) have already settled, could it check the parent's messages (or Queue.size) directly, as the first test does?
There was a problem hiding this comment.
Two of the three earlier points are fixed. Watches are now registered only through notify, so only background children are tracked. The module-level emptyFirst flag is gone, and each test pushes its own responses. The 500 ms waits used to prove nothing arrived are still there; that thread stays open.
The new handoff between the job and the follow-ups looks sound. The pending count and the settled idle marker prevent double reports, and catchUp covers a busy period that ends while the job's result is being delivered. Deleting the child or its parent clears its tracking, and skipping empty successes and interruptions keeps plugins retrying empty steps from waking the parent. tool-subagent.test.ts passes 16/16 in three runs in a row; subagent-lifetime, command-subagent and job tests pass 18/18.
Public API: unchanged.
…tifications directly in tests
There was a problem hiding this comment.
The last open point is fixed: the 500 ms "nothing arrived" waits are gone, and those tests now check Queue.size once the sessions settle. If resume fails, the job now records the child's newest idle marker, as it already did on success. So the job delivers that failure itself, and the follow-up path doesn't report it again. This looks right. tool-subagent.test.ts passes 16/16 in three runs in a row; subagent-lifetime, command-subagent and job tests pass 18/18.
Public API: unchanged.
|
Closing in favor of a simpler design: every subagent result goes to the parent through one path, with a saved marker instead of in-memory watchers. The tests here carry over as the spec. |
What it does
When a background subagent keeps working after it first reports, its parent now gets the real result. Before this, if a child ended a step with an empty response and went idle, the parent got "Subagent completed without a text response." If a plugin or a synthetic message then woke the child, it carried on working, but its result never reached the parent.
How it works
SubagentJobremembers each child it puts in the background, along with the idle marker of the last busy period the parent heard about. When one of those children succeeds or fails outside a subagent job, it reads that busy period, from its idle marker back to the previous one. It reports the busy period to the parent in the usual<subagent …>message only when it carries news:The job's own result is still delivered by the existing job path. Until that delivery finishes, follow-ups wait; then the newest busy period is checked once, so nothing is lost or reported out of order. Foreground children aren't tracked. Tracking is cleared when the child or its parent is deleted.
Risk
Proof
New tests in
tool-subagent.test.ts. The last three fail against the first version of this PR:Targeted runs:
tool-subagent,subagent-lifetime,command-subagent,session-execution,jobandlocation-layergive 94 pass.tool-subagentpassed 5 runs in a row. Typecheck is clean.Added by the opencode bot (AI-generated).
Related: #50751