Fail fast when a mutation-CLI flag is given no value - #1766
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMutation CLI argument parsing moves into ChangesMutation CLI parser
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
`deno task mutation --source` (or `--test`/`--jobs`/`--timeout`) with no following value used to quietly treat the flag itself as a positional glob. That surfaced much later as a confusing "No source files matched" or "Unexpected positional argument" error, hiding the real mistake. It now reports a clear "Missing value for --source." straight away. The pure argument parsing moves out of the CLI script into a new scripts/mutation/args.ts so it can be unit-tested on its own, leaving scripts/mutation.ts as a thin shell that expands globs and runs. Adds tests for parseArgs with full line/branch coverage and a 100% mutation kill rate, including the missing-value regression. Origin: TODO.md "Code-quality detector & test-strengthening follow-ups (from PR #1729)", item 3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191NXZLeokw3pYtJCwJK4VN
8523351 to
3fe7e37
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation/args.ts`:
- Around line 26-33: Validate the raw value in the "--timeout" handler before
converting it with Number(), rejecting empty or whitespace-only tokens instead
of treating them as zero; preserve numeric conversion for non-empty values.
Apply the same validation to the nearby "--jobs" numeric handler if it shares
this parsing behavior.
- Around line 55-58: Update the argument parsing branch around applyValue to
treat a following known option as a missing value, not as the current argument’s
value; preserve the first parsed.error and retain the existing handling for
valid values. Add regression coverage for adjacent flags such as --source --test
t.ts, asserting the missing-value error for --source.
- Around line 53-57: The VALUE_FLAGS lookup in parseArgs must only recognize own
flag entries, not inherited prototype keys. Replace the direct VALUE_FLAGS[arg]
access or add an own-property guard before invoking applyValue, so tokens such
as "__proto__" are treated as positional arguments while registered flags retain
their current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1602c299-50de-435f-9ba2-5955ce0304bf
📒 Files selected for processing (3)
scripts/mutation.tsscripts/mutation/args.tstest/scripts/mutation-args.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mutation.ts`:
- Line 21: Restore the USAGE symbol used by main() in scripts/mutation.ts by
either reintroducing the local help-text constant or exporting and importing it
from the mutation argument parser module; ensure console.log(USAGE) resolves
during compilation.
In `@scripts/mutation/args.ts`:
- Around line 11-19: Rename the ParsedArgs.error field to errorOrNull to
explicitly represent its nullable success state, then update all references in
scripts/mutation.ts and the associated tests to use the new property name
without changing behavior.
- Around line 72-87: Preserve errors already set by applyArg in the
positional-argument handling by changing both parsed.error assignments in the
shown parser flow to nullish assignment, so earlier errors such as a missing
--jobs value remain authoritative. Add a regression test covering positional
arguments combined with a flag missing its value and assert that the original
parser error is reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f5060e0-7e42-4124-92b8-11b9baf52830
📒 Files selected for processing (4)
TODO.mdscripts/mutation.tsscripts/mutation/args.tstest/scripts/mutation-args.test.ts
💤 Files with no reviewable changes (1)
- TODO.md
Address review findings on the argument parser: - Model both flag kinds as Maps (with a shared isKnownFlag predicate) instead of a plain-object lookup, so a token naming an Object.prototype member — __proto__, constructor, toString — is treated as an ordinary positional rather than resolving to an inherited value (and never invoked as a handler). - Treat a following known option as a missing value: `--source --test t.ts` now reports "Missing value for --source." rather than swallowing --test as the source glob. Negative numbers still work — `-5` is not a known flag, so `--timeout -5` is still read and rejected as invalid. - Route numeric flag values through a blank-safe parse so `--timeout ""` and `--jobs " "` are rejected instead of silently read as zero. - Use `??=` for the positional-argument errors so an earlier missing-value error stays authoritative. Each fix ships with a regression test; parseArgs keeps 100% line/branch coverage and a 100% mutation kill rate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191NXZLeokw3pYtJCwJK4VN
What this changes
Running
deno task mutation --source(or--test,--jobs,--timeout) with nothing after the flag used to quietly treat the flag itself as a positional value. That mistake then surfaced much later as a confusing message like "No source files matched" or "Unexpected positional argument", pointing the person at the wrong thing.Now the command stops straight away with a clear message: "Missing value for --source."
How
scripts/mutation/args.ts.scripts/mutation.tsis now a thin shell that expands globs and runs the tests; the parsing has no file access, so it can be tested on its own.applyArgnow recognises a value flag with no following token as a usage error instead of silently collecting the flag as a positional argument.Tests
New
test/scripts/mutation-args.test.tscoversparseArgswith 100% line and branch coverage and a 100% mutation-kill rate, including the missing-value regression for all four value flags and the non-negative/positive boundaries for--timeoutand--jobs.Origin
TODO.md→ "Code-quality detector & test-strengthening follow-ups (from PR #1729)", item 3. The TODO entry is removed as part of this change.🤖 Generated with Claude Code
https://claude.ai/code/session_0191NXZLeokw3pYtJCwJK4VN
Generated by Claude Code
Summary by CodeRabbit
mutationcommand-line parsing with stricter handling of boolean flags, value flags, and positionals.--jobsand--timeoutare validated reliably (including missing values, non-numeric input, and invalid ranges), with consistent error precedence.--source/--testflags (including edge cases).