Merge pull request #652 from fabro-sh/fix/publish-failures

Make publish failures terminal
This commit is contained in:
Bryan Helmkamp 2026-07-28 17:03:18 -04:00 committed by GitHub
commit 1aa7a153b0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
34 changed files with 1309 additions and 622 deletions

1
Cargo.lock generated
View file

@ -3394,6 +3394,7 @@ dependencies = [
"serde_json",
"sha2 0.10.9",
"shlex",
"strum 0.28.0",
"tempfile",
"thiserror 2.0.18",
"tokio",

View file

@ -2116,6 +2116,7 @@ These legacy events may appear in older run logs. Current CLI backend runs do no
"properties": {
"pr_url": "https://github.com/org/repo/pull/42",
"pr_number": 42,
"head_sha": "d34db33f",
"draft": true
}
}
@ -2125,6 +2126,7 @@ These legacy events may appear in older run logs. Current CLI backend runs do no
|----------|------|-------------|
| `pr_url` | string | Pull request URL |
| `pr_number` | number | Pull request number |
| `head_sha` | string (optional) | Verified commit SHA at the remote PR head; absent on older events |
| `draft` | boolean | Whether the PR is a draft |
### `pull_request.linked`

View file

@ -8894,6 +8894,7 @@ components:
type: string
enum:
- workflow_error
- publish_failed
- cancelled
- approval_denied
- terminated

View file

@ -55,6 +55,7 @@ When you choose the GitHub App strategy, the CLI opens GitHub with a pre-filled
| Permission | Level | Purpose |
|---|---|---|
| Contents | Write | Clone repos, push run branches and checkpoints |
| Workflows | Write | Push changes under `.github/workflows/` |
| Metadata | Read | Look up repository installation status |
| Pull requests | Write | Create and update PRs from workflows |
| Checks | Write | Report workflow status on commits |
@ -219,7 +220,7 @@ When a workflow runs in a remote sandbox (Daytona or Docker), Fabro clones the c
2. SSH URLs (e.g. `git@github.com:owner/repo.git`) are converted to HTTPS
3. Fabro signs a short-lived JWT using the App ID and private key (RS256, 10-minute validity)
4. Using the JWT, Fabro looks up the GitHub App installation for the repository (`GET /repos/\{owner\}/\{repo\}/installation`)
5. Fabro requests a scoped Installation Access Token with `contents: write` permission on the specific repository
5. Fabro requests a scoped Installation Access Token with `contents: write` and `workflows: write` permissions on the specific repository
6. The sandbox clones via HTTPS using `x-access-token` as the username and the token as the password
For public repositories, the clone works without credentials. The token is still generated because it's needed for pushing checkpoints.
@ -248,7 +249,9 @@ The upper bound on what Fabro will mint is whatever permissions the GitHub App i
### Checkpoint pushing
After each workflow stage, Fabro [checkpoints](/execution/checkpoints) by pushing the run branch and metadata branch to origin. Inside remote sandboxes, the git remote URL is configured with the Installation Access Token for authenticated pushing.
After each workflow stage, Fabro [checkpoints](/execution/checkpoints) by pushing the run branch and metadata branch to origin. Before a successful run becomes terminal, the publish stage pushes the final commit again and treats failure as a run failure. Inside remote sandboxes, the git remote URL is configured with the Installation Access Token for authenticated pushing.
When pull request creation is enabled, Fabro then checks that GitHub reports the run branch at the exact final commit before opening the PR. A failed final push, branch check, or PR creation marks the run as failed with `publish_failed`; the terminal run event is emitted only after this step finishes.
For long-running workflows, Fabro refreshes the token before each push since Installation Access Tokens are short-lived (typically 1 hour).

View file

@ -614,6 +614,7 @@ mod tests {
repo: "widgets".into(),
base_branch: "main".into(),
head_branch: "fabro/run/42".into(),
head_sha: Some("final-sha".to_string()),
title: "Ship the server-side PR".into(),
draft: true,
};

View file

@ -1383,6 +1383,7 @@ mod tests {
repo: "fabro".into(),
base_branch: "main".into(),
head_branch: "fabro/run/42".into(),
head_sha: Some("final-sha".to_string()),
title: "Ship the change".into(),
draft: true,
});

View file

@ -85,6 +85,7 @@ fn pr_view_reads_pull_request_from_store_without_pull_request_json() {
repo: "fabro".to_string(),
base_branch: "main".to_string(),
head_branch: "fabro/run/demo".to_string(),
head_sha: Some("final-sha".to_string()),
title: "Map the constellations".to_string(),
draft: false,
}),

View file

@ -1281,6 +1281,7 @@ mod runs {
fn parse_failure_reason(reason: &str) -> Option<FailureReason> {
match reason {
"workflow_error" => Some(FailureReason::WorkflowError),
"publish_failed" => Some(FailureReason::PublishFailed),
"cancelled" => Some(FailureReason::Cancelled),
"approval_denied" => Some(FailureReason::ApprovalDenied),
"terminated" => Some(FailureReason::Terminated),

View file

@ -2053,6 +2053,7 @@ fn build_github_app_manifest(
"public": false,
"default_permissions": {
"contents": "write",
"workflows": "write",
"metadata": "read",
"pull_requests": "write",
"checks": "write",
@ -2354,11 +2355,27 @@ mod tests {
InstallObjectStoreCredentialMode, InstallObjectStoreInput, InstallObjectStoreProvider,
InstallObjectStoreState, InstallSandboxProviderState, InstallSandboxState,
InstallTokenQuery, LlmProvidersInput, PendingInstall, ServerConfigInput, ServerSecrets,
classify_object_store_validation_error, detect_canonical_url, install_object_store_lookup,
lock_unpoisoned, post_install_finish, provider_base_url_override,
resolve_install_object_store_state, token_is_valid, write_artifact_store_metadata,
build_github_app_manifest, classify_object_store_validation_error, detect_canonical_url,
install_object_store_lookup, lock_unpoisoned, post_install_finish,
provider_base_url_override, resolve_install_object_store_state, token_is_valid,
write_artifact_store_metadata,
};
#[test]
fn github_app_manifest_allows_workflow_file_writes() {
let manifest = build_github_app_manifest(
"Fabro Test",
"https://fabro.example/setup",
"https://fabro.example/auth/callback/github",
"https://fabro.example/setup",
);
assert_eq!(
manifest["default_permissions"]["workflows"],
serde_json::Value::String("write".to_string())
);
}
#[test]
fn token_validation_accepts_any_matching_source() {
let state = InstallAppState::for_test("expected");

View file

@ -4172,9 +4172,15 @@ async fn execute_run_in_process(state: Arc<AppState>, run_id: RunId) {
let mut runs = state.runs.lock().expect("runs lock poisoned");
if let Some(managed_run) = runs.get_mut(&run_id) {
match &result {
ExecutionResult::Completed(result) => match result.as_ref() {
Ok(started) => match &started.finalized.outcome {
Ok(_) => {
ExecutionResult::Completed(result) => {
// A run can fail either before it produces a `Started` or in
// its own outcome; both carry the same `WorkflowError`.
let outcome = match result.as_ref() {
Ok(started) => started.finalized.outcome.as_ref().map(|_| ()),
Err(e) => Err(e),
};
match outcome {
Ok(()) => {
info!(run_id = %run_id, "Run completed");
managed_run.status = RunStatus::Succeeded {
reason: SuccessReason::Completed,
@ -4187,27 +4193,15 @@ async fn execute_run_in_process(state: Arc<AppState>, run_id: RunId) {
};
}
Err(e) => {
error!(run_id = %run_id, error = %e, "Run failed");
let detail = e.display_with_causes();
error!(run_id = %run_id, error = %detail, "Run failed");
managed_run.status = RunStatus::Failed {
reason: FailureReason::WorkflowError,
reason: e.failure_reason(),
};
managed_run.error = Some(e.to_string());
managed_run.error = Some(detail);
}
},
Err(WorkflowError::Cancelled) => {
info!(run_id = %run_id, "Run cancelled");
managed_run.status = RunStatus::Failed {
reason: FailureReason::Cancelled,
};
}
Err(e) => {
error!(run_id = %run_id, error = %e, "Run failed");
managed_run.status = RunStatus::Failed {
reason: FailureReason::WorkflowError,
};
managed_run.error = Some(e.to_string());
}
},
}
ExecutionResult::CancelledBySignal => {
info!(run_id = %run_id, "Run cancelled");
managed_run.status = RunStatus::Failed {

View file

@ -165,6 +165,7 @@ struct RunPrInputs<'a> {
goal: &'a str,
base_branch: &'a str,
run_branch: &'a str,
final_git_sha: &'a str,
diff: &'a str,
conclusion: &'a fabro_types::Conclusion,
normalized_origin: String,
@ -224,6 +225,17 @@ impl<'a> RunPrInputs<'a> {
"run_not_finished",
)
})?;
let final_git_sha = conclusion
.final_git_commit_sha
.as_deref()
.filter(|sha| !sha.trim().is_empty())
.ok_or_else(|| {
ApiError::with_code(
StatusCode::BAD_REQUEST,
"Run has no final git commit SHA — the remote branch cannot be verified.",
"missing_final_git_commit",
)
})?;
if !force && !conclusion.status.is_successful() {
return Err(ApiError::with_code(
StatusCode::BAD_REQUEST,
@ -240,6 +252,7 @@ impl<'a> RunPrInputs<'a> {
goal: run_spec.graph.goal(),
base_branch,
run_branch,
final_git_sha,
diff,
conclusion,
normalized_origin,
@ -323,6 +336,7 @@ async fn create_run_pull_request(
origin_url: &inputs.normalized_origin,
base_branch: inputs.base_branch,
head_branch: inputs.run_branch,
expected_head_sha: inputs.final_git_sha,
goal: inputs.goal,
diff: inputs.diff,
model: &model,
@ -334,15 +348,8 @@ async fn create_run_pull_request(
conclusion: Some(inputs.conclusion),
run_state: Some(run_state),
};
let created_pull_request = match pull_request::maybe_open_pull_request(request).await {
Ok(Some(created)) => created,
Ok(None) => {
return ApiError::new(
StatusCode::INTERNAL_SERVER_ERROR,
"Pull request creation returned no record unexpectedly.",
)
.into_response();
}
let created_pull_request = match pull_request::open_pull_request(request).await {
Ok(created) => created,
Err(err) => return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(),
};
@ -350,6 +357,7 @@ async fn create_run_pull_request(
&created_pull_request.link,
&created_pull_request.base_branch,
&created_pull_request.head_branch,
inputs.final_git_sha,
&created_pull_request.title,
true,
);

View file

@ -4685,6 +4685,7 @@ channel = "#deploys"
repo: "fabro".to_string(),
base_branch: "main".to_string(),
head_branch: "fabro/run/test".to_string(),
head_sha: Some("final-sha".to_string()),
title: "Ship <prod> & notify".to_string(),
draft: false,
},
@ -6541,6 +6542,7 @@ async fn create_run_with_pull_request_record(
repo: "widgets".to_string(),
base_branch: "main".to_string(),
head_branch: "feature".to_string(),
head_sha: Some("final-sha".to_string()),
title: title.to_string(),
draft: false,
},
@ -6635,7 +6637,7 @@ async fn create_completed_run_ready_for_pull_request(
status: "succeeded".to_string(),
reason: SuccessReason::Completed,
total_usd_micros: None,
final_git_commit_sha: None,
final_git_commit_sha: Some("final-sha".to_string()),
final_patch: Some(final_patch.to_string()),
diff_summary: None,
billing: None,
@ -9225,6 +9227,14 @@ async fn get_run_pull_request_returns_stored_github_association_when_github_pr_i
#[tokio::test]
async fn create_run_pull_request_creates_and_persists_record() {
let github = MockServer::start();
let branch_mock = github.mock(|when, then| {
when.method("GET")
.path("/repos/acme/widgets/branches/fabro/run/42")
.header("authorization", "Bearer ghu_test");
then.status(200)
.header("content-type", "application/json")
.body(json!({ "commit": { "sha": "final-sha" } }).to_string());
});
let create_mock = github.mock(|when, then| {
when.method("POST")
.path("/repos/acme/widgets/pulls")
@ -9322,6 +9332,7 @@ async fn create_run_pull_request_creates_and_persists_record() {
assert_eq!(state_body["pull_request"]["repo"], "widgets");
response_mock.assert_async().await;
branch_mock.assert();
create_mock.assert();
}
@ -16662,6 +16673,7 @@ async fn list_runs_includes_live_metadata_from_run_state() {
repo: "repo".to_string(),
base_branch: "main".to_string(),
head_branch: "fabro/run".to_string(),
head_sha: Some("final-sha".to_string()),
title: "Fix board metadata".to_string(),
draft: false,
},

View file

@ -596,7 +596,10 @@ async fn mint_installation_token_with_jwt(
})
}
/// Request a scoped Installation Access Token with `contents: write`.
/// Request a scoped Installation Access Token for git writes.
///
/// The `workflows` permission is required when a pushed commit creates or
/// updates files under `.github/workflows/`.
pub async fn create_installation_access_token(
client: &impl HttpClient,
jwt: &str,
@ -610,7 +613,7 @@ pub async fn create_installation_access_token(
owner,
repo,
base_url,
serde_json::json!({ "contents": "write" }),
serde_json::json!({ "contents": "write", "workflows": "write" }),
)
.await
}
@ -894,27 +897,36 @@ fn normalize_https_host_path(url: &str) -> String {
}
}
/// Check whether a branch exists in a GitHub repository.
/// Return the commit SHA at the head of a GitHub branch.
///
/// Uses a GitHub App installation token to query the branches API.
/// Returns `true` if the branch exists, `false` if it doesn't (404).
pub async fn branch_exists(
/// Returns `None` when the branch does not exist.
pub async fn branch_head_sha(
ctx: &GitHubContext<'_>,
owner: &str,
repo: &str,
branch: &str,
) -> anyhow::Result<bool> {
) -> anyhow::Result<Option<String>> {
let client = ctx.http_client()?;
branch_exists_with_client(&client, ctx, owner, repo, branch).await
branch_head_sha_with_client(&client, ctx, owner, repo, branch).await
}
async fn branch_exists_with_client(
async fn branch_head_sha_with_client(
client: &impl HttpClient,
ctx: &GitHubContext<'_>,
owner: &str,
repo: &str,
branch: &str,
) -> anyhow::Result<bool> {
) -> anyhow::Result<Option<String>> {
#[derive(Deserialize)]
struct BranchResponse {
commit: BranchCommit,
}
#[derive(Deserialize)]
struct BranchCommit {
sha: String,
}
let token = ctx
.creds
.resolve_bearer_token(
@ -922,7 +934,7 @@ async fn branch_exists_with_client(
owner,
repo,
ctx.base_url,
serde_json::json!({ "contents": "write" }),
serde_json::json!({ "contents": "read" }),
)
.await?;
@ -931,12 +943,17 @@ async fn branch_exists_with_client(
let resp = client
.request(HttpMethod::Get, &url, &github_headers(&auth), None)
.await
.context("Failed to check branch existence")?;
.context("Failed to read remote branch head")?;
match resp.status {
200 => Ok(true),
404 => Ok(false),
status => bail!("Unexpected status {status} checking branch '{branch}'"),
200 => {
let branch: BranchResponse = resp
.json()
.context("Failed to parse remote branch response")?;
Ok(Some(branch.commit.sha))
}
404 => Ok(None),
status => bail!("Unexpected status {status} reading branch '{branch}'"),
}
}
@ -1041,9 +1058,10 @@ pub async fn update_app_webhook_config(
/// Resolve git clone credentials for a GitHub repository.
///
/// Returns `(username, password)` for authenticated cloning.
/// Returns `(username, password)` for authenticated cloning and pushing.
/// Always generates a token regardless of repo visibility, since the token
/// is needed for pushing from the sandbox.
/// is needed for pushing from the sandbox. The token includes `workflows:
/// write` so a run can publish workflow-file changes.
pub async fn resolve_clone_credentials(
ctx: &GitHubContext<'_>,
owner: &str,
@ -1054,20 +1072,30 @@ pub async fn resolve_clone_credentials(
GitHubCredentials::Installation(token) => token.valid_token()?.to_string(),
GitHubCredentials::App(_) => {
let client = ctx.http_client()?;
ctx.creds
.resolve_bearer_token(
&client,
owner,
repo,
ctx.base_url,
serde_json::json!({ "contents": "write" }),
)
.await?
mint_git_write_token(&client, ctx, owner, repo).await?
}
};
Ok((Some("x-access-token".to_string()), Some(token)))
}
/// Mint an installation token scoped for git writes, including workflow files.
async fn mint_git_write_token(
client: &impl HttpClient,
ctx: &GitHubContext<'_>,
owner: &str,
repo: &str,
) -> anyhow::Result<String> {
ctx.creds
.resolve_bearer_token(
client,
owner,
repo,
ctx.base_url,
serde_json::json!({ "contents": "write", "workflows": "write" }),
)
.await
}
/// Embed a token into an HTTPS URL for authenticated git operations.
///
/// Converts `https://github.com/owner/repo` to
@ -1794,7 +1822,9 @@ mod tests {
r#"{"token": "ghs_xxx", "expires_at": "2099-01-01T00:00:00Z"}"#,
)
.with_req_header("Authorization", "Bearer test-jwt")
.with_req_body(r#"{"permissions":{"contents":"write"},"repositories":["repo"]}"#);
.with_req_body(
r#"{"permissions":{"contents":"write","workflows":"write"},"repositories":["repo"]}"#,
);
let token = create_installation_access_token(&mock, "test-jwt", "owner", "repo", "")
.await
@ -1927,12 +1957,21 @@ mod tests {
}
// -----------------------------------------------------------------------
// branch_exists
// branch_head_sha
// -----------------------------------------------------------------------
#[tokio::test]
async fn branch_exists_returns_true_on_200() {
let mock = MockHttpClient::new()
fn app_creds() -> GitHubCredentials {
GitHubCredentials::App(GitHubAppCredentials {
app_id: "test".to_string(),
private_key_pem: test_rsa_key().to_string(),
slug: None,
})
}
/// Mock the installation-token exchange every App-credentialed call makes
/// before it reaches the endpoint under test.
fn mock_with_installation_token() -> MockHttpClient {
MockHttpClient::new()
.on(
HttpMethod::Get,
"/repos/owner/repo/installation",
@ -1945,20 +1984,19 @@ mod tests {
201,
r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#,
)
.on(
HttpMethod::Get,
"/repos/owner/repo/branches/my-branch",
200,
r#"{"name": "my-branch"}"#,
);
}
let pem = test_rsa_key();
let creds = GitHubCredentials::App(GitHubAppCredentials {
app_id: "test".to_string(),
private_key_pem: pem.to_string(),
slug: None,
});
let result = branch_exists_with_client(
#[tokio::test]
async fn branch_head_sha_returns_commit_on_200() {
let mock = mock_with_installation_token().on(
HttpMethod::Get,
"/repos/owner/repo/branches/my-branch",
200,
r#"{"name": "my-branch", "commit": {"sha": "abc123"}}"#,
);
let creds = app_creds();
let result = branch_head_sha_with_client(
&mock,
&GitHubContext::new(&creds, ""),
"owner",
@ -1966,38 +2004,21 @@ mod tests {
"my-branch",
)
.await;
assert!(result.unwrap());
assert_eq!(result.unwrap(), Some("abc123".to_string()));
}
#[tokio::test]
async fn branch_exists_returns_false_on_404() {
let mock = MockHttpClient::new()
.on(
HttpMethod::Get,
"/repos/owner/repo/installation",
200,
r#"{"id": 1}"#,
)
.on(
HttpMethod::Post,
"/app/installations/1/access_tokens",
201,
r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#,
)
.on(
HttpMethod::Get,
"/repos/owner/repo/branches/no-such-branch",
404,
"",
);
async fn branch_head_sha_returns_none_on_404() {
let mock = mock_with_installation_token().on(
HttpMethod::Get,
"/repos/owner/repo/branches/no-such-branch",
404,
"",
);
let pem = test_rsa_key();
let creds = GitHubCredentials::App(GitHubAppCredentials {
app_id: "test".to_string(),
private_key_pem: pem.to_string(),
slug: None,
});
let result = branch_exists_with_client(
let creds = app_creds();
let result = branch_head_sha_with_client(
&mock,
&GitHubContext::new(&creds, ""),
"owner",
@ -2005,38 +2026,21 @@ mod tests {
"no-such-branch",
)
.await;
assert!(!result.unwrap());
assert_eq!(result.unwrap(), None);
}
#[tokio::test]
async fn branch_exists_returns_error_on_500() {
let mock = MockHttpClient::new()
.on(
HttpMethod::Get,
"/repos/owner/repo/installation",
200,
r#"{"id": 1}"#,
)
.on(
HttpMethod::Post,
"/app/installations/1/access_tokens",
201,
r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#,
)
.on(
HttpMethod::Get,
"/repos/owner/repo/branches/broken",
500,
"",
);
async fn branch_head_sha_returns_error_on_500() {
let mock = mock_with_installation_token().on(
HttpMethod::Get,
"/repos/owner/repo/branches/broken",
500,
"",
);
let pem = test_rsa_key();
let creds = GitHubCredentials::App(GitHubAppCredentials {
app_id: "test".to_string(),
private_key_pem: pem.to_string(),
slug: None,
});
let result = branch_exists_with_client(
let creds = app_creds();
let result = branch_head_sha_with_client(
&mock,
&GitHubContext::new(&creds, ""),
"owner",
@ -2044,22 +2048,23 @@ mod tests {
"broken",
)
.await;
assert!(result.is_err());
}
#[tokio::test]
async fn branch_exists_with_token_uses_direct_bearer_token() {
async fn branch_head_sha_with_token_uses_direct_bearer_token() {
let mock = MockHttpClient::new()
.on(
HttpMethod::Get,
"/repos/owner/repo/branches/my-branch",
200,
r#"{"name": "my-branch"}"#,
r#"{"name": "my-branch", "commit": {"sha": "abc123"}}"#,
)
.with_req_header("Authorization", "Bearer ghu_test");
let creds = GitHubCredentials::Pat("ghu_test".to_string());
let result = branch_exists_with_client(
let result = branch_head_sha_with_client(
&mock,
&GitHubContext::new(&creds, ""),
"owner",
@ -2068,7 +2073,7 @@ mod tests {
)
.await;
assert!(result.unwrap());
assert_eq!(result.unwrap(), Some("abc123".to_string()));
}
// -----------------------------------------------------------------------
@ -2315,6 +2320,37 @@ mod tests {
);
}
#[tokio::test]
async fn resolve_clone_credentials_requests_workflow_write_permission() {
let mock = MockHttpClient::new()
.on(
HttpMethod::Get,
"/repos/owner/repo/installation",
200,
r#"{"id": 123}"#,
)
.on(
HttpMethod::Post,
"/app/installations/123/access_tokens",
201,
r#"{"token": "ghs_xxx", "expires_at": "2099-01-01T00:00:00Z"}"#,
)
.with_req_body(
r#"{"permissions":{"contents":"write","workflows":"write"},"repositories":["repo"]}"#,
);
let credentials = GitHubCredentials::App(GitHubAppCredentials {
app_id: "test".to_string(),
private_key_pem: test_rsa_key().to_string(),
slug: None,
});
let context = GitHubContext::new(&credentials, "");
let token = mint_git_write_token(&mock, &context, "owner", "repo")
.await
.unwrap();
assert_eq!(token, "ghs_xxx");
}
#[test]
fn installation_token_valid_token_rejects_expired_tokens() {
let expired = InstallationToken {

View file

@ -4480,6 +4480,7 @@ mod tests {
repo: "fabro".to_string(),
base_branch: "main".to_string(),
head_branch: "fabro/run/demo".to_string(),
head_sha: Some("final-sha".to_string()),
title: "Add run PR chip".to_string(),
draft: false,
}),
@ -4529,6 +4530,7 @@ mod tests {
repo: github_pull_request.repo.clone(),
base_branch: "main".to_string(),
head_branch: "fabro/run/demo".to_string(),
head_sha: Some("final-sha".to_string()),
title: "Add run PR chip".to_string(),
draft: false,
}),

View file

@ -46,6 +46,7 @@ fabro-static.workspace = true
fabro-types = { path = "../../foundation/fabro-types" }
fabro-http.workspace = true
thiserror.workspace = true
strum.workspace = true
serde.workspace = true
serde_json.workspace = true
jsonschema.workspace = true

View file

@ -67,7 +67,8 @@ assert_eq!(graph.goal(), "Run tests");
use fabro_workflow::operations::start;
use fabro_workflow::pipeline;
// Use `operations::start(...)` for the full initialize -> execute -> finalize flow.
// Use `operations::start(...)` for the full
// initialize -> execute -> conclude -> publish -> finalize flow.
// Use `pipeline::initialize(...)` + `pipeline::execute(...)` when you need partial lifecycle control.
```

View file

@ -1,5 +1,5 @@
use std::fmt;
use std::sync::Arc;
use std::sync::{Arc, LazyLock};
use fabro_graphviz::Error as GraphvizError;
use fabro_llm::{Error as LlmError, ProviderErrorKind};
@ -11,6 +11,7 @@ use fabro_types::settings::AmbiguousModelRef;
use fabro_types::{ExecOutputTail, FailureReason, RunFailure};
use fabro_util::error::{SharedError, collect_causes, collect_chain, render_with_causes};
use fabro_validate::Diagnostic;
use regex::Regex;
use thiserror::Error as ThisError;
use crate::outcome::{FailureDetail, Outcome, StageOutcome};
@ -153,13 +154,21 @@ impl miette::Diagnostic for SharedTemplateError {
}
}
/// Matches git SHAs and other long hex blobs.
static HEX_RE: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"\b[0-9a-f]{7,64}\b").expect("hardcoded regex should compile"));
/// Classify a failure reason string using heuristics.
///
/// This is the fallback when structured error information is not available
/// (e.g. for `Handler(String)` or `Engine(String)` errors).
#[must_use]
pub fn classify_failure_reason(reason: &str) -> FailureCategory {
let lower = reason.to_lowercase();
// Mask commit SHAs first. They are hex, so one contains "500" or "503"
// often enough to matter, which would read as a transient infra hint. The
// bare status codes those hints look for are too short to be masked.
let lowered = reason.to_lowercase();
let lower = HEX_RE.replace_all(&lowered, "<hex>");
if lower.contains("interrupt")
|| (lower.contains("cancel")
@ -196,13 +205,6 @@ pub fn classify_failure_reason(reason: &str) -> FailureCategory {
/// semantically identical errors produce the same signature regardless of
/// line numbers, commit hashes, or timestamps.
pub fn normalize_failure_reason(reason: &str) -> String {
use std::sync::LazyLock;
use regex::Regex;
static HEX_RE: LazyLock<Regex> = LazyLock::new(|| {
Regex::new(r"\b[0-9a-f]{7,64}\b").expect("hardcoded regex should compile")
});
static DIGITS_RE: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"\b\d+\b").expect("hardcoded regex should compile"));
static COMMA_SPACE_RE: LazyLock<Regex> =
@ -252,6 +254,21 @@ impl FailureSignatureExt for FailureSignature {
}
}
/// Pipeline stage that produced an [`Error::Stage`].
///
/// The three stages share a failure shape — a message, an eagerly classified
/// [`FailureCategory`], an optional command output tail, and an optional
/// source — and differ only in where they run and whether a retry is possible.
#[derive(Debug, Clone, Copy, PartialEq, Eq, strum::Display)]
pub enum ErrorStage {
/// A node handler failed. Retryable: the engine can re-run the node.
Handler,
/// The engine itself failed while driving the graph. Retryable.
Engine,
/// The publish stage failed. Terminal: publish runs once, after execution.
Publish,
}
#[derive(ThisError, Debug, Clone)]
pub enum Error {
#[error("Parse error: {0}")]
@ -276,17 +293,9 @@ pub enum Error {
source: SharedTemplateError,
},
#[error("Engine error: {message}")]
Engine {
message: String,
failure_class: FailureCategory,
exec_output_tail: Option<ExecOutputTail>,
#[source]
source: Option<SharedError>,
},
#[error("Handler error: {message}")]
Handler {
#[error("{stage} error: {message}")]
Stage {
stage: ErrorStage,
message: String,
failure_class: FailureCategory,
exec_output_tail: Option<ExecOutputTail>,
@ -325,17 +334,48 @@ pub enum Error {
impl Error {
/// Smart constructor for Handler errors. Classifies the failure reason
/// eagerly.
pub fn handler(message: impl Into<String>) -> Self {
/// Build a stage error, classifying the message eagerly.
fn stage(
stage: ErrorStage,
message: impl Into<String>,
exec_output_tail: Option<ExecOutputTail>,
) -> Self {
let message = message.into();
let failure_class = classify_failure_reason(&message);
Self::Handler {
Self::Stage {
stage,
message,
failure_class,
exec_output_tail: None,
exec_output_tail,
source: None,
}
}
/// Build a stage error from a source, classifying the rendered chain so
/// hints buried in the causes still reach [`Self::failure_category`].
fn stage_with_source(
stage: ErrorStage,
message: impl Into<String>,
source: impl Into<anyhow::Error>,
exec_output_tail: Option<ExecOutputTail>,
) -> Self {
let message = message.into();
let source = SharedError::new(source.into());
let failure_class =
classify_failure_reason(&render_with_causes(&message, &collect_chain(&source)));
Self::Stage {
stage,
message,
failure_class,
exec_output_tail,
source: Some(source),
}
}
pub fn handler(message: impl Into<String>) -> Self {
Self::stage(ErrorStage::Handler, message, None)
}
pub fn template(message: impl Into<String>, source: TemplateError) -> Self {
Self::Template {
message: message.into(),
@ -347,14 +387,7 @@ impl Error {
message: impl Into<String>,
exec_output_tail: Option<ExecOutputTail>,
) -> Self {
let message = message.into();
let failure_class = classify_failure_reason(&message);
Self::Handler {
message,
failure_class,
exec_output_tail,
source: None,
}
Self::stage(ErrorStage::Handler, message, exec_output_tail)
}
pub fn handler_with_source(
@ -369,61 +402,52 @@ impl Error {
source: impl Into<anyhow::Error>,
exec_output_tail: Option<ExecOutputTail>,
) -> Self {
let message = message.into();
let source = SharedError::new(source.into());
let causes = collect_chain(&source);
let rendered = render_with_causes(&message, &causes);
let failure_class = classify_failure_reason(&rendered);
Self::Handler {
message,
failure_class,
exec_output_tail,
source: Some(source),
}
Self::stage_with_source(ErrorStage::Handler, message, source, exec_output_tail)
}
pub fn handler_with_anyhow(message: impl Into<String>, source: anyhow::Error) -> Self {
Self::handler_with_source(message, source)
}
/// Smart constructor for Engine errors. Classifies the failure reason
/// eagerly.
pub fn engine(message: impl Into<String>) -> Self {
let message = message.into();
let failure_class = classify_failure_reason(&message);
Self::Engine {
message,
failure_class,
exec_output_tail: None,
source: None,
}
Self::stage(ErrorStage::Engine, message, None)
}
pub fn engine_with_source(
message: impl Into<String>,
source: impl Into<anyhow::Error>,
) -> Self {
let message = message.into();
let source = SharedError::new(source.into());
let causes = collect_chain(&source);
let rendered = render_with_causes(&message, &causes);
let failure_class = classify_failure_reason(&rendered);
Self::Engine {
message,
failure_class,
exec_output_tail: None,
source: Some(source),
}
Self::stage_with_source(ErrorStage::Engine, message, source, None)
}
pub fn engine_with_anyhow(message: impl Into<String>, source: anyhow::Error) -> Self {
Self::engine_with_source(message, source)
}
/// Build an error for the required publish stage.
pub fn publish(message: impl Into<String>) -> Self {
Self::stage(ErrorStage::Publish, message, None)
}
pub fn publish_with_source(
message: impl Into<String>,
source: impl Into<anyhow::Error>,
) -> Self {
Self::publish_with_source_and_exec_output_tail(message, source, None)
}
pub fn publish_with_source_and_exec_output_tail(
message: impl Into<String>,
source: impl Into<anyhow::Error>,
exec_output_tail: Option<ExecOutputTail>,
) -> Self {
Self::stage_with_source(ErrorStage::Publish, message, source, exec_output_tail)
}
#[must_use]
pub fn causes(&self) -> Vec<String> {
match self {
Self::Engine { source, .. } | Self::Handler { source, .. } => source
Self::Stage { source, .. } => source
.as_ref()
.map_or_else(Vec::new, |source| collect_chain(source)),
Self::Template { source, .. } => collect_chain(source),
@ -439,15 +463,17 @@ impl Error {
/// Whether this error category is retryable (transient) or terminal.
///
/// Retryable: Handler (transient handler failures), Engine (could be
/// transient), Io (network/disk issues are often transient),
/// Llm (delegates to SdkError). Terminal: Parse, Validation,
/// OutputSchemaValidation, Stylesheet (configuration errors), Checkpoint
/// (storage integrity), Cancelled (explicit cancellation).
/// Retryable: Handler and Engine stages (the engine can re-run the node),
/// I/O, and LLM errors the SDK marks retryable. Terminal: the Publish
/// stage (it runs once, after execution), Parse, Validation,
/// OutputSchemaValidation, Stylesheet, Checkpoint, and Cancelled.
#[must_use]
pub fn is_retryable(&self) -> bool {
match self {
Self::Handler { .. } | Self::Engine { .. } | Self::Io(_) => true,
Self::Io(_) => true,
Self::Stage { stage, .. } => {
matches!(stage, ErrorStage::Handler | ErrorStage::Engine)
}
Self::Llm(sdk_err) => sdk_err.retryable(),
Self::Parse(_)
| Self::Validation(_)
@ -483,9 +509,20 @@ impl Error {
| Self::Unsupported(_)
| Self::OutputSchemaValidation(_) => FailureCategory::Deterministic,
Self::Precondition(_) | Self::RunNotFound(_) => FailureCategory::Structural,
Self::Handler { failure_class, .. } | Self::Engine { failure_class, .. } => {
*failure_class
}
Self::Stage { failure_class, .. } => *failure_class,
}
}
/// The terminal [`FailureReason`] this error maps to on a run.
#[must_use]
pub fn failure_reason(&self) -> FailureReason {
match self {
Self::Cancelled => FailureReason::Cancelled,
Self::Stage {
stage: ErrorStage::Publish,
..
} => FailureReason::PublishFailed,
_ => FailureReason::WorkflowError,
}
}
@ -501,18 +538,13 @@ impl Error {
#[must_use]
pub fn to_failure_detail(&self) -> FailureDetail {
let message = match self {
Self::Engine { message, .. } | Self::Handler { message, .. } => message.clone(),
_ => self.to_string(),
};
let explicit_exec_output_tail = match self {
Self::Engine {
exec_output_tail, ..
}
| Self::Handler {
exec_output_tail, ..
} => exec_output_tail.clone(),
_ => None,
let (message, explicit_exec_output_tail) = match self {
Self::Stage {
message,
exec_output_tail,
..
} => (message.clone(), exec_output_tail.clone()),
_ => (self.to_string(), None),
};
FailureDetail {
message,
@ -825,7 +857,10 @@ mod tests {
source,
});
assert!(matches!(fabro_error, Error::Engine { .. }));
assert!(matches!(fabro_error, Error::Stage {
stage: ErrorStage::Engine,
..
}));
let message = fabro_error.to_string();
assert!(message.contains("deserialize run spec on branch fabro/meta/run-1"));
assert!(message.contains(&source_message));
@ -1974,6 +2009,7 @@ mod tests {
}],
},
Error::engine("engine err"),
Error::publish("publish err"),
Error::handler("handler err"),
Error::Llm(SdkError::Network {
message: "refused".into(),
@ -2005,6 +2041,32 @@ mod tests {
);
}
/// Publish runs once, after execution, so no caller can retry it — even
/// when the message looks transient. The failure category is still
/// classified for reporting.
#[test]
fn publish_errors_are_terminal() {
assert!(!Error::publish("connection timed out").is_retryable());
assert!(!Error::publish("permission denied").is_retryable());
assert_eq!(
Error::publish("connection timed out").failure_category(),
FailureCategory::TransientInfra
);
}
#[test]
fn failure_reason_distinguishes_publish_and_cancelled() {
assert_eq!(
Error::publish("nope").failure_reason(),
FailureReason::PublishFailed
);
assert_eq!(Error::Cancelled.failure_reason(), FailureReason::Cancelled);
assert_eq!(
Error::engine("boom").failure_reason(),
FailureReason::WorkflowError
);
}
#[test]
fn failure_class_stability() {
let messages = [
@ -2025,6 +2087,23 @@ mod tests {
}
}
/// Commit SHAs are hex, so they contain digit runs like "503" often enough
/// to matter. Masking them keeps a deterministic failure from being
/// reported as transient just because of the SHA it names.
#[test]
fn commit_shas_do_not_trip_transient_infra_hints() {
let sha = "a503b1c9d4e2f7a8b6c3d0e1f2a3b4c5d6e7f8a9";
assert_eq!(
classify_failure_reason(&format!("failed to push final commit {sha} to branch 'x'")),
FailureCategory::Deterministic
);
// A real status code is still a transient hint.
assert_eq!(
classify_failure_reason("push rejected with 503"),
FailureCategory::TransientInfra
);
}
#[test]
fn to_fail_outcome_preserves_class() {
let err = Error::handler("timeout");

View file

@ -1313,6 +1313,7 @@ fn event_body_from_event(event: &Event) -> EventBody {
repo,
base_branch,
head_branch,
head_sha,
title,
draft,
} => EventBody::PullRequestCreated(fabro_types::PullRequestCreatedProps {
@ -1322,6 +1323,7 @@ fn event_body_from_event(event: &Event) -> EventBody {
repo: repo.clone(),
base_branch: base_branch.clone(),
head_branch: head_branch.clone(),
head_sha: head_sha.clone(),
title: title.clone(),
draft: *draft,
}),

View file

@ -733,6 +733,9 @@ pub enum Event {
repo: String,
base_branch: String,
head_branch: String,
/// Absent on events written before the head SHA was recorded.
#[serde(default, skip_serializing_if = "Option::is_none")]
head_sha: Option<String>,
title: String,
draft: bool,
},
@ -772,6 +775,7 @@ impl Event {
record: &PullRequestLink,
base_branch: &str,
head_branch: &str,
head_sha: &str,
title: &str,
draft: bool,
) -> Self {
@ -782,6 +786,7 @@ impl Event {
repo: record.repo.clone(),
base_branch: base_branch.to_string(),
head_branch: head_branch.to_string(),
head_sha: Some(head_sha.to_string()),
title: title.to_string(),
draft,
}

View file

@ -496,7 +496,7 @@ impl RunLifecycle<WorkflowGraph> for EventLifecycle {
}
for push in &result.push_results {
self.emitter.emit(&Event::GitPush {
branch: push.refspec.clone(),
branch: push.branch.clone(),
success: push.success,
exec_output_tail: push.exec_output_tail.clone(),
});

View file

@ -69,11 +69,24 @@ pub(crate) struct GitCheckpointResult {
#[derive(Debug, Clone)]
pub(crate) struct PushResult {
pub refspec: String,
pub branch: String,
pub success: bool,
pub exec_output_tail: Option<fabro_types::ExecOutputTail>,
}
/// Push a run branch to its remote counterpart.
///
/// Owns the refspec convention so the checkpoint push and the terminal publish
/// push cannot drift apart.
pub(crate) async fn push_run_branch(
sandbox: &dyn fabro_sandbox::Sandbox,
branch: &str,
) -> fabro_sandbox::Result<()> {
sandbox
.git_push_ref(&format!("refs/heads/{branch}:refs/heads/{branch}"))
.await
}
/// Sub-lifecycle responsible for git operations (checkpoint commits, pushes,
/// diffs).
pub(crate) struct GitLifecycle {
@ -307,15 +320,14 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
.as_ref()
.and_then(|g| g.run_branch.as_ref())
{
let refspec = format!("refs/heads/{branch}:refs/heads/{branch}");
let (push_ok, exec_output_tail) =
match self.sandbox.git_push_ref(&refspec).await {
match push_run_branch(self.sandbox.as_ref(), branch).await {
Ok(()) => (true, None),
Err(err) => {
let exec_output_tail =
fabro_sandbox::default_redacted_output_tail(&err);
tracing::warn!(
refspec = %refspec,
branch = %branch,
error = %fabro_sandbox::display_for_log(&err),
"git push from run lifecycle failed"
);
@ -329,7 +341,7 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
}
};
git_result.push_results.push(PushResult {
refspec,
branch: branch.clone(),
success: push_ok,
exec_output_tail,
});

View file

@ -37,8 +37,8 @@ use crate::event::{
use crate::handler::HandlerRegistry;
use crate::outcome::{Outcome, StageOutcome};
use crate::pipeline::{
self, FinalizeOptions, Finalized, InitOptions, LlmSpec, Persisted, PullRequestOptions,
ResumeState, SandboxEnvSpec, build_conclusion_from_store, classify_engine_result,
self, FinalizeOptions, Finalized, InitOptions, LlmSpec, Persisted, PublishOptions, ResumeState,
SandboxEnvSpec, build_conclusion_from_store, classify_engine_result,
};
#[cfg(test)]
use crate::records::Checkpoint;
@ -794,7 +794,7 @@ fn runtime_setup_commands(
}
impl RunSession {
/// Shared engine: initialize, execute, finalize, pull_request.
/// Shared engine: initialize, execute, conclude, publish, finalize.
async fn run(
self,
persisted: Persisted,
@ -921,22 +921,26 @@ impl RunSession {
.expect("last_git_sha mutex should not be poisoned: no code panics while holding this lock")
.clone(),
};
let pr_opts = PullRequestOptions {
let publish_opts = PublishOptions {
pr_config: self.pr_config,
github_app: self.pr_github_app,
origin_url: self.pr_origin_url,
model: self.pr_model,
};
let concluded = match Box::pin(pipeline::finalize(executed, &finalize_opts)).await {
Ok(concluded) => concluded,
let concluding = async {
let concluded = Box::pin(pipeline::conclude(executed, &finalize_opts)).await?;
let published = Box::pin(pipeline::publish(concluded, &publish_opts)).await;
Box::pin(pipeline::finalize(published, &finalize_opts)).await
};
let finalized = match concluding.await {
Ok(finalized) => finalized,
Err(err) => {
self.steering_hub.drain_pending_at_run_end();
store_progress_logger.flush().await;
return Err(err);
}
};
let finalized = Box::pin(pipeline::pull_request(concluded, &pr_opts)).await;
// Emit `agent.steer.dropped { reason: run_ended }` for any
// unconsumed pending steers on the success path, then flush. The
// scopeguard above re-runs as a no-op (drain is idempotent on an

View file

@ -9,7 +9,7 @@ use fabro_types::{BilledTokenCounts, DiffSummary, EventBody, RunFailure, RunProj
use fabro_util::error::collect_causes;
use fabro_util::time::elapsed_ms;
use super::types::{Concluded, Executed, FinalizeOptions};
use super::types::{Concluded, Executed, FinalizeOptions, Finalized, PublishOutcome, Published};
use crate::error::{Error, run_failure_from_error, run_failure_from_outcome_failure};
use crate::event::{Event, RunNoticeCode, RunNoticeLevel};
use crate::outcome::{Outcome, StageOutcome};
@ -44,27 +44,16 @@ pub fn classify_engine_result(
};
(status, failure, run_status)
}
Err(Error::Cancelled) => (
StageOutcome::Failed {
retry_requested: false,
},
Some(run_failure_from_error(
&Error::Cancelled,
FailureReason::Cancelled,
)),
RunStatus::Failed {
reason: FailureReason::Cancelled,
},
),
Err(err) => (
StageOutcome::Failed {
retry_requested: false,
},
Some(run_failure_from_error(err, FailureReason::WorkflowError)),
RunStatus::Failed {
reason: FailureReason::WorkflowError,
},
),
Err(err) => {
let reason = err.failure_reason();
(
StageOutcome::Failed {
retry_requested: false,
},
Some(run_failure_from_error(err, reason)),
RunStatus::Failed { reason },
)
}
}
}
@ -480,10 +469,7 @@ pub(crate) fn build_terminal_event(
}
let failure = match outcome {
Err(Error::Cancelled) => {
run_failure_from_error(&Error::Cancelled, FailureReason::Cancelled)
}
Err(err) => run_failure_from_error(err, FailureReason::WorkflowError),
Err(err) => run_failure_from_error(err, err.failure_reason()),
Ok(outcome) => {
if let Some(failure) = outcome.failure.as_ref() {
run_failure_from_outcome_failure(failure, FailureReason::WorkflowError)
@ -521,16 +507,13 @@ async fn stop_sandbox_on_terminal(
Ok(())
}
/// FINALIZE phase: build conclusion, write the meta branch, emit the terminal
/// `WorkflowRunCompleted`/`WorkflowRunFailed` event.
///
/// The terminal event is emitted here (not from `on_run_end`) so observers
/// can't act on "done" before the meta branch writes are flushed.
/// CONCLUDE phase: collect the execution result, final commit, and diff.
///
/// # Errors
///
/// Returns `Error` if persisting terminal state fails.
pub async fn finalize(executed: Executed, options: &FinalizeOptions) -> Result<Concluded, Error> {
/// Returns `Error` if the run state needed to build the conclusion cannot be
/// collected.
pub async fn conclude(executed: Executed, options: &FinalizeOptions) -> Result<Concluded, Error> {
let Executed {
graph,
outcome,
@ -561,7 +544,7 @@ pub async fn finalize(executed: Executed, options: &FinalizeOptions) -> Result<C
let checkpoint = projection
.as_ref()
.and_then(|state| state.current_checkpoint());
let conclusion = build_conclusion_from_parts(
let mut conclusion = build_conclusion_from_parts(
checkpoint,
&projection_billing,
&projection_order,
@ -571,10 +554,59 @@ pub async fn finalize(executed: Executed, options: &FinalizeOptions) -> Result<C
options.last_git_sha.clone(),
);
let ((final_patch, diff_summary), ()) = tokio::join!(
compute_final_patch(&run_options, &services, final_status),
write_finalize_commit(&run_options, &services, &conclusion),
);
let (final_patch, diff_summary) =
compute_final_patch(&run_options, &services, final_status).await;
conclusion.diff = fabro_types::RunDiff {
patch: final_patch,
summary: diff_summary,
};
Ok(Concluded {
outcome,
conclusion,
artifact_count,
graph,
run_options,
services,
})
}
/// FINALIZE phase: persist the final conclusion, emit the terminal event, and
/// clean up the sandbox.
///
/// This runs after PUBLISH so a required push or pull-request failure becomes
/// the terminal run result.
///
/// # Errors
///
/// Returns `Error` if persisting terminal state fails.
pub async fn finalize(published: Published, options: &FinalizeOptions) -> Result<Finalized, Error> {
let Published {
execution_outcome,
publish_outcome,
publish_error,
mut conclusion,
artifact_count,
run_options,
services,
} = published;
let PublishOutcome {
pushed_branch,
pr_url,
} = publish_outcome;
// An execution failure outranks a publish failure: publish only runs after
// a successful execution, so the two are never both set.
let outcome = match (execution_outcome, publish_error) {
(Err(error), _) | (Ok(_), Some(error)) => Err(error),
(Ok(outcome), None) => Ok(outcome),
};
let (final_status, failure, _run_status) = classify_engine_result(&outcome);
conclusion.status = final_status;
conclusion.failure = failure;
write_finalize_commit(&run_options, &services, &conclusion).await;
if services.metadata_runtime.metadata_degraded() {
services.emitter.notice(
@ -588,9 +620,9 @@ pub async fn finalize(executed: Executed, options: &FinalizeOptions) -> Result<C
&outcome,
conclusion.timing,
artifact_count,
options.last_git_sha.clone(),
final_patch,
diff_summary,
conclusion.final_git_commit_sha.clone(),
conclusion.diff.patch.clone(),
conclusion.diff.summary,
conclusion.billing.clone(),
);
services.emitter.emit(&terminal_event);
@ -626,12 +658,12 @@ pub async fn finalize(executed: Executed, options: &FinalizeOptions) -> Result<C
);
}
Ok(Concluded {
Ok(Finalized {
run_id: run_options.run_id,
outcome,
conclusion,
graph,
run_options,
services,
pushed_branch,
pr_url,
})
}
@ -658,6 +690,7 @@ mod tests {
use super::*;
use crate::context::Context;
use crate::error::ErrorStage;
use crate::event::{Emitter, StoreProgressLogger, append_event};
use crate::run_metadata::{RunMetadataRuntime, RunMetadataWriterHandle};
use crate::run_options::{GitCheckpointOptions, RunOptions};
@ -716,6 +749,21 @@ mod tests {
}
}
async fn finalize_executed(
executed: Executed,
options: &FinalizeOptions,
) -> Result<Finalized, Error> {
let concluded = conclude(executed, options).await?;
let published = crate::pipeline::publish(concluded, &crate::pipeline::PublishOptions {
pr_config: None,
github_app: None,
origin_url: None,
model: "test-model".to_string(),
})
.await;
finalize(published, options).await
}
fn test_store() -> Arc<Database> {
Arc::new(Database::new(
Arc::new(InMemory::new()),
@ -869,6 +917,26 @@ mod tests {
use crate::test_support::test_usage;
#[test]
fn publish_error_builds_publish_failed_terminal_event() {
let event = build_terminal_event(
&Err(Error::publish("GitHub rejected pull request creation")),
fabro_types::RunTiming::wall_only(10),
0,
Some("final-sha".to_string()),
Some("diff".to_string()),
None,
None,
);
match event {
Event::WorkflowRunFailed { failure, .. } => {
assert_eq!(failure.reason, FailureReason::PublishFailed);
}
other => panic!("expected run failure, got {other:?}"),
}
}
#[test]
fn conclusion_stage_order_follows_projection_first_event_order() {
let mut projection = test_projection();
@ -1068,7 +1136,7 @@ mod tests {
services,
);
let concluded = finalize(executed, &FinalizeOptions {
let concluded = finalize_executed(executed, &FinalizeOptions {
run_dir: run_dir.clone(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
@ -1249,7 +1317,7 @@ mod tests {
services,
);
finalize(executed, &FinalizeOptions {
finalize_executed(executed, &FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
@ -1273,6 +1341,337 @@ mod tests {
]);
}
#[tokio::test]
async fn configured_run_branch_without_remote_is_not_reported_as_pushed() {
let repo_dir = tempfile::tempdir().unwrap();
let emitter = Arc::new(Emitter::new(test_run_id()));
let events = record_events(&emitter);
let services = test_services(
RunStoreHandle::local(seeded_run_store().await),
emitter,
Arc::new(MockSandbox::linux()),
Arc::new(RunMetadataRuntime::new()),
None,
);
let mut run_options = test_run_options(repo_dir.path());
run_options.git = Some(GitCheckpointOptions {
base_sha: None,
run_branch: Some("fabro/run/test".to_string()),
meta_branch: None,
});
let executed = test_executed(
Graph::new("test"),
Ok(Outcome::success()),
run_options,
5,
services,
);
let options = FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
preserve_sandbox: false,
stop_on_terminal: true,
last_git_sha: Some("final-sha".to_string()),
};
let concluded = conclude(executed, &options).await.unwrap();
let published = crate::pipeline::publish(concluded, &crate::pipeline::PublishOptions {
pr_config: None,
github_app: None,
origin_url: None,
model: "test-model".to_string(),
})
.await;
assert_eq!(published.publish_outcome, PublishOutcome::default());
assert!(published.publish_error.is_none());
let finalized = finalize(published, &options).await.unwrap();
assert!(finalized.outcome.is_ok());
assert_eq!(finalized.pushed_branch, None);
let events = events.lock().unwrap();
let names = events.iter().map(RunEvent::event_name).collect::<Vec<_>>();
assert_eq!(names, vec!["run.completed"]);
}
#[tokio::test]
async fn final_push_failure_becomes_terminal_publish_failure() {
let repo_dir = tempfile::tempdir().unwrap();
let sandbox = Arc::new(MockSandbox::linux());
let emitter = Arc::new(Emitter::new(test_run_id()));
let events = record_events(&emitter);
let services = test_services(
RunStoreHandle::local(seeded_run_store().await),
emitter,
sandbox,
Arc::new(RunMetadataRuntime::new()),
None,
);
let mut run_options = test_run_options(repo_dir.path());
run_options.git = Some(GitCheckpointOptions {
base_sha: None,
run_branch: Some("fabro/run/test".to_string()),
meta_branch: None,
});
let executed = test_executed(
Graph::new("test"),
Ok(Outcome::success()),
run_options,
5,
services,
);
let options = FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
preserve_sandbox: false,
stop_on_terminal: true,
last_git_sha: Some("final-sha".to_string()),
};
let concluded = conclude(executed, &options).await.unwrap();
let published = crate::pipeline::publish(concluded, &crate::pipeline::PublishOptions {
pr_config: None,
github_app: None,
origin_url: Some("https://github.com/owner/repo.git".to_string()),
model: "test-model".to_string(),
})
.await;
assert!(matches!(
&published.publish_error,
Some(Error::Stage {
stage: ErrorStage::Publish,
..
})
));
let finalized = finalize(published, &options).await.unwrap();
assert!(matches!(
finalized.outcome,
Err(Error::Stage {
stage: ErrorStage::Publish,
..
})
));
assert_eq!(
finalized
.conclusion
.failure
.as_ref()
.map(|failure| failure.reason),
Some(FailureReason::PublishFailed)
);
let events = events.lock().unwrap();
let names = events.iter().map(RunEvent::event_name).collect::<Vec<_>>();
assert_eq!(names, vec!["git.push", "run.failed"]);
match &events.last().unwrap().body {
EventBody::RunFailed(props) => {
assert_eq!(props.failure.reason, FailureReason::PublishFailed);
}
other => panic!("expected run.failed, got {other:?}"),
}
}
/// An empty diff means there is nothing to open a pull request for. The
/// branch still gets pushed and the run still succeeds.
#[tokio::test]
async fn empty_diff_pushes_branch_without_opening_pull_request() {
let repo_dir = tempfile::tempdir().unwrap();
init_git_repo(repo_dir.path());
let emitter = Arc::new(Emitter::new(test_run_id()));
let events = record_events(&emitter);
let services = test_services(
RunStoreHandle::local(seeded_run_store().await),
emitter,
Arc::new(fabro_agent::LocalSandbox::new(
repo_dir.path().to_path_buf(),
)),
Arc::new(RunMetadataRuntime::new()),
None,
);
let mut run_options = test_run_options(repo_dir.path());
run_options.base_branch = Some("main".to_string());
run_options.git = Some(GitCheckpointOptions {
base_sha: None,
run_branch: Some("fabro/run/test".to_string()),
meta_branch: None,
});
let executed = test_executed(
Graph::new("test"),
Ok(Outcome::success()),
run_options,
5,
services,
);
let options = FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
preserve_sandbox: false,
stop_on_terminal: true,
last_git_sha: Some("final-sha".to_string()),
};
let mut concluded = conclude(executed, &options).await.unwrap();
concluded.conclusion.diff.patch = None;
let published = crate::pipeline::publish(concluded, &crate::pipeline::PublishOptions {
pr_config: Some(fabro_types::settings::run::PullRequestSettings {
enabled: true,
draft: true,
auto_merge: false,
merge_strategy: fabro_types::settings::run::MergeStrategy::Squash,
}),
github_app: None,
origin_url: Some("https://github.com/owner/repo.git".to_string()),
model: "test-model".to_string(),
})
.await;
assert!(published.publish_error.is_none());
let finalized = finalize(published, &options).await.unwrap();
assert!(finalized.outcome.is_ok());
assert_eq!(finalized.pushed_branch.as_deref(), Some("fabro/run/test"));
assert_eq!(finalized.pr_url, None);
let events = events.lock().unwrap();
let names = events.iter().map(RunEvent::event_name).collect::<Vec<_>>();
assert_eq!(names, vec!["git.push", "run.completed"]);
}
/// `base_sha` is where the run started, not what it produced. Reporting it
/// as the final commit would both mis-state a durable field and make the
/// remote-head check reject a branch that was pushed correctly.
#[tokio::test]
async fn untracked_final_commit_does_not_fall_back_to_base_sha() {
let repo_dir = tempfile::tempdir().unwrap();
init_git_repo(repo_dir.path());
let emitter = Arc::new(Emitter::new(test_run_id()));
let services = test_services(
RunStoreHandle::local(seeded_run_store().await),
emitter,
Arc::new(fabro_agent::LocalSandbox::new(
repo_dir.path().to_path_buf(),
)),
Arc::new(RunMetadataRuntime::new()),
None,
);
let mut run_options = test_run_options(repo_dir.path());
run_options.base_branch = Some("main".to_string());
run_options.git = Some(GitCheckpointOptions {
base_sha: Some("base-sha".to_string()),
run_branch: Some("fabro/run/test".to_string()),
meta_branch: None,
});
let executed = test_executed(
Graph::new("test"),
Ok(Outcome::success()),
run_options,
5,
services,
);
let options = FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
preserve_sandbox: false,
stop_on_terminal: true,
last_git_sha: None,
};
let mut concluded = conclude(executed, &options).await.unwrap();
assert_eq!(concluded.conclusion.final_git_commit_sha, None);
// No pull request wanted, so publish still pushes the branch and the
// run succeeds without needing a commit SHA at all.
concluded.conclusion.diff.patch = None;
let published = crate::pipeline::publish(concluded, &crate::pipeline::PublishOptions {
pr_config: None,
github_app: None,
origin_url: Some("https://github.com/owner/repo.git".to_string()),
model: "test-model".to_string(),
})
.await;
let finalized = finalize(published, &options).await.unwrap();
assert!(finalized.outcome.is_ok());
assert_eq!(finalized.pushed_branch.as_deref(), Some("fabro/run/test"));
assert_eq!(finalized.conclusion.final_git_commit_sha, None);
}
#[tokio::test]
async fn pull_request_failure_precedes_terminal_publish_failure() {
let repo_dir = tempfile::tempdir().unwrap();
init_git_repo(repo_dir.path());
let emitter = Arc::new(Emitter::new(test_run_id()));
let events = record_events(&emitter);
let services = test_services(
RunStoreHandle::local(seeded_run_store().await),
emitter,
Arc::new(fabro_agent::LocalSandbox::new(
repo_dir.path().to_path_buf(),
)),
Arc::new(RunMetadataRuntime::new()),
None,
);
let mut run_options = test_run_options(repo_dir.path());
run_options.base_branch = Some("main".to_string());
run_options.git = Some(GitCheckpointOptions {
base_sha: None,
run_branch: Some("fabro/run/test".to_string()),
meta_branch: None,
});
let executed = test_executed(
Graph::new("test"),
Ok(Outcome::success()),
run_options,
5,
services,
);
let options = FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
preserve_sandbox: false,
stop_on_terminal: true,
last_git_sha: Some("final-sha".to_string()),
};
let mut concluded = conclude(executed, &options).await.unwrap();
concluded.conclusion.diff.patch =
Some("diff --git a/a b/a\n+published change\n".to_string());
let published = crate::pipeline::publish(concluded, &crate::pipeline::PublishOptions {
pr_config: Some(fabro_types::settings::run::PullRequestSettings {
enabled: true,
draft: true,
auto_merge: false,
merge_strategy: fabro_types::settings::run::MergeStrategy::Squash,
}),
github_app: None,
origin_url: Some("https://github.com/owner/repo.git".to_string()),
model: "test-model".to_string(),
})
.await;
let finalized = finalize(published, &options).await.unwrap();
assert!(matches!(
finalized.outcome,
Err(Error::Stage {
stage: ErrorStage::Publish,
..
})
));
// The push landed before the pull request failed, so the branch is
// still reported — that is exactly the run where the user needs it.
assert_eq!(finalized.pushed_branch.as_deref(), Some("fabro/run/test"));
let events = events.lock().unwrap();
let names = events.iter().map(RunEvent::event_name).collect::<Vec<_>>();
assert_eq!(names, vec!["git.push", "pull_request.failed", "run.failed"]);
match &events.last().unwrap().body {
EventBody::RunFailed(props) => {
assert_eq!(props.failure.reason, FailureReason::PublishFailed);
}
other => panic!("expected run.failed, got {other:?}"),
}
}
#[tokio::test]
async fn finalize_stops_sandbox_on_terminal_without_deleting() {
let repo_dir = tempfile::tempdir().unwrap();
@ -1292,7 +1691,7 @@ mod tests {
services,
);
finalize(executed, &FinalizeOptions {
finalize_executed(executed, &FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
@ -1326,7 +1725,7 @@ mod tests {
services,
);
finalize(executed, &FinalizeOptions {
finalize_executed(executed, &FinalizeOptions {
run_dir: repo_dir.path().to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),
@ -1379,7 +1778,7 @@ mod tests {
services,
);
finalize(executed, &FinalizeOptions {
finalize_executed(executed, &FinalizeOptions {
run_dir: repo.to_path_buf(),
run_id: test_run_id(),
workflow_name: "test".to_string(),

View file

@ -3,6 +3,7 @@ mod finalize;
mod initialize;
mod parse;
mod persist;
mod publish;
mod pull_request;
mod transform;
pub(crate) mod types;
@ -12,18 +13,19 @@ pub use execute::execute;
pub(crate) use finalize::build_conclusion_from_store;
#[cfg(any(test, feature = "test-support"))]
pub(crate) use finalize::{billing_from_projection, build_terminal_event};
pub use finalize::{classify_engine_result, finalize, write_finalize_commit};
pub use finalize::{classify_engine_result, conclude, finalize, write_finalize_commit};
pub use initialize::initialize;
pub use parse::parse;
pub(crate) use persist::persist;
pub use publish::publish;
pub use pull_request::{
AutoMergeOptions, CreatedPullRequest, OpenPullRequestRequest, PrContent, build_pr_content,
maybe_open_pull_request, pull_request,
open_pull_request,
};
pub use transform::transform;
pub use types::{
Concluded, Executed, FinalizeOptions, Finalized, InitOptions, Initialized, LlmSpec, Parsed,
Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, TEMPLATE_UNDEFINED_VARIABLE_RULE,
TransformOptions, Transformed, Validated,
Persisted, PublishOptions, PublishOutcome, Published, ResumeState, SandboxEnvSpec,
TEMPLATE_UNDEFINED_VARIABLE_RULE, TransformOptions, Transformed, Validated,
};
pub use validate::validate;

View file

@ -0,0 +1,192 @@
use std::sync::Arc;
use super::pull_request::{AutoMergeOptions, OpenPullRequestRequest, open_pull_request};
use super::types::{Concluded, PublishOptions, PublishOutcome, Published};
use crate::error::Error;
use crate::event::Event;
use crate::lifecycle::git::push_run_branch;
/// PUBLISH phase: push the final run commit and, when configured, open a pull
/// request.
///
/// Publish is always present in the pipeline. It becomes a no-op when the run
/// did not succeed, is a dry run, or has no remote branch configured.
pub async fn publish(concluded: Concluded, options: &PublishOptions) -> Published {
let mut publish_outcome = PublishOutcome::default();
let publish_error = concluded.publish(options, &mut publish_outcome).await.err();
let Concluded {
outcome,
conclusion,
artifact_count,
graph: _,
run_options,
services,
} = concluded;
Published {
execution_outcome: outcome,
publish_outcome,
publish_error,
conclusion,
artifact_count,
run_options,
services,
}
}
impl Concluded {
/// Run the publish steps, recording each one into `outcome` as it lands.
///
/// `outcome` accumulates what actually happened, so a branch that reached
/// the remote is still reported when pull request creation later fails.
async fn publish(
&self,
options: &PublishOptions,
outcome: &mut PublishOutcome,
) -> Result<(), Error> {
// A run that did not succeed, or that never intended to touch the
// remote, has nothing to publish — even when a pull request was asked
// for. Only a run that got far enough to publish can fail publishing.
if !self
.outcome
.as_ref()
.is_ok_and(|o| o.status.is_successful())
|| self.run_options.dry_run_enabled()
{
return Ok(());
}
let pull_request_requested = options.pr_config.is_some();
let (origin_url, run_branch) = match self.publish_target(options) {
Ok(target) => target,
Err(_) if !pull_request_requested => return Ok(()),
Err(reason) => return Err(self.pull_request_error(reason)),
};
self.push_final_commit(run_branch).await?;
outcome.pushed_branch = Some(run_branch.to_string());
let Some(pr_config) = options.pr_config.as_ref() else {
return Ok(());
};
let diff = self.conclusion.diff.patch.as_deref().unwrap_or_default();
if diff.trim().is_empty() {
return Ok(());
}
// Only pull request creation needs the SHA, to check that the remote
// branch really carries this run's work. Pushing does not: the refspec
// sends whatever the branch points at.
let final_sha = self
.conclusion
.final_git_commit_sha
.as_deref()
.ok_or_else(|| {
self.pull_request_error("pull request creation requires the run's final commit SHA")
})?;
let base_branch = self.run_options.base_branch.as_deref().ok_or_else(|| {
self.pull_request_error("pull request creation requires a base branch")
})?;
let credentials = options.github_app.as_ref().ok_or_else(|| {
self.pull_request_error("pull request creation requires GitHub credentials")
})?;
let github_base_url = fabro_github::github_api_base_url();
let created = open_pull_request(OpenPullRequestRequest {
github: fabro_github::GitHubContext::new(credentials, &github_base_url),
origin_url,
base_branch,
head_branch: run_branch,
expected_head_sha: final_sha,
goal: self.graph.goal(),
diff,
model: &options.model,
draft: pr_config.draft,
auto_merge: pr_config.auto_merge.then_some(AutoMergeOptions {
merge_strategy: pr_config.merge_strategy,
}),
run_store: &self.services.run_store,
llm_source: self.services.llm_source.as_ref(),
catalog: Arc::clone(&self.services.catalog),
conclusion: Some(&self.conclusion),
run_state: None,
})
.await
.map_err(|error| {
self.services.emitter.emit(&Event::PullRequestFailed {
error: error.clone(),
});
Error::publish_with_source("failed to create pull request", anyhow::anyhow!(error))
})?;
self.services.emitter.emit(&Event::pull_request_created(
&created.link,
&created.base_branch,
&created.head_branch,
final_sha,
&created.title,
pr_config.draft,
));
outcome.pr_url = Some(created.link.html_url());
Ok(())
}
/// The origin and run branch to publish to.
///
/// `Err` carries why there is no target. That is only a failure when a
/// pull request was requested; otherwise publish just has nothing to do.
fn publish_target<'a>(
&'a self,
options: &'a PublishOptions,
) -> Result<(&'a str, &'a str), &'static str> {
let origin_url = options
.origin_url
.as_deref()
.filter(|origin| !origin.trim().is_empty())
.ok_or("pull request creation requires a GitHub origin URL")?;
let run_branch = self
.run_options
.run_branch()
.ok_or("pull request creation requires a run branch")?;
if !self.run_options.settings.run.run_branch.push {
return Err("pull request creation requires run branch pushing");
}
Ok((origin_url, run_branch))
}
async fn push_final_commit(&self, run_branch: &str) -> Result<(), Error> {
match push_run_branch(self.services.sandbox.as_ref(), run_branch).await {
Ok(()) => {
self.services.emitter.emit(&Event::GitPush {
branch: run_branch.to_string(),
success: true,
exec_output_tail: None,
});
Ok(())
}
Err(error) => {
let exec_output_tail = fabro_sandbox::default_redacted_output_tail(&error);
self.services.emitter.emit(&Event::GitPush {
branch: run_branch.to_string(),
success: false,
exec_output_tail: exec_output_tail.clone(),
});
Err(Error::publish_with_source_and_exec_output_tail(
format!("failed to push run branch '{run_branch}'"),
error,
exec_output_tail,
))
}
}
}
fn pull_request_error(&self, message: &str) -> Error {
self.services.emitter.emit(&Event::PullRequestFailed {
error: message.to_string(),
});
Error::publish(message)
}
}

View file

@ -1,5 +1,6 @@
use std::collections::HashSet;
use std::sync::{Arc, LazyLock};
use std::time::Duration;
use fabro_auth::CredentialSource;
use fabro_github::{self as github_app, ssh_url_to_https};
@ -11,11 +12,10 @@ use fabro_store::RunProjection;
use fabro_types::PullRequestLink;
use fabro_types::settings::run::MergeStrategy;
use fabro_util::text::strip_goal_decoration;
use tokio::time::sleep;
use tracing::{debug, info, warn};
use super::types::{Concluded, Finalized, PullRequestOptions};
use crate::event::{Event, RunNoticeCode, RunNoticeLevel};
use crate::outcome::{StageOutcome, format_cost as outcome_format_cost};
use crate::outcome::format_cost as outcome_format_cost;
use crate::records::{Conclusion, RunSpec};
use crate::runtime_store::RunStoreHandle;
@ -327,22 +327,6 @@ fn assemble_pr_body(
parts.join("\n")
}
async fn load_pull_request_diff(run_store: &RunStoreHandle) -> String {
run_store
.state()
.await
.inspect_err(|err| {
tracing::warn!(error = %err, "Failed to load final patch from store for PR");
})
.ok()
.and_then(|state| {
state
.conclusion
.and_then(|conclusion| conclusion.diff.patch)
})
.unwrap_or_default()
}
/// Build complete PR content by combining LLM-generated narrative with
/// deterministic fallbacks and programmatic sections.
pub async fn build_pr_content(
@ -459,22 +443,25 @@ pub struct AutoMergeOptions {
pub merge_strategy: MergeStrategy,
}
/// Inputs for [`maybe_open_pull_request`].
/// Inputs for [`open_pull_request`].
pub struct OpenPullRequestRequest<'a> {
pub github: github_app::GitHubContext<'a>,
pub origin_url: &'a str,
pub base_branch: &'a str,
pub head_branch: &'a str,
pub goal: &'a str,
pub diff: &'a str,
pub model: &'a str,
pub draft: bool,
pub auto_merge: Option<AutoMergeOptions>,
pub run_store: &'a RunStoreHandle,
pub llm_source: &'a dyn CredentialSource,
pub catalog: Arc<Catalog>,
pub conclusion: Option<&'a Conclusion>,
pub run_state: Option<&'a RunProjection>,
pub github: github_app::GitHubContext<'a>,
pub origin_url: &'a str,
pub base_branch: &'a str,
pub head_branch: &'a str,
/// Commit that must be visible at the remote branch before the PR is
/// opened.
pub expected_head_sha: &'a str,
pub goal: &'a str,
pub diff: &'a str,
pub model: &'a str,
pub draft: bool,
pub auto_merge: Option<AutoMergeOptions>,
pub run_store: &'a RunStoreHandle,
pub llm_source: &'a dyn CredentialSource,
pub catalog: Arc<Catalog>,
pub conclusion: Option<&'a Conclusion>,
pub run_state: Option<&'a RunProjection>,
}
#[derive(Debug, Clone, PartialEq, Eq)]
@ -485,22 +472,69 @@ pub struct CreatedPullRequest {
pub head_branch: String,
}
/// Optionally open a pull request after a successful workflow run.
/// How many times to read the remote branch head before giving up.
///
/// Returns `Ok(Some(CreatedPullRequest))` if a PR was created, `Ok(None)` if
/// the diff was empty, or `Err` on failure.
pub async fn maybe_open_pull_request(
req: OpenPullRequestRequest<'_>,
) -> Result<Option<CreatedPullRequest>, String> {
if req.diff.is_empty() {
debug!("Empty diff, skipping pull request creation");
return Ok(None);
/// `GET /repos/{owner}/{repo}/branches/{branch}` is replica-served, so shortly
/// after the push that publish just made it can still report the previous
/// commit — or 404 for a branch that is new on the remote.
const BRANCH_HEAD_ATTEMPTS: u32 = 3;
const BRANCH_HEAD_RETRY_DELAY: Duration = Duration::from_millis(500);
/// Confirm the remote branch points at the run's final commit.
///
/// Publish failures are terminal, so a replica that has not caught up yet must
/// not be mistaken for a genuinely stale branch.
async fn verify_remote_head(
req: &OpenPullRequestRequest<'_>,
owner: &str,
repo: &str,
) -> Result<(), String> {
let mut last_seen = Ok(None);
for attempt in 1..=BRANCH_HEAD_ATTEMPTS {
last_seen = github_app::branch_head_sha(&req.github, owner, repo, req.head_branch).await;
match &last_seen {
Ok(Some(head)) if head == req.expected_head_sha => return Ok(()),
Ok(head) => debug!(
attempt,
head = ?head,
expected = req.expected_head_sha,
"Remote branch head does not match the final commit yet"
),
Err(err) => debug!(attempt, error = %err, "Failed to read remote branch head"),
}
if attempt < BRANCH_HEAD_ATTEMPTS {
sleep(BRANCH_HEAD_RETRY_DELAY).await;
}
}
Err(match last_seen {
Ok(Some(head)) => format!(
"remote branch '{}' points to commit {head}, expected final commit {}",
req.head_branch, req.expected_head_sha
),
Ok(None) => format!(
"remote branch '{}' does not exist; expected final commit {}",
req.head_branch, req.expected_head_sha
),
Err(err) => format!("failed to verify remote branch head: {err:#}"),
})
}
/// Open a pull request for a completed run.
///
/// Callers are responsible for skipping runs with an empty diff; reaching here
/// means a pull request is expected, so every failure is an error.
pub async fn open_pull_request(
req: OpenPullRequestRequest<'_>,
) -> Result<CreatedPullRequest, String> {
let https_url = ssh_url_to_https(req.origin_url);
let (owner, repo) =
github_app::parse_github_owner_repo(&https_url).map_err(|err| format!("{err:#}"))?;
// Verify before generating content: this is the cheap check, and a stale
// branch would otherwise cost a full LLM call before failing.
verify_remote_head(&req, &owner, &repo).await?;
let content = build_pr_content(
req.diff,
req.goal,
@ -560,114 +594,12 @@ pub async fn maybe_open_pull_request(
number: created.number,
};
Ok(Some(CreatedPullRequest {
Ok(CreatedPullRequest {
link,
title,
base_branch: req.base_branch.to_string(),
head_branch: req.head_branch.to_string(),
}))
}
/// PULL_REQUEST phase: optionally create a pull request after finalize.
///
/// This stage is infallible: failures are emitted and logged, but the pipeline
/// completes.
pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> Finalized {
let Concluded {
outcome,
conclusion,
graph,
run_options,
services,
} = concluded;
let mut pr_url = None;
if let Some(pr_cfg) = &options.pr_config {
if run_options.dry_run_enabled() {
tracing::debug!("Skipping PR creation: run is in dry-run mode");
} else if let Err(ref e) = outcome {
tracing::debug!(error = %e, "Skipping PR creation: engine returned an error");
} else if let Ok(ref result) = outcome {
if matches!(
result.status,
StageOutcome::Succeeded | StageOutcome::PartiallySucceeded
) {
let diff = load_pull_request_diff(&services.run_store).await;
if let (Some(base_branch), Some(run_branch), Some(creds), Some(origin)) = (
&run_options.base_branch,
run_options.run_branch(),
&options.github_app,
&options.origin_url,
) {
let auto_merge = if pr_cfg.auto_merge {
Some(AutoMergeOptions {
merge_strategy: pr_cfg.merge_strategy,
})
} else {
None
};
match maybe_open_pull_request(OpenPullRequestRequest {
github: github_app::GitHubContext::new(
creds,
&github_app::github_api_base_url(),
),
origin_url: origin,
base_branch,
head_branch: run_branch,
goal: graph.goal(),
diff: &diff,
model: &options.model,
draft: pr_cfg.draft,
auto_merge,
run_store: &services.run_store,
llm_source: services.llm_source.as_ref(),
catalog: Arc::clone(&services.catalog),
conclusion: Some(&conclusion),
run_state: None,
})
.await
{
Ok(Some(created)) => {
services.emitter.emit(&Event::pull_request_created(
&created.link,
&created.base_branch,
&created.head_branch,
&created.title,
pr_cfg.draft,
));
pr_url = Some(created.link.html_url());
}
Ok(None) => {}
Err(e) => {
services
.emitter
.emit(&Event::PullRequestFailed { error: e.clone() });
services.emitter.notice(
RunNoticeLevel::Warn,
RunNoticeCode::PullRequestFailed,
format!("PR creation failed: {e}"),
);
}
}
}
}
}
}
Finalized {
run_id: run_options.run_id,
outcome,
conclusion,
pushed_branch: run_options
.settings
.run
.run_branch
.push
.then(|| run_options.run_branch().map(str::to_string))
.flatten(),
pr_url,
}
})
}
#[cfg(test)]
@ -677,7 +609,7 @@ mod tests {
use std::time::Duration;
use chrono::Utc;
use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource};
use fabro_auth::{CredentialSource, VaultCredentialSource};
use fabro_graphviz::graph::Graph;
use fabro_llm::Error as LlmError;
use fabro_llm::client::Client;
@ -691,18 +623,14 @@ mod tests {
};
use fabro_vault::{SecretType, Vault};
use futures::stream;
use httpmock::Method::POST;
use httpmock::Method::{GET, POST};
use httpmock::MockServer;
use object_store::memory::InMemory;
use tokio::sync::RwLock as AsyncRwLock;
use tokio_util::sync::CancellationToken;
use super::*;
use crate::event::{Event, append_event};
use crate::outcome::Outcome;
use crate::records::StageSummary;
use crate::run_options::{GitCheckpointOptions, RunOptions};
use crate::services::EngineServices;
struct MockProvider {
name: String,
@ -787,10 +715,6 @@ mod tests {
))
}
fn test_catalog() -> Arc<Catalog> {
Arc::new(Catalog::from_builtin().expect("default catalog should build"))
}
fn test_catalog_with_provider_base_url(provider: &str, base_url: &str) -> Arc<Catalog> {
let mut settings = LlmCatalogSettings::default();
settings
@ -818,10 +742,6 @@ mod tests {
))
}
fn test_llm_source() -> Arc<dyn CredentialSource> {
Arc::new(EnvCredentialSource::new())
}
fn test_projection() -> RunProjection {
RunProjection::new(
"Test run".to_string(),
@ -917,48 +837,6 @@ mod tests {
}
}
#[tokio::test]
async fn pull_request_omits_pushed_branch_when_run_branch_push_disabled() {
let temp = tempfile::tempdir().unwrap();
let mut settings = WorkflowSettings::default();
settings.run.run_branch.push = false;
let run_options = RunOptions {
settings,
run_dir: temp.path().to_path_buf(),
cancel_token: CancellationToken::new(),
run_id: fixtures::RUN_1,
labels: HashMap::new(),
workflow_slug: None,
github_app: None,
pre_run_git: None,
fork_source_ref: None,
base_branch: None,
display_base_sha: None,
git: Some(GitCheckpointOptions {
base_sha: None,
run_branch: Some("fabro/run/test".to_string()),
meta_branch: None,
}),
};
let concluded = Concluded {
outcome: Ok(Outcome::success()),
conclusion: make_test_conclusion(),
graph: Graph::new("test"),
run_options,
services: EngineServices::test_default().run,
};
let finalized = pull_request(concluded, &PullRequestOptions {
pr_config: None,
github_app: None,
origin_url: None,
model: "test-model".to_string(),
})
.await;
assert_eq!(finalized.pushed_branch, None);
}
// ── format_arc_details_section tests ────────────────────────────────
#[test]
@ -1543,112 +1421,43 @@ mod tests {
}
#[tokio::test]
async fn empty_diff_returns_none() {
let store = test_store();
let run_store = store.create_run(&fixtures::RUN_1).await.unwrap();
let run_store_handle: RunStoreHandle = run_store.into();
let llm_source = test_llm_source();
let creds = fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials {
app_id: "123".to_string(),
private_key_pem: "unused".to_string(),
slug: None,
});
let base_url = github_app::github_api_base_url();
let result = maybe_open_pull_request(OpenPullRequestRequest {
github: github_app::GitHubContext::new(&creds, &base_url),
origin_url: "https://github.com/owner/repo.git",
base_branch: "main",
head_branch: "fabro/run/123",
goal: "Fix bug",
diff: "",
model: "claude-sonnet-4-20250514",
draft: false,
auto_merge: None,
run_store: &run_store_handle,
llm_source: llm_source.as_ref(),
catalog: test_catalog(),
conclusion: None,
run_state: None,
})
.await;
assert!(result.is_ok());
assert!(result.unwrap().is_none());
}
#[tokio::test]
async fn load_pull_request_diff_uses_store_without_disk_patch() {
let tmp = tempfile::tempdir().unwrap();
let store = test_store();
let run_store = store.create_run(&fixtures::RUN_1).await.unwrap();
let run_spec = RunSpec {
run_id: fixtures::RUN_1,
settings: fabro_types::WorkflowSettings::default(),
graph: Graph::new("test"),
graph_source: None,
workflow_slug: None,
automation: None,
source_directory: Some(tmp.path().display().to_string()),
git: None,
labels: std::collections::HashMap::new(),
provenance: test_support::test_run_provenance(),
manifest_blob: None,
definition_blob: None,
fork_source_ref: None,
};
append_event(&run_store, &fixtures::RUN_1, &Event::RunCreated {
run_id: fixtures::RUN_1,
title: None,
settings: serde_json::to_value(&run_spec.settings).unwrap(),
graph: serde_json::to_value(&run_spec.graph).unwrap(),
workflow_source: None,
workflow_config: None,
labels: run_spec.labels.clone().into_iter().collect(),
run_dir: tmp.path().display().to_string(),
source_directory: run_spec.source_directory.clone(),
workflow_slug: None,
automation: None,
db_prefix: None,
provenance: run_spec.provenance.clone(),
manifest_blob: None,
git: None,
fork_source_ref: None,
retried_from: None,
parent_id: None,
web_url: None,
async fn stale_remote_branch_is_rejected_before_pull_request_creation() {
let payload = pr_content_json("Fix bug", "Narrative.");
let harness = setup_fallback_test_harness_with_branch_sha(&payload, "stale-sha").await;
let github_base_url = harness.github_server.url("");
let error = open_pull_request(OpenPullRequestRequest {
github: fabro_github::GitHubContext::new(&harness.creds, &github_base_url),
origin_url: "https://github.com/owner/repo.git",
base_branch: "main",
head_branch: "fabro/run/123",
expected_head_sha: "final-sha",
goal: "Fix bug",
diff: "diff --git a/src/lib.rs b/src/lib.rs\n+fn x() {}\n",
model: "claude-sonnet-4-20250514",
draft: false,
auto_merge: None,
run_store: &harness.run_store,
llm_source: harness.llm_source.as_ref(),
catalog: harness.catalog.clone(),
conclusion: None,
run_state: None,
})
.await
.unwrap();
append_event(&run_store, &fixtures::RUN_1, &Event::RunRunnable {
source: fabro_types::RunRunnableSource::StartRequested,
actor: None,
})
.await
.unwrap();
append_event(&run_store, &fixtures::RUN_1, &Event::RunStarting)
.await
.unwrap();
append_event(&run_store, &fixtures::RUN_1, &Event::RunRunning)
.await
.unwrap();
append_event(&run_store, &fixtures::RUN_1, &Event::WorkflowRunCompleted {
timing: fabro_types::RunTiming::wall_only(1),
artifact_count: 0,
status: "succeeded".to_string(),
reason: SuccessReason::Completed,
total_usd_micros: None,
final_git_commit_sha: None,
final_patch: Some(
"diff --git a/src/lib.rs b/src/lib.rs\n+fn from_store() {}\n".to_string(),
),
diff_summary: None,
billing: None,
})
.await
.unwrap();
.expect_err("stale remote branch must prevent PR creation");
let diff = load_pull_request_diff(&run_store.clone().into()).await;
assert!(diff.contains("from_store"));
assert!(error.contains("stale-sha"));
assert!(error.contains("final-sha"));
// The branch is re-read to ride out replica lag...
httpmock::Mock::new(harness.branch_mock_id, &harness.github_server)
.assert_calls_async(BRANCH_HEAD_ATTEMPTS as usize)
.await;
// ...but the check runs first, so no LLM call and no PR creation.
httpmock::Mock::new(harness.openai_mock_id, &harness.openai_server)
.assert_calls_async(0)
.await;
httpmock::Mock::new(harness.github_mock_id, &harness.github_server)
.assert_calls_async(0)
.await;
}
// ── Structured-output PR content tests ──────────────────────────────
@ -1793,9 +1602,9 @@ mod tests {
assert!(body.contains("Generated with [Fabro](https://fabro.sh)"));
}
// ── maybe_open_pull_request fallback tests ──────────────────────────
// ── open_pull_request fallback tests ──────────────────────────
/// Set of mock servers and credentials for the `maybe_open_pull_request`
/// Set of mock servers and credentials for the `open_pull_request`
/// fallback path. The builder's `Client::from_source` rebuilds the LLM
/// client from the credential source, so the in-process MockProvider
/// cannot intercept — we mock the OpenAI HTTP endpoint instead.
@ -1807,6 +1616,7 @@ mod tests {
openai_server: MockServer,
github_server: MockServer,
openai_mock_id: usize,
branch_mock_id: usize,
github_mock_id: usize,
llm_source: Arc<dyn CredentialSource>,
catalog: Arc<Catalog>,
@ -1819,6 +1629,9 @@ mod tests {
httpmock::Mock::new(self.openai_mock_id, &self.openai_server)
.assert_async()
.await;
httpmock::Mock::new(self.branch_mock_id, &self.github_server)
.assert_async()
.await;
httpmock::Mock::new(self.github_mock_id, &self.github_server)
.assert_async()
.await;
@ -1830,6 +1643,13 @@ mod tests {
/// credential source, and a run store seeded with a non-empty
/// `final_patch`.
async fn setup_fallback_test_harness(openai_payload_text: &str) -> FallbackHarness {
setup_fallback_test_harness_with_branch_sha(openai_payload_text, "final-sha").await
}
async fn setup_fallback_test_harness_with_branch_sha(
openai_payload_text: &str,
branch_sha: &str,
) -> FallbackHarness {
let openai_server = MockServer::start_async().await;
let openai_mock = openai_server
.mock_async(|when, then| {
@ -1843,6 +1663,19 @@ mod tests {
.await;
let github_server = MockServer::start_async().await;
let branch_sha = branch_sha.to_string();
let branch_mock = github_server
.mock_async(move |when, then| {
when.method(GET)
.path("/repos/owner/repo/branches/fabro/run/123")
.header("authorization", "Bearer test-token");
then.status(200)
.header("content-type", "application/json")
.json_body(serde_json::json!({
"commit": { "sha": branch_sha }
}));
})
.await;
let github_mock = github_server
.mock_async(|when, then| {
when.method(POST)
@ -1878,8 +1711,7 @@ mod tests {
let store = test_store();
let run_store = store.create_run(&fixtures::RUN_1).await.unwrap();
// Seed a non-empty `final_patch` so `load_pull_request_diff` returns
// diff content and the early-return for empty diffs does not fire.
// Seed a completed run so the PR body can include run details.
let run_spec = RunSpec {
run_id: fixtures::RUN_1,
settings: fabro_types::WorkflowSettings::default(),
@ -1947,6 +1779,7 @@ mod tests {
.unwrap();
let openai_mock_id = openai_mock.id;
let branch_mock_id = branch_mock.id;
let github_mock_id = github_mock.id;
FallbackHarness {
@ -1954,6 +1787,7 @@ mod tests {
openai_server,
github_server,
openai_mock_id,
branch_mock_id,
github_mock_id,
llm_source,
catalog,
@ -1966,18 +1800,19 @@ mod tests {
/// falls back to `pr_title_from_goal` (first line, decoration stripped)
/// and PR creation succeeds with that title.
#[tokio::test]
async fn maybe_open_pull_request_falls_back_to_goal_title_when_llm_returns_empty_title() {
async fn open_pull_request_falls_back_to_goal_title_when_llm_returns_empty_title() {
let payload = pr_content_json("", "Narrative.");
let harness = setup_fallback_test_harness(&payload).await;
let github_base_url = harness.github_server.url("");
let github = github_app::GitHubContext::new(&harness.creds, &github_base_url);
let result = maybe_open_pull_request(OpenPullRequestRequest {
let result = open_pull_request(OpenPullRequestRequest {
github,
origin_url: "https://github.com/owner/repo.git",
base_branch: "main",
head_branch: "fabro/run/123",
expected_head_sha: "final-sha",
goal: "Fix telemetry leak\n\ndetails...",
diff: "diff --git a/src/lib.rs b/src/lib.rs\n+fn x() {}\n",
model: "gpt-5.4",
@ -1992,15 +1827,14 @@ mod tests {
.await
.expect("PR creation should succeed");
let record = result.expect("PR record should be Some");
assert_eq!(record.title, "Fix telemetry leak");
assert_eq!(result.title, "Fix telemetry leak");
harness.assert_mocks_called_once().await;
}
/// LLM returns an empty title; the content builder fallback still caps
/// the deterministic goal title at 72 chars ending with `…`.
#[tokio::test]
async fn maybe_open_pull_request_caps_fallback_title_at_72_chars() {
async fn open_pull_request_caps_fallback_title_at_72_chars() {
let payload = pr_content_json("", "Narrative.");
let harness = setup_fallback_test_harness(&payload).await;
@ -2010,11 +1844,12 @@ mod tests {
// Single ~200-char line, no `Plan:` / heading prefix, no newlines.
let goal = "x".repeat(200);
let result = maybe_open_pull_request(OpenPullRequestRequest {
let result = open_pull_request(OpenPullRequestRequest {
github,
origin_url: "https://github.com/owner/repo.git",
base_branch: "main",
head_branch: "fabro/run/123",
expected_head_sha: "final-sha",
goal: &goal,
diff: "diff --git a/src/lib.rs b/src/lib.rs\n+fn x() {}\n",
model: "gpt-5.4",
@ -2029,8 +1864,7 @@ mod tests {
.await
.expect("PR creation should succeed");
let record = result.expect("PR record should be Some");
let title = record.title;
let title = result.title;
assert_eq!(title.chars().count(), 72);
assert!(title.ends_with('\u{2026}'));
harness.assert_mocks_called_once().await;

View file

@ -337,17 +337,41 @@ pub struct Executed {
pub model: String,
}
/// Output of the FINALIZE phase.
/// Output of the CONCLUDE phase.
#[non_exhaustive]
pub struct Concluded {
pub outcome: Result<Outcome, Error>,
pub conclusion: Conclusion,
pub graph: Graph,
pub run_options: RunOptions,
pub services: Arc<RunServices>,
pub outcome: Result<Outcome, Error>,
pub conclusion: Conclusion,
pub artifact_count: usize,
pub graph: Graph,
pub run_options: RunOptions,
pub services: Arc<RunServices>,
}
/// Output of the PULL_REQUEST phase.
/// What the PUBLISH phase actually accomplished.
///
/// Recorded separately from the phase's error so a branch that reached the
/// remote is still reported when a later step, such as pull request creation,
/// fails. An all-`None` value means publish had nothing to do.
#[derive(Debug, Clone, Default, PartialEq, Eq)]
pub struct PublishOutcome {
pub pushed_branch: Option<String>,
pub pr_url: Option<String>,
}
/// Output of the PUBLISH phase.
#[non_exhaustive]
pub struct Published {
pub execution_outcome: Result<Outcome, Error>,
pub publish_outcome: PublishOutcome,
pub publish_error: Option<Error>,
pub conclusion: Conclusion,
pub artifact_count: usize,
pub run_options: RunOptions,
pub services: Arc<RunServices>,
}
/// Output of the FINALIZE phase.
#[non_exhaustive]
pub struct Finalized {
pub run_id: RunId,
@ -380,8 +404,8 @@ pub struct FinalizeOptions {
pub last_git_sha: Option<String>,
}
/// Options for the PULL_REQUEST phase.
pub struct PullRequestOptions {
/// Options for the PUBLISH phase.
pub struct PublishOptions {
pub pr_config: Option<PullRequestSettings>,
pub github_app: Option<fabro_github::GitHubCredentials>,
pub origin_url: Option<String>,

View file

@ -1,4 +1,4 @@
pub use crate::pipeline::{
AutoMergeOptions, CreatedPullRequest, OpenPullRequestRequest, PrContent, build_pr_content,
maybe_open_pull_request,
open_pull_request,
};

View file

@ -113,6 +113,7 @@ fn success_reason_json_tokens_match_openapi() {
#[test]
fn failure_reason_json_tokens_match_openapi() {
assert_string_json(FailureReason::WorkflowError, "workflow_error");
assert_string_json(FailureReason::PublishFailed, "publish_failed");
assert_string_json(FailureReason::Cancelled, "cancelled");
assert_string_json(FailureReason::ApprovalDenied, "approval_denied");
assert_string_json(FailureReason::Terminated, "terminated");

View file

@ -251,6 +251,8 @@ pub struct PullRequestCreatedProps {
pub repo: String,
pub base_branch: String,
pub head_branch: String,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub head_sha: Option<String>,
pub title: String,
pub draft: bool,
}

View file

@ -290,6 +290,7 @@ pub enum SuccessReason {
#[strum(serialize_all = "snake_case")]
pub enum FailureReason {
WorkflowError,
PublishFailed,
Cancelled,
ApprovalDenied,
Terminated,

View file

@ -20,6 +20,7 @@
export const FailureReason = {
WORKFLOW_ERROR: 'workflow_error',
PUBLISH_FAILED: 'publish_failed',
CANCELLED: 'cancelled',
APPROVAL_DENIED: 'approval_denied',
TERMINATED: 'terminated',

View file

@ -172,6 +172,51 @@ mod tests {
server.shutdown().await;
}
/// Run branches contain slashes (`fabro/run/<id>`). GitHub routes the
/// branch as the rest of the path, so the twin must too.
#[tokio::test]
async fn branch_with_slashes_returns_200() {
let pem = test_rsa_private_key();
let mut state = AppState::new();
state.register_app(AppOptions {
app_id: "100".to_string(),
slug: "test-app".to_string(),
owner_login: "owner".to_string(),
public: true,
private_key_pem: pem.to_string(),
webhook_secret: None,
});
state.add_installation("100", "owner", vec!["repo".to_string()], false);
state.add_repository(
"owner",
"repo",
vec!["main".to_string(), "fabro/run/123".to_string()],
false,
);
let server = TestServer::start(state).await;
let jwt = sign_test_jwt("100", pem);
let client = test_http_client();
let token = get_installation_token(&client, &jwt, "owner", "repo", server.url()).await;
let resp = client
.get(format!(
"{}/repos/owner/repo/branches/fabro/run/123",
server.url()
))
.header("Authorization", format!("Bearer {token}"))
.header("Accept", "application/vnd.github+json")
.send()
.await
.unwrap();
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["name"], "fabro/run/123");
server.shutdown().await;
}
#[tokio::test]
async fn branch_not_found_returns_404() {
let pem = test_rsa_private_key();

View file

@ -30,8 +30,10 @@ pub fn build_router(state: SharedState) -> Router {
post(installations::create_access_token),
)
// Branch endpoints
// Wildcard: branch names contain slashes (`fabro/run/<id>`), and
// GitHub routes the branch as the remainder of the path.
.route(
"/repos/{owner}/{repo}/branches/{branch}",
"/repos/{owner}/{repo}/branches/{*branch}",
get(branches::get_branch),
)
// Pull request endpoints