Skip to content

feat(permission): restore permission ask hook safely - #42633

Open
kernel-oops wants to merge 1 commit into
anomalyco:devfrom
kernel-oops:fix/permission-ask-hook
Open

feat(permission): restore permission ask hook safely#42633
kernel-oops wants to merge 1 commit into
anomalyco:devfrom
kernel-oops:fix/permission-ask-hook

Conversation

@kernel-oops

@kernel-oops kernel-oops commented Aug 14, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #7006

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Restores the declared permission.ask plugin hook before OpenCode creates an interactive permission request. Static deny remains final, static allow bypasses the hook, and plugins can resolve an ask to allow or deny.

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 structuredClone prevents nested plugin mutation of the retained request. The SDK hook input uses the current v2 PermissionRequest type.

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

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions github-actions Bot added the needs:compliance This means the issue will auto-close after 2 hours. label Aug 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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):

  1. fix(plugin): restore permission ask hook #39442 - fix(plugin): restore permission ask hook

  2. feat(core): optional plugin gate in PermissionV2 for allow→ask #34329 - feat(core): optional plugin gate in PermissionV2 for allow→ask

    • Related to permission hook infrastructure but addresses a different feature (optional plugin gate).
  3. feat(permission): wire permission.ask plugin hook #30509 - feat(permission): wire permission.ask plugin hook

    • Historical permission hook implementation, not a duplicate.
  4. fix(opencode): add permission.ask plugin hook back #19453 - fix(opencode): add permission.ask plugin hook back

    • Historical permission hook fix, not a duplicate.

No duplicate PRs found

@github-actions github-actions Bot removed the needs:compliance This means the issue will auto-close after 2 hours. label Aug 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for updating your PR! It now meets our contributing guidelines. 👍

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Scope: restores the permission.ask plugin hook: plugins receive a structuredClone of the request info and may set output.status to "allow"/"deny"; anything else falls through to the interactive prompt.

  • Security posture is right where it matters most: plugin failure resets to "ask" (interactive prompt) rather than failing open to allow, interrupts propagate instead of being swallowed, and structuredClone(info) stops plugins from mutating shared request state. A crashing plugin degrades to the pre-hook behavior — exactly what "restore safely" should mean.
  • Cause.hasInterrupts re-failure distinction is a nice touch; generic catch-all handlers that also eat interrupts are a common Effect bug.
  • Design question worth confirming: the hook only runs when the configured rules resolved to "ask", so plugins can upgrade an ask → allow/deny but can never tighten an explicit config allow/deny. That seems like the intended layering (config is authoritative, plugins advise) — just make sure docs say plugins cannot restrict what config allows.
  • Test-infra nit: waitForPending timeout went 1s → 10s. Understandable with plugin loading in the loop, but blanket 10x timeouts can mask real latency regressions; consider scoping the longer timeout to the plugin-specific tests only.
  • The deny path rebuilds the ruleset-filtered DeniedError — consistent with native denials, so downstream consumers can't distinguish plugin denials from rule denials. Intentional? Fine either way, but worth a sentence in the PR.

@fwa-wup fwa-wup left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts94 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. Full bun run script/build.ts → standalone binaries compile.
  • No layer-startup cycle from the new Permission.node → Plugin.node dep: the compiled binary boots and completes real sessions normally.

Behavior on a real compiled binary:

  • Stock 1.18.25, same probe: plugin loads, permission.ask never fires, run auto-rejects (PROBE FAIL, exit 1) — repro of #7006.
  • This PR's binary: hook fires, awaited before the pending-request/Event.Asked path, i.e. before run-mode auto-rejection. A plugin that sets output.status = "allow" genuinely suppresses the prompt and the command executes; "deny" produces DeniedError; 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. typepermission, singular patternpatterns[]. 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.

@kernel-oops

Copy link
Copy Markdown
Author

Thank you — this is exceptionally useful validation, especially the real run-mode test and layer start-up check. Great catch on the payload compatibility point too; agreed that the typepermission and patternpatterns[] change should be called out in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

permission.ask plugin hook is defined but not triggered

3 participants