Skip to content

fix(cli): reject flag-shaped and empty values on string flags - #397

Merged
oekazuma merged 1 commit into
mainfrom
advisor/043-string-flag-value-guard
Aug 8, 2026
Merged

oekazuma merged 1 commit into
mainfrom
advisor/043-string-flag-value-guard

Conversation

@oekazuma

@oekazuma oekazuma commented Aug 7, 2026 •

Copy link
Copy Markdown
Owner

Summary

PR #392 migrated argument parsing from mri to node:util's parseArgs (strict: false). Under parseArgs, a declared string flag consumes the next token even when that token is another flag, and --flag= passes an empty string. Both silently un-gate a CI run:

  • svelte-vitals --route --staged parsed as { route: "--staged" } — the run analyzed a route literally named --staged (matching nothing), reported a clean result, and exited 0, with --staged silently dropped.
  • svelte-vitals --min-health= parsed as ''; Number('') === 0 passed the range check, so the gate became "health ≥ 0" — a gate that can never fail. --min-health="$THRESHOLD" with an unset CI variable is the realistic way to hit this.
  • --meta-components= yielded [], discarding the config file's metaComponents list and producing false "missing title/description" criticals.

The repo already guarded exactly one flag against this class (--baseline, with a comment naming the failure mode). This PR extends that stance to every value-carrying flag.

Changes

  • resolve-args.ts: a shared guard over the 11 value-carrying string flags — a value that is missing, empty, or flag-shaped (leading -) is now a fatal error (exit 2). --baseline keeps its existing, more specific message; --diff is exempt by its documented optional-value contract (bare/empty defaults to HEAD).
  • --min-health parsing moved from bin.ts into resolveArgs, where every other flag is validated (single message shape, unit-testable without process exit). run()'s programmatic-API range check is intentionally untouched.
  • Tests: a table test over all 11 guarded flags (flag-followed-by-flag), empty-value shapes, --min-health numeric/range shapes incl. boundary values 0/100, and a pin for the --diff exemption.

Verification

  • pnpm build / pnpm -r typecheck / pnpm test (core 1292, cli 841, vite 207) / pnpm lint all green.
  • Built CLI: --route --staged → --route requires a value., exit 2; --min-health= → exit 2; --diff --reporter json unaffected (no value error).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • CLI string flags now reject missing, empty, or flag-shaped values with exit code 2.
    • --min-health now validates numeric values within the 0–100 range.
    • Invalid --min-health input is reported consistently during argument processing.
    • Preserved existing --diff behavior: bare or flag-followed usage defaults to HEAD.

parseArgs (strict:false) lets a declared string flag consume a following
flag token (--route --staged -> route '--staged') and lets --flag= pass
an empty string; either silently un-gates a CI run. Extend the existing
--baseline guard to every other string flag, and move --min-health's
range validation into resolveArgs alongside it so `Number('')` can no
longer coerce an empty --min-health into a health gate that never fails.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The CLI now rejects missing, empty, and flag-shaped values for value-carrying flags. --min-health validation occurs in resolveArgs, which returns the parsed value. Bare --diff usage still defaults to HEAD.

Changes

CLI validation flow

Layer / File(s) Summary
Argument validation and regression coverage
packages/cli/src/resolve-args.ts, packages/cli/test/resolve-args.test.ts
resolveArgs validates value flags and --min-health, returns minHealth, and tests invalid and valid inputs. Tests also preserve bare --diff behavior.
CLI execution wiring
packages/cli/src/bin.ts, .changeset/reject-flag-shaped-values.md
The CLI entrypoint uses minHealth from resolveArgs. The changeset documents the updated flag behavior.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main CLI change: rejecting flag-shaped and empty values on string flags.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/cli/src/resolve-args.ts (1)

114-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the redundant property comment.

minHealth?: number already expresses this information.

As per coding guidelines, comments should explain constraints, rejected alternatives, or non-local dependencies that code cannot express; remove comments that merely restate code.

🤖 Prompt for 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.

In `@packages/cli/src/resolve-args.ts` around lines 114 - 115, Remove the
redundant property comment immediately above minHealth in the parsed arguments
type, leaving the minHealth?: number declaration unchanged.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@packages/cli/src/resolve-args.ts`:
- Around line 114-115: Remove the redundant property comment immediately above
minHealth in the parsed arguments type, leaving the minHealth?: number
declaration unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f4095f2-78fa-41fb-941b-caa0d1f90b32

📥 Commits

Reviewing files that changed from the base of the PR and between 9e0cf9e and 8b1d4f1.

📒 Files selected for processing (4)
  • .changeset/reject-flag-shaped-values.md
  • packages/cli/src/bin.ts
  • packages/cli/src/resolve-args.ts
  • packages/cli/test/resolve-args.test.ts

@oekazuma

oekazuma commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Re: the nitpick on the minHealth property comment — keeping it as is. "when present and valid" states the field's contract (an invalid value yields undefined plus an entry in errors, never NaN), which the type alone doesn't express, and it matches the documentation style of the sibling warnings/errors fields in the same interface.

@oekazuma
oekazuma merged commit ca4ff54 into main Aug 8, 2026
8 checks passed
@oekazuma
oekazuma deleted the advisor/043-string-flag-value-guard branch August 8, 2026 00:44
oekazuma added a commit that referenced this pull request Aug 8, 2026
…7) (#402)

* docs: record the 2026-08-08 audit plans and execution results (043-047)

Adds the five implementation plans produced by the 2026-08-08 deep audit
(commit 9e0cf9e) and updates plans/README.md with the audit's vetted
backlog, rejected findings, direction notes, and the DONE records for
plans 043-047 (shipped as PRs #397-#401).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs: format the audit plan files with oxfmt

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs: address review — fence language and Plan 039 status clarification

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant