diff --git a/.env.example b/.env.example index 843edbcd52..a2cc364cb7 100644 --- a/.env.example +++ b/.env.example @@ -54,15 +54,29 @@ GITTENSORY_REVIEW_ENRICHMENT=false # REES_URL=https://enrichment.example.internal # REES_SHARED_SECRET= # bearer secret configured on the REES service # REES_TIMEOUT_MS=8000 # optional; minimum 1000, default 8000 +# REES_PROFILE=balanced # optional; fast | balanced | deep. Unset uses balanced. # REES_FORWARD_GITHUB_TOKEN=false # optional; default false. Set true only when REES_URL is inside # # your trust boundary and token-aware analyzers need a GitHub # # token for CODEOWNERS/blob-size reads (installation token when # # available; otherwise GITHUB_PUBLIC_TOKEN). -# REES_ANALYZERS=all # all | comma-list of exact names: -# # dependency,lockfileDrift,secret,license,installScript, -# # actionPin,eol,redos,provenance,codeowners,secretLog, -# # assetWeight,typosquat +# REES_ANALYZERS=all # all | comma-list of exact names. # # Unknown names warn and are ignored; a typo-only list runs no analyzers. +# BEGIN GENERATED REES ANALYZERS +# Current analyzer names: +# dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos +# provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild +# history,docCommentDrift +# +# Profile defaults: +# fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol +# redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild +# balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency +# actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature +# iacMisconfig,nativeBuild,history,docCommentDrift +# deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol +# redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig +# nativeBuild,history,docCommentDrift +# END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep # submitters to a deterministic-only review. Never surfaced publicly. diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index aaf4fd4e9e..a73ff5eec9 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -1,153 +1,510 @@ +// Generated by review-enrichment/scripts/generate-analyzer-metadata.mjs. +// Do not edit by hand; update review-enrichment analyzer descriptors instead. + +export type ReesProfileName = "fast" | "balanced" | "deep"; + export type ReesAnalyzerDoc = { name: string; title: string; - summary: string; - looksAt: string; - reports: string; - network: string; - notes: string; + category: string; + cost: string; + defaultEnabled: boolean; + profiles: readonly ReesProfileName[]; + requires: readonly string[]; + limits: Readonly>; + docs: { + summary: string; + looksAt: string; + reports: string; + network: string; + notes: string; + }; +}; + +export type ReesProfileDoc = { + name: ReesProfileName; + default: boolean; + costClasses: readonly string[]; + concurrency: Readonly>; + timeoutMs: Readonly>; + responseReserveMs: number; }; -export const REES_ANALYZERS: ReesAnalyzerDoc[] = [ +export const REES_DEFAULT_PROFILE = "balanced" as const; + +export const REES_PROFILES = [ + { + name: "fast", + default: false, + costClasses: ["local", "registry"], + concurrency: { + local: 8, + registry: 2, + "github-light": 0, + "github-heavy": 0, + tooling: 0, + }, + timeoutMs: { + local: 400, + registry: 800, + "github-light": 0, + "github-heavy": 0, + tooling: 0, + }, + responseReserveMs: 500, + }, + { + name: "balanced", + default: true, + costClasses: ["local", "registry", "github-light", "github-heavy", "tooling"], + concurrency: { + local: 8, + registry: 3, + "github-light": 2, + "github-heavy": 1, + tooling: 1, + }, + timeoutMs: { + local: 750, + registry: 1400, + "github-light": 1400, + "github-heavy": 2200, + tooling: 1400, + }, + responseReserveMs: 750, + }, + { + name: "deep", + default: false, + costClasses: ["local", "registry", "github-light", "github-heavy", "tooling"], + concurrency: { + local: 8, + registry: 4, + "github-light": 2, + "github-heavy": 1, + tooling: 1, + }, + timeoutMs: { + local: 1000, + registry: 2500, + "github-light": 2500, + "github-heavy": 4000, + tooling: 2500, + }, + responseReserveMs: 1000, + }, +] as const satisfies readonly ReesProfileDoc[]; + +export const REES_ANALYZERS = [ { name: "dependency", title: "Dependency vulnerabilities", - summary: "Checks changed direct dependency versions against OSV.dev.", - looksAt: "Added or upgraded dependencies in package.json, requirements.txt, and go.mod diffs.", - reports: - "Known CVEs with severity, advisory id, summary, and fixed version when OSV publishes one.", - network: "Calls OSV.dev. No GitHub token required.", - notes: "Manifest-only by design; use lockfileDrift for transitive lockfile changes.", + category: "supply-chain", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxManifestFiles: 20, + maxPatchLinesPerFile: 500, + maxDependencyQueries: 25, + }, + docs: { + summary: "Checks changed direct dependency versions against OSV.dev.", + looksAt: + "Added or upgraded dependencies in package.json, requirements.txt, and go.mod diffs.", + reports: + "Known CVEs with severity, advisory id, summary, and fixed version when OSV publishes one.", + network: "Calls OSV.dev. No GitHub token required.", + notes: "Manifest-only by design; use lockfileDrift for transitive lockfile changes.", + }, }, { name: "lockfileDrift", title: "Lockfile drift", - summary: - "Finds vulnerable transitive dependency versions introduced only through lockfile changes.", - looksAt: - "package-lock.json, yarn.lock, and poetry.lock patches, excluding packages already named in a changed manifest.", - reports: "Lockfile line, package/version, ecosystem, direction, and OSV vulnerability details.", - network: "Calls OSV.dev querybatch. No GitHub token required.", - notes: - "Useful when a PR does not touch a top-level manifest but changes resolved dependency pins.", + category: "supply-chain", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxLockfileFiles: 12, + maxPatchLinesPerFile: 1200, + maxOsvQueries: 40, + }, + docs: { + summary: + "Finds vulnerable transitive dependency versions introduced only through lockfile changes.", + looksAt: + "package-lock.json, yarn.lock, and poetry.lock patches, excluding packages already named in a changed manifest.", + reports: + "Lockfile line, package/version, ecosystem, direction, and OSV vulnerability details.", + network: "Calls OSV.dev querybatch. No GitHub token required.", + notes: + "Useful when a PR does not touch a top-level manifest but changes resolved dependency pins.", + }, }, { name: "secret", title: "Hardcoded secrets", - summary: "Scans added diff lines for credential-shaped values.", - looksAt: "Added lines in every changed file patch.", - reports: "File, line, secret kind, and confidence. The matched value is never returned.", - network: "Pure local analyzer. No external network call.", - notes: - "High-confidence patterns are treated as rotate-and-remove candidates; generic assignments stay verify-first.", + category: "security", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: {}, + docs: { + summary: "Scans added diff lines for credential-shaped values.", + looksAt: "Added lines in every changed file patch.", + reports: "File, line, secret kind, and confidence. The matched value is never returned.", + network: "Pure local analyzer. No external network call.", + notes: + "High-confidence patterns are treated as rotate-and-remove candidates; generic assignments stay verify-first.", + }, }, { name: "license", title: "Dependency licenses", - summary: "Checks licenses for newly added or upgraded dependencies.", - looksAt: "The same direct dependency changes used by the dependency analyzer.", - reports: - "Copyleft or unknown license classifications that need maintainer compatibility review.", - network: "Calls deps.dev. No GitHub token required.", - notes: "Permissive and otherwise-known licenses are intentionally silent.", + category: "supply-chain", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxLicenseLookups: 25, + }, + docs: { + summary: "Checks licenses for newly added or upgraded dependencies.", + looksAt: "The same direct dependency changes used by the dependency analyzer.", + reports: + "Copyleft or unknown license classifications that need maintainer compatibility review.", + network: "Calls deps.dev. No GitHub token required.", + notes: "Permissive and otherwise-known licenses are intentionally silent.", + }, }, { name: "installScript", title: "npm install scripts", - summary: "Flags npm packages that run lifecycle hooks during install.", - looksAt: "New or upgraded npm dependencies.", - reports: "Package, version, hook names, and publish date when available.", - network: "Calls the npm registry. No GitHub token required.", - notes: "The script body is not returned, which keeps the brief compact and non-executable.", + category: "supply-chain", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: {}, + docs: { + summary: "Flags npm packages that run lifecycle hooks during install.", + looksAt: "New or upgraded npm dependencies.", + reports: "Package, version, hook names, and publish date when available.", + network: "Calls the npm registry. No GitHub token required.", + notes: "The script body is not returned, which keeps the brief compact and non-executable.", + }, + }, + { + name: "heavyDependency", + title: "Heavy dependencies used trivially", + category: "performance", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxWeightLookups: 20, + maxFindings: 15, + }, + docs: { + summary: "Flags materially heavy npm dependencies used only a few times in changed lines.", + looksAt: "New or upgraded npm dependencies plus direct uses in added lines.", + reports: "Package size, dependency count, usage count, and line-cited usage locations.", + network: "Calls Bundlephobia. No GitHub token required.", + notes: "Only reports packages with trivial direct usage so the finding stays actionable.", + }, }, { name: "actionPin", title: "Unpinned GitHub Actions", - summary: "Detects third-party workflow actions pinned to mutable tags or branches.", - looksAt: "Added uses: lines in .github/workflows YAML patches.", - reports: "Workflow file, line, action, and mutable ref.", - network: "Pure local analyzer. No external network call.", - notes: "Official actions/* and github/* actions are excluded to keep the signal focused.", + category: "supply-chain", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: {}, + docs: { + summary: "Detects third-party workflow actions pinned to mutable tags or branches.", + looksAt: "Added uses: lines in .github/workflows YAML patches.", + reports: "Workflow file, line, action, and mutable ref.", + network: "Pure local analyzer. No external network call.", + notes: "Official actions/* and github/* actions are excluded to keep the signal focused.", + }, }, { name: "eol", title: "End-of-life runtimes", - summary: "Checks changed runtime and base-image pins against EOL calendars.", - looksAt: "Dockerfile FROM lines, .nvmrc, and go.mod runtime pins.", - reports: - "File, product, version, EOL date, and whether the release is already EOL or close to EOL.", - network: "Calls endoflife.date. No GitHub token required.", - notes: "Only changed pins are checked; existing old runtimes outside the PR are not reported.", + category: "supply-chain", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxFiles: 40, + maxPatchLines: 1000, + maxPins: 80, + }, + docs: { + summary: "Checks changed runtime and base-image pins against EOL calendars.", + looksAt: "Dockerfile FROM lines, .nvmrc, and go.mod runtime pins.", + reports: + "File, product, version, EOL date, and whether the release is already EOL or close to EOL.", + network: "Calls endoflife.date. No GitHub token required.", + notes: + "Only changed pins are checked; existing old runtimes outside the PR are not reported.", + }, }, { name: "redos", title: "ReDoS-prone regex", - summary: "Finds newly introduced regex shapes that can catastrophically backtrack.", - looksAt: "Regex literals and RegExp constructor string arguments in added lines.", - reports: "File, line, and a truncated vulnerable pattern.", - network: "Pure local analyzer. No external network call.", - notes: - "Structural and precision-first; it flags nested unbounded quantifier shapes such as (a+)+.", + category: "security", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + maxPatternChars: 1000, + maxLineChars: 2000, + }, + docs: { + summary: "Finds newly introduced regex shapes that can catastrophically backtrack.", + looksAt: "Regex literals and RegExp constructor string arguments in added lines.", + reports: "File, line, and a truncated vulnerable pattern.", + network: "Pure local analyzer. No external network call.", + notes: + "Structural and precision-first; it flags nested unbounded quantifier shapes such as (a+)+.", + }, }, { name: "provenance", title: "Provenance and committed artifacts", - summary: "Checks package attestations and reviewability of newly added artifacts.", - looksAt: "New npm/PyPI dependency versions plus added binary, vendored, and minified files.", - reports: - "Missing attestations, binary files without reviewable source, and vendored or minified code.", - network: - "Calls npm and PyPI attestation/provenance endpoints for package checks. Path checks are local.", - notes: "Network failures fail safe; it flags only confident no-attestation responses.", + category: "supply-chain", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxAttestationChecks: 20, + maxFindings: 30, + }, + docs: { + summary: "Checks package attestations and reviewability of newly added artifacts.", + looksAt: "New npm/PyPI dependency versions plus added binary, vendored, and minified files.", + reports: + "Missing attestations, binary files without reviewable source, and vendored or minified code.", + network: + "Calls npm and PyPI attestation/provenance endpoints for package checks. Path checks are local.", + notes: "Network failures fail safe; it flags only confident no-attestation responses.", + }, }, { name: "codeowners", title: "CODEOWNERS coverage", - summary: "Checks whether changed files cross ownership domains not owned by the PR author.", - looksAt: ".github/CODEOWNERS, CODEOWNERS, or docs/CODEOWNERS plus the changed file list.", - reports: - "Owned files where the PR author is not listed, plus ownership blast-radius context in the rendered brief.", - network: - "Calls the GitHub API. Requires author plus GitHub token forwarding for private repos.", - notes: - "Leave REES_FORWARD_GITHUB_TOKEN unset/false to disable token forwarding; this analyzer will then skip when it cannot read CODEOWNERS.", + category: "ownership", + cost: "github-light", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "author", "github-token"], + limits: { + maxFilesReported: 20, + maxCodeownersBytes: 65536, + maxCodeownersRules: 1000, + }, + docs: { + summary: "Checks whether changed files cross ownership domains not owned by the PR author.", + looksAt: ".github/CODEOWNERS, CODEOWNERS, or docs/CODEOWNERS plus the changed file list.", + reports: + "Owned files where the PR author is not listed, plus ownership blast-radius context in the rendered brief.", + network: + "Calls the GitHub API. Requires author plus GitHub token forwarding for private repos.", + notes: + "Leave REES_FORWARD_GITHUB_TOKEN unset/false to disable token forwarding; this analyzer will then skip when it cannot read CODEOWNERS.", + }, }, { name: "secretLog", title: "Secrets or PII in logs", - summary: "Flags added code that writes sensitive values to logs or stdout.", - looksAt: "Added lines that call console, logger, process.stdout, or process.stderr sinks.", - reports: "File, line, sink, and category: secret, pii, or request-object.", - network: "Pure local analyzer. No external network call.", - notes: - "String log messages are stripped before matching, so ordinary prose like password reset is not enough to trigger.", + category: "security", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + maxLineChars: 2000, + }, + docs: { + summary: "Flags added code that writes sensitive values to logs or stdout.", + looksAt: "Added lines that call console, logger, process.stdout, or process.stderr sinks.", + reports: "File, line, sink, and category: secret, pii, or request-object.", + network: "Pure local analyzer. No external network call.", + notes: + "String log messages are stripped before matching, so ordinary prose like password reset is not enough to trigger.", + }, }, { name: "assetWeight", title: "Heavy binary assets", - summary: - "Finds large binary assets added to a PR, and growth deltas when base size is available.", - looksAt: - "Changed binary assets such as images, fonts, archives, PDFs, videos, and compiled binaries.", - reports: "Path, size, delta, and whether the asset was added or grown.", - network: - "Calls the GitHub API. Requires headSha and GitHub token forwarding for private repos.", - notes: - "Added asset detection works from headSha. Growth comparison needs baseSha in the enrichment request.", + category: "performance", + cost: "github-heavy", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token", "head-sha"], + limits: { + maxFindings: 50, + }, + docs: { + summary: + "Finds large binary assets added to a PR, and growth deltas when base size is available.", + looksAt: + "Changed binary assets such as images, fonts, archives, PDFs, videos, and compiled binaries.", + reports: "Path, size, delta, and whether the asset was added or grown.", + network: + "Calls the GitHub API. Requires headSha and GitHub token forwarding for private repos.", + notes: + "Added asset detection works from headSha. Growth comparison needs baseSha in the enrichment request.", + }, }, { name: "typosquat", title: "Typosquat and dependency-confusion risk", - summary: - "Checks newly added dependency names for near-miss and publicly claimable package names.", - looksAt: "Newly added npm and PyPI dependency names.", - reports: - "Typosquat matches against popular packages, or unscoped names missing from the public registry.", - network: - "Uses bundled popular-package lists plus npm/PyPI registry lookups for dependency-confusion checks.", - notes: - "Scoped npm packages are treated as namespace-protected and are not flagged as typosquats.", - }, -]; + category: "supply-chain", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxDeps: 50, + maxConfusionQueries: 15, + }, + docs: { + summary: + "Checks newly added dependency names for near-miss and publicly claimable package names.", + looksAt: "Newly added npm and PyPI dependency names.", + reports: + "Typosquat matches against popular packages, or unscoped names missing from the public registry.", + network: + "Uses bundled popular-package lists plus npm/PyPI registry lookups for dependency-confusion checks.", + notes: + "Scoped npm packages are treated as namespace-protected and are not flagged as typosquats.", + }, + }, + { + name: "commitSignature", + title: "Head commit signature", + category: "supply-chain", + cost: "github-light", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["github-token", "head-sha"], + limits: {}, + docs: { + summary: "Checks head commit signature and public author provenance.", + looksAt: "The head commit plus a bounded slice of recent repository commit history.", + reports: "GitHub signature verification reason and public boolean provenance flags.", + network: + "Calls the GitHub API. Requires headSha and GitHub token forwarding for private repos.", + notes: + "Does not expose emails or private identity data; only public GitHub commit facts are surfaced.", + }, + }, + { + name: "iacMisconfig", + title: "IaC / config misconfiguration", + category: "config", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + maxLineChars: 2000, + }, + docs: { + summary: "Flags risky IaC/config changes such as public buckets or insecure CORS.", + looksAt: "Added lines in Docker, Terraform, YAML, JSON, and similar config files.", + reports: "File, line, and public-safe rule kind.", + network: "Pure local analyzer. No external network call.", + notes: "Reports configuration shapes only; it does not inspect private runtime config.", + }, + }, + { + name: "nativeBuild", + title: "Native-build dependencies", + category: "performance", + cost: "registry", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files", "public-network"], + limits: { + maxQueries: 25, + maxRegistryJsonBytes: 2097152, + }, + docs: { + summary: "Flags newly-added dependencies that compile native code or ship sdist-only builds.", + looksAt: "New npm/PyPI dependency versions.", + reports: "Package, version, ecosystem, native-build kind, and public-safe reason.", + network: "Calls npm and PyPI registries. No GitHub token required.", + notes: "Registry JSON is capped so large package metadata cannot monopolize REES memory.", + }, + }, + { + name: "history", + title: "Author and change-area history", + category: "history", + cost: "github-heavy", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token", "author"], + limits: { + maxFilesProbed: 5, + commitsPerFile: 10, + maxPrLookups: 12, + maxSimilarPrs: 8, + }, + docs: { + summary: + "Shows public author track record, same-file PR history, and linked-issue alignment.", + looksAt: + "The PR author, changed file paths, linked issue text, added diff lines, and bounded GitHub history lookups.", + reports: + "Prior PR counts, similar past PRs, linked issue coverage, and partial/degraded status.", + network: + "Calls GitHub API with bounded fanout. Requires author plus GitHub token forwarding for private repos.", + notes: + "Returns partial findings when GitHub lookups are skipped, capped, or budget-exhausted.", + }, + }, + { + name: "docCommentDrift", + title: "Doc-comment drift", + category: "quality", + cost: "github-light", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token", "head-sha"], + limits: { + maxFiles: 20, + maxFindings: 50, + }, + docs: { + summary: + "Flags a JSDoc/TSDoc @param that names a parameter the PR removed or renamed but left documented.", + looksAt: + "Changed TS/JS source files at headSha, comparing each named function's old vs new parameter list.", + reports: "File, line, function, and the stale parameter name(s).", + network: + "Calls the GitHub API for changed file contents. Requires headSha and token forwarding for private repos.", + notes: + "Conservative: only named function declarations with confidently-enumerable params; non-parameter signature edits are not reported.", + }, + }, +] as const satisfies readonly ReesAnalyzerDoc[]; export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); diff --git a/apps/gittensory-ui/src/routes/docs.self-hosting-rees-analyzers.tsx b/apps/gittensory-ui/src/routes/docs.self-hosting-rees-analyzers.tsx index d5c384a03b..f25def2ea9 100644 --- a/apps/gittensory-ui/src/routes/docs.self-hosting-rees-analyzers.tsx +++ b/apps/gittensory-ui/src/routes/docs.self-hosting-rees-analyzers.tsx @@ -2,7 +2,7 @@ import { createFileRoute, Link } from "@tanstack/react-router"; import { DocsPage } from "@/components/site/docs-page"; import { Callout, CodeBlock, FeatureRow } from "@/components/site/primitives"; -import { REES_ANALYZERS, REES_ANALYZER_NAMES } from "@/lib/rees-analyzers"; +import { REES_ANALYZERS, REES_ANALYZER_NAMES, REES_PROFILES } from "@/lib/rees-analyzers"; export const Route = createFileRoute("/docs/self-hosting-rees-analyzers")({ head: () => ({ @@ -37,7 +37,8 @@ function SelfHostingReesAnalyzers() { REES runs analyzers independently. A failed analyzer is marked degraded, completed analyzers still return findings, and an empty result produces no user-facing brief. Use exact analyzer names in REES_ANALYZERS. A typo-only analyzer list fails closed with no - analyzers selected. + analyzers selected. Leave REES_PROFILE unset for the balanced profile, or set + fast during incidents to favor local and low-cost registry checks.

+

Profiles

+
+ {REES_PROFILES.map((profile) => ( +
+
+

{profile.name}

+ {profile.default ? ( + + default + + ) : null} +
+
+
+
Cost classes
+
{profile.costClasses.join(", ")}
+
+
+
Concurrency caps
+
+ {Object.entries(profile.concurrency) + .filter(([, value]) => value > 0) + .map(([key, value]) => `${key}:${value}`) + .join(", ")} +
+
+
+
Response reserve
+
{profile.responseReserveMs} ms
+
+
+
+ ))} +
+

All analyzer names

@@ -61,17 +97,17 @@ REES_ANALYZERS=unknownName`} { title: "Pure analyzers", description: - "secret, actionPin, redos, and secretLog work only from the diff/files sent to REES.", + "secret, actionPin, redos, secretLog, and iacMisconfig work only from the diff/files sent to REES.", }, { title: "Public registry analyzers", description: - "dependency, lockfileDrift, license, installScript, eol, provenance, and typosquat call public package or lifecycle APIs.", + "dependency, lockfileDrift, license, installScript, heavyDependency, eol, provenance, typosquat, and nativeBuild call public package or lifecycle APIs.", }, { title: "GitHub API analyzers", description: - "codeowners and assetWeight need author/head metadata and GitHub token forwarding when the repo is private.", + "codeowners, assetWeight, commitSignature, and history need author/head metadata and GitHub token forwarding when the repo is private.", }, ]} /> @@ -89,29 +125,42 @@ REES_ANALYZERS=unknownName`}

{analyzer.title}

- {analyzer.summary} + {analyzer.docs.summary}

- - {analyzer.name} - +
+ + {analyzer.name} + + + {analyzer.cost} + +
Looks at
-
{analyzer.looksAt}
+
{analyzer.docs.looksAt}
Reports
-
{analyzer.reports}
+
{analyzer.docs.reports}
Network
-
{analyzer.network}
+
{analyzer.docs.network}
Operational note
-
{analyzer.notes}
+
{analyzer.docs.notes}
+
+
+
Profiles
+
{analyzer.profiles.join(", ")}
+
+
+
Requirements
+
{analyzer.requires.join(", ")}
diff --git a/apps/gittensory-ui/src/routes/docs.self-hosting-rees.tsx b/apps/gittensory-ui/src/routes/docs.self-hosting-rees.tsx index b160cd29a6..b75ec86e38 100644 --- a/apps/gittensory-ui/src/routes/docs.self-hosting-rees.tsx +++ b/apps/gittensory-ui/src/routes/docs.self-hosting-rees.tsx @@ -79,6 +79,7 @@ GITTENSORY_REVIEW_ENRICHMENT=true REES_URL=https://enrichment.example.internal REES_SHARED_SECRET= REES_TIMEOUT_MS=8000 +REES_PROFILE=balanced REES_FORWARD_GITHUB_TOKEN=false REES_ANALYZERS=all`} /> @@ -101,6 +102,11 @@ REES_ANALYZERS=all`} title: "REES_TIMEOUT_MS", description: "Request timeout. Defaults to 8000 ms and is clamped to at least 1000 ms.", }, + { + title: "REES_PROFILE", + description: + "Optional analyzer profile. balanced is the default; fast favors local/registry checks during incidents; deep allows larger per-class budgets.", + }, { title: "REES_FORWARD_GITHUB_TOKEN", description: @@ -127,13 +133,18 @@ REES_FORWARD_GITHUB_TOKEN=true`}

Analyzer selection

- Leave REES_ANALYZERS unset, all, or * to run the full - REES registry. To run a subset, use exact comma-separated analyzer names. Unknown names are - ignored with a rees_analyzer_config_invalid warning and the remaining valid - analyzers still run. If every configured name is invalid, the engine sends an empty analyzer - list so the typo fails closed instead of running the full registry. + Leave REES_ANALYZERS unset, all, or * to use the + selected REES_PROFILE defaults. To run a subset, use exact comma-separated + analyzer names. Unknown names are ignored with a rees_analyzer_config_invalid{" "} + warning and the remaining valid analyzers still run. If every configured name is invalid, + the engine sends an empty analyzer list so the typo fails closed instead of running the full + registry.

- +

See the REES analyzer reference for each diff --git a/package.json b/package.json index c4166829c3..dc4f8a0401 100644 --- a/package.json +++ b/package.json @@ -22,6 +22,8 @@ "test:mcp-pack": "node scripts/check-mcp-package.mjs", "rees:install": "npm ci --prefix review-enrichment --prefer-offline --no-audit --no-fund", "rees:test": "npm run rees:install && npm --prefix review-enrichment test", + "rees:metadata": "npm --prefix review-enrichment run metadata", + "rees:metadata:check": "npm --prefix review-enrichment run metadata:check", "rees:validate-sourcemaps": "npm --prefix review-enrichment run validate:sourcemaps", "db:migrations:check": "node scripts/check-migrations.mjs", "actionlint": "node scripts/actionlint.mjs", diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json new file mode 100644 index 0000000000..9ad7e58ad8 --- /dev/null +++ b/review-enrichment/analyzer-metadata.json @@ -0,0 +1,584 @@ +{ + "schemaVersion": 1, + "generatedFrom": "review-enrichment/src/analyzers/registry.ts", + "defaultProfile": "balanced", + "profiles": [ + { + "name": "fast", + "default": false, + "costClasses": [ + "local", + "registry" + ], + "concurrency": { + "local": 8, + "registry": 2, + "github-light": 0, + "github-heavy": 0, + "tooling": 0 + }, + "timeoutMs": { + "local": 400, + "registry": 800, + "github-light": 0, + "github-heavy": 0, + "tooling": 0 + }, + "responseReserveMs": 500 + }, + { + "name": "balanced", + "default": true, + "costClasses": [ + "local", + "registry", + "github-light", + "github-heavy", + "tooling" + ], + "concurrency": { + "local": 8, + "registry": 3, + "github-light": 2, + "github-heavy": 1, + "tooling": 1 + }, + "timeoutMs": { + "local": 750, + "registry": 1400, + "github-light": 1400, + "github-heavy": 2200, + "tooling": 1400 + }, + "responseReserveMs": 750 + }, + { + "name": "deep", + "default": false, + "costClasses": [ + "local", + "registry", + "github-light", + "github-heavy", + "tooling" + ], + "concurrency": { + "local": 8, + "registry": 4, + "github-light": 2, + "github-heavy": 1, + "tooling": 1 + }, + "timeoutMs": { + "local": 1000, + "registry": 2500, + "github-light": 2500, + "github-heavy": 4000, + "tooling": 2500 + }, + "responseReserveMs": 1000 + } + ], + "analyzers": [ + { + "name": "dependency", + "title": "Dependency vulnerabilities", + "category": "supply-chain", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxManifestFiles": 20, + "maxPatchLinesPerFile": 500, + "maxDependencyQueries": 25 + }, + "docs": { + "summary": "Checks changed direct dependency versions against OSV.dev.", + "looksAt": "Added or upgraded dependencies in package.json, requirements.txt, and go.mod diffs.", + "reports": "Known CVEs with severity, advisory id, summary, and fixed version when OSV publishes one.", + "network": "Calls OSV.dev. No GitHub token required.", + "notes": "Manifest-only by design; use lockfileDrift for transitive lockfile changes." + } + }, + { + "name": "lockfileDrift", + "title": "Lockfile drift", + "category": "supply-chain", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxLockfileFiles": 12, + "maxPatchLinesPerFile": 1200, + "maxOsvQueries": 40 + }, + "docs": { + "summary": "Finds vulnerable transitive dependency versions introduced only through lockfile changes.", + "looksAt": "package-lock.json, yarn.lock, and poetry.lock patches, excluding packages already named in a changed manifest.", + "reports": "Lockfile line, package/version, ecosystem, direction, and OSV vulnerability details.", + "network": "Calls OSV.dev querybatch. No GitHub token required.", + "notes": "Useful when a PR does not touch a top-level manifest but changes resolved dependency pins." + } + }, + { + "name": "secret", + "title": "Hardcoded secrets", + "category": "security", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": {}, + "docs": { + "summary": "Scans added diff lines for credential-shaped values.", + "looksAt": "Added lines in every changed file patch.", + "reports": "File, line, secret kind, and confidence. The matched value is never returned.", + "network": "Pure local analyzer. No external network call.", + "notes": "High-confidence patterns are treated as rotate-and-remove candidates; generic assignments stay verify-first." + } + }, + { + "name": "license", + "title": "Dependency licenses", + "category": "supply-chain", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxLicenseLookups": 25 + }, + "docs": { + "summary": "Checks licenses for newly added or upgraded dependencies.", + "looksAt": "The same direct dependency changes used by the dependency analyzer.", + "reports": "Copyleft or unknown license classifications that need maintainer compatibility review.", + "network": "Calls deps.dev. No GitHub token required.", + "notes": "Permissive and otherwise-known licenses are intentionally silent." + } + }, + { + "name": "installScript", + "title": "npm install scripts", + "category": "supply-chain", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": {}, + "docs": { + "summary": "Flags npm packages that run lifecycle hooks during install.", + "looksAt": "New or upgraded npm dependencies.", + "reports": "Package, version, hook names, and publish date when available.", + "network": "Calls the npm registry. No GitHub token required.", + "notes": "The script body is not returned, which keeps the brief compact and non-executable." + } + }, + { + "name": "heavyDependency", + "title": "Heavy dependencies used trivially", + "category": "performance", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxWeightLookups": 20, + "maxFindings": 15 + }, + "docs": { + "summary": "Flags materially heavy npm dependencies used only a few times in changed lines.", + "looksAt": "New or upgraded npm dependencies plus direct uses in added lines.", + "reports": "Package size, dependency count, usage count, and line-cited usage locations.", + "network": "Calls Bundlephobia. No GitHub token required.", + "notes": "Only reports packages with trivial direct usage so the finding stays actionable." + } + }, + { + "name": "actionPin", + "title": "Unpinned GitHub Actions", + "category": "supply-chain", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": {}, + "docs": { + "summary": "Detects third-party workflow actions pinned to mutable tags or branches.", + "looksAt": "Added uses: lines in .github/workflows YAML patches.", + "reports": "Workflow file, line, action, and mutable ref.", + "network": "Pure local analyzer. No external network call.", + "notes": "Official actions/* and github/* actions are excluded to keep the signal focused." + } + }, + { + "name": "eol", + "title": "End-of-life runtimes", + "category": "supply-chain", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxFiles": 40, + "maxPatchLines": 1000, + "maxPins": 80 + }, + "docs": { + "summary": "Checks changed runtime and base-image pins against EOL calendars.", + "looksAt": "Dockerfile FROM lines, .nvmrc, and go.mod runtime pins.", + "reports": "File, product, version, EOL date, and whether the release is already EOL or close to EOL.", + "network": "Calls endoflife.date. No GitHub token required.", + "notes": "Only changed pins are checked; existing old runtimes outside the PR are not reported." + } + }, + { + "name": "redos", + "title": "ReDoS-prone regex", + "category": "security", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25, + "maxPatternChars": 1000, + "maxLineChars": 2000 + }, + "docs": { + "summary": "Finds newly introduced regex shapes that can catastrophically backtrack.", + "looksAt": "Regex literals and RegExp constructor string arguments in added lines.", + "reports": "File, line, and a truncated vulnerable pattern.", + "network": "Pure local analyzer. No external network call.", + "notes": "Structural and precision-first; it flags nested unbounded quantifier shapes such as (a+)+." + } + }, + { + "name": "provenance", + "title": "Provenance and committed artifacts", + "category": "supply-chain", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxAttestationChecks": 20, + "maxFindings": 30 + }, + "docs": { + "summary": "Checks package attestations and reviewability of newly added artifacts.", + "looksAt": "New npm/PyPI dependency versions plus added binary, vendored, and minified files.", + "reports": "Missing attestations, binary files without reviewable source, and vendored or minified code.", + "network": "Calls npm and PyPI attestation/provenance endpoints for package checks. Path checks are local.", + "notes": "Network failures fail safe; it flags only confident no-attestation responses." + } + }, + { + "name": "codeowners", + "title": "CODEOWNERS coverage", + "category": "ownership", + "cost": "github-light", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "author", + "github-token" + ], + "limits": { + "maxFilesReported": 20, + "maxCodeownersBytes": 65536, + "maxCodeownersRules": 1000 + }, + "docs": { + "summary": "Checks whether changed files cross ownership domains not owned by the PR author.", + "looksAt": ".github/CODEOWNERS, CODEOWNERS, or docs/CODEOWNERS plus the changed file list.", + "reports": "Owned files where the PR author is not listed, plus ownership blast-radius context in the rendered brief.", + "network": "Calls the GitHub API. Requires author plus GitHub token forwarding for private repos.", + "notes": "Leave REES_FORWARD_GITHUB_TOKEN unset/false to disable token forwarding; this analyzer will then skip when it cannot read CODEOWNERS." + } + }, + { + "name": "secretLog", + "title": "Secrets or PII in logs", + "category": "security", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25, + "maxLineChars": 2000 + }, + "docs": { + "summary": "Flags added code that writes sensitive values to logs or stdout.", + "looksAt": "Added lines that call console, logger, process.stdout, or process.stderr sinks.", + "reports": "File, line, sink, and category: secret, pii, or request-object.", + "network": "Pure local analyzer. No external network call.", + "notes": "String log messages are stripped before matching, so ordinary prose like password reset is not enough to trigger." + } + }, + { + "name": "assetWeight", + "title": "Heavy binary assets", + "category": "performance", + "cost": "github-heavy", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token", + "head-sha" + ], + "limits": { + "maxFindings": 50 + }, + "docs": { + "summary": "Finds large binary assets added to a PR, and growth deltas when base size is available.", + "looksAt": "Changed binary assets such as images, fonts, archives, PDFs, videos, and compiled binaries.", + "reports": "Path, size, delta, and whether the asset was added or grown.", + "network": "Calls the GitHub API. Requires headSha and GitHub token forwarding for private repos.", + "notes": "Added asset detection works from headSha. Growth comparison needs baseSha in the enrichment request." + } + }, + { + "name": "typosquat", + "title": "Typosquat and dependency-confusion risk", + "category": "supply-chain", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxDeps": 50, + "maxConfusionQueries": 15 + }, + "docs": { + "summary": "Checks newly added dependency names for near-miss and publicly claimable package names.", + "looksAt": "Newly added npm and PyPI dependency names.", + "reports": "Typosquat matches against popular packages, or unscoped names missing from the public registry.", + "network": "Uses bundled popular-package lists plus npm/PyPI registry lookups for dependency-confusion checks.", + "notes": "Scoped npm packages are treated as namespace-protected and are not flagged as typosquats." + } + }, + { + "name": "commitSignature", + "title": "Head commit signature", + "category": "supply-chain", + "cost": "github-light", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "github-token", + "head-sha" + ], + "limits": {}, + "docs": { + "summary": "Checks head commit signature and public author provenance.", + "looksAt": "The head commit plus a bounded slice of recent repository commit history.", + "reports": "GitHub signature verification reason and public boolean provenance flags.", + "network": "Calls the GitHub API. Requires headSha and GitHub token forwarding for private repos.", + "notes": "Does not expose emails or private identity data; only public GitHub commit facts are surfaced." + } + }, + { + "name": "iacMisconfig", + "title": "IaC / config misconfiguration", + "category": "config", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25, + "maxLineChars": 2000 + }, + "docs": { + "summary": "Flags risky IaC/config changes such as public buckets or insecure CORS.", + "looksAt": "Added lines in Docker, Terraform, YAML, JSON, and similar config files.", + "reports": "File, line, and public-safe rule kind.", + "network": "Pure local analyzer. No external network call.", + "notes": "Reports configuration shapes only; it does not inspect private runtime config." + } + }, + { + "name": "nativeBuild", + "title": "Native-build dependencies", + "category": "performance", + "cost": "registry", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files", + "public-network" + ], + "limits": { + "maxQueries": 25, + "maxRegistryJsonBytes": 2097152 + }, + "docs": { + "summary": "Flags newly-added dependencies that compile native code or ship sdist-only builds.", + "looksAt": "New npm/PyPI dependency versions.", + "reports": "Package, version, ecosystem, native-build kind, and public-safe reason.", + "network": "Calls npm and PyPI registries. No GitHub token required.", + "notes": "Registry JSON is capped so large package metadata cannot monopolize REES memory." + } + }, + { + "name": "history", + "title": "Author and change-area history", + "category": "history", + "cost": "github-heavy", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token", + "author" + ], + "limits": { + "maxFilesProbed": 5, + "commitsPerFile": 10, + "maxPrLookups": 12, + "maxSimilarPrs": 8 + }, + "docs": { + "summary": "Shows public author track record, same-file PR history, and linked-issue alignment.", + "looksAt": "The PR author, changed file paths, linked issue text, added diff lines, and bounded GitHub history lookups.", + "reports": "Prior PR counts, similar past PRs, linked issue coverage, and partial/degraded status.", + "network": "Calls GitHub API with bounded fanout. Requires author plus GitHub token forwarding for private repos.", + "notes": "Returns partial findings when GitHub lookups are skipped, capped, or budget-exhausted." + } + }, + { + "name": "docCommentDrift", + "title": "Doc-comment drift", + "category": "quality", + "cost": "github-light", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token", + "head-sha" + ], + "limits": { + "maxFiles": 20, + "maxFindings": 50 + }, + "docs": { + "summary": "Flags a JSDoc/TSDoc @param that names a parameter the PR removed or renamed but left documented.", + "looksAt": "Changed TS/JS source files at headSha, comparing each named function's old vs new parameter list.", + "reports": "File, line, function, and the stale parameter name(s).", + "network": "Calls the GitHub API for changed file contents. Requires headSha and token forwarding for private repos.", + "notes": "Conservative: only named function declarations with confidently-enumerable params; non-parameter signature edits are not reported." + } + } + ] +} diff --git a/review-enrichment/package-lock.json b/review-enrichment/package-lock.json index 5d4b500539..11a02820da 100644 --- a/review-enrichment/package-lock.json +++ b/review-enrichment/package-lock.json @@ -15,6 +15,7 @@ }, "devDependencies": { "@types/node": "^22.10.2", + "prettier": "3.8.4", "typescript": "^5.7.2" }, "engines": { @@ -609,6 +610,22 @@ "integrity": "sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==", "license": "MIT" }, + "node_modules/prettier": { + "version": "3.8.4", + "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.8.4.tgz", + "integrity": "sha512-N2MylSdi48+5N/6S5j+maeHbUSIzzZ5uOcX5Hm4QpV8Dkb1HFjfAKTKX6yNPJQD9AhcT3ifHNB66tWTTJDi11Q==", + "dev": true, + "license": "MIT", + "bin": { + "prettier": "bin/prettier.cjs" + }, + "engines": { + "node": ">=14" + }, + "funding": { + "url": "https://github.com/prettier/prettier?sponsor=1" + } + }, "node_modules/progress": { "version": "2.0.3", "resolved": "https://registry.npmjs.org/progress/-/progress-2.0.3.tgz", diff --git a/review-enrichment/package.json b/review-enrichment/package.json index 0b919e0d61..404ebd3477 100644 --- a/review-enrichment/package.json +++ b/review-enrichment/package.json @@ -9,10 +9,12 @@ }, "scripts": { "build": "tsc -p tsconfig.json", + "metadata": "npm run build && node scripts/generate-analyzer-metadata.mjs", + "metadata:check": "npm run build && node scripts/generate-analyzer-metadata.mjs --check", "validate:sourcemaps": "node scripts/validate-sourcemaps.mjs", "start": "node dist/server.js", "dev": "node --experimental-strip-types --watch src/server.ts", - "test": "npm run build && npm run validate:sourcemaps && node --test --experimental-strip-types \"test/**/*.test.ts\"" + "test": "npm run build && npm run validate:sourcemaps && node scripts/generate-analyzer-metadata.mjs --check && node --test --experimental-strip-types \"test/**/*.test.ts\"" }, "dependencies": { "@hono/node-server": "^1.13.7", @@ -22,6 +24,7 @@ }, "devDependencies": { "@types/node": "^22.10.2", + "prettier": "3.8.4", "typescript": "^5.7.2" } } diff --git a/review-enrichment/scripts/generate-analyzer-metadata.mjs b/review-enrichment/scripts/generate-analyzer-metadata.mjs new file mode 100644 index 0000000000..6bc05c3715 --- /dev/null +++ b/review-enrichment/scripts/generate-analyzer-metadata.mjs @@ -0,0 +1,157 @@ +import { readFile, writeFile } from "node:fs/promises"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { format, resolveConfig } from "prettier"; + +import { ANALYZER_DESCRIPTORS } from "../dist/analyzers/registry.js"; +import { reesProfileMetadata } from "../dist/scheduler.js"; + +const CHECK = process.argv.includes("--check"); +const scriptDir = dirname(fileURLToPath(import.meta.url)); +const reviewRoot = resolve(scriptDir, ".."); +const repoRoot = resolve(reviewRoot, ".."); +const jsonPath = resolve(reviewRoot, "analyzer-metadata.json"); +const uiPath = resolve(repoRoot, "apps/gittensory-ui/src/lib/rees-analyzers.ts"); +const envPath = resolve(repoRoot, ".env.example"); +const envStart = "# BEGIN GENERATED REES ANALYZERS"; +const envEnd = "# END GENERATED REES ANALYZERS"; + +const profiles = reesProfileMetadata(); +const defaultProfile = profiles.find((profile) => profile.default)?.name ?? "balanced"; + +const analyzers = ANALYZER_DESCRIPTORS.map((descriptor) => ({ + name: descriptor.name, + title: descriptor.title, + category: descriptor.category, + cost: descriptor.cost, + defaultEnabled: descriptor.defaultEnabled, + profiles: profiles + .filter( + (profile) => + descriptor.defaultEnabled && + profile.costClasses.includes(descriptor.cost), + ) + .map((profile) => profile.name), + requires: [...descriptor.requires], + limits: descriptor.limits ? { ...descriptor.limits } : {}, + docs: { ...descriptor.docs }, +})); + +const metadata = { + schemaVersion: 1, + generatedFrom: "review-enrichment/src/analyzers/registry.ts", + defaultProfile, + profiles, + analyzers, +}; + +const generatedJson = `${JSON.stringify(metadata, null, 2)}\n`; +const rawGeneratedUi = `// Generated by review-enrichment/scripts/generate-analyzer-metadata.mjs. +// Do not edit by hand; update review-enrichment analyzer descriptors instead. + +export type ReesProfileName = "fast" | "balanced" | "deep"; + +export type ReesAnalyzerDoc = { + name: string; + title: string; + category: string; + cost: string; + defaultEnabled: boolean; + profiles: readonly ReesProfileName[]; + requires: readonly string[]; + limits: Readonly>; + docs: { + summary: string; + looksAt: string; + reports: string; + network: string; + notes: string; + }; +}; + +export type ReesProfileDoc = { + name: ReesProfileName; + default: boolean; + costClasses: readonly string[]; + concurrency: Readonly>; + timeoutMs: Readonly>; + responseReserveMs: number; +}; + +export const REES_DEFAULT_PROFILE = ${JSON.stringify(defaultProfile)} as const; + +export const REES_PROFILES = ${JSON.stringify(profiles, null, 2)} as const satisfies readonly ReesProfileDoc[]; + +export const REES_ANALYZERS = ${JSON.stringify(analyzers, null, 2)} as const satisfies readonly ReesAnalyzerDoc[]; + +export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name); +`; +const uiPrettierOptions = (await resolveConfig(uiPath)) ?? {}; +const generatedUi = await format(rawGeneratedUi, { + ...uiPrettierOptions, + filepath: uiPath, + parser: "typescript", +}); + +const envBlock = [ + envStart, + "# Current analyzer names:", + ...wrapComment(analyzers.map((analyzer) => analyzer.name).join(",")), + "#", + "# Profile defaults:", + ...profiles.flatMap((profile) => + wrapComment( + `${profile.name}${profile.default ? " (default)" : ""}: ${analyzers + .filter((analyzer) => analyzer.profiles.includes(profile.name)) + .map((analyzer) => analyzer.name) + .join(",")}`, + ), + ), + envEnd, +].join("\n"); + +const currentEnv = await readFile(envPath, "utf8"); +const generatedEnv = replaceGeneratedEnvBlock(currentEnv, envBlock); + +await writeOrCheck(jsonPath, generatedJson); +await writeOrCheck(uiPath, generatedUi); +await writeOrCheck(envPath, generatedEnv); + +function wrapComment(value, max = 96) { + const words = value.split(","); + const lines = []; + let current = "# "; + for (const word of words) { + const next = current === "# " ? `${current}${word}` : `${current},${word}`; + if (next.length > max && current !== "# ") { + lines.push(current); + current = `# ${word}`; + } else { + current = next; + } + } + if (current !== "# ") lines.push(current); + return lines; +} + +function replaceGeneratedEnvBlock(content, block) { + const start = content.indexOf(envStart); + const end = content.indexOf(envEnd); + if (start === -1 || end === -1 || end < start) { + throw new Error(".env.example is missing generated REES analyzer markers"); + } + const afterEnd = end + envEnd.length; + return `${content.slice(0, start)}${block}${content.slice(afterEnd)}`; +} + +async function writeOrCheck(path, expected) { + if (!CHECK) { + await writeFile(path, expected); + return; + } + const actual = await readFile(path, "utf8").catch(() => ""); + if (actual !== expected) { + throw new Error(`${path.replace(`${repoRoot}/`, "")} is stale; run npm --prefix review-enrichment run metadata`); + } +} diff --git a/review-enrichment/src/analysis-context.ts b/review-enrichment/src/analysis-context.ts index 1a1564e653..d69ce04944 100644 --- a/review-enrichment/src/analysis-context.ts +++ b/review-enrichment/src/analysis-context.ts @@ -1,9 +1,21 @@ -import type { AnalyzerMetricsDiagnostics, EnrichRequest } from "./types.js"; +import type { + AnalyzerDiagnostics, + AnalyzerMetricsDiagnostics, + EnrichRequest, +} from "./types.js"; import { extractDependencyChanges, type DepChange, type ScanLimits, } from "./analyzers/dependency-scan.js"; +import { + boundedFetchJson, + boundedFetchText, + externalFetchCacheKey, + safeEndpointCategory, + type BoundedFetchOptions, + type BoundedFetchResult, +} from "./external-fetch.js"; type ChangedFile = NonNullable[number]; @@ -64,12 +76,29 @@ export interface AnalysisContext { key: string, load: () => Promise, ): Promise; + fetchJson( + url: string, + options: AnalysisFetchJsonOptions, + ): Promise>; + fetchText( + url: string, + options: AnalysisFetchJsonOptions, + ): Promise>; dependencyChanges(limits?: ScanLimits): readonly DepChange[]; packageChanges(limits?: ScanLimits): readonly DepChange[]; remainingMs(deadlineMs?: number): number; snapshotMetrics(): AnalysisContextMetrics; } +export interface AnalysisFetchJsonOptions + extends Omit { + endpointCategory: string; + cache?: boolean; + cacheKey?: string; + maxCallsPerCategory?: number; + diagnostics?: AnalyzerDiagnostics; +} + export class AnalysisMetrics { cacheHits = 0; cacheMisses = 0; @@ -94,6 +123,10 @@ export class AnalysisMetrics { incrementByCategory(this.externalCallsByCategory, category, count); } + externalCallCount(category: string): number { + return this.externalCallsByCategory[safeMetricCategory(category)] ?? 0; + } + recordSkippedWork(category: string, count = 1): void { incrementByCategory(this.skippedWorkByCategory, category, count); } @@ -181,6 +214,12 @@ export function createAnalysisContext( return load(); }); }, + fetchJson(url: string, options: AnalysisFetchJsonOptions) { + return cachedBoundedFetch(cache, metrics, url, options, boundedFetchJson); + }, + fetchText(url: string, options: AnalysisFetchJsonOptions) { + return cachedBoundedFetch(cache, metrics, url, options, boundedFetchText); + }, dependencyChanges(limits: ScanLimits = {}) { const key = dependencyLimitKey(limits); const cached = dependencyChangeCache.get(key); @@ -306,7 +345,7 @@ function categorizeFile(path: string): FileCategory { } if ( /^Dockerfile(?:\..*)?$/.test(basename) || - [".env", ".ini", ".json", ".toml", ".yaml", ".yml"].includes(extension) + [".env", ".hcl", ".ini", ".json", ".tf", ".toml", ".yaml", ".yml"].includes(extension) ) { return { path, extension, category: "config" }; } @@ -351,3 +390,53 @@ function safeMetricCategory(category: string): string { const safe = category.replace(/[^A-Za-z0-9_.:-]+/g, "_").slice(0, 80); return safe || "unknown"; } + +function markExternalCap( + diagnostics: AnalyzerDiagnostics, + endpointCategory: string, +): void { + diagnostics.partialStatus = "partial"; + diagnostics.partialReason ??= `${endpointCategory}_call_cap`; + diagnostics.captureDegradation = true; + diagnostics.endpointCategory = endpointCategory; + diagnostics.externalFailureReason = "call_cap"; + diagnostics.subcall = endpointCategory; + diagnostics.capped = true; + if (endpointCategory.startsWith("github-")) { + diagnostics.githubEndpointCategory = endpointCategory; + } +} + +function cachedBoundedFetch( + cache: RequestScopedCache, + metrics: AnalysisMetrics, + url: string, + options: AnalysisFetchJsonOptions, + loadBounded: ( + url: string, + options: BoundedFetchOptions, + ) => Promise>, +): Promise> { + const category = safeEndpointCategory(options.endpointCategory); + const cacheKey = options.cacheKey ?? externalFetchCacheKey(url, options); + const load = () => { + if ( + typeof options.maxCallsPerCategory === "number" && + metrics.externalCallCount(category) >= options.maxCallsPerCategory + ) { + metrics.recordCappedWork(`${category}_calls`); + options.diagnostics && markExternalCap(options.diagnostics, category); + return Promise.resolve({ + ok: false as const, + reason: "call_cap" as const, + bytes: null, + elapsedMs: 0, + endpointCategory: category, + capped: true, + }); + } + metrics.recordExternalCall(category); + return loadBounded(url, { ...options, endpointCategory: category }); + }; + return options.cache === false ? load() : cache.getOrSet(category, cacheKey, load); +} diff --git a/review-enrichment/src/analyzers/asset-weight.ts b/review-enrichment/src/analyzers/asset-weight.ts index 158e535938..a74427e55b 100644 --- a/review-enrichment/src/analyzers/asset-weight.ts +++ b/review-enrichment/src/analyzers/asset-weight.ts @@ -5,9 +5,16 @@ // which also sidesteps the Contents API's 1 MB cap. Pure size arithmetic after that; no external service. // Fail-safe: returns [] without a token/headSha or when the head tree fetch is not OK; growth findings require a // matching base size. -import type { EnrichRequest, AssetWeightFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + AssetWeightFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; const MAX_FINDINGS = 50; // keep the brief bounded after evaluating every changed binary candidate +const MAX_PATH_SIZE_LOOKUPS = 50; // fallback Contents API calls when a recursive tree is truncated const THRESHOLD_BYTES = 100 * 1024; // flag a newly-added blob >= 100 KB, or growth >= 100 KB const GITHUB_API = "https://api.github.com"; const GITHUB_API_VERSION = "2022-11-28"; @@ -66,6 +73,8 @@ const BINARY_EXTS = new Set([ interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } // A single repo path segment (owner or name): word chars, dot, dash only. Whole-segment `.`/`..` are rejected @@ -103,6 +112,31 @@ function encodeRepoPath(path: string): string | null { return segments.map(encodeURIComponent).join("/"); } +async function fetchGithubJson( + url: string, + token: string, + fetchImpl: typeof fetch, + signal: AbortSignal | undefined, + options: ScanOptions, + endpointCategory: "github-trees" | "github-contents", +): Promise { + const fetchOptions = { + endpointCategory, + headers: githubHeaders(token), + signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "asset-weight", + subcall: endpointCategory, + maxBytes: endpointCategory === "github-trees" ? 4 * 1024 * 1024 : 256 * 1024, + maxCallsPerCategory: endpointCategory === "github-contents" ? MAX_PATH_SIZE_LOOKUPS : 2, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + return response.ok ? response.data : null; +} + /** Parse `owner/repo`, rejecting anything that isn't exactly two safe segments — no extra `/`, no `.`/`..` * traversal, no query/fragment characters. This stops a hostile `repoFullName` from redirecting the * token-bearing request to another repository. Returns null when unsafe. */ @@ -129,20 +163,17 @@ async function fetchTreeSizes( sha: string, token: string, fetchImpl: typeof fetch, - signal?: AbortSignal, + signal: AbortSignal | undefined, + options: ScanOptions, ): Promise<{ sizes: Map; truncated: boolean }> { const sizes = new Map(); if (!SHA_RE.test(sha)) return { sizes, truncated: false }; const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/git/trees/${encodeURIComponent(sha)}?recursive=1`; - const res = await fetchImpl(url, { - headers: githubHeaders(token), - signal, - }); - if (!res.ok) return { sizes, truncated: false }; - const json = (await res.json()) as { + const json = await fetchGithubJson<{ tree?: Array<{ path?: string; type?: string; size?: number }>; truncated?: boolean; - }; + }>(url, token, fetchImpl, signal, options, "github-trees"); + if (!json) return { sizes, truncated: false }; for (const entry of json.tree ?? []) { if (entry.type === "blob" && typeof entry.size === "number" && entry.path) { sizes.set(entry.path, entry.size); @@ -158,17 +189,24 @@ async function fetchPathSizes( token: string, paths: Iterable, fetchImpl: typeof fetch, - signal?: AbortSignal, + signal: AbortSignal | undefined, + options: ScanOptions, ): Promise> { const sizes = new Map(); if (!SHA_RE.test(sha)) return sizes; - for (const path of new Set(paths)) { + for (const path of [...new Set(paths)].slice(0, MAX_PATH_SIZE_LOOKUPS)) { const encodedPath = encodeRepoPath(path); if (!encodedPath) continue; const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${encodedPath}?ref=${encodeURIComponent(sha)}`; - const res = await fetchImpl(url, { headers: githubHeaders(token), signal }); - if (!res.ok) continue; - const json = (await res.json()) as { type?: string; size?: number } | unknown[]; + const json = await fetchGithubJson<{ type?: string; size?: number } | unknown[]>( + url, + token, + fetchImpl, + signal, + options, + "github-contents", + ); + if (!json) continue; if (!Array.isArray(json) && typeof json.size === "number") { sizes.set(path, json.size); } @@ -183,11 +221,12 @@ async function fetchRelevantSizes( token: string, paths: Iterable, fetchImpl: typeof fetch, - signal?: AbortSignal, + signal: AbortSignal | undefined, + options: ScanOptions, ): Promise> { - const tree = await fetchTreeSizes(owner, repo, sha, token, fetchImpl, signal); + const tree = await fetchTreeSizes(owner, repo, sha, token, fetchImpl, signal, options); if (!tree.truncated) return tree.sizes; - return fetchPathSizes(owner, repo, sha, token, paths, fetchImpl, signal); + return fetchPathSizes(owner, repo, sha, token, paths, fetchImpl, signal, options); } /** Analyzer entrypoint: flag heavy binary assets the PR adds or grows past the threshold. Pure size arithmetic over @@ -215,6 +254,7 @@ export async function scanAssetWeight( binaries.map((file) => file.path), fetchImpl, options.signal, + options, ); const basePaths = binaries.flatMap((file) => basePathForGrowth(file) ?? []); const needBase = binaries.some((f) => basePathForGrowth(f) !== null); @@ -225,10 +265,11 @@ export async function scanAssetWeight( repo.repo, req.baseSha, token, - basePaths, - fetchImpl, - options.signal, - ) + basePaths, + fetchImpl, + options.signal, + options, + ) : new Map(); const findings: AssetWeightFinding[] = []; diff --git a/review-enrichment/src/analyzers/codeowners.ts b/review-enrichment/src/analyzers/codeowners.ts index 2dc50e1039..101b63e761 100644 --- a/review-enrichment/src/analyzers/codeowners.ts +++ b/review-enrichment/src/analyzers/codeowners.ts @@ -4,7 +4,13 @@ // time from the unique set of ownership domains (users/teams) crossed by the PR. // CODEOWNERS matching uses a bounded, linear glob matcher instead of repository-controlled regular expressions. // Fail-safe: returns [] on any network error, non-ok response, or missing/unreadable CODEOWNERS file. -import type { EnrichRequest, CodeownersFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + CodeownersFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchText } from "../external-fetch.js"; const SLUG_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; // rejects `..` and other path-traversal segments const CODEOWNERS_PATHS = [ @@ -26,6 +32,12 @@ interface ParsedRule { owners: string[]; } +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + const MAX_CODEOWNERS_BYTES = 64 * 1024; const MAX_CODEOWNERS_RULES = 1000; const MAX_CODEOWNERS_PATTERN_LENGTH = 512; @@ -190,19 +202,25 @@ async function fetchCodeowners( repo: string, headers: Record, fetchFn: typeof fetch, - signal?: AbortSignal, + options: ScanOptions = {}, ): Promise { for (const path of CODEOWNERS_PATHS) { - try { - const resp = await fetchFn( - `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${path}`, - { headers, signal }, - ); - if (!resp.ok) continue; - return await resp.text(); - } catch { - // network error or already-aborted signal → try next location - } + const url = `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${path}`; + const fetchOptions = { + endpointCategory: "github-contents", + headers, + signal: options.signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "codeowners", + subcall: "github-contents", + maxBytes: MAX_CODEOWNERS_BYTES, + maxCallsPerCategory: CODEOWNERS_PATHS.length, + }; + const response = options.analysis + ? await options.analysis.fetchText(url, fetchOptions) + : await boundedFetchText(url, fetchOptions); + if (response.ok) return response.data; } return null; } @@ -213,7 +231,7 @@ async function fetchCodeowners( export async function scanCodeowners( req: EnrichRequest, fetchFn: typeof fetch, - opts?: { signal?: AbortSignal }, + opts: ScanOptions = {}, ): Promise { const { repoFullName, githubToken, author, files = [] } = req; if (!githubToken || !author) return []; @@ -240,7 +258,7 @@ export async function scanCodeowners( repoName, headers, fetchFn, - opts?.signal, + opts, ); if (!content) return []; diff --git a/review-enrichment/src/analyzers/commit-signature.ts b/review-enrichment/src/analyzers/commit-signature.ts index acc6bfc527..287ef9504c 100644 --- a/review-enrichment/src/analyzers/commit-signature.ts +++ b/review-enrichment/src/analyzers/commit-signature.ts @@ -4,7 +4,13 @@ // These are supply-chain / impersonation signals the no-checkout `claude --print` reviewer cannot derive // (no GitHub commit-verification API access, no repo history). Surfaces ONLY GitHub's public verification // verdict (`verified` + `reason`) and boolean provenance flags — never tokens, emails, or private identities. -import type { EnrichRequest, CommitSignatureFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + CommitSignatureFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; const GITHUB_API = "https://api.github.com"; // Pull a bounded slice of recent commits — enough to decide "has any verified history" without paging the whole @@ -16,6 +22,8 @@ const SLUG_RE = /^[A-Za-z0-9._-]+$/; interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } // The slice of the GitHub commit payload this analyzer reads. Everything else on the response is ignored. @@ -41,6 +49,30 @@ function githubHeaders(token: string): Record { }; } +async function fetchGithubJson( + url: string, + headers: Record, + fetchFn: typeof fetch, + signal?: AbortSignal, + options: Pick = {}, +): Promise { + const fetchOptions = { + endpointCategory: "github-commits", + headers, + signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "commit-signature", + subcall: "github-commits", + maxBytes: 256 * 1024, + maxCallsPerCategory: 3, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + return response.ok ? response.data : null; +} + /** Fetch the head commit's verification + identity payload. Returns null on any error / non-200 (fail-safe). */ export async function fetchHeadCommit( owner: string, @@ -49,18 +81,16 @@ export async function fetchHeadCommit( headers: Record, fetchFn: typeof fetch, signal?: AbortSignal, + options: Pick = {}, ): Promise { if (signal?.aborted) return null; - try { - const resp = await fetchFn( - `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(headSha)}`, - { headers, signal }, - ); - if (!resp.ok) return null; - return (await resp.json()) as CommitResponse; - } catch { - return null; - } + return fetchGithubJson( + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(headSha)}`, + headers, + fetchFn, + signal, + options, + ); } /** Fetch one bounded page of the repo's recent commits, optionally filtered to a single author, and report @@ -73,21 +103,19 @@ export async function hasVerifiedHistory( fetchFn: typeof fetch, author?: string, signal?: AbortSignal, + options: Pick = {}, ): Promise { if (signal?.aborted) return null; const authorQuery = author ? `author=${encodeURIComponent(author)}&` : ""; - try { - const resp = await fetchFn( - `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits?${authorQuery}per_page=${HISTORY_PER_PAGE}`, - { headers, signal }, - ); - if (!resp.ok) return null; - const commits = (await resp.json()) as HistoryCommit[]; - if (!Array.isArray(commits)) return null; - return commits.some((c) => c.commit?.verification?.verified === true); - } catch { - return null; - } + const commits = await fetchGithubJson( + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits?${authorQuery}per_page=${HISTORY_PER_PAGE}`, + headers, + fetchFn, + signal, + options, + ); + if (!Array.isArray(commits)) return null; + return commits.some((c) => c.commit?.verification?.verified === true); } /** Analyzer entrypoint: inspect the head commit's signature + author provenance. Fail-safe — returns no finding @@ -116,6 +144,7 @@ export async function scanCommitSignature( headers, fetchFn, options.signal, + options, ); if (!head?.commit) return []; @@ -142,6 +171,7 @@ export async function scanCommitSignature( fetchFn, authorLogin, options.signal, + options, ); if (authorVerified === false && !options.signal?.aborted) { const repoVerified = await hasVerifiedHistory( @@ -151,6 +181,7 @@ export async function scanCommitSignature( fetchFn, undefined, options.signal, + options, ); newCommitter = repoVerified === true; } diff --git a/review-enrichment/src/analyzers/dependency-scan.ts b/review-enrichment/src/analyzers/dependency-scan.ts index f1f87edf53..f14bb0c63c 100644 --- a/review-enrichment/src/analyzers/dependency-scan.ts +++ b/review-enrichment/src/analyzers/dependency-scan.ts @@ -1,8 +1,14 @@ // Dependency-diff + OSV.dev CVE analyzer (#1474). Parses the changed manifests in the PR diff for added/upgraded // dependencies, then queries OSV.dev (free, no key) for known vulnerabilities in the NEW versions. This is the // heavy/external work the no-checkout `claude --print` reviewer cannot do (Bash/WebFetch disallowed, no CVE DB). -import type { EnrichRequest, DependencyFinding, Cve } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + DependencyFinding, + Cve, +} from "../types.js"; import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; export interface DepChange { ecosystem: string; @@ -22,11 +28,14 @@ export interface ScanLimits { } type ExternalCallCache = Pick; +type ExternalFetchContext = Pick; interface ScanOptions { signal?: AbortSignal; limits?: ScanLimits; cache?: ExternalCallCache; + analysis?: ExternalFetchContext; + diagnostics?: AnalyzerDiagnostics; } // Per-manifest line parsers. Each returns [name, version] for a `+`/`-` diff line, or null. Heuristic (line-based, @@ -158,24 +167,8 @@ function fixedOf(vuln: OsvVuln): string | null { return null; } -/** Query OSV.dev for vulnerabilities affecting a specific package version. Best-effort: returns [] on any error. */ -export async function queryOsv( - ecosystem: string, - name: string, - version: string, - fetchImpl: typeof fetch = fetch, - signal?: AbortSignal, -): Promise { - if (signal?.aborted) return []; - const response = await fetchImpl("https://api.osv.dev/v1/query", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ package: { name, ecosystem }, version }), - signal, - }); - if (!response.ok) return []; - const data = (await response.json()) as { vulns?: OsvVuln[] }; - return (data.vulns ?? []).map((vuln) => ({ +function mapOsvVulns(vulns: OsvVuln[] | undefined): Cve[] { + return (vulns ?? []).map((vuln) => ({ id: vuln.id, severity: severityOf(vuln), summary: (vuln.summary ?? vuln.details ?? "") @@ -185,6 +178,97 @@ export async function queryOsv( })); } +async function queryOsvWithAnalysis( + change: DepChange, + fetchImpl: typeof fetch, + options: ScanOptions, +): Promise { + if (options.signal?.aborted) return []; + const body = JSON.stringify({ + package: { name: change.package, ecosystem: change.ecosystem }, + version: change.to, + }); + const response = await options.analysis!.fetchJson<{ vulns?: OsvVuln[] }>( + "https://api.osv.dev/v1/query", + { + endpointCategory: "osv-query", + method: "POST", + headers: { "content-type": "application/json" }, + body, + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "dependency", + subcall: "osv-query", + maxBytes: 512 * 1024, + maxCallsPerCategory: options.limits?.maxDependencyQueries, + }, + ); + if (!response.ok) return []; + return mapOsvVulns(response.data.vulns); +} + +/* Legacy direct path kept for tests and injected callers that do not have request context. */ +async function queryOsvDirect( + ecosystem: string, + name: string, + version: string, + fetchImpl: typeof fetch, + signal?: AbortSignal, + diagnostics?: AnalyzerDiagnostics, +): Promise { + if (signal?.aborted) return []; + const response = await boundedFetchJson<{ vulns?: OsvVuln[] }>( + "https://api.osv.dev/v1/query", + { + endpointCategory: "osv-query", + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ package: { name, ecosystem }, version }), + signal, + fetchImpl, + diagnostics, + phase: "dependency", + subcall: "osv-query", + maxBytes: 512 * 1024, + }, + ); + if (!response.ok) return []; + return mapOsvVulns(response.data.vulns); +} + +/** Query OSV.dev through a request-scoped context when available. */ +async function queryOsvBounded( + change: DepChange, + fetchImpl: typeof fetch, + options: ScanOptions, +): Promise { + if (options.analysis) return queryOsvWithAnalysis(change, fetchImpl, options); + return queryOsvDirect( + change.ecosystem, + change.package, + change.to, + fetchImpl, + options.signal, + options.diagnostics, + ); +} + +/* + * queryOsv remains exported for existing direct unit tests. It intentionally delegates to the bounded direct path + * so even injected callers get timeout, byte-cap, and safe diagnostic behavior without request-cache context. + */ +export async function queryOsv( + ecosystem: string, + name: string, + version: string, + fetchImpl: typeof fetch = fetch, + signal?: AbortSignal, + diagnostics?: AnalyzerDiagnostics, +): Promise { + return queryOsvDirect(ecosystem, name, version, fetchImpl, signal, diagnostics); +} + function osvCacheKey(change: DepChange): string { return `${change.ecosystem}:${change.package}:${change.to}`; } @@ -194,14 +278,7 @@ async function queryOsvForChange( fetchImpl: typeof fetch, options: ScanOptions, ): Promise { - const load = () => - queryOsv( - change.ecosystem, - change.package, - change.to, - fetchImpl, - options.signal, - ); + const load = () => queryOsvBounded(change, fetchImpl, options); return options.cache ? options.cache.cachedExternalCall("osv", osvCacheKey(change), load) : load(); diff --git a/review-enrichment/src/analyzers/dependency/descriptor.ts b/review-enrichment/src/analyzers/dependency/descriptor.ts index 78e73bc445..0ca408a391 100644 --- a/review-enrichment/src/analyzers/dependency/descriptor.ts +++ b/review-enrichment/src/analyzers/dependency/descriptor.ts @@ -26,14 +26,15 @@ export const dependencyAnalyzer: AnalyzerDescriptor<"dependency"> = { notes: "Manifest-only by design; use lockfileDrift for transitive lockfile changes.", }, - run: (_req, { signal, analysis }) => + run: (_req, { signal, analysis, diagnostics }) => scanDependencyChanges( analysis.dependencyChanges(DEPENDENCY_LIMITS), fetch, { signal, limits: DEPENDENCY_LIMITS, - cache: analysis, + analysis, + diagnostics, }, ), render: (deps, { safeCodeSpan, promptText }) => { diff --git a/review-enrichment/src/analyzers/eol-check.ts b/review-enrichment/src/analyzers/eol-check.ts index d6ed3af9c4..cbb076f921 100644 --- a/review-enrichment/src/analyzers/eol-check.ts +++ b/review-enrichment/src/analyzers/eol-check.ts @@ -1,7 +1,13 @@ // End-of-life runtime regression analyzer (#1504). Parses runtime/base-image/engine version pins a PR changes // (Dockerfile FROM, .nvmrc, go.mod) and checks endoflife.date (free, no key) — flagging a pin onto a release that // is already past end-of-support or goes EOL within 90 days. The no-checkout reviewer has no EOL calendar; this does. -import type { EnrichRequest, EolFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + EolFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; // Docker image / source → endoflife.date product slug. const DOCKER_PRODUCT: Record = { @@ -21,6 +27,12 @@ interface VersionPin { version: string; } +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + const MAX_EOL_FILES = 40; const MAX_EOL_PATCH_LINES = 1_000; const MAX_EOL_PINS = 80; @@ -109,13 +121,23 @@ function eolStatus( async function fetchCycles( product: string, fetchImpl: typeof fetch, + options: ScanOptions = {}, ): Promise { - const response = await fetchImpl( - `https://endoflife.date/api/${product}.json`, - ); - if (!response.ok) return null; - const data = (await response.json()) as Cycle[]; - return Array.isArray(data) ? data : null; + const url = `https://endoflife.date/api/${product}.json`; + const fetchOptions = { + endpointCategory: "endoflife", + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "eol", + subcall: "endoflife", + maxBytes: 256 * 1024, + maxCallsPerCategory: MAX_EOL_PINS, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + return response.ok && Array.isArray(response.data) ? response.data : null; } /** Analyzer entrypoint: changed runtime pins → endoflife.date → only the EOL / EOL-soon ones. `now` injectable. */ @@ -123,6 +145,7 @@ export async function scanEol( req: EnrichRequest, fetchImpl: typeof fetch = fetch, now: number = Date.now(), + options: ScanOptions = {}, ): Promise { const findings: EolFinding[] = []; const seen = new Set(); @@ -132,7 +155,7 @@ export async function scanEol( if (seen.has(key)) continue; seen.add(key); if (!cyclesByProduct.has(pin.product)) - cyclesByProduct.set(pin.product, await fetchCycles(pin.product, fetchImpl)); + cyclesByProduct.set(pin.product, await fetchCycles(pin.product, fetchImpl, options)); const cycles = cyclesByProduct.get(pin.product); if (!cycles) continue; const cycle = matchCycle(cycles, pin.version); diff --git a/review-enrichment/src/analyzers/heavy-dependency.ts b/review-enrichment/src/analyzers/heavy-dependency.ts index 7688706512..d8e1eada0d 100644 --- a/review-enrichment/src/analyzers/heavy-dependency.ts +++ b/review-enrichment/src/analyzers/heavy-dependency.ts @@ -1,8 +1,14 @@ // Heavy-dependency-for-trivial-use analyzer (#1505). For each newly-added/upgraded npm dependency, count direct // import/require usage in the PR's added lines and fetch package weight metadata. Flag only when the package is // both materially heavy and used trivially, so the review brief can ask whether a local helper/native API would do. -import type { EnrichRequest, HeavyDependencyFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + HeavyDependencyFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; +import { boundedFetchJson } from "../external-fetch.js"; const MAX_WEIGHT_LOOKUPS = 20; const MAX_FINDINGS = 15; @@ -31,6 +37,8 @@ export interface PackageWeight { interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } function isSafeNpmPackageVersion(name: string, version: string): boolean { @@ -132,21 +140,37 @@ export async function queryPackageWeight( version: string, fetchImpl: typeof fetch = fetch, signal?: AbortSignal, + options: Pick = {}, ): Promise { if (signal?.aborted) return null; try { const packageSpec = encodeURIComponent(`${pkg}@${version}`); - const response = await fetchImpl( - `https://bundlephobia.com/api/size?package=${packageSpec}`, - { signal }, - ); - if (!response.ok) return null; - const data = (await response.json()) as { - installSize?: unknown; - size?: unknown; - gzip?: unknown; - dependencyCount?: unknown; + const url = `https://bundlephobia.com/api/size?package=${packageSpec}`; + const fetchOptions = { + endpointCategory: "bundlephobia-size", + signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "heavy-dependency", + subcall: "bundlephobia-size", + maxBytes: 256 * 1024, + maxCallsPerCategory: MAX_WEIGHT_LOOKUPS, }; + const response = options.analysis + ? await options.analysis.fetchJson<{ + installSize?: unknown; + size?: unknown; + gzip?: unknown; + dependencyCount?: unknown; + }>(url, fetchOptions) + : await boundedFetchJson<{ + installSize?: unknown; + size?: unknown; + gzip?: unknown; + dependencyCount?: unknown; + }>(url, fetchOptions); + if (!response.ok) return null; + const data = response.data; return { installSizeBytes: numberOrNull(data.installSize), bundleSizeBytes: numberOrNull(data.size), @@ -203,6 +227,7 @@ export async function scanHeavyDependencies( change.to, fetchImpl, options.signal, + options, ); if (!weight || !isHeavyPackageWeight(weight)) continue; diff --git a/review-enrichment/src/analyzers/history.ts b/review-enrichment/src/analyzers/history.ts index 92a74c5e82..5081744fa4 100644 --- a/review-enrichment/src/analyzers/history.ts +++ b/review-enrichment/src/analyzers/history.ts @@ -10,6 +10,8 @@ // 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 { AnalyzerDiagnostics, EnrichRequest, HistoryFinding } from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; const GITHUB_API = "https://api.github.com"; const GITHUB_API_VERSION = "2022-11-28"; @@ -46,9 +48,14 @@ interface ScanOptions { timeoutMs?: number; githubSubcallTimeoutMs?: number; diagnostics?: AnalyzerDiagnostics; + analysis?: Pick; } -type GithubEndpointCategory = "search_issues" | "user" | "commits_by_path" | "commit_pulls"; +type GithubEndpointCategory = + | "github-search" + | "github-users" + | "github-commits" + | "github-commit-pulls"; function markPartial(options: ScanOptions, reason: string, captureDegradation = false): void { const diagnostics = options.diagnostics; @@ -84,43 +91,36 @@ function hasResponseBudget(options: ScanOptions): boolean { return remainingMs(options) > HISTORY_RESPONSE_RESERVE_MS; } -function startGithubSubcall( - options: ScanOptions, - category: GithubEndpointCategory, -): { signal: AbortSignal; cleanup: () => void } | null { - const diagnostics = options.diagnostics; - if (diagnostics) { - diagnostics.githubEndpointCategory = category; - diagnostics.subcall = category; - } - if (category === "commits_by_path") addCount(diagnostics, "fileLookupCount"); - if (category === "commit_pulls") addCount(diagnostics, "prLookupCount"); +function githubSubcallTimeoutMs(options: ScanOptions): number | null { if (!hasResponseBudget(options)) { markPartial(options, options.signal?.aborted ? "history_aborted" : "history_budget_exhausted", true); return null; } - - const controller = new AbortController(); - const parent = options.signal; - const abortFromParent = () => controller.abort(); - if (parent) parent.addEventListener("abort", abortFromParent, { once: true }); - const remaining = remainingMs(options); - const timeoutMs = Math.max( + return Math.max( 1, Math.min( options.githubSubcallTimeoutMs ?? GITHUB_SUBCALL_TIMEOUT_MS, Number.isFinite(remaining) ? Math.max(1, remaining - HISTORY_RESPONSE_RESERVE_MS) : GITHUB_SUBCALL_TIMEOUT_MS, ), ); - const timer = setTimeout(() => controller.abort(), timeoutMs); - return { - signal: controller.signal, - cleanup: () => { - clearTimeout(timer); - if (parent) parent.removeEventListener("abort", abortFromParent); - }, - }; +} + +function prepareGithubSubcall( + options: ScanOptions, + category: GithubEndpointCategory, +): number | null { + const diagnostics = options.diagnostics; + if (diagnostics) { + diagnostics.githubEndpointCategory = category; + diagnostics.endpointCategory = category; + diagnostics.subcall = category; + } + const timeoutMs = githubSubcallTimeoutMs(options); + if (timeoutMs === null) return null; + if (category === "github-commits") addCount(diagnostics, "fileLookupCount"); + if (category === "github-commit-pulls") addCount(diagnostics, "prLookupCount"); + return timeoutMs; } async function fetchGithubJson( @@ -130,21 +130,36 @@ async function fetchGithubJson( options: ScanOptions, category: GithubEndpointCategory, ): Promise { - const subcall = startGithubSubcall(options, category); - if (!subcall) return null; - try { - const res = await fetchImpl(url, { headers: githubHeaders(token), signal: subcall.signal }); - if (!res.ok) { - markPartial(options, `github_${category}_http_${res.status}`, res.status === 403 || res.status === 429); - return null; + const timeoutMs = prepareGithubSubcall(options, category); + if (timeoutMs === null) return null; + const fetchOptions = { + endpointCategory: category, + headers: githubHeaders(token), + signal: options.signal, + timeoutMs, + fetchImpl, + diagnostics: options.diagnostics, + phase: options.diagnostics?.phase, + subcall: category, + maxBytes: 512 * 1024, + maxCallsPerCategory: category === "github-commit-pulls" ? MAX_PR_LOOKUPS : undefined, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, fetchOptions) + : await boundedFetchJson(url, fetchOptions); + if (!response.ok) { + if (response.reason === "http_error") { + markPartial( + options, + `${category}_http_${response.status ?? "unknown"}`, + response.status === 403 || response.status === 429, + ); + } else { + markPartial(options, `${category}_${response.reason}`, true); } - return (await res.json()) as T; - } catch { - markPartial(options, subcall.signal.aborted ? "github_subcall_aborted" : "github_subcall_failed", true); return null; - } finally { - subcall.cleanup(); } + return response.data; } /** Parse `owner/repo`, rejecting anything that isn't exactly two safe segments (no traversal, no extra slashes) so a @@ -248,7 +263,7 @@ async function fetchSearchCount( ): Promise { try { const url = `${GITHUB_API}/search/issues?q=${encodeURIComponent(query)}&per_page=1`; - const json = await fetchGithubJson<{ total_count?: number }>(url, token, fetchImpl, options, "search_issues"); + const json = await fetchGithubJson<{ total_count?: number }>(url, token, fetchImpl, options, "github-search"); if (!json) return null; return typeof json.total_count === "number" ? json.total_count : null; } catch { @@ -266,7 +281,7 @@ async function fetchAccountAgeDays( ): Promise { try { const url = `${GITHUB_API}/users/${encodeURIComponent(login)}`; - const json = await fetchGithubJson<{ created_at?: string }>(url, token, fetchImpl, options, "user"); + const json = await fetchGithubJson<{ created_at?: string }>(url, token, fetchImpl, options, "github-users"); if (!json) return null; if (!json.created_at) return null; const created = Date.parse(json.created_at); @@ -323,7 +338,7 @@ async function fetchCommitsForPath( const json = await fetchGithubJson>(url, token, fetchImpl, options, "commits_by_path"); + }>>(url, token, fetchImpl, options, "github-commits"); if (!json) return null; if (!Array.isArray(json)) return null; const out: Array<{ sha: string; message: string }> = []; @@ -351,7 +366,7 @@ async function fetchPullsForCommit( if (!SHA_RE.test(sha)) return []; try { const url = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(sha)}/pulls`; - const json = await fetchGithubJson>(url, token, fetchImpl, options, "commit_pulls"); + const json = await fetchGithubJson>(url, token, fetchImpl, options, "github-commit-pulls"); if (!json) return null; if (!Array.isArray(json)) return null; const out: Array<{ number: number; title: string }> = []; diff --git a/review-enrichment/src/analyzers/install-scripts.ts b/review-enrichment/src/analyzers/install-scripts.ts index 6937aab462..0816079bdf 100644 --- a/review-enrichment/src/analyzers/install-scripts.ts +++ b/review-enrichment/src/analyzers/install-scripts.ts @@ -3,14 +3,27 @@ // vector (a script runs on `npm install`, before any code review of the package's source). The shipped CVE scan // misses this entirely; the no-checkout reviewer can't fetch a packument. Public-safe output: package@version + the // hook names + publish date (NOT the script body, to keep the brief compact and non-executable). -import type { EnrichRequest, InstallScriptFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + InstallScriptFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; +import { boundedFetchJson } from "../external-fetch.js"; const INSTALL_HOOKS = ["preinstall", "install", "postinstall"]; const NPM_PACKAGE_RE = /^(?:@[a-z0-9][a-z0-9._-]*\/[a-z0-9][a-z0-9._-]*|[a-z0-9][a-z0-9._-]*)$/; const SEMVER_RE = /^(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/; +const MAX_PACKUMENT_LOOKUPS = 25; + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} function isSafeNpmChange(name: string, version: string): boolean { return NPM_PACKAGE_RE.test(name) && SEMVER_RE.test(version); @@ -20,22 +33,40 @@ function isSafeNpmChange(name: string, version: string): boolean { export async function scanInstallScripts( req: EnrichRequest, fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, ): Promise { const findings: InstallScriptFinding[] = []; + let lookups = 0; for (const change of extractDependencyChanges(req.files ?? [])) { + if (options.signal?.aborted || lookups >= MAX_PACKUMENT_LOOKUPS) break; if ( change.ecosystem !== "npm" || !isSafeNpmChange(change.package, change.to) ) continue; - const response = await fetchImpl( - `https://registry.npmjs.org/${encodeURIComponent(change.package)}`, - ); - if (!response.ok) continue; - const data = (await response.json()) as { - versions?: Record }>; - time?: Record; + lookups += 1; + const url = `https://registry.npmjs.org/${encodeURIComponent(change.package)}`; + const fetchOptions = { + endpointCategory: "npm-packument", + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "install-script", + subcall: "npm-packument", + maxBytes: 1024 * 1024, + maxCallsPerCategory: MAX_PACKUMENT_LOOKUPS, }; + const response = options.analysis + ? await options.analysis.fetchJson<{ + versions?: Record }>; + time?: Record; + }>(url, fetchOptions) + : await boundedFetchJson<{ + versions?: Record }>; + time?: Record; + }>(url, fetchOptions); + if (!response.ok) continue; + const data = response.data; const scripts = data.versions?.[change.to]?.scripts ?? {}; const hooks = INSTALL_HOOKS.filter( (hook) => typeof scripts[hook] === "string", diff --git a/review-enrichment/src/analyzers/license-check.ts b/review-enrichment/src/analyzers/license-check.ts index 2bb61923b6..d79ae38205 100644 --- a/review-enrichment/src/analyzers/license-check.ts +++ b/review-enrichment/src/analyzers/license-check.ts @@ -2,8 +2,14 @@ // deps.dev (free, no key, covers npm/PyPI/Go) and flags the ones a maintainer should eyeball: copyleft (may be // incompatible with a permissive project) or unresolved/unknown. Permissive licenses (MIT/BSD/Apache/…) are not // flagged. The no-checkout reviewer can't resolve a dependency's published license — this can. -import type { EnrichRequest, LicenseFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + LicenseFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; +import { boundedFetchJson } from "../external-fetch.js"; // REES ecosystem label → deps.dev system path segment. const SYSTEM: Record = { npm: "npm", PyPI: "pypi", Go: "go" }; @@ -13,6 +19,12 @@ const COPYLEFT = /^(A?GPL|LGPL|MPL|EPL|CDDL|EUPL|OSL|SSPL|CPAL|CECILL)/i; const MAX_LICENSE_LOOKUPS = 25; const LICENSE_LOOKUP_TIMEOUT_MS = 1500; +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + function classify(licenses: string[]): LicenseFinding["classification"] | null { const resolved = licenses.filter( (license) => license && !/^NOASSERTION$/i.test(license), @@ -28,26 +40,32 @@ async function fetchLicenses( name: string, version: string, fetchImpl: typeof fetch, + options: ScanOptions, ): Promise { const url = `https://api.deps.dev/v3/systems/${system}/packages/${encodeURIComponent(name)}/versions/${encodeURIComponent(version)}`; - const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), LICENSE_LOOKUP_TIMEOUT_MS); - try { - const response = await fetchImpl(url, { signal: controller.signal }); - if (!response.ok) return null; - const data = (await response.json()) as { licenses?: string[] }; - return Array.isArray(data.licenses) ? data.licenses : []; - } catch { - return null; - } finally { - clearTimeout(timer); - } + const fetchOptions = { + endpointCategory: "deps-dev", + signal: options.signal, + timeoutMs: LICENSE_LOOKUP_TIMEOUT_MS, + fetchImpl, + diagnostics: options.diagnostics, + phase: "license", + subcall: "deps-dev", + maxBytes: 256 * 1024, + maxCallsPerCategory: MAX_LICENSE_LOOKUPS, + }; + const response = options.analysis + ? await options.analysis.fetchJson<{ licenses?: string[] }>(url, fetchOptions) + : await boundedFetchJson<{ licenses?: string[] }>(url, fetchOptions); + if (!response.ok) return null; + return Array.isArray(response.data.licenses) ? response.data.licenses : []; } /** Analyzer entrypoint: changed deps → deps.dev license → only the copyleft/unknown ones. */ export async function scanLicenses( req: EnrichRequest, fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, ): Promise { const findings: LicenseFinding[] = []; const changes = extractDependencyChanges(req.files ?? []).slice( @@ -62,6 +80,7 @@ export async function scanLicenses( change.package, change.to, fetchImpl, + options, ); if (licenses === null) continue; // resolution failed — don't false-flag const classification = classify(licenses); diff --git a/review-enrichment/src/analyzers/lockfile-drift.ts b/review-enrichment/src/analyzers/lockfile-drift.ts index 2695eaddc8..443dce626a 100644 --- a/review-enrichment/src/analyzers/lockfile-drift.ts +++ b/review-enrichment/src/analyzers/lockfile-drift.ts @@ -2,11 +2,14 @@ // lockfile changes, where the top-level manifest diff does not name the package. This catches transitive pins and // downgraded resolved versions that the manifest-only dependency analyzer cannot see. import type { + AnalyzerDiagnostics, Cve, EnrichRequest, LockfileDriftFinding, } from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; +import { boundedFetchJson } from "../external-fetch.js"; interface LockfileChange { file: string; @@ -26,6 +29,8 @@ interface ScanLimits { interface ScanOptions { signal?: AbortSignal; limits?: ScanLimits; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } interface PatchLine { @@ -363,31 +368,42 @@ export async function queryOsvBatch( changes: LockfileChange[], fetchImpl: typeof fetch = fetch, signal?: AbortSignal, + options: Pick = {}, ): Promise> { const results = new Map(); if (!changes.length || signal?.aborted) return results; - try { - const response = await fetchImpl("https://api.osv.dev/v1/querybatch", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ - queries: changes.map((change) => ({ - package: { name: change.package, ecosystem: change.ecosystem }, - version: change.to, - })), - }), - signal, - }); - if (!response.ok) return results; - const data = (await response.json()) as { + const fetchOptions = { + endpointCategory: "osv-querybatch", + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + queries: changes.map((change) => ({ + package: { name: change.package, ecosystem: change.ecosystem }, + version: change.to, + })), + }), + signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "lockfile-drift", + subcall: "osv-querybatch", + maxBytes: 1024 * 1024, + maxCallsPerCategory: 1, + }; + const response = options.analysis + ? await options.analysis.fetchJson<{ + results?: Array<{ vulns?: OsvVuln[] }>; + }>("https://api.osv.dev/v1/querybatch", fetchOptions) + : await boundedFetchJson<{ results?: Array<{ vulns?: OsvVuln[] }>; - }; - changes.forEach((change, index) => { - results.set(`${change.ecosystem}::${change.package}@${change.to}`, toCves(data.results?.[index]?.vulns)); - }); - } catch { - return results; - } + }>("https://api.osv.dev/v1/querybatch", fetchOptions); + if (!response.ok) return results; + changes.forEach((change, index) => { + results.set( + `${change.ecosystem}::${change.package}@${change.to}`, + toCves(response.data.results?.[index]?.vulns), + ); + }); return results; } @@ -401,7 +417,7 @@ export async function scanLockfileDrift( 0, options.limits?.maxOsvQueries ?? MAX_OSV_QUERIES, ); - const cvesByKey = await queryOsvBatch(changes, fetchImpl, options.signal); + const cvesByKey = await queryOsvBatch(changes, fetchImpl, options.signal, options); const findings: LockfileDriftFinding[] = []; for (const change of changes) { const cves = cvesByKey.get(`${change.ecosystem}::${change.package}@${change.to}`) ?? []; diff --git a/review-enrichment/src/analyzers/native-build.ts b/review-enrichment/src/analyzers/native-build.ts index 68258e9f07..aad03add47 100644 --- a/review-enrichment/src/analyzers/native-build.ts +++ b/review-enrichment/src/analyzers/native-build.ts @@ -3,8 +3,14 @@ // (node-gyp / gypfile) on install, or a PyPI release that ships no prebuilt wheel (sdist-only) so pip compiles from // source. Both are a hidden CI cold-start cost and a frequent cross-platform breakage source. Factual signals from // the registry metadata only — the no-checkout reviewer can fetch neither. Reports package@version + the property. -import type { EnrichRequest, NativeBuildFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + NativeBuildFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; +import { boundedFetchJson } from "../external-fetch.js"; const MAX_QUERIES = 25; const MAX_REGISTRY_JSON_BYTES = 2 * 1024 * 1024; @@ -35,6 +41,8 @@ interface ScanLimits { interface ScanOptions { signal?: AbortSignal; limits?: ScanLimits; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } /** npm packument version metadata, the subset that signals a native build. */ @@ -73,52 +81,24 @@ export function pypiSdistOnly(urls: PypiUrl[]): boolean { async function fetchJson( fetchImpl: typeof fetch, url: string, - signal?: AbortSignal, + options: ScanOptions, + endpointCategory: "npm-packument" | "pypi-json", ): Promise { - if (signal?.aborted) return null; - try { - const response = await fetchImpl(url, { signal }); - if (!response.ok) return null; - const text = await readJsonText(response); - return text === null ? null : JSON.parse(text); - } catch { - return null; - } -} - -async function readJsonText(response: Response): Promise { - const contentLength = response.headers.get("content-length"); - if (contentLength !== null) { - const parsedLength = Number.parseInt(contentLength, 10); - if (Number.isFinite(parsedLength) && parsedLength > MAX_REGISTRY_JSON_BYTES) return null; - } - - const reader = response.body?.getReader(); - if (!reader) { - const buffer = await response.arrayBuffer(); - if (buffer.byteLength > MAX_REGISTRY_JSON_BYTES) return null; - return new TextDecoder().decode(buffer); - } - - const decoder = new TextDecoder(); - let totalBytes = 0; - let text = ""; - try { - while (true) { - const { done, value } = await reader.read(); - if (done) break; - totalBytes += value.byteLength; - if (totalBytes > MAX_REGISTRY_JSON_BYTES) { - await reader.cancel(); - return null; - } - text += decoder.decode(value, { stream: true }); - } - text += decoder.decode(); - return text; - } finally { - reader.releaseLock(); - } + if (options.signal?.aborted) return null; + const boundedOptions = { + endpointCategory, + signal: options.signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "native-build", + subcall: endpointCategory, + maxBytes: MAX_REGISTRY_JSON_BYTES, + maxCallsPerCategory: options.limits?.maxQueries ?? MAX_QUERIES, + }; + const response = options.analysis + ? await options.analysis.fetchJson(url, boundedOptions) + : await boundedFetchJson(url, boundedOptions); + return response.ok ? response.data : null; } /** Analyzer entrypoint: added/changed deps → registry metadata → only the versions with a native-build install cost. */ @@ -140,7 +120,8 @@ export async function scanNativeBuild( const data = (await fetchJson( fetchImpl, `https://registry.npmjs.org/${encodeURIComponent(change.package)}`, - options.signal, + options, + "npm-packument", )) as { versions?: Record } | null; const meta = data?.versions?.[change.to]; const native = meta && npmNativeBuild(meta); @@ -159,7 +140,8 @@ export async function scanNativeBuild( const data = (await fetchJson( fetchImpl, `https://pypi.org/pypi/${encodeURIComponent(change.package)}/${encodeURIComponent(change.to)}/json`, - options.signal, + options, + "pypi-json", )) as { urls?: PypiUrl[] } | null; if (data && pypiSdistOnly(data.urls ?? [])) { findings.push({ diff --git a/review-enrichment/src/analyzers/provenance.ts b/review-enrichment/src/analyzers/provenance.ts index 3d231db323..eb74bda6c1 100644 --- a/review-enrichment/src/analyzers/provenance.ts +++ b/review-enrichment/src/analyzers/provenance.ts @@ -5,8 +5,14 @@ // reviewer cannot detect. // 2. Binary files and vendored/minified code committed by the PR — artifacts without an auditable source // the reviewer can inspect. Detected purely by path pattern + extension (no network). -import type { EnrichRequest, ProvenanceFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + ProvenanceFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; +import { boundedFetchJson } from "../external-fetch.js"; const MAX_ATTESTATION_CHECKS = 20; // bound network round-trips const MAX_FINDINGS = 30; // keep the brief bounded @@ -51,17 +57,26 @@ export async function hasNpmAttestation( version: string, fetchImpl: typeof fetch, signal?: AbortSignal, + options: Pick = {}, ): Promise { if (signal?.aborted) return true; try { - const res = await fetchImpl( - `https://registry.npmjs.org/-/npm/v1/attestations/${encodeURIComponent(`${pkg}@${version}`)}`, - { signal }, - ); - if (res.status === 404) return false; // unambiguously absent - if (!res.ok) return true; // other registry error → fail-safe - const data = (await res.json()) as { attestations?: unknown[] }; - return (data.attestations?.length ?? 0) > 0; + const url = `https://registry.npmjs.org/-/npm/v1/attestations/${encodeURIComponent(`${pkg}@${version}`)}`; + const fetchOptions = { + endpointCategory: "npm-attestations", + signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "provenance", + subcall: "npm-attestations", + maxBytes: 256 * 1024, + maxCallsPerCategory: MAX_ATTESTATION_CHECKS, + }; + const res = options.analysis + ? await options.analysis.fetchJson<{ attestations?: unknown[] }>(url, fetchOptions) + : await boundedFetchJson<{ attestations?: unknown[] }>(url, fetchOptions); + if (!res.ok) return res.status === 404 ? false : true; // other registry error → fail-safe + return (res.data.attestations?.length ?? 0) > 0; } catch { return true; // network / parse error → fail-safe } @@ -91,21 +106,31 @@ export async function hasPypiProvenance( version: string, fetchImpl: typeof fetch, signal?: AbortSignal, + options: Pick = {}, ): Promise { if (signal?.aborted) return true; try { - const res = await fetchImpl( - `https://pypi.org/simple/${encodeURIComponent(pkg.toLowerCase())}/`, - { - signal, - headers: { Accept: "application/vnd.pypi.simple.v1+json" }, - }, - ); - if (!res.ok) return true; // fail-safe - const data = (await res.json()) as { - files?: Array<{ filename: string; provenance?: string }>; + const url = `https://pypi.org/simple/${encodeURIComponent(pkg.toLowerCase())}/`; + const fetchOptions = { + endpointCategory: "pypi-simple", + signal, + headers: { Accept: "application/vnd.pypi.simple.v1+json" }, + fetchImpl, + diagnostics: options.diagnostics, + phase: "provenance", + subcall: "pypi-simple", + maxBytes: 1024 * 1024, + maxCallsPerCategory: MAX_ATTESTATION_CHECKS, }; - const versionFiles = (data.files ?? []).filter((f) => + const res = options.analysis + ? await options.analysis.fetchJson<{ + files?: Array<{ filename: string; provenance?: string }>; + }>(url, fetchOptions) + : await boundedFetchJson<{ + files?: Array<{ filename: string; provenance?: string }>; + }>(url, fetchOptions); + if (!res.ok) return true; // fail-safe + const versionFiles = (res.data.files ?? []).filter((f) => matchesPypiVersion(f.filename, pkg, version), ); if (!versionFiles.length) return true; // can't determine → don't flag @@ -117,6 +142,8 @@ export async function hasPypiProvenance( interface ScanOptions { signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } /** Analyzer entrypoint: scan for newly-added deps lacking provenance attestations + binary/vendored files. */ @@ -154,6 +181,7 @@ export async function scanProvenance( change.to, fetchImpl, options.signal, + options, ); } else if (change.ecosystem === "PyPI") { attested = await hasPypiProvenance( @@ -161,6 +189,7 @@ export async function scanProvenance( change.to, fetchImpl, options.signal, + options, ); } else { continue; // Go and other ecosystems — no provenance API to check yet diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 767e21cafe..9d89670555 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -56,7 +56,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "Useful when a PR does not touch a top-level manifest but changes resolved dependency pins.", }, - run: (req, { signal }) => scanLockfileDrift(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanLockfileDrift(req, fetch, { signal, analysis, diagnostics }), }), secretAnalyzer, descriptor({ @@ -75,7 +76,8 @@ export const ANALYZER_DESCRIPTORS = [ network: "Calls deps.dev. No GitHub token required.", notes: "Permissive and otherwise-known licenses are intentionally silent.", }, - run: (req) => scanLicenses(req), + run: (req, { signal, analysis, diagnostics }) => + scanLicenses(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "installScript", @@ -92,7 +94,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "The script body is not returned, which keeps the brief compact and non-executable.", }, - run: (req) => scanInstallScripts(req), + run: (req, { signal, analysis, diagnostics }) => + scanInstallScripts(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "heavyDependency", @@ -112,7 +115,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "Only reports packages with trivial direct usage so the finding stays actionable.", }, - run: (req, { signal }) => scanHeavyDependencies(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanHeavyDependencies(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "actionPin", @@ -146,7 +150,8 @@ export const ANALYZER_DESCRIPTORS = [ network: "Calls endoflife.date. No GitHub token required.", notes: "Only changed pins are checked; existing old runtimes outside the PR are not reported.", }, - run: (req) => scanEol(req), + run: (req, { signal, analysis, diagnostics }) => + scanEol(req, fetch, Date.now(), { signal, analysis, diagnostics }), }), descriptor({ name: "redos", @@ -183,7 +188,8 @@ export const ANALYZER_DESCRIPTORS = [ "Calls npm and PyPI attestation/provenance endpoints for package checks. Path checks are local.", notes: "Network failures fail safe; it flags only confident no-attestation responses.", }, - run: (req, { signal }) => scanProvenance(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanProvenance(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "codeowners", @@ -207,7 +213,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "Leave REES_FORWARD_GITHUB_TOKEN unset/false to disable token forwarding; this analyzer will then skip when it cannot read CODEOWNERS.", }, - run: (req, { signal }) => scanCodeowners(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanCodeowners(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "secretLog", @@ -246,7 +253,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "Added asset detection works from headSha. Growth comparison needs baseSha in the enrichment request.", }, - run: (req, { signal }) => scanAssetWeight(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanAssetWeight(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "typosquat", @@ -267,7 +275,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "Scoped npm packages are treated as namespace-protected and are not flagged as typosquats.", }, - run: (req, { signal }) => scanTyposquat(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanTyposquat(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "commitSignature", @@ -286,7 +295,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "Does not expose emails or private identity data; only public GitHub commit facts are surfaced.", }, - run: (req, { signal }) => scanCommitSignature(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanCommitSignature(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "iacMisconfig", @@ -322,7 +332,8 @@ export const ANALYZER_DESCRIPTORS = [ notes: "Registry JSON is capped so large package metadata cannot monopolize REES memory.", }, - run: (req, { signal }) => scanNativeBuild(req, fetch, { signal }), + run: (req, { signal, analysis, diagnostics }) => + scanNativeBuild(req, fetch, { signal, analysis, diagnostics }), }), descriptor({ name: "history", @@ -354,6 +365,7 @@ export const ANALYZER_DESCRIPTORS = [ deadlineMs: context.deadlineMs, timeoutMs: context.timeoutMs, diagnostics: context.diagnostics, + analysis: context.analysis, }), }), descriptor({ diff --git a/review-enrichment/src/analyzers/types.ts b/review-enrichment/src/analyzers/types.ts index 9b6fec5ce9..d94a3c7bd9 100644 --- a/review-enrichment/src/analyzers/types.ts +++ b/review-enrichment/src/analyzers/types.ts @@ -2,6 +2,7 @@ import type { AnalyzerDiagnostics, BriefFindings, EnrichRequest, + ReesProfileName, } from "../types.js"; import type { AnalysisContext } from "../analysis-context.js"; import type { AnalyzerRenderHelpers } from "../render-helpers.js"; @@ -39,6 +40,9 @@ export interface AnalyzerRunContext { timeoutMs: number; startedAtMs: number; deadlineMs: number; + requestDeadlineMs: number; + profile: ReesProfileName; + costClass: AnalyzerCostClass; diagnostics: AnalyzerDiagnostics; analysis: AnalysisContext; } diff --git a/review-enrichment/src/analyzers/typosquat.ts b/review-enrichment/src/analyzers/typosquat.ts index d32b52b8c0..c6266866ba 100644 --- a/review-enrichment/src/analyzers/typosquat.ts +++ b/review-enrichment/src/analyzers/typosquat.ts @@ -3,8 +3,14 @@ // (edit-distance / homoglyph / separator / scope-swap) — a likely typosquat; (2) an unscoped name that is NOT // published on the public registry and is therefore publicly claimable — a dependency-confusion vector. Pure // name analysis runs offline against a bundled popular-package list; the confusion check uses an injected fetch. -import type { EnrichRequest, TyposquatFinding } from "../types.js"; +import type { + AnalyzerDiagnostics, + EnrichRequest, + TyposquatFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import { extractDependencyChanges } from "./dependency-scan.js"; +import { boundedFetchText } from "../external-fetch.js"; const MAX_DEPS = 50; const MAX_CONFUSION_QUERIES = 15; @@ -19,6 +25,8 @@ interface ScanLimits { interface ScanOptions { signal?: AbortSignal; limits?: ScanLimits; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; } // Bundled top popular packages per ecosystem — the high-traffic names typosquatters impersonate. Not exhaustive; @@ -136,17 +144,26 @@ export async function isPublished( name: string, fetchImpl: typeof fetch = fetch, signal?: AbortSignal, + options: Pick = {}, ): Promise { const toUrl = REGISTRY_URL[ecosystem]; if (!toUrl || signal?.aborted) return null; - try { - const response = await fetchImpl(toUrl(name), { signal }); - if (response.status === 404) return false; - if (response.ok) return true; - return null; - } catch { - return null; - } + const endpointCategory = ecosystem === "npm" ? "npm-packument" : "pypi-json"; + const fetchOptions = { + endpointCategory, + signal, + fetchImpl, + diagnostics: options.diagnostics, + phase: "typosquat", + subcall: endpointCategory, + maxBytes: 16 * 1024, + maxCallsPerCategory: options.limits?.maxConfusionQueries ?? MAX_CONFUSION_QUERIES, + }; + const response = options.analysis + ? await options.analysis.fetchText(toUrl(name), fetchOptions) + : await boundedFetchText(toUrl(name), fetchOptions); + if (!response.ok) return response.status === 404 ? false : null; + return true; } /** Analyzer entrypoint: newly-added deps → typosquat near-miss (pure) + dependency-confusion (registry 404). */ @@ -184,7 +201,7 @@ export async function scanTyposquat( const isScoped = dep.package.startsWith("@"); if (!isScoped && confusionQueries < maxConfusion && REGISTRY_URL[dep.ecosystem]) { confusionQueries += 1; - const published = await isPublished(dep.ecosystem, dep.package, fetchImpl, options.signal); + const published = await isPublished(dep.ecosystem, dep.package, fetchImpl, options.signal, options); if (published === false) { findings.push({ ecosystem: dep.ecosystem, diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 301ac71877..0982d4f52d 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -7,10 +7,12 @@ import type { BriefFindings, AnalyzerStatus, AnalyzerDiagnostics, + AnalyzerTelemetry, } from "./types.js"; import type { AnalyzerRegistry, AnalyzerRunContext, + AnalyzerCostClass, } from "./analyzers/types.js"; import { createAnalysisContext, @@ -18,10 +20,19 @@ import { } from "./analysis-context.js"; import { ANALYZERS } from "./analyzers/registry.js"; import { renderBrief } from "./render.js"; +import { + COST_ORDER, + analyzerTimeoutMs, + costClassConcurrency, + planAnalyzers, + shouldStartAnalyzer, + type AnalyzerPlanItem, +} from "./scheduler.js"; import { captureAnalyzerDegradation } from "./sentry.js"; const DEFAULT_ANALYZER_TIMEOUT_MS = 8000; const MIN_ANALYZER_TIMEOUT_MS = 1; +const PUBLIC_PARTIAL_REASON_RE = /^[A-Za-z0-9_.:-]{1,120}$/; interface BuildBriefOptions { requestId?: string; @@ -39,6 +50,11 @@ function runWithTimeout( ms: number, diagnostics: AnalyzerDiagnostics, analysis: AnalysisContext, + meta: { + requestDeadlineMs: number; + profile: AnalyzerRunContext["profile"]; + costClass: AnalyzerCostClass; + }, ): Promise { const controller = new AbortController(); const startedAtMs = Date.now(); @@ -47,6 +63,9 @@ function runWithTimeout( timeoutMs: ms, startedAtMs, deadlineMs: startedAtMs + ms, + requestDeadlineMs: meta.requestDeadlineMs, + profile: meta.profile, + costClass: meta.costClass, diagnostics, analysis, }; @@ -81,6 +100,48 @@ function resultIsPartial(result: unknown): boolean { ); } +async function runWithConcurrency( + items: readonly T[], + limit: number, + run: (item: T) => Promise, +): Promise { + const concurrency = Math.max(1, Math.floor(limit)); + let index = 0; + async function worker(): Promise { + for (;;) { + const item = items[index]; + index += 1; + if (!item) return; + await run(item); + } + } + await Promise.all( + Array.from( + { length: Math.min(concurrency, items.length) }, + () => worker(), + ), + ); +} + +function statusFromDiagnostics( + diagnostics: AnalyzerDiagnostics, + fallback: AnalyzerStatus, +): AnalyzerStatus { + if (diagnostics.partialReason === "analyzer_timeout") return "timeout"; + if (diagnostics.capped || diagnostics.externalFailureReason === "call_cap") return "capped"; + return fallback; +} + +function timeoutStatus(error: unknown, diagnostics: AnalyzerDiagnostics): AnalyzerStatus { + if (error instanceof Error && error.message === "analyzer_timeout") return "timeout"; + return statusFromDiagnostics(diagnostics, "degraded"); +} + +function publicPartialReason(value: string | undefined, fallback: string): string { + if (value && PUBLIC_PARTIAL_REASON_RE.test(value)) return value; + return fallback; +} + function captureDegradation( error: unknown, input: { @@ -90,6 +151,9 @@ function captureDegradation( timeoutMs: number; elapsedMs: number; analyzerStatus: AnalyzerStatus; + profile: string; + costClass?: string; + responseReserveMs?: number; diagnostics: AnalyzerDiagnostics; options: BuildBriefOptions; }, @@ -103,10 +167,16 @@ function captureDegradation( timeoutMs: input.timeoutMs, elapsedMs: input.elapsedMs, analyzerStatus: input.analyzerStatus, + profile: input.profile, + costClass: input.costClass, + responseReserveMs: input.responseReserveMs, partialStatus: input.diagnostics.partialStatus, partialReason: input.diagnostics.partialReason, phase: input.diagnostics.phase, subcall: input.diagnostics.subcall, + endpointCategory: input.diagnostics.endpointCategory, + externalFailureReason: input.diagnostics.externalFailureReason, + externalElapsedMs: input.diagnostics.externalElapsedMs, fileLookupCount: input.diagnostics.fileLookupCount, commitLookupCount: input.diagnostics.commitLookupCount, prLookupCount: input.diagnostics.prLookupCount, @@ -144,91 +214,220 @@ export async function buildBrief( ): Promise { const start = Date.now(); const all = Object.keys(analyzers) as Array; - const requested = Array.isArray(req.analyzers) - ? all.filter((name) => req.analyzers!.includes(name)) - : all; const budgetMs = resolveAnalyzerTimeoutMs(req.budget?.timeoutMs); const analysis = createAnalysisContext(req, { startedAtMs: start, deadlineMs: start + budgetMs, }); + const plan = planAnalyzers(req, analyzers, analysis, { + budgetMs, + startedAtMs: start, + }); const findings: BriefFindings = {}; const analyzerStatus: Record = {}; + const analyzerTelemetry: Record = {}; let partial = false; - await Promise.all( - requested.map(async (name) => { - const analyzerStartedAt = Date.now(); - const diagnostics: AnalyzerDiagnostics = { - partialStatus: "complete", + for (const item of plan.skipped) { + analyzerStatus[item.name] = "skipped"; + analyzerTelemetry[item.name] = { + status: "skipped", + elapsedMs: 0, + costClass: item.descriptor.cost, + skipReason: item.skipReason, + }; + } + + async function runAnalyzer(item: AnalyzerPlanItem): Promise { + const name = item.name; + const analyzerStartedAt = Date.now(); + const diagnostics: AnalyzerDiagnostics = { + partialStatus: "complete", + }; + const remainingMs = plan.executionDeadlineMs - Date.now(); + if (!shouldStartAnalyzer(plan.profile, remainingMs)) { + analyzerStatus[name] = "capped"; + analyzerTelemetry[name] = { + status: "capped", + elapsedMs: Date.now() - analyzerStartedAt, + costClass: item.descriptor.cost, + partialStatus: "partial", + partialReason: "analyzer_budget_exhausted", + capped: true, + }; + partial = true; + analysis.metrics.recordCappedWork("analyzer_budget", 1); + return; + } + const timeoutMs = analyzerTimeoutMs( + plan.profile, + item.descriptor.cost, + remainingMs, + plan.explicitAnalyzers, + ); + if (timeoutMs <= 0) { + analyzerStatus[name] = "capped"; + analyzerTelemetry[name] = { + status: "capped", + elapsedMs: Date.now() - analyzerStartedAt, + timeoutMs, + costClass: item.descriptor.cost, + partialStatus: "partial", + partialReason: "analyzer_budget_exhausted", + capped: true, }; - try { - const analyzer = analyzers[name]; - if (!analyzer) throw new Error("analyzer_unregistered"); - const result = await runWithTimeout( - (context) => analyzer(req, context), - budgetMs, - diagnostics, - analysis, + partial = true; + analysis.metrics.recordCappedWork(`analyzer_${item.descriptor.cost}`, 1); + return; + } + try { + const analyzer = analyzers[name]; + if (!analyzer) throw new Error("analyzer_unregistered"); + const result = await runWithTimeout( + (context) => analyzer(req, context), + timeoutMs, + diagnostics, + analysis, + { + requestDeadlineMs: plan.executionDeadlineMs, + profile: plan.profile, + costClass: item.descriptor.cost, + }, + ); + findings[name] = result as never; + if (resultIsPartial(result) || diagnostics.partialStatus === "partial") { + const status = statusFromDiagnostics(diagnostics, "degraded"); + const partialReason = publicPartialReason( + diagnostics.partialReason, + status === "capped" ? "analyzer_capped" : "analyzer_partial", ); - findings[name] = result as never; - if (resultIsPartial(result)) { - analyzerStatus[name] = "degraded"; - partial = true; - diagnostics.partialStatus = "partial"; - diagnostics.partialReason ??= "analyzer_partial"; - if (diagnostics.captureDegradation) { - attachAnalysisMetrics(diagnostics, analysis); - captureDegradation(new Error(diagnostics.partialReason), { - analyzer: name, - requested, - req, - timeoutMs: budgetMs, - elapsedMs: Date.now() - analyzerStartedAt, - analyzerStatus: "degraded", - diagnostics, - options, - }); - } - } else { - analyzerStatus[name] = "ok"; - } - } catch (error) { - analyzerStatus[name] = "degraded"; + analyzerStatus[name] = status; + analyzerTelemetry[name] = { + status, + elapsedMs: Date.now() - analyzerStartedAt, + timeoutMs, + costClass: item.descriptor.cost, + partialStatus: "partial", + partialReason, + capped: status === "capped" || diagnostics.capped, + }; partial = true; diagnostics.partialStatus = "partial"; - diagnostics.partialReason ??= error instanceof Error ? error.message : "analyzer_error"; - attachAnalysisMetrics(diagnostics, analysis); - captureDegradation(error, { - analyzer: name, - requested, - req, - timeoutMs: budgetMs, + diagnostics.partialReason = partialReason; + if (diagnostics.captureDegradation) { + attachAnalysisMetrics(diagnostics, analysis); + captureDegradation(new Error(diagnostics.partialReason), { + analyzer: name, + requested: plan.requested, + req, + timeoutMs, + elapsedMs: Date.now() - analyzerStartedAt, + analyzerStatus: status, + profile: plan.profile, + costClass: item.descriptor.cost, + responseReserveMs: plan.responseReserveMs, + diagnostics, + options, + }); + } + } else { + analyzerStatus[name] = "ok"; + analyzerTelemetry[name] = { + status: "ok", elapsedMs: Date.now() - analyzerStartedAt, - analyzerStatus: "degraded", - diagnostics, - options, - }); + timeoutMs, + costClass: item.descriptor.cost, + partialStatus: diagnostics.partialStatus, + }; } - }), - ); + } catch (error) { + const status = timeoutStatus(error, diagnostics); + const partialReason = publicPartialReason(diagnostics.partialReason, "analyzer_error"); + analyzerStatus[name] = status; + analyzerTelemetry[name] = { + status, + elapsedMs: Date.now() - analyzerStartedAt, + timeoutMs, + costClass: item.descriptor.cost, + partialStatus: "partial", + partialReason, + capped: status === "capped" || diagnostics.capped, + }; + partial = true; + diagnostics.partialStatus = "partial"; + diagnostics.partialReason = partialReason; + attachAnalysisMetrics(diagnostics, analysis); + captureDegradation(new Error(partialReason), { + analyzer: name, + requested: plan.requested, + req, + timeoutMs, + elapsedMs: Date.now() - analyzerStartedAt, + analyzerStatus: status, + profile: plan.profile, + costClass: item.descriptor.cost, + responseReserveMs: plan.responseReserveMs, + diagnostics, + options, + }); + } + } + + for (const cost of COST_ORDER) { + const items = plan.runnable.filter((item) => item.descriptor.cost === cost); + if (!items.length) continue; + await runWithConcurrency( + items, + costClassConcurrency(plan.profile, cost, plan.explicitAnalyzers), + runAnalyzer, + ); + } + for (const name of all) - if (!requested.includes(name)) analyzerStatus[name] = "skipped"; + if (!plan.requested.includes(name)) { + analyzerStatus[name] = "skipped"; + analyzerTelemetry[name] ??= { + status: "skipped", + elapsedMs: 0, + skipReason: "not_requested", + }; + } const { promptSection, systemSuffix } = renderBrief( findings, req.budget?.maxBriefChars ?? 6000, ); + const elapsedMs = Date.now() - start; + const metrics = analysis.snapshotMetrics(); + const cacheTotal = metrics.cacheHits + metrics.cacheMisses; return { schemaVersion: 1, repoFullName: req.repoFullName, prNumber: req.prNumber, headSha: req.headSha ?? null, generatedAtIso: new Date().toISOString(), - elapsedMs: Date.now() - start, + elapsedMs, partial, analyzerStatus, + telemetry: { + profile: plan.profile, + responseReserveMs: plan.responseReserveMs, + requestedAnalyzers: plan.requested, + analyzerCount: { + requested: plan.requested.length, + runnable: plan.runnable.length, + skipped: plan.skipped.length, + }, + analyzers: analyzerTelemetry, + cacheHits: metrics.cacheHits, + cacheMisses: metrics.cacheMisses, + cacheHitRate: cacheTotal > 0 ? metrics.cacheHits / cacheTotal : 0, + externalCallsByCategory: metrics.externalCallsByCategory, + skippedWorkByCategory: metrics.skippedWorkByCategory, + cappedWorkByCategory: metrics.cappedWorkByCategory, + elapsedMs, + }, findings, promptSection, systemSuffix, diff --git a/review-enrichment/src/external-fetch.ts b/review-enrichment/src/external-fetch.ts new file mode 100644 index 0000000000..897a7730c1 --- /dev/null +++ b/review-enrichment/src/external-fetch.ts @@ -0,0 +1,266 @@ +import type { AnalyzerDiagnostics } from "./types.js"; + +export type BoundedFetchFailureReason = + | "aborted" + | "timeout" + | "network_error" + | "http_error" + | "response_too_large" + | "invalid_json" + | "call_cap"; + +export interface BoundedFetchOk { + ok: true; + status: number; + data: T; + bytes: number | null; + elapsedMs: number; + endpointCategory: string; +} + +export interface BoundedFetchFailure { + ok: false; + status?: number; + reason: BoundedFetchFailureReason; + bytes: number | null; + elapsedMs: number; + endpointCategory: string; + capped?: boolean; +} + +export type BoundedFetchResult = BoundedFetchOk | BoundedFetchFailure; + +export interface BoundedFetchOptions { + endpointCategory: string; + method?: string; + headers?: HeadersInit; + body?: BodyInit | null; + signal?: AbortSignal; + timeoutMs?: number; + maxBytes?: number; + fetchImpl?: typeof fetch; + diagnostics?: AnalyzerDiagnostics; + phase?: string; + subcall?: string; +} + +const DEFAULT_EXTERNAL_TIMEOUT_MS = 1200; +const DEFAULT_MAX_JSON_BYTES = 512 * 1024; + +export function safeEndpointCategory(category: string): string { + const safe = category.replace(/[^A-Za-z0-9_.:-]+/g, "_").slice(0, 80); + return safe || "unknown"; +} + +export function externalFetchCacheKey( + url: string, + options: Pick = {}, +): string { + const method = (options.method ?? "GET").toUpperCase(); + return `${method}:${url}:body:${hashBody(options.body)}`; +} + +export async function boundedFetchJson( + url: string, + options: BoundedFetchOptions, +): Promise> { + const text = await boundedFetchText(url, options); + if (!text.ok) return text; + try { + return { + ...text, + data: JSON.parse(text.data) as T, + }; + } catch { + const result = failure( + text.endpointCategory, + "invalid_json", + Date.now() - text.elapsedMs, + text.bytes, + text.status, + ); + attachDiagnostics(result, options); + return result; + } +} + +export async function boundedFetchText( + url: string, + options: BoundedFetchOptions, +): Promise> { + const endpointCategory = safeEndpointCategory(options.endpointCategory); + const startedAtMs = Date.now(); + const signal = options.signal; + if (signal?.aborted) { + const result = failure(endpointCategory, "aborted", startedAtMs, null); + attachDiagnostics(result, options); + return result; + } + + const controller = new AbortController(); + let timedOut = false; + const abortFromParent = () => controller.abort(); + signal?.addEventListener("abort", abortFromParent, { once: true }); + const timeoutMs = Math.max( + 1, + Math.floor(options.timeoutMs ?? DEFAULT_EXTERNAL_TIMEOUT_MS), + ); + const timer = setTimeout(() => { + timedOut = true; + controller.abort(); + }, timeoutMs); + + try { + const response = await (options.fetchImpl ?? fetch)(url, { + method: options.method, + headers: options.headers, + body: options.body, + signal: controller.signal, + }); + const status = response.status; + if (!response.ok) { + const result = failure(endpointCategory, "http_error", startedAtMs, null, status); + attachDiagnostics(result, options); + return result; + } + + const maxBytes = Math.max(1, Math.floor(options.maxBytes ?? DEFAULT_MAX_JSON_BYTES)); + const text = await readResponseText(response, maxBytes); + if (text === null) { + const result = failure( + endpointCategory, + "response_too_large", + startedAtMs, + null, + status, + true, + ); + attachDiagnostics(result, options); + return result; + } + + return { + ok: true, + status, + data: text, + bytes: byteLength(text), + elapsedMs: Date.now() - startedAtMs, + endpointCategory, + }; + } catch { + const reason = + timedOut || controller.signal.aborted ? (timedOut ? "timeout" : "aborted") : "network_error"; + const result = failure(endpointCategory, reason, startedAtMs, null); + attachDiagnostics(result, options); + return result; + } finally { + clearTimeout(timer); + signal?.removeEventListener("abort", abortFromParent); + } +} + +function failure( + endpointCategory: string, + reason: BoundedFetchFailureReason, + startedAtMs: number, + bytes: number | null, + status?: number, + capped = false, +): BoundedFetchFailure { + return { + ok: false, + ...(status !== undefined ? { status } : {}), + reason, + bytes, + elapsedMs: Date.now() - startedAtMs, + endpointCategory, + ...(capped ? { capped: true } : {}), + }; +} + +function attachDiagnostics( + result: BoundedFetchFailure, + options: BoundedFetchOptions, +): void { + const diagnostics = options.diagnostics; + if (!diagnostics || !shouldMarkDegraded(result)) return; + diagnostics.partialStatus = "partial"; + diagnostics.partialReason ??= `${result.endpointCategory}_${result.reason}`; + diagnostics.captureDegradation = true; + diagnostics.endpointCategory = result.endpointCategory; + diagnostics.externalFailureReason = result.reason; + diagnostics.externalElapsedMs = result.elapsedMs; + if (result.capped) diagnostics.capped = true; + if (result.endpointCategory.startsWith("github-")) { + diagnostics.githubEndpointCategory = result.endpointCategory; + } + if (options.phase) diagnostics.phase = options.phase; + diagnostics.subcall = options.subcall ?? result.endpointCategory; +} + +function shouldMarkDegraded(result: BoundedFetchFailure): boolean { + if (result.reason !== "http_error") return true; + const status = result.status ?? 0; + return status === 403 || status === 429 || status >= 500; +} + +async function readResponseText( + response: Response, + maxBytes: number, +): Promise { + const contentLength = response.headers?.get("content-length"); + if (contentLength !== null && contentLength !== undefined) { + const parsedLength = Number.parseInt(contentLength, 10); + if (Number.isFinite(parsedLength) && parsedLength > maxBytes) return null; + } + + const reader = response.body?.getReader(); + if (!reader) { + const text = + typeof response.text === "function" + ? await response.text() + : typeof response.json === "function" + ? JSON.stringify(await response.json()) + : ""; + return byteLength(text) > maxBytes ? null : text; + } + + const decoder = new TextDecoder(); + let totalBytes = 0; + let text = ""; + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + totalBytes += value.byteLength; + if (totalBytes > maxBytes) { + await reader.cancel(); + return null; + } + text += decoder.decode(value, { stream: true }); + } + text += decoder.decode(); + return text; + } finally { + reader.releaseLock(); + } +} + +function hashBody(body: BodyInit | null | undefined): string { + if (body === null || body === undefined) return "none"; + if (typeof body === "string") return `${body.length}:${fnv1a(body)}`; + return "stream"; +} + +function fnv1a(value: string): string { + let hash = 0x811c9dc5; + for (let index = 0; index < value.length; index += 1) { + hash ^= value.charCodeAt(index); + hash = Math.imul(hash, 0x01000193) >>> 0; + } + return hash.toString(16).padStart(8, "0"); +} + +function byteLength(value: string): number { + return new TextEncoder().encode(value).byteLength; +} diff --git a/review-enrichment/src/request-guardrails.ts b/review-enrichment/src/request-guardrails.ts new file mode 100644 index 0000000000..5cbf2ade61 --- /dev/null +++ b/review-enrichment/src/request-guardrails.ts @@ -0,0 +1,152 @@ +import type { EnrichRequest } from "./types.js"; + +export const MAX_BODY_BYTES = 2 * 1024 * 1024; +const MAX_FILES = 300; +const MAX_DIFF_BYTES = 1_000_000; +const MAX_TOTAL_PATCH_BYTES = 1_500_000; +const MAX_PATH_CHARS = 1000; +const MAX_ANALYZERS = 100; + +export type EnrichRequestParseResult = + | { ok: true; payload: EnrichRequest; bodyBytes: number } + | { ok: false; status: 400 | 413; error: string; bodyBytes: number }; + +export type EnrichRequestBodyReadResult = + | { ok: true; raw: string; bodyBytes: number } + | { ok: false; status: 413; error: "request_too_large"; bodyBytes: number }; + +export async function readEnrichRequestText(request: Request): Promise { + const contentLength = request.headers.get("content-length"); + if (contentLength) { + const parsedLength = Number.parseInt(contentLength, 10); + if (Number.isFinite(parsedLength) && parsedLength > MAX_BODY_BYTES) { + return { + ok: false, + status: 413, + error: "request_too_large", + bodyBytes: parsedLength, + }; + } + } + + const reader = request.body?.getReader(); + if (!reader) return { ok: true, raw: "", bodyBytes: 0 }; + + const chunks: Uint8Array[] = []; + let bodyBytes = 0; + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + bodyBytes += value.byteLength; + if (bodyBytes > MAX_BODY_BYTES) { + await reader.cancel(); + return { + ok: false, + status: 413, + error: "request_too_large", + bodyBytes, + }; + } + chunks.push(value); + } + } finally { + reader.releaseLock(); + } + + return { ok: true, raw: decodeChunks(chunks, bodyBytes), bodyBytes }; +} + +export function parseEnrichRequestBody(raw: string): EnrichRequestParseResult { + const bodyBytes = byteLength(raw); + if (bodyBytes > MAX_BODY_BYTES) { + return { ok: false, status: 413, error: "request_too_large", bodyBytes }; + } + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return { ok: false, status: 400, error: "bad_json", bodyBytes }; + } + + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + return { ok: false, status: 400, error: "bad_request", bodyBytes }; + } + + const payload = parsed as EnrichRequest; + if (!validRepo(payload.repoFullName) || !validPullNumber(payload.prNumber)) { + return { ok: false, status: 400, error: "bad_request", bodyBytes }; + } + if (payload.files !== undefined && !Array.isArray(payload.files)) { + return { ok: false, status: 400, error: "bad_files", bodyBytes }; + } + if ((payload.files?.length ?? 0) > MAX_FILES) { + return { ok: false, status: 413, error: "too_many_files", bodyBytes }; + } + if (typeof payload.diff === "string" && byteLength(payload.diff) > MAX_DIFF_BYTES) { + return { ok: false, status: 413, error: "diff_too_large", bodyBytes }; + } + if (!validAnalyzers(payload.analyzers)) { + return { ok: false, status: 400, error: "bad_analyzers", bodyBytes }; + } + if (!validFiles(payload.files)) { + return { ok: false, status: 400, error: "bad_files", bodyBytes }; + } + if (totalPatchBytes(payload.files) > MAX_TOTAL_PATCH_BYTES) { + return { ok: false, status: 413, error: "patches_too_large", bodyBytes }; + } + + return { ok: true, payload, bodyBytes }; +} + +function validRepo(value: unknown): value is string { + return ( + typeof value === "string" && + /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(value) && + value.length <= 200 + ); +} + +function validPullNumber(value: unknown): value is number { + return typeof value === "number" && Number.isInteger(value) && value > 0; +} + +function validAnalyzers(value: unknown): boolean { + if (value === undefined) return true; + if (!Array.isArray(value) || value.length > MAX_ANALYZERS) return false; + return value.every((entry) => typeof entry === "string" && entry.length <= 80); +} + +function validFiles(files: EnrichRequest["files"]): boolean { + if (!files) return true; + return files.every((file) => { + if (!file || typeof file !== "object") return false; + if (typeof file.path !== "string" || !file.path || file.path.length > MAX_PATH_CHARS) return false; + if (file.patch !== undefined && typeof file.patch !== "string") return false; + if (file.status !== undefined && typeof file.status !== "string") return false; + if (file.previousPath !== undefined && typeof file.previousPath !== "string") return false; + return true; + }); +} + +function totalPatchBytes(files: EnrichRequest["files"]): number { + return (files ?? []).reduce( + (total, file) => total + (typeof file.patch === "string" ? byteLength(file.patch) : 0), + 0, + ); +} + +function decodeChunks(chunks: readonly Uint8Array[], bodyBytes: number): string { + const buffer = new Uint8Array(bodyBytes); + let offset = 0; + for (const chunk of chunks) { + buffer.set(chunk, offset); + offset += chunk.byteLength; + } + return new TextDecoder().decode(buffer); +} + +function byteLength(value: string): number { + return new TextEncoder().encode(value).byteLength; +} diff --git a/review-enrichment/src/scheduler.ts b/review-enrichment/src/scheduler.ts new file mode 100644 index 0000000000..51fdb523bc --- /dev/null +++ b/review-enrichment/src/scheduler.ts @@ -0,0 +1,395 @@ +import type { AnalysisContext } from "./analysis-context.js"; +import { + ANALYZER_NAMES, + getAnalyzerDescriptor, +} from "./analyzers/registry.js"; +import type { + AnalyzerCostClass, + AnalyzerDescriptor, + AnalyzerName, + AnalyzerRegistry, + AnyAnalyzerDescriptor, +} from "./analyzers/types.js"; +import type { + AnalyzerStatus, + EnrichRequest, + ReesProfileName, +} from "./types.js"; + +export const DEFAULT_REES_PROFILE: ReesProfileName = "balanced"; + +export const REES_PROFILES = ["fast", "balanced", "deep"] as const satisfies readonly ReesProfileName[]; + +export const COST_ORDER: readonly AnalyzerCostClass[] = [ + "local", + "registry", + "github-light", + "github-heavy", + "tooling", +]; + +const PROFILE_CONFIG: Record< + ReesProfileName, + { + costs: ReadonlySet; + concurrency: Record; + timeoutMs: Record; + responseReserveMs: number; + minStartMs: number; + } +> = { + fast: { + costs: new Set(["local", "registry"]), + concurrency: { + local: 8, + registry: 2, + "github-light": 0, + "github-heavy": 0, + tooling: 0, + }, + timeoutMs: { + local: 400, + registry: 800, + "github-light": 0, + "github-heavy": 0, + tooling: 0, + }, + responseReserveMs: 500, + minStartMs: 1, + }, + balanced: { + costs: new Set(COST_ORDER), + concurrency: { + local: 8, + registry: 3, + "github-light": 2, + "github-heavy": 1, + tooling: 1, + }, + timeoutMs: { + local: 750, + registry: 1400, + "github-light": 1400, + "github-heavy": 2200, + tooling: 1400, + }, + responseReserveMs: 750, + minStartMs: 1, + }, + deep: { + costs: new Set(COST_ORDER), + concurrency: { + local: 8, + registry: 4, + "github-light": 2, + "github-heavy": 1, + tooling: 1, + }, + timeoutMs: { + local: 1000, + registry: 2500, + "github-light": 2500, + "github-heavy": 4000, + tooling: 2500, + }, + responseReserveMs: 1000, + minStartMs: 1, + }, +}; + +export interface AnalyzerPlanItem { + name: AnalyzerName; + descriptor: AnalyzerDescriptor; + status?: AnalyzerStatus; + skipReason?: string; +} + +export interface AnalyzerPlan { + profile: ReesProfileName; + explicitAnalyzers: boolean; + requested: AnalyzerName[]; + runnable: AnalyzerPlanItem[]; + skipped: AnalyzerPlanItem[]; + responseReserveMs: number; + executionDeadlineMs: number; +} + +export interface ReesProfileMetadata { + name: ReesProfileName; + default: boolean; + costClasses: AnalyzerCostClass[]; + concurrency: Record; + timeoutMs: Record; + responseReserveMs: number; +} + +export function resolveReesProfile(value: unknown): ReesProfileName { + if (typeof value !== "string") return DEFAULT_REES_PROFILE; + const normalized = value.trim().toLowerCase(); + return isReesProfileName(normalized) ? normalized : DEFAULT_REES_PROFILE; +} + +export function isReesProfileName(value: string): value is ReesProfileName { + return (REES_PROFILES as readonly string[]).includes(value); +} + +export function reesProfileMetadata(): ReesProfileMetadata[] { + return REES_PROFILES.map((name) => { + const config = PROFILE_CONFIG[name]; + return { + name, + default: name === DEFAULT_REES_PROFILE, + costClasses: COST_ORDER.filter((cost) => config.costs.has(cost)), + concurrency: { ...config.concurrency }, + timeoutMs: { ...config.timeoutMs }, + responseReserveMs: config.responseReserveMs, + }; + }); +} + +export function responseReserveMs(profile: ReesProfileName, budgetMs: number): number { + const configured = PROFILE_CONFIG[profile].responseReserveMs; + const proportional = Math.floor(Math.max(0, budgetMs) * 0.2); + const reserve = Math.min(configured, Math.max(150, proportional)); + return Math.min(Math.max(0, budgetMs - 1), reserve); +} + +export function costClassConcurrency( + profile: ReesProfileName, + cost: AnalyzerCostClass, + explicitAnalyzer = false, +): number { + const configured = Math.max(0, PROFILE_CONFIG[profile].concurrency[cost] ?? 0); + if (configured > 0 || !explicitAnalyzer) return configured; + return Math.max(1, PROFILE_CONFIG[DEFAULT_REES_PROFILE].concurrency[cost] ?? 1); +} + +export function analyzerTimeoutMs( + profile: ReesProfileName, + cost: AnalyzerCostClass, + remainingMs: number, + explicitAnalyzer = false, +): number { + const configured = Math.max(0, PROFILE_CONFIG[profile].timeoutMs[cost] ?? 0); + const classBudget = + configured > 0 || !explicitAnalyzer + ? configured + : Math.max(0, PROFILE_CONFIG[DEFAULT_REES_PROFILE].timeoutMs[cost] ?? 0); + return Math.max(0, Math.min(classBudget, Math.floor(remainingMs))); +} + +export function shouldStartAnalyzer( + profile: ReesProfileName, + remainingMs: number, +): boolean { + return remainingMs >= PROFILE_CONFIG[profile].minStartMs; +} + +export function planAnalyzers( + req: EnrichRequest, + analyzers: AnalyzerRegistry, + analysis: AnalysisContext, + options: { budgetMs: number; startedAtMs: number }, +): AnalyzerPlan { + const profile = resolveReesProfile(req.profile); + const explicitAnalyzers = Array.isArray(req.analyzers); + const configuredReserve = responseReserveMs(profile, options.budgetMs); + const executionDeadlineMs = options.startedAtMs + options.budgetMs - configuredReserve; + const allNames = analyzerNamesForRegistry(analyzers); + const requested = selectRequestedAnalyzers(req, allNames, profile); + const runnable: AnalyzerPlanItem[] = []; + const skipped: AnalyzerPlanItem[] = []; + + for (const name of requested) { + const descriptor = descriptorForAnalyzer(name); + const skipReason = skipReasonForAnalyzer( + req, + analysis, + descriptor, + profile, + explicitAnalyzers, + ); + if (skipReason) { + skipped.push({ + name, + descriptor, + status: "skipped", + skipReason, + }); + analysis.metrics.recordSkippedWork(`analyzer_${skipReason}`); + continue; + } + runnable.push({ name, descriptor }); + } + + runnable.sort( + (left, right) => + COST_ORDER.indexOf(left.descriptor.cost) - COST_ORDER.indexOf(right.descriptor.cost) || + allNames.indexOf(left.name) - allNames.indexOf(right.name), + ); + + return { + profile, + explicitAnalyzers, + requested, + runnable, + skipped, + responseReserveMs: configuredReserve, + executionDeadlineMs, + }; +} + +function analyzerNamesForRegistry(analyzers: AnalyzerRegistry): AnalyzerName[] { + const names = Object.keys(analyzers) as AnalyzerName[]; + return names.sort((left, right) => { + const leftIndex = ANALYZER_NAMES.indexOf(left); + const rightIndex = ANALYZER_NAMES.indexOf(right); + if (leftIndex === -1 && rightIndex === -1) return left.localeCompare(right); + if (leftIndex === -1) return 1; + if (rightIndex === -1) return -1; + return leftIndex - rightIndex; + }); +} + +function selectRequestedAnalyzers( + req: EnrichRequest, + names: readonly AnalyzerName[], + profile: ReesProfileName, +): AnalyzerName[] { + if (Array.isArray(req.analyzers)) { + return names.filter((name) => req.analyzers!.includes(name)); + } + const config = PROFILE_CONFIG[profile]; + return names.filter((name) => { + const descriptor = descriptorForAnalyzer(name); + return descriptor.defaultEnabled && config.costs.has(descriptor.cost); + }); +} + +function descriptorForAnalyzer(name: AnalyzerName): AnalyzerDescriptor { + const descriptor = getAnalyzerDescriptor(name); + if (descriptor) return descriptor as AnalyzerDescriptor; + return { + name, + title: name, + category: "quality", + cost: "local", + defaultEnabled: true, + requires: [], + docs: { + summary: "Custom analyzer supplied by a caller.", + looksAt: "Caller-provided inputs.", + reports: "Caller-defined findings.", + network: "Unknown.", + notes: "Synthetic descriptor used for tests or injected registries.", + }, + run: async () => [] as never, + }; +} + +function skipReasonForAnalyzer( + req: EnrichRequest, + analysis: AnalysisContext, + descriptor: AnyAnalyzerDescriptor | AnalyzerDescriptor, + profile: ReesProfileName, + explicitAnalyzers: boolean, +): string | null { + if (!explicitAnalyzers && !PROFILE_CONFIG[profile].costs.has(descriptor.cost)) return "profile"; + if (!explicitAnalyzers && costClassConcurrency(profile, descriptor.cost) <= 0) return "profile"; + + if ( + descriptor.requires.includes("files") && + analysis.changedFiles.length === 0 && + !historyCanRunWithoutGitHub(req, descriptor.name) + ) { + return "no_files"; + } + if (descriptor.requires.includes("head-sha") && !req.headSha) { + return "missing_head_sha"; + } + if (descriptor.requires.includes("base-sha") && !req.baseSha) { + return "missing_base_sha"; + } + if (descriptor.requires.includes("author") && !req.author && descriptor.name !== "history") { + return "missing_author"; + } + if ( + descriptor.requires.includes("github-token") && + !req.githubToken && + !historyCanRunWithoutGitHub(req, descriptor.name) + ) { + return "missing_github_token"; + } + + return inputSkipReason(descriptor.name, analysis, req); +} + +function inputSkipReason( + name: AnalyzerName, + analysis: AnalysisContext, + req: EnrichRequest, +): string | null { + switch (name) { + case "dependency": + case "license": + case "installScript": + case "heavyDependency": + case "typosquat": + case "nativeBuild": + return analysis.dependencyManifestPaths.length ? null : "no_dependency_manifest"; + case "lockfileDrift": + return analysis.fileCategories.some((file) => file.category === "lockfile") + ? null + : "no_lockfile"; + case "actionPin": + return analysis.fileCategories.some((file) => file.category === "workflow") + ? null + : "no_workflow"; + case "eol": + return analysis.changedFilePaths.some(isRuntimePinPath) ? null : "no_runtime_pin"; + case "redos": + case "secret": + case "secretLog": + return analysis.addedLines.length ? null : "no_added_lines"; + case "provenance": + return analysis.dependencyManifestPaths.length || + analysis.changedFiles.some((file) => file.status === "added" || file.status === "copied") + ? null + : "no_provenance_input"; + case "codeowners": + return analysis.changedFilePaths.length ? null : "no_changed_paths"; + case "assetWeight": + return analysis.fileCategories.some((file) => file.category === "asset") + ? null + : "no_asset_paths"; + case "commitSignature": + return null; + case "iacMisconfig": + return analysis.fileCategories.some((file) => file.category === "config") + ? null + : "no_config_paths"; + case "history": + if (historyCanRunWithoutGitHub(req, name)) return null; + return req.githubToken && req.author && analysis.changedFilePaths.length + ? null + : "no_history_input"; + default: + return null; + } +} + +function historyCanRunWithoutGitHub( + req: EnrichRequest, + name: AnalyzerName, +): boolean { + return name === "history" && Boolean(req.linkedIssue && (req.diff || req.files?.length)); +} + +function isRuntimePinPath(path: string): boolean { + const basename = path.split("/").pop() ?? path; + return ( + /^Dockerfile(?:\..*)?$/.test(basename) || + basename === ".nvmrc" || + basename === "go.mod" + ); +} diff --git a/review-enrichment/src/sentry.ts b/review-enrichment/src/sentry.ts index dabf2d0a73..c178dbbf69 100644 --- a/review-enrichment/src/sentry.ts +++ b/review-enrichment/src/sentry.ts @@ -111,10 +111,16 @@ export interface AnalyzerDegradationContext { timeoutMs?: number; elapsedMs?: number; analyzerStatus?: string; + profile?: string; + costClass?: string; + responseReserveMs?: number; partialStatus?: string; partialReason?: string; phase?: string; subcall?: string; + endpointCategory?: string; + externalFailureReason?: string; + externalElapsedMs?: number; fileLookupCount?: number; commitLookupCount?: number; prLookupCount?: number; @@ -144,10 +150,16 @@ export function captureAnalyzerDegradation(error: unknown, context: AnalyzerDegr timeoutMs: context.timeoutMs, elapsedMs: context.elapsedMs, analyzerStatus: context.analyzerStatus, + profile: context.profile, + costClass: context.costClass, + responseReserveMs: context.responseReserveMs, partialStatus: context.partialStatus, partialReason: context.partialReason, phase: context.phase, subcall: context.subcall, + endpointCategory: context.endpointCategory, + externalFailureReason: context.externalFailureReason, + externalElapsedMs: context.externalElapsedMs, fileLookupCount: context.fileLookupCount, commitLookupCount: context.commitLookupCount, prLookupCount: context.prLookupCount, @@ -181,16 +193,26 @@ export function captureAnalyzerDegradation(error: unknown, context: AnalyzerDegr if (timeoutTag) scope.setTag("timeoutMs", timeoutTag); if (releaseTag) scope.setTag("release", releaseTag); const analyzerStatusTag = sentryTagValue(context.analyzerStatus); + const profileTag = sentryTagValue(context.profile); + const costClassTag = sentryTagValue(context.costClass); + const responseReserveTag = sentryTagValue(context.responseReserveMs); const partialStatusTag = sentryTagValue(context.partialStatus); const phaseTag = sentryTagValue(context.phase); + const endpointCategoryTag = sentryTagValue(context.endpointCategory); + const externalFailureReasonTag = sentryTagValue(context.externalFailureReason); const endpointTag = sentryTagValue(context.githubEndpointCategory); const requestIdTag = sentryTagValue(context.requestId); const traceIdTag = sentryTagValue(context.traceId); const cacheHitsTag = sentryTagValue(context.cacheHits); const cacheMissesTag = sentryTagValue(context.cacheMisses); if (analyzerStatusTag) scope.setTag("analyzerStatus", analyzerStatusTag); + if (profileTag) scope.setTag("profile", profileTag); + if (costClassTag) scope.setTag("costClass", costClassTag); + if (responseReserveTag) scope.setTag("responseReserveMs", responseReserveTag); if (partialStatusTag) scope.setTag("partialStatus", partialStatusTag); if (phaseTag) scope.setTag("phase", phaseTag); + if (endpointCategoryTag) scope.setTag("endpointCategory", endpointCategoryTag); + if (externalFailureReasonTag) scope.setTag("externalFailureReason", externalFailureReasonTag); if (endpointTag) scope.setTag("githubEndpointCategory", endpointTag); if (requestIdTag) scope.setTag("requestId", requestIdTag); if (traceIdTag) scope.setTag("traceId", traceIdTag); diff --git a/review-enrichment/src/server.ts b/review-enrichment/src/server.ts index 248ac2f8cc..c981b80186 100644 --- a/review-enrichment/src/server.ts +++ b/review-enrichment/src/server.ts @@ -10,8 +10,11 @@ import { serve } from "@hono/node-server"; import { Hono } from "hono"; import { normalizeSharedSecret, verifyBearer } from "./auth.js"; -import type { EnrichRequest } from "./types.js"; import { buildBrief } from "./brief.js"; +import { + parseEnrichRequestBody, + readEnrichRequestText, +} from "./request-guardrails.js"; import { captureError, flushSentry, @@ -54,18 +57,13 @@ app.post("/v1/enrich", async (c) => { if (!verifyBearer(c.req.header("authorization"), secret)) return c.json({ error: "unauthorized" }, 401); - const payload = (await c.req - .json() - .catch(() => null)) as EnrichRequest | null; - if ( - !payload || - typeof payload.repoFullName !== "string" || - typeof payload.prNumber !== "number" - ) { - return c.json({ error: "bad_request" }, 400); - } + const body = await readEnrichRequestText(c.req.raw); + if (!body.ok) return c.json({ error: body.error }, body.status); + + const parsed = parseEnrichRequestBody(body.raw); + if (!parsed.ok) return c.json({ error: parsed.error }, parsed.status); - const brief = await buildBrief(payload, undefined, { + const brief = await buildBrief(parsed.payload, undefined, { requestId: c.req.header("x-gittensory-request-id") ?? c.req.header("x-request-id"), traceId: traceIdFromTraceparent(c.req.header("traceparent")), }); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 51f4ad567e..d3e4bd61e7 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -25,9 +25,12 @@ export interface EnrichRequest { * whether the diff covers the issue's stated requirement without an extra fetch. Absent ⇒ alignment omitted. (#1478) */ linkedIssue?: EnrichLinkedIssue; budget?: { timeoutMs?: number; maxBriefChars?: number }; + profile?: ReesProfileName; analyzers?: string[]; } +export type ReesProfileName = "fast" | "balanced" | "deep"; + /** 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 { @@ -302,7 +305,7 @@ export interface DocCommentDriftFinding { staleParams: string[]; } -export type AnalyzerStatus = "ok" | "degraded" | "skipped"; +export type AnalyzerStatus = "ok" | "degraded" | "skipped" | "capped" | "timeout"; /** Internal, public-safe analyzer diagnostics for Sentry. Never attach request bodies, diffs, tokens, or raw prompts. */ export interface AnalyzerDiagnostics { @@ -311,6 +314,9 @@ export interface AnalyzerDiagnostics { partialStatus?: "complete" | "partial"; partialReason?: string; githubEndpointCategory?: string; + endpointCategory?: string; + externalFailureReason?: string; + externalElapsedMs?: number; fileLookupCount?: number; commitLookupCount?: number; prLookupCount?: number; @@ -344,7 +350,38 @@ export interface ReviewBrief { elapsedMs: number; partial: boolean; analyzerStatus: Record; + telemetry: ReviewBriefTelemetry; findings: BriefFindings; promptSection: string; systemSuffix: string; } + +export interface ReviewBriefTelemetry { + profile: ReesProfileName; + responseReserveMs: number; + requestedAnalyzers: string[]; + analyzerCount: { + requested: number; + runnable: number; + skipped: number; + }; + analyzers: Record; + cacheHits: number; + cacheMisses: number; + cacheHitRate: number; + externalCallsByCategory: Record; + skippedWorkByCategory: Record; + cappedWorkByCategory: Record; + elapsedMs: number; +} + +export interface AnalyzerTelemetry { + status: AnalyzerStatus; + elapsedMs: number; + timeoutMs?: number; + costClass?: string; + partialStatus?: "complete" | "partial"; + partialReason?: string; + skipReason?: string; + capped?: boolean; +} diff --git a/review-enrichment/test/analyzer-metadata.test.ts b/review-enrichment/test/analyzer-metadata.test.ts new file mode 100644 index 0000000000..e7e2c80486 --- /dev/null +++ b/review-enrichment/test/analyzer-metadata.test.ts @@ -0,0 +1,49 @@ +import { readFileSync } from "node:fs"; +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { ANALYZER_DESCRIPTORS, ANALYZER_NAMES } from "../dist/analyzers/registry.js"; +import { reesProfileMetadata } from "../dist/scheduler.js"; + +test("generated analyzer metadata matches the runtime registry and profiles", () => { + const metadata = JSON.parse(readFileSync("analyzer-metadata.json", "utf8")) as { + schemaVersion: number; + defaultProfile: string; + profiles: Array<{ name: string; default: boolean; costClasses: string[] }>; + analyzers: Array<{ + name: string; + title: string; + category: string; + cost: string; + defaultEnabled: boolean; + profiles: string[]; + requires: string[]; + limits: Record; + docs: Record; + }>; + }; + + assert.equal(metadata.schemaVersion, 1); + assert.equal(metadata.defaultProfile, "balanced"); + assert.deepEqual( + metadata.profiles.map((profile) => profile.name), + reesProfileMetadata().map((profile) => profile.name), + ); + assert.deepEqual( + metadata.analyzers.map((analyzer) => analyzer.name), + ANALYZER_NAMES, + ); + + for (const descriptor of ANALYZER_DESCRIPTORS) { + const generated = metadata.analyzers.find((analyzer) => analyzer.name === descriptor.name); + assert.ok(generated, `missing generated metadata for ${descriptor.name}`); + assert.equal(generated.title, descriptor.title); + assert.equal(generated.category, descriptor.category); + assert.equal(generated.cost, descriptor.cost); + assert.equal(generated.defaultEnabled, descriptor.defaultEnabled); + assert.deepEqual(generated.requires, descriptor.requires); + assert.deepEqual(generated.limits, descriptor.limits ?? {}); + assert.equal(generated.docs.summary, descriptor.docs.summary); + assert.ok(generated.profiles.includes("balanced")); + } +}); diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 5c441c6caa..837fd0bd2e 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -618,6 +618,7 @@ test("buildBrief: runs dependency analyzer, marks others skipped, partial=false repoFullName: "o/r", prNumber: 7, headSha: "abc", + analyzers: ["dependency"], files: [{ path: "package.json", patch: '+ "lodash": "4.17.20",' }], }); assert.equal(brief.schemaVersion, 1); @@ -735,6 +736,10 @@ test("buildBrief: dependency + secret analyzers both run", async () => { repoFullName: "o/r", prNumber: 9, files: [ + { + path: "package.json", + patch: '+ "lodash": "4.17.20",', + }, { path: "app.ts", patch: @@ -1669,7 +1674,7 @@ test("buildBrief: timeout aborts dependency scan so OSV work stops", async () => repoFullName: "o/r", prNumber: 10, analyzers: ["dependency"], - budget: { timeoutMs: 1 }, + budget: { timeoutMs: 200 }, files: Array.from({ length: 5 }, (_, index) => ({ path: "package.json", patch: `+ "pkg-${index}": "1.0.0",`, @@ -1677,7 +1682,7 @@ test("buildBrief: timeout aborts dependency scan so OSV work stops", async () => }); assert.equal(brief.partial, true); - assert.equal(brief.analyzerStatus.dependency, "degraded"); + assert.equal(brief.analyzerStatus.dependency, "timeout"); assert.equal(fetchCount, 1); assert.equal(signals.length, 1); assert.equal(signals[0].aborted, true); @@ -3015,8 +3020,8 @@ test("buildBrief: provenance analyzer fetch failure fails safe", async () => { analyzers: ["provenance"], files: [{ path: "package.json", patch: '+ "pkg": "1.0.0",' }], }); - assert.equal(brief.analyzerStatus.provenance, "ok"); - assert.equal(brief.partial, false); + assert.equal(brief.analyzerStatus.provenance, "degraded"); + assert.equal(brief.partial, true); assert.deepEqual(brief.findings.provenance, []); } finally { globalThis.fetch = realFetch; diff --git a/review-enrichment/test/external-fetch.test.ts b/review-enrichment/test/external-fetch.test.ts new file mode 100644 index 0000000000..6aef88f575 --- /dev/null +++ b/review-enrichment/test/external-fetch.test.ts @@ -0,0 +1,122 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { createAnalysisContext } from "../dist/analysis-context.js"; +import { boundedFetchJson } from "../dist/external-fetch.js"; + +test("boundedFetchJson aborts slow subcalls and records safe diagnostics", async () => { + const diagnostics = {}; + const fetchImpl = async (_url, init = {}) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener("abort", () => reject(new Error("aborted")), { + once: true, + }); + }); + + const result = await boundedFetchJson("https://registry.example.test/private", { + endpointCategory: "npm-packument", + timeoutMs: 5, + body: "sensitive request body should not be attached", + fetchImpl, + diagnostics, + phase: "test-phase", + subcall: "test-subcall", + }); + + assert.equal(result.ok, false); + assert.equal(result.reason, "timeout"); + assert.equal(diagnostics.partialStatus, "partial"); + assert.equal(diagnostics.partialReason, "npm-packument_timeout"); + assert.equal(diagnostics.endpointCategory, "npm-packument"); + assert.equal(diagnostics.externalFailureReason, "timeout"); + assert.equal(diagnostics.phase, "test-phase"); + assert.equal(diagnostics.subcall, "test-subcall"); + const serialized = JSON.stringify(diagnostics); + assert.equal(serialized.includes("registry.example.test"), false); + assert.equal(serialized.includes("sensitive request body"), false); +}); + +test("boundedFetchJson caps oversized responses before reading the body", async () => { + const diagnostics = {}; + let bodyRead = false; + + const result = await boundedFetchJson("https://api.example.test/large", { + endpointCategory: "pypi-json", + maxBytes: 4, + diagnostics, + fetchImpl: async () => ({ + ok: true, + status: 200, + headers: new Headers({ "content-length": "5" }), + text: async () => { + bodyRead = true; + return "{}"; + }, + }), + }); + + assert.equal(result.ok, false); + assert.equal(result.reason, "response_too_large"); + assert.equal(result.capped, true); + assert.equal(bodyRead, false); + assert.equal(diagnostics.capped, true); + assert.equal(diagnostics.endpointCategory, "pypi-json"); + assert.equal(diagnostics.externalFailureReason, "response_too_large"); +}); + +test("AnalysisContext fetchJson de-dupes identical in-flight calls and caps new category calls", async () => { + const context = createAnalysisContext({ + repoFullName: "JSONbored/gittensory", + prNumber: 1812, + }); + let calls = 0; + const fetchImpl = async () => { + calls += 1; + await new Promise((resolve) => setTimeout(resolve, 5)); + return new Response(JSON.stringify({ ok: true })); + }; + + const [first, second] = await Promise.all([ + context.fetchJson("https://api.osv.dev/v1/query", { + endpointCategory: "osv-query", + method: "POST", + body: JSON.stringify({ id: "one" }), + fetchImpl, + maxCallsPerCategory: 1, + }), + context.fetchJson("https://api.osv.dev/v1/query", { + endpointCategory: "osv-query", + method: "POST", + body: JSON.stringify({ id: "one" }), + fetchImpl, + maxCallsPerCategory: 1, + }), + ]); + + assert.equal(first.ok, true); + assert.strictEqual(first, second); + assert.equal(calls, 1); + assert.deepEqual(context.snapshotMetrics().externalCallsByCategory, { + "osv-query": 1, + }); + assert.equal(context.snapshotMetrics().cacheMisses, 1); + assert.equal(context.snapshotMetrics().cacheHits, 1); + + const cappedDiagnostics = {}; + const capped = await context.fetchJson("https://api.osv.dev/v1/query", { + endpointCategory: "osv-query", + method: "POST", + body: JSON.stringify({ id: "two" }), + fetchImpl, + maxCallsPerCategory: 1, + diagnostics: cappedDiagnostics, + }); + + assert.equal(capped.ok, false); + assert.equal(capped.reason, "call_cap"); + assert.equal(calls, 1); + assert.deepEqual(context.snapshotMetrics().cappedWorkByCategory, { + "osv-query_calls": 1, + }); + assert.equal(cappedDiagnostics.partialReason, "osv-query_call_cap"); +}); diff --git a/review-enrichment/test/history.test.ts b/review-enrichment/test/history.test.ts index cef7d73f82..38a16105f1 100644 --- a/review-enrichment/test/history.test.ts +++ b/review-enrichment/test/history.test.ts @@ -258,9 +258,10 @@ test("scanHistory: aborts slow GitHub subcalls and degrades instead of waiting f assert.equal(out.length, 1); assert.equal(out[0].partial, true); assert.ok(calls >= 1); - assert.equal(diagnostics.partialReason, "github_subcall_aborted"); + assert.equal(diagnostics.partialReason, "github-users_timeout"); assert.equal(diagnostics.captureDegradation, true); - assert.equal(diagnostics.githubEndpointCategory, "user"); + assert.equal(diagnostics.githubEndpointCategory, "github-users"); + assert.equal(diagnostics.endpointCategory, "github-users"); }); test("scanHistory: caps file and commit-to-PR fanout and records lookup counts", async () => { diff --git a/review-enrichment/test/request-guardrails.test.ts b/review-enrichment/test/request-guardrails.test.ts new file mode 100644 index 0000000000..ae7b2fc953 --- /dev/null +++ b/review-enrichment/test/request-guardrails.test.ts @@ -0,0 +1,153 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + MAX_BODY_BYTES, + parseEnrichRequestBody, + readEnrichRequestText, +} from "../dist/request-guardrails.js"; + +test("parseEnrichRequestBody accepts a minimal valid enrichment request", () => { + const result = parseEnrichRequestBody( + JSON.stringify({ + repoFullName: "JSONbored/gittensory", + prNumber: 1814, + files: [{ path: "src/a.ts", patch: "@@ -1,0 +1,1 @@\n+export const a = 1;" }], + }), + ); + + assert.equal(result.ok, true); + if (result.ok) { + assert.equal(result.payload.repoFullName, "JSONbored/gittensory"); + assert.equal(result.payload.prNumber, 1814); + assert.ok(result.bodyBytes > 0); + } +}); + +test("parseEnrichRequestBody rejects malformed JSON and invalid shallow schema", () => { + const malformed = parseEnrichRequestBody("{not json"); + assert.deepEqual(malformed, { + ok: false, + status: 400, + error: "bad_json", + bodyBytes: 9, + }); + + const badSchema = parseEnrichRequestBody(JSON.stringify({ repoFullName: "bad", prNumber: 0 })); + assert.equal(badSchema.ok, false); + if (!badSchema.ok) { + assert.equal(badSchema.status, 400); + assert.equal(badSchema.error, "bad_request"); + } +}); + +test("parseEnrichRequestBody rejects oversized body, file list, diff, and patch payloads", () => { + const hugeBody = parseEnrichRequestBody("x".repeat(2 * 1024 * 1024 + 1)); + assert.equal(hugeBody.ok, false); + if (!hugeBody.ok) { + assert.equal(hugeBody.status, 413); + assert.equal(hugeBody.error, "request_too_large"); + } + + const tooManyFiles = parseEnrichRequestBody( + JSON.stringify({ + repoFullName: "JSONbored/gittensory", + prNumber: 1814, + files: Array.from({ length: 301 }, (_, index) => ({ path: `src/${index}.ts` })), + }), + ); + assert.equal(tooManyFiles.ok, false); + if (!tooManyFiles.ok) assert.equal(tooManyFiles.error, "too_many_files"); + + const hugeDiff = parseEnrichRequestBody( + JSON.stringify({ + repoFullName: "JSONbored/gittensory", + prNumber: 1814, + diff: "x".repeat(1_000_001), + }), + ); + assert.equal(hugeDiff.ok, false); + if (!hugeDiff.ok) assert.equal(hugeDiff.error, "diff_too_large"); + + const hugePatch = parseEnrichRequestBody( + JSON.stringify({ + repoFullName: "JSONbored/gittensory", + prNumber: 1814, + files: [{ path: "src/a.ts", patch: "x".repeat(1_500_001) }], + }), + ); + assert.equal(hugePatch.ok, false); + if (!hugePatch.ok) assert.equal(hugePatch.error, "patches_too_large"); +}); + +test("readEnrichRequestText rejects an oversized Content-Length without reading the body", async () => { + const body = new ReadableStream({ + pull(controller) { + controller.enqueue(new Uint8Array([123])); + controller.close(); + }, + }); + const request = new Request("https://rees.example/v1/enrich", { + method: "POST", + headers: { "content-length": String(MAX_BODY_BYTES + 1) }, + body, + duplex: "half", + } as RequestInit); + + const result = await readEnrichRequestText(request); + + assert.equal(result.ok, false); + if (!result.ok) { + assert.equal(result.status, 413); + assert.equal(result.error, "request_too_large"); + assert.equal(result.bodyBytes, MAX_BODY_BYTES + 1); + } + assert.equal(request.bodyUsed, false); +}); + +test("readEnrichRequestText stops streaming once the request body exceeds the cap", async () => { + let pulls = 0; + let canceled = false; + const body = new ReadableStream({ + pull(controller) { + pulls += 1; + controller.enqueue(new Uint8Array(1024 * 1024)); + if (pulls > 5) controller.close(); + }, + cancel() { + canceled = true; + }, + }); + const request = new Request("https://rees.example/v1/enrich", { + method: "POST", + body, + duplex: "half", + } as RequestInit); + + const result = await readEnrichRequestText(request); + + assert.equal(result.ok, false); + if (!result.ok) { + assert.equal(result.status, 413); + assert.equal(result.error, "request_too_large"); + assert.ok(result.bodyBytes > MAX_BODY_BYTES); + } + assert.equal(canceled, true); + assert.ok(pulls < 6); +}); + +test("readEnrichRequestText returns a small request body", async () => { + const raw = JSON.stringify({ repoFullName: "JSONbored/gittensory", prNumber: 1836 }); + const request = new Request("https://rees.example/v1/enrich", { + method: "POST", + body: raw, + }); + + const result = await readEnrichRequestText(request); + + assert.equal(result.ok, true); + if (result.ok) { + assert.equal(result.raw, raw); + assert.equal(result.bodyBytes, new TextEncoder().encode(raw).byteLength); + } +}); diff --git a/review-enrichment/test/scheduler.test.ts b/review-enrichment/test/scheduler.test.ts new file mode 100644 index 0000000000..c562479651 --- /dev/null +++ b/review-enrichment/test/scheduler.test.ts @@ -0,0 +1,180 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { buildBrief } from "../dist/brief.js"; + +test("fast profile skips GitHub-heavy defaults without running them", async () => { + let ran = false; + const brief = await buildBrief( + { + repoFullName: "JSONbored/gittensory", + prNumber: 1811, + profile: "fast", + githubToken: "token", + author: "jsonbored", + headSha: "abcdef1234567890", + files: [{ path: "src/a.ts", patch: "@@ -1,0 +1,1 @@\n+export const a = 1;" }], + }, + { + history: async () => { + ran = true; + return []; + }, + }, + ); + + assert.equal(ran, false); + assert.equal(brief.partial, false); + assert.equal(brief.analyzerStatus.history, "skipped"); +}); + +test("explicit analyzer selection overrides profile membership while retaining bounded budgets", async () => { + let sawProfile = ""; + let sawCostClass = ""; + let sawTimeoutMs = 0; + const brief = await buildBrief( + { + repoFullName: "JSONbored/gittensory", + prNumber: 1811, + profile: "fast", + analyzers: ["history"], + githubToken: "token", + author: "jsonbored", + headSha: "abcdef1234567890", + files: [{ path: "src/a.ts", patch: "@@ -1,0 +1,1 @@\n+export const a = 1;" }], + budget: { timeoutMs: 2000 }, + }, + { + history: async (_req, context) => { + sawProfile = context.profile; + sawCostClass = context.costClass; + sawTimeoutMs = context.timeoutMs; + return []; + }, + }, + ); + + assert.equal(brief.analyzerStatus.history, "ok"); + assert.equal(sawProfile, "fast"); + assert.equal(sawCostClass, "github-heavy"); + assert.ok(sawTimeoutMs > 0); + assert.ok(sawTimeoutMs < 2000); +}); + +test("slow analyzers time out inside the reserved response budget", async () => { + const started = Date.now(); + const brief = await buildBrief( + { + repoFullName: "JSONbored/gittensory", + prNumber: 1811, + analyzers: ["history"], + githubToken: "token", + author: "jsonbored", + headSha: "abcdef1234567890", + files: [{ path: "src/a.ts", patch: "@@ -1,0 +1,1 @@\n+export const a = 1;" }], + budget: { timeoutMs: 300 }, + }, + { + history: async () => new Promise(() => undefined), + }, + ); + + assert.equal(brief.partial, true); + assert.equal(brief.analyzerStatus.history, "timeout"); + assert.equal(brief.telemetry.profile, "balanced"); + assert.equal(brief.telemetry.requestedAnalyzers[0], "history"); + assert.equal(brief.telemetry.analyzers.history.status, "timeout"); + assert.equal(brief.telemetry.analyzers.history.partialReason, "analyzer_timeout"); + assert.ok((brief.telemetry.analyzers.history.timeoutMs ?? 0) < 300); + assert.ok(brief.telemetry.responseReserveMs > 0); + assert.ok(Date.now() - started < 1000); + assert.ok(brief.elapsedMs < 1000); +}); + +test("cost classes run in priority order instead of starting all at once", async () => { + const events: string[] = []; + + const brief = await buildBrief( + { + repoFullName: "JSONbored/gittensory", + prNumber: 1811, + analyzers: ["secret", "dependency", "history"], + githubToken: "token", + author: "jsonbored", + headSha: "abcdef1234567890", + files: [ + { + path: "package.json", + patch: [ + "@@ -1,3 +1,4 @@", + ' { "dependencies": {', + '+ "left-pad": "1.3.0",', + '+ "apiKey": "test"', + ].join("\n"), + }, + ], + budget: { timeoutMs: 2000 }, + }, + { + secret: async () => { + events.push("local:start"); + await new Promise((resolve) => setTimeout(resolve, 10)); + events.push("local:end"); + return []; + }, + dependency: async () => { + events.push("registry:start"); + assert.deepEqual(events, ["local:start", "local:end", "registry:start"]); + events.push("registry:end"); + return []; + }, + history: async () => { + events.push("github-heavy:start"); + assert.deepEqual(events, [ + "local:start", + "local:end", + "registry:start", + "registry:end", + "github-heavy:start", + ]); + events.push("github-heavy:end"); + return []; + }, + }, + ); + + assert.equal(brief.partial, false); + assert.deepEqual(events, [ + "local:start", + "local:end", + "registry:start", + "registry:end", + "github-heavy:start", + "github-heavy:end", + ]); +}); + +test("registry analyzers skip when their relevant inputs are absent", async () => { + let dependencyRan = false; + const brief = await buildBrief( + { + repoFullName: "JSONbored/gittensory", + prNumber: 1811, + files: [{ path: "src/a.ts", patch: "@@ -1,0 +1,1 @@\n+export const a = 1;" }], + }, + { + dependency: async () => { + dependencyRan = true; + return []; + }, + secret: async () => [], + }, + ); + + assert.equal(dependencyRan, false); + assert.equal(brief.analyzerStatus.dependency, "skipped"); + assert.equal(brief.analyzerStatus.secret, "ok"); + assert.equal(brief.telemetry.analyzers.dependency.skipReason, "no_dependency_manifest"); + assert.equal(brief.telemetry.analyzers.secret.status, "ok"); + assert.ok(brief.telemetry.skippedWorkByCategory.analyzer_no_dependency_manifest >= 1); +}); diff --git a/review-enrichment/test/sentry-degradation.test.ts b/review-enrichment/test/sentry-degradation.test.ts index 9b35babc36..75195b4b58 100644 --- a/review-enrichment/test/sentry-degradation.test.ts +++ b/review-enrichment/test/sentry-degradation.test.ts @@ -130,10 +130,16 @@ test("captureAnalyzerDegradation attaches safe attribution context for history f timeoutMs: 7000, elapsedMs: 6812, analyzerStatus: "degraded", + profile: "balanced", + costClass: "github-heavy", + responseReserveMs: 750, partialStatus: "partial", partialReason: "history_budget_exhausted", phase: "similar_past_prs", subcall: "commit_pulls", + endpointCategory: "github-commit-pulls", + externalFailureReason: "timeout", + externalElapsedMs: 1200, fileLookupCount: 5, commitLookupCount: 13, prLookupCount: 12, @@ -158,8 +164,13 @@ test("captureAnalyzerDegradation attaches safe attribution context for history f assert.equal(sentry.tags.headShaPrefix, "abcdef123456"); assert.equal(sentry.tags.timeoutMs, "7000"); assert.equal(sentry.tags.analyzerStatus, "degraded"); + assert.equal(sentry.tags.profile, "balanced"); + assert.equal(sentry.tags.costClass, "github-heavy"); + assert.equal(sentry.tags.responseReserveMs, "750"); assert.equal(sentry.tags.partialStatus, "partial"); assert.equal(sentry.tags.phase, "similar_past_prs"); + assert.equal(sentry.tags.endpointCategory, "github-commit-pulls"); + assert.equal(sentry.tags.externalFailureReason, "timeout"); assert.equal(sentry.tags.githubEndpointCategory, "commit_pulls"); assert.equal(sentry.tags.cacheHits, "4"); assert.equal(sentry.tags.cacheMisses, "9"); @@ -175,10 +186,16 @@ test("captureAnalyzerDegradation attaches safe attribution context for history f timeoutMs: 7000, elapsedMs: 6812, analyzerStatus: "degraded", + profile: "balanced", + costClass: "github-heavy", + responseReserveMs: 750, partialStatus: "partial", partialReason: "history_budget_exhausted", phase: "similar_past_prs", subcall: "commit_pulls", + endpointCategory: "github-commit-pulls", + externalFailureReason: "timeout", + externalElapsedMs: 1200, fileLookupCount: 5, commitLookupCount: 13, prLookupCount: 12, @@ -204,17 +221,20 @@ test("captureAnalyzerDegradation attaches safe attribution context for history f test("buildBrief stays fail-open and captures a degraded analyzer", async () => { const sentry = sentryHarness(); + const fakeToken = ["ghp", "abcdefghijklmnopqrstuvwxyz1234567890"].join("_"); const brief = await buildBrief( { repoFullName: "JSONbored/gittensory", prNumber: 42, headSha: "head-sha", - budget: { timeoutMs: 50 }, + analyzers: ["dependency"], + files: [{ path: "package.json", patch: '+ "lodash": "4.17.20",' }], + budget: { timeoutMs: 200 }, }, { dependency: async () => { - throw new Error("osv unavailable"); + throw new Error(`osv unavailable for ${fakeToken}`); }, }, ); @@ -224,23 +244,58 @@ test("buildBrief stays fail-open and captures a degraded analyzer", async () => assert.deepEqual(brief.findings, {}); assert.equal(brief.repoFullName, "JSONbored/gittensory"); assert.equal(brief.prNumber, 42); + assert.equal(brief.telemetry.analyzers.dependency.partialReason, "analyzer_error"); + assert.equal(JSON.stringify(brief.telemetry).includes(fakeToken), false); + assert.equal(JSON.stringify(brief.telemetry).includes("osv unavailable"), false); assert.equal(sentry.captured.length, 1); - assert.equal(sentry.captured[0].message, "osv unavailable"); + assert.equal(sentry.captured[0].message, "analyzer_error"); assert.equal(sentry.tags.analyzer, "dependency"); assert.equal(sentry.tags.repo, "JSONbored/gittensory"); assert.equal(sentry.tags.pullNumber, "42"); assert.equal(sentry.tags.headShaPrefix, "head-sha"); - assert.equal(sentry.tags.timeoutMs, "50"); + const capturedTimeoutMs = Number(sentry.tags.timeoutMs); + assert.ok(capturedTimeoutMs > 0); + assert.ok(capturedTimeoutMs <= 200); +}); + +test("buildBrief normalizes unsafe analyzer partial reasons before response telemetry", async () => { + const fakeToken = ["ghp", "abcdefghijklmnopqrstuvwxyz1234567890"].join("_"); + + const brief = await buildBrief( + { + repoFullName: "JSONbored/gittensory", + prNumber: 42, + analyzers: ["history"], + linkedIssue: { number: 9, title: "add history context" }, + diff: `+${fakeToken}`, + budget: { timeoutMs: 200 }, + }, + { + history: async (_req, context) => { + context.diagnostics.partialReason = `unsafe ${fakeToken}`; + return [{ author: null, similarPastPrs: [], linkedIssueAlignment: null, partial: true }]; + }, + }, + ); + + assert.equal(brief.partial, true); + assert.equal(brief.analyzerStatus.history, "degraded"); + assert.equal(brief.telemetry.analyzers.history.partialReason, "analyzer_partial"); + assert.equal(JSON.stringify(brief.telemetry).includes(fakeToken), false); }); -test("buildBrief returns a degraded partial response before the caller timeout budget is spent", async () => { +test("buildBrief returns a timed-out partial response before the caller timeout budget is spent", async () => { const started = Date.now(); const brief = await buildBrief( { repoFullName: "JSONbored/metagraphed", prNumber: 2359, headSha: "abcdef1234567890", - budget: { timeoutMs: 20 }, + analyzers: ["history"], + githubToken: "token", + author: "jsonbored", + files: [{ path: "src/a.ts", patch: "@@ -1,0 +1,1 @@\n+export const a = 1;" }], + budget: { timeoutMs: 300 }, }, { history: async () => new Promise(() => undefined), @@ -249,7 +304,7 @@ test("buildBrief returns a degraded partial response before the caller timeout b ); assert.equal(brief.partial, true); - assert.equal(brief.analyzerStatus.history, "degraded"); + assert.equal(brief.analyzerStatus.history, "timeout"); assert.deepEqual(brief.findings, {}); assert.ok(Date.now() - started < 500); assert.ok(brief.elapsedMs < 500); diff --git a/src/review/enrichment-wire.ts b/src/review/enrichment-wire.ts index c1cbb139ce..e0bb183e9c 100644 --- a/src/review/enrichment-wire.ts +++ b/src/review/enrichment-wire.ts @@ -16,6 +16,7 @@ interface EnrichmentEnv { REES_SHARED_SECRET?: string | undefined; REES_TIMEOUT_MS?: string | undefined; REES_ANALYZERS?: string | undefined; + REES_PROFILE?: string | undefined; REES_FORWARD_GITHUB_TOKEN?: string | undefined; } @@ -94,6 +95,9 @@ export const REES_ANALYZER_NAMES = [ ] as const; const REES_ANALYZER_NAME_SET = new Set(REES_ANALYZER_NAMES); +const REES_PROFILE_NAMES = ["fast", "balanced", "deep"] as const; +type ReesProfileName = (typeof REES_PROFILE_NAMES)[number]; +const REES_PROFILE_NAME_SET = new Set(REES_PROFILE_NAMES); function sanitizeEnrichmentPromptSection(value: unknown): string | undefined { if (typeof value !== "string") return undefined; @@ -178,6 +182,21 @@ export function resolveReesAnalyzers(env: Env): string[] | undefined { return selected; } +export function resolveReesProfile(env: Env): ReesProfileName | undefined { + const raw = reesConfig(env).REES_PROFILE?.trim(); + if (!raw) return undefined; + const normalized = raw.toLowerCase(); + if (REES_PROFILE_NAME_SET.has(normalized)) return normalized as ReesProfileName; + console.warn( + JSON.stringify({ + level: "warn", + event: "rees_profile_config_invalid", + profile: raw.slice(0, 40), + }), + ); + return undefined; +} + /** POST the PR to the REES and return the spliceable brief, or undefined on any error/timeout/empty (fail-safe). */ export async function buildReviewEnrichment( env: Env, @@ -195,6 +214,7 @@ export async function buildReviewEnrichment( const timeoutMs = resolveReesTransportTimeoutMs(cfg.REES_TIMEOUT_MS); const analyzerBudgetMs = resolveReesAnalyzerBudgetMs(timeoutMs); const analyzers = resolveReesAnalyzers(env); + const profile = resolveReesProfile(env); const requestId = newReesRequestId(); try { const response = await fetch(`${base.replace(/\/+$/, "")}/v1/enrich`, { @@ -225,6 +245,7 @@ export async function buildReviewEnrichment( })), diff: input.diff, ...(analyzers ? { analyzers } : {}), + ...(profile ? { profile } : {}), budget: { timeoutMs: analyzerBudgetMs, maxBriefChars: MAX_ENRICHMENT_PROMPT_SECTION_CHARS, @@ -249,6 +270,7 @@ export async function buildReviewEnrichment( requestId, timeoutMs, analyzerBudgetMs, + reesProfile: profile ?? "default", requestedAnalyzers: analyzers ?? "all", authConfigured, authHeaderSent: authConfigured, @@ -296,6 +318,7 @@ export async function buildReviewEnrichment( requestId, timeoutMs, analyzerBudgetMs, + reesProfile: profile ?? "default", requestedAnalyzers: analyzers ?? "all", authConfigured, authHeaderSent: authConfigured, diff --git a/test/unit/enrichment-wire.test.ts b/test/unit/enrichment-wire.test.ts index 82cafae732..0e5ea5c198 100644 --- a/test/unit/enrichment-wire.test.ts +++ b/test/unit/enrichment-wire.test.ts @@ -5,6 +5,7 @@ import { isReesGithubTokenForwardingEnabled, resolveReesAnalyzers, resolveReesAnalyzerBudgetMs, + resolveReesProfile, resolveReesTransportTimeoutMs, } from "../../src/review/enrichment-wire"; @@ -108,6 +109,7 @@ describe("buildReviewEnrichment", () => { expect(body.author).toBe("alice"); expect(body.githubToken).toBe("gh-read-token"); expect(body.analyzers).toBeUndefined(); + expect(body.profile).toBeUndefined(); expect(body.budget).toEqual({ timeoutMs: 11000, maxBriefChars: 8000 }); expect(body.files).toEqual([ { @@ -174,6 +176,24 @@ describe("buildReviewEnrichment", () => { ]); }); + it("sends a configured REES profile when no explicit analyzer subset is required", async () => { + const calls: RequestInit[] = []; + globalThis.fetch = vi.fn(async (_url: unknown, init: RequestInit) => { + calls.push(init); + return { + ok: true, + json: async () => ({ promptSection: "brief" }), + } as Response; + }) as unknown as typeof fetch; + await buildReviewEnrichment( + env({ REES_URL: "https://r", REES_PROFILE: " fast " }), + input, + ); + const body = JSON.parse(calls[0]!.body as string); + expect(body.profile).toBe("fast"); + expect(body.analyzers).toBeUndefined(); + }); + it("sends an explicit empty analyzer list when REES_ANALYZERS has no valid names", async () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const calls: RequestInit[] = []; @@ -516,6 +536,31 @@ describe("resolveReesAnalyzers", () => { }); }); +describe("resolveReesProfile", () => { + it("returns undefined for unset profiles", () => { + expect(resolveReesProfile(env({}))).toBeUndefined(); + }); + + it("normalizes supported profile names", () => { + expect(resolveReesProfile(env({ REES_PROFILE: " FAST " }))).toBe("fast"); + expect(resolveReesProfile(env({ REES_PROFILE: "balanced" }))).toBe("balanced"); + expect(resolveReesProfile(env({ REES_PROFILE: "Deep" }))).toBe("deep"); + }); + + it("warns and omits unsupported profiles", () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect(resolveReesProfile(env({ REES_PROFILE: "everything" }))).toBeUndefined(); + expect( + warnSpy.mock.calls.some( + (c) => + String(c[0]).includes("rees_profile_config_invalid") && + String(c[0]).includes("everything"), + ), + ).toBe(true); + warnSpy.mockRestore(); + }); +}); + describe("REES timeout budget helpers", () => { it("keeps analyzer execution below the HTTP transport timeout", () => { expect(resolveReesTransportTimeoutMs(undefined)).toBe(8000);