Repository navigation
cmuxTests: stop grading the CLI spawn on a five-second stopwatch - #8867
Conversation
|
Marking this draft until I have compiled it. The change is twenty lines of test-only edit and a named constant, but |
📝 WalkthroughWalkthroughThe CLI session list tests introduce a shared 60-second process spawn timeout and replace seven hardcoded five-second ChangesCLI test timeout standardization
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a load-sensitive flaky test by replacing hardcoded 5-second timeouts with a named constant set to 60 seconds in
Confidence Score: 5/5Safe to merge — the change is isolated to test scaffolding and makes existing flakiness-prone assertions more reliable without altering any behavior. The only file touched is a test file under cmuxTests/. The constant is immutable (private let), well-documented, and replaces magic numbers at exactly the call sites that caused the reported flake. No production code, no logic changes, no new assertions added or removed. Files Needing Attention: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test calls runProcess] --> B{cliSpawnTimeout = 60s}
B --> C[cmux binary spawns and emits JSON]
C --> D{timedOut?}
D -- No --> E[#expect status == 0, JSON parse, field checks]
D -- Yes/hang --> F[Test fails: output truncated, assertions cannot pass]
E --> G[Test passes]
Reviews (3): Last reviewed commit: "cmuxTests: stop grading the CLI spawn on..." | Re-trigger Greptile |
|
Build check came back FAILED with 7 errors — and none of them are from this change. All seven are
Re-running the check with this stacked on #8857, since that is the only way to build anything today. Staying draft until that comes back green, then I will flip it with the result. |
|
Verified stacked on #8857 (the only way to compile anything until it lands):
|
testSessionsListTreatsTranscriptBackedClaudeRecordAsRestorable failed on a loaded
builder with:
Expectation failed: !(result.timedOut)
ProcessRunResult(status: 0, stdout: "{ ...complete sessions JSON... }", timedOut: true)
The command had succeeded. It exited 0 and emitted the full, correct JSON the rest of the
test then parses; it just took 6.0s against a 5s budget while sixty other suites were
running on the same machine. The same commit passed on an idle box, so the suite's verdict
was decided by machine load.
These cases fork the real cmux binary and wait for it to emit JSON, so their wall-clock
cost is a property of the host, not of the behaviour under test. The timeout is there to
catch a hang, and the assertions that follow each call already grade correctness: status,
then a JSON parse, then the field checks. A process killed at the deadline cannot satisfy
those, because its output is truncated.
So the budget moves to a named constant set far above any healthy run, with the reasoning
next to it. Same seven call sites, no behavioural assertions changed.
Note the rest of cmuxTests carries 490 more `timeout: 5` call sites across 30 separately
copy-pasted runProcess helpers. That is the same trap thirty times over and wants one
shared helper, but it is a refactor of its own rather than a rider on this fix.
43d5c96 to
ed9cde1
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Updated the PR description to the required template and clarified that the timeout is a process liveness deadline, while status/JSON/field assertions independently grade correctness. The later review pass tightened the current value to 20 seconds; see the follow-up below. Re-requested review after merging current @codex review |
|
To use Codex here, create a Codex account and connect to github. |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 5 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
|
Addressed the structured-review timeout concern on the latest HEAD: the shared liveness deadline is now 20 seconds (over 3× the observed loaded runtime), so every same-path invocation gets load headroom without allowing a serialized hang to stall for minutes. The touched test file is also back below 500 lines. @codex review |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 5 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
…-not-a-speed-gate
2 participants
Summary
runProcessdeadlines in the CLI sessions-list regression suite with one 20-second liveness deadline.The original failure occurred on a loaded builder after 6.0 seconds even though the command exited 0 and returned complete, correct JSON. The same commit passed on an idle builder, so the old deadline made host load part of the test verdict. Twenty seconds gives more than three times the observed loaded runtime while still detecting a genuine hang promptly.
This change intentionally stays within the seven calls sharing the same runner. Other five-second values in the repository have different meanings, including protocol waits, so a repository-wide process-runner consolidation should be a separate, semantics-aware refactor rather than a blanket replacement.
Testing
git diff --check./scripts/lint-pbxproj-test-wiring.sh— passed for 602 test filescmuxTests/CMUXCLISessionsListTranscriptPidTests.swiftis 499 lines and the PR does not touch either Swift budget TSVDemo Video
Review Trigger (Copy/Paste as PR comment)
Checklist