From b99895e873fcc576db5660df45aabe6a503ff115 Mon Sep 17 00:00:00 2001 From: "fabro-releases[bot]" Date: Wed, 13 May 2026 10:06:39 +0000 Subject: [PATCH 1/8] Bump version to 0.232.0-nightly.0 --- Cargo.lock | 92 +++++++++++++++++++++++++++--------------------------- Cargo.toml | 2 +- 2 files changed, 47 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 54c39c172..850319cd3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1580,7 +1580,7 @@ dependencies = [ [[package]] name = "fabro-acp" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "agent-client-protocol", "agent-client-protocol-tokio", @@ -1602,7 +1602,7 @@ dependencies = [ [[package]] name = "fabro-agent" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -1641,7 +1641,7 @@ dependencies = [ [[package]] name = "fabro-api" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "chrono", "fabro-config", @@ -1662,7 +1662,7 @@ dependencies = [ [[package]] name = "fabro-auth" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -1686,11 +1686,11 @@ dependencies = [ [[package]] name = "fabro-build-support" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" [[package]] name = "fabro-checkpoint" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "chrono", "fabro-config", @@ -1706,7 +1706,7 @@ dependencies = [ [[package]] name = "fabro-cli" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -1807,7 +1807,7 @@ dependencies = [ [[package]] name = "fabro-client" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -1836,7 +1836,7 @@ dependencies = [ [[package]] name = "fabro-config" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -1864,7 +1864,7 @@ dependencies = [ [[package]] name = "fabro-core" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "async-trait", "fabro-types", @@ -1879,7 +1879,7 @@ dependencies = [ [[package]] name = "fabro-dev" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -1898,7 +1898,7 @@ dependencies = [ [[package]] name = "fabro-devcontainer" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-http", "fabro-static", @@ -1915,7 +1915,7 @@ dependencies = [ [[package]] name = "fabro-dump" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -1929,7 +1929,7 @@ dependencies = [ [[package]] name = "fabro-github" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -1951,7 +1951,7 @@ dependencies = [ [[package]] name = "fabro-graphviz" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-types", @@ -1965,7 +1965,7 @@ dependencies = [ [[package]] name = "fabro-hooks" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "async-trait", "fabro-agent", @@ -1989,7 +1989,7 @@ dependencies = [ [[package]] name = "fabro-http" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-static", "http", @@ -1999,7 +1999,7 @@ dependencies = [ [[package]] name = "fabro-install" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -2014,7 +2014,7 @@ dependencies = [ [[package]] name = "fabro-interview" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "async-trait", "dialoguer", @@ -2029,7 +2029,7 @@ dependencies = [ [[package]] name = "fabro-llm" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2061,7 +2061,7 @@ dependencies = [ [[package]] name = "fabro-macros" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "clap", "fabro-options-metadata", @@ -2072,7 +2072,7 @@ dependencies = [ [[package]] name = "fabro-manifest" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-api", @@ -2090,7 +2090,7 @@ dependencies = [ [[package]] name = "fabro-mcp" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-config", @@ -2106,7 +2106,7 @@ dependencies = [ [[package]] name = "fabro-mcp-server" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2128,7 +2128,7 @@ dependencies = [ [[package]] name = "fabro-model" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-static", "insta", @@ -2142,7 +2142,7 @@ dependencies = [ [[package]] name = "fabro-oauth" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "axum", @@ -2164,7 +2164,7 @@ dependencies = [ [[package]] name = "fabro-options-metadata" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "serde", "serde_json", @@ -2172,7 +2172,7 @@ dependencies = [ [[package]] name = "fabro-proc" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "cc", "libc", @@ -2181,7 +2181,7 @@ dependencies = [ [[package]] name = "fabro-redact" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "aho-corasick", "ref-cast", @@ -2197,7 +2197,7 @@ dependencies = [ [[package]] name = "fabro-sandbox" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2240,7 +2240,7 @@ dependencies = [ [[package]] name = "fabro-server" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2322,7 +2322,7 @@ dependencies = [ [[package]] name = "fabro-slack" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-http", "fabro-interview", @@ -2343,18 +2343,18 @@ dependencies = [ [[package]] name = "fabro-spa" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "rust-embed", ] [[package]] name = "fabro-static" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" [[package]] name = "fabro-store" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "async-trait", "bytes", @@ -2381,7 +2381,7 @@ dependencies = [ [[package]] name = "fabro-telemetry" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -2407,7 +2407,7 @@ dependencies = [ [[package]] name = "fabro-template" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-util", @@ -2419,7 +2419,7 @@ dependencies = [ [[package]] name = "fabro-test" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "assert_cmd", "axum", @@ -2442,7 +2442,7 @@ dependencies = [ [[package]] name = "fabro-tracker" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2456,7 +2456,7 @@ dependencies = [ [[package]] name = "fabro-types" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "chrono", "clap", @@ -2477,7 +2477,7 @@ dependencies = [ [[package]] name = "fabro-util" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "console 0.15.11", @@ -2497,7 +2497,7 @@ dependencies = [ [[package]] name = "fabro-validate" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-graphviz", "fabro-model", @@ -2508,7 +2508,7 @@ dependencies = [ [[package]] name = "fabro-vault" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "chrono", "fabro-types", @@ -2520,7 +2520,7 @@ dependencies = [ [[package]] name = "fabro-workflow" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -7298,7 +7298,7 @@ dependencies = [ [[package]] name = "twin-github" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "axum", "base64", @@ -7317,7 +7317,7 @@ dependencies = [ [[package]] name = "twin-openai" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-stream", diff --git a/Cargo.toml b/Cargo.toml index 989b8080a..f01038d80 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ resolver = "2" [workspace.package] edition = "2021" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" license = "MIT" [workspace.dependencies] From 6480c333ff87566443acb8f02c15443ccb0f911c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 13 May 2026 07:27:26 -0400 Subject: [PATCH 2/8] docs: accept outside pull requests Switch the contribution policy from an issue-only model to welcoming outside PRs. Small fixes go straight to a PR; larger changes start with an issue or discussion. Co-Authored-By: Claude Opus 4.7 (1M context) --- CONTRIBUTING.md | 12 +++++------- README.md | 18 +++++------------- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7228fbed8..9ac7fcd02 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,17 +4,15 @@ Thanks for your interest in contributing to Fabro! ## How to contribute -Fabro uses an **issue-based contribution model**. Instead of accepting outside pull requests, we accept bug reports and feature requests as GitHub Issues. +Outside contributions are welcome! Whether it's a bug fix, a new feature, documentation, or a typo -- we'd love your help making Fabro better. -1. **Open an issue** -- File an issue on [GitHub Issues](https://github.com/fabro-sh/fabro/issues) with a bug report or feature request. The more detail your issue contains, the easier it will be for us to address it quickly and successfully. -2. **We build it** -- A Fabro maintainer will follow our software development process to create a patch, supervising AI coding agents and workflows. -3. **You get credit** -- We will include you as a co-author on the commit which lands the change. - -See the [README](README.md#contributing-to-fabro) for more on why we use this model. +- **Bug fixes and small improvements** -- Send a pull request directly. No need to open an issue first. +- **Larger features or changes** -- Please open a [GitHub Issue](https://github.com/fabro-sh/fabro/issues) or start a [Discussion](https://github.com/fabro-sh/fabro/discussions) first so we can align on the approach before you invest significant time. +- **Questions** -- Open a Discussion or email [bryan@qlty.sh](mailto:bryan@qlty.sh). ## Development setup -If you are maintaining a fork, the instructions below will help you build and test locally. +The instructions below will help you build and test Fabro locally. ### Prerequisites diff --git a/README.md b/README.md index 7cc53e30a..73a1e4ffc 100644 --- a/README.md +++ b/README.md @@ -161,21 +161,13 @@ See the [deployment overview](https://docs.fabro.sh/administration/deployment) f ## Contributing to Fabro -Fabro uses an **issue-based contribution model**. Instead of accepting outside pull requests, we accept bug reports and feature requests as GitHub Issues. +Outside contributions are welcome! Whether it's a bug fix, a new feature, documentation, or a typo -- we'd love your help making Fabro better. -AI can rapidly write or edit large amounts of plausible-looking code. Accepting these patches from external sources opens up risks to security and quality. To mitigate these risks, we are tightly controlling the inputs into the software development process. +- **Bug fixes and small improvements** -- Send a pull request directly. +- **Larger features or changes** -- Open a [GitHub Issue](https://github.com/fabro-sh/fabro/issues) or start a [Discussion](https://github.com/fabro-sh/fabro/discussions) first so we can align on the approach. +- **Questions** -- Open a Discussion or email [bryan@qlty.sh](mailto:bryan@qlty.sh). -Contributions follow these steps: - -1. **Open an issue** -- File an issue with a bug report or feature request. The more detail your issue contains, the easier it will be for us to address it quickly and successfully. - -2. **We build it** -- A Fabro maintainer will follow our software development process to create a patch, supervising AI coding agents and workflows. - -3. **You get credit** -- We will include you as a co-author on the commit which lands the change. - -As a result, you get the feature you need, without needing to keep a fork in sync. - -If you need a capability which is not in-scope for Fabro, you always have the option to maintain a fork of Fabro as it is distributed under the MIT license. +See [CONTRIBUTING.md](CONTRIBUTING.md) for build instructions and development workflow. --- From 69ce83be4335a1bc2a0e12aec5b3a1f8be6d2897 Mon Sep 17 00:00:00 2001 From: David Julia Date: Wed, 13 May 2026 05:32:43 -0600 Subject: [PATCH 3/8] fix(slack): make per-button action_id unique (re #251 ) (#252) Heads up: [CONTRIBUTING.md](https://github.com/fabro-sh/fabro/blob/main/CONTRIBUTING.md) says you don't accept outside PRs, *"Instead of accepting outside pull requests, we accept bug reports and feature requests as GitHub Issues."* I filed the canonical bug report as **#251**, and that's where any actual discussion belongs. This PR is a courtesy ready-made diff in case it's useful to whoever supervises the AI workflow that lands this fix. Feel free to close it without comment; nothing is being asked of you here. I just thought it'd be useful to have a reference for what I did locally to fix it. ## What Two-file behaviour fix in `lib/crates/fabro-slack/`: every interview button on a multi-button gate now gets a Slack-unique `action_id`. Today they all share `"interview.answer"`, so Slack rejects the `chat.postMessage` with `invalid_blocks` and `SlackService::handle_event` silently drops the error. ## Why Multi-button Slack interview gates never reach Slack. Full reproducer, MITM-captured `invalid_blocks` response, and root-cause walkthrough are in **#251**. ## Diff shape - `blocks.rs`: each button gets a unique suffix. - `YesNo` / `Confirmation`: `interview.answer.yes` / `interview.answer.no` - `MultipleChoice`: `interview.answer.` (index, not raw key, to dodge Slack's 255-char `action_id` cap and any author-supplied charset surprises; selected key still rides in the button `value`) - `interaction.rs`: `parse_interaction` accepts both the legacy exact-prefix shape (in-flight buttons keep working across upgrade) and the new suffixed shape, via a pre-computed `ANSWER_ACTION_ID_PREFIX_DOT` constant so the parse hot path doesn't `format!` on every event. - Tests: +5 in `interaction.rs` (suffixed yes/no, suffixed multi-choice, legacy exact prefix, lookalike `interview.answers.yes` rejected, prefix-sync assertion). Updated the existing block-builder tests to assert uniqueness instead of the old single constant. One fixture each in `dispatch.rs` and `connection.rs` updated to the suffixed shape; one legacy fixture left in each to document backwards compatibility. 74/74 `fabro-slack` tests pass (was 69/69). `cargo +nightly-2026-04-14 fmt --check --all` and `cargo +nightly-2026-04-14 clippy -p fabro-slack --all-targets -- -D warnings` clean. ## Verified end-to-end Built a patched `fabro` binary, swapped it for the brew install, triggered a fresh multi-choice `Approve Plan` gate against a real Slack workspace, message rendered correctly in the configured channel with two clickable `[A] Approve` and `[R] Revise` buttons. Before the patch, the exact same gate produced zero Slack output and only the swallowed `invalid_blocks` was visible via MITM. ## Suggested follow-up (separate concern, not in this diff) The silent error swallow in `SlackService::handle_event` (`if let Ok(posted) = self.client.post_message(...)`) is what hid this bug. Worth logging at `WARN`. Mentioned in #251 as a separate item. ## Closes Closes #251 if you choose to land this directly. Otherwise this PR is just background material for the issue. --- lib/crates/fabro-slack/src/blocks.rs | 45 +++++++++-- lib/crates/fabro-slack/src/connection.rs | 2 +- lib/crates/fabro-slack/src/dispatch.rs | 2 +- lib/crates/fabro-slack/src/interaction.rs | 98 ++++++++++++++++++++++- 4 files changed, 136 insertions(+), 11 deletions(-) diff --git a/lib/crates/fabro-slack/src/blocks.rs b/lib/crates/fabro-slack/src/blocks.rs index 7ebc92be9..db143890f 100644 --- a/lib/crates/fabro-slack/src/blocks.rs +++ b/lib/crates/fabro-slack/src/blocks.rs @@ -4,11 +4,23 @@ use serde_json::{Value, json}; use crate::payload::{SlackActionPayload, encode_action_value}; -const ANSWER_ACTION_ID: &str = "interview.answer"; +pub(crate) const ANSWER_ACTION_ID_PREFIX: &str = "interview.answer"; const MULTI_SELECT_BLOCK_ID: &str = "interview.checkboxes"; const MULTI_SELECT_ACTION_ID: &str = "interview.select"; const MULTI_SELECT_SUBMIT_ACTION_ID: &str = "interview.submit"; +/// Build a Slack-unique `action_id` for an interview button. +/// +/// Slack requires `action_id`s to be unique within a single message and caps +/// them at 255 characters. The selected option is carried in the button +/// `value` payload, so the `action_id` only needs to be unique — it doesn't +/// have to encode the selection. Suffixes are short, fixed-shape tokens +/// (`yes`, `no`, or the option index) to avoid any character-set or length +/// concerns when option keys are author-supplied. +fn answer_action_id(suffix: &str) -> String { + format!("{ANSWER_ACTION_ID_PREFIX}.{suffix}") +} + fn text_block(text: &str) -> Value { json!({ "type": "section", @@ -48,11 +60,11 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) button("Yes", &encode_action_value(&SlackActionPayload::Yes { run_id: run_id.to_string(), qid: question_id.to_string(), - }), ANSWER_ACTION_ID), + }), &answer_action_id("yes")), button("No", &encode_action_value(&SlackActionPayload::No { run_id: run_id.to_string(), qid: question_id.to_string(), - }), ANSWER_ACTION_ID), + }), &answer_action_id("no")), ] }); vec![section, actions] @@ -61,7 +73,8 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) let elements: Vec = question .options .iter() - .map(|opt| { + .enumerate() + .map(|(idx, opt)| { button( &opt.label, &encode_action_value(&SlackActionPayload::Selected { @@ -69,7 +82,7 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) qid: question_id.to_string(), key: opt.key.clone(), }), - ANSWER_ACTION_ID, + &answer_action_id(&idx.to_string()), ) }) .collect(); @@ -185,7 +198,23 @@ mod tests { let elements = actions["elements"].as_array().unwrap(); assert_eq!(elements.len(), 3); assert_eq!(elements[0]["text"]["text"], "Rust"); - assert_eq!(elements[0]["action_id"], ANSWER_ACTION_ID); + assert_eq!(elements[0]["action_id"], "interview.answer.0"); + assert_eq!(elements[1]["action_id"], "interview.answer.1"); + assert_eq!(elements[2]["action_id"], "interview.answer.2"); + // Slack requires action_id to be unique within a message. + let ids: std::collections::HashSet<&str> = elements + .iter() + .map(|e| e["action_id"].as_str().unwrap()) + .collect(); + assert_eq!(ids.len(), elements.len()); + // The option key remains in the button `value` payload so the server + // can still route the answer regardless of suffix scheme. + assert!( + elements[0]["value"] + .as_str() + .unwrap() + .contains("\"key\":\"rs\"") + ); assert!( elements[0]["value"] .as_str() @@ -217,7 +246,9 @@ mod tests { let actions = &blocks_json[1]; let elements = actions["elements"].as_array().unwrap(); - assert_eq!(elements[0]["action_id"], ANSWER_ACTION_ID); + assert_eq!(elements[0]["action_id"], "interview.answer.yes"); + assert_eq!(elements[1]["action_id"], "interview.answer.no"); + assert_ne!(elements[0]["action_id"], elements[1]["action_id"]); let value = elements[0]["value"].as_str().unwrap(); assert!(value.contains("\"run_id\":\"run-7\"")); assert!(value.contains("\"qid\":\"q-7\"")); diff --git a/lib/crates/fabro-slack/src/connection.rs b/lib/crates/fabro-slack/src/connection.rs index 9a863f2c6..afbf507a3 100644 --- a/lib/crates/fabro-slack/src/connection.rs +++ b/lib/crates/fabro-slack/src/connection.rs @@ -225,7 +225,7 @@ mod tests { "team": { "id": "T123" }, "user": { "id": "U123", "name": "ada" }, "actions": [{ - "action_id": "interview.answer", + "action_id": "interview.answer.yes", "type": "button", "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" }] diff --git a/lib/crates/fabro-slack/src/dispatch.rs b/lib/crates/fabro-slack/src/dispatch.rs index 0c0ada6c9..548de5ea1 100644 --- a/lib/crates/fabro-slack/src/dispatch.rs +++ b/lib/crates/fabro-slack/src/dispatch.rs @@ -77,7 +77,7 @@ mod tests { "team": { "id": "T123" }, "user": { "id": "U123", "name": "ada" }, "actions": [{ - "action_id": "interview.answer", + "action_id": "interview.answer.yes", "type": "button", "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" }] diff --git a/lib/crates/fabro-slack/src/interaction.rs b/lib/crates/fabro-slack/src/interaction.rs index ec8ca0006..4fc7bac1d 100644 --- a/lib/crates/fabro-slack/src/interaction.rs +++ b/lib/crates/fabro-slack/src/interaction.rs @@ -1,13 +1,23 @@ use fabro_interview::Answer; use serde_json::Value; +use crate::blocks::ANSWER_ACTION_ID_PREFIX; use crate::payload::{self, SlackActionPayload, SlackAnswerSubmission}; const MULTI_SELECT_BLOCK_ID: &str = "interview.checkboxes"; const MULTI_SELECT_ACTION_ID: &str = "interview.select"; -const ANSWER_ACTION_ID: &str = "interview.answer"; const MULTI_SELECT_SUBMIT_ACTION_ID: &str = "interview.submit"; +/// Buttons for the same question must each have a unique `action_id`, so the +/// outbound side stamps `interview.answer.` per element. This matches +/// either the exact prefix (legacy compatibility for messages posted before +/// the suffix scheme) or the suffixed form (current). +const ANSWER_ACTION_ID_PREFIX_DOT: &str = "interview.answer."; + +fn is_answer_action(action_id: &str) -> bool { + action_id == ANSWER_ACTION_ID_PREFIX || action_id.starts_with(ANSWER_ACTION_ID_PREFIX_DOT) +} + /// Parses a Slack interaction payload and returns a server-routable answer /// submission. pub fn parse_interaction(payload: &Value) -> Option { @@ -25,7 +35,7 @@ pub fn parse_interaction(payload: &Value) -> Option { let action_type = action["type"].as_str().unwrap_or("button"); let answer = match action_type { - "button" if action_id == ANSWER_ACTION_ID => match routed { + "button" if is_answer_action(action_id) => match routed { SlackActionPayload::Yes { .. } => Answer::yes(), SlackActionPayload::No { .. } => Answer::no(), SlackActionPayload::Selected { key, .. } => Answer { @@ -255,4 +265,88 @@ mod tests { }); assert!(parse_interaction(&payload).is_none()); } + + /// Suffixed `action_id`s (per-button uniqueness for Slack) must still + /// route to the correct answer. + #[test] + fn parse_suffixed_yes_action_id() { + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answer.yes", + "type": "button", + "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" + }] + }); + let submission = parse_interaction(&payload).unwrap(); + assert_eq!(submission.answer.value, AnswerValue::Yes); + } + + #[test] + fn parse_suffixed_multiple_choice_action_id() { + // `interview.answer.` is what `question_to_blocks` now produces + // for multiple_choice questions. + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answer.2", + "type": "button", + "value": "{\"kind\":\"selected\",\"run_id\":\"run-1\",\"qid\":\"q-1\",\"key\":\"py\"}" + }] + }); + let submission = parse_interaction(&payload).unwrap(); + assert_eq!( + submission.answer.value, + AnswerValue::Selected("py".to_string()) + ); + } + + /// Legacy `action_id` without a suffix must still parse so messages + /// posted by older Fabro builds remain clickable after upgrade. + #[test] + fn parse_legacy_unsuffixed_action_id() { + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answer", + "type": "button", + "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" + }] + }); + let submission = parse_interaction(&payload).unwrap(); + assert_eq!(submission.answer.value, AnswerValue::Yes); + } + + /// Action ids that merely share a prefix but are not the answer family + /// must not be misrouted (no false-positive prefix match). + #[test] + fn rejects_lookalike_action_id() { + let payload = serde_json::json!({ + "type": "block_actions", + "team": { "id": "T123" }, + "user": { "id": "U123", "name": "ada" }, + "actions": [{ + "action_id": "interview.answers.yes", + "type": "button", + "value": "{\"kind\":\"yes\",\"run_id\":\"run-1\",\"qid\":\"q-1\"}" + }] + }); + assert!(parse_interaction(&payload).is_none()); + } + + /// The dotted prefix constant must stay in sync with the canonical + /// prefix so outbound and inbound never drift. + #[test] + fn dotted_prefix_constant_matches_canonical_prefix() { + assert_eq!( + ANSWER_ACTION_ID_PREFIX_DOT, + format!("{ANSWER_ACTION_ID_PREFIX}.") + ); + } } From a591a1ca6382c07cea9c6ea7dbb68040e3861a11 Mon Sep 17 00:00:00 2001 From: David Julia Date: Wed, 13 May 2026 05:41:54 -0600 Subject: [PATCH 4/8] feat(slack): render plan summary + run link in interview messages (re #253, stacked on #252) (#254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This branch contains both commits: 1. The action_id uniqueness fix from #252 (`fix(slack): make per-button action_id unique to satisfy Slack's invalid_blocks check`). 2. The new context+link fix (`feat(slack): render context_display and run link in interview messages`). The first commit needs to land (or be rebuilt by the maintainer workflow) before the second is meaningful, because without it multi-button gates still fail with `invalid_blocks` and the new context block never reaches Slack. Reviewing #252 first and this issue/PR second is the cleanest flow. ## What Two-file change in `lib/crates/fabro-slack/` and `lib/crates/fabro-server/`. The Slack interview message now includes the upstream stage's response (`context_display`) and a link back to the run, so a reviewer in Slack can decide A vs R without opening the web UI. ## Why See **#253** for the full repro, screenshots, and threat model. Short version: today the Slack message contains only the hexagon node's `label` plus the buttons, which is not enough information to act on. ## Diff shape - `question_to_blocks` gains a `run_web_url: Option<&str>` argument. - New helpers: - `header_section(question, run_web_url)`: bold question text, optional `stage \`{stage}\`` hint, and an "Open in Fabro" link when the URL is known. - `context_section(context_display)`: renders the upstream stage's response (e.g. plan summary, Dossier URLs) below the header, separated by a `divider`. Empty context_display is skipped so the message falls back cleanly to the old two-block shape. - `escape_slack_controls(text)`: HTML-entity escapes `&`, `<`, `>` in untrusted strings (question, stage, context_display, answered_blocks question and answer text). Neutralises LLM-produced payloads like ``, `<@U…>`, `<#C…>` while leaving Markdown formatting (`*bold*`, `_italic_`, `` `code` ``, `~strike~`) intact. Per https://docs.slack.dev/messaging/formatting-message-text/#escaping. - `truncate_to_limit`: clamps both the header text and the context block against Slack's documented 3000-character section text limit, with the truncation suffix counted against the budget so the result is always under the cap. Defends against pathological questions or oversized LLM responses producing `invalid_blocks`. - `server.rs`: `start_optional_slack_service`'s event subscriber calls `state.run_web_url(&envelope.event.run_id)` per event and forwards the result to `SlackService::handle_event`, which threads it into `question_to_blocks`. Returns `None` (and the link is omitted) when `server.web.enabled` is `false` or `server.web.url` is unset. ## Tests 10 new in `lib/crates/fabro-slack/src/blocks.rs`: - `header_includes_run_link_when_url_provided` - `header_omits_link_when_url_missing` - `header_shows_stage_when_present` - `header_truncates_when_inputs_exceed_section_limit` - `context_display_renders_between_header_and_actions` - `context_display_truncates_oversized_text_to_fit_slack_budget` - `empty_context_display_is_skipped` - `slack_control_chars_in_question_text_are_escaped` - `slack_control_chars_in_context_display_are_escaped` (covers ``/`<@U…>`/`<#C…>` neutralisation and verifies Markdown survives) - `answered_blocks_escape_slack_control_chars` 84/84 `fabro-slack` tests pass (was 74 after #252). `cargo +nightly-2026-04-14 fmt --check --all` and `cargo +nightly-2026-04-14 clippy -p fabro-slack -p fabro-server --all-targets -- -D warnings` both clean. ## Verified end-to-end Built a patched binary, swapped it for the brew install, triggered a fresh multi-choice approve gate against a real Slack workspace. The Slack message now renders with bold "Approve Plan" header, `stage \`approve\`` hint, "Open in Fabro" link, the upstream plan summary block (Dossier canonical and version URLs, `tmp-docs/fabro-plan.html` artifact path, the plan-summary bullets), a divider, and the `[A] Approve` and `[R] Revise` buttons. Reviewer can act on the gate from Slack alone. ## Latent observation, not in this diff `lib/crates/fabro-server/src/server.rs::AppState::run_web_url` has a comment saying it is snapshotted at create-time so attach replays remain stable when `server.web.url` changes, but the implementation reads current settings via `server_settings()`/`canonical_origin()`. This patch is unaffected (per-event call), but the comment looks stale. ## Closes Closes #253 if you choose to land this directly. Otherwise this PR is background material for the issue. --------- Co-authored-by: Bryan Helmkamp --- lib/crates/fabro-server/src/server.rs | 12 +- lib/crates/fabro-slack/src/blocks.rs | 343 +++++++++++++++++++++++--- 2 files changed, 323 insertions(+), 32 deletions(-) diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index caed79573..aaba95dac 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -385,7 +385,7 @@ impl SlackService { } } - async fn handle_event(&self, event: &RunEvent) { + async fn handle_event(&self, event: &RunEvent, run_web_url: Option<&str>) { match &event.body { EventBody::InterviewStarted(props) => { if props.question_id.is_empty() { @@ -415,6 +415,7 @@ impl SlackService { &event.run_id.to_string(), &props.question_id, &question, + run_web_url, ); if let Ok(posted) = self @@ -919,7 +920,14 @@ fn start_optional_slack_service(state: &Arc) { loop { match rx.recv().await { Ok(envelope) => { - event_service.handle_event(&envelope.event).await; + // Resolve the run's web URL once per event so the Slack + // message can deep-link back to Fabro. Returns None when + // the web UI is disabled or `server.web.url` is unset, in + // which case `question_to_blocks` simply omits the link. + let run_web_url = event_state.run_web_url(&envelope.event.run_id); + event_service + .handle_event(&envelope.event, run_web_url.as_deref()) + .await; } Err(RecvError::Lagged(_)) => {} Err(RecvError::Closed) => break, diff --git a/lib/crates/fabro-slack/src/blocks.rs b/lib/crates/fabro-slack/src/blocks.rs index db143890f..83a94543b 100644 --- a/lib/crates/fabro-slack/src/blocks.rs +++ b/lib/crates/fabro-slack/src/blocks.rs @@ -1,3 +1,5 @@ +use std::fmt::Write as _; + use fabro_interview::Question; use fabro_types::QuestionType; use serde_json::{Value, json}; @@ -9,6 +11,23 @@ const MULTI_SELECT_BLOCK_ID: &str = "interview.checkboxes"; const MULTI_SELECT_ACTION_ID: &str = "interview.select"; const MULTI_SELECT_SUBMIT_ACTION_ID: &str = "interview.submit"; +/// Slack section block `text.text` is documented to accept at most 3000 +/// characters (Unicode scalars). Both the header section and the context +/// preview are capped against this so a pathological question, stage, URL, +/// or LLM-produced context_display can never produce an `invalid_blocks` +/// response. See https://docs.slack.dev/reference/block-kit/blocks/section-block/. +const SLACK_SECTION_TEXT_LIMIT: usize = 3000; + +/// Suffix appended when `context_display` is truncated. Included in the +/// budget arithmetic so the final block is guaranteed to fit under the +/// section limit no matter how long the upstream stage's response was. +const CONTEXT_TRUNCATION_SUFFIX: &str = + "\n…\n_(truncated; open the run in Fabro for the full context)_"; + +/// Suffix appended when the header text itself exceeds the section limit +/// (e.g. an extremely long question label combined with a long stage name). +const HEADER_TRUNCATION_SUFFIX: &str = " …"; + /// Build a Slack-unique `action_id` for an interview button. /// /// Slack requires `action_id`s to be unique within a single message and caps @@ -43,18 +62,116 @@ fn button(label: &str, value: &str, action_id: &str) -> Value { }) } +fn divider() -> Value { + json!({ "type": "divider" }) +} + +/// Escape Slack control characters in untrusted text. Slack treats `<…>` +/// as link/mention syntax and `&` as the escape character, so leaving them +/// raw lets an upstream LLM stage post ``, `<@U…>`, or `<#C…>` +/// payloads that ping people or surface channels. Escaping these does NOT +/// break legitimate markdown like `*bold*`, `_italic_`, `~strike~`, or +/// `` `code` `` — those characters are not escaped here on purpose so +/// formatted text (e.g. a plan summary) still renders. +/// Per https://docs.slack.dev/messaging/formatting-message-text/#escaping. +fn escape_slack_controls(text: &str) -> String { + text.replace('&', "&") + .replace('<', "<") + .replace('>', ">") +} + +/// Truncate a string to at most `limit` Unicode scalars, appending `suffix` +/// when truncation occurs. `suffix` is included in the budget so the result +/// is always `<= limit` characters total. +fn truncate_to_limit(text: &str, limit: usize, suffix: &str) -> String { + if text.chars().count() <= limit { + return text.to_string(); + } + let suffix_len = suffix.chars().count(); + let keep = limit.saturating_sub(suffix_len); + let mut out: String = text.chars().take(keep).collect(); + out.push_str(suffix); + out +} + +/// Build the leading section block for an interview message: question label, +/// stage hint, and a deep link back to the run when one is available. The +/// final text is bounded by Slack's section-text limit so even pathological +/// inputs cannot produce `invalid_blocks`. +fn header_section(question: &Question, run_web_url: Option<&str>) -> Value { + let mut text = format!("*{}*", escape_slack_controls(&question.text)); + if !question.stage.is_empty() { + let _ = write!( + text, + " · stage `{}`", + escape_slack_controls(&question.stage) + ); + } + if let Some(url) = run_web_url { + // The URL is server-owned (built from `server.web.url` + run id) and + // does not flow through escape_slack_controls so the `<…|…>` link + // syntax is preserved. + let _ = write!(text, "\n<{url}|Open in Fabro>"); + } + text_block(&truncate_to_limit( + &text, + SLACK_SECTION_TEXT_LIMIT, + HEADER_TRUNCATION_SUFFIX, + )) +} + +/// Build a context section showing the upstream stage's response so a Slack +/// reviewer has enough information to act on the buttons without having to +/// open the run in the web UI. Slack control characters are escaped (so +/// LLM-produced content can't trigger unintended pings or channel mentions) +/// while leaving Markdown formatting intact. Truncated to fit Slack's +/// section text limit. +fn context_section(context_display: &str) -> Option { + let trimmed = context_display.trim(); + if trimmed.is_empty() { + return None; + } + let neutralized = escape_slack_controls(trimmed); + let bounded = truncate_to_limit( + &neutralized, + SLACK_SECTION_TEXT_LIMIT, + CONTEXT_TRUNCATION_SUFFIX, + ); + Some(text_block(&bounded)) +} + +/// Assemble the leading blocks shared by every question shape: header +/// section + optional context preview + a divider before the buttons. +fn lead_blocks(question: &Question, run_web_url: Option<&str>) -> Vec { + let mut blocks = vec![header_section(question, run_web_url)]; + if let Some(context_display) = question.context_display.as_deref() { + if let Some(section) = context_section(context_display) { + blocks.push(section); + blocks.push(divider()); + } + } + blocks +} + pub fn answered_blocks(question_text: &str, answer_text: &str) -> Vec { vec![text_block(&format!( - "~{question_text}~\n*Answer:* {answer_text}" + "~{}~\n*Answer:* {}", + escape_slack_controls(question_text), + escape_slack_controls(answer_text), ))] } -pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) -> Vec { - let section = text_block(&question.text); +pub fn question_to_blocks( + run_id: &str, + question_id: &str, + question: &Question, + run_web_url: Option<&str>, +) -> Vec { + let mut blocks = lead_blocks(question, run_web_url); match question.question_type { QuestionType::YesNo | QuestionType::Confirmation => { - let actions = json!({ + blocks.push(json!({ "type": "actions", "elements": [ button("Yes", &encode_action_value(&SlackActionPayload::Yes { @@ -66,8 +183,7 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) qid: question_id.to_string(), }), &answer_action_id("no")), ] - }); - vec![section, actions] + })); } QuestionType::MultipleChoice => { let elements: Vec = question @@ -86,11 +202,10 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) ) }) .collect(); - let actions = json!({ + blocks.push(json!({ "type": "actions", - "elements": elements - }); - vec![section, actions] + "elements": elements, + })); } QuestionType::MultiSelect => { let options: Vec = question @@ -103,7 +218,7 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) }) }) .collect(); - let checkboxes = json!({ + blocks.push(json!({ "type": "actions", "block_id": MULTI_SELECT_BLOCK_ID, "elements": [{ @@ -111,8 +226,8 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) "action_id": MULTI_SELECT_ACTION_ID, "options": options }] - }); - let submit = json!({ + })); + blocks.push(json!({ "type": "actions", "elements": [ button("Submit", &encode_action_value(&SlackActionPayload::SubmitMulti { @@ -120,16 +235,15 @@ pub fn question_to_blocks(run_id: &str, question_id: &str, question: &Question) qid: question_id.to_string(), }), MULTI_SELECT_SUBMIT_ACTION_ID), ] - }); - vec![section, checkboxes, submit] + })); } QuestionType::Freeform => { - vec![text_block(&format!( - "{}\n_Please reply in thread (mention me with your answer)._", - question.text - ))] + blocks.push(text_block( + "_Reply in thread (mention me with your answer)._", + )); } } + blocks } #[cfg(test)] @@ -141,7 +255,7 @@ mod tests { #[test] fn yes_no_produces_two_buttons() { let q = Question::new("Approve this PR?", QuestionType::YesNo); - let blocks = question_to_blocks("run-1", "q-1", &q); + let blocks = question_to_blocks("run-1", "q-1", &q, None); let blocks_json: Value = serde_json::to_value(&blocks).unwrap(); let section = &blocks_json[0]; @@ -164,7 +278,7 @@ mod tests { #[test] fn confirmation_produces_two_buttons() { let q = Question::new("Continue?", QuestionType::Confirmation); - let blocks = question_to_blocks("run-1", "q-2", &q); + let blocks = question_to_blocks("run-1", "q-2", &q, None); let blocks_json: Value = serde_json::to_value(&blocks).unwrap(); let actions = &blocks_json[1]; @@ -191,7 +305,7 @@ mod tests { label: "Python".to_string(), }, ]; - let blocks = question_to_blocks("run-1", "q-3", &q); + let blocks = question_to_blocks("run-1", "q-3", &q, None); let blocks_json: Value = serde_json::to_value(&blocks).unwrap(); let actions = &blocks_json[1]; @@ -228,20 +342,22 @@ mod tests { #[test] fn freeform_produces_section_prompting_thread_reply() { let q = Question::new("What's the repo URL?", QuestionType::Freeform); - let blocks = question_to_blocks("run-1", "q-4", &q); + let blocks = question_to_blocks("run-1", "q-4", &q, None); let blocks_json: Value = serde_json::to_value(&blocks).unwrap(); - assert_eq!(blocks_json.as_array().unwrap().len(), 1); - let text = blocks_json[0]["text"]["text"].as_str().unwrap(); - assert!(text.contains("What's the repo URL?")); - assert!(text.contains("reply in thread")); - assert!(text.contains("mention me")); + let arr = blocks_json.as_array().unwrap(); + assert_eq!(arr.len(), 2, "header section + thread-reply prompt"); + let header_text = arr[0]["text"]["text"].as_str().unwrap(); + assert!(header_text.contains("What's the repo URL?")); + let prompt_text = arr[1]["text"]["text"].as_str().unwrap(); + assert!(prompt_text.contains("Reply in thread")); + assert!(prompt_text.contains("mention me")); } #[test] fn action_values_include_run_id_and_question_id() { let q = Question::new("Approve?", QuestionType::YesNo); - let blocks = question_to_blocks("run-7", "q-7", &q); + let blocks = question_to_blocks("run-7", "q-7", &q, None); let blocks_json: Value = serde_json::to_value(&blocks).unwrap(); let actions = &blocks_json[1]; @@ -254,6 +370,173 @@ mod tests { assert!(value.contains("\"qid\":\"q-7\"")); } + #[test] + fn header_includes_run_link_when_url_provided() { + let q = Question::new("Approve Plan", QuestionType::YesNo); + let blocks = question_to_blocks( + "run-1", + "q-1", + &q, + Some("http://127.0.0.1:32276/runs/run-1"), + ); + let header = serde_json::to_value(&blocks).unwrap()[0]["text"]["text"] + .as_str() + .unwrap() + .to_string(); + assert!(header.contains("")); + } + + #[test] + fn header_omits_link_when_url_missing() { + let q = Question::new("Approve Plan", QuestionType::YesNo); + let blocks = question_to_blocks("run-1", "q-1", &q, None); + let header = serde_json::to_value(&blocks).unwrap()[0]["text"]["text"] + .as_str() + .unwrap() + .to_string(); + assert!(!header.contains("Open in Fabro")); + } + + #[test] + fn header_shows_stage_when_present() { + let mut q = Question::new("Approve Plan", QuestionType::YesNo); + q.stage = "plan".to_string(); + let blocks = question_to_blocks("run-1", "q-1", &q, None); + let header = serde_json::to_value(&blocks).unwrap()[0]["text"]["text"] + .as_str() + .unwrap() + .to_string(); + assert!(header.contains("stage `plan`")); + } + + #[test] + fn header_truncates_when_inputs_exceed_section_limit() { + let mut q = Question::new("a".repeat(4000), QuestionType::YesNo); + q.stage = "b".repeat(2000); + let blocks = question_to_blocks( + "run-1", + "q-1", + &q, + Some("http://127.0.0.1:32276/runs/run-1"), + ); + let header = serde_json::to_value(&blocks).unwrap()[0]["text"]["text"] + .as_str() + .unwrap() + .to_string(); + assert!( + header.chars().count() <= 3000, + "header text exceeded Slack section limit: {} chars", + header.chars().count() + ); + assert!(header.ends_with(" …")); + } + + #[test] + fn context_display_renders_between_header_and_actions() { + let mut q = Question::new("Approve Plan", QuestionType::YesNo); + q.context_display = Some( + "Plan artifact created and published.\n\n\ + - Local artifact: tmp-docs/fabro-plan.html\n\ + - Dossier canonical URL: https://example.test/s/siv-1067/eng-design-doc" + .to_string(), + ); + let blocks_json = + serde_json::to_value(question_to_blocks("run-1", "q-1", &q, None)).unwrap(); + let arr = blocks_json.as_array().unwrap(); + // 0: header section, 1: context section, 2: divider, 3: actions + assert_eq!(arr.len(), 4); + assert_eq!(arr[0]["type"], "section"); + assert_eq!(arr[1]["type"], "section"); + assert_eq!(arr[2]["type"], "divider"); + assert_eq!(arr[3]["type"], "actions"); + let context_text = arr[1]["text"]["text"].as_str().unwrap(); + assert!(context_text.contains("Plan artifact created")); + assert!(context_text.contains("tmp-docs/fabro-plan.html")); + } + + #[test] + fn context_display_truncates_oversized_text_to_fit_slack_budget() { + let mut q = Question::new("Approve Plan", QuestionType::YesNo); + q.context_display = Some("x".repeat(10_000)); + let blocks_json = + serde_json::to_value(question_to_blocks("run-1", "q-1", &q, None)).unwrap(); + let context_text = blocks_json[1]["text"]["text"].as_str().unwrap(); + assert!( + context_text.chars().count() <= 3000, + "context block exceeded Slack section text limit: {} chars", + context_text.chars().count() + ); + assert!(context_text.contains("truncated")); + } + + #[test] + fn empty_context_display_is_skipped() { + let mut q = Question::new("Approve Plan", QuestionType::YesNo); + q.context_display = Some(" \n\t ".to_string()); + let blocks_json = + serde_json::to_value(question_to_blocks("run-1", "q-1", &q, None)).unwrap(); + let arr = blocks_json.as_array().unwrap(); + // Falls back to header + actions when there's nothing meaningful. + assert_eq!(arr.len(), 2); + assert_eq!(arr[0]["type"], "section"); + assert_eq!(arr[1]["type"], "actions"); + } + + #[test] + fn slack_control_chars_in_question_text_are_escaped() { + let q = Question::new("Approve & merge?", QuestionType::YesNo); + let blocks_json = + serde_json::to_value(question_to_blocks("run-1", "q-1", &q, None)).unwrap(); + let header = blocks_json[0]["text"]["text"].as_str().unwrap(); + // &, <, > must be escaped so Slack doesn't reinterpret them as link + // or mention syntax. Other Markdown metacharacters (*, _, ~, `) are + // intentionally left untouched so legitimate formatting still renders. + assert!(header.contains("<plan>")); + assert!(header.contains("&")); + } + + #[test] + fn slack_control_chars_in_context_display_are_escaped() { + // An LLM-produced context_display could embed ``, `<@U…>`, or + // `<#C…>` which Slack would treat as a notification or mention. The + // escape must neutralise them while keeping bullets/bold/code intact. + let mut q = Question::new("Approve Plan", QuestionType::YesNo); + q.context_display = Some( + "Heads up: please review\n\ + - tagged: <@U12345>\n\ + - moved channel: <#C67890>\n\ + - kept: *bold* _italic_ `code` ~strike~" + .to_string(), + ); + let blocks_json = + serde_json::to_value(question_to_blocks("run-1", "q-1", &q, None)).unwrap(); + let context = blocks_json[1]["text"]["text"].as_str().unwrap(); + // Pings are neutralised. + assert!(!context.contains("")); + assert!(!context.contains("<@U12345>")); + assert!(!context.contains("<#C67890>")); + assert!(context.contains("<!here>")); + assert!(context.contains("<@U12345>")); + assert!(context.contains("<#C67890>")); + // Markdown formatting is preserved. + assert!(context.contains("*bold*")); + assert!(context.contains("_italic_")); + assert!(context.contains("`code`")); + assert!(context.contains("~strike~")); + } + + #[test] + fn answered_blocks_escape_slack_control_chars() { + let blocks = answered_blocks("Approve ?", "Yes & ship"); + let text = serde_json::to_value(&blocks).unwrap()[0]["text"]["text"] + .as_str() + .unwrap() + .to_string(); + assert!(!text.contains("")); + assert!(text.contains("<plan>")); + assert!(text.contains("Yes & ship")); + } + #[test] fn answered_blocks_show_question_and_answer() { let blocks = answered_blocks("Do you approve?", "Yes"); @@ -291,7 +574,7 @@ mod tests { label: "Billing".to_string(), }, ]; - let blocks = question_to_blocks("run-1", "q-5", &q); + let blocks = question_to_blocks("run-1", "q-5", &q, None); let blocks_json: Value = serde_json::to_value(&blocks).unwrap(); // Checkboxes in their own block with a block_id From 32970e0a30bbefda99d7aa94aa839802aa578753 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Wed, 13 May 2026 05:29:09 -0700 Subject: [PATCH 5/8] fix(graphviz): accept multi-line DOT attribute blocks without commas (#255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #179. ## Summary - `fabro validate`'s DOT parser rejected multi-line node attribute blocks unless every attribute was comma-separated, forcing long node definitions onto a single line. - Per the DOT spec, the separator between attributes is optional — whitespace (including newlines) alone is sufficient, and `,` or `;` are both accepted as explicit separators. - `attr_block` now uses `many0(terminated(attr, opt(',' | ';')))` instead of `separated_list0(',', attr)`, so all three forms parse identically. ## Before / after ```dot // previously rejected — now parses inspect [ label="Inspect Code" shape=tab prompt="@prompts/inspect.md" class="heavy" reasoning_effort="high" ] ``` ## Test plan - [x] Added regression test `parse_attr_block_multiline_without_commas` covering the exact form from #179. - [x] `cargo nextest run -p fabro-graphviz` — 109/109 pass, including the new test and existing comma-separated multi-line tests. - [x] `cargo +nightly-2026-04-14 clippy -p fabro-graphviz --all-targets -- -D warnings` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Nate Aune <118984+natea@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) --- .../fabro-graphviz/src/parser/grammar.rs | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/crates/fabro-graphviz/src/parser/grammar.rs b/lib/crates/fabro-graphviz/src/parser/grammar.rs index dd9a83449..d205bc6c2 100644 --- a/lib/crates/fabro-graphviz/src/parser/grammar.rs +++ b/lib/crates/fabro-graphviz/src/parser/grammar.rs @@ -1,11 +1,11 @@ use nom::IResult; use nom::branch::alt; use nom::bytes::complete::tag; -use nom::character::complete::{char, multispace0}; +use nom::character::complete::{char, multispace0, one_of}; use nom::combinator::opt; use nom::error::{Error, ParseError}; -use nom::multi::{many0, separated_list0}; -use nom::sequence::{delimited, preceded, tuple}; +use nom::multi::many0; +use nom::sequence::{delimited, preceded, terminated, tuple}; use crate::parser::ast::{ AstValue, AttrBlock, DotGraph, EdgeStmt, NodeStmt, Statement, SubgraphStmt, @@ -19,11 +19,15 @@ fn attr(input: &str) -> IResult<&str, (String, AstValue)> { Ok((rest, (k, v))) } -/// Parse an attribute block: `[ attr (, attr)* ]`. +/// Parse an attribute block: `[ attr (sep? attr)* ]` where `sep` is `,` or `;`. +/// +/// Per the DOT spec, the separator between attributes is optional — whitespace +/// (including newlines) alone is enough. This accepts comma-separated, +/// semicolon-separated, and newline-separated attribute lists interchangeably. fn attr_block(input: &str) -> IResult<&str, AttrBlock> { delimited( preceded(ws, char('[')), - separated_list0(preceded(ws, char(',')), attr), + many0(terminated(attr, opt(preceded(ws, one_of(",;"))))), preceded(ws, char(']')), )(input) } @@ -191,6 +195,30 @@ mod tests { assert_eq!(rest, ""); } + // Regression test for https://github.com/fabro-sh/fabro/issues/179. + // Standard DOT allows newline (or any whitespace) as an attribute separator + // inside `[ ... ]`, with commas optional. The multi-line, comma-less form is + // what most DOT editors and formatters produce for long attribute lists. + #[test] + fn parse_attr_block_multiline_without_commas() { + let input = "[\n label=\"Inspect Code\"\n shape=tab\n \ + prompt=\"@prompts/inspect.md\"\n class=\"heavy\"\n \ + reasoning_effort=\"high\"\n]"; + let (rest, attrs) = attr_block(input).unwrap(); + assert_eq!(attrs.len(), 5); + assert_eq!(attrs[0].0, "label"); + assert_eq!(attrs[0].1, AstValue::Str("Inspect Code".into())); + assert_eq!(attrs[1].0, "shape"); + assert_eq!(attrs[1].1, AstValue::Ident("tab".into())); + assert_eq!(attrs[2].0, "prompt"); + assert_eq!(attrs[2].1, AstValue::Str("@prompts/inspect.md".into())); + assert_eq!(attrs[3].0, "class"); + assert_eq!(attrs[3].1, AstValue::Str("heavy".into())); + assert_eq!(attrs[4].0, "reasoning_effort"); + assert_eq!(attrs[4].1, AstValue::Str("high".into())); + assert_eq!(rest, ""); + } + #[test] fn parse_graph_attr_stmt() { let (_, stmt) = graph_attr_stmt("graph [goal=\"Run tests\"]").unwrap(); From 4c557a9be9d6bd3e061f652c34fb92762e0cf69d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Wed, 13 May 2026 09:31:56 -0700 Subject: [PATCH 6/8] chore(deps): patch openssl and rmcp Dependabot advisories (#257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Patches all three open Dependabot alerts on `main`: - **#26 / #28 (openssl, high + medium)** — bump `openssl` 0.10.78 → 0.10.79. Patches `GHSA-xp3w-r5p5-63rr` (UB in `X509Ref::ocsp_responders` for certs with non-UTF-8 OCSP URLs) and `GHSA-xv59-967r-8726` (heap buffer overflow in AES key-wrap-with-padding). Lockfile-only. - **#27 (rmcp, high)** — bump workspace `rmcp` from `1.3` to `1.4`, which resolves to 1.7.0. Patches `GHSA-89vp-x53w-74fx` (DNS rebinding in the Streamable HTTP **server** transport). Fabro only uses the streamable-http **client** transport (`lib/crates/fabro-mcp/src/client.rs`), so practical exposure was nil — bumping anyway to stay on a supported, patched line. Each fix is in its own commit so it can be reverted independently. ## Test plan - [x] `cargo build --workspace` clean after each bump - [x] `cargo nextest run -p fabro-mcp -p fabro-mcp-server` — 30/30 pass on rmcp 1.7 - [ ] CI green 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- Cargo.lock | 17 ++++++++--------- Cargo.toml | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 850319cd3..970c2360e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4765,15 +4765,14 @@ dependencies = [ [[package]] name = "openssl" -version = "0.10.78" +version = "0.10.79" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f38c4372413cdaaf3cc79dd92d29d7d9f5ab09b51b10dded508fb90bb70b9222" +checksum = "bf0b434746ee2832f4f0baf10137e1cabb18cbe6912c69e2e33263c45250f542" dependencies = [ "bitflags", "cfg-if", "foreign-types", "libc", - "once_cell", "openssl-macros", "openssl-sys", ] @@ -4812,9 +4811,9 @@ dependencies = [ [[package]] name = "openssl-sys" -version = "0.9.114" +version = "0.9.115" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "13ce1245cd07fcc4cfdb438f7507b0c7e4f3849a69fd84d52374c66d83741bb6" +checksum = "158fe5b292746440aa6e7a7e690e55aeb72d41505e2804c23c6973ad0e9c9781" dependencies = [ "cc", "libc", @@ -5737,9 +5736,9 @@ checksum = "3582f63211428f83597b51b2ddb88e2a91a9d52d12831f9d08f5e624e8977422" [[package]] name = "rmcp" -version = "1.3.0" +version = "1.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2231b2c085b371c01bc90c0e6c1cab8834711b6394533375bdbf870b0166d419" +checksum = "0810a9f717d9828f475fe1f629f4c305c8464b7f496c3a854b58d29e65f4058e" dependencies = [ "async-trait", "base64", @@ -5764,9 +5763,9 @@ dependencies = [ [[package]] name = "rmcp-macros" -version = "1.3.0" +version = "1.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "36ea0e100fadf81be85d7ff70f86cd805c7572601d4ab2946207f36540854b43" +checksum = "6aefac48c364756e97f04c0401ba3231e8607882c7c1d92da0437dc16307904d" dependencies = [ "darling 0.23.0", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index f01038d80..e3186498d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,7 +45,7 @@ git2 = { version = "0.20", default-features = false, features = ["vendored-libgi tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["fmt", "env-filter"] } tracing-appender = "0.2" -rmcp = { version = "1.3", default-features = false } +rmcp = { version = "1.4", default-features = false } walkdir = "2" regex = "1" semver = "1" From 0e3d0c5c973e1bcdd8872ac8320aa32f18609ea3 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Wed, 13 May 2026 09:32:15 -0700 Subject: [PATCH 7/8] feat(manifest): support path-based Daytona Dockerfiles (#258) ## Summary Supports `dockerfile = { path = "..." }` for Daytona snapshots declared in `.fabro/project.toml` and workflow-local `workflow.toml`, resolving each path relative to the TOML file that declared it. Manifest building now bundles project-level Dockerfiles into the target workflow file bundle, and server manifest preparation rewrites bundled Dockerfile paths to inline content before settings reach sandbox creation. The repo Daytona snapshot config now uses `.fabro/Dockerfile` instead of embedding the Dockerfile in TOML, preserving the prior Dockerfile content exactly. ## Testing - `cargo nextest run -p fabro-manifest build_manifest_bundles_project_config_daytona_dockerfile_relative_to_project_config` - `cargo nextest run -p fabro-manifest` - `cargo nextest run -p fabro-server prepare_manifest_inlines_project_config_daytona_dockerfile_from_bundle prepare_manifest_errors_when_project_config_dockerfile_bundle_is_missing` - `cargo nextest run -p fabro-server` - `cargo nextest run -p fabro-config` - `cargo nextest run -p fabro-manifest -p fabro-server -p fabro-config` - `cargo +nightly-2026-04-14 fmt --check --all` - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` Optional credentialed Daytona live smoke was not run. ## Post-Deploy Monitoring & Validation - Log queries/search terms: `missing bundled dockerfile`, `unsupported dockerfile reference`, `invalid manifest project config path`, `dockerfile path should have been resolved to inline content before sandbox creation`, Daytona snapshot creation failures. - Metrics/dashboards: run submission/preflight failure rate, Daytona sandbox startup failure rate, snapshot creation failure rate, and run validation errors for manifests using bundled files. - Healthy signals: runs using `.fabro/project.toml` with `dockerfile = { path = "Dockerfile" }` progress past manifest preparation and Daytona snapshot creation without path-resolution errors. - Failure signals and rollback trigger: any sustained increase in manifest preparation failures or Daytona snapshot failures containing the log terms above; rollback by reverting this PR or temporarily restoring inline Dockerfile TOML for affected deployments. - Validation window and owner: first 24 hours after deploy; owner is the deploying operator/on-call engineer. ![Compound Engineered: Codex CLI / GPT-5](https://img.shields.io/badge/Compound%20Engineered-Codex%20CLI%20%2F%20GPT--5-blue) --- .fabro/Dockerfile | 61 +++ .fabro/project.toml | 64 +-- ...26-05-13-path-based-daytona-dockerfiles.md | 420 ++++++++++++++++++ lib/crates/fabro-config/src/builders.rs | 8 + lib/crates/fabro-manifest/src/lib.rs | 92 +++- lib/crates/fabro-server/src/run_manifest.rs | 125 +++++- 6 files changed, 690 insertions(+), 80 deletions(-) create mode 100644 .fabro/Dockerfile create mode 100644 docs/superpowers/plans/2026-05-13-path-based-daytona-dockerfiles.md diff --git a/.fabro/Dockerfile b/.fabro/Dockerfile new file mode 100644 index 000000000..f523c9c99 --- /dev/null +++ b/.fabro/Dockerfile @@ -0,0 +1,61 @@ +FROM ubuntu:24.04 + +RUN apt-get update && apt-get install -y --no-install-recommends \ + curl git ca-certificates build-essential pkg-config libssl-dev unzip python3 \ + xvfb xfce4 xfce4-terminal x11vnc novnc dbus-x11 \ + libx11-6 libxrandr2 libxext6 libxrender1 libxfixes3 libxss1 libxtst6 libxi6 \ + && rm -rf /var/lib/apt/lists/* + +# Install real Chromium (not the snap stub) via xtradeb PPA +RUN apt-get update && apt-get install -y --no-install-recommends \ + software-properties-common curl gnupg \ + && add-apt-repository -y ppa:xtradeb/apps \ + && apt-get update \ + && apt-get install -y --no-install-recommends chromium \ + && rm -rf /var/lib/apt/lists/* + +# Wrapper: Chromium needs --no-sandbox when running as root in a container, +# and --disable-dev-shm-usage avoids crashes from small /dev/shm +RUN printf '#!/bin/bash\nexec /usr/bin/chromium --no-sandbox --disable-dev-shm-usage "$@"\n' \ + > /usr/local/bin/chromium-wrapper \ + && chmod +x /usr/local/bin/chromium-wrapper + +# Make the wrapper the default in the system .desktop file and via alternatives +RUN sed -i 's|^Exec=.*|Exec=/usr/local/bin/chromium-wrapper %U|' \ + /usr/share/applications/chromium.desktop \ + && update-alternatives --install /usr/bin/x-www-browser x-www-browser \ + /usr/local/bin/chromium-wrapper 100 + +# Tell XFCE's exo-open that Chromium is the WebBrowser helper (system-wide) +RUN mkdir -p /etc/xdg/xfce4 /usr/share/xfce4/helpers \ + && printf 'WebBrowser=custom-WebBrowser\n' > /etc/xdg/xfce4/helpers.rc \ + && printf '[Desktop Entry]\n\ +Version=1.0\n\ +Type=X-XFCE-Helper\n\ +Name=Chromium\n\ +Icon=chromium\n\ +X-XFCE-Category=WebBrowser\n\ +X-XFCE-CommandsWithParameter=/usr/local/bin/chromium-wrapper "%%s"\n\ +X-XFCE-Commands=/usr/local/bin/chromium-wrapper\n' \ + > /usr/share/xfce4/helpers/custom-WebBrowser.desktop + +# GitHub CLI +RUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \ + | dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg \ + && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \ + | tee /etc/apt/sources.list.d/github-cli.list > /dev/null \ + && apt-get update && apt-get install -y --no-install-recommends gh \ + && rm -rf /var/lib/apt/lists/* + +# Rust +RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y +ENV PATH="/root/.cargo/bin:${PATH}" +RUN rustup toolchain install nightly-2026-04-14 --profile minimal --component clippy,rustfmt +RUN cargo install cargo-nextest --locked +ENV CARGO_INCREMENTAL=0 + +# Bun +RUN curl -fsSL https://bun.sh/install | bash +ENV PATH="/root/.bun/bin:${PATH}" + +WORKDIR /root diff --git a/.fabro/project.toml b/.fabro/project.toml index 35667daaf..c3c290f54 100644 --- a/.fabro/project.toml +++ b/.fabro/project.toml @@ -15,69 +15,7 @@ name = "fabro-v10" cpu = 8 memory = "16GB" disk = "20GB" -dockerfile = """ -FROM ubuntu:24.04 - -RUN apt-get update && apt-get install -y --no-install-recommends \ - curl git ca-certificates build-essential pkg-config libssl-dev unzip python3 \ - xvfb xfce4 xfce4-terminal x11vnc novnc dbus-x11 \ - libx11-6 libxrandr2 libxext6 libxrender1 libxfixes3 libxss1 libxtst6 libxi6 \ - && rm -rf /var/lib/apt/lists/* - -# Install real Chromium (not the snap stub) via xtradeb PPA -RUN apt-get update && apt-get install -y --no-install-recommends \ - software-properties-common curl gnupg \ - && add-apt-repository -y ppa:xtradeb/apps \ - && apt-get update \ - && apt-get install -y --no-install-recommends chromium \ - && rm -rf /var/lib/apt/lists/* - -# Wrapper: Chromium needs --no-sandbox when running as root in a container, -# and --disable-dev-shm-usage avoids crashes from small /dev/shm -RUN printf '#!/bin/bash\nexec /usr/bin/chromium --no-sandbox --disable-dev-shm-usage "$@"\n' \ - > /usr/local/bin/chromium-wrapper \ - && chmod +x /usr/local/bin/chromium-wrapper - -# Make the wrapper the default in the system .desktop file and via alternatives -RUN sed -i 's|^Exec=.*|Exec=/usr/local/bin/chromium-wrapper %U|' \ - /usr/share/applications/chromium.desktop \ - && update-alternatives --install /usr/bin/x-www-browser x-www-browser \ - /usr/local/bin/chromium-wrapper 100 - -# Tell XFCE's exo-open that Chromium is the WebBrowser helper (system-wide) -RUN mkdir -p /etc/xdg/xfce4 /usr/share/xfce4/helpers \ - && printf 'WebBrowser=custom-WebBrowser\n' > /etc/xdg/xfce4/helpers.rc \ - && printf '[Desktop Entry]\n\ -Version=1.0\n\ -Type=X-XFCE-Helper\n\ -Name=Chromium\n\ -Icon=chromium\n\ -X-XFCE-Category=WebBrowser\n\ -X-XFCE-CommandsWithParameter=/usr/local/bin/chromium-wrapper "%%s"\n\ -X-XFCE-Commands=/usr/local/bin/chromium-wrapper\n' \ - > /usr/share/xfce4/helpers/custom-WebBrowser.desktop - -# GitHub CLI -RUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \ - | dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg \ - && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \ - | tee /etc/apt/sources.list.d/github-cli.list > /dev/null \ - && apt-get update && apt-get install -y --no-install-recommends gh \ - && rm -rf /var/lib/apt/lists/* - -# Rust -RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y -ENV PATH="/root/.cargo/bin:${PATH}" -RUN rustup toolchain install nightly-2026-04-14 --profile minimal --component clippy,rustfmt -RUN cargo install cargo-nextest --locked -ENV CARGO_INCREMENTAL=0 - -# Bun -RUN curl -fsSL https://bun.sh/install | bash -ENV PATH="/root/.bun/bin:${PATH}" - -WORKDIR /root -""" +dockerfile = { path = "Dockerfile" } # [[run.hooks]] # id = "cargo-fmt" diff --git a/docs/superpowers/plans/2026-05-13-path-based-daytona-dockerfiles.md b/docs/superpowers/plans/2026-05-13-path-based-daytona-dockerfiles.md new file mode 100644 index 000000000..9760685ff --- /dev/null +++ b/docs/superpowers/plans/2026-05-13-path-based-daytona-dockerfiles.md @@ -0,0 +1,420 @@ +# Path-Based Daytona Dockerfiles Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Support `dockerfile = { path = "..." }` consistently from `.fabro/project.toml` and workflow-local `workflow.toml`, resolving paths relative to the TOML file that declares them. + +**Architecture:** Keep `fabro-sandbox` as the provider boundary: Daytona snapshot creation only accepts inline Dockerfile content. Resolve and bundle path-based Dockerfiles in the manifest build/prepare layers, then rewrite those path references to inline content before `WorkflowSettingsBuilder` materializes run settings. + +**Tech Stack:** Rust, `fabro-manifest`, `fabro-server`, `fabro-config`, `fabro-workflow::ManifestPath`, generated `fabro_api::types`. + +--- + +## Summary + +- Existing config syntax stays valid: + + ```toml + [run.sandbox.daytona.snapshot] + dockerfile = { path = "Dockerfile" } + ``` + +- Relative paths resolve against the file containing the reference: + - `.fabro/project.toml` + `Dockerfile` resolves to `.fabro/Dockerfile`. + - `.fabro/workflows/demo/workflow.toml` + `Dockerfile` resolves to `.fabro/workflows/demo/Dockerfile`. +- No OpenAPI schema change is needed. Dockerfile contents continue to travel through the existing manifest file bundle with `ref.type = "dockerfile"`. +- Scope is project-level `.fabro/project.toml` and workflow-local `workflow.toml`. User settings are left unchanged in this pass. +- Absolute paths and `~` references remain unsupported for manifest-bundled Dockerfiles, matching existing manifest reference rules. + +## Task 1: Bundle Project-Level Dockerfile Paths + +**Files:** +- Modify: `lib/crates/fabro-manifest/src/lib.rs` + +- [x] **Step 1: Add failing project-config bundling test** + + Add a unit test near the existing manifest bundling tests: + + ```rust + #[test] + fn build_manifest_bundles_project_config_daytona_dockerfile_relative_to_project_config() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = project.join(".fabro/workflows/demo"); + std::fs::create_dir_all(&workflow_dir).unwrap(); + + std::fs::write( + project.join(".fabro/project.toml"), + r#"_version = 1 + + [run.sandbox.daytona.snapshot] + name = "fabro-test" + dockerfile = { path = "Dockerfile" } + "#, + ) + .unwrap(); + std::fs::write(project.join(".fabro/Dockerfile"), "FROM ubuntu:24.04\n").unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r"digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() + }) + .unwrap(); + + let root = &built.manifest.workflows[".fabro/workflows/demo/workflow.fabro"]; + let entry = root + .files + .get(".fabro/Dockerfile") + .expect("project Dockerfile should be bundled with target workflow"); + assert_eq!(entry.content, "FROM ubuntu:24.04\n"); + assert_eq!(entry.ref_.type_, types::ManifestFileRefType::Dockerfile); + assert_eq!(entry.ref_.original, "Dockerfile"); + assert_eq!(entry.ref_.from.as_deref(), Some(".fabro/project.toml")); + } + ``` + +- [x] **Step 2: Run the failing test** + + Run: + + ```bash + cargo nextest run -p fabro-manifest build_manifest_bundles_project_config_daytona_dockerfile_relative_to_project_config + ``` + + Expected: FAIL because project config Dockerfile paths are not bundled. + +- [x] **Step 3: Extract and reuse Dockerfile bundling helper** + + In `lib/crates/fabro-manifest/src/lib.rs`, replace `collect_workflow_config_files` with a helper that accepts a config path, source, and destination file map: + + ```rust + fn collect_config_dockerfile( + context: &CollectContext<'_>, + config_path: &ManifestPath, + source: &str, + files: &mut HashMap, + ) -> Result<()> { + let mut document: toml::Table = source + .parse() + .context("Failed to parse run config TOML")?; + let run = document + .remove("run") + .map(toml::Value::try_into::) + .transpose() + .context("Failed to parse run config TOML")? + .unwrap_or_default(); + let dockerfile = run + .sandbox + .as_ref() + .and_then(|sandbox| sandbox.daytona.as_ref()) + .and_then(|daytona| daytona.snapshot.as_ref()) + .and_then(|snapshot| snapshot.dockerfile.as_ref()); + + let Some(DaytonaDockerfileLayer::Path { path }) = dockerfile else { + return Ok(()); + }; + + let absolute_config_path = context.cwd.join(config_path.as_path()); + collect_bundled_file( + files, + absolute_config_path + .parent() + .unwrap_or_else(|| Path::new(".")), + context.cwd, + path, + types::ManifestFileRefType::Dockerfile, + Some(config_path.clone()), + )?; + Ok(()) + } + ``` + + Update workflow config collection to parse the workflow config path and call this helper. + +- [x] **Step 4: Bundle project config Dockerfile into target workflow files** + + In `build_run_manifest`, read and retain the discovered project config source/path before `collect_workflow_entry`. After the target workflow entry has been inserted into `context.workflows`, call `collect_config_dockerfile` with the project config path/source and the target workflow's `files`. + + Keep the manifest `configs` entry for project config unchanged. + +- [x] **Step 5: Verify manifest tests pass** + + Run: + + ```bash + cargo nextest run -p fabro-manifest build_manifest_bundles_project_config_daytona_dockerfile_relative_to_project_config + cargo nextest run -p fabro-manifest + ``` + + Expected: PASS. + +## Task 2: Inline Bundled Dockerfiles During Server Manifest Preparation + +**Files:** +- Modify: `lib/crates/fabro-config/src/builders.rs` +- Modify: `lib/crates/fabro-server/src/run_manifest.rs` + +- [x] **Step 1: Add failing project-config preparation test** + + Add a unit test near existing `prepare_manifest` tests: + + ```rust + #[test] + fn prepare_manifest_inlines_project_config_daytona_dockerfile_from_bundle() { + let mut manifest = minimal_manifest(); + manifest.configs.push(types::ManifestConfig { + path: Some(".fabro/project.toml".to_string()), + source: Some( + r#"_version = 1 + + [run.sandbox] + provider = "daytona" + + [run.sandbox.daytona.snapshot] + name = "fabro-test" + dockerfile = { path = "Dockerfile" } + "# + .to_string(), + ), + type_: types::ManifestConfigType::Project, + }); + manifest + .workflows + .get_mut("workflow.fabro") + .unwrap() + .files + .insert(".fabro/Dockerfile".to_string(), types::ManifestFileEntry { + content: "FROM ubuntu:24.04\n".to_string(), + ref_: types::ManifestFileRef { + from: Some(".fabro/project.toml".to_string()), + original: "Dockerfile".to_string(), + type_: types::ManifestFileRefType::Dockerfile, + }, + }); + + let prepared = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ) + .unwrap(); + + let dockerfile = prepared + .settings + .run + .sandbox + .daytona + .as_ref() + .and_then(|daytona| daytona.snapshot.as_ref()) + .and_then(|snapshot| snapshot.dockerfile.as_ref()) + .expect("project Dockerfile should resolve"); + match dockerfile { + DockerfileSource::Inline(value) => assert_eq!(value, "FROM ubuntu:24.04\n"), + DockerfileSource::Path { path } => { + panic!("project Dockerfile should be inline, got path {path}") + } + } + } + ``` + +- [x] **Step 2: Add failing missing-bundle test** + + Add: + + ```rust + #[test] + fn prepare_manifest_errors_when_project_config_dockerfile_bundle_is_missing() { + let mut manifest = minimal_manifest(); + manifest.configs.push(types::ManifestConfig { + path: Some(".fabro/project.toml".to_string()), + source: Some( + r#"_version = 1 + + [run.sandbox.daytona.snapshot] + name = "fabro-test" + dockerfile = { path = "Dockerfile" } + "# + .to_string(), + ), + type_: types::ManifestConfigType::Project, + }); + + let err = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ) + .expect_err("missing bundled Dockerfile should fail"); + let message = format!("{err:#}"); + assert!( + message.contains("missing bundled dockerfile"), + "expected missing bundled dockerfile error, got: {message}" + ); + } + ``` + +- [x] **Step 3: Run failing tests** + + Run: + + ```bash + cargo nextest run -p fabro-server prepare_manifest_inlines_project_config_daytona_dockerfile_from_bundle prepare_manifest_errors_when_project_config_dockerfile_bundle_is_missing + ``` + + Expected: FAIL because project config Dockerfile paths are not rewritten. + +- [x] **Step 4: Generalize Dockerfile rewrite** + + Replace `resolve_manifest_dockerfile` with a helper that rewrites any `RunLayer` using: + + ```rust + fn resolve_manifest_dockerfile( + run: &mut RunLayer, + config_path: &ManifestPath, + files: &HashMap, + ) -> Result<()> + ``` + + Keep the existing behavior for workflow configs. + +- [x] **Step 5: Add a project settings builder entrypoint for rewritten run layers** + + In `lib/crates/fabro-config/src/builders.rs`, add a public method that preserves the parsed project TOML but replaces only its `[run]` layer: + + ```rust + pub fn project_toml_with_run_layer(self, source: &str, run: RunLayer) -> Result { + let mut layer = source + .parse::() + .map_err(|err| Error::parse("Failed to parse settings file", err))?; + layer.run = Some(run); + Ok(self.project_layer(layer)) + } + ``` + + This avoids ad hoc TOML string rewriting and keeps non-`[run]` project settings intact. + +- [x] **Step 6: Resolve project config path anchoring and rewrite project run layers** + + In `prepare_manifest`, for each project config with source: + + - Parse its run layer with `parse_run_layer_from_settings_toml(source)`. + - Convert `ManifestConfig.path` to a `ManifestPath`: absolute paths use `ManifestPath::from_absolute(Path::new(path), &cwd)`, relative paths use `ManifestPath::from_wire(path)`. + - Call `resolve_manifest_dockerfile(&mut run, &config_manifest_path, &workflow_input.files)`. + - Feed the result to `WorkflowSettingsBuilder::project_toml_with_run_layer(source, run)`. + + Do not let `DockerfileSource::Path` reach `fabro-sandbox`. + + Error message requirements: + - invalid/missing config path with a Dockerfile path: include `invalid manifest project config path`. + - missing bundled file: include `missing bundled dockerfile`. + +- [x] **Step 7: Verify server tests pass** + + Run: + + ```bash + cargo nextest run -p fabro-server prepare_manifest_inlines_project_config_daytona_dockerfile_from_bundle prepare_manifest_errors_when_project_config_dockerfile_bundle_is_missing + cargo nextest run -p fabro-server + ``` + + Expected: PASS. + +## Task 3: Move Repo Daytona Dockerfile Out of Inline TOML + +**Files:** +- Create: `.fabro/Dockerfile` +- Modify: `.fabro/project.toml` +- Modify: `lib/crates/fabro-config/src/project.rs` + +- [x] **Step 1: Move inline Dockerfile content** + + Create `.fabro/Dockerfile` containing the exact Dockerfile currently embedded in `.fabro/project.toml`, beginning with: + + ```dockerfile + FROM ubuntu:24.04 + ``` + + and ending with: + + ```dockerfile + WORKDIR /root + ``` + +- [x] **Step 2: Replace inline config reference** + + Change `.fabro/project.toml`: + + ```toml + dockerfile = { path = "Dockerfile" } + ``` + + Leave snapshot name/resources unchanged. + +- [x] **Step 3: Replace obsolete inline-newline test** + + Remove `project_daytona_dockerfile_preserves_chromium_wrapper_newline_escapes` if it exists. Coverage for this bug should now live in manifest/server path-resolution tests, not in `fabro-config`. + +- [x] **Step 4: Verify config and manifest tests** + + Run: + + ```bash + cargo nextest run -p fabro-config + cargo nextest run -p fabro-manifest + ``` + + Expected: PASS. + +## Task 4: Final Verification + +**Files:** +- No additional source files expected. + +- [x] **Step 1: Run targeted crate tests** + + Run: + + ```bash + cargo nextest run -p fabro-manifest -p fabro-server -p fabro-config + ``` + + Expected: PASS. + +- [x] **Step 2: Run format check** + + Run: + + ```bash + cargo +nightly-2026-04-14 fmt --check --all + ``` + + Expected: PASS. + +- [x] **Step 3: Run clippy** + + Run: + + ```bash + cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings + ``` + + Expected: PASS. + +- [ ] **Step 4: Optional live smoke** + + If Daytona credentials are available and the implementer wants to verify provider integration, run: + + ```bash + fabro run --sandbox daytona --goal-file /Users/bhelmkamp/p/fabro-sh/fabro/docs/plans/2026-04-15-canonical-blocked-run-status-plan.md implement-plan + ``` + + Expected: the run gets past Daytona snapshot Dockerfile parsing. The workflow itself may still fail later for unrelated implementation-plan reasons. diff --git a/lib/crates/fabro-config/src/builders.rs b/lib/crates/fabro-config/src/builders.rs index f32bdb9b3..f5fd4aa21 100644 --- a/lib/crates/fabro-config/src/builders.rs +++ b/lib/crates/fabro-config/src/builders.rs @@ -453,6 +453,14 @@ impl WorkflowSettingsBuilder { Ok(self.project_layer(layer)) } + pub fn project_toml_with_run_layer(self, source: &str, run: RunLayer) -> Result { + let mut layer = source + .parse::() + .map_err(|err| Error::parse("Failed to parse settings file", err))?; + layer.run = Some(run); + Ok(self.project_layer(layer)) + } + pub fn project_file(self, path: &Path) -> Result { Ok(self.project_layer(load_settings_path(path)?)) } diff --git a/lib/crates/fabro-manifest/src/lib.rs b/lib/crates/fabro-manifest/src/lib.rs index 3087a5bf5..7455c61c6 100644 --- a/lib/crates/fabro-manifest/src/lib.rs +++ b/lib/crates/fabro-manifest/src/lib.rs @@ -143,6 +143,16 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { .into()); } let project_config = discover_project_config(&root_location.dir)?; + let project_config_source = project_config + .as_ref() + .map(|path| { + let source = std::fs::read_to_string(path) + .with_context(|| format!("Failed to read {}", path.display()))?; + let manifest_path = manifest_path_from_absolute(path, &input.cwd)?; + Ok::<_, anyhow::Error>((path.clone(), manifest_path, source)) + }) + .transpose()?; + let mut workflow_settings_builder = WorkflowSettingsBuilder::new(); if let Some(run) = input.run_overrides.clone() { workflow_settings_builder = workflow_settings_builder.run_overrides(run); @@ -178,6 +188,13 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { visited_workflows: HashSet::new(), }; collect_workflow_entry(&mut context, &input.workflow, &input.cwd)?; + if let Some((_, config_path, source)) = project_config_source.as_ref() { + let workflow = context + .workflows + .get_mut(&target_key) + .ok_or_else(|| anyhow!("root workflow missing from manifest bundle"))?; + collect_config_dockerfile(context.cwd, config_path, source, &mut workflow.files)?; + } let root_source = context .workflows @@ -186,9 +203,7 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { .ok_or_else(|| anyhow!("root workflow missing from manifest bundle"))?; let mut configs = Vec::new(); - if let Some(path) = project_config { - let source = std::fs::read_to_string(&path) - .with_context(|| format!("Failed to read {}", path.display()))?; + if let Some((path, _, source)) = project_config_source { configs.push(types::ManifestConfig { path: Some(path.display().to_string()), source: Some(source), @@ -285,7 +300,9 @@ fn collect_workflow_entry( let mut files = HashMap::new(); let mut visited_imports = HashSet::new(); if let Some(config) = config.as_ref() { - collect_workflow_config_files(context, config, &mut files)?; + let config_path = ManifestPath::from_wire(&config.path) + .ok_or_else(|| anyhow!("invalid manifest workflow config path: {}", config.path))?; + collect_config_dockerfile(context.cwd, &config_path, &config.source, &mut files)?; } collect_workflow_files(context, &scan, &mut files, &mut visited_imports)?; @@ -405,15 +422,13 @@ fn render_workflow_scan_source( .with_context(|| format!("Failed to render {} for manifest scanning", path.display())) } -fn collect_workflow_config_files( - context: &CollectContext<'_>, - config: &types::ManifestWorkflowConfig, +fn collect_config_dockerfile( + cwd: &Path, + config_path: &ManifestPath, + source: &str, files: &mut HashMap, ) -> Result<()> { - let mut document: toml::Table = config - .source - .parse() - .context("Failed to parse run config TOML")?; + let mut document: toml::Table = source.parse().context("Failed to parse run config TOML")?; let run = document .remove("run") .map(toml::Value::try_into::) @@ -431,18 +446,16 @@ fn collect_workflow_config_files( return Ok(()); }; - let config_path = ManifestPath::from_wire(&config.path) - .ok_or_else(|| anyhow!("invalid manifest workflow config path: {}", config.path))?; - let absolute_config_path = context.cwd.join(config_path.as_path()); + let absolute_config_path = cwd.join(config_path.as_path()); collect_bundled_file( files, absolute_config_path .parent() .unwrap_or_else(|| Path::new(".")), - context.cwd, + cwd, path, types::ManifestFileRefType::Dockerfile, - Some(config_path), + Some(config_path.clone()), )?; Ok(()) } @@ -846,6 +859,53 @@ mod tests { ); } + #[test] + fn build_manifest_bundles_project_config_daytona_dockerfile_relative_to_project_config() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path(); + let workflow_dir = project.join(".fabro/workflows/demo"); + std::fs::create_dir_all(&workflow_dir).unwrap(); + + std::fs::write( + project.join(".fabro/project.toml"), + r#"_version = 1 + +[run.sandbox.daytona.snapshot] +name = "fabro-test" +dockerfile = { path = "Dockerfile" } +"#, + ) + .unwrap(); + std::fs::write(project.join(".fabro/Dockerfile"), "FROM ubuntu:24.04\n").unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r"digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); + + let built = build_run_manifest(ManifestBuildInput { + workflow: PathBuf::from(".fabro/workflows/demo/workflow.toml"), + cwd: project.to_path_buf(), + ..Default::default() + }) + .unwrap(); + + let root = &built.manifest.workflows[".fabro/workflows/demo/workflow.fabro"]; + let entry = root + .files + .get(".fabro/Dockerfile") + .expect("project Dockerfile should be bundled with target workflow"); + assert_eq!(entry.content, "FROM ubuntu:24.04\n"); + assert_eq!(entry.ref_.type_, types::ManifestFileRefType::Dockerfile); + assert_eq!(entry.ref_.original, "Dockerfile"); + assert_eq!(entry.ref_.from.as_deref(), Some(".fabro/project.toml")); + } + #[test] fn build_manifest_uses_input_overrides_for_structural_file_scanning() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 3d47379f5..32137f36e 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -110,7 +110,16 @@ pub(crate) fn prepare_manifest( .filter(|config| config.type_ == types::ManifestConfigType::Project) { if let Some(source) = config.source.as_deref() { - workflow_settings_builder = workflow_settings_builder.project_toml(source)?; + let mut run = parse_run_layer_from_settings_toml(source) + .context("Failed to parse project config TOML")?; + if run_has_path_dockerfile(&run) { + let config_path = manifest_project_config_path(config, &cwd)?; + resolve_manifest_dockerfile(&mut run, &config_path, &workflow_input.files)?; + workflow_settings_builder = + workflow_settings_builder.project_toml_with_run_layer(source, run)?; + } else { + workflow_settings_builder = workflow_settings_builder.project_toml(source)?; + } } } for config in manifest @@ -415,6 +424,34 @@ fn resolve_manifest_dockerfile( Ok(()) } +fn run_has_path_dockerfile(run: &RunLayer) -> bool { + matches!( + run.sandbox + .as_ref() + .and_then(|sandbox| sandbox.daytona.as_ref()) + .and_then(|daytona| daytona.snapshot.as_ref()) + .and_then(|snapshot| snapshot.dockerfile.as_ref()), + Some(DaytonaDockerfileLayer::Path { .. }) + ) +} + +fn manifest_project_config_path( + config: &types::ManifestConfig, + cwd: &Path, +) -> Result { + let path = config + .path + .as_deref() + .ok_or_else(|| anyhow!("invalid manifest project config path: missing path"))?; + let path_ref = Path::new(path); + let manifest_path = if path_ref.is_absolute() { + ManifestPath::from_absolute(path_ref, cwd) + } else { + ManifestPath::from_wire(path) + }; + manifest_path.ok_or_else(|| anyhow!("invalid manifest project config path: {path}")) +} + async fn build_preflight_report( state: &AppState, prepared: &PreparedManifest, @@ -1402,6 +1439,92 @@ enabled = {clone_enabled} (prepared, resolved) } + #[test] + fn prepare_manifest_inlines_project_config_daytona_dockerfile_from_bundle() { + let mut manifest = minimal_manifest(); + manifest.configs.push(types::ManifestConfig { + path: Some(".fabro/project.toml".to_string()), + source: Some( + r#"_version = 1 + +[run.sandbox] +provider = "daytona" + +[run.sandbox.daytona.snapshot] +name = "fabro-test" +dockerfile = { path = "Dockerfile" } +"# + .to_string(), + ), + type_: types::ManifestConfigType::Project, + }); + manifest + .workflows + .get_mut("workflow.fabro") + .unwrap() + .files + .insert(".fabro/Dockerfile".to_string(), types::ManifestFileEntry { + content: "FROM ubuntu:24.04\n".to_string(), + ref_: types::ManifestFileRef { + from: Some(".fabro/project.toml".to_string()), + original: "Dockerfile".to_string(), + type_: types::ManifestFileRefType::Dockerfile, + }, + }); + + let prepared = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ) + .unwrap(); + + let dockerfile = prepared + .settings + .run + .sandbox + .daytona + .as_ref() + .and_then(|daytona| daytona.snapshot.as_ref()) + .and_then(|snapshot| snapshot.dockerfile.as_ref()) + .expect("project Dockerfile should resolve"); + match dockerfile { + DockerfileSource::Inline(value) => assert_eq!(value, "FROM ubuntu:24.04\n"), + DockerfileSource::Path { path } => { + panic!("project Dockerfile should be inline, got path {path}") + } + } + } + + #[test] + fn prepare_manifest_errors_when_project_config_dockerfile_bundle_is_missing() { + let mut manifest = minimal_manifest(); + manifest.configs.push(types::ManifestConfig { + path: Some(".fabro/project.toml".to_string()), + source: Some( + r#"_version = 1 + +[run.sandbox.daytona.snapshot] +name = "fabro-test" +dockerfile = { path = "Dockerfile" } +"# + .to_string(), + ), + type_: types::ManifestConfigType::Project, + }); + + let Err(err) = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ) else { + panic!("missing bundled Dockerfile should fail"); + }; + let message = format!("{err:#}"); + assert!( + message.contains("missing bundled dockerfile"), + "expected missing bundled dockerfile error, got: {message}" + ); + } + #[tokio::test] async fn repository_access_check_skips_when_clone_is_disabled() { let (prepared, resolved) = prepared_and_resolved_for_sandbox( From 6297b200f771007a22929988716c3b8fbbbed8e8 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Wed, 13 May 2026 09:32:28 -0700 Subject: [PATCH 8/8] refactor(run): add rich failure contract (#256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Terminal run failures now use a first-class `RunFailure` contract so downstream consumers receive structured diagnostics instead of flat `error` / `causes` / `reason` fields. The wire shape keeps concise public messages, source-chain causes, classification, optional actor/signature data, and redacted exec output tail in one nested value. Refs fabro-sh/fabro#198 ## What Changed - Added `fabro_types::RunFailure` and changed `run.failed` to emit `properties.failure` with `final_git_commit_sha` for failed-run commit state. - Replaced `Conclusion.failure_reason` with `Conclusion.failure` while leaving stage-level `StageCompletion.failure_reason` untouched. - Updated workflow internals to preserve owned error source chains until terminal event projection, then convert them into `RunFailure.causes`. - Updated store, server, CLI, OpenAPI, and generated TypeScript client consumers to use the nested failure object. - Added serialization, OpenAPI replacement, projection, and lifecycle coverage for the new contract. ## Validation - `cargo nextest run -p fabro-api -p fabro-types -p fabro-workflow -p fabro-store -p fabro-server -p fabro-cli` - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` - `cargo +nightly-2026-04-14 fmt --check --all` - `cd apps/fabro-web && bun run typecheck` - `cd apps/fabro-web && bun test` - `git diff --check` --- [![Compound Engineering](https://img.shields.io/badge/Compound_Engineering-6366f1)](https://github.com/EveryInc/compound-engineering-plugin) 🤖 Generated with GPT-5 via [Codex](https://openai.com/codex) --- ...26-05-13-rich-run-failure-contract-plan.md | 81 ++++++ docs/public/api-reference/fabro-api.yaml | 67 ++++- lib/crates/fabro-api/build.rs | 5 + lib/crates/fabro-api/src/lib.rs | 18 +- .../fabro-api/tests/run_failure_round_trip.rs | 108 ++++++++ .../fabro-cli/src/commands/run/events.rs | 7 +- .../fabro-cli/src/commands/run/output.rs | 6 +- .../fabro-cli/src/commands/run/runner.rs | 48 ++-- lib/crates/fabro-cli/src/commands/run/wait.rs | 17 +- lib/crates/fabro-dump/src/lib.rs | 2 +- lib/crates/fabro-server/src/run_files.rs | 2 +- lib/crates/fabro-server/src/server.rs | 233 +++++++----------- lib/crates/fabro-server/src/server/tests.rs | 2 +- .../tests/it/scenario/lifecycle.rs | 6 +- lib/crates/fabro-store/src/run_state.rs | 115 ++++++--- lib/crates/fabro-store/src/slate/mod.rs | 14 +- lib/crates/fabro-types/src/conclusion.rs | 4 +- lib/crates/fabro-types/src/lib.rs | 2 + lib/crates/fabro-types/src/run_event/mod.rs | 7 +- lib/crates/fabro-types/src/run_event/run.rs | 21 +- lib/crates/fabro-types/src/run_failure.rs | 18 ++ .../fabro-types/tests/run_failure_serde.rs | 135 ++++++++++ lib/crates/fabro-workflow/src/artifact.rs | 32 +-- lib/crates/fabro-workflow/src/command_log.rs | 8 +- .../fabro-workflow/src/devcontainer_bridge.rs | 2 +- lib/crates/fabro-workflow/src/error.rs | 133 +++++----- .../fabro-workflow/src/event/convert.rs | 97 +++++--- lib/crates/fabro-workflow/src/event/events.rs | 58 ++++- lib/crates/fabro-workflow/src/git.rs | 12 +- .../fabro-workflow/src/handler/agent.rs | 2 +- .../fabro-workflow/src/handler/command.rs | 4 +- .../fabro-workflow/src/handler/llm/acp.rs | 6 +- .../fabro-workflow/src/handler/llm/api.rs | 4 +- .../fabro-workflow/src/handler/llm/cli.rs | 11 +- .../src/handler/llm/launch_env.rs | 6 +- .../fabro-workflow/src/handler/parallel.rs | 10 +- .../fabro-workflow/src/lifecycle/git.rs | 2 +- .../fabro-workflow/src/operations/archive.rs | 22 +- .../fabro-workflow/src/operations/create.rs | 2 +- .../fabro-workflow/src/operations/start.rs | 74 +++--- .../fabro-workflow/src/pipeline/finalize.rs | 72 +++--- .../fabro-workflow/src/pipeline/initialize.rs | 24 +- .../src/pipeline/pull_request.rs | 2 +- .../fabro-workflow/tests/it/integration.rs | 2 +- .../src/.openapi-generator/FILES | 3 + .../fabro-api-client/src/api/models-api.ts | 2 +- .../fabro-api-client/src/models/conclusion.ts | 5 +- .../src/models/exec-output-tail.ts | 26 ++ .../src/models/failure-category.ts | 33 +++ .../fabro-api-client/src/models/index.ts | 3 + .../fabro-api-client/src/models/model.ts | 6 +- .../src/models/run-failure.ts | 46 ++++ 52 files changed, 1140 insertions(+), 487 deletions(-) create mode 100644 docs/plans/2026-05-13-rich-run-failure-contract-plan.md create mode 100644 lib/crates/fabro-api/tests/run_failure_round_trip.rs create mode 100644 lib/crates/fabro-types/src/run_failure.rs create mode 100644 lib/crates/fabro-types/tests/run_failure_serde.rs create mode 100644 lib/packages/fabro-api-client/src/models/exec-output-tail.ts create mode 100644 lib/packages/fabro-api-client/src/models/failure-category.ts create mode 100644 lib/packages/fabro-api-client/src/models/run-failure.ts diff --git a/docs/plans/2026-05-13-rich-run-failure-contract-plan.md b/docs/plans/2026-05-13-rich-run-failure-contract-plan.md new file mode 100644 index 000000000..70c6f1858 --- /dev/null +++ b/docs/plans/2026-05-13-rich-run-failure-contract-plan.md @@ -0,0 +1,81 @@ +--- +title: "refactor: Rich run failure contract" +type: refactor +status: completed +date: 2026-05-13 +origin: https://github.com/fabro-sh/fabro/issues/198 +--- + +# refactor: Rich run failure contract + +## Summary + +Refactor terminal run failures around a first-class `RunFailure` value object. Preserve rich internal errors and source chains until terminal event projection, then serialize a structured failure payload with message, causes, classification, optional signature, actor, and redacted exec output tail. + +Assumption: this is greenfield. Do not preserve the old flat `run.failed.properties.error` / `causes` / `reason` contract or old `Conclusion.failure_reason` shape. + +## Key Changes + +- Add `fabro_types::RunFailure` with this wire shape: + - `message: String` + - `causes: Vec`, omitted when empty + - `reason: FailureReason` + - `category: FailureCategory` + - `system_actor: Option` + - `signature: Option` + - `exec_output_tail: Option` +- Change `RunFailedProps` to contain: + - `failure: RunFailure` + - `duration_ms` + - `final_git_commit_sha: Option`, replacing failed-run `git_commit_sha` + - `final_patch`, `diff_summary`, and optional `billing` +- Change `Conclusion` from `failure_reason: Option` to `failure: Option`. Keep `StageCompletion.failure_reason` unchanged; stage-level failure text is separate from terminal run failure diagnostics. +- Update OpenAPI `Conclusion` schemas and regenerate `lib/packages/fabro-api-client` models. `RunEvent.properties` is still generic, but docs/examples should show nested `failure`. + +## Implementation + +- In `fabro-workflow`, remove `fabro_workflow::Error` from `Event::WorkflowRunFailed`; the event carries `failure: RunFailure`. +- Add workflow-local projection helpers: + - `run_failure_from_error(error, reason)` for rich `Error` values. + - `run_failure_from_outcome_failure(failure_detail, reason)` for failed `Outcome` values without an error. +- Preserve source chains in `fabro_workflow::Error` before projection: + - Replace rendered `causes: Vec` fields on `Engine` / `Handler` with `source: Option`. + - Make `engine_with_source` / `handler_with_source` take owned `impl Into`, not borrowed `&dyn Error`. + - Remove `Serialize` / `Deserialize` from `fabro_workflow::Error`; it is internal error transport, not durable wire data. + - Keep `Error::causes()` and `display_with_causes()` as boundary helpers backed by `source()`. +- Populate `RunFailure` as follows: + - `message`: concise public message; for `Engine` / `Handler`, use the stored message without the `"Engine error:"` / `"Handler error:"` prefix. + - `causes`: collected source chain strings. + - `category`: `error.failure_category()` or `FailureDetail.category`. + - `signature`: existing `FailureDetail.signature` or `error.failure_signature_hint()`, wrapped as `FailureSignature`. + - `system_actor`: from `FailureDetail.system_actor`; `None` for generic errors. + - `exec_output_tail`: `fabro_sandbox::default_redacted_output_tail(error)`. +- Update consumers to read the new nested shape: + - run projection status uses `props.failure.reason` + - conclusion stores `Some(props.failure.clone())` + - server managed-run summaries use `props.failure.message` + - CLI progress/output renders `failure.message` plus `failure.causes` where full diagnostics are appropriate + - event tracing logs failure metadata and tail byte/truncation metadata only, never tail contents + +## Test Plan + +- `fabro-types` serialization tests: + - `run.failed` serializes with nested `properties.failure`. + - no top-level `error`, `causes`, `reason`, or `git_commit_sha` remains. + - `exec_output_tail` and empty `causes` are omitted when absent. + - `Conclusion` serializes `failure: RunFailure`. +- `fabro-workflow` projection tests: + - owned source chains survive through `Error::engine_with_source` / `handler_with_source`. + - a nested sandbox exec error produces `failure.exec_output_tail`. + - raw stdout/stderr never appear in `failure.message`. + - secrets in exec output are redacted in the serialized event payload. + - failed `Outcome` maps its `FailureDetail` category, actor, and signature into `RunFailure`. +- Projection/consumer tests: + - run state projects failed status from `props.failure.reason`. + - conclusion renders full message plus causes for CLI output. + - server managed-run failure summaries continue showing a concise message. + - existing `run.failed` fixtures and inline snapshots are updated intentionally. +- Verification commands: + - `cargo nextest run -p fabro-types -p fabro-workflow -p fabro-store -p fabro-server -p fabro-cli` + - `cd apps/fabro-web && bun run typecheck` + - regenerate API client after OpenAPI edits, then typecheck affected TypeScript. diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 6c1eda3ba..47097a334 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -4904,6 +4904,67 @@ components: - bootstrap_failed - sandbox_init_failed + FailureCategory: + description: Product-level classification for grouping and retry policy. + type: string + enum: + - transient_infra + - deterministic + - budget_exhausted + - compilation_loop + - canceled + - structural + + FailureSignature: + description: Stable normalized signature for grouping related failures. + type: string + + ExecOutputTail: + description: Redacted tail of command stdout/stderr captured for diagnostics. + type: object + properties: + stdout: + type: ["string", "null"] + stderr: + type: ["string", "null"] + stdout_truncated: + type: boolean + default: false + stderr_truncated: + type: boolean + default: false + + RunFailure: + description: Rich terminal run failure diagnostics. + type: object + required: + - message + - reason + - category + properties: + message: + type: string + causes: + type: array + items: + type: string + reason: + $ref: "#/components/schemas/FailureReason" + category: + $ref: "#/components/schemas/FailureCategory" + system_actor: + oneOf: + - $ref: "#/components/schemas/SystemActorKind" + - type: "null" + signature: + oneOf: + - $ref: "#/components/schemas/FailureSignature" + - type: "null" + exec_output_tail: + oneOf: + - $ref: "#/components/schemas/ExecOutputTail" + - type: "null" + RunManifest: description: Self-contained workflow run manifest. type: object @@ -6402,8 +6463,10 @@ components: type: integer format: uint64 minimum: 0 - failure_reason: - type: ["string", "null"] + failure: + oneOf: + - $ref: "#/components/schemas/RunFailure" + - type: "null" final_git_commit_sha: type: ["string", "null"] stages: diff --git a/lib/crates/fabro-api/build.rs b/lib/crates/fabro-api/build.rs index e81126d4e..ff492c02a 100644 --- a/lib/crates/fabro-api/build.rs +++ b/lib/crates/fabro-api/build.rs @@ -189,6 +189,9 @@ fn main() { ("RunStatus", "fabro_types::status::RunStatus", &[]), ("SuccessReason", "fabro_types::status::SuccessReason", &[]), ("FailureReason", "fabro_types::status::FailureReason", &[]), + ("FailureCategory", "fabro_types::FailureCategory", &[]), + ("FailureSignature", "fabro_types::FailureSignature", &[]), + ("RunFailure", "fabro_types::RunFailure", &[]), ("BlockedReason", "fabro_types::status::BlockedReason", &[]), ( "RunControlAction", @@ -338,6 +341,7 @@ fn main() { ("SystemActorKind", "fabro_types::SystemActorKind", &[]), ("QuestionType", "fabro_types::QuestionType", &[]), ("StageCompletion", "fabro_types::StageCompletion", &[]), + ("Conclusion", "fabro_types::Conclusion", &[]), ("StageOutcome", "fabro_types::StageOutcome", &[]), ("StageHandler", "fabro_types::StageHandler", &[]), ("StageState", "fabro_types::StageState", &[]), @@ -356,6 +360,7 @@ fn main() { &[], ), ("BilledTokenCounts", "fabro_types::BilledTokenCounts", &[]), + ("ExecOutputTail", "fabro_types::ExecOutputTail", &[]), ("ProviderId", "fabro_model::ProviderId", &[]), ("Model", "fabro_model::Model", &[]), ("ModelLimits", "fabro_model::ModelLimits", &[]), diff --git a/lib/crates/fabro-api/src/lib.rs b/lib/crates/fabro-api/src/lib.rs index e5a6e62a7..035f3deb6 100644 --- a/lib/crates/fabro-api/src/lib.rs +++ b/lib/crates/fabro-api/src/lib.rs @@ -29,15 +29,15 @@ pub mod types { BlockedReason, FailureReason, RunControlAction, RunStatus, SuccessReason, }; pub use fabro_types::{ - AuthMethod, BilledTokenCounts, CommandTermination, DiffStats, DiffSummary, DirtyStatus, - EventEnvelope, GitContext, IdpIdentity, InterviewOption, InterviewQuestionRecord, - PendingInterviewRecord, PreRunPushOutcome, Principal, PullRequest, PullRequestDetails, - QuestionType, RepositoryRef, Run, RunClientProvenance, RunEvent, RunProjection, - RunProvenance, RunSandbox, RunSandboxRuntime, RunServerProvenance, SandboxDetails, - SandboxProvider, SandboxResources, SandboxService, SandboxServiceListResponse, - SandboxState, SandboxTimestamps, SecretMetadata, SecretType, ServerSettings, - StageCompletion, StageHandler, StageOutcome, StageProjection, StageState, SystemActorKind, - UserPrincipal, WorkflowSettings, + AuthMethod, BilledTokenCounts, CommandTermination, Conclusion, DiffStats, DiffSummary, + DirtyStatus, EventEnvelope, ExecOutputTail, FailureCategory, FailureSignature, GitContext, + IdpIdentity, InterviewOption, InterviewQuestionRecord, PendingInterviewRecord, + PreRunPushOutcome, Principal, PullRequest, PullRequestDetails, QuestionType, RepositoryRef, + Run, RunClientProvenance, RunEvent, RunFailure, RunProjection, RunProvenance, RunSandbox, + RunSandboxRuntime, RunServerProvenance, SandboxDetails, SandboxProvider, SandboxResources, + SandboxService, SandboxServiceListResponse, SandboxState, SandboxTimestamps, + SecretMetadata, SecretType, ServerSettings, StageCompletion, StageHandler, StageOutcome, + StageProjection, StageState, SystemActorKind, UserPrincipal, WorkflowSettings, }; pub use crate::generated::types::*; diff --git a/lib/crates/fabro-api/tests/run_failure_round_trip.rs b/lib/crates/fabro-api/tests/run_failure_round_trip.rs new file mode 100644 index 000000000..418688c09 --- /dev/null +++ b/lib/crates/fabro-api/tests/run_failure_round_trip.rs @@ -0,0 +1,108 @@ +use std::any::{TypeId, type_name}; + +use fabro_api::types::{ + Conclusion as ApiConclusion, ExecOutputTail as ApiExecOutputTail, + FailureCategory as ApiFailureCategory, FailureSignature as ApiFailureSignature, + RunFailure as ApiRunFailure, +}; +use fabro_types::{ + Conclusion, ExecOutputTail, FailureCategory, FailureReason, FailureSignature, RunFailure, + StageOutcome, +}; +use serde::Serialize; +use serde_json::{Value, json}; + +#[test] +fn run_failure_family_reuses_domain_types() { + assert_same_type::(); + assert_same_type::(); + assert_same_type::(); + assert_same_type::(); + assert_same_type::(); +} + +#[test] +fn run_failure_json_matches_openapi_shape() { + assert_json( + RunFailure { + message: "Failed to initialize sandbox".to_string(), + causes: vec!["connection refused".to_string()], + reason: FailureReason::SandboxInitFailed, + category: FailureCategory::TransientInfra, + system_actor: None, + signature: Some(FailureSignature("init|transient_infra|docker".to_string())), + exec_output_tail: Some(ExecOutputTail { + stdout: None, + stderr: Some("last stderr line".to_string()), + stdout_truncated: false, + stderr_truncated: true, + }), + }, + json!({ + "message": "Failed to initialize sandbox", + "causes": ["connection refused"], + "reason": "sandbox_init_failed", + "category": "transient_infra", + "signature": "init|transient_infra|docker", + "exec_output_tail": { + "stderr": "last stderr line", + "stderr_truncated": true + } + }), + ); +} + +#[test] +fn conclusion_json_uses_failure_object() { + assert_json( + Conclusion { + timestamp: chrono::DateTime::parse_from_rfc3339("2026-05-13T12:00:00Z") + .unwrap() + .with_timezone(&chrono::Utc), + status: StageOutcome::Failed { + retry_requested: false, + }, + duration_ms: 42, + failure: Some(RunFailure { + message: "boom".to_string(), + causes: Vec::new(), + reason: FailureReason::WorkflowError, + category: FailureCategory::Deterministic, + system_actor: None, + signature: None, + exec_output_tail: None, + }), + final_git_commit_sha: None, + stages: Vec::new(), + billing: None, + total_retries: 0, + diff: Default::default(), + }, + json!({ + "timestamp": "2026-05-13T12:00:00Z", + "status": "failed", + "duration_ms": 42, + "failure": { + "message": "boom", + "reason": "workflow_error", + "category": "deterministic" + }, + "total_retries": 0, + "diff": {} + }), + ); +} + +fn assert_same_type() { + assert_eq!( + TypeId::of::(), + TypeId::of::(), + "{} should reuse {}", + type_name::(), + type_name::() + ); +} + +fn assert_json(value: T, expected: Value) { + assert_eq!(serde_json::to_value(value).unwrap(), expected); +} diff --git a/lib/crates/fabro-cli/src/commands/run/events.rs b/lib/crates/fabro-cli/src/commands/run/events.rs index e87da50e9..b6cc09f1c 100644 --- a/lib/crates/fabro-cli/src/commands/run/events.rs +++ b/lib/crates/fabro-cli/src/commands/run/events.rs @@ -454,7 +454,10 @@ fn format_event_pretty_value(envelope: &serde_json::Value, styles: &Styles) -> O Some(lines.join("\n")) } "run.failed" => { - let error = prop_str_field(envelope, "error").unwrap_or("unknown error"); + let error = prop_field(envelope, "failure") + .and_then(|failure| failure.get("message")) + .and_then(serde_json::Value::as_str) + .unwrap_or("unknown error"); Some(format!( "{} {} {}", styles.dim.apply_to(&ts), @@ -1260,7 +1263,7 @@ mod tests { #[test] fn pretty_workflow_run_failed() { let styles = no_color_styles(); - let line = r#"{"ts":"2026-01-01T14:23:32Z","run_id":"abc123","event":"run.failed","properties":{"error":"sandbox timeout"}}"#; + let line = r#"{"ts":"2026-01-01T14:23:32Z","run_id":"abc123","event":"run.failed","properties":{"failure":{"message":"sandbox timeout","reason":"workflow_error","category":"deterministic"}}}"#; let result = format_event_pretty(line, &styles).unwrap(); assert!(result.contains("Failed"), "got: {result}"); assert!(result.contains("sandbox timeout"), "got: {result}"); diff --git a/lib/crates/fabro-cli/src/commands/run/output.rs b/lib/crates/fabro-cli/src/commands/run/output.rs index d23aa2704..9278dd9b4 100644 --- a/lib/crates/fabro-cli/src/commands/run/output.rs +++ b/lib/crates/fabro-cli/src/commands/run/output.rs @@ -7,6 +7,7 @@ use cli_table::{Cell, CellStruct, Style, Table}; use fabro_api::types; use fabro_types::{PullRequestRecord, RunBlobId, RunId, parse_blob_ref}; use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; +use fabro_util::error::render_with_causes; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; use fabro_util::text::strip_goal_decoration; @@ -242,8 +243,9 @@ pub(crate) fn print_run_conclusion( } } - if let Some(ref failure) = conclusion.failure_reason { - fabro_util::printerr!(printer, "Failure: {}", styles.red.apply_to(failure)); + if let Some(ref failure) = conclusion.failure { + let rendered = render_with_causes(&failure.message, &failure.causes); + fabro_util::printerr!(printer, "Failure: {}", styles.red.apply_to(rendered)); } if pushed_branch.is_some() || pr_url.is_some() { diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index ca20d7a47..4b31c923b 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -508,7 +508,7 @@ fn worker_title_phase_for_event(body: &EventBody) -> Option { } EventBody::RunPaused(_) => Some(WorkerTitlePhase::Paused), EventBody::RunCompleted(_) => Some(WorkerTitlePhase::Succeeded), - EventBody::RunFailed(props) => Some(if props.reason == FailureReason::Cancelled { + EventBody::RunFailed(props) => Some(if props.failure.reason == FailureReason::Cancelled { WorkerTitlePhase::Cancelled } else { WorkerTitlePhase::Failed @@ -651,8 +651,8 @@ mod tests { RunFailedProps, RunStatusTransitionProps, }; use fabro_types::{ - AuthMethod, EventBody, FailureReason, IdpIdentity, Principal, QuestionType, SuccessReason, - fixtures, + AuthMethod, EventBody, FailureCategory, FailureReason, IdpIdentity, Principal, + QuestionType, RunFailure, SuccessReason, fixtures, }; use fabro_vault::{SecretType, Vault}; use fabro_workflow::event::RunEventSink; @@ -777,25 +777,39 @@ mod tests { ); assert_eq!( worker_title_phase_for_event(&EventBody::RunFailed(RunFailedProps { - error: "cancelled".to_string(), - causes: Vec::new(), - duration_ms: 10, - reason: FailureReason::Cancelled, - git_commit_sha: None, - final_patch: None, - diff_summary: None, + failure: RunFailure { + message: "cancelled".to_string(), + causes: Vec::new(), + reason: FailureReason::Cancelled, + category: FailureCategory::Canceled, + system_actor: None, + signature: None, + exec_output_tail: None, + }, + duration_ms: 10, + final_git_commit_sha: None, + final_patch: None, + diff_summary: None, + billing: None, })), Some(WorkerTitlePhase::Cancelled) ); assert_eq!( worker_title_phase_for_event(&EventBody::RunFailed(RunFailedProps { - error: "boom".to_string(), - causes: Vec::new(), - duration_ms: 10, - reason: FailureReason::Terminated, - git_commit_sha: None, - final_patch: None, - diff_summary: None, + failure: RunFailure { + message: "boom".to_string(), + causes: Vec::new(), + reason: FailureReason::Terminated, + category: FailureCategory::Deterministic, + system_actor: None, + signature: None, + exec_output_tail: None, + }, + duration_ms: 10, + final_git_commit_sha: None, + final_patch: None, + diff_summary: None, + billing: None, })), Some(WorkerTitlePhase::Failed) ); diff --git a/lib/crates/fabro-cli/src/commands/run/wait.rs b/lib/crates/fabro-cli/src/commands/run/wait.rs index a2d5bf148..6bde50d83 100644 --- a/lib/crates/fabro-cli/src/commands/run/wait.rs +++ b/lib/crates/fabro-cli/src/commands/run/wait.rs @@ -135,7 +135,8 @@ fn print_human_output( #[cfg(test)] mod tests { use fabro_types::{ - BilledTokenCounts, RunDiff, RunStatus, StageOutcome, SuccessReason, fixtures, + BilledTokenCounts, FailureCategory, FailureReason, RunDiff, RunFailure, RunStatus, + StageOutcome, SuccessReason, fixtures, }; use fabro_workflow::records::Conclusion; @@ -152,7 +153,7 @@ mod tests { timestamp: chrono::Utc::now(), status: StageOutcome::Succeeded, duration_ms: 12345, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: vec![], billing: Some(BilledTokenCounts { @@ -211,7 +212,15 @@ mod tests { retry_requested: false, }, duration_ms: 500, - failure_reason: Some("error".into()), + failure: Some(RunFailure { + message: "error".into(), + causes: Vec::new(), + reason: FailureReason::WorkflowError, + category: FailureCategory::Deterministic, + system_actor: None, + signature: None, + exec_output_tail: None, + }), final_git_commit_sha: None, stages: vec![], billing: None, @@ -237,7 +246,7 @@ mod tests { timestamp: chrono::Utc::now(), status: StageOutcome::Succeeded, duration_ms: 8000, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: vec![], billing: Some(BilledTokenCounts { diff --git a/lib/crates/fabro-dump/src/lib.rs b/lib/crates/fabro-dump/src/lib.rs index 06505dca1..8fed39198 100644 --- a/lib/crates/fabro-dump/src/lib.rs +++ b/lib/crates/fabro-dump/src/lib.rs @@ -549,7 +549,7 @@ mod tests { .unwrap(), status: StageOutcome::Succeeded, duration_ms: 5, - failure_reason: None, + failure: None, final_git_commit_sha: Some("abc123".to_string()), stages: Vec::new(), billing: None, diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index 8dac630a1..d93c02d69 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -2391,7 +2391,7 @@ index 1111111..2222222 160000 timestamp: chrono::Utc::now(), status: fabro_types::StageOutcome::Succeeded, duration_ms: 1, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: Vec::new(), billing: None, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index aaba95dac..3a41c0959 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2082,19 +2082,10 @@ pub(crate) async fn reconcile_incomplete_runs_on_startup( summary.lifecycle.pending_control, "Fabro server restarted before the run reached a terminal state.".to_string(), ); - workflow_event::append_event( - &run_store, - &summary.id, - &workflow_event::Event::WorkflowRunFailed { - error, - duration_ms: 0, - reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await?; + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &error, 0, reason, None, None, None, None, + ); + workflow_event::append_event(&run_store, &summary.id, &failure_event).await?; reconciled += 1; } @@ -2136,19 +2127,10 @@ async fn persist_shutdown_run_failures( run_state.pending_control, "Fabro server shut down before the run reached a terminal state.".to_string(), ); - workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error, - duration_ms: 0, - reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await?; + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &error, 0, reason, None, None, None, None, + ); + workflow_event::append_event(&run_store, &run_id, &failure_event).await?; } Ok(()) @@ -2216,19 +2198,16 @@ async fn persist_cancelled_run_status(state: &AppState, run_id: RunId) -> anyhow return Ok(()); } - workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error: WorkflowError::Cancelled, - duration_ms: 0, - reason: FailureReason::Cancelled, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &WorkflowError::Cancelled, + 0, + FailureReason::Cancelled, + None, + None, + None, + None, + ); + workflow_event::append_event(&run_store, &run_id, &failure_event).await } async fn finish_cancelled_run_before_execution(state: &Arc, run_id: RunId) { @@ -2255,19 +2234,17 @@ async fn fail_run_before_execution( ) { match state.store.open_run(&run_id).await { Ok(run_store) => { - if let Err(err) = workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error: WorkflowError::engine(message.clone()), - duration_ms: 0, - reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &WorkflowError::engine(message.clone()), + 0, + reason, + None, + None, + None, + None, + ); + if let Err(err) = + workflow_event::append_event(&run_store, &run_id, &failure_event).await { error!(run_id = %run_id, error = %err, "Failed to persist run failure status"); } @@ -2427,9 +2404,9 @@ fn update_live_run_from_event(state: &AppState, run_id: RunId, event: &RunEvent) } EventBody::RunFailed(props) => { managed_run.status = RunStatus::Failed { - reason: props.reason, + reason: props.failure.reason, }; - managed_run.error = Some(props.error.clone()); + managed_run.error = Some(props.failure.message.clone()); managed_run.active_api_stages.clear(); managed_run.active_non_steerable_agent_stages.clear(); } @@ -2540,21 +2517,11 @@ async fn append_worker_exit_failure( state.pending_control, format!("Worker exited before emitting a terminal run event: {wait_status}"), ); + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &error, 0, reason, None, None, None, None, + ); - if let Err(err) = workflow_event::append_event( - run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error, - duration_ms: 0, - reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await - { + if let Err(err) = workflow_event::append_event(run_store, &run_id, &failure_event).await { tracing::warn!(run_id = %run_id, error = %err, "Failed to append worker exit failure"); } } @@ -3202,25 +3169,18 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { Ok(child) => child, Err(err) => { tracing::error!(run_id = %run_id, error = %err, "Failed to spawn worker"); - let _ = workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error: WorkflowError::engine(err.to_string()), - duration_ms: 0, - reason: FailureReason::LaunchFailed, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await; - fail_managed_run( - &state, - run_id, + let message = format!("Failed to spawn worker: {err}"); + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &WorkflowError::engine_with_anyhow("Failed to spawn worker", err), + 0, FailureReason::LaunchFailed, - format!("Failed to spawn worker: {err}"), + None, + None, + None, + None, ); + let _ = workflow_event::append_event(&run_store, &run_id, &failure_event).await; + fail_managed_run(&state, run_id, FailureReason::LaunchFailed, message); state.scheduler_notify.notify_one(); return; } @@ -3230,19 +3190,16 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { let message = "Worker process did not report a PID".to_string(); tracing::error!(run_id = %run_id, "{message}"); let _ = child.start_kill(); - let _ = workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error: WorkflowError::engine(message.clone()), - duration_ms: 0, - reason: FailureReason::LaunchFailed, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await; + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &WorkflowError::engine(message.clone()), + 0, + FailureReason::LaunchFailed, + None, + None, + None, + None, + ); + let _ = workflow_event::append_event(&run_store, &run_id, &failure_event).await; fail_managed_run(&state, run_id, FailureReason::LaunchFailed, message); state.scheduler_notify.notify_one(); return; @@ -3261,19 +3218,16 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { let message = "Worker stdin pipe was unavailable".to_string(); tracing::error!(run_id = %run_id, "{message}"); let _ = child.start_kill(); - let _ = workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error: WorkflowError::engine(message.clone()), - duration_ms: 0, - reason: FailureReason::LaunchFailed, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await; + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &WorkflowError::engine(message.clone()), + 0, + FailureReason::LaunchFailed, + None, + None, + None, + None, + ); + let _ = workflow_event::append_event(&run_store, &run_id, &failure_event).await; fail_managed_run(&state, run_id, FailureReason::LaunchFailed, message); state.scheduler_notify.notify_one(); return; @@ -3283,19 +3237,16 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { let message = "Worker stderr pipe was unavailable".to_string(); tracing::error!(run_id = %run_id, "{message}"); let _ = child.start_kill(); - let _ = workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error: WorkflowError::engine(message.clone()), - duration_ms: 0, - reason: FailureReason::LaunchFailed, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await; + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &WorkflowError::engine(message.clone()), + 0, + FailureReason::LaunchFailed, + None, + None, + None, + None, + ); + let _ = workflow_event::append_event(&run_store, &run_id, &failure_event).await; fail_managed_run(&state, run_id, FailureReason::LaunchFailed, message); state.scheduler_notify.notify_one(); return; @@ -3316,26 +3267,19 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { Ok(status) => status, Err(err) => { tracing::error!(run_id = %run_id, error = %err, "Failed while waiting on worker"); + let message = format!("Worker wait failed: {err}"); let _ = child.start_kill(); - let _ = workflow_event::append_event( - &run_store, - &run_id, - &workflow_event::Event::WorkflowRunFailed { - error: WorkflowError::engine(err.to_string()), - duration_ms: 0, - reason: FailureReason::Terminated, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }, - ) - .await; - fail_managed_run( - &state, - run_id, + let failure_event = workflow_event::Event::workflow_run_failed_from_error( + &WorkflowError::engine_with_source("Worker wait failed", err), + 0, FailureReason::Terminated, - format!("Worker wait failed: {err}"), + None, + None, + None, + None, ); + let _ = workflow_event::append_event(&run_store, &run_id, &failure_event).await; + fail_managed_run(&state, run_id, FailureReason::Terminated, message); state.scheduler_notify.notify_one(); return; } @@ -3408,7 +3352,12 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { managed_run.error = final_state .conclusion .as_ref() - .and_then(|conclusion| conclusion.failure_reason.clone()) + .and_then(|conclusion| { + conclusion + .failure + .as_ref() + .map(|failure| failure.message.clone()) + }) .or_else(|| managed_run.error.clone()); managed_run.checkpoint = final_state.current_checkpoint().cloned(); managed_run.run_dir = Some(run_dir); diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index b072e7509..00d3a2090 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -2332,7 +2332,7 @@ async fn persist_cancelled_run_status_ignores_already_terminal_runs() { assert!(!run_store.list_events().await.unwrap().iter().any(|event| { matches!( event.event.body, - EventBody::RunFailed(ref props) if props.reason == FailureReason::Cancelled + EventBody::RunFailed(ref props) if props.failure.reason == FailureReason::Cancelled ) })); } diff --git a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs index d4c5c1583..e67a5e2fc 100644 --- a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs +++ b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs @@ -350,10 +350,12 @@ async fn cancel_at_human_gate_persists_cancelled_terminal_event() { .filter(|&event| event["event"] == "run.failed") .map(|event| { ( - event["properties"]["reason"] + event["properties"]["failure"]["reason"] + .as_str() + .map(ToOwned::to_owned), + event["properties"]["failure"]["message"] .as_str() .map(ToOwned::to_owned), - event["properties"]["error"].as_str().map(ToOwned::to_owned), ) }) .collect::>(); diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index 176b32c5a..973b94aec 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -16,7 +16,6 @@ use fabro_types::{ RunStatus, RunSummary, RunTimestamps, SandboxProvider, StageCompletion, StageHandler, StageId, StageOutcome, StageProjection, StageState, StartRecord, WorkflowRef, first_event_seq, }; -use fabro_util::error::render_with_causes; use serde_json::Value; use crate::{Error, EventEnvelope, Result}; @@ -149,7 +148,7 @@ impl RunProjectionReducer for RunProjection { EventBody::RunFailed(props) => { self.try_apply_status( RunStatus::Failed { - reason: props.reason, + reason: props.failure.reason, }, ts, )?; @@ -779,7 +778,7 @@ fn conclusion_from_completed( status: StageOutcome::from_str(&props.status) .map_err(|err| Error::InvalidEvent(format!("invalid completed stage status: {err}")))?, duration_ms: props.duration_ms, - failure_reason: None, + failure: None, final_git_commit_sha: props.final_git_commit_sha.clone(), stages: Vec::new(), billing: props.billing.clone(), @@ -798,10 +797,10 @@ fn conclusion_from_failed(props: &RunFailedProps, timestamp: DateTime) -> C retry_requested: false, }, duration_ms: props.duration_ms, - failure_reason: Some(render_with_causes(&props.error, &props.causes)), - final_git_commit_sha: props.git_commit_sha.clone(), + failure: Some(props.failure.clone()), + final_git_commit_sha: props.final_git_commit_sha.clone(), stages: Vec::new(), - billing: None, + billing: props.billing.clone(), total_retries: 0, diff: RunDiff { patch: props.final_patch.clone(), @@ -2181,13 +2180,20 @@ mod tests { .apply_event(&test_event( 1, EventBody::RunFailed(RunFailedProps { - error: "boom".to_string(), - causes: Vec::new(), - duration_ms: 42, - reason: FailureReason::WorkflowError, - git_commit_sha: Some("abc123".to_string()), - final_patch: Some(patch.to_string()), - diff_summary: None, + failure: fabro_types::RunFailure { + message: "boom".to_string(), + causes: Vec::new(), + reason: FailureReason::WorkflowError, + category: FailureCategory::Deterministic, + system_actor: None, + signature: None, + exec_output_tail: None, + }, + duration_ms: 42, + final_git_commit_sha: Some("abc123".to_string()), + final_patch: Some(patch.to_string()), + diff_summary: None, + billing: None, }), None, )) @@ -2294,9 +2300,12 @@ mod tests { 3, "run.failed", &json!({ - "error": "boom", + "failure": { + "message": "boom", + "reason": "workflow_error", + "category": "deterministic" + }, "duration_ms": 42, - "reason": "workflow_error", "diff_summary": { "files_changed": 5, "additions": 20, @@ -2323,27 +2332,71 @@ mod tests { .apply_event(&test_event( 1, EventBody::RunFailed(RunFailedProps { - error: "Engine error: Failed to initialize sandbox".to_string(), - causes: vec![ - "Failed to pull Docker image buildpack-deps:noble".to_string(), - "connection refused".to_string(), - ], - duration_ms: 42, - reason: FailureReason::WorkflowError, - git_commit_sha: None, - final_patch: None, - diff_summary: None, + failure: fabro_types::RunFailure { + message: "Failed to initialize sandbox".to_string(), + causes: vec![ + "Failed to pull Docker image buildpack-deps:noble".to_string(), + "connection refused".to_string(), + ], + reason: FailureReason::WorkflowError, + category: FailureCategory::TransientInfra, + system_actor: None, + signature: None, + exec_output_tail: None, + }, + duration_ms: 42, + final_git_commit_sha: None, + final_patch: None, + diff_summary: None, + billing: None, }), None, )) .unwrap(); - assert_eq!( - state.conclusion.unwrap().failure_reason.as_deref(), - Some( - "Engine error: Failed to initialize sandbox\n caused by: Failed to pull Docker image buildpack-deps:noble\n caused by: connection refused" - ) - ); + let failure = state.conclusion.unwrap().failure.unwrap(); + assert_eq!(failure.message, "Failed to initialize sandbox"); + assert_eq!(failure.causes, vec![ + "Failed to pull Docker image buildpack-deps:noble".to_string(), + "connection refused".to_string(), + ]); + } + + #[test] + fn run_failed_projection_uses_nested_failure_reason_and_conclusion() { + let mut state = running_projection(); + let failure = fabro_types::RunFailure { + message: "Failed to initialize sandbox".to_string(), + causes: vec!["connection refused".to_string()], + reason: FailureReason::SandboxInitFailed, + category: FailureCategory::TransientInfra, + system_actor: Some(fabro_types::SystemActorKind::Engine), + signature: Some(fabro_types::FailureSignature( + "init|transient_infra|docker".to_string(), + )), + exec_output_tail: None, + }; + state + .apply_event(&test_event( + 1, + EventBody::RunFailed(RunFailedProps { + failure: failure.clone(), + duration_ms: 42, + final_git_commit_sha: Some("abc123".to_string()), + final_patch: None, + diff_summary: None, + billing: None, + }), + None, + )) + .unwrap(); + + assert_eq!(state.status, RunStatus::Failed { + reason: FailureReason::SandboxInitFailed, + }); + let conclusion = state.conclusion.unwrap(); + assert_eq!(conclusion.failure, Some(failure)); + assert_eq!(conclusion.final_git_commit_sha.as_deref(), Some("abc123")); } #[test] diff --git a/lib/crates/fabro-store/src/slate/mod.rs b/lib/crates/fabro-store/src/slate/mod.rs index 7e90f2603..d94762152 100644 --- a/lib/crates/fabro-store/src/slate/mod.rs +++ b/lib/crates/fabro-store/src/slate/mod.rs @@ -724,9 +724,12 @@ mod tests { "2026-03-27T12:00:07Z", "run.failed", &serde_json::json!({ - "error": "cancelled", + "failure": { + "message": "cancelled", + "reason": "cancelled", + "category": "canceled" + }, "duration_ms": 1, - "reason": "cancelled", }), )) .await @@ -1122,9 +1125,12 @@ mod tests { "2026-03-27T12:00:04Z", "run.failed", &serde_json::json!({ - "error": "workflow failed", + "failure": { + "message": "workflow failed", + "reason": "workflow_error", + "category": "deterministic" + }, "duration_ms": 1, - "reason": "workflow_error", }), )) .await diff --git a/lib/crates/fabro-types/src/conclusion.rs b/lib/crates/fabro-types/src/conclusion.rs index 50d1eaa4d..9cced24bb 100644 --- a/lib/crates/fabro-types/src/conclusion.rs +++ b/lib/crates/fabro-types/src/conclusion.rs @@ -2,7 +2,7 @@ use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use crate::outcome::StageOutcome; -use crate::{BilledTokenCounts, RunDiff}; +use crate::{BilledTokenCounts, RunDiff, RunFailure}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct StageSummary { @@ -20,7 +20,7 @@ pub struct Conclusion { pub status: StageOutcome, pub duration_ms: u64, #[serde(default, skip_serializing_if = "Option::is_none")] - pub failure_reason: Option, + pub failure: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub final_git_commit_sha: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index e4da87ec6..7b11683b5 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -21,6 +21,7 @@ pub mod repository; pub mod run; pub mod run_blob_id; pub mod run_event; +pub mod run_failure; pub mod run_id; pub mod run_projection; pub mod run_sandbox; @@ -78,6 +79,7 @@ pub use run_event::{ EventBody, ExecOutputTail, InterviewOption, MetadataSnapshotFailureKind, MetadataSnapshotPhase, RunEvent, RunNoticeCode, RunNoticeLevel, SessionCapability, }; +pub use run_failure::RunFailure; pub use run_id::{RunId, fixtures}; pub use run_projection::{ CheckpointRecord, PendingInterviewRecord, RunProjection, StageProjection, first_event_seq, diff --git a/lib/crates/fabro-types/src/run_event/mod.rs b/lib/crates/fabro-types/src/run_event/mod.rs index b473ad6b4..2c46231ed 100644 --- a/lib/crates/fabro-types/src/run_event/mod.rs +++ b/lib/crates/fabro-types/src/run_event/mod.rs @@ -1086,9 +1086,12 @@ mod tests { ( "run.failed", json!({ - "error": "boom", + "failure": { + "message": "boom", + "reason": "workflow_error", + "category": "deterministic" + }, "duration_ms": 42, - "reason": "workflow_error", "diff_summary": { "files_changed": 2, "additions": 10, diff --git a/lib/crates/fabro-types/src/run_event/run.rs b/lib/crates/fabro-types/src/run_event/run.rs index 3b932883e..123d89c39 100644 --- a/lib/crates/fabro-types/src/run_event/run.rs +++ b/lib/crates/fabro-types/src/run_event/run.rs @@ -3,9 +3,9 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; use super::{BilledTokenCounts, ExecOutputTail, RunNoticeLevel}; -use crate::status::{BlockedReason, FailureReason, SuccessReason}; +use crate::status::{BlockedReason, SuccessReason}; use crate::{ - DiffSummary, ForkSourceRef, GitContext, Graph, RunBlobId, RunControlAction, RunId, + DiffSummary, ForkSourceRef, GitContext, Graph, RunBlobId, RunControlAction, RunFailure, RunId, RunProvenance, WorkflowSettings, }; @@ -151,19 +151,16 @@ pub struct RunCompletedProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunFailedProps { - pub error: String, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub causes: Vec, - pub duration_ms: u64, - pub reason: FailureReason, + pub failure: RunFailure, + pub duration_ms: u64, #[serde(default, skip_serializing_if = "Option::is_none")] - pub git_commit_sha: Option, - // Optional unified-patch text captured at run end. Additive for back-compat: - // pre-change events replay with `final_patch: None` via serde default. + pub final_git_commit_sha: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub final_patch: Option, + pub final_patch: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub diff_summary: Option, + pub diff_summary: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub billing: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-types/src/run_failure.rs b/lib/crates/fabro-types/src/run_failure.rs new file mode 100644 index 000000000..f519063b5 --- /dev/null +++ b/lib/crates/fabro-types/src/run_failure.rs @@ -0,0 +1,18 @@ +use serde::{Deserialize, Serialize}; + +use crate::{ExecOutputTail, FailureCategory, FailureReason, FailureSignature, SystemActorKind}; + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct RunFailure { + pub message: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub causes: Vec, + pub reason: FailureReason, + pub category: FailureCategory, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub system_actor: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub signature: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub exec_output_tail: Option, +} diff --git a/lib/crates/fabro-types/tests/run_failure_serde.rs b/lib/crates/fabro-types/tests/run_failure_serde.rs new file mode 100644 index 000000000..a5bf63270 --- /dev/null +++ b/lib/crates/fabro-types/tests/run_failure_serde.rs @@ -0,0 +1,135 @@ +use fabro_types::run_event::run::RunFailedProps; +use fabro_types::{ + Conclusion, EventBody, ExecOutputTail, FailureCategory, FailureReason, FailureSignature, + RunDiff, RunFailure, StageOutcome, SystemActorKind, +}; +use serde_json::json; + +#[test] +fn run_failed_serializes_nested_failure_contract() { + let body = EventBody::RunFailed(RunFailedProps { + failure: RunFailure { + message: "Failed to initialize sandbox".to_string(), + causes: vec![ + "Failed to pull Docker image buildpack-deps:noble".to_string(), + "connection refused".to_string(), + ], + reason: FailureReason::SandboxInitFailed, + category: FailureCategory::TransientInfra, + system_actor: Some(SystemActorKind::Engine), + signature: Some(FailureSignature( + "init|transient_infra|docker-pull".to_string(), + )), + exec_output_tail: Some(ExecOutputTail { + stdout: Some("last stdout line".to_string()), + stderr: Some("last stderr line".to_string()), + stdout_truncated: false, + stderr_truncated: true, + }), + }, + duration_ms: 42, + final_git_commit_sha: Some("abc123".to_string()), + final_patch: Some("diff --git a/file b/file".to_string()), + diff_summary: None, + billing: None, + }); + + let value = serde_json::to_value(&body).expect("run.failed body should serialize"); + + assert_eq!(value["event"], "run.failed"); + assert_eq!( + value["properties"], + json!({ + "failure": { + "message": "Failed to initialize sandbox", + "causes": [ + "Failed to pull Docker image buildpack-deps:noble", + "connection refused" + ], + "reason": "sandbox_init_failed", + "category": "transient_infra", + "system_actor": "engine", + "signature": "init|transient_infra|docker-pull", + "exec_output_tail": { + "stdout": "last stdout line", + "stderr": "last stderr line", + "stderr_truncated": true + } + }, + "duration_ms": 42, + "final_git_commit_sha": "abc123", + "final_patch": "diff --git a/file b/file" + }) + ); + assert!(value["properties"].get("error").is_none()); + assert!(value["properties"].get("causes").is_none()); + assert!(value["properties"].get("reason").is_none()); + assert!(value["properties"].get("git_commit_sha").is_none()); +} + +#[test] +fn run_failed_omits_empty_failure_optional_fields() { + let body = EventBody::RunFailed(RunFailedProps { + failure: RunFailure { + message: "boom".to_string(), + causes: Vec::new(), + reason: FailureReason::WorkflowError, + category: FailureCategory::Deterministic, + system_actor: None, + signature: None, + exec_output_tail: None, + }, + duration_ms: 1, + final_git_commit_sha: None, + final_patch: None, + diff_summary: None, + billing: None, + }); + + let value = serde_json::to_value(&body).expect("run.failed body should serialize"); + + assert_eq!( + value["properties"], + json!({ + "failure": { + "message": "boom", + "reason": "workflow_error", + "category": "deterministic" + }, + "duration_ms": 1 + }) + ); +} + +#[test] +fn conclusion_serializes_rich_failure() { + let conclusion = Conclusion { + timestamp: chrono::DateTime::parse_from_rfc3339("2026-05-13T12:00:00Z") + .unwrap() + .with_timezone(&chrono::Utc), + status: StageOutcome::Failed { + retry_requested: false, + }, + duration_ms: 42, + failure: Some(RunFailure { + message: "run failed".to_string(), + causes: vec!["leaf cause".to_string()], + reason: FailureReason::WorkflowError, + category: FailureCategory::Deterministic, + system_actor: None, + signature: None, + exec_output_tail: None, + }), + final_git_commit_sha: None, + stages: Vec::new(), + billing: None, + total_retries: 0, + diff: RunDiff::default(), + }; + + let value = serde_json::to_value(&conclusion).expect("conclusion should serialize"); + + assert_eq!(value["failure"]["message"], "run failed"); + assert_eq!(value["failure"]["causes"], json!(["leaf cause"])); + assert!(value.get("failure_reason").is_none()); +} diff --git a/lib/crates/fabro-workflow/src/artifact.rs b/lib/crates/fabro-workflow/src/artifact.rs index c991bf78c..510a37ab5 100644 --- a/lib/crates/fabro-workflow/src/artifact.rs +++ b/lib/crates/fabro-workflow/src/artifact.rs @@ -36,13 +36,13 @@ pub async fn offload_large_values( ) -> Result<()> { for value in updates.values_mut() { let bytes = serde_json::to_vec(&*value) - .map_err(|e| Error::engine_with_source("artifact serialize failed", &e))?; + .map_err(|e| Error::engine_with_source("artifact serialize failed", e))?; if bytes.len() > BLOB_OFFLOAD_THRESHOLD { let blob_id = run_store .write_blob(&bytes) .await - .map_err(|e| Error::engine_with_anyhow("artifact blob write failed", &e))?; + .map_err(|e| Error::engine_with_anyhow("artifact blob write failed", e))?; *value = Value::String(format_blob_ref(&blob_id)); } } @@ -171,10 +171,10 @@ pub async fn resolve_text_or_blob_ref_str( let bytes = run_store .read_blob(&blob_id) .await - .map_err(|e| Error::engine_with_anyhow("text blob read failed", &e))? + .map_err(|e| Error::engine_with_anyhow("text blob read failed", e))? .ok_or_else(|| Error::engine(format!("text blob missing: {blob_id}")))?; serde_json::from_slice::(&bytes) - .map_err(|e| Error::engine_with_source("text blob was not a JSON string", &e)) + .map_err(|e| Error::engine_with_source("text blob was not a JSON string", e)) } /// Sync artifact files to a remote sandbox. @@ -204,13 +204,13 @@ pub async fn sync_artifacts_to_env( Err(e) => { return Err(Error::engine_with_source( "failed to check artifact existence", - &e, + e, )); } } let content = fs::read_to_string(&local_path).await.map_err(|e| { - Error::engine_with_source(format!("failed to read local artifact {local_path}"), &e) + Error::engine_with_source(format!("failed to read local artifact {local_path}"), e) })?; let filename = std::path::Path::new(&local_path) @@ -222,7 +222,7 @@ pub async fn sync_artifacts_to_env( env.write_file(&remote_path, &content) .await - .map_err(|e| Error::engine_with_source("failed to write artifact to remote env", &e))?; + .map_err(|e| Error::engine_with_source("failed to write artifact to remote env", e))?; *value = Value::String(format!("{ARTIFACT_POINTER_PREFIX}{remote_path}")); } @@ -309,7 +309,7 @@ async fn materialize_blob_ref( let bytes = run_store .read_blob(blob_id) .await - .map_err(|e| Error::engine_with_anyhow("artifact blob read failed", &e))? + .map_err(|e| Error::engine_with_anyhow("artifact blob read failed", e))? .ok_or_else(|| Error::engine(format!("artifact blob missing: {blob_id}")))?; if is_local_execution(env, run_dir).await? { @@ -334,12 +334,12 @@ async fn materialize_blob_ref( if !env .file_exists(&remote_path) .await - .map_err(|e| Error::engine_with_source("failed to check blob existence", &e))? + .map_err(|e| Error::engine_with_source("failed to check blob existence", e))? { let content = String::from_utf8(bytes.to_vec()) - .map_err(|e| Error::engine_with_source("artifact blob was not valid UTF-8 JSON", &e))?; + .map_err(|e| Error::engine_with_source("artifact blob was not valid UTF-8 JSON", e))?; env.write_file(&remote_path, &content).await.map_err(|e| { - Error::engine_with_source("failed to write artifact blob to sandbox", &e) + Error::engine_with_source("failed to write artifact blob to sandbox", e) })?; } @@ -354,13 +354,13 @@ async fn resolve_explicit_file_ref(value: &str, env: &dyn Sandbox) -> Result Result Result Result { env.file_exists(&run_dir.to_string_lossy()) .await - .map_err(|e| Error::engine_with_source("failed to inspect sandbox locality", &e)) + .map_err(|e| Error::engine_with_source("failed to inspect sandbox locality", e)) } fn local_materialized_blob_path(run_dir: &Path, blob_id: &RunBlobId) -> PathBuf { diff --git a/lib/crates/fabro-workflow/src/command_log.rs b/lib/crates/fabro-workflow/src/command_log.rs index 0c6519660..67c4373e1 100644 --- a/lib/crates/fabro-workflow/src/command_log.rs +++ b/lib/crates/fabro-workflow/src/command_log.rs @@ -115,10 +115,10 @@ pub async fn read_json_string_blob( let bytes = run_store .read_blob(&blob_id) .await - .map_err(|err| Error::engine_with_anyhow("command log blob read failed", &err))? + .map_err(|err| Error::engine_with_anyhow("command log blob read failed", err))? .ok_or_else(|| Error::engine(format!("command log blob missing: {blob_id}")))?; let text = serde_json::from_slice::(&bytes) - .map_err(|err| Error::engine_with_source("command log blob was not a JSON string", &err))?; + .map_err(|err| Error::engine_with_source("command log blob was not a JSON string", err))?; Ok(Some(text)) } @@ -154,10 +154,10 @@ async fn remove_if_exists(path: &Path) -> Result<()> { async fn write_json_string_blob(run_store: &RunStoreHandle, text: &str) -> Result { let value = Value::String(text.to_string()); let bytes = serde_json::to_vec(&value) - .map_err(|err| Error::engine_with_source("command log JSON serialization failed", &err))?; + .map_err(|err| Error::engine_with_source("command log JSON serialization failed", err))?; let blob_id = run_store .write_blob(&bytes) .await - .map_err(|err| Error::engine_with_anyhow("command log blob write failed", &err))?; + .map_err(|err| Error::engine_with_anyhow("command log blob write failed", err))?; Ok(format_blob_ref(&blob_id)) } diff --git a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs index 37e2cf255..5d9cc092d 100644 --- a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs +++ b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs @@ -184,7 +184,7 @@ async fn run_single_lifecycle_command( .exec_command(command, timeout_ms, None, None, Some(child_token.clone())) .await .map_err(|e| { - Error::engine_with_source(format!("Devcontainer {phase} command failed"), &e) + Error::engine_with_source(format!("Devcontainer {phase} command failed"), e) })?; if cancel_token.is_cancelled() { return Err(Error::Cancelled); diff --git a/lib/crates/fabro-workflow/src/error.rs b/lib/crates/fabro-workflow/src/error.rs index 187129b44..cd830eb05 100644 --- a/lib/crates/fabro-workflow/src/error.rs +++ b/lib/crates/fabro-workflow/src/error.rs @@ -2,9 +2,9 @@ use fabro_graphviz::Error as GraphvizError; use fabro_llm::{Error as LlmError, ProviderErrorKind}; pub use fabro_types::failure_signature::FailureSignature; pub use fabro_types::outcome::FailureCategory; -use fabro_util::error::{collect_causes, collect_chain, render_with_causes}; +use fabro_types::{FailureReason, RunFailure}; +use fabro_util::error::{SharedError, collect_causes, collect_chain, render_with_causes}; use fabro_validate::Diagnostic; -use serde::{Deserialize, Serialize}; use thiserror::Error as ThisError; use crate::outcome::{FailureDetail, Outcome, StageOutcome}; @@ -194,8 +194,7 @@ impl FailureSignatureExt for FailureSignature { } } -#[derive(ThisError, Debug, Clone, Serialize, Deserialize)] -#[serde(tag = "type", content = "data", rename_all = "snake_case")] +#[derive(ThisError, Debug, Clone)] pub enum Error { #[error("Parse error: {0}")] Parse(String), @@ -210,16 +209,16 @@ pub enum Error { Engine { message: String, failure_class: FailureCategory, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - causes: Vec, + #[source] + source: Option, }, #[error("Handler error: {message}")] Handler { message: String, failure_class: FailureCategory, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - causes: Vec, + #[source] + source: Option, }, #[error("LLM error: {0}")] @@ -256,27 +255,28 @@ impl Error { Self::Handler { message, failure_class, - causes: Vec::new(), + source: None, } } pub fn handler_with_source( message: impl Into, - source: &(dyn std::error::Error + 'static), + source: impl Into, ) -> Self { let message = message.into(); - let causes = collect_chain(source); + 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, - causes, + source: Some(source), } } - pub fn handler_with_anyhow(message: impl Into, source: &anyhow::Error) -> Self { - Self::handler_with_source(message, source.as_ref()) + pub fn handler_with_anyhow(message: impl Into, source: anyhow::Error) -> Self { + Self::handler_with_source(message, source) } /// Smart constructor for Engine errors. Classifies the failure reason @@ -287,33 +287,36 @@ impl Error { Self::Engine { message, failure_class, - causes: Vec::new(), + source: None, } } pub fn engine_with_source( message: impl Into, - source: &(dyn std::error::Error + 'static), + source: impl Into, ) -> Self { let message = message.into(); - let causes = collect_chain(source); + 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, - causes, + source: Some(source), } } - pub fn engine_with_anyhow(message: impl Into, source: &anyhow::Error) -> Self { - Self::engine_with_source(message, source.as_ref()) + pub fn engine_with_anyhow(message: impl Into, source: anyhow::Error) -> Self { + Self::engine_with_source(message, source) } #[must_use] pub fn causes(&self) -> Vec { match self { - Self::Engine { causes, .. } | Self::Handler { causes, .. } => causes.clone(), + Self::Engine { source, .. } | Self::Handler { source, .. } => source + .as_ref() + .map_or_else(Vec::new, |source| collect_chain(source)), Self::Llm(err) => collect_causes(err), _ => Vec::new(), } @@ -396,6 +399,39 @@ impl Error { } } +#[must_use] +pub fn run_failure_from_error(error: &Error, reason: FailureReason) -> RunFailure { + let message = match error { + Error::Engine { message, .. } | Error::Handler { message, .. } => message.clone(), + _ => error.to_string(), + }; + RunFailure { + message, + causes: error.causes(), + reason, + category: error.failure_category(), + system_actor: None, + signature: error.failure_signature_hint().map(FailureSignature), + exec_output_tail: fabro_sandbox::default_redacted_output_tail(error), + } +} + +#[must_use] +pub fn run_failure_from_outcome_failure( + failure: &FailureDetail, + reason: FailureReason, +) -> RunFailure { + RunFailure { + message: failure.message.clone(), + causes: Vec::new(), + reason, + category: failure.category, + system_actor: failure.system_actor, + signature: failure.signature.clone().map(FailureSignature), + exec_output_tail: None, + } +} + impl From for Error { fn from(err: std::io::Error) -> Self { Self::Io(err.to_string()) @@ -431,13 +467,15 @@ impl From for Error { impl From for Error { fn from(err: fabro_checkpoint::MetadataError) -> Self { - let message = err.to_string(); match err { - fabro_checkpoint::MetadataError::Deserialize { + err @ fabro_checkpoint::MetadataError::Deserialize { entity: "checkpoint", .. - } => Self::Checkpoint(message), - _ => Self::engine(message), + } => Self::Checkpoint(err.to_string()), + err => { + let message = err.to_string(); + Self::engine_with_source(message, err) + } } } } @@ -520,7 +558,7 @@ mod tests { message: "Failed to pull Docker image buildpack-deps:noble", source: TestCause("connection refused"), }; - let err = Error::engine_with_source("Failed to initialize sandbox", &source); + let err = Error::engine_with_source("Failed to initialize sandbox", source); assert_eq!( err.to_string(), @@ -1707,14 +1745,10 @@ mod tests { } #[test] - fn handler_eager_classification_roundtrip() { + fn handler_eager_classification_survives_clone() { let err = Error::handler("connection refused"); - let json = serde_json::to_string(&err).unwrap(); - let deserialized: Error = serde_json::from_str(&json).unwrap(); - assert_eq!( - deserialized.failure_category(), - FailureCategory::TransientInfra - ); + let cloned = err.clone(); + assert_eq!(cloned.failure_category(), FailureCategory::TransientInfra); } #[test] @@ -1730,7 +1764,7 @@ mod tests { } #[test] - fn arc_error_serde_roundtrip_all_variants() { + fn error_clone_preserves_display_for_all_variants() { let errors: Vec = vec![ Error::Parse("bad".into()), Error::Validation("bad".into()), @@ -1755,10 +1789,8 @@ mod tests { Error::Io("io err".into()), Error::Cancelled, ]; - for err in &errors { - let json = serde_json::to_string(err).unwrap(); - let deserialized: Error = serde_json::from_str(&json).unwrap(); - assert_eq!(err.to_string(), deserialized.to_string()); + for err in errors { + assert_eq!(err.to_string(), err.clone().to_string()); } } @@ -1876,27 +1908,14 @@ mod tests { } #[test] - fn e2e_serde_stability_arc_error() { + fn e2e_run_failure_projection_uses_handler_error_shape() { let err = Error::handler("connection refused"); - let json = serde_json::to_string(&err).unwrap(); - let v: serde_json::Value = serde_json::from_str(&json).unwrap(); + let failure = run_failure_from_error(&err, FailureReason::WorkflowError); - // Verify wire format - assert_eq!(v["type"], "handler"); - assert!( - v["data"]["message"] - .as_str() - .unwrap() - .contains("connection refused") - ); - assert_eq!(v["data"]["failure_class"], "transient_infra"); - - // Round-trip - let deserialized: Error = serde_json::from_str(&json).unwrap(); - assert_eq!( - deserialized.failure_category(), - FailureCategory::TransientInfra - ); + assert_eq!(failure.message, "connection refused"); + assert_eq!(failure.causes, Vec::::new()); + assert_eq!(failure.reason, FailureReason::WorkflowError); + assert_eq!(failure.category, FailureCategory::TransientInfra); } #[test] diff --git a/lib/crates/fabro-workflow/src/event/convert.rs b/lib/crates/fabro-workflow/src/event/convert.rs index da2af76fc..3ae8bbfaf 100644 --- a/lib/crates/fabro-workflow/src/event/convert.rs +++ b/lib/crates/fabro-workflow/src/event/convert.rs @@ -166,20 +166,19 @@ fn event_body_from_event(event: &Event) -> EventBody { billing: billing.clone(), }), Event::WorkflowRunFailed { - error, + failure, duration_ms, - reason, - git_commit_sha, + final_git_commit_sha, final_patch, diff_summary, + billing, } => EventBody::RunFailed(fabro_types::RunFailedProps { - error: error.to_string(), - causes: error.causes(), - duration_ms: *duration_ms, - reason: *reason, - git_commit_sha: git_commit_sha.clone(), - final_patch: final_patch.clone(), - diff_summary: *diff_summary, + failure: failure.clone(), + duration_ms: *duration_ms, + final_git_commit_sha: final_git_commit_sha.clone(), + final_patch: final_patch.clone(), + diff_summary: *diff_summary, + billing: billing.clone(), }), Event::RunNotice { level, @@ -1528,44 +1527,82 @@ mod tests { #[test] fn run_event_workflow_failure_uses_display_error() { - let stored = to_run_event(&fixtures::RUN_6, &Event::WorkflowRunFailed { - error: Error::handler("boom"), - duration_ms: 900, - reason: FailureReason::WorkflowError, - git_commit_sha: Some("abc123".to_string()), - final_patch: None, - diff_summary: None, - }); + let event = Event::workflow_run_failed_from_error( + &Error::handler("boom"), + 900, + FailureReason::WorkflowError, + Some("abc123".to_string()), + None, + None, + None, + ); + let stored = to_run_event(&fixtures::RUN_6, &event); assert_eq!(stored.event_name(), "run.failed"); let properties = stored.properties().unwrap(); - assert_eq!(properties["error"], "Handler error: boom"); + assert_eq!(properties["failure"]["message"], "boom"); assert_eq!(properties["duration_ms"], 900); } #[test] fn run_event_workflow_failure_serializes_causes() { let source = EventTestCause; - let stored = to_run_event(&fixtures::RUN_6, &Event::WorkflowRunFailed { - error: Error::engine_with_source("Failed to initialize sandbox", &source), - duration_ms: 900, - reason: FailureReason::WorkflowError, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }); + let event = Event::workflow_run_failed_from_error( + &Error::engine_with_source("Failed to initialize sandbox", source), + 900, + FailureReason::WorkflowError, + None, + None, + None, + None, + ); + let stored = to_run_event(&fixtures::RUN_6, &event); let properties = stored.properties().unwrap(); assert_eq!( - properties["error"], - "Engine error: Failed to initialize sandbox" + properties["failure"]["message"], + "Failed to initialize sandbox" ); assert_eq!( - properties["causes"], + properties["failure"]["causes"], serde_json::json!(["connection refused"]) ); } + #[test] + fn run_event_workflow_failure_projects_nested_failure_contract() { + let source = EventTestCause; + let event = Event::workflow_run_failed_from_error( + &Error::engine_with_source("Failed to initialize sandbox", source), + 900, + FailureReason::SandboxInitFailed, + Some("abc123".to_string()), + None, + None, + None, + ); + let stored = to_run_event(&fixtures::RUN_6, &event); + + assert_eq!(stored.event_name(), "run.failed"); + let properties = stored.properties().unwrap(); + assert_eq!( + properties["failure"]["message"], + "Failed to initialize sandbox" + ); + assert_eq!( + properties["failure"]["causes"], + serde_json::json!(["connection refused"]) + ); + assert_eq!(properties["failure"]["reason"], "sandbox_init_failed"); + assert_eq!(properties["failure"]["category"], "transient_infra"); + assert_eq!(properties["duration_ms"], 900); + assert_eq!(properties["final_git_commit_sha"], "abc123"); + assert!(properties.get("error").is_none()); + assert!(properties.get("causes").is_none()); + assert!(properties.get("reason").is_none()); + assert!(properties.get("git_commit_sha").is_none()); + } + #[test] fn stage_started_populates_parallel_ids_when_present() { let stored = to_run_event_at( diff --git a/lib/crates/fabro-workflow/src/event/events.rs b/lib/crates/fabro-workflow/src/event/events.rs index f2067c5f4..3de11ce1b 100644 --- a/lib/crates/fabro-workflow/src/event/events.rs +++ b/lib/crates/fabro-workflow/src/event/events.rs @@ -2,14 +2,14 @@ use std::collections::BTreeMap; use ::fabro_types::{ BilledTokenCounts, BlockedReason, CommandTermination, DiffSummary, FailureReason, - ForkSourceRef, GitContext, ParallelBranchId, Principal, PullRequestRecord, RunBlobId, RunId, - RunNoticeLevel, RunProvenance, SandboxProvider, StageId, SuccessReason, + ForkSourceRef, GitContext, ParallelBranchId, Principal, PullRequestRecord, RunBlobId, + RunFailure, RunId, RunNoticeLevel, RunProvenance, SandboxProvider, StageId, SuccessReason, run_event as fabro_types, }; use fabro_agent::{AgentEvent, SandboxEvent}; use serde::{Deserialize, Serialize}; -use crate::error::Error; +use crate::error::{Error, run_failure_from_error}; use crate::outcome::{BilledModelUsage, FailureDetail, Outcome}; /// Events emitted during workflow run execution for observability. @@ -133,15 +133,16 @@ pub enum Event { billing: Option, }, WorkflowRunFailed { - error: Error, - duration_ms: u64, - reason: FailureReason, + failure: RunFailure, + duration_ms: u64, #[serde(default, skip_serializing_if = "Option::is_none")] - git_commit_sha: Option, + final_git_commit_sha: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - final_patch: Option, + final_patch: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - diff_summary: Option, + diff_summary: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + billing: Option, }, RunNotice { level: RunNoticeLevel, @@ -680,6 +681,26 @@ pub enum Event { } impl Event { + #[must_use] + pub fn workflow_run_failed_from_error( + error: &Error, + duration_ms: u64, + reason: FailureReason, + final_git_commit_sha: Option, + final_patch: Option, + diff_summary: Option, + billing: Option, + ) -> Self { + Self::WorkflowRunFailed { + failure: run_failure_from_error(error, reason), + duration_ms, + final_git_commit_sha, + final_patch, + diff_summary, + billing, + } + } + pub fn pull_request_created(record: &PullRequestRecord, draft: bool) -> Self { Self::PullRequestCreated { pr_url: record.html_url.clone(), @@ -781,11 +802,24 @@ impl Event { ); } Self::WorkflowRunFailed { - error, duration_ms, .. + failure, + duration_ms, + .. } => { + let tail = + fabro_types::ExecOutputTail::trace_summary(failure.exec_output_tail.as_ref()); error!( - error = %error, - causes = ?error.causes(), + message = %failure.message, + reason = %failure.reason, + category = %failure.category, + system_actor = ?failure.system_actor, + signature = ?failure.signature, + cause_count = failure.causes.len(), + exec_output_tail_present = tail.present, + exec_stdout_tail_bytes = tail.stdout_bytes, + exec_stderr_tail_bytes = tail.stderr_bytes, + exec_stdout_truncated = tail.stdout_truncated, + exec_stderr_truncated = tail.stderr_truncated, duration_ms, "Workflow run failed" ); diff --git a/lib/crates/fabro-workflow/src/git.rs b/lib/crates/fabro-workflow/src/git.rs index bd244a99f..6734ebe1c 100644 --- a/lib/crates/fabro-workflow/src/git.rs +++ b/lib/crates/fabro-workflow/src/git.rs @@ -46,7 +46,7 @@ pub fn ensure_clean(repo: &Path) -> Result<()> { let output = git_cmd(repo) .args(["status", "--porcelain"]) .output() - .map_err(|e| Error::engine_with_source("git status failed", &e))?; + .map_err(|e| Error::engine_with_source("git status failed", e))?; if !output.status.success() { return Err(git_error("not a git repository")); @@ -65,7 +65,7 @@ pub fn head_sha(repo: &Path) -> Result { let output = git_cmd(repo) .args(["rev-parse", "HEAD"]) .output() - .map_err(|e| Error::engine_with_source("git rev-parse failed", &e))?; + .map_err(|e| Error::engine_with_source("git rev-parse failed", e))?; if !output.status.success() { return Err(git_error("git rev-parse HEAD failed")); @@ -79,7 +79,7 @@ pub fn create_branch(repo: &Path, name: &str) -> Result<()> { let output = git_cmd(repo) .args(["branch", "--force", name, "HEAD"]) .output() - .map_err(|e| Error::engine_with_source("git branch failed", &e))?; + .map_err(|e| Error::engine_with_source("git branch failed", e))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); @@ -96,7 +96,7 @@ pub fn add_worktree(repo: &Path, path: &Path, branch: &str) -> Result<()> { .arg(path) .arg(branch) .output() - .map_err(|e| Error::engine_with_source("git worktree add failed", &e))?; + .map_err(|e| Error::engine_with_source("git worktree add failed", e))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); @@ -112,7 +112,7 @@ pub fn remove_worktree(repo: &Path, path: &Path) -> Result<()> { .args(["worktree", "remove", "--force"]) .arg(path) .output() - .map_err(|e| Error::engine_with_source("git worktree remove failed", &e))?; + .map_err(|e| Error::engine_with_source("git worktree remove failed", e))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); @@ -132,7 +132,7 @@ pub fn replace_worktree(repo: &Path, path: &Path, branch: &str) -> Result<()> { fn run_git_push(cmd: &mut Command) -> Result<()> { let output = cmd .output() - .map_err(|e| Error::engine_with_source("git push failed", &e))?; + .map_err(|e| Error::engine_with_source("git push failed", e))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); return Err(git_error(format!("git push failed: {stderr}"))); diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 376d45431..2f8b5abf1 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -286,7 +286,7 @@ impl Handler for AgentHandler { let run_id = context .run_id() .parse::() - .map_err(|err| Error::handler_with_source("invalid internal run_id", &err))?; + .map_err(|err| Error::handler_with_source("invalid internal run_id", err))?; let tool_hooks: Option> = services.run.hook_runner.as_ref().map(|hr| { Arc::new(fabro_hooks::WorkflowToolHookCallback { diff --git a/lib/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index 9916aaf82..c830bb8b6 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -105,7 +105,7 @@ impl Handler for CommandHandler { let env = services .env_for_stage() .await - .map_err(|err| Error::handler_with_anyhow("Failed to resolve stage env", &err))?; + .map_err(|err| Error::handler_with_anyhow("Failed to resolve stage env", err))?; let env_vars = if env.is_empty() { None } else { Some(&env) }; let cancel_token = services.run.cancel_token().child_token(); let stage_id = stage_scope.stage_id(); @@ -140,7 +140,7 @@ impl Handler for CommandHandler { Ok(streaming) => streaming, Err(err) => { recorder.discard().await?; - return Err(Error::handler_with_source("Failed to spawn script", &err)); + return Err(Error::handler_with_source("Failed to spawn script", err)); } }; let result = streaming.result; diff --git a/lib/crates/fabro-workflow/src/handler/llm/acp.rs b/lib/crates/fabro-workflow/src/handler/llm/acp.rs index 971a4c8ed..f3e27c4f1 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/acp.rs @@ -240,7 +240,7 @@ fn acp_command_error_to_workflow(error: AcpCommandError) -> Error { Error::handler("only stdio ACP commands are supported") } AcpCommandError::Parse(source) => { - Error::handler_with_source("Failed to resolve ACP command", &source) + Error::handler_with_source("Failed to resolve ACP command", source) } } } @@ -258,8 +258,8 @@ fn acp_error_to_workflow(error: AcpError) -> Error { AcpError::StopReason { stop_reason, text } => { Error::handler(format!("ACP prompt stopped with {stop_reason}: {text}")) } - AcpError::Sandbox(source) => Error::handler_with_source("ACP turn failed", &source), - other => Error::handler_with_source("ACP turn failed", &other), + AcpError::Sandbox(source) => Error::handler_with_source("ACP turn failed", source), + other => Error::handler_with_source("ACP turn failed", other), } } diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index 5270490f7..722b43b1b 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -348,7 +348,7 @@ impl AgentApiBackend { ) -> Result { let client = Client::from_source(source) .await - .map_err(|e| Error::handler_with_source("Failed to create LLM client", &e))?; + .map_err(|e| Error::handler_with_source("Failed to create LLM client", e))?; let mut profile = build_profile(model, provider); @@ -485,7 +485,7 @@ impl CodergenBackend for AgentApiBackend { let client = Client::from_source(self.source.as_ref()) .await - .map_err(|e| Error::handler_with_source("Failed to create LLM client", &e))?; + .map_err(|e| Error::handler_with_source("Failed to create LLM client", e))?; let model = node.model().unwrap_or(&self.model); let provider = node diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index 129583c83..e4e60ea27 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -96,7 +96,7 @@ async fn verify_cli_available( ) .await .map_err(|e| { - Error::handler_with_source(format!("Failed to check {cli_name} availability"), &e) + Error::handler_with_source(format!("Failed to check {cli_name} availability"), e) })?; if availability_check.is_success() { @@ -400,7 +400,7 @@ impl CodergenBackend for AgentCliBackend { sandbox .write_file(&prompt_path, prompt) .await - .map_err(|e| Error::handler_with_source("Failed to write prompt file", &e))?; + .map_err(|e| Error::handler_with_source("Failed to write prompt file", e))?; // 3. Build CLI command let model = node.model().unwrap_or(&self.model); @@ -451,7 +451,7 @@ impl CodergenBackend for AgentCliBackend { sandbox .write_file(&env_path, &env_lines.join("\n")) .await - .map_err(|e| Error::handler_with_source("Failed to write env file", &e))?; + .map_err(|e| Error::handler_with_source("Failed to write env file", e))?; // Disable auto-stop so the sandbox stays alive during long CLI runs. if let Err(e) = sandbox.set_autostop_interval(0).await { @@ -522,10 +522,7 @@ impl CodergenBackend for AgentCliBackend { Ok(streaming) => streaming, Err(err) => { cleanup_temp_files().await; - return Err(Error::handler_with_source( - "Failed to run CLI command", - &err, - )); + return Err(Error::handler_with_source("Failed to run CLI command", err)); } }; let result = streaming.result; diff --git a/lib/crates/fabro-workflow/src/handler/llm/launch_env.rs b/lib/crates/fabro-workflow/src/handler/llm/launch_env.rs index 134dea313..6f43da2a3 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/launch_env.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/launch_env.rs @@ -38,7 +38,7 @@ pub(crate) async fn resolve_agent_launch_env( .map_err(|err| { Error::handler_with_source( format!("Failed to resolve {} credential", request.stage_label), - &err, + err, ) })?; let ResolvedCredential::Cli(cli_credential) = resolved else { @@ -58,7 +58,7 @@ pub(crate) async fn resolve_agent_launch_env( .map_err(|err| { Error::handler_with_source( format!("{} credential login failed", request.stage_label), - &err, + err, ) })?; if !login_result.is_success() { @@ -97,7 +97,7 @@ pub(crate) async fn resolve_agent_launch_env( let tool_env = provider.resolve().await.map_err(|err| { Error::handler_with_anyhow( format!("Failed to resolve {} agent env", request.stage_label), - &err, + err, ) })?; launch_env.extend(tool_env); diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index 083688526..f67a8d37d 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -167,7 +167,7 @@ impl Handler for ParallelHandler { let run_id = context .run_id() .parse::() - .map_err(|err| Error::handler_with_source("invalid internal run_id", &err))?; + .map_err(|err| Error::handler_with_source("invalid internal run_id", err))?; let mut hook_ctx = HookContext::new(HookEvent::ParallelStart, run_id, graph.name.clone()); set_hook_node(&mut hook_ctx, node); @@ -200,7 +200,7 @@ impl Handler for ParallelHandler { match result { Ok(sha) => Some(sha), Err(e) if e.to_string() == "sandbox git unavailable" => { - return Err(Error::handler_with_source("sandbox git unavailable", &e)); + return Err(Error::handler_with_source("sandbox git unavailable", e)); } Err(e) => { tracing::warn!( @@ -277,7 +277,7 @@ impl Handler for ParallelHandler { wt_sandbox .initialize() .await - .map_err(|e| Error::handler_with_source("worktree setup failed", &e))?; + .map_err(|e| Error::handler_with_source("worktree setup failed", e))?; branch_context.set(keys::INTERNAL_WORK_DIR, serde_json::json!(&wt_path_str)); @@ -330,7 +330,7 @@ impl Handler for ParallelHandler { let _permit = sem .acquire() .await - .map_err(|e| Error::handler_with_source("semaphore error", &e))?; + .map_err(|e| Error::handler_with_source("semaphore error", e))?; parent_run.emitter.emit_scoped( &Event::ParallelBranchStarted { @@ -572,7 +572,7 @@ impl Handler for ParallelHandler { let run_id = context .run_id() .parse::() - .map_err(|err| Error::handler_with_source("invalid internal run_id", &err))?; + .map_err(|err| Error::handler_with_source("invalid internal run_id", err))?; let mut hook_ctx = HookContext::new(HookEvent::ParallelComplete, run_id, graph.name.clone()); set_hook_node(&mut hook_ctx, node); diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index a2a95c422..d5a6c9af1 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -1210,7 +1210,7 @@ mod tests { timestamp: chrono::Utc::now(), status: StageOutcome::Succeeded, duration_ms: 10, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: Vec::new(), billing: None, diff --git a/lib/crates/fabro-workflow/src/operations/archive.rs b/lib/crates/fabro-workflow/src/operations/archive.rs index 56c3394b7..8a6044f16 100644 --- a/lib/crates/fabro-workflow/src/operations/archive.rs +++ b/lib/crates/fabro-workflow/src/operations/archive.rs @@ -183,16 +183,18 @@ mod tests { event::append_event(&run_store, run_id, &Event::RunRunning) .await .unwrap(); - event::append_event(&run_store, run_id, &Event::WorkflowRunFailed { - error: crate::error::Error::engine("boom"), - duration_ms: 10, - reason: FailureReason::WorkflowError, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }) - .await - .unwrap(); + let failure_event = Event::workflow_run_failed_from_error( + &crate::error::Error::engine("boom"), + 10, + FailureReason::WorkflowError, + None, + None, + None, + None, + ); + event::append_event(&run_store, run_id, &failure_event) + .await + .unwrap(); } async fn seed_running(store: &Database, run_id: &RunId) { diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index a4febb472..1962a24ac 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -151,7 +151,7 @@ pub async fn create( ) }) .await - .map_err(|err| Error::engine_with_source("workflow create task failed", &err))??; + .map_err(|err| Error::engine_with_source("workflow create task failed", err))??; let workflow_config = resolved .workflow_toml_path diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index dc9b823c3..bff57dd32 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -260,16 +260,16 @@ async fn persist_terminal_engine_failure( RunStatus::Failed { reason } => reason, _ => FailureReason::WorkflowError, }; - if let Err(err) = append_event_to_sink(event_sink, &run_id, &Event::WorkflowRunFailed { - error: error.clone(), - duration_ms: crate::millis_u64(duration), + let failure_event = Event::workflow_run_failed_from_error( + error, + crate::millis_u64(duration), reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }) - .await - { + None, + None, + None, + None, + ); + if let Err(err) = append_event_to_sink(event_sink, &run_id, &failure_event).await { tracing::warn!(error = %err, "Failed to append terminal engine failure event"); } } @@ -747,6 +747,13 @@ impl RunSession { } } } + event if matches!(&event.body, EventBody::RunFailed(_)) => { + if let EventBody::RunFailed(props) = &event.body { + if let Some(sha) = props.final_git_commit_sha.as_ref() { + *sha_clone.lock().unwrap() = Some(sha.clone()); + } + } + } event if matches!(&event.body, EventBody::GitCommit(_)) => { if let EventBody::GitCommit(props) = &event.body { *sha_clone.lock().unwrap() = Some(props.sha.clone()); @@ -891,15 +898,16 @@ impl Drop for DetachedRunBootstrapGuard { let event_sink = self.event_sink.clone(); if let Ok(handle) = Handle::try_current() { handle.spawn(async move { - let _ = append_event_to_sink(&event_sink, &run_id, &Event::WorkflowRunFailed { - error: Error::engine(reason.to_string()), - duration_ms: 0, + let failure_event = Event::workflow_run_failed_from_error( + &Error::engine(reason.to_string()), + 0, reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }) - .await; + None, + None, + None, + None, + ); + let _ = append_event_to_sink(&event_sink, &run_id, &failure_event).await; }); } } @@ -957,15 +965,16 @@ impl Drop for DetachedRunCompletionGuard { let run_id = self.run_id; if let Ok(handle) = Handle::try_current() { handle.spawn(async move { - let _ = append_event_to_sink(&event_sink, &run_id, &Event::WorkflowRunFailed { - error: Error::engine(message.to_string()), - duration_ms: 0, + let failure_event = Event::workflow_run_failed_from_error( + &Error::engine(message.to_string()), + 0, reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }) - .await; + None, + None, + None, + None, + ); + let _ = append_event_to_sink(&event_sink, &run_id, &failure_event).await; let _ = append_event_to_sink(&event_sink, &run_id, &Event::RunNotice { level: RunNoticeLevel::Error, code: code.to_string(), @@ -988,16 +997,9 @@ async fn persist_detached_failure( ) -> Result<(), Error> { let message = error.to_string(); - if let Err(err) = append_event_to_sink(event_sink, &run_id, &Event::WorkflowRunFailed { - error: error.clone(), - duration_ms: 0, - reason, - git_commit_sha: None, - final_patch: None, - diff_summary: None, - }) - .await - { + let failure_event = + Event::workflow_run_failed_from_error(error, 0, reason, None, None, None, None); + if let Err(err) = append_event_to_sink(event_sink, &run_id, &failure_event).await { tracing::warn!(error = %err, "Failed to append detached failure event"); } @@ -1498,7 +1500,7 @@ mod tests { timestamp: Utc::now(), status: StageOutcome::Succeeded, duration_ms: 1, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: vec![], billing: None, diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 09dbbeb86..6ea3ff537 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -5,14 +5,14 @@ use std::time::Instant; use fabro_dump::RunDump; use fabro_hooks::{HookContext, HookEvent}; use fabro_types::run_event::{MetadataSnapshotFailureKind, MetadataSnapshotPhase}; -use fabro_types::{BilledTokenCounts, DiffSummary, EventBody, RunProjection}; +use fabro_types::{BilledTokenCounts, DiffSummary, EventBody, RunFailure, RunProjection}; use fabro_util::error::collect_causes; use fabro_util::time::elapsed_ms; use super::types::{Concluded, Executed, FinalizeOptions}; -use crate::error::Error; +use crate::error::{Error, run_failure_from_error, run_failure_from_outcome_failure}; use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; -use crate::outcome::{Outcome, OutcomeExt, StageOutcome}; +use crate::outcome::{Outcome, StageOutcome}; use crate::records::{Checkpoint, Conclusion, StageSummary}; use crate::run_metadata::MetadataSnapshot; use crate::run_options::RunOptions; @@ -24,11 +24,13 @@ use crate::{ProjectionBillingRollup, billing_rollup_from_projection}; pub fn classify_engine_result( engine_result: &Result, -) -> (StageOutcome, Option, RunStatus) { +) -> (StageOutcome, Option, RunStatus) { match engine_result { Ok(outcome) => { let status = outcome.status; - let failure_reason = outcome.failure_reason().map(String::from); + let failure = outcome.failure.as_ref().map(|failure| { + run_failure_from_outcome_failure(failure, FailureReason::WorkflowError) + }); let run_status = match status { StageOutcome::Succeeded | StageOutcome::Skipped => RunStatus::Succeeded { reason: SuccessReason::Completed, @@ -40,13 +42,16 @@ pub fn classify_engine_result( reason: FailureReason::WorkflowError, }, }; - (status, failure_reason, run_status) + (status, failure, run_status) } Err(Error::Cancelled) => ( StageOutcome::Failed { retry_requested: false, }, - Some("Cancelled".to_string()), + Some(run_failure_from_error( + &Error::Cancelled, + FailureReason::Cancelled, + )), RunStatus::Failed { reason: FailureReason::Cancelled, }, @@ -55,7 +60,7 @@ pub fn classify_engine_result( StageOutcome::Failed { retry_requested: false, }, - Some(err.display_with_causes()), + Some(run_failure_from_error(err, FailureReason::WorkflowError)), RunStatus::Failed { reason: FailureReason::WorkflowError, }, @@ -66,7 +71,7 @@ pub fn classify_engine_result( pub(crate) async fn build_conclusion_from_store( run_store: &RunStoreHandle, status: StageOutcome, - failure_reason: Option, + failure: Option, run_duration_ms: u64, final_git_commit_sha: Option, ) -> Conclusion { @@ -88,7 +93,7 @@ pub(crate) async fn build_conclusion_from_store( &projection_billing, &projection_order, status, - failure_reason, + failure, run_duration_ms, final_git_commit_sha, ) @@ -99,7 +104,7 @@ fn build_conclusion_from_parts( projection_billing: &ProjectionBillingRollup, projection_order: &HashMap, status: StageOutcome, - failure_reason: Option, + failure: Option, run_duration_ms: u64, final_git_commit_sha: Option, ) -> Conclusion { @@ -178,7 +183,7 @@ fn build_conclusion_from_parts( timestamp: chrono::Utc::now(), status, duration_ms: run_duration_ms, - failure_reason, + failure, final_git_commit_sha, stages, billing: projection_billing.billing_if_present(), @@ -445,17 +450,6 @@ pub(crate) fn build_terminal_event( diff_summary: Option, billing: Option, ) -> Event { - if matches!(outcome, Err(Error::Cancelled)) { - return Event::WorkflowRunFailed { - error: Error::Cancelled, - duration_ms, - reason: FailureReason::Cancelled, - git_commit_sha: final_git_commit_sha, - final_patch, - diff_summary, - }; - } - let outcome_status = outcome.as_ref().map_or( StageOutcome::Failed { retry_requested: false, @@ -483,21 +477,27 @@ pub(crate) fn build_terminal_event( }; } - let error = match outcome { - Err(err) => err.clone(), - Ok(o) => Error::engine( - o.failure - .as_ref() - .map_or_else(|| "run failed".to_string(), |f| f.message.clone()), - ), + let failure = match outcome { + Err(Error::Cancelled) => { + run_failure_from_error(&Error::Cancelled, FailureReason::Cancelled) + } + Err(err) => run_failure_from_error(err, FailureReason::WorkflowError), + Ok(outcome) => { + if let Some(failure) = outcome.failure.as_ref() { + run_failure_from_outcome_failure(failure, FailureReason::WorkflowError) + } else { + let fallback = Error::engine("run failed"); + run_failure_from_error(&fallback, FailureReason::WorkflowError) + } + } }; Event::WorkflowRunFailed { - error, + failure, duration_ms, - reason: FailureReason::WorkflowError, - git_commit_sha: final_git_commit_sha, + final_git_commit_sha, final_patch, diff_summary, + billing, } } @@ -1091,7 +1091,7 @@ mod tests { timestamp: chrono::Utc::now(), status: StageOutcome::Succeeded, duration_ms: 10, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: Vec::new(), billing: None, @@ -1154,7 +1154,7 @@ mod tests { timestamp: chrono::Utc::now(), status: StageOutcome::Succeeded, duration_ms: 10, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: Vec::new(), billing: None, @@ -1206,7 +1206,7 @@ mod tests { timestamp: chrono::Utc::now(), status: StageOutcome::Succeeded, duration_ms: 10, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: Vec::new(), billing: None, diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index bd80072cd..dd83aaae6 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -93,12 +93,12 @@ fn build_sandbox_env( }; let https_url = fabro_github::ssh_url_to_https(origin_url); let (owner, repo) = fabro_github::parse_github_owner_repo(&https_url) - .map_err(|err| Error::engine_with_anyhow("Failed to parse GitHub origin", &err))?; + .map_err(|err| Error::engine_with_anyhow("Failed to parse GitHub origin", err))?; let permissions = serde_json::to_value(permissions).map_err(|err| { - Error::engine_with_source("Failed to serialize GitHub permissions", &err) + Error::engine_with_source("Failed to serialize GitHub permissions", err) })?; let http = fabro_http::http_client() - .map_err(|err| Error::engine_with_source("Failed to build HTTP client", &err))?; + .map_err(|err| Error::engine_with_source("Failed to build HTTP client", err))?; let install_url = app.installation_url(&owner); let minter = AppIatMinter::new( app.clone(), @@ -242,7 +242,7 @@ async fn resolve_devcontainer(options: &mut InitOptions) -> Result<(), Error> { let config = fabro_devcontainer::DevcontainerResolver::resolve(&devcontainer.resolve_dir) .await - .map_err(|e| Error::engine_with_source("Failed to resolve devcontainer", &e))?; + .map_err(|e| Error::engine_with_source("Failed to resolve devcontainer", e))?; let lifecycle_command_count = config.on_create_commands.len() + config.post_create_commands.len() @@ -279,7 +279,7 @@ async fn resolve_devcontainer(options: &mut InitOptions) -> Result<(), Error> { .map_err(|e| { Error::engine_with_source( format!("Failed to execute devcontainer initializeCommand: {shell_command}"), - &e, + e, ) })?; @@ -410,7 +410,7 @@ pub async fn initialize( Some(Arc::clone(&sandbox_event_callback)), ) .await - .map_err(|err| Error::engine_with_anyhow("Failed to reconnect sandbox for resume", &err))?; + .map_err(|err| Error::engine_with_anyhow("Failed to reconnect sandbox for resume", err))?; sandbox_initialized = false; Arc::new(ReadBeforeWriteSandbox::new(Arc::from(sandbox))) } else { @@ -419,7 +419,7 @@ pub async fn initialize( .sandbox .build(Some(Arc::clone(&sandbox_event_callback))) .await - .map_err(|e| Error::engine_with_anyhow("Failed to build sandbox", &e))?, + .map_err(|e| Error::engine_with_anyhow("Failed to build sandbox", e))?, )) }; let cleanup_guard = (!attach_existing).then(|| { @@ -436,12 +436,12 @@ pub async fn initialize( sandbox .start() .await - .map_err(|e| Error::engine_with_source("Failed to start sandbox", &e))?; + .map_err(|e| Error::engine_with_source("Failed to start sandbox", e))?; } else { sandbox .initialize() .await - .map_err(|e| Error::engine_with_source("Failed to initialize sandbox", &e))?; + .map_err(|e| Error::engine_with_source("Failed to initialize sandbox", e))?; } let hook_ctx = HookContext::new( @@ -526,7 +526,7 @@ pub async fn initialize( sandbox_git .ensure_git_available(&*sandbox) .await - .map_err(|err| Error::engine_with_source("sandbox git unavailable", &err))?; + .map_err(|err| Error::engine_with_source("sandbox git unavailable", err))?; } match sandbox.setup_git(&intent).await { Ok(Some(info)) => { @@ -563,7 +563,7 @@ pub async fn initialize( } } Err(e) => { - return Err(Error::engine_with_source("Sandbox git setup failed", &e)); + return Err(Error::engine_with_source("Sandbox git setup failed", e)); } } } @@ -589,7 +589,7 @@ pub async fn initialize( Some(cancel_token.clone()), ) .await - .map_err(|e| Error::engine_with_source("Setup command failed", &e))?; + .map_err(|e| Error::engine_with_source("Setup command failed", e))?; if options.run_options.cancel_token.is_cancelled() { return Err(Error::Cancelled); } diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index bf504e728..1b53e0b89 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -845,7 +845,7 @@ mod tests { timestamp: Utc::now(), status: crate::outcome::StageOutcome::Succeeded, duration_ms: 150_000, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: vec![ StageSummary { diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 67ca26fd8..07bc6b80c 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -6997,7 +6997,7 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { timestamp: Utc::now(), status: StageOutcome::Succeeded, duration_ms: 1, - failure_reason: None, + failure: None, final_git_commit_sha: None, stages: Vec::new(), billing: None, diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index 714dd5cb1..b5ad4cda8 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -95,9 +95,11 @@ models/error-response-entry.ts models/error-response.ts models/event-envelope.ts models/event-seq.ts +models/exec-output-tail.ts models/execute-query-request.ts models/execute-query-response-rows-inner-inner.ts models/execute-query-response.ts +models/failure-category.ts models/failure-reason.ts models/features-namespace.ts models/file-checkpoint.ts @@ -251,6 +253,7 @@ models/run-diff.ts models/run-error.ts models/run-event.ts models/run-execution-settings.ts +models/run-failure.ts models/run-files-meta.ts models/run-git-settings.ts models/run-goal-file.ts diff --git a/lib/packages/fabro-api-client/src/api/models-api.ts b/lib/packages/fabro-api-client/src/api/models-api.ts index 8d28f601f..1f119b1d1 100644 --- a/lib/packages/fabro-api-client/src/api/models-api.ts +++ b/lib/packages/fabro-api-client/src/api/models-api.ts @@ -5,7 +5,7 @@ * HTTP API for managing Fabro workflow run executions. * * The version of the OpenAPI document: 0.1.0 - * + * * * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). * https://openapi-generator.tech diff --git a/lib/packages/fabro-api-client/src/models/conclusion.ts b/lib/packages/fabro-api-client/src/models/conclusion.ts index daf429e69..b25c1b32f 100644 --- a/lib/packages/fabro-api-client/src/models/conclusion.ts +++ b/lib/packages/fabro-api-client/src/models/conclusion.ts @@ -21,6 +21,9 @@ import type { BilledTokenCounts } from './billed-token-counts'; import type { RunDiff } from './run-diff'; // May contain unused imports in some cases // @ts-ignore +import type { RunFailure } from './run-failure'; +// May contain unused imports in some cases +// @ts-ignore import type { StageOutcome } from './stage-outcome'; // May contain unused imports in some cases // @ts-ignore @@ -33,7 +36,7 @@ export interface Conclusion { 'timestamp': string; 'status': StageOutcome; 'duration_ms': number; - 'failure_reason'?: string | null; + 'failure'?: RunFailure | null; 'final_git_commit_sha'?: string | null; 'stages': Array; 'billing'?: BilledTokenCounts | null; diff --git a/lib/packages/fabro-api-client/src/models/exec-output-tail.ts b/lib/packages/fabro-api-client/src/models/exec-output-tail.ts new file mode 100644 index 000000000..ea85dead1 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/exec-output-tail.ts @@ -0,0 +1,26 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +/** + * Redacted tail of command stdout/stderr captured for diagnostics. + */ +export interface ExecOutputTail { + 'stdout'?: string | null; + 'stderr'?: string | null; + 'stdout_truncated'?: boolean; + 'stderr_truncated'?: boolean; +} + diff --git a/lib/packages/fabro-api-client/src/models/failure-category.ts b/lib/packages/fabro-api-client/src/models/failure-category.ts new file mode 100644 index 000000000..cc4c38929 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/failure-category.ts @@ -0,0 +1,33 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +/** + * Product-level classification for grouping and retry policy. + */ + +export const FailureCategory = { + TRANSIENT_INFRA: 'transient_infra', + DETERMINISTIC: 'deterministic', + BUDGET_EXHAUSTED: 'budget_exhausted', + COMPILATION_LOOP: 'compilation_loop', + CANCELED: 'canceled', + STRUCTURAL: 'structural' +} as const; + +export type FailureCategory = typeof FailureCategory[keyof typeof FailureCategory]; + + + diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index fddfb31bc..27a1a8488 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -73,9 +73,11 @@ export * from './error-response'; export * from './error-response-entry'; export * from './event-envelope'; export * from './event-seq'; +export * from './exec-output-tail'; export * from './execute-query-request'; export * from './execute-query-response'; export * from './execute-query-response-rows-inner-inner'; +export * from './failure-category'; export * from './failure-reason'; export * from './features-namespace'; export * from './file-checkpoint'; @@ -229,6 +231,7 @@ export * from './run-diff'; export * from './run-error'; export * from './run-event'; export * from './run-execution-settings'; +export * from './run-failure'; export * from './run-files-meta'; export * from './run-git-settings'; export * from './run-goal'; diff --git a/lib/packages/fabro-api-client/src/models/model.ts b/lib/packages/fabro-api-client/src/models/model.ts index 37154fb75..4b03f415b 100644 --- a/lib/packages/fabro-api-client/src/models/model.ts +++ b/lib/packages/fabro-api-client/src/models/model.ts @@ -5,7 +5,7 @@ * HTTP API for managing Fabro workflow run executions. * * The version of the OpenAPI document: 0.1.0 - * + * * * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). * https://openapi-generator.tech @@ -67,9 +67,7 @@ export interface Model { */ 'default': boolean; /** - * Whether credential material is present for this model\'s provider on the server (vault entry or environment variable). Does NOT imply the credential is valid or that requests will succeed; call `POST /models/{id}/test` to verify usability. + * Whether credential material is present for this model\'s provider on the server (vault entry or environment variable). Does NOT imply the credential is valid or that requests will succeed; call `POST /models/{id}/test` to verify usability. */ 'configured': boolean; } - - diff --git a/lib/packages/fabro-api-client/src/models/run-failure.ts b/lib/packages/fabro-api-client/src/models/run-failure.ts new file mode 100644 index 000000000..dc70023d0 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/run-failure.ts @@ -0,0 +1,46 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + +// May contain unused imports in some cases +// @ts-ignore +import type { ExecOutputTail } from './exec-output-tail'; +// May contain unused imports in some cases +// @ts-ignore +import type { FailureCategory } from './failure-category'; +// May contain unused imports in some cases +// @ts-ignore +import type { FailureReason } from './failure-reason'; +// May contain unused imports in some cases +// @ts-ignore +import type { SystemActorKind } from './system-actor-kind'; + +/** + * Rich terminal run failure diagnostics. + */ +export interface RunFailure { + 'message': string; + 'causes'?: Array; + 'reason': FailureReason; + 'category': FailureCategory; + 'system_actor'?: SystemActorKind | null; + /** + * Stable normalized signature for grouping related failures. + */ + 'signature'?: string | null; + 'exec_output_tail'?: ExecOutputTail | null; +} + + +