Repository navigation
Stabilize Windows PTY and login test timing (VibeTV #336) - #20
marcus7989 wants to merge 2 commits into
Conversation
DreamyTalesPAN
left a comment
There was a problem hiding this comment.
Counter-review of #20 at head ba43f0d2113aeb6ded6490399eff729fd770bd4c (read-only review, nothing built or pushed on my side).
Verdict: correct, test-only, no hiding of real failures. No blocker. Two non-blocking notes.
- Product code untouched. In
rust/src/managed_process.rsall hunks are insidemod tests(opens at line 829, hunks start at 1056). Inlogin.rsthe test module opens at 377, the hunk is at 582. Production prefixes are unchanged in both files. - Timeouts not loosened. The 15 s marker wait and the 10 s shutdown/cleanup limits stay as they were. The login test no longer races a 10 s budget against PowerShell startup; it reaps the fixture first and then calls
wait_for_login(..., Duration::ZERO). That is valid becausewait_for_logincallstry_wait()before it checks the timeout, so an already-reaped child takes the exit-status path, not the timeout path. - Assertions are stronger, not weaker. The failed fixture must now exit with code
Some(1)and the error must containexit code 1(matchesLoginFailure::CliExit). The running-process timeout/cancel path is still covered by the untouchedcancelled_and_timed_out_logins_reap_child. - PTY containment test still tests the real thing. It still spawns a real descendant, checks it is alive, runs the real Job Object
shutdown(10 s)and asserts the descendant dies. Using the native test executable instead of two PowerShell starts removes only the startup cost. The new fixture test is a no-op unlessCODEXBAR_MANAGED_DESCENDANT_MARKERis set, so it is harmless in a normal suite run. The new early-exit assert and the richer timeout message are an improvement. - Non-blocking 1:
child.wait()in the login test has no deadline, so a hung PowerShell would hang the test until the job timeout instead of failing with a message. Acceptable for a test-only change, but worth knowing. - Non-blocking 2 (evidence): this PR has no CI run on its own head (
no checks reported). The runtime evidence is from VibeTV nesszer#336, where the same patch was applied to .16, and the PR text says so honestly. The files are byte-identical on the .15 base. The full suite stays red onrestart_uses_manifest_gui_and_leaves_standalone_cli_aloneuntil #21 lands, which fits your merge order #21 → #20. After #21 is merged, please check one full-suite run on thevibetvresult.
Not checked: a real-Windows run on my side; I relied on the linked runs.
|
[K-Marcus] @codex review W307 addressed Paul's unbounded This budget applies only before Preparing native full library + CLI CI on an isolated #21 + this head source, plus the four-PR cumulative source. These are pre-merge test candidates, not target-branch merges. After #21 is actually integrated, one full suite on the actual |
|
[K-Marcus] W307 native full suite GREEN on isolated #21 + current #20 source The post-integration suite is prepared: verify actual |
[K-Marcus] Windows PTY/login fixtures raced PowerShell startup against test budgets. Use the native test executable for the real PTY descendant, and reap login fixtures before
wait_for_login(..., Duration::ZERO). The fixture wait is now bounded to 60 seconds: a stalled child is terminated and produces a clear timeout error, addressing Paul's counter-review. A new test verifies timeout and child termination.Only
#[cfg(test)]code inrust/src/managed_process.rsandrust/src/providers/claude/accounts/login.rschanges. Product code, the 15-second descendant marker wait and 10-second shutdown/cleanup limits are unchanged. Live descendant/Job Object assertions, isolated account identity, failed exit code 1 and its error message remain checked.Validation for exact PR head
762beabcb4d817e07de7e807d6cb5b6124279e21together with prerequisite #21c8e11559ed78df845a1ee732e511025b458d1320:e749083ad573fed8815c204c60e2a5824af944a0, native full suite: 2,038 library + 1 CLI passed, 0 failed, 1 pre-existing ignored, 0 filtered. Includes the new stalled-fixture test. Rust 1.98.0, locked MSVC x64, static CRT. The tested files are byte-identical to the named PR heads; this is a separate cumulative source, not a green check on standalone Stabilize Windows PTY and login test timing (VibeTV #336) #20.ef4aace55a59ed61fc66ab9b7d341129b82afa69, suite: 2,049 library + 1 CLI passed, 0 failed, same one ignored. Doc tests have 0 cases.Paul's counter-review was on ba43f0d and had no blocker; his unbounded-wait note is fixed here. Codex review requested on this new head, still pending. Keep Draft; no merge approval. Actual post-integration full-suite rerun is prepared and remains required after #21/#20 are integrated. No devices, merge, release or .16 pin changes.
Additional repository-required native Desktop-shell unit suite on unchanged cumulative source
ef4aace55a59ed61fc66ab9b7d341129b82afa69: run 40908176267, 431 passed / 0 failed / 0 ignored / 0 filtered. Combined backend, CLI and Desktop-shell total: 2,481 PASS / 0 FAIL / 1 pre-existing ignored, same exact cumulative source. Both Rust manifests are verified; no app/device/customer-flow test is claimed.The current-head Codex request has now had at least 20 minutes without a bot review, inline finding or bot reaction. Review remains pending; no automated no-findings or merge readiness is claimed.