Skip to content

Commit 0d5f79e

Browse files
committed
test(mcp): cover the miner ops actions and the full dispatch path
The store operations and the registered handlers were only reachable through the MCP tools, so nothing measured them directly. These drive each action end to end -- action name through the governor gate to the store call -- plus the validator guard clauses for null, primitive, and array params, and the fail-closed paths (flag disabled, unknown action, malformed params never reaching a store). registerMinerOpsChatActions gains the same evaluateGate seam the dashboard's own registerPortfolioQueueChatActions already exposes, so a test can drive the dispatch path without the real chokepoint; production passes none and gets the real one. paramsOf loses its `?? {}`: every action that calls it has a validator requiring an object, and dispatchChatAction runs that validator first, so the fallback had no reachable case.
1 parent a8e5d9f commit 0d5f79e

3 files changed

Lines changed: 243 additions & 3 deletions

File tree

packages/loopover-miner/lib/chat-miner-ops-actions.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,13 @@ function isPurgeParams(params: unknown): boolean {
6464
return typeof repoFullName === "string" && repoFullName.includes("/");
6565
}
6666

67+
/**
68+
* No nullish fallback: every action that calls this has a params validator requiring an OBJECT, and
69+
* dispatchChatAction runs that validator before the handler. A `?? {}` here would be unreachable defensive
70+
* code -- `miner_run_migrations`, the one action that accepts absent params, takes none and never calls this.
71+
*/
6772
function paramsOf<T>(request: ChatActionRequest): T {
68-
return (request?.params ?? {}) as T;
73+
return request.params as T;
6974
}
7075

7176
/**
@@ -76,7 +81,18 @@ function paramsOf<T>(request: ChatActionRequest): T {
7681
* `evaluateGovernorChokepointGate`, and through it the fail-closed precedence ladder in
7782
* @loopover/engine's governor chokepoint.
7883
*/
79-
export function registerMinerOpsChatActions(actions: MinerOpsActions, registry: ChatActionRegistry = chatActionRegistry): void {
84+
export function registerMinerOpsChatActions(
85+
actions: MinerOpsActions,
86+
registry: ChatActionRegistry = chatActionRegistry,
87+
/**
88+
* Override the chokepoint evaluator. Mirrors the dashboard's own
89+
* `registerPortfolioQueueChatActions({ evaluateGate })` seam: production always uses the DEFAULT (the real
90+
* `evaluateGovernorChokepointGate`), and only a test supplies one, so the gate cannot be weakened by
91+
* configuration.
92+
*/
93+
options: { evaluateGate?: (input: unknown, gateOptions?: unknown) => unknown } = {},
94+
): void {
95+
const gateOpts = options.evaluateGate ? { evaluateGate: options.evaluateGate } : undefined;
8096
const definitions: [string, (params: unknown) => boolean, (request: ChatActionRequest) => Promise<Record<string, unknown>>][] = [
8197
[
8298
MINER_QUEUE_RELEASE_ACTION,
@@ -104,7 +120,7 @@ export function registerMinerOpsChatActions(actions: MinerOpsActions, registry:
104120

105121
for (const [name, paramsValidator, run] of definitions) {
106122
if (registry.has(name)) continue;
107-
registry.register(name, { paramsValidator, handler: governorGatedHandler(run) });
123+
registry.register(name, { paramsValidator, handler: governorGatedHandler(run, gateOpts) });
108124
}
109125
}
110126

test/unit/miner-mcp-governor-gating.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest";
22
import { TOOL_CONTRACTS } from "@loopover/contract/tools";
33
import { createChatActionRegistry, governorGatedHandler } from "../../packages/loopover-miner/lib/chat-action-registry";
44
import { MINER_OPS_CHAT_ACTIONS, registerMinerOpsChatActions, type MinerOpsActions } from "../../packages/loopover-miner/lib/chat-miner-ops-actions";
5+
import { CHAT_ACTION_DISPATCH_ENABLE_VALUE, CHAT_ACTION_DISPATCH_FLAG, dispatchChatAction } from "../../packages/loopover-miner/lib/chat-action-dispatch";
56

67
// #9523 requirement 2 — the structural guarantee, not a spot check.
78
//
@@ -100,6 +101,16 @@ describe("params validators reject malformed input before any dispatch (#9523)",
100101
["miner_run_migrations", { apply: true }],
101102
["miner_purge_repo", { repoFullName: "no-slash" }],
102103
["miner_purge_repo", {}],
104+
// The non-object guards: null, a primitive, and an array are all rejected before any field is read.
105+
["miner_queue_release", null],
106+
["miner_queue_release", "owner/repo"],
107+
["miner_queue_release", []],
108+
["miner_deny_hooks_decide", null],
109+
["miner_deny_hooks_decide", []],
110+
["miner_purge_repo", null],
111+
["miner_purge_repo", []],
112+
["miner_run_migrations", []],
113+
["miner_run_migrations", "go"],
103114
])("%s rejects %j", (action, params) => {
104115
expect(registry.get(action)!.paramsValidator(params)).toBe(false);
105116
});
@@ -112,6 +123,12 @@ describe("params validators reject malformed input before any dispatch (#9523)",
112123
])("%s accepts %j", (action, params) => {
113124
expect(registry.get(action)!.paramsValidator(params)).toBe(true);
114125
});
126+
127+
it("migrations accept the nullish 'no arguments' spellings", () => {
128+
// `migrate` takes no options at all, so a caller may omit params entirely rather than send `{}`.
129+
expect(registry.get("miner_run_migrations")!.paramsValidator(null)).toBe(true);
130+
expect(registry.get("miner_run_migrations")!.paramsValidator(undefined)).toBe(true);
131+
});
115132
});
116133

117134
describe("the mutating tool catalog matches what is registered (#9523)", () => {
@@ -135,3 +152,61 @@ describe("the mutating tool catalog matches what is registered (#9523)", () => {
135152
}
136153
});
137154
});
155+
156+
describe("end-to-end dispatch: MCP action name -> gate -> store operation (#9523)", () => {
157+
// The whole path the MCP tools actually take. Anything that bypasses it cannot be registered at all, so
158+
// this is the only route a mutation can travel — worth exercising per action rather than per unit.
159+
const enabled = { [CHAT_ACTION_DISPATCH_FLAG]: CHAT_ACTION_DISPATCH_ENABLE_VALUE };
160+
161+
function harness() {
162+
const registry = createChatActionRegistry();
163+
const calls: string[] = [];
164+
// An allowing gate: this block is about the DISPATCH path, and the gate's own refusal behavior is
165+
// covered above. Production supplies no evaluateGate, so it gets the real chokepoint.
166+
registerMinerOpsChatActions(fakeActions(calls), registry, { evaluateGate: () => ({ decision: { stage: "allow" } }) });
167+
return { registry, calls };
168+
}
169+
170+
it.each([
171+
["miner_queue_release", { repoFullName: "owner/repo", issueNumber: 1 }, "releaseQueueItem"],
172+
["miner_queue_requeue", { repoFullName: "owner/repo", issueNumber: 2 }, "requeueQueueItem"],
173+
["miner_claim_release", { repoFullName: "owner/repo", issueNumber: 3 }, "releaseClaim"],
174+
["miner_deny_hooks_decide", { repoFullName: "owner/repo", hookId: "h1", decision: "approve" }, "decideDenyHook"],
175+
["miner_run_migrations", {}, "runMigrations"],
176+
["miner_purge_repo", { repoFullName: "owner/repo" }, "purgeRepo"],
177+
])("%s dispatches to %s", async (action, params, expectedCall) => {
178+
const { registry, calls } = harness();
179+
const result = await dispatchChatAction({ action, params }, { registry, env: enabled });
180+
expect(result.ok, `${action} should dispatch: ${JSON.stringify(result)}`).toBe(true);
181+
expect(calls.join("|")).toContain(expectedCall);
182+
});
183+
184+
it("dispatches an action whose request carries NO params at all", async () => {
185+
const { registry, calls } = harness();
186+
const result = await dispatchChatAction({ action: "miner_run_migrations" }, { registry, env: enabled });
187+
expect(result.ok, JSON.stringify(result)).toBe(true);
188+
expect(calls.join("|")).toContain("runMigrations");
189+
});
190+
191+
it("rejects malformed params BEFORE reaching the store operation", async () => {
192+
const { registry, calls } = harness();
193+
const result = await dispatchChatAction({ action: "miner_purge_repo", params: { repoFullName: "no-slash" } }, { registry, env: enabled });
194+
expect(result.ok).toBe(false);
195+
expect(result.status).toBe("invalid_params");
196+
expect(calls, "an invalid request must not reach the store").toEqual([]);
197+
});
198+
199+
it("fails CLOSED when the chat-action flag is not enabled", async () => {
200+
const { registry, calls } = harness();
201+
const result = await dispatchChatAction({ action: "miner_purge_repo", params: { repoFullName: "owner/repo" } }, { registry, env: {} });
202+
expect(result.ok).toBe(false);
203+
expect(result.status).toBe("disabled");
204+
expect(calls).toEqual([]);
205+
});
206+
207+
it("rejects an action that was never registered", async () => {
208+
const { registry } = harness();
209+
const result = await dispatchChatAction({ action: "miner_not_a_thing", params: {} }, { registry, env: enabled });
210+
expect(result.status).toBe("unknown_action");
211+
});
212+
});
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
import { createMinerOpsActions } from "../../packages/loopover-miner/lib/miner-ops-actions";
3+
4+
// #9523: the store operations behind the miner's mutating MCP tools. Every seam is injected here, so these
5+
// assert the wiring — which store call each action makes, and that every store it opens is closed — without
6+
// touching disk. The governor gate in front of them is covered by miner-mcp-governor-gating.test.ts.
7+
8+
function fakeQueue(overrides: Record<string, unknown> = {}) {
9+
const closed = { count: 0 };
10+
const store = {
11+
reclaimStuckItem: vi.fn(() => ({ id: "entry" })),
12+
requeueItem: vi.fn(() => ({ id: "entry" })),
13+
close: vi.fn(() => {
14+
closed.count += 1;
15+
}),
16+
...overrides,
17+
};
18+
return { store, closed };
19+
}
20+
21+
describe("releaseQueueItem", () => {
22+
it("reclaims the lease by the item's string identifier and closes the store", () => {
23+
const { store, closed } = fakeQueue();
24+
const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never });
25+
expect(actions.releaseQueueItem({ repoFullName: "owner/repo", issueNumber: 12 })).toEqual({ released: true, entry: { id: "entry" } });
26+
// The queue keys items by a STRING identifier; an issue number must be stringified, not passed raw.
27+
expect(store.reclaimStuckItem).toHaveBeenCalledWith("owner/repo", "12");
28+
expect(closed.count).toBe(1);
29+
});
30+
31+
it("reports released=false when the item was not held, and still closes", () => {
32+
const { store, closed } = fakeQueue({ reclaimStuckItem: vi.fn(() => null) });
33+
const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never });
34+
expect(actions.releaseQueueItem({ repoFullName: "owner/repo", issueNumber: 1 })).toEqual({ released: false, entry: null });
35+
expect(closed.count).toBe(1);
36+
});
37+
38+
it("closes the store even when the operation throws", () => {
39+
const { store, closed } = fakeQueue({
40+
reclaimStuckItem: vi.fn(() => {
41+
throw new Error("db locked");
42+
}),
43+
});
44+
const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never });
45+
expect(() => actions.releaseQueueItem({ repoFullName: "owner/repo", issueNumber: 1 })).toThrow("db locked");
46+
expect(closed.count, "a thrown operation must not leak the handle").toBe(1);
47+
});
48+
});
49+
50+
describe("requeueQueueItem", () => {
51+
it("requeues by identifier and closes", () => {
52+
const { store, closed } = fakeQueue();
53+
const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never });
54+
expect(actions.requeueQueueItem({ repoFullName: "owner/repo", issueNumber: 7 })).toEqual({ requeued: true, entry: { id: "entry" } });
55+
expect(store.requeueItem).toHaveBeenCalledWith("owner/repo", "7");
56+
expect(closed.count).toBe(1);
57+
});
58+
59+
it("reports requeued=false for an unknown item", () => {
60+
const { store } = fakeQueue({ requeueItem: vi.fn(() => null) });
61+
const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never });
62+
expect(actions.requeueQueueItem({ repoFullName: "owner/repo", issueNumber: 7 })).toEqual({ requeued: false, entry: null });
63+
});
64+
});
65+
66+
describe("releaseClaim", () => {
67+
it("releases the claim by NUMBER — the claim ledger keys by issue number, unlike the queue", () => {
68+
const close = vi.fn();
69+
const releaseClaim = vi.fn(() => ({ status: "released" }));
70+
const actions = createMinerOpsActions({ openClaims: () => ({ releaseClaim, close }) as never });
71+
expect(actions.releaseClaim({ repoFullName: "owner/repo", issueNumber: 3 })).toEqual({ released: true, entry: { status: "released" } });
72+
expect(releaseClaim).toHaveBeenCalledWith("owner/repo", 3);
73+
expect(close).toHaveBeenCalledOnce();
74+
});
75+
76+
it("reports released=false when there was no claim", () => {
77+
const actions = createMinerOpsActions({ openClaims: () => ({ releaseClaim: () => null, close: vi.fn() }) as never });
78+
expect(actions.releaseClaim({ repoFullName: "owner/repo", issueNumber: 3 })).toEqual({ released: false, entry: null });
79+
});
80+
});
81+
82+
describe("decideDenyHook", () => {
83+
it("approves a proposal it can find in that repo", () => {
84+
const setProposalStatus = vi.fn();
85+
const close = vi.fn();
86+
const actions = createMinerOpsActions({
87+
openDenyHooks: () => ({ listProposals: () => [{ id: "hook-1" }], setProposalStatus, close }) as never,
88+
});
89+
expect(actions.decideDenyHook({ repoFullName: "owner/repo", hookId: "hook-1", decision: "approve" })).toEqual({
90+
decided: true,
91+
hookId: "hook-1",
92+
status: "approved",
93+
});
94+
expect(setProposalStatus).toHaveBeenCalledWith("owner/repo", "hook-1", "approved");
95+
expect(close).toHaveBeenCalledOnce();
96+
});
97+
98+
it("maps reject to the rejected status", () => {
99+
const setProposalStatus = vi.fn();
100+
const actions = createMinerOpsActions({
101+
openDenyHooks: () => ({ listProposals: () => [{ id: "hook-2" }], setProposalStatus, close: vi.fn() }) as never,
102+
});
103+
expect(actions.decideDenyHook({ repoFullName: "owner/repo", hookId: "hook-2", decision: "reject" })).toMatchObject({ status: "rejected" });
104+
});
105+
106+
it("reports notFound WITHOUT writing when the proposal is not in that repo", () => {
107+
const setProposalStatus = vi.fn();
108+
const actions = createMinerOpsActions({
109+
openDenyHooks: () => ({ listProposals: () => [{ id: "other" }], setProposalStatus, close: vi.fn() }) as never,
110+
});
111+
expect(actions.decideDenyHook({ repoFullName: "owner/repo", hookId: "missing", decision: "approve" })).toEqual({
112+
decided: false,
113+
notFound: true,
114+
hookId: "missing",
115+
});
116+
expect(setProposalStatus, "an unknown proposal must not be written").not.toHaveBeenCalled();
117+
});
118+
});
119+
120+
describe("runMigrations", () => {
121+
it("reports ok when every store migrated cleanly", () => {
122+
const actions = createMinerOpsActions({
123+
migrate: () => [
124+
{ name: "a", ok: true, status: "migrated" },
125+
{ name: "b", ok: true, status: "up-to-date" },
126+
] as never,
127+
});
128+
expect(actions.runMigrations()).toMatchObject({ ok: true });
129+
});
130+
131+
it("reports ok=false when ANY store failed — a partial sweep is not a success", () => {
132+
const actions = createMinerOpsActions({
133+
migrate: () => [
134+
{ name: "a", ok: true, status: "migrated" },
135+
{ name: "b", ok: false, status: "failed" },
136+
] as never,
137+
});
138+
expect(actions.runMigrations()).toMatchObject({ ok: false });
139+
});
140+
});
141+
142+
describe("purgeRepo", () => {
143+
it("delegates to the CLI's own purge core, so both surfaces cover the same stores", () => {
144+
const purge = vi.fn(() => ({ outcome: "purged", totalPurged: 4 }));
145+
const actions = createMinerOpsActions({ purge: purge as never });
146+
expect(actions.purgeRepo({ repoFullName: "owner/repo" })).toEqual({ outcome: "purged", totalPurged: 4 });
147+
expect(purge).toHaveBeenCalledWith("owner/repo");
148+
});
149+
});

0 commit comments

Comments
 (0)