mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-15 23:32:46 +00:00
parent
0a9a0f6f79
commit
07f2f2fd2d
6 changed files with 1034 additions and 23 deletions
281
run.json
281
run.json
File diff suppressed because one or more lines are too long
329
stages/006-simplify_opus@1/diff.patch
Normal file
329
stages/006-simplify_opus@1/diff.patch
Normal file
|
|
@ -0,0 +1,329 @@
|
|||
diff --git a/apps/fabro-web/app/components/stage-sidebar.tsx b/apps/fabro-web/app/components/stage-sidebar.tsx
|
||||
index 4936eb0f..46138ac5 100644
|
||||
--- a/apps/fabro-web/app/components/stage-sidebar.tsx
|
||||
+++ b/apps/fabro-web/app/components/stage-sidebar.tsx
|
||||
@@ -11,7 +11,7 @@ import {
|
||||
} from "@heroicons/react/24/solid";
|
||||
import { Bars3BottomLeftIcon, DocumentTextIcon, MapIcon } from "@heroicons/react/24/outline";
|
||||
import { formatDurationSecs } from "../lib/format";
|
||||
-import { ACTIVE_STAGE_STATES } from "../lib/stage-sidebar";
|
||||
+import { ACTIVE_STAGE_STATES, formatStageLabel } from "../lib/stage-sidebar";
|
||||
|
||||
export interface Stage {
|
||||
id: string;
|
||||
@@ -101,7 +101,7 @@ export function StageSidebar({ stages, runId, selectedStageId, activeLink }: Sta
|
||||
}`}
|
||||
>
|
||||
<Icon className={`size-4 shrink-0 ${config.color} ${ACTIVE_STAGE_STATES.has(stage.status) ? "animate-spin" : ""}`} />
|
||||
- <span className="flex-1 truncate">{stage.visit > 1 ? `${stage.name} (${stage.visit})` : stage.name}</span>
|
||||
+ <span className="flex-1 truncate">{formatStageLabel(stage)}</span>
|
||||
<span className="font-mono text-xs tabular-nums text-fg-muted">{stageDuration(stage)}</span>
|
||||
</Link>
|
||||
</li>
|
||||
diff --git a/apps/fabro-web/app/lib/run-events.ts b/apps/fabro-web/app/lib/run-events.ts
|
||||
index d964cc4d..28115899 100644
|
||||
--- a/apps/fabro-web/app/lib/run-events.ts
|
||||
+++ b/apps/fabro-web/app/lib/run-events.ts
|
||||
@@ -136,10 +136,10 @@ export function subscribeToRunEvents(
|
||||
}
|
||||
|
||||
function stageIdFromPayload(payload: RunEventPayload): string | undefined {
|
||||
- if (typeof payload.stage_id === "string") return payload.stage_id;
|
||||
- if (typeof payload.node_id === "string") return payload.node_id;
|
||||
- const nodeId = payload.properties?.node_id;
|
||||
- return typeof nodeId === "string" ? nodeId : undefined;
|
||||
+ // Only return a true `node_id@visit` StageId. A bare `node_id` would not
|
||||
+ // match the suffixed `stageTurns(runId, "verify@1")` cache key, so falling
|
||||
+ // back to it would silently no-op the invalidation.
|
||||
+ return typeof payload.stage_id === "string" ? payload.stage_id : undefined;
|
||||
}
|
||||
|
||||
export function useRunEvents(runId: string | undefined) {
|
||||
diff --git a/apps/fabro-web/app/lib/stage-sidebar.ts b/apps/fabro-web/app/lib/stage-sidebar.ts
|
||||
index 73fdc112..a820f549 100644
|
||||
--- a/apps/fabro-web/app/lib/stage-sidebar.ts
|
||||
+++ b/apps/fabro-web/app/lib/stage-sidebar.ts
|
||||
@@ -10,6 +10,15 @@ export const SUCCEEDED_STAGE_STATES: ReadonlySet<StageState> = new Set([
|
||||
"partially_succeeded",
|
||||
]);
|
||||
|
||||
+/**
|
||||
+ * Display label for a stage. Suffixes `(N)` for visits > 1 so a looped node
|
||||
+ * (e.g. `verify`) renders as `verify`, `verify (2)`, `verify (3)` in the
|
||||
+ * sidebar and stage header.
|
||||
+ */
|
||||
+export function formatStageLabel(stage: { name: string; visit: number }): string {
|
||||
+ return stage.visit > 1 ? `${stage.name} (${stage.visit})` : stage.name;
|
||||
+}
|
||||
+
|
||||
export function mapRunStagesToSidebarStages(
|
||||
stagesResult: PaginatedRunStageList | null | undefined,
|
||||
): Stage[] {
|
||||
@@ -39,21 +48,27 @@ export function aggregateGraphNodeStatus(stages: readonly Stage[]): Map<
|
||||
string,
|
||||
{ displayStatus: StageState; latestStageId: string }
|
||||
> {
|
||||
- const grouped = new Map<string, Stage[]>();
|
||||
+ // Single pass per nodeId: track the visit with the highest `visit` overall
|
||||
+ // (drives click target + terminal status) and the highest-visit *active*
|
||||
+ // stage (drives display when any visit is in flight).
|
||||
+ const latest = new Map<string, Stage>();
|
||||
+ const latestActive = new Map<string, Stage>();
|
||||
for (const stage of stages) {
|
||||
- const list = grouped.get(stage.nodeId) ?? [];
|
||||
- list.push(stage);
|
||||
- grouped.set(stage.nodeId, list);
|
||||
+ const prevLatest = latest.get(stage.nodeId);
|
||||
+ if (!prevLatest || stage.visit > prevLatest.visit) {
|
||||
+ latest.set(stage.nodeId, stage);
|
||||
+ }
|
||||
+ if (ACTIVE_STAGE_STATES.has(stage.status)) {
|
||||
+ const prevActive = latestActive.get(stage.nodeId);
|
||||
+ if (!prevActive || stage.visit > prevActive.visit) {
|
||||
+ latestActive.set(stage.nodeId, stage);
|
||||
+ }
|
||||
+ }
|
||||
}
|
||||
const result = new Map<string, { displayStatus: StageState; latestStageId: string }>();
|
||||
- for (const [nodeId, list] of grouped) {
|
||||
- list.sort((a, b) => a.visit - b.visit);
|
||||
- const latest = list[list.length - 1];
|
||||
- const activeVisit = [...list]
|
||||
- .reverse()
|
||||
- .find((s) => ACTIVE_STAGE_STATES.has(s.status));
|
||||
- const display = activeVisit ?? latest;
|
||||
- result.set(nodeId, { displayStatus: display.status, latestStageId: latest.id });
|
||||
+ for (const [nodeId, latestStage] of latest) {
|
||||
+ const display = latestActive.get(nodeId) ?? latestStage;
|
||||
+ result.set(nodeId, { displayStatus: display.status, latestStageId: latestStage.id });
|
||||
}
|
||||
return result;
|
||||
}
|
||||
\ No newline at end of file
|
||||
diff --git a/apps/fabro-web/app/routes/run-stages.tsx b/apps/fabro-web/app/routes/run-stages.tsx
|
||||
index bb66bfd7..90ba4ed0 100644
|
||||
--- a/apps/fabro-web/app/routes/run-stages.tsx
|
||||
+++ b/apps/fabro-web/app/routes/run-stages.tsx
|
||||
@@ -41,7 +41,7 @@ import { EmptyState } from "../components/state";
|
||||
import { CopyButton } from "../components/ui";
|
||||
import { formatDurationSecs } from "../lib/format";
|
||||
import { fetchRunCommandLog, useRunEventsList, useRunStageTurns, useRunStages } from "../lib/queries";
|
||||
-import { mapRunStagesToSidebarStages } from "../lib/stage-sidebar";
|
||||
+import { formatStageLabel, mapRunStagesToSidebarStages } from "../lib/stage-sidebar";
|
||||
import { getNumber, getString, type UnknownRecord } from "../lib/unknown";
|
||||
import {
|
||||
CommandOutputStream,
|
||||
@@ -638,7 +638,7 @@ export default function RunStages() {
|
||||
<div className="sticky top-0 z-10 -mx-2 flex items-center gap-2 bg-page/85 px-2 py-2 backdrop-blur">
|
||||
<SelectedIcon className={`size-5 ${selectedConfig.color} ${isRunning ? "animate-spin" : ""}`} />
|
||||
<h3 className="text-base font-semibold text-fg">
|
||||
- {selectedStage.visit > 1 ? `${selectedStage.name} (${selectedStage.visit})` : selectedStage.name}
|
||||
+ {formatStageLabel(selectedStage)}
|
||||
</h3>
|
||||
<span className="font-mono text-xs tabular-nums text-fg-muted">
|
||||
<RunningStageDuration
|
||||
diff --git a/lib/crates/fabro-server/src/server/handler/billing.rs b/lib/crates/fabro-server/src/server/handler/billing.rs
|
||||
index b3ae9c1b..6bf442c0 100644
|
||||
--- a/lib/crates/fabro-server/src/server/handler/billing.rs
|
||||
+++ b/lib/crates/fabro-server/src/server/handler/billing.rs
|
||||
@@ -17,46 +17,39 @@ pub(super) fn routes() -> Router<Arc<AppState>> {
|
||||
.route("/runs/{id}/billing", get(get_run_billing))
|
||||
}
|
||||
|
||||
-/// Pick the stage state from the latest lifecycle event for `stage_id`,
|
||||
-/// falling back to the projection's stored completion when no lifecycle
|
||||
-/// events have landed yet (e.g. an empty event log for a completed run
|
||||
-/// recovered from snapshot only).
|
||||
-fn stage_status_from_events(
|
||||
- events: &[EventEnvelope],
|
||||
- stage_id: &StageId,
|
||||
- projection: &RunProjection,
|
||||
-) -> StageState {
|
||||
- let latest = events.iter().rev().find(|envelope| {
|
||||
- envelope.event.stage_id.as_ref() == Some(stage_id)
|
||||
- && matches!(
|
||||
- &envelope.event.body,
|
||||
- EventBody::StageStarted(_)
|
||||
- | EventBody::StageRetrying(_)
|
||||
- | EventBody::StageCompleted(_)
|
||||
- | EventBody::StageFailed(_)
|
||||
- )
|
||||
- });
|
||||
-
|
||||
- if let Some(envelope) = latest {
|
||||
- return match &envelope.event.body {
|
||||
- EventBody::StageStarted(_) => StageState::Running,
|
||||
- EventBody::StageRetrying(_) => StageState::Retrying,
|
||||
- EventBody::StageFailed(props) => {
|
||||
- if props.will_retry {
|
||||
- StageState::Retrying
|
||||
- } else {
|
||||
- StageState::Failed
|
||||
- }
|
||||
- }
|
||||
- EventBody::StageCompleted(props) => StageState::from(props.status),
|
||||
- _ => StageState::Pending,
|
||||
- };
|
||||
+/// Map a `stage.*` lifecycle event body to the [`StageState`] it implies.
|
||||
+/// Returns `None` for any other variant.
|
||||
+fn stage_state_from_lifecycle(body: &EventBody) -> Option<StageState> {
|
||||
+ match body {
|
||||
+ EventBody::StageStarted(_) => Some(StageState::Running),
|
||||
+ EventBody::StageRetrying(_) => Some(StageState::Retrying),
|
||||
+ EventBody::StageFailed(props) => Some(if props.will_retry {
|
||||
+ StageState::Retrying
|
||||
+ } else {
|
||||
+ StageState::Failed
|
||||
+ }),
|
||||
+ EventBody::StageCompleted(props) => Some(StageState::from(props.status)),
|
||||
+ _ => None,
|
||||
}
|
||||
+}
|
||||
|
||||
- projection
|
||||
- .stage(stage_id)
|
||||
- .and_then(|stage| stage.completion.as_ref())
|
||||
- .map_or(StageState::Pending, |c| StageState::from(c.outcome))
|
||||
+/// Single-pass scan over `events` building the latest [`StageState`] for each
|
||||
+/// [`StageId`] from lifecycle events (started/retrying/completed/failed). Each
|
||||
+/// later lifecycle event overwrites earlier ones, leaving the latest as the
|
||||
+/// stored value — equivalent to "scan in reverse, take first match" but in O(E)
|
||||
+/// for the whole list rather than O(stages × events).
|
||||
+fn latest_stage_states(events: &[EventEnvelope]) -> HashMap<StageId, StageState> {
|
||||
+ let mut states = HashMap::new();
|
||||
+ for envelope in events {
|
||||
+ let Some(stage_id) = envelope.event.stage_id.as_ref() else {
|
||||
+ continue;
|
||||
+ };
|
||||
+ let Some(state) = stage_state_from_lifecycle(&envelope.event.body) else {
|
||||
+ continue;
|
||||
+ };
|
||||
+ states.insert(stage_id.clone(), state);
|
||||
+ }
|
||||
+ states
|
||||
}
|
||||
|
||||
async fn list_run_stages(
|
||||
@@ -77,21 +70,30 @@ async fn list_run_stages(
|
||||
|
||||
let projection = RunProjection::apply_events(&events).unwrap_or_default();
|
||||
let stage_durations = fabro_workflow::extract_stage_durations_by_stage_id(&events);
|
||||
+ let lifecycle_states = latest_stage_states(&events);
|
||||
|
||||
let mut entries: Vec<(&StageId, &fabro_types::StageProjection)> =
|
||||
projection.iter_stages().collect();
|
||||
- entries.sort_by_key(|(_, projection)| projection.first_event_seq);
|
||||
+ entries.sort_by_key(|(_, stage)| stage.first_event_seq);
|
||||
|
||||
let mut stages = Vec::with_capacity(entries.len());
|
||||
- for (stage_id, _projection_stage) in entries {
|
||||
- let duration_ms = stage_durations.get(stage_id).copied();
|
||||
+ for (stage_id, stage_projection) in entries {
|
||||
+ let node_id = stage_id.node_id().to_string();
|
||||
let visit = NonZeroU32::new(stage_id.visit()).expect("StageId.visit is 1-based");
|
||||
+ // Prefer the latest lifecycle event; fall back to the projection's
|
||||
+ // stored completion (e.g. for runs recovered from snapshot only).
|
||||
+ let status = lifecycle_states.get(stage_id).copied().unwrap_or_else(|| {
|
||||
+ stage_projection
|
||||
+ .completion
|
||||
+ .as_ref()
|
||||
+ .map_or(StageState::Pending, |c| StageState::from(c.outcome))
|
||||
+ });
|
||||
stages.push(RunStage {
|
||||
id: stage_id.to_string(),
|
||||
- name: stage_id.node_id().to_string(),
|
||||
- status: stage_status_from_events(&events, stage_id, &projection),
|
||||
- duration_secs: duration_ms.map(|ms| ms as f64 / 1000.0),
|
||||
- node_id: stage_id.node_id().to_string(),
|
||||
+ name: node_id.clone(),
|
||||
+ status,
|
||||
+ duration_secs: stage_durations.get(stage_id).map(|ms| *ms as f64 / 1000.0),
|
||||
+ node_id,
|
||||
visit,
|
||||
});
|
||||
}
|
||||
@@ -215,4 +217,4 @@ async fn get_run_billing(
|
||||
};
|
||||
|
||||
(StatusCode::OK, Json(response)).into_response()
|
||||
-}
|
||||
+}
|
||||
\ No newline at end of file
|
||||
diff --git a/lib/crates/fabro-workflow/src/lib.rs b/lib/crates/fabro-workflow/src/lib.rs
|
||||
index fd6398be..40a48733 100644
|
||||
--- a/lib/crates/fabro-workflow/src/lib.rs
|
||||
+++ b/lib/crates/fabro-workflow/src/lib.rs
|
||||
@@ -20,6 +20,7 @@ use std::sync::Arc;
|
||||
|
||||
use fabro_retro::retro::CompletedStage;
|
||||
use fabro_store::EventEnvelope;
|
||||
+use fabro_types::EventBody;
|
||||
|
||||
/// Callback invoked when a workflow node starts executing.
|
||||
pub type OnNodeCallback = Option<Arc<dyn Fn(&str) + Send + Sync>>;
|
||||
@@ -86,23 +87,23 @@ pub fn build_completed_stages(cp: &records::Checkpoint, run_failed: bool) -> Vec
|
||||
stages
|
||||
}
|
||||
|
||||
+/// Extract the `duration_ms` from a `stage.completed` / `stage.failed`
|
||||
+/// event body, or `None` for any other variant.
|
||||
+fn stage_completion_duration_ms(body: &EventBody) -> Option<u64> {
|
||||
+ match body {
|
||||
+ EventBody::StageCompleted(props) => Some(props.duration_ms),
|
||||
+ EventBody::StageFailed(props) => Some(props.duration_ms),
|
||||
+ _ => None,
|
||||
+ }
|
||||
+}
|
||||
+
|
||||
pub fn extract_stage_durations_from_events(events: &[EventEnvelope]) -> HashMap<String, u64> {
|
||||
let mut durations = HashMap::new();
|
||||
for envelope in events {
|
||||
- let event = &envelope.event;
|
||||
- let event_name = event.event_name();
|
||||
- if event_name != "stage.completed" && event_name != "stage.failed" {
|
||||
- continue;
|
||||
- }
|
||||
- let Some(node_id) = event.node_id.as_deref() else {
|
||||
+ let Some(duration_ms) = stage_completion_duration_ms(&envelope.event.body) else {
|
||||
continue;
|
||||
};
|
||||
- let Some(duration_ms) = event
|
||||
- .properties()
|
||||
- .ok()
|
||||
- .and_then(|properties| properties.get("duration_ms").cloned())
|
||||
- .and_then(|duration| duration.as_u64())
|
||||
- else {
|
||||
+ let Some(node_id) = envelope.event.node_id.as_deref() else {
|
||||
continue;
|
||||
};
|
||||
durations.insert(node_id.to_string(), duration_ms);
|
||||
@@ -120,20 +121,10 @@ pub fn extract_stage_durations_by_stage_id(
|
||||
) -> HashMap<fabro_types::StageId, u64> {
|
||||
let mut durations = HashMap::new();
|
||||
for envelope in events {
|
||||
- let event = &envelope.event;
|
||||
- let event_name = event.event_name();
|
||||
- if event_name != "stage.completed" && event_name != "stage.failed" {
|
||||
- continue;
|
||||
- }
|
||||
- let Some(stage_id) = event.stage_id.as_ref() else {
|
||||
+ let Some(duration_ms) = stage_completion_duration_ms(&envelope.event.body) else {
|
||||
continue;
|
||||
};
|
||||
- let Some(duration_ms) = event
|
||||
- .properties()
|
||||
- .ok()
|
||||
- .and_then(|properties| properties.get("duration_ms").cloned())
|
||||
- .and_then(|duration| duration.as_u64())
|
||||
- else {
|
||||
+ let Some(stage_id) = envelope.event.stage_id.as_ref() else {
|
||||
continue;
|
||||
};
|
||||
durations.insert(stage_id.clone(), duration_ms);
|
||||
@@ -188,4 +179,4 @@ mod stage_scope;
|
||||
pub mod test_support;
|
||||
#[doc(hidden)]
|
||||
pub mod transforms;
|
||||
-pub mod workflow_bundle;
|
||||
+pub mod workflow_bundle;
|
||||
\ No newline at end of file
|
||||
6
stages/006-simplify_opus@1/status.json
Normal file
6
stages/006-simplify_opus@1/status.json
Normal file
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"outcome": "succeeded",
|
||||
"notes": "Stage completed: simplify_opus",
|
||||
"failure_reason": null,
|
||||
"timestamp": "2026-05-04T18:37:59.591026Z"
|
||||
}
|
||||
415
stages/007-simplify_gpt@1/prompt.md
Normal file
415
stages/007-simplify_gpt@1/prompt.md
Normal file
|
|
@ -0,0 +1,415 @@
|
|||
Goal: # Stage URLs encode visit (`node@visit`)
|
||||
|
||||
## Context
|
||||
|
||||
Today, in the Fabro web UI, stages that re-run (e.g. `verify` in a loop) all
|
||||
collapse to the same URL: `/runs/{id}/stages/verify`. The sidebar lists them
|
||||
multiple times but every link/selection points at the first visit.
|
||||
|
||||
A **Stage** is a Node + a Visit (1-indexed; bumped each time the workflow
|
||||
re-enters that node). The data model already knows this:
|
||||
`StageId(node_id, visit)` exists in `lib/crates/fabro-types/src/stage_id.rs`,
|
||||
and the OpenAPI `StageId` path parameter
|
||||
(`docs/public/api-reference/fabro-api.yaml:2925`) already documents the
|
||||
`node_id@visit` form. The bug is that `GET /api/v1/runs/{id}/stages` returns
|
||||
`RunStage.id = node_id` (no visit suffix), and the events fallback in the UI
|
||||
filters by `node_id` instead of the full `stage_id`.
|
||||
|
||||
Note: "visit" is deliberate. There is a separate retry-attempt counter
|
||||
inside a single visit (`StageStartedProps.attempt` in
|
||||
`lib/crates/fabro-types/src/run_event/stage.rs:13`) — that's not what we're
|
||||
modeling here. URLs and the new field both refer to **visits**.
|
||||
|
||||
Outcome: each stage gets a distinct URL (e.g. `verify@1`, `verify@2`) that
|
||||
loads only that visit's turns/logs, with a `(N)` indicator in the sidebar
|
||||
when `N > 1`.
|
||||
|
||||
## Approach
|
||||
|
||||
**Server**: rebuild the stage list from `RunProjection::iter_stages()`, which
|
||||
is already keyed by full `StageId` (`HashMap<StageId, StageProjection>` in
|
||||
`lib/crates/fabro-types/src/run_projection.rs:32`). This deletes the
|
||||
`checkpoint.completed_nodes` walk (which loses visit info — it's a
|
||||
`Vec<String>` of node_ids only) and the `next_node_id` branch entirely.
|
||||
|
||||
**Status derivation is event-driven, not completion-driven.**
|
||||
`StageProjection.completion` is set by `StageFailed` *even when* the workflow
|
||||
is about to retry (`run_state.rs:329` — `StageRetrying` does not clear it),
|
||||
so reading completion alone would show `failed` for a stage that's
|
||||
retrying. For each stage, scan its events (filtered by exact `stage_id`)
|
||||
and take the **latest** lifecycle event:
|
||||
- `stage.retrying` → `StageState::Retrying`
|
||||
- `stage.failed(props)` with `props.will_retry == true` → `StageState::Retrying`
|
||||
- `stage.failed(props)` with `props.will_retry == false` → `StageState::Failed`
|
||||
- `stage.completed` → `StageState::from(StageCompletedProps.status)`
|
||||
- `stage.started` (no later completed/failed/retrying) → `StageState::Running`
|
||||
|
||||
Use the projection's `completion` only as a tiebreaker when no lifecycle
|
||||
events for that stage_id exist (defensive case). The
|
||||
`StageState::from(StageOutcome)` impl is at
|
||||
`lib/crates/fabro-types/src/outcome.rs:136`.
|
||||
|
||||
**API contract**: on `RunStage`, add a required `visit: integer` field, and
|
||||
**rename `dot_id` → `node_id`** (required) for consistency with
|
||||
`StageId::node_id()`, `EventEnvelope.node_id`, and the rest of the type
|
||||
vocabulary. Tighten the `id` description to call out the `node_id@visit`
|
||||
form. This is a breaking field rename; per project policy
|
||||
("simplest change possible, we don't care about migration"), we do it now
|
||||
rather than carrying both names.
|
||||
|
||||
**Frontend**: links and selection already use `stage.id`, so they propagate
|
||||
naturally once the API returns `verify@1`/`verify@2`. The events-fallback
|
||||
filter switches from `e.node_id === stageId` to `e.stage_id === stageId`.
|
||||
Sidebar/header append `(N)` only when `visit > 1`. The
|
||||
`isVisibleStage(stage.id)` filter switches to `isVisibleStage(stage.node_id)`
|
||||
so that `start@1`/`exit@1` are still hidden.
|
||||
|
||||
One fixture run with two visits of the same node is added to demo data so
|
||||
this code path stays under test.
|
||||
|
||||
## Files to change (in order)
|
||||
|
||||
### 1. OpenAPI — `docs/public/api-reference/fabro-api.yaml`
|
||||
|
||||
`RunStage` schema (line 6315):
|
||||
- `id`: clarify description: `StageId in "node_id@visit" form, e.g. verify@2`. Update example to `verify@2`.
|
||||
- Add `visit: { type: integer, format: uint32, minimum: 1, description: "1-based visit count; bumped each time the workflow re-enters this node" }`. Mark required. (`format: uint32` + `minimum: 1` codegens to `NonZeroU32`, matching `StageProjection.first_event_seq` at line 5286–5288 of the spec.)
|
||||
- **Rename `dot_id` → `node_id`** and mark required. Description: "Node id in the workflow graph; multiple stages with different visits share the same node_id." Example: `verify`.
|
||||
|
||||
### 2. Generated code
|
||||
|
||||
- `cargo build -p fabro-api` — regenerates Rust types via `progenitor`.
|
||||
- `cd lib/packages/fabro-api-client && bun run generate` — regenerates TS client.
|
||||
|
||||
### 3. Workflow lib — `lib/crates/fabro-workflow/src/lib.rs`
|
||||
|
||||
Add a sibling to `extract_stage_durations_from_events` (line 89). Leave the
|
||||
existing function alone — `finalize.rs:81,506` and `retro.rs:56` operate on a
|
||||
single visit per node and shouldn't change. New function:
|
||||
|
||||
```rust
|
||||
pub fn extract_stage_durations_by_stage_id(
|
||||
events: &[EventEnvelope],
|
||||
) -> HashMap<StageId, u64>
|
||||
```
|
||||
|
||||
Filters `stage.completed`/`stage.failed`, keys by `envelope.stage_id`.
|
||||
|
||||
### 4. Server handler — `lib/crates/fabro-server/src/server/handler/billing.rs`
|
||||
|
||||
Rewrite `list_run_stages` (lines 38–126):
|
||||
|
||||
- Replace `checkpoint.completed_nodes` iteration with
|
||||
`projection.iter_stages()`, collected and sorted by `first_event_seq`.
|
||||
- Per stage, build `RunStage`:
|
||||
- `id = stage_id.to_string()`
|
||||
- `node_id = stage_id.node_id().to_string()`
|
||||
- `name = stage_id.node_id().to_string()` (UI adds the suffix)
|
||||
- `visit = NonZeroU32::new(stage_id.visit()).expect("StageId.visit is 1-based")`
|
||||
(generated type is `NonZeroU32` because of `format: uint32` + `minimum: 1`)
|
||||
- `status = stage_status_from_events(events, &stage_id, &projection)`
|
||||
(see Status derivation below)
|
||||
- `duration_secs`: from the new `extract_stage_durations_by_stage_id`.
|
||||
- **Status derivation** — replace `active_stage_state_from_events` (line 19)
|
||||
with `stage_status_from_events(events: &[EventEnvelope], stage_id: &StageId,
|
||||
projection: &RunProjection) -> StageState`. Implementation:
|
||||
1. Filter events to those with `envelope.event.stage_id == Some(stage_id)`.
|
||||
2. Find the **latest** lifecycle event among `stage.started`,
|
||||
`stage.retrying`, `stage.completed`, `stage.failed` for that stage_id.
|
||||
3. Map:
|
||||
- `stage.started` → `Running`
|
||||
- `stage.retrying` → `Retrying`
|
||||
- `stage.failed(props)` with `props.will_retry == true` → `Retrying`
|
||||
(a will-retry failure is conceptually mid-retry, even before the
|
||||
`stage.retrying` envelope lands; field defined at
|
||||
`lib/crates/fabro-types/src/run_event/stage.rs:57`)
|
||||
- `stage.failed(props)` with `props.will_retry == false` → `Failed`
|
||||
- `stage.completed` → `StageState::from(StageCompletedProps.status)`
|
||||
(using the existing `From<StageOutcome> for StageState` impl)
|
||||
4. Fallback: if no lifecycle events, use
|
||||
`StageState::from(completion.outcome)` from the projection if present,
|
||||
else `Pending`.
|
||||
- Drop the `next_node_id` branch (lines 113–123) entirely — the projection
|
||||
now carries the in-flight stage.
|
||||
|
||||
### 5. Demo fixtures — `lib/crates/fabro-server/src/demo/mod.rs:1156`
|
||||
|
||||
Suffix existing four IDs with `@1` and set `visit: 1`. Add a 5th entry to
|
||||
model a re-run:
|
||||
|
||||
```rust
|
||||
fn visit(n: u32) -> NonZeroU32 { NonZeroU32::new(n).expect("visit is 1-based") }
|
||||
|
||||
RunStage { id: "apply-changes@1".into(), name: "apply-changes".into(),
|
||||
status: Succeeded, duration_secs: Some(118.0),
|
||||
node_id: "apply".into(), visit: visit(1) },
|
||||
RunStage { id: "apply-changes@2".into(), name: "apply-changes".into(),
|
||||
status: Running, duration_secs: None,
|
||||
node_id: "apply".into(), visit: visit(2) },
|
||||
```
|
||||
|
||||
`visit` codegens to `NonZeroU32` (see Section 1) — direct `1`/`2` literals
|
||||
won't compile. Use the helper above (or inline
|
||||
`NonZeroU32::new(n).unwrap()`).
|
||||
|
||||
Both share `node_id: "apply"` so the graph node lights up regardless of
|
||||
selection.
|
||||
|
||||
### 6. Frontend mapping — `apps/fabro-web/app/lib/stage-sidebar.ts:13`
|
||||
|
||||
- Pass through `visit` and `node_id` (renamed from `dot_id`) to the sidebar
|
||||
`Stage` shape.
|
||||
- Change filter to `isVisibleStage(stage.node_id)` (line 17) so suffixed IDs
|
||||
still hide `start`/`exit`.
|
||||
- Drop the `?? stage.id` fallback (line 21) — `node_id` is now required.
|
||||
|
||||
### 7. Sidebar component — `apps/fabro-web/app/components/stage-sidebar.tsx`
|
||||
|
||||
- Rename the `dotId` field on `Stage` to `nodeId` (line 21).
|
||||
- Add `visit: number` to the `Stage` interface (line 16).
|
||||
- Render display label as `${stage.name}` when `visit <= 1`, otherwise
|
||||
`${stage.name} (${visit})` in the `<span>` at line 103.
|
||||
- Update any callers reading `stage.dotId` (graph highlighting) to
|
||||
`stage.nodeId`.
|
||||
|
||||
### 8. SSE cache invalidation — `apps/fabro-web/app/lib/run-events.ts`
|
||||
|
||||
Two fixes here:
|
||||
|
||||
1. **Suffixed StageId routing** (`:132`): `stageIdFromPayload` currently
|
||||
returns `payload.node_id`. Once
|
||||
`queryKeys.runs.stageTurns(runId, stageId)` is keyed by `verify@1` (lines
|
||||
84, 95 of the same file), invalidations passing `verify` won't match.
|
||||
- Add `stage_id?: string` to `RunEventPayload` (line 14).
|
||||
- In `stageIdFromPayload`: prefer `payload.stage_id`; fall back to
|
||||
`payload.node_id` for events that don't carry the full StageId (e.g.
|
||||
pre-stage envelopes).
|
||||
2. **Add `stage.retrying` to `STAGE_EVENTS`** (line 35). Today the set is
|
||||
`["stage.started", "stage.completed", "stage.failed"]`. The new
|
||||
server-side status logic relies on `stage.retrying`, and the workflow
|
||||
already emits it (`lib/crates/fabro-workflow/src/lifecycle/event.rs:232`).
|
||||
Without this, a selected stage stays visually `failed` until another
|
||||
invalidating event arrives — defeats the P1 fix above.
|
||||
|
||||
Tests:
|
||||
- An envelope with `stage_id: "verify@2"` and `event: "stage.retrying"`
|
||||
invalidates `stages`, `events`, `graph`, run `detail`, and
|
||||
`stageTurns(runId, "verify@2")`.
|
||||
|
||||
### 9. Run-stages route — `apps/fabro-web/app/routes/run-stages.tsx`
|
||||
|
||||
- Line 72: change `events.filter((e) => e.node_id === stageId)` to
|
||||
`events.filter((e) => e.stage_id === stageId)`. The filter narrows the
|
||||
scope so `stageId` (the function parameter) is the authoritative StageId
|
||||
inside the loop.
|
||||
- Lines 117 and 126: drop the `` ?? `${stageId}@1` `` fallback. Note:
|
||||
`EventEnvelope.stage_id` is generated as `string | null | undefined`
|
||||
(see `lib/packages/fabro-api-client/src/models/run-event.ts:34`), so
|
||||
assigning `stageId: e.stage_id` directly fails typecheck. Use the
|
||||
function parameter instead — after the filter, all surviving events have
|
||||
`stage_id === stageId` by construction:
|
||||
```ts
|
||||
pendingCommand = { stageId, script, language };
|
||||
...
|
||||
turns.push({ kind: "command", stageId, ... });
|
||||
```
|
||||
- Header (line ~640): when `selectedStage.visit > 1`, render
|
||||
`${selectedStage.name} (${selectedStage.visit})`.
|
||||
|
||||
### 10. Graph aggregation policy — `apps/fabro-web/app/routes/run-overview.tsx:66-77`
|
||||
|
||||
Today the graph code maps `Map<dotId, stageId>`; with two visits sharing a
|
||||
node_id, the second entry silently overwrites the first, and the status
|
||||
sets union all visits. Make the policy explicit:
|
||||
|
||||
- **Click target**: open the **latest** visit for that node_id (highest
|
||||
`visit`). Build the map deterministically — `Map.set(nodeId, latestStage.id)`
|
||||
after sorting visits ascending.
|
||||
- **Status policy**: *latest visit wins for terminal states; active states
|
||||
win globally.* That is — for a given node, if any visit is `running` or
|
||||
`retrying`, the node renders that active state. Otherwise the node renders
|
||||
the **latest visit's** terminal state. So:
|
||||
- `(failed, running)` → `running` (active wins)
|
||||
- `(failed, succeeded)` → `succeeded` (latest visit wins; failure-then-fix
|
||||
should look healed, not failed)
|
||||
- `(succeeded, failed)` → `failed` (latest visit wins)
|
||||
- `(running, retrying)` → `retrying` (active; pick the latest)
|
||||
- The current if/else cascade in run-overview.tsx orders running before
|
||||
failed unconditionally — switch it to a two-step compute: pick the
|
||||
display status per node by the rule above, *then* render once.
|
||||
- **Frontend tests**: `(failed, running)` → running color, click → `verify@2`.
|
||||
`(failed, succeeded)` → succeeded color, click → `verify@2`.
|
||||
`(succeeded, failed)` → failed color, click → `verify@2`.
|
||||
|
||||
### 11. Tests
|
||||
|
||||
- **`lib/crates/fabro-server/src/server/tests.rs`** (alongside existing
|
||||
`list_run_stages_projects_retrying_until_completion` at line 2126): add
|
||||
`list_run_stages_distinguishes_visits` — build a run with two visits of
|
||||
the same node, hit `GET /runs/{id}/stages`, assert two `RunStage`
|
||||
entries with distinct `id`/`visit` and the same `node_id`.
|
||||
- **`lib/crates/fabro-server/src/server/tests.rs`**:
|
||||
`list_run_stages_shows_retrying_after_failed_event` — a stage where the
|
||||
latest event is `stage.failed` followed by `stage.retrying` renders as
|
||||
`Retrying`, not `Failed`.
|
||||
- **`lib/crates/fabro-server/src/server/tests.rs`**:
|
||||
`list_run_stages_shows_retrying_when_failed_will_retry` — a stage whose
|
||||
*only* lifecycle event so far is `stage.failed { will_retry: true }`
|
||||
(no `stage.retrying` envelope yet) still renders as `Retrying`. Narrower
|
||||
guard for the will_retry branch.
|
||||
- **`apps/fabro-web/app/routes/run-stages.test.ts`**: `turnsFromEvents`
|
||||
filters correctly on `stage_id` (verify@1 events vs verify@2 events do
|
||||
not cross-contaminate).
|
||||
- **`apps/fabro-web/app/lib/stage-sidebar.test.ts`** (new or existing): map
|
||||
fixture with two `apply-changes` visits → two distinct sidebar entries,
|
||||
display labels `apply-changes` and `apply-changes (2)`.
|
||||
- **`apps/fabro-web/app/lib/run-events.test.tsx`**: SSE envelope with
|
||||
`stage_id: "verify@2"` triggers invalidation of
|
||||
`stageTurns(runId, "verify@2")`.
|
||||
- **Graph test** (`apps/fabro-web/app/routes/run-overview.test.tsx` or
|
||||
similar): two visits of the same node — graph status follows the
|
||||
cascade, click target is the latest visit.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Wiring up the production `/runs/{id}/stages/{stageId}/turns` handler
|
||||
(`lib/crates/fabro-server/src/server/handler/mod.rs:116` is
|
||||
`not_implemented`). The events fallback is doing the work today and will
|
||||
keep doing it; the per-stage filter fix is what unblocks multi-visit
|
||||
display.
|
||||
- Per-visit billing breakdown in `get_run_billing` — that path still uses
|
||||
the existing per-node duration map.
|
||||
|
||||
## Critical files
|
||||
|
||||
- `docs/public/api-reference/fabro-api.yaml` — schema source of truth
|
||||
- `lib/crates/fabro-server/src/server/handler/billing.rs` — `list_run_stages`
|
||||
- `lib/crates/fabro-types/src/run_projection.rs` — `iter_stages` data source
|
||||
- `lib/crates/fabro-workflow/src/lib.rs` — new duration extractor
|
||||
- `lib/crates/fabro-server/src/demo/mod.rs` — fixture
|
||||
- `apps/fabro-web/app/routes/run-stages.tsx` — events filter + header
|
||||
- `apps/fabro-web/app/components/stage-sidebar.tsx` — display label
|
||||
- `apps/fabro-web/app/lib/stage-sidebar.ts` — visibility filter + mapping
|
||||
- `apps/fabro-web/app/lib/run-events.ts` — SSE cache invalidation
|
||||
- `apps/fabro-web/app/routes/run-overview.tsx` — graph aggregation policy
|
||||
- `lib/crates/fabro-types/src/outcome.rs` — `From<StageOutcome> for StageState` (already exists; reuse)
|
||||
|
||||
## Verification
|
||||
|
||||
Build:
|
||||
- `cargo build -p fabro-api` — regenerates types from updated YAML
|
||||
- `cd lib/packages/fabro-api-client && bun run generate`
|
||||
- `cargo build --workspace`
|
||||
|
||||
Tests:
|
||||
- `cargo nextest run -p fabro-server` — conformance + new tests in
|
||||
`lib/crates/fabro-server/src/server/tests.rs` (distinguish_visits,
|
||||
shows_retrying_after_failed_event, shows_retrying_when_failed_will_retry)
|
||||
- `cd apps/fabro-web && bun test && bun run typecheck`
|
||||
|
||||
End-to-end (single-visit regression):
|
||||
- `fabro server start` → open the demo URL → confirm
|
||||
`detect-drift`/`propose-changes`/`review-changes` show no `(N)` suffix.
|
||||
URLs are `.../stages/detect-drift@1` etc. Graph still highlights correctly.
|
||||
|
||||
End-to-end (the fix):
|
||||
- Demo: two `apply-changes` rows. Sidebar shows `apply-changes` and
|
||||
`apply-changes (2)`. URLs `.../stages/apply-changes@1` vs
|
||||
`.../stages/apply-changes@2` are distinct and load distinct content. Graph
|
||||
lights up the same `apply` node either way.
|
||||
- Real loop run: trigger a workflow that loops `verify` (fail → fix → pass).
|
||||
Confirm two distinct entries with distinct statuses, durations, turns, and
|
||||
command logs (`/stages/verify@1/logs/stdout` vs
|
||||
`/stages/verify@2/logs/stdout`).
|
||||
|
||||
API contract:
|
||||
- `curl /api/v1/runs/{id}/stages | jq` — `id` contains `@`; `visit` is
|
||||
present and ≥ 1; `node_id` is the bare node id with no `@`. The old
|
||||
`dot_id` field is gone.
|
||||
|
||||
Negative checks:
|
||||
- Terminal run: no trailing in-flight row.
|
||||
- Parallel fanout: still one row per group (parallel branches don't promote
|
||||
to separate `RunStage` entries).
|
||||
- Empty checkpoint: empty list, no panic.
|
||||
- **Retry mid-flight**: trigger a stage that fails and then retries; sidebar
|
||||
shows `Retrying`, not `Failed`. Confirms P1 regression guard.
|
||||
- **SSE liveness**: while a run is active and a stage emits events, the
|
||||
selected stage's turn list updates without a manual refresh — confirms
|
||||
cache invalidation works against suffixed keys.
|
||||
|
||||
|
||||
## Completed stages
|
||||
- **toolchain**: succeeded
|
||||
- Script: `command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1`
|
||||
- Stdout:
|
||||
```
|
||||
cargo 1.95.0 (f2d3ce0bd 2026-03-21)
|
||||
```
|
||||
- Stderr: (empty)
|
||||
- **preflight_compile**: succeeded
|
||||
- Script: `cargo check -q --workspace 2>&1`
|
||||
- Stdout: (empty)
|
||||
- Stderr: (empty)
|
||||
- **preflight_lint**: succeeded
|
||||
- Script: `cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1`
|
||||
- Stdout: (empty)
|
||||
- Stderr: (empty)
|
||||
- **implement**: succeeded
|
||||
- Model: claude-opus-4-7, 176.9k tokens in / 61.5k out
|
||||
- Files: /home/daytona/workspace/apps/fabro-web/app/components/stage-sidebar.tsx, /home/daytona/workspace/apps/fabro-web/app/lib/run-events.test.tsx, /home/daytona/workspace/apps/fabro-web/app/lib/run-events.ts, /home/daytona/workspace/apps/fabro-web/app/lib/stage-sidebar.test.ts, /home/daytona/workspace/apps/fabro-web/app/lib/stage-sidebar.ts, /home/daytona/workspace/apps/fabro-web/app/routes/run-overview.tsx, /home/daytona/workspace/apps/fabro-web/app/routes/run-stages.test.ts, /home/daytona/workspace/apps/fabro-web/app/routes/run-stages.tsx, /home/daytona/workspace/docs/public/api-reference/fabro-api.yaml, /home/daytona/workspace/lib/crates/fabro-server/src/demo/mod.rs, /home/daytona/workspace/lib/crates/fabro-server/src/server/handler/billing.rs, /home/daytona/workspace/lib/crates/fabro-server/src/server/tests.rs, /home/daytona/workspace/lib/crates/fabro-workflow/src/lib.rs, /home/daytona/workspace/lib/packages/fabro-api-client/src/models/run-stage.ts
|
||||
- **simplify_opus**: succeeded
|
||||
- Model: claude-opus-4-7, 60.9k tokens in / 18.1k out
|
||||
- Files: /home/daytona/workspace/apps/fabro-web/app/components/stage-sidebar.tsx, /home/daytona/workspace/apps/fabro-web/app/lib/run-events.ts, /home/daytona/workspace/apps/fabro-web/app/lib/stage-sidebar.ts, /home/daytona/workspace/apps/fabro-web/app/routes/run-stages.tsx, /home/daytona/workspace/lib/crates/fabro-server/src/server/handler/billing.rs, /home/daytona/workspace/lib/crates/fabro-workflow/src/lib.rs
|
||||
|
||||
|
||||
# Simplify: Code Review and Cleanup
|
||||
|
||||
Review changes vs. origin for reuse, quality, and efficiency. Fix any issues found.
|
||||
|
||||
## Phase 1: Identify Changes
|
||||
|
||||
Run git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.
|
||||
|
||||
## Phase 2: Launch Three Review Agents in Parallel
|
||||
|
||||
Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.
|
||||
|
||||
### Agent 1: Code Reuse Review
|
||||
|
||||
For each change:
|
||||
|
||||
1. Search for existing utilities and helpers that could replace newly written code. Use Grep to find similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.
|
||||
2. Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.
|
||||
3. Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.
|
||||
|
||||
Note: This is a greenfield app, so focus on maximizing simplicity and don't worry about changing things to achieve it.
|
||||
|
||||
### Agent 2: Code Quality Review
|
||||
|
||||
Review the same changes for hacky patterns:
|
||||
|
||||
1. Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls
|
||||
2. Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones
|
||||
3. Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction
|
||||
4. Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
|
||||
5. Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase
|
||||
|
||||
Note: This is a greenfield app, so be aggressive in optimizing quality.
|
||||
|
||||
### Agent 3: Efficiency Review
|
||||
|
||||
Review the same changes for efficiency:
|
||||
|
||||
1. Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns
|
||||
2. Missed concurrency: independent operations run sequentially when they could run in parallel
|
||||
3. Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths
|
||||
4. Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error
|
||||
5. Memory: unbounded data structures, missing cleanup, event listener leaks
|
||||
6. Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one
|
||||
|
||||
## Phase 3: Fix Issues
|
||||
|
||||
Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.
|
||||
|
||||
When done, briefly summarize what was fixed (or confirm the code was already clean).
|
||||
5
stages/007-simplify_gpt@1/provider_used.json
Normal file
5
stages/007-simplify_gpt@1/provider_used.json
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
{
|
||||
"mode": "agent",
|
||||
"provider": "openai",
|
||||
"model": "gpt-5.5"
|
||||
}
|
||||
21
stages/007-simplify_gpt@1/response.md
Normal file
21
stages/007-simplify_gpt@1/response.md
Normal file
|
|
@ -0,0 +1,21 @@
|
|||
Reviewed the stage URL changes and applied cleanup fixes.
|
||||
|
||||
Note: I launched the three parallel review agents twice, but the subagent tool failed on `wait` both times with an LLM item persistence error. I closed those agents and completed the reuse/quality/efficiency review locally.
|
||||
|
||||
Fixed:
|
||||
- Restored `stageIdFromPayload` fallback to `node_id` / `properties.node_id` while still preferring full `stage_id`.
|
||||
- Added a fallback invalidation test for node-id-only SSE payloads.
|
||||
- Treated `retrying` as an active state in the stage detail header, so the icon spins and duration ticks like `running`.
|
||||
- Added assertions for sidebar display labels (`Apply Changes`, `Apply Changes (2)`).
|
||||
- Simplified a no-op async/cancellation wrapper in the graph SVG annotation effect.
|
||||
- Cleaned Rust type imports to use named types (`StageProjection`, `StageId`) instead of fully-qualified type paths.
|
||||
- Applied rustfmt’s suggested function signature formatting.
|
||||
- Ensured changed text files end with newlines.
|
||||
|
||||
Validation passed:
|
||||
- `cargo check -p fabro-server -p fabro-workflow`
|
||||
- `cargo +nightly-2026-04-14 fmt --check --all`
|
||||
- `cargo nextest run -p fabro-server list_run_stages_distinguishes_visits list_run_stages_shows_retrying_after_failed_event list_run_stages_shows_retrying_when_failed_will_retry list_run_stages_projects_retrying_until_completion`
|
||||
- `cd apps/fabro-web && bun test app/lib/run-events.test.tsx app/lib/stage-sidebar.test.ts app/routes/run-stages.test.ts`
|
||||
- `cd apps/fabro-web && bun run typecheck`
|
||||
- `git diff --check`
|
||||
Loading…
Add table
Reference in a new issue