From e37d20bd6fe1b4333ea43ceac86e21328a13fb7c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 07:55:32 -0400 Subject: [PATCH 1/5] test fix --- lib/crates/fabro-test/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index b034f11e1..5107dfdf9 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -1737,7 +1737,10 @@ mod tests { fn run_and_create_commands_include_test_labels() { let _lock = env_lock().lock().expect("env lock poisoned"); let _guard = EnvGuard::set("NEXTEST_RUN_ID", Some("run-cmd-labels")); - let context = TestContext::new(PathBuf::from("/tmp/fabro")); + let missing_bin_root = tempfile::tempdir().expect("failed to create temp dir"); + // Keep the binary path missing so this unit test never bootstraps a + // shared server based on ambient filesystem state at /tmp/fabro. + let context = TestContext::new(missing_bin_root.path().join("fabro")); let run_args = context .run_cmd() From 90b373c3668b0ddbbfa4c159e84e7f94a3b1e00a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 10:13:17 -0400 Subject: [PATCH 2/5] fix(test): eliminate recovery, cancel, and label flakes Stabilize the recovery scenario around rebuilt metadata timing and node ordinals, make in-process run cancellation converge on a cancelled reason, and keep the label assertion unit test out of the shared TestContext session lifecycle. --- .../fabro-cli/tests/it/scenario/recovery.rs | 82 ++++++++++++++----- lib/crates/fabro-server/src/server.rs | 12 ++- .../tests/it/scenario/lifecycle.rs | 22 ++++- lib/crates/fabro-test/src/lib.rs | 21 +++-- 4 files changed, 103 insertions(+), 34 deletions(-) diff --git a/lib/crates/fabro-cli/tests/it/scenario/recovery.rs b/lib/crates/fabro-cli/tests/it/scenario/recovery.rs index 82f37470d..12dfc57bf 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/recovery.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/recovery.rs @@ -5,7 +5,7 @@ use fabro_checkpoint::branch::BranchStore; use fabro_checkpoint::git::Store as GitStore; use fabro_test::{fabro_snapshot, test_context}; use fabro_types::Checkpoint; -use fabro_workflow::operations::build_timeline; +use fabro_workflow::operations::{RunTimeline, build_timeline}; use git2::{Repository, Signature}; use crate::support::unique_run_id; @@ -58,16 +58,59 @@ fn latest_metadata_checkpoint(repo_dir: &Path, run_id: &str) -> Checkpoint { } fn timeline_run_shas(repo_dir: &Path, run_id: &str) -> Vec> { - let repo = Repository::discover(repo_dir).unwrap(); - let store = GitStore::new(repo); - build_timeline(&store, run_id) - .unwrap() + build_timeline_when_ready(repo_dir, run_id) .entries .into_iter() .map(|entry| entry.run_commit_sha) .collect() } +fn timeline_node_names(repo_dir: &Path, run_id: &str) -> Vec { + build_timeline_when_ready(repo_dir, run_id) + .entries + .into_iter() + .map(|entry| entry.node_name) + .collect() +} + +fn build_timeline_when_ready(repo_dir: &Path, run_id: &str) -> RunTimeline { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + loop { + let repo = Repository::discover(repo_dir).unwrap(); + let store = GitStore::new(repo); + match build_timeline(&store, run_id) { + Ok(timeline) => return timeline, + Err(err) => { + assert!( + std::time::Instant::now() < deadline, + "timeline for {run_id} never became readable: {err}" + ); + std::thread::sleep(std::time::Duration::from_millis(50)); + } + } + } +} + +fn delete_metadata_branch_when_ready(repo_dir: &Path, run_id: &str) { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + loop { + let repo = Repository::discover(repo_dir).unwrap(); + let mut reference = repo + .find_reference(&format!("refs/heads/fabro/meta/{run_id}")) + .unwrap(); + match reference.delete() { + Ok(()) => return, + Err(err) => { + assert!( + std::time::Instant::now() < deadline, + "metadata branch for {run_id} never became writable: {err}" + ); + std::thread::sleep(std::time::Duration::from_millis(50)); + } + } + } +} + fn init_repo_with_workflow(repo_dir: &Path) { std::fs::write(repo_dir.join("README.md"), "recovery test\n").unwrap(); std::fs::write( @@ -142,12 +185,7 @@ fn rewind_and_fork_recover_missing_metadata_from_real_run_state() { filters.push((r"\b[0-9a-f]{7,40}\b".to_string(), "[SHA]".to_string())); filters.extend(context.filters()); - Repository::discover(repo_dir.path()) - .unwrap() - .find_reference(&format!("refs/heads/fabro/meta/{source_run_id}")) - .unwrap() - .delete() - .unwrap(); + delete_metadata_branch_when_ready(repo_dir.path(), &source_run_id); assert!( list_metadata_run_ids(repo_dir.path()).is_empty(), @@ -158,15 +196,14 @@ fn rewind_and_fork_recover_missing_metadata_from_real_run_state() { rewind_list.current_dir(repo_dir.path()); rewind_list.args(["rewind", &source_run_id, "--list"]); rewind_list.timeout(std::time::Duration::from_secs(15)); - fabro_snapshot!(filters.clone(), rewind_list, @" - success: true - exit_code: 0 - ----- stdout ----- - ----- stderr ----- - @ Node Details - @1 plan - @2 build - "); + rewind_list.assert().success(); + + let rebuilt_nodes = timeline_node_names(repo_dir.path(), &source_run_id); + assert_eq!(rebuilt_nodes.last().map(String::as_str), Some("build")); + assert!( + rebuilt_nodes.ends_with(&["plan".to_string(), "build".to_string()]), + "expected rebuilt timeline to end with plan -> build, got {rebuilt_nodes:?}" + ); let rebuilt_checkpoints = metadata_checkpoints(repo_dir.path(), &source_run_id); assert_eq!( @@ -202,17 +239,18 @@ fn rewind_and_fork_recover_missing_metadata_from_real_run_state() { regex::escape(&source_run_id[..8]), "[RUN_PREFIX]".to_string(), )); + rewind_filters.push((r"@\d+".to_string(), "@[ORDINAL]".to_string())); let mut source_rewind = context.command(); source_rewind.current_dir(repo_dir.path()); - source_rewind.args(["rewind", &source_run_id, "@2", "--no-push"]); + source_rewind.args(["rewind", &source_run_id, "build", "--no-push"]); source_rewind.timeout(std::time::Duration::from_secs(15)); fabro_snapshot!(rewind_filters, source_rewind, @" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- - Rewound metadata branch to @2 (build) + Rewound metadata branch to @[ORDINAL] (build) Rewound run branch fabro/run/[ULID] to [SHA] To resume: fabro resume [RUN_PREFIX] diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index ad0d0ffb4..3d22c4a01 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -5182,11 +5182,17 @@ async fn cancel_run( if let Some(token) = &cancel_token { token.store(true, Ordering::SeqCst); } - if let Some(cancel_tx) = cancel_tx { + let sent_cancel_signal = if let Some(cancel_tx) = cancel_tx { let _ = cancel_tx.send(()); - } + true + } else { + false + }; if let Some(answer_transport) = answer_transport { - let _ = answer_transport.cancel_run().await; + if !(sent_cancel_signal && matches!(answer_transport, RunAnswerTransport::InProcess { .. })) + { + let _ = answer_transport.cancel_run().await; + } } if let Some(worker_pid) = worker_pid { #[cfg(unix)] diff --git a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs index a1286253b..5d8849e5e 100644 --- a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs +++ b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs @@ -69,6 +69,24 @@ async fn wait_for_question(app: &axum::Router, run_id: &str) -> serde_json::Valu panic!("question should have appeared"); } +async fn wait_for_run_state( + app: &axum::Router, + run_id: &str, + expected_status: &str, + expected_reason: &str, +) -> serde_json::Value { + for _ in 0..POLL_ATTEMPTS { + let body = run_json(app, run_id).await; + if body["status"].as_str() == Some(expected_status) + && body["status_reason"].as_str() == Some(expected_reason) + { + return body; + } + sleep(POLL_INTERVAL).await; + } + panic!("run {run_id} did not reach status={expected_status} reason={expected_reason}"); +} + const GATE_DOT: &str = r#"digraph GateTest { graph [goal="Test gate"] start [shape=Mdiamond] @@ -197,8 +215,6 @@ async fn full_http_lifecycle_cancel() { assert_eq!(body["pending_control"], "cancel"); // Verify the durable store view converges to cancelled failure. - let status = wait_for_run_status(&app, &run_id, &["failed"]).await; - assert_eq!(status, "failed"); - let body = run_json(&app, &run_id).await; + let body = wait_for_run_state(&app, &run_id, "failed", "cancelled").await; assert_eq!(body["status_reason"], "cancelled"); } diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 5107dfdf9..183351fcc 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -1735,12 +1735,21 @@ mod tests { #[test] fn run_and_create_commands_include_test_labels() { - let _lock = env_lock().lock().expect("env lock poisoned"); - let _guard = EnvGuard::set("NEXTEST_RUN_ID", Some("run-cmd-labels")); - let missing_bin_root = tempfile::tempdir().expect("failed to create temp dir"); - // Keep the binary path missing so this unit test never bootstraps a - // shared server based on ambient filesystem state at /tmp/fabro. - let context = TestContext::new(missing_bin_root.path().join("fabro")); + let context_root = tempfile::tempdir().expect("failed to create temp dir"); + let context = TestContext { + temp_dir: context_root.path().join("temp"), + home_dir: context_root.path().join("home"), + storage_dir: context_root.path().join("storage"), + test_case_id: "case-123".to_string(), + test_run_id: "run-cmd-labels".to_string(), + session_root: context_root.path().join("session"), + fabro_bin: context_root.path().join("fabro"), + filters: Vec::new(), + active_socket_path: context_root.path().join("fabro.sock"), + isolated_server: None, + managed_storage_dirs: Vec::new(), + _context_root: context_root, + }; let run_args = context .run_cmd() From 9ae606cfc1b7eeda7b08a4763b9f1b8dff96bb45 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 12:51:34 -0400 Subject: [PATCH 3/5] fix(test): use no_proxy client in detach signal test to prevent macOS timeout flakes Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/run/attach.rs | 5 +---- lib/crates/fabro-cli/src/server_client.rs | 12 ++++++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/run/attach.rs b/lib/crates/fabro-cli/src/commands/run/attach.rs index 9ebb1eb9c..417cf80b6 100644 --- a/lib/crates/fabro-cli/src/commands/run/attach.rs +++ b/lib/crates/fabro-cli/src/commands/run/attach.rs @@ -580,10 +580,7 @@ mod tests { .header("Content-Type", "application/json") .body(terminal_run_state_response().to_string()); }); - let client = - server_client::connect_server_target_direct(&format!("{}/api/v1", server.base_url())) - .await - .unwrap(); + let client = server_client::ServerStoreClient::new_no_proxy(&server.base_url()).unwrap(); handle_detach_signal(&client, &run_id, true).await; diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 4e8234dbd..14b86167e 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -272,6 +272,18 @@ struct ArtifactBatchUploadEntry { } impl ServerStoreClient { + /// Build a client for tests that bypasses proxy discovery. + #[cfg(test)] + pub(crate) fn new_no_proxy(base_url: &str) -> Result { + let http_client = cli_http_client_builder().no_proxy().build()?; + let client = fabro_api::Client::new_with_client(base_url, http_client.clone()); + Ok(Self { + client, + http_client, + base_url: base_url.to_string(), + }) + } + pub(crate) fn clone_for_reuse(&self) -> Self { self.clone() } From 35fac654c3a283e95468fcc5635d9024b78d73b2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 14:29:00 -0400 Subject: [PATCH 4/5] fix(test): reduce test server stop timeout from 8s to 500ms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- lib/crates/fabro-test/src/lib.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 183351fcc..28aab0c78 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -596,9 +596,11 @@ fn stop_test_server(server: &ServerPaths) { } fn test_server_stop_timeout() -> std::time::Duration { - // Allow the real server to finish its own 5s worker-shutdown grace before - // we escalate and risk orphaning active run workers. - std::time::Duration::from_secs(8) + // Give the server a brief window to flush state, then escalate. + // The server's own 5s worker-shutdown grace is unnecessary in tests + // because no real work needs preserving — any lingering workers are + // from already-completed runs racing to exit. + std::time::Duration::from_millis(500) } fn shared_server_paths(root: &Path) -> ServerPaths { @@ -1772,10 +1774,10 @@ mod tests { } #[test] - fn stop_test_server_timeout_exceeds_server_worker_grace() { + fn stop_test_server_timeout_is_short() { assert!( - test_server_stop_timeout() >= std::time::Duration::from_secs(6), - "test harness must give the server longer than its 5s worker shutdown grace" + test_server_stop_timeout() <= std::time::Duration::from_secs(1), + "test harness should SIGKILL quickly — no real work to preserve" ); } From 15ccf9aef9528c59f8f9b66b0f773b02a937cbdd Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 8 Apr 2026 14:43:00 -0400 Subject: [PATCH 5/5] plan --- ...-services-command-context-refactor-plan.md | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md diff --git a/docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md b/docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md new file mode 100644 index 000000000..c90755082 --- /dev/null +++ b/docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md @@ -0,0 +1,149 @@ +# CLI CommandContext And Server Access Refactor Plan + +## Summary +Refactor `fabro-cli` around an invocation-scoped `CommandContext` that centralizes local settings loading and server connection setup for user-facing server-backed commands. This is a greenfield codebase with no backward-compatibility constraints, so the refactor should be done in one full pass for the in-scope command surface rather than preserving a long-lived mixed model. Keep config-layer composition command-local where commands genuinely need it, keep the generated `fabro_api::Client` as the main HTTP interface, and reuse the existing `ServerStoreClient` type instead of layering a second handwritten endpoint facade on top of it. + +## Public Types And Interfaces +- Add eager, invocation-scoped `CommandContext`: + - Holds `cwd`, `base_config_path`, `machine_settings`, `server_mode`, and a cached server client cell. + - `base_config_path` is the local settings file path chosen by `--config`, `FABRO_CONFIG`, or the default path. + - `machine_settings` is the result of the existing local settings loaders for the command: + - base settings for commands using `load_settings()` + - base settings plus storage-dir override for commands using `load_settings_with_storage_dir(...)` + - `machine_settings` does not include workflow or project config layers. + - Do not store `ConfigLayer` on `CommandContext`. +- Keep config-layer composition command-local: + - `fabro settings` continues to build `EffectiveSettingsLayers` with its existing helpers. + - workflow and manifest code continues to use `ConfigLayer::for_workflow(...)`, `discover_project_config(...)`, and existing workflow resolution structs where individual layers matter. + - `CommandContext` should not attempt to reconstruct or cache workflow/project config layers. +- Use two explicit server access modes that match the real connection paths in the codebase: + - `ServerMode::None` + - `ServerMode::ByTarget { target_override: Option }` + - `ServerMode::ByStorageDir { target_override: Option, storage_dir_override: Option }` + - `ByTarget` maps to the current `connect_server_only(...)` behavior. + - `ByTarget` also covers the current `ServerSummaryLookup::connect(...)` resolution path used by run, pr, runs, and artifact lookup commands. + - `ByStorageDir` maps to the current `connect_server_backed_api_client(...)` and `connect_server_backed_api_client_with_storage_dir(...)` behaviors. + - Do not collapse these two behaviors into one variant with optional target and storage fields. +- Reuse existing target concepts instead of introducing a second target enum: + - keep `ServerTargetArgs`, `ServerConnectionArgs`, and the resolved `ServerTarget` model already used by `user_config` and `server_client` + - do not introduce `ServerTargetInput` in this pass +- Keep `ServerStoreClient`: + - do not rename it during the same refactor + - add narrow accessors as needed for: + - the generated `fabro_api::Client` + - raw `reqwest::Client` + - `base_url` + - this preserves the current `exec` adapter path without adding a second server session type +- `CommandContext::server().await?`: + - available only for `ServerMode::ByTarget` and `ServerMode::ByStorageDir` + - returns `Arc` + - caches the first successful connection in a `OnceCell` + - does not cache failures; a later retry should attempt a fresh connection + - `ByTarget` performs current target-based resolution and Unix-socket auto-start behavior + - `ByStorageDir` performs current storage-dir-backed daemon resolution and startup behavior + - to make both modes uniform, refactor the current storage-dir-backed path, which now returns a bare `fabro_api::Client`, to construct a `ServerStoreClient` first and expose the generated client through an accessor + - migrated connection logic must use `machine_settings` and `base_config_path` already loaded on `CommandContext`; it should stop re-calling `load_settings()` and `load_settings_with_storage_dir(...)` inside `server_client.rs` + - HTTP and HTTPS targets never auto-start + +## Implementation Changes +- Keep `main` bootstrap ordering intact: + - continue loading enough local settings before tracing init to determine log level and upgrade-check behavior + - `CommandContext` begins after tracing is initialized; it does not replace that earlier bootstrap phase +- Do not introduce `Services` in this pass: + - the current review surfaced that a `Services` wrapper does not pull enough independent process-scoped concerns to justify the extra indirection + - if a later refactor reveals multiple real process-scoped services, add that separately +- Keep style construction local for now: + - several current command paths still rely on `&'static Styles` + - this refactor should not add a style ownership change on top of settings/server wiring cleanup +- Make `map_api_error` deduplication a firm deliverable: + - migrated commands should reuse the shared `server_client::map_api_error` + - remove remaining verbatim local copies in the in-scope command surface +- Implement in this order: + - Step 1: add `CommandContext`, `ServerMode`, `ctx.server()` caching semantics, convert the storage-dir-backed connect path to construct `ServerStoreClient`, and thread preloaded `machine_settings` / `base_config_path` into server resolution so migrated commands stop re-loading settings inside connection helpers + - Step 2: migrate the workflow-oriented server commands: + - the main `run` command in [`commands/run/command.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-cli/src/commands/run/command.rs) + - `run create` + - `run start` + - `run attach` + - `run diff` + - `run logs` + - `run preview` + - `run ssh` + - `run resume` + - `run rewind` + - `run fork` + - `run wait` + - `run cp` + - include the existing internal create → start → attach chain where multiple settings loads and server connects exist today + - `preflight` + - `validate` + - `graph` + - Step 3: migrate the remaining user-facing commands that resolve by target or resolved-target lookup: + - `pr` (treat separately inside this step because it mixes settings for app ID and `ServerSummaryLookup::connect`) + - `runs` + - `artifact` + - Step 4: migrate the user-facing commands that use the storage-dir-aware settings loader: + - `model` + - `secret` + - `provider login` + - `repo init` + - `doctor` + - `system info` + - `system df` + - `system events` + - `system prune` + - Step 5: adapt `fabro settings` to use `CommandContext` only for base local settings inputs while keeping its existing layer-building and effective-settings logic + - Step 6: remove obsolete helper entrypoints from migrated call sites and reduce `server_client.rs` to the minimal shared surface still needed by explicit out-of-scope and internal commands: + - migrated commands should stop calling `connect_server_only(...)`, `connect_server_backed_api_client(...)`, `connect_server_backed_api_client_with_storage_dir(...)`, and `ServerSummaryLookup::connect(...)` directly + - keep `connect_server(...)`, `connect_api_client(...)`, and `connect_server_target_direct(...)` only for direct storage-dir or direct-target flows that remain explicit out-of-scope or internal +- Stage dependencies: + - Steps 2, 3, and 4 all depend on Step 1 + - Step 5 depends on Step 1 but not on the migration of other command groups + - Step 6 happens only after the other steps are complete +- Explicitly out of scope: + - `exec` + - `server` lifecycle commands + - hidden `run worker` + - `store dump` + - `workflow` + - `parse` + - `repo deinit` + - `install` + - `sandbox` + - `upgrade` + - hidden analytics and panic upload commands +- Cleanup target after the pass: + - the remaining old helper surface should exist only for those explicitly out-of-scope or internal commands + - migrated user-facing commands should no longer call settings loaders or top-level server connect helpers directly + +## Test Plan +- New unit tests for `CommandContext` construction: + - base config path precedence remains `--config` > `FABRO_CONFIG` > default path (`$FABRO_HOME/settings.toml` if `FABRO_HOME` is set, else `$HOME/.fabro/settings.toml`) + - missing default base config path is allowed; missing explicit config path still errors + - `machine_settings` reflect only the command's existing local settings load path: + - base settings for target-based commands + - base settings plus storage-dir override for storage-backed commands +- New unit tests for server access modes: + - `ServerMode::ByTarget` matches current target-based resolution + - `ServerMode::ByStorageDir` matches current storage-dir-backed resolution + - Unix-socket targets may auto-start; HTTP and HTTPS targets never auto-start + - `ctx.server()` caches a successful client and retries after failures +- Existing regression coverage that must keep passing for config-layer commands: + - workflow and manifest code still layer workflow and project config exactly as before + - `fabro settings` local, daemon, and remote effective-settings modes remain unchanged +- Existing regression coverage that must keep passing for special cases: + - `exec` with no explicit server target still runs directly against providers + - `exec` with an explicit server target still constructs the server-backed adapter path correctly using `ServerStoreClient` accessors +- Existing integration coverage that must keep passing for migrated command groups: + - Step 2: the main `run` command, `run create`, `run start`, `run attach`, `run diff`, `run logs`, `run preview`, `run ssh`, `run resume`, `run rewind`, `run fork`, `run wait`, `run cp`, `preflight`, `validate`, and `graph` + - Step 3: representative `pr`, `artifact`, and `runs` commands + - Step 4: representative `model`, `secret`, `provider login`, `repo init`, `doctor`, `system info`, `system df`, `system events`, and `system prune` commands + - Step 5: `fabro settings` base-settings-input path adopted from `CommandContext` while layer-building and effective-settings logic stay unchanged + +## Assumptions And Defaults +- This is a greenfield codebase with no production compatibility constraints, so one full-pass refactor across the in-scope user-facing command surface is acceptable. +- `CommandContext` is for shared local settings loading and server access only; it is not a universal repository for every config layer or every command concern. +- Commands that need workflow/project layering continue to compute those layers locally from the existing config helpers. +- `ServerStoreClient` remains the shared server connection type in this pass and may gain accessors, but not a second handwritten endpoint layer. +- `exec` remains outside the main abstraction because it still has a real direct-provider fallback mode that is different from normal server-backed commands. +- "Resolved path" means the same path shape produced by current helpers: expanded and made absolute where the helpers already do so, but not canonicalized in a way that would require the file to exist.