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.