From de102cfd52ff2f743e9376222c8cdcf9166baef6 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:36:51 +0000 Subject: [PATCH] mcp(stdio): restore the CurrentBranchInput narrowing on the eight local-branch tools LocalBranchAnalysisInput widened login/repoFullName to required so the remote server (no checkout) could advertise the full vocabulary, but the eight stdio registrations of that contract never narrowed back down -- so the stdio server advertised and enforced the same required fields the remote does, even though its handlers resolve both from the active session and the checkout. Declares a StdioLocalBranchAnalysisInput/StdioDraftPrBodyInput narrowing derived from the contract (mirroring StdioMarkNotificationsReadInput) and wires it into the eight registerStdioTool call sites via the existing input override seam. --- .../src/tools/local-branch.ts | 12 ++ packages/loopover-mcp/bin/loopover-mcp.ts | 12 ++ test/unit/contract-registry.test.ts | 40 +++++++ .../unit/mcp-cli-current-branch-input.test.ts | 105 ++++++++++++++++++ 4 files changed, 169 insertions(+) create mode 100644 test/unit/mcp-cli-current-branch-input.test.ts diff --git a/packages/loopover-contract/src/tools/local-branch.ts b/packages/loopover-contract/src/tools/local-branch.ts index aa7e897b8..1153f8a14 100644 --- a/packages/loopover-contract/src/tools/local-branch.ts +++ b/packages/loopover-contract/src/tools/local-branch.ts @@ -133,6 +133,14 @@ export const LocalBranchAnalysisInput = CurrentBranchInput.extend({ .optional(), }); +/** + * What the STDIO server serves: it reads `login`/`repoFullName` off the active session and the + * checkout's own remote instead of asking the caller to restate them (#10034). Derived rather than + * hand-written, per the rule the registry states: a narrowing is only ever a `.partial()`/`.omit()` + * of the contract it narrows, never a fresh `z.object({...})` beside the registration. + */ +export const StdioLocalBranchAnalysisInput = LocalBranchAnalysisInput.partial({ login: true, repoFullName: true }); + export const preflightCurrentBranchTool = defineTool({ name: "loopover_preflight_current_branch", title: "Preflight current branch", @@ -242,6 +250,10 @@ export const reviewPrBeforePushTool = defineTool({ export const DraftPrBodyInput = LocalBranchAnalysisInput.extend({ format: z.enum(["json", "markdown"]).optional(), }); + +/** Same stdio narrowing as `StdioLocalBranchAnalysisInput`, plus the `format` field this tool alone takes. */ +export const StdioDraftPrBodyInput = DraftPrBodyInput.partial({ login: true, repoFullName: true }); + export const draftPrBodyTool = defineTool({ name: "loopover_draft_pr_body", title: "Draft PR body", diff --git a/packages/loopover-mcp/bin/loopover-mcp.ts b/packages/loopover-mcp/bin/loopover-mcp.ts index 3cf119ca7..c733a5e96 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.ts +++ b/packages/loopover-mcp/bin/loopover-mcp.ts @@ -166,6 +166,8 @@ import { StdioWatchIssuesInput, StdioCompareLocalVariantsInput, StdioComparePrVariantsInput, + StdioLocalBranchAnalysisInput, + StdioDraftPrBodyInput, getToolContract, projectToolDefinition, ListPendingActionsStdioInput, @@ -1670,6 +1672,9 @@ registerStdioTool( workspaceIntelligence: publicSafeWorkspaceIntelligence(result.analysis.workspaceIntelligence), }); }, + // #10034: this server resolves login/repoFullName from the checkout and active session, so it + // accepts a call without either -- stated as a declared narrowing of the contract's wider shape. + { input: StdioLocalBranchAnalysisInput }, ); registerStdioTool( @@ -1689,6 +1694,7 @@ registerStdioTool( recommendedRerunCondition: result.analysis.recommendedRerunCondition, }); }, + { input: StdioLocalBranchAnalysisInput }, ); registerStdioTool( @@ -1697,6 +1703,7 @@ registerStdioTool( const result = await analyzeCurrentBranch(await withClientWorkspaceRoots(input)); return toolResult("LoopOver local next-action ranking.", { local: result.local, nextActions: result.analysis.nextActions, rewardRisk: result.analysis.rewardRisk, recommendedRerunCondition: result.analysis.recommendedRerunCondition }); }, + { input: StdioLocalBranchAnalysisInput }, ); registerStdioTool( @@ -1713,6 +1720,7 @@ registerStdioTool( recommendedRerunCondition: result.analysis.recommendedRerunCondition, }); }, + { input: StdioLocalBranchAnalysisInput }, ); registerStdioTool( @@ -1723,6 +1731,7 @@ registerStdioTool( const { localScorerStatus: _localScorerStatus, ...body } = payload; return toolResult("LoopOver remediation plan.", await apiPost("/v1/local/remediation-plan", body)); }, + { input: StdioLocalBranchAnalysisInput }, ); registerStdioTool( @@ -1731,6 +1740,7 @@ registerStdioTool( const result = await analyzeCurrentBranch(await withClientWorkspaceRoots(input)); return toolResult("LoopOver public-safe PR packet.", { local: result.local, prPacket: result.analysis.prPacket }); }, + { input: StdioLocalBranchAnalysisInput }, ); // #6741: CLI stdio mirror of loopover_draft_pr_body — same analyzeCurrentBranch fetch as prepare_pr_packet, @@ -1755,6 +1765,7 @@ registerStdioTool( draft, ); }, + { input: StdioDraftPrBodyInput }, ); registerStdioTool( @@ -1823,6 +1834,7 @@ registerStdioTool( registerStdioTool( "loopover_agent_prepare_pr_packet", async (input: z.infer) => toolResult("LoopOver base-agent public-safe PR packet.", await agentPreparePrPacket(await withClientWorkspaceRoots(input))), + { input: StdioLocalBranchAnalysisInput }, ); // Only this tool declares an outputSchema today; every other tool returns text + unschematized diff --git a/test/unit/contract-registry.test.ts b/test/unit/contract-registry.test.ts index 06e9907e4..30d7e4ea9 100644 --- a/test/unit/contract-registry.test.ts +++ b/test/unit/contract-registry.test.ts @@ -30,6 +30,7 @@ import { QUEUE_STATUSES, CLAIM_STATUSES, MINER_RUN_STATES, + toJsonSchema, } from "@loopover/contract"; import { TEST_FRAMEWORKS as ENGINE_TEST_FRAMEWORKS } from "../../packages/loopover-engine/src/signals/test-evidence"; import { QUEUE_STATUSES as MINER_QUEUE_STATUSES } from "../../packages/loopover-miner/lib/portfolio-queue"; @@ -37,6 +38,7 @@ import { CLAIM_STATUSES as MINER_CLAIM_STATUSES } from "../../packages/loopover- import { RUN_STATES as LIVE_MINER_RUN_STATES } from "../../packages/loopover-miner/lib/run-state"; import { LocalStatusStructuredInput } from "@loopover/contract/tools"; import { GetRepoContextInput } from "@loopover/contract/tools"; +import { StdioLocalBranchAnalysisInput, StdioDraftPrBodyInput } from "@loopover/contract/tools"; import { PREFLIGHT_LIMITS as ENGINE_PREFLIGHT_LIMITS } from "../../packages/loopover-engine/src/signals/preflight-limits"; import { PUBLIC_SURFACE_SKIP_REASONS as SERVER_PUBLIC_SURFACE_SKIP_REASONS } from "../../src/signals/settings-preview"; import { AUTONOMY_LEVELS as ENGINE_AUTONOMY_LEVELS, AGENT_ACTION_CLASSES as ENGINE_AGENT_ACTION_CLASSES } from "../../packages/loopover-engine/src/settings/autonomy"; @@ -319,4 +321,42 @@ describe("contract enums", () => { expect(PLAN_STEP_STATUSES as readonly string[], status).toContain(status); } }); + + // #10034: the eight local-branch tools the stdio server narrows away from LocalBranchAnalysisInput's + // required login/repoFullName. Asserts both halves checkInputNarrowing (scripts/lib/validate-mcp) + // cares about: the narrowing requires no MORE than the contract, and declares no property the + // contract does not already have. + it("narrows the eight stdio local-branch tools to drop the required login/repoFullName", () => { + const STDIO_LOCAL_BRANCH_TOOLS = [ + { name: "loopover_preflight_current_branch", narrowing: StdioLocalBranchAnalysisInput }, + { name: "loopover_preview_current_branch_score", narrowing: StdioLocalBranchAnalysisInput }, + { name: "loopover_rank_local_next_actions", narrowing: StdioLocalBranchAnalysisInput }, + { name: "loopover_explain_local_blockers", narrowing: StdioLocalBranchAnalysisInput }, + { name: "loopover_remediation_plan", narrowing: StdioLocalBranchAnalysisInput }, + { name: "loopover_prepare_pr_packet", narrowing: StdioLocalBranchAnalysisInput }, + { name: "loopover_draft_pr_body", narrowing: StdioDraftPrBodyInput }, + { name: "loopover_agent_prepare_pr_packet", narrowing: StdioLocalBranchAnalysisInput }, + ]; + for (const { name, narrowing } of STDIO_LOCAL_BRANCH_TOOLS) { + const contract = getToolContract(name); + expect(contract, name).toBeDefined(); + const contractSchema = toJsonSchema(contract!.input) as { properties?: Record; required?: string[] }; + const narrowedSchema = toJsonSchema(narrowing) as { properties?: Record; required?: string[] }; + const contractRequired = new Set(contractSchema.required ?? []); + const narrowedRequired = narrowedSchema.required ?? []; + // Every property the narrowing requires, the contract requires too -- the left half of + // checkInputNarrowing, which is what makes advertising this shape a true narrowing. + for (const property of narrowedRequired) expect(contractRequired.has(property), `${name}: ${property}`).toBe(true); + expect(narrowedRequired, name).not.toContain("login"); + expect(narrowedRequired, name).not.toContain("repoFullName"); + // Every property the narrowing declares also exists on the contract's input. + const contractProperties = new Set(Object.keys(contractSchema.properties ?? {})); + for (const property of Object.keys(narrowedSchema.properties ?? {})) { + expect(contractProperties.has(property), `${name}: ${property}`).toBe(true); + } + } + // loopover_draft_pr_body's narrowing must still declare `format` -- the one field the issue + // called out as easy to drop by narrowing from the wrong base. + expect(Object.keys(toJsonSchema(StdioDraftPrBodyInput).properties ?? {})).toContain("format"); + }); }); diff --git a/test/unit/mcp-cli-current-branch-input.test.ts b/test/unit/mcp-cli-current-branch-input.test.ts new file mode 100644 index 000000000..8b1416f2a --- /dev/null +++ b/test/unit/mcp-cli-current-branch-input.test.ts @@ -0,0 +1,105 @@ +// REGRESSION: the stdio branch tools must not demand a login the CLI resolves itself (#10034). +// +// LocalBranchAnalysisInput made `login`/`repoFullName` required so the REMOTE server (which has no +// checkout) could advertise the full vocabulary a caller may supply. The eight stdio registrations +// of that contract never narrowed back down, so `tools/list` advertised the same required fields the +// remote does, even though the stdio handler resolves both from the active session / checkout and +// never needs them. Same in-process InMemoryTransport pattern as test/contract/validate-mcp.test.ts. +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { closeFixtureServer, startFixtureServer } from "./support/mcp-cli-harness"; + +const STDIO_LOCAL_BRANCH_TOOL_NAMES = [ + "loopover_preflight_current_branch", + "loopover_preview_current_branch_score", + "loopover_rank_local_next_actions", + "loopover_explain_local_blockers", + "loopover_remediation_plan", + "loopover_prepare_pr_packet", + "loopover_draft_pr_body", + "loopover_agent_prepare_pr_packet", +] as const; + +// The bin lives at ../../packages/loopover-mcp/bin/loopover-mcp.ts. Routed through a variable +// (rather than a literal import(".../loopover-mcp.ts")) because tsc's `--noEmit` root build rejects +// a literal `.ts`-suffixed specifier without `allowImportingTsExtensions` -- same indirection +// test/unit/mcp-cli-contributor-profile-inprocess.test.ts uses for the same reason. +const BIN_MODULE_SPECIFIER = ["..", "..", "packages", "loopover-mcp", "bin", "loopover-mcp.ts"].join("/"); + +type BinModule = { server: { connect: (transport: unknown) => Promise } }; + +let tempDir = ""; +let mod: BinModule; + +beforeAll(async () => { + tempDir = mkdtempSync(join(tmpdir(), "loopover-current-branch-input-")); + const apiUrl = await startFixtureServer(); + // The bin reads LOOPOVER_API_URL at module load, so set the env BEFORE importing (hence the dynamic import). + process.env.LOOPOVER_API_URL = apiUrl; + process.env.LOOPOVER_API_TOKEN = "in-process-token"; + process.env.LOOPOVER_API_TIMEOUT_MS = "2000"; + process.env.LOOPOVER_CONFIG_DIR = tempDir; + process.env.LOOPOVER_SKIP_NPM_VERSION_CHECK = "1"; + mod = (await import(BIN_MODULE_SPECIFIER)) as unknown as BinModule; +}, 120_000); + +afterAll(async () => { + await closeFixtureServer(); + if (tempDir) rmSync(tempDir, { recursive: true, force: true }); + delete process.env.LOOPOVER_API_URL; + delete process.env.LOOPOVER_API_TOKEN; + delete process.env.LOOPOVER_API_TIMEOUT_MS; + delete process.env.LOOPOVER_CONFIG_DIR; + delete process.env.LOOPOVER_SKIP_NPM_VERSION_CHECK; +}); + +describe("REGRESSION: the stdio branch tools must not demand a login the CLI resolves itself (#10034)", () => { + it("advertises no required login/repoFullName for any of the eight local-branch tools", async () => { + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await mod.server.connect(serverTransport); + const client = new Client({ name: "current-branch-input-test", version: "0.1.0" }, { capabilities: {} }); + await client.connect(clientTransport); + try { + const { tools } = await client.listTools(); + const byName = new Map(tools.map((tool) => [tool.name, tool])); + for (const name of STDIO_LOCAL_BRANCH_TOOL_NAMES) { + const tool = byName.get(name); + expect(tool, name).toBeDefined(); + const required = (tool!.inputSchema as { required?: string[] }).required ?? []; + expect(required, name).not.toContain("login"); + expect(required, name).not.toContain("repoFullName"); + } + // loopover_draft_pr_body's narrowing must not have dropped `format` along the way. + const draftPrBody = byName.get("loopover_draft_pr_body"); + const properties = Object.keys((draftPrBody!.inputSchema as { properties?: Record }).properties ?? {}); + expect(properties).toContain("format"); + } finally { + await client.close().catch(() => undefined); + } + }); + + it("still reaches the handler with no login/repoFullName — schema validation no longer rejects the ordinary call", async () => { + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await mod.server.connect(serverTransport); + const client = new Client({ name: "current-branch-input-empty-call-test", version: "0.1.0" }, { capabilities: {} }); + await client.connect(clientTransport); + try { + // Called from a non-git tempDir: with the narrowing restored, the SDK's own schema check no + // longer rejects this at -32602 for a missing `login`/`repoFullName` -- it reaches the handler, + // which then fails for the unrelated, expected reason that tempDir is not a git checkout. + let threwProtocolError = false; + try { + await client.callTool({ name: "loopover_preflight_current_branch", arguments: { cwd: tempDir } }); + } catch (error) { + threwProtocolError = error instanceof Error && /-32602/.test(error.message); + } + expect(threwProtocolError).toBe(false); + } finally { + await client.close().catch(() => undefined); + } + }); +});