diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index dc8c345010..bb7a536d80 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -17126,6 +17126,109 @@ "funnel", "note" ] + }, + "FindOpportunitiesRequest": { + "type": "object", + "properties": { + "targets": { + "type": "array", + "items": { + "type": "object", + "properties": { + "owner": { + "type": "string", + "minLength": 1, + "maxLength": 39 + }, + "repo": { + "type": "string", + "minLength": 1, + "maxLength": 100 + } + }, + "required": [ + "owner", + "repo" + ] + }, + "maxItems": 25 + }, + "searchQuery": { + "type": "string", + "minLength": 1, + "maxLength": 500 + }, + "goalSpec": { + "type": "object", + "properties": { + "lane": { + "type": "string", + "minLength": 1 + }, + "minRankScore": { + "type": "number", + "minimum": 0, + "maximum": 100 + }, + "languages": { + "type": "array", + "items": { + "type": "string", + "minLength": 1, + "maxLength": 30 + }, + "maxItems": 20 + } + } + }, + "limit": { + "type": "integer", + "minimum": 1, + "maximum": 50 + } + } + }, + "IssueRagRetrieveRequest": { + "type": "object", + "properties": { + "owner": { + "type": "string", + "minLength": 1, + "maxLength": 39 + }, + "repo": { + "type": "string", + "minLength": 1, + "maxLength": 100 + }, + "title": { + "type": "string", + "minLength": 1, + "maxLength": 300 + }, + "body": { + "type": "string", + "maxLength": 20000 + }, + "labels": { + "type": "array", + "items": { + "type": "string", + "maxLength": 100 + }, + "maxItems": 50 + }, + "topK": { + "type": "integer", + "minimum": 1, + "maximum": 12 + } + }, + "required": [ + "owner", + "repo", + "title" + ] } }, "parameters": {}, @@ -23601,65 +23704,7 @@ "content": { "application/json": { "schema": { - "type": "object", - "properties": { - "targets": { - "type": "array", - "items": { - "type": "object", - "properties": { - "owner": { - "type": "string", - "minLength": 1, - "maxLength": 39 - }, - "repo": { - "type": "string", - "minLength": 1, - "maxLength": 100 - } - }, - "required": [ - "owner", - "repo" - ] - }, - "maxItems": 25 - }, - "searchQuery": { - "type": "string", - "minLength": 1, - "maxLength": 500 - }, - "goalSpec": { - "type": "object", - "properties": { - "lane": { - "type": "string", - "minLength": 1 - }, - "minRankScore": { - "type": "number", - "minimum": 0, - "maximum": 100 - }, - "languages": { - "type": "array", - "items": { - "type": "string", - "minLength": 1, - "maxLength": 30 - }, - "maxItems": 20 - } - } - }, - "limit": { - "type": "integer", - "minimum": 1, - "maximum": 50 - } - } + "$ref": "#/components/schemas/FindOpportunitiesRequest" } } } @@ -23696,7 +23741,8 @@ "operationId": "postOpportunitiesFind", "tags": [ "Discovery" - ] + ], + "description": "Rank contribution opportunities across the given targets or search query. At least one of `targets` (non-empty) or `searchQuery` is required — JSON Schema cannot express that cross-field rule, so an empty body is rejected with reason `targets_or_search_query_required` (#10040)." } }, "/v1/issue-rag/retrieve": { @@ -23706,43 +23752,7 @@ "content": { "application/json": { "schema": { - "type": "object", - "properties": { - "owner": { - "type": "string", - "maxLength": 39 - }, - "repo": { - "type": "string", - "maxLength": 100 - }, - "title": { - "type": "string", - "maxLength": 300 - }, - "body": { - "type": "string", - "maxLength": 20000 - }, - "labels": { - "type": "array", - "items": { - "type": "string", - "maxLength": 100 - }, - "maxItems": 50 - }, - "topK": { - "type": "integer", - "minimum": 1, - "maximum": 12 - } - }, - "required": [ - "owner", - "repo", - "title" - ] + "$ref": "#/components/schemas/IssueRagRetrieveRequest" } } } diff --git a/packages/loopover-contract/src/tools/local-branch.ts b/packages/loopover-contract/src/tools/local-branch.ts index f13769c612..aa7e897b8e 100644 --- a/packages/loopover-contract/src/tools/local-branch.ts +++ b/packages/loopover-contract/src/tools/local-branch.ts @@ -527,9 +527,11 @@ export const findOpportunitiesTool = defineTool({ /** Same: the remote copy carried the bounds, the stdio copy carried none. */ export const RetrieveIssueContextInput = z.object({ - owner: z.string().max(39), - repo: z.string().max(100), - title: z.string().max(PREFLIGHT_LIMITS.titleChars), + // #10040: `.min(1)` matches `ownerRepoInput` and the hand-rolled `owner_and_repo_required` / + // `title_required` checks — the catalog previously published empty strings as valid. + owner: z.string().min(1).max(39), + repo: z.string().min(1).max(100), + title: z.string().min(1).max(PREFLIGHT_LIMITS.titleChars), body: z.string().max(PREFLIGHT_LIMITS.bodyChars).optional(), labels: z.array(z.string().max(PREFLIGHT_LIMITS.labelChars)).max(PREFLIGHT_LIMITS.labels).optional(), topK: z.number().int().min(1).max(12).optional(), diff --git a/src/api/routes.ts b/src/api/routes.ts index 33025838bd..f2d0050c1e 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -225,6 +225,7 @@ import { } from "../services/control-panel-roles"; import { runFindOpportunities, validateFindOpportunitiesInput, type FindOpportunitiesInput } from "../mcp/find-opportunities"; import { runIssueRagRetrieval, validateIssueRagInput, type IssueRagInput } from "../mcp/issue-rag"; +import { FindOpportunitiesRequestSchema, IssueRagRetrieveRequestSchema } from "../openapi/schemas"; import { buildBoundaryTestGenerationFinding, buildBoundaryTestGenerationSpec } from "../signals/boundary-test-generation"; import { buildTestEvidenceReport } from "../signals/test-evidence"; import { buildStructuralImprovementAssessment } from "../signals/improvement"; @@ -3592,7 +3593,13 @@ export function createApp() { /* v8 ignore next -- Protected middleware rejects unauthenticated private routes before route-specific guards. */ if (!identity) return c.json({ error: "unauthorized" }, 401); const body = await c.req.json().catch(() => null); - const parsed = validateFindOpportunitiesInput((body ?? {}) as FindOpportunitiesInput); + // #10040: same zod object the OpenAPI document publishes — refuse before the hand-rolled + // cross-field / normalisation pass, and keep the existing invalid_request body shape. + const schemaParsed = FindOpportunitiesRequestSchema.safeParse(body ?? {}); + if (!schemaParsed.success) { + return c.json({ status: "invalid_request", ranked: [], totalCandidates: 0, reason: "invalid_body" }, 400); + } + const parsed = validateFindOpportunitiesInput(schemaParsed.data as FindOpportunitiesInput); if (!parsed.ok) { return c.json({ status: "invalid_request", ranked: [], totalCandidates: 0, reason: parsed.reason }, 400); } @@ -3617,7 +3624,15 @@ export function createApp() { /* v8 ignore next -- Protected middleware rejects unauthenticated private routes before route-specific guards. */ if (!identity) return c.json({ error: "unauthorized" }, 401); const body = await c.req.json().catch(() => null); - const parsed = validateIssueRagInput((body ?? {}) as IssueRagInput); + // #10040: same zod object the OpenAPI document publishes — refuse before the hand-rolled pass. + const schemaParsed = IssueRagRetrieveRequestSchema.safeParse(body ?? {}); + if (!schemaParsed.success) { + return c.json( + { status: "invalid_request", repoFullName: "", reason: "invalid_body", telemetry: { attempted: false, injected: false, retrievedPaths: [] } }, + 400, + ); + } + const parsed = validateIssueRagInput(schemaParsed.data as IssueRagInput); if (!parsed.ok) { return c.json({ status: "invalid_request", repoFullName: "", reason: parsed.reason, telemetry: { attempted: false, injected: false, retrievedPaths: [] } }, 400); } diff --git a/src/mcp/find-opportunities.ts b/src/mcp/find-opportunities.ts index 7ecde8229e..9e49f92889 100644 --- a/src/mcp/find-opportunities.ts +++ b/src/mcp/find-opportunities.ts @@ -80,6 +80,9 @@ export function normalizeFindOpportunitiesLimit(limit: number | null | undefined export function validateFindOpportunitiesInput( input: FindOpportunitiesInput, ): { ok: true; value: FindOpportunitiesInput } | { ok: false; reason: string } { + // #10040: bound/type checks live on FindOpportunitiesInput (and the OpenAPI request schema derived + // from it). This helper keeps only what zod cannot express: the cross-field targets/searchQuery + // rule, target de-duplication, and trimming/normalisation. const targets = Array.isArray(input.targets) ? input.targets : undefined; const searchQuery = typeof input.searchQuery === "string" ? input.searchQuery.trim() : ""; const hasTargets = Boolean(targets && targets.length > 0); @@ -89,36 +92,18 @@ export function validateFindOpportunitiesInput( } let normalizedTargets: FindOpportunitiesTarget[] | undefined; if (hasTargets) { - if (targets!.length > MAX_FIND_OPPORTUNITIES_TARGETS) return { ok: false, reason: "too_many_targets" }; const seenTargets = new Set(); normalizedTargets = []; for (const target of targets!) { const owner = typeof target?.owner === "string" ? target.owner.trim() : ""; const repo = typeof target?.repo === "string" ? target.repo.trim() : ""; if (!owner || !repo) return { ok: false, reason: "invalid_target" }; - if (owner.length > MAX_FIND_OPPORTUNITIES_OWNER_LENGTH) return { ok: false, reason: "owner_too_long" }; - if (repo.length > MAX_FIND_OPPORTUNITIES_REPO_LENGTH) return { ok: false, reason: "repo_too_long" }; const key = `${owner.toLowerCase()}/${repo.toLowerCase()}`; if (seenTargets.has(key)) continue; seenTargets.add(key); normalizedTargets.push({ owner, repo }); } } - if (hasSearch && searchQuery.length > 500) return { ok: false, reason: "search_query_too_long" }; - const languages = input.goalSpec?.languages; - if (languages !== undefined) { - if (!Array.isArray(languages) || languages.length > MAX_FIND_OPPORTUNITIES_LANGUAGES) { - return { ok: false, reason: "invalid_languages" }; - } - for (const language of languages) { - const value = typeof language === "string" ? language.trim() : ""; - if (!value || value.length > MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH) return { ok: false, reason: "invalid_languages" }; - } - } - const minRankScore = input.goalSpec?.minRankScore; - if (minRankScore !== undefined && (!Number.isFinite(minRankScore) || minRankScore < 0 || minRankScore > 100)) { - return { ok: false, reason: "invalid_min_rank_score" }; - } return { ok: true, value: { diff --git a/src/mcp/issue-rag.ts b/src/mcp/issue-rag.ts index e14dee2577..03f97cd16d 100644 --- a/src/mcp/issue-rag.ts +++ b/src/mcp/issue-rag.ts @@ -26,32 +26,39 @@ export type IssueRagResult = { telemetry: IssueRagTelemetry; }; -function cleanLabels(labels: string[] | undefined): string[] | undefined { - if (!labels) return undefined; - const cleaned = labels.map((label) => label.trim()).filter(Boolean).slice(0, PREFLIGHT_LIMITS.labels); - return cleaned.length > 0 ? cleaned : undefined; +function cleanLabels(labels: string[] | undefined): { ok: true; value: string[] | undefined } | { ok: false; reason: string } { + if (!labels) return { ok: true, value: undefined }; + // #10040: do not silently truncate — over-long arrays are rejected by RetrieveIssueContextInput + // before this runs; here we only trim/filter empties and enforce per-label length after trim. + const cleaned: string[] = []; + for (const label of labels) { + const value = typeof label === "string" ? label.trim() : ""; + if (!value) continue; + if (value.length > PREFLIGHT_LIMITS.labelChars) return { ok: false, reason: "invalid_labels" }; + cleaned.push(value); + } + return { ok: true, value: cleaned.length > 0 ? cleaned : undefined }; } export function validateIssueRagInput( input: IssueRagInput, ): { ok: true; value: IssueRagInput & { repoFullName: string } } | { ok: false; reason: string } { + // #10040: bound/type checks live on RetrieveIssueContextInput. This helper keeps trimming and the + // post-trim emptiness checks zod cannot see (whitespace-only owner/repo/title), and refuses + // over-long body instead of slicing it. const owner = typeof input.owner === "string" ? input.owner.trim() : ""; const repo = typeof input.repo === "string" ? input.repo.trim() : ""; const title = typeof input.title === "string" ? input.title.trim() : ""; if (!owner || !repo) return { ok: false, reason: "owner_and_repo_required" }; if (!title) return { ok: false, reason: "title_required" }; - if (owner.length > MAX_ISSUE_RAG_OWNER_LENGTH) return { ok: false, reason: "owner_too_long" }; - if (repo.length > MAX_ISSUE_RAG_REPO_LENGTH) return { ok: false, reason: "repo_too_long" }; - if (title.length > PREFLIGHT_LIMITS.titleChars) return { ok: false, reason: "title_too_long" }; - const body = typeof input.body === "string" ? input.body.slice(0, PREFLIGHT_LIMITS.bodyChars) : undefined; - const labels = cleanLabels(input.labels); - if (labels) { - for (const label of labels) { - if (label.length > PREFLIGHT_LIMITS.labelChars) return { ok: false, reason: "invalid_labels" }; - } + if (typeof input.body === "string" && input.body.length > PREFLIGHT_LIMITS.bodyChars) { + return { ok: false, reason: "body_too_long" }; } + const body = typeof input.body === "string" ? input.body : undefined; + const labelsResult = cleanLabels(input.labels); + if (!labelsResult.ok) return labelsResult; const topK = input.topK; - if (topK !== undefined && (!Number.isFinite(topK) || topK < 1 || topK > 12)) { + if (topK !== undefined && (!Number.isFinite(topK) || topK < 1 || topK > 12 || !Number.isInteger(topK))) { return { ok: false, reason: "invalid_top_k" }; } return { @@ -61,7 +68,7 @@ export function validateIssueRagInput( repo, title, ...(body !== undefined ? { body } : {}), - ...(labels ? { labels } : {}), + ...(labelsResult.value ? { labels: labelsResult.value } : {}), ...(topK !== undefined ? { topK: normalizeIssueRagTopK(topK) } : {}), repoFullName: `${owner}/${repo}`, }, diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 2f2fce0307..016e8d33cf 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -6,15 +6,7 @@ import { checkBeforeStartSchema, slopRiskSchema, validateFocusManifestSchema, va import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions"; import { MAX_PRIORITY_ELIGIBILITY_WINDOW_MINUTES } from "../review/priority-eligibility-window"; import { MAX_CONTRIBUTOR_OPEN_ITEM_CAP } from "../types"; -import { - MAX_FIND_OPPORTUNITIES_TARGETS, - MAX_FIND_OPPORTUNITIES_OWNER_LENGTH, - MAX_FIND_OPPORTUNITIES_REPO_LENGTH, - MAX_FIND_OPPORTUNITIES_LANGUAGES, - MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH, -} from "../mcp/find-opportunities"; -import { MAX_ISSUE_RAG_OWNER_LENGTH, MAX_ISSUE_RAG_REPO_LENGTH } from "../mcp/issue-rag"; -import { PREFLIGHT_LIMITS } from "../signals/preflight-limits"; +import { FindOpportunitiesInput, RetrieveIssueContextInput } from "@loopover/contract/tools"; import { extendZodWithOpenApi } from "@asteasolutions/zod-to-openapi"; import { PublicStatsSchema as PublicStatsShape } from "@loopover/contract/public-api"; @@ -2027,31 +2019,13 @@ export const ScoreBreakdownResponseSchema = z }) .openapi("ScoreBreakdownResponse"); -// #9310 — request/response schemas for the two discovery routes below, mirroring the MCP tools' -// own Zod shapes verbatim (src/mcp/server.ts's findOpportunitiesShape/issueRagShape, and -// FindOpportunitiesOutput/RetrieveIssueContextOutput in @loopover/contract) so the OpenAPI contract -// can't silently drift from what the -// MCP tools actually validate. -export const FindOpportunitiesRequestSchema = z.object({ - targets: z - .array( - z.object({ - owner: z.string().min(1).max(MAX_FIND_OPPORTUNITIES_OWNER_LENGTH), - repo: z.string().min(1).max(MAX_FIND_OPPORTUNITIES_REPO_LENGTH), - }), - ) - .max(MAX_FIND_OPPORTUNITIES_TARGETS) - .optional(), - searchQuery: z.string().min(1).max(500).optional(), - goalSpec: z - .object({ - lane: z.string().min(1).optional(), - minRankScore: z.number().min(0).max(100).optional(), - languages: z.array(z.string().min(1).max(MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH)).max(MAX_FIND_OPPORTUNITIES_LANGUAGES).optional(), - }) - .optional(), - limit: z.number().int().min(1).max(50).optional(), -}); +/** + * Request body for POST /v1/opportunities/find. Field-level parity with `FindOpportunitiesInput` in + * @loopover/contract (the `loopover_find_opportunities` MCP tool `inputSchema`) — #10040. Re-wrapped + * through the local `z.object` so `.openapi(...)` lands on the OpenAPI-extended zod, same pattern as + * `CheckBeforeStartRequestSchema`. + */ +export const FindOpportunitiesRequestSchema = z.object(FindOpportunitiesInput.shape).openapi("FindOpportunitiesRequest"); export const FindOpportunitiesResponseSchema = z .object({ @@ -2089,14 +2063,11 @@ export const FindOpportunitiesResponseSchema = z }) .openapi("FindOpportunitiesResponse"); -export const IssueRagRetrieveRequestSchema = z.object({ - owner: z.string().max(MAX_ISSUE_RAG_OWNER_LENGTH), - repo: z.string().max(MAX_ISSUE_RAG_REPO_LENGTH), - title: z.string().max(PREFLIGHT_LIMITS.titleChars), - body: z.string().max(PREFLIGHT_LIMITS.bodyChars).optional(), - labels: z.array(z.string().max(PREFLIGHT_LIMITS.labelChars)).max(PREFLIGHT_LIMITS.labels).optional(), - topK: z.number().int().min(1).max(12).optional(), -}); +/** + * Request body for POST /v1/issue-rag/retrieve. Field-level parity with `RetrieveIssueContextInput` in + * @loopover/contract (the `loopover_retrieve_issue_context` MCP tool `inputSchema`) — #10040. + */ +export const IssueRagRetrieveRequestSchema = z.object(RetrieveIssueContextInput.shape).openapi("IssueRagRetrieveRequest"); export const IssueRagRetrieveResponseSchema = z .object({ diff --git a/src/openapi/spec.ts b/src/openapi/spec.ts index c0e67e7eff..534c426e9d 100644 --- a/src/openapi/spec.ts +++ b/src/openapi/spec.ts @@ -1786,6 +1786,8 @@ export function buildOpenApiSpec() { operationId: "postOpportunitiesFind", tags: ["Discovery"], summary: "Find cross-repo contribution opportunities (#9310)", + description: + "Rank contribution opportunities across the given targets or search query. At least one of `targets` (non-empty) or `searchQuery` is required — JSON Schema cannot express that cross-field rule, so an empty body is rejected with reason `targets_or_search_query_required` (#10040).", request: { body: { content: { "application/json": { schema: FindOpportunitiesRequestSchema } }, diff --git a/test/integration/api.test.ts b/test/integration/api.test.ts index 3ce8c35307..0e38ea76ec 100644 --- a/test/integration/api.test.ts +++ b/test/integration/api.test.ts @@ -1451,7 +1451,8 @@ describe("api routes", () => { expect(invalidIssueRag.status).toBe(400); await expect(invalidIssueRag.json()).resolves.toMatchObject({ status: "invalid_request", - reason: "title_required", + // #10040: empty title fails RetrieveIssueContextInput.min(1) at the published schema. + reason: "invalid_body", }); const { token: minerSessionToken } = await createSessionForGitHubUser(env, { login: "ordinary-mcp-user", id: 4243 }); @@ -6671,6 +6672,94 @@ describe("api routes", () => { const unauth = await app.request("/v1/repos/entrius/allways-ui/live-gate-thresholds", {}, env); expect(unauth.status).toBe(401); }); + + it("REGRESSION: discovery routes validate against the schemas their OpenAPI publishes (#10040)", async () => { + const app = createApp(); + const env = createTestEnv(); + const findTargets = [{ owner: "acme", repo: "widgets" }]; + const { PREFLIGHT_LIMITS: limits } = await import("@loopover/contract"); + + for (const [label, body] of [ + ["non-integer limit", { targets: findTargets, limit: 7.9 }], + ["over-max limit", { targets: findTargets, limit: 999 }], + ["empty searchQuery", { targets: findTargets, searchQuery: "" }], + ] as const) { + const response = await app.request( + "/v1/opportunities/find", + { method: "POST", headers: apiHeaders(env), body: JSON.stringify(body) }, + env, + ); + expect(response.status, label).toBe(400); + await expect(response.json(), label).resolves.toMatchObject({ + status: "invalid_request", + ranked: [], + totalCandidates: 0, + reason: "invalid_body", + }); + } + + const emptyFind = await app.request( + "/v1/opportunities/find", + { method: "POST", headers: apiHeaders(env), body: JSON.stringify({}) }, + env, + ); + expect(emptyFind.status).toBe(400); + await expect(emptyFind.json()).resolves.toMatchObject({ + status: "invalid_request", + reason: "targets_or_search_query_required", + }); + + for (const [label, body] of [ + ["non-integer topK", { owner: "acme", repo: "widgets", title: "Add observability context for self-hosted review planning failures", topK: 3.5 }], + [ + "over-long labels", + { + owner: "acme", + repo: "widgets", + title: "Add observability context for self-hosted review planning failures", + labels: Array.from({ length: limits.labels + 1 }, (_, i) => `label${i}`), + }, + ], + [ + "over-long body", + { + owner: "acme", + repo: "widgets", + title: "Add observability context for self-hosted review planning failures", + body: "x".repeat(limits.bodyChars + 1), + }, + ], + ["empty owner", { owner: "", repo: "widgets", title: "Add observability context for self-hosted review planning failures" }], + ] as const) { + const response = await app.request( + "/v1/issue-rag/retrieve", + { method: "POST", headers: apiHeaders(env), body: JSON.stringify(body) }, + env, + ); + expect(response.status, label).toBe(400); + await expect(response.json(), label).resolves.toMatchObject({ + status: "invalid_request", + repoFullName: "", + reason: "invalid_body", + telemetry: { attempted: false, injected: false, retrievedPaths: [] }, + }); + } + + const whitespaceTitle = await app.request( + "/v1/issue-rag/retrieve", + { + method: "POST", + headers: apiHeaders(env), + body: JSON.stringify({ owner: "acme", repo: "widgets", title: " " }), + }, + env, + ); + expect(whitespaceTitle.status).toBe(400); + await expect(whitespaceTitle.json()).resolves.toMatchObject({ + status: "invalid_request", + reason: "title_required", + }); + }); }); async function signWebhook(body: string, secret: string | undefined): Promise { diff --git a/test/unit/contract-registry.test.ts b/test/unit/contract-registry.test.ts index 9ee016d2d5..06e9907e4d 100644 --- a/test/unit/contract-registry.test.ts +++ b/test/unit/contract-registry.test.ts @@ -87,6 +87,16 @@ describe("contract tool registry", () => { expect(getToolContract(first.name)).toBe(first); expect(getToolContract("loopover_not_a_real_tool")).toBeUndefined(); }); + + it("rejects empty owner/repo/title on RetrieveIssueContextInput and stays a ZodObject (#10040)", () => { + const contract = getToolContract("loopover_retrieve_issue_context"); + expect(contract).toBeDefined(); + expect(contract!.input).toBeInstanceOf(z.ZodObject); + expect(contract!.input.safeParse({ owner: "", repo: "x", title: "t" }).success).toBe(false); + expect(contract!.input.safeParse({ owner: "o", repo: "", title: "t" }).success).toBe(false); + expect(contract!.input.safeParse({ owner: "o", repo: "r", title: "" }).success).toBe(false); + expect(contract!.input.safeParse({ owner: "o", repo: "r", title: "t" }).success).toBe(true); + }); }); describe("contract projection", () => { diff --git a/test/unit/find-opportunities.test.ts b/test/unit/find-opportunities.test.ts index 0e652306ea..5c6703d60d 100644 --- a/test/unit/find-opportunities.test.ts +++ b/test/unit/find-opportunities.test.ts @@ -4,9 +4,6 @@ import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it, vi } from "vitest"; import { MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH, - MAX_FIND_OPPORTUNITIES_LANGUAGES, - MAX_FIND_OPPORTUNITIES_OWNER_LENGTH, - MAX_FIND_OPPORTUNITIES_REPO_LENGTH, MAX_FIND_OPPORTUNITIES_TARGETS, normalizeFindOpportunitiesLimit, publicRankScore, @@ -74,8 +71,9 @@ describe("validateFindOpportunitiesInput", () => { expect(validateFindOpportunitiesInput({})).toEqual({ ok: false, reason: "targets_or_search_query_required" }); }); - it("rejects invalid targets and oversized search queries", () => { + it("rejects empty or non-string targets after trimming (#10040 keeps only cross-field / trim checks)", () => { expect(validateFindOpportunitiesInput({ targets: [{ owner: "", repo: "demo" }] })).toEqual({ ok: false, reason: "invalid_target" }); + expect(validateFindOpportunitiesInput({ targets: [{ owner: " ", repo: "demo" }] })).toEqual({ ok: false, reason: "invalid_target" }); expect(validateFindOpportunitiesInput({ targets: [{ owner: 123 as unknown as string, repo: "demo" }] })).toEqual({ ok: false, reason: "invalid_target", @@ -84,25 +82,6 @@ describe("validateFindOpportunitiesInput", () => { ok: false, reason: "invalid_target", }); - expect( - validateFindOpportunitiesInput({ - targets: Array.from({ length: MAX_FIND_OPPORTUNITIES_TARGETS + 1 }, () => ({ owner: "acme", repo: "demo" })), - }), - ).toEqual({ ok: false, reason: "too_many_targets" }); - expect( - validateFindOpportunitiesInput({ targets: [{ owner: "x".repeat(MAX_FIND_OPPORTUNITIES_OWNER_LENGTH + 1), repo: "demo" }] }), - ).toEqual({ ok: false, reason: "owner_too_long" }); - expect( - validateFindOpportunitiesInput({ targets: [{ owner: "acme", repo: "x".repeat(MAX_FIND_OPPORTUNITIES_REPO_LENGTH + 1) }] }), - ).toEqual({ ok: false, reason: "repo_too_long" }); - expect(validateFindOpportunitiesInput({ searchQuery: "x".repeat(501) })).toEqual({ ok: false, reason: "search_query_too_long" }); - expect( - validateFindOpportunitiesInput({ searchQuery: "docs", goalSpec: { minRankScore: 101 } }), - ).toEqual({ ok: false, reason: "invalid_min_rank_score" }); - expect(validateFindOpportunitiesInput({ searchQuery: "docs", goalSpec: { languages: [""] } })).toEqual({ - ok: false, - reason: "invalid_languages", - }); }); it("accepts trimmed targets and search queries", () => { @@ -144,37 +123,7 @@ describe("validateFindOpportunitiesInput", () => { if (parsed.ok) expect(parsed.value.targets).toHaveLength(MAX_FIND_OPPORTUNITIES_TARGETS); }); - it("rejects a non-array goalSpec.languages", () => { - expect( - validateFindOpportunitiesInput({ searchQuery: "docs", goalSpec: { languages: "typescript" as unknown as string[] } }), - ).toEqual({ ok: false, reason: "invalid_languages" }); - }); - - it("rejects a non-string language entry", () => { - expect( - validateFindOpportunitiesInput({ searchQuery: "docs", goalSpec: { languages: [123 as unknown as string] } }), - ).toEqual({ ok: false, reason: "invalid_languages" }); - }); - - it("rejects more than MAX_FIND_OPPORTUNITIES_LANGUAGES languages", () => { - expect( - validateFindOpportunitiesInput({ - searchQuery: "docs", - goalSpec: { languages: Array.from({ length: MAX_FIND_OPPORTUNITIES_LANGUAGES + 1 }, (_, i) => `lang${i}`) }, - }), - ).toEqual({ ok: false, reason: "invalid_languages" }); - }); - - it("rejects a language entry longer than MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH", () => { - expect( - validateFindOpportunitiesInput({ - searchQuery: "docs", - goalSpec: { languages: ["x".repeat(MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH + 1)] }, - }), - ).toEqual({ ok: false, reason: "invalid_languages" }); - }); - - it("accepts a valid languages list at or under the boundary", () => { + it("passes a valid languages list through unchanged (bound checks live on the contract schema)", () => { const parsed = validateFindOpportunitiesInput({ searchQuery: "docs", goalSpec: { languages: ["typescript", "x".repeat(MAX_FIND_OPPORTUNITIES_LANGUAGE_LENGTH)] }, diff --git a/test/unit/issue-rag-mcp.test.ts b/test/unit/issue-rag-mcp.test.ts index 9882bbab69..e56ce3e609 100644 --- a/test/unit/issue-rag-mcp.test.ts +++ b/test/unit/issue-rag-mcp.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { runIssueRagRetrieval, validateIssueRagInput } from "../../src/mcp/issue-rag"; import { emptyIssueRagTelemetry } from "../../src/review/issue-rag-retrieval"; +import { PREFLIGHT_LIMITS } from "@loopover/contract"; import { createTestEnv } from "../helpers/d1"; describe("runIssueRagRetrieval (#4293)", () => { @@ -21,19 +22,25 @@ describe("runIssueRagRetrieval (#4293)", () => { }); }); - it("rejects oversized repos and invalid labels", () => { - expect(validateIssueRagInput({ owner: "acme", repo: "r".repeat(101), title: "Add observability context for self-hosted review planning failures" })).toMatchObject({ - ok: false, - reason: "repo_too_long", - }); + it("rejects invalid labels and over-long bodies (length bounds for owner/repo/title live on the schema)", () => { + // #10040: repo/title max-length are on RetrieveIssueContextInput; this helper only rejects after trim / + // per-label length / body length / topK shape. expect( validateIssueRagInput({ owner: "acme", repo: "demo", title: "Add observability context for self-hosted review planning failures", - labels: ["x".repeat(101)], + labels: ["x".repeat(PREFLIGHT_LIMITS.labelChars + 1)], }), ).toMatchObject({ ok: false, reason: "invalid_labels" }); + expect( + validateIssueRagInput({ + owner: "acme", + repo: "demo", + title: "Add observability context for self-hosted review planning failures", + body: "x".repeat(PREFLIGHT_LIMITS.bodyChars + 1), + }), + ).toMatchObject({ ok: false, reason: "body_too_long" }); }); it("covers validation branches for owner/title/body/labels/topK normalization", () => { @@ -41,13 +48,14 @@ describe("runIssueRagRetrieval (#4293)", () => { ok: false, reason: "owner_and_repo_required", }); + // #10040: over-long title is a schema concern; whitespace-only title is the helper's title_required. expect( validateIssueRagInput({ owner: "acme", repo: "demo", - title: "x".repeat(301), + title: " ", }), - ).toMatchObject({ ok: false, reason: "title_too_long" }); + ).toMatchObject({ ok: false, reason: "title_required" }); expect( validateIssueRagInput({ owner: "acme", diff --git a/test/unit/mcp-find-opportunities.test.ts b/test/unit/mcp-find-opportunities.test.ts index fe898bc3ed..b09ed5eb60 100644 --- a/test/unit/mcp-find-opportunities.test.ts +++ b/test/unit/mcp-find-opportunities.test.ts @@ -161,4 +161,27 @@ describe("MCP loopover_find_opportunities", () => { expect(targets.isError).toBe(true); expect(JSON.stringify(targets.content)).toMatch(/session cannot access this repository/i); }); + + it("agrees with POST /v1/opportunities/find that a non-integer limit is refused (#10040)", async () => { + const env = createTestEnv(); + const body = { targets: [{ owner: "acme", repo: "allowed" }], limit: 7.9 }; + + const client = await connect(env); + const mcp = await client.callTool({ name: "loopover_find_opportunities", arguments: body }); + expect(mcp.isError).toBe(true); + + const { createApp } = await import("../../src/api/routes"); + const app = createApp(); + const rest = await app.request( + "/v1/opportunities/find", + { + method: "POST", + headers: { authorization: `Bearer ${env.LOOPOVER_API_TOKEN}`, "content-type": "application/json" }, + body: JSON.stringify(body), + }, + env, + ); + expect(rest.status).toBe(400); + await expect(rest.json()).resolves.toMatchObject({ status: "invalid_request", reason: "invalid_body" }); + }); }); diff --git a/test/unit/mcp-issue-rag.test.ts b/test/unit/mcp-issue-rag.test.ts index bc28ca0d87..b5fb82816f 100644 --- a/test/unit/mcp-issue-rag.test.ts +++ b/test/unit/mcp-issue-rag.test.ts @@ -5,6 +5,7 @@ import { createSessionForGitHubUser, type AuthIdentity } from "../../src/auth/se import { upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { LoopoverMcp } from "../../src/mcp/server"; import { validateIssueRagInput } from "../../src/mcp/issue-rag"; +import { PREFLIGHT_LIMITS } from "@loopover/contract"; import { createTestEnv } from "../helpers/d1"; async function connect(env: Env, identity?: AuthIdentity) { @@ -20,32 +21,67 @@ afterEach(() => { vi.unstubAllGlobals(); }); -describe("validateIssueRagInput (#4293)", () => { - it("rejects missing owner/repo/title and oversized fields", () => { +describe("validateIssueRagInput (#4293, #10040)", () => { + it("rejects missing owner/repo/title after trim and non-integer topK", () => { expect(validateIssueRagInput({ owner: "", repo: "demo", title: "Add observability context for self-hosted review planning failures" }).ok).toBe(false); expect(validateIssueRagInput({ owner: "acme", repo: "", title: "Add observability context for self-hosted review planning failures" }).ok).toBe(false); expect(validateIssueRagInput({ owner: "acme", repo: "demo", title: "" }).ok).toBe(false); - expect(validateIssueRagInput({ owner: "a".repeat(40), repo: "demo", title: "Add observability context for self-hosted review planning failures" })).toMatchObject({ ok: false, reason: "owner_too_long" }); - expect(validateIssueRagInput({ owner: "acme", repo: "demo", title: "Add observability context for self-hosted review planning failures", topK: 0 })).toMatchObject({ ok: false, reason: "invalid_top_k" }); + expect(validateIssueRagInput({ owner: "acme", repo: "demo", title: "Add observability context for self-hosted review planning failures", topK: 0 })).toMatchObject({ + ok: false, + reason: "invalid_top_k", + }); + expect(validateIssueRagInput({ owner: "acme", repo: "demo", title: "Add observability context for self-hosted review planning failures", topK: 3.5 })).toMatchObject({ + ok: false, + reason: "invalid_top_k", + }); + }); + + it("rejects an over-long body instead of silently truncating it", () => { + expect( + validateIssueRagInput({ + owner: "acme", + repo: "demo", + title: "Add observability context for self-hosted review planning failures", + body: "x".repeat(PREFLIGHT_LIMITS.bodyChars + 1), + }), + ).toMatchObject({ ok: false, reason: "body_too_long" }); + }); + + it("rejects an over-long label after trim instead of silently dropping it via slice", () => { + expect( + validateIssueRagInput({ + owner: "acme", + repo: "demo", + title: "Add observability context for self-hosted review planning failures", + labels: ["x".repeat(PREFLIGHT_LIMITS.labelChars + 1)], + }), + ).toMatchObject({ ok: false, reason: "invalid_labels" }); + }); + + it("rejects whitespace-only title after trim when the schema already accepted minLength", () => { + expect( + validateIssueRagInput({ + owner: "acme", + repo: "demo", + title: " ", + }), + ).toMatchObject({ ok: false, reason: "title_required" }); }); }); describe("MCP loopover_retrieve_issue_context", () => { - it("registers the tool and rejects invalid requests before authorization", async () => { + it("registers the tool and rejects an empty title at the contract schema", async () => { const env = createTestEnv(); const client = await connect(env); const { tools } = await client.listTools(); expect(tools.map((tool) => tool.name)).toContain("loopover_retrieve_issue_context"); + // #10040: empty title fails RetrieveIssueContextInput.min(1) before the handler runs. const invalid = await client.callTool({ name: "loopover_retrieve_issue_context", arguments: { owner: "acme", repo: "widgets", title: "" }, }); - expect(invalid.isError).toBeFalsy(); - expect(invalid.structuredContent).toMatchObject({ - status: "invalid_request", - reason: "title_required", - }); + expect(invalid.isError).toBe(true); }); it("returns query_too_short for a one-line issue below the retrieval floor", async () => { diff --git a/test/unit/routes-issue-rag.test.ts b/test/unit/routes-issue-rag.test.ts index 8cdb76356d..cdb857ecae 100644 --- a/test/unit/routes-issue-rag.test.ts +++ b/test/unit/routes-issue-rag.test.ts @@ -61,6 +61,7 @@ describe("issue-rag retrieve route (#4293)", () => { const app = createApp(); const env = createTestEnv(); + // #10040: empty title fails IssueRagRetrieveRequestSchema.min(1) → invalid_body before the hand-rolled pass. const invalid = await app.request( ISSUE_RAG_PATH, { @@ -71,7 +72,20 @@ describe("issue-rag retrieve route (#4293)", () => { env, ); expect(invalid.status).toBe(400); - await expect(invalid.json()).resolves.toMatchObject({ status: "invalid_request", reason: "title_required" }); + await expect(invalid.json()).resolves.toMatchObject({ status: "invalid_request", reason: "invalid_body" }); + + // Whitespace-only title clears the schema min(1) then fails validateIssueRagInput's post-trim check. + const whitespaceTitle = await app.request( + ISSUE_RAG_PATH, + { + method: "POST", + headers: { authorization: `Bearer ${env.LOOPOVER_API_TOKEN}`, "content-type": "application/json" }, + body: JSON.stringify({ owner: "repo-owner", repo: "owned-repo", title: " " }), + }, + env, + ); + expect(whitespaceTitle.status).toBe(400); + await expect(whitespaceTitle.json()).resolves.toMatchObject({ status: "invalid_request", reason: "title_required" }); const malformed = await app.request( ISSUE_RAG_PATH, @@ -83,7 +97,7 @@ describe("issue-rag retrieve route (#4293)", () => { env, ); expect(malformed.status).toBe(400); - await expect(malformed.json()).resolves.toMatchObject({ status: "invalid_request", reason: "owner_and_repo_required" }); + await expect(malformed.json()).resolves.toMatchObject({ status: "invalid_request", reason: "invalid_body" }); }); it("allows sessions through the path allowlist and scopes repo access", async () => {