fix: keep upstream Windows codex app-server for node_repl compatibility#91
Open
siyuan-123 wants to merge 2 commits into
Open
fix: keep upstream Windows codex app-server for node_repl compatibility#91siyuan-123 wants to merge 2 commits into
siyuan-123 wants to merge 2 commits into
Conversation
Reviewer's GuideWindows builds now retain the upstream codex.exe app-server to stay MCP protocol-aligned with bundled cua_node/node_repl, while upstream sync, ASAR handling, and ZIP/archive workflows are hardened and made more resilient across platforms. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
siyuan-123
marked this pull request as ready for review
June 28, 2026 06:04
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 security issues, and left some high level feedback:
Security issues:
- Detected calls to child_process from a function argument
platform. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link) - Detected calls to child_process from a function argument
platform. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link) - Detected calls to child_process from a function argument
zipPath. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link) - Detected calls to child_process from a function argument
zipPath. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link)
General comments:
- In encodeScopedPackagePath, replacing all '@' characters with '%40' may over-encode non-scoped segments; consider limiting the replacement to leading '@' in path parts that are actually package scopes.
- createZip falls back to the bare
zipCLI on non-Windows platforms without checking availability or handling errors; you might want to detect its presence or surface a clearer failure whenzipis not installed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In encodeScopedPackagePath, replacing all '@' characters with '%40' may over-encode non-scoped segments; consider limiting the replacement to leading '@' in path parts that are actually package scopes.
- createZip falls back to the bare `zip` CLI on non-Windows platforms without checking availability or handling errors; you might want to detect its presence or surface a clearer failure when `zip` is not installed.
## Individual Comments
### Comment 1
<location path="scripts/build-from-upstream.js" line_range="161" />
<code_context>
execSync(`npx asar pack "${asarDir}" "${asarPath}" --unpack-dir "{node_modules/better-sqlite3,node_modules/node-pty}" --unpack "{**/*.node,**/node-pty/build/Release/*.exe}"`);
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `platform`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>
### Comment 2
<location path="scripts/build-from-upstream.js" line_range="234" />
<code_context>
execSync(`npx asar pack "${asarDir}" "${asarPath}" --unpack-dir "{node_modules/better-sqlite3,node_modules/node-pty,node_modules/@worklouder}"`);
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `platform`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>
### Comment 3
<location path="scripts/build-from-upstream.js" line_range="329" />
<code_context>
execSync(`7zz a -tzip -mx=5 "${zipPath}" .`, { cwd, stdio: "pipe" });
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `zipPath`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="scripts/build-from-upstream.js" line_range="349" />
<code_context>
execSync(`zip -r "${zipPath}" .`, { cwd, stdio: "pipe" });
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `zipPath`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
Addressed the Sourcery feedback in a follow-up commit:
Validation: The Windows build completed successfully and still keeps the upstream @sourcery-ai review |
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
This PR fixes a Windows rebuild compatibility issue where the bundled Desktop app-server can configure
node_replwith MCP metadata that newercua_node/node_replrejects:Root cause
The Windows rebuild flow was replacing the upstream MSIX
resources/codex.exewith the@cometix/codexCLI. In the failing build this mixed:cua_node/node_repl.exefrom a newer upstream packageresources/codex.exefrom an older / different@cometix/codexapp-server buildcodex.exe app-serveris responsible for writing Desktop MCP configuration and request metadata fornode_repl.node_replvalidatescodex/sandbox-state-meta.sandboxCwdand now requires a file URI. Mixing the app-server binary with a different Desktop bundle can therefore break browser / Node REPL tools even though the rest of the app starts normally.A local reproduction confirmed the issue disappeared after replacing the rebuilt Windows
resources/codex.exewith the upstream MSIXcodex.exefrom the same package ascua_node/node_repl.exe.Changes
resources/codex.exefor Windows builds instead of replacing it with@cometix/codex.prepare-src.jsso forge-style Windows preparation also preserves the upstream app-server binary.node_replmust stay protocol-aligned.7zz/7zis unavailable%40worklouderapp.asar.7zzis unavailable.Validation
Run on Windows:
Observed:
The generated Windows build keeps upstream:
After swapping that binary into the affected local Desktop rebuild and restarting,
node_repl/jssucceeded and no longer returned thesandboxCwd must use the file URI schemeerror.Summary by Sourcery
Keep the upstream Windows codex app-server binary to restore compatibility between desktop MCP metadata and bundled node_repl, while hardening the Windows upstream sync and packaging flow.
Enhancements:
Build:
Documentation: