feat(enrichment): doc-comment vs signature drift analyzer (#1519)

review-enrichment/src/analyzers/doc-comment.ts

@@ -0,0 +1,300 @@
+// 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 `<anonymous>`). */
+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 "<anonymous>";
+}
+
+/** 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;
+    }
+
+    const docParams = extractJsDocParams(docBlock);
+    if (docParams.length === 0) {
+      i = docEnd + 1;
+      continue;
+    }
+
+    // Look across up to 8 lines to handle multi-line signatures.
+    const sigEnd = Math.min(fnIdx + 8, n);
+    const sigText = lines
+      .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.
+      i = docEnd + 1;
+      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) {
+      // 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<DocCommentFinding[]> {
+  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;
+}

review-enrichment/src/brief.ts

@@ -14,13 +14,13 @@ 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 { scanCodeowners } from "./analyzers/codeowners.js";
 import { scanSecretLog } from "./analyzers/secret-log.js";
 import { renderBrief } from "./render.js";
 
 type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise<unknown>;
 
-// The analyzer registry. More land behind this same shape: license (#1475), secret (#1476), static (#1477), history (#1478).
 const ANALYZERS: Record<keyof BriefFindings, AnalyzerFn> = {
   dependency: (req, signal) => scanDependencies(req, fetch, { signal }),
   secret: (req) => scanSecrets(req),
@@ -29,6 +29,7 @@ const ANALYZERS: Record<keyof BriefFindings, AnalyzerFn> = {
   actionPin: (req) => scanActionPins(req),
   eol: (req) => scanEol(req),
   redos: (req) => scanRedos(req),
+  docComment: (req) => scanDocComment(req),
   codeowners: (req, signal) => scanCodeowners(req, fetch, { signal }),
   secretLog: (req, signal) => scanSecretLog(req, signal),
 };

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}`);
+    }
+  }
+
   const codeownersViolations = findings.codeowners ?? [];
   if (codeownersViolations.length) {
     const allOwners = new Set(codeownersViolations.flatMap((f) => f.owners));

review-enrichment/src/types.ts

@@ -93,6 +93,16 @@ export interface RedosFinding {
   pattern: string;
 }
 
+/** 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;
+}
+
 /** 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 {
@@ -118,6 +128,7 @@ export interface BriefFindings {
   installScript?: InstallScriptFinding[];
   eol?: EolFinding[];
   redos?: RedosFinding[];
+  docComment?: DocCommentFinding[];
   codeowners?: CodeownersFinding[];
   secretLog?: SecretLogFinding[];
 }