- Return the existing PullRequestRecord on 409 from POST /runs/{id}/pull_request
so structured clients can recover the URL/number without a follow-up call.
The response now includes both the error envelope and a pull_request field
(same shape precedent as /install/finish's leftover_env_keys).
- Add server tests for merge/close error paths: 404 no_stored_record, 400
unsupported_host, 503 integration_unavailable, 400 invalid_merge_method, 502
github_not_found.
- Add a dedicated regression test proving the PR handlers use the
github_api_base_url captured at AppState construction, not a request-time
env read (SSRF defense invariant from the plan).
- Add an upgrade hint on unstructured 404s from the new PR client methods so
a new CLI against an old server sees "Upgrade the fabro server" instead of
an opaque failure.
- Refresh the stale CLI docs paragraph so it describes server-side GitHub
credentials, matching the post-refactor reality.
- Regenerate the TypeScript API client (had fallen behind the prior OpenAPI
schema additions) and add PullRequestRecord to ErrorResponse as an optional
field for the 409 case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The view, merge, and close handlers each repeated the same ~25-line
prologue: open the run reader, load state, unwrap the stored record,
parse owner/repo with the host check, and load GitHub creds. Move it
into `load_pull_request_github_context` so each handler keeps only the
work that's unique to it. Net -31 lines with no behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After Unit 3.1 of the config boundary refactor, fabro-types no longer has
any #[derive(Combine)] sites — the fabro-macros dep is unused. Likewise
`Duration as DurationLayer` was an artifact from when layer and vocabulary
types lived side-by-side; the resolved Duration type has no Layer form now,
so the alias was misleading.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Return `&'static str` from the mock provider's `name()` and drop
redundant `.to_string()` calls on `github.base_url()` (already owned).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FINALIZE loaded the run event log twice: once via build_conclusion_from_store
for stage durations, then again to count ArtifactCaptured events. Merged into
a single walk feeding both the conclusion and the artifact count.
Collapsed six near-identical pipeline::execute + emit_terminal + flush blocks
in test_support into one execute_and_emit_terminal helper. Also trimmed
narrative comments that described caller ordering, control flow, or the fix
commit rather than non-obvious invariants.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-ups to the FINALIZE terminal-event refactor, surfaced during
review:
- build_terminal_event: drop re-wrapping Err outcomes in Error::engine,
which doubled the "Engine error: " prefix on display. Surface the
original error directly.
- Unify loop billing: move billing aggregation into a shared
billing_from_checkpoint helper iterating node_outcomes.values() once
per unique node. Both Conclusion.billing and the emitted terminal
event use it, so the persisted metadata snapshot and the run.completed
event can't disagree.
- Dedupe conclusion.stages by node id while preserving execution order.
completed_nodes has duplicates for looping workflows, but
node_outcomes, node_retries, and stage_durations are all keyed by
node_id with overwrite semantics, so duplicate StageSummary rows
carried identical latest-visit values and inflated total_retries /
the PR Fabro Details table.
- test_support: flush StoreProgressLogger before reading state.
StoreProgressLogger forwards events via mpsc, so state() right after
execute could miss StageCompleted entries and return stale billing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`fabro dump` had a `DumpDataSource` trait with two impls: `ServerDumpSource`
(production, goes through the HTTP client) and a `#[cfg(test)] LocalDumpSource`
that constructed a `fabro_store::{Database, ArtifactStore}` in-process and
replayed hand-written events into it. The trait existed solely to let tests
bypass the server boundary, which meant the production path was never
exercised by unit tests and every storage-layer refactor leaked up into CLI
test fixtures.
Delete the trait, both impls, the `export_run(&RunDatabase, &ArtifactStore, …)`
test-only helper, and the 500-line inline event-replay test. The single
remaining path calls `Client::{list_run_events, read_run_blob,
list_run_artifacts, download_stage_artifact}` directly. End-to-end coverage
lives in `tests/it/cmd/dump.rs` (real server, real runs), and pure layout
logic is covered by `fabro_workflow::run_dump::tests` — both of which match
the project's testing-strategy.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The headless Chrome screenshot test polled for the screenshot file every
100 ms for 20 seconds, then panicked. Slow CI runners (Ubuntu 24.04 GHA)
sometimes took longer than the polling deadline, surfacing as a flake.
Chrome with `--screenshot` exits when the file is written, so waiting on
the process is the deterministic completion signal — no polling, no
arbitrary deadline that might be too short.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `WorkflowRunCompleted` / `WorkflowRunFailed` event was emitted from
`EventLifecycle::on_run_end`, a callback the executor fires at the end of
the EXECUTE phase. But the run isn't done at that point — RETRO and
FINALIZE still need to run, and FINALIZE writes the meta branch's finalize
commit. Observers that treat the event as "done" (CLI attach, daemon SSE
consumers) could observe terminal state and act on it before the worker
flushed its remaining writes.
The recovery scenario test exposed this: it deletes the meta branch
right after `fabro run` returns, then asserts the branch is empty. On
loaded CI runners the worker's finalize commit landed after the delete,
recreating the branch and failing the assertion.
Move the terminal event emission to `pipeline::finalize::finalize`, after
`write_finalize_commit`. The lifecycle's `on_run_end` overrides for event
and git become empty (deleted — the trait already provides a no-op
default). Three pieces of cross-cutting state (`final_patch`,
`captured_artifact_count`, the dead `EventLifecycle` reads of
`last_git_sha`) only existed to ferry data from EXECUTE to the terminal
event; deleted those too. The aggregator collapses to a one-line
delegate to `hook.on_run_end`.
`write_finalize_commit` now takes the conclusion as a parameter and
injects it into the projection copy, since the terminal event hasn't run
through the run store yet when the meta branch is written.
`build_terminal_event` is `pub(crate)` so `test_support` helpers (which
stop at EXECUTE) can mirror the production payload.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
build_manifest_git() was called with the CLI's cwd, which detects the
wrong repo/branch when fabro is invoked from a workspace directory that
differs from the target repo (e.g. via `[run] working_dir = "repos/foo"`
in .fabro/project.toml). Now resolve working_directory once in
build_run_manifest, share it with resolve_manifest_goal (dropping the
duplicate resolution), and pass it to build_manifest_git.
Also rename the build_manifest_git parameter from `cwd` to `repo_path`
to reflect that it now receives the resolved working directory.
Add a regression test that spins up a workspace git repo and a
separate target git repo beneath it, points `[run] working_dir` at the
target, and asserts the manifest's git branch and origin come from the
target repo.
Ports https://github.com/durandom/fabro/pull/2 to the post-v2-schema
code (Settings -> SettingsLayer).
Closes#159
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
