refactor(plugin): independent per-load registration with marker-based bootstrap idempotency#352
Conversation
… bootstrap idempotency Each call to SystematicPlugin now runs initializePlugin independently and returns its own hooks surface. With multiple OpenCode plugin sources configured (project + user config), each source gets its own systematic_skill tool, its own config handler, and its own chat.system.transform closure. hasLoggedInit moves into per-init closure state, so the process emits N init log lines for N registrations — honest signal that init ran N times. Bootstrap content idempotency is now enforced at injection time, not init time. applyBootstrapContent walks output.system for any prior <SYSTEMATIC_WORKFLOWS>...</SYSTEMATIC_WORKFLOWS> block (non-greedy regex) and replaces it in-place; missing-block falls through to append. Under OpenCode's verified FIFO hook iteration, the last transform to run owns the final block — most-recently-registered plugin wins, which matches the project-after-user load order developers expect. This reverts the plugInOnce singleton from PR #335, which turned out to be over-correction. OpenCode registers tools per-source even with a shared hooks reference, so the singleton's init-dedup had no visible effect on the TUI tool catalog — what it did do in dev setups was silently collapse all loads onto whichever ran first, shadowing later sources. Also removes _resetPluginSingleton from the integration test setup; the singleton itself is deleted in a follow-up commit. 12 new behavior tests cover marker-replacement in every slot, non-greedy boundary, multi-block edge cases, per-invocation distinct references, and the cross-registration integration scenario.
The plugInOnce abstraction and its test file are no longer reachable — src/index.ts stopped importing them in the prior commit, and the consumer test files (plugin.test.ts and opencode.test.ts) dropped the _resetPluginSingleton reset calls. Deletes 227 lines of now-dead infrastructure: the singleton helper, its five test cases, and the module-level exports. The architectural rationale lives in docs/brainstorms/2026-05-10-multi-load-plugin-registration-requirements.md and the implementation plan at docs/plans/2026-05-10-002-refactor-multi-load-plugin-registration-plan.md.
Updates module count in AGENTS.md and src/lib/AGENTS.md from 16/14 to 15 (post-deletion of plugin-singleton.ts). Appends a 2026-05-10 follow-up section to the PR #335 solution doc noting the singleton was removed and documenting marker-based idempotency as the current correctness contract. Also commits the implementation plan with all four units marked complete. The plan was untracked locally during execution per project convention; landing it here gives the architectural inversion a visible paper trail.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fro-bot
left a comment
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
This is a clean, well-tested architectural inversion that correctly reverts the over-corrected singleton pattern from PR #335. The marker-based bootstrap idempotency model is simpler, more honest about multi-source behavior, and leaves no dead code behind.
Blocking issues
None.
Non-blocking concerns
-
Named export from plugin entry point.
src/index.tsnow exportsapplyBootstrapContentas a named export alongside the defaultSystematicPlugin.src/lib/bootstrap.tscarries an explicit comment that "additional named exports break loading" in OpenCode's plugin loader. Integration tests pass today (25/25), so current OpenCode handles it correctly, but this is the first named export from the entry point and introduces fragility against loader changes. Consider extracting the helper to a separate internal module if the loader contract ever narrows. -
Anachronistic test name.
tests/unit/plugin.test.tsretains a test titledduplicate factory invocations return real hooks without warnings. The concept of "duplicate" invocations no longer exists post-singleton-removal — every invocation is independent. The test still validates a useful property (all calls return valid hooks and emit no warnings), but the name carries the old mental model.
Missing tests
None. The 12 new behavior tests comprehensively cover:
- Marker replacement in every array slot (empty, last, non-last)
- Non-greedy regex boundary (multi-block same-slot edge case)
- Sequential transform invocations leaving exactly one block with last-writer-wins
- Per-invocation distinct hook references (object + function identity)
- Cross-registration integration scenario (two plugins, same
output.system) - Per-init
hasLoggedInitscoping (two init log lines for two registrations) - Per-turn idempotency (one block per turn on fresh arrays)
Risk assessment
LOW — Single-source users see zero behavioral change. Multi-source users gain correct per-source registration instead of silent shadowing. The external contract (default export shape, hook names, tool name) is unchanged. Build, typecheck, and all 734 tests (709 unit + 25 integration) pass. Lint reports 0 errors in changed files.
The only elevated concern is the named-export-from-entry-point fragility, but current OpenCode loader behavior is verified by integration tests.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | marcusrbrown/systematic |
| Run ID | 25655747463 |
| Cache | hit |
| Session | ses_1ea1e2ef7ffedgq4Gey5wZKTl4 |
… ReDoS The non-greedy regex /<SYSTEMATIC_WORKFLOWS>[\s\S]*?<\/SYSTEMATIC_WORKFLOWS>/ was flagged by CodeQL as polynomial-time on uncontrolled input — when the opening tag repeats and the closing tag is absent, the engine backtracks through every prefix. With per-load registration now letting any plugin source contribute system prompt fragments, this regex sees content the plugin itself didn't author. Replaces the regex with a small indexOf/slice helper. Fixed literal delimiters never needed regex; the linear scan is provably immune to ReDoS and unchanged in behavior for the existing seven marker-replacement tests. Adds a regression test that runs the helper against 10000 repeated opening markers with no closing tag and asserts completion in well under 1s. Closes CodeQL alerts #42 and #43.
|
The two ReDoS alerts (#42, #43) on the marker regex are closed in Added a regression test that runs the helper against 10,000 repeated opening markers with no closing tag and asserts completion in well under 1s — the previous regex would have taken seconds-to-minutes on this input. All seven existing marker-replacement test scenarios still pass unchanged (the non-greedy semantics map cleanly to "first opening tag, then first closing tag after that position" via |
fro-bot
left a comment
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
The PR is now even stronger after the ReDoS fix. The marker-based bootstrap idempotency model replaces the over-corrected singleton from PR #335 with a simpler, more honest, and now provably safe linear-scan approach.
Blocking issues
None.
Non-blocking concerns
-
Named export from plugin entry point.
src/index.tsexportsapplyBootstrapContentas a named export alongside the defaultSystematicPlugin.src/lib/bootstrap.tscarries an explicit comment that "additional named exports break loading" in OpenCode's plugin loader. Integration tests pass today (25/25), so current OpenCode handles it correctly, but this is the first named export from the entry point and introduces fragility against loader changes. Consider extracting the helper to a separate internal module if the loader contract ever narrows. -
Anachronistic test name.
tests/unit/plugin.test.tsretains a test titledduplicate factory invocations return real hooks without warnings. The concept of "duplicate" invocations no longer exists post-singleton-removal — every invocation is independent. The test still validates a useful property, but the name carries the old mental model.
Missing tests
None. The 13 behavior tests (12 original + 1 ReDoS regression test) comprehensively cover:
- Marker replacement in every array slot (empty, last, non-last)
- Non-greedy boundary (multi-block same-slot edge case)
- Sequential transform invocations leaving exactly one block with last-writer-wins
- Per-invocation distinct hook references (object + function identity)
- Cross-registration integration scenario (two plugins, same
output.system) - Per-init
hasLoggedInitscoping (two init log lines for two registrations) - Per-turn idempotency (one block per turn on fresh arrays)
- ReDoS regression: linear-time completion on 10,000 repeated opening markers with no closing tag
Risk assessment
LOW — Single-source users see zero behavioral change. Multi-source users gain correct per-source registration instead of silent shadowing. The ReDoS fix eliminates a security concern from the original regex approach. External contract unchanged. Build, typecheck, and all 735 tests (710 unit + 25 integration) pass. Lint reports 0 errors in changed files.
The only elevated concern remains the named-export-from-entry-point fragility, but current OpenCode loader behavior is verified by integration tests.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | marcusrbrown/systematic |
| Run ID | 25656360204 |
| Cache | hit |
| Session | ses_1ea1e2ef7ffedgq4Gey5wZKTl4 |
…version (#353) Documents the CodeQL ReDoS finding that surfaced during PR #352's multi-load registration cycle. The marker regex itself never changed — the trust boundary around it did. Architectural changes that expand the set of input authors can reclassify a previously safe regex as unsafe. Documents the rejected alternatives (bounded-length cap, atomic groups, inline CodeQL suppression), the indexOf/slice solution, the regression test shape, and four prevention rules. Cross-references three adjacent learnings: the PR #335 origin doc that PR #352 reverted, the config overlay trust-boundary doc, and the agent-color schema regression.
… plugin load (#355) PR #352's per-load registration refactor added `export const applyBootstrapContent` to src/index.ts so unit tests could import it directly. OpenCode's plugin loader treats every named export from a plugin entry as an additional plugin factory and invokes each one with no arguments — `applyBootstrapContent` crashed on `output.system.length` and the loader aborted the whole plugin, including the intended `default` export. Users on v2.12.1 saw zero Systematic skills or slash commands; reverting to v2.12.0 restored everything. Same class as the v2.5.0 / PR #309 regression where `INTERNAL_AGENT_SIGNATURES` was exported from the entry. That fix moved the constant to src/lib/bootstrap.ts with a comment "must NOT be re-exported from the plugin entry point." The comment did not prevent PR #352 from exporting a different helper, because comments in a sibling file are invisible during entry-file edits. Three coordinated changes: 1. Move applyBootstrapContent, findBootstrapMarkerBlock, and the marker constants into src/lib/bootstrap.ts. The entry point now imports the helper. `Object.keys(dist/index.js)` is back to `['default']`. 2. Update tests/unit/plugin.test.ts import path to follow the symbol. 3. Harden the CI Node ESM smoke test to assert default-only exports with an explanatory error message. This is the structural gate that would have also caught the v2.5.0 regression, and now prevents the entire class for future PRs. Captures the pattern in docs/solutions/integration-issues/opencode-plugin-named-exports-break-loader-2026-05-11.md — including cross-references to the v2.5.0 incident, the PR #335 origin of `plugInOnce`, and the v2.9.2 agent-colors regression that shares the same "CI green → npm publish → user install breaks" failure mode.
…leton divergence rationale (#123) * refactor(plugin): move wireRpcServerCleanup out of the plugin entry OpenCode's plugin loader treats every named export from a plugin entry point as a separate plugin factory and invokes it with `undefined` input. The exported `wireRpcServerCleanup` was being called that way, which would have crashed on the first reference to its callback if the loader ever swept it up. Move the helper to `src/lib/rpc-cleanup.ts` (matching the existing `src/lib/` style alongside `ansi.ts`, `errno.ts`, `kill-tree.ts`, `normalize-tool-arg-schemas.ts`). The plugin entry now imports it internally and re-exports nothing beyond `default`. Same lesson Systematic learned in v2.5.0 (PR #309) and again in v2.12.1/v2.12.2 (PR #355): plugin entries must expose `export default` only. Helpers belong in `src/lib/`. No behavior change — the helper's identity-once semantics and its single caller are byte-identical. Sets the stage for the Node-loadable build (next commit) and the CI export-shape gate (subsequent commit). * refactor(build): switch plugin entry build target from bun to node `scripts/build.ts` was emitting `dist/index.js` with `target: 'bun'`, which produces Bun-only code like `__require = import.meta.require`. OpenCode runs Bun at runtime so the plugin still loaded, but the build artifact would not load under plain Node ESM. That blocked the natural CI export-shape gate that requires `node --input-type=module -e "import('./dist/index.js')"` to succeed (next commit). The plugin entry now builds with `target: 'node'`. The TUI entry stays on `target: 'bun'` because `@opentui/solid` is Bun-specific. Pre-switch grep audit confirmed zero production uses of Bun-only APIs (`Bun.spawn` / `Bun.file` / `Bun.serve` / `Bun.build`) in `src/` — production code is Bun-API-free; the build target switch is safe. * ci(test): assert plugin entry exports only default Institutionalize the export-shape contract so future PRs cannot reintroduce the regression class. Two coordinated surfaces: - `tests/package-exports.test.ts` extends the existing build-then-assert pattern with a new case that imports `dist/index.js` under Node-ESM semantics and verifies `Object.keys(m).sort() === ['default']` and `typeof m.default === 'function'`. The error message points at this class of bug so future contributors find the rationale. - `.github/workflows/ci.yaml` adds a `Node ESM export-shape smoke test` step in the `check` job between `Build` and `Unit tests`. Same assertion as the local test; defense in depth so the gate runs even if unit tests are skipped or fail for unrelated reasons. Requires Node so the workflow now sets up Node 22 alongside the existing mise toolchain (matches `release.yaml`'s pin of `actions/setup-node@v6.4.0`). Sentinel teeth-check verified: temporarily adding `export const __testSentinel = () => {}` to `src/index.ts` and rebuilding makes the new test fail with the documented error message; removing the sentinel returns the suite to green. * docs(plugin): explain the singleton divergence from Systematic PR #352 Capture why this plugin retains `plugInOnce` (the first-wins singleton pattern) even though Systematic replaced that same pattern with per-load registration in PR #352. The divergence is justified: Systematic's plugins are stateless and idempotent — they register tools and inject prompts. Running the same factory twice in the same process produces the same registrations as once. This plugin's `doInit` binds a TCP port and writes a PID file. A second `doInit` call in the same process would race on those exclusive resources (EADDRINUSE on the port, double-entry in the PID file). `src/runtime/plugin-singleton.ts` and `src/lib/rpc-cleanup.ts` each get top-of-file JSDoc explaining the constraint with concrete cross- references to marcusrbrown/systematic#352. Doc-only change. No code touched. Existing `tests/plugin-singleton.test.ts` coverage continues to assert the singleton behavior unchanged. Also flips the plan doc status to `completed` and ticks Units 2-5. * fix(plugin): address Fro Bot review feedback on PR #123 Apply the blocking finding and one of two non-blocking concerns: - **`process.once` for both `beforeExit` and `SIGTERM` (was: `on` for beforeExit, `once` for SIGTERM).** The asymmetry meant `beforeExit` could in principle re-fire while `closeRpcServer()` was still pending and re-enter `closeOnce` before `closePromise` was assigned. The race was very narrow (mitigated almost entirely by `closePromise` being a module-level `let`) but the idiomatic fix is to remove the inconsistency. Both handlers now use `process.once`. The `process.off` calls in the `.finally()` block become belt-and-suspenders no-ops, kept cheap. - **CI Node version bumped 22 → 24 to match `release.yaml`.** Fro Bot correctly noted the original PR description claimed the pin "matched release.yaml" — it did not; release pins Node 24. Bumping the CI pin to 24 makes the export-shape smoke run against the same Node major the release artifact ships against, removing a pointless inconsistency. Adds a new `tests/rpc-cleanup.test.ts` with two cases: - A direct idempotency test (`closeOnce` × 3 → 1 call). - A re-entrancy regression test: registers the helper, captures the installed `beforeExit` listener, fires `process.emit('beforeExit', 0)` three times synchronously, and asserts the underlying close runs exactly once. The test also verifies the listener is gone after the first emit (`process.once` semantics) — guard against future drift back to `process.on`. Non-applied: Fro Bot's second non-blocking concern (`onSigterm` could hang if `closeRpcServer` never resolves). That's a pre-existing design choice noted in the README and not introduced here. Worth a future PR that adds a `setTimeout` deadline to the close path.
After several major architectural arcs (CEP divorce, plugin singleton removal, content-integrity gate, provider-availability v2.13.0/v2.14.3/v2.14.4) the docs/solutions/ corpus accumulated drift. Performed a full audit and refreshed 21 docs. Actions taken: - Updated 6 docs to remove dead references to deleted CEP-sync infrastructure and add last_refreshed dates - Deleted 2 already-archived docs whose lessons are obsolete: structured-manual-override-tracking and batch-import-cep-agents - Replaced 2 docs whose subject was reversed or deleted: opencode-plugin-factory-duplicate-registration now documents the current per-load registration + marker-based bootstrap model (PR #352 reversed the singleton); sync-cep-missing-sub-files generalizes to a multi-file batch import integrity pattern enforced by scripts/content-integrity.ts - Consolidated discovery-before-validation-lifecycle (3 patterns) into provider-availability-source-defaults (now 9 patterns), since the newer doc was a refinement of the older's discriminated envelope plus two additional patterns The 11 docs not touched were verified current against shipped behavior.
3 participants
Why
With multiple OpenCode plugin sources configured (e.g., project config
./src/index.tsalongside user config@fro.bot/systematic), PR #335'splugInOncesingleton collapsed all loads onto whichever ran first — silently shadowing the live source being edited. The duplicate-tool-entry concern that motivated the singleton turned out to be a non-issue: OpenCode registers tools per-source regardless of whether the hooks reference is shared, so the singleton's init-dedup had no visible effect on the TUI tool catalog. What it did do was hide the second registration entirely.What
Each call to
SystematicPluginnow runsinitializePluginindependently and returns its own hooks surface. With two sources configured, each gets its ownsystematic_skilltool, its own config handler, and its ownchat.system.transformclosure.hasLoggedInitmoves into per-init closure state, so the process emits two init log lines for two registrations — honest signal that init ran twice.Bootstrap content idempotency is now enforced at injection time, not init time.
applyBootstrapContentwalksoutput.systemfor any prior<SYSTEMATIC_WORKFLOWS>...</SYSTEMATIC_WORKFLOWS>block (non-greedy regex) and replaces it in-place; missing-block falls through to append. Under OpenCode's verified-FIFO hook iteration, the last transform to run owns the final block — most-recently-registered plugin wins, which matches the project-after-user load order developers expect.Behavioral change
Users with one plugin source see no change.
Diff shape
src/lib/plugin-singleton.ts(plugInOnce+_resetPluginSingleton) and its test file — 227 LOC of now-dead infrastructure.src/index.ts, exported for direct testing.src/lib/AGENTS.mdmodule count synced (16→15); rootAGENTS.mdcount synced; PR fix(plugin): deduplicate factory registration across opencode.json sources #335's solution doc gets a dated follow-up.docs/plans/2026-05-10-002-refactor-multi-load-plugin-registration-plan.mdfor the architectural-decision paper trail.Net: +275 / −258 across 8 files.
Testing
Compatibility
External contract unchanged. Same default export shape, same hook names, same
systematic_skilltool name. v2.13.0 minor — bug fix + small architectural inversion, no breaking changes to documented surface.Risks
output.systemper hook invocation: would break cross-registration coordination. Verified during planning that the same object reference is passed today (packages/opencode/src/session/llm.ts).<SYSTEMATIC_WORKFLOWS>tag: no current emitters in OpenCode core or other plugins. If one appears, the marker would over-match; documented as scope boundary.