feat(permission): restore permission ask hook safely - #42633
Conversation
|
The following comment was made by an LLM, it may be inaccurate: Based on the search results, I found related PRs, but none that appear to be duplicates of PR #42633: Related PRs (not duplicates):
No duplicate PRs found |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
Scope: restores the
|
fwa-wup
left a comment
There was a problem hiding this comment.
Tested & verified — works on the real run-mode platform ✅
I maintain a downstream plugin whose entire contract is the permission.ask hook, so I built and verified this PR against a purpose-built probe + acceptance matrix (isolated temp projects, provider-only config in memory, opencode run with permission.bash: {"*": "ask"}).
Build & tests (head dd25dface, bun 1.3.14, linux-x64):
bun test test/permission/ test/plugin/trigger.test.ts→ 94 pass / 0 fail (the PR's new allow/deny/failure-fallback/interruption/structuredClone-isolation/multi-plugin-override tests included). Worth noting: PR CI currently only runs the bot checks — the test suite never ran here, so I ran it locally.tsgo --noEmit→ clean. Fullbun run script/build.ts→ standalone binaries compile.- No layer-startup cycle from the new
Permission.node → Plugin.nodedep: the compiled binary boots and completes real sessions normally.
Behavior on a real compiled binary:
- Stock 1.18.25, same probe: plugin loads,
permission.asknever fires, run auto-rejects (PROBE FAIL, exit 1) — repro of #7006. - This PR's binary: hook fires, awaited before the pending-request/
Event.Askedpath, i.e. before run-mode auto-rejection. A plugin that setsoutput.status = "allow"genuinely suppresses the prompt and the command executes;"deny"producesDeniedError; a throwing plugin falls back to the normal prompt.main(df6aecd) still has zero trigger call sites, so this PR is exactly the missing piece.
One ecosystem heads-up (no code change requested): this PR re-types the hook input from the v1 Permission to v2 PermissionRequest — i.e. type → permission, singular pattern → patterns[]. Existing plugins written against the shipped type declaration will read input.type === undefined and silently become no-ops on builds with this fix (I hit exactly that in my own plugin, since the hook never fired anywhere the field mismatch stayed invisible). If the changelog/release notes call out the field rename, it would save other plugin authors a confusing debugging session.
Thanks @kernel-oops for sticking with this (and for the interruption-propagation handling, which the earlier attempts got wrong). From the downstream-plugin side: this fixes us. Happy to re-test on request or when it moves.
|
Thank you — this is exceptionally useful validation, especially the real |
dd25dfa to
b4dfbdc
Compare
Issue for this PR
Closes #7006
Type of change
What does this PR do?
Restores the declared
permission.askplugin hook before OpenCode creates an interactive permission request. Static deny remains final, static allow bypasses the hook, and plugins can resolve anasktoallowordeny.This follows #39442 but preserves Effect interruption instead of turning cancellation into a pending prompt. Ordinary plugin failures fail closed to the normal prompt, and
structuredCloneprevents nested plugin mutation of the retained request. The SDK hook input uses the current v2PermissionRequesttype.How did you verify your code works?
Added tests for allow, deny, failure fallback, interruption without a pending request, nested metadata isolation, and sequential multi-plugin overrides. Ran 86 permission tests (132 assertions), plugin trigger tests, OpenCode/plugin typechecks, the plugin build, and format/diff checks.
Screenshots / recordings
N/A — plugin API and permission-flow change only.
Checklist