fix: maxGroups cap violation in groupFilesHeuristic + surface real errors in validateBranchName - #218
Conversation
…rors in validateBranchName
|
@bh462007 is attempting to deploy a commit to the Gunjan Ghate's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR modifies error handling in ChangesBranch Name Validation Fix
Estimated code review effort: 2 (Simple) | ~15 minutes Split Grouping Heuristic Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
cli/helpers/safeBranchOps.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. cli/helpers/splitLogic.jsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. cli/tests/safeBranchOps.test.jsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/helpers/splitLogic.js (1)
176-268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
maxGroupscan still be exceeded by non-source categories
docs/tests/config/stylesare pushed unconditionally before the source-file budget check runs, sogroups.lengthcan already exceedmaxGroupswhen those categories alone are present. If the contract is “never exceedmaxGroups,” this needs the same cap/merge logic outside the source block too.🤖 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 `@cli/helpers/splitLogic.js` around lines 176 - 268, The grouping logic in splitLogic.js can still exceed maxGroups because the docs/tests/config/styles branches push groups before the source budget check runs. Update the group-building flow in the same function that assembles groups so all categories, not just source files, are capped or merged against maxGroups. Reuse the existing grouping behavior around filesByCategory and groups to either stop adding new groups when the limit is reached or fold extra non-source files into an existing group.
🧹 Nitpick comments (2)
cli/tests/splitLogic.test.js (1)
16-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a case for
remainingSlots === 1.Current test only exercises the
remainingSlots <= 0fold branch (line 229 in splitLogic.js). The broadenedremainingSlots <= 1condition (previously likely=== 1) isn't directly exercised by a scenario whereremainingSlotsequals exactly 1 with multiple source directories.Example additional case
// Test: remainingSlots === 1 with multiple source dirs still merges into one group const filesData2 = { files: [ { path: 'README.md' }, { path: 'test/foo.test.js' }, { path: 'package.json' }, { path: 'a/one.js' }, { path: 'b/two.js' }, ] }; const groups2 = groupFilesHeuristic(filesData2, 4); assert(groups2.length <= 4, `groups2.length (${groups2.length}) should be <= maxGroups (4)`);🤖 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 `@cli/tests/splitLogic.test.js` around lines 16 - 32, Add a test in splitLogic.test.js that specifically exercises the `remainingSlots === 1` path in `groupFilesHeuristic`, since the current case only covers the `remainingSlots <= 0` fold behavior. Create a second scenario with multiple source directories and a `maxGroups` value that leaves exactly one slot after non-source categories are accounted for, then assert the grouping still stays within the limit and the files remain present. Use the existing `groupFilesHeuristic` helper and the current test style so the new case clearly targets the `remainingSlots` branch in splitLogic.js.cli/tests/safeBranchOps.test.js (1)
39-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
'main'assumes repo state.This test assumes a local
mainbranch always exists. If the checkout's default/current branch differs (e.g.,master), this test fails for reasons unrelated to the code under test.🔧 Use the current branch instead of a hardcoded name
-// Test 4: an existing branch (main) returns false -const existsResult = await validateBranchName('main'); -assert(existsResult === false, 'existing branch "main" returns false'); +// Test 4: an existing branch (the current branch) returns false +const { execa } = await import('execa'); +const { stdout: currentBranch } = await execa('git', ['symbolic-ref', '--short', 'HEAD']); +const existsResult = await validateBranchName(currentBranch); +assert(existsResult === false, `existing branch "${currentBranch}" returns false`);🤖 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 `@cli/tests/safeBranchOps.test.js` around lines 39 - 41, The safe branch validation test is hardcoding the branch name and assuming `main` exists in every checkout. Update the `validateBranchName` test case in `safeBranchOps.test.js` to use the current repository branch (or another branch discovered at runtime) instead of the fixed `'main'`, so the assertion remains valid regardless of whether the default branch is `main` or `master`.
🤖 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 `@cli/tests/safeBranchOps.test.js`:
- Around line 16-25: `assertThrows` in `safeBranchOps.test.js` is unused, so the
new `validateBranchName` throw path has no coverage. Add a test that actually
calls `assertThrows` against `validateBranchName` from
`cli/helpers/safeBranchOps.js`, ideally by switching to a temporary non-git
directory (or another real git failure case) and restoring `originalCwd`
afterward. Make sure the new test verifies that unexpected git failures are
surfaced as throws rather than swallowed.
---
Outside diff comments:
In `@cli/helpers/splitLogic.js`:
- Around line 176-268: The grouping logic in splitLogic.js can still exceed
maxGroups because the docs/tests/config/styles branches push groups before the
source budget check runs. Update the group-building flow in the same function
that assembles groups so all categories, not just source files, are capped or
merged against maxGroups. Reuse the existing grouping behavior around
filesByCategory and groups to either stop adding new groups when the limit is
reached or fold extra non-source files into an existing group.
---
Nitpick comments:
In `@cli/tests/safeBranchOps.test.js`:
- Around line 39-41: The safe branch validation test is hardcoding the branch
name and assuming `main` exists in every checkout. Update the
`validateBranchName` test case in `safeBranchOps.test.js` to use the current
repository branch (or another branch discovered at runtime) instead of the fixed
`'main'`, so the assertion remains valid regardless of whether the default
branch is `main` or `master`.
In `@cli/tests/splitLogic.test.js`:
- Around line 16-32: Add a test in splitLogic.test.js that specifically
exercises the `remainingSlots === 1` path in `groupFilesHeuristic`, since the
current case only covers the `remainingSlots <= 0` fold behavior. Create a
second scenario with multiple source directories and a `maxGroups` value that
leaves exactly one slot after non-source categories are accounted for, then
assert the grouping still stays within the limit and the files remain present.
Use the existing `groupFilesHeuristic` helper and the current test style so the
new case clearly targets the `remainingSlots` branch in splitLogic.js.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a86e1ff7-be77-4f1b-9efc-e4b19fc2be13
📒 Files selected for processing (4)
cli/helpers/safeBranchOps.jscli/helpers/splitLogic.jscli/tests/safeBranchOps.test.jscli/tests/splitLogic.test.js
| async function assertThrows(fn, message) { | ||
| try { | ||
| await fn(); | ||
| console.log(`❌ ${message} (expected throw, but none occurred)`); | ||
| failed++; | ||
| } catch (err) { | ||
| console.log(`✅ ${message}`); | ||
| passed++; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
New throw path is untested; assertThrows is dead code.
The PR's core change is that validateBranchName now throws on real git failures (not-a-repo, missing git, permissions), but assertThrows is defined and never invoked anywhere in this file. The new error-throwing behavior added in cli/helpers/safeBranchOps.js ships without coverage.
Consider exercising it, e.g., by running from a temp non-git directory:
// Test 5: unexpected git failures are surfaced, not swallowed
await assertThrows(async () => {
const { execa } = await import('execa');
const tmpDir = await import('node:fs/promises').then(fs => fs.mkdtemp('/tmp/not-a-repo-'));
process.chdir(tmpDir);
try {
await validateBranchName('some-branch');
} finally {
process.chdir(originalCwd);
}
}, 'non-repo directory causes validateBranchName to throw');Want me to draft this test and open a follow-up?
Also applies to: 35-41
🤖 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 `@cli/tests/safeBranchOps.test.js` around lines 16 - 25, `assertThrows` in
`safeBranchOps.test.js` is unused, so the new `validateBranchName` throw path
has no coverage. Add a test that actually calls `assertThrows` against
`validateBranchName` from `cli/helpers/safeBranchOps.js`, ideally by switching
to a temporary non-git directory (or another real git failure case) and
restoring `originalCwd` afterward. Make sure the new test verifies that
unexpected git failures are surfaced as throws rather than swallowed.
There was a problem hiding this comment.
Pull request overview
This PR targets two CLI correctness gaps: enforcing the maxGroups cap in heuristic file grouping (used by gg split fallback behavior) and making validateBranchName() surface real Git execution failures instead of silently treating all errors as “branch available”.
Changes:
- Updates
groupFilesHeuristic()to avoid exceedingmaxGroupswhen non-source categories already consume the group budget. - Updates
validateBranchName()to distinguish “ref not found” from real Git failures and rethrow unexpected errors. - Adds new unit-test scripts for both behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
cli/helpers/splitLogic.js |
Adjusts source grouping logic when the group budget is exhausted. |
cli/helpers/safeBranchOps.js |
Changes error-handling behavior in validateBranchName() and adds docs around it. |
cli/tests/splitLogic.test.js |
Adds a test for maxGroups cap + “no files dropped” behavior. |
cli/tests/safeBranchOps.test.js |
Adds tests for invalid input/characters and “available vs existing” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const allDirs = Object.keys(sourceByDir); | ||
|
|
||
| if (allDirs.length === 1 || remainingSlots === 1) { | ||
| if (remainingSlots <= 0 && groups.length > 0) { |
| } | ||
| } | ||
|
|
||
| /** |
| // git's normal "ref not found" message contains this specific phrase | ||
| if (stderr.includes('not a valid ref')) { | ||
| return true; // genuinely available — ref simply doesn't exist | ||
| } | ||
| // anything else (not a repo, git missing, permissions, etc.) is a real failure | ||
| throw new Error(`Could not validate branch name: ${stderr || error.message}`); |
| // Test 3: a genuinely available name inside this real repo returns true | ||
| const available = await validateBranchName('totally-new-unique-branch-name-xyz-123'); | ||
| assert(available === true, 'available branch name in a real git repo returns true'); | ||
|
|
||
| // Test 4: an existing branch (main) returns false | ||
| const existsResult = await validateBranchName('main'); | ||
| assert(existsResult === false, 'existing branch "main" returns false'); |
| @@ -0,0 +1,38 @@ | |||
| import { groupFilesHeuristic } from '../helpers/splitLogic.js'; | |||
| @@ -0,0 +1,47 @@ | |||
| import { validateBranchName } from '../helpers/safeBranchOps.js'; | |||
|
@bh462007 please add demo or screenshot of working or fixed issue |
|
@gunjanghate Sure! Here's a quick demo showing both fixes working with the exact reproduction steps mentioned in the issues.
I also added dedicated test suites for both fixes ( |
Fixes #217
Summary
This PR addresses the two issues discussed in #217.
1.
groupFilesHeuristic()(cli/helpers/splitLogic.js)Previously,
groupFilesHeuristic()could exceed the requestedmaxGroupslimit when non-source categories (docs/tests/config/styles) had already consumed the available group budget.The root cause was the interaction between
remainingSlotsreaching0and negative array slicing (slice(-1)/slice(0, -1)), which caused the function to create new source groups even though no slots remained. As a result, requestingmaxGroups: 3could still produce 6 groups.Fix
groups.lengthnever exceedsmaxGroups.2.
validateBranchName()(cli/helpers/safeBranchOps.js)Previously,
validateBranchName()caught every error thrown bygit show-refand treated it as "branch name is available."While this worked for the expected "reference not found" case, it also masked genuine failures such as:
PATHThis could cause the function to incorrectly return
trueand continue execution despite an actual failure.Fix
trueonly when the branch genuinely does not exist.Testing
Added tests
cli/tests/splitLogic.test.jsgroups.length <= maxGroupseven when the group budget is exhausted before source files are processed.cli/tests/safeBranchOps.test.jsVerification
npm test) — all 39 existing tests continue to pass with no regressions.groupFilesHeuristic()ignoresmaxGroupswhen category groups already consume all available slots #217.Summary by CodeRabbit
Bug Fixes
Tests