diff --git a/packages/loopover-contract/src/enums.ts b/packages/loopover-contract/src/enums.ts index c1be14f7b..6c520af80 100644 --- a/packages/loopover-contract/src/enums.ts +++ b/packages/loopover-contract/src/enums.ts @@ -34,6 +34,27 @@ export type AutonomyLevel = (typeof AUTONOMY_LEVELS)[number]; * Do not "sync" this to the engine's list; the difference is the point. */ export const MAINTAIN_ACTION_CLASSES = ["review", "request_changes", "approve", "merge", "close", "label"] as const; + +/** + * The engine's FULL action-class list (#9762). + * + * A superset of MAINTAIN_ACTION_CLASSES: the autonomy record is keyed by every class the agent can take, + * while the maintain dial exposes only the operator-settable subset. Restated here and pinned against + * @loopover/engine's own by a meta-test, the posture limits.ts established -- the settings response schema + * needs this list and cannot import the engine. + */ +export const AGENT_ACTION_CLASSES = [ + "review", + "request_changes", + "approve", + "merge", + "close", + "label", + "review_state_label", + "update_branch", + "assign", +] as const; +export type AgentActionClass = (typeof AGENT_ACTION_CLASSES)[number]; export type MaintainActionClass = (typeof MAINTAIN_ACTION_CLASSES)[number]; /** diff --git a/packages/loopover-mcp/bin/loopover-mcp.ts b/packages/loopover-mcp/bin/loopover-mcp.ts index f76b753fa..9681827a6 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.ts +++ b/packages/loopover-mcp/bin/loopover-mcp.ts @@ -158,7 +158,7 @@ import { getToolContract, ListPendingActionsStdioInput, } from "@loopover/contract/tools"; -import type { ToolContract } from "@loopover/contract"; +import { AUTONOMY_LEVELS as MAINTAIN_AUTONOMY_LEVELS, MAINTAIN_ACTION_CLASSES, PROPOSE_ACTION_CLASSES, type ToolContract } from "@loopover/contract"; import { buildBranchAnalysisPayload, collectLocalDiff, collectLocalBranchMetadata, probeLocalScorer, referenceScorePreviewExample, resolveScorePreviewCommand, resolveWorkspaceCwd, sanitizeLocalScorerStatus, setupGuidanceForLocalScorer, isTestFile } from "../lib/local-branch.js"; import { formatTable } from "../lib/format-table.js"; import { argsWantJson, describeCliError, reportCliFailure } from "../lib/cli-error.js"; @@ -359,28 +359,18 @@ const REPEATABLE_FLAGS = new Set(Object.entries(CLI_FLAG_SPEC).filter(([, kind]) const BOOLEAN_FLAGS = new Set(Object.entries(CLI_FLAG_SPEC).filter(([, kind]) => kind === "boolean").map(([flag]) => flag)); const COMPLETION_SHELLS = ["bash", "zsh", "fish", "powershell"]; const AGENT_PROFILE_IDS = ["miner-planner", "miner-auto-dev", "maintainer-triage", "repo-owner-intake"]; -// #784 maintain set-level — the autonomy dial's action classes + levels. +// #784 maintain set-level — the autonomy dial's action classes + levels, from @loopover/contract. // -// Both are hand-synced literals, not imports: this file resolves @loopover/engine through the PUBLISHED package -// (`^3.0.0`), whose export map exposes only `.` + a few `./scoring/*`/`./signals/*` subpaths — neither surfaces -// AUTONOMY_LEVELS, so importing the canonical list would mean widening the engine's public API (#6153). The -// drift this invites is real and has bitten once already, so test/unit/mcp-cli-maintain.test.ts pins LEVELS -// against the live enum and fails the moment the two disagree. +// #9762: these were hand-synced literals, justified by a comment saying the canonical lists could not be +// imported because this file resolves @loopover/engine through the PUBLISHED package, whose export map does +// not surface AUTONOMY_LEVELS (#6153). That reason went away when the contract did: this file already +// imports @loopover/contract, and the contract carries all three lists as its own pinned copies. The +// drift the old comment warned about (LEVELS carried "suggest"/"propose" for the whole life of #4620, +// after the server had dropped them) is now impossible rather than test-detected. // -// LEVELS mirrors AUTONOMY_LEVELS (src/settings/autonomy.ts -> packages/loopover-engine/src/settings/autonomy.ts) -// exactly. #6153: it carried "suggest"/"propose" for the whole life of #4620, which dropped them server-side -- -// PUT /settings validates against the live enum (src/api/routes.ts), so every value this list accepted but the -// server didn't turned an immediate, clear client-side error into a confusing 400 from the API. -// -// ACTION_CLASSES is deliberately NOT the engine's full AGENT_ACTION_CLASSES: it is the operator-settable subset -// the maintain surface exposes, and src/mcp/server.ts's MAINTAIN_AUTONOMY_ACTION_CLASSES mirrors these six on -// purpose. Do not "sync" it to the engine list. -const MAINTAIN_ACTION_CLASSES = ["review", "request_changes", "approve", "merge", "close", "label"]; -const MAINTAIN_AUTONOMY_LEVELS = ["observe", "auto_with_approval", "auto"]; -// #6744: the loopover_propose_action / POST .../agent/pending-actions action-class enum. A superset of -// MAINTAIN_ACTION_CLASSES (adds review_state_label) — kept separate so `maintain propose` accepts exactly what the -// route + MCP tool accept, while set-level keeps its own autonomy-configurable subset above. -const PROPOSE_ACTION_CLASSES = ["review", "request_changes", "approve", "merge", "close", "label", "review_state_label"]; +// MAINTAIN_ACTION_CLASSES is deliberately NOT the engine's full AGENT_ACTION_CLASSES: it is the +// operator-settable subset the maintain surface exposes. PROPOSE_ACTION_CLASSES is that subset plus +// review_state_label, which the propose route accepts and the set-level dial does not. // #783 plan DAG -- one implementation, in @loopover/engine (#9537). This file carried a // hand-copied, untyped duplicate of buildPlanDag/validatePlanDag/nextReadySteps/the step state diff --git a/src/mcp/server.ts b/src/mcp/server.ts index bccab23f6..337aa5036 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -6,10 +6,15 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import type { RequestHandlerExtra } from "@modelcontextprotocol/sdk/shared/protocol.js"; import { ElicitResultSchema, type ServerNotification, type ServerRequest } from "@modelcontextprotocol/sdk/types.js"; import { z } from "zod"; -// #9517: the pilot tools' schemas come from the shared contract instead of being declared here. -// `.shape` is what the MCP SDK's registerTool wants; the same ZodObject also drives the JSON Schema -// the agent-tool specs and the contract validator read, so there is one definition per tool rather -// than one per server. +// #9517: the pilot tools' schemas come from the shared contract instead of being declared here, so there +// is one definition per tool rather than one per server. +// +// #9762: registerTool is handed the ZodObject, NOT its `.shape`. The SDK accepts either, but a raw shape is +// re-wrapped in a plain `z.object` that DISCARDS the catchall -- so a `looseObject` input would be advertised +// and enforced as `additionalProperties: false`, and every extra field the payload carries becomes a -32602 +// the caller cannot act on. That is #9518's defect class. None of these inputs is loose today, which is +// precisely why the 23 sites that passed `.shape` were indistinguishable from the correct ones by reading; +// test/unit/mcp-register-tool-shape-guard.test.ts is what keeps them that way. import { AdminGetConfigInput, AdminGetConfigOutput, @@ -283,7 +288,7 @@ import { amsTenantHealthTool, amsTenantWakeTool, } from "@loopover/contract/tools"; -import { TOOL_CATEGORIES, type ToolCategory } from "@loopover/contract"; +import { ROTATABLE_SECRET_NAMES, TOOL_CATEGORIES, type ToolCategory } from "@loopover/contract"; import { MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH, MAX_FIND_OPPORTUNITIES_LANGUAGES, @@ -607,7 +612,7 @@ const findOpportunitiesShape = { // is not cosmetic: src/selfhost/load-file-secrets.ts only .trim()s the file, so a label line above the // value silently becomes part of the credential. const adminRotateSecretShape = { - secret: z.enum(["claude_code_oauth_token", "github_webhook_secret", "loopover_api_token", "loopover_mcp_token", "loopover_mcp_admin_token", "pagerduty_routing_key"]), + secret: z.enum(ROTATABLE_SECRET_NAMES), value: z .string() .min(1) @@ -2436,22 +2441,22 @@ export class LoopoverMcp { ); register( "loopover_admin_get_status", - { description: adminGetStatusTool.description, inputSchema: AdminGetStatusInput.shape, outputSchema: AdminGetStatusOutput, annotations: contractAnnotations(adminGetStatusTool) }, + { description: adminGetStatusTool.description, inputSchema: AdminGetStatusInput, outputSchema: AdminGetStatusOutput, annotations: contractAnnotations(adminGetStatusTool) }, async () => this.toolResult(await this.adminGetStatus()), ); register( "loopover_admin_doctor", - { description: adminDoctorTool.description, inputSchema: AdminDoctorInput.shape, outputSchema: AdminDoctorOutput, annotations: contractAnnotations(adminDoctorTool) }, + { description: adminDoctorTool.description, inputSchema: AdminDoctorInput, outputSchema: AdminDoctorOutput, annotations: contractAnnotations(adminDoctorTool) }, async () => this.toolResult(await this.adminDoctor()), ); register( "loopover_admin_tail_logs", - { description: adminTailLogsTool.description, inputSchema: AdminTailLogsInput.shape, outputSchema: AdminTailLogsOutput, annotations: contractAnnotations(adminTailLogsTool) }, + { description: adminTailLogsTool.description, inputSchema: AdminTailLogsInput, outputSchema: AdminTailLogsOutput, annotations: contractAnnotations(adminTailLogsTool) }, async (input) => this.toolResult(await this.adminTailLogs(input)), ); register( "loopover_admin_get_backup_status", - { description: adminGetBackupStatusTool.description, inputSchema: AdminGetBackupStatusInput.shape, outputSchema: AdminGetBackupStatusOutput, annotations: contractAnnotations(adminGetBackupStatusTool) }, + { description: adminGetBackupStatusTool.description, inputSchema: AdminGetBackupStatusInput, outputSchema: AdminGetBackupStatusOutput, annotations: contractAnnotations(adminGetBackupStatusTool) }, async () => this.toolResult(await this.adminGetBackupStatus()), ); register( @@ -2557,7 +2562,7 @@ export class LoopoverMcp { "loopover_ops_list_dead_letter_jobs", { description: opsListDeadLetterJobsTool.description, - inputSchema: OpsListDeadLetterJobsInput.shape, + inputSchema: OpsListDeadLetterJobsInput, outputSchema: OpsListDeadLetterJobsOutput, annotations: contractAnnotations(opsListDeadLetterJobsTool), }, @@ -2567,7 +2572,7 @@ export class LoopoverMcp { "loopover_ops_replay_dead_letter_job", { description: opsReplayDeadLetterJobTool.description, - inputSchema: OpsReplayDeadLetterJobInput.shape, + inputSchema: OpsReplayDeadLetterJobInput, outputSchema: OpsReplayDeadLetterJobOutput, annotations: contractAnnotations(opsReplayDeadLetterJobTool), }, @@ -2577,7 +2582,7 @@ export class LoopoverMcp { "loopover_ops_delete_dead_letter_job", { description: opsDeleteDeadLetterJobTool.description, - inputSchema: OpsDeleteDeadLetterJobInput.shape, + inputSchema: OpsDeleteDeadLetterJobInput, outputSchema: OpsDeleteDeadLetterJobOutput, annotations: contractAnnotations(opsDeleteDeadLetterJobTool), }, @@ -2587,7 +2592,7 @@ export class LoopoverMcp { "loopover_ops_purge_dead_letter_jobs", { description: opsPurgeDeadLetterJobsTool.description, - inputSchema: OpsPurgeDeadLetterJobsInput.shape, + inputSchema: OpsPurgeDeadLetterJobsInput, outputSchema: OpsPurgeDeadLetterJobsOutput, annotations: contractAnnotations(opsPurgeDeadLetterJobsTool), }, @@ -2597,7 +2602,7 @@ export class LoopoverMcp { "loopover_ops_get_kill_switch", { description: opsGetKillSwitchTool.description, - inputSchema: OpsGetKillSwitchInput.shape, + inputSchema: OpsGetKillSwitchInput, outputSchema: OpsGetKillSwitchOutput, annotations: contractAnnotations(opsGetKillSwitchTool), }, @@ -2607,7 +2612,7 @@ export class LoopoverMcp { "loopover_ops_set_kill_switch", { description: opsSetKillSwitchTool.description, - inputSchema: OpsSetKillSwitchInput.shape, + inputSchema: OpsSetKillSwitchInput, outputSchema: OpsSetKillSwitchOutput, annotations: contractAnnotations(opsSetKillSwitchTool), }, @@ -2617,7 +2622,7 @@ export class LoopoverMcp { "loopover_ops_get_operator_dashboard", { description: opsGetOperatorDashboardTool.description, - inputSchema: OpsGetOperatorDashboardInput.shape, + inputSchema: OpsGetOperatorDashboardInput, outputSchema: OpsGetOperatorDashboardOutput, annotations: contractAnnotations(opsGetOperatorDashboardTool), }, @@ -2631,88 +2636,88 @@ export class LoopoverMcp { // checks -- so registration is never the gate. register( "loopover_fleet_list_instances", - { description: fleetListInstancesTool.description, inputSchema: FleetListInstancesInput.shape, outputSchema: FleetListInstancesOutput, annotations: contractAnnotations(fleetListInstancesTool) }, + { description: fleetListInstancesTool.description, inputSchema: FleetListInstancesInput, outputSchema: FleetListInstancesOutput, annotations: contractAnnotations(fleetListInstancesTool) }, async () => this.toolResult(await this.fleetListInstances()), ); register( "loopover_fleet_register_instance", - { description: fleetRegisterInstanceTool.description, inputSchema: FleetRegisterInstanceInput.shape, outputSchema: FleetRegisterInstanceOutput, annotations: contractAnnotations(fleetRegisterInstanceTool) }, + { description: fleetRegisterInstanceTool.description, inputSchema: FleetRegisterInstanceInput, outputSchema: FleetRegisterInstanceOutput, annotations: contractAnnotations(fleetRegisterInstanceTool) }, async (input) => this.toolResult(await this.fleetRegisterInstance(input)), ); register( "loopover_fleet_list_installations", - { description: fleetListInstallationsTool.description, inputSchema: FleetListInstallationsInput.shape, outputSchema: FleetListInstallationsOutput, annotations: contractAnnotations(fleetListInstallationsTool) }, + { description: fleetListInstallationsTool.description, inputSchema: FleetListInstallationsInput, outputSchema: FleetListInstallationsOutput, annotations: contractAnnotations(fleetListInstallationsTool) }, async () => this.toolResult(await this.fleetListInstallations()), ); register( "loopover_fleet_register_installation", - { description: fleetRegisterInstallationTool.description, inputSchema: FleetRegisterInstallationInput.shape, outputSchema: FleetRegisterInstallationOutput, annotations: contractAnnotations(fleetRegisterInstallationTool) }, + { description: fleetRegisterInstallationTool.description, inputSchema: FleetRegisterInstallationInput, outputSchema: FleetRegisterInstallationOutput, annotations: contractAnnotations(fleetRegisterInstallationTool) }, async (input) => this.toolResult(await this.fleetRegisterInstallation(input)), ); register( "loopover_fleet_backfill_installations", - { description: fleetBackfillInstallationsTool.description, inputSchema: FleetBackfillInstallationsInput.shape, outputSchema: FleetBackfillInstallationsOutput, annotations: contractAnnotations(fleetBackfillInstallationsTool) }, + { description: fleetBackfillInstallationsTool.description, inputSchema: FleetBackfillInstallationsInput, outputSchema: FleetBackfillInstallationsOutput, annotations: contractAnnotations(fleetBackfillInstallationsTool) }, async () => this.toolResult(await this.fleetBackfillInstallations()), ); register( "loopover_fleet_issue_enrollment", - { description: fleetIssueEnrollmentTool.description, inputSchema: FleetIssueEnrollmentInput.shape, outputSchema: FleetEnrollmentOutput, annotations: contractAnnotations(fleetIssueEnrollmentTool) }, + { description: fleetIssueEnrollmentTool.description, inputSchema: FleetIssueEnrollmentInput, outputSchema: FleetEnrollmentOutput, annotations: contractAnnotations(fleetIssueEnrollmentTool) }, async (input) => this.toolResult(await this.fleetIssueEnrollment(input)), ); register( "loopover_fleet_rotate_enrollment", - { description: fleetRotateEnrollmentTool.description, inputSchema: FleetRotateEnrollmentInput.shape, outputSchema: FleetEnrollmentOutput, annotations: contractAnnotations(fleetRotateEnrollmentTool) }, + { description: fleetRotateEnrollmentTool.description, inputSchema: FleetRotateEnrollmentInput, outputSchema: FleetEnrollmentOutput, annotations: contractAnnotations(fleetRotateEnrollmentTool) }, // Rotation IS issuance with rotate forced on -- one implementation, so the two can never diverge on // what "replace the live enrollment" means. async (input) => this.toolResult(await this.fleetIssueEnrollment({ ...input, rotate: true })), ); register( "loopover_fleet_revoke_enrollment", - { description: fleetRevokeEnrollmentTool.description, inputSchema: FleetRevokeEnrollmentInput.shape, outputSchema: FleetRevokeEnrollmentOutput, annotations: contractAnnotations(fleetRevokeEnrollmentTool) }, + { description: fleetRevokeEnrollmentTool.description, inputSchema: FleetRevokeEnrollmentInput, outputSchema: FleetRevokeEnrollmentOutput, annotations: contractAnnotations(fleetRevokeEnrollmentTool) }, async (input, extra) => this.toolResult(await this.fleetRevokeEnrollment(input, extra, server)), ); register( "loopover_fleet_config_push", - { description: fleetConfigPushTool.description, inputSchema: FleetConfigPushInput.shape, outputSchema: FleetConfigPushOutput, annotations: contractAnnotations(fleetConfigPushTool) }, + { description: fleetConfigPushTool.description, inputSchema: FleetConfigPushInput, outputSchema: FleetConfigPushOutput, annotations: contractAnnotations(fleetConfigPushTool) }, async (input, extra) => this.toolResult(await this.fleetConfigPush(input, extra, server)), ); register( "loopover_fleet_run_job", - { description: fleetRunJobTool.description, inputSchema: FleetRunJobInput.shape, outputSchema: FleetRunJobOutput, annotations: contractAnnotations(fleetRunJobTool) }, + { description: fleetRunJobTool.description, inputSchema: FleetRunJobInput, outputSchema: FleetRunJobOutput, annotations: contractAnnotations(fleetRunJobTool) }, async (input) => this.toolResult(await this.fleetRunJob(input)), ); // ── #9522 hosted-tenant tools ────────────────────────────────────── register( "loopover_tenant_create", - { description: tenantCreateTool.description, inputSchema: TenantCreateInput.shape, outputSchema: TenantCreateOutput, annotations: contractAnnotations(tenantCreateTool) }, + { description: tenantCreateTool.description, inputSchema: TenantCreateInput, outputSchema: TenantCreateOutput, annotations: contractAnnotations(tenantCreateTool) }, async (input) => this.toolResult(await this.tenantCreate(input)), ); register( "loopover_tenant_list", - { description: tenantListTool.description, inputSchema: TenantListInput.shape, outputSchema: TenantListOutput, annotations: contractAnnotations(tenantListTool) }, + { description: tenantListTool.description, inputSchema: TenantListInput, outputSchema: TenantListOutput, annotations: contractAnnotations(tenantListTool) }, async () => this.toolResult(await this.tenantList()), ); register( "loopover_tenant_set_orb_installation", - { description: tenantSetOrbInstallationTool.description, inputSchema: TenantSetOrbInstallationInput.shape, outputSchema: TenantSetOrbInstallationOutput, annotations: contractAnnotations(tenantSetOrbInstallationTool) }, + { description: tenantSetOrbInstallationTool.description, inputSchema: TenantSetOrbInstallationInput, outputSchema: TenantSetOrbInstallationOutput, annotations: contractAnnotations(tenantSetOrbInstallationTool) }, async (input) => this.toolResult(await this.tenantSetOrbInstallation(input)), ); register( "loopover_tenant_destroy", - { description: tenantDestroyTool.description, inputSchema: TenantDestroyInput.shape, outputSchema: TenantDestroyOutput, annotations: contractAnnotations(tenantDestroyTool) }, + { description: tenantDestroyTool.description, inputSchema: TenantDestroyInput, outputSchema: TenantDestroyOutput, annotations: contractAnnotations(tenantDestroyTool) }, 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) }, + { description: amsTenantHealthTool.description, inputSchema: AmsTenantHealthInput, 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) }, + { description: amsTenantWakeTool.description, inputSchema: AmsTenantWakeInput, outputSchema: AmsTenantWakeOutput, annotations: contractAnnotations(amsTenantWakeTool) }, async (input) => this.toolResult(await this.amsTenantWake(input)), ); diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index a518899ae..5857d5117 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -1,4 +1,6 @@ import { z } from "zod"; +// #9762: the enums these schemas validate against, from the one place that defines them. +import { AGENT_ACTION_CLASSES, AUTONOMY_LEVELS, FEASIBILITY_VERDICTS, PUBLIC_SURFACE_SKIP_REASONS } from "@loopover/contract"; import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions"; import { MAX_CONTRIBUTOR_OPEN_ITEM_CAP } from "../types"; import { @@ -873,8 +875,8 @@ export const RepositorySettingsSchema = z // exact drift class #9517's enum notes warned about, republished here. The compile-time parity // assertion in src/openapi/schema-type-parity.ts is what finally caught it. autonomy: z.partialRecord( - z.enum(["review", "request_changes", "approve", "merge", "close", "label", "review_state_label", "update_branch", "assign"]), - z.enum(["observe", "auto_with_approval", "auto"]), + z.enum(AGENT_ACTION_CLASSES), + z.enum(AUTONOMY_LEVELS), ), autoMaintain: z.object({ requireApprovals: z.number().int(), mergeMethod: z.enum(["merge", "squash", "rebase"]) }).optional(), agentPaused: z.boolean().optional(), @@ -1158,7 +1160,7 @@ export const RepoSettingsPreviewSchema = z willLabel: z.boolean(), willCheckRun: z.boolean(), skipped: z.boolean(), - skipReason: z.enum(["surface_off", "missing_author", "bot_author", "ignored_author", "maintainer_author", "miner_detection_unavailable", "not_official_gittensor_miner"]).nullable(), + skipReason: z.enum(PUBLIC_SURFACE_SKIP_REASONS).nullable(), actions: z.array(z.enum(["skip", "comment", "label", "check_run", "none"])), summary: z.string(), }), @@ -2296,7 +2298,7 @@ export const IntakeIdeaRequestSchema = z export const IntakeIdeaResponseSchema = z .object({ ok: z.boolean(), - verdict: z.enum(["go", "raise", "avoid"]).optional(), + verdict: z.enum(FEASIBILITY_VERDICTS).optional(), taskGraph: z.unknown().optional(), errors: z.array(z.string()).optional(), }) @@ -2331,7 +2333,7 @@ export const PlanIdeaClaimsRequestSchema = z export const PlanIdeaClaimsResponseSchema = z .object({ ok: z.boolean(), - verdict: z.enum(["go", "raise", "avoid"]).optional(), + verdict: z.enum(FEASIBILITY_VERDICTS).optional(), claimPlan: z.unknown().optional(), errors: z.array(z.string()).optional(), }) diff --git a/src/openapi/spec.ts b/src/openapi/spec.ts index 8e36ce144..703f08f61 100644 --- a/src/openapi/spec.ts +++ b/src/openapi/spec.ts @@ -1,4 +1,5 @@ import { OpenApiGeneratorV3, OpenAPIRegistry } from "@asteasolutions/zod-to-openapi"; +import { PUBLIC_SURFACE_SKIP_REASONS } from "@loopover/contract"; import { requiresApiToken } from "../auth/route-auth"; import { registerDiscoveryRouteSpecs } from "./discovery-route-specs"; import { registerOrbAndControlRouteSpecs } from "./orb-and-control-route-specs"; @@ -2206,7 +2207,7 @@ export function buildOpenApiSpec() { param: { description: "Optional repository filter. Browser sessions must have control-panel access to this repo." }, example: "JSONbored/loopover", }), - reason: z.enum(["surface_off", "missing_author", "bot_author", "ignored_author", "maintainer_author", "miner_detection_unavailable", "not_official_gittensor_miner"]).optional().openapi({ + reason: z.enum(PUBLIC_SURFACE_SKIP_REASONS).optional().openapi({ param: { description: "Optional PR skip reason filter." }, example: "not_official_gittensor_miner", }), diff --git a/test/unit/mcp-cli-maintain.test.ts b/test/unit/mcp-cli-maintain.test.ts index 9b15732a2..08abfb32e 100644 --- a/test/unit/mcp-cli-maintain.test.ts +++ b/test/unit/mcp-cli-maintain.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -10,6 +10,7 @@ import { it, vi, } from "vitest"; +import { AUTONOMY_LEVELS as CONTRACT_AUTONOMY_LEVELS } from "@loopover/contract"; import { AUTONOMY_LEVELS } from "../../src/settings/autonomy"; // TS5097: keep the .ts specifier out of a literal import() position (same indirection as the template). @@ -20,22 +21,14 @@ import { startFixtureServer, } from "./support/mcp-cli-harness"; -// #6153: MAINTAIN_AUTONOMY_LEVELS is a hand-synced copy of the live enum (the CLI reaches @loopover/engine only -// through its published export map, which doesn't surface AUTONOMY_LEVELS), so nothing but a test can catch the -// two drifting apart. The source is parsed rather than imported because bin/loopover-mcp.js is an executable -// entrypoint that starts a server on import. -const CLI_SOURCE = readFileSync( - join(process.cwd(), "packages/loopover-mcp/dist/bin/loopover-mcp.js"), - "utf8", -); - -/** The `maintain set-level` levels the committed CLI source really accepts. */ -function declaredLevels(): string[] { - const raw = - /const MAINTAIN_AUTONOMY_LEVELS = \[([^\]]*)\];/.exec(CLI_SOURCE)?.[1] ?? - ""; - return [...raw.matchAll(/"([^"]+)"/g)].map((m) => m[1]!); -} +// #6153: the CLI carried its own hand-synced copy of the autonomy levels, because it reaches +// @loopover/engine through a published export map that does not surface AUTONOMY_LEVELS -- and it drifted, +// accepting "suggest"/"propose" for the whole life of #4620 after the server dropped them. #9762 deleted the +// copy: the bin imports the list from @loopover/contract, which both packages can reach. +// +// So the guard below stopped being a text comparison and became a behavioural one. It runs `set-level` for +// every level the live enum declares and asserts each is accepted, then asserts an invented level is not -- +// which is the property that actually matters, and one no import-shaped assertion could establish. // #8587: these cases assert JSON / plain business output that the exported runCli produces in-process (the // isProcessEntrypoint guard lets the committed .ts source be imported without hijacking argv), so they no longer @@ -670,10 +663,24 @@ describe("loopover-mcp CLI — maintain (#784)", () => { ).rejects.toThrow(/Unknown level/); }, 45_000); - // Pins the INVARIANT (the two lists agree), not today's three values -- restating the literal here would just - // create a third hand-synced copy that rots alongside the one this guards. - it("set-level's levels stay in sync with the live autonomy enum (#6153)", () => { - expect(declaredLevels()).toEqual([...AUTONOMY_LEVELS]); + // Pins the INVARIANT -- the CLI accepts exactly what the live enum declares -- by EXERCISING it, not by + // reading the source. #9762 removed the hand-synced literal this used to scrape, and a grep for the + // replacement import would assert how the code is written rather than what it does: it would pass just as + // happily on a CLI that imported the list and then ignored it. + it("accepts every level the live autonomy enum declares (#6153, #9762)", async () => { + for (const level of AUTONOMY_LEVELS) { + await expect(cli(["maintain", "set-level", "review", level, "--repo", "owner/repo", "--json"])).resolves.toBeDefined(); + } + }, 45_000); + + it("accepts NOTHING outside it, and says so naming the live enum (#6153, #9762)", async () => { + // The #6153 defect: the CLI carried "suggest"/"propose" for the whole life of #4620, after the server had + // dropped them. The rejection message is derived from the same enum, so a level added server-side cannot + // leave the error text stale either. + await expect(cli(["maintain", "set-level", "review", "definitely-not-a-level", "--repo", "owner/repo"])).rejects.toThrow( + new RegExp(AUTONOMY_LEVELS.join(", ")), + ); + expect([...CONTRACT_AUTONOMY_LEVELS], "the value the CLI imports is still the engine's").toEqual([...AUTONOMY_LEVELS]); }); // #6153 regression: the CLI accepted "suggest"/"propose" for the whole life of #4620, which dropped them diff --git a/test/unit/mcp-register-tool-shape-guard.test.ts b/test/unit/mcp-register-tool-shape-guard.test.ts new file mode 100644 index 000000000..6dff0f9db --- /dev/null +++ b/test/unit/mcp-register-tool-shape-guard.test.ts @@ -0,0 +1,100 @@ +import { readFileSync, readdirSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { AGENT_ACTION_CLASSES, AUTONOMY_LEVELS, MAINTAIN_ACTION_CLASSES, PROPOSE_ACTION_CLASSES } from "@loopover/contract"; +import { AGENT_ACTION_CLASSES as ENGINE_ACTION_CLASSES } from "../../packages/loopover-engine/src/settings/autonomy"; +import { AUTONOMY_LEVELS as ENGINE_AUTONOMY_LEVELS } from "../../packages/loopover-engine/src/settings/autonomy"; + +// #9762: the two things that kept #9515's premise -- nothing written down twice -- from being true. +// +// The first is a trap rather than a bug. The MCP SDK's registerTool accepts either a ZodObject or a raw +// `.shape`, and re-wraps a raw shape in a plain `z.object` that DISCARDS the catchall. A `looseObject` +// output therefore gets advertised and enforced as `additionalProperties: false`, and every field the +// payload carries beyond the modelled set becomes a -32602 the caller cannot do anything about -- #9518's +// defect class, found the hard way. 23 call sites passed `.shape`; none of their inputs was loose, which is +// exactly what made them indistinguishable from the correct ones by reading. +// +// The second is the pinned restatements. A contract constant that merely MIRRORS an engine list is safe +// only while something fails when the two diverge. + +/** Every `.ts` under a root, so the guard cannot be dodged by adding a new server file. */ +function sourceFiles(root: string): string[] { + const found: string[] = []; + const walk = (dir: string): void => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const path = join(dir, entry.name); + if (entry.isDirectory()) { + if (!/node_modules|dist/.test(path)) walk(path); + } else if (path.endsWith(".ts")) found.push(path); + } + }; + walk(root); + return found; +} + +const SERVER_ROOTS = ["src/mcp", "packages/loopover-mcp/bin", "packages/loopover-mcp/lib", "packages/loopover-miner/lib"]; + +describe("no registerTool call hands the SDK a raw shape (#9762)", () => { + const offenders = SERVER_ROOTS.flatMap((root) => + sourceFiles(root).flatMap((file) => + readFileSync(file, "utf8") + .split("\n") + .map((line, index) => ({ file, line: index + 1, text: line })) + // The pure form only. `{ ...X.shape, extra }` genuinely COMPOSES a new shape and is not this defect. + .filter(({ text }) => /\b(?:input|output)Schema:\s*[A-Za-z0-9_]+\.shape\b/.test(text)) + .map(({ file, line, text }) => `${file}:${line} ${text.trim()}`), + ), + ); + + it("finds none", () => { + expect(offenders).toEqual([]); + }); + + it("would catch one if it came back", () => { + // The guard's own regex, proven against the exact line shape it exists to reject -- otherwise a future + // refactor could silently make the pattern unmatchable and this suite would go quiet. + const pattern = /\b(?:input|output)Schema:\s*[A-Za-z0-9_]+\.shape\b/; + expect(pattern.test(" inputSchema: AdminDoctorInput.shape,")).toBe(true); + expect(pattern.test(" outputSchema: AdminDoctorOutput.shape,")).toBe(true); + // A composed shape is legitimate and must NOT be flagged. + expect(pattern.test(" argsSchema: { ...GetAutomationStateInput.shape, login: z.string() },")).toBe(false); + expect(pattern.test(" inputSchema: AdminDoctorInput,")).toBe(false); + }); +}); + +describe("the contract's restated action-class lists still match the engine's (#9762)", () => { + it("AGENT_ACTION_CLASSES is the engine's list exactly, order included", () => { + expect([...AGENT_ACTION_CLASSES]).toEqual([...ENGINE_ACTION_CLASSES]); + }); + + it("AUTONOMY_LEVELS is the engine's list exactly", () => { + expect([...AUTONOMY_LEVELS]).toEqual([...ENGINE_AUTONOMY_LEVELS]); + }); + + it("MAINTAIN_ACTION_CLASSES stays a strict SUBSET — it is deliberately not the engine's full list", () => { + // Named because "sync it to the engine" is the obvious wrong fix: the maintain dial exposes only the + // operator-settable classes, and widening it would offer an operator switches the surface cannot honor. + for (const cls of MAINTAIN_ACTION_CLASSES) expect(ENGINE_ACTION_CLASSES).toContain(cls); + expect(MAINTAIN_ACTION_CLASSES.length).toBeLessThan(ENGINE_ACTION_CLASSES.length); + }); + + it("PROPOSE_ACTION_CLASSES is the maintain subset plus review_state_label, and nothing else", () => { + expect([...PROPOSE_ACTION_CLASSES]).toEqual([...MAINTAIN_ACTION_CLASSES, "review_state_label"]); + }); +}); + +describe("the stdio bin no longer hand-syncs what the contract exports (#9762)", () => { + const bin = readFileSync("packages/loopover-mcp/bin/loopover-mcp.ts", "utf8"); + + it("declares no literal copy of the autonomy or action-class lists", () => { + expect(bin).not.toContain('["observe", "auto_with_approval", "auto"]'); + expect(bin).not.toContain('["review", "request_changes", "approve", "merge", "close", "label"]'); + }); + + it("dropped the stale comment that said they could not be imported", () => { + // It cited #6153: the bin resolves @loopover/engine through the published package, whose export map + // does not surface AUTONOMY_LEVELS. True of the engine, never true of the contract -- which this file + // already imports. A load-bearing comment that is no longer true is worse than no comment. + expect(bin).not.toContain("hand-synced literals, not imports"); + }); +});