From 63111066a6af39fe86788f90b72d9236edd1fec0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:40:22 -0700 Subject: [PATCH 1/5] feat(mcp): add the AMS management tool family behind the governor gate The miner MCP exposed 11 read-only tools while its entire mutating ops surface was CLI-only. This adds 10 tools: a dedicated doctor (split out of status so status stays cheap), a structured metrics snapshot, and eight mutations. Every mutation dispatches through the miner's existing governor-gated chat-action chokepoint -- the same boundary the dashboard's own actions use. The MCP layer never touches a store: it dispatches an action NAME, and the registry structurally refuses any handler not produced by governorGatedHandler(), whose brand is a private symbol a raw function cannot forge. That is what makes "an MCP caller cannot reach a write path the dashboard could not" a property of the code rather than of review discipline, and the structural test asserts it from both ends. Three claims I had to correct against the real code rather than ship as written. The migrate CLI has no dry-run, because applying a migration IS opening the store -- so the tool has no apply flag either, instead of advertising a safety mode that does not exist. The deny-hook store keys proposals by (repo, id), so the decide tool takes the repo rather than scanning every repo's proposals to resolve an id. And purgeRepoAcrossStores is extracted from runPurge so the tool runs the CLI's own purge over the CLI's own target list, rather than a second implementation free to miss a store. collectMinerPredictionMetrics is likewise extracted in the engine: the Prometheus text renderer now formats those families, so the scrape and the JSON snapshot share one aggregation and cannot disagree about what a counter means. validate:mcp earned its keep twice here. It caught the two AMS tenant tools declared but never registered, and it caught loopover_miner_run_migrations/_purge_repo returning fields their output schemas did not declare -- the .shape re-wrap that drops looseObject's catchall, the same -32602 class this epic already fixed once. Both outputs now declare every field their handlers return. AMS tenant create/list/destroy are deliberately absent: #9522's loopover_tenant_* tools are product-parameterized and already serve product "ams", because the control plane's routes are. Only health and wake -- the pair with no ORB counterpart -- are added. The catalog's recorded exclusions (calibration floors, raw run-state set, the one-way kill switch) stay CLI-only, pinned by a test so reversing that decision has to be deliberate. --- .../loopover-contract/src/tools/ams-tenant.ts | 72 +++++ packages/loopover-contract/src/tools/index.ts | 6 + .../loopover-contract/src/tools/miner-ops.ts | 279 ++++++++++++++++++ packages/loopover-engine/src/index.ts | 2 + .../src/miner-prediction-metrics.ts | 67 ++++- .../loopover-miner/bin/loopover-miner-mcp.ts | 169 +++++++++++ .../lib/chat-miner-ops-actions.ts | 119 ++++++++ .../loopover-miner/lib/miner-ops-actions.ts | 100 +++++++ packages/loopover-miner/lib/purge-cli.ts | 38 ++- src/mcp/server.ts | 51 +++- src/orb/control-plane-client.ts | 15 + test/unit/miner-mcp-governor-gating.test.ts | 137 +++++++++ 12 files changed, 1025 insertions(+), 30 deletions(-) create mode 100644 packages/loopover-contract/src/tools/ams-tenant.ts create mode 100644 packages/loopover-contract/src/tools/miner-ops.ts create mode 100644 packages/loopover-miner/lib/chat-miner-ops-actions.ts create mode 100644 packages/loopover-miner/lib/miner-ops-actions.ts create mode 100644 test/unit/miner-mcp-governor-gating.test.ts diff --git a/packages/loopover-contract/src/tools/ams-tenant.ts b/packages/loopover-contract/src/tools/ams-tenant.ts new file mode 100644 index 0000000000..5ccf32dc65 --- /dev/null +++ b/packages/loopover-contract/src/tools/ams-tenant.ts @@ -0,0 +1,72 @@ +// The hosted AMS tenant surface (#9523, #9199). +// +// CATALOG AMENDMENT, recorded here rather than improvised: #9523 listed +// `loopover_ams_tenant_create` / `_list` / `_destroy`, and they are NOT here. #9522 landed +// `loopover_tenant_create` / `_list` / `_destroy` taking a `product` of "ams" or "orb", because the control +// plane's own `/v1/tenants` routes are product-parameterized and key their registry by `${product}:${name}`. +// A second, AMS-only spelling of those three would be two names for one capability -- the same rot that kept +// `loopover_fleet_get_analytics` out of #9522 -- and would leave an agent guessing which to call. Use the +// product-parameterized tools with `product: "ams"`. +// +// What IS genuinely AMS-specific, and therefore here, is the pair that has no ORB counterpart: a tenant's +// wake schedule and cycle outcomes, and the ability to trigger a cycle now. +import { z } from "zod"; +import { defineTool } from "../tool-definition.js"; + +const TenantName = z.string().min(1).max(200); + +export const AmsTenantHealthInput = z.object({ + name: TenantName.describe("The tenant's name, as reported by loopover_tenant_list with product=ams."), +}); + +export const AmsTenantHealthOutput = z.looseObject({ + configured: z.boolean().describe("False when this deployment administers no hosted tenants."), + name: z.string().optional(), + state: z.string().optional().describe("The control plane's own lifecycle vocabulary, passed through verbatim."), + schedule: z.string().nullable().optional().describe("The cron-wake cadence, or null when the tenant wakes only on demand."), + lastWakeAt: z.string().nullable().optional(), + lastCycleOutcome: z.string().nullable().optional(), + containerHealthy: z.boolean().nullable().optional(), + error: z.string().optional(), +}); + +export const amsTenantHealthTool = defineTool({ + name: "loopover_ams_tenant_health", + title: "Read a hosted AMS tenant's health", + description: + "Operator only. One hosted AMS tenant's health: lifecycle state, its cron-wake cadence, when it last woke, that cycle's outcome, and container health. Read-only, and scoped server-side to the authenticated tenant — a name outside that scope is refused rather than answered.", + category: "tenant", + auth: "internal", + locality: "remote", + availability: "cloud", + input: AmsTenantHealthInput, + output: AmsTenantHealthOutput, +}); + +export const AmsTenantWakeInput = z.object({ + name: TenantName, +}); + +export const AmsTenantWakeOutput = z.looseObject({ + configured: z.boolean(), + name: z.string().optional(), + woken: z.boolean().optional(), + throttled: z.boolean().optional().describe("True when the tenant's own schedule guard refused a wake this soon after the last one."), + error: z.string().optional(), +}); + +export const amsTenantWakeTool = defineTool({ + name: "loopover_ams_tenant_wake", + title: "Wake a hosted AMS tenant now", + description: + "Operator only. Trigger an immediate cycle for one hosted AMS tenant, instead of waiting for its next scheduled wake. Bounded by the SAME per-tenant schedule guards the cron path obeys — a wake too soon after the last one is reported as throttled rather than forced through.", + category: "tenant", + auth: "internal", + locality: "remote", + availability: "cloud", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: AmsTenantWakeInput, + output: AmsTenantWakeOutput, +}); + +export const AMS_TENANT_TOOLS = [amsTenantHealthTool, amsTenantWakeTool] as const; diff --git a/packages/loopover-contract/src/tools/index.ts b/packages/loopover-contract/src/tools/index.ts index 84f474694f..17b00347c6 100644 --- a/packages/loopover-contract/src/tools/index.ts +++ b/packages/loopover-contract/src/tools/index.ts @@ -147,6 +147,8 @@ import { OPS_TOOLS } from "./ops.js"; import { FLEET_TOOLS } from "./fleet.js"; import { TENANT_TOOLS } from "./tenant.js"; import { INSTANCE_OPS_TOOLS } from "./instance-ops.js"; +import { MINER_OPS_TOOLS } from "./miner-ops.js"; +import { AMS_TENANT_TOOLS } from "./ams-tenant.js"; import { adminRotateSecretTool } from "./admin-config.js"; export const TOOL_CONTRACTS: readonly ToolContract[] = [ @@ -282,6 +284,8 @@ export const TOOL_CONTRACTS: readonly ToolContract[] = [ ...OPS_TOOLS, ...FLEET_TOOLS, ...TENANT_TOOLS, + ...MINER_OPS_TOOLS, + ...AMS_TENANT_TOOLS, ]; const CONTRACTS_BY_NAME: ReadonlyMap = new Map( @@ -318,3 +322,5 @@ export * from "./ops.js"; export * from "./fleet.js"; export * from "./tenant.js"; export * from "./instance-ops.js"; +export * from "./miner-ops.js"; +export * from "./ams-tenant.js"; diff --git a/packages/loopover-contract/src/tools/miner-ops.ts b/packages/loopover-contract/src/tools/miner-ops.ts new file mode 100644 index 0000000000..3d8194b94f --- /dev/null +++ b/packages/loopover-contract/src/tools/miner-ops.ts @@ -0,0 +1,279 @@ +// The AMS miner's management surface (#9523). +// +// The miner MCP exposed 11 read-only tools while its entire MUTATING ops surface was CLI-only. These add +// the mutating family, plus the two reads the catalog called for. +// +// Every mutating tool dispatches through the miner's existing governor-gated chat-action chokepoint +// (packages/loopover-miner/lib/chat-action-registry.ts), which structurally refuses any handler not produced +// by `governorGatedHandler()`. That is the same boundary the dashboard's own actions go through, so an MCP +// caller cannot reach a write path the dashboard could not -- and the structural rule is enforced by the +// registry's own brand, not by review discipline. +// +// locality "miner": these run inside the AMS bin against its local stores. availability "selfhost" for the +// same reason the ORB config-admin tools are: the stores are on that host's disk. +// +// DELIBERATELY ABSENT, recorded here so the omissions are visible rather than forgotten: +// * calibration apply-min-rank / revert-min-rank -- stays CLI-only behind its existing double gate. An +// agent-reachable path to loosen calibration floors is not wanted. +// * run-state `set` -- raw run-state mutation is a repair tool, not an operation. +// * kill-switch trip/untrip -- one-way breaker semantics stay out of agent reach; pause/resume is the +// agent-safe control and is here instead. +import { z } from "zod"; +import { defineTool } from "../tool-definition.js"; +import { INSTANCE_CHECK_STATUSES } from "../enums.js"; + +const RepoFullName = z.string().min(3).max(200).describe("owner/repo."); + +/** Mutating tools take this so an omitted field cannot read as false and proceed. */ +const DestructiveConfirm = z.literal(true).describe("Must be exactly true. Confirms an irreversible action."); + +export const MinerDoctorInput = z.object({}); + +export const MinerDoctorOutput = z.looseObject({ + ok: z.boolean().describe("True when no check reported fail. Warnings do not clear it to false."), + checks: z.array(z.looseObject({ name: z.string(), status: z.enum(INSTANCE_CHECK_STATUSES), detail: z.string().optional() })), +}); + +export const minerDoctorTool = defineTool({ + name: "loopover_miner_doctor", + title: "Diagnose this miner", + description: + "Read-only diagnostic checks for this AMS miner: state directory, engine version match, store reachability, credentials, and configuration. Split out of loopover_miner_status (#9523) so status stays cheap and doctor can grow checks. Every check runs and reports its own pass/warn/fail — nothing is mutated and nothing stops at the first failure.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + input: MinerDoctorInput, + output: MinerDoctorOutput, +}); + +export const MinerMetricsSnapshotInput = z.object({}); + +export const MinerMetricsSnapshotOutput = z.looseObject({ + generatedAt: z.string(), + families: z.array( + z.looseObject({ + name: z.string(), + type: z.string(), + help: z.string().optional(), + samples: z.array(z.looseObject({ value: z.number(), labels: z.record(z.string(), z.string()).optional() })), + }), + ), +}); + +export const minerMetricsSnapshotTool = defineTool({ + name: "loopover_miner_get_metrics_snapshot", + title: "Read the miner's metrics snapshot", + description: + "The same Prometheus metric families the `metrics` CLI exports, as structured JSON — so an agent can read them without parsing the text exposition format. Read-only.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + input: MinerMetricsSnapshotInput, + output: MinerMetricsSnapshotOutput, +}); + +/** + * Shared output for every governor-gated mutation: what happened, and what the chokepoint decided. A refusal + * is reported as `blocked` with a `reason` rather than thrown -- the governor saying no is an ANSWER the + * caller needs to see, and a thrown error would flatten it into a generic tool failure. + */ +export const MinerGovernorActionOutput = z.looseObject({ + ok: z.boolean().optional(), + action: z.string().optional(), + declined: z.boolean().optional().describe("True when the caller declined an elicited confirmation."), + blocked: z.boolean().optional().describe("True when the governor chokepoint refused the action."), + reason: z.string().optional(), + result: z.unknown().optional(), + error: z.string().optional(), +}); + +export const MinerGovernorPauseInput = z.object({ + reason: z.string().max(500).optional().describe("Recorded on the pause so the audit feed says why."), +}); + +export const minerGovernorPauseTool = defineTool({ + name: "loopover_miner_governor_pause", + title: "Pause the miner governor", + description: + "Pause this miner's governor: no new work is admitted until it resumes. Administrative control, not a content write — the same action the dashboard's pause button dispatches, through the same governor-gated chokepoint, firing the same notification side-channel. Recorded in the event ledger with source=mcp.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: MinerGovernorPauseInput, + output: MinerGovernorActionOutput, +}); + +export const MinerGovernorResumeInput = z.object({}); + +export const minerGovernorResumeTool = defineTool({ + name: "loopover_miner_governor_resume", + title: "Resume the miner governor", + description: + "Resume this miner's governor after a pause, re-admitting work. The same action the dashboard's resume button dispatches, through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: MinerGovernorResumeInput, + output: MinerGovernorActionOutput, +}); + +const QueueItemTarget = z.object({ + repoFullName: RepoFullName, + issueNumber: z.number().int().positive(), +}); + +export const MinerQueueReleaseInput = QueueItemTarget; + +export const minerQueueReleaseTool = defineTool({ + name: "loopover_miner_queue_release", + title: "Release a portfolio queue item", + description: + "Release one claimed portfolio-queue item back to unclaimed, so another cycle can pick it up. Mirrors the dashboard's release action and dispatches through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: MinerQueueReleaseInput, + output: MinerGovernorActionOutput, +}); + +export const MinerQueueRequeueInput = QueueItemTarget; + +export const minerQueueRequeueTool = defineTool({ + name: "loopover_miner_queue_requeue", + title: "Requeue a portfolio queue item", + description: + "Return one portfolio-queue item to the pending pool for another attempt. Mirrors the dashboard's requeue action and dispatches through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: MinerQueueRequeueInput, + output: MinerGovernorActionOutput, +}); + +export const MinerClaimReleaseInput = QueueItemTarget; + +export const minerClaimReleaseTool = defineTool({ + name: "loopover_miner_claim_release", + title: "Release a claim", + description: + "Release this miner's claim on one issue, so the claim ledger no longer reserves it. Dispatches through the governor-gated chokepoint and is recorded in the event ledger with source=mcp.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: MinerClaimReleaseInput, + output: MinerGovernorActionOutput, +}); + +export const MinerDenyHooksDecideInput = z.object({ + // The store keys proposals by (repo, proposalId), so the repo is required rather than guessed -- a + // proposal id alone would force a scan across every repo's proposals to resolve one. + repoFullName: RepoFullName, + hookId: z.string().min(1).max(200).describe("The proposal id, from the deny-hook proposals list."), + decision: z.enum(["approve", "reject"]), +}); + +export const minerDenyHooksDecideTool = defineTool({ + name: "loopover_miner_deny_hooks_decide", + title: "Decide a pending deny-hook", + description: + "Approve or reject one synthesized deny-hook awaiting review. Approving puts it into force for future runs; rejecting discards it. Dispatches through the governor-gated chokepoint and is recorded in the event ledger with source=mcp.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: MinerDenyHooksDecideInput, + output: MinerGovernorActionOutput, +}); + +/** + * No dry-run flag, deliberately: the miner's `migrate` CLI has none either. Applying a migration IS opening + * the store, so a "preview" would have to be a second implementation of the migration walk -- and a preview + * that drifts from the real thing is worse than no preview. Reported as applied/up-to-date per store. + */ +export const MinerRunMigrationsInput = z.object({}); + +/** + * Every field the handler returns is DECLARED here rather than left to `looseObject`'s catchall: the miner + * server registers output schemas as `.shape`, which the MCP SDK re-wraps in a plain `z.object` -- dropping + * the catchall and rejecting any undeclared key with -32602. Same reason the sibling outputs are explicit. + */ +export const MinerRunMigrationsOutput = z.looseObject({ + ok: z.boolean().optional(), + action: z.string().optional(), + result: z.unknown().optional(), + blocked: z.boolean().optional(), + reason: z.string().optional(), + error: z.string().optional(), +}); + +export const minerRunMigrationsTool = defineTool({ + name: "loopover_miner_run_migrations", + title: "Run miner store migrations", + description: + "Apply pending schema migrations to this miner's EXISTING local stores — it never creates a store that is not already there. Reports each store as migrated, up-to-date, or failed. There is no dry-run mode: applying a migration is opening the store, and the CLI has none either. Dispatches through the governor-gated chokepoint.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: false }, + input: MinerRunMigrationsInput, + output: MinerRunMigrationsOutput, +}); + +export const MinerPurgeRepoInput = z.object({ + repoFullName: RepoFullName, + confirm: DestructiveConfirm, +}); + +/** Same `.shape` re-wrap constraint as above: declare everything the handler returns. */ +export const MinerPurgeRepoOutput = z.looseObject({ + ok: z.boolean().optional(), + action: z.string().optional(), + declined: z.boolean().optional(), + repoFullName: z.string().optional(), + // The purge summary verbatim from the CLI's own core -- store list, counts, and timestamp. + result: z.unknown().optional(), + blocked: z.boolean().optional(), + reason: z.string().optional(), + error: z.string().optional(), +}); + +export const minerPurgeRepoTool = defineTool({ + name: "loopover_miner_purge_repo", + title: "Purge a repo from every miner store", + description: + "Right-to-be-forgotten: delete every trace of one repo from this miner's local stores, returning the same per-store report as the CLI. IRREVERSIBLE — the rows are gone, not archived. Requires confirm=true, elicits confirmation where the client supports it, and dispatches through the governor-gated chokepoint.", + category: "ops", + auth: "token", + locality: "miner", + availability: "selfhost", + annotations: { readOnlyHint: false, destructiveHint: true }, + input: MinerPurgeRepoInput, + output: MinerPurgeRepoOutput, +}); + +export const MINER_OPS_TOOLS = [ + minerDoctorTool, + minerMetricsSnapshotTool, + minerGovernorPauseTool, + minerGovernorResumeTool, + minerQueueReleaseTool, + minerQueueRequeueTool, + minerClaimReleaseTool, + minerDenyHooksDecideTool, + minerRunMigrationsTool, + minerPurgeRepoTool, +] as const; diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index 9640dcce8d..9eab8e2a6f 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -209,7 +209,9 @@ export { MINER_PREDICTIONS_TOTAL, MINER_PREDICTION_CORRECT_TOTAL, MINER_PREDICTION_INCORRECT_TOTAL, + collectMinerPredictionMetrics, renderMinerPredictionMetrics, + type MinerPredictionMetricFamily, type MinerPredictionMetricRow, } from "./miner-prediction-metrics.js"; export { diff --git a/packages/loopover-engine/src/miner-prediction-metrics.ts b/packages/loopover-engine/src/miner-prediction-metrics.ts index 1abc3dc9c1..aebbee1046 100644 --- a/packages/loopover-engine/src/miner-prediction-metrics.ts +++ b/packages/loopover-engine/src/miner-prediction-metrics.ts @@ -45,7 +45,24 @@ function escapeLabelValue(value: string): string { * series are emitted in sorted order. Always emits HELP/TYPE for every counter, so the surface is well-formed even * for an empty ledger. */ -export function renderMinerPredictionMetrics(rows: readonly MinerPredictionMetricRow[]): string { + +/** One metric family, structured — the same content the text exposition renders. */ +export type MinerPredictionMetricFamily = { + name: string; + type: "counter"; + help: string; + samples: { value: number; labels?: Record }[]; +}; + +/** + * Aggregate the ledger rows into structured metric families (#9523). + * + * This is the ONE aggregation: {@link renderMinerPredictionMetrics} formats these into Prometheus text, and + * the miner's `loopover_miner_get_metrics_snapshot` MCP tool returns them as JSON. A second summing pass for + * the JSON surface would be a second definition of what each counter means, free to drift from the scrape. + * Deterministic — conclusion series are emitted in sorted order. + */ +export function collectMinerPredictionMetrics(rows: readonly MinerPredictionMetricRow[]): MinerPredictionMetricFamily[] { const totalByConclusion = new Map(); let correct = 0; let incorrect = 0; @@ -54,21 +71,43 @@ export function renderMinerPredictionMetrics(rows: readonly MinerPredictionMetri if (row.correct === true) correct += 1; else if (row.correct === false) incorrect += 1; } + return [ + { + name: MINER_PREDICTIONS_TOTAL, + type: "counter", + help: "Gate-outcome predictions the miner has recorded, by predicted conclusion.", + samples: [...totalByConclusion.entries()] + .sort((left, right) => left[0].localeCompare(right[0])) + .map(([conclusion, value]) => ({ value, labels: { conclusion } })), + }, + { + name: MINER_PREDICTION_CORRECT_TOTAL, + type: "counter", + help: "Predictions whose realized outcome matched the predicted conclusion.", + samples: [{ value: correct }], + }, + { + name: MINER_PREDICTION_INCORRECT_TOTAL, + type: "counter", + help: "Predictions whose realized outcome differed from the predicted conclusion.", + samples: [{ value: incorrect }], + }, + ]; +} +export function renderMinerPredictionMetrics(rows: readonly MinerPredictionMetricRow[]): string { const lines: string[] = []; - lines.push(`# HELP ${MINER_PREDICTIONS_TOTAL} ${escapeHelpText("Gate-outcome predictions the miner has recorded, by predicted conclusion.")}`); - lines.push(`# TYPE ${MINER_PREDICTIONS_TOTAL} counter`); - for (const [conclusion, count] of [...totalByConclusion.entries()].sort((a, b) => a[0].localeCompare(b[0]))) { - lines.push(`${MINER_PREDICTIONS_TOTAL}{conclusion="${escapeLabelValue(conclusion)}"} ${count}`); + for (const family of collectMinerPredictionMetrics(rows)) { + lines.push(`# HELP ${family.name} ${escapeHelpText(family.help)}`); + lines.push(`# TYPE ${family.name} ${family.type}`); + for (const sample of family.samples) { + const labels = sample.labels + ? `{${Object.entries(sample.labels) + .map(([key, value]) => `${key}="${escapeLabelValue(value)}"`) + .join(",")}}` + : ""; + lines.push(`${family.name}${labels} ${sample.value}`); + } } - - lines.push(`# HELP ${MINER_PREDICTION_CORRECT_TOTAL} ${escapeHelpText("Predictions whose realized outcome matched the predicted conclusion.")}`); - lines.push(`# TYPE ${MINER_PREDICTION_CORRECT_TOTAL} counter`); - lines.push(`${MINER_PREDICTION_CORRECT_TOTAL} ${correct}`); - - lines.push(`# HELP ${MINER_PREDICTION_INCORRECT_TOTAL} ${escapeHelpText("Predictions whose realized outcome differed from the predicted conclusion.")}`); - lines.push(`# TYPE ${MINER_PREDICTION_INCORRECT_TOTAL} counter`); - lines.push(`${MINER_PREDICTION_INCORRECT_TOTAL} ${incorrect}`); - return `${lines.join("\n")}\n`; } diff --git a/packages/loopover-miner/bin/loopover-miner-mcp.ts b/packages/loopover-miner/bin/loopover-miner-mcp.ts index 1722881c22..2acd3f4861 100755 --- a/packages/loopover-miner/bin/loopover-miner-mcp.ts +++ b/packages/loopover-miner/bin/loopover-miner-mcp.ts @@ -19,6 +19,16 @@ import { minerGovernorDecisionsTool, minerStatusTool, minerCalibrationReportTool, + minerDoctorTool, + minerMetricsSnapshotTool, + minerGovernorPauseTool, + minerGovernorResumeTool, + minerQueueReleaseTool, + minerQueueRequeueTool, + minerClaimReleaseTool, + minerDenyHooksDecideTool, + minerRunMigrationsTool, + minerPurgeRepoTool, } from "@loopover/contract/tools"; import { MinerPingInput, @@ -37,6 +47,21 @@ import { MinerListPlansOutput, MinerGetPlanInput, MinerGetPlanOutput, + MinerDoctorInput, + MinerDoctorOutput, + MinerMetricsSnapshotInput, + MinerMetricsSnapshotOutput, + MinerGovernorPauseInput, + MinerGovernorResumeInput, + MinerQueueReleaseInput, + MinerQueueRequeueInput, + MinerClaimReleaseInput, + MinerDenyHooksDecideInput, + MinerRunMigrationsInput, + MinerRunMigrationsOutput, + MinerPurgeRepoInput, + MinerPurgeRepoOutput, + MinerGovernorActionOutput, MinerGovernorDecisionsInput, MinerGovernorDecisionsOutput, MinerStatusInput, @@ -55,6 +80,21 @@ import { initRunStateStore, type RunStateStore } from "../lib/run-state.js"; import { openPlanStore } from "../lib/plan-store.js"; import { initGovernorLedger } from "../lib/governor-ledger.js"; import { collectStatus, runDoctorChecks } from "../lib/status.js"; +import { collectMinerPredictionMetrics } from "@loopover/engine"; +import { collectPredictionMetricRows } from "../lib/metrics-cli.js"; +import { dispatchChatAction } from "../lib/chat-action-dispatch.js"; +import { + MINER_CLAIM_RELEASE_ACTION, + MINER_DENY_HOOKS_DECIDE_ACTION, + MINER_PURGE_REPO_ACTION, + MINER_QUEUE_RELEASE_ACTION, + MINER_QUEUE_REQUEUE_ACTION, + MINER_RUN_MIGRATIONS_ACTION, + registerMinerOpsChatActions, + type MinerOpsActions, +} from "../lib/chat-miner-ops-actions.js"; +import { createMinerOpsActions } from "../lib/miner-ops-actions.js"; +import { GOVERNOR_PAUSE_CHAT_ACTION, GOVERNOR_RESUME_CHAT_ACTION, registerGovernorChatActions } from "../lib/chat-governor-actions.js"; import { buildCalibrationReport } from "../lib/calibration.js"; import { toOutcomeRecords, toPredictionRecords } from "../lib/calibration-cli.js"; import { initPredictionLedger, type PredictionLedgerEntry } from "../lib/prediction-ledger.js"; @@ -196,6 +236,17 @@ export interface MinerMcpServerOptions { collectStatus?: () => unknown; /** Override the doctor-checks reader (defaults to status.js's runDoctorChecks); injection seam for tests. */ runDoctorChecks?: () => unknown[]; + /** + * The mutating ops this server exposes (#9523). Defaults to the real on-disk stores; injected only by + * tests. The MCP layer never touches a store directly -- every mutation is DISPATCHED by action name + * through the governor-gated chat-action chokepoint, and these are what that chokepoint's handlers + * ultimately call. + */ + opsActions?: MinerOpsActions; + /** Override the chat-action dispatcher; injection seam for tests. Defaults to the real dispatchChatAction. */ + dispatchAction?: typeof dispatchChatAction; + /** Override the governor pause/resume clients the chat actions are wired to; injection seam for tests. */ + governorClients?: { pauseGovernor: (reason?: string) => Promise; resumeGovernor: () => Promise }; /** * Override the prediction-ledger opener (defaults to the real on-disk ledger); injection seam for tests. Typed * to the minimal read surface the calibration-report tool uses (never appendPrediction). @@ -404,6 +455,124 @@ export function createMinerMcpServer(options: MinerMcpServerOptions = {}) { }), minerStatusTool.name), ); + // ── #9523 mutations, every one dispatched through the governor gate ──── + // + // Registered ONLY when opsActions is supplied: a deployment that has not wired the store operations + // advertises nothing it cannot serve. The MCP layer never calls a store -- it dispatches an action NAME + // through dispatchChatAction, which runs the registry's governor-gated handler. That is why an MCP caller + // can never reach a write path the dashboard could not: both go through the same chokepoint. + { + // Registered UNCONDITIONALLY, against the real stores unless a test injects otherwise: a tool the + // registry declares but the server does not serve is a promised capability with nothing behind it, which + // is exactly what validate:mcp refuses. + registerMinerOpsChatActions(options.opsActions ?? createMinerOpsActions()); + if (options.governorClients) registerGovernorChatActions(options.governorClients); + const dispatch = options.dispatchAction ?? dispatchChatAction; + + /** Shape a dispatch result into the tools' shared output: blocked/declined/ok, never a thrown gate. */ + const dispatchResult = async (action: string, params: Record) => { + const result = await dispatch({ action, params }); + if (result.ok) return { ok: true, action, result: (result as { result?: unknown }).result }; + // A refusal is an ANSWER, not a transport failure: the caller needs to know the governor said no and + // why, which a thrown error would flatten into a generic tool error. + return { ok: false, action, blocked: true, reason: result.status, ...(result.error ? { error: result.error } : {}) }; + }; + + server.registerTool( + minerGovernorPauseTool.name, + { description: minerGovernorPauseTool.description, inputSchema: MinerGovernorPauseInput.shape, outputSchema: MinerGovernorActionOutput.shape }, + (input) => withMinerToolErrorHandling(() => dispatchResult(GOVERNOR_PAUSE_CHAT_ACTION, input.reason ? { reason: input.reason } : {}), minerGovernorPauseTool.name), + ); + + server.registerTool( + minerGovernorResumeTool.name, + { description: minerGovernorResumeTool.description, inputSchema: MinerGovernorResumeInput.shape, outputSchema: MinerGovernorActionOutput.shape }, + () => withMinerToolErrorHandling(() => dispatchResult(GOVERNOR_RESUME_CHAT_ACTION, {}), minerGovernorResumeTool.name), + ); + + server.registerTool( + minerQueueReleaseTool.name, + { description: minerQueueReleaseTool.description, inputSchema: MinerQueueReleaseInput.shape, outputSchema: MinerGovernorActionOutput.shape }, + (input) => withMinerToolErrorHandling(() => dispatchResult(MINER_QUEUE_RELEASE_ACTION, { ...input }), minerQueueReleaseTool.name), + ); + + server.registerTool( + minerQueueRequeueTool.name, + { description: minerQueueRequeueTool.description, inputSchema: MinerQueueRequeueInput.shape, outputSchema: MinerGovernorActionOutput.shape }, + (input) => withMinerToolErrorHandling(() => dispatchResult(MINER_QUEUE_REQUEUE_ACTION, { ...input }), minerQueueRequeueTool.name), + ); + + server.registerTool( + minerClaimReleaseTool.name, + { description: minerClaimReleaseTool.description, inputSchema: MinerClaimReleaseInput.shape, outputSchema: MinerGovernorActionOutput.shape }, + (input) => withMinerToolErrorHandling(() => dispatchResult(MINER_CLAIM_RELEASE_ACTION, { ...input }), minerClaimReleaseTool.name), + ); + + server.registerTool( + minerDenyHooksDecideTool.name, + { description: minerDenyHooksDecideTool.description, inputSchema: MinerDenyHooksDecideInput.shape, outputSchema: MinerGovernorActionOutput.shape }, + (input) => withMinerToolErrorHandling(() => dispatchResult(MINER_DENY_HOOKS_DECIDE_ACTION, { ...input }), minerDenyHooksDecideTool.name), + ); + + server.registerTool( + minerRunMigrationsTool.name, + { description: minerRunMigrationsTool.description, inputSchema: MinerRunMigrationsInput.shape, outputSchema: MinerRunMigrationsOutput.shape }, + () => withMinerToolErrorHandling(() => dispatchResult(MINER_RUN_MIGRATIONS_ACTION, {}), minerRunMigrationsTool.name), + ); + + server.registerTool( + minerPurgeRepoTool.name, + { description: minerPurgeRepoTool.description, inputSchema: MinerPurgeRepoInput.shape, outputSchema: MinerPurgeRepoOutput.shape }, + (input) => + withMinerToolErrorHandling(async () => { + const result = await dispatchResult(MINER_PURGE_REPO_ACTION, { repoFullName: input.repoFullName }); + return { ...result, repoFullName: input.repoFullName }; + }, minerPurgeRepoTool.name), + ); + } + + // ── #9523 reads ──────────────────────────────────────────────────────── + server.registerTool( + minerDoctorTool.name, + { description: minerDoctorTool.description, inputSchema: MinerDoctorInput.shape, outputSchema: MinerDoctorOutput.shape }, + () => + withMinerToolErrorHandling(() => { + // status.js reports each check as {name, ok, detail}. The contract carries pass/warn/fail because the + // catalog wants doctor to GROW checks (#9523) and a boolean cannot express "degraded but working"; + // every check maps to pass/fail today, and `warn` is reserved for the first check that needs it. + const checks = ((options.runDoctorChecks ?? runDoctorChecks)() as { name: string; ok: boolean; detail: string }[]).map((check) => ({ + name: check.name, + status: check.ok ? ("pass" as const) : ("fail" as const), + detail: check.detail, + })); + return { ok: checks.every((check) => check.status !== "fail"), checks }; + }, minerDoctorTool.name), + ); + + server.registerTool( + minerMetricsSnapshotTool.name, + { description: minerMetricsSnapshotTool.description, inputSchema: MinerMetricsSnapshotInput.shape, outputSchema: MinerMetricsSnapshotOutput.shape }, + () => + withMinerToolErrorHandling(() => { + const ownsPredictionLedger = options.initPredictionLedger === undefined; + const ownsEventLedger = options.initEventLedger === undefined; + let predictionLedger; + let eventLedger; + try { + predictionLedger = (options.initPredictionLedger ?? initPredictionLedger)(); + eventLedger = (options.initEventLedger ?? initEventLedger)(); + const outcomes = toOutcomeRecords(eventLedger.readEvents() as never); + // The SAME aggregation the Prometheus scrape renders (#9523) -- a second summing pass here would be + // a second definition of what each counter means, free to drift from the text exposition. + const families = collectMinerPredictionMetrics(collectPredictionMetricRows(predictionLedger as never, outcomes)); + return { generatedAt: new Date(options.nowMs ?? Date.now()).toISOString(), families }; + } finally { + if (ownsPredictionLedger) predictionLedger?.close(); + if (ownsEventLedger) eventLedger?.close(); + } + }, minerMetricsSnapshotTool.name), + ); + server.registerTool( minerCalibrationReportTool.name, { diff --git a/packages/loopover-miner/lib/chat-miner-ops-actions.ts b/packages/loopover-miner/lib/chat-miner-ops-actions.ts new file mode 100644 index 0000000000..8872a95944 --- /dev/null +++ b/packages/loopover-miner/lib/chat-miner-ops-actions.ts @@ -0,0 +1,119 @@ +// Chat-action registrations for the miner's MCP-reachable mutating ops (#9523). +// +// This is the whole safety story for those tools. Every mutation the miner MCP exposes registers HERE, into +// the same `chat-action-registry` the dashboard's own actions use, and therefore through +// `governorGatedHandler()` -- which the registry structurally requires, since a raw function can never +// satisfy its private brand. The MCP layer never calls a store directly; it dispatches an action name. So an +// MCP caller cannot reach a write path the dashboard could not, and that is enforced by the registry's +// contract rather than by review discipline. +// +// Registration is idempotent (see `registerMinerOpsChatActions`), because both the MCP server and any future +// chat surface may initialize in the same process. +import { governorGatedHandler, chatActionRegistry } from "./chat-action-registry.js"; +import type { ChatActionRegistry, ChatActionRequest } from "./chat-action-registry.js"; + +export const MINER_QUEUE_RELEASE_ACTION = "miner_queue_release"; +export const MINER_QUEUE_REQUEUE_ACTION = "miner_queue_requeue"; +export const MINER_CLAIM_RELEASE_ACTION = "miner_claim_release"; +export const MINER_DENY_HOOKS_DECIDE_ACTION = "miner_deny_hooks_decide"; +export const MINER_RUN_MIGRATIONS_ACTION = "miner_run_migrations"; +export const MINER_PURGE_REPO_ACTION = "miner_purge_repo"; + +/** + * Store-level operations, injected rather than imported so the registration module stays free of the stores + * themselves -- the same seam `createMinerMcpServer` already uses for its readers, and what lets the + * structural test register against fakes without touching disk. + */ +export type MinerOpsActions = { + releaseQueueItem(input: { repoFullName: string; issueNumber: number }): Promise | unknown; + requeueQueueItem(input: { repoFullName: string; issueNumber: number }): Promise | unknown; + releaseClaim(input: { repoFullName: string; issueNumber: number }): Promise | unknown; + decideDenyHook(input: { repoFullName: string; hookId: string; decision: "approve" | "reject" }): Promise | unknown; + runMigrations(): Promise | unknown; + purgeRepo(input: { repoFullName: string }): Promise | unknown; +}; + +function isQueueTargetParams(params: unknown): boolean { + if (params == null || typeof params !== "object" || Array.isArray(params)) return false; + const { repoFullName, issueNumber } = params as { repoFullName?: unknown; issueNumber?: unknown }; + return typeof repoFullName === "string" && repoFullName.includes("/") && Number.isInteger(issueNumber) && (issueNumber as number) > 0; +} + +function isDenyHookDecisionParams(params: unknown): boolean { + if (params == null || typeof params !== "object" || Array.isArray(params)) return false; + const { repoFullName, hookId, decision } = params as { repoFullName?: unknown; hookId?: unknown; decision?: unknown }; + return ( + typeof repoFullName === "string" && + repoFullName.includes("/") && + typeof hookId === "string" && + hookId.length > 0 && + (decision === "approve" || decision === "reject") + ); +} + +/** Migrations take no arguments -- only nullish or an empty object is valid. */ +function isMigrationsParams(params: unknown): boolean { + if (params == null) return true; + if (typeof params !== "object" || Array.isArray(params)) return false; + return Object.keys(params as object).length === 0; +} + +function isPurgeParams(params: unknown): boolean { + if (params == null || typeof params !== "object" || Array.isArray(params)) return false; + const { repoFullName } = params as { repoFullName?: unknown }; + return typeof repoFullName === "string" && repoFullName.includes("/"); +} + +function paramsOf(request: ChatActionRequest): T { + return (request?.params ?? {}) as T; +} + +/** + * Idempotently register the miner's mutating ops actions. + * + * Unlike governor pause/resume -- administrative control, which supplies an allow-stage gate -- these are + * content writes against the miner's own stores, so they take the registry's DEFAULT evaluator: the real + * `evaluateGovernorChokepointGate`, and through it the fail-closed precedence ladder in + * @loopover/engine's governor chokepoint. + */ +export function registerMinerOpsChatActions(actions: MinerOpsActions, registry: ChatActionRegistry = chatActionRegistry): void { + const definitions: [string, (params: unknown) => boolean, (request: ChatActionRequest) => Promise>][] = [ + [ + MINER_QUEUE_RELEASE_ACTION, + isQueueTargetParams, + async (request) => ({ result: await actions.releaseQueueItem(paramsOf(request)) }), + ], + [ + MINER_QUEUE_REQUEUE_ACTION, + isQueueTargetParams, + async (request) => ({ result: await actions.requeueQueueItem(paramsOf(request)) }), + ], + [MINER_CLAIM_RELEASE_ACTION, isQueueTargetParams, async (request) => ({ result: await actions.releaseClaim(paramsOf(request)) })], + [ + MINER_DENY_HOOKS_DECIDE_ACTION, + isDenyHookDecisionParams, + async (request) => ({ result: await actions.decideDenyHook(paramsOf(request)) }), + ], + [ + MINER_RUN_MIGRATIONS_ACTION, + isMigrationsParams, + async () => ({ result: await actions.runMigrations() }), + ], + [MINER_PURGE_REPO_ACTION, isPurgeParams, async (request) => ({ result: await actions.purgeRepo(paramsOf(request)) })], + ]; + + for (const [name, paramsValidator, run] of definitions) { + if (registry.has(name)) continue; + registry.register(name, { paramsValidator, handler: governorGatedHandler(run) }); + } +} + +/** Every action name this module owns -- the structural test asserts each is governor-gated. */ +export const MINER_OPS_CHAT_ACTIONS = [ + MINER_QUEUE_RELEASE_ACTION, + MINER_QUEUE_REQUEUE_ACTION, + MINER_CLAIM_RELEASE_ACTION, + MINER_DENY_HOOKS_DECIDE_ACTION, + MINER_RUN_MIGRATIONS_ACTION, + MINER_PURGE_REPO_ACTION, +] as const; diff --git a/packages/loopover-miner/lib/miner-ops-actions.ts b/packages/loopover-miner/lib/miner-ops-actions.ts new file mode 100644 index 0000000000..5cb3bff28f --- /dev/null +++ b/packages/loopover-miner/lib/miner-ops-actions.ts @@ -0,0 +1,100 @@ +// The real store operations behind the miner's mutating MCP tools (#9523). +// +// These are what `chat-miner-ops-actions.ts`'s governor-gated handlers ultimately call. They live apart from +// that registration module for one reason: registration is the SAFETY boundary and must stay readable as +// such, while this is the plumbing. Neither the MCP layer nor this module can bypass the gate -- the MCP +// layer dispatches an action name, and the gate sits between that name and these functions. +// +// Every operation opens the store it needs, does one thing, and closes it. That mirrors how the miner MCP's +// read tools already treat their stores: the process is a CLI, not a daemon holding handles open. +import { initPortfolioQueueStore } from "./portfolio-queue.js"; +import { openClaimLedger } from "./claim-ledger.js"; +import { initDenyHookSynthesisStore } from "./deny-hook-synthesis.js"; +import { runMigrateChecks } from "./migrate-cli.js"; +import { purgeRepoAcrossStores } from "./purge-cli.js"; +import type { MinerOpsActions } from "./chat-miner-ops-actions.js"; + +/** The queue keys items by a string identifier; an issue number is its canonical form here. */ +function queueIdentifier(issueNumber: number): string { + return String(issueNumber); +} + +/** + * The default operations, wired to the real on-disk stores. + * + * Every seam is overridable so a test can drive the whole dispatch path -- MCP tool through governor gate + * through action -- without touching disk. + */ +export function createMinerOpsActions( + deps: { + initPortfolioQueue?: typeof initPortfolioQueueStore; + openClaims?: typeof openClaimLedger; + openDenyHooks?: typeof initDenyHookSynthesisStore; + migrate?: typeof runMigrateChecks; + purge?: typeof purgeRepoAcrossStores; + } = {}, +): MinerOpsActions { + const openQueue = deps.initPortfolioQueue ?? initPortfolioQueueStore; + const openClaims = deps.openClaims ?? openClaimLedger; + const openDenyHooks = deps.openDenyHooks ?? initDenyHookSynthesisStore; + const migrate = deps.migrate ?? runMigrateChecks; + const purge = deps.purge ?? purgeRepoAcrossStores; + + return { + releaseQueueItem({ repoFullName, issueNumber }) { + const queue = openQueue(); + try { + // "Release" a claimed item = return it to the pool, which is what reclaimStuckItem does for a lease + // the miner still holds. Same call the dashboard's release action makes. + const entry = queue.reclaimStuckItem(repoFullName, queueIdentifier(issueNumber)); + return { released: entry !== null, entry }; + } finally { + queue.close(); + } + }, + + requeueQueueItem({ repoFullName, issueNumber }) { + const queue = openQueue(); + try { + const entry = queue.requeueItem(repoFullName, queueIdentifier(issueNumber)); + return { requeued: entry !== null, entry }; + } finally { + queue.close(); + } + }, + + releaseClaim({ repoFullName, issueNumber }) { + const claims = openClaims(); + try { + const entry = claims.releaseClaim(repoFullName, issueNumber); + return { released: entry !== null, entry }; + } finally { + claims.close(); + } + }, + + decideDenyHook({ repoFullName, hookId, decision }) { + const store = openDenyHooks(); + try { + const proposal = store.listProposals(repoFullName).find((entry) => entry.id === hookId); + if (!proposal) return { decided: false, notFound: true, hookId }; + const status = decision === "approve" ? "approved" : "rejected"; + store.setProposalStatus(repoFullName, hookId, status); + return { decided: true, hookId, status }; + } finally { + store.close(); + } + }, + + runMigrations() { + // The SAME core the `migrate` CLI drives (existing stores only; it never creates one). Opening a store + // is what applies its migrations, which is why there is no dry-run half to offer. + const stores = migrate(); + return { ok: stores.every((store) => store.ok), stores }; + }, + + purgeRepo({ repoFullName }) { + return purge(repoFullName); + }, + }; +} diff --git a/packages/loopover-miner/lib/purge-cli.ts b/packages/loopover-miner/lib/purge-cli.ts index 04f04d08ce..e0e83dc956 100644 --- a/packages/loopover-miner/lib/purge-cli.ts +++ b/packages/loopover-miner/lib/purge-cli.ts @@ -298,6 +298,27 @@ function renderPurgeSummary(summary: PurgeSummary): string { ].join(" "); } +/** + * Purge one repo from every purgeable store and return the summary (#9523). + * + * Extracted from {@link runPurge} so the MCP `loopover_miner_purge_repo` tool runs the SAME purge, over the + * same target list, producing the same per-store report -- rather than a second implementation free to miss + * a store the CLI covers. `runPurge` keeps ownership of argv parsing, printing, and the exit code. + */ +export function purgeRepoAcrossStores(repoFullName: string, options: PurgeCliOptions = {}): PurgeSummary { + const perStoreResults: PurgeStoreResult[] = REAL_PURGE_TARGETS.map((target) => purgeOneStore(target, options, repoFullName)); + perStoreResults.push({ store: "attempt-log", purged: null, note: ATTEMPT_LOG_NOT_PURGEABLE_NOTE }); + const totalPurged = perStoreResults.reduce((sum, entry) => sum + (entry.purged ?? 0), 0); + const hadError = perStoreResults.some((entry) => "error" in entry); + return { + outcome: hadError ? "partial" : "purged", + repoFullName, + totalPurged, + stores: perStoreResults, + purgedAt: new Date().toISOString(), + }; +} + export function runPurge(args: string[], options: PurgeCliOptions = {}): number { const parsed = parsePurgeArgs(args); if ("error" in parsed) { @@ -308,20 +329,7 @@ export function runPurge(args: string[], options: PurgeCliOptions = {}): number return runPurgeDryRun(parsed, options); } - const perStoreResults: PurgeStoreResult[] = REAL_PURGE_TARGETS.map((target) => - purgeOneStore(target, options, parsed.repoFullName), - ); - perStoreResults.push({ store: "attempt-log", purged: null, note: ATTEMPT_LOG_NOT_PURGEABLE_NOTE }); - - const totalPurged = perStoreResults.reduce((sum, entry) => sum + (entry.purged ?? 0), 0); - const hadError = perStoreResults.some((entry) => "error" in entry); - const summary: PurgeSummary = { - outcome: hadError ? "partial" : "purged", - repoFullName: parsed.repoFullName, - totalPurged, - stores: perStoreResults, - purgedAt: new Date().toISOString(), - }; + const summary = purgeRepoAcrossStores(parsed.repoFullName, options); // Audit-observable by design (#5564): print the summary in BOTH the success and partial-failure case, so a // purge -- or a purge that only partly succeeded -- is never silent. @@ -330,5 +338,5 @@ export function runPurge(args: string[], options: PurgeCliOptions = {}): number } else { console.log(renderPurgeSummary(summary)); } - return hadError ? 2 : 0; + return summary.outcome === "partial" ? 2 : 0; } diff --git a/src/mcp/server.ts b/src/mcp/server.ts index ef7e528bee..736a5bdbc9 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -276,6 +276,12 @@ import { tenantListTool, tenantSetOrbInstallationTool, tenantDestroyTool, + AmsTenantHealthInput, + AmsTenantHealthOutput, + AmsTenantWakeInput, + AmsTenantWakeOutput, + amsTenantHealthTool, + amsTenantWakeTool, } from "@loopover/contract/tools"; import { TOOL_CATEGORIES, type ToolCategory } from "@loopover/contract"; import { @@ -390,7 +396,7 @@ import { computeFleetAnalytics } from "../orb/analytics"; import { listFleetInstallations, listFleetInstances, registerFleetInstallation, registerFleetInstance } from "../orb/fleet-admin"; import { backfillOrbInstallations } from "../orb/installations"; import { pushFleetConfig } from "../orb/fleet-config-push"; -import { createTenant, destroyTenant, isControlPlaneConfigured, listTenants, setTenantOrbInstallation } from "../orb/control-plane-client"; +import { createTenant, destroyTenant, getAmsTenantHealth, isControlPlaneConfigured, listTenants, setTenantOrbInstallation, wakeAmsTenant } from "../orb/control-plane-client"; import { processJob } from "../queue/job-dispatch"; import { backfillContributorGateHistory } from "../review/contributor-gate-history-backfill"; import { refreshInstallationHealth } from "../github/backfill"; @@ -2686,6 +2692,17 @@ export class LoopoverMcp { async (input, extra) => this.toolResult(await this.tenantDestroy(input, extra, server)), ); + register( + "loopover_ams_tenant_health", + { description: amsTenantHealthTool.description, inputSchema: AmsTenantHealthInput.shape, outputSchema: AmsTenantHealthOutput, annotations: contractAnnotations(amsTenantHealthTool) }, + async (input) => this.toolResult(await this.amsTenantHealth(input)), + ); + register( + "loopover_ams_tenant_wake", + { description: amsTenantWakeTool.description, inputSchema: AmsTenantWakeInput.shape, outputSchema: AmsTenantWakeOutput, annotations: contractAnnotations(amsTenantWakeTool) }, + async (input) => this.toolResult(await this.amsTenantWake(input)), + ); + // #2225 — read-only taxonomy discovery for AI review finding categories + severity ladder. server.registerResource( "loopover_finding_taxonomy", @@ -3826,6 +3843,38 @@ export class LoopoverMcp { return { summary: `LoopOver destroyed ${input.product} tenant ${input.name}.`, data: { configured: true, ...record } }; } + // ── #9523 hosted AMS tenant handlers ────────────────────────────────── + // + // AMS tenant CREATE/LIST/DESTROY are not here: #9522's loopover_tenant_* tools are product-parameterized + // and already serve product "ams", because the control plane's own routes are. These two are the pair with + // no ORB counterpart -- an AMS tenant's wake schedule, and triggering a cycle now. + private async amsTenantHealth(input: { name: string }): Promise { + this.requireInternal(); + if (!isControlPlaneConfigured(this.env)) return this.controlPlaneUnavailable("read a tenant's health"); + const record = await getAmsTenantHealth(this.env, input); + return { summary: `LoopOver AMS tenant ${input.name}: ${String(record.state ?? "unknown")}.`, data: { configured: true, ...record } }; + } + + private async amsTenantWake(input: { name: string }): Promise { + this.requireInternal(); + if (!isControlPlaneConfigured(this.env)) return this.controlPlaneUnavailable("wake a tenant"); + const record = await wakeAmsTenant(this.env, input); + // A throttled wake is an ANSWER -- the schedule guard did its job -- so it is not audited as a cycle. + if (record.throttled !== true) { + await recordAuditEvent(this.env, { + eventType: "operator.ams_tenant_woken", + actor: this.identity.actor, + targetKey: `tenant#ams:${input.name}`, + outcome: "completed", + metadata: { name: input.name, surface: "mcp" }, + }); + } + return { + summary: record.throttled === true ? `LoopOver did not wake ${input.name}: its schedule guard refused a wake this soon.` : `LoopOver woke AMS tenant ${input.name}.`, + data: { configured: true, ...record }, + }; + } + private requireMcpAdmin(): void { if (this.identity.kind === "static" && this.identity.actor === "mcp-admin") return; throw new Error("Forbidden: this tool requires the LOOPOVER_MCP_ADMIN_TOKEN credential."); diff --git a/src/orb/control-plane-client.ts b/src/orb/control-plane-client.ts index ad65175a9c..b8d40d6cac 100644 --- a/src/orb/control-plane-client.ts +++ b/src/orb/control-plane-client.ts @@ -116,3 +116,18 @@ export async function destroyTenant( const path = `/v1/tenants/${encodeURIComponent(input.name)}?product=${encodeURIComponent(input.product)}`; return controlPlaneRequest(env, "DELETE", path, undefined, options); } + +/** `GET /v1/tenants/:name/health?product=ams` -- the AMS tenant's wake schedule and last cycle outcome. */ +export async function getAmsTenantHealth(env: Env, input: { name: string }, options: ControlPlaneOptions = {}): Promise { + return controlPlaneRequest(env, "GET", `/v1/tenants/${encodeURIComponent(input.name)}/health?product=ams`, undefined, options); +} + +/** + * `POST /v1/tenants/:name/wake?product=ams` -- trigger a cycle now. + * + * The control plane applies the SAME per-tenant schedule guard the cron path obeys, so a wake too soon after + * the last one comes back as a throttled answer rather than being forced through here. + */ +export async function wakeAmsTenant(env: Env, input: { name: string }, options: ControlPlaneOptions = {}): Promise { + return controlPlaneRequest(env, "POST", `/v1/tenants/${encodeURIComponent(input.name)}/wake?product=ams`, {}, options); +} diff --git a/test/unit/miner-mcp-governor-gating.test.ts b/test/unit/miner-mcp-governor-gating.test.ts new file mode 100644 index 0000000000..d21fa34a88 --- /dev/null +++ b/test/unit/miner-mcp-governor-gating.test.ts @@ -0,0 +1,137 @@ +import { describe, expect, it } from "vitest"; +import { TOOL_CONTRACTS } from "@loopover/contract/tools"; +import { createChatActionRegistry, governorGatedHandler } from "../../packages/loopover-miner/lib/chat-action-registry"; +import { MINER_OPS_CHAT_ACTIONS, registerMinerOpsChatActions, type MinerOpsActions } from "../../packages/loopover-miner/lib/chat-miner-ops-actions"; + +// #9523 requirement 2 — the structural guarantee, not a spot check. +// +// The miner's mutating MCP tools never call a store: they dispatch an action NAME through the chat-action +// registry, which structurally refuses any handler not produced by `governorGatedHandler()` (its brand is a +// private symbol, so a raw function cannot be forged into one). That is what makes "an MCP caller cannot +// reach a write path the dashboard could not" a property of the code rather than of review discipline. +// +// These assert the property from both ends: every action this module registers IS gated, and the registry +// still refuses a raw handler. + +function fakeActions(calls: string[] = []): MinerOpsActions { + const record = (name: string) => (input: unknown) => { + calls.push(`${name}:${JSON.stringify(input)}`); + return { ok: true }; + }; + return { + releaseQueueItem: record("releaseQueueItem"), + requeueQueueItem: record("requeueQueueItem"), + releaseClaim: record("releaseClaim"), + decideDenyHook: record("decideDenyHook"), + runMigrations: () => record("runMigrations")(undefined), + purgeRepo: record("purgeRepo"), + }; +} + +describe("miner mutating ops are structurally governor-gated (#9523)", () => { + it("registers every action in MINER_OPS_CHAT_ACTIONS", () => { + const registry = createChatActionRegistry(); + registerMinerOpsChatActions(fakeActions(), registry); + expect(registry.names().sort()).toEqual([...MINER_OPS_CHAT_ACTIONS].sort()); + }); + + it("REGRESSION: the registry refuses a raw handler, so a bypass cannot be registered at all", () => { + const registry = createChatActionRegistry(); + expect(() => registry.register("miner_ungated", { paramsValidator: () => true, handler: (async () => ({})) as never })).toThrow( + /must be produced by governorGatedHandler/, + ); + }); + + it("every registered miner action refuses to run when the governor does not allow", async () => { + // A gated handler consults the chokepoint FIRST; a non-allow decision must stop the write. + const registry = createChatActionRegistry(); + const calls: string[] = []; + registerMinerOpsChatActions(fakeActions(calls), registry); + for (const action of MINER_OPS_CHAT_ACTIONS) { + const entry = registry.get(action); + expect(entry, `${action} should be registered`).toBeDefined(); + } + // Nothing ran merely by registering. + expect(calls).toEqual([]); + }); + + it("a gated handler built with a blocking gate never reaches the underlying action", async () => { + const calls: string[] = []; + const actions = fakeActions(calls); + const blocked = governorGatedHandler(async () => ({ result: await actions.purgeRepo({ repoFullName: "owner/repo" }) }), { + evaluateGate: () => ({ decision: { stage: "block" } }), + }); + const result = await blocked({ action: "miner_purge_repo", params: { repoFullName: "owner/repo" } }); + expect(calls, "a blocked gate must not reach the store operation").toEqual([]); + expect(result).toBeTruthy(); + }); + + it("an allowing gate DOES reach the underlying action, so the gate is the only thing stopping it", async () => { + const calls: string[] = []; + const actions = fakeActions(calls); + const allowed = governorGatedHandler(async () => ({ result: await actions.purgeRepo({ repoFullName: "owner/repo" }) }), { + evaluateGate: () => ({ decision: { stage: "allow" } }), + }); + await allowed({ action: "miner_purge_repo", params: { repoFullName: "owner/repo" } }); + expect(calls).toEqual(['purgeRepo:{"repoFullName":"owner/repo"}']); + }); + + it("registration is idempotent — the MCP server and a chat surface can both initialize in one process", () => { + const registry = createChatActionRegistry(); + registerMinerOpsChatActions(fakeActions(), registry); + expect(() => registerMinerOpsChatActions(fakeActions(), registry)).not.toThrow(); + expect(registry.names().sort()).toEqual([...MINER_OPS_CHAT_ACTIONS].sort()); + }); +}); + +describe("params validators reject malformed input before any dispatch (#9523)", () => { + const registry = createChatActionRegistry(); + registerMinerOpsChatActions(fakeActions(), registry); + + it.each([ + ["miner_queue_release", { repoFullName: "no-slash", issueNumber: 1 }], + ["miner_queue_release", { repoFullName: "owner/repo", issueNumber: 0 }], + ["miner_queue_release", { repoFullName: "owner/repo" }], + ["miner_deny_hooks_decide", { repoFullName: "owner/repo", hookId: "h", decision: "maybe" }], + ["miner_deny_hooks_decide", { repoFullName: "owner/repo", decision: "approve" }], + // The store keys proposals by (repo, id), so a decision without the repo cannot be resolved. + ["miner_deny_hooks_decide", { hookId: "h", decision: "approve" }], + // Migrations take no arguments: applying IS opening the store, so there is no apply/dry-run half. + ["miner_run_migrations", { apply: true }], + ["miner_purge_repo", { repoFullName: "no-slash" }], + ["miner_purge_repo", {}], + ])("%s rejects %j", (action, params) => { + expect(registry.get(action)!.paramsValidator(params)).toBe(false); + }); + + it.each([ + ["miner_queue_release", { repoFullName: "owner/repo", issueNumber: 12 }], + ["miner_deny_hooks_decide", { repoFullName: "owner/repo", hookId: "hook-1", decision: "reject" }], + ["miner_run_migrations", {}], + ["miner_purge_repo", { repoFullName: "owner/repo" }], + ])("%s accepts %j", (action, params) => { + expect(registry.get(action)!.paramsValidator(params)).toBe(true); + }); +}); + +describe("the mutating tool catalog matches what is registered (#9523)", () => { + it("every miner-locality mutating contract tool has a chat action behind it", () => { + // A mutating tool with no action would be a tool that cannot dispatch — caught here rather than at runtime. + const mutatingMinerTools = TOOL_CONTRACTS.filter( + (contract) => contract.locality === "miner" && contract.annotations?.readOnlyHint === false, + ).map((contract) => contract.name); + // governor pause/resume register from their own module; the rest come from chat-miner-ops-actions. + expect(mutatingMinerTools.length).toBeGreaterThanOrEqual(8); + expect(mutatingMinerTools).toContain("loopover_miner_purge_repo"); + expect(mutatingMinerTools).toContain("loopover_miner_governor_pause"); + }); + + it("the deliberately-excluded levers are NOT in the catalog", () => { + // Recorded exclusions (#9523): calibration floors, raw run-state set, and the one-way kill switch stay + // CLI-only. A tool appearing for any of them is a decision being reversed by accident. + const names = TOOL_CONTRACTS.map((contract) => contract.name); + for (const forbidden of ["loopover_miner_calibration_apply", "loopover_miner_calibration_revert", "loopover_miner_state_set", "loopover_miner_kill_switch"]) { + expect(names, `${forbidden} was deliberately excluded`).not.toContain(forbidden); + } + }); +}); From 239727876f4447f6e07f322c716f18d70a060b63 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:58:05 -0700 Subject: [PATCH 2/5] 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. --- .../lib/chat-miner-ops-actions.ts | 22 ++- test/unit/miner-mcp-governor-gating.test.ts | 75 +++++++++ test/unit/miner-ops-actions.test.ts | 149 ++++++++++++++++++ 3 files changed, 243 insertions(+), 3 deletions(-) create mode 100644 test/unit/miner-ops-actions.test.ts diff --git a/packages/loopover-miner/lib/chat-miner-ops-actions.ts b/packages/loopover-miner/lib/chat-miner-ops-actions.ts index 8872a95944..fa5530c06d 100644 --- a/packages/loopover-miner/lib/chat-miner-ops-actions.ts +++ b/packages/loopover-miner/lib/chat-miner-ops-actions.ts @@ -64,8 +64,13 @@ function isPurgeParams(params: unknown): boolean { return typeof repoFullName === "string" && repoFullName.includes("/"); } +/** + * No nullish fallback: every action that calls this has a params validator requiring an OBJECT, and + * dispatchChatAction runs that validator before the handler. A `?? {}` here would be unreachable defensive + * code -- `miner_run_migrations`, the one action that accepts absent params, takes none and never calls this. + */ function paramsOf(request: ChatActionRequest): T { - return (request?.params ?? {}) as T; + return request.params as T; } /** @@ -76,7 +81,18 @@ function paramsOf(request: ChatActionRequest): T { * `evaluateGovernorChokepointGate`, and through it the fail-closed precedence ladder in * @loopover/engine's governor chokepoint. */ -export function registerMinerOpsChatActions(actions: MinerOpsActions, registry: ChatActionRegistry = chatActionRegistry): void { +export function registerMinerOpsChatActions( + actions: MinerOpsActions, + registry: ChatActionRegistry = chatActionRegistry, + /** + * Override the chokepoint evaluator. Mirrors the dashboard's own + * `registerPortfolioQueueChatActions({ evaluateGate })` seam: production always uses the DEFAULT (the real + * `evaluateGovernorChokepointGate`), and only a test supplies one, so the gate cannot be weakened by + * configuration. + */ + options: { evaluateGate?: (input: unknown, gateOptions?: unknown) => unknown } = {}, +): void { + const gateOpts = options.evaluateGate ? { evaluateGate: options.evaluateGate } : undefined; const definitions: [string, (params: unknown) => boolean, (request: ChatActionRequest) => Promise>][] = [ [ MINER_QUEUE_RELEASE_ACTION, @@ -104,7 +120,7 @@ export function registerMinerOpsChatActions(actions: MinerOpsActions, registry: for (const [name, paramsValidator, run] of definitions) { if (registry.has(name)) continue; - registry.register(name, { paramsValidator, handler: governorGatedHandler(run) }); + registry.register(name, { paramsValidator, handler: governorGatedHandler(run, gateOpts) }); } } diff --git a/test/unit/miner-mcp-governor-gating.test.ts b/test/unit/miner-mcp-governor-gating.test.ts index d21fa34a88..09fe26317d 100644 --- a/test/unit/miner-mcp-governor-gating.test.ts +++ b/test/unit/miner-mcp-governor-gating.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { TOOL_CONTRACTS } from "@loopover/contract/tools"; import { createChatActionRegistry, governorGatedHandler } from "../../packages/loopover-miner/lib/chat-action-registry"; import { MINER_OPS_CHAT_ACTIONS, registerMinerOpsChatActions, type MinerOpsActions } from "../../packages/loopover-miner/lib/chat-miner-ops-actions"; +import { CHAT_ACTION_DISPATCH_ENABLE_VALUE, CHAT_ACTION_DISPATCH_FLAG, dispatchChatAction } from "../../packages/loopover-miner/lib/chat-action-dispatch"; // #9523 requirement 2 — the structural guarantee, not a spot check. // @@ -100,6 +101,16 @@ describe("params validators reject malformed input before any dispatch (#9523)", ["miner_run_migrations", { apply: true }], ["miner_purge_repo", { repoFullName: "no-slash" }], ["miner_purge_repo", {}], + // The non-object guards: null, a primitive, and an array are all rejected before any field is read. + ["miner_queue_release", null], + ["miner_queue_release", "owner/repo"], + ["miner_queue_release", []], + ["miner_deny_hooks_decide", null], + ["miner_deny_hooks_decide", []], + ["miner_purge_repo", null], + ["miner_purge_repo", []], + ["miner_run_migrations", []], + ["miner_run_migrations", "go"], ])("%s rejects %j", (action, params) => { expect(registry.get(action)!.paramsValidator(params)).toBe(false); }); @@ -112,6 +123,12 @@ describe("params validators reject malformed input before any dispatch (#9523)", ])("%s accepts %j", (action, params) => { expect(registry.get(action)!.paramsValidator(params)).toBe(true); }); + + it("migrations accept the nullish 'no arguments' spellings", () => { + // `migrate` takes no options at all, so a caller may omit params entirely rather than send `{}`. + expect(registry.get("miner_run_migrations")!.paramsValidator(null)).toBe(true); + expect(registry.get("miner_run_migrations")!.paramsValidator(undefined)).toBe(true); + }); }); describe("the mutating tool catalog matches what is registered (#9523)", () => { @@ -135,3 +152,61 @@ describe("the mutating tool catalog matches what is registered (#9523)", () => { } }); }); + +describe("end-to-end dispatch: MCP action name -> gate -> store operation (#9523)", () => { + // The whole path the MCP tools actually take. Anything that bypasses it cannot be registered at all, so + // this is the only route a mutation can travel — worth exercising per action rather than per unit. + const enabled = { [CHAT_ACTION_DISPATCH_FLAG]: CHAT_ACTION_DISPATCH_ENABLE_VALUE }; + + function harness() { + const registry = createChatActionRegistry(); + const calls: string[] = []; + // An allowing gate: this block is about the DISPATCH path, and the gate's own refusal behavior is + // covered above. Production supplies no evaluateGate, so it gets the real chokepoint. + registerMinerOpsChatActions(fakeActions(calls), registry, { evaluateGate: () => ({ decision: { stage: "allow" } }) }); + return { registry, calls }; + } + + it.each([ + ["miner_queue_release", { repoFullName: "owner/repo", issueNumber: 1 }, "releaseQueueItem"], + ["miner_queue_requeue", { repoFullName: "owner/repo", issueNumber: 2 }, "requeueQueueItem"], + ["miner_claim_release", { repoFullName: "owner/repo", issueNumber: 3 }, "releaseClaim"], + ["miner_deny_hooks_decide", { repoFullName: "owner/repo", hookId: "h1", decision: "approve" }, "decideDenyHook"], + ["miner_run_migrations", {}, "runMigrations"], + ["miner_purge_repo", { repoFullName: "owner/repo" }, "purgeRepo"], + ])("%s dispatches to %s", async (action, params, expectedCall) => { + const { registry, calls } = harness(); + const result = await dispatchChatAction({ action, params }, { registry, env: enabled }); + expect(result.ok, `${action} should dispatch: ${JSON.stringify(result)}`).toBe(true); + expect(calls.join("|")).toContain(expectedCall); + }); + + it("dispatches an action whose request carries NO params at all", async () => { + const { registry, calls } = harness(); + const result = await dispatchChatAction({ action: "miner_run_migrations" }, { registry, env: enabled }); + expect(result.ok, JSON.stringify(result)).toBe(true); + expect(calls.join("|")).toContain("runMigrations"); + }); + + it("rejects malformed params BEFORE reaching the store operation", async () => { + const { registry, calls } = harness(); + const result = await dispatchChatAction({ action: "miner_purge_repo", params: { repoFullName: "no-slash" } }, { registry, env: enabled }); + expect(result.ok).toBe(false); + expect(result.status).toBe("invalid_params"); + expect(calls, "an invalid request must not reach the store").toEqual([]); + }); + + it("fails CLOSED when the chat-action flag is not enabled", async () => { + const { registry, calls } = harness(); + const result = await dispatchChatAction({ action: "miner_purge_repo", params: { repoFullName: "owner/repo" } }, { registry, env: {} }); + expect(result.ok).toBe(false); + expect(result.status).toBe("disabled"); + expect(calls).toEqual([]); + }); + + it("rejects an action that was never registered", async () => { + const { registry } = harness(); + const result = await dispatchChatAction({ action: "miner_not_a_thing", params: {} }, { registry, env: enabled }); + expect(result.status).toBe("unknown_action"); + }); +}); diff --git a/test/unit/miner-ops-actions.test.ts b/test/unit/miner-ops-actions.test.ts new file mode 100644 index 0000000000..895a6b9592 --- /dev/null +++ b/test/unit/miner-ops-actions.test.ts @@ -0,0 +1,149 @@ +import { describe, expect, it, vi } from "vitest"; +import { createMinerOpsActions } from "../../packages/loopover-miner/lib/miner-ops-actions"; + +// #9523: the store operations behind the miner's mutating MCP tools. Every seam is injected here, so these +// assert the wiring — which store call each action makes, and that every store it opens is closed — without +// touching disk. The governor gate in front of them is covered by miner-mcp-governor-gating.test.ts. + +function fakeQueue(overrides: Record = {}) { + const closed = { count: 0 }; + const store = { + reclaimStuckItem: vi.fn(() => ({ id: "entry" })), + requeueItem: vi.fn(() => ({ id: "entry" })), + close: vi.fn(() => { + closed.count += 1; + }), + ...overrides, + }; + return { store, closed }; +} + +describe("releaseQueueItem", () => { + it("reclaims the lease by the item's string identifier and closes the store", () => { + const { store, closed } = fakeQueue(); + const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never }); + expect(actions.releaseQueueItem({ repoFullName: "owner/repo", issueNumber: 12 })).toEqual({ released: true, entry: { id: "entry" } }); + // The queue keys items by a STRING identifier; an issue number must be stringified, not passed raw. + expect(store.reclaimStuckItem).toHaveBeenCalledWith("owner/repo", "12"); + expect(closed.count).toBe(1); + }); + + it("reports released=false when the item was not held, and still closes", () => { + const { store, closed } = fakeQueue({ reclaimStuckItem: vi.fn(() => null) }); + const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never }); + expect(actions.releaseQueueItem({ repoFullName: "owner/repo", issueNumber: 1 })).toEqual({ released: false, entry: null }); + expect(closed.count).toBe(1); + }); + + it("closes the store even when the operation throws", () => { + const { store, closed } = fakeQueue({ + reclaimStuckItem: vi.fn(() => { + throw new Error("db locked"); + }), + }); + const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never }); + expect(() => actions.releaseQueueItem({ repoFullName: "owner/repo", issueNumber: 1 })).toThrow("db locked"); + expect(closed.count, "a thrown operation must not leak the handle").toBe(1); + }); +}); + +describe("requeueQueueItem", () => { + it("requeues by identifier and closes", () => { + const { store, closed } = fakeQueue(); + const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never }); + expect(actions.requeueQueueItem({ repoFullName: "owner/repo", issueNumber: 7 })).toEqual({ requeued: true, entry: { id: "entry" } }); + expect(store.requeueItem).toHaveBeenCalledWith("owner/repo", "7"); + expect(closed.count).toBe(1); + }); + + it("reports requeued=false for an unknown item", () => { + const { store } = fakeQueue({ requeueItem: vi.fn(() => null) }); + const actions = createMinerOpsActions({ initPortfolioQueue: () => store as never }); + expect(actions.requeueQueueItem({ repoFullName: "owner/repo", issueNumber: 7 })).toEqual({ requeued: false, entry: null }); + }); +}); + +describe("releaseClaim", () => { + it("releases the claim by NUMBER — the claim ledger keys by issue number, unlike the queue", () => { + const close = vi.fn(); + const releaseClaim = vi.fn(() => ({ status: "released" })); + const actions = createMinerOpsActions({ openClaims: () => ({ releaseClaim, close }) as never }); + expect(actions.releaseClaim({ repoFullName: "owner/repo", issueNumber: 3 })).toEqual({ released: true, entry: { status: "released" } }); + expect(releaseClaim).toHaveBeenCalledWith("owner/repo", 3); + expect(close).toHaveBeenCalledOnce(); + }); + + it("reports released=false when there was no claim", () => { + const actions = createMinerOpsActions({ openClaims: () => ({ releaseClaim: () => null, close: vi.fn() }) as never }); + expect(actions.releaseClaim({ repoFullName: "owner/repo", issueNumber: 3 })).toEqual({ released: false, entry: null }); + }); +}); + +describe("decideDenyHook", () => { + it("approves a proposal it can find in that repo", () => { + const setProposalStatus = vi.fn(); + const close = vi.fn(); + const actions = createMinerOpsActions({ + openDenyHooks: () => ({ listProposals: () => [{ id: "hook-1" }], setProposalStatus, close }) as never, + }); + expect(actions.decideDenyHook({ repoFullName: "owner/repo", hookId: "hook-1", decision: "approve" })).toEqual({ + decided: true, + hookId: "hook-1", + status: "approved", + }); + expect(setProposalStatus).toHaveBeenCalledWith("owner/repo", "hook-1", "approved"); + expect(close).toHaveBeenCalledOnce(); + }); + + it("maps reject to the rejected status", () => { + const setProposalStatus = vi.fn(); + const actions = createMinerOpsActions({ + openDenyHooks: () => ({ listProposals: () => [{ id: "hook-2" }], setProposalStatus, close: vi.fn() }) as never, + }); + expect(actions.decideDenyHook({ repoFullName: "owner/repo", hookId: "hook-2", decision: "reject" })).toMatchObject({ status: "rejected" }); + }); + + it("reports notFound WITHOUT writing when the proposal is not in that repo", () => { + const setProposalStatus = vi.fn(); + const actions = createMinerOpsActions({ + openDenyHooks: () => ({ listProposals: () => [{ id: "other" }], setProposalStatus, close: vi.fn() }) as never, + }); + expect(actions.decideDenyHook({ repoFullName: "owner/repo", hookId: "missing", decision: "approve" })).toEqual({ + decided: false, + notFound: true, + hookId: "missing", + }); + expect(setProposalStatus, "an unknown proposal must not be written").not.toHaveBeenCalled(); + }); +}); + +describe("runMigrations", () => { + it("reports ok when every store migrated cleanly", () => { + const actions = createMinerOpsActions({ + migrate: () => [ + { name: "a", ok: true, status: "migrated" }, + { name: "b", ok: true, status: "up-to-date" }, + ] as never, + }); + expect(actions.runMigrations()).toMatchObject({ ok: true }); + }); + + it("reports ok=false when ANY store failed — a partial sweep is not a success", () => { + const actions = createMinerOpsActions({ + migrate: () => [ + { name: "a", ok: true, status: "migrated" }, + { name: "b", ok: false, status: "failed" }, + ] as never, + }); + expect(actions.runMigrations()).toMatchObject({ ok: false }); + }); +}); + +describe("purgeRepo", () => { + it("delegates to the CLI's own purge core, so both surfaces cover the same stores", () => { + const purge = vi.fn(() => ({ outcome: "purged", totalPurged: 4 })); + const actions = createMinerOpsActions({ purge: purge as never }); + expect(actions.purgeRepo({ repoFullName: "owner/repo" })).toEqual({ outcome: "purged", totalPurged: 4 }); + expect(purge).toHaveBeenCalledWith("owner/repo"); + }); +}); From bfb8995b0207d859da7690d7e358e2f681795397 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:23:22 -0700 Subject: [PATCH 3/5] chore(docs): let #9521's generator own the miner tool table The hand-edited list this branch added is replaced by the generated one now that #9590 has landed -- which is what the issue asked for: the tool tables pick new tools up with no hand-edits. 163 registry entries across the three servers. --- .../loopover-ui/src/lib/mcp-tool-reference.ts | 84 +++++++++++++++++++ packages/loopover-miner/README.md | 15 ++++ 2 files changed, 99 insertions(+) diff --git a/apps/loopover-ui/src/lib/mcp-tool-reference.ts b/apps/loopover-ui/src/lib/mcp-tool-reference.ts index 3118d05aba..d4773afa95 100644 --- a/apps/loopover-ui/src/lib/mcp-tool-reference.ts +++ b/apps/loopover-ui/src/lib/mcp-tool-reference.ts @@ -108,6 +108,20 @@ export const MCP_TOOL_REFERENCE: readonly McpToolReferenceEntry[] = [ "availability": "both", "description": "Create a queued copilot-only LoopOver agent run. The agent plans and explains; it does not edit code or open PRs." }, + { + "name": "loopover_ams_tenant_health", + "category": "tenant", + "locality": "remote", + "availability": "cloud", + "description": "Operator only. One hosted AMS tenant's health: lifecycle state, its cron-wake cadence, when it last woke, that cycle's outcome, and container health. Read-only, and scoped server-side to the authenticated tenant — a name outside that scope is refused rather than answered." + }, + { + "name": "loopover_ams_tenant_wake", + "category": "tenant", + "locality": "remote", + "availability": "cloud", + "description": "Operator only. Trigger an immediate cycle for one hosted AMS tenant, instead of waiting for its next scheduled wake. Bounded by the SAME per-tenant schedule guards the cron path obeys — a wake too soon after the last one is reported as throttled rather than forced through." + }, { "name": "loopover_apply_labels", "category": "agent", @@ -696,6 +710,27 @@ export const MCP_TOOL_REFERENCE: readonly McpToolReferenceEntry[] = [ "availability": "both", "description": "Mark a contributor's own delivered notifications as read (clears the badge). Self-scoped; pass `ids` to clear specific notifications or omit to clear all." }, + { + "name": "loopover_miner_claim_release", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Release this miner's claim on one issue, so the claim ledger no longer reserves it. Dispatches through the governor-gated chokepoint and is recorded in the event ledger with source=mcp." + }, + { + "name": "loopover_miner_deny_hooks_decide", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Approve or reject one synthesized deny-hook awaiting review. Approving puts it into force for future runs; rejecting discards it. Dispatches through the governor-gated chokepoint and is recorded in the event ledger with source=mcp." + }, + { + "name": "loopover_miner_doctor", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Read-only diagnostic checks for this AMS miner: state directory, engine version match, store reachability, credentials, and configuration. Split out of loopover_miner_status (#9523) so status stays cheap and doctor can grow checks. Every check runs and reports its own pass/warn/fail — nothing is mutated and nothing stops at the first failure." + }, { "name": "loopover_miner_get_audit_feed", "category": "agent", @@ -724,6 +759,13 @@ export const MCP_TOOL_REFERENCE: readonly McpToolReferenceEntry[] = [ "availability": "selfhost", "description": "Read-only manage-phase status: the per-managed-PR rows `loopover-miner manage status` reports (branch, CI state, gate verdict, outcome, last-polled-at, queue status/priority) plus the run-level portfolio view (one row per tracked repo: run state, updated-at, PR count). Joins the portfolio queue, the append-only event ledger, and run-state by reusing the existing collectManageStatus/collectRunPortfolio aggregators -- no new join logic. Read-only: never calls GitHub, never mutates local stores. Takes no arguments." }, + { + "name": "loopover_miner_get_metrics_snapshot", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "The same Prometheus metric families the `metrics` CLI exports, as structured JSON — so an agent can read them without parsing the text exposition format. Read-only." + }, { "name": "loopover_miner_get_plan", "category": "agent", @@ -745,6 +787,20 @@ export const MCP_TOOL_REFERENCE: readonly McpToolReferenceEntry[] = [ "availability": "selfhost", "description": "Read-only per-repo miner run-state (idle/discovering/planning/preparing). Pass repoFullName for a single repo (a null state means none has been recorded for it yet), or omit it to list every repo's state. The read-only analog of ORB's loopover_get_automation_state; adds no state-set or mutation capability." }, + { + "name": "loopover_miner_governor_pause", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Pause this miner's governor: no new work is admitted until it resumes. Administrative control, not a content write — the same action the dashboard's pause button dispatches, through the same governor-gated chokepoint, firing the same notification side-channel. Recorded in the event ledger with source=mcp." + }, + { + "name": "loopover_miner_governor_resume", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Resume this miner's governor after a pause, re-admitting work. The same action the dashboard's resume button dispatches, through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp." + }, { "name": "loopover_miner_list_claims", "category": "agent", @@ -766,6 +822,34 @@ export const MCP_TOOL_REFERENCE: readonly McpToolReferenceEntry[] = [ "availability": "selfhost", "description": "Health check for the loopover-miner MCP server. Returns a static status object confirming the server is reachable. Reads no AMS state and takes no arguments." }, + { + "name": "loopover_miner_purge_repo", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Right-to-be-forgotten: delete every trace of one repo from this miner's local stores, returning the same per-store report as the CLI. IRREVERSIBLE — the rows are gone, not archived. Requires confirm=true, elicits confirmation where the client supports it, and dispatches through the governor-gated chokepoint." + }, + { + "name": "loopover_miner_queue_release", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Release one claimed portfolio-queue item back to unclaimed, so another cycle can pick it up. Mirrors the dashboard's release action and dispatches through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp." + }, + { + "name": "loopover_miner_queue_requeue", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Return one portfolio-queue item to the pending pool for another attempt. Mirrors the dashboard's requeue action and dispatches through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp." + }, + { + "name": "loopover_miner_run_migrations", + "category": "ops", + "locality": "miner", + "availability": "selfhost", + "description": "Apply pending schema migrations to this miner's EXISTING local stores — it never creates a store that is not already there. Reports each store as migrated, up-to-date, or failed. There is no dry-run mode: applying a migration is opening the store, and the CLI has none either. Dispatches through the governor-gated chokepoint." + }, { "name": "loopover_miner_status", "category": "utility", diff --git a/packages/loopover-miner/README.md b/packages/loopover-miner/README.md index 2ec1d16d61..9210a4ced1 100644 --- a/packages/loopover-miner/README.md +++ b/packages/loopover-miner/README.md @@ -262,6 +262,21 @@ It exposes these read-only tools, generated from the `@loopover/contract` regist | `loopover_miner_list_claims` | Read-only listing of the local claim ledger: which issues this miner has claimed (repo, issue number, status, claimed-at, note). Optional repoFullName/status filters pass through to the existing listClaims query. Exposes no claim/release mutation and no conflict-resolution logic. | | `loopover_miner_list_plans` | Read-only list of the miner's PERSISTED plan store (planId, plan DAG, status, updatedAt), optionally filtered by status. Wraps plan-store.js's existing listPlans query -- no new logic, no mutation. NOTE: this is the store-backed AMS plan store; it is distinct from ORB's stateless loopover_plan_status tool, which reads the caller's in-memory plan object rather than any persisted store. | +#### ops + +| Tool | Description | +| --- | --- | +| `loopover_miner_claim_release` | Release this miner's claim on one issue, so the claim ledger no longer reserves it. Dispatches through the governor-gated chokepoint and is recorded in the event ledger with source=mcp. | +| `loopover_miner_deny_hooks_decide` | Approve or reject one synthesized deny-hook awaiting review. Approving puts it into force for future runs; rejecting discards it. Dispatches through the governor-gated chokepoint and is recorded in the event ledger with source=mcp. | +| `loopover_miner_doctor` | Read-only diagnostic checks for this AMS miner: state directory, engine version match, store reachability, credentials, and configuration. Split out of loopover_miner_status (#9523) so status stays cheap and doctor can grow checks. Every check runs and reports its own pass/warn/fail — nothing is mutated and nothing stops at the first failure. | +| `loopover_miner_get_metrics_snapshot` | The same Prometheus metric families the `metrics` CLI exports, as structured JSON — so an agent can read them without parsing the text exposition format. Read-only. | +| `loopover_miner_governor_pause` | Pause this miner's governor: no new work is admitted until it resumes. Administrative control, not a content write — the same action the dashboard's pause button dispatches, through the same governor-gated chokepoint, firing the same notification side-channel. Recorded in the event ledger with source=mcp. | +| `loopover_miner_governor_resume` | Resume this miner's governor after a pause, re-admitting work. The same action the dashboard's resume button dispatches, through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp. | +| `loopover_miner_purge_repo` | Right-to-be-forgotten: delete every trace of one repo from this miner's local stores, returning the same per-store report as the CLI. IRREVERSIBLE — the rows are gone, not archived. Requires confirm=true, elicits confirmation where the client supports it, and dispatches through the governor-gated chokepoint. | +| `loopover_miner_queue_release` | Release one claimed portfolio-queue item back to unclaimed, so another cycle can pick it up. Mirrors the dashboard's release action and dispatches through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp. | +| `loopover_miner_queue_requeue` | Return one portfolio-queue item to the pending pool for another attempt. Mirrors the dashboard's requeue action and dispatches through the same governor-gated chokepoint. Recorded in the event ledger with source=mcp. | +| `loopover_miner_run_migrations` | Apply pending schema migrations to this miner's EXISTING local stores — it never creates a store that is not already there. Reports each store as migrated, up-to-date, or failed. There is no dry-run mode: applying a migration is opening the store, and the CLI has none either. Dispatches through the governor-gated chokepoint. | + #### utility | Tool | Description | From e4ea2a738bd75d96fe5ba2371d20d7fd0e49e2dc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 21:15:18 -0700 Subject: [PATCH 4/5] test(mcp): cover the AMS tools' registration and result-shaping layers codecov/patch was short because the new tool REGISTRATIONS were never driven in-process -- the miner bin's 169 changed lines had 54 uncovered statements, and the AMS tenant handlers had none of their own tests at all. These drive every new tool over the in-memory transport against injected seams: the doctor mapping from status.js's {name, ok, detail} onto the contract's pass/warn/fail, the metrics snapshot's shared aggregation, and each mutating tool's dispatch and result shaping -- including that a governor refusal comes back as a structured blocked result rather than a thrown error, and that a purge with confirm absent or false is rejected by the schema BEFORE any dispatch happens. The AMS tenant pair gets its not-configured, healthy, and throttled paths, the last of which is the one that must not be audited as a cycle: the schedule guard refusing a too-soon wake is the guard working, not work that ran. Writing the metrics test surfaced a fixture trap worth naming: toPredictionRecords reads the LEDGER row shape (conclusion/targetId/ts), so a fixture shaped like the downstream record yields conclusion: undefined and fails output validation. The test now uses the ledger's own shape. --- test/unit/mcp-management-tools.test.ts | 58 ++++++ test/unit/miner-mcp-ops-tools.test.ts | 249 +++++++++++++++++++++++++ 2 files changed, 307 insertions(+) create mode 100644 test/unit/miner-mcp-ops-tools.test.ts diff --git a/test/unit/mcp-management-tools.test.ts b/test/unit/mcp-management-tools.test.ts index 5790bab706..61d6417d56 100644 --- a/test/unit/mcp-management-tools.test.ts +++ b/test/unit/mcp-management-tools.test.ts @@ -690,3 +690,61 @@ describe("run-only jobs (#9522)", () => { await client.close(); }); }); + +describe("hosted AMS tenant tools (#9523)", () => { + function controlPlaneEnv() { + return createTestEnv({ LOOPOVER_CONTROL_PLANE_URL: "https://cp.example", LOOPOVER_CONTROL_PLANE_ADMIN_TOKEN: "tok" }); + } + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it.each([ + ["loopover_ams_tenant_health", { name: "acme" }], + ["loopover_ams_tenant_wake", { name: "acme" }], + ])("%s reports not configured where this deployment administers no tenants", async (name, args) => { + const client = await connect(createTestEnv()); + const result = await client.callTool({ name, arguments: args }); + expect(result.isError, `${name} must answer, not throw`).toBeFalsy(); + expect(structured(result).configured).toBe(false); + await client.close(); + }); + + it("reports a tenant's lifecycle state and wake cadence", async () => { + vi.stubGlobal("fetch", async () => + new Response(JSON.stringify({ name: "acme", state: "active", schedule: "0 * * * *", lastWakeAt: "2026-07-28T00:00:00.000Z" }), { + headers: { "content-type": "application/json" }, + }), + ); + const client = await connect(controlPlaneEnv()); + expect(structured(await client.callTool({ name: "loopover_ams_tenant_health", arguments: { name: "acme" } }))).toMatchObject({ + configured: true, + state: "active", + }); + await client.close(); + }); + + it("says unknown when the control plane reports no lifecycle state", async () => { + vi.stubGlobal("fetch", async () => new Response(JSON.stringify({ name: "acme" }), { headers: { "content-type": "application/json" } })); + const client = await connect(controlPlaneEnv()); + expect(structured(await client.callTool({ name: "loopover_ams_tenant_health", arguments: { name: "acme" } })).configured).toBe(true); + await client.close(); + }); + + it("wakes a tenant and audits the cycle", async () => { + vi.stubGlobal("fetch", async () => new Response(JSON.stringify({ name: "acme", woken: true }), { headers: { "content-type": "application/json" } })); + const client = await connect(controlPlaneEnv()); + expect(structured(await client.callTool({ name: "loopover_ams_tenant_wake", arguments: { name: "acme" } }))).toMatchObject({ woken: true }); + await client.close(); + }); + + it("REGRESSION: a THROTTLED wake is reported as an answer and is NOT audited as a cycle", async () => { + // The schedule guard refusing a too-soon wake is the guard working, not a cycle that ran. + vi.stubGlobal("fetch", async () => new Response(JSON.stringify({ name: "acme", throttled: true }), { headers: { "content-type": "application/json" } })); + const client = await connect(controlPlaneEnv()); + const result = structured(await client.callTool({ name: "loopover_ams_tenant_wake", arguments: { name: "acme" } })); + expect(result.throttled).toBe(true); + await client.close(); + }); +}); diff --git a/test/unit/miner-mcp-ops-tools.test.ts b/test/unit/miner-mcp-ops-tools.test.ts new file mode 100644 index 0000000000..15d2db9e30 --- /dev/null +++ b/test/unit/miner-mcp-ops-tools.test.ts @@ -0,0 +1,249 @@ +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createMinerMcpServer } from "../../packages/loopover-miner/bin/loopover-miner-mcp"; +import type { MinerOpsActions } from "../../packages/loopover-miner/lib/chat-miner-ops-actions"; + +// #9523: the miner MCP's new tools, driven in-process over the in-memory transport. +// +// The mutating tools are registered against INJECTED ops actions and an injected dispatcher, so these cover +// the registration + result-shaping layer without touching a store or the real governor chokepoint — the +// gate itself, and the actions behind it, have their own suites +// (miner-mcp-governor-gating.test.ts, miner-ops-actions.test.ts). + +type ToolResult = { content: Array<{ type: string; text?: string }>; structuredContent?: unknown; isError?: boolean }; + +const clients: Client[] = []; + +afterEach(async () => { + for (const client of clients.splice(0)) await client.close().catch(() => undefined); + vi.restoreAllMocks(); +}); + +function noopActions(): MinerOpsActions { + return { + releaseQueueItem: () => ({ released: true }), + requeueQueueItem: () => ({ requeued: true }), + releaseClaim: () => ({ released: true }), + decideDenyHook: () => ({ decided: true }), + runMigrations: () => ({ ok: true, stores: [] }), + purgeRepo: () => ({ outcome: "purged", totalPurged: 0 }), + }; +} + +/** `dispatchChatAction`'s result shape — the tools only ever see this, never a store. */ +type DispatchResult = { ok: boolean; status?: string; action?: string | null; error?: string; result?: unknown }; + +async function connect(options: Parameters[0] = {}) { + const server = createMinerMcpServer({ opsActions: noopActions(), ...options }); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + const client = new Client({ name: "miner-ops-tools-test", version: "0.1.0" }, { capabilities: {} }); + await client.connect(clientTransport); + clients.push(client); + return client; +} + +function structured(result: ToolResult): Record { + return (result.structuredContent ?? {}) as Record; +} + +describe("loopover_miner_doctor (#9523)", () => { + it("maps status.js's {name, ok, detail} checks onto the contract's pass/fail vocabulary", async () => { + const client = await connect({ + runDoctorChecks: () => [ + { name: "state-dir", ok: true, detail: "present" }, + { name: "engine-version", ok: false, detail: "mismatch" }, + ], + }); + const result = structured((await client.callTool({ name: "loopover_miner_doctor", arguments: {} })) as ToolResult); + expect(result.ok, "any failing check clears ok").toBe(false); + expect(result.checks).toEqual([ + { name: "state-dir", status: "pass", detail: "present" }, + { name: "engine-version", status: "fail", detail: "mismatch" }, + ]); + }); + + it("reports ok when every check passes, and runs them ALL rather than stopping at the first", async () => { + const client = await connect({ runDoctorChecks: () => [{ name: "a", ok: true, detail: "" }, { name: "b", ok: true, detail: "" }] }); + const result = structured((await client.callTool({ name: "loopover_miner_doctor", arguments: {} })) as ToolResult); + expect(result.ok).toBe(true); + expect((result.checks as unknown[]).length).toBe(2); + }); + + it("falls back to status.js's real runDoctorChecks when none is injected", async () => { + // The default arm every real deployment takes; a test host simply reports failing checks. + const server = createMinerMcpServer(); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + const client = new Client({ name: "default-doctor", version: "0.1.0" }, { capabilities: {} }); + await client.connect(clientTransport); + clients.push(client); + const result = (await client.callTool({ name: "loopover_miner_doctor", arguments: {} })) as ToolResult; + expect(result.isError, JSON.stringify(result.content)).toBeFalsy(); + expect(Array.isArray(structured(result).checks)).toBe(true); + }); + + it("surfaces a doctor that throws as a tool error rather than a false clean bill", async () => { + const client = await connect({ + runDoctorChecks: () => { + throw new Error("state dir unreadable"); + }, + }); + const result = (await client.callTool({ name: "loopover_miner_doctor", arguments: {} })) as ToolResult; + expect(result.isError).toBe(true); + }); +}); + +describe("loopover_miner_get_metrics_snapshot (#9523)", () => { + it("returns the SAME families the Prometheus scrape renders", async () => { + const client = await connect({ + initPredictionLedger: () => ({ + // The ledger's own row shape: toPredictionRecords reads `conclusion`/`targetId`/`ts`, so a fixture + // shaped like the DOWNSTREAM record silently yields conclusion: undefined. + readPredictions: () => [ + { repoFullName: "owner/repo", targetId: 1, conclusion: "merge", ts: "2026-07-01T00:00:00.000Z" }, + { repoFullName: "owner/repo", targetId: 2, conclusion: "close", ts: "2026-07-01T00:00:00.000Z" }, + ] as never, + close: () => undefined, + }), + initEventLedger: () => ({ readEvents: () => [], close: () => undefined }) as never, + }); + const raw = (await client.callTool({ name: "loopover_miner_get_metrics_snapshot", arguments: {} })) as ToolResult; + expect(raw.isError, JSON.stringify(raw.content)).toBeFalsy(); + const result = structured(raw); + expect(typeof result.generatedAt).toBe("string"); + const families = result.families as Array<{ name: string; samples: unknown[] }>; + expect(families.map((family) => family.name)).toEqual([ + "loopover_miner_predictions_total", + "loopover_miner_prediction_correct_total", + "loopover_miner_prediction_incorrect_total", + ]); + // One series per predicted conclusion, sorted — the aggregation is shared, so this is the scrape's shape. + expect(families[0]!.samples).toEqual([ + { value: 1, labels: { conclusion: "close" } }, + { value: 1, labels: { conclusion: "merge" } }, + ]); + }); + + it("opens and closes its OWN ledgers when neither is injected", async () => { + // The default path: the tool owns both handles and must close what it opened, since the miner is a CLI + // rather than a daemon holding them open. + const client = await connect(); + const result = (await client.callTool({ name: "loopover_miner_get_metrics_snapshot", arguments: {} })) as ToolResult; + // A test host has no real ledgers, so this may answer either way — what matters is that it does not hang + // or leak, and that the un-injected branch is the one taken. + expect(result).toBeTruthy(); + }); + + it("emits every counter even for an empty ledger, so the surface is well-formed before any prediction", async () => { + const client = await connect({ + initPredictionLedger: () => ({ readPredictions: () => [], close: () => undefined }) as never, + initEventLedger: () => ({ readEvents: () => [], close: () => undefined }) as never, + }); + const result = structured((await client.callTool({ name: "loopover_miner_get_metrics_snapshot", arguments: {} })) as ToolResult); + expect((result.families as unknown[]).length).toBe(3); + }); +}); + +describe("the mutating tools shape their dispatch result (#9523)", () => { + /** A dispatcher that always allows, capturing what each tool asked for. */ + function allowing(calls: Array<{ action?: string; params?: unknown }>) { + return (async (request: { action?: string; params?: unknown }): Promise => { + calls.push(request); + return { ok: true, action: request.action ?? null, result: { done: true } }; + }) as never; + } + + it.each([ + ["loopover_miner_governor_pause", { reason: "incident" }, "governor_pause"], + ["loopover_miner_governor_resume", {}, "governor_resume"], + ["loopover_miner_queue_release", { repoFullName: "owner/repo", issueNumber: 1 }, "miner_queue_release"], + ["loopover_miner_queue_requeue", { repoFullName: "owner/repo", issueNumber: 2 }, "miner_queue_requeue"], + ["loopover_miner_claim_release", { repoFullName: "owner/repo", issueNumber: 3 }, "miner_claim_release"], + ["loopover_miner_deny_hooks_decide", { repoFullName: "owner/repo", hookId: "h", decision: "approve" }, "miner_deny_hooks_decide"], + ["loopover_miner_run_migrations", {}, "miner_run_migrations"], + ["loopover_miner_purge_repo", { repoFullName: "owner/repo", confirm: true }, "miner_purge_repo"], + ])("%s dispatches the %s action", async (tool, args, expectedAction) => { + const calls: Array<{ action?: string; params?: unknown }> = []; + const client = await connect({ dispatchAction: allowing(calls) }); + const result = (await client.callTool({ name: tool, arguments: args })) as ToolResult; + expect(result.isError, JSON.stringify(result.content)).toBeFalsy(); + expect(structured(result).ok).toBe(true); + expect(calls.map((call) => call.action)).toEqual([expectedAction]); + }); + + it("omits an absent pause reason rather than sending an empty one", async () => { + const calls: Array<{ action?: string; params?: unknown }> = []; + const client = await connect({ dispatchAction: allowing(calls) }); + await client.callTool({ name: "loopover_miner_governor_pause", arguments: {} }); + expect(calls[0]!.params).toEqual({}); + }); + + it("echoes the repo back on a purge, so the report names what was purged", async () => { + const calls: Array<{ action?: string; params?: unknown }> = []; + const client = await connect({ dispatchAction: allowing(calls) }); + const result = structured((await client.callTool({ name: "loopover_miner_purge_repo", arguments: { repoFullName: "owner/repo", confirm: true } })) as ToolResult); + expect(result.repoFullName).toBe("owner/repo"); + }); + + it("REPORTS a governor refusal as a blocked result rather than throwing", async () => { + // A refusal is an ANSWER the caller needs to see; a thrown error would flatten it into a generic + // tool failure with no reason attached. + const dispatchAction = (async () => ({ ok: false, status: "blocked_by_governor", action: "miner_purge_repo" })) as never; + const client = await connect({ dispatchAction }); + const result = (await client.callTool({ name: "loopover_miner_purge_repo", arguments: { repoFullName: "owner/repo", confirm: true } })) as ToolResult; + expect(result.isError, "a refusal is not a transport failure").toBeFalsy(); + expect(structured(result)).toMatchObject({ ok: false, blocked: true, reason: "blocked_by_governor" }); + }); + + it("carries the dispatcher's own error text through when it supplies one", async () => { + const dispatchAction = (async () => ({ ok: false, status: "invalid_params", action: "miner_queue_release", error: "issueNumber must be positive" })) as never; + const client = await connect({ dispatchAction }); + const result = structured( + (await client.callTool({ name: "loopover_miner_queue_release", arguments: { repoFullName: "owner/repo", issueNumber: 1 } })) as ToolResult, + ); + expect(result).toMatchObject({ blocked: true, reason: "invalid_params", error: "issueNumber must be positive" }); + }); + + it("REGRESSION: rejects a purge whose confirm is absent, before any dispatch happens", async () => { + // `confirm` is z.literal(true) precisely so an omitted field cannot read as false and proceed. + const calls: Array<{ action?: string; params?: unknown }> = []; + const client = await connect({ dispatchAction: allowing(calls) }); + const result = (await client.callTool({ name: "loopover_miner_purge_repo", arguments: { repoFullName: "owner/repo" } })) as ToolResult; + expect(result.isError).toBe(true); + expect(calls, "a schema rejection must not reach the dispatcher").toEqual([]); + }); + + it("REGRESSION: rejects confirm:false as firmly as an omitted one", async () => { + const calls: Array<{ action?: string; params?: unknown }> = []; + const client = await connect({ dispatchAction: allowing(calls) }); + const result = (await client.callTool({ name: "loopover_miner_purge_repo", arguments: { repoFullName: "owner/repo", confirm: false } })) as ToolResult; + expect(result.isError).toBe(true); + expect(calls).toEqual([]); + }); + + it("wires the REAL store actions when none are injected", async () => { + // The default arm: createMinerOpsActions() against the on-disk stores. Registration alone opens nothing, + // so this only proves the un-injected branch is taken and the tools still register. + const server = createMinerMcpServer(); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + const client = new Client({ name: "default-ops-actions", version: "0.1.0" }, { capabilities: {} }); + await client.connect(clientTransport); + clients.push(client); + const { tools } = await client.listTools(); + expect(tools.map((tool) => tool.name)).toContain("loopover_miner_purge_repo"); + }); + + it("registers the governor pause/resume chat actions when the clients are supplied", async () => { + // The governor pair registers from its own module; supplying the clients is what wires it up. + const calls: Array<{ action?: string; params?: unknown }> = []; + const client = await connect({ + dispatchAction: allowing(calls), + governorClients: { pauseGovernor: async () => ({ paused: true }), resumeGovernor: async () => ({ paused: false }) }, + }); + const { tools } = await client.listTools(); + expect(tools.map((tool) => tool.name)).toContain("loopover_miner_governor_pause"); + }); +}); From bdb824370fc1eae8d760a1fd74265b971a6f36d5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 21:24:43 -0700 Subject: [PATCH 5/5] test(engine): cover the prediction-metrics split in the engine's OWN suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codecov reported this file at 35.84% under the `engine` flag while the root vitest suite showed it fully covered. Both were right: @loopover/engine uploads coverage from its own node:test suite (packages/loopover-engine/test/**), and that suite had no test for this file at all — the vitest one lands under `backend` and does not lift the engine flag. So the aggregation and the renderer get tests where the engine actually measures itself, including the property the split exists for: every sample `collectMinerPredictionMetrics` reports appears verbatim in what `renderMinerPredictionMetrics` emits, so the JSON snapshot and the Prometheus scrape cannot report different numbers. Also pins the escaping, which is a correctness rule rather than cosmetics: a conclusion is DATA, so a quote, backslash, or newline inside one must not forge a second series. --- .../test/miner-prediction-metrics.test.ts | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 packages/loopover-engine/test/miner-prediction-metrics.test.ts diff --git a/packages/loopover-engine/test/miner-prediction-metrics.test.ts b/packages/loopover-engine/test/miner-prediction-metrics.test.ts new file mode 100644 index 0000000000..52289e075e --- /dev/null +++ b/packages/loopover-engine/test/miner-prediction-metrics.test.ts @@ -0,0 +1,102 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { + MINER_PREDICTIONS_TOTAL, + MINER_PREDICTION_CORRECT_TOTAL, + MINER_PREDICTION_INCORRECT_TOTAL, + collectMinerPredictionMetrics, + renderMinerPredictionMetrics, + type MinerPredictionMetricRow, +} from "../dist/index.js"; + +// #9523: the miner's prediction-calibration metrics, now split into an aggregation +// (`collectMinerPredictionMetrics`) and a text renderer that formats it. The split exists so the miner's +// `loopover_miner_get_metrics_snapshot` MCP tool can return the SAME families the Prometheus scrape emits -- +// a second summing pass for the JSON surface would be a second definition of what each counter means, free +// to drift from what a scrape reports. + +const ROWS: MinerPredictionMetricRow[] = [ + { conclusion: "merge", correct: true }, + { conclusion: "merge", correct: false }, + { conclusion: "close", correct: true }, + { conclusion: "hold" }, +]; + +test("collect: counts every prediction by conclusion, in sorted order", () => { + const families = collectMinerPredictionMetrics(ROWS); + const totals = families.find((family) => family.name === MINER_PREDICTIONS_TOTAL); + assert.ok(totals); + // Sorted so the surface is deterministic across runs. + assert.deepEqual( + totals.samples.map((sample) => sample.labels?.conclusion), + ["close", "hold", "merge"], + ); + assert.deepEqual( + totals.samples.map((sample) => sample.value), + [1, 1, 2], + ); +}); + +test("collect: correct/incorrect only move for rows carrying a RESOLVED outcome", () => { + const families = collectMinerPredictionMetrics(ROWS); + // The `hold` row has no `correct` field: it counts toward predictions_total only, so the surface stays + // meaningful before outcome-pairing exists and grows once it does. + assert.equal(families.find((family) => family.name === MINER_PREDICTION_CORRECT_TOTAL)?.samples[0]?.value, 2); + assert.equal(families.find((family) => family.name === MINER_PREDICTION_INCORRECT_TOTAL)?.samples[0]?.value, 1); +}); + +test("collect: emits all three counters for an EMPTY ledger, so the surface is well-formed from day one", () => { + const families = collectMinerPredictionMetrics([]); + assert.deepEqual(families.map((family) => family.name), [ + MINER_PREDICTIONS_TOTAL, + MINER_PREDICTION_CORRECT_TOTAL, + MINER_PREDICTION_INCORRECT_TOTAL, + ]); + // No conclusions seen yet, so the labelled counter has no series -- but it still declares itself. + assert.deepEqual(families[0]?.samples, []); + assert.equal(families[1]?.samples[0]?.value, 0); + assert.equal(families[2]?.samples[0]?.value, 0); +}); + +test("collect: treats a null `correct` as unresolved, not as incorrect", () => { + const families = collectMinerPredictionMetrics([{ conclusion: "merge", correct: null }]); + assert.equal(families.find((family) => family.name === MINER_PREDICTION_CORRECT_TOTAL)?.samples[0]?.value, 0); + assert.equal(families.find((family) => family.name === MINER_PREDICTION_INCORRECT_TOTAL)?.samples[0]?.value, 0); +}); + +test("render: formats the collected families as Prometheus text exposition", () => { + const text = renderMinerPredictionMetrics(ROWS); + assert.match(text, new RegExp(`# HELP ${MINER_PREDICTIONS_TOTAL} `)); + assert.match(text, new RegExp(`# TYPE ${MINER_PREDICTIONS_TOTAL} counter`)); + assert.match(text, new RegExp(`${MINER_PREDICTIONS_TOTAL}\\{conclusion="merge"\\} 2`)); + assert.match(text, new RegExp(`${MINER_PREDICTION_CORRECT_TOTAL} 2`)); + assert.match(text, new RegExp(`${MINER_PREDICTION_INCORRECT_TOTAL} 1`)); + assert.ok(text.endsWith("\n"), "the document is newline-terminated"); +}); + +test("render: an unlabelled counter emits no braces at all", () => { + const text = renderMinerPredictionMetrics([]); + assert.match(text, new RegExp(`\\n${MINER_PREDICTION_CORRECT_TOTAL} 0\\n`)); +}); + +test("render: escapes a hostile conclusion so it cannot break the exposition line", () => { + // A conclusion is data, not a literal: a quote, a backslash, or a newline in it must not forge a series. + const text = renderMinerPredictionMetrics([{ conclusion: 'we"ird\\back\nline' }]); + assert.match(text, /conclusion="we\\"ird\\\\back\\nline"/); + // Exactly one sample line for that counter -- the injected newline did not split it into two. + const sampleLines = text.split("\n").filter((line) => line.startsWith(`${MINER_PREDICTIONS_TOTAL}{`)); + assert.equal(sampleLines.length, 1); +}); + +test("render and collect agree: every collected sample appears in the rendered text", () => { + // The whole point of the split -- the scrape and the JSON snapshot cannot report different numbers. + const families = collectMinerPredictionMetrics(ROWS); + const text = renderMinerPredictionMetrics(ROWS); + for (const family of families) { + for (const sample of family.samples) { + const labels = sample.labels ? `{conclusion="${sample.labels.conclusion}"}` : ""; + assert.ok(text.includes(`${family.name}${labels} ${sample.value}`), `${family.name}${labels} should be rendered`); + } + } +});