fix(batch): classify RequestTimeoutError as timeout in --all --wait fan-out#154
fix(batch): classify RequestTimeoutError as timeout in --all --wait fan-out#154Awad-de wants to merge 3 commits into
Conversation
…an-out When test run --all --wait or test rerun --all --wait hit a per-request timeout during batch fan-out polling, RequestTimeoutError rejected the fan-out before out.print(). Classify it as status:'timeout' in pollFreshAccepted and pollAccepted so stdout always lists every dispatched runId. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesPer-run timeout handling in batch fan-out
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
src/commands/test.ts (1)
5459-5474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate error-classification logic between
pollFreshAcceptedandpollAccepted.The
RequestTimeoutErrorbranch (and the surroundingTimeoutError/ApiErrorbranches) inpollFreshAccepted(Lines 5459-5474) is essentially identical to the one inpollAccepted(Lines 6627-6642), differing only inrunIdvsentry.runId. Consider extracting a sharedclassifyFanOutPollError(err, { testId, runId, timeoutSeconds })helper to avoid drift between the two copies (e.g., the existingApiErrorbranches already diverge inexitCodehandling between the two functions).♻️ Suggested shared helper (illustrative)
function classifyFanOutPollError( err: unknown, entry: { testId: string; runId: string }, timeoutSeconds: number, ): { testId: string; runId: string; status: string; error: { code: string; message: string; exitCode: number } } { if (err instanceof TimeoutError) { return { testId: entry.testId, runId: entry.runId, status: 'timeout', error: { code: 'UNSUPPORTED', message: `Timed out after ${timeoutSeconds}s`, exitCode: 7 }, }; } if (err instanceof RequestTimeoutError) { return { testId: entry.testId, runId: entry.runId, status: 'timeout', error: { code: 'UNSUPPORTED', message: err.message, exitCode: err.exitCode }, }; } throw err; }Also applies to: 6627-6642
🤖 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/commands/test.ts` around lines 5459 - 5474, The timeout/error classification logic in pollFreshAccepted and pollAccepted is duplicated and already drifting, so extract a shared helper such as classifyFanOutPollError that handles TimeoutError, RequestTimeoutError, and ApiError consistently. Update both pollFreshAccepted and pollAccepted to call the helper with their entry/testId/runId context so the runId difference is localized and future exitCode or message changes stay in one place.
🤖 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.
Nitpick comments:
In `@src/commands/test.ts`:
- Around line 5459-5474: The timeout/error classification logic in
pollFreshAccepted and pollAccepted is duplicated and already drifting, so
extract a shared helper such as classifyFanOutPollError that handles
TimeoutError, RequestTimeoutError, and ApiError consistently. Update both
pollFreshAccepted and pollAccepted to call the helper with their
entry/testId/runId context so the runId difference is localized and future
exitCode or message changes stay in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7d396e0-eb26-46a3-8e43-59fff4ffbc20
📒 Files selected for processing (3)
src/commands/test.rerun.spec.tssrc/commands/test.run.spec.tssrc/commands/test.ts
|
Discord: |
|
Approved — this redo is exactly what we asked for: only the claimed fix, mirroring the existing RequestTimeoutError partial-stdout contract. Its sibling (#153) merged today, which is also what made this one go CONFLICTING (same regions of |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/test.run.spec.ts (1)
3696-3748: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConfirm the new regression test also verifies the documented
nextActionhint.Per the change summary, this block asserts
exitCode: 7andaccepted[].status === 'timeout'for the twoRequestTimeoutErrorruns, but doesn't appear to assert thenextActionresume hint (testsprite test wait <runId>) that the batch timeout contract emits (perApiError.fromEnvelopeinrunTestRunAll). Since DOCUMENTATION.md specifies this hint as part of the timeout contract, consider assertingerror.nextAction(or the equivalent thrown error field) contains bothrunIds so the contract is fully pinned down by this regression test, not just the exit code and stdout accepted-status shape.🤖 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/commands/test.run.spec.ts` around lines 3696 - 3748, The new regression test for runTestRunAll should also pin the timeout contract’s nextAction resume hint, not just exitCode: 7 and the accepted[] timeout statuses. Update the test case in test.run.spec.ts to assert the thrown error from runTestRunAll (or the ApiError-like object it returns) includes nextAction with the documented “testsprite test wait <runId>” guidance for both accepted runIds. Use the existing runTestRunAll and RequestTimeoutError setup so the test fully verifies the batch timeout behavior end-to-end.Source: Path instructions
🤖 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/commands/test.run.spec.ts`:
- Line 8: The test file imports readFileSync from node:fs but never uses it,
triggering ESLint/CI. Remove the unused readFileSync import from the top-level
import statement in test.run.spec.ts, keeping the remaining fs imports used by
the spec.
---
Nitpick comments:
In `@src/commands/test.run.spec.ts`:
- Around line 3696-3748: The new regression test for runTestRunAll should also
pin the timeout contract’s nextAction resume hint, not just exitCode: 7 and the
accepted[] timeout statuses. Update the test case in test.run.spec.ts to assert
the thrown error from runTestRunAll (or the ApiError-like object it returns)
includes nextAction with the documented “testsprite test wait <runId>” guidance
for both accepted runIds. Use the existing runTestRunAll and RequestTimeoutError
setup so the test fully verifies the batch timeout behavior end-to-end.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 4e6b3ac4-ca58-4bf7-8b2a-4291fa0fd59e
📒 Files selected for processing (3)
src/commands/test.rerun.spec.tssrc/commands/test.run.spec.tssrc/commands/test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/test.rerun.spec.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/test.run.spec.ts (1)
3696-3748: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConfirm the new regression test also verifies the documented
nextActionhint.Per the change summary, this block asserts
exitCode: 7andaccepted[].status === 'timeout'for the twoRequestTimeoutErrorruns, but doesn't appear to assert thenextActionresume hint (testsprite test wait <runId>) that the batch timeout contract emits (perApiError.fromEnvelopeinrunTestRunAll). Since DOCUMENTATION.md specifies this hint as part of the timeout contract, consider assertingerror.nextAction(or the equivalent thrown error field) contains bothrunIds so the contract is fully pinned down by this regression test, not just the exit code and stdout accepted-status shape.🤖 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/commands/test.run.spec.ts` around lines 3696 - 3748, The new regression test for runTestRunAll should also pin the timeout contract’s nextAction resume hint, not just exitCode: 7 and the accepted[] timeout statuses. Update the test case in test.run.spec.ts to assert the thrown error from runTestRunAll (or the ApiError-like object it returns) includes nextAction with the documented “testsprite test wait <runId>” guidance for both accepted runIds. Use the existing runTestRunAll and RequestTimeoutError setup so the test fully verifies the batch timeout behavior end-to-end.Source: Path instructions
🤖 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/commands/test.run.spec.ts`:
- Line 8: The test file imports readFileSync from node:fs but never uses it,
triggering ESLint/CI. Remove the unused readFileSync import from the top-level
import statement in test.run.spec.ts, keeping the remaining fs imports used by
the spec.
---
Nitpick comments:
In `@src/commands/test.run.spec.ts`:
- Around line 3696-3748: The new regression test for runTestRunAll should also
pin the timeout contract’s nextAction resume hint, not just exitCode: 7 and the
accepted[] timeout statuses. Update the test case in test.run.spec.ts to assert
the thrown error from runTestRunAll (or the ApiError-like object it returns)
includes nextAction with the documented “testsprite test wait <runId>” guidance
for both accepted runIds. Use the existing runTestRunAll and RequestTimeoutError
setup so the test fully verifies the batch timeout behavior end-to-end.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 4e6b3ac4-ca58-4bf7-8b2a-4291fa0fd59e
📒 Files selected for processing (3)
src/commands/test.rerun.spec.tssrc/commands/test.run.spec.tssrc/commands/test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/test.rerun.spec.ts
🛑 Comments failed to post (1)
src/commands/test.run.spec.ts (1)
8-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove unused
readFileSyncimport.Flagged by ESLint/CI as unused.
🧹 Proposed fix
-import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs';📝 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.import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs';🧰 Tools
🪛 ESLint
[error] 8-8: 'readFileSync' is defined but never used.
(
@typescript-eslint/no-unused-vars)🪛 GitHub Actions: CI / 0_Lint & Format.txt
[error] 8-8: ESLint (
@typescript-eslint/no-unused-vars): 'readFileSync' is defined but never used.🪛 GitHub Actions: CI / Lint & Format
[error] 8-8: ESLint (
@typescript-eslint/no-unused-vars): 'readFileSync' is defined but never used.🪛 GitHub Check: Lint & Format
[failure] 8-8:
'readFileSync' is defined but never used🤖 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/commands/test.run.spec.ts` at line 8, The test file imports readFileSync from node:fs but never uses it, triggering ESLint/CI. Remove the unused readFileSync import from the top-level import statement in test.run.spec.ts, keeping the remaining fs imports used by the spec.Source: Linters/SAST tools
2 participants
What does this PR do?
Fixes empty stdout in
--output jsonmode whentest run --all --waitortest rerun --all --waithit a per-request timeout during batch fan-out polling.Before:
RequestTimeoutErrorfrom any member poll rejected the fan-outpromise before
out.print()— stdout empty, agents could not recover runIds.After:
pollFreshAccepted/pollAcceptedclassifyRequestTimeoutErroras
status: 'timeout'(mirrorscreate-batch --runand existingTimeoutErrorhandling). Fan-out completes, stdout carries every dispatched runId, exit 7
fires with
testsprite test wait <runId>hints in stderr.Complements #153 (single-run TimeoutError) and #155 (single rerun TimeoutError).
Related issue
Fixes #158
Type of change
Checklist
mainbranch.npm run lintandnpm run typecheckpass.npm testpasses (1570 tests).test.run.spec.tsandtest.rerun.spec.ts.Notes for reviewers
pollFreshAcceptedandpollAccepted.runBatchRun(create-batch --run) at ~line 2806.gap on the
--all --waitfresh-run and batch-rerun paths.Summary by CodeRabbit
test run --waitandtest rerun --waitfan-out soRequestTimeoutErroris handled per run instead of failing the whole operation.status: "timeout", while retaining the expected exit code and non-empty stdout.--waitfan-out behavior when polling times out, including validation ofacceptedrunIds and per-entry timeout status.