feat(subagents): child-safe nested fanout and MCP tool allowlists

[flora131]

Summary

Syncs packages/subagents with upstream nicobailon/pi-subagents v0.25.0, introducing nested child-safe fanout execution, parent-visible nested status/control rendering, and MCP tool allowlist handling — while preserving all Atomic-specific import, config, and runtime adaptations.

Closes #1019

Added

• Nested fanout child registration (fanout-child.ts): registers a scoped subagent tool in child processes running under SUBAGENT_FANOUT_CHILD_ENV, enabling explicitly authorized agents to spawn their own subagents with a read-only management surface and bidirectional control (interrupt/resume) via a polling inbox.
• Nested events system (nested-events.ts): file-based event bus for nested run lifecycle (started/updated/completed) and bidirectional control (interrupt/resume) between parent and child fanout processes, with depth-bounded route inheritance and registry state.
• Nested path utilities (nested-path.ts): depth-bounded, sanitized path IDs for routing nested control messages safely across process boundaries.
• Nested render (nested-render.ts): formats nested subagent step summaries as inline status lines with glyphs for display in parent TUI widgets.
• MCP direct tool allowlist (mcp-direct-tool-allowlist.ts): resolves and caches configured MCP direct-tool names from global/project configs across all supported clients (Claude Code, Cursor, Windsurf, Codex, VS Code), injecting them into explicit child tool allowlists at spawn time.
• Run-ID resolver (run-id-resolver.ts): resolves canonical run IDs from environment variables or filesystem state for accurate async job targeting across foreground, async, and nested run kinds.
• Result intercom (result-intercom.ts): delivers intercom message events to live nested child agents by address, and attaches nested children to result intercom payloads.
• New types: NestedRunState, NestedOwnerState, NestedRunAddress, NestedStepSummary, NestedRunSummary, PublicNestedRunSummary, and PublicNestedStepSummary in shared/types.ts.

Fixed

• Honor nested control interrupt responses from child fanout processes.
• Include MCP direct tool names in explicit child tool allowlists when config resolution is available.
• Treat provider-coerced single-run output: "false" the same as boolean false, preventing literal "false" output files.
• Respect read-only completion guard overrides; support disabling completion guards for read-only builtin agents.
• Write async runner config files with owner-only permissions (0o600) so nested route capability tokens are not exposed via permissive umasks.
• Suppress flashing console windows when spawning child processes on Windows (windowsHide: true).

Changed

• Removed local animation-stability timer/remount changes in render.ts; upstream rendering is now used directly with only required Atomic import/type adaptations retained.
• Updated subagent README, skill guidance, and changelog entries for Atomic behavior and the nested fanout authorization model.

Validation

• bun run typecheck — clean
• bun run test:unit — 1385 passing (6 new test files: subagents-async-config, subagents-mcp-direct-tool-allowlist, subagents-nested-events, subagents-nested-render, subagents-pi-args, subagents-result-intercom, subagents-run-id-resolver)
• git diff --check — no whitespace errors
• Interactive tmux QA (bun packages/coding-agent/src/cli.ts):
  • subagent list smoke test found codebase-analyzer
  • Single / parallel / chain / async subagent runs all returned expected sentinel values
  • Child-safe nested fanout custom temp agent returned QA_NESTED_PARENT_OK and QA_NESTED_CHILD_OK

[claude]

Review — PR #1029 (sync subagents upstream)

Reviewed against CLAUDE.md conventions (Bun, strict TS, raw-TS workspace, .js import specifiers, prefer no comments unless WHY is non-obvious). Notes are line-cited against the diff. Overall this is a careful upstream sync that correctly preserves Atomic adaptations (@bastani/atomic imports, APP_NAME/PACKAGE_NAME, .atomic/~/.atomic paths with legacy .pi compat, bundled intercom). The new fanout authorization gating and child-safety boundary changes look sound, but a few items warrant attention before merge.

Findings

1. Unbounded seen set in fanout-child control listener — packages/subagents/src/extension/fanout-child.ts:90

const seen = new Set<string>();
…
seen.add(request.requestId);

The listener's seen set grows for the lifetime of the fanout child and is never trimmed. Contrast with nested-events.ts which explicitly caps processedEvents to slice(-1000) (line ~5265 of the patch). In practice this is bounded by request files (which are unlinkSync'd after success), so the dedup-on-replay case keeps it from going wild — but if fs.unlinkSync ever silently fails (eg. perm error, racy fs), seen is the only thing standing between you and infinite re-processing, and it keeps growing. A simple if (seen.size > 1024) { /* keep last 512 */ } would match the pattern used elsewhere.

The sibling pendingResults map (line 92) has the same shape: it's only deleted on successful writeNestedControlResult. A persistently-failing inbox write will accumulate entries.

2. Timer disposal on extension reload — fanout-child.ts:154 and registration block ~165–175

const timer = setInterval(() => { … }, 200);
timer.unref?.();
return timer;

The returned timer is dropped at the registration site (startNestedControlInboxListener(pi, state) is called as a statement, return value discarded). Combined with the globalThis['__piSubagentFanoutChildRegisteredApis'] WeakSet guard, the registration is idempotent only as long as the same pi reference is reused. If the module is reloaded in-process (dev/HMR/extension restart) the WeakSet is preserved on globalThis but old timers are orphaned — so reloads will accumulate intervals. unref() prevents process-hang but doesn't fix the leak; consider tracking the timer on state and exposing a disposer, or attaching to a pi.onDispose-equivalent if the SDK provides one.

3. Confusing userDir resolution — packages/subagents/src/agents/agents.ts:866

const userDir = getEnvValue('ATOMIC_CODING_AGENT_DIR') ? userDirOld[0]! : fs.existsSync(userDirNew) ? userDirNew : userDirOld[0]!;

Two ternaries on one line with the env-var branch yielding the variable named userDirOld[0] is hard to follow. If the intent is "when the env override is set, trust whatever getAgentConfigPaths returned at index 0", a one-line comment explaining that (and renaming userDirOld or capturing the env-override path into a named local) would save the next reader a trip to @bastani/atomic. This is an exception to the no-comments rule — the WHY is genuinely non-obvious.

4. Completion-guard bypass surface — packages/subagents/src/runs/shared/completion-guard.ts and config plumbing
The read-only-tools shortcut at declaresOnlyReadOnlyTools(...) is good (line 4316 of diff) and the agent-frontmatter / builtin-override threading looks consistent. However, completionGuard: false in agent frontmatter is now accepted as a hard opt-out (agents.ts:684–688, executor checks step.completionGuard !== false / agent.completionGuard !== false). A user-defined or project-defined agent can therefore disable the guard entirely with no further gating. That may be intentional, but it'd be worth noting in the CHANGELOG/README that completionGuard: false is a user-authored escape hatch rather than only an override-only knob — the README change for completionGuard doesn't mention frontmatter usage.

5. Test coverage gap on new files — test/unit/
The four new files with the most non-trivial logic — fanout-child.ts (204 LOC, control-inbox state machine), mcp-direct-tool-allowlist.ts (368 LOC, cache-driven allowlist), nested-events.ts (826 LOC, fs I/O + projection + registry), and run-id-resolver.ts (83 LOC, prefix disambiguation) — ship with no unit tests in this PR. The diff to subagents-render-stability.test.ts only removes tests for the local animation-timer behavior that this PR also removes, so that delete is consistent. But the new surfaces have nothing exercising them in bun:test. At minimum, run-id-resolver (pure logic, ambiguity throw path) and the mcp-direct-tool-allowlist cache/prefix resolution would be cheap to cover and protect against silent regressions from future upstream syncs.

6. Nits

• fanout-child.ts:1–17: import grouping mixes node: builtins, @bastani/atomic, and local ../ siblings without a blank-line separator — most other files in this package use the grouped style.
• fanout-child.ts:13: SubagentParams is imported from ./schemas.ts but the file uses .ts not .js extension. Per CLAUDE.md ("Source files use .js import extensions (TypeScript ESM convention)"), this should be ./schemas.js. Same for the other intra-package imports in this new file. Verify against neighbor files — if the rest of the package is on .ts, ignore; if .js, please align.
• subagent-executor.ts and several other files: windowsHide: true (line 215 of diff) added to spawned children — good defensive change, worth a one-line note in the changelog.

Done well

• Path containment checks via containedPath on nested event sinks and capability-token validation give a solid TOCTOU posture for the new fs-backed event channels.
• Fanout child mode is gated by two env vars (SUBAGENT_CHILD_ENV === '1' and SUBAGENT_FANOUT_CHILD_ENV === '1', fanout-child.ts:163) and explicitly disables mutating management actions (allowMutatingManagementActions: false, fanout-child.ts:194) — good defense-in-depth.
• MCP direct-tool allowlist defaults to [] on missing/stale cache rather than allowing-all — correct deny-by-default posture.
• Atomic adaptations (APP_NAME, getEnvValue, .atomic/~/.atomic with .pi legacy fallbacks) are threaded consistently through the new code; the legacy-compat strategy matches what the README/SKILL.md now documents.
• processedEvents projection is capped at slice(-1000) and event files are written atomically via temp+rename — bounded memory + safe writes.

Verification I did

• git diff line-citing for the seen-set, completion-guard, env-var, and test-removal claims.
• Read of the current main-branch subagents-render-stability.test.ts confirms the deleted tests targeted exactly the local-timer behavior this PR removes — the delete is consistent with the stated upstream-alignment goal.

LGTM after addressing the unbounded-set / timer-disposal items (1, 2) and at least a short comment on the userDir ternary (3). The test gap (5) is the most important non-blocking item to track.

[claude]

Review: feat(subagents): sync upstream fanout updates

Reviewed the diff as a large upstream sync (~5.2k+/3.7k-, 39 files). The Atomic-specific reapply work looks careful and consistent — @bastani/atomic imports, APP_NAME/getEnvValue/getAgentConfigPaths adapters, ATOMIC_CODING_AGENT_DIR for legacy-config fallback, .atomic paths, and bundled skill/agent assets are reapplied correctly across the touched files. The render-stability regression test in test/unit/subagents-render-stability.test.ts is preserved and still asserts the behaviors the upstream sync drops the timer-based code for.

Things I found while reading. Most are minor; one or two are worth a follow-up.

Bugs / correctness

1. nestedArtifactEnv hardcodes PI_ prefix and is dead. packages/subagents/src/runs/shared/nested-events.ts:810-815 returns PI_SUBAGENT_NESTED_ROOT_RUN_ID/PI_SUBAGENT_NESTED_PARENT_RUN_ID. The function is exported but has no callers in the package. If it's intentionally kept for upstream parity, either delete it (no callers — it would just be misleading) or switch the keys to ${APP_NAME.toUpperCase()}_SUBAGENT_NESTED_* to match every other env elsewhere in this sync (e.g. pi-args.ts, subagent-executor.ts:2465).

2. NESTED_EVENTS_DIR is never cleaned up. subagent-executor.ts:2327 creates a new nested route (createNestedRoute(runId)) for every top-level subagent invocation that doesn't inherit one, including foreground single runs that never actually need cross-process IPC. The route is materialized at <TEMP_ROOT_DIR>/nested-subagent-events/<rootRunId>-<token>/ with events/, controls/, and route.json files. I couldn't find any teardown — no rmSync after run completion, no scan-and-cleanup analogous to cleanupOldChainDirs() / cleanupAllArtifactDirs() at extension boot. Over long sessions this directory will accumulate orphaned route dirs from completed runs.

Consider either (a) only creating a route when a child can actually emit (i.e. when fanoutAuthorized is true at the relevant call site), or (b) adding a startup sweep that deletes route dirs older than N hours.

3. fanout-child.ts inbox-control retry has no upper bound. packages/subagents/src/extension/fanout-child.ts:75-156 keeps pendingResults to retry when writeNestedControlResult fails. There's no cap on map size and no max-retry. If the parent goes away mid-run, every subsequent inbox poll re-tries those entries forever (the original request file isn't unlinked until the result is written). At 200ms poll interval that's a low-cost-but-unbounded loop. Worth either dropping the request after K failures or aging entries out by timestamp.

4. Silent fs.unlinkSync failure in the control inbox. Same file, line 144: try { fs.unlinkSync(request.filePath); } catch {}. If unlink fails (permission, EBUSY) the entry stays in seen but the file stays on disk, and on the next interval its request.requestId is in seen.has(...) so it's skipped — that's correct for dedup, but the leftover file is silently abandoned. A one-line console.error would at least make the leak observable.

5. mcp-direct-tool-allowlist.ts:81 swallows all errors. resolveMcpDirectToolNames returns [] on any throw. That silently strips MCP direct tools from the child allowlist if config loading or cache reads hiccup, and the failure is invisible. Logging the error (matching the pattern used in loadConfig at extension/config.ts:12) would help debugging.

6. run-id-resolver.ts ambiguity check. Line 77: the dedup map keys are ${match.kind}:${match.id}, so identical IDs in different namespaces (e.g. async + nested) won't dedupe and will trip the ambiguity error at line 79–80. That's likely the intended behavior (it surfaces the collision), but the error message says "matched: foreground:X, async:X" which is a little confusing because the user supplied just X. Worth a brief test that confirms this case.

Style / convention notes

• packages/subagents/src/runs/shared/nested-events.ts:127 does name.replace(/^[A-Z0-9]+_/, "PI_") to fall back to legacy PI_ env names. Worth a one-line comment that this is the Atomic backcompat shim for the PI_* → ATOMIC_* rename, not a generic env lookup. Easy to misread otherwise.
• Several places format env names dynamically via \${APP_NAME.toUpperCase()}SUBAGENT_INTERCOM_SESSION_NAME`(e.g.subagent-executor.ts:2465, async-execution.ts:479,679). Extracting these as exported constants in pi-args.tsalongside the otherSUBAGENT*_ENV` constants would reduce typo risk and make them grep-friendly.
• packages/subagents/src/runs/shared/pi-args.ts:163 chains four ?? fallbacks for parentRunId; readable but a quick comment on the precedence (explicit > env-inherited > current-run > legacy) would help future readers.

Things that look good

• nested-events.ts validation is solid: safe-id checks, capability-token + rootRunId cross-validation against route.json, containedPath guards, 0o700/0o600 modes on the route dirs/files, and per-record size limits (MAX_EVENT_BYTES, MAX_STEPS, MAX_CHILDREN, MAX_DEPTH). The atomic-rename in writeRouteRecord (write tmp → rename) avoids partial reads.
• Nested resume safety in subagent-executor.ts:447-467 is careful — rejects symlinks, requires .jsonl, checks fs.realpathSync is inside trusted session roots, and verifies the session path includes the nested run ID. Good defense against directory traversal via session-file injection.
• executeAsyncChain/executeAsyncSingle correctly thread the inherited nested route through to the runner and only emit subagent.nested.started events when both inheritedNestedRoute and nestedAddress are present (async-execution.ts:463, 663), guarding against partial-env states.
• The fanout child registers globalThis.__piSubagentFanoutChildRegisteredApis (a WeakSet) to dedupe extension API registration across reloads — small but nice touch.
• Tests pass (1385 per PR description) and bun run typecheck is clean per the PR notes. The single render-stability test that was kept matches the behavior the new upstream render preserves.

Testing

I'd add lightweight coverage for:

• A nested route lifecycle test that confirms createNestedRoute dirs get cleaned up (or document explicitly that they don't, and surface the limitation).
• fanout-child.ts control inbox: simulate a writeNestedControlResult failure and assert pendingResults doesn't grow unboundedly.
• resolveMcpDirectToolNames: a smoke test that emits a log/warning instead of silently returning [] when config parse fails.

Overall the sync looks coherent and the Atomic-specific adaptations are applied uniformly. The route-cleanup gap (#2) and the unbounded retry map (#3) are the items I'd most want addressed before merge or in a quick follow-up. The rest is polish.

[claude]

Code Review — Upstream subagents sync

Reviewed against CLAUDE.md style/conventions and the new fanout/nested-run subsystem. Overall this is a well-scoped sync; the new IPC surface (nested-events, nested-path, fanout-child) takes input validation seriously, uses 0o700/0o600 modes for capability-token directories, and containedPath correctly guards against traversal across route roots. Atomic adaptations (@bastani/atomic imports, APP_NAME, .atomic config paths, fanout-child extension wiring) look correctly reapplied.

A few items worth addressing before merge:

Correctness / Robustness

1. packages/subagents/src/extension/fanout-child.ts:115 — fs.unlinkSync(request.filePath) swallows errors with empty catch. seen.add() runs before unlink, so retries are skipped correctly — but seen and inFlight are unbounded for the lifetime of the child process. Long-running fanout children polling at 200 ms intervals will accumulate request IDs. Consider an LRU cap, or relying on the persisted control-result event to drive dedupe so the in-memory set can be bounded.

2. packages/subagents/src/runs/shared/nested-events.ts:549 — processedEvents is truncated to .slice(-1000). If the event sink ever exceeds 1000 files, older entries will fall out of seen on the next projectNestedEvents pass and the same events will replay. Given MAX_CHILDREN = 16 and MAX_DEPTH = 3 the bound is generous, but worth a one-line comment about the implicit cap or a console.warn when the cap is hit.

3. packages/subagents/src/runs/shared/mcp-direct-tool-allowlist.ts:73-84 — resolveMcpDirectToolNames returns [] on any thrown error with no diagnostic. A corrupt mcp-cache.json or transient FS error silently disables direct MCP tools for child subagents. Other modules in this PR (e.g. fanout-child.ts:122) log via console.error; doing the same here would make this debuggable without changing behavior.

4. packages/subagents/src/runs/shared/mcp-direct-tool-allowlist.ts:267-281 — computeMcpServerHash includes the resolved bearer token in the SHA-256 input. The digest is stored in mcp-cache.json under getAgentDir(). SHA-256 isn't reversible so the risk is low, but hashing the env-var name + a presence marker (rather than the value) sidesteps the need to reason about cache file permissions on multi-user systems.

5. packages/subagents/src/runs/shared/nested-events.ts:142, 152 — Capability tokens appear verbatim in Error messages and console.error lines (resolveInheritedNestedRouteFromEnv catch). Tokens are session-scoped UUIDs so the impact is bounded, but redacting them in user-facing error strings would avoid leaking via shared log buffers.

Test coverage

6. Seven new files (~1700 LOC) ship with zero tests in test/unit/ — nested-events.ts (826 lines), nested-path.ts, nested-render.ts, run-id-resolver.ts, mcp-direct-tool-allowlist.ts, fanout-child.ts, extension/config.ts. CLAUDE.md explicitly calls out the tdd skill and bun:test. At minimum the input-sanitization surface (isSafeNestedPathId, sanitizeNestedPath, sanitizeSummary, parseRecord, parseControlRequest) deserves focused tests for malformed input and prototype-pollution attempts — this is the trust boundary between parent and child processes. The PR notes 1385 passing tests + interactive QA, but unit coverage on these helpers would catch regressions on the next upstream sync without re-running the whole tmux harness.

7. test/unit/subagents-render-stability.test.ts lost 147 lines. The PR description explains this is intentional (the local animation stabilization was reverted to align with upstream), and the remaining 3 tests still exercise widgetRenderKey stability and legacy timer cleanup, which is reasonable. However the CHANGELOG.md [Unreleased] Fixed entry "Smooth live subagent widget animation by requesting redraws on spinner ticks without remounting the widget…" describes the now-reverted local fix and is misleading as written. Either drop that entry or move it to a ### Reverted / ### Changed block.

Style / Maintainability

8. packages/subagents/src/runs/foreground/subagent-executor.ts grew by 319 lines on top of an already-large file. The nested-resume, nested-status, and resume-target resolution logic could reasonably extract to nested-resume.ts / nested-status.ts siblings of run-id-resolver.ts. Not blocking, but the file is approaching a size where readers will struggle to hold the executor state in their head.

9. packages/subagents/src/runs/shared/nested-events.ts:127 — legacyName.replace(/^[A-Z0-9]+_/, \"PI_\") is doing a backwards-compat env name fallback for parents using the old PI_* prefix. A one-line // fallback for legacy pi-prefixed env vars would save a reader from rebuilding the intent.

10. packages/subagents/src/extension/fanout-child.ts:30-50 — createChildSafeState uses noop shapes for resultFileCoalescer, cleanupTimers, etc. A short // child-safe stub: parent owns watcher/coalescer/cleanup makes the intent obvious without a reader needing to grep for the real implementations.

Observations (no action needed)

• The capability-token-gated nested route validation (validateRouteShape, commonRouteRoot cross-check at nested-events.ts:110) is solid; capability tokens are randomly generated UUIDs, paths are constrained under NESTED_EVENTS_DIR, and route metadata is re-validated against env-provided values on every resolve.
• run-id-resolver.ts correctly surfaces ambiguous-prefix errors with all candidate matches in the message, which will save users from generic "not found" confusion.
• writeAtomicJson uses temp-file + rename and cleans up in finally — correct pattern, with the noted caveat that the unlink-after-rename will harmlessly ENOENT and is force'd. Good.

Verdict

Approve with the test-coverage gap (#6) being the most important item to address before this lands, since the new code is a parent/child IPC trust boundary. Items 1–5 are bug-fix nits or hardening that can be done in follow-ups. The Atomic-side reapplications all look intact.

🤖 Generated with Claude Code

[claude]

ok

[claude]

Review part 1/4 - overview and bugs/correctness

Reviewed the diff on top of e671f3c. The sync is large (~5k +, ~3.7k -) but well-structured: nested events, capability tokens, path containment, atomic writes, and 0o700/0o600 permissions are applied consistently across the new runs/shared/nested-events.ts, extension/fanout-child.ts, and mcp-direct-tool-allowlist.ts. A few things worth addressing before merge.

Bugs / correctness:

1. Missing cleanup of nested route + run dirs. createNestedRoute (packages/subagents/src/runs/shared/nested-events.ts:113) creates ${TEMP_ROOT_DIR}/nested-subagent-events/${rootRunId}-${capabilityToken}/, and executeAsyncChain/executeAsyncSingle create sibling ${TEMP_ROOT_DIR}/nested-subagent-runs/${rootRunId}/${id}/ dirs, but I cannot find any code path that removes either when the root run terminates. cleanupOldArtifacts/cleanupAllArtifactDirs only walk the artifact directories. Over time these accumulate in TEMP_ROOT_DIR, and the processedEvents registry grows alongside them. Either extend the artifact age-based sweep to these roots or call cleanup in the foreground executor right after the root completes.

2. Unbounded pendingResults in fanout-child inbox listener. packages/subagents/src/extension/fanout-child.ts:62,109 keeps unwritten control results in a Map keyed by requestId and retries forever if writeNestedControlResult keeps throwing (e.g. the parent route was removed). There is no eviction policy and no cap, so a stuck inbox leaks indefinitely. Suggest a TTL or max-size, and dropping the pending entry after N failed writes with a single error log.

3. SUBAGENT_PARENT_DEPTH_ENV is never clamped on write. pi-args.ts:170 does inheritedDepth + 1 and writes it unbounded. Reads in nested-events.ts:161 clamp to MAX_DEPTH=3, but the env value itself grows without bound in deep chains. Not a correctness bug today because checkSubagentDepth uses a separate SUBAGENT_DEPTH_ENV, but it is a footgun if anything ever reads SUBAGENT_PARENT_DEPTH_ENV directly. Recommend Math.min(inheritedDepth + 1, MAX_DEPTH) at the write site, with MAX_DEPTH exported from nested-events.ts so the two stay in sync.

4. BUILTIN_TOOL_NAMES duplicated. mcp-direct-tool-allowlist.ts:10 hardcodes ["read", "bash", "edit", "write", "grep", "find", "ls", "mcp"]. If pi/atomic adds another builtin tool, an MCP tool that collides will quietly leak into the allowlist until someone updates this set. Source this from the canonical pi tool registry rather than maintaining a parallel list.

5. globalStore[registeredKey] instanceof WeakSet cast is unsafe in fanout-child.ts:133-137. The as WeakSet assertion is fine in practice but any other extension squatting on __piSubagentFanoutChildRegisteredApis with a different value type would silently bypass the dedup. Consider a more specific sentinel key or branding.

[claude]

Review part 2/4 - maintainability and tests

Maintainability:

6. Lost named-constant exports for parent env vars. Pre-PR, pi-args.ts exported SUBAGENT_INHERIT_PROJECT_CONTEXT_ENV, SUBAGENT_INHERIT_SKILLS_ENV, and SUBAGENT_INTERCOM_SESSION_NAME_ENV as named constants. The PR inlines them as ${ENV_PREFIX}_SUBAGENT_... strings in three places (pi-args.ts:189-194, intercom/index.ts:26, subagent-prompt-runtime.ts:6-8). Renaming APP_NAME would silently desync these. Worth re-exporting from one shared module.

Tests:

7. No new tests for the largest additions. ~2,400 LOC of new logic in nested-events.ts, fanout-child.ts, run-id-resolver.ts, nested-render.ts, and mcp-direct-tool-allowlist.ts ship with zero new unit tests. The PR body covers this with interactive tmux QA, which is great as smoke verification but does not lock in the security-critical invariants (capability token check, path containment in validateRouteShape, depth/size clamps in sanitizeSummary, prefix disambiguation in resolveSubagentRunId). At minimum I would want:

   • validateRouteShape rejects routes outside NESTED_EVENTS_DIR or with mismatched (eventSink, controlInbox) roots
   • parseRecord rejects events with wrong capabilityToken / rootRunId
   • resolveSubagentRunId throws on ambiguous prefix and dedupes via the kind:id key
   • MAX_EVENT_BYTES / MAX_STEPS / MAX_CHILDREN actually clamp

8. Removed render-stability tests in test/unit/subagents-render-stability.test.ts are intentional per the PR description (Atomic-local animation helpers are being dropped to track upstream). The three remaining tests still pin the no-wall-clock-driven-render-churn invariant, so this is fine - just calling it out so reviewers do not miss it.

[claude]

Review part 3/4 - performance and security

Performance / resource use:

9. 200 ms polling in startNestedControlInboxListener (fanout-child.ts:124) means each fanout child does ~5 readdirSync+statSync cycles per second on its inbox. The result-watcher path already uses an fs.watch + 50 ms coalescer pattern in result-watcher.ts; consider mirroring that here so idle children do not burn syscalls. timer.unref() helps process exit but not steady-state cost.

Security:

10. The capability-token + filename-prefix scheme on NESTED_EVENTS_DIR/${rootRunId}-${capabilityToken}/ is good. One small wart: findNestedRouteForRootId (nested-events.ts:374) and listNestedRoutes (nested-events.ts:452) iterate readdirSync(NESTED_EVENTS_DIR) and JSON.parse every route.json. A malicious local user could plant a directory matching ${rootRunId}-*; validateRouteShape catches it via containedPath, but the per-entry try/catch { continue; } will silently swallow EACCES etc. Worth at least logging unexpected errors (non-ENOENT) once.

[claude]

Review part 4/4 - nits and summary

Nits:

• fanout-child.ts:54 declares let route; without a type annotation - given the strict noUnusedLocals/noUnusedParameters settings noted in CLAUDE.md, prefer let route: NestedRoute | undefined; for clarity.
• nested-events.ts:553 parent-projection comment is helpful - consider a similar one near writeNestedEvent to document the child/runner-never-rewrite-registry invariant for future readers.
• run-id-resolver.ts:64 ambiguity error mentions "stale registries are cleaned up", which ties back to issue (1): without a cleanup path the user is told to wait for a thing that will not happen.

Summary:

Overall this looks like a careful sync. The big-ticket items I would want before merge are (1) nested temp cleanup, (2) pendingResults eviction, and (7) some targeted unit tests around the new validation logic - the rest are nits or polish.

(Note: please ignore the earlier single-word "ok" comment - that was a test of stdin posting since the sandbox blocked posting the full review in one shot.)

[claude]

Review: feat(subagents): add nested fanout support with child-safe execution and MCP allowlists

Thanks for the meticulous upstream sync — this is a large change (5,697 / 3,717) but the Atomic-specific seams (env prefix derived from APP_NAME, @bastani/atomic imports, getAgentDir()/getAgentConfigPaths(), atomic JSON writes, legacy PI_-prefixed env compat) are preserved cleanly. The new security model around nested fanout — capability tokens, path containment, file size bounds, depth/breadth clamps, read-only child executor — is layered carefully. A few notes below.

Strengths

• Capability-based routing. nested-events.ts validates both the path containment (events/controls must sit under NESTED_EVENTS_DIR with a shared route root) and the metadata sidecar (rootRunId + capabilityToken must match) before any read or write. Child processes can only act on routes they were explicitly handed via env. (packages/subagents/src/runs/shared/nested-events.ts:107-113, :187-195)
• Defense-in-depth sanitization. sanitizeSummary/sanitizeStep enforce MAX_NESTED_DEPTH, MAX_NESTED_STEPS, MAX_NESTED_CHILDREN, and string-length caps on every field, including the recursive children slot. (packages/subagents/src/runs/shared/nested-events.ts:251-323)
• Read-only fanout child mode. fanout-child.ts registers a tool with allowMutatingManagementActions: false, and the description string explicitly enumerates blocked actions (create, update, delete). The executor route-scopes nested resolution to the inherited route only when this flag is set. (packages/subagents/src/extension/fanout-child.ts:213-241, packages/subagents/src/runs/foreground/subagent-executor.ts:209-217)
• Bounded retry queue. shouldDropPendingResult caps in-flight nested control results by attempts, age, and size — important since the inbox is polled every 200ms. (packages/subagents/src/extension/fanout-child.ts:55-90)
• Symlink-aware session validation. validateNestedSessionFile rejects symlinks via lstatSync, walks realpathSync, and asserts the resolved path lies under a trusted session root and under the nested run's own id-named directory — good prevention of resume-target spoofing. (packages/subagents/src/runs/foreground/subagent-executor.ts:448-468)
• File permissions. Route dirs at 0o700, route files at 0o600. Atomic temp-then-rename in writeRouteRecord. (packages/subagents/src/runs/shared/nested-events.ts:119-128, :616-626)
• Tests are real. The four new test files exercise core invariants (env parsing legacy compat, capability-token mismatch rejection, cleanup of stale runtime dirs, ambiguity errors in resolveSubagentRunId, fanout-authorized arg construction).

Bugs / things worth a second look

1. spawnRunner writes the async config without explicit mode bits. In async-execution.ts:188, fs.writeFileSync(cfgPath, JSON.stringify(cfg)) is called without { mode: 0o600 }. The config JSON now carries nestedRoute.capabilityToken (added by this PR), so on a multi-user box the default umask could expose the token to other local accounts. Other route writes in nested-events.ts already use 0o600; tightening this one to match would close the gap.

2. Possibly unused local in executeAsyncChain. nestedAddress is computed (async-execution.ts:271) but I don't see it consumed in the surviving flow; if it's genuinely dead after the refactor, removing it avoids a confusing read. Worth a quick grep before merging.

3. resolveNestedRouteFromEnv can throw from JSON.parse. Most callers wrap with resolveInheritedNestedRouteFromEnv (which swallows + logs), but the fanout-child listener calls resolveNestedRouteFromEnv directly and `catch`-returns `undefined`. That's fine. Worth double-checking any future call sites don't accidentally use the throwing variant. (packages/subagents/src/runs/shared/nested-events.ts:182-196, packages/subagents/src/extension/fanout-child.ts:135-141)

4. Status sanitizer accepts both `"complete"` and `"completed"`. `sanitizeStep` allows either spelling for compat (`nested-events.ts:256-258`), but the `terminal()` predicate only matches `complete | failed | paused` (`nested-events.ts:371-373`). A `"completed"` value flowing through unchanged would render fine but be treated as non-terminal — could cause `hasLiveNestedDescendants` to falsely report live work. Worth normalizing to a single spelling at the sanitization boundary.

5. `MCP_DIRECT_TOOLS` env var is not prefixed. Every other subagent env var uses the `${APP_NAME.toUpperCase()}_` prefix; `MCP_DIRECT_TOOLS` is bare (`pi-args.ts:217-221`). If this matches an upstream contract that the MCP loader depends on, fine; otherwise prefixing it would be consistent with the rest of the file.

Test coverage gaps

The four new tests are well-targeted, but several substantial new modules have no direct coverage:

• `fanout-child.ts` (246 lines, security-sensitive): no test for the child-safe registration guard, management-action blocking, control-inbox dedup/retry/aging, or the in-flight set. Even a couple of focused tests around "fanout child cannot create/update/delete agents" and "duplicate `requestId` is dropped without re-running" would catch the most important regressions.
• `result-intercom.ts` (108 new lines): `compactNestedRun` depth-clamping and `attachNestedChildrenToResultChildren`'s parent/index matching are pure functions ripe for unit tests.
• `nested-render.ts` (115 lines): `formatNestedAggregate`, `countNestedRuns`, and `formatNestedRunStatusLines` are deterministic and worth pinning, especially since they drive parent TUI output.
• `subagents-render-stability.test.ts` was trimmed from 148→1 lines. The description explains why (Atomic's local animation/remount layer was removed in favor of upstream rendering), but please confirm CI shows the deleted assertions are genuinely obsolete rather than silently passing because the imports they used disappeared.

Style / conventions

• `.ts` import specifiers used consistently. Good.
• New files follow the repo's `bun:test` + `node:assert/strict` pattern.
• `writeAtomicJson` used for the registry sidecar — matches the project's atomic-write convention.
• `nested-events.ts` is now 886 lines and `subagent-executor.ts` is over 1,000. Not a blocker, but a follow-up split (e.g., extracting `sanitize*` to its own module, splitting the executor into nested-control vs. resume vs. async-dispatch concerns) would help long-term maintenance.

Verification

I was unable to run `bun run typecheck` or `bun run test:unit` from this review sandbox, so I'm relying on the PR's stated `1385 passing` and clean typecheck. Recommend confirming CI is green before merging given the surface area.

Overall: solid sync with thoughtful Atomic-side preservation and good security primitives. The `spawnRunner` mode fix is the only thing I'd consider close to a blocker; the rest are nice-to-haves.

🤖 Generated with Claude Code

[claude]

test body

[claude]

Code Review — PR #1029

Thanks for the careful upstream sync. The new nested-fanout machinery is well-defended (capability tokens, path containment, bounded sizes, symlink rejection) and the unit tests for the data layer are thorough. A few items worth a second look:

Bugs / correctness

1. Race on the nested registry sidecar (packages/subagents/src/runs/shared/nested-events.ts in projectNestedEvents). The function reads registry.json, mutates it, then writeAtomicJsons it back without any lock. The header comment says "Parent projection is the only writer", but findNestedRouteForRootId then projectNestedEvents can be reached from the child fanout process via resolveSubagentRunId (and from multiple parent threads/timers concurrently). Atomic rename prevents corruption, but two readers projecting at the same time will both overwrite each others processedEvents -- fine for correctness because event application is idempotent, but it does mean events can replay until the registry "wins" once.

2. processedEvents cap under-sized for theoretical fanout. projectNestedEvents keeps slice(-1000), but with MAX_NESTED_DEPTH=3 times MAX_NESTED_CHILDREN=16 times multiple events per run, an active session can exceed 1000 event filenames. Once truncated, still-on-disk events get re-read on every poll. Either bump the cap (e.g. to ~5000) or also delete the on-disk files once their filename falls out of the dedupe window.

3. getFinalOutput behavior change (packages/subagents/src/shared/utils.ts). The function now skips assistant messages with errorMessage/stopReason === "error" and requires non-empty text. If every assistant message in a run has an error, callers now get an empty string instead of the prior error text -- verify downstream callers handle empty output gracefully (intercom payloads still get "(no output)" substitution, but other paths may not).

Style / consistency

4. Magic depth < 2 in compactNestedRun (packages/subagents/src/intercom/result-intercom.ts). nested-events.ts already exports MAX_NESTED_DEPTH; consider reusing it (or a sibling MAX_NESTED_RESULT_DEPTH) so depth limits stay coordinated.

5. MUTATING_MANAGEMENT_ACTIONS = new Set(["create", "update", "delete"]) is defined inline in subagent-executor.ts. If a new mutating action is added later this set has to be updated too -- consider deriving it from a single source of truth alongside the action enum.

6. Import extension convention: CLAUDE.md says "Source files use .js import extensions (TypeScript ESM convention)", but the new files (fanout-child.ts, nested-events.ts, mcp-direct-tool-allowlist.ts, nested-path.ts, nested-render.ts, run-id-resolver.ts) all use .ts for internal imports. This matches existing code inside packages/subagents/, so the convention seems to be .ts internally and .js only in tests -- worth either updating CLAUDE.md or aligning the files.

Performance

7. 200 ms hot-polling in startNestedControlInboxListener (fanout-child.ts). Fine for a short-lived child, but if many fanout children run concurrently this is N x 5 Hz of readdirSync on the inbox. fs.watch-based notification with a short polling fallback would be cheaper. Not blocking.

8. waitForNestedControlResult reads ALL control results every 50 ms (subagent-executor.ts). For a 1-second timeout this is 20 full directory scans plus JSON parses; consider tracking the highest-mtime cursor or filtering by filename before parsing.

Test coverage

9. fanout-child.ts has no dedicated unit test. The inbox listeners retry-with-cap, MAX_PENDING_RESULT_* thresholds, request dedupe (seen vs inFlight), and stale-cleanup behavior are non-trivial. A test that drives startNestedControlInboxListener with a synthetic pi.events plus injected route would catch regressions in this critical control surface.

10. Nested resume path (resolveNestedResumeTarget then validateNestedSessionFile) is security-sensitive (rejects symlinks, requires session file under a trusted root, rejects non-.jsonl). It would be worth a unit test covering the rejection cases -- symlinked session file, file outside trusted roots, missing run-id directory segment.

Security -- looks good

The defensive primitives are solid:

• Capability tokens in route metadata (createNestedRoute)
• containedPath/pathWithin guards on event sinks, control inboxes, async dirs, session files
• 0o600/0o700 perms on route files, control records, async config (with a regression test in subagents-async-config.test.ts)
• isSafeNestedPathId rejects "..", "/", "\", absolute, and over-length ids before any filesystem operation
• Sanitization caps on summary/step/event records (depth, breadth, byte length)
• Symlink rejection plus real-path resolution in validateNestedSessionFile

Nice work overall -- this is a defensible foundation. The race in (1) and event-replay churn in (2) are the only items I would want to see addressed before merging; the rest are quality-of-life suggestions.

---

Note: a stray "test body" comment (#issuecomment-4529407616) was posted earlier while validating the gh pr comment --body-file - stdin path. Feel free to delete it.

[claude]

Code Review — Subagents upstream sync (child-safe nested fanout + MCP allowlists)

Reviewed the new files (fanout-child.ts, nested-events.ts, nested-path.ts, nested-render.ts, mcp-direct-tool-allowlist.ts, run-id-resolver.ts, result-intercom.ts additions) and the wiring in pi-args.ts, subagent-executor.ts, async-execution.ts, and completion-guard.ts. Overall the security boundary is well thought through (capability tokens, route containment, depth/breadth clamping, gated mutating actions, fanout-only nested env injection). A few findings, ordered roughly by severity.

1. fs.writeFileSync mode does not retighten existing files — capability-token leak window

In packages/subagents/src/runs/background/async-execution.ts:175 and packages/subagents/src/runs/shared/nested-events.ts:672 both write secrets with the mode 0o600 option:

fs.writeFileSync(cfgPath, JSON.stringify(cfg), { mode: 0o600 });

Node's mode option only applies when the file is created — if a stale file at the same path exists from a previous run (or from before this fix landed) with 0o644, the new write keeps the old permissions. For writeRouteRecord this is a non-issue (filename includes randomUUID), but for writeAsyncRunnerConfig the path is deterministic: TEMP_ROOT_DIR/async-cfg-SUFFIX.json. The new test test/unit/subagents-async-config.test.ts:17 uses a Date.now-derived suffix so it never exercises the "stale file" case.

Suggested fix — explicitly create with truncation+mode, or unlink first:

const fd = fs.openSync(cfgPath, fs.constants.O_WRONLY | fs.constants.O_CREAT | fs.constants.O_TRUNC, 0o600);
try { fs.writeFileSync(fd, JSON.stringify(cfg)); } finally { fs.closeSync(fd); }
// or: try { fs.unlinkSync(cfgPath); } catch {}  then writeFileSync(..., { mode: 0o600 })

And ideally a regression test that pre-writes the path with 0o644 and asserts the mode is 0o600 after writeAsyncRunnerConfig.

2. sleepSync blocks the entire event loop under registry-lock contention

packages/subagents/src/runs/shared/nested-events.ts:434-436 uses Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) to spin-block in 10 ms increments for up to 2 s (REGISTRY_LOCK_TIMEOUT_MS). With multiple fanout children projecting the registry concurrently, this can stall the host event loop noticeably (status polling is already ~250 ms cadence). projectNestedEvents is sync because callers want immediate consistency, but the contention case is the worst time to block.

Options:

• Add an async variant projectNestedEventsAsync for the polling/update sites and keep the sync version only for the rare subagent action=status path.
• Keep sync but reduce the timeout, or fall back to a soft "registry temporarily unavailable" read of the last on-disk registry.json after ~500 ms.

At minimum, a code comment at the sleepSync definition spelling out that this intentionally blocks the loop and pointing to the bounded REGISTRY_LOCK_TIMEOUT_MS would help future maintainers.

3. Capability tokens appear in error logs

packages/subagents/src/extension/fanout-child.ts:168,170,183 and packages/subagents/src/runs/shared/nested-events.ts:494,574 log route.controlInbox / routeRoot paths on failure. Those paths embed the per-route capability token (the routeRoot is rootRunId-capabilityToken from createNestedRoute at line 127). If logs are aggregated to a shared sink the token is exfiltrated for the remaining lifetime of the route. Tokens are per-run and short-lived, so this is low-severity, but consider redacting to just route.rootRunId (or hashing the token) in error strings.

4. collectNestedRuns walks steps[].children and run.children separately without dedup

packages/subagents/src/runs/shared/nested-events.ts:511,531 flatten child.steps?.flatMap(step => step.children ?? []) to find/collect runs, but attachNestedChildrenToResultChildren in result-intercom.ts:138-144 also re-merges children from both the run-level and step-level lists. If the same child id appears under both run.children and a step's children (can happen during parallel chain steps), collectNestedRuns will yield duplicates and findNestedRunMatchesById may double-count. Worth a dedup pass on run.id in collectNestedRuns, or a unit test asserting "child appearing under both run.children and step.children is resolved once."

5. MAX_PROCESSED_NESTED_EVENTS = 20_000 can replay events if cleanup lags

projectNestedEvents (nested-events.ts:653) caps processedEvents at 20k via [...seen].slice(-MAX_PROCESSED_NESTED_EVENTS). Filenames are timestamp-prefixed and cleanupOldNestedRuntimeDirs removes them by mtime, so in normal use it self-balances. But a long-running root with a high-fanout burst could exceed 20k before cleanup runs — the oldest filenames would be dropped from the seen set and, if still on disk, re-applied. applyNestedEvent is idempotent for completed states (mergeSummary honors terminal), but "updated" states could regress visible run state to an older snapshot. The comment on line 652 ("worst-case bounded fanout") is reassuring; a guard like "skip events older than registry.updatedAt minus replayWindow" would make this water-tight.

6. Minor: expandTilde is ~/-only

fanout-child.ts:28-30 and the executor's expandTilde do not handle ~user/... or Windows-style backslash tilde paths. This is consistent with the rest of the codebase, so just noting it; if a user sets defaultSessionDir: "~me/subagents" in config they'd get a directory literally named ~me. Probably fine, but worth a TODO.

7. parseNestedEventRecords accepts a trailing trimmed single-line record without newline

nested-events.ts:367-368 has a fallback branch that parses single-line content. writeRouteRecord always appends a newline (line 666), so this branch only fires for malformed/legacy files. Fine, but worth a one-line comment so future readers don't assume single-record files are an expected format.

8. Test coverage observations (positive)

• test/unit/subagents-nested-events.test.ts covers route containment, legacy env resolution, control roundtrip, sanitization clamps, stale dir cleanup, lock stale-safety, processed-events cap, and nestedArtifactEnv. Good breadth.
• test/unit/subagents-pi-args.test.ts correctly asserts the fanout authorization gate ("read" alone leaves SUBAGENT_FANOUT_CHILD_ENV=0, "subagent" tool sets it to "1") and verifies the MCP_DIRECT_TOOLS=none sentinel contract.
• test/unit/subagents-run-id-resolver.test.ts covers the foreground-vs-async ambiguity throw and unsafe-id rejection.
• test/unit/subagents-mcp-direct-tool-allowlist.test.ts verifies prefix modes, exclusions, cache TTL, hash invalidation, and crucially that bearerToken presence (not value) is hashed — nice.

Gaps:

• No test for the writeAsyncRunnerConfig stale-file case (see finding 1).
• No test for fanout-child's MUTATING_MANAGEMENT_ACTIONS gate (subagent-executor.ts:2271) returning an isError for create/update/delete. Worth a test since this is a real security boundary.
• No test for startNestedControlInboxListener's retry/drop logic (the MAX_PENDING_RESULT_ATTEMPTS/MAX_PENDING_RESULT_AGE_MS paths in fanout-child.ts:86-90). A unit test that injects a failing writeNestedControlResult and asserts the seen-set growth + file unlink would lock the invariant in.

9. Style / convention

• nested-events.ts:39 — MAX_NESTED_DEPTH = SUBAGENT_PARENT_MAX_DEPTH is duplicated naming. Either reuse the existing constant directly at use sites or rename to drop the indirection.
• pi-args.ts:184-199 — eight near-identical fanoutAuthorized ternaries. A small helper setFanoutEnv(key, value) would shrink the block and reduce typo risk.
• fanout-child.ts:194-210 — keying cleanup-across-reload state on globalThis is pragmatic but unusual; a one-line comment explaining the reload scenario it protects against would help future readers (and ideally a test for the reload path).

TL;DR

Solid sync — the new boundaries (capability token, route containment, depth/breadth clamps, allowMutatingManagementActions: false, fanout-only env injection) are coherent and well-tested. The biggest follow-up is finding 1 (deterministic config path + writeFileSync mode semantics) since it directly weakens the documented owner-only-permissions fix. Findings 2-3 are also worth addressing before this hits a high-fanout production session.