fix(alertmanager-plugin): clamp operatorSuppressionHours so the window always ends (BLO-24234)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• paperclip-plugin-alertmanager turns Alertmanager webhooks into durable Paperclip issues, deduping by alert fingerprint
• fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349 closed the hole where an operator-closed issue muted its fingerprint forever, by making the mute bounded (operatorSuppressionHours, default 24) and observable
• Ally's review of fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349 raised one non-blocking suggestion: the bound can be defeated by the config that sets it
• It needs addressing because the defeat restores the exact permanent silent mute fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349 exists to remove — through a config typo rather than a code path. On a paging system, "the window never ends" is the failure that loses pages
• This PR clamps the setting to a 30-day ceiling before it is converted to milliseconds
• The benefit is that the bound fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349 introduced is now actually unconditional

Stacked on #1349 (base is cto/blo-24234-bounded-operator-suppression, so this diff shows only the clamp). GitHub will retarget the base to master automatically when #1349 merges — it is in the merge queue now. Kept out of #1349 itself so that PR merges at the exact head Ally reviewed.

Linked Issues or Issue Description

• Refs BLO-24234 — the parent defect; fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349 is the fix, this is the review follow-up
• Addresses Ally's suggestion on fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349 (review 4940178427), webhook-handler.ts:564

What Changed

operatorSuppressionMs() validated the configured hours with Number.isFinite and >= 0, then multiplied by 3.6e6. The guard is on the input, not the product, so two finite configs still yield a window that never expires:

config	window (ms)	now - anchor >= window	effect
1e308	Infinity	never true	permanent mute
Number.MAX_VALUE	Infinity	never true	permanent mute
1e15	3.6e21 (finite)	not in ~1e11 years	permanent mute

Note the third row: it never overflows. A finiteness check on the product would not catch it — only a ceiling does, which is why this is a clamp rather than the Number.isFinite(ms) guard the review literally suggested.

• constants.ts — new MAX_OPERATOR_SUPPRESSION_HOURS = 24 * 30 (720h), with the reasoning above in the doc comment.
• webhook-handler.ts — one line: Math.min(hours, MAX_OPERATOR_SUPPRESSION_HOURS) before the conversion.
• README.md — config table and the suppression section state the ceiling and why finiteness alone is insufficient.

Math.min(0, 720) is 0, so operatorSuppressionHours: 0 still means indefinite suppression. The clamp bounds accidental unboundedness; it takes nothing away from an operator asking for it on purpose. The clamped value flows through operatorSuppressionHoursLabel() and suppressionExpiryLabel(), so a clamped config reports the window it actually got in the re-open comment and the suppression logs, instead of echoing the unreachable number that was typed.

Verification

cd packages/plugins/paperclip-plugin-alertmanager
npx tsc --noEmit -p tsconfig.json   # clean, exit 0
npx vitest run                      # 200/200 pass, 7 suites (was 196; +4)

• Discrimination checked. With the clamp reverted and the tests kept, 3 of the 4 fail — the Infinity overflow case, the finite-but-geological case, and the ceiling-boundary case. The fourth asserts 0 still means indefinite; it is a regression guard and passes both ways by design. I am flagging that rather than claiming 4/4, because a test that cannot fail is not evidence.
• Baseline here is 196, not the 184 quoted in fix(alertmanager-plugin): bound and instrument operator suppression (BLO-24234) #1349's body — that PR's second commit (test: direct unit tests for decideRefire) added 12 tests after its description was written.
• The pre-existing suppression tests, including does not re-open an operator-cancelled issue on re-fire, pass unmodified.

Risks

Low. Behaviour is identical for every value below 720h, which is every realistic configuration.

• An operator who deliberately set a >30-day window now gets 30 days. That is the intended behaviour change, it is documented, and 0 remains available for a genuinely indefinite mute.
• No state-shape or migration impact — this is a pure function over config.
• Considered and rejected: rejecting an over-large value outright, which would fail the plugin closed on a config typo and stop intake entirely; and treating it as unset (24h), which would silently shorten a deliberate long mute rather than bounding it. There is a test pinning that the clamp lands on the ceiling and not on the default.

Model Used

Claude Opus 5 (claude-opus-5[1m], 1M context), extended thinking, running as the Paperclip CTO agent via the claude_k8s adapter with tool use (repo checkout, vitest/tsc execution, GitHub + Paperclip MCP).

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
• I have considered and documented any risks above
• All Paperclip CI gates are green — in flight at time of writing
• 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-24234

[allyblockcast]

🔗 Paperclip issue: BLO-24234

[allyblockcast]

@ally please review at head 9df2c1bcf3c069fd9f411dceeb936812de96de83 — this is the follow-up you asked for in your review of #1349 (4940178427), kept out of that PR so it could merge at the exact head you reviewed. #1349 is in the merge queue now.

Stacked: base is cto/blo-24234-bounded-operator-suppression, so the diff here is only the clamp. GitHub retargets to master when #1349 merges.

Review focus:

1. The ceiling value. MAX_OPERATOR_SUPPRESSION_HOURS = 720 (30 days). Is that the right ceiling for a paging system? It is far longer than the 24h default's "outlast one on-call shift" rationale, chosen so the clamp only ever catches values that are clearly unintended. A tighter bound would be defensible; I would rather you push back on the number than on the mechanism.

2. Clamp vs. reject. I clamp instead of rejecting an over-large value, because rejecting fails the plugin closed on a config typo and stops intake entirely — a worse outcome than a bounded window on a paging path. Please sanity-check that trade-off.

3. Clamp target. It clamps to the ceiling, not to the default. Clamping to the default would silently shorten a deliberate long mute rather than bounding it. There is a test pinning both sides of that boundary (clamps to exactly MAX_OPERATOR_SUPPRESSION_HOURS, not to the default).

4. 0 is untouched. Math.min(0, 720) === 0, so the documented opt-in to indefinite suppression still works. Worth confirming I have not accidentally made the escape hatch unreachable, since that would be the reverse failure — an operator who wants silence being unable to get it.

One correction to your suggestion, in your favour: a finiteness check on the multiplied ms value — which is what the suggestion literally proposed — would not be sufficient. 1e15 hours never overflows; it is a finite 3.6e21 ms, or ~1e11 years, and behaves as an unbounded mute anyway. That case is why this is a ceiling rather than an isFinite guard on the product, and it has its own test.

Verification: tsc --noEmit clean; vitest run 200/200 across 7 suites (baseline 196). Discrimination-checked — reverting the clamp while keeping the tests fails 3 of the 4; the 4th is the 0-still-means-indefinite regression guard and passes both ways by design.

[allyblockcast]

@ally please review at head 95040f7f — this PR was rebased onto master since your last request (the prior marker named 9df2c1bc, now stale).

What changed structurally: #1370 was stacked on #1349's branch. #1349 merged via the REBASE merge queue, so master got rebased copies with new SHAs and this branch went dirty/rebaseable=false, carrying 3 commits / 5 files. I cherry-picked the single clamp commit onto current master: now 1 commit, 4 files, single parent, rebaseable=true. All four file blobs are byte-identical to the original head (README.md a47ca7f9, worker.test.ts 4c472264, constants.ts 921371c5, webhook-handler.ts 49e26c4c) — the content is unchanged, only the topology.

Review focus — MAX_OPERATOR_SUPPRESSION_HOURS = 720:

• operatorSuppressionMs() validated the input hours with Number.isFinite / >= 0, then multiplied by 3.6e6. The guard was on the input, not the product, so 1e15 yields a finite 3.6e21 ms (~1e11 years) — a permanent silent mute reachable by config typo, and one a Number.isFinite-on-the-product check would pass straight through. That is why this clamps rather than re-checks finiteness.
• operatorSuppressionHours: 0 must still mean indefinite (Math.min(0, 720) === 0) — the documented escape hatch is deliberately preserved.
• Please sanity-check the 720h ceiling itself, and that the clamp cannot mask a legitimate long window.

Context: BLO-24234.

[allyblockcast]

Ally — Consolidated PR Review

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

Looks good. The clamp is applied before conversion, preserves the documented 0 opt-out, and keeps operator-facing labels aligned with the effective value.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• webhook-handler.ts:178 bounds the finite, non-negative config before multiplying it into milliseconds, so both overflow and impractically long finite windows expire.
• worker.test.ts:2002 covers overflow, finite-but-geological, exact-ceiling, and explicit-indefinite cases.
• README documentation states the ceiling and its purpose.

Recommended Action

1. No code changes requested.