fix(test): let the host timeout own supervisor cleanup - #11576
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe ChangesTimeout fixture configuration
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The test-only timeout adjustment preserves the host-timeout and cleanup assertions without introducing production risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit b1aeede in the Show a line coverage summary of the most impacted files.
TypeScript / code-coverage/cliThe overall line coverage in commit b1aeede in the Show a line coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Outcome
The launch supervisor timeout test now lets the host timeout terminate the command before the fixture's internal deadline expires.
Reason
The fixture's two-second session budget could expire before the 2.5-second host timeout. This caused
fixture.result.timedOutto be false.The same assertion failed on main CI and blocked PR #11487.
Changes
Give
supervisor-timeoutthe existing ten-second session budget used byrestored-canonical-timeout. Keep the host timeout and all descendant-cleanup assertions unchanged.No production code, retry policy, or new mechanism changes.
Verification
--init:npx vitest run --project e2e-support test/e2e/support/launch-agent-turn.test.ts -t "host command times out" --reporter=verbose— passed; 1 test passed, 43 deselected. This ran before rebase; the test file is unchanged by rebase.npx vitest run --project e2e-support test/e2e/support/launch-agent-turn.test.ts— 6 passed, 38 Linux-only tests skipped.npm --prefix nemoclaw run buildandnpm run build:cli— passed.NODE_OPTIONS=--max-old-space-size=5120 npm run validate:pr— passed forb1aeedef5b80ae061745a502eb5938b755526f43, with canonical validation base41c5625e8b831ed213cd5c381385973adc58659c.Review notes
Local Advisor sandbox creation is unavailable in this shepherd session. Alternative review completed for the one-line diff: correctness, unchanged cleanup assertions, sibling timeout budgets, and security boundaries were checked. This is self-review, not independent Advisor clearance. Hosted checks and review remain pending.
An initial Linux container without an init process retained zombie descendants; the corrected container passed the affected test. The full Linux suite did not complete, so no full-suite pass is claimed.
Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit