fix: auto-commit worktree changes and validate commits before PR creation#2006
fix: auto-commit worktree changes and validate commits before PR creation#2006octo-patch wants to merge 1 commit intoAndyMik90:developfrom
Conversation
…tion (fixes AndyMik90#1928) When the agent writes files to the worktree but does not call `git commit`, the subsequent `gh pr create` fails with a cryptic GraphQL error: 'Head sha can\'t be blank, Base sha can\'t be blank, No commits between main and <branch>'. Fix: - Auto-stage and commit any uncommitted changes in the worktree before pushing (handles agents that skip the commit step) - After pushing, verify at least one commit exists ahead of the base branch and return a clear, actionable error message if none are found
|
|
📝 WalkthroughWalkthroughEnhanced PR creation process with automatic git change handling and pre-validation. Added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| gitPath, | ||
| ['commit', '-m', `auto-claude: implement ${specId}`], | ||
| { cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' }, | ||
| ); | ||
|
|
||
| return undefined; | ||
| } catch (err: unknown) { | ||
| const stderr = err instanceof Error && 'stderr' in err | ||
| ? String((err as NodeJS.ErrnoException & { stderr?: string }).stderr) | ||
| : String(err); | ||
| return stderr || 'Auto-commit failed'; |
There was a problem hiding this comment.
Bug: The autoCommitWorktreeChanges function silently fails if git user identity is not configured. The error is caught but ignored, leading to a misleading downstream error message.
Severity: MEDIUM
Suggested Fix
At the call site for autoCommitWorktreeChanges (around line 357), log the error if the function returns one. This will make the root cause (e.g., missing git identity) visible during debugging, instead of being masked by a misleading downstream error message.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/desktop/src/main/ai/runners/github/pr-creator.ts#L246-L257
Potential issue: The `autoCommitWorktreeChanges` function attempts to commit staged
files but can fail if a git user identity is not configured in the environment. This is
common in CI/CD or containerized environments. The function catches the error from the
failed `git commit` command and returns it. However, the caller at line 357 explicitly
ignores this return value and does not log the error, leading to a silent failure.
Consequently, a downstream validation check incorrectly reports a misleading error, "No
commits found...", which masks the true root cause of the problem.
Did we get this right? 👍 / 👎 to inform future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts`:
- Around line 238-250: The PR currently stages all files using
execFileSync(gitPath, ['add', '.'], { cwd: worktreePath, ... }) which can
accidentally include unintended or sensitive files; change this to a targeted
staging approach by using either git add -u to stage only tracked changes or by
generating an explicit list of paths to add (e.g., parse git status or your
expected output list) and call execFileSync with those specific paths; update
the code that calls execFileSync for staging (the git add invocation referencing
gitPath and worktreePath) to implement one of these safer strategies and ensure
the commit message logic that uses specId remains unchanged.
- Around line 390-391: The call to gatherPRContext is using baseBranch (which
can contain an origin/ prefix) instead of the normalized effectiveBase, causing
double-prefixing; update the invocation to pass effectiveBase (i.e., call
gatherPRContext(worktreePath, gitPath, effectiveBase)) so the function receives
the already-normalized branch name and avoids trying "origin/origin/...". Ensure
you change the argument at the gatherPRContext call and keep worktreePath and
gitPath unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2f05d49-fa57-4aa9-884a-f0e81a9b3630
📒 Files selected for processing (1)
apps/desktop/src/main/ai/runners/github/pr-creator.ts
| // Stage all changes | ||
| execFileSync( | ||
| gitPath, | ||
| ['add', '.'], | ||
| { cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' }, | ||
| ); | ||
|
|
||
| // Commit with a descriptive message | ||
| execFileSync( | ||
| gitPath, | ||
| ['commit', '-m', `auto-claude: implement ${specId}`], | ||
| { cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' }, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider security implications of staging all files.
Using git add . stages all unignored files in the worktree, which could include files the agent created that weren't intended for commit (e.g., debug files, downloaded dependencies, or files with sensitive data that aren't in .gitignore).
Consider a more targeted approach using git add -u (staged only tracked files) or parsing the status output to selectively stage expected file patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts` around lines 238 -
250, The PR currently stages all files using execFileSync(gitPath, ['add', '.'],
{ cwd: worktreePath, ... }) which can accidentally include unintended or
sensitive files; change this to a targeted staging approach by using either git
add -u to stage only tracked changes or by generating an explicit list of paths
to add (e.g., parse git status or your expected output list) and call
execFileSync with those specific paths; update the code that calls execFileSync
for staging (the git add invocation referencing gitPath and worktreePath) to
implement one of these safer strategies and ensure the commit message logic that
uses specId remains unchanged.
| // Step 2: Gather context for AI description | ||
| const { diffSummary, commitLog } = gatherPRContext(worktreePath, gitPath, baseBranch); |
There was a problem hiding this comment.
Inconsistent use of baseBranch vs effectiveBase.
This call passes the original baseBranch (which may include origin/ prefix), while line 375 correctly uses effectiveBase. The gatherPRContext function prepends origin/ internally (lines 103, 122), so if baseBranch is "origin/main", it attempts "origin/origin/main" before falling back.
Pass effectiveBase for consistency:
🔧 Proposed fix
// Step 2: Gather context for AI description
- const { diffSummary, commitLog } = gatherPRContext(worktreePath, gitPath, baseBranch);
+ const { diffSummary, commitLog } = gatherPRContext(worktreePath, gitPath, effectiveBase);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts` around lines 390 -
391, The call to gatherPRContext is using baseBranch (which can contain an
origin/ prefix) instead of the normalized effectiveBase, causing
double-prefixing; update the invocation to pass effectiveBase (i.e., call
gatherPRContext(worktreePath, gitPath, effectiveBase)) so the function receives
the already-normalized branch name and avoids trying "origin/origin/...". Ensure
you change the argument at the gatherPRContext call and keep worktreePath and
gitPath unchanged.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a mechanism to automatically commit uncommitted changes and verifies that the branch is ahead of the base before creating a PR. Key feedback includes transitioning from synchronous to asynchronous process execution to prevent UI hangs, improving error reporting for the auto-commit step, and making the remote name configurable instead of hardcoding 'origin'.
| const commitCount = execFileSync( | ||
| gitPath, | ||
| ['rev-list', '--count', `origin/${effectiveBase}..HEAD`], | ||
| { cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' }, | ||
| ).trim(); |
There was a problem hiding this comment.
The execFileSync call is synchronous and will block the main thread while counting commits. In a large repository or over a slow network (if origin needs to be resolved), this could cause the UI to hang. Since createPR is already an async function, it is better to use an asynchronous execution method like execFile from node:child_process wrapped in a promise, or a similar utility if available in the project.
| // Step 1a: Auto-commit any uncommitted changes in the worktree before pushing. | ||
| // This handles the case where the agent wrote files but didn't run `git commit`. | ||
| // A failure here is non-fatal — we still attempt the push in case prior commits exist. | ||
| autoCommitWorktreeChanges(worktreePath, gitPath, specId); |
There was a problem hiding this comment.
The return value of autoCommitWorktreeChanges is ignored. While the comment states that failure is non-fatal, if the auto-commit fails due to a configuration issue (e.g., user.email not set), the user won't receive any feedback until the later commit count check fails. It would be better to at least log the error or provide a warning if the function returns an error string.
| try { | ||
| const commitCount = execFileSync( | ||
| gitPath, | ||
| ['rev-list', '--count', `origin/${effectiveBase}..HEAD`], |
There was a problem hiding this comment.
The commit count check hardcodes the remote name as origin. While this is consistent with the rest of the file (e.g., pushBranch), it will fail if the user has configured a different remote name for their upstream. Consider making the remote name configurable or detecting it from the git configuration.
2 participants
Fixes #1928
Problem
When the AI agent writes files to the worktree but does not explicitly run
git add+git commit, clicking Create PR in the UI fails with acryptic GitHub GraphQL error:
The branch is pushed successfully, but because it has no commits ahead of
the base branch, GitHub refuses to create the PR.
Solution
Two changes to
pr-creator.ts:Auto-commit before pushing — Before calling
git push, check foruncommitted changes in the worktree (
git status --porcelain). If anyexist, stage them with
git add .and commit them with a descriptivemessage (
auto-claude: implement <specId>). This handles the common casewhere the agent wrote files but skipped the commit step.
Validate commit count after pushing — After the push succeeds, run
git rev-list --count origin/<base>..HEADto verify at least one commitexists ahead of the base. If none are found, return a clear, actionable
error message instead of letting
gh pr createfail with a confusingGraphQL error.
Testing
creation now auto-commits and succeeds.
human-readable error explaining that the agent may not have committed
any changes.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores