From 6997b6b76874b2cc1799f4436836a80d2f2bf7ee Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 15:00:27 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat(enrichment):=20history=20analyzer=20?= =?UTF-8?q?=E2=80=94=20author=20record,=20similar=20PRs,=20issue=20alignme?= =?UTF-8?q?nt=20(#1478)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- review-enrichment/src/analyzers/history.ts | 368 +++++++++++++++++++++ review-enrichment/src/brief.ts | 2 + review-enrichment/src/render.ts | 34 ++ review-enrichment/src/types.ts | 41 +++ review-enrichment/test/history.test.ts | 283 ++++++++++++++++ 5 files changed, 728 insertions(+) create mode 100644 review-enrichment/src/analyzers/history.ts create mode 100644 review-enrichment/test/history.test.ts diff --git a/review-enrichment/src/analyzers/history.ts b/review-enrichment/src/analyzers/history.ts new file mode 100644 index 0000000000..b884cc00cf --- /dev/null +++ b/review-enrichment/src/analyzers/history.ts @@ -0,0 +1,368 @@ +// Author / change-area history analyzer (#1478). Surfaces public-safe historical context the no-checkout +// `claude --print` reviewer is blind to and the engine deliberately does NOT compute: the PR author's track record +// IN THIS repo (prior merged/closed PRs, account age, first-time flag), past PRs that already changed the same files +// (with their merged/reverted outcome — revert/regression history), and whether the diff covers the linked issue's +// stated requirement. It surfaces ONLY public GitHub facts — never the engine's internal submitter reputation, nor +// any trust / reward / score value (those are private and intentionally absent here). +// +// Author context + similar-PR history use the request's short-lived githubToken; linked-issue alignment uses the +// linkedIssue passed in the request envelope and needs no fetch. Every GitHub call is wrapped so a missing token or +// a rate-limit/error degrades THIS analyzer only (the block is returned with `partial: true`) — the rest of the +// brief still ships. Fail-safe: returns [] when there is nothing to report. +import type { EnrichRequest, HistoryFinding } from "../types.js"; + +const GITHUB_API = "https://api.github.com"; +const GITHUB_API_VERSION = "2022-11-28"; +const MAX_FILES_PROBED = 5; // bound the per-file commit-history fan-out +const COMMITS_PER_FILE = 10; // recent commits to inspect per probed file +const MAX_PR_LOOKUPS = 12; // global cap on commit→PR resolution calls +const MAX_SIMILAR_PRS = 8; // cap the rendered similar-PR list +const MIN_TOKEN_LENGTH = 4; // requirement keywords shorter than this are ignored +const FULL_COVERAGE_RATIO = 0.6; // >= this share of requirement keywords present in the diff ⇒ "full" + +// A single repo path segment (owner or name): word chars, dot, dash only. Whole-segment `.`/`..` are rejected +// separately so a hostile repoFullName can't traverse or redirect the token-bearing request to another repository. +const REPO_SEGMENT = /^[A-Za-z0-9._-]+$/; +const SHA_RE = /^[0-9a-fA-F]{7,64}$/; + +// Generic English + PR/issue-boilerplate words carry no signal about WHAT the issue asks for, so they are dropped +// before measuring requirement-vs-diff overlap (otherwise every diff would "cover" "feature"/"add"/"update"). +const REQUIREMENT_STOPWORDS = new Set([ + "this", "that", "with", "from", "into", "when", "then", "than", "they", "them", + "your", "have", "will", "shall", "should", "would", "could", "about", "there", + "their", "which", "feat", "feature", "support", "implement", "implementation", + "issue", "pull", "request", "code", "test", "tests", "added", "adds", "change", + "changes", "update", "updates", "should", +]); + +interface ScanOptions { + signal?: AbortSignal; + /** Injectable clock so account-age math is deterministic in tests; defaults to Date.now(). */ + now?: number; +} + +/** Parse `owner/repo`, rejecting anything that isn't exactly two safe segments (no traversal, no extra slashes) so a + * hostile `repoFullName` cannot redirect the token-bearing request elsewhere. Returns null when unsafe. */ +export function parseRepo( + repoFullName: string, +): { owner: string; repo: string } | null { + const parts = repoFullName.split("/"); + if (parts.length !== 2) return null; + const [owner, repo] = parts; + for (const seg of [owner, repo]) { + if (!seg || seg === "." || seg === ".." || !REPO_SEGMENT.test(seg)) { + return null; + } + } + return { owner: owner!, repo: repo! }; +} + +function githubHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": GITHUB_API_VERSION, + "User-Agent": "gittensory-review-enrichment", + }; +} + +// ── Linked-issue alignment (no fetch — the issue text is in the envelope) ─────── + +/** Extract lowercased keyword tokens (length >= MIN_TOKEN_LENGTH, minus stopwords) from the issue's stated + * requirement so coverage can be measured against the diff. Deduplicated, in first-seen order. */ +export function requirementTokens(text: string): string[] { + const seen = new Set(); + for (const raw of text.toLowerCase().split(/[^a-z0-9]+/)) { + if (raw.length < MIN_TOKEN_LENGTH || REQUIREMENT_STOPWORDS.has(raw)) continue; + seen.add(raw); + } + return [...seen]; +} + +/** Classify how much of the linked issue's requirement the diff appears to cover: `none` (no keyword present), + * `full` (>= FULL_COVERAGE_RATIO of keywords present), else `partial`. Advisory keyword overlap, never a proof. */ +export function classifyCoverage( + requirement: string, + haystack: string, +): "full" | "partial" | "none" { + const tokens = requirementTokens(requirement); + if (tokens.length === 0) return "none"; + const hay = haystack.toLowerCase(); + const covered = tokens.filter((t) => hay.includes(t)).length; + if (covered === 0) return "none"; + return covered / tokens.length >= FULL_COVERAGE_RATIO ? "full" : "partial"; +} + +/** The added ('+') lines of the PR, from req.diff or the per-file patches — the text the requirement is matched + * against (alongside the changed file paths). */ +function diffAddedText(req: EnrichRequest): string { + const sources: string[] = []; + if (req.diff) sources.push(req.diff); + for (const f of req.files ?? []) if (f.patch) sources.push(f.patch); + const added: string[] = []; + for (const src of sources) { + for (const line of src.split("\n")) { + if (line.startsWith("+") && !line.startsWith("+++")) added.push(line.slice(1)); + } + } + return added.join("\n"); +} + +/** Build the linked-issue alignment block from the envelope-provided issue text + the diff. `null` when there is no + * linked issue, or it carries no title/body to assess. */ +export function buildLinkedIssueAlignment( + req: EnrichRequest, +): HistoryFinding["linkedIssueAlignment"] { + const issue = req.linkedIssue; + const title = issue?.title?.trim() ?? ""; + const body = issue?.body?.trim() ?? ""; + if (!issue || (!title && !body)) return null; + const statedRequirement = (title || body.split("\n")[0] || "").slice(0, 160); + const requirementText = `${title}\n${body}`; + const haystack = `${(req.files ?? []) + .map((f) => f.path) + .join(" ")}\n${diffAddedText(req)}`; + return { + issue: issue.number, + statedRequirement, + diffCovers: classifyCoverage(requirementText, haystack), + }; +} + +// ── Author track record (GitHub Search + Users API) ───────────────────────────── + +/** Issue/PR-search `total_count` for a query, or null on a non-OK reply / network error (so the caller degrades). */ +async function fetchSearchCount( + query: string, + token: string, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise { + try { + const url = `${GITHUB_API}/search/issues?q=${encodeURIComponent(query)}&per_page=1`; + const res = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!res.ok) return null; + const json = (await res.json()) as { total_count?: number }; + return typeof json.total_count === "number" ? json.total_count : null; + } catch { + return null; + } +} + +/** Account age in whole days from the Users API `created_at`, or null when unavailable / unparseable. */ +async function fetchAccountAgeDays( + login: string, + token: string, + fetchImpl: typeof fetch, + now: number, + signal?: AbortSignal, +): Promise { + try { + const url = `${GITHUB_API}/users/${encodeURIComponent(login)}`; + const res = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!res.ok) return null; + const json = (await res.json()) as { created_at?: string }; + if (!json.created_at) return null; + const created = Date.parse(json.created_at); + if (Number.isNaN(created)) return null; + return Math.max(0, Math.floor((now - created) / 86_400_000)); + } catch { + return null; + } +} + +/** Author track record in this repo. `partial` is true when any sub-query failed (the counts then fall back to 0). */ +async function buildAuthorContext( + owner: string, + repo: string, + author: string, + token: string, + fetchImpl: typeof fetch, + now: number, + signal?: AbortSignal, +): Promise<{ author: NonNullable; partial: boolean }> { + const repoQ = `repo:${owner}/${repo} type:pr author:${author}`; + const merged = await fetchSearchCount(`${repoQ} is:merged`, token, fetchImpl, signal); + const closed = await fetchSearchCount(`${repoQ} is:unmerged is:closed`, token, fetchImpl, signal); + const accountAgeDays = await fetchAccountAgeDays(author, token, fetchImpl, now, signal); + const priorMergedInRepo = merged ?? 0; + const priorClosedInRepo = closed ?? 0; + return { + author: { + priorMergedInRepo, + priorClosedInRepo, + accountAgeDays, + firstTimeContributor: priorMergedInRepo === 0 && priorClosedInRepo === 0, + }, + partial: merged === null || closed === null || accountAgeDays === null, + }; +} + +// ── Similar past PRs (commits-by-path → associated PRs, with revert detection) ─── + +/** Recent commits touching `path` as {sha, message}, or null on a non-OK reply / network error. */ +async function fetchCommitsForPath( + owner: string, + repo: string, + path: string, + token: string, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise | null> { + try { + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits?path=${encodeURIComponent(path)}&per_page=${COMMITS_PER_FILE}`; + const res = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!res.ok) return null; + const json = (await res.json()) as Array<{ + sha?: string; + commit?: { message?: string }; + }>; + if (!Array.isArray(json)) return null; + const out: Array<{ sha: string; message: string }> = []; + for (const c of json) { + if (typeof c.sha === "string") { + out.push({ sha: c.sha, message: c.commit?.message ?? "" }); + } + } + return out; + } catch { + return null; + } +} + +/** PRs associated with a commit as {number, title}, or null on a non-OK reply / network error. */ +async function fetchPullsForCommit( + owner: string, + repo: string, + sha: string, + token: string, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise | null> { + if (!SHA_RE.test(sha)) return []; + try { + const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(sha)}/pulls`; + const res = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!res.ok) return null; + const json = (await res.json()) as Array<{ number?: number; title?: string }>; + if (!Array.isArray(json)) return null; + const out: Array<{ number: number; title: string }> = []; + for (const p of json) { + if (typeof p.number === "number") { + out.push({ number: p.number, title: typeof p.title === "string" ? p.title : "" }); + } + } + return out; + } catch { + return null; + } +} + +/** Collect PR numbers referenced by a revert commit message (`Revert "…" (#N)`, `This reverts … #N`) into `into`. */ +export function collectRevertRefs( + message: string | undefined, + into: Set, +): void { + if (!message || !/\brevert/i.test(message)) return; + for (const m of message.matchAll(/#(\d+)/g)) { + const n = Number(m[1]); + if (Number.isInteger(n) && n > 0) into.add(n); + } +} + +/** Past PRs that already changed the same files. `partial` is true when any commit/PR lookup failed or the global + * lookup budget capped the scan. A PR referenced by a revert commit is marked `reverted`; otherwise `merged` + * (commits-by-path only surface merged history). The current PR is excluded. */ +async function buildSimilarPastPrs( + owner: string, + repo: string, + token: string, + files: NonNullable, + currentPrNumber: number, + fetchImpl: typeof fetch, + signal?: AbortSignal, +): Promise<{ similarPastPrs: HistoryFinding["similarPastPrs"]; partial: boolean }> { + let partial = false; + let lookups = 0; + const revertedRefs = new Set(); + const prs = new Map }>(); + + for (const file of files.slice(0, MAX_FILES_PROBED)) { + const commits = await fetchCommitsForPath(owner, repo, file.path, token, fetchImpl, signal); + if (commits === null) { + partial = true; + continue; + } + for (const commit of commits) { + collectRevertRefs(commit.message, revertedRefs); + if (lookups >= MAX_PR_LOOKUPS) { + partial = true; + continue; + } + lookups++; + const pulls = await fetchPullsForCommit(owner, repo, commit.sha, token, fetchImpl, signal); + if (pulls === null) { + partial = true; + continue; + } + for (const pull of pulls) { + if (pull.number === currentPrNumber) continue; + const existing = prs.get(pull.number) ?? { title: pull.title, overlap: new Set() }; + existing.overlap.add(file.path); + prs.set(pull.number, existing); + } + } + } + + const similarPastPrs = [...prs.entries()] + .map(([number, value]) => ({ + number, + title: value.title, + outcome: (revertedRefs.has(number) ? "reverted" : "merged") as "merged" | "reverted", + overlapPaths: [...value.overlap].sort(), + })) + .sort((a, b) => b.number - a.number) + .slice(0, MAX_SIMILAR_PRS); + return { similarPastPrs, partial }; +} + +// ── Analyzer entrypoint ───────────────────────────────────────────────────────── + +/** Surface public-safe author + change-area history for the PR. Author context and similar-PR history need the + * request token; linked-issue alignment needs only the envelope. Fail-safe: any missing input or failed fetch + * degrades to a `partial` block (or [] when there is nothing to report) — never throws, never blocks the brief. */ +export async function scanHistory( + req: EnrichRequest, + fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + const now = options.now ?? Date.now(); + const repo = parseRepo(req.repoFullName); + const token = req.githubToken; + + let author: HistoryFinding["author"] = null; + let similarPastPrs: HistoryFinding["similarPastPrs"] = []; + let partial = false; + + if (repo && token && req.author) { + const ctx = await buildAuthorContext(repo.owner, repo.repo, req.author, token, fetchImpl, now, options.signal); + author = ctx.author; + if (ctx.partial) partial = true; + } else { + // No repo/token/author ⇒ the author track record can't be computed; flag the block as incomplete. + partial = true; + } + + if (repo && token && (req.files?.length ?? 0) > 0) { + const similar = await buildSimilarPastPrs(repo.owner, repo.repo, token, req.files!, req.prNumber, fetchImpl, options.signal); + similarPastPrs = similar.similarPastPrs; + if (similar.partial) partial = true; + } + + const linkedIssueAlignment = buildLinkedIssueAlignment(req); + + // Nothing to report (no token AND no linked issue) ⇒ contribute nothing, byte-identical to before the analyzer. + if (!author && similarPastPrs.length === 0 && !linkedIssueAlignment) return []; + + return [{ author, similarPastPrs, linkedIssueAlignment, partial }]; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index ed19645260..5343a70584 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -22,6 +22,7 @@ import { scanSecretLog } from "./analyzers/secret-log.js"; import { scanAssetWeight } from "./analyzers/asset-weight.js"; import { scanTyposquat } from "./analyzers/typosquat.js"; import { scanNativeBuild } from "./analyzers/native-build.js"; +import { scanHistory } from "./analyzers/history.js"; import { renderBrief } from "./render.js"; import { captureAnalyzerDegradation } from "./sentry.js"; @@ -46,6 +47,7 @@ const ANALYZERS: Record = { assetWeight: (req, signal) => scanAssetWeight(req, fetch, { signal }), typosquat: (req, signal) => scanTyposquat(req, fetch, { signal }), nativeBuild: (req, signal) => scanNativeBuild(req, fetch, { signal }), + history: (req, signal) => scanHistory(req, fetch, { signal }), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 7edcebbdbf..d6834639c4 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -293,6 +293,40 @@ export function renderBrief( } } + const history = findings.history ?? []; + for (const item of history) { + const entries: string[] = []; + if (item.author) { + const a = item.author; + const record = a.firstTimeContributor + ? "first-time contributor to this repo" + : `${a.priorMergedInRepo} merged / ${a.priorClosedInRepo} closed prior PRs here`; + const age = + a.accountAgeDays === null + ? "account age unknown" + : `account ${a.accountAgeDays}d old`; + entries.push(`- Author: ${record}; ${age}`); + } + for (const pr of item.similarPastPrs) { + const paths = pr.overlapPaths.map((p) => safeCodeSpan(p)).join(", "); + entries.push( + `- This area was previously changed in #${pr.number} (${pr.outcome}): ${promptText(pr.title)} — overlaps ${paths}`, + ); + } + if (item.linkedIssueAlignment) { + const al = item.linkedIssueAlignment; + entries.push( + `- Linked issue #${al.issue} coverage: **${al.diffCovers}** — ${promptText(al.statedRequirement)}`, + ); + } + if (entries.length) { + lines.push("### Author & change-area history (public GitHub record)"); + if (item.partial) + lines.push("- _(partial — some history could not be retrieved)_"); + lines.push(...entries); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 639b7d1970..1dce08d169 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -21,10 +21,21 @@ export interface EnrichRequest { diff?: string; /** Optional GitHub read token for GitHub-backed analyzers. Never logged. */ githubToken?: string; + /** The PR's linked issue, resolved engine-side and passed in the envelope so the history analyzer can judge + * whether the diff covers the issue's stated requirement without an extra fetch. Absent ⇒ alignment omitted. (#1478) */ + linkedIssue?: EnrichLinkedIssue; budget?: { timeoutMs?: number; maxBriefChars?: number }; analyzers?: string[]; } +/** A PR's linked issue, as carried in the request envelope. `title`/`body` hold the stated requirement the history + * analyzer measures the diff against; only the number is mandatory. (#1478) */ +export interface EnrichLinkedIssue { + number: number; + title?: string; + body?: string; +} + /** A known vulnerability for a dependency version, sourced from OSV.dev. */ export interface Cve { id: string; @@ -193,6 +204,35 @@ export interface NativeBuildFinding { reason: string; } +/** Public-safe historical context the no-checkout reviewer is blind to and the engine deliberately does NOT compute: + * the author's track record IN THIS repo, past PRs that already changed the same files (with their outcome), and + * whether the diff covers the linked issue's stated requirement. Surfaced as a single block (0-or-1 element array). + * Carries ONLY public GitHub facts — never the engine's internal submitter reputation, trust, reward, or score. (#1478) */ +export interface HistoryFinding { + /** Author track record in THIS repo. `null` when no token/author was available to query the GitHub API. */ + author: { + priorMergedInRepo: number; + priorClosedInRepo: number; + accountAgeDays: number | null; + firstTimeContributor: boolean; + } | null; + /** Past PRs that already changed the same files, with the outcome of each and the overlapping paths. */ + similarPastPrs: Array<{ + number: number; + title: string; + outcome: "merged" | "reverted"; + overlapPaths: string[]; + }>; + /** Whether the diff covers the linked issue's stated requirement. `null` when the PR has no linked issue. */ + linkedIssueAlignment: { + issue: number; + statedRequirement: string; + diffCovers: "full" | "partial" | "none"; + } | null; + /** True when a GitHub sub-query was skipped (no token) or degraded (rate-limit/error), so the block is incomplete. */ + partial: boolean; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -210,6 +250,7 @@ export interface BriefFindings { assetWeight?: AssetWeightFinding[]; typosquat?: TyposquatFinding[]; nativeBuild?: NativeBuildFinding[]; + history?: HistoryFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/history.test.ts b/review-enrichment/test/history.test.ts new file mode 100644 index 0000000000..f0d48dbb2a --- /dev/null +++ b/review-enrichment/test/history.test.ts @@ -0,0 +1,283 @@ +// Units for the author / change-area history analyzer (#1478). Kept in its own file (not enrichment.test.ts) so +// concurrent analyzer PRs don't collide on a shared test file. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + parseRepo, + requirementTokens, + classifyCoverage, + collectRevertRefs, + buildLinkedIssueAlignment, + scanHistory, +} from "../dist/analyzers/history.js"; +import { renderBrief } from "../dist/render.js"; + +// Minimal Response-like stubs (ok/status/json/text), matching the other analyzer tests. +const res = (body, { ok = true, status = 200 } = {}) => ({ + ok, + status, + json: async () => body, + text: async () => JSON.stringify(body), +}); +const notOk = (status) => res({}, { ok: false, status }); +const throwingFetch = async () => { + throw new Error("network down"); +}; + +// A tiny URL router so each test declares only the endpoints it exercises. Routes are matched in order; a string +// matches by substring, a RegExp by test(). Unmatched URLs 404 (so a stray call degrades rather than silently passing). +function router(routes) { + return async (url) => { + for (const [match, handler] of routes) { + const hit = typeof match === "string" ? url.includes(match) : match.test(url); + if (hit) return handler; + } + return notOk(404); + }; +} + +const NOW = Date.parse("2026-06-29T00:00:00Z"); + +test("scanHistory: author track record for a repeat contributor", async () => { + const fetchImpl = router([ + ["is%3Aunmerged", res({ total_count: 2 })], // closed-unmerged (checked first; its URL lacks is%3Amerged) + ["is%3Amerged", res({ total_count: 7 })], + ["/users/octocat", res({ created_at: "2020-01-01T00:00:00Z" })], + ]); + const out = await scanHistory( + { repoFullName: "o/r", prNumber: 5, author: "octocat", githubToken: "t", files: [] }, + fetchImpl, + { now: NOW }, + ); + assert.equal(out.length, 1); + const a = out[0].author; + assert.equal(a.priorMergedInRepo, 7); + assert.equal(a.priorClosedInRepo, 2); + assert.equal(a.firstTimeContributor, false); + assert.ok(a.accountAgeDays > 2000); + assert.equal(out[0].partial, false); + assert.equal(out[0].linkedIssueAlignment, null); +}); + +test("scanHistory: flags a first-time contributor with account age", async () => { + const fetchImpl = router([ + ["is%3Aunmerged", res({ total_count: 0 })], + ["is%3Amerged", res({ total_count: 0 })], + ["/users/newbie", res({ created_at: "2026-06-01T00:00:00Z" })], + ]); + const out = await scanHistory( + { repoFullName: "o/r", prNumber: 1, author: "newbie", githubToken: "t", files: [] }, + fetchImpl, + { now: NOW }, + ); + assert.equal(out[0].author.firstTimeContributor, true); + assert.equal(out[0].author.accountAgeDays, 28); + assert.equal(out[0].partial, false); +}); + +test("scanHistory: surfaces similar past PRs and marks a reverted one", async () => { + const shaA = "a".repeat(40); + const shaB = "b".repeat(40); + const fetchImpl = router([ + ["is%3Aunmerged", res({ total_count: 0 })], + ["is%3Amerged", res({ total_count: 1 })], + ["/users/dev", res({ created_at: "2024-01-01T00:00:00Z" })], + [ + "/commits?path=", + res([ + { sha: shaA, commit: { message: 'Revert "add foo" (#10)' } }, + { sha: shaB, commit: { message: "add foo" } }, + ]), + ], + [new RegExp(`/commits/${shaA}/pulls`), res([{ number: 11, title: 'Revert "add foo" (#10)' }])], + [new RegExp(`/commits/${shaB}/pulls`), res([{ number: 10, title: "add foo" }])], + ]); + const out = await scanHistory( + { + repoFullName: "o/r", + prNumber: 5, + author: "dev", + githubToken: "t", + files: [{ path: "src/foo.ts", status: "modified" }], + }, + fetchImpl, + { now: NOW }, + ); + const prs = out[0].similarPastPrs; + const pr10 = prs.find((p) => p.number === 10); + const pr11 = prs.find((p) => p.number === 11); + assert.equal(pr10.outcome, "reverted"); + assert.equal(pr11.outcome, "merged"); + assert.deepEqual(pr10.overlapPaths, ["src/foo.ts"]); +}); + +test("scanHistory: excludes the current PR from similar past PRs", async () => { + const shaA = "c".repeat(40); + const fetchImpl = router([ + [/\/search\/issues/, res({ total_count: 0 })], + ["/users/dev", res({ created_at: "2024-01-01T00:00:00Z" })], + ["/commits?path=", res([{ sha: shaA, commit: { message: "touch foo" } }])], + [new RegExp(`/commits/${shaA}/pulls`), res([{ number: 5, title: "the current PR" }])], + ]); + const out = await scanHistory( + { repoFullName: "o/r", prNumber: 5, author: "dev", githubToken: "t", files: [{ path: "src/foo.ts" }] }, + fetchImpl, + { now: NOW }, + ); + assert.deepEqual(out[0].similarPastPrs, []); +}); + +test("buildLinkedIssueAlignment: full / partial / none / absent", () => { + const withDiff = (diff) => ({ + repoFullName: "o/r", + prNumber: 1, + linkedIssue: { number: 42, title: "add history analyzer enrichment" }, + diff, + }); + assert.equal(buildLinkedIssueAlignment(withDiff("+history analyzer enrichment")).diffCovers, "full"); + assert.equal(buildLinkedIssueAlignment(withDiff("+only history here")).diffCovers, "partial"); + assert.equal(buildLinkedIssueAlignment(withDiff("+nothing relevant")).diffCovers, "none"); + assert.equal(buildLinkedIssueAlignment({ repoFullName: "o/r", prNumber: 1 }), null); + const alignment = buildLinkedIssueAlignment(withDiff("+history analyzer enrichment")); + assert.equal(alignment.issue, 42); + assert.equal(alignment.statedRequirement, "add history analyzer enrichment"); +}); + +test("scanHistory: no token still ships linked-issue alignment as a partial block", async () => { + const out = await scanHistory( + { + repoFullName: "o/r", + prNumber: 1, + linkedIssue: { number: 42, title: "add history analyzer" }, + diff: "+history analyzer", + }, + throwingFetch, // must NOT be called without a token + ); + assert.equal(out.length, 1); + assert.equal(out[0].author, null); + assert.equal(out[0].partial, true); + assert.equal(out[0].linkedIssueAlignment.diffCovers, "full"); + assert.deepEqual(out[0].similarPastPrs, []); +}); + +test("scanHistory: returns [] when there is no token and no linked issue", async () => { + assert.deepEqual( + await scanHistory({ repoFullName: "o/r", prNumber: 1, files: [] }, throwingFetch), + [], + ); +}); + +test("scanHistory: a rate-limited GitHub query degrades the block (partial) without throwing", async () => { + const out = await scanHistory( + { + repoFullName: "o/r", + prNumber: 1, + author: "dev", + githubToken: "t", + files: [], + linkedIssue: { number: 9, title: "do the thing properly" }, + }, + router([ + ["/search/issues", notOk(403)], + ["/users/", notOk(403)], + ]), + ); + assert.equal(out.length, 1); + assert.equal(out[0].partial, true); + assert.equal(out[0].author.priorMergedInRepo, 0); + assert.equal(out[0].author.firstTimeContributor, true); // counts defaulted to 0 on failure + assert.equal(out[0].linkedIssueAlignment.issue, 9); // the rest of the block still ships +}); + +test("scanHistory: a thrown GitHub fetch degrades safely", async () => { + const out = await scanHistory( + { + repoFullName: "o/r", + prNumber: 1, + author: "dev", + githubToken: "t", + files: [{ path: "src/x.ts", status: "modified" }], + }, + throwingFetch, + ); + assert.equal(out.length, 1); + assert.equal(out[0].partial, true); + assert.deepEqual(out[0].similarPastPrs, []); +}); + +test("scanHistory: an unsafe repoFullName is rejected before any fetch", async () => { + const out = await scanHistory( + { repoFullName: "o/r/../x", prNumber: 1, author: "dev", githubToken: "t", files: [] }, + throwingFetch, + ); + assert.deepEqual(out, []); +}); + +test("requirementTokens drops short words and stopwords", () => { + assert.deepEqual(requirementTokens("Add the History Analyzer to enrichment"), [ + "history", + "analyzer", + "enrichment", + ]); +}); + +test("classifyCoverage thresholds", () => { + assert.equal(classifyCoverage("history analyzer enrichment", "history analyzer enrichment"), "full"); + assert.equal(classifyCoverage("history analyzer enrichment", "history only"), "partial"); + assert.equal(classifyCoverage("history analyzer enrichment", "unrelated"), "none"); + assert.equal(classifyCoverage("", "anything"), "none"); +}); + +test("collectRevertRefs only collects from revert messages", () => { + const s1 = new Set(); + collectRevertRefs('Revert "x" (#10) and #12', s1); + assert.deepEqual([...s1].sort((a, b) => a - b), [10, 12]); + const s2 = new Set(); + collectRevertRefs("normal commit referencing #5", s2); + assert.equal(s2.size, 0); + collectRevertRefs(undefined, s2); + assert.equal(s2.size, 0); +}); + +test("parseRepo rejects unsafe names", () => { + assert.deepEqual(parseRepo("o/r"), { owner: "o", repo: "r" }); + assert.equal(parseRepo("o"), null); + assert.equal(parseRepo("o/r/x"), null); + assert.equal(parseRepo("../x"), null); + assert.equal(parseRepo("o/.."), null); +}); + +test("renderBrief emits a public-safe history block", () => { + const { promptSection } = renderBrief({ + history: [ + { + author: { priorMergedInRepo: 7, priorClosedInRepo: 2, accountAgeDays: 1500, firstTimeContributor: false }, + similarPastPrs: [{ number: 10, title: "add foo", outcome: "reverted", overlapPaths: ["src/foo.ts"] }], + linkedIssueAlignment: { issue: 42, statedRequirement: "add the history analyzer", diffCovers: "partial" }, + partial: false, + }, + ], + }); + assert.match(promptSection, /Author & change-area history/); + assert.match(promptSection, /7 merged \/ 2 closed/); + assert.match(promptSection, /previously changed in #10 \(reverted\)/); + assert.match(promptSection, /Linked issue #42 coverage: \*\*partial\*\*/); +}); + +test("renderBrief notes a partial history block and omits an empty one", () => { + const partialOut = renderBrief({ + history: [ + { + author: null, + similarPastPrs: [], + linkedIssueAlignment: { issue: 1, statedRequirement: "x", diffCovers: "none" }, + partial: true, + }, + ], + }); + assert.match(partialOut.promptSection, /partial — some history/); + const emptyOut = renderBrief({ + history: [{ author: null, similarPastPrs: [], linkedIssueAlignment: null, partial: true }], + }); + assert.equal(emptyOut.promptSection, ""); +}); From 35bfd79d5b7113743ee0fd715345244067d5baf0 Mon Sep 17 00:00:00 2001 From: dev-miro26 Date: Mon, 29 Jun 2026 15:47:55 -0700 Subject: [PATCH 2/2] fix(enrichment): treat failed author-history lookups as unknown, not first-time (#1478) --- review-enrichment/src/analyzers/history.ts | 26 +++++++++++++--------- review-enrichment/src/render.ts | 19 ++++++++++++---- review-enrichment/src/types.ts | 8 ++++--- review-enrichment/test/history.test.ts | 26 +++++++++++++--------- 4 files changed, 51 insertions(+), 28 deletions(-) diff --git a/review-enrichment/src/analyzers/history.ts b/review-enrichment/src/analyzers/history.ts index b884cc00cf..751f76848f 100644 --- a/review-enrichment/src/analyzers/history.ts +++ b/review-enrichment/src/analyzers/history.ts @@ -87,8 +87,10 @@ export function classifyCoverage( ): "full" | "partial" | "none" { const tokens = requirementTokens(requirement); if (tokens.length === 0) return "none"; - const hay = haystack.toLowerCase(); - const covered = tokens.filter((t) => hay.includes(t)).length; + // Match whole words only — tokenize the haystack the same way the requirement is tokenized — so a short keyword + // can't be "covered" by an unrelated word it is a substring of (e.g. `test` inside `latest`). (#1478) + const hayWords = new Set(haystack.toLowerCase().split(/[^a-z0-9]+/)); + const covered = tokens.filter((t) => hayWords.has(t)).length; if (covered === 0) return "none"; return covered / tokens.length >= FULL_COVERAGE_RATIO ? "full" : "partial"; } @@ -185,14 +187,16 @@ async function buildAuthorContext( const merged = await fetchSearchCount(`${repoQ} is:merged`, token, fetchImpl, signal); const closed = await fetchSearchCount(`${repoQ} is:unmerged is:closed`, token, fetchImpl, signal); const accountAgeDays = await fetchAccountAgeDays(author, token, fetchImpl, now, signal); - const priorMergedInRepo = merged ?? 0; - const priorClosedInRepo = closed ?? 0; + // A failed Search lookup is UNKNOWN, not zero — keep it null so a 403 / rate-limit can never be rendered as a + // first-time contributor. firstTimeContributor is decided ONLY when both counts are known. (#1478) + const firstTimeContributor = + merged === null || closed === null ? null : merged === 0 && closed === 0; return { author: { - priorMergedInRepo, - priorClosedInRepo, + priorMergedInRepo: merged, + priorClosedInRepo: closed, accountAgeDays, - firstTimeContributor: priorMergedInRepo === 0 && priorClosedInRepo === 0, + firstTimeContributor, }, partial: merged === null || closed === null || accountAgeDays === null, }; @@ -258,13 +262,15 @@ async function fetchPullsForCommit( } } -/** Collect PR numbers referenced by a revert commit message (`Revert "…" (#N)`, `This reverts … #N`) into `into`. */ +/** Collect the reverted PR number(s) from a revert commit/PR message into `into`. GitHub's revert title is + * `Revert " (#N)"` — the reverted PR is the number INSIDE the quoted original title, so we match + * only that. This avoids misclassifying a trailing revert-PR number or an unrelated `fixes #X` in the body. (#1478) */ export function collectRevertRefs( message: string | undefined, into: Set, ): void { - if (!message || !/\brevert/i.test(message)) return; - for (const m of message.matchAll(/#(\d+)/g)) { + if (!message) return; + for (const m of message.matchAll(/\brevert\s+"[^"]*\(#(\d+)\)"/gi)) { const n = Number(m[1]); if (Number.isInteger(n) && n > 0) into.add(n); } diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index b02e6304bf..42d44010f6 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -329,9 +329,18 @@ export function renderBrief( const entries: string[] = []; if (item.author) { const a = item.author; - const record = a.firstTimeContributor - ? "first-time contributor to this repo" - : `${a.priorMergedInRepo} merged / ${a.priorClosedInRepo} closed prior PRs here`; + let record: string; + if ( + a.firstTimeContributor === null || + a.priorMergedInRepo === null || + a.priorClosedInRepo === null + ) { + record = "prior PR history unavailable"; + } else if (a.firstTimeContributor) { + record = "first-time contributor to this repo"; + } else { + record = `${a.priorMergedInRepo} merged / ${a.priorClosedInRepo} closed prior PRs here`; + } const age = a.accountAgeDays === null ? "account age unknown" @@ -339,7 +348,9 @@ export function renderBrief( entries.push(`- Author: ${record}; ${age}`); } for (const pr of item.similarPastPrs) { - const paths = pr.overlapPaths.map((p) => safeCodeSpan(p)).join(", "); + const paths = pr.overlapPaths.length + ? pr.overlapPaths.map((p) => safeCodeSpan(p)).join(", ") + : "unknown paths"; entries.push( `- This area was previously changed in #${pr.number} (${pr.outcome}): ${promptText(pr.title)} — overlaps ${paths}`, ); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index c6a089b3ff..f9df311fc7 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -225,10 +225,12 @@ export interface NativeBuildFinding { export interface HistoryFinding { /** Author track record in THIS repo. `null` when no token/author was available to query the GitHub API. */ author: { - priorMergedInRepo: number; - priorClosedInRepo: number; + /** Prior PRs by this author in this repo; `null` when the GitHub Search lookup failed / was unavailable. */ + priorMergedInRepo: number | null; + priorClosedInRepo: number | null; accountAgeDays: number | null; - firstTimeContributor: boolean; + /** `true`/`false` ONLY when both PR-count lookups succeeded; `null` when a count was unavailable (never guessed). */ + firstTimeContributor: boolean | null; } | null; /** Past PRs that already changed the same files, with the outcome of each and the overlapping paths. */ similarPastPrs: Array<{ diff --git a/review-enrichment/test/history.test.ts b/review-enrichment/test/history.test.ts index f0d48dbb2a..5a3bb8e2a1 100644 --- a/review-enrichment/test/history.test.ts +++ b/review-enrichment/test/history.test.ts @@ -85,11 +85,11 @@ test("scanHistory: surfaces similar past PRs and marks a reverted one", async () [ "/commits?path=", res([ - { sha: shaA, commit: { message: 'Revert "add foo" (#10)' } }, + { sha: shaA, commit: { message: 'Revert "add foo (#10)"' } }, { sha: shaB, commit: { message: "add foo" } }, ]), ], - [new RegExp(`/commits/${shaA}/pulls`), res([{ number: 11, title: 'Revert "add foo" (#10)' }])], + [new RegExp(`/commits/${shaA}/pulls`), res([{ number: 11, title: 'Revert "add foo (#10)"' }])], [new RegExp(`/commits/${shaB}/pulls`), res([{ number: 10, title: "add foo" }])], ]); const out = await scanHistory( @@ -184,8 +184,8 @@ test("scanHistory: a rate-limited GitHub query degrades the block (partial) with ); assert.equal(out.length, 1); assert.equal(out[0].partial, true); - assert.equal(out[0].author.priorMergedInRepo, 0); - assert.equal(out[0].author.firstTimeContributor, true); // counts defaulted to 0 on failure + assert.equal(out[0].author.priorMergedInRepo, null); // a failed lookup is UNKNOWN, not zero + assert.equal(out[0].author.firstTimeContributor, null); // never claim a first-timer on a degraded lookup assert.equal(out[0].linkedIssueAlignment.issue, 9); // the rest of the block still ships }); @@ -228,15 +228,19 @@ test("classifyCoverage thresholds", () => { assert.equal(classifyCoverage("", "anything"), "none"); }); -test("collectRevertRefs only collects from revert messages", () => { +test("collectRevertRefs collects only the reverted PR from a GitHub revert title", () => { const s1 = new Set(); - collectRevertRefs('Revert "x" (#10) and #12', s1); - assert.deepEqual([...s1].sort((a, b) => a - b), [10, 12]); + collectRevertRefs('Revert "add foo (#10)"', s1); + assert.deepEqual([...s1], [10]); + // The trailing revert-PR number and an unrelated `fixes #N` in the body are NOT collected. const s2 = new Set(); - collectRevertRefs("normal commit referencing #5", s2); - assert.equal(s2.size, 0); - collectRevertRefs(undefined, s2); - assert.equal(s2.size, 0); + collectRevertRefs('Revert "add foo (#10)" (#20)\n\nThis reverts commit abc123. fixes #99', s2); + assert.deepEqual([...s2], [10]); + const s3 = new Set(); + collectRevertRefs("normal commit referencing #5", s3); + assert.equal(s3.size, 0); + collectRevertRefs(undefined, s3); + assert.equal(s3.size, 0); }); test("parseRepo rejects unsafe names", () => {