Repository navigation
fix(link): keep Windows containment refusal at 126; correct the #8100 fan-out test fixture - #8106
Merged
Merged
Conversation
…n-out test fixture #8100 packed every Windows remote command and inflated it before the containment helper existed, outside any try. A Child whose PowerShell runs in Constrained Language Mode could not inflate it and exited 1, which the Home reads as an ordinary ocx failure. Guard only the inflate so it exits 126; Invoke-Expression stays outside the guard so the inflated script's own errors and exit codes pass through. The new local-only fan-out test spawned the CLI with a stripped POSIX environment (no SystemRoot, a ':' PATH, a sh-script ssh) and without the owned-service-home preload that keeps Codex write admission away from the runner's real Task Scheduler. It timed out on Windows and could not prove "no ssh" there at all. Give CLI children the inherited environment and the owned-service-home fixture, add a Bun-spawn ssh sentinel with positive controls, and prove local-only in process as well.
…es in the Windows contract
lidge-jun
added
the
ci:full
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
intake: hygiene-blocked
Contributor
|
Contributor
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Inflation error handling and coverage src/link/remote-windows.ts, structure/remote-link.md, tests/clients/link-ssh-argv-discovery.test.ts, tests/clients/link-pinned-sync-argv.test.ts, devlog/_plan/261103_fix_8100_windows_batch/010_plan.md |
packedPowerShellArgv catches failures while decoding and reading the inflated script, then exits with status 126. Invoke-Expression remains outside the catch. The documentation describes the Windows command behavior. Windows tests cover containment refusal, script errors, and the pinned-sync watchdog. The plan records regression cases and verification steps. |
Sync fan-out test isolation
| Layer / File(s) | Summary |
|---|---|
Child-process harness and SSH sentinel tests/cli/sync-command-fanout.test.ts, tests/helpers/ssh-spawn-sentinel-preload.ts, devlog/_plan/261103_fix_8100_windows_batch/010_plan.md |
Subprocess tests use an inherited environment, an owned service-home preload, and a 12-second timeout. The sentinel records and rejects SSH launches through Bun.spawn and Bun.spawnSync. The local-only and damaged-link-store tests use the shared runner. The plan records the fixture changes and verification steps. |
Local-only fan-out assertions tests/cli/sync-command-fanout.test.ts |
An in-process test compares behavior with and without --local-only. Without the flag, status lookup, the SSH runner, and a child process are reached. With the flag, only Home sync runs and JSON reports fan-out disabled. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related issues
- lidge-jun/opencodex#7297: The issue requests durable diagnostics for Remote Link failures. This change assigns a distinct exit code to Windows containment refusals but does not add durable logging.
Possibly related PRs
- lidge-jun/opencodex#7832: Introduced the Windows Remote Link path that this change updates.
- lidge-jun/opencodex#7496: Established earlier Windows Remote Link refusal behavior and documentation.
- lidge-jun/opencodex#8100: Introduced Home sync fan-out, which the updated tests exercise.
Merge Risk: 
1800b
-
Windows constrained-language failures can still lack the documented refusal explanation, but the core containment exit code and fan-out test behavior are otherwise preserved.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies both primary changes: preserving Windows containment refusal exit code 126 and correcting the #8100fan-out test fixture.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.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 5 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
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
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.
Contributor
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
removed
the
intake: hygiene-blocked
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…ors out of 126 Address review: the local-only subprocess case now asserts the Home outcome its fixture is built for, and a Windows case shows an error raised by an inflated script keeps its own exit instead of the containment refusal (packedPowerShellArgv is exported for it).
… and shorten the pinned-sync watchdog case PR test 4/4 at 7f4924e hit the 15-minute job timeout (job 125416446261) with no failing test. Its merge with dev ran tests/update/update-orchestrated-entry.test.ts (#8103) for 98 s, and like the other files #8099, #8100, #8103 and #8104 added it had no recorded duration and weighed the 176 ms median: the shard predicted 728 s and ran 852 s of tests. Both tables are refreshed with scripts/ci/test-durations.ts. Linux/macOS: run 41462914814 (dev 09b5585, Linux and macOS shards) plus the Linux shards of pull_request runs 41485932590, 41482553737 (#8103) and 41483744349 (#8104); every test file now has a row and the four Linux shards predict 716 s each. Windows: run 41462914814's eleven shards; the #8103/#8104 files and three files of the two stopped shards have no Windows measurement yet and still weigh the median. The POSIX "version writer ignoring signals" case waited out the production ten-second watchdog. It now runs the same script with a one-second watchdog and asserts the shipped script keeps "sleep 10": the case drops from about 10.4 s to 1.4 s.
Owner
Author
|
Maintainer integration into Exact-head verification for
|
3 tasks done
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.
Summary
Fixes the two Windows shards that turned the
devbatch red at09b558580(run 41462914814). Both come from #8100 (4948c04d9). The last tree before it (be51cb817, run 41455278872) was green on every platform.windows 10/11: an uncontainable Windows ocx exited 1 instead of 126 (product fix). #8100 sent every Windows remote command deflated and inflated it on the Child (
[IO.MemoryStream]::new,DeflateStream, thenInvoke-Expression) before the containment helper and itsFail → exit 126existed, outside anytry. Constrained Language Mode, the AppLocker/WDAC casestructure/remote-link.mdnames, blocks those types, so the session ended with 1. The Home reads 1 as an ordinary ocx failure; 126 is whatclassifySyncResultmaps tochild-ocx-not-contained. Nothing ran uncontained, but the refusal was misreported.packedPowerShellArgvnow guards the inflate alone with$ErrorActionPreference='Stop'; try{…}catch{…; exit 126}.Invoke-Expressionstays outside the guard, so the inflated script's own errors and exit codes pass through unchanged. The encoded commands grow by 416–420 characters (native 5,320 → 5,736, discovery 5,820 → 6,240, pinned sync 6,924 → 7,344, all under the 8,000 limit).windows 2/11: the new fan-out test's fixture was wrong on Windows (test fix, no product change).
tests/cli/sync-command-fanout.test.ts"local-only JSON does not invoke ssh" was added by #8100 and timed out at 15 s with empty stdout. Evidence that the fixture is at fault:--local-only,runHomeFanoutreturns before reading the store, the status API or any SSH runner. What runs is the Home sync feat(sync): Home ocx sync fans out to linked Children (sync.home-fanout/1) #8100 moved fromdispatch.tsunchanged.{HOME, OPENCODEX_HOME, CODEX_HOME, PATH: "<temp>:/usr/bin:/bin"}: noSystemRoot/USERPROFILE/PATHEXT, a POSIX delimiter, and a#!/bin/sh"ssh" Windows cannot run. So the "no ssh" assertion proved nothing on Windows.claimOwnedServiceHome+withOwnedServiceHomePreload, which every subprocess test reaching Codex write admission uses because the Windows probe runs the machine's real System32schtasks.exe/sc.exe(and can fall back to a 20 s listing, Windows (zh-CN): ocx sync always refuses — localized schtasks output breaks the task-absence probe, and the 2s full-listing fallback times out #2914).tests/cli/cli-restore-back.test.ts,ocx syncwith Codex on) passed on Windows in this same batch in 688–721 ms.The log discarded the child's stderr, so the exact blocking operation was not observed. The Windows dispatch on this head is the check. CLI children in this file now get the inherited environment with temp homes, the owned-service-home fixture, a delimiter-correct PATH, a 12 s spawn timeout under the 15 s case budget, and the child's stderr in the failure message. "No ssh" is now proven on every platform: a new test-only preload (
tests/helpers/ssh-spawn-sentinel-preload.ts) stopsBun.spawn/Bun.spawnSyncofssh/ssh.exeand records the attempt. Positive controls check both APIs, pass-through, and that the Windows service fixture still answers beside it. A new in-process case shows--local-onlynever reaches link status, the SSH runner or a Child process, even with a committed catalog and a connected link. Its positive half shows the same spies are reached without the flag. The subprocess case also pins the Home outcome its fixture is built for ({state: "failed", exit: 1}with the Codex-home refusal on stderr). No assertion was removed and no timeout was raised.CI shard balance (requested after PR test 4/4 at
7f4924ec4hit the 15-minute cap with no failing test). The cancelled shard's merge withdevrantests/update/update-orchestrated-entry.test.ts(#8103) for 98 s. Like every other file #8099, #8100, #8103 and #8104 added, it had no recorded duration and weighed the 176 ms median, so the shard predicted 728 s and ran 852 s of tests. Both tables are refreshed withscripts/ci/test-durations.ts, as #8102 did. Linux/macOS: batch run 41462914814 (Linux and macOS shards) plus the Linux shards of PR runs 41485932590, 41482553737 (#8103) and 41483744349 (#8104). Every test file now has a row, and the four Linux shards predict 716 s each. Windows: run 41462914814's eleven shards. The #8103/#8104 files and three files from the two stopped shards have never run on Windows, so they still weigh the median there. The branch mergesorigin/dev(e4ced1838) so those rows exist. One #8100 case waited out the production ten-second watchdog ("a version writer ignoring signals is killed…"). It now runs the same script with a one-second watchdog and asserts the shipped script keepssleep 10, so it takes 1.4 s instead of about 10.4 s.structure/remote-link.mdnow describes whole-script packing and when 126 happens, which is also the stale comment #8100 left behind.Plan and audit trail:
devlog/_plan/261103_fix_8100_windows_batch/010_plan.md.Verification
bun test tests/cli/sync-command-fanout.test.ts tests/clients/link-ssh-argv-discovery.test.ts tests/clients/link-pinned-sync-argv.test.ts tests/ci-workflows/ci-shard-balance.test.ts tests/test-layout.test.ts: 62 pass, 14 skip (Windows-only), 0 fail.sync-command-fanout: 11 pass (macOS). Removing thelocalOnlyshort-circuit inrunHomeFanoutfails both the subprocess case and the new in-process case.bun test tests/clients/link-ssh-argv-discovery.test.ts tests/clients/link-pinned-sync-argv.test.ts: pass on macOS. The Windows-only cases skip: two new Constrained Language Mode cases (discovery and pinned sync) and the new post-inflation error case.bun run typecheck,bun run structure:check,bun run privacy:scan: pass.bun scripts/test.ts --changed=dev --parallel=1: the default run hit the harness's 900 s whole-run cap with 15,746 passes and 0 failures. Rerun withOCX_TEST_MAIN_TIMEOUT_MS=3600000(a local wall-clock cap, not a test timeout): 33,691 tests across 1,459 files, 26 failures. All are server/proxy cases that block for 5–20 s on this shared, loaded host. None is in the link or sync area. They flake either way: an A/B rerun ofcombo-replay-refusal-attributionwithdev'sremote-windows.tsfailed once and passed once, and so did this branch, andserver-searchfails the same two cases ondev's file. The four full Ubuntu shards (ci:full) ran the same suites and passed on this PR.1800b4d7f: run 41491122166. Every case this PR targets passed: "a Windows ocx that cannot be contained in a kill-on-close job is refused before it runs" (184 ms), both new Constrained Language Mode cases, the post-inflation error case, the exit-23/stdin, Desktop-first discovery, kill-on-close and pinned-sync success paths (windows 1/11, 4/11), and "local-only JSON does not invoke ssh" (711 ms, was a 15 s timeout), together with the sentinel controls (windows 3/11). Linux test 1/4–4/4 passed in 710–812 s and macOS 1/4–4/4 in 660–928 s. Four Windows shards are red, none in code this PR changes:tests/update/update-bun-ownership-lease.test.ts"… replaces only the package and releases its lease" (Installer termination was not confirmed).tests/update/update-orchestrated-identity.test.ts"identity requires a stable, attested runtime record" (POSIX fixture paths,kind: "unknown").devafter the last Windows batch, so this was their first Windows run. They will fail the nextdevbatch whether or not this PR merges.tests/claude-integration/claude-intercept-acl-barrier.test.ts. It setsOPENCODEX_ACL_TIMEOUT_MS=1000, and both cases ended at that budget during a runnericaclsstall. It passed in 220 ms in run 41462914814.codex-cli-update-windows-apply-installat 216 s against 153 s. Eleven Windows shards at about 22 minutes of predicted tests leave little headroom under 30.9ed997ac4), fresh final approval APPROVE on7f4924ec4. CodeRabbit: two findings adopted in7f4924ec4. The third, the containment message being silent under Constrained Language Mode, is rebutted in its thread as pre-existing and outside this fix; a specific enrollment message for 126 is a possible follow-up.Checklist