Extends handleLifecycleToastResult to cover the cancel intent and
switches cancel's effect onto the shared helper. lastProcessed is now
keyed per intent so the three effects don't clobber each other's dedup
state, and cancel picks up the same replay guard that archive and
unarchive already had.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds expect(disallowed_types) at the tests module for the intentional
sync BufReader usage in the zombie-process-group helper, and drops the
absolute-path call site by bringing pre_exec_setpgid into scope.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merges handleArchiveToastResult and handleUnarchiveToastResult into a
single helper. Replaces the content-hash dedup key with object identity
on fetcher.data and collapses the two "last key" fields into one
lastProcessed. Tests now import the exported helper directly instead of
casting through Record<string, unknown>.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Split raw PID existence from actual process liveness in fabro-proc and
switch the server shutdown paths to the running-process predicate. This
avoids waiting out stop timeouts for unreaped zombie children while
keeping process-group behavior covered by measured regression tests.
Expose cancel, archive, and unarchive from the run detail view,
surface blocked-question context, and route run-detail and run-files
notifications through a single shared toast provider.
This also refreshes the embedded SPA bundle and marks the lifecycle
actions plan complete.
- Move the GitHub App webhook config update to fabro-github as
update_app_webhook_config, matching the crate's existing HttpClient +
Result<_, String> conventions. Server-side callers go through the new
symbol.
- Add Bind::tcp_port() on the enum itself and drop the free function.
- Collapse the six near-identical "webhook strategy configured but ...;
skipping webhook startup" warn branches into resolve_webhook_preconditions
returning a Ready/Skip enum, with one warn! at the call site.
- Replace the per-file test-helper wrappers (assert_status, checked_response,
response_json, response_bytes) with local macro_rules! macros so
file!()/line!() expand at the caller. Panic context now identifies the
failing assertion's source line instead of the wrapper's definition.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Distinguish the two GitHub webhook auth-failure warn messages (missing
signature header vs. HMAC mismatch) so logs can tell them apart.
- Route update_github_app_webhook through fabro_github::github_api_base_url()
so GITHUB_BASE_URL overrides the webhook config endpoint too.
- Drop a narrative shutdown comment that restated the next two lines.
- Replace concat!(file!(), ":", line!()) inside local test-helper wrappers;
those macros expand at the wrapper definition site, so every panic
reported the same phantom location. Pass the wrapper name instead.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add shared axum/reqwest response assertion helpers in fabro-test,
migrate the Rust HTTP test surface to use them, and document the
new rule in the testing strategy.
CI's TypeScript Build job upgraded bun to 1.3.13 (via setup-bun@v2.2.0
pulling the latest release), which produces a different content-hashed
entry CSS than the bundle committed under bun 1.3.10. The drift was
caught by the widened path filter in c8b807f30 and failed the
git diff --exit-code check on lib/crates/fabro-spa/assets.
Rebuilds with bun 1.3.13 so the embedded SPA matches the build CI
reproduces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Capture webhook secret at route mount time via Arc<[u8]> router
state so the handler drops its per-request server_secret lookup
and the dead NOT_FOUND fallback.
- Extract WEBHOOK_ROUTE and WEBHOOK_SECRET_ENV constants; apply
across serve.rs, server.rs, and TailscaleFunnelManager so the
mounted route and the URLs pushed to GitHub cannot drift.
- Flatten the seven-level nested webhook startup match in serve.rs
into a single start_webhook_strategy helper with early returns,
short-circuiting when the secret is absent and replacing the
server.api.url .expect with a propagated error.
- Share compute_signature and a new read_repo_file helper across
tests; delete the duplicated webhook_signature, TestHmacSha256,
and read_doc/repo_root copies.
- Replace the nested for-loops in the new webhook auth tests with
five flat #[tokio::test] cases per CLAUDE.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prevents silent bun.lock drift across CI runs (root cause of the
nightly fabro-spa staleness failure) and re-runs typescript.yml when
nightly.yml itself changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Model the GitHub webhook request body as JSON so the generated TypeScript
client exposes a coherent request shape, and add explicit conformance
coverage for the secret-gated webhook route.
Move GitHub webhook intake onto the main API router, add explicit
server_url and tailscale_funnel strategies, and validate strategy
requirements at config resolution. This also updates the API contract,
generated client, and operator docs to match the new webhook model.
Enable clippy's unwrap_used lint at warn level, document the long-term
policy carveouts for tests and LockResult, and localize the generated
OpenAPI client exemption so the remaining warning surface is real repo
code.
Enable clippy::allow_attributes_without_reason at the workspace level.
Add concise, callsite-specific reasons to existing allow attributes, including generated code paths.
Resolve rm/archive/unarchive selectors through the server-owned
runs/resolve endpoint instead of CLI-side summary matching, and move
active-run delete force semantics into DELETE /runs/{id}.
Reverts the `single_node_ack` acknowledgment flag and all the cross-node /
multi-node qualifier text introduced in the previous two commits. fabro-server
is single-node by design; there is no multi-node deployment model to design
against. The in-process per-hash and per-code mutexes in Units 9 and 10
provide the full atomicity guarantees R3 requires. R12 restored to the
original web-enabled + SESSION_SECRET-length check. No ack flag, no
config-surface EULA, no cross-node tests, no multi-node risks-table row.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `server.auth.github.single_node_ack: bool` (default `false`) as a new
config field required by startup validation when `github` is in
`auth.methods`. Without the flag, the server refuses to boot. This turns the
previously documentation-only single-node constraint into a fail-closed
startup check — an operator can still misdeploy to multi-node after setting
the flag, but they must affirmatively acknowledge the tradeoff first.
Alternative (auto-detect via SlateDB boot-heartbeat) deferred as future
work; explicit acknowledgment is lower-complexity and avoids rolling-deploy
false positives. Enforcement lives in Unit 5 startup validation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a 22-unit implementation plan for `fabro auth login`/`logout`/`status`
with GitHub OAuth + PKCE, HS256 JWT access tokens, rotating opaque refresh
tokens, and a typed `IdpIdentity` flowing across fabro-types / fabro-store /
fabro-server. v1 is scoped to single-node deployments with `github` in
`auth.methods` (distributed refresh-token rotation coordination is deferred).
Origin spec: docs/superpowers/specs/2026-04-19-cli-auth-login-design.md
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two follow-ups the workspace lint now catches:
- fabro-server tests/it/api/install.rs: a newer install-router integration
test was missing the `.await` after `build_install_router(...)` -- the
fn became async when the devcontainer/install-mode resolver was
converted to tokio::fs in commit 19939c5f0.
- fabro-cli main.rs: add #[expect(clippy::disallowed_methods)] to the
#[cfg(test)] module whose write_test_settings helper uses sync
std::fs::write to stage CLI settings fixtures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
origin/main introduced a new `archived` lifecycle status as a terminal
state reached by explicit user action on a previously terminal run.
deriveEmptyKind didn't know about it — archived runs with files would
have rendered "The diff for this run is no longer available. If you
expect files here, please report it.", which is wrong; the diff was
captured normally, the run was just archived later.
Adds `archived` to the terminal-success branch so archived runs show
the correct empty-state copy (R4b or R4c2) based on total_changed,
same as a succeeded run.
The regression-guard test is also tightened: it now iterates over
`RunStatus` from @qltysh/fabro-api-client rather than a hand-
maintained list, so any future addition to the OpenAPI spec fails
this test until the decision table grows a branch. This exact class
of silent-regression is what made me miss archived in the first
place.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a server-native run selector endpoint and migrate CLI single-run flows to
use it instead of local workflow-store heuristics. This also moves store dump
export assembly into the CLI, removes the production CLI dependency on
fabro_workflow run lookup and dump helpers, and records the remaining
cli-to-workflow coupling in an audit document.
Replace the remaining blocking filesystem touches in shared async code with
Tokio-native I/O or explicit blocking boundaries. This keeps provider file
loading, workflow metadata rebuilds, and related export paths compatible with
the stricter clippy async-fs rules without changing their external behavior.
Integrates 39 commits from origin/main (archive/unarchive feature, UI
unification, theme/light-mode polish, Settings nav promotion, server
and CLI hardening).
Conflict resolutions:
- apps/fabro-web/app/routes/run-detail.tsx: origin removed the
`broken` field from the tab config; local added the Files Changed
tab. Kept the Files Changed tab, dropped the broken field per
origin's shape.
- lib/crates/fabro-store/src/run_state.rs: both sides added tests
in the same region. Kept local's two final_patch tests and all
four of origin's archive/unarchive tests.
- lib/crates/fabro-spa/assets/: embedded SPA bundle rebuilt from
the merged web source.
- lib/crates/fabro-workflow/src/operations/archive.rs: origin's new
archive tests construct Event::WorkflowRunFailed{..}; added the
final_patch: None field that local's lifecycle change introduced.
Workspace verification after merge: 4247 Rust tests + 95 web tests
all pass; clippy clean; fmt clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two coverage gaps closed:
1. `parse_head_show_output` — extracted from `resolve_head_sha_and_time`
as a pure function so it can be tested without a sandbox. Six tests
cover: well-formed sha+iso line, non-UTC timezone normalization,
sha-only output (missing %cI), malformed date (parser tolerates
and returns sha with None date), empty-input rejection, and
surrounding-whitespace tolerance. New code from the simplify pass,
previously unverified.
2. `fetch_blob_table` two-phase error isolation — `ScriptedBlobSandbox`
(hand-written minimal Sandbox impl) returns different exec responses
for `cat-file --batch-check` vs `cat-file --batch`. The phase-2
failure test proves that a malformed --batch parse outcome doesn't
corrupt phase-1-classified oversized entries — the doc-comment's
promise that the two phases are isolated now has a regression test
behind it. The phase-1-skip test enforces the
METADATA_PHASE_SHA_THRESHOLD contract by making phase 1's
batch-check response an error: if the threshold logic regressed
and phase 1 ran, the test would fail with a 503.
Also adds `Debug` to `ApiError` (required by `Result::expect` in the
new tests) and adds `async-trait`/`tokio-util` as dev-dependencies
plus the `test-support` feature on fabro-sandbox.
Total workspace test count: 4173 -> 4180.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applied fixes from three parallel reviews (reuse, quality, efficiency):
Server
- Delete dead `sandbox_git_env()` in run_files_security.rs — duplicated
`sandbox_git.rs::sandbox_git_hardening_env` but had no callers.
- Combine `resolve_head_sha` + `resolve_commit_time` into one
`resolve_head_sha_and_time` using `git show -s --format=%H\ %cI HEAD`
— saves ~100ms per request (one fewer sandbox round-trip).
- Parallelize `list_changed_files_raw` + `list_binary_paths` with
`tokio::join!` — both are mutually independent once `to_sha` is
known, saves another ~100ms per request.
- Skip phase-1 `cat-file --batch-check` for SHA lists below 10 entries.
Phase-2 size-caps per-blob anyway; the pre-filter earned its cost
only for large batches where a single malformed blob could poison
the parse. Saves another ~100ms on small diffs.
- Extract `transient_503(op, message)` helper — dedupes three identical
`DiffError::Transient => ApiError::new(503, ...)` arms.
- Strip plan-referencing comments ("§ Unit 5", "P1-X", "P2-Y regression")
from production code and tests. The R4/R5 taxonomy labels are kept
where they anchor semantic intent.
Web
- Dedupe `extractRequestId`: one canonical parser in `run-files.tsx`
(consumed by the loader), one ErrorBoundary-only variant in
`states.tsx::extractRequestIdFromUnknown`. Both share the same logic;
separated only so each source can pick its own type discipline.
- Extract `renderStatusError({status, requestId, onRetry})` shared
between the loader's inline-error path and `RunFilesErrorBoundary`.
One canonical source of R5 copy.
- Gate the `useFreshness` 10s interval on `hasLabel` — previously it
ticked every 10s even when `meta == null` and there was no label to
refresh, re-rendering the whole route for nothing. Now the interval
only runs while there's actually a timestamp label mounted.
- Fix render-time ref mutation (`lastGoodDataRef.current = result.data`
in the render body) — violates React render purity. Moved into the
`useEffect` that watches `result?.data`. Also collapsed
`previousDataLengthRef` and `lastToShaRef` into single reads off
`lastGoodDataRef.current` — both were derivable from the cached
last-good payload.
- Type `DegradedBanner.reason` and `bannerCopyForReason` as
`RunFilesMetaDegradedReasonEnum` instead of raw `string`.
Tests: 4172 Rust + 94 web, clippy clean, fmt clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Origin brought 21 commits of UI/install/test-helpers work that lived in
parallel with the archive feature. Only the SPA build outputs conflicted
(old bundle hashes on both sides). Resolution: accept origin's
resolution on the deleted files, then re-run scripts/refresh-fabro-spa.sh
from the merged source so the embedded bundle reflects both sides —
origin's Settings-nav/theme/stage-sidebar work plus this branch's
archived-status TypeScript changes in apps/fabro-web/app/data/runs.ts.
Verification:
- cargo build --workspace: clean
- cargo nextest run --workspace: 4198 passed, 182 skipped
- cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings: clean
- cargo +nightly-2026-04-14 fmt --check --all: clean
- apps/fabro-web bun run typecheck: clean
- apps/fabro-web bun test app/data/runs.test.ts: 8 pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adversarial review surfaced that both RunArchived and RunUnarchived apply
arms were naive — any out-of-spec event in the log (concurrent double
archive, tampered import, replayed retry) would permanently corrupt the
projection:
- RunArchived unconditionally captured current status into prior_status.
A second RunArchived would set prior_status=Some(Archived). Unarchive
would then emit restored_status=Archived, the apply arm would set
status=Archived and clear prior_status, and the run would be unrecoverable.
- RunUnarchived trusted restored_status unconditionally. An imported
event with restored_status=Running produced a projection reporting
status=Running with no RunRunning event in the log — breaking
is_active/is_terminal invariants.
Both arms now require a sensible pre-state before mutating:
- RunArchived only transitions from Succeeded|Failed|Dead.
- RunUnarchived only runs from Archived with a terminal restored_status.
Adds three regression tests:
- double_archive_preserves_prior_status
- run_unarchived_with_non_terminal_restored_status_is_ignored
- run_archived_on_non_terminal_projection_is_ignored
The operations layer (archive/unarchive in fabro-workflow) still validates
at emit time; the projection guards are a defensive second line for
replay, imports, and any future code path that double-writes.
Two follow-ups from internal review:
1. deriveEmptyKind was incomplete. The full RunStatus enum (per
fabro-types/src/status.rs and apps/fabro-web/app/data/runs.ts) has
ten values — submitted, queued, starting, running, blocked,
paused, removing, succeeded, failed, dead. My decision table
covered only six and incorrectly included "partialsuccess" which
is a stage status, not a run status. Unhandled statuses
(blocked, paused, removing, dead) silently fell through to the
"diff_lost" branch, which showed users the alarmist "the diff for
this run is no longer available" copy for runs that are merely
paused or being torn down.
New table:
- submitted / queued / starting → R4(a) "starting"
- running / blocked / paused → R4(b) "no_changes" (yet — user
can refresh)
- failed / dead → R4(c1) "failed before checkpoint"
(R4b-equivalent when a degraded
patch did survive)
- succeeded / removing → R4(c2) "diff_lost" if
total_changed > 0, else R4(b)
- unknown future status → R4 "unknown" fallback
Test suite now drives each documented status through a regression
guard that asserts no known status collapses to "unknown" when a
more-specific kind should apply.
2. Loader integration tests. The `extractRequestId` unit test covers
only the extractor; nothing exercised the full fetch → body-read
→ requestId → error chain. Added 8 loader tests covering:
- 200 OK returns the parsed envelope
- 404 / 501 collapse to the empty-envelope signal (null + null)
- 500 with `request_id` in errors[0] populates error.requestId
- 500 without a request_id leaves it null
- 500 with non-JSON body still surfaces the status
- 503 populates error without requestId
- 401 surfaces as an error (no in-loader redirect — that concern
lives in apiFetch, which the Files loader deliberately bypasses
to preserve error bodies)
The tests stub globalThis.fetch; the loader already accepts the
cancellation-signal-only `request` object.
Refs docs/plans/2026-04-19-002-feat-run-files-changed-tab-plan.md §
Unit 11 R4/R5 taxonomies.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Earlier refactor to a discriminated-union loader accidentally
discarded the plan's R5 error taxonomy. `apiJsonOrNull` throws a
body-less Response on non-ok statuses, so the loader's try/catch had
no way to recover the server's error envelope or the request_id for
500s. The initial-error render then collapsed all statuses into
either `<EmptyState kind="unknown">` (401/403) or a generic
InlineErrorBanner — losing the plan-specified copy for access denied,
transient failures, and 500 with request ID.
Fixes:
- Loader now uses `fetch` directly against the API path so the
response body is preserved on non-ok statuses.
- 404/501 still collapse to `{data: null, error: null}` (the empty-
envelope signal the UI maps to R4).
- Any other non-ok parses the body as JSON, extracts request_id from
either the top-level `request_id` field or the uniform error
envelope (`errors[0].request_id` or parsed out of
`errors[0].detail`), and threads it through `error.requestId`.
- Component's `initialError` branch now applies the full R5 taxonomy:
R5(c) access denied for 401/403 with the specific copy, R5(a)
retry banner for 429/503, R5(d) "Something went wrong. Request ID:
<id>. Contact support." for 500s, and a generic retryable banner
for any other 4xx.
Adds run-files.test.ts covering extractRequestId across the three
locations request_id can show up in a server error body (top-level,
errors[0].request_id, errors[0].detail regex).
The RunFilesErrorBoundary export stays in place as defense-in-depth
for React render crashes — the loader no longer throws, but ensuring
the route always has a fallback is cheap.
Refs docs/plans/2026-04-19-002-feat-run-files-changed-tab-plan.md §
Unit 11 R5 taxonomy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1. Empty-state taxonomy was reading the wrong field. The parent Run
Detail loader returns status as `run.lifecycleStatus`, not
`run.status` (apps/fabro-web/app/data/runs.ts:86). resolveRunStatus
looked for `status` and always fell back to `unknown`, so R4(a)
starting / R4(c1) failed_before_checkpoint / R4(c2) diff_lost were
unreachable in the real route. Fixed to read `lifecycleStatus`.
2. Revalidation error state was dead code — the UI rendered
InlineErrorBanner from `revalidationError` but nothing ever set it
to non-null. Fixed by changing the loader contract to a
discriminated union `{ data, error }` that catches Response throws
and returns them in-band. This lets both initial-load and
revalidation errors flow through the same render path:
- Initial load with error + no prior data → inline error render
(no unmount, no ErrorBoundary trip)
- Revalidation error with prior data → keep prior data mounted,
show InlineErrorBanner + Retry
The plan's intent (§ Unit 11) was specifically "prior content stays
mounted" on mid-session failures; this finally implements it.
3. Live diff path skipped the planned stream_blob_metadata phase. A
single malformed blob in --batch output was collapsing the whole
fetch to an empty map and flagging every file in the response as
truncated. Two-phase fetch:
- Phase 1: stream_blob_metadata to identify oversized blobs by
size before any content fetch.
- Phase 2: stream_blobs on only the remaining under-cap SHAs.
A phase-2 parse error now only affects its own SHAs;
phase-1-classified oversized entries keep their correct
classification rather than all flipping to undifferentiated
truncated placeholders.
Refs plan docs/plans/2026-04-19-002-feat-run-files-changed-tab-plan.md
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three cleanups from `/simplify` review:
- Promote `archived_rejection_message` and `ensure_not_archived` to `pub`
via operations/mod.rs and reuse them from `resume`, the CLI rewind
caller, and the server's `reject_if_archived` guard so the canonical
error string lives in exactly one place.
- Tighten `RewindInput.current_status` from `Option<RunStatus>` to
`RunStatus`. The runtime check for None was enforcing a compile-time
invariant. CLI callers already load the projection and now surface a
clean error up-front if it's missing. Drop the None-branch test that
existed only to cover the removed runtime check.
- Collapse `archive_run` / `unarchive_run` HTTP handlers into a shared
`run_archive_action` body with an `ArchiveAction` enum, mirroring the
CLI pattern. Removes ~20 lines of copy-paste and unifies error-mapping.
Also drop narrative comments that referenced plan unit numbers in the
scenario tests, and clean up the convoluted `ps_runs` helper pattern
that built an empty-slot arg vec before filling it in.
No behavior change. Full workspace: 4185 tests pass, clippy clean.
Two fixes from the Run Files security review
(docs/agent/reviews/2026-04-19-run-files-security-review.md):
Medium — Add `-c core.quotePath=false` to git invocations that feed
the denylist.
- git_diff_with_timeout (produces final_patch for the degraded
fallback) — without this, a tracked file with non-ASCII chars,
tabs, quotes, or backslashes in its name makes git emit a
header like `diff --git "a/…" "b/…"`. The Run Files server's
strip_denylisted_sections parser only recognizes unquoted
`a/<old> b/<new>` forms and would let the sensitive section pass
through unfiltered.
- GIT_HARDENED (the raw-diff / cat-file prefix used by the Run
Files enumerator) — applied for symmetry so any future consumer
parsing these invocations' output can't be tripped by the same
quoted-path divergence.
Low — is_sensitive path normalization switches to ASCII-only case
fold. Full Unicode `to_lowercase()` can expand a codepoint into
multiple chars (e.g. `İ` -> `i\u{307}`), which then silently fails
to match an ASCII glob like `id_rsa`. ASCII-only folding makes the
homoglyphic-path failure mode explicit — a path a reviewer can see
is homoglyphic just doesn't match — rather than disguising it
behind an opaque lowercase routine. All denylist globs are ASCII by
design.
Other findings in the review (denylist policy coverage gaps around
id_rsa_backup / .netrc / .npmrc / etc.) are policy decisions, not
matcher bugs, and are deferred.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends `archived_runs_reject_mutations_with_actionable_body` to assert
the archive guard fires on the four write surfaces the Unit 4 audit
guarded but the scenario skipped: POST /questions/{qid}/answer, POST
/stages/{stage_id}/artifacts, PUT /sandbox/file, POST /blobs. Synthetic
stage/question/filename values are fine — `reject_if_archived` runs
before each endpoint's state-specific lookups.
