diff --git a/lib/crates/fabro-cli/src/commands/pr/create.rs b/lib/crates/fabro-cli/src/commands/pr/create.rs index 873630cba..84228097e 100644 --- a/lib/crates/fabro-cli/src/commands/pr/create.rs +++ b/lib/crates/fabro-cli/src/commands/pr/create.rs @@ -83,8 +83,17 @@ async fn create_from( .as_deref() .context("Run has no run_branch — was it run with git push enabled?")?; - let diff = std::fs::read_to_string(run_dir.join("final.patch")) - .context("Failed to read final.patch — no diff available")?; + let diff = match run_store.as_ref() { + Some(run_store) => run_store + .get_final_patch() + .await + .ok() + .flatten() + .or_else(|| std::fs::read_to_string(run_dir.join("final.patch")).ok()) + .context("Failed to read final.patch — no diff available")?, + None => std::fs::read_to_string(run_dir.join("final.patch")) + .context("Failed to read final.patch — no diff available")?, + }; if diff.trim().is_empty() { bail!("final.patch is empty — nothing to create a PR for"); } @@ -148,6 +157,11 @@ async fn create_from( match record { Some(record) => { info!(pr_url = %record.html_url, "Pull request created"); + if let Some(run_store) = run_store.as_ref() { + if let Err(err) = run_store.put_pull_request(&record).await { + tracing::warn!(error = %err, "Failed to persist pull request in run store"); + } + } if let Err(err) = record.save(&run_dir.join("pull_request.json")) { tracing::warn!(error = %err, "Failed to save pull_request.json"); } diff --git a/lib/crates/fabro-cli/src/commands/pr/list.rs b/lib/crates/fabro-cli/src/commands/pr/list.rs index 63a62020d..bbcbd4380 100644 --- a/lib/crates/fabro-cli/src/commands/pr/list.rs +++ b/lib/crates/fabro-cli/src/commands/pr/list.rs @@ -2,7 +2,7 @@ use std::path::Path; use anyhow::{Context, Result}; use fabro_config::FabroSettingsExt; -use fabro_workflow::pull_request::PullRequestRecord; +use fabro_types::PullRequestRecord; use fabro_workflow::run_lookup::{runs_base, scan_runs_combined}; use futures::future::join_all; use serde::Serialize; @@ -50,6 +50,12 @@ async fn list_from( let mut entries: Vec<(String, PullRequestRecord)> = Vec::new(); for run in &runs { + if let Ok(Some(run_store)) = store.open_run_reader(&run.run_id).await { + if let Ok(Some(record)) = run_store.get_pull_request().await { + entries.push((run.run_id.to_string(), record)); + continue; + } + } let pr_path = run.path.join("pull_request.json"); if let Ok(content) = std::fs::read_to_string(&pr_path) { if let Ok(record) = serde_json::from_str::(&content) { diff --git a/lib/crates/fabro-cli/src/commands/pr/mod.rs b/lib/crates/fabro-cli/src/commands/pr/mod.rs index 67a36628b..40b94b993 100644 --- a/lib/crates/fabro-cli/src/commands/pr/mod.rs +++ b/lib/crates/fabro-cli/src/commands/pr/mod.rs @@ -8,7 +8,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; -use fabro_workflow::pull_request::PullRequestRecord; +use fabro_types::PullRequestRecord; use fabro_workflow::run_lookup::resolve_run_combined; use crate::args::{GlobalArgs, PrCommand, PrNamespace}; @@ -35,9 +35,14 @@ pub(crate) async fn load_pr_record( ) -> Result<(PullRequestRecord, PathBuf)> { let storage_dir = base.parent().unwrap_or(base); let store = store::build_store(storage_dir)?; - let run_dir = resolve_run_combined(store.as_ref(), base, run_id) - .await? - .path; + let run = resolve_run_combined(store.as_ref(), base, run_id).await?; + let run_dir = run.path; + let run_store = store::open_run_reader(storage_dir, &run.run_id).await?; + if let Some(run_store) = run_store { + if let Some(record) = run_store.get_pull_request().await.ok().flatten() { + return Ok((record, run_dir)); + } + } let pr_path = run_dir.join("pull_request.json"); let content = std::fs::read_to_string(&pr_path).with_context(|| { format!( diff --git a/lib/crates/fabro-cli/src/commands/run/diff.rs b/lib/crates/fabro-cli/src/commands/run/diff.rs index 32e6ca7f0..8ce3e98ab 100644 --- a/lib/crates/fabro-cli/src/commands/run/diff.rs +++ b/lib/crates/fabro-cli/src/commands/run/diff.rs @@ -82,6 +82,13 @@ async fn resolve_diff( .as_deref() .ok_or_else(|| anyhow::anyhow!("This run was not git-checkpointed; no diff available"))?; + if let Some(run_store) = run_store { + if let Ok(Some(patch)) = run_store.get_final_patch().await { + debug!("Reading final.patch from store"); + return Ok(patch); + } + } + let final_patch_path = run_dir.join("final.patch"); if final_patch_path.exists() { debug!("Reading final.patch"); diff --git a/lib/crates/fabro-cli/src/commands/run/output.rs b/lib/crates/fabro-cli/src/commands/run/output.rs index 847424c0a..f0a3e63ff 100644 --- a/lib/crates/fabro-cli/src/commands/run/output.rs +++ b/lib/crates/fabro-cli/src/commands/run/output.rs @@ -3,12 +3,12 @@ use std::time::Duration; use fabro_graphviz::graph::Graph; use fabro_store::RuntimeState; +use fabro_types::PullRequestRecord; use fabro_util::terminal::Styles; use fabro_util::text::strip_goal_decoration; use fabro_workflow::asset_snapshot::collect_asset_paths; use fabro_workflow::outcome::{StageStatus, format_cost}; use fabro_workflow::pipeline::{Persisted, Validated}; -use fabro_workflow::pull_request::PullRequestRecord; use fabro_workflow::records::{Checkpoint, CheckpointExt, Conclusion, ConclusionExt}; use indicatif::HumanDuration; diff --git a/lib/crates/fabro-cli/tests/it/cmd/diff.rs b/lib/crates/fabro-cli/tests/it/cmd/diff.rs index 9a59fa499..fc9840006 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/diff.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/diff.rs @@ -1,7 +1,28 @@ +use std::sync::Arc; + +use fabro_store::Store; use fabro_test::{fabro_snapshot, test_context}; +use fabro_types::RunId; +use object_store::local::LocalFileSystem; use super::support::{git_filters, setup_git_backed_changed_run, setup_git_backed_noop_run}; +fn with_runtime(f: impl FnOnce(&tokio::runtime::Runtime) -> T) -> T { + let runtime = tokio::runtime::Runtime::new().unwrap(); + f(&runtime) +} + +fn build_store(storage_dir: &std::path::Path) -> Arc { + let store_path = storage_dir.join("store"); + std::fs::create_dir_all(&store_path).unwrap(); + let object_store = Arc::new(LocalFileSystem::new_with_prefix(&store_path).unwrap()); + Arc::new(fabro_store::SlateStore::new( + object_store, + "", + std::time::Duration::from_millis(5), + )) +} + #[test] fn help() { let context = test_context!(); @@ -89,6 +110,41 @@ fn diff_completed_run_with_changes_prints_patch() { "); } +#[test] +fn diff_completed_run_reads_store_final_patch_without_disk_file() { + let context = test_context!(); + let setup = setup_git_backed_changed_run(&context); + let run_id: RunId = setup.run.run_id.parse().unwrap(); + let patch = std::fs::read_to_string(setup.run.run_dir.join("final.patch")).unwrap(); + std::fs::remove_file(setup.run.run_dir.join("final.patch")).unwrap(); + + with_runtime(|runtime| { + runtime.block_on(async { + let store = build_store(&context.storage_dir); + let run_store = store.open_run(&run_id).await.unwrap().unwrap(); + run_store.put_final_patch(&patch).await.unwrap(); + }); + }); + + let mut cmd = context.command(); + cmd.args(["diff", &setup.run.run_id]); + + fabro_snapshot!(git_filters(&context), cmd, @" + success: true + exit_code: 0 + ----- stdout ----- + diff --git a/story.txt b/story.txt + index [SHA]..[SHA] 100644 + --- a/story.txt + +++ b/story.txt + @@ -1 +1,3 @@ + line 1 + +line 2 + +line 3 + ----- stderr ----- + "); +} + #[test] fn diff_node_outputs_specific_patch() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_create.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_create.rs index 99a468c14..78091dd50 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/pr_create.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/pr_create.rs @@ -44,7 +44,6 @@ fn pr_create_unfinished_run_errors_before_network() { ----- stdout ----- ----- stderr ----- error: Failed to load start.json - > I/O error: No such file or directory (os error 2) "); } @@ -63,3 +62,21 @@ fn pr_create_completed_dry_run_without_run_branch_errors() { error: Run has no run_branch — was it run with git push enabled? "); } + +#[test] +fn pr_create_uses_store_run_record_without_run_json() { + let context = test_context!(); + let run = setup_completed_dry_run(&context); + std::fs::remove_file(run.run_dir.join("run.json")).unwrap(); + + let mut cmd = context.command(); + cmd.args(["pr", "create", &run.run_id]); + + fabro_snapshot!(context.filters(), cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + error: Run has no run_branch — was it run with git push enabled? + "); +} diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs index 8dc4011b6..b65f661b7 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs @@ -1,7 +1,28 @@ +use std::sync::Arc; + +use fabro_store::Store; use fabro_test::{fabro_snapshot, test_context}; +use fabro_types::{PullRequestRecord, RunId}; +use object_store::local::LocalFileSystem; use super::support::setup_completed_dry_run; +fn with_runtime(f: impl FnOnce(&tokio::runtime::Runtime) -> T) -> T { + let runtime = tokio::runtime::Runtime::new().unwrap(); + f(&runtime) +} + +fn build_store(storage_dir: &std::path::Path) -> Arc { + let store_path = storage_dir.join("store"); + std::fs::create_dir_all(&store_path).unwrap(); + let object_store = Arc::new(LocalFileSystem::new_with_prefix(&store_path).unwrap()); + Arc::new(fabro_store::SlateStore::new( + object_store, + "", + std::time::Duration::from_millis(5), + )) +} + #[test] fn help() { let context = test_context!(); @@ -46,3 +67,45 @@ fn pr_view_missing_pull_request_json_errors() { > No such file or directory (os error 2) "); } + +#[test] +fn pr_view_reads_pull_request_from_store_without_pull_request_json() { + let context = test_context!(); + let run = setup_completed_dry_run(&context); + let run_id: RunId = run.run_id.parse().unwrap(); + + with_runtime(|runtime| { + runtime.block_on(async { + let store = build_store(&context.storage_dir); + let run_store = store.open_run(&run_id).await.unwrap().unwrap(); + run_store + .put_pull_request(&PullRequestRecord { + html_url: "https://github.com/fabro-sh/fabro/pull/123".to_string(), + number: 123, + owner: "fabro-sh".to_string(), + repo: "fabro".to_string(), + base_branch: "main".to_string(), + head_branch: "fabro/run/demo".to_string(), + title: "Map the constellations".to_string(), + }) + .await + .unwrap(); + }); + }); + + let pr_path = run.run_dir.join("pull_request.json"); + if pr_path.exists() { + std::fs::remove_file(pr_path).unwrap(); + } + + let mut cmd = context.command(); + cmd.args(["pr", "view", &run.run_id]); + + fabro_snapshot!(context.filters(), cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + error: GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id + "); +} diff --git a/lib/crates/fabro-workflow/src/pipeline/mod.rs b/lib/crates/fabro-workflow/src/pipeline/mod.rs index 8ddbb9d85..707d32b08 100644 --- a/lib/crates/fabro-workflow/src/pipeline/mod.rs +++ b/lib/crates/fabro-workflow/src/pipeline/mod.rs @@ -10,6 +10,7 @@ pub(crate) mod types; mod validate; pub use execute::execute; +pub use fabro_types::PullRequestRecord; pub(crate) use finalize::build_conclusion_from_store; pub use finalize::{ build_conclusion, classify_engine_result, finalize, persist_terminal_outcome, @@ -18,9 +19,7 @@ pub use finalize::{ pub use initialize::initialize; pub use parse::parse; pub(crate) use persist::persist; -pub use pull_request::{ - AutoMergeOptions, PullRequestRecord, build_pr_body, maybe_open_pull_request, pull_request, -}; +pub use pull_request::{AutoMergeOptions, build_pr_body, maybe_open_pull_request, pull_request}; pub use retro::{retro, run_retro}; pub use transform::transform; pub use types::{ diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 99231787e..84b238ff7 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -2,7 +2,7 @@ use std::path::Path; use fabro_config::run::MergeStrategy; use fabro_store::RunStore; -use serde::{Deserialize, Serialize}; +use fabro_types::PullRequestRecord; use tracing::{debug, info}; use fabro_github::{self as github_app, GitHubAppCredentials, ssh_url_to_https}; @@ -19,26 +19,6 @@ use tokio::fs::read_to_string; use super::types::{Concluded, Finalized, PullRequestOptions}; -/// Record of a pull request created for a workflow run. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct PullRequestRecord { - pub html_url: String, - pub number: u64, - pub owner: String, - pub repo: String, - pub base_branch: String, - pub head_branch: String, - pub title: String, -} - -impl PullRequestRecord { - pub fn save(&self, path: &Path) -> Result<(), String> { - let json = serde_json::to_string_pretty(self) - .map_err(|e| format!("Failed to serialize pull_request.json: {e}"))?; - std::fs::write(path, json).map_err(|e| format!("Failed to write pull_request.json: {e}")) - } -} - /// Derive a PR title from the workflow goal. /// /// Uses the first line, truncated to 120 characters for readability.