From c4971b93d32a950bb971fa3c5902e083c8905b2e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 25 Jul 2026 14:54:38 -0400 Subject: [PATCH 01/36] fix(timing): accumulate active time for in-flight stages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `active_time_ms` was only ever computed from terminal stage events, so a stage still running contributed zero to the run rollup. A run parked in one long agent stage reported 2m 8s of active time against 16m 53s of wall clock — the two finished stages — while the running stage had been doing continuous inference and tool work for over 14 minutes. `live_run_timing` summed `filter_map(|stage| stage.timing)`, and `stage.timing` is only written at finalization. Wall time ticked live off `start_time`; active time did not tick at all. Stage projections now accumulate brackets from the event log: - Closing an inference bracket folds its span into `live_inference_ms` instead of discarding it, including across retries, matching the in-process stopwatch. - Tool calls open a batch on the first outstanding call and close it when the last one drains, so tools running concurrently within a turn count once — the same span `execute_tool_calls` is bracketed by. Summing per-call durations would over-count parallel tool use. Subagent tool events are excluded; they run inside the root call's span already. - `StageProjection::live_timing(now)` composes accumulators with any open bracket, per handler: agent stages use the brackets, prompt and command stages count elapsed time as inference and tool respectively, and handlers that wait on a human, timer, condition, or child branches report zero. Active is clamped to wall per stage. A worker killed mid-turn leaves its bracket open forever, and without the clamp it would tick up unbounded. The clamp does not need to detect the dead worker: a stage cannot have been active longer than it has existed. `watchdog.timeout` remains the authority on whether a run is stuck. The clamp is deliberately not applied at run level, where concurrent branches can legitimately sum past run wall time. Timing is derived from events rather than emitted by the worker, so this needs no event-schema change and applies to runs already stored. `StageProjection.timing` keeps its terminal-only meaning, and the authoritative breakdown still replaces the live estimate at terminal events. The billing endpoint had the same hole behind its `wall_only` fallback: running stages reported zero inference/tool/active. Not visible in the product, which renders only `wall_time_ms`, but wrong for any other consumer of `GET /runs/{id}/billing`. Parallel branch stages lose their breakdown permanently, even after completion, because `parallel.branch.completed` carries only `duration_ms`. That is a separate data-loss bug, tracked in #644. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/routes/run-detail/header.tsx | 9 +- ...05-21-wall-and-active-time-metrics-plan.md | 11 +- docs/public/api-reference/fabro-api.yaml | 66 ++- .../src/server/handler/billing.rs | 16 +- lib/components/fabro-store/src/run_state.rs | 475 +++++++++++++++++- .../fabro-types/src/run_projection.rs | 462 ++++++++++++++++- lib/foundation/fabro-types/src/timing.rs | 26 + .../src/.openapi-generator/FILES | 1 + .../fabro-api-client/src/models/index.ts | 1 + .../fabro-api-client/src/models/run-timing.ts | 2 +- .../src/models/stage-projection.ts | 12 + .../src/models/stage-timing.ts | 2 +- .../src/models/stage-tool-batch-projection.ts | 29 ++ 13 files changed, 1060 insertions(+), 52 deletions(-) create mode 100644 lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts diff --git a/apps/fabro-web/app/routes/run-detail/header.tsx b/apps/fabro-web/app/routes/run-detail/header.tsx index efc68f09b..3b7081596 100644 --- a/apps/fabro-web/app/routes/run-detail/header.tsx +++ b/apps/fabro-web/app/routes/run-detail/header.tsx @@ -326,6 +326,7 @@ function DurationPopover({ }) { const endMs = completedAt != null ? Date.parse(completedAt) : now; const sinceCreatedMs = Math.max(0, endMs - Date.parse(createdAt)); + const isRunning = completedAt == null; return ( <> Duration @@ -335,8 +336,14 @@ function DurationPopover({
{formatDurationMs(sinceCreatedMs)}
-
Active (inference + tools)
+
+ Active (inference + tools){isRunning ? " — estimated" : ""} +
{formatDurationMs(timing.active_time_ms)}
+
+ {formatDurationMs(timing.inference_time_ms)} inference ·{" "} + {formatDurationMs(timing.tool_time_ms)} tools +
diff --git a/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md b/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md index 90438d3b5..3683e5879 100644 --- a/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md +++ b/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md @@ -157,7 +157,14 @@ git diff --check provider-reported model-only compute time. - LLM retry backoff, queueing outside a request/stream, human waits, steering waits, and scheduler gaps are wall time but not active time. -- Active timing is finalized-event based in v1; live active-time ticking can be - added later if it becomes necessary. +- ~~Active timing is finalized-event based in v1; live active-time ticking can + be added later if it becomes necessary.~~ **Superseded 2026-07-25.** It became + necessary: a run parked in one long agent stage reported ~12% of its wall time + as active, because in-flight stages contributed nothing. Stage projections now + accumulate inference and tool brackets from the event log and expose + `StageProjection::live_timing(now)`, the active-time twin of + `live_wall_time_ms`. Finalized values remain authoritative and still replace + the live estimate at terminal events. See + `.ai/plans/live-active-time-accumulation.md`. - No compatibility layer is required for existing API clients or stored run event data. diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index b028d89c0..568aa30e5 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -10673,7 +10673,38 @@ components: - type: "null" description: | Per-attempt timing breakdown for the latest terminal attempt: - wall time plus the active inference/tool breakdown. + wall time plus the active inference/tool breakdown. Null while the + stage is still in flight; the live estimate is derived from + `live_inference_ms`, `live_tool_ms`, and any open bracket. + live_inference_ms: + type: integer + format: uint64 + minimum: 0 + default: 0 + description: | + Inference time accumulated from closed brackets during the current + attempt. Live estimate only — the authoritative value arrives with + the terminal event and lands in `timing`. Excludes the currently + open bracket, whose span is measured from `inference.started_at`. + example: 78230 + live_tool_ms: + type: integer + format: uint64 + minimum: 0 + default: 0 + description: | + Tool time accumulated from closed tool batches during the current + attempt. A batch spans the first dispatched call through the + completion that drains the last outstanding one, so tools running + concurrently within a turn are counted once. + example: 7588 + tool_batch: + oneOf: + - $ref: "#/components/schemas/StageToolBatchProjection" + - type: "null" + description: | + Open tool batch: when the batch started and which calls have not + yet reported completion. usage: $ref: "#/components/schemas/BilledTokenCounts" model: @@ -10734,6 +10765,28 @@ components: $ref: "#/components/schemas/StageState" description: Lifecycle state of the stage projection. + StageToolBatchProjection: + description: > + One open tool batch: tool calls dispatched together that have not all + reported completion. `open_call_ids` is a set rather than a count so a + duplicated completion in a replayed log cannot drain the batch early. + type: object + required: + - started_at + - open_call_ids + properties: + started_at: + type: string + format: date-time + description: > + When the batch opened — the first dispatched call observed while no + other calls were outstanding. + open_call_ids: + type: array + items: + type: string + description: Calls dispatched but not yet completed, by tool call id. + StageInferenceProjection: description: > One open inference bracket: a dispatched LLM request that has not yet @@ -12244,6 +12297,12 @@ components: observed LLM request/stream elapsed time; `tool_time_ms` is tool or command execution elapsed time; `active_time_ms` equals `inference_time_ms + tool_time_ms`. + + For a terminal stage these come from the worker's own stopwatch and are + authoritative. For a stage still in flight they are a live estimate + reconstructed from the event log, and `active_time_ms` is clamped to + `wall_time_ms`. The estimate is replaced by the authoritative + breakdown when the stage reaches a terminal event. type: object required: - wall_time_ms @@ -12278,6 +12337,11 @@ components: Timing rollup for an entire run. Active fields sum work across stage visits, so `active_time_ms` can exceed `wall_time_ms` when parallel branches run concurrently. + + For a running run, stages still in flight contribute a live estimate + rather than nothing, so wall and active both advance continuously. + Unlike `StageTiming`, active is not clamped to wall here — concurrent + branches can legitimately sum past run wall time. type: object required: - wall_time_ms diff --git a/lib/apps/fabro-server/src/server/handler/billing.rs b/lib/apps/fabro-server/src/server/handler/billing.rs index a1888713c..6cc4cbdde 100644 --- a/lib/apps/fabro-server/src/server/handler/billing.rs +++ b/lib/apps/fabro-server/src/server/handler/billing.rs @@ -175,7 +175,7 @@ fn live_billing_rows(projection: &RunProjection, now: DateTime) -> Vec= row.latest_visit { @@ -188,20 +188,6 @@ fn live_billing_rows(projection: &RunProjection, now: DateTime) -> Vec) -> StageTiming { - if let Some(timing) = stage.timing { - return timing; - } - if let Some(live_wall) = stage.live_wall_time_ms(now) { - return StageTiming::wall_only(live_wall); - } - StageTiming::default() -} - fn stage_has_billing_row(stage: &StageProjection) -> bool { stage.completion.is_some() || stage.timing.is_some() diff --git a/lib/components/fabro-store/src/run_state.rs b/lib/components/fabro-store/src/run_state.rs index 9ab1e2b2d..df05f84b3 100644 --- a/lib/components/fabro-store/src/run_state.rs +++ b/lib/components/fabro-store/src/run_state.rs @@ -449,7 +449,7 @@ impl RunProjectionReducer for RunProjection { context_window.event_seq = Some(event.seq); stage.context_window = Some(context_window); } - close_inference_bracket(self, stored, props.visit, event.seq); + close_inference_bracket(self, stored, props.visit, event.seq, ts); } EventBody::AgentLlmStarted(props) => { open_inference_bracket(self, stored, props, event.seq, ts); @@ -478,10 +478,10 @@ impl RunProjectionReducer for RunProjection { inference.first_output_kind = None; } EventBody::AgentError(props) => { - close_inference_bracket(self, stored, props.visit, event.seq); + close_inference_bracket(self, stored, props.visit, event.seq, ts); } EventBody::AgentSessionEnded(_) => { - close_inference_brackets_for_session(self, stored); + close_inference_brackets_for_session(self, stored, ts); } EventBody::AgentSessionActivated(props) => { let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) @@ -497,7 +497,7 @@ impl RunProjectionReducer for RunProjection { return Ok(()); }; stage.agent_control = AgentControlState::WaitingForSteer; - close_inference_bracket(self, stored, props.visit, event.seq); + close_inference_bracket(self, stored, props.visit, event.seq, ts); } EventBody::AgentSteeringInjected(props) => { let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) @@ -746,6 +746,7 @@ impl RunProjectionReducer for RunProjection { }); } EventBody::AgentToolStarted(props) => { + let is_root_session = stored.parent_session_id.is_none(); let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) else { return Ok(()); @@ -766,6 +767,22 @@ impl RunProjectionReducer for RunProjection { projection.invoked = true; } } + // A subagent's tools run inside the root session's tool call, + // so the root batch already covers them. Timing them again + // would double-count that span. + if is_root_session { + stage.open_tool_call(props.tool_call_id.clone(), ts); + } + } + EventBody::AgentToolCompleted(props) => { + if stored.parent_session_id.is_some() { + return Ok(()); + } + let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) + else { + return Ok(()); + }; + stage.close_tool_call(&props.tool_call_id, ts); } _ => {} } @@ -1044,6 +1061,22 @@ fn matching_inference_slot<'a>( visit: u32, seq: u32, ) -> Option<&'a mut Option> { + Some(&mut matching_inference_stage(state, stored, visit, seq)?.inference) +} + +/// Resolve the stage owning a bracket this event is allowed to mutate, +/// borrowing the whole projection so the caller can also fold elapsed time +/// into the stage's live accumulators. +/// +/// Same gating as [`matching_inference_slot`]: `None` for child-session +/// events, for a stage with no open bracket, and for a bracket belonging to a +/// different session (which is what post-failover events look like). +fn matching_inference_stage<'a>( + state: &'a mut RunProjection, + stored: &RunEvent, + visit: u32, + seq: u32, +) -> Option<&'a mut StageProjection> { if stored.parent_session_id.is_some() { return None; } @@ -1053,7 +1086,7 @@ fn matching_inference_slot<'a>( .inference .as_ref() .is_some_and(|inference| inference.session_id == session_id); - opened_here.then_some(&mut stage.inference) + opened_here.then_some(stage) } /// Resolve the open inference bracket this event is allowed to mutate. @@ -1066,11 +1099,37 @@ fn matching_inference_bracket<'a>( matching_inference_slot(state, stored, visit, seq)?.as_mut() } -/// Close the bracket on a stage-addressed terminal event. -fn close_inference_bracket(state: &mut RunProjection, stored: &RunEvent, visit: u32, seq: u32) { - if let Some(slot) = matching_inference_slot(state, stored, visit, seq) { - *slot = None; - } +/// Close the bracket on a stage-addressed terminal event, folding its elapsed +/// time into the stage's live inference accumulator. +fn close_inference_bracket( + state: &mut RunProjection, + stored: &RunEvent, + visit: u32, + seq: u32, + ts: DateTime, +) { + let Some(stage) = matching_inference_stage(state, stored, visit, seq) else { + return; + }; + close_bracket_on_stage(stage, ts); +} + +/// Take the open bracket and add its span to `live_inference_ms`. +/// +/// Retries inside the bracket are deliberately included: the in-process +/// stopwatch counts a retried attempt's elapsed time as inference, and +/// `agent.llm.retry` keeps the bracket open rather than reopening it. +fn close_bracket_on_stage(stage: &mut StageProjection, ts: DateTime) { + let Some(inference) = stage.inference.take() else { + return; + }; + stage.accumulate_inference_ms(elapsed_ms(inference.started_at, ts)); +} + +/// Non-negative milliseconds between two instants, saturating at zero so a +/// clock skew or an out-of-order replay cannot produce a negative span. +fn elapsed_ms(from: DateTime, to: DateTime) -> u64 { + u64::try_from(to.signed_duration_since(from).num_milliseconds().max(0)).unwrap_or(0) } /// Close every bracket opened by the session that just ended. @@ -1088,7 +1147,11 @@ fn close_inference_bracket(state: &mut RunProjection, stored: &RunEvent, visit: /// opened. Implemented as a normal stage lookup it would find no target and /// silently no-op, leaving the bracket open forever on exactly the path it /// exists to cover. -fn close_inference_brackets_for_session(state: &mut RunProjection, stored: &RunEvent) { +fn close_inference_brackets_for_session( + state: &mut RunProjection, + stored: &RunEvent, + ts: DateTime, +) { if stored.parent_session_id.is_some() { return; } @@ -1101,7 +1164,7 @@ fn close_inference_brackets_for_session(state: &mut RunProjection, stored: &RunE .as_ref() .is_some_and(|inference| inference.session_id == session_id); if opened_here { - stage.inference = None; + close_bracket_on_stage(stage, ts); } } } @@ -1417,18 +1480,17 @@ fn finalize_unfinished_stages_after_run_failed( continue; } + // Close any bracket still open so its span is not dropped on the + // floor when the live estimate is frozen into `timing` below. + close_bracket_on_stage(stage, timestamp); + + // Freeze the live estimate before flipping to a terminal state: + // `live_timing` reads `effective_state` and would return wall-only + // once the stage no longer looks in-flight. + let frozen = stage.live_timing(timestamp); stage.state = terminal_state; - if stage.timing.is_none() { - if let Some(started_at) = stage.started_at { - let wall_time_ms = u64::try_from( - timestamp - .signed_duration_since(started_at) - .num_milliseconds() - .max(0), - ) - .expect("non-negative milliseconds fit in u64"); - stage.timing = Some(fabro_types::StageTiming::wall_only(wall_time_ms)); - } + if stage.timing.is_none() && stage.started_at.is_some() { + stage.timing = Some(frozen); } } } @@ -1560,6 +1622,373 @@ mod tests { use super::{RunProjection, RunProjectionReducer, build_summary}; use crate::{Error, EventEnvelope, StageId}; + /// Live accumulation of inference and tool time while a stage is in + /// flight. The finalized breakdown still arrives with the terminal event + /// and replaces these; these exist so a long-running stage is not reported + /// as doing no work. + mod live_active_accumulation { + use fabro_types::run_event::{ + AgentLlmFirstOutputProps, AgentLlmRetryProps, AgentLlmStartedProps, + AgentToolCompletedProps, AgentToolStartedProps, + }; + use fabro_types::{ + LlmOutputKind, LlmRetryPhase, ModelRef, Speed, StageOutcome, StageProjection, + }; + + use super::*; + + fn stage_id() -> StageId { + StageId::new("plan", 1) + } + + fn agent_event(seq: u32, ts: &str, body: EventBody) -> EventEnvelope { + let mut event = test_stage_event_at(seq, ts, body, stage_id()); + event.event.session_id = Some("session-1".to_string()); + event + } + + /// An event from a sub-agent session nested under the root session. + fn child_event(seq: u32, ts: &str, body: EventBody) -> EventEnvelope { + let mut event = agent_event(seq, ts, body); + event.event.session_id = Some("session-child".to_string()); + event.event.parent_session_id = Some("session-1".to_string()); + event + } + + fn llm_started() -> EventBody { + EventBody::AgentLlmStarted(AgentLlmStartedProps { + requested_model: ModelRef { + provider: "anthropic".parse().unwrap(), + model_id: "claude-fable-5".into(), + speed: Some(Speed::Fast), + }, + visit: 1, + }) + } + + fn tool_started(tool_call_id: &str) -> EventBody { + EventBody::AgentToolStarted(AgentToolStartedProps { + tool_name: "Bash".to_string(), + tool_call_id: tool_call_id.to_string(), + arguments: json!({}), + visit: 1, + tool_call: None, + turn_id: None, + parent_message_id: None, + }) + } + + fn tool_completed(tool_call_id: &str) -> EventBody { + EventBody::AgentToolCompleted(AgentToolCompletedProps { + tool_name: "Bash".to_string(), + tool_call_id: tool_call_id.to_string(), + output: json!("ok"), + is_error: false, + visit: 1, + tool_result: None, + turn_id: None, + }) + } + + fn agent_message() -> EventBody { + EventBody::AgentMessage(live_agent_message_props(live_counts(10, 5))) + } + + fn started_state() -> RunProjection { + let mut state = initialized_projection(); + state + .apply_event(&test_stage_event_at( + 1, + "2026-04-07T12:00:00Z", + EventBody::StageStarted(started_props()), + stage_id(), + )) + .unwrap(); + state + } + + fn stage(state: &RunProjection) -> &StageProjection { + state.stage(&stage_id()).unwrap() + } + + #[test] + fn closing_an_inference_bracket_accumulates_its_span() { + let mut state = started_state(); + state + .apply_event(&agent_event(2, "2026-04-07T12:00:05Z", llm_started())) + .unwrap(); + state + .apply_event(&agent_event( + 3, + "2026-04-07T12:00:06Z", + EventBody::AgentLlmFirstOutput(AgentLlmFirstOutputProps { + kind: LlmOutputKind::Text, + visit: 1, + }), + )) + .unwrap(); + state + .apply_event(&agent_event(4, "2026-04-07T12:00:12Z", agent_message())) + .unwrap(); + + // 12:00:05 -> 12:00:12; first_output is a marker, not the close. + assert_eq!(stage(&state).live_inference_ms, 7_000); + assert!(stage(&state).inference.is_none()); + } + + #[test] + fn concurrent_tool_calls_count_once_not_per_call() { + let mut state = started_state(); + for (seq, id) in [(2, "call-a"), (3, "call-b"), (4, "call-c")] { + state + .apply_event(&agent_event(seq, "2026-04-07T12:00:00Z", tool_started(id))) + .unwrap(); + } + // All three finish 10s later. Summing per-call spans would report + // 30s; the batch actually occupied 10s of wall time. + for (seq, id) in [(5, "call-a"), (6, "call-b"), (7, "call-c")] { + state + .apply_event(&agent_event( + seq, + "2026-04-07T12:00:10Z", + tool_completed(id), + )) + .unwrap(); + } + + assert_eq!(stage(&state).live_tool_ms, 10_000); + assert!(stage(&state).tool_batch.is_none()); + } + + #[test] + fn a_batch_stays_open_until_its_last_call_reports() { + let mut state = started_state(); + state + .apply_event(&agent_event( + 2, + "2026-04-07T12:00:00Z", + tool_started("call-a"), + )) + .unwrap(); + state + .apply_event(&agent_event( + 3, + "2026-04-07T12:00:02Z", + tool_started("call-b"), + )) + .unwrap(); + state + .apply_event(&agent_event( + 4, + "2026-04-07T12:00:05Z", + tool_completed("call-a"), + )) + .unwrap(); + + assert_eq!( + stage(&state).live_tool_ms, + 0, + "batch must not close while call-b is outstanding" + ); + + state + .apply_event(&agent_event( + 5, + "2026-04-07T12:00:09Z", + tool_completed("call-b"), + )) + .unwrap(); + + // Measured from the batch open, not from the last call's start. + assert_eq!(stage(&state).live_tool_ms, 9_000); + } + + #[test] + fn successive_batches_accumulate() { + let mut state = started_state(); + for (seq, ts, body) in [ + (2, "2026-04-07T12:00:00Z", tool_started("call-a")), + (3, "2026-04-07T12:00:04Z", tool_completed("call-a")), + (4, "2026-04-07T12:00:10Z", tool_started("call-b")), + (5, "2026-04-07T12:00:16Z", tool_completed("call-b")), + ] { + state.apply_event(&agent_event(seq, ts, body)).unwrap(); + } + + assert_eq!(stage(&state).live_tool_ms, 10_000); + } + + #[test] + fn a_duplicate_completion_does_not_drain_the_batch_early() { + let mut state = started_state(); + state + .apply_event(&agent_event( + 2, + "2026-04-07T12:00:00Z", + tool_started("call-a"), + )) + .unwrap(); + state + .apply_event(&agent_event( + 3, + "2026-04-07T12:00:00Z", + tool_started("call-b"), + )) + .unwrap(); + // call-a reports twice, as a replayed or duplicated log can. + state + .apply_event(&agent_event( + 4, + "2026-04-07T12:00:03Z", + tool_completed("call-a"), + )) + .unwrap(); + state + .apply_event(&agent_event( + 5, + "2026-04-07T12:00:04Z", + tool_completed("call-a"), + )) + .unwrap(); + + assert_eq!(stage(&state).live_tool_ms, 0); + assert!(stage(&state).tool_batch.is_some()); + } + + #[test] + fn subagent_tool_calls_do_not_double_count_against_the_root_batch() { + let mut state = started_state(); + state + .apply_event(&agent_event( + 2, + "2026-04-07T12:00:00Z", + tool_started("root-call"), + )) + .unwrap(); + // The sub-agent's own tools run inside the root call's span. + state + .apply_event(&child_event( + 3, + "2026-04-07T12:00:01Z", + tool_started("child-call"), + )) + .unwrap(); + state + .apply_event(&child_event( + 4, + "2026-04-07T12:00:02Z", + tool_completed("child-call"), + )) + .unwrap(); + state + .apply_event(&agent_event( + 5, + "2026-04-07T12:00:08Z", + tool_completed("root-call"), + )) + .unwrap(); + + assert_eq!(stage(&state).live_tool_ms, 8_000); + } + + #[test] + fn session_end_accumulates_rather_than_discarding_the_bracket() { + let mut state = started_state(); + state + .apply_event(&agent_event(2, "2026-04-07T12:00:05Z", llm_started())) + .unwrap(); + + let mut ended = test_stage_event_at( + 3, + "2026-04-07T12:00:20Z", + EventBody::AgentSessionEnded(AgentSessionEndedProps {}), + stage_id(), + ); + ended.event.session_id = Some("session-1".to_string()); + state.apply_event(&ended).unwrap(); + + assert_eq!(stage(&state).live_inference_ms, 15_000); + assert!(stage(&state).inference.is_none()); + } + + #[test] + fn a_foreign_session_close_leaves_the_bracket_and_accumulator_alone() { + let mut state = started_state(); + state + .apply_event(&agent_event(2, "2026-04-07T12:00:05Z", llm_started())) + .unwrap(); + + // Post-failover: a new session emits the message, so the old + // bracket is not this event's to close or bill. + let mut foreign = + test_stage_event_at(3, "2026-04-07T12:00:20Z", agent_message(), stage_id()); + foreign.event.session_id = Some("session-2".to_string()); + state.apply_event(&foreign).unwrap(); + + assert_eq!(stage(&state).live_inference_ms, 0); + assert!(stage(&state).inference.is_some()); + } + + #[test] + fn a_retry_keeps_accumulating_within_one_bracket() { + let mut state = started_state(); + state + .apply_event(&agent_event(2, "2026-04-07T12:00:00Z", llm_started())) + .unwrap(); + state + .apply_event(&agent_event( + 3, + "2026-04-07T12:00:04Z", + EventBody::AgentLlmRetry(AgentLlmRetryProps { + provider: "anthropic".to_string(), + model: "claude-fable-5".to_string(), + attempt: 0, + delay_secs: 0.0, + error: json!({ "kind": "stream" }), + phase: Some(LlmRetryPhase::Consume), + visit: 1, + }), + )) + .unwrap(); + state + .apply_event(&agent_event(4, "2026-04-07T12:00:11Z", agent_message())) + .unwrap(); + + // The whole bracket counts, retry included, matching the + // in-process stopwatch. + assert_eq!(stage(&state).live_inference_ms, 11_000); + assert_eq!(stage(&state).inference, None); + } + + #[test] + fn stage_completion_replaces_the_live_estimate_with_finalized_timing() { + let mut state = started_state(); + state + .apply_event(&agent_event(2, "2026-04-07T12:00:00Z", llm_started())) + .unwrap(); + state + .apply_event(&agent_event(3, "2026-04-07T12:00:09Z", agent_message())) + .unwrap(); + assert_eq!(stage(&state).live_inference_ms, 9_000); + + state + .apply_event(&test_stage_event_at( + 4, + "2026-04-07T12:00:10Z", + EventBody::StageCompleted(completed_props(10_000, StageOutcome::Succeeded)), + stage_id(), + )) + .unwrap(); + + let stage = stage(&state); + assert_eq!( + stage.live_timing(test_dt("2026-04-07T12:30:00Z")), + stage.timing.unwrap(), + "a terminal stage reports its finalized breakdown, not a live estimate" + ); + } + } + fn test_event(seq: u32, body: EventBody, node_id: Option<&str>) -> EventEnvelope { let event = RunEvent { id: format!("evt-{seq}"), diff --git a/lib/foundation/fabro-types/src/run_projection.rs b/lib/foundation/fabro-types/src/run_projection.rs index 027cce2fc..0576d32f6 100644 --- a/lib/foundation/fabro-types/src/run_projection.rs +++ b/lib/foundation/fabro-types/src/run_projection.rs @@ -1,5 +1,5 @@ use std::borrow::Cow; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::num::NonZeroU32; use chrono::{DateTime, Utc}; @@ -354,10 +354,31 @@ pub struct StageProjection { /// immutable projections under their own `StageId`s. /// /// `None` for stages still in flight (`started_at` is set but no terminal - /// event has been observed yet). For live wall-time ticking, the UI uses - /// `started_at`; once terminal this carries the finalized breakdown. + /// event has been observed yet). For a live breakdown while in flight, use + /// [`StageProjection::live_timing`]; once terminal this carries the + /// finalized, authoritative breakdown. #[serde(default, skip_serializing_if = "Option::is_none")] pub timing: Option, + /// Inference time accumulated from closed brackets during this attempt. + /// + /// Live estimate only: the authoritative value arrives with the terminal + /// event and lands in `timing`. Excludes the currently-open bracket, which + /// [`StageProjection::live_timing`] adds from `inference.started_at`. + #[serde(default, skip_serializing_if = "is_zero_ms")] + pub live_inference_ms: u64, + /// Tool time accumulated from closed tool batches during this attempt. + /// + /// A batch spans the first `agent.tool.started` with no outstanding calls + /// through the `agent.tool.completed` that drains the last one, so tools + /// running concurrently within a turn are counted once. This matches how + /// the in-process stopwatch brackets `execute_tool_calls`; summing + /// per-call durations would over-count parallel tool use. + #[serde(default, skip_serializing_if = "is_zero_ms")] + pub live_tool_ms: u64, + /// Open tool batch for this stage: when the current batch started, and the + /// `tool_call_id`s that have not yet reported completion. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub tool_batch: Option, #[serde(default)] pub usage: BilledTokenCounts, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -395,6 +416,30 @@ pub struct StageProjection { pub state: StageState, } +/// Serde guard so zero-valued live accumulators stay off the wire. +#[allow( + clippy::trivially_copy_pass_by_ref, + reason = "serde skip_serializing_if predicates receive fields by reference" +)] +fn is_zero_ms(value: &u64) -> bool { + *value == 0 +} + +/// One open tool batch: tool calls dispatched together that have not all +/// reported completion. +/// +/// `open_call_ids` is a set rather than a count because `agent.tool.completed` +/// identifies its call by id, and a projection replaying a truncated or +/// duplicated log must not let a repeated completion drain the batch early. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct StageToolBatchProjection { + /// When the batch opened — the first `agent.tool.started` observed while + /// no other calls were outstanding. + pub started_at: DateTime, + /// Calls dispatched but not yet completed, by `tool_call_id`. + pub open_call_ids: BTreeSet, +} + /// One open inference bracket: a dispatched LLM request that has not yet /// produced a message, error, or interrupt. #[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize)] @@ -506,6 +551,9 @@ impl StageProjection { response: None, completion: None, timing: None, + live_inference_ms: 0, + live_tool_ms: 0, + tool_batch: None, usage: BilledTokenCounts::default(), model: None, root_agent_todos: None, @@ -563,6 +611,118 @@ impl StageProjection { self.timing.map(|timing| timing.wall_time_ms) } + /// Live timing breakdown in milliseconds — the active-time twin of + /// [`Self::live_wall_time_ms`]. + /// + /// Once terminal, returns the stored `timing` unchanged: the finalized + /// breakdown comes from the worker's own stopwatch and is authoritative. + /// + /// While in flight, returns an estimate reconstructed from the event log: + /// accumulated closed brackets plus whatever bracket is open right now. + /// The estimate is per-handler, because only agent stages emit brackets at + /// all: + /// + /// - `Agent` — accumulated inference and tool brackets, plus the open + /// inference bracket and open tool batch. + /// - `Prompt` — one inference call spanning the stage, so elapsed time + /// since `started_at` counts as inference. Matches the finalized + /// `active_only(inference, 0)`. + /// - `Command` — the command *is* the work, so elapsed time counts as tool. + /// Matches the finalized `active_only(0, duration_ms)`. + /// - Everything else — zero. Waiting on a human, a timer, a condition, or + /// child branches is wall time, not active time. + /// + /// Active is clamped to wall. A worker killed mid-turn leaves its bracket + /// open forever (see [`StageInferenceProjection`]), and without the clamp + /// that bracket would tick up without bound. The clamp does not need to + /// know the worker died: a stage cannot have been active longer than it + /// has existed. `watchdog.timeout` remains the authority on whether a run + /// is actually stuck. + #[must_use] + pub fn live_timing(&self, now: DateTime) -> StageTiming { + if let Some(timing) = self.timing { + return timing; + } + + let wall_time_ms = self.live_wall_time_ms(now).unwrap_or(0); + let elapsed = |since: DateTime| { + u64::try_from(now.signed_duration_since(since).num_milliseconds().max(0)).unwrap_or(0) + }; + + // `handler` is absent on projections built from events written before + // stage execution identity. Treat those as agent stages, matching + // `StageHandler::from_handler_type`: the accumulators below are only + // ever populated by agent events, so a legacy non-agent stage still + // reads zero rather than being credited work it never did. + let handler = self.handler.unwrap_or(StageHandler::Agent); + let (inference_time_ms, tool_time_ms) = match handler { + StageHandler::Agent => { + let open_inference = self + .inference + .as_ref() + .map_or(0, |inference| elapsed(inference.started_at)); + let open_tool = self + .tool_batch + .as_ref() + .map_or(0, |batch| elapsed(batch.started_at)); + ( + self.live_inference_ms.saturating_add(open_inference), + self.live_tool_ms.saturating_add(open_tool), + ) + } + StageHandler::Prompt => (wall_time_ms, 0), + StageHandler::Command => (0, wall_time_ms), + // Waiting on a human, a timer, a condition, or child branches is + // wall time, not active time. + StageHandler::Human + | StageHandler::Wait + | StageHandler::Conditional + | StageHandler::Parallel + | StageHandler::ParallelFanIn + | StageHandler::StackManagerLoop + | StageHandler::Start + | StageHandler::Exit => (0, 0), + }; + + StageTiming::new(wall_time_ms, inference_time_ms, tool_time_ms).clamped_to_wall() + } + + /// Fold a closed inference bracket into the live accumulator. + pub fn accumulate_inference_ms(&mut self, elapsed_ms: u64) { + self.live_inference_ms = self.live_inference_ms.saturating_add(elapsed_ms); + } + + /// Record a dispatched tool call, opening a batch if none is outstanding. + pub fn open_tool_call(&mut self, tool_call_id: String, started_at: DateTime) { + self.tool_batch + .get_or_insert_with(|| StageToolBatchProjection { + started_at, + open_call_ids: BTreeSet::new(), + }) + .open_call_ids + .insert(tool_call_id); + } + + /// Retire a tool call. Folds the batch into the live accumulator once the + /// last outstanding call reports, so concurrent calls count once. + pub fn close_tool_call(&mut self, tool_call_id: &str, now: DateTime) { + let Some(batch) = self.tool_batch.as_mut() else { + return; + }; + batch.open_call_ids.remove(tool_call_id); + if !batch.open_call_ids.is_empty() { + return; + } + let elapsed = u64::try_from( + now.signed_duration_since(batch.started_at) + .num_milliseconds() + .max(0), + ) + .unwrap_or(0); + self.live_tool_ms = self.live_tool_ms.saturating_add(elapsed); + self.tool_batch = None; + } + /// Begin a new automatic attempt within this stage execution: clear every /// per-attempt field so prior-attempt data does not leak, then record /// `started_at` and `state = Running`. Preserves `first_event_seq` @@ -709,10 +869,13 @@ impl RunProjection { /// terminal conclusion yet. /// /// Run-level wall time ticks from `run.started` to `now`. Active time sums - /// inference and tool timing from stages that have already emitted a - /// terminal stage event. Stage projections do not currently track live - /// inference/tool time while a stage is still running, so active time steps - /// forward when each stage completes while wall time advances continuously. + /// [`StageProjection::live_timing`] across every stage, so an in-flight + /// stage contributes its live estimate rather than nothing — both halves + /// advance continuously. Terminal stages contribute their finalized, + /// authoritative breakdown. + /// + /// Active is not clamped to run wall time here: concurrent branches can + /// legitimately sum past it. The clamp applies per stage. #[must_use] pub fn live_run_timing(&self, now: DateTime) -> Option { let start = self.start.as_ref()?; @@ -720,7 +883,7 @@ impl RunProjection { let active = self .stages .values() - .filter_map(|stage| stage.timing) + .map(|stage| stage.live_timing(now)) .fold(RunTiming::default(), |acc, timing| { acc.saturating_add(&RunTiming::from(timing)) }); @@ -971,3 +1134,286 @@ mod iter_stages_tests { } } } + +#[cfg(test)] +mod live_timing_tests { + use std::collections::HashMap; + use std::num::NonZeroU32; + + use chrono::{DateTime, TimeZone, Utc}; + + use super::{RunProjection, StageToolBatchProjection}; + use crate::{ + Graph, ModelRef, RunId, RunSpec, StageHandler, StageInferenceProjection, StageProjection, + StageState, StageTiming, StartRecord, WorkflowSettings, test_support, + }; + + fn seq(n: u32) -> NonZeroU32 { + NonZeroU32::new(n).unwrap() + } + + fn at(seconds: i64) -> DateTime { + Utc.timestamp_opt(1_700_000_000 + seconds, 0).unwrap() + } + + fn projection() -> RunProjection { + RunProjection::new( + "Test run".to_string(), + RunSpec { + run_id: RunId::new(), + settings: WorkflowSettings::default(), + graph: Graph::new("test"), + graph_source: None, + workflow_slug: None, + automation: None, + source_directory: None, + labels: HashMap::default(), + provenance: test_support::test_run_provenance(), + manifest_blob: None, + definition_blob: None, + git: None, + fork_source_ref: None, + }, + at(0), + ) + } + + /// In-flight stage that started at `at(0)`. + fn running(handler: StageHandler) -> StageProjection { + let mut stage = StageProjection::new(seq(1)); + stage.handler = Some(handler); + stage.started_at = Some(at(0)); + stage.state = StageState::Running; + stage + } + + fn open_bracket(started_at: DateTime) -> StageInferenceProjection { + StageInferenceProjection { + session_id: "session-1".to_string(), + started_at, + requested_model: ModelRef { + provider: "anthropic".parse().unwrap(), + model_id: "claude-sonnet-5".into(), + speed: None, + }, + first_output_at: None, + first_output_kind: None, + retries: 0, + } + } + + #[test] + fn terminal_stage_returns_stored_timing_unchanged() { + let mut stage = running(StageHandler::Agent); + stage.state = StageState::Succeeded; + stage.timing = Some(StageTiming::new(90_000, 78_000, 7_000)); + // Live accumulators are stale leftovers; the finalized value wins. + stage.live_inference_ms = 5; + stage.live_tool_ms = 5; + + assert_eq!( + stage.live_timing(at(600)), + StageTiming::new(90_000, 78_000, 7_000) + ); + } + + #[test] + fn agent_stage_sums_accumulators_and_open_brackets() { + let mut stage = running(StageHandler::Agent); + stage.live_inference_ms = 30_000; + stage.live_tool_ms = 5_000; + // Inference open for 20s, tools open for 10s, at t=120s. + stage.inference = Some(open_bracket(at(100))); + stage.tool_batch = Some(StageToolBatchProjection { + started_at: at(110), + open_call_ids: ["call-1".to_string()].into_iter().collect(), + }); + + assert_eq!( + stage.live_timing(at(120)), + StageTiming::new(120_000, 50_000, 15_000) + ); + } + + #[test] + fn agent_stage_without_brackets_reports_only_accumulators() { + let mut stage = running(StageHandler::Agent); + stage.live_inference_ms = 30_000; + stage.live_tool_ms = 5_000; + + assert_eq!( + stage.live_timing(at(120)), + StageTiming::new(120_000, 30_000, 5_000) + ); + } + + #[test] + fn prompt_stage_counts_elapsed_as_inference() { + let stage = running(StageHandler::Prompt); + + assert_eq!( + stage.live_timing(at(45)), + StageTiming::new(45_000, 45_000, 0) + ); + } + + #[test] + fn command_stage_counts_elapsed_as_tool() { + let stage = running(StageHandler::Command); + + assert_eq!( + stage.live_timing(at(45)), + StageTiming::new(45_000, 0, 45_000) + ); + } + + #[test] + fn waiting_handlers_report_wall_time_with_zero_active() { + for handler in [ + StageHandler::Human, + StageHandler::Wait, + StageHandler::Conditional, + StageHandler::Parallel, + StageHandler::ParallelFanIn, + StageHandler::StackManagerLoop, + StageHandler::Start, + StageHandler::Exit, + ] { + let stage = running(handler); + let timing = stage.live_timing(at(600)); + + assert_eq!( + timing, + StageTiming::new(600_000, 0, 0), + "{handler} should report wall time only" + ); + } + } + + #[test] + fn open_bracket_from_a_killed_worker_is_clamped_to_wall() { + let mut stage = running(StageHandler::Agent); + // Bracket opened before the stage even started — the pathological + // shape a killed worker leaves behind. Without the clamp this would + // report 700s of inference against 600s of wall. + stage.inference = Some(open_bracket(at(-100))); + + let timing = stage.live_timing(at(600)); + + assert_eq!(timing.wall_time_ms, 600_000); + assert_eq!(timing.active_time_ms, 600_000); + } + + #[test] + fn clamping_preserves_the_inference_tool_split() { + let mut stage = running(StageHandler::Agent); + // 3:1 inference:tool, totalling 200s of active against 100s of wall. + stage.live_inference_ms = 150_000; + stage.live_tool_ms = 50_000; + + let timing = stage.live_timing(at(100)); + + assert_eq!(timing.wall_time_ms, 100_000); + assert_eq!(timing.active_time_ms, 100_000); + assert_eq!(timing.inference_time_ms, 75_000); + assert_eq!(timing.tool_time_ms, 25_000); + } + + #[test] + fn live_run_timing_counts_in_flight_stages_not_just_terminal_ones() { + // The shape that motivated this change: two finished stages and one + // long-running agent stage that had been active nearly the whole run. + let mut projection = projection(); + projection.start = Some(StartRecord { + start_time: at(0), + run_branch: None, + base_sha: None, + }); + + let baseline = projection.stage_entry("baseline", 1, seq(1)); + baseline.handler = Some(StageHandler::Command); + baseline.state = StageState::Succeeded; + baseline.timing = Some(StageTiming::new(42_666, 0, 42_663)); + + let assess = projection.stage_entry("assess", 1, seq(2)); + assess.handler = Some(StageHandler::Agent); + assess.state = StageState::Succeeded; + assess.timing = Some(StageTiming::new(86_025, 78_230, 7_588)); + + let plan = projection.stage_entry("plan", 1, seq(3)); + plan.handler = Some(StageHandler::Agent); + plan.started_at = Some(at(146)); + plan.state = StageState::Running; + plan.live_inference_ms = 700_000; + plan.live_tool_ms = 150_000; + + let timing = projection.live_run_timing(at(1_013)).unwrap(); + + assert_eq!(timing.wall_time_ms, 1_013_000); + assert_eq!(timing.inference_time_ms, 778_230); + assert_eq!(timing.tool_time_ms, 200_251); + // Before this change the in-flight stage contributed nothing and the + // run reported 128,481 ms of active time against 1,013,000 ms of wall. + assert_eq!(timing.active_time_ms, 978_481); + } + + #[test] + fn live_run_timing_may_exceed_run_wall_when_branches_overlap() { + let mut projection = projection(); + projection.start = Some(StartRecord { + start_time: at(0), + run_branch: None, + base_sha: None, + }); + + for (index, node) in ["branch-a", "branch-b", "branch-c"].iter().enumerate() { + let stage = projection.stage_entry(node, 1, seq(u32::try_from(index).unwrap() + 1)); + stage.handler = Some(StageHandler::Agent); + stage.state = StageState::Succeeded; + stage.timing = Some(StageTiming::new(60_000, 60_000, 0)); + } + + let timing = projection.live_run_timing(at(60)).unwrap(); + + assert_eq!(timing.wall_time_ms, 60_000); + assert_eq!( + timing.active_time_ms, 180_000, + "concurrent branches legitimately sum past run wall time" + ); + } +} + +#[cfg(test)] +mod live_timing_legacy_tests { + use chrono::{DateTime, TimeZone, Utc}; + + use crate::{StageProjection, StageState, StageTiming, first_event_seq}; + + fn at(seconds: i64) -> DateTime { + Utc.timestamp_opt(1_700_000_000 + seconds, 0).unwrap() + } + + #[test] + fn a_legacy_stage_without_a_recorded_handler_uses_its_accumulators() { + let mut stage = StageProjection::new(first_event_seq(1)); + stage.handler = None; + stage.started_at = Some(at(0)); + stage.state = StageState::Running; + stage.live_inference_ms = 30_000; + + assert_eq!( + stage.live_timing(at(120)), + StageTiming::new(120_000, 30_000, 0) + ); + } + + #[test] + fn a_legacy_stage_with_no_accumulators_reports_no_active_time() { + let mut stage = StageProjection::new(first_event_seq(1)); + stage.handler = None; + stage.started_at = Some(at(0)); + stage.state = StageState::Running; + + assert_eq!(stage.live_timing(at(120)), StageTiming::new(120_000, 0, 0)); + } +} diff --git a/lib/foundation/fabro-types/src/timing.rs b/lib/foundation/fabro-types/src/timing.rs index 0d1e555e8..4fe45a36f 100644 --- a/lib/foundation/fabro-types/src/timing.rs +++ b/lib/foundation/fabro-types/src/timing.rs @@ -62,6 +62,32 @@ impl StageTiming { Self::new(0, inference_time_ms, tool_time_ms) } + /// Scale the breakdown down so `active_time_ms` does not exceed + /// `wall_time_ms`, preserving the inference/tool ratio. + /// + /// Only meaningful for live estimates of a single in-flight stage, where + /// an open bracket left behind by a killed worker would otherwise tick up + /// without bound. Finalized timings come from the worker's stopwatch and + /// already satisfy the invariant. + /// + /// Deliberately *not* applied at run level: concurrent branches can + /// legitimately sum past run wall time. + #[must_use] + pub fn clamped_to_wall(&self) -> Self { + if self.active_time_ms <= self.wall_time_ms { + return *self; + } + // Preserve the split rather than truncating one side, so a clamped + // stage still shows where its time went. Widen for the multiply: the + // quotient is bounded by `wall_time_ms` because `active_time_ms` + // exceeds it here, so it always fits back into u64. + let scaled = u128::from(self.inference_time_ms) * u128::from(self.wall_time_ms) + / u128::from(self.active_time_ms); + let inference_time_ms = u64::try_from(scaled).unwrap_or(self.wall_time_ms); + let tool_time_ms = self.wall_time_ms.saturating_sub(inference_time_ms); + Self::new(self.wall_time_ms, inference_time_ms, tool_time_ms) + } + /// Sum two timings field-by-field. Used to aggregate visits of one node /// and to accumulate run-level rollups. #[must_use] diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index e3a1a7c0e..4ae265b76 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -487,6 +487,7 @@ models/stage-projection.ts models/stage-state.ts models/stage-summary.ts models/stage-timing.ts +models/stage-tool-batch-projection.ts models/start-record.ts models/start-run-request.ts models/steer-run-request.ts diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index 5cd29601d..029a2d3ab 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -457,6 +457,7 @@ export * from './stage-projection'; export * from './stage-state'; export * from './stage-summary'; export * from './stage-timing'; +export * from './stage-tool-batch-projection'; export * from './start-record'; export * from './start-run-request'; export * from './steer-run-request'; diff --git a/lib/packages/fabro-api-client/src/models/run-timing.ts b/lib/packages/fabro-api-client/src/models/run-timing.ts index fb585b0e9..d4fa62262 100644 --- a/lib/packages/fabro-api-client/src/models/run-timing.ts +++ b/lib/packages/fabro-api-client/src/models/run-timing.ts @@ -15,7 +15,7 @@ /** - * Timing rollup for an entire run. Active fields sum work across stage visits, so `active_time_ms` can exceed `wall_time_ms` when parallel branches run concurrently. + * Timing rollup for an entire run. Active fields sum work across stage visits, so `active_time_ms` can exceed `wall_time_ms` when parallel branches run concurrently. For a running run, stages still in flight contribute a live estimate rather than nothing, so wall and active both advance continuously. Unlike `StageTiming`, active is not clamped to wall here — concurrent branches can legitimately sum past run wall time. */ export interface RunTiming { 'wall_time_ms': number; diff --git a/lib/packages/fabro-api-client/src/models/stage-projection.ts b/lib/packages/fabro-api-client/src/models/stage-projection.ts index 57fd342ca..8cb88c0ac 100644 --- a/lib/packages/fabro-api-client/src/models/stage-projection.ts +++ b/lib/packages/fabro-api-client/src/models/stage-projection.ts @@ -60,6 +60,9 @@ import type { StageState } from './stage-state'; import type { StageTiming } from './stage-timing'; // May contain unused imports in some cases // @ts-ignore +import type { StageToolBatchProjection } from './stage-tool-batch-projection'; +// May contain unused imports in some cases +// @ts-ignore import type { SubAgentProjection } from './sub-agent-projection'; // May contain unused imports in some cases // @ts-ignore @@ -96,6 +99,15 @@ export interface StageProjection { */ 'started_at'?: string | null; 'timing'?: StageTiming | null; + /** + * Inference time accumulated from closed brackets during the current attempt. Live estimate only — the authoritative value arrives with the terminal event and lands in `timing`. Excludes the currently open bracket, whose span is measured from `inference.started_at`. + */ + 'live_inference_ms'?: number; + /** + * Tool time accumulated from closed tool batches during the current attempt. A batch spans the first dispatched call through the completion that drains the last outstanding one, so tools running concurrently within a turn are counted once. + */ + 'live_tool_ms'?: number; + 'tool_batch'?: StageToolBatchProjection | null; 'usage': BilledTokenCounts; 'model'?: BillingModelRef | null; 'todos'?: TodoListProjection | null; diff --git a/lib/packages/fabro-api-client/src/models/stage-timing.ts b/lib/packages/fabro-api-client/src/models/stage-timing.ts index d9915c409..5bdd92bb4 100644 --- a/lib/packages/fabro-api-client/src/models/stage-timing.ts +++ b/lib/packages/fabro-api-client/src/models/stage-timing.ts @@ -15,7 +15,7 @@ /** - * Timing breakdown for one stage visit. Fields are all milliseconds. `wall_time_ms` is elapsed clock time; `inference_time_ms` is Fabro- observed LLM request/stream elapsed time; `tool_time_ms` is tool or command execution elapsed time; `active_time_ms` equals `inference_time_ms + tool_time_ms`. + * Timing breakdown for one stage visit. Fields are all milliseconds. `wall_time_ms` is elapsed clock time; `inference_time_ms` is Fabro- observed LLM request/stream elapsed time; `tool_time_ms` is tool or command execution elapsed time; `active_time_ms` equals `inference_time_ms + tool_time_ms`. For a terminal stage these come from the worker\'s own stopwatch and are authoritative. For a stage still in flight they are a live estimate reconstructed from the event log, and `active_time_ms` is clamped to `wall_time_ms`. The estimate is replaced by the authoritative breakdown when the stage reaches a terminal event. */ export interface StageTiming { 'wall_time_ms': number; diff --git a/lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts b/lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts new file mode 100644 index 000000000..a6bd7718c --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts @@ -0,0 +1,29 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +/** + * One open tool batch: tool calls dispatched together that have not all reported completion. `open_call_ids` is a set rather than a count so a duplicated completion in a replayed log cannot drain the batch early. + */ +export interface StageToolBatchProjection { + /** + * When the batch opened — the first dispatched call observed while no other calls were outstanding. + */ + 'started_at': string; + /** + * Calls dispatched but not yet completed, by tool call id. + */ + 'open_call_ids': Array; +} From dd9f75fb05d55b29b5f2db10d8e0153acc0e95eb Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 25 Jul 2026 23:43:43 -0400 Subject: [PATCH 02/36] fix(timing): harden live active projections --- apps/fabro-web/app/lib/queries.ts | 12 +- apps/fabro-web/app/lib/query-keys.test.ts | 15 +- apps/fabro-web/app/lib/run-events.test.tsx | 20 ++ apps/fabro-web/app/lib/run-events.ts | 29 ++- apps/fabro-web/app/routes/run-detail.tsx | 6 +- ...05-21-wall-and-active-time-metrics-plan.md | 8 +- docs/public/api-reference/fabro-api.yaml | 9 + .../fabro-server/src/server/handler/runs.rs | 30 ++- lib/apps/fabro-server/src/server/tests.rs | 89 +++++++++ lib/components/fabro-store/src/run_state.rs | 182 +++++++++++++++--- lib/foundation/fabro-api/build.rs | 5 + lib/foundation/fabro-api/src/lib.rs | 8 +- .../tests/stage_projection_round_trip.rs | 31 ++- lib/foundation/fabro-types/src/lib.rs | 2 +- .../fabro-types/src/run_projection.rs | 109 ++++++++--- lib/foundation/fabro-types/src/timing.rs | 48 ++++- .../src/models/stage-tool-batch-projection.ts | 6 +- 17 files changed, 523 insertions(+), 86 deletions(-) diff --git a/apps/fabro-web/app/lib/queries.ts b/apps/fabro-web/app/lib/queries.ts index 3c8eb8a1d..aa7328b5e 100644 --- a/apps/fabro-web/app/lib/queries.ts +++ b/apps/fabro-web/app/lib/queries.ts @@ -73,6 +73,7 @@ import { type RunFileSelection, type RunGraphDirection, } from "./query-keys"; +import { isTerminalRunStatus } from "./run-actions"; const immutableOptions: SWRConfiguration = { revalidateIfStale: false, @@ -179,10 +180,19 @@ export function useRunsPage(opts: RunsPageOptions = {}, enabled = true) { ); } -export function useRun(id: string | undefined) { +export function useRun(id: string | undefined, refreshInterval?: number) { return useSWR( id ? queryKeys.runs.detail(id) : null, () => apiNullableData(() => runsApi.retrieveRun(id!)), + refreshInterval + ? { + refreshInterval: (run) => + run?.timestamps.started_at && + !isTerminalRunStatus(run.lifecycle.status.kind) + ? refreshInterval + : 0, + } + : undefined, ); } diff --git a/apps/fabro-web/app/lib/query-keys.test.ts b/apps/fabro-web/app/lib/query-keys.test.ts index ac441b998..52a5ccef3 100644 --- a/apps/fabro-web/app/lib/query-keys.test.ts +++ b/apps/fabro-web/app/lib/query-keys.test.ts @@ -87,8 +87,6 @@ describe("queryKeys", () => { test("agent activity events invalidate per-stage resources", () => { for (const event of [ "stage.prompt", - "agent.tool.started", - "agent.tool.completed", "command.started", "command.completed", ]) { @@ -97,8 +95,19 @@ describe("queryKeys", () => { queryKeys.runs.stageContextWindow("run-1", "stage-1"), ]); } + for (const event of ["agent.tool.started", "agent.tool.completed"]) { + expect(queryKeysForRunEvent("run-1", event, "stage-1")).toEqual([ + queryKeys.runs.detail("run-1"), + queryKeys.runs.state("run-1"), + queryKeys.runs.billing("run-1"), + queryKeys.runs.stageEvents("run-1", "stage-1"), + queryKeys.runs.stageContextWindow("run-1", "stage-1"), + ]); + } expect(queryKeysForRunEvent("run-1", "agent.message", "stage-1")).toEqual([ + queryKeys.runs.detail("run-1"), queryKeys.runs.state("run-1"), + queryKeys.runs.billing("run-1"), queryKeys.runs.stageEvents("run-1", "stage-1"), queryKeys.runs.stageContextWindow("run-1", "stage-1"), ]); @@ -106,7 +115,9 @@ describe("queryKeys", () => { test("agent message without a node_id still invalidates projected state", () => { expect(queryKeysForRunEvent("run-1", "agent.message")).toEqual([ + queryKeys.runs.detail("run-1"), queryKeys.runs.state("run-1"), + queryKeys.runs.billing("run-1"), ]); }); }); diff --git a/apps/fabro-web/app/lib/run-events.test.tsx b/apps/fabro-web/app/lib/run-events.test.tsx index e984be74d..4fce18b4d 100644 --- a/apps/fabro-web/app/lib/run-events.test.tsx +++ b/apps/fabro-web/app/lib/run-events.test.tsx @@ -85,6 +85,8 @@ describe("queryKeysForRunEvent", () => { test("interrupt settlement invalidates projected control state and stage activity", () => { expect(queryKeysForRunEvent("run-1", "agent.round.interrupted", "nap@1")).toEqual([ + queryKeys.runs.detail("run-1"), + queryKeys.runs.billing("run-1"), queryKeys.runs.state("run-1"), queryKeys.runs.events("run-1", 1000), queryKeys.runs.stageEvents("run-1", "nap@1"), @@ -134,22 +136,40 @@ describe("queryKeysForRunEvent", () => { "agent.error", ]) { expect(queryKeysForRunEvent("run-1", event, "code@1")).toEqual([ + queryKeys.runs.detail("run-1"), queryKeys.runs.state("run-1"), + queryKeys.runs.billing("run-1"), queryKeys.runs.stageEvents("run-1", "code@1"), ]); } expect( queryKeysForRunEvent("run-1", "agent.message", "code@1"), ).toEqual([ + queryKeys.runs.detail("run-1"), queryKeys.runs.state("run-1"), + queryKeys.runs.billing("run-1"), queryKeys.runs.stageEvents("run-1", "code@1"), queryKeys.runs.stageContextWindow("run-1", "code@1"), ]); expect(queryKeysForRunEvent("run-1", "agent.session.ended")).toEqual([ + queryKeys.runs.detail("run-1"), queryKeys.runs.state("run-1"), + queryKeys.runs.billing("run-1"), ]); }); + test("tool timing events invalidate live summaries and stage resources", () => { + for (const event of ["agent.tool.started", "agent.tool.completed"]) { + expect(queryKeysForRunEvent("run-1", event, "code@1")).toEqual([ + queryKeys.runs.detail("run-1"), + queryKeys.runs.state("run-1"), + queryKeys.runs.billing("run-1"), + queryKeys.runs.stageEvents("run-1", "code@1"), + queryKeys.runs.stageContextWindow("run-1", "code@1"), + ]); + } + }); + test("watchdog timeout refreshes the stage events for that stage", () => { expect( queryKeysForRunEvent("run-1", "watchdog.timeout", "code@1"), diff --git a/apps/fabro-web/app/lib/run-events.ts b/apps/fabro-web/app/lib/run-events.ts index d0d4f4b4d..523774e18 100644 --- a/apps/fabro-web/app/lib/run-events.ts +++ b/apps/fabro-web/app/lib/run-events.ts @@ -118,6 +118,10 @@ const INFERENCE_EVENTS = new Set([ "agent.error", "agent.session.ended", ]); +const TOOL_TIMING_EVENTS = new Set([ + "agent.tool.started", + "agent.tool.completed", +]); // Todo / task mutation events refresh `getRunState` consumers (so per-stage // todo projections update live) and the run events list. const TODO_EVENTS = new Set([ @@ -184,6 +188,12 @@ export function queryKeysForRunEvent( if (AGENT_CONTROL_STATE_EVENTS.has(event)) { keys.unshift(queryKeys.runs.state(runId)); } + if (event === "agent.round.interrupted") { + keys.unshift( + queryKeys.runs.detail(runId), + queryKeys.runs.billing(runId), + ); + } if (stageId) { keys.push(queryKeys.runs.stageEvents(runId, stageId)); keys.push(queryKeys.runs.stageContextWindow(runId, stageId)); @@ -192,7 +202,11 @@ export function queryKeysForRunEvent( } if (INFERENCE_EVENTS.has(event)) { - const keys: Key[] = [queryKeys.runs.state(runId)]; + const keys: Key[] = [ + queryKeys.runs.detail(runId), + queryKeys.runs.state(runId), + queryKeys.runs.billing(runId), + ]; if (stageId) { keys.push(queryKeys.runs.stageEvents(runId, stageId)); if (event === "agent.message") { @@ -202,6 +216,19 @@ export function queryKeysForRunEvent( return keys; } + if (TOOL_TIMING_EVENTS.has(event)) { + const keys: Key[] = [ + queryKeys.runs.detail(runId), + queryKeys.runs.state(runId), + queryKeys.runs.billing(runId), + ]; + if (stageId) { + keys.push(queryKeys.runs.stageEvents(runId, stageId)); + keys.push(queryKeys.runs.stageContextWindow(runId, stageId)); + } + return keys; + } + if (event === "watchdog.timeout") { return stageId ? [queryKeys.runs.stageEvents(runId, stageId)] : []; } diff --git a/apps/fabro-web/app/routes/run-detail.tsx b/apps/fabro-web/app/routes/run-detail.tsx index 97ce00702..ad2d78986 100644 --- a/apps/fabro-web/app/routes/run-detail.tsx +++ b/apps/fabro-web/app/routes/run-detail.tsx @@ -69,6 +69,8 @@ import { export const handle = { hideHeader: true }; +const RUN_TIMING_REFRESH_INTERVAL_MS = 30_000; + type LifecycleTrigger = () => Promise; export function meta({ data }: any) { @@ -78,7 +80,7 @@ export function meta({ data }: any) { export default function RunDetail({ params }: { params: { id: string } }) { const demoMode = useDemoMode(); - const runQuery = useRun(params.id); + const runQuery = useRun(params.id, RUN_TIMING_REFRESH_INTERVAL_MS); const runStateQuery = useRunState(params.id); const summary = runQuery.data; const run = summary ? buildRunDetailRun(summary) : null; @@ -116,7 +118,7 @@ export default function RunDetail({ params }: { params: { id: string } }) { childrenCount, }); const steerBarRef = useRef(null); - const now = useTickingNow(30_000); + const now = useTickingNow(RUN_TIMING_REFRESH_INTERVAL_MS); const { fullHeight, hideSteerBar } = childRouteLayoutFlags(matches); useRunEvents(params.id); diff --git a/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md b/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md index 3683e5879..c0980ed01 100644 --- a/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md +++ b/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md @@ -155,8 +155,9 @@ git diff --check - Inference time is Fabro-observed LLM request/stream elapsed time, not provider-reported model-only compute time. -- LLM retry backoff, queueing outside a request/stream, human waits, steering - waits, and scheduler gaps are wall time but not active time. +- Queueing outside a request/stream, human waits, steering waits, and scheduler + gaps are wall time but not active time. Retry delay inside an open LLM request + bracket follows the executor stopwatch and counts as inference time. - ~~Active timing is finalized-event based in v1; live active-time ticking can be added later if it becomes necessary.~~ **Superseded 2026-07-25.** It became necessary: a run parked in one long agent stage reported ~12% of its wall time @@ -164,7 +165,6 @@ git diff --check accumulate inference and tool brackets from the event log and expose `StageProjection::live_timing(now)`, the active-time twin of `live_wall_time_ms`. Finalized values remain authoritative and still replace - the live estimate at terminal events. See - `.ai/plans/live-active-time-accumulation.md`. + the live estimate at terminal events. Implemented in PR #647. - No compatibility layer is required for existing API clients or stored run event data. diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 568aa30e5..f2460a69b 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -10772,9 +10772,16 @@ components: duplicated completion in a replayed log cannot drain the batch early. type: object required: + - session_id - started_at - open_call_ids properties: + session_id: + type: string + description: > + Root agent session that dispatched the batch. Transitions are + gated on it so delayed events from a replaced session cannot + mutate the current batch. started_at: type: string format: date-time @@ -10783,6 +10790,8 @@ components: other calls were outstanding. open_call_ids: type: array + minItems: 1 + uniqueItems: true items: type: string description: Calls dispatched but not yet completed, by tool call id. diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 0e2c8cbcb..4b9acdf1c 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -11,7 +11,7 @@ use axum_extra::extract::Query as ExtraQuery; use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use bytes::Bytes; -use chrono::Utc; +use chrono::{DateTime, Utc}; use fabro_api::types::{ BoardColumn, RunManifest, SubmitAnswerRequest, UpdateRunParentRequest, UpdateRunRequest, }; @@ -23,7 +23,7 @@ use fabro_store::{ }; use fabro_types::settings::ResolveError; use fabro_types::{ - AutomationRef, Principal, RunClientProvenance, RunId, RunProvenance, RunServerProvenance, + AutomationRef, Principal, Run, RunClientProvenance, RunId, RunProvenance, RunServerProvenance, RunStatusKind, StageContextWindow, StageContextWindowStaleness, StageContextWindowUnavailableReason, StageHandler, StageModelUsage, StageProjection, SystemActorKind, WorkflowSettings, parse_blob_ref, @@ -298,7 +298,7 @@ async fn validate_parent_link( } async fn updated_run_response(state: &AppState, run_id: &RunId) -> Response { - match state.stores.run_summaries.get(run_id, Utc::now()).await { + match run_summary_at(state, run_id, Utc::now()).await { Ok(Some(summary)) => ( StatusCode::OK, Json(state.decorate_run_summary(summary).await), @@ -311,6 +311,28 @@ async fn updated_run_response(state: &AppState, run_id: &RunId) -> Response { } } +/// Read the durable summary and overlay its timing from the live projection. +/// +/// The SQLite read model stores active timing as of the most recent event. +/// An open inference or tool bracket keeps accruing between events, so detail +/// reads need the projection's current estimate while the run is non-terminal. +async fn run_summary_at( + state: &AppState, + run_id: &RunId, + now: DateTime, +) -> fabro_store::Result> { + let Some(mut summary) = state.stores.run_summaries.get(run_id, now).await? else { + return Ok(None); + }; + if summary.timestamps.completed_at.is_none() { + let cached = state.stores.runs.get_cached_run(run_id).await?; + if let Some(timing) = cached.and_then(|cached| cached.projection.live_run_timing(now)) { + summary.timing = Some(timing); + } + } + Ok(Some(summary)) +} + async fn list_runs( _auth: RequiredRunManagementActor, State(state): State>, @@ -934,7 +956,7 @@ async fn get_run_status( RequireRunManagementTarget(id, _actor): RequireRunManagementTarget, State(state): State>, ) -> Response { - match state.stores.run_summaries.get(&id, Utc::now()).await { + match run_summary_at(&state, &id, Utc::now()).await { Ok(Some(run)) => { (StatusCode::OK, Json(state.decorate_run_summary(run).await)).into_response() } diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 71b4dfebf..790089ab1 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -5503,6 +5503,50 @@ async fn list_run_stages_exposes_execution_identity_for_resumed_stage() { assert_eq!(second["resumed_from_stage_id"], "work@1"); } +#[tokio::test] +async fn run_billing_includes_live_stage_timing_in_rows_and_totals() { + let state = test_app_state_with_isolated_storage(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let run_id = RunId::new(); + create_durable_run_with_events(&state, run_id, &[ + workflow_event::Event::RunSubmitted { + definition_blob: None, + }, + workflow_event::Event::RunStarting, + workflow_event::Event::RunRunning, + workflow_run_started_event(run_id), + ]) + .await; + append_scoped_stage_event( + &state, + run_id, + "work", + 1, + &stage_started_event("work", "command"), + ) + .await; + + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + let response = app + .oneshot( + Request::builder() + .method("GET") + .uri(api(&format!("/runs/{run_id}/billing"))) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let body = response_json!(response, StatusCode::OK).await; + let stages = body["stages"].as_array().unwrap(); + + assert_eq!(stages.len(), 1); + let row_timing = &stages[0]["timing"]; + assert!(row_timing["active_time_ms"].as_u64().unwrap() > 0); + assert_eq!(row_timing["tool_time_ms"], row_timing["active_time_ms"]); + assert_eq!(&body["totals"]["timing"], row_timing); +} + /// `checkpoint.completed_nodes` records every visit, so a looped node appears /// once per re-entry. Billing must dedup so a retried node renders as one row /// and `runtime_secs` is summed across all visits exactly once. @@ -7800,6 +7844,51 @@ async fn get_run_status_returns_status() { assert!(body["labels"].is_object()); } +#[tokio::test] +async fn get_run_status_advances_live_active_timing_between_events() { + let state = test_app_state_with_isolated_storage(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let run_id = RunId::new(); + create_durable_run_with_events(&state, run_id, &[ + workflow_event::Event::RunSubmitted { + definition_blob: None, + }, + workflow_event::Event::RunStarting, + workflow_event::Event::RunRunning, + workflow_run_started_event(run_id), + ]) + .await; + append_scoped_stage_event( + &state, + run_id, + "work", + 1, + &stage_started_event("work", "command"), + ) + .await; + + // The SQLite summary stores timing at the StageStarted event. A later + // detail read must overlay the in-flight command's active time from the + // projection even though no newer event has arrived. + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + let response = app + .oneshot( + Request::builder() + .method("GET") + .uri(api(&format!("/runs/{run_id}"))) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let body = response_json!(response, StatusCode::OK).await; + let timing = &body["timing"]; + + assert!(timing["active_time_ms"].as_u64().unwrap() > 0); + assert_eq!(timing["tool_time_ms"], timing["active_time_ms"]); + assert!(timing["wall_time_ms"].as_u64().unwrap() >= timing["active_time_ms"].as_u64().unwrap()); +} + #[tokio::test] async fn get_run_status_not_found() { let app = test_app_with(); diff --git a/lib/components/fabro-store/src/run_state.rs b/lib/components/fabro-store/src/run_state.rs index df05f84b3..14e86124a 100644 --- a/lib/components/fabro-store/src/run_state.rs +++ b/lib/components/fabro-store/src/run_state.rs @@ -19,6 +19,7 @@ use fabro_types::{ SandboxProviderKind, StageCompletion, StageHandler, StageId, StageInferenceProjection, StageModelUsage, StageOutcome, StageProjection, StageState, StartRecord, SubAgentProjection, SubAgentStatus, TodoListKind, TodoListProjection, TodoProjection, WorkflowRef, first_event_seq, + timing, }; use fabro_util::error::render_compact_with_causes; @@ -405,7 +406,7 @@ impl RunProjectionReducer for RunProjection { }; stage.response = response; stage.completion = Some(completion); - stage.timing = Some(props.timing); + stage.set_authoritative_timing(props.timing); if let Some(billing) = &props.billing { stage.usage.replace_with_billed_usage(billing); stage.model = Some(billing.model().clone()); @@ -428,7 +429,7 @@ impl RunProjectionReducer for RunProjection { failure_reason, timestamp: ts, }); - stage.timing = Some(props.timing); + stage.set_authoritative_timing(props.timing); if let Some(billing) = &props.billing { stage.usage.replace_with_billed_usage(billing); stage.model = Some(billing.model().clone()); @@ -481,7 +482,7 @@ impl RunProjectionReducer for RunProjection { close_inference_bracket(self, stored, props.visit, event.seq, ts); } EventBody::AgentSessionEnded(_) => { - close_inference_brackets_for_session(self, stored, ts); + close_active_brackets_for_session(self, stored, ts); } EventBody::AgentSessionActivated(props) => { let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) @@ -746,7 +747,11 @@ impl RunProjectionReducer for RunProjection { }); } EventBody::AgentToolStarted(props) => { - let is_root_session = stored.parent_session_id.is_none(); + let root_session_id = if stored.parent_session_id.is_none() { + stored.session_id.clone() + } else { + None + }; let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) else { return Ok(()); @@ -770,19 +775,22 @@ impl RunProjectionReducer for RunProjection { // A subagent's tools run inside the root session's tool call, // so the root batch already covers them. Timing them again // would double-count that span. - if is_root_session { - stage.open_tool_call(props.tool_call_id.clone(), ts); + if let Some(session_id) = root_session_id { + stage.open_tool_call(session_id, props.tool_call_id.clone(), ts); } } EventBody::AgentToolCompleted(props) => { if stored.parent_session_id.is_some() { return Ok(()); } + let Some(session_id) = stored.session_id.as_deref() else { + return Ok(()); + }; let Some(stage) = stage_at_stored_or_visit(self, stored, props.visit, event.seq) else { return Ok(()); }; - stage.close_tool_call(&props.tool_call_id, ts); + stage.close_tool_call(session_id, &props.tool_call_id, ts); } _ => {} } @@ -1123,16 +1131,10 @@ fn close_bracket_on_stage(stage: &mut StageProjection, ts: DateTime) { let Some(inference) = stage.inference.take() else { return; }; - stage.accumulate_inference_ms(elapsed_ms(inference.started_at, ts)); + stage.accumulate_inference_ms(timing::elapsed_ms(inference.started_at, ts)); } -/// Non-negative milliseconds between two instants, saturating at zero so a -/// clock skew or an out-of-order replay cannot produce a negative span. -fn elapsed_ms(from: DateTime, to: DateTime) -> u64 { - u64::try_from(to.signed_duration_since(from).num_milliseconds().max(0)).unwrap_or(0) -} - -/// Close every bracket opened by the session that just ended. +/// Close every active bracket opened by the session that just ended. /// /// `agent.session.ended` is the only ordering-safe backstop for terminal /// cancel and wall-clock timeout, which tear the session down through @@ -1147,7 +1149,7 @@ fn elapsed_ms(from: DateTime, to: DateTime) -> u64 { /// opened. Implemented as a normal stage lookup it would find no target and /// silently no-op, leaving the bracket open forever on exactly the path it /// exists to cover. -fn close_inference_brackets_for_session( +fn close_active_brackets_for_session( state: &mut RunProjection, stored: &RunEvent, ts: DateTime, @@ -1166,6 +1168,7 @@ fn close_inference_brackets_for_session( if opened_here { close_bracket_on_stage(stage, ts); } + stage.close_tool_batch_for_session(session_id, ts); } } @@ -1475,14 +1478,15 @@ fn finalize_unfinished_stages_after_run_failed( StageState::Failed }; - for (_, stage) in state.iter_stages_mut() { + for (_, stage) in state.iter_stages_unordered_mut() { if stage.state.is_terminal() { continue; } - // Close any bracket still open so its span is not dropped on the + // Close any brackets still open so their spans are not dropped on the // floor when the live estimate is frozen into `timing` below. close_bracket_on_stage(stage, timestamp); + stage.close_open_tool_batch(timestamp); // Freeze the live estimate before flipping to a terminal state: // `live_timing` reads `effective_state` and would return wall-only @@ -1490,7 +1494,9 @@ fn finalize_unfinished_stages_after_run_failed( let frozen = stage.live_timing(timestamp); stage.state = terminal_state; if stage.timing.is_none() && stage.started_at.is_some() { - stage.timing = Some(frozen); + stage.set_authoritative_timing(frozen); + } else { + stage.clear_live_timing(); } } } @@ -1641,12 +1647,16 @@ mod tests { StageId::new("plan", 1) } - fn agent_event(seq: u32, ts: &str, body: EventBody) -> EventEnvelope { + fn session_event(seq: u32, ts: &str, session_id: &str, body: EventBody) -> EventEnvelope { let mut event = test_stage_event_at(seq, ts, body, stage_id()); - event.event.session_id = Some("session-1".to_string()); + event.event.session_id = Some(session_id.to_string()); event } + fn agent_event(seq: u32, ts: &str, body: EventBody) -> EventEnvelope { + session_event(seq, ts, "session-1", body) + } + /// An event from a sub-agent session nested under the root session. fn child_event(seq: u32, ts: &str, body: EventBody) -> EventEnvelope { let mut event = agent_event(seq, ts, body); @@ -1855,6 +1865,81 @@ mod tests { assert!(stage(&state).tool_batch.is_some()); } + #[test] + fn a_foreign_session_completion_does_not_mutate_the_open_batch() { + let mut state = started_state(); + state + .apply_event(&agent_event( + 2, + "2026-04-07T12:00:00Z", + tool_started("call-a"), + )) + .unwrap(); + state + .apply_event(&session_event( + 3, + "2026-04-07T12:00:05Z", + "session-2", + tool_completed("call-a"), + )) + .unwrap(); + + let batch = stage(&state).tool_batch.as_ref().unwrap(); + assert_eq!(batch.session_id, "session-1"); + assert!(batch.open_call_ids.contains("call-a")); + assert_eq!(stage(&state).live_tool_ms, 0); + } + + #[test] + fn a_replacement_session_starts_a_separate_tool_batch() { + let mut state = started_state(); + state + .apply_event(&agent_event( + 2, + "2026-04-07T12:00:00Z", + tool_started("old-call"), + )) + .unwrap(); + state + .apply_event(&session_event( + 3, + "2026-04-07T12:00:05Z", + "session-2", + tool_started("new-call"), + )) + .unwrap(); + + assert_eq!(stage(&state).live_tool_ms, 5_000); + let batch = stage(&state).tool_batch.as_ref().unwrap(); + assert_eq!(batch.session_id, "session-2"); + assert_eq!( + batch.open_call_ids, + ["new-call".to_string()].into_iter().collect() + ); + + // A delayed completion from the old session cannot close the new + // session's batch even when call ids happen to collide. + state + .apply_event(&agent_event( + 4, + "2026-04-07T12:00:07Z", + tool_completed("new-call"), + )) + .unwrap(); + assert!(stage(&state).tool_batch.is_some()); + + state + .apply_event(&session_event( + 5, + "2026-04-07T12:00:09Z", + "session-2", + tool_completed("new-call"), + )) + .unwrap(); + assert_eq!(stage(&state).live_tool_ms, 9_000); + assert!(stage(&state).tool_batch.is_none()); + } + #[test] fn subagent_tool_calls_do_not_double_count_against_the_root_batch() { let mut state = started_state(); @@ -1892,14 +1977,21 @@ mod tests { } #[test] - fn session_end_accumulates_rather_than_discarding_the_bracket() { + fn session_end_accumulates_every_open_active_bracket() { let mut state = started_state(); state .apply_event(&agent_event(2, "2026-04-07T12:00:05Z", llm_started())) .unwrap(); + state + .apply_event(&agent_event( + 3, + "2026-04-07T12:00:07Z", + tool_started("call-a"), + )) + .unwrap(); let mut ended = test_stage_event_at( - 3, + 4, "2026-04-07T12:00:20Z", EventBody::AgentSessionEnded(AgentSessionEndedProps {}), stage_id(), @@ -1908,7 +2000,9 @@ mod tests { state.apply_event(&ended).unwrap(); assert_eq!(stage(&state).live_inference_ms, 15_000); + assert_eq!(stage(&state).live_tool_ms, 13_000); assert!(stage(&state).inference.is_none()); + assert!(stage(&state).tool_batch.is_none()); } #[test] @@ -1986,6 +2080,48 @@ mod tests { stage.timing.unwrap(), "a terminal stage reports its finalized breakdown, not a live estimate" ); + assert_eq!(stage.live_inference_ms, 0); + assert_eq!(stage.live_tool_ms, 0); + assert!(stage.inference.is_none()); + assert!(stage.tool_batch.is_none()); + } + + #[test] + fn run_failure_freezes_open_work_and_clears_live_bookkeeping() { + let mut state = started_state(); + state.status = RunStatus::Running; + state + .apply_event(&agent_event(2, "2026-04-07T12:00:01Z", llm_started())) + .unwrap(); + state + .apply_event(&agent_event(3, "2026-04-07T12:00:04Z", agent_message())) + .unwrap(); + state + .apply_event(&agent_event( + 4, + "2026-04-07T12:00:05Z", + tool_started("call-a"), + )) + .unwrap(); + + let mut failed = test_event( + 5, + EventBody::RunFailed(run_failed_props(FailureReason::WorkflowError)), + None, + ); + failed.event.ts = test_dt("2026-04-07T12:00:10Z"); + state.apply_event(&failed).unwrap(); + + let stage = stage(&state); + assert_eq!( + stage.timing, + Some(fabro_types::StageTiming::new(10_000, 3_000, 5_000)) + ); + assert_eq!(stage.state, StageState::Failed); + assert_eq!(stage.live_inference_ms, 0); + assert_eq!(stage.live_tool_ms, 0); + assert!(stage.inference.is_none()); + assert!(stage.tool_batch.is_none()); } } diff --git a/lib/foundation/fabro-api/build.rs b/lib/foundation/fabro-api/build.rs index fc943e99d..740585545 100644 --- a/lib/foundation/fabro-api/build.rs +++ b/lib/foundation/fabro-api/build.rs @@ -361,6 +361,11 @@ fn main() { "fabro_types::StageInferenceProjection", &[], ), + ( + "StageToolBatchProjection", + "fabro_types::StageToolBatchProjection", + &[], + ), ("LlmOutputKind", "fabro_types::LlmOutputKind", &[]), ("PermissionLevel", "fabro_types::PermissionLevel", &[]), ( diff --git a/lib/foundation/fabro-api/src/lib.rs b/lib/foundation/fabro-api/src/lib.rs index 4312acb91..b9a6a7185 100644 --- a/lib/foundation/fabro-api/src/lib.rs +++ b/lib/foundation/fabro-api/src/lib.rs @@ -69,10 +69,10 @@ pub mod types { StageContextWindowCategory, StageContextWindowCountMethod, StageContextWindowProjection, StageContextWindowStaleness, StageContextWindowUnavailableReason, StageContextWindowWarning, StageHandler, StageId, StageInferenceProjection, - StageModelUsage, StageOutcome, StageProjection, StageState, SubAgentProjection, - SubAgentStatus, SystemActorKind, SystemIntegrationStatus, SystemIntegrationsResponse, - TodoListProjection, TurnId, UpdateVariableRequest, UserPrincipal, Variable, - VariableListResponse, WorkflowSettings, + StageModelUsage, StageOutcome, StageProjection, StageState, StageToolBatchProjection, + SubAgentProjection, SubAgentStatus, SystemActorKind, SystemIntegrationStatus, + SystemIntegrationsResponse, TodoListProjection, TurnId, UpdateVariableRequest, + UserPrincipal, Variable, VariableListResponse, WorkflowSettings, }; pub use crate::generated::types::*; diff --git a/lib/foundation/fabro-api/tests/stage_projection_round_trip.rs b/lib/foundation/fabro-api/tests/stage_projection_round_trip.rs index 5faed1bd3..22a544e69 100644 --- a/lib/foundation/fabro-api/tests/stage_projection_round_trip.rs +++ b/lib/foundation/fabro-api/tests/stage_projection_round_trip.rs @@ -18,6 +18,7 @@ use fabro_api::types::{ StageContextWindowUnavailableReason as ApiStageContextWindowUnavailableReason, StageContextWindowWarning as ApiStageContextWindowWarning, StageInferenceProjection as ApiStageInferenceProjection, StageProjection as ApiStageProjection, + StageToolBatchProjection as ApiStageToolBatchProjection, SubAgentProjection as ApiSubAgentProjection, SubAgentStatus as ApiSubAgentStatus, TodoListProjection as ApiTodoListProjection, }; @@ -29,8 +30,8 @@ use fabro_types::{ ParallelBranchResult, PermissionLevel, SkillsProjection, StageContextWindow, StageContextWindowBreakdownItem, StageContextWindowCategory, StageContextWindowCountMethod, StageContextWindowProjection, StageContextWindowStaleness, StageContextWindowUnavailableReason, - StageContextWindowWarning, StageInferenceProjection, StageProjection, SubAgentProjection, - SubAgentStatus, TodoListKind, TodoListProjection, + StageContextWindowWarning, StageInferenceProjection, StageProjection, StageToolBatchProjection, + SubAgentProjection, SubAgentStatus, TodoListKind, TodoListProjection, }; use serde_json::json; @@ -68,9 +69,32 @@ fn stage_projection_reuses_nested_agent_state_types() { ); assert_same_type::(); assert_same_type::(); + assert_same_type::(); assert_same_type::(); } +#[test] +fn stage_tool_batch_projection_matches_openapi_json_shape() { + let batch = StageToolBatchProjection { + session_id: "ses_root".to_string(), + started_at: "2026-04-29T12:34:00Z".parse().unwrap(), + open_call_ids: ["call_1".to_string(), "call_2".to_string()] + .into_iter() + .collect(), + }; + let value = serde_json::to_value(&batch).unwrap(); + assert_eq!( + value, + json!({ + "session_id": "ses_root", + "started_at": "2026-04-29T12:34:00Z", + "open_call_ids": ["call_1", "call_2"] + }) + ); + let api_batch: ApiStageToolBatchProjection = serde_json::from_value(value).unwrap(); + assert_eq!(api_batch, batch); +} + #[test] fn stage_inference_projection_matches_openapi_json_shape() { let inference = StageInferenceProjection { @@ -148,6 +172,9 @@ fn stage_projection_without_inference_round_trips() { let stage: StageProjection = serde_json::from_value(value.clone()).unwrap(); assert!(stage.inference.is_none()); + assert!(stage.tool_batch.is_none()); + assert_eq!(stage.live_inference_ms, 0); + assert_eq!(stage.live_tool_ms, 0); assert_eq!(serde_json::to_value(stage).unwrap(), value); } diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 68f5644a2..b7f3b0474 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -125,7 +125,7 @@ pub use run_projection::{ StageContextWindowBreakdownItem, StageContextWindowCategory, StageContextWindowCountMethod, StageContextWindowProjection, StageContextWindowStaleness, StageContextWindowUnavailableReason, StageContextWindowWarning, StageInferenceProjection, StageModelUsage, StageProjection, - SubAgentProjection, SubAgentStatus, first_event_seq, + StageToolBatchProjection, SubAgentProjection, SubAgentStatus, first_event_seq, }; pub use run_sandbox::{ RunSandbox, RunSandboxFailure, RunSandboxInstance, RunSandboxKind, RunSandboxPlan, diff --git a/lib/foundation/fabro-types/src/run_projection.rs b/lib/foundation/fabro-types/src/run_projection.rs index 0576d32f6..e2b9ad3f6 100644 --- a/lib/foundation/fabro-types/src/run_projection.rs +++ b/lib/foundation/fabro-types/src/run_projection.rs @@ -12,7 +12,7 @@ use crate::{ AgentToolSummary, BilledTokenCounts, Checkpoint, Conclusion, InterviewQuestionRecord, InvalidTransition, LlmOutputKind, ModelRef, PermissionLevel, PullRequestLink, RunApproval, RunControlAction, RunDiff, RunId, RunSandbox, RunSpec, RunStatus, RunTiming, StageCompletion, - StageHandler, StageId, StageState, StageTiming, StartRecord, TodoListProjection, + StageHandler, StageId, StageState, StageTiming, StartRecord, TodoListProjection, timing, }; #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] @@ -433,6 +433,10 @@ fn is_zero_ms(value: &u64) -> bool { /// duplicated log must not let a repeated completion drain the batch early. #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct StageToolBatchProjection { + /// Root agent session that dispatched the batch. Later transitions are + /// gated on it so delayed events from a replaced session cannot mutate + /// the current session's batch. + pub session_id: String, /// When the batch opened — the first `agent.tool.started` observed while /// no other calls were outstanding. pub started_at: DateTime, @@ -603,10 +607,9 @@ impl StageProjection { state, StageState::Running | StageState::Retrying | StageState::Pending ) { - return self.started_at.map(|started| { - u64::try_from(now.signed_duration_since(started).num_milliseconds().max(0)) - .unwrap_or(0) - }); + return self + .started_at + .map(|started| timing::elapsed_ms(started, now)); } self.timing.map(|timing| timing.wall_time_ms) } @@ -645,9 +648,6 @@ impl StageProjection { } let wall_time_ms = self.live_wall_time_ms(now).unwrap_or(0); - let elapsed = |since: DateTime| { - u64::try_from(now.signed_duration_since(since).num_milliseconds().max(0)).unwrap_or(0) - }; // `handler` is absent on projections built from events written before // stage execution identity. Treat those as agent stages, matching @@ -660,11 +660,11 @@ impl StageProjection { let open_inference = self .inference .as_ref() - .map_or(0, |inference| elapsed(inference.started_at)); + .map_or(0, |inference| timing::elapsed_ms(inference.started_at, now)); let open_tool = self .tool_batch .as_ref() - .map_or(0, |batch| elapsed(batch.started_at)); + .map_or(0, |batch| timing::elapsed_ms(batch.started_at, now)); ( self.live_inference_ms.saturating_add(open_inference), self.live_tool_ms.saturating_add(open_tool), @@ -693,9 +693,26 @@ impl StageProjection { } /// Record a dispatched tool call, opening a batch if none is outstanding. - pub fn open_tool_call(&mut self, tool_call_id: String, started_at: DateTime) { + /// + /// If a replacement root session starts work before the old session's end + /// event arrives, freeze the old batch at this boundary before opening + /// the new one. This keeps the sessions separate without dropping time. + pub fn open_tool_call( + &mut self, + session_id: String, + tool_call_id: String, + started_at: DateTime, + ) { + let replaces_open_batch = self + .tool_batch + .as_ref() + .is_some_and(|batch| batch.session_id != session_id); + if replaces_open_batch { + self.close_open_tool_batch(started_at); + } self.tool_batch .get_or_insert_with(|| StageToolBatchProjection { + session_id, started_at, open_call_ids: BTreeSet::new(), }) @@ -705,21 +722,52 @@ impl StageProjection { /// Retire a tool call. Folds the batch into the live accumulator once the /// last outstanding call reports, so concurrent calls count once. - pub fn close_tool_call(&mut self, tool_call_id: &str, now: DateTime) { + pub fn close_tool_call(&mut self, session_id: &str, tool_call_id: &str, now: DateTime) { let Some(batch) = self.tool_batch.as_mut() else { return; }; - batch.open_call_ids.remove(tool_call_id); - if !batch.open_call_ids.is_empty() { + if batch.session_id != session_id + || !batch.open_call_ids.remove(tool_call_id) + || !batch.open_call_ids.is_empty() + { return; } - let elapsed = u64::try_from( - now.signed_duration_since(batch.started_at) - .num_milliseconds() - .max(0), - ) - .unwrap_or(0); - self.live_tool_ms = self.live_tool_ms.saturating_add(elapsed); + self.close_open_tool_batch(now); + } + + /// Close a tool batch only when it belongs to `session_id`. + pub fn close_tool_batch_for_session(&mut self, session_id: &str, now: DateTime) { + let opened_here = self + .tool_batch + .as_ref() + .is_some_and(|batch| batch.session_id == session_id); + if opened_here { + self.close_open_tool_batch(now); + } + } + + /// Fold any open tool batch into the live accumulator. + pub fn close_open_tool_batch(&mut self, now: DateTime) { + let Some(batch) = self.tool_batch.take() else { + return; + }; + self.live_tool_ms = self + .live_tool_ms + .saturating_add(timing::elapsed_ms(batch.started_at, now)); + } + + /// Install a worker-provided terminal timing and discard transient live + /// bookkeeping that is no longer authoritative. + pub fn set_authoritative_timing(&mut self, timing: StageTiming) { + self.timing = Some(timing); + self.clear_live_timing(); + } + + /// Discard transient timing accumulators and open brackets. + pub fn clear_live_timing(&mut self) { + self.live_inference_ms = 0; + self.live_tool_ms = 0; + self.inference = None; self.tool_batch = None; } @@ -1138,20 +1186,15 @@ mod iter_stages_tests { #[cfg(test)] mod live_timing_tests { use std::collections::HashMap; - use std::num::NonZeroU32; use chrono::{DateTime, TimeZone, Utc}; use super::{RunProjection, StageToolBatchProjection}; use crate::{ Graph, ModelRef, RunId, RunSpec, StageHandler, StageInferenceProjection, StageProjection, - StageState, StageTiming, StartRecord, WorkflowSettings, test_support, + StageState, StageTiming, StartRecord, WorkflowSettings, first_event_seq, test_support, }; - fn seq(n: u32) -> NonZeroU32 { - NonZeroU32::new(n).unwrap() - } - fn at(seconds: i64) -> DateTime { Utc.timestamp_opt(1_700_000_000 + seconds, 0).unwrap() } @@ -1180,7 +1223,7 @@ mod live_timing_tests { /// In-flight stage that started at `at(0)`. fn running(handler: StageHandler) -> StageProjection { - let mut stage = StageProjection::new(seq(1)); + let mut stage = StageProjection::new(first_event_seq(1)); stage.handler = Some(handler); stage.started_at = Some(at(0)); stage.state = StageState::Running; @@ -1225,6 +1268,7 @@ mod live_timing_tests { // Inference open for 20s, tools open for 10s, at t=120s. stage.inference = Some(open_bracket(at(100))); stage.tool_batch = Some(StageToolBatchProjection { + session_id: "session-1".to_string(), started_at: at(110), open_call_ids: ["call-1".to_string()].into_iter().collect(), }); @@ -1330,17 +1374,17 @@ mod live_timing_tests { base_sha: None, }); - let baseline = projection.stage_entry("baseline", 1, seq(1)); + let baseline = projection.stage_entry("baseline", 1, first_event_seq(1)); baseline.handler = Some(StageHandler::Command); baseline.state = StageState::Succeeded; baseline.timing = Some(StageTiming::new(42_666, 0, 42_663)); - let assess = projection.stage_entry("assess", 1, seq(2)); + let assess = projection.stage_entry("assess", 1, first_event_seq(2)); assess.handler = Some(StageHandler::Agent); assess.state = StageState::Succeeded; assess.timing = Some(StageTiming::new(86_025, 78_230, 7_588)); - let plan = projection.stage_entry("plan", 1, seq(3)); + let plan = projection.stage_entry("plan", 1, first_event_seq(3)); plan.handler = Some(StageHandler::Agent); plan.started_at = Some(at(146)); plan.state = StageState::Running; @@ -1367,7 +1411,8 @@ mod live_timing_tests { }); for (index, node) in ["branch-a", "branch-b", "branch-c"].iter().enumerate() { - let stage = projection.stage_entry(node, 1, seq(u32::try_from(index).unwrap() + 1)); + let stage = + projection.stage_entry(node, 1, first_event_seq(u32::try_from(index).unwrap() + 1)); stage.handler = Some(StageHandler::Agent); stage.state = StageState::Succeeded; stage.timing = Some(StageTiming::new(60_000, 60_000, 0)); diff --git a/lib/foundation/fabro-types/src/timing.rs b/lib/foundation/fabro-types/src/timing.rs index 4fe45a36f..812a72ef2 100644 --- a/lib/foundation/fabro-types/src/timing.rs +++ b/lib/foundation/fabro-types/src/timing.rs @@ -18,6 +18,16 @@ use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; +/// Non-negative milliseconds between two instants. +/// +/// Clock skew or an out-of-order replay can put `end` before `start`; those +/// spans contribute zero rather than wrapping into a large unsigned value. +#[must_use] +pub fn elapsed_ms(start: DateTime, end: DateTime) -> u64 { + u64::try_from(end.signed_duration_since(start).num_milliseconds().max(0)) + .expect("non-negative chrono millisecond durations fit in u64") +} + /// Timing breakdown for one stage visit. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct StageTiming { @@ -74,16 +84,18 @@ impl StageTiming { /// legitimately sum past run wall time. #[must_use] pub fn clamped_to_wall(&self) -> Self { - if self.active_time_ms <= self.wall_time_ms { + let active_time_ms = u128::from(self.inference_time_ms) + u128::from(self.tool_time_ms); + if active_time_ms <= u128::from(self.wall_time_ms) { return *self; } // Preserve the split rather than truncating one side, so a clamped // stage still shows where its time went. Widen for the multiply: the - // quotient is bounded by `wall_time_ms` because `active_time_ms` - // exceeds it here, so it always fits back into u64. - let scaled = u128::from(self.inference_time_ms) * u128::from(self.wall_time_ms) - / u128::from(self.active_time_ms); - let inference_time_ms = u64::try_from(scaled).unwrap_or(self.wall_time_ms); + // quotient is bounded by `wall_time_ms` because the exact, widened + // active total exceeds it here, so it always fits back into u64. + let scaled = + u128::from(self.inference_time_ms) * u128::from(self.wall_time_ms) / active_time_ms; + let inference_time_ms = + u64::try_from(scaled).expect("scaled inference time is bounded by wall time"); let tool_time_ms = self.wall_time_ms.saturating_sub(inference_time_ms); Self::new(self.wall_time_ms, inference_time_ms, tool_time_ms) } @@ -166,8 +178,7 @@ impl RunTiming { /// Milliseconds elapsed from `start` to `now`, clamped at zero. #[must_use] pub fn wall_time_ms_since(start: DateTime, now: DateTime) -> u64 { - u64::try_from(now.signed_duration_since(start).num_milliseconds().max(0)) - .expect("non-negative milliseconds fit in u64") + elapsed_ms(start, now) } } @@ -184,7 +195,9 @@ impl From for RunTiming { #[cfg(test)] mod tests { - use super::{RunTiming, StageTiming}; + use chrono::{TimeZone, Utc}; + + use super::{RunTiming, StageTiming, elapsed_ms}; #[test] fn stage_timing_new_derives_active_as_sum_of_inference_and_tool() { @@ -215,6 +228,23 @@ mod tests { assert_eq!(sum.active_time_ms, 175); } + #[test] + fn stage_timing_clamp_uses_the_unsaturated_active_total() { + let timing = StageTiming::new(u64::MAX, u64::MAX, u64::MAX).clamped_to_wall(); + + assert_eq!(timing.inference_time_ms, u64::MAX / 2); + assert_eq!(timing.tool_time_ms, u64::MAX.saturating_sub(u64::MAX / 2)); + assert_eq!(timing.active_time_ms, u64::MAX); + } + + #[test] + fn elapsed_ms_clamps_out_of_order_instants_to_zero() { + let later = Utc.timestamp_opt(100, 0).unwrap(); + let earlier = Utc.timestamp_opt(99, 0).unwrap(); + + assert_eq!(elapsed_ms(later, earlier), 0); + } + #[test] fn run_timing_wall_only_zeroes_breakdown_and_active() { let timing = RunTiming::wall_only(1500); diff --git a/lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts b/lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts index a6bd7718c..c382b5dd1 100644 --- a/lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts +++ b/lib/packages/fabro-api-client/src/models/stage-tool-batch-projection.ts @@ -18,6 +18,10 @@ * One open tool batch: tool calls dispatched together that have not all reported completion. `open_call_ids` is a set rather than a count so a duplicated completion in a replayed log cannot drain the batch early. */ export interface StageToolBatchProjection { + /** + * Root agent session that dispatched the batch. Transitions are gated on it so delayed events from a replaced session cannot mutate the current batch. + */ + 'session_id': string; /** * When the batch opened — the first dispatched call observed while no other calls were outstanding. */ @@ -25,5 +29,5 @@ export interface StageToolBatchProjection { /** * Calls dispatched but not yet completed, by tool call id. */ - 'open_call_ids': Array; + 'open_call_ids': Set; } From 0b24649e7617d372d1ff1738ee258719fb767240 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 26 Jul 2026 09:25:47 -0400 Subject: [PATCH 03/36] fix(cli): keep offline validation catalog-free --- lib/apps/fabro-cli/src/commands/run/create.rs | 7 +- lib/apps/fabro-cli/src/commands/run/runner.rs | 7 +- lib/apps/fabro-cli/src/commands/validate.rs | 6 +- lib/apps/fabro-cli/tests/it/cmd/create.rs | 46 +++++++ lib/apps/fabro-cli/tests/it/cmd/validate.rs | 33 +++++ .../fabro-mcp-server/src/manifest_builder.rs | 2 +- .../fabro-server/src/manifest_validation.rs | 23 +++- lib/apps/fabro-server/src/run_manifest.rs | 40 +++--- .../fabro-server/src/run_tool_manifest.rs | 11 +- .../fabro-server/src/server/handler/graph.rs | 2 +- .../fabro-server/src/server/handler/runs.rs | 22 ++- .../src/handler/manager_loop.rs | 42 +++--- .../fabro-workflow/src/operations/create.rs | 109 +++++++++++---- .../fabro-workflow/src/operations/mod.rs | 2 +- .../fabro-workflow/src/operations/validate.rs | 73 +++++++--- .../fabro-workflow/src/pipeline/mod.rs | 8 +- .../fabro-workflow/src/pipeline/transform.rs | 128 +++++++++++------- .../fabro-workflow/src/pipeline/types.rs | 34 ++++- .../fabro-workflow/src/pipeline/validate.rs | 47 ++++--- .../fabro-workflow/tests/it/integration.rs | 23 ++-- 20 files changed, 449 insertions(+), 216 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index ad4360384..159160d7e 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -61,11 +61,8 @@ pub(crate) async fn create_run( None }; - let mut validation = manifest_validation::validate_manifest( - &RunLayer::default(), - &built.manifest, - ctx.catalog()?, - )?; + let mut validation = + manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; manifest_validation::promote_template_undefined_variables_to_errors(&mut validation); let diagnostics = api_diagnostics_to_local(&validation.workflow.diagnostics); if !quiet { diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 4e6b72b8b..4c6fcf578 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -263,12 +263,7 @@ impl fabro_tool::RunManifestBuilder for WorkerRunManifestBuilder { cwd: &Path, user_settings_path: &Path, ) -> fabro_tool::ToolResult { - run_tool_manifest::build_run_tool_manifest( - spec, - cwd, - user_settings_path, - Arc::clone(&self.catalog), - ) + run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, &self.catalog) } } diff --git a/lib/apps/fabro-cli/src/commands/validate.rs b/lib/apps/fabro-cli/src/commands/validate.rs index b87460f28..e157b50b3 100644 --- a/lib/apps/fabro-cli/src/commands/validate.rs +++ b/lib/apps/fabro-cli/src/commands/validate.rs @@ -23,11 +23,7 @@ pub(crate) fn run( user_settings_path: Some(active_settings_path(None)), ..Default::default() })?; - let response = manifest_validation::validate_manifest( - &RunLayer::default(), - &built.manifest, - base_ctx.catalog()?, - )?; + let response = manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; let diagnostics = api_diagnostics_to_local(&response.workflow.diagnostics); if base_ctx.json_output() { diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index 5a9a4402f..462a0996f 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -101,6 +101,52 @@ fn create_uses_explicit_server_target_and_prints_remote_run_id() { assert_eq!(output_stdout(&output).trim(), run_id.as_str()); } +#[test] +fn create_defers_provider_validation_to_the_server() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let mock = server.mock(|when, then| { + when.method("POST") + .path("/api/v1/runs") + .body_includes(r#"provider=\"server-only\""#); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + let workflow_path = context.temp_dir.join("server-model.fabro"); + context.write_temp( + "server-model.fabro", + r#"digraph ServerModel { + graph [goal="Use a server-owned model"] + start [shape=Mdiamond] + work [prompt="Do work", model="private-model", provider="server-only"] + exit [shape=Msquare] + start -> work -> exit + }"#, + ); + + let output = context + .create_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + workflow_path.to_str().unwrap(), + ]) + .output() + .expect("command should execute"); + + assert!( + output.status.success(), + "local validation should not reject a server-owned provider\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + mock.assert(); + assert_eq!(output_stdout(&output).trim(), run_id.as_str()); +} + #[test] fn create_uses_configured_server_target_without_server_flag() { let context = test_context!(); diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 190c2817b..2bbec8c76 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -69,6 +69,39 @@ fn simple_does_not_connect_to_configured_server() { ); } +#[test] +#[expect( + clippy::disallowed_methods, + reason = "sync CLI test writes one workflow fixture before spawning the subprocess" +)] +fn server_owned_provider_is_not_rejected_by_offline_validation() { + let cli = LightweightCli::new(); + let workflow = cli.home().join("server-model.fabro"); + std::fs::write( + &workflow, + r#"digraph ServerModel { + graph [goal="Use a server-owned model"] + start [shape=Mdiamond] + work [prompt="Do work", model="private-model", provider="server-only"] + exit [shape=Msquare] + start -> work -> exit + }"#, + ) + .expect("workflow fixture should be written"); + let mut cmd = cli.command(); + cmd.env("FABRO_SERVER", "http://127.0.0.1:9") + .arg("validate") + .arg(&workflow); + + let output = cmd.output().expect("validate should execute"); + assert!( + output.status.success(), + "offline validation should leave provider availability to the server\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} + #[test] fn branching() { let context = test_context!(); diff --git a/lib/apps/fabro-mcp-server/src/manifest_builder.rs b/lib/apps/fabro-mcp-server/src/manifest_builder.rs index 53e899f1e..8c1cb8125 100644 --- a/lib/apps/fabro-mcp-server/src/manifest_builder.rs +++ b/lib/apps/fabro-mcp-server/src/manifest_builder.rs @@ -32,5 +32,5 @@ fn build_mcp_run_manifest( Catalog::from_builtin_with_overrides(&llm_catalog_settings) .map_err(|err| ToolError::message(err.to_string()))?, ); - run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, catalog) + run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, &catalog) } diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index baca3f014..3016fde65 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -12,21 +12,34 @@ use crate::run_manifest; pub fn validate_manifest( manifest_run_defaults: &RunLayer, manifest: &types::RunManifest, - catalog: Arc, ) -> Result { validate_manifest_with_environment_defaults( manifest_run_defaults, &fabro_environment::seeded_catalog_layer(), manifest, - catalog, ) } +pub fn validate_manifest_with_catalog( + manifest_run_defaults: &RunLayer, + manifest: &types::RunManifest, + catalog: &Arc, +) -> Result { + let prepared = run_manifest::prepare_manifest_with_environment_defaults( + manifest_run_defaults, + &fabro_environment::seeded_catalog_layer(), + &HashMap::new(), + manifest, + )?; + let validated = + run_manifest::validate_prepared_manifest(&prepared, catalog).map_err(anyhow::Error::new)?; + Ok(run_manifest::validate_response(&prepared, &validated)) +} + pub fn validate_manifest_with_environment_defaults( manifest_run_defaults: &RunLayer, manifest_environment_defaults: &MergeMap, manifest: &types::RunManifest, - catalog: Arc, ) -> Result { let prepared = run_manifest::prepare_manifest_with_environment_defaults( manifest_run_defaults, @@ -34,8 +47,8 @@ pub fn validate_manifest_with_environment_defaults( &HashMap::new(), manifest, )?; - let validated = - run_manifest::validate_prepared_manifest(&prepared, catalog).map_err(anyhow::Error::new)?; + let validated = run_manifest::validate_prepared_manifest_structural(&prepared) + .map_err(anyhow::Error::new)?; Ok(run_manifest::validate_response(&prepared, &validated)) } diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 65cc6d36e..76a81db51 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -35,7 +35,8 @@ use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSecti use fabro_validate::Severity; use fabro_workflow::Error as WorkflowError; use fabro_workflow::operations::{ - CreateRunInput, ValidateInput, WorkflowInput, validate, validate_with_ready_providers, + CreateRunInput, ValidateInput, WorkflowInput, validate, validate_with_catalog, + validate_with_ready_providers, }; use fabro_workflow::pipeline::Validated; use fabro_workflow::run_materialization::materialize_run_with_ready_providers; @@ -187,34 +188,40 @@ pub(crate) fn prepare_manifest_with_environment_defaults( pub(crate) fn validate_prepared_manifest( prepared: &PreparedManifest, - catalog: Arc, + catalog: &Arc, ) -> Result { validate_prepared_manifest_with_vars(prepared, catalog, HashMap::new()) } +pub(crate) fn validate_prepared_manifest_structural( + prepared: &PreparedManifest, +) -> Result { + validate(manifest_validate_input(prepared, HashMap::new())) +} + pub(crate) fn validate_prepared_manifest_with_vars( prepared: &PreparedManifest, - catalog: Arc, + catalog: &Arc, vars: HashMap, ) -> Result { - validate(manifest_validate_input(prepared, catalog, vars)) + validate_with_catalog(manifest_validate_input(prepared, vars), catalog) } pub(crate) fn validate_prepared_manifest_for_preflight( prepared: &PreparedManifest, - catalog: Arc, + catalog: &Arc, vars: HashMap, ready_providers: &[ProviderId], ) -> Result { validate_with_ready_providers( - manifest_validate_input(prepared, catalog, vars), + manifest_validate_input(prepared, vars), + catalog, ready_providers, ) } fn manifest_validate_input( prepared: &PreparedManifest, - catalog: Arc, vars: HashMap, ) -> ValidateInput { ValidateInput { @@ -223,7 +230,6 @@ fn manifest_validate_input( vars, cwd: prepared.cwd.clone(), custom_transforms: Vec::new(), - catalog, } } @@ -1548,7 +1554,7 @@ digraph Demo {{ .unwrap(); let validated = validate_prepared_manifest_for_preflight( &prepared, - state.catalog(), + &state.catalog(), HashMap::new(), &ready_providers, ) @@ -1633,7 +1639,7 @@ enabled = {clone_enabled} &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); let resolved = materialize_run( prepared.settings.clone(), validated.graph(), @@ -2193,7 +2199,7 @@ name = "Control Plane" &invalid_manifest(), ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); assert!(validated.has_errors()); @@ -2238,7 +2244,7 @@ issues = "read" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); assert!(!validated.has_errors()); let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) @@ -2288,7 +2294,7 @@ id = "local" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); assert!(!validated.has_errors()); @@ -2397,7 +2403,7 @@ id = "daytona" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) .await @@ -2465,7 +2471,7 @@ digraph Demo { &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); let (response, ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) .await @@ -2579,7 +2585,7 @@ digraph Demo { &manifest, ) .unwrap(); - let Err(error) = validate_prepared_manifest(&prepared, test_catalog()) else { + let Err(error) = validate_prepared_manifest(&prepared, &test_catalog()) else { panic!("unknown provider should fail static validation"); }; @@ -2646,7 +2652,7 @@ digraph Demo { assert!(ready_providers.is_empty()); let validated = validate_prepared_manifest_for_preflight( &prepared, - state.catalog(), + &state.catalog(), HashMap::new(), &ready_providers, ) diff --git a/lib/apps/fabro-server/src/run_tool_manifest.rs b/lib/apps/fabro-server/src/run_tool_manifest.rs index 8e38e8317..66204e208 100644 --- a/lib/apps/fabro-server/src/run_tool_manifest.rs +++ b/lib/apps/fabro-server/src/run_tool_manifest.rs @@ -14,7 +14,7 @@ pub fn build_run_tool_manifest( spec: &ValidatedCreateRunSpec, cwd: &Path, user_settings_path: &Path, - catalog: Arc, + catalog: &Arc, ) -> ToolResult { let built = fabro_manifest::build_run_manifest(ManifestBuildInput { workflow: PathBuf::from(&spec.workflow), @@ -29,9 +29,12 @@ pub fn build_run_tool_manifest( }) .map_err(|err| ToolError::from_anyhow(&err))?; - let mut validation = - manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest, catalog) - .map_err(|err| ToolError::from_anyhow(&err))?; + let mut validation = manifest_validation::validate_manifest_with_catalog( + &RunLayer::default(), + &built.manifest, + catalog, + ) + .map_err(|err| ToolError::from_anyhow(&err))?; manifest_validation::promote_template_undefined_variables_to_errors(&mut validation); if !validation.ok { return Err(ToolError::message("workflow manifest validation failed")); diff --git a/lib/apps/fabro-server/src/server/handler/graph.rs b/lib/apps/fabro-server/src/server/handler/graph.rs index 364b19cad..ea95b674a 100644 --- a/lib/apps/fabro-server/src/server/handler/graph.rs +++ b/lib/apps/fabro-server/src/server/handler/graph.rs @@ -52,7 +52,7 @@ async fn render_graph_from_manifest( Ok(prepared) => prepared, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; - let validated = match run_manifest::validate_prepared_manifest(&prepared, state.catalog()) { + let validated = match run_manifest::validate_prepared_manifest(&prepared, &state.catalog()) { Ok(validated) => validated, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 0e2c8cbcb..a0cbcbf70 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -829,7 +829,7 @@ async fn run_preflight( let (llm_result, ready_providers) = state.resolve_llm_client_with_ready_ids().await; let mut validated = match run_manifest::validate_prepared_manifest_for_preflight( &prepared, - state.catalog(), + &state.catalog(), vars, &ready_providers, ) { @@ -879,17 +879,15 @@ async fn validate_run_manifest( return ApiError::bad_request(format!("Run config variable interpolation failed: {err}")) .into_response(); } - let validated = match run_manifest::validate_prepared_manifest_with_vars( - &prepared, - state.catalog(), - vars, - ) { - Ok(validated) => validated, - Err(WorkflowError::Parse(_)) => { - return ApiError::bad_request("Validation failed").into_response(); - } - Err(err) => return ApiError::bad_request(err.to_string()).into_response(), - }; + let validated = + match run_manifest::validate_prepared_manifest_with_vars(&prepared, &state.catalog(), vars) + { + Ok(validated) => validated, + Err(WorkflowError::Parse(_)) => { + return ApiError::bad_request("Validation failed").into_response(); + } + Err(err) => return ApiError::bad_request(err.to_string()).into_response(), + }; ( StatusCode::OK, Json(run_manifest::validate_response(&prepared, &validated)), diff --git a/lib/components/fabro-workflow/src/handler/manager_loop.rs b/lib/components/fabro-workflow/src/handler/manager_loop.rs index 98804d5e9..ddefe84be 100644 --- a/lib/components/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/components/fabro-workflow/src/handler/manager_loop.rs @@ -16,7 +16,7 @@ use crate::artifact_upload::ArtifactSink; use crate::condition::evaluate_condition; use crate::context::{Context, WorkflowContext, context_diff_public, keys}; use crate::error::Error; -use crate::operations::{ValidateInput, WorkflowInput, validate}; +use crate::operations::{ValidateInput, WorkflowInput, validate_with_catalog}; use crate::outcome::{Outcome, OutcomeExt, StageOutcome}; use crate::pipeline::types::Initialized; use crate::run_options::RunOptions; @@ -65,17 +65,19 @@ fn parse_child_graph(node: &Node, services: &EngineServices) -> Result Result Some(workflow.path.clone()), WorkflowInput::Path(_) | WorkflowInput::DotSource { .. } => None, }; - let mut validated = validate(ValidateInput { - workflow, - settings: WorkflowSettings::default(), - vars: std::collections::HashMap::new(), - cwd, - custom_transforms: Vec::new(), - catalog: Arc::clone(&services.run.catalog), - })?; + let mut validated = validate_with_catalog( + ValidateInput { + workflow, + settings: WorkflowSettings::default(), + vars: std::collections::HashMap::new(), + cwd, + custom_transforms: Vec::new(), + }, + &services.run.catalog, + )?; validated.promote_template_undefined_variables_to_errors(); validated.raise_on_errors()?; let (graph, _, _) = validated.into_parts(); diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 2ec40d443..4fff69898 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -24,7 +24,9 @@ use crate::error::Error; use crate::event::{Event, append_event, to_run_event_at}; use crate::file_resolver::FileResolver; use crate::pipeline::types::PersistOptions; -use crate::pipeline::{self, Persisted, TransformOptions, Validated}; +use crate::pipeline::{ + self, ModelResolutionOptions, Persisted, TransformOptions, Transformed, Validated, +}; use crate::records::RunSpec; use crate::run_lookup::default_scratch_base; use crate::run_materialization::materialize_run; @@ -340,6 +342,69 @@ pub(super) fn preprocess_and_validate( catalog_fallback: bool, catalog: &Arc, ) -> Result { + let model_resolution = ModelResolutionOptions { + catalog: Arc::clone(catalog), + default_provider, + eligible_providers: eligible_providers.iter().cloned().collect(), + catalog_fallback, + }; + let transformed = preprocess( + dot_source, + source_name, + current_dir, + file_resolver, + custom_transforms, + template_context, + goal_override, + render_mode, + Some(model_resolution), + )?; + Ok(pipeline::validate_with_catalog( + transformed, + catalog.as_ref(), + &[], + )) +} + +pub(super) fn preprocess_and_validate_structural( + dot_source: &str, + source_name: Option, + current_dir: Option, + file_resolver: Option>, + custom_transforms: Vec>, + template_context: TemplateContext, + goal_override: Option<&str>, + render_mode: RenderMode, +) -> Result { + let transformed = preprocess( + dot_source, + source_name, + current_dir, + file_resolver, + custom_transforms, + template_context, + goal_override, + render_mode, + None, + )?; + Ok(pipeline::validate(transformed, &[])) +} + +#[expect( + clippy::too_many_arguments, + reason = "pipeline stages have distinct source, rendering, and model-resolution inputs" +)] +fn preprocess( + dot_source: &str, + source_name: Option, + current_dir: Option, + file_resolver: Option>, + custom_transforms: Vec>, + template_context: TemplateContext, + goal_override: Option<&str>, + render_mode: RenderMode, + model_resolution: Option, +) -> Result { let mut parsed = pipeline::parse(dot_source)?; apply_goal_override(&mut parsed.graph, goal_override); @@ -350,12 +415,9 @@ pub(super) fn preprocess_and_validate( source_name, render_mode, custom_transforms, - catalog: Arc::clone(catalog), - default_provider, - eligible_providers: eligible_providers.iter().cloned().collect(), - catalog_fallback, + model_resolution, })?; - Ok(pipeline::validate(transformed, catalog.as_ref(), &[])) + Ok(transformed) } pub(super) fn template_context( @@ -462,7 +524,7 @@ mod tests { use object_store::memory::InMemory; use super::*; - use crate::operations::{ValidateInput, validate}; + use crate::operations::{ValidateInput, validate, validate_with_catalog}; use crate::pipeline::types::{GOAL_SELF_REFERENCE_RULE, TEMPLATE_UNDEFINED_VARIABLE_RULE}; use crate::workflow_bundle::BundledWorkflow; fn memory_store() -> Arc { @@ -553,17 +615,19 @@ reasoning = false } fn validate_dot(dot_source: &str, settings: WorkflowSettings) -> Validated { - validate(ValidateInput { - workflow: WorkflowInput::DotSource { - source: dot_source.to_string(), - base_dir: None, + validate_with_catalog( + ValidateInput { + workflow: WorkflowInput::DotSource { + source: dot_source.to_string(), + base_dir: None, + }, + settings, + vars: HashMap::new(), + cwd: PathBuf::from("."), + custom_transforms: Vec::new(), }, - settings, - vars: HashMap::new(), - cwd: PathBuf::from("."), - custom_transforms: Vec::new(), - catalog: test_catalog(), - }) + &test_catalog(), + ) .unwrap() } @@ -852,7 +916,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }); assert!(result.is_err()); @@ -901,7 +964,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); let file_missing = validate(ValidateInput { @@ -920,7 +982,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); assert_eq!( @@ -944,7 +1005,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); let file_goal = validate(ValidateInput { @@ -963,7 +1023,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); assert_eq!( @@ -1055,7 +1114,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }); assert!(result.is_err()); } @@ -1100,7 +1158,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: vec![Box::new(TagTransform)], - catalog: test_catalog(), }) .unwrap(); validated.raise_on_errors().unwrap(); @@ -1135,7 +1192,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); validated.raise_on_errors().unwrap(); @@ -1177,7 +1233,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); @@ -1227,7 +1282,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); @@ -1278,7 +1332,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); diff --git a/lib/components/fabro-workflow/src/operations/mod.rs b/lib/components/fabro-workflow/src/operations/mod.rs index 38b7d9aaa..9523c5ff5 100644 --- a/lib/components/fabro-workflow/src/operations/mod.rs +++ b/lib/components/fabro-workflow/src/operations/mod.rs @@ -22,7 +22,7 @@ pub use rewind::{RewindInput, RewindOutcome, rewind}; pub use source::WorkflowInput; pub use start::{StartServices, Started, start}; pub use timeline::{ForkTarget, RunTimeline, TimelineEntry, build_timeline, timeline}; -pub use validate::{ValidateInput, validate, validate_with_ready_providers}; +pub use validate::{ValidateInput, validate, validate_with_catalog, validate_with_ready_providers}; pub use crate::pipeline::{LlmSpec, SandboxEnvSpec}; pub use crate::transforms::RenderMode; diff --git a/lib/components/fabro-workflow/src/operations/validate.rs b/lib/components/fabro-workflow/src/operations/validate.rs index c2b990f5c..7ac5e4531 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -5,7 +5,9 @@ use std::sync::Arc; use fabro_model::{Catalog, ProviderId}; use fabro_types::WorkflowSettings; -use super::create::{preprocess_and_validate, template_context}; +use super::create::{ + preprocess_and_validate, preprocess_and_validate_structural, template_context, +}; use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; use crate::error::Error; use crate::operations::RenderMode; @@ -20,20 +22,50 @@ pub struct ValidateInput { pub vars: HashMap, pub cwd: PathBuf, pub custom_transforms: Vec>, - pub catalog: Arc, } -/// Parse, transform, and validate a DOT source string. +/// Parse, transform, and structurally validate a DOT source string without a +/// model catalog. /// /// Returns `Validated` even when validation produced errors. Call /// `validated.raise_on_errors()` if the caller wants to fail fast. pub fn validate(input: ValidateInput) -> Result { - let eligible_providers = input - .catalog - .all_provider_ids() - .into_iter() - .collect::>(); - validate_with_eligible_providers(input, &eligible_providers, false) + let ValidateInput { + workflow, + settings, + vars, + cwd, + custom_transforms, + } = input; + let resolved = resolve_workflow(ResolveWorkflowInput { + workflow, + settings, + cwd, + }) + .map_err(|err| Error::Parse(err.to_string()))?; + + preprocess_and_validate_structural( + &resolved.raw_source, + resolved + .dot_path + .as_ref() + .map(|path| path.display().to_string()), + resolved.current_dir, + resolved.file_resolver, + custom_transforms, + template_context(Some(&resolved.settings), vars), + resolved.goal_override.as_deref(), + RenderMode::Structural, + ) +} + +/// Parse, transform, and validate a DOT source string against `catalog`. +pub fn validate_with_catalog( + input: ValidateInput, + catalog: &Arc, +) -> Result { + let eligible_providers = catalog.all_provider_ids().into_iter().collect::>(); + validate_with_eligible_providers(input, catalog, &eligible_providers, false) } /// Parse, transform, and validate, resolving models against the ready @@ -41,20 +73,29 @@ pub fn validate(input: ValidateInput) -> Result { /// provider-readiness selection failures. pub fn validate_with_ready_providers( input: ValidateInput, + catalog: &Arc, ready_providers: &[ProviderId], ) -> Result { - validate_with_eligible_providers(input, ready_providers, true) + validate_with_eligible_providers(input, catalog, ready_providers, true) } fn validate_with_eligible_providers( input: ValidateInput, + catalog: &Arc, eligible_providers: &[ProviderId], catalog_fallback: bool, ) -> Result { + let ValidateInput { + workflow, + settings, + vars, + cwd, + custom_transforms, + } = input; let resolved = resolve_workflow(ResolveWorkflowInput { - workflow: input.workflow, - settings: input.settings, - cwd: input.cwd, + workflow, + settings, + cwd, }) .map_err(|err| Error::Parse(err.to_string()))?; @@ -66,8 +107,8 @@ fn validate_with_eligible_providers( .map(|path| path.display().to_string()), resolved.current_dir, resolved.file_resolver, - input.custom_transforms, - template_context(Some(&resolved.settings), input.vars), + custom_transforms, + template_context(Some(&resolved.settings), vars), resolved.goal_override.as_deref(), RenderMode::Structural, resolved @@ -80,6 +121,6 @@ fn validate_with_eligible_providers( .map(fabro_model::ProviderId::new), eligible_providers, catalog_fallback, - &input.catalog, + catalog, ) } diff --git a/lib/components/fabro-workflow/src/pipeline/mod.rs b/lib/components/fabro-workflow/src/pipeline/mod.rs index d0ba5ae1b..c1cc3b4f9 100644 --- a/lib/components/fabro-workflow/src/pipeline/mod.rs +++ b/lib/components/fabro-workflow/src/pipeline/mod.rs @@ -22,8 +22,8 @@ pub use pull_request::{ }; pub use transform::transform; pub use types::{ - Concluded, Executed, FinalizeOptions, Finalized, InitOptions, Initialized, LlmSpec, Parsed, - Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, TEMPLATE_UNDEFINED_VARIABLE_RULE, - TransformOptions, Transformed, Validated, + Concluded, Executed, FinalizeOptions, Finalized, InitOptions, Initialized, LlmSpec, + ModelResolutionOptions, Parsed, Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, + TEMPLATE_UNDEFINED_VARIABLE_RULE, TransformOptions, Transformed, Validated, }; -pub use validate::validate; +pub use validate::{validate, validate_with_catalog}; diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index b399d6637..d73b26275 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -63,13 +63,17 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result TransformOptions { TransformOptions { - current_dir: None, - file_resolver: None, - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + current_dir: None, + file_resolver: None, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), } } @@ -177,16 +180,13 @@ mod tests { ) .unwrap(); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), }) .unwrap(); @@ -227,21 +227,18 @@ mod tests { ) .unwrap(); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), - template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from( - [( + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from([ + ( "task".to_string(), toml::Value::String("Launch".to_string()), - )], - )), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + ), + ])), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), }) .unwrap(); @@ -349,6 +346,38 @@ mod tests { ); } + #[test] + fn structural_transform_preserves_catalog_owned_model_selection() { + let dot = r#"digraph Test { + graph [goal="Test"] + start [shape=Mdiamond] + work [prompt="Do work", model="private-model", provider="server-only"] + exit [shape=Msquare] + start -> work -> exit + }"#; + let parsed = parse(dot).unwrap(); + let transformed = transform(parsed, &TransformOptions { + current_dir: None, + file_resolver: None, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: None, + }) + .unwrap(); + let work = &transformed.graph.nodes["work"]; + + assert_eq!( + work.attrs.get("model").and_then(AttrValue::as_str), + Some("private-model") + ); + assert_eq!( + work.attrs.get("provider").and_then(AttrValue::as_str), + Some("server-only") + ); + } + #[test] fn transform_reports_goal_self_reference_once_across_passes() { // FileInlining renders the goal for prompt context, but TemplateTransform @@ -365,16 +394,13 @@ mod tests { ) .unwrap(); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Structural, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Structural, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), }) .unwrap(); diff --git a/lib/components/fabro-workflow/src/pipeline/types.rs b/lib/components/fabro-workflow/src/pipeline/types.rs index 425cba9aa..29f5fbf9e 100644 --- a/lib/components/fabro-workflow/src/pipeline/types.rs +++ b/lib/components/fabro-workflow/src/pipeline/types.rs @@ -359,13 +359,20 @@ pub struct Finalized { /// Options for the TRANSFORM phase. pub struct TransformOptions { - pub current_dir: Option, - pub file_resolver: Option>, - pub template_context: TemplateContext, - pub source_name: Option, - pub render_mode: RenderMode, - pub custom_transforms: Vec>, - pub catalog: Arc, + pub current_dir: Option, + pub file_resolver: Option>, + pub template_context: TemplateContext, + pub source_name: Option, + pub render_mode: RenderMode, + pub custom_transforms: Vec>, + /// Catalog-backed model resolution to perform. `None` preserves authored + /// model and provider selectors for catalog-free structural validation. + pub model_resolution: Option, +} + +/// Catalog-backed model resolution options for the TRANSFORM phase. +pub struct ModelResolutionOptions { + pub catalog: Arc, pub default_provider: Option, pub eligible_providers: HashSet, /// Fall back to the full catalog when the eligible providers cannot @@ -373,6 +380,19 @@ pub struct TransformOptions { pub catalog_fallback: bool, } +impl ModelResolutionOptions { + #[must_use] + pub fn new(catalog: Arc) -> Self { + let eligible_providers = catalog.all_provider_ids(); + Self { + catalog, + default_provider: None, + eligible_providers, + catalog_fallback: false, + } + } +} + /// Options for the FINALIZE phase. pub struct FinalizeOptions { pub run_dir: PathBuf, diff --git a/lib/components/fabro-workflow/src/pipeline/validate.rs b/lib/components/fabro-workflow/src/pipeline/validate.rs index f0cd51dbd..00601b4a4 100644 --- a/lib/components/fabro-workflow/src/pipeline/validate.rs +++ b/lib/components/fabro-workflow/src/pipeline/validate.rs @@ -1,15 +1,28 @@ -use fabro_model::Catalog; use fabro_validate::LintRule; use super::types::{Transformed, Validated}; -/// VALIDATE phase: run lint rules against the transformed graph. +/// VALIDATE phase: run catalog-free lint rules against the transformed graph. /// /// **Infallible.** Always returns `Validated` with diagnostics. Caller decides /// whether to fail via `validated.raise_on_errors()`. -pub fn validate( +pub fn validate(transformed: Transformed, extra_rules: &[&dyn LintRule]) -> Validated { + let Transformed { + graph, + source, + mut diagnostics, + } = transformed; + diagnostics.extend(fabro_validate::validate(&graph, extra_rules)); + Validated::new(graph, source, diagnostics) +} + +/// VALIDATE phase: run catalog-free and catalog-backed lint rules. +/// +/// **Infallible.** Always returns `Validated` with diagnostics. Caller decides +/// whether to fail via `validated.raise_on_errors()`. +pub fn validate_with_catalog( transformed: Transformed, - catalog: &Catalog, + catalog: &fabro_model::Catalog, extra_rules: &[&dyn LintRule], ) -> Validated { let Transformed { @@ -27,34 +40,24 @@ pub fn validate( #[cfg(test)] mod tests { - use fabro_model::Catalog; - use super::*; use crate::pipeline::parse::parse; use crate::pipeline::transform; use crate::pipeline::types::TransformOptions; - fn test_catalog() -> std::sync::Arc { - std::sync::Arc::new(Catalog::from_builtin().unwrap()) - } - fn run_pipeline(dot: &str) -> Validated { - let catalog = test_catalog(); let parsed = parse(dot).unwrap(); let transformed = transform::transform(parsed, &TransformOptions { - current_dir: None, - file_resolver: None, - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: std::sync::Arc::clone(&catalog), - default_provider: None, - eligible_providers: catalog.all_provider_ids(), - catalog_fallback: false, + current_dir: None, + file_resolver: None, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: None, }) .unwrap(); - validate(transformed, catalog.as_ref(), &[]) + validate(transformed, &[]) } #[test] diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index dc13bcbf8..b07f600fe 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -4853,7 +4853,9 @@ async fn manager_loop_child_workflow_e2e() { #[tokio::test] async fn import_e2e_through_engine() { - use fabro_workflow::pipeline::{TransformOptions, transform, validate}; + use fabro_workflow::pipeline::{ + ModelResolutionOptions, TransformOptions, transform, validate_with_catalog, + }; let dir = tempfile::tempdir().unwrap(); let catalog = std::sync::Arc::new( @@ -4897,21 +4899,18 @@ async fn import_e2e_through_engine() { ) .expect("parse should succeed"); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(std::sync::Arc::new( + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(std::sync::Arc::new( fabro_workflow::file_resolver::FilesystemFileResolver::new(None), )), - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: fabro_workflow::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: std::sync::Arc::clone(&catalog), - default_provider: None, - eligible_providers: catalog.all_provider_ids(), - catalog_fallback: false, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: fabro_workflow::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(std::sync::Arc::clone(&catalog))), }) .unwrap(); - let validated = validate(transformed, catalog.as_ref(), &[]); + let validated = validate_with_catalog(transformed, catalog.as_ref(), &[]); validated .raise_on_errors() .expect("validation should pass after imports expand"); From 59b1c2e59ffca7cf5f491c4fe65e7650afb48a28 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 13:53:54 -0400 Subject: [PATCH 04/36] Reject nodes referenced by an edge but never declared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DOT parser created a node for every edge endpoint, and nothing recorded whether a node came from a declaration or was synthesized from an edge. The edge_target_exists rule only checked whether the node id was present in the graph, which was always true by then, so a misspelled endpoint became an attribute-free node that defaulted to shape=box — an LLM stage. Validation emitted a prompt_on_llm_nodes warning and exited 0. Node now carries `implicit`, set only when the parser synthesizes the node from an edge endpoint. A declaration anywhere in the workflow clears it, so order does not matter and subgraph declarations count. Node::new leaves it false, so programmatic construction and graphs deserialized from older checkpoints read as declared. edge_target_exists treats an endpoint as valid only when it exists and is declared, reporting each undeclared node once. The near-identical missing-source and missing-target branches collapse into one path. The import transform copies the flag onto spliced nodes so an edge-only node inside an imported fragment is caught too. parse_and_validate_human_gate had two edge-only nodes and now declares them; it was an instance of the bug rather than a casualty of the fix. No shipped workflow, docs example, or CLI fixture relied on the old behavior. Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/reference/dot-language.mdx | 2 +- lib/apps/fabro-cli/tests/it/cmd/validate.rs | 20 +++ .../fabro-graphviz/src/parser/semantic.rs | 93 +++++++++- .../src/rules/edge_target_exists.rs | 168 ++++++++++++++---- .../fabro-workflow/src/transforms/import.rs | 49 +++++ .../fabro-workflow/tests/it/integration.rs | 3 + lib/foundation/fabro-types/src/graph.rs | 18 +- .../fabro-types/src/run_event/mod.rs | 7 +- test/edge_only_node.fabro | 10 ++ 9 files changed, 326 insertions(+), 44 deletions(-) create mode 100644 test/edge_only_node.fabro diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 8c350d1ba..ee44c3668 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -113,7 +113,7 @@ plan [label="Plan", prompt="Create an implementation plan."] **Node identifiers** must start with a letter or underscore, followed by letters, digits, or underscores (e.g. `run_tests`, `gate_1`, `_private`). -Nodes referenced in edges are auto-created if not explicitly declared. +Every node used by an edge needs its own declaration. Validation fails when an edge names a node the workflow never declares, because that is nearly always a typo or a rename that missed an edge. The declaration can come before or after the edges that use it, and it can live in a subgraph. ### Edge declarations diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 190c2817b..98b0f9cfe 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -278,6 +278,26 @@ fn validate_reports_missing_template_dependency() { "); } +/// A node named only by an edge is almost always a typo, so validation must +/// fail instead of quietly running it as a default agent stage. +#[test] +fn edge_only_node() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("edge_only_node.fabro")); + fabro_snapshot!(context.filters(), cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + Workflow: EdgeOnlyNode (3 nodes, 2 edges) + Graph: [FIXTURES]/edge_only_node.fabro + error [node: misspelled_node]: Node 'misspelled_node' is referenced by edge 'start -> misspelled_node' but has no node declaration (edge_target_exists) + warning [node: misspelled_node]: LLM node 'misspelled_node' has no prompt or label attribute (prompt_on_llm_nodes) + × Validation failed + "); +} + #[test] fn invalid() { let context = test_context!(); diff --git a/lib/components/fabro-graphviz/src/parser/semantic.rs b/lib/components/fabro-graphviz/src/parser/semantic.rs index 8ac364cab..4e9433e6b 100644 --- a/lib/components/fabro-graphviz/src/parser/semantic.rs +++ b/lib/components/fabro-graphviz/src/parser/semantic.rs @@ -57,6 +57,14 @@ fn derive_class_from_label(label: &str) -> String { .collect() } +/// How a statement named a node. A node stays implicit only while every +/// mention of it is an edge endpoint, so declaration order does not matter. +#[derive(Clone, Copy, PartialEq, Eq)] +enum Mention { + Declaration, + EdgeEndpoint, +} + struct SemanticState { graph: Graph, node_defaults: HashMap, @@ -72,13 +80,25 @@ impl SemanticState { } } - fn ensure_node(&mut self, id: &str) { + /// Insert the node if this is the first statement to mention it, and record + /// whether the workflow ever declares it. + fn ensure_node(&mut self, id: &str, mention: Mention) { if !self.graph.nodes.contains_key(id) { let mut node = Node::new(id); for (k, v) in &self.node_defaults { node.attrs.insert(k.clone(), v.clone()); } + node.implicit = mention == Mention::EdgeEndpoint; self.graph.nodes.insert(id.to_string(), node); + return; + } + if mention == Mention::Declaration { + let node = self + .graph + .nodes + .get_mut(id) + .expect("contains_key returned true, so get_mut cannot return None"); + node.implicit = false; } } @@ -90,7 +110,7 @@ impl SemanticState { } fn process_node(&mut self, node_stmt: &NodeStmt, subgraph_class: Option<&str>) { - self.ensure_node(&node_stmt.id); + self.ensure_node(&node_stmt.id, Mention::Declaration); let node = self .graph .nodes @@ -127,7 +147,7 @@ impl SemanticState { fn process_edge(&mut self, edge_stmt: &EdgeStmt, subgraph_class: Option<&str>) { for id in &edge_stmt.nodes { - self.ensure_node(id); + self.ensure_node(id, Mention::EdgeEndpoint); if let Some(cls) = subgraph_class { let node = self.graph.nodes.get_mut(id).expect( @@ -527,5 +547,72 @@ mod tests { let graph = ast_to_graph(&dot).unwrap(); assert!(graph.nodes.contains_key("a")); assert!(graph.nodes.contains_key("b")); + assert!(graph.nodes["a"].implicit); + assert!(graph.nodes["b"].implicit); + } + + #[test] + fn ast_to_graph_marks_declared_nodes_explicit() { + let dot = DotGraph { + name: "Declared".into(), + statements: vec![ + Statement::Node(NodeStmt { + id: "a".into(), + attrs: None, + }), + Statement::Edge(EdgeStmt { + nodes: vec!["a".into(), "b".into()], + attrs: None, + }), + ], + }; + + let graph = ast_to_graph(&dot).unwrap(); + assert!(!graph.nodes["a"].implicit); + assert!(graph.nodes["b"].implicit); + } + + #[test] + fn ast_to_graph_declaration_after_edge_still_counts() { + let dot = DotGraph { + name: "DeclaredLater".into(), + statements: vec![ + Statement::Edge(EdgeStmt { + nodes: vec!["a".into(), "b".into()], + attrs: None, + }), + Statement::Node(NodeStmt { + id: "b".into(), + attrs: Some(vec![("prompt".into(), AstValue::Str("Do it".into()))]), + }), + ], + }; + + let graph = ast_to_graph(&dot).unwrap(); + assert!(!graph.nodes["b"].implicit); + } + + #[test] + fn ast_to_graph_subgraph_declaration_counts() { + let dot = DotGraph { + name: "SubgraphDeclared".into(), + statements: vec![ + Statement::Edge(EdgeStmt { + nodes: vec!["start".into(), "plan".into()], + attrs: None, + }), + Statement::Subgraph(SubgraphStmt { + name: Some("cluster_loop".into()), + statements: vec![Statement::Node(NodeStmt { + id: "plan".into(), + attrs: None, + })], + }), + ], + }; + + let graph = ast_to_graph(&dot).unwrap(); + assert!(!graph.nodes["plan"].implicit); + assert!(graph.nodes["start"].implicit); } } diff --git a/lib/components/fabro-validate/src/rules/edge_target_exists.rs b/lib/components/fabro-validate/src/rules/edge_target_exists.rs index 8cfe67282..adc70283b 100644 --- a/lib/components/fabro-validate/src/rules/edge_target_exists.rs +++ b/lib/components/fabro-validate/src/rules/edge_target_exists.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use fabro_graphviz::graph::Graph; use crate::{Diagnostic, LintRule, Severity}; @@ -8,6 +10,33 @@ pub(super) fn rule() -> Box { struct Rule; +impl Rule { + /// An edge endpoint is only usable when the workflow declares it. A node + /// the parser synthesized from the edge itself carries no attributes, so it + /// would silently run as a default agent stage. + fn is_declared(graph: &Graph, node_id: &str) -> bool { + graph.nodes.get(node_id).is_some_and(|node| !node.implicit) + } + + fn diagnostic(&self, node_id: &str, from: &str, to: &str) -> Diagnostic { + Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: format!( + "Node '{node_id}' is referenced by edge '{from} -> {to}' but has no node \ + declaration" + ), + node_id: Some(node_id.to_string()), + edge: Some((from.to_string(), to.to_string())), + fix: Some(format!( + "Declare node '{node_id}' or correct the edge endpoint" + )), + + ..Diagnostic::default() + } + } +} + impl LintRule for Rule { fn name(&self) -> &'static str { "edge_target_exists" @@ -15,36 +44,12 @@ impl LintRule for Rule { fn apply(&self, graph: &Graph) -> Vec { let mut diagnostics = Vec::new(); + let mut reported = HashSet::new(); for edge in &graph.edges { - if !graph.nodes.contains_key(&edge.to) { - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message: format!( - "Edge from '{}' targets non-existent node '{}'", - edge.from, edge.to - ), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(format!("Define node '{}' or fix the edge target", edge.to)), - - ..Diagnostic::default() - }); - } - if !graph.nodes.contains_key(&edge.from) { - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message: format!("Edge source '{}' references non-existent node", edge.from), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(format!( - "Define node '{}' or fix the edge source", - edge.from - )), - - ..Diagnostic::default() - }); + for endpoint in [&edge.to, &edge.from] { + if !Self::is_declared(graph, endpoint) && reported.insert(endpoint) { + diagnostics.push(self.diagnostic(endpoint, &edge.from, &edge.to)); + } } } diagnostics @@ -53,11 +58,112 @@ impl LintRule for Rule { #[cfg(test)] mod tests { - use fabro_graphviz::graph::Edge; + use fabro_graphviz::graph::{Edge, Graph}; + use fabro_graphviz::parser; use super::Rule; use crate::rules::test_support::minimal_graph; - use crate::{LintRule, Severity}; + use crate::{Diagnostic, LintRule, Severity}; + + fn parse(dot: &str) -> Graph { + parser::parse(dot).expect("fixture should parse") + } + + fn undeclared_nodes(graph: &Graph) -> Vec { + Rule.apply(graph) + .iter() + .map(|d| d.node_id.clone().expect("diagnostic should name a node")) + .collect() + } + + #[test] + fn edge_only_node_is_rejected() { + let graph = parse( + r"digraph EdgeOnly { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> misspelled_node + misspelled_node -> exit + }", + ); + + let diagnostics = Rule.apply(&graph); + assert_eq!(diagnostics.len(), 1, "diagnostics: {diagnostics:?}"); + let Diagnostic { + severity, + node_id, + edge, + .. + } = &diagnostics[0]; + assert_eq!(*severity, Severity::Error); + assert_eq!(node_id.as_deref(), Some("misspelled_node")); + assert_eq!( + edge.clone(), + Some(("start".to_string(), "misspelled_node".to_string())) + ); + } + + #[test] + fn declaration_after_the_edge_is_accepted() { + let graph = parse( + r#"digraph DeclaredLater { + start -> work + work [prompt="Do the work"] + work -> exit + start [shape=Mdiamond] + exit [shape=Msquare] + }"#, + ); + + assert!(Rule.apply(&graph).is_empty()); + } + + #[test] + fn chained_edges_report_every_undeclared_endpoint() { + let graph = parse( + r"digraph Chained { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> first -> second -> exit + }", + ); + + assert_eq!(undeclared_nodes(&graph), vec!["first", "second"]); + } + + #[test] + fn a_node_is_reported_once_no_matter_how_many_edges_use_it() { + let graph = parse( + r"digraph Repeated { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> typo + typo -> exit + typo -> start + }", + ); + + assert_eq!(undeclared_nodes(&graph), vec!["typo"]); + } + + #[test] + fn subgraph_declaration_is_accepted() { + let graph = parse( + r#"digraph Subgraphed { + start [shape=Mdiamond] + exit [shape=Msquare] + + subgraph cluster_loop { + label = "Loop A" + plan [prompt="Plan the work"] + } + + start -> plan -> exit + }"#, + ); + + assert!(Rule.apply(&graph).is_empty()); + } #[test] fn edge_target_exists_rule_missing_target() { diff --git a/lib/components/fabro-workflow/src/transforms/import.rs b/lib/components/fabro-workflow/src/transforms/import.rs index b5406ddc6..cfda146c3 100644 --- a/lib/components/fabro-workflow/src/transforms/import.rs +++ b/lib/components/fabro-workflow/src/transforms/import.rs @@ -346,6 +346,7 @@ impl ImportTransform { let prefixed_id = format!("{placeholder_id}.{node_id}"); let mut merged_node = Node::new(&prefixed_id); + merged_node.implicit = node.implicit; merged_node.attrs.clone_from(&placeholder.default_attrs); merged_node.attrs.extend(node.attrs); Self::remap_retry_target(&mut merged_node.attrs, placeholder_id); @@ -951,6 +952,54 @@ mod tests { ); } + #[test] + fn imported_node_declarations_survive_splicing() { + let dir = tempfile::tempdir().unwrap(); + write_file(&dir.path().join("validate.fabro"), basic_import_source()); + + let graph = apply_import( + r#"digraph Deploy { + start [shape=Mdiamond] + validate [import="./validate.fabro"] + exit [shape=Msquare] + start -> validate -> exit + }"#, + dir.path(), + None, + ); + + assert!(!graph.nodes["validate.lint"].implicit); + assert!(!graph.nodes["validate.test"].implicit); + } + + #[test] + fn edge_only_node_in_imported_fragment_stays_undeclared() { + let dir = tempfile::tempdir().unwrap(); + write_file( + &dir.path().join("validate.fabro"), + r#"digraph validate { + start [shape=Mdiamond] + lint [prompt="Run clippy"] + exit [shape=Msquare] + start -> lint -> typo -> exit + }"#, + ); + + let graph = apply_import( + r#"digraph Deploy { + start [shape=Mdiamond] + validate [import="./validate.fabro"] + exit [shape=Msquare] + start -> validate -> exit + }"#, + dir.path(), + None, + ); + + assert!(graph.nodes["validate.typo"].implicit); + assert!(!graph.nodes["validate.lint"].implicit); + } + #[test] fn import_reports_structural_diagnostic_for_imported_prompt_templates() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index dc13bcbf8..ec716adbc 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -388,6 +388,9 @@ fn parse_and_validate_human_gate() { type="human" ] + ship_it [prompt="Ship the change"] + fixes [prompt="Apply the requested fixes"] + start -> review_gate review_gate -> ship_it [label="[A] Approve"] review_gate -> fixes [label="[F] Fix"] diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index 7ef9bad99..3ed6851a6 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -119,20 +119,26 @@ pub fn shape_to_handler_type(shape: &str) -> Option<&'static str> { /// A node in the workflow graph. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Node { - pub id: String, - pub attrs: HashMap, + pub id: String, + pub attrs: HashMap, /// CSS-like classes for model stylesheet targeting (from `class` attr and /// subgraph derivation). #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub classes: Vec, + pub classes: Vec, + /// True when the node was synthesized from an edge endpoint instead of a + /// node declaration. Validation rejects these because an edge-only node in + /// an executable workflow is almost always a typo. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub implicit: bool, } impl Node { pub fn new(id: impl Into) -> Self { Self { - id: id.into(), - attrs: HashMap::new(), - classes: Vec::new(), + id: id.into(), + attrs: HashMap::new(), + classes: Vec::new(), + implicit: false, } } diff --git a/lib/foundation/fabro-types/src/run_event/mod.rs b/lib/foundation/fabro-types/src/run_event/mod.rs index d4c8e55c1..e4e99e59f 100644 --- a/lib/foundation/fabro-types/src/run_event/mod.rs +++ b/lib/foundation/fabro-types/src/run_event/mod.rs @@ -995,9 +995,10 @@ mod tests { let graph = Graph { name: "test".to_string(), nodes: HashMap::from([("start".to_string(), Node { - id: "start".to_string(), - attrs: HashMap::new(), - classes: Vec::new(), + id: "start".to_string(), + attrs: HashMap::new(), + classes: Vec::new(), + implicit: false, })]), edges: vec![Edge { from: "start".to_string(), diff --git a/test/edge_only_node.fabro b/test/edge_only_node.fabro new file mode 100644 index 000000000..3d45b3346 --- /dev/null +++ b/test/edge_only_node.fabro @@ -0,0 +1,10 @@ +digraph EdgeOnlyNode { + graph [goal="Reference a node that was never declared"] + + /* `misspelled_node` is only ever named by an edge, never declared. */ + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + start -> misspelled_node + misspelled_node -> exit +} From c501c67185892cad0714859eedb4676e1552d7a6 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 14:06:31 -0400 Subject: [PATCH 05/36] Show each diagnostic's suggested fix in CLI output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnostics have carried a `fix` field all along, but the CLI renderer never printed it — the suggestion was only reachable through --json. The actionable half of every validation failure was invisible to the person running the command. print_diagnostics now emits the fix as a dim-labelled continuation line under any diagnostic that has one, at both error and warning severity. Gating it behind --verbose would defeat the point, and printing it only for errors would read as "this warning has no fix" — the warning suggestions are useful on their own. Diagnostics that set no fix simply omit the line. The severity match moved into print_diagnostic so the fix line is appended once in the loop rather than copied into all five arms; the rest of the diff is reindentation. print_diagnostics is shared by validate, preflight, graph, exec, and dry-run, so this covers all five. Eleven inline snapshots across four files gain a fix line; every change is additive. Co-Authored-By: Claude Opus 5 (1M context) --- lib/apps/fabro-cli/src/shared/utilities.rs | 98 ++++++++++--------- lib/apps/fabro-cli/tests/it/cmd/graph.rs | 4 + lib/apps/fabro-cli/tests/it/cmd/preflight.rs | 4 + lib/apps/fabro-cli/tests/it/cmd/validate.rs | 9 ++ .../tests/it/workflow/dry_run_examples.rs | 1 + 5 files changed, 72 insertions(+), 44 deletions(-) diff --git a/lib/apps/fabro-cli/src/shared/utilities.rs b/lib/apps/fabro-cli/src/shared/utilities.rs index e9cd32bf6..8cfb05af7 100644 --- a/lib/apps/fabro-cli/src/shared/utilities.rs +++ b/lib/apps/fabro-cli/src/shared/utilities.rs @@ -49,54 +49,64 @@ where pub(crate) fn print_diagnostics(diagnostics: &[Diagnostic], styles: &Styles, printer: Printer) { for d in diagnostics { - let location = match (&d.node_id, &d.edge) { - (Some(node), _) => format!(" [node: {node}]"), - (_, Some((from, to))) => format!(" [edge: {from} -> {to}]"), - _ => String::new(), - }; - let source_prefix = source_prefix(d); - match d.severity { - Severity::Error if source_prefix.is_empty() => fabro_util::printerr!( - printer, - "{}{location}: {} ({})", - styles.red.apply_to("error"), - d.message, - styles.dim.apply_to(&d.rule), - ), - Severity::Error => fabro_util::printerr!( - printer, - "{}: {source_prefix}{}{location} ({})", - styles.red.apply_to("error"), - d.message, - styles.dim.apply_to(&d.rule), - ), - Severity::Warning if source_prefix.is_empty() => fabro_util::printerr!( - printer, - "{}{location}: {} ({})", - styles.yellow.apply_to("warning"), - d.message, - styles.dim.apply_to(&d.rule), - ), - Severity::Warning => fabro_util::printerr!( - printer, - "{}: {source_prefix}{}{location} ({})", - styles.yellow.apply_to("warning"), - d.message, - styles.dim.apply_to(&d.rule), - ), - Severity::Info => fabro_util::printerr!( - printer, - "{}", - styles.dim.apply_to(if source_prefix.is_empty() { - format!("info{location}: {} ({})", d.message, d.rule) - } else { - format!("info: {source_prefix}{}{location} ({})", d.message, d.rule) - }), - ), + print_diagnostic(d, styles, printer); + // The fix is the actionable half of a diagnostic, so it follows every + // severity rather than hiding behind --verbose. Rules that have nothing + // useful to suggest leave it unset. + if let Some(fix) = &d.fix { + fabro_util::printerr!(printer, " {} {fix}", styles.dim.apply_to("fix:")); } } } +fn print_diagnostic(d: &Diagnostic, styles: &Styles, printer: Printer) { + let location = match (&d.node_id, &d.edge) { + (Some(node), _) => format!(" [node: {node}]"), + (_, Some((from, to))) => format!(" [edge: {from} -> {to}]"), + _ => String::new(), + }; + let source_prefix = source_prefix(d); + match d.severity { + Severity::Error if source_prefix.is_empty() => fabro_util::printerr!( + printer, + "{}{location}: {} ({})", + styles.red.apply_to("error"), + d.message, + styles.dim.apply_to(&d.rule), + ), + Severity::Error => fabro_util::printerr!( + printer, + "{}: {source_prefix}{}{location} ({})", + styles.red.apply_to("error"), + d.message, + styles.dim.apply_to(&d.rule), + ), + Severity::Warning if source_prefix.is_empty() => fabro_util::printerr!( + printer, + "{}{location}: {} ({})", + styles.yellow.apply_to("warning"), + d.message, + styles.dim.apply_to(&d.rule), + ), + Severity::Warning => fabro_util::printerr!( + printer, + "{}: {source_prefix}{}{location} ({})", + styles.yellow.apply_to("warning"), + d.message, + styles.dim.apply_to(&d.rule), + ), + Severity::Info => fabro_util::printerr!( + printer, + "{}", + styles.dim.apply_to(if source_prefix.is_empty() { + format!("info{location}: {} ({})", d.message, d.rule) + } else { + format!("info: {source_prefix}{}{location} ({})", d.message, d.rule) + }), + ), + } +} + fn source_prefix(diagnostic: &Diagnostic) -> String { match ( diagnostic.source_path.as_deref(), diff --git a/lib/apps/fabro-cli/tests/it/cmd/graph.rs b/lib/apps/fabro-cli/tests/it/cmd/graph.rs index 6b801928a..fbf48599f 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/graph.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/graph.rs @@ -95,7 +95,9 @@ fn graph_allow_invalid_renders_after_diagnostics() { ----- stdout ----- ----- stderr ----- error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node "); let svg = read_text(&output_path); @@ -119,7 +121,9 @@ fn graph_invalid_workflow_fails_after_diagnostics() { ----- stdout ----- ----- stderr ----- error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node × Validation failed "); } diff --git a/lib/apps/fabro-cli/tests/it/cmd/preflight.rs b/lib/apps/fabro-cli/tests/it/cmd/preflight.rs index 8bb035f3a..4a2e44c8a 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/preflight.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/preflight.rs @@ -52,7 +52,9 @@ fn preflight_invalid_workflow_fails_with_validation_output() { Workflow: Invalid (2 nodes, 1 edges) Graph: [FIXTURES]/invalid.fabro error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node × Validation failed "); } @@ -74,7 +76,9 @@ fn preflight_rejects_unbound_template_inputs() { Goal: Demo error: [FIXTURES]/templated_unbound.fabro:2:26: undefined template variable `inputs.app_dir` in graph attribute `goal` (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` error: [FIXTURES]/templated_unbound.fabro:7:44: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` × Validation failed "); } diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 98b0f9cfe..caeab11ca 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -82,6 +82,7 @@ fn branching() { Workflow: Branch (6 nodes, 6 edges) Graph: [FIXTURES]/branching.fabro warning [node: implement]: Node 'implement' has goal_gate=true but no retry_target or fallback_retry_target (goal_gate_has_retry) + fix: Add retry_target or fallback_retry_target attribute Validation: OK "); } @@ -163,7 +164,9 @@ fn bare_fabro_with_unbound_inputs_validates_structurally_with_warning() { Workflow: TemplatedUnbound (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound.fabro warning: [FIXTURES]/templated_unbound.fabro:2:26: undefined template variable `inputs.app_dir` in graph attribute `goal` (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` warning: [FIXTURES]/templated_unbound.fabro:7:44: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` Validation: OK "); } @@ -186,6 +189,7 @@ fn bare_fabro_with_unbound_inputs_in_imported_prompt_validates_structurally_with Workflow: TemplatedUnboundImported (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound_imported/workflow.fabro warning: [FIXTURES]/templated_unbound_imported/work.md:1:12: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` Validation: OK "); } @@ -207,6 +211,7 @@ fn bare_fabro_with_unbound_inputs_in_template_partial_validates_structurally_wit Workflow: TemplatedUnboundPartial (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound_partial/workflow.fabro warning: [FIXTURES]/templated_unbound_partial/test-include.partial.md:1:4: undefined template variable `inputs.hello` in node `test_imported_include` attribute `prompt` [node: test_imported_include] (template_undefined_variable) + fix: bind `inputs.hello` via `[run.inputs]` in workflow.toml, or pass `--input inputs.hello=` Validation: OK "); } @@ -293,7 +298,9 @@ fn edge_only_node() { Workflow: EdgeOnlyNode (3 nodes, 2 edges) Graph: [FIXTURES]/edge_only_node.fabro error [node: misspelled_node]: Node 'misspelled_node' is referenced by edge 'start -> misspelled_node' but has no node declaration (edge_target_exists) + fix: Declare node 'misspelled_node' or correct the edge endpoint warning [node: misspelled_node]: LLM node 'misspelled_node' has no prompt or label attribute (prompt_on_llm_nodes) + fix: Add a prompt or label attribute × Validation failed "); } @@ -311,7 +318,9 @@ fn invalid() { Workflow: Invalid (2 nodes, 1 edges) Graph: [FIXTURES]/invalid.fabro error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node × Validation failed "); } diff --git a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs index 658dd1fea..d6852765e 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs @@ -19,6 +19,7 @@ fn dry_run_branching() { Goal: Implement and validate a feature warning [node: implement]: Node 'implement' has goal_gate=true but no retry_target or fallback_retry_target (goal_gate_has_retry) + fix: Add retry_target or fallback_retry_target attribute Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) From 716ba1778069f5f916f34515de79a1f0c417511e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 15:19:57 -0400 Subject: [PATCH 06/36] Show billed amount in runs list size tooltip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Size column in the runs list rendered SizeChip without the billed total, so its tooltip read "Size M" while the run detail header showed "Size M · $12.34 billed". The tooltip was also unreachable: the row title link paints a `before:absolute before:inset-0` overlay across the whole row, which sat above the chip and swallowed hover. Wrapping the chip in `relative z-10` lifts it above that overlay, matching how the created-by and pull request cells already handle interactive content. Runs without terminal billing keep the plain "Size M" label, same as the header. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/components/runs-list/run-table-row.tsx | 6 +++++- apps/fabro-web/app/data/runs.test.ts | 11 +++++++++++ apps/fabro-web/app/data/runs.ts | 2 ++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/apps/fabro-web/app/components/runs-list/run-table-row.tsx b/apps/fabro-web/app/components/runs-list/run-table-row.tsx index 850c36467..e7447bce5 100644 --- a/apps/fabro-web/app/components/runs-list/run-table-row.tsx +++ b/apps/fabro-web/app/components/runs-list/run-table-row.tsx @@ -116,7 +116,11 @@ export function RunTableRow({ )} {show("size") && ( - {run.size != null && } + {run.size != null && ( + + + + )} )} {show("changes") && ( diff --git a/apps/fabro-web/app/data/runs.test.ts b/apps/fabro-web/app/data/runs.test.ts index 0ad593526..77fdc91e2 100644 --- a/apps/fabro-web/app/data/runs.test.ts +++ b/apps/fabro-web/app/data/runs.test.ts @@ -103,6 +103,17 @@ describe("mapRunListItem", () => { expect(mapRunListItem(summary).title).toBe("Untitled run"); }); + + test("carries the billed total so the size chip can show it on hover", () => { + expect(mapRunListItem(makeRun()).totalUsdMicros).toBe(500000); + }); + + test("leaves the billed total undefined for runs without terminal billing", () => { + expect(mapRunListItem(makeRun({ billing: null })).totalUsdMicros).toBeUndefined(); + expect( + mapRunListItem(makeRun({ billing: { total_usd_micros: null } })).totalUsdMicros, + ).toBeUndefined(); + }); }); describe("mapRunToRunItem", () => { diff --git a/apps/fabro-web/app/data/runs.ts b/apps/fabro-web/app/data/runs.ts index 4fb704133..c5f29f6ae 100644 --- a/apps/fabro-web/app/data/runs.ts +++ b/apps/fabro-web/app/data/runs.ts @@ -46,6 +46,7 @@ export interface RunItem { createdBy: Principal; lastEventAt?: string; size?: RunSize; + totalUsdMicros?: number; } export const columnStatuses = [ @@ -119,6 +120,7 @@ export function mapRunListItem(item: Run): RunItem { additions: item.diff?.additions, deletions: item.diff?.deletions, size: item.size, + totalUsdMicros: item.billing?.total_usd_micros ?? undefined, }; } From 991f160a0b09f54f931fe27813ccc011779073ec Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 15:23:01 -0400 Subject: [PATCH 07/36] Drop "billed" from the size chip tooltip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tooltip now reads "Size M · $12.34" instead of "Size M · $12.34 billed". Co-Authored-By: Claude Opus 5 (1M context) --- apps/fabro-web/app/components/size-chip.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/fabro-web/app/components/size-chip.tsx b/apps/fabro-web/app/components/size-chip.tsx index e3d07ed40..d23210955 100644 --- a/apps/fabro-web/app/components/size-chip.tsx +++ b/apps/fabro-web/app/components/size-chip.tsx @@ -19,10 +19,10 @@ export function SizeChip({ totalUsdMicros?: number | null; }) { const tone = SIZE_TONE[size]; - const billed = totalUsdMicros != null ? ` · ${formatUsdMicros(totalUsdMicros)} billed` : ""; + const amount = totalUsdMicros != null ? ` · ${formatUsdMicros(totalUsdMicros)}` : ""; const tooltip = tone.note != null - ? `Size ${size} (${tone.note})${billed}` - : `Size ${size}${billed}`; + ? `Size ${size} (${tone.note})${amount}` + : `Size ${size}${amount}`; return ( From 53c580ce5fde89d4ef348bf091da8074c5a66aee Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 15:25:44 -0400 Subject: [PATCH 08/36] Swap the board card's elapsed time for a size chip The board cards showed wall-clock duration in the footer's bottom-right corner. Replace it with the same SizeChip the list view and run detail header use, so the cost signal is consistent across all three views. The chip inherits the tooltip, which names the tier and adds the cost once a run has terminal billing. Add SizeChip tests pinning the tooltip label for each tier. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/components/size-chip.test.tsx | 40 +++++++++++++++++++ apps/fabro-web/app/routes/runs.tsx | 11 ++--- 2 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 apps/fabro-web/app/components/size-chip.test.tsx diff --git a/apps/fabro-web/app/components/size-chip.test.tsx b/apps/fabro-web/app/components/size-chip.test.tsx new file mode 100644 index 000000000..b355f5db1 --- /dev/null +++ b/apps/fabro-web/app/components/size-chip.test.tsx @@ -0,0 +1,40 @@ +import { describe, expect, test } from "bun:test"; +import TestRenderer, { act } from "react-test-renderer"; + +import { SizeChip } from "./size-chip"; +import { Tooltip } from "./ui"; + +function tooltipLabel(element: React.ReactElement): string { + let renderer: TestRenderer.ReactTestRenderer | undefined; + act(() => { + renderer = TestRenderer.create(element); + }); + return renderer!.root.findByType(Tooltip).props.label as string; +} + +describe("SizeChip", () => { + test("renders the size letter", () => { + let renderer: TestRenderer.ReactTestRenderer | undefined; + act(() => { + renderer = TestRenderer.create(); + }); + + expect(JSON.stringify(renderer!.toJSON())).toContain("M"); + }); + + test("appends the cost to the tooltip", () => { + expect(tooltipLabel()) + .toBe("Size M · $12.34"); + }); + + test("omits the cost when the run has no billing yet", () => { + expect(tooltipLabel()).toBe("Size M"); + expect(tooltipLabel()).toBe("Size M"); + }); + + test("calls out the tiers that warrant attention", () => { + expect(tooltipLabel()) + .toBe("Size L (risky) · $150.00"); + expect(tooltipLabel()).toBe("Size XL (unhealthy)"); + }); +}); diff --git a/apps/fabro-web/app/routes/runs.tsx b/apps/fabro-web/app/routes/runs.tsx index 7da0719ea..fe21e36c8 100644 --- a/apps/fabro-web/app/routes/runs.tsx +++ b/apps/fabro-web/app/routes/runs.tsx @@ -26,6 +26,7 @@ import { ciConfig, columnForRun, columnStatusDisplay, columnStatuses, deriveCiSt import type { CiStatus, CheckRun, CheckStatus, RunItem } from "../data/runs"; import { EmptyState } from "../components/state"; import { PullRequestChip } from "../components/pull-request-chip"; +import { SizeChip } from "../components/size-chip"; import { summarizeBatchLifecycleAction, } from "../components/runs-list/batch-lifecycle"; @@ -345,7 +346,7 @@ function PrCard({ // All inline footer metadata on PrCard belongs in this one row. Adding a new // piece as a sibling `
` below the card body recreates a recurring bug -// where stats stack onto separate lines instead of sitting next to elapsed/actions. +// where stats stack onto separate lines instead of sitting next to size/actions. function PrCardFooter({ pr, actions }: { pr: RunItem; actions?: string[] }) { const hasActions = actions != null && actions.length > 0; const hasStats = @@ -354,7 +355,7 @@ function PrCardFooter({ pr, actions }: { pr: RunItem; actions?: string[] }) { (pr.additions != null && pr.additions !== 0) || (pr.deletions != null && pr.deletions !== 0); - if (!hasStats && !hasActions && pr.elapsed == null) return null; + if (!hasStats && !hasActions && pr.size == null) return null; return (
@@ -416,9 +417,9 @@ function PrCardFooter({ pr, actions }: { pr: RunItem; actions?: string[] }) { ))}
)} - {pr.elapsed != null && ( - - {pr.elapsed} + {pr.size != null && ( + + )}
From 7841a77f2c0962439864ebdf8f04ba9441fba7fd Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 16:06:19 -0400 Subject: [PATCH 09/36] feat(web): show stage tokens and cost in the model popover The model indicator on a stage page hovered to provider, model, and reasoning effort only. Seeing what a stage actually spent meant leaving for the Billing tab, which reports per node rather than per visit. The stage list had no token data to show, so add a per-visit `billing` block to `GET /runs/{id}/stages`. The Billing tab's pricing rule (a provider-reported cost wins, otherwise the server catalog prices the tokens) was private to `billing_rollup`; move it to `StageProjection::billed_usage` and drive both call sites from it so the two views cannot drift. The popover's buckets use the Billing tab's labels verbatim. It stays scoped to one visit, so a looped node's row on the Billing tab is the sum of what each of its visits shows here. Co-Authored-By: Claude Opus 5 (1M context) --- apps/fabro-web/app/lib/stage-sidebar.test.ts | 22 +++ apps/fabro-web/app/lib/stage-sidebar.ts | 7 + .../app/routes/run-stages-details.test.tsx | 87 ++++++++++- apps/fabro-web/app/routes/run-stages.tsx | 61 +++++++- docs/public/api-reference/fabro-api.yaml | 10 ++ lib/apps/fabro-server/src/demo/mod.rs | 1 + .../src/server/handler/billing.rs | 6 +- lib/apps/fabro-server/src/server/tests.rs | 141 ++++++++++++++++++ .../fabro-workflow/src/billing_rollup.rs | 29 +--- .../fabro-types/src/run_projection.rs | 86 ++++++++++- .../fabro-api-client/src/models/run-stage.ts | 7 + 11 files changed, 423 insertions(+), 34 deletions(-) diff --git a/apps/fabro-web/app/lib/stage-sidebar.test.ts b/apps/fabro-web/app/lib/stage-sidebar.test.ts index a71323654..e1cc9092b 100644 --- a/apps/fabro-web/app/lib/stage-sidebar.test.ts +++ b/apps/fabro-web/app/lib/stage-sidebar.test.ts @@ -17,6 +17,7 @@ function makeStage(nodeId: string, visit: number, status: StageState): Stage { duration: "--", startedAt: null, providerUsed: null, + billing: null, }; } @@ -38,6 +39,15 @@ describe("mapRunStagesToSidebarStages", () => { model: "gpt-5.5", reasoning_effort: "high", }, + billing: { + input_tokens: 28_640, + output_tokens: 7_550, + total_tokens: 43_690, + reasoning_tokens: 1_200, + cache_read_tokens: 4_800, + cache_write_tokens: 1_500, + total_usd_micros: 720_000, + }, }, { id: "apply-changes@2", @@ -46,6 +56,14 @@ describe("mapRunStagesToSidebarStages", () => { status: "running", node_id: "apply", visit: 2, + billing: { + input_tokens: 0, + output_tokens: 0, + total_tokens: 0, + reasoning_tokens: 0, + cache_read_tokens: 0, + cache_write_tokens: 0, + }, }, ], meta: { has_more: false }, @@ -64,6 +82,10 @@ describe("mapRunStagesToSidebarStages", () => { model: "gpt-5.5", reasoning_effort: "high", }); + // Each visit keeps its own tokens and cost, so the stage popover never + // shows a sibling visit's usage. + expect(result[0].billing?.total_usd_micros).toBe(720_000); + expect(result[1].billing?.total_usd_micros).toBeUndefined(); expect(formatStageLabel(result[0])).toBe("Apply Changes"); expect(result[1].id).toBe("apply-changes@2"); diff --git a/apps/fabro-web/app/lib/stage-sidebar.ts b/apps/fabro-web/app/lib/stage-sidebar.ts index c165adee0..191681459 100644 --- a/apps/fabro-web/app/lib/stage-sidebar.ts +++ b/apps/fabro-web/app/lib/stage-sidebar.ts @@ -1,5 +1,6 @@ import { StageState } from "@qltysh/fabro-api-client"; import type { + BilledTokenCounts, PaginatedRunStageList, StageHandler, StageModelUsage, @@ -27,6 +28,11 @@ export interface Stage { resumedFromStageId: string | null; startedAt: string | null; providerUsed: StageModelUsage | null; + /** + * Tokens and cost for this visit alone, priced the same way the Billing tab + * prices its per-node rows. All-zero counts mean the stage called no model. + */ + billing: BilledTokenCounts | null; } export const ACTIVE_STAGE_STATES: ReadonlySet = new Set([ @@ -102,6 +108,7 @@ export function mapRunStagesToSidebarStages( : "--", startedAt: stage.started_at ?? null, providerUsed: stage.provider_used ?? null, + billing: stage.billing ?? null, }); } return stages; diff --git a/apps/fabro-web/app/routes/run-stages-details.test.tsx b/apps/fabro-web/app/routes/run-stages-details.test.tsx index da18f7e67..2f453c63e 100644 --- a/apps/fabro-web/app/routes/run-stages-details.test.tsx +++ b/apps/fabro-web/app/routes/run-stages-details.test.tsx @@ -1,9 +1,13 @@ import { describe, expect, test } from "bun:test"; import { renderToStaticMarkup } from "react-dom/server"; -import type { ReasoningOutput } from "@qltysh/fabro-api-client"; +import type { + BilledTokenCounts, + ReasoningOutput, + StageModelUsage, +} from "@qltysh/fabro-api-client"; -import { EventDetails } from "./run-stages"; +import { EventDetails, ModelUsagePopover } from "./run-stages"; const RUN_START = "2026-04-09T12:00:00Z"; @@ -72,3 +76,82 @@ describe("EventDetails reasoning", () => { expect(html).toContain(`${"x".repeat(280)}…`); }); }); + +const PROVIDER_USED: StageModelUsage = { + mode: "agent", + provider: "moonshot", + model: "kimi-k3", + reasoning_effort: "max", +}; + +function billing(partial: Partial): BilledTokenCounts { + return { + input_tokens: 0, + output_tokens: 0, + total_tokens: 0, + reasoning_tokens: 0, + cache_read_tokens: 0, + cache_write_tokens: 0, + ...partial, + }; +} + +function popoverMarkup(counts: BilledTokenCounts | null): string { + return renderToStaticMarkup( + , + ); +} + +describe("ModelUsagePopover billing", () => { + test("shows the visit's token buckets and cost next to the model", () => { + const html = popoverMarkup( + billing({ + input_tokens: 28_640, + output_tokens: 7_550, + reasoning_tokens: 1_200, + cache_read_tokens: 4_800, + cache_write_tokens: 1_500, + total_tokens: 43_690, + total_usd_micros: 720_000, + }), + ); + + expect(html).toContain("kimi-k3"); + expect(html).toContain("Cache read"); + expect(html).toContain("4.8k"); + expect(html).toContain("Cache creation"); + expect(html).toContain("1.5k"); + expect(html).toContain("Uncached"); + expect(html).toContain("28.6k"); + // Output folds in reasoning tokens, matching the Billing tab. + expect(html).toContain("Output"); + expect(html).toContain("8.8k"); + expect(html).toContain("Cost"); + expect(html).toContain("$0.72"); + }); + + test("omits the token section for a stage that called no model", () => { + const html = popoverMarkup(billing({})); + + expect(html).toContain("kimi-k3"); + expect(html).not.toContain("Tokens"); + expect(html).not.toContain("Cost"); + }); + + test("still shows tokens when nothing priced the stage", () => { + const html = popoverMarkup( + billing({ input_tokens: 1_000, output_tokens: 500, total_tokens: 1_500 }), + ); + + expect(html).toContain("Uncached"); + expect(html).toContain("1.0k"); + expect(html).not.toContain("Cost"); + }); + + test("renders the model rows alone when the stage list carried no billing", () => { + const html = popoverMarkup(null); + + expect(html).toContain("kimi-k3"); + expect(html).not.toContain("Tokens"); + }); +}); diff --git a/apps/fabro-web/app/routes/run-stages.tsx b/apps/fabro-web/app/routes/run-stages.tsx index 43f4410f1..246f3b2ed 100644 --- a/apps/fabro-web/app/routes/run-stages.tsx +++ b/apps/fabro-web/app/routes/run-stages.tsx @@ -67,6 +67,7 @@ import { formatBytes, formatDurationMs, formatTokenCount, + formatUsdMicros, } from "../lib/format"; import { plural } from "../lib/plural"; import { @@ -93,6 +94,7 @@ import { type UnknownRecord, } from "../lib/unknown"; import type { + BilledTokenCounts, EventEnvelope, ReasoningOutput, StageHandler, @@ -866,10 +868,59 @@ export function formatStageModelUsageLabel( return effort ? `${model}[${effort}]` : model; } -function ModelUsagePopover({ +const POPOVER_NUMBER = "block text-right font-mono tabular-nums"; + +/** + * The disjoint token buckets behind a stage's usage, labelled and ordered to + * match the Billing tab's breakdown so the two views read the same. `Uncached` + * is input that missed the cache; `Output` folds in reasoning tokens. + */ +function stageTokenBuckets(billing: BilledTokenCounts) { + return [ + { label: "Cache read", value: billing.cache_read_tokens }, + { label: "Cache creation", value: billing.cache_write_tokens }, + { label: "Uncached", value: billing.input_tokens }, + { + label: "Output", + value: billing.output_tokens + billing.reasoning_tokens, + }, + ]; +} + +/** Tokens and cost for this stage visit alone. */ +function StageBillingRows({ billing }: { billing: BilledTokenCounts }) { + const buckets = stageTokenBuckets(billing); + if (buckets.every((bucket) => bucket.value === 0)) return null; + const cost = formatUsdMicros(billing.total_usd_micros); + return ( +
+ Tokens + + {buckets.map((bucket) => ( + + + {bucket.value === 0 + ? "0" + : formatTokenCount(bucket.value, { compactDecimal: true })} + + + ))} + {cost && ( + + {cost} + + )} + +
+ ); +} + +export function ModelUsagePopover({ providerUsed, + billing, }: { providerUsed: StageModelUsage; + billing: BilledTokenCounts | null; }) { return ( <> @@ -892,6 +943,7 @@ function ModelUsagePopover({ {providerUsed.speed} )} + {billing && } ); } @@ -1905,6 +1957,7 @@ function EventsToolbar({ filteredCount, totalCount, providerUsed, + billing, events, runId, stageId, @@ -1924,6 +1977,7 @@ function EventsToolbar({ filteredCount: number; totalCount: number; providerUsed: StageModelUsage | null; + billing: BilledTokenCounts | null; events: EventEnvelope[]; runId: string; stageId: string; @@ -2004,7 +2058,9 @@ function EventsToolbar({ className={`inline-flex items-center gap-1.5 text-xs text-fg-muted ${ showFilters ? "" : "ml-auto" }`} - content={} + content={ + + } >