Repository navigation
test: stabilize QueryGuard keepalive interruption test - #2323
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title is concise, scoped to the test change, and accurately describes stabilizing the QueryGuard keepalive interruption test. |
| Description check | |
The description follows the required template and provides the change rationale, impact, testing commands and results, limitations, and notes. It is complete and directly related to the diff. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Risk Surface Disclosed | |
PASS — The PR changes only src/services/api/codexShim.interruption.test.ts. The diff adds fake-timer control and cleanup to a test; it does not change authentication, provider routing, permissions, … |
| No Hidden Policy Change | |
PASS — The PR changes only src/services/api/codexShim.interruption.test.ts. The diff adds fake-timer control, event-loop yielding, boundary assertions, and cleanup for the test. The stream frames, Q… |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
Full details: Risk Surface Disclosed
-
Explanation
PASS — The PR changes only
src/services/api/codexShim.interruption.test.ts. The diff adds fake-timer control and cleanup to a test; it does not change authentication, provider routing, permissions, outbound network behavior, background execution, startup/config-home behavior, skills/plugins/MCP, CI permissions, or release scripts. No listed risk surface applies, so no blocker disclosure is required.
Full details: No Hidden Policy Change
-
Explanation
PASS — The PR changes only
src/services/api/codexShim.interruption.test.ts. The diff adds fake-timer control, event-loop yielding, boundary assertions, and cleanup for the test. The stream frames, QueryGuard settings, and production imports remain test fixtures. No product behavior, trust model, routing default, telemetry/network behavior, or permission policy changes are present.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
— jatmn's PR Review agent
Test-only stabilization for QueryGuard keepalive idle timing in codexShim.interruption.test.ts. The diff is surgical (one file, no production paths). I verified fake-timer scoping, the 249/250 ms boundary, and that the test still guards against keepalive-driven idle resets. Clear the merge-readiness items before landing.
Merge readiness
- Rebase onto current
main. Livemainis75c73ea3811f359a8bad23b073bd4713846ea833(docs: add CONTRIBUTING notes for CI caching and subprocess test timeouts (#2322)). This head branches from1c42a10628df59cdaaf3a75a89978737f592721a, so it does not include that docs commit. Rebase (or mergemainforward) before merge so the upgrade-plan stack stays current. - Mark the PR ready for review. GitHub still lists this PR as a draft (
draft: true). Mark it ready when you want the merge gate to treat it as landable. - Merge-base decision:
live-base-three-dot— reviewed range1c42a10628df59cdaaf3a75a89978737f592721a..1cf50c499d2e2d231d1eb026c49c8cd65cb0abf5(one commit). No scope creep in the diff. - CI at head
1cf50c499d2e2d231d1eb026c49c8cd65cb0abf5:changes,pr-intent-scan,smoke-and-tests,dedicated-tests,typecheck, all eightunit-testsshards, launcher compatibility jobs — success.CodeQL (actions),CodeQL (javascript-typescript), andAnalyze (javascript-typescript)— success. AggregateCodeQLcheck — success.web— skipped (expected for this path set). Commit status contextCodeRabbit— success (“Review approved”). No PR-owned failing required checks on this head.
Test fix verification
- Fake timer scope and cleanup:
jest.useFakeTimers()runs only insidekeepalives and parsed-but-ignored frames cannot reset QueryGuard.jest.useRealTimers()runs in the testfinallybefore trace/env cleanup. The next test in the same file still uses realsetIntervaltiming and passed in a full-file local run (14/14) and CI. - Real event-loop yield:
nextTurn()wraps hostsetImmediatecaptured before fake timers, so eachjest.advanceTimersByTimeis followed by a real turn. That drainsdriveQueryEvents/ stream parsing between ticks instead of racing the monotonic idle deadline against a backed-up interval queue. - 249 ms / 250 ms boundary: The loop advances through 249 ms of synthetic time with
settled === false, then the final 1 ms triggers idle timeout withsettled === trueandoutcome.timeout.elapsedMs === 250, matchingidleTimeoutMs: 250and QueryGuard’sperformance.now()-based idle math. - Regression sensitivity: Production keepalives and ignored parsed frames must not call
registerActivity(only yielded query events do). If idle fired too early, the 249 ms assertion would fail; if keepalives incorrectly extended activity, the 250 ms settled assertion orelapsedMscheck would fail; existing assertions still requiregetEmissionCount() > 3, ignored/control frame trace counts, and idlereasonwith onlymessage_startyielded.
Automated review audit
| Source | Thread / check ID | Status on head | Disposition |
|---|---|---|---|
| CodeRabbit | Issue comment #6330167366 | Summary / in-progress UI only; no review line comments | Drift — nothing actionable to promote |
| CodeRabbit | Commit status context CodeRabbit |
Review approved | No open bot findings |
| CodeQL | Check runs CodeQL, CodeQL (javascript-typescript), Analyze (javascript-typescript) on 1cf50c499d2e2d231d1eb026c49c8cd65cb0abf5 |
Completed success | Drift — no PR-introduced alert; test-only diff |
| CodeQL | Earlier aggregate neutral “configuration not found” note |
Superseded by successful javascript-typescript run on head | Drift — environmental, not author fix |
Findings
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
Summary
keepalives and parsed-but-ignored frames cannot reset QueryGuardinsrc/services/api/codexShim.interruption.test.ts.elapsedMsis 250). All four frame variants, the 25 ms interval, the 250/500/1,500 ms deadlines, and every existing assertion are unchanged. Cleanup restores real timers and settles the consumption promise before restoring trace state.main.Impact
Testing
Repro and fix, fresh processes with isolated
HOME/TMPDIR:bun test --isolate --feature=UNATTENDED_RETRY --max-concurrency=1 ./src/services/api/codexShim.interruption.test.tsEvery "before" failure was this test (two or three emissions, then the idle deadline fired). On the rebased head, I repeated the 24-worker batch: 32/32 passed.
Whole suite in one process:
bun test --isolate --feature=UNATTENDED_RETRY --max-concurrency=1, run twice on the final head: 10,485 pass, 0 fail, 3 skip across 780 files each time.bun run check: passbun run typecheck,bun run typecheck:type-tests: passnode bin/openclaude --version,NODE_DISABLE_COMPILE_CACHE=1 node bin/openclaude --version: passbun run security:pr-scan -- --base FETCH_HEAD --head HEAD: passgit diff --check: cleanbun run checkabove ran on the previousmain. After rebasing onto the currentmain, whose two new commits don't touch this file, I reran the changed file together withsrc/utils/ShellCommand.test.ts10/10, plusbun run typecheck,bun run check:static, and the intent scan, all passing. In two earlier heavy-contention batches (before the final clean ones), a different, untouched test in the same file hit Bun's default per-test timeout once; it didn't recur in the final batches.Notes
Summary by CodeRabbit