Repository navigation
fix(cli): reject flags as --base branch values - #1431
vianmangal wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Required value validation and regression tests src/cli/args.ts, tests/cli/base-args.test.ts |
Both --base forms use a helper that rejects undefined, empty, and hyphen-prefixed values. Tests cover invalid values, valid branch names, and preserving a following --offline option. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Low
Suggested reviewers: sonukapoor
Merge Risk: 
4e8f6
-
The --base validation change is mergeable with no identified current-head risk.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: rejecting flag-shaped values for the --base branch argument. Description check The description includes the required change rationale and issue reference. It also provides implementation details and validation results. Linked Issues check Issue #1430requires rejection of missing, empty, and flag-shaped--basevalues before scanning.src/cli/args.tsnow appliesrequireFlagValueto both spaced and equals forms. The guard rejects `…Out of Scope Changes check The pull request changes only src/cli/args.tsand adds focused parser tests intests/cli/base-args.test.ts. These changes directly implement and verify issue#1430. No unrelated production behavio…Full details: Linked Issues check
-
Explanation
Issue
#1430requires rejection of missing, empty, and flag-shaped--basevalues before scanning.src/cli/args.tsnow appliesrequireFlagValueto both spaced and equals forms. The guard rejectsundefined, empty strings, and values starting with-, and reports--base requires a branch name argument.tests/cli/base-args.test.tscovers both forms, long and short flags, and valid branch names followed by--offline.
Full details: Out of Scope Changes check
-
Explanation
The pull request changes only
src/cli/args.tsand adds focused parser tests intests/cli/base-args.test.ts. These changes directly implement and verify issue#1430. No unrelated production behavior or unrelated files are changed.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
|
Add Carrot credits or activate Agent usage billing to use Autofix |
There was a problem hiding this comment.
🟢 Approval recommended
Both parsing paths enforce the intended validation, and focused regression coverage addresses the reported bug with no blocking issues identified.
No open issues 🎉
What changed in this PR
This PR fixes --base argument parsing so malformed branch values are rejected instead of consuming a following flag such as --offline.
Changes:
- Adds required-value validation to both
--base valueand--base=value. - Adds 11 regression cases covering invalid values and valid branches that preserve
--offline.
| File | Description |
|---|---|
| tests/cli/base-args.test.ts | Tests rejected values and valid branches in both argument forms. |
| src/cli/args.ts | Adds and applies the required-value guard to --base. |
Review effort: Balanced
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks, nice first PR, and the tests catch it: reverting the fix fails four of them, and --base main --offline still keeps offline on.
One thing on ordering: requireFlagValue is also arriving with #1382 and #1392, so this will land after those and should use that copy rather than adding its own. I'll check it again once they're in.
What changed and why
--base --offlinewas accepted with--offlinestored as the branch name, silently disabling offline mode. The--base=--offlineform also accepted a flag as its branch value.Both forms now use required-value validation to reject missing, empty, or dash-prefixed branch values before scanning. Valid branches, including names with slashes and hyphens, preserve a following
--offlineflag. The guard uses the samerequireFlagValueimplementation as #1382, applied to--basehere.Closes #1430
Validation
npm ci --no-audit --no-fundnpm run lint:testsnpm run buildnode dist/index.js advisories syncnpm test: 184 suites and 2,519 tests passed.node scripts/typecheck-changed-tests.mjs origin/main: the new test file passed.git diff origin/main HEAD --checkThe 11 new parser cases cover both forms, missing and empty values, long and short flags, and valid branch values followed by
--offline. Four regression cases failed on the original code and pass with this fix. Both malformed CLI invocations now exit 1 with--base requires a branch name argument.The older
src/docs/CONTRIBUTING.mdmentionsnpm run format, but currentmainhas no format script or Prettier configuration. This patch follows the existing style and passes the whitespace check.Summary by CodeRabbit
--baseoption now rejects missing, empty, or flag-like values and reports that a branch name is required.=syntax.