Replace 16 inline copies of the `[cli.target] type = "http"` settings TOML across CLI integration tests with a single `set_http_target(&base_url)` method on `TestContext`. Removes a brittle format string that was maintained in ten files but only meaningfully asserted-against in one.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Persist dev-token credentials in auth.json alongside OAuth entries so CLI targets resolve credentials consistently across TCP and Unix socket flows.
Move install-time token minting to runtime storage, add auth login --dev-token, and refresh the embedded SPA after updating the stale dev-token hint.
Add fabro-static::EnvVars as the shared registry for fixed environment variable names and migrate env reads, clap env bindings, and subprocess/test allowlists to use it.
Add clippy bans for raw std::env lookup APIs so future dynamic env facades must be documented explicitly.
`Bind` and `ServerDaemon` are serde-serialized descriptions of on-disk
server state (the `server.json` record). They belong with
`RuntimeDirectory` in fabro-config rather than in fabro-server's web
layer.
The practical payoff: fabro-test was hand-parsing `server.json` via
`serde_json::Value["pid"]` because fabro-server already depends on
fabro-test (cycle blocked the reverse edge). Moving these types into
fabro-config lets fabro-test call `ServerDaemon::{load_running, read,
remove}` directly, dropping ~20 lines of duplicated record parsing.
fabro-config gains `fabro-proc` and `tempfile` as deps to cover
`ServerDaemon::{is_running, write}`. All 16 `fabro_server::{bind,
daemon}` import sites in fabro-server and fabro-cli are rewritten to
`fabro_config::{bind, daemon}`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The dev-token gating commit added ensure_home_server_auth_methods only to
run_cmd/create_cmd helpers, but many integration tests use context.command()
directly to invoke run/start/attach/etc. Patch the offenders rather than
hoisting auth-injection into command() itself, since command() is also used
by tests (e.g. uninstall) that explicitly want a stable settings file.
- attach, start, scenario lifecycle/recovery, json_global graph: call
context.ensure_home_server_auth_methods() up front
- validate(): hoist into the helper itself, since every validate test
needs it
- server_status, uninstall legacy-record tests: bake methods=["dev-token"]
into their hand-written settings.toml fixtures and pass FABRO_DEV_TOKEN
via env so the spawned server actually boots
- install: write_artifact_store_metadata_creates_marker test fixture also
needs explicit methods after the resolver became strict
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add FABRO_SUPPRESS_OPEN_BROWSER env knob via fabro_util::browser::try_open.
apply_test_isolation now sets it, so install-mode and auth-login tests that
spawn a real fabro binary no longer pop real browser windows. All six
open::that call sites route through the helper; consolidates the direct
open crate dep into fabro-util.
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>
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>
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 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.
Enable clippy::allow_attributes_without_reason at the workspace level.
Add concise, callsite-specific reasons to existing allow attributes, including generated code paths.
Nothing behavioral — each change is what clippy asked for:
- fabro-test: wrap the three polling-helper thread::sleep calls in a
single poll_sleep() with an #[expect(clippy::disallowed_methods,
reason = …)] since the helpers are deliberately blocking
- fabro-test: server_log_files now uses Path::extension() with
eq_ignore_ascii_case("log") instead of a case-sensitive ends_with
- fabro-workflow: import default_storage_dir rather than calling it
through its full module path
- fabro-cli/server/record: same absolute_paths fix
- fabro-cli/main tests: use a `use tokio::runtime::Runtime` to stop
referencing `tokio::runtime::Runtime` by full path
- fabro-cli/tests: replace three `as u32` casts on as_u64() results
with u32::try_from(...).expect(…)
- fabro-cli/tests: six `format!("...", var)` assertions switched to
the inline `{var}` form clippy prefers
Full verification passes: fmt, clippy, cargo nextest (4141 tests),
bun typecheck, bun test (40 tests), bun build, SPA embed diff clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2/3 of the std::fs lint initiative (Phase 1 refactors landed in
commit 9d1c0d98c).
clippy.toml additions (appended to disallowed-methods):
std::fs::read, read_to_string, write, read_dir, copy, canonicalize
std::fs::File::open, File::create, File::create_new
std::fs::OpenOptions::open
File::options was deliberately excluded — it returns an OpenOptions
builder with no syscall. OpenOptions::open is where the block happens.
Non-blocking std::fs items (metadata, exists, create_dir_all, remove_*,
rename, and all std::fs types) remain legal.
Annotation policy (per updated plan):
- Mixed async/sync production source: function- or statement-scoped
#[expect(...)] so future accidental Tokio-path regressions in the
same file still fire.
- Fully-sync production source, test modules, integration tests,
build.rs: file-level #![expect(...)].
- Every #[expect] has a specific reason identifying the sync context.
Annotations added in ~90 files across the workspace. Notable narrow
placements: fabro-server server.rs current_server_target,
build_disk_usage_response, create_test_app_state_with_session_key;
fabro-server install.rs read_to_string rollback snapshot;
fabro-sandbox local.rs list_recursive; fabro-agent cli.rs FOLLOW-UP on
the JSON-stdout writer; fabro-llm providers/common.rs FOLLOW-UP for
load_file_as_base64 (7 translator call sites; revisit if file:// URL
usage grows).
build.rs blanket allows: fabro-api/build.rs, fabro-util/build.rs.
Pre-existing unrelated nightly-clippy warnings fixed under scope:
fabro-sandbox sandbox_spec.rs (unused_imports, unused_async),
reconnect.rs (unused_variables, unused_async).
Verified: cargo +nightly-2026-04-14 clippy --workspace --all-targets
-- -D warnings passes; fmt clean; 4129/4131 tests pass (two known
flakes under parallel nextest load, both pass individually and are
unrelated to this change).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move wait_for_path, wait_for_log_line, stop_pid, server_log_files, and
isolated_storage_dir out of the three integration test files that duplicated
them and into fabro-test's public surface next to apply_test_isolation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implement the web-first install experience across the server, CLI, API spec,
web app, and packaged SPA assets.
This also removes test-side process env mutation by pushing env-dependent
decision points behind explicit helpers and test wiring.
Integration tests spawn the fabro binary as a subprocess and call
env_clear() for isolation, which strips LLVM_PROFILE_FILE. Under
cargo-llvm-cov this dropped subprocess coverage into orphaned
default.profraw files in tempdirs instead of the merged profile.
Add a preserve_coverage_env! macro in fabro-test and call it after
each env_clear() in apply_test_isolation, LightweightCli, and the
exec.rs sites. No-op when the env var is unset (normal test runs).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Non-release builds now append the profile to `fabro --version`
(`x.y (sha date debug)`), `fabro version`, and `fabro system info`,
so users can tell a local build apart from a shipped release. The
API's `SystemInfoResponse` gains a `profile` field so the client
can render the server's build profile too.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLI integration tests spawned the real fabro binary while letting the
parent process's env pass through. The pr_view "no credentials" snapshot
failed in CI because the Nightly workflow's minted GITHUB_TOKEN was
inherited by the child and turned the expected "credentials required"
error into a real GitHub API call (404 / 401). On developer laptops the
same leak occurs whenever gh auth login is active.
Introduce apply_test_isolation(cmd, home) in fabro-test: env_clear() +
re-populate PATH, HOME, NO_COLOR, and the FABRO_* test overrides. Route
TestContext::command(), the internal server bootstrap, and the four
ad-hoc spawners in tests/it/cmd/{attach,render_graph,runner,server_start}
through the same helper so the isolation is systemic instead of
per-callsite. Tests that deliberately need a credential (OPENAI_API_KEY,
GITHUB_APP_PRIVATE_KEY, etc.) continue to set it explicitly on the
returned Command; those survive the clear.
Add a regression test that sets sentinel GITHUB_TOKEN and
ANTHROPIC_API_KEY in the parent, spawns /usr/bin/env through the helper,
and asserts the child sees neither credential while still seeing PATH
and the harness's FABRO_NO_UPGRADE_CHECK override.
Verified: the full workspace (4022 tests) passes with GITHUB_TOKEN and
ANTHROPIC_API_KEY set in the parent, which previously broke the
pr_view_reads_pull_request_from_store_without_pull_request_json
snapshot. cargo fmt and nightly clippy are clean.
Move the shared session lock file out of the deletable session root so
cleanup cannot unlink the lock another test process is relying on.
This hardens the nextest shared-server harness against dev-token startup
races and adds a regression test for the lock path.
In release builds with SEGMENT_WRITE_KEY baked in (i.e. CI), the
post-command telemetry flush calls spawn_fabro_subcommand, which in turn
does create_dir_all(~/.fabro/tmp) and writes a JSONL event file. That
silently undoes the directory removal that `fabro uninstall --yes` just
performed — leaving a stray ~/.fabro/tmp/ behind and breaking the
uninstall integration tests on CI release runs.
Fixes:
- run_uninstall calls fabro_telemetry::shutdown() before removal so the
buffered "CLI Executed" track in main() can't be queued or flushed,
and the background thread can't spawn the sender subprocess.
- TestContext::command() exports FABRO_TELEMETRY=off so test subprocesses
never initialise telemetry at all, as a belt-and-suspenders guard.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Update VERSION filter regex to handle prerelease suffixes (e.g., 0.204.0-beta.1)
- Add VERSION filter to JSON snapshots in fabro_json_snapshot macro
- Fix attach test to use [VERSION] placeholder instead of hardcoded version
- Update upgrade help snapshot to include new --prerelease flag
- Change fake version in upgrade test from v0.176.3 to v999.0.0 to avoid collision
- Strip --watch-web from server start help (debug-only flag, varies by build)
- Gate test_panic module with #[cfg(debug_assertions)] (debug-only command)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Config discovery walks from the workflow file's parent directory, so
tests using fixtures at their repo path (test/simple.fabro) would find
the repo's .fabro/project.toml. This caused settings like preserve=true
to leak into tests and break sandbox cleanup event assertions.
Add TestContext::install_fixture() which copies fixtures into the test's
temp dir. Update all CLI run/attach/start tests to use it. Remove the
now-unused example_fixture() function.
Restore preserve=true in .fabro/project.toml — tests are now isolated.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the old strategy matrix with server.auth.methods, browser session
cookies, and raw dev-token bearer auth. Remove mTLS auth leftovers, auto-
provision local session secrets, and update tests and docs to the new auth
surface.
Move async subprocess paths to Tokio or spawn_blocking, document the
intentional synchronous std::process::Command callsites, and make CI run
Clippy with --all-targets so the guardrail applies to test code too.
Add a Clippy disallowed-methods guardrail for std::thread sleep/spawn
and convert the CLI polling paths to tokio::time::sleep so they no
longer block Tokio workers. Keep the intentional OS-thread sites with
narrow #[expect(...)] annotations that explain why std::thread is
required there.
Add the shared fabro-http transport crate and route hand-written HTTP client construction through it.
Use FABRO_HTTP_PROXY_POLICY for test no-proxy defaults, remove direct reqwest deps from ordinary crates, and add clippy bans for raw reqwest entrypoints.
No production deployments exist, so there's no need for migration shims.
Remove all six backwards-compat type aliases (AgentError, SdkError,
CoreError, GraphvizError, StoreError, FabroError) and migrate ~880
callsites to use the canonical Error name directly within each crate,
or qualified imports (e.g., `use fabro_llm::Error as LlmError`) for
cross-crate references. Also fix a pre-existing absolute-path clippy
lint in fabro-server error.rs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TestContext::command() was inheriting all parent env vars, so a
developer (or CI) running with FABRO_CONFIG set would pollute child
test subprocesses, causing settings_local_* IT tests to fail with
opaque assertion errors.
Iterate std::env::vars_os() and env_remove every FABRO_* key before
re-adding the controlled set (FABRO_NO_UPGRADE_CHECK, etc.). Safe to
iterate because the prior two commits eliminated all std::env::set_var
callers in fabro-cli and fabro-config tests.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Close out consumer migration with targeted behavior fixes and the
remaining integration-test fixture rewrites. The full workspace
nextest run now reports 3,760 passed / 0 failed / 182 skipped.
Runtime fixes:
- effective_settings::apply_server_defaults now propagates the full
server-side Settings shape (llm, sandbox, setup, checkpoint,
pull_request, artifacts, hooks, mcp_servers, github, slack, fabro)
into the resolved CLI settings, matching the pre-Stage-3 'merge
everything server' behavior for RemoteServer/LocalDaemon modes
- fabro-cli commands/run/overrides: route --verbose through
cli.output.verbosity = verbose instead of a run.metadata stash,
so it resolves to settings.verbose via the bridge
- fabro-server run_manifest manifest_args_layer: same — emit a
CliLayer with cli.output.verbosity rather than stuffing the flag
into run.metadata
- fabro-test settings_storage_dir: detect the managed marker and
return None instead of parsing the injected server.storage.root,
so isolated_server correctly spins up a new storage dir
- fabro-server run_manifest_local_daemon test now passes with full
server-side settings snapshot propagation
Test fixture + assertion updates:
- cmd::config::settings_local_explicit_workflow_path_uses_workflow_project_layers:
assertion updated for v2 R30 whole-list replacement of
run.prepare.steps across layers (only workflow-setup survives)
- cmd::config::create_explicit_workflow_path_uses_project_config_relative_to_workflow:
same correction for the persisted run.settings.setup.commands
- cmd::attach::attach_json_errors_without_prompting_for_human_input
and cmd::run::json_run_implies_auto_approve_for_human_gates: strip
the bridge-emitted settings.server and settings.version fields from
the JSON snapshot so the randomised unix-socket path does not flap
the insta snapshot
- cmd::server_start::concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up:
rewrite the injected settings.toml to v2 shape with
[server.storage] root and [cli.target] type = unix path
- scenario::smoke::attach_smoke_covers_arg_validation_and_remote_server_behaviors:
two [server] target fixtures rewritten to [cli.target]
type = http url
Accepted insta snapshots for attach and run JSON outputs. Workspace
build + clippy both clean under -D warnings.
Stage 3 of the settings TOML redesign. Switches the core parse/merge/
resolve path to the v2 namespaced schema while keeping the legacy flat
Settings shape accessible via the bridge for not-yet-migrated consumers.
Parser and layering:
- ConfigLayer is now a newtype around v2 SettingsFile. Loading via
ConfigLayer::parse/load/settings/for_workflow/project now hard-fails
on legacy top-level keys (version, llm, vars, sandbox, etc.) with
targeted rename hints emitted by fabro_types::settings::v2::tree
- new fabro_config::merge module encodes the merge matrix directly:
replace-by-default maps, sticky merge for run.sandbox.env and
provider-native labels, splice-aware string arrays for
run.model.fallbacks and notification route events, whole-list
replacement for run.prepare.steps, field-merge keyed objects for
notifications/MCPs/web-auth providers, and ordered hook id-aware
replacement
- ConfigLayer::resolve delegates to fabro_types::settings::v2::bridge
so consumers keep reading through the legacy Settings shape until
Stage 4 migrates them off it
- effective_settings::resolve_settings now treats project/workflow/
run/features as shared layered domains and strips cli/server from
non-local layers before merging, fulfilling the owner-first trust
boundary rule
Consumer migration (Stage 4 preview, kept to the files that block
the workspace build):
- fabro-server run_manifest builds v2 RunLayer from ManifestArgs and
resolves manifest dockerfile references through the v2 sandbox
daytona snapshot tree
- fabro-cli manifest_builder consults run.goal via v2; user_config
writes the v2 server.storage.root field under the CLI storage-dir
override; run/overrides constructs a v2 RunLayer from RunArgs
- fabro-cli scaffolds (repo init, workflow create) emit _version = 1
with project.directory/workflow.graph/run.sandbox etc.
fabro-config / fabro-types legacy parse-time types (ProjectConfig,
LlmConfig, SandboxConfig, PullRequestConfig, ExecConfig, SettingsFile
try_into, etc.) are deleted from the parse path; the resolved type
re-exports (LlmSettings, SandboxSettings, etc.) remain as shims so
unmigrated consumers keep compiling.
fabro-test helper: settings.toml fixtures now use _version = 1 plus
[server.storage] root and [cli.target] type = "unix" path. Legacy
flat storage_dir/server.target handling removed from the sync path.
Known Stage 4/5 follow-ups:
- fabro-cli integration test fixtures still use legacy-shape TOML
(version = 1, [llm], [sandbox], [vars], [exec], [fabro], etc.);
tests currently fail to parse against the v2 schema as intended.
Migrating them is the bulk of Stage 4 and lands in subsequent
commits.
- OpenAPI ServerSettings schema, generated clients, apps/fabro-web
workflowData fallback, and docs/reference examples are unchanged
and land in Stage 5.
- fabro-types: remove redundant "freeform" match arm (match_same_arms)
- fabro-server: use let...else and remove needless return
- fabro-cli/runner: use while-let instead of match loop, unwrap Option
from build_artifact_uploader return type
- fabro-cli/attach: introduce AttachOptions struct to reduce bool
parameter count (fn_params_excessive_bools)
- fabro-test: fix unused variable and needless continue in session lock
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two flake sources identified across 100+ full-suite runs:
1. Session lock EINVAL race: cleanup_session_root's remove_dir_all
could delete the session root between with_session_lock's
create_dir_all and File::create, causing EINVAL. Fix: retry the
create-dir + create-file sequence as a unit.
2. mTLS cert generation: openssl req -key /dev/stdin failed under fd
pressure with "Bad file descriptor". Fix: read from the already-
written server.key file path instead of piping through /dev/stdin.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test harness waited 8s for the server to shut down gracefully,
accommodating the server's 5s WORKER_CANCEL_GRACE. But in tests,
the CLI returns before workers exit (terminal SSE event → CLI exits →
TestContext drops → SIGTERM while workers still cleaning up), so the
last test in every session paid a ~5s penalty. No real work needs
preserving in tests, so SIGKILL after 500ms instead.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
