From fdbdbf88706309b6bf60f274ec1d006a79f4ab31 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:16:33 -0700 Subject: [PATCH] fix(contract): close the last duplications and make the .shape trap impossible (#9762) #9515's premise is that no tool, schema, or enum here is written down twice. Every sub-issue had landed and the premise was still not true in two ways. TWENTY-SEVEN registerTool call sites passed the SDK a raw `.shape` rather than the ZodObject. The SDK accepts either and re-wraps a raw shape 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 -- #9518's defect class, found the hard way. None of those inputs is loose today, which is exactly the problem: the sites were indistinguishable from the correct ones by reading, and the day one becomes loose the failure is silent and remote. A guard test now walks every server source and fails on the pure form, while leaving a composed `{ ...X.shape, extra }` alone, since that genuinely builds a new shape. Six literal enum copies of values @loopover/contract already exports now import them: the settings schema's action classes and autonomy levels, the public-surface skip reasons in both the schema and the spec's query parameter, the feasibility verdicts, and the rotatable secret names. The two in the stdio bin came with a comment justifying them -- the bin resolves @loopover/engine through the PUBLISHED package, whose export map does not surface AUTONOMY_LEVELS (#6153). True of the engine, never true of the contract, which this file already imports. The comment and the literals are gone together; a load-bearing comment that is no longer true is worse than none. AGENT_ACTION_CLASSES joins the contract as its own pinned copy, since the settings response schema needs the engine's full list and cannot import it. Pinned against the engine's, alongside new pins proving MAINTAIN_ACTION_CLASSES stays a strict subset and PROPOSE_ACTION_CLASSES is that subset plus review_state_label -- "sync it to the engine" being the obvious wrong fix for both. The regenerated openapi.json is byte-identical: every substitution has the same members in the same order, which is what makes this a substitution rather than a schema change. Not changed, having checked: the miner's three `["global", "repo", "none"]` literals are kill-switch scopes, not CONFIG_ADMIN_WRITE_SCOPES -- a different concept that shares a prefix. --- packages/loopover-contract/src/enums.ts | 21 ++++ packages/loopover-mcp/bin/loopover-mcp.ts | 32 ++---- src/mcp/server.ts | 71 +++++++------ src/openapi/schemas.ts | 12 ++- src/openapi/spec.ts | 3 +- test/unit/mcp-cli-maintain.test.ts | 49 +++++---- .../mcp-register-tool-shape-guard.test.ts | 100 ++++++++++++++++++ 7 files changed, 207 insertions(+), 81 deletions(-) create mode 100644 test/unit/mcp-register-tool-shape-guard.test.ts 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"); + }); +});