Skip to content

fix(executor): make the close-explanation comment marker-idempotent so a failed close's retry never stacks duplicates - #8810

Merged
JSONbored merged 2 commits into
mainfrom
fix/close-comment-marker-dedup
Jul 26, 2026
Merged

fix(executor): make the close-explanation comment marker-idempotent so a failed close's retry never stacks duplicates#8810
JSONbored merged 2 commits into
mainfrom
fix/close-comment-marker-dedup

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Round-2 audit fix (#8803): the executor's close path posted closeComment via a plain unmarked POST immediately before closePullRequest — a transient failure on the close meant every subsequent replan re-posted the identical "why we closed you" comment before re-attempting, stacking duplicates until the close succeeded.

Changes

  • src/github/comments.tscloseExplanationMarker(closeKind) (per-kind, so distinct close reasons keep separate canonical comments) + exported createOrUpdateCloseExplanationComment that embeds the marker in the posted body (HTML comment, renders invisibly — the search-side helper only finds comments whose body carries the marker, same as the intelligence/visual callers). Byte-identical retry → skip with duplicate cleanup; reworded → PATCH.
  • src/services/agent-action-executor.ts — the close case routes through the wrapper, passing action.closeKind.

Test plan

  • npm run typecheck — clean
  • 3 new helper tests: first-attempt POST carries marker + body; identical-body retry skips with zero POST/PATCH (the exact incident shape); distinct/undefined closeKind markers
  • Executor suite updated to the new call (mocked like its pr-actions siblings) — 206/206 across both suites
  • Changed hunks: 0 uncovered statements/branches

Closes #8803

…o a failed close's retry never stacks duplicates (#8803)

The executor's close case posted action.closeComment via a plain
createIssueComment POST with no marker — when the comment landed but the
immediately-following closePullRequest failed transiently, the next replan
re-derived the same violation and posted a SECOND identical "why we closed
you" comment before retrying the close, repeating every failed cycle.
closeComment is populated for essentially every real close reason
(blacklist, contributor-cap, review-nag, copycat, screenshot-table,
linked-issue hard rule, unlinked-issue match, generic heuristic).

- comments.ts: closeExplanationMarker(closeKind) + an exported
  createOrUpdateCloseExplanationComment wrapper embedding the marker in the
  posted body (HTML comment — invisible to the contributor) so the existing
  marker machinery finds it on retry: byte-identical replan → skip, reworded
  → PATCH the canonical comment. Distinct closeKinds keep distinct markers.
- agent-action-executor.ts: the close case routes through the wrapper.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 26, 2026
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

❌ 3 Tests Failed:

Tests completed Failed Passed Skipped
21240 3 21237 21
View the top 3 failed test(s) by shortest run time
test/unit/queue.test.ts > queue processors > linked-issue hard-rule violation persistence (#linked-issue-hard-rule-persistence) > REGRESSION (live-issue-state-change-before-re-evaluation): Pass 1 flags a real violation; Pass 2's live re-parse re-evaluates the SAME issue as clean (assignee removed) but the persisted violation still closes the PR
Stack Traces | 0.338s run time
AssertionError: expected 'error' to be 'completed' // Object.is equality

Expected: "completed"
Received: "error"

 ❯ test/unit/queue.test.ts:2812:32
test/unit/queue-2.test.ts > queue processors > #4603: the SAME sub-floor defect one-shot-closes when aiReviewLowConfidenceDisposition is explicitly one_shot
Stack Traces | 0.456s run time
AssertionError: expected 'error' to be 'completed' // Object.is equality

Expected: "completed"
Received: "error"

 ❯ test/unit/queue-2.test.ts:361:28
test/unit/queue.test.ts > queue processors > linked-issue hard-rule violation persistence (#linked-issue-hard-rule-persistence) > REGRESSION (body-edit-during-grace-window): Pass 1 flags a real owner-assigned violation; Pass 2's live re-parse finds NO linked issues (body edited) but the persisted violation still closes the PR
Stack Traces | 0.518s run time
AssertionError: expected 'error' to be 'completed' // Object.is equality

Expected: "completed"
Received: "error"

 ❯ test/unit/queue.test.ts:2767:32

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-26 07:19:47 UTC

4 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Manual Review

Review summary
This closes the loop on #8803 (round-2 of the close-comment duplication bug): the PR's close path now posts through createOrUpdateCloseExplanationComment, which embeds a closeKind-scoped HTML marker in the body and reuses the existing createOrUpdateIssueCommentWithMarker skip/PATCH machinery, so a transient close failure's replan re-posts the identical body and gets skipped (with duplicate cleanup) instead of stacking a new comment every retry cycle. The wiring is correct end-to-end: agent-action-executor.ts:1052-1058 threads action.closeKind through to the new helper, and the new unit tests in github-comments.test.ts drive the real HTTP-mocked path (not a fabricated payload) to prove both the first-POST and identical-body-skip shapes. The one gap is that executeIssueMaintenanceActions's own close branch (still visible unchanged in the full file content, using plain createIssueComment) has the exact same duplicate-on-retry exposure for issue closes and was left untouched — worth confirming that's intentionally out of scope for #8803.

Nits — 5 non-blocking
  • executeIssueMaintenanceActions's close branch (agent-action-executor.ts, the `else { if (action.closeComment) await createIssueComment(...) }` block) still posts via the plain unmarked call, so the same retry-duplication bug this PR fixes for PRs remains open for issue closes — worth a follow-up or a note on why it's excluded from fix(executor): close-comment has no marker dedup — a failed close retry re-posts the identical 'why we closed you' comment every cycle #8803's scope.
  • The new tests use 'blacklist' as a closeKind literal (test/unit/github-comments.test.ts) — consistent with existing codebase terminology (coupledCloseOutcome already references 'blacklist' as a closeKind), so not introduced by this PR, but the external brief's denylist suggestion is worth a follow-up naming pass if the team wants to standardize.
  • The PATCH (reworded-body) branch of createOrUpdateCloseExplanationComment is only exercised indirectly via the shared helper's existing generic tests, not with a close-explanation-specific PATCH test — low risk since the code path is identical, but a fourth test would make the 'reworded → PATCH' claim in the description directly verifiable.
  • Add a close-explanation-specific PATCH test (reworded body, distinct closeKind) mirroring the existing skip test, to directly cover the 'reworded → PATCH' claim in the PR description.
  • Consider routing executeIssueMaintenanceActions's close branch through the same createOrUpdateCloseExplanationComment helper for parity, since issue closes are exposed to the identical failed-close-then-retry duplication.

CI checks failing

  • validate
  • validate-tests

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8803
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 13 registered-repo PR(s), 13 merged, 246 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 13 PR(s), 246 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The PR routes the executor's closeComment call through a new marker-based createOrUpdateCloseExplanationComment helper (keyed by repo/PR/closeKind), replacing the plain createIssueComment call, so a failed close's retry will PATCH/skip the canonical comment instead of posting a duplicate — directly matching the issue's requested fix and includes a test reproducing the comment-then-close-fails-then

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 13 PR(s), 246 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 26, 2026
@JSONbored
JSONbored merged commit a6a280a into main Jul 26, 2026
3 of 4 checks passed
@JSONbored
JSONbored deleted the fix/close-comment-marker-dedup branch July 26, 2026 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(executor): close-comment has no marker dedup — a failed close retry re-posts the identical 'why we closed you' comment every cycle

1 participant