- 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>
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 `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>
The worker subprocess is spawned with env_clear+allowlist by the server, so
the only sensitive value in its env is FABRO_WORKER_TOKEN itself. Read the
token and remove_var it from the process env in main() before Tokio starts
worker threads, then thread it explicitly through runner::execute(&str).
Every descendant (hooks, local sandbox, devcontainer initializeCommand,
MCP stdio, etc.) now inherits a worker env with no bearer in it, so an
unscrubbed spawn site cannot leak the token. This makes the prior denylist
scrub in fabro-hooks and fabro-sandbox redundant — delete it and the shared
WORKER_SECRET_ENV_DENYLIST constant. The sandbox keeps its _api_key/_secret/
_token/_password/_credential suffix heuristic for user-supplied env_vars
hygiene.
Extend the server-dispatched-worker env-leak integration test to also
assert a Bash stage running in the worker does not observe FABRO_WORKER_TOKEN.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- fabro-client: collapse identical match arms for DevToken/Worker bearer
- fabro-hooks: rewrite filter_map(bool::then) as filter().map() chain
- fabro-sandbox: import WORKER_SECRET_ENV_DENYLIST rather than absolute path
- fabro-server: box large execute_run_in_process future; take path: &str in
test-only bearer_request; use let-else in session-secret test; replace unit
pattern _ with () in worker_token request_parts helper; import StatusCode
- fabro-cli run/mod.rs: box large runner::execute future
- fabro-cli worker_auth.rs: drop unused async on shutdown, allow
clippy::unwrap_used at file level for subprocess test harness setup
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Collapse splice_model_fallbacks and splice_events into a single
generic splice_combine guarded by a new SpliceMarker trait; the
Combine impl for Vec<T: SpliceMarker> replaces the two per-enum impls.
- Drop inherent iter/iter_mut (redundant with Deref/DerefMut),
AsRef/AsMut, and IntoIterator for &_/&mut _ on ReplaceMap/StickyMap/
MergeMap — they had zero external callers. DerefMut and IntoIterator
for Self stay because labels.extend(...) relies on both.
- Drop unused _api parameter on resolve_web.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The check `ctx.user_settings().cli.output.verbosity == OutputVerbosity::Verbose`
repeated 6 times across doctor, preflight, run command/resume/mod. Add a
`verbose()` method and replace every call site. `cargo fix` handles the
now-unused `OutputVerbosity` imports.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Auto-applied by `cargo +nightly-2026-04-14 fmt --all`. `cargo fix`
earlier inserted the import in a position that violated the grouped
ordering.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After StubEnv was removed, the EnvSource trait had only two impls
(ProcessEnv unit struct and a blanket HashMap impl) and tests already
passed HashMaps. Replace the trait with a free `process_env_snapshot()`
function and take `HashMap<String, String>` by value in
`ServerSecrets::load` and the startup validators.
Also flatten `StartupResolution` to a `(AuthMode, ServerSecrets)` tuple
and drop the `StartupValidationError` wrapper in favor of
`anyhow::Result`, and inline the `*_with_lookup` test-only wrappers in
`spawn_env.rs` so tests call `apply_allowlist` directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The cfg(test) helper only re-implemented the production `with_server_mode`
struct literal so tests could inject settings without disk I/O. Three
tests used it, but each one was self-referential — asserting what the
helper itself does rather than exercising production code. Remove the
helper and those three tests.
`synthetic_context_with_settings` is no longer needed either (its
flexibility only mattered for the removed tests); collapse it into
`synthetic_context`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`ResolvedBaseContext` was a staging struct that held pre-context
settings so main.rs could read `user_settings.cli` before committing
to a full `CommandContext`. Now that install no longer needs the
indirection, the staging step doesn't earn its weight.
Give `CommandContext` a direct `from_disk(cli_layer, process_local_json)`
constructor that does the load + printer derivation + struct build in
one shot. main.rs builds one `base_ctx` before the dispatch match and
every arm borrows it — the `build_base_ctx` closure and ~20 duplicate
`let base_ctx = build_base_ctx()?;` lines disappear.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Public entry points (`execute`, `run_install`, `run_install_github_command`,
`run_install_inner`, `run_install_github_inner`) now take `&CommandContext`
instead of a 4-tuple of `(cli, cli_layer, process_local_json, printer)`.
Extract cli/printer/json from the context once at the top of each.
The nested doctor invocation inside run_install_inner previously built a
fresh `ResolvedBaseContext::from_disk(...).to_context()` with
`process_local_json = false`. Doctor only reads the resolved output
format (`base_ctx.json_output()`), not the invocation flag, so passing
the parent ctx through is behaviorally equivalent and avoids a second
disk load.
main.rs drops the `cli_settings` local entirely — no remaining command
needs it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These top-level commands still took `&CliNamespace` + `Printer`
separately. Thread `&CommandContext` through the public entry points
and pull what's needed (`user_settings().cli`, `printer()`,
`json_output()`) from the context:
- parse: both args were unused — drop entirely.
- workflow list/create: use `ctx.json_output()` / `ctx.printer()`.
- exec: bind `cli = &ctx.user_settings().cli` at the top; drop unused
printer param.
- upgrade: extract cli/printer inside run_upgrade; leave the private
run_upgrade_brew helper with its existing signature (unit tests use
`CliNamespace::default()` directly).
- uninstall: use `ctx.json_output()` / `ctx.printer()`.
Install remains on the old signature — its nested callback structure
makes a larger refactor than this simplification pass warrants.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
StubEnv was a thin newtype only used by tests but compiled into every
build. Implementing EnvSource directly on HashMap<String, String> lets
test sites pass a HashMap and removes the type entirely.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The helper was a 2-line indirection with a single caller. Inline its
body so the whole command fits in one function.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`load_pr_record` built a `with_target`-derived context internally and
threw it away, forcing callers to re-derive or fall back to `base_ctx`.
Return the context alongside the record and let close/merge/view use
it directly for printer/json access and github-credentials lookup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `fabro store dump` -> `fabro dump` rename left `commands/store/` as a
vestigial directory with a stale one-line `StoreRunExport` alias. Move
`dump.rs` and `rebuild.rs` up to `commands/`, import `RunDump` directly,
rename `dump::dump_command` -> `dump::run`, and clean up stale docs and
a noise test that only asserted clap's default error output.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`CommandContext::base` had a single caller (install.rs) and duplicated
the disk-load path that `ResolvedBaseContext::from_disk` already
provides. Route install through `ResolvedBaseContext::from_disk(...).to_context()`
and drop the standalone constructor. `base_with_settings` stays as the
private shared helper behind both `to_context` entry points.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `cli_settings` local was a standalone clone of `user_settings.cli`.
Drop the clone and rebind it as `&resolved_base.user_settings().cli`
inside the async block — all callers already borrowed it anyway.
Pre-async uses inline `resolved_base.user_settings().cli.<field>`
directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The secret dispatcher pre-computed json/printer and threaded them into
every subcommand. Pass the context directly so each subcommand pulls
what it needs, dropping the fabro_util:🖨️:Printer import from
three files along the way.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`run_bulk` and `remove_from` took separate `json: bool` + `printer`
parameters. Thread the context through instead and pull json/printer
out of it inside the helper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The helper only exists to let tests inject pre-loaded settings. Inline
the struct-literal into the sole production caller and mark the helper
`#[cfg(test)]` so the test-seam intent is explicit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The check `ctx.user_settings().cli.output.format == OutputFormat::Json`
(and its `!=` variant) was repeated 51 times across 36 files. Add a
`json_output()` method on CommandContext and replace every call site.
`cargo fix` handles the now-unused `OutputFormat` imports.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- ssh/graph: remove `explicit_json_requested() &&` guard before
`require_no_json_override()` (the call already no-ops without --json).
- pr close/merge/view: drop the outer `with_target` derivation that was
used only for `printer()` and the output format — both match base_ctx,
so the derivation was an unused disk-read + settings re-merge.
- command_context tests: collapse `synthetic_context` to delegate to
`synthetic_context_with_settings`, removing duplicated struct literals.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
