Descriptive agent notifications: workspace title + specific bodies - #7336
Closed
lawrencecchen wants to merge 22 commits into
Closed
lawrencecchen wants to merge 22 commits into
lawrencecchen wants to merge 22 commits into
Conversation
Banner title becomes the workspace name for agent notifications, with "<Agent> · <status>" as the subtitle, via a new agent-identity meta field (a=<agent>) on the notify wire format and a pure banner-composition helper. Claude idle nags reuse the saved/transcript last assistant message instead of "Claude is waiting for your input"; generic agent stops fall back to "Finished: <prompt>" instead of "session completed" (and skip JSON-blob assistant messages); feed permission/question/plan banners show the workspace plus the concrete command, file, or question; opencode gains a gated turn-complete banner fed by the plugin's session.idle event. Legacy 3-field and c=..;p=.. payloads parse and gate byte-identically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR threads agent identity and prompt context through notification parsing, delivery, storage, and banner rendering. It adds a new opencode stop-notification path, updates localized strings, and expands tests and project wiring for the new notification flows. ChangesAgent-tagged notification pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR replaces generic agent notification banners with workspace-aware, context-specific content: the OS banner title becomes the workspace name, subtitle becomes
Confidence Score: 5/5Safe to merge — the notification wire format is extended strictly backwards-compatibly, and all three delivery surfaces share the same scrubbed composition path. The change spans CLI hook payloads, socket meta parsing, the notification store, feed coordinator, and the opencode JS plugin, but each boundary is well-tested with regression and integration tests. The meta parser is strictly c=-anchored so legacy 3-field payloads are byte-identical. The hookModifiedText guard correctly preserves user-defined hook rewrites. Localization is complete. No new actor-isolation or blocking-runtime issues were introduced. Both main architectural concerns from prior rounds were addressed by the author. No files require special attention. Important Files Changed
Reviews (13): Last reviewed commit: "Merge origin/main into feat-descriptive-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+119
to
+125
| let detail = notificationBannerNonEmpty(object?["command"] as? String) | ||
| ?? notificationBannerFileBasename(object?["file_path"] as? String) | ||
| ?? notificationBannerFileBasename(object?["filePath"] as? String) | ||
| ?? notificationBannerFirstPattern(object?["patterns"]) | ||
| ?? tool | ||
|
|
||
| if let tool, let detail { |
Contributor
There was a problem hiding this comment.
When no command, file_path, or patterns field is found in the tool input JSON, detail falls back to tool (line 123). Both tool and detail are then non-nil and equal, so the two-argument format fires and produces the degenerate string "Allow Shell: Shell" — confirmed by permissionBodyFallsBackToToolNameForOpaqueInput which asserts that exact output. The single-argument "%@ needs approval" branch is unreachable for this case. The fix is to separate the specific-detail lookup from the tool variable so the two-arg form is only used when a meaningful distinguishing detail exists.
Suggested change
| let detail = notificationBannerNonEmpty(object?["command"] as? String) | |
| ?? notificationBannerFileBasename(object?["file_path"] as? String) | |
| ?? notificationBannerFileBasename(object?["filePath"] as? String) | |
| ?? notificationBannerFirstPattern(object?["patterns"]) | |
| ?? tool | |
| if let tool, let detail { | |
| let detail = notificationBannerNonEmpty(object?["command"] as? String) | |
| ?? notificationBannerFileBasename(object?["file_path"] as? String) | |
| ?? notificationBannerFileBasename(object?["filePath"] as? String) | |
| ?? notificationBannerFirstPattern(object?["patterns"]) | |
| if let tool, let detail { |
Contributor
Author
There was a problem hiding this comment.
Fixed in 6df8a54: detail no longer falls back to tool, so the two-arg "Allow %@: %@" form only fires with a specific detail (command / file basename / pattern); opaque input now yields "Shell needs approval". Test renamed and updated accordingly.
Comment on lines
+10
to
+17
| nonisolated func composeNotificationBannerContent( | ||
| title: String, | ||
| subtitle: String, | ||
| body: String, | ||
| agentId: String?, | ||
| workspaceTitle: String?, | ||
| appName: String | ||
| ) -> NotificationBannerContent { |
Contributor
There was a problem hiding this comment.
composeNotificationBannerContent, composeFeedNotificationContent, and notificationBannerSnippet are module-internal nonisolated funcs at file scope, called from three different production files (TerminalNotificationStore, FeedCoordinator, TerminalController). The rule requires top-level free functions used as API to be owned by a constructable, injectable type — a caseless enum or a static-only namespace type hits the same flag. Prefer static funcs on NotificationBannerContent itself, or a dedicated NotificationBannerComposer struct whose callers construct or inject it.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
There was a problem hiding this comment.
Fixed in 6df8a54: composeNotificationBannerContent, composeFeedNotificationContent, notificationBannerSnippet, and the new JSON-blob helpers are now static members of caseless enum NotificationBannerComposer; all call sites and tests updated.
Comment on lines
+26007
to
+26018
| return classified | ||
| } | ||
|
|
||
| private func isGenericClaudeNotificationBody(_ body: String) -> Bool { | ||
| let normalized = normalizedSingleLine(body) | ||
| let placeholders: Set<String> = [ | ||
| "Claude is waiting for your input", | ||
| "Claude needs your input", | ||
| "Claude needs your attention" | ||
| ] | ||
| return placeholders.contains(normalized) | ||
| } |
Contributor
There was a problem hiding this comment.
isGenericClaudeNotificationBody matches against three hardcoded English literals to decide whether to replace the notification body. If any of those strings are changed on the generation side — or if Claude's CLI begins emitting a localized variant — the replacement silently stops firing and users see the generic body again. The cmux-reliability-single-source-of-truth rule requires correctness-critical detection to use a structured signal, not a string/title heuristic. A notification_type value of e.g. "idle_nag" emitted by the Claude hook (or a shared constant matched byte-for-byte against the generator) would make the detection robust to copy changes.
Rule Used: Flag correctness-critical detection/identity deriv... (source)
Contributor
Author
There was a problem hiding this comment.
Keeping the literal set, now documented in 6df8a54: these strings are external copy emitted by the Claude Code CLI notification hook, which offers no structured idle-nag signal today. A match miss degrades gracefully to the same generic body users saw before this PR, so it is a cosmetic fallback rather than correctness-critical detection. The doc comment says to extend the set if Claude Code copy changes.
… length budget Pure code motion, no logic changes: CLI summary helpers to CLI/CMUXCLI+NotificationSummaryHelpers.swift, opencode turn-complete banner to Sources/TerminalController+AgentNotifications.swift, TerminalNotification model to Sources/TerminalNotification.swift, banner content plumbing to Sources/TerminalNotificationStore+BannerContent.swift, and this PR's new CLI tests to cmuxTests/DescriptiveAgentNotificationCLITests.swift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…banner composer namespace - readTranscriptSummary rejects JSON-shaped assistant messages so raw JSON never becomes a stop/idle banner body (cursor) - opencode turn-complete banner checks the untruncated assistantFinalMessage for JSON before snippeting via new NotificationBannerComposer.assistantMessageSnippetRejectingJSONBlob (cursor + judge) - permission body uses "Allow <tool>: <detail>" only when a specific detail exists; opaque input now yields "<tool> needs approval" instead of "Allow Shell: Shell" (greptile P1) - banner composition helpers move under caseless enum NotificationBannerComposer (greptile P2) - document why isGenericClaudeNotificationBody matches Claude Code copy literals (greptile P2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ations Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/TerminalNotificationStore.swift (1)
1636-1665: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale
workspaceTitleafter rebind to a new workspace.
tabIdis updated todestinationTabId, butworkspaceTitleis copied verbatim from the old notification (resolved forsourceTabId), never recomputed fordestinationTabId. Since the banner composer usesworkspaceTitleas the primary banner title for agent-tagged notifications, a rebound notification will display the wrong workspace name after this move.🐛 Proposed fix
return TerminalNotification( id: notification.id, tabId: destinationTabId, surfaceId: notification.surfaceId, panelId: notification.panelId, title: notification.title, subtitle: notification.subtitle, body: notification.body, agentId: notification.agentId, - workspaceTitle: notification.workspaceTitle, + workspaceTitle: resolvedWorkspaceTitle(forTabId: destinationTabId) ?? notification.workspaceTitle, createdAt: notification.createdAt, isRead: notification.isRead, paneFlash: notification.paneFlash, clickAction: notification.clickAction )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalNotificationStore.swift` around lines 1636 - 1665, The rebind logic in rebindSurfaceNotifications keeps the old workspaceTitle when moving a notification from sourceTabId to destinationTabId, so the banner can show the wrong workspace name. Update the TerminalNotification reconstruction to recompute workspaceTitle for the destination tab/surface using the same resolution path the composer uses, and ensure the moved notification reflects destinationTabId consistently while preserving the other fields.Sources/TerminalController.swift (1)
12452-12481: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a regression for legacy bodies ending in
|a=...
parseNotificationPayloadtreats any 4th segment starting witha=as metadata, so a legacy body likeLegacy Body|a=claudewill be stripped instead of preserved. Add a test alongside the existing|c=...legacy-body case, or only promote the 4th segment when the fullAgentNotificationMetagrammar matches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalController.swift` around lines 12452 - 12481, `parseNotificationPayload` is incorrectly treating legacy bodies that end with `|a=...` as metadata and stripping that text from the body. Update the parsing in `TerminalController.parseNotificationPayload(_:)` so the 4th segment is only promoted to `AgentNotificationMeta` when the full grammar parses, otherwise it stays in the body; also add a regression test next to the existing `|c=...` legacy-body case to cover `|a=...`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+NotificationSummaryHelpers.swift:
- Around line 4-21: Update claudeAssistantMessageFromHookPayload to take a
non-optional [String: Any] input with a default empty dictionary at the call
site, since the function’s nil case is equivalent to no matching keys. Keep the
existing lookup logic with firstString(in:keys:) over the main object and its
extra field, then preserve the normalizedSingleLine and
isJSONBlobAssistantMessage filtering before returning the final message.
- Around line 23-31: The JSON-blob predicate is duplicated between
isJSONBlobAssistantMessage in CMUXCLI+NotificationSummaryHelpers and the
matching logic in NotificationBannerComposition, so extract it into a shared
SwiftPM target and have both call the same helper. Create a single shared
predicate function in the common module, move the trimming/JSONSerialization
check there, and update both call sites to use that shared symbol so CLI and app
stay consistent.
In `@cmuxTests/DescriptiveAgentNotificationCLITests.swift`:
- Around line 215-291: The Unix-socket client logic duplicated in
sendSocketCommands, writeSocketLine, readSocketLine, and socketTestError should
be moved into shared test support rather than kept as another private copy.
Extract the AF_UNIX connect/write/read/error-handling behavior into a reusable
helper used by this test and the existing
TerminalNotificationQueueTests/TerminalNotificationSocketActionTests codepaths,
then have sendSocketCommands delegate to that shared helper.
In `@Sources/NotificationBannerComposition.swift`:
- Around line 123-127: The fallback in feedNotificationSourceDisplayName is
incorrectly capitalizing opaque feed sources, which changes their original text.
Update the logic so RestorableAgentKind(rawValue:) still uses displayName for
known kinds, but the notificationBannerNonEmpty(source) fallback returns the
source verbatim without capitalizing it; only keep the localized Agent default
for empty or unknown inputs.
In `@Sources/TerminalNotification.swift`:
- Around line 3-17: Mark TerminalNotification as nonisolated since it is a pure
value Sendable model and should not inherit MainActor isolation by default.
Update the struct declaration itself, keeping the Identifiable, Hashable, and
Sendable conformance intact, so future consumers can use TerminalNotification
off-main without unnecessary actor hops.
---
Outside diff comments:
In `@Sources/TerminalController.swift`:
- Around line 12452-12481: `parseNotificationPayload` is incorrectly treating
legacy bodies that end with `|a=...` as metadata and stripping that text from
the body. Update the parsing in
`TerminalController.parseNotificationPayload(_:)` so the 4th segment is only
promoted to `AgentNotificationMeta` when the full grammar parses, otherwise it
stays in the body; also add a regression test next to the existing `|c=...`
legacy-body case to cover `|a=...`.
In `@Sources/TerminalNotificationStore.swift`:
- Around line 1636-1665: The rebind logic in rebindSurfaceNotifications keeps
the old workspaceTitle when moving a notification from sourceTabId to
destinationTabId, so the banner can show the wrong workspace name. Update the
TerminalNotification reconstruction to recompute workspaceTitle for the
destination tab/surface using the same resolution path the composer uses, and
ensure the moved notification reflects destinationTabId consistently while
preserving the other fields.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06cf97ad-e37e-44d1-a90a-4a7d615caf0d
📒 Files selected for processing (20)
CLI/CMUXCLI+NotificationSummaryHelpers.swiftCLI/cmux.swiftResources/Localizable.xcstringsResources/opencode-plugin.jsSources/AgentNotificationGate.swiftSources/Feed/FeedCoordinator.swiftSources/NotificationBannerComposition.swiftSources/TerminalController+AgentNotifications.swiftSources/TerminalController.swiftSources/TerminalNotification.swiftSources/TerminalNotificationQueue.swiftSources/TerminalNotificationStore+BannerContent.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentNotificationGateTests.swiftcmuxTests/ClaudeBackgroundWorkNotifyTests.swiftcmuxTests/DescriptiveAgentNotificationCLITests.swiftcmuxTests/FeedNotificationContentTests.swiftcmuxTests/NotificationBannerCompositionTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
Comment on lines
+4
to
+21
| func claudeAssistantMessageFromHookPayload(_ object: [String: Any]?) -> String? { | ||
| guard let object else { return nil } | ||
| let keys = [ | ||
| "last_assistant_message", | ||
| "lastAssistantMessage", | ||
| "assistantPreamble", | ||
| "assistant_preamble", | ||
| "assistant_response", | ||
| "assistantResponse", | ||
| ] | ||
| let extra = (object["extra"] as? [String: Any]) ?? [:] | ||
| let message = firstString(in: object, keys: keys) | ||
| ?? firstString(in: extra, keys: keys) | ||
| guard let message else { return nil } | ||
| let normalized = normalizedSingleLine(message) | ||
| guard !isJSONBlobAssistantMessage(normalized) else { return nil } | ||
| return normalized.isEmpty ? nil : normalized | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer non-optional [String: Any] with a default empty value.
SwiftLint flags the [String: Any]? parameter. Since the function immediately returns nil for a nil input and would equally return nil for an empty dictionary (no keys match), the optional adds no semantic value here.
🧹 Proposed fix
- func claudeAssistantMessageFromHookPayload(_ object: [String: Any]?) -> String? {
- guard let object else { return nil }
+ func claudeAssistantMessageFromHookPayload(_ object: [String: Any] = [:]) -> String? {
let keys = [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func claudeAssistantMessageFromHookPayload(_ object: [String: Any]?) -> String? { | |
| guard let object else { return nil } | |
| let keys = [ | |
| "last_assistant_message", | |
| "lastAssistantMessage", | |
| "assistantPreamble", | |
| "assistant_preamble", | |
| "assistant_response", | |
| "assistantResponse", | |
| ] | |
| let extra = (object["extra"] as? [String: Any]) ?? [:] | |
| let message = firstString(in: object, keys: keys) | |
| ?? firstString(in: extra, keys: keys) | |
| guard let message else { return nil } | |
| let normalized = normalizedSingleLine(message) | |
| guard !isJSONBlobAssistantMessage(normalized) else { return nil } | |
| return normalized.isEmpty ? nil : normalized | |
| } | |
| func claudeAssistantMessageFromHookPayload(_ object: [String: Any] = [:]) -> String? { | |
| let keys = [ | |
| "last_assistant_message", | |
| "lastAssistantMessage", | |
| "assistantPreamble", | |
| "assistant_preamble", | |
| "assistant_response", | |
| "assistantResponse", | |
| ] | |
| let extra = (object["extra"] as? [String: Any]) ?? [:] | |
| let message = firstString(in: object, keys: keys) | |
| ?? firstString(in: extra, keys: keys) | |
| guard let message else { return nil } | |
| let normalized = normalizedSingleLine(message) | |
| guard !isJSONBlobAssistantMessage(normalized) else { return nil } | |
| return normalized.isEmpty ? nil : normalized | |
| } |
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 4-4: Prefer empty collection over optional collection
(discouraged_optional_collection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/CMUXCLI`+NotificationSummaryHelpers.swift around lines 4 - 21, Update
claudeAssistantMessageFromHookPayload to take a non-optional [String: Any] input
with a default empty dictionary at the call site, since the function’s nil case
is equivalent to no matching keys. Keep the existing lookup logic with
firstString(in:keys:) over the main object and its extra field, then preserve
the normalizedSingleLine and isJSONBlobAssistantMessage filtering before
returning the final message.
Source: Linters/SAST tools
Comment on lines
+23
to
+31
| func isJSONBlobAssistantMessage(_ value: String) -> Bool { | ||
| let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard trimmed.hasPrefix("{") || trimmed.hasPrefix("["), | ||
| let data = trimmed.data(using: .utf8), | ||
| (try? JSONSerialization.jsonObject(with: data, options: [.fragmentsAllowed])) != nil else { | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether NotificationBannerComposer defines its own copy of isJSONBlobAssistantMessage
fd -e swift . Sources | xargs rg -n 'func isJSONBlobAssistantMessage' -A5Repository: manaflow-ai/cmux
Length of output: 848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the surrounding implementations and call sites.
for f in CLI/CMUXCLI+NotificationSummaryHelpers.swift Sources/NotificationBannerComposition.swift; do
echo "===== $f ====="
nl -ba "$f" | sed -n '1,220p'
done
echo "===== call sites ====="
rg -n "isJSONBlobAssistantMessage" CLI Sources cmuxTests -A2 -B2
echo "===== package targets ====="
fd -a -t f Package.swift Packages . | sed -n '1,120p'Repository: manaflow-ai/cmux
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in CLI/CMUXCLI+NotificationSummaryHelpers.swift Sources/NotificationBannerComposition.swift; do
echo "===== $f ====="
sed -n '1,220p' "$f" | cat -n
done
echo "===== call sites ====="
rg -n "isJSONBlobAssistantMessage" CLI Sources cmuxTests -A2 -B2
echo "===== package targets ====="
find . -name Package.swift -o -path "./Packages/*/Package.swift" | sortRepository: manaflow-ai/cmux
Length of output: 24373
Extract the JSON-blob predicate into shared code.
isJSONBlobAssistantMessage is duplicated in both CLI/CMUXCLI+NotificationSummaryHelpers.swift and Sources/NotificationBannerComposition.swift, so the CLI and app can drift on the same input. Move it into a shared SwiftPM target and call that from both sides.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/CMUXCLI`+NotificationSummaryHelpers.swift around lines 23 - 31, The
JSON-blob predicate is duplicated between isJSONBlobAssistantMessage in
CMUXCLI+NotificationSummaryHelpers and the matching logic in
NotificationBannerComposition, so extract it into a shared SwiftPM target and
have both call the same helper. Create a single shared predicate function in the
common module, move the trimming/JSONSerialization check there, and update both
call sites to use that shared symbol so CLI and app stay consistent.
Source: Path instructions
Comment on lines
+215
to
+291
| private nonisolated func sendSocketCommands(_ commands: [String], to socketPath: String) throws -> [String] { | ||
| let fd = Darwin.socket(AF_UNIX, SOCK_STREAM, 0) | ||
| guard fd >= 0 else { throw socketTestError("socket(AF_UNIX)") } | ||
| defer { Darwin.close(fd) } | ||
|
|
||
| var addr = sockaddr_un() | ||
| addr.sun_family = sa_family_t(AF_UNIX) | ||
| let bytes = Array(socketPath.utf8) | ||
| let maxPathLen = MemoryLayout.size(ofValue: addr.sun_path) | ||
| guard bytes.count < maxPathLen else { | ||
| throw NSError(domain: NSPOSIXErrorDomain, code: Int(ENAMETOOLONG)) | ||
| } | ||
|
|
||
| withUnsafeMutablePointer(to: &addr.sun_path) { pathPtr in | ||
| let cPath = UnsafeMutableRawPointer(pathPtr).assumingMemoryBound(to: CChar.self) | ||
| cPath.initialize(repeating: 0, count: maxPathLen) | ||
| for (index, byte) in bytes.enumerated() { | ||
| cPath[index] = CChar(bitPattern: byte) | ||
| } | ||
| } | ||
|
|
||
| let addrLen = socklen_t(MemoryLayout<sa_family_t>.size + bytes.count + 1) | ||
| let connectResult = withUnsafePointer(to: &addr) { ptr -> Int32 in | ||
| ptr.withMemoryRebound(to: sockaddr.self, capacity: 1) { sockaddrPtr in | ||
| Darwin.connect(fd, sockaddrPtr, addrLen) | ||
| } | ||
| } | ||
| guard connectResult == 0 else { throw socketTestError("connect(\(socketPath))") } | ||
|
|
||
| var responses: [String] = [] | ||
| for command in commands { | ||
| try writeSocketLine(command, to: fd) | ||
| responses.append(try readSocketLine(from: fd)) | ||
| } | ||
| return responses | ||
| } | ||
|
|
||
| private func codexLaunchEnvironment(context: ClaudeHookContext, sessionId _: String) -> [String: String] { | ||
| [ | ||
| "CMUX_AGENT_LAUNCH_KIND": "codex", | ||
| "CMUX_AGENT_LAUNCH_EXECUTABLE": "/usr/local/bin/codex", | ||
| "CMUX_AGENT_LAUNCH_CWD": context.root.path, | ||
| "CMUX_AGENT_LAUNCH_ARGV_B64": base64NULSeparated(["/usr/local/bin/codex", "--model", "gpt-5.4"]), | ||
| ] | ||
| } | ||
|
|
||
| private nonisolated func writeSocketLine(_ command: String, to fd: Int32) throws { | ||
| let payload = Array((command + "\n").utf8) | ||
| var offset = 0 | ||
| while offset < payload.count { | ||
| let wrote = payload.withUnsafeBytes { raw in | ||
| Darwin.write(fd, raw.baseAddress!.advanced(by: offset), payload.count - offset) | ||
| } | ||
| guard wrote >= 0 else { throw socketTestError("write(\(command))") } | ||
| offset += wrote | ||
| } | ||
| } | ||
|
|
||
| private nonisolated func readSocketLine(from fd: Int32) throws -> String { | ||
| var data = Data() | ||
| var byte: UInt8 = 0 | ||
| while true { | ||
| let count = Darwin.read(fd, &byte, 1) | ||
| guard count > 0 else { throw socketTestError("read") } | ||
| if byte == 0x0A { break } | ||
| data.append(byte) | ||
| } | ||
| return String(data: data, encoding: .utf8) ?? "" | ||
| } | ||
|
|
||
| private nonisolated func socketTestError(_ operation: String) -> NSError { | ||
| NSError( | ||
| domain: NSPOSIXErrorDomain, | ||
| code: Int(errno), | ||
| userInfo: [NSLocalizedDescriptionKey: "\(operation) failed: \(String(cString: strerror(errno)))"] | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing raw-socket write/connect helpers in cmuxTests to avoid duplication
rg -n --type=swift -C2 'AF_UNIX|sockaddr_un' cmuxTests | rg -v 'DescriptiveAgentNotificationCLITests'Repository: manaflow-ai/cmux
Length of output: 48092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Current file helpers ==\n'
sed -n '215,291p' cmuxTests/DescriptiveAgentNotificationCLITests.swift
printf '\n== Shared socket support ==\n'
sed -n '1,180p' cmuxTests/CLIMockSocketServerSupport.swift
printf '\n---\n'
sed -n '1,140p' cmuxTests/CLINotifyProcessTestSupport.swift
printf '\n== Closest existing client helpers ==\n'
sed -n '530,620p' cmuxTests/TerminalNotificationQueueTests.swift
printf '\n---\n'
sed -n '430,470p' cmuxTests/TerminalNotificationSocketActionTests.swiftRepository: manaflow-ai/cmux
Length of output: 20235
Extract the shared Unix-socket client helper
sendSocketCommands / writeSocketLine / readSocketLine / socketTestError repeat the same AF_UNIX connect/write/read/error-handling pattern already used in TerminalNotificationQueueTests.swift and TerminalNotificationSocketActionTests.swift; move that into shared test support instead of keeping another private copy here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/DescriptiveAgentNotificationCLITests.swift` around lines 215 - 291,
The Unix-socket client logic duplicated in sendSocketCommands, writeSocketLine,
readSocketLine, and socketTestError should be moved into shared test support
rather than kept as another private copy. Extract the AF_UNIX
connect/write/read/error-handling behavior into a reusable helper used by this
test and the existing
TerminalNotificationQueueTests/TerminalNotificationSocketActionTests codepaths,
then have sendSocketCommands delegate to that shared helper.
Comment on lines
+123
to
+127
| private nonisolated func feedNotificationSourceDisplayName(_ source: String) -> String { | ||
| RestorableAgentKind(rawValue: source)?.displayName | ||
| ?? notificationBannerNonEmpty(source)?.capitalized | ||
| ?? String(localized: "feed.notification.source.agent", defaultValue: "Agent") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm RestorableAgentKind has no "custom" case and verify String.capitalized behavior assumption
rg -n "enum RestorableAgentKind" -A 30 --type=swift
rg -n "case custom" --type=swift -g '*RestorableAgentKind*'Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant symbols and inspect the helper/tests.
rg -n "enum RestorableAgentKind|struct RestorableAgentKind|typealias RestorableAgentKind|feedNotificationSourceDisplayName|permissionBodyUsesSingleToolApprovalForOpaqueInput" Sources cmuxTests -S
# Show the surrounding implementation in the reviewed file.
FILE="Sources/NotificationBannerComposition.swift"
if [ -f "$FILE" ]; then
nl -ba "$FILE" | sed -n '100,150p'
fi
# Show the enum definition if present.
rg -n "RestorableAgentKind" Sources cmuxTests -S -A 20 -B 5Repository: manaflow-ai/cmux
Length of output: 671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused search for the exact test expectations and any custom-source cases.
rg -n '"custom permission"|"custom · permission"|permissionBodyUsesSingleToolApprovalForOpaqueInput|source: "custom"|rawValue: source|capitalized' cmuxTests Sources -S -A 3 -B 3Repository: manaflow-ai/cmux
Length of output: 6036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the definition by nearby names and inspect any helper that normalizes source strings.
rg -n "displayName.*source|source.*displayName|notificationBannerNonEmpty|feedNotificationSourceDisplayName|AgentKind|Restorable" Sources cmuxTests -S -A 4 -B 4
# If the reviewed file is present, dump the full helper region.
FILE="Sources/NotificationBannerComposition.swift"
[ -f "$FILE" ] && nl -ba "$FILE" | sed -n '1,220p'Repository: manaflow-ai/cmux
Length of output: 50375
Keep opaque feed sources verbatim. notificationBannerNonEmpty(source)?.capitalized turns custom into Custom, but opaque-input banners expect the source text to stay lowercase; known agent kinds already go through displayName.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/NotificationBannerComposition.swift` around lines 123 - 127, The
fallback in feedNotificationSourceDisplayName is incorrectly capitalizing opaque
feed sources, which changes their original text. Update the logic so
RestorableAgentKind(rawValue:) still uses displayName for known kinds, but the
notificationBannerNonEmpty(source) fallback returns the source verbatim without
capitalizing it; only keep the localized Agent default for empty or unknown
inputs.
Contributor
Author
There was a problem hiding this comment.
Not a live bug: RestorableAgentKind(rawValue: "custom") resolves through the default branch to .custom("custom") (it's a valid vault-agent id), and .custom's displayName returns the id verbatim — lowercase, exactly what the tests assert. The .capitalized fallback is reachable only for sources that fail CmuxVaultAgentRegistration.isValidID (arbitrary non-id strings), where capitalizing is an intentional cosmetic fallback for a display label. Keeping as is.
— Claude Code
There was a problem hiding this comment.
@lawrencecchen Thanks for the clarification — that makes sense. Since RestorableAgentKind(rawValue:) already routes valid vault-agent ids (like "custom") through .custom(id) with a verbatim lowercase displayName, the .capitalized fallback only ever applies to arbitrary non-id strings where a cosmetic capitalized label is the intended behavior. Withdrawing this comment.
🧠 Learnings used
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 2545
File: Sources/GhosttyTerminalView.swift:3891-3903
Timestamp: 2026-04-02T21:37:21.463Z
Learning: In Swift source files like Sources/GhosttyTerminalView.swift, avoid logging raw startup commands or initialInput even in DEBUG (to prevent leaking sensitive paths/tokens and multiline content). If you need to diagnose startup/input, log only non-sensitive metadata such as (1) presence flags (e.g., hasStartupCommand/hasInitialInput), (2) byte counts, and (3) the relevant surface id (so issues can be correlated without exposing the underlying strings).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:6439-6444
Timestamp: 2026-04-03T03:35:54.082Z
Learning: In this repo’s keyboard shortcut implementation, ensure `KeyboardShortcutSettings.setShortcut(...)` does nothing (no-op) when `KeyboardShortcutSettings.isManagedBySettingsFile(action)` returns `true` (i.e., the shortcut is managed via `settings.json`). This prevents writing back overrides into `UserDefaults` and keeps `settings.json` as the source of truth.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2964
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-17T21:35:25.493Z
Learning: In this repo’s shell session resume flow, always build resume commands using the cwd guard helper exposed by SessionEntry (e.g., resumeCommandWithCwd). The helper should produce a command of the form `cd <shell-quoted cwd> && <resumeCommand>`. Update all call sites that generate “resume” commands (e.g., clipboard actions, drag-drop terminal, and in-app resume) to use this helper so that rc files and newly spawned shells cannot start outside the intended directory. Avoid constructing resume commands directly without the guarded `cd` + shell-quoting + `&&` composition.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2978
File: Sources/Workspace.swift:0-0
Timestamp: 2026-04-22T08:13:36.833Z
Learning: In manaflow-ai/cmux, note that Sources/RestorableAgentSession.swift’s `SessionRestorableAgentSnapshot.resumeCommand` already includes a cwd guard when `workingDirectory` is present (it returns a string like `cd <shell-quoted cwd> && <resumeCommand>`). At call sites (e.g., `Workspace.createPanel(...)`), pass `.resumeCommand` through directly and do not prepend another `cd`/cwd guard or wrap it with an additional `cd <...> &&`—otherwise the working directory may be applied twice or incorrectly.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2978
File: Sources/Workspace.swift:0-0
Timestamp: 2026-04-22T08:14:04.901Z
Learning: In the cmux Swift sources, when restoring a restorable agent session, call sites should pass `resumeCommand` directly (ensuring it has the expected trailing newline if required) to `sendInputWhenReady`. Do not wrap `resumeCommand` with an additional `cd '<cwd>' && ...` guard, because `SessionRestorableAgentSnapshot.resumeCommand` already returns a `cwd`-guarded command; adding another guard can result in `double-cd`. Apply this especially along restore paths (e.g., `Sources/Workspace.swift` restore logic) whenever using `resumeCommand`.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3084
File: Sources/AppDelegate.swift:4958-4975
Timestamp: 2026-04-22T11:37:36.238Z
Learning: In Swift code, when re-registering or updating an existing window/session context (e.g., in AppDelegate.registerMainWindow or similar flows), only update an existing *cmuxConfigStore* (or equivalent per-window configuration store) if the incoming configuration/store value is non-nil. Do not overwrite an existing per-window store with nil, so the previous per-window configuration is preserved across re-registration paths.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3128
File: Sources/Panels/BrowserPanelView.swift:4271-4290
Timestamp: 2026-04-23T11:23:49.934Z
Learning: In OmnibarSuggestionsView (and other omnibar-related debug/telemetry logging), never log raw omnibar suggestion content (e.g., URLs, titles, queries). Instead, log only non-sensitive metadata such as suggestion kind/category and the byte length of the text (e.g., "browser.suggestionClick kind=<kind> textBytes=<len>"). Apply this rule consistently to all omnibar-related debug logs to avoid leaking user/search data.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3046
File: Sources/TerminalController.swift:2096-2136
Timestamp: 2026-04-24T22:17:52.550Z
Learning: In Swift request/JSON handlers (e.g., v2 JSON-socket handlers) in Sources, prefer using the v2 helpers for numeric parsing—use `v2Int(params, "<key>")` for normal integer inputs and `v2StrictInt(...)` when strictness is required—rather than casting with `as? Int`. JSONSerialization may yield NSNumber/Double for numeric fields, so v2Int/v2StrictInt ensures correct extraction and type handling. If parsing is used for safety (e.g., timeouts), clamp/validate the parsed value as appropriate (as in `vm.exec` parsing `timeout_ms` and enforcing `>= 1`).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3166
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-27T06:58:21.434Z
Learning: In the Swift UI code (e.g., Sources/ContentView.swift), when `preferLiquidGlass`/`materialPolicy.preferLiquidGlass` is enabled for sidebar Liquid Glass, only omit the `Color` tint overlay if `NSGlassEffectView` (native Liquid Glass) is actually available. Compute `usingNativeLiquidGlass = materialPolicy.preferLiquidGlass && SidebarVisualEffectBackground.liquidGlassAvailable`, and when `usingNativeLiquidGlass` is false keep the overlay so the non-native `NSVisualEffectView` fallback still receives the configured tint.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3182
File: Sources/ContentView.swift:10850-10875
Timestamp: 2026-04-27T10:11:36.830Z
Learning: When computing NSTextView content height for NSTextView-based editors (e.g., a method like naturalDocumentHeight(...)), account for trailing newline layout. Specifically, include `layoutManager.extraLineFragmentRect.height` in the measured height only when `extraLineFragmentTextContainer == textContainer`. If you don’t, the caret on the final blank line can be clipped. Apply this rule to future NSTextView-based editors in this repo.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3139
File: Sources/Panels/FilePreviewPanel.swift:534-537
Timestamp: 2026-04-28T05:45:32.192Z
Learning: When implementing workspace/panel teardown or close-confirmation logic in Sources (e.g., close/collapse/workspace-close flows), rely on the shared dirty-state gate driven by the panel’s `isDirty` property rather than adding panel-specific teardown special-casing. Ensure each panel (including `FilePreviewPanel`) exposes and keeps its `isDirty` state up to date (e.g., via `Published private(set) var isDirty` and any subscriptions/synchronization logic), so the generic `panel.isDirty` check correctly covers all panel types during teardown.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3218
File: Sources/AppDelegate.swift:4800-4802
Timestamp: 2026-04-28T11:43:53.356Z
Learning: In Swift code that selects or activates the next main-window context (e.g., iterating window-context collections), avoid iterating `mainWindowContexts.values` directly while calling `resolvedWindow(for:)`. Since `resolvedWindow(for:)` may reindex/mutate `mainWindowContexts`, this can cause mutation-during-enumeration issues. Instead, snapshot first with `Array(mainWindowContexts.values)`, then resolve/reindex against that snapshot, and only then call `activateMainWindowContext(_:)` using the resolved result.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3247
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-29T01:08:39.652Z
Learning: Maintain the behavior contract for “Copy Workspace ID(s)”: when triggered from the sidebar context menu (e.g., in Sources/ContentView.swift TabItemView), the command must copy plain UUIDs (IDs-only), not references/refs. For command palette identifier-copy commands where refs are required, ensure the implementation explicitly passes includeRefs: true. This preserves backward compatibility for scripts expecting UUID-only output while allowing the palette to return richer payloads when needed.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3256
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-04-29T01:20:59.683Z
Learning: In this repo’s Swift implementation, keep browser-creation/open behavior consistent when `BrowserAvailabilitySettings` is disabled. For both V1 and V2, any command that includes a URL when creating/opening a browser (e.g., `open_browser` with a URL, `v2 surface.create` with `type=browser` and `url`, `browser.tab.new` with `url`, `v2 browser.open_split` with `url`) must open the URL externally using `NSWorkspace.shared.open(...)` and return appropriate success metadata. Only URL-less/blank browser creations should fail with the `browser_disabled` error.
Learnt from: pgbezerra
Repo: manaflow-ai/cmux PR: 3307
File: Sources/cmuxApp.swift:6413-6417
Timestamp: 2026-04-30T11:55:31.575Z
Learning: In this repo (manaflow-ai/cmux), when adding a new Settings section in SwiftUI (e.g., in Sources/cmuxApp.swift or related Views), don’t wire navigation/search with a raw anchor string alone. Instead: (1) create a corresponding SettingsNavigationTarget enum case (e.g., .workspaces); (2) provide the localized title, symbol, search text, and aliases for that case; (3) add/update the matching entry in SettingsSearchIndex so the sidebar/search can navigate to it; and (4) apply .settingsSearchAnchor(SettingsSearchIndex.sectionID(for: <target>)) to the section header. This prevents broken jump-to behavior by ensuring the navigation anchor and the search index stay consistent.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3430
File: Sources/ContentView.swift:0-0
Timestamp: 2026-05-02T06:07:12.997Z
Learning: In this repo’s SwiftUI views, any view placed under LazyVStack, LazyHStack, List, or ForEach must not capture or hold ObservableObject store instances (e.g., a TabManager). Instead, pass immutable value snapshots (e.g., currentSelectedTabId, sidebarIndexForTabId) plus action closures (e.g., moveToExistingWorkspace, moveToNewWorkspace). Prefer refactoring child view APIs to accept the needed values/closures rather than an ObservableObject reference (e.g., SidebarBonsplitTabWorkspaceDropOverlay should take closures instead of a TabManager).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3480
File: Sources/GhosttyTerminalView.swift:0-0
Timestamp: 2026-05-04T05:31:52.905Z
Learning: In this repo’s Swift sources, keep “surface-scoped” Ghostty config reloads strictly scoped to the target surface. Specifically, GhosttyApp.reloadSurfaceConfiguration(_:soft:source:) should update only the surface via ghostty_surface_update_config and invalidate GhosttyConfig’s load cache, but it must not replace or promote the per-surface config into GhosttyApp’s app-level config/cache (e.g., it must not overwrite GhosttyApp.config or modify app-level cached state). App-level helpers like scrollbarVisibility() and focusFollowsMouseEnabled() must continue to read GhosttyApp.config until a full app reload path is taken.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3502
File: Sources/ContentView.swift:0-0
Timestamp: 2026-05-04T11:09:27.707Z
Learning: This repo’s CI enforces a Swift file-length budget for large view files (e.g., Sources/ContentView.swift). When adding helper views/small components, avoid bloating the existing file: extract the subview into a dedicated Swift file under Sources (e.g., Sources/SidebarScrim.swift) and keep it under the CI length threshold. Use access control deliberately: if a extracted view/type must be referenced from other files, do not mark it `private` (use `internal` by omitting `private`); only use `private` for declarations that are truly local to the same file.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3471
File: Sources/CmuxTopSnapshotScopeCache.swift:1-54
Timestamp: 2026-05-05T02:19:22.055Z
Learning: In this repo (manafow-ai/cmux), `CmuxTopProcessSnapshot` and `CmuxTopProcessScope` are app-internal types defined under `Sources/`.
When reviewing Swift files under `Sources/`, do not recommend extracting/creating a separate SwiftPM package for extensions over these types (e.g., `CmuxTopSnapshotScopeCache.swift`). Only suggest SwiftPM extraction if the repo introduces a dedicated process-inspection package (e.g., a new `MuxCore`/`process-inspection`-style package) rather than trying to extract app-internal extensions in isolation.
Learnt from: psh4607
Repo: manaflow-ai/cmux PR: 3559
File: Sources/GhosttyTerminalView.swift:3824-3846
Timestamp: 2026-05-05T16:41:00.198Z
Learning: In cmux (Sources), the method `AppDelegate.shared?.workspaceContainingPanel(panelId:preferredWorkspaceId:)` returns an optional *named tuple* that includes a `workspace` field (not an optional `Workspace` directly). When using it, access the workspace through `.workspace`, e.g. `AppDelegate.shared?.workspaceContainingPanel(...)?.workspace ?? fallback`, and avoid treating the method’s return value as `Workspace?`.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3582
File: Sources/SessionIndexModels.swift:261-276
Timestamp: 2026-05-06T07:12:22.050Z
Learning: In manaflow-ai/cmux, SessionEntry.resumeWorkingDirectory is the single source of truth for the working directory used in registered-agent resume commands. It must be passed as the 'workingDirectory' into AgentResumeCommandBuilder.resumeShellCommand, and resumeCommandWithCwd should consult it before prepending a 'cd <dir>' guard. Registrations with cwd: .ignore set resumeWorkingDirectory to nil, suppressing both the cwd guard in the resume command and the terminal working directory at placement time (ContentView resume, Workspace drag/drop). Apply this guidance to all Swift files under Sources (and related modules) to preserve consistent behavior.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3629
File: Sources/TerminalController.swift:8094-8105
Timestamp: 2026-05-06T10:18:57.314Z
Learning: Redact sensitive fields from all workstream event payloads before any persistence or streaming in the manaflow-ai/cmux project. Specifically redact tool_input, context, and extra in every encoded payload prior to emitting via events.stream or writing to ~/.cmuxterm/events.jsonl. Implement a centralized sanitizer in the encoding/transport stack (e.g., FeedSocketEncoding) and ensure TerminalController.v2FeedPush, CmuxEventBus.publishWorkstreamEvent, and all related encoding paths apply redaction. Add tests that verify sensitive strings do not appear in encoded event payloads. Use an allowlist approach for permitted fields and replace redacted fields with a stable placeholder (e.g., [REDACTED]). Ensure no redacted values are logged or persisted, and that CI checks enforce the rule.
Learnt from: ryouka0731
Repo: manaflow-ai/cmux PR: 3415
File: Sources/Worktree/WorktreeManager.swift:1-10
Timestamp: 2026-05-06T19:24:46.399Z
Learning: Do not recommend extracting Foundation-only Swift files into a new SwiftPM package target in this repo. The established pattern is to keep app-internal helper modules under Sources/ unless the repository introduces a dedicated package (e.g., CMUXAuthCore, CMUXDebugLog, CMUXWorkstream) with a clear architectural rationale. Speculative SwiftPM extractions for individual Foundation-only modules expand PR scope without documented justification. Apply this guideline when reviewing changes to Swift files under Sources/; require a documented package architecture justification before considering packaging changes.
Learnt from: say8425
Repo: manaflow-ai/cmux PR: 3680
File: Sources/cmuxApp.swift:0-0
Timestamp: 2026-05-07T05:01:28.476Z
Learning: In the cmux Swift app, the right sidebar’s visibility should come from `RightSidebarVisibilityTracker.shared` (an `ObservableObject`) and be bound to the active main window’s `FileExplorerState.isVisible`. Update/bind the UI state on window focus changes: `AppDelegate.activateMainWindowContext(...)` must call `bind(to:)` when the window changes so menu/command-palette titles reflect the focused window. Do not mirror these labels via a global `AppStorage("fileExplorer.isVisible")` (or other global storage) used as a label/visibility source; it should reflect the active window state instead.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 3626
File: Sources/TabManager.swift:1817-1818
Timestamp: 2026-05-07T08:37:03.967Z
Learning: In this Swift repo (manafow-ai/cmux), when cleaning up stale agent process entries, use `Workspace.clearAgentPID(key:panelId:)` as the single cleanup path. Do not directly mutate `Workspace.statusEntries` or `Workspace.agentPIDs` from outside the dedicated helpers; for example, `TabManager.sweepStaleAgentPIDs` should only call `clearAgentPID` rather than performing its own mutations. This ensures panel-scoped side effects and port/refresh logic run consistently.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 3626
File: Sources/TerminalController.swift:15973-15983
Timestamp: 2026-05-07T09:14:20.991Z
Learning: In the manaflow-ai/cmux Swift codebase, ensure option/argument parsing for panel/surface targeting handles explicitly provided but empty values as errors. For example, in Sources/TerminalController.swift, agentTrackingPanelTarget(options:) must treat an explicit empty value for --panel/--surface (e.g., `--panel ""`) as a parse error such as “Missing value for --panel”, rather than falling back to an unscoped/default target. This prevents unintended workspace-wide mutations when a scoped UUID/panel target is required. Apply the same validation rule to any related socket option parsers that support --panel/--surface targeting.
Learnt from: psh4607
Repo: manaflow-ai/cmux PR: 3696
File: cmuxTests/ShortcutAndCommandPaletteTests.swift:1716-1771
Timestamp: 2026-05-07T10:56:50.266Z
Learning: In the manaflow-ai/cmux repo, SwiftLint does not enforce a `required_deinit` rule (no project `.swiftlint.yml` in cmux itself, no `required_deinit` in `.github/review-bot-rules/`, and no SwiftLint CI run in `.github/workflows/`). During code reviews, do not raise findings for missing `deinit` on `XCTestCase` subclasses or other Swift classes based on a `required_deinit` rule.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3784
File: README.md:161-161
Timestamp: 2026-05-09T04:48:35.413Z
Learning: In the cmux project, the right-sidebar keyboard shortcut labels were intentionally swapped (per PR `#3784`). Reviewers should NOT flag the ⌘⇧E (Cmd+Shift+E) label as “Open file explorer.” Use these mappings consistently: ⌘⇧E → `focusRightSidebar` with the user-facing label “Toggle right sidebar focus”; ⌘⌥B (Cmd+Option+B) → `toggleFileExplorer` with the user-facing label “Open file explorer.”
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 4353
File: Sources/AppIconDockTilePlugin.swift:91-97
Timestamp: 2026-05-19T07:50:03.218Z
Learning: When working with an `NSDockTilePlugIn`/dock tile plugin, remember it runs in the Dock process (`com.apple.dock`), not the main app process. Do not try to read app launch flags via `ProcessInfo.processInfo.arguments` inside the dock plugin—those arguments aren’t visible to the Dock process. For cross-process state, use a shared `UserDefaults` suite identified by the app’s bundle identifier (e.g., `UserDefaults(suiteName: appBundleIdentifier)`), since it’s accessible from both the Dock tile plugin and the main app. If you need deterministic behavior in smoke/UI tests, seed any relevant `UserDefaults` keys (e.g., via `defaults write "$BUNDLE_ID" ...`) before calling `open` so the dock plugin observes the flag before the app starts, and remove the key during cleanup after the smoke test.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 4626
File: Sources/Workspace.swift:0-0
Timestamp: 2026-05-23T03:33:51.872Z
Learning: In this repo’s Swift code, the close-tab confirmation trigger must be explicit and wired through all close flows.
- For close operations, ensure the close-tab confirmation policy accepts and uses an explicit `CloseTabConfirmationTrigger` parameter.
- Specifically, `Workspace.markExplicitClose(surfaceId:trigger:)` and `TabManager.closeWorkspaceFromCloseTabGesture(_:trigger:)` must NOT rely on default parameter values for the trigger; each callsite must pass an appropriate trigger.
- At each callsite, pass the correct trigger: `.tabCloseButton` for the X-button, `.shortcut` for Cmd+W, and a purpose-specific value for programmatic/API-driven close paths.
- When touching the related trigger enums, keep them `nonisolated` and `Sendable` as required by the concurrency model.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 4855
File: Sources/cmuxApp.swift:6302-6314
Timestamp: 2026-05-27T08:47:03.390Z
Learning: In the manaflow-ai/cmux settings UI, keep `app.menuBarOnly` / “Menu Bar Only” under the **App** settings bucket because it controls app presence behavior (Dock icon and Cmd+Tab visibility), not notification behavior. Ensure `SettingsNavigation` maps this setting to the `.app` section with the `menu-bar-only` anchor, and that command-palette settings descriptors report it as belonging to the **App** section. When reviewing, do not suggest moving it to **Notifications** solely because it sits near menu bar/Dock-related notification settings.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 6841
File: Sources/App/VSCodeServeWebSupport.swift:856-909
Timestamp: 2026-06-26T10:45:17.360Z
Learning: In manaflow-ai/cmux, when reviewing Swift production code for blocking synchronization (per .github/review-bot-rules/swift-blocking-runtime.md), do not flag issues if the PR only relocates existing blocking code verbatim (a move/rename/re-file move) without introducing new blocking synchronization and without worsening the blocking pattern (e.g., no additional call sites, no increased frequency/usage, and the moved code logic remains unchanged). For example, moving ServeWebOutputCollector from one Swift source to another should be treated as allowed under this exception when the diff contains only a verbatim relocation.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 6905
File: Sources/OfflineNotesStore.swift:0-0
Timestamp: 2026-06-26T12:31:31.190Z
Learning: When reviewing Swift code in this repo’s `Sources/`, do not automatically flag an app-global “persisted owner” as incorrect just because it uses a single shared instance (e.g., `static let shared`), if—and only if—the feature is backed by exactly one process-wide persistence (one store/file/DB) such that creating multiple per-window instances would cause races on the same persistence.
This singleton pattern is acceptable when the shared store is lazily created, the ownership model is truly app-global, and concurrency is handled appropriately (e.g., `MainActor` for `Observable` state or equivalent thread-safety). Also ensure test seams/injection are available so the persistence boundary can be controlled in tests.
Under these conditions, patterns like the ones used in `Sources/OfflineNotesStore.swift` (e.g., `MainActor Observable` + `static let shared`) are consistent with existing precedents; otherwise, prefer per-window instances or explicit coordination to prevent persistence races.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 6905
File: Sources/OfflineNotesStore.swift:0-0
Timestamp: 2026-06-26T12:31:53.114Z
Learning: When reviewing Swift code in this repo, an app-global `Observable` singleton (e.g., `static let shared`) can be acceptable if it truly *owns* a single persisted app-wide backing resource, so creating multiple window-scoped instances would otherwise race on the same store. Treat the pattern as non-problematic (don’t auto-flag) when all/most of these hold: (1) the singleton is the sole owner of one persisted resource (e.g., one JSON file / one shared store), (2) window-scoped instances would contend for the same underlying storage, (3) the singleton is annotated appropriately for concurrency (often `MainActor` when used on the main actor), (4) it supports testability (e.g., is injectable or can be swapped in tests), and (5) any reachability/background work is started lazily when the relevant UI/panel opens. Use the existing app-global precedents (e.g., `PaneMemoryGuardrail`, `DiffCommentStore.shared`) as confirmation of this intended pattern. Example: `Sources/OfflineNotesStore.swift` can remain `MainActor Observable static let shared` when it persists one offline-notes JSON file app-wide and lazily begins its work when the Notes panel opens.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 4608
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-07-01T22:19:25.948Z
Learning: When reviewing Swift localization, do not require `String(localized:)` for v1 socket-protocol command reply strings used by the cmux CLI/hooks/tests (i.e., reply literals over the wire that are machine-parsed and must match exact text like `OK` / `ERROR: ...` or command “usage” text). Localizing these reply strings can break scripted clients and tests that match the literal replies. Only flag missing localization for genuine UI-visible copy (e.g., actual notification/display text), which should follow the project’s localization approach.
Comment on lines
+3
to
+17
| struct TerminalNotification: Identifiable, Hashable, Sendable { | ||
| let id: UUID | ||
| let tabId: UUID | ||
| let surfaceId: UUID? | ||
| let panelId: UUID? | ||
| let title: String | ||
| let subtitle: String | ||
| let body: String | ||
| let agentId: String? | ||
| let workspaceTitle: String? | ||
| let createdAt: Date | ||
| var isRead: Bool | ||
| var paneFlash: Bool = true | ||
| var clickAction: TerminalNotificationClickAction? | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider marking TerminalNotification nonisolated.
This is a newly introduced pure-value, Sendable model struct without an explicit nonisolated marker. If this target uses Swift 6 MainActor-by-default isolation (as referenced elsewhere in this repo's lint rules), the struct implicitly inherits MainActor isolation despite being a plain Sendable value type, which can force unnecessary actor hops for any future off-main consumer.
♻️ Suggested fix
-struct TerminalNotification: Identifiable, Hashable, Sendable {
+nonisolated struct TerminalNotification: Identifiable, Hashable, Sendable {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| struct TerminalNotification: Identifiable, Hashable, Sendable { | |
| let id: UUID | |
| let tabId: UUID | |
| let surfaceId: UUID? | |
| let panelId: UUID? | |
| let title: String | |
| let subtitle: String | |
| let body: String | |
| let agentId: String? | |
| let workspaceTitle: String? | |
| let createdAt: Date | |
| var isRead: Bool | |
| var paneFlash: Bool = true | |
| var clickAction: TerminalNotificationClickAction? | |
| nonisolated struct TerminalNotification: Identifiable, Hashable, Sendable { | |
| let id: UUID | |
| let tabId: UUID | |
| let surfaceId: UUID? | |
| let panelId: UUID? | |
| let title: String | |
| let subtitle: String | |
| let body: String | |
| let agentId: String? | |
| let workspaceTitle: String? | |
| let createdAt: Date | |
| var isRead: Bool | |
| var paneFlash: Bool = true | |
| var clickAction: TerminalNotificationClickAction? |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalNotification.swift` around lines 3 - 17, Mark
TerminalNotification as nonisolated since it is a pure value Sendable model and
should not inherit MainActor isolation by default. Update the struct declaration
itself, keeping the Identifiable, Hashable, and Sendable conformance intact, so
future consumers can use TerminalNotification off-main without unnecessary actor
hops.
Source: Coding guidelines
…ifyProcessIntegrationRegressionTests The budget code motion put this PR's new tests in a standalone XCTestCase that referenced ClaudeHookContext and the hook-run helpers, which are private to CLINotifyProcessIntegrationRegressionTests, so the test target failed to compile on CI. The moved tests now live in a cross-file extension of that class and the five shared helper declarations drop private. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/DescriptiveAgentNotificationCLITests.swift (2)
252-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
codexLaunchEnvironmentduplicates the helper already inCLINotifyProcessIntegrationRegressionTests.swift.This is a near-identical private redeclaration of
codexLaunchEnvironment(context:sessionId:)already defined inCLINotifyProcessIntegrationRegressionTests.swift. Sinceprivateis file-scoped, this extension file can't call the sibling's version, forcing a copy. The PR's commit message says shared helper declarations were "relaxed" so the extension could reach them — that relaxation coveredClaudeHookContext,runCodexHook,startAgentHookMockServerAccepting,makeClaudeHookContext, andrunClaudeHook, but missedcodexLaunchEnvironment(andagentLaunchEnvironment), leaving this duplicate. Consider dropping theprivatemodifier on the original inCLINotifyProcessIntegrationRegressionTests.swiftand deleting this copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/DescriptiveAgentNotificationCLITests.swift` around lines 252 - 259, The helper codexLaunchEnvironment(context:sessionId:) is duplicated here because the shared version in CLINotifyProcessIntegrationRegressionTests.swift is still private and inaccessible from this extension. Relax the access level on the original codexLaunchEnvironment helper (and the matching agentLaunchEnvironment if needed) so this test file can reuse it, then remove the local duplicate from DescriptiveAgentNotificationCLITests.
83-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the extra mock server here
runClaudeHookalready starts its own accept loop, sostartAgentHookMockServerAcceptingjust adds a second listener on the same socket and can make this test flaky. Keep the per-call server only, or switch the test torunClaudeHookWithoutServerif it really needs the shared agent handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/DescriptiveAgentNotificationCLITests.swift` around lines 83 - 141, The test is starting an extra mock server listener even though runClaudeHook already opens its own accept loop, which can create a second bind on the same socket and make the test flaky. Remove the explicit startAgentHookMockServerAccepting call from testClaudeTranscriptJSONAssistantMessageDoesNotBecomeNotificationBody, or switch the test to runClaudeHookWithoutServer if it truly needs shared agent handling. Keep the rest of the transcript and notification assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift`:
- Line 8622: Relax the visibility of codexLaunchEnvironment and
agentLaunchEnvironment to match the other shared harness helpers in
CLINotifyProcessIntegrationRegressionTests, so
DescriptiveAgentNotificationCLITests can reuse the same launch-environment setup
instead of duplicating it. Keep the change consistent with the
already-unprivatized symbols such as ClaudeHookContext, runCodexHook,
startAgentHookMockServerAccepting, makeClaudeHookContext, and runClaudeHook, and
update any call sites that rely on the shared environment helpers.
---
Outside diff comments:
In `@cmuxTests/DescriptiveAgentNotificationCLITests.swift`:
- Around line 252-259: The helper codexLaunchEnvironment(context:sessionId:) is
duplicated here because the shared version in
CLINotifyProcessIntegrationRegressionTests.swift is still private and
inaccessible from this extension. Relax the access level on the original
codexLaunchEnvironment helper (and the matching agentLaunchEnvironment if
needed) so this test file can reuse it, then remove the local duplicate from
DescriptiveAgentNotificationCLITests.
- Around line 83-141: The test is starting an extra mock server listener even
though runClaudeHook already opens its own accept loop, which can create a
second bind on the same socket and make the test flaky. Remove the explicit
startAgentHookMockServerAccepting call from
testClaudeTranscriptJSONAssistantMessageDoesNotBecomeNotificationBody, or switch
the test to runClaudeHookWithoutServer if it truly needs shared agent handling.
Keep the rest of the transcript and notification assertions unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c34901a-969d-4ca0-b046-7445a575be7f
📒 Files selected for processing (2)
cmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/DescriptiveAgentNotificationCLITests.swift
| } | ||
|
|
||
| private struct ClaudeHookContext { | ||
| struct ClaudeHookContext { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Visibility relaxation looks correct, but inconsistent with codexLaunchEnvironment.
Dropping private on ClaudeHookContext, runCodexHook, startAgentHookMockServerAccepting, makeClaudeHookContext, and runClaudeHook is a reasonable, minimal way to let the new DescriptiveAgentNotificationCLITests.swift extension reuse this file's mock-server harness. However, codexLaunchEnvironment/agentLaunchEnvironment were left private, so the other file had to redeclare its own copy of codexLaunchEnvironment — see the corresponding comment in DescriptiveAgentNotificationCLITests.swift. Consider relaxing those too for consistency.
Also applies to: 8675-8675, 8718-8718, 8787-8787, 8803-8803
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift` at line 8622,
Relax the visibility of codexLaunchEnvironment and agentLaunchEnvironment to
match the other shared harness helpers in
CLINotifyProcessIntegrationRegressionTests, so
DescriptiveAgentNotificationCLITests can reuse the same launch-environment setup
instead of duplicating it. Keep the change consistent with the
already-unprivatized symbols such as ClaudeHookContext, runCodexHook,
startAgentHookMockServerAccepting, makeClaudeHookContext, and runClaudeHook, and
update any call sites that rely on the shared environment helpers.
… form Codex review flagged that a bare a=<agent-id> 4th segment is easy for a legacy pipe-containing body tail (e.g. "...|a=prod") to collide with. The CLI now serializes uncategorized agent notifications as c=other;p=0;a=<id> and the parser accepts agent identity only when paired with the category grammar, so the reserved-segment surface stays the c=-anchored shape that predates this PR. c=other without an agent id stays invalid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/AgentNotificationGateTests.swift (1)
141-146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise invalid IDs through the canonical meta form.
Lines 142-145 currently pass even if
validAgentIdaccepts empty, underscore, or overlong values, because barea=is rejected before validation. Usec=...;p=...;a=...for each invalid case.Proposed test tightening
- `#expect`(AgentNotificationMeta(meta: "a=") == nil) - `#expect`(AgentNotificationMeta(meta: "a=UPPER") == nil) - `#expect`(AgentNotificationMeta(meta: "a=has_underscore") == nil) - `#expect`(AgentNotificationMeta(meta: "a=abcdefghijklmnopqrstuvwxyzabcdefg") == nil) + `#expect`(AgentNotificationMeta(meta: "c=turn-complete;p=1;a=") == nil) + `#expect`(AgentNotificationMeta(meta: "c=turn-complete;p=1;a=UPPER") == nil) + `#expect`(AgentNotificationMeta(meta: "c=turn-complete;p=1;a=has_underscore") == nil) + `#expect`(AgentNotificationMeta(meta: "c=turn-complete;p=1;a=abcdefghijklmnopqrstuvwxyzabcdefg") == nil) `#expect`(AgentNotificationMeta(meta: "c=turn-complete;p=1;a=UPPER") == nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/AgentNotificationGateTests.swift` around lines 141 - 146, The invalid AgentNotificationMeta cases are not exercising validAgentId because bare a= gets rejected before agent ID validation. Update the meta-based tests in AgentNotificationGateTests to use the canonical c=...;p=...;a=... form for each invalid agent ID case, including empty, uppercase, underscore, and overlong values. Keep the existing meta parsing path but make sure the a= field is the part under test so validAgentId is actually reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 12467-12469: The metadata parsing in AgentNotificationMeta
handling is too permissive because the fourth segment is being trimmed before
strict parsing, which can incorrectly accept legacy tails as canonical metadata.
Update the parsing path in the related body-splitting logic to use the raw
fourth segment from parts[3] (without trimming) when constructing
AgentNotificationMeta, and only strip it from the body if that strict parse
succeeds.
---
Outside diff comments:
In `@cmuxTests/AgentNotificationGateTests.swift`:
- Around line 141-146: The invalid AgentNotificationMeta cases are not
exercising validAgentId because bare a= gets rejected before agent ID
validation. Update the meta-based tests in AgentNotificationGateTests to use the
canonical c=...;p=...;a=... form for each invalid agent ID case, including
empty, uppercase, underscore, and overlong values. Keep the existing meta
parsing path but make sure the a= field is the part under test so validAgentId
is actually reached.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19398d88-2292-4905-8991-635894ff4a63
📒 Files selected for processing (5)
CLI/CMUXCLI+NotificationSummaryHelpers.swiftSources/AgentNotificationGate.swiftSources/TerminalController.swiftcmuxTests/AgentNotificationGateTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
Comment on lines
+12467
to
+12469
| let candidate = parts[3].trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if candidate.hasPrefix("c="), let parsed = AgentNotificationMeta(meta: candidate) { | ||
| if candidate.hasPrefix("c="), | ||
| let parsed = AgentNotificationMeta(meta: candidate) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not trim before strict meta parsing.
Line 12467 weakens the “exact canonical serialization” guarantee: a legacy body tail like | c=turn-complete;p=1 is trimmed, parsed as metadata, and removed from the body. Parse the raw fourth segment instead.
Proposed fix
- let candidate = parts[3].trimmingCharacters(in: .whitespacesAndNewlines)
+ let candidate = parts[3]
if candidate.hasPrefix("c="),
let parsed = AgentNotificationMeta(meta: candidate) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let candidate = parts[3].trimmingCharacters(in: .whitespacesAndNewlines) | |
| if candidate.hasPrefix("c="), let parsed = AgentNotificationMeta(meta: candidate) { | |
| if candidate.hasPrefix("c="), | |
| let parsed = AgentNotificationMeta(meta: candidate) { | |
| let candidate = parts[3] | |
| if candidate.hasPrefix("c="), | |
| let parsed = AgentNotificationMeta(meta: candidate) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController.swift` around lines 12467 - 12469, The metadata
parsing in AgentNotificationMeta handling is too permissive because the fourth
segment is being trimmed before strict parsing, which can incorrectly accept
legacy tails as canonical metadata. Update the parsing path in the related
body-splitting logic to use the raw fourth segment from parts[3] (without
trimming) when constructing AgentNotificationMeta, and only strip it from the
body if that strict parse succeeds.
…sition If a notification hook rewrote the title, subtitle, or body, the hook owns the banner verbatim: the store drops the workspace title for that delivery so agent recomposition never demotes a hook-provided title into the subtitle. Untouched notifications keep the composed workspace-aware banner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The opencode plugin maps every session.idle callback to a Stop feed event with no turn identity, so a replayed idle for the same turn re-notified. The app-side handler now records a session+message fingerprint per surface in a bounded lock-protected map and drops repeats before enqueueing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lastPrompt is per-turn state. A prompt-submit without an extractable prompt previously preserved the prior turn's prompt, so a later completion banner could say "Finished: <previous prompt>". recordPromptSubmit now replaces the field unconditionally; other session upserts still preserve it. Adds a CLI process regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Review exception (documented deliberately): the structured review flagged that permission banners now include the command text ("Allow Bash: "). Keeping as designed: showing the concrete command needing approval is the point of this PR's permission banners, the snippet is capped at 120 chars, it mirrors what the agent already prints in the visible terminal, and macOS notification previews remain OS-gated (Notification Center preview settings). If we later want redaction for tokens in commands, that's a follow-up across all permission surfaces, not just banners. |
Review flagged that a pure text fingerprint could permanently drop a later turn whose banner text repeats an earlier one byte-for-byte. The plugin pushes UserPromptSubmit through the same feed path, so the handler now clears the surface's fingerprint on every user prompt: same-turn idle replays stay suppressed, identical-text later turns still notify. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The static property inherited TerminalController's main-actor isolation and tripped the Swift warning budget when referenced from the nonisolated feed path. The deduper is Sendable (lock-protected state), so nonisolated is correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Feed permission/question/plan banners quote raw tool input (shell commands, question text), which can embed tokens, URL credentials, or key=value secrets. Native banners escape the app via Notification Center and lock screen previews, so the composed body now goes through CmuxFoundation's SentryScrubber (secrets, URL credentials, emails, home paths) before delivery. Adds a redaction test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Update on the review exception above: resolved in e258a5a instead of excepted. Feed banner bodies (permission commands, question text) now pass through CmuxFoundation's SentryScrubber before OS notification delivery, so tokens, URL credentials, key=value secrets, emails, and home paths are redacted while the command stays readable ("Allow Bash: API_TOKEN= ./deploy.sh"). Regression test added. |
Desktop banners route through NotificationBannerComposer's scrub at the single materialization point, but PhonePushClient.forward() read the raw stored TerminalNotification fields directly, bypassing it. Build the phone payload from the same scrubbed banner content instead. Also fixes the ordering in notificationBannerSnippet: scrub before truncating, so a credential straddling the length boundary can't be cut mid-pattern and evade the scrubber.
Two follow-on gaps in the banner scrubbing: (1) Path-A banners (agent stop and idle bodies, prompt fallbacks, cmux notify, custom notification commands) were not scrubbed at all; composeNotificationBannerContent now redacts the body at the single materialization point every producer flows through. (2) notificationBannerSnippet truncated before scrubbing, so a credential straddling the length boundary could survive as a partial leak; the helper now redacts first and truncates the redacted text. Tests cover both, including a boundary-straddling token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e per turn NotificationBannerComposer only scrubbed body. workspaceTitle (derived from a path, prompt, or auto-name) now flows into native/feed banner titles and subtitles too, so scrub every field at the single materialization point before returning NotificationBannerContent. Resources/opencode-plugin.js: state.assistantPreamble survived across turns, so a session.idle firing before the new turn emitted any assistant text part reused the previous turn's summary as the new completion banner body. Reset it when a new user prompt arrives.
Contributor
Author
|
Final review-boundary note: structured review asked (pass 9) to also scrub workspace titles and subtitles in banner composition. Deliberately not doing that. Scrubbing is applied to every free-text field that carries agent/tool-produced content (permission commands, question/plan text, assistant messages, prompt fallbacks, and the phone-push forwarding path), redacted before truncation. Titles/subtitles carry the user's own workspace label, the agent display name, and localized status words; scrubbing them would redact directory-derived workspace names (the scrubber treats home paths as secrets), mangling the feature's primary output to hide the user's own path from themselves. Hook-rewritten titles are the user's own automation output by construction. |
UUID in the deduper tests needs it; all four app-host CI shards failed on test-target compile. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
OpenCode installs both the legacy CLI-subprocess plugin and the new socket-RPC plugin, so every turn completion fired two notifications: the CLI's generic notify_target_async path and the new app-side v2PostOpenCodeStopNotificationIfNeeded. Set publishesStopNotification to false for opencode (same pattern already used for grok) so only the new descriptive path fires. composeFeedNotificationContent scrubbed title and body but not subtitle, leaving the source/kind label out of the redaction pass at the same materialization point as the other fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The publishesStopNotification: false addition for opencode grew the file to 783 lines, one over the checked-in budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This PR's notify meta grammar now tags the Claude Stop-hook notification with agentId "claude" (CLI/cmux.swift:23339), so the pre-existing regression test's expected notify_target_async string needs the ;a=claude suffix to match.
Resolves conflicts from main's Kimi hook refactor (agentDefs extracted to CMUXCLI+AgentHookCatalog.swift) intersecting with this PR's notification descriptive-body work: - CMUXCLI+AgentHookDefinitions.swift: took main's version (agentDefs moved to CMUXCLI+AgentHookCatalog.swift); reapplied this PR's opencode publishesStopNotification:false fix to the new file. - CLI/cmux.swift: removed main's stale copy of notification helper functions this PR already relocated into CMUXCLI+NotificationSummaryHelpers.swift (with agentId support). - Sources/TerminalController.swift: kept main's nonisolated annotation on parseNotificationPayload plus this PR's updated doc comment covering the a=<agent-id> meta segment. - project.pbxproj: merged both branches' new file group/build-phase entries (NotificationSummaryHelpers.swift + ClaudePushNotificationHook.swift/ AgentHookRestoreEvidence.swift). - swift-file-length-budget.tsv: dropped the stale pre-refactor entry for CMUXCLI+AgentHookDefinitions.swift; main's correct 559-line entry already exists. Brings in main's fix for the release-build compiler-timeout in MobileTerminalRenderGridReplay+ModeReset.swift that was causing this branch's stale, un-merged CI runs to fail.
…ift growth CI's workflow-guard-tests job failed: threading agentId through the notification queue (Sources/TerminalNotificationQueue.swift) grew the file from 503 to 509 lines, past its tracked budget. Regenerated via scripts/swift_file_length_budget.py --write-budget, which also tightens a few entries that shrank since the last snapshot.
Resolve the current swift-file-length-budget and Xcode project conflicts. Tighten notification meta parsing so only byte-exact canonical c=... payload tails are stripped, and add regression coverage for whitespace-prefixed legacy body tails plus canonical invalid agent IDs.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7eadab5. Configure here.
| ?? parsedInput.transcriptPath | ||
| .flatMap({ readTranscriptSummary(path: $0)?.lastAssistantMessage }) | ||
| .flatMap(normalizedHookValue) { | ||
| summary = (subtitle: summary.subtitle, body: truncate(replacementBody, maxLength: 180)) |
There was a problem hiding this comment.
Idle nag prefers stale body
Medium Severity
When replacing Claude’s generic idle nag text, the handler uses the session’s saved lastBody before re-reading the transcript. If Stop stored a generic “session completed” fallback in lastBody, a later idle notification can show that boilerplate instead of a newer assistant message that exists in the transcript path supplied on the notification.
Reviewed by Cursor Bugbot for commit 7eadab5. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
2 participants
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.
Agent notification banners were generic: title was the agent name ("Claude Code", "Codex"), the workspace never appeared, and bodies frequently fell back to boilerplate ("Claude is waiting for your input", " session completed", "Agent is asking a question").
Now every agent banner is composed at delivery time as: title = workspace name, subtitle = " · ", body = the most specific message available.
Mechanism:
c=<cat>;p=<0|1>;a=<agent>plus an agent-onlya=<agent>form for ungated notifications. Parsing stays strict; legacy 3-field andc=..;p=..payloads parse and gate byte-identically (regression-tested at both the meta parser and a live-socket process test).composeNotificationBannerContenthelper applied at every banner materialization point (UN content, local/suppressed feedback, custom notification command). Stored notification fields are unchanged, so the in-app Notifications page keeps its rendering.permission_promptmessages are untouched, and category/pending gating logic is unchanged.{"findings":[]...}banners from Codex).session.idleevent now carries the last assistant/user message and the app posts a Path-A notification gated by the same turn-complete setting, restricted tosource == "opencode"so hook-based agents cannot double-notify.publishCodexMonitorUserInput/Failure) are taggeda=codexso they get workspace composition too.All new user-facing strings are localized (en + ja). New test files are pbxproj-wired: AgentNotificationGateTests extensions, NotificationBannerCompositionTests, FeedNotificationContentTests, CLI process tests for idle-body replacement, prompt fallback, JSON-blob guard, and legacy byte-identity.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches notification delivery, wire meta parsing, and multiple agent hook paths; legacy payload compatibility is explicitly preserved but the surface area for missed banners or double-notify is non-trivial.
Overview
Agent and terminal notifications are reworked so OS banners emphasize the workspace title, an agent · status subtitle, and a specific body instead of generic agent names and boilerplate.
Notify payloads gain optional
a=<agent-id>on the existingc=…;p=…meta (includingc=other;p=0;a=…for ungated alerts). Parsing stays strict so legacy 3-field payloads behave unchanged. Hook paths for Claude, Codex, and generic agents attach agent identity; completion bodies can use "Finished: <prompt>" from per-turnlastPromptstored on prompt-submit.NotificationBannerComposercentralizes banner text at delivery (native notifications, suppressed feedback, phone push) with secret/path scrubbing before truncation; stored notification fields are unchanged for the in-app list.TerminalNotificationcarries optionalagentIdandworkspaceTitle.Claude idle nags replace stock "waiting for your input" copy with the last assistant message from the session store or transcript, skipping JSON-shaped assistant text. OpenCode plugin sends
surface_id, clears stale assistant text per user message, includes assistant text onsession.idle, disables CLI stop notifications, and the app posts gated turn-complete banners with per-surface dedupe reset on each prompt. Feed permission/question/plan notifications use the shared composer with workspace titles and tool-input-derived bodies.Reviewed by Cursor Bugbot for commit 7eadab5. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Make agent notifications descriptive and workspace-aware. OS banners now show the workspace title, an " · " subtitle, and specific bodies; all fields are scrubbed before truncation, and phone push uses the same content.
New Features
c=<category>;p=<0|1>;a=<agent>; the parser only accepts thisc=-anchored form. Uncategorized alerts usec=other;p=0;a=<agent>.NotificationBannerComposer.composeNotificationBannerContentand use it for agent, feed, and custom banners;TerminalNotificationstores optionalagentIdandworkspaceTitlefor composition.opencodeposts a gated turn-complete banner viasession.idlewith per-surface dedupe re-armed on each user prompt. All user strings localized (en, ja).Bug Fixes
opencodesnippets; permission banners fall back to " needs approval" when no detail exists; notification-hook rewrites are preserved verbatim.c=...;p=<0|1>[;a=<id>]tails are stripped; whitespace-prefixed or malformed tails remain in the body to preserve legacy behavior.opencodeturn-complete banners per surface and disabled the legacy CLI stop notification path (publishesStopNotification: false); reset the OpenCode assistant preamble per turn; clearedlastPrompton every prompt-submit; legacy 3-field andc=..;p=..payloads parse and gate exactly as before.Written for commit 7eadab5. Summary will update on new commits.
Summary by CodeRabbit