From 4585082f8a4e8b2dfa65e14c5051a7fd8026e141 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 20:16:16 -0700 Subject: [PATCH] fix(rees): harden SLUG_RE against dot-segment path traversal in 17 analyzers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change SLUG_RE from the weak /^[A-Za-z0-9._-]+$/ to /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/ in all 17 review-enrichment analyzers still using the old pattern, matching duplication-delta.ts and codeowners.ts (the only two files already hardened). A bare [A-Za-z0-9._-]+ class lets a slug segment made entirely of dots (e.g. owner="..") pass validation, since every character in ".." is individually allowed. These owner/repo values get spliced into GitHub Contents-API URLs passed to fetch()/new URL(); a leading-dot segment could let a URL parser's dot-segment resolution rewrite the path, sending the auth token somewhere other than the intended owner/repo. Requiring an alphanumeric first character closes that gap without any behavior change for a legitimate slug (GitHub owner/repo names always start with an alphanumeric character). Each changed analyzer gets a test confirming a ".."-shaped owner/repo segment is rejected (fails safe, no finding, never throws) — extending an existing invalid-slug test where one already existed, adding a new one otherwise. Also normalizes three test files (exhaustiveness-drift, flaky-test, unused-export) from pre-existing mixed CRLF/LF line endings to plain LF, matching every other file in the package; the mixed endings were already present on main and were tripping git diff --check on these files now that they're touched. --- .../src/analyzers/approval-integrity.ts | 2 +- review-enrichment/src/analyzers/blame-link.ts | 2 +- .../src/analyzers/caller-impact.ts | 2 +- .../src/analyzers/churn-hotspot.ts | 2 +- .../src/analyzers/commit-hygiene.ts | 2 +- .../src/analyzers/commit-lint.ts | 2 +- .../src/analyzers/commit-signature.ts | 2 +- .../src/analyzers/complexity-delta.ts | 2 +- .../src/analyzers/coverage-delta.ts | 2 +- .../src/analyzers/doc-comment-drift.ts | 2 +- .../src/analyzers/exhaustiveness-drift.ts | 2 +- review-enrichment/src/analyzers/flaky-test.ts | 2 +- .../src/analyzers/pending-review-requests.ts | 2 +- .../src/analyzers/revert-recurrence.ts | 2 +- .../src/analyzers/stale-branch.ts | 2 +- .../src/analyzers/undocumented-export.ts | 2 +- .../src/analyzers/unused-export.ts | 2 +- .../test/approval-integrity.test.ts | 9 + review-enrichment/test/blame-link.test.ts | 9 + review-enrichment/test/caller-impact.test.ts | 5 + review-enrichment/test/churn-hotspot.test.ts | 2 + review-enrichment/test/commit-hygiene.test.ts | 9 + review-enrichment/test/commit-lint.test.ts | 2 + .../test/commit-signature.test.ts | 4 +- .../test/complexity-delta.test.ts | 20 + review-enrichment/test/coverage-delta.test.ts | 2 + .../test/doc-comment-drift.test.ts | 14 + .../test/exhaustiveness-drift.test.ts | 298 +++++++-------- review-enrichment/test/flaky-test.test.ts | 342 +++++++++--------- .../test/pending-review-requests.test.ts | 9 + .../test/revert-recurrence.test.ts | 8 + review-enrichment/test/stale-branch.test.ts | 9 + .../test/undocumented-export.test.ts | 9 + review-enrichment/test/unused-export.test.ts | 286 ++++++++------- 34 files changed, 610 insertions(+), 461 deletions(-) diff --git a/review-enrichment/src/analyzers/approval-integrity.ts b/review-enrichment/src/analyzers/approval-integrity.ts index a95054e9f2..9c599fcb9c 100644 --- a/review-enrichment/src/analyzers/approval-integrity.ts +++ b/review-enrichment/src/analyzers/approval-integrity.ts @@ -17,7 +17,7 @@ import { boundedFetchJson } from "../external-fetch.js"; import { githubHeaders } from "../github-headers.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const REVIEWS_PER_PAGE = 100; // GitHub returns PR reviews oldest-first with no reorder option, so a single `per_page=100` fetch would silently // read only the OLDEST reviews on any PR with more — exactly backwards for "each reviewer's latest vote". Walk diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index a373513779..511b480908 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -16,7 +16,7 @@ import { githubHeaders } from "../github-headers.js"; import { isHistoryUninformativePath } from "./history-path.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_FILES_PROBED = 6; // bound the files we probe, matching the other history-class analyzers const MAX_LOOKUPS = 12; // hard cap on total GitHub round-trips (each file costs up to 2: commits + pulls) const SHA_PREFIX_LEN = 12; diff --git a/review-enrichment/src/analyzers/caller-impact.ts b/review-enrichment/src/analyzers/caller-impact.ts index 442cb1adc3..80d8182e4c 100644 --- a/review-enrichment/src/analyzers/caller-impact.ts +++ b/review-enrichment/src/analyzers/caller-impact.ts @@ -33,7 +33,7 @@ import { isTestPath } from "./test-ratio.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_SYMBOLS = 6; // removed symbols searched per PR (Code Search rate budget) const MAX_SEARCHES = 6; // bounded Code Search queries per PR const MAX_FILE_FETCHES = 12; // bounded candidate-caller content fetches per PR diff --git a/review-enrichment/src/analyzers/churn-hotspot.ts b/review-enrichment/src/analyzers/churn-hotspot.ts index 3f05fa535b..ba0ab214b9 100644 --- a/review-enrichment/src/analyzers/churn-hotspot.ts +++ b/review-enrichment/src/analyzers/churn-hotspot.ts @@ -15,7 +15,7 @@ import { githubHeaders } from "../github-headers.js"; import { isHistoryUninformativePath } from "./history-path.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const WINDOW_DAYS = 90; const PER_PAGE = 100; // one page; a file with a full page of commits in the window is already a clear hotspot const MAX_FILES_PROBED = 8; // bound the GitHub round-trips, matching the other history-class analyzers diff --git a/review-enrichment/src/analyzers/commit-hygiene.ts b/review-enrichment/src/analyzers/commit-hygiene.ts index a30eb7a361..10425282d7 100644 --- a/review-enrichment/src/analyzers/commit-hygiene.ts +++ b/review-enrichment/src/analyzers/commit-hygiene.ts @@ -20,7 +20,7 @@ import { githubHeaders } from "../github-headers.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_COMMITS = 100; const MAX_FINDINGS = DEFAULT_MAX_FINDINGS; const SHA_PREFIX_LEN = 12; diff --git a/review-enrichment/src/analyzers/commit-lint.ts b/review-enrichment/src/analyzers/commit-lint.ts index 19026a3cea..ed329fcd8c 100644 --- a/review-enrichment/src/analyzers/commit-lint.ts +++ b/review-enrichment/src/analyzers/commit-lint.ts @@ -16,7 +16,7 @@ import { githubHeaders } from "../github-headers.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_COMMITS = 100; const MAX_FINDINGS = DEFAULT_MAX_FINDINGS; const SHA_PREFIX_LEN = 12; diff --git a/review-enrichment/src/analyzers/commit-signature.ts b/review-enrichment/src/analyzers/commit-signature.ts index 861a57163f..227f4d7d99 100644 --- a/review-enrichment/src/analyzers/commit-signature.ts +++ b/review-enrichment/src/analyzers/commit-signature.ts @@ -19,7 +19,7 @@ const GITHUB_API = "https://api.github.com"; // analyzers cap their network round-trips. const HISTORY_PER_PAGE = 30; // Only repository slugs that look like real `owner/repo` segments are ever interpolated into a request URL. -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments interface ScanOptions { signal?: AbortSignal; diff --git a/review-enrichment/src/analyzers/complexity-delta.ts b/review-enrichment/src/analyzers/complexity-delta.ts index 3431e62766..b94fb9e2b7 100644 --- a/review-enrichment/src/analyzers/complexity-delta.ts +++ b/review-enrichment/src/analyzers/complexity-delta.ts @@ -36,7 +36,7 @@ import { isJsTsPath, scanContentForComplexity } from "./complexity.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_FILES = 20; const MAX_FINDINGS = DEFAULT_MAX_FINDINGS; const MAX_FETCH_BYTES = 1_000_000; diff --git a/review-enrichment/src/analyzers/coverage-delta.ts b/review-enrichment/src/analyzers/coverage-delta.ts index c577bd132b..4563c3588e 100644 --- a/review-enrichment/src/analyzers/coverage-delta.ts +++ b/review-enrichment/src/analyzers/coverage-delta.ts @@ -18,7 +18,7 @@ import { boundedFetchJson } from "../external-fetch.js"; import { githubHeaders } from "../github-headers.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_RUNS_PROBED = 5; // recent successful runs to search for a coverage artifact const MAX_ARTIFACT_BYTES = 8 * 1024 * 1024; // skip an artifact zip larger than this (bounded download) const MAX_ENTRY_BYTES = 4 * 1024 * 1024; // skip a single uncompressed zip entry larger than this diff --git a/review-enrichment/src/analyzers/doc-comment-drift.ts b/review-enrichment/src/analyzers/doc-comment-drift.ts index 59cc937075..257b7eb6b2 100644 --- a/review-enrichment/src/analyzers/doc-comment-drift.ts +++ b/review-enrichment/src/analyzers/doc-comment-drift.ts @@ -18,7 +18,7 @@ const MAX_SIGNATURE_LINES = 40; const MAX_FETCH_BYTES = 1_000_000; const SOURCE_RE = /\.(?:ts|tsx|mts|cts|js|jsx|mjs|cjs)$/; const SKIP_RE = /(?:\.d\.ts$|\.min\.|\.test\.|\.spec\.|__tests__\/|(?:^|\/)tests?\/)/; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments // Matches a named `function` declaration up to its parameter `(`. A single, non-nested generic clause is allowed; // a nested-generic declaration (e.g. `function f>(x)`) simply does not match and // the function is skipped — a deliberate recall/precision trade-off, never a false positive. diff --git a/review-enrichment/src/analyzers/exhaustiveness-drift.ts b/review-enrichment/src/analyzers/exhaustiveness-drift.ts index 26800ed311..c17fb2b89d 100644 --- a/review-enrichment/src/analyzers/exhaustiveness-drift.ts +++ b/review-enrichment/src/analyzers/exhaustiveness-drift.ts @@ -13,7 +13,7 @@ import { isTestPath } from "./test-ratio.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_FILES = 10; const MAX_FETCHES = 10; const MAX_FINDINGS = DEFAULT_MAX_FINDINGS; diff --git a/review-enrichment/src/analyzers/flaky-test.ts b/review-enrichment/src/analyzers/flaky-test.ts index c294f32ef0..1dc8d2ba93 100644 --- a/review-enrichment/src/analyzers/flaky-test.ts +++ b/review-enrichment/src/analyzers/flaky-test.ts @@ -15,7 +15,7 @@ import { isTestPath } from "./test-ratio.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const WINDOW_DAYS = 30; const WINDOW_LABEL = "30d"; const MAX_FILES_PROBED = 6; diff --git a/review-enrichment/src/analyzers/pending-review-requests.ts b/review-enrichment/src/analyzers/pending-review-requests.ts index c23b33a8d6..184cfdb5fb 100644 --- a/review-enrichment/src/analyzers/pending-review-requests.ts +++ b/review-enrichment/src/analyzers/pending-review-requests.ts @@ -18,7 +18,7 @@ import { boundedFetchJson } from "../external-fetch.js"; import { githubHeaders } from "../github-headers.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const TIMELINE_PER_PAGE = 100; const MAX_TIMELINE_PAGES = 5; const STALE_THRESHOLD_MS = 48 * 60 * 60 * 1000; // 48 hours diff --git a/review-enrichment/src/analyzers/revert-recurrence.ts b/review-enrichment/src/analyzers/revert-recurrence.ts index a4e2ec8f8c..80e62341a0 100644 --- a/review-enrichment/src/analyzers/revert-recurrence.ts +++ b/review-enrichment/src/analyzers/revert-recurrence.ts @@ -18,7 +18,7 @@ import { isHistoryUninformativePath } from "./history-path.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_FILES_PROBED = 5; // bound the per-file commit-history fan-out, matching the other history-class analyzers const COMMITS_PER_FILE = 15; // recent commits to inspect per probed file when looking for a revert const MAX_REVERT_LOOKUPS = 10; // global cap on revert-commit detail fetches across all probed files diff --git a/review-enrichment/src/analyzers/stale-branch.ts b/review-enrichment/src/analyzers/stale-branch.ts index 593360fcee..87a07162c8 100644 --- a/review-enrichment/src/analyzers/stale-branch.ts +++ b/review-enrichment/src/analyzers/stale-branch.ts @@ -16,7 +16,7 @@ import { boundedFetchJson } from "../external-fetch.js"; import { githubHeaders } from "../github-headers.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments // Below this many commits behind, drifting from the default branch is normal PR life, not a staleness risk. const BEHIND_THRESHOLD = 100; diff --git a/review-enrichment/src/analyzers/undocumented-export.ts b/review-enrichment/src/analyzers/undocumented-export.ts index 251ff3f81d..007d78f27d 100644 --- a/review-enrichment/src/analyzers/undocumented-export.ts +++ b/review-enrichment/src/analyzers/undocumented-export.ts @@ -16,7 +16,7 @@ const GITHUB_API = "https://api.github.com"; const MAX_FILES = 10; const MAX_FINDINGS = 30; const MAX_FETCH_BYTES = 1_000_000; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments // A public entrypoint barrel — an `index.` source file. Declaration (.d.ts), test, and generated output are // excluded: they are not the hand-authored public surface this scan is about. const ENTRYPOINT_RE = /(?:^|\/)index\.(?:ts|tsx|mts|cts|js|jsx|mjs|cjs)$/; diff --git a/review-enrichment/src/analyzers/unused-export.ts b/review-enrichment/src/analyzers/unused-export.ts index e32327fa99..62a81323a5 100644 --- a/review-enrichment/src/analyzers/unused-export.ts +++ b/review-enrichment/src/analyzers/unused-export.ts @@ -18,7 +18,7 @@ import { isTestPath } from "./test-ratio.js"; import { DEFAULT_MAX_FINDINGS } from "./limits.js"; const GITHUB_API = "https://api.github.com"; -const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const MAX_SYMBOLS = 10; const MAX_SEARCHES = 10; const MAX_FILE_FETCHES = 10; diff --git a/review-enrichment/test/approval-integrity.test.ts b/review-enrichment/test/approval-integrity.test.ts index 8a6ae46bd4..008590ff10 100644 --- a/review-enrichment/test/approval-integrity.test.ts +++ b/review-enrichment/test/approval-integrity.test.ts @@ -192,6 +192,15 @@ test("scanApprovalIntegrity: a malformed repoFullName is skipped, not thrown", a assert.deepEqual(findings, []); }); +test("scanApprovalIntegrity: a dot-segment owner (path-traversal shape) is rejected, not thrown", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + const findings = await scanApprovalIntegrity( + req({ repoFullName: "../evil" }), + reviewsFetch([review("alice", "APPROVED", "old-sha", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + test("scanApprovalIntegrity: a fetch failure yields no finding", async () => { const findings = await scanApprovalIntegrity(req(), async () => jsonResponse({ message: "bad" }, 500)); assert.deepEqual(findings, []); diff --git a/review-enrichment/test/blame-link.test.ts b/review-enrichment/test/blame-link.test.ts index 251f553aab..e03399284c 100644 --- a/review-enrichment/test/blame-link.test.ts +++ b/review-enrichment/test/blame-link.test.ts @@ -175,3 +175,12 @@ test("scanBlameLink: no GitHub token → skipped (no finding, no throw)", async ); assert.deepEqual(findings, []); }); + +test("scanBlameLink: a dot-segment owner (path-traversal shape) is rejected, not thrown", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + const findings = await scanBlameLink( + req([{ path: "src/app.ts", status: "modified", patch: modifyPatch(2) }], { repoFullName: "../evil" }), + routedFetch({ commitSha: "abcdef1234567890", prNumber: 1 }), + ); + assert.deepEqual(findings, []); +}); diff --git a/review-enrichment/test/caller-impact.test.ts b/review-enrichment/test/caller-impact.test.ts index bfb7b818d7..dddc50848f 100644 --- a/review-enrichment/test/caller-impact.test.ts +++ b/review-enrichment/test/caller-impact.test.ts @@ -360,6 +360,11 @@ test("scanCallerImpact: no token / no headSha / invalid slug / no removed export await scanCallerImpact(req(files, { repoFullName: "octo/re po" }), failFetch), [], ); + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + assert.deepEqual( + await scanCallerImpact(req(files, { repoFullName: "../evil" }), failFetch), + [], + ); assert.deepEqual( await scanCallerImpact( req([{ path: "src/utils.ts", patch: ["@@ -0,0 +1,1 @@", "+export function added() {}"].join("\n") }]), diff --git a/review-enrichment/test/churn-hotspot.test.ts b/review-enrichment/test/churn-hotspot.test.ts index b882208b92..b305ece437 100644 --- a/review-enrichment/test/churn-hotspot.test.ts +++ b/review-enrichment/test/churn-hotspot.test.ts @@ -93,6 +93,8 @@ test("scanChurnHotspot: skips lockfiles, binaries, and newly-added files without test("scanChurnHotspot: requires a github token and a valid repo slug", async () => { assert.deepEqual(await scanChurnHotspot({ repoFullName: "octo/repo", prNumber: 1, files: [{ path: "src/a.ts" }] }, async () => jsonResponse(commits(20, 2))), []); assert.deepEqual(await scanChurnHotspot({ repoFullName: "bad slug/x!", prNumber: 1, githubToken: "t", files: [{ path: "src/a.ts" }] }, async () => jsonResponse(commits(20, 2))), []); + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + assert.deepEqual(await scanChurnHotspot({ repoFullName: "../evil", prNumber: 1, githubToken: "t", files: [{ path: "src/a.ts" }] }, async () => jsonResponse(commits(20, 2))), []); }); test("scanChurnHotspot: rejects multi-segment repo slugs without fetching", async () => { diff --git a/review-enrichment/test/commit-hygiene.test.ts b/review-enrichment/test/commit-hygiene.test.ts index 7ac019782a..cf68325374 100644 --- a/review-enrichment/test/commit-hygiene.test.ts +++ b/review-enrichment/test/commit-hygiene.test.ts @@ -196,6 +196,15 @@ test("scanCommitHygiene: a malformed repoFullName is skipped, not thrown", async assert.deepEqual(findings, []); }); +test("scanCommitHygiene: a dot-segment owner (path-traversal shape) is rejected, not thrown", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + const findings = await scanCommitHygiene( + req({ repoFullName: "../evil" }), + commitsFetch([commit(SHA_A, "fixup! x")]), + ); + assert.deepEqual(findings, []); +}); + test("scanCommitHygiene: a fetch failure yields no finding", async () => { const findings = await scanCommitHygiene(req(), async () => jsonResponse({ message: "bad" }, 500)); assert.deepEqual(findings, []); diff --git a/review-enrichment/test/commit-lint.test.ts b/review-enrichment/test/commit-lint.test.ts index 3d4cd4a0b5..c47b981462 100644 --- a/review-enrichment/test/commit-lint.test.ts +++ b/review-enrichment/test/commit-lint.test.ts @@ -87,6 +87,8 @@ test("scanCommitLint: fail-safe — no token, a bad repo slug, or a fetch error assert.deepEqual(await scanCommitLint(req({ githubToken: undefined }), good), []); assert.deepEqual(await scanCommitLint(req({ repoFullName: "octo/repo/extra" }), good), []); assert.deepEqual(await scanCommitLint(req({ repoFullName: "bad slug!/x" }), good), []); + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + assert.deepEqual(await scanCommitLint(req({ repoFullName: "../evil" }), good), []); const err = async () => new Response("nope", { status: 500 }); assert.deepEqual(await scanCommitLint(req(), err), []); }); diff --git a/review-enrichment/test/commit-signature.test.ts b/review-enrichment/test/commit-signature.test.ts index c5491dab20..77dee18f67 100644 --- a/review-enrichment/test/commit-signature.test.ts +++ b/review-enrichment/test/commit-signature.test.ts @@ -147,7 +147,9 @@ test("scanCommitSignature fails closed on a malformed repo slug WITHOUT any netw // A spy that records invocation: a malformed slug must be rejected BEFORE any GitHub request, so the guard // can never query the wrong repository. (A throwing fetch would be swallowed by the analyzer's fail-safe // try/catch and could mask a slug that slipped through, so assert the call never happens instead.) - for (const repoFullName of ["not-a-slug", "o/r/extra", "/r", "o/", "a/b/c/d"]) { + // "../evil"/"o/.."/"o/." each individually satisfy a bare `[A-Za-z0-9._-]+` class (every char in ".."/"." is + // allowed); only a first-character requirement on owner AND repo independently catches them. + for (const repoFullName of ["not-a-slug", "o/r/extra", "/r", "o/", "a/b/c/d", "../evil", "o/..", "o/."]) { let called = false; const spyFetch: typeof fetch = async () => { called = true; diff --git a/review-enrichment/test/complexity-delta.test.ts b/review-enrichment/test/complexity-delta.test.ts index b80b8b011f..7ce6f3444f 100644 --- a/review-enrichment/test/complexity-delta.test.ts +++ b/review-enrichment/test/complexity-delta.test.ts @@ -129,6 +129,26 @@ test("scanComplexityDelta: rejects multi-segment repo slugs without fetching", a assert.equal(called, false); }); +test("scanComplexityDelta: rejects a dot-segment owner/repo slug without fetching", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + let called = false; + const out = await scanComplexityDelta( + { + repoFullName: "../evil", + prNumber: 1, + headSha: "abc123", + githubToken: "ght", + files: [{ path: "src/a.ts", patch: CALC_PATCH }], + }, + async () => { + called = true; + return fileWith(HEAD_CONTENT)(); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + test("scanComplexityDelta: skips non-source, test, and patch-less files without fetching", async () => { let called = false; const out = await scanComplexityDelta( diff --git a/review-enrichment/test/coverage-delta.test.ts b/review-enrichment/test/coverage-delta.test.ts index 7d815d46e1..b67944526f 100644 --- a/review-enrichment/test/coverage-delta.test.ts +++ b/review-enrichment/test/coverage-delta.test.ts @@ -207,6 +207,8 @@ test("scanCoverageDelta: requires a github token, a head sha, and a single valid assert.deepEqual(await scanCoverageDelta(req(files, { headSha: undefined }), call), []); assert.deepEqual(await scanCoverageDelta(req(files, { repoFullName: "octo/repo/extra" }), call), []); assert.deepEqual(await scanCoverageDelta(req(files, { repoFullName: "bad slug/x!" }), call), []); + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + assert.deepEqual(await scanCoverageDelta(req(files, { repoFullName: "../evil" }), call), []); }); test("scanCoverageDelta: a PR that adds no lines never touches the network", async () => { diff --git a/review-enrichment/test/doc-comment-drift.test.ts b/review-enrichment/test/doc-comment-drift.test.ts index 384cd9fc86..4eae3f2270 100644 --- a/review-enrichment/test/doc-comment-drift.test.ts +++ b/review-enrichment/test/doc-comment-drift.test.ts @@ -308,6 +308,20 @@ test("scanDocCommentDrift: rejects multi-segment repo slugs without fetching", a assert.equal(called, false); }); +test("scanDocCommentDrift: rejects a dot-segment owner/repo slug without fetching", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + let called = false; + const out = await scanDocCommentDrift( + { repoFullName: "../evil", prNumber: 1, headSha: "abc123", githubToken: "ght", files: [{ path: "src/a.ts", patch: DRIFT_PATCH }] }, + async () => { + called = true; + return fileWith(DRIFTED)(); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + test("scanDocCommentDrift: skips non-source and test files without fetching", async () => { let called = false; const out = await scanDocCommentDrift( diff --git a/review-enrichment/test/exhaustiveness-drift.test.ts b/review-enrichment/test/exhaustiveness-drift.test.ts index afdb0a9980..b2f812e6d6 100644 --- a/review-enrichment/test/exhaustiveness-drift.test.ts +++ b/review-enrichment/test/exhaustiveness-drift.test.ts @@ -1,62 +1,62 @@ -// Units for the exhaustiveness-drift analyzer (#2028). 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 { - parseAddedTypeMembers, - extractEnumMembers, - extractUnionMembers, - findExhaustivenessGap, - scanExhaustivenessDrift, -} from "../dist/analyzers/exhaustiveness-drift.js"; -import { renderBrief } from "../dist/render.js"; - -const req = (files, extra = {}) => ({ - repoFullName: "octo/repo", - prNumber: 1, - githubToken: "ghp_test", - headSha: "abc123", - files, - ...extra, -}); - -const HEAD_UNCOVERED = [ - "export enum Status {", - " Active,", - " Pending,", - " Archived,", - "}", - "", - "export function dispatch(status: Status) {", - " switch (status) {", - " case Status.Active:", - " case Status.Pending:", - " break;", - " }", - "}", -].join("\n"); - -const PATCH_ADD_ARCHIVED = [ - "@@ -1,4 +1,5 @@", - " export enum Status {", - " Active,", - " Pending,", - "+ Archived,", - " }", -].join("\n"); - -test("parseAddedTypeMembers: collects added enum members with line numbers", () => { - assert.deepEqual(parseAddedTypeMembers(PATCH_ADD_ARCHIVED), [ - { unionName: "Status", addedMember: "Archived", line: 4, kind: "enum" }, - ]); -}); - -test("findExhaustivenessGap: flags a switch that covered all old enum members but omits the new one", () => { - const oldMembers = new Set(["Active", "Pending"]); - const gap = findExhaustivenessGap(HEAD_UNCOVERED, "enum", "Status", oldMembers, "Archived"); - assert.deepEqual(gap, { line: 8 }); -}); - +// Units for the exhaustiveness-drift analyzer (#2028). 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 { + parseAddedTypeMembers, + extractEnumMembers, + extractUnionMembers, + findExhaustivenessGap, + scanExhaustivenessDrift, +} from "../dist/analyzers/exhaustiveness-drift.js"; +import { renderBrief } from "../dist/render.js"; + +const req = (files, extra = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_test", + headSha: "abc123", + files, + ...extra, +}); + +const HEAD_UNCOVERED = [ + "export enum Status {", + " Active,", + " Pending,", + " Archived,", + "}", + "", + "export function dispatch(status: Status) {", + " switch (status) {", + " case Status.Active:", + " case Status.Pending:", + " break;", + " }", + "}", +].join("\n"); + +const PATCH_ADD_ARCHIVED = [ + "@@ -1,4 +1,5 @@", + " export enum Status {", + " Active,", + " Pending,", + "+ Archived,", + " }", +].join("\n"); + +test("parseAddedTypeMembers: collects added enum members with line numbers", () => { + assert.deepEqual(parseAddedTypeMembers(PATCH_ADD_ARCHIVED), [ + { unionName: "Status", addedMember: "Archived", line: 4, kind: "enum" }, + ]); +}); + +test("findExhaustivenessGap: flags a switch that covered all old enum members but omits the new one", () => { + const oldMembers = new Set(["Active", "Pending"]); + const gap = findExhaustivenessGap(HEAD_UNCOVERED, "enum", "Status", oldMembers, "Archived"); + assert.deepEqual(gap, { line: 8 }); +}); + test("findExhaustivenessGap: does not flag when the switch already covers the new member", () => { const covered = HEAD_UNCOVERED.replace( " case Status.Pending:", @@ -78,80 +78,80 @@ test("findExhaustivenessGap: bounds malformed switch headers before EOF", () => }); test("extractUnionMembers: reads string-literal union members from a type alias", () => { - const src = 'export type Role = "admin" | "user";'; - assert.deepEqual([...extractUnionMembers(src, "Role")!], ["admin", "user"]); -}); - -test("scanExhaustivenessDrift: end-to-end flags an uncovered added enum member and renders it", async () => { - const fetchFn = async (url) => { - if (url.includes("/contents/")) return new Response(HEAD_UNCOVERED, { status: 200 }); - return new Response("", { status: 404 }); - }; - const findings = await scanExhaustivenessDrift( - req([{ path: "src/status.ts", status: "modified", patch: PATCH_ADD_ARCHIVED }]), - fetchFn, - ); - assert.deepEqual(findings, [ - { - file: "src/status.ts", - line: 4, - unionName: "Status", - addedMember: "Archived", - }, - ]); - const brief = renderBrief({ exhaustiveness: findings }).promptSection; - assert.match(brief, /exhaustiveness drift/i); - assert.match(brief, /Archived/); -}); - -test("scanExhaustivenessDrift: does not flag when the switch is updated in the same file", async () => { - const head = HEAD_UNCOVERED.replace( - " case Status.Pending:", - " case Status.Pending:\n case Status.Archived:", - ); - const patch = [ - "@@ -1,4 +1,5 @@", - " export enum Status {", - " Active,", - " Pending,", - "+ Archived,", - " }", - "@@ -10,3 +11,4 @@", - " case Status.Active:", - " case Status.Pending:", - "+ case Status.Archived:", - " break;", - ].join("\n"); - const fetchFn = async (url) => { - if (url.includes("/contents/")) return new Response(head, { status: 200 }); - return new Response("", { status: 404 }); - }; - const findings = await scanExhaustivenessDrift( - req([{ path: "src/status.ts", status: "modified", patch }]), - fetchFn, - ); - assert.deepEqual(findings, []); -}); - -test("scanExhaustivenessDrift: enforces the maxFetches cap", async () => { - const patch = ["@@ -0,0 +1,2 @@", "+export enum E {", "+ A,", "+}"].join("\n"); - const files = Array.from({ length: 12 }, (_, i) => ({ - path: `src/file${i}.ts`, - status: "added", - patch: patch.replace("E", `E${i}`).replace("A", `A${i}`), - })); - let fetches = 0; - const fetchFn = async (url) => { - if (url.includes("/contents/")) { - fetches += 1; - return new Response("export enum E0 { A0 }\n", { status: 200 }); - } - return new Response("", { status: 404 }); - }; - await scanExhaustivenessDrift(req(files), fetchFn); - assert.equal(fetches, 10); -}); - + const src = 'export type Role = "admin" | "user";'; + assert.deepEqual([...extractUnionMembers(src, "Role")!], ["admin", "user"]); +}); + +test("scanExhaustivenessDrift: end-to-end flags an uncovered added enum member and renders it", async () => { + const fetchFn = async (url) => { + if (url.includes("/contents/")) return new Response(HEAD_UNCOVERED, { status: 200 }); + return new Response("", { status: 404 }); + }; + const findings = await scanExhaustivenessDrift( + req([{ path: "src/status.ts", status: "modified", patch: PATCH_ADD_ARCHIVED }]), + fetchFn, + ); + assert.deepEqual(findings, [ + { + file: "src/status.ts", + line: 4, + unionName: "Status", + addedMember: "Archived", + }, + ]); + const brief = renderBrief({ exhaustiveness: findings }).promptSection; + assert.match(brief, /exhaustiveness drift/i); + assert.match(brief, /Archived/); +}); + +test("scanExhaustivenessDrift: does not flag when the switch is updated in the same file", async () => { + const head = HEAD_UNCOVERED.replace( + " case Status.Pending:", + " case Status.Pending:\n case Status.Archived:", + ); + const patch = [ + "@@ -1,4 +1,5 @@", + " export enum Status {", + " Active,", + " Pending,", + "+ Archived,", + " }", + "@@ -10,3 +11,4 @@", + " case Status.Active:", + " case Status.Pending:", + "+ case Status.Archived:", + " break;", + ].join("\n"); + const fetchFn = async (url) => { + if (url.includes("/contents/")) return new Response(head, { status: 200 }); + return new Response("", { status: 404 }); + }; + const findings = await scanExhaustivenessDrift( + req([{ path: "src/status.ts", status: "modified", patch }]), + fetchFn, + ); + assert.deepEqual(findings, []); +}); + +test("scanExhaustivenessDrift: enforces the maxFetches cap", async () => { + const patch = ["@@ -0,0 +1,2 @@", "+export enum E {", "+ A,", "+}"].join("\n"); + const files = Array.from({ length: 12 }, (_, i) => ({ + path: `src/file${i}.ts`, + status: "added", + patch: patch.replace("E", `E${i}`).replace("A", `A${i}`), + })); + let fetches = 0; + const fetchFn = async (url) => { + if (url.includes("/contents/")) { + fetches += 1; + return new Response("export enum E0 { A0 }\n", { status: 200 }); + } + return new Response("", { status: 404 }); + }; + await scanExhaustivenessDrift(req(files), fetchFn); + assert.equal(fetches, 10); +}); + test("scanExhaustivenessDrift: uses the analysis-context fetchText when supplied, instead of the bare fetch path", async () => { // #4759: the file-content fetch now goes through the shared boundedFetchText helper, which prefers // options.analysis.fetchText (mirrors duplication-delta.ts's own fetchFileAtHead) when an AnalysisContext is @@ -183,12 +183,26 @@ test("scanExhaustivenessDrift: uses the analysis-context fetchText when supplied ]); }); -test("scanExhaustivenessDrift: returns no findings without a GitHub token", async () => { - const findings = await scanExhaustivenessDrift( - req([{ path: "src/status.ts", status: "modified", patch: PATCH_ADD_ARCHIVED }], { - githubToken: undefined, - }), - async () => new Response("", { status: 500 }), - ); - assert.deepEqual(findings, []); -}); +test("scanExhaustivenessDrift: rejects a dot-segment owner/repo slug without fetching", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + let called = false; + const out = await scanExhaustivenessDrift( + req([{ path: "src/status.ts", status: "modified", patch: PATCH_ADD_ARCHIVED }], { repoFullName: "../evil" }), + async () => { + called = true; + return new Response(HEAD_UNCOVERED, { status: 200 }); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanExhaustivenessDrift: returns no findings without a GitHub token", async () => { + const findings = await scanExhaustivenessDrift( + req([{ path: "src/status.ts", status: "modified", patch: PATCH_ADD_ARCHIVED }], { + githubToken: undefined, + }), + async () => new Response("", { status: 500 }), + ); + assert.deepEqual(findings, []); +}); diff --git a/review-enrichment/test/flaky-test.test.ts b/review-enrichment/test/flaky-test.test.ts index d871d9faaa..a3e74abd28 100644 --- a/review-enrichment/test/flaky-test.test.ts +++ b/review-enrichment/test/flaky-test.test.ts @@ -1,164 +1,178 @@ -// Units for the flaky-test history annotator (#2033). 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 { - isTestCheckName, - referencesTestFile, - countCommitTestFailures, - scanFlakyTest, -} from "../dist/analyzers/flaky-test.js"; -import { renderBrief } from "../dist/render.js"; - -const jsonResponse = (body, code = 200) => - new Response(JSON.stringify(body), { status: code }); - -const req = (files, extra = {}) => ({ - repoFullName: "octo/repo", - prNumber: 1, - githubToken: "ghp_test", - files, - ...extra, -}); - -test("isTestCheckName: matches common test job names", () => { - assert.equal(isTestCheckName("validate-code"), true); - assert.equal(isTestCheckName("jest / unit"), true); - assert.equal(isTestCheckName("build"), false); -}); - -test("referencesTestFile: matches full path, basename, or stem in structured output", () => { - assert.equal( - referencesTestFile({ summary: "FAIL src/foo/bar.test.ts" }, "src/foo/bar.test.ts"), - true, - ); - assert.equal(referencesTestFile({ title: "bar.test.ts failed" }, "src/foo/bar.test.ts"), true); - assert.equal(referencesTestFile({ summary: "unrelated failure" }, "src/foo/bar.test.ts"), false); -}); - -test("countCommitTestFailures: counts only failed test checks referencing the file", () => { - const runs = [ - { - name: "validate-code", - status: "completed", - conclusion: "failure", - output: { summary: "FAIL src/app.test.ts" }, - }, - { - name: "build", - status: "completed", - conclusion: "failure", - output: { summary: "src/app.test.ts compile error" }, - }, - { - name: "jest", - status: "completed", - conclusion: "success", - output: { summary: "src/app.test.ts" }, - }, - ]; - assert.equal(countCommitTestFailures(runs, "src/app.test.ts"), 1); -}); - -test("scanFlakyTest: flags a changed test file with repeated recent CI failures", async () => { - const fetchFn = async (url) => { - if (url.includes("/repos/octo/repo") && !url.includes("/commits")) { - return jsonResponse({ default_branch: "main" }); - } - if (url.includes("path=src%2Fapp.test.ts") && !url.includes("/check-runs")) { - return jsonResponse([{ sha: "aaa" }, { sha: "bbb" }, { sha: "ccc" }]); - } - if (url.includes("/check-runs")) { - return jsonResponse({ - check_runs: [ - { - name: "validate-code", - status: "completed", - conclusion: "failure", - output: { summary: "FAIL src/app.test.ts" }, - }, - ], - }); - } - return new Response("", { status: 404 }); - }; - const findings = await scanFlakyTest( - req([{ path: "src/app.test.ts", status: "modified" }]), - fetchFn, - ); - assert.deepEqual(findings, [{ file: "src/app.test.ts", recentFailures: 3, window: "30d" }]); - const brief = renderBrief({ flakyTest: findings }).promptSection; - assert.match(brief, /Flaky-test history/i); -}); - -test("scanFlakyTest: does not flag when recent test checks are clean", async () => { - const fetchFn = async (url) => { - if (url.includes("/repos/octo/repo") && !url.includes("/commits")) { - return jsonResponse({ default_branch: "main" }); - } - if (url.includes("path=src%2Fclean.test.ts")) { - return jsonResponse([{ sha: "aaa" }, { sha: "bbb" }]); - } - if (url.includes("/check-runs")) { - return jsonResponse({ - check_runs: [ - { - name: "validate-code", - status: "completed", - conclusion: "success", - output: { summary: "ok src/clean.test.ts" }, - }, - ], - }); - } - return new Response("", { status: 404 }); - }; - const findings = await scanFlakyTest( - req([{ path: "src/clean.test.ts", status: "modified" }]), - fetchFn, - ); - assert.deepEqual(findings, []); -}); - -test("scanFlakyTest: marks partial status when the check-run probe cap is hit", async () => { - const files = Array.from({ length: 8 }, (_, i) => ({ - path: `src/t${i}.test.ts`, - status: "modified", - })); - let checkRunCalls = 0; - const diagnostics = {}; - const fetchFn = async (url) => { - if (url.includes("/repos/octo/repo") && !url.includes("/commits")) { - return jsonResponse({ default_branch: "main" }); - } - if (url.includes("/commits?") && url.includes("path=")) { - return jsonResponse([{ sha: "aaa" }, { sha: "bbb" }, { sha: "ccc" }]); - } - if (url.includes("/check-runs")) { - checkRunCalls += 1; - return jsonResponse({ - check_runs: [ - { - name: "validate-code", - status: "completed", - conclusion: "failure", - output: { summary: "FAIL src/t0.test.ts" }, - }, - ], - }); - } - return new Response("", { status: 404 }); - }; - await scanFlakyTest(req(files), fetchFn, { diagnostics }); - assert.equal(checkRunCalls, 12); - assert.equal(diagnostics.partialStatus, "partial"); - assert.equal(diagnostics.partialReason, "flaky_test_probe_cap"); -}); - -test("scanFlakyTest: returns no findings without a GitHub token", async () => { - const findings = await scanFlakyTest( - req([{ path: "src/app.test.ts", status: "modified" }], { githubToken: undefined }), - async () => jsonResponse({}), - ); - assert.deepEqual(findings, []); -}); +// Units for the flaky-test history annotator (#2033). 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 { + isTestCheckName, + referencesTestFile, + countCommitTestFailures, + scanFlakyTest, +} from "../dist/analyzers/flaky-test.js"; +import { renderBrief } from "../dist/render.js"; + +const jsonResponse = (body, code = 200) => + new Response(JSON.stringify(body), { status: code }); + +const req = (files, extra = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_test", + files, + ...extra, +}); + +test("isTestCheckName: matches common test job names", () => { + assert.equal(isTestCheckName("validate-code"), true); + assert.equal(isTestCheckName("jest / unit"), true); + assert.equal(isTestCheckName("build"), false); +}); + +test("referencesTestFile: matches full path, basename, or stem in structured output", () => { + assert.equal( + referencesTestFile({ summary: "FAIL src/foo/bar.test.ts" }, "src/foo/bar.test.ts"), + true, + ); + assert.equal(referencesTestFile({ title: "bar.test.ts failed" }, "src/foo/bar.test.ts"), true); + assert.equal(referencesTestFile({ summary: "unrelated failure" }, "src/foo/bar.test.ts"), false); +}); + +test("countCommitTestFailures: counts only failed test checks referencing the file", () => { + const runs = [ + { + name: "validate-code", + status: "completed", + conclusion: "failure", + output: { summary: "FAIL src/app.test.ts" }, + }, + { + name: "build", + status: "completed", + conclusion: "failure", + output: { summary: "src/app.test.ts compile error" }, + }, + { + name: "jest", + status: "completed", + conclusion: "success", + output: { summary: "src/app.test.ts" }, + }, + ]; + assert.equal(countCommitTestFailures(runs, "src/app.test.ts"), 1); +}); + +test("scanFlakyTest: flags a changed test file with repeated recent CI failures", async () => { + const fetchFn = async (url) => { + if (url.includes("/repos/octo/repo") && !url.includes("/commits")) { + return jsonResponse({ default_branch: "main" }); + } + if (url.includes("path=src%2Fapp.test.ts") && !url.includes("/check-runs")) { + return jsonResponse([{ sha: "aaa" }, { sha: "bbb" }, { sha: "ccc" }]); + } + if (url.includes("/check-runs")) { + return jsonResponse({ + check_runs: [ + { + name: "validate-code", + status: "completed", + conclusion: "failure", + output: { summary: "FAIL src/app.test.ts" }, + }, + ], + }); + } + return new Response("", { status: 404 }); + }; + const findings = await scanFlakyTest( + req([{ path: "src/app.test.ts", status: "modified" }]), + fetchFn, + ); + assert.deepEqual(findings, [{ file: "src/app.test.ts", recentFailures: 3, window: "30d" }]); + const brief = renderBrief({ flakyTest: findings }).promptSection; + assert.match(brief, /Flaky-test history/i); +}); + +test("scanFlakyTest: does not flag when recent test checks are clean", async () => { + const fetchFn = async (url) => { + if (url.includes("/repos/octo/repo") && !url.includes("/commits")) { + return jsonResponse({ default_branch: "main" }); + } + if (url.includes("path=src%2Fclean.test.ts")) { + return jsonResponse([{ sha: "aaa" }, { sha: "bbb" }]); + } + if (url.includes("/check-runs")) { + return jsonResponse({ + check_runs: [ + { + name: "validate-code", + status: "completed", + conclusion: "success", + output: { summary: "ok src/clean.test.ts" }, + }, + ], + }); + } + return new Response("", { status: 404 }); + }; + const findings = await scanFlakyTest( + req([{ path: "src/clean.test.ts", status: "modified" }]), + fetchFn, + ); + assert.deepEqual(findings, []); +}); + +test("scanFlakyTest: marks partial status when the check-run probe cap is hit", async () => { + const files = Array.from({ length: 8 }, (_, i) => ({ + path: `src/t${i}.test.ts`, + status: "modified", + })); + let checkRunCalls = 0; + const diagnostics = {}; + const fetchFn = async (url) => { + if (url.includes("/repos/octo/repo") && !url.includes("/commits")) { + return jsonResponse({ default_branch: "main" }); + } + if (url.includes("/commits?") && url.includes("path=")) { + return jsonResponse([{ sha: "aaa" }, { sha: "bbb" }, { sha: "ccc" }]); + } + if (url.includes("/check-runs")) { + checkRunCalls += 1; + return jsonResponse({ + check_runs: [ + { + name: "validate-code", + status: "completed", + conclusion: "failure", + output: { summary: "FAIL src/t0.test.ts" }, + }, + ], + }); + } + return new Response("", { status: 404 }); + }; + await scanFlakyTest(req(files), fetchFn, { diagnostics }); + assert.equal(checkRunCalls, 12); + assert.equal(diagnostics.partialStatus, "partial"); + assert.equal(diagnostics.partialReason, "flaky_test_probe_cap"); +}); + +test("scanFlakyTest: rejects a dot-segment owner/repo slug without fetching", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + let called = false; + const out = await scanFlakyTest( + req([{ path: "src/app.test.ts", status: "modified" }], { repoFullName: "../evil" }), + async () => { + called = true; + return jsonResponse({ default_branch: "main" }); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanFlakyTest: returns no findings without a GitHub token", async () => { + const findings = await scanFlakyTest( + req([{ path: "src/app.test.ts", status: "modified" }], { githubToken: undefined }), + async () => jsonResponse({}), + ); + assert.deepEqual(findings, []); +}); diff --git a/review-enrichment/test/pending-review-requests.test.ts b/review-enrichment/test/pending-review-requests.test.ts index 50ea997ed5..17865799d0 100644 --- a/review-enrichment/test/pending-review-requests.test.ts +++ b/review-enrichment/test/pending-review-requests.test.ts @@ -178,6 +178,15 @@ test("scanPendingReviewRequests: a malformed repoFullName is skipped, not thrown assert.deepEqual(findings, []); }); +test("scanPendingReviewRequests: a dot-segment owner (path-traversal shape) is rejected, not thrown", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + const findings = await scanPendingReviewRequests( + req({ repoFullName: "../evil" }), + async () => jsonResponse({}), + ); + assert.deepEqual(findings, []); +}); + test("scanPendingReviewRequests: the requested-reviewers fetch failing yields no finding", async () => { const findings = await scanPendingReviewRequests(req(), async () => jsonResponse({ message: "bad" }, 500)); assert.deepEqual(findings, []); diff --git a/review-enrichment/test/revert-recurrence.test.ts b/review-enrichment/test/revert-recurrence.test.ts index c493316512..77510ccc05 100644 --- a/review-enrichment/test/revert-recurrence.test.ts +++ b/review-enrichment/test/revert-recurrence.test.ts @@ -190,6 +190,14 @@ test("scanRevertRecurrence: requires a github token and a single valid repo slug ), [], ); + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + assert.deepEqual( + await scanRevertRecurrence( + { repoFullName: "../evil", prNumber: 1, githubToken: "t", files: [{ path: "src/a.ts", patch: PR_PATCH }] }, + routed(OVERLAP_LIST, OVERLAP_DETAIL), + ), + [], + ); }); test("scanRevertRecurrence: rejects multi-segment repo slugs without fetching", async () => { diff --git a/review-enrichment/test/stale-branch.test.ts b/review-enrichment/test/stale-branch.test.ts index 5f8c13b8a3..c33c509df1 100644 --- a/review-enrichment/test/stale-branch.test.ts +++ b/review-enrichment/test/stale-branch.test.ts @@ -104,6 +104,15 @@ test("scanStaleBranch: a malformed repoFullName is skipped, not thrown", async ( assert.deepEqual(findings, []); }); +test("scanStaleBranch: a dot-segment owner (path-traversal shape) is rejected, not thrown", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + const findings = await scanStaleBranch( + req({ repoFullName: "../evil" }), + routedFetch({ defaultBranch: "main", behindBy: 150, status: "behind" }), + ); + assert.deepEqual(findings, []); +}); + test("scanStaleBranch: the repo-info fetch failing yields no finding (and never calls compare)", async () => { let compareCalled = false; const findings = await scanStaleBranch(req(), async (url) => { diff --git a/review-enrichment/test/undocumented-export.test.ts b/review-enrichment/test/undocumented-export.test.ts index eaf82e3032..2a5b666a3f 100644 --- a/review-enrichment/test/undocumented-export.test.ts +++ b/review-enrichment/test/undocumented-export.test.ts @@ -203,6 +203,15 @@ test("scanUndocumentedExport: a rejected fetch or a non-OK (404) response yields assert.deepEqual(await scanUndocumentedExport(req(files), notOk), []); // resp.ok false → content skipped }); +test("scanUndocumentedExport: a dot-segment owner/repo slug is rejected, not thrown", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + const findings = await scanUndocumentedExport( + req([{ path: "src/index.ts", status: "modified", patch: PATCH }], { repoFullName: "../evil" }), + headFetch(HEAD), + ); + assert.deepEqual(findings, []); +}); + test("scanUndocumentedExport: no token or no headSha → skipped (no finding, no throw)", async () => { const files = [{ path: "src/index.ts", status: "modified", patch: PATCH }]; assert.deepEqual(await scanUndocumentedExport(req(files, { githubToken: undefined }), headFetch(HEAD)), []); diff --git a/review-enrichment/test/unused-export.test.ts b/review-enrichment/test/unused-export.test.ts index 573fa3a7e5..91d7b1f311 100644 --- a/review-enrichment/test/unused-export.test.ts +++ b/review-enrichment/test/unused-export.test.ts @@ -1,108 +1,108 @@ -// Units for the unused-export analyzer (#2025). 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 { - isDeadOnArrivalFromSearch, - referencesSymbolInSource, - scanUnusedExport, -} from "../dist/analyzers/unused-export.js"; -import { renderBrief } from "../dist/render.js"; - -const searchJson = (total, items, incomplete = false) => - JSON.stringify({ total_count: total, incomplete_results: incomplete, items }); - -const req = (files, extra = {}) => ({ - repoFullName: "octo/repo", - prNumber: 1, - githubToken: "ghp_test", - headSha: "abc123", - files, - ...extra, -}); - -test("isDeadOnArrivalFromSearch: zero indexed hits is dead; external or multiple hits are alive", () => { - assert.equal(isDeadOnArrivalFromSearch("src/util.ts", { total_count: 0, items: [] }), true); - assert.equal( - isDeadOnArrivalFromSearch("src/util.ts", { - total_count: 1, - items: [{ path: "src/util.ts" }], - }), - true, - ); - assert.equal( - isDeadOnArrivalFromSearch("src/util.ts", { - total_count: 2, - items: [{ path: "src/util.ts" }, { path: "src/app.ts" }], - }), - false, - ); - assert.equal( - isDeadOnArrivalFromSearch("src/util.ts", { total_count: 1, incomplete_results: true, items: [] }), - null, - ); -}); - -test("referencesSymbolInSource: ignores the declaration line but catches same-file uses", () => { - const src = ["export function helper() {}", "helper();", "export const other = 1;"].join("\n"); - assert.equal(referencesSymbolInSource(src, "helper", 1), true); - assert.equal(referencesSymbolInSource(src, "other", 3), false); -}); - -test("scanUnusedExport: flags a newly added export absent from the default-branch index", async () => { - const patch = ["@@ -0,0 +1,1 @@", "+export function orphanHelper() {}"].join("\n"); - const head = "export function orphanHelper() {}"; - const fetchFn = async (url) => { - if (url.includes("/contents/")) return new Response(head, { status: 200 }); - if (url.includes("/search/code")) { - return new Response(searchJson(0, []), { status: 200 }); - } - return new Response("", { status: 404 }); - }; - const findings = await scanUnusedExport( - req([{ path: "src/util.ts", status: "added", patch }]), - fetchFn, - ); - assert.deepEqual(findings, [{ file: "src/util.ts", line: 1, symbol: "orphanHelper" }]); - const brief = renderBrief({ unusedExport: findings }).promptSection; - assert.match(brief, /Unused exports/i); - assert.match(brief, /orphanHelper/); -}); - -test("scanUnusedExport: does not flag when search finds a reference in another file", async () => { - const patch = ["@@ -0,0 +1,1 @@", "+export const shared = 1;"].join("\n"); - const fetchFn = async (url) => { - if (url.includes("/contents/")) return new Response("export const shared = 1;", { status: 200 }); - if (url.includes("/search/code")) { - return new Response( - searchJson(2, [{ path: "src/util.ts" }, { path: "src/app.ts" }]), - { status: 200 }, - ); - } - return new Response("", { status: 404 }); - }; - const findings = await scanUnusedExport( - req([{ path: "src/util.ts", status: "added", patch }]), - fetchFn, - ); - assert.deepEqual(findings, []); -}); - -test("scanUnusedExport: does not flag when the head file uses the export locally", async () => { - const patch = ["@@ -0,0 +1,2 @@", "+export function helper() {}", "+helper();"].join("\n"); - const head = "export function helper() {}\nhelper();"; - const fetchFn = async (url) => { - if (url.includes("/contents/")) return new Response(head, { status: 200 }); - if (url.includes("/search/code")) return new Response(searchJson(0, []), { status: 200 }); - return new Response("", { status: 404 }); - }; - const findings = await scanUnusedExport( - req([{ path: "src/util.ts", status: "added", patch }]), - fetchFn, - ); - assert.deepEqual(findings, []); -}); - +// Units for the unused-export analyzer (#2025). 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 { + isDeadOnArrivalFromSearch, + referencesSymbolInSource, + scanUnusedExport, +} from "../dist/analyzers/unused-export.js"; +import { renderBrief } from "../dist/render.js"; + +const searchJson = (total, items, incomplete = false) => + JSON.stringify({ total_count: total, incomplete_results: incomplete, items }); + +const req = (files, extra = {}) => ({ + repoFullName: "octo/repo", + prNumber: 1, + githubToken: "ghp_test", + headSha: "abc123", + files, + ...extra, +}); + +test("isDeadOnArrivalFromSearch: zero indexed hits is dead; external or multiple hits are alive", () => { + assert.equal(isDeadOnArrivalFromSearch("src/util.ts", { total_count: 0, items: [] }), true); + assert.equal( + isDeadOnArrivalFromSearch("src/util.ts", { + total_count: 1, + items: [{ path: "src/util.ts" }], + }), + true, + ); + assert.equal( + isDeadOnArrivalFromSearch("src/util.ts", { + total_count: 2, + items: [{ path: "src/util.ts" }, { path: "src/app.ts" }], + }), + false, + ); + assert.equal( + isDeadOnArrivalFromSearch("src/util.ts", { total_count: 1, incomplete_results: true, items: [] }), + null, + ); +}); + +test("referencesSymbolInSource: ignores the declaration line but catches same-file uses", () => { + const src = ["export function helper() {}", "helper();", "export const other = 1;"].join("\n"); + assert.equal(referencesSymbolInSource(src, "helper", 1), true); + assert.equal(referencesSymbolInSource(src, "other", 3), false); +}); + +test("scanUnusedExport: flags a newly added export absent from the default-branch index", async () => { + const patch = ["@@ -0,0 +1,1 @@", "+export function orphanHelper() {}"].join("\n"); + const head = "export function orphanHelper() {}"; + const fetchFn = async (url) => { + if (url.includes("/contents/")) return new Response(head, { status: 200 }); + if (url.includes("/search/code")) { + return new Response(searchJson(0, []), { status: 200 }); + } + return new Response("", { status: 404 }); + }; + const findings = await scanUnusedExport( + req([{ path: "src/util.ts", status: "added", patch }]), + fetchFn, + ); + assert.deepEqual(findings, [{ file: "src/util.ts", line: 1, symbol: "orphanHelper" }]); + const brief = renderBrief({ unusedExport: findings }).promptSection; + assert.match(brief, /Unused exports/i); + assert.match(brief, /orphanHelper/); +}); + +test("scanUnusedExport: does not flag when search finds a reference in another file", async () => { + const patch = ["@@ -0,0 +1,1 @@", "+export const shared = 1;"].join("\n"); + const fetchFn = async (url) => { + if (url.includes("/contents/")) return new Response("export const shared = 1;", { status: 200 }); + if (url.includes("/search/code")) { + return new Response( + searchJson(2, [{ path: "src/util.ts" }, { path: "src/app.ts" }]), + { status: 200 }, + ); + } + return new Response("", { status: 404 }); + }; + const findings = await scanUnusedExport( + req([{ path: "src/util.ts", status: "added", patch }]), + fetchFn, + ); + assert.deepEqual(findings, []); +}); + +test("scanUnusedExport: does not flag when the head file uses the export locally", async () => { + const patch = ["@@ -0,0 +1,2 @@", "+export function helper() {}", "+helper();"].join("\n"); + const head = "export function helper() {}\nhelper();"; + const fetchFn = async (url) => { + if (url.includes("/contents/")) return new Response(head, { status: 200 }); + if (url.includes("/search/code")) return new Response(searchJson(0, []), { status: 200 }); + return new Response("", { status: 404 }); + }; + const findings = await scanUnusedExport( + req([{ path: "src/util.ts", status: "added", patch }]), + fetchFn, + ); + assert.deepEqual(findings, []); +}); + test("scanUnusedExport: uses the analysis-context fetchText for file content when supplied, instead of the bare fetch path", async () => { // #4824: the file-content fetch now goes through the shared boundedFetchText helper, which prefers // options.analysis.fetchText (mirrors duplication-delta.ts's own fetchFileAtHead) when an AnalysisContext is @@ -132,35 +132,47 @@ test("scanUnusedExport: uses the analysis-context fetchText for file content whe assert.deepEqual(findings, []); }); -test("scanUnusedExport: enforces the maxSearches cap", async () => { - const patch = ["@@ -0,0 +1,1 @@", "+export function fn() {}"].join("\n"); - const files = Array.from({ length: 12 }, (_, i) => ({ - path: `src/file${i}.ts`, - status: "added", - patch: patch.replace("fn", `fn${i}`), - })); - let searches = 0; - const fetchFn = async (url) => { - if (url.includes("/contents/")) { - const match = /file(\d+)\.ts/.exec(url); - const idx = match ? match[1] : "0"; - return new Response(`export function fn${idx}() {}`, { status: 200 }); - } - if (url.includes("/search/code")) { - searches += 1; - return new Response(searchJson(0, []), { status: 200 }); - } - return new Response("", { status: 404 }); - }; - await scanUnusedExport(req(files), fetchFn); - assert.equal(searches, 10); -}); - -test("scanUnusedExport: returns no findings without a GitHub token", async () => { - const patch = ["@@ -0,0 +1,1 @@", "+export function lonely() {}"].join("\n"); - const findings = await scanUnusedExport( - req([{ path: "src/util.ts", status: "added", patch }], { githubToken: undefined }), - async () => new Response("", { status: 500 }), - ); - assert.deepEqual(findings, []); -}); +test("scanUnusedExport: enforces the maxSearches cap", async () => { + const patch = ["@@ -0,0 +1,1 @@", "+export function fn() {}"].join("\n"); + const files = Array.from({ length: 12 }, (_, i) => ({ + path: `src/file${i}.ts`, + status: "added", + patch: patch.replace("fn", `fn${i}`), + })); + let searches = 0; + const fetchFn = async (url) => { + if (url.includes("/contents/")) { + const match = /file(\d+)\.ts/.exec(url); + const idx = match ? match[1] : "0"; + return new Response(`export function fn${idx}() {}`, { status: 200 }); + } + if (url.includes("/search/code")) { + searches += 1; + return new Response(searchJson(0, []), { status: 200 }); + } + return new Response("", { status: 404 }); + }; + await scanUnusedExport(req(files), fetchFn); + assert.equal(searches, 10); +}); + +test("scanUnusedExport: a dot-segment owner/repo slug is rejected, not thrown", async () => { + // ".." individually satisfies a bare `[A-Za-z0-9._-]+` class; only a first-character requirement catches it. + const patch = ["@@ -0,0 +1,1 @@", "+export function orphanHelper() {}"].join("\n"); + const findings = await scanUnusedExport( + req([{ path: "src/util.ts", status: "added", patch }], { repoFullName: "../evil" }), + async () => { + throw new Error("should not fetch"); + }, + ); + assert.deepEqual(findings, []); +}); + +test("scanUnusedExport: returns no findings without a GitHub token", async () => { + const patch = ["@@ -0,0 +1,1 @@", "+export function lonely() {}"].join("\n"); + const findings = await scanUnusedExport( + req([{ path: "src/util.ts", status: "added", patch }], { githubToken: undefined }), + async () => new Response("", { status: 500 }), + ); + assert.deepEqual(findings, []); +});