Summary
#2920 made the four reporting CLIs reject a mistyped flag NAME. It left the other half of the same defect open: an unusable flag VALUE still falls through to the default, and the command exits 0 with a report for parameters the caller never supplied.
CodeRabbit flagged this on #2920 as the merge risk and it was right; I did not act on it before merge. Six cases, measured on current main:
detect-reposts --window --summary -> exit 0, 90-day window
detect-reposts --window abc -> exit 0, 90-day window
detect-reposts --window -5 -> exit 0, negative window accepted
process-quality --min-threshold abc -> exit 0, threshold 1
process-quality --file -> exit 0, read the default path
weekly-digest --dir -> exit 0, digested the default dir
This is exactly what #2919 described, arriving through the value instead of the name. --windo 60 is now refused; --window abc is not, and both produce the same wrong report.
The family is already inconsistent with itself: rejection-latency.mjs, converted in the same PR, exits 2 on a missing or flag-like value because it carried its own argValue helper. The other three use bare flagValue and default.
The part that needs a decision, not just a patch
Two existing tests assert the current behaviour explicitly:
eq('--window abc falls back to 90', badWindowJson.metadata.windowDays, 90); // detect-reposts.test.mjs
eq('--min-threshold abc falls back to 1', badThresholdJson.metadata.minThreshold, 1); // process-quality.test.mjsSo the fallback was written down as intended, not merely unexamined. Reversing it is a behaviour change on a documented contract, and that is your call.
The case for reversing: it is the same behaviour you described on merging #2920 — "worse than a crash: it answers a question nobody asked and nothing flags it" — and rejection-latency already does the opposite in the same family.
The case against: a report with a default window is arguably still useful output, and --window abc is a rarer mistake than --windo.
I lean to reversing, but say so explicitly so it is not slipped in under a test update.
Also in scope
Two review nits from #2920 that are now in main:
- @Scott-Emberson:
new RegExp(typo.replace(/^--/, '--')) in tests/cli-flag-validation.test.mjs:55 replaces -- with itself — a no-op. CodeQL independently flagged it as "Replacement of a substring with itself". It also compiles the typo into a regex, so a typo containing a metacharacter would match as a pattern rather than literally.
- @Scott-Emberson: only
rejection-latency asserts on output; the other three rest on --help and an exit code, which cannot distinguish "the value was honoured" from "the value was dropped and the default produced the same exit status".
And @artemtrofymenko's structural point from the same review: a KNOWN_FLAGS allow-list in a file makes every in-flight PR that adds a flag a silent coordination dependency — he showed #2702's --min-span merging without a textual conflict and then being rejected by its own file at runtime. He suggested a marker comment in each block so the next rebase catches it.
I have all of this working and can open the PR.
Summary
#2920 made the four reporting CLIs reject a mistyped flag NAME. It left the other half of the same defect open: an unusable flag VALUE still falls through to the default, and the command exits 0 with a report for parameters the caller never supplied.
CodeRabbit flagged this on #2920 as the merge risk and it was right; I did not act on it before merge. Six cases, measured on current
main:This is exactly what #2919 described, arriving through the value instead of the name.
--windo 60is now refused;--window abcis not, and both produce the same wrong report.The family is already inconsistent with itself:
rejection-latency.mjs, converted in the same PR, exits 2 on a missing or flag-like value because it carried its ownargValuehelper. The other three use bareflagValueand default.The part that needs a decision, not just a patch
Two existing tests assert the current behaviour explicitly:
So the fallback was written down as intended, not merely unexamined. Reversing it is a behaviour change on a documented contract, and that is your call.
The case for reversing: it is the same behaviour you described on merging #2920 — "worse than a crash: it answers a question nobody asked and nothing flags it" — and
rejection-latencyalready does the opposite in the same family.The case against: a report with a default window is arguably still useful output, and
--window abcis a rarer mistake than--windo.I lean to reversing, but say so explicitly so it is not slipped in under a test update.
Also in scope
Two review nits from #2920 that are now in
main:new RegExp(typo.replace(/^--/, '--'))intests/cli-flag-validation.test.mjs:55replaces--with itself — a no-op. CodeQL independently flagged it as "Replacement of a substring with itself". It also compiles the typo into a regex, so a typo containing a metacharacter would match as a pattern rather than literally.rejection-latencyasserts on output; the other three rest on--helpand an exit code, which cannot distinguish "the value was honoured" from "the value was dropped and the default produced the same exit status".And @artemtrofymenko's structural point from the same review: a
KNOWN_FLAGSallow-list in a file makes every in-flight PR that adds a flag a silent coordination dependency — he showed #2702's--min-spanmerging without a textual conflict and then being rejected by its own file at runtime. He suggested a marker comment in each block so the next rebase catches it.I have all of this working and can open the PR.