From d971655039d4ae2f7bb73eafccd5ab23a86c201d Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Mon, 29 Jun 2026 18:41:53 -0500 Subject: [PATCH] feat(enrichment): doc-comment-vs-signature drift analyzer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a REES analyzer that flags a verifiable doc-vs-code mismatch the PR introduced: a JSDoc/TSDoc `@param` tag that names a parameter the adjacent function no longer declares (a signature changed but the doc was left stale). The hunk often splits the doc block from the signature, so it fetches the full changed file at headSha (one authed contents fetch) and parses it. Deliberately conservative for precision: only NAMED `function` declarations whose parameters are confidently enumerable (any destructuring or arrow-default param → the function is skipped), only functions whose signature the PR actually changed (added lines), and only top-level `@param` names (nested `opts.x` tags reference an existing param and are ignored). Non-source and test files are skipped; missing token/headSha and any fetch error fail safe. Registered in the brief orchestrator and rendered as a public-safe block (file:line + symbol + the stale param names). Tests in their own file. Closes #1519 --- .../src/analyzers/doc-comment-drift.ts | 312 ++++++++++++++++++ review-enrichment/src/analyzers/registry.ts | 21 ++ review-enrichment/src/render.ts | 13 + review-enrichment/src/types.ts | 13 + .../test/analyzer-registry.test.ts | 1 + .../test/doc-comment-drift.test.ts | 225 +++++++++++++ 6 files changed, 585 insertions(+) create mode 100644 review-enrichment/src/analyzers/doc-comment-drift.ts create mode 100644 review-enrichment/test/doc-comment-drift.test.ts 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/); +});