fix(auto-closer): fix comment-before-close, missing pagination, merge and test bugs#87
Conversation
📝 WalkthroughWalkthroughThis pull request modifies the auto-closer configuration and logic across three files. It changes how DryRun is propagated in configuration merging, updates the hasHumanActivity function to paginate through all comment pages, and reorders the closeIssue operation sequence to close issues before managing labels and posting comments. Additionally, test coverage is refactored to use table-driven testing for result count validation. Changes
Sequence DiagramsequenceDiagram
participant Client as Caller
participant Closer as closeIssue()
participant IssueSvc as Issue Service
participant LabelSvc as Label Service
participant CommentSvc as Comment Service
Client->>Closer: closeIssue(issue, labels, comment)
Closer->>IssueSvc: Close issue
IssueSvc-->>Closer: Close successful
Closer->>LabelSvc: Remove "potential-duplicate" label
LabelSvc-->>Closer: Label removed
Closer->>LabelSvc: Add "duplicate" label
LabelSvc-->>Closer: Label added
Closer->>CommentSvc: Post closing comment
CommentSvc-->>Closer: Comment posted / Error
alt Comment Success
Closer-->>Client: Return nil (success)
else Comment Failure
Closer-->>Client: Log warning, return nil
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
… and test bugs - closeIssue: post closing comment AFTER issue is confirmed closed to prevent ghost "Auto-Closed" comments on issues that failed to close - hasHumanActivity: paginate through all comment pages so human activity beyond the first 100 comments is never silently missed - mergeConfigs: always copy AutoClose.DryRun from child so a child config can explicitly override a parent's dry_run: true back to false - TestAutoCloseResultCounts: include error count in the total so the assertion holds for runs that encounter partial failures Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
Kavirubc
force-pushed
the
fix/auto-closer-bugs
branch
from
aecc0fc to
40a2968
Compare
Kavirubc
requested review from
Sachindu-Nethmin and
Copilot
and removed request for
Sachindu-Nethmin
There was a problem hiding this comment.
Pull request overview
This pull request addresses four bugs in the auto-closer functionality for duplicate issues: (1) reordering operations in closeIssue to prevent misleading comments on failed closes, (2) adding pagination to hasHumanActivity to avoid missing comments beyond the first 100, (3) updating the config merge logic for DryRun to allow child configs to override parent settings, and (4) fixing test assertions to properly account for error cases.
Changes:
- Reordered
closeIssueto close issue first, then swap labels, then comment (prevents "closed" comments on still-open issues if close fails) - Added full pagination loop to
hasHumanActivityto check all comments, not just the first 100 - Modified
mergeConfigsto always copyDryRunfrom child config - Updated
TestAutoCloseResultCountsto include error count in total validation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/steps/auto_closer.go | Reordered close operation sequence and added comment pagination loop |
| internal/core/config/config.go | Changed DryRun merge logic to always copy from child config |
| internal/steps/auto_closer_test.go | Enhanced test to validate error counting and updated author attribution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if child.AutoClose.DryRun { | ||
| result.AutoClose.DryRun = child.AutoClose.DryRun | ||
| } | ||
| result.AutoClose.DryRun = child.AutoClose.DryRun |
There was a problem hiding this comment.
The unconditional copy of DryRun will cause unintended behavior when a child config doesn't explicitly set dry_run in YAML. Since DryRun is a plain bool (not *bool), it defaults to false when not specified. This means a child config that doesn't mention dry_run will always override a parent's dry_run: true setting back to false, which is likely not the intended behavior.
To properly support overriding a parent's dry_run: true with dry_run: false, the field should be changed to *bool type (like Transfer.Enabled and other boolean config fields that need to distinguish between "not set" and "explicitly set to false"). Then the merge logic should check if child.AutoClose.DryRun != nil before copying.
Alternatively, if the current behavior is intentional and child configs are always expected to explicitly specify dry_run, this should be documented clearly.
🧪 E2E Test✅ Bot responded: yes Test repo → gh-simili-bot/simili-e2e-22379609803 Auto-generated by E2E pipeline |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/config/config.go`:
- Around line 393-398: The AutoClose.DryRun field must be changed from bool to
*bool (update the AutoCloseConfig type) so we can distinguish "unset" vs
"explicit false"; in mergeConfigs (where result.AutoClose.DryRun is currently
unconditionally assigned from child.AutoClose.DryRun) only copy the value when
child.AutoClose.DryRun != nil, leaving the parent value intact otherwise; update
NewAutoCloser (auto_closer.go) and any other code that reads
cfg.AutoClose.DryRun to handle the nil case (nil means inherit/absent — use the
parent/default behavior or explicitly coerce to false where intended).
In `@internal/steps/auto_closer.go`:
- Around line 252-255: The verbose log can panic when IssueComment.CreatedAt is
nil because the code accesses comment.CreatedAt.Time directly; replace that
access with the nil-safe accessor by calling comment.GetCreatedAt() and using
its .Time (e.g., createdAt := comment.GetCreatedAt(); use createdAt.Time in the
log) or guard with an explicit nil check before logging; update the log in the
auto-closer block where ac.verbose is checked to use comment.GetCreatedAt() to
avoid the nil pointer dereference.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/core/config/config.gointernal/steps/auto_closer.gointernal/steps/auto_closer_test.go
| // AutoClose: override if fields are set. | ||
| // DryRun is always copied so a child config can explicitly set it to false. | ||
| if child.AutoClose.GracePeriodHours != 0 { | ||
| result.AutoClose.GracePeriodHours = child.AutoClose.GracePeriodHours | ||
| } | ||
| if child.AutoClose.DryRun { | ||
| result.AutoClose.DryRun = child.AutoClose.DryRun | ||
| } | ||
| result.AutoClose.DryRun = child.AutoClose.DryRun |
There was a problem hiding this comment.
DryRun bool can't distinguish "child omitted it" from "child explicitly set it to false"
yaml:"dry_run,omitempty" means a child config that simply doesn't mention dry_run will have child.AutoClose.DryRun == false after unmarshal — identical to one that explicitly writes dry_run: false. The unconditional assignment therefore silently overrides a parent's dry_run: true whenever the child is silent, defeating its safety-guard purpose.
Every other tri-state boolean in this file uses *bool for exactly this reason (e.g. CrossRepoSearch, Transfer.Enabled, Transfer.LLMRoutingEnabled).
🛡️ Proposed fix — use *bool for DryRun
In AutoCloseConfig:
-DryRun bool `yaml:"dry_run,omitempty"` // If true, log actions without executing
+DryRun *bool `yaml:"dry_run,omitempty"` // If true, log actions without executingIn mergeConfigs:
-result.AutoClose.DryRun = child.AutoClose.DryRun
+if child.AutoClose.DryRun != nil {
+ result.AutoClose.DryRun = child.AutoClose.DryRun
+}In NewAutoCloser (auto_closer.go):
-dryRun: dryRun || cfg.AutoClose.DryRun,
+dryRun: dryRun || (cfg.AutoClose.DryRun != nil && *cfg.AutoClose.DryRun),Any read of cfg.AutoClose.DryRun elsewhere would likewise need a nil-guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config/config.go` around lines 393 - 398, The AutoClose.DryRun
field must be changed from bool to *bool (update the AutoCloseConfig type) so we
can distinguish "unset" vs "explicit false"; in mergeConfigs (where
result.AutoClose.DryRun is currently unconditionally assigned from
child.AutoClose.DryRun) only copy the value when child.AutoClose.DryRun != nil,
leaving the parent value intact otherwise; update NewAutoCloser (auto_closer.go)
and any other code that reads cfg.AutoClose.DryRun to handle the nil case (nil
means inherit/absent — use the parent/default behavior or explicitly coerce to
false where intended).
| if ac.verbose { | ||
| log.Printf("[auto-closer] #%d: human comment by %q at %v", | ||
| number, author, comment.CreatedAt.Time) | ||
| } |
There was a problem hiding this comment.
Nil pointer dereference on comment.CreatedAt.Time in verbose log
IssueComment.CreatedAt is *github.Timestamp; if the field is nil, accessing .Time panics. Although the GitHub API reliably populates created_at, defensive access in a hot loop is worth the one-liner cost.
🛡️ Proposed fix
- if ac.verbose {
- log.Printf("[auto-closer] #%d: human comment by %q at %v",
- number, author, comment.CreatedAt.Time)
- }
+ if ac.verbose {
+ log.Printf("[auto-closer] #%d: human comment by %q at %v",
+ number, author, comment.GetCreatedAt().Time)
+ }GetCreatedAt() returns a zero-value Timestamp instead of panicking when the field is nil.
#!/bin/bash
# Verify that IssueComment.CreatedAt is a pointer type in go-github v60
# and that GetCreatedAt() is a safe nil-aware accessor.
rg -n "CreatedAt" --type go -C2 | grep -A2 "IssueComment"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/steps/auto_closer.go` around lines 252 - 255, The verbose log can
panic when IssueComment.CreatedAt is nil because the code accesses
comment.CreatedAt.Time directly; replace that access with the nil-safe accessor
by calling comment.GetCreatedAt() and using its .Time (e.g., createdAt :=
comment.GetCreatedAt(); use createdAt.Time in the log) or guard with an explicit
nil check before logging; update the log in the auto-closer block where
ac.verbose is checked to use comment.GetCreatedAt() to avoid the nil pointer
dereference.
2 participants
Summary
closeIssue: moved closing comment to after the issue is confirmed closed — previously a failedCloseIssuecall would leave a misleading "Auto-Closed as Duplicate" comment on a still-open issuehasHumanActivity: added full pagination loop so comments beyond the first 100 are checked — previously human activity on high-traffic issues could be silently missed, causing legitimate issues to be wrongly auto-closedmergeConfigs:AutoClose.DryRunis now always copied from the child config, allowing a child to explicitly override a parent'sdry_run: trueback tofalseTestAutoCloseResultCounts: updated assertion to includelen(Errors)in the total so the test accurately reflects real runs where some issues hit error pathsTest plan
go build ./...passes cleanlygo test ./internal/steps/... ./internal/core/config/...passescloseIssue: close → swap labels → commenthasHumanActivitymirrors pattern used infetchPotentialDuplicatesandListIssueEvents🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Tests