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>
