From 60a0b87793b09fc57f2b11332cc6b8e6aff777c0 Mon Sep 17 00:00:00 2001 From: dale053 Date: Sat, 27 Jun 2026 18:02:38 -0400 Subject: [PATCH 1/3] feat(enrichment): doc-comment vs signature drift analyzer (#1519) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a REES analyzer that flags JSDoc @param tags that don't match the adjacent function signature when the PR touches those lines — stale tags referencing removed params and missing tags for newly added params. Pure compute, no network calls. Operates on unified diff patches only: reconstructs the new-file view from hunk lines, slides a window over JSDoc-to-function pairs, and checks for drift only when at least one line in the window is a PR-added line (so pre-existing drift is not flagged). Destructured top-level params are skipped to avoid false positives from wrapper-name conventions. Caps at 20 findings per PR. Supports JS/TS/JSX/TSX/MJS/CJS. Handles bare @param, {Type} @param, [optional] bracket notation, rest params, and the TS `this` pseudo-param. Closes #1519. --- .../src/analyzers/doc-comment.ts | 280 ++++++++++++++ review-enrichment/src/brief.ts | 3 +- review-enrichment/src/render.ts | 16 + review-enrichment/src/types.ts | 13 +- review-enrichment/test/enrichment.test.ts | 352 ++++++++++++++++++ 5 files changed, 662 insertions(+), 2 deletions(-) create mode 100644 review-enrichment/src/analyzers/doc-comment.ts diff --git a/review-enrichment/src/analyzers/doc-comment.ts b/review-enrichment/src/analyzers/doc-comment.ts new file mode 100644 index 0000000000..a3c1bd8135 --- /dev/null +++ b/review-enrichment/src/analyzers/doc-comment.ts @@ -0,0 +1,280 @@ +// Doc-comment drift analyzer (#1519). Flags JSDoc @param tags that don't match the adjacent +// function signature when the PR touches the relevant lines — stale tags for removed params +// and missing tags for added params. Pure compute; no network calls. +import type { EnrichRequest, DocCommentFinding } from "../types.js"; + +const MAX_FINDINGS = 20; +const MAX_LINE_CHARS = 2000; + +// Only scan JS/TS files — JSDoc is idiomatic there. +const JS_TS_EXT = /\.(js|ts|jsx|tsx|mjs|cjs)$/i; + +// --- JSDoc @param extraction --- + +/** Extract @param names from a JSDoc block. Handles {Type} prefix and [optional] bracket notation. */ +export function extractJsDocParams(block: string): string[] { + const names: string[] = []; + // @param {optional-type} [optional-bracket] name — all prefix forms via greedy optional groups + const re = /@param\s+(?:\{[^}]*\}\s+)?\[?(\$?[a-zA-Z_]\w*)/g; + let m: RegExpExecArray | null; + while ((m = re.exec(block)) !== null) { + const name = m[1]!; + if (name !== "this") names.push(name); // TS `this` pseudo-param is not a real parameter + } + return names; +} + +// --- Function signature parsing --- + +// Split a comma-separated param list at top-level commas, respecting nested <>, (), [], {}. +function splitTopLevel(s: string): string[] { + const parts: string[] = []; + let depth = 0; + let start = 0; + for (let i = 0; i < s.length; i++) { + const c = s[i]!; + if (c === "<" || c === "(" || c === "[" || c === "{") depth++; + else if (c === ">" || c === ")" || c === "]" || c === "}") depth--; + else if (c === "," && depth === 0) { + parts.push(s.slice(start, i).trim()); + start = i + 1; + } + } + const last = s.slice(start).trim(); + if (last) parts.push(last); + return parts.filter(Boolean); +} + +/** Extract simple parameter names from a JS/TS parameter list string (between `(` and `)`). + * Returns hasDestructured=true when any top-level param is destructured — those functions are + * skipped to avoid false positives from wrapper-name mismatches (e.g. `@param options` vs `{ a, b }`). */ +export function extractFunctionParams( + paramList: string, +): { params: string[]; hasDestructured: boolean } { + const params: string[] = []; + let hasDestructured = false; + for (const p of splitTopLevel(paramList)) { + const trimmed = p.trim(); + if (!trimmed) continue; + if (trimmed.startsWith("{") || trimmed.startsWith("[")) { + hasDestructured = true; + continue; + } + // Rest param: ...name or ...name: Type + const rest = /^\.\.\.(\$?[a-zA-Z_]\w*)/.exec(trimmed); + if (rest) { + params.push(rest[1]!); + continue; + } + // Normal: name, name?: Type, name: Type = default — extract just the identifier + const norm = /^(\$?[a-zA-Z_]\w*)/.exec(trimmed); + if (norm && norm[1] !== "this") params.push(norm[1]!); + } + return { params, hasDestructured }; +} + +// --- Detect function signatures --- + +// Patterns that identify JS/TS function definitions (not calls or control structures). +const FUNC_PATTERNS: RegExp[] = [ + // Traditional: `function name(` or `async function name(` or `export (default)? function` + /(?:^|\s)(?:export\s+(?:default\s+)?)?(?:async\s+)?function\s*\*?\s*\w+\s*[<(]/, + // Class constructor + /(?:^|\s)constructor\s*\(/, + // Class methods with TS access/abstract/static/async/override modifiers + /(?:^|\s)(?:(?:public|private|protected|static|abstract|override|async|readonly)\s+)+\w+\s*[<(]/, + // Arrow functions assigned to a binding: `const name = async? (` + /(?:^|\s)(?:export\s+)?(?:const|let|var)\s+\w+\s*=\s*(?:async\s+)?\(/, +]; + +function looksLikeFunction(line: string): boolean { + return FUNC_PATTERNS.some((re) => re.test(line)); +} + +/** Extract a function name from a signature line (best-effort; falls back to ``). */ +function extractFunctionName(line: string): string { + let m = /function\s+(\w+)/.exec(line); + if (m) return m[1]!; + if (/constructor\s*\(/.test(line)) return "constructor"; + m = + /(?:public|private|protected|static|abstract|override|async|readonly)(?:\s+(?:public|private|protected|static|abstract|override|async|readonly))*\s+(\w+)\s*[<(]/.exec( + line, + ); + if (m) return m[1]!; + m = /(?:const|let|var)\s+(\w+)\s*=/.exec(line); + if (m) return m[1]!; + return ""; +} + +/** Find the param list body (between the first balanced `(` and `)`) in concatenated signature + * lines. Returns null when parens are unbalanced — we skip rather than guess. */ +function extractParamListBody(text: string): string | null { + const open = text.indexOf("("); + if (open === -1) return null; + let depth = 0; + for (let i = open; i < text.length; i++) { + const c = text[i]!; + if (c === "(") depth++; + else if (c === ")") { + depth--; + if (depth === 0) return text.slice(open + 1, i); + } + } + return null; +} + +// --- Diff reconstruction --- + +interface ReconLine { + content: string; + isAdded: boolean; + newLine: number; +} + +/** Reconstruct the new-file view from a unified diff patch, tagging `+` lines as `isAdded`. */ +export function reconstructLines(patch: string): ReconLine[] { + const result: ReconLine[] = []; + let lineNum = 0; + for (const raw of patch.split("\n")) { + if (raw.startsWith("---") || raw.startsWith("+++")) continue; + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); + if (hunk) { + lineNum = Number(hunk[1]); + continue; + } + if (raw.startsWith("+")) { + result.push({ content: raw.slice(1), isAdded: true, newLine: lineNum++ }); + } else if (!raw.startsWith("-")) { + // context line (starts with space, or empty in some diff tools) + result.push({ content: raw.slice(1), isAdded: false, newLine: lineNum++ }); + } + } + return result; +} + +// --- Core scanning --- + +/** Scan one file's patch for doc-comment drift. + * + * Strategy: reconstruct the new-file view from hunk lines, then slide a window over + * JSDoc-to-function pairs. When any line in the window is a `+` diff line and the JSDoc + * has at least one @param, compare the tags to the signature's param list. */ +export function scanPatchForDocDrift( + path: string, + patch: string, +): DocCommentFinding[] { + const findings: DocCommentFinding[] = []; + const lines = reconstructLines(patch); + const n = lines.length; + let i = 0; + + while (i < n && findings.length < MAX_FINDINGS) { + const content = lines[i]!.content; + if (content.length > MAX_LINE_CHARS) { + i++; + continue; + } + + if (!content.trimStart().startsWith("/**")) { + i++; + continue; + } + + // Collect the JSDoc block through the line that contains `*/`. + const docStart = i; + let docEnd = i; + if (!content.includes("*/")) { + docEnd++; + while (docEnd < n && !lines[docEnd]!.content.includes("*/")) docEnd++; + } + const docBlock = lines + .slice(docStart, docEnd + 1) + .map((l) => l.content) + .join("\n"); + + // Skip blank lines between JSDoc and the function definition. + let fnIdx = docEnd + 1; + while (fnIdx < n && lines[fnIdx]!.content.trim() === "") fnIdx++; + + if (fnIdx >= n || !looksLikeFunction(lines[fnIdx]!.content)) { + i = docEnd + 1; + continue; + } + + // Only flag when this PR actually touched something in the JSDoc-signature window. + const windowHasChange = lines + .slice(docStart, fnIdx + 1) + .some((l) => l.isAdded); + if (!windowHasChange) { + i = docEnd + 1; + continue; + } + + const docParams = extractJsDocParams(docBlock); + if (docParams.length === 0) { + i = docEnd + 1; + continue; + } + + // Look across up to 8 lines to handle multi-line signatures. + const sigText = lines + .slice(fnIdx, Math.min(fnIdx + 8, n)) + .map((l) => l.content) + .join(" "); + const paramListBody = extractParamListBody(sigText); + if (paramListBody === null) { + // Can't parse the param list — skip rather than produce a false positive. + i = docEnd + 1; + continue; + } + + const { params: fnParams, hasDestructured } = + extractFunctionParams(paramListBody); + if (hasDestructured) { + // JSDoc typically names the wrapper object (e.g. `@param options`), not the inner + // keys — a mismatch here is expected convention, not drift. + i = docEnd + 1; + continue; + } + + const fnName = extractFunctionName(lines[fnIdx]!.content); + const fnLine = lines[fnIdx]!.newLine; + const fnParamSet = new Set(fnParams); + const docParamSet = new Set(docParams); + + // Stale: @param in JSDoc for a param that no longer exists in the signature. + for (const dp of docParams) { + if (!fnParamSet.has(dp)) { + findings.push({ file: path, line: fnLine, fn: fnName, kind: "stale-param", param: dp }); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + + // Missing: param in signature with no corresponding @param in the JSDoc. + for (const fp of fnParams) { + if (!docParamSet.has(fp)) { + findings.push({ file: path, line: fnLine, fn: fnName, kind: "missing-param", param: fp }); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + + i = docEnd + 1; + } + + return findings; +} + +/** Analyzer entrypoint: scan every JS/TS file in the PR for doc-comment drift. */ +export async function scanDocComment( + req: EnrichRequest, +): Promise { + const findings: DocCommentFinding[] = []; + for (const file of req.files ?? []) { + if (!file.patch || !JS_TS_EXT.test(file.path)) continue; + for (const finding of scanPatchForDocDrift(file.path, file.patch)) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 8313780e6e..d20fd32955 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -14,11 +14,11 @@ import { scanInstallScripts } from "./analyzers/install-scripts.js"; import { scanActionPins } from "./analyzers/actions-pin.js"; import { scanEol } from "./analyzers/eol-check.js"; import { scanRedos } from "./analyzers/redos.js"; +import { scanDocComment } from "./analyzers/doc-comment.js"; import { renderBrief } from "./render.js"; type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise; -// The analyzer registry. More land behind this same shape: license (#1475), secret (#1476), static (#1477), history (#1478). const ANALYZERS: Record = { dependency: (req, signal) => scanDependencies(req, fetch, { signal }), secret: (req) => scanSecrets(req), @@ -27,6 +27,7 @@ const ANALYZERS: Record = { actionPin: (req) => scanActionPins(req), eol: (req) => scanEol(req), redos: (req) => scanRedos(req), + docComment: (req) => scanDocComment(req), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 848a2f67de..c4dd509763 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -129,6 +129,22 @@ export function renderBrief( } } + const docComments = findings.docComment ?? []; + if (docComments.length) { + lines.push( + "### Doc-comment drift (JSDoc @param mismatch — update the doc-comment before merging)", + ); + for (const item of docComments) { + const loc = safeCodeSpan(`${item.file}:${item.line}`); + const fn = safeCodeSpan(item.fn); + const msg = + item.kind === "stale-param" + ? `${safeCodeSpan(`@param ${item.param}`)} in JSDoc does not match any parameter of ${fn} — remove or rename` + : `${fn} has parameter ${safeCodeSpan(item.param)} with no @param in the JSDoc — add it`; + lines.push(`- ${loc} — ${msg}`); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index ae893825ca..1b9530784e 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -93,7 +93,17 @@ export interface RedosFinding { pattern: string; } -/** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ +/** A JSDoc/docstring whose @param tags don't match the adjacent function signature after the PR's changes — + * either a stale tag for a removed param, or a missing tag for an added param (#1519). */ +export interface DocCommentFinding { + file: string; + line: number; + fn: string; + kind: "stale-param" | "missing-param"; + param: string; +} + +/** Structured analyzer output. Each analyzer fills its own key. */ export interface BriefFindings { dependency?: DependencyFinding[]; secret?: SecretFinding[]; @@ -102,6 +112,7 @@ export interface BriefFindings { installScript?: InstallScriptFinding[]; eol?: EolFinding[]; redos?: RedosFinding[]; + docComment?: DocCommentFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 6f189d1143..914b8af560 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -21,6 +21,13 @@ import { scanPatchForRedos, scanRedos, } from "../dist/analyzers/redos.js"; +import { + extractJsDocParams, + extractFunctionParams, + reconstructLines, + scanPatchForDocDrift, + scanDocComment, +} from "../dist/analyzers/doc-comment.js"; const NOW = new Date("2026-06-26").getTime(); const eolFetch = @@ -711,6 +718,351 @@ test("buildBrief: ReDoS analyzer runs (pure, no network)", async () => { } }); +// --- doc-comment drift tests --- + +test("extractJsDocParams: bare, typed, [bracket], [bracket=default] forms; skips `this`", () => { + const block = [ + "/**", + " * @param name bare", + " * @param {string} typed", + " * @param {Type} [optBracket] optional", + " * @param {number} [withDefault=0] with default", + " * @param this TS pseudo-param — should be skipped", + " */", + ].join("\n"); + assert.deepEqual(extractJsDocParams(block), [ + "name", + "typed", + "optBracket", + "withDefault", + ]); +}); + +test("extractJsDocParams: no @param tags returns empty array", () => { + assert.deepEqual(extractJsDocParams("/** Does something. */"), []); +}); + +test("extractFunctionParams: simple, optional, default, rest params; skips `this`", () => { + const { params, hasDestructured } = extractFunctionParams( + "a: string, b?: number, c = 0, ...rest: string[]", + ); + assert.deepEqual(params, ["a", "b", "c", "rest"]); + assert.equal(hasDestructured, false); +}); + +test("extractFunctionParams: destructured object param sets hasDestructured", () => { + const { params, hasDestructured } = extractFunctionParams( + "{ name, age }: Person, extra: string", + ); + assert.equal(hasDestructured, true); + assert.deepEqual(params, ["extra"]); +}); + +test("extractFunctionParams: destructured array param sets hasDestructured", () => { + const { hasDestructured } = extractFunctionParams("[first, ...rest]: string[]"); + assert.equal(hasDestructured, true); +}); + +test("extractFunctionParams: skips TS `this` pseudo-param", () => { + const { params } = extractFunctionParams("this: Context, x: string"); + assert.deepEqual(params, ["x"]); +}); + +test("extractFunctionParams: empty param list", () => { + const { params, hasDestructured } = extractFunctionParams(""); + assert.deepEqual(params, []); + assert.equal(hasDestructured, false); +}); + +test("reconstructLines: tags added lines, skips removed, uses hunk header for line numbers", () => { + const patch = [ + "@@ -5,3 +5,4 @@", + " context", + "+added", + "-removed", + " another", + ].join("\n"); + const lines = reconstructLines(patch); + assert.equal(lines.length, 3); + assert.deepEqual(lines[0], { content: "context", isAdded: false, newLine: 5 }); + assert.deepEqual(lines[1], { content: "added", isAdded: true, newLine: 6 }); + assert.deepEqual(lines[2], { content: "another", isAdded: false, newLine: 7 }); +}); + +test("reconstructLines: skips --- and +++ header lines", () => { + const patch = [ + "--- a/src/x.ts", + "+++ b/src/x.ts", + "@@ -1,0 +1,1 @@", + "+const x = 1;", + ].join("\n"); + const lines = reconstructLines(patch); + assert.equal(lines.length, 1); + assert.equal(lines[0]!.isAdded, true); + assert.equal(lines[0]!.newLine, 1); +}); + +test("reconstructLines: multiple hunks reset line counter", () => { + const patch = [ + "@@ -1,1 +1,1 @@", + "+first", + "@@ -10,1 +10,1 @@", + "+second", + ].join("\n"); + const lines = reconstructLines(patch); + assert.equal(lines[0]!.newLine, 1); + assert.equal(lines[1]!.newLine, 10); +}); + +test("scanPatchForDocDrift: stale-param — @param removed from function but still in JSDoc", () => { + const patch = [ + "@@ -1,7 +1,6 @@", + " /**", + " * @param name the name", + " * @param age the age", + " */", + "-function greet(name: string, age: number): string {", + "+function greet(name: string): string {", + ].join("\n"); + const findings = scanPatchForDocDrift("src/x.ts", patch); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.kind, "stale-param"); + assert.equal(findings[0]!.param, "age"); + assert.equal(findings[0]!.fn, "greet"); + assert.equal(findings[0]!.file, "src/x.ts"); +}); + +test("scanPatchForDocDrift: missing-param — new param added to function with no @param in JSDoc", () => { + const patch = [ + "@@ -1,6 +1,7 @@", + " /**", + " * @param name the name", + " */", + "-function greet(name: string): string {", + "+function greet(name: string, email: string): string {", + ].join("\n"); + const findings = scanPatchForDocDrift("src/x.ts", patch); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.kind, "missing-param"); + assert.equal(findings[0]!.param, "email"); +}); + +test("scanPatchForDocDrift: no change in JSDoc-function window — pre-existing drift is not flagged", () => { + // The JSDoc and function sig are both context lines; only body lines inside the function are added. + const patch = [ + "@@ -1,7 +1,8 @@", + " /**", + " * @param name the name", + " * @param stale gone", + " */", + " function greet(name: string): string {", + "+ return name;", + " }", + ].join("\n"); + const findings = scanPatchForDocDrift("src/x.ts", patch); + assert.equal(findings.length, 0); +}); + +test("scanPatchForDocDrift: JSDoc with no @param tags — nothing to drift against", () => { + const patch = [ + "@@ -1,5 +1,5 @@", + "+/**", + "+ * Does something.", + "+ */", + "+function doSomething(x: string): void {", + ].join("\n"); + assert.equal(scanPatchForDocDrift("src/x.ts", patch).length, 0); +}); + +test("scanPatchForDocDrift: destructured params — skipped to avoid wrapper-name false positives", () => { + const patch = [ + "@@ -1,5 +1,5 @@", + "+/**", + "+ * @param options the options", + "+ */", + "+function configure({ host, port }: Options): void {", + ].join("\n"); + assert.equal(scanPatchForDocDrift("src/x.ts", patch).length, 0); +}); + +test("scanPatchForDocDrift: JSDoc not followed by a function — skipped", () => { + const patch = [ + "@@ -1,5 +1,5 @@", + "+/**", + "+ * @param foo the foo", + "+ */", + "+const obj = { key: 'value' };", + ].join("\n"); + assert.equal(scanPatchForDocDrift("src/x.ts", patch).length, 0); +}); + +test("scanPatchForDocDrift: single-line JSDoc (/** ... */ on one line)", () => { + const patch = [ + "@@ -1,3 +1,3 @@", + " /** @param name the name @param stale gone */", + "-function greet(name: string, stale: number): void {", + "+function greet(name: string): void {", + ].join("\n"); + const findings = scanPatchForDocDrift("src/x.ts", patch); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.kind, "stale-param"); + assert.equal(findings[0]!.param, "stale"); +}); + +test("scanPatchForDocDrift: class method with modifiers — name extracted correctly", () => { + const patch = [ + "@@ -1,7 +1,6 @@", + " /**", + " * @param id the id", + " * @param name the name", + " */", + "- public async update(id: string, name: string): Promise {", + "+ public async update(id: string): Promise {", + ].join("\n"); + const findings = scanPatchForDocDrift("src/svc.ts", patch); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.fn, "update"); + assert.equal(findings[0]!.kind, "stale-param"); + assert.equal(findings[0]!.param, "name"); +}); + +test("scanPatchForDocDrift: arrow function — name extracted from const binding", () => { + const patch = [ + "@@ -1,6 +1,7 @@", + " /**", + " * @param a first", + " */", + "-const add = (a: number): number => a;", + "+const add = (a: number, b: number): number => a + b;", + ].join("\n"); + const findings = scanPatchForDocDrift("src/x.ts", patch); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.fn, "add"); + assert.equal(findings[0]!.kind, "missing-param"); + assert.equal(findings[0]!.param, "b"); +}); + +test("scanPatchForDocDrift: rest param in function matches @param in JSDoc", () => { + const patch = [ + "@@ -1,5 +1,6 @@", + "+/**", + "+ * @param args the args", + "+ */", + "+function foo(...args: string[]): void {", + ].join("\n"); + // args is documented and present — no drift + assert.equal(scanPatchForDocDrift("src/x.ts", patch).length, 0); +}); + +test("scanPatchForDocDrift: unbalanced parens in signature — skipped", () => { + const patch = [ + "@@ -1,5 +1,5 @@", + "+/**", + "+ * @param x the x", + "+ */", + "+function broken(x: Map { + // Build a patch with 22 functions each with one stale param + const lines: string[] = ["@@ -1,1 +1,110 @@"]; + for (let k = 0; k < 22; k++) { + lines.push(`+/** @param stale gone */`); + lines.push(`+function fn${k}(real: string): void {}`); + } + const findings = scanPatchForDocDrift("src/x.ts", lines.join("\n")); + assert.equal(findings.length, 20); +}); + +test("scanDocComment: skips non-JS/TS files and files without patches", async () => { + const findings = await scanDocComment({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: "README.md", + patch: "@@ -1,0 +1,2 @@\n+/** @param foo */\n+function bar(baz: string) {}", + }, + { path: "src/a.ts" }, // no patch + { + path: "src/b.ts", + patch: [ + "@@ -1,5 +1,5 @@", + " /**", + " * @param name ok", + " * @param stale gone", + " */", + "+function f(name: string) {", + ].join("\n"), + }, + ], + }); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.file, "src/b.ts"); +}); + +test("scanDocComment: caps MAX_FINDINGS across multiple files", async () => { + const files = Array.from({ length: 25 }, (_, k) => ({ + path: `src/f${k}.ts`, + patch: `@@ -1,0 +1,2 @@\n+/** @param stale gone */\n+function fn(real: string): void {}`, + })); + const findings = await scanDocComment({ repoFullName: "o/r", prNumber: 1, files }); + assert.equal(findings.length, 20); +}); + +test("renderBrief: renders stale-param and missing-param doc-comment blocks", () => { + const r = renderBrief({ + docComment: [ + { file: "src/x.ts", line: 10, fn: "greet", kind: "stale-param", param: "age" }, + { file: "src/y.ts", line: 20, fn: "configure", kind: "missing-param", param: "timeout" }, + ], + }); + assert.match(r.promptSection, /Doc-comment drift/); + assert.match(r.promptSection, /`src\/x\.ts:10`/); + assert.match(r.promptSection, /`@param age`/); + assert.match(r.promptSection, /does not match any parameter of `greet`/); + assert.match(r.promptSection, /`src\/y\.ts:20`/); + assert.match(r.promptSection, /`configure` has parameter `timeout`/); + assert.match(r.promptSection, /no @param in the JSDoc/); +}); + +test("renderBrief: no doc-comment section when docComment is empty", () => { + assert.equal(renderBrief({ docComment: [] }).promptSection, ""); +}); + +test("buildBrief: doc-comment analyzer runs (pure, no network)", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async () => ({ ok: true, json: async () => ({}) }); + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { + path: "src/x.ts", + patch: [ + "@@ -1,5 +1,5 @@", + " /**", + " * @param name the name", + " * @param staleParam gone", + " */", + "+function greet(name: string): string {", + ].join("\n"), + }, + ], + }); + assert.equal(brief.analyzerStatus.docComment, "ok"); + assert.equal(brief.findings.docComment!.length, 1); + assert.equal(brief.findings.docComment![0]!.kind, "stale-param"); + assert.match(brief.promptSection, /Doc-comment drift/); + } finally { + globalThis.fetch = realFetch; + } +}); + test("extractDependencyChanges: caps manifest files and patch lines", () => { const changes = extractDependencyChanges( [ From ebb145fcbf0e205aebf5da544fdc3fb043d1d91c Mon Sep 17 00:00:00 2001 From: dale053 Date: Sun, 28 Jun 2026 00:09:37 -0400 Subject: [PATCH 2/3] fix(enrichment): close unclosed docComment if-block in renderBrief and remove orphaned JSDoc comment --- review-enrichment/src/render.ts | 3 +++ review-enrichment/src/types.ts | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index f4d7c3f0c5..0352ab128a 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -142,6 +142,9 @@ export function renderBrief( ? `${safeCodeSpan(`@param ${item.param}`)} in JSDoc does not match any parameter of ${fn} — remove or rename` : `${fn} has parameter ${safeCodeSpan(item.param)} with no @param in the JSDoc — add it`; lines.push(`- ${loc} — ${msg}`); + } + } + const codeownersViolations = findings.codeowners ?? []; if (codeownersViolations.length) { const allOwners = new Set(codeownersViolations.flatMap((f) => f.owners)); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index b795df261a..228e67b532 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -103,7 +103,6 @@ export interface DocCommentFinding { param: string; } -/** Structured analyzer output. Each analyzer fills its own key. */ /** A changed file governed by a CODEOWNERS rule where the PR author is not listed as an owner (#1515). * The blast radius (distinct ownership domains crossed) is derived at render time from the full findings set. */ export interface CodeownersFinding { From 883de5cf17975fd2c44972b36dacb7e5788708da Mon Sep 17 00:00:00 2001 From: dale053 Date: Sun, 28 Jun 2026 14:54:11 -0400 Subject: [PATCH 3/3] fix(enrichment): bound windowHasChange to the closing-paren line in doc-comment drift (#1519) --- .../src/analyzers/doc-comment.ts | 40 ++++++++++++++----- review-enrichment/test/enrichment.test.ts | 21 ++++++++++ 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/review-enrichment/src/analyzers/doc-comment.ts b/review-enrichment/src/analyzers/doc-comment.ts index a3c1bd8135..62d139b7e5 100644 --- a/review-enrichment/src/analyzers/doc-comment.ts +++ b/review-enrichment/src/analyzers/doc-comment.ts @@ -201,15 +201,6 @@ export function scanPatchForDocDrift( continue; } - // Only flag when this PR actually touched something in the JSDoc-signature window. - const windowHasChange = lines - .slice(docStart, fnIdx + 1) - .some((l) => l.isAdded); - if (!windowHasChange) { - i = docEnd + 1; - continue; - } - const docParams = extractJsDocParams(docBlock); if (docParams.length === 0) { i = docEnd + 1; @@ -217,10 +208,12 @@ export function scanPatchForDocDrift( } // Look across up to 8 lines to handle multi-line signatures. + const sigEnd = Math.min(fnIdx + 8, n); const sigText = lines - .slice(fnIdx, Math.min(fnIdx + 8, n)) + .slice(fnIdx, sigEnd) .map((l) => l.content) .join(" "); + const paramListBody = extractParamListBody(sigText); if (paramListBody === null) { // Can't parse the param list — skip rather than produce a false positive. @@ -228,6 +221,33 @@ export function scanPatchForDocDrift( continue; } + // Locate the line holding the closing paren of the param list so the change-gate window + // ends exactly at the signature and does not reach into the function body. + let sigCloseLineIdx = fnIdx; + { + let depth = 0; + let seenOpen = false; + outer: for (let li = fnIdx; li < sigEnd; li++) { + for (const ch of lines[li]!.content) { + if (ch === "(") { depth++; seenOpen = true; } + else if (ch === ")" && seenOpen) { + depth--; + if (depth === 0) { sigCloseLineIdx = li; break outer; } + } + } + } + } + + // Only flag when this PR actually touched the JSDoc-through-signature window. + // Bounded by the closing-paren line so body-only changes don't trigger drift checks. + const windowHasChange = lines + .slice(docStart, sigCloseLineIdx + 1) + .some((l) => l.isAdded); + if (!windowHasChange) { + i = docEnd + 1; + continue; + } + const { params: fnParams, hasDestructured } = extractFunctionParams(paramListBody); if (hasDestructured) { diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 6f6169fe2d..1695260d6f 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -28,6 +28,7 @@ import { scanPatchForDocDrift, scanDocComment, } from "../dist/analyzers/doc-comment.js"; +import { findOwners, parseCodeowners, patternToRegex, @@ -1008,6 +1009,26 @@ test("scanPatchForDocDrift: caps at MAX_FINDINGS across one file", () => { assert.equal(findings.length, 20); }); +test("scanPatchForDocDrift: change on a later line of a multi-line signature is detected", () => { + // Regression: windowHasChange must cover the full sig window, not just the first sig line. + // The JSDoc and first signature line are context; only a later param line is added. + const patch = [ + "@@ -1,6 +1,7 @@", + " /**", + " * @param a first param", + " */", + " function foo(", + "- a: string", + "+ a: string,", + "+ b: number", + " ) {}", + ].join("\n"); + const findings = scanPatchForDocDrift("src/x.ts", patch); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.kind, "missing-param"); + assert.equal(findings[0]!.param, "b"); +}); + test("scanDocComment: skips non-JS/TS files and files without patches", async () => { const findings = await scanDocComment({ repoFullName: "o/r",