diff --git a/review-enrichment/src/analyzers/doc-comment-drift.ts b/review-enrichment/src/analyzers/doc-comment-drift.ts new file mode 100644 index 0000000000..32b021bdd3 --- /dev/null +++ b/review-enrichment/src/analyzers/doc-comment-drift.ts @@ -0,0 +1,312 @@ +// Doc-comment-vs-signature drift analyzer (#1519). Flags a JSDoc/TSDoc `@param` tag that names a parameter the PR +// REMOVED or RENAMED while leaving the doc stale. It fetches the full changed file at headSha (one authed contents +// fetch), reverse-applies the patch to reconstruct the pre-PR file, and compares each function's OLD parameter set +// against its NEW one: a `@param` is drift only when it was a real parameter before and is gone now. This makes a +// non-parameter signature edit (return type, name, modifier, parameter type) over PRE-EXISTING stale docs a +// non-finding. Deliberately conservative: only NAMED `function` declarations whose parameters are confidently +// enumerable (any destructuring / non-identifier param → skip the function). Reports symbol + stale params + line. +import type { EnrichRequest, DocCommentDriftFinding } from "../types.js"; + +const MAX_FILES = 20; +const MAX_FINDINGS = 50; +const MAX_SIGNATURE_LINES = 40; +const SOURCE_RE = /\.(?:ts|tsx|js|jsx|mjs|cjs)$/; +const SKIP_RE = /(?:\.d\.ts$|\.min\.|\.test\.|\.spec\.|__tests__\/|(?:^|\/)tests?\/)/; +const SLUG_RE = /^[A-Za-z0-9._-]+$/; +// Matches a named `function` declaration up to its parameter `(`. A single, non-nested generic clause is allowed; +// a nested-generic declaration (e.g. `function f>(x)`) simply does not match and +// the function is skipped — a deliberate recall/precision trade-off, never a false positive. +const FUNC_DECL_RE = /^\s*(?:export\s+)?(?:default\s+)?(?:async\s+)?function\s*\*?\s*([A-Za-z_$][\w$]*)\s*(?:<[^>]*>)?\s*\(/; + +interface ScanOptions { + signal?: AbortSignal; +} + +/** Reconstruct the pre-PR content of a file by reverse-applying its unified `patch` to the post-PR `newContent`: + * context and removed (`-`) lines rebuild the old text; added (`+`) lines are dropped. Returns null if a hunk's + * position runs past the content (so the caller falls back to "no old parameters" and reports nothing). Pure. */ +export function reconstructOldContent(newContent: string, patch: string): string | null { + const newLines = newContent.split("\n"); + const patchLines = patch.split("\n"); + const out: string[] = []; + let cursor = 0; // next unconsumed index into newLines + let i = 0; + while (i < patchLines.length) { + const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(patchLines[i]!); + if (!header) { + i += 1; + continue; + } + const hunkStart = Number(header[1]) - 1; // 0-based new-file line the hunk begins at + if (hunkStart < cursor || hunkStart > newLines.length) return null; + while (cursor < hunkStart) out.push(newLines[cursor++]!); // unchanged lines before the hunk + i += 1; + while (i < patchLines.length && !patchLines[i]!.startsWith("@@")) { + const l = patchLines[i]!; + if (!l.startsWith("\\")) { + const sign = l[0]; + const body = l.slice(1); + if (sign === "-") { + out.push(body); // removed: present in old only + } else { + // added or context lines must match the fetched head content at the cursor; a mismatch means the patch + // doesn't align with `newContent` (malformed/truncated input) → bail so we never trust a bad old signature. + if (newLines[cursor] !== body) return null; + if (sign !== "+") out.push(body); // context is present in old too; an added line is not + cursor += 1; + } + } + i += 1; + } + } + while (cursor < newLines.length) out.push(newLines[cursor++]!); + return out.join("\n"); +} + +/** Map every named `function NAME` declaration in `content` to its enumerable parameter-name set. A function whose + * parameters aren't confidently enumerable is omitted; a name DECLARED MORE THAN ONCE (overload/duplicate) is + * excluded entirely, so a lookup can never return a sibling declaration's parameters. Used to compare OLD vs NEW. */ +export function extractFunctionParams(content: string): Map> { + const lines = content.split("\n"); + const byName = new Map>(); + const seen = new Set(); + for (let i = 0; i < lines.length; i++) { + const decl = FUNC_DECL_RE.exec(lines[i]!); + if (!decl) continue; + const name = decl[1]!; + if (seen.has(name)) { + byName.delete(name); // a second declaration of this name → ambiguous, exclude it + continue; + } + seen.add(name); + const params = extractParamSource(lines, i, decl[0].length - 1); + if (!params) continue; + const names = parseFunctionParams(params.src); + if (names) byName.set(name, new Set(names)); + } + return byName; +} + +/** Top-level `@param` identifier names from a JSDoc block. Only real TAG LINES are read — a line whose content + * (after an optional `*` gutter) begins with `@param` — so a `@param` token sitting inside prose or an `@example` + * body never fabricates a documented parameter. The type group tolerates one level of brace nesting + * (`@param {{x: string}} opts`). Nested tags (`@param obj.prop`) reference an existing param and are ignored. Pure. */ +export function parseDocParams(jsdoc: string): string[] { + const names: string[] = []; + // `@param` must begin the line's content after an optional `/**` opener (single-line block) or `*` gutter, so a + // single-line `/** @param x */` is read while a `@param` token buried in prose or an `@example` body is not. + const tag = /^\s*(?:\/\*\*+\s*)?\*?\s*@param\s+(?:\{(?:[^{}]|\{[^{}]*\})*\}\s*)?\[?\s*([A-Za-z_$][\w$]*)(\.[\w$]+)?/; + for (const line of jsdoc.split("\n")) { + const match = tag.exec(line); + if (match && !match[2]) names.push(match[1]!); + } + return names; +} + +/** Split a parameter-list source on top-level commas, tracking ()/{}/[] depth and string literals. Angle brackets + * are intentionally NOT tracked: `<`/`>` are ambiguous between generics and comparison/arrow operators, so a + * default like `n = max > 0 ? max : 1` would be mis-balanced. A comma inside a generic (`Map`) therefore + * splits, but the resulting type-argument fragment is dropped by `parseFunctionParams`. Returns null only if the + * unambiguous brackets never balance. */ +function splitParams(src: string): string[] | null { + const parts: string[] = []; + let depth = 0; + let start = 0; + let quote: string | null = null; + for (let i = 0; i < src.length; i++) { + const ch = src[i]!; + if (quote) { + if (ch === quote && src[i - 1] !== "\\") quote = null; + continue; + } + if (ch === '"' || ch === "'" || ch === "`") quote = ch; + else if (ch === "(" || ch === "{" || ch === "[") depth += 1; + else if (ch === ")" || ch === "}" || ch === "]") { + depth -= 1; + if (depth < 0) return null; + } else if (ch === "," && depth === 0) { + parts.push(src.slice(start, i)); + start = i + 1; + } + } + if (depth !== 0) return null; + parts.push(src.slice(start)); + return parts; +} + +/** Parameter names of a function from its parenthesised source, or null when not confidently enumerable. + * Each comma-separated segment yields its leading identifier (after an optional rest marker). A destructured + * segment (`{…}`/`[…]`) makes the set ambiguous → null. A segment whose identifier is immediately followed by + * `<`/`>` is a generic type-argument fragment left over from splitting a generic (`Map`), not a real + * parameter, and is dropped — so comparison defaults and callback params stay enumerable without ever inventing + * a name. Pure. */ +export function parseFunctionParams(paramSrc: string): string[] | null { + const trimmed = paramSrc.trim(); + if (!trimmed) return []; + const parts = splitParams(trimmed); + if (!parts) return null; // unbalanced brackets — not confidently enumerable + const names: string[] = []; + for (const raw of parts) { + const part = raw.trim().replace(/^\.\.\.\s*/, ""); // drop a rest marker + if (!part) continue; + if (part.startsWith("{") || part.startsWith("[")) return null; // destructured pattern — names not enumerable + const name = /^([A-Za-z_$][\w$]*)/.exec(part); + if (!name) return null; // not a plain identifier — ambiguous + const id = name[1]!; + // After the name a real parameter has only a type (`:`), an optional marker (`?`), a default (`=`), or nothing. + // Anything else means this segment is a generic type-argument fragment left over from splitting a generic + // (e.g. `readonly V[]>` from `Map`) — fail closed and skip the whole function rather than + // invent a parameter name. + const rest = part.slice(id.length).trimStart(); + if (rest && !/^[:?=]/.test(rest)) return null; + if (id === "this") continue; // a TS `this` pseudo-parameter is not a real argument + names.push(id); + } + return names; +} + +/** From the `(` at `lines[startLine][openIdx]`, return the inner parameter source (balanced, possibly multi-line) + * and the line index of the matching `)`. Null if unbalanced within the line budget. */ +function extractParamSource( + lines: string[], + startLine: number, + openIdx: number, +): { src: string; endLine: number } | null { + let depth = 0; + let src = ""; + let quote: string | null = null; + const limit = Math.min(lines.length, startLine + MAX_SIGNATURE_LINES); + for (let li = startLine; li < limit; li++) { + const line = lines[li]!; + for (let k = li === startLine ? openIdx : 0; k < line.length; k++) { + const ch = line[k]!; + if (quote) { + if (ch === quote && line[k - 1] !== "\\") quote = null; + src += ch; + continue; + } + if (ch === '"' || ch === "'" || ch === "`") { + quote = ch; + src += ch; + continue; + } + if (ch === "(") { + depth += 1; + if (depth === 1) continue; // drop the outer opening paren + } else if (ch === ")") { + depth -= 1; + if (depth === 0) return { src, endLine: li }; + } + src += ch; + } + src += "\n"; + } + return null; +} + +/** The JSDoc block directly above `lines[funcLine]` (only blank lines may separate them), or null. The adjacent + * block must be a real `/**` JSDoc: we take the contiguous block ending at the nearest `*/` and walk up to ITS + * opener (the first `/*` — block comments don't nest). A plain `/* … *​/` block returns null, so an unrelated + * earlier JSDoc above a plain comment is never mis-attached. */ +function precedingJsdoc(lines: string[], funcLine: number): string | null { + let end = funcLine - 1; + while (end >= 0 && lines[end]!.trim() === "") end -= 1; + if (end < 0 || !lines[end]!.trimEnd().endsWith("*/")) return null; + let start = end; + while (start >= 0 && !lines[start]!.includes("/*")) start -= 1; + if (start < 0 || !lines[start]!.trimStart().startsWith("/**")) return null; // opener must be a real JSDoc block + return lines.slice(start, end + 1).join("\n"); +} + +/** Pure: find functions whose preceding JSDoc documents a `@param` that was a REAL parameter before this PR + * (`oldParamsByName`, from the reconstructed old file) but is absent from the CURRENT signature — i.e. the PR + * removed or renamed that parameter and left the doc stale. Gating on the name having been an actual OLD parameter + * (rather than merely a token the patch touched) is what keeps a non-parameter signature edit — a return type, + * name, modifier, or parameter type — over pre-existing stale docs from being reported as PR-introduced drift. + * Conservative: only named `function` declarations with confidently-enumerable params and an adjacent `/**` JSDoc. */ +export function findDocCommentDrift( + content: string, + oldParamsByName: Map>, +): Array<{ symbol: string; line: number; staleParams: string[] }> { + const lines = content.split("\n"); + // Count declarations per name so an overload/duplicate (which can't be matched 1:1 to an old signature) is skipped. + const nameCounts = new Map(); + for (const line of lines) { + const decl = FUNC_DECL_RE.exec(line); + if (decl) nameCounts.set(decl[1]!, (nameCounts.get(decl[1]!) ?? 0) + 1); + } + const findings: Array<{ symbol: string; line: number; staleParams: string[] }> = []; + for (let i = 0; i < lines.length; i++) { + const decl = FUNC_DECL_RE.exec(lines[i]!); + if (!decl) continue; + if ((nameCounts.get(decl[1]!) ?? 0) > 1) continue; // duplicate-named declaration — ambiguous, skip + const params = extractParamSource(lines, i, decl[0].length - 1); + if (!params) continue; + + const actual = parseFunctionParams(params.src); + if (actual === null) continue; // ambiguous current signature — skip + const jsdoc = precedingJsdoc(lines, i); + if (!jsdoc) continue; + + const oldParams = oldParamsByName.get(decl[1]!); + if (!oldParams) continue; // no confidently-enumerable old signature for this name → nothing was removed + + const declared = new Set(actual); + // Stale drift = a documented param that WAS a real parameter before the PR and is gone now (removed/renamed). + const stale = [ + ...new Set(parseDocParams(jsdoc).filter((name) => oldParams.has(name) && !declared.has(name))), + ]; + if (stale.length) findings.push({ symbol: decl[1]!, line: i + 1, staleParams: stale }); + } + return findings; +} + +/** Analyzer entrypoint: fetch each changed source file at headSha, report doc-vs-signature drift. Fail-safe. */ +export async function scanDocCommentDrift( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const { repoFullName, githubToken, headSha, files = [] } = req; + if (!githubToken || !headSha) return []; + const [owner, repo] = repoFullName.split("/"); + if (!owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + + const headers: Record = { + Authorization: `Bearer ${githubToken}`, + Accept: "application/vnd.github.raw", + "X-GitHub-Api-Version": "2022-11-28", + }; + const sources = files + .filter((file) => file.patch && SOURCE_RE.test(file.path) && !SKIP_RE.test(file.path)) + .slice(0, MAX_FILES); + + const findings: DocCommentDriftFinding[] = []; + for (const file of sources) { + if (options.signal?.aborted) break; + + let content: string | null = null; + try { + const path = file.path.split("/").map(encodeURIComponent).join("/"); + const resp = await fetchFn( + `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${path}?ref=${encodeURIComponent(headSha)}`, + { headers, signal: options.signal }, + ); + if (resp.ok) content = await resp.text(); + } catch { + content = null; + } + if (!content) continue; + if (options.signal?.aborted) break; // an abort during the fetch should suppress this file's findings too + + // Reverse-apply the patch to get the PRE-PR file, then compare each function's OLD vs NEW parameters. + const oldContent = reconstructOldContent(content, file.patch!); + if (!oldContent) continue; // couldn't reconstruct the pre-PR file → fail closed, report nothing + const oldParamsByName = extractFunctionParams(oldContent); + for (const drift of findDocCommentDrift(content, oldParamsByName)) { + findings.push({ file: file.path, line: drift.line, symbol: drift.symbol, staleParams: drift.staleParams }); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 89369c84ec..767e21cafe 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -3,6 +3,7 @@ import { scanAssetWeight } from "./asset-weight.js"; import { scanCodeowners } from "./codeowners.js"; import { scanCommitSignature } from "./commit-signature.js"; import { dependencyAnalyzer } from "./dependency/descriptor.js"; +import { scanDocCommentDrift } from "./doc-comment-drift.js"; import { scanEol } from "./eol-check.js"; import { scanHeavyDependencies } from "./heavy-dependency.js"; import { scanHistory } from "./history.js"; @@ -355,6 +356,26 @@ export const ANALYZER_DESCRIPTORS = [ diagnostics: context.diagnostics, }), }), + descriptor({ + name: "docCommentDrift", + title: "Doc-comment drift", + category: "quality", + cost: "github-light", + defaultEnabled: true, + requires: ["files", "github-token", "head-sha"], + limits: { maxFiles: 20, maxFindings: 50 }, + docs: { + summary: + "Flags a JSDoc/TSDoc @param that names a parameter the PR removed or renamed but left documented.", + looksAt: + "Changed TS/JS source files at headSha, comparing each named function's old vs new parameter list.", + reports: "File, line, function, and the stale parameter name(s).", + network: "Calls the GitHub API for changed file contents. Requires headSha and token forwarding for private repos.", + notes: + "Conservative: only named function declarations with confidently-enumerable params; non-parameter signature edits are not reported.", + }, + run: (req, { signal }) => scanDocCommentDrift(req, fetch, { signal }), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 4fcbad3524..c90461c434 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -340,6 +340,19 @@ export function renderBrief( } } + const docDrift = findings.docCommentDrift ?? []; + if (docDrift.length) { + lines.push( + "### Doc-comment drift (JSDoc @param names the signature no longer declares — update the doc)", + ); + for (const item of docDrift) { + const params = item.staleParams.map((name) => safeCodeSpan(name)).join(", "); + lines.push( + `- ${safeCodeSpan(`${item.file}:${item.line}`)} ${safeCodeSpan(item.symbol)} documents ${params} — no longer a parameter`, + ); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 4d8f382a87..ecb7d7dd54 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -287,6 +287,19 @@ export interface BriefFindings { iacMisconfig?: IacMisconfigFinding[]; nativeBuild?: NativeBuildFinding[]; history?: HistoryFinding[]; + docCommentDrift?: DocCommentDriftFinding[]; +} + +/** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a + * verifiable doc-vs-signature drift the PR introduced by changing the signature. Reports the function name + + * the stale parameter names + location only. Functions with destructured/ambiguous params are skipped (so the + * param set is always confidently enumerable). (#1519) */ +export interface DocCommentDriftFinding { + file: string; + line: number; + symbol: string; + /** `@param` names documented but absent from the function's actual parameter list. */ + staleParams: string[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 9041ad97db..6ec7364335 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -28,6 +28,7 @@ const EXPECTED_ANALYZERS = [ "iacMisconfig", "nativeBuild", "history", + "docCommentDrift", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/doc-comment-drift.test.ts b/review-enrichment/test/doc-comment-drift.test.ts new file mode 100644 index 0000000000..6d6e217d34 --- /dev/null +++ b/review-enrichment/test/doc-comment-drift.test.ts @@ -0,0 +1,225 @@ +// Units for the doc-comment-vs-signature drift analyzer (#1519). Own file (not enrichment.test.ts) so concurrent +// analyzer PRs don't collide. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + reconstructOldContent, + extractFunctionParams, + parseDocParams, + parseFunctionParams, + findDocCommentDrift, + scanDocCommentDrift, +} from "../dist/analyzers/doc-comment-drift.js"; +import { renderBrief } from "../dist/render.js"; + +const baseReq = (files) => ({ + repoFullName: "o/r", + prNumber: 1, + headSha: "abc123", + githubToken: "ght", + files, +}); +const fileWith = (content) => async () => ({ ok: true, text: async () => content }); +const status = (code) => async () => ({ ok: code >= 200 && code < 300, status: code, text: async () => "" }); +const oldParams = (entries) => new Map(entries.map(([name, ids]) => [name, new Set(ids)])); + +const DRIFTED = `/**\n * @param oldName the old one\n */\nexport function doThing(newName) {\n return newName;\n}\n`; +const DRIFT_PATCH = `@@ -1,6 +1,6 @@\n /**\n * @param oldName the old one\n */\n-export function doThing(oldName) {\n+export function doThing(newName) {\n return newName;\n }`; + +test("reconstructOldContent: reverse-applies a patch to rebuild the pre-PR file", () => { + const old = reconstructOldContent(DRIFTED, DRIFT_PATCH); + assert.match(old, /function doThing\(oldName\)/); // the old parameter name is restored + assert.doesNotMatch(old, /newName\) \{/); // the added signature line is dropped +}); + +test("extractFunctionParams: maps each enumerable named function to its parameter set", () => { + const map = extractFunctionParams(`export function f(a, b) {}\nfunction g({ x }) {}\nfunction h(c) {}\n`); + assert.deepEqual([...map.get("f")], ["a", "b"]); + assert.deepEqual([...map.get("h")], ["c"]); + assert.equal(map.has("g"), false); // destructured params aren't enumerable → omitted +}); + +test("extractFunctionParams: excludes a name declared more than once (overload/duplicate)", () => { + const map = extractFunctionParams(`function dup(a) {}\nfunction other(z) {}\nfunction dup(a, b) {}\n`); + assert.equal(map.has("dup"), false); // ambiguous — never returns one declaration's params for another + assert.deepEqual([...map.get("other")], ["z"]); +}); + +test("reconstructOldContent: bails (null) when the patch context does not match the head content", () => { + // The context line ` other` doesn't exist in newContent → misaligned patch → fail closed. + assert.equal(reconstructOldContent(`a\nb\n`, `@@ -1,2 +1,2 @@\n-x\n+a\n other`), null); +}); + +test("findDocCommentDrift: a duplicate-named function is skipped (no cross-declaration false positive)", () => { + // Two `dup` declarations; a stale @param on the first must not borrow the other's old params. + const content = `/**\n * @param gone\n */\nexport function dup(a) {}\nfunction dup(b) {}\n`; + assert.deepEqual(findDocCommentDrift(content, oldParams([["dup", ["gone", "a"]]])), []); +}); + +test("parseDocParams: top-level names only; nested and typed/optional handled", () => { + const jsdoc = `/**\n * @param {string} a\n * @param [b]\n * @param {T} [c=1] desc\n * @param {{x: string}} d\n * @param opts.nested skip-me\n */`; + assert.deepEqual(parseDocParams(jsdoc), ["a", "b", "c", "d"]); +}); + +test("parseDocParams: reads a single-line JSDoc block tag, but still not prose inside one", () => { + assert.deepEqual(parseDocParams(`/** @param oldName the value */`), ["oldName"]); // single-line block + assert.deepEqual(parseDocParams(`/** describes the @param convention */`), []); // @param buried in prose +}); + +test("parseDocParams: ignores @param inside prose or an @example body (only real tag lines)", () => { + const jsdoc = `/**\n * Pass the @param oldName through; see below.\n * @example\n * doThing(); // @param oldName demo\n * @param realName the only true tag\n */`; + assert.deepEqual(parseDocParams(jsdoc), ["realName"]); +}); + +test("parseDocParams: a long malformed @param brace line yields no name (fail-safe, linear)", () => { + const jsdoc = `/**\n * @param ${"{".repeat(64)} unterminated type and no name\n */`; + assert.deepEqual(parseDocParams(jsdoc), []); +}); + +test("parseFunctionParams: enumerates simple params, strips types/defaults/rest/this", () => { + assert.deepEqual(parseFunctionParams("a, b, c"), ["a", "b", "c"]); + assert.deepEqual(parseFunctionParams("a: number, b?: string"), ["a", "b"]); + assert.deepEqual(parseFunctionParams("a, b = 1"), ["a", "b"]); + assert.deepEqual(parseFunctionParams("...args"), ["args"]); + assert.deepEqual(parseFunctionParams("this: Foo, a"), ["a"]); + assert.deepEqual(parseFunctionParams("opts: { x: string }, b"), ["opts", "b"]); + assert.deepEqual(parseFunctionParams(""), []); +}); + +test("parseFunctionParams: comparison defaults and callback/arrow params stay enumerable (no false skip)", () => { + assert.deepEqual(parseFunctionParams("limit = max > 0 ? max : 1, b"), ["limit", "b"]); + assert.deepEqual(parseFunctionParams("a = b > c, d"), ["a", "d"]); + assert.deepEqual(parseFunctionParams("cb: (x: string) => void, b"), ["cb", "b"]); + assert.deepEqual(parseFunctionParams("a, cb = () => a"), ["a", "cb"]); +}); + +test("parseFunctionParams: fails closed (null) on destructuring, unbalanced, or generic-comma fragments", () => { + assert.equal(parseFunctionParams("{ a, b }"), null); + assert.equal(parseFunctionParams("[a, b]"), null); + assert.equal(parseFunctionParams("a, (b"), null); + assert.equal(parseFunctionParams("a: Map, b"), null); + assert.equal(parseFunctionParams("a: Map, b"), null); +}); + +test("findDocCommentDrift: flags a @param that was a real OLD parameter and is now gone (rename)", () => { + const out = findDocCommentDrift(DRIFTED, oldParams([["doThing", ["oldName"]]])); + assert.equal(out.length, 1); + assert.equal(out[0].symbol, "doThing"); + assert.equal(out[0].line, 4); + assert.deepEqual(out[0].staleParams, ["oldName"]); +}); + +test("findDocCommentDrift: a pre-existing stale @param is NOT flagged when the parameter set didn't change", () => { + // `oldName` was never a real parameter (old params are `newName`, same as now) — a non-parameter edit elsewhere. + assert.deepEqual(findDocCommentDrift(DRIFTED, oldParams([["doThing", ["newName"]]])), []); +}); + +test("findDocCommentDrift: catches a param removed from a multi-line signature", () => { + const content = `/**\n * @param a\n * @param b\n */\nexport function multi(\n a,\n) {}\n`; + const out = findDocCommentDrift(content, oldParams([["multi", ["a", "b"]]])); + assert.equal(out.length, 1); + assert.deepEqual(out[0].staleParams, ["b"]); +}); + +test("findDocCommentDrift: skips an ambiguous (destructured) current signature", () => { + const content = `/**\n * @param missing\n */\nexport function f({ a, b }) {\n return a + b;\n}\n`; + assert.deepEqual(findDocCommentDrift(content, oldParams([["f", ["missing"]]])), []); +}); + +test("findDocCommentDrift: no finding when every @param still exists", () => { + const content = `/**\n * @param a\n * @param b\n */\nfunction g(a, b) {}\n`; + assert.deepEqual(findDocCommentDrift(content, oldParams([["g", ["a", "b"]]])), []); +}); + +test("findDocCommentDrift: handles a multi-line signature", () => { + const content = `/**\n * @param gone\n */\nexport function multi(\n a: number,\n b: string,\n) {}\n`; + const out = findDocCommentDrift(content, oldParams([["multi", ["a", "b", "gone"]]])); + assert.equal(out.length, 1); + assert.deepEqual(out[0].staleParams, ["gone"]); +}); + +test("findDocCommentDrift: a plain block comment between an earlier JSDoc and the function is not attached", () => { + const content = `/**\n * @param gone\n */\n/* a plain note */\nexport function f(a) {}\n`; + assert.deepEqual(findDocCommentDrift(content, oldParams([["f", ["a", "gone"]]])), []); +}); + +test("findDocCommentDrift: a nested @param (opts.x) is never stale when opts exists", () => { + const content = `/**\n * @param opts\n * @param opts.x\n */\nfunction h(opts) {}\n`; + assert.deepEqual(findDocCommentDrift(content, oldParams([["h", ["opts"]]])), []); +}); + +test("scanDocCommentDrift: a non-parameter signature edit over PRE-EXISTING stale docs is NOT reported", async () => { + // The PR only changes the RETURN TYPE; `@param ghost` was already stale (never a real parameter). + const content = `/**\n * @param a\n * @param ghost\n */\nexport function f(a): Promise {}\n`; + const patch = `@@ -1,5 +1,5 @@\n /**\n * @param a\n * @param ghost\n */\n-export function f(a): void {}\n+export function f(a): Promise {}`; + assert.deepEqual(await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch }]), fileWith(content)), []); +}); + +test("scanDocCommentDrift: an unrelated same-named removal elsewhere does NOT trip a return-type-only edit", async () => { + const content = `const keep = 2;\n/**\n * @param a\n * @param ghost\n */\nexport function f(a): Promise {}\n`; + const patch = `@@ -1,2 +1,2 @@\n-const ghost = 1;\n+const keep = 2;\n@@ -6,1 +6,1 @@\n-export function f(a): void {}\n+export function f(a): Promise {}`; + assert.deepEqual(await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch }]), fileWith(content)), []); +}); + +test("scanDocCommentDrift: a parameter the PR actually removed IS reported", async () => { + const content = `/**\n * @param a\n * @param removed\n */\nexport function f(a) {}\n`; + const patch = `@@ -1,5 +1,5 @@\n /**\n * @param a\n * @param removed\n */\n-export function f(a, removed) {}\n+export function f(a) {}`; + const findings = await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch }]), fileWith(content)); + assert.equal(findings.length, 1); + assert.deepEqual(findings[0].staleParams, ["removed"]); +}); + +test("scanDocCommentDrift: fetches the file at headSha and reports drift", async () => { + const findings = await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch: DRIFT_PATCH }]), fileWith(DRIFTED)); + assert.equal(findings.length, 1); + assert.equal(findings[0].file, "src/a.ts"); + assert.deepEqual(findings[0].staleParams, ["oldName"]); +}); + +test("scanDocCommentDrift: requires a github token and a head sha", async () => { + assert.deepEqual(await scanDocCommentDrift({ repoFullName: "o/r", prNumber: 1, headSha: "x", files: [{ path: "src/a.ts", patch: DRIFT_PATCH }] }, fileWith(DRIFTED)), []); + assert.deepEqual(await scanDocCommentDrift({ repoFullName: "o/r", prNumber: 1, githubToken: "t", files: [{ path: "src/a.ts", patch: DRIFT_PATCH }] }, fileWith(DRIFTED)), []); +}); + +test("scanDocCommentDrift: skips non-source and test files without fetching", async () => { + let called = false; + const out = await scanDocCommentDrift( + baseReq([ + { path: "README.md", patch: DRIFT_PATCH }, + { path: "src/a.test.ts", patch: DRIFT_PATCH }, + ]), + async () => { + called = true; + return fileWith(DRIFTED)(); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanDocCommentDrift: fails safe on a non-ok or throwing fetch", async () => { + assert.deepEqual(await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch: DRIFT_PATCH }]), status(404)), []); + assert.deepEqual( + await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch: DRIFT_PATCH }]), async () => { + throw new Error("network"); + }), + [], + ); +}); + +test("scanDocCommentDrift: stops on an already-aborted signal", async () => { + const out = await scanDocCommentDrift(baseReq([{ path: "src/a.ts", patch: DRIFT_PATCH }]), fileWith(DRIFTED), { + signal: AbortSignal.abort(), + }); + assert.deepEqual(out, []); +}); + +test("renderBrief emits a public-safe doc-comment-drift block", () => { + const { promptSection } = renderBrief({ + docCommentDrift: [{ file: "src/a.ts", line: 4, symbol: "doThing", staleParams: ["oldName"] }], + }); + assert.match(promptSection, /Doc-comment drift/); + assert.match(promptSection, /src\/a\.ts:4/); + assert.match(promptSection, /doThing/); + assert.match(promptSection, /oldName/); +});