fix(session): harden tool-call/result replay integrity and subagent dispatch

[flora131]

Summary

Refs #1527. Prevents orphaned toolResult messages from reaching Anthropic/GitHub Copilot providers after context compaction, and hardens subagent dispatch to reject duplicate concurrent calls. The fix spans session-replay reconciliation, LLM serialization guarding, and upstream subagent sync.

Changes

Session replay repair (packages/coding-agent)

• New session-manager-tool-dependencies.ts — reconciles persisted context_compaction deletion filters during active-context rebuild so deleting one side of a tool-call/tool-result pair always deletes or preserves the paired side consistently; converges via a fixpoint loop bounded by path.length * 2 passes.
• New repairOrphanToolResults in messages.ts — final LLM serialization guard that drops any toolResult message whose matching assistant toolCall is no longer the immediately preceding tool-use group, preventing provider replay failures.
• Wired reconcilePersistedToolDependencyFilters into buildEffectiveContextDeletionFilters in session-manager-history.ts.
• Docs compaction.md updated to document the tool-call/tool-result pair replay invariant and the two-layer guard strategy.

Upstream subagent hardening (packages/subagents)

• Duplicate dispatch guard subagent-executor.ts wraps execute with executeWithSingleDispatchGuard — rejects concurrent subagent dispatches within the same turn with a clear error; fanout-child.ts and index.ts initialize the new subagentInProgress flag in safe state.
• Fanout child history subagent-prompt-runtime.ts preserves nested subagent call/result history for fanout children (SUBAGENT_FANOUT_CHILD_ENV=1) and adds subagent-slash-text-result to the parent-only strip set.
• Failed-run diagnostics subagent-executor-single.ts formats failed foreground run output with the error message, captured child output, and output artifact path for easier debugging.
• Compact tool-call summaries prompt-template-bridge.ts embeds toolCall content blocks from delegated run summaries into delegation messages as readable text; fixes a null-safety gap when agent or task is missing from a prompt template request.
• Schema simplification schemas.ts removes provider-hostile allOf conditionals from the chain step schema.

Regression coverage

• New test/unit/subagents-upstream-sync.test.ts: duplicate dispatch rejection, fanout child history preservation, compact tool-call summary embedding, failed-run output formatting, schema keyword audit, and malformed fanout shape error.
• Extended test/session-manager/build-context-02.suite.ts: stale compaction split-pair replay cases (block-level and entry-level deletion).
• New packages/coding-agent/test/messages.test.ts: orphan toolResult repair in convertToLlm.

Validation

bun test test/unit/subagents-upstream-sync.test.ts
bunx --bun --no-install vitest --run test/session-manager/build-context-01.suite.ts test/session-manager/build-context-02.suite.ts test/messages.test.ts
bun run typecheck
bun run check:file-length
git diff --check

Pre-commit/pre-push hooks: bun run lint, bun run check:file-length, bun run test:unit

[claude]

Review: fix(subagents): harden tool-result replay (#1528)

Thanks for this — the defense-in-depth approach (filter reconciliation → final LLM guard) is the right shape for the Copilot/Anthropic orphan-toolResult class of bug, and the test coverage across messages.test.ts, build-context-02.suite.ts, and subagents-upstream-sync.test.ts is solid. A few things worth a look before merge.

Potential issue — synthetic toolCall blocks have no id (highest priority)

In prompt-template-bridge.ts, buildDelegationMessages now emits assistant toolCall blocks reconstructed from delegated summaries:

return name ? [{ type: "toolCall", name, arguments: { summary: ... } }] : [];

These blocks have no id and, by construction, no following toolResult. That's the symmetric mirror of the bug this PR fixes: repairOrphanToolResults only drops orphaned results, never orphaned tool calls. If one of these assistant messages reaches Anthropic/Copilot serialization, the provider rejects it with the "tool_use ids without tool_result" error — exactly the replay-failure family #1527 is about.

Could you confirm where the prompt-template:subagent:response messages are consumed, and that the host either (a) flattens these to text before provider serialization, or (b) assigns ids + synthesizes paired results? If neither, this needs an id + a paired synthetic toolResult, or these should stay as text parts. A regression test asserting the rebuilt message survives convertToLlm without leaving an unpaired tool-use would lock this down.

Minor — wasted work in buildDelegationMessages

toolCallParts is computed before the early if (Array.isArray(result.messages) && result.messages.length > 0) return result.messages;. When messages is present that flatMap is discarded. Move the toolCallParts computation below the early return.

reconcilePersistedToolDependencyFilters

• The fixpoint bound remainingPasses = Math.max(1, path.length * 2) plus a full callsById scan per pass is worst-case ~O(n²) on session length. It's a cold rebuild path so this is acceptable, but a one-line comment noting the loop normally converges in 1–2 passes and the bound is just a non-termination backstop would help future readers.
• The function mutates the passed-in filters in place and returns the same object. That's fine given buildEffectiveContextDeletionFilters constructs it fresh each call, but a doc comment making the in-place contract explicit would prevent a future caller from passing a cached/shared filter set.

Schema removal

Dropping the allOf/if/then/not conditionals from ChainItem is reasonable for provider compatibility, and the structural invariants do appear to be enforced at runtime (validateDynamicCollection, expandChainParallelCounts, the "First step in chain cannot be dynamic fanout" guard). Just confirm a malformed expand/collect/parallel shape now produces a clear user-facing runtime error rather than a confusing downstream failure, since the schema no longer rejects it up front.

Nits

• The single-dispatch guard (executeWithSingleDispatchGuard) looks correct — action calls bypass it and the flag is cleared in a finally. 👍
• getToolResultCallId / tool-call-id collection helpers are now duplicated between messages.ts and session-manager-tool-dependencies.ts. Minor, but a shared helper would avoid drift if the message shape changes.

Overall a careful, well-tested change; the synthetic-toolCall-without-id question is the one thing I'd want resolved before merge.

[flora131]

Addressed in 1d31f7384:

• Changed compact delegated tool-call summaries to be flattened into text (Tool calls:\n- ...) instead of synthetic toolCall blocks, so prompt-template responses cannot introduce unpaired tool-use history.
• Moved summary processing after the existing result.messages passthrough early return.
• Added a regression assertion that the rebuilt prompt-template response survives convertToLlm without tool-call parts.
• Added a clear runtime-error test for malformed dynamic fanout shapes now that provider-hostile schema conditionals are removed.
• Documented that reconcilePersistedToolDependencyFilters mutates fresh filters in place and that its bounded loop is only a non-termination backstop.

Validation reran locally and via hooks: targeted Bun/Vitest tests, bun run typecheck, bun run check:file-length, and pre-push bun run test:unit.

[claude]

Review: fix(subagents): harden tool-call/result replay integrity

Solid, well-scoped fix for #1527. The defense-in-depth approach — reconciling persisted compaction filters at active-context rebuild and a final orphan-drop guard at LLM serialization — is the right shape, and the new code is clean, typed without any/unknown escape hatches (narrow as { ... } shape probes), and well-documented. Files stay under the 500-line gate and the changelog/docs are updated per convention. Findings ordered by impact.

Correctness / potential issues

1. repairOrphanToolResults doesn't narrow allowedToolCallIds after consuming a result (messages.ts). Once a toolResult is kept, its toolCallId stays in the allowed set, so a duplicate toolResult with the same id would also pass through. Anthropic requires exactly one tool_result per tool_use, so a duplicate would still trigger the provider replay failure this guard exists to prevent. Likely unreachable in practice, but since provider-safety is the function's whole purpose, consider allowedToolCallIds.delete(toolCallId) after a match — or at least a comment noting duplicates aren't handled. Low priority.

2. Single-dispatch guard only spans the synchronous/foreground window. In executeWithSingleDispatchGuard, subagentInProgress is set, then await execute(...), then cleared in finally. But execute takes the runAsyncPath early-return for async dispatches (subagent-executor.ts:227) and returns as soon as the job is kicked off, not when it completes. So the flag clears almost immediately for async runs, and two async subagent calls in the same turn are not rejected — only truly concurrent foreground dispatches are. If intentional (async is fire-and-forget), fine; if the intent matches the user-facing message "Issue exactly ONE subagent call per turn", async slips past it. Worth confirming/documenting the carve-out.

Test coverage

3. The retainedThinkingCall restoration branch is untested. reconcilePersistedToolDependencyFilters has a subtle branch (session-manager-tool-dependencies.ts:256-262) that restores a deleted toolResult when a thinking-bearing tool-call ref is retained — since thinking-bearing assistant messages can't have blocks deleted individually. Neither new build-context-02.suite.ts case exercises a thinking-bearing assistant whose paired result was deleted; both hit the callDeleted path or the non-thinking delete path. This is the subtlest invariant in the PR and the one most likely to regress against provider extended-thinking rules — a direct test (ideally importing reconcilePersistedToolDependencyFilters directly to document the fixpoint contract) would be valuable.

   Otherwise coverage is good — duplicate-dispatch rejection, fanout history preservation, compact tool-call embedding, failed-run formatting, schema-keyword omission, and orphan repair are all covered, including the management-action bypass.

Minor / nits

4. prompt-template-bridge.ts null-safety — replacing fallbackTask!.agent non-null assertions with if (!agent || !task) return undefined is a genuine improvement. It also tightens the hasSingle path (now rejects single requests missing agent/task) — looks intentional and correct.

5. Schema allOf removal (schemas.ts) — dropping the provider-hostile if/then/not conditionals is justified, and the new malformed-fanout test confirms validation moved to runtime rather than being lost. The omits provider-rejected chain schema keywords test is a nice regression lock.

6. Fixpoint bound — Math.max(1, path.length * 2) runs on every rebuild, but collectToolReferences early-returns with no calls/results and changed short-circuits, so normal sessions converge in 1–2 passes. The backstop doc comment is appreciated. No concern.

Summary

No blocking bugs. Recommend (a) a test for the thinking-bearing restoration branch (#3), and (b) clarifying the async carve-out in the dispatch guard (#2). #1 is a low-probability hardening nit. Nice work — the docs + changelog discipline here is exemplary.

🤖 Automated review by Claude