fix(alertmanager): record close authorship instead of inferring it from resolvedAt (BLO-31736) - #1648
Conversation
…om resolvedAt `decideRefire` discriminated "the plugin closed this issue" from "a human or an agent closed it" using `state.resolvedAt`. That field records only that the alert last cleared, not who closed the row, and the two diverge on exactly the case the resolve path's terminal guard exists to protect. An agent closes an alert issue `done`. The alert resolves: the guard correctly declines to overwrite the close, and the state write sets `resolvedAt` anyway, outside that guard. The next re-fire reads terminal + `resolvedAt` as "plugin_resolved" and patches the row back to `todo`; the resolve after it now finds a non-terminal row and cancels it. A deliberate `done` becomes a plugin-authored `cancelled` — once per fire/clear cycle, indefinitely, and it looks like a normal auto-close on every triage surface. Observed on BLO-10130 and BLO-10129 (`ClusterAdminDrift`, fires ~06:00Z and clears ~06:06Z daily). The same inference made BLO-24234's operator suppression dead code for any alert that had ever resolved — i.e. every flapping alert, precisely the ones operators close by hand. Fix: persist `pluginClosedAt`, written only on the branch where the plugin's own `cancelled` patch actually landed, and key the re-fire decision on that. - Cleared by any firing delivery that observed the issue's status, which is what makes suppression reachable for a previously-resolved alert. - NOT cleared by `issue_missing` or a failed RPC, mirroring the existing suppression-anchor rule: those learn nothing, and clearing there would let one transient `issues.get` failure mute a live recurring alert. - NOT cleared when the resolve path declines to cancel, so a redelivered `resolved` notification cannot erase our own close and mute the next real recurrence. - `done` is never a close of ours (the plugin's only status writes are `todo` and `cancelled`), so the reported defect is fixed on contact rather than one migration cycle later. - Legacy rows with no `pluginClosedAt` fall back to `resolvedAt`. Deliberate and asymmetric: reading our close as an operator's would mute a live alert for a whole window, while the reverse costs one unwanted re-open that the next firing state-write corrects. Silence is the worse failure. Tests: the existing re-fire tests were all single-shot against a hand-written state row, which is why this shipped — the first cycle looks correct. Adds a multi-cycle suite that carries both the state row and the issue status forward between deliveries, with `issues.update` mutating the live issue so the second stage observes the real cascade instead of assuming it. 6 new assertions fail on the parent commit. Refs: BLO-31736, BLO-24234, BLO-12033 Co-Authored-By: Paperclip <noreply@paperclip.ing>
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 |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: f54a179
The core thesis is right and well argued: resolvedAt records when the alert cleared, not who closed the issue, and keying the re-fire decision on it is the defect. closedByPlugin is the correct shape, the done-is-never-ours precedence rule is sound, and the asymmetry argument for the legacy fallback (silence is worse than one spurious re-open) is the right call. The multi-cycle test harness is a genuine improvement over the single-shot tests that let this ship.
One finding blocks: the new authorship record is written per-fingerprint, but the close it records is per-aggregate. That gap re-introduces the muting failure this PR exists to remove, for a whole class of alerts.
Critical Issues (1)
-
[code / gstack-review]
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:2052— In a multi-member aggregate, only the fingerprint whose resolve delivery actually lands the cancel getspluginClosedAtstamped. Every sibling row keepspluginClosedAt: nullwhile the shared issue is plugin-cancelled, so the sibling's next re-fire is misread as an operator close and suppressed. This is a regression against currentmaster.Concretely, for an aggregate with member fingerprints A and B (both firing, both rows
pluginClosedAt: nullfrom:1476/:1733):- A resolves first → the aggregate resolution returns
has-unresolved-siblings(:741) →shouldCancelis false (:1946) →pluginCancelLandedstays false → A's row is written withresolvedAtpopulated (:2042, unconditional) butpluginClosedAtstillnull(:2052spread contributes nothing). - B resolves →
last-member-resolved→ the cancel lands (:1983) → only B's row getspluginClosedAt. The shared issue is nowcancelled, authored by the plugin. - A re-fires.
handleFiringreads the shared issue (A's row points at it —paperclipIssueId: aggregateResolution.issueId,:2041) and callsdecideRefirewith A's row.closedByPlugin(:1017) sees statuscancelledandpluginClosedAt === null— an explicit "not ours", so not even the legacyresolvedAtfallback rescues it — and returnsfalse. - Result:
kind: "suppressed", muted for the fulloperatorSuppressionHours, with:1381logging "re-fired against operator-closed issue" about a close the plugin itself authored.
Before this PR, A's row had
resolvedAtpopulated (written unconditionally on its own resolve delivery), so step 4 wasreopen/plugin_resolved. That branch is also the only path to the rebind-to-live-winner logic at:1266-1280, which is documented as existing precisely for "a different firing in this aggregate" — so that recovery becomes unreachable for non-last members too.This is deterministic, not a race: it applies to every aggregate with two or more member fingerprints, on every fire/clear cycle. It is the muting direction the PR's own risk section calls "the one to worry about", and the three stated guards do not cover it — the value is a positive
null, notundefined.- Record authorship at the level the close happens. Either resolve
closedByPluginagainst the aggregate's close record rather than the per-fingerprint row, or have alast-member-resolvedcancel stamppluginClosedAton all member rows for that aggregate (the members are already enumerable via the aggregate members table,:704-758). A narrower alternative: when the issue is terminal and the row'saggregateKeyis populated, consult the aggregate member/close record before falling through to the operator-suppression branch.
- A resolves first → the aggregate resolution returns
Important Issues (2)
-
[comments / error-handling]
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1445-1458— The comment states thatissue_missingand a failed RPC "must leave it alone", because clearing "would let one transientissues.getfailure convert our own close into an apparent operator close and mute a live recurring alert for a whole suppression window". For legacy rows that guarantee does not hold, and it is defeated by a sibling line in the same object literal:resolvedAt: nullis written unconditionally at:1475, whilepluginClosureUpdateis{}. So a legacy row (pluginClosedAt: undefined,resolvedAtpopulated by the plugin's own close) that hits one failed-RPC re-fire ends up withresolvedAt: nullandpluginClosedAtstillundefined— and the legacy fallback inclosedByPluginreads exactlyBoolean(existing.resolvedAt), so the next re-fire suppresses. That is the described failure, arriving through the field the fallback depends on.The behaviour matches
master(this path already nulledresolvedAt), so it is not a regression — but the comment asserts an invariant the code does not provide during the migration window, which is the window where the fallback is load-bearing.- Either preserve
resolvedAton the not-applied path as well (mirroring thesuppressionAnchor/pluginClosedAtrule, which would make the stated invariant true), or narrow the comment to say the protection covers rows carrying an explicitpluginClosedAtand that legacy rows remain exposed until their first successfully-applied re-fire.
- Either preserve
-
[tests]
packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:1171— The new multi-cycle suite is the right idea but is single-fingerprint throughout:mkCycleWorldmodels one state row and one issue, andmocks.state.getreturns that single row regardless of fingerprint. No test in the file exercises a multi-member aggregate across resolve → re-fire (the suite contains no reference tohas-unresolved-siblingsorlast-member-resolved), which is why the Critical above is green on CI — all 20 checks pass at this head.The PR correctly diagnoses that the original defect shipped because every re-fire test was single-shot; the same reasoning applies one level up — these tests are single-fingerprint, and the authorship record's scope mismatch only shows up with siblings.
- Add a cycle test with two member fingerprints on one
aggregateKey: fire both, resolve the non-last one, resolve the last one (cancel lands), then re-fire the non-last one and assertalertmanager.firing.reopenedrather thanalertmanager.firing.suppressed.
- Add a cycle test with two member fingerprints on one
Suggestions (2)
-
[type-design]
packages/plugins/paperclip-plugin-alertmanager/src/types.ts:201—pluginClosedAt?: string | nullcarries three meanings (string= ours,null= positively not ours,undefined= unknown/legacy). The tri-state is well documented and the reasoning is sound, but becausePick<AlertStateRecord, "pluginClosedAt">keeps the property optional, a future call site can omit it entirely and silently land in the legacy fallback with no type error. Consider requiring it explicitly (pluginClosedAt: string | null | undefined) indecideRefire's parameter type so omission is a compile error while absence stays expressible. -
[comments]
packages/plugins/paperclip-plugin-alertmanager/README.md:369— The updated decision table's rows are keyed onpluginClosedAtbeing populated ornull, but the legacyundefinedcase is only described in the prose below. Since legacy rows take a different branch than thenullrow directly above them, a fourth row (or a parenthetical on thecancelledrow) would keep the table self-contained for the migration window.
Strengths
- The root-cause framing is exact and the fix is at the right level: replacing an inference with a recorded fact, rather than adding another special case on top of
resolvedAt. pluginCancelLandedis flipped at precisely the right point (:1983, immediately after the successfulissues.update), and the execution-lock/withheld-cancel branch correctly does not flip it — that distinction is easy to get wrong.- The conditional-spread write is genuinely necessary and its test ("keeps re-opening its own close when a resolved notification is redelivered") captures the real regression it prevents; I verified both
state.setsites build full records from...existing/...tracked, so "leaves the previous value untouched" is accurate. pluginClosureUpdatedeliberately mirrors thesuppressionAnchorrule directly above it, with the reasoning spelled out — the two now cannot drift apart silently.- The
buildRecoveredStateRecordcomment's claim that both callers reject terminal issues is accurate (:803,:2110), sopluginClosedAt: nullthere is safe. - The PR description's honesty about what was not verified (no real Alertmanager delivery, no PostgreSQL state store, mocked aggregate paths) is exactly right — and it is the aggregate path, called out there as mocked, where the Critical sits.
Recommended Action
- Fix the Critical before merge — the aggregate authorship scope mismatch silently mutes genuine recurrences for every multi-member aggregate, in the direction this PR exists to eliminate.
- Address the Important issues this cycle: add the two-fingerprint aggregate cycle test (it is the test that would have caught the Critical), and either fix or narrow the
issue_missing/failed-RPC invariant comment. - Consider the Suggestions opportunistically.
…he close happens Ally review of #1648 found a Critical: `pluginClosedAt` is written per fingerprint, but the close it records happens to the aggregate's *shared* issue, and only the last member to resolve lands it. Every sibling kept the firing write's `pluginClosedAt: null` while the shared issue was plugin-cancelled, so the sibling's next re-fire read an explicit "not ours", took the operator-suppression branch, and muted a genuine recurrence for the whole window. Deterministic for any aggregate with two or more members, and a regression against master, where the sibling's `resolvedAt` produced a re-open. A member whose resolve defers the close to a sibling has not learned anything about authorship, so it now stops asserting `null` and drops to `undefined` ("unknown"), which falls back to `resolvedAt` exactly as a legacy row does. That keeps the error in the cheap direction — one spurious re-open rather than silence — and needs no fan-out write across member rows, so a partial failure cannot strand a sibling in the muting state. Also from the same review: - `handleFiring` cleared `resolvedAt` unconditionally while deliberately leaving `pluginClosedAt` alone on the `issue_missing` / failed-RPC path. For a legacy row `resolvedAt` *is* the authorship signal, so one transient `issues.get` failure converted our own close into an apparent operator close — the exact failure the neighbouring comment claims to prevent. Preserve it on the not-applied path so the stated invariant is true. - Add the two-fingerprint aggregate cycle test that would have caught the Critical: resolve the non-last member, resolve the last (cancel lands), re-fire the non-last, assert reopened rather than suppressed. Verified to fail on f54a179 at exactly the authorship assertion. - README: give the unknown-authorship row its own line in the decision table and name both sources. Refs BLO-31736
Addressed the Critical + both Important findings — new 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: b067014
All three prior blockers are genuinely fixed, and the aggregate fix is the right shape — authorship dropped to "unknown" rather than asserted, at the exact branch where this delivery decides nothing. I traced the undefined end-to-end and it survives persistence, so the fix is durable and not an artifact of the in-memory test mock.
One new finding blocks, and it comes from the other half of the commit. Closing the legacy-row hole by preserving resolvedAt is correct for closedByPlugin, but resolvedAt is also the escalation kill-switch — so the fix silently disables paging for an actively-firing alert.
Prior Findings Dispositioned (3)
- prior:f54a179 critical 1 — fixed —
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:2073—closeDeferredToSiblingnow catcheshas-unresolved-siblingsand writespluginClosedAt: undefined, so a non-last member stops asserting anullit cannot back. I checked the three neighbouring dispositions:finalization-pendingthrows at:1949before the state write so it cannot leave a stale record;last-member-resolvedwith the terminal guard declining, and a withheld cancel, both correctly leave the value untouched. Theconfig.autoCloseOnResolve !== falseconjunct is right — with auto-close off the plugin never closes, so a terminal status really is an operator's andnullshould stand. - prior:f54a179 important 1 — fixed —
webhook-handler.ts:1469—resolvedAtUpdatenow mirrorspluginClosureUpdateon the same!decisionApplied || issue_missingpredicate, so the stated invariant holds for legacy rows too. Fixed as described; see Critical 1 for the consequence of fixing it this way rather than by narrowing the comment. - prior:f54a179 important 2 — fixed —
packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:1361— two-member aggregate cycle test added, driving fire → non-last resolve → last resolve (cancel lands) → re-fire of the non-last member, assertingalertmanager.firing.reopenedand explicitly assertingsuppressedwas not written.
Critical Issues (1)
-
[code / gstack-review]
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1469— PreservingresolvedAton a not-applied firing delivery silently disables escalation for that alert.resolvedAtis not only the legacy authorship signal; it is a hard bail-out in the escalation sweep, and this commit changed an unconditional write into a conditional one without accounting for that second consumer.The chain is three hops and each one is at the current head:
:1469-1470—resolvedAtUpdateis{}when!decisionApplied || decision.kind === "issue_missing", so the spread at:1487contributes nothing and the row keeps its populatedresolvedAt. Before this commit that line was an unconditionalresolvedAt: null.packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts:380—advanceIssueLadderreturns immediately whenstate.resolvedAtis truthy. That bail precedes every rung: no owner wake, no reporting-chain walk, no board cover at:394-396.- So a fingerprint that had resolved and then re-fired through a single failed
ctx.issues.getor re-sync throw (caught at:1405) keepsresolvedAtpopulated against an issue that is open and actively firing. The sweep reaches that issue —:377only skipsdone/cancelled— and skips it at:380on every pass.
Recovery needs a later delivery that actually applies a decision, i.e. one Alertmanager
repeat_interval(4h by default). For that window the alert pages nobody, and nothing logs that escalation was skipped. This is the muting direction the ticket exists to remove, arriving through the field the previous fix freed.Two details worth having:
-
It is not limited to transient RPC failure.
decideRefirereturnsissue_missingwhen the issue is simply gone (:1054), anddecisionAppliedistrueon that path, so the guard still preservesresolvedAt. That variant is benign only because:377skips a missing issue anyway. -
Secondary effect at
:1430:ladderRestart = Boolean(existing.resolvedAt) || …stays true across the preserved deliveries, so the first delivery that does apply a decision re-armsnextEscalationAt/escalationAttempt/escalationCompletefrom scratch instead of letting the ladder progress — one extra rung interval on top of the gap above. -
Restore the unconditional
resolvedAt: nulland take the other remedy the prior review offered for prior:f54a179 important 1 — narrow the comment at:1451-1455to say the guarantee covers rows carrying an explicitpluginClosedAt, and that legacy rows stay exposed until their first successfully-applied re-fire.resolvedAtmeans "the alert is currently cleared", which is demonstrably false on a firing delivery, so freezing it to protect an inference is the wrong lever. -
If the legacy hole should be closed in code rather than in prose, carry the authorship across explicitly instead of freezing the field it is inferred from: on a not-applied delivery for a row with
pluginClosedAt === undefined, writepluginClosedAt: existing.resolvedAt ?? nulland still clearresolvedAt. That preserves exactly the inferenceclosedByPlugin:1024would have made, while leaving escalation eligibility alone. It does promote an inference to a recorded fact, which cuts against the PR's thesis — worth weighing against the prose fix. -
Either way, a regression test belongs next to the new cycle suite: resolve, then re-fire with
ctx.issues.getrejecting once, then assert the row is still escalation-eligible (resolvedAt === null, oradvanceIssueLaddernot bailing).
Important Issues (0)
Suggestions (3)
-
[comments]
packages/plugins/paperclip-plugin-alertmanager/src/types.ts:214— The field's own docblock still reads "undefinedmeans the row predates this field and authorship is genuinely unknown". As of this commit there is a second source — an aggregate member that deferred its close to a sibling.closedByPlugin's docblock (webhook-handler.ts:1008) and the README table (README.md:373) both gained that case; the canonical field definition is now the only place that describes the tri-state incompletely, and it is the one a reader hits first from an editor jump-to-definition. -
[code]
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:2076—no-membershipon a row that does carry a stored aggregate key keeps the firing write'snull, while a sibling may still land the shared issue's close — the same false assertioncloseDeferredToSiblingwas just added to remove, one branch over. The commit's comment enumerates "a missing membership" under "learned nothing", but for a member row the previous value is a positive claim rather than an absence. This is a consistency note and not a live bug::704-711documents that nothing deletes member rows,upsertAggregateMemberruns on every firing, and:1931prefersstoredAggregateKeyso a config change cannot orphan the membership either — so the branch is a fail-closed guard against corrupted state. Foldingno-membershipintocloseDeferredToSiblingwould make that guard fail-closed in both directions for one extra disjunct. -
[tests]
packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts:1364—memberStatetakes afingerprintparameter it never reads, so both member rows inheritfreshlyFiredState()'s fingerprint. Harmless here because the mock keys rows offstateKey, but it is a fidelity gap in the one test whose point is that two distinct fingerprints share one issue — and it would hide a regression in any path that readsexisting.fingerprintrather thanalert.fingerprint. Either spread{ fingerprint }into the record or drop the parameter.
Strengths
- The aggregate fix is at the right level and picks the correct value:
undefined("unknown") rather than clearing tonull, which keeps the asymmetry argument intact — a spurious re-open costs one cycle, silence costs a window. - I verified the
undefinedsurvives the full round trip, which the in-memory test mock cannot show:serializeMessageisJSON.stringify(packages/plugins/sdk/src/protocol.ts:2274), which drops anundefined-valued key rather than coercing it tonull, andserver/src/services/plugin-state-store.ts:179-185replacesvalue_jsonwholesale viaonConflictDoUpdaterather than merging — so the key reads back absent, i.e.undefined. A JSON-merge store would have silently defeated the whole fix by leaving the oldnullin place; it does not. finalization-pendingthrowing at:1949before the state write is what makes the three-case comment exhaustive rather than merely plausible — the fourth disposition can never reach the record.- The new test asserts both the positive (
reopened) and the negative (suppressednever written), which is what makes it a real regression test rather than a happy-path smoke test. - The comment block at
:2050-2072explains the scope mismatch — per-fingerprint record, per-aggregate close — in terms of why the value is unknowable here rather than just what the code does, so the next reader can extend it correctly. - The README gained the absent-
pluginClosedAtrow, so the decision table is now self-contained for the migration window (prior Suggestion 2 fromf54a179, addressed).
Recommended Action
- Fix the Critical before merge — the escalation kill-switch at
escalation.ts:380makes theresolvedAtpreservation a paging outage for any alert that re-fires through one failedissues.get, which is the failure direction this PR exists to close. - No Important issues outstanding; the three prior blockers are all genuinely resolved.
- Consider the Suggestions opportunistically — the
types.tsdocblock is the cheapest and most likely to mislead a future reader.
CI note: at this head Build, policy, security-review, review, Helm chart and the adapter checks are green, but Typecheck + Release Registry, all six General tests shards and e2e were still in_progress when this review was written, so I could not confirm the new cycle test passes.
Thinking Path
Linked Issues or Issue Description
cancelled-branch halfDedup search. Searched the GitHub PR list (all states) for
resolvedAt,pluginClosedAt,decideRefire,alertmanager,31736,24234. No PR addresses this defect. Related, none overlapping:webhook-handler.ts, but in the intake/owner-resolution/aggregate paths, notdecideRefireor the resolve state writeWhat Changed
types.ts— new optionalAlertStateRecord.pluginClosedAt, documenting whyresolvedAtcould not carry this meaning.webhook-handler.ts— new exportedclosedByPlugin(issue, existing);decideRefirekeys on it instead ofexisting.resolvedAt.handleResolved— tracks whether the plugin's ownstatus: "cancelled"patch actually landed and stampspluginClosedAtonly then. Every other branch (terminal guard declining, withheld cancel, missing membership,autoCloseOnResolve: false) leaves the previous value untouched rather than writingnull.handleFiring— clearspluginClosedAton any delivery that applied a re-fire decision, and leaves it alone onissue_missingor a failed RPC, mirroring thesuppressionAnchorrule directly above it.pluginClosedAt: nullexplicitly so fresh and reconstructed rows never enter the legacy fallback.worker.test.ts— new multi-cycle suite (3 handler cases) plus 3decideRefirecases; two pre-existing tests that asserted the defect updated to the shape the plugin actually produces.README.md— the re-fire decision table's middle column literally readresolvedAt in state; it documented the wrong inference as the contract.Why
doneis special-casedThe plugin's only status writes are
todo(fire, re-open) andcancelled(resolve) — verified, there is no path that closes an issuedone. Sodoneis always someone else's disposition regardless of state, which is what fixes the reported defect on contact rather than one migration cycle later, including for state rows written before this field existed.Why legacy rows fall back to
resolvedAtA row with
pluginClosedAt: undefinedpredates the field and its authorship is genuinely unknown. The two possible errors are not symmetric: reading our close as an operator's mutes a live recurring alert for a whole suppression window, while reading an operator's close as ours costs one unwanted re-open that the very next firing state-write corrects. Silence is the worse failure, so the fallback keeps the old reading. Legacy rows drain on their first post-deploy firing.Verification
The load-bearing check — the new tests fail on the parent commit. Stash only the two source files, keeping the new tests, and re-run:
git stash push -- packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts \ packages/plugins/paperclip-plugin-alertmanager/src/types.ts pnpm vitest run src/__tests__/worker.test.tsWhy the existing tests missed this, and what the new ones do differently. Every prior re-fire test was single-shot against a hand-written state row, so cycle 1 — where the guard correctly holds — looks correct. The suite in the ticket (
worker.test.ts:2679) only exercised the state-missing recovery path. The new tests carry both the state row and the issue status forward between deliveries, withissues.updatemutating the live issue, so if a delivery does resurrect the row the next one genuinely cancels it. The second-stage assertion observes the real cascade rather than assuming it.Deploy parity. The ticket specified the CI assertion against
ef14130while production runsa66afc8e; the 9-commit gap touches no alertmanager files (alldb/*andlinear/*), so the assertion and the production observation measure the same code. This branch is cut fromfae70d7.Not verified here: this has not been run against a real Alertmanager delivery or a real state store. The aggregate/fence paths are mocked and the two-cycle sequence has not been exercised against PostgreSQL. The production check on the issue (two daily
ClusterAdminDriftfire/clear cycles with noactorType: pluginstatus patch) is the real confirmation.Risks
Low-to-moderate, and the moderate part is deliberate and bounded.
donealert issue is no longer re-opened on re-fire. It is suppressed instead, and fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349's suppression is time-bounded, so a persistently firing alert still re-opens with an explanatory comment once the window expires. It does not go silent permanently.autoCloseOnResolve: false: with auto-close off the plugin never cancels, sopluginClosedAtstays null and every terminal row now reads as operator-closed. That is the correct reading for that config — all terminal transitions there are human- or agent-authored — but it is a change from the previous "reopen on any re-fire after a resolve".issue_missing/failed-RPC rule, and the no-clear-when-the-resolve-declines rule (the last has its own test, for a redeliveredresolvednotification). Each exists so that a close of ours is never mistaken for an operator's, which would suppress a genuine recurrence.undefinedis handled explicitly. No schema change, no backfill.types.tsandwebhook-handler.ts. Different regions (intake, aggregate store), so textual conflicts are likely only in theAlertStateRecordinterface andworker.test.tsimports; no semantic overlap withdecideRefireor the resolve state write.cancelledwith the plugin as last author, so after this lands they read as ordinary plugin-authored recurrences and keep cycling. Restoring their destroyeddonehas to happen after deploy or the next 06:00Z cycle overwrites it again. Tracked on the issue, not in this PR.Model Used
claude-opus-4-5), extended thinking, via Claude Code in a Paperclip heartbeat run. Tool use: repo read/edit,vitest,tsc,gh, Paperclip MCP.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template