fix(github): exclude the panel's per-pass timestamp from the comment idempotency check (#9069) - #9073
Merged
Merged
Conversation
…idempotency check (#9069) The PR panel embeds `<sub>Review updated: <timestamp></sub>`, re-stamped from `reviewedAt ?? new Date()` on every render. createOrUpdateIssueCommentWithMarker compared raw bodies, so the panel could never match its own posted body and the idempotency skip was unreachable for it: every re-gate tick PATCHed GitHub purely to move a clock, and each PATCH generated an inbound issue_comment.edited delivery that ingress then classified as our own noise and discarded. That loop accounted for 79,612 of ~309,600 lifetime webhook deliveries (26%), plus the matching outbound writes against the REST rate limit. Compare through comparableCommentBody, which normalizes only that generated line. Compare-only: the posted body keeps its real timestamp, and a body whose content genuinely changed still PATCHes with the fresh one. The surviving timestamp then means "the review last changed at X" rather than "we last looked at X". Also makes the #6724 no-op accounting honest — pr_public_surface_published was being recorded on every clock-only pass, inflating the public reviews-completed count and the review-burst anomaly counter. markPullRequestSurfacePublished stays outside that branch, so the surface stamp still advances and the repair sweeps are unaffected. Closes #9069
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9073 +/- ##
==========================================
+ Coverage 90.56% 92.65% +2.09%
==========================================
Files 96 807 +711
Lines 22490 80502 +58012
Branches 3884 24410 +20526
==========================================
+ Hits 20367 74589 +54222
- Misses 1945 4840 +2895
- Partials 178 1073 +895
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The PR panel embeds
<sub>Review updated: <timestamp></sub>(src/review/unified-comment.ts), re-stamped fromreviewedAt ?? new Date()on every render (unified-comment-bridge.ts) — and the processor never passesreviewedAt.createOrUpdateIssueCommentWithMarkercompared raw bodies:So the panel could never match its own posted body, and the idempotency skip — whose stated purpose is "without this, every cycle PATCHes GitHub (a write + rate-limit cost) for no visible change" — was unreachable for the one comment it mattered most for.
Every re-gate tick therefore PATCHed GitHub purely to move a clock, and each PATCH generated an inbound
issue_comment.editeddelivery that ingress then classified as our own noise and discarded.Measured impact (live ledger)
check_suite.completedissue_comment.editedissue_comment.created~309,600 deliveries against ~12,500 reviews — roughly 25 webhooks per review, with a quarter of it this self-feeding loop, plus the matching outbound writes against the REST rate limit.
Change
Compare through a new
comparableCommentBody, which normalizes only that one generated line.Compare-only — the body actually posted keeps its real timestamp, and a body whose content genuinely changed still PATCHes carrying the fresh one. The surviving timestamp then reads as "the review last changed at X" rather than "we last looked at X", which is the more useful meaning and the one the wording already implies.
The regex is bounded to
[^<]*and anchored per-line, so it can only ever match this exact generated line; every caller-supplied string reaching a comment body is angle-escaped upstream (escapePublicHtmlAngles), so contributor text cannot forge a<sub>wrapper. Comments without the line (close explanations, visual follow-ups) keep byte-exact comparison.Secondary correctness win
changed: falsenow propagates correctly to the #6724 no-op accounting.pr_public_surface_publishedwas previously recorded on every clock-only pass, inflating both the public "reviews completed" count and the review-burst anomaly counter.markPullRequestSurfacePublisheddeliberately stays outside that branch, so the surface stamp still advances every pass and the repair sweeps (surfaceRepairPriorityPullNumbers, backlog convergence) are unaffected.Validation
npx vitest run test/unit/github-comments.test.ts— 24 passed, including 3 new cases.src/github/comments.ts: the only uncovered line iscreateOrUpdateAgentCommandComment, which is pre-existing and unrelated (it sits at line 93 onmainand line 117 here, shifted by this patch's added lines). No new uncovered lines or branches.tsc --noEmit --incremental falseclean.main), so they are deliberately left unreformatted rather than dragging in a large unrelated diff.Tests added
changed: false.<sub>sequence is untouched, and multi-occurrence/gbehavior is pinned.Closes #9069