fix(cli): deliver --no-record to the daemon (supersedes #1305)#1311
Merged
Conversation
#1305 claimed to forward --no-record from "every recordable command reader". Measured through the real argv -> reader -> client -> daemon chain on its own merge commit, the flag reached the daemon for ONE command (`open`). It is now 5 of 33 on current main -- `open` plus get/is/find/snapshot, the latter four only incidentally, because #1303 declared `noRecord` in their metadata. #1305's fix was inert because it fixed a layer that is not load-bearing. Its test asserted on `readInputFromCli` output -- an intermediate object two later layers rebuild from scratch: 1. `defineExecutableCommand.invoke` runs `metadata.readInput(input)` -> `readFieldInput`, which keeps ONLY declared metadata fields plus `readCommonInput`'s output. `noRecord` was neither, so it was filtered. (`open` survived solely because its metadata declares the field.) 2. Each `to*Options` projection rebuilds the client options from `commonToClientOptions` plus its own named fields; that helper did not carry `noRecord` either. So `--no-record` parsed, was accepted on every command, and was silently dropped before dispatch -- including on press/click/fill and, per the maintainer's review, gesture/back/home. Fixed at the seams the flag must survive, not per reader: - `commonInputFromFlags` and `selectionOptionsFromFlags` (the reader layer has TWO parallel common helpers -- reader-input shape vs client-options shape -- so both must carry it; `settings` used only the latter, which is why it was the last gap); - `readCommonInput` (stop `readFieldInput` filtering it); - `commonToClientOptions` (stop `to*Options` dropping it). Measured after: 33/33 deliver the flag, with zero hand-listed commands. #1305's `noRecordInputFromFlags` helper and all 13 hand-added call sites are deleted: they are redundant against the seams, and leaving both would be two sources of truth for one behavior -- exactly how the next gap breeds. Preserves the --record asymmetry (ADR 0012 decision 6 amendment): --no-record is common and rides the common seam; --record stays scoped to snapshot/get/is plus a dynamically-validated find, on its own narrow helper. Also fixes `get --record`, dead through the CLI since #1303 for the same re-projection reason (`toGetOptions` rebuilds its options object), which that PR's daemon-level scenario could not see. Coverage is asserted where it is observable, not at the intermediate object: - `cli-record-flag-delivery.test.ts` drives real argv and asserts on the DAEMON REQUEST for all 32 recordable routes; it fails on reverting either seam ("press accepted --no-record but never delivered it to the daemon"). - `no-record-recorder-routes.test.ts` is a healed-script regression: gesture/ back/home with --no-record must not land in a written .ad. Reverted, it fails with the leaked `gesture "fling" "up" 100 200` line in the script. A derived `recordsSessionAction` classification + completeness gate follows in a separate PR: this fixes the 32, that makes a 33rd impossible.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
added
the
ready-for-human
Member
Author
|
Exact-head review ( The descriptor completeness/MCP schema-discoverability work remains correctly separated in #1310; it is residual follow-up, not a current runtime defect. Minor prose nit only: the PR says 33/33 in one place while its explicit runtime fixture contains 32 commands—please normalize the count. All checks are green and the PR merges cleanly. Applying |
|
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.
#1305 was incomplete — this supersedes it
#1305 said it forwarded
--no-recordfrom "every recordable command reader." That claim was wrong, and not by three commands.Measured through the real chain a user's argv takes —
parseArgs→readInputFromCli→runCommand→ client → transport, asserting on the daemon request — the flag reached the daemon for 1 of 33 commands on #1305's own merge commit (open). On current main it is 4 of 33:open,is,find,snapshot.Those four work only incidentally. #1303 declared
noRecordin the metadata forget/is/find/snapshot, which gets them pastreadFieldInput's filter — but metadata declaration is necessary, not sufficient: you must also survive theto*Optionsre-projection.is/findpass their whole input through (client.interactions.is(input as IsOptions)) andsnapshot/openlikewise, so they land.getdoes not — it is the one that rebuilds its options object viatoGetOptions(...), which names onlycommonToClientOptions+ target + selector-snapshot +format, and drops everything else. Soget --no-recordis declared, survivesreadInput, and still dies one layer later.--no-recordparsed, was accepted on every command (it's inCOMMON_COMMAND_SUPPORTED_FLAG_KEYS), and was silently dropped before dispatch — including onpress/click/fill, and on thegesture/back/homeroutes the maintainer's review flagged.Why it shipped green while inert
#1305's test asserted on
readInputFromClioutput — an intermediate object that two later layers rebuild from scratch:defineExecutableCommand.invokerunsmetadata.readInput(input)→readFieldInput, which keeps only declared metadata fields plusreadCommonInput's output.noRecordwas neither → filtered. (opensurvived solely because its metadata declares the field.)to*Optionsprojection rebuilds client options fromcommonToClientOptions+ its own named fields; that helper didn't carrynoRecordeither.A reader-level assertion is structurally incapable of catching this. That's the same methodological defect the review identified — one layer deeper than expected.
The fix: seams, not a longer list
commonInputFromFlagsselectionOptionsFromFlagsreadCommonInputreadFieldInputfiltering itcommonToClientOptionsto*Optionsdropping itThe reader layer has two parallel common helpers, not one —
deviceTargetshape vstargetshape. They're different projections, not duplicates, so the flag must ride both.settingsused only the second, which is exactly why it was the last remaining gap; it's now fixed by the seam, not individually.Measured after: 33/33 deliver the flag. The fix is seam-level — zero hand-listed call sites, which is the actual win here: a reader cannot forget the flag, because it never names it.
That is a claim about the fix, not about the tests, and the two must not be conflated.
RECORDABLE_ARGVin the delivery test hand-lists all 33[command, argv]pairs, because per-command positionals cannot be derived. That is a legitimate fixture, not evidence of completeness: a 34th recordable command would not auto-join it, and this suite would stay green. Attaching a completeness claim to a hand-written table is precisely the stamp that made #1305 look finished. Completeness enforcement is PR 2's job (see Follow-up).Deleted
noRecordInputFromFlagsand all 13 hand-added call sites — redundant against the seams, and keeping both would be two sources of truth for one behavior, which is how the next gap breeds.--recordasymmetry preservedPer ADR 0012 decision 6's amendment:
--no-recordis common and rides the common seam;--recordstays scoped tosnapshot/get/isplus a dynamically-validatedfind, on its own narrowobservationRecordInputFromFlagshelper. A test pins both directions (including thatpress --recordis refused by the grammar).Also fixes
get --record— dead through the CLI since #1303 (mine) for the identical re-projection reason:toGetOptionsrebuilds its options object and dropped it. #1303's scenario set daemon flags directly, so it couldn't see this. Same root cause as the bug this PR fixes; leaving it while touching that exact function wasn't defensible.Coverage, asserted where it's observable
src/__tests__/cli-record-flag-delivery.test.ts— drives real argv, asserts on the daemon request for all 32 recordable routes. Reverting either seam fails it:press accepted --no-record but never delivered it to the daemon.test/integration/provider-scenarios/no-record-recorder-routes.test.ts— healed-script regression at the recorder route:gesture/back/homewith--no-recordmust not land in a written.ad, with an unflaggedpressas the live control. Reverted, it fails with the leaked line in the script:Follow-up (separate PR, next)
A derived
recordsSessionActiondescriptor classification + completeness gate, on the existingrefFrameEffectpattern. This PR fixes the 32; that one makes a 33rd impossible.The MCP schema-advertisement gap (schemas won't advertise
noRecordbeyondopen, sincereadCommonInputis runtime-only) is discoverability, not function — filed separately, deliberately not bundled into a forwarding fix.Gates
tsc · oxlint --deny-warnings · format:check · fallow (11 files, no issues) ·
test:integration:progress:check(exit 0) · unit-core targeted 185 files/1845 tests · provider-integration 36/36 serial.Full unit-core shows 6 failures in files this PR doesn't touch (
install-source,artifact-materialization,runtime-hints,screenshot-density) — all 5s real-time timeouts / temp-dir races on a loaded box, zero AssertionErrors; all 4 files pass in isolation.