This commit is contained in:
Bryan Helmkamp 2026-04-06 10:17:30 -04:00
parent bd8f0fe5ee
commit 729a5bdda6
9 changed files with 4038 additions and 0 deletions

View file

@ -0,0 +1,419 @@
# CLI De-globalize Server URL And Storage Dir
## Summary
Make `--storage-dir` and `--server-url` command-scoped instead of global so the CLI surface matches the architecture we now have.
This pass should:
- keep only truly global flags in `GlobalArgs`
- move local-storage selection onto the commands that actually use local storage
- move remote-server selection onto the commands that actually support remote targeting
- remove false affordances from help output, docs, and env-var wiring
This plan is the direct follow-on to [2026-04-05-next-steps-after-cli-mode-removal.md](../ideation/2026-04-05-next-steps-after-cli-mode-removal.md).
## Scope Boundaries
In scope:
- de-globalize `--storage-dir` / `FABRO_STORAGE_DIR`
- de-globalize `--server-url` / `FABRO_SERVER_URL`
- update clap types, parser tests, dispatch signatures, and command help
- keep `model` and `exec` behavior consistent with the recent cleanup, but expressed through command-local args
- update docs/examples that still show top-level target flags
Out of scope:
- changing server/runtime behavior for `run`, `model`, or `exec`
- adding new remote-capable commands
- making `exec` server-owned
- changing `[server].base_url` semantics again
- broad config/schema changes outside the CLI arg surface
## Problem Frame
The code no longer has a meaningful whole-program “mode”, but the CLI still advertises target-selection flags as if every command can choose between local storage and a remote server.
That is now misleading in several different ways:
- many commands still show `--server-url` even though they never use it
- many commands still show `--storage-dir` even though they do not read local runtime state
- docs still describe these flags as global CLI surface area
- help snapshot churn is broader than the real behavior surface because the flags leak into unrelated commands
The goal is not to invent new behavior. The goal is to make the CLI honest about which commands actually support which target-selection controls.
## Key Decisions
- `GlobalArgs` remains, but only for true global flags:
- `--json`
- `--debug`
- `--no-upgrade-check`
- `--quiet`
- `--verbose`
- `--storage-dir` and `--server-url` stop being top-level/global clap args entirely.
- target-selection args belong to leaf commands, not parent namespaces.
- Rationale: keep natural syntax such as:
- `fabro run foo.fabro --storage-dir /tmp/fabro`
- `fabro model list --server-url https://fabro.example.com/api/v1`
- `fabro server start --storage-dir /tmp/fabro`
- Avoid awkward parent-namespace syntax like `fabro model --server-url ... list`.
- `FABRO_STORAGE_DIR` and `FABRO_SERVER_URL` remain supported, but only for commands that define the corresponding arg.
- command-local target syntax becomes the supported surface.
- Old forms like `fabro --server-url ... model list` and `fabro --storage-dir ... run ...` are intentionally removed in this pass.
- during implementation, temporary duplication between global target args and the new leaf-command args is acceptable.
- Rationale: this refactor touches many commands, so the migration should stay compile-safe until the old global fields are fully unused and can be removed in one cleanup step.
- `settings` keeps `--storage-dir`, because it changes the resolved local settings output.
- `settings` does not keep `--server-url`.
- Rationale: command-local remote target overrides should not masquerade as merged durable config.
- `preflight` keeps `--storage-dir`, because it resolves a local run-oriented settings stack and should continue to allow explicit local storage selection.
- `exec` keeps only `--server-url`.
- `exec` does not need `--storage-dir`.
- `model list` and `model test` keep both:
- `--server-url` for explicit remote server targeting
- `--storage-dir` for explicit local auto-start/storage selection
- the existing `model` config/defaulting contract stays intact:
- explicit `--server-url` wins
- explicit `--storage-dir` suppresses configured `[server].base_url`
- otherwise `model` may default to configured `[server].base_url`
- the existing `exec` contract stays intact:
- server routing only happens when the command-local `server_url` value is set, whether by CLI flag or `FABRO_SERVER_URL`
- configured `[server].base_url` alone still does not reroute `exec`
- `fabro model` with no subcommand remains the convenience alias for default listing behavior.
- target overrides are not a compatibility goal for the bare alias in this pass; users should use `fabro model list ...` when specifying target args explicitly.
## Command Matrix
### `--server-url` only
- `fabro exec`
### `--storage-dir` and `--server-url`
- `fabro model list`
- `fabro model test`
### `--storage-dir` only
- `fabro run`
- `fabro create`
- `fabro start`
- `fabro attach`
- `fabro logs`
- `fabro resume`
- `fabro rewind`
- `fabro fork`
- `fabro wait`
- `fabro diff`
- hidden `fabro __runner`
- `fabro ps`
- `fabro rm`
- `fabro inspect`
- `fabro artifact list`
- `fabro artifact cp`
- `fabro sandbox cp`
- `fabro sandbox preview`
- `fabro sandbox ssh`
- `fabro store dump`
- `fabro pr create`
- `fabro pr list`
- `fabro pr view`
- `fabro pr merge`
- `fabro pr close`
- `fabro system prune`
- `fabro system df`
- `fabro server start`
- `fabro server stop`
- `fabro server status`
- hidden `fabro server __serve`
- `fabro settings`
- `fabro preflight`
### Neither target arg
- `fabro validate`
- `fabro graph`
- `fabro parse`
- `fabro doctor`
- `fabro install`
- `fabro repo init`
- `fabro repo deinit`
- `fabro workflow list`
- `fabro workflow create`
- `fabro provider login`
- hidden `fabro skill install`
- `fabro secret get`
- `fabro secret list`
- `fabro secret set`
- `fabro secret rm`
- `fabro docs`
- `fabro discord`
- `fabro completion`
- `fabro upgrade`
- internal analytics/panic commands
## Implementation Changes
### 1. Narrow `GlobalArgs` and add command-scoped target arg structs
In `lib/crates/fabro-cli/src/args.rs`:
- add the new command-scoped target arg structs first, while temporarily leaving `storage_dir` and `server_url` on `GlobalArgs`
- keep `GlobalArgs` as the long-term home only for the true-global output/logging/upgrade-check surface
- add small reusable arg structs:
- `StorageDirArgs { storage_dir: Option<PathBuf> }`
- `ServerUrlArgs { server_url: Option<String> }`
- `ModelTargetArgs { storage_dir: Option<PathBuf>, server_url: Option<String> }`
- keep the `storage_dir` / `server_url` conflict on `ModelTargetArgs`
- keep the existing env bindings:
- `FABRO_STORAGE_DIR`
- `FABRO_SERVER_URL`
This temporary duplication is intentional. The crate should continue to compile while commands migrate off `GlobalArgs`.
Because the target args will now live on leaf commands, convert inline/anonymous clap variants into explicit args structs where needed. The important conversions are:
- `Exec(AgentArgs)` -> `Exec(ExecArgs)` where `ExecArgs` flattens:
- `ServerUrlArgs`
- `fabro_agent::cli::AgentArgs`
- `ModelsCommand::List { ... }` -> `ModelsCommand::List(ModelListArgs)`
- `ModelsCommand::Test { ... }` -> `ModelsCommand::Test(ModelTestArgs)`
- `RunCommands::Start { run: String }` -> `RunCommands::Start(StartArgs)` where `StartArgs` flattens `StorageDirArgs`
- `RunCommands::Attach { run: String }` -> `RunCommands::Attach(AttachArgs)` where `AttachArgs` flattens `StorageDirArgs`
- `RunCommands::Runner { ... }` -> `RunCommands::Runner(RunnerArgs)` where `RunnerArgs` flattens `StorageDirArgs`
- `ServerCommand::Start { ... }` -> `ServerCommand::Start(ServerStartArgs)` where `ServerStartArgs` flattens:
- `StorageDirArgs`
- the existing `foreground` flag
- `ServeArgs`
- `ServerCommand::Stop { ... }` -> `ServerCommand::Stop(ServerStopArgs)` where `ServerStopArgs` flattens `StorageDirArgs`
- `ServerCommand::Status { ... }` -> `ServerCommand::Status(ServerStatusArgs)` where `ServerStatusArgs` flattens `StorageDirArgs`
- `ServerCommand::Serve { ... }` -> `ServerCommand::Serve(ServerServeArgs)` where `ServerServeArgs` flattens:
- `StorageDirArgs`
- the existing `record_path` field
- `ServeArgs`
Also flatten `StorageDirArgs` into the leaf arg types that currently rely on global storage selection, including:
- `RunArgs`
- `LogsArgs`
- `DiffArgs`
- `ResumeArgs`
- `RewindArgs`
- `ForkArgs`
- `WaitArgs`
- `RunsListArgs`
- `RunsRemoveArgs`
- `InspectArgs`
- `ArtifactListArgs`
- `ArtifactCpArgs`
- `CpArgs`
- `PreviewArgs`
- `SshArgs`
- `StoreDumpArgs`
- `PrCreateArgs`
- `PrListArgs`
- `PrViewArgs`
- `PrMergeArgs`
- `PrCloseArgs`
- `RunsPruneArgs`
- `DfArgs`
- `SettingsArgs`
- `PreflightArgs`
### 2. Rework parser and dispatch wiring around leaf-command target args
In `lib/crates/fabro-cli/src/main.rs`:
- keep `Cli { globals, command }`, but with the narrower `GlobalArgs`
- update dispatch pattern matches to use the new wrapper arg structs:
- `ExecArgs`
- `ModelListArgs`
- `ModelTestArgs`
- `StartArgs`
- `AttachArgs`
- `RunnerArgs`
- replace parser tests that assume global target flags with command-local parser tests
Parser behavior to lock down:
- `fabro run test/simple.fabro --storage-dir /tmp/fabro` parses
- `fabro model list --server-url http://localhost:3000/api/v1` parses
- `fabro exec --server-url http://localhost:3000/api/v1 "prompt"` parses
- `fabro model list --storage-dir /tmp/fabro --server-url http://localhost:3000/api/v1` fails with the command-local conflict
- `fabro --server-url http://localhost:3000/api/v1 model list` no longer parses
- `fabro --storage-dir /tmp/fabro run test/simple.fabro` no longer parses
These parser assertions belong at the end of the migration, after `storage_dir` and `server_url` have actually been removed from `GlobalArgs`. Before that cleanup step, the old top-level forms will still parse and should not be treated as failures yet.
### 3. Replace “with globals” settings helpers with explicit local/remote override helpers
The current helper layer in `lib/crates/fabro-cli/src/user_config.rs` is still shaped around global target args. That should be simplified to explicit command-local override helpers.
In `lib/crates/fabro-cli/src/user_config.rs`:
- remove or rename the generic helpers that imply target args are global:
- `user_layer_with_globals(...)`
- `load_user_settings_with_globals(...)`
- `apply_global_overrides(...)`
- replace them with explicit helpers such as:
- `user_layer_with_storage_dir(storage_dir: Option<&Path>) -> anyhow::Result<ConfigLayer>`
- `load_user_settings_with_storage_dir(storage_dir: Option<&Path>) -> anyhow::Result<Settings>`
- `exec_server_target(args: &ServerUrlArgs, settings: &Settings) -> Option<ServerTarget>`
- `model_server_target(args: &ModelTargetArgs, settings: &Settings) -> Option<ServerTarget>`
- keep `build_server_client(...)`
Behavior to preserve:
- local-storage commands can still resolve settings with an explicit storage-dir override
- `model` keeps its existing defaulting behavior
- `exec` only resolves a remote target when its command-local `server_url` value is set
- TLS continues to come from `[server].tls` when a remote target is selected
### 4. Repoint command implementations to explicit target args
Update the commands that currently read target selection through `GlobalArgs`.
In `lib/crates/fabro-cli/src/commands/model.rs`:
- switch from `GlobalArgs`-based target lookup to `ModelTargetArgs`
- keep the recent server-canonical behavior unchanged
In `lib/crates/fabro-cli/src/commands/exec.rs`:
- switch from `GlobalArgs`-based target lookup to `ServerUrlArgs`
- remove any remaining dependence on `storage_dir`
- load settings with plain `load_user_settings()`, not a storage-dir-aware helper
- `exec` does not need local storage override semantics anymore
- it should then pass `&ServerUrlArgs` to `exec_server_target(...)`
In the local-storage command modules:
- replace `load_user_settings_with_globals(globals)` with the explicit storage-dir helper
- read `storage_dir` from the commands own args struct, not from `globals`
Key files here include:
- `lib/crates/fabro-cli/src/commands/run/mod.rs`
- `lib/crates/fabro-cli/src/commands/run/command.rs`
- `lib/crates/fabro-cli/src/commands/run/logs.rs`
- `lib/crates/fabro-cli/src/commands/run/resume.rs`
- `lib/crates/fabro-cli/src/commands/run/rewind.rs`
- `lib/crates/fabro-cli/src/commands/run/fork.rs`
- `lib/crates/fabro-cli/src/commands/run/wait.rs`
- `lib/crates/fabro-cli/src/commands/run/diff.rs`
- `lib/crates/fabro-cli/src/commands/run/cp.rs`
- `lib/crates/fabro-cli/src/commands/run/preview.rs`
- `lib/crates/fabro-cli/src/commands/run/ssh.rs`
- `lib/crates/fabro-cli/src/commands/runs/list.rs`
- `lib/crates/fabro-cli/src/commands/runs/rm.rs`
- `lib/crates/fabro-cli/src/commands/runs/inspect.rs`
- `lib/crates/fabro-cli/src/commands/artifact/list.rs`
- `lib/crates/fabro-cli/src/commands/artifact/cp.rs`
- `lib/crates/fabro-cli/src/commands/store/dump.rs`
- `lib/crates/fabro-cli/src/commands/pr/*.rs`
- `lib/crates/fabro-cli/src/commands/system/*.rs`
- `lib/crates/fabro-cli/src/commands/server/mod.rs`
- `lib/crates/fabro-cli/src/commands/preflight.rs`
- `lib/crates/fabro-cli/src/commands/config/mod.rs`
Important internal-path detail:
- hidden commands still need explicit local storage wiring
- `fabro server __serve` must keep receiving `--storage-dir` from `lib/crates/fabro-cli/src/commands/server/start.rs`
- hidden `fabro __runner` should continue to accept explicit storage-dir selection through its own args struct rather than through `GlobalArgs`
### 5. Make help output and docs reflect the new command contract
In `docs/reference/cli.mdx`:
- remove `--storage-dir` and `--server-url` from the “Global options” table
- add a short “Command-scoped target selection” section or matrix
- update examples to use command-local placement:
- `fabro model list --server-url ...`
- `fabro server start --storage-dir ...`
In `docs/reference/user-configuration.mdx`:
- keep `[server].base_url` documentation
- clarify that it is a default only for commands that support remote server targets
- keep the `model` / `exec` asymmetry explicit
- update examples away from top-level `fabro --server-url ...`
In `docs/administration/deploy-server.mdx`:
- update “point the CLI at a server” examples to command-local syntax
- clarify that `model` can use configured `[server].base_url`, while `exec` still needs explicit `--server-url`
In `docs/reference/run-directory.mdx`:
- change “global `--storage-dir` flag” wording to command-local run-family wording
Also scan for stale examples in nearby docs and changelog/admin references and update only the ones that would now be actively misleading.
### 6. Update unit tests, parser tests, and help snapshots
#### Unit and parser coverage
In `lib/crates/fabro-cli/src/main.rs` tests:
- replace the old global target-flag parse tests with command-local parse tests
- add the explicit “old global placement no longer parses” cases
In `lib/crates/fabro-cli/src/user_config.rs` tests:
- update helper tests to use the new arg structs
- preserve coverage for:
- `exec` CLI/env `server_url` routing
- `exec` ignoring configured `[server].base_url`
- `model` configured `[server].base_url` defaulting
- `model` `storage_dir` suppressing configured remote target
- TLS inheritance
#### Integration coverage
Update behavior/help coverage in:
- `lib/crates/fabro-cli/tests/it/cmd/exec.rs`
- `lib/crates/fabro-cli/tests/it/cmd/model.rs`
- `lib/crates/fabro-cli/tests/it/cmd/model_list.rs`
- `lib/crates/fabro-cli/tests/it/cmd/model_test.rs`
- `lib/crates/fabro-cli/tests/it/cmd/config.rs`
- `lib/crates/fabro-cli/tests/it/cmd/run.rs`
- `lib/crates/fabro-cli/tests/it/cmd/create.rs`
- `lib/crates/fabro-cli/tests/it/cmd/attach.rs`
- `lib/crates/fabro-cli/tests/it/cmd/logs.rs`
- `lib/crates/fabro-cli/tests/it/cmd/resume.rs`
- `lib/crates/fabro-cli/tests/it/cmd/rewind.rs`
- `lib/crates/fabro-cli/tests/it/cmd/wait.rs`
- `lib/crates/fabro-cli/tests/it/cmd/diff.rs`
- `lib/crates/fabro-cli/tests/it/cmd/runner.rs`
- `lib/crates/fabro-cli/tests/it/cmd/ps.rs`
- `lib/crates/fabro-cli/tests/it/cmd/inspect.rs`
- `lib/crates/fabro-cli/tests/it/cmd/store.rs`
- `lib/crates/fabro-cli/tests/it/cmd/store_dump.rs`
- `lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs`
- `lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs`
- `lib/crates/fabro-cli/tests/it/cmd/sandbox_cp.rs`
- `lib/crates/fabro-cli/tests/it/cmd/sandbox_preview.rs`
- `lib/crates/fabro-cli/tests/it/cmd/sandbox_ssh.rs`
- `lib/crates/fabro-cli/tests/it/cmd/pr.rs`
- `lib/crates/fabro-cli/tests/it/cmd/pr_list.rs`
- `lib/crates/fabro-cli/tests/it/cmd/pr_view.rs`
- `lib/crates/fabro-cli/tests/it/cmd/pr_merge.rs`
- `lib/crates/fabro-cli/tests/it/cmd/pr_close.rs`
- `lib/crates/fabro-cli/tests/it/cmd/system.rs`
- `lib/crates/fabro-cli/tests/it/cmd/system_df.rs`
- `lib/crates/fabro-cli/tests/it/cmd/system_prune.rs`
- `lib/crates/fabro-cli/tests/it/cmd/server_start.rs`
- `lib/crates/fabro-cli/tests/it/cmd/server_stop.rs`
- `lib/crates/fabro-cli/tests/it/cmd/server_status.rs`
- `lib/crates/fabro-cli/tests/it/cmd/preflight.rs`
- `lib/crates/fabro-cli/tests/it/cmd/fabro.rs`
Behavior scenarios to add or preserve:
- `exec` still routes through the server only when its command-local `server_url` is set
- `model` still honors configured `[server].base_url` when no explicit `storage_dir` is set
- `model` command-local `server_url` still overrides configured base URL
- `model` command-local `storage_dir` still forces local behavior
- old top-level target-flag placement fails at parse time
For snapshot churn:
- use the repo workflow from `CLAUDE.md`
- `cargo insta pending-snapshots`
- verify the expected help/output changes
- `cargo insta accept`
## Dependencies And Sequencing
Apply the change in this order:
1. add the new command-scoped target arg structs in `args.rs`, but keep `storage_dir` and `server_url` on `GlobalArgs` temporarily so the crate still compiles during migration
2. add the new explicit helpers in `user_config.rs` alongside the old global-based helpers
3. repoint `exec` and `model` to the new target arg structs and helpers
4. repoint the local-storage and server command modules, plus test harness/env wiring, off `GlobalArgs`
5. remove `storage_dir` and `server_url` from `GlobalArgs`, then delete the old global-based helper path once it is unused
6. update parser tests for the final clap shape, then update docs and help snapshots
This ordering keeps the refactor compile-safe: helper and command migration happen before the old global fields are removed, and parser assertions about the old top-level syntax move to the final cleanup step where they become true.
## Test Plan
- targeted parser/unit tests:
- `cargo test -p fabro-cli --lib main::tests -- --nocapture`
- `cargo test -p fabro-cli --lib user_config::tests -- --nocapture`
- targeted CLI integration tests:
- `cargo nextest run -p fabro-cli cmd::exec:: --no-fail-fast`
- `cargo nextest run -p fabro-cli cmd::model:: cmd::model_list:: cmd::model_test:: cmd::config:: --no-fail-fast`
- `cargo nextest run -p fabro-cli cmd::run:: cmd::create:: cmd::attach:: cmd::logs:: cmd::resume:: cmd::rewind:: cmd::wait:: cmd::diff:: cmd::runner:: --no-fail-fast`
- `cargo nextest run -p fabro-cli cmd::server_start:: cmd::server_stop:: cmd::server_status:: --no-fail-fast`
- broader CLI sweep after targeted coverage is green:
- `cargo nextest run -p fabro-cli --no-fail-fast`
- final verification:
- `cargo fmt --check --all`
- `cargo clippy --workspace --all-targets -- -D warnings`
## Assumptions And Defaults
- Pre-production status means removing the old global target-flag placement is acceptable; no compatibility shim is required.
- `FABRO_STORAGE_DIR` and `FABRO_SERVER_URL` remain useful and should stay, but only where the corresponding command actually supports the underlying behavior.
- The recent `model` and `exec` behavioral contracts are already correct; this pass is about CLI honesty and arg ownership, not product redefinition.
- If we later want a broader “remote-capable command matrix” abstraction, it should be built on top of these command-local args rather than by reintroducing misleading global target flags.

View file

@ -0,0 +1,242 @@
# CLI Exec Explicit Local Sessions And Mode Removal
## Summary
Simplify the last misleading CLI mode seam by:
- making `fabro exec` explicitly CLI-owned/local
- removing the global `ExecutionMode` / `resolve_mode` abstraction entirely
- deleting `mode` from user config and resolved `Settings`
- treating Fabro server usage as a command-specific connection choice instead of a whole-program execution mode
This pass does **not** move the agent loop into the server. `fabro exec` remains a local agent session. The only server-backed `exec` behavior in scope is optional model transport through `FabroServerAdapter` and the existing `/completions` endpoint.
## Scope Boundaries
In scope:
- `fabro exec`
- removal of `ExecutionMode`, `ResolvedMode`, and `resolve_mode`
- removal of `mode` from `user.toml` / `Settings`
- command-scoped server-target resolution for `exec`
- small adjacent `model` cleanup needed so `[server]` remains meaningful after `mode` is removed
- docs/help/config/test cleanup for the new contract
Out of scope:
- server-owned interactive agent sessions
- new `/exec`, `/sessions`, or worker-RPC server APIs
- moving tool execution, MCP, permissions, or output rendering into the server
- changing the run lifecycle commands
- changing the `/completions` API contract
- adding a new persistent `exec` config field for default server routing
## Key Decisions
- `fabro exec` always owns the agent session locally:
- prompt loop
- tool execution
- permission prompts
- MCP connections
- event/output rendering
- when `fabro exec` uses a Fabro server, only the LLM transport changes:
- requests go through `fabro_llm::providers::FabroServerAdapter`
- the adapter continues to call the existing `/completions` endpoint
- remove `mode` entirely instead of narrowing it.
- Once `exec` stops using it, there is no legitimate runtime consumer left.
- `fabro exec` remote routing stays an explicit per-invocation choice in this pass:
- `--server-url` enables server-routed model transport
- configured `[server].base_url` alone does **not** reroute `exec`
- Rationale: this keeps `exec` honest as a local command and avoids inventing a new “force direct” override surface immediately.
- `[server]` remains a real config surface after `mode` removal, but its meaning changes:
- it stores connection information for commands that support a remote Fabro server target
- it is not a whole-program execution mode switch
- `fabro model` should honor configured `[server].base_url` as its default remote target once `mode` is gone.
- `--server-url` still overrides config
- explicit `--storage-dir` still forces local auto-start for `model`
- this is a small collateral cleanup to keep the config contract coherent
- keep the global `--server-url` / `--storage-dir` clap conflict unchanged in this pass.
- Rationale: reducing the misleading mode abstraction does not require broad CLI flag-surface churn, and `exec` does not need both simultaneously.
- global help/docs must stop claiming:
- `--storage-dir` implies standalone mode
- `--server-url` implies server mode
- no backward-compatibility shim is required for `mode` in `user.toml`, `fabro settings`, or docs.
- `mode = "server"` / `mode = "standalone"` should disappear completely from the supported config surface:
- remove it from docs, examples, tests, and resolved settings output
- existing user config files that still contain it are out of contract after this pass
- if deserialization happens to ignore the stale key, that is incidental behavior, not preserved product surface
## Implementation Changes
### 1. Remove global mode from shared config and settings types
Delete the dead mode concept from the shared config graph.
In `lib/crates/fabro-types/src/settings/user.rs`:
- delete `ExecutionMode`
In `lib/crates/fabro-types/src/settings/mod.rs`:
- remove the `ExecutionMode` re-export
- remove `Settings.mode`
In `lib/crates/fabro-config/src/user.rs`:
- stop re-exporting `ExecutionMode`
In `lib/crates/fabro-config/src/config.rs`:
- remove `ConfigLayer.mode`
- remove `mode` combine logic
In `lib/crates/fabro-config/src/settings.rs`:
- stop mapping `value.mode` into `Settings`
Behavior:
- `fabro settings` no longer emits a `mode` field
- `user.toml` no longer documents or accepts `mode` as a meaningful setting in this pass
### 2. Replace `resolve_mode` with command-scoped server-target helpers
Stop encoding command behavior as a fake global mode decision.
In `lib/crates/fabro-cli/src/user_config.rs`:
- delete:
- `ResolvedMode`
- `resolve_mode(...)`
- keep `build_server_client(...)`
- add one small shared remote-target type, for example:
- `ServerTarget { base_url: String, tls: Option<ClientTlsSettings> }`
- update `apply_global_overrides(...)` so it only applies:
- `storage_dir`
- `server.base_url`
- and does **not** synthesize any `mode`
- if `apply_global_overrides(...)` becomes a trivial two-field merge helper after `mode` removal, inline it at the call sites instead of preserving it mechanically
- add concrete helpers that reflect the real command boundaries:
- `exec_server_target(globals: &GlobalArgs, settings: &Settings) -> Option<ServerTarget>`
- `model_server_target(globals: &GlobalArgs, settings: &Settings) -> Option<ServerTarget>`
Expected helper semantics:
- `exec` helper:
- returns a remote server target only when `--server-url` is present
- uses configured `[server].tls` if available
- ignores configured `[server].base_url` when no CLI flag is present
- `model` helper:
- returns `--server-url` when present
- otherwise returns configured `[server].base_url` when present and no explicit `--storage-dir` was provided
- otherwise returns no remote target so the command falls back to local server auto-start
This helper split is intentional. `exec` and `model` are both server-aware, but they do not have the same defaulting rules.
### 3. Make `fabro exec` explicitly local with optional server-routed model transport
Remove the last server-vs-standalone branching from the command implementation.
In `lib/crates/fabro-cli/src/commands/exec.rs`:
- remove the `resolve_mode(...)` call
- remove the `match resolved.mode { ... }` branch
- always build the session as a local CLI-owned agent session
- when the `exec` server-target helper returns `Some(target)`:
- build the HTTP client with `build_server_client(...)`
- create a `FabroServerAdapter`
- register it on a `fabro_llm::Client`
- call `run_with_args_and_client(...)`
- when the helper returns `None`:
- keep the direct provider path via `run_with_args(...)`
- replace logging from `mode = "server"/"standalone"` to something transport-shaped such as:
- `transport = "server"`
- `transport = "direct"`
Keep unchanged:
- permission behavior
- MCP server wiring
- output format behavior
- sub-agent behavior
- sandbox/tool execution ownership
### 4. Keep `[server]` meaningful after removing `mode`
Make the shared server config still useful without preserving the abstract mode layer.
In `lib/crates/fabro-cli/src/commands/model.rs`:
- stop hand-rolling the `globals.server_url` match
- use the new model-target helper from `user_config.rs`
- preserve current user-visible `model` behavior apart from the new config defaulting:
- remote target when `--server-url` is passed
- remote target when `[server].base_url` is configured and no explicit `--storage-dir` is passed
- local auto-start otherwise
This is the only planned collateral behavior change outside `exec`, and it exists to keep `[server]` coherent after `mode` removal.
### 5. Remove stale docs and help text
Rewrite the user-facing contract around explicit server targets instead of execution modes.
In `lib/crates/fabro-cli/src/args.rs`:
- no `--mode` flag exists today, so there is no parser flag removal in this file
- update the global flag docstrings:
- `--storage-dir` should describe local data/storage selection only
- `--server-url` should describe targeting a Fabro API server for commands that support it
- remove any wording that says either flag “implies” a mode
In docs:
- `docs/reference/user-configuration.mdx`
- remove the `mode` section
- rewrite `[server]` as connection info for remote-target-capable commands
- clarify the `exec` vs `model` behavior split explicitly
- `docs/reference/cli.mdx`
- update the global option descriptions for `--storage-dir` and `--server-url`
- `docs/administration/deploy-server.mdx`
- remove guidance telling users to set `mode = "server"` in `user.toml`
- rewrite the “Pointing the CLI at a server” section around:
- `--server-url`
- configured `[server].base_url`
- command-specific behavior
- update any nearby docs that still describe “whole CLI server mode” rather than explicit server-target selection
### 6. Remove or rewrite stale tests
Delete tests that only exist to preserve `mode` semantics, and add coverage for the real command boundaries.
In `lib/crates/fabro-cli/src/user_config.rs` tests:
- replace `resolve_mode_*` tests with helper-focused tests covering:
- `exec` has no server target by default
- `exec` uses CLI `--server-url`
- `exec` ignores configured `[server].base_url` without CLI `--server-url`
- `model` uses configured `[server].base_url`
- `model` CLI `--server-url` overrides configured base URL
- `model` explicit `--storage-dir` suppresses configured remote targeting
- TLS is still taken from `[server].tls` when a remote target is selected
In `lib/crates/fabro-cli/tests/it/cmd/exec.rs`:
- update help snapshots for the new global flag wording
- add one behavior test proving `--server-url` changes the failure mode away from local missing-provider-key validation
- for example: with no local provider key configured and an unreachable `--server-url`, the command should fail on remote connection rather than `API key not set for provider ...`
- add one regression test proving configured `[server].base_url` alone does not reroute `exec`
- expected outcome: with no local provider key, `exec` still fails on the local missing-key path
- add one explicit override test proving CLI `--server-url` wins over configured `[server].base_url` for `exec`
- expected outcome: with both present, the command targets the CLI URL and the failure shape reflects that URL/path rather than the configured one
In `lib/crates/fabro-cli/tests/it/cmd/model.rs` and/or `lib/crates/fabro-cli/tests/it/cmd/model_list.rs`:
- add coverage showing configured `[server].base_url` is honored without passing `--server-url`
In `lib/crates/fabro-cli/tests/it/cmd/config.rs`:
- remove `ExecutionMode` imports and expectations
- update `fabro settings` assertions so `mode` is no longer expected in resolved output
- keep coverage that `--server-url` still overrides configured `[server].base_url`
In help snapshots:
- update any snapshots whose global options block still mentions implied server/standalone mode
- especially:
- `lib/crates/fabro-cli/tests/it/cmd/fabro.rs`
- `lib/crates/fabro-cli/tests/it/cmd/exec.rs`
- `lib/crates/fabro-cli/tests/it/cmd/model.rs`
- `lib/crates/fabro-cli/tests/it/cmd/model_list.rs`
- `lib/crates/fabro-cli/tests/it/cmd/config.rs`
- use the repo snapshot workflow from `CLAUDE.md`:
- run `cargo insta pending-snapshots`
- verify the expected help/output changes
- then run `cargo insta accept`
## Test Plan
- Targeted unit tests:
- `cargo test -p fabro-cli user_config::tests -- --nocapture`
- Targeted CLI integration tests:
- `cargo nextest run -p fabro-cli cmd::exec:: cmd::model:: cmd::model_list:: cmd::config:: --no-fail-fast`
- Broader regression sweep after the targeted tests are green:
- `cargo nextest run -p fabro-cli --no-fail-fast`
- Final verification:
- `cargo fmt --check --all`
- `cargo clippy --workspace --all-targets -- -D warnings`
## Assumptions And Defaults
- `fabro exec` remains a local agent session in this pass. If we later want server-owned interactive sessions, that should be a separate product/architecture plan.
- The existing `/completions` endpoint and `FabroServerAdapter` are sufficient for optional server-routed `exec` model traffic.
- Pre-production status means removing `mode` outright is acceptable; no shim or migration warning is required.
- `[server].base_url` remains valuable as a remote target config surface for commands like `model`, even after `mode` is removed.
- `exec` staying CLI-flag-only for server routing is intentional in this pass. If users later need a default server-routed `exec`, that should be introduced as an explicit `exec`-scoped config surface rather than reintroducing a fake whole-program mode.

View file

@ -0,0 +1,364 @@
# CLI Model Server Canonicalization And LLM Namespace Removal
## Summary
Take the next low-risk simplification step after the run lifecycle cleanup by:
- removing the entire unused `fabro llm` CLI namespace
- making the `fabro model` family fully server-canonical
- leaving `fabro exec` unchanged for now
This pass is intentionally asymmetric:
- `fabro llm` is removed from the CLI surface entirely
- `fabro model` remains, but becomes a server-backed command family instead of a mixed standalone/server command
- `fabro model test` keeps its current bulk/fan-out role in the CLI, but the server endpoint remains single-model only with an explicit test mode
Because backward compatibility is not required yet, this pass should prefer simplification over shims. `model test --deep` stays, but it becomes an explicit server capability via `mode=basic|deep` on the single-model test endpoint instead of relying on the old mixed local/server split.
## Scope Boundaries
In scope:
- remove `fabro llm`
- add `provider` and `query` filters to `GET /api/v1/models`
- keep `POST /api/v1/models/{id}/test` as a single-model health/test endpoint
- add optional `mode=basic|deep` to `POST /api/v1/models/{id}/test`
- make `fabro model list` and `fabro model test` always use the server
Out of scope:
- `fabro exec`
- broader removal of `ExecutionMode` / `resolve_mode` outside the `model` command family
- changing `POST /models/{id}/test` into a bulk endpoint
- changing model storage/catalog ownership away from the servers built-in catalog
- deleting lower-level `fabro_llm::cli` prompt/chat helpers unless they become dead and trivially removable during the refactor
## Key Decisions
- `GET /api/v1/models` gets flat query params, not a generic filter object:
- `provider=<name>`
- `query=<substring>`
- `query` matches `id`, `display_name`, and `aliases`, case-insensitively.
- Pagination applies after filtering.
- filtered `/api/v1/models` results preserve the built-in catalog order.
- invalid `provider` filter values are rejected with `400`, not treated as “no filter” or “no matches”.
- "invalid" means the query value fails to parse as a known `fabro_model::Provider` enum variant.
- a parsed `provider` value that happens to match zero catalog entries still returns `200` with an empty page.
- `POST /api/v1/models/{id}/test` continues to test exactly one model per request and gains one optional query param:
- `mode=basic|deep`
- default: `basic`
- CLI fan-out stays in `fabro model test`, not in the HTTP API.
- `model test --deep` is preserved and maps to `mode=deep` on repeated single-model server calls.
- `fabro model` should use the generated `fabro_api::Client` for model HTTP calls, not ad hoc raw `reqwest` + URL assembly.
- `fabro model` gets a command-specific server-target helper rather than reusing global `ExecutionMode` branching.
- if `--server-url` is present, use remote HTTP(S) with configured TLS
- otherwise use local server auto-start + Unix socket for the selected storage dir
- this does not change `resolve_mode` behavior for other commands
- `basic` is the explicit name for the current simple health check mode.
- Rationale: an enum-shaped mode is clearer than `deep=true` and leaves room for future test kinds without changing the endpoint shape.
- `POST /api/v1/models/{id}/test` accepts either a canonical model ID or an alias.
- The server resolves aliases using the built-in catalog.
- The response should always return the canonical `model_id`, not the alias string from the path.
- `model test --model <id-or-alias>` should POST the provided value directly to `/api/v1/models/{id-or-alias}/test`.
- the CLI does not pre-resolve aliases via `GET /models`
- CLI `model list --json` preserves its current output contract as a plain array of model objects.
- The server remains paginated internally, but the CLI should flatten that to preserve the existing CLI JSON surface.
- Deep-mode API results remain binary at the schema level:
- success cases return `status: ok`
- any deep validation failure returns `status: error`
- the failure details go in `error_message`
- no new warning/partial result state is introduced in this pass
- models without required tool support still return HTTP `200` with `status: error`
- absence of reasoning traces alone does not fail deep mode in this pass
## Implementation Changes
### 1. Remove the `fabro llm` CLI namespace
Update the CLI surface so `fabro llm` no longer exists.
In `lib/crates/fabro-cli/src/args.rs`:
- remove the `Commands::Llm` variant
- remove `LlmNamespace` and `LlmCommand`
- remove `ChatArgs` / `PromptArgs` imports from `fabro_llm::cli`
- remove command-name mapping for `llm prompt` and `llm chat`
In `lib/crates/fabro-cli/src/main.rs`:
- remove the `Commands::Llm(...)` dispatch arm
In `lib/crates/fabro-cli/src/commands/mod.rs`:
- remove `pub(crate) mod llm;`
Delete:
- `lib/crates/fabro-cli/src/commands/llm/mod.rs`
- `lib/crates/fabro-cli/src/commands/llm/chat.rs`
- `lib/crates/fabro-cli/src/commands/llm/prompt.rs`
Test/support cleanup:
- remove `mod llm;` and `mod llm_prompt;` from `lib/crates/fabro-cli/tests/it/cmd/mod.rs`
- delete:
- `lib/crates/fabro-cli/tests/it/cmd/llm.rs`
- `lib/crates/fabro-cli/tests/it/cmd/llm_prompt.rs`
- remove the now-unused `TestContext::llm()` helper from `lib/crates/fabro-test/src/lib.rs`
- update top-level help snapshots in `lib/crates/fabro-cli/tests/it/cmd/fabro.rs` and any parser/help coverage that still mentions `llm`
This pass should only remove the CLI namespace. Do not widen the blast radius by opportunistically deleting unrelated lower-level LLM helpers unless the compiler proves they are now dead and the deletion is mechanical.
Because `fabro llm` is the only CLI surface for these paths today, expect some prompt/chat internals to become dead as a direct consequence of this removal. If the compiler confirms there are no remaining callers, delete the dead items in this pass rather than preserving unreachable code:
- `PromptArgs`
- `ChatArgs`
- `run_prompt`
- `run_chat`
- `run_prompt_via_server`
- `run_chat_via_server`
### 2. Make `/api/v1/models` a real server-owned list endpoint
Treat `/api/v1/models` as a canonical non-demo API surface.
In `docs/api-reference/fabro-api.yaml`:
- add optional query parameters to `GET /api/v1/models`:
- `provider`
- `query`
- document `query` matching semantics explicitly:
- substring match
- case-insensitive
- fields: `id`, `display_name`, `aliases`
- add optional query parameter to `POST /api/v1/models/{id}/test`:
- `mode=basic|deep`
- default behavior when omitted: `basic`
- document `basic` as the current simple prompt/availability check
- document `deep` as the multi-turn tool-use / reasoning round-trip check
In `lib/crates/fabro-server/src/server.rs`:
- keep `/models/{id}/test` wired to the real server handler
- stop treating `/models` as demo semantics only
- replace the `demo::list_models` route usage with a non-demo handler
- implement a small query extractor for model filters and pagination in the real server path
- implement a small query extractor for model test mode in the `test_model` handler
- fix the `test_model` response to use `info.id` (the canonical model ID from the catalog lookup) instead of the raw `id` path parameter in all response branches, so alias lookups return the canonical ID
In `lib/crates/fabro-server/src/demo/mod.rs`:
- `demo::list_models` is currently wired in both `demo_routes()` and `real_routes()`; replace both usages with the new non-demo handler
- remove the now-unused `list_models` helper from the demo module
Behavior:
- server builds the list from `fabro_model::Catalog::builtin()`
- applies `provider` filter if present
- applies `query` filter if present
- paginates the filtered list
- returns the same `PaginatedModelList` schema shape as today
- preserves built-in catalog order after filtering
- rejects unknown `provider` values with `400`
- this means parse failure against `fabro_model::Provider`
- a valid parsed provider with zero matching models still returns `200` and an empty page
- `POST /models/{id}/test`:
- defaults to `basic`
- accepts either a canonical model ID or an alias
- runs the current simple one-prompt check in `basic` mode
- runs the deeper multi-turn tool-use / reasoning check in `deep` mode
- still returns one result object for one model
### 3. Make `fabro model` server-canonical
Remove standalone/server branching from the CLI `model` command family.
In `lib/crates/fabro-cli/src/commands/model.rs`:
- stop using `resolve_mode`
- stop passing `Option<ServerConnection>` into `run_models(...)`
- construct a typed `fabro_api::Client` up front and pass it through unconditionally
In `lib/crates/fabro-cli/src/server_client.rs`:
- keep the existing local auto-start + Unix-socket path for storage-backed connections
- add one small model-command helper that returns a typed `fabro_api::Client` from either:
- explicit remote base URL + configured TLS (`--server-url`), or
- local auto-start + Unix socket for the selected storage dir
- if needed, split the current Unix-only local connect helper into:
- a local auto-start helper, and
- a thin typed-client constructor for remote HTTP(S) base URLs
- do not route `model` through `ServerStoreClient`; `model` should use the generated API client directly
This change is intentionally command-specific:
- `--server-url` keeps working for remote server usage
- `--storage-dir` or default local storage uses local server auto-start
- `model` no longer branches on `ExecutionMode`
- global `resolve_mode` behavior for other commands stays unchanged in this pass
### 4. Simplify model selection and test orchestration
Keep the CLI in charge of bulk orchestration, but move catalog authority to the server.
In `lib/crates/fabro-llm/src/cli.rs`:
- replace the raw `reqwest` + `base_url` model HTTP helpers with generated `fabro_api::Client` calls
- update `fetch_models_from_server(...)` to forward `provider` and `query` to the server instead of filtering locally after the response
- update the fetch helper to follow pagination until `meta.has_more` is false instead of assuming one page is enough
- remove local provider filtering from the fetch helper
- remove the local query filtering in `run_models(...)` (the `if let Some(q) = &query { ... models.retain(...) }` block), since the server now handles query filtering
- simplify `run_models(...)` so it no longer accepts an optional server connection; `model` should always call the server-backed path
- keep `model test` fan-out behavior in the CLI:
- `model test --model <id-or-alias>` calls `POST /models/{id-or-alias}/test` once
- `model test --provider <provider>` first resolves the filtered list via `GET /models?provider=...`, then POSTs `/test` once per returned model
- bare `model test` first resolves the full list via `GET /models`, then POSTs `/test` once per model
- `model test --deep` maps each request to `POST /models/{id}/test?mode=deep`
- default `model test` behavior maps each request to `basic` mode
- any shared fetch helper used from test fan-out paths passes `query=None`, because query filtering remains list-only in this pass
Preserve current broad behavior where reasonable:
- `--model` remains the direct single-model selector and should pass the user-supplied ID or alias through unchanged
- unknown model inputs should continue to surface an “unknown model” style error when the server returns 404
- CLI fan-out result formatting and failure aggregation should remain substantially the same
- `model list --json` should continue to print a plain JSON array, not the servers paginated envelope
- CLI output order should preserve the server/catalog order
- CLI selection behavior remains:
- `--model` takes precedence over `--provider`
- query filtering applies only to `list`, not `test`, unless explicitly added later
### 5. Preserve `model test --deep` via server-owned test modes
`--deep` remains useful, but it should stop depending on the old local test execution branch. The server should own both single-model test modes, and the CLI should only orchestrate fan-out.
Server-side implementation:
- extract the single-model test logic out of CLI-only code and into a reusable non-CLI helper in `fabro-llm` so the server can execute:
- `basic` mode
- `deep` mode
- keep that logic in `fabro-llm`, not inline in the axum handler; this is LLM-domain behavior, not route-local glue
- shape the extracted helper around an internal outcome type that already matches the binary API contract:
- success => `status: ok`
- failure => `status: error` plus message detail
- preserve the current timeout budgets:
- `basic` uses the current short timeout budget (`30s`)
- `deep` uses the current long timeout budget (`90s`)
- keep deep mode's current in-process tool-closure pattern from `build_deep_test_params`; this pass does not add RPC or external tool-runner infrastructure
- `mode=deep` for a model without tool support returns HTTP `200` with `status: error` and a clear `error_message`
- if a reasoning-capable model completes deep mode without reasoning traces, do not fail for that fact alone in this pass
- the initial source material is the existing logic in `lib/crates/fabro-llm/src/cli.rs`:
- `build_deep_test_params`
- `validate_deep_result`
- current one-model test logic
- the end state should be:
- server calls a reusable `fabro-llm` helper for one-model test execution
- CLI no longer owns authoritative one-model test behavior
- do not leave deep behavior only in the CLI-local path once `model` becomes server-canonical
CLI-side implementation:
- keep `deep` on `ModelsCommand::Test`
- update the server-call helper(s) to pass `mode=deep` when requested
- remove only the dead local standalone branches once the server owns both modes
- remove the `"Warning: --deep is not supported in server mode"` diagnostic from `test_models_via_server`, since deep mode is now a server-owned capability
- remove the `deep_unsupported` field from `ModelTestOutput` JSON serialization, since deep mode is now fully supported via the server
In CLI help/snapshots:
- keep `--deep` on `model test --help`
- update wording if needed so it reflects a server-backed deep test rather than a local-only path
### 6. Dependencies And Sequencing
Apply this in order so the refactor has stable interfaces to land on:
- land the OpenAPI changes for `/models` filters and `/models/{id}/test?mode=basic|deep` first
- update the server list/test handlers, including provider parsing, alias handling, and canonical response IDs
- extract the reusable single-model `basic` / `deep` test helper in `fabro-llm`
- regenerate the Rust typed API client via `cargo build -p fabro-api`
- repoint `fabro model` and its fetch/test helpers to the generated `fabro_api::Client`
- remove the `fabro llm` CLI namespace and any compiler-confirmed dead prompt/chat code
- regenerate the TypeScript client once the server contract is settled
Section 2's endpoint and mode changes must land before replacing the deep-mode server flow described in Section 5.
### 7. Regenerate typed API clients through the normal workflow
Because `/api/v1/models` changes, follow the repos API workflow instead of hand-editing generated clients.
Source of truth:
- `docs/api-reference/fabro-api.yaml`
Generated/regenerated artifacts:
- Rust API client/types via `cargo build -p fabro-api`
- TypeScript client via `cd lib/packages/fabro-api-client && bun run generate`
Expected generated updates include:
- `lib/packages/fabro-api-client/src/api/models-api.ts`
- related generated model/type files under `lib/packages/fabro-api-client/src/models/`
## Important Interface / Behavior Changes
- `fabro llm` is removed from the CLI surface completely.
- `fabro model` always talks to the server.
- `GET /api/v1/models` accepts:
- `provider`
- `query`
- invalid `provider` values return `400`
- invalid `provider` means the value does not parse as a known `fabro_model::Provider`
- valid provider filters that simply match zero models still return `200` with an empty page
- filtered results preserve built-in catalog order
- `POST /api/v1/models/{id}/test` accepts:
- optional `mode=basic|deep`
- default mode `basic`
- `/models/{id}/test` accepts aliases but returns the canonical `model_id`
- `model test --model <alias>` posts that alias directly and relies on server-side alias resolution
- `model test --deep` remains supported and maps to `mode=deep`.
- `model list --json` continues to output a plain JSON array.
## Test Plan
CLI command surface:
- `lib/crates/fabro-cli/tests/it/cmd/fabro.rs`
- top-level help no longer lists `llm`
- `lib/crates/fabro-cli/tests/it/cmd/model.rs`
- bare `model` and `model list` still work
- provider/query list behavior still matches expected snapshots
- JSON output still parses as a plain array
- `model list` still works with an auto-started local server and with `--server-url`
- `lib/crates/fabro-cli/tests/it/cmd/model_list.rs`
- help text still matches the new server-backed behavior
- `lib/crates/fabro-cli/tests/it/cmd/model_test.rs`
- help text still mentions `--deep`
- unknown model still errors cleanly
- `--model <alias>` calls the single-model test endpoint directly and succeeds via server-side alias resolution
- deep mode still routes through the server-backed model test flow
- JSON output no longer includes `deep_unsupported`
- delete now-obsolete `llm` CLI tests:
- `lib/crates/fabro-cli/tests/it/cmd/llm.rs`
- `lib/crates/fabro-cli/tests/it/cmd/llm_prompt.rs`
Server/API behavior:
- `lib/crates/fabro-server/src/server.rs`
- add tests for `GET /api/v1/models` with:
- no filters
- `provider`
- `query`
- combined `provider + query`
- case-insensitive query matching
- pagination applied after filtering
- invalid provider returns `400`
- valid parsed provider + no matching query returns `200` with an empty page
- filtered results preserve catalog order
- extend `POST /api/v1/models/{id}/test` coverage for:
- omitted mode defaults to `basic`
- explicit `mode=basic`
- explicit `mode=deep`
- invalid mode rejected cleanly
- alias path values resolve successfully and return the canonical `model_id`
- unknown model still returns 404
- `mode=deep` on a model without tool support returns `200` with `status: error`
LLM CLI internals:
- `lib/crates/fabro-llm/src/cli.rs`
- update `fetch_models_from_server` unit tests to assert outgoing `provider` / `query` params
- add coverage that multi-page server responses are fully traversed
- update/remove tests that assumed client-side provider filtering
- add coverage that test fan-out paths call the shared fetch helper with `query=None`
- add or update server-call tests so deep mode is forwarded as `mode=deep`
- add helper-level coverage for the binary deep-mode contract:
- tool-less models return `error`
- missing reasoning traces alone do not fail
- remove only the dead standalone-only test coverage after server ownership is in place
Full verification:
- `cargo build -p fabro-api`
- `cargo nextest run -p fabro-cli --no-fail-fast`
- `cargo nextest run -p fabro-server`
- `cargo fmt --check --all`
- `cargo clippy --workspace -- -D warnings`
- `cd lib/packages/fabro-api-client && bun run generate`
## Assumptions And Defaults
- `fabro llm` is dead product surface and should be removed cleanly, not hidden.
- `exec` remains a separate product surface and is intentionally untouched in this pass.
- `GET /api/v1/models` remains a built-in catalog view; this pass does not introduce live provider discovery.
- CLI model fetches must traverse paginated `/models` responses until exhaustion.
- `model test` remains a CLI orchestrator over repeated single-model HTTP calls.
- local-storage-backed `fabro model` may auto-start the local server on first use.
- for local auto-started servers, model testing assumes the daemon inherits the CLI environment, including provider credentials.
- for explicit remote `--server-url` usage, provider credential availability is the remote server operator's responsibility.
- `POST /api/v1/models/{id}/test` uses a single optional query param for mode selection:
- `basic`
- `deep`
- Deep-mode validation failures are represented as `status: error` plus `error_message`, not a new intermediate status.
- If both `--model` and `--provider` are supplied to `model test`, keep the current effective behavior of prioritizing the explicit model selection rather than inventing a new validation rule in this pass.
- No backward-compatibility shims are needed for removed CLI commands or flags.

View file

@ -0,0 +1,570 @@
# Run-Adjacent Server-Only Cleanup
## Summary
Make the remaining user-facing run-adjacent commands server-only:
- `fabro resume`
- `fabro diff`
- `fabro fork`
- `fabro rewind`
- `fabro artifact list`
- `fabro artifact cp`
- `fabro pr create|list|view|merge|close`
- `fabro sandbox preview`
- `fabro sandbox ssh`
- `fabro sandbox cp`
After this pass, those commands should no longer resolve runs through local `runs/` directories or flatten `--storage-dir`. They should all target a server the same way the core lifecycle commands already do:
1. explicit `--server` / `FABRO_SERVER`
2. configured `[server].target`
3. default local server instance, auto-started via the default local storage dir
There is intentionally no separate “force local” override for these commands in this pass. If `[server].target` is configured, that target wins unless the user passes an explicit `--server` value pointing at a local Unix socket. This is a simplicity tradeoff, not an accidental regression.
This is cleanup/compaction, not a new subsystem. The goal is to finish the architectural move already made for `run`, `create`, `start`, `attach`, `logs`, `wait`, `ps`, `inspect`, and `rm`.
## Scope Boundaries
In scope:
- the commands listed above
- shared run-resolution helpers in `fabro-cli`
- missing thin server APIs needed to make sandbox-oriented commands truly server-only
- CLI help/docs/snapshots for the new contract
Out of scope:
- `fabro store dump`
- `fabro system df`
- `fabro system prune`
- hidden/internal commands like `__runner`
- changing run execution topology
- changing checkpoint format or git metadata format
- changing GitHub auth/secrets flows
## Problem Frame
The run lifecycle is only partially simplified today.
Core lifecycle commands are already server-only, but the rest of the run-adjacent surface still falls back to local path-based lookup through `ServerRunLookup` in [server_runs.rs](lib/crates/fabro-cli/src/server_runs.rs). That creates three kinds of drift:
- some commands still expose `--storage-dir` even though the real abstraction is now a server target
- some commands read local run files even when equivalent data already exists in server state/store
- some commands still reconnect directly from the CLI to sandboxes, which breaks the “server-owned runs” abstraction for remote targets
That is unnecessary complexity in a greenfield app with no production compatibility constraints.
## Key Decisions
- All in-scope commands become server-only.
- They flatten `ServerTargetArgs`, not `StorageDirArgs`.
- `--storage-dir` is removed from their public CLI surface.
- `FABRO_STORAGE_DIR` is not part of the public contract for these commands.
- If `[server].target` is configured, these commands use it unless an explicit `--server` is passed.
- There is no separate “force local default instance” flag.
- `ServerRunLookup` becomes local/admin-only.
- Keep it only for genuinely local maintenance commands like `store dump`, `system df`, and `system prune`.
- User-facing run commands should resolve selectors through `ServerSummaryLookup` plus server state/events.
- `fabro diff` is simplified to stored server-backed output only.
- Drop the live sandbox reconnect fallback.
- Drop `--stat` and `--shortstat`.
- Output comes only from stored diff data already present in run state (`final_patch` and per-node `diff`).
- If no stored diff exists, return a clear error.
- `fabro fork` and `fabro rewind` remain local git operations, but not local run-store operations.
- Run selection and run event/state loading come from the target server.
- Repo mutation still happens against the callers local checkout, which is the correct boundary.
- Before mutating the repo, the CLI should compare the current checkout against a durable stored repo identity and fail fast on obvious mismatch.
- `fabro pr *` stays CLI-owned for GitHub network calls, but server-owned for run lookup and pull-request record loading.
- This keeps the current GitHub App model intact while removing local run-dir dependence.
- `pr create` should use the same repo-mismatch guard as `fork` and `rewind` before operating on the callers checkout.
- `fabro artifact *` should reuse the existing stage artifact API instead of walking local `artifacts/` directories.
- The CLI can derive relevant stage IDs from `RunProjection.nodes` and call the existing stage artifact list/download endpoints.
- `fabro sandbox preview`, `fabro sandbox ssh`, and `fabro sandbox cp` should stop reconnecting to sandboxes directly from the CLI.
- The server owns the sandbox record and should own the reconnect.
- The CLI becomes a thin consumer of server responses.
- The server API should stay thin and capability-shaped.
- Reuse existing APIs where they already exist.
- Add only the missing endpoints required for truly server-only sandbox operations.
- Backward compatibility is not a goal.
- Remove obsolete flags and behavior instead of shimming them.
- Prefer deleting speculative unused surface over preserving it.
## Sequencing
This plan should land in two phases.
Phase 1: pure CLI/server-state cleanup using APIs that already exist
- `resume`
- `fork`
- `rewind`
- `pr *`
- `artifact list`
- `artifact cp` (the stage artifact list/download routes already exist and are implemented)
- `diff`
- Phase 1 OpenAPI cleanup for `diff`:
- delete `/api/v1/runs/{id}/files`
- shared `ServerSummaryLookup` / `ServerRunLookup` narrowing
- help/docs/snapshot churn for those commands
Phase 2: thin server API completion for sandbox-owned commands
- `sandbox preview`
- `sandbox ssh`
- `sandbox cp`
This keeps the simpler repoints from being blocked on the few missing server capability routes.
The final `ServerRunLookup` narrowing should happen only after the last in-scope command that still imports it has been migrated. Do not try to enforce an admin-only boundary halfway through the sequence while migrated and unmigrated command families still coexist.
## Implementation Changes
### 0. Add a durable repo identity field for local-repo operations
The current `RunRecord.host_repo_path` is a filesystem path, not a durable cross-machine identity. It should not be used for a server-targeted repo-mismatch guard.
Add a new persisted field to `RunRecord`, for example:
- `repo_origin_url: Option<String>`
Recommended semantics:
- source it from `RunManifest.git.origin_url`, which is already sanitized before it reaches the server
- persist it in the run record when a manifest-sourced run is created
- treat it as the canonical repo-identity signal for CLI commands that mutate or inspect the callers local checkout on behalf of a server-selected run
Normalization rules should be explicit and shared:
- strip embedded credentials
- normalize GitHub-style SSH URLs to HTTPS form
- trim a trailing `.git`
- trim trailing `/`
Guard behavior:
- `fork`, `rewind`, and `pr create` detect the current checkouts origin URL locally
- normalize it with the same helper used for the stored field
- if both sides are present and clearly differ, fail with a targeted repo-mismatch error
- if the stored field is absent, skip the guard rather than inventing a heuristic fallback
Files expected to change:
- [run.rs](lib/crates/fabro-types/src/run.rs)
- the manifest-backed run creation path that already receives `manifest.git.origin_url`
- any serialization/projection paths that persist and reload `RunRecord`
### 1. Convert remaining run-adjacent args to `ServerTargetArgs`
In [args.rs](lib/crates/fabro-cli/src/args.rs):
- replace `StorageDirArgs` with `ServerTargetArgs` for:
- `ArtifactListArgs`
- `ArtifactCpArgs`
- `CpArgs`
- `PreviewArgs`
- `SshArgs`
- `DiffArgs`
- `ResumeArgs`
- `RewindArgs`
- `ForkArgs`
- `PrCreateArgs`
- `PrListArgs`
- `PrViewArgs`
- `PrMergeArgs`
- `PrCloseArgs`
Also simplify `DiffArgs`:
- remove `stat`
- remove `shortstat`
- update help text to describe stored diff output only
Resulting CLI contract:
- `fabro diff <run> --server http://127.0.0.1:3000/api/v1`
- `fabro artifact list <run> --server /var/run/fabro.sock`
- `fabro sandbox ssh <run> --server https://fabro.example.com/api/v1`
- no `--storage-dir` on these commands
- no public `FABRO_STORAGE_DIR` support on these commands
### 2. Narrow shared lookup helpers
In [server_runs.rs](lib/crates/fabro-cli/src/server_runs.rs):
- keep `ServerSummaryLookup` as the default user-facing selector path
- add any missing helper methods needed for:
- selector resolution
- filtered summary listing
- summary-to-state/event follow-up work
- keep `ServerRunLookup` only for commands that remain explicitly local/admin-only
Do not try to finish that narrowing until the last in-scope migrated command is off `ServerRunLookup`.
In [server_client.rs](lib/crates/fabro-cli/src/server_client.rs):
- add thin wrappers for the server APIs the CLI now needs, split by phase:
- Phase 1:
- list stage artifacts
- download stage artifact
- Phase 2:
- generate preview URL
- create SSH access
- sandbox file listing/download/upload
Do not introduce a parallel target-resolution stack. Reuse:
- `connect_server_only(...)`
- `server_only_command_connection(...)`
If tests still need `FABRO_STORAGE_DIR` internally to steer the default local server instance, treat that as harness-only plumbing rather than user-facing behavior.
### 3. Repoint `resume` to the server-only lifecycle helpers
In [resume.rs](lib/crates/fabro-cli/src/commands/run/resume.rs):
- stop using `ServerRunLookup`
- resolve the run via `ServerSummaryLookup`
- call the existing direct-client start helper
- for foreground resume:
- attach through the existing direct-client attach helper
- print the existing server-backed summary output with no local `run_dir`
This is intentionally a small mechanical repoint. It should make `resume` match the already-simplified `start`/`attach` contract without introducing new behavior.
### 4. Repoint `fork` and `rewind` to server-backed run state
In [fork.rs](lib/crates/fabro-cli/src/commands/run/fork.rs) and [rewind.rs](lib/crates/fabro-cli/src/commands/run/rewind.rs):
- stop using `ServerRunLookup`
- resolve the run via `ServerSummaryLookup`
- fetch events/state via the resolved `ServerStoreClient`
- keep the local git/checkpoint mutation logic unchanged
- before mutating the local repo, validate obvious identity against stored run metadata:
- compare the current checkouts detected repo identity against `RunRecord.repo_origin_url` when present
- if they clearly do not match, fail with a targeted error instead of mutating the wrong checkout
In [rewind.rs](lib/crates/fabro-cli/src/commands/run/rewind.rs):
- remove dependence on `run.path` for rewound-state cleanup
- if a small local run-dir cleanup is still required, compute it server-side or remove it
- keep durable run-state restoration (`run.rewound`, restored checkpoint, `run.submitted`) server-backed
The point is to make rewind/fork depend on the local repo, not on local run-store layout.
### 5. Repoint `pr *` to server-backed run selection and record loading
In [pr/mod.rs](lib/crates/fabro-cli/src/commands/pr/mod.rs) and subcommands:
- remove `runs_base(...)` / `ServerRunLookup::connect_from_runs_base(...)`
- resolve runs through `ServerSummaryLookup`
- load pull-request state from `get_run_state(...)`
Specific changes:
- [pr/list.rs](lib/crates/fabro-cli/src/commands/pr/list.rs)
- replace `scan_runs_with_summaries(...)` with summary iteration from `ServerSummaryLookup`
- keep current GitHub detail-fetch fanout in the CLI
- [pr/create.rs](lib/crates/fabro-cli/src/commands/pr/create.rs)
- rebuild run state from server events instead of local path lookup
- keep local repo detection via `detect_repo_info(...)`
- apply the same repo-mismatch guard used by `fork` / `rewind` before proceeding
- [pr/view.rs](lib/crates/fabro-cli/src/commands/pr/view.rs)
- [pr/close.rs](lib/crates/fabro-cli/src/commands/pr/close.rs)
- [pr/merge.rs](lib/crates/fabro-cli/src/commands/pr/merge.rs)
- load the stored PR record from the target server only
### 6. Make `diff` fully server-backed and simpler
In [diff.rs](lib/crates/fabro-cli/src/commands/run/diff.rs):
- stop using `ServerRunLookup`
- resolve via `ServerSummaryLookup`
- load `RunProjection` from the target server
- keep only two sources of diff output:
- per-node `diff`
- run-level `final_patch`
- remove sandbox reconnect and live diff generation entirely
Behavior:
- `fabro diff <run>` prints `final_patch`
- `fabro diff <run> --node <id>` prints the stored node diff
- if no stored diff exists, error with a clear message
Because this pass intentionally simplifies the product surface:
- remove `--stat`
- remove `--shortstat`
- update docs/tests accordingly
Also remove dead speculative server API surface tied to the old diff shape:
- delete `/api/v1/runs/{id}/files` from [fabro-api.yaml](docs/api-reference/fabro-api.yaml)
- remove the corresponding `not_implemented` route from [server.rs](lib/crates/fabro-server/src/server.rs)
This is a Phase 1 OpenAPI/spec change and should be treated as part of that phase explicitly.
That API is currently unimplemented and unused by the CLI. Keeping it around only adds drift.
### 7. Repoint `artifact list` and `artifact cp` to the existing server artifact API
In [artifact/list.rs](lib/crates/fabro-cli/src/commands/artifact/list.rs) and [artifact/cp.rs](lib/crates/fabro-cli/src/commands/artifact/cp.rs):
- stop using `ServerRunLookup` and `RuntimeState`
- resolve the run via `ServerSummaryLookup`
- fetch `RunProjection`
- enumerate stage IDs from `RunProjection.nodes`
- use the existing stage artifact routes for each relevant stage:
- list artifact filenames
- download artifact bytes
Keep filtering behavior in the CLI:
- `--node`
- `--retry`
- tree/no-tree output layout
- filename collision handling
This preserves the current artifact UX while removing local artifact-dir reads.
### 8. Finish preview/SSH/file-transfer as real server-owned sandbox operations
This is the only part of the plan that needs new or completed server APIs.
#### 8a. Preview
The route already exists in [fabro-api.yaml](docs/api-reference/fabro-api.yaml) and [server.rs](lib/crates/fabro-server/src/server.rs), but the real handler is still `not_implemented`.
Implement it in [server.rs](lib/crates/fabro-server/src/server.rs):
- load the runs sandbox record from store/state
- reconnect server-side
- for Daytona:
- generate signed or unsigned preview URL as requested
- return `409` if:
- no active sandbox
- sandbox provider does not support preview
Then repoint [preview.rs](lib/crates/fabro-cli/src/commands/run/preview.rs) to the server API instead of direct Daytona reconnect.
#### 8b. SSH
Add a new route to [fabro-api.yaml](docs/api-reference/fabro-api.yaml):
- `POST /api/v1/runs/{id}/ssh`
Recommended request/response shape:
- request:
- `ttl_minutes`
- response:
- `command`
Implement the handler in [server.rs](lib/crates/fabro-server/src/server.rs):
- load sandbox record
- reconnect server-side
- generate SSH access for supported providers
- return `409` when unsupported or unavailable
Then repoint [ssh.rs](lib/crates/fabro-cli/src/commands/run/ssh.rs):
- `--print` prints the returned command
- non-`--print` locally `exec`s the returned command
That preserves current UX while removing direct CLI sandbox reconnect.
#### 8c. Sandbox file transfer (`fabro sandbox cp`)
Add a small server-owned file-transfer surface for sandboxes.
Recommended routes:
- `GET /api/v1/runs/{id}/sandbox/files`
- query:
- `path`
- optional `depth`
- returns directory entries
- `GET /api/v1/runs/{id}/sandbox/file`
- query:
- `path`
- returns raw file bytes
- `PUT /api/v1/runs/{id}/sandbox/file`
- query:
- `path`
- request body:
- raw file bytes
Implementation in [server.rs](lib/crates/fabro-server/src/server.rs):
- load sandbox record
- reconnect server-side
- delegate to the existing `Sandbox` trait:
- `list_directory`
- `download_file_to_local` equivalent via temp file or direct read/write helper
- `upload_file_from_local` equivalent via temp file or direct write helper
CLI changes in [cp.rs](lib/crates/fabro-cli/src/commands/run/cp.rs):
- stop reconnecting to sandboxes directly
- resolve runs via `ServerSummaryLookup`
- for recursive download:
- list directory via server
- download files one by one via server
- for upload:
- recursively walk local input
- upload files one by one via server
This keeps the current UX and avoids inventing a tar/archive protocol.
### 9. Update docs/help text to match the new surface
Update:
- [docs/reference/cli.mdx](docs/reference/cli.mdx)
- [docs/reference/user-configuration.mdx](docs/reference/user-configuration.mdx)
- [docs/core-concepts/how-fabro-works.mdx](docs/core-concepts/how-fabro-works.mdx)
The docs should explicitly reflect:
- in-scope run-adjacent commands now use `--server`, not `--storage-dir`
- `diff` is stored-output only
- sandbox preview/SSH/file transfer are server-mediated
- local storage-dir maintenance commands still exist, but they are not the normal user-facing run lifecycle
## Test Plan
### Help/parser coverage
Update snapshots in:
- [artifact_list.rs](lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs)
- [artifact_cp.rs](lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs)
- [diff.rs](lib/crates/fabro-cli/tests/it/cmd/diff.rs)
- [fork.rs](lib/crates/fabro-cli/tests/it/cmd/fork.rs)
- [resume.rs](lib/crates/fabro-cli/tests/it/cmd/resume.rs)
- [rewind.rs](lib/crates/fabro-cli/tests/it/cmd/rewind.rs)
- [pr_create.rs](lib/crates/fabro-cli/tests/it/cmd/pr_create.rs)
- [pr_list.rs](lib/crates/fabro-cli/tests/it/cmd/pr_list.rs)
- [pr_view.rs](lib/crates/fabro-cli/tests/it/cmd/pr_view.rs)
- [pr_close.rs](lib/crates/fabro-cli/tests/it/cmd/pr_close.rs)
- [pr_merge.rs](lib/crates/fabro-cli/tests/it/cmd/pr_merge.rs)
- [sandbox_cp.rs](lib/crates/fabro-cli/tests/it/cmd/sandbox_cp.rs)
- [sandbox_preview.rs](lib/crates/fabro-cli/tests/it/cmd/sandbox_preview.rs)
- [sandbox_ssh.rs](lib/crates/fabro-cli/tests/it/cmd/sandbox_ssh.rs)
Scenarios:
- help shows `--server`
- help no longer shows `--storage-dir`
- `diff --help` no longer shows `--stat` or `--shortstat`
- no docs/help text implies these commands honor `FABRO_STORAGE_DIR`
Use the normal snapshot workflow:
1. `cargo insta pending-snapshots`
2. inspect changes
3. `cargo insta accept`
### CLI targeting behavior
Add or update CLI tests for each in-scope command family:
- explicit `--server` wins
- configured `[server].target` is used when no flag is passed
- no explicit target uses the default local server instance
Concrete tests:
- `resume` uses configured server target without local run-dir lookup
- `artifact list` uses configured server target without local artifact-dir lookup
- `artifact cp` uses configured server target and downloads through the server
- `pr list` uses configured server target without scanning local runs/
- `pr view`/`pr merge`/`pr close` resolve records from the server target
- `sandbox preview` uses the server endpoint instead of direct Daytona reconnect
- `sandbox ssh --print` uses the server endpoint and prints the returned command
- `sandbox cp` upload/download works against a target server without CLI-side sandbox reconnect
- when `[server].target` is configured, these commands use it by default
- there is no separate local override path besides passing an explicit local `--server`
### Diff behavior coverage
In [diff.rs](lib/crates/fabro-cli/tests/it/cmd/diff.rs):
- completed run with stored final patch still prints patch
- missing stored final patch errors cleanly
- stored node diff still works
- remove tests that depend on live diff fallback semantics
### Fork/rewind/resume behavior coverage
In:
- [fork.rs](lib/crates/fabro-cli/tests/it/cmd/fork.rs)
- [rewind.rs](lib/crates/fabro-cli/tests/it/cmd/rewind.rs)
- [resume.rs](lib/crates/fabro-cli/tests/it/cmd/resume.rs)
Add server-target coverage:
- configured `[server].target` works without local run-store lookup
- explicit `--server` overrides configured target
- rewind/fork/resume continue to mutate only the local repo, not local run-store metadata files
- rewind/fork/pr-create fail fast when the selected runs stored `repo_origin_url` clearly does not match the current checkout
- rewind/fork/pr-create skip the guard cleanly when older runs do not have a stored durable repo identity yet
### Server API coverage
Add server tests in [server.rs](lib/crates/fabro-server/src/server.rs) or the server integration suite for:
- preview URL generation for a supported sandbox
- preview rejects missing/unsupported sandboxes with `409`
- SSH command generation for a supported sandbox
- SSH rejects missing/unsupported sandboxes with `409`
- sandbox file list/download/upload round-trip
- stage artifact list/download continues to work for the CLI use case
### Full verification
- `cargo fmt --check --all`
- `cargo clippy --workspace --all-targets -- -D warnings`
- `cargo nextest run --workspace`
## Risks
- The biggest risk is accidentally preserving local run-path assumptions under a server-only CLI surface.
- Mitigation: delete `--storage-dir` from in-scope commands and remove local lookup usage outright instead of trying to support both models.
- `fork`, `rewind`, and `pr create` still depend on the callers local repo matching the selected run closely enough.
- Mitigation: keep that boundary, but add an explicit repo-mismatch guard using stored run metadata so obvious mistakes fail fast.
- `sandbox cp` is the largest unit because it needs a new server-owned file transfer surface.
- Mitigation: keep the API thin and capability-shaped; do not design a generic virtual filesystem protocol.
- Preview/SSH capability is provider-specific.
- Mitigation: standardize on `409` for unsupported or unavailable sandbox capability.
## Follow-on
After this lands, the remaining local/admin seam should be small and explicit:
- `store dump`
- `system df`
- `system prune`
- any hidden/internal commands that truly operate on local storage
At that point, `ServerRunLookup` should either:
- be deleted entirely if those commands are also repointed later, or
- be clearly renamed/documented as a local maintenance helper rather than a normal user-facing run abstraction

View file

@ -0,0 +1,243 @@
# Run/Create Server Target Support
## Summary
Make `fabro run` and `fabro create` targetable via `--server` / `[server].target` now that run submission is manifest-based and server-owned.
This pass should:
- add `--server` support to `fabro run` and `fabro create`
- align their target-resolution semantics with `preflight`, `validate`, and `graph`
- remove the last local-storage-only assumptions from the run submission path
- keep local run behavior unchanged when a local server is selected
This is primarily cleanup/compaction, not a new subsystem. The manifest refactor already made remote submission possible; the CLI surface just has not caught up yet.
## Scope Boundaries
In scope:
- `fabro run`
- `fabro create`
- the internal start/attach/summary helpers required for `fabro run` to work against an explicit server target
- docs/help/tests for the new targeting contract
Out of scope:
- adding `--server` to top-level `fabro start`, `fabro attach`, `fabro wait`, `fabro logs`, `fabro inspect`, `fabro diff`, `fabro resume`, or `fabro rewind`
- changing the HTTP API
- changing manifest structure or workflow bundle persistence
- changing server-side run ownership or execution topology
Accepted temporary asymmetry:
- `fabro create --server ...` will be supported in this pass even though the standalone follow-up lifecycle commands remain local-only.
- That is acceptable because `create` already prints only a run ID and is useful for automation. A later pass can broaden remote targeting across the rest of the run lifecycle surface.
## Problem Frame
The CLI/server boundary is now inconsistent:
- `preflight`, `validate`, and `graph` already build manifests and target either a local auto-started server or an explicit remote `--server`
- `run` and `create` already build manifests, but they still only flatten `--storage-dir` and then hard-wire submission to `connect_server(settings.storage_dir())`
- `run` still assumes every submitted run has a meaningful local run directory for attach and final summary output
That is architectural drift. The system is already manifest-first and server-canonical. `run` and `create` are the remaining commands that still behave as if run submission is inherently local.
## Key Decisions
- `RunArgs` should flatten `ServerConnectionArgs`, not `StorageDirArgs`.
- `fabro create` inherits the same args because it already reuses `RunArgs`.
- `run` and `create` should use the same connection contract as other server-backed commands:
- explicit `--server` wins
- explicit `--storage-dir` selects a local server and suppresses configured `[server].target`
- otherwise configured `[server].target` may be used
- otherwise the command defaults to the local server for the resolved storage dir
- `fabro run` should use one resolved server connection end-to-end for:
- manifest submission
- `POST /runs/{id}/start`
- live attach / polling
- `fabro create` should remain run-ID-only output.
- It should not pretend there is always a local `run_dir`.
- foreground `fabro run` against a remote/configured server should attach successfully.
- This requires decoupling attach from local run-dir inference.
- remote foreground `run` should print a server-backed final summary that omits local-only fields.
- Keep: run ID, status, duration, cost/tokens, failure reason, PR URL, final output
- Omit: local run directory path and local artifact listing when there is no local run dir
- local `run` behavior should remain unchanged.
- If the resolved connection is local, keep the existing local run-dir summary and asset listing behavior.
- No OpenAPI change is required.
- This is CLI cleanup on top of the existing manifest-backed `POST /runs`.
## Implementation Changes
### 1. Add target args to `run` / `create`
In `lib/crates/fabro-cli/src/args.rs`:
- change `RunArgs` to flatten `ServerConnectionArgs`
- remove the dedicated `StorageDirArgs` field from `RunArgs`
- keep all existing workflow/run override flags unchanged
This updates both:
- `fabro run`
- `fabro create`
Help/CLI contract to lock down:
- `fabro run foo.fabro --server http://127.0.0.1:3000/api/v1`
- `fabro create foo.fabro --server /var/run/fabro.sock`
- `fabro run foo.fabro --storage-dir /tmp/fabro`
- `fabro create foo.fabro` still defaults to local storage unless `[server].target` is configured
### 2. Resolve run/create connections the same way as other server-backed commands
In `lib/crates/fabro-cli/src/commands/run/command.rs` and `lib/crates/fabro-cli/src/commands/run/create.rs`:
- keep using local user-config resolution for manifest defaults
- load settings with storage-dir override only, using the command-local `storage_dir` value if present
- stop deriving the submission client from `settings.storage_dir()`
- instead resolve the server connection with the existing server-backed connection logic in `lib/crates/fabro-cli/src/user_config.rs`
- connect using the resolved connection, not a hard-coded local store path
Recommended shape:
- let `create_run(...)` return a richer value than `(RunId, PathBuf)`, for example:
- `CreatedRun { run_id, local_run_dir: Option<PathBuf>, connection: ServerConnection }`
Rationale:
- `command::execute()` needs more than a run ID now
- remote runs have no trustworthy local run dir
- passing the resolved connection forward keeps the rest of the flow honest
In `lib/crates/fabro-cli/src/server_client.rs`:
- add a small helper that returns a `ServerStoreClient` from a resolved `ServerConnection`
- reuse the existing resolved API-client path rather than introducing parallel target parsing
### 3. Refactor `run` to start and attach through the resolved server connection
In `lib/crates/fabro-cli/src/commands/run/start.rs`:
- keep the current public/local helper for top-level `fabro start`
- add a connection-agnostic helper that can start a run from an already-connected `ServerStoreClient`
In `lib/crates/fabro-cli/src/commands/run/attach.rs`:
- preserve the existing top-level `attach_run(...)` entrypoint for local-storage workflows
- extract the existing server-backed attach logic into a helper that accepts:
- `&ServerStoreClient`
- `&RunId`
- `Option<&Path>` for a local run dir
- existing `kill_on_detach`, `styles`, and `json_output` flags
- make the current top-level local path delegate to that extracted helper after doing its storage-dir/run-id inference
In `lib/crates/fabro-cli/src/commands/run/command.rs`:
- for `fabro run`, use the resolved connection returned by `create_run(...)`
- if `--detach` is set:
- print the run ID and exit exactly as today
- otherwise:
- start via the resolved server client
- attach via the extracted direct-client attach helper
- print the final run summary using the same resolved connection
This keeps `fabro run` coherent for both:
- local auto-started server flows
- explicit/configured remote server flows
### 4. Decouple final summary rendering from local run-dir assumptions
In `lib/crates/fabro-cli/src/commands/run/output.rs`:
- split summary fetching from summary rendering
- make the renderer accept:
- server-backed run state / conclusion / checkpoint
- `Option<&Path>` for a local run dir
Concrete behavior:
- when `local_run_dir` is present:
- keep printing the local run path
- keep printing local artifact listings
- when `local_run_dir` is absent:
- do not print a local run path line
- do not attempt local artifact discovery
- still print the rest of the run conclusion and final output
This is the smallest cleanup that makes remote foreground `run` feel intentional without broadening the whole remote lifecycle command surface.
### 5. Keep standalone follow-up lifecycle commands local for now
Do **not** add `--server` to these commands in this pass:
- `fabro start`
- `fabro attach`
- `fabro wait`
- `fabro logs`
- `fabro inspect`
- `fabro diff`
- `fabro resume`
- `fabro rewind`
Rationale:
- they form a larger remote lifecycle surface with selector semantics, replay UX, and local-path assumptions of their own
- broadening them now would turn a cleanup pass into a larger capability expansion
But the plan should call the temporary boundary out explicitly in docs/help text where useful:
- `fabro create --server ...` is valid, but follow-up manipulation of that run outside `fabro run` remains a later pass
### 6. Update docs and help text
Update the user-facing references that describe server targeting:
- `docs/reference/cli.mdx`
- `docs/reference/user-configuration.mdx`
- `docs/administration/deploy-server.mdx`
The docs should explicitly say:
- `fabro run` and `fabro create` now honor `--server` / `[server].target`
- `fabro exec` still requires explicit `--server`
- top-level run lifecycle follow-up commands are still local-storage commands in this pass
## Test Plan
### CLI help / parser surface
Update snapshots in:
- `lib/crates/fabro-cli/tests/it/cmd/run.rs`
- `lib/crates/fabro-cli/tests/it/cmd/create.rs`
Scenarios:
- `run --help` shows both `--storage-dir` and `--server`
- `create --help` shows both `--storage-dir` and `--server`
### `create` targeting behavior
In `lib/crates/fabro-cli/tests/it/cmd/create.rs`:
- `create --server <http-target>` submits to the explicit server and prints the created run ID
- configured `[server].target` reroutes `create` when no explicit target args are passed
- explicit `--storage-dir` suppresses configured `[server].target`
- explicit `--server` overrides configured `[server].target`
- remote-targeted `create` does not require local run-dir inspection to succeed
### `run` targeting behavior
In `lib/crates/fabro-cli/tests/it/cmd/run.rs`:
- `run --server <http-target> --detach ...` submits and prints a run ID without relying on a local run dir
- foreground `run --server <http-target> ...` creates, starts, attaches, and exits successfully
- configured `[server].target` reroutes `run` when no explicit target args are passed
- explicit `--storage-dir` suppresses configured `[server].target`
- explicit `--server` overrides configured `[server].target`
- remote foreground `run` prints a final summary without a local run-directory line
- local `run --storage-dir ...` still prints the local run-directory line and local artifact section exactly as today
### Test infrastructure
Prefer a real TCP-bound fabro test server over `httpmock` for `run`.
Reason:
- `run` needs multiple real endpoints (`POST /runs`, `POST /runs/{id}/start`, event replay, run-state polling, question polling)
- mocking all of that would verify request wiring but not the actual remote run lifecycle
If the current CLI integration helpers do not already provide this, add a small reusable helper in:
- `lib/crates/fabro-cli/tests/it/support.rs`
or
- `lib/crates/fabro-test/src/lib.rs`
That helper should:
- launch a real fabro server bound to loopback TCP
- return a usable `http://127.0.0.1:PORT/api/v1` target string
- keep fixture storage isolated from the invoking CLIs local storage dir
## Risks
- The biggest risk is hidden local-run-dir assumptions in attach/summary code.
- Mitigation: refactor those surfaces explicitly rather than trying to fake a local path for remote runs.
- Config-target defaulting could surprise users if docs are not updated.
- Mitigation: update CLI docs and help snapshots in the same pass.
- Supporting `create --server` before the broader remote lifecycle commands is intentionally asymmetric.
- Mitigation: call it out in the plan/docs instead of pretending the whole run lifecycle is remote-ready.
## Follow-on
After this lands, the next logical cleanup is a dedicated remote run-lifecycle plan for:
- `start`
- `attach`
- `wait`
- `logs`
- `inspect`
- `diff`
- `resume`
- `rewind`
That should be a separate pass, not folded into this one.

View file

@ -0,0 +1,842 @@
# Run Manifest and Preflight
## Summary
Replace the current `POST /runs` request — which sends a filesystem path and relies on the server reading workflow definition files from disk — with a self-contained **run manifest**. The CLI gathers all workflow-definition inputs (DOT source, TOML configs, referenced prompt files, imported graphs, child workflows) into a single JSON payload. The server owns all interpretation: config merging, variable expansion, transforms, validation.
This also introduces `POST /api/v1/preflight`, which accepts the same manifest and returns a structured health report without creating a run.
After this change, the server no longer reads workflow/config/prompt/import files from the CLI's filesystem. It still uses the manifest `cwd` / resolved working directory for execution context (for example local sandbox and repo-aware behavior). The path-based `workflow_path` submission mode is removed.
## Scope Boundaries
In scope:
- define the `RunManifest` schema in the OpenAPI spec
- CLI-side manifest builder that walks the workflow tree and bundles all referenced files
- file resolver abstraction for transforms (bundle-backed instead of disk-backed)
- refactor `FileInliningTransform` and `ImportTransform` to use file resolver
- server-side config resolution from manifest layers (args, workflow TOML, project TOML, user TOML)
- child workflow resolution from the manifest's workflow map
- replace `POST /api/v1/runs` request body with the manifest
- new `POST /api/v1/preflight` endpoint using the same manifest
- update `fabro run`, `fabro create`, `fabro preflight` to build and send manifests
- demo mode for preflight and updated run creation
Out of scope:
- changes to run execution, checkpointing, or resume
- changes to sandbox creation or the execution engine
- changes to `fabro exec`
- encrypted or compressed manifests
- manifest size limits or streaming upload
## Manifest Shape
```json
{
"version": 1,
"run_id": "01HV6D7S5YF4Z4B2M7K4N0Q6T9",
"cwd": "/Users/user/p/my-project",
"git": {
"origin_url": "https://github.com/acme/my-app.git",
"branch": "feature/foo",
"sha": "abc123",
"clean": true
},
"goal": {
"type": "file",
"path": "goal.md",
"text": "Build and test the app..."
},
"args": {
"model": "claude-opus-4-6",
"sandbox": "local"
},
"target": {
"identifier": "smoke",
"path": "fabro/workflows/smoke/workflow.fabro"
},
"configs": [
{ "type": "project", "path": "fabro.toml", "source": "[fabro]\nroot = \"fabro/\"\n..." },
{ "type": "user", "path": "/Users/user/.fabro/user.toml", "source": "..." }
],
"workflows": {
"fabro/workflows/smoke/workflow.fabro": {
"source": "digraph { ... }",
"config": {
"path": "fabro/workflows/smoke/workflow.toml",
"source": "version = 1\n[vars]\nlanguage = \"rust\""
},
"files": {
"prompts/review.md": {
"content": "You are a code reviewer...",
"ref": { "type": "file_inline", "original": "@prompts/review.md", "from": "workflow.fabro" }
},
"validate.fabro": {
"content": "digraph { ... }",
"ref": { "type": "import", "original": "./validate.fabro", "from": "workflow.fabro" }
}
}
},
"fabro/workflows/implement-plan/workflow.fabro": {
"source": "digraph { ... }",
"files": {
"prompts/simplify.md": {
"content": "...",
"ref": { "type": "file_inline", "original": "@prompts/simplify.md", "from": "workflow.fabro" }
}
}
}
}
}
```
Field semantics:
- `version` — manifest schema version, currently `1`
- `run_id` — optional pre-generated run ID. Used by detached/local create flows that allocate the run ID in the CLI before submission
- `cwd` — the CLI's working directory at invocation time
- `git` — optional, observable git state from the CLI's working directory. Omitted if not in a git repo
- `origin_url` — remote origin URL, **sanitized** (credentials stripped from HTTPS URLs to prevent token leakage)
- `branch` — current branch name
- `sha` — current commit SHA
- `clean` — whether the working tree has uncommitted changes
- `goal` — resolved goal with provenance, always includes `text` (the content) and `type` (`"value"` for literal string, `"file"` for file-sourced, `"graph"` for graph-attribute-sourced). When `type` is `"file"`, includes `path` (original file path from TOML or CLI `--goal-file`). The server uses `text` directly and clears any merged `goal_file` path
- `args` — command-local run/preflight args that affect run settings. Sparse: omitted flags are absent. This is not a generic env layer or a dump of global CLI flags
- `target.identifier` — what the user typed (slug like `"smoke"` or path like `"./custom.fabro"`)
- `target.path` — resolved path, keys into the `workflows` map
- `configs` — non-workflow config sources, each with `type` (`"project"` or `"user"`), `path`, and raw TOML `source`
- `workflows` — flat map of all workflows (root + children), keyed by resolved path
- `source` — raw DOT source (unexpanded, pre-transform)
- `config` — optional, the workflow's TOML config with `path` and `source`
- `files` — map of normalized logical path (relative to that workflow's root directory) to file entry, each with `content` (file content) and `ref` (discovery metadata: `type`, `original` reference string, and optional `from` logical path for nested imports). Types: `file_inline` (`@file`), `import`, `dockerfile`. Note: `goal_file` no longer appears here — goals are in the top-level `goal` object
## Key Decisions
- **CLI gathers, server transforms.** The CLI's only job is reading files from disk and bundling them. All interpretation — TOML parsing, config merging, variable expansion, graph transforms, validation — happens server-side.
- **Detached/create flows keep CLI-allocated run IDs.** The manifest carries an optional `run_id`, preserving the current `run -d` / `create` behavior where the CLI can pre-generate the run ID before submission.
- **Flat workflow map.** All workflows (root and children, at any nesting depth) are in a single flat `workflows` map. Relationships are implicit via `stack.child_workflow` attributes in the DOT source. This avoids deep nesting and naturally deduplicates shared children.
- **Inline child workflows stay inline.** `stack.child_dot_source` continues to work exactly as it does today and does not need manifest bundling. Manifest child-workflow support is specifically for `stack.child_workflow` / `stack.child_dotfile`.
- **Child workflows don't have their own settings.** Today `parse_child_graph()` passes `Settings::default()` to children and never loads their TOML. The manifest preserves this — child workflow entries carry their DOT source and files but no separate config layers. If a child has a `workflow.toml`, it can optionally be included in `config` for future use, but the server does not merge it today.
- **Config resolution moves to the server.** The CLI currently merges `cli_args.combine(workflow_config).combine(project_config).combine(user_config).resolve()`. The manifest ships the raw layers and the server performs the merge. Merge precedence is determined by the server based on config `type`, not by array order.
- **Server merge precedence:** `args` > workflow `config` > `project` config > `user` config > server defaults. There is no separate manifest `env` layer. Server-owned operational settings such as `storage_dir`, `[server]`, `api`, `web`, `features`, `log`, and `exec` are ignored from manifest configs; the active server instance owns those.
- **File resolution must stay contextual.** Transforms currently resolve relative paths based on the current graph/file location. The manifest refactor cannot collapse that to `resolve("foo.md") -> content`; the resolver must accept the current logical directory so nested imports like `subflow/imported.fabro -> @prompts/foo.md` still resolve correctly.
- **Goal is resolved by the CLI and travels as a top-level object.** The CLI resolves the final goal using the current precedence rules (`--goal` / `--goal-file` over merged config `goal` / `goal_file`, otherwise graph-level `goal`) and sends it as `manifest.goal`. The server applies `manifest.goal.text` after config merge and clears `goal_file`, so goal handling never requires filesystem reads server-side.
- **Git state travels in the manifest.** The CLI captures origin URL (sanitized — credentials stripped from HTTPS URLs), current branch, commit SHA, and clean/dirty status. This replaces the server's need to run git commands or access the repo filesystem. Credential sanitization is mandatory to prevent token leakage in HTTPS URLs with embedded PATs or installation tokens.
- **`workflow_path` mode is removed.** After migration, the server only accepts manifests. The `dot_source` / `workflow_path` fields in `CreateRunRequest` are replaced by the manifest.
- **Preflight uses the same manifest.** `POST /api/v1/preflight` accepts a `RunManifest` and returns a `PreflightResponse` with workflow diagnostics plus the rendered checks payload. No validated manifest round-trip.
- **Manifest discovery walks the DOT AST.** The CLI must parse the DOT source enough to find `@file` references (in `prompt` and `goal` attributes), `import` attributes, and `stack.child_workflow` / `stack.child_dotfile` attributes. It does NOT run the full transform pipeline — just scans for file references.
## Implementation Changes
### 1. File resolver abstraction
Create `lib/crates/fabro-workflow/src/file_resolver.rs`.
```rust
pub trait FileResolver: Send + Sync {
/// Resolve a logical reference string relative to the current logical directory.
/// Returns the normalized logical path plus file content.
fn resolve(&self, current_dir: &Path, reference: &str) -> Option<ResolvedFile>;
}
pub struct ResolvedFile {
pub logical_path: PathBuf,
pub content: String,
}
```
One implementation:
**`BundleFileResolver`** — reads from a manifest's files map:
```rust
pub struct BundleFileResolver {
files: HashMap<PathBuf, String>,
}
```
The resolver normalizes `current_dir.join(reference)` into a workflow-relative logical path (strip leading `./`, collapse `.` / `..`) and looks up that normalized key. This preserves the current import/file-inlining semantics without any filesystem access. The `files` map is built from the manifest's `ManifestFileEntry` objects (extracting `content` by normalized logical path key).
No `DiskFileResolver` is needed — this is a hard cutover. The existing filesystem-based resolution logic in the transforms is replaced entirely.
Add `pub mod file_resolver;` to `lib/crates/fabro-workflow/src/lib.rs`.
Tests:
- `BundleFileResolver` with test data, verify exact key lookup works
- Verify `None` for missing files
- Verify path normalization handles `./` prefix stripping and nested `..`
- Verify nested import scoping resolves relative to the imported file's logical directory
### 2. Refactor FileInliningTransform
In `lib/crates/fabro-workflow/src/transforms/file_inlining.rs`:
Change the struct to hold a resolver instead of paths:
```rust
pub struct FileInliningTransform {
resolver: Arc<dyn FileResolver>,
}
impl FileInliningTransform {
pub fn new(resolver: Arc<dyn FileResolver>) -> Self {
Self { resolver }
}
}
```
Update `resolve_file_ref` to use the resolver:
- strip the `@` prefix to get the relative path
- call `self.resolver.resolve(current_dir, path_str)` instead of `std::fs::read_to_string`
- thread the current logical directory through the transform so imported files can inline their own relative references correctly
- remove tilde expansion and `canonicalize` logic (the CLI resolved all paths during bundling)
The `apply()` method stays structurally the same — it iterates node prompts and graph goal, calling the updated resolution logic.
### 3. Refactor ImportTransform
In `lib/crates/fabro-workflow/src/transforms/import.rs`:
Same pattern — hold a resolver:
```rust
pub struct ImportTransform {
resolver: Arc<dyn FileResolver>,
}
```
Update `resolve_import_path` and `prepare_import`:
- `resolve_import_path` uses `self.resolver.resolve(current_dir, path_str)` instead of filesystem canonicalize
- `prepare_import` gets the file content from the resolver instead of `std::fs::read_to_string`
- when applying `FileInliningTransform` to imported content, pass the same resolver plus the imported file's logical parent directory
The recursive import expansion and circular import detection stay the same.
### 4. Update transform pipeline
In `lib/crates/fabro-workflow/src/pipeline/transform.rs`:
Change `TransformOptions` to carry a resolver:
```rust
pub struct TransformOptions {
pub file_resolver: Option<Arc<dyn FileResolver>>,
pub custom_transforms: Vec<Box<dyn Transform>>,
}
```
Update the `transform` function:
- where it currently checks `options.base_dir.is_some()` to gate `ImportTransform` and `FileInliningTransform`, check `options.file_resolver.is_some()` instead
- construct the transforms with the resolver
### 5. Manifest schema in OpenAPI
In `docs/api-reference/fabro-api.yaml`, add schemas:
```yaml
RunManifest:
description: Self-contained workflow run manifest.
type: object
required:
- version
- cwd
- target
- workflows
properties:
version:
type: integer
description: Manifest schema version.
example: 1
run_id:
type: string
nullable: true
description: Optional pre-generated run ID to use instead of allocating a new ULID.
example: "01HV6D7S5YF4Z4B2M7K4N0Q6T9"
cwd:
type: string
description: CLI working directory at invocation time.
git:
$ref: "#/components/schemas/ManifestGit"
goal:
$ref: "#/components/schemas/ManifestGoal"
args:
$ref: "#/components/schemas/ManifestArgs"
target:
$ref: "#/components/schemas/ManifestTarget"
configs:
type: array
items:
$ref: "#/components/schemas/ManifestConfig"
workflows:
type: object
additionalProperties:
$ref: "#/components/schemas/ManifestWorkflow"
ManifestGit:
description: Observable git state from the CLI working directory.
type: object
required:
- origin_url
- branch
- sha
- clean
properties:
origin_url:
type: string
description: >
Remote origin URL, sanitized (credentials stripped from HTTPS URLs).
e.g. https://user:token@github.com/acme/app.git becomes https://github.com/acme/app.git
example: "https://github.com/acme/my-app.git"
branch:
type: string
description: Current branch name.
example: feature/foo
sha:
type: string
description: Current commit SHA.
example: abc123def
clean:
type: boolean
description: Whether the working tree has uncommitted changes.
ManifestGoal:
description: Resolved goal with provenance.
type: object
required:
- type
- text
properties:
type:
type: string
enum:
- value
- file
- graph
description: >
How the goal was sourced: "value" (literal from TOML goal field or --goal flag),
"file" (resolved from goal_file), "graph" (from graph-level goal attribute in DOT).
text:
type: string
description: The resolved goal content. Server uses this directly.
path:
type: string
description: Original file path (only present when type is "file").
ManifestTarget:
type: object
required:
- identifier
- path
properties:
identifier:
type: string
description: What the user typed (slug or path).
example: smoke
path:
type: string
description: Resolved path, keys into the workflows map.
example: fabro/workflows/smoke/workflow.fabro
ManifestConfig:
type: object
required:
- type
properties:
type:
type: string
enum:
- project
- user
path:
type: string
description: Filesystem path to the config file.
source:
type: string
description: Raw TOML source of the config file.
ManifestWorkflowConfig:
type: object
required:
- path
- source
properties:
path:
type: string
description: Path to the workflow TOML file.
source:
type: string
description: Raw TOML source.
ManifestArgs:
description: Command-local run/preflight flags that affect run settings. All fields optional (sparse).
type: object
properties:
model:
type: string
provider:
type: string
sandbox:
type: string
verbose:
type: boolean
dry_run:
type: boolean
auto_approve:
type: boolean
no_retro:
type: boolean
preserve_sandbox:
type: boolean
label:
type: array
items:
type: string
ManifestFileEntry:
description: A bundled file with discovery metadata.
type: object
required:
- content
- ref
properties:
content:
type: string
description: File content.
ref:
$ref: "#/components/schemas/ManifestFileRef"
ManifestFileRef:
description: How this file was discovered.
type: object
required:
- type
- original
properties:
type:
type: string
enum:
- file_inline
- import
- dockerfile
description: Discovery type.
original:
type: string
description: The reference string as it appeared in the DOT/TOML.
example: "@prompts/review.md"
from:
type: string
description: Optional logical path of the file/graph that referenced this entry.
ManifestWorkflow:
type: object
required:
- source
properties:
source:
type: string
description: Raw DOT source (unexpanded, pre-transform).
config:
$ref: "#/components/schemas/ManifestWorkflowConfig"
files:
type: object
additionalProperties:
$ref: "#/components/schemas/ManifestFileEntry"
description: >
Map of normalized logical path to file entry with content and discovery metadata.
```
Replace the `CreateRunRequest` schema with `RunManifest` on `POST /api/v1/runs`.
Add `POST /api/v1/preflight`:
```yaml
/api/v1/preflight:
post:
operationId: runPreflight
tags: [Runs]
summary: Validate a workflow manifest without creating a run.
description: >
Accepts the same manifest as POST /runs. Validates the workflow,
checks sandbox availability, LLM provider access, and GitHub token
minting. Returns a structured pass/fail report.
requestBody:
required: true
content:
application/json:
schema:
$ref: "#/components/schemas/RunManifest"
responses:
"200":
description: Preflight report.
content:
application/json:
schema:
$ref: "#/components/schemas/PreflightResponse"
```
The preflight response should preserve the current CLI JSON contract:
- `workflow` summary/diagnostics from validation
- `checks` as the rendered report payload
So add:
```yaml
PreflightResponse:
type: object
required:
- workflow
- checks
properties:
workflow:
$ref: "#/components/schemas/PreflightWorkflowSummary"
checks:
$ref: "#/components/schemas/DiagnosticsReport"
```
Rebuild `fabro-api`:
```
cargo build -p fabro-api
```
### 6. Server-side config resolution from manifest
Create `lib/crates/fabro-server/src/manifest.rs`.
This module:
1. **Parses run-relevant config layers from the manifest:**
- `args` → converted to a `ConfigLayer` (map CLI arg names to `ConfigLayer` fields, similar to `TryFrom<&RunArgs>` in `overrides.rs`)
- workflow `config.source` → parsed as TOML via `fabro_config` and converted to `ConfigLayer`
- each `configs[]` entry → parsed as TOML and converted to `ConfigLayer`
- strip or ignore non-run fields from uploaded configs (`storage_dir`, `[server]`, `api`, `web`, `features`, `log`, `exec`, `max_concurrent_runs`)
2. **Merges in server-determined precedence:**
```rust
pub fn resolve_settings(manifest: &RunManifest, server_defaults: &Settings) -> Result<Settings> {
let args_layer = parse_args_layer(&manifest.args)?;
let workflow_layer = parse_workflow_config(manifest)?;
let project_layer = find_config_layer(manifest, "project")?;
let user_layer = find_config_layer(manifest, "user")?;
args_layer
.combine(workflow_layer)
.combine(project_layer)
.combine(user_layer)
.resolve()
}
```
3. **Applies the top-level goal after merge.**
- if `manifest.goal` is present, set `settings.goal = Some(manifest.goal.text.clone())`
- clear `settings.goal_file` so server-side goal handling never tries to read the filesystem
4. **Builds a `BundleFileResolver`** from the target workflow's `files` map.
5. **Constructs a `CreateRunInput`** with:
- `WorkflowInput::DotSource { source, base_dir: None }` — using the raw DOT from the manifest
- resolved `Settings`
- `cwd` from the manifest
- a `file_resolver` for the transform pipeline
The `parse_args_layer` function maps manifest `args` keys to `ConfigLayer` fields. The mapping mirrors the current `TryFrom<&RunArgs>` / `TryFrom<&PreflightArgs>` logic in `overrides.rs`, but without `goal` / `goal_file` because those are represented by the top-level `goal` object. The keys are the same field names: `model`, `provider`, `sandbox`, `verbose`, `dry_run`, `auto_approve`, `no_retro`, `preserve_sandbox`, `label`.
### 7. Update POST /runs handler
In `lib/crates/fabro-server/src/server.rs`:
Replace the current `create_run` handler. The new handler:
1. Deserializes the request body as `RunManifest`.
2. Validates manifest version is supported.
3. Calls `manifest::resolve_settings(&manifest, &state.settings)` to merge configs.
4. Looks up the root workflow in `manifest.workflows` using `manifest.target.path`.
5. Builds a `BundleFileResolver` from the root workflow's `files` map.
6. Parses `manifest.run_id` when present, preserving the current detached/local create behavior.
7. Constructs `CreateRunInput`:
```rust
CreateRunInput {
workflow: WorkflowInput::DotSource {
source: root_workflow.source.clone(),
base_dir: None,
},
settings,
cwd: PathBuf::from(&manifest.cwd),
workflow_slug: Some(manifest.target.identifier.clone()),
run_id: Some(run_id),
host_repo_path: None,
base_branch: None,
}
```
8. Passes the manifest's workflow map to `operations::create()` so child workflows can be resolved later.
The `operations::create()` and `validate()` paths in `fabro-workflow` need to accept the file resolver (via `TransformOptions`) and the workflow map (for child resolution). This requires updating `CreateRunInput`, `ValidateInput`, or shared workflow-resolution state:
```rust
pub struct CreateRunInput {
pub workflow: WorkflowInput,
pub settings: Settings,
pub cwd: PathBuf,
pub workflow_slug: Option<String>,
pub run_id: Option<RunId>,
pub host_repo_path: Option<String>,
pub base_branch: Option<String>,
pub file_resolver: Option<Arc<dyn FileResolver>>,
pub workflow_bundle: Option<HashMap<String, ManifestWorkflow>>,
}
```
In `operations::create()` (`create.rs`) and `validate()` (`validate.rs`), when `file_resolver` is `Some`, use it in `TransformOptions` instead of relying on `base_dir`.
### 8. Child workflow resolution from manifest
In `lib/crates/fabro-workflow/src/handler/manager_loop.rs`:
`parse_child_graph()` currently resolves `stack.child_workflow` as `WorkflowInput::Path` and reads from disk. Update it to check for a workflow bundle first:
```rust
fn parse_child_graph(node: &Node, services: &EngineServices) -> Result<...> {
// ... existing stack.child_dot_source handling ...
if let Some(path) = node.attr("stack.child_workflow").or(node.attr("stack.child_dotfile")) {
let bundle = services.workflow_bundle.as_ref()
.ok_or_else(|| anyhow!("no workflow bundle available"))?;
let child = bundle.get(path)
.ok_or_else(|| anyhow!("child workflow not found in manifest: {path}"))?;
let resolver = BundleFileResolver::new(child.files.clone());
// Pass resolver plus the child's logical root to validate()
Ok(WorkflowInput::DotSource {
source: child.source.clone(),
base_dir: None,
})
}
}
```
Add `workflow_bundle: Option<Arc<HashMap<String, ManifestWorkflow>>>` to `EngineServices` so it is accessible during execution.
When the child workflow is resolved from the bundle, its own `files` map provides a scoped `BundleFileResolver` for that child's transforms (`@file` refs, imports within the child).
### 9. CLI manifest builder
Create `lib/crates/fabro-cli/src/manifest_builder.rs`.
This module builds a `RunManifest` from CLI inputs:
```rust
pub struct ManifestBuilder;
impl ManifestBuilder {
pub fn build_for_run(cwd: PathBuf, args: &RunArgs) -> Result<RunManifest> { ... }
pub fn build_for_preflight(cwd: PathBuf, args: &PreflightArgs) -> Result<RunManifest> { ... }
}
```
The build process:
1. **Resolve workflow path**: call `project_config::resolve_workflow_path(&args.workflow, &cwd)` to get the `.fabro` file path and optional `.toml` config path.
2. **Read the root workflow**:
- read the `.fabro` file: `std::fs::read_to_string(&dot_path)`
- if a `.toml` exists, read it: `std::fs::read_to_string(&toml_path)`
3. **Discover file references in the DOT source**:
- parse the DOT source with `parser::parse(&source)`
- scan all nodes for `prompt` attributes starting with `@` → collect file paths
- scan graph-level `goal` attribute for `@` prefix → collect file path
- scan all nodes for `import` attributes → collect file paths
- scan all nodes for `stack.child_workflow` / `stack.child_dotfile` attributes → collect child workflow paths
4. **Resolve the final goal**:
- compute the final goal using the current precedence rules (`--goal` / `--goal-file` over merged config `goal` / `goal_file`, otherwise graph-level `goal`)
- store it in top-level `manifest.goal`
- do **not** add `goal_file` to the workflow `files` map
5. **Resolve file references from the TOML config**:
- if the TOML has `sandbox.daytona.snapshot.dockerfile.path`, read the Dockerfile and add to `files`
6. **Read all discovered files** into the `files` map, keyed by normalized logical path relative to the workflow root. Resolve relative to the `.fabro` file's parent directory, with `~/.fabro` as fallback (matching current `resolve_file_ref` logic).
7. **Recursively process child workflows** (step 3-6 for each child). Children go into the flat `workflows` map. Detect circular references via a visited set.
8. **Process imported `.fabro` files**: imports also go into the workflow's `files` map (they are read and their content is stored under workflow-relative logical paths). Imported files may themselves contain `@file` refs and nested imports — the builder must recursively discover these too.
9. **Gather configs**:
- read `fabro.toml` via `project_config::discover_project_config()`
- read `~/.fabro/user.toml` via the user config path
- for each, record `type`, `path`, and raw `source`
10. **Gather args**: serialize the command-local args that affect run settings. Use the same field names as `ConfigLayer` (`model`, `provider`, `sandbox`, etc.) but exclude `goal` / `goal_file` because those are represented by top-level `manifest.goal`. Only include fields that the user actually set (sparse).
11. **Carry the optional run ID** from `RunArgs.run_id` when present.
12. **Assemble and return** the `RunManifest`.
The DOT parser is already available in `fabro-workflow`. The CLI already depends on `fabro-workflow` (it calls `validate()`). The manifest builder reuses the parser for discovery but does NOT run transforms.
### 10. Update CLI commands
In `lib/crates/fabro-cli/src/commands/run/create.rs`:
Replace the current flow:
```rust
// Before (sends path + pre-resolved settings):
let settings = cli_args_config.combine(workflow_config).combine(cli_defaults).resolve()?;
client.create_run_from_workflow_path(workflow_path, &cwd, &settings, run_id)
// After (sends manifest):
let manifest = ManifestBuilder::build_for_run(cwd, &args)?;
client.create_run_from_manifest(&manifest)
```
Remove `create_run_from_workflow_path` from `server_client.rs`. Add `create_run_from_manifest` that POSTs the manifest JSON.
The optional local validation step (lines 39-51) can be removed — the server validates as part of run creation. Or it can stay as a fast-fail with a note that it won't catch everything the server checks.
In `lib/crates/fabro-cli/src/commands/run/overrides.rs`:
The `TryFrom<&RunArgs> for ConfigLayer` conversion is no longer needed for the settings merge (the server does it). But the args serialization for the manifest's `args` field needs similar logic. Consider:
- keeping the conversion as a helper for building the manifest's `args` object
- or writing a new `RunArgs::to_manifest_args()` method that produces a JSON map
In `lib/crates/fabro-cli/src/commands/preflight.rs`:
Replace the current flow:
```rust
// Before (runs all checks CLI-side):
let settings = cli_args_config.combine(workflow_config).combine(cli_defaults).resolve()?;
validate(ValidateInput { ... })?;
run_preflight(&settings, ...)?;
// After (sends manifest to server):
let manifest = ManifestBuilder::build_for_preflight(cwd, &args)?;
let cli_settings = load_user_settings_with_storage_dir(args.storage_dir.as_deref())?;
let client = server_client::connect_server(&cli_settings.storage_dir()).await?;
let response = client.run_preflight().body(manifest).send().await?;
render_report(&response.checks);
```
Remove all local preflight check functions. The CLI becomes a thin client that builds the manifest, sends it, and renders the response.
### 11. Preflight handler on server
In `lib/crates/fabro-server/src/server.rs`:
```rust
async fn run_preflight(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
Json(manifest): Json<RunManifest>,
) -> Response {
let report = preflight::run_preflight(&state, &manifest).await;
(StatusCode::OK, Json(report)).into_response()
}
```
Create `lib/crates/fabro-server/src/preflight.rs`.
This module adapts the checks from the current CLI-side `preflight.rs`:
1. **Resolve settings** from manifest via `manifest::resolve_settings()`.
2. **Validate the workflow** — parse, transform (using `BundleFileResolver`), validate.
3. **Check sandbox** — resolve sandbox provider from settings, attempt to create/initialize a test sandbox.
4. **Check LLM providers** — for each model used by the graph, verify the provider is configured (secret exists in secret store) and reachable.
5. **Check GitHub token** — if the workflow has `github.permissions`, attempt to mint a test installation access token.
Each check produces a `CheckResult`. The function returns a `PreflightResponse` with:
- `workflow` summary (`name`, node/edge counts, goal, diagnostics)
- `checks` as the `DiagnosticsReport` payload
Add route in `real_routes()`:
```rust
.route("/preflight", post(run_preflight))
```
### 12. Demo mode
In `lib/crates/fabro-server/src/demo/mod.rs`:
**Run creation demo**: The demo handler already exists for `POST /runs`. Update it to accept the manifest schema. The demo can ignore the manifest contents and return a canned run response as it does today.
**Preflight demo**: Return an all-passing preflight report:
```rust
pub(crate) async fn run_preflight(
_auth: AuthenticatedService,
State(_state): State<Arc<AppState>>,
Json(_manifest): Json<RunManifest>,
) -> Response {
(StatusCode::OK, Json(serde_json::json!({
"workflow": {
"name": "demo",
"nodes": 3,
"edges": 2,
"goal": "Demo",
"diagnostics": []
},
"checks": {
"version": fabro_util::version::FABRO_VERSION,
"sections": [
{
"title": "Workflow",
"checks": [
{ "name": "Parse & Validate", "status": "pass", "summary": "3 nodes, 2 edges, goal set", "details": [] },
]
},
{
"title": "Sandbox",
"checks": [
{ "name": "Provider", "status": "pass", "summary": "local sandbox available", "details": [] },
]
},
{
"title": "LLM",
"checks": [
{ "name": "Providers", "status": "pass", "summary": "Anthropic, OpenAI reachable", "details": [] },
]
},
]
}
}))).into_response()
}
```
Wire in `demo_routes()`:
```rust
.route("/preflight", post(demo::run_preflight))
```
### 13. Cleanup
After the manifest is working:
- **Remove `workflow_path` mode** from the `POST /runs` handler. Remove the `workflow_path`, `cwd`, `settings_json` fields from the request schema. Remove `CreateRunRequest` from the OpenAPI spec and replace with `RunManifest`.
- **Keep `WorkflowInput::Path` for local-only CLI commands.** Do not remove it from `source.rs`; commands like `validate` and `graph` still use local path resolution. The cleanup is server-specific: remove the server's path-based submission path, not the workflow crate's local path input abstraction.
- **No `DiskFileResolver` to remove** — it was never created (hard cutover).
- **Remove `create_run_from_workflow_path`** from `server_client.rs`.
- **Remove `ConfigLayer::for_workflow()`** usage from CLI run/preflight commands (the server does config resolution now). The function itself may still be useful for other CLI code paths.
- **Remove CLI-side preflight check functions** from `preflight.rs`.
- **Remove `TryFrom<&RunArgs> for ConfigLayer`** if replaced by manifest args serialization.
## Implementation Order
```
1 File resolver trait + BundleFileResolver (no deps)
2 Refactor FileInliningTransform to use resolver (depends on 1)
3 Refactor ImportTransform to use resolver (depends on 1)
4 Update transform pipeline (TransformOptions) (depends on 2, 3)
5 Manifest schema in OpenAPI + rebuild fabro-api (no deps, parallel with 1-4)
6 Server-side config resolution (manifest.rs) (depends on 5)
7 CLI manifest builder (depends on 5)
8 Update POST /runs handler to accept manifest (depends on 4, 6)
9 Child workflow resolution from manifest (depends on 8)
10 Update CLI run/create to send manifest (depends on 7, 8)
11 Preflight server handler (depends on 4, 6)
12 Update CLI preflight to send manifest (depends on 7, 11)
13 Demo mode (depends on 5)
14 Cleanup: remove workflow_path mode + dead code (depends on 10, 12)
```
Steps 1-4 (resolver refactor) and 5 (schema) can proceed in parallel. Step 7 (CLI builder) and 6 (server config) can proceed in parallel once the schema exists.
## Resolved Questions
1. **`args` field schema**: **Typed and limited to run settings.** The OpenAPI schema defines explicit fields matching the command-local run/preflight overrides (model, provider, sandbox, verbose, dry_run, auto_approve, no_retro, preserve_sandbox, label). `goal` / `goal_file` are excluded because the final goal is represented by top-level `manifest.goal`. All fields are optional (sparse — only set fields are present).
2. **Import path scoping**: **Workflow-relative logical paths plus contextual resolution.** All file paths in a workflow's `files` map are normalized logical paths relative to that workflow's root directory. Each file entry carries `ref` metadata with `type`, `original`, and optional `from`. The server resolves nested imports and `@file` references by passing the current logical directory into the resolver; it does not use metadata fields for lookup.
3. **Backward compatibility**: **Hard cutover.** The old `CreateRunRequest` (workflow_path/dot_source) is removed when the manifest lands. CLI and local server are the same binary, so they upgrade together. Remote servers need coordinated upgrade.
4. **DOT parser for discovery**: **Full parse.** The CLI uses the existing `fabro-workflow` parser (already a dependency). It's fast, handles all edge cases (quoted strings, comments, escapes), and is more reliable than regex scanning.
5. **No manifest `env` layer**: the current CLI does not have a separate run-settings env layer like `FABRO_MODEL` or `FABRO_PROVIDER`. Manifest config precedence is `args` > workflow config > project config > user config > server defaults, with server-owned operational settings stripped from uploaded configs.

View file

@ -0,0 +1,984 @@
# Server-Canonical Secrets, Doctor, and Repo Init
## Summary
Migrate five CLI command families from local-only to server-canonical:
- **secrets** — move from `~/.fabro/.env` to server-owned JSON store with write-only API
- **provider login** — keep validation in CLI, save credentials via server API
- **repo init** — call server to verify repo access after scaffolding
- **doctor** — replace local probing with a single server diagnostics endpoint
- **health** — add server version to `GET /health` for CLI/server parity checks
After this pass, the CLI has no direct file I/O for secrets and no direct probing of external services for health checks. The server is the single owner of credentials and the single source of diagnostic truth.
This plan deliberately excludes the run manifest (server-canonical `POST /runs` body). That is a separate, larger effort.
## Scope Boundaries
In scope:
- add `version` to `GET /health`
- server-side secret store (JSON file, in-memory cache, store-backed secret accessors)
- `PUT /api/v1/secrets/{name}`, `DELETE /api/v1/secrets/{name}`, `GET /api/v1/secrets`
- rewrite `fabro secret set`, `fabro secret list`, `fabro secret rm` as API clients
- remove `fabro secret get` (secrets are write-only)
- rewrite `fabro provider login` to save credentials via the server
- update `fabro install` to save GitHub App secrets via the local server and print restart guidance when needed
- `GET /api/v1/repos/github/{owner}/{name}` for repo access checks
- update `fabro repo init` to call repo endpoint
- `POST /api/v1/health/diagnostics` with server-side health probing
- rewrite `fabro doctor` as API client + version parity check + retained local config warning
- demo mode handlers for all new endpoints
- add shared `--server <target>` support for these server-canonical CLI commands, where `<target>` is either an HTTP(S) base URL or an absolute Unix socket path
Out of scope:
- run manifest / workflow packaging
- changes to `fabro exec` credential handling (`OPENAI_API_KEY=secret fabro exec ...` is the intended path)
- remote server auth (mTLS, JWT) — endpoints follow existing auth patterns
- encrypted-at-rest secret storage — JSON file with filesystem permissions is sufficient for now
- openssl system dependency check (being removed soon)
- broader CLI target cleanup outside the command families touched by this plan
## Key Decisions
- Secrets are **write-only**. No endpoint exposes secret values after they are stored. `GET /api/v1/secrets` returns names and timestamps only. `fabro secret get` is removed.
- Secret storage is a JSON file at `<data_dir>/secrets.json` under the active server data dir.
- The server does **not** mutate process env vars on secret writes. Server-side secret consumers read through a shared store-backed adapter so updated secrets take effect immediately for request-time flows.
- Startup-time components that only initialize once at server boot are allowed to require restart after credential changes. `fabro install` should print that restart requirement when it detects the server was already running.
- `GET /health` gains a `version` field. The CLI checks version parity before rendering diagnostics.
- `POST /api/v1/health/diagnostics` (not GET) because it triggers expensive external probes (LLM providers, GitHub, sandbox). The response reuses the shape of the existing `CheckReport` struct.
- `GET /api/v1/repos/github/{owner}/{name}` is intentionally GitHub-specific in the URL. Another segment can be added for other providers later.
- `provider login` keeps its interactive prompting and OAuth flow on the CLI side. After obtaining credentials, it saves them via `PUT /api/v1/secrets/{name}`.
- `doctor` keeps one local CLI check for user config files and legacy `.env` warning, then requires a connected server for everything else. There is no fast/offline mode.
- `dot` system dependency check moves server-side. `openssl` check is dropped. `node` check is dropped (build-time dependency only).
- The `--show-values` flag on `fabro secret list` is removed as a consequence of the write-only secret model.
- `PUT /api/v1/secrets/{name}` accepts any env-var-like key name and rejects invalid names with `400`.
- These server-canonical CLI commands use one explicit override flag, `--server <target>`, where `<target>` is either an HTTP(S) URL or an absolute Unix socket path. When omitted, they connect to the local server for the active storage dir, starting it if necessary.
- `[server].base_url` is replaced by `[server].target`, using the same string syntax as `--server`.
- `fabro install` is local-only. It never targets a remote server.
- All new endpoints have demo mode handlers.
## Implementation Changes
### 1. Add version to `GET /health`
In `docs/api-reference/fabro-api.yaml`:
- add `version` field to `HealthResponse` schema:
```yaml
HealthResponse:
description: Service health check response.
type: object
required:
- status
- version
properties:
status:
type: string
description: Health status indicator.
example: ok
version:
type: string
description: Server version string.
example: "0.176.2"
```
In `lib/crates/fabro-server/src/server.rs`:
- update the `health` handler to include the version:
```rust
async fn health() -> Response {
Json(serde_json::json!({
"status": "ok",
"version": fabro_util::version::FABRO_VERSION,
}))
.into_response()
}
```
Rebuild `fabro-api` to pick up the schema change:
```
cargo build -p fabro-api
```
Add a test in `server.rs` inline tests:
- send `GET /health`, assert status 200, assert `version` field is a non-empty string, assert `status` is `"ok"`.
### 2. Server-side secret store
Create `lib/crates/fabro-server/src/secret_store.rs`.
This module owns a JSON file and an in-memory cache:
```rust
pub struct SecretEntry {
pub value: String,
pub created_at: String, // ISO 8601
pub updated_at: String, // ISO 8601
}
pub struct SecretMetadata {
pub name: String,
pub created_at: String,
pub updated_at: String,
}
pub struct SecretStore {
path: PathBuf,
entries: HashMap<String, SecretEntry>,
}
```
Public API:
- `SecretStore::load(path: PathBuf) -> Result<Self>` — reads JSON file (or creates empty if missing), parses into `entries`.
- `store.set(name: &str, value: &str) -> Result<SecretMetadata>` — validates the key name, upserts entry with current timestamp, writes atomically (write to temp file, rename to `secrets.json`), returns metadata.
- `store.remove(name: &str) -> Result<()>` — validates the key name, removes entry (error if not found), writes atomically (write to temp file, rename).
- `store.list() -> Vec<SecretMetadata>` — returns names + timestamps, sorted by name. No values.
- `store.get(name: &str) -> Option<&str>` — reads a single secret value for server-side consumers.
- `store.snapshot() -> HashMap<String, String>` — clones the current key/value map for request-time consumers that need a full view.
- `SecretStore::validate_name(name: &str) -> Result<()>` — accept only env-var-like keys (`[A-Za-z_][A-Za-z0-9_]*`).
The JSON file format:
```json
{
"ANTHROPIC_API_KEY": {
"value": "sk-ant-...",
"created_at": "2026-04-05T10:30:00Z",
"updated_at": "2026-04-05T10:30:00Z"
}
}
```
File permissions: `0o600` on Unix (same as current `.env`).
Inline tests in `secret_store.rs`:
- `load` from empty/missing file returns empty store
- `set` creates entry, verify file written
- `set` existing key updates `updated_at`, preserves `created_at`
- `remove` deletes entry, verify file written
- `remove` missing key returns error
- `list` returns sorted metadata without values
- invalid names are rejected
- use `tempdir` for file paths in tests
In `lib/crates/fabro-server/src/server.rs`:
- add `pub secret_store: tokio::sync::RwLock<SecretStore>` to `AppState`
- update `build_app_state` to derive `secrets.json` from the active server data dir and call `SecretStore::load(path)?`
- update `create_app_state` (test helper) to use a temp path
In `lib/crates/fabro-server/src/lib.rs`:
- add `pub mod secret_store;`
Also in the server layer:
- add small store-backed adapters for the server-side secret consumers touched by this plan instead of continuing to call `std::env::var(...)` / `from_env()`
- the adapters only need to cover the flows touched by this plan:
- LLM client construction for diagnostics/model probing
- GitHub App credentials for repo checks
- GitHub client secret and session secret reads in web auth
- diagnostics secret presence/probe checks
- request-time server flows should read from the current `SecretStore` snapshot so new credentials take effect immediately
- startup-time flows may continue to require restart if they only read credentials during boot
### 3. Secret CRUD API endpoints
In `docs/api-reference/fabro-api.yaml`:
- add schemas:
```yaml
SetSecretRequest:
description: Request to store a secret value.
type: object
required:
- value
properties:
value:
type: string
description: The secret value to store.
SecretMetadata:
description: Metadata for a stored secret (value is never exposed).
type: object
required:
- name
- created_at
- updated_at
properties:
name:
type: string
description: Secret key name.
example: ANTHROPIC_API_KEY
created_at:
type: string
format: date-time
description: When the secret was first stored.
updated_at:
type: string
format: date-time
description: When the secret was last updated.
SecretListResponse:
description: List of stored secret metadata.
type: object
required:
- data
properties:
data:
type: array
items:
$ref: "#/components/schemas/SecretMetadata"
```
- add paths:
```yaml
/api/v1/secrets:
get:
operationId: listSecrets
tags: [Secrets]
summary: List stored secrets (names and timestamps only).
responses:
"200":
description: Secret metadata list.
content:
application/json:
schema:
$ref: "#/components/schemas/SecretListResponse"
/api/v1/secrets/{name}:
put:
operationId: setSecret
tags: [Secrets]
summary: Store or update a secret.
parameters:
- name: name
in: path
required: true
schema:
type: string
requestBody:
required: true
content:
application/json:
schema:
$ref: "#/components/schemas/SetSecretRequest"
responses:
"200":
description: Secret stored.
content:
application/json:
schema:
$ref: "#/components/schemas/SecretMetadata"
"400":
description: Invalid secret name or request body.
delete:
operationId: deleteSecret
tags: [Secrets]
summary: Delete a stored secret.
parameters:
- name: name
in: path
required: true
schema:
type: string
responses:
"204":
description: Secret deleted.
"400":
description: Invalid secret name.
"404":
description: Secret not found.
"500":
description: Secret store write failed.
```
Rebuild `fabro-api`:
```
cargo build -p fabro-api
```
In `lib/crates/fabro-server/src/server.rs`:
- add handlers:
```rust
async fn list_secrets(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
) -> Response {
let store = state.secret_store.read().await;
let data = store.list();
(StatusCode::OK, Json(serde_json::json!({ "data": data }))).into_response()
}
async fn set_secret(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
Path(name): Path<String>,
Json(body): Json<types::SetSecretRequest>,
) -> Response {
let mut store = state.secret_store.write().await;
match store.set(&name, &body.value) {
Ok(meta) => (StatusCode::OK, Json(meta)).into_response(),
Err(SecretStoreError::InvalidName(_)) => {
ApiError::new(StatusCode::BAD_REQUEST, "invalid secret name").into_response()
}
Err(e) => ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, e).into_response(),
}
}
async fn delete_secret(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
Path(name): Path<String>,
) -> Response {
let mut store = state.secret_store.write().await;
match store.remove(&name) {
Ok(()) => StatusCode::NO_CONTENT.into_response(),
Err(SecretStoreError::InvalidName(_)) => StatusCode::BAD_REQUEST.into_response(),
Err(SecretStoreError::NotFound(_)) => StatusCode::NOT_FOUND.into_response(),
Err(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response(),
}
}
```
- update the `axum::routing` import to include `put` and `delete`
- ensure request-body logging/redaction treats `PUT /secrets/{name}` values as sensitive and never logs the raw secret
- add routes in `real_routes()`:
```rust
.route("/secrets", get(list_secrets))
.route("/secrets/{name}", put(set_secret).delete(delete_secret))
```
- add routes in `demo_routes()` pointing to demo handlers (see §10).
Tests in `server.rs` inline tests:
- `PUT /secrets/TEST_KEY` with `{"value": "test-val"}` → 200, response has `name`, `created_at`, `updated_at`
- `GET /secrets` → 200, `data` array contains the key just set, no `value` field present
- `PUT` same key again → 200, `updated_at` changes
- `PUT /secrets/NOT-VALID` → 400
- `DELETE /secrets/TEST_KEY` → 204
- `DELETE /secrets/NONEXISTENT` → 404
- `GET /secrets` after delete → empty `data`
### 4. Rewrite `fabro secret` CLI commands as API clients
In `lib/crates/fabro-cli/src/commands/secret/mod.rs`:
- remove `SecretCommand::Get` variant
- make `execute` async (it currently dispatches to sync functions)
- each subcommand uses the shared explicit server-target helper, then the generated `fabro_api::Client`
In `lib/crates/fabro-cli/src/args.rs`:
- add `ServerTargetArgs`:
```rust
#[derive(Args, Debug, Clone, Default)]
pub(crate) struct ServerTargetArgs {
/// Fabro server target: http(s) URL or absolute Unix socket path
#[arg(long = "server", env = "FABRO_SERVER")]
pub(crate) target: Option<String>,
}
```
- flatten `ServerTargetArgs` into:
- `SecretNamespace`
- `ProviderLoginArgs`
- `DoctorArgs` (convert the inline `Doctor { ... }` variant to a named args struct)
- `RepoInitArgs` (convert the inline `RepoCommand::Init { ... }` variant to a named args struct)
- do **not** add `ServerTargetArgs` to `InstallArgs`; `install` stays local-only
In `lib/crates/fabro-cli/src/user_config.rs`:
- replace `[server].base_url` support with `[server].target`
- add a shared parser/resolver for explicit server targets used by these command families:
- `http://...` or `https://...` => remote HTTP target
- absolute path => Unix socket target
- anything else => clear parse error
- when `ServerTargetArgs` is absent, fall back to the local storage-dir/default-storage-dir server and auto-start it if necessary
- use this helper from `secret`, `provider login`, `repo init`, and `doctor`
In `lib/crates/fabro-cli/src/commands/secret/set.rs`:
- replace the body with:
```rust
pub(super) async fn set_command(
args: &SecretSetArgs,
server: &ServerTargetArgs,
globals: &GlobalArgs,
) -> Result<()> {
let client = secret_client(server).await?;
let meta = client.set_secret()
.name(&args.key)
.body(types::SetSecretRequest { value: args.value.clone() })
.send()
.await?;
if globals.json {
print_json_pretty(&meta)?;
} else {
eprintln!("Set {}", args.key);
}
Ok(())
}
```
In `lib/crates/fabro-cli/src/commands/secret/list.rs`:
- remove `--show-values` flag from `SecretListArgs`
- replace the body with:
```rust
pub(super) async fn list_command(
args: &SecretListArgs,
server: &ServerTargetArgs,
globals: &GlobalArgs,
) -> Result<()> {
let client = secret_client(server).await?;
let resp = client.list_secrets().send().await?;
if globals.json {
print_json_pretty(&resp.data)?;
} else {
for secret in &resp.data {
println!("{}\t{}", secret.name, secret.updated_at);
}
}
Ok(())
}
```
In `lib/crates/fabro-cli/src/commands/secret/rm.rs`:
- replace the body with:
```rust
pub(super) async fn rm_command(
args: &SecretRmArgs,
server: &ServerTargetArgs,
globals: &GlobalArgs,
) -> Result<()> {
let client = secret_client(server).await?;
client.delete_secret().name(&args.key).send().await?;
if globals.json {
print_json_pretty(&serde_json::json!({ "key": args.key }))?;
} else {
eprintln!("Removed {}", args.key);
}
Ok(())
}
```
Delete `lib/crates/fabro-cli/src/commands/secret/get.rs`.
In `lib/crates/fabro-cli/src/args.rs`:
- remove `SecretCommand::Get` and `SecretGetArgs`
Update any integration tests that test `fabro secret get` — remove them.
### 5. Rewrite `fabro provider login` to save via server
In `lib/crates/fabro-cli/src/commands/provider/login.rs`:
- after obtaining validated `env_pairs` (the `Vec<(String, String)>` of env var name → key value), replace the `provider_auth::write_env_file(...)` call with API calls:
```rust
let client = provider_secret_client(&args.server).await?;
for (env_var, key) in &env_pairs {
client.set_secret()
.name(env_var)
.body(types::SetSecretRequest { value: key.clone() })
.send()
.await
.with_context(|| format!("failed to save {env_var} to server"))?;
}
```
- remove the `provider_auth::write_env_file` call
- add a temporary warning if a legacy `.env` file exists under the active local storage dir:
```
Warning: ~/.fabro/.env is no longer read by fabro server. Re-enter credentials with `fabro provider login` or `fabro secret set`.
```
In `lib/crates/fabro-cli/src/shared/provider_auth.rs`:
- `write_env_file` may become dead code after this change. If no other callers exist, delete it.
- `validate_api_key` currently calls `std::env::set_var` temporarily to validate. This still works because validation happens before saving. However, consider whether the validation should instead construct the LLM client explicitly with the key rather than mutating process env. This is a follow-up concern — for now the existing validation approach is fine since the CLI process is single-threaded for this flow.
### 5a. Update `fabro install` to save GitHub App secrets via the local server
In `lib/crates/fabro-cli/src/commands/install.rs`:
- keep `install` local-only. It should always operate on the local server for the active storage dir and never accept `--server`
- after writing `server.toml` / `user.toml` and producing GitHub App secret env pairs, persist those secret values via the local server's `PUT /secrets/{name}` API instead of writing `.env`
- detect whether the local server was already running before `install`
- if the server was not running, letting the local client auto-start it is fine
- if the server was already running, print a clear restart warning after saving secrets:
```
Fabro server was already running. Restart it to pick up startup-time credential changes (for example webhook listener configuration).
```
- remove the `.env` reload
- update the final doctor invocation to the new signature / args shape
This is intentionally a hard break from `.env`, but `install` should print a temporary migration warning if it sees a legacy `.env` file in the local storage dir.
### 6. `GET /api/v1/repos/github/{owner}/{name}` endpoint
In `docs/api-reference/fabro-api.yaml`:
- add schema:
```yaml
RepoCheckResponse:
description: Repository access check result.
type: object
required:
- owner
- name
- accessible
properties:
owner:
type: string
description: GitHub repository owner.
example: acme-corp
name:
type: string
description: GitHub repository name.
example: my-app
accessible:
type: boolean
description: Whether the server has read-write access to this repository.
default_branch:
type: string
nullable: true
description: Default branch name, if accessible.
example: main
private:
type: boolean
nullable: true
description: Whether the repository is private, if accessible.
permissions:
type: object
nullable: true
description: Detected permission levels.
properties:
pull:
type: boolean
push:
type: boolean
admin:
type: boolean
install_url:
type: string
nullable: true
description: GitHub App installation URL when the repo is not yet accessible.
```
- add path:
```yaml
/api/v1/repos/github/{owner}/{name}:
get:
operationId: getGithubRepo
tags: [Repos]
summary: Check server access to a GitHub repository.
parameters:
- name: owner
in: path
required: true
schema:
type: string
- name: name
in: path
required: true
schema:
type: string
responses:
"200":
description: Repository access details.
content:
application/json:
schema:
$ref: "#/components/schemas/RepoCheckResponse"
```
Rebuild `fabro-api`:
```
cargo build -p fabro-api
```
In `lib/crates/fabro-server/Cargo.toml`:
- add `fabro-github` as a dependency if not already present
In `lib/crates/fabro-server/src/server.rs`:
- add handler:
```rust
async fn get_github_repo(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
Path((owner, name)): Path<(String, String)>,
) -> Response
```
The handler:
1. Reads non-secret GitHub App config (`app_id`, `slug`) from `Settings`. Reads `GITHUB_APP_PRIVATE_KEY` from `SecretStore`.
2. Signs a JWT via `fabro_github::sign_app_jwt`.
3. Calls `GET /repos/{owner}/{name}/installation` to check if the App is installed.
4. If installed, mints an installation token and calls `GET /repos/{owner}/{name}` to get repo details (default branch, private flag, permissions).
5. If not installed, returns `accessible: false` with null optional fields and, when possible, an `install_url`.
6. Returns `RepoCheckResponse`.
If GitHub App credentials are not configured (missing from settings or secret store), return `accessible: false` with a descriptive error or a 503.
- add route in `real_routes()`:
```rust
.route("/repos/github/{owner}/{name}", get(get_github_repo))
```
- add route in `demo_routes()` pointing to demo handler (see §10).
Tests:
- Testing the real handler requires mocking the GitHub API or the `HttpClient` trait. Use a unit test that exercises the response shape with a mock `AppState` that has a test GitHub client, or test at the integration level with the demo handler.
- At minimum, test the demo handler returns 200 with the expected shape.
### 7. Update `fabro repo init` to call repo endpoint
In `lib/crates/fabro-cli/src/commands/repo/init.rs`:
- replace `check_github_app_installation()` with a server call:
```rust
async fn check_repo_access(owner: &str, name: &str, args: &RepoInitArgs) -> Result<()> {
let client = repo_client(&args.server).await?;
let resp = client.get_github_repo()
.owner(owner)
.name(name)
.send()
.await?;
if resp.accessible {
println!(" {} GitHub repo {}/{} is accessible", green_check, owner, name);
if let Some(branch) = &resp.default_branch {
println!(" Default branch: {branch}");
}
} else {
println!(" {} GitHub repo {}/{} is not accessible", yellow_warn, owner, name);
println!(" Install the GitHub App to enable PR creation and webhook triggers.");
if let Some(url) = &resp.install_url {
println!(" Install at: {url}");
}
}
Ok(())
}
```
- The function still parses the git remote to extract `owner`/`name` — that stays CLI-side since it reads the local git config.
- Remove the direct `fabro_github::sign_app_jwt`, `fabro_github::check_app_installed`, `build_github_app_credentials` calls.
- Keep the `fabro-github` dependency in `fabro-cli/Cargo.toml` — it has many other callers (pr/*, preflight.rs, shared/github.rs).
- preserve the current interactive UX:
- when the repo is not yet accessible and stdin is a terminal, print the install URL, wait for Enter, then call the repo endpoint again
- print the second check result after the re-check
### 8. `POST /api/v1/health/diagnostics` endpoint
In `docs/api-reference/fabro-api.yaml`:
- add schemas:
```yaml
DiagnosticsReport:
description: Server health diagnostics report.
type: object
required:
- version
- sections
properties:
version:
type: string
description: Server version.
sections:
type: array
items:
$ref: "#/components/schemas/DiagnosticsSection"
DiagnosticsSection:
type: object
required:
- title
- checks
properties:
title:
type: string
checks:
type: array
items:
$ref: "#/components/schemas/DiagnosticsCheck"
DiagnosticsCheck:
type: object
required:
- name
- status
- summary
properties:
name:
type: string
status:
type: string
enum:
- pass
- warning
- error
summary:
type: string
details:
type: array
items:
$ref: "#/components/schemas/DiagnosticsDetail"
remediation:
type: string
nullable: true
DiagnosticsDetail:
type: object
required:
- text
- warn
properties:
text:
type: string
warn:
type: boolean
```
- add path:
```yaml
/api/v1/health/diagnostics:
post:
operationId: runDiagnostics
tags: [Discovery]
summary: Run server health diagnostics.
description: Probes external services (LLM providers, GitHub, sandbox) and checks server configuration. May be slow.
responses:
"200":
description: Diagnostics report.
content:
application/json:
schema:
$ref: "#/components/schemas/DiagnosticsReport"
```
Rebuild `fabro-api`:
```
cargo build -p fabro-api
```
Create `lib/crates/fabro-server/src/diagnostics.rs`.
This module contains the server-side check functions. Many can be adapted from the existing `doctor.rs` in `fabro-cli`. The key checks, grouped into sections:
**Section "Credentials":**
- `check_llm_providers` — for each provider in `Provider::ALL`, check if a secret exists in the store and probe connectivity by sending a test message.
- `check_github_app` — check that `GITHUB_APP_ID`, `GITHUB_APP_PRIVATE_KEY`, etc. exist in settings/store, validate JWT signing, and probe `GET /app` on GitHub API.
- `check_sandbox` — check `DAYTONA_API_KEY` exists. Probe Daytona API.
- `check_brave_search` — check `BRAVE_SEARCH_API_KEY` exists. Probe Brave API.
**Section "System":**
- `check_system_dep_dot` — check `dot` is in PATH and version ≥ 2.0.0.
**Section "Configuration":**
- `check_crypto` — validate mTLS certs/keys, JWT keys, session secret (same checks as current doctor).
Dropped from diagnostics (compared to current doctor):
- `check_api` / `check_web` — the CLI's ability to call the diagnostics endpoint is itself the connectivity check. No circular self-check.
- `check_system_dep_node` — node is a build-time dependency only, not needed at server runtime.
- `check_system_dep_openssl` — being removed soon.
The handler:
```rust
async fn run_diagnostics(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
) -> Response {
let report = diagnostics::run_all(&state).await;
(StatusCode::OK, Json(report)).into_response()
}
```
`diagnostics::run_all` runs all checks concurrently (where possible) and returns a `DiagnosticsReport`. The probes (LLM, GitHub, Daytona, Brave) should be run concurrently via `tokio::join!` or `futures::join!`.
- apply explicit timeouts to the live probes so `doctor` cannot hang indefinitely
- keep concurrency bounded to this fixed set of checks; do not allow unbounded fan-out
Each check function returns a `DiagnosticsCheck` struct that maps 1:1 to the API schema. The existing `CheckResult` from `fabro_util::check_report` is very close — consider either:
- reusing `CheckResult` directly and serializing it (it already has `Serialize`)
- or mapping to the generated `fabro_api` types
For the wire contract, prefer an explicit conversion step rather than assuming `CheckResult` serialization is automatically stable enough for the API surface.
Add route in `real_routes()`:
```rust
.route("/health/diagnostics", post(run_diagnostics))
```
Add route in `demo_routes()` pointing to demo handler (see §10).
Tests:
- test that `POST /health/diagnostics` returns 200 with a `version` field and a non-empty `sections` array
- test that each section has a `title` and `checks` array
- test the demo handler returns the same shape
### 9. Rewrite `fabro doctor` as API client
In `lib/crates/fabro-cli/src/commands/doctor.rs`:
- replace `run_doctor` with a thin client:
```rust
pub async fn run_doctor(args: &DoctorArgs, globals: &GlobalArgs) -> Result<()> {
let client = doctor_client(&args.server).await?;
// Local config warning block
let local_checks = render_local_config_checks()?;
// Version parity check
let health = client.get_health().send().await?;
let server_version = &health.version;
let cli_version = fabro_util::version::FABRO_VERSION;
// Run diagnostics
let report = client.run_diagnostics().send().await?;
if globals.json {
print_json_pretty(&report)?;
return Ok(());
}
// Version parity
if server_version != cli_version {
eprintln!(
"⚠ Version mismatch: CLI={cli_version} Server={server_version}"
);
}
// Render the retained local config warnings first, then the server diagnostics report.
render_local_checks(&local_checks);
render_diagnostics(&report);
Ok(())
}
```
- the rendering logic from `CheckReport::render()` can be reused. Either:
- convert the API response into a `CheckReport` and call `render()`
- or extract the rendering logic into a function that takes the diagnostics fields directly
- remove the local dry-run / offline mode flag
- keep the local user-config and legacy `.env` checks
- remove the other local check functions (`check_llm_providers`, `check_github_app`, `check_sandbox`, `check_brave_search`, `check_system_deps`, `check_api`, `check_web`, `check_crypto`)
- remove the corresponding test functions if they only tested the removed local checks
- keep the `CheckReport`/`CheckResult` rendering code in `fabro_util::check_report` — it is still useful for rendering the server's response
If the server client connection fails (server unreachable), the error message should be clear:
```
Error: could not connect to fabro server. Run `fabro server start` or check `fabro server status`.
```
Also:
- if a legacy local `.env` file exists, print a temporary warning that the server no longer reads it
- `doctor` should still succeed in rendering that local warning even when the diagnostics call later fails
### 10. Demo mode handlers
In `lib/crates/fabro-server/src/demo/mod.rs`:
**Secrets demo:**
- maintain a static in-memory `HashMap` with pre-populated fake secrets:
```rust
pub(crate) async fn list_secrets(...) -> Response {
let data = vec![
serde_json::json!({
"name": "ANTHROPIC_API_KEY",
"created_at": "2026-01-15T09:00:00Z",
"updated_at": "2026-03-20T14:30:00Z",
}),
serde_json::json!({
"name": "OPENAI_API_KEY",
"created_at": "2026-01-15T09:05:00Z",
"updated_at": "2026-02-10T11:00:00Z",
}),
serde_json::json!({
"name": "GITHUB_APP_PRIVATE_KEY",
"created_at": "2026-01-15T09:10:00Z",
"updated_at": "2026-01-15T09:10:00Z",
}),
];
(StatusCode::OK, Json(serde_json::json!({ "data": data }))).into_response()
}
pub(crate) async fn set_secret(...) -> Response {
// return fake metadata with current timestamps
}
pub(crate) async fn delete_secret(...) -> Response {
StatusCode::NO_CONTENT.into_response()
}
```
**Repo demo:**
- return a fake accessible repo:
```rust
pub(crate) async fn get_github_repo(
...,
Path((owner, name)): Path<(String, String)>,
) -> Response {
(StatusCode::OK, Json(serde_json::json!({
"owner": owner,
"name": name,
"accessible": true,
"default_branch": "main",
"private": false,
"permissions": { "pull": true, "push": true, "admin": false },
}))).into_response()
}
```
**Diagnostics demo:**
- return an all-passing report:
```rust
pub(crate) async fn run_diagnostics(...) -> Response {
(StatusCode::OK, Json(serde_json::json!({
"version": fabro_util::version::FABRO_VERSION,
"sections": [
{
"title": "Credentials",
"checks": [
{ "name": "LLM Providers", "status": "pass", "summary": "Anthropic, OpenAI configured", "details": [], "remediation": null },
{ "name": "GitHub App", "status": "pass", "summary": "JWT signing OK", "details": [], "remediation": null },
{ "name": "Sandbox", "status": "pass", "summary": "Daytona reachable", "details": [], "remediation": null },
{ "name": "Brave Search", "status": "pass", "summary": "API key configured", "details": [], "remediation": null },
]
},
{
"title": "System",
"checks": [
{ "name": "dot", "status": "pass", "summary": "dot 12.1.2", "details": [], "remediation": null },
]
},
{
"title": "Configuration",
"checks": [
{ "name": "Crypto", "status": "pass", "summary": "All keys valid", "details": [], "remediation": null },
]
},
]
}))).into_response()
}
```
Wire all demo handlers in `demo_routes()`:
```rust
.route("/secrets", get(demo::list_secrets))
.route("/secrets/{name}", put(demo::set_secret).delete(demo::delete_secret))
.route("/repos/github/{owner}/{name}", get(demo::get_github_repo))
.route("/health/diagnostics", post(demo::run_diagnostics))
```
## Implementation Order
```
1 Health version (no deps, small)
2 Secret store + adapters (no deps)
3 Secret CRUD API (depends on 2)
4 Shared server target plumbing (parallel with 2-3)
5 Secret CLI migration (depends on 3, 4)
6 Provider login migration (depends on 3, 4)
7 Install migration (depends on 3)
8 Repo check API (depends on 2)
9 Repo init migration (depends on 4, 8)
10 Diagnostics API (depends on 2)
11 Doctor CLI migration (depends on 1, 4, 10)
```
Steps 1, 2, and 4 can start in parallel. Steps 5 and 6 can run in parallel once 3 and 4 are done.
## Resolved Questions
1. **Secret store path**: `<data_dir>/secrets.json` under the active server data dir. Credentials are owned by the server instance, not by a global shared file.
2. **Migration from existing `.env`**: no auto-import. Hard break. Users must re-enter credentials via `fabro provider login` or `fabro secret set`, and the CLI/server should emit a temporary warning when they detect a legacy `.env`.
3. **GitHub App credentials for repo check**: non-secret config (`app_id`, `slug`) goes in server settings (not mixed with secrets). Secret values (`GITHUB_APP_PRIVATE_KEY`) go in the secret store. The repo check handler reads `app_id`/`slug` from `Settings` and `GITHUB_APP_PRIVATE_KEY` from `SecretStore`.
4. **Diagnostics: `check_api` and `check_web`**: dropped. The CLI's ability to call the diagnostics endpoint *is* the API connectivity check — if the server is unreachable, the CLI gets a connection error before any diagnostics run. No circular self-check needed. The one retained local CLI check is user config / legacy `.env`.
5. **`node` dependency**: dropped from diagnostics. The web app is an SPA served by the Rust server; `node` is a build-time dependency only, not needed at server runtime.
6. **`dot` dependency**: moves server-side. `openssl` dependency: dropped (being removed soon).
7. **Server targeting**: server-canonical admin commands use `--server <target>` and `[server].target`, where `<target>` is either an HTTP(S) base URL or an absolute Unix socket path.
8. **`fabro install` targeting**: `install` is local-only and writes config plus secrets for the local server host. If the server was already running, `install` prints that a restart is required for startup-time features to pick up new secrets.

View file

@ -0,0 +1,231 @@
# CLI Config, Socket, And Storage Separation
## Summary
Separate machine config, server target, and server storage so the CLI no longer conflates:
- config path: `~/.fabro/settings.toml`
- default socket target: `~/.fabro/fabro.sock`
- default storage dir: `~/.fabro/storage`
Normal user-facing commands should always talk to a server target. A Unix socket target may auto-start the daemon. An HTTP target may not. Serverless/direct-storage command behavior should be removed from normal commands in this pass.
## Scope Boundaries
In scope:
- add `FABRO_CONFIG` support for machine settings loading
- default the server target to `~/.fabro/fabro.sock`
- default server storage to `~/.fabro/storage`
- decouple socket-path resolution from storage-dir resolution
- keep daemon auto-start only for Unix socket targets
- remove normal-command fallback to direct storage-based targeting
- keep `fabro server *`, hidden `fabro __runner`, and `fabro install` as storage-owning commands
- keep user-facing `--server` on server-targeted commands
- remove user-facing `--storage-dir` from server-targeted commands
Out of scope:
- `fabro exec`
- `fabro system df`
- `fabro system prune`
- `fabro store dump`
- new remote maintenance endpoints for deferred commands
## Problem Frame
The current CLI still mixes together two different ideas:
- a local daemon reached over a Unix socket
- serverless behavior where the CLI uses `storage_dir` as the command target
That has produced the wrong defaults and the wrong abstractions:
- the socket path is currently derived from `storage_dir`
- many commands still model targeting as "server or storage dir"
- normal commands can still fall back to a storage-driven local connection shape
- autostart is keyed off the local/storage connection path instead of the actual server target type
The intended model is simpler:
- normal commands always resolve a server target
- the default server target is a Unix socket in `~/.fabro`
- Unix socket targets may auto-start a daemon
- HTTP targets may not auto-start a daemon
- storage is server-owned runtime state, not the primary targeting mechanism for normal commands
## Key Decisions
- `FABRO_CONFIG` selects the active machine settings file.
- For server lifecycle commands and daemon auto-start, precedence is: explicit `--config` where supported, then `FABRO_CONFIG`, then `~/.fabro/settings.toml`.
- Normal user-facing commands do not gain a new `--config` flag in this pass; they resolve settings from `FABRO_CONFIG` or the default path.
- `FABRO_SERVER` selects the effective server target for normal commands.
- It accepts either an absolute Unix socket path or an `http(s)` URL.
- If unset, use `settings.server.target`.
- If that is unset, default to `~/.fabro/fabro.sock`.
- Server-targeted commands keep `--server` as the standard one-off override.
- `FABRO_SERVER` remains the env-var equivalent.
- `FABRO_STORAGE_DIR` is no longer a normal command-targeting mechanism.
- It remains an override for storage-owning commands only.
- Precedence for storage-owning commands: explicit `--storage-dir` where still supported, then `FABRO_STORAGE_DIR`, then `settings.storage_dir`, then `~/.fabro/storage`.
- `settings.server.target` remains the durable place to configure the machine's server target.
- `settings.storage_dir` remains the durable place to configure where the local server stores data.
- `fabro server start` keeps `--config` and `--bind`.
- Its default bind is `~/.fabro/fabro.sock`, not `<storage_dir>/fabro.sock`.
- `fabro server stop` and `fabro server status` remain storage-owning commands and continue to resolve the local server instance from storage-owned records.
- `fabro settings` is a local config-inspection command, not a server-targeted command.
- It keeps its current local settings-resolution behavior in this pass.
- `fabro exec` is unchanged in this pass.
## Command Classification
### Storage-owning commands
These commands continue to resolve and use local storage directly:
- `fabro server start`
- `fabro server stop`
- `fabro server status`
- hidden `fabro server __serve`
- hidden `fabro run __runner`
- `fabro install`
### Local config-inspection commands
These commands stay outside the server-targeting cleanup in this pass:
- `fabro settings`
### Server-targeted commands
These commands should resolve a `ServerTarget` only and should not use storage-dir fallback semantics:
- `fabro run`
- `fabro create`
- `fabro preflight`
- `fabro validate`
- `fabro graph`
- `fabro model list`
- `fabro model test`
- `fabro doctor`
- `fabro repo init`
- `fabro provider login`
- `fabro secret list`
- `fabro secret rm`
- `fabro secret set`
- `fabro ps`
- `fabro rm`
- `fabro inspect`
- `fabro run start`
- `fabro run attach`
- `fabro run logs`
- `fabro run resume`
- `fabro run rewind`
- `fabro run fork`
- `fabro run wait`
- hidden `fabro run diff`
- `fabro artifact list`
- `fabro artifact cp`
- `fabro sandbox cp`
- `fabro sandbox preview`
- `fabro sandbox ssh`
- `fabro pr create`
- `fabro pr list`
- `fabro pr view`
- `fabro pr merge`
- `fabro pr close`
### Deferred local-maintenance commands
These remain unchanged in this pass:
- `fabro system df`
- `fabro system prune`
- `fabro store dump`
These deferred commands continue to use `StorageDirArgs` and the existing hybrid `ServerRunLookup::connect(storage_dir)` path in this pass, and should keep working against the new default storage dir without being reclassified as server-targeted commands yet.
## Implementation Changes
### 1. Shared settings and path resolution
- Add a shared helper for the active settings path used by CLI and server code.
- This helper must honor `FABRO_CONFIG`.
- Add a shared helper for the default socket path: `~/.fabro/fabro.sock`.
- Change the default storage dir helper to return `~/.fabro/storage`.
- Stop deriving the socket path from `storage_dir`.
### 2. Target resolution model
- Refactor CLI target resolution so normal commands resolve a `ServerTarget`, not a "local vs target" union.
- Remove `ServerConnection::Local` as a normal command-routing concept.
- Split helpers into two categories:
- server-target resolution for normal commands
- storage-dir resolution for storage-owning commands
- Keep TLS handling attached to `HttpUrl` targets exactly as today.
### 3. Connection and auto-start behavior
- Update the server client helpers so they accept or derive a `ServerTarget`.
- If the resolved target is `UnixSocket(path)`:
- attempt to connect to that socket
- if unavailable, auto-start the daemon
- auto-start the daemon bound to that exact socket path
- If the resolved target is `HttpUrl(url)`:
- attempt to connect once
- if unavailable, fail with a clean reachability error
- do not auto-start anything
- Auto-start must pass the active config path through to the spawned server with `--config`.
- The auto-start helper should take the resolved active config path, resolved Unix socket path, and resolved local storage dir as explicit inputs.
- It should not rediscover config via environment variables or reload settings internally during daemon launch.
- Auto-start may also pass the resolved storage dir for the local server process, but only as runtime/server lifecycle plumbing, not as the command target abstraction.
- Any active-server record lookup used by `server stop`, `server status`, `install`, or daemon auto-start should temporarily fall back to the legacy implicit storage root `~/.fabro` when no explicit storage location is provided and no record exists under the new default `~/.fabro/storage`.
- This is only to find already-running daemons started before the default-storage change.
### 4. CLI arg surface cleanup
- Keep user-facing `--server` on all server-targeted commands listed above.
- Remove user-facing `--storage-dir` from all server-targeted commands listed above.
- Remove the `storage_dir_explicit` conflict plumbing for those commands.
- Remove the custom `--server` / `--storage-dir` conflict detection in `main.rs` once no supported command still accepts both flags together.
- Keep `--storage-dir` only on storage-owning commands in this pass.
- Keep `--config` only where already appropriate for server lifecycle.
- Update help text and parser tests so normal commands still advertise `--server` but no longer imply that storage-dir is a general targeting control.
### 5. Run/create local run-dir handling
- `run` / `create` currently thread a synthesized local run dir through the result object to print asset paths after completion.
- Preserve that behavior only when the effective target is the machine's local Unix socket and the effective local storage dir is known.
- For HTTP targets, do not synthesize a local run dir.
- This should be derived from "effective target is local socket" plus resolved storage dir, not from a `ServerConnection::Local` enum variant.
### 6. Docs and user-visible messaging
- Rewrite docs and examples so:
- normal commands use config-driven target resolution by default
- temporary overrides use `FABRO_SERVER=... fabro ...`
- config-file overrides use `FABRO_CONFIG=... fabro ...`
- Update language to avoid "local mode" as a user-facing concept.
- Use "Unix socket target" or "HTTP target".
- Use "serverless" only for the behavior being removed.
- Make `doctor` and `settings` print:
- active config path
- effective server target
- effective storage dir
- any active env-var overrides
## Test Plan
- Unit tests for path and precedence helpers:
- active config path honors `FABRO_CONFIG`
- default socket path is `~/.fabro/fabro.sock`
- default storage dir is `~/.fabro/storage`
- `FABRO_SERVER` overrides `settings.server.target`
- `FABRO_STORAGE_DIR` overrides `settings.storage_dir` for storage-owning commands only
- `FABRO_CONFIG=/custom/path/settings.toml` loads that file's contents and the resolved `server.target` / `storage_dir` from that file actually take effect
- Unit tests for target resolution:
- normal commands default to a Unix socket target when nothing is configured
- normal commands no longer resolve a storage-dir fallback connection
- HTTP targets never route into autostart code
- Integration tests for daemon behavior:
- `fabro server start` defaults to binding `~/.fabro/fabro.sock`
- auto-start for socket-targeted commands binds the requested socket, not `<storage_dir>/fabro.sock`
- auto-start passes the active config path through to the daemon
- HTTP-targeted commands fail cleanly when unreachable
- `server stop` / `server status` still find an already-running daemon whose record lives under the legacy implicit storage root
- Parser/help tests:
- server-targeted commands still accept `--server`
- server-targeted commands no longer accept `--storage-dir`
- storage-owning commands still accept `--storage-dir` where intended
- `fabro settings` keeps its existing local settings override surface in this pass
- Workflow behavior tests:
- `run`, `create`, `model`, `doctor`, `ps`, and `secret` work via the default socket target
- local Unix socket runs still print local asset/run-dir info when appropriate
- HTTP-targeted runs do not print synthesized local run-dir paths
## Assumptions
- This is a hard cut for CLI targeting semantics. No deprecation period for removed normal-command flags.
- `FABRO_SERVER` is the single override for user-facing command targeting; no separate `FABRO_SOCKET` env var is added.
- `server.target` remains the canonical durable target field in `settings.toml`.
- Deferred commands will be handled in a later pass rather than forced into this refactor.

View file

@ -0,0 +1,143 @@
# Settings Command Server/Local Merge Plan
## Summary
Refactor `fabro settings` so it answers “what settings will actually be used?” instead of only dumping locally merged config.
The command contract becomes:
- `fabro settings`
- show the effective merged settings for the selected server target plus local config
- `fabro settings --local`
- show only locally resolved settings, with no server call
- `fabro settings WORKFLOW`
- show the effective merged settings for that workflow after server defaults are applied
- `fabro settings --local WORKFLOW`
- show the local-only merged settings for that workflow, with no server call
This plan is intentionally separate from the broader config/socket/storage cleanup. It can land independently as long as the command reuses the current shared target-resolution behavior that exists at implementation time.
## Problem Frame
The current `fabro settings` command is purely local. It resolves project/workflow config plus local machine settings and prints the result in YAML or JSON.
That is no longer the most useful answer for users, because workflows run on a server and the final settings are not purely local. The server already applies its own defaults and runtime adjustments during manifest preparation, so the current output is only a partial picture.
The goal of this pass is to make `fabro settings` answer two distinct questions clearly:
- what would the CLI resolve locally without talking to a server?
- what settings will actually be used when this workflow runs on the selected server?
## Key Decisions
- `fabro settings` becomes server-targeted by default.
- It should fetch server settings and merge them with local config using the same merge logic the server uses for real runs.
- `fabro settings --local` skips the server entirely and preserves the current local inspection behavior.
- `fabro settings` keeps the optional positional `WORKFLOW`.
- With no workflow argument, it shows the effective baseline settings for the current repo/config context.
- With a workflow argument, it shows the effective run settings for that workflow.
- `fabro settings` should support `--server` as the normal one-off server-target override.
- `fabro settings --local` conflicts with `--server`.
- `fabro settings` should no longer take `--storage-dir`.
- Storage targeting is not the purpose of this command.
- Output format stays the same:
- default: YAML `Settings`
- `--json`: JSON `Settings`
- The command should return only the resolved `Settings` object, not extra metadata fields.
- Effective target/config-path diagnostics belong in `doctor` or other output, not in the `settings` payload itself.
- Bare `fabro settings` should use the same server-target resolution behavior as other server-targeted commands at the time this plan lands.
- If the resolved target is a Unix socket, it may use the normal socket/autostart path.
- If the resolved target is an HTTP target and the server is unreachable, the command should fail clearly.
- It should not silently fall back to `--local`.
- The `/api/v1/settings` endpoint should return the full effective runtime `Settings` object from the server.
- The shared merge helper remains responsible for precedence and for preserving the existing distinction between full server defaults and local-daemon-only overrides.
## Implementation Changes
### 1. CLI surface
- Change `SettingsArgs` in [`lib/crates/fabro-cli/src/args.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-cli/src/args.rs) to:
- add `ServerTargetArgs`
- add `--local`
- keep optional `WORKFLOW`
- remove `StorageDirArgs`
- Update help text and parser tests in [`lib/crates/fabro-cli/tests/it/cmd/config.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-cli/tests/it/cmd/config.rs) accordingly.
### 2. Shared merge logic
- Extract the server/default merge logic currently embedded in [`lib/crates/fabro-server/src/run_manifest.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-server/src/run_manifest.rs) into a shared helper in `fabro-config` that both the server and CLI can call.
- The helper should live in `fabro-config` because both `fabro-cli` and `fabro-server` already depend on it, while `fabro-cli` should not depend on `fabro-server` merge internals.
- The shared helper should accept:
- the local config layers already resolved by the CLI side
- server settings fetched from the target server
- a mode flag matching current manifest preparation semantics
- The helper must preserve the current distinction between:
- normal remote/server merge behavior
- local-daemon merge behavior
- The helper must make the precedence explicit:
- `fabro settings`: `project + user + server_defaults`
- `fabro settings WORKFLOW`: `workflow + project + user + server_defaults`
- `fabro settings --local`: `project + user`
- `fabro settings --local WORKFLOW`: `workflow + project + user`
- The helper should continue to apply the same server-side rules that exist today:
- normal mode uses the full `server_defaults_layer()`
- local-daemon mode uses `local_daemon_server_overrides_layer()`
- any required post-resolution overrides, such as forcing `storage_dir` from the active server settings, remain in the shared helper rather than being reimplemented by the CLI
- The server should be switched to use the shared helper so `fabro settings` cannot drift from actual run semantics.
### 3. Server settings source
- Implement the real `/api/v1/settings` route in [`lib/crates/fabro-server/src/server.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-server/src/server.rs), matching the existing OpenAPI contract in [`docs/api-reference/fabro-api.yaml`](/Users/bhelmkamp/p/fabro-sh/fabro/docs/api-reference/fabro-api.yaml).
- The route should return the servers current effective runtime settings from `AppState`, not just a raw disk parse.
- The route should return the full runtime `Settings` object, not a reduced server-owned subset.
- The CLI settings command should call this route when `--local` is not set.
### 4. Command behavior
- In [`lib/crates/fabro-cli/src/commands/config/mod.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-cli/src/commands/config/mod.rs):
- preserve the existing local merge path for `--local`
- add a server-targeted path for the default behavior
- Local-only resolution should keep the current semantics:
- no workflow: project config from cwd + local settings
- workflow: workflow config + project config + local settings
- Server-targeted resolution should:
- resolve the server target using the normal shared target-resolution helpers
- fetch server settings from `/api/v1/settings`
- build the same local layers the command already knows how to build
- combine them with fetched server settings using the shared merge helper
- If the server is unreachable:
- Unix socket targets should follow the same socket/autostart behavior normal server-targeted commands use at the time this plan lands
- HTTP targets should fail clearly
- the command should not fall back to local-only mode unless the user explicitly asked for `--local`
- For `WORKFLOW`, the command should not call `preflight` or create a run.
- It should compute and print the resolved settings only.
### 5. Separation from broader targeting cleanup
- This plan should not block on the larger config/socket/storage refactor.
- If the broader refactor lands first, `fabro settings` should reuse the new target-resolution helpers.
- If it lands first, `fabro settings` should reuse the current `--server` / configured server-target behavior and keep its implementation scoped to this command.
- This plan does not change `exec`, `system df`, `system prune`, or `store dump`.
## Test Plan
- CLI parser/help coverage in [`lib/crates/fabro-cli/tests/it/cmd/config.rs`](/Users/bhelmkamp/p/fabro-sh/fabro/lib/crates/fabro-cli/tests/it/cmd/config.rs):
- `fabro settings --help` shows `--local` and `--server`
- `fabro settings --local --server ...` is rejected
- `--storage-dir` is no longer accepted
- Local behavior tests:
- `fabro settings --local` preserves current merged local output
- `fabro settings --local WORKFLOW` resolves workflow + project + local settings only
- Server-targeted behavior tests:
- `fabro settings` fetches server settings and merges them with local config
- `fabro settings WORKFLOW` merges workflow + project + local + server settings
- `fabro settings --server http://...` uses the explicit target
- socket-targeted settings resolution behaves the same way normal commands do at the time this plan lands
- unreachable HTTP targets fail clearly and do not fall back to local-only output
- Shared merge helper tests:
- `prepare_manifest_with_mode()` delegates to the shared helper for the merge/defaults path
- helper tests directly cover the layer precedence for:
- `project + user`
- `workflow + project + user`
- `project + user + server_defaults`
- `workflow + project + user + server_defaults`
- local-daemon merge semantics remain distinct from remote-server merge semantics where that distinction already exists
- Server route tests:
- real `/api/v1/settings` returns the structured settings shape from the OpenAPI contract
- route reflects effective runtime settings, including active storage-dir/runtime overrides
## Assumptions
- Returning the raw `Settings` payload is sufficient; no new wrapper response is needed for the CLI command.
- Reusing the existing `/api/v1/settings` contract is preferable to adding a second settings-resolution endpoint in this pass.
- `fabro settings WORKFLOW` may perform local workflow/project discovery exactly as the command does today; the only new remote input is the selected servers settings.
- This pass is command-focused and does not attempt to solve all settings introspection use cases elsewhere in the API.