Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The idea is right, but as written this can hide alerts for permissions that are still waiting for the user. I checked the code and ran the new tests (bun test test/attention.test.ts: 3 pass) and the type check (bun run typecheck in packages/tui: clean). I couldn't play audio in the sandbox, so I didn't check the sound end to end.
Public API: unchanged (createTuiAttention's new permissionMode input is internal; its only caller is AttentionProvider).
Concerns:
- Pending permissions can go silent. In the v2 TUI, auto-accept happens in the client: only the open session route replies to permission requests, and only for the open session and its subagents (
routes/session/index.tsx, theautoApprovedeffect). A permission from a session in another tab, or one that arrives while you're on the home screen, is not approved and stays pending. With this change it also plays no sound, so the user may never notice it. The sound should be skipped only for requests this client will actually auto-approve. - Layer.
createTuiAttentionbackscontext.attention.notify, which plugins use. Hard-coding thepermissionsound name there silences any caller that asks for that sound. Thepermission.askedhandler infeature-plugins/system/notifications.tsis a better place, since it knows the request's session and can apply the rule from (1). - The terminal notification for the same event still fires when the terminal isn't focused. That's the spam described in #53525, so that issue is only partly addressed.
|
/opencode The spam described in #53525 seems like it should have it's own issue. |
96849b1 to
0c10bae
Compare
c78b606 to
54f3db7
Compare
|
/opencode review the latest changes |
|
User kyan0045 does not have write permissions |
|
@simonklee since you own #48579, would you mind taking a look/re-running the review whenever you have a moment? |
There was a problem hiding this comment.
My two earlier concerns are resolved. The check now sits in the permission.asked handler in notifications.ts and skips the alert only for requests this client actually auto-approves (the open top-level session and its subagents). Pending permissions elsewhere still alert, and createTuiAttention is generic again. It also suppresses the terminal notification for those requests, not only the sound.
I ran bun test test/cli/cmd/tui/notifications.test.ts (13 pass) and bun run typecheck in packages/tui and packages/plugin (clean). Sound playback can't be observed in the sandbox, so I didn't check it end to end in a running TUI.
Public API: the plugin API gains ui.permission.mode(): "prompt" | "autoaccept" and an exported PermissionMode type in @opencode/plugin/tui/context. Existing plugins keep working. This does make the permission mode part of the public plugin API, which maintainers should agree to.
No other concerns.
Issue for this PR
Closes #48579
Type of change
What does this PR do?
The TUI alerted (sound plus terminal notification) whenever the agent asked for permission, even when that request was auto-approved and never shown to the user. Two paths auto-approve: the auto-approve permission setting, and the
--autoflag. The permission request still fires in both, so the alert was noise.The alert is now skipped only for requests this client will actually auto-approve: the open session and its subagents while the live permission mode is auto-accept. Requests from another tab, from the home screen, or from a session opened as a subagent still alert, because those are not auto-approved and stay pending. Question, done, error, and other sounds are unaffected. This also covers the
--autocase reported in #53525.The check lives in the
permission.askedhandler infeature-plugins/system/notifications.ts, socreateTuiAttentionstays generic. To read the live mode there, the TUI plugin context gainsui.permission.mode()and an exportedPermissionModetype. Both are additive, so existing plugins are unaffected. Flagging it because it does widen the public plugin API.How did you verify your code works?
bun test test/cli/cmd/tui/notifications.test.tsinpackages/tuicovers auto-accepted requests staying silent, requests outside the open session still alerting, and subagent-applied requests staying silent.bun typecheckis clean inpackages/tuiandpackages/plugin.Screenshots / recordings
Checklist
Added by the opencode bot (AI-generated).
Fixes #53525
Related: #52486
Public API: adds ui.permission.mode() and the PermissionMode type to the TUI plugin context (backwards compatible)