Skip to content

fix: auto-commit worktree changes and validate commits before PR creation #2006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
octo-patch wants to merge 1 commit into from
Closed

fix: auto-commit worktree changes and validate commits before PR creation #2006

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 83 additions & 8 deletions apps/desktop/src/main/ai/runners/github/pr-creator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,56 @@ Write a professional PR description. Output ONLY the Markdown body — no preamb
}
}

// =============================================================================
// Auto-Commit Uncommitted Changes
// =============================================================================

/**
* Stage and commit any uncommitted changes in the worktree.
* Called before pushing to ensure the branch has commits to push.
* Returns an error string on failure, or undefined on success (or no changes).
*/
function autoCommitWorktreeChanges(
worktreePath: string,
gitPath: string,
specId: string,
): string | undefined {
try {
// Check for uncommitted changes (staged or unstaged, tracked or untracked)
const status = execFileSync(
gitPath,
['status', '--porcelain'],
{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
).trim();

if (!status) {
// No uncommitted changes — nothing to do
return undefined;
}

// 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' },
);
Comment on lines +238 to +250

@coderabbitai coderabbitai Bot Apr 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


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';
Comment on lines +247 to +257

@sentry sentry Bot Apr 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

}
}

// =============================================================================
// Push Branch
// =============================================================================
Expand Down Expand Up @@ -296,7 +346,17 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>
thinkingLevel = 'low',
} = config;

// Step 1: Push the branch to origin
// Strip remote prefix from base branch once — used in all steps below
const effectiveBase = baseBranch.startsWith('origin/')
? baseBranch.slice('origin/'.length)
: baseBranch;

// 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);

@gemini-code-assist gemini-code-assist Bot Apr 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.


// Step 1b: Push the branch to origin
const pushError = pushBranch(worktreePath, gitPath, branchName);
if (pushError) {
// If it looks like the branch is already up-to-date, don't bail
Expand All @@ -307,6 +367,26 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>
}
}

// Step 1c: Verify there is at least one commit ahead of the base branch.
// Without this check, `gh pr create` fails with a confusing GraphQL error.
try {
const commitCount = execFileSync(
gitPath,
['rev-list', '--count', `origin/${effectiveBase}..HEAD`],

@gemini-code-assist gemini-code-assist Bot Apr 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
).trim();
Comment on lines +373 to +377

@gemini-code-assist gemini-code-assist Bot Apr 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

if (commitCount === '0') {
return {
success: false,
error: `No commits found between '${effectiveBase}' and '${branchName}'. ` +
'The agent may not have committed any changes. ' +
'Please verify the implementation completed successfully before creating a PR.',
};
}
} catch {
// Unable to count commits — proceed and let `gh pr create` surface the error
}

// Step 2: Gather context for AI description
const { diffSummary, commitLog } = gatherPRContext(worktreePath, gitPath, baseBranch);
Comment on lines 390 to 391

@coderabbitai coderabbitai Bot Apr 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.


Expand All @@ -324,12 +404,7 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>

const prBody = aiBody || extractSpecSummary(projectDir, specId);

// Step 4: Strip remote prefix from base branch if present
const effectiveBase = baseBranch.startsWith('origin/')
? baseBranch.slice('origin/'.length)
: baseBranch;

// Step 5: Build gh pr create command
// Step 4: Build gh pr create command
const ghArgs = [
'pr', 'create',
'--base', effectiveBase,
Expand All @@ -342,7 +417,7 @@ export async function createPR(config: CreatePRConfig): Promise<CreatePRResult>
ghArgs.push('--draft');
}

// Step 6: Execute gh pr create with retry on network errors
// Step 5: Execute gh pr create with retry on network errors
for (let attempt = 0; attempt < 3; attempt++) {
try {
const output = execFileSync(ghPath, ghArgs, {
Expand Down
Loading