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/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/Cargo.lock b/Cargo.lock index b655ac6c6..020d27985 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", @@ -1687,11 +1687,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", @@ -1707,7 +1707,7 @@ dependencies = [ [[package]] name = "fabro-cli" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -1808,7 +1808,7 @@ dependencies = [ [[package]] name = "fabro-client" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -1837,7 +1837,7 @@ dependencies = [ [[package]] name = "fabro-config" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -1865,7 +1865,7 @@ dependencies = [ [[package]] name = "fabro-core" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "async-trait", "fabro-types", @@ -1880,7 +1880,7 @@ dependencies = [ [[package]] name = "fabro-dev" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -1899,7 +1899,7 @@ dependencies = [ [[package]] name = "fabro-devcontainer" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-http", "fabro-static", @@ -1916,7 +1916,7 @@ dependencies = [ [[package]] name = "fabro-dump" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -1930,7 +1930,7 @@ dependencies = [ [[package]] name = "fabro-github" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -1952,7 +1952,7 @@ dependencies = [ [[package]] name = "fabro-graphviz" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-types", @@ -1966,7 +1966,7 @@ dependencies = [ [[package]] name = "fabro-hooks" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "async-trait", "fabro-agent", @@ -1990,7 +1990,7 @@ dependencies = [ [[package]] name = "fabro-http" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-static", "http", @@ -2000,7 +2000,7 @@ dependencies = [ [[package]] name = "fabro-install" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -2015,7 +2015,7 @@ dependencies = [ [[package]] name = "fabro-interview" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "async-trait", "dialoguer", @@ -2030,7 +2030,7 @@ dependencies = [ [[package]] name = "fabro-llm" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2063,7 +2063,7 @@ dependencies = [ [[package]] name = "fabro-macros" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "clap", "fabro-options-metadata", @@ -2074,7 +2074,7 @@ dependencies = [ [[package]] name = "fabro-manifest" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-api", @@ -2092,7 +2092,7 @@ dependencies = [ [[package]] name = "fabro-mcp" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-config", @@ -2108,7 +2108,7 @@ dependencies = [ [[package]] name = "fabro-mcp-server" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2131,7 +2131,7 @@ dependencies = [ [[package]] name = "fabro-model" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-static", "insta", @@ -2145,7 +2145,7 @@ dependencies = [ [[package]] name = "fabro-oauth" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "axum", @@ -2167,7 +2167,7 @@ dependencies = [ [[package]] name = "fabro-options-metadata" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "serde", "serde_json", @@ -2175,7 +2175,7 @@ dependencies = [ [[package]] name = "fabro-proc" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "cc", "libc", @@ -2184,7 +2184,7 @@ dependencies = [ [[package]] name = "fabro-redact" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "aho-corasick", "ref-cast", @@ -2200,7 +2200,7 @@ dependencies = [ [[package]] name = "fabro-sandbox" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2243,7 +2243,7 @@ dependencies = [ [[package]] name = "fabro-server" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2325,7 +2325,7 @@ dependencies = [ [[package]] name = "fabro-slack" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-http", "fabro-interview", @@ -2346,18 +2346,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", @@ -2384,7 +2384,7 @@ dependencies = [ [[package]] name = "fabro-telemetry" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -2410,7 +2410,7 @@ dependencies = [ [[package]] name = "fabro-template" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "fabro-util", @@ -2422,7 +2422,7 @@ dependencies = [ [[package]] name = "fabro-test" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "assert_cmd", "axum", @@ -2445,7 +2445,7 @@ dependencies = [ [[package]] name = "fabro-tracker" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2459,7 +2459,7 @@ dependencies = [ [[package]] name = "fabro-types" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "chrono", "clap", @@ -2480,7 +2480,7 @@ dependencies = [ [[package]] name = "fabro-util" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "console 0.15.11", @@ -2500,7 +2500,7 @@ dependencies = [ [[package]] name = "fabro-validate" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "fabro-graphviz", "fabro-model", @@ -2512,7 +2512,7 @@ dependencies = [ [[package]] name = "fabro-vault" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "chrono", "fabro-types", @@ -2524,7 +2524,7 @@ dependencies = [ [[package]] name = "fabro-workflow" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -4769,15 +4769,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", ] @@ -4816,9 +4815,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", @@ -5741,9 +5740,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", @@ -5768,9 +5767,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", @@ -7302,7 +7301,7 @@ dependencies = [ [[package]] name = "twin-github" -version = "0.231.0-nightly.3" +version = "0.232.0-nightly.0" dependencies = [ "axum", "base64", @@ -7321,7 +7320,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..e3186498d 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] @@ -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" 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. --- 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 0879a39b8..c785223ea 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -4919,6 +4919,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 @@ -6417,8 +6478,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/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-api/build.rs b/lib/crates/fabro-api/build.rs index e36ff6fba..1ecf7aaa1 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", &[]), @@ -358,6 +362,7 @@ fn main() { ("BilledTokenCounts", "fabro_types::BilledTokenCounts", &[]), ("BillingModelRef", "fabro_model::ModelRef", &[]), ("BillingSpeed", "fabro_model::Speed", &[]), + ("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 a69d6a4f7..c72597ac5 100644 --- a/lib/crates/fabro-api/src/lib.rs +++ b/lib/crates/fabro-api/src/lib.rs @@ -32,15 +32,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 985becf26..3f4af9cee 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -516,7 +516,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 @@ -659,8 +659,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; @@ -785,25 +785,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-config/src/builders.rs b/lib/crates/fabro-config/src/builders.rs index 25c5ad81f..8f19d0792 100644 --- a/lib/crates/fabro-config/src/builders.rs +++ b/lib/crates/fabro-config/src/builders.rs @@ -463,6 +463,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-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-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(); 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_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/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index a600234d4..c28a436e5 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 @@ -417,6 +426,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, @@ -1413,6 +1450,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( diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index b2e2c9bd1..40f7309ea 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 @@ -923,7 +924,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, @@ -2080,19 +2088,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; } @@ -2134,19 +2133,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(()) @@ -2214,19 +2204,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) { @@ -2253,19 +2240,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"); } @@ -2425,9 +2410,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(); } @@ -2538,21 +2523,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 +3177,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 +3198,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 +3226,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 +3245,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 +3275,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 +3360,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 8a946eed2..fc496eaf4 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -2487,7 +2487,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-slack/src/blocks.rs b/lib/crates/fabro-slack/src/blocks.rs index 7ebc92be9..83a94543b 100644 --- a/lib/crates/fabro-slack/src/blocks.rs +++ b/lib/crates/fabro-slack/src/blocks.rs @@ -1,14 +1,45 @@ +use std::fmt::Write as _; + use fabro_interview::Question; use fabro_types::QuestionType; 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"; +/// 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 +/// 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", @@ -31,37 +62,135 @@ 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 { 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] + })); } QuestionType::MultipleChoice => { let elements: Vec = question .options .iter() - .map(|opt| { + .enumerate() + .map(|(idx, opt)| { button( &opt.label, &encode_action_value(&SlackActionPayload::Selected { @@ -69,15 +198,14 @@ 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(); - let actions = json!({ + blocks.push(json!({ "type": "actions", - "elements": elements - }); - vec![section, actions] + "elements": elements, + })); } QuestionType::MultiSelect => { let options: Vec = question @@ -90,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": [{ @@ -98,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 { @@ -107,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)] @@ -128,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]; @@ -151,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]; @@ -178,14 +305,30 @@ 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]; 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() @@ -199,30 +342,201 @@ 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]; 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\"")); } + #[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"); @@ -260,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 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}.") + ); + } } 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 0d22fd66a..403411a6c 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, @@ -1524,44 +1523,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 115a371cf..84a8ce966 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -243,7 +243,7 @@ fn parse_reasoning_effort(node: &Node, value: &str) -> Result Result { "Invalid speed \"{value}\" for node \"{}\"; expected one of: standard, fast", node.id ), - &source, + source, ) }) } @@ -570,7 +570,7 @@ impl AgentApiBackend { effective_request_controls(catalog.as_ref(), run_model_controls, model, node)?; let client = Client::from_source_with_catalog(source, Arc::clone(&catalog)) .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, @@ -715,7 +715,7 @@ impl CodergenBackend for AgentApiBackend { let client = Client::from_source_with_catalog(self.source.as_ref(), Arc::clone(&self.catalog)) .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 = self.resolve_provider_context(model, node.provider())?; diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index a510711ed..aafac6820 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -99,7 +99,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() { @@ -428,7 +428,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); @@ -485,7 +485,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 { @@ -556,10 +556,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 c6862c4b0..b9182ced2 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -1216,7 +1216,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 f2eedc5fc..e94e9ba8e 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -169,7 +169,7 @@ pub async fn create_with_catalog( ) }) .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 e5ae4e733..5425c0429 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -262,16 +262,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"); } } @@ -796,6 +796,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()); @@ -941,15 +948,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; }); } } @@ -1007,15 +1015,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(), @@ -1038,16 +1047,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"); } @@ -1553,7 +1555,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 7d28954f5..867295264 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, } } @@ -1101,7 +1101,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, @@ -1164,7 +1164,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, @@ -1216,7 +1216,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 3b9250644..9207f8601 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -94,12 +94,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(), @@ -254,7 +254,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() @@ -291,7 +291,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, ) })?; @@ -424,7 +424,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 { @@ -433,7 +433,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(|| { @@ -450,12 +450,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( @@ -541,7 +541,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)) => { @@ -578,7 +578,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)); } } } @@ -604,7 +604,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 d55ee6a9d..6d792ec1f 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -860,7 +860,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 4fcb20b76..03849f859 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -7014,7 +7014,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 cdceffd1b..b99bcfd58 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 @@ -252,6 +254,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/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 bc83aad0d..f4b7cd730 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'; @@ -230,6 +232,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/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; +} + + +