refactor: simplify command architecture through local ownership#69
Open
francoischalifour wants to merge 24 commits into
Open
refactor: simplify command architecture through local ownership#69francoischalifour wants to merge 24 commits into
francoischalifour wants to merge 24 commits into
Conversation
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.
…rchitecture # Conflicts: # cli/src/lib/updater.test.ts
albert20260301
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
lib/.lib/contains only behavior shared across command families.Hide Citty behind the CLI-owned
Command,defineCommand, anddefineArgsAPIs. 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:
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:
--replace-profilelogin flows behave correctly;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.