Replace hand-built 409 response that inlined {errors,pull_request} with
a regular ApiError::with_code(CONFLICT, ..., "pull_request_exists") and
drop the optional pull_request field that had been added to the
ErrorResponse OpenAPI schema solely to carry the existing record.
Clients receiving a 409 can GET /runs/{id}/pull_request to retrieve the
stored record when they need it — the detail string still includes the
existing html_url, which is the field most clients branch on.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `sleep_inhibitor` feature pulled in 20 pedantic/nightly lints that
CI (default features) never exercised. Narrow all `pub` items in the
module to `pub(crate)`/`pub(super)`, replace the `use
super::iokit_bindings::*` wildcard with explicit imports, use `&raw
mut` for FFI pointer borrows, drop the always-`Some` wrapping in
`DummySleepInhibitor::acquire`, and bring `crate::sleep_inhibitor`
into scope at the three call sites so they don't trip
`clippy::absolute_paths`.
Verified: `cargo +nightly-2026-04-14 clippy --workspace --all-targets
--all-features -- -D warnings` clean, `cargo nextest run --workspace
--all-features` 4563 tests passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
run_retro fetched list_events twice — once for stage_durations and
again for run_retro_agent's payload. Load once at the top and reuse.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Command::Parallel entries were previously flattened into the same
sequential for-loop as Shell/Args, defeating the devcontainer spec's
parallel-safe guarantee. Extract a run_shell helper and dispatch on
Command kind: Shell/Args await one command, Parallel uses try_join_all.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
compute_final_patch (up to 30s git diff) and write_finalize_commit
(network push to meta branch) are independent — run via tokio::join!
so worst-case wall time is max(diff, push) instead of their sum.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverses the #[cfg(test)] gating on RunServices::with_run_store /
with_emitter / with_sandbox / with_cancel_requested — manager_loop and
parallel handlers have production callers that were unpacking 7 fields
into locals just to reconstruct RunServices::new(...).
manager_loop builds its child via
parent_run.with_run_store(...).with_cancel_requested(None).
parallel builds each branch via parent_run.with_sandbox(...).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce PullRequestApiError with a structured NotFound variant and an
Other(String) catch-all for non-classified failures. Update
get_pull_request, merge_pull_request, and close_pull_request to return
the new type so callers can branch on shape rather than substring.
Server PR handlers now match Err(PullRequestApiError::NotFound { .. })
to map a missing GitHub PR to the existing github_pull_request_not_found
ApiError, removing three err.contains("not found") substring checks.
The Display impl for NotFound preserves the prior message format
("Pull request #N not found in owner/repo") so logging and the
catch-all BAD_GATEWAY response keep their human-readable text.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SettingsLayer::{test_default, ensure_test_auth_methods} are `pub(crate)`
and only called from fabro-config's own in-crate tests, but their impl
block was gated on `cfg(any(test, feature = "test-support"))`. No
external crate enabled the `test-support` feature, so under
`--all-features` the methods compiled in without reachable callers and
clippy flagged them as dead code. Narrow the gate to `cfg(test)` and
drop the vestigial feature entry.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reconciles origin's "emit terminal event from FINALIZE" refactor
(41c47dbe1, e8a89ac39, 904c8842f) with the local RunServices refactor.
finalize() now performs origin's single list_events walk for stage
durations + artifact count, origin's compute_final_patch, deduped
stages/billing via billing_from_checkpoint, and origin's terminal event
emission — but reads run_store/sandbox/emitter from the shared
RunServices instead of individual Retroed fields. services.emitter.notice
replaces origin's local emit_run_notice helper.
test_support's execute_and_emit_terminal (added by origin) now accesses
run_store/emitter via executed.engine.run.* since Executed bundles
EngineServices. execute/tests.rs drops the terminal-event status
assertion origin deleted — status is no longer set at EXECUTE end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move PullRequestDetail, PullRequestGithubDetail, PullRequestUser,
PullRequestRef, and MergeMethod into fabro-types. Register them as
fabro-api with_replacement targets so the OpenAPI client and the server
share one canonical type per concept.
PullRequestDetail composes a stored PullRequestRecord with a flattened
PullRequestGithubDetail mirroring GitHub's REST payload, removing the
hand-rolled pull_request_detail_json builder in the server. Change the
PullRequestRef wire field from `ref_name` to `ref` so the same Rust
type round-trips through both GitHub and our API without aliases.
The server now uses fabro_api::types::{Create,Merge,Close}* directly,
deleting the hand-defined request/response shadows and the
`body.method.parse::<...>()` call (the typed MergeMethod enum drives
deserialization). Drops fabro-cli's `i64::try_from(record.number)`
panic path and the AutoMergeMethod enum (replaced by MergeMethod).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Integrates origin's worker-JWT-auth work (commits 8a6f83bb0..c847a828d)
with the config-boundary refactor that landed locally. Conflicts
resolved:
- commands/dump.rs: take origin's removal of the 500-line in-process
test block (replaced by real-server integration coverage).
- commands/run/runner.rs: keep local's dense WorkflowSettings import,
drop dead SettingsLayer import, pull in origin's ActorRef.
- manifest_builder.rs: adopt origin's lifted working_directory
resolution (fixes#159 - manifest git detection in nested repos),
but via local's resolve_working_directory_from_run API that takes
the dense RunNamespace. Update the regression test's
ManifestBuildInput literal to local's run_overrides/cli_overrides
field shape.
- server.rs: keep origin's jwt_auth_mode/jwt_auth_state/
test_user_subject/issue_test_user_jwt/issue_test_worker_token/
create_run_with_bearer/bearer_request test helpers, adapt
jwt_auth_state to local's create_test_app_state_with_session_key
signature (ServerSettings + RunLayer), keep local's dense
canonical_origin_settings that returns ServerSettings via
server_settings_from_toml. Rewrite
build_app_state_requires_session_secret_for_worker_tokens against
the dense AppStateConfig (resolved_settings +
resolved_runtime_settings_for_tests).
Post-merge verification: workspace builds clean, cargo +nightly
fmt --check all clean, cargo +nightly clippy --workspace
--all-targets -- -D warnings clean, cargo nextest run --workspace
4560 tests passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Let callers decide whether to wrap in Arc. Also consolidates the two
state() fetches in build_pr_body into one.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sub-workflows were hardcoding Anthropic + EnvCredentialSource instead of
inheriting the parent run's provider and source, so vault-only auth and
non-default providers silently broke inside manager_loop.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three test modules drifted to non-canonical style during the post-merge
CI fixup; reformatting brings them back in line with the pinned nightly
rustfmt config so `cargo +nightly-2026-04-14 fmt --check --all` is clean
again.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Parallelize `pr list` discovery loop via buffer_unordered; thread RunId
through the stream to drop the run_id.parse().expect(...) panic path.
- Skip computing the default model in create_run_pull_request when the
request already supplies one (common path from `fabro pr create`).
- Delete the dead user_config::storage_dir wrapper (test-only, zero
callers, stale deprecation note); point its tests at
local_server::storage_dir directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- 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>
