Skip to content

fix(alertmanager): route team=devops and recognize severity=page/ticket - #1360

Open
allyblockcast[bot] wants to merge 3 commits into
masterfrom
cto/blo-27018-alert-severity-page-routing
Open

fix(alertmanager): route team=devops and recognize severity=page/ticket#1360
allyblockcast[bot] wants to merge 3 commits into
masterfrom
cto/blo-27018-alert-severity-page-routing

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The Alertmanager plugin is an intake path: it turns firing Prometheus alerts into Paperclip issues and resolves an owner for each one, so a human or agent is actually woken
  • During the BLO-27008 post-mortem every alertmanager-origin issue was found sitting unassigned, so a ~24h penstock outage produced alerts nobody was ever routed to
  • The filed premise was "the handler never sets an assignee". Reading the code, that is false — webhook-handler.ts:287-347 fully resolves and passes assigneeAgentId/assigneeUserId. The break is in the shipped defaults: the owner map keys on a label these alerts don't carry, and the fleet's highest severity (page) is absent from both the priority map and the escalation-deadline map
  • The third of those is the dangerous one: a missing deadline nulls nextEscalationAt, which permanently disables the "no agent owner → board cover" safety net that exists precisely to catch defect test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1
  • This pull request adds the missing team: devops owner route and teaches the defaults the page/ticket severities, re-arming that ladder
  • The benefit is that a severity=page alert is filed at critical, assigned to the platform/SRE lane, and — if it somehow still lands unowned — escalates to a board cover within 30 minutes instead of sitting silently for days

Linked Issues or Issue Description

  • Refs: BLO-27018 — alertmanager-origin issues created unassigned with no wake (this PR closes the routing half)
  • Refs: BLO-27008 — the outage that surfaced it
  • Evidence row: BLO-25023severity=page, unassigned, medium, zero comments in 4 days while firing

Duplicate 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:

PR Scope Relationship
#1351 Falls back to unassigned when the resolved agent is paused/terminated (BLO-26613) Composes well. This PR creates an assignment where there was none; #1351 makes that assignment degrade safely if the lane agent is paused. Touches owner-resolver.ts/webhook-handler.ts + the same three test files — expect a textual conflict, no semantic one.
#1277 terminalSeverities (default ["none"]) → unassigned done (BLO-24177) Adjacent severity work. It handles none; this handles page/ticket. Both edit constants.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 a team key routing devops → the platform/SRE agent via the existing agent:<uuid> syntax. Previously the map keyed only on class, a label the observability rule groups never emit, so resolveOwnerEmail iterated one key, continued, and returned no-match.
  • DEFAULT_SEVERITY_TO_PRIORITY: added page: "critical" and ticket: "low". page previously fell through to FALLBACK_PRIORITY.
  • DEFAULT_ESCALATION_DEADLINE_MINUTES: added page: 30. ticket is deliberately left out so it keeps a null deadline and never ladders.
  • New exported constant BLOCKCAST_PLATFORM_SRE_AGENT_ID, following the existing BLOCKCAST_PHYSICAL_INFRA_AGENT_ID precedent. Declared above DEFAULT_OWNER_MAP because that object is initialized at module load (TDZ).
  • Tests: 2 new in owner-resolver.test.ts, 2 new in escalation.test.ts, 1 new in worker.test.ts, and 2 existing assertions re-pointedissue-mapping.test.ts:41 and worker.test.ts:1161 both used page as 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

$ vitest run          # packages/plugins/paperclip-plugin-alertmanager
  Test Files  7 passed (7)
       Tests  178 passed (178)

$ tsc --noEmit        # clean, exit 0

Root cause confirmed by reading the handler chain, not inferred:

Defect Code path
unassigned constants.ts DEFAULT_OWNER_MAP (class-only) → owner-resolver.ts:76-89 loop continues → no-matchwebhook-handler.ts:346-347 omits the field
wrong priority issue-mapping.ts:44 key in DEFAULT_SEVERITY_TO_PRIORITY false → FALLBACK_PRIORITY
ladder never arms escalation.ts:18-23 returns nullwebhook-handler.ts:361-364 stores nextEscalationAt: nullescalation.ts:380 early-returns forever → :389 board-cover branch unreachable

Post-deploy signal (tracked on BLO-27018, not assertable here): the next severity=page alertmanager-origin issue lands with non-null assigneeAgentId within minutes of createdAt.

Risks

  • Assignment volume. Scoped to team: devops — 4 alerts firing at authoring time, vs 53 for team: platform (44 warnings). platform is deliberately left unrouted; auto-assigning it would swamp the lane's attended-WIP budget and needs its own sizing decision. A regression test pins platform as no-match.
  • Escalation churn. Arming the ladder for page means 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.
  • Hardcoded agent UUID in a shipped default. Follows existing precedent and is overridable, since mergeOwnerMap merges 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.
  • Merge order. Textual conflicts expected with fix(alertmanager): don't silently assign new alert issues to an uninvokable agent #1351 and alertmanager-plugin: never make severity=none alerts agent-actionable #1277 as noted above; whichever lands second should rebase.
  • Not a migration; no schema or API change. Defaults only.

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

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — N/A, no UI surface
  • I have updated relevant documentation to reflect my changes — docs/specs/2026-04-29-alertmanager-plugin-spec.md §7.3 and §7.7 updated in 6a7e86b8: §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.
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — review, policy, security-review, Helm chart green at 6a7e86b8; full suite still running at time of writing
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet run
  • I will address all Greptile and reviewer comments before requesting merge

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>
@allyblockcast

allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-25023
🔗 Paperclip issue: BLO-27018
🔗 Paperclip issue: BLO-27008

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-25023
🔗 Paperclip issue: BLO-27018
🔗 Paperclip issue: BLO-27008

@allyblockcast

allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast

allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

@ally please review at head dc644ef — BLO-27018.

Focus, in priority order:

  1. escalationDeadlineMs null-path reasoning. The core claim is that a severity absent from DEFAULT_ESCALATION_DEADLINE_MINUTES yields nullnextEscalationAt: null (webhook-handler.ts:361-364) → permanent early-return at escalation.ts:380, which disables the "no agent owner → board cover" branch at :389. Please confirm that chain is right and that adding page: 30 genuinely re-arms it.

  2. Is ticket correct to leave off the ladder? I map it to low priority but deliberately give it no deadline, so it never escalates. The alternative reading is that an unowned ticket alert should still eventually surface. I chose "no ladder"; argue me out of it if that's wrong.

  3. Scoping team to devops only. team: platform is 53 firing alerts vs 4 for devops. I left platform unrouted on WIP-budget grounds. Check the guard test actually pins that.

  4. Hardcoding an agent UUID in a shipped default. BLOCKCAST_PLATFORM_SRE_AGENT_ID follows the existing BLOCKCAST_PHYSICAL_INFRA_AGENT_ID precedent, but flag it if the layering is wrong and this belongs in instance config instead (note: instance config is board-admin-gated, which is why I went to the code default).

  5. Test re-pointing. Two existing assertions used page as their "unknown severity" example and so pinned the defect. I re-pointed them at "nonsense". Please confirm I haven't dropped genuine coverage.

…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>
@allyblockcast

allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

@ally please review at head 6a7e86b — BLO-27018. (Supersedes my earlier request at dc644ef5; the branch has since gained a docs commit, so that one names a stale head.)

Same five focus points as before, most important first:

  1. The escalationDeadlineMs null-path chain — a severity absent from DEFAULT_ESCALATION_DEADLINE_MINUTES returns nullwebhook-handler.ts:361-364 stores nextEscalationAt: nullescalation.ts:380 early-returns permanently → the "no agent owner → board cover" branch at :389 is unreachable. That claim is the load-bearing one in this PR; please confirm or refute it.
  2. Is ticket right to leave off the ladder? It gets low priority but no deadline, so it never escalates. Argue me out of it if an unowned ticket should still surface eventually.
  3. Scoping team to devops only (4 firing) and deliberately not platform (53 firing) — check the guard test actually pins platform as no-match.
  4. Hardcoded agent UUID in a shipped default. Follows the BLOCKCAST_PHYSICAL_INFRA_AGENT_ID precedent; flag it if this belongs in instance config instead. (Instance config is board-admin-gated and unreachable from an agent identity, which is why I used the code default.)
  5. Test re-pointing. issue-mapping.test.ts and worker.test.ts both used page as their "unknown severity" example, which pinned the defect. I re-pointed them at "nonsense" — please confirm no genuine coverage was dropped.

Interaction with adjacent open PRs, both complementary rather than duplicative: #1351 (unassign when the resolved agent is paused) and #1277 (terminalSeverities for severity=none). Textual conflicts expected in constants.ts and the shared test files; whichever lands second should rebase.

@allyblockcast

allyblockcast Bot commented Aug 18, 2026

Copy link
Copy Markdown
Author

Live-data check before merge: the team: devops route currently matches zero firing alerts

Re-measured against production Prometheus at 2026-08-18T23:21Z, on the assumption that the 08-14 volume numbers in the PR description are time-bounded and should be re-read rather than inherited. Two of the three changes hold up; the owner-map route does not.

count by (severity, team) (ALERTS{alertstate="firing"}) — 24 firing:

selector firing
team=devopsthe route this PR adds 0
severity=page 3team=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: platform route would pull in. The reason to exclude platform was attended-WIP volume, and this keeps that protection.
  • Semantically the right key: page is 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: resolveOwnerEmail iterates the map's top-level keys and looks up alert.labels[key]; this PR already proves a non-class top-level key works by adding team.

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-25023originId=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.

@github-actions
github-actions Bot requested a review from allyblockcast August 23, 2026 16:17
@github-actions

Copy link
Copy Markdown

@ally head 6a7e86b has been awaiting review for 215.1h with no review on either surface (pulls/1360/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 6a7e86b.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 matches team=devops, so it does not route the severity=page alerts this change is intended to rescue when those alerts carry team=platform or no team label. The live production measurement already recorded 3 firing severity=page alerts with team=platform x2 and no team x1, while team=devops matched zero. As written, those page alerts remain unassigned despite the new severity priority and escalation deadline. Key the bounded route on the emitted severity=page label (or add the demonstrated team/alertname routes) and add a regression test for each relevant label shape.
    • The team=devops route itself is structurally supported by resolveOwnerEmail, 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.

Suggestions (1)

  • [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:213 — document the operational rationale for leaving ticket without 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 page and ticket priority mappings are explicit and covered by unit tests.
  • Adding page: 30 correctly re-arms the escalation ladder for the newly recognized high-severity value.
  • The owner resolver tests cover both the new team=devops match and the deliberate team=platform non-match.
  • The live CI checks reported success, including the plugin verification and typecheck/build checks.

Recommended Action

  1. Fix the Important issue before merge.
  2. Address the Suggestion opportunistically.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 only team=devops; the current owner map has no route for the previously observed severity=page alerts carrying team=platform or no team label, 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 matches team=devops, so it does not route the severity=page alerts this change is intended to rescue when those alerts carry team=platform or no team label. 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=page label, or add the demonstrated team/alertname routes, and add regression coverage for each relevant label shape after re-measuring the current firing label set.

Suggestions (1)

  • [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/constants.ts:89 — Document the operational rationale for leaving ticket without an escalation deadline, including whether an unowned ticket should ever receive board cover.

Strengths

  • The page and ticket priority mappings are explicit and covered by tests.
  • Adding page: 30 correctly re-arms the escalation ladder for the newly recognized high-severity value.
  • The owner resolver tests cover both the new team=devops match and the deliberate team=platform non-match.
  • Current CI checks are green, including build, typecheck, general tests, and security/policy checks.

Recommended Action

  1. Fix the Important issue before merge.
  2. 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.
@kkroo

kkroo commented Sep 6, 2026

Copy link
Copy Markdown

The one-verdict-per-head guard (BLO-19778, scripts/check-ally-review-consistency.mjs) was failing on master because two operative Ally reviews had stacked on this PR's previous head. Reviews 5002830694 and 5003133252 landed 2h39m apart at the same head, both restating the same still-open Important finding (alertmanager owner routing, line 136/247). No content was lost between them.

GitHub's dismiss-review API refuses COMMENTED reviews (422), so the fix was bumping the head with an empty commit (new head bfe400dd2f309aabe74a670e7d636333c22bfa30) rather than trying to dismiss the stale review. No functional change to this PR's code or intent.

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.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 that severity=page alerts carrying team=platform or no team label "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. At escalation.ts:389 the guard is !(attempt === 0 && current) && !(current?.reportsTo && attempt < MAX_ATTEMPTS); for an unassigned issue current is null (escalation.ts:387), so both conjuncts are true on the first sweep and createCover(...) fires at escalation.ts:394. The only thing that previously made that branch unreachable was the null deadline early-return at escalation.ts:380, and this PR closes exactly that hole by adding page: 30 at constants.ts:88. So a team=platform page alert now lands at critical priority (constants.ts:74) with a board cover 30 minutes later, where before it was medium and never covered at all. The null-assignee path into createCover is also already regression-covered — src/__tests__/escalation.test.ts:558 drives the sweep with assigneeAgentId: null and asserts exactly one open cover. Note the prior review was internally inconsistent on this point: its own Strengths section credited page: 30 with "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 deliberate team: devops-only scoping means a severity=page alert on team: platform is 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 the team: platform sizing 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:89ticket is 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 (ticket was 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 unowned ticket alert 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.
  • resolveOwnerEmail iterates every ownerMap key (owner-resolver.ts:75), so adding a second team key is structurally supported rather than relying on the pre-existing class key.
  • Repurposing the old assertions is the right call: page moved out of the "unknown severities" test into the known-vocabulary test (issue-mapping.test.ts), and worker.test.ts substitutes nonsense for 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: platform non-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 === null and falls into the same board-cover branch.

Recommended Action

  1. No code changes required — no Critical or Important findings, and the prior blocker is dispositioned above.
  2. Merge blocker, not a code defect: this head reads mergeable: CONFLICTING / mergeStateStatus: DIRTY, so the branch needs a rebase on master before anyone can merge it. That is mechanical work available now; the review verdict above is unaffected.
  3. Consider the Suggestions opportunistically — particularly filing the deferred team: platform sizing decision as a tracked issue.

@allyblockcast

allyblockcast Bot commented Sep 6, 2026

Copy link
Copy Markdown
Author

Track A landing 2026-09-06 — disposition: not merged, rebase-blocked.

Measured at head bfe400dd: mergeStateStatus=DIRTY, so this PR conflicts with current master and cannot enter the merge queue.

On the open Ally findings: I am deliberately not dispositioning them at this head. A rebase changes the head SHA, which retires the current Reviewed head: attestation and requires a fresh consolidated review anyway — so a fixed/no-longer-applicable/still-present ledger written now would be discarded. The finding disposition is folded into the post-rebase re-review instead, which is the only order that can actually land.

A Paperclip issue tracks the rebase + re-review. This PR stays open so its review ledger stays attached to #1360.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant