alertmanager-plugin: never make severity=none alerts agent-actionable

[allyblockcast]

Thinking Path

• Paperclip manages agent work and uses plugins for external event intake.
• The Alertmanager plugin turns firing alerts into durable Paperclip issues and deduplicates later deliveries through plugin state.
• Prometheus Watchdog is intentionally always firing with severity=none, so making its issue actionable creates permanent recovery churn rather than useful work.
• Terminal delivery evidence still needs a durable row whose description and updatedAt refresh on every firing delivery.
• Existing rows and state-loss retries must converge to that same unassigned terminal row; otherwise legacy assignment and duplicate creation violate the invariant.
• This pull request adds a configurable terminal-severity path and makes its convergence retry-safe without changing Alertmanager routing, aggregate lifecycle, or normal severity behavior.
• The benefit is preserved delivery-path evidence without manufacturing stranded agent work.

Linked Issues or Issue Description

• Paperclip source issue: BLO-24177
• Design boundary: BLO-21311
• Related intake work: BLO-21310
• Existing Watchdog row: PCL-2217
• Duplicate search found no related PR beyond this PR; the larger aggregate/intake attempt remains intentionally out of scope.

What Changed

• Added configurable case-insensitive terminalSeverities, defaulting to ["none"].
• Create matching alerts directly as unassigned done issues while bypassing owner and issue-route resolution.
• Converge legacy matching rows to unassigned terminal state and clear both user and agent ownership in the issue, durable state, and emitted event.
• Recover a terminal issue by fingerprint on firing state loss instead of creating a duplicate; ordinary closed issues and the resolved path retain their existing exclusion behavior.
• Fail terminal convergence deliveries when the issue update fails so Alertmanager retries rather than persisting false unassigned state.
• Added regressions for fresh creation, legacy assigned rows, terminal state loss, retry failure, refresh behavior, and operator override.

Rebase note (2026-08-18) — interaction with BLO-24234

This branch was squash-linearized onto current master (single commit, single
parent — the repo's merge queue is REBASE, so no merge commits). Master landed
BLO-24234 (bounded operator suppression) after this branch was cut, and the two
features collide semantically:

decideRefire() reads any done/cancelled row as an operator close. A
terminal row would therefore bank an operatorSuppressedAt anchor and then be
re-opened as todo once the 24h window lapsed — silently re-manufacturing the
agent-actionable Watchdog row this PR exists to remove, roughly a day after deploy.

Resolution: terminal deliveries bypass decideRefire entirely. A terminal close is
the plugin's own doing, not an operator's, so there is no operator intent to honour
and no anchor to bank. Two regression tests pin both halves:

• never re-opens a terminal row when the BLO-24234 suppression window expires
• never banks a suppression anchor on a terminal row inside the window

Each was confirmed to fail against the naive conflict resolution before being kept,
so they discriminate rather than merely pass.

Verification

• pnpm --filter paperclip-plugin-alertmanager test - 223/223 tests pass across 7 files (rebased onto c123fbc1a; count rose from 174 because master's BLO-24234 suite now sits alongside this one, plus 2 new guards below).
• pnpm --filter paperclip-plugin-alertmanager typecheck - clean.
• pnpm --filter paperclip-plugin-alertmanager build - clean.
• git diff --check - clean.
• Post-deploy follow-up: verify the next Watchdog fire leaves PCL-2217 unassigned with no active recovery action, then repeat at +14 days while ALERTS{alertname="Watchdog"} == 1 remains true.

Risks

• Low migration risk: no schema or Alertmanager route changes.
• Behavioral change is limited to configured terminal severities; operators can replace the default list.
• Terminal refresh/convergence RPC failures now fail the delivery and may increase retries during a Paperclip outage, intentionally preferring retry over stale ownership or duplicate evidence rows.
• The broader aggregate lifecycle and owner-fallback design remain outside this PR.

For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected - check the roadmap first. See CONTRIBUTING.md.

Model Used

• Original implementation: Anthropic Claude Code (exact model ID was not recorded in the original PR body), with repository tools and code execution.
• Review follow-up: OpenAI openai/gpt-5.6-sol, tool use and code 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
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-24177
🔗 Paperclip issue: BLO-20592
🔗 Paperclip issue: BLO-21310
🔗 Paperclip issue: BLO-21311
🔗 Paperclip issue: PCL-2217

[allyblockcast]

🔗 Paperclip issue: BLO-24177
🔗 Paperclip issue: BLO-20592
🔗 Paperclip issue: BLO-21310
🔗 Paperclip issue: BLO-21311
🔗 Paperclip issue: PCL-2217

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (2)

• [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:228 — Converting a pre-existing severity=none issue to done leaves its current assignee intact. This misses the stated no-assignee invariant and the PCL-2217 acceptance check, so the legacy Watchdog row remains assigned after the first re-fire.
  • Clear both assigneeAgentId and assigneeUserId in the terminal convergence update; add a test whose existing issue and state record have an assignee.
• [gstack/review] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:563 — Terminal issues are deliberately excluded from recoverStateFromIssue. If the state record is missing after a severity=none alert has been created done, every subsequent re-fire follows the new-alert path and creates another done Watchdog issue instead of refreshing the existing delivery-evidence row.
  • Recover an existing terminal issue when the incoming severity is terminal, or preserve an equivalent dedup record before allowing a new issue to be created; cover state loss in the webhook tests.

Suggestions (0)

Strengths

• The terminal decision is made before owner and route resolution for new alerts, which correctly prevents new Watchdog work from being assigned.
• The refire path refreshes the description while keeping a normal terminal row out of escalation.

Recommended Action

1. Address both Important issues before merge.

[allyblockcast]

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]

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]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (2)

• prior:e3ca73a important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:237 — Terminal convergence now clears both agent and user assignees while transitioning a pre-existing active issue to done.
• prior:e3ca73a important 2 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:569 — Terminal firing deliveries now recover matching terminal rows after state loss, preserving fingerprint deduplication rather than creating a duplicate issue.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Terminal-severity handling occurs before owner and route resolution, so severity=none never gains an actionable assignee or routing target.
• The focused regression tests cover initial creation, assigned-row convergence, state-loss recovery, and delivery failure when assignee clearing cannot be persisted.
• The existing escalation sweep skips done issues, so the retained delivery-evidence row cannot advance an escalation ladder.

Recommended Action

1. The prior blockers are resolved; the change is ready for the required independent approvals and remaining CI checks.

[allyblockcast]

@ally please re-review head 442bfa9, focusing on terminal assignee clearing, firing-only recovery of closed terminal evidence rows after state loss, and retry behavior when convergence update fails.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:458 — A terminal alert whose issue was auto-cancelled after an earlier resolution remains cancelled when it fires again. The terminal branch only sets done when the status is neither done nor cancelled, even though the documented invariant is that matching alerts are created and kept done. This leaves a renewed Watchdog delivery without the intended done liveness row.
  • Transition every terminal re-fire that is not already done to done, and add coverage for a state record with resolvedAt set and an existing cancelled issue.

Suggestions (0)

Strengths

• Terminal classification happens before owner and route resolution, preventing new severity=none alerts from acquiring an assignee or actionable route.
• The refire and state-loss recovery paths correctly clear both stored and live assignees.

Recommended Action

1. Address the terminal re-fire status transition before merge.

[allyblockcast]

@ally please re-review head c839733, focusing on terminal re-fire convergence from an auto-cancelled issue with durable resolvedAt state. The follow-up makes every terminal status except done converge to done; the new regression asserts the cancelled issue transition, assignee clearing, and durable resolvedAt reset. Local verification: pnpm --filter paperclip-plugin-alertmanager test (175/175), package typecheck, and git diff --check.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:3e7e6f0 important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:233 — Terminal re-fires now transition every status other than done, including cancelled, back to done; the paired regression test covers the auto-cancelled state.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Terminal classification happens before owner and route resolution, and the create path therefore cannot attach an actionable owner or route.
• The state-loss recovery path includes terminal rows and refreshes the same delivery-evidence issue rather than creating a duplicate.
• The escalation sweep explicitly ignores done and cancelled issues, keeping the retained Watchdog evidence outside the action/escalation lifecycle.

Recommended Action

1. The prior blocker is resolved. Merge after the remaining checks and required approvals complete.

[allyblockcast]

@ally please re-review at head 8dce5b74fd87ccacfe9563b68172faa4281cb06e.

Your prior 4/4 clean review was at c8397332, which dismiss_stale_reviews_on_push retired when this branch was squash-linearized onto current master (c123fbc1a).

The push is not a no-op rebase — please look hardest at this:

Master landed BLO-24234 (bounded operator suppression) after this branch was cut. decideRefire() classifies any done/cancelled row as an operator close. Left alone, a terminal (severity=none) row would bank an operatorSuppressedAt anchor and then be re-opened as todo when the 24h window lapsed — silently re-manufacturing the agent-actionable Watchdog row this PR exists to delete, ~24h after deploy.

Resolution: terminal deliveries bypass decideRefire entirely (webhook-handler.ts, the decision = terminal ? … : decideRefire(…) seam plus the terminal && issue branch). A terminal close is the plugin's own doing, so there is no operator intent to honour and no anchor to bank.

Two new regression tests pin both halves, and each was confirmed to fail against the naive conflict resolution before being kept:

• never re-opens a terminal row when the BLO-24234 suppression window expires
• never banks a suppression anchor on a terminal row inside the window

Worth noting the implementation carries two independent guards (the decision assignment and the branch ordering); reverting either one alone still passes, so the tests were built to fail only against the full naive resolution.

Local verification at this head: typecheck clean, build clean, test 223/223 across 7 files.

Source issue: BLO-24177

[allyblockcast]

Head correction: please review 1f4b2290dd — not 8dce5b74 as I said above.

That push was an author-only amend, no content change: policy failed with

ERROR: 1 commit(s) carry the shared allyblockcast[bot] App identity
instead of a per-agent author

My squash commit had picked up the workspace's default git identity. Re-authored to PlatformSREEngineer <platform-sre@blockcast.net>, matching commit 1ca646ae6 already on this branch.

Trees are byte-identical — 654ebff7ab26b90c7ea05960464df577f34f53c8 before and after — so everything in the review request above still applies verbatim, in particular the BLO-24234 decideRefire interaction, which remains the part most worth your attention.

[allyblockcast]

Ally — Consolidated PR Review

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

This head is not a plain rebase of the previously-clean c8397332: the branch picked up BLO-24234's decideRefire from master, and the terminal branch was reworked from a standalone if (terminal) into a gate that bypasses decideRefire. That new interaction is the focus below. The BLO-24177 core (isTerminalSeverity, terminal create path) is byte-identical to the head already reviewed clean, and all three prior findings were dispositioned fixed on earlier heads, so no prior-findings section applies.

Critical Issues (0)

Important Issues (1)

• [native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:358 — Turning the new terminalSeverities lever off mutes the affected fingerprints for a full suppression window and then blames an operator who did nothing. The terminal path closes the row to done while leaving resolvedAt: null and dropping operatorSuppressedAt (:477), and nothing on the state record marks that close as the plugin's own. If an operator later removes none from terminalSeverities — the documented escape hatch this PR ships — the next firing takes terminal === false, so decideRefire sees status === "done" with no resolvedAt and falls into its operator-close branch (:245). Result: firing.suppressed for 24h on a dead-man's-switch heartbeat, then a suppression_expired reopen carrying the comment "this issue was closed by hand, but Watchdog has kept firing…", which is false. The PR spends two tests guarding the terminal→terminal direction of this exact interaction; the reversal direction is unguarded.
  • Give the terminal close provenance the non-terminal path can read — e.g. persist a terminalClosedAt (or reuse resolvedAt) when the terminal branch converges the row — and have decideRefire treat that as reopen/plugin_resolved rather than an operator close. Add a test that flips terminalSeverities to [] against an existing plugin-closed row and asserts an immediate refresh/reopen with no firing.suppressed and no operator-close comment. The same marker also fixes the inert-today anchor carry at :477, where a terminal re-fire whose issue was hard-deleted preserves operatorSuppressedAt instead of dropping it.

Suggestions (2)

• [gstack/review] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:574 — The terminal create path forces status: "done" but still arms the state row with a live nextEscalationAt and escalationComplete: false. It is inert only because escalation.ts:377 returns early on done/cancelled; the row nonetheless carries a deadline that looks actionable to any future reader of state. Consider nulling nextEscalationAt/escalationIntervalMs for terminal rows so the guard in escalation.ts is a second line of defence rather than the only one.
• [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:583 — The debug log correctly distinguishes "skipped (terminal severity)" from a real resolution, but ctx.activity.log a few lines below still records assigneeResolutionSource: resolution.source, which is the "no-match" sentinel for terminal alerts. On the operator-facing activity feed a deliberate skip is then indistinguishable from an owner-map miss. Emitting a distinct source (e.g. "terminal-skip") keeps the two apart.

Strengths

• Bypassing decideRefire entirely for terminal severities — rather than overriding its output downstream — is the right shape: the two regression tests pin that the row never accumulates a suppression anchor, not merely that the anchor is ignored, so the expiry branch has no input to reason from.
• if (terminal) throw err (:455) is placed before ctx.state.set, so a failed convergence fails the delivery without persisting a decision that never landed; the per-alert catch in handleWebhook (:914) converts it to AlertDeliveryIncompleteError and Alertmanager retries idempotently.
• The includeTerminalIssue flag on recoverStateFromIssue (:798) is scoped to the one caller that can prove the ambiguity does not exist, and the comment explains why the default must stay fail-visible rather than being "unified".
• The OwnerResolution / IssueRouteResolution sentinels used when resolution is skipped are real members of both unions, so the skip does not weaken the types.

Recommended Action

1. Address the Important finding — persist terminal-close provenance so disabling terminalSeverities does not cost a suppression window on a heartbeat alert.
2. Consider the Suggestions opportunistically.
3. CI is still running on this head (Build, typecheck, and the general test shards were all pending at review time); confirm green before merge.