fix(ops): scope the PagerDuty cooldown to rows that actually paged - #9810
Conversation
The cooldown counted every `external_notification.pagerduty` row for the dedupKey via `countRecentAuditEventsForActorAndTarget`, which filters neither `outcome` nor `detail`. But that eventType is written for every path: a real page (completed/triggered), a suppression (denied/cooldown_active), a failed page (error), and an auto-resolve (completed/resolved). So a single page self-renewed the window every cron tick (its own denied row kept the count > 0), a non-page silenced a real one, a failed page blocked its own retry, and an auto-resolve suppressed the re-page for a flapping condition. Add `countRecentAuditEventsForActorTargetAndOutcome`, shaped like its sibling with `outcome` + `detail` equality terms, and count only completed/triggered rows for both the loopover and legacy gittensory actors. Export `PAGERDUTY_AUDIT_DETAIL_TRIGGERED`/`_RESOLVED` and use them at both the write and read sites so the two spellings can never drift. The denied/error/resolved rows are still written (the operator's evidence); they are simply no longer counted. Closes JSONbored#9695
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 11:02:12 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9810 +/- ##
==========================================
+ Coverage 77.18% 77.84% +0.65%
==========================================
Files 283 285 +2
Lines 59729 61784 +2055
Branches 6696 7377 +681
==========================================
+ Hits 46104 48097 +1993
- Misses 13342 13351 +9
- Partials 283 336 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Held for manual review: the gate and required CI are green, but GitHub reports this pull request's mergeable state as |
GitHub reports `mergeable_state: "unstable"` both for a failing non-required check and for checks that are still running. The gate treated both as a manual-review hold, so every PR whose CI had not finished got an enforcement label: the executor denies merge and approve while it is present, and merge-train.ts evicts a held sibling from the train entirely. Section 1b's release condition also tested the same state, which made the label a latch -- applied while CI was red, then not liftable because CI was red. Observed on #10098, stuck until a human removed it by hand. Neither escalation path could clear it: `releasedHolds` contains only `guardrailHit`, and `guardrailEscalationCleared` requires green CI. An unstable state no longer holds. It does not become mergeable either: `wouldApprove` gains its own explicit `!unstableHolds` term (previously implied by the hold), keyed on `unstableHolds` rather than the raw state so #9810's ignored-check dismissal stays approvable, and `wouldMerge` already required `clean`. Both label sites stop emitting -- the merge-autonomy fallback is removed and the disposition ternary returns no label rather than falsely claiming `ready-to-merge`. Closes #10116
…ust its cache key (#10163) gate.ignoredCheckRuns (#9813) is supposed to exclude a maintainer-declared check-run from CI resolution entirely, and the reducer + fetchLiveCiAggregatePreferGraphQl honour it. But ci-resolution.ts folded it into the durable cache key at all three hand-offs while dropping it from the arg object passed downstream, so cachedFetchLiveCiAggregate's ignoredCheckRuns was always undefined -- TypeScript can't catch it because the property is optional at every hop. The consequence on the live maintenance path: ciAggregate's ignoredCheckDetails is always [], ignoredCheckNonPassing is always empty, and unstableExplainedByIgnoredChecks is always false, so the #9810 follow-up that stops an ignored check from holding a PR at mergeable_state 'unstable' can never fire in production, and the planner and executor can disagree about the same PR's CI state. Forward args.ignoredCheckRuns at all three call sites (fetchLiveCiAggregateWithRequiredContexts, cachedLiveCiAggregate, refreshLiveCiAggregate), mirroring how advisoryCheckRuns is already threaded through the same hops. Signatures stay optional so positional callers are byte-identical; the requiredContextsKey composition and advisoryCheckRuns threading are unchanged, and nothing outside ci-resolution.ts is touched. Closes #10018
What & why
The PagerDuty cooldown suppresses a repeat page for the same
dedupKeywithin the window. It counted recent rows viacountRecentAuditEventsForActorAndTarget, which filters neitheroutcomenordetail. ButauditPagerDutyNotificationwrites the sameexternal_notification.pagerdutyeventType +targetKeyfor every path:completed/triggereddenied/cooldown_activeerrorcompleted/resolvedSo the count was
> 0for causes that never paged anyone. Concretely, at the default 60-minute cooldown with a sub-hourly cron:deniedrow suppresses a later genuine anomaly.The fix
countRecentAuditEventsForActorTargetAndOutcometosrc/db/repositories.ts, shaped exactly likecountRecentAuditEventsForActorAndTargetwith additionaleq(outcome)+eq(detail)terms.triggerPagerDutyIncidentcounts onlyoutcome: "completed",detail: "triggered"rows, for both theloopoverand legacygittensoryactors (the two-actor union is preserved).PAGERDUTY_AUDIT_DETAIL_TRIGGERED/PAGERDUTY_AUDIT_DETAIL_RESOLVEDand use them at both the write (auditPagerDutyNotificationcalls) and the read (cooldown count) sites, so the two spellings can never drift.denied/error/resolvedrows are still written — they are the operator's evidence that suppression/failure/resolution happened; they are simply no longer counted. No change to the cooldown-minutes or min-severity resolution.Tests (
test/unit/notify-pagerduty.test.ts)Three new regression cases, each failing on
main: a recentdenied/cooldown_activerow, acompleted/resolvedrow, and anerrorrow each do not suppress the next page (callslength 1). The existing "a real triggered page suppresses" and two-actor cases still pass.Validation
npm run typecheckgreen; the pagerduty suite (38 tests) green.git diff --check <base> HEADclean; no route/schema/migration change (a DB counter + service logic).Closes #9695