refactor: centralize command family facets#849
Conversation
|
#849 is not review-ready yet: GitHub reports Please rebase/update the branch on latest |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Reviewed the family-facet centralization. The core idea — one CommandFamilyFacet per family composed from per-command facets, with a single family/registry.ts merging metadata/definitions/readers/writers/schemas/formatters — is a clean consolidation, and the new command-surface-metadata.test.ts coverage is a good addition. A few things to address before this is mergeable:
🔴 1. #848's batch-step validation appears to be reverted (blocking)
This branch was cut before #848 (fix: validate batch steps through command contracts) landed on main, and the merge conflict has effectively undone it. On main, prepareBatchDaemonCommandRequest validates each step through the command contract before projecting:
return writer(metadata.readInput(input) as CommandInput);
// ...wrapped with: `Batch step ${stepNumber} ${command} input is invalid: ${message}`In this PR command-projection.ts collapses to return writer(input), drops the stepNumber parameter (also removed from batch/projection.ts's PrepareDaemonCommandRequest type), and batch/cli.test.ts deletes the guard test batch rejects invalid structured step input before daemon projection. The "structured interaction target" test was also edited to drop its platform/udid input and assertions — consistent with step input no longer flowing through readInput.
Net effect: invalid structured batch steps (e.g. {"command":"focus","input":{"x":10}} missing y) are no longer rejected with the friendly per-step error, and common-option normalization on step input is lost. Note this regression won't show in the GitHub diff until you rebase (the PR's merge base predates #848), which makes it easy to miss. Please rebase onto main and preserve #848 — ideally by relocating the readInput validation into the new facet path rather than dropping it.
🟠 2. Compile-time exhaustiveness traded for runtime checks (see inline comment)
Several registries moved from … satisfies Record<CommandName, …> to listCommandFamily…() as Record<CommandName, …> (cli-grammar/registry.ts, cli-output.ts, cli-command-overrides.ts, command-surface.ts, command-metadata.ts). The satisfies previously guaranteed at compile time that every command had a reader/writer/schema; the cast removes that, leaving only the new runtime tests + Missing … for command throws. Worth checking whether defineCommandFamilyFromFacets can restore a CommandName-keyed exhaustiveness check.
🟠 3. Runner command traits decentralized + tests removed
runner-command-traits.ts and its test are deleted, and the single satisfies Record<RunnerCommand['command'], RunnerCommandTraits> table is split into three independent inline definitions: isReadOnlyRunnerCommand (|| chain in runner-contract.ts), PREFLIGHT_SKIP_ELIGIBLE_RUNNER_COMMANDS (set in runner-session.ts), and isRunnerReadinessProbeCommand (inline in runner-session.ts). This is the opposite of the PR's "centralize" theme, and it removes both the exhaustiveness guarantee and the test that enumerated every command — on a correctness-sensitive path (readiness-preflight skipping). If the decentralization is intentional, please add a test asserting these three sets cover the full RunnerCommand['command'] union.
⚪ Minor
mergeable_stateisdirty/ conflicting withmain(per your own bot comment). Resolving #1 via rebase should also clear this.package.json: switchingformat/format:checktonode ./node_modules/oxfmt/bin/oxfmtto avoid the flaky shim is reasonable, but it now hard-codes the package layout — fine, just flagging it's coupled to oxfmt'sbinpath.
Items 2 and 3 are judgment calls (the runtime tests do provide a safety net); item 1 is the one I'd consider blocking.
Generated by Claude Code
thymikee
force-pushed
the
refactor/command-family-facets
branch
from
8d34c27 to
fe0dc97
Compare
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
CI is past the merge conflict now, but Integration Tests is failing in the provider-backed progress check: |
thymikee
force-pushed
the
refactor/command-family-facets
branch
from
fe0dc97 to
b458e4c
Compare
|
Reviewer pass is clean: no actionable findings. Merge-ready from my side. I checked the updated PR metadata/check rollup and an isolated review pass traced the changed command-family path through family registry -> CLI grammar -> command projection -> daemon routing/output. CI is green, including Integration Tests after the Residual risk: future command additions rely more on the guard tests staying in the required validation path than on one compile-time exhaustive map, but the current PR has coverage for the affected surfaces. |
1 participant
Summary
Centralizes command family facets behind a single registry and moves capture to the command-level shape from the architecture review.
Before: command metadata, executable definitions, CLI readers, daemon writers, output formatters, and CLI overrides were stitched together across separate central registries.
After: each family exports one family facet, and capture composes per-command facets for snapshot, screenshot, diff, wait, alert, and settings. This removes the old client-command facet registry and narrows intermediate exports. Also fixes format scripts to invoke the installed oxfmt entrypoint directly, avoiding the flaky shim path.
Touched files: 29. Scope stayed within command surface/family architecture and formatter script wiring.
Validation
Verified with formatter, command-surface guard tests, capture command tests, CLI args tests, fallow, tooling/build, and the full unit/smoke aggregate.