Make publish failures terminal

[brynary]

Summary

• Add an always-present publish phase between conclude and finalize.
• Make final push, remote branch verification, and configured PR creation failures terminal with the publish_failed reason.
• Require the GitHub run branch head to equal the final commit SHA before opening a PR.
• Request workflows: write for repository-scoped GitHub App push tokens and record the verified head SHA in PR events.
• Stop reporting a pushed branch unless a remote is configured and the final push succeeds.

This prevents a run from finishing green and opening a PR from a stale checkpoint when GitHub rejects the implementation push.

Testing

• cargo nextest run -p fabro-workflow -p fabro-github: 1,301 passed
• cargo nextest run -p fabro-server: 771 passed
• cargo nextest run -p fabro-types -p fabro-api -p fabro-store -p fabro-cli: 1,756 passed
• cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings
• cargo +nightly-2026-04-14 fmt --check --all
• cargo build --workspace
• API client TypeScript typecheck

[brynary]

Reviewer notes:

• Checkpoint push failures remain warnings so transient failures can recover; the final push in publish is authoritative and can fail the run.
• The remote-head check runs immediately before the PR API call and also applies to manually created run PRs.
• pull_request.created.head_sha is optional when reading older events.
• Auto-merge setup remains warning-only because the PR already exists at that point.
• Existing GitHub App installations must grant and approve Workflows: write before workflow-file pushes will succeed.

[Copilot]

Pull request overview

This PR restructures the fabro-workflow pipeline to introduce a dedicated, always-present publish phase (between conclude and finalize) and makes publish-related failures (final push, remote head verification, PR creation) produce a terminal run failure (publish_failed). It also extends PR-created events to record the verified remote head SHA and updates GitHub App token permissions to support workflow-file writes.

Changes:

• Add a new publish pipeline phase that re-pushes the final run branch and (optionally) opens a PR only after verifying the remote branch head SHA.
• Introduce FailureReason::PublishFailed across Rust, OpenAPI, and the TS client, and classify publish errors as terminal.
• Extend pull request created event payloads with head_sha, and request workflows: write permissions for GitHub App tokens.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
lib/packages/fabro-api-client/src/models/failure-reason.ts	Add publish_failed to the generated TS failure-reason tokens.
lib/foundation/fabro-types/src/status.rs	Add FailureReason::PublishFailed to shared Rust status vocabulary.
lib/foundation/fabro-types/src/run_event/misc.rs	Add optional head_sha to PR-created event props.
lib/foundation/fabro-api/tests/status_round_trip.rs	Assert OpenAPI/Rust JSON token parity for publish_failed.
lib/components/fabro-workflow/src/pipeline/types.rs	Introduce PublishOutcome/Published and rename PR options to publish options.
lib/components/fabro-workflow/src/pipeline/pull_request.rs	Require remote head SHA verification before creating a PR; plumb expected head SHA through request/response types.
lib/components/fabro-workflow/src/pipeline/publish.rs	New publish phase: final push + optional PR creation; emit events and convert failures into publish errors.
lib/components/fabro-workflow/src/pipeline/mod.rs	Wire the new conclude + publish steps into pipeline exports.
lib/components/fabro-workflow/src/pipeline/finalize.rs	Split finalize into conclude() (collect diff/result) and finalize() (persist + emit terminal), and map publish errors to publish_failed.
lib/components/fabro-workflow/src/operations/start.rs	Update run lifecycle sequencing to initialize -> execute -> conclude -> publish -> finalize.
lib/components/fabro-workflow/src/event/events.rs	Add head_sha to internal PullRequestCreated event and constructor.
lib/components/fabro-workflow/src/event/convert.rs	Convert internal head_sha string into optional API field.
lib/components/fabro-workflow/src/error.rs	Add Error::Publish plus helpers, retryability classification, and failure-detail support.
lib/components/fabro-workflow/README.md	Update docs to reflect the new pipeline phase ordering.
lib/components/fabro-store/src/run_state.rs	Update tests/fixtures to include persisted PR head_sha.
lib/components/fabro-github/src/lib.rs	Request workflows: write for push tokens and add branch_head_sha() helper for remote head verification.
lib/apps/fabro-server/src/server/tests.rs	Update tests to account for PR head SHA verification and storage.
lib/apps/fabro-server/src/server/handler/pull_requests.rs	Require final_git_commit_sha and enforce remote head SHA verification before server-side PR creation.
lib/apps/fabro-server/src/server.rs	Treat publish failures distinctly and map them to FailureReason::PublishFailed.
lib/apps/fabro-server/src/install.rs	Add workflows: write to the GitHub App manifest and assert it in tests.
lib/apps/fabro-server/src/demo/mod.rs	Add parsing support for the new publish_failed failure reason.
lib/apps/fabro-cli/tests/it/cmd/pr_view.rs	Update CLI integration test fixtures for PR head_sha.
lib/apps/fabro-cli/src/commands/run/run_progress/mod.rs	Update CLI progress rendering tests for PR head_sha.
lib/apps/fabro-cli/src/commands/run/run_progress/event.rs	Update event parsing tests for PR head_sha.
docs/public/integrations/github.mdx	Document workflows: write permission and the new publish-stage behavior/guarantees.
docs/public/api-reference/fabro-api.yaml	Add publish_failed to the API failure reason enum.
docs/internal/events.md	Document head_sha in legacy PR-linked event properties.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[brynary]

Ran a simplification pass over this branch (three review agents: reuse, quality, efficiency). Pushed as 73f48ee — net -124 lines, all 7407 tests pass, clippy clean.

One real bug fixed: if the final push succeeded but PR creation then failed, Finalized.pushed_branch came back None, so the CLI stopped printing the branch on exactly the run where you need it. PublishOutcome now records what actually happened and carries the error separately instead of hiding both behind a Result.

Duplication this branch introduced, collapsed:

• Error::{Engine,Publish,Handler} → one Error::Stage with an ErrorStage discriminator. They shared a field shape and had to be edited together in four match groups; nine near-identical constructors became two helpers.
• Added Error::failure_reason(), replacing the same error→FailureReason mapping written out in four places (including two byte-identical blocks in server.rs).
• Three near-identical precondition guards in publish.rs → one publish_target().
• Centralized the run-branch refspec in lifecycle::git::push_run_branch. The two emitters disagreed on Event::GitPush.branch — lifecycle sent a full refspec, the new code sent a bare name. Docs say branch name, so lifecycle was fixed.

Dead code removed:

• branch_exists / branch_exists_with_client had zero callers and duplicated the new branch_head_sha (which also correctly asks for contents: read instead of write). Gave branch_head_sha the _with_client split its siblings have and ported the tests to MockHttpClient.
• maybe_open_pull_request's Ok(None) path was unreachable — both callers already reject empty diffs. Now open_pull_request returns the record directly.
• CreatedPullRequest.head_sha echoed back its own input.
• The copy-pasted credential match in resolve_clone_credentials.

Other: PullRequestCreated.head_sha is Option<String> rather than using "" to mean absent; publish moved onto Concluded as methods.

One behavior change worth a look: is_retryable() now returns false for publish errors. Publish runs once after execution, so nothing could ever act on the transient-infra classification — it was unreachable and read against the PR's own title. The category is still classified for reporting. I updated the test to match.

Left alone, flagging for you:

• conclude() widens final_git_commit_sha to fall back to git.base_sha. That's a durable, API-exposed field, so a run that committed nothing now reports the base SHA as its final SHA with no way for consumers to tell. Looks deliberate, but it is a real semantic widening.
• RunNoticeCode::PullRequestFailed now has no emitter. I left the variant since removing it would break deserialization of already-persisted run.notice events.
• open_pull_request still returns Result<_, String>, and those strings become the terminal failure detail via anyhow!(String). A typed error there would fix the rendered-chain-in-API-response issue too — bigger than this pass.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

lib/components/fabro-workflow/src/pipeline/finalize.rs:552

• final_git_commit_sha is being derived from git.base_sha when last_git_sha is missing. base_sha is the diff base, not the final commit, and using it here can cause publish/PR remote-head verification to compare against the wrong SHA.

    let final_git_commit_sha = options.last_git_sha.clone().or_else(|| {
        run_options
            .git
            .as_ref()
            .and_then(|git| git.base_sha.clone())
    });

lib/components/fabro-workflow/src/pipeline/pull_request.rs:501

• Remote branch head verification happens after build_pr_content(...) (which can invoke the LLM). When the branch is stale/missing, this pays LLM latency/cost before failing. Consider verifying the remote head first, then building PR content once the branch is confirmed.

    let remote_head = github_app::branch_head_sha(&req.github, &owner, &repo, req.head_branch)
        .await
        .map_err(|err| format!("failed to verify remote branch head: {err:#}"))?;

[brynary]

Follow-up from the efficiency review — pushed as fa6f7e5. Three fixes:

1. The branch check now runs before the LLM call. open_pull_request generated the PR title and body first and only verified the remote head afterwards, so every stale-branch failure cost a full content generation before failing. The verification is the cheap check, so it goes first.

2. The branch read tolerates GitHub replica lag. GET /repos/{owner}/{repo}/branches/{branch} is replica-served and can briefly report the previous commit — or 404 for a branch that is new on the remote — right after the push publish just made. It was read once, with no retry. Since publish failure is now terminal, a replica that hadn't caught up would discard a fully successful run. Now up to 3 attempts, 500ms apart.

These two land together deliberately: the LLM call was the only thing buying slack against that race, so reordering without the retry would have made it more likely.

3. Commit SHAs no longer contaminate failure classification. classify_failure_reason substring-matches bare 500/502/503/504 as transient-infra hints, and both publish messages embed a commit SHA — a 40-char hex string hits one of those a few percent of the time, so a deterministic failure could be labeled transient. Long hex runs are masked before matching; the 3-digit status codes are too short to be affected. The hex regex is now shared with normalize_failure_reason, which already had its own copy.

Test coverage: the stale-branch test now asserts the branch was read 3 times and that neither the LLM nor PR creation was called. I did not add a test for "recovers on attempt 2" — it needs timing-dependent mock swapping, which seemed a worse trade than the call-count assertion.

---

Unrelated, but it blocked verification — .config/nextest.toml has an ordering bug. nextest applies the first matching override, so:

filter = "package(fabro-server)"                                    # 5s x 4 = 20s
filter = "package(fabro-server) & test(all_spec_routes_are_routable)"  # 15s x 4 = 60s

the general rule shadows the specific one, and all_spec_routes_are_routable gets a 20s cap instead of the intended 60s. It takes ~16s standalone and times out under any real machine load. Moving the specific override above the general one fixes it — I couldn't edit that path from my sandbox. (Verified the test itself passes via cargo test: 16.32s, ok.)

Still open from the review, not addressed here:

• The metadata branch commit now carries the whole run diff (conclude populates conclusion.diff.patch before write_finalize_commit clones the conclusion into the projection). Megabytes per run on a large refactor. Your call whether that belongs in run.json.
• Publish mints ~4 separate installation tokens and builds a fresh reqwest client for each. Sharing one client via GitHubContext::with_http_client is safe; collapsing the mints would widen the token scope, so that wants a deliberate decision.
• The push itself and create_pull_request are still single-shot — same terminal-failure exposure as Plan: Improve fabro ps output #2, just less likely.
• Config-shaped failures (no base branch, no credentials, run_branch.push = false with PRs enabled) are all knowable before execute but reported after the agent has done its work.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 33 out of 34 changed files in this pull request and generated no new comments.