fix(engine): surface subagent failures cleanly — stopReason error (#26) + turn-budget partial (#25) + EMPTY_RESULT (#22) - #30
Merged
Conversation
…ty completed (#26) When a child session's model call ends with stopReason "error" (a stale explicit-model override whose provider 401s, a provider outage, or a rate limit after the SDK exhausts retries), the pi SDK emits message_end with stopReason "error" and prompt() resolves WITHOUT throwing. spawnSubagent's status logic then fell through to `completed` with an empty finalText — the controller saw "(no tool output)" and couldn't tell a broken model from a no-op run (#26/#22). Fix: in the session.subscribe handler, capture a modelError when an assistant message_end carries stopReason "error" (preserving any failure text the SDK attached). In the status determination, mark the run `failed` with that error before the `completed` fallthrough. The modelError branch sits after runError (prompt() throw) and budget exhaustion, so the existing pre-flight "no API key configured" throw from prompt() and the turn-budget path are unchanged. The `modelError` tracker is declared before session.subscribe() because some child sessions emit events synchronously inside subscribe() (temporal-dead-zone guard). Tests: add two #26 tests in spawnSubagent.test.mts — one where the error stop carries content text ("Authentication failed for \"openrouter\"") and one where it carries empty content (the fix synthesizes a diagnostic naming the model). Also harden subagent-tool.test.mts's fakeFactory to emit a realistic assistant message_end (the agent loop always emits ≥1; the prior no-event stub would trip the related #22 EMPTY_RESULT guard when it lands). 598/598 pass (was 596, +2); typecheck clean.
…0 chars (#25) On turn-budget exhaustion the error was `hit turn budget (N) mid-task; partial result: ${finalText.slice(0, 200)}` — a 200-char window that cut mid-sentence (e.g. "...except the G48 viola"). The controller reads `res.error` (the tool surfaces error, not finalText, for failed runs), so it saw a mid-thought fragment and couldn't act. Fix: window the partial to 4000 chars (~600 tokens — enough for any structured summary the model emitted) with an explicit truncation marker naming the run log for the full output. Short partials (<4000) are surfaced whole with no marker. The wind-down nudge (option 3 from the issue — injecting a "you have ~N turns left, emit your partial now" message before the hard cut) is the higher-leverage future enhancement; it needs mid-loop injection semantics and is tracked in #25, not handled here. Tests: two #25 tests — a >4000-char partial is windowed with the truncation marker and its structured header survives; a short partial is surfaced whole with no marker. The existing turn-budget test still passes (error still includes "turn budget"). 597/597 pass on this branch; typecheck clean.
rz1989s
changed the title
…not silent completed (#22) When prompt() resolved cleanly but the child produced NO assistant message_end at all (a silent backend failure, hung provider, or premature exit), spawnSubagent fell through to `completed` with an empty finalText — the controller saw "(no tool output)" with no status, error, or run id (#22). A real agent loop always emits at least one assistant message; zero means a silent failure. Fix: track sawAssistantMessage in the event handler; add an `else if (!sawAssistantMessage)` branch before the `completed` fallthrough that marks the run `failed` with a structured EMPTY_RESULT diagnostic (naming the model) so orchestration can escalate models or retry rather than silently succeeding. The #26 stopReason "error" capture (commit e8c8eb2) already handles the 401/rate-limit sub-case of #22; this handles the no-events-at-all sub-case. Together they close the foreground empty-success gap. Tests: add a #22 test — a silent child (prompt resolves, no events) is failed with EMPTY_RESULT, not completed. Updated the spec2 fakeChild to emit a realistic assistant message_end (the prior no-event stub would now trip the guard — a real child always emits ≥1). Note: the background worker-disappearance watchdog (the bg-run half of #22, where a worker exits without a terminal record and the TODO stays in_progress) is a separate hardening item — not handled here; stays tracked in #22. 598/598 pass (was 597, +1); typecheck clean.
rz1989s
changed the title
…recedence doc, EOF newlines Address the PR #30 code review findings: Critical — test isolation (subagent-tool.test.mts): the fakeHandlers array was a module-level mutable global; handlers accumulated across tests and prompt() replayed events to stale registries. Moved the handlers array inside fakeFactory.create() so each child gets a fresh, isolated array (mirrors spawn-subagent-spec2.test.mts's fakeChild pattern). Important — document the modelError-overrides-success precedence (spawnSubagent.ts): added a precedence note to the modelError status branch explaining that a late error-stop (stopReason "error" on turn N) overrides a prior successful turn's finalText — the run is incomplete and the error is more actionable than a partial. finalText is preserved (not cleared) so the run log still carries the partial; only the surfaced status is failed. Gating on `!finalText` is noted as a future design call, not this fix. Current "last error wins" is the defensible default. Minor — EOF newlines on the 3 touched test files (spawnSubagent.test.mts, spawn-subagent-spec2.test.mts, subagent-tool.test.mts) per repo convention. 598/598 pass; typecheck clean.
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.
Three spawnSubagent status-determination robustness fixes
All prevent a failed/empty run from being mis-surfaced to the controller as an empty or mid-sentence success. Separate commits per fix. Together they close the foreground empty-success gap reported across #22, #25, and #26.
The controller reads
res.error(the tool surfaces error, not finalText, for failed runs), so the error text is the primary signal — these fixes make it accurate and actionable.#26 — failed explicit-model override returns empty output (commit
e8c8eb2)Root cause: a child whose model call ends with
stopReason: "error"(stale override whose provider 401s, provider outage, rate limit after retries) — the SDK emitsmessage_endwithstopReason: "error"andprompt()resolves without throwing. spawnSubagent fell through tocompletedwith empty finalText.Already handled: the pi SDK's
prompt()pre-flight throws on "no API key configured" → spawnSubagent'scatchalready surfaced that. This fixes the 401-at-runtime sub-case.Fix: capture
modelErroronstopReason: "error"; markfailedbefore thecompletedfallthrough.#25 — turn-budget partial sliced mid-sentence at 200 chars (commit
a5350cb)Root cause:
hit turn budget (N) mid-task; partial result: ${finalText.slice(0, 200)}cut mid-thought ("…except the G48 viola").Fix: window the partial to 4000 chars (~600 tokens) with a truncation marker; short partials surfaced whole. The wind-down nudge (option 3 — mid-loop injection) is a future enhancement tracked in #25.
#22 — no-output foreground run silent-completed (commit
2a9b3f0) — foreground half onlyRoot cause:
prompt()resolving with no assistantmessage_end(silent backend failure, hung provider, premature exit) fell through tocompletedwith empty finalText — "(no tool output)" with no status/run id.Fix: track
sawAssistantMessage; anelse if (!sawAssistantMessage)branch marks itfailedwith a structuredEMPTY_RESULTdiagnostic naming the model. A real agent loop always emits ≥1 assistant message; zero means a silent failure.NOT handled here (stays open in #22): the background worker-disappearance watchdog — where a bg worker exits without a terminal record and the durable run + tracked TODO stay
in_progressindefinitely. That's separate hardening (a watchdog/reaper that atomically marks the runWORKER_EXITED_WITHOUT_RESULTand cancels the TODO). This PR does NOT close #22 — it addresses only the foreground half; the bg-watchdog half remains open for a follow-up.Verification
pnpm typecheck— cleanpnpm test:run— 598/598 pass (main's 593 + 2 subagent tool: failed explicit-model override returns empty output instead of a clean error #26 + 2 subagent: turn-budget exhaustion returns truncated mid-sentence partial, not a structured partial #25 + 1 subagent: foreground runs can return empty success with no status, error, or run id #22)git diff --check— cleanCloses #26. Closes #25. Does not close #22 (foreground half addressed; bg-watchdog half remains open).