diff --git a/.env.example b/.env.example index f2dd7d2edd..5ea3445a87 100644 --- a/.env.example +++ b/.env.example @@ -65,12 +65,12 @@ GITTENSORY_REVIEW_ENRICHMENT=false # Current analyzer names: # dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,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,magicNumber -# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,complexity -# unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep -# revertRecurrence,coverageDelta,callerImpact +# commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,duplicationDelta +# churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch +# commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology +# todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting +# errorSwallow,complexity,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint +# apiBreak,deprecatedDep,revertRecurrence,coverageDelta,callerImpact # # Profile defaults: # fast: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency @@ -81,19 +81,19 @@ GITTENSORY_REVIEW_ENRICHMENT=false # balanced (default): dependency,dependencyDiff,lockfileDrift,secret,license,installScript # heavyDependency,hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight # typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication +# duplicationDelta,churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport +# staleBranch,commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange +# terminology,todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise +# deepNesting,errorSwallow,complexity,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest +# commitLint,apiBreak,deprecatedDep,revertRecurrence,coverageDelta,callerImpact +# deep: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency +# hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat +# commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,duplicationDelta # churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch # commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology # todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting # errorSwallow,complexity,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint # apiBreak,deprecatedDep,revertRecurrence,coverageDelta,callerImpact -# deep: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency -# hardcodedUrl,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,magicNumber -# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,complexity -# unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep -# revertRecurrence,coverageDelta,callerImpact # 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 e0b8407c2c..664a23e1f1 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -582,6 +582,34 @@ export const REES_ANALYZERS = [ "Conservative: trivial/boilerplate lines are dropped and a long contiguous run is required, so incidental overlap is not flagged. Never returns code content.", }, }, + { + name: "duplicationDelta", + title: "Resolved duplication (before/after)", + category: "quality", + cost: "github-light", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token", "head-sha"], + limits: { + minRun: 8, + maxFiles: 20, + maxFindings: 25, + maxFetchBytes: 1000000, + maxBlocksPerFile: 150, + }, + docs: { + summary: + "Flags a duplicate block pair that existed in a changed file's pre-PR content and is no longer both present — a consolidation the no-checkout reviewer cannot see.", + looksAt: + "Each changed file's pre-PR content (reconstructed from its patch) compared against its own post-PR content, using the same chunk-normalization + suffix-automaton matcher as the `duplication` analyzer.", + reports: + "The file and the pre-PR locations of the resolved duplicate pair, plus the matched line count. Never file contents.", + network: + "Calls the GitHub API for each changed file's content at headSha. Requires headSha and token forwarding for private repos.", + notes: + "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a greedy (not globally optimal) old-to-new block assignment, which can rarely under-report a resolved pair as still-present in multi-candidate scenarios -- an acknowledged v1 heuristic limit, not a correctness/data-integrity issue.", + }, + }, { name: "churnHotspot", title: "Churn hotspots", diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 40ca05a34f..bc7b3ce29d 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -664,6 +664,36 @@ "notes": "Conservative: trivial/boilerplate lines are dropped and a long contiguous run is required, so incidental overlap is not flagged. Never returns code content." } }, + { + "name": "duplicationDelta", + "title": "Resolved duplication (before/after)", + "category": "quality", + "cost": "github-light", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token", + "head-sha" + ], + "limits": { + "minRun": 8, + "maxFiles": 20, + "maxFindings": 25, + "maxFetchBytes": 1000000, + "maxBlocksPerFile": 150 + }, + "docs": { + "summary": "Flags a duplicate block pair that existed in a changed file's pre-PR content and is no longer both present — a consolidation the no-checkout reviewer cannot see.", + "looksAt": "Each changed file's pre-PR content (reconstructed from its patch) compared against its own post-PR content, using the same chunk-normalization + suffix-automaton matcher as the `duplication` analyzer.", + "reports": "The file and the pre-PR locations of the resolved duplicate pair, plus the matched line count. Never file contents.", + "network": "Calls the GitHub API for each changed file's content at headSha. Requires headSha and token forwarding for private repos.", + "notes": "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a greedy (not globally optimal) old-to-new block assignment, which can rarely under-report a resolved pair as still-present in multi-candidate scenarios -- an acknowledged v1 heuristic limit, not a correctness/data-integrity issue." + } + }, { "name": "churnHotspot", "title": "Churn hotspots", diff --git a/review-enrichment/src/analyzers/duplication-delta.ts b/review-enrichment/src/analyzers/duplication-delta.ts new file mode 100644 index 0000000000..ba776406ed --- /dev/null +++ b/review-enrichment/src/analyzers/duplication-delta.ts @@ -0,0 +1,267 @@ +// Duplication-REMOVAL delta analyzer (#4741, part of epic #4737). The sibling `duplication-scan.ts` analyzer +// (confirmed by reading it, not assumed — see its own header + this file's PR description) only detects NEW +// duplication a PR ADDS: a contiguous run of added lines that near-verbatim-matches something ALREADY elsewhere in +// the repo at headSha. It has no notion of a changed file's OWN pre-PR state, so it cannot see the complementary +// signal: a PR CONSOLIDATING duplication that already existed (two near-identical blocks reduced to one). This +// analyzer closes that gap using the shared `reconstructOldContent` primitive (#4739) to recover each changed +// file's pre-PR text, then reuses `duplication-scan.ts`'s OWN chunk-normalization + suffix-automaton +// longest-shared-run matcher (same `MIN_RUN`, same `normalizeLine`/block-splitting) so "what counts as a +// duplicate" is identical between the add-detector and this remove-detector — never a second, differently-tuned +// similarity algorithm running side by side with the first. +// +// Scope (v1): PER FILE only. For each changed file, find pairs of near-identical blocks that existed in its +// RECONSTRUCTED OLD content, then GREEDILY ASSIGN each old block (in file order) to an unclaimed matching NEW +// block. That assignment step matters: if OLD had two near-identical copies of some text and NEW keeps exactly +// one, a naive "does this old block's text exist ANYWHERE in NEW" check would say BOTH old copies "survive" (the +// single remaining occurrence matches either query) and the resolved-duplication signal would never fire. Greedy +// assignment lets only as many old blocks "survive" as there are still-distinct matching occurrences in NEW; any +// old block left unclaimed — but that WAS part of an old duplicate pair — is the resolved-duplication finding. +// Cross-file duplication removal (the twin lived in a DIFFERENT file, changed or not) is NOT detected — +// deliberately out of scope for this version (see PR description for the follow-up), not silently approximated. +// +// Fail-safe throughout: [] on missing token/headSha, a bad repo slug, an aborted signal, or when a file's content +// can't be fetched. `reconstructOldContent` returning either null (unreconstructable patch) or "" (file did not +// exist before this PR) means "no usable before-content" and is checked via plain truthiness — never `=== null` — +// so a wholly-added file is never mistreated as having pre-existing duplication to report on. +import type { + AnalyzerDiagnostics, + DuplicationDeltaFinding, + EnrichRequest, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchText } from "../external-fetch.js"; +import { githubHeaders } from "../github-headers.js"; +import { reconstructOldContent } from "./reconstruct-old-content.js"; +import { DEFAULT_MAX_FINDINGS } from "./limits.js"; +import { + isSourceExt, + isExcludedPath, + normalizeFileBlocks, + buildMatchIndex, + longestSharedRun, + type NormBlock, + type MatchIndex, +} from "./duplication-scan.js"; + +const GITHUB_API = "https://api.github.com"; +// Requires an alphanumeric FIRST character (unlike a bare `[A-Za-z0-9._-]+`, which a segment of exactly ".." or +// "." would also satisfy — every char in ".." is individually allowed by that class). A leading-dot segment could +// let a URL parser's dot-segment resolution rewrite `/repos/../evil/contents/...` into an unintended path, sending +// the auth token somewhere other than the intended `owner/repo`. Mirrors codeowners.ts's stricter slug guard. +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; +const MAX_FILES = 20; // changed files probed per scan, mirroring doc-comment-drift's cap +const MAX_FINDINGS = DEFAULT_MAX_FINDINGS; +const MAX_FETCH_BYTES = 1_000_000; +// Defensive cap on the O(blocks^2) internal self-pairing pass: a file with more significant-line blocks than this is +// skipped for the self-duplication check rather than risking pathological scan time on a huge/generated file. +const MAX_BLOCKS_PER_FILE = 150; + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + +interface InternalPair { + /** Index into the OLD blocks array for each side of the pair (i < j, never equal). */ + i: number; + j: number; + /** 1-based OLD-content line where each side's matched run begins. */ + iLine: number; + jLine: number; + /** Contiguous significant lines that matched verbatim (whitespace-normalized). */ + length: number; +} + +/** Fetch a changed file's raw content at `headSha` through the shared bounded-text helper (with the analysis + * context's caching/metering when supplied, mirroring `codeowners.ts`). Returns null on any non-OK / oversized / + * network outcome so the caller fails safe. */ +async function fetchFileAtHead( + owner: string, + repo: string, + path: string, + headSha: string, + token: string, + fetchFn: typeof fetch, + options: ScanOptions, +): Promise { + const encoded = path.split("/").map(encodeURIComponent).join("/"); + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encoded}?ref=${encodeURIComponent(headSha)}`; + const fetchOptions = { + endpointCategory: "github-contents", + headers: githubHeaders(token, { raw: true }), + signal: options.signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "duplicationDelta", + subcall: "github-contents", + maxBytes: MAX_FETCH_BYTES, + maxCallsPerCategory: MAX_FILES, + }; + const response = options.analysis + ? await options.analysis.fetchText(url, fetchOptions) + : await boundedFetchText(url, fetchOptions); + return response.ok ? response.data : null; +} + +/** Find every pair of DIFFERENT blocks in `blocks` sharing a contiguous run of >= MIN_RUN significant lines — + * i.e. content that was internally near-duplicated within this one file's OLD (pre-PR) state. Builds one + * suffix-automaton index per block and reuses it for every other block, the same indexing strategy + * `duplication-scan.ts` uses per candidate file. Bailing to [] on an aborted signal (mid-build or mid-match) + * discards any partial pairing rather than reporting it as complete. */ +export function findInternalDuplicatePairs( + blocks: NormBlock[], + signal: AbortSignal | undefined, +): InternalPair[] { + const pairs: InternalPair[] = []; + if (blocks.length < 2 || blocks.length > MAX_BLOCKS_PER_FILE) return pairs; + + const indices = blocks.map((block) => buildMatchIndex(block, signal)); + for (let i = 0; i < blocks.length; i += 1) { + if (signal?.aborted) return []; + for (let j = i + 1; j < blocks.length; j += 1) { + const index = indices[j]; + if (!index) continue; // index build was aborted for this candidate — no usable index + const run = longestSharedRun(blocks[i]!, index, signal); + if (run?.status === "aborted") return []; + if (run?.status === "matched") { + pairs.push({ i, j, iLine: run.headLine, jLine: run.sourceLine, length: run.length }); + } + } + } + return pairs; +} + +/** Greedily assign each OLD block (in file order) to an UNCLAIMED matching NEW block (>= MIN_RUN significant + * lines), each NEW block usable by at most one OLD block. This is what lets duplicate COUNT reductions show up: + * if OLD had N near-identical copies of some text and NEW retains only M (M < N), exactly M of the N old blocks + * claim a surviving NEW occurrence and the remaining (N - M) do not — instead of every old copy independently + * matching the SAME still-present text and all appearing to "survive". Returns a parallel boolean array over + * `oldBlocks`. An aborted signal stops early; every OLD block not yet processed is left `false` ("not confirmed + * to survive") rather than risk reporting a stale/partial comparison as conclusive. + * + * KNOWN v1 LIMITATION: this is a greedy, order-dependent assignment, not a globally optimal bipartite matching. + * In a multi-candidate scenario where old blocks match NEW occurrences asymmetrically (e.g. old block A matches + * BOTH remaining new occurrences but old block B matches only one of them), first-come-first-claimed can let A + * grab the occurrence B needed, leaving B unmatched even though a different (optimal) assignment would have + * paired both. The practical effect is a false "resolved" report for a duplicate pair that is, in fact, still + * present — never a crash or a data-integrity issue, since this is an ADVISORY-ONLY signal (see epic #4737's + * design constraints) that never gates anything. A true maximum-bipartite-matching algorithm (e.g. an + * augmenting-path search) would close this gap; tracked as a separate, non-urgent follow-up rather than + * attempted here. */ +export function assignSurvivors( + oldBlocks: NormBlock[], + newIndices: MatchIndex[], + signal: AbortSignal | undefined, +): boolean[] { + const claimed = new Array(newIndices.length).fill(false); + const survived = new Array(oldBlocks.length).fill(false); + for (let i = 0; i < oldBlocks.length; i += 1) { + if (signal?.aborted) return survived; + for (let n = 0; n < newIndices.length; n += 1) { + if (claimed[n]) continue; + if (signal?.aborted) return survived; + const run = longestSharedRun(oldBlocks[i]!, newIndices[n]!, signal); + if (run?.status === "aborted") return survived; + if (run?.status === "matched") { + claimed[n] = true; + survived[i] = true; + break; + } + } + } + return survived; +} + +/** Analyzer entrypoint: for each changed file, recover its pre-PR content and flag a duplicate block pair that + * existed before the change and is no longer both recognizable after — the reverse of what `duplication-scan.ts` + * can see (it only compares ADDED lines against the rest of the repo at head, never a file against its own + * past). Reports per-file/per-block deltas (never a bare total) so a downstream aggregator can consume + * structured findings. Fail-safe: [] on missing token/headSha, a bad repo slug, an aborted signal, or when a + * file's content can't be fetched or its old content can't be reconstructed. */ +export async function scanDuplicationDelta( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const { repoFullName, githubToken, headSha, files = [] } = req; + if (!githubToken || !headSha) return []; + if (options.signal?.aborted) return []; + const parts = repoFullName.split("/"); + const owner = parts[0]; + const repo = parts[1]; + if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) { + return []; + } + + // Same file-eligibility filter as `duplication-scan.ts` (source-extension, not generated/vendored/minified/ + // declaration) so a file that could never produce an ADDED-duplication finding there is equally excluded here. + const candidates = files + .filter( + (f) => f.status !== "removed" && !!f.patch && isSourceExt(f.path) && !isExcludedPath(f.path), + ) + .slice(0, MAX_FILES); + if (!candidates.length) return []; + + const findings: DuplicationDeltaFinding[] = []; + + for (const file of candidates) { + if (options.signal?.aborted || findings.length >= MAX_FINDINGS) break; + + const headContent = await fetchFileAtHead( + owner, + repo, + file.path, + headSha, + githubToken, + fetchFn, + options, + ); + if (!headContent || options.signal?.aborted) continue; + + // Both null (unreconstructable patch) and "" (file did not exist before this PR) mean "no usable + // before-content" — a plain truthiness check, never `=== null` (see reconstructOldContent's own contract). + const oldContent = reconstructOldContent(headContent, file.patch!); + if (!oldContent) continue; + + const oldBlocks = normalizeFileBlocks(oldContent); + const pairs = findInternalDuplicatePairs(oldBlocks, options.signal); + if (!pairs.length || options.signal?.aborted) continue; + + const newIndices = normalizeFileBlocks(headContent) + .map((block) => buildMatchIndex(block, options.signal)) + .filter((index): index is MatchIndex => index !== null); + + const survived = assignSurvivors(oldBlocks, newIndices, options.signal); + if (options.signal?.aborted) continue; + + // Each OLD block that took part in at least one duplicate pair and did NOT claim a surviving NEW block is + // reported once — referencing the first pair partner encountered — even if it was part of several old + // duplicate pairs (a block with 3+ near-identical old siblings never produces more than one finding for it). + const reportedBlocks = new Set(); + for (const pair of pairs) { + if (findings.length >= MAX_FINDINGS) break; + if (!survived[pair.i] && !reportedBlocks.has(pair.i)) { + reportedBlocks.add(pair.i); + findings.push({ + file: file.path, + line: pair.iLine, + duplicateOfLine: pair.jLine, + lines: pair.length, + }); + } + if (findings.length >= MAX_FINDINGS) break; + if (!survived[pair.j] && !reportedBlocks.has(pair.j)) { + reportedBlocks.add(pair.j); + findings.push({ + file: file.path, + line: pair.jLine, + duplicateOfLine: pair.iLine, + lines: pair.length, + }); + } + } + } + + return findings.slice(0, MAX_FINDINGS); +} diff --git a/review-enrichment/src/analyzers/duplication-scan.ts b/review-enrichment/src/analyzers/duplication-scan.ts index 55289d21d5..627737db4d 100644 --- a/review-enrichment/src/analyzers/duplication-scan.ts +++ b/review-enrichment/src/analyzers/duplication-scan.ts @@ -8,6 +8,12 @@ // `source:path:line`) + the matched line count — never the code content. Fail-safe: returns [] without a token / // headSha, on a bad repoFullName, or when the tree fetch fails; a single malformed candidate is skipped, never // aborting the scan. +// +// The chunk-normalization + suffix-automaton matching primitives below (`isSourceExt`, `isExcludedPath`, +// `normalizeFileBlocks`, `buildMatchIndex`, `longestSharedRun`) are also exported for `duplication-delta.ts` (#4741, +// part of #4737), which detects the REVERSE signal — duplication a PR REMOVES/consolidates rather than introduces — +// by reusing this exact "what counts as a duplicate" definition against reconstructed pre-PR content instead of +// inventing a second, differently-tuned similarity algorithm. import type { AnalyzerDiagnostics, EnrichRequest, @@ -103,13 +109,13 @@ function extOf(path: string): string | null { return path.slice(dot + 1).toLowerCase(); } -function isSourceExt(path: string): boolean { +export function isSourceExt(path: string): boolean { const ext = extOf(path); return ext !== null && SOURCE_EXTS.has(ext); } /** Paths that are generated / vendored / minified / type-declaration: copy-paste there is noise, not a defect. */ -function isExcludedPath(path: string): boolean { +export function isExcludedPath(path: string): boolean { if (path.endsWith(".d.ts")) return true; if (path.includes(".min.")) return true; const lower = path.toLowerCase(); @@ -145,7 +151,7 @@ export function normalizeLine(raw: string): string | null { return collapsed; } -interface NormBlock { +export interface NormBlock { /** Normalized significant lines, in order. */ norm: string[]; /** Parallel array: the ORIGINAL new-file line number for each entry of `norm`. */ @@ -209,7 +215,7 @@ export function extractAddedBlocks(patch: string | undefined): NormBlock[] { /** Split a full file's text into blocks of significant lines (with 1-based source line numbers), breaking a block * at every blank/trivial line — the same gap-aware grouping `extractAddedBlocks` uses. Indexing each block * separately means a matched run can never bridge across a blank or trivial line in the source file. */ -function normalizeFileBlocks(text: string): NormBlock[] { +export function normalizeFileBlocks(text: string): NormBlock[] { const blocks: NormBlock[] = []; let current: NormBlock | null = null; const lines = text.split("\n"); @@ -228,26 +234,26 @@ function normalizeFileBlocks(text: string): NormBlock[] { return blocks; } -interface SuffixState { +export interface SuffixState { len: number; link: number; firstPos: number; next: Map; } -interface MatchIndex { +export interface MatchIndex { block: NormBlock; states: SuffixState[]; } -type SharedRunResult = +export type SharedRunResult = | { status: "matched"; headLine: number; sourceLine: number; length: number } | { status: "aborted" } | null; /** Build a suffix automaton over candidate significant lines, so longest-run lookup is exact and linear instead of * order-dependent on a capped list of repeated MIN_RUN-window starts. */ -function buildMatchIndex(block: NormBlock, signal?: AbortSignal): MatchIndex | null { +export function buildMatchIndex(block: NormBlock, signal?: AbortSignal): MatchIndex | null { const states: SuffixState[] = [ { len: 0, link: -1, firstPos: -1, next: new Map() }, ]; @@ -302,7 +308,7 @@ function buildMatchIndex(block: NormBlock, signal?: AbortSignal): MatchIndex | n /** Find the LONGEST contiguous run shared between an added block and an indexed candidate. Returns the head + source * line numbers of the run start and its length, or null when no run of >= MIN_RUN significant lines is shared. */ -function longestSharedRun( +export function longestSharedRun( added: NormBlock, index: MatchIndex, signal?: AbortSignal, diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index b70f10e9ac..5b96e77152 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -11,6 +11,7 @@ import { dependencyAnalyzer } from "./dependency/descriptor.js"; import { scanDependencyDiffInventory } from "./dependency-diff.js"; import { scanDocCommentDrift } from "./doc-comment-drift.js"; import { scanDuplication } from "./duplication-scan.js"; +import { scanDuplicationDelta } from "./duplication-delta.js"; import { scanEol } from "./eol-check.js"; import { scanHardcodedUrl } from "./hardcoded-url.js"; import { scanHeavyDependencies } from "./heavy-dependency.js"; @@ -518,6 +519,47 @@ export const ANALYZER_DESCRIPTORS = [ run: (req, { signal, analysis, diagnostics }) => scanDuplication(req, fetch, { signal, analysis, diagnostics }), }), + descriptor({ + name: "duplicationDelta", + title: "Resolved duplication (before/after)", + category: "quality", + cost: "github-light", + defaultEnabled: true, + requires: ["files", "github-token", "head-sha"], + limits: { + minRun: 8, + maxFiles: 20, + maxFindings: DEFAULT_MAX_FINDINGS, + maxFetchBytes: 1_000_000, + maxBlocksPerFile: 150, + }, + docs: { + summary: + "Flags a duplicate block pair that existed in a changed file's pre-PR content and is no longer both present — a consolidation the no-checkout reviewer cannot see.", + looksAt: + "Each changed file's pre-PR content (reconstructed from its patch) compared against its own post-PR content, using the same chunk-normalization + suffix-automaton matcher as the `duplication` analyzer.", + reports: + "The file and the pre-PR locations of the resolved duplicate pair, plus the matched line count. Never file contents.", + network: + "Calls the GitHub API for each changed file's content at headSha. Requires headSha and token forwarding for private repos.", + notes: + "Complements `duplication` (which flags NEW duplication introduced) with the reverse, before/after signal. Per-file only in this version: a duplicate pair split across two different files is not detected. Uses a greedy (not globally optimal) old-to-new block assignment, which can rarely under-report a resolved pair as still-present in multi-candidate scenarios -- an acknowledged v1 heuristic limit, not a correctness/data-integrity issue.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = [ + "### Resolved duplication (previously-duplicated code now consolidated or removed)", + ]; + for (const item of findings) { + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} was near-identical to ${helpers.safeCodeSpan(`${item.file}:${item.duplicateOfLine}`)} before this PR (~${item.lines} lines) — no longer both present`, + ); + } + return lines; + }, + run: (req, { signal, analysis, diagnostics }) => + scanDuplicationDelta(req, fetch, { signal, analysis, diagnostics }), + }), descriptor({ name: "churnHotspot", title: "Churn hotspots", diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index b450e75efa..01d4aabea5 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -463,6 +463,8 @@ export function renderBrief( } } + lines.push(...renderDescriptorSection("duplicationDelta", findings.duplicationDelta)); + const churnHotspots = findings.churnHotspot ?? []; if (churnHotspots.length) { lines.push( diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 983a8b2428..7c1d41e80f 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -696,6 +696,7 @@ export interface BriefFindings { history?: HistoryFinding[]; docCommentDrift?: DocCommentDriftFinding[]; duplication?: DuplicationFinding[]; + duplicationDelta?: DuplicationDeltaFinding[]; churnHotspot?: ChurnHotspotFinding[]; blameLink?: BlameLinkFinding[]; approvalIntegrity?: ApprovalIntegrityFinding[]; @@ -760,6 +761,24 @@ export interface DuplicationFinding { lines: number; } +/** The reverse of `DuplicationFinding`: a duplicate block PAIR that existed within a changed file's PRE-PR content + * and is no longer both present after — a consolidation/removal the no-checkout reviewer cannot see (it has no + * visibility into the file's past). Uses the shared `reconstructOldContent` primitive (#4739) to recover the + * pre-PR text, then the same chunk-normalization + suffix-automaton matcher `duplication-scan.ts` uses, so "what + * counts as a duplicate" stays identical between the add- and remove-detectors. Scope: per-file only — a pair + * split across two different files is not detected. Reports pre-PR locations + the matched line count only, + * never code content. (#4741) */ +export interface DuplicationDeltaFinding { + /** Path of the changed file whose pre-PR content held the now-resolved duplicate pair. */ + file: string; + /** Pre-PR (old-content) line where the duplicate copy that is NO LONGER present in the new content began. */ + line: number; + /** Pre-PR (old-content) line of the pair's other half. */ + duplicateOfLine: number; + /** Number of contiguous significant lines that matched verbatim pre-PR (after whitespace normalization). */ + lines: number; +} + export type AnalyzerStatus = "ok" | "degraded" | "skipped" | "capped" | "timeout"; /** Internal, public-safe analyzer diagnostics for Sentry. Never attach request bodies, diffs, tokens, or raw prompts. */ diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 1ccfec0b71..7efc171ae7 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -32,6 +32,7 @@ const EXPECTED_ANALYZERS = [ "history", "docCommentDrift", "duplication", + "duplicationDelta", "churnHotspot", "blameLink", "approvalIntegrity", diff --git a/review-enrichment/test/duplication-delta.test.ts b/review-enrichment/test/duplication-delta.test.ts new file mode 100644 index 0000000000..bd8dd2289e --- /dev/null +++ b/review-enrichment/test/duplication-delta.test.ts @@ -0,0 +1,531 @@ +// Units for the duplication-removal delta analyzer (#4741, part of epic #4737). Own file (not duplication-scan.test.ts) +// since this is a distinct analyzer, even though it reuses duplication-scan.ts's chunk-matching primitives. Runs +// against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + scanDuplicationDelta, + findInternalDuplicatePairs, + assignSurvivors, +} from "../dist/analyzers/duplication-delta.js"; +import { buildMatchIndex } from "../dist/analyzers/duplication-scan.js"; +import { renderBrief } from "../dist/render.js"; + +// ── fixtures ───────────────────────────────────────────────────────────────── + +// A run of exactly MIN_RUN (8) significant, non-trivial, non-import lines — long enough after normalization +// (>= 12 chars trimmed) to register as one contiguous significant-line block, short enough to stay readable. +const DUP_BLOCK = [ + "const baseAmountForMiner = minerStats.baseReward * minerStats.decayFactor", + "const clampedAmountValue = Math.min(minerStats.maxReward, Math.max(0, baseAmountForMiner))", + "const streakBonusApplied = minerStats.streakBonus * minerStats.consistencyFactor", + "const violationPenaltyApplied = minerStats.violationCount * minerStats.penaltyWeight", + "const settledRewardAmount = clampedAmountValue + streakBonusApplied - violationPenaltyApplied", + "const finalRewardRounded = Math.round(settledRewardAmount * 1000000) / 1000000", + "const safeFinalReward = Number.isFinite(finalRewardRounded) ? finalRewardRounded : 0", + "const persistedRewardForMiner = safeFinalReward", +]; +const HEADER = "const fileHeaderMarkerForFixture = 'v1'"; +const TRAILER = "const fileTrailerMarkerForFixture = 'end'"; + +// Build a synthetic (patch, oldContent, newContent) trio for a PURE CONTIGUOUS DELETION: oldLines = prefixLines + +// removedLines + suffixLines; newLines = prefixLines + suffixLines. reconstructOldContent only ever reads the +// hunk header's `+` (new-file) start number — never the `-` side — so a single-hunk, all-removed-lines patch +// anchored at prefixLines.length correctly reverse-applies regardless of what the (unread) old-side numbers say. +function buildDeletionFixture(prefixLines, removedLines, suffixLines) { + const newLines = [...prefixLines, ...suffixLines]; + const oldLines = [...prefixLines, ...removedLines, ...suffixLines]; + const hunkNewStart = prefixLines.length + 1; // 1-based + const body = removedLines.map((l) => `-${l}`).join("\n"); + return { + oldContent: oldLines.join("\n"), + newContent: newLines.join("\n"), + patch: `@@ -${hunkNewStart},${removedLines.length} +${hunkNewStart},0 @@\n${body}`, + }; +} + +const baseReq = (overrides = {}) => ({ + repoFullName: "o/r", + prNumber: 1, + headSha: "a".repeat(40), + githubToken: "tok", + files: [], + ...overrides, +}); + +// Mock fetch keyed by file path: returns 200 + body for a known path, 404 otherwise. `counter`, if given, records +// every requested URL so a test can assert how many (and which) content fetches were made. +function makeContentFetch(byPath, { counter } = {}) { + return async (url) => { + if (counter) counter.push(url); + for (const [path, body] of Object.entries(byPath)) { + const encoded = path.split("/").map(encodeURIComponent).join("/"); + if (url.includes(`/contents/${encoded}?`)) { + return new Response(body, { status: 200 }); + } + } + return new Response("not found", { status: 404 }); + }; +} + +// ── findInternalDuplicatePairs (unit) ─────────────────────────────────────── + +test("findInternalDuplicatePairs: fewer than 2 blocks yields no pairs", () => { + assert.deepEqual(findInternalDuplicatePairs([], undefined), []); + assert.deepEqual( + findInternalDuplicatePairs([{ norm: ["a single significant line here"], lineNos: [1] }], undefined), + [], + ); +}); + +test("findInternalDuplicatePairs: more blocks than the defensive cap (150) are skipped entirely", () => { + const blocks = Array.from({ length: 151 }, (_, i) => ({ + norm: [`unique significant filler line number ${i} for the cap test`], + lineNos: [i + 1], + })); + assert.deepEqual(findInternalDuplicatePairs(blocks, undefined), []); +}); + +test("findInternalDuplicatePairs: finds a matching pair with correct block indices, lines, and length", () => { + const blocks = [ + { norm: DUP_BLOCK, lineNos: [10, 11, 12, 13, 14, 15, 16, 17] }, // index 0 + { norm: ["a totally unrelated short filler line here"], lineNos: [30] }, // index 1 — no match + { norm: DUP_BLOCK, lineNos: [50, 51, 52, 53, 54, 55, 56, 57] }, // index 2 — duplicate of index 0 + ]; + const pairs = findInternalDuplicatePairs(blocks, undefined); + assert.equal(pairs.length, 1); + assert.equal(pairs[0].i, 0); + assert.equal(pairs[0].j, 2); + assert.equal(pairs[0].iLine, 10); + assert.equal(pairs[0].jLine, 50); + assert.equal(pairs[0].length, 8); +}); + +test("findInternalDuplicatePairs: an aborted signal discards any partial pairing (empty, not partial)", () => { + const blocks = [ + { norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }, + { norm: DUP_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }, + ]; + assert.deepEqual(findInternalDuplicatePairs(blocks, AbortSignal.abort()), []); +}); + +test("findInternalDuplicatePairs: a signal that flips aborted DURING an in-progress match discards the whole result, not a partial pairing", () => { + // The 4th read of `.aborted` lands inside longestSharedRun's own poll (reads 1-2: each block's index build; + // read 3: the outer loop's per-i check; read 4: longestSharedRun's first-line poll while comparing the only + // pair) — everything up to that point looked healthy, but a mid-match abort must still discard the result + // rather than publish whatever partial comparison had run so far. + let reads = 0; + const fakeSignal = { + get aborted() { + reads += 1; + return reads === 4; + }, + }; + const blocks = [ + { norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }, + { norm: DUP_BLOCK, lineNos: [20, 21, 22, 23, 24, 25, 26, 27] }, + ]; + assert.deepEqual(findInternalDuplicatePairs(blocks, fakeSignal), []); +}); + +test("findInternalDuplicatePairs: a candidate whose OWN index build failed is never matched AS a candidate, but still matches normally as a query", () => { + // buildMatchIndex polls `signal.aborted` once per block build (at the very first line of each block, since the + // poll interval mask always trips at index 0). A fake signal that reports aborted on ONLY the 3rd read — the + // check inside block-index-2's own index build — leaves every other read (the earlier/later index builds, the + // outer loop's per-i checks, and every longestSharedRun call's own poll) seeing "not aborted", so the function + // proceeds normally apart from block 2's index being unusable as a match TARGET. This mirrors + // duplication-scan.test.ts's own technique for exercising a mid-loop abort poll via a stateful signal getter. + let reads = 0; + const fakeSignal = { + get aborted() { + reads += 1; + return reads === 3; + }, + }; + const blocks = [ + { norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }, // index 0 + { norm: ["a totally unrelated short filler line here"], lineNos: [20] }, // index 1 + { norm: DUP_BLOCK, lineNos: [30, 31, 32, 33, 34, 35, 36, 37] }, // index 2 — its own index build "fails" + { norm: DUP_BLOCK, lineNos: [50, 51, 52, 53, 54, 55, 56, 57] }, // index 3 + ]; + const pairs = findInternalDuplicatePairs(blocks, fakeSignal); + assert.equal(pairs.length, 2); + // Block 2 never appears as the CANDIDATE (j) side — its index never built — but its own content is still a + // valid QUERY (i) side against another block's (successfully built) index. + assert.equal(pairs.some((p) => p.j === 2), false); + assert.ok(pairs.some((p) => p.i === 0 && p.j === 3)); + assert.ok(pairs.some((p) => p.i === 2 && p.j === 3)); +}); + +// ── assignSurvivors (unit) ─────────────────────────────────────────────────── + +test("assignSurvivors: two identical old blocks with only ONE matching new block — exactly one survives", () => { + // This is the exact scenario a naive per-block "does this text exist anywhere in NEW" check gets wrong: both old + // copies would independently "see" the single surviving occurrence and both would appear to survive. Greedy + // assignment must let only ONE claim it. + const oldBlocks = [ + { norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] }, + { norm: DUP_BLOCK, lineNos: [12, 13, 14, 15, 16, 17, 18, 19] }, + ]; + const newIndices = [buildMatchIndex({ norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] })]; + const survived = assignSurvivors(oldBlocks, newIndices, undefined); + assert.deepEqual(survived, [true, false]); +}); + +test("assignSurvivors: both old blocks survive when NEW still has two distinct matching occurrences", () => { + const oldBlocks = [ + { norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] }, + { norm: DUP_BLOCK, lineNos: [12, 13, 14, 15, 16, 17, 18, 19] }, + ]; + const newIndices = [ + buildMatchIndex({ norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] }), + buildMatchIndex({ norm: DUP_BLOCK, lineNos: [12, 13, 14, 15, 16, 17, 18, 19] }), + ]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, undefined), [true, true]); +}); + +test("assignSurvivors: neither old block survives when NEW has no matching occurrence at all", () => { + const oldBlocks = [ + { norm: DUP_BLOCK, lineNos: [3, 4, 5, 6, 7, 8, 9, 10] }, + { norm: DUP_BLOCK, lineNos: [12, 13, 14, 15, 16, 17, 18, 19] }, + ]; + const newIndices = [buildMatchIndex({ norm: ["something completely unrelated here"], lineNos: [1] })]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, undefined), [false, false]); +}); + +test("assignSurvivors: an empty newIndices array means nothing survives", () => { + const oldBlocks = [{ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }]; + assert.deepEqual(assignSurvivors(oldBlocks, [], undefined), [false]); +}); + +test("assignSurvivors: an already-aborted signal leaves every block unconfirmed (false)", () => { + const oldBlocks = [{ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] }]; + const newIndices = [buildMatchIndex({ norm: DUP_BLOCK, lineNos: [1, 2, 3, 4, 5, 6, 7, 8] })]; + assert.deepEqual(assignSurvivors(oldBlocks, newIndices, AbortSignal.abort()), [false]); +}); + +// ── scanDuplicationDelta: fail-safe guards ────────────────────────────────── + +test("scanDuplicationDelta: fails safe with no githubToken", async () => { + const out = await scanDuplicationDelta(baseReq({ githubToken: undefined }), async () => { + throw new Error("should not fetch"); + }); + assert.deepEqual(out, []); +}); + +test("scanDuplicationDelta: fails safe with no headSha", async () => { + const out = await scanDuplicationDelta(baseReq({ headSha: undefined }), async () => { + throw new Error("should not fetch"); + }); + assert.deepEqual(out, []); +}); + +test("scanDuplicationDelta: fails safe on an already-aborted signal without fetching", async () => { + const out = await scanDuplicationDelta( + baseReq({ files: [{ path: "src/a.ts", status: "modified", patch: "@@ -1,1 +1,1 @@\n-x\n+y" }] }), + async () => { + throw new Error("should not fetch"); + }, + { signal: AbortSignal.abort() }, + ); + assert.deepEqual(out, []); +}); + +test("scanDuplicationDelta: fails safe on a bad repoFullName", async () => { + // A non-empty, otherwise-qualifying files array, so this genuinely exercises the slug guard rather than + // short-circuiting on "no candidate files" first. A leading-dot owner segment (".." here) must be rejected — + // not just any non-alphanumeric character — since every character in ".." individually passes a bare + // `[A-Za-z0-9._-]+` class check (dots are allowed); only a first-character requirement catches it. + const req = baseReq({ + repoFullName: "../evil", + files: [{ path: "src/a.ts", status: "modified", patch: "@@ -1,1 +1,1 @@\n-x\n+y" }], + }); + const out = await scanDuplicationDelta(req, async () => { + throw new Error("should not fetch"); + }); + assert.deepEqual(out, []); +}); + +test("scanDuplicationDelta: fails safe on a repoFullName with the wrong number of segments", async () => { + const req = baseReq({ + repoFullName: "only-one-segment", + files: [{ path: "src/a.ts", status: "modified", patch: "@@ -1,1 +1,1 @@\n-x\n+y" }], + }); + const out = await scanDuplicationDelta(req, async () => { + throw new Error("should not fetch"); + }); + assert.deepEqual(out, []); +}); + +test("scanDuplicationDelta: no candidate files (removed status, missing patch, wrong extension, excluded path) → [] without fetching", async () => { + const req = baseReq({ + files: [ + { path: "src/gone.ts", status: "removed", patch: "@@ -1,1 +0,0 @@\n-x" }, + { path: "src/no-patch.ts", status: "modified" }, + { path: "README.md", status: "modified", patch: "@@ -1,1 +1,1 @@\n-a\n+b" }, + { path: "src/generated.d.ts", status: "modified", patch: "@@ -1,1 +1,1 @@\n-a\n+b" }, + ], + }); + const out = await scanDuplicationDelta(req, async () => { + throw new Error("should not fetch"); + }); + assert.deepEqual(out, []); +}); + +// ── scanDuplicationDelta: reconstructOldContent null vs "" (DISTINCT cases) ─ + +test("scanDuplicationDelta: reconstructOldContent returning null (unreconstructable patch) skips the file", async () => { + // Hunk anchored far past the head content's length — reconstructOldContent bails to null (mirrors its own + // "bails when a hunk starts beyond the head content's length" test). + const headContent = "const shortLine = 1\nconst anotherShortLine = 2"; + const req = baseReq({ + files: [{ path: "src/broken.ts", status: "modified", patch: "@@ -1,1 +9999,1 @@\n-x" }], + }); + const out = await scanDuplicationDelta(req, makeContentFetch({ "src/broken.ts": headContent })); + assert.deepEqual(out, []); +}); + +test('scanDuplicationDelta: reconstructOldContent returning "" (wholly new file) skips the file — distinct from null', async () => { + // A pure-addition patch (old range 0,0): the file did not exist before this PR. reconstructOldContent correctly + // returns "" here (not null) — both are falsy, but this exercises the OTHER falsy branch from the null case above. + const newFileLines = ["const brandNewLineOne = 1", "const brandNewLineTwo = 2", "const brandNewLineThree = 3"]; + const headContent = newFileLines.join("\n"); + const patch = `@@ -0,0 +1,${newFileLines.length} @@\n${newFileLines.map((l) => `+${l}`).join("\n")}`; + const req = baseReq({ + files: [{ path: "src/brand-new.ts", status: "added", patch }], + }); + const out = await scanDuplicationDelta(req, makeContentFetch({ "src/brand-new.ts": headContent })); + assert.deepEqual(out, []); +}); + +// ── scanDuplicationDelta: detection ───────────────────────────────────────── + +test("scanDuplicationDelta: a PR that consolidates two near-identical old blocks into one produces a resolved-duplication finding", async () => { + const prefixLines = [HEADER, "", ...DUP_BLOCK, ""]; + const removedLines = [...DUP_BLOCK, ""]; // the second (now-gone) copy, plus its trailing blank + const suffixLines = [TRAILER]; + const { oldContent, newContent, patch } = buildDeletionFixture(prefixLines, removedLines, suffixLines); + // Sanity-check the fixture plumbing itself before trusting the analyzer's output against it. + assert.equal(newContent.split("\n").length, prefixLines.length + suffixLines.length); + assert.equal(oldContent.split("\n").length, prefixLines.length + removedLines.length + suffixLines.length); + + const out = await scanDuplicationDelta( + baseReq({ files: [{ path: "src/rewards.ts", status: "modified", patch }] }), + makeContentFetch({ "src/rewards.ts": newContent }), + ); + assert.equal(out.length, 1); + assert.equal(out[0].file, "src/rewards.ts"); + assert.equal(out[0].lines, 8); + // The SECOND (removed) copy began at old-content line 12 (1 header + 1 blank + 8 lines + 1 blank + 1); + // the surviving copy it used to duplicate began at old-content line 3. + assert.equal(out[0].line, 12); + assert.equal(out[0].duplicateOfLine, 3); +}); + +test("scanDuplicationDelta: no finding when both old copies still survive in the new content (unresolved duplication)", async () => { + // Only the header text changes; BOTH copies of DUP_BLOCK are untouched in old and new — nothing was consolidated. + const OLD_HEADER = "const fileHeaderMarkerForFixture = 'old'"; + const NEW_HEADER = "const fileHeaderMarkerForFixture = 'new'"; + const oldLines = [OLD_HEADER, "", ...DUP_BLOCK, "", ...DUP_BLOCK, "", TRAILER]; + const newLines = [NEW_HEADER, "", ...DUP_BLOCK, "", ...DUP_BLOCK, "", TRAILER]; + const patch = `@@ -1,1 +1,1 @@\n-${OLD_HEADER}\n+${NEW_HEADER}`; + + const out = await scanDuplicationDelta( + baseReq({ files: [{ path: "src/rewards.ts", status: "modified", patch }] }), + makeContentFetch({ "src/rewards.ts": newLines.join("\n") }), + ); + assert.deepEqual(out, []); +}); + +test("scanDuplicationDelta: a duplicate pair where NEITHER old block survives is still reported (both sides gone)", async () => { + const REPLACEMENT = [ + "const totallyDifferentComputationHere = doSomethingElse(inputValue)", + "const anotherUnrelatedComputationLine = doAnotherThing(inputValue)", + ]; + const prefixLines = [HEADER, "", ...DUP_BLOCK, "", ...DUP_BLOCK, "", TRAILER]; + const newLines = [HEADER, "", ...REPLACEMENT, "", TRAILER]; + // Both DUP_BLOCK copies (and their surrounding blanks) are replaced by REPLACEMENT — a single hunk covering the + // whole middle section: context HEADER + blank, then remove both copies + their blanks, add REPLACEMENT + blank. + const patch = [ + "@@ -1,2 +1,2 @@", + ` ${HEADER}`, + " ", + ...DUP_BLOCK.map((l) => `-${l}`), + "-", + ...DUP_BLOCK.map((l) => `-${l}`), + "-", + ...REPLACEMENT.map((l) => `+${l}`), + "+", + ].join("\n"); + const oldContent = prefixLines.join("\n"); + const newContent = newLines.join("\n"); + // Sanity-check the fixture's intended pre-PR shape (21 lines: header, blank, 2x[8-line block + blank], trailer) + // before trusting the analyzer's output against it. + assert.equal(oldContent.split("\n").length, prefixLines.length); + + const out = await scanDuplicationDelta( + baseReq({ files: [{ path: "src/rewards.ts", status: "modified", patch }] }), + makeContentFetch({ "src/rewards.ts": newContent }), + ); + // Both copies are gone, so BOTH sides of the pair are reported (each references the other as its pre-PR + // duplicate partner). + assert.equal(out.length, 2); + assert.deepEqual( + out.map((f) => f.line).sort((a, b) => a - b), + [3, 12], + ); + for (const finding of out) { + assert.equal(finding.file, "src/rewards.ts"); + assert.equal(finding.lines, 8); + } +}); + +test("scanDuplicationDelta: three mutually-duplicated old blocks collapsed to one report each removed block exactly once", async () => { + const removedLines = [...DUP_BLOCK, "", ...DUP_BLOCK, ""]; // second and third copies removed + const { newContent, patch } = buildDeletionFixture( + [HEADER, "", ...DUP_BLOCK, ""], + removedLines, + [TRAILER], + ); + + const out = await scanDuplicationDelta( + baseReq({ files: [{ path: "src/rewards.ts", status: "modified", patch }] }), + makeContentFetch({ "src/rewards.ts": newContent }), + ); + // Exactly 2 findings (the two removed copies), never one finding per PAIR (which would be 3 for a 3-way mutual + // match: (1,2), (1,3), (2,3)) — the same OLD block is never reported twice. + assert.equal(out.length, 2); + const reportedLines = out.map((f) => f.line).sort((a, b) => a - b); + assert.deepEqual(reportedLines, [12, 21]); + for (const finding of out) { + assert.equal(finding.duplicateOfLine, 3); // both resolve against the surviving first copy + } +}); + +// ── scanDuplicationDelta: network fail-safety + bounding ──────────────────── + +test("scanDuplicationDelta: a file whose content fetch fails (404) is skipped; scan continues to the next file", async () => { + const prefixLines = [HEADER, "", ...DUP_BLOCK, ""]; + const removedLines = [...DUP_BLOCK, ""]; + const { newContent, patch } = buildDeletionFixture(prefixLines, removedLines, [TRAILER]); + + const req = baseReq({ + files: [ + { path: "src/missing.ts", status: "modified", patch: "@@ -1,1 +1,1 @@\n-a\n+b" }, + { path: "src/rewards.ts", status: "modified", patch }, + ], + }); + const out = await scanDuplicationDelta(req, makeContentFetch({ "src/rewards.ts": newContent })); + assert.equal(out.length, 1); + assert.equal(out[0].file, "src/rewards.ts"); +}); + +test("scanDuplicationDelta: a signal aborted right as the first file's fetch resolves skips that file AND stops before the next", async () => { + // Realistic, non-contrived cancellation: the caller's controller fires while our fetch was already in flight, + // landing right between the fetch resolving and the rest of that file's processing. The first file's (already + // truthy) content is discarded rather than analyzed, and the second file's fetch is never even attempted. + const controller = new AbortController(); + const okBody = "const shortHeadContentLineOne = 1\nconst shortHeadContentLineTwo = 2"; + let fetchCalls = 0; + const fetchImpl = async () => { + fetchCalls += 1; + controller.abort(); // simulate the cancellation landing exactly as this fetch completes + return new Response(okBody, { status: 200 }); + }; + const req = baseReq({ + files: [ + { path: "src/first.ts", status: "modified", patch: "@@ -1,1 +1,1 @@\n-x\n+y" }, + { path: "src/second.ts", status: "modified", patch: "@@ -1,1 +1,1 @@\n-x\n+y" }, + ], + }); + const out = await scanDuplicationDelta(req, fetchImpl, { signal: controller.signal }); + assert.deepEqual(out, []); + assert.equal(fetchCalls, 1); // the second file's fetch never happens — the per-file loop's abort check stops it +}); + +test("scanDuplicationDelta: the per-pair loop stops immediately after an i-side push reaches MAX_FINDINGS, never checking that pair's j-side", async () => { + // 13 independent two-block "neither side survives" groups (each internally duplicated, but distinguished from + // every other group by a unique first line so groups never cross-match). Each group normally contributes 2 + // findings (i-side then j-side); the 13th group's i-side push is the 25th finding overall, so its OWN j-side — + // which would otherwise also qualify — must never be reached. + const pairBlock = (groupIndex) => [ + `const uniqueGroupMarkerForFindingsCapTest${groupIndex} = computeMarkerValue(${groupIndex})`, + ...DUP_BLOCK.slice(1), + ]; + const groups = 13; + const removedLines = Array.from({ length: groups }, (_, k) => { + const block = pairBlock(k); + return [...block, "", ...block, ""]; // two copies of this group's block, each followed by a blank + }).flat(); + const prefixLines = [HEADER, ""]; + const { newContent, patch } = buildDeletionFixture(prefixLines, removedLines, [TRAILER]); + + const out = await scanDuplicationDelta( + baseReq({ files: [{ path: "src/many-groups.ts", status: "modified", patch }] }), + makeContentFetch({ "src/many-groups.ts": newContent }), + ); + assert.equal(out.length, 25); +}); + +test("scanDuplicationDelta: uses the analysis-context fetchText when supplied, instead of the bare fetch path", async () => { + const prefixLines = [HEADER, "", ...DUP_BLOCK, ""]; + const removedLines = [...DUP_BLOCK, ""]; + const { newContent, patch } = buildDeletionFixture(prefixLines, removedLines, [TRAILER]); + + let analysisCalls = 0; + const analysis = { + fetchText: async (_url, _opts) => { + analysisCalls += 1; + return { ok: true, status: 200, data: newContent, bytes: newContent.length, elapsedMs: 0, endpointCategory: "github-contents" }; + }, + }; + const req = baseReq({ files: [{ path: "src/rewards.ts", status: "modified", patch }] }); + const out = await scanDuplicationDelta( + req, + async () => { + throw new Error("bare fetch should not be used when analysis.fetchText is supplied"); + }, + { analysis }, + ); + assert.equal(analysisCalls, 1); + assert.equal(out.length, 1); +}); + +test("scanDuplicationDelta: caps total findings at the shared DEFAULT_MAX_FINDINGS (25)", async () => { + // 27 mutually-identical old copies of DUP_BLOCK, only the first survives → 26 raw resolved-duplication + // candidates, capped at 25. + const copies = 27; + const removedLines = Array.from({ length: copies - 1 }, () => [...DUP_BLOCK, ""]).flat(); // all but the first copy + const prefixLines = [HEADER, "", ...DUP_BLOCK, ""]; + const { newContent, patch } = buildDeletionFixture(prefixLines, removedLines, [TRAILER]); + + const out = await scanDuplicationDelta( + baseReq({ files: [{ path: "src/many-dupes.ts", status: "modified", patch }] }), + makeContentFetch({ "src/many-dupes.ts": newContent }), + ); + assert.equal(out.length, 25); +}); + +// ── render ─────────────────────────────────────────────────────────────────── + +test("renderBrief emits a public-safe resolved-duplication block with file:line, escaping paths, never the code", () => { + const { promptSection } = renderBrief({ + duplicationDelta: [{ file: "src/rewards.ts", line: 12, duplicateOfLine: 3, lines: 8 }], + }); + assert.match(promptSection, /Resolved duplication/); + assert.match(promptSection, /`src\/rewards\.ts:12`/); + assert.match(promptSection, /`src\/rewards\.ts:3`/); + assert.match(promptSection, /~8 lines/); + assert.ok(!promptSection.includes("baseAmountForMiner")); // no code content ever leaks into the rendered brief +}); + +test("renderBrief omits the resolved-duplication section when there are no findings", () => { + assert.equal(renderBrief({ duplicationDelta: [] }).promptSection, ""); +}); + +test("renderBrief escapes a backtick in a duplicationDelta path (no code-span breakout)", () => { + const { promptSection } = renderBrief({ + duplicationDelta: [{ file: "src/we`ird.ts", line: 1, duplicateOfLine: 2, lines: 8 }], + }); + assert.ok(!promptSection.includes("we`ird")); +});