From c411cfe58a4e7c7a612acccf391745fd3d609165 Mon Sep 17 00:00:00 2001 From: JSONbored Date: Wed, 29 Jul 2026 02:38:48 -0700 Subject: [PATCH 1/2] fix(ci): normalize committed CRLF to LF and name the failure for what it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twenty files were committed with CRLF and the repo had no .gitattributes. The `changes` job runs `git diff --check`, and git's default core.whitespace is `blank-at-eol` with `cr-at-eol` off, so a carriage return at end-of-line counts as trailing whitespace. Any PR adding a line to one of those files failed CI pointing at lines whose visible content is spotless, with nothing in the log saying "CRLF" — it hit #9787 (run 30436384202) on two lines of inline-suggestion-anchor.ts. Convert all twenty to LF via `git add --renormalize`, byte-identical otherwise (`git diff -w` reports only the two new files below), and pin the repo with `* text=auto eol=lf`. `text=auto` rather than a bare `text` is load-bearing: five .ts files embed NUL bytes as untrusted-text and sanitizer fixtures, and auto-detection classifies them binary and passes them through untouched. The whitespace step now names CRLF before falling through to the generic check. .gitattributes stops CRLF entering through `git add`, but checkin filters don't run on blobs written directly (web editor, Contents API) or on a merge of a branch cut earlier, so the diagnostic still has a job. It is scoped to added lines, so it fires on exactly what `git diff --check` already flagged — a relabel, not a new failure mode. Closes #9798 --- .gitattributes | 28 + .github/workflows/ci.yml | 22 + .../src/signals/issue-quality-report.ts | 626 ++++++------- .../src/analyzers/binary-extensions.ts | 208 ++--- .../test/binary-extensions.test.ts | 154 ++-- review-enrichment/test/codeowners.test.ts | 134 +-- review-enrichment/test/debug-leftover.test.ts | 190 ++-- .../test/dependency-diff.test.ts | 208 ++--- .../test/floating-promise.test.ts | 170 ++-- review-enrichment/test/hardcoded-url.test.ts | 176 ++-- .../test/i18n-regression.test.ts | 154 ++-- review-enrichment/test/size-smell.test.ts | 158 ++-- src/review/changed-files-diff-link.ts | 48 +- src/review/inline-comment-label.ts | 34 +- src/review/inline-comment-range.ts | 72 +- src/review/inline-suggestion-anchor.ts | 132 +-- test/unit/changed-files-diff-link.test.ts | 62 +- test/unit/inline-comment-label.test.ts | 62 +- test/unit/inline-comment-range.test.ts | 132 +-- test/unit/inline-comments-select.test.ts | 286 +++--- test/unit/inline-suggestion-anchor.test.ts | 198 ++-- .../unit/issue-quality-report-package.test.ts | 850 +++++++++--------- 22 files changed, 2077 insertions(+), 2027 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000000..9d1e2adaa4 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,28 @@ +# Line endings are LF, in the repository and in every working tree. +# +# Without this file, a contributor on a CRLF platform (or an editor that preserved a file's existing +# CRLF convention) could commit CRLF, and the `changes` CI job's `git diff --check` would then report +# every added line as "trailing whitespace" -- git's default core.whitespace is `blank-at-eol` with +# `cr-at-eol` OFF, so a carriage return at end-of-line IS trailing whitespace to it. The failure names +# lines whose visible content is spotless, which is undiagnosable from the CI log alone (#9798). +# +# `text=auto` (not a bare `text`) is deliberate: it leaves git's binary detection in charge, so files +# that embed NUL bytes on purpose -- the untrusted-text and sanitizer fixtures under src/, test/, +# scripts/ and review-enrichment/ -- are classified binary and pass through byte-for-byte. Forcing +# `* text eol=lf` would strip them of that protection and corrupt those fixtures. +* text=auto eol=lf + +# Belt-and-braces for formats where a line-ending rewrite would corrupt the file outright. `text=auto` +# above already spares them via NUL detection; these entries mean a format that happens to start with +# printable bytes never depends on that heuristic. +*.png binary +*.ico binary +*.jpg binary +*.jpeg binary +*.gif binary +*.webp binary +*.woff binary +*.woff2 binary +*.pdf binary +*.zip binary +*.gz binary diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f865988618..d6ebea4ea1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -83,6 +83,28 @@ jobs: exit 0 fi git fetch --depth=1 origin "$BASE_SHA" + # `git diff --check` reports a carriage return at end-of-line as "trailing whitespace": + # git's default core.whitespace is `blank-at-eol` with `cr-at-eol` OFF, so a CR really is + # trailing whitespace to it. The result names lines whose visible content is spotless, and + # nothing in the log says "CRLF" -- undiagnosable without cloning the branch and hexdumping + # the file (#9798). Name the real cause first, then fall through to the generic check for + # everything else. .gitattributes (`* text=auto eol=lf`) normalizes CRLF away on `git add`, so + # this can no longer be reached by an ordinary commit -- but checkin filters don't run on blobs + # written directly (the GitHub web editor, the Contents API) or on a merge of a branch cut + # before .gitattributes landed, and those still carry CRLF straight into the diff. Scoped to + # ADDED lines so it fires on exactly what `git diff --check` would have flagged anyway -- this + # relabels the failure, it does not fail anything new. + crlf=$(git diff "$BASE_SHA" HEAD | awk ' + /^\+\+\+ / { f = $2; sub(/^b\//, "", f); next } + /^\+/ { if ($0 ~ /\r$/) n[f]++ } + END { for (k in n) printf " %s (%d added line(s) ending in CRLF)\n", k, n[k] } + ') + if [ -n "$crlf" ]; then + echo "::error::CRLF line endings in added lines -- this repo is LF-only (see .gitattributes)" + printf '%s\n' "$crlf" + echo "Fix: git add --renormalize && git commit --amend" + exit 1 + fi git diff --check "$BASE_SHA" HEAD - name: Filter changed paths id: filter diff --git a/packages/loopover-engine/src/signals/issue-quality-report.ts b/packages/loopover-engine/src/signals/issue-quality-report.ts index 52df0b1253..7044b6ca15 100644 --- a/packages/loopover-engine/src/signals/issue-quality-report.ts +++ b/packages/loopover-engine/src/signals/issue-quality-report.ts @@ -1,313 +1,313 @@ -/** - * Package-local `buildIssueQualityReport` (#6057). - * - * Canonical scoring lives in the host signals engine (`signals/engine.ts`), which is intentionally - * excluded from this package's `tsc` emit (host-bound imports). Re-exporting that file from the public - * barrel breaks `npm run build` inside `@loopover/engine` — see closed #6139. - * - * This module is the portable twin of that function, matching the same call signature and status rules, - * built only on package-local types/helpers the way `buildCollisionReport` already is. - */ - -import type { - BountyLifecycle, - BountyRecord, - CollisionCluster, - CollisionReport, - IssueQualityReport, - IssueRecord, - LaneAdvice, - PullRequestRecord, - RecentMergedPullRequestRecord, - RepositoryRecord, -} from "../types/predicted-gate-types.js"; -import { nowIso } from "../utils/json.js"; -import { - bountyIssueKey, - buildCollisionReport, - buildLaneAdvice, - classifyBountyLifecycle, - indexBountiesByIssue, -} from "./predicted-gate-engine.js"; - -const ISSUE_QUALITY_REPORT_CAP = 100; -const ISSUE_DISCOVERY_LIFECYCLE_REPORT_CAP = 300; - -const MAINTAINER_WIP_LABELS = new Set([ - "wip", - "work in progress", - "work-in-progress", - "in progress", - "in-progress", - "blocked", - "on hold", - "on-hold", - "draft", - "do not work", - "do-not-work", - "internal", -]); - -type IssueDiscoveryLifecycleState = - | "open" - | "closed_not_solved" - | "solved" - | "valid_solved" - | "stale" - | "duplicate" - | "invalid"; - -type LifecycleEntry = { - number: number; - title: string; - state: IssueDiscoveryLifecycleState; - solvedByPullRequests: number[]; - reasons: string[]; -}; - -function clamp(value: number, min: number, max: number): number { - return Math.max(min, Math.min(max, value)); -} - -function daysSince(value: string | null | undefined): number { - if (!value) return 0; - const parsed = Date.parse(value); - /* v8 ignore next -- Invalid timestamps normalize to fresh. */ - if (!Number.isFinite(parsed)) return 0; - return Math.floor((Date.now() - parsed) / 86_400_000); -} - -function isMaintainerAssociation(value: string | null | undefined): boolean { - return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"; -} - -function sameLogin(value: string | null | undefined, login: string): boolean { - return value?.toLowerCase() === login.toLowerCase(); -} - -function isMaintainerWipIssue(issue: IssueRecord): boolean { - return isMaintainerAssociation(issue.authorAssociation) && issue.labels.some((label) => MAINTAINER_WIP_LABELS.has(label.toLowerCase().trim())); -} - -function indexPullRequestsByLinkedIssue(pullRequests: T[]): Map { - const byIssue = new Map(); - for (const pr of pullRequests) { - for (const issueNumber of new Set(pr.linkedIssues)) { - const bucket = byIssue.get(issueNumber); - if (bucket) bucket.push(pr); - else byIssue.set(issueNumber, [pr]); - } - } - return byIssue; -} - -function indexCollisionClustersByIssue(clusters: CollisionCluster[]): Map { - const byIssue = new Map(); - for (const cluster of clusters) { - const issueNumbers = new Set(); - for (const item of cluster.items) if (item.type === "issue") issueNumbers.add(item.number); - for (const issueNumber of issueNumbers) { - const bucket = byIssue.get(issueNumber); - if (bucket) bucket.push(cluster); - else byIssue.set(issueNumber, [cluster]); - } - } - return byIssue; -} - -function resolveLinkedPullRequests( - issue: IssueRecord, - pullRequests: T[], - byLinkedIssue: Map, - byNumber: Map, -): T[] { - const linkingPrs = byLinkedIssue.get(issue.number) ?? []; - let addedBackReference = false; - const matchedNumbers = new Set(linkingPrs.map((pr) => pr.number)); - for (const prNumber of issue.linkedPrs) { - if (byNumber.has(prNumber) && !matchedNumbers.has(prNumber)) { - matchedNumbers.add(prNumber); - addedBackReference = true; - } - } - if (!addedBackReference) return [...linkingPrs]; - return pullRequests.filter((pr) => matchedNumbers.has(pr.number)); -} - -function classifyIssueDiscoveryLifecycle( - issue: IssueRecord, - pullRequests: PullRequestRecord[], - recentMergedPullRequests: RecentMergedPullRequestRecord[], - lane: LaneAdvice, - linkedIndex?: { open: Map; merged: Map }, -): LifecycleEntry { - // With a prebuilt index (the per-repo lifecycle report) look up this issue's linked PRs in O(1); ad-hoc - // single-issue callers pass no index and fall back to the original filter. buildIssueQualityReport always - // supplies an index, so the filter fallbacks are defensive mirrors of the host engine. - /* v8 ignore next 3 -- Ad-hoc no-index path is unused by this module's only caller. */ - const linkedOpenPrs = linkedIndex ? (linkedIndex.open.get(issue.number) ?? []) : pullRequests.filter((pr) => pr.linkedIssues.includes(issue.number)); - /* v8 ignore next 3 -- Ad-hoc no-index path is unused by this module's only caller. */ - const linkedMergedPrs = linkedIndex - ? (linkedIndex.merged.get(issue.number) ?? []) - : recentMergedPullRequests.filter((pr) => pr.linkedIssues.includes(issue.number)); - const mergedSolverPrs = [...linkedOpenPrs.filter((pr) => pr.mergedAt || pr.state === "merged"), ...linkedMergedPrs]; - const solvedByPullRequests = [...new Set(mergedSolverPrs.map((pr) => pr.number))].sort((left, right) => left - right); - const issueAuthorLogin = issue.authorLogin; - const selfSolvedLoop = Boolean( - issueAuthorLogin && mergedSolverPrs.length > 0 && mergedSolverPrs.every((pr) => sameLogin(pr.authorLogin, issueAuthorLogin)), - ); - const labels = issue.labels.map((label) => label.toLowerCase()); - const stale = daysSince(issue.updatedAt ?? issue.createdAt) > 90; - const duplicate = labels.some((label) => /duplicate/.test(label)); - const invalid = labels.some((label) => /invalid|wontfix|not planned|won't fix/.test(label)); - const state: IssueDiscoveryLifecycleState = duplicate - ? "duplicate" - : invalid - ? "invalid" - : solvedByPullRequests.length > 0 - ? (lane.lane === "issue_discovery" || lane.lane === "split") && !selfSolvedLoop - ? "valid_solved" - : "solved" - : issue.state !== "open" - ? "closed_not_solved" - : stale - ? "stale" - : "open"; - const reasons = [ - ...(duplicate ? ["Issue carries duplicate labeling."] : []), - ...(invalid ? ["Issue carries invalid or not-planned labeling."] : []), - ...(solvedByPullRequests.length > 0 ? [`Linked solver PR(s): ${solvedByPullRequests.map((number) => `#${number}`).join(", ")}.`] : []), - ...(selfSolvedLoop ? ["Linked solver PR author matches the issue reporter; cache treats this as solved but not valid issue-discovery evidence."] : []), - ...(issue.state !== "open" && solvedByPullRequests.length === 0 ? ["Issue is closed without cached solver PR evidence."] : []), - ...(stale && issue.state === "open" ? ["Issue is stale in cached metadata."] : []), - ...(lane.lane === "direct_pr" ? ["Repo is direct-PR first; lifecycle should not encourage issue filing."] : []), - ]; - return { - number: issue.number, - title: issue.title, - state, - solvedByPullRequests, - reasons: reasons.length > 0 ? reasons : ["Issue is open with no solver or duplicate signal."], - }; -} - -function buildLifecycleByIssue( - repo: RepositoryRecord | null, - issues: IssueRecord[], - pullRequests: PullRequestRecord[], - fullName: string, - recentMergedPullRequests: RecentMergedPullRequestRecord[], -): Map { - const lane = buildLaneAdvice(repo, fullName); - const linkedIndex = { - open: indexPullRequestsByLinkedIssue(pullRequests), - merged: indexPullRequestsByLinkedIssue(recentMergedPullRequests), - }; - // Bulk classify the first CAP entries (host engine does the same), then PIN every remaining - // open issue that reports will evaluate — otherwise an open issue past the slice would be missing - // from the map while buildIssueQualityReport still iterates it (#6057 / closed #6141). - const cappedIssues = issues.slice(0, ISSUE_DISCOVERY_LIFECYCLE_REPORT_CAP); - const cappedNumbers = new Set(cappedIssues.map((issue) => issue.number)); - const pinnedOpenOutsideCap = issues.filter((issue) => issue.state === "open" && !cappedNumbers.has(issue.number)); - const issuesToClassify = pinnedOpenOutsideCap.length > 0 ? [...cappedIssues, ...pinnedOpenOutsideCap] : cappedIssues; - return new Map( - issuesToClassify.map((issue) => [issue.number, classifyIssueDiscoveryLifecycle(issue, pullRequests, recentMergedPullRequests, lane, linkedIndex)]), - ); -} - -/** - * Evaluate open issues for contribution readiness. - * - * Call signature matches the host engine: - * `(repo, issues, pullRequests, fullName, bounties?, prebuiltCollisions?, recentMergedPullRequests?)`. - */ -export function buildIssueQualityReport( - repo: RepositoryRecord | null, - issues: IssueRecord[], - pullRequests: PullRequestRecord[], - fullName: string, - bounties: BountyRecord[] = [], - prebuiltCollisions?: CollisionReport, - recentMergedPullRequests: RecentMergedPullRequestRecord[] = [], -): IssueQualityReport { - const lane = buildLaneAdvice(repo, fullName); - const collisions = prebuiltCollisions ?? buildCollisionReport(fullName, issues, pullRequests, recentMergedPullRequests); - const bountyByIssue = indexBountiesByIssue(bounties); - const prsByLinkedIssue = indexPullRequestsByLinkedIssue(pullRequests); - const prByNumber = new Map(pullRequests.map((pr) => [pr.number, pr] as const)); - const mergedPrsByLinkedIssue = indexPullRequestsByLinkedIssue(recentMergedPullRequests); - const mergedPrByNumber = new Map(recentMergedPullRequests.map((pr) => [pr.number, pr] as const)); - const clustersByIssue = indexCollisionClustersByIssue(collisions.clusters); - const lifecycleByIssue = buildLifecycleByIssue(repo, issues, pullRequests, fullName, recentMergedPullRequests); - const reports = issues - .filter((issue) => issue.state === "open") - .map((issue) => { - const linkedPrs = resolveLinkedPullRequests(issue, pullRequests, prsByLinkedIssue, prByNumber); - const linkedMergedPrs = resolveLinkedPullRequests(issue, recentMergedPullRequests, mergedPrsByLinkedIssue, mergedPrByNumber); - const issueCollisions = clustersByIssue.get(issue.number) ?? []; - /* v8 ignore next -- Missing dates normalize to zero age. */ - const age = daysSince(issue.updatedAt ?? issue.createdAt); - /* v8 ignore next -- Defensive host-mirror fallback; open issues past the CAP are pinned into the map above. */ - const lifecycle = lifecycleByIssue.get(issue.number)?.state ?? "open"; - const bodyLength = issue.body?.trim().length ?? 0; - const bounty = bountyByIssue.get(bountyIssueKey(fullName, issue.number)) ?? null; - const bountyLifecycle: BountyLifecycle | null = bounty ? classifyBountyLifecycle(bounty, issue) : null; - const linkedWorkCount = linkedPrs.length + linkedMergedPrs.length + issue.linkedPrs.length; - const maintainerAuthored = isMaintainerAssociation(issue.authorAssociation); - const maintainerWip = isMaintainerWipIssue(issue); - const reasons = [ - ...(bodyLength >= 200 ? ["Issue has enough body detail to evaluate."] : []), - ...(issue.labels.length > 0 ? [`Labels: ${issue.labels.join(", ")}.`] : []), - ...(linkedWorkCount === 0 ? ["No active PR is linked in cached metadata."] : []), - ...(bountyLifecycle === "active" ? ["Active bounty context is attached (contribution context, not guaranteed payout)."] : []), - ]; - const warnings = [ - ...(bodyLength < 80 ? ["Issue body is thin; contributor may need more proof before acting."] : []), - ...(linkedPrs.length > 0 ? [`${linkedPrs.length} active PR(s) already reference this issue.`] : []), - ...(linkedMergedPrs.length > 0 ? [`${linkedMergedPrs.length} merged PR(s) already reference this issue.`] : []), - ...(issue.linkedPrs.length > 0 && linkedPrs.length === 0 && linkedMergedPrs.length === 0 - ? [`Cached issue metadata already references PR(s): ${issue.linkedPrs.map((number) => `#${number}`).join(", ")}.`] - : []), - ...(issueCollisions.length > 0 ? ["Potential duplicate or overlapping issue/PR context exists."] : []), - ...(age > 90 ? ["Issue is stale in cached metadata."] : []), - ...(lifecycle !== "open" ? [`Issue lifecycle is ${lifecycle.replace(/_/g, " ")}.`] : []), - ...(lane.lane === "direct_pr" ? ["Repo is direct-PR first; issue filing is not the primary Gittensor lane."] : []), - ...(bountyLifecycle === "completed" ? ["A completed bounty is attached; the work is likely already solved, not an open opportunity."] : []), - ...(bountyLifecycle === "cancelled" ? ["A cancelled bounty is attached; this is not an active opportunity."] : []), - ...(bountyLifecycle === "historical" - ? ["Historical bounty context is attached; this is not an active opportunity without upstream confirmation."] - : []), - ...(bountyLifecycle === "stale" ? ["Bounty context for this issue looks stale; confirm it is still active before acting."] : []), - ...(bountyLifecycle === "ambiguous" ? ["Bounty state for this issue is ambiguous; verify it before acting."] : []), - ...(maintainerAuthored && !maintainerWip ? ["Maintainer-authored; confirm it is open for outside contribution before starting."] : []), - ...(maintainerWip - ? ["Maintainer-authored and labelled in-progress/internal; not a recommended outside-contributor target without confirmation."] - : []), - ]; - const score = clamp(100 - warnings.length * 18 + reasons.length * 5 - (age > 180 ? 15 : 0), 0, 100); - const bountyBlocks = bountyLifecycle === "completed" || bountyLifecycle === "cancelled" || bountyLifecycle === "historical"; - const bountyCaution = bountyLifecycle === "stale" || bountyLifecycle === "ambiguous"; - // Note: the host engine also has a `score < 45 → hold` arm, but with the current warning vocabulary - // that arm is unreachable without first matching needs_proof/do_not_use (only two warnings can fire - // without flipping those statuses). Ready is therefore observationally identical for reachable inputs. - const status: IssueQualityReport["issues"][number]["status"] = - linkedWorkCount > 0 || - issueCollisions.some((cluster) => cluster.risk === "high") || - bountyBlocks || - ["duplicate", "invalid", "solved", "valid_solved"].includes(lifecycle) - ? "do_not_use" - : maintainerWip || warnings.some((warning) => /thin|stale|direct-PR/i.test(warning)) || bountyCaution || lifecycle === "stale" - ? "needs_proof" - : "ready"; - return { number: issue.number, title: issue.title, status, score, reasons, warnings }; - }) - .sort((left, right) => right.score - left.score || left.number - right.number) - .slice(0, ISSUE_QUALITY_REPORT_CAP); - return { - repoFullName: fullName, - generatedAt: nowIso(), - lane, - issues: reports, - summary: `${reports.length} open issue(s) evaluated; ${reports.filter((report) => report.status === "ready").length} look ready from cached metadata.`, - }; -} +/** + * Package-local `buildIssueQualityReport` (#6057). + * + * Canonical scoring lives in the host signals engine (`signals/engine.ts`), which is intentionally + * excluded from this package's `tsc` emit (host-bound imports). Re-exporting that file from the public + * barrel breaks `npm run build` inside `@loopover/engine` — see closed #6139. + * + * This module is the portable twin of that function, matching the same call signature and status rules, + * built only on package-local types/helpers the way `buildCollisionReport` already is. + */ + +import type { + BountyLifecycle, + BountyRecord, + CollisionCluster, + CollisionReport, + IssueQualityReport, + IssueRecord, + LaneAdvice, + PullRequestRecord, + RecentMergedPullRequestRecord, + RepositoryRecord, +} from "../types/predicted-gate-types.js"; +import { nowIso } from "../utils/json.js"; +import { + bountyIssueKey, + buildCollisionReport, + buildLaneAdvice, + classifyBountyLifecycle, + indexBountiesByIssue, +} from "./predicted-gate-engine.js"; + +const ISSUE_QUALITY_REPORT_CAP = 100; +const ISSUE_DISCOVERY_LIFECYCLE_REPORT_CAP = 300; + +const MAINTAINER_WIP_LABELS = new Set([ + "wip", + "work in progress", + "work-in-progress", + "in progress", + "in-progress", + "blocked", + "on hold", + "on-hold", + "draft", + "do not work", + "do-not-work", + "internal", +]); + +type IssueDiscoveryLifecycleState = + | "open" + | "closed_not_solved" + | "solved" + | "valid_solved" + | "stale" + | "duplicate" + | "invalid"; + +type LifecycleEntry = { + number: number; + title: string; + state: IssueDiscoveryLifecycleState; + solvedByPullRequests: number[]; + reasons: string[]; +}; + +function clamp(value: number, min: number, max: number): number { + return Math.max(min, Math.min(max, value)); +} + +function daysSince(value: string | null | undefined): number { + if (!value) return 0; + const parsed = Date.parse(value); + /* v8 ignore next -- Invalid timestamps normalize to fresh. */ + if (!Number.isFinite(parsed)) return 0; + return Math.floor((Date.now() - parsed) / 86_400_000); +} + +function isMaintainerAssociation(value: string | null | undefined): boolean { + return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR"; +} + +function sameLogin(value: string | null | undefined, login: string): boolean { + return value?.toLowerCase() === login.toLowerCase(); +} + +function isMaintainerWipIssue(issue: IssueRecord): boolean { + return isMaintainerAssociation(issue.authorAssociation) && issue.labels.some((label) => MAINTAINER_WIP_LABELS.has(label.toLowerCase().trim())); +} + +function indexPullRequestsByLinkedIssue(pullRequests: T[]): Map { + const byIssue = new Map(); + for (const pr of pullRequests) { + for (const issueNumber of new Set(pr.linkedIssues)) { + const bucket = byIssue.get(issueNumber); + if (bucket) bucket.push(pr); + else byIssue.set(issueNumber, [pr]); + } + } + return byIssue; +} + +function indexCollisionClustersByIssue(clusters: CollisionCluster[]): Map { + const byIssue = new Map(); + for (const cluster of clusters) { + const issueNumbers = new Set(); + for (const item of cluster.items) if (item.type === "issue") issueNumbers.add(item.number); + for (const issueNumber of issueNumbers) { + const bucket = byIssue.get(issueNumber); + if (bucket) bucket.push(cluster); + else byIssue.set(issueNumber, [cluster]); + } + } + return byIssue; +} + +function resolveLinkedPullRequests( + issue: IssueRecord, + pullRequests: T[], + byLinkedIssue: Map, + byNumber: Map, +): T[] { + const linkingPrs = byLinkedIssue.get(issue.number) ?? []; + let addedBackReference = false; + const matchedNumbers = new Set(linkingPrs.map((pr) => pr.number)); + for (const prNumber of issue.linkedPrs) { + if (byNumber.has(prNumber) && !matchedNumbers.has(prNumber)) { + matchedNumbers.add(prNumber); + addedBackReference = true; + } + } + if (!addedBackReference) return [...linkingPrs]; + return pullRequests.filter((pr) => matchedNumbers.has(pr.number)); +} + +function classifyIssueDiscoveryLifecycle( + issue: IssueRecord, + pullRequests: PullRequestRecord[], + recentMergedPullRequests: RecentMergedPullRequestRecord[], + lane: LaneAdvice, + linkedIndex?: { open: Map; merged: Map }, +): LifecycleEntry { + // With a prebuilt index (the per-repo lifecycle report) look up this issue's linked PRs in O(1); ad-hoc + // single-issue callers pass no index and fall back to the original filter. buildIssueQualityReport always + // supplies an index, so the filter fallbacks are defensive mirrors of the host engine. + /* v8 ignore next 3 -- Ad-hoc no-index path is unused by this module's only caller. */ + const linkedOpenPrs = linkedIndex ? (linkedIndex.open.get(issue.number) ?? []) : pullRequests.filter((pr) => pr.linkedIssues.includes(issue.number)); + /* v8 ignore next 3 -- Ad-hoc no-index path is unused by this module's only caller. */ + const linkedMergedPrs = linkedIndex + ? (linkedIndex.merged.get(issue.number) ?? []) + : recentMergedPullRequests.filter((pr) => pr.linkedIssues.includes(issue.number)); + const mergedSolverPrs = [...linkedOpenPrs.filter((pr) => pr.mergedAt || pr.state === "merged"), ...linkedMergedPrs]; + const solvedByPullRequests = [...new Set(mergedSolverPrs.map((pr) => pr.number))].sort((left, right) => left - right); + const issueAuthorLogin = issue.authorLogin; + const selfSolvedLoop = Boolean( + issueAuthorLogin && mergedSolverPrs.length > 0 && mergedSolverPrs.every((pr) => sameLogin(pr.authorLogin, issueAuthorLogin)), + ); + const labels = issue.labels.map((label) => label.toLowerCase()); + const stale = daysSince(issue.updatedAt ?? issue.createdAt) > 90; + const duplicate = labels.some((label) => /duplicate/.test(label)); + const invalid = labels.some((label) => /invalid|wontfix|not planned|won't fix/.test(label)); + const state: IssueDiscoveryLifecycleState = duplicate + ? "duplicate" + : invalid + ? "invalid" + : solvedByPullRequests.length > 0 + ? (lane.lane === "issue_discovery" || lane.lane === "split") && !selfSolvedLoop + ? "valid_solved" + : "solved" + : issue.state !== "open" + ? "closed_not_solved" + : stale + ? "stale" + : "open"; + const reasons = [ + ...(duplicate ? ["Issue carries duplicate labeling."] : []), + ...(invalid ? ["Issue carries invalid or not-planned labeling."] : []), + ...(solvedByPullRequests.length > 0 ? [`Linked solver PR(s): ${solvedByPullRequests.map((number) => `#${number}`).join(", ")}.`] : []), + ...(selfSolvedLoop ? ["Linked solver PR author matches the issue reporter; cache treats this as solved but not valid issue-discovery evidence."] : []), + ...(issue.state !== "open" && solvedByPullRequests.length === 0 ? ["Issue is closed without cached solver PR evidence."] : []), + ...(stale && issue.state === "open" ? ["Issue is stale in cached metadata."] : []), + ...(lane.lane === "direct_pr" ? ["Repo is direct-PR first; lifecycle should not encourage issue filing."] : []), + ]; + return { + number: issue.number, + title: issue.title, + state, + solvedByPullRequests, + reasons: reasons.length > 0 ? reasons : ["Issue is open with no solver or duplicate signal."], + }; +} + +function buildLifecycleByIssue( + repo: RepositoryRecord | null, + issues: IssueRecord[], + pullRequests: PullRequestRecord[], + fullName: string, + recentMergedPullRequests: RecentMergedPullRequestRecord[], +): Map { + const lane = buildLaneAdvice(repo, fullName); + const linkedIndex = { + open: indexPullRequestsByLinkedIssue(pullRequests), + merged: indexPullRequestsByLinkedIssue(recentMergedPullRequests), + }; + // Bulk classify the first CAP entries (host engine does the same), then PIN every remaining + // open issue that reports will evaluate — otherwise an open issue past the slice would be missing + // from the map while buildIssueQualityReport still iterates it (#6057 / closed #6141). + const cappedIssues = issues.slice(0, ISSUE_DISCOVERY_LIFECYCLE_REPORT_CAP); + const cappedNumbers = new Set(cappedIssues.map((issue) => issue.number)); + const pinnedOpenOutsideCap = issues.filter((issue) => issue.state === "open" && !cappedNumbers.has(issue.number)); + const issuesToClassify = pinnedOpenOutsideCap.length > 0 ? [...cappedIssues, ...pinnedOpenOutsideCap] : cappedIssues; + return new Map( + issuesToClassify.map((issue) => [issue.number, classifyIssueDiscoveryLifecycle(issue, pullRequests, recentMergedPullRequests, lane, linkedIndex)]), + ); +} + +/** + * Evaluate open issues for contribution readiness. + * + * Call signature matches the host engine: + * `(repo, issues, pullRequests, fullName, bounties?, prebuiltCollisions?, recentMergedPullRequests?)`. + */ +export function buildIssueQualityReport( + repo: RepositoryRecord | null, + issues: IssueRecord[], + pullRequests: PullRequestRecord[], + fullName: string, + bounties: BountyRecord[] = [], + prebuiltCollisions?: CollisionReport, + recentMergedPullRequests: RecentMergedPullRequestRecord[] = [], +): IssueQualityReport { + const lane = buildLaneAdvice(repo, fullName); + const collisions = prebuiltCollisions ?? buildCollisionReport(fullName, issues, pullRequests, recentMergedPullRequests); + const bountyByIssue = indexBountiesByIssue(bounties); + const prsByLinkedIssue = indexPullRequestsByLinkedIssue(pullRequests); + const prByNumber = new Map(pullRequests.map((pr) => [pr.number, pr] as const)); + const mergedPrsByLinkedIssue = indexPullRequestsByLinkedIssue(recentMergedPullRequests); + const mergedPrByNumber = new Map(recentMergedPullRequests.map((pr) => [pr.number, pr] as const)); + const clustersByIssue = indexCollisionClustersByIssue(collisions.clusters); + const lifecycleByIssue = buildLifecycleByIssue(repo, issues, pullRequests, fullName, recentMergedPullRequests); + const reports = issues + .filter((issue) => issue.state === "open") + .map((issue) => { + const linkedPrs = resolveLinkedPullRequests(issue, pullRequests, prsByLinkedIssue, prByNumber); + const linkedMergedPrs = resolveLinkedPullRequests(issue, recentMergedPullRequests, mergedPrsByLinkedIssue, mergedPrByNumber); + const issueCollisions = clustersByIssue.get(issue.number) ?? []; + /* v8 ignore next -- Missing dates normalize to zero age. */ + const age = daysSince(issue.updatedAt ?? issue.createdAt); + /* v8 ignore next -- Defensive host-mirror fallback; open issues past the CAP are pinned into the map above. */ + const lifecycle = lifecycleByIssue.get(issue.number)?.state ?? "open"; + const bodyLength = issue.body?.trim().length ?? 0; + const bounty = bountyByIssue.get(bountyIssueKey(fullName, issue.number)) ?? null; + const bountyLifecycle: BountyLifecycle | null = bounty ? classifyBountyLifecycle(bounty, issue) : null; + const linkedWorkCount = linkedPrs.length + linkedMergedPrs.length + issue.linkedPrs.length; + const maintainerAuthored = isMaintainerAssociation(issue.authorAssociation); + const maintainerWip = isMaintainerWipIssue(issue); + const reasons = [ + ...(bodyLength >= 200 ? ["Issue has enough body detail to evaluate."] : []), + ...(issue.labels.length > 0 ? [`Labels: ${issue.labels.join(", ")}.`] : []), + ...(linkedWorkCount === 0 ? ["No active PR is linked in cached metadata."] : []), + ...(bountyLifecycle === "active" ? ["Active bounty context is attached (contribution context, not guaranteed payout)."] : []), + ]; + const warnings = [ + ...(bodyLength < 80 ? ["Issue body is thin; contributor may need more proof before acting."] : []), + ...(linkedPrs.length > 0 ? [`${linkedPrs.length} active PR(s) already reference this issue.`] : []), + ...(linkedMergedPrs.length > 0 ? [`${linkedMergedPrs.length} merged PR(s) already reference this issue.`] : []), + ...(issue.linkedPrs.length > 0 && linkedPrs.length === 0 && linkedMergedPrs.length === 0 + ? [`Cached issue metadata already references PR(s): ${issue.linkedPrs.map((number) => `#${number}`).join(", ")}.`] + : []), + ...(issueCollisions.length > 0 ? ["Potential duplicate or overlapping issue/PR context exists."] : []), + ...(age > 90 ? ["Issue is stale in cached metadata."] : []), + ...(lifecycle !== "open" ? [`Issue lifecycle is ${lifecycle.replace(/_/g, " ")}.`] : []), + ...(lane.lane === "direct_pr" ? ["Repo is direct-PR first; issue filing is not the primary Gittensor lane."] : []), + ...(bountyLifecycle === "completed" ? ["A completed bounty is attached; the work is likely already solved, not an open opportunity."] : []), + ...(bountyLifecycle === "cancelled" ? ["A cancelled bounty is attached; this is not an active opportunity."] : []), + ...(bountyLifecycle === "historical" + ? ["Historical bounty context is attached; this is not an active opportunity without upstream confirmation."] + : []), + ...(bountyLifecycle === "stale" ? ["Bounty context for this issue looks stale; confirm it is still active before acting."] : []), + ...(bountyLifecycle === "ambiguous" ? ["Bounty state for this issue is ambiguous; verify it before acting."] : []), + ...(maintainerAuthored && !maintainerWip ? ["Maintainer-authored; confirm it is open for outside contribution before starting."] : []), + ...(maintainerWip + ? ["Maintainer-authored and labelled in-progress/internal; not a recommended outside-contributor target without confirmation."] + : []), + ]; + const score = clamp(100 - warnings.length * 18 + reasons.length * 5 - (age > 180 ? 15 : 0), 0, 100); + const bountyBlocks = bountyLifecycle === "completed" || bountyLifecycle === "cancelled" || bountyLifecycle === "historical"; + const bountyCaution = bountyLifecycle === "stale" || bountyLifecycle === "ambiguous"; + // Note: the host engine also has a `score < 45 → hold` arm, but with the current warning vocabulary + // that arm is unreachable without first matching needs_proof/do_not_use (only two warnings can fire + // without flipping those statuses). Ready is therefore observationally identical for reachable inputs. + const status: IssueQualityReport["issues"][number]["status"] = + linkedWorkCount > 0 || + issueCollisions.some((cluster) => cluster.risk === "high") || + bountyBlocks || + ["duplicate", "invalid", "solved", "valid_solved"].includes(lifecycle) + ? "do_not_use" + : maintainerWip || warnings.some((warning) => /thin|stale|direct-PR/i.test(warning)) || bountyCaution || lifecycle === "stale" + ? "needs_proof" + : "ready"; + return { number: issue.number, title: issue.title, status, score, reasons, warnings }; + }) + .sort((left, right) => right.score - left.score || left.number - right.number) + .slice(0, ISSUE_QUALITY_REPORT_CAP); + return { + repoFullName: fullName, + generatedAt: nowIso(), + lane, + issues: reports, + summary: `${reports.length} open issue(s) evaluated; ${reports.filter((report) => report.status === "ready").length} look ready from cached metadata.`, + }; +} diff --git a/review-enrichment/src/analyzers/binary-extensions.ts b/review-enrichment/src/analyzers/binary-extensions.ts index 5fbd052b4d..796e682846 100644 --- a/review-enrichment/src/analyzers/binary-extensions.ts +++ b/review-enrichment/src/analyzers/binary-extensions.ts @@ -1,104 +1,104 @@ -// Shared binary-file extension inventory for analyzers that classify opaque blobs by path extension. -// asset-weight.ts (size bloat) and provenance.ts (unauditable committed artifacts) must stay in parity — -// a single source list prevents one analyzer from drifting and missing formats the other already flags. - -/** Lowercase extensions for genuinely binary assets. Text formats like .svg/.json are excluded. */ -export const BINARY_FILE_EXTENSIONS = [ - // Images - "png", - "jpg", - "jpeg", - "gif", - "bmp", - "tiff", - "tif", - "ico", - "webp", - "avif", - "heic", - "heif", - // Fonts - "woff", - "woff2", - "ttf", - "otf", - "eot", - // Media - "mp4", - "mov", - "avi", - "webm", - "mkv", - "mp3", - "wav", - "flac", - "ogg", - // Archives / compression - "zip", - "tar", - "gz", - "tgz", - "bz2", - "7z", - "rar", - "xz", - "zst", - "lz4", - "br", - // Documents / design - "pdf", - "psd", - "ai", - "sketch", - "fig", - "xcf", - // Native / compiled - "exe", - "dll", - "so", - "dylib", - "bin", - "dat", - "wasm", - "node", - "jar", - "class", - "pyc", - "pyo", - "pyd", - "o", - "a", - "war", - "ear", - // ML checkpoints - "safetensors", - "gguf", - "onnx", - "pt", - "pth", - "ckpt", - // Scientific / ML data artifacts - "h5", - "hdf5", - "pb", - "npy", - "npz", - "parquet", - "feather", - "arrow", - "orc", - "msgpack", -] as const; - -const BINARY_EXT_SET = new Set(BINARY_FILE_EXTENSIONS); - -/** True when `ext` (without a leading dot) is a known binary file extension. Case-insensitive. Pure. */ -export function isBinaryFileExtension(ext: string): boolean { - return BINARY_EXT_SET.has(ext.toLowerCase()); -} - -/** Extension-anchored, case-insensitive regex matching any shared binary extension at path end. Pure. */ -export const BINARY_EXT_RE = new RegExp( - `\\.(?:${BINARY_FILE_EXTENSIONS.join("|")})$`, - "i", -); +// Shared binary-file extension inventory for analyzers that classify opaque blobs by path extension. +// asset-weight.ts (size bloat) and provenance.ts (unauditable committed artifacts) must stay in parity — +// a single source list prevents one analyzer from drifting and missing formats the other already flags. + +/** Lowercase extensions for genuinely binary assets. Text formats like .svg/.json are excluded. */ +export const BINARY_FILE_EXTENSIONS = [ + // Images + "png", + "jpg", + "jpeg", + "gif", + "bmp", + "tiff", + "tif", + "ico", + "webp", + "avif", + "heic", + "heif", + // Fonts + "woff", + "woff2", + "ttf", + "otf", + "eot", + // Media + "mp4", + "mov", + "avi", + "webm", + "mkv", + "mp3", + "wav", + "flac", + "ogg", + // Archives / compression + "zip", + "tar", + "gz", + "tgz", + "bz2", + "7z", + "rar", + "xz", + "zst", + "lz4", + "br", + // Documents / design + "pdf", + "psd", + "ai", + "sketch", + "fig", + "xcf", + // Native / compiled + "exe", + "dll", + "so", + "dylib", + "bin", + "dat", + "wasm", + "node", + "jar", + "class", + "pyc", + "pyo", + "pyd", + "o", + "a", + "war", + "ear", + // ML checkpoints + "safetensors", + "gguf", + "onnx", + "pt", + "pth", + "ckpt", + // Scientific / ML data artifacts + "h5", + "hdf5", + "pb", + "npy", + "npz", + "parquet", + "feather", + "arrow", + "orc", + "msgpack", +] as const; + +const BINARY_EXT_SET = new Set(BINARY_FILE_EXTENSIONS); + +/** True when `ext` (without a leading dot) is a known binary file extension. Case-insensitive. Pure. */ +export function isBinaryFileExtension(ext: string): boolean { + return BINARY_EXT_SET.has(ext.toLowerCase()); +} + +/** Extension-anchored, case-insensitive regex matching any shared binary extension at path end. Pure. */ +export const BINARY_EXT_RE = new RegExp( + `\\.(?:${BINARY_FILE_EXTENSIONS.join("|")})$`, + "i", +); diff --git a/review-enrichment/test/binary-extensions.test.ts b/review-enrichment/test/binary-extensions.test.ts index 2ae0395202..2ffb4fba3e 100644 --- a/review-enrichment/test/binary-extensions.test.ts +++ b/review-enrichment/test/binary-extensions.test.ts @@ -1,77 +1,77 @@ -// Units for the shared binary extension inventory. Keeps asset-weight and provenance parity testable in one place. -import { test } from "node:test"; -import assert from "node:assert/strict"; - -import { - BINARY_EXT_RE, - BINARY_FILE_EXTENSIONS, - isBinaryFileExtension, -} from "../dist/analyzers/binary-extensions.js"; - -test("isBinaryFileExtension and BINARY_EXT_RE agree on known binary paths", () => { - for (const path of [ - "assets/logo.png", - "cache/model.zst", - "dist/bundle.tar.br", - "snapshots/data.lz4", - "models/weights.safetensors", - "models/llama.gguf", - "data/train.h5", - "data/features.hdf5", - "models/saved_model.pb", - "data/embeddings.npy", - "data/batch.npz", - "warehouse/events.parquet", - "warehouse/snapshot.feather", - "lake/part-000.arrow", - "lake/part-000.orc", - "wire/msg.msgpack", - "native/mod.pyd", - "build/Release/addon.node", - ]) { - const ext = path.slice(path.lastIndexOf(".") + 1); - assert.equal(isBinaryFileExtension(ext), true, path); - assert.equal(BINARY_EXT_RE.test(path), true, path); - } -}); - -test("isBinaryFileExtension is case-insensitive", () => { - assert.equal(isBinaryFileExtension("PNG"), true); - assert.equal(isBinaryFileExtension("Parquet"), true); - assert.equal(BINARY_EXT_RE.test("data/TRAIN.H5"), true); -}); - -test("isBinaryFileExtension rejects text and extensionless paths", () => { - for (const path of [ - "src/index.ts", - "icons/logo.svg", - "data/config.json", - "Makefile", - "src/parquet.ts", - "lib/npy_utils.py", - ]) { - const dot = path.lastIndexOf("."); - const ext = dot >= 0 ? path.slice(dot + 1) : ""; - if (ext) assert.equal(isBinaryFileExtension(ext), false, path); - assert.equal(BINARY_EXT_RE.test(path), false, path); - } -}); - -test("BINARY_FILE_EXTENSIONS includes ML checkpoint and scientific data formats", () => { - for (const ext of [ - "safetensors", - "gguf", - "onnx", - "h5", - "hdf5", - "parquet", - "feather", - "arrow", - "orc", - "msgpack", - "lz4", - "br", - ]) { - assert.ok(BINARY_FILE_EXTENSIONS.includes(ext), ext); - } -}); +// Units for the shared binary extension inventory. Keeps asset-weight and provenance parity testable in one place. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + BINARY_EXT_RE, + BINARY_FILE_EXTENSIONS, + isBinaryFileExtension, +} from "../dist/analyzers/binary-extensions.js"; + +test("isBinaryFileExtension and BINARY_EXT_RE agree on known binary paths", () => { + for (const path of [ + "assets/logo.png", + "cache/model.zst", + "dist/bundle.tar.br", + "snapshots/data.lz4", + "models/weights.safetensors", + "models/llama.gguf", + "data/train.h5", + "data/features.hdf5", + "models/saved_model.pb", + "data/embeddings.npy", + "data/batch.npz", + "warehouse/events.parquet", + "warehouse/snapshot.feather", + "lake/part-000.arrow", + "lake/part-000.orc", + "wire/msg.msgpack", + "native/mod.pyd", + "build/Release/addon.node", + ]) { + const ext = path.slice(path.lastIndexOf(".") + 1); + assert.equal(isBinaryFileExtension(ext), true, path); + assert.equal(BINARY_EXT_RE.test(path), true, path); + } +}); + +test("isBinaryFileExtension is case-insensitive", () => { + assert.equal(isBinaryFileExtension("PNG"), true); + assert.equal(isBinaryFileExtension("Parquet"), true); + assert.equal(BINARY_EXT_RE.test("data/TRAIN.H5"), true); +}); + +test("isBinaryFileExtension rejects text and extensionless paths", () => { + for (const path of [ + "src/index.ts", + "icons/logo.svg", + "data/config.json", + "Makefile", + "src/parquet.ts", + "lib/npy_utils.py", + ]) { + const dot = path.lastIndexOf("."); + const ext = dot >= 0 ? path.slice(dot + 1) : ""; + if (ext) assert.equal(isBinaryFileExtension(ext), false, path); + assert.equal(BINARY_EXT_RE.test(path), false, path); + } +}); + +test("BINARY_FILE_EXTENSIONS includes ML checkpoint and scientific data formats", () => { + for (const ext of [ + "safetensors", + "gguf", + "onnx", + "h5", + "hdf5", + "parquet", + "feather", + "arrow", + "orc", + "msgpack", + "lz4", + "br", + ]) { + assert.ok(BINARY_FILE_EXTENSIONS.includes(ext), ext); + } +}); diff --git a/review-enrichment/test/codeowners.test.ts b/review-enrichment/test/codeowners.test.ts index 62fdd8b259..21caa25463 100644 --- a/review-enrichment/test/codeowners.test.ts +++ b/review-enrichment/test/codeowners.test.ts @@ -1,67 +1,67 @@ -// Units for the CODEOWNERS analyzer (#1515). Own file (not enrichment.test.ts) so concurrent analyzer PRs -// don't collide. All network is mocked. Runs against the compiled dist/. -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { scanCodeowners } from "../dist/analyzers/codeowners.js"; - -const req = (overrides: Record = {}) => ({ - repoFullName: "octo/repo", - prNumber: 1, - githubToken: "ghp_test", - author: "alice", - files: [{ path: "src/a.ts" }], - ...overrides, -}); - -test("scanCodeowners: rejects multi-segment repo slugs without fetching", async () => { - let called = false; - const out = await scanCodeowners( - req({ repoFullName: "octo/repo/extra" }), - async () => { - called = true; - return new Response("* @bob\n", { status: 200 }); - }, - ); - assert.deepEqual(out, []); - assert.equal(called, false); -}); - -test("scanCodeowners: requires token, author, and a valid owner/repo slug", async () => { - let called = false; - const fetch = async () => { - called = true; - return new Response("* @bob\n", { status: 200 }); - }; - assert.deepEqual(await scanCodeowners(req({ githubToken: undefined }), fetch), []); - assert.deepEqual(await scanCodeowners(req({ author: undefined }), fetch), []); - assert.deepEqual(await scanCodeowners(req({ repoFullName: "bad slug/x!" }), fetch), []); - assert.equal(called, false); -}); - -test("scanCodeowners: fetches CODEOWNERS for a canonical owner/repo slug", async () => { - let url = ""; - const out = await scanCodeowners(req(), async (input) => { - url = String(input); - return new Response("* @alice\n", { status: 200 }); - }); - assert.match(url, /\/repos\/octo\/repo\/contents\//); - assert.deepEqual(out, []); -}); - -test("scanCodeowners: reports files where the author is not listed as an owner", async () => { - const out = await scanCodeowners(req({ author: "alice" }), async () => - new Response("src/a.ts @bob\n", { status: 200 }), - ); - assert.deepEqual(out, [{ file: "src/a.ts", owners: ["@bob"] }]); -}); - -test("scanCodeowners: a trailing-slash directory rule owns files at any depth", async () => { - // `config/` has only a trailing slash, so per gitignore/CODEOWNERS semantics it must match at - // any depth — not just repo-root `config/…`. Regression for the anchoring being computed after - // the `config/` → `config/**` expansion, which wrongly treated the rule as root-anchored. - const out = await scanCodeowners( - req({ author: "alice", files: [{ path: "services/api/config/settings.yaml" }] }), - async () => new Response("config/ @bob\n", { status: 200 }), - ); - assert.deepEqual(out, [{ file: "services/api/config/settings.yaml", owners: ["@bob"] }]); -}); +// Units for the CODEOWNERS analyzer (#1515). Own file (not enrichment.test.ts) so concurrent analyzer PRs +// don't collide. All network is mocked. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { scanCodeowners } from "../dist/analyzers/codeowners.js"; + +const req = (overrides: Record = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_test", + author: "alice", + files: [{ path: "src/a.ts" }], + ...overrides, +}); + +test("scanCodeowners: rejects multi-segment repo slugs without fetching", async () => { + let called = false; + const out = await scanCodeowners( + req({ repoFullName: "octo/repo/extra" }), + async () => { + called = true; + return new Response("* @bob\n", { status: 200 }); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanCodeowners: requires token, author, and a valid owner/repo slug", async () => { + let called = false; + const fetch = async () => { + called = true; + return new Response("* @bob\n", { status: 200 }); + }; + assert.deepEqual(await scanCodeowners(req({ githubToken: undefined }), fetch), []); + assert.deepEqual(await scanCodeowners(req({ author: undefined }), fetch), []); + assert.deepEqual(await scanCodeowners(req({ repoFullName: "bad slug/x!" }), fetch), []); + assert.equal(called, false); +}); + +test("scanCodeowners: fetches CODEOWNERS for a canonical owner/repo slug", async () => { + let url = ""; + const out = await scanCodeowners(req(), async (input) => { + url = String(input); + return new Response("* @alice\n", { status: 200 }); + }); + assert.match(url, /\/repos\/octo\/repo\/contents\//); + assert.deepEqual(out, []); +}); + +test("scanCodeowners: reports files where the author is not listed as an owner", async () => { + const out = await scanCodeowners(req({ author: "alice" }), async () => + new Response("src/a.ts @bob\n", { status: 200 }), + ); + assert.deepEqual(out, [{ file: "src/a.ts", owners: ["@bob"] }]); +}); + +test("scanCodeowners: a trailing-slash directory rule owns files at any depth", async () => { + // `config/` has only a trailing slash, so per gitignore/CODEOWNERS semantics it must match at + // any depth — not just repo-root `config/…`. Regression for the anchoring being computed after + // the `config/` → `config/**` expansion, which wrongly treated the rule as root-anchored. + const out = await scanCodeowners( + req({ author: "alice", files: [{ path: "services/api/config/settings.yaml" }] }), + async () => new Response("config/ @bob\n", { status: 200 }), + ); + assert.deepEqual(out, [{ file: "services/api/config/settings.yaml", owners: ["@bob"] }]); +}); diff --git a/review-enrichment/test/debug-leftover.test.ts b/review-enrichment/test/debug-leftover.test.ts index 32cb4222d6..ddec85fe8b 100644 --- a/review-enrichment/test/debug-leftover.test.ts +++ b/review-enrichment/test/debug-leftover.test.ts @@ -1,95 +1,95 @@ -// Units for the debug-leftover analyzer (#2015). 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 { - detectDebugLeftover, - scanDebugLeftover, - scanPatchForDebugLeftover, -} from "../dist/analyzers/debug-leftover.js"; -import { renderBrief } from "../dist/render.js"; - -const patchOf = (lines: string[]) => - `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; - -test("detectDebugLeftover: recognizes debugger, console sinks, and print()", () => { - assert.equal(detectDebugLeftover(" debugger;"), "debugger"); - assert.equal(detectDebugLeftover("console.log('hi')"), "console"); - assert.equal(detectDebugLeftover(" console.debug(state)"), "console"); - assert.equal(detectDebugLeftover("print('debug')", "lib/b.py"), "print"); -}); - -test("detectDebugLeftover: print() is Python-only and does not match method calls like document.print()", () => { - assert.equal(detectDebugLeftover("document.print()"), null); - assert.equal(detectDebugLeftover("printer.print('x')"), null); - assert.equal(detectDebugLeftover("print('debug')", "src/widget.ts"), null); - assert.equal(detectDebugLeftover("obj.print('x')", "pkg/widget.py"), null); -}); - -test("detectDebugLeftover: a console call inside a string literal is not flagged", () => { - assert.equal(detectDebugLeftover('const s = "console.log(\\"nope\\")"'), null); - assert.equal(detectDebugLeftover("log(`hint: console.log(here)`);"), null); -}); - -test("detectDebugLeftover: debugger inside a string is not flagged", () => { - assert.equal(detectDebugLeftover('const msg = "debugger;"'), null); -}); - -test("scanPatchForDebugLeftover: flags added lines with correct locations", () => { - const findings = scanPatchForDebugLeftover( - "src/widget.ts", - patchOf(["function f() {", " debugger;", " console.log('x');", " return g();", "}"]), - ); - assert.deepEqual(findings, [ - { file: "src/widget.ts", line: 2, kind: "debugger" }, - { file: "src/widget.ts", line: 3, kind: "console" }, - ]); -}); - -test("scanPatchForDebugLeftover: only ADDED lines are scanned", () => { - const patch = [ - "@@ -10,2 +10,2 @@", - " function f() {", - "- console.log('old');", - "+ print('new')", - ].join("\n"); - assert.deepEqual(scanPatchForDebugLeftover("pkg/widget.py", patch), [ - { file: "pkg/widget.py", line: 11, kind: "print" }, - ]); -}); - -test("scanPatchForDebugLeftover: skips test/spec files", () => { - assert.deepEqual( - scanPatchForDebugLeftover("src/widget.test.ts", patchOf(["console.log('in test')"])), - [], - ); - assert.deepEqual( - scanPatchForDebugLeftover("tests/widget.spec.js", patchOf(["debugger;"])), - [], - ); -}); - -test("scanPatchForDebugLeftover: respects the findings cap", () => { - const lines = Array.from({ length: 30 }, (_, i) => `console.log(${i});`); - assert.equal(scanPatchForDebugLeftover("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); -}); - -test("scanDebugLeftover: aggregates across files and renders in the brief", async () => { - const findings = await scanDebugLeftover({ - files: [ - { path: "src/a.ts", patch: patchOf(["debugger;"]) }, - { path: "lib/b.py", patch: patchOf(["print('x')"]) }, - ], - }); - assert.deepEqual(findings, [ - { file: "src/a.ts", line: 1, kind: "debugger" }, - { file: "lib/b.py", line: 1, kind: "print" }, - ]); - - const { promptSection } = renderBrief({ - debugLeftover: findings, - }); - assert.match(promptSection, /Debug leftovers/); - assert.match(promptSection, /src\/a\.ts:1/); - assert.match(promptSection, /lib\/b\.py:1/); -}); +// Units for the debug-leftover analyzer (#2015). 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 { + detectDebugLeftover, + scanDebugLeftover, + scanPatchForDebugLeftover, +} from "../dist/analyzers/debug-leftover.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("detectDebugLeftover: recognizes debugger, console sinks, and print()", () => { + assert.equal(detectDebugLeftover(" debugger;"), "debugger"); + assert.equal(detectDebugLeftover("console.log('hi')"), "console"); + assert.equal(detectDebugLeftover(" console.debug(state)"), "console"); + assert.equal(detectDebugLeftover("print('debug')", "lib/b.py"), "print"); +}); + +test("detectDebugLeftover: print() is Python-only and does not match method calls like document.print()", () => { + assert.equal(detectDebugLeftover("document.print()"), null); + assert.equal(detectDebugLeftover("printer.print('x')"), null); + assert.equal(detectDebugLeftover("print('debug')", "src/widget.ts"), null); + assert.equal(detectDebugLeftover("obj.print('x')", "pkg/widget.py"), null); +}); + +test("detectDebugLeftover: a console call inside a string literal is not flagged", () => { + assert.equal(detectDebugLeftover('const s = "console.log(\\"nope\\")"'), null); + assert.equal(detectDebugLeftover("log(`hint: console.log(here)`);"), null); +}); + +test("detectDebugLeftover: debugger inside a string is not flagged", () => { + assert.equal(detectDebugLeftover('const msg = "debugger;"'), null); +}); + +test("scanPatchForDebugLeftover: flags added lines with correct locations", () => { + const findings = scanPatchForDebugLeftover( + "src/widget.ts", + patchOf(["function f() {", " debugger;", " console.log('x');", " return g();", "}"]), + ); + assert.deepEqual(findings, [ + { file: "src/widget.ts", line: 2, kind: "debugger" }, + { file: "src/widget.ts", line: 3, kind: "console" }, + ]); +}); + +test("scanPatchForDebugLeftover: only ADDED lines are scanned", () => { + const patch = [ + "@@ -10,2 +10,2 @@", + " function f() {", + "- console.log('old');", + "+ print('new')", + ].join("\n"); + assert.deepEqual(scanPatchForDebugLeftover("pkg/widget.py", patch), [ + { file: "pkg/widget.py", line: 11, kind: "print" }, + ]); +}); + +test("scanPatchForDebugLeftover: skips test/spec files", () => { + assert.deepEqual( + scanPatchForDebugLeftover("src/widget.test.ts", patchOf(["console.log('in test')"])), + [], + ); + assert.deepEqual( + scanPatchForDebugLeftover("tests/widget.spec.js", patchOf(["debugger;"])), + [], + ); +}); + +test("scanPatchForDebugLeftover: respects the findings cap", () => { + const lines = Array.from({ length: 30 }, (_, i) => `console.log(${i});`); + assert.equal(scanPatchForDebugLeftover("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); +}); + +test("scanDebugLeftover: aggregates across files and renders in the brief", async () => { + const findings = await scanDebugLeftover({ + files: [ + { path: "src/a.ts", patch: patchOf(["debugger;"]) }, + { path: "lib/b.py", patch: patchOf(["print('x')"]) }, + ], + }); + assert.deepEqual(findings, [ + { file: "src/a.ts", line: 1, kind: "debugger" }, + { file: "lib/b.py", line: 1, kind: "print" }, + ]); + + const { promptSection } = renderBrief({ + debugLeftover: findings, + }); + assert.match(promptSection, /Debug leftovers/); + assert.match(promptSection, /src\/a\.ts:1/); + assert.match(promptSection, /lib\/b\.py:1/); +}); diff --git a/review-enrichment/test/dependency-diff.test.ts b/review-enrichment/test/dependency-diff.test.ts index 393b630a0f..0a45d6df94 100644 --- a/review-enrichment/test/dependency-diff.test.ts +++ b/review-enrichment/test/dependency-diff.test.ts @@ -1,104 +1,104 @@ -// Units for the dependency-diff analyzer (#2020). Own file so concurrent analyzer PRs don't collide. -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { - extractDependencyInventoryChanges, - extractDependencyChanges, -} from "../dist/analyzers/dependency-scan.js"; -import { - scanDependencyDiff, - scanDependencyDiffInventory, -} from "../dist/analyzers/dependency-diff.js"; -import { renderBrief } from "../dist/render.js"; - -const npmPatch = (lines: string[]) => - [`@@ -1,3 +1,3 @@`, ...lines.map((l) => (l.startsWith("+") || l.startsWith("-") ? l : ` ${l}`))].join( - "\n", - ); - -test("extractDependencyInventoryChanges: reports npm add, remove, and change", () => { - const files = [ - { - path: "package.json", - patch: npmPatch(['-"lodash": "4.17.20",', '+"lodash": "4.17.21",', '+"axios": "1.6.0",', '-"left-pad": "1.0.0",']), - }, - ]; - assert.deepEqual(extractDependencyInventoryChanges(files), [ - { - ecosystem: "npm", - package: "lodash", - from: "4.17.20", - to: "4.17.21", - direction: "change", - }, - { ecosystem: "npm", package: "axios", from: null, to: "1.6.0", direction: "add" }, - { ecosystem: "npm", package: "left-pad", from: "1.0.0", to: null, direction: "remove" }, - ]); - assert.deepEqual(extractDependencyChanges(files), [ - { - ecosystem: "npm", - package: "lodash", - from: "4.17.20", - to: "4.17.21", - }, - { ecosystem: "npm", package: "axios", from: null, to: "1.6.0" }, - ]); -}); - -test("extractDependencyInventoryChanges: reports PyPI requirements.txt changes", () => { - const files = [ - { - path: "requirements.txt", - patch: ["@@ -1,2 +1,2 @@", "-requests==2.31.0", "+requests==2.32.0"].join("\n"), - }, - ]; - assert.deepEqual(extractDependencyInventoryChanges(files), [ - { - ecosystem: "PyPI", - package: "requests", - from: "2.31.0", - to: "2.32.0", - direction: "change", - }, - ]); -}); - -test("extractDependencyInventoryChanges: respects the findings cap", () => { - const lines = Array.from({ length: 30 }, (_, i) => [`+"pkg${i}": "1.0.0",`]).flat(); - const files = [{ path: "package.json", patch: npmPatch(lines) }]; - assert.equal(extractDependencyInventoryChanges(files, {}, 3).length, 3); -}); - -test("extractDependencyInventoryChanges: keeps add/remove separate across manifest files", () => { - const files = [ - { - path: "apps/a/package.json", - patch: npmPatch(['-"axios": "1.6.0",']), - }, - { - path: "apps/b/package.json", - patch: npmPatch(['+"axios": "1.6.0",']), - }, - ]; - assert.deepEqual(extractDependencyInventoryChanges(files), [ - { ecosystem: "npm", package: "axios", from: "1.6.0", to: null, direction: "remove" }, - { ecosystem: "npm", package: "axios", from: null, to: "1.6.0", direction: "add" }, - ]); -}); - -test("scanDependencyDiffInventory: renders a public-safe brief", async () => { - const findings = await scanDependencyDiffInventory({ - files: [ - { - path: "package.json", - patch: npmPatch(['+"axios": "1.6.0",']), - }, - ], - }); - assert.deepEqual(findings, [ - { ecosystem: "npm", package: "axios", from: null, to: "1.6.0", direction: "add" }, - ]); - const { promptSection } = renderBrief({ dependencyDiff: findings }); - assert.match(promptSection, /Dependency inventory changes/); - assert.match(promptSection, /axios/); -}); +// Units for the dependency-diff analyzer (#2020). Own file so concurrent analyzer PRs don't collide. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + extractDependencyInventoryChanges, + extractDependencyChanges, +} from "../dist/analyzers/dependency-scan.js"; +import { + scanDependencyDiff, + scanDependencyDiffInventory, +} from "../dist/analyzers/dependency-diff.js"; +import { renderBrief } from "../dist/render.js"; + +const npmPatch = (lines: string[]) => + [`@@ -1,3 +1,3 @@`, ...lines.map((l) => (l.startsWith("+") || l.startsWith("-") ? l : ` ${l}`))].join( + "\n", + ); + +test("extractDependencyInventoryChanges: reports npm add, remove, and change", () => { + const files = [ + { + path: "package.json", + patch: npmPatch(['-"lodash": "4.17.20",', '+"lodash": "4.17.21",', '+"axios": "1.6.0",', '-"left-pad": "1.0.0",']), + }, + ]; + assert.deepEqual(extractDependencyInventoryChanges(files), [ + { + ecosystem: "npm", + package: "lodash", + from: "4.17.20", + to: "4.17.21", + direction: "change", + }, + { ecosystem: "npm", package: "axios", from: null, to: "1.6.0", direction: "add" }, + { ecosystem: "npm", package: "left-pad", from: "1.0.0", to: null, direction: "remove" }, + ]); + assert.deepEqual(extractDependencyChanges(files), [ + { + ecosystem: "npm", + package: "lodash", + from: "4.17.20", + to: "4.17.21", + }, + { ecosystem: "npm", package: "axios", from: null, to: "1.6.0" }, + ]); +}); + +test("extractDependencyInventoryChanges: reports PyPI requirements.txt changes", () => { + const files = [ + { + path: "requirements.txt", + patch: ["@@ -1,2 +1,2 @@", "-requests==2.31.0", "+requests==2.32.0"].join("\n"), + }, + ]; + assert.deepEqual(extractDependencyInventoryChanges(files), [ + { + ecosystem: "PyPI", + package: "requests", + from: "2.31.0", + to: "2.32.0", + direction: "change", + }, + ]); +}); + +test("extractDependencyInventoryChanges: respects the findings cap", () => { + const lines = Array.from({ length: 30 }, (_, i) => [`+"pkg${i}": "1.0.0",`]).flat(); + const files = [{ path: "package.json", patch: npmPatch(lines) }]; + assert.equal(extractDependencyInventoryChanges(files, {}, 3).length, 3); +}); + +test("extractDependencyInventoryChanges: keeps add/remove separate across manifest files", () => { + const files = [ + { + path: "apps/a/package.json", + patch: npmPatch(['-"axios": "1.6.0",']), + }, + { + path: "apps/b/package.json", + patch: npmPatch(['+"axios": "1.6.0",']), + }, + ]; + assert.deepEqual(extractDependencyInventoryChanges(files), [ + { ecosystem: "npm", package: "axios", from: "1.6.0", to: null, direction: "remove" }, + { ecosystem: "npm", package: "axios", from: null, to: "1.6.0", direction: "add" }, + ]); +}); + +test("scanDependencyDiffInventory: renders a public-safe brief", async () => { + const findings = await scanDependencyDiffInventory({ + files: [ + { + path: "package.json", + patch: npmPatch(['+"axios": "1.6.0",']), + }, + ], + }); + assert.deepEqual(findings, [ + { ecosystem: "npm", package: "axios", from: null, to: "1.6.0", direction: "add" }, + ]); + const { promptSection } = renderBrief({ dependencyDiff: findings }); + assert.match(promptSection, /Dependency inventory changes/); + assert.match(promptSection, /axios/); +}); diff --git a/review-enrichment/test/floating-promise.test.ts b/review-enrichment/test/floating-promise.test.ts index 9f604f93ba..eabd18ea39 100644 --- a/review-enrichment/test/floating-promise.test.ts +++ b/review-enrichment/test/floating-promise.test.ts @@ -1,85 +1,85 @@ -// Units for the floating-promise analyzer (#2023). Own file so concurrent analyzer PRs don't collide. -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { - detectFloatingPromise, - scanFloatingPromise, - scanPatchForFloatingPromise, -} from "../dist/analyzers/floating-promise.js"; -import { renderBrief } from "../dist/render.js"; - -const patchOf = (lines: string[]) => - `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; - -test("detectFloatingPromise: flags bare promise-shaped calls", () => { - assert.equal(detectFloatingPromise("loadUserAsync();"), "loadUserAsync"); - assert.equal(detectFloatingPromise(" service.saveAsync(payload);"), "service.saveAsync"); - assert.equal(detectFloatingPromise("fetch('/api/users');"), "fetch"); - assert.equal(detectFloatingPromise("Promise.all(items.map(runAsync));"), "Promise.all"); - assert.equal(detectFloatingPromise("new Promise((resolve) => resolve(1));"), "Promise"); -}); - -test("detectFloatingPromise: does not flag awaited, returned, voided, or chained calls", () => { - assert.equal(detectFloatingPromise("await loadUserAsync();"), null); - assert.equal(detectFloatingPromise("return await loadUserAsync();"), null); - assert.equal(detectFloatingPromise("return loadUserAsync();"), null); - assert.equal(detectFloatingPromise("void fetch('/health');"), null); - assert.equal(detectFloatingPromise("fetch('/x').catch(() => {});"), null); - assert.equal(detectFloatingPromise("fetch('/x').then(handleOk);"), null); - assert.equal(detectFloatingPromise("fetch('/x').finally(cleanup);"), "fetch"); - assert.equal(detectFloatingPromise('fetch("/.catch(");'), "fetch"); -}); - -test("detectFloatingPromise: skips assignments, non-promise calls, and comments", () => { - assert.equal(detectFloatingPromise("const user = loadUserAsync();"), null); - assert.equal(detectFloatingPromise("console.log('hi');"), null); - assert.equal(detectFloatingPromise("saveUser(user);"), null); - assert.equal(detectFloatingPromise("// await loadUserAsync();"), null); -}); - -test("scanPatchForFloatingPromise: flags added lines with correct locations", () => { - const findings = scanPatchForFloatingPromise( - "src/worker.ts", - patchOf([ - "export function run() {", - " syncSetup();", - " flushQueueAsync();", - "}", - ]), - ); - assert.deepEqual(findings, [{ file: "src/worker.ts", line: 3, call: "flushQueueAsync" }]); -}); - -test("scanPatchForFloatingPromise: skips test files and non-JS/TS paths", () => { - assert.deepEqual( - scanPatchForFloatingPromise("src/worker.test.ts", patchOf(["loadUserAsync();"])), - [], - ); - assert.deepEqual( - scanPatchForFloatingPromise("lib/worker.py", patchOf(["load_user_async()"])), - [], - ); -}); - -test("scanPatchForFloatingPromise: respects the findings cap", () => { - const lines = Array.from({ length: 30 }, (_, i) => `task${i}Async();`); - assert.equal(scanPatchForFloatingPromise("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); -}); - -test("scanFloatingPromise: aggregates across files and renders in the brief", async () => { - const findings = await scanFloatingPromise({ - files: [ - { path: "src/a.ts", patch: patchOf(["fetch('/api');"]) }, - { path: "src/b.ts", patch: patchOf(["syncJobAsync();"]) }, - ], - }); - assert.deepEqual(findings, [ - { file: "src/a.ts", line: 1, call: "fetch" }, - { file: "src/b.ts", line: 1, call: "syncJobAsync" }, - ]); - - const { promptSection } = renderBrief({ floatingPromise: findings }); - assert.match(promptSection, /Floating promises/); - assert.match(promptSection, /src\/a\.ts:1/); - assert.match(promptSection, /src\/b\.ts:1/); -}); +// Units for the floating-promise analyzer (#2023). Own file so concurrent analyzer PRs don't collide. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + detectFloatingPromise, + scanFloatingPromise, + scanPatchForFloatingPromise, +} from "../dist/analyzers/floating-promise.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("detectFloatingPromise: flags bare promise-shaped calls", () => { + assert.equal(detectFloatingPromise("loadUserAsync();"), "loadUserAsync"); + assert.equal(detectFloatingPromise(" service.saveAsync(payload);"), "service.saveAsync"); + assert.equal(detectFloatingPromise("fetch('/api/users');"), "fetch"); + assert.equal(detectFloatingPromise("Promise.all(items.map(runAsync));"), "Promise.all"); + assert.equal(detectFloatingPromise("new Promise((resolve) => resolve(1));"), "Promise"); +}); + +test("detectFloatingPromise: does not flag awaited, returned, voided, or chained calls", () => { + assert.equal(detectFloatingPromise("await loadUserAsync();"), null); + assert.equal(detectFloatingPromise("return await loadUserAsync();"), null); + assert.equal(detectFloatingPromise("return loadUserAsync();"), null); + assert.equal(detectFloatingPromise("void fetch('/health');"), null); + assert.equal(detectFloatingPromise("fetch('/x').catch(() => {});"), null); + assert.equal(detectFloatingPromise("fetch('/x').then(handleOk);"), null); + assert.equal(detectFloatingPromise("fetch('/x').finally(cleanup);"), "fetch"); + assert.equal(detectFloatingPromise('fetch("/.catch(");'), "fetch"); +}); + +test("detectFloatingPromise: skips assignments, non-promise calls, and comments", () => { + assert.equal(detectFloatingPromise("const user = loadUserAsync();"), null); + assert.equal(detectFloatingPromise("console.log('hi');"), null); + assert.equal(detectFloatingPromise("saveUser(user);"), null); + assert.equal(detectFloatingPromise("// await loadUserAsync();"), null); +}); + +test("scanPatchForFloatingPromise: flags added lines with correct locations", () => { + const findings = scanPatchForFloatingPromise( + "src/worker.ts", + patchOf([ + "export function run() {", + " syncSetup();", + " flushQueueAsync();", + "}", + ]), + ); + assert.deepEqual(findings, [{ file: "src/worker.ts", line: 3, call: "flushQueueAsync" }]); +}); + +test("scanPatchForFloatingPromise: skips test files and non-JS/TS paths", () => { + assert.deepEqual( + scanPatchForFloatingPromise("src/worker.test.ts", patchOf(["loadUserAsync();"])), + [], + ); + assert.deepEqual( + scanPatchForFloatingPromise("lib/worker.py", patchOf(["load_user_async()"])), + [], + ); +}); + +test("scanPatchForFloatingPromise: respects the findings cap", () => { + const lines = Array.from({ length: 30 }, (_, i) => `task${i}Async();`); + assert.equal(scanPatchForFloatingPromise("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); +}); + +test("scanFloatingPromise: aggregates across files and renders in the brief", async () => { + const findings = await scanFloatingPromise({ + files: [ + { path: "src/a.ts", patch: patchOf(["fetch('/api');"]) }, + { path: "src/b.ts", patch: patchOf(["syncJobAsync();"]) }, + ], + }); + assert.deepEqual(findings, [ + { file: "src/a.ts", line: 1, call: "fetch" }, + { file: "src/b.ts", line: 1, call: "syncJobAsync" }, + ]); + + const { promptSection } = renderBrief({ floatingPromise: findings }); + assert.match(promptSection, /Floating promises/); + assert.match(promptSection, /src\/a\.ts:1/); + assert.match(promptSection, /src\/b\.ts:1/); +}); diff --git a/review-enrichment/test/hardcoded-url.test.ts b/review-enrichment/test/hardcoded-url.test.ts index d16029803b..47ef4bf73c 100644 --- a/review-enrichment/test/hardcoded-url.test.ts +++ b/review-enrichment/test/hardcoded-url.test.ts @@ -1,88 +1,88 @@ -// Units for the hardcoded-URL analyzer (#2027). Own file so concurrent analyzer PRs don't collide. -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { - detectHardcodedUrl, - scanHardcodedUrl, - scanPatchForHardcodedUrl, -} from "../dist/analyzers/hardcoded-url.js"; -import { renderBrief } from "../dist/render.js"; - -const patchOf = (lines: string[]) => - `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; - -test("detectHardcodedUrl: flags absolute HTTP(S) URLs and IP:port endpoints", () => { - assert.deepEqual(detectHardcodedUrl(`const base = "https://api.prod.example.org/v1";`), { - kind: "http-url", - host: "api.prod.example.org", - }); - assert.deepEqual(detectHardcodedUrl("await fetch('http://10.0.0.5:8080/health');"), { - kind: "http-url", - host: "10.0.0.5", - }); - assert.deepEqual(detectHardcodedUrl("db.connect('192.168.0.12:5432')"), { - kind: "ip-endpoint", - host: "192.168.0.12", - }); -}); - -test("detectHardcodedUrl: allowlists localhost, 127.0.0.1, and example.com", () => { - assert.equal(detectHardcodedUrl("fetch('http://localhost:3000')"), null); - assert.equal(detectHardcodedUrl("fetch('http://127.0.0.1:8080')"), null); - assert.equal(detectHardcodedUrl("fetch('https://api.example.com/v1')"), null); - assert.equal(detectHardcodedUrl("fetch('https://docs.staging.example.com')"), null); -}); - -test("detectHardcodedUrl: skips comment and import lines", () => { - assert.equal(detectHardcodedUrl("// docs: https://evil.example.net/guide"), null); - assert.equal(detectHardcodedUrl("import config from 'https://cdn.example.net/schema.json'"), null); - assert.equal(detectHardcodedUrl("# see https://evil.example.net"), null); -}); - -test("scanPatchForHardcodedUrl: flags added lines with correct locations", () => { - const findings = scanPatchForHardcodedUrl( - "src/client.ts", - patchOf([ - "export async function load() {", - " return fetch('https://billing.internal.example/v1');", - "}", - ]), - ); - assert.deepEqual(findings, [ - { file: "src/client.ts", line: 2, kind: "http-url", host: "billing.internal.example" }, - ]); -}); - -test("scanPatchForHardcodedUrl: skips test and config files", () => { - assert.deepEqual( - scanPatchForHardcodedUrl("src/widget.test.ts", patchOf(["fetch('https://evil.example.net')"])), - [], - ); - assert.deepEqual( - scanPatchForHardcodedUrl("config/settings.yml", patchOf(["url: https://evil.example.net"])), - [], - ); -}); - -test("scanPatchForHardcodedUrl: respects the findings cap", () => { - const lines = Array.from({ length: 30 }, (_, i) => `fetch('https://host${i}.example.net');`); - assert.equal(scanPatchForHardcodedUrl("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); -}); - -test("scanHardcodedUrl: aggregates across files and renders in the brief", async () => { - const findings = await scanHardcodedUrl({ - files: [ - { path: "src/a.ts", patch: patchOf(["fetch('https://api.evil.example.net');"]) }, - { path: "lib/db.py", patch: patchOf(["connect('10.1.2.3:5432')"]) }, - ], - }); - assert.deepEqual(findings, [ - { file: "src/a.ts", line: 1, kind: "http-url", host: "api.evil.example.net" }, - { file: "lib/db.py", line: 1, kind: "ip-endpoint", host: "10.1.2.3" }, - ]); - - const { promptSection } = renderBrief({ hardcodedUrl: findings }); - assert.match(promptSection, /Hardcoded URLs\/endpoints/); - assert.match(promptSection, /src\/a\.ts:1/); - assert.match(promptSection, /lib\/db\.py:1/); -}); +// Units for the hardcoded-URL analyzer (#2027). Own file so concurrent analyzer PRs don't collide. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + detectHardcodedUrl, + scanHardcodedUrl, + scanPatchForHardcodedUrl, +} from "../dist/analyzers/hardcoded-url.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("detectHardcodedUrl: flags absolute HTTP(S) URLs and IP:port endpoints", () => { + assert.deepEqual(detectHardcodedUrl(`const base = "https://api.prod.example.org/v1";`), { + kind: "http-url", + host: "api.prod.example.org", + }); + assert.deepEqual(detectHardcodedUrl("await fetch('http://10.0.0.5:8080/health');"), { + kind: "http-url", + host: "10.0.0.5", + }); + assert.deepEqual(detectHardcodedUrl("db.connect('192.168.0.12:5432')"), { + kind: "ip-endpoint", + host: "192.168.0.12", + }); +}); + +test("detectHardcodedUrl: allowlists localhost, 127.0.0.1, and example.com", () => { + assert.equal(detectHardcodedUrl("fetch('http://localhost:3000')"), null); + assert.equal(detectHardcodedUrl("fetch('http://127.0.0.1:8080')"), null); + assert.equal(detectHardcodedUrl("fetch('https://api.example.com/v1')"), null); + assert.equal(detectHardcodedUrl("fetch('https://docs.staging.example.com')"), null); +}); + +test("detectHardcodedUrl: skips comment and import lines", () => { + assert.equal(detectHardcodedUrl("// docs: https://evil.example.net/guide"), null); + assert.equal(detectHardcodedUrl("import config from 'https://cdn.example.net/schema.json'"), null); + assert.equal(detectHardcodedUrl("# see https://evil.example.net"), null); +}); + +test("scanPatchForHardcodedUrl: flags added lines with correct locations", () => { + const findings = scanPatchForHardcodedUrl( + "src/client.ts", + patchOf([ + "export async function load() {", + " return fetch('https://billing.internal.example/v1');", + "}", + ]), + ); + assert.deepEqual(findings, [ + { file: "src/client.ts", line: 2, kind: "http-url", host: "billing.internal.example" }, + ]); +}); + +test("scanPatchForHardcodedUrl: skips test and config files", () => { + assert.deepEqual( + scanPatchForHardcodedUrl("src/widget.test.ts", patchOf(["fetch('https://evil.example.net')"])), + [], + ); + assert.deepEqual( + scanPatchForHardcodedUrl("config/settings.yml", patchOf(["url: https://evil.example.net"])), + [], + ); +}); + +test("scanPatchForHardcodedUrl: respects the findings cap", () => { + const lines = Array.from({ length: 30 }, (_, i) => `fetch('https://host${i}.example.net');`); + assert.equal(scanPatchForHardcodedUrl("src/a.ts", patchOf(lines), { maxFindings: 3 }).length, 3); +}); + +test("scanHardcodedUrl: aggregates across files and renders in the brief", async () => { + const findings = await scanHardcodedUrl({ + files: [ + { path: "src/a.ts", patch: patchOf(["fetch('https://api.evil.example.net');"]) }, + { path: "lib/db.py", patch: patchOf(["connect('10.1.2.3:5432')"]) }, + ], + }); + assert.deepEqual(findings, [ + { file: "src/a.ts", line: 1, kind: "http-url", host: "api.evil.example.net" }, + { file: "lib/db.py", line: 1, kind: "ip-endpoint", host: "10.1.2.3" }, + ]); + + const { promptSection } = renderBrief({ hardcodedUrl: findings }); + assert.match(promptSection, /Hardcoded URLs\/endpoints/); + assert.match(promptSection, /src\/a\.ts:1/); + assert.match(promptSection, /lib\/db\.py:1/); +}); diff --git a/review-enrichment/test/i18n-regression.test.ts b/review-enrichment/test/i18n-regression.test.ts index fa6405bf8c..83dbd37828 100644 --- a/review-enrichment/test/i18n-regression.test.ts +++ b/review-enrichment/test/i18n-regression.test.ts @@ -1,32 +1,32 @@ -// Units for the i18n regression analyzer (#2029). Own file so concurrent analyzer PRs don't collide. -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { - detectHardcodedUiString, - detectI18nConvention, - looksLikeUserFacingLiteral, - scanI18nRegression, - scanPatchForI18nRegression, -} from "../dist/analyzers/i18n-regression.js"; -import { renderBrief } from "../dist/render.js"; - -const patchOf = (lines: string[]) => - `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; - -test("detectI18nConvention: recognizes common translation call patterns", () => { - assert.equal(detectI18nConvention("const label = t('common.save');"), true); - assert.equal(detectI18nConvention("const { t } = useTranslation();"), true); - assert.equal(detectI18nConvention(""), true); - assert.equal(detectI18nConvention(""), false); -}); - -test("looksLikeUserFacingLiteral: distinguishes copy from keys and ids", () => { - assert.equal(looksLikeUserFacingLiteral("Save changes"), true); - assert.equal(looksLikeUserFacingLiteral("common.save"), false); - assert.equal(looksLikeUserFacingLiteral("btn"), false); - assert.equal(looksLikeUserFacingLiteral("Save"), true); -}); - +// Units for the i18n regression analyzer (#2029). Own file so concurrent analyzer PRs don't collide. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + detectHardcodedUiString, + detectI18nConvention, + looksLikeUserFacingLiteral, + scanI18nRegression, + scanPatchForI18nRegression, +} from "../dist/analyzers/i18n-regression.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("detectI18nConvention: recognizes common translation call patterns", () => { + assert.equal(detectI18nConvention("const label = t('common.save');"), true); + assert.equal(detectI18nConvention("const { t } = useTranslation();"), true); + assert.equal(detectI18nConvention(""), true); + assert.equal(detectI18nConvention(""), false); +}); + +test("looksLikeUserFacingLiteral: distinguishes copy from keys and ids", () => { + assert.equal(looksLikeUserFacingLiteral("Save changes"), true); + assert.equal(looksLikeUserFacingLiteral("common.save"), false); + assert.equal(looksLikeUserFacingLiteral("btn"), false); + assert.equal(looksLikeUserFacingLiteral("Save"), true); +}); + test("detectHardcodedUiString: flags JSX text and user-facing props", () => { assert.equal(detectHardcodedUiString(""), true); assert.equal(detectHardcodedUiString(''), true); @@ -45,51 +45,51 @@ test("detectHardcodedUiString: scans malformed JSX text in linear time", () => { test("scanPatchForI18nRegression: flags hardcoded UI strings when i18n is in use", () => { const findings = scanPatchForI18nRegression( - "src/Widget.tsx", - patchOf([ - "export function Widget() {", - " const { t } = useTranslation();", - " return ;", - "}", - "export function Banner() {", - " return

Welcome back

;", - "}", - ]), - ); - assert.deepEqual(findings, [{ file: "src/Widget.tsx", line: 6 }]); -}); - -test("scanPatchForI18nRegression: inactive when the diff shows no i18n convention", () => { - assert.deepEqual( - scanPatchForI18nRegression("src/Widget.tsx", patchOf(["export function Widget() {", " return

Hello

;", "}"])), - [], - ); -}); - -test("scanPatchForI18nRegression: skips test files and respects the cap", () => { - assert.deepEqual( - scanPatchForI18nRegression("src/Widget.test.tsx", patchOf(["const { t } = useTranslation();", "

Hi

"])), - [], - ); - const lines = [ - "const { t } = useTranslation();", - ...Array.from({ length: 30 }, (_, i) => `

Message ${i}

`), - ]; - assert.equal(scanPatchForI18nRegression("src/a.tsx", patchOf(lines), { maxFindings: 3 }).length, 3); -}); - -test("scanI18nRegression: aggregates across files and renders a public-safe brief", async () => { - const findings = await scanI18nRegression({ - files: [ - { - path: "src/a.tsx", - patch: patchOf(["const { t } = useTranslation();", "x"]), - }, - ], - }); - assert.deepEqual(findings, [{ file: "src/a.tsx", line: 2 }]); - const { promptSection } = renderBrief({ i18n: findings }); - assert.match(promptSection, /i18n regressions/); - assert.match(promptSection, /src\/a\.tsx:2/); - assert.doesNotMatch(promptSection, /Save draft/); -}); + "src/Widget.tsx", + patchOf([ + "export function Widget() {", + " const { t } = useTranslation();", + " return ;", + "}", + "export function Banner() {", + " return

Welcome back

;", + "}", + ]), + ); + assert.deepEqual(findings, [{ file: "src/Widget.tsx", line: 6 }]); +}); + +test("scanPatchForI18nRegression: inactive when the diff shows no i18n convention", () => { + assert.deepEqual( + scanPatchForI18nRegression("src/Widget.tsx", patchOf(["export function Widget() {", " return

Hello

;", "}"])), + [], + ); +}); + +test("scanPatchForI18nRegression: skips test files and respects the cap", () => { + assert.deepEqual( + scanPatchForI18nRegression("src/Widget.test.tsx", patchOf(["const { t } = useTranslation();", "

Hi

"])), + [], + ); + const lines = [ + "const { t } = useTranslation();", + ...Array.from({ length: 30 }, (_, i) => `

Message ${i}

`), + ]; + assert.equal(scanPatchForI18nRegression("src/a.tsx", patchOf(lines), { maxFindings: 3 }).length, 3); +}); + +test("scanI18nRegression: aggregates across files and renders a public-safe brief", async () => { + const findings = await scanI18nRegression({ + files: [ + { + path: "src/a.tsx", + patch: patchOf(["const { t } = useTranslation();", "x"]), + }, + ], + }); + assert.deepEqual(findings, [{ file: "src/a.tsx", line: 2 }]); + const { promptSection } = renderBrief({ i18n: findings }); + assert.match(promptSection, /i18n regressions/); + assert.match(promptSection, /src\/a\.tsx:2/); + assert.doesNotMatch(promptSection, /Save draft/); +}); diff --git a/review-enrichment/test/size-smell.test.ts b/review-enrichment/test/size-smell.test.ts index aff6ee86f5..17d8bda9c6 100644 --- a/review-enrichment/test/size-smell.test.ts +++ b/review-enrichment/test/size-smell.test.ts @@ -1,79 +1,79 @@ -// Units for the size-smell analyzer (#2019). Own file so concurrent analyzer PRs don't collide. -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { - DEFAULT_MAX_FILE_LINES, - DEFAULT_MAX_FUNCTION_LINES, - estimateFileLengthFromPatch, - scanPatchForSizeSmell, - scanSizeSmell, -} from "../dist/analyzers/size-smell.js"; -import { renderBrief } from "../dist/render.js"; - -const patchOf = (lines: string[]) => - `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; - -test("estimateFileLengthFromPatch: reads ending line from hunk headers", () => { - const patch = ["@@ -0,0 +1,3 @@", "+a", "+b", "+c"].join("\n"); - assert.equal(estimateFileLengthFromPatch(patch), 3); - assert.equal( - estimateFileLengthFromPatch(["@@ -10,5 +10,8 @@", "+x"].join("\n")), - 17, - ); -}); - -test("scanPatchForSizeSmell: flags a long resulting file", () => { - const patch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 1} @@\n${"+line\n".repeat(DEFAULT_MAX_FILE_LINES + 1)}`; - assert.deepEqual(scanPatchForSizeSmell("src/big.ts", patch), [ - { - file: "src/big.ts", - kind: "long-file", - measure: DEFAULT_MAX_FILE_LINES + 1, - threshold: DEFAULT_MAX_FILE_LINES, - }, - ]); -}); - -test("scanPatchForSizeSmell: flags a big function body added across lines", () => { - const header = ["function big() {"]; - const body = Array.from({ length: DEFAULT_MAX_FUNCTION_LINES }, (_, i) => ` step${i}();`); - const footer = ["}"]; - const findings = scanPatchForSizeSmell("src/widget.ts", patchOf([...header, ...body, ...footer])); - assert.equal(findings.length, 1); - assert.equal(findings[0]?.kind, "big-function"); - assert.equal(findings[0]?.name, "big"); - assert.equal(findings[0]?.measure, DEFAULT_MAX_FUNCTION_LINES + 2); -}); - -test("scanPatchForSizeSmell: clean sub-threshold file and function", () => { - const lines = ["function small() {", " return 1;", "}"]; - assert.deepEqual(scanPatchForSizeSmell("src/widget.ts", patchOf(lines)), []); -}); - -test("scanPatchForSizeSmell: skips test files and respects the cap", () => { - const longPatch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 1} @@\n${"+line\n".repeat(DEFAULT_MAX_FILE_LINES + 1)}`; - assert.deepEqual(scanPatchForSizeSmell("src/widget.test.ts", longPatch), []); -}); - -test("scanSizeSmell: respects the findings cap across files", async () => { - const longPatch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 1} @@\n${"+line\n".repeat(DEFAULT_MAX_FILE_LINES + 1)}`; - const findings = await scanSizeSmell({ - files: Array.from({ length: 30 }, (_, i) => ({ path: `src/f${i}.ts`, patch: longPatch })), - }); - assert.equal(findings.length, 25); -}); - -test("scanSizeSmell: renders a public-safe brief", async () => { - const findings = await scanSizeSmell({ - files: [{ path: "src/a.ts", patch: patchOf(["function small() {", " return 1;", "}"]) }], - }); - assert.deepEqual(findings, []); - const longPatch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 5} @@\n${"+x\n".repeat(DEFAULT_MAX_FILE_LINES + 5)}`; - const longFindings = await scanSizeSmell({ - files: [{ path: "src/b.ts", patch: longPatch }], - }); - assert.equal(longFindings[0]?.kind, "long-file"); - const { promptSection } = renderBrief({ sizeSmell: longFindings }); - assert.match(promptSection, /Size smells/); - assert.match(promptSection, /src\/b\.ts/); -}); +// Units for the size-smell analyzer (#2019). Own file so concurrent analyzer PRs don't collide. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + DEFAULT_MAX_FILE_LINES, + DEFAULT_MAX_FUNCTION_LINES, + estimateFileLengthFromPatch, + scanPatchForSizeSmell, + scanSizeSmell, +} from "../dist/analyzers/size-smell.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("estimateFileLengthFromPatch: reads ending line from hunk headers", () => { + const patch = ["@@ -0,0 +1,3 @@", "+a", "+b", "+c"].join("\n"); + assert.equal(estimateFileLengthFromPatch(patch), 3); + assert.equal( + estimateFileLengthFromPatch(["@@ -10,5 +10,8 @@", "+x"].join("\n")), + 17, + ); +}); + +test("scanPatchForSizeSmell: flags a long resulting file", () => { + const patch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 1} @@\n${"+line\n".repeat(DEFAULT_MAX_FILE_LINES + 1)}`; + assert.deepEqual(scanPatchForSizeSmell("src/big.ts", patch), [ + { + file: "src/big.ts", + kind: "long-file", + measure: DEFAULT_MAX_FILE_LINES + 1, + threshold: DEFAULT_MAX_FILE_LINES, + }, + ]); +}); + +test("scanPatchForSizeSmell: flags a big function body added across lines", () => { + const header = ["function big() {"]; + const body = Array.from({ length: DEFAULT_MAX_FUNCTION_LINES }, (_, i) => ` step${i}();`); + const footer = ["}"]; + const findings = scanPatchForSizeSmell("src/widget.ts", patchOf([...header, ...body, ...footer])); + assert.equal(findings.length, 1); + assert.equal(findings[0]?.kind, "big-function"); + assert.equal(findings[0]?.name, "big"); + assert.equal(findings[0]?.measure, DEFAULT_MAX_FUNCTION_LINES + 2); +}); + +test("scanPatchForSizeSmell: clean sub-threshold file and function", () => { + const lines = ["function small() {", " return 1;", "}"]; + assert.deepEqual(scanPatchForSizeSmell("src/widget.ts", patchOf(lines)), []); +}); + +test("scanPatchForSizeSmell: skips test files and respects the cap", () => { + const longPatch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 1} @@\n${"+line\n".repeat(DEFAULT_MAX_FILE_LINES + 1)}`; + assert.deepEqual(scanPatchForSizeSmell("src/widget.test.ts", longPatch), []); +}); + +test("scanSizeSmell: respects the findings cap across files", async () => { + const longPatch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 1} @@\n${"+line\n".repeat(DEFAULT_MAX_FILE_LINES + 1)}`; + const findings = await scanSizeSmell({ + files: Array.from({ length: 30 }, (_, i) => ({ path: `src/f${i}.ts`, patch: longPatch })), + }); + assert.equal(findings.length, 25); +}); + +test("scanSizeSmell: renders a public-safe brief", async () => { + const findings = await scanSizeSmell({ + files: [{ path: "src/a.ts", patch: patchOf(["function small() {", " return 1;", "}"]) }], + }); + assert.deepEqual(findings, []); + const longPatch = `@@ -0,0 +1,${DEFAULT_MAX_FILE_LINES + 5} @@\n${"+x\n".repeat(DEFAULT_MAX_FILE_LINES + 5)}`; + const longFindings = await scanSizeSmell({ + files: [{ path: "src/b.ts", patch: longPatch }], + }); + assert.equal(longFindings[0]?.kind, "long-file"); + const { promptSection } = renderBrief({ sizeSmell: longFindings }); + assert.match(promptSection, /Size smells/); + assert.match(promptSection, /src\/b\.ts/); +}); diff --git a/src/review/changed-files-diff-link.ts b/src/review/changed-files-diff-link.ts index 1ab75541de..7ed9e886d5 100644 --- a/src/review/changed-files-diff-link.ts +++ b/src/review/changed-files-diff-link.ts @@ -1,24 +1,24 @@ -/** GitHub PR Files-tab diff anchors for changed-files summary links (#2157). */ - -import { createHash } from "node:crypto"; - -const REPO_FULL_NAME = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; - -/** PURE: SHA-256 hex of the bare repo-relative path — GitHub's `#diff-…` anchor on the Files tab. */ -export function githubPrFileDiffAnchor(path: string): string | null { - const trimmed = path.trim(); - if (!trimmed || trimmed.includes("\0")) return null; - return createHash("sha256").update(trimmed, "utf8").digest("hex"); -} - -/** PURE: public-safe PR Files-tab URL for one changed file, or null when inputs cannot be anchored. */ -export function githubPrFileDiffUrl( - repoFullName: string, - pullNumber: number, - path: string, -): string | null { - if (!REPO_FULL_NAME.test(repoFullName) || !Number.isInteger(pullNumber) || pullNumber <= 0) return null; - const anchor = githubPrFileDiffAnchor(path); - if (anchor === null) return null; - return `https://github.com/${repoFullName}/pull/${pullNumber}/files#diff-${anchor}`; -} +/** GitHub PR Files-tab diff anchors for changed-files summary links (#2157). */ + +import { createHash } from "node:crypto"; + +const REPO_FULL_NAME = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; + +/** PURE: SHA-256 hex of the bare repo-relative path — GitHub's `#diff-…` anchor on the Files tab. */ +export function githubPrFileDiffAnchor(path: string): string | null { + const trimmed = path.trim(); + if (!trimmed || trimmed.includes("\0")) return null; + return createHash("sha256").update(trimmed, "utf8").digest("hex"); +} + +/** PURE: public-safe PR Files-tab URL for one changed file, or null when inputs cannot be anchored. */ +export function githubPrFileDiffUrl( + repoFullName: string, + pullNumber: number, + path: string, +): string | null { + if (!REPO_FULL_NAME.test(repoFullName) || !Number.isInteger(pullNumber) || pullNumber <= 0) return null; + const anchor = githubPrFileDiffAnchor(path); + if (anchor === null) return null; + return `https://github.com/${repoFullName}/pull/${pullNumber}/files#diff-${anchor}`; +} diff --git a/src/review/inline-comment-label.ts b/src/review/inline-comment-label.ts index 4580d52e45..ad8cdd6ff8 100644 --- a/src/review/inline-comment-label.ts +++ b/src/review/inline-comment-label.ts @@ -1,17 +1,17 @@ -/** Pure inline-comment severity/category label rendering (#2149 / #1958). */ - -import { classifyFindingCategory, type FindingCategory } from "./finding-category-classify"; -import type { InlineFinding } from "../services/ai-review"; - -/** Human-readable category name for inline labels — title-cased enum literal, never free text. */ -export function titleCaseFindingCategory(category: FindingCategory): string { - return category.charAt(0).toUpperCase() + category.slice(1); -} - -/** Build the bolded severity prefix for an inline comment (`Blocker · Security`, or severity-only when off). */ -export function formatInlineCommentSeverityLabel(finding: InlineFinding, categoriesEnabled: boolean): string { - const severityLabel = finding.severity === "blocker" ? "Blocker" : "Nit"; - if (!categoriesEnabled) return severityLabel; - const category = finding.category ?? classifyFindingCategory(finding); - return `${severityLabel} · ${titleCaseFindingCategory(category)}`; -} +/** Pure inline-comment severity/category label rendering (#2149 / #1958). */ + +import { classifyFindingCategory, type FindingCategory } from "./finding-category-classify"; +import type { InlineFinding } from "../services/ai-review"; + +/** Human-readable category name for inline labels — title-cased enum literal, never free text. */ +export function titleCaseFindingCategory(category: FindingCategory): string { + return category.charAt(0).toUpperCase() + category.slice(1); +} + +/** Build the bolded severity prefix for an inline comment (`Blocker · Security`, or severity-only when off). */ +export function formatInlineCommentSeverityLabel(finding: InlineFinding, categoriesEnabled: boolean): string { + const severityLabel = finding.severity === "blocker" ? "Blocker" : "Nit"; + if (!categoriesEnabled) return severityLabel; + const category = finding.category ?? classifyFindingCategory(finding); + return `${severityLabel} · ${titleCaseFindingCategory(category)}`; +} diff --git a/src/review/inline-comment-range.ts b/src/review/inline-comment-range.ts index 0211ed93ff..79b38f0d25 100644 --- a/src/review/inline-comment-range.ts +++ b/src/review/inline-comment-range.ts @@ -1,38 +1,38 @@ -/** Multi-line inline comment range validation (#2141). */ - -import { rightSideLinesFromPatch } from "./inline-comments-select"; -import type { InlineFinding } from "../services/ai-review"; -import type { PullRequestFileRecord } from "../types"; - -/** Normalized [start, end] for an inline finding — `line` is always the start; invalid/inverted/absent `endLine` - * collapses to a single-line anchor. */ -export function parseInlineLineRange(finding: Pick): { start: number; end: number } { - const start = finding.line; - const end = finding.endLine != null && finding.endLine > start ? finding.endLine : start; - return { start, end }; -} - -/** True when every line in the inclusive [start, end] range is present in `lines`. */ -export function everyLineInSet(start: number, end: number, lines: Set): boolean { - for (let line = start; line <= end; line += 1) { - if (!lines.has(line)) return false; - } - return true; -} - -/** Build per-file RIGHT-side commentable line sets from PR file records. */ -export function rightLinesByPath( - files: Pick[], -): Map> { - const out = new Map>(); - for (const file of files) { - const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (patch) out.set(file.path, rightSideLinesFromPatch(patch)); - } - return out; -} - -/** Resolve the GitHub inline-comment anchor for a finding. Multi-line ONLY when every line in [start,end] is +/** Multi-line inline comment range validation (#2141). */ + +import { rightSideLinesFromPatch } from "./inline-comments-select"; +import type { InlineFinding } from "../services/ai-review"; +import type { PullRequestFileRecord } from "../types"; + +/** Normalized [start, end] for an inline finding — `line` is always the start; invalid/inverted/absent `endLine` + * collapses to a single-line anchor. */ +export function parseInlineLineRange(finding: Pick): { start: number; end: number } { + const start = finding.line; + const end = finding.endLine != null && finding.endLine > start ? finding.endLine : start; + return { start, end }; +} + +/** True when every line in the inclusive [start, end] range is present in `lines`. */ +export function everyLineInSet(start: number, end: number, lines: Set): boolean { + for (let line = start; line <= end; line += 1) { + if (!lines.has(line)) return false; + } + return true; +} + +/** Build per-file RIGHT-side commentable line sets from PR file records. */ +export function rightLinesByPath( + files: Pick[], +): Map> { + const out = new Map>(); + for (const file of files) { + const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; + if (patch) out.set(file.path, rightSideLinesFromPatch(patch)); + } + return out; +} + +/** Resolve the GitHub inline-comment anchor for a finding. Multi-line ONLY when every line in [start,end] is * commentable on the RIGHT side; otherwise downgrade to the single start line (fail-safe, no 422). * `anchorable` is false when `start` ITSELF is not a commentable RIGHT-side line -- the fallback used to * return that start line anyway, presenting an un-postable anchor as a safe one and leaving the "no 422" @@ -51,4 +51,4 @@ export function resolveInlineCommentAnchor( } if (end > start) return { start, end, multiLine: true, anchorable: true }; return { start, end: start, multiLine: false, anchorable: true }; -} +} diff --git a/src/review/inline-suggestion-anchor.ts b/src/review/inline-suggestion-anchor.ts index c6009d575b..ac13e372bb 100644 --- a/src/review/inline-suggestion-anchor.ts +++ b/src/review/inline-suggestion-anchor.ts @@ -1,15 +1,15 @@ -/** Suggestion anchor-safety for inline PR review comments (#2140). */ - -import { parseInlineLineRange } from "./inline-comment-range"; -import type { InlineFinding } from "../services/ai-review"; -import type { PullRequestFileRecord } from "../types"; - -/** PURE: RIGHT-side line numbers that are ADDED ("+") in a unified-diff patch — the only lines GitHub - * accepts a ```suggestion block on. Context lines are commentable for plain inline notes but not for - * suggested changes. */ -export function addedLinesFromPatch(patch: string): Set { - const lines = new Set(); - let right = 0; +/** Suggestion anchor-safety for inline PR review comments (#2140). */ + +import { parseInlineLineRange } from "./inline-comment-range"; +import type { InlineFinding } from "../services/ai-review"; +import type { PullRequestFileRecord } from "../types"; + +/** PURE: RIGHT-side line numbers that are ADDED ("+") in a unified-diff patch — the only lines GitHub + * accepts a ```suggestion block on. Context lines are commentable for plain inline notes but not for + * suggested changes. */ +export function addedLinesFromPatch(patch: string): Set { + const lines = new Set(); + let right = 0; // Mirror rightSideLinesFromPatch (inline-comments-select.ts) line-for-line so both walkers agree on every // RIGHT-side line number (#9663). A patch ending in "\n" splits to a trailing empty element that is a split // artifact, not a diff line; drop it here so that a remaining empty element genuinely means "a context line @@ -17,62 +17,62 @@ export function addedLinesFromPatch(patch: string): Set { const rawLines = patch.split("\n"); if (rawLines.length > 0 && rawLines[rawLines.length - 1] === "") rawLines.pop(); for (const raw of rawLines) { - const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); - if (header?.[1]) { - right = Number.parseInt(header[1], 10); - continue; - } - if (right === 0) continue; - const marker = raw[0]; + const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); + if (header?.[1]) { + right = Number.parseInt(header[1], 10); + continue; + } + if (right === 0) continue; + const marker = raw[0]; if (marker === "-" || marker === "\\") continue; // A zero-length patch line (marker `undefined`) is a context line whose single space was stripped, not a // line that does not exist (#9076/#9663). It is NOT added, but it must still advance `right` — skipping it // desynced every subsequent added-line number, so a blocker anchored after a blank context line was dropped // and one anchored on the blank context line itself was wrongly accepted. - if (marker === "+") lines.add(right); - right += 1; - } - return lines; -} - -/** Build per-file ADDED-line sets from PR file records — skips files with empty or non-string patches. */ -export function addedLinesByPath( - files: Pick[], -): Map> { - const out = new Map>(); - for (const file of files) { - const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (patch) out.set(file.path, addedLinesFromPatch(patch)); - } - return out; -} - -/** True when a finding's line is an ADDED RIGHT-side line that can carry a ```suggestion block. */ -export function isSuggestionAnchorable( - finding: Pick, - addedLines: Map>, -): boolean { - const validLines = addedLines.get(finding.path); - if (validLines == null) return false; - const { start, end } = parseInlineLineRange(finding); - for (let line = start; line <= end; line += 1) { - if (!validLines.has(line)) return false; - } - return true; -} - -/** GitHub suggestion fence — dropped when blank or when the text would break the fence (#1956). */ -export function safeSuggestionBlock(suggestion: string | undefined): string { - if (!suggestion || suggestion.includes("```")) return ""; - return `\n\n\`\`\`suggestion\n${suggestion}\n\`\`\``; -} - -/** Render a suggestion block only when enabled and the anchor is an added RIGHT-side line (#2140). */ -export function anchoredSuggestionBlock( - finding: InlineFinding, - suggestionsEnabled: boolean, - addedLines: Map>, -): string { - if (!suggestionsEnabled || !isSuggestionAnchorable(finding, addedLines)) return ""; - return safeSuggestionBlock(finding.suggestion); -} + if (marker === "+") lines.add(right); + right += 1; + } + return lines; +} + +/** Build per-file ADDED-line sets from PR file records — skips files with empty or non-string patches. */ +export function addedLinesByPath( + files: Pick[], +): Map> { + const out = new Map>(); + for (const file of files) { + const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; + if (patch) out.set(file.path, addedLinesFromPatch(patch)); + } + return out; +} + +/** True when a finding's line is an ADDED RIGHT-side line that can carry a ```suggestion block. */ +export function isSuggestionAnchorable( + finding: Pick, + addedLines: Map>, +): boolean { + const validLines = addedLines.get(finding.path); + if (validLines == null) return false; + const { start, end } = parseInlineLineRange(finding); + for (let line = start; line <= end; line += 1) { + if (!validLines.has(line)) return false; + } + return true; +} + +/** GitHub suggestion fence — dropped when blank or when the text would break the fence (#1956). */ +export function safeSuggestionBlock(suggestion: string | undefined): string { + if (!suggestion || suggestion.includes("```")) return ""; + return `\n\n\`\`\`suggestion\n${suggestion}\n\`\`\``; +} + +/** Render a suggestion block only when enabled and the anchor is an added RIGHT-side line (#2140). */ +export function anchoredSuggestionBlock( + finding: InlineFinding, + suggestionsEnabled: boolean, + addedLines: Map>, +): string { + if (!suggestionsEnabled || !isSuggestionAnchorable(finding, addedLines)) return ""; + return safeSuggestionBlock(finding.suggestion); +} diff --git a/test/unit/changed-files-diff-link.test.ts b/test/unit/changed-files-diff-link.test.ts index 3a712f1915..c36cdb41cc 100644 --- a/test/unit/changed-files-diff-link.test.ts +++ b/test/unit/changed-files-diff-link.test.ts @@ -1,31 +1,31 @@ -import { describe, expect, it } from "vitest"; -import { githubPrFileDiffAnchor, githubPrFileDiffUrl } from "../../src/review/changed-files-diff-link"; - -describe("githubPrFileDiffAnchor (#2157)", () => { - it("returns the SHA-256 hex of the bare repo-relative path", () => { - expect(githubPrFileDiffAnchor("src/app.ts")).toBe( - "841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27b", - ); - }); - - it("returns null for empty or whitespace-only paths", () => { - expect(githubPrFileDiffAnchor("")).toBeNull(); - expect(githubPrFileDiffAnchor(" ")).toBeNull(); - expect(githubPrFileDiffAnchor("\0bad")).toBeNull(); - }); -}); - -describe("githubPrFileDiffUrl (#2157)", () => { - it("builds a public-safe Files-tab URL with the diff anchor", () => { - expect(githubPrFileDiffUrl("acme/widgets", 42, "src/app.ts")).toBe( - "https://github.com/acme/widgets/pull/42/files#diff-841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27b", - ); - }); - - it("returns null for invalid repo, PR number, or path", () => { - expect(githubPrFileDiffUrl("not-a-repo", 1, "src/a.ts")).toBeNull(); - expect(githubPrFileDiffUrl("acme/widgets", 0, "src/a.ts")).toBeNull(); - expect(githubPrFileDiffUrl("acme/widgets", 1.5, "src/a.ts")).toBeNull(); - expect(githubPrFileDiffUrl("acme/widgets", 1, "")).toBeNull(); - }); -}); +import { describe, expect, it } from "vitest"; +import { githubPrFileDiffAnchor, githubPrFileDiffUrl } from "../../src/review/changed-files-diff-link"; + +describe("githubPrFileDiffAnchor (#2157)", () => { + it("returns the SHA-256 hex of the bare repo-relative path", () => { + expect(githubPrFileDiffAnchor("src/app.ts")).toBe( + "841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27b", + ); + }); + + it("returns null for empty or whitespace-only paths", () => { + expect(githubPrFileDiffAnchor("")).toBeNull(); + expect(githubPrFileDiffAnchor(" ")).toBeNull(); + expect(githubPrFileDiffAnchor("\0bad")).toBeNull(); + }); +}); + +describe("githubPrFileDiffUrl (#2157)", () => { + it("builds a public-safe Files-tab URL with the diff anchor", () => { + expect(githubPrFileDiffUrl("acme/widgets", 42, "src/app.ts")).toBe( + "https://github.com/acme/widgets/pull/42/files#diff-841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27b", + ); + }); + + it("returns null for invalid repo, PR number, or path", () => { + expect(githubPrFileDiffUrl("not-a-repo", 1, "src/a.ts")).toBeNull(); + expect(githubPrFileDiffUrl("acme/widgets", 0, "src/a.ts")).toBeNull(); + expect(githubPrFileDiffUrl("acme/widgets", 1.5, "src/a.ts")).toBeNull(); + expect(githubPrFileDiffUrl("acme/widgets", 1, "")).toBeNull(); + }); +}); diff --git a/test/unit/inline-comment-label.test.ts b/test/unit/inline-comment-label.test.ts index d7b5da4289..efa1f994a1 100644 --- a/test/unit/inline-comment-label.test.ts +++ b/test/unit/inline-comment-label.test.ts @@ -1,31 +1,31 @@ -import { describe, expect, it } from "vitest"; -import { formatInlineCommentSeverityLabel, titleCaseFindingCategory } from "../../src/review/inline-comment-label"; -import type { InlineFinding } from "../../src/services/ai-review"; - -describe("titleCaseFindingCategory (#2149)", () => { - it("title-cases every fixed finding-category enum literal", () => { - expect(titleCaseFindingCategory("security")).toBe("Security"); - expect(titleCaseFindingCategory("correctness")).toBe("Correctness"); - expect(titleCaseFindingCategory("style")).toBe("Style"); - }); -}); - -describe("formatInlineCommentSeverityLabel (#2149)", () => { - const blocker: InlineFinding = { path: "src/a.ts", line: 1, severity: "blocker", body: "x", category: "security" }; - const nit: InlineFinding = { path: "src/a.ts", line: 1, severity: "nit", body: "x", category: "style" }; - - it("returns severity-only labels when categories are disabled", () => { - expect(formatInlineCommentSeverityLabel(blocker, false)).toBe("Blocker"); - expect(formatInlineCommentSeverityLabel(nit, false)).toBe("Nit"); - }); - - it("renders blocker and nit labels with a title-cased category when enabled", () => { - expect(formatInlineCommentSeverityLabel(blocker, true)).toBe("Blocker · Security"); - expect(formatInlineCommentSeverityLabel(nit, true)).toBe("Nit · Style"); - }); - - it("falls back to the deterministic classifier when the finding omits category", () => { - const uncategorized: InlineFinding = { path: "src/app.test.ts", line: 1, severity: "nit", body: "Use const." }; - expect(formatInlineCommentSeverityLabel(uncategorized, true)).toBe("Nit · Tests"); - }); -}); +import { describe, expect, it } from "vitest"; +import { formatInlineCommentSeverityLabel, titleCaseFindingCategory } from "../../src/review/inline-comment-label"; +import type { InlineFinding } from "../../src/services/ai-review"; + +describe("titleCaseFindingCategory (#2149)", () => { + it("title-cases every fixed finding-category enum literal", () => { + expect(titleCaseFindingCategory("security")).toBe("Security"); + expect(titleCaseFindingCategory("correctness")).toBe("Correctness"); + expect(titleCaseFindingCategory("style")).toBe("Style"); + }); +}); + +describe("formatInlineCommentSeverityLabel (#2149)", () => { + const blocker: InlineFinding = { path: "src/a.ts", line: 1, severity: "blocker", body: "x", category: "security" }; + const nit: InlineFinding = { path: "src/a.ts", line: 1, severity: "nit", body: "x", category: "style" }; + + it("returns severity-only labels when categories are disabled", () => { + expect(formatInlineCommentSeverityLabel(blocker, false)).toBe("Blocker"); + expect(formatInlineCommentSeverityLabel(nit, false)).toBe("Nit"); + }); + + it("renders blocker and nit labels with a title-cased category when enabled", () => { + expect(formatInlineCommentSeverityLabel(blocker, true)).toBe("Blocker · Security"); + expect(formatInlineCommentSeverityLabel(nit, true)).toBe("Nit · Style"); + }); + + it("falls back to the deterministic classifier when the finding omits category", () => { + const uncategorized: InlineFinding = { path: "src/app.test.ts", line: 1, severity: "nit", body: "Use const." }; + expect(formatInlineCommentSeverityLabel(uncategorized, true)).toBe("Nit · Tests"); + }); +}); diff --git a/test/unit/inline-comment-range.test.ts b/test/unit/inline-comment-range.test.ts index 853e19b7ba..80dd7f3359 100644 --- a/test/unit/inline-comment-range.test.ts +++ b/test/unit/inline-comment-range.test.ts @@ -1,69 +1,69 @@ -import { describe, expect, it } from "vitest"; -import { - everyLineInSet, - parseInlineLineRange, - resolveInlineCommentAnchor, - rightLinesByPath, -} from "../../src/review/inline-comment-range"; - -const multiPatch = "@@ -1,0 +1,3 @@\n+one\n+two\n+three"; -const mixedPatch = "@@ -1,2 +1,4 @@\n ctx\n+add2\n ctx4\n+add4"; - -describe("parseInlineLineRange (#2141)", () => { - it("collapses absent, equal, or inverted endLine to a single-line range", () => { - expect(parseInlineLineRange({ line: 2 })).toEqual({ start: 2, end: 2 }); - expect(parseInlineLineRange({ line: 2, endLine: 2 })).toEqual({ start: 2, end: 2 }); - expect(parseInlineLineRange({ line: 5, endLine: 3 })).toEqual({ start: 5, end: 5 }); - }); - - it("keeps a valid forward range", () => { - expect(parseInlineLineRange({ line: 1, endLine: 3 })).toEqual({ start: 1, end: 3 }); - }); -}); - -describe("everyLineInSet (#2141)", () => { - it("requires every line in the inclusive range", () => { - const lines = new Set([1, 2, 3]); - expect(everyLineInSet(1, 3, lines)).toBe(true); - expect(everyLineInSet(1, 4, lines)).toBe(false); - }); -}); - -describe("rightLinesByPath (#2141)", () => { - it("omits files with empty or non-string patches", () => { - const map = rightLinesByPath([ - { path: "src/empty.ts", payload: { patch: "" } }, - { path: "src/bad.ts", payload: { patch: 42 as unknown as string } }, - { path: "src/a.ts", payload: { patch: multiPatch } }, - ]); - expect(map.size).toBe(1); - expect(map.has("src/a.ts")).toBe(true); - }); -}); - -describe("resolveInlineCommentAnchor (#2141)", () => { - const files = [{ path: "src/a.ts", payload: { patch: multiPatch } }]; - - it("emits a multi-line anchor when every line in the range is commentable", () => { - const rightLines = rightLinesByPath(files); - expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 1, endLine: 3 }, rightLines)).toEqual({ +import { describe, expect, it } from "vitest"; +import { + everyLineInSet, + parseInlineLineRange, + resolveInlineCommentAnchor, + rightLinesByPath, +} from "../../src/review/inline-comment-range"; + +const multiPatch = "@@ -1,0 +1,3 @@\n+one\n+two\n+three"; +const mixedPatch = "@@ -1,2 +1,4 @@\n ctx\n+add2\n ctx4\n+add4"; + +describe("parseInlineLineRange (#2141)", () => { + it("collapses absent, equal, or inverted endLine to a single-line range", () => { + expect(parseInlineLineRange({ line: 2 })).toEqual({ start: 2, end: 2 }); + expect(parseInlineLineRange({ line: 2, endLine: 2 })).toEqual({ start: 2, end: 2 }); + expect(parseInlineLineRange({ line: 5, endLine: 3 })).toEqual({ start: 5, end: 5 }); + }); + + it("keeps a valid forward range", () => { + expect(parseInlineLineRange({ line: 1, endLine: 3 })).toEqual({ start: 1, end: 3 }); + }); +}); + +describe("everyLineInSet (#2141)", () => { + it("requires every line in the inclusive range", () => { + const lines = new Set([1, 2, 3]); + expect(everyLineInSet(1, 3, lines)).toBe(true); + expect(everyLineInSet(1, 4, lines)).toBe(false); + }); +}); + +describe("rightLinesByPath (#2141)", () => { + it("omits files with empty or non-string patches", () => { + const map = rightLinesByPath([ + { path: "src/empty.ts", payload: { patch: "" } }, + { path: "src/bad.ts", payload: { patch: 42 as unknown as string } }, + { path: "src/a.ts", payload: { patch: multiPatch } }, + ]); + expect(map.size).toBe(1); + expect(map.has("src/a.ts")).toBe(true); + }); +}); + +describe("resolveInlineCommentAnchor (#2141)", () => { + const files = [{ path: "src/a.ts", payload: { patch: multiPatch } }]; + + it("emits a multi-line anchor when every line in the range is commentable", () => { + const rightLines = rightLinesByPath(files); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 1, endLine: 3 }, rightLines)).toEqual({ start: 1, end: 3, multiLine: true, anchorable: true, - }); - }); - - it("downgrades to the start line when any line in the range is not commentable", () => { - const rightLines = rightLinesByPath([{ path: "src/a.ts", payload: { patch: mixedPatch } }]); - expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 2, endLine: 99 }, rightLines)).toEqual({ + }); + }); + + it("downgrades to the start line when any line in the range is not commentable", () => { + const rightLines = rightLinesByPath([{ path: "src/a.ts", payload: { patch: mixedPatch } }]); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 2, endLine: 99 }, rightLines)).toEqual({ start: 2, end: 2, multiLine: false, anchorable: true, - }); - }); - + }); + }); + it("reports NOT anchorable when the file path is missing from the RIGHT-side line map (#8352)", () => { // Previously this returned start line 1 as a "safe" single-line anchor even though no line on this // path was ever validated -- an un-postable anchor presented as postable (the 422 this guards). @@ -83,15 +83,15 @@ describe("resolveInlineCommentAnchor (#2141)", () => { multiLine: false, anchorable: false, }); - }); - - it("keeps a single-line anchor when the range collapses to one commentable line", () => { - const rightLines = rightLinesByPath(files); - expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 2 }, rightLines)).toEqual({ + }); + + it("keeps a single-line anchor when the range collapses to one commentable line", () => { + const rightLines = rightLinesByPath(files); + expect(resolveInlineCommentAnchor({ path: "src/a.ts", line: 2 }, rightLines)).toEqual({ start: 2, end: 2, multiLine: false, anchorable: true, - }); - }); -}); + }); + }); +}); diff --git a/test/unit/inline-comments-select.test.ts b/test/unit/inline-comments-select.test.ts index 9206765eda..d6f6fd291e 100644 --- a/test/unit/inline-comments-select.test.ts +++ b/test/unit/inline-comments-select.test.ts @@ -1,143 +1,143 @@ -import { describe, expect, it } from "vitest"; -import type { InlineFinding } from "../../src/services/ai-review"; -import { - compareInlineFindingPriority, - DEFAULT_MAX_INLINE_COMMENTS, - inlineFindingCategory, - rightSideLinesFromPatch, - selectAnchoredInlineFindings, -} from "../../src/review/inline-comments-select"; - -const fileWith = (path: string, patch: string) => ({ path, payload: { patch } }); -const files = [fileWith("src/a.ts", "@@ -1,0 +1,6 @@\n+1\n+2\n+3\n+4\n+5\n+6")]; - -describe("rightSideLinesFromPatch (#2159)", () => { - it("returns RIGHT-side line numbers and ignores deleted/no-newline markers", () => { - const patch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n+added3\n ctx4\n\\ No newline at end of file"; - expect([...rightSideLinesFromPatch(patch)].sort((a, b) => a - b)).toEqual([1, 2, 3, 4]); - expect(rightSideLinesFromPatch("").size).toBe(0); - }); - - it("ignores trailing split artifacts and preamble before the first hunk", () => { - expect([...rightSideLinesFromPatch("@@ -1,1 +1,2 @@\n ctx\n+added2\n")].sort((a, b) => a - b)).toEqual([1, 2]); - expect(rightSideLinesFromPatch("preamble only").size).toBe(0); - }); -}); - -describe("inlineFindingCategory + compareInlineFindingPriority (#2159)", () => { - it("uses the model category when present and falls back otherwise", () => { - expect(inlineFindingCategory({ path: "src/a.ts", line: 1, severity: "nit", body: "x", category: "security" })).toBe("security"); - expect(inlineFindingCategory({ path: "src/app.test.ts", line: 1, severity: "nit", body: "x" })).toBe("tests"); - }); - - it("ranks blockers ahead of nits and higher-priority categories ahead of style", () => { - const securityBlocker: InlineFinding = { path: "src/a.ts", line: 1, severity: "blocker", body: "x", category: "security" }; - const styleNit: InlineFinding = { path: "src/a.ts", line: 2, severity: "nit", body: "y", category: "style" }; - const performanceNit: InlineFinding = { path: "src/a.ts", line: 3, severity: "nit", body: "z", category: "performance" }; - const correctnessBlocker: InlineFinding = { path: "src/a.ts", line: 4, severity: "blocker", body: "c", category: "correctness" }; - expect(compareInlineFindingPriority(securityBlocker, styleNit)).toBeLessThan(0); - expect(compareInlineFindingPriority(performanceNit, styleNit)).toBeLessThan(0); - expect(compareInlineFindingPriority(securityBlocker, correctnessBlocker)).toBeLessThan(0); - expect(compareInlineFindingPriority(styleNit, styleNit)).toBe(0); - }); -}); - -describe("selectAnchoredInlineFindings (#2159)", () => { - it("drops unanchorable, duplicate, and below-threshold findings before capping", () => { - const findings: InlineFinding[] = [ - { path: "src/a.ts", line: 1, severity: "nit", body: "ok", category: "style" }, - { path: "src/a.ts", line: 1, severity: "blocker", body: "dup", category: "security" }, - { path: "src/a.ts", line: 99, severity: "nit", body: "missing", category: "style" }, - { path: "src/missing.ts", line: 1, severity: "nit", body: "unknown file", category: "style" }, - { path: "src/no-patch.ts", line: 1, severity: "nit", body: "no patch", category: "style" }, - ]; - const mixedFiles = [ - ...files, - { path: "src/no-patch.ts", payload: {} }, - ]; - expect( - selectAnchoredInlineFindings(findings, mixedFiles, { minFindingSeverity: "major" }).map((finding) => finding.body), - ).toEqual(["dup"]); - expect(DEFAULT_MAX_INLINE_COMMENTS).toBe(10); - }); - - it("preserves first-seen order when perCategoryCap is unset", () => { - const findings: InlineFinding[] = [ - { path: "src/a.ts", line: 1, severity: "nit", body: "first", category: "style" }, - { path: "src/a.ts", line: 2, severity: "blocker", body: "second", category: "security" }, - { path: "src/a.ts", line: 3, severity: "nit", body: "third", category: "style" }, - ]; - expect(selectAnchoredInlineFindings(findings, files, {}).map((f) => f.body)).toEqual(["first", "second", "third"]); - }); - - it("trims overflowing categories and keeps higher-priority findings when perCategoryCap is set", () => { - const findings: InlineFinding[] = [ - { path: "src/a.ts", line: 1, severity: "nit", body: "style-1", category: "style" }, - { path: "src/a.ts", line: 2, severity: "nit", body: "style-2", category: "style" }, - { path: "src/a.ts", line: 3, severity: "nit", body: "style-3", category: "style" }, - { path: "src/a.ts", line: 4, severity: "blocker", body: "security", category: "security" }, - { path: "src/a.ts", line: 5, severity: "nit", body: "style-4", category: "style" }, - ]; - const selected = selectAnchoredInlineFindings(findings, files, { perCategoryCap: 2, maxComments: 10 }); - expect(selected.map((f) => f.body)).toEqual(["security", "style-1", "style-2"]); - expect(selected.filter((f) => inlineFindingCategory(f) === "style")).toHaveLength(2); - }); - - it("preserves first-seen index order among equal-priority findings when sorting for caps", () => { - const findings: InlineFinding[] = [ - { path: "src/a.ts", line: 1, severity: "nit", body: "first", category: "style" }, - { path: "src/a.ts", line: 2, severity: "nit", body: "second", category: "style" }, - ]; - expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 1 }).map((finding) => finding.body)).toEqual(["first"]); - }); - - it("dedupes same path:line anchors before capping", () => { - const findings: InlineFinding[] = [ - { path: "src/a.ts", line: 1, severity: "blocker", body: "first", category: "security" }, - { path: "src/a.ts", line: 1, severity: "blocker", body: "duplicate", category: "security" }, - ]; - expect(selectAnchoredInlineFindings(findings, files, {}).map((finding) => finding.body)).toEqual(["first"]); - }); - - it("skips every category when perCategoryCap is zero", () => { - const findings: InlineFinding[] = [{ path: "src/a.ts", line: 1, severity: "nit", body: "style", category: "style" }]; - expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 0 })).toEqual([]); - }); - - it("ignores files with empty or missing patch content", () => { - const findings: InlineFinding[] = [{ path: "src/empty.ts", line: 1, severity: "nit", body: "missing patch" }]; - expect(selectAnchoredInlineFindings(findings, [{ path: "src/empty.ts", payload: { patch: "" } }], {})).toEqual([]); - expect( - selectAnchoredInlineFindings(findings, [{ path: "src/empty.ts", payload: { patch: 42 as unknown as string } }], {}), - ).toEqual([]); - expect(rightSideLinesFromPatch("preamble only").size).toBe(0); - }); - - it("breaks on the total cap without per-category sorting when perCategoryCap is unset", () => { - const twelveLineFiles = [ - fileWith( - "src/a.ts", - "@@ -1,0 +1,12 @@\n+1\n+2\n+3\n+4\n+5\n+6\n+7\n+8\n+9\n+10\n+11\n+12", - ), - ]; - const findings: InlineFinding[] = Array.from({ length: 12 }, (_, index) => ({ - path: "src/a.ts", - line: index + 1, - severity: "nit" as const, - body: `body-${index + 1}`, - category: "style" as const, - })); - expect(selectAnchoredInlineFindings(findings, twelveLineFiles, {})).toHaveLength(DEFAULT_MAX_INLINE_COMMENTS); - }); - - it("still enforces the total cap after per-category trimming", () => { - const findings: InlineFinding[] = Array.from({ length: 6 }, (_, index) => ({ - path: "src/a.ts", - line: index + 1, - severity: "nit" as const, - body: `body-${index + 1}`, - category: "style" as const, - })); - expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 5, maxComments: 3 })).toHaveLength(3); - }); -}); +import { describe, expect, it } from "vitest"; +import type { InlineFinding } from "../../src/services/ai-review"; +import { + compareInlineFindingPriority, + DEFAULT_MAX_INLINE_COMMENTS, + inlineFindingCategory, + rightSideLinesFromPatch, + selectAnchoredInlineFindings, +} from "../../src/review/inline-comments-select"; + +const fileWith = (path: string, patch: string) => ({ path, payload: { patch } }); +const files = [fileWith("src/a.ts", "@@ -1,0 +1,6 @@\n+1\n+2\n+3\n+4\n+5\n+6")]; + +describe("rightSideLinesFromPatch (#2159)", () => { + it("returns RIGHT-side line numbers and ignores deleted/no-newline markers", () => { + const patch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n+added3\n ctx4\n\\ No newline at end of file"; + expect([...rightSideLinesFromPatch(patch)].sort((a, b) => a - b)).toEqual([1, 2, 3, 4]); + expect(rightSideLinesFromPatch("").size).toBe(0); + }); + + it("ignores trailing split artifacts and preamble before the first hunk", () => { + expect([...rightSideLinesFromPatch("@@ -1,1 +1,2 @@\n ctx\n+added2\n")].sort((a, b) => a - b)).toEqual([1, 2]); + expect(rightSideLinesFromPatch("preamble only").size).toBe(0); + }); +}); + +describe("inlineFindingCategory + compareInlineFindingPriority (#2159)", () => { + it("uses the model category when present and falls back otherwise", () => { + expect(inlineFindingCategory({ path: "src/a.ts", line: 1, severity: "nit", body: "x", category: "security" })).toBe("security"); + expect(inlineFindingCategory({ path: "src/app.test.ts", line: 1, severity: "nit", body: "x" })).toBe("tests"); + }); + + it("ranks blockers ahead of nits and higher-priority categories ahead of style", () => { + const securityBlocker: InlineFinding = { path: "src/a.ts", line: 1, severity: "blocker", body: "x", category: "security" }; + const styleNit: InlineFinding = { path: "src/a.ts", line: 2, severity: "nit", body: "y", category: "style" }; + const performanceNit: InlineFinding = { path: "src/a.ts", line: 3, severity: "nit", body: "z", category: "performance" }; + const correctnessBlocker: InlineFinding = { path: "src/a.ts", line: 4, severity: "blocker", body: "c", category: "correctness" }; + expect(compareInlineFindingPriority(securityBlocker, styleNit)).toBeLessThan(0); + expect(compareInlineFindingPriority(performanceNit, styleNit)).toBeLessThan(0); + expect(compareInlineFindingPriority(securityBlocker, correctnessBlocker)).toBeLessThan(0); + expect(compareInlineFindingPriority(styleNit, styleNit)).toBe(0); + }); +}); + +describe("selectAnchoredInlineFindings (#2159)", () => { + it("drops unanchorable, duplicate, and below-threshold findings before capping", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "ok", category: "style" }, + { path: "src/a.ts", line: 1, severity: "blocker", body: "dup", category: "security" }, + { path: "src/a.ts", line: 99, severity: "nit", body: "missing", category: "style" }, + { path: "src/missing.ts", line: 1, severity: "nit", body: "unknown file", category: "style" }, + { path: "src/no-patch.ts", line: 1, severity: "nit", body: "no patch", category: "style" }, + ]; + const mixedFiles = [ + ...files, + { path: "src/no-patch.ts", payload: {} }, + ]; + expect( + selectAnchoredInlineFindings(findings, mixedFiles, { minFindingSeverity: "major" }).map((finding) => finding.body), + ).toEqual(["dup"]); + expect(DEFAULT_MAX_INLINE_COMMENTS).toBe(10); + }); + + it("preserves first-seen order when perCategoryCap is unset", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "first", category: "style" }, + { path: "src/a.ts", line: 2, severity: "blocker", body: "second", category: "security" }, + { path: "src/a.ts", line: 3, severity: "nit", body: "third", category: "style" }, + ]; + expect(selectAnchoredInlineFindings(findings, files, {}).map((f) => f.body)).toEqual(["first", "second", "third"]); + }); + + it("trims overflowing categories and keeps higher-priority findings when perCategoryCap is set", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "style-1", category: "style" }, + { path: "src/a.ts", line: 2, severity: "nit", body: "style-2", category: "style" }, + { path: "src/a.ts", line: 3, severity: "nit", body: "style-3", category: "style" }, + { path: "src/a.ts", line: 4, severity: "blocker", body: "security", category: "security" }, + { path: "src/a.ts", line: 5, severity: "nit", body: "style-4", category: "style" }, + ]; + const selected = selectAnchoredInlineFindings(findings, files, { perCategoryCap: 2, maxComments: 10 }); + expect(selected.map((f) => f.body)).toEqual(["security", "style-1", "style-2"]); + expect(selected.filter((f) => inlineFindingCategory(f) === "style")).toHaveLength(2); + }); + + it("preserves first-seen index order among equal-priority findings when sorting for caps", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "first", category: "style" }, + { path: "src/a.ts", line: 2, severity: "nit", body: "second", category: "style" }, + ]; + expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 1 }).map((finding) => finding.body)).toEqual(["first"]); + }); + + it("dedupes same path:line anchors before capping", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "blocker", body: "first", category: "security" }, + { path: "src/a.ts", line: 1, severity: "blocker", body: "duplicate", category: "security" }, + ]; + expect(selectAnchoredInlineFindings(findings, files, {}).map((finding) => finding.body)).toEqual(["first"]); + }); + + it("skips every category when perCategoryCap is zero", () => { + const findings: InlineFinding[] = [{ path: "src/a.ts", line: 1, severity: "nit", body: "style", category: "style" }]; + expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 0 })).toEqual([]); + }); + + it("ignores files with empty or missing patch content", () => { + const findings: InlineFinding[] = [{ path: "src/empty.ts", line: 1, severity: "nit", body: "missing patch" }]; + expect(selectAnchoredInlineFindings(findings, [{ path: "src/empty.ts", payload: { patch: "" } }], {})).toEqual([]); + expect( + selectAnchoredInlineFindings(findings, [{ path: "src/empty.ts", payload: { patch: 42 as unknown as string } }], {}), + ).toEqual([]); + expect(rightSideLinesFromPatch("preamble only").size).toBe(0); + }); + + it("breaks on the total cap without per-category sorting when perCategoryCap is unset", () => { + const twelveLineFiles = [ + fileWith( + "src/a.ts", + "@@ -1,0 +1,12 @@\n+1\n+2\n+3\n+4\n+5\n+6\n+7\n+8\n+9\n+10\n+11\n+12", + ), + ]; + const findings: InlineFinding[] = Array.from({ length: 12 }, (_, index) => ({ + path: "src/a.ts", + line: index + 1, + severity: "nit" as const, + body: `body-${index + 1}`, + category: "style" as const, + })); + expect(selectAnchoredInlineFindings(findings, twelveLineFiles, {})).toHaveLength(DEFAULT_MAX_INLINE_COMMENTS); + }); + + it("still enforces the total cap after per-category trimming", () => { + const findings: InlineFinding[] = Array.from({ length: 6 }, (_, index) => ({ + path: "src/a.ts", + line: index + 1, + severity: "nit" as const, + body: `body-${index + 1}`, + category: "style" as const, + })); + expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 5, maxComments: 3 })).toHaveLength(3); + }); +}); diff --git a/test/unit/inline-suggestion-anchor.test.ts b/test/unit/inline-suggestion-anchor.test.ts index 288bd4d9f9..a56d1d36a5 100644 --- a/test/unit/inline-suggestion-anchor.test.ts +++ b/test/unit/inline-suggestion-anchor.test.ts @@ -1,25 +1,25 @@ -import { describe, expect, it } from "vitest"; -import { - addedLinesByPath, - addedLinesFromPatch, - anchoredSuggestionBlock, - isSuggestionAnchorable, - safeSuggestionBlock, -} from "../../src/review/inline-suggestion-anchor"; -import type { InlineFinding } from "../../src/services/ai-review"; - -const mixedPatch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n ctx4"; - -describe("addedLinesFromPatch (#2140)", () => { - it("returns only ADDED (+) RIGHT-side lines, not context lines", () => { - expect([...addedLinesFromPatch(mixedPatch)].sort((a, b) => a - b)).toEqual([2]); - expect([...addedLinesFromPatch("@@ -1,0 +1,2 @@\n+only-added\n+second")].sort((a, b) => a - b)).toEqual([1, 2]); - }); - - it("returns an empty set for patches with no hunks", () => { - expect(addedLinesFromPatch("").size).toBe(0); - expect(addedLinesFromPatch("preamble only").size).toBe(0); - }); +import { describe, expect, it } from "vitest"; +import { + addedLinesByPath, + addedLinesFromPatch, + anchoredSuggestionBlock, + isSuggestionAnchorable, + safeSuggestionBlock, +} from "../../src/review/inline-suggestion-anchor"; +import type { InlineFinding } from "../../src/services/ai-review"; + +const mixedPatch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n ctx4"; + +describe("addedLinesFromPatch (#2140)", () => { + it("returns only ADDED (+) RIGHT-side lines, not context lines", () => { + expect([...addedLinesFromPatch(mixedPatch)].sort((a, b) => a - b)).toEqual([2]); + expect([...addedLinesFromPatch("@@ -1,0 +1,2 @@\n+only-added\n+second")].sort((a, b) => a - b)).toEqual([1, 2]); + }); + + it("returns an empty set for patches with no hunks", () => { + expect(addedLinesFromPatch("").size).toBe(0); + expect(addedLinesFromPatch("preamble only").size).toBe(0); + }); describe("blank context lines and trailing-newline artifacts (#9663)", () => { // A blank context line (its single leading space stripped by the split -> marker `undefined`) must still @@ -51,80 +51,80 @@ describe("addedLinesFromPatch (#2140)", () => { expect(isSuggestionAnchorable({ path: "src/a.ts", line: 2 }, addedLines)).toBe(false); }); }); -}); - -describe("addedLinesByPath + isSuggestionAnchorable (#2140)", () => { - const files = [{ path: "src/a.ts", payload: { patch: mixedPatch } }]; - - it("treats added lines as suggestion-anchorable and context lines as not", () => { - const addedLines = addedLinesByPath(files); - expect(isSuggestionAnchorable({ path: "src/a.ts", line: 2 }, addedLines)).toBe(true); - expect(isSuggestionAnchorable({ path: "src/a.ts", line: 1 }, addedLines)).toBe(false); - expect(isSuggestionAnchorable({ path: "src/missing.ts", line: 1 }, addedLines)).toBe(false); - }); - - it("returns false when the file path is absent from the added-line map (#2141)", () => { - expect(isSuggestionAnchorable({ path: "src/unknown.ts", line: 1, endLine: 2 }, new Map())).toBe(false); - }); - - it("omits files with empty or non-string patches", () => { - const addedLines = addedLinesByPath([ - { path: "src/empty.ts", payload: { patch: "" } }, - { path: "src/bad.ts", payload: { patch: 42 as unknown as string } }, - ]); - expect(addedLines.size).toBe(0); - }); -}); - -describe("anchoredSuggestionBlock (#2140)", () => { - const files = [{ path: "src/a.ts", payload: { patch: mixedPatch } }]; - const addedLines = addedLinesByPath(files); - const withSuggestion: InlineFinding = { - path: "src/a.ts", - line: 2, - severity: "nit", - body: "Use const.", - suggestion: "const x = 1;", - }; - - it("keeps the suggestion on an added line", () => { - expect(anchoredSuggestionBlock(withSuggestion, true, addedLines)).toContain("```suggestion"); - }); - - it("drops the suggestion on a context line but leaves the caller to keep the finding text", () => { - expect(anchoredSuggestionBlock({ ...withSuggestion, line: 1 }, true, addedLines)).toBe(""); - }); - - it("keeps a multi-line suggestion when every line in the range is added (#2141)", () => { - const multiAdded = addedLinesByPath([{ path: "src/a.ts", payload: { patch: "@@ -1,0 +1,2 @@\n+one\n+two" } }]); - expect( - anchoredSuggestionBlock( - { ...withSuggestion, line: 1, endLine: 2, suggestion: "one\ntwo" }, - true, - multiAdded, - ), - ).toContain("```suggestion"); - }); - - it("drops a multi-line suggestion when any line in the range is context (#2141)", () => { - expect( - anchoredSuggestionBlock( - { ...withSuggestion, line: 1, endLine: 2, suggestion: "ctx\nadd" }, - true, - addedLines, - ), - ).toBe(""); - }); - - it("drops unsafe suggestion fences even on an added line", () => { - expect( - anchoredSuggestionBlock({ ...withSuggestion, suggestion: "```\nescape\n```" }, true, addedLines), - ).toBe(""); - expect(safeSuggestionBlock(undefined)).toBe(""); - expect(safeSuggestionBlock("")).toBe(""); - }); - - it("preserves multi-line suggestion text verbatim inside the fence (#2139)", () => { - expect(safeSuggestionBlock("line1\nline2")).toBe("\n\n```suggestion\nline1\nline2\n```"); - }); -}); +}); + +describe("addedLinesByPath + isSuggestionAnchorable (#2140)", () => { + const files = [{ path: "src/a.ts", payload: { patch: mixedPatch } }]; + + it("treats added lines as suggestion-anchorable and context lines as not", () => { + const addedLines = addedLinesByPath(files); + expect(isSuggestionAnchorable({ path: "src/a.ts", line: 2 }, addedLines)).toBe(true); + expect(isSuggestionAnchorable({ path: "src/a.ts", line: 1 }, addedLines)).toBe(false); + expect(isSuggestionAnchorable({ path: "src/missing.ts", line: 1 }, addedLines)).toBe(false); + }); + + it("returns false when the file path is absent from the added-line map (#2141)", () => { + expect(isSuggestionAnchorable({ path: "src/unknown.ts", line: 1, endLine: 2 }, new Map())).toBe(false); + }); + + it("omits files with empty or non-string patches", () => { + const addedLines = addedLinesByPath([ + { path: "src/empty.ts", payload: { patch: "" } }, + { path: "src/bad.ts", payload: { patch: 42 as unknown as string } }, + ]); + expect(addedLines.size).toBe(0); + }); +}); + +describe("anchoredSuggestionBlock (#2140)", () => { + const files = [{ path: "src/a.ts", payload: { patch: mixedPatch } }]; + const addedLines = addedLinesByPath(files); + const withSuggestion: InlineFinding = { + path: "src/a.ts", + line: 2, + severity: "nit", + body: "Use const.", + suggestion: "const x = 1;", + }; + + it("keeps the suggestion on an added line", () => { + expect(anchoredSuggestionBlock(withSuggestion, true, addedLines)).toContain("```suggestion"); + }); + + it("drops the suggestion on a context line but leaves the caller to keep the finding text", () => { + expect(anchoredSuggestionBlock({ ...withSuggestion, line: 1 }, true, addedLines)).toBe(""); + }); + + it("keeps a multi-line suggestion when every line in the range is added (#2141)", () => { + const multiAdded = addedLinesByPath([{ path: "src/a.ts", payload: { patch: "@@ -1,0 +1,2 @@\n+one\n+two" } }]); + expect( + anchoredSuggestionBlock( + { ...withSuggestion, line: 1, endLine: 2, suggestion: "one\ntwo" }, + true, + multiAdded, + ), + ).toContain("```suggestion"); + }); + + it("drops a multi-line suggestion when any line in the range is context (#2141)", () => { + expect( + anchoredSuggestionBlock( + { ...withSuggestion, line: 1, endLine: 2, suggestion: "ctx\nadd" }, + true, + addedLines, + ), + ).toBe(""); + }); + + it("drops unsafe suggestion fences even on an added line", () => { + expect( + anchoredSuggestionBlock({ ...withSuggestion, suggestion: "```\nescape\n```" }, true, addedLines), + ).toBe(""); + expect(safeSuggestionBlock(undefined)).toBe(""); + expect(safeSuggestionBlock("")).toBe(""); + }); + + it("preserves multi-line suggestion text verbatim inside the fence (#2139)", () => { + expect(safeSuggestionBlock("line1\nline2")).toBe("\n\n```suggestion\nline1\nline2\n```"); + }); +}); diff --git a/test/unit/issue-quality-report-package.test.ts b/test/unit/issue-quality-report-package.test.ts index 45bcbae42c..cc8a5210ac 100644 --- a/test/unit/issue-quality-report-package.test.ts +++ b/test/unit/issue-quality-report-package.test.ts @@ -1,425 +1,425 @@ -import { describe, expect, it } from "vitest"; -import { buildIssueQualityReport } from "../../packages/loopover-engine/src/signals/issue-quality-report"; -import type { - BountyRecord, - CollisionReport, - IssueRecord, - PullRequestRecord, - RecentMergedPullRequestRecord, - RegistryRepoConfig, - RepositoryRecord, -} from "../../packages/loopover-engine/src/types/predicted-gate-types"; - -function now(): string { - return new Date().toISOString(); -} - -function daysAgoIso(days: number): string { - return new Date(Date.now() - days * 86_400_000).toISOString(); -} - -function registryConfig(overrides: Partial = {}): RegistryRepoConfig { - return { - repo: "acme/widgets", - emissionShare: 1, - issueDiscoveryShare: 0.5, - labelMultipliers: {}, - maintainerCut: 0, - raw: {}, - ...overrides, - }; -} - -function repo(fullName: string, overrides: Partial = {}): RepositoryRecord { - const [owner, name] = fullName.split("/"); - return { - fullName, - owner: owner!, - name: name!, - installationId: undefined, - isInstalled: true, - isRegistered: true, - isPrivate: false, - htmlUrl: `https://github.com/${fullName}`, - defaultBranch: "main", - registryConfig: registryConfig({ repo: fullName }), - ...overrides, - }; -} - -function issueDiscoveryRepo(fullName: string): RepositoryRecord { - return repo(fullName, { registryConfig: registryConfig({ repo: fullName, issueDiscoveryShare: 1 }) }); -} - -function directPrRepo(fullName: string): RepositoryRecord { - return repo(fullName, { registryConfig: registryConfig({ repo: fullName, issueDiscoveryShare: 0 }) }); -} - -function issue(repoFullName: string, number: number, title: string, overrides: Partial = {}): IssueRecord { - return { - repoFullName, - number, - title, - state: "open", - authorLogin: "reporter", - authorAssociation: "NONE", - htmlUrl: `https://github.com/${repoFullName}/issues/${number}`, - body: "x".repeat(220), - createdAt: now(), - updatedAt: now(), - closedAt: null, - labels: [], - linkedPrs: [], - ...overrides, - }; -} - -function pr(repoFullName: string, number: number, overrides: Partial = {}): PullRequestRecord { - return { - repoFullName, - number, - title: `PR ${number}`, - state: "open", - authorLogin: "contributor", - authorAssociation: "NONE", - headSha: "abc", - headRef: "branch", - baseRef: "main", - htmlUrl: `https://github.com/${repoFullName}/pull/${number}`, - mergedAt: null, - isDraft: false, - mergeableState: "clean", - reviewDecision: null, - body: "", - createdAt: now(), - updatedAt: now(), - closedAt: null, - labels: [], - linkedIssues: [], - ...overrides, - }; -} - -function merged(repoFullName: string, number: number, overrides: Partial = {}): RecentMergedPullRequestRecord { - return { - repoFullName, - number, - title: `Merged ${number}`, - authorLogin: "contributor", - htmlUrl: `https://github.com/${repoFullName}/pull/${number}`, - labels: [], - linkedIssues: [], - ...overrides, - }; -} - -function bounty(repoFullName: string, issueNumber: number, status: string, overrides: Partial = {}): BountyRecord { - return { - id: `b-${issueNumber}-${status}`, - repoFullName, - issueNumber, - status, - discoveredAt: now(), - updatedAt: now(), - payload: {}, - ...overrides, - }; -} - -function emptyCollisions(fullName: string): CollisionReport { - return { - repoFullName: fullName, - generatedAt: now(), - clusters: [], - summary: { clusterCount: 0, highRiskCount: 0, itemsReviewed: 0 }, - }; -} - -describe("buildIssueQualityReport (#6057 package-local export)", () => { - it("keeps every fixed call-signature slot and returns ready for a detailed open issue with no linked work", () => { - const r = issueDiscoveryRepo("acme/ready"); - const report = buildIssueQualityReport(r, [issue(r.fullName, 1, "Actionable")], [], r.fullName); - expect(report.issues).toHaveLength(1); - expect(report.issues[0]).toMatchObject({ - number: 1, - status: "ready", - reasons: expect.arrayContaining(["Issue has enough body detail to evaluate.", "No active PR is linked in cached metadata."]), - }); - expect(report.repoFullName).toBe("acme/ready"); - }); - - it("marks linked open-PR issues do_not_use and downgrades thin bodies to needs_proof", () => { - const r = issueDiscoveryRepo("acme/linked"); - const withPr = buildIssueQualityReport( - r, - [issue(r.fullName, 2, "Already claimed")], - [pr(r.fullName, 9, { linkedIssues: [2], body: "Fixes #2" })], - r.fullName, - ); - expect(withPr.issues[0]?.status).toBe("do_not_use"); - expect(withPr.issues[0]?.warnings.some((w) => /active PR/i.test(w))).toBe(true); - - const thin = buildIssueQualityReport(r, [issue(r.fullName, 3, "Thin", { body: "Short." })], [], r.fullName); - expect(thin.issues[0]?.status).toBe("needs_proof"); - }); - - it("accepts a prebuilt CollisionReport in the 6th positional slot and empty bounty/recent-merged arrays", () => { - const r = directPrRepo("acme/direct"); - const issues = [issue(r.fullName, 4, "Direct lane", { body: "x".repeat(220), labels: ["bug"] })]; - const report = buildIssueQualityReport(r, issues, [], r.fullName, [], emptyCollisions(r.fullName), []); - expect(report.lane.lane).toBe("direct_pr"); - expect(report.issues[0]?.status).toBe("needs_proof"); - expect(report.issues[0]?.warnings.some((w) => /direct-PR first/i.test(w))).toBe(true); - }); - - it("honors bounty lifecycle branches: active/completed/cancelled/historical/stale/ambiguous", () => { - const r = issueDiscoveryRepo("acme/bounty"); - const openIssue = issue(r.fullName, 5, "Bountied"); - - const active = buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "active")]); - expect(active.issues[0]?.reasons.some((reason) => /Active bounty/i.test(reason))).toBe(true); - - expect(buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "completed")]).issues[0]?.status).toBe("do_not_use"); - expect(buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "cancelled")]).issues[0]?.status).toBe("do_not_use"); - expect(buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "historical")]).issues[0]?.status).toBe("do_not_use"); - - const staleBounty = buildIssueQualityReport(r, [openIssue], [], r.fullName, [ - bounty(r.fullName, 5, "active", { updatedAt: daysAgoIso(60), discoveredAt: daysAgoIso(60) }), - ]); - expect(staleBounty.issues[0]?.status).toBe("needs_proof"); - expect(staleBounty.issues[0]?.warnings.some((w) => /stale/i.test(w))).toBe(true); - - const ambiguous = buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "weird-unknown-status")]); - expect(ambiguous.issues[0]?.status).toBe("needs_proof"); - expect(ambiguous.issues[0]?.warnings.some((w) => /ambiguous/i.test(w))).toBe(true); - }); - - it("classifies duplicate/invalid labels and closed issues as lifecycle do_not_use / closed_not_solved", () => { - const r = issueDiscoveryRepo("acme/labels"); - const dup = buildIssueQualityReport(r, [issue(r.fullName, 10, "Dup", { labels: ["duplicate"] })], [], r.fullName); - expect(dup.issues[0]?.status).toBe("do_not_use"); - expect(dup.issues[0]?.warnings.some((w) => /duplicate/i.test(w))).toBe(true); - - const invalid = buildIssueQualityReport(r, [issue(r.fullName, 11, "Bad", { labels: ["wontfix"] })], [], r.fullName); - expect(invalid.issues[0]?.status).toBe("do_not_use"); - - // Closed issues are filtered from the quality report (open-only), but still exercise lifecycle helpers - // when co-present: an open sibling plus a closed one only emits open. - const mixed = buildIssueQualityReport( - r, - [issue(r.fullName, 12, "Closed", { state: "closed" }), issue(r.fullName, 13, "Still open")], - [], - r.fullName, - ); - expect(mixed.issues.map((i) => i.number)).toEqual([13]); - }); - - it("treats merged solvers as solved/valid_solved and flags self-solved reporter loops", () => { - const r = issueDiscoveryRepo("acme/solved"); - const solved = buildIssueQualityReport( - r, - [issue(r.fullName, 20, "Solved open still listed")], - [], - r.fullName, - [], - undefined, - [merged(r.fullName, 100, { linkedIssues: [20], authorLogin: "other" })], - ); - expect(solved.issues[0]?.status).toBe("do_not_use"); - expect(solved.issues[0]?.warnings.some((w) => /merged PR/i.test(w) || /valid solved|lifecycle is/i.test(w))).toBe(true); - - const selfSolved = buildIssueQualityReport( - r, - [issue(r.fullName, 21, "Self", { authorLogin: "reporter" })], - [pr(r.fullName, 101, { linkedIssues: [21], authorLogin: "reporter", mergedAt: now(), state: "merged" })], - r.fullName, - ); - expect(selfSolved.issues[0]?.status).toBe("do_not_use"); - }); - - it("marks stale open issues needs_proof and applies age>180 score penalty", () => { - const r = issueDiscoveryRepo("acme/stale"); - const stale = buildIssueQualityReport( - r, - [issue(r.fullName, 30, "Old", { updatedAt: daysAgoIso(100), createdAt: daysAgoIso(100) })], - [], - r.fullName, - ); - expect(stale.issues[0]?.status).toBe("needs_proof"); - expect(stale.issues[0]?.warnings.some((w) => /stale/i.test(w))).toBe(true); - - const ancient = buildIssueQualityReport( - r, - [issue(r.fullName, 31, "Ancient", { updatedAt: daysAgoIso(200), createdAt: daysAgoIso(200), body: "x".repeat(220) })], - [], - r.fullName, - ); - expect(ancient.issues[0]?.score).toBeLessThan( - buildIssueQualityReport(r, [issue(r.fullName, 32, "Fresh")], [], r.fullName).issues[0]!.score, - ); - }); - - it("warns on maintainer-authored and maintainer-WIP issues", () => { - const r = issueDiscoveryRepo("acme/maint"); - const authored = buildIssueQualityReport( - r, - [issue(r.fullName, 40, "From owner", { authorAssociation: "OWNER" })], - [], - r.fullName, - ); - expect(authored.issues[0]?.warnings.some((w) => /Maintainer-authored; confirm/i.test(w))).toBe(true); - - const wip = buildIssueQualityReport( - r, - [issue(r.fullName, 41, "WIP", { authorAssociation: "MEMBER", labels: ["wip"] })], - [], - r.fullName, - ); - expect(wip.issues[0]?.status).toBe("needs_proof"); - expect(wip.issues[0]?.warnings.some((w) => /in-progress\/internal/i.test(w))).toBe(true); - }); - - it("resolves issue.linkedPrs back-references and cached-only linkedPr metadata warnings", () => { - const r = issueDiscoveryRepo("acme/backref"); - // PR does not list the issue, but the issue lists the PR — resolveLinkedPullRequests must add it. - const backref = buildIssueQualityReport( - r, - [issue(r.fullName, 50, "Backref", { linkedPrs: [77] })], - [pr(r.fullName, 77, { linkedIssues: [] })], - r.fullName, - ); - expect(backref.issues[0]?.status).toBe("do_not_use"); - expect(backref.issues[0]?.warnings.some((w) => /active PR/i.test(w))).toBe(true); - - // linkedPrs point at a PR not present in the fetched list → cached-metadata warning. - const cachedOnly = buildIssueQualityReport( - r, - [issue(r.fullName, 51, "Cached", { linkedPrs: [999] })], - [], - r.fullName, - ); - expect(cachedOnly.issues[0]?.warnings.some((w) => /Cached issue metadata already references PR\(s\): #999/i.test(w))).toBe(true); - }); - - it("treats high-risk collision clusters as do_not_use and medium/low as warning-only", () => { - const r = issueDiscoveryRepo("acme/collisions"); - const high: CollisionReport = { - repoFullName: r.fullName, - generatedAt: now(), - summary: { clusterCount: 1, highRiskCount: 1, itemsReviewed: 2 }, - clusters: [ - { - id: "c1", - risk: "high", - reason: "overlap", - items: [ - { type: "issue", number: 60, title: "A" }, - { type: "pull_request", number: 1, title: "B" }, - ], - }, - ], - }; - expect(buildIssueQualityReport(r, [issue(r.fullName, 60, "A")], [], r.fullName, [], high).issues[0]?.status).toBe("do_not_use"); - - const medium: CollisionReport = { - ...high, - summary: { clusterCount: 1, highRiskCount: 0, itemsReviewed: 2 }, - clusters: [{ ...high.clusters[0]!, risk: "medium" }], - }; - const medReport = buildIssueQualityReport(r, [issue(r.fullName, 60, "A")], [], r.fullName, [], medium); - expect(medReport.issues[0]?.warnings.some((w) => /duplicate or overlapping/i.test(w))).toBe(true); - expect(medReport.issues[0]?.status).not.toBe("do_not_use"); - }); - - it("sorts by score desc then number asc, and skips building collisions when prebuilt is provided", () => { - const r = issueDiscoveryRepo("acme/sort"); - const report = buildIssueQualityReport( - r, - [ - issue(r.fullName, 2, "Thin second", { body: "Short." }), - issue(r.fullName, 1, "Ready first"), - issue(r.fullName, 3, "Ready third"), - ], - [], - r.fullName, - [], - emptyCollisions(r.fullName), - ); - expect(report.issues.map((i) => i.number)).toEqual([1, 3, 2]); - }); - - it("handles null repo, missing dates, and blank bounty status without throwing", () => { - const report = buildIssueQualityReport( - null, - [issue("acme/null", 70, "No dates", { updatedAt: null, createdAt: null, body: null })], - [], - "acme/null", - [bounty("acme/null", 70, " ")], - ); - expect(report.lane.lane).toBe("unknown"); - expect(report.issues[0]?.status).toBe("needs_proof"); - }); - - it("covers multi-PR index buckets, split-lane valid_solved, and invalid timestamps", () => { - const split = repo("acme/split", { - registryConfig: registryConfig({ repo: "acme/split", emissionShare: 1, issueDiscoveryShare: 0.4 }), - }); - // issueDiscoveryShare 0.4 with emission > 0 → split lane in buildLaneAdvice - expect( - buildIssueQualityReport( - split, - [issue(split.fullName, 90, "Split solved")], - [], - split.fullName, - [], - undefined, - [merged(split.fullName, 900, { linkedIssues: [90], authorLogin: "other" })], - ).issues[0]?.status, - ).toBe("do_not_use"); - - // Two open PRs link the same issue → indexPullRequestsByLinkedIssue bucket.push path - const r = issueDiscoveryRepo("acme/multi"); - const multi = buildIssueQualityReport( - r, - [issue(r.fullName, 91, "Crowded")], - [pr(r.fullName, 1, { linkedIssues: [91] }), pr(r.fullName, 2, { linkedIssues: [91, 91] })], - r.fullName, - ); - expect(multi.issues[0]?.status).toBe("do_not_use"); - expect(multi.issues[0]?.warnings.some((w) => /2 active PR/i.test(w))).toBe(true); - - // Invalid timestamps normalize to age 0 via daysSince - const badDate = buildIssueQualityReport( - r, - [issue(r.fullName, 92, "Bad date", { updatedAt: "not-a-date", createdAt: "also-bad" })], - [], - r.fullName, - ); - expect(badDate.issues[0]?.status).toBe("ready"); - }); - - it("returns the default open-lifecycle reason when no other signal applies", () => { - const r = issueDiscoveryRepo("acme/plain"); - // No labels, no linked work, body detailed — reasons include detail + no-linked; lifecycle reasons default - // is only for the internal classify helper when reasons.length===0. Hit that by classifying a bare open - // issue through lifecycle (already exercised); assert ready stays stable. - const report = buildIssueQualityReport(r, [issue(r.fullName, 93, "Plain", { labels: [] })], [], r.fullName); - expect(report.issues[0]?.status).toBe("ready"); - }); - - it("does not throw when an open issue sits past the lifecycle CAP, and still classifies it (#6141)", () => { - const r = issueDiscoveryRepo("acme/cap"); - // 300 closed filler issues occupy the bulk slice; the 301st is open + duplicate and must: - // 1) not throw on lifecycleByIssue.get(...).state - // 2) be pinned into classification so duplicate → do_not_use (not silently "open") - const filler = Array.from({ length: 300 }, (_, i) => - issue(r.fullName, i + 1, `Closed ${i + 1}`, { state: "closed", body: "x".repeat(220) }), - ); - const beyondCap = issue(r.fullName, 301, "Beyond cap duplicate", { labels: ["duplicate"], body: "x".repeat(220) }); - expect(() => buildIssueQualityReport(r, [...filler, beyondCap], [], r.fullName)).not.toThrow(); - const report = buildIssueQualityReport(r, [...filler, beyondCap], [], r.fullName); - expect(report.issues).toHaveLength(1); - expect(report.issues[0]).toMatchObject({ number: 301, status: "do_not_use" }); - expect(report.issues[0]?.warnings.some((w) => /duplicate/i.test(w))).toBe(true); - }); -}); +import { describe, expect, it } from "vitest"; +import { buildIssueQualityReport } from "../../packages/loopover-engine/src/signals/issue-quality-report"; +import type { + BountyRecord, + CollisionReport, + IssueRecord, + PullRequestRecord, + RecentMergedPullRequestRecord, + RegistryRepoConfig, + RepositoryRecord, +} from "../../packages/loopover-engine/src/types/predicted-gate-types"; + +function now(): string { + return new Date().toISOString(); +} + +function daysAgoIso(days: number): string { + return new Date(Date.now() - days * 86_400_000).toISOString(); +} + +function registryConfig(overrides: Partial = {}): RegistryRepoConfig { + return { + repo: "acme/widgets", + emissionShare: 1, + issueDiscoveryShare: 0.5, + labelMultipliers: {}, + maintainerCut: 0, + raw: {}, + ...overrides, + }; +} + +function repo(fullName: string, overrides: Partial = {}): RepositoryRecord { + const [owner, name] = fullName.split("/"); + return { + fullName, + owner: owner!, + name: name!, + installationId: undefined, + isInstalled: true, + isRegistered: true, + isPrivate: false, + htmlUrl: `https://github.com/${fullName}`, + defaultBranch: "main", + registryConfig: registryConfig({ repo: fullName }), + ...overrides, + }; +} + +function issueDiscoveryRepo(fullName: string): RepositoryRecord { + return repo(fullName, { registryConfig: registryConfig({ repo: fullName, issueDiscoveryShare: 1 }) }); +} + +function directPrRepo(fullName: string): RepositoryRecord { + return repo(fullName, { registryConfig: registryConfig({ repo: fullName, issueDiscoveryShare: 0 }) }); +} + +function issue(repoFullName: string, number: number, title: string, overrides: Partial = {}): IssueRecord { + return { + repoFullName, + number, + title, + state: "open", + authorLogin: "reporter", + authorAssociation: "NONE", + htmlUrl: `https://github.com/${repoFullName}/issues/${number}`, + body: "x".repeat(220), + createdAt: now(), + updatedAt: now(), + closedAt: null, + labels: [], + linkedPrs: [], + ...overrides, + }; +} + +function pr(repoFullName: string, number: number, overrides: Partial = {}): PullRequestRecord { + return { + repoFullName, + number, + title: `PR ${number}`, + state: "open", + authorLogin: "contributor", + authorAssociation: "NONE", + headSha: "abc", + headRef: "branch", + baseRef: "main", + htmlUrl: `https://github.com/${repoFullName}/pull/${number}`, + mergedAt: null, + isDraft: false, + mergeableState: "clean", + reviewDecision: null, + body: "", + createdAt: now(), + updatedAt: now(), + closedAt: null, + labels: [], + linkedIssues: [], + ...overrides, + }; +} + +function merged(repoFullName: string, number: number, overrides: Partial = {}): RecentMergedPullRequestRecord { + return { + repoFullName, + number, + title: `Merged ${number}`, + authorLogin: "contributor", + htmlUrl: `https://github.com/${repoFullName}/pull/${number}`, + labels: [], + linkedIssues: [], + ...overrides, + }; +} + +function bounty(repoFullName: string, issueNumber: number, status: string, overrides: Partial = {}): BountyRecord { + return { + id: `b-${issueNumber}-${status}`, + repoFullName, + issueNumber, + status, + discoveredAt: now(), + updatedAt: now(), + payload: {}, + ...overrides, + }; +} + +function emptyCollisions(fullName: string): CollisionReport { + return { + repoFullName: fullName, + generatedAt: now(), + clusters: [], + summary: { clusterCount: 0, highRiskCount: 0, itemsReviewed: 0 }, + }; +} + +describe("buildIssueQualityReport (#6057 package-local export)", () => { + it("keeps every fixed call-signature slot and returns ready for a detailed open issue with no linked work", () => { + const r = issueDiscoveryRepo("acme/ready"); + const report = buildIssueQualityReport(r, [issue(r.fullName, 1, "Actionable")], [], r.fullName); + expect(report.issues).toHaveLength(1); + expect(report.issues[0]).toMatchObject({ + number: 1, + status: "ready", + reasons: expect.arrayContaining(["Issue has enough body detail to evaluate.", "No active PR is linked in cached metadata."]), + }); + expect(report.repoFullName).toBe("acme/ready"); + }); + + it("marks linked open-PR issues do_not_use and downgrades thin bodies to needs_proof", () => { + const r = issueDiscoveryRepo("acme/linked"); + const withPr = buildIssueQualityReport( + r, + [issue(r.fullName, 2, "Already claimed")], + [pr(r.fullName, 9, { linkedIssues: [2], body: "Fixes #2" })], + r.fullName, + ); + expect(withPr.issues[0]?.status).toBe("do_not_use"); + expect(withPr.issues[0]?.warnings.some((w) => /active PR/i.test(w))).toBe(true); + + const thin = buildIssueQualityReport(r, [issue(r.fullName, 3, "Thin", { body: "Short." })], [], r.fullName); + expect(thin.issues[0]?.status).toBe("needs_proof"); + }); + + it("accepts a prebuilt CollisionReport in the 6th positional slot and empty bounty/recent-merged arrays", () => { + const r = directPrRepo("acme/direct"); + const issues = [issue(r.fullName, 4, "Direct lane", { body: "x".repeat(220), labels: ["bug"] })]; + const report = buildIssueQualityReport(r, issues, [], r.fullName, [], emptyCollisions(r.fullName), []); + expect(report.lane.lane).toBe("direct_pr"); + expect(report.issues[0]?.status).toBe("needs_proof"); + expect(report.issues[0]?.warnings.some((w) => /direct-PR first/i.test(w))).toBe(true); + }); + + it("honors bounty lifecycle branches: active/completed/cancelled/historical/stale/ambiguous", () => { + const r = issueDiscoveryRepo("acme/bounty"); + const openIssue = issue(r.fullName, 5, "Bountied"); + + const active = buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "active")]); + expect(active.issues[0]?.reasons.some((reason) => /Active bounty/i.test(reason))).toBe(true); + + expect(buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "completed")]).issues[0]?.status).toBe("do_not_use"); + expect(buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "cancelled")]).issues[0]?.status).toBe("do_not_use"); + expect(buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "historical")]).issues[0]?.status).toBe("do_not_use"); + + const staleBounty = buildIssueQualityReport(r, [openIssue], [], r.fullName, [ + bounty(r.fullName, 5, "active", { updatedAt: daysAgoIso(60), discoveredAt: daysAgoIso(60) }), + ]); + expect(staleBounty.issues[0]?.status).toBe("needs_proof"); + expect(staleBounty.issues[0]?.warnings.some((w) => /stale/i.test(w))).toBe(true); + + const ambiguous = buildIssueQualityReport(r, [openIssue], [], r.fullName, [bounty(r.fullName, 5, "weird-unknown-status")]); + expect(ambiguous.issues[0]?.status).toBe("needs_proof"); + expect(ambiguous.issues[0]?.warnings.some((w) => /ambiguous/i.test(w))).toBe(true); + }); + + it("classifies duplicate/invalid labels and closed issues as lifecycle do_not_use / closed_not_solved", () => { + const r = issueDiscoveryRepo("acme/labels"); + const dup = buildIssueQualityReport(r, [issue(r.fullName, 10, "Dup", { labels: ["duplicate"] })], [], r.fullName); + expect(dup.issues[0]?.status).toBe("do_not_use"); + expect(dup.issues[0]?.warnings.some((w) => /duplicate/i.test(w))).toBe(true); + + const invalid = buildIssueQualityReport(r, [issue(r.fullName, 11, "Bad", { labels: ["wontfix"] })], [], r.fullName); + expect(invalid.issues[0]?.status).toBe("do_not_use"); + + // Closed issues are filtered from the quality report (open-only), but still exercise lifecycle helpers + // when co-present: an open sibling plus a closed one only emits open. + const mixed = buildIssueQualityReport( + r, + [issue(r.fullName, 12, "Closed", { state: "closed" }), issue(r.fullName, 13, "Still open")], + [], + r.fullName, + ); + expect(mixed.issues.map((i) => i.number)).toEqual([13]); + }); + + it("treats merged solvers as solved/valid_solved and flags self-solved reporter loops", () => { + const r = issueDiscoveryRepo("acme/solved"); + const solved = buildIssueQualityReport( + r, + [issue(r.fullName, 20, "Solved open still listed")], + [], + r.fullName, + [], + undefined, + [merged(r.fullName, 100, { linkedIssues: [20], authorLogin: "other" })], + ); + expect(solved.issues[0]?.status).toBe("do_not_use"); + expect(solved.issues[0]?.warnings.some((w) => /merged PR/i.test(w) || /valid solved|lifecycle is/i.test(w))).toBe(true); + + const selfSolved = buildIssueQualityReport( + r, + [issue(r.fullName, 21, "Self", { authorLogin: "reporter" })], + [pr(r.fullName, 101, { linkedIssues: [21], authorLogin: "reporter", mergedAt: now(), state: "merged" })], + r.fullName, + ); + expect(selfSolved.issues[0]?.status).toBe("do_not_use"); + }); + + it("marks stale open issues needs_proof and applies age>180 score penalty", () => { + const r = issueDiscoveryRepo("acme/stale"); + const stale = buildIssueQualityReport( + r, + [issue(r.fullName, 30, "Old", { updatedAt: daysAgoIso(100), createdAt: daysAgoIso(100) })], + [], + r.fullName, + ); + expect(stale.issues[0]?.status).toBe("needs_proof"); + expect(stale.issues[0]?.warnings.some((w) => /stale/i.test(w))).toBe(true); + + const ancient = buildIssueQualityReport( + r, + [issue(r.fullName, 31, "Ancient", { updatedAt: daysAgoIso(200), createdAt: daysAgoIso(200), body: "x".repeat(220) })], + [], + r.fullName, + ); + expect(ancient.issues[0]?.score).toBeLessThan( + buildIssueQualityReport(r, [issue(r.fullName, 32, "Fresh")], [], r.fullName).issues[0]!.score, + ); + }); + + it("warns on maintainer-authored and maintainer-WIP issues", () => { + const r = issueDiscoveryRepo("acme/maint"); + const authored = buildIssueQualityReport( + r, + [issue(r.fullName, 40, "From owner", { authorAssociation: "OWNER" })], + [], + r.fullName, + ); + expect(authored.issues[0]?.warnings.some((w) => /Maintainer-authored; confirm/i.test(w))).toBe(true); + + const wip = buildIssueQualityReport( + r, + [issue(r.fullName, 41, "WIP", { authorAssociation: "MEMBER", labels: ["wip"] })], + [], + r.fullName, + ); + expect(wip.issues[0]?.status).toBe("needs_proof"); + expect(wip.issues[0]?.warnings.some((w) => /in-progress\/internal/i.test(w))).toBe(true); + }); + + it("resolves issue.linkedPrs back-references and cached-only linkedPr metadata warnings", () => { + const r = issueDiscoveryRepo("acme/backref"); + // PR does not list the issue, but the issue lists the PR — resolveLinkedPullRequests must add it. + const backref = buildIssueQualityReport( + r, + [issue(r.fullName, 50, "Backref", { linkedPrs: [77] })], + [pr(r.fullName, 77, { linkedIssues: [] })], + r.fullName, + ); + expect(backref.issues[0]?.status).toBe("do_not_use"); + expect(backref.issues[0]?.warnings.some((w) => /active PR/i.test(w))).toBe(true); + + // linkedPrs point at a PR not present in the fetched list → cached-metadata warning. + const cachedOnly = buildIssueQualityReport( + r, + [issue(r.fullName, 51, "Cached", { linkedPrs: [999] })], + [], + r.fullName, + ); + expect(cachedOnly.issues[0]?.warnings.some((w) => /Cached issue metadata already references PR\(s\): #999/i.test(w))).toBe(true); + }); + + it("treats high-risk collision clusters as do_not_use and medium/low as warning-only", () => { + const r = issueDiscoveryRepo("acme/collisions"); + const high: CollisionReport = { + repoFullName: r.fullName, + generatedAt: now(), + summary: { clusterCount: 1, highRiskCount: 1, itemsReviewed: 2 }, + clusters: [ + { + id: "c1", + risk: "high", + reason: "overlap", + items: [ + { type: "issue", number: 60, title: "A" }, + { type: "pull_request", number: 1, title: "B" }, + ], + }, + ], + }; + expect(buildIssueQualityReport(r, [issue(r.fullName, 60, "A")], [], r.fullName, [], high).issues[0]?.status).toBe("do_not_use"); + + const medium: CollisionReport = { + ...high, + summary: { clusterCount: 1, highRiskCount: 0, itemsReviewed: 2 }, + clusters: [{ ...high.clusters[0]!, risk: "medium" }], + }; + const medReport = buildIssueQualityReport(r, [issue(r.fullName, 60, "A")], [], r.fullName, [], medium); + expect(medReport.issues[0]?.warnings.some((w) => /duplicate or overlapping/i.test(w))).toBe(true); + expect(medReport.issues[0]?.status).not.toBe("do_not_use"); + }); + + it("sorts by score desc then number asc, and skips building collisions when prebuilt is provided", () => { + const r = issueDiscoveryRepo("acme/sort"); + const report = buildIssueQualityReport( + r, + [ + issue(r.fullName, 2, "Thin second", { body: "Short." }), + issue(r.fullName, 1, "Ready first"), + issue(r.fullName, 3, "Ready third"), + ], + [], + r.fullName, + [], + emptyCollisions(r.fullName), + ); + expect(report.issues.map((i) => i.number)).toEqual([1, 3, 2]); + }); + + it("handles null repo, missing dates, and blank bounty status without throwing", () => { + const report = buildIssueQualityReport( + null, + [issue("acme/null", 70, "No dates", { updatedAt: null, createdAt: null, body: null })], + [], + "acme/null", + [bounty("acme/null", 70, " ")], + ); + expect(report.lane.lane).toBe("unknown"); + expect(report.issues[0]?.status).toBe("needs_proof"); + }); + + it("covers multi-PR index buckets, split-lane valid_solved, and invalid timestamps", () => { + const split = repo("acme/split", { + registryConfig: registryConfig({ repo: "acme/split", emissionShare: 1, issueDiscoveryShare: 0.4 }), + }); + // issueDiscoveryShare 0.4 with emission > 0 → split lane in buildLaneAdvice + expect( + buildIssueQualityReport( + split, + [issue(split.fullName, 90, "Split solved")], + [], + split.fullName, + [], + undefined, + [merged(split.fullName, 900, { linkedIssues: [90], authorLogin: "other" })], + ).issues[0]?.status, + ).toBe("do_not_use"); + + // Two open PRs link the same issue → indexPullRequestsByLinkedIssue bucket.push path + const r = issueDiscoveryRepo("acme/multi"); + const multi = buildIssueQualityReport( + r, + [issue(r.fullName, 91, "Crowded")], + [pr(r.fullName, 1, { linkedIssues: [91] }), pr(r.fullName, 2, { linkedIssues: [91, 91] })], + r.fullName, + ); + expect(multi.issues[0]?.status).toBe("do_not_use"); + expect(multi.issues[0]?.warnings.some((w) => /2 active PR/i.test(w))).toBe(true); + + // Invalid timestamps normalize to age 0 via daysSince + const badDate = buildIssueQualityReport( + r, + [issue(r.fullName, 92, "Bad date", { updatedAt: "not-a-date", createdAt: "also-bad" })], + [], + r.fullName, + ); + expect(badDate.issues[0]?.status).toBe("ready"); + }); + + it("returns the default open-lifecycle reason when no other signal applies", () => { + const r = issueDiscoveryRepo("acme/plain"); + // No labels, no linked work, body detailed — reasons include detail + no-linked; lifecycle reasons default + // is only for the internal classify helper when reasons.length===0. Hit that by classifying a bare open + // issue through lifecycle (already exercised); assert ready stays stable. + const report = buildIssueQualityReport(r, [issue(r.fullName, 93, "Plain", { labels: [] })], [], r.fullName); + expect(report.issues[0]?.status).toBe("ready"); + }); + + it("does not throw when an open issue sits past the lifecycle CAP, and still classifies it (#6141)", () => { + const r = issueDiscoveryRepo("acme/cap"); + // 300 closed filler issues occupy the bulk slice; the 301st is open + duplicate and must: + // 1) not throw on lifecycleByIssue.get(...).state + // 2) be pinned into classification so duplicate → do_not_use (not silently "open") + const filler = Array.from({ length: 300 }, (_, i) => + issue(r.fullName, i + 1, `Closed ${i + 1}`, { state: "closed", body: "x".repeat(220) }), + ); + const beyondCap = issue(r.fullName, 301, "Beyond cap duplicate", { labels: ["duplicate"], body: "x".repeat(220) }); + expect(() => buildIssueQualityReport(r, [...filler, beyondCap], [], r.fullName)).not.toThrow(); + const report = buildIssueQualityReport(r, [...filler, beyondCap], [], r.fullName); + expect(report.issues).toHaveLength(1); + expect(report.issues[0]).toMatchObject({ number: 301, status: "do_not_use" }); + expect(report.issues[0]?.warnings.some((w) => /duplicate/i.test(w))).toBe(true); + }); +}); From b649971997816b63cdce79948a0f0fe9bede5c7e Mon Sep 17 00:00:00 2001 From: JSONbored Date: Wed, 29 Jul 2026 03:02:47 -0700 Subject: [PATCH 2/2] test(engine): cover the package-local buildIssueQualityReport in the engine suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildIssueQualityReport` is re-exported from the engine barrel (src/index.ts) but this package's own node:test suite never called it. `npm run engine:coverage` therefore saw the module load — top-level constants only — and reported it LF:313 LH:96 with BRF:0, i.e. no function-level data at all, while the root vitest suite covered the same file 87/87 lines and 143/143 branches. Two uploads disagreeing about the same file is invisible while patch coverage only reads changed lines, because nothing changes all 313 at once. Renormalizing the file's line endings did, and the merged report capped at ~57%: of the 217 lines the engine flag called uncovered, 84 were rescued by vitest and 133 were function-signature and closing-brace lines v8 never lists as statements, so nothing could rescue them. Cover it where this package is actually graded rather than leaning on the vitest duplicate — the outcome scripts/engine-coverage.ts (#9064) was written to encourage. The file now reports 313/313 lines and 163/163 branches under the engine model. 17 cases covering the lane arms (issue_discovery / direct_pr / split / unknown), every bounty lifecycle, duplicate and invalid labelling, self-solved loops, maintainer-authored vs maintainer-WIP, linkedPrs back-references for both open and merged PRs, collision risk tiers, sort order, the lifecycle and report caps, and the null-repo / unparseable-date degradation paths. --- .../test/issue-quality-report.test.ts | 360 ++++++++++++++++++ 1 file changed, 360 insertions(+) create mode 100644 packages/loopover-engine/test/issue-quality-report.test.ts diff --git a/packages/loopover-engine/test/issue-quality-report.test.ts b/packages/loopover-engine/test/issue-quality-report.test.ts new file mode 100644 index 0000000000..164222b20b --- /dev/null +++ b/packages/loopover-engine/test/issue-quality-report.test.ts @@ -0,0 +1,360 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { buildIssueQualityReport } from "../dist/index.js"; + +// The package-local twin of the host engine's buildIssueQualityReport (#6057) was reachable from the +// barrel but never invoked by this suite: `npm run engine:coverage` saw the module load (top-level +// constants only) and reported it BRF:0 with 96/313 lines, while the root vitest suite exercised it +// fully. Two uploads disagreeing about the same file is what surfaced as a phantom patch-coverage +// failure the moment a whole-file diff touched it (#9798). Cover it here, in the suite that actually +// grades this package, rather than leaning on the vitest duplicate. + +type Json = Record; + +function now(): string { + return new Date().toISOString(); +} + +function daysAgoIso(days: number): string { + return new Date(Date.now() - days * 86_400_000).toISOString(); +} + +function registryConfig(overrides: Json = {}): Json { + return { repo: "acme/widgets", emissionShare: 1, issueDiscoveryShare: 0.5, labelMultipliers: {}, maintainerCut: 0, raw: {}, ...overrides }; +} + +function repo(fullName: string, overrides: Json = {}): Json { + const [owner, name] = fullName.split("/"); + return { + fullName, + owner, + name, + installationId: undefined, + isInstalled: true, + isRegistered: true, + isPrivate: false, + htmlUrl: `https://github.com/${fullName}`, + defaultBranch: "main", + registryConfig: registryConfig({ repo: fullName }), + ...overrides, + }; +} + +/** issueDiscoveryShare 1 → `issue_discovery` lane; 0 → `direct_pr`; 0.4 with emission → `split`. */ +function laneRepo(fullName: string, issueDiscoveryShare: number): Json { + return repo(fullName, { registryConfig: registryConfig({ repo: fullName, issueDiscoveryShare }) }); +} + +function issue(repoFullName: string, number: number, title: string, overrides: Json = {}): Json { + return { + repoFullName, + number, + title, + state: "open", + authorLogin: "reporter", + authorAssociation: "NONE", + htmlUrl: `https://github.com/${repoFullName}/issues/${number}`, + body: "x".repeat(220), + createdAt: now(), + updatedAt: now(), + closedAt: null, + labels: [], + linkedPrs: [], + ...overrides, + }; +} + +function pr(repoFullName: string, number: number, overrides: Json = {}): Json { + return { + repoFullName, + number, + title: `PR ${number}`, + state: "open", + authorLogin: "contributor", + authorAssociation: "NONE", + headSha: "abc", + headRef: "branch", + baseRef: "main", + htmlUrl: `https://github.com/${repoFullName}/pull/${number}`, + mergedAt: null, + isDraft: false, + mergeableState: "clean", + reviewDecision: null, + body: "", + createdAt: now(), + updatedAt: now(), + closedAt: null, + labels: [], + linkedIssues: [], + ...overrides, + }; +} + +function merged(repoFullName: string, number: number, overrides: Json = {}): Json { + return { + repoFullName, + number, + title: `Merged ${number}`, + authorLogin: "contributor", + htmlUrl: `https://github.com/${repoFullName}/pull/${number}`, + labels: [], + linkedIssues: [], + ...overrides, + }; +} + +function bounty(repoFullName: string, issueNumber: number, status: string, overrides: Json = {}): Json { + return { id: `b-${issueNumber}-${status}`, repoFullName, issueNumber, status, discoveredAt: now(), updatedAt: now(), payload: {}, ...overrides }; +} + +function emptyCollisions(fullName: string): Json { + return { repoFullName: fullName, generatedAt: now(), clusters: [], summary: { clusterCount: 0, highRiskCount: 0, itemsReviewed: 0 } }; +} + +/** The dist barrel is consumed untyped here (same as the sibling suites), so pin the shape once. */ +const build = buildIssueQualityReport as unknown as (...args: unknown[]) => { + repoFullName: string; + lane: { lane: string }; + summary: string; + issues: { number: number; title: string; status: string; score: number; reasons: string[]; warnings: string[] }[]; +}; + +const first = (report: ReturnType) => report.issues[0]!; + +test("barrel: the public entrypoint re-exports the package-local issue-quality API", () => { + assert.equal(typeof buildIssueQualityReport, "function"); +}); + +test("buildIssueQualityReport: a detailed open issue with no linked work is ready", () => { + const r = laneRepo("acme/ready", 1); + const report = build(r, [issue("acme/ready", 1, "Actionable")], [], "acme/ready"); + assert.equal(report.issues.length, 1); + assert.equal(first(report).status, "ready"); + assert.equal(report.repoFullName, "acme/ready"); + assert.ok(first(report).reasons.includes("Issue has enough body detail to evaluate.")); + assert.ok(first(report).reasons.includes("No active PR is linked in cached metadata.")); + assert.match(report.summary, /1 open issue\(s\) evaluated; 1 look ready/); +}); + +test("buildIssueQualityReport: a linked open PR blocks the issue and a thin body needs proof", () => { + const r = laneRepo("acme/linked", 1); + const withPr = build(r, [issue("acme/linked", 2, "Claimed")], [pr("acme/linked", 9, { linkedIssues: [2] })], "acme/linked"); + assert.equal(first(withPr).status, "do_not_use"); + assert.ok(first(withPr).warnings.some((w) => /active PR/i.test(w))); + + const thin = build(r, [issue("acme/linked", 3, "Thin", { body: "Short." })], [], "acme/linked"); + assert.equal(first(thin).status, "needs_proof"); + assert.ok(first(thin).warnings.some((w) => /body is thin/i.test(w))); +}); + +test("buildIssueQualityReport: labels are echoed as a reason and a direct-PR lane warns", () => { + const r = laneRepo("acme/direct", 0); + const report = build(r, [issue("acme/direct", 4, "Direct", { labels: ["bug", "good first issue"] })], [], "acme/direct", [], emptyCollisions("acme/direct"), []); + assert.equal(report.lane.lane, "direct_pr"); + assert.equal(first(report).status, "needs_proof"); + assert.ok(first(report).reasons.includes("Labels: bug, good first issue.")); + assert.ok(first(report).warnings.some((w) => /direct-PR first/i.test(w))); +}); + +test("buildIssueQualityReport: every bounty lifecycle arm maps to its own status", () => { + const r = laneRepo("acme/bounty", 1); + const open = issue("acme/bounty", 5, "Bountied"); + + const active = build(r, [open], [], "acme/bounty", [bounty("acme/bounty", 5, "active")]); + assert.ok(first(active).reasons.some((reason) => /Active bounty context/i.test(reason))); + + for (const status of ["completed", "cancelled", "historical"]) { + const report = build(r, [open], [], "acme/bounty", [bounty("acme/bounty", 5, status)]); + assert.equal(first(report).status, "do_not_use", `${status} bounty should block`); + } + + const stale = build(r, [open], [], "acme/bounty", [bounty("acme/bounty", 5, "active", { updatedAt: daysAgoIso(60), discoveredAt: daysAgoIso(60) })]); + assert.equal(first(stale).status, "needs_proof"); + assert.ok(first(stale).warnings.some((w) => /looks stale/i.test(w))); + + const ambiguous = build(r, [open], [], "acme/bounty", [bounty("acme/bounty", 5, "weird-unknown-status")]); + assert.equal(first(ambiguous).status, "needs_proof"); + assert.ok(first(ambiguous).warnings.some((w) => /ambiguous/i.test(w))); +}); + +test("buildIssueQualityReport: duplicate and invalid labelling drive lifecycle do_not_use", () => { + const r = laneRepo("acme/labels", 1); + for (const label of ["duplicate", "wontfix", "invalid", "not planned", "won't fix"]) { + const report = build(r, [issue("acme/labels", 10, "Labelled", { labels: [label] })], [], "acme/labels"); + assert.equal(first(report).status, "do_not_use", `${label} should block`); + assert.ok(first(report).warnings.some((w) => /lifecycle is/i.test(w))); + } +}); + +test("buildIssueQualityReport: only open issues are reported, closed ones are filtered", () => { + const r = laneRepo("acme/mixed", 1); + const report = build(r, [issue("acme/mixed", 12, "Closed", { state: "closed" }), issue("acme/mixed", 13, "Open")], [], "acme/mixed"); + assert.deepEqual( + report.issues.map((i) => i.number), + [13], + ); +}); + +test("buildIssueQualityReport: merged solvers mark valid_solved, and a self-solved loop stays solved", () => { + const discovery = laneRepo("acme/solved", 1); + const solved = build(discovery, [issue("acme/solved", 20, "Solved")], [], "acme/solved", [], undefined, [ + merged("acme/solved", 100, { linkedIssues: [20], authorLogin: "other" }), + ]); + assert.equal(first(solved).status, "do_not_use"); + assert.ok(first(solved).warnings.some((w) => /merged PR/i.test(w))); + + // Reporter authored the solving PR → selfSolvedLoop suppresses valid_solved. + const selfSolved = build( + discovery, + [issue("acme/solved", 21, "Self", { authorLogin: "reporter" })], + [pr("acme/solved", 101, { linkedIssues: [21], authorLogin: "reporter", mergedAt: now(), state: "merged" })], + "acme/solved", + ); + assert.equal(first(selfSolved).status, "do_not_use"); + + // split lane (issueDiscoveryShare 0.4 + emission) also earns valid_solved. + const split = laneRepo("acme/split", 0.4); + const splitSolved = build(split, [issue("acme/split", 90, "Split")], [], "acme/split", [], undefined, [ + merged("acme/split", 900, { linkedIssues: [90], authorLogin: "other" }), + ]); + assert.equal(first(splitSolved).status, "do_not_use"); +}); + +test("buildIssueQualityReport: stale issues warn and age over 180 days costs score", () => { + const r = laneRepo("acme/stale", 1); + const stale = build(r, [issue("acme/stale", 30, "Old", { updatedAt: daysAgoIso(100), createdAt: daysAgoIso(100) })], [], "acme/stale"); + assert.equal(first(stale).status, "needs_proof"); + assert.ok(first(stale).warnings.some((w) => /stale/i.test(w))); + + const ancient = build(r, [issue("acme/stale", 31, "Ancient", { updatedAt: daysAgoIso(200), createdAt: daysAgoIso(200) })], [], "acme/stale"); + const fresh = build(r, [issue("acme/stale", 32, "Fresh")], [], "acme/stale"); + assert.ok(first(ancient).score < first(fresh).score); +}); + +test("buildIssueQualityReport: maintainer-authored and maintainer-WIP issues carry distinct warnings", () => { + const r = laneRepo("acme/maint", 1); + for (const association of ["OWNER", "MEMBER", "COLLABORATOR"]) { + const authored = build(r, [issue("acme/maint", 40, "From staff", { authorAssociation: association })], [], "acme/maint"); + assert.ok(first(authored).warnings.some((w) => /Maintainer-authored; confirm/i.test(w)), association); + } + + const wip = build(r, [issue("acme/maint", 41, "WIP", { authorAssociation: "MEMBER", labels: ["Work In Progress "] })], [], "acme/maint"); + assert.equal(first(wip).status, "needs_proof"); + assert.ok(first(wip).warnings.some((w) => /in-progress\/internal/i.test(w))); + // The WIP arm replaces the plain maintainer-authored warning rather than stacking with it. + assert.equal( + first(wip).warnings.some((w) => /Maintainer-authored; confirm/i.test(w)), + false, + ); + + // A non-maintainer carrying a WIP label is not treated as maintainer WIP. + const outsider = build(r, [issue("acme/maint", 42, "Outsider WIP", { authorAssociation: "NONE", labels: ["wip"] })], [], "acme/maint"); + assert.equal( + first(outsider).warnings.some((w) => /in-progress\/internal/i.test(w)), + false, + ); +}); + +test("buildIssueQualityReport: issue.linkedPrs back-references resolve, unknown ones warn from cache", () => { + const r = laneRepo("acme/backref", 1); + // The PR does not declare the issue; the issue declares the PR → resolveLinkedPullRequests adds it. + const backref = build(r, [issue("acme/backref", 50, "Backref", { linkedPrs: [77] })], [pr("acme/backref", 77, { linkedIssues: [] })], "acme/backref"); + assert.equal(first(backref).status, "do_not_use"); + assert.ok(first(backref).warnings.some((w) => /active PR/i.test(w))); + + // linkedPrs points at a PR absent from the fetched list → cached-metadata warning only. + const cachedOnly = build(r, [issue("acme/backref", 51, "Cached", { linkedPrs: [999] })], [], "acme/backref"); + assert.ok(first(cachedOnly).warnings.some((w) => /already references PR\(s\): #999/.test(w))); + + // Merged-PR back-reference travels the same path through the recent-merged index. + const mergedBackref = build(r, [issue("acme/backref", 52, "Merged backref", { linkedPrs: [78] })], [], "acme/backref", [], undefined, [ + merged("acme/backref", 78, { linkedIssues: [] }), + ]); + assert.equal(first(mergedBackref).status, "do_not_use"); +}); + +test("buildIssueQualityReport: high-risk collision clusters block, lower risk only warns", () => { + const r = laneRepo("acme/collide", 1); + const cluster = (risk: string): Json => ({ + repoFullName: "acme/collide", + generatedAt: now(), + summary: { clusterCount: 1, highRiskCount: risk === "high" ? 1 : 0, itemsReviewed: 2 }, + clusters: [ + { + id: "c1", + risk, + reason: "overlap", + items: [ + { type: "issue", number: 60, title: "A" }, + { type: "pull_request", number: 1, title: "B" }, + ], + }, + ], + }); + + const high = build(r, [issue("acme/collide", 60, "A")], [], "acme/collide", [], cluster("high")); + assert.equal(first(high).status, "do_not_use"); + + const medium = build(r, [issue("acme/collide", 60, "A")], [], "acme/collide", [], cluster("medium")); + assert.ok(first(medium).warnings.some((w) => /duplicate or overlapping/i.test(w))); + assert.notEqual(first(medium).status, "do_not_use"); +}); + +test("buildIssueQualityReport: results sort by score desc then issue number asc", () => { + const r = laneRepo("acme/sort", 1); + const report = build( + r, + [issue("acme/sort", 2, "Thin", { body: "Short." }), issue("acme/sort", 1, "Ready"), issue("acme/sort", 3, "Ready too")], + [], + "acme/sort", + [], + emptyCollisions("acme/sort"), + ); + assert.deepEqual( + report.issues.map((i) => i.number), + [1, 3, 2], + ); +}); + +test("buildIssueQualityReport: a null repo, absent dates and a blank bounty status degrade safely", () => { + const report = build(null, [issue("acme/null", 70, "No dates", { updatedAt: null, createdAt: null, body: null })], [], "acme/null", [ + bounty("acme/null", 70, " "), + ]); + assert.equal(report.lane.lane, "unknown"); + assert.equal(first(report).status, "needs_proof"); + + // Unparseable timestamps normalize to age 0 rather than throwing or reading as ancient. + const r = laneRepo("acme/null", 1); + const badDate = build(r, [issue("acme/null", 71, "Bad date", { updatedAt: "not-a-date", createdAt: "also-bad" })], [], "acme/null"); + assert.equal(first(badDate).status, "ready"); +}); + +test("buildIssueQualityReport: repeated linked-issue references collapse into one PR bucket", () => { + const r = laneRepo("acme/multi", 1); + const multi = build( + r, + [issue("acme/multi", 91, "Crowded")], + [pr("acme/multi", 1, { linkedIssues: [91] }), pr("acme/multi", 2, { linkedIssues: [91, 91] })], + "acme/multi", + ); + assert.equal(first(multi).status, "do_not_use"); + assert.ok(first(multi).warnings.some((w) => /^2 active PR/.test(w))); +}); + +test("buildIssueQualityReport: an open issue past the lifecycle cap is still classified (#6141)", () => { + const r = laneRepo("acme/cap", 1); + const filler = Array.from({ length: 300 }, (_, i) => issue("acme/cap", i + 1, `Closed ${i + 1}`, { state: "closed" })); + const beyondCap = issue("acme/cap", 301, "Beyond cap duplicate", { labels: ["duplicate"] }); + const report = build(r, [...filler, beyondCap], [], "acme/cap"); + assert.equal(report.issues.length, 1); + assert.equal(first(report).number, 301); + assert.equal(first(report).status, "do_not_use"); +}); + +test("buildIssueQualityReport: the report caps at 100 issues", () => { + const r = laneRepo("acme/cap100", 1); + const many = Array.from({ length: 140 }, (_, i) => issue("acme/cap100", i + 1, `Open ${i + 1}`)); + const report = build(r, many, [], "acme/cap100"); + assert.equal(report.issues.length, 100); +});