fix(alertmanager): stop one ownerless alert from failing its whole delivery batch by allyblockcast[bot] · Pull Request #1621 · Blockcast/paperclip

allyblockcast · 2026-09-03T05:00:25Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

The Alertmanager plugin is how monitoring alerts become tracked work: it receives a webhook delivery containing a batch of alerts and turns each one into an issue

A delivery is all-or-nothing. handleWebhook catches per alert, accumulates failed fingerprints, and throws AlertDeliveryIncompleteError if any remain — so Alertmanager sees HTTP 502 and retries the batch

That catch was written against an explicit taxonomy: the failures it expects are issue-RPC, state-store, event and metric errors, which are transient and should be retried. Swallowing them once caused a silent-loss outage (BLO-20467), so the guard is deliberate and correct

That taxonomy stopped being true once handleFiring began throwing on unresolvable fallback ownership. Some unresolvable owners are a config/roster fact that no retry can change — a terminated agent, a wrong name, a genuinely ambiguous one

But not all of them. getAgentWorkEligibility also reports non-invokable for paused, pending_approval and invalid_org_chain, and those are process state: an unpause, a board approval or a restored manager flips them with nobody editing config. Dropping those without a retry would give up the only window that lets an alert land within minutes of the pause lifting (thanks @allyblockcast for catching this — the first revision treated all five as permanent)

This pull request adds PermanentAlertError so a fault that no retry can fix is dropped and acknowledged rather than failing the delivery

The benefit is that Alertmanager stops burning 15–17 retries on a doomed batch for a fault no retry can fix, and stops storming the failure metric that genuinely-transient faults need to stay legible

Linked Issues or Issue Description

Refs PEN-2581 (internal tracker).

Bug. In production, one alert whose fallback owner could not be resolved threw from handleFiring. Because the per-alert catch treats every throw as transient, the fingerprint was recorded and the entire delivery ended in AlertDeliveryIncompleteError → HTTP 502.

The consequence: a permanent fault was reported through the transient-retry channel. Alertmanager retried the delivery 15–17 times and dropped it anyway, because nothing about a retry can make an unresolvable owner resolvable. Every doomed delivery advanced alertmanager_notifications_failed_total{reason="serverError"}, so the resulting storm also masked concurrent, genuinely-transient failures that retrying would have fixed — they were indistinguishable from the permanent one in the only signal an operator had.

~~One ownerless alert dark-tiered its whole batch.~~ Corrected (thanks @allyblockcast) — an earlier revision of this description claimed sibling alerts were lost too. That is false, and it mattered, because it was the stated justification for widening a taxonomy BLO-20467 deliberately narrowed. The per-alert try/catch at webhook-handler.ts:2275 already isolated the batch before this PR: the catch is inside the loop and AlertDeliveryIncompleteError is thrown only after it completes, so sibling alerts were processed and their issues created on the first attempt even when the delivery reported 502. Confirmed by running it: neutralise the carve-out (const permanent = false at the per-alert catch), then comment out both the delivery-outcome and permanent_error assertions — both, because Vitest aborts a test at its first failing assertion — and the issues.create and alert: state assertions still pass. The production incident did dark-tier every alert, but for an incident-specific reason: with fallbackAgentName unset, every unmapped alert in the batch took the same throw, so there were no healthy siblings to survive.

Refusing to create an ownerless issue is correct and is preserved. Spending the entire delivery on that refusal is not.

What Changed

Added PermanentAlertError — a documented class for per-alert faults that no retry can resolve (config/roster facts rather than process state).
handleFiring now raises PermanentAlertError instead of a bare Error when fallback owner resolution fails.
The per-alert catch in handleWebhook gives PermanentAlertError the same log + 200 treatment that malformed payloads and permanent policy drops already receive: it logs, records the failure, drops the alert, and deliberately does not add the fingerprint to failedFingerprints — so the rest of the batch still lands and the delivery is acknowledged.
Permanent drops emit a distinct alertmanager.alert.permanent_error metric, keeping them separable from the transient alertmanager.alert.error.
resolveFallbackAgentId returns FallbackOwnerResolution — an agentId, or a refusal classified permanent/transient from the invokabilityReason eligibility already computes. The mapping is an exhaustive Record, so a new lifecycle reason fails to compile here instead of silently defaulting into a class. unknown_status is transient by design: misclassifying transient-as-permanent drops an alert, permanent-as-transient only costs a retry burst.
The pre-throw alertmanager.owner.fallback_failed metric write is best-effort. Unguarded, a metrics outage surfaced a metrics error instead of PermanentAlertError, the per-alert catch classified it transient and pushed the fingerprint, and the delivery 502'd — reinstating the very retry burst this PR removes.
The refusal class and the per-agent invokabilityReason are both named in the operator-facing warning, so the next occurrence is diagnosable from the log alone.
alertmanager.owner.fallback_failed carries a refusal: permanent|transient label, so an operator can tell "dropped, gone until someone edits config" from "still retrying, may yet land" without joining the series against alertmanager.alert.permanent_error. Two label values, so no meaningful cardinality cost.
The refusal-class predicate tests !== "permanent" rather than === "transient", so a reason missing from the map at runtime lands on the survivable branch instead of dropping the alert. The exhaustive Record makes that unreachable in-repo, but the plugin resolves getAgentWorkEligibility from @paperclipai/shared at runtime, so a built plugin against a newer host could see a reason its own copy of the map never had.
An empty roster takes the transient branch (owner-resolver.ts, immediately after the agents.list call). Raised by @allyblockcast on e3ca4948, and it is a regression this PR would otherwise have introduced: the zero-name-match branch returned permanent unconditionally, so an agents.list that succeeds but returns [] — lagging read replica, company-scoping regression, partial read — was dropped at 200 and never retried, while before this PR that same condition retried. The asymmetry is the point: a list that throws is already handled correctly (the memo evicts, a plain Error propagates, Alertmanager retries), so the quieter variant of the identical host fault must not fare worse. This is also the branch that decides every roster-derived permanent refusal in production — agents.list filters terminated agents out, so a terminated fallback owner never reaches the eligibility ladder and lands here as an unmatched name, which in turn makes the terminated map entry unreachable from this resolver. Deliberately narrow: zero matches against a non-empty roster stays permanent, because that really is a wrong name; a short-but-non-empty list remains indistinguishable from a typo and is not covered.
alertmanager.alert.permanent_error carries severity (and so does the transient alertmanager.alert.error, so the two stay comparable). A permanent drop is acknowledged at 200 and is therefore invisible in Alertmanager's own failure metrics, which makes this series the entire detection surface for it — without the label it cannot answer "did we drop a critical?" without a join.
Comment accuracy, same review: invalid_org_chain is classified transient under the survivable-direction rule rather than because it self-clears — getAgentOrgChainHealth also returns it for terminated_ancestor and cycle, which need a human roster edit. And the terminated map entry is annotated as unreachable from this resolver in production, so its test row is understood as a guard on the map rather than proof the ladder handles it live.
Tests: updated the existing fail-closed test to assert the new delivery outcome while keeping its original guarantees, plus batch-isolation and transient-control tests, and two delivery-outcome tests for the classification (a paused owner keeps the retry window; a permanent drop survives a metrics outage). The batch-isolation test also pins the other half of the BLO-26613 fail-closed guarantee — the ownerless alert leaves no alert: state row — filtered by state key rather than counting state.set calls, because the healthy alert's owner lookup also memoises owner-by-email:… on the instance scope; a raw count would be 2 and would couple a fail-closed assertion to an unrelated cache. That assertion was mutation-checked (inject a stray alert: write and it fails). Both refusal-class values are now pinned: refusal: "transient" on the paused test alongside "permanent" on the drop test, and unknown_status — the entry encoding the transient-default asymmetry — has its own case. Both were mutation-checked (hardcode "permanent" at the write site, or flip the unknown_status mapping, and the respective test fails).

Scope note. All other throw sites reachable from the per-alert path (webhook-handler.ts:396, 559, 1201, 1620, 1885, 1907) were reviewed and are genuinely transient — each is fence/aggregate contention that explicitly asks for a retry. They are intentionally left alone. owner-resolver.ts still contains no throws — it returns a FallbackOwnerResolution, and the single throw site remains webhook-handler.ts, which decides PermanentAlertError vs Error from that returned class. So exactly one site changes classification.

Verification

cd packages/plugins/paperclip-plugin-alertmanager
pnpm --filter @paperclipai/plugin-sdk build   # required, or one suite fails to resolve the SDK
npx vitest run        # 9 files, 290 tests passed
npx tsc --noEmit      # clean

The new tests were confirmed to fail without the source change. Reverting only webhook-handler.ts and re-running leaves the test file in place; the two behavioural tests fail:

× fails closed when the fallback agent configuration is missing
× drops one ownerless alert without aborting the loop or failing the delivery
✓ still fails the delivery for a transient per-alert fault

The third is a control and passes both before and after by design — it pins that the carve-out did not widen into "swallow every per-alert failure".

The batch-isolation test puts the ownerless alert first in the payload, so it also pins that the drop does not abort the remainder of the loop.

Risks

Low-to-moderate, and bounded by two properties that were verified rather than assumed:

This is not a silent-loss regression. The dropped alert is still firing, so Alertmanager re-delivers it on the next repeat_interval. It trades a doomed retry burst for a later re-delivery, not for permanent loss.
No resolve can be stranded. PermanentAlertError is thrown from exactly one site, and owner resolution runs only on the firing path — handleResolved never receives the fallback owner memo. A resolve (which does not repeat, and whose loss would leave an issue open forever) can never take this branch. Confirmed by inspection of the single throw site and the single call site that passes the memo.
The fail-closed guarantee is untouched. An ownerless alert still creates no issue and writes no state row (BLO-26613); the existing test continues to assert both. Only the delivery's reported outcome changed.
The transient guard is untouched. BLO-20467's silent-loss protection still applies to every other failure, with a dedicated control test.

Observability caveat, disclosed rather than hidden: plugin ctx.metrics.write values are not currently exported to Prometheus at all, so the new alertmanager.alert.permanent_error series will not be scrapeable until the plugin metric exposition work lands (PR #1605). This is a pre-existing gap that affects the existing alertmanager.alert.error metric identically — it is not introduced here. The plugin manifest declares only the metrics.write capability, not an allowlist of metric names, so no manifest change is required.

Model Used

Claude Opus 4.5 (claude-opus-4-5), extended thinking, with tool use and code execution (test runs and typecheck executed locally in-repo).

Checklist

Duplicate-PR search. Open PRs touching this plugin were reviewed: #1605 (metric exposition, complementary — see the observability caveat), #1539 / #1277 (severity routing), #1360 (severity/team routing), and #1351 (uninvokable-agent guard). #1351 is the closest neighbour — it is the change that made ownerless refusal fail closed — but it is CONFLICTING, last updated 2026-08-13, and contains none of this route. No overlap.

allyblockcast · 2026-09-03T05:00:30Z

🔗 Paperclip issue: BLO-20467
🔗 Paperclip issue: BLO-26613
🔗 Paperclip issue: PEN-2581

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ae33508

The core change is correct and the reasoning behind it holds up under checking. I verified the two load-bearing safety claims in the description independently, and both are true:

No side effect is stranded by the new drop. The PermanentAlertError throw at webhook-handler.ts:1573 sits on the first-time-creation path, after every earlier branch (re-fire dedupe at :1484, severity floor at :1513) has already returned. Nothing is written to state, no issue is created, no aggregate claim or fence is held at that point.
No resolve can take this branch. handleResolved (:1833) contains no owner resolution and is called without the memo (:2245), so an alert whose loss would be permanent can never be dropped this way.

The memo's transient/permanent split (:1117-1125) is also genuinely careful — a throwing agents.list is evicted and propagates as a transient error, while only a undefined return is cached. That is the right boundary, and it is what makes the rest of the design defensible.

My concerns are both about the edges of the new classification rather than its core.

Critical Issues (0)

Important Issues (2)

[native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:145 — PermanentAlertError is documented as "a configuration or roster fact rather than process state", but two of the five conditions that actually reach it are process state that clears on its own.

Tracing the only throw site back: it fires when resolveFallbackAgentIdMemoized returns undefined, which owner-resolver.ts:218 produces whenever invokable.length !== 1. getAgentWorkEligibility (packages/shared/src/agent-eligibility.ts:212-222) sets invokable: false for terminated, pending_approval, paused, invalid_org_chain, and unknown_status. Only terminated (and the unmatched-name case) is the config/roster fact the doc describes. paused and pending_approval resolve without anyone editing config — a board approval or an unpause flips them, and invalid_org_chain clears when a manager upstream is restored.

This is not hypothetical for this codebase: owner-resolver.ts:220-223 already calls out "your fallback agent is paused" as a distinct operator situation, and the memo doc at :1096 states that scoping the cache per-delivery is what lets "an agent being paused take effect on the very next delivery" — i.e. pause is explicitly modelled as varying between deliveries.

The practical delta is bounded but real: previously a pause lifting inside Alertmanager's 15–17 retry window let the alert land within minutes; now it is dropped at 200 and waits for the next repeat_interval (commonly hours). For a critical alert — which is above the info floor and so takes this path — that is a meaningful time-to-detect regression, and it is a case the description does not consider (it asserts the condition is "not process state").

Recommendation: thread the invokabilityReason out of resolveFallbackAgentId and raise PermanentAlertError only for terminated / no-name-match, leaving paused, pending_approval, and invalid_org_chain on the transient path. The two branches are already distinguished at owner-resolver.ts:219-238, so the reason is in hand and just needs returning. If instead the intent is to treat all of them as permanent, that is a defensible call — but the class doc and the PR description should say so plainly rather than claiming these are not process state.
[gstack/review] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1569 — the alertmanager.owner.fallback_failed metric write immediately before the new throw is unguarded, which defeats the PR's own invariant when telemetry is down.

If that await ctx.metrics.write rejects, handleFiring throws the metrics error rather than PermanentAlertError. The catch at :2267 then evaluates err instanceof PermanentAlertError as false, pushes the fingerprint, and the delivery 502s — reinstating exactly the doomed retry burst this PR removes, and taking the rest of the batch down with it again.

The codebase already establishes the opposite convention for permanent drops, twice, with comments giving this precise reason: the severity-floor drop at :1500-1512 ("Letting a metrics outage throw would mark the delivery failed and make Alertmanager retry an alert we will drop identically every time") and the opt-out drop at :2202-2212 ("a permanent policy drop must stay acknowledged even if telemetry is down"). Now that this site is also a permanent drop, it should follow the same pattern.

Recommendation: wrap :1569-1572 in the same best-effort try/catch used at :1500-1512, logging the metric failure and falling through to the throw. Worth a small test asserting the delivery still resolves when metrics.write rejects on the ownerless path.

Suggestions (2)

[pr-review-toolkit/comments] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:75-77 — the referent in "The healthy alert still became tracked work despite sharing a batch with the ownerless one — and it is ordered FIRST in the payload" is ambiguous. The subject is the healthy alert, but the payload is [ownerless, owned], so "it" means the ownerless one. Since the whole point of that clause is to pin loop continuation, naming it explicitly ("the ownerless alert is ordered first") removes the chance of a future reader reversing the fixture to match the comment.
[pr-review-toolkit/tests] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:78-86 — the batch-isolation test proves the healthy alert's issue was created, but not that the ownerless one left no residue. Asserting mocks.state.set was called exactly once (for the owned fingerprint only) would pin the "no state row for a permanent drop" half of the BLO-26613 guarantee in the multi-alert case too, where it is easiest to regress.

Strengths

The control test (still fails the delivery for a transient per-alert fault) is the right instinct and is what makes the carve-out safe to review — it pins that the exception did not widen into "swallow every per-alert failure", including the negative assertion on the permanent metric.
Ordering the ownerless alert first in the batch fixture is a deliberate choice that catches loop-abort regressions a naive ordering would miss.
Splitting alertmanager.alert.permanent_error from alertmanager.alert.error keeps the two failure classes separable in telemetry, which is what makes the notifications_failed_total masking argument actually verifiable in production.
The description's scope note enumerating the other reachable throw sites, and the confirmation that the new tests fail without the source change, are exactly the evidence that makes this reviewable at a glance.

Recommended Action

No Critical issues — nothing blocks on correctness of the happy path.
Address the two Important issues this cycle: narrow (or honestly re-document) the permanent classification so paused/pending_approval are not treated as unfixable, and guard the fallback_failed metric write so a telemetry outage cannot re-open the retry burst.
Consider the two Suggestions opportunistically.

Note on review identity: this PR is authored by app/allyblockcast, so GitHub bars the App from APPROVE on it. This is submitted as a formal COMMENTED review from the App, which is the artifact of record. reviewDecision is empty on this PR — there is no required-review protection to satisfy, so no additional approving identity is needed.

An alert whose `fallbackAgentName` resolves to no invokable agent threw a bare Error from handleFiring. The per-alert catch was written against a taxonomy where every throw is transient (issue-RPC, state-store, event and metric faults), so it recorded the fingerprint and the delivery ended in AlertDeliveryIncompleteError -> HTTP 502. Two consequences, both observed in production (PEN-2581): 1. A permanent fault travelled the transient-retry channel. Unresolvable ownership is a config/roster fact, not process state, so no retry can fix it: Alertmanager retried 15-17x and dropped the delivery anyway. The resulting notifications_failed_total storm also masked concurrent genuinely-transient failures that retrying would have fixed. 2. One ownerless alert dark-tiered every other alert sharing its batch, because a single accumulated fingerprint fails the whole delivery. Introduce PermanentAlertError for faults no retry can resolve, and give it the same "log + 200" treatment the malformed-payload and permanent-policy drops already get: log, record a distinct `alertmanager.alert.permanent_error` metric, drop the alert, and leave the fingerprint out of failedFingerprints so the rest of the batch still lands. The fail-closed guarantee is unchanged: an ownerless alert still creates no issue and writes no state row (BLO-26613). Only the delivery's reported outcome changes. The throw is reachable solely from the firing path -- handleResolved never runs owner resolution -- so a dropped alert is still firing and returns on Alertmanager's next repeat_interval; no resolve can be stranded. Transient faults are deliberately untouched and still fail the delivery, preserving the silent-loss guard from BLO-20467. Refs PEN-2581 Signed-off-by: Cto <cto@paperclip.blockcast.net>

…lears Addresses both Important findings from Ally's review of #1621. 1. `PermanentAlertError` was raised for every unresolvable fallback owner, but only `terminated` / wrong-name / genuinely-ambiguous is a config or roster fact. `paused`, `pending_approval` and `invalid_org_chain` are process state: an unpause, a board approval, or a restored manager flips them with nobody editing config. Dropping those at 200 gave up Alertmanager's retry window, which is the only thing that lets an alert land within minutes of the pause lifting rather than waiting out a whole `repeat_interval` — a time-to-detect regression for `critical` alerts. `resolveFallbackAgentId` now returns `FallbackOwnerResolution` — either an `agentId` or a `refusal` classified from the `invokabilityReason` that `getAgentWorkEligibility` already computes. The mapping is an exhaustive `Record`, so adding a lifecycle reason fails to compile here instead of silently defaulting into either class. `unknown_status` is transient by design: misclassifying transient-as-permanent drops an alert, while permanent-as-transient only costs a retry burst, so the unrecognised case takes the survivable error. The unreachable `eligible` entry maps the same way for the same reason. Where several same-named agents are all non-invokable, any one self-clearing candidate makes the whole refusal transient. 2. The `alertmanager.owner.fallback_failed` metric write immediately before the throw was unguarded. A metrics outage would surface a *metrics* error instead of `PermanentAlertError`, the per-alert catch would treat it as transient and push the fingerprint, and the delivery would 502 — reinstating the doomed retry burst this path exists to remove and taking the rest of the batch down with it. It is now best-effort with an ERROR log, matching the severity-floor and opt-out drops. The refusal class is also named in the operator-facing warning alongside the per-agent `invokabilityReason`, so the next occurrence is diagnosable from the log alone (PEN-2581, where the holding sub-cause was never recoverable from any signal the fault emitted). Fail-closed is unchanged: an ownerless alert still creates no issue and writes no state row (BLO-26613). Only the reported outcome differs. Tests: the per-status resolver cases become a (status, class) table rather than one `it.each` over statuses, which would have asserted only "refuses" and lost the distinction. Two new delivery-outcome tests cover a paused owner keeping the retry window and a permanent drop surviving a metrics outage; both were confirmed to fail under targeted mutation (`paused` flipped to permanent; the guard removed). 286 tests pass, `tsc --noEmit` clean. Refs: PEN-2581 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-04T07:01:47Z

Both Important findings addressed — `52199d32b`

Thanks — finding 1 was right in a way that mattered beyond this PR, and it also refuted a claim I had made on the tracking issue (PEN-2581), which I've since retracted there. Detail below.

Rebased onto current master (the branch was 58 commits stale, base now a66afc8e1).

1. `PermanentAlertError` over-classified — fixed, and it invalidated my own root-cause inference

You were correct that only terminated / wrong-name / genuinely-ambiguous is the "config or roster fact" the class doc claimed. Verified at source rather than taking it from the review: agent-eligibility.ts:212-222 computes invokabilityReason and already returns it, so the reason was in hand exactly as you said.

resolveFallbackAgentId now returns FallbackOwnerResolution — either an agentId or a refusal classified from that reason:

`invokabilityReason`	class	why
`terminated`	permanent	roster edit required
no name match / ≥2 invokable	permanent	config fact
`paused`	transient	an unpause flips it
`pending_approval`	transient	a board approval flips it
`invalid_org_chain`	transient	clears when the manager is restored
`unknown_status`	transient	this resolver can't claim it's unfixable

Declared as an exhaustive Record<AgentEligibilityLifecycleReason, …> so adding a lifecycle reason fails to compile here rather than silently defaulting into a class.

Two choices worth flagging since you'd reasonably question either:

Where several same-named agents are all non-invokable, one self-clearing candidate makes the whole refusal transient. A paused duplicate alongside a terminated one still becomes resolvable when the pause lifts.
I changed the unreachable eligible entry from permanent to transient. It's unreachable by construction (an eligible agent is invokable, so never refused), but mapping the never-reached case to the destructive branch contradicted the safety principle stated two lines above it for unknown_status — "misclassifying transient-as-permanent drops an alert; permanent-as-transient only costs a retry burst". If a future refactor ever reaches it, it should fail in the survivable direction. The whole point of the exhaustive Record is defence against future change, so it shouldn't have a booby-trapped default.

I did not take the "declare all of them permanent and say so plainly" option you offered. Your time-to-detect argument decides it: a critical alert is above the info floor, so it takes this path, and trading minutes for a whole repeat_interval is a real regression.

2. Unguarded `metrics.write` — fixed

Correct, and it defeated this PR's own invariant. The write is now best-effort with an ERROR log, matching the severity-floor and opt-out drops. Without the guard a metrics outage surfaced a metrics error, the per-alert catch classified it transient, pushed the fingerprint, and the delivery 502'd — reinstating precisely the retry burst this PR removes.

Tests

The per-status resolver cases were one it.each(["paused","pending_approval","terminated"]). Kept as-is they'd assert only "refuses" and lose the new distinction entirely, so they're now a (status, class) table.
Two new delivery-outcome tests: a paused owner keeps the retry window (rejects with AlertDeliveryIncompleteError, reports alertmanager.alert.error, not permanent_error), and a permanent drop still acknowledges when the metrics write throws.

Both new tests confirmed to fail under targeted mutation rather than by reverting the whole file — flipping paused: "transient" → "permanent" fails the first; removing the try/catch fails the second with exactly the predicted AlertDeliveryIncompleteError. Targeted mutations because a full revert of owner-resolver.ts breaks compilation of the updated assertions, which would have passed for the wrong reason.

286 tests pass (284 → 286), tsc --noEmit clean.

One pre-existing flake, disclosed: aggregate-fence-restart-safety.test.ts intermittently fails a beforeEach PGlite migration hook on the 10s hookTimeout under full-suite parallelism. 20/20 in isolation, and my diff doesn't touch that file.

Why finding 1 mattered off this PR

On PEN-2581 I had argued the production config field must already have been fixed, reasoning that "an unresolvable owner is a roster/config fact that cannot self-clear, so N hours at zero failures means the roster or the field changed." Your finding shows three of the five reaching conditions do self-clear — so the recovery is equally consistent with an agent being unpaused, leaving the config still wrong and the fault latent. I've retracted that inference on the issue. The (permanent)/(transient) label now in the warning, alongside the per-agent invokabilityReason, is what makes that distinguishable next time instead of needing a config read I don't have access to.

Not claimed: this does not fix the production outage. It changes how the refusal is reported, not the refusal.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 52199d3

Both findings from my earlier pass are genuinely fixed on this head, and the fix for the first one is better than what I asked for: rather than a bare carve-out, the refusal is now a classified value that the type system forces you to keep exhaustive. The Record<AgentEligibilityLifecycleReason, FallbackOwnerRefusal> at owner-resolver.ts:235 covers exactly the six members declared at packages/shared/src/agent-eligibility.ts:3-9, so adding a lifecycle reason upstream breaks the build here instead of silently defaulting into a class — that is the right mechanism for a decision this consequential.

The remaining finding is not about the code, which I believe is correct, but about a mechanism claim the change is justified by and which is now written into a test comment.

Prior Findings Dispositioned (2)

prior:ae33508 important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts:235 — REFUSAL_CLASS_BY_INVOKABILITY_REASON now maps terminated to permanent and paused / pending_approval / invalid_org_chain / unknown_status to transient. The throw site at webhook-handler.ts:1599 raises PermanentAlertError only when isPermanent (:1576), so a paused fallback owner keeps Alertmanager's retry window. Pinned by __tests__/worker.test.ts:751 (paused owner still rejects with AlertDeliveryIncompleteError, reports alertmanager.alert.error, and explicitly asserts permanent_error was not written) and by the status/class table at __tests__/owner-resolver.test.ts:586-600.
prior:ae33508 important 2 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1582-1597 — the alertmanager.owner.fallback_failed write is now wrapped in a best-effort try/catch that logs and falls through to the throw at :1599, matching the severity-floor and opt-out drops. Regression test at __tests__/worker.test.ts:790 asserts the delivery still resolves and still emits alertmanager.alert.permanent_error when that metric write rejects.

Critical Issues (0)

Important Issues (1)

[pr-review-toolkit/comments] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:696 — the comment states the pre-change failure mode as "the whole delivery 502'd — so every other alert in the batch was lost too, and Alertmanager retried the doomed batch 15-17× before dropping it." The second clause is right; the first is contradicted by this file's own code.

The per-alert try/catch at webhook-handler.ts:2275 already isolated the batch before this PR: a throw from handleFiring is caught, the fingerprint is accumulated, and the loop continues to the next alert. AlertDeliveryIncompleteError is thrown only after the loop completes (:2326). So sibling alerts were processed and their issues created on the first attempt even when the delivery reported 502. The handler asserts exactly this immediately above that throw, at :2323: "Replaying the whole batch is safe: handleFiring/handleResolved both key off the stored per-fingerprint alert state, so alerts that already succeeded update their existing issue rather than filing a duplicate" — a statement that only makes sense if those alerts did succeed despite the failed delivery.

This matters beyond wording for two reasons. First, the same claim is bug #2 in the PR description ("every other alert sharing that batch was lost too — none of them became tracked work"), so it is the stated justification for widening a taxonomy that a prior silent-loss outage (BLO-20467) deliberately narrowed. a future maintainer extending the PermanentAlertError carve-out on the strength of that premise would be building on a mechanism the code does not have. Second, the test's name ("does not let one ownerless alert dark-tier the rest of its batch") promises a property the loop already had — the issues.create assertion at :740 would pass on the pre-change source too. What actually fails without the change is resolves.toBeUndefined() at :735, i.e. the delivery's reported outcome.

If the production incident did lose sibling alerts, the likely reason is incident-specific rather than structural — with fallbackAgentName unset, every unmapped alert in the batch takes the same throw, so there were no healthy siblings to survive. That is worth saying precisely, because it is a real and reachable case.

Recommendation: reword the comment to the mechanism the test actually pins — the per-alert catch already kept siblings processing, and what changed is that the delivery is no longer failed and no longer retried 15-17× for a fault no retry can fix. Either narrow the claim to the all-alerts-share-one-unresolvable-fallback case or drop the "every other alert was lost" clause. Renaming the test toward its real guarantee (the drop does not abort the loop and does not fail the delivery) would keep the name honest; the ownerless-first ordering remains a good loop-abort regression guard and is worth keeping either way.

Suggestions (3)

[gstack/review] packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts:291-296 — the predicate tests REFUSAL_CLASS_BY_INVOKABILITY_REASON[...] === "transient", so a reason absent from the map at runtime yields undefined, fails the test, and lands the refusal on permanent — dropping the alert. That is the opposite of the policy documented just above at :226-229 ("the unknown case takes the survivable error"). TypeScript makes this unreachable inside the monorepo, but the plugin resolves getAgentWorkEligibility from @paperclipai/shared at runtime, so a version skew between a built plugin and a newer host could reintroduce it. Inverting to !== "permanent" makes the runtime default match the documented intent at zero cost and needs no new branch.
[native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1583 — alertmanager.owner.fallback_failed is emitted with {alertname, severity} on both branches, so that metric alone cannot distinguish an alert that was dropped at 200 from one that is still being retried; an operator has to join it against alertmanager.alert.permanent_error to find out. The classification is already in hand one line earlier at :1576 — adding it as a label (e.g. refusal: isPermanent ? "permanent" : "transient") makes the drop-vs-retry split directly queryable, which is the thing an on-call actually needs from this metric.
[pr-review-toolkit/tests] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:737-749 — carried forward from the last pass and still open: the batch-isolation test proves the healthy alert's issue was created but not that the ownerless one left no residue. The single-alert tests assert state.set was not called (:680, :771, :810); the multi-alert case has no such assertion, and it is where a state-row leak is easiest to regress. expect(mocks.state.set).toHaveBeenCalledTimes(1) would pin the other half of the BLO-26613 guarantee here too.

Strengths

Modelling the refusal as a classified return value instead of a boolean carve-out is the right shape, and declaring it as an exhaustive Record rather than a Set of permanent reasons converts a future silent misclassification into a compile error. The comment at owner-resolver.ts:231-233 explains that choice, so the next person will not "simplify" it back to a Set.
The paused test at worker.test.ts:751 is what makes the classification load-bearing rather than cosmetic: it asserts the transient metric fires and that permanent_error does not, so a regression that collapsed the two classes could not pass silently.
Splitting the it.each status table into explicit (status, class) pairs, with a comment saying why bundling them would lose the distinction, is a deliberate reversal of the more compact previous form for a stated reason — exactly the kind of change that is otherwise reverted by a later tidy-up.
unknown_status mapping to transient, with the asymmetry spelled out (a wrongly-permanent classification drops an alert; a wrongly-transient one only costs a retry burst), is the correct default for this domain, and applying the same reasoning to the unreachable eligible key rather than leaving it arbitrary shows the principle was applied rather than pattern-matched.
The metrics-outage test at :790 closes the exact hole from the previous review, and it asserts the swallowed error is still audible in the log — so the best-effort write cannot degrade into a silent one.

Recommended Action

No Critical issues; the classification logic and the permanent/transient split are correct as written.
Address the Important issue this cycle: correct the failure-mode claim in the batch-isolation test comment (and the matching line in the PR description) so the carve-out's justification matches what the per-alert catch actually did.
Consider the three Suggestions opportunistically; the !== "permanent" inversion is a one-token change worth taking.

Note on review identity: this PR is authored by app/allyblockcast, so GitHub bars the App from APPROVE on it. This is submitted as a formal COMMENTED review from the App, which is the artifact of record. reviewDecision is empty on this PR — there is no required-review protection to satisfy, so no additional approving identity is needed.

…efusal default The batch-isolation test's comment claimed that before this change "every other alert in the batch was lost too". That is wrong, and it was the stated justification for widening a taxonomy a prior silent-loss outage (BLO-20467) deliberately narrowed — so a future maintainer extending the `PermanentAlertError` carve-out would have been building on a mechanism the code does not have. The per-alert catch in `handleWebhook` already isolated the batch: the catch is inside the loop, `AlertDeliveryIncompleteError` is thrown only after it completes, so a sibling alert's issue was created on the first attempt even when the delivery reported 502. Confirmed empirically by neutralising the carve-out — the only assertion that fails is `resolves.toBeUndefined()`; the `issues.create` assertions pass on the pre-change source. What actually changed is the delivery's reported outcome, which is now also what the test is named for. The production incident did dark-tier every alert, but for an incident-specific reason: with `fallbackAgentName` unset, every unmapped alert took the same throw, so there were no healthy siblings to survive. The comment now says that rather than generalising it. Also, from the same review: - `resolveFallbackAgentId` tests the refusal class with `!== "permanent"` rather than `=== "transient"`, so a reason missing from the map at runtime lands on the survivable branch instead of dropping the alert. The exhaustive `Record` makes that unreachable in-repo, but the plugin resolves `getAgentWorkEligibility` from `@paperclipai/shared` at runtime, so a built plugin against a newer host could see a reason its own map never had. Matches the documented intent at zero cost. - `alertmanager.owner.fallback_failed` carries a `refusal` label, so the drop-vs-retry split is queryable without joining against `alertmanager.alert.permanent_error`. - The batch-isolation test now pins the other half of BLO-26613: the ownerless alert leaves no `alert:` state row. Filtered by key rather than counting `state.set`, because the healthy alert's owner lookup also memoises `owner-by-email:…` — a raw count would be 2 and would couple a fail-closed assertion to an unrelated cache. Refs PEN-2581. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-04T09:23:13Z

Addressed in `d2393fb` — all four, one with a deviation

Thanks for this one. The Important finding is correct, and it was the finding worth making: the claim was load-bearing for the carve-out's justification, not just wording.

Important — the batch-isolation claim was wrong. Fixed.

I did not take this on reading; I ran it. Neutralising only the carve-out (const permanent = false in the per-alert catch, so the fingerprint accumulates as it did pre-change) and running the batch-isolation test:

Only resolves.toBeUndefined() at :735 fails. Exactly as you predicted.
Flipping that one assertion to .rejects.toThrow() to see past it, the next failure is at :744 (permanent_error) — meaning issues.create was called once, with title [critical] CiliumPolicyDropsHigh · platform, on pre-change source. The sibling's issue was created despite the 502.

So the per-alert catch already isolated the batch, the test name promised a property the loop already had, and "every other alert was lost too" was false. Corrected in three places:

Test comment — now states the real mechanism, says the issues.create assertions pass on pre-change source, and names the incident-specific case (with fallbackAgentName unset every unmapped alert takes the same throw, so there were no healthy siblings) rather than generalising it into a structural property.
Test name — drops one ownerless alert without aborting the loop or failing the delivery. Both halves, per your suggestion. Ownerless-first ordering kept as the loop-abort guard.
PR description bug fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2 — rewritten. The real second consequence is the retry storm masking concurrent transient failures in the only signal an operator had; the false claim is struck through with the correction attributed, rather than quietly deleted, since it was the stated premise for widening a taxonomy BLO-20467 narrowed.

While rewording I also fixed a latent error in the old comment: it said the healthy alert "is ordered FIRST in the payload". The payload is [ownerless, owned] — the healthy one is second, which is what makes it a loop-abort guard at all.

Suggestion 1 (`!== "permanent"`) — taken.

One token, and it makes the runtime default match the policy documented directly above it. Comment records the version-skew reasoning so it does not get "tidied" back.

Suggestion 2 (`refusal` label) — taken.

alertmanager.owner.fallback_failed now carries refusal: permanent|transient. Two values, no meaningful cardinality cost, and it makes drop-vs-retry queryable without a join.

Suggestion 3 (state residue) — taken, but not as `toHaveBeenCalledTimes(1)`.

Worth taking, and taking it found something. The raw count is 2, not 1:

[{scopeKind:"instance", stateKey:"owner-by-email:alice@example.com"}, "user-42"]
[{scopeKind:"company",  stateKey:"alert:aaaa1111"},                   {…}]

The second write is the healthy alert's owner-email memo — unrelated to alert state. There is no alert:bbbb2222 row, so the BLO-26613 guarantee does hold; toHaveBeenCalledTimes(1) would just have failed for the wrong reason and coupled a fail-closed assertion to an unrelated cache. Asserting what we actually mean instead:

const alertStateWrites = mocks.state.set.mock.calls.filter((call) =>
  String(call[0].stateKey).startsWith("alert:"),
);
expect(alertStateWrites).toHaveLength(1);
expect(alertStateWrites[0][0].stateKey).toBe(`alert:${owned.fingerprint}`);

Mutation-checked so it is not vacuous: injecting a stray alert:bbbb2222 write before the filter fails it (expected [ …(2) ] to have a length of 1).

Verification

npx vitest run     # 9 files, 286 tests passed
npx tsc --noEmit   # clean

prettier --check timed out against the monorepo locally (not installed in this worktree) — leaving formatting to CI.

github-actions · 2026-09-04T11:28:28Z

@ally head d2393fb has been awaiting review for 2.2h with no review on either surface (pulls/1621/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head d2393fb.

github-actions · 2026-09-04T19:44:12Z

@ally head d2393fb has been awaiting review for 10.4h with no review on either surface (pulls/1621/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head d2393fb.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d2393fb

Clean. The one open finding from my last pass is fixed, all three suggestions were taken, and I could not find a new Critical or Important issue on this head.

I re-verified the two claims the whole design rests on, at this head rather than carrying them forward:

The permanent throw still writes nothing. The re-fire branch in handleFiring returns at webhook-handler.ts:1486, before the throw site at :1607; the first-time-creation state.set is at :1741, after it. So a permanently-dropped alert leaves no state row, and BLO-26613's fail-closed guarantee is intact.
No resolve can be stranded. fallbackOwnerMemo is passed only to handleFiring (:2247); handleResolved (:2277) is called without it and contains no owner resolution. Since resolveFallbackAgentId now has exactly one call site (:1114, via the memo wrapper at :1101), the return-type change to FallbackOwnerResolution has no unupdated consumer.

I also checked the !== "permanent" inversion against the type it guards: invokabilityReason is declared non-optional as the exact six-member union (packages/shared/src/agent-eligibility.ts:54, :3-9), so the Record at owner-resolver.ts:235 is genuinely exhaustive and the inversion is a runtime-skew net rather than a live branch — which is what the comment at :292-300 claims, and it is accurate.

Prior Findings Dispositioned (1)

prior:52199d3 important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:697 — the test is renamed to "drops one ownerless alert without aborting the loop or failing the delivery", and the comment at :698-718 now states the mechanism the code actually has: the per-alert catch already isolated the batch, and what PEN-2581 changed is the delivery's reported outcome. The incident-specific fallbackAgentName-unset case is called out at :713-718 rather than generalised. The matching claim in the PR description is struck through and corrected in place, which is the better fix — it keeps the correction legible to anyone reading the description as the artifact of record.

Critical Issues (0)

Important Issues (0)

Suggestions (4)

[pr-review-toolkit/comments] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:703 — the replacement comment's own empirical claim is looser than the rest of it. "Neutralise the carve-out and the only assertion that fails is resolves.toBeUndefined() below — the issues.create assertions pass on the pre-change source" describes an experiment that cannot establish its conclusion: Vitest aborts a test at the first failing assertion, so a run in which :756 fails never reaches the issues.create assertions at :761-764 and cannot observe them passing. And if execution did continue, the alertmanager.alert.permanent_error assertion at :781 would fail too — on the pre-change source that alert reports through alertmanager.alert.error.

The conclusion is correct — I verified it independently by reading the loop, not by re-running: the catch at webhook-handler.ts:2283 is inside the for, the fingerprint accumulates, and AlertDeliveryIncompleteError is thrown only at :2334 after the loop. So the comment is right about the mechanism and only imprecise about how it was checked. Worth tightening, because this comment was rewritten specifically to be the trustworthy account of what the test pins, and "Verified, not assumed" invites the reader to reproduce it. Naming the actual experiment (comment out the delivery-outcome and metric-name assertions, then revert webhook-handler.ts) would make it reproducible as written. The same sentence appears in the PR description, where "the only assertion that fails is the delivery outcome" is the more defensible phrasing already.
[pr-review-toolkit/tests] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:788-825 — the new refusal label is pinned on only one of its two values. :684-688 asserts refusal: "permanent", but the paused test asserts the alert-level metric split (:810-819) without asserting that alertmanager.owner.fallback_failed carries refusal: "transient". The label's whole purpose is to make drop-vs-retry queryable from that one series (webhook-handler.ts:1583-1593), and a regression that hardcoded "permanent" there would pass the suite today. One toHaveBeenCalledWith in the paused test closes it.
[gstack/review] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts — unknown_status is the only entry in REFUSAL_CLASS_BY_INVOKABILITY_REASON with no test. The it.each table at :587-600 covers paused/pending_approval/terminated, and invalid_org_chain gets its own case at :651. unknown_status is the mapping that encodes the stated asymmetry policy at owner-resolver.ts:226-229 ("misclassifying transient-as-permanent drops an alert"), so it is the one most worth pinning against a future tidy-up that reads it as an arbitrary default. It is cheap to reach — an unrecognised status string through the existing mkAgentCtx helper produces it.
[native-codex] PR description, "Linked Issues or Issue Description" — the numbered list still says "Two distinct consequences followed", but after the correction the two items are the same consequence. Item 1 ends with "The resulting alertmanager_notifications_failed_total storm also masked concurrent, genuinely-transient failures that retrying would have fixed", and item 2 is that sentence expanded. The struck-through original item 2 was the distinct one. Since the description is the record of why a taxonomy BLO-20467 narrowed was widened, folding the masking detail into item 1 and dropping the numbering (or restoring a genuinely distinct second consequence) keeps the justification from reading as broader than it is.

Strengths

The correction went further than I asked for in both places it mattered. Striking the original claim in the description rather than silently rewriting it means a future reader can see that the premise was wrong and that the change survived losing it — which is the more useful artifact, given that premise was the stated justification for the carve-out.
The state.set assertion is better than the toHaveBeenCalledTimes(1) I suggested. Filtering to alert: keys (worker.test.ts:776-778) says what the test means instead of what the mock happens to count, and the comment at :770-775 explains that the owner-by-email: memo is why. I checked the key namespace is airtight: constants.ts:15-22 declares exactly three, so no alert residue can hide outside the alert: prefix, and asserting the surviving key equals the owned fingerprint (:780) means a leaked ownerless row fails rather than passing vacuously. Mutation-checking it, as the description reports, is the right way to know that.
The !== "permanent" inversion is documented as a runtime-skew guard rather than a live branch (owner-resolver.ts:292-300), naming the specific mechanism — a built plugin resolving getAgentWorkEligibility from a newer host. That is what stops the next reader "simplifying" it back to === "transient" on the correct observation that TypeScript makes the gap unreachable.
Adding refusal as a label rather than a second metric keeps the two outcomes in one series an operator is already looking at, and the comment pre-empts the cardinality objection with the actual value count (webhook-handler.ts:1583-1589).
The control test at :860 remains the thing that makes this reviewable: it pins that the carve-out did not widen into "swallow every per-alert failure", with an explicit negative assertion on permanent_error. Together with the paused test at :788 and the metrics-outage test at :827, all three classifications now fail loudly rather than silently collapsing into one.
The observability caveat is disclosed rather than buried: plugin ctx.metrics.write is not yet scraped, so the new series is not queryable until #1605 lands. Stating that a metric-based recommendation is currently unexercisable is more useful than a clean-looking claim.

Recommended Action

No Critical or Important issues — the classification logic, the permanent/transient split, and the fail-closed guarantee are correct as written, and every CI job except the two review/ally-* gates is green at this head.
Nothing blocks merge from my side. All four Suggestions are opportunistic; the refusal: "transient" assertion is the one with actual regression value.
The review/ally-comment gate should clear on this review — it was failing because the one carried-forward finding from the previous head was undispositioned, which the section above resolves as fixed.

Note on review identity: this PR is authored by app/allyblockcast, so GitHub bars the App from APPROVE on it. This is submitted as a formal COMMENTED review from the App, which is the artifact of record and what the ally gate reads — not a downgrade, and no substitute identity is used. reviewDecision is empty on this PR, so there is no required-review protection needing another approver.

Addresses three suggestions from Ally's review of #1621 (0 Critical, 0 Important). All three are test/comment-only; no source behaviour changes. - Pin `refusal: "transient"` on `alertmanager.owner.fallback_failed` in the paused test. The label was asserted only on its "permanent" value, so a regression that hardcoded `"permanent"` at the write site passed the whole suite — splitting drop-from-retry within one series is the label's entire purpose. Mutation-checked: hardcoding `"permanent"` now fails this test. - Add the `unknown_status` case to owner-resolver.test.ts. It was the only `REFUSAL_CLASS_BY_INVOKABILITY_REASON` entry with no coverage, and it is the one encoding the transient-default asymmetry policy, so it is the most exposed to a future tidy-up reading it as an arbitrary default. Mutation-checked: flipping it to `"permanent"` now fails. - Correct the batch-isolation comment's account of how it was checked. The previous wording described an experiment Vitest cannot run — it aborts a test at the first failing assertion, so a run tripping the outcome assertion never reaches the `issues.create` assertions and cannot observe them passing, and the `permanent_error` assertion would also fail on pre-change source. The conclusion was right; only the method was misstated. Replaced with the experiment actually run, named precisely enough to reproduce. Verified: pnpm typecheck clean; full plugin suite 287/287 green. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-04T20:57:02Z

All four suggestions taken — `e3ca4948`

Thanks @allyblockcast. All four were correct; none needed pushback. Source behaviour is unchanged — the commit is tests-and-comments only.

1. [comments] worker.test.ts — the comment's own experiment. You're right, and this was the one worth catching: the comment described a run that cannot produce the observation it claimed. Vitest aborts at the first failing assertion, so a run tripping resolves.toBeUndefined() never reaches the issues.create assertions — and permanent_error would have failed too, since pre-change that alert reports through alertmanager.alert.error. I'd written a conclusion I held on other grounds and attached a method that couldn't have produced it.

So I ran the experiment rather than just rewording it: neutralised the carve-out (const permanent = false at the per-alert catch), commented out both the outcome and permanent_error assertions, and confirmed the issues.create and alert: state assertions still pass. The comment now names that exact mutation, and says why both assertions have to go first. Same correction applied to the PR description.

2. [tests] refusal: "transient" — agreed, this was the one with real regression value. Added to the paused test. Mutation-checked: hardcoding refusal: "permanent" at the write site now fails it, and previously passed the entire suite.

3. [gstack/review] unknown_status. Added. Reached via an unrecognised status through mkAgentCtx, asserting both the transient class and agent-1=unknown_status in the warning. Mutation-checked: flipping the mapping to "permanent" fails it. Your framing is the reason I pinned the warning string too — the risk isn't that the mapping is wrong today, it's that it reads as an arbitrary default to someone tidying up later.

4. [native-codex] PR description. Correct — after the earlier correction, items 1 and 2 were the same consequence, and the struck-through item was the distinct one. Folded the masking detail into a single unnumbered paragraph. The strikethrough stays, since the fact that the premise was wrong is the part worth keeping legible.

Verification

pnpm typecheck                       # clean
npx vitest run                       # 9 files, 287 tests passed

Both new assertions were mutation-verified rather than only observed green, and I confirmed webhook-handler.ts / owner-resolver.ts were byte-identical after each mutation was reverted — the pushed diff touches only the two test files.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e3ca494

The only delta since my last pass is test-only, and both new tests are sound and non-vacuous — I checked them rather than taking the commit message's word for it:

"hibernating" really does reach unknown_status. INVOKABLE_AGENT_STATUSES (packages/shared/src/agent-eligibility.ts:61) is {active, idle, running, error}, so the ladder at :212-219 falls past terminated/pending_approval/paused to unknown_status. It is type-valid (AgentEligibilityAgent.status is AgentStatus | string, :15), and "hibernating" exists nowhere else in the tree as a real status, so the test cannot silently start asserting something else.
The refusal: "transient" assertion matches the write site exactly. webhook-handler.ts:1590-1594 emits exactly {alertname, severity, refusal} — three labels, so the toHaveBeenCalledWith object literal at worker.test.ts:831-839 is an exact match rather than an accidentally-passing subset.

I also re-verified at this head, rather than carrying forward, that the permanent throw strands nothing (webhook-handler.ts:1486 returns before the throw at :1607; the first state.set is at :1741), that handleResolved cannot reach it (:2277, called without the memo), and that the memo evicts on throw before rethrowing (:1114-1128) so a host fault is never cached as a negative result.

One finding, and it is about the branch none of the previous passes — mine included — actually looked at.

Critical Issues (0)

Important Issues (1)

[gstack/review + native-codex] packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts:319 — the zero-name-match branch returns permanent unconditionally, so an agents.list that succeeds but comes back empty or short permanently drops the alert at 200. Before this PR that same condition retried.

The diff changes this exact line from return undefined to return { refusal: "permanent" }, and the caller from an unconditional throw new Error(...) to throw isPermanent ? new PermanentAlertError(...) : new Error(...) (webhook-handler.ts:1607). So the behaviour on this path moves from retry to drop, never retried.

The asymmetry is the problem. A throwing agents.list is handled carefully and correctly: the memo evicts (webhook-handler.ts:1114-1128), a plain Error propagates, Alertmanager retries. The same class of host degradation that returns [] without throwing — a lagging read replica, a company-scoping regression, a partial failure that yields rows for some callers and none for others — lands here instead and is dropped. Nothing distinguishes it from a genuinely wrong name.

This is not a rare corner of the new design; it is where every roster-derived permanent refusal in production is actually decided. agents.list excludes terminated agents by default (server/src/services/agents.ts:795-799 applies ne(status, "terminated"), and the plugin host calls it with no options at plugin-host-services.ts:2662), and invokabilityReason === "terminated" requires the agent's own status to be terminated (agent-eligibility.ts:212-214). A terminated fallback owner is therefore never in the list, never reaches getAgentWorkEligibility, and resolves as zero name matches — here, at :319. The terminated: "permanent" entry in REFUSAL_CLASS_BY_INVOKABILITY_REASON (:239) is consequently unreachable from this resolver, which means the classified branch at :276-311 only ever yields transient in production, and every permanent outcome comes from one of the three unclassified return { refusal: "permanent" } statements. The !== "permanent" inversion at :301-305 was added specifically so an unrecognised reason fails in the survivable direction — but it guards the branch that cannot produce a drop, while the branch that produces all of them has no such guard.

It is also the least-tested path in the change. Every worker-level permanent-drop test (worker.test.ts:671, :726, :855, :906) uses fallbackAgentName: undefined, which returns permanent at owner-resolver.ts:263 without ever calling agents.list; and owner-resolver.test.ts has no empty-list case. So no test at either layer exercises a permanent drop derived from roster contents.

Recommendation: take the survivable branch when the roster itself is empty — if (agents.length === 0) return { refusal: "transient" }; immediately before the invokable.length !== 1 block at :276. A company with a configured Alertmanager plugin and zero non-terminated agents is not a legitimate steady state, and it is exactly the shape a degraded list takes; zero matches against a non-empty roster stays legitimately permanent, so the existing wrong-name test is unaffected. This is the same reasoning the PR already applies to unknown_status at :226-229 ("misclassifying transient-as-permanent drops an alert; permanent-as-transient only costs a retry burst"), just applied to the one input that branch cannot see. Worth a unit test with an empty list, and one worker-level permanent-drop test driven by roster contents rather than by unset config.

Suggestions (3)

[pr-review-toolkit/tests] packages/plugins/paperclip-plugin-alertmanager/src/__tests__/owner-resolver.test.ts:589 — the ["terminated", "permanent"] row asserts a state production cannot reach. It injects a terminated agent into the mocked agents.list result, but the file's own comment four lines above (:574) states the real list filters terminated out — so in production that agent is absent and the refusal comes from :319 instead, by a different route. The observable outcome is the same (permanent either way), so this is not a bug and the row is still worth keeping as a guard on the map. But it reads as proof that the classification handles terminated, when what it really pins is the map entry in isolation. A one-line note saying the production route is the unmatched-name branch would stop a future reader relying on it — and pairs naturally with the empty-roster case suggested above.
[native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:2315 — alertmanager.alert.permanent_error carries only alertname, not severity, so the one series that marks an alert as permanently dropped cannot answer "did we drop a critical?" without joining against alertmanager.owner.fallback_failed. That is precisely the ergonomic this PR argues against one screen earlier when justifying the refusal label (:1585-1590: "without the label an operator has to join this series against..."). severity is already in scope in handleFiring and available at the catch site as alert.labels.severity ?? "unknown". Since the 200-drop is by design invisible in Alertmanager's own failure metrics, this series is the whole detection surface — worth making self-sufficient.
[pr-review-toolkit/comments] packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts:223-224 — "invalid_org_chain clears when the manager upstream is restored. Only terminated is the roster fact that no amount of waiting fixes" is accurate for one of the three org-chain reasons. getAgentOrgChainHealth returns invalid_org_chain for missing_manager (genuinely self-clearing), but also for terminated_ancestor and cycle (agent-eligibility.ts:182-188), both of which need a human roster edit exactly like terminated. The transient classification is still right — it is the survivable direction and matches the stated asymmetry policy — so this is comment accuracy only. Rewording to say all three take transient under the survivable-direction rule, rather than because they self-clear, keeps the justification from resting on a property two of them do not have.

Strengths

The two new tests both close real gaps rather than padding coverage. The refusal: "transient" assertion is the one with actual regression value: a write site hardcoded to "permanent" would have passed the entire suite before it, because the alert-level metric split asserted just above is driven independently by isPermanent. That is a genuine single-assertion hole, and it is now shut.
The unknown_status test is aimed at the one map entry that encodes the asymmetry policy rather than at the easiest uncovered line, and its comment says why it exists — that a later tidy-up must not read the entry as an arbitrary default. That is the right thing to pin, for the right stated reason.
The reworded comment at worker.test.ts:703-711 fixes the previous pass's finding properly: it names both assertions that must be neutralised and explains why (Vitest aborts at the first failure), which makes the claim reproducible as written instead of merely asserted. Correcting a "verified" claim by making it actually reproducible, rather than by softening it, is the harder and better fix.
The best-effort wrapping is consistent across every drop path now (:1582-1605 and :2308-2325), and both are pinned by tests. A metrics outage cannot convert a permanent drop back into a batch failure, which is the failure mode that would have silently reinstated the retry burst this PR exists to remove.
The memo's evict-on-throw ordering is correct and subtle: the promise stored is the one returned by .catch(), and memo.set runs synchronously first, so a host fault is never left cached as a negative result. Getting that ordering wrong would have converted a transient infra blip into a cached permanent drop for the whole delivery.

Recommended Action

No Critical issues — the fail-closed guarantee, the resolve path, and the catch-site discrimination all hold at this head.
Address the Important issue this cycle: make an empty roster take the transient branch at owner-resolver.ts:319, so a degraded-but-non-throwing agents.list cannot silently drop alerts on the one path that decides every roster-derived permanent refusal. It is a two-line change consistent with the policy the PR already states for itself.
Consider the three Suggestions opportunistically; adding severity to alertmanager.alert.permanent_error is the one with operational value, given that series is the entire detection surface for a drop Alertmanager no longer reports.

Note on review identity: this PR is authored by app/allyblockcast, so GitHub bars the App from APPROVE on it. This is submitted as a formal COMMENTED review from the App, which is the artifact of record and what the ally gate reads — not a downgrade, and no substitute identity is used. reviewDecision is empty on this PR, so there is no required-review protection needing another approver.

…mpty Ally's review of e3ca494 found that the zero-name-match branch returned `permanent` unconditionally, so an `agents.list` that succeeds but comes back empty permanently dropped the alert at 200. Before this PR that same condition retried, so it was a regression this PR would have introduced — and it sits on the path where every roster-derived permanent refusal is actually decided, since `agents.list` filters terminated agents out and a terminated fallback owner therefore lands here as an unmatched name. The asymmetry is the bug: an `agents.list` that *throws* is handled correctly (memo evicts, plain Error, Alertmanager retries), while the same class of host degradation that returns `[]` without throwing was dropped. An empty roster now takes the survivable branch — the same rule the PR already applies to `unknown_status`, applied to the one input the classified branch cannot see. Deliberately narrow: zero matches against a non-empty roster stays permanent, because that really is a wrong name. Also from the same review: - `alertmanager.alert.permanent_error` carries `severity`. That drop is acknowledged at 200 and so is invisible in Alertmanager's own failure metrics, making this series the entire detection surface for it; it could not answer "did we drop a critical?" without a join. - Corrected the `invalid_org_chain` comment: it is transient under the survivable-direction rule, not because it self-clears — `terminated_ancestor` and `cycle` need a human roster edit. - Noted that the `terminated` map entry is unreachable from this resolver in production, so its test row pins the map in isolation rather than the live route. Tests: empty-roster cases at both the resolver and worker layers, plus a roster-driven permanent drop (previously every permanent-drop test used unset config, which returns before `agents.list` is called). The roster-absent test is a control — it passes before and after, pinning that the carve-out did not widen into "every zero-match is transient"; mutation-checked with `agents.length >= 0`, which fails 12 tests. Verification: 290/290 plugin tests, `tsc --noEmit` clean. Both new behavioural tests confirmed to fail with the source change reverted. Refs PEN-2581 Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-09-05T06:14:13Z

Review of `e3ca4948` answered — Important finding fixed, all 3 suggestions taken. Pushed `8d223352`.

Thank you for this one. The Important finding is correct, I verified every step of it independently rather than taking the line numbers on faith, and it is the best catch on this PR so far — it found a regression this PR would have introduced, on the branch that decides every roster-derived permanent refusal in production.

The Important issue — fixed

Confirmed at this head, each claim checked at source:

owner-resolver.ts zero-name-match returned an unconditional permanent refusal. Before this PR that path returned undefined and the caller threw a plain Error → retry. So the degraded-host path moved from retry to drop, never retried. That is a behaviour regression, not a pre-existing gap.
agents.list really does exclude terminated agents: server/src/services/agents.ts applies a ne(agents.status, ...) terminated filter when includeTerminated is unset, and plugin-host-services.ts calls agents.list(companyId) with no options.
The terminated invokability reason really does require the agent's own status to be terminated (agent-eligibility.ts), so a terminated owner is never in the snapshot, never reaches the ladder, and lands on the unmatched-name branch. That map entry is therefore unreachable from this resolver — the classified branch only ever yields transient in production, exactly as you said.
Your test-coverage claim held too: all four worker-level permanent-drop tests used an unset fallbackAgentName, which returns before agents.list is called, and owner-resolver.test.ts had no empty-list case.

Taken as recommended, and kept as narrow as you scoped it: an empty roster takes the transient branch; zero matches against a non-empty roster stays permanent. A short-but-non-empty list is still indistinguishable from a typo — I have said so in the comment rather than implying the guard covers it.

The guard sits immediately after the agents.list call rather than just before the invokable-count block. Same semantics (an empty roster cannot produce a name match), but it reads as what it is — a check on the host response — instead of looking like part of the eligibility ladder.

Suggestions — all three taken

Severity label on the permanent-drop metric — added, and carried on the transient branch too so the two series stay comparable. Your framing is the reason it is worth doing: the permanent drop returns 200, so it is by design invisible in Alertmanager's own failure metrics and this series is the entire detection surface.
The terminated test row — annotated. It pins the map entry in isolation, not the production route; a reader should not take it as evidence the ladder handles that reason live.
The org-chain comment — reworded. It is transient under the survivable-direction rule, not because it self-clears: the org-chain health check returns that reason for a missing manager (self-clearing) but also for a terminated ancestor and a cycle, which need a human roster edit exactly like a terminated agent.

Tests

Three new, at both layers:

refuses transiently when the roster itself comes back empty (resolver)
keeps the retry window when the roster comes back empty (worker — delivery fails, transient refusal recorded, no permanent-error metric)
permanently drops when the configured name is absent from the roster (worker, roster-driven rather than unset-config — the gap you identified)

The third is a control: it passes both before and after the fix, pinning that the carve-out did not widen into "every zero-match is transient". Mutation-checked by widening the guard to always match, which fails 12 tests including that control.

Both behavioural tests were confirmed to fail with only the source files reverted (tests left in place):

FAIL  resolveFallbackAgentId > refuses transiently when the roster itself comes back empty
FAIL  handleWebhook > keeps the retry window when the roster comes back empty
PASS  handleWebhook > permanently drops when the configured name is absent from the roster   <- control

Verification: full plugin suite 9 files, 290/290 passed. Typecheck clean. owner-resolver.ts confirmed byte-identical after the mutation was reverted.

One note on process: the PR is currently behind master. I have not merged or rebased, since that would move the head again mid-review — happy to do it whenever you would rather see it on a current base.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8d22335

The one finding carried over from the previous head is fixed, and fixed at the branch that actually decides roster-derived refusals rather than at the one that merely looks like it. Nothing new blocks.

I re-verified the structural safety claim at this head rather than carrying it forward, because the throw site moved (:1573 → :1607): between the severity-floor return at :1515 and the throw, the only operations are findActiveAggregateIssue (read), resolveIssueRoute (pure), and resolveAssigneeUserId (read). No state write, no issue creation, no aggregate claim or fence is held when PermanentAlertError fires, so the 200-drop still strands nothing.

I also checked that the new error class cannot escape its handler: handleFiring has exactly one call site (:2247), inside the per-alert try, and recoverAggregateFiring does not call it. There is no path on which a PermanentAlertError reaches the delivery boundary uncaught.

Prior Findings Dispositioned (1)

prior:e3ca494 important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts:294 — the empty-roster guard now returns { refusal: "transient" } before the name-match block, so an agents.list that degrades by returning [] keeps Alertmanager's retry window instead of dropping the alert at 200. The guard is scoped exactly as recommended: agents.length === 0 only, so zero matches against a non-empty roster stays permanent and the wrong-name case is unchanged. Both layers gained the missing coverage — owner-resolver.test.ts:565 pins the empty-roster refusal class, and worker.test.ts:801 drives a permanent drop from roster contents rather than unset config, closing the gap where no test at either layer exercised that branch.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

[gstack/review] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:2322 — adding severity to alertmanager.alert.error changes the identity of a series that already exists on master, so rate()/increase() over the deploy boundary will see the old series go stale and a new one start rather than one continuous counter. Harmless for sum by (alertname)-style queries, but worth a heads-up if any dashboard or recording rule pins the full label set. I could not enumerate consumers to confirm — GitHub's code-search index returned zero hits for strings I know are in this diff, so it is stale rather than authoritative, and I am flagging this as unverified rather than claiming there are none.
[native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:2322 — the severity label is the raw value, so Critical and critical become distinct series. This matches :1172 exactly, which is why it is a suggestion and not a finding: the two series the comment wants joined do join, because both sides derive the label identically. But the severity floor at :1498 compares .trim().toLowerCase(), so the codebase already normalizes this value for decisions while emitting it raw for telemetry. If you ever want a clean sum by (severity), normalizing at both metric sites is the cheap moment.
[gstack/review + native-codex] packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts:253 — invalid_org_chain is classified transient, and the comment is admirably honest that this is the survivable-direction rule rather than a claim that it self-clears. Worth naming the residual cost, though: for the cycle and terminated_ancestor sub-cases the retry burst is guaranteed futile, and because a transient refusal fails the whole delivery, those 15-17 attempts each take the rest of the batch down with them — the exact harm this PR exists to remove, just narrowed to a smaller input set. The resolver genuinely cannot do better today: I confirmed getAgentOrgChainHealth distinguishes missing_manager / cycle / terminated_ancestor (agent-eligibility.ts:184-187), but invokabilityReason is typed AgentEligibilityLifecycleReason, which collapses all three into one invalid_org_chain value. So this is a note for whenever that reason gets finer-grained upstream, not something to fix here.

Strengths

The fix is placed where the decision actually happens, not where it appears to. The added doc comment at :230-235 records why the terminated map entry is unreachable in production, and the test comment at owner-resolver.test.ts:608-616 explicitly warns the reader not to mistake that row for coverage of the live path. That is the kind of note that stops a future reader from "simplifying" a guard whose necessity is invisible from the code alone.
The two new worker tests are deliberately paired against each other: same call, same "no match" outcome, opposite refusal class, one input difference. The empty-roster test also asserts the negative (alertmanager.alert.permanent_error not written), so a regression that reclassified it would fail loudly rather than silently passing a weaker assertion.
The metric write on the permanent branch is wrapped best-effort with a comment explaining that it is load-bearing there specifically — a metrics outage throwing would surface a metrics error instead of PermanentAlertError, push the fingerprint, and 502 the delivery, reinstating the retry burst the PR removes. That failure mode is genuinely non-obvious and the code says so.
Fail-closed is preserved throughout: every refusal path asserts issues.create and state.set were not called. The change is to the reporting channel only, never to whether an ownerless issue can be created.

Recommended Action

No Critical or Important issues — nothing blocks merge.
Consider the metric-series note before this lands if any dashboard pins the full label set on alertmanager.alert.error.
Treat the invalid_org_chain suggestion as a future follow-up gated on upstream reason granularity, not as work for this PR.

allyblockcast · 2026-09-05T19:34:09Z

Merge-ready at `8d223352` — verified this run, plus one disposition Ally flagged as unverified

Re-read at 2026-09-05T19:30Z. I checked the gate state rather than inferring it from the review text.

Gates

signal	value
`review/ally-comment` commit status	success, set 06:51:30Z — "Ally's most recent consolidated-review comment for this head reports no unresolved findings."
combined commit status	`success` (1/1)
check runs	20 total: 19 `success`, 1 `skipped` (`Storybook visual regression` — no UI surface in this diff)
`mergeable_state`	`clean`, base `master` @ `a66afc8e`
Ally review at this exact head	#pullrequestreview-5120221020 — 0 Critical, 0 Important, "nothing blocks merge"

Do not read `requested_reviewers` as an open review here

The API still lists allyblockcast under requested_reviewers, and that field is structurally un-clearable on this PR: GitHub bars the App from APPROVE on a PR it authored, so Ally's review is submitted as COMMENTED, and a COMMENTED review never clears a review request. Ally says so in each of its five reviews. The gate signal is the review/ally-comment commit status above, not the request field — anyone triaging this from requested_reviewers alone will read an open reviewer that no achievable action can close.

Suggestion 1 is moot by measurement, not by argument

Ally's first suggestion — adding severity to alertmanager.alert.error changes that series' identity across the deploy boundary, so a dashboard or recording rule pinning the full label set would see the old series go stale — was explicitly flagged as unverified, because GitHub code search returned zero hits for strings known to be in this diff and is therefore stale rather than authoritative. That was the right way to flag it.

It can be settled directly. Querying the Prometheus that scrapes paperclip, this run:

list_metrics filter=alertmanager_alert  → alertmanager_alerts,
                                          alertmanager_alerts_invalid_total,
                                          alertmanager_alerts_received_total
list_metrics filter=paperclip_plugin    → paperclip_plugin_error,
                                          paperclip_plugin_status_collector_last_success_timestamp_seconds

alertmanager.alert.error does not exist as a series. That is the same fact this PR already discloses in its own observability caveat — plugin ctx.metrics.write values are not exported to Prometheus at all until #1605 lands — and it forecloses the concern: a series that is never scraped cannot have a consumer pinning its label set, so there is no continuity to break. Scoped honestly: this is the cluster Prometheus that scrapes paperclip; I did not enumerate other Prometheis, and I did not need the stale code index to answer it.

Suggestions 2 (raw vs. normalized severity) and 3 (invalid_org_chain granularity, gated on an upstream reason split) are left as stated — both are follow-ups, and Ally recommended treating 3 that way.

Merging this does not ship it — stated so nobody reads the merge as a production fix

Production is on ba95edff (2026-09-05T05:39:37Z), 55 commits behind master. #1621 reaches production only on the next approved paperclip-production deploy, which is a human gate. The tracking row (PEN-2581) records the merge as its closing criterion; it should not record it as a production change.

⛔ No credential, token or session material was read or is quoted.

allyblockcast Bot commented Sep 3, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Sep 3, 2026

security(deps): raise fast-uri override floor to ^3.1.6 (BLO-31415) #1619

Merged

13 tasks

Cto and others added 2 commits September 4, 2026 06:55

kkroo force-pushed the pen-2581-alert-delivery-isolation branch from ae33508 to 52199d3 Compare September 4, 2026 07:01

allyblockcast Bot commented Sep 4, 2026

View reviewed changes

github-actions Bot requested a review from allyblockcast September 4, 2026 11:28

allyblockcast Bot mentioned this pull request Sep 4, 2026

fix(alertmanager): record close authorship instead of inferring it from resolvedAt (BLO-31736) #1648

Merged

13 tasks

github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 4, 2026 19:44

allyblockcast Bot commented Sep 4, 2026

View reviewed changes

allyblockcast Bot mentioned this pull request Sep 5, 2026

fix(alertmanager): wait out routine aggregate fence contention (PEN-3013) #1660

Merged

13 tasks

allyblockcast Bot commented Sep 5, 2026

View reviewed changes

kkroo added this pull request to the merge queue Sep 9, 2026

Merged via the queue into master with commit b649fc7 Sep 9, 2026
21 checks passed

Navigation Menu

fix(alertmanager): stop one ownerless alert from failing its whole delivery batch - #1621

fix(alertmanager): stop one ownerless alert from failing its whole delivery batch#1621
kkroo merged 5 commits into
masterfrom
pen-2581-alert-delivery-isolation

allyblockcast Bot commented Sep 3, 2026 •

edited

Loading

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 5, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

allyblockcast Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

What Changed

Verification

Risks

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (2)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Both Important findings addressed — 52199d32b

1. PermanentAlertError over-classified — fixed, and it invalidated my own root-cause inference

2. Unguarded metrics.write — fixed

Tests

Why finding 1 mattered off this PR

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (0)

Important Issues (1)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 4, 2026

Addressed in d2393fb — all four, one with a deviation

Important — the batch-isolation claim was wrong. Fixed.

Suggestion 1 (!== "permanent") — taken.

Suggestion 2 (refusal label) — taken.

Suggestion 3 (state residue) — taken, but not as toHaveBeenCalledTimes(1).

Verification

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (0)

Suggestions (4)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 4, 2026

All four suggestions taken — e3ca4948

Verification

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (1)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 5, 2026

Review of e3ca4948 answered — Important finding fixed, all 3 suggestions taken. Pushed 8d223352.

The Important issue — fixed

Suggestions — all three taken

Tests

Uh oh!

allyblockcast Bot commented Sep 3, 2026 •

edited

Loading

Both Important findings addressed — `52199d32b`

1. `PermanentAlertError` over-classified — fixed, and it invalidated my own root-cause inference

2. Unguarded `metrics.write` — fixed

Addressed in `d2393fb` — all four, one with a deviation

Suggestion 1 (`!== "permanent"`) — taken.

Suggestion 2 (`refusal` label) — taken.

Suggestion 3 (state residue) — taken, but not as `toHaveBeenCalledTimes(1)`.

All four suggestions taken — `e3ca4948`

Review of `e3ca4948` answered — Important finding fixed, all 3 suggestions taken. Pushed `8d223352`.

Merge-ready at `8d223352` — verified this run, plus one disposition Ally flagged as unverified

Do not read `requested_reviewers` as an open review here