Repository navigation
Improve OpenCode hook detail rendering - #4847
Closed
lawrencecchen wants to merge 1 commit into
Closed
lawrencecchen wants to merge 1 commit into
lawrencecchen wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds incremental text buffering and permissionToolInput in the OpenCode plugin; centralizes feed notification body generation; makes Feed UI source-aware (markdown and OpenCode display name) and refactors permission input parsing; updates CLI agent flag and adds tests and localization entries. ChangesOpenCode Feed & Permission Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (10 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 |
Contributor
Greptile SummaryThis PR enriches the OpenCode feed hook pipeline end-to-end: the JS plugin captures assistant text during
Confidence Score: 5/5Safe to merge; no functional regressions were found across the JS plugin, Swift coordinator, or CLI changes. The three-layer change (JS plugin → socket protocol → Swift rendering) is well-tested with unit tests covering each new notification body path and an end-to-end harness that exercises socket framing, Stop payload propagation, and the absence of presentation keys in PermissionRequest payloads. The @mainactor call site is correctly wrapped in the existing MainActor.assumeIsolated block. The only gaps are a robustness edge in permissionToolInput and an unused xcstrings entry — both inconsequential for current behaviour. No files require special attention for correctness; Resources/opencode-plugin.js has the metadata-spread robustness gap noted in the review comment. Important Files Changed
Sequence DiagramsequenceDiagram
participant OC as OpenCode runtime
participant JS as opencode-plugin.js (CMUXFeed)
participant Sock as cmux Unix socket
participant FC as FeedCoordinator (Swift)
participant NS as TerminalNotificationStore
OC->>JS: "message.updated (role=assistant)"
OC->>JS: message.part.updated (text part)
JS->>JS: recordMessageText → state.assistantPreamble
OC->>JS: "permission.asked (permission=bash)"
JS->>JS: permissionToolInput → canonical action, no presentation keys
JS->>Sock: feed.push PermissionRequest (blocking)
Sock->>JS: status approved
JS->>OC: hook reply
OC->>JS: session.idle
JS->>JS: stopToolInput → reason + last_assistant_message
JS->>Sock: feed.push Stop (telemetry)
Sock->>FC: "WorkstreamEvent Stop source=opencode"
FC->>FC: ingestBlocking → MainActor.assumeIsolated
FC->>FC: postTelemetryNotificationIfNeeded
FC->>FC: "basicStopNotificationContent title=OpenCode completed"
FC->>NS: addNotification tabId surfaceId title body
Reviews (6): Last reviewed commit: "Improve OpenCode feed hook details" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
1567
to
+1653
| let dict = (try? JSONSerialization.jsonObject( | ||
| with: Data(toolInputJSON.utf8) | ||
| )) as? [String: Any] ?? [:] | ||
|
|
||
| switch toolName.lowercased() { | ||
| let effective = Self.effectiveInput(from: dict) | ||
| let name = (Self.firstString( | ||
| effective["permission"], | ||
| effective["toolName"], | ||
| effective["tool_name"], | ||
| toolName | ||
| ) ?? toolName).lowercased() | ||
| let summary = Self.firstString(effective["summary"], effective["title"]) | ||
| let detailLines = Self.stringArray(effective["lines"]) | ||
| let patterns = Self.stringArray(effective["patterns"]) | ||
|
|
||
| switch name { | ||
| case "bash": | ||
| self.sigil = "$" | ||
| self.primary = (dict["command"] as? String) ?? toolInputJSON | ||
| self.secondary = (dict["description"] as? String) | ||
| case "write", "edit", "multiedit": | ||
| let primary = Self.firstString(effective["command"], effective["cmd"], detailLines.first) ?? toolInputJSON | ||
| self.primary = primary | ||
| self.secondary = Self.secondaryText( | ||
| preferred: Self.firstString(effective["description"], summary), | ||
| excluding: primary, | ||
| fallbackLines: Array(detailLines.dropFirst()) | ||
| ) | ||
| case "write", "edit", "multiedit", "apply_patch": | ||
| self.sigil = nil | ||
| self.primary = (dict["file_path"] as? String) ?? toolInputJSON | ||
| if toolName.lowercased() == "write" { | ||
| let content = (dict["content"] as? String) ?? "" | ||
| let primary = Self.firstString( | ||
| effective["filePath"], | ||
| effective["file_path"], | ||
| effective["filepath"], | ||
| effective["path"], | ||
| patterns.first, | ||
| summary | ||
| ) ?? (toolInputJSON == "{}" ? nil : toolInputJSON) | ||
| self.primary = primary | ||
| if name == "write" { | ||
| let content = Self.firstString(effective["content"]) ?? "" | ||
| let preview = content.split(separator: "\n").first.map(String.init) ?? "" | ||
| self.secondary = preview.isEmpty ? nil : preview | ||
| self.secondary = Self.secondaryText( | ||
| preferred: preview.isEmpty ? nil : preview, | ||
| excluding: primary, | ||
| fallbackLines: detailLines | ||
| ) | ||
| } else { | ||
| self.secondary = nil | ||
| self.secondary = Self.secondaryText( | ||
| preferred: Self.firstString(effective["description"]), | ||
| excluding: primary, | ||
| fallbackLines: detailLines | ||
| ) | ||
| } | ||
| case "read": | ||
| self.sigil = nil | ||
| self.primary = (dict["file_path"] as? String) ?? toolInputJSON | ||
| self.secondary = nil | ||
| let primary = Self.firstString( | ||
| effective["filePath"], | ||
| effective["file_path"], | ||
| effective["filepath"], | ||
| effective["path"], | ||
| patterns.first, | ||
| summary | ||
| ) ?? (toolInputJSON == "{}" ? nil : toolInputJSON) | ||
| self.primary = primary | ||
| self.secondary = Self.secondaryText(excluding: primary, fallbackLines: detailLines) | ||
| case "glob", "grep": | ||
| self.sigil = nil | ||
| let primary = Self.firstString(effective["pattern"], patterns.first, summary) ?? (toolInputJSON == "{}" ? nil : toolInputJSON) | ||
| self.primary = primary | ||
| self.secondary = Self.secondaryText(excluding: primary, fallbackLines: detailLines) | ||
| case "list": | ||
| self.sigil = nil | ||
| let primary = Self.firstString(effective["path"], effective["directory"], patterns.first, summary) ?? (toolInputJSON == "{}" ? nil : toolInputJSON) | ||
| self.primary = primary | ||
| self.secondary = Self.secondaryText(excluding: primary, fallbackLines: detailLines) | ||
| case "task": | ||
| self.sigil = "*" | ||
| let primary = Self.firstString(effective["description"], effective["prompt"], summary) ?? (toolInputJSON == "{}" ? nil : toolInputJSON) | ||
| self.primary = primary | ||
| self.secondary = Self.secondaryText(excluding: primary, fallbackLines: detailLines) | ||
| case "webfetch": | ||
| self.sigil = "%" | ||
| let primary = Self.firstString(effective["url"], patterns.first, summary) ?? (toolInputJSON == "{}" ? nil : toolInputJSON) | ||
| self.primary = primary | ||
| self.secondary = Self.secondaryText(excluding: primary, fallbackLines: detailLines) | ||
| case "websearch": | ||
| self.sigil = "*" | ||
| let primary = Self.firstString(effective["query"], patterns.first, summary) ?? (toolInputJSON == "{}" ? nil : toolInputJSON) | ||
| self.primary = primary | ||
| self.secondary = Self.secondaryText(excluding: primary, fallbackLines: detailLines) |
Contributor
There was a problem hiding this comment.
FeedPanelView.swift is 4 056 lines post-merge and now gains effectiveInput, firstString, stringArray, and secondaryText — pure parsing/data helpers — inside PermissionInputPreview. The swift-file-package-boundaries rule flags files that mix UI rendering with parsing, and calls out that the threshold for new additions to an already-oversized file is 250 lines. This PR adds ~158 lines, just under the numeric trigger, but the helpers have no dependency on SwiftUI and would be independently testable. A PermissionPayloadParser value type (or a small FeedPermission package target) would clear the mixed-responsibility concern and enable unit tests without constructing any views.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Resources/opencode-plugin.js`:
- Around line 58-163: The permissionDisplayInfo function is emitting English UI
text (summary/title/lines/icon) which must not be serialized for localization;
change permissionDisplayInfo to return canonical, language-neutral fields (e.g.,
action/verb, resource/file, operation, pattern, position/line/character, diff,
patterns array, description) instead of human-readable titles/lines/icons, and
update permissionToolInput to copy those canonical fields into the toolInput
payload (keep permission, patterns, always, metadata, filePath/file_path, diff,
and add action/verb/resource/operation/position/patterns/description fields)
while removing summary/title/lines/icon from the emitted payload so the Swift
side can render localized labels.
🪄 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: e36aa805-0506-436c-b728-9b19edd58f6b
📒 Files selected for processing (5)
Resources/Localizable.xcstringsResources/opencode-plugin.jsSources/Feed/FeedCoordinator.swiftSources/Feed/FeedPanelView.swiftcmuxTests/FeedCoordinatorTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lawrencecchen
force-pushed
the
task-opencode-rich-hooks
branch
from
May 27, 2026 04:27
0dfa50b to
9d09af2
Compare
lawrencecchen
force-pushed
the
task-opencode-rich-hooks
branch
from
May 27, 2026 04:42
9d09af2 to
0e5c827
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Feed/FeedCoordinator.swift`:
- Around line 432-472: permissionNotificationBody currently injects raw
commands, paths, URLs and queries into notifications; create and call a
sanitizer (e.g., redactSensitive or sanitizeNotificationField) before building
the array passed to compactNotificationText so sensitive values are replaced
with safe summaries. Specifically, when handling "bash" replace command text
with a generic "Bash command" or a truncated/safe basename via a helper; for
edit/write/read/list convert filePath/file_path/filepath/path to basename-only
or "file path" placeholder; for webfetch/websearch/grep/glob replace
url/query/pattern with "URL" or "Search query" or a redacted preview; apply this
sanitizer to values returned by firstString(...) and stringArray(...) (and to
the final lines fallback) so permissionNotificationBody never returns raw
secrets.
🪄 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: 77331ec7-9d90-4cb5-8d48-8ef6fbca97ac
📒 Files selected for processing (7)
CLI/CMUXCLI+AgentHookDefinitions.swiftResources/Localizable.xcstringsResources/opencode-plugin.jsSources/Feed/FeedCoordinator.swiftSources/Feed/FeedPanelView.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/FeedCoordinatorTests.swift
lawrencecchen
force-pushed
the
task-opencode-rich-hooks
branch
from
May 27, 2026 05:27
0e5c827 to
e30995d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Sources/Feed/FeedCoordinator.swift (1)
417-512:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep notification bodies coarse and non-sensitive.
These builders still lift raw commands, file paths, URLs, search queries, question text/options, plan text, and assistant stop text into notification bodies. Those values can contain secrets or private workspace context and may be shown outside the app. Please keep banner copy to a localized summary and reserve the rich payload for the in-app Feed.
As per coding guidelines, "user-facing errors, alerts, command output, API error bodies, and recovery copy must not expose ... unredacted payload dumps."
🤖 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/Feed/FeedCoordinator.swift` around lines 417 - 512, The notification builders (notificationBody, permissionNotificationBody, exitPlanNotificationBody, questionNotificationBody, stopNotificationBody) currently include raw commands, file paths, URLs, queries, plan text, question/options and assistant final text; change them to return coarse, non-sensitive localized summaries (e.g. "Action requires permission", "Plan completed", "Assistant asked a question", "Operation stopped") or nil instead of embedding raw fields like dict["command"], dict["filePath"], dict["url"], dict["query"], preview.planText, event.assistantFinalMessage, or question option text; keep the detailed payload only in the in-app Feed and ensure compactNotificationText is fed only these short, generic strings.Resources/opencode-plugin.js (1)
60-165:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep permission payloads language-neutral.
permissionDisplayInfonow serializes English UI copy intosummary/title/lines, and the Swift side consumes those fields directly. That hard-locks OpenCode permission cards and notifications to English and makes later localization much harder. Please emit canonical fields here (path, pattern, operation, position, diff, etc.) and build the user-facing strings in Swift instead.As per coding guidelines, "All user-facing strings must be localized."
🤖 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 `@Resources/opencode-plugin.js` around lines 60 - 165, permissionDisplayInfo and permissionToolInput are embedding English UI strings into summary/title/lines which prevents localization; change permissionDisplayInfo to return canonical machine-readable fields (e.g., command, file, path, pattern, operation, position, diff, description, type, url, query, patterns) instead of English sentences, keep only minimal non-textual markers like icon, and then have permissionToolInput populate toolInput with those canonical keys (e.g., filePath/file_path/path, pattern, operation, position, diff, command, description, url, query, type) rather than setting summary/title/lines so the Swift client can render localized strings from those canonical fields (update references to permissionDisplayInfo and permissionToolInput accordingly).
🤖 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/Feed/FeedPanelView.swift`:
- Around line 46-64: The feedDisplayName fallback currently uses
rawValue.capitalized which misformats camel-cased sources; update the
WorkstreamSource.feedDisplayName to explicitly map every enum case (e.g.,
hermesAgent, StopActionArea, etc.) to the correct display string instead of
falling back to rawValue.capitalized, or introduce a dedicated formatter that
preserves branding/casing and call it from feedDisplayName; locate the private
extension WorkstreamSource and replace the default branch with explicit case
mappings or a call to the new formatter so no source relies on capitalized
rawValue.
In `@tests/test_opencode_plugin_install.py`:
- Line 291: Replace the fixed 100ms sleep (the await new Promise((resolve) =>
setTimeout(resolve, 100)) call) with an event-driven wait that listens for the
Stop frame from the plugin/process under test and uses a timeout guard;
specifically, locate the test in tests/test_opencode_plugin_install.py where the
sleep appears and swap that line for a wait helper (e.g., waitForFrame or an
equivalent promise that resolves when frame.type === 'Stop') with a reasonable
timeout (e.g., 5s) to fail fast on hangs while avoiding flaky timing-based
failures.
- Line 159: Replace the hardcoded /tmp socket path by constructing the socket
under the test's temporary directory (e.g., use the pytest tmp_path fixture):
set check_env["CMUX_SOCKET_PATH"] = str(tmp_path /
f"cmux-opencode-feed-{os.getpid()}.sock") and ensure the test function signature
includes the tmp_path parameter so the fixture is available; update any helper
calls if needed to pass tmp_path into the scope where check_env is set.
---
Duplicate comments:
In `@Resources/opencode-plugin.js`:
- Around line 60-165: permissionDisplayInfo and permissionToolInput are
embedding English UI strings into summary/title/lines which prevents
localization; change permissionDisplayInfo to return canonical machine-readable
fields (e.g., command, file, path, pattern, operation, position, diff,
description, type, url, query, patterns) instead of English sentences, keep only
minimal non-textual markers like icon, and then have permissionToolInput
populate toolInput with those canonical keys (e.g., filePath/file_path/path,
pattern, operation, position, diff, command, description, url, query, type)
rather than setting summary/title/lines so the Swift client can render localized
strings from those canonical fields (update references to permissionDisplayInfo
and permissionToolInput accordingly).
In `@Sources/Feed/FeedCoordinator.swift`:
- Around line 417-512: The notification builders (notificationBody,
permissionNotificationBody, exitPlanNotificationBody, questionNotificationBody,
stopNotificationBody) currently include raw commands, file paths, URLs, queries,
plan text, question/options and assistant final text; change them to return
coarse, non-sensitive localized summaries (e.g. "Action requires permission",
"Plan completed", "Assistant asked a question", "Operation stopped") or nil
instead of embedding raw fields like dict["command"], dict["filePath"],
dict["url"], dict["query"], preview.planText, event.assistantFinalMessage, or
question option text; keep the detailed payload only in the in-app Feed and
ensure compactNotificationText is fed only these short, generic strings.
🪄 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: c043b598-7fc4-428c-948a-b1210e2e4ef1
📒 Files selected for processing (8)
CLI/CMUXCLI+AgentHookDefinitions.swiftResources/Localizable.xcstringsResources/opencode-plugin.jsSources/Feed/FeedCoordinator.swiftSources/Feed/FeedPanelView.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/FeedCoordinatorTests.swifttests/test_opencode_plugin_install.py
Comment on lines
+46
to
+64
| private extension WorkstreamSource { | ||
| var feedDisplayName: String { | ||
| switch self { | ||
| case .opencode: | ||
| return String(localized: "feed.source.opencode", defaultValue: "OpenCode") | ||
| default: | ||
| return rawValue.capitalized | ||
| } | ||
| } | ||
|
|
||
| var rendersFeedMarkdown: Bool { | ||
| switch self { | ||
| case .claude, .opencode: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't derive source display names from rawValue.capitalized.
That fallback misformats camel-cased sources like hermesAgent as Hermesagent, and StopActionArea now shows this helper directly to users. Please map every supported WorkstreamSource explicitly, or use a dedicated formatter that preserves known branding/casing.
🤖 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/Feed/FeedPanelView.swift` around lines 46 - 64, The feedDisplayName
fallback currently uses rawValue.capitalized which misformats camel-cased
sources; update the WorkstreamSource.feedDisplayName to explicitly map every
enum case (e.g., hermesAgent, StopActionArea, etc.) to the correct display
string instead of falling back to rawValue.capitalized, or introduce a dedicated
formatter that preserves branding/casing and call it from feedDisplayName;
locate the private extension WorkstreamSource and replace the default branch
with explicit case mappings or a call to the new formatter so no source relies
on capitalized rawValue.
| check_env["CMUX_TEST_OPENCODE_PLUGIN_PATH"] = str(plugin_path) | ||
| check_env["CMUX_TEST_OPENCODE_PLUGIN_COPY_PATH"] = str(plugin_copy_path) | ||
| check_env["CMUX_TEST_OPENCODE_FEED_PLUGIN_PATH"] = str(feed_plugin_path) | ||
| check_env["CMUX_SOCKET_PATH"] = f"/tmp/cmux-opencode-feed-{os.getpid()}.sock" |
There was a problem hiding this comment.
Use the test temp directory for the socket path.
CMUX_SOCKET_PATH currently uses a predictable global /tmp location. Prefer the already-isolated temp test directory to avoid collision/hijack edge cases.
Suggested patch
- check_env["CMUX_SOCKET_PATH"] = f"/tmp/cmux-opencode-feed-{os.getpid()}.sock"
+ check_env["CMUX_SOCKET_PATH"] = str(root / f"cmux-opencode-feed-{os.getpid()}.sock")🧰 Tools
🪛 Ruff (0.15.14)
[error] 159-159: Probable insecure usage of temporary file or directory: "/tmp/cmux-opencode-feed-"
(S108)
🤖 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 `@tests/test_opencode_plugin_install.py` at line 159, Replace the hardcoded
/tmp socket path by constructing the socket under the test's temporary directory
(e.g., use the pytest tmp_path fixture): set check_env["CMUX_SOCKET_PATH"] =
str(tmp_path / f"cmux-opencode-feed-{os.getpid()}.sock") and ensure the test
function signature includes the tmp_path parameter so the fixture is available;
update any helper calls if needed to pass tmp_path into the scope where
check_env is set.
| }, | ||
| }); | ||
| await hooks.event({ event: { type: "session.idle", properties: { sessionID: "ses-rich-stop" } } }); | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Replace fixed sleep with event-driven wait to reduce flakiness.
The 100ms delay can intermittently fail on slower CI. Wait for the Stop frame (with timeout guard) instead of wall-clock sleeping.
Suggested patch
const frames = [];
+let stopSeenResolve;
+const stopSeen = new Promise((resolve) => { stopSeenResolve = resolve; });
const sockets = new Set();
@@
- frames.push(JSON.parse(line));
+ const frame = JSON.parse(line);
+ frames.push(frame);
+ if (frame.params?.event?.hook_event_name === "Stop") {
+ stopSeenResolve?.();
+ }
@@
await hooks.event({ event: { type: "session.idle", properties: { sessionID: "ses-rich-stop" } } });
-await new Promise((resolve) => setTimeout(resolve, 100));
+await Promise.race([
+ stopSeen,
+ new Promise((_, reject) => setTimeout(() => reject(new Error("timed out waiting for Stop frame")), 1500)),
+]);
sockets.forEach((socket) => socket.destroy());🤖 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 `@tests/test_opencode_plugin_install.py` at line 291, Replace the fixed 100ms
sleep (the await new Promise((resolve) => setTimeout(resolve, 100)) call) with
an event-driven wait that listens for the Stop frame from the plugin/process
under test and uses a timeout guard; specifically, locate the test in
tests/test_opencode_plugin_install.py where the sleep appears and swap that line
for a wait helper (e.g., waitForFrame or an equivalent promise that resolves
when frame.type === 'Stop') with a reasonable timeout (e.g., 5s) to fail fast on
hangs while avoiding flaky timing-based failures.
lawrencecchen
force-pushed
the
task-opencode-rich-hooks
branch
from
May 27, 2026 05:41
e30995d to
19a5bb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Resources/opencode-plugin.js (1)
129-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip upstream presentation keys before flattening
metadataintotool_input.
toolInputstill blindly spreadsmetadataandmetadata.input. If OpenCode includessummary,title,icon, or displaylinesthere, those English presentation strings will flow straight into Feed/notifications again and bypass Swift-side localization. Filter those keys before the spread and keep this payload language-neutral.♻️ Minimal patch
const metadata = isObject(props.metadata) ? props.metadata : {}; const input = isObject(metadata.input) ? metadata.input : {}; + const { + summary: _metadataSummary, + title: _metadataTitle, + icon: _metadataIcon, + lines: _metadataLines, + displayLines: _metadataDisplayLines, + display_lines: _metadataDisplayLinesSnake, + ...safeMetadata + } = metadata; + const { + summary: _inputSummary, + title: _inputTitle, + icon: _inputIcon, + lines: _inputLines, + displayLines: _inputDisplayLines, + display_lines: _inputDisplayLinesSnake, + ...safeInput + } = input; const patterns = stringList(props.patterns); const always = stringList(props.always); const payloadInfo = permissionPayloadInfo(permission, input, metadata, patterns); const toolInput = { - ...metadata, - ...input, + ...safeMetadata, + ...safeInput, permission, patterns, always,As per coding guidelines, "For production user-facing text, fail partial localization..." and "All user-facing strings must be localized."
🤖 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 `@Resources/opencode-plugin.js` around lines 129 - 143, The toolInput construction in permissionToolInput currently spreads metadata and metadata.input directly, which can leak presentation strings (summary, title, icon, lines) into downstream feeds; create filteredMetadata and filteredInput by copying metadata and input while deleting or omitting keys ['summary','title','icon','lines'] (or using a small helper to strip those keys), then spread filteredMetadata and filteredInput into toolInput (and still include the original metadata under the metadata key if needed) so the payload remains language-neutral; update any uses of metadata/input in permissionToolInput to use those filtered variants before building toolInput.
🤖 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 `@Resources/opencode-plugin.js`:
- Around line 604-614: The current handler for event.type ===
"message.part.delta" records buffered delta text as assistant output even when
the message role is unknown; update the logic so you only call recordMessageText
when we have an explicit role (meta?.role) for the message. Concretely, inside
the block that builds key/text (using messagePartKey, messagePartText,
messagePartTypes), change the condition from checking only
messagePartTypes.get(key) === "text" to also require meta?.role (e.g., if
(messagePartTypes.get(key) === "text" && meta?.role) {
recordMessageText(sessionId, props.messageID, meta.role, text); }), leaving text
buffering in messagePartText intact so the delta can be recorded later when
messageRoles is set.
---
Duplicate comments:
In `@Resources/opencode-plugin.js`:
- Around line 129-143: The toolInput construction in permissionToolInput
currently spreads metadata and metadata.input directly, which can leak
presentation strings (summary, title, icon, lines) into downstream feeds; create
filteredMetadata and filteredInput by copying metadata and input while deleting
or omitting keys ['summary','title','icon','lines'] (or using a small helper to
strip those keys), then spread filteredMetadata and filteredInput into toolInput
(and still include the original metadata under the metadata key if needed) so
the payload remains language-neutral; update any uses of metadata/input in
permissionToolInput to use those filtered variants before building toolInput.
🪄 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: e054f7ec-9ffb-4e37-8d04-f45502566dff
📒 Files selected for processing (8)
CLI/CMUXCLI+AgentHookDefinitions.swiftResources/Localizable.xcstringsResources/opencode-plugin.jsSources/Feed/FeedCoordinator.swiftSources/Feed/FeedPanelView.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/FeedCoordinatorTests.swifttests/test_opencode_plugin_install.py
Comment on lines
+604
to
+614
| if (event.type === "message.part.delta") { | ||
| if (props.field !== "text" || !props.partID || !props.messageID) return null; | ||
| const meta = messageRoles.get(props.messageID); | ||
| const sessionId = meta?.sessionId || props.sessionID; | ||
| const key = messagePartKey(props.messageID, props.partID); | ||
| const text = `${messagePartText.get(key) || ""}${props.delta || ""}`; | ||
| messagePartText.set(key, text); | ||
| trimMap(messagePartText); | ||
| if (messagePartTypes.get(key) === "text") { | ||
| recordMessageText(sessionId, props.messageID, meta?.role || "assistant", text); | ||
| } |
There was a problem hiding this comment.
Don't treat role-less deltas as assistant text.
When message.part.delta arrives before message.updated, this path writes the buffered text into assistantPreamble by default. If that message later resolves to user, the stale assistant value is never cleared, so PermissionRequest and Stop can show the user's prompt as assistant output. Only record delta text once the role is known.
🐛 Minimal patch
- if (messagePartTypes.get(key) === "text") {
- recordMessageText(sessionId, props.messageID, meta?.role || "assistant", text);
+ if (messagePartTypes.get(key) === "text" && meta?.role) {
+ recordMessageText(sessionId, props.messageID, meta.role, text);
}🤖 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 `@Resources/opencode-plugin.js` around lines 604 - 614, The current handler for
event.type === "message.part.delta" records buffered delta text as assistant
output even when the message role is unknown; update the logic so you only call
recordMessageText when we have an explicit role (meta?.role) for the message.
Concretely, inside the block that builds key/text (using messagePartKey,
messagePartText, messagePartTypes), change the condition from checking only
messagePartTypes.get(key) === "text" to also require meta?.role (e.g., if
(messagePartTypes.get(key) === "text" && meta?.role) {
recordMessageText(sessionId, props.messageID, meta.role, text); }), leaving text
buffering in messagePartText intact so the delta can be recorded later when
messageRoles is set.
lawrencecchen
force-pushed
the
task-opencode-rich-hooks
branch
from
May 27, 2026 06:11
19a5bb7 to
4b3642d
Compare
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 4b3642d. Configure here.
| const flushOutboxes = () => { | ||
| if (!flushOutboxQueue(telemetryOutbox)) return false; | ||
| return flushOutboxQueue(replyOutbox); | ||
| }; |
There was a problem hiding this comment.
Outbox flush order prioritizes telemetry over blocking replies
Medium Severity
flushOutboxes drains telemetryOutbox before replyOutbox. Reply frames correspond to blocking permission requests where the OpenCode agent is actively suspended waiting for a decision. Telemetry frames are fire-and-forget. If the connection fails mid-flush (e.g., writeConnected throws during telemetry), resetConnection resolves all pending reply promises as timed_out — even though the replies haven't been attempted yet. Flushing the replyOutbox first would ensure time-sensitive blocking requests get priority over best-effort telemetry.
Reviewed by Cursor Bugbot for commit 4b3642d. 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.
Summary
cmux-feed.jsthe single OpenCode completion notification owner;cmux-session.jsonly owns restore/status.OpenCode completed).Upstream reference
anomalyco/opencodelocally to match current hook payload shapes.Testing
./scripts/reload.sh --tag ocrichCMUX_CLI_BIN='/Users/lawrence/Library/Developer/Xcode/DerivedData/cmux-ocrich/Build/Products/Debug/cmux DEV ocrich.app/Contents/Resources/bin/cmux' python3 tests/test_opencode_plugin_install.pyactionwithoutsummary/title/lines/icon, and Stop emits assistant text intool_input.reason.node --check Resources/opencode-plugin.jsjq empty Resources/Localizable.xcstringsgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests
Note
Medium Risk
Touches agent hook install, OpenCode launch port behavior, and notification/permission paths where regressions could affect approvals or duplicate/missed alerts; mitigated by focused tests and explicit notification ownership split.
Overview
This PR splits OpenCode integration so
cmux-feed.jsowns Feed events (permissions, plans, questions, Stop) and completion-style notifications, whilecmux-session.jsstays limited to session restore and status—session hooks no longer emit generic stop notifications.The feed plugin now maps OpenCode events into canonical
tool_input(including permission kinds like bash/edit/doom loop), tracks assistant text from message parts/deltas for Stop payloads, and hardens the cmux socket client with outboxes and reconnect behavior. CLI install/OMO shadow config registers both managed plugins and only setsOPENCODE_PORTwhen the user or env explicitly provides a port (no more implicit default/fallback binding).On the app side, Feed shows richer OpenCode permission/plan previews, source-aware Stop reply UI and markdown where appropriate, and privacy-safe localized notification summaries. OpenCode Stop telemetry can post a tab notification (title-only when there is no assistant text). New en/ja strings back the UI and notification copy.
Tests cover plugin registration idempotency, feed plugin socket/Stop/permission shape, and notification body generation.
Reviewed by Cursor Bugbot for commit 4b3642d. Bugbot is set up for automated code reviews on this repo. Configure here.