What happened
During the automated review of PR #962, the review agent discovered two pre-existing bugs in pkg/provider/pagerduty/pagerduty.go:
-
Pagination offset bug (review comment, Sep 10): In GetHistoricalAlertsForCluster, incidentListOffset is never reset between iterations of pdServiceIDs. After querying the first service, subsequent services start pagination at the wrong offset, potentially skipping incidents.
-
Incidents slice accumulation bug (review comment, Sep 10): The incidents slice is declared outside the outer pdServiceIDs loop and never reset, causing incidents from one service to carry over into the count for the next service, inflating totals.
Both bugs were fixed in PR #962's second and third commits (04befce, 5c96b53), but that PR was closed without merging on Sep 23 as a Jira integration test artifact. The surviving PR #942 does not include these fixes, and the bugs remain in the codebase on master.
What could go better
These are pre-existing bugs that affect any cluster with multiple PagerDuty service IDs. They cause incorrect incident counts in GetHistoricalAlertsForCluster — pagination may skip results from services after the first, and accumulated counts across services inflate totals.
The bugs went unnoticed because they only manifest when a cluster maps to multiple PD service IDs, which may be uncommon in practice. Confidence is high that these are real bugs based on the code structure — the review agent's analysis of the loop/variable scoping is straightforward to verify.
The knowledge was lost when PR #962 was closed because there is no mechanism to transfer review findings from a superseded PR to the surviving one.
Proposed change
In pkg/provider/pagerduty/pagerduty.go, in GetHistoricalAlertsForCluster:
- Reset
incidentListOffset = 0 at the start of each iteration of the outer for _, pdServiceID := range pdServiceIDs loop.
- Move the
incidents slice declaration (or reset it with incidents = incidents[:0]) inside the outer loop so it does not accumulate across service IDs.
These are the same fixes applied in PR #962 commits 04befce and 5c96b53. The fixes can be cherry-picked or re-applied to the current master branch or to PR #942 before it merges.
Validation criteria
After the fix: (1) A unit test with multiple pdServiceIDs confirms that incidentListOffset resets to 0 for each service ID. (2) A unit test confirms that incidents from service A do not appear in the results for service B. (3) Existing PagerDuty provider tests continue to pass.
Generated by retro agent from #962
What happened
During the automated review of PR #962, the review agent discovered two pre-existing bugs in
pkg/provider/pagerduty/pagerduty.go:Pagination offset bug (review comment, Sep 10): In
GetHistoricalAlertsForCluster,incidentListOffsetis never reset between iterations ofpdServiceIDs. After querying the first service, subsequent services start pagination at the wrong offset, potentially skipping incidents.Incidents slice accumulation bug (review comment, Sep 10): The
incidentsslice is declared outside the outerpdServiceIDsloop and never reset, causing incidents from one service to carry over into the count for the next service, inflating totals.Both bugs were fixed in PR #962's second and third commits (
04befce,5c96b53), but that PR was closed without merging on Sep 23 as a Jira integration test artifact. The surviving PR #942 does not include these fixes, and the bugs remain in the codebase onmaster.What could go better
These are pre-existing bugs that affect any cluster with multiple PagerDuty service IDs. They cause incorrect incident counts in
GetHistoricalAlertsForCluster— pagination may skip results from services after the first, and accumulated counts across services inflate totals.The bugs went unnoticed because they only manifest when a cluster maps to multiple PD service IDs, which may be uncommon in practice. Confidence is high that these are real bugs based on the code structure — the review agent's analysis of the loop/variable scoping is straightforward to verify.
The knowledge was lost when PR #962 was closed because there is no mechanism to transfer review findings from a superseded PR to the surviving one.
Proposed change
In
pkg/provider/pagerduty/pagerduty.go, inGetHistoricalAlertsForCluster:incidentListOffset = 0at the start of each iteration of the outerfor _, pdServiceID := range pdServiceIDsloop.incidentsslice declaration (or reset it withincidents = incidents[:0]) inside the outer loop so it does not accumulate across service IDs.These are the same fixes applied in PR #962 commits
04befceand5c96b53. The fixes can be cherry-picked or re-applied to the currentmasterbranch or to PR #942 before it merges.Validation criteria
After the fix: (1) A unit test with multiple
pdServiceIDsconfirms thatincidentListOffsetresets to 0 for each service ID. (2) A unit test confirms that incidents from service A do not appear in the results for service B. (3) Existing PagerDuty provider tests continue to pass.Generated by retro agent from #962