diff --git a/apps/loopover-miner-ui/src/lib/chat-portfolio-queue-actions.test.tsx b/apps/loopover-miner-ui/src/lib/chat-portfolio-queue-actions.test.tsx index 7906f19a66..aa65fec6f6 100644 --- a/apps/loopover-miner-ui/src/lib/chat-portfolio-queue-actions.test.tsx +++ b/apps/loopover-miner-ui/src/lib/chat-portfolio-queue-actions.test.tsx @@ -397,3 +397,19 @@ describe("formatPortfolioQueueChatResultMessage + MessageList (#6520)", () => { expect(result.messages[0]?.content).toMatch(/Couldn't determine a portfolio-queue target/i); }); }); + +// #8640: the live registration below is the ONLY portfolio release/requeue registration in the repo. The +// deleted packages/loopover-miner/lib/chat-portfolio-actions.ts was a fully-orphaned duplicate that registered +// the same handlers under the `portfolio_release` / `portfolio_requeue` UNDERSCORE names. This guard fails the +// instant that duplicate (or its underscore action names) is reintroduced anywhere the live registration runs. +describe("portfolio chat-action registration is single-sourced (#8640)", () => { + it("registers only the dotted portfolio.release/requeue names, never the deleted duplicate's underscore variant", () => { + const registry = isolatedRegistry(); + registerPortfolioQueueChatActions({ registry: registry as never, evaluateGate: allowGate }); + const names = registry.names(); + expect(names).toContain(PORTFOLIO_QUEUE_CHAT_RELEASE_ACTION); + expect(names).toContain(PORTFOLIO_QUEUE_CHAT_REQUEUE_ACTION); + expect(names).not.toContain("portfolio_release"); + expect(names).not.toContain("portfolio_requeue"); + }); +}); diff --git a/packages/loopover-miner/lib/chat-action-registry.ts b/packages/loopover-miner/lib/chat-action-registry.ts index e90fa73c4a..7139247a1d 100644 --- a/packages/loopover-miner/lib/chat-action-registry.ts +++ b/packages/loopover-miner/lib/chat-action-registry.ts @@ -1,8 +1,11 @@ // Allowlist registry + governor-gated handler contract for chat-issued miner actions (#6519). // -// Shared scaffolding ONLY: this module ships with ZERO registered actions. The three action-family child -// issues (portfolio release/requeue, governor pause/resume, discover/attempt) register their handlers into -// this registry -- none are added here, and the default `chatActionRegistry` instance starts empty. +// Shared scaffolding ONLY: this module ships with ZERO registered actions. The action families register their +// handlers into this registry -- none are added here, and the default `chatActionRegistry` instance starts +// empty. Governor pause/resume and discover/attempt register from packages/loopover-miner; portfolio +// release/requeue registers from apps/loopover-miner-ui (chat-portfolio-queue-actions.ts) under the +// `portfolio.release` / `portfolio.requeue` names -- #8640 deleted an orphaned duplicate of that family that +// had lived here in packages/loopover-miner under the divergent `portfolio_release` / `portfolio_requeue` names. // // The registration contract is the safety boundary. `register` refuses any handler that was not produced by // `governorGatedHandler()`, and `governorGatedHandler()` routes every invocation through diff --git a/packages/loopover-miner/lib/chat-discover-attempt-actions.ts b/packages/loopover-miner/lib/chat-discover-attempt-actions.ts index ee8303c1e8..cab4532671 100644 --- a/packages/loopover-miner/lib/chat-discover-attempt-actions.ts +++ b/packages/loopover-miner/lib/chat-discover-attempt-actions.ts @@ -17,7 +17,8 @@ // (vite-discover-api.ts:13-14), so the CLI has none and the route adds none. // Re-evaluating the chokepoint here would therefore be a SECOND, competing gate on a path that already has // one (or needs none) — exactly what those route comments rule out, and it would gate chat more strictly than -// the equivalent CLI invocation. So, like chat-governor-actions.js and chat-portfolio-actions.js, we satisfy +// the equivalent CLI invocation. So, like chat-governor-actions.js (#8640 deleted the orphaned miner-side +// chat-portfolio-actions.js; portfolio release/requeue now registers from apps/loopover-miner-ui), we satisfy // the registry's `governorGatedHandler` brand with an allow-stage evaluateGate. Execution still stays behind // the shared LOOPOVER_MINER_CHAT_ACTIONS flag via `dispatchChatAction`, and `evaluateGate` stays injectable. diff --git a/packages/loopover-miner/lib/chat-portfolio-actions.ts b/packages/loopover-miner/lib/chat-portfolio-actions.ts deleted file mode 100644 index c225b60a10..0000000000 --- a/packages/loopover-miner/lib/chat-portfolio-actions.ts +++ /dev/null @@ -1,100 +0,0 @@ -// Portfolio release/requeue chat-action registrations (#6838). -// -// Child issue of the chat action-dispatch scaffolding (#6519). Registers `portfolio_release` / -// `portfolio_requeue` into a chat-action registry. Handlers MUST be wired to the miner-ui clients -// `releasePortfolioQueueItem` / `requeuePortfolioQueueItem` (apps/loopover-miner-ui/src/lib/ -// portfolio-queue-actions.ts), so chat POSTs the SAME `/api/portfolio-queue/{release,requeue}` routes the -// dashboard's existing buttons already call — never portfolio-queue.js directly, and never a hand-rolled -// fetch. The miner-ui wire module passes those clients in; this module only owns the registration contract -// + params validators. That is what keeps chat from becoming a parallel write path (#6504's design). -// -// Release/requeue is local queue administration, not a chokepoint content-write: the route it lands on is a -// thin bridge to the same store methods the CLI's `queue release` / `queue requeue` already use -// (vite-portfolio-queue-actions-api.ts → reclaimStuckItem / requeueItem), and it invokes no chokepoint of its -// own. Requiring one only for the chat path would gate chat MORE strictly than the button beside it, which -// #6838 forbids ("No changes to the existing route or button-triggered flow"). So, exactly like -// chat-governor-actions.js's administrative pause/resume, we satisfy the registry's `governorGatedHandler` -// brand with an allow-stage evaluateGate rather than routing through governor-chokepoint.js. Execution still -// stays behind the shared LOOPOVER_MINER_CHAT_ACTIONS flag via `dispatchChatAction`. - -import { governorGatedHandler, chatActionRegistry } from "./chat-action-registry.js"; -import type { ChatActionRegistry } from "./chat-action-registry.js"; - -export const PORTFOLIO_RELEASE_CHAT_ACTION = "portfolio_release"; -export const PORTFOLIO_REQUEUE_CHAT_ACTION = "portfolio_requeue"; - -export type PortfolioChatActionItem = { - repoFullName: string; - identifier: string; - apiBaseUrl?: string; -}; - -/** Local queue administration is not a chokepoint content-write (#6838); satisfy the registry brand only. */ -const allowAdministrativeGate = () => ({ decision: { stage: "allow" } }); - -/** - * Params for both actions: the queue item to act on. `repoFullName` + `identifier` are required non-empty - * strings; `apiBaseUrl` is optional (the route defaults it, mirroring the client's own - * `Pick` shape, where the buttons - * always pass one but the CLI path does not). Unknown keys are rejected rather than ignored: a typo'd param - * from a model-authored call must fail loudly, not silently act on the wrong item. - */ -export function isPortfolioItemChatParams(params: unknown): boolean { - if (params == null || typeof params !== "object" || Array.isArray(params)) return false; - const record = params as Record; - for (const key of Object.keys(record)) { - if (key !== "repoFullName" && key !== "identifier" && key !== "apiBaseUrl") return false; - } - if (typeof record.repoFullName !== "string" || record.repoFullName.trim() === "") return false; - if (typeof record.identifier !== "string" || record.identifier.trim() === "") return false; - if (record.apiBaseUrl !== undefined && typeof record.apiBaseUrl !== "string") return false; - return true; -} - -/** - * Narrow validated params to the client's item shape. `apiBaseUrl` is only forwarded when present, so an - * omitted one stays omitted rather than becoming an explicit `undefined` in the POST body. - */ -function readPortfolioItem(params: unknown): PortfolioChatActionItem { - const record = params as { repoFullName: string; identifier: string; apiBaseUrl?: unknown }; - const item = { repoFullName: record.repoFullName, identifier: record.identifier }; - return typeof record.apiBaseUrl === "string" ? { ...item, apiBaseUrl: record.apiBaseUrl } : item; -} - -/** Idempotently register `portfolio_release` / `portfolio_requeue`. */ -export function registerPortfolioChatActions(options: { - releaseItem: (item: PortfolioChatActionItem) => Promise; - requeueItem: (item: PortfolioChatActionItem) => Promise; - registry?: ChatActionRegistry; - evaluateGate?: () => { decision: { stage: string } }; -}): void { - const releaseItem = options?.releaseItem; - const requeueItem = options?.requeueItem; - if (typeof releaseItem !== "function") { - throw new TypeError("registerPortfolioChatActions: releaseItem must be a function"); - } - if (typeof requeueItem !== "function") { - throw new TypeError("registerPortfolioChatActions: requeueItem must be a function"); - } - - const registry = options.registry ?? chatActionRegistry; - const evaluateGate = options.evaluateGate ?? allowAdministrativeGate; - - if (!registry.has(PORTFOLIO_RELEASE_CHAT_ACTION)) { - registry.register(PORTFOLIO_RELEASE_CHAT_ACTION, { - paramsValidator: isPortfolioItemChatParams, - handler: governorGatedHandler(async (request) => releaseItem(readPortfolioItem(request?.params)), { - evaluateGate, - }), - }); - } - - if (!registry.has(PORTFOLIO_REQUEUE_CHAT_ACTION)) { - registry.register(PORTFOLIO_REQUEUE_CHAT_ACTION, { - paramsValidator: isPortfolioItemChatParams, - handler: governorGatedHandler(async (request) => requeueItem(readPortfolioItem(request?.params)), { - evaluateGate, - }), - }); - } -} diff --git a/test/unit/miner-chat-portfolio-actions.test.ts b/test/unit/miner-chat-portfolio-actions.test.ts deleted file mode 100644 index e112e407b9..0000000000 --- a/test/unit/miner-chat-portfolio-actions.test.ts +++ /dev/null @@ -1,178 +0,0 @@ -import { describe, expect, it, vi } from "vitest"; - -// governor-chokepoint.js (imported transitively by chat-action-registry.js) pulls in @loopover/engine, whose -// dist is not built in the test workspace -- resolve it against source, matching the sibling miner tests. -vi.mock("@loopover/engine", async () => { - return import("../../packages/loopover-engine/src/index"); -}); - -import { - CHAT_ACTION_DISPATCH_ENABLE_VALUE, - CHAT_ACTION_DISPATCH_FLAG, - dispatchChatAction, -} from "../../packages/loopover-miner/lib/chat-action-dispatch.js"; -import { chatActionRegistry, createChatActionRegistry } from "../../packages/loopover-miner/lib/chat-action-registry.js"; -import { - isPortfolioItemChatParams, - PORTFOLIO_RELEASE_CHAT_ACTION, - PORTFOLIO_REQUEUE_CHAT_ACTION, - registerPortfolioChatActions, -} from "../../packages/loopover-miner/lib/chat-portfolio-actions.js"; - -const enabledEnv = { [CHAT_ACTION_DISPATCH_FLAG]: CHAT_ACTION_DISPATCH_ENABLE_VALUE }; - -const item = { repoFullName: "acme/widgets", identifier: "issue:12", apiBaseUrl: "https://api.github.com" }; -const released = { ok: true, entry: { repoFullName: "acme/widgets", identifier: "issue:12", status: "queued" } }; - -type ChatItem = { repoFullName: string; identifier: string; apiBaseUrl?: string }; - -function setup(over: Partial[0]> = {}) { - const registry = createChatActionRegistry(); - // Typed params (rather than `vi.fn(async () => …)`) so `mock.calls[0][0]` is a real, inspectable argument -- - // the untyped form infers a zero-length tuple and the forwarded item can't be asserted on. - const releaseItem = vi.fn(async (_item: ChatItem) => released); - const requeueItem = vi.fn(async (_item: ChatItem) => released); - registerPortfolioChatActions({ registry, releaseItem, requeueItem, ...over }); - return { registry, releaseItem, requeueItem }; -} - -describe("isPortfolioItemChatParams (#6838)", () => { - it("accepts a full item and an item without the optional apiBaseUrl", () => { - expect(isPortfolioItemChatParams(item)).toBe(true); - expect(isPortfolioItemChatParams({ repoFullName: "acme/widgets", identifier: "issue:12" })).toBe(true); - }); - - it("rejects a missing, non-object, or array params value", () => { - // Unlike governor pause/resume, these actions have REQUIRED params: there is no sensible default item to - // act on, so nullish must not silently resolve to one. - expect(isPortfolioItemChatParams(null)).toBe(false); - expect(isPortfolioItemChatParams(undefined)).toBe(false); - expect(isPortfolioItemChatParams("acme/widgets")).toBe(false); - expect(isPortfolioItemChatParams([item])).toBe(false); - }); - - it("rejects a missing or empty repoFullName / identifier", () => { - expect(isPortfolioItemChatParams({ identifier: "issue:12" })).toBe(false); - expect(isPortfolioItemChatParams({ repoFullName: "acme/widgets" })).toBe(false); - expect(isPortfolioItemChatParams({ repoFullName: "", identifier: "issue:12" })).toBe(false); - expect(isPortfolioItemChatParams({ repoFullName: "acme/widgets", identifier: " " })).toBe(false); - }); - - it("rejects a non-string repoFullName / identifier / apiBaseUrl", () => { - expect(isPortfolioItemChatParams({ repoFullName: 42, identifier: "issue:12" })).toBe(false); - expect(isPortfolioItemChatParams({ repoFullName: "acme/widgets", identifier: 12 })).toBe(false); - expect(isPortfolioItemChatParams({ ...item, apiBaseUrl: 42 })).toBe(false); - }); - - it("rejects an unknown key rather than ignoring it", () => { - // A model-authored call that typos a param must fail loudly, not act on a different item than intended. - expect(isPortfolioItemChatParams({ ...item, status: "done" })).toBe(false); - expect(isPortfolioItemChatParams({ ...item, repo_full_name: "acme/other" })).toBe(false); - }); -}); - -describe("registerPortfolioChatActions (#6838)", () => { - it("registers both actions on the supplied registry", () => { - const { registry } = setup(); - expect(registry.names().sort()).toEqual([PORTFOLIO_RELEASE_CHAT_ACTION, PORTFOLIO_REQUEUE_CHAT_ACTION].sort()); - }); - - it("throws when releaseItem or requeueItem is not a function", () => { - const registry = createChatActionRegistry(); - expect(() => registerPortfolioChatActions({ registry, requeueItem: async () => released } as never)).toThrow( - "releaseItem must be a function", - ); - expect(() => registerPortfolioChatActions({ registry, releaseItem: async () => released } as never)).toThrow( - "requeueItem must be a function", - ); - }); - - it("is idempotent: a second registration does not throw on the already-registered name", () => { - const { registry, releaseItem, requeueItem } = setup(); - expect(() => registerPortfolioChatActions({ registry, releaseItem, requeueItem })).not.toThrow(); - expect(registry.size).toBe(2); - }); - - it("falls back to the shared chatActionRegistry when no registry is supplied", () => { - // The production wiring omits `registry`, so this nullish default is the path that actually ships -- every - // other test here injects an isolated registry and would never exercise it. - expect(chatActionRegistry.has(PORTFOLIO_RELEASE_CHAT_ACTION)).toBe(false); - registerPortfolioChatActions({ releaseItem: async () => released, requeueItem: async () => released }); - expect(chatActionRegistry.has(PORTFOLIO_RELEASE_CHAT_ACTION)).toBe(true); - expect(chatActionRegistry.has(PORTFOLIO_REQUEUE_CHAT_ACTION)).toBe(true); - }); - - it("registers handlers the registry accepts as governor-gated", () => { - // The registry rejects any raw handler, so a successful register() IS the proof the brand is present. - const { registry } = setup(); - expect(registry.has(PORTFOLIO_RELEASE_CHAT_ACTION)).toBe(true); - expect(registry.has(PORTFOLIO_REQUEUE_CHAT_ACTION)).toBe(true); - }); -}); - -describe("portfolio chat actions through dispatchChatAction (#6838)", () => { - it("releases via the injected miner-ui client, forwarding the exact item", async () => { - const { registry, releaseItem, requeueItem } = setup(); - const result = await dispatchChatAction( - { action: PORTFOLIO_RELEASE_CHAT_ACTION, params: item }, - { registry, env: enabledEnv }, - ); - // dispatchChatAction wraps the handler's own result: the outer envelope reports the dispatch, the inner - // one reports the gate verdict + the client's return value. - expect(result).toMatchObject({ ok: true, status: "dispatched", action: PORTFOLIO_RELEASE_CHAT_ACTION }); - expect(result.result).toMatchObject({ ok: true, status: "executed", result: released }); - // Routed through the client that POSTs /api/portfolio-queue/release -- never the store directly. - expect(releaseItem).toHaveBeenCalledWith(item); - expect(requeueItem).not.toHaveBeenCalled(); - }); - - it("requeues via the injected miner-ui client", async () => { - const { registry, releaseItem, requeueItem } = setup(); - await dispatchChatAction({ action: PORTFOLIO_REQUEUE_CHAT_ACTION, params: item }, { registry, env: enabledEnv }); - expect(requeueItem).toHaveBeenCalledWith(item); - expect(releaseItem).not.toHaveBeenCalled(); - }); - - it("omits apiBaseUrl from the forwarded item when it was not supplied", async () => { - // Not passed as an explicit `undefined`: the client spreads the item into the POST body, so a stray key - // would serialize as `"apiBaseUrl": undefined` and change the request the buttons already send. - const { registry, releaseItem } = setup(); - await dispatchChatAction( - { action: PORTFOLIO_RELEASE_CHAT_ACTION, params: { repoFullName: "acme/widgets", identifier: "issue:12" } }, - { registry, env: enabledEnv }, - ); - // toHaveBeenCalledWith uses toEqual semantics, which treat an explicit `undefined` key as absent -- so the - // key list is asserted directly, since that is the exact thing this test exists to pin. - expect(Object.keys(releaseItem.mock.calls[0]![0])).toEqual(["repoFullName", "identifier"]); - }); - - it("does not run the client when the shared action flag is off", async () => { - const { registry, releaseItem } = setup(); - const result = await dispatchChatAction({ action: PORTFOLIO_RELEASE_CHAT_ACTION, params: item }, { registry, env: {} }); - expect(result).toMatchObject({ ok: false }); - expect(releaseItem).not.toHaveBeenCalled(); - }); - - it("does not run the client when params fail validation", async () => { - const { registry, releaseItem } = setup(); - const result = await dispatchChatAction( - { action: PORTFOLIO_RELEASE_CHAT_ACTION, params: { repoFullName: "acme/widgets" } }, - { registry, env: enabledEnv }, - ); - expect(result).toMatchObject({ ok: false }); - expect(releaseItem).not.toHaveBeenCalled(); - }); - - it("does not run the client when the gate denies", async () => { - // The registry's brand guarantees every handler consults a gate first; a non-allow stage must short-circuit - // BEFORE the write, not report a gated result after performing it. - const { registry, releaseItem } = setup({ evaluateGate: () => ({ decision: { stage: "deny" } }) }); - const result = await dispatchChatAction( - { action: PORTFOLIO_RELEASE_CHAT_ACTION, params: item }, - { registry, env: enabledEnv }, - ); - // The dispatch itself still succeeds -- it is the HANDLER's inner result that reports the refusal. - expect(result.result).toMatchObject({ ok: false, status: "gated", decision: { stage: "deny" } }); - expect(releaseItem).not.toHaveBeenCalled(); - }); -});