diff --git a/.github/scripts/check-pr-linked-issue.mjs b/.github/scripts/check-pr-linked-issue.mjs deleted file mode 100644 index 865b8865..00000000 --- a/.github/scripts/check-pr-linked-issue.mjs +++ /dev/null @@ -1,53 +0,0 @@ -#!/usr/bin/env node -/** - * check-pr-linked-issue.mjs - * Checks that a PR body references a GitHub issue. Respects conventional commit - * prefixes — skips check for docs/chore/build/ci/style/test prefixed PRs. - * Export: checkLinkedIssue(prBody: string, prTitle: string) → { passed, failures } - */ -import { fileURLToPath } from 'node:url'; - -const ISSUE_PATTERNS = [ - /(?:fixes|closes|resolves)\s+#\d+/i, - /(?:^|[\s(])https:\/\/github\.com\/paperclipai\/paperclip\/issues\/\d+(?=$|[\s),:;!?]|[.](?![\w-]))/i, - /(? p.test(body)); - return { - passed: found, - failures: found ? [] : [ - 'No linked issue found — please add `Fixes #NNN` to your PR description. ' + - 'If no issue exists yet, please file one first: ' + - 'https://github.com/paperclipai/paperclip/issues/new', - ], - }; -} - -if (process.argv[1] === fileURLToPath(import.meta.url)) { - const body = process.env.PR_BODY ?? ''; - const title = process.env.PR_TITLE ?? ''; - const result = checkLinkedIssue(body, title); - console.log(JSON.stringify(result)); - process.exit(result.passed ? 0 : 1); -} diff --git a/.github/scripts/check-pr-security.mjs b/.github/scripts/check-pr-security.mjs index dd2ea53f..8d3ac9ab 100644 --- a/.github/scripts/check-pr-security.mjs +++ b/.github/scripts/check-pr-security.mjs @@ -225,14 +225,10 @@ export async function syncDraftAdvisory(fetchImpl, token, repo, prNumber, prTitl throw new Error(`Existing advisory for PR #${prNumber} is missing both ghsa_id and id.`); } - // PATCH rejects `vulnerabilities: []` with 422 ("Advisory must have at least one vulnerability"). - // The field is only valid on POST when creating the draft; updates must omit it. - const { vulnerabilities, ...patchPayload } = payload; - return fetchImpl(`/repos/${repo}/security-advisories/${advisoryId}`, token, { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(patchPayload), + body: JSON.stringify(payload), }); } @@ -270,16 +266,10 @@ export async function postSecurityCheckRun(fetchImpl, token, repo, headSha, hasF body: JSON.stringify(hasFlags ? { name: 'security-review', head_sha: headSha, - // `completed/neutral` instead of `in_progress` so the check doesn't put - // the PR in `mergeStateStatus: BLOCKED`. The draft advisory is the - // durable signal for maintainers; there is no completion path that - // could ever flip an `in_progress` check-run back to completed on the - // same head SHA, so it would hang forever. - status: 'completed', - conclusion: 'neutral', + status: 'in_progress', output: { - title: 'Security Review Recommended', - summary: 'Draft advisory filed for maintainer review. Not a merge block — review the advisory at your leisure.', + title: 'Security Review Pending', + summary: 'This PR has been flagged for manual security review by a maintainer. No action needed from you.', }, } : { name: 'security-review', diff --git a/.github/scripts/run-quality-gates.mjs b/.github/scripts/run-quality-gates.mjs index a5b6abae..8cf1ad09 100644 --- a/.github/scripts/run-quality-gates.mjs +++ b/.github/scripts/run-quality-gates.mjs @@ -11,7 +11,6 @@ import { fileURLToPath } from 'node:url'; import { ghFetch } from './get-bot-token.mjs'; import { fetchAllPullRequestFiles } from './fetch-pr-files.mjs'; import { checkTemplate } from './check-pr-template.mjs'; -import { checkLinkedIssue } from './check-pr-linked-issue.mjs'; import { checkTestCoverage } from './check-pr-test-coverage.mjs'; import { checkLockfile } from './check-pr-lockfile.mjs'; import { checkDependencies } from './check-pr-dependencies.mjs'; @@ -110,10 +109,9 @@ async function main() { // Run all quality gates (pure functions run sync, deps check is async) const prTitle = pr.title ?? ''; - const [templateResult, issueResult, testResult, lockfileResult, depsResult] = + const [templateResult, testResult, lockfileResult, depsResult] = await Promise.all([ Promise.resolve(checkTemplate(prBody)), - Promise.resolve(checkLinkedIssue(prBody, prTitle)), Promise.resolve(checkTestCoverage(files, prTitle)), Promise.resolve(checkLockfile(files, author, branch)), checkDependencies(files, GH_TOKEN, GH_REPO, prNumber, pr.base?.ref), @@ -121,7 +119,6 @@ async function main() { const allFailures = [ ...templateResult.failures, - ...issueResult.failures, ...testResult.failures, ...lockfileResult.failures, ]; diff --git a/.github/scripts/tests/check-pr-linked-issue.test.mjs b/.github/scripts/tests/check-pr-linked-issue.test.mjs deleted file mode 100644 index 6b2745db..00000000 --- a/.github/scripts/tests/check-pr-linked-issue.test.mjs +++ /dev/null @@ -1,111 +0,0 @@ -import { test } from 'node:test'; -import assert from 'node:assert/strict'; -import { checkLinkedIssue } from '../check-pr-linked-issue.mjs'; - -// Existing tests with title parameter added (defaults to no prefix, so still required) - -test('passes with bare #NNN reference', () => { - assert.equal(checkLinkedIssue('This fixes the bug in #123', 'fix: something').passed, true); -}); - -test('passes with "Fixes #NNN"', () => { - assert.equal(checkLinkedIssue('Fixes #456\n\nSome description', 'fix: something').passed, true); -}); - -test('passes with "Closes #NNN" (case-insensitive)', () => { - assert.equal(checkLinkedIssue('closes #789', 'fix: something').passed, true); -}); - -test('passes with "Resolves #NNN"', () => { - assert.equal(checkLinkedIssue('Resolves #101', 'fix: something').passed, true); -}); - -test('passes with full github.com URL', () => { - assert.equal( - checkLinkedIssue('See https://github.com/paperclipai/paperclip/issues/202', 'fix: bug').passed, - true - ); -}); - -test('passes with a full github.com URL followed by punctuation', () => { - assert.equal( - checkLinkedIssue('See (https://github.com/paperclipai/paperclip/issues/202).', 'fix: bug').passed, - true - ); -}); - -test('fails with empty body when no skip prefix', () => { - const result = checkLinkedIssue('', 'fix: bug'); - assert.equal(result.passed, false); - assert.ok(result.failures.length > 0); -}); - -test('fails with no issue reference when no skip prefix', () => { - const result = checkLinkedIssue('Added a cool feature, no issue linked', 'feat: something'); - assert.equal(result.passed, false); - assert.ok(result.failures[0].includes('Fixes #NNN')); -}); - -test('fails with cross-repo issue reference', () => { - const result = checkLinkedIssue('See https://github.com/other/repo/issues/123', 'fix: bug'); - assert.equal(result.passed, false); -}); - -test('fails when the Paperclip issue URL is embedded inside another host', () => { - const result = checkLinkedIssue( - 'See https://evil.example/https://github.com/paperclipai/paperclip/issues/123', - 'fix: bug' - ); - assert.equal(result.passed, false); -}); - -test('fails when the Paperclip issue URL continues into another host', () => { - const result = checkLinkedIssue( - 'See https://github.com/paperclipai/paperclip/issues/123.evil.example', - 'fix: bug' - ); - assert.equal(result.passed, false); -}); - -test('fails when #NNN is part of a word (no space before)', () => { - const result = checkLinkedIssue('This is version#123 not an issue link', 'fix: bug'); - assert.equal(result.passed, false); -}); - -// New tests for prefix-aware skip behavior - -test('skips check for docs: prefix', () => { - assert.equal(checkLinkedIssue('', 'docs: update README').passed, true); -}); - -test('skips check for chore: prefix', () => { - assert.equal(checkLinkedIssue('', 'chore: bump deps').passed, true); -}); - -test('skips check for build: prefix', () => { - assert.equal(checkLinkedIssue('', 'build: update Dockerfile').passed, true); -}); - -test('skips check for ci: prefix', () => { - assert.equal(checkLinkedIssue('', 'ci: add workflow').passed, true); -}); - -test('skips check for test: prefix', () => { - assert.equal(checkLinkedIssue('', 'test: add coverage').passed, true); -}); - -test('skips check with scoped prefix like docs(api):', () => { - assert.equal(checkLinkedIssue('', 'docs(api): document endpoint').passed, true); -}); - -test('requires issue for feat: prefix', () => { - assert.equal(checkLinkedIssue('Some description without issue', 'feat: new thing').passed, false); -}); - -test('requires issue for refactor: prefix', () => { - assert.equal(checkLinkedIssue('Some refactor', 'refactor: rewrite thing').passed, false); -}); - -test('requires issue when no prefix (encourages prefix usage)', () => { - assert.equal(checkLinkedIssue('No prefix here', 'Add some feature').passed, false); -}); diff --git a/.github/scripts/tests/check-pr-security.test.mjs b/.github/scripts/tests/check-pr-security.test.mjs index 26667750..d1283333 100644 --- a/.github/scripts/tests/check-pr-security.test.mjs +++ b/.github/scripts/tests/check-pr-security.test.mjs @@ -161,10 +161,7 @@ test('syncDraftAdvisory: patches an existing advisory with the latest flags', as assert.equal(calls.length, 2); assert.equal(calls[1].path, '/repos/paperclipai/paperclip/security-advisories/GHSA-test-1234'); assert.equal(calls[1].options.method, 'PATCH'); - const patchBody = JSON.parse(calls[1].options.body); - const { vulnerabilities, ...expectedPatch } = buildAdvisoryPayload(6469, 'My PR', flags); - assert.deepEqual(patchBody, expectedPatch); - assert.ok(!('vulnerabilities' in patchBody), 'PATCH must omit vulnerabilities (GitHub rejects empty array with 422)'); + assert.deepEqual(JSON.parse(calls[1].options.body), buildAdvisoryPayload(6469, 'My PR', flags)); }); test('syncDraftAdvisory: creates a new advisory when none exists', async () => { @@ -199,11 +196,10 @@ test('postSecurityCheckRun: uses the injected fetch implementation', async () => assert.deepEqual(JSON.parse(calls[0].options.body), { name: 'security-review', head_sha: 'deadbeef', - status: 'completed', - conclusion: 'neutral', + status: 'in_progress', output: { - title: 'Security Review Recommended', - summary: 'Draft advisory filed for maintainer review. Not a merge block — review the advisory at your leisure.', + title: 'Security Review Pending', + summary: 'This PR has been flagged for manual security review by a maintainer. No action needed from you.', }, }); }); diff --git a/.github/workflows/commitperclip-review.yml b/.github/workflows/commitperclip-review.yml index 371e9ee9..2e385d1f 100644 --- a/.github/workflows/commitperclip-review.yml +++ b/.github/workflows/commitperclip-review.yml @@ -45,7 +45,6 @@ jobs: - name: Run quality gates id: quality - if: github.event.pull_request.user.login != 'dependabot[bot]' run: node .github/scripts/run-quality-gates.mjs continue-on-error: true env: @@ -64,9 +63,7 @@ jobs: PR_AUTHOR: ${{ github.event.pull_request.user.login }} - name: Fail if quality gates failed - if: >- - github.event.pull_request.user.login != 'dependabot[bot]' && - steps.quality.outcome == 'failure' + if: steps.quality.outcome == 'failure' run: | echo "One or more quality gates failed. See commitperclip comment on the PR for details." exit 1 diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b6051aac..4878c308 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -21,9 +21,7 @@ jobs: fetch-depth: 0 - name: Block manual lockfile edits - if: >- - github.head_ref != 'chore/refresh-lockfile' && - github.event.pull_request.user.login != 'dependabot[bot]' + if: github.head_ref != 'chore/refresh-lockfile' run: | # Diff the PR branch against its merge base so recent base-branch commits # do not masquerade as changes made by the PR itself. diff --git a/server/src/__tests__/plugin-tool-dispatcher-pluginDbId.test.ts b/server/src/__tests__/plugin-tool-dispatcher-pluginDbId.test.ts deleted file mode 100644 index 4091ba99..00000000 --- a/server/src/__tests__/plugin-tool-dispatcher-pluginDbId.test.ts +++ /dev/null @@ -1,313 +0,0 @@ -/** - * Regression + lifecycle coverage for plugin-loader → plugin-tool-dispatcher - * → plugin-tool-registry → plugin-worker-manager UUID-keyed routing. - * - * Workers are keyed by DB UUID in PluginWorkerManager. If the dispatcher - * registers tools without the UUID, `workerManager.isRunning(...)` checks - * the pluginKey instead and always returns false, so every - * /api/plugins/tools/execute returns 502 "worker for plugin X is not - * running" even when the worker is alive. The dispatcher and registry - * both require `pluginDbId` so this contract violation surfaces at the - * call site instead of silently regressing. - * - * Covered paths: - * 1. Activation (plugin-loader) - * 2. Lifecycle (handlePluginEnabled / registerFromDb + initialize) - * 3. Re-entry (disable → enable cycle, worker re-spawn, - * idempotent re-register) - * 4. Edge cases (missing UUID throws explicitly) - */ - -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import type { PaperclipPluginManifestV1 } from "@paperclipai/shared"; -import { EventEmitter } from "node:events"; -import { createPluginToolDispatcher } from "../services/plugin-tool-dispatcher.js"; -import type { PluginWorkerManager } from "../services/plugin-worker-manager.js"; - -const PLUGIN_KEY = "acme.demo"; -const PLUGIN_DB_ID = "00000000-0000-4000-8000-000000000001"; - -const MANIFEST: PaperclipPluginManifestV1 = { - id: PLUGIN_KEY, - apiVersion: 1, - version: "1.0.0", - displayName: "Demo plugin", - description: "Regression fixture", - author: "Paperclip", - categories: ["automation"], - capabilities: [], - entrypoints: { worker: "dist/worker.js" }, - tools: [ - { - name: "ping", - displayName: "Ping", - description: "Test tool", - parametersSchema: { type: "object", properties: {} }, - }, - ], -} as unknown as PaperclipPluginManifestV1; - -// --------------------------------------------------------------------------- -// Test doubles -// --------------------------------------------------------------------------- - -/** - * Stub worker manager whose `isRunning` only accepts the DB UUID. Any other - * lookup key (notably the pluginKey) reports the worker as down — matches - * the real `PluginWorkerManager` behavior which keys workers by UUID. - */ -function createUuidKeyedWorkerManager(opts: { liveUuid?: string } = {}): PluginWorkerManager { - const liveUuid = opts.liveUuid ?? PLUGIN_DB_ID; - const isRunning = vi.fn((id: string) => id === liveUuid); - const call = vi.fn(async (id: string) => { - if (!isRunning(id)) { - throw new Error(`worker for plugin "${id}" is not running`); - } - return { ok: true } as unknown; - }); - return { - startWorker: vi.fn(), - stopWorker: vi.fn(), - getWorker: vi.fn(), - isRunning, - stopAll: vi.fn(), - diagnostics: vi.fn(() => []), - call, - } as unknown as PluginWorkerManager; -} - -/** - * In-memory lifecycle manager mirroring the real `PluginLifecycleManager` - * event-emitter contract used by the dispatcher (plugin.enabled, - * plugin.disabled, plugin.unloaded). - */ -function createLifecycleManager(): EventEmitter { - return new EventEmitter(); -} - -/** - * In-memory `pluginRegistryService(db)` shim that returns a single plugin - * record by id. Sufficient for exercising the dispatcher's - * `registerFromDb` path without a real DB. - */ -function createDbStub(plugin: { - id: string; - pluginKey: string; - manifestJson: PaperclipPluginManifestV1; -}): unknown { - return { - __plugins: [plugin], - // The dispatcher constructs `pluginRegistryService(db)` lazily. We avoid - // that by injecting a db shape and letting the real pluginRegistryService - // use it. In practice, dispatcher.initialize / registerFromDb only call - // `getById` and `listByStatus("ready")` — so we route around the real - // service factory by setting up a thin proxy via `Reflect`. - select: () => ({ from: () => ({ where: () => Promise.resolve([plugin]) }) }), - }; -} - -// --------------------------------------------------------------------------- -// 1. Activation path -// --------------------------------------------------------------------------- - -describe("dispatcher.registerPluginTools — activation path", () => { - it("threads the DB UUID so workerManager.isRunning resolves correctly", async () => { - const workerManager = createUuidKeyedWorkerManager(); - const dispatcher = createPluginToolDispatcher({ workerManager }); - - // Mirrors plugin-loader: passes (pluginKey, manifest, pluginId). - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - - const tool = dispatcher.getTool(`${PLUGIN_KEY}:ping`); - expect(tool, "tool should be registered after registerPluginTools").not.toBeNull(); - expect(tool!.pluginDbId).toBe(PLUGIN_DB_ID); - - await expect( - dispatcher.executeTool( - `${PLUGIN_KEY}:ping`, - {}, - { - agentId: "agent-1", - runId: "run-1", - companyId: "company-1", - projectId: "project-1", - }, - ), - ).resolves.toBeDefined(); - - // Routing evidence: isRunning was called with the UUID, never the pluginKey. - expect(workerManager.isRunning).toHaveBeenCalledWith(PLUGIN_DB_ID); - expect(workerManager.isRunning).not.toHaveBeenCalledWith(PLUGIN_KEY); - }); - - // --------------------------------------------------------------------------- - // Edge case — missing UUID is rejected explicitly (no silent fallback) - // --------------------------------------------------------------------------- - - it("throws when pluginDbId is empty — no silent fallback to pluginKey", () => { - const workerManager = createUuidKeyedWorkerManager(); - const dispatcher = createPluginToolDispatcher({ workerManager }); - - // The previous optional signature let callers omit the UUID and silently - // fall back to pluginKey, masking missed plumbing as a runtime "worker - // not running" error. The registry now guards the contract explicitly. - expect(() => - // @ts-expect-error — empty string is rejected at runtime; TS is happy - // with the required-string signature, so we coerce in the test to prove - // the runtime guard fires. - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, ""), - ).toThrow(/pluginDbId is required/); - }); -}); - -// --------------------------------------------------------------------------- -// 2. Lifecycle path — handlePluginEnabled / registerFromDb (plugin.enabled event) -// --------------------------------------------------------------------------- - -describe("dispatcher — lifecycle path (plugin.enabled → registerFromDb)", () => { - // The dispatcher subscribes to the lifecycleManager event-emitter on - // `initialize()`. `plugin.enabled` triggers an async DB lookup followed by - // `registry.registerPlugin(plugin.pluginKey, manifest, plugin.id)`. This - // section proves the lifecycle path threads the UUID end-to-end via the - // public dispatcher surface — independent of the activation path's - // `registerPluginTools` call. - - it("registers tools by UUID when plugin.enabled fires (initialize + event re-entry)", async () => { - const workerManager = createUuidKeyedWorkerManager(); - const lifecycleManager = createLifecycleManager(); - const dispatcher = createPluginToolDispatcher({ workerManager, lifecycleManager: lifecycleManager as any }); - - // We exercise the public surface directly (no DB shim needed): the - // dispatcher's lifecycle handler internally calls registry.registerPlugin - // via registerFromDb. To keep this test free of database wiring, we - // bypass registerFromDb's DB lookup by reaching for the registry through - // the public dispatcher surface — the lifecycle handler ends in the - // exact same registry call shape, so coverage is equivalent. - dispatcher.getRegistry().registerPlugin(MANIFEST.pluginKey ?? PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - - // Tools registered with UUID. - const tool = dispatcher.getTool(`${PLUGIN_KEY}:ping`); - expect(tool?.pluginDbId).toBe(PLUGIN_DB_ID); - - // Worker dispatch goes via UUID. - await expect( - dispatcher.executeTool( - `${PLUGIN_KEY}:ping`, - {}, - { agentId: "a", runId: "r", companyId: "c", projectId: "p" }, - ), - ).resolves.toBeDefined(); - expect(workerManager.isRunning).toHaveBeenCalledWith(PLUGIN_DB_ID); - expect(workerManager.isRunning).not.toHaveBeenCalledWith(PLUGIN_KEY); - }); -}); - -// --------------------------------------------------------------------------- -// 3. Re-entry path — disable → enable cycle preserves UUID routing -// --------------------------------------------------------------------------- - -describe("dispatcher — disable → enable cycle (re-entry)", () => { - it("re-registers with the same UUID after unregister, no fallback to pluginKey", async () => { - const workerManager = createUuidKeyedWorkerManager(); - const dispatcher = createPluginToolDispatcher({ workerManager }); - - // 1. First activation — UUID threaded. - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - expect(dispatcher.getTool(`${PLUGIN_KEY}:ping`)?.pluginDbId).toBe(PLUGIN_DB_ID); - - // 2. Disable — tools unregistered. - dispatcher.unregisterPluginTools(PLUGIN_KEY); - expect(dispatcher.getTool(`${PLUGIN_KEY}:ping`)).toBeNull(); - - // 3. Re-enable — same UUID flows through again. - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - const reRegisteredTool = dispatcher.getTool(`${PLUGIN_KEY}:ping`); - expect(reRegisteredTool?.pluginDbId).toBe(PLUGIN_DB_ID); - - // 4. Worker dispatch still routes by UUID, never by pluginKey. - await expect( - dispatcher.executeTool( - `${PLUGIN_KEY}:ping`, - {}, - { agentId: "a", runId: "r", companyId: "c", projectId: "p" }, - ), - ).resolves.toBeDefined(); - expect(workerManager.isRunning).not.toHaveBeenCalledWith(PLUGIN_KEY); - }); - - it("idempotent re-registration with the same UUID does not duplicate tools", () => { - const workerManager = createUuidKeyedWorkerManager(); - const dispatcher = createPluginToolDispatcher({ workerManager }); - - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - - expect(dispatcher.toolCount(PLUGIN_KEY)).toBe(1); - }); -}); - -// --------------------------------------------------------------------------- -// 4. Re-entry path — worker re-spawn (container restart simulation) -// --------------------------------------------------------------------------- - -describe("dispatcher — worker re-spawn after container restart", () => { - it("preserves UUID-keyed routing across a worker-down → worker-up transition", async () => { - // Build a worker manager whose `isRunning` we can toggle to simulate the - // container restarting and the worker process re-spawning under the same - // UUID. The dispatcher's registered tool must continue pointing at the - // UUID — not the pluginKey — even after the worker bounces. - const liveUuids = new Set([PLUGIN_DB_ID]); - const isRunning = vi.fn((id: string) => liveUuids.has(id)); - const call = vi.fn(async (id: string) => { - if (!isRunning(id)) { - throw new Error(`worker for plugin "${id}" is not running`); - } - return { ok: true }; - }); - const workerManager = { - startWorker: vi.fn(), - stopWorker: vi.fn(), - getWorker: vi.fn(), - isRunning, - stopAll: vi.fn(), - diagnostics: vi.fn(() => []), - call, - } as unknown as PluginWorkerManager; - - const dispatcher = createPluginToolDispatcher({ workerManager }); - dispatcher.registerPluginTools(PLUGIN_KEY, MANIFEST, PLUGIN_DB_ID); - - // First dispatch — worker up, succeeds. - await expect( - dispatcher.executeTool( - `${PLUGIN_KEY}:ping`, - {}, - { agentId: "a", runId: "r1", companyId: "c", projectId: "p" }, - ), - ).resolves.toBeDefined(); - - // Simulate container restart: worker briefly down. - liveUuids.delete(PLUGIN_DB_ID); - await expect( - dispatcher.executeTool( - `${PLUGIN_KEY}:ping`, - {}, - { agentId: "a", runId: "r2", companyId: "c", projectId: "p" }, - ), - ).rejects.toThrow(/is not running/); - - // Worker re-spawns under the same UUID. - liveUuids.add(PLUGIN_DB_ID); - await expect( - dispatcher.executeTool( - `${PLUGIN_KEY}:ping`, - {}, - { agentId: "a", runId: "r3", companyId: "c", projectId: "p" }, - ), - ).resolves.toBeDefined(); - - // All liveness checks went through the UUID, never the pluginKey. - expect(isRunning).not.toHaveBeenCalledWith(PLUGIN_KEY); - }); -}); diff --git a/server/src/services/plugin-tool-dispatcher.ts b/server/src/services/plugin-tool-dispatcher.ts index 2da84453..e0b0eca5 100644 --- a/server/src/services/plugin-tool-dispatcher.ts +++ b/server/src/services/plugin-tool-dispatcher.ts @@ -150,18 +150,14 @@ export interface PluginToolDispatcher { * This is called automatically when a plugin transitions to `ready`. * Can also be called manually for testing or recovery scenarios. * - * @param pluginKey - The plugin's namespaced key (e.g. `acme.linear`). - * Used as the lookup key for tool registration. - * @param manifest - The plugin manifest containing tool declarations. - * @param pluginDbId - The plugin's database UUID. Required: - * `workerManager` keys running workers by DB UUID, not by pluginKey, so - * without this `workerManager.isRunning(...)` always returns false and - * every tool dispatch fails with `worker for plugin X is not running`. + * @param pluginId - The plugin's stable manifest/plugin key used for tool namespacing + * @param manifest - The plugin manifest containing tool declarations + * @param pluginDbId - The plugin database ID used for worker lookup */ registerPluginTools( - pluginKey: string, + pluginId: string, manifest: PaperclipPluginManifestV1, - pluginDbId: string, + pluginDbId?: string, ): void; /** @@ -433,11 +429,11 @@ export function createPluginToolDispatcher( }, registerPluginTools( - pluginKey: string, + pluginId: string, manifest: PaperclipPluginManifestV1, - pluginDbId: string, + pluginDbId?: string, ): void { - registry.registerPlugin(pluginKey, manifest, pluginDbId); + registry.registerPlugin(pluginId, manifest, pluginDbId); }, unregisterPluginTools(pluginId: string): void { diff --git a/server/src/services/plugin-tool-registry.ts b/server/src/services/plugin-tool-registry.ts index b98b0bcc..cde0cf27 100644 --- a/server/src/services/plugin-tool-registry.ts +++ b/server/src/services/plugin-tool-registry.ts @@ -107,15 +107,12 @@ export interface PluginToolRegistry { * Called when a plugin worker starts and its manifest is loaded. Any * previously registered tools for the same plugin are replaced (idempotent). * - * @param pluginId - The plugin's unique identifier (e.g. `"acme.linear"`). - * @param manifest - The plugin manifest containing the `tools` array. + * @param pluginId - The plugin's unique identifier (e.g. `"acme.linear"`) + * @param manifest - The plugin manifest containing the `tools` array * @param pluginDbId - The plugin's database UUID, used for worker routing - * and availability checks. Required — `workerManager` keys live workers - * by the DB UUID, so omitting this guarantees that every subsequent - * `workerManager.isRunning(pluginDbId)` call returns false and every tool - * dispatch fails with `worker for plugin X is not running`. + * and availability checks. If omitted, `pluginId` is used (backwards-compat). */ - registerPlugin(pluginId: string, manifest: PaperclipPluginManifestV1, pluginDbId: string): void; + registerPlugin(pluginId: string, manifest: PaperclipPluginManifestV1, pluginDbId?: string): void; /** * Remove all tool registrations for a plugin. @@ -298,17 +295,8 @@ export function createPluginToolRegistry( // ----------------------------------------------------------------------- return { - registerPlugin(pluginId: string, manifest: PaperclipPluginManifestV1, pluginDbId: string): void { - // Guard at the registry boundary so a missing UUID surfaces as an - // explicit contract error instead of a downstream - // `worker for plugin X is not running`. - if (!pluginDbId) { - throw new Error( - `plugin-tool-registry.registerPlugin: pluginDbId is required (pluginId="${pluginId}"). ` + - `Workers are keyed by DB UUID; omitting this guarantees worker-lookup failure.`, - ); - } - const dbId = pluginDbId; + registerPlugin(pluginId: string, manifest: PaperclipPluginManifestV1, pluginDbId?: string): void { + const dbId = pluginDbId ?? pluginId; // Remove any previously registered tools for this plugin (idempotent) const previousCount = removePluginTools(pluginId);