Reuse completed subagent sessions

[brynary]

Summary

• retain a child Session after a successful task so the same subagent can receive later remediation work
• route input by lifecycle state: queue it for a running agent, start a new generation for a completed agent, and reject it for a closed agent
• make waits and parent notifications generation-aware
• allow completed agents to be closed explicitly
• add agent.sub.turn.started and generation metadata while preserving generation 1 defaults for older stored events
• update run projections and verbose CLI progress without creating a second projected subagent row

Why

Review findings currently require fresh remediation agents after the original implementer finishes. Each replacement must reload the work order, plan, style guides, code, and review context. This change keeps the original child session and history available for follow-up work during the lifetime of the parent supervisor.

Concurrency behavior

The completion commit and send_input use one state ordering. Input accepted before the completion commit stays in the current generation. Input accepted after that commit starts the next generation. Stored per-generation results prevent a later turn from hiding the result a waiter originally targeted.

Scope

Session reuse lasts for the current parent supervisor lifetime. This change does not add cross-process subagent persistence or resume support.

Validation

• cargo check --tests --workspace
• cargo +nightly-2026-04-14 fmt --check --all
• cargo +nightly-2026-04-14 clippy --workspace --all-targets --all-features -- -D warnings
• 2,605 tests passed across fabro-agent, fabro-types, fabro-workflow, and fabro-store
• 966 fabro-cli tests passed
• no pending snapshots

[brynary]

Reviewer note

Suggested review order:

1. lib/components/fabro-agent/src/subagent.rs contains the lifecycle and concurrency change.
2. lib/components/fabro-agent/src/types.rs and fabro-types/src/run_event/ define generation-aware events and backward-compatible defaults.
3. lib/components/fabro-store/src/run_state.rs and the run-progress files show how reuse remains one projected agent while its status returns to running.

The main invariants to check are:

• A running send_input and the final completion commit acquire supervisor state before the follow-up queue. This makes the boundary deterministic and prevents accepted input from being lost.
• A completed send_input reserves command-channel capacity before changing the agent to running or incrementing its generation.
• Each wait captures one generation and reads that generation result from the result map, even if watch updates coalesce with a later turn.
• The child runner owns one Session and only shuts it down on explicit close or parent-supervisor shutdown.
• Reuse emits agent.sub.turn.started, not another spawn event. The lifecycle event queue preserves completion-before-next-turn ordering.

The areas most worth extra scrutiny are shutdown races, parent-notification suppression across generations, and the state/follow-up lock ordering. The focused tests near the end of subagent.rs cover completed-session history reuse, running follow-ups, generation-pinned waits, repeat notifications, idle close, callback re-entry, and cleanup races.

[brynary]

Review focus

Please focus review on these four questions:

1. Can accepted input be lost at completion? In subagent.rs, both running send_input and commit_turn_result take the supervisor-state lock before the follow-up-queue lock. Input before the commit must remain in the current generation; input after it must start the next generation.
2. Can an agent become running without queued work? The completed-agent path reserves bounded command-channel capacity before it increments the generation or changes status to running.
3. Does retained-session cleanup remain bounded? A completed child intentionally keeps its Session alive, but close_agent, shutdown_all, supervisor drop, cancellation, and the shutdown timeout must still stop and join the runner and event-forwarder tasks.
4. Do consumers see reuse as one logical agent? A reused child emits agent.sub.turn.started rather than another spawn. Legacy events default to generation 1, and the run projection must update the original row instead of appending one.

Intentional semantics to validate:

• input sent while running is part of the current generation
• input sent after completion starts a new generation with the same conversation history
• a wait targets the generation current when the wait begins
• reuse lasts only for the current parent supervisor lifetime

The most relevant regression tests are send_input_to_completed_agent_reuses_its_session_and_history, send_input_to_running_agent_joins_the_current_generation, wait_returns_its_target_generation_after_a_later_turn_starts, a_reused_agent_delivers_each_generation_to_the_parent, and close_completed_agent_closes_its_idle_session.

[Copilot]

🟡 Not ready to approve

send_input can silently fail to dispatch a new-generation start command (dropped mpsc receiver) after already flipping the agent to Running, risking stuck waits/notifications.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR enhances Fabro’s subagent lifecycle so completed child Sessions can be reused for later remediation work, adding explicit per-turn generation tracking and a new agent.sub.turn.started event to keep projections/CLI progress coherent without creating duplicate subagent rows.

Changes:

• Add a generation concept to subagent lifecycle events and introduce agent.sub.turn.started to represent subsequent turns in the same child session.
• Update event conversion, run projections, and CLI progress rendering to be generation-aware while preserving legacy defaults (generation = 1).
• Refactor the subagent supervisor to support queuing input, starting new generations for completed agents, and explicitly closing completed agents.

File summaries

File	Description
lib/foundation/fabro-types/src/run_event/mod.rs	Adds agent.sub.turn.started event body and tests for generation defaults on legacy events.
lib/foundation/fabro-types/src/run_event/agent.rs	Adds generation fields (defaulting to 1 where needed) and defines AgentSubTurnStartedProps.
lib/components/fabro-workflow/src/handler/llm/api.rs	Updates test event construction to include generation.
lib/components/fabro-workflow/src/event/names.rs	Maps new SubAgentTurnStarted to agent.sub.turn.started and updates tests.
lib/components/fabro-workflow/src/event/convert.rs	Converts new/updated agent events into typed run event bodies with generation metadata.
lib/components/fabro-store/src/run_state.rs	Updates run projection reducer to handle AgentSubTurnStarted by returning subagents to Running.
lib/components/fabro-agent/src/types.rs	Adds generation to subagent lifecycle events, introduces SubAgentTurnStarted, and adds legacy serde default test.
lib/components/fabro-agent/src/subagent.rs	Implements session reuse via per-generation results, turn commands, lifecycle event draining, and explicit close semantics.
lib/components/fabro-agent/src/profiles/claude5_tools.rs	Updates tool descriptions to reflect that completed agents can be messaged/stopped; strengthens schema tests.
lib/components/fabro-agent/src/cli.rs	Prints generation metadata for subagent lifecycle events and adds output for turn start.
lib/apps/fabro-cli/src/commands/run/run_progress/stage_display.rs	Adds verbose UI line for subagent turn starts (includes generation).
lib/apps/fabro-cli/src/commands/run/run_progress/mod.rs	Wires new progress event through to stage display and updates snapshots/tests.
lib/apps/fabro-cli/src/commands/run/run_progress/event.rs	Adds ProgressEvent::SubagentTurnStarted and maps it from stored run events.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[brynary]

Simplification pass

Ran a three-way review (reuse, quality, efficiency) over the branch diff and applied the findings in 1bb70ad. No intended behavior changes; 7,680 workspace tests pass, clippy and fmt clean, no pending snapshots.

Duplication removed

• One definition of the initial generation, exported from fabro-types, replacing three copies.
• One SubAgentHandle per child instead of threading state / event_callback / notifications_changed / agent_id / depth through five functions. commit_turn_result goes from 8 params to 3, run_subagent_session from 10 to 6, and the repeated signal-then-drain pairs become publish().
• SupervisorState::agent / agent_mut replace five copies of the same not-found error.
• One ProgressEvent::SubagentStarted and one display method for spawn and later turns; the glyph and turn N label derive from the generation, so CLI output is unchanged.
• One set_subagent_status helper replaces four identical reducer arms.

Correctness and robustness

• reusable moved inside SubAgentStatus::Finished, so reusable-plus-closed is no longer representable.
• The lifecycle draining flag is now cleared by an RAII guard. Previously a panicking callback left lifecycle_draining true forever, silently dropping every later lifecycle event.
• A session that fails to initialize is torn down immediately. It was parking on runner_stop.cancelled() first, holding the session and its sandbox until the parent closed the agent — a regression against pre-PR behavior.

Dead code

• The unreachable cleanup_started branch in begin_shutdown (reaching it implies status was Running or Finished, which implies the flag was false) and the field itself; a debug_assert! records the invariant.
• Test-only emit_event, whose only caller was the test asserting emit_event does not panic. The real no-callback path is drain_lifecycle_events, covered by every other test.

Tests

• wait_returns_its_target_generation_after_a_later_turn_starts now drives spawn and send_input instead of hand-writing results, status, and generation under the state lock. It never exercised the code that creates a generation, and it hardcoded the representation it was meant to test.

Left alone, flagged for review

1. A failed turn's error is dropped when a follow-up is queued (subagent.rs, commit_turn_result). The follow-up is claimed on reusable alone, and reusable is true after a failed turn because the session returns to Idle on error. So a turn that fails while input is queued continues in the same generation with no agent.sub.failed event and no results entry. Gating on result.is_ok() would instead strand the accepted input in the queue with the agent Finished, so this needs your call on intended semantics rather than a mechanical fix.
2. results and agents never shrink. One SubAgentResult per generation, output string included, is retained for the parent session's lifetime. Bounded by turns times subagents, so likely harmless in practice. The clean fix is a per-generation watch channel per turn, which pins the generation structurally and frees on the last waiter drop — too invasive for this pass.
3. The projection keeps the spawn task across turns while the CLI shows each turn's task. A test asserts the current behavior, so I documented the choice in the reducer rather than reverse it. Worth confirming that is the contract you want for SubAgentProjection.task.
4. supervise_test_task no longer mirrors the production runner. It awaits a Result-shaped child task where production awaits JoinHandle<()>, so the abort-after-grace tests exercise a shape production no longer produces. Its remaining purpose — simulating a child that ignores cancellation — is hard to script through a real session, so I only removed the duplicated commit logic.
5. agent.sub.turn.started is written in four hand-maintained places (serde rename, event_name(), is_known_event_name, names.rs) with nothing asserting they agree. CLAUDE.md's strum rule covers this; it is pre-existing and out of scope here.
6. Copilot's permit.send finding is a false positive. OwnedPermit::send returns (), not a Result, so there is no send result to ignore. The only interleaving that drops a Start needs a close between releasing the state lock and the send, and that path already drives the agent to Closed and releases waiters.

Also undocumented: the new event is missing from docs/internal/events.md, docs/public/agents/subagents.mdx, and docs/internal/fabro-event-schema-v2-concrete-shape.md, which list every other agent.sub.* name.

[Copilot]

🟡 Not ready to approve

send_input() can leave a reused subagent stuck in Running if the reserved command permit fails to send, causing waits/notifications to hang.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[brynary]

Second pass: efficiency review

The efficiency reviewer's findings landed after the first pass. One was a real bug, fixed in 8c19b1e.

Fixed

• The event forwarder stopped permanently on the first broadcast lag. while let Ok(event) = rx.recv().await exits on RecvError::Lagged, but a lagged receiver stays usable. One transient lag silenced that child's events for the rest of its life, and because the task completed normally, run_shutdown joined it and noticed nothing. Reuse widens the exposure from a single turn to the whole parent session. Now Lagged continues and only Closed breaks.
• A per-notification clone of the whole result output, replaced with a borrow.

Confirmed clean by that review (recorded so it isn't re-litigated): no std lock is held across an await; signal_notifications and drain_lifecycle_events run after guard release at all seven sites; the lifecycle_draining flag is race-free; task ownership is complete across begin_shutdown, run_shutdown, the drop impls, and the shutdown timeout, with no path leaking a task or session; send_input holds the state lock across try_reserve_owned, so the capacity-1 channel cannot spuriously reject; the projection and CLI reducers are O(1) per event.

Additional items left for you

• Retention has no bound. Depth is capped, but the number of spawned agents is not, and nothing auto-closes a finished one — close_agent is model-driven. Each retained agent holds its Session plus three parked tasks. Compaction caps each child's history at ~80% of the context window, and children share the parent's sandbox Arc with no per-child MCP, so the estimate is roughly 0.5-1 MB per finished agent times the spawn count. Worth an explicit cap on retained-idle sessions or an idle deadline in the runner's select!.
• The newest result is retained twice, once in results[generation] and once in the Finished status payload. An Arc in both would halve it; I left it because it changes a public payload type and the pattern matches in several tests, and it pairs naturally with whatever you decide about the retention bound above.
• turns_used can report 0. It differences history lengths across a generation, and compaction shrinks turns(), so saturating_sub collapses to zero. Pre-PR this metric was the raw history length, so the per-generation intent is new; counting turns appended during the generation would fix it.

Not worth changing: two to_string calls in send_input, a double agent_id clone and O(n) retain in the notification batch, and one retained MultiProgress bar per reuse turn in verbose TTY mode — all bounded and consistent with the surrounding code.

Verification for both commits: 7,680 workspace tests pass, clippy --workspace --all-targets -D warnings clean, fmt clean, no pending snapshots.

[Copilot]

🟡 Human review recommended

It changes core subagent lifecycle/concurrency behavior across multiple crates (agent supervision, event logging, projections, and CLI rendering) and warrants focused human review of edge cases and ordering guarantees.

Review details

Suppressed comments (1)

lib/components/fabro-agent/src/subagent.rs:846

• suppress_parent_notification currently clears pending_generations but keeps parent_notification intact, so a later send_input on a completed/reusable agent will push a new generation into pending_generations and automatic delivery will resume. If suppression is meant to be permanent for the agent (as the name suggests), this should probably set agent.parent_notification = None; if it’s meant to be per-generation, the naming/docs should make that explicit to avoid surprising duplicate notifications after reuse.

        }
    }

    /// Stop automatic delivery for an agent whose result the parent retrieved
    /// explicitly.
    pub(crate) fn suppress_parent_notification(&self, agent_id: &str) {
        let cleared = {
            let mut state = self.state.lock().expect("subagent state lock poisoned");

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

It introduces substantial concurrency and lifecycle-state refactoring around reusable subagent sessions (generation-aware results, waits, notifications, and shutdown), which warrants final human verification.

Review details

Suppressed comments (1)

lib/apps/fabro-cli/src/commands/run/run_progress/event.rs:204

• With subagent session reuse, multiple completion events for the same agent_id can occur (one per generation), but ProgressEvent::SubagentCompleted doesn't carry generation, so verbose progress output can become ambiguous when events interleave across agents. Consider including generation on SubagentCompleted (and printing it similarly to SubagentStarted) so each completion can be attributed to the correct turn.

    SubagentCompleted {
        stage_node_id: String,
        agent_id:      String,
        success:       bool,
        turns_used:    u64,

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.