[WRONG BRANCH] Merge dev into preview: Windows suite green - #1907
Merged
Conversation
Codex-login 5.6 rows advertised 922k with no user action. Follow the live catalog default of 272k, and let the native switch or overlay raise to the measured 922k ceiling. gpt-5.4 stays 1M; the API-key 1.05M path is unchanged.
…sktop writers Pass full nativeContextLimits into model-info, catalog, and Desktop apply paths so a 1M overlay is not dropped. Keep the combo fallback ceiling on the 5.6 family so a GPT-5.4 native alias does not inherit 922k.
Bare icacls.exe on PATH threw ENOENT under a bun-shim environment and was reported as missing NTFS ACL support, which blocked ocx service install. Use GetSystemDirectoryW like schtasks/powershell, and classify spawn failure as EICACLS.
The Windows coordinator namespace resolved LocalAppData through .NET GetFolderPath(SpecialFolder.LocalApplicationData), which follows USERPROFILE and returns an EMPTY STRING -- not an error -- when the profile it computes has no AppData directory on disk. Any caller with a redirected USERPROFILE therefore refused every coordinator lookup with "Windows effective-account lookup returned an empty value", which is precisely the environment dependence this module exists to eliminate. The suite hid it by handing each child the real profile back, so the defect read as unrelated assertion failures across locking, transition-state, catalog serialization and sync. Use SHGetKnownFolderPath with a null token and KF_FLAG_DEFAULT_PATH instead: it reads the known-folder registration for the effective token, returns the real per-user path whether or not the directory exists, and is unaffected by USERPROFILE, LOCALAPPDATA, HOMEDRIVE or HOMEPATH. A non-null token is NOT equivalent: passing (HANDLE)-1 resolves the built-in Default profile, which would key coordination to a namespace no real account writes to. The write-lock contention child published its hold marker with Bun.write, whose write only lands on a later event-loop turn. The callback that follows is a synchronous busy wait by contract, so the marker appeared ~3s late, after the hold had already ended, and the contender met an unheld lock and reported acquired where the test demands busy. Write the marker synchronously. The symlink spelling case needed Developer Mode to create a directory symlink; an NTFS junction needs no privilege and exercises the same realpath canonicalization, so the invariant stays proven on an unelevated machine.
…st isolation Six failures on an unelevated Windows checkout, none of which were product bugs in the code they pointed at: The test sandbox moves HOME, and git resolves ~/.gitconfig from HOME, so the developer's `safe.directory` became invisible to every git call a test made. On a checkout whose directory owner differs from the running account -- ordinary on Windows when a tool or installer created the tree -- git then refused with "detected dubious ownership", the adapter read that as "not a git repository", and command-code asserted against its empty fallback. Pin GIT_CONFIG_GLOBAL to the real file before HOME moves; the sandbox is unchanged, since git writes nothing there. claude-management-api spoofed process.platform globally, which sent Windows management-token initialization down the POSIX ACL path and answered 503 before the assertion under test was ever reached. Project the capability through an explicit management dependency instead, so the platform under test is named rather than impersonated. codex-sqlite-home asserted a POSIX-shaped literal for a relative-path resolution whose point is anchoring, not spelling; every neighbouring case already spells it through resolve/join. codex-history-reachability compared backslash paths against a forward-slash inventory, so the named permitted module could not match itself. codex-catalog-writer asserted chmod through stat mode bits that Windows only synthesizes, while the recorded harden effect proves the same transition. cli models and the catalog resync exceeded Bun's 5s default while doing real multi-process CLI work, and now use the repository's existing spawn budget. codex-config-generation created fixtures under tests/ and replaced its sandbox root with a file, so a failed SQLite open kept a Windows handle and teardown left the directory behind; it uses the OS temp dir and a directory at the database path, keeping the typed-error coverage. The catalog-sync workaround that handed children back the real USERPROFILE is removed: the defect it described is fixed at the source in the parent commit, and a workaround outliving its cause only hides the next regression.
…of the code The bare-PATH resolution case builds its launcher with a file symlink, which needs Developer Mode or admin on Windows and failed with EPERM before the probe under test ever ran. No privilege-free substitute preserves what it proves: the resolver follows the PATH entry through realpath into `@openai/codex/bin/` to reach the sibling platform package, and a copy erases that association, a hard link reports its own path as its realpath, and a .cmd wrapper is never matched for a bare command. Report a visible skip where the OS withholds the privilege, in the shape claude-agents-inject and codex-service-manager-probe already use. The key-delegation case called codexFeaturesInvocation with no seams, so it read the developer's own Codex install. Where that install is the npm codex.cmd, the invocation is correctly wrapped in `cmd /d /s /c` and the raw-args assertion failed -- describing the machine's install shape, not the delegation under test. Name the platform and resolution seams, exactly as the invocation-shape case further down the same file already does.
…values rootTomlString and providerTableString returned the raw bytes between the quotes, so a basic TOML string was never unescaped. On Windows that matters immediately: a path is written as an escaped basic string, so reading it back yielded doubled backslashes and a value that matches nothing on disk. The journal records injectedCatalogPath through exactly this path, so restore after a Codex app rewrite could not recognize the catalog it had written itself (#1798). paths.ts already had the correct reader -- readRootTomlString captures the quoted value and decodes it with parseTomlString. These two helpers are the same idea spelled a second time without that step, which is why the divergence went unseen on POSIX, where an escaped path and its raw bytes are usually identical. Capture the value with its quotes and decode it through the same parser rather than maintaining a second, subtly weaker interpretation of the format.
The effective token's SID and its known-folder local AppData were re-derived by a fresh PowerShell on every call: about 150ms and 310ms respectively, and the coordinator asks for both on every config write and lock acquisition. Neither can change without a new logon token, and both lookups deliberately ignore the environment, so the second spawn only re-establishes what the first already knew. On Windows that overhead was not merely wasteful: it pushed real multi-process injection tests past their budget, where they timed out at 5s while doing genuine work. Memoize successful lookups for the process lifetime -- roughly 510ms to 1ms for a coordinator path resolution. Refusals are not cached, so a transient failure cannot pin a process into a permanently refusing state.
…und fixtures The core/Lab boundary test never ran on Windows. It built its repository root from `new URL(import.meta.url).pathname`, which yields "/C:/..." there, so resolving it produced "C:\\C:\\..." and every case threw ENOENT while opening its own sources. Two further spellings assumed POSIX separators: the walk matched the literal "/src/lab/", which no backslash path can contain, and the reported chain kept the native separator so the attack cases could not match it. That combination matters more than a red test. This guard exists because the original violation hid in a six-hop import chain and pulled ~69 Lab modules into every install; with the path broken it would have reported clean for a real Lab import exactly as it did for a missing file. Its own adversarial cases now fail before the fix and pass after it, which is the evidence that it is live again. config.ts dotfiles cases need a file symlink, which no privilege-free construct substitutes for, so they take the visible skip this repository already uses for the same constraint. The DSH settings case asserted 0o600 through stat, but Windows synthesizes mode from the read-only attribute and always answers 0o666; assert the file exists everywhere and the permission bits only where they mean something.
… own file rebuildLabProjection closed its database without finalizing the statements it had prepared. Bun keeps a prepared statement alive until it is finalized or collected, and on Windows an outstanding statement holds the file open: `close()` leaves the handle behind and `close(true)` throws "database is locked". The next rebuild then could not unlink the projection it was replacing, and the retry loop in wipeSqlite could only convert that into a slower failure -- "failed to remove stale projection file after retries". POSIX permits unlinking an open file, which is why a rebuild that is deterministic by contract was only ever non-deterministic on Windows. Collect the prepared statements and finalize them before the close. This is the real defect behind ten Compatibility Lab failures across the ledger, fabric-task and public-evidence suites, all of which called rebuild more than once. Two server tests also exceeded Bun's 5s default while binding real proxies: the Retry-After case runs two full pool-passthrough cycles and the #702 case binds one proxy per route class to prove none of them reaches upstream. In both the servers are the assertion, so they take the existing SERVER_BUDGET_MS rather than a new knob.
The contention test released its holder and dropped the exit promise on the floor, so afterEach could remove the temp root while that child still had the coordinator database open. Windows refuses to unlink a file another process holds, so teardown threw EBUSY and the failure was attributed to a test that had already proved its assertion. POSIX unlinks an open file regardless, which is why this only ever appeared on Windows, and only under full-suite load where the child exits slower. Await the holder, and let teardown retry briefly before giving the directory back to the OS: `force` covers a missing path, not a locked one, and a temp directory left behind is a smaller lie than a green test reported red.
Filling the affinity cap persists CODEX_THREAD_AFFINITY_MAX_ENTRIES real mappings, and that store work is the eviction proof rather than incidental setup. On Windows the pair sits right on Bun default of 5s -- one measured 5.7s and its neighbour 5.25s -- so the cap test failed on load while the test beside it passed by a quarter second. Both take the existing STORE_BUDGET_MS.
The isolated Codex home rethrew when its temp tree could not be removed. On Windows a proxy or child that is still shutting down can hold a file there past the 2.5s retry budget, and the throw landed in afterEach -- so a test that had already asserted everything it claims was reported red, and the red pointed at whatever happened to run in that slot rather than at an OS release race. The env restore is the part other tests depend on and still runs unconditionally; the directory is disposable. Leave it to the OS when the retries are exhausted. The rate-limit E2E teardown had the same shape with a worse consequence: a failed removal skipped the clearKeyCooldowns() call after it, leaking cooldown state into the next test.
completeMockCodexOAuth waited between login-status polls with queueMicrotask. A microtask only yields to work already queued, but the login flow awaits real I/O -- credential reads and the WHAM fetch -- so under load its continuation lands on the macrotask queue and 500 microtask turns can pass without it running once. The flow then reached its own 150-poll ceiling and reported "Login timed out before OAuth completed" where the test asserts a specific commit-failure message, which reads as a behavioural regression rather than a starved poller. setImmediate yields past the microtask queue, so each poll observes the state the flow actually reached.
The refusal is only proven by letting a connection attempt reach its own 2s socket timeout, on top of starting and stopping a real proxy and listener. On a loaded Windows box that measured 5.04s against Bun default of 5s, so the case failed for the wait that IS its assertion. Use the existing SERVER_BUDGET_MS.
discoverProjectCodexConfigPaths walks up to 12 parents, and on Windows the OS temp directory lives under C:\Users\<user> -- so the fixture's walk climbed out of the fixture and found the developer's real ~/.codex/config.toml. The identity check cannot exclude it, because it genuinely is a different file from the fixture's codexConfigPath. Bound the walk; the assertion is that a parent walk does not rediscover the global config, not how far it may travel. The claim-narrowing case asserted 0o644 before and 0o600 after, but Windows synthesizes mode from the read-only attribute and answers 0o666 regardless, so neither end of the transition is observable there. The call still runs on every platform; only the POSIX-shaped observation is conditional. The auth-temp residue case needs a real file symlink to prove it refuses to follow one, and that needs Developer Mode or admin. Take the visible skip; the hard-link case beside it still proves the scrubber will not truncate a shared target here.
…wn race Four responses-state cases are irreducibly about symlink resolution -- following a symlinked snapshot to its real directory, or refusing an oversized or non-regular one -- and creating a file symlink needs Developer Mode or admin on Windows. They failed in the fixture, before the behaviour under test ran. Detect the privilege once and take the visible skip this repository already uses for the constraint. The CL-06 boundary teardown removed its temp root unconditionally and threw EBUSY when a shutting-down server still held a file there, failing a test that had already asserted. The state that matters is reset before it; leave the directory to the OS.
Starting two real servers, driving a policy job to idle, and stopping both IS the assertion that one stop leaves the other process-wide work alone. That sequence measured 5.4s against Bun default of 5s on Windows, so it failed for its own evidence. Use the existing SERVER_BUDGET_MS.
inspectNpmCacheDirectory judges accessibility from POSIX owner bits, and a Windows directory reports 0o666 with no execute bit -- so the owner-rwx check can never pass and every inspection answered cache_entry_inaccessible. That is not a defect to fix: the module inspects a Unix npm cache, and runNpmCachePreflight already returns windows_skip before reaching it. The worker round-trip case additionally spawns the real npm while claiming a non-Windows platform, which is slow and proves nothing here. Both are now explicitly non-Windows, and the windows_skip case beside them still covers the branch this platform actually takes. Three real-home guard cases and three npm-cache cases need genuine symlinks to prove the guard resolves through one; that needs Developer Mode or admin. They take the visible skip already used elsewhere for the same constraint.
Both failures are on origin/dev independently of this branch, and both come from the same shape: a test that asserts on the TEXT of a source file, pinned to a spelling the implementation has since changed. 8b67220 threaded nativeContextLimits through the remaining Codex and Desktop writers, but sync-client-integrations still required the retired providerContextCap spelling -- so the check failed against the very change it exists to pin. The GUI cap-display check required a one-line expression that is now wrapped and has grown a native branch, so it was pinning formatting rather than behaviour. Match the current spellings, and match the GUI expression as fragments so a reflow cannot fail it again. Verified on origin/dev before this branch was rebased onto it: the sync case fails there with the same message, and the GUI case fails there in a clean worktree.
fix(windows): make the suite pass on an unelevated Windows checkout
Contributor
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
Contributor
⏳ DRAFT
What to do
Its title has been prefixed with |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (79)
📝 WalkthroughWalkthroughGPT-5.6 native models now default to a 272k-token context window and support a clamped 922k opt-in. Native context limits replace the OpenAI provider cap across catalog, server, integrations, and GUI paths. Windows process resolution, SQLite cleanup, and cross-platform tests were also hardened. ChangesNative GPT-5.6 context handling
Windows and test reliability
Estimated code review effort: 4 (Complex) | ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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
1 participant
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
Promotes
devtopreview: the Windows suite work merged in #1881, plus the 21 commits already ondev.Fast-forward-shaped merge of
origin/devwith no conflicts. Highlights from #1881:user-identity.tsresolves LocalAppData viaSHGetKnownFolderPathinstead of the .NET wrapper that followsUSERPROFILEand returns an empty string.injected-marker.tsdecodes TOML string escapes, so a Windows path in the journal round-trips (ocx stop/restore leave unmarked openai_base_url + proxy models_cache behind when Codex app rewrote config after injection #1798).lab/projection/rebuild.tsfinalizes prepared statements before closing, so a rebuild can replace its own file on Windows.Verification
bun x tsc --noEmitandbun run privacy:scangreen on this merge commit.dev.Checklist
Summary by CodeRabbit