fix: stop background subagents from blocking the main agent - #1140
Draft
josemonteiro wants to merge 7 commits into
Draft
josemonteiro wants to merge 7 commits into
josemonteiro wants to merge 7 commits into
Conversation
Kimchi Code Review
Summary📊 Review Score: 90/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Comprehensive regression coverage was added: guideline content assertions guard against endorsing polling/blocking, three contract tests cover the 60s cap, abort-signal interruption, and normal completion paths for 📝 Found 1 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 90/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Comprehensive regression coverage was added: guideline content assertions guard against endorsing polling/blocking, three contract tests cover the 60s cap, abort-signal interruption, and normal completion paths for get_subagent_result(wait: true), plus a background-spawn contract test verifying the caller is told not to block. Tests use fake timers to verify the cap and verify that a capped/aborted wait leaves the result unconsumed so the completion notification can still fire.
📝 Found 1 issue(s). See inline comments for details.
josemonteiro
added a commit
that referenced
this pull request
Sep 4, 2026
Three follow-ups from the two-axis spec review of PR #1140: - wait: true now distinguishes outcomes: a capped join reports "Waited 60s (cap)" with re-join guidance for hard dependencies (e.g. ferment worker join), and an aborted wait reports "Wait cancelled". Previously both collapsed into the generic still-running text, which also countermanded the ferment "wait, then retry complete_ferment_step" instruction. - Queued workers (background-by-default behind the concurrency cap) now report "queued behind the cap, not started" instead of falling through to "No output.", for both plain and wait: true calls. - Result-consumption semantics unchanged: timeout/abort/queued never consume the result and the completion nudge stays armed. Tests: timeout/abort assertion updates plus queued-state cases in get_subagent_result wait contract. Co-Authored-By: Kimchi <noreply@kimchi.dev>
josemonteiro
added a commit
that referenced
this pull request
Sep 14, 2026
Three follow-ups from the two-axis spec review of PR #1140: - wait: true now distinguishes outcomes: a capped join reports "Waited 60s (cap)" with re-join guidance for hard dependencies (e.g. ferment worker join), and an aborted wait reports "Wait cancelled". Previously both collapsed into the generic still-running text, which also countermanded the ferment "wait, then retry complete_ferment_step" instruction. - Queued workers (background-by-default behind the concurrency cap) now report "queued behind the cap, not started" instead of falling through to "No output.", for both plain and wait: true calls. - Result-consumption semantics unchanged: timeout/abort/queued never consume the result and the completion nudge stays armed. Tests: timeout/abort assertion updates plus queued-state cases in get_subagent_result wait contract. Co-Authored-By: Kimchi <noreply@kimchi.dev>
josemonteiro
force-pushed
the
fix/background-subagent-blocking
branch
from
September 14, 2026 20:30
c20e658 to
6f1d418
Compare
Background agents already fire a completion notification, but the get_subagent_result tool invited blocking with its text and waited indefinitely with no timeout or abort handling. This froze the main agent's turn and queued user input. - Rework LLM-facing text so backgrounding is paired with "you will be notified" instead of "use get_subagent_result / wait: true". - Cap get_subagent_result(wait: true) at 60s and wire the AbortSignal so Esc aborts the wait. - On timeout/abort, leave the result unconsumed and the nudge armed so the completion notification still delivers the result later. - Preserve the legitimate bounded-join use case used by ferment worker handoff (steps.ts:185). Closes internal report: background subagent polling blocks main agent. Label: bug Co-Authored-By: Kimchi <noreply@kimchi.dev>
Backgrounding is now the default for Agent/subagent calls. This aligns the harness with the expectation that subagents run independently and notify the caller on completion, instead of blocking the main agent by default. - resolveAgentInvocationConfig defaults runInBackground to true. - Agent tool guidelines/schema now say: omit run_in_background for background (default), set run_in_background: false only for hard dependencies. - Custom agent template default updated to true. - Orchestration and ferment instruction text updated to reflect the new default. - Added regression tests for the default and for explicit opt-out. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Three follow-ups from the two-axis spec review of PR #1140: - wait: true now distinguishes outcomes: a capped join reports "Waited 60s (cap)" with re-join guidance for hard dependencies (e.g. ferment worker join), and an aborted wait reports "Wait cancelled". Previously both collapsed into the generic still-running text, which also countermanded the ferment "wait, then retry complete_ferment_step" instruction. - Queued workers (background-by-default behind the concurrency cap) now report "queued behind the cap, not started" instead of falling through to "No output.", for both plain and wait: true calls. - Result-consumption semantics unchanged: timeout/abort/queued never consume the result and the completion nudge stays armed. Tests: timeout/abort assertion updates plus queued-state cases in get_subagent_result wait contract. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Background-by-default broke the ferment-oneshot-exit smoke test in CI: in --print/headless mode there is no interactive loop to consume completion notifications, so the orchestrator ended its turn after spawning a backgrounded worker and the process exited mid-step (ferment abandoned). Master CI is green on the same base, confirming the regression is ours. resolveAgentInvocationConfig now takes a caller-supplied runInBackground fallback driven by ctx.hasUI: interactive sessions default to background, headless runs keep the old foreground default. Explicit run_in_background params and persona policy still override both defaults, so scripts that opt into backgrounded headless work are unaffected and the interactive UX is unchanged. Verified locally: ferment-oneshot-exit smoke test passes; agent and invocation-config suites green; lint and typecheck clean. (The ferment-v2-print-exit/workflows smoke failures reproduced on the pre-fix commit locally as well while passing in CI — local environment flakes, unrelated.) Co-Authored-By: Kimchi <noreply@kimchi.dev>
Background-by-default (55187f9) changed what these auto-model E2E specs exercised: their agentCall helper omitted run_in_background, so Agent calls silently went background in the interactive TUI fixture. Two specs failed: the foreground-child routing decision count gained a notification-driven turn (4 chat requests vs 3), and Escape no longer aborted the backgrounded child's in-flight router request. The helper now always passes run_in_background explicitly so each spec exercises the mode it was written for; background remains covered by the dedicated background-child spec. Verified locally: all 13 auto-model TUI tests pass. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Same class of fix as the auto-model specs: foregroundAgentCall omitted run_in_background, so the scripted child silently went background under the new interactive default — there was nothing for Ctrl+B to detach and the "ctrl+b to run in background" marker never rendered. Pin run_in_background: false so these specs keep exercising the detach-to-background UX they were written for. Verified locally: all 4 background-agents TUI tests pass. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Follow-ups to the background-by-default rollout, resolving three tensions from design review: - Guidelines now spell out the three dependency patterns: foreground only when the agent blocks your very next action; background + bounded wait-join for later-step dependencies; background + notification for fire-and-forget/parallel work. Ferment worker dispatch documents that workers may stay backgrounded and are joined via get_subagent_result wait: true. - Headless sessions now ignore persona runInBackground pins: in automation, backgrounded work can outlive the process, so backgrounding requires an explicit per-call opt-in (explicit run_in_background: true still wins in headless). Documented in the custom-agent frontmatter template. - Orchestration text no longer hard-codes "3 concurrent"; guidance references the background concurrency cap the harness enforces (DEFAULT_MAX_CONCURRENT = 4, rest queued). Verified: 2169 tests across agents/ferment/orchestration suites, lint and typecheck clean. (permissions/acp/web-fetch unit failures reproduce identically on the pre-change tree — local environment issues.) Co-Authored-By: Kimchi <noreply@kimchi.dev>
josemonteiro
force-pushed
the
fix/background-subagent-blocking
branch
from
September 17, 2026 15:32
6f1d418 to
f026cfb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background agents already fire a completion notification, but the
get_subagent_resulttool invited blocking with its text and waited indefinitely with no timeout or abort handling. This froze the main agent's turn and queued user input. This PR makes background the default, fixes the blocking, and makes wait outcomes explicit.get_subagent_result(wait: true)at 60s and wire the AbortSignal so Esc aborts the wait.run_in_backgroundnow defaults totrue; setrun_in_background: falseonly when the next step in the workflow depends on the agent's result. Persona policy and explicit caller values still override the default.src/extensions/ferment/tools/steps.ts:185).Closes internal report: background subagent polling blocks main agent.
Commits
cbdaa47d— bounded, interruptibleget_subagent_result(wait: true)+ LLM contract fixes55187f91— default subagents to background unless the workflow depends on them329c0d57— explicit wait outcomes (timeout/abort) and queued state, from two-axis spec reviewChecklist
pnpm run lintandpnpm run typecheck.