Skip to content

small fixes #261

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

Merged
webdevcody merged 1 commit into from
Dec 24, 2025
Merged

small fixes #261

Show file tree
Hide file tree
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
53 changes: 39 additions & 14 deletions apps/server/src/routes/worktree/routes/create-pr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,32 +56,56 @@ export function createCreatePRHandler() {
}

// Check for uncommitted changes
console.log(`[CreatePR] Checking for uncommitted changes in: ${worktreePath}`);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 24, 2025

Choose a reason for hiding this comment

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

medium

There are several console.log statements added for debugging purposes (lines 59, 65, 67, 75, 79, 83, 95). These should be removed before merging to keep the production logs clean. If this level of logging is desired, consider using a dedicated logger with a debug level.

const { stdout: status } = await execAsync('git status --porcelain', {
cwd: worktreePath,
env: execEnv,
});
const hasChanges = status.trim().length > 0;
console.log(`[CreatePR] Has uncommitted changes: ${hasChanges}`);
if (hasChanges) {
console.log(`[CreatePR] Changed files:\n${status}`);
}

// If there are changes, commit them
let commitHash: string | null = null;
let commitError: string | null = null;
if (hasChanges) {
const message = commitMessage || `Changes from ${branchName}`;
console.log(`[CreatePR] Committing changes with message: ${message}`);

// Stage all changes
await execAsync('git add -A', { cwd: worktreePath, env: execEnv });
try {
// Stage all changes
console.log(`[CreatePR] Running: git add -A`);
await execAsync('git add -A', { cwd: worktreePath, env: execEnv });

// Create commit
await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, {
cwd: worktreePath,
env: execEnv,
});
// Create commit
console.log(`[CreatePR] Running: git commit`);
await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, {
cwd: worktreePath,
env: execEnv,
});
Comment on lines +84 to +87
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Potential command injection vulnerability in commit message.

The commit message escapes only double quotes, but user input could contain other shell metacharacters (e.g., $(...), backticks, \n, $VAR) that may be interpreted by the shell.

Consider using execFile with an array of arguments instead of execAsync with string interpolation to avoid shell interpretation entirely.

🔎 Proposed fix using execFile for safer execution 
+import { execFile } from 'child_process';
+import { promisify } from 'util';
+
+const execFileAsync = promisify(execFile);

// In the commit block:
-          await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, {
-            cwd: worktreePath,
-            env: execEnv,
-          });
+          await execFileAsync('git', ['commit', '-m', message], {
+            cwd: worktreePath,
+            env: execEnv,
+          });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, {
cwd: worktreePath,
env: execEnv,
});
import { execFile } from 'child_process';
import { promisify } from 'util';
const execFileAsync = promisify(execFile);
Suggested change
await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, {
cwd: worktreePath,
env: execEnv,
});
await execFileAsync('git', ['commit', '-m', message], {
cwd: worktreePath,
env: execEnv,
});
🤖 Prompt for AI Agents 
In apps/server/src/routes/worktree/routes/create-pr.ts around lines 84 to 87,
the current git commit uses execAsync with an interpolated string which only
escapes quotes and allows shell metacharacters to be interpreted; replace this
with a shell-less call (e.g., execFile/spawn with args) so the commit message is
passed as a single argument to git commit (["commit", "-m", message]) and set
cwd and env on the child process; ensure the message is not concatenated into a
shell string, preserve existing cwd/env handling, and handle the returned
promise/errors as before.


// Get commit hash
const { stdout: hashOutput } = await execAsync('git rev-parse HEAD', {
cwd: worktreePath,
env: execEnv,
});
commitHash = hashOutput.trim().substring(0, 8);
// Get commit hash
const { stdout: hashOutput } = await execAsync('git rev-parse HEAD', {
cwd: worktreePath,
env: execEnv,
});
commitHash = hashOutput.trim().substring(0, 8);
console.log(`[CreatePR] Commit successful: ${commitHash}`);
} catch (commitErr: unknown) {
const err = commitErr as { stderr?: string; message?: string };
commitError = err.stderr || err.message || 'Commit failed';
console.error(`[CreatePR] Commit failed: ${commitError}`);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 24, 2025

Choose a reason for hiding this comment

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

medium

For consistency with the error handling in the rest of the file (see line 401), please use the imported logError function instead of console.error.

Suggested change
console.error(`[CreatePR] Commit failed: ${commitError}`);
logError(commitErr, 'Commit failed');


// Return error immediately - don't proceed with push/PR if commit fails
res.status(500).json({
success: false,
error: `Failed to commit changes: ${commitError}`,
commitError,
});
return;
}
}

// Push the branch to remote
Expand Down Expand Up @@ -360,8 +384,9 @@ export function createCreatePRHandler() {
success: true,
result: {
branch: branchName,
committed: hasChanges,
committed: hasChanges && !commitError,
commitHash,
commitError: commitError || undefined,
pushed: true,
prUrl,
prNumber,
Expand Down
2 changes: 1 addition & 1 deletion apps/ui/src/components/views/board-view/dialogs/archive-all-verified-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export function ArchiveAllVerifiedDialog({
</Button>
<Button variant="default" onClick={onConfirm} data-testid="confirm-archive-all-verified">
<Archive className="w-4 h-4 mr-2" />
Archive All
Complete All
</Button>
Comment on lines 46 to 49
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 24, 2025

Choose a reason for hiding this comment

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

medium

The button text has been changed to 'Complete All', but the data-testid on the parent <Button> component still refers to 'archive'. To maintain consistency and prevent potential issues with tests, please update the test ID to match the new action.

Suggested change
<Button variant="default" onClick={onConfirm} data-testid="confirm-archive-all-verified">
<Archive className="w-4 h-4 mr-2" />
Archive All
Complete All
</Button>
<Button variant="default" onClick={onConfirm} data-testid="confirm-complete-all-verified">
<Archive className="w-4 h-4 mr-2" />
Complete All
</Button>

</DialogFooter>
</DialogContent>
Expand Down
2 changes: 1 addition & 1 deletion apps/ui/src/components/views/board-view/kanban-board.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export function KanbanBoard({
data-testid="archive-all-verified-button"
>
<Archive className="w-3 h-3 mr-1" />
Archive All
Complete All
</Button>
Comment on lines 117 to 121
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 24, 2025

Choose a reason for hiding this comment

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

medium

The button text has been changed to 'Complete All', but the data-testid on the parent <Button> component still refers to 'archive'. To maintain consistency and prevent potential issues with tests, please update the test ID to match the new action.

Suggested change
data-testid="archive-all-verified-button"
>
<Archive className="w-3 h-3 mr-1" />
Archive All
Complete All
</Button>
data-testid="complete-all-verified-button"
>
<Archive className="w-3 h-3 mr-1" />
Complete All
</Button>

) : column.id === 'backlog' ? (
<div className="flex items-center gap-1">
Expand Down
Loading