Skip to content

refactor(notify): bundle KDCO opencode-notify and remove built-in session-notification#3520

Closed
kdcokenny wants to merge 8 commits intodevfrom
feature/externalize-session-notification
Closed

refactor(notify): bundle KDCO opencode-notify and remove built-in session-notification#3520
kdcokenny wants to merge 8 commits intodevfrom
feature/externalize-session-notification

Conversation

@kdcokenny
Copy link
Copy Markdown
Collaborator

@kdcokenny kdcokenny commented Apr 19, 2026

Summary

Replaces oh-my-opencode's built-in session-notification subsystem with a bundled KDCO opencode-notify integration shipped inside the package. OMO now enforces a single canonical bundled notify owner in OpenCode plugin config while keeping background-notification behavior unchanged.

Changes

  • Removed built-in session-notification runtime, hook wiring, and legacy config surface (notification.force_enable)
  • Added bundled notify artifact under dist/opencode-notify with third-party notice file
  • Added bootstrap/ownership migration logic to ensure exactly one user-scope bundled notify plugin entry
  • Added deterministic migration handling for recognized kdco/notify entries and loud-fail behavior for custom/unsafe notify entries
  • Aligned idle ready-notification suppression with todo auto-continuation semantics (does not suppress for blocked/deleted only)
  • Updated docs/install guidance for bundled ownership model

Testing

  • bun run typecheck passes
  • Targeted tests for bundled ownership, notify suppression semantics, and touched runtime paths pass
  • bun run build passes and emits bundled notify artifact + notice files
  • Full bun test still has unrelated pre-existing failures in slash-command/auto-slash-command areas (no new regressions introduced)

Migration Notes

Existing recognized kdco/notify entries are migrated to the bundled owner automatically. Custom/unsafe notify entries are not rewritten and fail loudly with cleanup guidance to avoid duplicate owners.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 37 files

Confidence score: 5/5

  • This looks low risk to merge: the only noted issue is a documentation consistency mismatch, not a runtime or user-facing behavior change.
  • In AGENTS.md, the top-level “51 hooks” summary conflicts with the tier breakdown total of 52, which could confuse maintainers but should not affect functionality.
  • Pay close attention to AGENTS.md - reconcile the hook totals so the summary and breakdown stay aligned.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="AGENTS.md">

<violation number="1" location="AGENTS.md:7">
P3: Keep the hook counts in this doc aligned. The 51-hook summary conflicts with the hook-tier breakdown below, which still totals 52.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread AGENTS.md
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.

@kdcokenny kdcokenny changed the title refactor(hooks): remove built-in session-notification subsystem in favor of KDCO opencode-notify refactor(notify): bundle KDCO opencode-notify and remove built-in session-notification Apr 19, 2026
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

4 issues found across 17 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/shared/bundled-notify-ownership.ts">

<violation number="1" location="src/shared/bundled-notify-ownership.ts:73">
P1: Broad `notify` substring matching can misclassify unrelated plugins as unsafe and hard-fail bootstrap.</violation>

<violation number="2" location="src/shared/bundled-notify-ownership.ts:101">
P1: Treat stale bundled `file://.../dist/opencode-notify` entries as migratable instead of throwing on them.</violation>
</file>

<file name="postinstall.mjs">

<violation number="1" location="postinstall.mjs:140">
P1: This bootstrap imports a JS file that the current build never emits, so bundled notify ownership setup silently never runs.</violation>
</file>

<file name="src/bundled-opencode-notify/index.ts">

<violation number="1" location="src/bundled-opencode-notify/index.ts:85">
P2: Do not treat todo-fetch failures as "ready" here; a transient error can send a false idle notification while incomplete todos still exist.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread src/shared/bundled-notify-ownership.ts Outdated
Comment thread postinstall.mjs
Comment thread src/shared/bundled-notify-ownership.ts Outdated
Comment thread src/bundled-opencode-notify/index.ts Outdated
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 7 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/shared/bundled-notify-ownership.ts">

<violation number="1" location="src/shared/bundled-notify-ownership.ts:127">
P1: This only marks path-based notify plugins as unsafe, so custom notify package IDs now slip through as `other` and remain beside the bundled owner.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread src/shared/bundled-notify-ownership.ts
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/shared/bundled-notify-ownership.ts">

<violation number="1" location="src/shared/bundled-notify-ownership.ts:91">
P2: npm alias specs like `alias@npm:@custom/opencode-notify@1.2.3` bypass this unsafe-entry check, so a custom notify owner can survive migration and coexist with the bundled owner.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread src/shared/bundled-notify-ownership.ts Outdated
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/shared/bundled-notify-ownership.ts">

<violation number="1" location="src/shared/bundled-notify-ownership.ts:92">
P2: Aliased `kdco/notify` specs still bypass migration and duplicate-owner checks.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread src/shared/bundled-notify-ownership.ts
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/shared/bundled-notify-ownership.ts">

<violation number="1" location="src/shared/bundled-notify-ownership.ts:79">
P2: This regex rejects valid npm dist-tags like `release_candidate`, so recognized `kdco/notify@<tag>` entries can now fail bootstrap instead of being migrated.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread src/shared/bundled-notify-ownership.ts Outdated
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

0 issues found across 2 files (changes from recent commits).

Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.

@kdcokenny kdcokenny closed this Apr 20, 2026
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