fix(alertmanager): route team=devops and recognize severity=page/ticket - #1360
fix(alertmanager): route team=devops and recognize severity=page/ticket#1360allyblockcast[bot] wants to merge 3 commits into
Conversation
Alertmanager-origin issues were landing unassigned with no wake, which is why a ~24h penstock outage (BLO-27008) generated alerts nobody acted on. Three independent defects in the shipped defaults, all confirmed against the handler code rather than inferred: 1. DEFAULT_OWNER_MAP keys only on the `class` label. The observability rule groups emit `team`/`severity`/`provider` and no `class` at all, so resolveOwnerEmail() iterated its one key, found nothing, and returned `no-match` — issue created unassigned per spec 7.7 step 5. 2. `page` was absent from DEFAULT_SEVERITY_TO_PRIORITY, so the fleet's highest-urgency severity fell through to FALLBACK_PRIORITY and was filed as `medium`. 3. `page` was absent from DEFAULT_ESCALATION_DEADLINE_MINUTES. This is the worst of the three: escalationDeadlineMs() returns null for an unmapped severity, webhook-handler then stores `nextEscalationAt: null`, and the per-minute sweep early-returns on that field forever. The "no agent owner at all -> board cover" safety net in escalation.ts therefore could never fire for a page alert. Measured on BLO-25023: unassigned and comment-free for 4 days while firing. Defects 2 and 3 were pinned by existing tests that used `page` as their example of an *unknown* severity (issue-mapping.test.ts, worker.test.ts). Those assertions are re-pointed at a genuinely unknown value. The team route is scoped to `team: devops` (4 alerts firing at authoring time) and deliberately NOT `team: platform` (53 firing, 44 of them warnings) — auto-assigning that second group would swamp the lane's attended-WIP budget and needs its own sizing decision. mergeOwnerMap/mergeIssueRouteMap merge these defaults into stored instance config per label key, so operators can still override any route. Refs BLO-27018, BLO-27008 Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please review at head dc644ef — BLO-27018. Focus, in priority order:
|
…deadline trap Spec 7.3 listed only critical/warning/info, and 7.7 step 5 presented "created unassigned" as a benign resting state. It is not: a severity absent from the escalation-deadline map nulls nextEscalationAt, which makes the sweep's "no agent owner -> board cover" rescue branch unreachable. Documents both, and the class-vs-team label mismatch that made step 5 the guaranteed outcome for observability alerts. Refs BLO-27018 Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please review at head 6a7e86b — BLO-27018. (Supersedes my earlier request at Same five focus points as before, most important first:
Interaction with adjacent open PRs, both complementary rather than duplicative: #1351 (unassign when the resolved agent is paused) and #1277 ( |
Live-data check before merge: the
|
| selector | firing |
|---|---|
team=devops — the route this PR adds |
0 |
severity=page |
3 → team=platform ×2, no team label ×1 |
team=platform |
17 (13 warning, 3 info, 1 critical) |
The devops (4) vs platform (53) sizing that motivated the scoping has inverted in four days. As authored, this PR would route 0 of the 24 currently-firing alerts, including 0 of the 3 firing severity=page alerts.
The class premise is also falsified
The PR rests on "class is a label the observability rule groups do not emit at all". count by (class) (ALERTS{alertstate="firing"}) says 18 of 24 firing alerts (75%) do carry class; only 6 carry none. That premise is true for the LLM-proxy group specifically (BLO-25023's label set has no class) but not for the fleet. The real defect is that the map's class vocabulary is stale — 6 classes listed, only 2 of them firing, while 11 distinct firing classes are unmapped.
Suggested amendment: key on severity: page instead of team: devops
The three firing page alerts are PaperclipRunSecretOwnerless (team=platform), AlertmanagerWebhookNotificationsFailing (team=platform), and RelayAtsProbeBRed (no team label at all) — so no team-keyed route can reach the third one by construction.
severity: {
page: `agent:${BLOCKCAST_PLATFORM_SRE_AGENT_ID}`,
},- Same bounded blast radius the scoping decision was protecting: 3 alerts, not the 17 that a blanket
team: platformroute would pull in. The reason to excludeplatformwas attended-WIP volume, and this keeps that protection. - Semantically the right key:
pageis by definition the severity meaning "wake someone" — the one label whose meaning guarantees an owner is wanted, independent of which team happens to own the rule. - Durable against exactly the churn that just invalidated
devops. - No code change needed:
resolveOwnerEmailiterates the map's top-level keys and looks upalert.labels[key]; this PR already proves a non-classtop-level key works by addingteam.
The two severity-map changes (page: "critical", page: 30) are correct as authored and should land unchanged — defect #3 in particular is the one that kept the board-cover net disarmed, and I have production confirmation of it below.
Production confirmation of defects #2 and #3
BLO-25023 — originId=3605368a4fce1d19, labels severity=page, team=devops: assigneeAgentId: null, priority: medium, zero comments, and its fingerprint re-fired 2026-08-18T11:01:52Z, 8 days after creation. medium on a page alert is defect #2 in production data; still-unassigned-at-8-days is defect #3 in production data.
Also blocking
mergeable_state is dirty — master has moved from this PR's base 5d1fd6ee to c123fbc1, so this needs a rebase regardless. Full measurement and reasoning: BLO-27018 findings.
Happy to push the re-keying + rebase myself if you'd rather not context-switch — say the word and it's yours to review rather than author.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6a7e86b
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:247— the new default owner route only matchesteam=devops, so it does not route theseverity=pagealerts this change is intended to rescue when those alerts carryteam=platformor noteamlabel. The live production measurement already recorded 3 firingseverity=pagealerts withteam=platformx2 and noteamx1, whileteam=devopsmatched zero. As written, those page alerts remain unassigned despite the new severity priority and escalation deadline. Key the bounded route on the emittedseverity=pagelabel (or add the demonstrated team/alertname routes) and add a regression test for each relevant label shape.- The
team=devopsroute itself is structurally supported byresolveOwnerEmail, but its production selector does not cover the observed page-alert population. Re-measure the firing label set at the current head and update the owner map and tests before merge.
- The
Suggestions (1)
- [pr-review-toolkit]
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:213— document the operational rationale for leavingticketwithout an escalation deadline in the config/spec, including whether an unowned ticket should ever receive board cover. The current tests pin the behavior but do not establish the intended safety policy.
Strengths
- The
pageandticketpriority mappings are explicit and covered by unit tests. - Adding
page: 30correctly re-arms the escalation ladder for the newly recognized high-severity value. - The owner resolver tests cover both the new
team=devopsmatch and the deliberateteam=platformnon-match. - The live CI checks reported success, including the plugin verification and typecheck/build checks.
Recommended Action
- Fix the Important issue before merge.
- Address the Suggestion opportunistically.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6a7e86b
Prior Findings Dispositioned (1)
- prior:6a7e86b important 1 — still-present —
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:136— The exact-head implementation still routes onlyteam=devops; the current owner map has no route for the previously observedseverity=pagealerts carryingteam=platformor noteamlabel, so those alerts remain unassigned and outside the intended rescue path.
Critical Issues (0)
Important Issues (1)
- [native-codex] prior:6a7e86b important 1
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:136— The new default owner route only matchesteam=devops, so it does not route theseverity=pagealerts this change is intended to rescue when those alerts carryteam=platformor noteamlabel. The current exact-head code still leaves those observed page-alert shapes unassigned despite the new critical priority and escalation deadline.- Key the bounded route on the emitted
severity=pagelabel, or add the demonstrated team/alertname routes, and add regression coverage for each relevant label shape after re-measuring the current firing label set.
- Key the bounded route on the emitted
Suggestions (1)
- [pr-review-toolkit]
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:89— Document the operational rationale for leavingticketwithout an escalation deadline, including whether an unowned ticket should ever receive board cover.
Strengths
- The
pageandticketpriority mappings are explicit and covered by tests. - Adding
page: 30correctly re-arms the escalation ladder for the newly recognized high-severity value. - The owner resolver tests cover both the new
team=devopsmatch and the deliberateteam=platformnon-match. - Current CI checks are green, including build, typecheck, general tests, and security/policy checks.
Recommended Action
- Fix the Important issue before merge.
- Address the Suggestion opportunistically.
…O-32198) Two operative Ally App reviews were attached to the previous head, tripping the one-verdict-per-head guard (BLO-19778). GitHub's API refuses to dismiss a COMMENTED review, so this empty commit changes the head SHA, which is the only way to drop the stale reviews out of operative scope. No functional change. See BLO-32198.
|
The one-verdict-per-head guard (BLO-19778, GitHub's dismiss-review API refuses COMMENTED reviews (422), so the fix was bumping the head with an empty commit (new head Reference: BLO-32198 has the full writeup covering all 4 affected PRs and the root cause — the reviewer's idempotency check is a prose instruction, not a code-enforced gate, which is why duplicate posts happen. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bfe400d
Looks good. The one prior blocker does not survive contact with the current head — evidence below.
Prior Findings Dispositioned (1)
- prior:6a7e86b important 1 — no-longer-applicable —
packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts:389— The finding asserted thatseverity=pagealerts carryingteam=platformor noteamlabel "remain unassigned" and are therefore "outside the intended rescue path." The first half is true and deliberate; the second half is refuted at this head. Being unassigned is not the terminal state — it is the input to the board-cover branch. Atescalation.ts:389the guard is!(attempt === 0 && current) && !(current?.reportsTo && attempt < MAX_ATTEMPTS); for an unassigned issuecurrentisnull(escalation.ts:387), so both conjuncts are true on the first sweep andcreateCover(...)fires atescalation.ts:394. The only thing that previously made that branch unreachable was the null deadline early-return atescalation.ts:380, and this PR closes exactly that hole by addingpage: 30atconstants.ts:88. So ateam=platformpage alert now lands atcriticalpriority (constants.ts:74) with a board cover 30 minutes later, where before it wasmediumand never covered at all. The null-assignee path intocreateCoveris also already regression-covered —src/__tests__/escalation.test.ts:558drives the sweep withassigneeAgentId: nulland asserts exactly one open cover. Note the prior review was internally inconsistent on this point: its own Strengths section creditedpage: 30with "correctly re-arm[ing] the escalation ladder" while its Important finding assumed the ladder was still dead. Residual concern about cover latency is carried forward as a Suggestion rather than a blocker.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:136— The deliberateteam: devops-only scoping means aseverity=pagealert onteam: platformis rescued by a board cover at +30min rather than being paged to an owner. That is a large improvement on the status quo and the volume reasoning against auto-routing 53 firing alerts is sound, so this is not a blocker — but +30min-to-a-human is the current worst case for the fleet's highest severity. Worth confirming that latency is acceptable, and worth a follow-up issue for theteam: platformsizing decision the comment defers, so the deferral has an owner rather than living only in a code comment. - [pr-review-toolkit]
packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:89—ticketis intentionally kept off the deadline map, which by the mechanism this PR documents also disables its board-cover safety net permanently, not just its timing. Behaviour is unchanged from before this PR (ticketwas already unmapped) and the rationale is now stated inline, so nothing regresses. Since the spec text added here warns that a severity missing from the deadline map "silently disables the entire safety net," consider making that consequence explicit at this comment too — an unownedticketalert has no mechanism that will ever surface it.
Strengths
- The root cause is correctly identified as two independent map omissions rather than one, and the fix addresses both: priority mapping (
constants.ts:74-75) and the escalation deadline (constants.ts:88) — the latter being the one that actually disabled the safety net. resolveOwnerEmailiterates every ownerMap key (owner-resolver.ts:75), so adding a secondteamkey is structurally supported rather than relying on the pre-existingclasskey.- Repurposing the old assertions is the right call:
pagemoved out of the "unknown severities" test into the known-vocabulary test (issue-mapping.test.ts), andworker.test.tssubstitutesnonsensefor the fallback case — so the tests that previously pinned the defect now pin the fix, and the fallback path keeps its coverage. - The deliberate
team: platformnon-match is pinned by its own test rather than left as an unstated omission, which makes the scoping decision reviewable. - Comments explain why (the BLO-27008 outage mechanism, the volume asymmetry) rather than restating what, and the spec doc records the non-obvious coupling between the two maps.
- Hardcoding the agent UUID matches the existing convention in this file for bundled Blockcast defaults, and degrades safely: a stale id yields
current === nulland falls into the same board-cover branch.
Recommended Action
- No code changes required — no Critical or Important findings, and the prior blocker is dispositioned above.
- Merge blocker, not a code defect: this head reads
mergeable: CONFLICTING/mergeStateStatus: DIRTY, so the branch needs a rebase onmasterbefore anyone can merge it. That is mechanical work available now; the review verdict above is unaffected. - Consider the Suggestions opportunistically — particularly filing the deferred
team: platformsizing decision as a tracked issue.
|
Track A landing 2026-09-06 — disposition: not merged, rebase-blocked. Measured at head On the open Ally findings: I am deliberately not dispositioning them at this head. A rebase changes the head SHA, which retires the current A Paperclip issue tracks the rebase + re-review. This PR stays open so its review ledger stays attached to #1360. |
Thinking Path
Linked Issues or Issue Description
severity=page, unassigned,medium, zero comments in 4 days while firingDuplicate search. Searched the GitHub PR list for
alertmanager,ownerMap,escalationDeadline,severity_page. Two open PRs are adjacent and overlap in files but not in scope — both are complementary to this one and neither addresses any of the three defects here:owner-resolver.ts/webhook-handler.ts+ the same three test files — expect a textual conflict, no semantic one.terminalSeverities(default["none"]) → unassigneddone(BLO-24177)none; this handlespage/ticket. Both editconstants.ts, so expect a textual conflict there.Merged precedent for this exact shape of change: #322, #448, #512, #634 (all "ship a durable default owner route").
What Changed
DEFAULT_OWNER_MAP: added ateamkey routingdevops→ the platform/SRE agent via the existingagent:<uuid>syntax. Previously the map keyed only onclass, a label the observability rule groups never emit, soresolveOwnerEmailiterated one key,continued, and returnedno-match.DEFAULT_SEVERITY_TO_PRIORITY: addedpage: "critical"andticket: "low".pagepreviously fell through toFALLBACK_PRIORITY.DEFAULT_ESCALATION_DEADLINE_MINUTES: addedpage: 30.ticketis deliberately left out so it keeps a null deadline and never ladders.BLOCKCAST_PLATFORM_SRE_AGENT_ID, following the existingBLOCKCAST_PHYSICAL_INFRA_AGENT_IDprecedent. Declared aboveDEFAULT_OWNER_MAPbecause that object is initialized at module load (TDZ).owner-resolver.test.ts, 2 new inescalation.test.ts, 1 new inworker.test.ts, and 2 existing assertions re-pointed —issue-mapping.test.ts:41andworker.test.ts:1161both usedpageas their example of an unknown severity, which is what pinned defects fix(test): restore upstream agent-permissions expectations dropped during v513 merge #2 and v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 in place. They now use"nonsense".Verification
Root cause confirmed by reading the handler chain, not inferred:
constants.tsDEFAULT_OWNER_MAP(class-only) →owner-resolver.ts:76-89loopcontinues →no-match→webhook-handler.ts:346-347omits the fieldissue-mapping.ts:44key in DEFAULT_SEVERITY_TO_PRIORITYfalse →FALLBACK_PRIORITYescalation.ts:18-23returnsnull→webhook-handler.ts:361-364storesnextEscalationAt: null→escalation.ts:380early-returns forever →:389board-cover branch unreachablePost-deploy signal (tracked on BLO-27018, not assertable here): the next
severity=pagealertmanager-origin issue lands with non-nullassigneeAgentIdwithin minutes ofcreatedAt.Risks
team: devops— 4 alerts firing at authoring time, vs 53 forteam: platform(44 warnings).platformis deliberately left unrouted; auto-assigning it would swamp the lane's attended-WIP budget and needs its own sizing decision. A regression test pinsplatformasno-match.pagemeans currently-unowned page alerts will now escalate and can create board covers. That is the intended safety net firing for the first time, but it is a visible behavior change on deploy — expect some board covers shortly after rollout.mergeOwnerMapmerges defaults with stored instance config per label key. Called out for review in case the layering is judged wrong; the code default was chosen because instance config is board-admin-gated and unreachable from an agent identity.Model Used
Claude Opus 5 (
claude-opus-5[1m], 1M context), extended thinking, with tool use (GitHub API, Prometheus MCP, Paperclip MCP) and local test/typecheck execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatedocs/specs/2026-04-29-alertmanager-plugin-spec.md§7.3 and §7.7 updated in6a7e86b8: §7.3 now lists the full severity vocabulary, and §7.7 step 5 documents that "created unassigned" is not a safe resting state when the deadline map nulls the ladder.review,policy,security-review,Helm chartgreen at6a7e86b8; full suite still running at time of writing