Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
The fix works: Code Mode's fetch now follows the webfetch permission.
I ran the #54595 reproduction with opencode-drive and the simulated model on the base and on this branch. The webfetch rule was set to deny, as an explicit permissions rule, because drive's own * allow rule otherwise overrides the config. On the base, the local server received the request and execute succeeded. With this PR, the server got no request and the call failed with Permission denied: webfetch. With ask, the usual WebFetch permission prompt appears and nothing is sent until it is answered. The three changed test files pass (138 tests), and packages/core type-checks.
Public API: unchanged. CodeModeTool.create and CodeModeWeb.extension are internal to core.
One point inline: the permission check is awaited with Effect.runPromise, so it runs outside the execute fiber and is not cancelled with it.
| async function fetch(input: string | URL, init: Init = {}) { | ||
| if (URL.parse(String(input))?.protocol === "file:") | ||
| throw new Error("fetch cannot be used for filesystem access.") | ||
| await Effect.runPromise(guard(String(input))) |
There was a problem hiding this comment.
Effect.runPromise runs the permission check in a detached fiber, outside the execute call that started it. If the user interrupts execute while an ask prompt is open, that check isn't interrupted: the request stays pending, and if it is approved later the fetch could still go out. The Code Mode interpreter accepts native functions that return an Effect, so it may be possible to run the guard (and the request) inside the caller's fiber and let cancellation carry through. Could you check whether extension globals can do that, and if not, at least tie the guard's lifetime to the execute call?
|
Checked whether the guard can run in the caller's fiber, and it cannot: So I tied the guard's lifetime to the call instead (
New test |
5149303 to
1080ab5
Compare
|
Rebased onto the current The runtime is now cached per tool inventory, with the per-execution parts in a To read a service from inside a global, That last point removes the whole reason for One consequence worth flagging in review: |
cf2e331 to
bdfb126
Compare
|
Rebased onto The rebase itself had one conflict, in the import block of Two changes were needed beyond that conflict:
Verified on the rebased head: I also corrected the commit message: the earlier wording said an Effect-returning global "runs in the caller's fiber", which #54276 made inaccurate for Promises and therefore for Effects — it is forked into the execution's scope, immediately. |
There was a problem hiding this comment.
The new commit addresses the cancellation point. fetch now returns an Effect that runs in the execute call's fiber, so interrupting execute also interrupts a permission prompt that is still waiting. The guard is read per call from the Run service rather than captured by the cached runtime, so a later execution can't reuse an earlier call's guard. The new interruption tests in packages/core and packages/codemode cover this.
I couldn't re-run the TUI reproduction on this head: in my sandbox, simulated tool calls (even a plain glob) don't run on the current base, so neither checkout could be checked end to end. The previous head passed the same reproduction (deny blocked the request, ask showed the WebFetch prompt). On this head the affected core tests (142), packages/codemode/test/extensions.test.ts (28) and both packages' type checks pass.
Public API: no change for users. The only new capability is that a Code Mode extension global may now return an Effect, which is internal and backwards compatible.
No new concerns.
Code Mode's `fetch` global reached the network without consulting the `webfetch` permission, so a `"webfetch": "deny"` rule could be bypassed from `execute`. The check runs inside the `fetch` global, before the request leaves the process. The `extension.before` hook cannot gate it: an extension's body runs before its hook, so denying there would only deny the result. `fetch` returns an Effect so the guard can resolve services from the execution that started it. The Codemode runtime is cached per tool inventory while a permission check belongs to one call, so a guard closed over by the runtime would be shared by every later call; reading it from the runtime's `Run` service keeps the runtime shared and the check per call. Extension globals may now return an Effect, forked into the execution's scope exactly like a returned Promise, which is what makes that possible. It also makes `execute` wait on a permission prompt that a finishing or interrupted call no longer needs: the interpreter interrupts the check with the program's pending work instead of leaving it detached.
bdfb126 to
6b5f354
Compare
|
Two notes on the new head
|
There was a problem hiding this comment.
The only change since my last review is a comment in extensions.ts, which now describes a returned Effect as forked into the execution's scope. That matches the code, and no behaviour changed. My earlier point about the permission check running in a detached fiber was already addressed in the previous commit.
Public API: no change for users. The only new capability is that a Code Mode extension global may now return an Effect, which is internal and backwards compatible.
No new concerns.
|
This PR's mechanism has been overtaken, so I would rather ask than keep polishing it. It adds the permission gate for Code Mode's #57976 rewrites that whole area — one job queue, So, one of these three, and I will do whichever you pick:
If I hear nothing I will leave it as it is and stop touching it, since #57976 decides the shape either way. No action is needed from you today — a one-word answer is enough. |
|
Withdrawing this PR rather than keeping it parked indefinitely. Why. When I opened it, the permission gate for Code Mode fetches was mine to place. Since then the mechanism around it has been rewritten upstream: State of the review. It has never had a human review. The only reviews are from What was still useful. The gate itself is small and the tests are real: Offer. If a maintainer wants this once the Code Mode job-queue work settles, say so here and I will rebuild it on whatever |


Issue for this PR
Closes #54595
Type of change
What does this PR do?
A
"webfetch": "deny"rule did not apply to Code Mode. Thefetchglobal that Code Mode injects called theplatform's fetch directly, so code passed to
executecould reach the network even though the same URL wasrefused for the
webfetchtool.The fix consults the
webfetchpermission inside thefetchglobal, before the request leaves the process, andfails the call with the permission error the tool path uses. Denying earlier is not an option: an extension's
extension.beforehook runs after the extension body for the globals Code Mode installs, so a denial therewould only deny the result, not the request.
Rebased onto the current
dev, where the Codemode runtime became one object cached per tool inventory with theper-execution parts in a
Runservice. A guard closed over by the runtime would therefore be shared by everylater call, so the guard now travels in
RunwithexecuteToolandhooks:RuncarriesguardFetch.CodeModeWeb.extensionbuilds itsfetchglobal from it, so the runtime stayscached and the check stays per call.
fetchreturns an Effect instead of a Promise, which is what lets the global read a service at all.Extension globals may now return an Effect, run in the caller's fiber exactly like a returned Promise, so the
interpreter gives it the services of the execution that started it.
work when the program ends, so a permission prompt still waiting at that point is cancelled with it. That is
asserted rather than assumed — see the interruption case below.
Tool.nodenow depends onPermission.node, since the tool layer is what holds the permission service when itbuilds the Code Mode tool.
packages/core/test/tool-todo.test.tsbinds a permission layer as a result.How did you verify your code works?
packages/core/test/tool-execute.test.tsdrives the globals against a real local server:Permission.BlockedError— the call settles as an error namingPermission denied: webfetch, and the server records zero requests. The "zero hits" assertion is whatmakes this a test of the gate rather than of the message.
not outlive the call.
packages/codemode/test/extensions.test.tscovers the new capability on its own terms: a returned Effect seesthe services provided for that execution, a failed Effect reaches the program as its error, and an Effect still
waiting when the program ends is interrupted with it.
packages/core/test/tool-webfetch.test.tsdrives the same gate through the real tool registry with a permissionlayer that denies: the registry asserts
webfetchwith the URL, no request is made, and the filesystem rejectionis left to the runtime rather than the permission check.
Results on the rebased head:
packages/core6612 pass, and its 64 failures are the pre-existingsandbox-bound ones (ripgrep, pty, shell, plugin spawning) — the same set the tree fails without this change, and
one of them,
tool-todo, needed the permission layer this PR adds and passes with it.packages/codemode1413 pass, 1 fail, the RegExp failure that also predates this branch.
tsc -bis clean in both packages, asare
oxlintandprettier --check.Screenshots / recordings
No response.
Checklist