Conversation
Contributor
📝 WalkthroughWalkthroughPer-worktree worktree/branch context and remote awareness added across server and UI: capacity checks, branch-aware auto-mode execution and events, new worktree remote listing route, configurable merge/push targets, drag-and-drop dependency linking, and dialogs for remote/push/merge conflict flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RunRoute as /auto-mode/run-feature
participant AutoMode as AutoModeService
participant FeatureStore
participant Response
Client->>RunRoute: POST /auto-mode/run-feature (projectPath, featureId)
RunRoute->>AutoMode: checkWorktreeCapacity(projectPath, featureId)
AutoMode->>FeatureStore: load feature (featureId)
FeatureStore-->>AutoMode: feature { branchName, ... }
AutoMode->>AutoMode: calculate runningCount & maxAgents for branchName
AutoMode-->>RunRoute: { hasCapacity, currentAgents, maxAgents, branchName }
alt hasCapacity == false
RunRoute-->>Client: 429 { success:false, error, details:{ currentAgents, maxAgents, branchName }}
else
RunRoute->>AutoMode: executeFeature(projectPath, featureId, { branchName })
AutoMode-->>RunRoute: execution started
RunRoute-->>Client: 200 { success:true }
end
sequenceDiagram
participant User
participant UI as MergeWorktreeDialog
participant API
participant ServerRoute as /worktree/merge
participant Git
User->>UI: Confirm merge with targetBranch, options
UI->>API: POST /api/worktree/merge { worktreePath, branchName, targetBranch, options }
API->>ServerRoute: invoke merge handler
ServerRoute->>Git: run git merge (targetBranch)
alt conflicts
Git-->>ServerRoute: exit code 1 (conflict)
ServerRoute-->>API: 409 { hasConflicts: true, message }
API-->>UI: conflict response
UI->>User: show conflict UI -> create-resolve-feature?
else success
Git-->>ServerRoute: merge success
ServerRoute->>Git: optional delete worktree/branch
ServerRoute-->>API: 200 { success, targetBranch, deleted:{worktreeDeleted, branchDeleted} }
API-->>UI: success -> onMerged callback
end
sequenceDiagram
participant User
participant BoardView
participant DragHook as useBoardDragDrop
participant Dialog as DependencyLinkDialog
participant API as HTTP API
User->>BoardView: drag Feature A over Feature B (backlog)
BoardView->>DragHook: handleDragOver(A,B)
DragHook->>DragHook: set pendingDependencyLink { draggedFeature:A, targetFeature:B }
BoardView->>Dialog: open DependencyLinkDialog (pendingDependencyLink)
User->>Dialog: choose "Set as Parent" or "Set as Child"
Dialog->>API: persist feature update (add parent/child)
API-->>Dialog: success
Dialog->>DragHook: clearPendingDependencyLink()
DragHook-->>BoardView: refresh features / show toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/server/src/services/event-hook-service.ts (1)
158-181: LoadedfeatureNameis never used.The feature name is loaded from
featureLoader(Lines 162-165) but is stored in a local variable that is never propagated to thecontextobject. Line 174 still usespayload.featureName, which may be undefined, causing the HTTP hook payload to miss the feature name.🐛 Proposed fix
// Load feature name if we have featureId but no featureName - let featureName: string | undefined = undefined; - if (payload.featureId && payload.projectPath && this.featureLoader) { + let loadedFeatureName: string | undefined = payload.featureName; + if (!loadedFeatureName && payload.featureId && payload.projectPath && this.featureLoader) { try { const feature = await this.featureLoader.get(payload.projectPath, payload.featureId); if (feature?.title) { - featureName = feature.title; + loadedFeatureName = feature.title; } } catch (error) { logger.warn(`Failed to load feature ${payload.featureId} for event hook:`, error); } } // Build context for variable substitution const context: HookContext = { featureId: payload.featureId, - featureName: payload.featureName, + featureName: loadedFeatureName, projectPath: payload.projectPath,apps/server/src/routes/worktree/routes/merge.ts (2)
42-66: Command injection vulnerability via unsanitized branch names.The
branchNameandmergeTo(derived fromtargetBranch) parameters are used directly in shell commands viaexecAsyncwithout validation. An attacker could craft a malicious branch name like; rm -rf /to execute arbitrary commands.The code validates
branchNamewithisValidBranchNameat Line 119 before deletion, but this same validation must be applied before using these values in any shell command.🔒 Proposed fix - validate branch names early
if (!projectPath || !branchName || !worktreePath) { res.status(400).json({ success: false, error: 'projectPath, branchName, and worktreePath are required', }); return; } // Determine the target branch (default to 'main') const mergeTo = targetBranch || 'main'; + // Validate branch names to prevent command injection + if (!isValidBranchName(branchName)) { + res.status(400).json({ + success: false, + error: `Invalid branch name: "${branchName}"`, + }); + return; + } + if (!isValidBranchName(mergeTo)) { + res.status(400).json({ + success: false, + error: `Invalid target branch name: "${mergeTo}"`, + }); + return; + } + // Validate source branch exists try { - await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath }); + await execGitCommand(['rev-parse', '--verify', branchName], projectPath); } catch {Also consider using
execGitCommand(which usesspawnProcess) instead ofexecAsyncfor the merge operations, as it avoids shell interpretation entirely.
91-96: Potential command injection via commit message.The
options?.messageis used directly in the shell command without escaping. Special characters like double quotes, backticks, or$()could break the command or inject shell code.🔒 Proposed fix - use execGitCommand with proper argument separation
// If squash merge, need to commit if (options?.squash) { - await execAsync(`git commit -m "${options?.message || `Merge ${branchName} (squash)`}"`, { - cwd: projectPath, - }); + const commitMessage = options?.message || `Merge ${branchName} (squash)`; + await execGitCommand(['commit', '-m', commitMessage], projectPath); }Similarly, the merge command at Lines 64-66 should use
execGitCommandwith separate arguments:- const mergeCmd = options?.squash - ? `git merge --squash ${branchName}` - : `git merge ${branchName} -m "${options?.message || `Merge ${branchName} into ${mergeTo}`}"`; - - try { - await execAsync(mergeCmd, { cwd: projectPath }); + try { + if (options?.squash) { + await execGitCommand(['merge', '--squash', branchName], projectPath); + } else { + const mergeMessage = options?.message || `Merge ${branchName} into ${mergeTo}`; + await execGitCommand(['merge', branchName, '-m', mergeMessage], projectPath); + }apps/ui/src/hooks/use-auto-mode.ts (1)
499-542: MissinggetMaxConcurrencyForWorktreeinstartcallback dependencies.The
startcallback usesgetMaxConcurrencyForWorktreeat line 515 but doesn't include it in the dependency array at line 542. This could cause stale closures if the function reference changes.Proposed fix
- }, [currentProject, branchName, setAutoModeRunning]); + }, [currentProject, branchName, setAutoModeRunning, getMaxConcurrencyForWorktree]);apps/ui/src/components/views/board-view/kanban-board.tsx (1)
1-11: Missing React imports for VirtualizedList hooks and types.Line 1 only imports
useMemo, butVirtualizedList(lines 113-254) usesuseState,useCallback,useEffect,useRef, and types likeRefObject,UIEvent, andReactNode. This will cause TypeScript errors.Proposed fix
-import { useMemo } from 'react'; +import { useMemo, useState, useCallback, useEffect, useRef, type RefObject, type UIEvent, type ReactNode } from 'react';
🤖 Fix all issues with AI agents
In `@apps/server/src/routes/worktree/routes/merge.ts`:
- Around line 107-115: The fallback path incorrectly sets worktreeDeleted = true
after running execGitCommand(['worktree', 'prune'], ...) even though prune only
cleans stale refs and may not remove the specific worktreePath; change the logic
in the merge route (where execGitCommand, worktreeDeleted and worktreePath are
used) to verify actual removal before marking success—e.g., run the prune, then
check the filesystem for the existence of worktreePath (using fs.existsSync or
fs.stat) or inspect the prune output for the specific path, and only set
worktreeDeleted = true if that verification confirms deletion; otherwise leave
worktreeDeleted false and log a clear warning via logger.warn.
In `@apps/server/src/routes/worktree/routes/push.ts`:
- Around line 38-49: The user-supplied remote is interpolated into a shell
string (remote -> targetRemote) passed to execAsync, allowing command injection;
fix by validating/sanitizing remote (e.g., allow only /^[A-Za-z0-9._-]+$/) and
if it fails, reject or default to 'origin', and/or switch to a safe exec variant
that accepts argument arrays instead of a shell string (use
execFile/child_process.spawn or make execAsync call pass args) when calling
execAsync with targetRemote, branchName, forceFlag and worktreePath to ensure no
shell interpolation of user input.
In `@apps/server/src/services/auto-mode-service.ts`:
- Around line 250-252: The key generation in getWorktreeAutoLoopKey can collide
when a real branch is named "__main__"; change the scheme to avoid collisions by
namespacing real branch names and using an unmistakable sentinel for the
worktree main branch. Update getWorktreeAutoLoopKey to prefix real branches
(e.g., "branch:") and use a distinct sentinel (e.g., "__worktree_main__" or
"worktree:main") for the main/null case so returned keys are unambiguous; keep
the function name getWorktreeAutoLoopKey and update any callers if they rely on
the previous bare "__main__" value.
- Around line 3047-3083: resolveDependencies is being run against
pendingFeatures which treats completed (non-pending) features as missing and
causes valid dependencies to be pruned; update the call in loadPendingFeatures
to compute missingDependencies against the complete persisted feature set (e.g.,
allFeatures or the full features array used elsewhere) instead of
pendingFeatures so only truly absent IDs are considered missing, then continue
to filter and save using feature.dependencies and this.featureLoader.update as
before (refer to resolveDependencies, pendingFeatures, missingDependencies,
orderedFeatures, and featureLoader.update).
- Around line 517-523: The lookup key for per-worktree settings currently builds
`key` using `${projectId}::${branchName ?? '__main__'}`, which misses cases
where branchName === 'main'; change the key construction to use the same
normalization as getWorktreeAutoLoopKey (i.e., call
getWorktreeAutoLoopKey(projectId, branchName) or normalize branchName so 'main'
maps to '__main__') before reading `autoModeByWorktree[key]` so the per-worktree
entry for main is found correctly.
In
`@apps/ui/src/components/views/board-view/components/list-view/list-header.tsx`:
- Around line 36-43: The priority column definition (object with id: 'priority'
in list-header.tsx) uses an empty label which causes the E2E test to fail;
update that column's label from '' to 'Priority' (or the appropriate i18n
key/value used by your app) so the header renders visible text and satisfies the
test assertion that checks data-testid="list-header-priority" contains
"Priority".
In
`@apps/ui/src/components/views/board-view/dialogs/pull-resolve-conflicts-dialog.tsx`:
- Around line 84-101: The effect that auto-selects remote/branch (useEffect
watching remotes, selectedRemote, worktree) doesn't handle the case where the
previously selectedRemote is no longer present; update the logic to detect when
selectedRemote is not found in remotes and then clear/reset selections so the
auto-select runs: in the useEffect, if remotes.length > 0 and (no selectedRemote
OR remotes.find(r => r.name === selectedRemote) is undefined) then call
setSelectedRemote to the default remote.name and setSelectedBranch appropriately
(or clear with setSelectedBranch('') before computing) so stale selections are
reset when remotes refresh.
In `@apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts`:
- Around line 128-143: The current branch-equality check can miss that undefined
(feature on main) and the literal "main" are equivalent; normalize both sides
before comparing by mapping the worktree target branch and
draggedFeature.branchName so that "main" is treated as undefined (or
vice-versa), then perform the equality check and early return; ensure
newBranchName still uses worktreeData.isMain to set undefined for main and
non-main targetBranch for others, and call updateFeature and
persistFeatureUpdate only when the normalized values differ (use identifiers
draggedFeature.branchName, targetBranch, worktreeData.isMain, newBranchName,
updateFeature, persistFeatureUpdate).
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Around line 215-216: Remove the duplicate property assignment for eventHooks
in the object literal created in use-settings-migration (the second occurrence
where eventHooks: state.eventHooks is set); keep the original assignment (the
one at the earlier occurrence) and delete the redundant line so the resulting
GlobalSettings object only assigns eventHooks once, ensuring the type cast to
GlobalSettings['eventHooks'] remains on the retained assignment if needed.
In `@start-automaker.sh`:
- Line 12: Replace the unsafe export $(grep -v '^#' .env | xargs) in
start-automaker.sh with a safe loader: first check that the .env file exists,
then load it in a way that preserves spaces and special characters (e.g., enable
automatic export with set -a and source the file or read lines with IFS and read
to parse KEY=VALUE pairs, ignoring comments and empty lines), and ensure command
substitutions are quoted or avoided so values with spaces/quotes/globs aren’t
split or globbed; update the logic around the current export invocation
accordingly.
- Line 1157: The export line "export $(grep -v '^#' .env | xargs)" suffers from
word-splitting (SC2046), has a stray trailing space, and duplicates the .env
load earlier in the script; either remove this redundant reload or replace it
with the same robust loader used at script start (avoid command substitution
with xargs), e.g., use the proven pattern implemented at line 12 (the script's
initial .env loader) or a safe read loop that uses read -r and explicit exports
to preserve values and quoting; ensure you remove the trailing space after xargs
if keeping the line and make the change where the exact snippet appears so
behavior and quoting match the startup .env handling.
🧹 Nitpick comments (12)
apps/server/src/routes/backlog-plan/routes/apply.ts (1)
88-91: Guard the optional AI ID so it doesn’t clobber auto-generated IDs.
change.feature.idcan beundefinedor empty; passing it unconditionally may override default ID generation or trigger validation errors depending onFeatureLoader.createsemantics. Consider only addingidwhen it’s a non-empty string.✅ Suggested adjustment
- const newFeature = await featureLoader.create(projectPath, { - id: change.feature.id, // Use descriptive ID from AI if provided + const newFeature = await featureLoader.create(projectPath, { + ...(typeof change.feature.id === 'string' && change.feature.id.trim().length > 0 + ? { id: change.feature.id.trim() } + : {}), title: change.feature.title, description: change.feature.description || '', category: change.feature.category || 'Uncategorized', dependencies: change.feature.dependencies,apps/ui/tests/features/list-view-priority.spec.ts (2)
106-111: ReplacewaitForTimeoutwith explicit waits.Using
waitForTimeoutis a flaky test anti-pattern. Prefer waiting for the list view element to be visible immediately after clicking, which you already do on line 111.Proposed fix
// Switch to list view await page.click('[data-testid="view-toggle-list"]'); - await page.waitForTimeout(500); // Verify list view is active await expect(page.locator('[data-testid="list-view"]')).toBeVisible({ timeout: 5000 });
141-156: Replace sorting waits with stable assertions.The
waitForTimeout(300)calls after sorting clicks are prone to flakiness. Instead, wait for the DOM to stabilize by asserting the expected state directly.Proposed fix
// Click on priority header to sort await page.click('[data-testid="list-header-priority"]'); - await page.waitForTimeout(300); // Get all rows within the backlog group and verify they are sorted by priority // (High priority first when sorted ascending by priority value 1, 2, 3) const backlogGroup = page.locator('[data-testid="list-group-backlog"]'); const rows = backlogGroup.locator('[data-testid^="list-row-feature-"]'); // The first row should be high priority (value 1 = lowest number = first in ascending) - const firstRow = rows.first(); - await expect(firstRow).toHaveAttribute('data-testid', 'list-row-feature-high-priority'); + await expect(rows.first()).toHaveAttribute('data-testid', 'list-row-feature-high-priority', { timeout: 5000 }); // Click again to reverse sort (descending - low priority first) await page.click('[data-testid="list-header-priority"]'); - await page.waitForTimeout(300); // Now the first row should be low priority (value 3 = highest number = first in descending) - const firstRowDesc = rows.first(); - await expect(firstRowDesc).toHaveAttribute('data-testid', 'list-row-feature-low-priority'); + await expect(rows.first()).toHaveAttribute('data-testid', 'list-row-feature-low-priority', { timeout: 5000 });apps/server/src/routes/worktree/routes/list-remotes.ts (1)
9-13: Use sharedexecAsyncfromcommon.tsinstead of creating a local duplicate.The
execAsyncutility is already exported from../common.js(seeapps/server/src/routes/worktree/common.tsline 11). Creating a local version duplicates code and could lead to inconsistencies.♻️ Suggested fix
import type { Request, Response } from 'express'; -import { exec } from 'child_process'; -import { promisify } from 'util'; -import { getErrorMessage, logWorktreeError } from '../common.js'; - -const execAsync = promisify(exec); +import { execAsync, getErrorMessage, logWorktreeError } from '../common.js';apps/server/src/routes/worktree/routes/list-branches.ts (1)
9-13: Use sharedexecAsyncfromcommon.tsinstead of creating a local duplicate.Same issue as in
list-remotes.ts- theexecAsyncutility is already exported from../common.js.♻️ Suggested fix
import type { Request, Response } from 'express'; -import { exec } from 'child_process'; -import { promisify } from 'util'; -import { getErrorMessage, logWorktreeError } from '../common.js'; - -const execAsync = promisify(exec); +import { execAsync, getErrorMessage, logWorktreeError } from '../common.js';apps/ui/src/components/views/board-view/dialogs/pull-resolve-conflicts-dialog.tsx (1)
27-33: Reuse shared WorktreeInfo type to avoid drift.This interface matches the shared
@automaker/typesWorktreeInfo; importing it avoids divergence across UI/server.
As per coding guidelines, prefer@automaker/* for shared types.♻️ Suggested change
-import { createLogger } from '@automaker/utils/logger'; +import { createLogger } from '@automaker/utils/logger'; +import type { WorktreeInfo } from '@automaker/types'; @@ -interface WorktreeInfo { - path: string; - branch: string; - isMain: boolean; - hasChanges?: boolean; - changedFilesCount?: number; -}apps/ui/src/components/views/board-view/dialogs/dependency-link-dialog.tsx (2)
35-42: Circular dependency check is shallow — consider transitive cycles.The current check only detects direct existing links (A→B or B→A). If A depends on B and B depends on C, linking C→A would create a cycle that this logic wouldn't catch. For the current drag-and-drop UX this may be acceptable, but consider documenting this limitation or extending the check if cycles become an issue.
93-125: Button labels may confuse users — consider clarifying UX text.The "Set as Parent" button calls
onLink('child')and vice versa. While the code comments explain the intent (thelinkTypedescribes the relationship from the dragged feature's perspective), the UI labeling can be counterintuitive. The inline hints(target depends on this)help, but consider whether the button labels could more directly state the outcome.apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
300-322: Consider handling push errors more gracefully.In
handleConfirmPushToRemote, the error handling only shows a generic toast. For network failures or authentication issues, more specific feedback could help users troubleshoot.apps/ui/src/components/views/board-view/dialogs/push-to-remote-dialog.tsx (2)
52-57: Missing dependency in useEffect.The effect calls
fetchRemotes()but doesn't include it in the dependency array. While this works becausefetchRemotesis stable (only depends onworktree), ESLint's exhaustive-deps rule would flag this. Consider wrappingfetchRemotesinuseCallbackor inlining the logic.
76-105: Extract shared remote-fetching logic to reduce duplication.
fetchRemotesandhandleRefreshcontain nearly identical API call and response handling. Consider extracting the core logic into a shared helper.Suggested approach
const fetchRemotesCore = async (showToast: boolean) => { if (!worktree) return; try { const api = getHttpApiClient(); const result = await api.worktree.listRemotes(worktree.path); if (result.success && result.result) { const remoteInfos: RemoteInfo[] = result.result.remotes.map((r) => ({ name: r.name, url: r.url, })); setRemotes(remoteInfos); if (showToast) toast.success('Remotes refreshed'); if (remoteInfos.length === 0) { setError('No remotes found in this repository. Please add a remote first.'); } } else { const errMsg = result.error || 'Failed to fetch remotes'; if (showToast) toast.error(errMsg); else setError(errMsg); } } catch (err) { logger.error('Failed to fetch remotes:', err); if (showToast) toast.error('Failed to fetch remotes'); else setError('Failed to fetch remotes'); } };Also applies to: 107-133
apps/ui/src/components/views/board-view/dialogs/merge-worktree-dialog.tsx (1)
113-158: Consider extracting duplicate conflict detection logic.The same conflict detection pattern appears twice (lines 116-119 and 140-143). While the contexts differ slightly (API error string vs caught exception), this could be extracted to reduce duplication.
♻️ Optional refactor to extract conflict detection
+ const isConflictError = (message: string): boolean => + message.toLowerCase().includes('conflict') || + message.toLowerCase().includes('merge failed') || + message.includes('CONFLICT'); + const handleMerge = async () => { // ... existing code ... } else { const errorMessage = result.error || ''; - const hasConflicts = - errorMessage.toLowerCase().includes('conflict') || - errorMessage.toLowerCase().includes('merge failed') || - errorMessage.includes('CONFLICT'); + const hasConflicts = isConflictError(errorMessage); // ... rest of handler } } catch (err) { const errorMessage = err instanceof Error ? err.message : 'Unknown error'; - const hasConflicts = - errorMessage.toLowerCase().includes('conflict') || - errorMessage.toLowerCase().includes('merge failed') || - errorMessage.includes('CONFLICT'); + const hasConflicts = isConflictError(errorMessage);
Comment on lines
+107
to
+115
| } catch { | ||
| // Try with prune if remove fails | ||
| try { | ||
| await execGitCommand(['worktree', 'prune'], projectPath); | ||
| worktreeDeleted = true; | ||
| } catch { | ||
| logger.warn(`Failed to remove worktree: ${worktreePath}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prune fallback may incorrectly report worktree deletion success.
git worktree prune removes references to worktrees that no longer exist on disk, but doesn't guarantee the specific worktreePath was removed. Setting worktreeDeleted = true after prune could be misleading if the actual removal failed.
💡 Suggested improvement
} catch {
// Try with prune if remove fails
try {
await execGitCommand(['worktree', 'prune'], projectPath);
- worktreeDeleted = true;
+ // Prune cleans up stale worktree references but doesn't confirm this specific worktree was removed
+ logger.info(`Ran worktree prune after remove failed for: ${worktreePath}`);
} catch {
logger.warn(`Failed to remove worktree: ${worktreePath}`);
}
}📝 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
| } catch { | |
| // Try with prune if remove fails | |
| try { | |
| await execGitCommand(['worktree', 'prune'], projectPath); | |
| worktreeDeleted = true; | |
| } catch { | |
| logger.warn(`Failed to remove worktree: ${worktreePath}`); | |
| } | |
| } | |
| } catch { | |
| // Try with prune if remove fails | |
| try { | |
| await execGitCommand(['worktree', 'prune'], projectPath); | |
| // Prune cleans up stale worktree references but doesn't confirm this specific worktree was removed | |
| logger.info(`Ran worktree prune after remove failed for: ${worktreePath}`); | |
| } catch { | |
| logger.warn(`Failed to remove worktree: ${worktreePath}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/server/src/routes/worktree/routes/merge.ts` around lines 107 - 115, The
fallback path incorrectly sets worktreeDeleted = true after running
execGitCommand(['worktree', 'prune'], ...) even though prune only cleans stale
refs and may not remove the specific worktreePath; change the logic in the merge
route (where execGitCommand, worktreeDeleted and worktreePath are used) to
verify actual removal before marking success—e.g., run the prune, then check the
filesystem for the existence of worktreePath (using fs.existsSync or fs.stat) or
inspect the prune output for the specific path, and only set worktreeDeleted =
true if that verification confirms deletion; otherwise leave worktreeDeleted
false and log a clear warning via logger.warn.
Comment on lines
+38
to
+49
| // Use specified remote or default to 'origin' | ||
| const targetRemote = remote || 'origin'; | ||
|
|
||
| // Push the branch | ||
| const forceFlag = force ? '--force' : ''; | ||
| try { | ||
| await execAsync(`git push -u origin ${branchName} ${forceFlag}`, { | ||
| await execAsync(`git push -u ${targetRemote} ${branchName} ${forceFlag}`, { | ||
| cwd: worktreePath, | ||
| }); | ||
| } catch { | ||
| // Try setting upstream | ||
| await execAsync(`git push --set-upstream origin ${branchName} ${forceFlag}`, { | ||
| await execAsync(`git push --set-upstream ${targetRemote} ${branchName} ${forceFlag}`, { |
Contributor
There was a problem hiding this comment.
Command injection vulnerability via unsanitized remote parameter.
The remote parameter is user-supplied and interpolated directly into shell commands without validation or escaping. An attacker could inject arbitrary shell commands.
For example, if remote is "origin; cat /etc/passwd", the executed command becomes:
git push -u origin; cat /etc/passwd branchName
Validate or sanitize the remote value before use. Git remote names follow specific conventions (alphanumeric, hyphens, underscores, dots).
🔒 Proposed fix: validate remote name
+// Git remote names: alphanumeric, hyphens, underscores, dots, slashes
+const VALID_REMOTE_PATTERN = /^[a-zA-Z0-9_.\-/]+$/;
+
// Use specified remote or default to 'origin'
-const targetRemote = remote || 'origin';
+const targetRemote = remote || 'origin';
+
+if (!VALID_REMOTE_PATTERN.test(targetRemote)) {
+ res.status(400).json({
+ success: false,
+ error: 'Invalid remote name',
+ });
+ return;
+}📝 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
| // Use specified remote or default to 'origin' | |
| const targetRemote = remote || 'origin'; | |
| // Push the branch | |
| const forceFlag = force ? '--force' : ''; | |
| try { | |
| await execAsync(`git push -u origin ${branchName} ${forceFlag}`, { | |
| await execAsync(`git push -u ${targetRemote} ${branchName} ${forceFlag}`, { | |
| cwd: worktreePath, | |
| }); | |
| } catch { | |
| // Try setting upstream | |
| await execAsync(`git push --set-upstream origin ${branchName} ${forceFlag}`, { | |
| await execAsync(`git push --set-upstream ${targetRemote} ${branchName} ${forceFlag}`, { | |
| // Git remote names: alphanumeric, hyphens, underscores, dots, slashes | |
| const VALID_REMOTE_PATTERN = /^[a-zA-Z0-9_.\-/]+$/; | |
| // Use specified remote or default to 'origin' | |
| const targetRemote = remote || 'origin'; | |
| if (!VALID_REMOTE_PATTERN.test(targetRemote)) { | |
| res.status(400).json({ | |
| success: false, | |
| error: 'Invalid remote name', | |
| }); | |
| return; | |
| } | |
| // Push the branch | |
| const forceFlag = force ? '--force' : ''; | |
| try { | |
| await execAsync(`git push -u ${targetRemote} ${branchName} ${forceFlag}`, { | |
| cwd: worktreePath, | |
| }); | |
| } catch { | |
| // Try setting upstream | |
| await execAsync(`git push --set-upstream ${targetRemote} ${branchName} ${forceFlag}`, { |
🤖 Prompt for AI Agents
In `@apps/server/src/routes/worktree/routes/push.ts` around lines 38 - 49, The
user-supplied remote is interpolated into a shell string (remote ->
targetRemote) passed to execAsync, allowing command injection; fix by
validating/sanitizing remote (e.g., allow only /^[A-Za-z0-9._-]+$/) and if it
fails, reject or default to 'origin', and/or switch to a safe exec variant that
accepts argument arrays instead of a shell string (use
execFile/child_process.spawn or make execAsync call pass args) when calling
execAsync with targetRemote, branchName, forceFlag and worktreePath to ensure no
shell interpolation of user input.
Comment on lines
250
to
+252
| function getWorktreeAutoLoopKey(projectPath: string, branchName: string | null): string { | ||
| return `${projectPath}::${branchName ?? '__main__'}`; | ||
| const normalizedBranch = branchName === 'main' ? null : branchName; | ||
| return `${projectPath}::${normalizedBranch ?? '__main__'}`; |
Contributor
There was a problem hiding this comment.
Avoid potential key collision for __main__.
Using __main__ as the sentinel can collide with a real branch named __main__. Consider prefixing real branch keys to keep main worktree distinct.
🔧 Suggested fix
function getWorktreeAutoLoopKey(projectPath: string, branchName: string | null): string {
const normalizedBranch = branchName === 'main' ? null : branchName;
- return `${projectPath}::${normalizedBranch ?? '__main__'}`;
+ const branchKey = normalizedBranch === null ? '__main__' : `branch:${normalizedBranch}`;
+ return `${projectPath}::${branchKey}`;
}📝 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
| function getWorktreeAutoLoopKey(projectPath: string, branchName: string | null): string { | |
| return `${projectPath}::${branchName ?? '__main__'}`; | |
| const normalizedBranch = branchName === 'main' ? null : branchName; | |
| return `${projectPath}::${normalizedBranch ?? '__main__'}`; | |
| function getWorktreeAutoLoopKey(projectPath: string, branchName: string | null): string { | |
| const normalizedBranch = branchName === 'main' ? null : branchName; | |
| const branchKey = normalizedBranch === null ? '__main__' : `branch:${normalizedBranch}`; | |
| return `${projectPath}::${branchKey}`; | |
| } |
🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 250 - 252, The
key generation in getWorktreeAutoLoopKey can collide when a real branch is named
"__main__"; change the scheme to avoid collisions by namespacing real branch
names and using an unmistakable sentinel for the worktree main branch. Update
getWorktreeAutoLoopKey to prefix real branches (e.g., "branch:") and use a
distinct sentinel (e.g., "__worktree_main__" or "worktree:main") for the
main/null case so returned keys are unambiguous; keep the function name
getWorktreeAutoLoopKey and update any callers if they rely on the previous bare
"__main__" value.
Comment on lines
517
to
523
| const projectId = settings.projects?.find((project) => project.path === projectPath)?.id; | ||
| const autoModeByWorktree = (settings as unknown as Record<string, unknown>) | ||
| .autoModeByWorktree; | ||
| const autoModeByWorktree = settings.autoModeByWorktree; | ||
|
|
||
| if (projectId && autoModeByWorktree && typeof autoModeByWorktree === 'object') { | ||
| const key = `${projectId}::${branchName ?? '__main__'}`; | ||
| const entry = (autoModeByWorktree as Record<string, unknown>)[key] as | ||
| | { maxConcurrency?: number } | ||
| | undefined; | ||
| const entry = autoModeByWorktree[key]; | ||
| if (entry && typeof entry.maxConcurrency === 'number') { |
Contributor
There was a problem hiding this comment.
Normalize "main" for per-worktree settings lookup.
autoModeByWorktree keys will miss the main entry if branchName is "main" instead of null. Normalize the same way as getWorktreeAutoLoopKey to avoid falling back to the global limit unexpectedly.
🔧 Suggested fix
- const projectId = settings.projects?.find((project) => project.path === projectPath)?.id;
+ const projectId = settings.projects?.find((project) => project.path === projectPath)?.id;
+ const normalizedBranch = branchName === 'main' ? null : branchName;
const autoModeByWorktree = settings.autoModeByWorktree;
if (projectId && autoModeByWorktree && typeof autoModeByWorktree === 'object') {
- const key = `${projectId}::${branchName ?? '__main__'}`;
+ const key = `${projectId}::${normalizedBranch ?? '__main__'}`;
const entry = autoModeByWorktree[key];📝 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
| const projectId = settings.projects?.find((project) => project.path === projectPath)?.id; | |
| const autoModeByWorktree = (settings as unknown as Record<string, unknown>) | |
| .autoModeByWorktree; | |
| const autoModeByWorktree = settings.autoModeByWorktree; | |
| if (projectId && autoModeByWorktree && typeof autoModeByWorktree === 'object') { | |
| const key = `${projectId}::${branchName ?? '__main__'}`; | |
| const entry = (autoModeByWorktree as Record<string, unknown>)[key] as | |
| | { maxConcurrency?: number } | |
| | undefined; | |
| const entry = autoModeByWorktree[key]; | |
| if (entry && typeof entry.maxConcurrency === 'number') { | |
| const projectId = settings.projects?.find((project) => project.path === projectPath)?.id; | |
| const normalizedBranch = branchName === 'main' ? null : branchName; | |
| const autoModeByWorktree = settings.autoModeByWorktree; | |
| if (projectId && autoModeByWorktree && typeof autoModeByWorktree === 'object') { | |
| const key = `${projectId}::${normalizedBranch ?? '__main__'}`; | |
| const entry = autoModeByWorktree[key]; | |
| if (entry && typeof entry.maxConcurrency === 'number') { |
🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 517 - 523, The
lookup key for per-worktree settings currently builds `key` using
`${projectId}::${branchName ?? '__main__'}`, which misses cases where branchName
=== 'main'; change the key construction to use the same normalization as
getWorktreeAutoLoopKey (i.e., call getWorktreeAutoLoopKey(projectId, branchName)
or normalize branchName so 'main' maps to '__main__') before reading
`autoModeByWorktree[key]` so the per-worktree entry for main is found correctly.
Comment on lines
+3047
to
+3083
| const { orderedFeatures, missingDependencies } = resolveDependencies(pendingFeatures); | ||
|
|
||
| // Remove missing dependencies from features and save them | ||
| // This allows features to proceed when their dependencies have been deleted or don't exist | ||
| if (missingDependencies.size > 0) { | ||
| for (const [featureId, missingDepIds] of missingDependencies) { | ||
| const feature = pendingFeatures.find((f) => f.id === featureId); | ||
| if (feature && feature.dependencies) { | ||
| // Filter out the missing dependency IDs | ||
| const validDependencies = feature.dependencies.filter( | ||
| (depId) => !missingDepIds.includes(depId) | ||
| ); | ||
|
|
||
| logger.warn( | ||
| `[loadPendingFeatures] Feature ${featureId} has missing dependencies: ${missingDepIds.join(', ')}. Removing them automatically.` | ||
| ); | ||
|
|
||
| // Update the feature in memory | ||
| feature.dependencies = validDependencies.length > 0 ? validDependencies : undefined; | ||
|
|
||
| // Save the updated feature to disk | ||
| try { | ||
| await this.featureLoader.update(projectPath, featureId, { | ||
| dependencies: feature.dependencies, | ||
| }); | ||
| logger.info( | ||
| `[loadPendingFeatures] Updated feature ${featureId} - removed missing dependencies` | ||
| ); | ||
| } catch (error) { | ||
| logger.error( | ||
| `[loadPendingFeatures] Failed to save feature ${featureId} after removing missing dependencies:`, | ||
| error | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing-dependency pruning uses the wrong feature set.
resolveDependencies(pendingFeatures) treats dependencies that are completed (and thus not pending) as missing, which will remove valid dependencies from persisted feature data. This can silently corrupt dependency graphs.
✅ Safer approach
- const { orderedFeatures, missingDependencies } = resolveDependencies(pendingFeatures);
+ const {
+ orderedFeatures: orderedAll,
+ missingDependencies,
+ } = resolveDependencies(allFeatures);
+ const orderedFeatures = orderedAll.filter((f) =>
+ pendingFeatures.some((p) => p.id === f.id)
+ );🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 3047 - 3083,
resolveDependencies is being run against pendingFeatures which treats completed
(non-pending) features as missing and causes valid dependencies to be pruned;
update the call in loadPendingFeatures to compute missingDependencies against
the complete persisted feature set (e.g., allFeatures or the full features array
used elsewhere) instead of pendingFeatures so only truly absent IDs are
considered missing, then continue to filter and save using feature.dependencies
and this.featureLoader.update as before (refer to resolveDependencies,
pendingFeatures, missingDependencies, orderedFeatures, and
featureLoader.update).
Comment on lines
+84
to
+101
| // Auto-select default remote and branch when remotes are loaded | ||
| useEffect(() => { | ||
| if (remotes.length > 0 && !selectedRemote) { | ||
| // Default to 'origin' if available, otherwise first remote | ||
| const defaultRemote = remotes.find((r) => r.name === 'origin') || remotes[0]; | ||
| setSelectedRemote(defaultRemote.name); | ||
|
|
||
| // Try to select a matching branch name or default to main/master | ||
| if (defaultRemote.branches.length > 0 && worktree) { | ||
| const matchingBranch = defaultRemote.branches.find((b) => b.name === worktree.branch); | ||
| const mainBranch = defaultRemote.branches.find( | ||
| (b) => b.name === 'main' || b.name === 'master' | ||
| ); | ||
| const defaultBranch = matchingBranch || mainBranch || defaultRemote.branches[0]; | ||
| setSelectedBranch(defaultBranch.fullRef); | ||
| } | ||
| } | ||
| }, [remotes, selectedRemote, worktree]); |
Contributor
There was a problem hiding this comment.
Reset stale selections when remotes refresh.
If the selected remote disappears after a refresh, the selection stays non-empty and the auto-select never re-runs, leaving an invalid selection and empty branch list.
🐛 Suggested guard
useEffect(() => {
+ if (selectedRemote && !remotes.some((r) => r.name === selectedRemote)) {
+ setSelectedRemote('');
+ setSelectedBranch('');
+ return;
+ }
if (remotes.length > 0 && !selectedRemote) {
// Default to 'origin' if available, otherwise first remote
const defaultRemote = remotes.find((r) => r.name === 'origin') || remotes[0];
setSelectedRemote(defaultRemote.name);📝 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
| // Auto-select default remote and branch when remotes are loaded | |
| useEffect(() => { | |
| if (remotes.length > 0 && !selectedRemote) { | |
| // Default to 'origin' if available, otherwise first remote | |
| const defaultRemote = remotes.find((r) => r.name === 'origin') || remotes[0]; | |
| setSelectedRemote(defaultRemote.name); | |
| // Try to select a matching branch name or default to main/master | |
| if (defaultRemote.branches.length > 0 && worktree) { | |
| const matchingBranch = defaultRemote.branches.find((b) => b.name === worktree.branch); | |
| const mainBranch = defaultRemote.branches.find( | |
| (b) => b.name === 'main' || b.name === 'master' | |
| ); | |
| const defaultBranch = matchingBranch || mainBranch || defaultRemote.branches[0]; | |
| setSelectedBranch(defaultBranch.fullRef); | |
| } | |
| } | |
| }, [remotes, selectedRemote, worktree]); | |
| // Auto-select default remote and branch when remotes are loaded | |
| useEffect(() => { | |
| if (selectedRemote && !remotes.some((r) => r.name === selectedRemote)) { | |
| setSelectedRemote(''); | |
| setSelectedBranch(''); | |
| return; | |
| } | |
| if (remotes.length > 0 && !selectedRemote) { | |
| // Default to 'origin' if available, otherwise first remote | |
| const defaultRemote = remotes.find((r) => r.name === 'origin') || remotes[0]; | |
| setSelectedRemote(defaultRemote.name); | |
| // Try to select a matching branch name or default to main/master | |
| if (defaultRemote.branches.length > 0 && worktree) { | |
| const matchingBranch = defaultRemote.branches.find((b) => b.name === worktree.branch); | |
| const mainBranch = defaultRemote.branches.find( | |
| (b) => b.name === 'main' || b.name === 'master' | |
| ); | |
| const defaultBranch = matchingBranch || mainBranch || defaultRemote.branches[0]; | |
| setSelectedBranch(defaultBranch.fullRef); | |
| } | |
| } | |
| }, [remotes, selectedRemote, worktree]); |
🤖 Prompt for AI Agents
In
`@apps/ui/src/components/views/board-view/dialogs/pull-resolve-conflicts-dialog.tsx`
around lines 84 - 101, The effect that auto-selects remote/branch (useEffect
watching remotes, selectedRemote, worktree) doesn't handle the case where the
previously selectedRemote is no longer present; update the logic to detect when
selectedRemote is not found in remotes and then clear/reset selections so the
auto-select runs: in the useEffect, if remotes.length > 0 and (no selectedRemote
OR remotes.find(r => r.name === selectedRemote) is undefined) then call
setSelectedRemote to the default remote.name and setSelectedBranch appropriately
(or clear with setSelectedBranch('') before computing) so stale selections are
reset when remotes refresh.
Comment on lines
+128
to
+143
| const targetBranch = worktreeData.branch; | ||
| const currentBranch = draggedFeature.branchName; | ||
|
|
||
| // If already on the same branch, nothing to do | ||
| if (currentBranch === targetBranch) { | ||
| return; | ||
| } | ||
|
|
||
| // For main worktree, set branchName to undefined/null to indicate it should use main | ||
| // For other worktrees, set branchName to the target branch | ||
| const newBranchName = worktreeData.isMain ? undefined : targetBranch; | ||
|
|
||
| // Update feature's branchName | ||
| updateFeature(featureId, { branchName: newBranchName }); | ||
| await persistFeatureUpdate(featureId, { branchName: newBranchName }); | ||
|
|
Contributor
There was a problem hiding this comment.
Normalize main-branch comparison to avoid redundant moves.
Dropping a feature already on the main worktree can still trigger an update because branchName is often undefined while the target is "main".
🐛 Suggested fix
- const targetBranch = worktreeData.branch;
- const currentBranch = draggedFeature.branchName;
+ const targetBranch = worktreeData.branch;
+ const currentBranch = draggedFeature.branchName ?? null;
+ const normalizedTarget = worktreeData.isMain ? null : targetBranch;
// If already on the same branch, nothing to do
- if (currentBranch === targetBranch) {
+ if (currentBranch === normalizedTarget) {
return;
}
// For main worktree, set branchName to undefined/null to indicate it should use main
// For other worktrees, set branchName to the target branch
- const newBranchName = worktreeData.isMain ? undefined : targetBranch;
+ const newBranchName = normalizedTarget ?? undefined;📝 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
| const targetBranch = worktreeData.branch; | |
| const currentBranch = draggedFeature.branchName; | |
| // If already on the same branch, nothing to do | |
| if (currentBranch === targetBranch) { | |
| return; | |
| } | |
| // For main worktree, set branchName to undefined/null to indicate it should use main | |
| // For other worktrees, set branchName to the target branch | |
| const newBranchName = worktreeData.isMain ? undefined : targetBranch; | |
| // Update feature's branchName | |
| updateFeature(featureId, { branchName: newBranchName }); | |
| await persistFeatureUpdate(featureId, { branchName: newBranchName }); | |
| const targetBranch = worktreeData.branch; | |
| const currentBranch = draggedFeature.branchName ?? null; | |
| const normalizedTarget = worktreeData.isMain ? null : targetBranch; | |
| // If already on the same branch, nothing to do | |
| if (currentBranch === normalizedTarget) { | |
| return; | |
| } | |
| // For main worktree, set branchName to undefined/null to indicate it should use main | |
| // For other worktrees, set branchName to the target branch | |
| const newBranchName = normalizedTarget ?? undefined; | |
| // Update feature's branchName | |
| updateFeature(featureId, { branchName: newBranchName }); | |
| await persistFeatureUpdate(featureId, { branchName: newBranchName }); |
🤖 Prompt for AI Agents
In `@apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts` around
lines 128 - 143, The current branch-equality check can miss that undefined
(feature on main) and the literal "main" are equivalent; normalize both sides
before comparing by mapping the worktree target branch and
draggedFeature.branchName so that "main" is treated as undefined (or
vice-versa), then perform the equality check and early return; ensure
newBranchName still uses worktreeData.isMain to set undefined for main and
non-main targetBranch for others, and call updateFeature and
persistFeatureUpdate only when the normalized values differ (use identifiers
draggedFeature.branchName, targetBranch, worktreeData.isMain, newBranchName,
updateFeature, persistFeatureUpdate).
Comment on lines
+215
to
+216
| // Event hooks | ||
| eventHooks: state.eventHooks as GlobalSettings['eventHooks'], |
Contributor
There was a problem hiding this comment.
Duplicate eventHooks property assignment.
The eventHooks property is already assigned at line 198. This second assignment is redundant and should be removed.
🔧 Proposed fix
activeClaudeApiProfileId:
(state.activeClaudeApiProfileId as GlobalSettings['activeClaudeApiProfileId']) ?? null,
- // Event hooks
- eventHooks: state.eventHooks as GlobalSettings['eventHooks'],
};📝 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
| // Event hooks | |
| eventHooks: state.eventHooks as GlobalSettings['eventHooks'], | |
| activeClaudeApiProfileId: | |
| (state.activeClaudeApiProfileId as GlobalSettings['activeClaudeApiProfileId']) ?? null, | |
| }; |
🤖 Prompt for AI Agents
In `@apps/ui/src/hooks/use-settings-migration.ts` around lines 215 - 216, Remove
the duplicate property assignment for eventHooks in the object literal created
in use-settings-migration (the second occurrence where eventHooks:
state.eventHooks is set); keep the original assignment (the one at the earlier
occurrence) and delete the redundant line so the resulting GlobalSettings object
only assigns eventHooks once, ensuring the type cast to
GlobalSettings['eventHooks'] remains on the retained assignment if needed.
| # CONFIGURATION & CONSTANTS | ||
| # ============================================================================ | ||
|
|
||
| export $(grep -v '^#' .env | xargs) |
Contributor
There was a problem hiding this comment.
Unquoted command substitution and missing file check can cause failures.
This pattern has several issues flagged by shellcheck (SC2046):
- Unquoted
$(...)causes word splitting—values containing spaces or glob characters will break - If
.envdoesn't exist,grepfails and withset -ethe script exits unexpectedly - Values with quotes or special characters won't be parsed correctly
Proposed fix with safer .env loading
-export $(grep -v '^#' .env | xargs)
+if [ -f .env ]; then
+ set -a
+ # shellcheck disable=SC1091
+ source .env
+ set +a
+fiAlternatively, if you need to keep the grep | xargs approach:
-export $(grep -v '^#' .env | xargs)
+if [ -f .env ]; then
+ # shellcheck disable=SC2046
+ export $(grep -v '^#' .env | xargs)
+fi📝 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
| export $(grep -v '^#' .env | xargs) | |
| if [ -f .env ]; then | |
| set -a | |
| # shellcheck disable=SC1091 | |
| source .env | |
| set +a | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 12-12: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In `@start-automaker.sh` at line 12, Replace the unsafe export $(grep -v '^#' .env
| xargs) in start-automaker.sh with a safe loader: first check that the .env
file exists, then load it in a way that preserves spaces and special characters
(e.g., enable automatic export with set -a and source the file or read lines
with IFS and read to parse KEY=VALUE pairs, ignoring comments and empty lines),
and ensure command substitutions are quoted or avoided so values with
spaces/quotes/globs aren’t split or globbed; update the logic around the current
export invocation accordingly.
| # Execute the appropriate command | ||
| case $MODE in | ||
| web) | ||
| export $(grep -v '^#' .env | xargs) |
Contributor
There was a problem hiding this comment.
Same word-splitting issue as line 12; consider consolidating.
This has the same shellcheck SC2046 warning. Additionally, since .env was already loaded at script startup (line 12), this reload may be redundant unless you expect .env to change between script start and reaching this code path.
If the reload is intentional (e.g., to pick up any file changes), apply the same fix as line 12 for consistency. Also note the trailing space after xargs.
Proposed fix
- export $(grep -v '^#' .env | xargs)
+ if [ -f .env ]; then
+ set -a
+ # shellcheck disable=SC1091
+ source .env
+ set +a
+ fi📝 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
| export $(grep -v '^#' .env | xargs) | |
| if [ -f .env ]; then | |
| set -a | |
| # shellcheck disable=SC1091 | |
| source .env | |
| set +a | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 1157-1157: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In `@start-automaker.sh` at line 1157, The export line "export $(grep -v '^#' .env
| xargs)" suffers from word-splitting (SC2046), has a stray trailing space, and
duplicates the .env load earlier in the script; either remove this redundant
reload or replace it with the same robust loader used at script start (avoid
command substitution with xargs), e.g., use the proven pattern implemented at
line 12 (the script's initial .env loader) or a safe read loop that uses read -r
and explicit exports to preserve values and quoting; ensure you remove the
trailing space after xargs if keeping the line and make the change where the
exact snippet appears so behavior and quoting match the startup .env handling.
…s and action items - Introduced a comprehensive security audit document detailing critical command injection vulnerabilities in merge and push handlers, as well as unsafe environment variable handling in a shell script. - Provided recommendations for immediate fixes, including input validation and safer command execution practices. - Highlighted positive security findings and outlined testing recommendations for command injection prevention.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@SECURITY_TODO.md`:
- Around line 3-4: Replace the literal shell expression '$(date)' in the
markdown header line "**Date:** $(date)" with a concrete ISO 8601 timestamp
(e.g., "2026-01-20" or "2026-01-20T14:30:00Z") so the audit record has a stable
provenance; update the same pattern found elsewhere (the occurrences noted
around lines 299-300) to the same explicit timestamp format to keep the document
consistent.
Comment on lines
+3
to
+4
| **Date:** $(date) | ||
| **Audit Type:** Git diff security review against v0.13.0rc branch |
Contributor
There was a problem hiding this comment.
Replace $(date) with a concrete timestamp.
Markdown won’t evaluate $(date), so the audit trail ends up with a literal string. Please set an explicit date/time (e.g., 2026-01-20) to preserve provenance.
💡 Suggested change
-**Date:** $(date)
+**Date:** 2026-01-20
...
-**Last Updated:** $(date)
+**Last Updated:** 2026-01-20Also applies to: 299-300
🤖 Prompt for AI Agents
In `@SECURITY_TODO.md` around lines 3 - 4, Replace the literal shell expression
'$(date)' in the markdown header line "**Date:** $(date)" with a concrete ISO
8601 timestamp (e.g., "2026-01-20" or "2026-01-20T14:30:00Z") so the audit
record has a stable provenance; update the same pattern found elsewhere (the
occurrences noted around lines 299-300) to the same explicit timestamp format to
keep the document consistent.
7 tasks
This was referenced Feb 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.