From edea2d5753cb8dd69398f7e5c97e5079f01c012b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 29 Jun 2026 22:30:07 -0700 Subject: [PATCH] perf(rees): share analyzer request context --- review-enrichment/README.md | 19 + review-enrichment/src/analysis-context.ts | 353 ++++++++++++++++++ .../src/analyzers/dependency-scan.ts | 61 ++- .../src/analyzers/dependency/descriptor.ts | 25 +- .../src/analyzers/secret-scan.ts | 21 ++ .../src/analyzers/secret/descriptor.ts | 5 +- review-enrichment/src/analyzers/types.ts | 2 + review-enrichment/src/brief.ts | 32 ++ review-enrichment/src/sentry.ts | 16 + review-enrichment/src/types.ts | 15 + .../test/analysis-context.test.ts | 188 ++++++++++ .../test/sentry-degradation.test.ts | 14 + 12 files changed, 728 insertions(+), 23 deletions(-) create mode 100644 review-enrichment/src/analysis-context.ts create mode 100644 review-enrichment/test/analysis-context.test.ts diff --git a/review-enrichment/README.md b/review-enrichment/README.md index 4d1b417045..7859bb96c0 100644 --- a/review-enrichment/README.md +++ b/review-enrichment/README.md @@ -70,6 +70,25 @@ classes, per-analyzer limits, and self-host configuration. When adding or migrat - Make external-call analyzers fail open and respect the orchestrator abort signal when the scanner supports it. - Prefer a focused analyzer test file instead of expanding the shared `enrichment.test.ts` mega-test. +## Shared analysis context + +Each `/v1/enrich` request now gets a request-scoped `AnalysisContext` before analyzers run. New and migrated +analyzers should prefer it for shared PR facts instead of reparsing the envelope: + +| Context surface | Purpose | +| --------------- | ------- | +| `changedFiles` / `changedFilePaths` | The request's changed file list and paths. | +| `addedLines` | Unified-diff added lines with file and new-line number tracking. | +| `patchHunks` | Parsed hunk locations for analyzers that need bounded line-aware scans. | +| `fileCategories` | Coarse public-safe file categories used for fast filtering. | +| `dependencyChanges()` / `packageChanges()` | Cached direct package changes from changed manifests. | +| `cachedExternalCall(category, key, load)` | Request-scoped in-flight de-duplication for identical external lookups. | + +Context caches are request-scoped only. They are for avoiding duplicate work inside one enrichment run, not for +cross-request TTL storage. Cache metrics are aggregate and public-safe: hit/miss counts, external-call counts by +category, skipped/capped work counts by category, and elapsed time. Never put request bodies, diffs, prompts, +comments, tokens, private configs, or raw external payloads into cache categories, metric keys, Sentry tags, or logs. + The engine also sends `budget.timeoutMs` with one second of headroom below `REES_TIMEOUT_MS`, so REES can return a partial/degraded brief before the caller aborts the HTTP request. If Railway is still running an older REES build, temporarily raise the engine-side `REES_TIMEOUT_MS` above the REES analyzer budget, or set `REES_ANALYZERS` to a diff --git a/review-enrichment/src/analysis-context.ts b/review-enrichment/src/analysis-context.ts new file mode 100644 index 0000000000..1a1564e653 --- /dev/null +++ b/review-enrichment/src/analysis-context.ts @@ -0,0 +1,353 @@ +import type { AnalyzerMetricsDiagnostics, EnrichRequest } from "./types.js"; +import { + extractDependencyChanges, + type DepChange, + type ScanLimits, +} from "./analyzers/dependency-scan.js"; + +type ChangedFile = NonNullable[number]; + +export interface AddedLine { + file: string; + line: number; + text: string; +} + +export interface PatchHunk { + file: string; + oldStart: number; + oldLines: number; + newStart: number; + newLines: number; +} + +export interface FileCategory { + path: string; + extension: string; + category: + | "dependency-manifest" + | "lockfile" + | "workflow" + | "config" + | "asset" + | "docs" + | "source" + | "unknown"; +} + +export interface RepoIdentity { + owner: string | null; + repo: string | null; + fullName: string; + prNumber: number; + headSha: string | null; +} + +export interface AnalysisContextMetrics extends AnalyzerMetricsDiagnostics { + externalCallsByCategory: Record; + skippedWorkByCategory: Record; + cappedWorkByCategory: Record; +} + +export interface AnalysisContext { + repo: RepoIdentity; + changedFiles: readonly ChangedFile[]; + changedFilePaths: readonly string[]; + addedLines: readonly AddedLine[]; + patchHunks: readonly PatchHunk[]; + fileCategories: readonly FileCategory[]; + dependencyManifestPaths: readonly string[]; + cache: RequestScopedCache; + metrics: AnalysisMetrics; + cachedExternalCall( + category: string, + key: string, + load: () => Promise, + ): Promise; + dependencyChanges(limits?: ScanLimits): readonly DepChange[]; + packageChanges(limits?: ScanLimits): readonly DepChange[]; + remainingMs(deadlineMs?: number): number; + snapshotMetrics(): AnalysisContextMetrics; +} + +export class AnalysisMetrics { + cacheHits = 0; + cacheMisses = 0; + externalCallsByCategory: Record = {}; + skippedWorkByCategory: Record = {}; + cappedWorkByCategory: Record = {}; + + constructor( + private readonly startedAtMs: number, + private readonly now: () => number, + ) {} + + recordCacheHit(count = 1): void { + this.cacheHits += Math.max(0, count); + } + + recordCacheMiss(count = 1): void { + this.cacheMisses += Math.max(0, count); + } + + recordExternalCall(category: string, count = 1): void { + incrementByCategory(this.externalCallsByCategory, category, count); + } + + recordSkippedWork(category: string, count = 1): void { + incrementByCategory(this.skippedWorkByCategory, category, count); + } + + recordCappedWork(category: string, count = 1): void { + incrementByCategory(this.cappedWorkByCategory, category, count); + } + + snapshot(): AnalysisContextMetrics { + return { + cacheHits: this.cacheHits, + cacheMisses: this.cacheMisses, + externalCallsByCategory: { ...this.externalCallsByCategory }, + skippedWorkByCategory: { ...this.skippedWorkByCategory }, + cappedWorkByCategory: { ...this.cappedWorkByCategory }, + analysisElapsedMs: Math.max(0, Math.floor(this.now() - this.startedAtMs)), + }; + } +} + +export class RequestScopedCache { + private readonly entries = new Map>(); + + constructor(private readonly metrics: AnalysisMetrics) {} + + get size(): number { + return this.entries.size; + } + + getOrSet( + category: string, + key: string, + load: () => Promise, + ): Promise { + const cacheKey = requestCacheKey(category, key); + const existing = this.entries.get(cacheKey); + if (existing) { + this.metrics.recordCacheHit(); + return existing as Promise; + } + this.metrics.recordCacheMiss(); + const promise = Promise.resolve() + .then(load) + .catch((error) => { + this.entries.delete(cacheKey); + throw error; + }); + this.entries.set(cacheKey, promise); + return promise; + } +} + +function requestCacheKey(category: string, key: string): string { + return JSON.stringify([safeMetricCategory(category), key]); +} + +export function createAnalysisContext( + req: EnrichRequest, + options: { startedAtMs?: number; deadlineMs?: number; now?: () => number } = {}, +): AnalysisContext { + const now = options.now ?? Date.now; + const startedAtMs = options.startedAtMs ?? now(); + const changedFiles = req.files ?? []; + const metrics = new AnalysisMetrics(startedAtMs, now); + const cache = new RequestScopedCache(metrics); + const dependencyChangeCache = new Map(); + const fileCategories = changedFiles.map((file) => categorizeFile(file.path)); + const dependencyManifestPaths = fileCategories + .filter((file) => file.category === "dependency-manifest") + .map((file) => file.path); + + const context: AnalysisContext = { + repo: parseRepoIdentity(req), + changedFiles, + changedFilePaths: changedFiles.map((file) => file.path), + addedLines: collectAddedLines(changedFiles), + patchHunks: collectPatchHunks(changedFiles), + fileCategories, + dependencyManifestPaths, + cache, + metrics, + cachedExternalCall(category, key, load) { + return cache.getOrSet(category, key, () => { + metrics.recordExternalCall(category); + return load(); + }); + }, + dependencyChanges(limits: ScanLimits = {}) { + const key = dependencyLimitKey(limits); + const cached = dependencyChangeCache.get(key); + if (cached) { + metrics.recordCacheHit(); + return cached; + } + metrics.recordCacheMiss(); + if ( + typeof limits.maxManifestFiles === "number" && + dependencyManifestPaths.length > limits.maxManifestFiles + ) { + metrics.recordCappedWork( + "dependency_manifest_files", + dependencyManifestPaths.length - limits.maxManifestFiles, + ); + } + const extracted = extractDependencyChanges(changedFiles, limits); + const maxDependencyQueries = limits.maxDependencyQueries; + const changes = + typeof maxDependencyQueries === "number" + ? extracted.slice(0, maxDependencyQueries) + : extracted; + if ( + typeof maxDependencyQueries === "number" && + extracted.length > maxDependencyQueries + ) { + metrics.recordCappedWork( + "dependency_queries", + extracted.length - maxDependencyQueries, + ); + } + dependencyChangeCache.set(key, changes); + return changes; + }, + packageChanges(limits: ScanLimits = {}) { + return context.dependencyChanges(limits); + }, + remainingMs(deadlineMs = options.deadlineMs) { + if (typeof deadlineMs !== "number") return Number.POSITIVE_INFINITY; + return Math.max(0, deadlineMs - now()); + }, + snapshotMetrics() { + return metrics.snapshot(); + }, + }; + + return context; +} + +export function collectAddedLines(files: readonly ChangedFile[]): AddedLine[] { + const addedLines: AddedLine[] = []; + for (const file of files) { + if (!file.patch) continue; + let newLine = 0; + for (const line of file.patch.split("\n")) { + if (line.startsWith("+++") || line.startsWith("---")) continue; + if (line.startsWith("diff ") || line.startsWith("index ")) continue; + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + continue; + } + if (line.startsWith("+")) { + addedLines.push({ file: file.path, line: newLine, text: line.slice(1) }); + newLine += 1; + } else if (!line.startsWith("-")) { + newLine += 1; + } + } + } + return addedLines; +} + +export function collectPatchHunks(files: readonly ChangedFile[]): PatchHunk[] { + const hunks: PatchHunk[] = []; + for (const file of files) { + if (!file.patch) continue; + for (const line of file.patch.split("\n")) { + const hunk = + /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (!hunk) continue; + hunks.push({ + file: file.path, + oldStart: Number(hunk[1]), + oldLines: hunk[2] ? Number(hunk[2]) : 1, + newStart: Number(hunk[3]), + newLines: hunk[4] ? Number(hunk[4]) : 1, + }); + } + } + return hunks; +} + +function parseRepoIdentity(req: EnrichRequest): RepoIdentity { + const parts = req.repoFullName.split("/"); + const owner = parts.length === 2 && parts[0] ? parts[0] : null; + const repo = parts.length === 2 && parts[1] ? parts[1] : null; + return { + owner, + repo, + fullName: req.repoFullName, + prNumber: req.prNumber, + headSha: req.headSha ?? null, + }; +} + +function categorizeFile(path: string): FileCategory { + const basename = path.split("/").pop() ?? path; + const extension = extensionOf(basename); + if (["package.json", "requirements.txt", "go.mod"].includes(basename)) { + return { path, extension, category: "dependency-manifest" }; + } + if ( + ["package-lock.json", "yarn.lock", "pnpm-lock.yaml", "poetry.lock", "go.sum"].includes( + basename, + ) + ) { + return { path, extension, category: "lockfile" }; + } + if (path.startsWith(".github/workflows/")) { + return { path, extension, category: "workflow" }; + } + if ( + /^Dockerfile(?:\..*)?$/.test(basename) || + [".env", ".ini", ".json", ".toml", ".yaml", ".yml"].includes(extension) + ) { + return { path, extension, category: "config" }; + } + if ([".md", ".mdx", ".rst", ".txt"].includes(extension)) { + return { path, extension, category: "docs" }; + } + if ( + [".png", ".jpg", ".jpeg", ".gif", ".webp", ".svg", ".pdf", ".zip", ".gz"].includes( + extension, + ) + ) { + return { path, extension, category: "asset" }; + } + if (extension) return { path, extension, category: "source" }; + return { path, extension, category: "unknown" }; +} + +function extensionOf(basename: string): string { + const index = basename.lastIndexOf("."); + if (index <= 0) return ""; + return basename.slice(index).toLowerCase(); +} + +function dependencyLimitKey(limits: ScanLimits): string { + return [ + limits.maxManifestFiles ?? "", + limits.maxPatchLinesPerFile ?? "", + limits.maxDependencyQueries ?? "", + ].join(":"); +} + +function incrementByCategory( + target: Record, + category: string, + count: number, +): void { + const safeCategory = safeMetricCategory(category); + target[safeCategory] = (target[safeCategory] ?? 0) + Math.max(0, count); +} + +function safeMetricCategory(category: string): string { + const safe = category.replace(/[^A-Za-z0-9_.:-]+/g, "_").slice(0, 80); + return safe || "unknown"; +} diff --git a/review-enrichment/src/analyzers/dependency-scan.ts b/review-enrichment/src/analyzers/dependency-scan.ts index 7abe465d2e..f1f87edf53 100644 --- a/review-enrichment/src/analyzers/dependency-scan.ts +++ b/review-enrichment/src/analyzers/dependency-scan.ts @@ -2,8 +2,9 @@ // 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 { AnalysisContext } from "../analysis-context.js"; -interface DepChange { +export interface DepChange { ecosystem: string; package: string; from: string | null; @@ -14,15 +15,18 @@ const MAX_MANIFEST_FILES = 20; const MAX_PATCH_LINES_PER_FILE = 500; const MAX_DEPENDENCY_QUERIES = 25; -interface ScanLimits { +export interface ScanLimits { maxManifestFiles?: number; maxPatchLinesPerFile?: number; maxDependencyQueries?: number; } +type ExternalCallCache = Pick; + interface ScanOptions { signal?: AbortSignal; limits?: ScanLimits; + cache?: ExternalCallCache; } // Per-manifest line parsers. Each returns [name, version] for a `+`/`-` diff line, or null. Heuristic (line-based, @@ -181,26 +185,42 @@ export async function queryOsv( })); } -/** Analyzer entrypoint: changed deps → OSV → only the deps that carry vulnerabilities. */ -export async function scanDependencies( - req: EnrichRequest, +function osvCacheKey(change: DepChange): string { + return `${change.ecosystem}:${change.package}:${change.to}`; +} + +async function queryOsvForChange( + change: DepChange, + fetchImpl: typeof fetch, + options: ScanOptions, +): Promise { + const load = () => + queryOsv( + change.ecosystem, + change.package, + change.to, + fetchImpl, + options.signal, + ); + return options.cache + ? options.cache.cachedExternalCall("osv", osvCacheKey(change), load) + : load(); +} + +/** Scan already-extracted dependency changes → OSV → only the deps that carry vulnerabilities. */ +export async function scanDependencyChanges( + changes: readonly DepChange[], fetchImpl: typeof fetch = fetch, options: ScanOptions = {}, ): Promise { - const changes = extractDependencyChanges(req.files ?? [], options.limits).slice( + const boundedChanges = changes.slice( 0, options.limits?.maxDependencyQueries ?? MAX_DEPENDENCY_QUERIES, ); const findings: DependencyFinding[] = []; - for (const change of changes) { + for (const change of boundedChanges) { if (options.signal?.aborted) break; - const cves = await queryOsv( - change.ecosystem, - change.package, - change.to, - fetchImpl, - options.signal, - ); + const cves = await queryOsvForChange(change, fetchImpl, options); if (cves.length) { findings.push({ ...change, @@ -211,3 +231,16 @@ export async function scanDependencies( } return findings; } + +/** Analyzer entrypoint: changed deps → OSV → only the deps that carry vulnerabilities. */ +export async function scanDependencies( + req: EnrichRequest, + fetchImpl: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise { + return scanDependencyChanges( + extractDependencyChanges(req.files ?? [], options.limits), + fetchImpl, + options, + ); +} diff --git a/review-enrichment/src/analyzers/dependency/descriptor.ts b/review-enrichment/src/analyzers/dependency/descriptor.ts index b42b7b526f..78e73bc445 100644 --- a/review-enrichment/src/analyzers/dependency/descriptor.ts +++ b/review-enrichment/src/analyzers/dependency/descriptor.ts @@ -1,7 +1,13 @@ import type { AnalyzerDescriptor } from "../types.js"; -import { scanDependencies } from "../dependency-scan.js"; +import { scanDependencyChanges, type ScanLimits } from "../dependency-scan.js"; import { SEVERITY_RANK } from "../../render-helpers.js"; +const DEPENDENCY_LIMITS = { + maxManifestFiles: 20, + maxPatchLinesPerFile: 500, + maxDependencyQueries: 25, +} satisfies ScanLimits; + export const dependencyAnalyzer: AnalyzerDescriptor<"dependency"> = { name: "dependency", title: "Dependency vulnerabilities", @@ -9,11 +15,7 @@ export const dependencyAnalyzer: AnalyzerDescriptor<"dependency"> = { cost: "registry", defaultEnabled: true, requires: ["files", "public-network"], - limits: { - maxManifestFiles: 20, - maxPatchLinesPerFile: 500, - maxDependencyQueries: 25, - }, + limits: DEPENDENCY_LIMITS, docs: { summary: "Checks changed direct dependency versions against OSV.dev.", looksAt: @@ -24,7 +26,16 @@ export const dependencyAnalyzer: AnalyzerDescriptor<"dependency"> = { notes: "Manifest-only by design; use lockfileDrift for transitive lockfile changes.", }, - run: (req, { signal }) => scanDependencies(req, fetch, { signal }), + run: (_req, { signal, analysis }) => + scanDependencyChanges( + analysis.dependencyChanges(DEPENDENCY_LIMITS), + fetch, + { + signal, + limits: DEPENDENCY_LIMITS, + cache: analysis, + }, + ), render: (deps, { safeCodeSpan, promptText }) => { const lines: string[] = []; if (!deps.length) return lines; diff --git a/review-enrichment/src/analyzers/secret-scan.ts b/review-enrichment/src/analyzers/secret-scan.ts index bcb504ae9e..816f36641d 100644 --- a/review-enrichment/src/analyzers/secret-scan.ts +++ b/review-enrichment/src/analyzers/secret-scan.ts @@ -2,6 +2,7 @@ // assignments, citing file:line and the KIND only — the matched secret VALUE is never returned (so the brief is // safe to splice into a public review). Higher-recall than the engine's in-process regex pass, and line-cited via // the hunk headers so the reviewer can point at the exact line. +import type { AddedLine } from "../analysis-context.js"; import type { EnrichRequest, SecretFinding } from "../types.js"; interface Rule { @@ -78,6 +79,26 @@ export function scanPatch(path: string, patch: string): SecretFinding[] { return findings; } +export function scanAddedLinesForSecrets( + addedLines: readonly AddedLine[], +): SecretFinding[] { + const findings: SecretFinding[] = []; + for (const line of addedLines) { + for (const rule of RULES) { + if (rule.re.test(line.text)) { + findings.push({ + file: line.file, + line: line.line, + kind: rule.kind, + confidence: rule.confidence, + }); + break; + } + } + } + return findings; +} + /** Analyzer entrypoint: scan every changed file's patch for leaked credentials. */ export async function scanSecrets( req: EnrichRequest, diff --git a/review-enrichment/src/analyzers/secret/descriptor.ts b/review-enrichment/src/analyzers/secret/descriptor.ts index 0820becbf6..aa80bfde78 100644 --- a/review-enrichment/src/analyzers/secret/descriptor.ts +++ b/review-enrichment/src/analyzers/secret/descriptor.ts @@ -1,5 +1,5 @@ import type { AnalyzerDescriptor } from "../types.js"; -import { scanSecrets } from "../secret-scan.js"; +import { scanAddedLinesForSecrets } from "../secret-scan.js"; export const secretAnalyzer: AnalyzerDescriptor<"secret"> = { name: "secret", @@ -17,7 +17,8 @@ export const secretAnalyzer: AnalyzerDescriptor<"secret"> = { notes: "High-confidence patterns are treated as rotate-and-remove candidates; generic assignments stay verify-first.", }, - run: (req) => scanSecrets(req), + run: (_req, { analysis }) => + Promise.resolve(scanAddedLinesForSecrets(analysis.addedLines)), render: (secrets, { safeCodeSpan }) => { const lines: string[] = []; if (!secrets.length) return lines; diff --git a/review-enrichment/src/analyzers/types.ts b/review-enrichment/src/analyzers/types.ts index 0b7eda6855..9b6fec5ce9 100644 --- a/review-enrichment/src/analyzers/types.ts +++ b/review-enrichment/src/analyzers/types.ts @@ -3,6 +3,7 @@ import type { BriefFindings, EnrichRequest, } from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; import type { AnalyzerRenderHelpers } from "../render-helpers.js"; export type AnalyzerName = keyof BriefFindings; @@ -39,6 +40,7 @@ export interface AnalyzerRunContext { startedAtMs: number; deadlineMs: number; diagnostics: AnalyzerDiagnostics; + analysis: AnalysisContext; } export type AnalyzerResult = diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 5fff68c040..301ac71877 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -12,6 +12,10 @@ import type { AnalyzerRegistry, AnalyzerRunContext, } from "./analyzers/types.js"; +import { + createAnalysisContext, + type AnalysisContext, +} from "./analysis-context.js"; import { ANALYZERS } from "./analyzers/registry.js"; import { renderBrief } from "./render.js"; import { captureAnalyzerDegradation } from "./sentry.js"; @@ -34,6 +38,7 @@ function runWithTimeout( run: (context: AnalyzerRunContext) => Promise, ms: number, diagnostics: AnalyzerDiagnostics, + analysis: AnalysisContext, ): Promise { const controller = new AbortController(); const startedAtMs = Date.now(); @@ -43,6 +48,7 @@ function runWithTimeout( startedAtMs, deadlineMs: startedAtMs + ms, diagnostics, + analysis, }; return new Promise((resolve, reject) => { const timer = setTimeout(() => { @@ -107,11 +113,30 @@ function captureDegradation( skippedFileCount: input.diagnostics.skippedFileCount, githubEndpointCategory: input.diagnostics.githubEndpointCategory, capped: input.diagnostics.capped, + cacheHits: input.diagnostics.cacheHits, + cacheMisses: input.diagnostics.cacheMisses, + externalCallsByCategory: input.diagnostics.externalCallsByCategory, + skippedWorkByCategory: input.diagnostics.skippedWorkByCategory, + cappedWorkByCategory: input.diagnostics.cappedWorkByCategory, + analysisElapsedMs: input.diagnostics.analysisElapsedMs, requestId: input.options.requestId, traceId: input.options.traceId, }); } +function attachAnalysisMetrics( + diagnostics: AnalyzerDiagnostics, + analysis: AnalysisContext, +): void { + const metrics = analysis.snapshotMetrics(); + diagnostics.cacheHits = metrics.cacheHits; + diagnostics.cacheMisses = metrics.cacheMisses; + diagnostics.externalCallsByCategory = metrics.externalCallsByCategory; + diagnostics.skippedWorkByCategory = metrics.skippedWorkByCategory; + diagnostics.cappedWorkByCategory = metrics.cappedWorkByCategory; + diagnostics.analysisElapsedMs = metrics.analysisElapsedMs; +} + export async function buildBrief( req: EnrichRequest, analyzers: AnalyzerRegistry = ANALYZERS, @@ -123,6 +148,10 @@ export async function buildBrief( ? all.filter((name) => req.analyzers!.includes(name)) : all; const budgetMs = resolveAnalyzerTimeoutMs(req.budget?.timeoutMs); + const analysis = createAnalysisContext(req, { + startedAtMs: start, + deadlineMs: start + budgetMs, + }); const findings: BriefFindings = {}; const analyzerStatus: Record = {}; @@ -141,6 +170,7 @@ export async function buildBrief( (context) => analyzer(req, context), budgetMs, diagnostics, + analysis, ); findings[name] = result as never; if (resultIsPartial(result)) { @@ -149,6 +179,7 @@ export async function buildBrief( diagnostics.partialStatus = "partial"; diagnostics.partialReason ??= "analyzer_partial"; if (diagnostics.captureDegradation) { + attachAnalysisMetrics(diagnostics, analysis); captureDegradation(new Error(diagnostics.partialReason), { analyzer: name, requested, @@ -168,6 +199,7 @@ export async function buildBrief( partial = true; diagnostics.partialStatus = "partial"; diagnostics.partialReason ??= error instanceof Error ? error.message : "analyzer_error"; + attachAnalysisMetrics(diagnostics, analysis); captureDegradation(error, { analyzer: name, requested, diff --git a/review-enrichment/src/sentry.ts b/review-enrichment/src/sentry.ts index 204d17a65e..dabf2d0a73 100644 --- a/review-enrichment/src/sentry.ts +++ b/review-enrichment/src/sentry.ts @@ -121,6 +121,12 @@ export interface AnalyzerDegradationContext { skippedFileCount?: number; githubEndpointCategory?: string; capped?: boolean; + cacheHits?: number; + cacheMisses?: number; + externalCallsByCategory?: Record; + skippedWorkByCategory?: Record; + cappedWorkByCategory?: Record; + analysisElapsedMs?: number; requestId?: string; traceId?: string; } @@ -148,6 +154,12 @@ export function captureAnalyzerDegradation(error: unknown, context: AnalyzerDegr skippedFileCount: context.skippedFileCount, githubEndpointCategory: context.githubEndpointCategory, capped: context.capped, + cacheHits: context.cacheHits, + cacheMisses: context.cacheMisses, + externalCallsByCategory: context.externalCallsByCategory, + skippedWorkByCategory: context.skippedWorkByCategory, + cappedWorkByCategory: context.cappedWorkByCategory, + analysisElapsedMs: context.analysisElapsedMs, requestId: context.requestId, traceId: context.traceId, release: activeRelease, @@ -174,12 +186,16 @@ export function captureAnalyzerDegradation(error: unknown, context: AnalyzerDegr 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 (partialStatusTag) scope.setTag("partialStatus", partialStatusTag); if (phaseTag) scope.setTag("phase", phaseTag); if (endpointTag) scope.setTag("githubEndpointCategory", endpointTag); if (requestIdTag) scope.setTag("requestId", requestIdTag); if (traceIdTag) scope.setTag("traceId", traceIdTag); + if (cacheHitsTag) scope.setTag("cacheHits", cacheHitsTag); + if (cacheMissesTag) scope.setTag("cacheMisses", cacheMissesTag); scope.setTag("environment", sentryTagValue(activeEnvironment) ?? "production"); Sentry!.captureException(error instanceof Error ? error : new Error(String(error))); }); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 4d8f382a87..f7943ad7f5 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -303,9 +303,24 @@ export interface AnalyzerDiagnostics { prLookupCount?: number; skippedFileCount?: number; capped?: boolean; + cacheHits?: number; + cacheMisses?: number; + externalCallsByCategory?: Record; + skippedWorkByCategory?: Record; + cappedWorkByCategory?: Record; + analysisElapsedMs?: number; captureDegradation?: boolean; } +export interface AnalyzerMetricsDiagnostics { + cacheHits: number; + cacheMisses: number; + externalCallsByCategory: Record; + skippedWorkByCategory: Record; + cappedWorkByCategory: Record; + analysisElapsedMs: number; +} + /** Service → engine response. `promptSection` is spliced verbatim; `findings` is the structured backing data. */ export interface ReviewBrief { schemaVersion: 1; diff --git a/review-enrichment/test/analysis-context.test.ts b/review-enrichment/test/analysis-context.test.ts new file mode 100644 index 0000000000..568ab4f051 --- /dev/null +++ b/review-enrichment/test/analysis-context.test.ts @@ -0,0 +1,188 @@ +// Units for the shared REES analysis context (#1810). Kept separate so future analyzer PRs can add their own +// migrations without fighting over the broad enrichment test file. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { createAnalysisContext } from "../dist/analysis-context.js"; +import { scanDependencyChanges } from "../dist/analyzers/dependency-scan.js"; + +const jsonResponse = (body, init = {}) => + new Response(JSON.stringify(body), { + status: 200, + headers: { "content-type": "application/json" }, + ...init, + }); + +test("createAnalysisContext parses common PR state once", () => { + let now = 130; + const syntheticGithubToken = ["ghp", "abcdefghijklmnopqrstuvwxyz1234567890"].join("_"); + const context = createAnalysisContext( + { + repoFullName: "JSONbored/gittensory", + prNumber: 1810, + headSha: "abcdef1234567890", + files: [ + { + path: "src/config.ts", + patch: [ + "@@ -2,2 +2,3 @@", + " const safe = true;", + "-const oldToken = null;", + `+const token = "${syntheticGithubToken}";`, + ].join("\n"), + }, + { + path: "package.json", + patch: [ + "@@ -5,2 +5,2 @@", + '- "lodash": "^4.17.20",', + '+ "lodash": "^4.17.21",', + ].join("\n"), + }, + ], + }, + { startedAtMs: 100, deadlineMs: 250, now: () => now }, + ); + + assert.deepEqual(context.repo, { + owner: "JSONbored", + repo: "gittensory", + fullName: "JSONbored/gittensory", + prNumber: 1810, + headSha: "abcdef1234567890", + }); + assert.deepEqual(context.changedFilePaths, ["src/config.ts", "package.json"]); + assert.deepEqual(context.dependencyManifestPaths, ["package.json"]); + assert.deepEqual(context.patchHunks.map((hunk) => [hunk.file, hunk.newStart]), [ + ["src/config.ts", 2], + ["package.json", 5], + ]); + assert.deepEqual( + context.addedLines.map((line) => [line.file, line.line, line.text]), + [ + ["src/config.ts", 3, `const token = "${syntheticGithubToken}";`], + ["package.json", 5, ' "lodash": "^4.17.21",'], + ], + ); + + const limits = { + maxManifestFiles: 20, + maxPatchLinesPerFile: 500, + maxDependencyQueries: 25, + }; + const firstChanges = context.dependencyChanges(limits); + const secondChanges = context.dependencyChanges(limits); + assert.strictEqual(secondChanges, firstChanges); + assert.deepEqual(firstChanges, [ + { + ecosystem: "npm", + package: "lodash", + from: "4.17.20", + to: "4.17.21", + }, + ]); + + now = 175; + assert.equal(context.remainingMs(250), 75); + assert.deepEqual(context.snapshotMetrics(), { + cacheHits: 1, + cacheMisses: 1, + externalCallsByCategory: {}, + skippedWorkByCategory: {}, + cappedWorkByCategory: {}, + analysisElapsedMs: 75, + }); +}); + +test("request cache de-dupes in-flight external lookups and records safe metrics", async () => { + const context = createAnalysisContext({ + repoFullName: "JSONbored/gittensory", + prNumber: 1810, + }); + let loads = 0; + const load = async () => { + loads += 1; + await new Promise((resolve) => setTimeout(resolve, 5)); + return { ok: true }; + }; + + const [first, second] = await Promise.all([ + context.cachedExternalCall("github commit/pulls", "commit:abc123", load), + context.cachedExternalCall("github commit/pulls", "commit:abc123", load), + ]); + + assert.equal(loads, 1); + assert.strictEqual(first, second); + assert.deepEqual(context.snapshotMetrics().externalCallsByCategory, { + github_commit_pulls: 1, + }); + assert.equal(context.snapshotMetrics().cacheMisses, 1); + assert.equal(context.snapshotMetrics().cacheHits, 1); +}); + +test("request cache preserves category and key boundaries", async () => { + const context = createAnalysisContext({ + repoFullName: "JSONbored/gittensory", + prNumber: 1810, + }); + let loads = 0; + + const first = await context.cachedExternalCall("a:b", "c", async () => { + loads += 1; + return "category-with-colon"; + }); + const second = await context.cachedExternalCall("a", "b:c", async () => { + loads += 1; + return "key-with-colon"; + }); + const repeatedFirst = await context.cachedExternalCall("a:b", "c", async () => { + throw new Error("cache miss"); + }); + const repeatedSecond = await context.cachedExternalCall("a", "b:c", async () => { + throw new Error("cache miss"); + }); + + assert.equal(first, "category-with-colon"); + assert.equal(second, "key-with-colon"); + assert.equal(repeatedFirst, first); + assert.equal(repeatedSecond, second); + assert.equal(loads, 2); + assert.equal(context.cache.size, 2); + assert.equal(context.snapshotMetrics().cacheMisses, 2); + assert.equal(context.snapshotMetrics().cacheHits, 2); +}); + +test("scanDependencyChanges reuses cached OSV package lookups inside one request", async () => { + const context = createAnalysisContext({ + repoFullName: "JSONbored/gittensory", + prNumber: 1810, + }); + let fetchCalls = 0; + const fetchImpl = async () => { + fetchCalls += 1; + return jsonResponse({ + vulns: [ + { + id: "GHSA-test", + summary: "test advisory", + database_specific: { severity: "HIGH" }, + }, + ], + }); + }; + const duplicateChanges = [ + { ecosystem: "npm", package: "lodash", from: null, to: "4.17.20" }, + { ecosystem: "npm", package: "lodash", from: null, to: "4.17.20" }, + ]; + + const findings = await scanDependencyChanges(duplicateChanges, fetchImpl, { + cache: context, + limits: { maxDependencyQueries: 25 }, + }); + + assert.equal(fetchCalls, 1); + assert.equal(findings.length, 2); + assert.equal(findings[0].cves[0].id, "GHSA-test"); + assert.deepEqual(context.snapshotMetrics().externalCallsByCategory, { osv: 1 }); + assert.equal(context.snapshotMetrics().cacheMisses, 1); + assert.equal(context.snapshotMetrics().cacheHits, 1); +}); diff --git a/review-enrichment/test/sentry-degradation.test.ts b/review-enrichment/test/sentry-degradation.test.ts index 11e2e54d0b..9b35babc36 100644 --- a/review-enrichment/test/sentry-degradation.test.ts +++ b/review-enrichment/test/sentry-degradation.test.ts @@ -140,6 +140,12 @@ test("captureAnalyzerDegradation attaches safe attribution context for history f skippedFileCount: 2, githubEndpointCategory: "commit_pulls", capped: true, + cacheHits: 4, + cacheMisses: 9, + externalCallsByCategory: { osv: 3, commit_pulls: 12 }, + skippedWorkByCategory: { history_budget: 2 }, + cappedWorkByCategory: { history_files: 2 }, + analysisElapsedMs: 6812, requestId: "req-123", traceId: "0123456789abcdef0123456789abcdef", diff: `+${fakeToken}`, @@ -155,6 +161,8 @@ test("captureAnalyzerDegradation attaches safe attribution context for history f assert.equal(sentry.tags.partialStatus, "partial"); assert.equal(sentry.tags.phase, "similar_past_prs"); assert.equal(sentry.tags.githubEndpointCategory, "commit_pulls"); + assert.equal(sentry.tags.cacheHits, "4"); + assert.equal(sentry.tags.cacheMisses, "9"); assert.equal(sentry.tags.requestId, "req-123"); const analyzerContext = sentry.contexts.rees_analyzer as Record; assert.deepEqual(analyzerContext, { @@ -177,6 +185,12 @@ test("captureAnalyzerDegradation attaches safe attribution context for history f skippedFileCount: 2, githubEndpointCategory: "commit_pulls", capped: true, + cacheHits: 4, + cacheMisses: 9, + externalCallsByCategory: { osv: 3, commit_pulls: 12 }, + skippedWorkByCategory: { history_budget: 2 }, + cappedWorkByCategory: { history_files: 2 }, + analysisElapsedMs: 6812, requestId: "req-123", traceId: "0123456789abcdef0123456789abcdef", release: "gittensory-rees@test",