Skip to content

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

Open
kevholmes wants to merge 2 commits into
anomalyco:devfrom
kevholmes:permission-ask-hook
Open

fix(plugin): restore permission ask hook#39442
kevholmes wants to merge 2 commits into
anomalyco:devfrom
kevholmes:permission-ask-hook

Conversation

@kevholmes

Copy link
Copy Markdown

Issue for this PR

Fixes #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 prompts the user. Plugins can allow or deny requests that static rules resolve to ask; plugin failures fall back to the normal prompt.

This rebases the approach from #30509 onto the current Effect/LayerNode service shape and credits @yohi's original implementation. The hook receives the current v2 PermissionRequest type and a copy of mutable request data.

How did you verify your code works?

  • Added tests for plugin allow, deny, failure fallback, and request mutation isolation.
  • Ran the permission and plugin trigger tests: 85 passed.
  • Ran the repository pre-push typecheck: 30 packages passed.
  • Built a standalone binary and tested a real plugin round trip: allow suppressed the prompt, deny blocked execution, and a thrown hook fell back to the normal prompt.

Checklist

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

@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

Based on my search, I found several related PRs that are worth noting:

Related PRs (Not Duplicates)

  1. feat(permission): wire permission.ask plugin hook #30509 - feat(permission): wire permission.ask plugin hook

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

    • Related: Works with the permission system and plugin gates, complementary to the permission ask hook
  3. fix(opencode): add permission.ask plugin hook back #19453 - fix(opencode): add permission.ask plugin hook back

  4. fix(mcp): pass tool name and args to permission ask #39057 - fix(mcp): pass tool name and args to permission ask

    • Related: Recent fix specifically for passing MCP tool context to permission ask

No duplicate PRs found

@kernel-oops

Copy link
Copy Markdown

We are very keen to see this PR land. This hook is the missing primitive for building genuinely useful permission-review plugins rather than either prompting for everything or blindly auto-approving everything.

That feels especially timely now that OpenAI is pushing Luna more explicitly as a request-approval/review model: a cheaper second model reviews proposed actions, allows low-risk requests, and leaves higher-risk actions for the user. OpenCode cannot cleanly implement comparable features in plugins while permission.ask is declared but never invoked. Restoring this hook would unlock that whole category of approval agents without forcing it into core.

The overall shape of this patch looks right to us: static allow and deny remain authoritative, only requests resolving to ask reach plugins, and plugin failure falls back to the normal human prompt.

We did notice two implementation details that seem worth tightening before merge:

  1. Effect.catchCause also catches interruption. If the session/tool fiber is cancelled while a permission hook is pending, the handler appears able to reset the result to ask and continue into creation/publication of a pending permission request, rather than preserving cancellation. It would be safer to re-fail interruption causes and use the human-prompt fallback only for ordinary hook failures/defects. A cancellation test would help lock this down.

  2. The defensive request copy is shallow. metadata is Record<string, unknown> and can contain nested mutable data such as tool input, so { ...info.metadata } still lets a plugin mutate nested state retained in the eventual pending request. Either deep-copy the request data or explicitly define/document nested metadata mutation as part of the hook contract; the current mutation-isolation test only covers top-level metadata.

Neither point changes our strong support for the PR or the API direction. We would really like this capability available: it is a small hook with disproportionately useful consequences for safer, lower-friction OpenCode workflows.

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

2 participants