Skip to content

fix(runtime): make subagent spawn selection explicit and recoverable - #5534

Merged
M4n5ter merged 3 commits into
apache:mainfrom
Sun-GLiang:codex/fix-subagent-spawn-selection
Sep 21, 2026
Merged

M4n5ter merged 3 commits into
apache:mainfrom
Sun-GLiang:codex/fix-subagent-spawn-selection

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Summary

agent_list can return no presets while the built-in implementation profile is available. A model nevertheless used subagent_id="implementation" and executor_id="default", causing three parallel spawn calls to fail before any child started. Improving descriptions alone was insufficient: in a live follow-up, the model kept filling inactive fields with placeholder values and exhausted 11 unsuccessful calls.

  • Return directly usable spawn_args from agent_list, and give actionable selector-error feedback. Missing-selector errors identify the required field for the selected mode, so following the feedback does not add a field that will be discarded.
  • Add optional target_kind=profile|preset and executor_mode=inherit|plugin. Discard inactive fields before validation, while strictly validating the selected branch. Explicit plugin mode requires an executor ID.
  • Preserve legacy precedence when modes are omitted, actual preset routing/availability, and explicit plugin selection. Unknown presets are not silently converted into built-in profiles.

No scheduler or concurrency-limit changes. Retries remain new calls chosen by the model after tool feedback; this does not add automatic runtime retries.

Verification

Real-model startup, recovery, and parallelism

Manual validation on 2026-09-20 used the actual Maka runtime-host/CLI path and gpt-5.6-sol through codex-subscription for both parent and children (parent thinking: high). Each run used an isolated state root and a temporary Git repository. Two Implementation children each read one fixture file in a separate worktree; no mock model or stub child session was used.

Case Parent elapsed Spawn calls / errors Children completed Child start gap Concurrent execution overlap
Description-only intermediate fix 69.305 s, manually aborted 11 / 11 0
Final fix: normal parallel dispatch 44.987 s 2 / 0 2 52 ms 6.356 s
Final fix: intentionally wrong first batch, then correction 51.940 s 4 / 2 2 49 ms 5.549 s

For the recovery case, the first batch deliberately selected target_kind=preset, subagent_id=implementation, executor_mode=plugin, and executor_id=default. Both calls failed before creating a child. After receiving the error, the model issued both corrected calls together in its next batch, 9.667 s later:

{
  "target_kind": "profile",
  "profile": "implementation",
  "executor_mode": "inherit",
  "isolation": "worktree",
  "write_back": "patch"
}

task is omitted above for brevity. Both successful calls shared one model step in each run. All four successful children made two real model requests each, used only Read, and returned the expected fixture values (ALPHA=17, BETA=29). The parent did not read the files itself. The recovery run created exactly two children, with no duplicate launches.

These samples demonstrate successful startup and parallel correction when parallel work is requested; they do not establish a general success rate or guarantee that the model will always choose parallel calls. Same-batch tools still settle before the next model request, so a slow sibling can delay model-driven correction. These live runs used the local development build; the isolated PR worktree checks below validate the two-file patch separately.

Follow-up: Local Read and Web Research

Additional real-model runs used gpt-5.6-sol and an isolated runtime-host state root:

  • Local Read normal dispatch: one spawn, zero errors, one completed child; the child made two real model requests and read ALPHA=17 using only Read.
  • Local Read recovery: intentionally using the profile as a preset ID failed before startup; the model corrected its arguments 7.793 s after the error and completed exactly one child, without duplicate launches.
  • Web Research: catalog reported unavailable / missing_tools, and no child was created. Search is disabled in the local configuration, no Tavily credential is configured, and the current Maka native-search capability resolver returns null for openai-codex. This is an availability/configuration limitation; successful live startup or search is not claimed.
  • Parameter checks for both profiles: seven schema checks plus one pre-start error-recovery check per profile passed, including inactive placeholder/empty/malformed fields and strict active-field validation. These are deterministic checks, not live child execution. An additional live run requested inactive placeholders and completed Local Read, but only normalized arguments were retained, so it is not counted as proof that the model actually submitted those placeholders.

Automated checks

  • node --test packages/runtime/dist/__tests__/subagent-tools.test.js: 27 passed, 0 failed in the isolated PR worktree. The two mode-specific missing-selector recovery tests both failed before the review follow-up fix and passed afterward, covering absent selectors and populated inactive fields. The earlier 25-test suite produced 20 passed, 5 failed against the original spawn implementation and 25 passed with the initial fix.
  • npm run lint, npm run format:check, npx knip --workspace apps/desktop, npx knip --workspace packages/ui, and git diff --check: passed.
  • npm run build and npm run typecheck: attempted but not passing in the isolated worktree. Runtime compilation reports existing TS7006 errors in model-factory-thinking.test.ts at lines 1037, 1081, and 1129, and openai-responses-plaintext-reasoning.test.ts at line 549. Restoring the original spawn implementation reproduced the same four errors. Root typecheck additionally reports missing downstream build outputs after the build stops. TypeScript emitted the runtime output used by the focused tests despite its nonzero exit.
  • The full repository test suite was not run. CI remains required; the earlier local development build passed, but that is not claimed as a clean PR build.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex implemented the runtime fix and regression tests, ran the automated and real-model checks, and prepared this PR description. The commit includes a Generated-by: Codex trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Expose callable catalog arguments, ignore inactive selector fields only in explicit modes, and provide actionable errors without changing legacy routing or parallel scheduling.

Generated-by: Codex
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 20, 2026
Report the selected identity field when an explicit spawn mode lacks its selector, retaining legacy guidance for callers without a mode. Cover recovery with and without an inactive selector present.

Generated-by: Codex

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed cb589961. The fix shape is right and matches an existing in-repo precedent: stream-graph-supervisor-tools.ts already solves this exact ambiguity with a target_kind discriminator + preprocess field discard, so agent_spawn converging on the same idiom is the correct move rather than a new mechanism. I walked the full preprocess matrix (every target_kind × selector and executor_mode × executor_id combination): legacy precedence is preserved (subagent_id wins when target_kind is absent), executor_id survives when executor_mode is absent (existing plugin callers unaffected), and the impl still resolves on surviving-field presence rather than re-reading the discriminator — so a preset spawn without target_kind can't fall through to the profile path. spawn_args templates omit task, so a verbatim copy fails required-field validation rather than spawning a garbage-task child — the safer failure direction. The unknown-preset recovery path correctly distinguishes "built-in profile id passed as preset" and hands back a usable corrected call. Tests pin the reported incident shape.

Remaining findings are all minor:

P3 — spawn_args is emitted on view=catalog entries regardless of availability. In projectAgentList the catalog view keeps unavailable presets/profiles (by design, for diagnosis) but now also attaches a copyable spawn_args template to them. A model operating from the catalog view can copy a template that is guaranteed to fail downstream. It fails closed with an actionable unavailable message, so this is cosmetic; omitting spawn_args on non-available entries would make the contract airtight.

P3 — target_kind vocabularies are disjoint across sibling tools. agent_spawn accepts profile|preset; update_agent_graph uses new_agent|new_preset|existing_operator, and the graph/swarm prompts explicitly teach target_kind=new_preset. A model carrying the graph vocabulary into agent_spawn gets a plain enum error — self-correcting, but the shared field name invites the mistake. Worth a one-line note in the description ("graph target_kind values are not valid here") or accepting as-is.

P3 (pre-existing, more traveled now) — second-resolution TOCTOU produces the least actionable error. The tool resolves the preset against a fresh listChildAgents snapshot, then resolveChildSessionSelectorsubagentCatalog.resolve re-fetches presets independently. A concurrent settings edit between the two yields Subagent preset "x" profile changed during spawn (session-manager.ts) — the only message on this path that names neither the field nor a remedy; the model can only retry identically. Not introduced by this PR, but spawn_args steering makes the copy-then-spawn path more common, so the window gets more traffic. A retry the same spawn_args hint in that message would close it cheaply.

Approved — the reported failure mode is addressed at the schema boundary where it originates, and the discarded-field semantics are explicit rather than implicit.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

@Astro-Han Thanks for the detailed review. I addressed the two concrete recovery/contract follow-ups in d79140d:

  • agent_list now omits spawn_args from unavailable preset and legacy-profile entries in view=catalog, while retaining their status and diagnostic reason. Coverage now pins both cases.
  • The preset-profile TOCTOU error now tells the caller to retry the same agent_spawn call, with a focused regression test.

I kept the sibling tools' target_kind vocabularies as-is for now. Each tool has a strict enum, and agent_spawn already describes its profile|preset values, so a graph-only value fails schema validation before any child starts. I would rather add cross-tool negative guidance only if real-model runs show that confusion in practice.

Validation:

  • node --test packages/runtime/dist/__tests__/subagent-tools.test.js packages/runtime/dist/__tests__/session-manager.test.js: 222 passed, 0 failed
  • focused Biome lint/format and git diff --check: passed
  • Runtime build still stops only on the same four pre-existing TS7006 errors already documented in the PR description

@M4n5ter
M4n5ter merged commit 5a1252c into apache:main Sep 21, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants