Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/review/enrichment-wire.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,11 @@ export async function buildReviewEnrichment(
? ENRICHMENT_SYSTEM_SUFFIX
: "",
};
} catch {
} catch (error) {
// Surface the failure (#5 review observability): the REES enrichment call can fail (timeout / network / parse)
// and the review then silently proceeds without the brief. ERROR level so the central Sentry forwarder captures
// a broken/slow REES backend instead of it degrading invisibly.
console.error(JSON.stringify({ level: "error", event: "review_context_fetch_failed", repository: input.repoFullName, contextType: "enrichment", message: String(error).slice(0, 200) }));
return undefined; // timeout / network / parse ⇒ fail-safe; review proceeds without the brief
}
}
4 changes: 3 additions & 1 deletion src/review/inline-comments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ export async function postInlineReviewComments(
await createPullRequestReviewComments(env, args.installationId, args.repoFullName, args.pullNumber, args.commitId, comments, args.mode);
return { posted: comments.length };
} catch (error) {
console.warn(JSON.stringify({ level: "warn", event: "inline_comments_post_failed", repository: args.repoFullName, pullNumber: args.pullNumber, count: comments.length, error: errorMessage(error) }));
// ERROR level (#5 review observability) so the central Sentry forwarder captures a failing inline-comment post
// (auth/permission/422) — it degrades silently (gate unaffected) and was otherwise invisible at warn.
console.error(JSON.stringify({ level: "error", event: "inline_comments_post_failed", repository: args.repoFullName, pullNumber: args.pullNumber, count: comments.length, error: errorMessage(error) }));
return { posted: 0 };
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/review/rag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,10 @@ export async function retrieveContext(
);
return out;
} catch (error) {
console.log(JSON.stringify({ ev: "rag_retrieve_error", message: String(error).slice(0, 200) }));
// ERROR level (#5 review observability): emit so the central Sentry forwarder captures a broken RAG backend
// (qdrant/embedder down) — retrieval degrades the review to diff-only, and this was previously a no-`level`
// console.log invisible to Sentry. Keeps the `ev` tag for log continuity.
console.error(JSON.stringify({ level: "error", event: "review_context_fetch_failed", contextType: "rag", ev: "rag_retrieve_error", message: String(error).slice(0, 200) }));
return "";
}
}
Expand Down
11 changes: 11 additions & 0 deletions test/unit/enrichment-wire.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ describe("buildReviewEnrichment", () => {
).toBeUndefined();
});

it("undefined on a fetch error (network/timeout) and surfaces it at ERROR for Sentry (#5)", async () => {
const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
globalThis.fetch = vi.fn(async () => {
throw new Error("network down");
}) as unknown as typeof fetch;
expect(await buildReviewEnrichment(env({ REES_URL: "https://r" }), input)).toBeUndefined();
// A broken/slow REES backend now surfaces at level:error (central Sentry forwarder) instead of degrading silently.
expect(errSpy.mock.calls.some((c) => String(c[0]).includes("review_context_fetch_failed") && String(c[0]).includes('"contextType":"enrichment"'))).toBe(true);
errSpy.mockRestore();
});

it("undefined on an empty promptSection (no findings)", async () => {
globalThis.fetch = vi.fn(
async () =>
Expand Down
6 changes: 5 additions & 1 deletion test/unit/inline-comments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,17 @@ describe("postInlineReviewComments (#inline-comments, fail-safe)", () => {
expect(calls[0]?.body).toMatchObject({ event: "COMMENT", commit_id: "headsha", comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit:** guard this" }] });
});

it("swallows an API error (the gate is never affected) and reports 0 posted", async () => {
it("swallows an API error (the gate is never affected), reports 0 posted, and surfaces it at ERROR for Sentry (#5)", async () => {
const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes("/access_tokens")) return Response.json({ token: "t" });
return new Response("boom", { status: 500 }); // /reviews → non-2xx → octokit throws → caught
});
expect(await postInlineReviewComments(envWithKey(), { ...base, commitId: "headsha", findings })).toEqual({ posted: 0 });
// The failure is now emitted at level:error so the central Sentry forwarder captures it (was an invisible warn).
expect(errSpy.mock.calls.some((c) => String(c[0]).includes("inline_comments_post_failed") && String(c[0]).includes('"level":"error"'))).toBe(true);
errSpy.mockRestore();
});
});

Expand Down
6 changes: 5 additions & 1 deletion test/unit/rag-wiring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,14 @@ describe("buildReviewRagContext: retrieval wiring + fail-safe", () => {
expect(vec.query).not.toHaveBeenCalled();
});

it("fail-safe: a THROWING vector query degrades to empty context (never throws)", async () => {
it("fail-safe: a THROWING vector query degrades to empty context (never throws) + surfaces it at ERROR for Sentry (#5)", async () => {
const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
const vec = { ...vectorizeStub(), query: vi.fn(async () => { throw new Error("vectorize down"); }) };
const env = createTestEnv({ DB: ragDbStub(), VECTORIZE: vec as unknown as Vectorize, AI: aiStub() as unknown as Ai });
await expect(buildReviewRagContext(env, { repoFullName: "acme/rag-throw-1", files: changedFiles })).resolves.toBe("");
// A broken RAG backend now surfaces at level:error (central Sentry forwarder) instead of degrading invisibly.
expect(errSpy.mock.calls.some((c) => String(c[0]).includes("review_context_fetch_failed") && String(c[0]).includes('"contextType":"rag"'))).toBe(true);
errSpy.mockRestore();
});

it("fail-safe: no changed files → empty context, no adapter use", async () => {
Expand Down
Loading