Repository navigation
Conversation
added 6 commits
September 11, 2026 19:58
…elete_branch_on_merge
…g test, copy rationale)
…gh ComSpec on Windows
|
✅ Target branch corrected This pull request now targets The |
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. |
Owner
Author
|
Superseded by the rebased PR against \dev. GitHub auto-closed this one when the L0 base branch was deleted by that merge, and reopening was refused because the head had already been rebased. Content is unchanged. |
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.
Layer L1 of the Windows issue sweep stack. Base is L0 (
codex/win-sweep-roadmap), notdev—enforce-targetwill flag this and that is expected for a manual chain. Do not retarget it atdev; that would collapse the chain into overlapping diffs.Closes the Windows half of #131 for
provider-bridge. The GUI andcxc servecopies are L6.The bug
On Windows an npm global install lays down both an extensionless sh shim and a
.cmdlauncher, andwhere ocxlists the extensionless one first. Two independent failures follow, measured on the reporting host:The extensionless file is not an executable image, and after the CVE-2024-27980 hardening a shell-less
.cmdspawn isEINVAL. Both land onstatus === null, so a healthy proxy was reported asmode: "error"withreason: "ocx status exited null".The fix, and why not the obvious one
The obvious fix is to pick a better line out of
wherestdout and wrap the.cmdin a ComSpec call. That was the first implementation, and an independent review rejected it: it would have been a third bespoke ComSpec wrapper in a repository that already haswin-exec.tsin four components, and its naive"${path}"quoting is a command injection. Reviewer reproduction: with a launcher path containing&, plain quoting makes cmd execute only the prefix and return status 1, whilecommandInvocationescapes it to^&and returns status 0.So this layer copies
cxc-ops/src/win-exec.tsintoprovider-bridgebyte-for-byte and routes through it:whichOcxcallsresolveWindowsCommand, which readsPATH/PATHEXTcase-insensitively (a child can arrive withPath,PATH, or both), splitsPATHon a literal;rather thannode:path's host-dependent delimiter, and retries the lowercased extension for case-sensitive filesystems. Thewhere-stdout parsing is deleted, not patched.runOcxStatuscallscommandInvocation, which routes only.cmd/.batthrough ComSpec and escapes cmd metacharacters.shell: trueis not used — Node does not escape there.cli.tsis 66 lines deleted for 31 added.detect.tsis untouched; the detect-only contract holds — noocx ensure, noocx sync, no config write.Why a copy rather than the re-export
messenger-bridgeusesA cross-component import is not illegal here —
build.mjs's convention is a comment, not a check, andmessenger-bridge/src/win-exec.tsalready re-exportssubagent-config/dist/win-exec.js. Butprovider-bridgeruns from a SessionStart hook that must always exit 0, so depending on another component'sdist/build state would turn a resolution failure into a silent provider-status degradation. The copy removes that coupling; the drift cost is converted into a loud test failure by a byte-identity assertion.Verification
Live, from the built
dist/on the reporting host:The parent commit emits
{"mode":"error","reason":"ocx status exited null"}for the same command.The test that actually gates this
The three helper tests exercise
win-exec.tsdirectly and all pass whencli.tsis untouched — copying the module alone would satisfy them. An audit caught that hole, so there are now two win32 subprocess tests that run the realcli.ts detectagainst a craftedPATHholding both an extensionless shim and a.cmdthat answersstatus --json. They assertmode === "provider", that the resolved path is the launcher, and thatrunning/portcame back, so they fail if either half regresses or if the wiring is missing.Known coverage limit: those two are win32-only, because the fixture needs a real
.cmd. Linux and macOS CI do not gate the wiring — only the Windows matrix jobs do.One behaviour difference, recorded rather than hidden
resolveWindowsCommandbuilds candidates with PATHEXT's own spelling and WindowsexistsSyncignores case, so the reported path ends.CMDwhere the oldwhere-parsing reported the on-disk.cmd. Caught by the new tests on first run. Cosmetic — Windows paths are case-insensitive and the only consumer readsocxPathas a path. It must not be normalised insidewin-exec.ts, whose bytes are shared with four components; the tests compare case-insensitively and assert/\.cmd$/i, pinning that the shim lost rather than a spelling.dist/win-exec.jsis a new file and.gitignore:2ignoresdist/, so it requiredgit add -f.Plan:
devlog/_plan/260911_windows_issue_sweep/010_wp2_provider_bridge_windows_detect.md.