refactor(cli): derive the common flag readers from the common-field table - #2292
Merged
Conversation
…able commonInputFromFlags and selectionOptionsFromFlags in src/commands/cli-grammar/common.ts each hand-restated the common input key list as object literals. A key added to the common-field table in commands/common-input-fields.ts (the schema/read/clientKey/audience source of truth) had no structural link to either flag reader, so keeping a new key flowing through the CLI depended on remembering to touch both literals by hand (the failure mode #1304/#1305 hit for --no-record). Add a flagKey/flagIn column to CommonInputFieldSpec: flagKey names the CliFlags property when its spelling differs from the row (only deviceTarget, which reads flags.target), and flagIn lists which of the two flag-derived projections ('input', 'selection') the row joins. commonFlagProjection(flags, projection) walks the table once and reuses the existing clientKey renaming (deviceTarget -> target) for the 'selection' projection, matching commonToClientOptions's existing convention. commonInputFromFlags and selectionOptionsFromFlags become thin wrappers over that projection; COMMON_COMMAND_SUPPORTED_FLAG_KEYS in flag-groups.ts is untouched, per its own documented reason for staying separate. Added src/commands/cli-grammar/common.test.ts as a characterization test before touching the implementation: it feeds a CliFlags fixture with every common flag set, and one with none set, and asserts the literal objects both functions return. It passed unmodified against both the old hand-written implementation and the new table-derived one, proving the projections are byte-identical. Validation: pnpm vitest run --project unit-core src/commands (575 tests, all green); pnpm check:quick (lint + typecheck) clean; pnpm check:layering clean (cli-grammar/common.ts importing commands/common-input-fields.ts is an intra-commands-zone import, not core/daemon reaching into commands, so R2 commands-floor does not apply).
Reviewer findings on the common-flag-reader refactor (9074367): - P2: commonFlagProjection cast flags to Record<string, unknown> and fell back to `field.flagKey ?? key`, so a row could declare `flagIn` with a key that is not a CliFlags property at all (cwd, debug) and compile cleanly while reading undefined forever. CommonInputFieldSpec now requires `flagKey: keyof CliFlags` whenever a row declares `flagIn`; the cast and fallback are gone and commonFlagProjection reads `flags[field.flagKey]` directly. Verified red/green: temporarily adding `flagKey: 'cwd'` made tsc reject the table; reverted after confirming. - P2: comments in common.ts and common-input-fields.ts claimed the table made a dropped common key "impossible" / that flagIn "rides structurally". A new common row that omits flagIn still silently joins neither projection, uncaught by any compiler check -- the flagKey fix narrows the failure mode, it doesn't close it. Reworded both comments to say what the table actually guarantees. - P3: common.test.ts's ALL_COMMON_FLAGS fixture left daemonBaseUrl, daemonAuthToken, tenant, runId, and leaseId unset, so a row wrongly gaining `flagIn: ['input']` for one of them would still pass -- compactRecord drops the unset value either way. Added NON_PROJECTED_COMMON_FLAGS with concrete values for those keys so such a row now surfaces as an extra key. Verified red/green: temporarily adding `flagIn: ['input']` to daemonBaseUrl failed the test; reverted after confirming. - P3: flag-groups.ts's comment said the common-input-fields table declares only envFlagKeys; it now also declares flagKey/flagIn. Amended to name both columns. Deferred one P3 (selectionOptionsFromFlags's cast to SelectionOptions) with a note in .tmp/common-readers-pr-body.md -- fixing it would import cli-grammar/types.ts back into common-input-fields.ts, reversing the documented one-way dependency for a cast the characterization test already pins. No layering, fallow, or gate-manifest surface touched, so no separate chore(gates) commit. Checks: pnpm vitest run --project unit-core src/commands (575 tests, green) and pnpm check:quick (lint + typecheck), both clean.
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks:
Top changed packed files
|
Member
Author
|
Reviewed exact head |
thymikee
added
the
ready-for-human
|
thymikee
added a commit
that referenced
this pull request
Sep 5, 2026
* origin/main: perf: bundle runtime dependencies and report full install size (#2310) ci: avoid unrelated Apple runner cache invalidation (#2303) fix(web): preserve the backend ref so snapshot refs match actionable refs (#2283) test(daemon): session-open-url-prewarm through the request seam (#2304) test(daemon): session-devices-batch-runtime through the request seam (#2305) chore(gates): layering baselines ratchet against merge-base (#2299) test(daemon): one typed conformance helper for the daemon runtime suites (#2298) chore(layering): derive the contracts export inventory from package.json (#2297) perf: bundle tar-stream to reduce install footprint (#2286) docs: simplify agent context and resolve conflicting guidance (#2287) refactor(cli): let help resolve command aliases itself and retire R12 (#2293) refactor(commands): retire the navigation-only type projection (#2294) feat(runtime): route managed leases through contained transports (#2285) refactor(contracts): build unavailable runtime facts once (#2291) refactor(cli): derive the common flag readers from the common-field table (#2292) feat(daemon): add managed allocation operation journal (#2284)
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
1 participant
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.
Summary
commonInputFromFlagsandselectionOptionsFromFlagsinsrc/commands/cli-grammar/common.tseach hand-restated the common CLIinput keys as object literals, with no structural link to the
common-field table in
commands/common-input-fields.tsthat alreadyowns each key's schema, reader, client-options name, and audience.
Before, adding or reclassifying a common key meant remembering to also
touch both literals by hand — the exact failure mode
--no-recordsuffered in #1304/#1305. After,
CommonInputFieldSpeccarries aflagKey/flagIncolumn,flagKeyis required and typed againstCliFlagswhereverflagInis set (closing a gap where a row couldbind to a non-existent flag like
cwdand silently readundefinedforever), and a new
commonFlagProjection(flags, projection)helperwalks the table once for both readers.
Validation
Tested commit: 8d4cb08 —
pnpm check:affected --rungreen locally (format, lint, typecheck, layering, fallow, build, vitest-related: 1656 tests).common.test.tsran green against theold literals and again unmodified against the table-derived version
— deep-equal (key order changed, nothing consumes it). Extended the
fixture so a row wrongly gaining
flagInshows an extra valueinstead of being compacted away; confirmed by temporarily adding
flagIntodaemonBaseUrl(test failed) andflagKey: 'cwd'(tscrejected it), then reverting both.
pnpm vitest run --project unit-core src/commands— 575 tests green.pnpm check:layering— clean; thecli-grammar/common.ts→commands/common-input-fields.tsimport is intra-commands-zone.Review notes
selectionOptionsFromFlagsstill casts toSelectionOptions.Removing it needs
common-input-fields.tsto import fromcli-grammar/types.ts, reversing the one-way dependency; not worthit for a cast the characterization test already pins.