Skip to content

Fix OpenCode review transitions and notification filtering#122

Closed
rookedsysc wants to merge 2 commits intodevfrom
release/dev-to-main-20260316-195431
Closed

Fix OpenCode review transitions and notification filtering#122
rookedsysc wants to merge 2 commits intodevfrom
release/dev-to-main-20260316-195431

Conversation

@rookedsysc
Copy link
Copy Markdown
Owner

Related References

  • None

What is this PR?

  • Fix two hook-notification edge cases before merging back into dev.

How was it solved?

Filter missing-target notifications by enabled status

  • Reuse enabledStatuses when handling hook-status-target-missing events.
  • Update the listener test so disabled statuses stay silent.

Move OpenCode sessions to review on assistant completion

  • Detect completed assistant message.updated payloads and map them to review.
  • Normalize session and message extraction across event payload variants while keeping main-session filtering intact.
  • Extend the generated plugin tests to cover the new completion detection logic.

Important Changes

Notification filtering

Skip disabled missing-target alerts

  • Why: prevent notifications for statuses the user explicitly turned off.
  • Modified files: src/components/NotificationListener.tsx, src/components/__tests__/NotificationListener.test.tsx

OpenCode review transitions

Treat completed assistant updates as review-ready

  • Why: move tasks into review as soon as the assistant finishes, without waiting for session.idle.
  • Modified files: src/lib/openCodeHooksSetup.ts, src/lib/__tests__/openCodeHooksSetup.test.ts

Support alternate OpenCode payload shapes

  • Why: handle both sessionID/sessionId and info/message payloads consistently during main-session checks.
  • Modified files: src/lib/openCodeHooksSetup.ts, src/lib/__tests__/openCodeHooksSetup.test.ts

@rookedsysc rookedsysc self-assigned this Mar 16, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Mar 16, 2026

Greptile Summary

This PR fixes two notification-related edge cases: it applies the enabledStatuses filter to hook-status-target-missing events (previously only task-status-changed was filtered), and it refactors the OpenCode plugin generator to transition tasks to review as soon as the assistant completes a message rather than waiting for session.idle.

Key changes:

  • NotificationListener.tsx: hook-status-target-missing events now respect enabledStatuses, preventing unwanted notifications for disabled statuses.
  • openCodeHooksSetup.ts: The plugin now defines typed helpers (OpenCodeEvent, OpenCodeMessage, getSessionID, getMessage, isAssistantMessageCompleted) and consolidates the main-session guard to a single early-return, simplifying all event branches.
  • isAssistantMessageCompleted detects completion via a wide ?? chain (stopReason, stop_reason, completedAt, completed, isComplete, isCompleted, status === "completed"), which could produce a false-positive review transition if OpenCode emits a message.updated event with a non-null stopReason during a tool-call pause (e.g., "tool_use").
  • The session ID extraction order in getSessionID differs from the original code for message.updated events (top-level properties.sessionID now takes priority over the nested message.sessionID), which is worth verifying against the OpenCode event schema.
  • session.idle still also triggers review, so double API calls will occur for every session completion.

Confidence Score: 3/5

  • Mostly safe to merge, but the assistant-completion detection in the generated plugin carries a risk of premature review transitions during tool-call pauses that should be verified against the actual OpenCode event schema.
  • The NotificationListener change is clean and low-risk. The OpenCode plugin refactor is well-structured, but isAssistantMessageCompleted accepts any non-null stopReason/stop_reason, which would match tool-call stop reasons (e.g., "tool_use") and potentially move tasks to review mid-session. The session ID extraction priority change for message.updated events is a silent behavioral shift that could affect sub-agent filtering. Both issues are confined to the generated plugin script and would only manifest at runtime with a live OpenCode instance.
  • Pay close attention to src/lib/openCodeHooksSetup.ts, specifically the isAssistantMessageCompleted function and the getSessionID priority order.

Important Files Changed

Filename Overview
src/lib/openCodeHooksSetup.ts Refactored OpenCode plugin generation: centralized session ID extraction and message extraction into helpers, moved the main-session guard to a single early-return gate, and added isAssistantMessageCompleted to trigger review on assistant message completion. The new function may produce false positives if OpenCode fires message.updated with a tool-call stopReason (e.g., "tool_use") before the session is truly finished; also the session ID extraction priority has changed for message.updated events.
src/components/NotificationListener.tsx Straightforward fix: hook-status-target-missing events now check enabledStatuses before firing a notification, mirroring the existing filter already applied to task-status-changed events.
src/lib/tests/openCodeHooksSetup.test.ts Tests updated to assert generated-plugin content matches new helpers (getSessionID, getMessage, isAssistantMessageCompleted) and the unified early-return session guard; a new dedicated test covers assistant-completion detection. Coverage is file-content pattern matching, which is appropriate for a code-generation function, but there is no test verifying that intermediate tool-call messages do NOT trigger review.
src/components/tests/NotificationListener.test.tsx Test description corrected and assertion flipped to verify that notifyHookStatusTargetMissing is NOT called when requestedStatus is absent from enabledStatuses, accurately reflecting the new behavior.

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
Loading

Last reviewed commit: bf1c771

Comment on lines +65 to +78
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"
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +70 to +78
return Boolean(
message?.stopReason ??
message?.stop_reason ??
message?.completedAt ??
message?.completed ??
message?.isComplete ??
message?.isCompleted ??
message?.status === "completed"
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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!

Comment on lines +50 to +63
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;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rookedsysc rookedsysc closed this Apr 12, 2026
@rookedsysc rookedsysc deleted the release/dev-to-main-20260316-195431 branch April 12, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant