Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive documentation for the OmniRoute plugin system, including guides for plugin development, the marketplace, and the SDK. While the documentation is highly detailed, several critical discrepancies between the documented features and the actual codebase implementation were identified. Specifically, the documentation references unsupported routing and stream events, incorrect code snippets for dev mode, outdated file line counts, and undocumented TypeScript interfaces for onPluginMessage and onRender. Additionally, the documented db and ipc permissions are missing from the PermissionSchema Zod enum, and the onPluginMessage and onRender events are not registered in BUILTIN_EVENTS. These inconsistencies should be resolved to ensure the documentation is accurate and does not mislead developers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Optional routing events | ||
| onModelSelect: async (ctx) => { | ||
| /* ... */ | ||
| }, | ||
| onComboResolve: async (ctx) => { | ||
| /* ... */ | ||
| }, | ||
| onRateLimit: async (ctx) => { | ||
| /* ... */ | ||
| }, | ||
| onQuotaExhaust: async (ctx) => { | ||
| /* ... */ | ||
| }, | ||
| onProviderError: async (ctx) => { | ||
| /* ... */ | ||
| }, | ||
|
|
||
| // Stream events | ||
| onStreamStart: async (ctx) => { | ||
| /* ... */ | ||
| }, | ||
| onStreamEnd: async (ctx) => { | ||
| /* ... */ | ||
| }, |
There was a problem hiding this comment.
The routing and stream events listed here (such as onModelSelect, onComboResolve, onRateLimit, onQuotaExhaust, onProviderError, onStreamStart, and onStreamEnd) are currently not supported or registered by the PluginManager (src/lib/plugins/manager.ts) or the loadPlugin function (src/lib/plugins/loader.ts). Only onRequest, onResponse, onError, and the lifecycle hooks (onInstall, onActivate, onDeactivate, onUninstall) are actually loaded and registered. Documenting these unsupported events can mislead developers.
|
|
||
| watcher = watch(pluginDir, { recursive: true }, (_eventType, filename) => { | ||
| if (!filename) return; | ||
| const pluginName = filename.replace(/\\/g, "/").split("/")[0]; |
There was a problem hiding this comment.
The code snippet shown here does not match the actual implementation in src/lib/plugins/devMode.ts. In the codebase, filename.split("/")[0] is used directly without replacing backslashes (which might cause issues on Windows). The documentation should be updated to match the actual code, or the code itself should be updated to handle Windows path separators.
| const pluginName = filename.replace(/\\/g, "/").split("/")[0]; | |
| const pluginName = filename.split("/")[0]; |
| ─────── ─────────────── | ||
| Local seed Remote registry | ||
| registry with ratings, | ||
| (3 entries) downloads, signed |
There was a problem hiding this comment.
The diagram mentions (3 entries) for the Phase 1 local seed registry, but the registry actually ships with 4 verified plugins (as documented on line 36 and implemented in src/lib/plugins/marketplace.ts). This should be updated to (4 entries) for accuracy.
| (3 entries) downloads, signed | |
| (4 entries) downloads, signed |
|
|
||
| | File | Lines | Purpose | | ||
| | -------------------------------- | ----- | ------------------------------------- | | ||
| | `src/lib/plugins/marketplace.ts` | 107 | Marketplace data + API | |
There was a problem hiding this comment.
The line counts listed in this table are outdated compared to the actual files in the repository:\n- marketplace.ts has 203 lines (not 107)\n- manager.ts has 569 lines (not 410+)\n- loader.ts has 327 lines (not 280+)\n- manifest.ts has 214 lines (not 200+)\n- signing.ts has 35 lines (not 34)\n\nConsider updating these numbers or removing the exact line counts to avoid maintaining outdated documentation.
| - `onPluginMessage` (function, optional) — Receives IPC messages from other plugins via `__omniroute.broadcast()` / `__omniroute.sendTo()` | ||
| - `onRender` (function, optional) — Renders a dashboard page when a user visits the plugin's admin page |
There was a problem hiding this comment.
The onPluginMessage and onRender functions are documented as optional parameters for definePlugin, but they are not defined in PluginDefinition or Plugin interfaces in src/lib/plugins/sdk.ts and src/lib/plugins/hooks.ts. Passing them to definePlugin will result in TypeScript compilation errors. Please update the documentation or the TypeScript definitions to align them.
| | `db` | `__omniroute.db` — persistent key-value store (SQLite-backed, isolated per plugin) | | ||
| | `ipc` | `__omniroute.broadcast()` / `__omniroute.sendTo()` — cross-plugin messaging | |
There was a problem hiding this comment.
The "db" and "ipc" permissions are listed in the permission system table, but they are not included in the PermissionSchema Zod enum in src/lib/plugins/manifest.ts. Any plugin declaring "db" or "ipc" in its plugin.json manifest will fail validation during installation/activation. Please add "db" and "ipc" to the PermissionSchema in src/lib/plugins/manifest.ts or correct the documentation if they are not intended to be declared as permissions.
| | `onPluginMessage` | IPC message from another plugin | `{ source, event, data }` | | ||
| | `onRender` | Dashboard page requested | `{ slug, params }` — return HTML or structured content | |
There was a problem hiding this comment.
The onPluginMessage and onRender events are listed under the Built-in Events table, but they are not defined in the BUILTIN_EVENTS array in src/lib/plugins/hooks.ts. As a result, they are unrecognized by the hooks system and will never be triggered. Please ensure these events are added to BUILTIN_EVENTS and properly registered in the plugin loader/manager.
|
Thanks @oyi77 — heads up: #4006 and #4007 have identical diffs (both add all three docs: PLUGIN_DEVELOPMENT / PLUGIN_MARKETPLACE / PLUGIN_SDK). Please consolidate into a single PR. Same two blockers apply to both (see my note on #4006): (1) |
Status updateVerified that the latest commits on this branch have addressed the review feedback:
Remaining blocker: Needs rebase onto current |
74d6c6c to
5b8dc8a
Compare
|
Consolidated from #4006 (now closed). Changes pushed:
|
b80c346 to
11f632c
Compare
|
Thanks @oyi77 for the documentation work 🙏 This PR is conflicting against |
11f632c to
c8845dd
Compare
|
Rebased onto latest |
c8845dd to
dfd806f
Compare
dfd806f to
1c06d4d
Compare
1c06d4d to
1c6e922
Compare
1c6e922 to
bdfc430
Compare
bdfc430 to
d07f2d0
Compare
d07f2d0 to
656be22
Compare
656be22 to
83d156d
Compare
|
Thanks for this @oyi77 — leaving it open for a rework rather than merging as-is, because a review against the current
Heads-up that the plugin system is heading into a redesign, so a rebased, deduped, accuracy-checked version is the way to land this cleanly. Happy to re-review once it's updated — appreciate the docs effort! 🙏 |
83d156d to
754cecf
Compare
…C+D) (cheaper-inference#4622) C — scripts/quality/validate-release-green.mjs (npm run check:release-green): reproduces the release-equivalent validation (typecheck, eslint, db-rules, public-creds, full unit, vitest, ratchets, optional --with-build package-artifact) against the current working tree and classifies each red as HARD (real defect, exit 1) vs DRIFT (ratchet — reported, never affects exit / never blocks). Pure helpers exported + orchestration behind a direct-run guard; unit-tested. D — .github/workflows/nightly-release-green.yml: runs C on the active release branch nightly (and on workflow_dispatch) and opens/updates a single tracking issue on HARD failures. Never a required check, never touches a contributor PR. Closes the gap where the full gate (ci.yml) only ran on the release PR, so reds accrued silently on release/** and surfaced in 40-min layers at release time. Non-blocking by construction; drift is the maintainer's to rebaseline at release. Co-authored-by: Diego Rodrigues de Sa e Souza <diego.souza@cdwasolutions.com.br>
410b690 to
808e7f3
Compare
|
Closing stale PR — conflicts with current release branch. Will reopen fresh if needed. |
Replaces #3787 as a clean, non-contaminated PR branched from release/v3.8.27.
Adds comprehensive plugin documentation:
Only docs/plugins/ changes, no unrelated churn.
Supersedes: #3787