Repository navigation
test: prove actual crashes in WSL auth recovery scenarios - #29507
Merged
Merged
Conversation
…aft-workflow-contracts
…ain-crash-receipts
OrcaWin
changed the base branch from
OrcaWin/test-audit-draft-workflow-contracts
to
main
October 16, 2026 07:19
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI5
Crash-recovery tests now prove that the script was actually killed. Five existing tests could pass after an ordinary error instead of a crash, giving false confidence that restart recovery worked.
What Changed
Keep all 33 scenarios and their filesystem assertions. Capture the initial apply process's typed status and signal, reject subprocess errors, and require SIGKILL in the seven crash cases. Only perform simulated post-crash mutations and inspection after that signal was observed. Clean each of the three fixture root directories in finally, after taking the result snapshot. No production code, generated scripts, interruption targeting, timeouts or CI routing change.
Why
The old harness flattened signal deaths, timeouts and other errors into a nonzero status. Graceful failures can run cleanup traps; SIGKILL cannot. Checking the actual signal distinguishes the two without adding a new injection hook or replacing the real shell scripts with mocks. Finally cleanup also covers failing assertions and setup/snapshot errors.
Linked Issue
Internal CI test-quality audit. #29499 is merged; this PR now targets main with the full-suite blockers repaired.
Visual Proof
N/A — test support and assertions only.
Testing
Linux Bun 1.4.3 and native Node 24.21 pass the actual SIGKILL cases. Candidate passes the apply/orchestration/rollback/interruption-source suites; native Node also passes the standalone session bridge sibling. Final five-suite verification: all 80 cases pass. Node typecheck, changed-code quality, normal lint, formatting and whitespace checks pass. All 33 declaration names and the 26 non-crash cases remain unchanged; both embedded shim payloads and production source remain byte-identical.
Controls preserve the owned-process ancestry checks and replace only the final kill. The original graceful-error control passes five of seven crash cases. Candidate graceful-error and suppressed-kill controls each fail all seven at expected SIGKILL / received null. Production and candidate bytes restored exactly afterward. Candidate normal runs and both failing controls leave zero fixture roots in an isolated task-owned temporary directory; the original control leaves seven. The outer runner removes only its own directory after recording evidence.
Local four-suite module observations: baseline 5,231 ms, candidate 5,068 ms; process wall 6,944 → 6,925 ms. Single unpaired observations, with typecheck contention during the candidate, do not establish a speed difference. This is a proof/resource-cleanup repair; no speedup claimed. Exact-head full PR run 38866618636 passed, including aggregate PR checks passed and all five clean unit shards. Five target/sibling suites passed 80 cases with no skips; official summed worker module cost 4,860 ms (four drain suites: 68 cases / 4,771 ms). Source/runtime qualified; Node 24.19 / Bun 1.4.3. No speed comparison claimed. Initial run 38864477865 was cancelled once the inherited browser-bridge ownership failure was confirmed on the same base; the branch is now refreshed onto main and requires a new full aggregate before merge. Refreshed local four-suite run passes with zero owned roots left; wall observation 6,920 ms. Independent review confirmed all affected bytes and prior controls remain unchanged. Native Windows skips the POSIX shell suite, as before; no native WSL or SSH runtime claim.
Review
Independent structural review approves the four-path change and byte preservation. Independent final review inspected all raw control failures, both runtime runs, cleanup observations and final 80-case run; approved.
Agent skill upstream boundary
Notes
No product, platform, SSH, folder, provider or wire behavior changes. Fixture deletion is resource cleanup, not proof of process-tree termination. Test/support net change: +20 lines; production: 0.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)