fix(beads-rust): recursively fetch all descendant tasks for epic hierarchy - #394
Open
oliveagle wants to merge 2 commits into
Open
fix(beads-rust): recursively fetch all descendant tasks for epic hierarchy#394oliveagle wants to merge 2 commits into
oliveagle wants to merge 2 commits into
Conversation
Implement a comprehensive timeout and retry system for agent execution that provides a Claude Code-like experience when requests take too long. Features: - Auto-interrupt on timeout (like Ctrl+C) - User prompt to retry, continue indefinitely, or cancel - Exponential backoff for retries (1.5x multiplier, max 5min) - Maximum 3 retry attempts - Visual timeout dialog with clear options Changes: - src/chat/types.ts: Add TimeoutConfig, TimeoutState, and event types - src/chat/engine.ts: Implement timeout handling with auto-interrupt - src/tui/components/PrdChatApp.tsx: Add TimeoutDialog UI component - src/tui/theme.ts: Add bg.overlay color for dialog backdrop 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…archy getChildIds previously only returned direct children of an epic, missing grandchild tasks (e.g., epic -> user-story -> tasks). Now recursively traverses parent-child relationships to include all descendants. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@oliveagle is attempting to deploy a commit to the plgeek Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThis PR adds timeout and retry capabilities to the chat engine, integrates a timeout dialog UI with keyboard-driven recovery options, refactors the task tracker to recursively collect all descendant tasks, and extends theme colors to support overlay styling. ChangesChat Engine Timeout & Retry Feature
Task Descendant Recursion
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/plugins/trackers/builtin/beads-rust/index.test.ts (1)
641-647: 💤 Low valueUnused mock response for
epic.1enrichment.The mock data for
epic.1(line 617) doesn't includedependency_count, soenrichDependencieswon't callenrichTaskDependenciesfor it. This mock response will remain unconsumed. Consider removing it to keep the test's mock sequence aligned with actual behavior.🧹 Suggested cleanup
// dep list epic.1.2 -> no children { exitCode: 0, stdout: JSON.stringify([]) }, - // dep list epic.1 for dependency enrichment - { - exitCode: 0, - stdout: JSON.stringify([ - { issue_id: 'epic.1', depends_on_id: 'epic', type: 'parent-child' }, - ]), - }, // dep list epic.1.1 for dependency enrichment🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/trackers/builtin/beads-rust/index.test.ts` around lines 641 - 647, The mock response includes an unused dependency row for 'epic.1' that doesn't have dependency_count so enrichDependencies will never call enrichTaskDependencies for it; fix by either removing the { issue_id: 'epic.1', depends_on_id: 'epic', type: 'parent-child' } entry from the mocked stdout array in the test or add a dependency_count field to the corresponding mocked task object so enrichDependencies treats it as having dependencies and invokes enrichTaskDependencies (look for enrichDependencies and enrichTaskDependencies in the test to locate the relevant mocks).src/chat/engine.ts (1)
252-267: ⚡ Quick win
doRetrydoes not enforcemaxRetries.The method increments
retryCountwithout checking if it exceedsconfig.timeoutConfig.maxRetries. While the UI may hide the retry option, the publicretry()method would still allow exceeding the limit if called directly.Proposed fix
private async doRetry(): Promise<void> { if (!this.pendingUserMessage || !this.pendingOptions) { this.setStatus('idle'); return; } + // Enforce max retries + if (this.timeoutState.retryCount >= this.config.timeoutConfig.maxRetries) { + this.setStatus('error'); + this.emit({ + type: 'error:occurred', + timestamp: new Date(), + error: `Maximum retries (${this.config.timeoutConfig.maxRetries}) exceeded`, + }); + this.pendingUserMessage = null; + this.pendingOptions = null; + return; + } + // Increment retry count and update timeout this.timeoutState.retryCount++;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat/engine.ts` around lines 252 - 267, doRetry increments timeoutState.retryCount without enforcing config.timeoutConfig.maxRetries; update doRetry (the method in the class handling retries) to check timeoutState.retryCount against config.timeoutConfig.maxRetries before incrementing and performing retry logic, and if the limit is reached set timeoutState.retryPending = false, call setStatus('idle') (or an appropriate failure state) and return early so retryCount never exceeds maxRetries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat/engine.ts`:
- Around line 409-419: The code nulls this.currentExecution immediately after
awaiting handle.promise, which prevents handleTimeout from being able to
interrupt the running handle; update the flow so the execution handle remains
available to handleTimeout by either moving the assignment this.currentExecution
= null to after the timeout handling block or changing handleTimeout to accept
the handle (e.g., pass the local handle returned from this.config.agent.execute
into handleTimeout) and use that handle for isRunning()/interrupt checks; ensure
references to currentExecution, handle, handleTimeout, execute, and promise are
adjusted consistently so interruption logic can access the live handle.
In `@src/tui/components/PrdChatApp.tsx`:
- Around line 539-555: The timeout handling and dialog are currently only wired
to engineRef events and the chat-phase UI, so timeouts originating from
taskEngineRef.current.sendMessage (review-phase/tracker generation) have no
retry/continue/cancel UI; to fix, subscribe the same timeout event handling for
taskEngineRef (or centralize the timeout handler used in the 'timeout:occurred'
and 'retry:started' cases so it responds to both engineRef and taskEngineRef
events) and render the TimeoutDialog in the review-phase return branch as well
(ensure showTimeoutDialog && timeoutState && <TimeoutDialog ... /> appears in
the review-phase JSX), keeping existing state variables setIsLoading,
setShowTimeoutDialog, setTimeoutState, and setLoadingStatus so the
retry/continue/cancel flow works for both engines.
---
Nitpick comments:
In `@src/chat/engine.ts`:
- Around line 252-267: doRetry increments timeoutState.retryCount without
enforcing config.timeoutConfig.maxRetries; update doRetry (the method in the
class handling retries) to check timeoutState.retryCount against
config.timeoutConfig.maxRetries before incrementing and performing retry logic,
and if the limit is reached set timeoutState.retryPending = false, call
setStatus('idle') (or an appropriate failure state) and return early so
retryCount never exceeds maxRetries.
In `@src/plugins/trackers/builtin/beads-rust/index.test.ts`:
- Around line 641-647: The mock response includes an unused dependency row for
'epic.1' that doesn't have dependency_count so enrichDependencies will never
call enrichTaskDependencies for it; fix by either removing the { issue_id:
'epic.1', depends_on_id: 'epic', type: 'parent-child' } entry from the mocked
stdout array in the test or add a dependency_count field to the corresponding
mocked task object so enrichDependencies treats it as having dependencies and
invokes enrichTaskDependencies (look for enrichDependencies and
enrichTaskDependencies in the test to locate the relevant mocks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5fc58109-6cca-4669-b847-2c7849c3e902
📒 Files selected for processing (6)
src/chat/engine.tssrc/chat/types.tssrc/plugins/trackers/builtin/beads-rust/index.test.tssrc/plugins/trackers/builtin/beads-rust/index.tssrc/tui/components/PrdChatApp.tsxsrc/tui/theme.ts
Comment on lines
409
to
+419
| const handle = this.config.agent.execute(prompt, [], agentOptions); | ||
| this.currentExecution = handle; | ||
|
|
||
| const result = await handle.promise; | ||
| this.currentExecution = null; | ||
|
|
||
| const durationMs = Date.now() - startTime; | ||
|
|
||
| if (result.status === 'timeout') { | ||
| // Handle timeout | ||
| return await this.handleTimeout(content, options, durationMs); |
Contributor
There was a problem hiding this comment.
currentExecution is nulled before handleTimeout can interrupt the process.
The assignment this.currentExecution = null at line 413 executes before the timeout check at line 417. When handleTimeout is called, it tries to interrupt via this.currentExecution?.isRunning() (lines 511-513), but the handle is already null, making that interrupt dead code.
Move the null assignment after the timeout branch, or pass the handle to handleTimeout.
Proposed fix
const handle = this.config.agent.execute(prompt, [], agentOptions);
this.currentExecution = handle;
const result = await handle.promise;
- this.currentExecution = null;
const durationMs = Date.now() - startTime;
if (result.status === 'timeout') {
// Handle timeout
- return await this.handleTimeout(content, options, durationMs);
+ const timeoutResult = await this.handleTimeout(content, options, durationMs);
+ this.currentExecution = null;
+ return timeoutResult;
}
+ this.currentExecution = null;
+
if (result.status !== 'completed') {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handle = this.config.agent.execute(prompt, [], agentOptions); | |
| this.currentExecution = handle; | |
| const result = await handle.promise; | |
| this.currentExecution = null; | |
| const durationMs = Date.now() - startTime; | |
| if (result.status === 'timeout') { | |
| // Handle timeout | |
| return await this.handleTimeout(content, options, durationMs); | |
| const handle = this.config.agent.execute(prompt, [], agentOptions); | |
| this.currentExecution = handle; | |
| const result = await handle.promise; | |
| const durationMs = Date.now() - startTime; | |
| if (result.status === 'timeout') { | |
| // Handle timeout | |
| const timeoutResult = await this.handleTimeout(content, options, durationMs); | |
| this.currentExecution = null; | |
| return timeoutResult; | |
| } | |
| this.currentExecution = null; | |
| if (result.status !== 'completed') { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat/engine.ts` around lines 409 - 419, The code nulls
this.currentExecution immediately after awaiting handle.promise, which prevents
handleTimeout from being able to interrupt the running handle; update the flow
so the execution handle remains available to handleTimeout by either moving the
assignment this.currentExecution = null to after the timeout handling block or
changing handleTimeout to accept the handle (e.g., pass the local handle
returned from this.config.agent.execute into handleTimeout) and use that handle
for isRunning()/interrupt checks; ensure references to currentExecution, handle,
handleTimeout, execute, and promise are adjusted consistently so interruption
logic can access the live handle.
Comment on lines
+539
to
+555
| case 'timeout:occurred': | ||
| if (isMountedRef.current) { | ||
| // Stop showing loading indicator | ||
| setIsLoading(false); | ||
| setTimeoutState(event.timeoutState); | ||
| setShowTimeoutDialog(true); | ||
| } | ||
| break; | ||
|
|
||
| case 'retry:started': | ||
| if (isMountedRef.current) { | ||
| setShowTimeoutDialog(false); | ||
| setTimeoutState(event.timeoutState); | ||
| setIsLoading(true); | ||
| setLoadingStatus(`Retrying request (attempt ${event.timeoutState.retryCount})...`); | ||
| } | ||
| break; |
Contributor
There was a problem hiding this comment.
Timeout recovery is scoped to engineRef only, so review-phase task timeouts cannot use retry/continue/cancel.
Line 539 handles timeout events only from engine.on(...), but tracker generation uses taskEngineRef.current.sendMessage(...) (review phase). Also, Line 1294 renders TimeoutDialog only in the chat-phase layout. Result: task-generation timeouts have no dialog-driven recovery path.
Suggested fix
@@
const engineRef = useRef<ChatEngine | null>(null);
const taskEngineRef = useRef<ChatEngine | null>(null);
+ const timeoutEngineRef = useRef<ChatEngine | null>(null);
@@
- const unsubscribe = engine.on((event: ChatEvent) => {
+ const handleEngineEvent = (source: ChatEngine) => (event: ChatEvent) => {
switch (event.type) {
@@
case 'timeout:occurred':
if (isMountedRef.current) {
// Stop showing loading indicator
setIsLoading(false);
setTimeoutState(event.timeoutState);
setShowTimeoutDialog(true);
+ timeoutEngineRef.current = source;
}
break;
@@
case 'retry:started':
if (isMountedRef.current) {
setShowTimeoutDialog(false);
setTimeoutState(event.timeoutState);
setIsLoading(true);
setLoadingStatus(`Retrying request (attempt ${event.timeoutState.retryCount})...`);
}
break;
}
- });
+ };
+
+ const unsubscribe = engine.on(handleEngineEvent(engine));
+ const unsubscribeTask = taskEngine.on(handleEngineEvent(taskEngine));
@@
return () => {
isMountedRef.current = false;
unsubscribe();
+ unsubscribeTask();
};
@@
const handleTimeoutRetry = useCallback(() => {
- if (!engineRef.current) return;
+ const timeoutEngine = timeoutEngineRef.current;
+ if (!timeoutEngine) return;
@@
- engineRef.current.retry();
+ timeoutEngine.retry();
}, []);
@@
const handleTimeoutCancel = useCallback(() => {
- if (!engineRef.current) return;
+ const timeoutEngine = timeoutEngineRef.current;
+ if (!timeoutEngine) return;
@@
- engineRef.current.cancelTimeout();
+ timeoutEngine.cancelTimeout();
}, []);
@@
const handleTimeoutContinue = useCallback(() => {
- if (!engineRef.current) return;
+ const timeoutEngine = timeoutEngineRef.current;
+ if (!timeoutEngine) return;
@@
- engineRef.current.continueIndefinitely();
+ timeoutEngine.continueIndefinitely();
}, []);// Also render TimeoutDialog in the review-phase return branch
{showTimeoutDialog && timeoutState && <TimeoutDialog timeoutState={timeoutState} />}Also applies to: 1294-1299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tui/components/PrdChatApp.tsx` around lines 539 - 555, The timeout
handling and dialog are currently only wired to engineRef events and the
chat-phase UI, so timeouts originating from taskEngineRef.current.sendMessage
(review-phase/tracker generation) have no retry/continue/cancel UI; to fix,
subscribe the same timeout event handling for taskEngineRef (or centralize the
timeout handler used in the 'timeout:occurred' and 'retry:started' cases so it
responds to both engineRef and taskEngineRef events) and render the
TimeoutDialog in the review-phase return branch as well (ensure
showTimeoutDialog && timeoutState && <TimeoutDialog ... /> appears in the
review-phase JSX), keeping existing state variables setIsLoading,
setShowTimeoutDialog, setTimeoutState, and setLoadingStatus so the
retry/continue/cancel flow works for both engines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
getChildIdspreviously only returned direct children of an epic, missing grandchild tasks (e.g., epic -> user-story -> tasks)Test plan
bun test src/plugins/trackers/builtin/beads-rust/index.test.ts— all 50 tests passralph-tui runin a project with multi-level hierarchy (epic -> user-story -> tasks) — all tasks should appear🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration Changes