fabro/docs/plans/2026-04-23-001-refactor-command-context-alignment-plan.md
Bryan Helmkamp bebf472ad2
chore: move testing-strategy doc into docs-internal
The strategy doc lived alone under files-internal/ while every sibling
strategy doc (logging, events, server-secrets) lived under docs-internal/.
Move it next to the others and update plan/spec references.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-25 12:38:41 -04:00

40 KiB
Raw Permalink Blame History

title type status date deepened
refactor: CommandContext alignment at CLI command boundaries refactor completed 2026-04-23 2026-04-23

CommandContext Alignment At CLI Command Boundaries

Overview

Finish the partially landed CommandContext refactor in fabro-cli by making CommandContext the shared command-boundary abstraction for invocation plumbing in the CLI command families that already depend on it or immediately reconstruct it. The goal is to stop threading &CliNamespace, &CliLayer, Printer, and process_local_json through command entrypoints when that data is already part of the same invocation context.

This is a follow-on plan to the earlier server-access refactor. The current codebase already centralizes cwd, merged settings, and server access in CommandContext, but command entrypoints still receive raw plumbing and then rebuild a context locally. This plan aligns the surface area around the existing struct instead of introducing a second context or service wrapper.

Problem Frame

CommandContext exists today in lib/crates/fabro-cli/src/command_context.rs, and many commands already depend on it for cwd, machine_settings, user_settings, and server() access. The refactor is only half-finished:

  • the printer field and printer() accessor are both marked #[allow(dead_code, reason = "...still being wired through")]
  • main.rs still passes raw plumbing into most command families
  • many command entrypoints still accept some combination of &CliNamespace, &CliLayer, Printer, and process_local_json
  • those same commands often construct CommandContext immediately inside the handler

That leaves the CLI with two overlapping models:

  • CommandContext as the intended abstraction for shared invocation state
  • raw parameter threading as the de facto command API

The result is more churn on command signatures, more repeated output format and JSON-gating branches, and a dead-code-marked printer field that signals the abstraction boundary is unfinished.

The refactor needs to finish in a way that preserves current CLI behavior and avoids turning CommandContext into a new god object.

Requirements Trace

  • R1. In-scope CLI command entrypoints use CommandContext as the shared invocation-plumbing abstraction instead of separately receiving &CliNamespace, &CliLayer, Printer, and process_local_json.
  • R2. CommandContext remains narrow: it carries invocation-scoped plumbing and shared derived state, not command-specific args, styles, or workflow-specific data.
  • R3. JSON output, verbosity behavior, printer routing, and global --json restrictions remain behaviorally unchanged for existing commands, including auth status remaining explicit-global---json only rather than switching to persisted cli.output.format = "json".
  • R4. Target-based and storage-dir-based server resolution semantics remain unchanged, including CommandContext::server() behavior and ServerSummaryLookup::from_client(...) usage.
  • R5. main.rs keeps its current pre-tracing bootstrap ordering for logging and upgrade checks; CommandContext begins after that phase.
  • R6. The current dead-code allowance on the printer field/accessor is removed by making printer access a real part of the abstraction, or by deleting redundant surface area if a narrower accessor is better.
  • R7. Representative unit and integration coverage exists for the context API, JSON/text output behavior, global --json gating, and target/connection-based command families touched by the refactor.

Scope Boundaries

  • In scope: command families in fabro-cli that already use CommandContext directly or immediately construct one from raw plumbing: run, runs, artifact, store dump, preflight, validate, graph, model, secret, pr, repo, auth, provider, config, version, doctor, system, and the public sandbox preview / sandbox ssh command boundary that currently forwards into commands/run/* leaf modules.
  • In scope: top-level dispatch cleanup in lib/crates/fabro-cli/src/main.rs and family dispatch modules where the only reason raw CLI plumbing is still passed down is command signature inertia.
  • Out of scope: exec, server, install, upgrade, workflow, parse, render_graph, hidden analytics/panic upload commands, hidden run worker, and sandbox cp, except for thin compile-only adapters if needed.
  • Out of scope: changing CLI text, response payloads, exit-code semantics, or server connection behavior beyond what is required to move the plumbing boundary.
  • Out of scope: moving Styles ownership into CommandContext or making low-level pure rendering helpers context-aware when explicit Printer or Styles parameters remain clearer.
  • Out of scope: redesigning server_client.rs connection semantics. This plan consumes the current CommandContext::server() model rather than reopening the earlier server-access design.

Context & Research

Relevant Code and Patterns

  • lib/crates/fabro-cli/src/command_context.rs — current CommandContext, constructors, user_settings() / machine_settings() accessors, and dead-code-marked printer storage.
  • lib/crates/fabro-cli/src/main.rs — top-level command dispatch still passing raw plumbing into most families after bootstrap.
  • lib/crates/fabro-cli/src/server_client.rs — current target and storage-dir connection behavior that must stay unchanged.
  • lib/crates/fabro-cli/src/commands/run/create.rs — existing example of a helper that already takes &CommandContext.
  • lib/crates/fabro-cli/src/commands/run/mod.rs — current mixed model: some subcommands build a CommandContext, some still read raw cli.output.format, and process_local_json is already vestigial at this boundary.
  • lib/crates/fabro-cli/src/commands/secret/mod.rs — good family-level pattern where a single derived server/client can be shared across subcommands.
  • lib/crates/fabro-cli/src/commands/pr/mod.rs — representative mixed local/server family using both CommandContext::base(...) and CommandContext::for_target(...).
  • lib/crates/fabro-cli/src/commands/auth/mod.rs and lib/crates/fabro-cli/src/commands/provider/mod.rs — the clearest examples of raw process_local_json still being threaded despite the rest of the state already belonging to the invocation.
  • docs-internal/testing-strategy.md — CLI integration tests should stay command-driven and black-box, with implementation-facing behavior covered by unit tests near the code.
  • lib/crates/fabro-cli/src/commands/sandbox/mod.rs — the real public boundary for preview/ssh command dispatch; leaf implementation lives in commands/run/preview.rs and commands/run/ssh.rs, but the command family boundary is sandbox.
  • lib/crates/fabro-cli/tests/it/cmd/sandbox_preview.rs and lib/crates/fabro-cli/tests/it/cmd/sandbox_ssh.rs — existing command-owned coverage for those public entrypoints.

Current-State Inventory

  • CommandContext::{base, for_target, for_connection} currently has 42 call sites across 37 command files under lib/crates/fabro-cli/src/commands.
  • The command tree contains one existing helper that already accepts &CommandContext directly: lib/crates/fabro-cli/src/commands/run/create.rs.
  • The printer() accessor currently has no call sites, which is why the dead-code allowance still exists.
  • For in-scope command files, the dominant remaining use of raw &CliNamespace is reading cli.output.format or cli.output.verbosity; that is a signal that the data belongs on the shared invocation context rather than each command signature.
  • process_local_json is now concentrated in auth, provider, graph, sandbox preview, and sandbox ssh. That makes it a good candidate for a dedicated invocation-context field/helper instead of continued parameter threading.
  • docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md — earlier plan that introduced the current CommandContext and server access model. This plan finishes the command-boundary alignment that document did not fully land.
  • docs/plans/2026-04-22-001-refactor-settings-api-entrypoints-plan.md — recent owner-first context plan that reinforces the repo preference for dense, owner-scoped context objects instead of repeated free-form plumbing.
  • git log -- lib/crates/fabro-cli/src/command_context.rs shows recent follow-on commits including simplify: drop duplicate settings plumbing from cli/server refactor, which is consistent with the current goal of collapsing overlapping command-boundary APIs.

Institutional Learnings

  • No relevant docs/solutions/ entries currently cover this seam.

External References

  • No external research used. The repo already has sufficient local context, existing partial implementation, and tests for this refactor.

Key Technical Decisions

  • Use CommandContext as the command-boundary API for in-scope commands. The abstraction already owns the hard parts: working directory, merged settings, and server access. The remaining refactor should move entrypoint APIs onto that abstraction instead of continuing to thread raw plumbing beside it.

  • Keep CommandContext narrow and invocation-scoped, not god-shaped. It should own: printer, merged CLI-derived settings (machine_settings, user_settings), cwd, config-path context, server-derivation state, and the invocation-only global --json flag. It should not own command args, Styles, render-only helpers, or workflow/build-specific state.

  • Do not store or expose the full CliNamespace publicly. Commands in scope only need a small subset of CLI plumbing: output format, output verbosity, and global --json restrictions. Output format and verbosity should come from ctx.user_settings().cli.output, which already reflects CLI override precedence through merged settings. The only truly extra invocation field is the global --json switch, which is not part of persisted settings and therefore belongs on the context explicitly.

  • Preserve the current error-timing split. CommandContext construction should keep doing what it does today: capture cwd, load local settings, and build merged invocation state. Server-target resolution and server-connection failures should remain deferred to ctx.server().await? or existing explicit resolve_server_target(...) calls. The refactor should not make malformed target/server resolution errors eager at context-construction time.

  • Keep auth status as an explicit-global-JSON command. Most in-scope commands should read output mode from ctx.user_settings().cli.output, but auth status is a special case: it currently emits JSON only for the explicit invocation-wide global --json path, not merely because resolved CLI settings say output.format = json. That distinction should remain intact, so the context needs both resolved output settings and a separate helper for the explicit global JSON flag.

  • Create a base invocation context once, then derive target/connection variants from it. main.rs should keep using raw settings during pre-tracing bootstrap. After that, it should build a base CommandContext once for each in-scope dispatch path. Command families then derive target-based or connection-based contexts from that base without re-supplying Printer and CliLayer.

  • Allow private derivation state inside CommandContext if that is the simplest way to avoid raw parameter threading. Storing a private CliLayer or equivalent internal builder state is acceptable if it enables methods like with_target(...) or with_connection(...) and keeps the raw plumbing hidden behind the abstraction boundary.

  • Keep render helpers explicit. Command entrypoints and family dispatchers should align on CommandContext, but low-level helpers such as table rendering, summary formatting, and browser-opening routines may keep explicit Printer / Styles / boolean parameters where that stays simpler than threading the full context downward.

  • Keep workflow/manifest layer assembly command-local. Commands such as run, preflight, validate, and graph should continue to build their workflow/project/manifests with the existing command-owned helpers. CommandContext can provide cwd, merged user settings for output behavior, and server access, but it should not absorb manifest-building policy or workflow-layer composition.

  • Migrate family-by-family with temporary compatibility wrappers if needed. This is a cross-cutting refactor with wide signature churn. It is better to allow short-lived constructor/adapter overlap during the migration than to force a single giant all-or-nothing patch that is harder to validate.

Open Questions

Resolved During Planning

  • Should this refactor introduce a second wrapper type such as Services or InvocationContext? No. The repo already has a partially landed CommandContext, and the simplest aligned design is to finish that abstraction rather than splitting responsibilities across two overlapping context types.

  • Should CommandContext absorb the entire CliNamespace? No. Public command consumers should read output mode and verbosity from ctx.user_settings().cli.output, while command-specific configuration stays local to the commands that own it.

  • Should process_local_json become part of CommandContext? Yes. It is invocation-scoped, currently leaks across multiple command signatures, and is the one remaining piece of global command plumbing that is not already represented by merged settings.

  • Should preview/ssh be treated as run work or sandbox work in this plan? Treat them as sandbox work at the public command boundary. The leaf implementation modules remain under commands/run/*, but the raw plumbing boundary in the current CLI is commands/sandbox/mod.rs and the plan should align to that boundary and its tests.

  • Should auth status remain “explicit global JSON only”? Yes. R3 for this plan is behavioral preservation, and the current contract is that auth status emits JSON only when the explicit invocation-global --json switch is active.

  • Should Styles move into CommandContext in the same pass? No. That would enlarge the abstraction without addressing the actual duplicated plumbing problem.

  • Should out-of-scope local commands be forced onto CommandContext just for uniformity? No. This pass should target the families where CommandContext already provides real value or is already partially adopted.

Deferred to Implementation

  • Exact API names for derived contexts. The implementation may settle on with_target(...), for_target_from(...), or similar naming. The important contract is that callers no longer pass raw Printer and CliLayer repeatedly.

  • Whether static constructors remain temporarily during migration. If temporary wrappers reduce compile churn while family-by-family patches land, they are acceptable. Final cleanup should remove the now-redundant raw-plumbing entrypoints from in-scope call sites.

  • Whether CommandContext should be cheaply cloneable or should build derived variants from private state on demand. Either is acceptable if it preserves the abstraction boundary and does not change runtime behavior.

High-Level Technical Design

This illustrates the intended approach and is directional guidance for review, not implementation specification. The implementing agent should treat it as context, not code to reproduce.

Boundary Current shape Target shape
main.rs -> command family dispatch(args, &cli_settings, &cli_layer, process_local_json, printer) dispatch(args, &base_ctx)
family dispatch -> leaf command raw CLI plumbing plus CommandContext::for_target(...) inside the leaf derive target_ctx / connection_ctx once from base_ctx, then pass &CommandContext or already-resolved client/output helpers
output mode lookup cli.output.format / cli.output.verbosity ctx.user_settings().cli.output.*
global --json guard separate process_local_json parameter ctx accessor/helper
printing raw printer parameter ctx.printer() or a narrower printer extracted from ctx

Directional flow:

bootstrap raw globals/settings for tracing + upgrade check
  -> build base CommandContext once for the in-scope command dispatch
  -> family dispatch receives &base_ctx
  -> family derives:
       target_ctx from target args
       connection_ctx from storage-dir-aware args
       base/local ctx for settings-only commands
  -> leaf command reads:
       ctx.cwd()
       ctx.machine_settings()
       ctx.user_settings().cli.output.*
       ctx.require_no_json_override() or equivalent
       ctx.printer()
       ctx.server().await?
  -> render-only helpers stay explicit over Printer / Styles where simpler

Implementation Units

  • Unit 1: Reframe CommandContext as the invocation-boundary object

Goal: Make CommandContext capable of carrying the invocation plumbing that is still leaking through command signatures, while keeping the type narrowly scoped.

Requirements: R1, R2, R3, R4, R6

Dependencies: None

Files:

  • Modify: lib/crates/fabro-cli/src/command_context.rs
  • Modify: lib/crates/fabro-cli/src/main.rs
  • Test: lib/crates/fabro-cli/src/command_context.rs

Approach:

  • Add the remaining invocation-only plumbing that does not already exist on the context, specifically the global --json switch used today via process_local_json.
  • Make printer access part of the live API so the dead-code allowance on the printer field/accessor can be removed.
  • Add context-derivation methods that let callers obtain target-based or connection-based variants from a base invocation context without re-supplying raw Printer and CliLayer.
  • Keep output format and verbosity sourced from ctx.user_settings().cli.output rather than storing a second public output-format copy on the side, while still exposing the explicit invocation-global JSON flag separately for commands like auth status whose contract is not identical to resolved output format.

Patterns to follow:

  • lib/crates/fabro-cli/src/command_context.rs
  • docs/plans/2026-04-22-001-refactor-settings-api-entrypoints-plan.md for owner-first context boundaries

Test scenarios:

  • Happy path: a base context exposes the same output format and verbosity that commands currently read from merged CLI settings.
  • Happy path: a base context preserves both resolved output settings and the explicit invocation-global JSON flag so commands can distinguish between “resolved output format is JSON” and “user passed global --json”.
  • Happy path: deriving a target-based context preserves printer/global JSON state and resolves server access through the existing server_client::connect_server_with_settings(...) path.
  • Edge case: deriving a connection-based context with a storage-dir override changes only the storage-backed settings path and preserves other invocation-scoped data.
  • Error path: settings-load failures tied to context construction still surface during context construction, while malformed target resolution remains deferred until ctx.server().await? or explicit resolve_server_target(...) calls.

Verification:

  • The printer field/accessor is no longer dead code.

  • There is one clear way to obtain a base context and derive target/connection variants without raw plumbing at the call site.

  • The context API makes the distinction between resolved output format and explicit global JSON invocation state unambiguous.

  • Unit 2: Move main.rs and the run/preflight/graph/sandbox boundary to context-first dispatch

Goal: Eliminate raw plumbing from the top-level dispatch path and the run-oriented command family plus the public sandbox preview / sandbox ssh boundary that already rely heavily on CommandContext.

Requirements: R1, R3, R4, R5, R6, R7

Dependencies: Unit 1

Files:

  • Modify: lib/crates/fabro-cli/src/main.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/command.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/create.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/preview.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/ssh.rs
  • Modify: lib/crates/fabro-cli/src/commands/sandbox/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/resume.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/rewind.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/fork.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/wait.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/diff.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/logs.rs
  • Modify: lib/crates/fabro-cli/src/commands/run/cp.rs
  • Modify: lib/crates/fabro-cli/src/commands/preflight.rs
  • Modify: lib/crates/fabro-cli/src/commands/validate.rs
  • Modify: lib/crates/fabro-cli/src/commands/graph.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/run.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/create.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/start.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/attach.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/diff.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/logs.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/preflight.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/validate.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/graph.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/sandbox_preview.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/sandbox_ssh.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/json_global.rs
  • Test: lib/crates/fabro-cli/tests/it/scenario/lifecycle.rs
  • Test: lib/crates/fabro-cli/tests/it/scenario/recovery.rs

Approach:

  • Build a base CommandContext in main.rs only after the existing bootstrap phase completes.
  • Change the run-family and run-adjacent entrypoints to accept context instead of raw cli / cli_layer / printer bundles.
  • Replace direct reads of cli.output.format and cli.output.verbosity with ctx.user_settings().cli.output.*.
  • Replace direct process_local_json threading with a context helper for the small number of commands that still need it (graph, commands/sandbox/mod.rs for the public preview/ssh boundary).
  • Move the public sandbox preview / sandbox ssh dispatch boundary to the same context-first shape as the run-family boundary, while leaving the leaf implementation modules in commands/run/* if that remains the cleanest internal organization.
  • Keep Styles local to the leaf commands and keep attach / start client helpers narrow if broadening them adds no value.

Execution note: Start by preserving or expanding characterization coverage for JSON/text output and global --json gating before removing the old raw-plumbing signatures from these entrypoints.

Patterns to follow:

  • lib/crates/fabro-cli/src/commands/run/create.rs
  • lib/crates/fabro-cli/src/commands/preflight.rs
  • lib/crates/fabro-cli/src/commands/graph.rs
  • lib/crates/fabro-cli/src/commands/sandbox/mod.rs

Test scenarios:

  • Happy path: fabro run --detach still prints a bare run ID in text mode and the same JSON payload in JSON mode after output format moves behind CommandContext.
  • Happy path: fabro run / resume / attach still inherit verbose rendering from CLI output verbosity and preserve idle-sleep behavior.
  • Edge case: fabro graph --json without an explicit output file still rejects the global JSON override exactly as it does today.
  • Edge case: fabro sandbox preview --open still opens the browser only when global JSON mode is not active.
  • Edge case: fabro sandbox ssh still allows --print under global JSON mode but continues to reject the unsupported interactive combination.
  • Error path: preflight and validate continue to fail on validation errors with the same text-vs-JSON contract and exit behavior.
  • Integration: create -> start -> attach and resume/recovery flows still resolve targets, stream output, and summarize results through the same black-box CLI contracts.

Verification:

  • main.rs no longer passes the raw plumbing bundle into the run / preflight / validate / graph / sandbox preview-ssh boundary.

  • Those command boundaries obtain shared invocation data exclusively via CommandContext.

  • Unit 3: Migrate runs/artifact/store families to context-first family dispatch

Goal: Remove repeated target-context reconstruction from the command families that already build a target-based CommandContext immediately and mostly use raw CLI state only for output mode.

Requirements: R1, R3, R4, R6, R7

Dependencies: Units 1-2

Files:

  • Modify: lib/crates/fabro-cli/src/commands/runs/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/runs/list.rs
  • Modify: lib/crates/fabro-cli/src/commands/runs/archive.rs
  • Modify: lib/crates/fabro-cli/src/commands/runs/rm.rs
  • Modify: lib/crates/fabro-cli/src/commands/runs/inspect.rs
  • Modify: lib/crates/fabro-cli/src/commands/artifact/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/artifact/list.rs
  • Modify: lib/crates/fabro-cli/src/commands/artifact/cp.rs
  • Modify: lib/crates/fabro-cli/src/commands/store/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/store/dump.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/archive.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/inspect.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/rm.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/store_dump.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/store.rs

Approach:

  • Have each family receive a context-first boundary and derive its target-based command context once, close to the namespace/selector boundary.
  • Switch output-mode branches to ctx.user_settings().cli.output.format.
  • Preserve existing client-sharing patterns such as ServerSummaryLookup::from_client(ctx.server().await?).
  • Keep leaf helpers narrow where they already only need a resolved client, resolved run ID, or printer.

Patterns to follow:

  • lib/crates/fabro-cli/src/commands/secret/mod.rs
  • lib/crates/fabro-cli/src/commands/artifact/mod.rs

Test scenarios:

  • Happy path: runs list, archive, unarchive, and rm still render the same JSON and text shapes after output-mode decisions move behind the context.
  • Happy path: runs inspect remains a JSON-only projection of server state and still resolves the selected run before fetching the state.
  • Edge case: “no runs found” and “no artifacts found” text-mode messages remain unchanged.
  • Error path: ambiguous or missing run selectors still fail through the existing server/client resolution path.
  • Integration: artifact listing/copy and store dump continue to hit the same server-backed data path and respect output-format selection.

Verification:

  • These family dispatchers no longer need separate cli_layer and printer arguments merely to reconstruct a target context.

  • Unit 4: Migrate PR, secret, and system command families

Goal: Align the families that mix local settings, target-based server access, and storage-dir-aware server access onto the same context-first boundary.

Requirements: R1, R3, R4, R6, R7

Dependencies: Units 1-3

Files:

  • Modify: lib/crates/fabro-cli/src/commands/pr/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/pr/list.rs
  • Modify: lib/crates/fabro-cli/src/commands/pr/create.rs
  • Modify: lib/crates/fabro-cli/src/commands/pr/view.rs
  • Modify: lib/crates/fabro-cli/src/commands/pr/merge.rs
  • Modify: lib/crates/fabro-cli/src/commands/pr/close.rs
  • Modify: lib/crates/fabro-cli/src/commands/secret/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/secret/list.rs
  • Modify: lib/crates/fabro-cli/src/commands/secret/set.rs
  • Modify: lib/crates/fabro-cli/src/commands/secret/rm.rs
  • Modify: lib/crates/fabro-cli/src/commands/system/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/system/info.rs
  • Modify: lib/crates/fabro-cli/src/commands/system/df.rs
  • Modify: lib/crates/fabro-cli/src/commands/system/events.rs
  • Modify: lib/crates/fabro-cli/src/commands/system/prune.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/pr.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/pr_list.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/pr_create.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/pr_view.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/pr_merge.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/pr_close.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/secret.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/secret_list.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/secret_set.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/secret_rm.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/system.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/system_info.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/system_df.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/system_events.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/system_prune.rs

Approach:

  • Keep prs split between base-settings work and target-based run lookup, but move both sides onto a common context-first boundary so raw cli_layer / printer threading disappears from the family API.
  • Preserve secrets existing pattern of resolving the server once at the family boundary and passing the client into subcommands.
  • For system, derive connection-aware contexts from the base invocation context so storage-dir override behavior remains explicit and unchanged.

Patterns to follow:

  • lib/crates/fabro-cli/src/commands/pr/mod.rs
  • lib/crates/fabro-cli/src/commands/secret/mod.rs
  • lib/crates/fabro-cli/src/commands/system/mod.rs

Test scenarios:

  • Happy path: PR list/view/create/merge/close continue to render the same JSON/text contracts and still resolve GitHub credentials from merged local settings.
  • Happy path: secret list/set/rm continue to resolve one server client at the namespace boundary and honor JSON mode.
  • Happy path: system info/df/events/prune continue to use storage-dir-aware connection mode where appropriate.
  • Edge case: missing secret / no matching runs-to-prune / empty PR list still produce the same user-visible text-mode outcomes.
  • Error path: invalid storage-dir or connection resolution still fails before the command attempts remote work.
  • Integration: the system family continues to respect explicit --storage-dir overrides and local daemon resolution semantics.

Verification:

  • These family boundaries no longer accept raw plumbing bundles when the only reason was to build a CommandContext or inspect output mode.

  • Unit 5: Finish remaining base/target context users

Goal: Complete the context-first migration for the remaining in-scope command surfaces that already use CommandContext but still expose raw plumbing at their entrypoints.

Requirements: R1, R2, R3, R5, R6, R7

Dependencies: Units 1-4

Files:

  • Modify: lib/crates/fabro-cli/src/commands/model.rs
  • Modify: lib/crates/fabro-cli/src/commands/repo/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/repo/init.rs
  • Modify: lib/crates/fabro-cli/src/commands/repo/deinit.rs
  • Modify: lib/crates/fabro-cli/src/commands/auth/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/auth/login.rs
  • Modify: lib/crates/fabro-cli/src/commands/auth/logout.rs
  • Modify: lib/crates/fabro-cli/src/commands/auth/status.rs
  • Modify: lib/crates/fabro-cli/src/commands/provider/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/provider/login.rs
  • Modify: lib/crates/fabro-cli/src/commands/config/mod.rs
  • Modify: lib/crates/fabro-cli/src/commands/version.rs
  • Modify: lib/crates/fabro-cli/src/commands/doctor.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/model.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/model_list.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/model_test.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/repo.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/repo_init.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/repo_deinit.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/auth.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/provider.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/provider_login.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/config.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/version.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/doctor.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/json_global.rs

Approach:

  • Move base-context-only command families (auth, provider, parts of repo) onto CommandContext so JSON gating and printer usage come from the shared invocation object.
  • Use ctx.machine_settings() / ctx.user_settings() consistently for local settings lookups rather than parallel raw-CLI arguments.
  • Preserve the special-case JSON contract for auth status: explicit invocation-global --json remains the only JSON trigger, even if resolved CLI settings say output.format = json.
  • Keep repo deinit and similar pure-local leaf helpers narrow if a derived json_output boolean or Printer extracted from the context keeps the internal helper clearer than passing the full context.

Patterns to follow:

  • lib/crates/fabro-cli/src/commands/auth/login.rs
  • lib/crates/fabro-cli/src/commands/auth/status.rs
  • lib/crates/fabro-cli/src/commands/config/mod.rs
  • lib/crates/fabro-cli/src/commands/version.rs

Test scenarios:

  • Happy path: auth status and provider login preserve global --json restrictions and still resolve the intended server target from merged settings.
  • Happy path: config, version, model, and doctor continue to honor text-vs-JSON output without a direct CliNamespace parameter.
  • Edge case: persisted cli.output.format = "json" without explicit global --json still leaves auth status on its current text-mode path.
  • Edge case: explicit global --json still forces auth status onto its JSON output path even though the implementation no longer receives a raw process_local_json parameter.
  • Edge case: repo init non-JSON progress output and JSON result payloads stay unchanged.
  • Edge case: repo deinit still stays local and does not accidentally require server access just because the family boundary now uses CommandContext.
  • Error path: missing auth sessions, invalid server targets, and missing GitHub access continue to fail with the same user-facing contract.

Verification:

  • Remaining in-scope command entrypoints no longer expose the raw plumbing bundle as part of their public internal API.

  • Unit 6: Remove migration scaffolding and prove the boundary is clean

Goal: Delete transitional surface area and confirm the in-scope command tree is consistently aligned on CommandContext.

Requirements: R1, R2, R6, R7

Dependencies: Units 1-5

Files:

  • Modify: lib/crates/fabro-cli/src/command_context.rs
  • Modify: lib/crates/fabro-cli/src/main.rs
  • Modify: in-scope command modules touched by transitional wrappers
  • Test: lib/crates/fabro-cli/tests/it/cmd/top_level.rs
  • Test: lib/crates/fabro-cli/tests/it/cmd/json_global.rs

Approach:

  • Remove temporary wrappers or compatibility constructors that were only present to get through the migration.
  • Delete stale comments and “still being wired through” dead-code annotations once the boundary is real.
  • Do a final sweep to ensure in-scope command boundaries are not still taking &CliNamespace, &CliLayer, Printer, and process_local_json together out of habit.

Patterns to follow:

  • Keep cleanup limited to true scaffolding removal; do not reopen command semantics or formatting behavior in the cleanup pass.

Test scenarios:

  • Test expectation: none -- cleanup-only unit. Behavioral coverage should already exist from Units 1-5.

Verification:

  • The command-boundary API is visibly simpler and consistent across the in-scope families.
  • No in-scope command still looks half-migrated.

System-Wide Impact

  • Interaction graph: main.rs bootstrap -> base CommandContext -> family dispatchers -> derived target/connection contexts -> server_client.rs / user_config.rs. This touches nearly every user-facing CLI family that already talks to settings or server resolution.
  • Error propagation: context-construction failures remain early and synchronous at the command boundary; server access errors still flow through ctx.server().await? and should not move deeper into render helpers.
  • State lifecycle risks: the biggest correctness risk is accidental reuse of the wrong derived context, especially storage-dir-aware contexts in the system family and target-based contexts in run/PR flows.
  • API surface parity: although this is an internal refactor, it touches external CLI contracts indirectly through JSON/text output, verbosity, global --json restrictions, and server-target resolution.
  • Integration coverage: black-box command tests and scenario tests are the main safety net. Unit tests should only cover context construction/derivation and not replace CLI integration coverage.
  • Unchanged invariants: tracing and upgrade bootstrap ordering stays in main.rs; exec keeps its distinct direct-provider path; server connection semantics stay in server_client.rs; Styles remain command-local.

Risks & Dependencies

Risk Mitigation
Output-mode drift when replacing cli.output.format reads with ctx.user_settings().cli.output.format Keep characterization coverage in existing cmd/* tests for both text and JSON paths before deleting the raw parameters
Global --json behavior changes while moving process_local_json into the context Add targeted coverage in json_global.rs, auth.rs, provider_login.rs, graph.rs, and sandbox preview/ssh command tests
Storage-dir-aware system commands accidentally derive a target-mode context instead of a connection-mode context Keep connection-specific derivation explicit and cover system_info, system_df, system_events, and system_prune with CLI integration tests
CommandContext grows into a second god object Keep explicit scope rules: invocation plumbing only, no command args, no Styles, no feature-specific render state
The refactor becomes a giant compile-fix patch with poor reviewability Land the work family-by-family with temporary compatibility wrappers where needed, and verify each family with its existing tests before cleanup

Documentation / Operational Notes

  • No user-facing documentation changes are expected.
  • Internal comments in command_context.rs and nearby command modules should be updated to describe the final boundary, not the transitional “still being wired through” state.
  • The earlier April 8 plan should remain as historical context; this plan supersedes it for the command-boundary alignment work.

Sources & References

  • Prior plan: docs/plans/2026-04-08-cli-services-command-context-refactor-plan.md
  • Related plan: docs/plans/2026-04-22-001-refactor-settings-api-entrypoints-plan.md
  • Related code: lib/crates/fabro-cli/src/command_context.rs lib/crates/fabro-cli/src/main.rs lib/crates/fabro-cli/src/server_client.rs lib/crates/fabro-cli/src/commands/run/mod.rs lib/crates/fabro-cli/src/commands/pr/mod.rs lib/crates/fabro-cli/src/commands/secret/mod.rs lib/crates/fabro-cli/src/commands/auth/mod.rs lib/crates/fabro-cli/src/commands/provider/mod.rs lib/crates/fabro-cli/src/commands/system/mod.rs
  • Testing guidance: docs-internal/testing-strategy.md
  • Related history: 93b6577cd simplify: drop duplicate settings plumbing from cli/server refactor 367fd9302 refactor(cli): centralize command settings and server access 4b30a5f16 refactor(cli): route command output through Printer