From bebbcf38297e190e83190c9e1aa5ea49d2854b4c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 5 May 2026 09:10:22 -0400 Subject: [PATCH] fabro(01KQT1V8JM80VEWG4ZQYJC885G): strengthen notice handling Add typed run notice codes for internal emitters, type local pipe drain stream names, and only warn about skipped non-git worktrees when worktree mode was explicitly set to always. --- lib/crates/fabro-cli/src/commands/run/logs.rs | 51 ++++++-- .../src/commands/run/run_progress/event.rs | 7 +- .../src/commands/run/run_progress/mod.rs | 14 +- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 6 +- lib/crates/fabro-cli/tests/it/cmd/run.rs | 2 +- .../tests/it/workflow/dry_run_examples.rs | 10 +- lib/crates/fabro-sandbox/src/local.rs | 22 +--- lib/crates/fabro-types/src/lib.rs | 2 +- lib/crates/fabro-types/src/run_event/infra.rs | 53 ++++++-- lib/crates/fabro-types/src/run_event/mod.rs | 4 +- lib/crates/fabro-workflow/src/event.rs | 2 +- .../fabro-workflow/src/event/convert.rs | 6 +- .../fabro-workflow/src/event/emitter.rs | 15 +-- .../fabro-workflow/src/handler/parallel.rs | 4 +- .../fabro-workflow/src/lifecycle/artifact.rs | 10 +- .../fabro-workflow/src/lifecycle/git.rs | 39 +++--- .../fabro-workflow/src/pipeline/finalize.rs | 42 ++++-- .../fabro-workflow/src/pipeline/initialize.rs | 123 ++++-------------- .../src/pipeline/pull_request.rs | 4 +- 19 files changed, 214 insertions(+), 202 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/run/logs.rs b/lib/crates/fabro-cli/src/commands/run/logs.rs index 8f01378cc..7d7e9063b 100644 --- a/lib/crates/fabro-cli/src/commands/run/logs.rs +++ b/lib/crates/fabro-cli/src/commands/run/logs.rs @@ -14,7 +14,7 @@ use std::time::Duration; use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; use fabro_redact::redact_jsonl_line; -use fabro_types::run_event::is_metadata_snapshot_compat_notice_code; +use fabro_types::RunNoticeCode; use fabro_util::json::normalize_json_value; use fabro_util::terminal::Styles; use tokio::time; @@ -801,7 +801,9 @@ fn format_event_pretty_value(envelope: &serde_json::Value, styles: &Styles) -> O } fn is_metadata_snapshot_compat_notice(envelope: &serde_json::Value) -> bool { - prop_str_field(envelope, "code").is_some_and(is_metadata_snapshot_compat_notice_code) + prop_str_field(envelope, "code") + .and_then(|code| code.parse::().ok()) + .is_some_and(RunNoticeCode::is_metadata_snapshot_compat) } fn str_field<'a>(value: &'a serde_json::Value, key: &str) -> Option<&'a str> { @@ -1150,14 +1152,27 @@ mod tests { #[test] fn pretty_run_notice_warn() { let styles = no_color_styles(); - let line = r#"{"ts":"2026-01-01T14:25:00Z","event":"run.notice","properties":{"level":"warn","code":"sandbox_cleanup_failed","message":"sandbox cleanup failed: boom"}}"#; - let result = format_event_pretty(line, &styles).unwrap(); + let code = RunNoticeCode::SandboxCleanupFailed.to_string(); + let line = serde_json::json!({ + "ts": "2026-01-01T14:25:00Z", + "event": "run.notice", + "properties": { + "level": "warn", + "code": code, + "message": "sandbox cleanup failed: boom", + }, + }) + .to_string(); + let result = format_event_pretty(&line, &styles).unwrap(); assert!(result.contains("Warning:"), "got: {result}"); assert!( result.contains("sandbox cleanup failed: boom"), "got: {result}" ); - assert!(result.contains("[sandbox_cleanup_failed]"), "got: {result}"); + assert!( + result.contains(&format!("[{}]", RunNoticeCode::SandboxCleanupFailed)), + "got: {result}" + ); } #[test] @@ -1235,13 +1250,31 @@ mod tests { fn pretty_stream_suppresses_metadata_compat_notice_only() { let styles = no_color_styles(); let failed = r#"{"ts":"2026-01-01T14:25:00Z","event":"metadata.snapshot.failed","properties":{"phase":"checkpoint","branch":"fabro/meta","duration_ms":900,"failure_kind":"write","error":"write failed"}}"#; - let compat_notice = r#"{"ts":"2026-01-01T14:25:01Z","event":"run.notice","properties":{"level":"warn","code":"checkpoint_metadata_write_failed","message":"legacy metadata warning"}}"#; - let degraded_notice = r#"{"ts":"2026-01-01T14:25:02Z","event":"run.notice","properties":{"level":"warn","code":"checkpoint_metadata_degraded","message":"metadata snapshots disabled"}}"#; + let compat_notice = serde_json::json!({ + "ts": "2026-01-01T14:25:01Z", + "event": "run.notice", + "properties": { + "level": "warn", + "code": RunNoticeCode::CheckpointMetadataWriteFailed, + "message": "legacy metadata warning", + }, + }) + .to_string(); + let degraded_notice = serde_json::json!({ + "ts": "2026-01-01T14:25:02Z", + "event": "run.notice", + "properties": { + "level": "warn", + "code": RunNoticeCode::CheckpointMetadataDegraded, + "message": "metadata snapshots disabled", + }, + }) + .to_string(); let mut state = PrettyEventState::default(); assert!(format_event_pretty_streamed(failed, &styles, &mut state).is_some()); - assert!(format_event_pretty_streamed(compat_notice, &styles, &mut state).is_none()); - let degraded = format_event_pretty_streamed(degraded_notice, &styles, &mut state).unwrap(); + assert!(format_event_pretty_streamed(&compat_notice, &styles, &mut state).is_none()); + let degraded = format_event_pretty_streamed(°raded_notice, &styles, &mut state).unwrap(); assert!( degraded.contains("metadata snapshots disabled"), "got: {degraded}" diff --git a/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs b/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs index 9e40f0d8b..ec56dce46 100644 --- a/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs +++ b/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs @@ -527,7 +527,7 @@ fn display_value(value: &Value) -> Option { mod tests { use fabro_agent::AgentEvent; use fabro_types::{MetadataSnapshotFailureKind, MetadataSnapshotPhase, fixtures}; - use fabro_workflow::event::{Event, to_run_event}; + use fabro_workflow::event::{Event, RunNoticeCode, to_run_event}; use super::*; @@ -804,10 +804,11 @@ mod tests { fn round_trip_run_notice() { let event = Event::RunNotice { level: RunNoticeLevel::Warn, - code: "sandbox_cleanup_failed".into(), + code: RunNoticeCode::SandboxCleanupFailed.to_string(), message: "sandbox cleanup failed".into(), exec_output_tail: None, }; + let expected_code = RunNoticeCode::SandboxCleanupFailed.to_string(); let stored = to_run_event(&fixtures::RUN_1, &event); let parsed = from_run_event(&stored).unwrap(); @@ -817,7 +818,7 @@ mod tests { level: RunNoticeLevel::Warn, code, message, - } if code == "sandbox_cleanup_failed" && message == "sandbox cleanup failed" + } if code == expected_code && message == "sandbox cleanup failed" )); } diff --git a/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs b/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs index 2004ac0ba..7e7df7f2d 100644 --- a/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs @@ -3,8 +3,7 @@ reason = "sync CLI run-progress renderer: writes to std::io::stderr directly" )] -use fabro_types::RunEvent; -use fabro_types::run_event::is_metadata_snapshot_compat_notice_code; +use fabro_types::{RunEvent, RunNoticeCode}; mod event; mod info_display; @@ -444,7 +443,10 @@ impl ProgressUI { message, } => { if self.saw_metadata_snapshot_failure - && is_metadata_snapshot_compat_notice_code(&code) + && code + .parse::() + .ok() + .is_some_and(RunNoticeCode::is_metadata_snapshot_compat) { return; } @@ -1208,7 +1210,7 @@ mod tests { emit(&mut ui, Event::RunNotice { level: RunNoticeLevel::Warn, - code: "sandbox_cleanup_failed".into(), + code: RunNoticeCode::SandboxCleanupFailed.to_string(), message: "sandbox cleanup failed".into(), exec_output_tail: None, }); @@ -1279,13 +1281,13 @@ mod tests { }); emit(&mut ui, Event::RunNotice { level: RunNoticeLevel::Warn, - code: "checkpoint_metadata_write_failed".into(), + code: RunNoticeCode::CheckpointMetadataWriteFailed.to_string(), message: "legacy metadata warning".into(), exec_output_tail: None, }); emit(&mut ui, Event::RunNotice { level: RunNoticeLevel::Warn, - code: "checkpoint_metadata_degraded".into(), + code: RunNoticeCode::CheckpointMetadataDegraded.to_string(), message: "metadata snapshots are disabled for this run".into(), exec_output_tail: None, }); diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index fae612257..7c6728f39 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -160,7 +160,7 @@ fn attach_replays_completed_detached_run() { ----- stdout ----- ----- stderr ----- Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ Start [TIME] ✓ Run Tests [TIME] @@ -268,7 +268,7 @@ fn attach_before_completion_streams_to_finished_state() { ----- stdout ----- ----- stderr ----- Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ start [DURATION] ✓ wait [DURATION] @@ -711,7 +711,7 @@ fn attach_json_errors_without_prompting_for_human_input() { "properties": { "code": "worktree_skipped_no_git", "level": "warn", - "message": "Worktree mode requested but no Git repository was found; running without a worktree." + "message": "Worktree mode `always` requested but no Git repository was found; running without a worktree." }, "run_id": "[ULID]", "ts": "[TIMESTAMP]" diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index 2fb38ec7f..2aa614979 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -684,7 +684,7 @@ fn dry_run_simple() { Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ Start [TIME] ✓ Run Tests [TIME] diff --git a/lib/crates/fabro-cli/tests/it/workflow/dry_run_examples.rs b/lib/crates/fabro-cli/tests/it/workflow/dry_run_examples.rs index 06b799a41..c3f8d836c 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/dry_run_examples.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/dry_run_examples.rs @@ -21,7 +21,7 @@ fn dry_run_branching() { warning [node: implement]: Node 'implement' has goal_gate=true but no retry_target or fallback_retry_target (goal_gate_has_retry) Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ Start [TIME] ✓ Plan [TIME] @@ -58,7 +58,7 @@ fn dry_run_conditions() { Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ start [TIME] ✓ Decide [TIME] @@ -93,7 +93,7 @@ fn dry_run_parallel() { Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ start [TIME] ✓ Fork Work [TIME] @@ -129,7 +129,7 @@ fn dry_run_styled() { Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ start [TIME] ✓ Plan [TIME] @@ -165,7 +165,7 @@ fn dry_run_legacy_tool() { Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] - Warning: Worktree mode requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] + Warning: Worktree mode `always` requested but no Git repository was found; running without a worktree. [worktree_skipped_no_git] Sandbox: local (ready in [TIME]) ✓ Start [TIME] ✓ Echo [TIME] diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index dd78cc806..66d084b5f 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -126,24 +126,14 @@ fn process_env_vars() -> Vec<(String, String)> { std::env::vars().collect() } -async fn drain_pipe(mut pipe: Option, stream: &'static str) -> String +async fn drain_pipe(mut pipe: Option, stream: CommandOutputStream) -> String where R: AsyncRead + Unpin, { let mut buf = String::new(); if let Some(ref mut reader) = pipe { if let Err(err) = reader.read_to_string(&mut buf).await { - match stream { - "stdout" => { - tracing::warn!(error = %err, stream, "Failed to drain child stdout"); - } - "stderr" => { - tracing::warn!(error = %err, stream, "Failed to drain child stderr"); - } - _ => { - tracing::warn!(error = %err, stream, "Failed to drain child output"); - } - } + tracing::warn!(error = %err, ?stream, "Failed to drain child output"); } } buf @@ -302,8 +292,10 @@ impl Sandbox for LocalSandbox { // on child.wait(). let stdout_pipe = child.stdout.take(); let stderr_pipe = child.stderr.take(); - let stdout_task = tokio::spawn(async move { drain_pipe(stdout_pipe, "stdout").await }); - let stderr_task = tokio::spawn(async move { drain_pipe(stderr_pipe, "stderr").await }); + let stdout_task = + tokio::spawn(async move { drain_pipe(stdout_pipe, CommandOutputStream::Stdout).await }); + let stderr_task = + tokio::spawn(async move { drain_pipe(stderr_pipe, CommandOutputStream::Stderr).await }); let (termination, exit_code) = tokio::select! { status_result = child.wait() => { @@ -752,7 +744,7 @@ mod tests { } } - let output = drain_pipe(Some(FailingReader), "stdout").await; + let output = drain_pipe(Some(FailingReader), CommandOutputStream::Stdout).await; assert!(output.is_empty()); } diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index c04702c1e..97d3846d1 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -72,7 +72,7 @@ pub use run::{ pub use run_blob_id::RunBlobId; pub use run_event::{ EventBody, ExecOutputTail, InterviewOption, MetadataSnapshotFailureKind, MetadataSnapshotPhase, - RunEvent, RunNoticeLevel, + RunEvent, RunNoticeCode, RunNoticeLevel, }; pub use run_id::{RunId, fixtures}; pub use run_projection::{PendingInterviewRecord, RunProjection, StageProjection, first_event_seq}; diff --git a/lib/crates/fabro-types/src/run_event/infra.rs b/lib/crates/fabro-types/src/run_event/infra.rs index 8ee1bc32c..77689087b 100644 --- a/lib/crates/fabro-types/src/run_event/infra.rs +++ b/lib/crates/fabro-types/src/run_event/infra.rs @@ -1,17 +1,48 @@ use serde::{Deserialize, Serialize}; -/// Legacy `run.notice` codes paired with the new `metadata.snapshot.failed` -/// event for backward compatibility. Display layers suppress these so the -/// typed event renders without a duplicate raw warning. -pub const NOTICE_CODE_CHECKPOINT_METADATA_WRITE_FAILED: &str = "checkpoint_metadata_write_failed"; -pub const NOTICE_CODE_CHECKPOINT_METADATA_PUSH_FAILED: &str = "checkpoint_metadata_push_failed"; +#[derive( + Debug, + Clone, + Copy, + PartialEq, + Eq, + Hash, + Serialize, + Deserialize, + strum::Display, + strum::EnumString, + strum::IntoStaticStr, +)] +#[serde(rename_all = "snake_case")] +#[strum(serialize_all = "snake_case")] +pub enum RunNoticeCode { + ArtifactCollectionFailed, + ArtifactOffloadFailed, + ArtifactSyncFailed, + ArtifactUploadFailed, + CheckpointMetadataDegraded, + CheckpointMetadataPushFailed, + CheckpointMetadataWriteFailed, + DirtyWorktree, + GitDiffFailed, + GitPushFailed, + GithubTokenFailed, + ParallelBaseCheckpointFailed, + PullRequestFailed, + SandboxCleanupFailed, + SandboxGitUnavailable, + SandboxPreserved, + WorktreeSkippedNoGit, +} -#[must_use] -pub fn is_metadata_snapshot_compat_notice_code(code: &str) -> bool { - matches!( - code, - NOTICE_CODE_CHECKPOINT_METADATA_WRITE_FAILED | NOTICE_CODE_CHECKPOINT_METADATA_PUSH_FAILED - ) +impl RunNoticeCode { + #[must_use] + pub fn is_metadata_snapshot_compat(self) -> bool { + matches!( + self, + Self::CheckpointMetadataWriteFailed | Self::CheckpointMetadataPushFailed + ) + } } #[derive( diff --git a/lib/crates/fabro-types/src/run_event/mod.rs b/lib/crates/fabro-types/src/run_event/mod.rs index 9d5fbf11d..647efc381 100644 --- a/lib/crates/fabro-types/src/run_event/mod.rs +++ b/lib/crates/fabro-types/src/run_event/mod.rs @@ -1366,7 +1366,7 @@ mod tests { for body in [ EventBody::RunNotice(RunNoticeProps { level: RunNoticeLevel::Warn, - code: "git_diff_failed".to_string(), + code: RunNoticeCode::GitDiffFailed.to_string(), message: "git diff failed".to_string(), exec_output_tail: Some(tail.clone()), }), @@ -1402,7 +1402,7 @@ mod tests { for body in [ EventBody::RunNotice(RunNoticeProps { level: RunNoticeLevel::Warn, - code: "git_diff_failed".to_string(), + code: RunNoticeCode::GitDiffFailed.to_string(), message: "git diff failed".to_string(), exec_output_tail: None, }), diff --git a/lib/crates/fabro-workflow/src/event.rs b/lib/crates/fabro-workflow/src/event.rs index f5120e62e..e64b2ae30 100644 --- a/lib/crates/fabro-workflow/src/event.rs +++ b/lib/crates/fabro-workflow/src/event.rs @@ -8,7 +8,7 @@ mod stored_fields; #[cfg(test)] mod test_support; -pub use fabro_types::{EventBody, RunNoticeLevel}; +pub use fabro_types::{EventBody, RunNoticeCode, RunNoticeLevel}; pub use self::convert::{to_run_event, to_run_event_at}; pub use self::emitter::Emitter; diff --git a/lib/crates/fabro-workflow/src/event/convert.rs b/lib/crates/fabro-workflow/src/event/convert.rs index 815aaec11..b3a50e855 100644 --- a/lib/crates/fabro-workflow/src/event/convert.rs +++ b/lib/crates/fabro-workflow/src/event/convert.rs @@ -1167,8 +1167,8 @@ mod tests { use std::collections::BTreeMap; use ::fabro_types::{ - EventBody, FailureReason, ParallelBranchId, Principal, RunNoticeLevel, RunProvenance, - StageId, SystemActorKind, fixtures, run_event as fabro_types, + EventBody, FailureReason, ParallelBranchId, Principal, RunNoticeCode, RunNoticeLevel, + RunProvenance, StageId, SystemActorKind, fixtures, run_event as fabro_types, }; use chrono::Utc; use fabro_agent::{AgentEvent, SandboxEvent}; @@ -1616,7 +1616,7 @@ mod tests { fn run_notice_maps_exec_output_tail_to_props() { let stored = to_run_event(&fixtures::RUN_1, &Event::RunNotice { level: RunNoticeLevel::Warn, - code: "git_diff_failed".to_string(), + code: RunNoticeCode::GitDiffFailed.to_string(), message: "git diff failed".to_string(), exec_output_tail: Some(exec_tail()), }); diff --git a/lib/crates/fabro-workflow/src/event/emitter.rs b/lib/crates/fabro-workflow/src/event/emitter.rs index a1bba727a..dc50a56dd 100644 --- a/lib/crates/fabro-workflow/src/event/emitter.rs +++ b/lib/crates/fabro-workflow/src/event/emitter.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicI64, Ordering}; -use ::fabro_types::{ExecOutputTail, RunEvent, RunId, RunNoticeLevel}; +use ::fabro_types::{ExecOutputTail, RunEvent, RunId, RunNoticeCode, RunNoticeLevel}; use chrono::Utc; use fabro_agent::{WorktreeEvent, WorktreeEventCallback}; @@ -76,15 +76,10 @@ impl Emitter { self.emit_with_scope(event, Some(scope)); } - pub fn notice( - &self, - level: RunNoticeLevel, - code: impl Into, - message: impl Into, - ) { + pub fn notice(&self, level: RunNoticeLevel, code: RunNoticeCode, message: impl Into) { self.emit(&Event::RunNotice { level, - code: code.into(), + code: code.to_string(), message: message.into(), exec_output_tail: None, }); @@ -93,13 +88,13 @@ impl Emitter { pub fn notice_with_tail( &self, level: RunNoticeLevel, - code: impl Into, + code: RunNoticeCode, message: impl Into, exec_output_tail: Option, ) { self.emit(&Event::RunNotice { level, - code: code.into(), + code: code.to_string(), message: message.into(), exec_output_tail, }); diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index cb8d3ea04..c58104f83 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -12,7 +12,7 @@ use tokio::sync::Semaphore; use super::{EngineServices, Handler}; use crate::context::{Context, WorkflowContext, keys}; use crate::error::Error; -use crate::event::{Event, RunNoticeLevel, StageScope}; +use crate::event::{Event, RunNoticeCode, RunNoticeLevel, StageScope}; use crate::git::sanitize_ref_component; use crate::hook_context::set_hook_node; use crate::millis_u64; @@ -209,7 +209,7 @@ impl Handler for ParallelHandler { ); services.run.emitter.notice_with_tail( RunNoticeLevel::Warn, - "parallel_base_checkpoint_failed", + RunNoticeCode::ParallelBaseCheckpointFailed, format!("Could not checkpoint base state before parallel branches: {e}"), fabro_sandbox::default_redacted_output_tail(&e), ); diff --git a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs index f65fcee86..50d6ccded 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/artifact.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/artifact.rs @@ -16,7 +16,7 @@ use tokio::time::sleep; use crate::artifact::{normalize_durable_updates, offload_large_values, sync_artifacts_to_env}; use crate::artifact_snapshot::collect_artifacts; use crate::artifact_upload::ArtifactSink; -use crate::event::{Emitter, Event, RunNoticeLevel}; +use crate::event::{Emitter, Event, RunNoticeCode, RunNoticeLevel}; use crate::graph::{WorkflowGraph, WorkflowNode}; use crate::lifecycle::event::{stage_scope_for, stage_visit}; use crate::outcome::BilledModelUsage; @@ -125,7 +125,7 @@ impl RunLifecycle for ArtifactLifecycle { { self.emitter.notice( RunNoticeLevel::Warn, - "artifact_upload_failed", + RunNoticeCode::ArtifactUploadFailed, format!("[node: {node_id}] artifact upload failed: {err}"), ); return Ok(()); @@ -151,7 +151,7 @@ impl RunLifecycle for ArtifactLifecycle { Err(e) => { self.emitter.notice( RunNoticeLevel::Warn, - "artifact_collection_failed", + RunNoticeCode::ArtifactCollectionFailed, format!("[node: {node_id}] artifact collection failed: {e}"), ); } @@ -174,7 +174,7 @@ impl RunLifecycle for ArtifactLifecycle { { self.emitter.notice( RunNoticeLevel::Warn, - "artifact_offload_failed", + RunNoticeCode::ArtifactOffloadFailed, format!("[node: {node_id}] artifact offload failed: {e}"), ); } @@ -187,7 +187,7 @@ impl RunLifecycle for ArtifactLifecycle { { self.emitter.notice( RunNoticeLevel::Warn, - "artifact_sync_failed", + RunNoticeCode::ArtifactSyncFailed, format!("[node: {node_id}] artifact sync failed: {e}"), ); } diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index 99454cf69..2f9d5bfdb 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -14,7 +14,7 @@ use fabro_util::error::collect_causes; use fabro_util::time::elapsed_ms; use crate::artifact; -use crate::event::{Emitter, Event, RunNoticeLevel, StageScope}; +use crate::event::{Emitter, Event, RunNoticeCode, RunNoticeLevel, StageScope}; use crate::graph::{WorkflowGraph, WorkflowNode}; use crate::lifecycle::event::stage_scope_for; use crate::outcome::BilledModelUsage; @@ -128,7 +128,10 @@ impl RunLifecycle for GitLifecycle { None, None, ); - self.emit_metadata_warning("checkpoint_metadata_write_failed", message); + self.emit_metadata_warning( + RunNoticeCode::CheckpointMetadataWriteFailed, + message, + ); } }, Err(err) => { @@ -145,7 +148,10 @@ impl RunLifecycle for GitLifecycle { None, None, ); - self.emit_metadata_warning("checkpoint_metadata_write_failed", message); + self.emit_metadata_warning( + RunNoticeCode::CheckpointMetadataWriteFailed, + message, + ); } } } @@ -217,7 +223,7 @@ impl RunLifecycle for GitLifecycle { Some(&scope), ); self.emit_metadata_warning( - "checkpoint_metadata_write_failed", + RunNoticeCode::CheckpointMetadataWriteFailed, message, ); None @@ -239,7 +245,10 @@ impl RunLifecycle for GitLifecycle { None, Some(&scope), ); - self.emit_metadata_warning("checkpoint_metadata_write_failed", message); + self.emit_metadata_warning( + RunNoticeCode::CheckpointMetadataWriteFailed, + message, + ); None } } @@ -294,7 +303,7 @@ impl RunLifecycle for GitLifecycle { ); self.emitter.notice_with_tail( RunNoticeLevel::Warn, - "git_push_failed", + RunNoticeCode::GitPushFailed, format!("Failed to push run branch {branch}: {err}"), exec_output_tail.clone(), ); @@ -327,7 +336,7 @@ impl RunLifecycle for GitLifecycle { fabro_sandbox::default_redacted_output_tail(&err); self.emitter.notice_with_tail( RunNoticeLevel::Warn, - "git_diff_failed", + RunNoticeCode::GitDiffFailed, format!("[node: {node_id}] git diff failed: {err}"), exec_output_tail, ); @@ -401,7 +410,10 @@ impl GitLifecycle { Some(snapshot.bytes), scope, ); - self.emit_metadata_warning("checkpoint_metadata_push_failed", message); + self.emit_metadata_warning( + RunNoticeCode::CheckpointMetadataPushFailed, + message, + ); } else { self.emit_metadata_snapshot_completed( phase, @@ -427,7 +439,7 @@ impl GitLifecycle { None, scope, ); - self.emit_metadata_warning("checkpoint_metadata_write_failed", message); + self.emit_metadata_warning(RunNoticeCode::CheckpointMetadataWriteFailed, message); None } } @@ -512,14 +524,9 @@ impl GitLifecycle { } } - fn emit_metadata_warning(&self, code: &str, message: String) { + fn emit_metadata_warning(&self, code: RunNoticeCode, message: String) { if self.metadata_runtime.mark_metadata_degraded() { - self.emitter.emit(&Event::RunNotice { - level: RunNoticeLevel::Warn, - code: code.to_string(), - message, - exec_output_tail: None, - }); + self.emitter.notice(RunNoticeLevel::Warn, code, message); } } } diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 921697369..63870bc89 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -10,7 +10,7 @@ use fabro_util::time::elapsed_ms; use super::types::{Concluded, FinalizeOptions, Retroed}; use crate::error::Error; -use crate::event::{Event, RunNoticeLevel}; +use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; use crate::outcome::{Outcome, OutcomeExt, StageOutcome}; use crate::records::{Checkpoint, Conclusion, StageSummary}; use crate::run_metadata::MetadataSnapshot; @@ -235,7 +235,11 @@ pub async fn write_finalize_commit( None, None, ); - emit_metadata_warning(services, "checkpoint_metadata_write_failed", message); + emit_metadata_warning( + services, + RunNoticeCode::CheckpointMetadataWriteFailed, + message, + ); return; } }; @@ -256,7 +260,11 @@ pub async fn write_finalize_commit( None, None, ); - emit_metadata_warning(services, "checkpoint_metadata_write_failed", message); + emit_metadata_warning( + services, + RunNoticeCode::CheckpointMetadataWriteFailed, + message, + ); return; } }; @@ -277,7 +285,11 @@ pub async fn write_finalize_commit( Some(snapshot.entry_count), Some(snapshot.bytes), ); - emit_metadata_warning(services, "checkpoint_metadata_push_failed", message); + emit_metadata_warning( + services, + RunNoticeCode::CheckpointMetadataPushFailed, + message, + ); } else { emit_metadata_snapshot_completed(services, phase, meta_branch, started, &snapshot); } @@ -296,7 +308,11 @@ pub async fn write_finalize_commit( None, None, ); - emit_metadata_warning(services, "checkpoint_metadata_write_failed", message); + emit_metadata_warning( + services, + RunNoticeCode::CheckpointMetadataWriteFailed, + message, + ); } } } @@ -359,7 +375,7 @@ fn emit_metadata_snapshot_failed( }); } -fn emit_metadata_warning(services: &RunServices, code: &str, message: String) { +fn emit_metadata_warning(services: &RunServices, code: RunNoticeCode, message: String) { if services.metadata_runtime.mark_metadata_degraded() { services.emitter.notice(RunNoticeLevel::Warn, code, message); } @@ -383,7 +399,7 @@ async fn compute_final_patch( Err(err) => { services.emitter.notice( RunNoticeLevel::Warn, - "git_diff_failed", + RunNoticeCode::GitDiffFailed, format!("final diff failed: {err}"), ); None @@ -534,7 +550,7 @@ pub async fn finalize(retroed: Retroed, options: &FinalizeOptions) -> Result Result Result Option { if let Some(env_name) = env_name { options.emitter.notice( RunNoticeLevel::Warn, - "dirty_worktree", + RunNoticeCode::DirtyWorktree, format!("Uncommitted changes will not be included in the {env_name}."), ); } @@ -200,6 +200,14 @@ fn resolve_worktree_plan(options: &mut InitOptions) -> Option { }) } +fn worktree_skipped_notice(mode: Option) -> Option<(RunNoticeCode, &'static str)> { + matches!(mode, Some(WorktreeMode::Always)).then_some(( + RunNoticeCode::WorktreeSkippedNoGit, + "Worktree mode `always` requested but no Git repository was found; running without a \ + worktree.", + )) +} + fn git_setup_intent(run_options: &RunOptions) -> GitSetupIntent { if let Some(source) = run_options.fork_source_ref.as_ref() { GitSetupIntent::ForkFromCheckpoint { @@ -243,7 +251,7 @@ async fn build_sandbox_env( tracing::warn!(error = %e, "Failed to mint GitHub token"); emitter.notice( RunNoticeLevel::Warn, - "github_token_failed", + RunNoticeCode::GithubTokenFailed, format!("Failed to mint GitHub token: {e}"), ); } @@ -519,16 +527,13 @@ pub async fn initialize( )) }; if worktree_plan.is_some() && !worktree_created { - tracing::warn!( - worktree_mode = ?options.worktree_mode, - "worktree requested but cwd is not a git repository; running without a worktree" - ); - options.emitter.notice( - RunNoticeLevel::Warn, - "worktree_skipped_no_git", - "Worktree mode requested but no Git repository was found; running without a \ - worktree.", - ); + if let Some((code, message)) = worktree_skipped_notice(options.worktree_mode) { + tracing::warn!( + worktree_mode = ?options.worktree_mode, + "worktree skipped: cwd is not a git repository" + ); + options.emitter.notice(RunNoticeLevel::Warn, code, message); + } options.run_options.git = None; } let cleanup_guard = scopeguard::guard(Arc::clone(&sandbox), |sandbox| { @@ -637,7 +642,7 @@ pub async fn initialize( if sandbox_has_origin { options.emitter.notice( RunNoticeLevel::Warn, - "sandbox_git_unavailable", + RunNoticeCode::SandboxGitUnavailable, "Sandbox could not set up Git despite a configured origin; running \ without checkpointing or PR support.", ); @@ -725,7 +730,7 @@ pub async fn initialize( if metadata_runtime.mark_metadata_degraded() { options.emitter.notice( RunNoticeLevel::Warn, - "checkpoint_metadata_write_failed", + RunNoticeCode::CheckpointMetadataWriteFailed, message, ); } @@ -1034,87 +1039,15 @@ mod tests { assert!(options.run_options.git.is_none()); } - #[tokio::test] - async fn initialize_emits_worktree_skipped_no_git_in_non_git_cwd() { - let temp = tempfile::tempdir().unwrap(); - let run_dir = temp.path().join("run"); - std::fs::create_dir_all(&run_dir).unwrap(); - // Non-git working directory: a tmpdir without a `.git` parent. - let cwd = temp.path().join("cwd"); - std::fs::create_dir_all(&cwd).unwrap(); + #[test] + fn worktree_skipped_notice_only_warns_for_always() { + assert!(worktree_skipped_notice(None).is_none()); + assert!(worktree_skipped_notice(Some(WorktreeMode::Clean)).is_none()); + assert!(worktree_skipped_notice(Some(WorktreeMode::Dirty)).is_none()); + assert!(worktree_skipped_notice(Some(WorktreeMode::Never)).is_none()); - let (graph, source) = simple_graph(); - let persisted = test_persisted(graph, source, &run_dir); - let emitter = Arc::new(crate::event::Emitter::new(test_run_id())); - let seen = Arc::new(std::sync::Mutex::new(Vec::new())); - emitter.on_event({ - let seen = Arc::clone(&seen); - move |event| seen.lock().unwrap().push(event.clone()) - }); - - // The notice we assert below is emitted before any later sandbox/setup work - // that could fail in this minimal fixture (no real - // git/devcontainer/lifecycle), so the overall result is intentionally - // ignored — the assertion runs against the captured event stream - // regardless of how `initialize` ultimately resolves. - let _ = initialize(persisted, InitOptions { - run_id: test_run_id(), - run_store: { - let store = memory_store(); - let inner = store.create_run(&test_run_id()).await.unwrap(); - inner.into() - }, - dry_run: false, - emitter, - sandbox: SandboxSpec::Local { - working_directory: cwd, - }, - llm: LlmSpec { - model: "test-model".to_string(), - provider: fabro_llm::Provider::Anthropic, - fallback_chain: Vec::new(), - mcp_servers: Vec::new(), - dry_run: true, - }, - interviewer: Arc::new(AutoApproveInterviewer::engine()), - lifecycle: crate::run_options::LifecycleOptions { - setup_commands: vec![], - setup_command_timeout_ms: 1_000, - devcontainer_phases: vec![], - }, - run_options: test_settings(&run_dir), - workflow_path: None, - workflow_bundle: None, - hooks: fabro_hooks::HookSettings { hooks: vec![] }, - sandbox_env: SandboxEnvSpec { - devcontainer_env: HashMap::new(), - toml_env: HashMap::new(), - github_permissions: None, - origin_url: None, - }, - vault: None, - devcontainer: None, - git: None, - worktree_mode: Some(WorktreeMode::Always), - run_control: None, - registry_override: None, - artifact_sink: None, - checkpoint: None, - seed_context: None, - }) - .await; - - let events = seen.lock().unwrap().clone(); - let notice = events - .iter() - .find_map(|event| match &event.body { - EventBody::RunNotice(props) if props.code == "worktree_skipped_no_git" => { - Some(props.clone()) - } - _ => None, - }) - .expect("worktree_skipped_no_git notice"); - assert!(matches!(notice.level, fabro_types::RunNoticeLevel::Warn)); + let (code, _) = worktree_skipped_notice(Some(WorktreeMode::Always)).unwrap(); + assert_eq!(code, RunNoticeCode::WorktreeSkippedNoGit); } #[tokio::test] diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 5b9355ccf..71a132fad 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -13,7 +13,7 @@ use fabro_util::text::strip_goal_decoration; use tracing::{debug, info}; use super::types::{Concluded, Finalized, PullRequestOptions}; -use crate::event::{Event, RunNoticeLevel}; +use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; use crate::outcome::{StageOutcome, format_cost as outcome_format_cost}; use crate::records::{Conclusion, RunSpec}; use crate::runtime_store::RunStoreHandle; @@ -603,7 +603,7 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> .emit(&Event::PullRequestFailed { error: e.clone() }); services.emitter.notice( RunNoticeLevel::Warn, - "pull_request_failed", + RunNoticeCode::PullRequestFailed, format!("PR creation failed: {e}"), ); }