Fix duplicate post-turn idle blocked alerts for unchanged sessions#1175
Fix duplicate post-turn idle blocked alerts for unchanged sessions#1175Yeachan-Heo merged 2 commits intodevfrom
Conversation
notify-hook re-entered the session-idle path after every lead-session turn while the idle cooldown module only remembered the last send timestamp. Once the timer elapsed, OMX could re-announce the same blocked idle state with no transition gate. This change fingerprints the idle summary at the emit site and persists that fingerprint in idle-cooldown state. Unchanged idle fingerprints stay suppressed, while progress, terminal states, or summary changes reopen the alert path without broadening downstream hook contracts. Constraint: Fix must stay source-side and avoid downstream hook contract changes Rejected: Hook-local in-memory dedupe | notify-hook runs per turn and needs durable state Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep idle fingerprints compact and user-visible; do not add non-meaningful fields without regression coverage Tested: npm run build Tested: npx biome lint src/scripts/notify-hook.ts src/notifications/idle-cooldown.ts src/notifications/__tests__/idle-cooldown.test.ts Tested: npm run check:no-unused Tested: node --test dist/notifications/__tests__/idle-cooldown.test.js dist/cli/__tests__/lifecycle-notifications.test.js Not-tested: End-to-end lifecycle delivery against a live notification provider
![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: 5b47e019ef
ℹ️ 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".
src/notifications/idle-cooldown.ts
Outdated
|
|
||
| // Cooldown of 0 means disabled — always send | ||
| if (cooldownSecs === 0) return true; | ||
| if (cooldownSecs === 0 && !normalizedFingerprint) return true; |
There was a problem hiding this comment.
shouldSendIdleNotification now treats OMX_IDLE_COOLDOWN_SECONDS=0 as “always send” only when no fingerprint is passed, but notify-hook always passes a fingerprint. In that common path, unchanged fingerprints are still suppressed, which regresses the documented behavior that cooldown 0 disables throttling entirely and can silently drop repeated idle notifications/hooks for users who explicitly opted out of throttling.
Useful? React with 👍 / 👎.
| .split('\n') | ||
| .map((line) => line.trim()) | ||
| .filter(Boolean); | ||
| const preferred = source.at(-1) || ''; | ||
| const normalized = preferred.replace(/\s+/g, ' ').trim(); |
There was a problem hiding this comment.
The fingerprint summary is taken from the last non-empty line of the assistant message. For multi-line replies that end with stable boilerplate (for example, a repeated closing prompt), different idle/blocker updates can collapse to the same summary and be treated as unchanged, so shouldSendIdleNotification suppresses notifications even when the underlying idle reason changed.
Useful? React with 👍 / 👎.
|
OMX review session result: request-changes. Blocker summary:
Repro used in review:
Please restore the zero-cooldown opt-out semantics in the fingerprinted path, then I'll re-check quickly. — |
PR #1175 still suppressed repeated idle notifications when fingerprints were present because the zero-cooldown path only bypassed throttling for non-fingerprinted sends. This follow-up makes cooldown=0 short-circuit before any persisted fingerprint comparison and adds regression coverage for unchanged fingerprint repeats. Constraint: Preserve existing fingerprint dedupe whenever cooldown remains above zero Rejected: Drop persisted fingerprints when cooldown is zero | unnecessary write-path coupling and the read path should already treat zero as a global bypass Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep zero-cooldown semantics ahead of both timestamp and fingerprint checks so disabled throttling stays consistent across idle notification paths Tested: npm run build; node --test dist/notifications/__tests__/idle-cooldown.test.js; direct pre/post reproduction script for unchanged fingerprint with OMX_IDLE_COOLDOWN_SECONDS=0 Not-tested: Full npm test suite
|
Follow-up fix pushed for the blocker around Fix summary
Verification
Pushed on |
1 participant
Summary
Root cause
src/scripts/notify-hook.tsre-entered thesession-idleemit path after every lead-session turn, butsrc/notifications/idle-cooldown.tsonly trackedlastSentAt. Once the cooldown elapsed, OMX could re-announce the same unchanged idle state and emit anotherpost_turn_idle_notificationblocked event without any transition gate.Verification
npm run buildnpx biome lint src/scripts/notify-hook.ts src/notifications/idle-cooldown.ts src/notifications/__tests__/idle-cooldown.test.tsnpm run check:no-unusednode --test dist/notifications/__tests__/idle-cooldown.test.js dist/cli/__tests__/lifecycle-notifications.test.js