diff --git a/packages/loopover-mcp/bin/loopover-mcp.js b/packages/loopover-mcp/bin/loopover-mcp.js index 21bf14dfe7..0986eea09c 100755 --- a/packages/loopover-mcp/bin/loopover-mcp.js +++ b/packages/loopover-mcp/bin/loopover-mcp.js @@ -64,9 +64,24 @@ const CLI_COMMAND_SPEC = { }; 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 (must mirror src/settings/autonomy.ts). +// #784 maintain set-level — the autonomy dial's action classes + levels. +// +// 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. +// +// 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", "suggest", "propose", "auto_with_approval", "auto"]; +const MAINTAIN_AUTONOMY_LEVELS = ["observe", "auto_with_approval", "auto"]; const AGENT_PROFILES = { "miner-planner": { id: "miner-planner", diff --git a/src/mcp/server.ts b/src/mcp/server.ts index ab0133605e..7665f6a0e6 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -551,11 +551,11 @@ const setAgentPausedOutputSchema = { agentPaused: z.boolean().optional(), }; -// `action` mirrors the CLI's MAINTAIN_ACTION_CLASSES exactly (loopover-mcp.js:65). `level` intentionally -// validates against the LIVE AUTONOMY_LEVELS (src/settings/autonomy.ts), not the CLI's own stale -// MAINTAIN_AUTONOMY_LEVELS -- that list still carries "suggest"/"propose", both removed server-side by #4620 -// and silently dropped by normalizeAutonomyPolicy on persist, so accepting them here would report success on a -// write that never actually took effect. +// `action` mirrors the CLI's MAINTAIN_ACTION_CLASSES exactly (loopover-mcp.js). `level` validates against the +// LIVE AUTONOMY_LEVELS (src/settings/autonomy.ts) rather than restating one: "suggest"/"propose" were removed +// server-side by #4620 and are silently dropped by normalizeAutonomyPolicy on persist, so accepting either here +// would report success on a write that never actually took effect. (#6153: the CLI's own MAINTAIN_AUTONOMY_LEVELS +// carried both until then -- binding to the live enum is what kept this surface correct while that one drifted.) const MAINTAIN_AUTONOMY_ACTION_CLASSES = ["review", "request_changes", "approve", "merge", "close", "label"] as const; const setActionAutonomyShape = { diff --git a/test/unit/mcp-cli-maintain.test.ts b/test/unit/mcp-cli-maintain.test.ts index 401bf4860b..f80641974b 100644 --- a/test/unit/mcp-cli-maintain.test.ts +++ b/test/unit/mcp-cli-maintain.test.ts @@ -1,9 +1,22 @@ -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; +import { AUTONOMY_LEVELS } from "../../src/settings/autonomy"; import { closeFixtureServer, runAsync, 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/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]!); +} + describe("loopover-mcp CLI — maintain (#784)", () => { let tempDir: string | null = null; @@ -92,6 +105,31 @@ describe("loopover-mcp CLI — maintain (#784)", () => { await expect(runAsync(["maintain", "set-level", "merge", "bogus", "--repo", "owner/repo"], e)).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]); + }); + + // #6153 regression: the CLI accepted "suggest"/"propose" for the whole life of #4620, which dropped them + // server-side. The fixture's PUT /settings echoes any autonomy body back as a success, exactly like a server + // with no enum -- so a rejection here can only have come from the CLI's own check, before any round-trip. + it("rejects levels #4620 removed server-side, client-side rather than via a 400 (#6153)", async () => { + const e = await env(); + for (const removed of ["suggest", "propose"]) { + // Derived from the live enum for the same reason as above: the point is that the error names exactly the + // levels the server accepts, not that it names three particular strings. + await expect(runAsync(["maintain", "set-level", "review", removed, "--repo", "owner/repo"], e)).rejects.toThrow( + new RegExp(`Unknown level: ${removed}\\. Use ${AUTONOMY_LEVELS.join(", ")}\\.`), + ); + } + // The dial still accepts every level the server does -- the fix narrowed the list, it didn't break it. + const json = JSON.parse(await runAsync(["maintain", "set-level", "review", "observe", "--repo", "owner/repo", "--json"], e)) as { + autonomy: Record; + }; + expect(json.autonomy).toMatchObject({ review: "observe" }); + }, 45_000); + it("prints help when invoked with no subcommand", async () => { const e = await env(); const out = await runAsync(["maintain"], e);