fix(desktop): aivis タスク完了通知が二重に読み上げられる問題を修正#324
Merged
Conversation
## 原因 Codex の完了通知経路が2本存在し、1回のタスク完了に対して /hook/complete が2回叩かれていたため、notificationsEmitter から AGENT_LIFECYCLE が2回 emit され aivis が二重に再生されていた。 - ~/.codex/hooks.json の Stop hook - Codex wrapper の `--enable codex_hooks -c 'notify=[...]'` オプション ## 修正 1. Codex wrapper 側の native `notify=` 注入を除去し、 hooks.json を唯一の完了通知ソースに一本化。 2. NotificationManager / notificationsApp.listen / notificationsEmitter.on(AGENT_LIFECYCLE, ...) の初期化を MainWindow() の外に出し、app.whenReady 直後に initNotifications() で 1回だけ実行するよう整理。ウィンドウ再生成経路でもリスナーが 多重登録されない構造にしておく（将来の再発防止）。 通知音 / aivis いずれも「1回の完了 = 1回」で鳴るようになる。 正規の並列完了（複数エージェントがたまたま同時終了）は許容する方針なので、 dedupe は入れない。
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughデスクトップアプリケーションの通知システムを再構成し、初期化タイミングを早期化しました。同時にCodexラッパーの実装を簡素化し、hooks.jsonベースの設定を優先するよう変更されました。 Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Process
participant NotifMgr as NotificationManager
participant NotifSrv as Notifications Server
participant Lifecycle as Agent Lifecycle
participant Terminal as Terminal Exit
Main->>NotifMgr: initNotifications()
activate NotifMgr
NotifMgr->>NotifSrv: Create & Listen
activate NotifSrv
NotifMgr->>Lifecycle: Register AGENT_LIFECYCLE listener
NotifMgr->>Terminal: Register terminalExit listener
deactivate NotifMgr
Lifecycle-->>NotifMgr: AGENT_LIFECYCLE event
Terminal-->>NotifMgr: TERMINAL_EXIT event
NotifMgr-->>NotifSrv: Emit notifications
Main->>NotifMgr: cleanupMainWindowResources()
activate NotifMgr
NotifMgr->>Lifecycle: Unregister listener
NotifMgr->>Terminal: Unregister listener
NotifMgr->>NotifSrv: Close
deactivate NotifMgr
deactivate NotifSrv
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d024a36716
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts (1)
175-200:⚠️ Potential issue | 🟡 Minorテスト名から completion notifications in wrapper を外してください。
Line 200 の期待値は正しいですが、このテスト名はまだ wrapper が完了通知を注入するように読めます。完了通知は
hooks.json側に一本化されたので、watcher とcodex_hooks有効化の検証に寄せると混乱が減ります。🧪 修正案
- it("injects codex start + permission watchers and completion notifications in wrapper", () => { + it("injects codex start + permission watchers and enables native hooks", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 175 - 200, Rename the test description string for the `it(...)` spec that calls `createCodexWrapper()` so it no longer claims to verify "completion notifications in wrapper" — update the test title to reflect that it only verifies codex start and permission watchers and the `codex_hooks` enablement (for example: "injects codex start + permission watchers in wrapper"); keep all existing assertions unchanged (they reference things like `createCodexWrapper`, the wrapper file content checks such as `_superset_emit_event`, `_superset_approval_fallback_seq`, `"$REAL_BIN" --enable codex_hooks "$@"`, and the various awk/printf checks).apps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.ts (1)
425-434:⚠️ Potential issue | 🟡 Minor
fallback表現が新しい方針と矛盾しています。同じコメント内で Line 427 は
hooks.jsonを fallback と呼んでいますが、Line 433-434 では primary source と説明しています。PR 方針に合わせて primary に統一してください。📝 修正案
/** * Writes Superset hook definitions directly into ~/.codex/hooks.json. - * This provides a fallback notification path that works even when the - * binary wrapper is not in PATH (e.g. user runs codex from outside - * a Superset terminal). + * This provides the primary Codex lifecycle notification path. It also works + * when the binary wrapper is not in PATH (e.g. user runs codex from outside + * a Superset terminal). * * The wrapper now only enables Codex hooks and keeps the session-log watcher🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.ts` around lines 425 - 434, The comment block is self-contradictory: it calls ~/.codex/hooks.json a "fallback" but later calls native hooks.json registration the "primary lifecycle source"; update the comment so it consistently states that hooks.json is the primary registration mechanism—replace the word "fallback" (near the phrase "Writes Superset hook definitions directly into ~/.codex/hooks.json.") with language indicating it is the primary registration/source, and tweak the following sentences (the one starting "The wrapper now only enables Codex hooks..." and the one "Native hooks.json registration remains the primary lifecycle source...") to read consistently (e.g., state that the wrapper provides a best-effort bridge while native hooks.json registration is the primary lifecycle source).apps/desktop/src/main/index.ts (1)
523-558:⚠️ Potential issue | 🔴 Criticalカスタムメディアプロトコルの登録が不足しています。
superset-workspace-mediaスキームが完全に未登録のため、FileViewerContent.tsx で使用される workspace ファイルの音声・動画再生が失敗します。同時にsuperset-temp-audioはstream: true特権が欠落しており、AudioEditor での再生とシーク機能が正常に動作しません。Electron の custom protocol でメディア再生を行うには、stream: true特権の設定が必須です。以下の修正が必要です：
workspace-media-protocol.tsの handler を再度登録し、スキーム特権にstream: trueを追加superset-temp-audioのスキーム特権にstream: trueを追加修正例
import { createTempAudioProtocolHandler } from "./lib/temp-audio-protocol"; +import { createWorkspaceMediaProtocolHandler } from "./lib/workspace-media-protocol"; @@ { scheme: "superset-temp-audio", privileges: { standard: true, secure: true, bypassCSP: true, supportFetchAPI: true, + stream: true, }, }, + { + scheme: "superset-workspace-media", + privileges: { + standard: true, + secure: true, + bypassCSP: true, + supportFetchAPI: true, + stream: true, + }, + }, @@ const tempAudioHandler = createTempAudioProtocolHandler(); protocol.handle("superset-temp-audio", tempAudioHandler); session .fromPartition("persist:superset") .protocol.handle("superset-temp-audio", tempAudioHandler); + + const workspaceMediaHandler = createWorkspaceMediaProtocolHandler(); + protocol.handle("superset-workspace-media", workspaceMediaHandler); + session + .fromPartition("persist:superset") + .protocol.handle("superset-workspace-media", workspaceMediaHandler);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/index.ts` around lines 523 - 558, The custom media protocol registration is missing `superset-workspace-media` and `superset-temp-audio` lacks the required `stream: true` privilege, causing workspace media playback and AudioEditor seek to fail; fix by adding a registration entry for the `superset-workspace-media` scheme in the protocol.registerSchemesAsPrivileged array and ensure both `superset-temp-audio` and `superset-workspace-media` privilege objects include "stream: true", and re-register the handler from workspace-media-protocol.ts (the handler registration call used previously) so the workspace-media handler is active when FileViewerContent.tsx and AudioEditor request media.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/index.ts`:
- Around line 760-761: Move the call to initNotifications() so it runs
immediately after app.whenReady() instead of after setupAgentHooks() and other
awaits; locate the app.whenReady() initiation and call initNotifications() there
(before invoking setupAgentHooks(), before awaiting makeAppSetup(() =>
MainWindow()), and before any async setup that could delay the notification
server/listener), ensuring notification server/listener is initialized early to
avoid missing /hook/complete events from Codex.
In
`@apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.sh`:
- Line 75: ドキュメントの説明が実装と矛盾しているため、apps/desktop/docs/EXTERNAL_FILES.md の「wrapper が
notify を inject する」と書かれた記述を削除または更新し、現在の実装（wrapper が "$REAL_BIN" --enable
codex_hooks "$@" を呼び出すだけで `notify=[...]` を注入しない）を明示してください；具体的には
EXTERNAL_FILES.md 内の notify に関する節を見つけて、inject を前提とした手順やサンプルを修正し、notify
を渡す方法（呼び出し側で明示的に引数として渡す等）を短く追記してください。
---
Outside diff comments:
In `@apps/desktop/src/main/index.ts`:
- Around line 523-558: The custom media protocol registration is missing
`superset-workspace-media` and `superset-temp-audio` lacks the required `stream:
true` privilege, causing workspace media playback and AudioEditor seek to fail;
fix by adding a registration entry for the `superset-workspace-media` scheme in
the protocol.registerSchemesAsPrivileged array and ensure both
`superset-temp-audio` and `superset-workspace-media` privilege objects include
"stream: true", and re-register the handler from workspace-media-protocol.ts
(the handler registration call used previously) so the workspace-media handler
is active when FileViewerContent.tsx and AudioEditor request media.
In
`@apps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.ts`:
- Around line 425-434: The comment block is self-contradictory: it calls
~/.codex/hooks.json a "fallback" but later calls native hooks.json registration
the "primary lifecycle source"; update the comment so it consistently states
that hooks.json is the primary registration mechanism—replace the word
"fallback" (near the phrase "Writes Superset hook definitions directly into
~/.codex/hooks.json.") with language indicating it is the primary
registration/source, and tweak the following sentences (the one starting "The
wrapper now only enables Codex hooks..." and the one "Native hooks.json
registration remains the primary lifecycle source...") to read consistently
(e.g., state that the wrapper provides a best-effort bridge while native
hooks.json registration is the primary lifecycle source).
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts`:
- Around line 175-200: Rename the test description string for the `it(...)` spec
that calls `createCodexWrapper()` so it no longer claims to verify "completion
notifications in wrapper" — update the test title to reflect that it only
verifies codex start and permission watchers and the `codex_hooks` enablement
(for example: "injects codex start + permission watchers in wrapper"); keep all
existing assertions unchanged (they reference things like `createCodexWrapper`,
the wrapper file content checks such as `_superset_emit_event`,
`_superset_approval_fallback_seq`, `"$REAL_BIN" --enable codex_hooks "$@"`, and
the various awk/printf checks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5215e15-8869-4124-a1cf-10b05413fc7e
📒 Files selected for processing (5)
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-claude-codex-opencode.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers.test.tsapps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.shapps/desktop/src/main/windows/main.ts
- index.ts の workspace-media-protocol 登録と stream 特権を誤って削除していたのを復旧 (CodeRabbit critical 指摘) - createCodexHooksJson のコメントを primary/fallback で矛盾しないよう修正 - codex wrapper テスト名から completion notifications 表記を除去 - biome フォーマット修正 (main.ts / agent-wrappers.test.ts)
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
Aivis によるタスク完了の音声読み上げが、1回の完了に対してほぼ同時に二重で再生されてしまう不具合を修正します。通知音（リングトーン）は短い効果音のため重複が聞き取りづらかったものの、実際には同様に二重発火していました。
原因
Codex の完了通知経路が2本存在し、同じ完了イベントが
notify.sh→/hook/completeに2回到達していたのが主因です。両経路がそれぞれ `/hook/complete` を叩くため、`notificationsEmitter` から `AGENT_LIFECYCLE` が2回発火し、`NotificationManager.handleAgentLifecycle` → `playAivisNotification` が2回呼ばれていました。
加えて、`MainWindow()` 内で `notificationsApp.listen` / `new NotificationManager` / `notificationsEmitter.on(AGENT_LIFECYCLE, ...)` を初期化していたため、macOS で全ウィンドウを閉じてから Dock 再アクティベートする経路 (`app.on("activate")` → `MainWindow()` 再実行) でリスナーが多重登録される構造的リスクも残っていました。
Claude Code / OpenCode / Cursor / Copilot / Gemini は通知経路が1本のため影響なく、本件は Codex セッション終了時に発生していました。
修正内容
1. Codex wrapper の `notify=` 注入を除去
`apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.sh`
```diff
-"$REAL_BIN" --enable codex_hooks -c 'notify=["bash","{{NOTIFY_PATH}}"]' "$@"
+"$REAL_BIN" --enable codex_hooks "$@"
```
`hooks.json` の Stop hook を唯一の完了通知ソースとし、重複配送を根本から止めます。wrapper 内の TUI session log watcher（prompt 開始・権限要求の検知）は引き続き必要なので残しています。
2. 通知系初期化を `app.whenReady` 直後に1回だけ走らせる
`apps/desktop/src/main/windows/main.ts` から通知系初期化を `initNotifications()` として切り出し、`apps/desktop/src/main/index.ts` の `app.whenReady` 後で1回だけ呼ぶように変更しました。`notificationsInitialized` フラグで冪等性を担保しています。
`MainWindow()` は BrowserWindow の生成のみを担当するようにし、ウィンドウ再生成時にリスナーが多重登録される経路を構造的に排除しました。
やらなかったこと（意図）
入口での dedupe Map による短時間の重複抑制は採用しません。ユーザー側が「複数エージェントがたまたま同時に終了して2回鳴る」という正規の並列完了を許容しているため、dedupe を入れると意図的な連続完了まで抑えてしまい、症状の隠蔽で別バグを生む可能性があるためです。重複ソースを根本から1本化する方針で対応しています。
動作確認ポイント
Summary by CodeRabbit
リリースノート
バグ修正
削除