Skip to content

Delete the trigger comments nothing needs any more#56

Open
kristofferR wants to merge 2 commits into
mainfrom
feat/tidy-consumed-commands
Open

Delete the trigger comments nothing needs any more#56
kristofferR wants to merge 2 commits into
mainfrom
feat/tidy-consumed-commands

Conversation

@kristofferR

@kristofferR kristofferR commented Jul 26, 2026

Copy link
Copy Markdown
Owner

A PR driven through a dozen rounds collects a dozen @coderabbitai review comments and a dozen
acknowledgements, which buries the conversation a human came to read. crq now removes its own half of
that as rounds progress, and crq tidy <repo> <pr> [--dry-run] does it on demand.

What it will not touch

Deleting a comment crq still reads is how this becomes expensive, so the rules are the feature.

Only comments crq posted. Candidates are the command IDs recorded on crq's own rounds — never
anything matching the text. A person's @coderabbitai review is their decision to ask and not crq's
to erase, and since crq posts under the same account, authorship alone cannot tell the two apart.

Only from rounds that have progressed (your rule). A live round keeps its command, because that
is the comment DecideFire adopts instead of posting another one.

Only after the bot answered it, so it was read rather than merely old.

Only if it predates the current head. Adoption only ever considers commands newer than the head
commit; delete one of those and the next pump sees no command, posts another, and buys a second
review of the same code.

Never the bots' own comments. An auto-generated reply can be a rate-limit notice or a
skipped-review notice, which crq classifies as evidence and surfaces as a finding. Deleting those
would quietly destroy feedback nobody had read yet — which is the one outcome worse than a noisy PR.

Where it runs

Automatically as rounds progress under crq autoreview (CRQ_TIDY=0 disables), hooked to the pump
result so it costs one observation on a PR crq was already looking at rather than a sweep of the
fleet. Housekeeping failures are logged and never break the pass that did the real work.

A limitation worth stating

Only commands crq still holds a round record for are candidates, and Archive is bounded — so
trigger comments from long-trimmed rounds stay. Widening it would mean matching on text, which is
exactly the step that risks deleting a human's comment, so it stays narrow.

A recorded command that is already gone counts as tidied rather than failing: the bot removes some of
its own command comments, and a person may have tidied by hand.

Verification

Exercised live on this repo. --dry-run first, then real: #48 lost six spent triggers while the live
round's own @coderabbitai review (18:42) and @codex review survived, along with every bot comment
and every human one. Across #50#55 it removed 16 and correctly reported "no progressed round has a
trigger comment to remove" for the PR that had none.

TestStaleCommandsKeepsWhatCrqStillReads covers each guard in the pure rule.
TestTidyRemovesOnlySpentCommandsCrqPosted stages a spent command, the live round's command, a
person's request and a bot reply, and asserts only the first goes.
TestTidyDryRunWritesNothing pins that a dry run reports the same decision and changes nothing.

gofmt -l . clean, go vet ./... clean, go test ./... -count=1 green.

Ref #42

Summary by CodeRabbit

  • New Features

    • Added crq tidy <repo> <pr> to remove spent review-trigger comments posted by crq.
    • Added --dry-run to preview comments that would be removed without making changes.
    • Automatic cleanup now runs during eligible autoreview workflows and can be disabled with CRQ_TIDY=0.
    • Cleanup preserves active commands, bot responses, and comments not posted by crq.
  • Documentation

    • Added CLI help and usage guidance for the tidy command and automatic cleanup behavior.

A PR driven through a dozen rounds collects a dozen "@coderabbitai
review" comments and a dozen acknowledgements, which buries the
conversation a human came to read. crq now removes its own half of that
as rounds progress, and `crq tidy` does it on demand.

Deleting a comment crq still reads is how this becomes expensive, so a
comment has to clear three guards. It belongs to a round that has
PROGRESSED — a live round keeps its command, because that is the comment
crq adopts instead of posting another. The bot acted after it, so it was
read rather than merely old. And it predates the current head, because
adoption only ever considers commands newer than the head commit; delete
one of those and the next pump posts a duplicate and buys a second
review.

Only comments crq posted. Candidates are the command IDs recorded on its
own rounds, never anything matching the text — a person's "@coderabbitai
review" is their decision to ask, not crq's to erase, and crq posts under
the same account so authorship alone cannot tell them apart.

Never the bots' own comments. An auto-generated reply can be a rate-limit
or skipped-review notice, which crq classifies as evidence and surfaces
as a finding, so deleting those would quietly destroy feedback nobody had
read yet.

A recorded command that is already gone counts as tidied rather than
failing: the bot removes some of its own command comments, and a person
may have tidied by hand.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@kristofferR, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 48 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c37d5b40-a997-46bb-afd6-866c1637b3e1

📥 Commits

Reviewing files that changed from the base of the PR and between b1c57ad and 75bffe7.

📒 Files selected for processing (5)
  • internal/crq/tidy.go
  • internal/engine/tidy.go
  • internal/engine/tidy_test.go
  • internal/state/spent_test.go
  • internal/state/state.go
📝 Walkthrough

Walkthrough

Adds stale review-trigger comment detection and deletion, with dry-run reporting, automatic autoreview cleanup, a new crq tidy CLI command, configuration control, tests, and documentation.

Changes

Tidy cleanup flow

Layer / File(s) Summary
Stale command detection
internal/engine/tidy.go, internal/engine/tidy_test.go
Defines tidy input types and identifies answered commands that are no longer live and precede the current head.
Service tidy execution
internal/crq/tidy.go, internal/crq/tidy_test.go
Collects command and activity timestamps, reports dry-run deletions, and performs best-effort cleanup of spent comments.
Automatic autoreview cleanup
internal/crq/config.go, internal/crq/auto.go
Adds CRQ_TIDY configuration and invokes cleanup after selected pump actions.
CLI and usage contract
cmd/crq/main.go, llms.txt, skills/coderabbit-queue/SKILL.md
Adds crq tidy <repo> <pr> [--dry-run] and documents its cleanup boundaries and configuration.
Estimated code review effort: 3 (Moderate) ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant ServiceTidy
  participant Engine
  participant GitHub
  CLI->>ServiceTidy: Tidy(repo, pr, dryRun)
  ServiceTidy->>GitHub: load rounds, comments, and PR activity
  ServiceTidy->>Engine: evaluate stale commands
  Engine-->>ServiceTidy: return spent comment IDs
  alt dry run
    ServiceTidy-->>CLI: return JSON deletion report
  else apply
    ServiceTidy->>GitHub: delete spent comments
    ServiceTidy-->>CLI: return JSON result
  end
Loading

Poem

A rabbit hops through rounds of review,
Sweeping old prompts from the view.
Dry-run paws point out what to clear,
Live notes and bot words stay near.
Tidy trails make the queue feel new!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title points to the main change: deleting trigger comments, even though the wording is awkward.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/tidy-consumed-commands

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1c57adf00

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/tidy.go
Comment on lines +135 to +136
if r.CommandID != 0 {
out = append(out, engine.CommandComment{ID: r.CommandID, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: at})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve command provenance before deletion

When DecideFire adopts a pre-existing trigger, including one posted manually by a person, the round stores its ID in the same CommandID; command discovery does not filter by author. This line therefore treats adopted comments as crq-owned, so after the round completes or is archived and later bot activity exists, tidy can delete a person's trigger comment. Record and require posted-versus-adopted provenance before making the ID a deletion candidate.

AGENTS.md reference: AGENTS.md:L83-L95

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
Comment on lines +195 to +196
switch res.Action {
case "deduped", "waiting", "requeued", "skipped":

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Tidy rounds that complete with cleared

A normal review that completes while holding the slot is returned by slotResult as Action: "cleared", but this whitelist ignores that action; completion in sweepReviewing is even less visible because Pump returns idle or the following fire instead of the completed round. Consequently the default autoreview path does not tidy trigger comments for the usual successful-round completion, leaving the advertised automatic cleanup ineffective unless another qualifying transition happens. Propagate the completed round or invoke cleanup at the completion transition.

AGENTS.md reference: AGENTS.md:L75-L77

Useful? React with 👍 / 👎.

Comment thread internal/engine/tidy.go Outdated
Comment on lines +53 to +54
if !in.HeadAt.IsZero() && !cmd.CreatedAt.Before(in.HeadAt) {
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep commands when the head timestamp is unavailable

If observe cannot fetch the current commit, it deliberately leaves HeadAt zero, but this condition then skips the head-age guard and permits deletion. In that transient/404 scenario tidy has not established the documented requirement that the command predates the current head, so it can remove a still-current trigger despite lacking the safety evidence; a zero HeadAt should conservatively make the command ineligible for deletion.

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
Comment on lines +106 to +107
for _, id := range stale {
err := s.gh.DeleteIssueComment(ctx, repo, id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Revalidate live ownership before deleting each comment

The candidate and Live sets come from a state snapshot loaded before the network observation, and there is no state recheck before this destructive call. If another fleet process or a concurrent manual command adopts one of those recorded IDs into a new active round during that window, tidy can delete the command that the new round now depends on; subsequent progress may then fail while fetching reactions for the deleted comment. Reload and verify that the ID is still absent from every active round immediately before deletion, ideally under a state claim/CAS.

AGENTS.md reference: AGENTS.md:L3-L5

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
Comment on lines +135 to +136
if r.CommandID != 0 {
out = append(out, engine.CommandComment{ID: r.CommandID, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: at})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retain every retry trigger ID for cleanup

This collects only the round's current CommandID, but a rate-limited or timed-out round remains active through awaiting_retry and Round.Fire overwrites that field when the retry posts its next trigger. The earlier ID is therefore neither archived nor available to tidy, so repeated attempts on one head permanently leave all but the last command behind—the exact conversation buildup this feature is meant to remove. Persist command history per round and collect every crq-posted attempt.

AGENTS.md reference: AGENTS.md:L66-L73

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
Comment on lines +135 to +136
if r.CommandID != 0 {
out = append(out, engine.CommandComment{ID: r.CommandID, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: at})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid treating a co-only anchor as a primary command

For fireCoOnly, the first co-reviewer trigger is stored both as Round.CommandID and as that bot's CoBots[login].CommandID, with CoOnly set. This branch appends the shared ID as if CodeRabbit owned it, and the following loop appends it again for the actual co-reviewer; consequently unrelated later CodeRabbit activity can satisfy the answer guard for an unanswered co-reviewer command, while a normally answered command is deleted twice and reported twice because the second 404 is counted as success. Skip the primary entry for co-only anchors and deduplicate IDs.

AGENTS.md reference: AGENTS.md:L125-L127

Useful? React with 👍 / 👎.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/engine/tidy_test.go`:
- Around line 11-63: Replace the separate tests in
TestStaleCommandsKeepsWhatCrqStillReads,
TestStaleCommandsNeverTouchesALiveRound, and
TestStaleCommandsWithoutAHeadStillRequiresEvidence with one table-driven test
covering live, unanswered, head-cutoff/adoption, and stale-command outcomes.
Give each case its own TidyInput and expected stale IDs, then run them through a
subtest loop while preserving the existing behavior expectations.

In `@internal/engine/tidy.go`:
- Around line 53-54: Retain candidates when the current-head timestamp is
unavailable: update the stale-command filtering around the HeadAt guard in
internal/engine/tidy.go lines 53-54 so a zero HeadAt produces no stale commands
rather than skipping the candidate. Update the corresponding expectation in
internal/engine/tidy_test.go lines 52-62 to verify the command is retained.

In `@llms.txt`:
- Around line 212-213: Update the user-facing command-removal contracts in
llms.txt (lines 212-213) and skills/coderabbit-queue/SKILL.md (lines 176-177) to
state that the command must predate the current head when that timestamp is
known, preserving the existing recorded, progressed, answered, and non-bot
conditions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: df17a142-421e-4d07-b2c0-5a7d4cc03d5e

📥 Commits

Reviewing files that changed from the base of the PR and between 081a791 and b1c57ad.

📒 Files selected for processing (9)
  • cmd/crq/main.go
  • internal/crq/auto.go
  • internal/crq/config.go
  • internal/crq/tidy.go
  • internal/crq/tidy_test.go
  • internal/engine/tidy.go
  • internal/engine/tidy_test.go
  • llms.txt
  • skills/coderabbit-queue/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
internal/engine/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

internal/engine/**/*.go: Keep internal/engine pure: decision logic must receive now, perform no I/O, context, or GitHub access, and table-test every rule.
DecideFire must be the single fire owner and evaluate guards in order: open, readable/current head, eligible phase, primary review unavailable, already reviewed, free slot, account quota, minimum interval, then adopt/post.
Keep convergence, fire, and co-reviewer rules in internal/engine; keep orchestration in internal/crq, and table-test engine rules.

Files:

  • internal/engine/tidy_test.go
  • internal/engine/tidy.go
internal/crq/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Keep internal/crq limited to orchestration and effects wiring; service.go is the only effects executor for CAS writes and PostIssueComment, and DryRun must write nothing.

Files:

  • internal/crq/config.go
  • internal/crq/auto.go
  • internal/crq/tidy_test.go
  • internal/crq/tidy.go
internal/crq/config.go

📄 CodeRabbit inference engine (AGENTS.md)

Add a default entry in parseCoBots when a new co-reviewer should be enabled by default.

Files:

  • internal/crq/config.go
🔇 Additional comments (3)
cmd/crq/main.go (3)

227-248: LGTM!


381-381: LGTM!


535-559: LGTM!

Comment on lines +11 to +63
func TestStaleCommandsKeepsWhatCrqStillReads(t *testing.T) {
base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC)
head := base.Add(30 * time.Minute)
answered := map[string]time.Time{"coderabbitai": base.Add(40 * time.Minute)}

in := TidyInput{
HeadAt: head,
AnsweredAt: answered,
Live: map[int64]bool{5: true},
Commands: []CommandComment{
{ID: 1, Bot: "coderabbitai", CreatedAt: base}, // stale: answered, old, not live
{ID: 5, Bot: "coderabbitai", CreatedAt: base.Add(time.Minute)}, // live round depends on it
{ID: 6, Bot: "codex", CreatedAt: base.Add(2 * time.Minute)}, // no evidence this bot ever acted
{ID: 7, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}, // newer than the head: still adoptable
{ID: 8, Bot: "coderabbitai", CreatedAt: base.Add(50 * time.Minute)}, // after the answer AND after the head
},
}

got := StaleCommands(in)
if len(got) != 1 || got[0] != 1 {
t.Fatalf("stale = %v, want only the answered, superseded, non-live command", got)
}
}

// A round that has not progressed keeps its command, whatever else is true.
func TestStaleCommandsNeverTouchesALiveRound(t *testing.T) {
base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC)
in := TidyInput{
HeadAt: base.Add(time.Hour),
AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(2 * time.Hour)},
Live: map[int64]bool{1: true, 2: true},
Commands: []CommandComment{
{ID: 1, Bot: "coderabbitai", CreatedAt: base},
{ID: 2, Bot: "codex", CreatedAt: base},
},
}
if got := StaleCommands(in); len(got) != 0 {
t.Errorf("stale = %v, want nothing while the round is live", got)
}
}

// With no head timestamp, the adoption guard cannot be evaluated, so it must not
// silently pass: an unreadable head is not permission to delete.
func TestStaleCommandsWithoutAHeadStillRequiresEvidence(t *testing.T) {
base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC)
in := TidyInput{
AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(time.Hour)},
Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}},
}
if got := StaleCommands(in); len(got) != 1 {
t.Errorf("stale = %v, want the answered command", got)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Table-test every engine rule.

These separate tests do not satisfy the internal/engine requirement to table-test every rule. Use a case table for live, unanswered, head-cutoff, and stale-command outcomes.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/tidy_test.go` around lines 11 - 63, Replace the separate
tests in TestStaleCommandsKeepsWhatCrqStillReads,
TestStaleCommandsNeverTouchesALiveRound, and
TestStaleCommandsWithoutAHeadStillRequiresEvidence with one table-driven test
covering live, unanswered, head-cutoff/adoption, and stale-command outcomes.
Give each case its own TidyInput and expected stale IDs, then run them through a
subtest loop while preserving the existing behavior expectations.

Source: Coding guidelines

Comment thread internal/engine/tidy.go Outdated
Comment thread llms.txt
Comment on lines +212 to +213
Only comments crq posted, only from rounds that have progressed, only after the bot answered, and
never the bots' own comments.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document the current-head guard in both user-facing contracts.

The tidy implementation removes a command only when it is recorded, from a progressed round, answered, and older than the current head when that timestamp is available. Both documents currently omit the final condition.

  • llms.txt#L212-L213: add that the command must predate the current head when known.
  • skills/coderabbit-queue/SKILL.md#L176-L177: add the same current-head requirement.
📍 Affects 2 files
  • llms.txt#L212-L213 (this comment)
  • skills/coderabbit-queue/SKILL.md#L176-L177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@llms.txt` around lines 212 - 213, Update the user-facing command-removal
contracts in llms.txt (lines 212-213) and skills/coderabbit-queue/SKILL.md
(lines 176-177) to state that the command must predate the current head when
that timestamp is known, preserving the existing recorded, progressed, answered,
and non-bot conditions.

A rate-limited round retries, and every retry posts a NEW
"@coderabbitai review" — the bot answered the previous one with the
rate-limit notice, so it can never be adopted again. crq overwrote
CommandID and forgot the old comment, which is why a throttled PR
collects a column of identical requests: nothing knew they were there,
and tidying could only ever find commands from rounds that had finished.

A round now records the commands it supersedes, bounded so a PR that
retries all day cannot grow its round without limit, and tidy treats them
as spent whichever phase the round is in. They are exempt from the
predates-the-head check on purpose: crq's own record that it has posted a
successor is stronger evidence than any timestamp.

Observed on PR #50 — five heads, nine review requests, three of them for
the first head alone. Codex is not affected: it gets one command per head
because crq skips the trigger when the bot has already reviewed it.
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75bffe7186

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/tidy.go
Comment on lines +147 to +148
if r.CommandID != 0 {
out = append(out, engine.CommandComment{ID: r.CommandID, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: at})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Exclude adopted comments from deletion

When a round adopted an existing trigger, Round.CommandID still contains that comment ID—Fire explicitly records both posted and adopted commands, and newestCommandSince does not retain the author. Treating every recorded ID here as crq-owned means that after the head advances and the bot answers, Tidy can delete a person's manually posted trigger, especially when crq runs with a maintainer token. Persist whether the command was actually posted by crq, or verify ownership before adding it as a deletion candidate.

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
Comment on lines +70 to +74
for _, id := range r.SpentCommands {
superseded[id] = true
commands = append(commands, engine.CommandComment{
ID: id, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: r.EnqueuedAt,
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the reviewer on spent command records

SpentCommands is populated by both primary Fire calls and co-reviewer SetCoCommand calls, but every spent ID is reconstructed here as belonging to cfg.Bot. In a co-only or primary-unavailable round, Codex/Bugbot can answer a superseded trigger while CodeRabbit never answers, so the intended cleanup retains that comment forever; conversely, a later unrelated CodeRabbit event can incorrectly qualify it for deletion. Store the bot identity alongside each spent command rather than discarding it.

AGENTS.md reference: AGENTS.md:L136-L139

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
return result, nil
}

obs, err := s.observe(ctx, repo, pr, nil, s.clock())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Count Codex reactions as answers

When Codex answers a clean round only with its supported thumbs-up reaction, this observe call passes no round, so the reaction-fetch branch in observe.go is skipped; answeredAt consequently sees no Codex activity and never removes the spent @codex review trigger. This affects both automatic and on-demand cleanup for clean Codex rounds, so tidy needs to inspect reactions for the recorded command IDs or otherwise carry that completion evidence.

AGENTS.md reference: AGENTS.md:L45-L46

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
Comment on lines +207 to +212
switch res.Action {
case "deduped", "waiting", "requeued", "skipped":
// The round reached a state where its earlier commands are answered and
// spent. "fired" is not here: that round is live and owns its command.
default:
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Run tidy after completed rounds

In the inspected autoreview path, a normal slot acknowledgement or completion is returned as Action: "cleared" by slotResult, while completion in sweepReviewing is followed by an idle or unrelated pump result. Because neither action reaches Tidy here, archived commands are cleaned only if a later pass happens to return waiting, requeued, or deduped for the same PR; a fast subsequent review or a PR with no further round leaves its eligible old triggers indefinitely despite tidy being enabled by default.

Useful? React with 👍 / 👎.

Comment thread internal/engine/tidy.go
Comment on lines +58 to +59
if !in.Superseded[cmd.ID] && !in.HeadAt.IsZero() && !cmd.CreatedAt.Before(in.HeadAt) {
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep commands when the head timestamp is unavailable

When GetCommit fails, observe deliberately leaves HeadAt zero, but this condition then skips the adoption guard instead of conservatively retaining the command. During a head transition, an answered command from a progressed round can therefore be deleted while the head timestamp is temporarily unreadable even though it may still be adoptable once the read recovers, causing the next pump to post and spend quota on a duplicate trigger. A missing cutoff should keep non-superseded commands rather than qualify all of them as stale.

Useful? React with 👍 / 👎.

Comment thread internal/crq/tidy.go
Comment on lines +118 to +125
for _, id := range stale {
err := s.gh.DeleteIssueComment(ctx, repo, id)
switch {
case err == nil, errors.Is(err, ghapi.ErrNotFound):
// Already gone is the outcome we wanted. A recorded command can
// vanish without crq: the bot removes some of its own command
// comments, and a person may tidy by hand.
result.Deleted = append(result.Deleted, id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Stop retrying comments already deleted

After a successful deletion, the command ID remains in the round/archive and is not filtered against the issue comments returned by observe, so every later qualifying tidy pass selects it again. The automatic waiting path can therefore issue a DELETE for each already-removed comment on every autoreview poll; the resulting 404 is counted as another successful deletion, repeatedly consuming GitHub requests and producing misleading Deleted results. Remove or tombstone successful IDs, or skip candidates absent from the observed comment set.

Useful? React with 👍 / 👎.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant