Repository navigation
[codex] fix(query): abort active work on QueryGuard timeout - #1623
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughAdds a watchdog timeout handler API to ChangesQuery Timeout Backstop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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/utils/QueryGuard.ts`:
- Around line 157-161: The _timeoutHandler callback invocation in the QueryGuard
try-finally block can throw exceptions that escape the timer callback and crash
the Node process. Wrap the _timeoutHandler?.(this._generation) call in an inner
try-catch block to catch and swallow any exceptions thrown by the timeout
handler, ensuring that forceEnd() still executes in the finally block and the
error does not propagate up and terminate the process.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aecbcf18-55fe-45e5-be72-0fa2d202d4c6
📒 Files selected for processing (3)
src/screens/REPL.tsxsrc/utils/QueryGuard.test.tssrc/utils/QueryGuard.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: smoke-and-tests
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/utils/QueryGuard.test.tssrc/utils/QueryGuard.tssrc/screens/REPL.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/utils/QueryGuard.test.tssrc/utils/QueryGuard.tssrc/screens/REPL.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/QueryGuard.test.tssrc/utils/QueryGuard.tssrc/screens/REPL.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/QueryGuard.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/QueryGuard.test.tssrc/utils/QueryGuard.tssrc/screens/REPL.tsx
🔇 Additional comments (3)
src/utils/QueryGuard.ts (1)
119-131: LGTM!src/utils/QueryGuard.test.ts (1)
59-89: LGTM!src/screens/REPL.tsx (1)
1729-1749: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/QueryGuard.test.ts`:
- Around line 91-108: The test `timeout handler errors do not escape the
watchdog callback` modifies global state by calling `vi.useFakeTimers()` and
`vi.spyOn()`, but the cleanup code that calls `consoleError.mockRestore()` and
`vi.useRealTimers()` is not failure-safe. Wrap the test body logic in a
try/finally block so that the cleanup code in the finally block always executes,
even if an assertion fails, ensuring test isolation is preserved and global
timer and mock state is properly restored.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8affd3cb-2464-4221-b315-bf22cfc5ca85
📒 Files selected for processing (2)
src/utils/QueryGuard.test.tssrc/utils/QueryGuard.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/utils/QueryGuard.tssrc/utils/QueryGuard.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/utils/QueryGuard.tssrc/utils/QueryGuard.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/QueryGuard.tssrc/utils/QueryGuard.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/QueryGuard.tssrc/utils/QueryGuard.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/QueryGuard.test.ts
🔇 Additional comments (1)
src/utils/QueryGuard.ts (1)
159-160: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/QueryGuard.test.ts (1)
59-73: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winApply the same try/finally pattern for timer cleanup in tests 1 and 2.
Both tests mutate global timer state with
vi.useFakeTimers()but rely on line-of-sight cleanup withvi.useRealTimers()at the end. If an assertion fails, the real timers won't be restored, leaking state into subsequent tests. Wrap each test body in try/finally as done in test 3 (lines 96-109) for consistency and reliable isolation.Proposed fix
For test 'timeout notifies owner with the timed-out generation' (lines 59-73):
test('timeout notifies owner with the timed-out generation', () => { vi.useFakeTimers() - const guard = new QueryGuard() - const onTimeout = vi.fn() - guard.setTimeoutHandler(onTimeout) - - const gen = guard.tryStart()! - vi.advanceTimersByTime(5 * 60 * 1000) - - expect(onTimeout).toHaveBeenCalledTimes(1) - expect(onTimeout).toHaveBeenCalledWith(gen) - expect(guard.isActive).toBe(false) - - vi.useRealTimers() + try { + const guard = new QueryGuard() + const onTimeout = vi.fn() + guard.setTimeoutHandler(onTimeout) + + const gen = guard.tryStart()! + vi.advanceTimersByTime(5 * 60 * 1000) + + expect(onTimeout).toHaveBeenCalledTimes(1) + expect(onTimeout).toHaveBeenCalledWith(gen) + expect(guard.isActive).toBe(false) + } finally { + vi.useRealTimers() + } })For test 'timeout handler cleanup prevents stale notification' (lines 75-89):
test('timeout handler cleanup prevents stale notification', () => { vi.useFakeTimers() - const guard = new QueryGuard() - const onTimeout = vi.fn() - const cleanup = guard.setTimeoutHandler(onTimeout) - cleanup() - - guard.tryStart() - vi.advanceTimersByTime(5 * 60 * 1000) - - expect(onTimeout).not.toHaveBeenCalled() - expect(guard.isActive).toBe(false) - - vi.useRealTimers() + try { + const guard = new QueryGuard() + const onTimeout = vi.fn() + const cleanup = guard.setTimeoutHandler(onTimeout) + cleanup() + + guard.tryStart() + vi.advanceTimersByTime(5 * 60 * 1000) + + expect(onTimeout).not.toHaveBeenCalled() + expect(guard.isActive).toBe(false) + } finally { + vi.useRealTimers() + } })Also applies to: 75-89
🤖 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/utils/QueryGuard.test.ts` around lines 59 - 73, Both the test 'timeout notifies owner with the timed-out generation' (lines 59-73) and the test 'timeout handler cleanup prevents stale notification' (lines 75-89) mutate global timer state with vi.useFakeTimers() but only call vi.useRealTimers() at the end without protection. If any assertion fails before cleanup, real timers won't be restored, leaking state to subsequent tests. Wrap each test body in a try/finally block where vi.useFakeTimers() is called before the try block and vi.useRealTimers() is called in the finally block, following the same pattern already established in the third test. This ensures reliable timer restoration regardless of test outcome.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/utils/QueryGuard.test.ts`:
- Around line 59-73: Both the test 'timeout notifies owner with the timed-out
generation' (lines 59-73) and the test 'timeout handler cleanup prevents stale
notification' (lines 75-89) mutate global timer state with vi.useFakeTimers()
but only call vi.useRealTimers() at the end without protection. If any assertion
fails before cleanup, real timers won't be restored, leaking state to subsequent
tests. Wrap each test body in a try/finally block where vi.useFakeTimers() is
called before the try block and vi.useRealTimers() is called in the finally
block, following the same pattern already established in the third test. This
ensures reliable timer restoration regardless of test outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 15fc46f3-5c54-4eff-86eb-adf0cc23466f
📒 Files selected for processing (1)
src/utils/QueryGuard.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: smoke-and-tests
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/utils/QueryGuard.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/utils/QueryGuard.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/QueryGuard.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/QueryGuard.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/QueryGuard.test.ts
🔇 Additional comments (1)
src/utils/QueryGuard.test.ts (1)
96-109: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
…#1623) * fix(query): abort active work on QueryGuard timeout * fix(query): contain timeout handler failures * test(query): harden timeout handler cleanup * test(query): centralize QueryGuard timer cleanup
Port provider-agnostic timeout abort mechanism to OpenCC. The upstream `[codex]` tag is misleading — the change hooks QueryGuard's timeout to abort in-flight work, which OpenCC needs as substrate for the Twigpine#1682 lifecycle port. Codex-specific paths are excluded per provider policy. Upstream SHA: 174ebd5
3 participants
Summary
QueryGuardtimeout callback so the REPL owner is notified when the watchdog fires.AbortControlleron timeout beforeQueryGuard.forceEnd()releases the guard.Root Cause
QueryGuardforce-ended its internal state after five minutes, which hid the spinner but did not abort the active query generator. If the provider stream or tool path remained stuck, the underlying work could continue after the UI appeared idle.Impact
A watchdog timeout now actively interrupts the running query instead of only resetting local guard state. This keeps stale provider/tool work from surviving behind an idle prompt and aligns timeout behavior with the existing abort semantics.
Validation
bun test src/utils/QueryGuard.test.tsbun run typecheckbun run smokebun run scripts/pr-intent-scan.ts --base upstream/maingit diff --checkSummary by CodeRabbit
New Features
Tests