Repository navigation
Fix Pi Windows npm shim probing and RPC pipes - #5271
Conversation
d014fb2 to
606364b
Compare
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 606364b2d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** | ||
| * Runs a CLI with buffered stdout/stderr, resolving Windows npm `.cmd` shims. | ||
| * Bounds output with `maxBuffer` and stops descendants on timeout or abort. | ||
| * Throws errors carrying `code`, `signal`, `killed`, `stdout` and `stderr`. |
There was a problem hiding this comment.
When a bridge passes an AbortSignal that is already aborted, execPortableFile exits at packages/process-utils/src/exec-portable-file.ts:25 by throwing signal.reason directly; that value may be a string or DOMException, so it lacks the five fields this new public declaration and audit entry promise on failure. Callers following the exported contract can therefore fail while handling this cancellation path; normalize the early-abort branch into the same structured AbortError used after launch (or explicitly narrow the guarantee).
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I've removed the SDK export from this PR, so it no longer introduces the AbortSignal error contract you flagged. The provider now uses its own Pi process-launch resolver with plain execFile. This removes the newly exposed contract rather than changing the existing internal early-abort behavior. Updated in 7f3043b.
yteruel31
changed the title
|
hey @yteruel31, i hit the same bug (#5332) with pi in windows. i raised a small fix PR in MayankBansal12#7 Same idea, but it leaves |
|
@MayankBansal12 Thanks, you're right about preserving Pi's installation identity. I've separated configuration resolution from the actual process launch and kept the fix provider-local, using plain |
The npm shim removes .JS from PATHEXT before it looks up `node`, so a `node.js` file in the working directory is never mistaken for the Node executable. The unwrapped launch searched the thread's working directory with the full PATHEXT, so a workspace with a root-level `node.js` had that file chosen as the command and Pi could not start. Co-Authored-By: Claude Code <noreply@anthropic.com>
3 participants
Human comments
What was wrong
Two Windows npm-shim issues remain after #4661.
probePiVersion()usesexecFile("pi", ["--version"]), which cannot launch the npmpi.cmdshim and can reportENOENTdespite Pi being installed. The RPC launcher can execute that shim throughcmd.exe, but the intermediate shell does not forward BB's extra stdio descriptors 3 and 4; the extension then fails withUnsupported fd type: UNKNOWN. These were reproduced with BB 0.45.0 and Pi 0.85.1, and the version-probe issue also reproduces with Pi 1.1.0. The unchanged probe is visible in the branch base.What changed
resolvePiLaunch()as the original configuration resolver. Installation identity, executable metadata, cache keys, and npm/Bun maintenance continue to describe Pi, not its Node interpreter.pi.cmdshims into Node plus the validated package CLI. Resolve against the child's environment/cwd, prefer siblingnode.exe, otherwise resolve Node from the child's PATH, and preserve configured prefix arguments.node_modules/.binlayouts. Validate manifest/bin contents and physical CLI containment; only accept standard npm shim templates targeting that manifest CLI. Custom wrappers remain unchanged, including custom files namedpi.cmdbeside a valid Pi package.node.exe, look upnodewith.JSremoved fromPATHEXT, as the npm shim does, so anode.jsfile in the thread's working directory is not chosen as the Node executable.node:child_processexecFilefor probing, including its existing 15 s timeout, stdout-only parsing, output bound, and failure diagnostics. RPC launches retain both the ordinary stream and the extra bidirectional extension channel.Following review feedback, this PR no longer introduces an SDK export or version bump. The earlier SDK/docs changes and unrelated attachment-preview fix have been removed. The resulting diff is confined to
provider-piand itswhich-commandlockfile entry. No host protocol, CLI, user-facing configuration, or installed-plugin changes.How you verified
Linux:
The final Turbo run passed 11/11 tasks: 233 tests passed, 7 skipped, plus lint, typecheck and bundled preparation. Windows-only tests are skipped on Linux. Both accepted npm templates and LF/CRLF handling are covered; custom-wrapper tests retain a valid npm target while injecting additional commands, so they test actual wrapper rejection rather than merely a mismatched target.
Native Windows, Node 24.18.0: bundled the final upstream implementation into a temporary standalone module without installing dependencies or modifying the running BB service. Global and local canonical npm fixtures verify version probing/install-gate success, original
pi.cmdinstallation identity, configured arguments, custom-wrapper preservation, ordinary RPC, and descriptor 4 → CLI → descriptor 3 round trips.The live smoke uses the machine's existing Pi 1.1.0,
--no-session --no-extensions, and an explicit temporary BB extension. An initial run with automatically loaded user extensions timed out; disabling those extensions isolated this launch fix and passed. No model prompt or full thread turn was sent, no native session was persisted, and installed providers/configuration were not changed. Windows regressions were exercised through equivalent standalone bundles, not a full native Windows Vitest run. CI and review still validate integration before merge.Fixes #5332