Skip to content

fix: skip permission notification when request is auto-approved - #98

Merged
mohak34 merged 3 commits into
mohak34:mainfrom
karayelxyz:main
Sep 17, 2026
Merged

mohak34 merged 3 commits into
mohak34:mainfrom
karayelxyz:main

Conversation

@karayelxyz

@karayelxyz karayelxyz commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #94

Problem
When OpenCode's auto-approve mode is enabled, permission requests are resolved automatically and instantly. The notifier plugin still fired a "Session needs permission" notification for every such request, which spams the user even though no manual approval is needed.

Root cause
The plugin notified unconditionally on the permission.asked event. It had no way to distinguish between:

  • a request that is still pending (requires manual approval → notification is useful), and
  • a request that was auto-approved (already resolved → notification is noise).
    An initial attempt to check pending state via the SDK client failed at runtime: the plugin is handed a v1 SDK client (PluginInput["client"]) which exposes no permission.list / client.permission.request.list API, so the pending lookup threw a TypeError and the notification was silently skipped in all cases — including the pending case where it was actually needed.

Solution
On permission.asked:

  1. Extract the permission request ID from the event (event.properties.id, with a properties.request.id fallback) via a new getPermissionIDFromEvent helper.
  2. Wait 300ms so that auto-approved requests have time to be resolved (they are approved immediately by the server).
  3. Query pending permissions through the raw HTTP client (client._client) using the v2 GET /permission endpoint — the v1 SDK client type has no typed method for this, so the raw client is used.
  4. Only notify if the request with the same ID is still present in the pending list. If the lookup fails for any reason, notify anyway (fail-open).
  5. Claim the shared permission dedupe window only when a notification is actually about to fire, so an auto-approved request cannot mute a real one arriving within the same second.

Result:

  • Auto-approve enabled → request resolved before the check → not pending → no notification (fixes the spam).
  • Auto-approve disabled → request stays pending → notification + sound fires as before.
  • If the pending check itself fails for any reason, we fall back to notifying (fail-open) so notifications are never lost unexpectedly.

Notes

  • client._client is protected in the SDK's generated client class, so the access uses a (client as any) cast — consistent with the existing (event as any) pattern already used in this file.
  • Verified locally: typecheck (tsc --noEmit), full test suite (90 tests), and build all pass. The permission decision matrix was additionally exercised against a fake client — the update comment below has the table.
  • Confirmed end-to-end on OpenCode 1.18.30: with auto-approve enabled the Session needs permission notification is gone, and with auto-approve disabled it still fires. question and complete notifications are unaffected, as expected.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new pending-permission check can silently suppress notifications when the raw HTTP client is missing or the response shape is unexpected, breaking the intended fail-open behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the notifier plugin’s permission.asked handling to avoid spamming “Session needs permission” notifications when OpenCode auto-approves requests, by checking whether the specific permission request is still pending before notifying.

Changes:

  • Added getPermissionIDFromEvent to extract the permission request ID from event payloads.
  • Introduced a short delay and a raw HTTP /permission lookup to detect whether the request is still pending.
  • Conditioned the permission notification on the request remaining pending (with an intended “fail-open” fallback).
File summaries
File Description
src/index.ts Adds permission request ID extraction and conditional notification logic based on pending-permission lookup.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
@nghiant03

Copy link
Copy Markdown

Is there an update on when this PR can be merged? Right now this bug can be a bit disruptive to auto-approved workflow.

The pending check now runs before the shared 1s dedupe window is claimed, so a silently skipped auto-approved request no longer mutes a real request that arrives within the same second. Verified with a fake client: auto-approved stays silent, pending notifies, every fail-open path notifies, a real request 250ms after an auto-approved one still notifies, and hook+event for the same request delivers exactly one notification.
@karayelxyz

Copy link
Copy Markdown
Contributor Author

Quick update on the current head (be46edc).

Automated checks: tsc --noEmit clean, bun test 90/90 pass, bun run build clean.

Fail-open behavior corrected (per the Copilot review): the pending check now defaults to "notify" and suppresses only when the pending list is fetched successfully and the request ID is absent. A missing raw client or an unexpected response shape no longer silently swallows notifications.

Dedupe ordering fixed: the shared permission dedupe window is now claimed only when a notification is actually about to fire. Previously an auto-approved request silently consumed that window, so a request that genuinely needed approval and arrived within the same second was deduped away and never notified.

The decision logic was verified by driving the real NotifierPlugin.event handler against a fake client:

Scenario Result
auto-approved (ID absent from the pending list) silent
still pending notifies
missing raw client notifies (fail-open)
get() throws notifies (fail-open)
unexpected response shape notifies (fail-open)
no ID in the event notifies
real request 250 ms after an auto-approved one notifies — 1 delivery
permission.ask hook firing before / after the event exactly 1 delivery

Two limitations worth flagging explicitly so expectations are clear:

  1. Dependency on GET /permission. The pending lookup relies on that route. On older OpenCode servers that do not expose it, the lookup fails, we fall back to notifying, and the auto-approve suppression simply does not apply there. No regression, but no fix on those versions either.

  2. The permission.ask hook path is untouched. The plugin still notifies unconditionally from that hook. On builds where that hook is wired and fires (see feat(permission): restore permission ask hook safely anomalyco/opencode#42633), an auto-approved request would still notify from the hook path even though the event path skips it, so the spam would return there. Out of scope for this change — the hook cannot reuse the pending check, since at hook time the request is created by that very path rather than already pending. Happy to follow up separately if that becomes the behavior on a released version.

The 300 ms grace window is a heuristic: auto-approved requests are resolved immediately by the server, so a request still pending after that delay is treated as a real one. It is deliberately not configurable to keep the change minimal.

@karayelxyz

karayelxyz commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor Author

@nghiant03 Not merged yet, but you can already run it from your own fork:

gh repo fork mohak34/opencode-notifier --clone
cd opencode-notifier
gh pr checkout 98 --repo mohak34/opencode-notifier
bun install
bun run build

Then back up dist/index.js in the notifier package opencode already loads (~/.config/opencode/node_modules/@mohak34/opencode-notifier/dist/index.js) and replace it with the file you just built. Restart opencode.

Heads-up: an npm install / npm update will overwrite the patch — if the notifications come back, just copy the file in again.

@mohak34
mohak34 requested a balanced review from Copilot September 16, 2026 22:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@mohak34
mohak34 merged commit 0e1789c into mohak34:main Sep 17, 2026
pull Bot pushed a commit to niyoh120/opencode-notifier that referenced this pull request Sep 17, 2026
Follow-up to mohak34#98: the inline pending-list probe becomes an exported
isPermissionStillPending helper (fail-open on every lookup failure),
the 300ms grace becomes a named constant, getPermissionIDFromEvent is
exported, and the author's claimed decision matrix is now covered by
tests against stub clients.
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.

Do not send notifications for "Session needs permission" if "auto" mode is enabled.

4 participants