feat/fix: add system notification message format config and fix idle notification not firing#1674
Open
centraldogma99 wants to merge 12 commits intocode-yeongyu:devfrom
Open
feat/fix: add system notification message format config and fix idle notification not firing#1674centraldogma99 wants to merge 12 commits intocode-yeongyu:devfrom
centraldogma99 wants to merge 12 commits intocode-yeongyu:devfrom
Conversation
…with 200 LOC rule
…nd {cwd} template variables
Contributor
|
All contributors have signed the CLA. Thank you! ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
…idle timer cancellation message.updated events with role 'user' (metadata updates like summary) were incorrectly treated as new agent activity, cancelling the idle notification timer ~21ms after session.idle. Now only role 'assistant' messages mark session activity, matching the pattern used by cli/run/events.ts and todo-continuation-enforcer.ts.
…add edge case tests Move cleanupOldSessions from platform module to utils module for better separation of concerns. Add tests for empty format string and missing template variables edge cases.
Replace toBe with exact macOS osascript command strings with toContain for message content only. Also normalize BDD comment style from //#given to // given for consistency.
Author
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
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
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.
Problem
The system notification message shown when an agent completes execution is hardcoded, making it difficult to identify which project's work has completed when working across multiple projects in parallel.
Solution
Expose the project name and working directory in the notification message, and allow users to freely customize the format through configuration. Additionally, fixed a bug discovered during development where notifications were never actually being sent.
Summary
message_formatconfig option to customize OS notification messages with{project}(folder name) and{cwd}(full path) template variablessession-notification.tsto comply with 200 LOC rule by extracting platform functions and cleanup logic into separate modulesindex.tswheremessage_formatwas not forwarded to the notification hookmessage.updatedbug: notifications were never firing becausemessage.updatedwithrole: "user"(metadata updates) was incorrectly cancelling the idle notification timer ~21ms aftersession.idleChanges
New Files
src/hooks/session-notification-format.ts— Format resolver withextractProjectName()andresolveMessageFormat()usingString.replace()src/hooks/session-notification-format.test.ts— 10 TDD tests covering project name extraction, template variable resolution, edge casessrc/hooks/session-notification-platform.ts— Extracted platform detection, notification sending, sound playback, session cleanupsrc/hooks/session-notification-utils.ts— MovedcleanupOldSessionsfor better separation of concernsModified Files
src/config/schema.ts— Addedmessage_format: z.string().optional()toNotificationConfigSchemasrc/hooks/session-notification.ts— Imports extracted modules, resolves format before sending notification; filtersmessage.updatedbyrole === "assistant"to fix notification bugsrc/hooks/session-notification.test.ts— Tests for default/custom format, literal strings, assistant vs user role filteringsrc/index.ts— Passesmessage_formatfrom plugin config to notification hookBug Fix: Notifications Never Firing
Root Cause
After
session.idlefires, OpenCode emits amessage.updatedevent (~21ms later) to update the user message'ssummarymetadata. The old code treated allmessage.updatedevents as "new agent activity", callingcancelPendingNotification()which cleared the idle notification timer before it could fire.Fix
Filter
message.updatedby role — onlyrole === "assistant"marks session activity. This matches the existing pattern incli/run/events.ts(line 269) andtodo-continuation-enforcer.ts(line 450).Usage
Template Variables
{project}path.basename)my-app{cwd}/Users/me/projects/my-appUnrecognized variables (e.g.,
{unknown}) pass through as-is — no errors.Backward Compatibility
"{project} — Agent is ready for input"(em dash U+2014)notification.force_enablecontinues to work as beforeVerification
bun run typecheck— zero errorsbun run build— succeedsbun test— 15 session-notification tests pass, all project tests pass