Public: anyone can read this; the crew can change it.
GET /api/v1/search names an unknown kind or a bad limit in a note, as /api/v1/explore names an unknown filter #8513
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/8492-get-api-v1-search-names-an-unknown-kind → main 2 commits fork 3 threads · 3 unresolved
7 files
changed +78
−26
src/actions/search.ts +22
| @@ -19,6 +19,7 @@ import { listOrigins } from "../core/origins.ts"; | ||
| 19 | 19 | import { canRead, enterableApps, listReposVisibleTo } from "../core/repos.ts"; |
| 20 | 20 | import { |
| 21 | 21 | cleanQuery, |
| 22 | searchKind, | |
| 22 | 23 | grepRepos, |
| 23 | 24 | searchLocalIssues, |
| 24 | 25 | matchOneRepo, |
| @@ -41,6 +42,27 @@ export const SEARCH_LIMIT = 20; | ||
| 41 | 42 | /** Hard ceiling on a caller-chosen limit. */ |
| 42 | 43 | export const SEARCH_LIMIT_MAX = 100; |
| 43 | 44 | |
| 45 | /** | |
| 46 | * The refusal an API or MCP caller gets for a `kind` or `limit` the search | |
| 47 | * does not accept, in one set of words; undefined when both are fine. The | |
| 48 | * page keeps its softer `searchNote`. | |
| 49 | */ | |
| 50 | export function searchRefusal( | |
| 51 | rawKind: string | null | undefined, | |
| 52 | rawLimit: string | number | null | undefined, | |
| 53 | ): { code: "invalid_kind" | "invalid_limit"; message: string } | undefined { | |
| 54 | if (rawKind !== null && rawKind !== undefined && rawKind !== "all" && searchKind(rawKind) === "all") { | |
| 55 | const asked = rawKind.length > 40 ? `${rawKind.slice(0, 39)}…` : rawKind; | |
| 56 | return { code: "invalid_kind", message: `kind ${JSON.stringify(asked)} is not a kind — use all, repos, issues or code` }; | |
| 57 | } | |
| 58 | if (rawLimit !== null && rawLimit !== undefined) { | |
| 59 | const n = typeof rawLimit === "number" ? rawLimit : /^\d{1,4}$/.test(rawLimit) ? Number(rawLimit) : 0; | |
| 60 | if (!Number.isInteger(n) || n < 1 || n > SEARCH_LIMIT_MAX) | |
| 61 | return { code: "invalid_limit", message: `limit is a whole number from 1 to ${SEARCH_LIMIT_MAX}` }; | |
| 62 | } | |
| 63 | return undefined; | |
| 64 | } | |
| 65 | ||
| 44 | 66 | /** |
| 45 | 67 | * The whole live half gets this long, all origins together. A search box |
| 46 | 68 | * that blocks for a slow forge is worse than one that quietly returns what |
src/contract/openapi/routes/getApiV1Search.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import type { Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/search", summary: "Search issues, pulls, repositories and code; `kind` narrows it", auth: false, query: { q: "string", kind: "string" }, reply: { repos: "array", issues: "array", code: "array", truncated: "object", moreCode: "boolean", codeSkipped: "boolean", note: "string, only when q is blank or kind is unknown: what to send" } }; | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/search", summary: "Search issues, pulls, repositories and code; `kind` narrows it", auth: false, query: { q: "string", kind: "string", limit: "integer" }, refusals: { invalid_kind: "kind is not all, repos, issues or code (422)", invalid_limit: "limit is not a whole number from 1 to 100 (422)" }, reply: { repos: "array", issues: "array", code: "array", truncated: "object", moreCode: "boolean", codeSkipped: "boolean", note: "string, only when q is blank: what to send" } }; | |
src/core/search.ts +3 −7
| @@ -61,15 +61,11 @@ export function searchKind(raw: string | null): SearchKind { | ||
| 61 | 61 | |
| 62 | 62 | /** |
| 63 | 63 | * What a caller who asked wrongly is told, in one sentence, or undefined when |
outdated
Duplicates SEARCH_LIMIT_MAX in actions/search.ts. Import that constant instead of adding a second 100. |
||
| 64 | * the request was fine. Plain text: the caller escapes it where it draws it. | |
| 64 | * the request was fine (an unknown kind or limit is `searchRefusal`'s 422, not a note). Plain text: the caller escapes it where it draws it. | |
| 65 | 65 | */ |
| 66 | export function searchNote(rawQ: string | null, rawKind: string | null): string | undefined { | |
| 67 | const kinds = "kind=all|repos|issues|code"; | |
| 68 | const asked = rawKind === null ? undefined : rawKind.length > 40 ? `${rawKind.slice(0, 39)}…` : rawKind; | |
| 69 | if (rawKind !== null && rawKind !== "all" && searchKind(rawKind) === "all") | |
| 70 | return `kind ${JSON.stringify(asked)} is not a kind — use ${kinds}`; | |
| 66 | export function searchNote(rawQ: string | null): string | undefined { | |
| 71 | 67 | if (cleanQuery(rawQ ?? "") === "") |
| 72 | return `q is empty — send what to find, e.g. q=parser&kind=code (${kinds})`; | |
| 68 | return "q is empty — send what to find, e.g. q=parser&kind=code (kind=all|repos|issues|code)"; | |
| 73 | 69 | return undefined; |
| 74 | 70 | } |
| 75 | 71 |
src/http/api.ts +4 −2
| @@ -97,7 +97,7 @@ import { holdPull, releasePull } from "../actions/holds.ts"; | ||
| 97 | 97 | import { getHold, type Hold } from "../core/holds.ts"; |
| 98 | 98 | import { watchMilestone } from "../actions/watches.ts"; |
| 99 | 99 | import { THOUGHT_MAX, keepThought } from "../actions/thought.ts"; |
| 100 | import { search, searchWithin, type SearchPhase } from "../actions/search.ts"; | |
| 100 | import { search, searchRefusal, searchWithin, type SearchPhase } from "../actions/search.ts"; | |
| 101 | 101 | import { runRepos } from "./run_gate.ts"; |
| 102 | 102 | import { |
| 103 | 103 | commitToBranch, |
| @@ -1383,6 +1383,8 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1383 | 1383 | router.add("GET", "/api/v1/search", async (ctx) => { |
| 1384 | 1384 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 1385 | 1385 | const q = ctx.url.searchParams; |
| 1386 | const refusal = searchRefusal(q.get("kind"), q.get("limit")); | |
| 1387 | if (refusal) return apiError(refusal.code, 422, refusal.message); | |
| 1386 | 1388 | const kind = searchKind(q.get("kind")); |
searchNote(q.get("q"), q.get("kind")) later in this handler can no longer see an unknown kind, because it is refused here. Drop the kind argument or branch where it is unreachable. |
||
| 1387 | 1389 | const query = q.get("q") ?? ""; |
| 1388 | 1390 | const greps = |
| @@ -1411,7 +1413,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1411 | 1413 | const found = await (mine === undefined |
| 1412 | 1414 | ? search(ctx.app, ctx.auth.user, timed) |
| 1413 | 1415 | : searchWithin(ctx.app, ctx.auth.user, mine, timed)); |
| 1414 | const note = searchNote(q.get("q"), q.get("kind")); | |
| 1416 | const note = searchNote(q.get("q")); | |
| 1415 | 1417 | return timePhase(ctx.request, "render", () => json(note === undefined ? found : { ...found, note })); |
| 1416 | 1418 | } finally { |
| 1417 | 1419 | held?.release(); |
src/http/mcp/tools/search.ts +11 −8
| @@ -1,18 +1,19 @@ | ||
| 1 | 1 | import type { Repo } from "../../../core/repos.ts"; |
| 2 | import { search } from "../../../actions/search.ts"; | |
| 3 | import { CODE_BUDGET_MS, searchNote } from "../../../core/search.ts"; | |
| 2 | import { search, searchRefusal } from "../../../actions/search.ts"; | |
| 3 | import { CODE_BUDGET_MS, searchKind, searchNote } from "../../../core/search.ts"; | |
| 4 | 4 | import { good, bad, type Tool, repoArg, isAnswer } from "../shared.ts"; |
| 5 | 5 | |
| 6 | 6 | export const tool: Tool = { |
| 7 | 7 | name: "search", |
| 8 | doc: "`{ q, kind?, repo?, all? }`: issues, pulls, repositories and code you can read, the same results as /search; `repo` keeps to one; `moreCode` says the code grep stopped at its budget, `all` greps the rest", | |
| 8 | doc: "`{ q, kind?, limit?, repo?, all? }`: issues, pulls, repositories and code you can read, the same results as /search; `repo` keeps to one; `moreCode` says the code grep stopped at its budget, `all` greps the rest", | |
| 9 | 9 | description: |
| 10 | 10 | "Find issues, pulls, repositories and code you can read; `kind` and `repo` narrow.", |
| 11 | 11 | inputSchema: { |
| 12 | 12 | type: "object", |
| 13 | 13 | properties: { |
| 14 | 14 | q: { type: "string", maxLength: 200 }, |
| 15 | kind: { type: "string", enum: ["issues", "repos", "code"] }, | |
| 15 | kind: { type: "string", enum: ["all", "issues", "repos", "code"] }, | |
| 16 | limit: { type: "integer" }, | |
| 16 | 17 | repo: { type: "string", maxLength: 140 }, |
| 17 | 18 | all: { type: "boolean" }, |
| 18 | 19 | }, |
| @@ -23,8 +24,9 @@ export const tool: Tool = { | ||
| 23 | 24 | const q = args["q"]; |
| 24 | 25 | if (typeof q !== "string" || q.length > 200) return bad("invalid", "`q` is a string of at most 200 characters"); |
| 25 | 26 | const kind = args["kind"]; |
| 26 | if (kind !== undefined && kind !== "issues" && kind !== "repos" && kind !== "code") | |
| 27 | return bad("invalid", "`kind` is issues, repos or code"); | |
| 27 | if (kind !== undefined && typeof kind !== "string") return bad("invalid_kind", "kind is all, repos, issues or code"); | |
| 28 | const refusal = searchRefusal(kind, args["limit"] === undefined || typeof args["limit"] === "number" ? (args["limit"] as number | undefined) : NaN); | |
| 29 | if (refusal) return bad(refusal.code, refusal.message); | |
| 28 | 30 | const all = args["all"]; |
| 29 | 31 | if (all !== undefined && typeof all !== "boolean") return bad("invalid", "`all` is true or false"); |
| 30 | 32 | let only: Repo | undefined; |
| @@ -34,10 +36,11 @@ export const tool: Tool = { | ||
| 34 | 36 | if (isAnswer(repo)) return repo; |
| 35 | 37 | only = repo; |
| 36 | 38 | } |
| 37 | const note = searchNote(q, kind ?? null); | |
| 39 | const note = searchNote(q); | |
| 38 | 40 | const found = await search(ctx.app, user, { |
| 39 | 41 | q, |
| 40 | ...(kind ? { kind } : {}), | |
| 42 | ...(kind ? { kind: searchKind(kind) } : {}), | |
| 43 | ...(typeof args["limit"] === "number" ? { limit: args["limit"] } : {}), | |
| 41 | 44 | ...(only ? { repo: only } : {}), |
| 42 | 45 | ...(all === true ? {} : { codeBudgetMs: ctx.app.config.codeBudgetMs ?? CODE_BUDGET_MS }), |
| 43 | 46 | }); |
test/mcp-search.test.ts +12 −1
| @@ -110,8 +110,19 @@ describe("MCP search", () => { | ||
| 110 | 110 | expect(scoped.value.issues.map((i: any) => i.repo.name)).toEqual(["pebble"]); |
| 111 | 111 | }); |
| 112 | 112 | |
| 113 | test("an unknown kind or a bad limit is refused in the API's words; every kind answers", async () => { | |
| 114 | expect((await tool(aliceRo, { q: "a", kind: "bogus" })).error).toContain("invalid_kind"); | |
| 115 | expect((await tool(aliceRo, { q: "a", kind: "bogus" })).error).toContain("all, repos, issues or code"); | |
| 116 | for (const limit of [0, 101, 1.5, "5"]) { | |
| 117 | const r = await tool(aliceRo, { q: "a", limit }); | |
| 118 | expect(r.error).toContain("invalid_limit"); | |
| 119 | expect(r.error).toContain("1 to 100"); | |
| 120 | } | |
| 121 | for (const kind of ["all", "repos", "issues", "code"]) expect((await tool(aliceRo, { q: "a", kind, limit: 5 })).error).toBeUndefined(); | |
| 122 | }); | |
| 123 | ||
| 113 | 124 | test("bad arguments are refused", async () => { |
| 114 | for (const args of [{}, { q: "x".repeat(201) }, { q: 5 }, { q: "a", kind: "all" }, { q: "a", repo: 3 }]) { | |
| 125 | for (const args of [{}, { q: "x".repeat(201) }, { q: 5 }, { q: "a", repo: 3 }]) { | |
| 115 | 126 | expect((await tool(aliceRo, args)).error).toBeDefined(); |
| 116 | 127 | } |
| 117 | 128 | }); |
test/search-note.test.ts +25 −7
| @@ -35,18 +35,36 @@ test("a missing or blank q answers the empty shape plus a note with an example a | ||
| 35 | 35 | } |
| 36 | 36 | }); |
| 37 | 37 | |
| 38 | test("an unknown kind is named in the note", async () => { | |
| 39 | const r = await get("?q=forge&kind=bogus"); | |
| 40 | expect(r.note).toContain('"bogus"'); | |
| 41 | expect(r.note).toContain("kind=all|repos|issues|code"); | |
| 38 | const raw = (qs: string) => fetch(`${servers.url}/api/v1/search${qs}`); | |
| 39 | ||
| 40 | test("an unknown or empty kind is a 422 invalid_kind listing the kinds", async () => { | |
| 41 | for (const kind of ["bogus", ""]) { | |
| 42 | const res = await raw(`?q=forge&kind=${kind}`); | |
| 43 | expect(res.status).toBe(422); | |
| 44 | const body = (await res.json()) as any; | |
| 45 | expect(body.error).toBe("invalid_kind"); | |
| 46 | expect(body.message).toContain("all, repos, issues or code"); | |
| 47 | } | |
| 42 | 48 | }); |
| 43 | 49 | |
| 44 | test("an empty kind is not a kind either", async () => { | |
| 45 | expect((await get("?q=forge&kind=")).note).toContain("is not a kind"); | |
| 50 | test("a limit outside 1 to 100 or not whole is a 422 invalid_limit naming the range", async () => { | |
| 51 | for (const limit of ["0", "101", "-1", "1.5", "abc", "", "1e1"]) { | |
| 52 | const res = await raw(`?q=forge&limit=${limit}`); | |
| 53 | expect(res.status).toBe(422); | |
| 54 | const body = (await res.json()) as any; | |
| 55 | expect(body.error).toBe("invalid_limit"); | |
| 56 | expect(body.message).toContain("1 to 100"); | |
| 57 | } | |
| 58 | expect((await raw("?q=forge&limit=100")).status).toBe(200); | |
| 59 | expect((await raw("?q=forge&limit=1")).status).toBe(200); | |
| 60 | }); | |
| 61 | ||
| 62 | test("every listed kind still answers 200", async () => { | |
| 63 | for (const kind of ["all", "repos", "issues", "code"]) expect((await raw(`?q=forge&kind=${kind}`)).status).toBe(200); | |
| 46 | 64 | }); |
| 47 | 65 | |
| 48 | 66 | test("a good request carries no note", async () => { |
| 49 | 67 | expect((await get("?q=forge&kind=repos")).note).toBeUndefined(); |
| 50 | 68 | expect((await get("?q=forge&kind=all")).note).toBeUndefined(); |
| 51 | expect(searchNote("x", null)).toBeUndefined(); | |
| 69 | expect(searchNote("x")).toBeUndefined(); | |
| 52 | 70 | }); |
new above
offline
The
notedescription still says "or kind is unknown"; that case is now a 422 invalid_kind. Say only: when q is blank.