Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/loopover-contract/src/tools/local-branch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
12 changes: 12 additions & 0 deletions packages/loopover-mcp/bin/loopover-mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,8 @@ import {
StdioWatchIssuesInput,
StdioCompareLocalVariantsInput,
StdioComparePrVariantsInput,
StdioLocalBranchAnalysisInput,
StdioDraftPrBodyInput,
getToolContract,
projectToolDefinition,
ListPendingActionsStdioInput,
Expand Down Expand Up @@ -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(
Expand All @@ -1689,6 +1694,7 @@ registerStdioTool(
recommendedRerunCondition: result.analysis.recommendedRerunCondition,
});
},
{ input: StdioLocalBranchAnalysisInput },
);

registerStdioTool(
Expand All @@ -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(
Expand All @@ -1713,6 +1720,7 @@ registerStdioTool(
recommendedRerunCondition: result.analysis.recommendedRerunCondition,
});
},
{ input: StdioLocalBranchAnalysisInput },
);

registerStdioTool(
Expand All @@ -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(
Expand All @@ -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,
Expand All @@ -1755,6 +1765,7 @@ registerStdioTool(
draft,
);
},
{ input: StdioDraftPrBodyInput },
);

registerStdioTool(
Expand Down Expand Up @@ -1823,6 +1834,7 @@ registerStdioTool(
registerStdioTool(
"loopover_agent_prepare_pr_packet",
async (input: z.infer<typeof CurrentBranchInput>) => 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
Expand Down
40 changes: 40 additions & 0 deletions test/unit/contract-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,15 @@ 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";
import { CLAIM_STATUSES as MINER_CLAIM_STATUSES } from "../../packages/loopover-miner/lib/claim-ledger";
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";
Expand Down Expand Up @@ -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<string, unknown>; required?: string[] };
const narrowedSchema = toJsonSchema(narrowing) as { properties?: Record<string, unknown>; 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");
});
});
105 changes: 105 additions & 0 deletions test/unit/mcp-cli-current-branch-input.test.ts
Original file line number Diff line number Diff line change
@@ -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<void> } };

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<string, unknown> }).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);
}
});
});