feat: Add parallel execution infrastructure for concurrent AI agent workflows#26
feat: Add parallel execution infrastructure for concurrent AI agent workflows#26anthonymq wants to merge 20 commits intosubsy:mainfrom
Conversation
Implement a resource-aware worktree pool manager for parallel agent execution: - WorktreePoolConfig interface with maxWorktrees, minFreeMemoryMB, maxCpuUtilization, worktreeDir, and cleanupOnSuccess properties - System resource monitoring (CPU and memory) before spawning worktrees - Worktrees created in .worktrees/ directory with descriptive names - Automatic cleanup immediately after successful merge - Pool respects configurable maximum worktrees (default: 4) - Event system for monitoring worktree lifecycle
… communication - Implement AgentMessage interface with types: discovery, warning, blocker, complete, failed - Add AgentStatus tracking: idle, working, blocked, complete, failed - Create message broker with pub/sub mechanism and subscription filters - Include message payload with category, summary, details, and affectedFiles - Add heartbeat-based agent timeout detection - Support broadcast and targeted message delivery - Export Coordinator and types from worktree module US-002: Coordinator Process with Message Passing
- Add ResourceLock interface tracking resource name, holder agent ID, read/write mode, acquired time, and timeout - Implement ResourceLockManager with readers-writer lock pattern - Add lock timeout expiration to prevent deadlocks - Implement deadlock detection via wait graph cycle detection - Add shared read-only cache with isolated writes per worktree - Pause worktree creation when resources are exhausted - Export new types and manager from worktree module
…dentification - Add task-graph-types.ts with interfaces for graph analysis (GraphTask, ParallelWorkUnit, ParallelizationAnalysis, BvPlanOutput, BvInsightsOutput) - Add TaskGraphAnalyzer class that integrates with bd/bv CLI tools - Uses bv --robot-plan for parallel execution tracks identification - Uses bv --robot-insights for PageRank, betweenness, critical path metrics - Groups independent tasks into parallelizable work units - Logs parallelization reasoning for transparency - Integrates with bd CLI for task status updates (in_progress, closed) - Falls back to basic dependency analysis when bv unavailable - Export new modules from worktree/index.ts Implements US-004: Integration with bd/bv for Task Graph Analysis
Implement configuration flag to disable auto-parallelization: - Add ParallelConfig interface with enabled, maxWorktrees, resourceLimits, worktreeDir, autoCleanup, and backupBranchPrefix options - Add ResourceLimitsConfig for memory/CPU thresholds - Default parallel.enabled to true for backwards compatibility - Add Zod schemas for validation (ParallelConfigSchema, ResourceLimitsConfigSchema) - Implement config merging in config/index.ts with nested resourceLimits support - Add --no-parallel CLI flag to override config and force sequential execution - Update help text in both cli.tsx and run.tsx This allows developers to disable auto-parallelization when predictable sequential execution is required.
…nd rollback - Add merge-engine-types.ts with configuration, state, and event types - Implement MergeEngine class with: - Automatic backup branch creation (pre-parallel-merge/<timestamp>) - Sequential merge with conflict detection - Reflog-based rollback to pre-merge state - Backup branch management (list, delete, cleanup old backups) - Export from worktree/index.ts Acceptance criteria met: - System merges all worktrees back to the original branch upon completion - Automatic backup branch is created before merge - Sequential merge with conflict detection is implemented - Reflog-based rollback to pre-merge state is supported
Implements US-007 with: - ConflictResolutionConfig with autoResolve and confidenceThreshold options - ConflictResolver class that analyzes conflicts and generates resolutions - Confidence scoring based on hunk analysis and semantic similarity - User prompt callback for low-confidence resolutions - Integration with MergeEngine for automatic resolution during merges - New merge statuses: conflict_resolved, conflict_pending_user - 85% success rate target via confidence threshold (default 0.8)
Implement US-008: Agent Broadcast System that enables parallel agents to broadcast discoveries (bugs, patterns, blockers) in real-time. Key features: - BroadcastManager class for managing broadcasts across agents - Configurable broadcast categories (default: bug, pattern, blocker) - Enable/disable broadcasting via configuration - Broadcasts include affected files for context - Agents can consume broadcasts and adjust work accordingly - Priority-based broadcast handling (low/normal/high/critical) - Subscription system for real-time notifications - Relevance scoring based on file overlap and priority - Action suggestions (stop/adjust/review/continue/acknowledge)
- Add ParallelExecutor for running multiple tasks concurrently - Continue executing other tasks when one fails - Collect all failures and report at completion - Preserve failed worktree state for debugging - Clear failure attribution showing which agent/task failed - Generate failure reports with summary and detailed logs per agent - Export types and executor from worktree module
- Add ParallelProgressSummary component for summary view of all streams - Add WorkStreamCard showing agent ID, task name, progress, activity - Add WorkStreamDrillDown for detailed view with broadcast messages - Add ParallelProgressView with keyboard navigation (↑↓ navigate, Enter drill-down, Esc back) - Add 'w' shortcut to access parallel work streams view - Include real-time progress indicators per stream - Display inter-agent messages and broadcasts in drill-down view - Export new components and types from TUI module
- Add merge-progress-types.ts with display types and helper functions - Add MergeProgressView component for monitoring merge phase - Add WorktreeMergeCard for individual worktree status display - Add ConflictResolutionPanel for conflict details and user actions - Support keyboard shortcuts: accept/reject AI resolution, use ours/theirs - Display backup branch name prominently - Show confidence levels for AI resolution suggestions
- Add preserveMergeAttemptBranch option to RollbackOptions for debugging - Extend RollbackResult with preservedBranch and preservedSha fields - Update MergeEngine.rollback() to create debug branch before reset - Add PostMergeValidationResult and validation events - Add RollbackPromptChoice type for user interaction - Create RollbackPromptPanel TUI component for validation failure prompt - Integrate rollback prompt mode into MergeProgressView - Support rollback, rollback with debug preservation, continue anyway
- Add parallel session persistence (parallel-session.json) - Implement orphaned worktree detection on restart - Add crash recovery for stale in-progress agents - Extend lock file with parallel mode fields - Session pause/resume/recovery for parallel execution
… (US-014) - Create ParallelAgentRunner to bridge parallel executor with AgentPlugin system - Update ParallelExecutor to use actual agents (claude, opencode) instead of placeholder - Add agent configuration options to ParallelExecutorConfig (agentId, model, tracing) - Integrate SubagentTraceParser for real-time subagent event tracking - Add task_output and subagent_event types to ParallelExecutorEvent - Export ParallelAgentRunner and related types from worktree module
…te (US-015) - Add Parallel Execution Mode section to README with architecture diagram - Document all parallel execution configuration options with examples - Document keyboard shortcuts for parallel mode (P, m, M, R, w, c, f) - Add test infrastructure with bun test runner - Create unit tests for WorktreePoolManager (pool management, events, lifecycle) - Create unit tests for Coordinator (agent registration, messaging, subscriptions) - Create unit tests for MergeEngine (session management, rollback, backup branches) - Create integration tests for end-to-end parallel execution flow - All 66 tests passing, typecheck and build verified
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis change implements comprehensive parallel execution infrastructure for concurrent AI agent workflows, adding worktree pooling, agent coordination via message passing, resource locking, AI-assisted merge conflict resolution, broadcast discovery, and TUI progress monitoring components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI/Config
participant TA as TaskGraphAnalyzer
participant WPM as WorktreePoolManager
participant PE as ParallelExecutor
participant Agent as Agent
participant ME as MergeEngine
participant CR as ConflictResolver
User->>CLI: Invoke with --parallel flag
CLI->>TA: analyse() to identify parallelizable tasks
TA-->>CLI: ParallelWorkUnits with task graph
PE->>WPM: initialize() create worktree pool
WPM-->>PE: ready with N worktrees
PE->>PE: execute(workUnits) start concurrent execution
loop For each WorkUnit in parallel
PE->>WPM: acquire(options) get worktree
WPM-->>PE: ManagedWorktree
PE->>Agent: run(task, worktree) execute agent
Agent->>Agent: process task in isolated worktree
Agent-->>PE: result with output/error
PE->>WPM: release(worktreeId) return worktree
WPM-->>PE: ok
end
PE->>ME: mergeAll(worktrees) consolidate changes
ME->>ME: checkout target branch
ME->>ME: create backup branch
loop For each worktree
ME->>ME: merge branch into target
alt Conflicts detected
ME->>CR: resolveConflicts(files)
CR->>CR: parse hunks and analyse
CR-->>ME: resolutions with confidence
ME->>CR: emit resolution events
end
ME-->>PE: WorktreeMergeResult
end
ME->>ME: emit session_completed
PE-->>User: ParallelExecutionResult with stats
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Fix all issues with AI agents
In @package.json:
- Line 56: The @types/bun entry in package.json is pinned to ^1.3.5 which
mismatches the Bun runtime; update the @types/bun dependency (where it appears
in package.json, likely under devDependencies) to "1.2.23" to match the 1.2.x
runtime, or alternatively bump the packageManager field in package.json to a Bun
1.3.x release so the runtime and types align; ensure you update only the version
string for the @types/bun dependency or the packageManager value accordingly and
run install to verify no new type/runtime mismatches.
In @README.md:
- Line 831: Change the verb "rollback" to the two-word form "roll back" in the
sentence that reads "After merge, you can rollback to pre-merge state if needed"
so it becomes "After merge, you can roll back to the pre-merge state if needed";
update the sentence exactly where that phrase appears in the README to correct
the grammar.
In @src/tui/components/ConflictResolutionPanel.tsx:
- Around line 31-43: ConflictResolutionPanel currently destructures all callback
props with underscore prefixes and never uses them, so keyboard hints do
nothing; fix by removing the underscore prefixes (use onAccept, onReject,
onUseOurs, onUseTheirs, onManualResolve, onAbortAll, onSelectConflict, onBack)
and register a keyboard handler (e.g., with useKeyboard inside
ConflictResolutionPanel) that maps keys to these callbacks: 'a' →
onAccept(selectedIndex) if suggestion exists, 'o' → onUseOurs(selectedIndex),
't' → onUseTheirs(selectedIndex), 'm' → onManualResolve(selectedIndex), 'r' →
onReject(selectedIndex), arrow keys / 'j'/'k' → onSelectConflict(newIndex)
bounded by conflicts.length, and 'escape' → onBack(); ensure handler
dependencies include selectedIndex, selectedConflict, conflicts.length and the
callback refs so keyboard navigation triggers the provided props.
In @src/tui/components/RollbackPromptPanel.tsx:
- Around line 112-161: The UI shows "Press any key to continue..." when
rollbackResult is set but there is no handler wired for that state; add a new
prop (e.g., onDismissResult: () => void) to RollbackPromptPanel and call it from
the existing keyboard handler (e.g., handleKey / onKeyDown) whenever
rollbackResult is truthy (regardless of isRollingBack) so any keypress
clears/dismisses the result; update the component signature to accept
onDismissResult, ensure the handler checks rollbackResult first and invokes
onDismissResult(), and add a noop default or required prop handling where the
component is used.
In @src/tui/components/WorktreeMergeCard.tsx:
- Around line 21-25: The WorktreeMergeCard component destructures props but
never invokes the onSelect callback from WorktreeMergeCardProps; wire onSelect
into the card's selection handler by including onSelect in the destructured
props for WorktreeMergeCard and calling it when the card is activated (e.g.,
bind onSelect to the card's click/selection event handler used by your UI
framework), passing any expected parameters (such as event or the item id) and
preserving existing isSelected/compact behavior so parent components receive
selection events.
In @src/worktree/broadcast-manager.ts:
- Around line 242-249: Fix the typo: rename the variable and returned property
from requireingAction to requiringAction in src/worktree/broadcast-manager.ts
(the local computed const and the property in the returned object) and update
the corresponding property name in the broadcast type definition in
src/worktree/broadcast-types.ts so the types and implementation match.
In @src/worktree/broadcast-types.ts:
- Around line 236-238: The field name requireingAction in the Broadcast-related
type is misspelled; rename the property to requiringAction everywhere it appears
(type/interface definition in broadcast-types.ts and any usages), updating any
imports, references, serialization keys, and tests to use requiringAction so the
property name is consistent and correctly spelled.
In @src/worktree/lock-manager.ts:
- Around line 80-86: Add a new failure reason (e.g., MaxLocksExceeded or
agent_max_locks_exceeded) to the LockAcquisitionFailureReason enum/type in
lock-types.ts, then update the check in LockManager.acquireLock (the block that
computes agentLockCount and compares to this.config.maxLocksPerAgent) to return
success: false with the new failure reason instead of 'agent_not_registered';
ensure any switch/case or tests that pattern-match on
LockAcquisitionFailureReason are updated to handle the new value.
- Around line 211-244: The catch in initializeWorktreeCache currently swallows
copy errors and forces cacheState.initialized = true; change it to preserve
failure visibility by logging the caught error (use this.logger.error or
console.error if no logger exists), do not set cacheState.initialized = true on
failure (leave it false), and optionally emit a failure event (e.g.,
this.emit('cacheInitFailed', { worktreeId, error })) so callers can observe the
failure; keep the sharedCachePath, cacheState, and copyOnWrite logic intact but
make failure observable instead of silently marking initialized.
In @src/worktree/manager.ts:
- Around line 149-184: The acquire method has a TOCTOU race around
activeWorktreeCount; add a private pendingAcquisitions number field to the class
and change acquire to check activeWorktreeCount + pendingAcquisitions against
this.config.maxWorktrees, increment pendingAcquisitions immediately after
passing that check, and decrement it in a finally block so concurrent acquires
cannot over-allocate; keep the existing emits and resource checks and ensure you
decrement pendingAcquisitions whether createWorktree(options) succeeds or
throws.
- Around line 107-147: The empty catch in syncWithGit swallows failures from
execGit so the manager can silently end up with an inconsistent this.worktrees
map; update syncWithGit to catch the error and either log it (using the existing
logger or emit an event) and preserve/restore previous state or rethrow so
callers can handle it; specifically, in the catch block for syncWithGit (which
calls execGit(['worktree','list','--porcelain'], this.projectRoot)) ensure you
record the error details (e.g., via this.logger.error or this.emit('error',
err)) and avoid silently returning an empty worktrees map—add error logging and
consider keeping the existing this.worktrees untouched on failure or rethrow the
error for higher-level handling.
- Around line 250-255: The checkout+merge sequence in cleanup() (when
options.mergeBefore is true) mutates the main repo HEAD and must be serialized;
add a shared mutex/lock (e.g., a class-level Promise/Mutex like this.mergeLock
or this.checkoutMergeMutex) around the block that calls getDefaultBranch(),
execGit(['checkout', ...]) and execGit(['merge', ...]) so concurrent cleanup()
calls wait; ensure cleanupAll() and any public cleanup() entry points
acquire/release this lock before changing worktree.status to 'merging' and
performing checkout/merge to prevent race conditions.
In @src/worktree/parallel-executor.ts:
- Around line 486-502: The FailureAttribution construction uses the non-null
assertion result.error! which can be undefined for failures (e.g., timeouts);
update the failedResults.map in parallel-executor.ts to safely handle missing
errors by replacing result.error! with a fallback like result.error ?? new
Error('Unknown failure') or a descriptive string, ensuring
FailureAttribution.error is always populated; keep the rest of the mapping
unchanged and reference the failures: FailureAttribution, failedResults.map, and
result.error when making the change.
- Around line 646-649: The dispose method currently calls shutdown() and
agentRunner.dispose() but never invokes cleanupAll(), leaving worktrees
uncleaned; update ParallelExecutor.dispose to call await this.cleanupAll() (or
this.cleanupAll() if synchronous) before or after shutdown as appropriate so all
remaining worktrees are cleaned up, ensuring the call is awaited and handles
errors (e.g., try/catch or propagating) and reference the dispose, shutdown,
cleanupAll, and agentRunner.dispose symbols when making the change.
In @src/worktree/task-graph-analyzer.ts:
- Around line 550-552: getLastDataHash() returns this.lastDataHash but
lastDataHash is never set; update the analyzeWithBv method (where
analysisResult.dataHash is computed) to assign this.lastDataHash =
analysisResult.dataHash after a successful analysis so getLastDataHash() returns
the latest value, and ensure any early returns or error paths do not leave
lastDataHash stale.
In @tests/unit/merge-engine.test.ts:
- Around line 71-81: The test name and assertion conflict: the spec currently
registers a listener via mergeEngine.addEventListener(listener) and then calls
mergeEngine.mergeAll(...) expecting the listener NOT to have been called, so
update the test title to match the behavior (e.g., change the test description
string in the test(...) from "should emit session_started event" to "should not
emit events for empty worktrees") so the name reflects what is being asserted;
keep the existing calls to addEventListener, mergeEngine.mergeAll, and
expect(listener).not.toHaveBeenCalled().
In @tsconfig.json:
- Line 8: The TS build changed because tsconfig.json's "rootDir" was set to ".",
causing declaration output to land under dist/src; update package.json entry
points so "types" points to "./dist/src/index.d.ts", update "exports" types path
to "./dist/src/index.d.ts", ensure "main" stays "./dist/index.js" and "bin"
points to "./dist/cli.js", and confirm your build:types script aligns with these
paths; alternatively revert tsconfig.json "rootDir" back to "./src" and remove
tests/**/* from "include" so declarations emit to dist/index.d.ts and
dist/cli.d.ts as originally expected.
🧹 Nitpick comments (45)
src/worktree/parallel-agent-runner.ts (1)
271-291: Consider adding agents to results even when unavailable.When
createInstancereturnsnull(line 279), the agent is silently omitted from the results map. Consumers might expect all registered plugins to appear in the results, even if unavailable.🔧 Suggested improvement
for (const meta of registry.getRegisteredPlugins()) { const instance = registry.createInstance(meta.id); if (instance) { const detectResult = await instance.detect(); results.set(meta.id, { available: detectResult.available, version: detectResult.version, error: detectResult.error, }); await instance.dispose(); + } else { + results.set(meta.id, { + available: false, + error: 'Failed to create agent instance', + }); } }src/worktree/lock-manager.ts (1)
616-639: Consider making contention thresholds configurable.The magic numbers
10(line 621) for wait queue threshold and3(line 633) for blocked agents threshold are hardcoded. These could be made configurable viaResourceLockManagerConfigfor different deployment scenarios.src/tui/merge-progress-types.ts (1)
374-382: Consider handling sub-second durations.The
formatMergeDurationfunction rounds to seconds, so durations under 1 second display as "0s". For fast merges, showing milliseconds might be more informative.🔧 Optional enhancement
export function formatMergeDuration(ms: number): string { + if (ms < 1000) { + return `${ms}ms`; + } const seconds = Math.floor(ms / 1000); const minutes = Math.floor(seconds / 60); if (minutes > 0) { return `${minutes}m ${seconds % 60}s`; } return `${seconds}s`; }src/tui/components/WorkStreamCard.tsx (2)
21-26: Consider clamping progress percentage to prevent overflow.If
percentexceeds 100,Math.round((percent / 100) * width)could produce a value greater thanwidth, resulting inemptybeing negative. While'░'.repeat(negative)produces an empty string in JavaScript, this could lead to a progress bar longer than intended.🔧 Suggested fix
function createProgressBar(percent: number, width: number): string { if (percent < 0) return '░'.repeat(width); - const filled = Math.round((percent / 100) * width); - const empty = width - filled; + const clampedPercent = Math.min(percent, 100); + const filled = Math.round((clampedPercent / 100) * width); + const empty = Math.max(0, width - filled); return '█'.repeat(filled) + '░'.repeat(empty); }
28-32: TheonSelectcallback fromWorkStreamCardPropsis not implemented in this component.The
WorkStreamCardPropsinterface defines an optionalonSelect?: () => voidcallback, butWorkStreamCarddoes not destructure or invoke it. The component uses theisSelectedprop only for visual styling (background and border colours). If interactivity is intended—either for click-based selection or alternative input patterns—consider implementing the callback. Otherwise, this unused prop could be removed from the interface.tests/unit/worktree-pool-manager.test.ts (2)
156-172: Configuration tests do not clean up temp directories.Unlike the main describe block which has
afterEachcleanup, these standalone tests create temp directories (lines 158, 165) that are never removed. While these are intmpdir()and will eventually be cleaned by the OS, it would be cleaner to either:
- Add cleanup to these tests
- Move them into the main describe block to benefit from the existing
afterEachcleanup🧹 Suggested fix - add cleanup
describe('WorktreePoolManager - Configuration', () => { + const tempDirs: string[] = []; + + afterEach(async () => { + for (const dir of tempDirs) { + try { + await rm(dir, { recursive: true, force: true }); + } catch { + /* cleanup errors ignored */ + } + } + tempDirs.length = 0; + }); + test('should use default config when not provided', () => { const tempDir = join(tmpdir(), 'ralph-test-default'); + tempDirs.push(tempDir); const manager = new WorktreePoolManager(tempDir); expect(manager.maxWorktrees).toBe(DEFAULT_WORKTREE_POOL_CONFIG.maxWorktrees); }); test('should merge custom config with defaults', () => { const tempDir = join(tmpdir(), 'ralph-test-custom'); + tempDirs.push(tempDir); const manager = new WorktreePoolManager(tempDir, { maxWorktrees: 8, }); expect(manager.maxWorktrees).toBe(8); }); });
38-61: Add unit tests for the happy-path worktree lifecycle.The unit tests currently cover edge cases (non-existent worktrees, pool exhaustion, disabled cleanup), but do not test the happy path. Whilst integration tests exist, they do not explicitly exercise the acquire → use → release → cleanup lifecycle. Add unit tests that verify successful worktree acquisition, usage, and release to ensure the core happy path works as expected.
README.md (1)
662-704: Consider adding a language identifier to the fenced code block.The ASCII diagram block lacks a language specifier. While this is optional for diagrams, adding
textorplaintextsatisfies linting rules and improves accessibility for screen readers.📝 Suggested fix
-``` +```text ┌─────────────────────────────────────────────────────────────────────────────┐tests/integration/parallel-execution.test.ts (2)
71-89: Consider adding assertions for manager initialisation.The test verifies agent registration and status updates work but doesn't assert that
manager.initialize()succeeded or verify any manager state. Consider adding an assertion after initialisation to confirm the manager is in the expected state.💡 Suggested enhancement
await manager.initialize(); coordinator.start(); + // Verify manager initialised successfully + expect(manager.activeWorktreeCount).toBe(0); + const agent = coordinator.registerAgent('agent-1', 'Test Agent');
56-67: Test verifies listener management but could be clearer about intent.The test adds and immediately removes a listener, then asserts it wasn't called. This confirms the mechanics work but the assertion is trivially true since no events were emitted. Consider documenting the test's purpose or adding a scenario where events would be emitted if the listener weren't removed.
src/tui/components/index.ts (1)
37-41: Re-export Props types for consistency with the established pattern.The three new components (
ParallelProgressSummary,WorkStreamCard,WorkStreamDrillDown) have props types defined inparallel-progress-types.js(ParallelProgressSummaryProps,WorkStreamCardProps,WorkStreamDrillDownProps), but these are not re-exported in the barrel file. However, the established pattern in this file is to re-export both the component and its Props type—as demonstrated by all other components, includingParallelProgressViewwhich is exported in the same change set. Add the missing type exports to maintain API consistency:export type { ParallelProgressSummaryProps } from '../parallel-progress-types.js'; export type { WorkStreamCardProps } from '../parallel-progress-types.js'; export type { WorkStreamDrillDownProps } from '../parallel-progress-types.js';src/tui/components/ConflictResolutionPanel.tsx (1)
149-155: Avoid duplicate function calls for the same value.
getConfidenceDisplay(selectedConflict.confidence)is called twice on consecutive lines. Consider caching the result.♻️ Suggested fix
+ const confidenceDisplay = getConfidenceDisplay(selectedConflict.confidence); <box style={{ flexDirection: 'row' }}> <text fg={colors.fg.muted}>Confidence: </text> - <text fg={getConfidenceDisplay(selectedConflict.confidence).color}> - {getConfidenceDisplay(selectedConflict.confidence).label} + <text fg={confidenceDisplay.color}> + {confidenceDisplay.label} {' '}({Math.round(selectedConflict.confidence * 100)}%) </text> </box>src/tui/components/RollbackPromptPanel.tsx (1)
28-53:optionsarray in dependency array causes unnecessary re-renders.The
optionsarray is defined inside the component and recreated on every render. Including it inhandleKeyboard's dependency array (line 107) means the callback is recreated every render, defeating the purpose ofuseCallback.Move
optionsoutside the component or memoize it.♻️ Suggested fix
+const OPTIONS: { key: SelectedOption; label: string; description: string; shortcut: string }[] = [ + { + key: 'rollback', + label: 'Rollback to pre-merge state', + description: 'Reset to backup branch, discard merge attempt', + shortcut: '1', + }, + { + key: 'rollback_debug', + label: 'Rollback & preserve for debugging', + description: 'Reset but keep failed merge on a debug branch', + shortcut: '2', + }, + { + key: 'continue', + label: 'Continue anyway', + description: 'Keep the merge despite validation failure', + shortcut: '3', + }, + { + key: 'abort', + label: 'Abort', + description: 'Stop merge process without changes', + shortcut: 'q', + }, +]; + export function RollbackPromptPanel({ // ... }: RollbackPromptPanelProps): ReactNode { const [selectedOption, setSelectedOption] = useState<SelectedOption>('rollback'); - const options: { key: SelectedOption; label: string; description: string; shortcut: string }[] = [ - // ... remove from here - ]; const handleKeyboard = useCallback( (key: { name: string }) => { if (isRollingBack) return; - const optionKeys = options.map(o => o.key); + const optionKeys = OPTIONS.map(o => o.key); // ... }, - [selectedOption, handleSelect, isRollingBack, options] + [selectedOption, handleSelect, isRollingBack] );Also applies to: 107-107
tests/unit/coordinator.test.ts (1)
135-143: Clarify test description for broadcast behaviour.The test description says "should send broadcast message to all agents" but
recipientCountis 1 with 2 agents registered. If broadcasts exclude the sender (which seems correct), consider updating the description to clarify this: "should send broadcast message to all other agents".♻️ Suggested improvement
- test('should send broadcast message to all agents', () => { + test('should send broadcast message to all other agents (excluding sender)', () => { coordinator.registerAgent('agent-1', 'Agent 1'); coordinator.registerAgent('agent-2', 'Agent 2'); const result = coordinator.send('agent-1', 'discovery', createTestPayload()); expect(result.success).toBe(true); - expect(result.recipientCount).toBe(1); + expect(result.recipientCount).toBe(1); // Excludes sender agent-1 });src/worktree/resources.ts (1)
9-9: Module-level mutable state may cause testing issues.
lastCpuInfois a module-level variable that persists across function calls. This could cause issues in tests where you need deterministic behaviour or want to reset state between test runs.Consider either exposing a reset function for testing or documenting this behaviour.
♻️ Optional: Add reset function for testing
let lastCpuInfo: { idle: number; total: number } | null = null; +/** + * Reset CPU info cache. Primarily for testing purposes. + * @internal + */ +export function _resetCpuInfoCache(): void { + lastCpuInfo = null; +}src/session/lock.ts (1)
334-352: Silent failure inupdateLockParallelModemay hide issues.If the lock file doesn't exist (line 340-342), the function silently returns without any indication. This could mask bugs where the lock was unexpectedly released or never acquired.
Consider either throwing an error or returning a boolean to indicate success.
♻️ Suggested improvement
export async function updateLockParallelMode( cwd: string, isParallelMode: boolean, activeWorktreeCount: number -): Promise<void> { +): Promise<boolean> { const lock = await readLockFile(cwd); if (!lock) { - return; + return false; } const updatedLock: LockFile = { ...lock, isParallelMode, activeWorktreeCount, }; const lockPath = getLockPath(cwd); await writeFile(lockPath, JSON.stringify(updatedLock, null, 2)); + return true; }src/worktree/conflict-resolver.ts (2)
75-82: Silent catch in event emission could hide important errors.The empty catch block in
emit()swallows all listener errors without logging. If a listener throws, the error is silently ignored, which could make debugging difficult.Consider at minimum logging the error, or re-throwing after all listeners are notified.
♻️ Suggested improvement
private emit(event: ConflictResolverEvent): void { for (const listener of this.listeners) { try { listener(event); - } catch { + } catch (error) { + // Log but don't propagate to avoid breaking event emission to other listeners + console.error('[ConflictResolver] Event listener error:', error); } } }
222-254: Malformed conflict markers could leave parsing in inconsistent state.If a file has an opening conflict marker (
<<<<<<<) but no closing marker (>>>>>>>),inConflictremains true at loop end and the partial hunk is never added tohunks. This is likely acceptable (returns empty hunks), but the incomplete hunk data is silently discarded.Consider adding a warning when parsing ends with
inConflictstill true.♻️ Suggested improvement
for (let i = 0; i < lines.length; i++) { // ... existing parsing logic } + + if (inConflict) { + // File has unclosed conflict marker - this is malformed + console.warn(`[ConflictResolver] Unclosed conflict marker in ${filePath}`); + } return { filePath, hunks, // ... };src/tui/components/ParallelProgressView.tsx (2)
104-106: Edge case:selectedIndexcould become -1 when streams array is empty.If
streams.lengthis 0, thenstreams.length - 1is -1, andMath.min(prev + 1, -1)could result in a negative index. The component does handle the empty state at line 162, but the keyboard handler could still be invoked before the render path is reached.Consider adding a guard:
Suggested fix
case 'down': case 'j': - setSelectedIndex(prev => Math.min(streams.length - 1, prev + 1)); + setSelectedIndex(prev => Math.min(Math.max(0, streams.length - 1), prev + 1)); break;
53-59: Output accumulation may produce leading newline.When
event.stream.outputPreviewis appended, the concatenation usescurrent + '\n' + event.stream.outputPreview. Ifcurrentis empty (initial state), this produces a leading newline character.Consider:
Suggested fix
const current = next.get(event.stream.id) || ''; - next.set(event.stream.id, current + '\n' + event.stream.outputPreview); + const separator = current ? '\n' : ''; + next.set(event.stream.id, current + separator + event.stream.outputPreview);src/worktree/task-graph-types.ts (1)
185-192:process.cwd()evaluated at module load time may not reflect intended working directory.The
workingDirdefault is set toprocess.cwd()when the module is first imported, which may differ from the actual working directory at runtime if the module is imported early in the application lifecycle.Consider making this a function or using a sentinel value that triggers runtime resolution:
Suggested approach
export const DEFAULT_TASK_GRAPH_ANALYZER_CONFIG: TaskGraphAnalyzerConfig = { - workingDir: process.cwd(), + workingDir: '', // Empty string indicates "use current working directory at runtime" maxParallelUnits: 4, minTasksPerUnit: 1, maxTasksPerUnit: 5, useBvAnalysis: true, bvTimeoutMs: 30000, };Then in the analyser, resolve empty string to
process.cwd()at execution time.src/tui/components/WorkStreamDrillDown.tsx (1)
74-76: Unused callback props.The
onBackandonAcknowledgeBroadcastprops are destructured with underscore prefixes indicating they're intentionally unused. However, the footer displays keyboard hints for these actions ("Esc Back to summary", "a Acknowledge broadcast").This appears intentional since
ParallelProgressViewhandles keyboard events centrally, but consider either:
- Removing the underscore prefixes and wiring the callbacks to local key handlers, or
- Adding a brief comment explaining that keyboard handling is delegated to the parent.
src/worktree/merge-engine.ts (7)
27-53: DuplicateexecGithelper function.This helper is duplicated verbatim in
src/worktree/manager.ts. Consider extracting it to a shared utility module (e.g.,src/worktree/git-utils.ts) to avoid code duplication and centralise any future improvements like timeout handling.
81-88: Silent error swallowing in event emission.The empty catch block silently ignores listener errors. Whilst this prevents a faulty listener from breaking the merge flow, consider logging the error at a debug level for observability.
Proposed improvement
for (const listener of this.listeners) { try { listener(event); - } catch { + } catch (error) { + // Log listener errors at debug level for observability + // console.debug('Event listener error:', error); } }
249-254: Silent catch blocks when deleting worktree branches.Empty catch blocks at lines 252-253 and 335-336 silently swallow branch deletion errors. If the branch is protected or doesn't exist, this could mask issues. Consider logging a warning or emitting an event.
Also applies to: 332-337
328-329: Magic number for AI resolution success threshold.The threshold
0.85for accepting AI resolutions is a magic number. Consider extracting this to theMergeEngineConfigorconflictResolutionconfig to make it configurable and self-documenting.Proposed improvement
- if (result.success && result.stats.successRate >= 0.85) { + const threshold = config.conflictResolution?.confidenceThreshold ?? 0.85; + if (result.success && result.stats.successRate >= threshold) {
384-389: Silent abort merge failure.If
git merge --abortfails (e.g., no merge in progress or git lock issues), the error is silently swallowed. This could leave the repository in an unexpected state.
431-437: Inconsistent indentation in the for loop.The for loop body at lines 432-436 has inconsistent indentation compared to the surrounding code.
Fix indentation
const branches = await this.getWorktreeBranches(); for (const branch of branches) { - try { - await execGit(['branch', '-D', branch], this.config.projectRoot); - deletedBranches.push(branch); - } catch { - } + try { + await execGit(['branch', '-D', branch], this.config.projectRoot); + deletedBranches.push(branch); + } catch { + } }
265-289: Conflict detection should use exit code or unmerged file checks instead of error message string matching.The conflict detection at line 265 relies on case-insensitive string matching that may miss conflicts if the error message format varies across git versions or locales. Git's merge command returns exit code 1 on conflict, which is more reliable. Consider either extracting and checking the exit code from the error message, or unconditionally calling
getConflictingFiles()after merge failure and treating any unmerged files as a conflict. This approach would be more robust than relying on the presence of a "conflict" keyword in the error message.src/config/schema.ts (1)
92-101: Type inconsistency betweenenabledCategoriesandBroadcastCategorySchema.
enabledCategoriesat line 94 usesz.array(z.string())butBroadcastCategorySchema(lines 79-90) defines a specific enum of valid categories. Consider using the enum schema for stronger type safety:Proposed fix
export const BroadcastConfigSchema = z.object({ enabled: z.boolean().optional(), - enabledCategories: z.array(z.string().min(1)).optional(), + enabledCategories: z.array(BroadcastCategorySchema).optional(), maxBroadcastHistory: z.number().int().min(1).max(10000).optional(), broadcastTtlMs: z.number().int().min(1000).max(86400000).optional(), autoConsume: z.boolean().optional(), cleanupIntervalMs: z.number().int().min(1000).max(3600000).optional(), requireAckForCritical: z.boolean().optional(), - customCategories: z.array(z.string().min(1)).optional(), + customCategories: z.array(z.string().min(1)).optional(), // Keep as string for user-defined categories });src/tui/parallel-progress-types.ts (1)
323-335: Missing handling for negative duration values.The
formatStreamDurationfunction doesn't handle negative millisecond values, which could occur if clocks are skewed or timestamps are incorrect. Consider adding a guard.Proposed improvement
export function formatStreamDuration(ms: number): string { + if (ms < 0) { + return '0s'; + } const seconds = Math.floor(ms / 1000); const minutes = Math.floor(seconds / 60); const hours = Math.floor(minutes / 60);src/config/index.ts (1)
642-643: Long export lines reduce readability.The type and default exports on lines 642-643 are quite long. Consider splitting them across multiple lines for better readability and easier diff reviews.
Proposed formatting
-export type { StoredConfig, RalphConfig, RuntimeOptions, ConfigValidationResult, SubagentDetailLevel, NotificationSoundMode, ParallelConfig, ResourceLimitsConfig, ConflictResolutionConfig } from './types.js'; -export { DEFAULT_CONFIG, DEFAULT_PARALLEL_CONFIG, DEFAULT_CONFLICT_RESOLUTION_CONFIG } from './types.js'; +export type { + StoredConfig, + RalphConfig, + RuntimeOptions, + ConfigValidationResult, + SubagentDetailLevel, + NotificationSoundMode, + ParallelConfig, + ResourceLimitsConfig, + ConflictResolutionConfig, +} from './types.js'; +export { + DEFAULT_CONFIG, + DEFAULT_PARALLEL_CONFIG, + DEFAULT_CONFLICT_RESOLUTION_CONFIG, +} from './types.js';src/tui/components/MergeProgressView.tsx (1)
231-245: Empty function fallbacks for callbacks.The fallback
() => {}foronRollback,onRollbackPreserveDebug,onContinueAnyway, andonAbortat lines 237-240 could be avoided by making these props required inRollbackPromptPanelif they're always needed, or by using optional chaining if the panel handles undefined callbacks gracefully.src/worktree/broadcast-manager.ts (3)
84-87: Direct mutation of config object.
updateConfigusesObject.assignto mutate the internal config directly. If any caller holds a reference to the config obtained viagetConfig(), they won't see updates (sincegetConfigreturns a shallow copy). However, this could cause subtle bugs ifthis.configis passed elsewhere. Consider creating a new config object instead.Safer approach
updateConfig(updates: Partial<BroadcastConfig>): void { - Object.assign(this.config, updates); + (this as { config: BroadcastConfig }).config = { ...this.config, ...updates }; this.emit({ type: 'config_changed', config: { ...this.config } }); }
367-387: File overlap detection may produce false positives.The relevance calculation at lines 373-374 uses
f.includes(wf) || wf.includes(f)for file matching. This could produce false positives, e.g.,src/utils.tswould matchsrc/utils.tsxor evenutils. Consider using path comparison or exact matching with directory awareness.More precise matching
const overlap = broadcast.payload.affectedFiles.filter((f) => - workingFiles.some((wf) => wf === f || f.includes(wf) || wf.includes(f)) + workingFiles.some((wf) => { + if (wf === f) return true; + // Check if files share the same directory or one is a parent path + const fParts = f.split('/'); + const wfParts = wf.split('/'); + // Match if in same directory or file is in a subdirectory + return fParts.slice(0, -1).join('/') === wfParts.slice(0, -1).join('/'); + }) );
472-479: Silent swallowing of subscription callback errors.Similar to the merge-engine, subscription callbacks that throw errors are silently ignored. Consider logging these for debugging purposes.
src/worktree/manager.ts (2)
24-50: DuplicateexecGithelper - same as merge-engine.ts.As noted earlier, this function is duplicated. Extract to a shared module.
257-257: Empty string in git args array.At line 257, when
options.forceis false, an empty string is passed to the git command. Whilefilter(Boolean)removes it, this pattern is slightly confusing. Consider a cleaner approach.Cleaner approach
- await execGit(['worktree', 'remove', worktree.path, options.force ? '--force' : ''].filter(Boolean), this.projectRoot); + const args = ['worktree', 'remove', worktree.path]; + if (options.force) args.push('--force'); + await execGit(args, this.projectRoot);src/worktree/coordinator.ts (2)
59-76: Consider validating agent ID uniqueness on registration.
registerAgentsilently overwrites any existing agent with the same ID. This could mask bugs where the same ID is registered twice accidentally.💡 Optional: Add duplicate detection
registerAgent(id: string, name: string, worktreeId?: string, taskId?: string): TrackedAgent { + if (this.agents.has(id)) { + console.warn(`Agent ${id} already registered, overwriting`); + } const now = new Date(); const agent: TrackedAgent = {
263-271: Consider logging callback errors for debugging.Subscription callback errors are silently swallowed, which is correct for broker resilience but may make debugging difficult.
💡 Optional: Add debug logging for callback errors
for (const sub of matchingSubs) { try { const result = sub.callback(message); if (result instanceof Promise) { - result.catch(() => {}); + result.catch((err) => { + // Debug: console.error(`Subscription callback error for ${sub.id}:`, err); + }); } - } catch { + } catch (err) { + // Debug: console.error(`Subscription callback error for ${sub.id}:`, err); } }src/worktree/parallel-executor.ts (1)
65-73: Consider dependency injection for testability.The constructor creates new instances of
WorktreePoolManager,Coordinator, andParallelAgentRunner. This works but makes unit testing harder without mocking modules.src/worktree/task-graph-analyzer.ts (1)
511-532: oldStatus is always 'unknown' in status update event.The
task_status_updatedevent always reportsoldStatus: 'unknown'because the previous status isn't tracked or looked up. Consider fetching the current status first if event consumers need accurate transition information.src/session/parallel-session.ts (3)
256-267: Version handling could mask original schema version.After warning about unknown versions, the code sets
parsed.version = 1which overwrites the original. Consider preserving the original version or implementing proper migration instead.💡 Suggested improvement
// Validate schema version const version = parsed.version ?? 1; if (version !== 1) { console.warn( `Unknown parallel session file version: ${version}. ` + 'Session may not load correctly.' ); + // Consider migrating or preserving original version + // For now, return as-is and let caller decide + return parsed; } - // Ensure version field is set - parsed.version = 1; return parsed;
341-348: Non-null assertion on workUnit lookup is safe but implicit.The
find(...).idpattern with!assertion relies on the fact that all tasks come fromworkUnits.flatMap(). This invariant is correct but implicit. Consider adding a defensive check or comment.💡 Optional: Make invariant explicit
tasks: allTasks.map((t) => ({ id: t.id, title: t.title, status: 'pending' as ParallelTaskStatus, - workUnitId: options.workUnits.find((wu) => - wu.tasks.some((wt) => wt.id === t.id) - )!.id, + workUnitId: options.workUnits.find((wu) => + wu.tasks.some((wt) => wt.id === t.id) + )?.id ?? 'unknown', // Should never be unknown given allTasks derivation })),
608-617: Unused error parameter in failParallelSession.The
_errorparameter is accepted but not stored in the session state. Consider storing it for debugging purposes.💡 Optional: Store error in session
export function failParallelSession( state: PersistedParallelSessionState, - _error?: string + error?: string ): PersistedParallelSessionState { return { ...state, status: 'failed', isPaused: false, + failureReason: error, }; }Note: This would require adding
failureReason?: stringto thePersistedParallelSessionStateinterface.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
README.mdpackage.jsonsrc/cli.tsxsrc/commands/run.tsxsrc/config/index.tssrc/config/schema.tssrc/config/types.tssrc/index.tssrc/session/index.tssrc/session/lock.tssrc/session/parallel-session.tssrc/session/types.tssrc/tui/components/ConflictResolutionPanel.tsxsrc/tui/components/MergeProgressView.tsxsrc/tui/components/ParallelProgressSummary.tsxsrc/tui/components/ParallelProgressView.tsxsrc/tui/components/RollbackPromptPanel.tsxsrc/tui/components/WorkStreamCard.tsxsrc/tui/components/WorkStreamDrillDown.tsxsrc/tui/components/WorktreeMergeCard.tsxsrc/tui/components/index.tssrc/tui/index.tssrc/tui/merge-progress-types.tssrc/tui/parallel-progress-types.tssrc/tui/theme.tssrc/worktree/broadcast-manager.tssrc/worktree/broadcast-types.tssrc/worktree/conflict-resolver-types.tssrc/worktree/conflict-resolver.tssrc/worktree/coordinator-types.tssrc/worktree/coordinator.tssrc/worktree/index.tssrc/worktree/lock-manager.tssrc/worktree/lock-types.tssrc/worktree/manager.tssrc/worktree/merge-engine-types.tssrc/worktree/merge-engine.tssrc/worktree/parallel-agent-runner.tssrc/worktree/parallel-executor-types.tssrc/worktree/parallel-executor.tssrc/worktree/resources.tssrc/worktree/task-graph-analyzer.tssrc/worktree/task-graph-types.tssrc/worktree/types.tstests/integration/parallel-execution.test.tstests/unit/coordinator.test.tstests/unit/merge-engine.test.tstests/unit/worktree-pool-manager.test.tstsconfig.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Start all code files with a file-level JSDoc comment section explaining the file's purpose, prefixed with 'ABOUTME: '
Files:
src/tui/components/WorktreeMergeCard.tsxsrc/cli.tsxsrc/index.tssrc/tui/components/WorkStreamCard.tsxsrc/tui/components/ParallelProgressView.tsxsrc/session/types.tstests/unit/worktree-pool-manager.test.tssrc/commands/run.tsxsrc/worktree/conflict-resolver.tstests/unit/coordinator.test.tstests/integration/parallel-execution.test.tssrc/worktree/parallel-agent-runner.tssrc/tui/parallel-progress-types.tssrc/tui/components/ParallelProgressSummary.tsxsrc/worktree/coordinator-types.tssrc/worktree/coordinator.tssrc/worktree/resources.tssrc/worktree/task-graph-types.tssrc/tui/index.tssrc/worktree/broadcast-types.tssrc/tui/components/WorkStreamDrillDown.tsxsrc/worktree/types.tssrc/tui/theme.tstests/unit/merge-engine.test.tssrc/worktree/parallel-executor-types.tssrc/worktree/index.tssrc/worktree/parallel-executor.tssrc/worktree/merge-engine.tssrc/worktree/manager.tssrc/tui/components/index.tssrc/worktree/broadcast-manager.tssrc/config/schema.tssrc/worktree/lock-types.tssrc/tui/components/RollbackPromptPanel.tsxsrc/config/types.tssrc/tui/merge-progress-types.tssrc/worktree/conflict-resolver-types.tssrc/session/index.tssrc/worktree/task-graph-analyzer.tssrc/tui/components/MergeProgressView.tsxsrc/tui/components/ConflictResolutionPanel.tsxsrc/worktree/merge-engine-types.tssrc/config/index.tssrc/session/parallel-session.tssrc/session/lock.tssrc/worktree/lock-manager.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Start all code files with a file-level JSDoc comment section explaining the file's purpose, prefixed with "ABOUTME: "
Files:
src/tui/components/WorktreeMergeCard.tsxsrc/cli.tsxsrc/index.tssrc/tui/components/WorkStreamCard.tsxsrc/tui/components/ParallelProgressView.tsxsrc/session/types.tstests/unit/worktree-pool-manager.test.tssrc/commands/run.tsxsrc/worktree/conflict-resolver.tstests/unit/coordinator.test.tstests/integration/parallel-execution.test.tssrc/worktree/parallel-agent-runner.tssrc/tui/parallel-progress-types.tssrc/tui/components/ParallelProgressSummary.tsxsrc/worktree/coordinator-types.tssrc/worktree/coordinator.tssrc/worktree/resources.tssrc/worktree/task-graph-types.tssrc/tui/index.tssrc/worktree/broadcast-types.tssrc/tui/components/WorkStreamDrillDown.tsxsrc/worktree/types.tssrc/tui/theme.tstests/unit/merge-engine.test.tssrc/worktree/parallel-executor-types.tssrc/worktree/index.tssrc/worktree/parallel-executor.tssrc/worktree/merge-engine.tssrc/worktree/manager.tssrc/tui/components/index.tssrc/worktree/broadcast-manager.tssrc/config/schema.tssrc/worktree/lock-types.tssrc/tui/components/RollbackPromptPanel.tsxsrc/config/types.tssrc/tui/merge-progress-types.tssrc/worktree/conflict-resolver-types.tssrc/session/index.tssrc/worktree/task-graph-analyzer.tssrc/tui/components/MergeProgressView.tsxsrc/tui/components/ConflictResolutionPanel.tsxsrc/worktree/merge-engine-types.tssrc/config/index.tssrc/session/parallel-session.tssrc/session/lock.tssrc/worktree/lock-manager.ts
🧠 Learnings (2)
📚 Learning: 2026-01-13T12:30:04.922Z
Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T12:30:04.922Z
Learning: After making code changes, always run: `bun run typecheck && bun run build`
Applied to files:
tsconfig.jsonpackage.json
📚 Learning: 2026-01-13T12:29:45.878Z
Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T12:29:45.878Z
Learning: After making code changes, always run: bun run typecheck && bun run build
Applied to files:
tsconfig.jsonpackage.json
🧬 Code graph analysis (28)
src/tui/components/WorktreeMergeCard.tsx (2)
src/tui/merge-progress-types.ts (5)
WorktreeMergeCardProps(189-201)getMergeStatusColor(292-308)getMergeStatusIndicator(313-331)getMergeStatusLabel(336-354)formatMergeDuration(374-382)src/tui/theme.ts (1)
colors(9-58)
src/tui/components/WorkStreamCard.tsx (3)
src/tui/components/index.ts (1)
WorkStreamCard(38-38)src/tui/parallel-progress-types.ts (5)
WorkStreamCardProps(178-190)getStreamStatusColor(258-272)getStreamStatusIndicator(277-291)formatProgress(313-318)formatStreamDuration(323-335)src/tui/theme.ts (1)
colors(9-58)
src/tui/components/ParallelProgressView.tsx (4)
src/tui/parallel-progress-types.ts (6)
WorkStreamProgress(14-62)ParallelProgressStats(103-142)DisplayBroadcast(68-98)ParallelProgressEventListener(230-230)ParallelProgressViewMode(147-147)ParallelProgressEvent(219-225)src/tui/theme.ts (1)
colors(9-58)src/tui/components/WorkStreamDrillDown.tsx (1)
WorkStreamDrillDown(69-239)src/tui/components/ParallelProgressSummary.tsx (1)
ParallelProgressSummary(12-124)
tests/unit/worktree-pool-manager.test.ts (1)
src/worktree/types.ts (1)
DEFAULT_WORKTREE_POOL_CONFIG(31-37)
tests/unit/coordinator.test.ts (1)
src/worktree/coordinator-types.ts (1)
AgentMessagePayload(43-58)
tests/integration/parallel-execution.test.ts (1)
src/worktree/parallel-executor-types.ts (1)
ParallelExecutorEvent(310-321)
src/worktree/parallel-agent-runner.ts (3)
src/worktree/index.ts (5)
ParallelAgentConfig(27-27)ParallelAgentResult(28-28)ParallelAgentRunOptions(29-29)ParallelAgentRunner(25-25)DEFAULT_PARALLEL_AGENT_CONFIG(26-26)src/worktree/task-graph-types.ts (1)
GraphTask(10-37)src/worktree/types.ts (1)
ManagedWorktree(53-80)
src/tui/parallel-progress-types.ts (3)
src/worktree/coordinator-types.ts (1)
AgentStatus(20-25)src/worktree/broadcast-types.ts (2)
BroadcastPriority(32-32)Broadcast(66-96)src/worktree/broadcast-manager.ts (1)
broadcast(111-162)
src/tui/components/ParallelProgressSummary.tsx (3)
src/tui/parallel-progress-types.ts (2)
ParallelProgressSummaryProps(152-173)formatStreamDuration(323-335)src/tui/theme.ts (1)
colors(9-58)src/tui/components/WorkStreamCard.tsx (1)
WorkStreamCard(28-104)
src/worktree/resources.ts (1)
src/worktree/types.ts (1)
SystemResources(85-100)
src/worktree/broadcast-types.ts (1)
src/worktree/broadcast-manager.ts (1)
broadcast(111-162)
src/tui/components/WorkStreamDrillDown.tsx (2)
src/tui/parallel-progress-types.ts (7)
DisplayBroadcast(68-98)getBroadcastPriorityColor(296-308)formatStreamDuration(323-335)WorkStreamDrillDownProps(195-213)getStreamStatusColor(258-272)getStreamStatusIndicator(277-291)formatProgress(313-318)src/tui/theme.ts (1)
colors(9-58)
tests/unit/merge-engine.test.ts (1)
src/worktree/types.ts (1)
ManagedWorktree(53-80)
src/worktree/parallel-executor-types.ts (2)
src/worktree/task-graph-types.ts (2)
GraphTask(10-37)ParallelWorkUnit(70-91)src/worktree/types.ts (1)
ManagedWorktree(53-80)
src/worktree/parallel-executor.ts (6)
src/worktree/task-graph-types.ts (2)
GraphTask(10-37)ParallelWorkUnit(70-91)src/worktree/types.ts (1)
ManagedWorktree(53-80)src/worktree/index.ts (5)
ParallelExecutor(23-23)WorktreePoolManager(16-16)Coordinator(17-17)ParallelAgentRunner(25-25)ParallelAgentConfig(27-27)src/worktree/parallel-executor-types.ts (11)
ParallelExecutorConfig(250-289)ParallelExecutorEvent(310-321)ParallelExecutionResult(215-245)ParallelTaskResult(24-57)ParallelExecutionFailureReport(180-210)ParallelTaskStatus(14-19)TaskExecutionError(62-77)TaskFailurePhase(82-88)FailureSummary(128-149)FailureAttribution(93-123)FailedAgentLog(154-175)src/worktree/parallel-agent-runner.ts (2)
ParallelAgentRunner(106-316)ParallelAgentConfig(24-39)src/logs/structured-logger.ts (2)
error(145-147)log(112-126)
src/worktree/manager.ts (2)
src/worktree/types.ts (9)
WorktreePoolConfig(11-26)ManagedWorktree(53-80)WorktreePoolEventListener(134-134)DEFAULT_WORKTREE_POOL_CONFIG(31-37)WorktreePoolEvent(122-129)WorktreeCreateOptions(139-154)WorktreeAcquisitionResult(105-107)WorktreeCleanupOptions(159-171)WorktreeStatus(42-48)src/worktree/resources.ts (1)
checkResourceAvailability(76-102)
src/config/schema.ts (1)
src/config/index.ts (3)
ResourceLimitsConfigSchema(656-656)ParallelConfigSchema(655-655)ConflictResolutionConfigSchema(657-657)
src/tui/components/RollbackPromptPanel.tsx (2)
src/tui/merge-progress-types.ts (1)
RollbackPromptPanelProps(440-450)src/tui/theme.ts (1)
colors(9-58)
src/config/types.ts (2)
src/config/index.ts (5)
ResourceLimitsConfig(642-642)ConflictResolutionConfig(642-642)DEFAULT_CONFLICT_RESOLUTION_CONFIG(643-643)ParallelConfig(642-642)DEFAULT_PARALLEL_CONFIG(643-643)src/worktree/broadcast-types.ts (1)
BroadcastConfig(101-125)
src/tui/merge-progress-types.ts (3)
src/worktree/types.ts (1)
ManagedWorktree(53-80)src/worktree/conflict-resolver-types.ts (3)
ConflictResolutionResult(41-55)UserResolutionChoice(57-63)FileResolutionResult(32-39)src/worktree/merge-engine-types.ts (6)
BackupBranchInfo(122-134)PostMergeValidationResult(209-216)RollbackResult(180-204)MergeSessionResult(90-117)WorktreeMergeStatus(51-59)WorktreeMergeResult(64-85)
src/worktree/conflict-resolver-types.ts (2)
src/config/types.ts (1)
ConflictResolutionConfig(62-71)src/config/index.ts (1)
ConflictResolutionConfig(642-642)
src/worktree/task-graph-analyzer.ts (1)
src/worktree/task-graph-types.ts (11)
TaskGraphAnalyzerConfig(156-180)TaskGraphEvent(270-279)ParallelizationAnalysis(96-126)ParallelizationReasoning(132-151)BvPlanOutput(197-213)BvInsightsOutput(252-265)GraphTask(10-37)ParallelWorkUnit(70-91)BvPlanTrack(218-223)TaskGraphMetrics(43-64)TaskGraphAnalyzerStats(289-307)
src/tui/components/MergeProgressView.tsx (5)
src/tui/merge-progress-types.ts (1)
MergeProgressViewProps(109-184)src/tui/theme.ts (1)
colors(9-58)src/tui/components/ConflictResolutionPanel.tsx (1)
ConflictResolutionPanel(31-240)src/tui/components/RollbackPromptPanel.tsx (1)
RollbackPromptPanel(15-309)src/tui/components/WorktreeMergeCard.tsx (1)
WorktreeMergeCard(21-120)
src/tui/components/ConflictResolutionPanel.tsx (2)
src/tui/merge-progress-types.ts (2)
ConflictResolutionPanelProps(206-239)getConfidenceDisplay(359-369)src/tui/theme.ts (1)
colors(9-58)
src/worktree/merge-engine-types.ts (3)
src/config/types.ts (1)
ConflictResolutionConfig(62-71)src/worktree/conflict-resolver-types.ts (2)
UserPromptCallback(65-68)ConflictResolutionResult(41-55)src/worktree/types.ts (1)
ManagedWorktree(53-80)
src/session/parallel-session.ts (5)
src/worktree/parallel-executor-types.ts (1)
ParallelExecutorConfig(250-289)src/worktree/types.ts (2)
WorktreeStatus(42-48)ManagedWorktree(53-80)src/session/types.ts (1)
SessionStatus(9-14)src/worktree/task-graph-types.ts (1)
GraphTask(10-37)src/session/lock.ts (1)
checkLock(99-113)
src/session/lock.ts (2)
src/worktree/manager.ts (1)
activeWorktreeCount(67-71)src/session/types.ts (1)
LockFile(16-24)
src/worktree/lock-manager.ts (2)
src/worktree/lock-types.ts (11)
ResourceLock(30-60)PendingLockRequest(105-117)ResourceLockManagerConfig(154-172)WorktreeCacheState(239-257)ResourceLockEventListener(202-202)LockAcquisitionOptions(82-100)LockAcquisitionResult(65-67)ResourceLockManagerStats(207-234)LockMode(12-12)ResourceCategory(17-24)ResourceLockEvent(189-197)src/worktree/index.ts (1)
ResourceLockManager(18-18)
🪛 LanguageTool
README.md
[grammar] ~831-~831: The word “rollback” is a noun. The verb is spelled with a space.
Context: ...Rollback Option**: After merge, you can rollback to pre-merge state if needed Example f...
(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.18.1)
README.md
662-662: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| export function WorktreeMergeCard({ | ||
| progress, | ||
| isSelected, | ||
| compact = false, | ||
| }: WorktreeMergeCardProps): ReactNode { |
There was a problem hiding this comment.
Unused onSelect callback prop.
The onSelect prop is destructured from the props but never used in the component. According to the WorktreeMergeCardProps interface (from merge-progress-types.ts), this callback should be invoked when the card is selected. This appears to be an oversight that would prevent parent components from receiving selection events.
🐛 Suggested fix - wire up onSelect to the card
export function WorktreeMergeCard({
progress,
isSelected,
compact = false,
+ onSelect,
}: WorktreeMergeCardProps): ReactNode {
const statusColor = getMergeStatusColor(progress.status);
const statusIndicator = getMergeStatusIndicator(progress.status);
const statusLabel = getMergeStatusLabel(progress.status);
const durationText = formatMergeDuration(progress.durationMs);
const bgColor = isSelected ? colors.bg.highlight : colors.bg.secondary;
const borderColor = isSelected ? colors.border.active : colors.border.normal;
const conflictIndicator = progress.conflictingFiles && progress.conflictingFiles.length > 0
? ` (${progress.conflictingFiles.length} files)`
: '';
if (compact) {
return (
<box
style={{
flexDirection: 'row',
backgroundColor: bgColor,
padding: 0,
}}
+ onClick={onSelect}
>Note: The exact click handler mechanism depends on how OpenTUI handles events. Verify the appropriate event binding for this framework.
📝 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.
| export function WorktreeMergeCard({ | |
| progress, | |
| isSelected, | |
| compact = false, | |
| }: WorktreeMergeCardProps): ReactNode { | |
| export function WorktreeMergeCard({ | |
| progress, | |
| isSelected, | |
| compact = false, | |
| onSelect, | |
| }: WorktreeMergeCardProps): ReactNode { | |
| const statusColor = getMergeStatusColor(progress.status); | |
| const statusIndicator = getMergeStatusIndicator(progress.status); | |
| const statusLabel = getMergeStatusLabel(progress.status); | |
| const durationText = formatMergeDuration(progress.durationMs); | |
| const bgColor = isSelected ? colors.bg.highlight : colors.bg.secondary; | |
| const borderColor = isSelected ? colors.border.active : colors.border.normal; | |
| const conflictIndicator = progress.conflictingFiles && progress.conflictingFiles.length > 0 | |
| ? ` (${progress.conflictingFiles.length} files)` | |
| : ''; | |
| if (compact) { | |
| return ( | |
| <box | |
| style={{ | |
| flexDirection: 'row', | |
| backgroundColor: bgColor, | |
| padding: 0, | |
| }} | |
| onClick={onSelect} | |
| > |
🤖 Prompt for AI Agents
In @src/tui/components/WorktreeMergeCard.tsx around lines 21 - 25, The
WorktreeMergeCard component destructures props but never invokes the onSelect
callback from WorktreeMergeCardProps; wire onSelect into the card's selection
handler by including onSelect in the destructured props for WorktreeMergeCard
and calling it when the card is activated (e.g., bind onSelect to the card's
click/selection event handler used by your UI framework), passing any expected
parameters (such as event or the item id) and preserving existing
isSelected/compact behavior so parent components receive selection events.
- Fix @types/bun version (1.3.5→1.2.23) to match Bun runtime 1.2.17 - Fix README grammar: 'rollback' → 'roll back' (verb form) - Wire keyboard handlers in ConflictResolutionPanel (was unused) - Add onDismissResult handler in RollbackPromptPanel for result state - Remove unused onSelect from WorktreeMergeCardProps (keyboard nav in parent) - Fix typo: 'requireingAction' → 'requiringAction' in broadcast module - Add 'max_locks_exceeded' failure reason in lock-types (was misleading) - Add error logging in lock-manager initializeWorktreeCache (was silent) - Add error emission in manager syncWithGit (was silent failure) - Fix TOCTOU race in manager acquire with pendingAcquisitions counter - Add merge mutex in manager cleanup for serialization of checkout/merge - Fix null reference in parallel-executor with result.error fallback - Call cleanupAll() in parallel-executor dispose() for cleanup - Assign lastDataHash in task-graph-analyzer analyzeWithBv - Fix test name in merge-engine.test.ts to match assertion - Revert tsconfig rootDir to './src' and remove tests from include
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @src/tui/components/MergeProgressView.tsx:
- Around line 237-240: The RollbackPromptPanel is being passed empty noop
fallbacks for onRollback, onRollbackPreserveDebug, onContinueAnyway and
onAbortAll which can silently disable buttons; update MergeProgressView so that
when isShowingRollbackPrompt is true these callback props are required in
MergeProgressViewProps (or use a discriminated union/conditional type to enforce
presence), and remove the noop fallbacks in the JSX (so TypeScript catches
missing handlers), or alternatively add a development-only warning inside
MergeProgressView before rendering the RollbackPromptPanel that console.warns if
any of onRollback/onRollbackPreserveDebug/onContinueAnyway/onAbortAll are
undefined; reference the MergeProgressView component, MergeProgressViewProps
type, and the RollbackPromptPanel usage to locate changes.
- Around line 59-65: The useEffect handling viewMode changes only sets viewMode
to 'rollback' or 'conflict' when the respective flags are true but does not
reset it back to 'overview' when both isShowingRollbackPrompt and
isResolvingConflict are false. Update the useEffect in MergeProgressView.tsx to
add a condition that sets viewMode to 'overview' when neither
isResolvingConflict nor isShowingRollbackPrompt is true, ensuring the viewMode
returns to 'overview' appropriately.
In @src/worktree/manager.ts:
- Around line 147-154: The catch block is emitting a worktree_error event with
an unsafe cast (undefined as unknown as ManagedWorktree); instead, update the
WorktreePoolEvent union in types.ts to either add a dedicated sync_error event
(e.g., { type: 'sync_error'; error: Error }) or make worktree optional on error
events (e.g., { type: 'worktree_error'; worktree?: ManagedWorktree; error: Error
}), then change the emit call in manager.ts (the emit invocation that uses
worktree_error) to emit the new sync_error event (or emit worktree_error without
a worktree if you chose the optional approach) and remove the cast to
ManagedWorktree so listeners no longer receive an invalid worktree value.
In @src/worktree/parallel-executor.ts:
- Around line 486-502: The failure attribution always sees unknown agent because
activeExecutions entries are removed before the failures map runs; add an
agentId field to the ParallelTaskResult type and set it inside executeTask where
the task runs (capture ctx.agentId before deleting from activeExecutions), then
update the failures mapping in ParallelExecutor to use result.agentId (falling
back to 'unknown') instead of looking up activeExecutions.get(result.task.id);
update any type definitions and places constructing ParallelTaskResult
accordingly.
- Around line 619-627: The running-average is using
this.stats.totalTasksExecuted (which already includes the current batch) to
compute a "previous" count; fix by computing previousCompleted =
this.stats.totalTasksExecuted - tasksWithDuration (use tasksWithDuration, not
results.length) and then compute prevTotal = this.stats.avgTaskDurationMs *
previousCompleted; update this.stats.avgTaskDurationMs = (prevTotal +
totalDurationFromTasks) / (previousCompleted + tasksWithDuration) (or just
divide by this.stats.totalTasksExecuted if you keep it unchanged), and handle
the edge-case previousCompleted === 0 by setting avg to totalDurationFromTasks /
tasksWithDuration; reference symbols: stats.avgTaskDurationMs,
stats.totalTasksExecuted, totalDurationFromTasks, tasksWithDuration.
In @src/worktree/task-graph-analyzer.ts:
- Around line 380-407: convertTrackToGraphTasks currently assigns
criticalPathPosition using insightsOutput?.CriticalPath?.indexOf(issue.id) which
yields -1 for missing IDs; change this to compute the index and set
criticalPathPosition to undefined when indexOf returns -1 (e.g., const idx =
insightsOutput?.CriticalPath?.indexOf(issue.id); criticalPathPosition: idx !==
undefined && idx >= 0 ? idx : undefined) so tasks not on the critical path store
undefined instead of -1 in the metrics object.
- Around line 269-310: fetchTasksFromBd currently calls
JSON.parse(result.stdout) without handling parse failures; wrap the parse in a
try-catch inside fetchTasksFromBd and on catch throw a new Error that includes a
clear message (e.g., "failed to parse bd list JSON"), the raw stdout (and stderr
if helpful) and the original error to aid debugging, consistent with how
execBvPlan handles errors; locate the JSON.parse call in fetchTasksFromBd and
replace it with safe parsing and a descriptive thrown error if parsing fails.
🧹 Nitpick comments (18)
README.md (1)
662-704: Add language specifier to fenced code block.The ASCII diagram should have a language specifier for the fenced code block. Use
textorplaintextto satisfy markdown linting rules.📝 Suggested fix
-``` +```text ┌─────────────────────────────────────────────────────────────────────────────┐ │ PARALLEL EXECUTION FLOW │src/tui/merge-progress-types.ts (1)
356-366: Consider differentiating Medium and Low confidence colours.Both "Medium" and "Low" confidence levels return the same warning colour (
#e0af68). If this is intentional to group them visually, it's fine; otherwise, consider using a distinct colour for "Low" to help users differentiate between confidence tiers.src/tui/components/ConflictResolutionPanel.tsx (2)
46-48: Potential undefined access when conflicts array is empty.If
conflictsis an empty array,selectedConflictwill beundefined, andconflicts.filter(...)will return0for both counts. The rendering at line 178 handles this with a conditional check, but consider adding an early return or guard for the empty state to make the component's behaviour more explicit.💡 Optional: Add early return for empty conflicts
export function ConflictResolutionPanel({ worktree, conflicts, selectedIndex, // ... other props }: ConflictResolutionPanelProps): ReactNode { + if (conflicts.length === 0) { + return ( + <box style={{ padding: 1 }}> + <text fg={colors.fg.muted}>No conflicts to resolve</text> + </box> + ); + } + const selectedConflict = conflicts[selectedIndex];
229-244: Reject action shown even without AI suggestion.The "[r] Reject AI suggestion" option is displayed regardless of whether an AI suggestion exists (unlike "[a] Accept" which is conditional on line 223). Consider hiding the reject option when there's no suggestion to reject, for consistency.
💡 Conditionally show reject option
- <text> - <span fg={colors.accent.tertiary}>[r]</span> - <span fg={colors.fg.secondary}> Reject AI suggestion</span> - </text> + {selectedConflict.suggestion && ( + <text> + <span fg={colors.accent.tertiary}>[r]</span> + <span fg={colors.fg.secondary}> Reject AI suggestion</span> + </text> + )}src/tui/components/RollbackPromptPanel.tsx (2)
29-54: Moveoptionsarray outside the component to avoid recreating on each render.The
optionsarray is static and doesn't depend on props or state, but it's recreated on every render. This also causes thehandleKeyboardcallback to be recreated unnecessarily sinceoptionsis in its dependency array (line 112).♻️ Extract options as a module-level constant
+const OPTIONS: { key: SelectedOption; label: string; description: string; shortcut: string }[] = [ + { + key: 'rollback', + label: 'Rollback to pre-merge state', + description: 'Reset to backup branch, discard merge attempt', + shortcut: '1', + }, + { + key: 'rollback_debug', + label: 'Rollback & preserve for debugging', + description: 'Reset but keep failed merge on a debug branch', + shortcut: '2', + }, + { + key: 'continue', + label: 'Continue anyway', + description: 'Keep the merge despite validation failure', + shortcut: '3', + }, + { + key: 'abort', + label: 'Abort', + description: 'Stop merge process without changes', + shortcut: 'q', + }, +]; + export function RollbackPromptPanel({ // ...props }: RollbackPromptPanelProps): ReactNode { const [selectedOption, setSelectedOption] = useState<SelectedOption>('rollback'); - - const options: { key: SelectedOption; label: string; description: string; shortcut: string }[] = [ - // ... option definitions - ];Then update usages from
optionstoOPTIONSand remove it from the dependency array.
73-113: Dependency array includesoptionswhich is recreated each render.Including
optionsin the dependency array (line 112) causeshandleKeyboardto be recreated on every render sinceoptionsis defined inside the component. After extractingoptionsas a constant, remove it from the dependency array.♻️ Update dependency array after extracting options
const handleKeyboard = useCallback( (key: { name: string }) => { // ... handler logic using OPTIONS instead of options - const optionKeys = options.map(o => o.key); + const optionKeys = OPTIONS.map(o => o.key); // ... }, - [selectedOption, handleSelect, isRollingBack, rollbackResult, onDismissResult, options] + [selectedOption, handleSelect, isRollingBack, rollbackResult, onDismissResult] );src/tui/components/MergeProgressView.tsx (1)
49-53: Consider memoizing or extractinggetInitialViewMode.The function is defined inline within the component body and called once during state initialization. While this works correctly with the function initializer pattern, extracting it could improve readability.
src/worktree/parallel-executor.ts (2)
244-255: Potential worktree leak when agent fails butpreserveFailedWorktreesis false.When
agentResult.successis false andpreserveFailedWorktreesis also false, the worktree is released at line 245. However, ifagentResult.erroris falsy (which shouldn't happen but isn't enforced), theelse ifbranch at line 246 won't execute, meaning no preservation event is emitted but the worktree is also not released.The logic would be clearer with explicit handling:
Suggested clarification
- if (!this.config.preserveFailedWorktrees || agentResult.success) { - await this.worktreeManager.release(worktreeResult.worktree.id); - } else if (agentResult.error) { + if (agentResult.success) { + await this.worktreeManager.release(worktreeResult.worktree.id); + } else if (this.config.preserveFailedWorktrees) { this.emit({ type: 'worktree_preserved', worktreeId: worktreeResult.worktree.id, path: worktreeResult.worktree.path, taskId: task.id, - error: agentResult.error.message, + error: agentResult.error?.message ?? 'Unknown error', }); this.stats.worktreesPreserved++; + } else { + await this.worktreeManager.release(worktreeResult.worktree.id); }
632-640: Shutdown does not await in-flight task completions.The
shutdown()method setsisShuttingDownand aborts active executions but doesn't wait for them to complete. This could cause issues ifdispose()is called immediately after, ascleanupAllmight run while tasks are still winding down.Consider awaiting the abort or documenting that callers should wait for the current
execute()to return before callingdispose().src/worktree/lock-manager.ts (3)
598-618: Deadlock victim selection is arbitrary.The
breakDeadlockmethod selects the last agent in the cycle as the victim (line 603). This approach is simple but may not be optimal. Consider documenting this choice or using a more sophisticated strategy (e.g., youngest lock, fewest locks held, or random).Additionally, the victim's lock request is rejected with
reason: 'timeout_waiting', which is misleading since it was actually broken due to deadlock detection.Consider adding a specific failure reason
Add
'deadlock_victim'toLockAcquisitionFailureReasonin lock-types.ts:export type LockAcquisitionFailureReason = | 'resource_locked_exclusive' | 'write_lock_blocked_by_readers' | 'timeout_waiting' + | 'deadlock_victim' | 'agent_not_registered' | 'max_locks_exceeded' | 'invalid_resource';Then update the resolve call:
request.resolve({ success: false, - reason: 'timeout_waiting', + reason: 'deadlock_victim', waitingBehind: cycle.filter((a) => a !== victimAgentId), });
620-643: Magic number for resource exhaustion threshold.The threshold of
waitQueue.length > 10at line 625 andblockedAgents.length >= 3at line 637 are hardcoded. Consider making these configurable viaResourceLockManagerConfig.Proposed configuration addition
Add to
ResourceLockManagerConfigin lock-types.ts:/** Wait queue size threshold to consider a resource exhausted (default: 10) */ resourceExhaustionQueueThreshold: number; /** Number of blocked agents to trigger worktree creation pause (default: 3) */ blockedAgentsPauseThreshold: number;
267-272: Silent failure in cache sync with swallowed error.The
.catch(() => {})at line 267 silently ignores copy failures for individual resources. While the method returnstruesuggesting success, some resources may have failed to sync. Consider tracking partial failures.Track partial sync failures
try { await mkdir(sharedCachePath, { recursive: true }); + let syncedCount = 0; for (const resource of cacheState.modifiedResources) { const src = join(cacheState.cachePath, resource); const dest = join(sharedCachePath, resource); - await cp(src, dest, { recursive: true }).catch(() => {}); + try { + await cp(src, dest, { recursive: true }); + syncedCount++; + } catch { + // Log or track failed resource sync + } } - return true; + return syncedCount === cacheState.modifiedResources.length; } catch { return false; }src/worktree/manager.ts (3)
277-277: Empty string in git command arguments.When
options.forceis false, the filter produces['worktree', 'remove', worktreePath, '']and thenfilter(Boolean)removes the empty string. However, this pattern is fragile. Consider using conditional spread instead.Clearer argument construction
- await execGit(['worktree', 'remove', worktree.path, options.force ? '--force' : ''].filter(Boolean), this.projectRoot); + const removeArgs = ['worktree', 'remove', worktree.path]; + if (options.force) { + removeArgs.push('--force'); + } + await execGit(removeArgs, this.projectRoot);
95-102: Empty catch block silently ignores listener errors.The empty catch block at line 99-100 silently ignores errors from event listeners. While this prevents a faulty listener from breaking the manager, consider at least logging these errors for debugging purposes.
Add minimal error logging
private emit(event: WorktreePoolEvent): void { for (const listener of this.listeners) { try { listener(event); - } catch { + } catch (err) { + // Listener errors should not break the manager, but log for debugging + console.debug('[WorktreePoolManager] Listener error:', err); } } }
364-370: Silent fallback to filesystem removal may leave git in inconsistent state.When
git worktree removefails at line 362, the code falls back to directly removing the directory at line 366. This could leave git's worktree tracking in an inconsistent state. Consider runninggit worktree pruneafter such removals.Add git worktree prune after fallback removal
try { await rm(worktreePath, { recursive: true, force: true }); + // Clean up git's worktree list after manual removal + await execGit(['worktree', 'prune'], this.projectRoot).catch(() => {}); pruned++; } catch { }src/worktree/lock-types.ts (1)
106-118: Internal implementation detail exposed in public type.The
PendingLockRequestinterface exposes theresolvefunction (line 112), which is an internal implementation detail for the Promise-based wait queue. This could lead to misuse by external consumers.Consider either:
- Not exporting this interface (keep it internal to lock-manager.ts)
- Creating a separate public-facing type without the
resolvefunctionsrc/worktree/task-graph-analyzer.ts (2)
100-108: Consider logging swallowed listener errors for observability.Silently ignoring listener errors (line 104-106) can hide bugs in event handlers. For debugging purposes, consider logging these errors to stderr or emitting a diagnostic event.
💡 Suggested improvement
private emit(event: TaskGraphEvent): void { for (const listener of this.listeners) { try { listener(event); - } catch { - // Ignore listener errors + } catch (error) { + // Log but don't propagate listener errors + console.error('[TaskGraphAnalyzer] Event listener error:', error); } } }
513-534:oldStatusis always 'unknown' in task_status_updated event.Line 527 hardcodes
oldStatus: 'unknown', which reduces the usefulness of the event for consumers that need to track state transitions. Consider fetching the current status before updating, or documenting this limitation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.mdpackage.jsonsrc/tui/components/ConflictResolutionPanel.tsxsrc/tui/components/MergeProgressView.tsxsrc/tui/components/RollbackPromptPanel.tsxsrc/tui/merge-progress-types.tssrc/worktree/broadcast-manager.tssrc/worktree/broadcast-types.tssrc/worktree/lock-manager.tssrc/worktree/lock-types.tssrc/worktree/manager.tssrc/worktree/parallel-executor.tssrc/worktree/task-graph-analyzer.tstests/unit/merge-engine.test.tstsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- tsconfig.json
- src/worktree/broadcast-manager.ts
- tests/unit/merge-engine.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Start all code files with a file-level JSDoc comment section explaining the file's purpose, prefixed with 'ABOUTME: '
Files:
src/tui/components/RollbackPromptPanel.tsxsrc/tui/merge-progress-types.tssrc/worktree/manager.tssrc/tui/components/ConflictResolutionPanel.tsxsrc/worktree/lock-manager.tssrc/worktree/broadcast-types.tssrc/tui/components/MergeProgressView.tsxsrc/worktree/parallel-executor.tssrc/worktree/task-graph-analyzer.tssrc/worktree/lock-types.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Start all code files with a file-level JSDoc comment section explaining the file's purpose, prefixed with "ABOUTME: "
Files:
src/tui/components/RollbackPromptPanel.tsxsrc/tui/merge-progress-types.tssrc/worktree/manager.tssrc/tui/components/ConflictResolutionPanel.tsxsrc/worktree/lock-manager.tssrc/worktree/broadcast-types.tssrc/tui/components/MergeProgressView.tsxsrc/worktree/parallel-executor.tssrc/worktree/task-graph-analyzer.tssrc/worktree/lock-types.ts
🧠 Learnings (2)
📚 Learning: 2026-01-13T12:30:04.922Z
Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T12:30:04.922Z
Learning: After making code changes, always run: `bun run typecheck && bun run build`
Applied to files:
package.json
📚 Learning: 2026-01-13T12:29:45.878Z
Learnt from: CR
Repo: subsy/ralph-tui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T12:29:45.878Z
Learning: After making code changes, always run: bun run typecheck && bun run build
Applied to files:
package.json
🧬 Code graph analysis (5)
src/tui/components/RollbackPromptPanel.tsx (2)
src/tui/merge-progress-types.ts (1)
RollbackPromptPanelProps(437-448)src/tui/theme.ts (1)
colors(9-58)
src/worktree/manager.ts (3)
src/worktree/index.ts (1)
WorktreePoolManager(16-16)src/worktree/types.ts (9)
WorktreePoolConfig(11-26)ManagedWorktree(53-80)WorktreePoolEventListener(134-134)DEFAULT_WORKTREE_POOL_CONFIG(31-37)WorktreePoolEvent(122-129)WorktreeCreateOptions(139-154)WorktreeAcquisitionResult(105-107)WorktreeCleanupOptions(159-171)WorktreeStatus(42-48)src/worktree/resources.ts (1)
checkResourceAvailability(76-102)
src/worktree/lock-manager.ts (1)
src/worktree/lock-types.ts (11)
ResourceLock(30-60)PendingLockRequest(106-118)ResourceLockManagerConfig(155-173)WorktreeCacheState(240-258)ResourceLockEventListener(203-203)DEFAULT_RESOURCE_LOCK_MANAGER_CONFIG(178-185)LockAcquisitionOptions(83-101)LockAcquisitionResult(65-67)LockMode(12-12)ResourceCategory(17-24)ResourceLockEvent(190-198)
src/worktree/broadcast-types.ts (1)
src/worktree/broadcast-manager.ts (1)
broadcast(111-162)
src/worktree/task-graph-analyzer.ts (1)
src/worktree/task-graph-types.ts (13)
TaskGraphAnalyzerConfig(156-180)TaskGraphEventListener(284-284)DEFAULT_TASK_GRAPH_ANALYZER_CONFIG(185-192)TaskGraphEvent(270-279)ParallelizationAnalysis(96-126)ParallelizationReasoning(132-151)BvPlanOutput(197-213)BvInsightsOutput(252-265)GraphTask(10-37)ParallelWorkUnit(70-91)BvPlanTrack(218-223)TaskGraphMetrics(43-64)TaskGraphAnalyzerStats(289-307)
🪛 markdownlint-cli2 (0.18.1)
README.md
662-662: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (53)
package.json (2)
23-24: LGTM!The test scripts are well-structured and align with the project's existing Bun-based tooling. The watch mode is a useful addition for development workflows.
56-56: **LGTM!**The addition of@types/bunis appropriate for TypeScript support with Bun APIs. The@types/bunpackage is a shim that loadsbun-types, which provides the type definitions. Recent versions like 1.3.3 exist, so the specified^1.2.23version range is valid and will resolve to a compatible version.README.md (1)
656-869: Comprehensive parallel execution documentation.The documentation thoroughly covers the parallel execution infrastructure with clear diagrams, configuration options, keyboard shortcuts, and failure handling guidance. This aligns well with the PR objectives for implementing parallel execution with worktree management, merge engine, and conflict resolution.
src/worktree/broadcast-types.ts (6)
1-32: Well-structured type definitions with proper ABOUTME header.The file follows the coding guidelines with an ABOUTME header. The
BroadcastCategorytype and defaults provide a sensible set of discovery categories for agent-to-agent communication during parallel execution.
37-96: Well-designed payload and broadcast interfaces.The
BroadcastPayloadandBroadcastinterfaces provide comprehensive structures for agent communication. The inclusion ofconsumedBy,acknowledgedBy, andsupersededfields enables proper lifecycle tracking. TheBroadcastCategory | stringunion appropriately balances type safety with extensibility for custom categories.
98-138: Sensible configuration defaults.The
BroadcastConfiginterface andDEFAULT_BROADCAST_CONFIGprovide well-considered defaults. The 1-hour TTL, 1000 message history limit, and 1-minute cleanup interval are reasonable for typical parallel execution scenarios.
140-187: Clear input/output contract for broadcast creation.The
CreateBroadcastOptionsandCreateBroadcastResulttypes provide a clear API contract. The result type'sdisabledandfilteredflags allow callers to distinguish between different non-success scenarios, which aligns well with the implementation inbroadcast-manager.ts.
189-241: Comprehensive broadcast consumption API.The consumption types provide flexible filtering options and enriched context for consumers. The
ConsumedBroadcastextension withrelevanceScoreandsuggestedActionTypeenables intelligent action prioritisation by consuming agents.
243-315: Complete event and subscription model.The
BroadcastEventdiscriminated union,BroadcastStatsmetrics, andBroadcastSubscriptiontypes form a complete eventing model. The subscription interface appropriately supports both sync and async callbacks viavoid | Promise<void>return type.src/tui/merge-progress-types.ts (6)
1-5: ABOUTME comment is present and well-documented.The file-level JSDoc comment follows the coding guidelines with the required
ABOUTME:prefix and clearly explains the file's purpose.
26-33: Type definitions are well-structured.The
MergeDisplayStatustype alias with inline comments for each status provides good documentation for consumers of this type.
265-284: Mapping function is exhaustive and correct.The
toDisplayStatusfunction correctly maps allWorktreeMergeStatusvalues to their display equivalents. The handling ofconflict_pending_usermapping to'conflict'makes sense for UI display purposes.
371-379: Duration formatting handles edge cases correctly.The function correctly handles sub-minute and multi-minute durations. For sub-second durations (ms < 1000), it returns "0s", which is acceptable for display purposes.
384-403: Converter function is well-implemented.The
resultToProgressfunction correctly maps engine results to UI progress models. The explicittaskTitle: undefinedis clear and allows consumers to populate this separately if needed.
408-421: Nullish coalescing for confidence is appropriate.Using
?? 0for the confidence fallback ensures a valid number is always present for display calculations.src/tui/components/ConflictResolutionPanel.tsx (4)
1-4: ABOUTME comment is present and descriptive.The file-level JSDoc comment follows the coding guidelines with the required
ABOUTME:prefix.
50-85: Keyboard handler implementation is well-structured.The handler correctly guards against undefined
selectedConflictwhen accepting AI suggestions (line 54), uses optional chaining for all callbacks, and includes vim-style navigation keys (k/j). The dependency array is complete.
143-166: List rendering uses stable keys correctly.Using
conflict.filePathas the key is appropriate since file paths should be unique within a single worktree's conflict list.
87-87:useKeyboardhook usage is correct.The code properly wraps
handleKeyboardinuseCallbackwith a complete dependency array, and the hook call aligns with the documented usage pattern for @opentui/react. Event listener cleanup is handled internally by the library, so no additional cleanup logic is required at the call site.src/tui/components/RollbackPromptPanel.tsx (4)
1-5: ABOUTME comment is present and descriptive.The file-level JSDoc comment follows the coding guidelines with the required
ABOUTME:prefix.
117-166: Rollback result rendering handles success and failure cases well.The conditional rendering correctly displays different content based on
rollbackResult.success, including the restored SHA (truncated to 7 characters) and optional debug branch information.
168-184: Rolling back state UI is clear and informative.The loading state displays the target SHA reference appropriately truncated.
261-282: Rendering uses stable keys correctly.Using
option.keyas the React key is appropriate since these are unique string identifiers.src/tui/components/MergeProgressView.tsx (4)
1-4: ABOUTME comment is present and descriptive.The file-level JSDoc comment follows the coding guidelines with the required
ABOUTME:prefix.
87-182: Keyboard handler is comprehensive and well-organised.The handler correctly separates logic by view mode, includes vim-style navigation (
k/j), and has a complete dependency array. The state updater function pattern (setLocalSelectedIndex(prev => ...)) is used correctly to avoid stale closure issues.
210-229: Conflict view correctly delegates to ConflictResolutionPanel.The props are passed through appropriately, and the
onSelectConflictcallback correctly updates both local and parent state.
310-316: List rendering uses stable worktree IDs as keys.Using
wt.worktree.idas the key is correct and provides stable identity for React's reconciliation.src/worktree/parallel-executor.ts (2)
1-5: LGTM! ABOUTME header is properly formatted.The file-level JSDoc comment follows the coding guidelines with the 'ABOUTME: ' prefix and clearly explains the file's purpose.
83-91: LGTM! Safe event emission with error isolation.The
emitmethod correctly catches and ignores listener errors to prevent a faulty listener from breaking the executor's operation.src/worktree/lock-manager.ts (3)
1-5: LGTM! ABOUTME header follows coding guidelines.The file-level JSDoc comment properly explains the purpose with the required 'ABOUTME: ' prefix.
363-373: LGTM! Correct readers-writer lock semantics.The
canAcquireLockmethod correctly implements readers-writer semantics:
- Multiple readers can hold locks simultaneously (line 369)
- Writers require exclusive access (line 372 returns false if any lock exists)
88-97: No action required—this is not a genuine race condition.The code is entirely synchronous between the
canAcquireLock()check and theresource.locks.push()operation. JavaScript's single-threaded event loop executes this synchronous block atomically, preventing any interleaving by concurrent agents. ThecanAcquireLock()method performs only synchronous checks on the locks array,createLock()is a synchronous object constructor, andtrackAgentLock()andemit()are synchronous operations. There is no TOCTOU vulnerability here; the code is already safe in Node.js without modification.Likely an incorrect or invalid review comment.
src/worktree/manager.ts (2)
1-5: LGTM! ABOUTME header follows coding guidelines.The file-level JSDoc comment properly explains the purpose with the required 'ABOUTME: ' prefix.
260-275: LGTM! Mutex pattern for merge serialisation is correctly implemented.The merge lock implementation using a Promise chain ensures that merge operations are serialised, preventing concurrent merges from corrupting the git state. The lock is properly released in the
finallyblock.src/worktree/lock-types.ts (3)
1-5: LGTM! ABOUTME header follows coding guidelines.The file-level JSDoc comment properly explains the purpose with the required 'ABOUTME: ' prefix.
144-150: LGTM! Well-structured default configurations.The default configuration objects are properly typed and provide sensible defaults. The nested
DEFAULT_CACHE_ISOLATION_CONFIGis correctly referenced inDEFAULT_RESOURCE_LOCK_MANAGER_CONFIG.Also applies to: 178-185
190-198: LGTM! Comprehensive event type union.The
ResourceLockEventunion covers all important lifecycle events for the lock manager, including acquisition, release, expiration, waiting, timeouts, deadlock detection, and resource exhaustion. This provides excellent observability.src/worktree/task-graph-analyzer.ts (16)
1-5: LGTM!The ABOUTME header follows the coding guidelines and clearly describes the file's purpose.
31-75: LGTM!The
execCommandhelper is well-structured with proper timeout handling, stream collection, and error propagation.
110-123: LGTM!The BV availability check with caching is appropriate for performance whilst avoiding repeated command executions.
125-153: LGTM!The
analyzemethod has clean orchestration with proper event emission, timing statistics, and error handling with re-throw semantics.
155-183: LGTM!Good use of
Promise.allfor parallel BV command execution. The fallback to null for insights allows graceful degradation.
185-213: LGTM!The BD fallback path provides appropriate degraded functionality with clear messaging about limitations and recommendations.
242-267: LGTM!Graceful degradation by returning
nullon failure is appropriate for optional insights data.
312-320: LGTM!The status mapping with a sensible default is reasonable.
322-345: LGTM!Proper deduplication using a Set ensures tasks appearing in multiple tracks are only counted once.
347-378: LGTM!Good guard against empty task arrays (line 357) preventing division by zero. Track limiting respects the configured maximum.
409-429: LGTM!Clear and informative grouping reasons that aid debugging and user understanding.
431-454: LGTM!Priority-based sorting and configurable batch sizing with minimum threshold ensures sensible work unit creation.
456-474: LGTM!The method assumes non-empty task arrays, which is guaranteed by the calling context in
createWorkUnitsFromTasks.
476-511: LGTM!Comprehensive reasoning generation with safe non-null assertions guarded by prior checks.
541-554: LGTM!Stats aggregation and hash retrieval methods are clean. The
cacheHitRate: 0suggests caching is not yet implemented, which is fine for the initial implementation.
215-240: Verify thebvCLI's actual label parameter support before assuming this is a limitation.The code uses only the first label for
bv --robot-planandbv --robot-insights(lines 218 and 245), whereas thebdcommand uses all labels via.join(',')(line 275). This pattern is likely intentional—different CLI tools may have different label parameter APIs. Without documentation confirming that thebvCLI supports multiple--labelflags or comma-separated values, passing all labels could cause unexpected behaviour or errors.
| } catch (error) { | ||
| // Git worktree list failed - emit error event but continue with current state | ||
| this.emit({ | ||
| type: 'worktree_error', | ||
| worktree: undefined as unknown as ManagedWorktree, | ||
| error: error instanceof Error ? error : new Error('Failed to sync with git'), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Type assertion undefined as unknown as ManagedWorktree is unsafe.
Casting undefined to ManagedWorktree at line 151 violates type safety and will cause issues for any listener that tries to access properties on event.worktree. Consider using a different event type for sync errors or making worktree optional in the error event.
Proposed fix
Option 1: Create a specific sync error event type in types.ts:
export type WorktreePoolEvent =
| { type: 'worktree_created'; worktree: ManagedWorktree }
// ... other events
| { type: 'worktree_error'; worktree: ManagedWorktree; error: Error }
| { type: 'sync_error'; error: Error }; // New event typeOption 2: Make worktree optional for errors:
| { type: 'worktree_error'; worktree?: ManagedWorktree; error: Error }Then update the code:
- this.emit({
- type: 'worktree_error',
- worktree: undefined as unknown as ManagedWorktree,
- error: error instanceof Error ? error : new Error('Failed to sync with git'),
- });
+ this.emit({
+ type: 'sync_error',
+ error: error instanceof Error ? error : new Error('Failed to sync with git'),
+ });🤖 Prompt for AI Agents
In @src/worktree/manager.ts around lines 147 - 154, The catch block is emitting
a worktree_error event with an unsafe cast (undefined as unknown as
ManagedWorktree); instead, update the WorktreePoolEvent union in types.ts to
either add a dedicated sync_error event (e.g., { type: 'sync_error'; error:
Error }) or make worktree optional on error events (e.g., { type:
'worktree_error'; worktree?: ManagedWorktree; error: Error }), then change the
emit call in manager.ts (the emit invocation that uses worktree_error) to emit
the new sync_error event (or emit worktree_error without a worktree if you chose
the optional approach) and remove the cast to ManagedWorktree so listeners no
longer receive an invalid worktree value.
Git conflict markers include branch names (e.g., '<<<<<<< HEAD') but buildConflictMarkerBlock created bare markers without branch names. This caused content.replace() to fail finding matches. Replace string-based matching with regex patterns that match conflict markers regardless of trailing branch name text.
|
@anthonymq is attempting to deploy a commit to the plgeek Team on Vercel. A member of the Team first needs to authorize it. |
- Add agentId to ParallelTaskResult for proper failure attribution - Fix running average calculation to use completed task count - Add try-catch for JSON.parse in task-graph-analyzer bd list parsing - Fix criticalPathPosition to return undefined instead of -1 for non-critical tasks
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/worktree/parallel-executor.ts`:
- Around line 625-631: The running-average update in parallel-executor.ts
miscomputes prevCompletedCount by subtracting results.length (which includes
cancelled tasks) causing negative or skewed averages; instead compute
prevCompletedCount using only tasks that contributed durations (use
tasksWithDuration) or count completed/failed entries in results, then compute
prevTotal = this.stats.avgTaskDurationMs * Math.max(0, prevCompletedCount) and
update this.stats.avgTaskDurationMs = completedTaskCount > 0 ? (prevTotal +
totalDurationFromTasks) / completedTaskCount : 0; adjust references to results,
tasksWithDuration, prevCompletedCount, totalDurationFromTasks, and
this.stats.avgTaskDurationMs accordingly.
- Around line 427-443: The cancelled result returned by createCancelledResult is
missing the agentId field, causing inconsistency with other result types; update
createCancelledResult to include agentId (e.g., agentId: ctx.agentId) in the
returned ParallelTaskResult object so cancelled results match the shape of
successful/failed results returned elsewhere (use the existing
TaskExecutionContext property used by other result creators).
- Around line 156-162: The shutdown loop aborts controllers and pushes cancelled
results but never awaits the underlying promises in runningPromises, allowing
tasks to keep running and leak resources; modify the shutdown logic in
parallel-executor.ts to collect the promise values from the runningPromises map
(not just keys), call abort on each execution via the
activeExecutions.get(taskId)?.abortController, then await
Promise.allSettled(collectedPromises) (or equivalent wait-with-timeout) before
returning so all tasks have a chance to terminate and release resources; keep
using createCancelledResult(ctx, 'Execution shutdown') to record cancellation
while ensuring the actual promise objects are awaited.
In `@src/worktree/task-graph-analyzer.ts`:
- Around line 352-383: The createWorkUnitsFromTracks method can divide by zero
when this.config.maxTasksPerUnit is 0 because limitedTasks may be empty; update
createWorkUnitsFromTracks (or validate this.config.maxTasksPerUnit in the
constructor) to guard against empty limitedTasks before computing avgPriority:
either skip creating the work unit if limitedTasks.length === 0 or compute
avgPriority with a safe default (e.g., 0) and avoid the division. Ensure the
check references limitedTasks and this.config.maxTasksPerUnit and keep emit({
type: 'work_unit_created', ... }) behavior consistent.
🧹 Nitpick comments (3)
src/worktree/parallel-executor.ts (2)
67-70: Consider making worktree directory configurable.The worktree directory is hardcoded to
.worktrees. IfParallelExecutorConfigalready supports a custom path, consider using it here.
93-97: Partial initialisation failure may leave resources in inconsistent state.If
agentRunner.initialize()throws,worktreeManageris already initialised but won't be cleaned up. Consider wrapping in try/catch with cleanup on failure, or documenting that callers must calldispose()on any failure.♻️ Proposed fix
async initialize(): Promise<void> { - await this.worktreeManager.initialize(); - await this.agentRunner.initialize(); - this.coordinator.start(); + await this.worktreeManager.initialize(); + try { + await this.agentRunner.initialize(); + this.coordinator.start(); + } catch (error) { + await this.worktreeManager.cleanupAll({ force: true }); + throw error; + } }src/worktree/task-graph-analyzer.ts (1)
549-558:cacheHitRateis always zero.Line 556 returns a hardcoded
0forcacheHitRate, but there's no caching mechanism in the class. Consider either implementing caching or removing this field from the stats if it's not planned.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/worktree/parallel-executor-types.tssrc/worktree/parallel-executor.tssrc/worktree/task-graph-analyzer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/worktree/parallel-executor-types.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Start all code files with a file-level JSDoc comment section explaining the file's purpose, prefixed with 'ABOUTME: '
Files:
src/worktree/parallel-executor.tssrc/worktree/task-graph-analyzer.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Start all code files with a file-level JSDoc comment section explaining the file's purpose, prefixed with "ABOUTME: "
Files:
src/worktree/parallel-executor.tssrc/worktree/task-graph-analyzer.ts
🔇 Additional comments (15)
src/worktree/parallel-executor.ts (5)
1-5: LGTM!The file-level ABOUTME comment is well-structured and clearly describes the file's purpose: parallel task execution with continue-on-error semantics across git worktrees.
267-280: LGTM!The
agentIdis now correctly included inParallelTaskResult(lines 279 and 312), addressing the earlier failure attribution issue where lookups fromactiveExecutionswould returnundefined.Also applies to: 302-313
345-362: LGTM!The bounded buffer implementation correctly truncates from the start (
slice(-maxOutputSizeBytes)) to retain the most recent output, which is the expected behaviour for debugging purposes.
488-503: LGTM!The failure attribution now correctly uses
result.agentIddirectly from theParallelTaskResultobject, resolving the earlier issue where lookups fromactiveExecutionswould always returnundefined.
651-655: LGTM!The
disposemethod now correctly callscleanupAll({ force: true })to clean up remaining worktrees, addressing the earlier feedback.src/worktree/task-graph-analyzer.ts (10)
1-5: LGTM! Well-documented file header.The ABOUTME comment clearly describes the module's purpose and integration points as per the coding guidelines.
31-75: Clean async command execution wrapper.The design to always resolve (rather than reject) provides consistent error handling at call sites. Timeout handling with SIGTERM and exit code 124 follows Unix conventions.
77-108: LGTM!Config merging, listener management, and internal state tracking are well-structured. The defensive error handling in
emit()prevents listener failures from disrupting the analyser.
110-153: LGTM!The analysis orchestration is clean with proper timing, event emission, and error handling. The
bvAvailablecaching avoids repeated filesystem checks.
155-183: LGTM!The parallel execution of
bv --robot-planandbv --robot-insightsis efficient. ThelastDataHashassignment at line 169 properly tracks the analysis state.
215-267: Good error handling for BV commands.Both
execBvPlanandexecBvInsightswrap JSON parsing in try-catch blocks with descriptive error messages. The graceful degradation (returningnull) for insights failures is appropriate since that data is supplementary.
269-315: LGTM!The JSON parsing is now wrapped in try-catch with informative error messages. The mapping logic correctly handles dependencies and filters by type.
385-415: LGTM!The
criticalPathPositionhandling now correctly returnsundefinedfor tasks not on the critical path (lines 398-400), avoiding the semantically incorrect-1value.
439-519: LGTM!The batch creation respects both
minTasksPerUnitandmaxParallelUnitsconstraints. The reasoning generation provides valuable context about parallelisation decisions, constraints, and recommendations.
560-562: LGTM!The
lastDataHashis now properly assigned inanalyzeWithBv(line 169), making this getter functional.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/worktree/parallel-executor.ts
Outdated
| for (const [taskId] of runningPromises) { | ||
| const ctx = this.activeExecutions.get(taskId); | ||
| if (ctx) { | ||
| ctx.abortController.abort(); | ||
| results.push(this.createCancelledResult(ctx, 'Execution shutdown')); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unresolved promises after shutdown may cause resource leaks.
When the executor shuts down, remaining runningPromises are aborted and cancelled results are pushed, but the actual promises are never awaited. This means tasks may still be executing in the background, potentially causing worktree or file handle leaks.
🐛 Proposed fix
+ // Wait for aborted tasks to settle
+ await Promise.allSettled(Array.from(runningPromises.values()));
+
for (const [taskId] of runningPromises) {
const ctx = this.activeExecutions.get(taskId);
if (ctx) {
ctx.abortController.abort();
results.push(this.createCancelledResult(ctx, 'Execution shutdown'));
}
}🤖 Prompt for AI Agents
In `@src/worktree/parallel-executor.ts` around lines 156 - 162, The shutdown loop
aborts controllers and pushes cancelled results but never awaits the underlying
promises in runningPromises, allowing tasks to keep running and leak resources;
modify the shutdown logic in parallel-executor.ts to collect the promise values
from the runningPromises map (not just keys), call abort on each execution via
the activeExecutions.get(taskId)?.abortController, then await
Promise.allSettled(collectedPromises) (or equivalent wait-with-timeout) before
returning so all tasks have a chance to terminate and release resources; keep
using createCancelledResult(ctx, 'Execution shutdown') to record cancellation
while ensuring the actual promise objects are awaited.
src/worktree/parallel-executor.ts
Outdated
| private createCancelledResult(ctx: TaskExecutionContext, reason: string): ParallelTaskResult { | ||
| const endedAt = new Date(); | ||
| this.stats.totalTasksCancelled++; | ||
| this.emit({ type: 'task_cancelled', task: ctx.task, reason }); | ||
|
|
||
| return { | ||
| task: ctx.task, | ||
| status: 'cancelled', | ||
| worktree: ctx.worktree, | ||
| startedAt: ctx.startedAt, | ||
| endedAt, | ||
| durationMs: endedAt.getTime() - ctx.startedAt.getTime(), | ||
| stdout: ctx.stdout, | ||
| stderr: ctx.stderr, | ||
| error: this.createExecutionError(reason, 'unknown'), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing agentId in cancelled result for consistency.
The createCancelledResult method doesn't include agentId in the returned ParallelTaskResult, while successful and failed results do include it. This may cause inconsistent handling downstream.
🐛 Proposed fix
return {
task: ctx.task,
status: 'cancelled',
worktree: ctx.worktree,
startedAt: ctx.startedAt,
endedAt,
durationMs: endedAt.getTime() - ctx.startedAt.getTime(),
stdout: ctx.stdout,
stderr: ctx.stderr,
error: this.createExecutionError(reason, 'unknown'),
+ agentId: ctx.agentId,
};🤖 Prompt for AI Agents
In `@src/worktree/parallel-executor.ts` around lines 427 - 443, The cancelled
result returned by createCancelledResult is missing the agentId field, causing
inconsistency with other result types; update createCancelledResult to include
agentId (e.g., agentId: ctx.agentId) in the returned ParallelTaskResult object
so cancelled results match the shape of successful/failed results returned
elsewhere (use the existing TaskExecutionContext property used by other result
creators).
- MergeProgressView: return to overview when rollback/conflict flags become false - MergeProgressView: require all callbacks before showing rollback panel - parallel-executor: await running promises on shutdown to prevent leaks - parallel-executor: exclude cancelled tasks from running average calculation - parallel-executor: remove unused createCancelledResult method - task-graph-analyzer: guard against division by zero with maxTasksPerUnit=0
|
@anthonymq thanks for this one - I've been thinking about this separately, along with worktree support and sandboxes. I'll need to review this in depth tomorrow and think about how to handle it. |
2 participants
Summary
Adds comprehensive parallel execution infrastructure to enable concurrent AI agent workflows, allowing multiple agents to work simultaneously on independent tasks.
Closes #25
Why Parallelize?
Sequential execution creates bottlenecks when agents have independent work. Parallelization enables:
Changes
Core Infrastructure
3d1d201) - Manages git worktrees for isolated parallel workspaces with resource awareness8c6ee69) - Message passing system for agent-to-agent communication0924a4c) - Coordinates shared resource access to prevent conflictsTask Analysis & Configuration
89a209c) - Integrates with bd/bv for parallel task identification51359fe) - CLI flags (--no-parallel) and configuration optionsMerge & Conflict Resolution
18de2eb) - Consolidates parallel branch changes with backup/rollback4d33685) - Intelligent merge conflict handlingExecution & Monitoring
2dcfd0d) - Real-time discovery sharing between agentsfd1d4a1) - Core executor with continue-on-error semantics2ca9a66) - Drill-down view for execution progress (US-010)b516042) - Conflict UI for merge operations (US-011)Recovery & Integration
f6b6bc0) - Recovery mechanism for failed merges (US-012)be0ffea) - Parallel session management (US-013)c2a5cd3) - Spawn infrastructure for parallel execution (US-014)6204909) - Comprehensive docs and test suite (US-015)Commits (15)
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.