fix(opencode): run the documented permission.ask plugin hook - #47675
fix(opencode): run the documented permission.ask plugin hook#47675Atmostone wants to merge 2 commits into
Conversation
The plugin API declares a `permission.ask` hook, but nothing ever calls it. `trigger` is invoked in six places and none of them is this hook, so a plugin that registers it is silently ignored. Give the permission service a reviewer, and register one from the plugin layer that runs the hook. The dependency points plugin -> permission on purpose: the reverse edge would drag plugin loading into every graph that builds Permission on its own, which several tests do. The hook is now declared with the shape it is actually handed, so both `as any` casts are gone. It receives copies of `patterns`, `metadata` and `always`, so a hook cannot rewrite the request the user is about to be shown. It sees the id the real request will carry. A status other than allow/ask/deny keeps the rule decision instead of failing open, and a hook that throws does the same rather than taking the permission check down. A denial can carry the plugin's own message, so the model is no longer told the user wrote a rule that does not exist. A rule that already denies settles the request before the hook runs, so the hook can observe allow and ask only. Co-Authored-By: Александр <lild1tz2023@gmail.com> Co-Authored-By: Timur Polishchuk <timur.polishchuk.official@gmail.com>
|
The following comment was made by an LLM, it may be inaccurate: Based on my search, I found several related PRs that address the permission.ask plugin hook: Potential Duplicates/Related PRs:
The most likely duplicate/related work is PR #42633 ("feat(permission): restore permission ask hook safely"), which has the same core objective of implementing the documented but non-functional permission.ask hook. You should verify if that PR was closed/merged and whether the current PR (47675) is a revival or improvement of that work. |
Every hook shares one `output` object, so a failure can follow a decision an earlier hook already made. The catch restored the pre-hook status unconditionally, which discarded a `deny` a well-behaved hook had set. Restore only when keeping what the failing hook left would be more permissive than the rules were. Co-Authored-By: Александр <lild1tz2023@gmail.com> Co-Authored-By: Timur Polishchuk <timur.polishchuk.official@gmail.com>
1 participant
Issue for this PR
Closes #47674
Type of change
What does this PR do?
packages/plugin/src/index.tsdeclares apermission.askhook. Nothing callsit —
triggerhas six call sites and this is not one of them — so a plugin thatregisters the hook never runs.
This wires it up. The permission service gains a reviewer that the plugin layer
registers, and
Permission.askruns the hook on the decision the rulesproduced. The hook can leave it, tighten it, or relax it, and can explain a
denial.
Four things I fixed while wiring it, because the hook is on the permission path:
Permissionshape, which sharesthree of nine fields with what is actually passed. A plugin written against
the published type would read
undefinedfortype,patternandtitle.It now declares
PermissionRequestfrom@opencode-ai/sdk/v2, which is theshape the service really hands over, and both
as anycasts are gone.patterns/metadata/alwaysarrays,which are the same objects the pending request uses. It now gets copies.
use. It now sees the id the request will carry.
that threw escaped
ask's declared error channel and killed the tool call.Both now keep the decision the rules made, and log.
Denials no longer synthesise a rule the user never wrote;
DeniedErrorgainedan optional
reasonthe message prefers.Not in scope, and worth saying: a rule that denies settles the request before
the hook runs, so a plugin cannot relax a configured deny. Hooks in this
codebase have no timeout —
triggerruns them all throughEffect.promise—and this PR does not change that for other hooks.
How did you verify your code works?
New test file
packages/opencode/test/plugin/permission-ask.test.ts, sevencases: allow/ask/deny each take effect; an unrecognised status does not
auto-allow; a throwing hook leaves both an allowing and an asking rule in
charge; and an allow written before a throw is discarded.
I checked the tests are not vacuous by removing the
Effect.catchCausewrapper(the three throwing-hook cases fail) and separately by dropping only the status
restore (just the "allow then throw" case fails).
All of the above was run locally with AI assistance rather than typed by hand.
I read the resulting diff myself and can explain every line of it.
Screenshots / recordings
N/A — no UI change.
Checklist