Replaces hand-written K/V stores in fabro-store with a shared Record trait
plus Repository<R> typed K/V layer. Adds KeyedMutex for per-key serialization
and transaction() for all-or-nothing WriteBatch commits. Renames
SlateAuthCodeStore/SlateAuthTokenStore to AuthCodeStore/RefreshTokenStore and
adds BlobStore and RunCatalogIndex wrappers on top of Repository. Deletes
catalog.rs in favor of RunCatalogIndex. Database gains blobs() and
catalog_index() accessors; auth_tokens() is renamed refresh_tokens().
Plan: docs/plans/2026-04-20-003-refactor-fabro-store-record-abstractions-plan.md
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- MetadataStore: drop redundant write_files (identical to write_snapshot)
and the brittle has_projection_data OR-chain in read_run_projection.
- operations:⏪ introduce find_run_id_by_prefix_opt using
META_BRANCH_PREFIX; delete the duplicate find_run_id_by_prefix_in_refs
helper in rebuild_meta and have it call the shared function.
- rebuild_meta: stop cloning latest_init_snapshot once the init snapshot
has been written; take() the stored snapshot instead of cloning again.
- retro::upload_data_files: collapse the ten eager *_path variables into
inline base.join(...) args by making upload_file take &Path; replace
Vec<String>.join("\n") + "\n" with a streaming String loop.
- Migrate retro_agent test std::fs::read_to_string to tokio::fs to
satisfy disallowed_methods clippy lint under tokio tests.
- Minor: fork.rs drop misnamed `now` var; metadata.rs doc comment
describes the unified RunProjection snapshot.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rename the last stale workflow test helpers and assertions that still used
pre-refactor checkpoint/retro file terminology, and update the retro docs
to describe the exported layout that now exists.
Replace the narrow decimal/hex obfuscation check with a general
comparison: if the parsed host is an IPv4 literal and the raw input
host differs from the canonical dotted-quad form, the user supplied
an obfuscated variant (octal, short-form, mixed radix, leading
zeros, decimal integer, hex integer) that url::Url has already
normalized to 127.0.0.1. All such variants are rejected. Test now
covers decimal, hex, octal, two-/three-part short, mixed hex/
decimal, and leading-zero octets.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements the plan at
docs/plans/2026-04-20-003-refactor-unify-run-vocabulary-metadata-plan.md.
- Rename RunRecord to RunSpec and RunProjection.run to .spec everywhere
in Rust source, tests, helpers, test names, and error messages.
- Introduce SerializableProjection wrapper that trims bulky node text
fields (prompt, response, diff, stdout, stderr) for run.json snapshots.
- Collapse metadata-branch and CLI export to one RunDump::from_projection
builder emitting run.json + graph.fabro + stages/{stage_id}/... and
drop legacy top-level start/status/checkpoint/sandbox/retro/conclusion
split files.
- Replace MetadataStore::write_checkpoint with write_snapshot returning
the commit SHA; add read_run_projection/read_run_spec; demote
read_checkpoint/read_start_record to projection-field extractors.
- Switch fork, rewind, rebuild_meta, CLI rewind recovery, and retro
upload to read the unified projection layout.
- Add additive query methods on RunSpec and RunProjection.
Serde-level `alias = "spec"` shim dropped; `rename = "run"` retained to
keep the server API wire format stable per the plan's scope boundary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
url::Url normalizes decimal (http://2130706433) and hex
(http://0x7f000001) IPv4 host forms into 127.0.0.1, so after
canonical_http_url rewrites the target the loopback classifier
cannot tell them apart from a legitimate http://127.0.0.1 and
lets a refresh token ride plaintext HTTP. Detect these forms on
the raw input string and bail out before the url crate can hide
them, and split the loopback test to cover the parse-time
rejection path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Expose apply_bearer_token_auth and ensure_refresh_target_transport
from fabro-client; drop the CLI's duplicate copies.
- Collapse AuthStore's two read paths into one NotFound-tolerant
reader and drop the pre-existence checks in get/remove/list.
- Avoid rewriting auth.json when remove found nothing.
- Inline the one-line user_config::build_public_http_client wrapper.
- Trim unused pub use fabro_api::types re-export and the narrating
doc comment in fabro-client/src/lib.rs.
- Clean up pre-existing unused imports in run/create.rs and
loopback.rs tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lift shared client DTOs into fabro-types, move auth/target/error/session
logic into fabro-client, and reduce fabro-cli to orchestration around the
builder-based client path.
This also lands the remaining plan cleanup for ApiError, ServerTarget
canonicalization, and the RunEventStream rename at the CLI boundary.
Document that unknown Fabro run events already fall back through
EventBody::Unknown, and that the refactor only needs to preserve
that behavior during the EventEnvelope move.
The `foreground_start_writes_tracing_to_storage_server_log` test
consistently took ~10.4 s. 10.3 s of that was spent inside `fabro
server stop`, which polls `process_running(pid)` every 100 ms until
the server exits. The test's server is spawned as a child of the test
process (`child.spawn()`), and the test only reaps it via
`child.wait_with_output()` after `fabro server stop` returns. After
Step A's revert, `process_running` is a plain `kill(pid, 0)`, which
returns true for a zombie — so the poll saw the dead-but-unreaped
server as alive and burned the full 10 s timeout.
Add `fabro_proc::process_running_strict(pid)` — the same
ps-shelling zombie-aware predicate commit 1ed8e6cbd introduced — and
use it only in `fabro-cli`'s server stop poll. The hot paths that
motivated Step A (test-harness marker scans, daemon-liveness probes)
continue to use the cheap `process_running`.
The ps cost (~2 ms per call) is paid at most once per 100 ms poll
interval and only while the server process still exists. In a normal
clean shutdown that's zero calls (process exits before the first
poll). In the zombie scenario the loop exits after ~1 poll instead
of running out the full timeout.
Verified on this branch:
cargo nextest run -p fabro-cli -E 'test(foreground_start_writes_tracing)'
before: 10.48s, 10.45s, 10.42s
after: 0.35s, 0.32s, 0.25s (30x faster)
The zombie regression test removed in commit da87f978c returns as
`process_running_strict_returns_false_for_unreaped_zombie_child`,
and also asserts that the cheap `process_running` keeps its
"zombie == alive" semantics so the harness hot paths stay honest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Capture the technical plan for extracting a new fabro-client crate from
fabro-cli, lifting domain DTOs (RunSummary, EventEnvelope, RunProjection,
ArtifactUpload) to fabro-types, and applying a set of OOP-style naming
cleanups along the way. Ran through ce-plan deepening and document-review
with feedback integrated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Delete the CliTargetTls settings, ClientTlsSettings struct, rustls-pemfile
dep, and the fabro-http wrapper methods (use_rustls_tls/identity/
add_root_certificate) that only the CLI client-auth path used. The server
no longer terminates TLS in-process and the CliAuthStrategy::Mtls variant
had no construction or match sites.
Also simplify ServerTarget::HttpUrl to a tuple variant (HttpUrl(String))
now that tls is gone, removing the struct-variant ceremony across 22
construction and destructure sites.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clippy (match_same_arms) on the Step C rewrite: Ok(false) and Err(_)
both mean "treat as alive", so expressing it as `if matches!(..., Ok(true))`
reads cleaner and satisfies the lint.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The probe helper served its purpose in narrowing the recent per-test
setup regression to `reap_stale_session_roots`. Remove it now that the
underlying cause (process_running shelling out to `ps`) is fixed and
the reap is amortized to once per process. The plan was to carry it
through verification so Step B could quote reap_nextest numbers, then
drop it — this commit is that drop.
This reverts commit 24e7e5af8.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace PID-based liveness probing in `live_marker_count` with flock
advisory-lock presence detection. Each test process opens
`<session_root>/clients/<pid>` once, holds LOCK_SH for the lifetime
of any live TestContext in the process, and releases it explicitly
when `cleanup_session_root` fires at refcount zero. Reapers probe with
LOCK_EX | LOCK_NB: success means the previous owner is gone (normal
exit, panic, SIGKILL, or zombie — the kernel releases advisory locks
at process exit in every case) and the stale marker is removed.
Compared to the PID check this was replacing:
- Handles PID recycling correctly (the new holder does not inherit
the previous owner's advisory lock).
- Handles zombies correctly without shelling out to `ps`.
- Costs one open + one flock per peer, ~50 us on macOS.
The marker handle is stored in a process-scoped
`Mutex<Option<(PathBuf, File)>>` so it can be released and
reacquired across the drop-to-zero / rise-from-zero cycles that
`session_refs` already implements. Storing the path alongside the
handle enables a debug assertion that the process never drifts
between session roots.
`ClientMarker` and its serde plumbing are removed; the marker file is
now empty, its existence and lock state carrying the signal.
Full workspace wall-clock after A+B+C: 13.3–13.6 s, down from 20–25 s
on HEAD before the fix and comparable to the 14 s Friday baseline
despite the intervening +85 tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
reap_stale_session_roots cleans up session roots left over by prior
nextest runs that crashed. TestContext::new called it twice per test
(once per SessionMode). Under a 721-test fabro-cli suite that was
~1400 reap calls where one would do, accounting for several seconds of
per-suite overhead even after the process_running regression was
reverted.
Gate each call behind a per-process OnceLock so at most one reap runs
per SessionMode per test binary. The reap itself (and its internal
per-root session lock, which iterates candidate roots) is unchanged;
we just stop re-entering it for every TestContext::new.
Measured on fabro-cli after this change:
reap_nextest probes: 356 calls, 302 under 1 ms (OnceLock fast path),
sum 1.4 s (down from Friday's 3.1 s and HEAD's 88.6 s before the
process_running revert).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit 1ed8e6cbd changed process_running(pid) to shell out to `ps` on
every call to distinguish running processes from zombies. That cost
~2 ms per invocation on macOS (fork + exec + wait), and the test
harness calls process_running O(tests × markers) times under session
flock contention. Across a `cargo nextest run -p fabro-cli` that added
up to ~90 s of suite time, and the zombie-aware semantics turned out
to have no production caller on Unix (the server's worker-termination
loop uses process_group_alive; the CLI stop/status paths don't need
zombie detection for a daemon that reparents to init).
Restore the pre-1ed8e6cbd body: process_running is now a straight
kill(pid, 0) via process_exists on Unix, true on non-unix. Delete
unix_process_state (the `ps` helper) and its zombie regression test,
since they describe behavior we're rolling back. process_group_alive
and its tests are unchanged.
Measured on this branch against baseline db953c838:
reap_nextest p50: 172 ms -> 0.3 ms
TestContext:🆕 316 ms mean -> 15 ms mean
If a future caller genuinely needs zombie-aware semantics, add it back
alongside that caller with a benchmark in context.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Gated test-harness diagnostic. Writes one tab-separated line per phase
of TestContext::new to the path named by FABRO_TEST_PROBE_LOG, using an
O_APPEND+single-write-per-line pattern so concurrent test processes do
not interleave. Disabled when the env var is unset.
Used to isolate the source of a recent test-suite slowdown; removed
again at the end of the same change set once verification is done.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two CLI/server tests racing against peer state on the shared fabro server
session, surfaced by running the default nextest profile 20 times.
pr_list_missing_github_credentials_errors depended on an empty shared
store; if pr_view_reads_pull_request_from_store_without_pull_request_json
ran first it left a PR record behind and this test hit the
credentials-required branch instead of "No pull requests found." The
snapshot captured the empty path, but the test name promises the error
path. Seed a PullRequestCreated event against the test's own run so the
store is guaranteed non-empty and the credentials-required error fires
deterministically.
full_http_lifecycle_cancel asserted that the cancel response body's
pending_control == "cancel", but that field is re-read from the store
projection after the worker has been signaled. The worker is sitting at
a human gate; on hot CI it can emit a clearing event before the handler
re-reads the projection, yielding a legitimate null. Relax the
assertion to accept "cancel" or null; durable convergence to
failed/cancelled is still asserted below.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Finish the rename started with the Client alias: drop the alias and use
the Client name directly for the server-facing CLI client struct.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dedupe bearer-token extraction and the trailing from_bundle construction
in the managed Unix-socket connect path, return Bind from the
ensure_server_running_on_socket helper to match its sibling, and drop
"target" from its overlong name.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ensure configured Unix socket targets still autostart on their requested
socket path during concurrent connection races, instead of falling back to
the storage-owned default bind.
Tighten CLI loopback target classification to use literal host checks,
update explicit local TCP auth coverage to match the remote-target
contract, and align server CSP assertions with the current external-script
SPA bundle. Also enable reqwest cookies in fabro-http so package-scoped
server tests compile without relying on workspace feature unification.
Consolidate three copies of `normalized_http_base_url` and
`build_public_http_client` into shared helpers in `user_config`,
add `Display for ServerTarget`, drop stale `#[allow(dead_code)]`
markers now that login/logout/JWT are wired, remove dead
`LOGIN_SUCCESSFUL` and `_error_description` field, and gate
test-only helpers behind `#[cfg(test)]`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the dark-only refactor the embedded SPA no longer contains an
inline theme-bootstrap script, so the two tests that asserted "embedded
index has >= 1 inline <script>" and "CSP header contains 'sha256-'" now
fail. The guards existed to catch accidental loss of the bootstrap
script; that loss was intentional. The rest of the CSP machinery (hash
extraction, policy assembly, external-script handling) is still
exercised by the remaining unit tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop dead http_client() accessor and its allow(dead_code).
- Scope map_api_error to module-private; all call sites are in-file.
- Rename test helper test_api_client to test_client to match what it returns.
- Run system df's two independent server GETs concurrently with try_join!.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Harden the CLI browser auth flow by moving auth-code issuance behind
an explicit same-origin confirmation step, and update the real-browser
test harness to submit the confirmation page.
Removes unused annotateRunningNodes from run-graph and inlines the
const gt = graphTheme / const theme = graphTheme shims left over from
the dark-mode-only refactor. Template strings reference graphTheme
directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Wrap the remaining CLI server API calls in server_client::Client,
remove the api/connect_api_client escape hatches, and migrate
model/install/tests to the new facade.
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>
Removes the light/dark toggle infrastructure in favor of a single dark
theme. Deletes the theme context, boot script, light-mode CSS overrides,
logotype-light asset, and the pierre-light diff theme. Collapses
graph-theme into a single constant. Adds scheme-only-dark on <html> so
native controls and the server-injected Graphviz @media query render
dark.
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.
Add the missing refresh transport guard, actionable auth-store lock errors
for unsupported filesystems, explicit OAuth state expiry, and the remaining
CLI auth regression coverage around replay revocation, HTML headers, and
secret-safe logging.
The CLI façade is the primary type callers reach for, so it deserves
the bare `Client` name (per `reqwest::Client`, `hyper::Client`
convention). The raw generated HTTP binding is secondary and is more
accurately named `ApiClient`. "Store" in `ServerStoreClient` was
leftover from the SlateDB-ownership refactor and no longer describes
the type.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
