Repository navigation
Conversation
![developing-today[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit details
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. |
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: b07147e7bc
ℹ️ 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".
| if rt.Kind == KindNode { | ||
| if err := checkNodeTransformTypes(rt.Bin); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
When a JavaScript shebang clears or overrides Node-related environment variables, this probe still inherits mcpx's original environment. For example, with an inherited NODE_OPTIONS=--require=/missing.cjs, a script using #!/usr/bin/env -S -u NODE_OPTIONS node previously worked because childBaseEnv removes that variable before execution. (node --help describes --require as a “CommonJS module to preload”.) Now the probe fails on the unwanted preload and rejects a working runtime, even after ResolveDeferred bypasses the initial failure. Probe with the effective child environment and directory, and include those inputs in the cache identity.
Useful? React with 👍 / 👎.
| if err := probe.Close(); err != nil { | ||
| return fmt.Errorf("probe node runtime %s for --experimental-transform-types: close temporary TypeScript script: %w", bin, err) | ||
| } | ||
| output, err := exec.Command(bin, "--no-warnings", "--experimental-transform-types", probePath).CombinedOutput() |
There was a problem hiding this comment.
If Node startup stalls, this synchronous command can block a run indefinitely despite its configured timeout or canceled context. For example, inherited NODE_OPTIONS=--inspect-brk=0 pauses the probe: the installed Node's --help describes this flag as “activate inspector on host:port and break at start of user script”. Initial resolution executes this probe before runCtx is created, and the command itself has no cancellation mechanism. Previously the actual script subprocess was bounded by the runCtx.Done() select in runner.go. Give the probe a bounded context and ensure caller cancellation terminates it.
Useful? React with 👍 / 👎.
| probe, err := os.CreateTemp("", "mcpx-node-transform-probe-*.ts") | ||
| if err != nil { | ||
| return fmt.Errorf("probe node runtime %s for --experimental-transform-types: create temporary TypeScript script: %w", bin, err) |
There was a problem hiding this comment.
In a read-only-root deployment with writable mcpx cache/state mounts but no writable /tmp, this unconditional temporary-file creation now rejects otherwise working Node runs. Previously, Run used the configured WorkDir or WorkRoot for generated files and only needed the system temporary directory when neither was configured; production execution forwards those directories through execsvc.go:705-706. The probe runs before that writable work area is selected, introducing an additional filesystem requirement despite an explicitly configured writable cache. Avoid the global temporary file or create the probe inside the configured work area.
Useful? React with 👍 / 👎.
Summary
Verification
cd pkgs/mcpx && go test ./internal/runner/...cd pkgs/mcpx && scripts/tier2-containers.sh alpine-node-exec(Docker; Alpine nodejs produced the actionable mcpx refusal)bash -n pkgs/mcpx/scripts/tier2-containers.shgit diff --checkCloses #2699.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.