Fix OpenCode review transitions and notification filtering#122
Fix OpenCode review transitions and notification filtering#122rookedsysc wants to merge 2 commits intodevfrom
Conversation
Greptile SummaryThis PR fixes two notification-related edge cases: it applies the Key changes:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[OpenCode event fires] --> B[Extract sessionID via getSessionID]
B --> C{isMainSession?}
C -- No --> Z[Return — ignore event]
C -- Yes --> D{eventType?}
D -- message.updated --> E{message.role === 'user'?}
E -- Yes --> F[updateStatus: progress]
E -- No --> G{isAssistantMessageCompleted?}
G -- Yes --> H[updateStatus: review]
G -- No --> Z2[Return — no-op]
D -- question.asked --> I[updateStatus: pending]
D -- question.replied --> J[updateStatus: progress]
D -- session.idle --> K[updateStatus: review]
subgraph isAssistantMessageCompleted
L{role === 'assistant'?} -- No --> M[return false]
L -- Yes --> N["Boolean(stopReason ?? stop_reason ?? completedAt ?? completed ?? isComplete ?? isCompleted ?? status==='completed')"]
end
G --> L
Last reviewed commit: bf1c771 |
| function isAssistantMessageCompleted(message: OpenCodeMessage | undefined): boolean { | ||
| if (message?.role !== "assistant") { | ||
| return false; | ||
| } | ||
|
|
||
| return Boolean( | ||
| message?.stopReason ?? | ||
| message?.stop_reason ?? | ||
| message?.completedAt ?? | ||
| message?.completed ?? | ||
| message?.isComplete ?? | ||
| message?.isCompleted ?? | ||
| message?.status === "completed" | ||
| ); |
There was a problem hiding this comment.
Potential premature review on tool-call stop reasons
isAssistantMessageCompleted returns true for any non-null, non-undefined value of stopReason / stop_reason. In typical LLM workflows (and common OpenCode internals), when the model pauses to invoke a tool, a message.updated event fires with stop_reason: "tool_use" even though the session is not done — the model still needs to receive the tool result and continue. As a result, a tool-call pause would satisfy this check and prematurely move the task to review.
Consider restricting to known completion stop reasons, or gating on additional session-level signals:
const COMPLETION_STOP_REASONS = new Set(["end_turn", "stop_sequence", "max_tokens"]);
function isAssistantMessageCompleted(message: OpenCodeMessage | undefined): boolean {
if (message?.role !== "assistant") {
return false;
}
const stopReason = message?.stopReason ?? message?.stop_reason;
if (stopReason != null) {
return COMPLETION_STOP_REASONS.has(stopReason);
}
return Boolean(
message?.completedAt ??
message?.completed ??
message?.isComplete ??
message?.isCompleted ??
(message?.status === "completed")
);
}If OpenCode always uses specific stop reason strings for tool calls, this safeguards against false review transitions mid-session.
| return Boolean( | ||
| message?.stopReason ?? | ||
| message?.stop_reason ?? | ||
| message?.completedAt ?? | ||
| message?.completed ?? | ||
| message?.isComplete ?? | ||
| message?.isCompleted ?? | ||
| message?.status === "completed" | ||
| ); |
There was a problem hiding this comment.
Explicit parentheses for operator-precedence clarity
message?.status === "completed" sits at the tail of a ?? chain without parentheses. JavaScript's operator-precedence rules do evaluate === before ??, so the logic is technically correct — but a reader unfamiliar with this quirk may assume the whole chain short-circuits and only the final boolean participates in ??. Wrapping it explicitly removes all ambiguity.
| return Boolean( | |
| message?.stopReason ?? | |
| message?.stop_reason ?? | |
| message?.completedAt ?? | |
| message?.completed ?? | |
| message?.isComplete ?? | |
| message?.isCompleted ?? | |
| message?.status === "completed" | |
| ); | |
| return Boolean( | |
| message?.stopReason ?? | |
| message?.stop_reason ?? | |
| message?.completedAt ?? | |
| message?.completed ?? | |
| message?.isComplete ?? | |
| message?.isCompleted ?? | |
| (message?.status === "completed") | |
| ); |
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!
| function getSessionID(event: OpenCodeEvent): string | undefined { | ||
| return ( | ||
| event?.properties?.sessionID ?? | ||
| event?.properties?.sessionId ?? | ||
| event?.properties?.message?.sessionID ?? | ||
| event?.properties?.message?.sessionId ?? | ||
| event?.properties?.info?.sessionID ?? | ||
| event?.properties?.info?.sessionId | ||
| ); | ||
| } | ||
|
|
||
| function getMessage(event: OpenCodeEvent): OpenCodeMessage | undefined { | ||
| return event?.properties?.info ?? event?.properties?.message; | ||
| } |
There was a problem hiding this comment.
Session ID priority changed for message.updated events
The original code extracted the session ID from the nested message object (message.sessionID) for message.updated events. getSessionID now checks event?.properties?.sessionID first, before falling back to event?.properties?.message?.sessionID.
If OpenCode's message.updated payload always puts the same ID at both levels this is harmless, but if properties.sessionID refers to a different scope (e.g., a parent or routing ID) while properties.message.sessionID holds the actual message's session, the isMainSession check would use the wrong value and could incorrectly allow or suppress sub-agent events.
It is worth confirming whether OpenCode ever populates properties.sessionID on message.updated events and, if so, whether it is always equal to properties.message.sessionID.
Related References
What is this PR?
dev.How was it solved?
Filter missing-target notifications by enabled status
enabledStatuseswhen handlinghook-status-target-missingevents.Move OpenCode sessions to review on assistant completion
message.updatedpayloads and map them toreview.Important Changes
Notification filtering
Skip disabled missing-target alerts
src/components/NotificationListener.tsx,src/components/__tests__/NotificationListener.test.tsxOpenCode review transitions
Treat completed assistant updates as review-ready
reviewas soon as the assistant finishes, without waiting forsession.idle.src/lib/openCodeHooksSetup.ts,src/lib/__tests__/openCodeHooksSetup.test.tsSupport alternate OpenCode payload shapes
sessionID/sessionIdandinfo/messagepayloads consistently during main-session checks.src/lib/openCodeHooksSetup.ts,src/lib/__tests__/openCodeHooksSetup.test.ts