EnvCredentialSource::credential_for hardcoded "ANTHROPIC_API_KEY",
"OPENAI_API_KEY", etc. in match arms, while configured_providers read
the same names from Provider::api_key_env_vars(). Renaming any env var
required editing both sites.
Pull the primary key lookup from api_key_env_vars() so the Provider
enum owns the env-var-name → provider mapping. Provider-specific extras
(ANTHROPIC_BASE_URL, OPENAI codex mode, etc.) stay inline — they aren't
about the API key itself.
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>
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>
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>
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>
Was linking to https://fabro.dev/getting-started/quick-start (wrong
domain, 404). Use relative /getting-started/introduction so it resolves
correctly on docs.fabro.sh.
Fixes#167
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
clippy.toml already bans std::env::{set_var,remove_var} via
disallowed_methods, and every existing call site carries a scoped
#[expect(clippy::disallowed_methods, reason = "...")]. The shell grep
is redundant and forced a second, less granular allowlist.
Also update server-secrets-strategy.md to describe clippy as the
enforcement mechanism.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both Boundary checks have been red on main for multiple commits:
- check-boundary.sh: install.rs reintroduced direct use of
fabro_config::ServerSettings::from_layer in 93b6577cd but was dropped
from server_symbol_allowlist in bb0d05be2. Re-add it.
- check-env-mutation.sh: the worker FABRO_WORKER_TOKEN scrub added in
077469d0c is documented as the approved pattern in
docs-internal/server-secrets-strategy.md but was missing from the
allowlist. Add the exact line.
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>
Patches GHSA-j687-52p2-xcff (CVE-2026-41067): XSS in define:vars via
incomplete </script> tag sanitization. Requires Astro >= 6.1.6.
Also bumps @astrojs/react to ^5.0.4 for Astro 6 compatibility.
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>
Thread the workflow's resolved LLM client into native pull request generation so PR bodies use the same vault-backed provider resolution as normal runs. This fixes auto-PR failures when OpenAI is configured via credentials like openai_codex instead of process environment variables, and keeps the legacy fabro pr create call site compatible with the new signature.
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>
