From 95dae5afacf0ee61adf090d1591c5f81528179c1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 1 May 2026 20:10:47 -0400 Subject: [PATCH 1/7] feat(run): separate stage state and artifact retries --- .../app/components/stage-sidebar.tsx | 6 +- apps/fabro-web/app/lib/stage-sidebar.ts | 6 +- docs/public/api-reference/fabro-api.yaml | 56 +++- lib/crates/fabro-api/build.rs | 4 +- lib/crates/fabro-api/src/lib.rs | 4 +- .../fabro-api/tests/node_state_round_trip.rs | 45 ---- .../tests/run_projection_round_trip.rs | 5 +- .../fabro-api/tests/stage_state_round_trip.rs | 67 ++--- .../tests/stage_status_round_trip.rs | 68 +++++ .../fabro-cli/src/commands/artifact/cp.rs | 2 +- .../fabro-cli/src/commands/artifact/mod.rs | 1 + lib/crates/fabro-cli/src/commands/dump.rs | 6 +- .../fabro-cli/src/commands/run/attach.rs | 2 +- lib/crates/fabro-cli/src/commands/run/diff.rs | 2 +- .../fabro-cli/src/commands/run/runner.rs | 3 + lib/crates/fabro-cli/tests/it/cmd/dump.rs | 17 +- lib/crates/fabro-cli/tests/it/cmd/inspect.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/run.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/runner.rs | 4 +- lib/crates/fabro-cli/tests/it/cmd/support.rs | 30 ++- .../fabro-cli/tests/it/scenario/smoke.rs | 2 +- .../tests/it/workflow/command_agent_mixed.rs | 7 +- .../tests/it/workflow/command_pipeline.rs | 7 +- .../fabro-cli/tests/it/workflow/full_stack.rs | 7 +- lib/crates/fabro-cli/tests/it/workflow/mod.rs | 28 ++ lib/crates/fabro-client/src/client.rs | 10 +- lib/crates/fabro-core/src/lib.rs | 2 +- lib/crates/fabro-core/src/outcome.rs | 2 +- lib/crates/fabro-dump/src/lib.rs | 224 ++++++++++++++-- lib/crates/fabro-retro/src/retro_agent.rs | 39 +-- lib/crates/fabro-server/src/demo/mod.rs | 8 +- lib/crates/fabro-server/src/server.rs | 169 +++++++++--- .../fabro-server/tests/it/scenario/archive.rs | 2 +- lib/crates/fabro-store/src/artifact_store.rs | 241 ++++++++++++++---- lib/crates/fabro-store/src/lib.rs | 8 +- lib/crates/fabro-store/src/run_state.rs | 90 +++++-- .../src/serializable_projection.rs | 6 +- .../tests/serializable_projection.rs | 8 +- lib/crates/fabro-types/src/lib.rs | 4 +- lib/crates/fabro-types/src/outcome.rs | 26 +- lib/crates/fabro-types/src/run_projection.rs | 42 ++- .../fabro-workflow/src/artifact_upload.rs | 1 + lib/crates/fabro-workflow/src/git.rs | 14 +- .../fabro-workflow/src/handler/agent.rs | 16 +- .../fabro-workflow/src/handler/command.rs | 40 +-- .../fabro-workflow/src/handler/parallel.rs | 8 +- .../fabro-workflow/src/handler/prompt.rs | 7 +- .../fabro-workflow/src/lifecycle/artifact.rs | 23 +- .../fabro-workflow/src/operations/fork.rs | 2 +- lib/crates/fabro-workflow/src/outcome.rs | 2 +- .../src/pipeline/execute/tests.rs | 4 +- .../src/pipeline/pull_request.rs | 22 +- .../tests/it/daytona_integration.rs | 9 +- .../fabro-workflow/tests/it/integration.rs | 69 ++--- .../src/.openapi-generator/FILES | 2 +- .../src/api/run-internals-api.ts | 48 +++- .../src/models/artifact-entry.ts | 10 +- .../src/models/artifact-list-response.ts | 2 +- .../fabro-api-client/src/models/index.ts | 2 +- .../fabro-api-client/src/models/model.ts | 1 - .../fabro-api-client/src/models/node-state.ts | 45 ---- .../src/models/run-projection.ts | 10 +- .../fabro-api-client/src/models/run-stage.ts | 4 +- .../src/models/stage-state.ts | 42 ++- .../src/models/stage-status.ts | 32 +++ 65 files changed, 1162 insertions(+), 517 deletions(-) delete mode 100644 lib/crates/fabro-api/tests/node_state_round_trip.rs create mode 100644 lib/crates/fabro-api/tests/stage_status_round_trip.rs delete mode 100644 lib/packages/fabro-api-client/src/models/node-state.ts create mode 100644 lib/packages/fabro-api-client/src/models/stage-status.ts diff --git a/apps/fabro-web/app/components/stage-sidebar.tsx b/apps/fabro-web/app/components/stage-sidebar.tsx index b72ee77bd..c7ae643da 100644 --- a/apps/fabro-web/app/components/stage-sidebar.tsx +++ b/apps/fabro-web/app/components/stage-sidebar.tsx @@ -1,6 +1,6 @@ import { useState, useEffect, useRef, type ComponentType } from "react"; import { Link } from "react-router"; -import type { StageState } from "@qltysh/fabro-api-client"; +import type { StageStatus } from "@qltysh/fabro-api-client"; import { ArrowPathIcon, CheckCircleIcon, @@ -16,12 +16,12 @@ import { ACTIVE_STAGE_STATES } from "../lib/stage-sidebar"; export interface Stage { id: string; name: string; - status: StageState; + status: StageStatus; duration: string; dotId?: string; } -export const statusConfig: Record; color: string }> = { +export const statusConfig: Record; color: string }> = { pending: { icon: PauseCircleIcon, color: "text-fg-muted" }, running: { icon: ArrowPathIcon, color: "text-teal-500" }, retrying: { icon: ArrowPathIcon, color: "text-amber" }, diff --git a/apps/fabro-web/app/lib/stage-sidebar.ts b/apps/fabro-web/app/lib/stage-sidebar.ts index 747c6a70e..c346722c1 100644 --- a/apps/fabro-web/app/lib/stage-sidebar.ts +++ b/apps/fabro-web/app/lib/stage-sidebar.ts @@ -1,11 +1,11 @@ -import type { PaginatedRunStageList, StageState } from "@qltysh/fabro-api-client"; +import type { PaginatedRunStageList, StageStatus } from "@qltysh/fabro-api-client"; import type { Stage } from "../components/stage-sidebar"; import { isVisibleStage } from "../data/runs"; import { formatDurationSecs } from "./format"; -export const ACTIVE_STAGE_STATES: ReadonlySet = new Set(["running", "retrying"]); -export const SUCCEEDED_STAGE_STATES: ReadonlySet = new Set([ +export const ACTIVE_STAGE_STATES: ReadonlySet = new Set(["running", "retrying"]); +export const SUCCEEDED_STAGE_STATES: ReadonlySet = new Set([ "succeeded", "partially_succeeded", ]); diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index bd1858b5a..9061603c4 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -2064,6 +2064,7 @@ paths: parameters: - $ref: "#/components/parameters/RunId" - $ref: "#/components/parameters/StageId" + - $ref: "#/components/parameters/ArtifactRetry" - name: filename in: query required: false @@ -2109,6 +2110,7 @@ paths: - $ref: "#/components/parameters/RunId" - $ref: "#/components/parameters/StageId" - $ref: "#/components/parameters/ArtifactFilename" + - $ref: "#/components/parameters/ArtifactRetry" responses: "200": description: Artifact contents @@ -2118,7 +2120,7 @@ paths: type: string format: binary "400": - description: Missing filename + description: Missing filename or retry headers: x-request-id: $ref: "#/components/headers/XRequestId" @@ -2980,6 +2982,17 @@ components: type: string example: src/lib.rs + ArtifactRetry: + name: retry + in: query + required: true + description: Retry attempt number for the artifact. + schema: + type: integer + format: int32 + minimum: 0 + example: 1 + SinceSeq: name: since_seq in: query @@ -4949,18 +4962,32 @@ components: example: true ArtifactEntry: - description: A single artifact filename. + description: A single artifact file for a stage. type: object required: - filename + - retry + - size properties: filename: type: string description: Artifact filename. example: src/lib.rs + retry: + type: integer + format: int32 + minimum: 0 + description: Retry attempt number. + example: 1 + size: + type: integer + format: int64 + minimum: 0 + description: Artifact size in bytes. + example: 1234 ArtifactListResponse: - description: List of artifact filenames for a stage. + description: List of artifact files for a stage. type: object required: - data @@ -5031,6 +5058,7 @@ components: retry: type: integer format: int32 + minimum: 0 description: Retry attempt number. relative_path: type: string @@ -5038,6 +5066,7 @@ components: size: type: integer format: int64 + minimum: 0 description: Artifact size in bytes. RunArtifactListResponse: @@ -5091,10 +5120,15 @@ components: type: string format: date-time - NodeState: - description: Internal node projection state. + StageState: + description: Internal stage projection state. type: object properties: + seq: + type: integer + format: int32 + minimum: 0 + description: Event-log sequence number of the first event that created this stage. Used to order stages by execution. prompt: type: ["string", "null"] response: @@ -5247,7 +5281,7 @@ components: description: Raw internal run projection derived from the event log. type: object required: - - nodes + - stages properties: spec: oneOf: @@ -5310,11 +5344,11 @@ components: type: object additionalProperties: $ref: "#/components/schemas/PendingInterviewRecord" - nodes: + stages: type: object - description: Map from StageId (`node_id@visit`) to NodeState. + description: Map from StageId (`node_id@visit`) to StageState. additionalProperties: - $ref: "#/components/schemas/NodeState" + $ref: "#/components/schemas/StageState" RunSummary: description: Durable run summary derived from the backing store. @@ -6097,7 +6131,7 @@ components: # ── Stage / Turn Schemas ───────────────────────────────────────────── - StageState: + StageStatus: description: Lifecycle projection state of a workflow stage. type: string enum: @@ -6127,7 +6161,7 @@ components: description: Human-readable stage name. example: Propose Changes status: - $ref: "#/components/schemas/StageState" + $ref: "#/components/schemas/StageStatus" duration_secs: type: number description: Time spent in this stage, in seconds. diff --git a/lib/crates/fabro-api/build.rs b/lib/crates/fabro-api/build.rs index 7f8a4c41e..004b59741 100644 --- a/lib/crates/fabro-api/build.rs +++ b/lib/crates/fabro-api/build.rs @@ -314,14 +314,14 @@ fn main() { ("QuestionType", "fabro_types::QuestionType", &[]), ("NodeStatusRecord", "fabro_types::NodeStatusRecord", &[]), ("StageOutcome", "fabro_types::StageOutcome", &[]), - ("StageState", "fabro_types::StageState", &[]), + ("StageStatus", "fabro_types::StageStatus", &[]), ( "CommandOutputStream", "fabro_types::CommandOutputStream", &[], ), ("CommandTermination", "fabro_types::CommandTermination", &[]), - ("NodeState", "fabro_types::NodeState", &[]), + ("StageState", "fabro_types::StageState", &[]), ("SecretMetadata", "fabro_types::SecretMetadata", &[]), ("InterviewOption", "fabro_types::InterviewOption", &[]), ( diff --git a/lib/crates/fabro-api/src/lib.rs b/lib/crates/fabro-api/src/lib.rs index 78e3f7ab1..ec3318881 100644 --- a/lib/crates/fabro-api/src/lib.rs +++ b/lib/crates/fabro-api/src/lib.rs @@ -31,9 +31,9 @@ pub mod types { pub use fabro_types::{ ActorKind, ActorRef, BilledTokenCounts, CommandOutputStream, CommandTermination, DiffStats, DirtyStatus, EventEnvelope, GitContext, InterviewOption, InterviewQuestionRecord, - NodeState, NodeStatusRecord, PendingInterviewRecord, PreRunPushOutcome, QuestionType, + NodeStatusRecord, PendingInterviewRecord, PreRunPushOutcome, QuestionType, RepositoryReference, RunEvent, RunProjection, RunSummary, SecretMetadata, SecretType, - ServerSettings, StageOutcome, StageState, WorkflowSettings, + ServerSettings, StageOutcome, StageState, StageStatus, WorkflowSettings, }; pub use crate::generated::types::*; diff --git a/lib/crates/fabro-api/tests/node_state_round_trip.rs b/lib/crates/fabro-api/tests/node_state_round_trip.rs deleted file mode 100644 index 7bc1ea598..000000000 --- a/lib/crates/fabro-api/tests/node_state_round_trip.rs +++ /dev/null @@ -1,45 +0,0 @@ -use std::any::{TypeId, type_name}; - -use fabro_api::types::NodeState as ApiNodeState; -use fabro_types::NodeState; -use serde_json::json; - -#[test] -fn node_state_reuses_canonical_type() { - assert_same_type::(); -} - -#[test] -fn node_state_round_trips_representative_json() { - let value = json!({ - "prompt": "build it", - "response": "done", - "status": { - "status": "succeeded", - "notes": null, - "failure_reason": null, - "timestamp": "2026-04-29T12:34:56Z" - }, - "provider_used": { "provider": "openai", "model": "gpt-5.2" }, - "diff": "diff --git a/file b/file", - "script_invocation": { "command": "cargo test" }, - "script_timing": { "duration_ms": 42 }, - "parallel_results": [{ "branch": 0, "status": "succeeded" }], - "stdout": "ok", - "stderr": "", - "termination": "exited" - }); - - let state: NodeState = serde_json::from_value(value.clone()).unwrap(); - assert_eq!(serde_json::to_value(state).unwrap(), value); -} - -fn assert_same_type() { - assert_eq!( - TypeId::of::(), - TypeId::of::(), - "{} should be the same type as {}", - type_name::(), - type_name::() - ); -} diff --git a/lib/crates/fabro-api/tests/run_projection_round_trip.rs b/lib/crates/fabro-api/tests/run_projection_round_trip.rs index 20c6bac2d..27a4c85d6 100644 --- a/lib/crates/fabro-api/tests/run_projection_round_trip.rs +++ b/lib/crates/fabro-api/tests/run_projection_round_trip.rs @@ -56,8 +56,9 @@ fn run_projection_round_trips_populated_projection() { "started_at": "2026-04-29T12:35:00Z" } }, - "nodes": { + "stages": { "build@2": { + "seq": 0, "prompt": null, "response": null, "status": null, @@ -96,7 +97,7 @@ fn run_projection_round_trips_with_pending_control_unset() { "pull_request": null, "superseded_by": null, "pending_interviews": {}, - "nodes": {} + "stages": {} }); let projection: RunProjection = serde_json::from_value(value.clone()).unwrap(); diff --git a/lib/crates/fabro-api/tests/stage_state_round_trip.rs b/lib/crates/fabro-api/tests/stage_state_round_trip.rs index f1c4f7740..29adc6171 100644 --- a/lib/crates/fabro-api/tests/stage_state_round_trip.rs +++ b/lib/crates/fabro-api/tests/stage_state_round_trip.rs @@ -10,51 +10,30 @@ fn stage_state_reuses_canonical_type() { } #[test] -fn stage_state_serializes_as_lifecycle_strings() { - assert_eq!( - serde_json::to_value(StageState::Pending).unwrap(), - json!("pending") - ); - assert_eq!( - serde_json::to_value(StageState::Running).unwrap(), - json!("running") - ); - assert_eq!( - serde_json::to_value(StageState::Retrying).unwrap(), - json!("retrying") - ); - assert_eq!( - serde_json::to_value(StageState::Succeeded).unwrap(), - json!("succeeded") - ); - assert_eq!( - serde_json::to_value(StageState::PartiallySucceeded).unwrap(), - json!("partially_succeeded") - ); - assert_eq!( - serde_json::to_value(StageState::Failed).unwrap(), - json!("failed") - ); - assert_eq!( - serde_json::to_value(StageState::Skipped).unwrap(), - json!("skipped") - ); - assert_eq!( - serde_json::to_value(StageState::Cancelled).unwrap(), - json!("cancelled") - ); -} +fn stage_state_round_trips_representative_json() { + let value = json!({ + "seq": 42, + "prompt": "build it", + "response": "done", + "status": { + "status": "succeeded", + "notes": null, + "failure_reason": null, + "timestamp": "2026-04-29T12:34:56Z" + }, + "provider_used": { "provider": "openai", "model": "gpt-5.2" }, + "diff": "diff --git a/file b/file", + "script_invocation": { "command": "cargo test" }, + "script_timing": { "duration_ms": 42 }, + "parallel_results": [{ "branch": 0, "status": "succeeded" }], + "stdout": "ok", + "stderr": "", + "termination": "exited" + }); -#[test] -fn stage_state_deserializes_representative_values() { - assert_eq!( - serde_json::from_value::(json!("retrying")).unwrap(), - StageState::Retrying - ); - assert_eq!( - serde_json::from_value::(json!("partially_succeeded")).unwrap(), - StageState::PartiallySucceeded - ); + let state: StageState = serde_json::from_value(value.clone()).unwrap(); + assert_eq!(state.seq, 42); + assert_eq!(serde_json::to_value(state).unwrap(), value); } fn assert_same_type() { diff --git a/lib/crates/fabro-api/tests/stage_status_round_trip.rs b/lib/crates/fabro-api/tests/stage_status_round_trip.rs new file mode 100644 index 000000000..de78bffcc --- /dev/null +++ b/lib/crates/fabro-api/tests/stage_status_round_trip.rs @@ -0,0 +1,68 @@ +use std::any::{TypeId, type_name}; + +use fabro_api::types::StageStatus as ApiStageStatus; +use fabro_types::StageStatus; +use serde_json::json; + +#[test] +fn stage_status_reuses_canonical_type() { + assert_same_type::(); +} + +#[test] +fn stage_status_serializes_as_lifecycle_strings() { + assert_eq!( + serde_json::to_value(StageStatus::Pending).unwrap(), + json!("pending") + ); + assert_eq!( + serde_json::to_value(StageStatus::Running).unwrap(), + json!("running") + ); + assert_eq!( + serde_json::to_value(StageStatus::Retrying).unwrap(), + json!("retrying") + ); + assert_eq!( + serde_json::to_value(StageStatus::Succeeded).unwrap(), + json!("succeeded") + ); + assert_eq!( + serde_json::to_value(StageStatus::PartiallySucceeded).unwrap(), + json!("partially_succeeded") + ); + assert_eq!( + serde_json::to_value(StageStatus::Failed).unwrap(), + json!("failed") + ); + assert_eq!( + serde_json::to_value(StageStatus::Skipped).unwrap(), + json!("skipped") + ); + assert_eq!( + serde_json::to_value(StageStatus::Cancelled).unwrap(), + json!("cancelled") + ); +} + +#[test] +fn stage_status_deserializes_representative_values() { + assert_eq!( + serde_json::from_value::(json!("retrying")).unwrap(), + StageStatus::Retrying + ); + assert_eq!( + serde_json::from_value::(json!("partially_succeeded")).unwrap(), + StageStatus::PartiallySucceeded + ); +} + +fn assert_same_type() { + assert_eq!( + TypeId::of::(), + TypeId::of::(), + "{} should be the same type as {}", + type_name::(), + type_name::() + ); +} diff --git a/lib/crates/fabro-cli/src/commands/artifact/cp.rs b/lib/crates/fabro-cli/src/commands/artifact/cp.rs index bf989aac2..e33ec2cfc 100644 --- a/lib/crates/fabro-cli/src/commands/artifact/cp.rs +++ b/lib/crates/fabro-cli/src/commands/artifact/cp.rs @@ -142,7 +142,7 @@ async fn write_artifact_file( .with_context(|| format!("creating directory {}", parent.display()))?; } let bytes = client - .download_stage_artifact(run_id, &entry.stage_id, &entry.relative_path) + .download_stage_artifact(run_id, &entry.stage_id, entry.retry, &entry.relative_path) .await?; std::fs::write(dest_file, bytes) .with_context(|| format!("Failed to write {}", dest_file.display()))?; diff --git a/lib/crates/fabro-cli/src/commands/artifact/mod.rs b/lib/crates/fabro-cli/src/commands/artifact/mod.rs index b9a5ffcf7..d69caf24e 100644 --- a/lib/crates/fabro-cli/src/commands/artifact/mod.rs +++ b/lib/crates/fabro-cli/src/commands/artifact/mod.rs @@ -52,6 +52,7 @@ pub(super) async fn resolve_artifacts( entries.sort_by(|a, b| { a.stage_id .cmp(&b.stage_id) + .then_with(|| a.retry.cmp(&b.retry)) .then_with(|| a.relative_path.cmp(&b.relative_path)) }); diff --git a/lib/crates/fabro-cli/src/commands/dump.rs b/lib/crates/fabro-cli/src/commands/dump.rs index 170260398..2d9bc0f57 100644 --- a/lib/crates/fabro-cli/src/commands/dump.rs +++ b/lib/crates/fabro-cli/src/commands/dump.rs @@ -96,8 +96,10 @@ async fn write_run_dump( .stage_id .parse() .with_context(|| format!("server returned invalid stage id {:?}", artifact.stage_id))?; + let retry = u32::try_from(artifact.retry) + .context("server returned invalid negative artifact retry")?; let data = client - .download_stage_artifact(run_id, &stage_id, &artifact.relative_path) + .download_stage_artifact(run_id, &stage_id, retry, &artifact.relative_path) .await .with_context(|| { format!( @@ -105,7 +107,7 @@ async fn write_run_dump( artifact.relative_path, artifact.stage_id ) })?; - dump.add_artifact_bytes(&stage_id, &artifact.relative_path, data)?; + dump.add_artifact_bytes(&stage_id, retry, &artifact.relative_path, data)?; } let output_dir = output_dir.to_path_buf(); diff --git a/lib/crates/fabro-cli/src/commands/run/attach.rs b/lib/crates/fabro-cli/src/commands/run/attach.rs index 4e71d6a28..58e377bf6 100644 --- a/lib/crates/fabro-cli/src/commands/run/attach.rs +++ b/lib/crates/fabro-cli/src/commands/run/attach.rs @@ -506,7 +506,7 @@ mod tests { "sandbox": null, "final_patch": null, "pull_request": null, - "nodes": {} + "stages": {} }) } diff --git a/lib/crates/fabro-cli/src/commands/run/diff.rs b/lib/crates/fabro-cli/src/commands/run/diff.rs index e4ee319fb..1c9394a71 100644 --- a/lib/crates/fabro-cli/src/commands/run/diff.rs +++ b/lib/crates/fabro-cli/src/commands/run/diff.rs @@ -51,7 +51,7 @@ pub(crate) async fn run(args: DiffArgs, base_ctx: &CommandContext) -> Result<()> fn resolve_diff(state: &RunProjection, args: &DiffArgs) -> Result { if let Some(ref node_id) = args.node { if let Some(visit) = state.list_node_visits(node_id).into_iter().max() { - if let Some(node) = state.node(&fabro_store::StageId::new(node_id, visit)) { + if let Some(node) = state.stage(&fabro_store::StageId::new(node_id, visit)) { if let Some(patch) = node.diff.clone() { debug!(node_id, visit, "Reading per-node diff from projected state"); return Ok(patch); diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 151774d39..87dfd4717 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -268,6 +268,7 @@ impl StageArtifactUploader for HttpArtifactUploader { async fn upload_stage_artifacts( &self, stage_id: &fabro_types::StageId, + retry: u32, artifact_capture_dir: &Path, artifacts: &[ArtifactUpload], ) -> Result<()> { @@ -282,6 +283,7 @@ impl StageArtifactUploader for HttpArtifactUploader { .upload_stage_artifact_file( &self.run_id, stage_id, + retry, &artifact.path, &artifact_capture_dir.join(&artifact.path), &self.worker_token, @@ -293,6 +295,7 @@ impl StageArtifactUploader for HttpArtifactUploader { .upload_stage_artifact_batch( &self.run_id, stage_id, + retry, artifact_capture_dir, artifacts, &self.worker_token, diff --git a/lib/crates/fabro-cli/tests/it/cmd/dump.rs b/lib/crates/fabro-cli/tests/it/cmd/dump.rs index 11b92c8ed..12b13ee9a 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/dump.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/dump.rs @@ -248,7 +248,10 @@ include = ["assets/**"] "run export should hydrate blob refs\n{run_json}" ); assert_eq!( - fs::read_to_string(output_dir.join("artifacts/big@1/assets/shared/report.txt")).unwrap(), + fs::read_to_string( + output_dir.join("artifacts/002-big@1/retry-0001/assets/shared/report.txt") + ) + .unwrap(), "exported" ); } @@ -282,12 +285,12 @@ fn dump_exports_completed_run_snapshot() { graph.fabro run.json run.log - stages/exit@1/status.json - stages/report@1/response.md - stages/report@1/status.json - stages/run_tests@1/response.md - stages/run_tests@1/status.json - stages/start@1/status.json + stages/001-start@1/status.json + stages/002-run_tests@1/response.md + stages/002-run_tests@1/status.json + stages/003-report@1/response.md + stages/003-report@1/status.json + stages/004-exit@1/status.json "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/inspect.rs b/lib/crates/fabro-cli/tests/it/cmd/inspect.rs index c9dad593b..26f6be124 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/inspect.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/inspect.rs @@ -83,7 +83,7 @@ fn inspect_resolves_selector_via_server_endpoint() { .path(format!("/api/v1/runs/{}/state", run_id.as_str())); then.status(200) .header("content-type", "application/json") - .body(r#"{"nodes": {}}"#); + .body(r#"{"stages": {}}"#); }); let mut cmd = context.command(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index 43bff5713..0b4df300e 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -63,7 +63,7 @@ fn remote_run_state_response() -> serde_json::Value { "sandbox": null, "final_patch": null, "pull_request": null, - "nodes": {} + "stages": {} }) } diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index d93c7c1cf..fafa1681c 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -553,7 +553,7 @@ methods = ["dev-token"] let state = run_state(&run_dir); let probe_stage_id = StageId::new("probe", 1); let _probe = state - .node(&probe_stage_id) + .stage(&probe_stage_id) .expect("probe node state should exist"); let stdout = command_log_text(&run_dir, &probe_stage_id, CommandOutputStream::Stdout); assert!( @@ -723,7 +723,7 @@ fn runner_reports_missing_run_spec_without_prefetching_events() { "sandbox": null, "final_patch": null, "pull_request": null, - "nodes": {} + "stages": {} }) .to_string(), ); diff --git a/lib/crates/fabro-cli/tests/it/cmd/support.rs b/lib/crates/fabro-cli/tests/it/cmd/support.rs index 09a187518..0d8b7911f 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/support.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/support.rs @@ -869,15 +869,24 @@ async fn seed_artifact_run(context: &TestContext) -> RunSetup { let (client, base_url) = server_endpoint(&context.storage_dir) .expect("test server endpoint should be available for seeded artifacts"); append_seeded_artifact_run_events(&client, &base_url, &run, context).await; - for (stage_id, path, contents) in [ - ("create_assets@1", "assets/node_a/summary.txt", "alpha"), - ("create_assets@1", "assets/shared/report.txt", "one"), - ("create_colliding@1", "assets/other/summary.txt", "beta"), - ("create_colliding@1", "assets/retry/report.txt", "second"), - ("retry_assets@1", "assets/retry/report.txt", "first"), - ("retry_assets@2", "assets/retry/report.txt", "second"), + for (stage_id, retry, path, contents) in [ + ("create_assets@1", 1, "assets/node_a/summary.txt", "alpha"), + ("create_assets@1", 1, "assets/shared/report.txt", "one"), + ("create_colliding@1", 1, "assets/other/summary.txt", "beta"), + ("create_colliding@1", 1, "assets/retry/report.txt", "second"), + ("retry_assets@1", 1, "assets/retry/report.txt", "first"), + ("retry_assets@1", 2, "assets/retry/report.txt", "second"), ] { - upload_seeded_artifact(&client, &base_url, &run.run_id, stage_id, path, contents).await; + upload_seeded_artifact( + &client, + &base_url, + &run.run_id, + stage_id, + retry, + path, + contents, + ) + .await; } run @@ -1368,12 +1377,13 @@ async fn upload_seeded_artifact( base_url: &str, run_id: &str, stage_id: &str, + retry: u32, path: &str, contents: &str, ) { let response = client .post(format!( - "{base_url}/api/v1/runs/{run_id}/stages/{stage_id}/artifacts?filename={path}" + "{base_url}/api/v1/runs/{run_id}/stages/{stage_id}/artifacts?filename={path}&retry={retry}" )) .header(fabro_http::header::CONTENT_TYPE, "application/octet-stream") .body(contents.to_string()) @@ -1383,7 +1393,7 @@ async fn upload_seeded_artifact( expect_reqwest_status( response, fabro_http::StatusCode::NO_CONTENT, - format!("POST /api/v1/runs/{run_id}/stages/{stage_id}/artifacts ({path})"), + format!("POST /api/v1/runs/{run_id}/stages/{stage_id}/artifacts ({path}, retry {retry})"), ) .await; } diff --git a/lib/crates/fabro-cli/tests/it/scenario/smoke.rs b/lib/crates/fabro-cli/tests/it/scenario/smoke.rs index 9cb223a33..6820875fe 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/smoke.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/smoke.rs @@ -21,7 +21,7 @@ fn live_run_state_response() -> serde_json::Value { "sandbox": null, "final_patch": null, "pull_request": null, - "nodes": {} + "stages": {} }) } diff --git a/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs b/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs index d01318b69..9bdd6b7ec 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/command_agent_mixed.rs @@ -7,7 +7,7 @@ use fabro_test::test_context; use super::{ completed_nodes, dump_export, find_run_dir, fixture, read_conclusion, run_id_for, - sandbox_tests, timeout_for, + sandbox_tests, stage_dump_dir, timeout_for, }; sandbox_tests!(command_agent_mixed, keys = ["ANTHROPIC_API_KEY"]); @@ -49,8 +49,9 @@ fn scenario_command_agent_mixed(sandbox: &str) { ); let export_dir = dump_export(&context, &run_id_for(&run_dir)); - let stdout = std::fs::read_to_string(export_dir.join("stages/verify@1/stdout.log")) - .expect("verify stdout.log should exist"); + let stdout = + std::fs::read_to_string(stage_dump_dir(&export_dir, "verify@1").join("stdout.log")) + .expect("verify stdout.log should exist"); assert!( stdout.contains("SCENARIO_FLAG_42"), "verify stdout should contain SCENARIO_FLAG_42, got: {stdout}" diff --git a/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs b/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs index 9120c123b..e691fb007 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/command_pipeline.rs @@ -7,7 +7,7 @@ use fabro_test::test_context; use super::{ completed_nodes, dump_export, find_run_dir, fixture, read_conclusion, run_id_for, - sandbox_tests, timeout_for, + sandbox_tests, stage_dump_dir, timeout_for, }; sandbox_tests!(command_pipeline); @@ -48,8 +48,9 @@ fn scenario_command_pipeline(sandbox: &str) { ); let export_dir = dump_export(&context, &run_id_for(&run_dir)); - let stdout1 = std::fs::read_to_string(export_dir.join("stages/step1@1/stdout.log")) - .expect("step1 stdout.log should exist"); + let stdout1 = + std::fs::read_to_string(stage_dump_dir(&export_dir, "step1@1").join("stdout.log")) + .expect("step1 stdout.log should exist"); assert!( stdout1.contains("hello-from-step1"), "step1 stdout should contain hello-from-step1, got: {stdout1}" diff --git a/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs b/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs index 555c53fcf..921d01855 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/full_stack.rs @@ -7,7 +7,7 @@ use fabro_test::test_context; use super::{ completed_nodes, dump_export, find_run_dir, fixture, has_event, read_conclusion, read_run_spec, - run_id_for, sandbox_tests, timeout_for, + run_id_for, sandbox_tests, stage_dump_dir, timeout_for, }; sandbox_tests!(full_stack, keys = ["ANTHROPIC_API_KEY"]); @@ -74,8 +74,9 @@ fn scenario_full_stack(sandbox: &str) { // Verify node stdout should contain PASS let export_dir = dump_export(&context, &run_id_for(&run_dir)); - let stdout = std::fs::read_to_string(export_dir.join("stages/verify@1/stdout.log")) - .expect("verify stdout.log should exist"); + let stdout = + std::fs::read_to_string(stage_dump_dir(&export_dir, "verify@1").join("stdout.log")) + .expect("verify stdout.log should exist"); assert!( stdout.contains("PASS"), "verify stdout should contain PASS, got: {stdout}" diff --git a/lib/crates/fabro-cli/tests/it/workflow/mod.rs b/lib/crates/fabro-cli/tests/it/workflow/mod.rs index b7911837a..15d8cd508 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/mod.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/mod.rs @@ -76,6 +76,34 @@ pub(super) fn dump_export(context: &TestContext, run_id: &str) -> PathBuf { output_dir } +#[expect( + clippy::disallowed_methods, + reason = "integration test helpers inspect exported files synchronously" +)] +pub(super) fn stage_dump_dir(export_dir: &Path, stage_id: &str) -> PathBuf { + let stages_dir = export_dir.join("stages"); + let mut matches: Vec<_> = std::fs::read_dir(&stages_dir) + .unwrap_or_else(|err| panic!("reading {} should succeed: {err}", stages_dir.display())) + .filter_map(|entry| entry.ok().map(|entry| entry.path())) + .filter(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| { + name == stage_id || name.split_once('-').is_some_and(|(_, id)| id == stage_id) + }) + }) + .collect(); + matches.sort(); + match matches.as_slice() { + [path] => path.clone(), + [] => panic!( + "stage dump dir for {stage_id} not found in {}", + stages_dir.display() + ), + _ => panic!("stage dump dir for {stage_id} was ambiguous: {matches:?}"), + } +} + /// Find the single run directory for this test context. pub(super) fn find_run_dir(context: &TestContext) -> PathBuf { context.single_run_dir() diff --git a/lib/crates/fabro-client/src/client.rs b/lib/crates/fabro-client/src/client.rs index bb91eb46f..36cd3f489 100644 --- a/lib/crates/fabro-client/src/client.rs +++ b/lib/crates/fabro-client/src/client.rs @@ -1200,6 +1200,7 @@ impl Client { &self, run_id: &RunId, stage_id: &StageId, + retry: u32, filename: &str, ) -> Result> { let response = self @@ -1208,6 +1209,7 @@ impl Client { .get_stage_artifact() .id(run_id.to_string()) .stage_id(stage_id.to_string()) + .retry(retry.cast_signed()) .filename(filename) .send() .await @@ -1248,12 +1250,15 @@ impl Client { &self, run_id: &RunId, stage_id: &StageId, + retry: u32, filename: &str, path: &Path, bearer_token: &str, ) -> Result<()> { let mut url = self.stage_artifacts_url(run_id, stage_id)?; url.query_pairs_mut().append_pair("filename", filename); + url.query_pairs_mut() + .append_pair("retry", &retry.to_string()); let file = File::open(path) .await @@ -1286,11 +1291,14 @@ impl Client { &self, run_id: &RunId, stage_id: &StageId, + retry: u32, artifact_capture_dir: &Path, artifacts: &[ArtifactUpload], bearer_token: &str, ) -> Result<()> { - let url = self.stage_artifacts_url(run_id, stage_id)?; + let mut url = self.stage_artifacts_url(run_id, stage_id)?; + url.query_pairs_mut() + .append_pair("retry", &retry.to_string()); let mut manifest_entries = Vec::with_capacity(artifacts.len()); let mut file_parts = Vec::with_capacity(artifacts.len()); diff --git a/lib/crates/fabro-core/src/lib.rs b/lib/crates/fabro-core/src/lib.rs index 853d4eeaf..acf7117f5 100644 --- a/lib/crates/fabro-core/src/lib.rs +++ b/lib/crates/fabro-core/src/lib.rs @@ -23,7 +23,7 @@ pub use lifecycle::{ }; pub use outcome::{ FailureCategory, FailureDetail, NodeResult, NodeResultExt, Outcome, OutcomeMeta, StageOutcome, - StageState, + StageStatus, }; pub use retry::{BackoffPolicy, RetryPolicy}; pub use stall::{ActivityMonitor, StallGuard, StallWatchdog}; diff --git a/lib/crates/fabro-core/src/outcome.rs b/lib/crates/fabro-core/src/outcome.rs index 53d590fbf..bca933718 100644 --- a/lib/crates/fabro-core/src/outcome.rs +++ b/lib/crates/fabro-core/src/outcome.rs @@ -1,7 +1,7 @@ use std::time::Duration; pub use fabro_types::outcome::{ - FailureCategory, FailureDetail, NodeResult, Outcome, OutcomeMeta, StageOutcome, StageState, + FailureCategory, FailureDetail, NodeResult, Outcome, OutcomeMeta, StageOutcome, StageStatus, }; use crate::error::Error; diff --git a/lib/crates/fabro-dump/src/lib.rs b/lib/crates/fabro-dump/src/lib.rs index d12df2210..16954bfe9 100644 --- a/lib/crates/fabro-dump/src/lib.rs +++ b/lib/crates/fabro-dump/src/lib.rs @@ -13,7 +13,9 @@ use std::path::{Component, Path, PathBuf}; use anyhow::{Context, Result, bail}; use bytes::Bytes; -use fabro_store::{EventEnvelope, RunProjection, SerializableProjection, StageId}; +use fabro_store::{ + EventEnvelope, RunProjection, SerializableProjection, StageId, retry_storage_segment, +}; use fabro_types::{RunBlobId, parse_blob_ref}; use futures::future::BoxFuture; @@ -21,7 +23,8 @@ pub type BlobReader = Box BoxFuture<'static, Result, + entries: Vec, + stage_ranks: HashMap, } #[derive(Debug, Clone)] @@ -47,17 +50,34 @@ impl RunDump { entries.push(RunDumpEntry::text("graph.fabro", graph_source.clone())); } - let mut stage_ids: Vec<_> = state - .iter_nodes() - .map(|(stage_id, _)| stage_id.clone()) + let mut stage_order: Vec<_> = state + .iter_stages() + .map(|(stage_id, stage)| (stage_id.clone(), stage.seq)) .collect(); - stage_ids.sort(); + if stage_order.len() > 999 { + bail!( + "run dump supports at most 999 stages with the current path prefix width (got {})", + stage_order.len() + ); + } + stage_order.sort_by(|(left_id, left_seq), (right_id, right_seq)| { + left_seq.cmp(right_seq).then_with(|| left_id.cmp(right_id)) + }); + let mut stage_ranks = HashMap::new(); + for (index, (stage_id, _)) in stage_order.iter().enumerate() { + let rank = u32::try_from(index + 1).context("stage rank should fit in u32")?; + stage_ranks.insert(stage_id.clone(), rank); + } - for stage_id in stage_ids { - let Some(node) = state.node(&stage_id) else { + for (stage_id, _) in stage_order { + let Some(node) = state.stage(&stage_id) else { continue; }; - let base = PathBuf::from("stages").join(stage_id.to_string()); + let rank = stage_ranks + .get(&stage_id) + .copied() + .context("stage rank should exist")?; + let base = PathBuf::from("stages").join(format!("{rank:03}-{stage_id}")); if let Some(prompt) = node.prompt.as_ref() { entries.push(RunDumpEntry::text_path( @@ -128,7 +148,10 @@ impl RunDump { )); } - Ok(Self { entries }) + Ok(Self { + entries, + stage_ranks, + }) } pub fn from_store_state_and_events( @@ -159,10 +182,14 @@ impl RunDump { pub fn add_artifact_bytes( &mut self, stage_id: &StageId, + retry: u32, filename: &str, data: Vec, ) -> Result<()> { - let path = artifact_dump_path(stage_id, filename)?; + let path = artifact_dump_path(&self.stage_ranks, stage_id, retry, filename)?; + if !self.stage_ranks.contains_key(stage_id) { + self.add_orphan_notice(stage_id); + } self.entries.push(RunDumpEntry::bytes_path(&path, data)); Ok(()) } @@ -223,6 +250,21 @@ impl RunDump { self.entries.len() } + fn add_orphan_notice(&mut self, stage_id: &StageId) { + let line = format!("notice: artifact stage {stage_id} was not present in run projection\n"); + if let Some(entry) = self + .entries + .iter_mut() + .find(|entry| entry.path == "dump.log") + { + if let RunDumpContents::Text(text) = &mut entry.contents { + text.push_str(&line); + return; + } + } + self.entries.push(RunDumpEntry::text("dump.log", line)); + } + pub fn write_to_dir(&self, root: &Path) -> Result { for entry in &self.entries { entry.write_to_dir(root)?; @@ -405,11 +447,21 @@ fn replace_blob_refs_in_value( Ok(()) } -fn artifact_dump_path(stage_id: &StageId, filename: &str) -> Result { +fn artifact_dump_path( + stage_ranks: &HashMap, + stage_id: &StageId, + retry: u32, + filename: &str, +) -> Result { validate_single_path_segment("node id", stage_id.node_id())?; let filename_path = validate_relative_path("artifact filename", filename)?; + let stage_dir = stage_ranks.get(stage_id).map_or_else( + || PathBuf::from("_orphans").join(stage_id.to_string()), + |rank| PathBuf::from(format!("{rank:03}-{stage_id}")), + ); Ok(PathBuf::from("artifacts") - .join(stage_id.to_string()) + .join(stage_dir) + .join(retry_storage_segment(retry)) .join(filename_path)) } @@ -427,7 +479,7 @@ mod tests { use std::collections::HashMap; use chrono::{TimeZone, Utc}; - use fabro_store::{NodeState, RunProjection, StageId}; + use fabro_store::{RunProjection, StageId, StageState}; use fabro_types::graph::Graph; use fabro_types::run::RunSpec; use fabro_types::{ @@ -522,7 +574,8 @@ mod tests { }); projection.retro_prompt = Some("retro prompt".to_string()); projection.retro_response = Some("retro response".to_string()); - projection.set_node(stage_id.clone(), NodeState { + projection.set_stage(stage_id.clone(), StageState { + seq: 1, prompt: Some("plan".to_string()), response: Some("done".to_string()), status: Some(NodeStatusRecord { @@ -559,16 +612,16 @@ mod tests { assert!(paths.contains(&"graph.fabro")); assert!(paths.contains(&"stages/retro/prompt.md")); assert!(paths.contains(&"stages/retro/response.md")); - assert!(paths.contains(&"stages/build@2/prompt.md")); - assert!(paths.contains(&"stages/build@2/response.md")); - assert!(paths.contains(&"stages/build@2/status.json")); - assert!(paths.contains(&"stages/build@2/provider_used.json")); - assert!(paths.contains(&"stages/build@2/diff.patch")); - assert!(paths.contains(&"stages/build@2/script_invocation.json")); - assert!(paths.contains(&"stages/build@2/script_timing.json")); - assert!(paths.contains(&"stages/build@2/parallel_results.json")); - assert!(paths.contains(&"stages/build@2/stdout.log")); - assert!(paths.contains(&"stages/build@2/stderr.log")); + assert!(paths.contains(&"stages/001-build@2/prompt.md")); + assert!(paths.contains(&"stages/001-build@2/response.md")); + assert!(paths.contains(&"stages/001-build@2/status.json")); + assert!(paths.contains(&"stages/001-build@2/provider_used.json")); + assert!(paths.contains(&"stages/001-build@2/diff.patch")); + assert!(paths.contains(&"stages/001-build@2/script_invocation.json")); + assert!(paths.contains(&"stages/001-build@2/script_timing.json")); + assert!(paths.contains(&"stages/001-build@2/parallel_results.json")); + assert!(paths.contains(&"stages/001-build@2/stdout.log")); + assert!(paths.contains(&"stages/001-build@2/stderr.log")); assert!(!paths.contains(&"start.json")); assert!(!paths.contains(&"status.json")); assert!(!paths.contains(&"checkpoint.json")); @@ -585,7 +638,7 @@ mod tests { panic!("run.json should be json"); }; let round_tripped: RunProjection = serde_json::from_value(value.clone()).unwrap(); - let node = round_tripped.node(&stage_id).expect("node should exist"); + let node = round_tripped.stage(&stage_id).expect("node should exist"); assert!(round_tripped.spec.is_some()); assert!(round_tripped.start.is_some()); @@ -593,6 +646,7 @@ mod tests { assert!(round_tripped.checkpoint.is_some()); assert!(round_tripped.conclusion.is_some()); assert!(round_tripped.sandbox.is_some()); + assert_eq!(node.seq, 1); assert_eq!(node.prompt, None); assert_eq!(node.response, None); assert_eq!(node.diff, None); @@ -604,16 +658,132 @@ mod tests { ); } + #[test] + fn from_projection_orders_stage_dirs_by_seq() { + let mut projection = RunProjection::default(); + projection.set_stage(StageId::new("zebra", 1), StageState { + seq: 2, + prompt: Some("zebra".to_string()), + ..StageState::default() + }); + projection.set_stage(StageId::new("apple", 1), StageState { + seq: 5, + prompt: Some("apple".to_string()), + ..StageState::default() + }); + + let dump = RunDump::from_projection(&projection).unwrap(); + let stage_prompt_paths = dump + .entries() + .iter() + .filter_map(|entry| { + entry + .path + .ends_with("prompt.md") + .then_some(entry.path.as_str()) + }) + .collect::>(); + + assert_eq!(stage_prompt_paths, vec![ + "stages/001-zebra@1/prompt.md", + "stages/002-apple@1/prompt.md", + ]); + } + + #[test] + fn from_projection_reads_legacy_nodes_alias_and_ties_seq_zero_by_stage_id() { + let projection: RunProjection = serde_json::from_value(serde_json::json!({ + "nodes": { + "zebra@1": { "prompt": "zebra" }, + "apple@1": { "prompt": "apple" } + } + })) + .unwrap(); + + let dump = RunDump::from_projection(&projection).unwrap(); + let stage_prompt_paths = dump + .entries() + .iter() + .filter_map(|entry| { + entry + .path + .ends_with("prompt.md") + .then_some(entry.path.as_str()) + }) + .collect::>(); + + assert_eq!(stage_prompt_paths, vec![ + "stages/001-apple@1/prompt.md", + "stages/002-zebra@1/prompt.md", + ]); + let serialized = serde_json::to_value(&projection).unwrap(); + assert!(serialized.get("stages").is_some()); + assert!(serialized.get("nodes").is_none()); + } + + #[test] + fn add_artifact_bytes_uses_stage_rank_and_retry() { + let stage_id = StageId::new("build", 1); + let mut projection = RunProjection::default(); + projection.set_stage(stage_id.clone(), StageState { + seq: 7, + ..StageState::default() + }); + let mut dump = RunDump::from_projection(&projection).unwrap(); + + dump.add_artifact_bytes(&stage_id, 1, "logs/output.txt", b"first".to_vec()) + .unwrap(); + dump.add_artifact_bytes(&stage_id, 2, "logs/output.txt", b"second".to_vec()) + .unwrap(); + + let paths = dump + .entries() + .iter() + .map(|entry| entry.path.as_str()) + .collect::>(); + assert!(paths.contains(&"artifacts/001-build@1/retry-0001/logs/output.txt")); + assert!(paths.contains(&"artifacts/001-build@1/retry-0002/logs/output.txt")); + } + + #[test] + fn add_artifact_bytes_places_orphans_under_sentinel() { + let mut dump = RunDump::from_projection(&RunProjection::default()).unwrap(); + + dump.add_artifact_bytes( + &StageId::new("missing", 1), + 1, + "logs/output.txt", + b"orphan".to_vec(), + ) + .unwrap(); + + let orphan = dump + .entries() + .iter() + .find(|entry| entry.path == "artifacts/_orphans/missing@1/retry-0001/logs/output.txt"); + assert!(orphan.is_some()); + let notice = dump + .entries() + .iter() + .find(|entry| entry.path == "dump.log") + .expect("dump log should include orphan notice"); + let RunDumpContents::Text(text) = ¬ice.contents else { + panic!("dump log should be text"); + }; + assert!(text.contains("missing@1")); + } + #[test] fn hydrate_referenced_blobs_ignores_legacy_artifact_file_refs() { let blob = serde_json::to_vec("hydrated legacy text").unwrap(); let blob_id = fabro_types::RunBlobId::new(&blob); let legacy_ref = format!("file:///sandbox/.fabro/artifacts/{blob_id}.json"); let mut dump = RunDump { - entries: vec![RunDumpEntry::json( + entries: vec![RunDumpEntry::json( "run.json", serde_json::json!({ "stdout": legacy_ref }), )], + stage_ranks: HashMap::new(), }; executor::block_on(async { diff --git a/lib/crates/fabro-retro/src/retro_agent.rs b/lib/crates/fabro-retro/src/retro_agent.rs index bc72467f4..a98275c66 100644 --- a/lib/crates/fabro-retro/src/retro_agent.rs +++ b/lib/crates/fabro-retro/src/retro_agent.rs @@ -334,7 +334,7 @@ mod tests { use chrono::{TimeZone, Utc}; use fabro_agent::LocalSandbox; - use fabro_store::{NodeState, StageId}; + use fabro_store::{StageId, StageState}; use fabro_types::{NodeStatusRecord, StageOutcome}; use tokio::fs; @@ -410,7 +410,8 @@ mod tests { let stage_id = StageId::new("build", 2); let mut state = RunProjection::default(); state.graph_source = Some("digraph Ship {}".to_string()); - state.set_node(stage_id, NodeState { + state.set_stage(stage_id, StageState { + seq: 1, prompt: Some("plan".to_string()), response: Some("done".to_string()), status: Some(NodeStatusRecord { @@ -455,8 +456,8 @@ mod tests { .expect("run.json should parse"); assert!(run_json.get("spec").is_some()); assert!(run_json.get("run").is_none()); - assert!(run_json["nodes"]["build@2"]["prompt"].is_null()); - assert!(run_json["nodes"]["build@2"]["diff"].is_null()); + assert!(run_json["stages"]["build@2"]["prompt"].is_null()); + assert!(run_json["stages"]["build@2"]["diff"].is_null()); assert_eq!( fs::read_to_string(target_dir.join("graph.fabro")) .await @@ -464,19 +465,19 @@ mod tests { "digraph Ship {}" ); assert_eq!( - fs::read_to_string(target_dir.join("stages/build@2/prompt.md")) + fs::read_to_string(target_dir.join("stages/001-build@2/prompt.md")) .await .expect("prompt file should exist"), "plan" ); assert_eq!( - fs::read_to_string(target_dir.join("stages/build@2/response.md")) + fs::read_to_string(target_dir.join("stages/001-build@2/response.md")) .await .expect("response file should exist"), "done" ); assert_eq!( - fs::read_to_string(target_dir.join("stages/build@2/stdout.log")) + fs::read_to_string(target_dir.join("stages/001-build@2/stdout.log")) .await .expect("stdout file should exist"), "stdout" @@ -494,7 +495,7 @@ mod tests { "server log\n" ); assert!( - target_dir.join("stages/build@2/status.json").exists(), + target_dir.join("stages/001-build@2/status.json").exists(), "status file should exist" ); assert!( @@ -521,7 +522,7 @@ mod tests { let mut state = RunProjection::default(); let stdout_ref = fabro_types::format_blob_ref(&stdout_id); let stderr_ref = fabro_types::format_blob_ref(&stderr_id); - state.set_node(stage_id, NodeState { + state.set_stage(stage_id, StageState { script_invocation: Some(serde_json::json!({ "command": "cargo test", "stdout": stdout_ref, @@ -534,7 +535,7 @@ mod tests { })), stdout: Some(stdout_ref), stderr: Some(stderr_ref), - ..NodeState::default() + ..StageState::default() }); let reader: BlobReader = Box::new(move |blob_id| { @@ -556,20 +557,20 @@ mod tests { .expect("retro files should upload"); assert_eq!( - fs::read_to_string(target_dir.join("stages/build@1/stdout.log")) + fs::read_to_string(target_dir.join("stages/001-build@1/stdout.log")) .await .expect("stdout file should exist"), "resolved stdout" ); assert_eq!( - fs::read_to_string(target_dir.join("stages/build@1/stderr.log")) + fs::read_to_string(target_dir.join("stages/001-build@1/stderr.log")) .await .expect("stderr file should exist"), "resolved stderr" ); let script_timing: serde_json::Value = serde_json::from_str( - &fs::read_to_string(target_dir.join("stages/build@1/script_timing.json")) + &fs::read_to_string(target_dir.join("stages/001-build@1/script_timing.json")) .await .expect("script timing should exist"), ) @@ -578,7 +579,7 @@ mod tests { assert_eq!(script_timing["stderr"], "resolved stderr"); let script_invocation: serde_json::Value = serde_json::from_str( - &fs::read_to_string(target_dir.join("stages/build@1/script_invocation.json")) + &fs::read_to_string(target_dir.join("stages/001-build@1/script_invocation.json")) .await .expect("script invocation should exist"), ) @@ -593,18 +594,18 @@ mod tests { ) .expect("run.json should parse"); assert_eq!( - run_json["nodes"]["build@1"]["script_timing"]["stdout"], + run_json["stages"]["build@1"]["script_timing"]["stdout"], "resolved stdout" ); assert_eq!( - run_json["nodes"]["build@1"]["script_timing"]["stderr"], + run_json["stages"]["build@1"]["script_timing"]["stderr"], "resolved stderr" ); assert_eq!( - run_json["nodes"]["build@1"]["script_invocation"]["stdout"], + run_json["stages"]["build@1"]["script_invocation"]["stdout"], "resolved stdout" ); - assert!(run_json["nodes"]["build@1"]["stdout"].is_null()); - assert!(run_json["nodes"]["build@1"]["stderr"].is_null()); + assert!(run_json["stages"]["build@1"]["stdout"].is_null()); + assert!(run_json["stages"]["build@1"]["stderr"].is_null()); } } diff --git a/lib/crates/fabro-server/src/demo/mod.rs b/lib/crates/fabro-server/src/demo/mod.rs index b1a56658f..38cea6acb 100644 --- a/lib/crates/fabro-server/src/demo/mod.rs +++ b/lib/crates/fabro-server/src/demo/mod.rs @@ -1158,28 +1158,28 @@ mod runs { RunStage { id: "detect-drift".into(), name: "Detect Drift".into(), - status: StageState::Succeeded, + status: StageStatus::Succeeded, duration_secs: Some(72.0), dot_id: Some("detect".into()), }, RunStage { id: "propose-changes".into(), name: "Propose Changes".into(), - status: StageState::Succeeded, + status: StageStatus::Succeeded, duration_secs: Some(154.0), dot_id: Some("propose".into()), }, RunStage { id: "review-changes".into(), name: "Review Changes".into(), - status: StageState::Succeeded, + status: StageStatus::Succeeded, duration_secs: Some(45.0), dot_id: Some("review".into()), }, RunStage { id: "apply-changes".into(), name: "Apply Changes".into(), - status: StageState::Running, + status: StageStatus::Running, duration_secs: Some(118.0), dot_id: Some("apply".into()), }, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 72bdc8cb2..9db317480 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -34,7 +34,7 @@ pub use fabro_api::types::{ PruneRunsRequest, PruneRunsResponse, RenderWorkflowGraphDirection, RenderWorkflowGraphRequest, RewindRequest, RewindResponse, RunArtifactEntry, RunArtifactListResponse, RunBilling, RunBillingStage, RunBillingTotals, RunError, RunManifest, RunStage, RunStatusResponse, - SandboxFileEntry, SandboxFileListResponse, SshAccessRequest, SshAccessResponse, StageState, + SandboxFileEntry, SandboxFileListResponse, SshAccessRequest, SshAccessResponse, StageStatus, StartRunRequest, SubmitAnswerRequest, SystemFeatures, SystemInfoResponse, SystemRunCounts, TimelineEntryResponse, WriteBlobResponse, }; @@ -63,7 +63,8 @@ use fabro_slack::threads::ThreadRegistry; use fabro_slack::{blocks as slack_blocks, connection as slack_connection}; use fabro_static::EnvVars; use fabro_store::{ - ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, StageId, + ArtifactKey, ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, + StageId, }; #[cfg(test)] use fabro_types::BlockedReason; @@ -222,6 +223,8 @@ struct GlobalAttachParams { struct ArtifactFilenameParams { #[serde(default)] filename: Option, + #[serde(default)] + retry: Option, } #[derive(serde::Deserialize)] @@ -2170,7 +2173,7 @@ async fn openapi_spec() -> Response { Json(value).into_response() } -fn active_stage_state_from_events(events: &[EventEnvelope], node_id: &str) -> StageState { +fn active_stage_state_from_events(events: &[EventEnvelope], node_id: &str) -> StageStatus { let latest = events.iter().rev().find(|envelope| { envelope.event.node_id.as_deref() == Some(node_id) && matches!( @@ -2180,9 +2183,9 @@ fn active_stage_state_from_events(events: &[EventEnvelope], node_id: &str) -> St }); if latest.is_some_and(|e| e.event.event_name() == "stage.retrying") { - StageState::Retrying + StageStatus::Retrying } else { - StageState::Running + StageStatus::Running } } @@ -2296,8 +2299,8 @@ async fn list_run_stages( for node_id in &checkpoint.completed_nodes { let duration_ms = stage_durations.get(node_id).copied().unwrap_or(0); let status = match checkpoint.node_outcomes.get(node_id) { - Some(outcome) => StageState::from(outcome.status), - None => StageState::Succeeded, + Some(outcome) => StageStatus::from(outcome.status), + None => StageStatus::Succeeded, }; stages.push(RunStage { id: node_id.clone(), @@ -3352,13 +3355,24 @@ pub(crate) fn parse_blob_id_path(blob_id: &str) -> Result { clippy::result_large_err, reason = "Missing filename validation returns HTTP 400 responses directly." )] -fn required_filename(params: ArtifactFilenameParams) -> Result { - match params.filename { - Some(filename) if !filename.is_empty() => Ok(filename), +fn required_filename(params: &ArtifactFilenameParams) -> Result { + match params.filename.as_ref() { + Some(filename) if !filename.is_empty() => Ok(filename.clone()), _ => Err(ApiError::bad_request("Missing filename query parameter.").into_response()), } } +#[allow( + clippy::result_large_err, + reason = "Missing retry validation returns HTTP 400 responses directly." +)] +fn required_retry(params: &ArtifactFilenameParams) -> Result { + match params.retry { + Some(retry) => Ok(retry), + None => Err(ApiError::bad_request("Missing retry query parameter.").into_response()), + } +} + #[allow( clippy::result_large_err, reason = "Artifact path validation returns HTTP 400 responses directly." @@ -5366,7 +5380,7 @@ async fn get_run_stage_command_log( .into_response(); } }; - let Some(node) = run_state.node(&stage_id) else { + let Some(node) = run_state.stage(&stage_id) else { return ApiError::not_found("Stage not found.").into_response(); }; @@ -6182,7 +6196,7 @@ async fn list_run_artifacts( .map(|entry| RunArtifactEntry { stage_id: entry.node.to_string(), node_slug: entry.node.node_id().to_string(), - retry: entry.node.visit().cast_signed(), + retry: entry.retry.cast_signed(), relative_path: entry.filename, size: entry.size.cast_signed(), }) @@ -6213,10 +6227,14 @@ async fn list_stage_artifacts( } match state.artifact_store.list_for_node(&id, &stage_id).await { - Ok(filenames) => Json(ArtifactListResponse { - data: filenames + Ok(entries) => Json(ArtifactListResponse { + data: entries .into_iter() - .map(|filename| ArtifactEntry { filename }) + .map(|entry| ArtifactEntry { + filename: entry.filename, + retry: entry.retry.cast_signed(), + size: entry.size.cast_signed(), + }) .collect(), }) .into_response(), @@ -6398,6 +6416,7 @@ async fn upload_stage_artifact_octet_stream( state: &AppState, run_id: &RunId, stage_id: &StageId, + retry: u32, filename: String, body: Body, content_length: Option, @@ -6413,10 +6432,10 @@ async fn upload_stage_artifact_octet_stream( )); } - let mut writer = match state - .artifact_store - .writer(run_id, stage_id, &relative_path) - { + let mut writer = match state.artifact_store.writer( + run_id, + &ArtifactKey::new(stage_id.clone(), retry, relative_path), + ) { Ok(writer) => writer, Err(err) => { return ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) @@ -6458,6 +6477,7 @@ async fn upload_stage_artifact_multipart( state: &AppState, run_id: &RunId, stage_id: &StageId, + retry: u32, boundary: String, body: Body, ) -> Response { @@ -6503,7 +6523,10 @@ async fn upload_stage_artifact_multipart( return bad_request_response(format!("unexpected multipart part: {part_name}")); }; - let mut writer = match state.artifact_store.writer(run_id, stage_id, &entry.path) { + let mut writer = match state.artifact_store.writer( + run_id, + &ArtifactKey::new(stage_id.clone(), retry, entry.path.clone()), + ) { Ok(writer) => writer, Err(err) => { return ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) @@ -6591,6 +6614,10 @@ async fn put_stage_artifact( if let Err(response) = load_run_spec(state.as_ref(), &id).await.map(|_| ()) { return response; } + let retry = match required_retry(¶ms) { + Ok(retry) => retry, + Err(response) => return response, + }; let content_length = match content_length_from_headers(&parts.headers) { Ok(length) => length, @@ -6598,7 +6625,7 @@ async fn put_stage_artifact( }; match artifact_upload_content_type(&parts.headers) { Ok(ArtifactUploadContentType::OctetStream) => { - let filename = match required_filename(params) { + let filename = match required_filename(¶ms) { Ok(filename) => filename, Err(response) => return response, }; @@ -6606,6 +6633,7 @@ async fn put_stage_artifact( state.as_ref(), &id, &stage_id, + retry, filename, body, content_length, @@ -6618,7 +6646,8 @@ async fn put_stage_artifact( "multipart upload exceeds the {MAX_MULTIPART_REQUEST_BYTES} byte limit" )); } - upload_stage_artifact_multipart(state.as_ref(), &id, &stage_id, boundary, body).await + upload_stage_artifact_multipart(state.as_ref(), &id, &stage_id, retry, boundary, body) + .await } Err(response) => response, } @@ -6638,10 +6667,14 @@ async fn get_stage_artifact( Ok(stage_id) => stage_id, Err(response) => return response, }; - let filename = match required_filename(params) { + let filename = match required_filename(¶ms) { Ok(filename) => filename, Err(response) => return response, }; + let retry = match required_retry(¶ms) { + Ok(retry) => retry, + Err(response) => return response, + }; let relative_path = match validate_relative_artifact_path("filename", &filename) { Ok(path) => path, Err(response) => return response, @@ -6652,7 +6685,10 @@ async fn get_stage_artifact( match state .artifact_store - .get(&id, &stage_id, &relative_path) + .get( + &id, + &ArtifactKey::new(stage_id.clone(), retry, relative_path), + ) .await { Ok(Some(bytes)) => octet_stream_response(bytes), @@ -10890,7 +10926,7 @@ slug = "fabro" let response = app.oneshot(req).await.unwrap(); let body = response_json!(response, StatusCode::OK).await; - assert!(body["nodes"].is_object()); + assert!(body["stages"].is_object()); } #[tokio::test] @@ -12292,7 +12328,7 @@ slug = "fabro" let req = Request::builder() .method("POST") .uri(api(&format!( - "/runs/{run_id}/stages/{stage_id}/artifacts?filename=src/lib.rs" + "/runs/{run_id}/stages/{stage_id}/artifacts?filename=src/lib.rs&retry=1" ))) .header("content-type", "application/octet-stream") .body(Body::from("fn main() {}")) @@ -12308,6 +12344,8 @@ slug = "fabro" let response = app.clone().oneshot(req).await.unwrap(); let body = response_json!(response, StatusCode::OK).await; assert_eq!(body["data"][0]["filename"], "src/lib.rs"); + assert_eq!(body["data"][0]["retry"], 1); + assert_eq!(body["data"][0]["size"], 12); let req = Request::builder() .method("GET") @@ -12316,11 +12354,66 @@ slug = "fabro" ))) .body(Body::empty()) .unwrap(); + let response = app.clone().oneshot(req).await.unwrap(); + assert_status!(response, StatusCode::BAD_REQUEST).await; + + let req = Request::builder() + .method("GET") + .uri(api(&format!( + "/runs/{run_id}/stages/{stage_id}/artifacts/download?filename=src/lib.rs&retry=1" + ))) + .body(Body::empty()) + .unwrap(); let response = app.oneshot(req).await.unwrap(); let bytes = response_bytes!(response, StatusCode::OK).await; assert_eq!(&bytes[..], b"fn main() {}"); } + #[tokio::test] + async fn stage_artifacts_keep_same_filename_per_retry() { + let state = create_app_state(); + let app = build_router(Arc::clone(&state), AuthMode::Disabled); + + let run_id = create_run(&app, MINIMAL_DOT).await; + let stage_id = "code@2"; + + for (retry, body) in [(1, "first"), (2, "second")] { + let req = Request::builder() + .method("POST") + .uri(api(&format!( + "/runs/{run_id}/stages/{stage_id}/artifacts?filename=logs/output.txt&retry={retry}" + ))) + .header("content-type", "application/octet-stream") + .body(Body::from(body)) + .unwrap(); + let response = app.clone().oneshot(req).await.unwrap(); + assert_status!(response, StatusCode::NO_CONTENT).await; + } + + let req = Request::builder() + .method("GET") + .uri(api(&format!("/runs/{run_id}/stages/{stage_id}/artifacts"))) + .body(Body::empty()) + .unwrap(); + let response = app.clone().oneshot(req).await.unwrap(); + let body = response_json!(response, StatusCode::OK).await; + assert_eq!(body["data"][0]["filename"], "logs/output.txt"); + assert_eq!(body["data"][0]["retry"], 1); + assert_eq!(body["data"][1]["filename"], "logs/output.txt"); + assert_eq!(body["data"][1]["retry"], 2); + + let req = Request::builder() + .method("GET") + .uri(api(&format!( + "/runs/{run_id}/stages/{stage_id}/artifacts/download?filename=logs/output.txt&retry=2" + ))) + .body(Body::empty()) + .unwrap(); + let response = app.oneshot(req).await.unwrap(); + let bytes = response_bytes!(response, StatusCode::OK).await; + assert_eq!(&bytes[..], b"second"); + } + #[tokio::test] async fn create_run_persists_run_spec() { let state = create_app_state(); @@ -12352,7 +12445,7 @@ slug = "fabro" let req = Request::builder() .method("POST") .uri(api(&format!( - "/runs/{run_id}/stages/code@2/artifacts?filename=../escape.txt" + "/runs/{run_id}/stages/code@2/artifacts?filename=../escape.txt&retry=1" ))) .header("content-type", "application/octet-stream") .body(Body::from("nope")) @@ -12493,7 +12586,7 @@ slug = "fabro" Request::builder() .method(Method::POST) .uri(api(&format!( - "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt" + "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt&retry=1" ))) .header(header::AUTHORIZATION, format!("Bearer {worker_token}")) .header(header::CONTENT_TYPE, "application/octet-stream") @@ -12510,7 +12603,7 @@ slug = "fabro" Request::builder() .method(Method::POST) .uri(api(&format!( - "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt" + "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt&retry=1" ))) .header(header::AUTHORIZATION, format!("Bearer {user_jwt}")) .header(header::CONTENT_TYPE, "application/octet-stream") @@ -12527,7 +12620,7 @@ slug = "fabro" Request::builder() .method(Method::POST) .uri(api(&format!( - "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt" + "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt&retry=1" ))) .header( header::AUTHORIZATION, @@ -12546,7 +12639,7 @@ slug = "fabro" Request::builder() .method(Method::POST) .uri(api(&format!( - "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt" + "/runs/{run_id}/stages/code@2/artifacts?filename=artifact.txt&retry=1" ))) .header(header::CONTENT_TYPE, "application/octet-stream") .body(Body::from("artifact")) @@ -12744,7 +12837,9 @@ slug = "fabro" let req = Request::builder() .method("POST") - .uri(api(&format!("/runs/{run_id}/stages/{stage_id}/artifacts"))) + .uri(api(&format!( + "/runs/{run_id}/stages/{stage_id}/artifacts?retry=1" + ))) .header( "content-type", format!("multipart/form-data; boundary={boundary}"), @@ -12765,12 +12860,16 @@ slug = "fabro" let response = app.clone().oneshot(req).await.unwrap(); let body = response_json!(response, StatusCode::OK).await; assert_eq!(body["data"][0]["filename"], "logs/output.txt"); + assert_eq!(body["data"][0]["retry"], 1); + assert_eq!(body["data"][0]["size"], log_bytes.len()); assert_eq!(body["data"][1]["filename"], "src/lib.rs"); + assert_eq!(body["data"][1]["retry"], 1); + assert_eq!(body["data"][1]["size"], source_bytes.len()); let req = Request::builder() .method("GET") .uri(api(&format!( - "/runs/{run_id}/stages/{stage_id}/artifacts/download?filename=logs/output.txt" + "/runs/{run_id}/stages/{stage_id}/artifacts/download?filename=logs/output.txt&retry=1" ))) .body(Body::empty()) .unwrap(); @@ -12792,7 +12891,9 @@ slug = "fabro" let req = Request::builder() .method("POST") - .uri(api(&format!("/runs/{run_id}/stages/code@2/artifacts"))) + .uri(api(&format!( + "/runs/{run_id}/stages/code@2/artifacts?retry=1" + ))) .header( "content-type", format!("multipart/form-data; boundary={boundary}"), diff --git a/lib/crates/fabro-server/tests/it/scenario/archive.rs b/lib/crates/fabro-server/tests/it/scenario/archive.rs index fd7a431d8..5a0c60bdf 100644 --- a/lib/crates/fabro-server/tests/it/scenario/archive.rs +++ b/lib/crates/fabro-server/tests/it/scenario/archive.rs @@ -93,7 +93,7 @@ async fn archived_runs_reject_mutations_with_actionable_body() { ), ( "POST", - format!("/runs/{run_id}/stages/fake@1/artifacts?filename=smoke.txt"), + format!("/runs/{run_id}/stages/fake@1/artifacts?filename=smoke.txt&retry=1"), "payload", "application/octet-stream", ), diff --git a/lib/crates/fabro-store/src/artifact_store.rs b/lib/crates/fabro-store/src/artifact_store.rs index d09bfcfdd..8cc244131 100644 --- a/lib/crates/fabro-store/src/artifact_store.rs +++ b/lib/crates/fabro-store/src/artifact_store.rs @@ -16,9 +16,35 @@ const ARTIFACT_SEGMENT_ENCODE_SET: &AsciiSet = &NON_ALPHANUMERIC.remove(b'.').remove(b'_').remove(b'-'); const STREAM_BUFFER_BYTES: usize = 1024 * 1024; +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct ArtifactKey { + pub stage_id: StageId, + pub retry: u32, + pub relative_path: String, +} + +impl ArtifactKey { + #[must_use] + pub fn new(stage_id: StageId, retry: u32, relative_path: impl Into) -> Self { + Self { + stage_id, + retry, + relative_path: relative_path.into(), + } + } +} + #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct NodeArtifact { pub node: StageId, + pub retry: u32, + pub filename: String, + pub size: u64, +} + +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct StageArtifactEntry { + pub retry: u32, pub filename: String, pub size: u64, } @@ -46,22 +72,16 @@ impl ArtifactStore { } } - pub async fn put( - &self, - run_id: &RunId, - node: &StageId, - filename: &str, - data: &[u8], - ) -> Result<()> { - let path = self.artifact_path(run_id, node, filename)?; + pub async fn put(&self, run_id: &RunId, key: &ArtifactKey, data: &[u8]) -> Result<()> { + let path = self.artifact_path(run_id, key)?; self.object_store .put(&path, Bytes::copy_from_slice(data).into()) .await?; Ok(()) } - pub fn writer(&self, run_id: &RunId, node: &StageId, filename: &str) -> Result { - let path = self.artifact_path(run_id, node, filename)?; + pub fn writer(&self, run_id: &RunId, key: &ArtifactKey) -> Result { + let path = self.artifact_path(run_id, key)?; Ok(BufWriter::with_capacity( Arc::clone(&self.object_store), path, @@ -72,14 +92,13 @@ impl ArtifactStore { pub async fn put_stream( &self, run_id: &RunId, - node: &StageId, - filename: &str, + key: &ArtifactKey, mut stream: S, ) -> Result<()> where S: futures::Stream> + Unpin, { - let mut writer = self.writer(run_id, node, filename)?; + let mut writer = self.writer(run_id, key)?; while let Some(chunk) = stream.next().await { let chunk = chunk?; writer @@ -94,13 +113,8 @@ impl ArtifactStore { Ok(()) } - pub async fn get( - &self, - run_id: &RunId, - node: &StageId, - filename: &str, - ) -> Result> { - let path = self.artifact_path(run_id, node, filename)?; + pub async fn get(&self, run_id: &RunId, key: &ArtifactKey) -> Result> { + let path = self.artifact_path(run_id, key)?; match self.object_store.get(&path).await { Ok(result) => Ok(Some(result.bytes().await?)), Err(object_store::Error::NotFound { .. }) => Ok(None), @@ -123,15 +137,23 @@ impl ArtifactStore { Ok(artifacts) } - pub async fn list_for_node(&self, run_id: &RunId, node: &StageId) -> Result> { + pub async fn list_for_node( + &self, + run_id: &RunId, + node: &StageId, + ) -> Result> { let prefix = self.node_prefix(run_id, node)?; let mut stream = self.object_store.list(Some(&prefix)); - let mut filenames = Vec::new(); + let mut entries = Vec::new(); while let Some(meta) = stream.next().await.transpose()? { - filenames.push(decode_filename(&prefix, &meta.location)?); + entries.push(decode_stage_artifact_entry( + &prefix, + &meta.location, + meta.size, + )?); } - filenames.sort(); - Ok(filenames) + entries.sort(); + Ok(entries) } pub async fn delete_for_run(&self, run_id: &RunId) -> Result<()> { @@ -171,9 +193,18 @@ impl ArtifactStore { ) } - fn artifact_path(&self, run_id: &RunId, node: &StageId, filename: &str) -> Result { + fn retry_prefix(&self, run_id: &RunId, node: &StageId, retry: u32) -> Result { let mut raw = self.node_prefix(run_id, node)?.to_string(); - for segment in validate_filename_segments(filename)? { + raw.push('/'); + raw.push_str(&retry_storage_segment(retry)); + parse_object_path(&raw) + } + + fn artifact_path(&self, run_id: &RunId, key: &ArtifactKey) -> Result { + let mut raw = self + .retry_prefix(run_id, &key.stage_id, key.retry)? + .to_string(); + for segment in validate_filename_segments(&key.relative_path)? { raw.push('/'); raw.push_str(&encode_path_segment(segment)); } @@ -225,6 +256,11 @@ pub fn stage_storage_segment(node: &StageId) -> String { ) } +#[must_use] +pub fn retry_storage_segment(retry: u32) -> String { + format!("retry-{retry:04}") +} + fn decode_path_segment(kind: &str, value: &str) -> Result { percent_decode_str(value) .decode_utf8() @@ -258,6 +294,12 @@ fn decode_artifact_location( "artifact location {location} has an invalid visit number: {err}" )) })?; + let retry_part = parts.next().ok_or_else(|| { + Error::Other(format!( + "artifact location {location} is missing a retry segment" + )) + })?; + let retry = decode_retry_segment(location, retry_part.as_ref())?; let filename_segments = parts .map(|part| decode_path_segment("artifact filename segment", part.as_ref())) .collect::>>()?; @@ -268,17 +310,28 @@ fn decode_artifact_location( } Ok(NodeArtifact { node: StageId::new(node_id, visit), + retry, filename: filename_segments.join("/"), size, }) } -fn decode_filename(prefix: &ObjectPath, location: &ObjectPath) -> Result { +fn decode_stage_artifact_entry( + prefix: &ObjectPath, + location: &ObjectPath, + size: u64, +) -> Result { let mut parts = location.prefix_match(prefix).ok_or_else(|| { Error::Other(format!( "artifact location {location} does not match expected prefix {prefix}" )) })?; + let retry_part = parts.next().ok_or_else(|| { + Error::Other(format!( + "artifact location {location} is missing a retry segment" + )) + })?; + let retry = decode_retry_segment(location, retry_part.as_ref())?; let filename_segments = parts .by_ref() .map(|part| decode_path_segment("artifact filename segment", part.as_ref())) @@ -288,7 +341,24 @@ fn decode_filename(prefix: &ObjectPath, location: &ObjectPath) -> Result "artifact location {location} is missing a filename" ))); } - Ok(filename_segments.join("/")) + Ok(StageArtifactEntry { + retry, + filename: filename_segments.join("/"), + size, + }) +} + +fn decode_retry_segment(location: &ObjectPath, segment: &str) -> Result { + let Some(value) = segment.strip_prefix("retry-") else { + return Err(Error::Other(format!( + "artifact location {location} has an invalid retry segment" + ))); + }; + value.parse::().map_err(|err| { + Error::Other(format!( + "artifact location {location} has an invalid retry number: {err}" + )) + }) } fn parse_object_path(raw: &str) -> Result { @@ -334,19 +404,25 @@ mod tests { let run_id = fixtures::RUN_1; let node = StageId::new("build/naive @ alpha/π", 12); let filename = "logs/unicode/naive file ☃.txt"; + let key = ArtifactKey::new(node.clone(), 3, filename); - store.put(&run_id, &node, filename, b"hello").await.unwrap(); + store.put(&run_id, &key, b"hello").await.unwrap(); assert_eq!( - store.get(&run_id, &node, filename).await.unwrap(), + store.get(&run_id, &key).await.unwrap(), Some(Bytes::from_static(b"hello")) ); assert_eq!(store.list_for_node(&run_id, &node).await.unwrap(), vec![ - filename.to_string() + StageArtifactEntry { + retry: 3, + filename: filename.to_string(), + size: 5, + } ]); assert_eq!(store.list_for_run(&run_id).await.unwrap(), vec![ NodeArtifact { node, + retry: 3, filename: filename.to_string(), size: 5, } @@ -359,12 +435,12 @@ mod tests { let run_id = fixtures::RUN_1; let node = StageId::new("build", 2); let filename = "logs/output.txt"; + let key = ArtifactKey::new(node, 1, filename); store .put_stream( &run_id, - &node, - filename, + &key, stream::iter(vec![ Ok(Bytes::from_static(b"hello ")), Ok(Bytes::from_static(b"world")), @@ -374,7 +450,7 @@ mod tests { .unwrap(); assert_eq!( - store.get(&run_id, &node, filename).await.unwrap(), + store.get(&run_id, &key).await.unwrap(), Some(Bytes::from_static(b"hello world")) ); } @@ -392,10 +468,8 @@ mod tests { "logs/./output.txt", r"logs\output.txt", ] { - let err = store - .put(&run_id, &node, filename, b"boom") - .await - .unwrap_err(); + let key = ArtifactKey::new(node.clone(), 1, filename); + let err = store.put(&run_id, &key, b"boom").await.unwrap_err(); assert!(err.to_string().contains("artifact filename")); } } @@ -407,13 +481,24 @@ mod tests { let other_run_id = fixtures::RUN_2; let node = StageId::new("build", 1); - store.put(&run_id, &node, "a.txt", b"a").await.unwrap(); store - .put(&run_id, &node, "nested/b.txt", b"b") + .put(&run_id, &ArtifactKey::new(node.clone(), 1, "a.txt"), b"a") .await .unwrap(); store - .put(&other_run_id, &node, "keep.txt", b"keep") + .put( + &run_id, + &ArtifactKey::new(node.clone(), 1, "nested/b.txt"), + b"b", + ) + .await + .unwrap(); + store + .put( + &other_run_id, + &ArtifactKey::new(node.clone(), 1, "keep.txt"), + b"keep", + ) .await .unwrap(); @@ -422,7 +507,77 @@ mod tests { assert!(store.list_for_run(&run_id).await.unwrap().is_empty()); assert_eq!( store.list_for_node(&other_run_id, &node).await.unwrap(), - vec!["keep.txt".to_string()] + vec![StageArtifactEntry { + retry: 1, + filename: "keep.txt".to_string(), + size: 4, + }] ); } + + #[tokio::test] + async fn preserves_same_filename_across_retries() { + let store = test_store(); + let run_id = fixtures::RUN_1; + let node = StageId::new("build", 1); + let first = ArtifactKey::new(node.clone(), 1, "logs/output.txt"); + let second = ArtifactKey::new(node.clone(), 2, "logs/output.txt"); + + store.put(&run_id, &first, b"first").await.unwrap(); + store.put(&run_id, &second, b"second").await.unwrap(); + + assert_eq!( + store.get(&run_id, &first).await.unwrap(), + Some(Bytes::from_static(b"first")) + ); + assert_eq!( + store.get(&run_id, &second).await.unwrap(), + Some(Bytes::from_static(b"second")) + ); + assert_eq!(store.list_for_node(&run_id, &node).await.unwrap(), vec![ + StageArtifactEntry { + retry: 1, + filename: "logs/output.txt".to_string(), + size: 5, + }, + StageArtifactEntry { + retry: 2, + filename: "logs/output.txt".to_string(), + size: 6, + }, + ]); + assert_eq!(store.list_for_run(&run_id).await.unwrap(), vec![ + NodeArtifact { + node: node.clone(), + retry: 1, + filename: "logs/output.txt".to_string(), + size: 5, + }, + NodeArtifact { + node, + retry: 2, + filename: "logs/output.txt".to_string(), + size: 6, + }, + ]); + } + + #[tokio::test] + async fn rejects_legacy_artifact_paths_without_retry_segment() { + let object_store: Arc = Arc::new(InMemory::new()); + let store = ArtifactStore::new(object_store.clone(), "artifacts"); + let run_id = fixtures::RUN_1; + object_store + .put( + &ObjectPath::from(format!("artifacts/{run_id}/build@0001/output.txt")), + Bytes::from_static(b"legacy").into(), + ) + .await + .unwrap(); + + let err = store.list_for_run(&run_id).await.unwrap_err(); + + assert!(err.to_string().contains("invalid retry segment")); + assert!(err.to_string().contains("build@0001/output.txt")); + } } diff --git a/lib/crates/fabro-store/src/lib.rs b/lib/crates/fabro-store/src/lib.rs index a88f9c77e..f78f499ea 100644 --- a/lib/crates/fabro-store/src/lib.rs +++ b/lib/crates/fabro-store/src/lib.rs @@ -10,10 +10,14 @@ mod serializable_projection; mod slate; mod types; -pub use artifact_store::{ArtifactStore, NodeArtifact, stage_storage_segment}; +pub use artifact_store::{ + ArtifactKey, ArtifactStore, NodeArtifact, StageArtifactEntry, retry_storage_segment, + stage_storage_segment, +}; pub use error::{Error, Result}; pub use fabro_types::{ - EventEnvelope, NodeState, PendingInterviewRecord, RunBlobId, RunProjection, RunSummary, StageId, + EventEnvelope, PendingInterviewRecord, RunBlobId, RunProjection, RunSummary, StageId, + StageState, }; pub(crate) use keyed_mutex::KeyedMutex; pub use run_state::RunProjectionReducer; diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index ada4202cb..e617c9c21 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -200,7 +200,7 @@ impl RunProjectionReducer for RunProjection { .and_then(|visit| u32::try_from(*visit).ok()) .unwrap_or(1); if let Some(diff) = props.diff.clone() { - self.node_mut(node_id, visit).diff = Some(diff); + stage_entry_for_event(self, event, node_id, visit).diff = Some(diff); } } self.checkpoint = Some(checkpoint.clone()); @@ -267,20 +267,27 @@ impl RunProjectionReducer for RunProjection { EventBody::InterviewInterrupted(props) if !props.question_id.is_empty() => { self.pending_interviews.remove(&props.question_id); } + EventBody::StageStarted(_props) => { + let Some(stage_id) = stored.stage_id.as_ref() else { + return Ok(()); + }; + self.stage_entry_id(stage_id, event.seq); + } EventBody::StagePrompt(props) => { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - let visit = props.visit; - self.node_mut(node_id, visit).prompt = Some(props.text.clone()); - self.node_mut(node_id, visit).provider_used = provider_used_from_prompt(props); + let stage = stage_entry_for_event(self, event, node_id, props.visit); + stage.prompt = Some(props.text.clone()); + stage.provider_used = provider_used_from_prompt(props); } EventBody::PromptCompleted(props) => { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; let visit = self.current_visit_for(node_id).unwrap_or(1); - self.node_mut(node_id, visit).response = Some(props.response.clone()); + stage_entry_for_event(self, event, node_id, visit).response = + Some(props.response.clone()); } EventBody::StageCompleted(props) => { let Some(node_id) = stored.node_id.as_deref() else { @@ -290,7 +297,7 @@ impl RunProjectionReducer for RunProjection { let response = props.response.clone(); let outcome = stage_outcome_from_props(props); let status = node_status_from_outcome(&outcome, ts); - let node = self.node_mut(node_id, visit); + let node = stage_entry_for_event(self, event, node_id, visit); node.response = response; node.status = Some(status); } @@ -300,7 +307,7 @@ impl RunProjectionReducer for RunProjection { }; let visit = self.current_visit_for(node_id).unwrap_or(1); let failure_reason = props.failure.as_ref().map(|detail| detail.message.clone()); - let node = self.node_mut(node_id, visit); + let node = stage_entry_for_event(self, event, node_id, visit); node.status = Some(NodeStatusRecord { status: StageOutcome::Failed { retry_requested: false, @@ -314,14 +321,14 @@ impl RunProjectionReducer for RunProjection { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - self.node_mut(node_id, props.visit).provider_used = + stage_entry_for_event(self, event, node_id, props.visit).provider_used = Some(provider_used_from_agent_session_started(props)); } EventBody::AgentCliStarted(props) => { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - self.node_mut(node_id, props.visit).provider_used = + stage_entry_for_event(self, event, node_id, props.visit).provider_used = Some(provider_used_from_agent_cli_started(props)); } EventBody::CommandStarted(props) => { @@ -329,7 +336,7 @@ impl RunProjectionReducer for RunProjection { return Ok(()); }; let visit = self.current_visit_for(node_id).unwrap_or(1); - self.node_mut(node_id, visit).script_invocation = + stage_entry_for_event(self, event, node_id, visit).script_invocation = Some(serde_json::to_value(props).map_err(|err| { Error::InvalidEvent(format!("invalid command.started payload: {err}")) })?); @@ -339,7 +346,7 @@ impl RunProjectionReducer for RunProjection { return Ok(()); }; let visit = self.current_visit_for(node_id).unwrap_or(1); - let node = self.node_mut(node_id, visit); + let node = stage_entry_for_event(self, event, node_id, visit); node.stdout = Some(props.stdout.clone()); node.stderr = Some(props.stderr.clone()); node.stdout_bytes = Some(props.stdout_bytes); @@ -356,7 +363,7 @@ impl RunProjectionReducer for RunProjection { return Ok(()); }; let visit = self.current_visit_for(node_id).unwrap_or(1); - self.node_mut(node_id, visit).parallel_results = + stage_entry_for_event(self, event, node_id, visit).parallel_results = Some(serde_json::to_value(&props.results).map_err(|err| { Error::InvalidEvent(format!("invalid parallel.completed payload: {err}")) })?); @@ -368,6 +375,19 @@ impl RunProjectionReducer for RunProjection { } } +fn stage_entry_for_event<'a>( + state: &'a mut RunProjection, + event: &EventEnvelope, + node_id: &str, + visit: u32, +) -> &'a mut fabro_types::StageState { + if let Some(stage_id) = event.event.stage_id.as_ref() { + state.stage_entry_id(stage_id, event.seq) + } else { + state.stage_entry(node_id, visit, event.seq) + } +} + pub(crate) fn build_summary(state: &RunProjection, run_id: &RunId) -> RunSummary { let workflow_name = state.spec.as_ref().map(|spec| { if spec.graph.name.is_empty() { @@ -573,11 +593,12 @@ mod tests { use fabro_types::run_event::run::RunFailedProps; use fabro_types::run_event::{ InterviewCompletedProps, InterviewOption, InterviewStartedProps, RunControlEffectProps, + StageStartedProps, }; use fabro_types::{ - BlockedReason, Checkpoint, EventBody, FailureReason, NodeState, QuestionType, RunBlobId, - RunControlAction, RunEvent, RunStatus, SuccessReason, TerminalStatus, WorkflowSettings, - fixtures, + BlockedReason, Checkpoint, EventBody, FailureReason, QuestionType, RunBlobId, + RunControlAction, RunEvent, RunStatus, StageState, SuccessReason, TerminalStatus, + WorkflowSettings, fixtures, }; use serde_json::json; @@ -688,8 +709,9 @@ mod tests { "node_visits": { "build": 2 } } ]], - "nodes": { + "stages": { "build@2": { + "seq": 0, "diff": "diff --git a/file b/file", "stdout": "done" } @@ -698,7 +720,7 @@ mod tests { .unwrap(); let stage_id = StageId::new("build", 2); - let node = state.node(&stage_id).unwrap(); + let node = state.stage(&stage_id).unwrap(); assert_eq!(node.diff.as_deref(), Some("diff --git a/file b/file")); assert_eq!(state.list_node_visits("build"), vec![2]); assert_eq!(state.pending_control, Some(RunControlAction::Cancel)); @@ -706,7 +728,7 @@ mod tests { let round_tripped: RunProjection = serde_json::from_value(serde_json::to_value(&state).unwrap()).unwrap(); let serialized = serde_json::to_value(&state).unwrap(); - let round_tripped_node = round_tripped.node(&stage_id).unwrap(); + let round_tripped_node = round_tripped.stage(&stage_id).unwrap(); assert_eq!(round_tripped_node.stdout.as_deref(), Some("done")); assert_eq!(round_tripped.list_node_visits("build"), vec![2]); assert_eq!( @@ -715,10 +737,12 @@ mod tests { ); assert!(serialized.get("spec").is_some()); assert!(serialized.get("run").is_none()); + assert!(serialized.get("stages").is_some()); + assert!(serialized.get("nodes").is_none()); } #[test] - fn set_node_round_trips_through_json() { + fn set_stage_round_trips_through_json() { let mut state = RunProjection::default(); state.pending_control = Some(RunControlAction::Unpause); state.checkpoints = vec![(7, Checkpoint { @@ -734,9 +758,9 @@ mod tests { restart_failure_signatures: HashMap::new(), node_visits: HashMap::from([("build".to_string(), 2usize)]), })]; - state.set_node(StageId::new("build", 2), NodeState { + state.set_stage(StageId::new("build", 2), StageState { stdout: Some("done".to_string()), - ..NodeState::default() + ..StageState::default() }); let round_tripped: RunProjection = @@ -744,7 +768,7 @@ mod tests { assert_eq!( round_tripped - .node(&StageId::new("build", 2)) + .stage(&StageId::new("build", 2)) .unwrap() .stdout .as_deref(), @@ -757,6 +781,28 @@ mod tests { ); } + #[test] + fn stage_started_stamps_seq_from_stage_id() { + let mut state = RunProjection::default(); + let stage_id = StageId::new("build", 2); + let mut event = test_event( + 17, + EventBody::StageStarted(StageStartedProps { + index: 0, + handler_type: "command".to_string(), + attempt: 3, + max_attempts: 3, + }), + Some("build"), + ); + event.event.stage_id = Some(stage_id.clone()); + + state.apply_event(&event).unwrap(); + + let stage = state.stage(&stage_id).unwrap(); + assert_eq!(stage.seq, 17); + } + #[test] fn interview_events_populate_and_clear_pending_interviews() { let mut state = RunProjection::default(); diff --git a/lib/crates/fabro-store/src/serializable_projection.rs b/lib/crates/fabro-store/src/serializable_projection.rs index 073a21790..face8b695 100644 --- a/lib/crates/fabro-store/src/serializable_projection.rs +++ b/lib/crates/fabro-store/src/serializable_projection.rs @@ -11,15 +11,15 @@ impl Serialize for SerializableProjection<'_> { { let mut projection = self.0.clone(); let stage_ids: Vec<_> = projection - .iter_nodes() + .iter_stages() .map(|(stage_id, _)| stage_id.clone()) .collect(); for stage_id in stage_ids { - let Some(node) = projection.node(&stage_id).cloned() else { + let Some(node) = projection.stage(&stage_id).cloned() else { continue; }; - projection.set_node(stage_id, crate::NodeState { + projection.set_stage(stage_id, crate::StageState { prompt: None, response: None, diff: None, diff --git a/lib/crates/fabro-store/tests/serializable_projection.rs b/lib/crates/fabro-store/tests/serializable_projection.rs index b7feb76d3..eeeffe443 100644 --- a/lib/crates/fabro-store/tests/serializable_projection.rs +++ b/lib/crates/fabro-store/tests/serializable_projection.rs @@ -1,7 +1,7 @@ use std::collections::{BTreeMap, HashMap}; use chrono::{TimeZone, Utc}; -use fabro_store::{NodeState, RunProjection, SerializableProjection, StageId}; +use fabro_store::{RunProjection, SerializableProjection, StageId, StageState}; use fabro_types::graph::Graph; use fabro_types::run::RunSpec; use fabro_types::{ @@ -77,7 +77,8 @@ fn serializable_projection_round_trips_and_trims_bulky_node_fields() { clone_branch: None, }); projection.pending_interviews = BTreeMap::new(); - projection.set_node(stage_id.clone(), NodeState { + projection.set_stage(stage_id.clone(), StageState { + seq: 7, prompt: Some("plan the work".to_string()), response: Some("done".to_string()), status: Some(NodeStatusRecord { @@ -107,7 +108,7 @@ fn serializable_projection_round_trips_and_trims_bulky_node_fields() { .expect("projection should serialize"); let round_tripped: RunProjection = serde_json::from_value(serialized).expect("serialized projection should deserialize"); - let node = round_tripped.node(&stage_id).expect("node should remain"); + let node = round_tripped.stage(&stage_id).expect("node should remain"); assert_eq!(round_tripped.spec().map(RunSpec::id), Some(fixtures::RUN_1)); assert_eq!( @@ -119,6 +120,7 @@ fn serializable_projection_round_trips_and_trims_bulky_node_fields() { ); assert_eq!(round_tripped.status(), Some(RunStatus::Running)); assert!(!round_tripped.is_terminal()); + assert_eq!(node.seq, 7); assert_eq!(node.prompt, None); assert_eq!(node.response, None); assert_eq!(node.diff, None); diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index 27de2185d..73ae59eee 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -51,7 +51,7 @@ pub use graph::{AttrValue, Edge, Graph, Node, is_llm_handler_type, shape_to_hand pub use interview::{InterviewQuestionRecord, QuestionType}; pub use node_status::NodeStatusRecord; pub use outcome::{ - FailureCategory, FailureDetail, NodeResult, Outcome, OutcomeMeta, StageOutcome, StageState, + FailureCategory, FailureDetail, NodeResult, Outcome, OutcomeMeta, StageOutcome, StageStatus, }; pub use pull_request::{ PullRequestDetail, PullRequestGithubDetail, PullRequestRecord, PullRequestRef, PullRequestUser, @@ -71,7 +71,7 @@ pub use run_event::{ MetadataSnapshotPhase, RunEvent, RunNoticeLevel, }; pub use run_id::{RunId, fixtures}; -pub use run_projection::{NodeState, PendingInterviewRecord, RunProjection}; +pub use run_projection::{PendingInterviewRecord, RunProjection, StageState}; pub use run_summary::RunSummary; pub use sandbox_record::SandboxRecord; pub use secret::{SecretMetadata, SecretType}; diff --git a/lib/crates/fabro-types/src/outcome.rs b/lib/crates/fabro-types/src/outcome.rs index 60ad9a811..16bbff240 100644 --- a/lib/crates/fabro-types/src/outcome.rs +++ b/lib/crates/fabro-types/src/outcome.rs @@ -106,7 +106,7 @@ impl<'de> Deserialize<'de> for StageOutcome { )] #[serde(rename_all = "snake_case")] #[strum(serialize_all = "snake_case")] -pub enum StageState { +pub enum StageStatus { Pending, Running, Retrying, @@ -117,7 +117,7 @@ pub enum StageState { Cancelled, } -impl StageState { +impl StageStatus { #[must_use] pub fn is_terminal(self) -> bool { matches!( @@ -131,7 +131,7 @@ impl StageState { } } -impl From for StageState { +impl From for StageStatus { fn from(outcome: StageOutcome) -> Self { match outcome { StageOutcome::Succeeded => Self::Succeeded, @@ -301,7 +301,7 @@ impl Outcome { mod tests { use serde_json::json; - use super::{StageOutcome, StageState}; + use super::{StageOutcome, StageStatus}; #[test] fn stage_outcome_failed_serde_is_lossy_for_retry_intent() { @@ -321,23 +321,23 @@ mod tests { } #[test] - fn stage_state_projects_terminal_outcomes() { + fn stage_status_projects_terminal_outcomes() { assert_eq!( - StageState::from(StageOutcome::Succeeded), - StageState::Succeeded + StageStatus::from(StageOutcome::Succeeded), + StageStatus::Succeeded ); assert_eq!( - StageState::from(StageOutcome::PartiallySucceeded), - StageState::PartiallySucceeded + StageStatus::from(StageOutcome::PartiallySucceeded), + StageStatus::PartiallySucceeded ); assert_eq!( - StageState::from(StageOutcome::Failed { + StageStatus::from(StageOutcome::Failed { retry_requested: true, }), - StageState::Failed + StageStatus::Failed ); - assert!(StageState::Cancelled.is_terminal()); - assert!(!StageState::Running.is_terminal()); + assert!(StageStatus::Cancelled.is_terminal()); + assert!(!StageStatus::Running.is_terminal()); } } diff --git a/lib/crates/fabro-types/src/run_projection.rs b/lib/crates/fabro-types/src/run_projection.rs index f3d404b73..bba87e156 100644 --- a/lib/crates/fabro-types/src/run_projection.rs +++ b/lib/crates/fabro-types/src/run_projection.rs @@ -28,7 +28,8 @@ pub struct RunProjection { pub pull_request: Option, pub superseded_by: Option, pub pending_interviews: BTreeMap, - nodes: HashMap, + #[serde(alias = "nodes")] + stages: HashMap, } #[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)] @@ -38,7 +39,9 @@ pub struct PendingInterviewRecord { } #[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)] -pub struct NodeState { +pub struct StageState { + #[serde(default)] + pub seq: u32, pub prompt: Option, pub response: Option, pub status: Option, @@ -62,25 +65,25 @@ pub struct NodeState { } impl RunProjection { - pub fn node(&self, node: &StageId) -> Option<&NodeState> { - self.nodes.get(node) + pub fn stage(&self, stage_id: &StageId) -> Option<&StageState> { + self.stages.get(stage_id) } - pub fn iter_nodes(&self) -> impl Iterator { - self.nodes.iter() + pub fn iter_stages(&self) -> impl Iterator { + self.stages.iter() } pub fn is_empty(&self) -> bool { - self.nodes.is_empty() + self.stages.is_empty() } - pub fn set_node(&mut self, node: StageId, state: NodeState) { - self.nodes.insert(node, state); + pub fn set_stage(&mut self, stage_id: StageId, state: StageState) { + self.stages.insert(stage_id, state); } pub fn list_node_visits(&self, node_id: &str) -> Vec { let mut visits = self - .nodes + .stages .keys() .filter(|node| node.node_id() == node_id) .map(StageId::visit) @@ -110,12 +113,25 @@ impl RunProjection { &self.pending_interviews } - pub fn node_mut(&mut self, node_id: &str, visit: u32) -> &mut NodeState { - self.nodes.entry(StageId::new(node_id, visit)).or_default() + pub fn stage_mut(&mut self, node_id: &str, visit: u32) -> &mut StageState { + self.stage_entry(node_id, visit, 0) + } + + pub fn stage_entry_id(&mut self, stage_id: &StageId, seq: u32) -> &mut StageState { + self.stages + .entry(stage_id.clone()) + .or_insert_with(|| StageState { + seq, + ..Default::default() + }) + } + + pub fn stage_entry(&mut self, node_id: &str, visit: u32, seq: u32) -> &mut StageState { + self.stage_entry_id(&StageId::new(node_id, visit), seq) } pub fn current_visit_for(&self, node_id: &str) -> Option { - self.nodes + self.stages .keys() .filter(|node| node.node_id() == node_id) .map(StageId::visit) diff --git a/lib/crates/fabro-workflow/src/artifact_upload.rs b/lib/crates/fabro-workflow/src/artifact_upload.rs index 2fb9d3c9a..265e2ad63 100644 --- a/lib/crates/fabro-workflow/src/artifact_upload.rs +++ b/lib/crates/fabro-workflow/src/artifact_upload.rs @@ -11,6 +11,7 @@ pub trait StageArtifactUploader: Send + Sync { async fn upload_stage_artifacts( &self, stage_id: &StageId, + retry: u32, artifact_capture_dir: &Path, artifacts: &[ArtifactUpload], ) -> Result<()>; diff --git a/lib/crates/fabro-workflow/src/git.rs b/lib/crates/fabro-workflow/src/git.rs index e8c5ead70..e6f7d08eb 100644 --- a/lib/crates/fabro-workflow/src/git.rs +++ b/lib/crates/fabro-workflow/src/git.rs @@ -556,13 +556,13 @@ mod tests { .git_entries() .unwrap(); let paths: Vec<&str> = files.iter().map(|(path, _)| path.as_str()).collect(); - assert!(paths.contains(&"stages/work@2/prompt.md")); - assert!(paths.contains(&"stages/work@2/response.md")); - assert!(paths.contains(&"stages/work@2/status.json")); - assert!(paths.contains(&"stages/work@2/provider_used.json")); - assert!(paths.contains(&"stages/work@2/script_invocation.json")); - assert!(paths.contains(&"stages/work@2/script_timing.json")); - assert!(paths.contains(&"stages/work@2/parallel_results.json")); + assert!(paths.contains(&"stages/001-work@2/prompt.md")); + assert!(paths.contains(&"stages/001-work@2/response.md")); + assert!(paths.contains(&"stages/001-work@2/status.json")); + assert!(paths.contains(&"stages/001-work@2/provider_used.json")); + assert!(paths.contains(&"stages/001-work@2/script_invocation.json")); + assert!(paths.contains(&"stages/001-work@2/script_timing.json")); + assert!(paths.contains(&"stages/001-work@2/parallel_results.json")); } #[test] diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 32d25ddf0..15c220c5a 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -505,9 +505,9 @@ mod tests { logger.flush().await; let state = run_store.state().await.unwrap(); - let node_state = state.node(&StageId::new("plan", 1)).unwrap(); + let stage_state = state.stage(&StageId::new("plan", 1)).unwrap(); assert_eq!( - node_state.prompt.as_deref(), + stage_state.prompt.as_deref(), Some("Achieve: Build a feature") ); } @@ -532,8 +532,8 @@ mod tests { logger.flush().await; let state = run_store.state().await.unwrap(); - let node_state = state.node(&StageId::new("work", 1)).unwrap(); - assert_eq!(node_state.prompt.as_deref(), Some("Do work")); + let stage_state = state.stage(&StageId::new("work", 1)).unwrap(); + assert_eq!(stage_state.prompt.as_deref(), Some("Do work")); } #[tokio::test] @@ -774,9 +774,9 @@ mod tests { logger.flush().await; let state = run_store.state().await.unwrap(); - let node_state = state.node(&StageId::new("step", 1)).unwrap(); + let stage_state = state.stage(&StageId::new("step", 1)).unwrap(); assert_eq!( - node_state.provider_used.as_ref().unwrap()["provider"], + stage_state.provider_used.as_ref().unwrap()["provider"], "openai" ); } @@ -1262,8 +1262,8 @@ Some text in between. logger.flush().await; let state = run_store.state().await.unwrap(); - let node_state = state.node(&StageId::new("report", 1)).unwrap(); - let prompt_content = node_state.prompt.as_deref().unwrap(); + let stage_state = state.stage(&StageId::new("report", 1)).unwrap(); + let prompt_content = stage_state.prompt.as_deref().unwrap(); assert!( prompt_content.contains("## Script Output\nAll tests passed"), "prompt.md should contain preamble" diff --git a/lib/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index b123727b6..819e0c3ac 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -509,8 +509,8 @@ mod tests { logger.flush().await; let snapshot = run_store.state().await.unwrap(); - let node_state = snapshot.node(&StageId::new("script_node", 1)).unwrap(); - let json = node_state.script_invocation.as_ref().unwrap(); + let stage_state = snapshot.stage(&StageId::new("script_node", 1)).unwrap(); + let json = stage_state.script_invocation.as_ref().unwrap(); assert_eq!(json["command"], "echo hello"); assert_eq!(json["language"], "shell"); assert_eq!(json["timeout_ms"], serde_json::Value::Null); @@ -540,8 +540,8 @@ mod tests { logger.flush().await; let snapshot = run_store.state().await.unwrap(); - let node_state = snapshot.node(&StageId::new("script_node", 1)).unwrap(); - let json = node_state.script_invocation.as_ref().unwrap(); + let stage_state = snapshot.stage(&StageId::new("script_node", 1)).unwrap(); + let json = stage_state.script_invocation.as_ref().unwrap(); assert_eq!(json["command"], "echo hello"); assert_eq!(json["language"], "shell"); assert_eq!(json["timeout_ms"], 5000); @@ -567,15 +567,15 @@ mod tests { logger.flush().await; let snapshot = run_store.state().await.unwrap(); - let node_state = snapshot.node(&StageId::new("script_node", 1)).unwrap(); - let stdout = node_state.stdout.as_deref().unwrap(); + let stage_state = snapshot.stage(&StageId::new("script_node", 1)).unwrap(); + let stdout = stage_state.stdout.as_deref().unwrap(); assert_eq!(command_log_text(&services, stdout).await.trim(), "hello"); - let stderr = node_state.stderr.as_deref().unwrap(); + let stderr = stage_state.stderr.as_deref().unwrap(); assert_eq!(command_log_text(&services, stderr).await, ""); - assert_eq!(node_state.stdout_bytes, Some(6)); - assert_eq!(node_state.stderr_bytes, Some(0)); - assert_eq!(node_state.streams_separated, Some(true)); - assert_eq!(node_state.live_streaming, Some(true)); + assert_eq!(stage_state.stdout_bytes, Some(6)); + assert_eq!(stage_state.stderr_bytes, Some(0)); + assert_eq!(stage_state.streams_separated, Some(true)); + assert_eq!(stage_state.live_streaming, Some(true)); } #[tokio::test] @@ -598,8 +598,8 @@ mod tests { logger.flush().await; let snapshot = run_store.state().await.unwrap(); - let node_state = snapshot.node(&StageId::new("script_node", 1)).unwrap(); - let stderr = node_state.stderr.as_deref().unwrap(); + let stage_state = snapshot.stage(&StageId::new("script_node", 1)).unwrap(); + let stderr = stage_state.stderr.as_deref().unwrap(); assert_eq!(command_log_text(&services, stderr).await.trim(), "oops"); } @@ -623,8 +623,8 @@ mod tests { logger.flush().await; let snapshot = run_store.state().await.unwrap(); - let node_state = snapshot.node(&StageId::new("script_node", 1)).unwrap(); - let json = node_state.script_timing.as_ref().unwrap(); + let stage_state = snapshot.stage(&StageId::new("script_node", 1)).unwrap(); + let json = stage_state.script_timing.as_ref().unwrap(); assert!(json["duration_ms"].is_u64()); assert_eq!(json["exit_code"], 0); assert_eq!(json["termination"], "exited"); @@ -648,8 +648,8 @@ mod tests { logger.flush().await; let snapshot = run_store.state().await.unwrap(); - let node_state = snapshot.node(&StageId::new("script_node", 1)).unwrap(); - let json = node_state.script_timing.as_ref().unwrap(); + let stage_state = snapshot.stage(&StageId::new("script_node", 1)).unwrap(); + let json = stage_state.script_timing.as_ref().unwrap(); assert_eq!(json["exit_code"], 1); assert_eq!(json["termination"], "exited"); } @@ -678,8 +678,8 @@ mod tests { logger.flush().await; let snapshot = run_store.state().await.unwrap(); - let node_state = snapshot.node(&StageId::new("script_node", 1)).unwrap(); - let json = node_state.script_timing.as_ref().unwrap(); + let stage_state = snapshot.stage(&StageId::new("script_node", 1)).unwrap(); + let json = stage_state.script_timing.as_ref().unwrap(); assert!(json["duration_ms"].is_u64()); assert_eq!(json["exit_code"], serde_json::Value::Null); assert_eq!(json["termination"], "timed_out"); @@ -706,7 +706,7 @@ mod tests { let snapshot = run_store.state().await.unwrap(); let node = snapshot - .node(&StageId::new("script_node", 1)) + .stage(&StageId::new("script_node", 1)) .cloned() .unwrap(); diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index 383cb005f..9e703d0e6 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -736,8 +736,8 @@ mod tests { assert!(results.is_some()); let state = run_store.state().await.unwrap(); - let node_state = state.node(&StageId::new("par", 1)).unwrap(); - let parsed = node_state.parallel_results.as_ref().unwrap(); + let stage_state = state.stage(&StageId::new("par", 1)).unwrap(); + let parsed = stage_state.parallel_results.as_ref().unwrap(); assert!( parsed.is_array(), "parallel_results.json should be a JSON array" @@ -781,8 +781,8 @@ mod tests { logger.flush().await; let state = run_store.state().await.unwrap(); - let node_state = state.node(&fabro_store::StageId::new("par", 1)).unwrap(); - let results = node_state.parallel_results.as_ref().unwrap(); + let stage_state = state.stage(&fabro_store::StageId::new("par", 1)).unwrap(); + let results = stage_state.parallel_results.as_ref().unwrap(); assert!(results.is_array()); assert_eq!(results.as_array().unwrap().len(), 2); } diff --git a/lib/crates/fabro-workflow/src/handler/prompt.rs b/lib/crates/fabro-workflow/src/handler/prompt.rs index 3881faf4f..37b190220 100644 --- a/lib/crates/fabro-workflow/src/handler/prompt.rs +++ b/lib/crates/fabro-workflow/src/handler/prompt.rs @@ -367,8 +367,11 @@ mod tests { logger.flush().await; let state = run_store.state().await.unwrap(); - let node_state = state.node(&StageId::new("classify", 1)).unwrap(); - assert_eq!(node_state.provider_used.as_ref().unwrap()["mode"], "prompt"); + let stage_state = state.stage(&StageId::new("classify", 1)).unwrap(); + assert_eq!( + stage_state.provider_used.as_ref().unwrap()["mode"], + "prompt" + ); } struct OneShotCapturingBackend { diff --git a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs index 128f3f5a2..24f5f8c90 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs @@ -8,7 +8,7 @@ use fabro_core::graph::NodeSpec; use fabro_core::lifecycle::{AttemptContext, AttemptResultContext, NodeDecision, RunLifecycle}; use fabro_core::outcome::NodeResult; use fabro_core::state::ExecutionState; -use fabro_store::ArtifactStore; +use fabro_store::{ArtifactKey, ArtifactStore}; use fabro_types::{ArtifactUpload, RunId, StageId}; use tokio::fs; use tokio::time::sleep; @@ -95,7 +95,8 @@ impl RunLifecycle for ArtifactLifecycle { } let epoch = self.attempt_start_epoch.lock().unwrap().unwrap_or(0.0); let node_id = ctx.node.id(); - let visit = state.node_visits.get(node_id).copied().unwrap_or(1); + let visit_count = state.node_visits.get(node_id).copied().unwrap_or(1); + let visit = u32::try_from(visit_count.max(1)).unwrap_or(u32::MAX); let node_slug = if visit <= 1 { node_id.to_string() } else { @@ -113,10 +114,11 @@ impl RunLifecycle for ArtifactLifecycle { .await { Ok(summary) if summary.files_copied > 0 => { - let stage_id = StageId::new(node_id.to_string(), ctx.attempt); + let stage_id = StageId::new(node_id.to_string(), visit); if let Err(err) = self .persist_artifacts( &stage_id, + ctx.attempt, artifact_capture_dir.path(), &summary.captured_assets, ) @@ -199,6 +201,7 @@ impl ArtifactLifecycle { async fn persist_artifacts( &self, stage_id: &StageId, + retry: u32, artifact_capture_dir: &std::path::Path, artifacts: &[ArtifactUpload], ) -> Result<()> { @@ -209,7 +212,7 @@ impl ArtifactLifecycle { let mut last_error = None; for attempt in 0..=ARTIFACT_UPLOAD_RETRY_DELAYS.len() { match self - .persist_artifacts_once(sink, stage_id, artifact_capture_dir, artifacts) + .persist_artifacts_once(sink, stage_id, retry, artifact_capture_dir, artifacts) .await { Ok(()) => return Ok(()), @@ -228,17 +231,18 @@ impl ArtifactLifecycle { &self, sink: &ArtifactSink, stage_id: &StageId, + retry: u32, artifact_capture_dir: &std::path::Path, artifacts: &[ArtifactUpload], ) -> Result<()> { match sink { ArtifactSink::Store(store) => { - self.store_artifacts(store, stage_id, artifact_capture_dir, artifacts) + self.store_artifacts(store, stage_id, retry, artifact_capture_dir, artifacts) .await } ArtifactSink::Uploader(uploader) => { uploader - .upload_stage_artifacts(stage_id, artifact_capture_dir, artifacts) + .upload_stage_artifacts(stage_id, retry, artifact_capture_dir, artifacts) .await } } @@ -248,6 +252,7 @@ impl ArtifactLifecycle { &self, store: &ArtifactStore, stage_id: &StageId, + retry: u32, artifact_capture_dir: &std::path::Path, artifacts: &[ArtifactUpload], ) -> Result<()> { @@ -257,7 +262,11 @@ impl ArtifactLifecycle { .await .with_context(|| format!("failed to read artifact {}", local_path.display()))?; store - .put(&self.run_id, stage_id, &artifact.path, &bytes) + .put( + &self.run_id, + &ArtifactKey::new(stage_id.clone(), retry, artifact.path.clone()), + &bytes, + ) .await .map_err(anyhow::Error::new)?; } diff --git a/lib/crates/fabro-workflow/src/operations/fork.rs b/lib/crates/fabro-workflow/src/operations/fork.rs index 3c119527f..9c7c48d12 100644 --- a/lib/crates/fabro-workflow/src/operations/fork.rs +++ b/lib/crates/fabro-workflow/src/operations/fork.rs @@ -401,7 +401,7 @@ mod tests { let forked_events = forked.list_events().await.unwrap(); let forked_state = fabro_store::RunProjection::apply_events(&forked_events).unwrap(); let node = forked_state - .node(&StageId::new("work", 1)) + .stage(&StageId::new("work", 1)) .expect("forked state should retain historical node projection"); assert_eq!(node.response.as_deref(), Some("historical response")); diff --git a/lib/crates/fabro-workflow/src/outcome.rs b/lib/crates/fabro-workflow/src/outcome.rs index 6fad164aa..9092abd86 100644 --- a/lib/crates/fabro-workflow/src/outcome.rs +++ b/lib/crates/fabro-workflow/src/outcome.rs @@ -1,5 +1,5 @@ pub use fabro_core::outcome::{ - FailureCategory, FailureDetail, OutcomeMeta, StageOutcome, StageState, + FailureCategory, FailureDetail, OutcomeMeta, StageOutcome, StageStatus, }; use fabro_llm::types::TokenCounts as LlmTokenCounts; use fabro_model::{ diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index 8377ca3ec..58e1919c0 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -770,7 +770,7 @@ async fn execute_persists_start_record_and_node_status() { ); assert_eq!(start.base_sha.as_deref(), Some("abc123")); - let node = state.node(&fabro_store::StageId::new("start", 1)).unwrap(); + let node = state.stage(&fabro_store::StageId::new("start", 1)).unwrap(); assert_eq!( node.status.as_ref().unwrap().status, StageOutcome::Succeeded @@ -825,7 +825,7 @@ async fn timeout_causes_fail_status_record() { .await; let state = executed.engine.run.run_store.state().await.unwrap(); let status = state - .node(&fabro_store::StageId::new("work", 1)) + .stage(&fabro_store::StageId::new("work", 1)) .unwrap() .status .as_ref() diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index a9b4cba5c..b58740a2a 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -213,7 +213,7 @@ fn parse_dot_summary(dot: &str) -> (String, usize, usize) { /// directory scan behavior. fn read_plan_text(state: &RunProjection) -> Option { let mut plan_nodes = state - .iter_nodes() + .iter_stages() .filter_map(|(stage_id, node)| { stage_id.node_id().starts_with("plan").then_some(( stage_id.node_id(), @@ -997,9 +997,9 @@ mod tests { #[test] fn read_plan_text_found() { let mut state = RunProjection::default(); - state.set_node( + state.set_stage( fabro_store::StageId::new("plan", 1), - fabro_store::NodeState { + fabro_store::StageState { response: Some("This is the plan".to_string()), ..Default::default() }, @@ -1012,9 +1012,9 @@ mod tests { #[test] fn read_plan_text_prefix_match() { let mut state = RunProjection::default(); - state.set_node( + state.set_stage( fabro_store::StageId::new("planning", 1), - fabro_store::NodeState { + fabro_store::StageState { response: Some("Planning content".to_string()), ..Default::default() }, @@ -1027,16 +1027,16 @@ mod tests { #[test] fn read_plan_text_prefers_alphabetically_first_plan_node() { let mut state = RunProjection::default(); - state.set_node( + state.set_stage( fabro_store::StageId::new("planning", 1), - fabro_store::NodeState { + fabro_store::StageState { response: Some("Planning content".to_string()), ..Default::default() }, ); - state.set_node( + state.set_stage( fabro_store::StageId::new("plan", 1), - fabro_store::NodeState { + fabro_store::StageState { response: Some("Plan content".to_string()), ..Default::default() }, @@ -1049,9 +1049,9 @@ mod tests { #[test] fn read_plan_text_not_found() { let mut state = RunProjection::default(); - state.set_node( + state.set_stage( fabro_store::StageId::new("implement", 1), - fabro_store::NodeState::default(), + fabro_store::StageState::default(), ); let result = read_plan_text(&state); diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 4fc56ec7d..b8188a539 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -27,7 +27,7 @@ use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_llm::provider::Provider; use fabro_sandbox::daytona::{DaytonaConfig, DaytonaSandbox, DaytonaSnapshotConfig}; use fabro_static::EnvVars; -use fabro_store::{ArtifactStore, Database}; +use fabro_store::{ArtifactKey, ArtifactStore, Database}; use fabro_types::{RunId, StageId, WorkflowSettings}; use fabro_workflow::artifact::sync_artifacts_to_env; use fabro_workflow::context::Context; @@ -1387,8 +1387,11 @@ async fn daytona_asset_collection() { test_artifact_store(dir.path()) .get( &run_options.run_id, - &StageId::new("create_assets", 1), - "test-results/report.xml", + &ArtifactKey::new( + StageId::new("create_assets", 1), + 1, + "test-results/report.xml", + ), ) .await .unwrap() diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 1c522911e..2325be740 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -31,7 +31,7 @@ use fabro_interview::{ QueueInterviewer, RecordingInterviewer, }; use fabro_llm::provider::Provider; -use fabro_store::{ArtifactStore, Database}; +use fabro_store::{ArtifactKey, ArtifactStore, Database}; use fabro_types::{CommandTermination, RunEvent, RunId, StageId, WorkflowSettings, parse_blob_ref}; use fabro_validate::{Severity, validate, validate_or_raise}; use fabro_workflow::context::Context; @@ -441,15 +441,15 @@ async fn end_to_end_linear_pipeline() { .contains(&"codergen_step".to_string()) ); - let node_state = state - .node(&fabro_types::StageId::new("codergen_step", 1)) + let stage_state = state + .stage(&fabro_types::StageId::new("codergen_step", 1)) .unwrap(); assert!( - node_state.response.is_some(), + stage_state.response.is_some(), "response should be projected" ); - assert!(node_state.status.is_some(), "status should be projected"); - let prompt_content = node_state.prompt.as_deref().unwrap(); + assert!(stage_state.status.is_some(), "status should be projected"); + let prompt_content = stage_state.prompt.as_deref().unwrap(); assert!( prompt_content.ends_with("Implement the feature"), "prompt should end with original prompt, got: {prompt_content}" @@ -1882,7 +1882,7 @@ async fn smoke_test_with_mock_codergen_backend() { "should NOT have traversed fix path" ); - let plan_state = state.node(&fabro_types::StageId::new("plan", 1)).unwrap(); + let plan_state = state.stage(&fabro_types::StageId::new("plan", 1)).unwrap(); let plan_response = plan_state .response .as_deref() @@ -3863,7 +3863,7 @@ async fn integration_smoke_plan_implement_review_done() { assert!(cp.completed_nodes.contains(&"implement".to_string())); assert!(cp.completed_nodes.contains(&"review".to_string())); - let plan_state = state.node(&fabro_types::StageId::new("plan", 1)).unwrap(); + let plan_state = state.stage(&fabro_types::StageId::new("plan", 1)).unwrap(); assert!(plan_state.prompt.is_some()); assert!(plan_state.response.is_some()); @@ -6410,7 +6410,7 @@ mod real_llm { // Verify actual LLM responses were written let plan_response = state - .node(&fabro_types::StageId::new("plan", 1)) + .stage(&fabro_types::StageId::new("plan", 1)) .and_then(|node| node.response.as_deref()) .unwrap(); assert!( @@ -6742,7 +6742,7 @@ mod real_llm { assert_eq!(outcome.status, StageOutcome::Succeeded); let response = state - .node(&fabro_types::StageId::new("classify", 1)) + .stage(&fabro_types::StageId::new("classify", 1)) .and_then(|node| node.response.as_deref()) .unwrap(); assert!(!response.is_empty(), "response.md should be non-empty"); @@ -7906,7 +7906,7 @@ async fn hook_stage_start_proceed_allows_execution() { assert!( state - .node(&fabro_types::StageId::new("work", 1)) + .stage(&fabro_types::StageId::new("work", 1)) .and_then(|node| node.response.as_ref()) .is_some(), "response should exist when StageStart hook proceeds" @@ -7933,7 +7933,7 @@ async fn hook_stage_start_skip_bypasses_node() { assert!( state - .node(&fabro_types::StageId::new("work", 1)) + .stage(&fabro_types::StageId::new("work", 1)) .and_then(|node| node.response.as_ref()) .is_none(), "response should not exist when StageStart hook skips node" @@ -7997,7 +7997,7 @@ async fn hook_stage_start_matcher_filters_by_node_id() { assert!( state - .node(&fabro_types::StageId::new("step1", 1)) + .stage(&fabro_types::StageId::new("step1", 1)) .and_then(|node| node.response.as_ref()) .is_some(), "step1 should execute because matcher doesn't match it" @@ -8005,7 +8005,7 @@ async fn hook_stage_start_matcher_filters_by_node_id() { assert!( state - .node(&fabro_types::StageId::new("step2", 1)) + .stage(&fabro_types::StageId::new("step2", 1)) .and_then(|node| node.response.as_ref()) .is_none(), "step2 should be skipped because matcher matches it" @@ -8498,14 +8498,14 @@ async fn hook_matcher_regex_pattern() { assert!( state - .node(&fabro_types::StageId::new("step1", 1)) + .stage(&fabro_types::StageId::new("step1", 1)) .and_then(|node| node.response.as_ref()) .is_none(), "step1 should be skipped by regex ^step" ); assert!( state - .node(&fabro_types::StageId::new("step2", 1)) + .stage(&fabro_types::StageId::new("step2", 1)) .and_then(|node| node.response.as_ref()) .is_none(), "step2 should be skipped by regex ^step" @@ -8715,7 +8715,7 @@ async fn run_fidelity_prompt_pipeline(fidelity: &str) -> String { .expect("pipeline should succeed"); state - .node(&fabro_types::StageId::new("report", 1)) + .stage(&fabro_types::StageId::new("report", 1)) .and_then(|node| node.prompt.clone()) .expect("report prompt should exist") } @@ -9430,10 +9430,10 @@ async fn node_dir_uses_visit_count_on_revisit() { assert_eq!(outcome.status, StageOutcome::Succeeded); let first = state - .node(&fabro_types::StageId::new("gated_work", 1)) + .stage(&fabro_types::StageId::new("gated_work", 1)) .unwrap(); let second = state - .node(&fabro_types::StageId::new("gated_work", 2)) + .stage(&fabro_types::StageId::new("gated_work", 2)) .unwrap(); assert_eq!( first.status.as_ref().unwrap().status, @@ -10335,7 +10335,7 @@ async fn full_pipeline_with_cli_backend_node() { assert_eq!(outcome.status, StageOutcome::Succeeded); let api_response = state - .node(&fabro_types::StageId::new("api_work", 1)) + .stage(&fabro_types::StageId::new("api_work", 1)) .and_then(|node| node.response.as_deref()) .unwrap(); assert!( @@ -10344,7 +10344,7 @@ async fn full_pipeline_with_cli_backend_node() { ); let cli_response = state - .node(&fabro_types::StageId::new("cli_work", 1)) + .stage(&fabro_types::StageId::new("cli_work", 1)) .and_then(|node| node.response.as_deref()) .unwrap(); assert_eq!( @@ -10353,7 +10353,7 @@ async fn full_pipeline_with_cli_backend_node() { ); let provider_json = state - .node(&fabro_types::StageId::new("cli_work", 1)) + .stage(&fabro_types::StageId::new("cli_work", 1)) .unwrap() .provider_used .as_ref() @@ -10454,7 +10454,7 @@ async fn stylesheet_backend_property_routes_to_cli() { assert_eq!(outcome.status, StageOutcome::Succeeded); let response = state - .node(&fabro_types::StageId::new("work", 1)) + .stage(&fabro_types::StageId::new("work", 1)) .and_then(|node| node.response.as_deref()) .unwrap(); assert_eq!( @@ -13003,15 +13003,20 @@ async fn asset_collection_local_sandbox_success() { "expected stored artifacts for both files" ); assert_eq!(artifacts[0].node, StageId::new("create_assets", 1)); + assert_eq!(artifacts[0].retry, 1); assert_eq!(artifacts[0].filename, "test-results/output.txt"); assert_eq!(artifacts[1].node, StageId::new("create_assets", 1)); + assert_eq!(artifacts[1].retry, 1); assert_eq!(artifacts[1].filename, "test-results/report.xml"); let report_content = String::from_utf8( artifact_store .get( &run_options.run_id, - &StageId::new("create_assets", 1), - "test-results/report.xml", + &ArtifactKey::new( + StageId::new("create_assets", 1), + 1, + "test-results/report.xml", + ), ) .await .unwrap() @@ -13132,8 +13137,11 @@ async fn asset_collection_local_sandbox_on_failure() { test_artifact_store(run_dir.path()) .get( &run_options.run_id, - &StageId::new("create_assets", 1), - "test-results/report.xml", + &ArtifactKey::new( + StageId::new("create_assets", 1), + 1, + "test-results/report.xml", + ), ) .await .unwrap() @@ -13236,8 +13244,11 @@ async fn asset_collection_docker_sandbox() { test_artifact_store(run_dir.path()) .get( &run_options.run_id, - &StageId::new("create_assets", 1), - "test-results/report.xml", + &ArtifactKey::new( + StageId::new("create_assets", 1), + 1, + "test-results/report.xml", + ), ) .await .unwrap() diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index b84e0dcea..3afbe10c3 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -154,7 +154,6 @@ models/model-reference.ts models/model-test-mode.ts models/model-test-result.ts models/model.ts -models/node-state.ts models/node-status-record.ts models/notification-provider-settings.ts models/notification-route-settings.ts @@ -289,6 +288,7 @@ models/ssh-access-request.ts models/ssh-access-response.ts models/stage-outcome.ts models/stage-state.ts +models/stage-status.ts models/stage-turn.ts models/start-run-request.ts models/submit-answer-request.ts diff --git a/lib/packages/fabro-api-client/src/api/run-internals-api.ts b/lib/packages/fabro-api-client/src/api/run-internals-api.ts index 6bcf36c3a..0852b16f6 100644 --- a/lib/packages/fabro-api-client/src/api/run-internals-api.ts +++ b/lib/packages/fabro-api-client/src/api/run-internals-api.ts @@ -290,16 +290,19 @@ export const RunInternalsApiAxiosParamCreator = function (configuration?: Config * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. * @param {string} filename Relative artifact path. `/` is allowed as a path separator. Backslash, empty segments, and traversal segments (`.` and `..`) are invalid. + * @param {number} retry Retry attempt number for the artifact. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - getStageArtifact: async (id: string, stageId: string, filename: string, options: RawAxiosRequestConfig = {}): Promise => { + getStageArtifact: async (id: string, stageId: string, filename: string, retry: number, options: RawAxiosRequestConfig = {}): Promise => { // verify required parameter 'id' is not null or undefined assertParamExists('getStageArtifact', 'id', id) // verify required parameter 'stageId' is not null or undefined assertParamExists('getStageArtifact', 'stageId', stageId) // verify required parameter 'filename' is not null or undefined assertParamExists('getStageArtifact', 'filename', filename) + // verify required parameter 'retry' is not null or undefined + assertParamExists('getStageArtifact', 'retry', retry) const localVarPath = `/api/v1/runs/{id}/stages/{stageId}/artifacts/download` .replace(`{${"id"}}`, encodeURIComponent(String(id))) .replace(`{${"stageId"}}`, encodeURIComponent(String(stageId))); @@ -324,6 +327,10 @@ export const RunInternalsApiAxiosParamCreator = function (configuration?: Config localVarQueryParameter['filename'] = filename; } + if (retry !== undefined) { + localVarQueryParameter['retry'] = retry; + } + localVarHeaderParameter['Accept'] = 'application/octet-stream,application/json'; setSearchParams(localVarUrlObj, localVarQueryParameter); @@ -578,16 +585,19 @@ export const RunInternalsApiAxiosParamCreator = function (configuration?: Config * @summary Put Stage Artifact * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. + * @param {number} retry Retry attempt number for the artifact. * @param {File} body * @param {string} [filename] Relative artifact path for `application/octet-stream` uploads. Ignored for multipart uploads. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - putStageArtifact: async (id: string, stageId: string, body: File, filename?: string, options: RawAxiosRequestConfig = {}): Promise => { + putStageArtifact: async (id: string, stageId: string, retry: number, body: File, filename?: string, options: RawAxiosRequestConfig = {}): Promise => { // verify required parameter 'id' is not null or undefined assertParamExists('putStageArtifact', 'id', id) // verify required parameter 'stageId' is not null or undefined assertParamExists('putStageArtifact', 'stageId', stageId) + // verify required parameter 'retry' is not null or undefined + assertParamExists('putStageArtifact', 'retry', retry) // verify required parameter 'body' is not null or undefined assertParamExists('putStageArtifact', 'body', body) const localVarPath = `/api/v1/runs/{id}/stages/{stageId}/artifacts` @@ -610,6 +620,10 @@ export const RunInternalsApiAxiosParamCreator = function (configuration?: Config // http bearer authentication required await setBearerAuthToObject(localVarHeaderParameter, configuration) + if (retry !== undefined) { + localVarQueryParameter['retry'] = retry; + } + if (filename !== undefined) { localVarQueryParameter['filename'] = filename; } @@ -882,11 +896,12 @@ export const RunInternalsApiFp = function(configuration?: Configuration) { * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. * @param {string} filename Relative artifact path. `/` is allowed as a path separator. Backslash, empty segments, and traversal segments (`.` and `..`) are invalid. + * @param {number} retry Retry attempt number for the artifact. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - async getStageArtifact(id: string, stageId: string, filename: string, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { - const localVarAxiosArgs = await localVarAxiosParamCreator.getStageArtifact(id, stageId, filename, options); + async getStageArtifact(id: string, stageId: string, filename: string, retry: number, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { + const localVarAxiosArgs = await localVarAxiosParamCreator.getStageArtifact(id, stageId, filename, retry, options); const localVarOperationServerIndex = configuration?.serverIndex ?? 0; const localVarOperationServerBasePath = operationServerMap['RunInternalsApi.getStageArtifact']?.[localVarOperationServerIndex]?.url; return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); @@ -969,13 +984,14 @@ export const RunInternalsApiFp = function(configuration?: Configuration) { * @summary Put Stage Artifact * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. + * @param {number} retry Retry attempt number for the artifact. * @param {File} body * @param {string} [filename] Relative artifact path for `application/octet-stream` uploads. Ignored for multipart uploads. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - async putStageArtifact(id: string, stageId: string, body: File, filename?: string, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { - const localVarAxiosArgs = await localVarAxiosParamCreator.putStageArtifact(id, stageId, body, filename, options); + async putStageArtifact(id: string, stageId: string, retry: number, body: File, filename?: string, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { + const localVarAxiosArgs = await localVarAxiosParamCreator.putStageArtifact(id, stageId, retry, body, filename, options); const localVarOperationServerIndex = configuration?.serverIndex ?? 0; const localVarOperationServerBasePath = operationServerMap['RunInternalsApi.putStageArtifact']?.[localVarOperationServerIndex]?.url; return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); @@ -1105,11 +1121,12 @@ export const RunInternalsApiFactory = function (configuration?: Configuration, b * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. * @param {string} filename Relative artifact path. `/` is allowed as a path separator. Backslash, empty segments, and traversal segments (`.` and `..`) are invalid. + * @param {number} retry Retry attempt number for the artifact. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - getStageArtifact(id: string, stageId: string, filename: string, options?: RawAxiosRequestConfig): AxiosPromise { - return localVarFp.getStageArtifact(id, stageId, filename, options).then((request) => request(axios, basePath)); + getStageArtifact(id: string, stageId: string, filename: string, retry: number, options?: RawAxiosRequestConfig): AxiosPromise { + return localVarFp.getStageArtifact(id, stageId, filename, retry, options).then((request) => request(axios, basePath)); }, /** * Lists captured artifact files for a run. @@ -1174,13 +1191,14 @@ export const RunInternalsApiFactory = function (configuration?: Configuration, b * @summary Put Stage Artifact * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. + * @param {number} retry Retry attempt number for the artifact. * @param {File} body * @param {string} [filename] Relative artifact path for `application/octet-stream` uploads. Ignored for multipart uploads. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - putStageArtifact(id: string, stageId: string, body: File, filename?: string, options?: RawAxiosRequestConfig): AxiosPromise { - return localVarFp.putStageArtifact(id, stageId, body, filename, options).then((request) => request(axios, basePath)); + putStageArtifact(id: string, stageId: string, retry: number, body: File, filename?: string, options?: RawAxiosRequestConfig): AxiosPromise { + return localVarFp.putStageArtifact(id, stageId, retry, body, filename, options).then((request) => request(axios, basePath)); }, /** * Reads a previously stored blob by identifier. @@ -1298,11 +1316,12 @@ export class RunInternalsApi extends BaseAPI { * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. * @param {string} filename Relative artifact path. `/` is allowed as a path separator. Backslash, empty segments, and traversal segments (`.` and `..`) are invalid. + * @param {number} retry Retry attempt number for the artifact. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - public getStageArtifact(id: string, stageId: string, filename: string, options?: RawAxiosRequestConfig) { - return RunInternalsApiFp(this.configuration).getStageArtifact(id, stageId, filename, options).then((request) => request(this.axios, this.basePath)); + public getStageArtifact(id: string, stageId: string, filename: string, retry: number, options?: RawAxiosRequestConfig) { + return RunInternalsApiFp(this.configuration).getStageArtifact(id, stageId, filename, retry, options).then((request) => request(this.axios, this.basePath)); } /** @@ -1373,13 +1392,14 @@ export class RunInternalsApi extends BaseAPI { * @summary Put Stage Artifact * @param {string} id Unique run identifier (ULID). * @param {string} stageId Identifier of a stage within a run\'s workflow graph, serialized as `node_id@visit`. + * @param {number} retry Retry attempt number for the artifact. * @param {File} body * @param {string} [filename] Relative artifact path for `application/octet-stream` uploads. Ignored for multipart uploads. * @param {*} [options] Override http request option. * @throws {RequiredError} */ - public putStageArtifact(id: string, stageId: string, body: File, filename?: string, options?: RawAxiosRequestConfig) { - return RunInternalsApiFp(this.configuration).putStageArtifact(id, stageId, body, filename, options).then((request) => request(this.axios, this.basePath)); + public putStageArtifact(id: string, stageId: string, retry: number, body: File, filename?: string, options?: RawAxiosRequestConfig) { + return RunInternalsApiFp(this.configuration).putStageArtifact(id, stageId, retry, body, filename, options).then((request) => request(this.axios, this.basePath)); } /** diff --git a/lib/packages/fabro-api-client/src/models/artifact-entry.ts b/lib/packages/fabro-api-client/src/models/artifact-entry.ts index 6fbd0e851..4bd667edc 100644 --- a/lib/packages/fabro-api-client/src/models/artifact-entry.ts +++ b/lib/packages/fabro-api-client/src/models/artifact-entry.ts @@ -15,12 +15,20 @@ /** - * A single artifact filename. + * A single artifact file for a stage. */ export interface ArtifactEntry { /** * Artifact filename. */ 'filename': string; + /** + * Retry attempt number. + */ + 'retry': number; + /** + * Artifact size in bytes. + */ + 'size': number; } diff --git a/lib/packages/fabro-api-client/src/models/artifact-list-response.ts b/lib/packages/fabro-api-client/src/models/artifact-list-response.ts index 708bb4ad5..dcf2bfc90 100644 --- a/lib/packages/fabro-api-client/src/models/artifact-list-response.ts +++ b/lib/packages/fabro-api-client/src/models/artifact-list-response.ts @@ -18,7 +18,7 @@ import type { ArtifactEntry } from './artifact-entry'; /** - * List of artifact filenames for a stage. + * List of artifact files for a stage. */ export interface ArtifactListResponse { 'data': Array; diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index 85bc80c70..df9d1f380 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -133,7 +133,6 @@ export * from './model-limits'; export * from './model-reference'; export * from './model-test-mode'; export * from './model-test-result'; -export * from './node-state'; export * from './node-status-record'; export * from './notification-provider-settings'; export * from './notification-route-settings'; @@ -268,6 +267,7 @@ export * from './ssh-access-request'; export * from './ssh-access-response'; export * from './stage-outcome'; export * from './stage-state'; +export * from './stage-status'; export * from './stage-turn'; export * from './start-run-request'; export * from './submit-answer-request'; diff --git a/lib/packages/fabro-api-client/src/models/model.ts b/lib/packages/fabro-api-client/src/models/model.ts index 834661c41..45751f7e9 100644 --- a/lib/packages/fabro-api-client/src/models/model.ts +++ b/lib/packages/fabro-api-client/src/models/model.ts @@ -73,4 +73,3 @@ export interface Model { } - diff --git a/lib/packages/fabro-api-client/src/models/node-state.ts b/lib/packages/fabro-api-client/src/models/node-state.ts deleted file mode 100644 index df5eb16a1..000000000 --- a/lib/packages/fabro-api-client/src/models/node-state.ts +++ /dev/null @@ -1,45 +0,0 @@ -/* 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. - */ - - -// May contain unused imports in some cases -// @ts-ignore -import type { CommandTermination } from './command-termination'; -// May contain unused imports in some cases -// @ts-ignore -import type { NodeStatusRecord } from './node-status-record'; - -/** - * Internal node projection state. - */ -export interface NodeState { - 'prompt'?: string | null; - 'response'?: string | null; - 'status'?: NodeStatusRecord | null; - 'provider_used'?: any; - 'diff'?: string | null; - 'script_invocation'?: any; - 'script_timing'?: any; - 'parallel_results'?: any; - 'stdout'?: string | null; - 'stderr'?: string | null; - 'stdout_bytes'?: number | null; - 'stderr_bytes'?: number | null; - 'streams_separated'?: boolean | null; - 'live_streaming'?: boolean | null; - 'termination'?: CommandTermination | null; -} - - - diff --git a/lib/packages/fabro-api-client/src/models/run-projection.ts b/lib/packages/fabro-api-client/src/models/run-projection.ts index fed006842..98e758d0b 100644 --- a/lib/packages/fabro-api-client/src/models/run-projection.ts +++ b/lib/packages/fabro-api-client/src/models/run-projection.ts @@ -13,9 +13,6 @@ */ -// May contain unused imports in some cases -// @ts-ignore -import type { NodeState } from './node-state'; // May contain unused imports in some cases // @ts-ignore import type { PendingInterviewRecord } from './pending-interview-record'; @@ -34,6 +31,9 @@ import type { RunSpec } from './run-spec'; // May contain unused imports in some cases // @ts-ignore import type { RunStatus } from './run-status'; +// May contain unused imports in some cases +// @ts-ignore +import type { StageState } from './stage-state'; /** * Raw internal run projection derived from the event log. @@ -60,9 +60,9 @@ export interface RunProjection { 'superseded_by'?: string | null; 'pending_interviews'?: { [key: string]: PendingInterviewRecord; }; /** - * Map from StageId (`node_id@visit`) to NodeState. + * Map from StageId (`node_id@visit`) to StageState. */ - 'nodes': { [key: string]: NodeState; }; + 'stages': { [key: string]: StageState; }; } diff --git a/lib/packages/fabro-api-client/src/models/run-stage.ts b/lib/packages/fabro-api-client/src/models/run-stage.ts index c98ec9bee..575b12c96 100644 --- a/lib/packages/fabro-api-client/src/models/run-stage.ts +++ b/lib/packages/fabro-api-client/src/models/run-stage.ts @@ -15,7 +15,7 @@ // May contain unused imports in some cases // @ts-ignore -import type { StageState } from './stage-state'; +import type { StageStatus } from './stage-status'; /** * A single stage in a run\'s workflow graph. @@ -29,7 +29,7 @@ export interface RunStage { * Human-readable stage name. */ 'name': string; - 'status': StageState; + 'status': StageStatus; /** * Time spent in this stage, in seconds. */ diff --git a/lib/packages/fabro-api-client/src/models/stage-state.ts b/lib/packages/fabro-api-client/src/models/stage-state.ts index 47c1b494a..c7b3e35de 100644 --- a/lib/packages/fabro-api-client/src/models/stage-state.ts +++ b/lib/packages/fabro-api-client/src/models/stage-state.ts @@ -13,23 +13,37 @@ */ +// May contain unused imports in some cases +// @ts-ignore +import type { CommandTermination } from './command-termination'; +// May contain unused imports in some cases +// @ts-ignore +import type { NodeStatusRecord } from './node-status-record'; /** - * Lifecycle projection state of a workflow stage. + * Internal stage projection state. */ - -export const StageState = { - PENDING: 'pending', - RUNNING: 'running', - RETRYING: 'retrying', - SUCCEEDED: 'succeeded', - PARTIALLY_SUCCEEDED: 'partially_succeeded', - FAILED: 'failed', - SKIPPED: 'skipped', - CANCELLED: 'cancelled' -} as const; - -export type StageState = typeof StageState[keyof typeof StageState]; +export interface StageState { + /** + * Event-log sequence number of the first event that created this stage. Used to order stages by execution. + */ + 'seq'?: number; + 'prompt'?: string | null; + 'response'?: string | null; + 'status'?: NodeStatusRecord | null; + 'provider_used'?: any; + 'diff'?: string | null; + 'script_invocation'?: any; + 'script_timing'?: any; + 'parallel_results'?: any; + 'stdout'?: string | null; + 'stderr'?: string | null; + 'stdout_bytes'?: number | null; + 'stderr_bytes'?: number | null; + 'streams_separated'?: boolean | null; + 'live_streaming'?: boolean | null; + 'termination'?: CommandTermination | null; +} diff --git a/lib/packages/fabro-api-client/src/models/stage-status.ts b/lib/packages/fabro-api-client/src/models/stage-status.ts new file mode 100644 index 000000000..33e31f774 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/stage-status.ts @@ -0,0 +1,32 @@ +/* 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. + */ + + + +/** + * Lifecycle projection state of a workflow stage. + */ + +export const StageStatus = { + PENDING: 'pending', + RUNNING: 'running', + RETRYING: 'retrying', + SUCCEEDED: 'succeeded', + PARTIALLY_SUCCEEDED: 'partially_succeeded', + FAILED: 'failed', + SKIPPED: 'skipped', + CANCELLED: 'cancelled' +} as const; + +export type StageStatus = typeof StageStatus[keyof typeof StageStatus]; From b01a666cd94c2fdd94a5cfd59e448d7b14e7eb0d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 1 May 2026 20:55:56 -0400 Subject: [PATCH 2/7] refactor: simplify retry-related helpers and orphan dump scan Remove unused RunProjection::stage_mut, share decode_retry_and_filename between artifact_store decoders, reuse stage_visit() in the artifact lifecycle, and cache the dump.log entry index so RunDump::add_orphan_notice no longer rescans entries on every call. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-dump/src/lib.rs | 22 +++++----- lib/crates/fabro-store/src/artifact_store.rs | 40 +++++++++---------- lib/crates/fabro-types/src/run_projection.rs | 4 -- .../fabro-workflow/src/lifecycle/artifact.rs | 5 +-- .../fabro-workflow/src/lifecycle/event.rs | 4 +- 5 files changed, 35 insertions(+), 40 deletions(-) diff --git a/lib/crates/fabro-dump/src/lib.rs b/lib/crates/fabro-dump/src/lib.rs index 16954bfe9..48f3d665c 100644 --- a/lib/crates/fabro-dump/src/lib.rs +++ b/lib/crates/fabro-dump/src/lib.rs @@ -23,8 +23,9 @@ pub type BlobReader = Box BoxFuture<'static, Result, - stage_ranks: HashMap, + entries: Vec, + stage_ranks: HashMap, + dump_log_index: Option, } #[derive(Debug, Clone)] @@ -151,6 +152,7 @@ impl RunDump { Ok(Self { entries, stage_ranks, + dump_log_index: None, }) } @@ -252,16 +254,15 @@ impl RunDump { fn add_orphan_notice(&mut self, stage_id: &StageId) { let line = format!("notice: artifact stage {stage_id} was not present in run projection\n"); - if let Some(entry) = self - .entries - .iter_mut() - .find(|entry| entry.path == "dump.log") - { - if let RunDumpContents::Text(text) = &mut entry.contents { + if let Some(index) = self.dump_log_index { + if let Some(RunDumpContents::Text(text)) = + self.entries.get_mut(index).map(|entry| &mut entry.contents) + { text.push_str(&line); return; } } + self.dump_log_index = Some(self.entries.len()); self.entries.push(RunDumpEntry::text("dump.log", line)); } @@ -779,11 +780,12 @@ mod tests { let blob_id = fabro_types::RunBlobId::new(&blob); let legacy_ref = format!("file:///sandbox/.fabro/artifacts/{blob_id}.json"); let mut dump = RunDump { - entries: vec![RunDumpEntry::json( + entries: vec![RunDumpEntry::json( "run.json", serde_json::json!({ "stdout": legacy_ref }), )], - stage_ranks: HashMap::new(), + stage_ranks: HashMap::new(), + dump_log_index: None, }; executor::block_on(async { diff --git a/lib/crates/fabro-store/src/artifact_store.rs b/lib/crates/fabro-store/src/artifact_store.rs index 8cc244131..25ba1a55d 100644 --- a/lib/crates/fabro-store/src/artifact_store.rs +++ b/lib/crates/fabro-store/src/artifact_store.rs @@ -294,24 +294,11 @@ fn decode_artifact_location( "artifact location {location} has an invalid visit number: {err}" )) })?; - let retry_part = parts.next().ok_or_else(|| { - Error::Other(format!( - "artifact location {location} is missing a retry segment" - )) - })?; - let retry = decode_retry_segment(location, retry_part.as_ref())?; - let filename_segments = parts - .map(|part| decode_path_segment("artifact filename segment", part.as_ref())) - .collect::>>()?; - if filename_segments.is_empty() { - return Err(Error::Other(format!( - "artifact location {location} is missing a filename" - ))); - } + let (retry, filename) = decode_retry_and_filename(location, &mut parts)?; Ok(NodeArtifact { node: StageId::new(node_id, visit), retry, - filename: filename_segments.join("/"), + filename, size, }) } @@ -326,6 +313,22 @@ fn decode_stage_artifact_entry( "artifact location {location} does not match expected prefix {prefix}" )) })?; + let (retry, filename) = decode_retry_and_filename(location, &mut parts)?; + Ok(StageArtifactEntry { + retry, + filename, + size, + }) +} + +fn decode_retry_and_filename<'a, I, P>( + location: &ObjectPath, + parts: &mut I, +) -> Result<(u32, String)> +where + I: Iterator, + P: AsRef + 'a, +{ let retry_part = parts.next().ok_or_else(|| { Error::Other(format!( "artifact location {location} is missing a retry segment" @@ -333,7 +336,6 @@ fn decode_stage_artifact_entry( })?; let retry = decode_retry_segment(location, retry_part.as_ref())?; let filename_segments = parts - .by_ref() .map(|part| decode_path_segment("artifact filename segment", part.as_ref())) .collect::>>()?; if filename_segments.is_empty() { @@ -341,11 +343,7 @@ fn decode_stage_artifact_entry( "artifact location {location} is missing a filename" ))); } - Ok(StageArtifactEntry { - retry, - filename: filename_segments.join("/"), - size, - }) + Ok((retry, filename_segments.join("/"))) } fn decode_retry_segment(location: &ObjectPath, segment: &str) -> Result { diff --git a/lib/crates/fabro-types/src/run_projection.rs b/lib/crates/fabro-types/src/run_projection.rs index bba87e156..67cf7de4c 100644 --- a/lib/crates/fabro-types/src/run_projection.rs +++ b/lib/crates/fabro-types/src/run_projection.rs @@ -113,10 +113,6 @@ impl RunProjection { &self.pending_interviews } - pub fn stage_mut(&mut self, node_id: &str, visit: u32) -> &mut StageState { - self.stage_entry(node_id, visit, 0) - } - pub fn stage_entry_id(&mut self, stage_id: &StageId, seq: u32) -> &mut StageState { self.stages .entry(stage_id.clone()) diff --git a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs index 24f5f8c90..f65fcee86 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs @@ -18,7 +18,7 @@ use crate::artifact_snapshot::collect_artifacts; use crate::artifact_upload::ArtifactSink; use crate::event::{Emitter, Event, RunNoticeLevel}; use crate::graph::{WorkflowGraph, WorkflowNode}; -use crate::lifecycle::event::stage_scope_for; +use crate::lifecycle::event::{stage_scope_for, stage_visit}; use crate::outcome::BilledModelUsage; use crate::runtime_store::RunStoreHandle; @@ -95,8 +95,7 @@ impl RunLifecycle for ArtifactLifecycle { } let epoch = self.attempt_start_epoch.lock().unwrap().unwrap_or(0.0); let node_id = ctx.node.id(); - let visit_count = state.node_visits.get(node_id).copied().unwrap_or(1); - let visit = u32::try_from(visit_count.max(1)).unwrap_or(u32::MAX); + let visit = stage_visit(state, node_id); let node_slug = if visit <= 1 { node_id.to_string() } else { diff --git a/lib/crates/fabro-workflow/src/lifecycle/event.rs b/lib/crates/fabro-workflow/src/lifecycle/event.rs index 4dc95e512..d97d7f613 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/event.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/event.rs @@ -74,9 +74,9 @@ fn response_from_outcome(node_id: &str, outcome: &Outcome) -> Option { .and_then(|value| value.as_str().map(ToOwned::to_owned)) } -fn stage_visit(state: &WfRunState, node_id: &str) -> u32 { +pub(super) fn stage_visit(state: &WfRunState, node_id: &str) -> u32 { let visits = state.node_visits.get(node_id).copied().unwrap_or(1); - u32::try_from(visits.max(1)).unwrap_or(u32::MAX) + u32::try_from(visits).unwrap_or(u32::MAX) } pub(crate) fn stage_scope_for(state: &WfRunState, node_id: &str) -> StageScope { From 576c43d2161d9f1909aa02e0eb34c45221294e92 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 1 May 2026 21:13:21 -0400 Subject: [PATCH 3/7] refactor(dump): bind rank width to a single source and drop dead retry validation Extract STAGE_RANK_WIDTH and a derived MAX_STAGES_IN_DUMP in fabro-dump so the path-prefix format and the stage-count cap can't drift, and replace the two `{rank:03}-...` literals with a shared stage_dir_name helper. Replace the cli/dump.rs `u32::try_from(artifact.retry)` with the symmetric inverse of the server's `cast_signed()` emit. The OpenAPI schema declares `minimum: 0`, so the negative branch is unreachable. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/commands/dump.rs | 3 +-- lib/crates/fabro-dump/src/lib.rs | 23 +++++++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/dump.rs b/lib/crates/fabro-cli/src/commands/dump.rs index 2d9bc0f57..81201fdc5 100644 --- a/lib/crates/fabro-cli/src/commands/dump.rs +++ b/lib/crates/fabro-cli/src/commands/dump.rs @@ -96,8 +96,7 @@ async fn write_run_dump( .stage_id .parse() .with_context(|| format!("server returned invalid stage id {:?}", artifact.stage_id))?; - let retry = u32::try_from(artifact.retry) - .context("server returned invalid negative artifact retry")?; + let retry = artifact.retry.cast_unsigned(); let data = client .download_stage_artifact(run_id, &stage_id, retry, &artifact.relative_path) .await diff --git a/lib/crates/fabro-dump/src/lib.rs b/lib/crates/fabro-dump/src/lib.rs index 48f3d665c..d9d00740c 100644 --- a/lib/crates/fabro-dump/src/lib.rs +++ b/lib/crates/fabro-dump/src/lib.rs @@ -21,6 +21,21 @@ use futures::future::BoxFuture; pub type BlobReader = Box BoxFuture<'static, Result>> + Send>; +const STAGE_RANK_WIDTH: usize = 3; +const MAX_STAGES_IN_DUMP: usize = { + let mut value = 1usize; + let mut i = 0usize; + while i < STAGE_RANK_WIDTH { + value *= 10; + i += 1; + } + value - 1 +}; + +fn stage_dir_name(rank: u32, stage_id: &StageId) -> String { + format!("{rank:0>STAGE_RANK_WIDTH$}-{stage_id}") +} + #[derive(Debug, Clone)] pub struct RunDump { entries: Vec, @@ -55,9 +70,9 @@ impl RunDump { .iter_stages() .map(|(stage_id, stage)| (stage_id.clone(), stage.seq)) .collect(); - if stage_order.len() > 999 { + if stage_order.len() > MAX_STAGES_IN_DUMP { bail!( - "run dump supports at most 999 stages with the current path prefix width (got {})", + "run dump supports at most {MAX_STAGES_IN_DUMP} stages with the current path prefix width (got {})", stage_order.len() ); } @@ -78,7 +93,7 @@ impl RunDump { .get(&stage_id) .copied() .context("stage rank should exist")?; - let base = PathBuf::from("stages").join(format!("{rank:03}-{stage_id}")); + let base = PathBuf::from("stages").join(stage_dir_name(rank, &stage_id)); if let Some(prompt) = node.prompt.as_ref() { entries.push(RunDumpEntry::text_path( @@ -458,7 +473,7 @@ fn artifact_dump_path( let filename_path = validate_relative_path("artifact filename", filename)?; let stage_dir = stage_ranks.get(stage_id).map_or_else( || PathBuf::from("_orphans").join(stage_id.to_string()), - |rank| PathBuf::from(format!("{rank:03}-{stage_id}")), + |rank| PathBuf::from(stage_dir_name(*rank, stage_id)), ); Ok(PathBuf::from("artifacts") .join(stage_dir) From cc81f538f473dd05299282931572ab02c08d4db4 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 1 May 2026 21:16:56 -0400 Subject: [PATCH 4/7] refactor(api): type StageState JSON-blob fields so the TS client stops emitting any provider_used, script_invocation, and script_timing become object | null; parallel_results becomes Array | null. The Rust StageState type is unaffected because fabro-api/build.rs replaces it with fabro_types::StageState. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/public/api-reference/fabro-api.yaml | 18 +++++++++++++---- .../fabro-api-client/src/models/model.ts | 3 ++- .../src/models/stage-state.ts | 20 +++++++++++++++---- .../src/models/stage-status.ts | 5 ++++- 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 9061603c4..bae632f8e 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -5137,12 +5137,22 @@ components: oneOf: - $ref: "#/components/schemas/NodeStatusRecord" - type: "null" - provider_used: {} + provider_used: + type: ["object", "null"] + description: Provider and model metadata recorded for the stage attempt. diff: type: ["string", "null"] - script_invocation: {} - script_timing: {} - parallel_results: {} + script_invocation: + type: ["object", "null"] + description: Command and environment recorded when the stage script ran. + script_timing: + type: ["object", "null"] + description: Wall-clock and step timing metadata for the stage script. + parallel_results: + type: ["array", "null"] + items: + type: object + description: Per-branch result objects produced by a parallel stage. stdout: type: ["string", "null"] stderr: diff --git a/lib/packages/fabro-api-client/src/models/model.ts b/lib/packages/fabro-api-client/src/models/model.ts index 45751f7e9..779786680 100644 --- a/lib/packages/fabro-api-client/src/models/model.ts +++ b/lib/packages/fabro-api-client/src/models/model.ts @@ -67,9 +67,10 @@ export interface Model { */ 'default': boolean; /** - * Whether credential material is present for this model\'s provider on the server (vault entry or environment variable). Does NOT imply the credential is valid or that requests will succeed; call `POST /models/{id}/test` to verify usability. + * Whether credential material is present for this model\'s provider on the server (vault entry or environment variable). Does NOT imply the credential is valid or that requests will succeed; call `POST /models/{id}/test` to verify usability. */ 'configured': boolean; } + diff --git a/lib/packages/fabro-api-client/src/models/stage-state.ts b/lib/packages/fabro-api-client/src/models/stage-state.ts index c7b3e35de..7838a276c 100644 --- a/lib/packages/fabro-api-client/src/models/stage-state.ts +++ b/lib/packages/fabro-api-client/src/models/stage-state.ts @@ -31,11 +31,23 @@ export interface StageState { 'prompt'?: string | null; 'response'?: string | null; 'status'?: NodeStatusRecord | null; - 'provider_used'?: any; + /** + * Provider and model metadata recorded for the stage attempt. + */ + 'provider_used'?: object | null; 'diff'?: string | null; - 'script_invocation'?: any; - 'script_timing'?: any; - 'parallel_results'?: any; + /** + * Command and environment recorded when the stage script ran. + */ + 'script_invocation'?: object | null; + /** + * Wall-clock and step timing metadata for the stage script. + */ + 'script_timing'?: object | null; + /** + * Per-branch result objects produced by a parallel stage. + */ + 'parallel_results'?: Array | null; 'stdout'?: string | null; 'stderr'?: string | null; 'stdout_bytes'?: number | null; diff --git a/lib/packages/fabro-api-client/src/models/stage-status.ts b/lib/packages/fabro-api-client/src/models/stage-status.ts index 33e31f774..0756f0150 100644 --- a/lib/packages/fabro-api-client/src/models/stage-status.ts +++ b/lib/packages/fabro-api-client/src/models/stage-status.ts @@ -5,7 +5,7 @@ * 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 @@ -30,3 +30,6 @@ export const StageStatus = { } as const; export type StageStatus = typeof StageStatus[keyof typeof StageStatus]; + + + From 5393b12beb57fb8b3ef562f65859f3f7b79b5730 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 1 May 2026 21:33:39 -0400 Subject: [PATCH 5/7] refactor: skip linear scans and serial awaits in run state and finalize Defer current_visit_for to the fallback branch in stage_entry_with_current_visit so events that already carry stage_id avoid an O(N stages) scan per event. Run state() and list_events() concurrently in build_conclusion_from_store, and share a single retry- segment prefix between encode and decode. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-store/src/artifact_store.rs | 6 ++-- lib/crates/fabro-store/src/run_state.rs | 28 ++++++++++++------- .../fabro-workflow/src/pipeline/finalize.rs | 11 ++------ 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/lib/crates/fabro-store/src/artifact_store.rs b/lib/crates/fabro-store/src/artifact_store.rs index 25ba1a55d..4cd4f85e1 100644 --- a/lib/crates/fabro-store/src/artifact_store.rs +++ b/lib/crates/fabro-store/src/artifact_store.rs @@ -256,9 +256,11 @@ pub fn stage_storage_segment(node: &StageId) -> String { ) } +const RETRY_SEGMENT_PREFIX: &str = "retry-"; + #[must_use] pub fn retry_storage_segment(retry: u32) -> String { - format!("retry-{retry:04}") + format!("{RETRY_SEGMENT_PREFIX}{retry:04}") } fn decode_path_segment(kind: &str, value: &str) -> Result { @@ -347,7 +349,7 @@ where } fn decode_retry_segment(location: &ObjectPath, segment: &str) -> Result { - let Some(value) = segment.strip_prefix("retry-") else { + let Some(value) = segment.strip_prefix(RETRY_SEGMENT_PREFIX) else { return Err(Error::Other(format!( "artifact location {location} has an invalid retry segment" ))); diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index e617c9c21..2d778ed94 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -285,8 +285,7 @@ impl RunProjectionReducer for RunProjection { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - let visit = self.current_visit_for(node_id).unwrap_or(1); - stage_entry_for_event(self, event, node_id, visit).response = + stage_entry_with_current_visit(self, event, node_id).response = Some(props.response.clone()); } EventBody::StageCompleted(props) => { @@ -305,9 +304,8 @@ impl RunProjectionReducer for RunProjection { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - let visit = self.current_visit_for(node_id).unwrap_or(1); let failure_reason = props.failure.as_ref().map(|detail| detail.message.clone()); - let node = stage_entry_for_event(self, event, node_id, visit); + let node = stage_entry_with_current_visit(self, event, node_id); node.status = Some(NodeStatusRecord { status: StageOutcome::Failed { retry_requested: false, @@ -335,8 +333,7 @@ impl RunProjectionReducer for RunProjection { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - let visit = self.current_visit_for(node_id).unwrap_or(1); - stage_entry_for_event(self, event, node_id, visit).script_invocation = + stage_entry_with_current_visit(self, event, node_id).script_invocation = Some(serde_json::to_value(props).map_err(|err| { Error::InvalidEvent(format!("invalid command.started payload: {err}")) })?); @@ -345,8 +342,7 @@ impl RunProjectionReducer for RunProjection { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - let visit = self.current_visit_for(node_id).unwrap_or(1); - let node = stage_entry_for_event(self, event, node_id, visit); + let node = stage_entry_with_current_visit(self, event, node_id); node.stdout = Some(props.stdout.clone()); node.stderr = Some(props.stderr.clone()); node.stdout_bytes = Some(props.stdout_bytes); @@ -362,8 +358,7 @@ impl RunProjectionReducer for RunProjection { let Some(node_id) = stored.node_id.as_deref() else { return Ok(()); }; - let visit = self.current_visit_for(node_id).unwrap_or(1); - stage_entry_for_event(self, event, node_id, visit).parallel_results = + stage_entry_with_current_visit(self, event, node_id).parallel_results = Some(serde_json::to_value(&props.results).map_err(|err| { Error::InvalidEvent(format!("invalid parallel.completed payload: {err}")) })?); @@ -388,6 +383,19 @@ fn stage_entry_for_event<'a>( } } +fn stage_entry_with_current_visit<'a>( + state: &'a mut RunProjection, + event: &EventEnvelope, + node_id: &str, +) -> &'a mut fabro_types::StageState { + if let Some(stage_id) = event.event.stage_id.as_ref() { + state.stage_entry_id(stage_id, event.seq) + } else { + let visit = state.current_visit_for(node_id).unwrap_or(1); + state.stage_entry(node_id, visit, event.seq) + } +} + pub(crate) fn build_summary(state: &RunProjection, run_id: &RunId) -> RunSummary { let workflow_name = state.spec.as_ref().map(|spec| { if spec.graph.name.is_empty() { diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 1e509ddea..5d1c34979 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -67,14 +67,9 @@ pub(crate) async fn build_conclusion_from_store( run_duration_ms: u64, final_git_commit_sha: Option, ) -> Conclusion { - let checkpoint = run_store - .state() - .await - .ok() - .and_then(|state| state.checkpoint); - let stage_durations = run_store - .list_events() - .await + let (state_result, events_result) = tokio::join!(run_store.state(), run_store.list_events()); + let checkpoint = state_result.ok().and_then(|state| state.checkpoint); + let stage_durations = events_result .map(|events| crate::extract_stage_durations_from_events(&events)) .unwrap_or_default(); From 8f4c12580cb5d807bbd829d23252a265cf17dc87 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 2 May 2026 01:16:10 -0400 Subject: [PATCH 6/7] refactor: dedupe artifact entry adapters, retry URL helper, query-param check Extract run_artifact_entry_from / artifact_entry_from in fabro-server so the two list-artifact handlers share a single conversion site. Push the ?retry=... query append into stage_artifacts_url in fabro-client so upload callers don't repeat it. Replace required_filename and required_retry with one generic required_query_param helper. (From impls were the cleaner shape but the orphan rule blocks them: NodeArtifact lives in fabro-store, RunArtifactEntry in fabro-api, neither is in fabro-server. Free fns achieve the same dedup without adding a fabro-store -> fabro-api coupling.) Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-client/src/client.rs | 17 ++++--- lib/crates/fabro-server/src/server.rs | 72 ++++++++++++--------------- 2 files changed, 41 insertions(+), 48 deletions(-) diff --git a/lib/crates/fabro-client/src/client.rs b/lib/crates/fabro-client/src/client.rs index 36cd3f489..86a63375c 100644 --- a/lib/crates/fabro-client/src/client.rs +++ b/lib/crates/fabro-client/src/client.rs @@ -1228,7 +1228,12 @@ impl Client { clippy::disallowed_types, reason = "Client builds raw server API request URLs for wire transit; logging redaction is handled at log boundaries." )] - fn stage_artifacts_url(&self, run_id: &RunId, stage_id: &StageId) -> Result { + fn stage_artifacts_url( + &self, + run_id: &RunId, + stage_id: &StageId, + retry: u32, + ) -> Result { let base_url = self.base_url(); let mut url = fabro_http::Url::parse(&base_url) .with_context(|| format!("invalid server base URL {base_url}"))?; @@ -1243,6 +1248,8 @@ impl Client { &stage_id.to_string(), "artifacts", ]); + url.query_pairs_mut() + .append_pair("retry", &retry.to_string()); Ok(url) } @@ -1255,10 +1262,8 @@ impl Client { path: &Path, bearer_token: &str, ) -> Result<()> { - let mut url = self.stage_artifacts_url(run_id, stage_id)?; + let mut url = self.stage_artifacts_url(run_id, stage_id, retry)?; url.query_pairs_mut().append_pair("filename", filename); - url.query_pairs_mut() - .append_pair("retry", &retry.to_string()); let file = File::open(path) .await @@ -1296,9 +1301,7 @@ impl Client { artifacts: &[ArtifactUpload], bearer_token: &str, ) -> Result<()> { - let mut url = self.stage_artifacts_url(run_id, stage_id)?; - url.query_pairs_mut() - .append_pair("retry", &retry.to_string()); + let url = self.stage_artifacts_url(run_id, stage_id, retry)?; let mut manifest_entries = Vec::with_capacity(artifacts.len()); let mut file_parts = Vec::with_capacity(artifacts.len()); diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index ed9c84d00..58a8d0419 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -63,8 +63,8 @@ use fabro_slack::threads::ThreadRegistry; use fabro_slack::{blocks as slack_blocks, connection as slack_connection}; use fabro_static::EnvVars; use fabro_store::{ - ArtifactKey, ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, - StageId, + ArtifactKey, ArtifactStore, Database, EventEnvelope, EventPayload, NodeArtifact, + PendingInterviewRecord, StageArtifactEntry, StageId, }; #[cfg(test)] use fabro_types::BlockedReason; @@ -3353,24 +3353,12 @@ pub(crate) fn parse_blob_id_path(blob_id: &str) -> Result { #[allow( clippy::result_large_err, - reason = "Missing filename validation returns HTTP 400 responses directly." + reason = "Missing query parameter validation returns HTTP 400 responses directly." )] -fn required_filename(params: &ArtifactFilenameParams) -> Result { - match params.filename.as_ref() { - Some(filename) if !filename.is_empty() => Ok(filename.clone()), - _ => Err(ApiError::bad_request("Missing filename query parameter.").into_response()), - } -} - -#[allow( - clippy::result_large_err, - reason = "Missing retry validation returns HTTP 400 responses directly." -)] -fn required_retry(params: &ArtifactFilenameParams) -> Result { - match params.retry { - Some(retry) => Ok(retry), - None => Err(ApiError::bad_request("Missing retry query parameter.").into_response()), - } +fn required_query_param(value: Option<&T>, name: &str) -> Result { + value.cloned().ok_or_else(|| { + ApiError::bad_request(format!("Missing {name} query parameter.")).into_response() + }) } #[allow( @@ -6191,16 +6179,7 @@ async fn list_run_artifacts( match state.artifact_store.list_for_run(&id).await { Ok(entries) => Json(RunArtifactListResponse { - data: entries - .into_iter() - .map(|entry| RunArtifactEntry { - stage_id: entry.node.to_string(), - node_slug: entry.node.node_id().to_string(), - retry: entry.retry.cast_signed(), - relative_path: entry.filename, - size: entry.size.cast_signed(), - }) - .collect(), + data: entries.into_iter().map(run_artifact_entry_from).collect(), }) .into_response(), Err(err) => { @@ -6209,6 +6188,24 @@ async fn list_run_artifacts( } } +fn run_artifact_entry_from(entry: NodeArtifact) -> RunArtifactEntry { + RunArtifactEntry { + stage_id: entry.node.to_string(), + node_slug: entry.node.node_id().to_string(), + retry: entry.retry.cast_signed(), + relative_path: entry.filename, + size: entry.size.cast_signed(), + } +} + +fn artifact_entry_from(entry: StageArtifactEntry) -> ArtifactEntry { + ArtifactEntry { + filename: entry.filename, + retry: entry.retry.cast_signed(), + size: entry.size.cast_signed(), + } +} + async fn list_stage_artifacts( _auth: AuthenticatedService, State(state): State>, @@ -6228,14 +6225,7 @@ async fn list_stage_artifacts( match state.artifact_store.list_for_node(&id, &stage_id).await { Ok(entries) => Json(ArtifactListResponse { - data: entries - .into_iter() - .map(|entry| ArtifactEntry { - filename: entry.filename, - retry: entry.retry.cast_signed(), - size: entry.size.cast_signed(), - }) - .collect(), + data: entries.into_iter().map(artifact_entry_from).collect(), }) .into_response(), Err(err) => { @@ -6614,7 +6604,7 @@ async fn put_stage_artifact( if let Err(response) = load_run_spec(state.as_ref(), &id).await.map(|_| ()) { return response; } - let retry = match required_retry(¶ms) { + let retry = match required_query_param(params.retry.as_ref(), "retry") { Ok(retry) => retry, Err(response) => return response, }; @@ -6625,7 +6615,7 @@ async fn put_stage_artifact( }; match artifact_upload_content_type(&parts.headers) { Ok(ArtifactUploadContentType::OctetStream) => { - let filename = match required_filename(¶ms) { + let filename = match required_query_param(params.filename.as_ref(), "filename") { Ok(filename) => filename, Err(response) => return response, }; @@ -6667,11 +6657,11 @@ async fn get_stage_artifact( Ok(stage_id) => stage_id, Err(response) => return response, }; - let filename = match required_filename(¶ms) { + let filename = match required_query_param(params.filename.as_ref(), "filename") { Ok(filename) => filename, Err(response) => return response, }; - let retry = match required_retry(¶ms) { + let retry = match required_query_param(params.retry.as_ref(), "retry") { Ok(retry) => retry, Err(response) => return response, }; From 349200f0566640b67b6a213d30ade38c80a50026 Mon Sep 17 00:00:00 2001 From: "fabro-releases[bot]" Date: Sat, 2 May 2026 09:39:00 +0000 Subject: [PATCH 7/7] Bump version to 0.221.0-nightly.0 --- Cargo.lock | 86 +++++++++++++++++++++++++++--------------------------- Cargo.toml | 2 +- 2 files changed, 44 insertions(+), 44 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d7a7d8ac1..8a1400250 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1536,7 +1536,7 @@ dependencies = [ [[package]] name = "fabro-agent" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -1575,7 +1575,7 @@ dependencies = [ [[package]] name = "fabro-api" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "chrono", "fabro-config", @@ -1596,7 +1596,7 @@ dependencies = [ [[package]] name = "fabro-auth" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -1620,7 +1620,7 @@ dependencies = [ [[package]] name = "fabro-checkpoint" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "chrono", "fabro-config", @@ -1636,7 +1636,7 @@ dependencies = [ [[package]] name = "fabro-cli" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -1732,7 +1732,7 @@ dependencies = [ [[package]] name = "fabro-client" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -1761,7 +1761,7 @@ dependencies = [ [[package]] name = "fabro-config" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -1788,7 +1788,7 @@ dependencies = [ [[package]] name = "fabro-core" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "async-trait", "fabro-types", @@ -1803,7 +1803,7 @@ dependencies = [ [[package]] name = "fabro-dev" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -1823,7 +1823,7 @@ dependencies = [ [[package]] name = "fabro-devcontainer" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "fabro-http", "fabro-static", @@ -1840,7 +1840,7 @@ dependencies = [ [[package]] name = "fabro-dump" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -1854,7 +1854,7 @@ dependencies = [ [[package]] name = "fabro-github" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -1876,7 +1876,7 @@ dependencies = [ [[package]] name = "fabro-graphviz" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "fabro-types", @@ -1890,7 +1890,7 @@ dependencies = [ [[package]] name = "fabro-hooks" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "async-trait", "fabro-agent", @@ -1914,7 +1914,7 @@ dependencies = [ [[package]] name = "fabro-http" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "fabro-static", "http", @@ -1924,7 +1924,7 @@ dependencies = [ [[package]] name = "fabro-install" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -1939,7 +1939,7 @@ dependencies = [ [[package]] name = "fabro-interview" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "async-trait", "dialoguer", @@ -1954,7 +1954,7 @@ dependencies = [ [[package]] name = "fabro-llm" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -1986,7 +1986,7 @@ dependencies = [ [[package]] name = "fabro-macros" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "clap", "fabro-options-metadata", @@ -1997,7 +1997,7 @@ dependencies = [ [[package]] name = "fabro-mcp" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "fabro-config", @@ -2013,7 +2013,7 @@ dependencies = [ [[package]] name = "fabro-model" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "fabro-static", "insta", @@ -2024,7 +2024,7 @@ dependencies = [ [[package]] name = "fabro-oauth" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "axum", @@ -2046,7 +2046,7 @@ dependencies = [ [[package]] name = "fabro-options-metadata" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "serde", "serde_json", @@ -2054,7 +2054,7 @@ dependencies = [ [[package]] name = "fabro-proc" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "cc", "libc", @@ -2063,7 +2063,7 @@ dependencies = [ [[package]] name = "fabro-redact" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "aho-corasick", "ref-cast", @@ -2079,7 +2079,7 @@ dependencies = [ [[package]] name = "fabro-retro" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2098,7 +2098,7 @@ dependencies = [ [[package]] name = "fabro-sandbox" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2134,7 +2134,7 @@ dependencies = [ [[package]] name = "fabro-server" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2215,7 +2215,7 @@ dependencies = [ [[package]] name = "fabro-slack" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "fabro-http", "fabro-interview", @@ -2236,18 +2236,18 @@ dependencies = [ [[package]] name = "fabro-spa" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "rust-embed", ] [[package]] name = "fabro-static" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" [[package]] name = "fabro-store" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "async-trait", "bytes", @@ -2274,7 +2274,7 @@ dependencies = [ [[package]] name = "fabro-telemetry" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -2300,7 +2300,7 @@ dependencies = [ [[package]] name = "fabro-template" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "fabro-util", @@ -2312,7 +2312,7 @@ dependencies = [ [[package]] name = "fabro-test" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "assert_cmd", "axum", @@ -2335,7 +2335,7 @@ dependencies = [ [[package]] name = "fabro-tracker" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "async-trait", "fabro-github", @@ -2348,7 +2348,7 @@ dependencies = [ [[package]] name = "fabro-types" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "chrono", "clap", @@ -2369,7 +2369,7 @@ dependencies = [ [[package]] name = "fabro-util" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "console 0.15.11", @@ -2389,7 +2389,7 @@ dependencies = [ [[package]] name = "fabro-validate" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "fabro-graphviz", "fabro-model", @@ -2399,7 +2399,7 @@ dependencies = [ [[package]] name = "fabro-vault" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "chrono", "fabro-types", @@ -2411,7 +2411,7 @@ dependencies = [ [[package]] name = "fabro-workflow" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -7161,7 +7161,7 @@ dependencies = [ [[package]] name = "twin-github" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "axum", "base64", @@ -7180,7 +7180,7 @@ dependencies = [ [[package]] name = "twin-openai" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" dependencies = [ "anyhow", "async-stream", diff --git a/Cargo.toml b/Cargo.toml index 145892365..9a3a113f4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ resolver = "2" [workspace.package] edition = "2021" -version = "0.220.0-nightly.2" +version = "0.221.0-nightly.0" license = "MIT" [workspace.dependencies]