feat(mt#4881): Alert on reviewer failures that never reach submission

[minsky-ai]

The reviewer's operator alerting is wired to the wrong tracker. mt#2363 (the cockpit Ask) and mt#2364 (the external sink) both hang off one emit point — sweeper.circuit_breaker_tripped, gated on the reviewer_submission_failures circuit breaker from mt#2350. That tracker is written from exactly one place: the catch around submitReview in guarded-submit.ts:123 (verified — it is the only non-test call site). A review that dies before it can submit creates no tracker row, so the circuit never opens, so no ask and no external alert is ever produced.

Measured against prod 2026-09-01, re-verified 2026-09-02: 88 failed_at_reviewer rows in 30 days against 16 tracker rows EVER, the newest from 2026-08-08. mt#1596 escalated this exact family in 2026-06 and its two children shipped DONE — the family sentence is still true, because the path that shipped reads a different table than the one that fills up.

What the planning pass corrected before implementation

Four things the spec asserted did not survive checking, and each changed the fix:

1. 9 of the 88 are not pre-submit — they are the mt#3852 submit-path 422, which does write a tracker row (their dates line up one-for-one with the tracker's own, and 2026-08-08's are alerted = true). Uncovered population is 79, and a naive "alert on every failed_at_reviewer" would double-alert on the one class already covered. That became SC8.
2. There are two failed_at_reviewer writers, not one — 51 rows carry stage: "reviewer" (server.ts), 33 carry stage: "boot_recovery" (boot-recovery.ts). Four write sites in total, since each file has both a .then and a .catch. A seam at review_error/webhook_processing_failed would have covered server.ts only and left ~38% of the class dark.
3. "No PR-visible signal" was false — server.ts's catch writes buildErrorBody to the PR status comment on every thrown failure. What is actually wrong is sharper: that comment is a single update-in-place comment, so the next review overwrites it. On edobry/peezombie.me PR build(deps-dev): bump @typescript-eslint/eslint-plugin from 7.18.0 to 8.32.0 #2 it was created at 17:21:37 (the minute of the first failure) and now reads "APPROVED", updated 22:59:03.
4. The durable in-PR signal is missing. publishCheckRun has exactly two call sites, both in review-finalize.ts, and the error one is reached only via finalizeReviewError — the empty-output and CoT-leakage cases. A throw is caught at review-worker.ts:942, which records unrecovered timing and rethrows, so it never gets there. The two dominant classes set no check-run conclusion at all. ADR-030 assigns "status, convergence, and liveness of record" to the check-run channel and names failure/liveness surfacing as its open follow-up; the spec cited it nowhere. That became SC7.

Key changes

• services/reviewer/src/failure-alert.ts (new) — the seam. All four failed_at_reviewer write sites now call recordReviewFailure instead of updateOutcome, so the outcome write and the alert cannot drift apart. It classifies the error (10 classes, every one derived from the measured population), suppresses duplicates, carries the aggregation facts, and skips what the circuit breaker already owns. Fail-open throughout — alerting must never affect a review.
• ask-emitter.ts — emitReviewFailureAlert + ReviewFailureAlertContext, alongside the existing circuit-breaker method. Dedup for this path lives in the caller, which is what mt#1596 asked for ("these emit points need their own dedup design").
• server.ts / boot-recovery.ts — the four call sites, plus the emitter built from the same domain container the sweeper uses. A skip resolved after dispatch keeps its existing failed_at_reviewer write and is deliberately not alerted (concurrent_inflight is in the measured 88 for exactly this reason).
• check-run-publisher.ts — ConvergenceState.roundNumber widened to number | null. On the thrown path the round is genuinely unknown (nothing has ingested the prior reviews), so null renders without the round parenthetical rather than fabricating "round 1". Output for a numeric round is byte-identical to before, asserted by test.
• scripts/replay-failure-alerting.ts (new) — the §7a verification artifact; see Live verification.

The judgment call the replay forced

The dedup key started as (owner, repo, pr_number, head_sha, error_class) — what the spec proposed. Replaying the real corpus showed that collapsing 88 failures to 60 asks, which is barely a reduction: a repo-wide outage fails each PR exactly once, so a per-PR key dedups nothing. 30 of those 60 were already flagged systemic.

So the key is now adaptive. The failure that tips a condition over the distinct-PR threshold still alerts, carrying the repo-wide signal; every later PR hit by the same condition inside the window is suppressed. Same corpus: 37 asks (~1.2/day). This is a deviation from the spec's proposed key, recorded here and in the code, and it is the reason the artifact exists rather than a nice-to-have.

Thresholds are grounded in observed cadence, not round numbers. The 60-minute suppression window comes from the originating burst (4 failures on one PR spanning 17:21-18:15, a 54-minute burst); the systemic threshold of 3 distinct PRs comes from the measured day-buckets splitting cleanly into single-PR days (1 PR) and repo-wide days (6, 11, 13 PRs) with nothing in between.

Testing

Execution evidence:

AT1 / AT3 / AT4 / AT5 / AT6, SC1-SC4, SC8 — the seam (38 tests, new file):

$ cd services/reviewer && bun test src/failure-alert.test.ts
 38 pass
 0 fail
 85 expect() calls
Ran 38 tests across 1 file. [154.00ms]

• AT1 — AT1: a pre-submit failure creates an ask naming the repo, PR, and error class
• AT3 — AT3: a burst on one (PR, class) produces exactly one ask (20 attempts -> 1 ask)
• AT4 — AT4: 5 distinct PRs with one class read as a systemic condition, not 5 one-offs (5 failures -> 3 asks, the third flagged systemic, the first two not overclaiming)
• AT5 / SC8 — AT5/SC8: a failure already owned by the circuit breaker does NOT double-alert
• AT6 — AT6: a boot_recovery-stage failure alerts on the same seam, carrying its stage
• AT7 / SC7 — a thrown failure yields conclusion=failure with no fabricated round, plus a numeric round renders byte-identically to the pre-change output as the regression guard on the existing path
• AT2 — the negative control below

SC5 (no backfill) — nothing to run: forward-only by construction, no migration, no backfill script. The 88 rows are historical and their PRs are resolved.

SC6 — [sc6-deferred: mt#4881] — the live before/after needs the deployed service; discharged post-merge in §10, see Live verification.

Negative control — AT2: the tests can fail, and did.

Restoring the full pre-mt#4881 behavior (write the outcome row, alert nobody) rather than reverting a single line, per the mt#4512 discipline:

$ MT4881_NEGATIVE_CONTROL=1 bun test src/failure-alert.test.ts
(fail) recordReviewFailure > AT1: a pre-submit failure creates an ask naming the repo, PR, and error class
(fail) recordReviewFailure > AT3: a burst on one (PR, class) produces exactly one ask
(fail) recordReviewFailure > a DIFFERENT error class on the same PR is not suppressed by the first
(fail) recordReviewFailure > AT4: the same class across 5 distinct PRs reports a systemic condition
(fail) recordReviewFailure > AT5/SC8: a failure already owned by the circuit breaker does NOT double-alert
(fail) recordReviewFailure > AT6: a boot_recovery-stage failure alerts on the same seam, carrying its stage
(fail) recordReviewFailure > SC3: an empty message still alerts, classified as the empty class
(fail) recordReviewFailure > fail-open: an emitter that throws does not propagate
(fail) recordReviewFailure > fail-open: a DB that throws on the aggregation query does not propagate
 25 pass
 9 fail

The 25 that still pass are the pure-function tests (classifier, coordinate extraction, aggregation, check-run payload), which correctly do not depend on the emit. The scaffold was removed before commit.

What this control does NOT buy: it ran in the real runtime against the real code, and it reverted the whole fix rather than one line — but it establishes nothing about coverage of the failure class. That axis is covered by the replay below, which is why the replay exists.

Full reviewer suite — no regression:

$ cd services/reviewer && bun test
 2426 pass
 0 fail
 5312 expect() calls
Ran 2426 tests across 92 files. [4.57s]

Repo-wide lint at the CI gate, and format:

$ bun run lint:strict     # eslint . --max-warnings=0
LINT_EXIT=0
$ bun run format:check
FORMAT_EXIT=0

Typecheck — clean across all 8 projects (., packages/domain, packages/shared, services/reviewer, services/site, src/cockpit/web, tsconfig.hooks.json, tsconfig.scripts.json), validated against the session workspace. infra/tsconfig.json skipped with its documented reason.

On the gated runner, stated plainly so it is not over-read: bun scripts/run-tests-gated.ts reports Ran 0 tests across 0 files and all test steps passed for this diff. That is correct and expected, not coverage — run-tests-main.ts's ROOTS does not include services/, so the gated runner structurally cannot execute a reviewer test. CI runs services/reviewer in its own job (.github/workflows/ci.yml:194), which is what the 2426-test run above corresponds to. Do not read the gated pass as evidence about this change.

Live verification

services/reviewer/scripts/replay-failure-alerting.ts is the verification artifact. It is read-only — no row written, no Ask created, no GitHub call — and replays the real historical failed_at_reviewer population through the exact shipped classifier and aggregatePriorFailures, rather than re-deriving the rule locally (a replay that re-implements the rule measures the copy, and the copy is what drifts).

Run here over an 88-row production export of the same 30-day window the spec measured, via the --fixture path (this session has no Postgres URL in env — probed: MINSKY_PERSISTENCE_POSTGRES_URL, MINSKY_SESSIONDB_POSTGRES_URL, MINSKY_POSTGRES_URL, DATABASE_URL all absent; the sanctioned loader is a secret-emitting script and is guard-blocked from direct invocation, so the rows were exported through the Supabase MCP instead):

{
  "source": "fixture:...mt4881-fixture.json",
  "windowDays": 30,
  "failuresReplayed": 88,
  "rowsWithoutUsableCoordinates": 0,
  "classification": {
    "distribution": {
      "provider_timeout": 19,
      "provider_unavailable": 18,
      "tls_self_signed": 16,
      "provider_credits_exhausted": 12,
      "github_submit_rejected": 9,
      "github_diff_too_large": 4,
      "provider_token_limit": 4,
      "unclassified": 2,
      "network_socket_closed": 2,
      "unclassified_empty": 2
    },
    "unclassified": 4,
    "coveragePct": 95.5
  },
  "alerting": {
    "suppressionWindowMinutes": 60,
    "systemicDistinctPrThreshold": 3,
    "wouldAlert": 37,
    "wouldSuppress": 51,
    "systemicAlerts": 5,
    "asksPerFailure": 0.42,
    "upperBoundCaveat": "excludes the circuit-breaker suppression; production volume is lower by the github_submit_rejected count"
  }
}

Two things this establishes that no unit test could: the classifier names 95.5% of the real corpus with 0 rows whose coordinates could not be extracted, and the dedup rule turns 30 days of failures into 37 asks, not 88. wouldAlert is an upper bound — the replay cannot see the circuit-breaker check, which removes the 9 github_submit_rejected in production.

UNVERIFIED — the end-to-end live exercise (SC6) is deferred to §10 post-deploy, because the ask is created by the deployed service against the live asks substrate and no failing review has been induced against it yet. Deploy-SUCCESS will not settle this: the alert path is fail-open by design, so a wired-but-broken emitter and a healthy one are indistinguishable from the deploy signal. Post-merge I will induce a pre-submit failure and confirm an operator ask appears, and report that result rather than the deploy.

Deploy verification: this PR changes deploy surface — isDeploySurfaceFile returns true for all 8 changed files (run over the actual diff, not recalled). Post-merge I will wait on the deployment bound to this merge (notBefore = merge time, expectCommitSha = merge SHA), read buildIdentity, and assert the health body's service identity rather than the status code.

Coordination

mt#2719 ("Reviewer auth-health: surface sustained GitHub auth failure as a cockpit operator Ask") was absent from the spec's duplicate check and substantially overlaps this: its 2026-07-31 extension scans the same review_error stream for sustained provider failure, covering ~30 of these 79 rows. Neither subsumes the other, so they coordinate — this task owns per-failure emission and the aggregation facts; mt#2719 owns the global health trackers (auth-health.ts is not per-PR and writes no failed_at_reviewer row) and the severity/paging escalation built on those facts, which this task deliberately does not set. A dependency edge and a coordination note were recorded on both specs. Both tasks add a method to AskEmitter; landing this first avoids two concurrent edits to that interface.

mt#4118 was being planned concurrently by another agent during this work. Zero file overlap (its scope is src/adapters/shared/commands/ and the merge-coordination skill), and its ## Out of scope names mt#4881 explicitly — reconciled in both directions. Two of its findings are adopted here: that error_details.message is best-effort (2 of 143 rows are empty, the mt#2465 class — the classifier degrades to unclassified_empty rather than assuming a message), and its independent reading of ADR-030, which corroborates SC7's placement.

Open PR #3412 (mt#4639) touches ask-emitter.ts, server.ts, sweeper.ts, auth-health.ts as a mechanical getLoggableErrorSummary conversion. It is mergeable_state: dirty and 6 days old. Textual conflict risk only — it changes logging-call arguments, not control flow — and this change's primary seam files (failure-alert.ts, webhook-events.ts, review-finalize.ts, check-run-publisher.ts, boot-recovery.ts) are untouched by it.

Full planning audit, corrected measurements, per-criterion gate verdicts and ref-drift dispositions: mt#4881.

🤖 Generated with Claude Code

https://claude.ai/code/session_0146ufGg5jbbdFTUP37MCeFE

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 610K prompt, 7K completion | Duration: 152s
Mode: normal

Commands

• /review — request a fresh review

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Solid, well-tested addition of a failure-alerting seam and durable check-run surfacing. The adaptive dedup logic and systemic classification are thoughtfully implemented with comprehensive tests, and the thrown-path check run is plumbed fail-open with a null-round rendering fix. However, two issues need attention before merge: (1) documentation/runbook updates are missing for these new operator-visible behaviors (pre-submit failure Asks, suppression semantics, and thrown-path check-run), which is a blocking gap; and (2) the replay verification script’s DB-url presence check ignores DATABASE_URL despite citing it, causing unnecessary skips — non-blocking but easy to fix. Address the docs gap and consider the replay script tweak; otherwise the code changes look coherent and scoped.

Findings

• [BLOCKING] services/reviewer/src/server.ts:587 — Docs/runbook updates missing for new operator-facing behaviors (pre-submit failure Asks and thrown-path check-run failures)
  This PR changes user-visible failure surfacing in two ways: (1) new operator Asks for pre-submission failures via emitReviewFailureAlert (e.g., ask-emitter.ts:232-316, invoked from server.ts:552-569 and boot-recovery.ts:334-363), and (2) a failure-conclusion check run for thrown errors with an omitted round (server.ts:587-606, check-run-publisher.ts:61-77,163-179). I could not find corresponding updates under docs/ or runbooks describing where and how failures now surface, the suppression window and systemic threshold semantics, or the new check-run shape. Per review policy, behavior changes that affect operators/users require documentation updates; otherwise existing docs become incomplete/misleading. Please add or update the relevant operator runbooks/ADR cross-references to reflect these changes (alerting seam, dedup rules, and check-run behavior).
• [NON-BLOCKING] services/reviewer/scripts/replay-failure-alerting.ts:64 — Replay script skips DB-backed mode unless a MINSKY_* URL is set — DATABASE_URL is ignored despite being cited and commonly used
  loadRows checks only MINSKY_PERSISTENCE_POSTGRES_URL / MINSKY_SESSIONDB_POSTGRES_URL / MINSKY_POSTGRES_URL to decide whether to connect, and returns null (skip) when none are present (services/reviewer/scripts/replay-failure-alerting.ts:64-79). Many environments surface the Postgres URL as DATABASE_URL, which your PR description also mentioned as probed, but this script’s precheck does not consider it — causing an unnecessary skip even though createDb() might succeed against DATABASE_URL. Suggest: include process.env.DATABASE_URL in the presence check (or drop the precheck entirely and rely on createDb() failure to decide), to align with the description and avoid false skips.

Spec verification

Criterion	Status	Evidence
SC1 — A review that fails before submission produces an operator-reachable signal through the same substrate mt#2363 built (...). Target population is the 79 pre-submit failures, not all 88.	Met	New seam recordReviewFailure is invoked in server.ts:552-569 (non-thrown error path) and server.ts:573-606 (thrown error path) and in boot-recovery.ts:334-363. It routes to AskEmitter.emitReviewFailureAlert implemented in ask-emitter.ts:232-316, which creates coordination.notify asks. Outcome rows are still written via updateOutcome inside recordReviewFailure (see failure-alert.ts:451-462).
SC2 — Dedup is designed, not inherited. Propose key on (owner, repo, pr_number, head_sha, error_class) with a suppression window.	Met	Custom dedup implemented via aggregatePriorFailures (failure-alert.ts:258-347) plus suppression window SUPPRESSION_WINDOW_MS = 60*60*1000 (failure-alert.ts:54-71). Adaptive systemic suppression via alreadySystemic logic (failure-alert.ts:317-347) and applied in recordReviewFailure suppression branch (failure-alert.ts:497-515). Tests cover AT3/AT4 (see failure-alert.test.ts:214-307).
SC3 — Carry error class; cover both stage values (reviewer and boot_recovery).	Met	Classifier classifyReviewFailure with explicit classes and empty-message handling (failure-alert.ts:93-184). Stage plumbed from both paths: server writes with stage: "reviewer" (e.g., server.ts:556-569 and server.ts:587-606), boot-recovery calls with stage: "boot_recovery" (boot-recovery.ts:334-363, 363-378). Tests assert stage propagation (failure-alert.test.ts:356-369).
SC4 — Distinguish repeated vs systemic by stated criterion; include occurrence count, distinct-PR count, and window in payload.	Met	Aggregation returns priorOccurrencesOnPr and distinctPrsWithClass with systemic threshold (failure-alert.ts:317-347). These are sent in emitReviewFailureAlert question+metadata (ask-emitter.ts:270-316). Tests verify systemic tipping and suppression (failure-alert.test.ts:270-307, 309-337).
SC5 — No backfill; forward-only.	Met	No migration/backfill added. Replay script is explicitly read-only and forward-only (services/reviewer/scripts/replay-failure-alerting.ts header, plus it returns 0 on no-DB and never writes). No code attempts to process historical rows beyond aggregation for suppression.
SC6 — Live before/after: after shipping, force a failure → ask appears within a sweep interval.	Unverifiable	Live, deployed behavior cannot be verified from the diff. The PR body itself marks it deferred post-merge; no in-repo artifact can prove it. No logs or run output included. Unverifiable from code alone.
SC7 — Thrown-failure path publishes a failure-conclusion check run (no fabricated round).	Met	createApp adds publishFailureCheckRunSafe and calls it in the thrown path (server.ts:587-606 and helper at server.ts:225-259). buildCheckRunPayload now supports roundNumber: null to omit the round parenthetical (check-run-publisher.ts:61-77, 163-179). Tests assert summary/title formatting for null (failure-alert.test.ts:497-557).
SC8 — No double-alert and no regression: submit-path 422 continues to alert via the circuit breaker and does NOT also alert from the new seam.	Met	isOwnedByCircuitBreaker checks the submission tracker and suppresses this path (failure-alert.ts:349-382 and applied at failure-alert.ts:471-485). Test AT5/SC8 asserts suppression when a tracker row exists (failure-alert.test.ts:339-355).

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
AskEmitter.emitReviewFailureAlert	function	services/reviewer/src/failure-alert.ts:466-516 — calls through injected AskEmitter to emit the operator Ask, services/reviewer/src/server.ts:212-259, 552-606 — constructs DomainAskEmitter; passes as dependency to recordReviewFailure, services/reviewer/src/boot-recovery.ts:220-241, 334-363 — accepts AskEmitter and passes to recordReviewFailure, services/reviewer/src/sweeper.test.ts:1208-1214, 1244-1250, 1393-1399 — test stubs to satisfy updated interface	Adopted	New AskEmitter method added for pre-submission failure alerts. Implemented by DomainAskEmitter (services/reviewer/src/ask-emitter.ts:232-316). Callers are wired via recordReviewFailure; the sweeper does not use this path (only circuit-breaker).

Documentation impact

• blocking-needs-update — This PR introduces user-visible behavior changes: (1) pre-submission reviewer failures now generate operator-routed Asks via emitReviewFailureAlert (services/reviewer/src/ask-emitter.ts:232-316) and (2) thrown failure paths now publish a failure-conclusion GitHub check run with an omitted round (“Reviewer failure: …”) (services/reviewer/src/server.ts:587-606; services/reviewer/src/check-run-publisher.ts:61-77,163-179). I found no accompanying updates under docs/ (none in the diff). Operator runbooks and the reviewer failure-surfacing docs should be updated to reflect the new alerting seam, suppression semantics (window and systemic thresholds), and the durable check-run behavior for thrown failures. Absent such updates, existing docs risk being incomplete or misleading about where failures surface.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

R2 verification: the prior BLOCKING concern on server’s pre-persist handling was addressed — missing GitHub headers now synthesize a unique delivery id and signature/event name are gated before any DB write, eliminating the silent row-collapsing/skip path. Operator alerting for pre-submission failures is implemented via a single recordReviewFailure seam used from both server.ts and boot-recovery.ts, with adaptive dedup and suppression. The thrown-path check-run emission is added and fail-open. The replay script is read-only and correctly consumes the same shipped rules, and small DB-client exports prevent env-var drift. README now documents both alert paths. I found no new critical defects introduced by the fix; tests and safety guards look sound. Approve to merge.

Spec verification

Criterion	Status	Evidence
SC1 — A review that fails before submission produces an operator-reachable signal through the same substrate (coordination.notify Ask via direct domain imports). Target population is the 79 pre-submit failures, not all 88.	Met	Implemented via recordReviewFailure seam and AskEmitter.emitReviewFailureAlert calls from both thrown and non-thrown paths. See services/reviewer/src/server.ts:330-406 (catch path calls recordReviewFailure) and services/reviewer/src/boot-recovery.ts:193-236, 246-263 (both resolve/catch paths). AskEmitter wired from domain container (server.ts:84-96).
SC2 — Dedup is designed (not inherited). A burst must not produce one Ask per failure; propose keying w/ suppression window.	Met	Suppression window and adaptive systemic dedup implemented. See services/reviewer/src/failure-alert.ts:33-68 (SUPPRESSION_WINDOW_MS, SYSTEMIC_DISTINCT_PR_THRESHOLD) and aggregation/suppression logic at :282-321 (uses aggregatePriorFailures to suppress prior-on-PR or already-systemic). Replay script demonstrates volume: services/reviewer/scripts/replay-failure-alerting.ts:147-213.
SC3 — Error class is carried; both stages covered (reviewer and boot_recovery).	Met	Classifier classifyReviewFailure with enumerated classes (services/reviewer/src/failure-alert.ts:76-143) and stage preserved from callers. Both server.ts (stage: "reviewer", :296-308, :360-371) and boot-recovery.ts (stage: "boot_recovery", :206-236, :246-263) call into the same seam.
SC4 — Repeated/systemic condition is distinguishable by a stated criterion; payload carries occurrence count, distinct-PR count, and window.	Met	aggregatePriorFailures returns priorOccurrencesOnPr, distinctPrsWithClass, systemic, alreadySystemic (services/reviewer/src/failure-alert.ts:210-251). These are passed to the Ask emitter (services/reviewer/src/failure-alert.ts:312-323).
SC5 — No backfill is attempted; forward-only.	Met	No migration/backfill code present; seam runs only on new failures. README explicitly documents forward-only replay artifact (services/reviewer/README.md:358-418).
SC6 — Measured before/after via a live, deliberately-failed review producing an Ask within one sweep interval (post-ship live verification).	Unverifiable	Live production exercise cannot be verified from the diff. PR body states defer-to-post-deploy. No repo artifact can prove a live Ask was created; requires deployed service and induced failure.
SC7 — Thrown-failure path publishes a failing check run (ADR-030 channel 2).	Met	New publishFailureCheckRunSafe emits a minsky-reviewer/findings check run on the catch path. See services/reviewer/src/server.ts:98-141 (definition) and :372-383 (invocation after thrown failure). Convergence state roundNumber widened to allow null per earlier commit (not in this slice).
SC8 — No double-alert; submit-path 422 remains owned by circuit breaker and is skipped by the new seam.	Met	isOwnedByCircuitBreaker queries reviewer_submission_failures for the same (owner, repo, prNumber, headSha) and returns early with suppressed_circuit_breaker (services/reviewer/src/failure-alert.ts:253-281). Callers respect this before emitting.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
services/reviewer/src/db/client.ts — CONNECTION_STRING_ENV_VARS, findConnectionStringEnvVar	function	services/reviewer/scripts/replay-failure-alerting.ts:22 — imports CONNECTION_STRING_ENV_VARS, findConnectionStringEnvVar	Adopted	New helper exports are internal to the reviewer service; adopted immediately by the replay script to avoid env-var drift.

Documentation impact

• updated-in-pr — Operator-facing behavior changed and is now documented: services/reviewer/README.md expands 'Operator alerts' into two paths, adding pre-submission Ask surfacing and thrown-path check-run visibility, with guidance on reading titles and dedup semantics. This addresses the prior iteration’s gap where only the circuit-breaker path was documented.
  Affected: services/reviewer/README.md

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Verification pass: the prior BLOCKING concern around server pre-persist handling is addressed; the new seam recordReviewFailure is correctly called from both server and boot-recovery paths, pairing the outcome write with operator alerting and adaptive dedup. The thrown-path check-run is added with roundNumber: null rendering and does not regress existing payloads. The replay script is read-only, consumes the shipped classifier/aggregation, and gates DB env vars via the service-exported list to avoid drift. Tests cover classifier, aggregation, suppression, and SC7 rendering. I found no new critical defects introduced by these changes. Documentation for the new alert path is included in README. Approve to merge.

Spec verification

Criterion	Status	Evidence
SC1 — A review that fails before submission produces an operator-reachable signal through the same substrate mt#2363 built (a coordination.notify ask via direct domain imports). Target population is the 79 pre-submit failures, not the 9 submit-path 422.	Met	Implemented via the new seam recordReviewFailure that routes to AskEmitter.emitReviewFailureAlert using a DomainAskEmitter wired from the booted domain container. Call sites in both server.ts and boot-recovery.ts now invoke recordReviewFailure for pre-submit failures. Evidence: services/reviewer/src/failure-alert.ts:430-538 (seam + emit), services/reviewer/src/server.ts:559-575 and :573-606 (thrown/non-thrown paths), services/reviewer/src/boot-recovery.ts:334-370 (then/catch paths). README documents Path B (pre-submission) accordingly.
SC2 — Dedup is designed, not inherited. Suppress bursts by keying on coordinates and error class with a window.	Met	Adaptive dedup implemented: per-PR/class suppression plus systemic suppression once alreadySystemic is true; window set by SUPPRESSION_WINDOW_MS = 60*60*1000. Aggregation implemented in aggregatePriorFailures, and used in recordReviewFailure to decide suppression. Evidence: services/reviewer/src/failure-alert.ts:76-94 (window), :248-334 (aggregatePriorFailures), :356-428 (fetchPriorFailures), :489-538 (suppression logic). Tests: services/reviewer/src/failure-alert.test.ts AT3/AT4.
SC3 — Carry the error class into the signal; cover both stage: reviewer and stage: boot_recovery.	Met	Classifier classifyReviewFailure maps real-world messages to 10 classes; empty message handled as unclassified_empty. Both server and boot-recovery pass stage through and use the same seam. Evidence: services/reviewer/src/failure-alert.ts:105-210 (classifier), :447-488 (Ask payload includes errorClass, stage). Callers pass stage appropriately: services/reviewer/src/server.ts:565-575, :589-604; services/reviewer/src/boot-recovery.ts:334-370. Tests: services/reviewer/src/failure-alert.test.ts — classification suite and AT6.
SC4 — Distinguish REPEATED vs SYSTEMIC by payload facts: occurrence count, distinct-PR count, and window.	Met	aggregatePriorFailures returns priorOccurrencesOnPr, distinctPrsWithClass, systemic, alreadySystemic, and window minutes are derived from SUPPRESSION_WINDOW_MS. These are included in the Ask metadata and used to shape the title. Evidence: services/reviewer/src/failure-alert.ts:248-334 (aggregation), :447-488 (Ask payload), services/reviewer/src/ask-emitter.ts:245-289 (title/body construction). Tests: services/reviewer/src/failure-alert.test.ts AT4 and related assertions.
SC5 — No backfill; forward-only.	Met	No migration/backfill code added; seam attaches only to live write points. README explicitly states forward-only and replay script is read-only. Evidence: services/reviewer/README.md Path B notes; absence of any migration/change scripts; replay script docs emphasize READ-ONLY (services/reviewer/scripts/replay-failure-alerting.ts:1-24).
SC6 — A measured before/after: post-ship, induce a failure and verify an Ask within one sweep interval (live).	Unverifiable	Live deployment verification occurs post-merge by design; the diff contains no runtime output. The PR provides a read-only replay tool instead (services/reviewer/scripts/replay-failure-alerting.ts), but end-to-end live Ask creation cannot be verified from repo content alone.
SC7 — Thrown-failure path publishes a failing check run (ADR-030 channel 2).	Met	server.ts adds publishFailureCheckRunSafe and invokes it on the thrown path with roundNumber: null to avoid fabricating a round; check-run-publisher.ts updated to accept roundNumber: number | null and render accordingly. Evidence: services/reviewer/src/server.ts:212-251 (publisher), :606-612 (invocation), services/reviewer/src/check-run-publisher.ts:61-76, :163-181. Tests: services/reviewer/src/failure-alert.test.ts — SC7 block.
SC8 — No double-alert with the existing circuit-breaker path; 422-on-submit remains single-alerted via circuit-breaker.	Met	recordReviewFailure checks circuit-breaker ownership via isOwnedByCircuitBreaker (query against submission_failures) and returns suppressed_circuit_breaker without emitting an Ask when owned. Evidence: services/reviewer/src/failure-alert.ts:336-355 (circuit check), :489-503 (suppression branch). Test: services/reviewer/src/failure-alert.test.ts — AT5/SC8.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
AskEmitter.emitReviewFailureAlert	function	services/reviewer/src/failure-alert.ts:472 — calls deps.askEmitter.emitReviewFailureAlert(...), services/reviewer/src/sweeper.test.ts:1211,1247,1396 — test stubs updated to satisfy interface	Adopted	New AskEmitter method for pre-submission failure alerts (mt#4881). DomainAskEmitter implements it; production consumer is the new seam in failure-alert.ts.
db/client.CONNECTION_STRING_ENV_VARS	function	services/reviewer/scripts/replay-failure-alerting.ts:33,52 — lists accepted env vars for DB URL gating and skip messaging	Adopted	Exported to prevent env-var list drift between service and script. Used by the replay script; no external wiring required.

Documentation impact

• updated-in-pr — Operator alerting behavior and the new pre-submission failure path are documented in services/reviewer/README.md under a new “Path B — pre-submission failures (mt#4881)” section. It differentiates Path A (circuit-breaker submission failures) from Path B, explains classification, aggregation fields, dedup policy, and the in-PR check-run signal. No other docs appear to require updates.
  Affected: services/reviewer/README.md

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

R3 verification: prior BLOCKING concerns were addressed — the pre-persist handling and the new failure-alert seam are correctly wired from both server and boot-recovery paths, deduping adaptively and skipping circuit-breaker-owned cases. The thrown-path check-run emission is added with roundNumber: null handling and guarded to fail open. The replay script is read-only, gates DB env vars via the exported list, and consumes the shipped classifier/aggregation. Tests comprehensively cover classification, aggregation, suppression, and SC7 rendering. I found no new critical defects introduced by the fixes. One minor pre-existing grammar nit was noted in check-run summary text. Docs were updated to reflect the two alert paths and replay guidance. Approve to merge.

Findings

• [PRE-EXISTING] services/reviewer/src/check-run-publisher.ts:173 — Minor grammar issue in summary string for singular blocking finding
  The summary string uses remain for both singular and plural: Round ${roundNumber}: ${blockingCount} blocking finding${blockingCount === 1 ? "" : "s"} remain. When blockingCount === 1, the correct verb is remains. This appears to predate this PR — only the round rendering changed here — but noting it for a future polish pass.

Spec verification

Criterion	Status	Evidence
SC1 — A review that fails before submission produces an operator-reachable signal via the asks substrate (on-cockpit), targeting the 79 pre-submit failures (not submit-path 422).	Met	Implemented in services/reviewer/src/failure-alert.ts as recordReviewFailure and wired from both server.ts (thrown and non-thrown failure paths) and boot-recovery.ts (then/catch paths). The Ask is emitted via AskEmitter.emitReviewFailureAlert with classification and PR coords. See server.ts:548-566, 571-585, boot-recovery.ts:336-363, and ask-emitter.ts:232-316.
SC2 — Dedup is designed (not inherited). A burst must not produce one Ask per failure; propose key with suppression window.	Met	Adaptive suppression implemented: prior-on-PR and already-systemic checks in aggregatePriorFailures and caller logic suppress duplicates within SUPPRESSION_WINDOW_MS. See failure-alert.ts:313-406, 482-538. Replay script verifies volume: scripts/replay-failure-alerting.ts uses shipped aggregatePriorFailures and constants.
SC3 — Carry the error class into the signal; cover both stage values (reviewer and boot_recovery).	Met	Classifier classifyReviewFailure produces errorClass including empty-message handling. recordReviewFailure includes stage from both call sites; boot-recovery.ts passes stage: "boot_recovery" (e.g., :346-360), server.ts passes stage: "reviewer" (:571-585). Ask payload includes errorClass and stage (ask-emitter.ts:270-316).
SC4 — Repeated/systemic condition is distinguishable by stated criterion; carry occurrence count, distinct-PR count, window.	Met	aggregatePriorFailures computes priorOccurrencesOnPr, distinctPrsWithClass, and systemic flags. emitReviewFailureAlert puts these into metadata and title/body. See failure-alert.ts:353-406, ask-emitter.ts:255-316.
SC5 — No backfill; forward-only.	Met	No migration or backfill code present. New path only triggers on future failures. Replay script is read-only and explicitly logs skip when no DB URL. See scripts/replay-failure-alerting.ts header and behavior (:1-43, :89-120).
SC6 — Live measured before/after: after shipping, induce a failure and verify an Ask within one sweep interval.	Unverifiable	This criterion depends on post-deploy live behavior outside the diff. The PR includes a replay artifact and describes deferral; no in-repo artifact can prove live emission. Unverifiable from repository content alone.
SC7 — Thrown-failure path publishes a failure-conclusion check run (ADR-030 channel 2) with proper round handling.	Met	server.ts adds publishFailureCheckRunSafe and invokes it in the thrown .catch path (:584-607, :571-585). check-run-publisher.ts widens ConvergenceState.roundNumber to number | null and renders titles/summaries without a fabricated round when null (:61-86, :163-179). Tests assert both null and numeric-round outputs (failure-alert.test.ts:516-618).
SC8 — No double-alert: submit-path 422 still alerts via circuit breaker and is NOT also alerted by the new seam.	Met	recordReviewFailure probes submissionFailuresTable for an owning row before emitting; returns suppressed_circuit_breaker and still writes the outcome row. See failure-alert.ts:413-449 and test AT5/SC8 in failure-alert.test.ts:414-448.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
AskEmitter.emitReviewFailureAlert	function	services/reviewer/src/server.ts:571 — called in thrown and non-thrown failure paths via recordReviewFailure, services/reviewer/src/boot-recovery.ts:341 — passed into recordReviewFailure for boot_recovery paths, services/reviewer/src/sweeper.test.ts:1208 — test stub satisfies interface	Adopted	New AskEmitter method added alongside emitCircuitBreakerAlert for pre-submission failure alerts; wired in server and boot-recovery.

Documentation impact

• updated-in-pr — services/reviewer/README.md was updated in this PR to document the two alert paths (submission circuit breaker vs. pre-submission failures), the dedup/systemic behavior, and the replay script usage, aligning docs with the implemented behavior.
  Affected: services/reviewer/README.md