Skip to content

feat(tui): auto-fork read-only same-route children onto the parent's cached prefix#4600

Merged
Hmbown merged 5 commits into
mainfrom
agent/091-cache-breakpoints
Jul 20, 2026
Merged

feat(tui): auto-fork read-only same-route children onto the parent's cached prefix#4600
Hmbown merged 5 commits into
mainfrom
agent/091-cache-breakpoints

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Token-cost driver #1 from the 2026-07-19 investigation: every subagent cold-started by default, re-prefilling system prompt + tools + a re-discovery crawl of context the parent already paid for (~100K input per child observed), even though the fork mechanism (per-turn engine snapshot + with_fork_context) was fully wired — the only fork_context: true in the tree was a unit test.

Design

fork_context is now tri-state at the agent tool boundary:

  • explicit true/false — unchanged, always wins;
  • omitted — resolved at spawn time by auto_fork_context_default, which forks only when it is both cheaper and coherent:
    1. the spawner is the engine turn itself (a nested spawner's snapshot is the root prefix, not its own conversation);
    2. the child resolved to the exact parent provider+model route (any other route pays a full cold prefill of the parent context);
    3. the child posture is read-only (explore/plan/review/verifier, or explicit read_only write authority);
    4. the child stays in the parent workspace (no worktree/cwd isolation);
    5. the estimated parent prefix is ≤ 200K tokens — above that, cached fork reads (~10% of the prefix per child step) cost more than a fresh brief.

Everything else keeps the fresh-brief default. The tool schema documents the tri-state so the model is aware of the tradeoff in one sentence; the Agent-mode prompt contract moved from "Fresh sessions are the default" to the auto-chosen contract (test updated with it).

Related findings (no code needed)

Evidence

  • New matrix test: postures, read_only authority upgrade, worktree/cwd isolation, cross-model, cross-spawner, oversized-prefix ceiling.
  • Parse tri-state coverage (explicit true / inherit_context / omitted → None).
  • Full locked TUI suite 7,612 passed / 0 failed / 3 ignored pre-merge and 515 focused prompts/subagent tests post-merge with main (post-feat(tui): compress the Agent mode prompt without losing tested invariants #4597 prompt reconciled); strict all-target/all-feature locked clippy clean.

Hmbown added 4 commits July 19, 2026 19:47
…efix

Token-cost driver #1: every subagent cold-started by default, re-prefilling
system prompt + tools + a re-discovery crawl of context the parent already
paid for, even though the fork mechanism (engine per-turn snapshot +
with_fork_context) was fully wired — the only fork_context: true in the
tree was a unit test.

fork_context is now tri-state at the tool boundary: explicit true/false
still wins, and an omitted field resolves at spawn time via
auto_fork_context_default. The auto policy forks only when it is both
cheaper and coherent: the spawner is the engine turn itself (a nested
spawner's snapshot is the root prefix, not its own conversation), the
child resolved to the exact parent provider+model route (any other route
pays a full cold prefill of the parent context), the child posture is
read-only (explore/plan/review/verifier, or an explicit read_only write
authority), and the child stays in the parent workspace without worktree
or cwd isolation. Write-capable, isolated, or re-routed children keep the
fresh-brief default.

The agent tool schema documents the tri-state; the Agent mode prompt and
the fork-context contract test moved from 'Fresh sessions are the default'
to the auto-chosen contract.

Verified locally: 398 subagent tests (new parse tri-state coverage and the
auto-fork decision matrix: postures, authority upgrade, worktree/cwd
isolation, cross-model, cross-spawner), 102 prompt+fork tests, strict
all-target/all-feature locked clippy, fmt.

Signed-off-by: Hunter B <hmbown@gmail.com>
Forking is only economical while the provider's cached-read rate on the
parent prefix stays below the cost of a fresh brief plus re-discovery;
every child step re-reads the whole forked prefix. Above an estimated
200K-token parent prefix the auto policy now keeps the fresh default.
Explicit fork_context: true is deliberately not subject to the ceiling.

The estimate walks the snapshot's text/thinking/tool blocks with the
conservative compaction estimator. Matrix regression added.

Verified: 399 subagent tests, strict locked clippy, fmt.
Signed-off-by: Hunter B <hmbown@gmail.com>
…mode

Merges origin/main (post #4597 compression) and keeps the auto-chosen
fork_context sentence in the compressed style; token ceiling moves
1350 -> 1360 for the added contract sentence.

Signed-off-by: Hunter B <hmbown@gmail.com>
Copilot AI review requested due to automatic review settings July 20, 2026 03:09
@cursor

cursor Bot commented Jul 20, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 27s —— View job


Claude finished reviewing

Summary

This is a clean, well-scoped fix for a real cost bug: fork_context defaulted to false everywhere, so the already-wired per-turn snapshot/with_fork_context machinery went unused outside a unit test. Making the field tri-state (Option<bool>) at the tool boundary and adding auto_fork_context_default to resolve None at spawn time is the right shape — explicit caller intent still always wins, and the auto policy is conservative (route-exact, read-only, same-workspace, size-capped).

No correctness bugs found. Highlights:

  • Tri-state migration is complete. spawn_request.fork_context (now Option<bool>) has exactly one read site (mod.rs:5795), so there's no stray code left assuming the old bool default. parse_optional_bool(...) no longer collapses to .unwrap_or(false), and the schema correctly leaves fork_context out of required.
  • Provider/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608). auto_fork_context_default gates on child_provider == parent_runtime.client.api_provider() (enum equality) and effective_model == parent_runtime.model (exact resolved wire-id equality) — both compared after the existing resolve_subagent_assignment_route / ensure_subagent_model_for_provider pipeline runs. This is a correctness check for cache-prefix byte-identity, not an inference from a provider-prefixed string, so it doesn't run afoul of the invariant.
  • Nested-spawner guard (parent_runtime.parent_agent_id.is_some() → never auto-fork) correctly prevents a sub-agent's own conversation from being mistaken for the root engine prefix.
  • Tests (auto_fork_context_default_forks_only_same_route_read_only_engine_children) cover the full matrix from the PR description: all four read-only postures, read_only authority upgrade on a General child, write-capable staying fresh, worktree/cwd isolation, cross-model, cross-spawner, and the 200K oversized-prefix ceiling. Good coverage; nothing obviously missing.

Minor, non-blocking notes

  1. estimated_fork_prefix_tokens (mod.rs:~7970-7994) mixes two token-estimation methods: crate::compaction::estimate_text_tokens_conservative for Text/Thinking/ToolResult, but a bare input.to_string().len() / 4 for ToolUse. Not wrong, but worth reusing the same conservative estimator for consistency if it's cheap to do so.
  2. The _ => {} catch-all in that same match silently contributes 0 for ImageUrl, ServerToolUse, and ToolSearchToolResult blocks. For a tool-result/image-heavy parent prefix this could under-count the estimate and let a genuinely oversized prefix through the 200K auto-fork ceiling. Low severity since this is a soft cost heuristic (explicit fork_context: true bypasses the ceiling anyway), but worth a // TODO or a follow-up if multimodal parent contexts are common in practice.
  3. No CHANGELOG entry in this diff — consistent with the immediately preceding prompt-compression PR (feat(tui): compress the Agent mode prompt without losing tested invariants #4597), so not flagging as a gap.

Verification caveat

I was not able to run cargo build/cargo test/cargo clippy in this session — the sandbox declined approval for cargo invocations. The PR description reports a full locked-suite pass (7,612 passed / 0 failed / 3 ignored) and clean strict clippy pre-merge, so I'm relying on that self-report rather than independent verification here. If build/test tool access can be granted to --allowedTools, I can re-run and confirm directly next time.

The auto-fork policy now (correctly) forks explorer children onto the
parent prefix, so their requests carry the parent conversation — which
broke this harness's content-based mock dispatch (child matcher excluded
requests containing the parent prompt). The crafted tool call now passes
fork_context: false explicitly, which always wins over the auto policy,
keeping the harness deterministic and exercising the explicit-fresh path.

Verified locally: release_runtime_qa 5 passed/1 ignored; qa_pty 17/1,
tool_lifecycle 4, plugin_e2e 43, integration_mock_llm 3 all green.

Signed-off-by: Hunter B <hmbown@gmail.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 36s —— View job


Claude finished reviewing

  • Gather context (PR body, diff stat, CLAUDE.md)
  • Read full diff (git diff origin/main...HEAD)
  • Review crates/tui/src/tools/subagent/mod.rs changes (core logic)
  • Review crates/tui/src/tools/subagent/tests.rs (new coverage)
  • Review crates/tui/src/prompts.rs / agent.md prompt changes
  • Review crates/tui/tests/release_runtime_qa.rs changes
  • Check provider/model/route safety invariant (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608)
  • Post final review

Summary

This sync added two small commits on top of what I reviewed in my previous pass (beaa117, eb0d815): 07b534b (reconcile the auto-fork prompt sentence with the unrelated #4597 prompt-compression merge) and 43cd96f (pin the six-worker QA harness to explicit fork_context: false). Both are narrow, mechanical, and correct — no new logic beyond what I already reviewed in the core auto_fork_context_default implementation.

  • 07b534b — after merging origin/main (post-feat(tui): compress the Agent mode prompt without losing tested invariants #4597 compression), the auto-fork contract sentence in agent.md:32 was kept in the compressed style (`fork_context` is auto-chosen: read-only children on the parent's exact route fork the byte-identical parent prefix...) and the token ceiling test in prompts.rs:2988 moved 1350 → 1360 to cover the added sentence. The comment explains why. Consistent with prompts.rs:3357-3359's assertions, which now check for the new phrasing (`fork_context` is auto-chosen, write-capable, isolated, or re-routed children start fresh) instead of the old "Fresh sessions are the default" string. Nothing to flag.
  • 43cd96f — this is the correct fix for a real interaction the auto-fork change surfaced: release_runtime_qa.rs's six-worker fanout QA harness dispatches mock SSE responses by matching on request content, and an auto-forked explorer child now carries the parent conversation (including the parent prompt) in its request body, which broke that content-based matcher. Adding "fork_context": false to the crafted tool call (release_runtime_qa.rs:94) is exactly right — explicit false always wins over the auto policy per the spawn_request.fork_context.unwrap_or_else(...) resolution in mod.rs:5795, so this keeps the harness deterministic while still exercising the explicit-fresh path. The inline comment justifies the choice well.

No correctness bugs, no provider/route-safety issues, nothing to add to my earlier findings from the core auto_fork_context_default review (tri-state migration, route-exact/read-only/same-workspace/size-capped gating, nested-spawner guard, and matrix test coverage) — those still stand and remain unaffected by this sync.

Still open from the previous review (non-blocking, unchanged by this sync)

  1. estimated_fork_prefix_tokens (mod.rs:~7970-7994) mixes estimators: crate::compaction::estimate_text_tokens_conservative for Text/Thinking/ToolResult, but a bare input.to_string().len() / 4 for ToolUse.
  2. The _ => {} catch-all in that match silently contributes 0 for ImageUrl/ServerToolUse/ToolSearchToolResult blocks, which could under-count a multimodal parent prefix against the 200K auto-fork ceiling. Low severity — explicit fork_context: true bypasses the ceiling anyway.

Verification caveat

Same as before: I could not run cargo build/test/clippy in this sandbox (no network/tool approval for cargo invocations, and git fetch was also declined here). The PR description's commit 43cd96f reports local verification (release_runtime_qa 5 passed/1 ignored; qa_pty 17/1, tool_lifecycle 4, plugin_e2e 43, integration_mock_llm 3 all green), so I'm relying on that self-report rather than independent confirmation.

@Hmbown
Hmbown merged commit 0aafd6a into main Jul 20, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants