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
8 changes: 6 additions & 2 deletions src/queue/ai-review-orchestration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@ export async function runAiReviewForAdvisory(
public_notes: hasPublicReviewAssessment(result.advisoryNotes),
/* v8 ignore next -- current review runner always supplies diagnostics for completed AI attempts. */
review_diagnostics: result.reviewDiagnostics ?? [],
});
}, "ai_review_inconclusive");
}
args.advisory.findings.push(...findings);
const metadataFor = (
Expand Down Expand Up @@ -828,6 +828,7 @@ export async function runAiReviewForAdvisory(
null,
combine: env.AI_REVIEW_PLAN?.combine ?? null,
},
"ai_review_public_summary_missing",
);
return {
notes:
Expand All @@ -848,13 +849,16 @@ export async function runAiReviewForAdvisory(
error: errorMessage(error),
}),
);
// error is a genuinely caught exception here (unlike the two captures above, which construct their own
// Error to report a known condition) -- named to mirror the structured log's own "event" field just above,
// not the exception's native class, so every unexpected review crash groups under one readable title.
captureReviewFailure(error, {
kind: "review",
installationId: args.installationId,
repo: args.repoFullName,
pr: args.pr.number,
head_sha: args.advisory.headSha,
});
}, "ai_review_failed");
return undefined;
} finally {
// #regate-dup-prep: only release a lock THIS call actually claimed. A caller-supplied
Expand Down
4 changes: 2 additions & 2 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7941,7 +7941,7 @@ async function maybePublishPrPublicSurface(
pr: pr.number,
head_sha: advisory.headSha,
failedOutputs: failedOutputs.map((failure) => failure.output),
});
}, "pr_public_surface_publish_failed");
// At least one output failed for a reason that can plausibly clear on its own (rate limit / 5xx / momentary
// token issue) — retry the whole job instead of leaving the review permanently unposted. A mix of transient
// and permanent failures still retries: the permanent one re-fails identically next pass and re-audits, but
Expand Down Expand Up @@ -9039,7 +9039,7 @@ async function maybePublishPrPublicSurface(
head_sha: advisory.headSha,
reviewer_count: aiReview?.reviewerCount ?? 0,
public_notes: hasPublicReviewAssessment(aiReview?.notes),
});
}, "ai_review_public_summary_missing");
}

// Secrets-scan (#audit-3.4): always scans the REAL resolved diff and, on a CONCRETE credential hit, appends a
Expand Down
12 changes: 6 additions & 6 deletions src/selfhost/pg-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ export function createPgQueue(
error: errorMessageWithCause(error),
}),
);
captureError(error, { kind: "queue_dead_letter_revive_crashed" });
captureError(error, { kind: "queue_dead_letter_revive_crashed" }, "queue_dead_letter_revive_crashed");
}
}

Expand Down Expand Up @@ -758,7 +758,7 @@ export function createPgQueue(
error: errorMessageWithCause(error),
}),
);
captureError(error, { kind: "queue_foreground_liveness_release_crashed" });
captureError(error, { kind: "queue_foreground_liveness_release_crashed" }, "queue_foreground_liveness_release_crashed");
}
}

Expand Down Expand Up @@ -1088,7 +1088,7 @@ export function createPgQueue(
reason: "processing_timeout",
recovered,
timeoutMs: processingTimeoutMs,
});
}, "processing_timeout");
}
const job = await claimNext();
if (!job) return false;
Expand Down Expand Up @@ -1116,7 +1116,7 @@ export function createPgQueue(
kind: "job_dead",
reason: "unparseable_payload",
jobId: job.id,
});
}, "unparseable_payload");
return true;
}
const jobTraceParent = message.type === "github-webhook" ? message.traceParent : undefined;
Expand Down Expand Up @@ -1427,7 +1427,7 @@ export function createPgQueue(
jobType: extractPayloadType(job.payload),
jobId: job.id,
attempts,
});
}, "job_dead");
} else {
const retryDelayMs = consumingRetryDelayMs(error, backoff(attempts));
await pool.query(
Expand Down Expand Up @@ -1474,7 +1474,7 @@ export function createPgQueue(
error: errorMessageWithCause(error),
}),
);
captureError(error, { kind: "queue_pump_crashed" });
captureError(error, { kind: "queue_pump_crashed" }, "queue_pump_crashed");
} finally {
active--;
}
Expand Down
28 changes: 20 additions & 8 deletions src/selfhost/sentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -431,26 +431,40 @@ export async function buildSentryOpenTelemetryBridge(): Promise<OpenTelemetryBri
};
}

/** Capture an error with optional structured context. No-op when Sentry is off. */
/** Name a synthetic Error before capture so its Sentry issue title reads "eventName: message" instead of the
* generic "Error: message" (or a caught exception's own class name, e.g. "HttpError: ..."). Mirrors
* forwardStructuredLogToSentry's `errorEvent.name = event` below — same discipline, applied to the handful of
* call sites that construct their OWN `new Error(...)` purely to report a known condition, not to a genuinely
* caught exception (which keeps its real name/type — mislabeling those would hide what actually threw). Only
* renames when the caller opts in; every other captureError/captureReviewFailure call is unaffected. */
function namedCaptureError(error: unknown, eventName?: string): Error {
const err = error instanceof Error ? error : new Error(String(error));
if (eventName) err.name = eventName;
return err;
}

/** Capture an error with optional structured context. No-op when Sentry is off. `eventName`, when given, becomes
* the Sentry issue title's prefix (see {@link namedCaptureError}) instead of the generic "Error". */
export function captureError(
error: unknown,
context?: Record<string, unknown>,
eventName?: string,
): void {
if (!active || !Sentry) return;
Sentry.withScope((scope) => {
setOtelTraceScope(scope);
if (context) { const safeContext = hashedInstallationContext(context); scope.setContext("gittensory", safeContext); applyOperationalTags(scope, safeContext); }
Sentry!.captureException(
error instanceof Error ? error : new Error(String(error)),
);
Sentry!.captureException(namedCaptureError(error, eventName));
});
}

/** Capture a failed review at ERROR level, tagged by repo/PR/SHA for triage. A review that cannot be produced is a
* real failure the maintainer must SEE — not a warning that hides in the noise. No-op when off. */
* real failure the maintainer must SEE — not a warning that hides in the noise. No-op when off. `eventName`, when
* given, becomes the Sentry issue title's prefix (see {@link namedCaptureError}) instead of the generic "Error". */
export function captureReviewFailure(
error: unknown,
context?: Record<string, unknown>,
eventName?: string,
): void {
if (!active || !Sentry) return;
Sentry.withScope((scope) => {
Expand All @@ -461,9 +475,7 @@ export function captureReviewFailure(
scope.setContext("review", safeContext);
applyOperationalTags(scope, safeContext);
}
Sentry!.captureException(
error instanceof Error ? error : new Error(String(error)),
);
Sentry!.captureException(namedCaptureError(error, eventName));
});
}

Expand Down
12 changes: 6 additions & 6 deletions src/selfhost/sqlite-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ export function createSqliteQueue(
error: errorMessageWithCause(error),
}),
);
captureError(error, { kind: "queue_dead_letter_revive_crashed" });
captureError(error, { kind: "queue_dead_letter_revive_crashed" }, "queue_dead_letter_revive_crashed");
}
}

Expand Down Expand Up @@ -441,7 +441,7 @@ export function createSqliteQueue(
error: errorMessageWithCause(error),
}),
);
captureError(error, { kind: "queue_foreground_liveness_release_crashed" });
captureError(error, { kind: "queue_foreground_liveness_release_crashed" }, "queue_foreground_liveness_release_crashed");
}
}

Expand Down Expand Up @@ -826,7 +826,7 @@ export function createSqliteQueue(
reason: "processing_timeout",
recovered,
timeoutMs: processingTimeoutMs,
});
}, "processing_timeout");
}
const job = claimNext();
if (!job) return false;
Expand Down Expand Up @@ -854,7 +854,7 @@ export function createSqliteQueue(
kind: "job_dead",
reason: "unparseable_payload",
jobId: job.id,
});
}, "unparseable_payload");
return true;
}
const jobTraceParent = message.type === "github-webhook" ? message.traceParent : undefined;
Expand Down Expand Up @@ -1113,7 +1113,7 @@ export function createSqliteQueue(
jobType: extractPayloadType(job.payload),
jobId: job.id,
attempts,
});
}, "job_dead");
} else {
const retryDelayMs = consumingRetryDelayMs(error, backoff(attempts));
driver.query(
Expand Down Expand Up @@ -1163,7 +1163,7 @@ export function createSqliteQueue(
error: errorMessageWithCause(error),
}),
);
captureError(error, { kind: "queue_pump_crashed" });
captureError(error, { kind: "queue_pump_crashed" }, "queue_pump_crashed");
} finally {
active--;
}
Expand Down
14 changes: 7 additions & 7 deletions src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,12 +317,12 @@ async function main(): Promise<void> {
}),
);
process.on("uncaughtException", (error) => {
captureError(error, { kind: "uncaughtException" });
captureError(error, { kind: "uncaughtException" }, "uncaughtException");
console.error(error);
void flushSentry().finally(() => process.exit(1));
});
process.on("unhandledRejection", (reason) => {
captureError(reason, { kind: "unhandledRejection" });
captureError(reason, { kind: "unhandledRejection" }, "unhandledRejection");
console.error(reason);
});
// Central error forwarding (#1468): operational failures are structured JSON logs emitted through stdout and
Expand Down Expand Up @@ -1069,7 +1069,7 @@ async function main(): Promise<void> {
state: orbRelayRegistrationState,
register: registerOrbRelayTargetWithRetry,
...(relayDrainState ? { drainState: relayDrainState } : {}),
}).catch((error) => captureError(error, { kind: "orb_relay_register" }));
}).catch((error) => captureError(error, { kind: "orb_relay_register" }, "orb_relay_register"));
void attemptOrbRelayRegistration();
setInterval(() => void attemptOrbRelayRegistration(), 60_000);
// Dashboard-visible counterparts to the streak/no-progress alert gate in isOrbRelayRegistrationAlerting:
Expand All @@ -1093,7 +1093,7 @@ async function main(): Promise<void> {
};
if (isD1SizeProbeEnabled(d1ProbeEnv)) {
/* v8 ignore start -- self-host entrypoint timer; probe logic itself is unit-tested in d1-size-probe.test.ts. */
const runD1Probe = () => runD1SizeProbe(d1ProbeEnv).catch((error) => captureError(error, { kind: "d1_size_probe" }));
const runD1Probe = () => runD1SizeProbe(d1ProbeEnv).catch((error) => captureError(error, { kind: "d1_size_probe" }, "d1_size_probe"));
void runD1Probe();
setInterval(runD1Probe, 900_000);
/* v8 ignore stop */
Expand Down Expand Up @@ -1127,14 +1127,14 @@ async function main(): Promise<void> {
}
};
void drainRelay().catch((error) =>
captureError(error, { kind: "orb_relay_drain" }),
captureError(error, { kind: "orb_relay_drain" }, "orb_relay_drain"),
);
// 30s matches broker-client's request timeout so a slow/degraded broker's in-flight drain has fully
// timed out (or completed) before the next tick would otherwise pile another request on top of it.
setInterval(
() =>
void drainRelay().catch((error) =>
captureError(error, { kind: "orb_relay_drain" }),
captureError(error, { kind: "orb_relay_drain" }, "orb_relay_drain"),
),
30_000,
);
Expand All @@ -1160,7 +1160,7 @@ async function main(): Promise<void> {
}

main().catch((error) => {
captureError(error, { kind: "boot" });
captureError(error, { kind: "boot" }, "boot");
console.error(error);
/* v8 ignore next -- boot failure exits the process; shutdown helper is covered independently. */
void Promise.all([shutdownOpenTelemetry(), flushSentry()]).finally(() => process.exit(1));
Expand Down
9 changes: 6 additions & 3 deletions src/services/agent-action-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
// terminal-hold capture below and the "a real failure the maintainer must see" convention already used
// for review-pass failures (selfhost/sentry.ts's captureReviewFailure, queue/processors.ts). Previously
// this class of failure was audit-log-only, invisible without a manual audit_events query.
captureError(error, { kind: "agent_action_execution_failed", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, actionClass: action.actionClass });
captureError(error, { kind: "agent_action_execution_failed", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, actionClass: action.actionClass }, "agent_action_execution_failed");
}
// #2265: a permission-looking 403 on a PR-write mutation can mean the LOCAL installations.permissions
// snapshot is stale after a maintainer-initiated downgrade (GitHub sends no downgrade webhook). Rate-limit
Expand Down Expand Up @@ -817,7 +817,7 @@ export async function executeIssueMaintenanceActions(env: Env, ctx: IssueActionE
await audit("error", errorMessage(error));
// Mirrors executeAgentMaintenanceActions's non-merge capture below -- issue-side label/close has no retry
// loop either, so a single failure here is already this pass's terminal outcome.
captureError(error, { kind: "agent_issue_action_execution_failed", repo: ctx.repoFullName, issue: ctx.issueNumber, installationId: ctx.installationId, actionClass: action.actionClass });
captureError(error, { kind: "agent_issue_action_execution_failed", repo: ctx.repoFullName, issue: ctx.issueNumber, installationId: ctx.installationId, actionClass: action.actionClass }, "agent_issue_action_execution_failed");
}
}

Expand Down Expand Up @@ -850,7 +850,10 @@ async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, er
// failure the maintainer must see" case captureReviewFailure already covers for an exhausted AI review pass.
// Fires once per hold (not per retry attempt), so a transient failure that resolves within MERGE_RETRY_CAP
// never reaches Sentry at all.
captureError(error, { kind: "agent_merge_blocked", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, reason: reason.slice(0, 280) });
// Named "agent_merge_blocked" (not the caught exception's own class, e.g. "HttpError") so every terminal
// merge hold groups under one readable title regardless of which HTTP status caused it -- the specific
// status/reason stays in the message and the "review" context object either way.
captureError(error, { kind: "agent_merge_blocked", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, reason: reason.slice(0, 280) }, "agent_merge_blocked");
await recordAuditEvent(env, {
eventType: "agent.action.merge_blocked",
actor: AGENT_ACTOR,
Expand Down
6 changes: 3 additions & 3 deletions test/unit/agent-action-executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1405,7 +1405,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
expect((await auditFor(env, "merge"))?.outcome).toBe("error");
// "not mergeable" is immediately terminal (classifyMergeFailure), so this held-for-human outcome must be
// Sentry-visible, not just an audit_events row a maintainer has to go looking for (#3862/#3863 gap sweep).
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_merge_blocked", repo: "owner/repo", pr: 7 }));
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_merge_blocked", repo: "owner/repo", pr: 7 }), "agent_merge_blocked");
captureSpy.mockRestore();
});

Expand Down Expand Up @@ -1444,7 +1444,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
expect(refreshInstallationHealthForInstallation).toHaveBeenCalledWith(env, 123);
// Non-merge action classes have no retry loop, so a single failure is already this pass's terminal outcome
// and must be Sentry-visible immediately (#3862/#3863 gap sweep).
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_action_execution_failed", actionClass: "close" }));
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_action_execution_failed", actionClass: "close" }), "agent_action_execution_failed");
captureSpy.mockRestore();
});

Expand Down Expand Up @@ -1875,7 +1875,7 @@ describe("executeIssueMaintenanceActions (#2270 issue-side actuation)", () => {
const outcomes = await executeIssueMaintenanceActions(env, issueCtx(), [issueClose]);
expect(outcomes[0]?.outcome).toBe("error");
expect((await auditFor(env, "close"))?.outcome).toBe("error");
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_issue_action_execution_failed", actionClass: "close" }));
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_issue_action_execution_failed", actionClass: "close" }), "agent_issue_action_execution_failed");
captureSpy.mockRestore();
});

Expand Down
4 changes: 3 additions & 1 deletion test/unit/ai-review-advisory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ describe("runAiReviewForAdvisory", () => {
expect.objectContaining({ status: "unparseable_output" }),
]),
}),
"ai_review_inconclusive",
);
captureSpy.mockRestore();
});
Expand All @@ -413,7 +414,7 @@ describe("runAiReviewForAdvisory", () => {
confirmedContributor: true,
});
expect(adv.findings.map((f) => f.code)).toEqual(["ai_review_inconclusive"]);
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ reason: "ai_review_inconclusive", repo: "acme/widgets", head_sha: "sha3" }));
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ reason: "ai_review_inconclusive", repo: "acme/widgets", head_sha: "sha3" }), "ai_review_inconclusive");
captureSpy.mockRestore();
});

Expand Down Expand Up @@ -684,6 +685,7 @@ describe("runAiReviewForAdvisory", () => {
head_sha: "sha3",
reviewer_count: 0,
}),
"ai_review_public_summary_missing",
);
captureSpy.mockRestore();
});
Expand Down
1 change: 1 addition & 0 deletions test/unit/queue-2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1489,6 +1489,7 @@ describe("queue processors", () => {
reviewer_count: 0,
public_notes: false,
}),
"ai_review_public_summary_missing",
);
captureSpy.mockRestore();
});
Expand Down
2 changes: 1 addition & 1 deletion test/unit/queue-4.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5597,7 +5597,7 @@ describe("queue processors", () => {
expect(aggregate?.metadata_json).toContain('"transient":true');
// The total publish failure (nothing reached the PR) escalates to Sentry at error level, not just the ledger —
// this still fires BEFORE the retryable throw, so the failure stays observable even though the job also retries.
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "publish", repo: "JSONbored/gittensory" }));
expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "publish", repo: "JSONbored/gittensory" }), "pr_public_surface_publish_failed");
captureSpy.mockRestore();
});

Expand Down
Loading
Loading