Repository navigation
Conversation
|
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 49 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fc696b9bd
ℹ️ 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".
| rep := preflight.TypeCheckWithPolicy(ctx, rt.Bin, scriptPath, preflight.TypeCheckMode(mode), timeout, | ||
| append(append([]string{}, rt.ConfigArgs...), depsFlags...), opts.Dir, | ||
| opts.TypeCheckOnError, opts.TypeCheckOnUnsupported) |
There was a problem hiding this comment.
Preserve dependency download policy when switching checkers
When Node or Bun executes a script with checking enabled, checkOnUnsupported=switch, and deps.autoInstall=off, the new Deno checker can still download uncached dependencies. The forwarded depsFlags were generated for the execution runtime: Node supplies none, and Bun's --no-install is discarded by ResolutionArgs. Only the Deno branch of runner/deps.go generates --cached-only, which Deno documents as “Require that remote dependencies are already cached.” Consequently, switching checkers bypasses the configured no-download policy before execution even starts. Generate checker-specific dependency flags when switching to Deno, preserving deps.autoInstall=off.
Useful? React with 👍 / 👎.
| What: "the program does not type check:\n" + indent(strings.TrimSpace(string(out))), | ||
| Where: "script.check", | ||
| Fatal: true, | ||
| Fatal: onError == "" || onError == "fail", |
There was a problem hiding this comment.
Deliver nonfatal check diagnostics to remote callers
For a remote /v1/exec request with typecheck=types and checkOnError=warn, a script that fails checking but executes successfully returns no warning. Downgrading the problem here sends it through runner.go:660–661, which writes directly to warnSink rather than the structured logger. execsvc.RunWith captures that writer in a stderr buffer (execsvc.go:934–948), but only exposes its contents when the script exits nonzero (execsvc.go:1063–1071). Consequently, neither the successful response nor its streamed frames contain the check diagnostic, making the new warn policy indistinguishable from run for remote callers. Route these warnings through the result/log delivery path even when execution succeeds.
Useful? React with 👍 / 👎.
Summary
Adds registry-backed settings and docs/API schema.
switchuses Deno as checker only; the selected runtime still executes the script. This is the first bounded slice of #1911, not the full Approach C pipeline.Still owed on #1911:
syntaxmode;script.compile/script.checkJs; local and remote text runtime reporting; one-time legacy-spelling notice; coordination note on #1868.Evidence
go test ./internal/preflight ./internal/runner ./internal/cli ./internal/execsvc ./internal/daemon ./internal/api -count=1— PASS (API after regeneratingplugin/opencode/mcpx/ops.gen.ts).git diff --check— PASS.proto.elicitationPerMinute,proto.samplingPerMinute, andproto.samplingSessionPerMinutegaps; the new policy settings are exempted against deterministic preflight tests.Merge Danger
Door: two-way.
Blast Radius: bounded — changes preflight handling only when script checking is enabled; defaults preserve fail-on-check-error and warn-on-unsupported-runtime behavior.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.