diff --git a/.env.example b/.env.example index 00868b138f..0b3f2fa660 100644 --- a/.env.example +++ b/.env.example @@ -67,22 +67,22 @@ GITTENSORY_REVIEW_ENRICHMENT=false # provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild # history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals # undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety -# looseRange,terminology +# looseRange,terminology,todoMarker # # Profile defaults: # fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild,testRatio,migrationSafety -# looseRange,terminology +# looseRange,terminology,todoMarker # balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency # actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature # iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink # approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene -# pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology +# pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker # deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol # redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig # nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity # ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio -# migrationSafety,looseRange,terminology +# migrationSafety,looseRange,terminology,todoMarker # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index 140937ec5f..e987e59a3a 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -819,6 +819,29 @@ export const REES_ANALYZERS = [ "Token-based matching avoids substring false positives (masterclass/postmaster are never flagged), and URLs are skipped. The term→suggestion table is a bounded in-file policy.", }, }, + { + name: "todoMarker", + title: "Incomplete-work markers", + category: "quality", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + maxLineChars: 2000, + maxNoteChars: 120, + }, + docs: { + summary: + "Surfaces TODO/FIXME/HACK/XXX markers a PR adds in comments, so a reviewer sees the change is shipping known-incomplete work.", + looksAt: "Added lines with an uppercase, comment-anchored marker across any changed file.", + reports: "File, line, tag, and a truncated note.", + network: "Pure local analyzer. No external network call.", + notes: + "Precision-first: only UPPERCASE, comment-anchored markers are reported (a lowercase `todo` identifier or a marker inside a string literal is never flagged); a bare marker inside a multi-line block comment is intentionally not matched.", + }, + }, ] as const satisfies readonly ReesAnalyzerDoc[]; export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index b4f964e94b..f9ac48fd8c 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -920,6 +920,33 @@ "network": "Pure local analyzer. No external network call.", "notes": "Token-based matching avoids substring false positives (masterclass/postmaster are never flagged), and URLs are skipped. The term→suggestion table is a bounded in-file policy." } + }, + { + "name": "todoMarker", + "title": "Incomplete-work markers", + "category": "quality", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25, + "maxLineChars": 2000, + "maxNoteChars": 120 + }, + "docs": { + "summary": "Surfaces TODO/FIXME/HACK/XXX markers a PR adds in comments, so a reviewer sees the change is shipping known-incomplete work.", + "looksAt": "Added lines with an uppercase, comment-anchored marker across any changed file.", + "reports": "File, line, tag, and a truncated note.", + "network": "Pure local analyzer. No external network call.", + "notes": "Precision-first: only UPPERCASE, comment-anchored markers are reported (a lowercase `todo` identifier or a marker inside a string literal is never flagged); a bare marker inside a multi-line block comment is intentionally not matched." + } } ] } diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index b0c971b0c8..f8725729fe 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -28,6 +28,7 @@ import { scanTestRatio } from "./test-ratio.js"; import { scanMigrationSafety } from "./migration-safety.js"; import { scanLooseRanges } from "./loose-range.js"; import { scanTerminology } from "./terminology.js"; +import { scanTodoMarker } from "./todo-marker.js"; import { scanTyposquat } from "./typosquat.js"; import { scanUndocumentedExport } from "./undocumented-export.js"; import type { @@ -824,6 +825,36 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req, { signal }) => scanTerminology(req, signal), }), + descriptor({ + name: "todoMarker", + title: "Incomplete-work markers", + category: "quality", + cost: "local", + defaultEnabled: true, + requires: ["files"], + limits: { maxFindings: 25, maxLineChars: 2000, maxNoteChars: 120 }, + docs: { + summary: + "Surfaces TODO/FIXME/HACK/XXX markers a PR adds in comments, so a reviewer sees the change is shipping known-incomplete work.", + looksAt: "Added lines with an uppercase, comment-anchored marker across any changed file.", + reports: "File, line, tag, and a truncated note.", + network: "Pure local analyzer. No external network call.", + notes: + "Precision-first: only UPPERCASE, comment-anchored markers are reported (a lowercase `todo` identifier or a marker inside a string literal is never flagged); a bare marker inside a multi-line block comment is intentionally not matched.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = ["### Incomplete-work markers (TODO/FIXME/HACK/XXX added by this PR)"]; + for (const item of findings) { + const note = item.note ? `: ${item.note}` : ""; + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} — ${helpers.safeCodeSpan(item.tag)}${helpers.promptText(note)}`, + ); + } + return lines; + }, + run: (req, { signal }) => scanTodoMarker(req, signal), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/analyzers/todo-marker.ts b/review-enrichment/src/analyzers/todo-marker.ts new file mode 100644 index 0000000000..63a394ec91 --- /dev/null +++ b/review-enrichment/src/analyzers/todo-marker.ts @@ -0,0 +1,113 @@ +// TODO/FIXME/HACK/XXX marker tracker (#2016). Surfaces known-incomplete-work markers a PR ADDS, with the tag, +// the file:line, and a truncated note — so a reviewer sees the change is shipping acknowledged-incomplete work. +// Pure compute over added lines, no network. Stateless per line — there is no multi-line comment/string state to +// track. Precision-first, false-negative-biased: a marker is reported ONLY when (a) it is UPPERCASE (the marker +// convention — a lowercase `todo` identifier is never flagged) AND (b) it is COMMENT-ANCHORED, i.e. immediately +// preceded on the same line by a comment lead-in (`//`, `#`, `/*`, `*`, `)\s*$/, "").trim(); + return trimmed.length > MAX_NOTE_CHARS ? trimmed.slice(0, MAX_NOTE_CHARS) : trimmed; +} + +/** Detect a comment-anchored uppercase marker on one line. Returns the tag + optional note, or null. Pure. */ +export function detectTodoMarker( + line: string, +): { tag: TodoMarkerFinding["tag"]; note?: string } | null { + const match = MARKER_RE.exec(codeOnly(line)); + if (!match) return null; + const tag = match[1] as TodoMarkerFinding["tag"]; + const note = cleanNote(match[2] ?? ""); + return note ? { tag, note } : { tag }; +} + +type ScanLimits = { + maxFindings?: number; + signal?: AbortSignal; +}; + +/** Scan one file patch's added lines for comment-anchored markers, line-cited via hunk headers. Pure. */ +export function scanPatchForTodoMarker( + path: string, + patch: string, + limits: ScanLimits = {}, +): TodoMarkerFinding[] { + const maxFindings = limits.maxFindings ?? MAX_FINDINGS; + if (maxFindings <= 0) return []; + const findings: TodoMarkerFinding[] = []; + let newLine = 0; + let inHunk = false; + for (const line of patch.split("\n")) { + if (limits.signal?.aborted) throw new Error("analyzer_aborted"); + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + inHunk = true; + continue; + } + // Skip pre-hunk preamble; inside a hunk `+++x`/`+++ x` is added content, not a header. + if (!inHunk) continue; + if (line.startsWith("+")) { + const body = line.slice(1); + if (body.length <= MAX_LINE_CHARS) { + const hit = detectTodoMarker(body); + if (hit) { + findings.push( + hit.note + ? { file: path, line: newLine, tag: hit.tag, note: hit.note } + : { file: path, line: newLine, tag: hit.tag }, + ); + if (findings.length >= maxFindings) return findings; + } + } + newLine++; + } else if (!line.startsWith("-") && !line.startsWith("\\")) { + // A `\ No newline at end of file` marker is not a new-file line — do not advance the cursor + // (same class as the actions-pin / secret-log fix). + newLine++; + } + } + return findings; +} + +/** Analyzer entrypoint: scan every changed file's added lines for newly-added incomplete-work markers. */ +export async function scanTodoMarker( + req: EnrichRequest, + signal?: AbortSignal, +): Promise { + const findings: TodoMarkerFinding[] = []; + for (const file of req.files ?? []) { + if (signal?.aborted) throw new Error("analyzer_aborted"); + if (!file.patch) continue; + for (const finding of scanPatchForTodoMarker(file.path, file.patch, { + maxFindings: MAX_FINDINGS - findings.length, + signal, + })) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index b5ba6462b9..3173f73aad 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -457,6 +457,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("migrationSafety", findings.migrationSafety)); lines.push(...renderDescriptorSection("looseRange", findings.looseRange)); lines.push(...renderDescriptorSection("terminology", findings.terminology)); + lines.push(...renderDescriptorSection("todoMarker", findings.todoMarker)); if (!lines.length) return { promptSection: "", systemSuffix: "" }; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 8edd2c5701..34a082923b 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -429,6 +429,15 @@ export interface TerminologyFinding { suggestion: string; } +/** A TODO/FIXME/HACK/XXX marker a PR added in a comment — known-incomplete work shipping in the change + * (#2016, part of #1499). Reports the location, the tag, and an optional truncated note. */ +export interface TodoMarkerFinding { + file: string; + line: number; + tag: "TODO" | "FIXME" | "HACK" | "XXX"; + note?: string; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -463,6 +472,7 @@ export interface BriefFindings { migrationSafety?: MigrationSafetyFinding[]; looseRange?: LooseRangeFinding[]; terminology?: TerminologyFinding[]; + todoMarker?: TodoMarkerFinding[]; } /** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 27404d4b72..56dc8d5b6b 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -42,6 +42,7 @@ const EXPECTED_ANALYZERS = [ "migrationSafety", "looseRange", "terminology", + "todoMarker", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/todo-marker.test.ts b/review-enrichment/test/todo-marker.test.ts new file mode 100644 index 0000000000..af7e98fa8f --- /dev/null +++ b/review-enrichment/test/todo-marker.test.ts @@ -0,0 +1,123 @@ +// Units for the TODO/FIXME/HACK/XXX marker analyzer (#2016). Own file (not enrichment.test.ts) so concurrent +// analyzer PRs don't collide. No network — pure, stateless per-line detection. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + detectTodoMarker, + scanPatchForTodoMarker, + scanTodoMarker, +} from "../dist/analyzers/todo-marker.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines) => `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("detectTodoMarker: recognizes each tag with its note across comment styles", () => { + assert.deepEqual(detectTodoMarker(" // TODO: wire up retries"), { tag: "TODO", note: "wire up retries" }); + assert.deepEqual(detectTodoMarker(" # FIXME handle nulls"), { tag: "FIXME", note: "handle nulls" }); + assert.deepEqual(detectTodoMarker("x(); /* HACK: temporary */"), { tag: "HACK", note: "temporary" }); + assert.deepEqual(detectTodoMarker(" * XXX revisit this"), { tag: "XXX", note: "revisit this" }); + assert.deepEqual(detectTodoMarker(""), { tag: "TODO", note: "update docs" }); +}); + +test("detectTodoMarker: a bare marker with no note has no note field", () => { + assert.deepEqual(detectTodoMarker("// TODO"), { tag: "TODO" }); + assert.deepEqual(detectTodoMarker(" // FIXME "), { tag: "FIXME" }); +}); + +test("detectTodoMarker: only UPPERCASE tags match — a lowercase identifier is not a marker", () => { + assert.equal(detectTodoMarker("const todoList = []"), null); + assert.equal(detectTodoMarker("// fixme later"), null); // lowercase in a comment is not the marker convention + assert.equal(detectTodoMarker("this.#todo = 1"), null); +}); + +test("detectTodoMarker: must be comment-anchored — an uppercase tag in code is not flagged", () => { + assert.equal(detectTodoMarker("const TODO = getPending()"), null); + assert.equal(detectTodoMarker("return base * TODO"), null); // a `*` mid-expression is not a JSDoc lead + assert.equal(detectTodoMarker("arr.push(TODO)"), null); +}); + +test("detectTodoMarker: a marker inside a string literal is not flagged", () => { + assert.equal(detectTodoMarker('const s = "// TODO: not real"'), null); + assert.equal(detectTodoMarker("log(`hint: // FIXME here`)"), null); +}); + +test("detectTodoMarker: TODOS / XXXL are not markers (word boundary after the tag)", () => { + assert.equal(detectTodoMarker("// TODOS: plural list"), null); + assert.equal(detectTodoMarker("// XXXL size chart"), null); +}); + +test("detectTodoMarker: a trailing block-comment/HTML close is stripped from the note", () => { + assert.deepEqual(detectTodoMarker("/* TODO: fix the leak */"), { tag: "TODO", note: "fix the leak" }); + assert.deepEqual(detectTodoMarker(""), { tag: "FIXME", note: "broken link" }); +}); + +test("detectTodoMarker: an overlong note is truncated to the cap", () => { + const long = "x".repeat(300); + const hit = detectTodoMarker(`// TODO: ${long}`); + assert.equal(hit.tag, "TODO"); + assert.equal(hit.note.length, 120); +}); + +test("scanPatchForTodoMarker: flags markers on added lines with correct locations", () => { + const findings = scanPatchForTodoMarker( + "src/net.ts", + patchOf(["function f() {", " // TODO: retries", " return g();", " // HACK sleep to avoid a race", "}"]), + ); + assert.deepEqual(findings, [ + { file: "src/net.ts", line: 2, tag: "TODO", note: "retries" }, + { file: "src/net.ts", line: 4, tag: "HACK", note: "sleep to avoid a race" }, + ]); +}); + +test("scanPatchForTodoMarker: only ADDED lines are scanned; new-file line numbers stay correct", () => { + const patch = [ + "@@ -10,2 +10,2 @@", + " function f() {", // context line 10 + "- // TODO: old note", // removed, does not advance + "+ // TODO: new note", // new-file line 11 + ].join("\n"); + assert.deepEqual(scanPatchForTodoMarker("src/a.ts", patch), [ + { file: "src/a.ts", line: 11, tag: "TODO", note: "new note" }, + ]); +}); + +test("scanPatchForTodoMarker: enforces the maxFindings cap", () => { + const lines = Array.from({ length: 30 }, (_, i) => `// TODO: item ${i}`); + const findings = scanPatchForTodoMarker("src/a.ts", patchOf(lines), { maxFindings: 5 }); + assert.equal(findings.length, 5); + assert.deepEqual( + scanPatchForTodoMarker("src/a.ts", patchOf(lines), { maxFindings: 0 }), + [], + ); +}); + +test("scanTodoMarker: scans every changed file and honors the global cap", async () => { + const todoLines = Array.from({ length: 30 }, (_, i) => `// TODO: ${i}`); + const findings = await scanTodoMarker({ + repoFullName: "octo/repo", + prNumber: 1, + files: [ + { path: "src/a.ts", patch: patchOf(["const x = 1;"]) }, + { path: "src/b.ts", patch: patchOf(todoLines) }, + ], + }); + assert.equal(findings.length, 25); + assert.ok(findings.every((f) => f.file === "src/b.ts")); +}); + +test("scanTodoMarker: no files yields no findings", async () => { + assert.deepEqual(await scanTodoMarker({ repoFullName: "octo/repo", prNumber: 1 }), []); +}); + +test("renderBrief: todo-marker findings render tag, location, and note", () => { + const { promptSection } = renderBrief({ + todoMarker: [ + { file: "src/net.ts", line: 2, tag: "TODO", note: "wire up retries" }, + { file: "src/net.ts", line: 9, tag: "HACK" }, + ], + }); + assert.match(promptSection, /Incomplete-work markers/); + assert.match(promptSection, /src\/net\.ts:2/); + assert.match(promptSection, /TODO/); + assert.match(promptSection, /wire up retries/); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 6e771991c1..20991c6e54 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -36,6 +36,7 @@ export const REES_ANALYZER_NAMES = [ "migrationSafety", "looseRange", "terminology", + "todoMarker", ] as const; export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number]; diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index 903c13bb57..71ea8a995f 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -626,7 +626,7 @@ describe("resolveReesAnalyzers", () => { resolveReesAnalyzers( env({ REES_ANALYZERS: - "dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology", + "dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker", }), ), ).toEqual([ @@ -662,6 +662,7 @@ describe("resolveReesAnalyzers", () => { "migrationSafety", "looseRange", "terminology", + "todoMarker", ]); });