Repository navigation
feat(vscode): add task attention notifications - #13771
marius-kilocode merged 12 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const text = this.text(sound) | ||
| if (!text) return | ||
| const windows = | ||
| config.get<boolean>("windowsNotifications", false) && !(this.opts.focused?.() ?? vscode.window.state.focused) |
There was a problem hiding this comment.
WARNING: OS-notification channel is selected on non-Windows hosts
When windowsNotifications is true and VS Code is unfocused, notify() treats the event as Windows-only and skips the workbench toast. showWindowsNotification() then returns immediately unless process.platform === "win32".
The setting is not machine-local, so Settings Sync or a copied settings.json from Windows will silently drop completion, input, and error alerts on macOS and Linux.
| config.get<boolean>("windowsNotifications", false) && !(this.opts.focused?.() ?? vscode.window.state.focused) | |
| config.get<boolean>("windowsNotifications", false) && process.platform === "win32" && !(this.opts.focused?.() ?? vscode.window.state.focused) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 4051293.
Rather than an inline process.platform check at this line, the OS channel is now gated on the handler being wired up at all: extension.ts passes windows only when process.platform === "win32", and the new channel() helper requires this.opts.windows before it can select "os". A windowsNotifications: true value arriving via Settings Sync or a copied settings.json on macOS/Linux therefore falls through to the workbench toast instead of being silently dropped.
Gating on the injected handler instead of the platform keeps AttentionService platform-agnostic and testable.
Regression test: falls back to a VS Code notification when the OS channel is unavailable.
| `[Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier('${app()}').Show($toast)`, | ||
| ].join("; ") | ||
| const encoded = Buffer.from(script, "utf16le").toString("base64") | ||
| void exec("powershell.exe", ["-NoProfile", "-NonInteractive", "-EncodedCommand", encoded]).catch((error) => { |
There was a problem hiding this comment.
WARNING: Unbounded fire-and-forget PowerShell processes
Each OS toast spawns powershell.exe with no timeout and no concurrency cap. The sibling playSound() path already queues work (limit = 3) for this reason. Rapid events (parallel Agent Manager sessions, permission/question bursts) or a hung Windows Runtime toast API can leave leaked PowerShell processes for the life of the extension host.
Consider a short timeout on exec() and a small in-flight limit similar to sound.ts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 4051293.
Adopted both suggestions, mirroring sound.ts:
- In-flight cap of 3 (
queued >= limitreturns early), so bursts from parallel Agent Manager sessions or permission/question storms cannot fan out. - Work is serialized on a promise
chaininstead of firing concurrently, andexec()now gets a 10stimeoutso a hung Windows Runtime toast call cannot leak a PowerShell process for the life of the extension host.
Dropping the alert when the queue is saturated is intentional: a toast that arrives late is worse than no toast, and the cap is only reachable when several notifications land within the same window.
| } | ||
| const details = this.opts.details?.(sessionID, directory) | ||
| if (!details) return send() | ||
| void details.then(send, () => send()) |
There was a problem hiding this comment.
SUGGESTION: Re-check focus/visibility after session details resolve
windows and notification are captured before details() (getSessionInfo plus retries). send() still uses those stale flags. If the user focuses VS Code or switches to the originating session during that round-trip, they can get an OS toast they no longer need, or a workbench toast for a session they are already viewing.
Consider re-evaluating focused/visible inside send.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 4051293.
Good catch — details() performs an HTTP round-trip, so the captured flags could easily be stale by the time it resolved.
The channel decision now lives in channel(sessionID), which re-reads configuration, focus, and the visible session. It is called twice: once as a fast path so a fully disabled setup never triggers the getSessionInfo round-trip, and again inside send() so the resolved alert is routed against current state. If nothing applies at that point the alert is dropped.
Two regression tests cover the transitions:
skips the alert when the session becomes visible while details resolveuses the VS Code channel when the window regains focus while details resolve
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summaries (4 snapshots, latest commit dd72b1d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit dd72b1d)Status: 3 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Files Reviewed (20 files)
Previous review (commit 43e6a6e)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 4051293)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous review (commit 1bf83fa)Status: 3 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (33 files)
Reviewed by grok-4.6 · Input: 174.9K · Output: 10.4K · Cached: 259.6K Review guidance: REVIEW.md from base branch |
|
@sylwester-liljegren so if I understand this right: This PR adds native notifications on Windows, but on mac and linux we use a popup? Not sure if we should have native parity on all platforms? We probably want to scope that out and only continue if this is fully possible on all platforms. Hope that makes sense, and thanks for the effort already! |
|
@marius-kilocode Well, I don't have access to other OS's, only to Windows 11, hence this PR being limited to Windows OS while preparing this PR. I can look into how I can test and perhaps implement the same thing for MacOS and Linux. If the OS notifications part might be a blocker right now, I definitely think that VS Code Notifications already enhance the experience and that should work across all platforms as the VS Code notifications are bound to the editor and not any specific OS (which in my opinion is far better than having just sound only). So we have a couple of options:
Personally, I would lean toward the second option as we can separate VS Code notifications more clearly from OS notifications and keep each PR within reasonable limit, because we can't estimate the size of future changes to provide support for OS notifications on all platforms at the same time. Plus, we get at least an improved notification system within VS Code within reasonable time. |
Extend attention alerts beyond Windows: build the notification command per platform (WinRT via PowerShell, osascript on macOS, notify-send on Linux) behind a shared serialized queue, and rename the setting and module from the Windows-only naming to OSNotifications / os.ts. The OS and VS Code channels now fire independently rather than exclusively. The OS toast is a transient ping for when no window is in view; the VS Code notification persists as an actionable Show entry that opens the session and jumps to the latest message instead of restoring the saved scroll position. Add a Test button under the toggle, shown only while OS notifications are enabled, that sends a real native notification and reports the actual command result so users can verify their desktop environment supports it.
Resolve the setSidebarVisible conflict by keeping both additions: this branch's sidebarVisible flag, which backs the session-aware notification suppression, and upstream's setStatsVisible call. Move the notification settings messages out of the webview message switch into handleNotificationSettingsMessage, following the existing handleMigrationMessage pattern. Upstream added cases to the same switch, pushing it past the 150 complexity limit once this branch's testOSNotification case landed alongside them.
| return { cmd: "osascript", args: ["-e", `display notification "${apple(text(notice))}" with title "Kilo Code"`] } | ||
| } | ||
| if (platform === "linux") { | ||
| return { cmd: "notify-send", args: ["--app-name=Kilo Code", "--urgency=normal", "Kilo Code", text(notice)] } |
There was a problem hiding this comment.
WARNING: Linux notify-send body is unescaped Pango markup
Windows XML-escapes session/workspace text and macOS AppleScript-escapes it, but the Linux body is passed through raw. libnotify parses that body as Pango, so a workspace like foo & bar or a session title containing </> can drop the toast or render injected markup. Consider passing --escape, or escaping &, <, and > in the body, before spawning notify-send.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 4cb8053. You're right, and the failure mode is worse than cosmetic: libnotify parses the body as Pango markup, so an unescaped < makes some daemons reject the notification outright rather than just render it oddly.
Added a pango() helper that escapes &, <, and > in the body only. Two deliberate choices there:
- I did not reuse the existing XML
escape(), because it also converts"and'. Those are not markup-significant, and on a daemon that does not advertise thebody-markupcapability the entities are displayed literally — a session title would readit's broken. Escaping only the three markup characters keeps quotes readable either way. - I did not use
--escape, since it is not a portablenotify-sendoption and relying on it would trade one breakage for another.
The summary argument is left raw, as it is taken literally rather than parsed as markup. Covered by a new case in attention-os.test.ts that asserts both the escaping and that quotes survive untouched.
| if (pendingRestore() !== id) return | ||
| const el = scrollEl() | ||
| if (!el) return | ||
| if (session.consumeScrollBottom(id)) { |
There was a problem hiding this comment.
WARNING: Show does not jump to latest when the session is already selected
consumeScrollBottom only runs inside the restore effect armed by on(session.currentSessionID). Solid skips that effect when openSession re-selects the same id, so scrollToBottom is ignored. That is the path when this session is still selected (for example the user is in another editor group) and they click Show. Consider consuming the flag and calling forceScrollToBottom even when currentSessionID does not change.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Good catch, fixed in 4cb8053. The analysis is exactly right: the restore pass was armed only by on(session.currentSessionID), so re-selecting the same id was a no-op and the request sat unconsumed. That is precisely the Show-while-already-selected path, which is a normal case when the session is open in another editor group.
Rather than duplicate the scroll logic, I exposed scrollBottomID on the session context and added a second effect that arms the same pendingRestore pass when a request lands for the current session. Reusing the existing machinery means it still waits out loading() and the requestAnimationFrame before scrolling, and the consume-first branch resolves it to forceScrollToBottom(). When the session does change, both effects set the same id, which is idempotent.
| console.warn("[Kilo New] Cannot select cloud preview session via selectSession") | ||
| return | ||
| } | ||
| if (options.scrollToBottom) setScrollBottomID(id) |
There was a problem hiding this comment.
WARNING: Pending scroll-to-bottom can leak across session switches
scrollBottomID is set only when scrollToBottom is true and is cleared only by consumeScrollBottom. A later selectSession without that flag leaves the pending id in place. If MessageList has not consumed it yet (loading() is true, so the restore effect returns early), a later normal switch back to that session jumps to the bottom and skips saved-scroll restore.
| if (options.scrollToBottom) setScrollBottomID(id) | |
| setScrollBottomID(options.scrollToBottom ? id : undefined) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 4cb8053, taking your suggestion as written. The leak is real: MessageList returns early while loading(), so the request could outlive the selection that created it and then hijack the saved scroll position on a later switch back to that session.
selectSession now always reassigns, so a plain selection clears a pending request instead of leaving it armed. Added a regression test asserting the old conditional form is gone, alongside the existing one-shot coverage.
Escape Pango markup in the Linux notify-send body. libnotify parses it as markup, so a workspace or session name containing & or < could make the daemon reject the notification. Quotes are left alone since they are not markup and would otherwise render as entities on daemons without markup support. Arm the MessageList restore pass from the scroll-to-bottom request as well as from a session change. Clicking Show for the session that is already selected leaves currentSessionID untouched, so the request was never consumed and the view stayed where it was. Clear an unconsumed scroll-to-bottom request on a later plain selection. MessageList returns early while loading, so a request could survive and override the saved scroll position on a future switch back.
|
@marius-kilocode I have now implemented support for both MacOS and Linux. I am yet to test how these OS notifications work on each platform, so I'll have to get back to you later on with evidence that they work on all the other platforms beside Windows. |
|
@sylwester-liljegren thanks, we can scope out a platform and ship that in a follow up if needed. |
|
@marius-kilocode On Linux (Ubuntu), look up to the upper right corner: 
Smaller video demonstrating the notification flow end-to-end: linux_os_notifications.1.mp4I'll try with MacOS within soon time, but I might be delayed with providing evidence. In my opinion, I think we might as well try MacOS first and then just ship this notification system right away unless you have any clear objections to this proposed idea? |
|
I can test this on mac tmr. It looks already really good in that way :) |
|
@marius-kilocode , sounds perfect if you could do that. I'll make sure to resolve the merge conflicts before tomorrow so you can test with desired outcome and, hopefully, we can ship this to the main branch :) |
Resolve the extension.ts import conflict by keeping both sides: this branch's showOSNotification import alongside upstream's new caffeination service imports. Both sides had edited the same import line.
| - Add a setting to enable/disable notifications (default: on) | ||
| - Notifications should include a "Show" button that focuses the webview panel | ||
| - Only fire when `KiloProvider.panel.visible` is false | ||
| - `kilo-code.new.attention.notifications` enables VS Code workbench notifications (default: off). |
There was a problem hiding this comment.
Do we still need the plan?
There was a problem hiding this comment.
No, it can be removed. I'll fix it soon.
There was a problem hiding this comment.
Removed. The file was the pre-implementation plan (its original "Remaining Work" section is what this PR implements), so it has been deleted along with its row in docs/opencode-migration-plan.md, which also tracked it as outstanding work.
| "[Windows.UI.Notifications.ToastNotificationManager, Windows.UI.Notifications, ContentType = WindowsRuntime] > $null", | ||
| "[Windows.Data.Xml.Dom.XmlDocument, Windows.Data.Xml.Dom, ContentType = WindowsRuntime] > $null", | ||
| "$xml = New-Object Windows.Data.Xml.Dom.XmlDocument", | ||
| `$xml.LoadXml('${xml}')`, |
There was a problem hiding this comment.
[P1] Can we keep the notification XML out of the PowerShell source? XML escaping does not escape Unicode smart single quotes (U+2018/U+2019), which PowerShell also treats as string delimiters. A crafted session title or workspace name can therefore break out of this string and execute commands when a native notification is sent on Windows. Encoding the whole script with -EncodedCommand does not prevent this.
Could we Base64-encode the XML separately and decode it inside a fixed script, with a regression test for smart quotes? This is based on the generated script and documented PowerShell parsing behavior, not a Windows runtime reproduction: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-5.1#including-quote-characters-in-a-string
There was a problem hiding this comment.
Good catch, and the analysis is right — fixed.
The toast XML and the app identity are now passed to PowerShell as environment variables (KILO_TOAST_XML as base64, KILO_TOAST_APPID) and decoded inside a fixed script:
$xml.LoadXml([Text.Encoding]::UTF8.GetString([Convert]::FromBase64String($env:KILO_TOAST_XML)))
...
[Windows.UI.Notifications.ToastNotificationManager]::CreateToastNotifier($env:KILO_TOAST_APPID).Show($toast)
No notice content or identity is interpolated into script source any more, so the smart-quote class of breakout has nothing to break out of. env is merged over process.env rather than replacing it, since execFile otherwise wipes PATH/SystemRoot.
Regression test added in attention-os.test.ts (cannot be escaped by Unicode smart quotes in a session title): it feeds a session title of \u2019; Start-Process calc.exe; ', asserts the decoded script contains neither Start-Process nor U+2019, and asserts the payload still arrives intact as inert data. A second test asserts the workspace name, session title, and identity never appear in the script at all.
| } | ||
|
|
||
| public async showMemory(): Promise<void> { | ||
| private async memory(action: "showMemory" | "toggleMemory", failure: string): Promise<void> { |
There was a problem hiding this comment.
This refactoring seems unrelated, why do we need it for the purpose of this feature?
There was a problem hiding this comment.
You're right that it did not belong here — reverted. AgentManagerProvider.ts is now byte-identical to main.
For context on why it was there: the feature needed a 4-line getActiveSessionId() accessor for the "is this session already on screen" check, and that pushed the file to 1904 lines against the hard 1900-line cap in agent-manager-arch.test.ts, so I had deduplicated showMemory/toggleMemory to make room. That was solving the wrong problem.
The better fix was to stop needing the accessor. Every surface already registers the session it displays via connectionService.registerVisible(...), gated on its own panel visibility, so suppression now asks connectionService.isVisible(sessionID) instead. That covers the sidebar, Kilo editor tabs, and Agent Manager through one existing mechanism, and it also let me drop the sidebarVisible field and isSidebarVisible() I had added to KiloProvider. Net effect: no Agent Manager change at all, and a smaller diff than before.
| } | ||
|
|
||
| function app() { | ||
| return vscode.env.appName.includes("Insiders") ? "Microsoft.VisualStudioCodeInsiders" : "Microsoft.VisualStudioCode" |
There was a problem hiding this comment.
Can we use the running editor’s own Windows notification identity instead of treating every non-Insiders host as Microsoft VS Code? We should support Cursor and Google Antigravity too.
Could we read and cache win32AppUserModelId from the host’s product.json under vscode.env.appRoot? That would avoid maintaining a hardcoded editor-name mapping. Since this is an internal file, we should verify the location on Windows and fail safely when the file or field is unavailable, keeping workbench notifications working and explaining the limitation through the Test button.
Please validate the value and pass it into PowerShell as data rather than interpolating it into script source, as with the XML fix. We should test native delivery in VS Code, Cursor, and Antigravity.
There was a problem hiding this comment.
Agreed — the hardcoded mapping is gone.
readAppID(appRoot) now reads win32AppUserModelId from the running host's own product.json under vscode.env.appRoot, so Cursor, Antigravity, and any other fork are attributed to themselves. The result is cached for the session, since appRoot cannot change while the host is running.
Validation and fail-safe behavior, as requested:
- The value must be a string, non-empty after trimming, at most 256 characters, and free of control characters. Anything else is treated as unavailable.
- A missing file, malformed JSON, or missing field is caught and logged at debug level.
- When no identity can be established,
notificationCommanddeclines to build a Windows command at all rather than guessing. Nothing is spawned, the workbench notification channel is unaffected, and the Test button surfaces the reason through the new localizedkilocode:attention.identitystring.
The identity is passed through $env:KILO_TOAST_APPID rather than interpolated, per your other comment.
Unit tests cover a fork identity, missing/malformed/absent-field product.json, and each rejected value shape. What I have not done is verify real native delivery in VS Code, Cursor, and Antigravity — I have only tested VS Code on Windows 11. The path degrades to "no native toast + explicit Test button reason" rather than a wrong-app toast, but a check on Cursor and Antigravity before merge would be worth it.
| return { os, vscode: panel } | ||
| } | ||
|
|
||
| private text(sound: TuiAttentionSoundName) { |
There was a problem hiding this comment.
Can we translate all four notification messages here, along with Show and the Workspace/Session labels in message()? Both native and workbench notifications should follow the selected language rather than always using English. Please use the same extension-host localization approach for this and os.ts.
There was a problem hiding this comment.
Done, using the existing extension-host services/i18n layer (the same t() used by the autocomplete strings), which resolves kilo-code.new.language first and falls back to vscode.env.language.
New kilocode:attention.* keys, added for all 21 locales under src/services/i18n/attention/:
- the four messages:
done,question,permission,error showfor the action buttonworkspaceandsessionfor the labelstestfor the test notification, plusunsupportedandidentityfor the two Test button failure reasons
The Workspace/Session labels were duplicated between the workbench notification and the native toast, so they now come from one shared notice.ts helper — the two channels stay in the same field order and differ only in separator (\n for native, | for the plaintext workbench toast).
The Kilo Code brand name and the actual workspace and session names are passed through untranslated. i18n-keys.test.ts passes, which enforces both that every key exists in all locales and that every literal key used in code is defined.
|
|
||
| /** Sends a real native notification and reports whether the underlying command succeeded. */ | ||
| export function testOSNotification(platform = process.platform): Promise<{ ok: boolean; error?: string }> { | ||
| const command = notificationCommand({ message: "This is a test notification from Kilo Code." }, platform) |
There was a problem hiding this comment.
Can we translate the test notification message and the other user-facing strings in this file, including Workspace/Session labels and the unsupported-platform message? These should follow the selected language, just like the notification settings UI. The Kilo Code brand and actual workspace/session names should stay unchanged.
There was a problem hiding this comment.
Covered by the same change as the service.ts comment. In this file specifically:
- the test notification message is now
t("kilocode:attention.test") - the unsupported-platform message is
t("kilocode:attention.unsupported") - the new "identity unavailable" reason is
t("kilocode:attention.identity") - the
Workspace/Sessionlabels come from the sharednotice.tshelper, so the native toast and the workbench notification use identical wording
Kilo Code stays as-is in the --app-name, the macOS title, and the notification summary, and the workspace and session values are never translated.
| activeTabProvider()?.getCurrentSessionId() === sessionID || | ||
| (agentManagerProvider.isActive() && agentManagerProvider.getActiveSessionId() === sessionID), | ||
| os: showOSNotification, | ||
| show: async (sessionID, directory) => { |
There was a problem hiding this comment.
Can we either handle Agent Manager navigation here or explicitly track it in a follow-up GitHub issue linked from this PR? When a notification comes from an Agent Manager session, clicking Show should probably focus Agent Manager, select the correct worktree and session tab, and scroll to the latest message. This currently always opens the session in the sidebar instead, which is unexpected when managing several agents. The sidebar fallback makes sense for sessions that are not owned by Agent Manager.
There was a problem hiding this comment.
Tracked as #13922, taking the follow-up option rather than doing it here.
I agree with the diagnosis: pulling an Agent Manager session into the sidebar is the wrong destination when you are managing several agents, and the sidebar fallback is only right for sessions Agent Manager does not own.
The reason I scoped it out is that only step one is cheap. Ownership detection already exists host-side (getSessionDirectories()), and focusPanel() handles the panel. But selecting the right project, worktree, and session tab needs a new host→webview reveal path — webview-ui/agent-manager/ has selectWorktree internally with no entry point driven from the extension host — plus handling for worktrees that are not loaded, multi-version siblings, sessions in a non-active project, and worktrees that have since been removed. Shipping a partial version that focuses Agent Manager but lands on the wrong session seemed worse than the current honest fallback.
The issue captures that breakdown, the existing hooks, and the edge cases. One thing I could not do: my account lacks permission to add items to the VS Code Extension board (AddProjectV2ItemById is denied), so #13922 needs to be added to the board by someone with org access.
|
@sylwester-liljegren I did give it a spin. It works great. I also gave it one more review, we should fix those remainders or scope them out then we can merge this. |
Resolve the attention service conflict by keeping both sides: upstream's goal-session suppression and this branch's session-scoped notify.
|
@marius-kilocode I have now addressed your reviews and tested (on Windows at least) to ensure nothing got broken. Feel free to go through the PR one (hopefully) final time before merging this into main. I believe that the Agent Manager navigation may need to be addressed in a separate PR to make it work. The current PR is already large as it is, so I think another PR with focus on that aforementioned navigation is the right approach forward. |
|
@sylwester-liljegren looks good, I approved. Only remainder is the red tests. |
Perfect, thanks a lot! I'm on the red test(s) right now. |
readAppID now returns undefined for a missing appRoot instead of throwing from path.join. The unit test only failed on CI because Linux took the win32 branch, where the test host reports no appRoot; Windows took the other branch and passed. The failure test now picks a non-win32 foreign platform so it exercises a real missing binary, and the win32 identity path gets its own explicit test. Add scrollBottomID and consumeScrollBottom to the Storybook session mock. It is cast to any, so the missing members typechecked fine but threw on mount once MessageList passed scrollBottomID to on(), which crashed every chat story in the visual regression suite. Added a source contract test so the mock cannot silently drift from the context again.
…feat/vscode-notifications
|
@marius-kilocode The tests have been fixed now :) |
Account for #14101 and #13951 after the original sync. Verify notification channels against #13771 and widget layouts against Kilo-Org/cloud#5620. Clarify partial change summaries from cloud#5810 and remove implementation-heavy prose.
Issue
Partially addresses #12618.
That issue asks for two things: per-state session tab icons and OS notifications for finished / needs-input sessions. This PR implements the notification half only; the tab icon work is not included, so the issue intentionally stays open.
Context
Attention events in the VS Code extension were sound-only (
kilo-code.new.attention.enabled). If you stepped away from the IDE, there was nothing to tell you a session finished, needed input, needed a permission decision, or died. The CLI/TUI already had a desktop-notification path; the extension did not.This adds two independent, opt-in notification channels on top of the existing attention event stream, plus a terminal-error event that previously produced no user-visible signal.
Implementation
AttentionServicealready consumed the SSE events (question.asked,permission.asked,session.turn.close,session.error) and owned the dedup/auto-approve logic, so all of this hangs off that single place rather than a new event pipeline.notify()now resolves display text, decides which channels fire, and enriches the alert with workspace + session context.Points worth reviewer attention:
services/attention/os.tsbuilds the platform command — WinRT via PowerShell on Windows,osascripton macOS,notify-sendon Linux — and everything else (queue cap, serialization, 10s timeout, failure logging) is shared. Unsupported platforms return no command and the setting is hidden. Values are escaped per platform: XML for the Windows toast payload, backslash/quote escaping for AppleScript, and Pango escaping for the Linux body, where an unescaped<makes some daemons reject the notification outright.-EncodedCommanddoes not help, since it encodes the already-composed source. There is a regression test that feeds a smart-quote payload and asserts it never reaches the script.product.jsonundervscode.env.appRootand cached for the session, so Cursor, Antigravity, and other forks are attributed correctly. The value is validated (non-empty, length-bounded, no control characters) and, if the file or field is missing or unusable, no native notification is sent at all instead of impersonating another editor — the workbench channel keeps working and the Test button reports the specific reason.Workspace/Sessionlabels, the test message, and both failure reasons now go through the existing extension-hostservices/i18nlayer, which follows thekilo-code.new.languagesetting and falls back to the VS Code display language. Keys were added for all 21 locales. The Kilo Code brand and the actual workspace and session names are never translated.session.erroralso fires for transient failures that a retry recovers from, so showing "task stopped due to an error" at that moment is often wrong. The notification is therefore held and only delivered when the root turn closes (parentID === undefined) without abusy/retryin between;MessageAbortedErroris never held, so manual interrupts stay silent. The sound keeps firing immediately, exactly as before — it is a local cue that something happened, and changing its timing would alter existing behavior that this PR has no business touching.KiloConnectionService.isVisible(), since every surface already registers the session it displays and clears that registration when hidden. That covers the sidebar, Kilo editor tabs, and Agent Manager through one existing mechanism instead of adding an accessor to each provider.selectSessiontakes ascrollToBottomoption that records a one-shot request, consumed byMessageListahead of the anchor-restore path. Being one-shot matters: switching back to that session later restores normally.notify-send). A Test button under the toggle sends a real notification through the same code path and reports the actual command result, so the failure is visible at configuration time rather than as silence hours later. It is shown only while the toggle is on, matching the sound row, and its result clears when the toggle is switched off.vscode://protocol activation. It was dropped: activation reliably reached VS Code but the round-trip did not land the user on the session, and it triggered a workspace-trust prompt. Native click-to-focus was then considered per-platform and also not pursued, to keep behavior uniform across Windows, macOS, and Linux rather than shipping an action that exists on one OS. The OS toast carries the identifying information instead, and in-IDE navigation is handled by the VS Code toast's Show action.KiloProvider.openSession()awaitswaitForReady()first, becausepostMessage()silently drops messages before the webview resolves.Workspace: … | Session: …suffix, matching the OS toast's field order.mainput this branch'stestOSNotificationcase alongside newly added upstream cases in the same switch, pushing that function to complexity 151 against the repo's limit of 150. Rather than raise the limit, the three notification-settings messages moved intohandleNotificationSettingsMessage, following the existinghandleMigrationMessagepattern of a pre-switch delegate that returns whether it handled the message.package.jsondeclaredkilo-code.new.attention.windowsNotificationswhile the runtime and webview read a different key, so the setting was already inert. Everything is now consistentlykilo-code.new.attention.OSNotifications, and the module/identifiers dropped their Windows-specific names now that the implementation is cross-platform.Screenshots / Video
Visual change: new toggles on the Kilo Settings → Notifications page, plus the notifications themselves.
Workspace: …andSession: …on their own rows; https://github.com/user-attachments/assets/ada8775b-2eac-4109-90d4-f5d1bdbf1ed6 VS Code toast with the same context and a "Show" action.How to Test
Manual/local verification
Executed by the agent, from
packages/kilo-vscode/:bun test tests/unit/attention.test.ts tests/unit/attention-os.test.ts— event→channel routing, dual-channel firing, session-aware suppression, deferred terminal errors, retry-clears-error, per-platform command construction and escaping, the smart-quote injection regression,product.jsonidentity parsing and validation, and the unsupported-platform pathbun test src/services/cli-backend/connection-service.test.ts— theisVisible()query used for suppressionbun test tests/unit/i18n-keys.test.ts— every new key exists in all 21 locales and every literal key used in code is definedbun test tests/unit/session-scroll-bottom.test.ts tests/unit/kilo-provider-open-session.test.ts— the scroll-to-latest wiring and theopenSessionreadiness barrierbun test tests/unit/agent-manager-i18n-split.test.ts— locale key parity across all 21 dictionaries after the translation additionsbun run lint,bun run typecheck,bun run compile,bun run format,bun run knipExecuted by a human (Windows 11, development extension):
Not yet verified on a real GUI host: macOS (
osascript) and Linux (notify-send). The commands and their escaping are unit-tested, and both paths degrade to a logged failure plus the portable VS Code channel, but a reviewer on either platform confirming an actual toast would be valuable.Reviewer test steps
Blocked checks and substitute verification
bun run check-kilocode-changecould not complete: the script is POSIX shell/grepand fails to parse under the Windows PowerShell runner used here. Substitute verification was an equivalent direct search forkilocode_changemarkers acrosspackages/kilo-vscode/andpackages/kilo-ui/, which found none introduced by this branch. These are Kilo-owned packages, where the markers are prohibited anyway.Checklist
Get in Touch