Public: anyone can read this; the crew can change it.
GET /api/v1/search with an empty q says what to send, as explore does #8496
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/8485-get-api-v1-search-with-an-empty-q-says-w → main 4 commits fork 2 threads · 2 unresolved
7 files
changed +94
−17
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" } }; | |
| 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" } }; |
src/core/search.ts +14
| @@ -59,6 +59,20 @@ export function searchKind(raw: string | null): SearchKind { | ||
| 59 | 59 | return raw === "repos" || raw === "issues" || raw === "code" ? raw : "all"; |
| 60 | 60 | } |
| 61 | 61 | |
| 62 | /** | |
| 63 | * What a caller who asked wrongly is told, in one sentence, or undefined when | |
| 64 | * the request was fine. Plain text: the caller escapes it where it draws it. | |
| 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}`; | |
| 71 | if (cleanQuery(rawQ ?? "") === "") | |
| 72 | return `q is empty — send what to find, e.g. q=parser&kind=code (${kinds})`; | |
| 73 | return undefined; | |
| 74 | } | |
| 75 | ||
| 62 | 76 | /** A repo here, or one cached from an origin the actor has connected. */ |
| 63 | 77 | export type RepoHit = |
| 64 | 78 | | { |
src/http/api.ts +3 −2
| @@ -156,7 +156,7 @@ import { | ||
| 156 | 156 | canWrite, |
| 157 | 157 | getRepoById, |
| 158 | 158 | } from "../core/repos.ts"; |
| 159 | import { cleanQuery, CODE_BUDGET_MS, searchKind } from "../core/search.ts"; | |
| 159 | import { cleanQuery, CODE_BUDGET_MS, searchKind, searchNote } from "../core/search.ts"; | |
| 160 | 160 | import { strangerCodeSearch } from "./strangers.ts"; |
| 161 | 161 | import { |
| 162 | 162 | isReviewState, |
| @@ -1394,7 +1394,8 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1394 | 1394 | const found = await (mine === undefined |
| 1395 | 1395 | ? search(ctx.app, ctx.auth.user, timed) |
| 1396 | 1396 | : searchWithin(ctx.app, ctx.auth.user, mine, timed)); |
| 1397 | return timePhase(ctx.request, "render", () => json(found)); | |
| 1397 | const note = searchNote(q.get("q"), q.get("kind")); | |
| 1398 | return timePhase(ctx.request, "render", () => json(note === undefined ? found : { ...found, note })); | |
| 1398 | 1399 | } finally { |
| 1399 | 1400 | held?.release(); |
| 1400 | 1401 | } |
src/http/mcp/tools/search.ts +11 −12
| @@ -1,6 +1,6 @@ | ||
| 1 | 1 | import type { Repo } from "../../../core/repos.ts"; |
| 2 | 2 | import { search } from "../../../actions/search.ts"; |
| 3 | import { CODE_BUDGET_MS } from "../../../core/search.ts"; | |
| 3 | import { CODE_BUDGET_MS, 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 = { |
| @@ -11,7 +11,7 @@ export const tool: Tool = { | ||
| 11 | 11 | inputSchema: { |
| 12 | 12 | type: "object", |
| 13 | 13 | properties: { |
| 14 | q: { type: "string", minLength: 1, maxLength: 200 }, | |
| 14 | q: { type: "string", maxLength: 200 }, | |
| 15 | 15 | kind: { type: "string", enum: ["issues", "repos", "code"] }, |
| 16 | 16 | repo: { type: "string", maxLength: 140 }, |
| 17 | 17 | all: { type: "boolean" }, |
| @@ -21,8 +21,7 @@ export const tool: Tool = { | ||
| 21 | 21 | writes: false, |
| 22 | 22 | async run(ctx, user, args) { |
| 23 | 23 | const q = args["q"]; |
| 24 | if (typeof q !== "string" || q.trim() === "" || q.length > 200) | |
| 25 | return bad("invalid", "`q` is 1 to 200 characters"); | |
| 24 | if (typeof q !== "string" || q.length > 200) return bad("invalid", "`q` is a string of at most 200 characters"); | |
| 26 | 25 | const kind = args["kind"]; |
| 27 | 26 | if (kind !== undefined && kind !== "issues" && kind !== "repos" && kind !== "code") |
| 28 | 27 | return bad("invalid", "`kind` is issues, repos or code"); |
| @@ -35,13 +34,13 @@ export const tool: Tool = { | ||
| 35 | 34 | if (isAnswer(repo)) return repo; |
| 36 | 35 | only = repo; |
| 37 | 36 | } |
| 38 | return good( | |
| 39 | await search(ctx.app, user, { | |
| 40 | q, | |
| 41 | ...(kind ? { kind } : {}), | |
| 42 | ...(only ? { repo: only } : {}), | |
| 43 | ...(all === true ? {} : { codeBudgetMs: ctx.app.config.codeBudgetMs ?? CODE_BUDGET_MS }), | |
| 44 | }), | |
| 45 | ); | |
| 37 | const note = searchNote(q, kind ?? null); | |
Indentation of this call and its arguments is off by one level; re-indent to match the file. |
||
| 38 | const found = await search(ctx.app, user, { | |
| 39 | q, | |
| 40 | ...(kind ? { kind } : {}), | |
| 41 | ...(only ? { repo: only } : {}), | |
| 42 | ...(all === true ? {} : { codeBudgetMs: ctx.app.config.codeBudgetMs ?? CODE_BUDGET_MS }), | |
| 43 | }); | |
| 44 | return good(note === undefined ? found : { ...found, note }); | |
| 46 | 45 | }, |
| 47 | 46 | }; |
test/mcp-search.test.ts +9 −1
| @@ -83,6 +83,14 @@ describe("MCP search", () => { | ||
| 83 | 83 | expect(repos.value.repos.map((x: any) => x.name)).toEqual(["pebble"]); |
| 84 | 84 | }); |
| 85 | 85 | |
| 86 | test("a blank q answers the empty shape with a note saying what to send", async () => { | |
| 87 | const r = await tool(aliceRo, { q: " " }); | |
| 88 | expect(r.value.repos).toEqual([]); | |
| 89 | expect(r.value.note).toContain("q is empty"); | |
| 90 | expect(r.value.note).toContain("kind=all|repos|issues|code"); | |
| 91 | expect((await tool(aliceRo, { q: "glow", kind: "issues" })).value.note).toBeUndefined(); | |
| 92 | }); | |
| 93 | ||
| 86 | 94 | test("a caller who cannot read a private repository finds nothing of it", async () => { |
| 87 | 95 | const r = await tool(bobRo, { q: "glow", kind: "issues" }); |
| 88 | 96 | expect(r.value.issues.map((i: any) => i.repo.name)).toEqual(["pebble"]); |
| @@ -103,7 +111,7 @@ describe("MCP search", () => { | ||
| 103 | 111 | }); |
| 104 | 112 | |
| 105 | 113 | test("bad arguments are refused", async () => { |
| 106 | for (const args of [{}, { q: "" }, { q: "x".repeat(201) }, { q: 5 }, { q: "a", kind: "all" }, { q: "a", repo: 3 }]) { | |
| 114 | for (const args of [{}, { q: "x".repeat(201) }, { q: 5 }, { q: "a", kind: "all" }, { q: "a", repo: 3 }]) { | |
| 107 | 115 | expect((await tool(aliceRo, args)).error).toBeDefined(); |
| 108 | 116 | } |
| 109 | 117 | }); |
test/search-note.test.ts added +52
| @@ -0,0 +1,52 @@ | ||
| 1 | /** A search asked wrongly says how to ask it right, over HTTP, as explore does. */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { searchNote } from "../src/core/search.ts"; | |
| 8 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 9 | ||
| 10 | let dataDir: string; | |
| 11 | let app: App; | |
| 12 | let servers: Servers; | |
| 13 | ||
| 14 | beforeAll(async () => { | |
| 15 | dataDir = await mkdtemp(join(tmpdir(), "forge-search-note-")); | |
| 16 | app = createApp({ dataDir, port: 0 }); | |
| 17 | servers = await serve(app); | |
| 18 | }); | |
| 19 | ||
| 20 | afterAll(async () => { | |
| 21 | servers.stop(); | |
| 22 | app.close(); | |
| 23 | await rm(dataDir, { recursive: true, force: true }); | |
| 24 | }); | |
| 25 | ||
| 26 | const get = async (qs: string) => (await fetch(`${servers.url}/api/v1/search${qs}`)).json() as Promise<any>; | |
| 27 | ||
| 28 | test("a missing or blank q answers the empty shape plus a note with an example and the kinds", async () => { | |
| 29 | for (const qs of ["", "?q=", "?q=%20%20"]) { | |
| 30 | const r = await get(qs); | |
| 31 | expect(r.repos).toEqual([]); | |
| 32 | expect(r.note).toContain("q is empty"); | |
| 33 | expect(r.note).toContain("q=parser"); | |
| 34 | expect(r.note).toContain("kind=all|repos|issues|code"); | |
| 35 | } | |
| 36 | }); | |
| 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"); | |
| 42 | }); | |
| 43 | ||
| 44 | test("an empty kind is not a kind either", async () => { | |
| 45 | expect((await get("?q=forge&kind=")).note).toContain("is not a kind"); | |
| 46 | }); | |
| 47 | ||
| 48 | test("a good request carries no note", async () => { | |
| 49 | expect((await get("?q=forge&kind=repos")).note).toBeUndefined(); | |
| 50 | expect((await get("?q=forge&kind=all")).note).toBeUndefined(); | |
| 51 | expect(searchNote("x", null)).toBeUndefined(); | |
| 52 | }); |
test/search.test.ts +4 −1
| @@ -29,6 +29,7 @@ const read = async <T>(response: Response): Promise<T> => | ||
| 29 | 29 | (await response.json()) as T; |
| 30 | 30 | |
| 31 | 31 | interface Results { |
| 32 | note?: string; | |
| 32 | 33 | moreCode?: boolean; |
| 33 | 34 | codeSkipped?: boolean; |
| 34 | 35 | repos: ( |
| @@ -391,7 +392,9 @@ describe("search: one query, four sources", () => { | ||
| 391 | 392 | test("an empty or whitespace query costs nothing and finds nothing", async () => { |
| 392 | 393 | for (const q of ["", "%20%20", "%00"]) { |
| 393 | 394 | const results = await find(`q=${q}`); |
| 394 | expect(results).toEqual({ | |
| 395 | expect(results.note).toContain("q is empty"); | |
| 396 | expect({ ...results, note: undefined }).toEqual({ | |
| 397 | note: undefined, | |
| 395 | 398 | repos: [], |
| 396 | 399 | issues: [], |
| 397 | 400 | code: [], |
new above
offline
Stale message: blank q is now allowed. Say "
qis a string of at most 200 characters".