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.
- cwd is the invocation working directory and replaces repeated inline std::env::current_dir() lookups in migrated commands such as preflight, validate, graph, fabro settings, and workflow-oriented run creation paths.
- 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<String> }
- ServerMode::ByStorageDir { target_override: Option<String>, storage_dir_override: Option<PathBuf> }
- 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_with_storage_dir(...) behavior when a real storage-dir-aware connection mode is needed.
- current callers of connect_server_backed_api_client(...) migrate to ByTarget in this refactor because their existing None storage-dir path collapses to the same local settings load as connect_server_only(...).
- 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<ServerStoreClient>
- 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
Keep the existing run-summary lookup pattern, but separate lookup construction from connection:
- add ServerSummaryLookup::from_client(client: Arc<ServerStoreClient>) -> Result<Self>
- make the existing ServerSummaryLookup::connect(...) a compatibility wrapper during migration, then remove direct call sites from migrated commands
- migrated commands that currently do ServerSummaryLookup::connect(...) should instead do ServerSummaryLookup::from_client(ctx.server().await?)
- this keeps summary listing, sorting, and selector resolution behavior intact while moving connection ownership into CommandContext

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, add ServerSummaryLookup::from_client(...), 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
  - 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 use target-based resolution or resolved-target lookup:
  - model
  - secret
  - provider login
  - repo init
  - doctor
  - pr (treat separately inside this step because it mixes settings for app ID and ServerSummaryLookup::connect)
  - runs
  - artifact
  - these commands should use ServerMode::ByTarget after migration, even when they currently call connect_server_backed_api_client(...), because their existing None storage-dir path collapses to the same local settings load as connect_server_only(...)
- Step 4: migrate the user-facing commands that genuinely resolve through a storage-dir-aware server mode:
  - system info
  - system df
  - system events
  - system prune
  - these commands should use ServerMode::ByStorageDir
- 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
  - migrated commands that need run lookup/resolve behavior should use ServerSummaryLookup::from_client(ctx.server().await?)
  - 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
- direct storage-dir run lookup flows, including ServerRunLookup, remain unchanged in this pass
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 model, secret, provider login, repo init, doctor, pr, artifact, and runs commands
- Step 4: representative 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.

12 KiB Raw Permalink Blame History