diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 5239b2f4cf..bad200be26 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -7,6 +7,7 @@ import type { BriefFindings, AnalyzerStatus, AnalyzerDiagnostics, + AnalyzerTelemetry, } from "./types.js"; import type { AnalyzerRegistry, @@ -31,6 +32,7 @@ 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; @@ -135,6 +137,11 @@ function timeoutStatus(error: unknown, diagnostics: AnalyzerDiagnostics): Analyz 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: { @@ -144,6 +151,9 @@ function captureDegradation( timeoutMs: number; elapsedMs: number; analyzerStatus: AnalyzerStatus; + profile: string; + costClass?: string; + responseReserveMs?: number; diagnostics: AnalyzerDiagnostics; options: BuildBriefOptions; }, @@ -157,6 +167,9 @@ 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, @@ -213,9 +226,18 @@ export async function buildBrief( const findings: BriefFindings = {}; const analyzerStatus: Record = {}; + const analyzerTelemetry: Record = {}; let partial = false; - for (const item of plan.skipped) analyzerStatus[item.name] = "skipped"; + 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; @@ -226,6 +248,14 @@ export async function buildBrief( 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; @@ -238,6 +268,15 @@ export async function buildBrief( ); 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, + }; partial = true; analysis.metrics.recordCappedWork(`analyzer_${item.descriptor.cost}`, 1); return; @@ -259,10 +298,23 @@ export async function buildBrief( 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", + ); 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 ??= status === "capped" ? "analyzer_capped" : "analyzer_partial"; + diagnostics.partialReason = partialReason; if (diagnostics.captureDegradation) { attachAnalysisMetrics(diagnostics, analysis); captureDegradation(new Error(diagnostics.partialReason), { @@ -272,27 +324,50 @@ export async function buildBrief( 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, + 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 ??= error instanceof Error ? error.message : "analyzer_error"; + diagnostics.partialReason = partialReason; attachAnalysisMetrics(diagnostics, analysis); - captureDegradation(error, { + 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, }); @@ -312,21 +387,49 @@ export async function buildBrief( ); for (const name of all) - if (!plan.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/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/sentry.ts b/review-enrichment/src/sentry.ts index 0722d0cec1..c178dbbf69 100644 --- a/review-enrichment/src/sentry.ts +++ b/review-enrichment/src/sentry.ts @@ -111,6 +111,9 @@ export interface AnalyzerDegradationContext { timeoutMs?: number; elapsedMs?: number; analyzerStatus?: string; + profile?: string; + costClass?: string; + responseReserveMs?: number; partialStatus?: string; partialReason?: string; phase?: string; @@ -147,6 +150,9 @@ 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, @@ -187,6 +193,9 @@ 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); @@ -197,6 +206,9 @@ export function captureAnalyzerDegradation(error: unknown, context: AnalyzerDegr 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); 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 671882d042..d3e4bd61e7 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -350,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/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index e1345a89f1..837fd0bd2e 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -1674,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",`, 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 index 845f95f284..ebd7bc90fe 100644 --- a/review-enrichment/test/scheduler.test.ts +++ b/review-enrichment/test/scheduler.test.ts @@ -81,6 +81,12 @@ test("slow analyzers time out inside the reserved response budget", async () => 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); }); @@ -105,4 +111,7 @@ test("registry analyzers skip when their relevant inputs are absent", 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 be9d0ab2d9..bfeba47d6c 100644 --- a/review-enrichment/test/sentry-degradation.test.ts +++ b/review-enrichment/test/sentry-degradation.test.ts @@ -130,6 +130,9 @@ 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", @@ -161,6 +164,9 @@ 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"); @@ -180,6 +186,9 @@ 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", @@ -212,6 +221,7 @@ 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( { @@ -224,7 +234,7 @@ test("buildBrief stays fail-open and captures a degraded analyzer", async () => }, { dependency: async () => { - throw new Error("osv unavailable"); + throw new Error(`osv unavailable for ${fakeToken}`); }, }, ); @@ -234,8 +244,11 @@ 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"); @@ -245,6 +258,32 @@ test("buildBrief stays fail-open and captures a degraded analyzer", async () => 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 timed-out partial response before the caller timeout budget is spent", async () => { const started = Date.now(); const brief = await buildBrief(