Skip to content

refactor: simplify command architecture through local ownership#69

Open
francoischalifour wants to merge 24 commits into
mainfrom
fc/simplify-command-architecture
Open

refactor: simplify command architecture through local ownership#69
francoischalifour wants to merge 24 commits into
mainfrom
fc/simplify-command-architecture

Conversation

@francoischalifour

Copy link
Copy Markdown
Member

The CLI’s source structure no longer reflected its public command tree. Command definitions, argument composition, execution helpers, and tests were distributed across broad shared modules and a generic operation/effect framework.

This made small command changes difficult to review: understanding one command often required tracing indirect builders, Citty types, shared registries, ambient runtime state, and tests located elsewhere.

Proposed solution

Restructure the CLI around explicit, local command ownership:

commands/<family>/index.ts
commands/<family>/<command>.ts
commands/<family>/<command>.test.ts
commands/<family>/lib/*
  • The directory tree now shows which commands exist.
  • Each command declares its public definition at the top of its module.
  • Family-specific helpers live under that command family’s lib/.
  • Top-level lib/ contains only behavior shared across command families.
  • Unit tests are colocated with their subjects; root tests remain black-box CLI tests.

Hide Citty behind the CLI-owned Command, defineCommand, and defineArgs APIs. Runtime, output, and execution dependencies are bound once at the root and passed explicitly instead of being accessed through compatibility facades or ambient state.

Replace the generic operation/effect framework with a direct command flow:

  1. parse arguments;
  2. construct a plain request value;
  3. execute it;
  4. present the response.

Declarative request values remain the seam for a future dry-run mode, without maintaining a generic execution framework before dry-run exists.

Shared arguments are composed from their semantic owners—resources, files, timeouts, formats, and presentation options—so commands expose only flags they actually support.

Higher-level tests and correctness

Tests now exercise commands through the public runtime and user-visible flows instead of calling exported implementation helpers. This made important behavioral gaps observable and allowed the corresponding fixes to be covered at the same level:

  • browser login persists the control-plane endpoint used to authenticate;
  • logging into another organization preserves the existing authenticated profile;
  • fresh-profile, lakehouse-only, and --replace-profile login flows behave correctly;
  • clearing a login removes all stored OAuth token state;
  • unexpected macOS Keychain deletion failures stop profile deletion instead of silently leaving credentials behind;
  • catalog request failures are preserved rather than masked by parallel work.

Duplicate suites and test-only production exports were removed while retaining these higher-value behavioral scenarios.

Result

The branch makes command ownership visible, reduces indirection and production exports, removes circular dependencies, and deletes more code than it adds while preserving the public CLI surface.

Move command assembly and argument normalization out of the bootstrap so command-surface changes have a single owner. Prepare Knip to recognize colocated unit tests under src.
Make the command surface visible from the filesystem by moving top-level commands into named directories. Move shared lakehouse argument parsing out of the command tree so non-command modules cannot be mistaken for commands.
Keep declarative HTTP request values as the future dry-run seam while removing the effect interpreter, operation catalog, and generic command builders. Leaf command modules now own their parsing, request execution, and presentation flow.
Make command directories reveal the public command tree, place family-private implementation under commands/<family>/lib, and reserve top-level lib for shared code. Move Bun unit tests beside their subjects and keep only black-box tests at the repository root.
Document direct command execution, declarative HTTP requests as the dry-run seam, narrow library ownership, centralized registration, and colocated unit tests.
Declare each command immediately after imports so reviewers see its public shape before implementation details. Keep Citty behind lib/command.ts, derive delegated command names from the registered tree, and document the boundary for future commands.
Use shared table, file, timeout, and format argument definitions for append, upload, upsert, and schema. Move append-only parsing and schema tests to their command families, share file inspection without cross-command imports, and remove stale duplicate API body tests.
Fetch databases before connections so a database HTTP error cannot be masked by a racing secondary request. This also makes the request order and early-exit behavior explicit.
Use defineArgs and defineCommand for test fixtures so tests exercise the same Citty abstraction boundary as production command definitions.
Rename shared test helpers to test-utils and add one harness that executes the real command tree while capturing user-visible output.\n\nMove query, append, upload, upsert, schema, DuckDB, API, and login coverage beside their commands. Replace helper-level login and schema tests with behavioral scenarios, and keep command-only helpers private.\n\nThe command-level login coverage also fixes control-plane overrides so they flow through the login session without mutating environment-backed profile selection.
Remove accidental re-export facades from the HTTP, lakehouse, profile, updater, and management modules so callers import the module that owns each value.\n\nInline single-use payload, schema-rendering, and URL-encoding wrappers. Keep catalog request construction colocated with catalog commands and remove dead command exports, leaving shared libraries for behavior used by multiple command families.
Keep writeCommandOutput as the single command-output composition point and require output sinks explicitly. Remove compatibility wrappers and the unused management transport that were referenced only by their own tests.\n\nMake query rendering and management row extraction independent of ambient output context. This removes six dependency cycles while preserving command behavior.
Keep lakehouse resource arguments separate from query presentation policy. Move request timeout ownership to timeout-args, keep query invocation arguments with the query command, and pass agent context explicitly when resolving output behavior.\n\nSchema now composes only the format, pager, and timeout fragments it supports, rejecting query-only layout, columns, and max-width flags instead of accepting ineffective options.
Move prompts, error presentation, catalog loading, query output, and release tooling to their semantic owners. Pass execution context explicitly during profile verification and remove dead profile presentation and test-only production APIs.\n\nAdd a production Knip gate and eliminate all circular dependencies so runtime ownership stays reviewable as commands evolve.
Carry the resolved control-plane root through browser login and save it on the selected profile. Reused derived profiles can no longer retain a stale endpoint after a successful authentication.
Route Keychain process access through a narrow SecretStore factory while preserving the existing top-level API. Profile rename and delete now accept only the secret capabilities they need, enabling macOS behavior tests without test-only production exports.
Keep defineCommand as a typed declarative identity and apply runtime, sink, and execution dependencies only when assembling the root tree. Completion tests now execute through that same public command boundary instead of calling handlers directly.
Split umbrella suites by module ownership, keep profile model coverage focused on state transitions, and move representation and renderer contracts beside their subjects. Consolidate duplicate assertions while retaining Keychain deletion and rename rollback coverage.
Treat a missing macOS Keychain item as an idempotent delete, but propagate every other security command failure before removing file fallback or profile state. Cover both the SecretStore contract and profile deletion behavior.
Exercise --replace-profile, login into a fresh empty profile, and lakehouse-only profile preservation through the browser-login command surface. Verify OAuth clearing removes both stored tokens as well as expiry metadata.
Track dynamically compiled colocated fixtures as Knip entries after moving tests into src. Remove the updater interval setter and global config writer that became test-only when the simplified update command from main was integrated; tests now exercise the underlying config-file boundary directly.
@francoischalifour
francoischalifour requested a review from redox July 16, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants