Conversation
📝 WalkthroughWalkthroughAdds a GitHub client method to list reactions on issue comments and extends the auto-closer to detect human activity via three signals (negative reactions on the bot's triage comment, human reopen events, and human comments) with paginated comment fetching and non-fatal reaction errors. Changes
Sequence DiagramsequenceDiagram
participant AC as Auto-closer
participant CH as GitHub Client
participant API as GitHub API
AC->>CH: fetchAllComments(ctx, org, repo, issueNum)
CH->>API: List issue comments (paginated)
API-->>CH: Comments[]
CH-->>AC: Comments[]
AC->>AC: Find bot triage comment
AC->>CH: ListIssueCommentReactions(ctx, org, repo, commentID)
CH->>API: List reactions (paginated, 100/page)
API-->>CH: Reactions[]
CH-->>AC: Reactions[]
AC->>CH: List issue events (paginated)
CH->>API: List events
API-->>CH: Events[]
CH-->>AC: Events[]
AC->>AC: Evaluate signals (negative reactions, reopen by human, human comments)
alt Any signal detected
AC->>AC: Skip closing (human activity or in-grace)
else No signals
AC->>AC: Proceed to close issue
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Possibly Related PRs
Suggested labels
Suggested reviewers
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 |
Enhance hasHumanActivity() with two additional signals beyond new comments: - Check A: negative reactions (👎 -1 or 😕 confused) by a non-bot user on the bot's triage comment (identified via isBotComment marker) - Check B: issue reopened by a human after the potential-duplicate label was applied, detected via ListIssueEvents Add ListIssueCommentReactions to the GitHub client (paginated). Add unit tests: TestIsBotComment, TestHasNegativeReaction, TestReopenedByHuman. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
Kavirubc
force-pushed
the
feature/delayed-actions-and-human-in-the-loop
branch
from
ead2dca to
722dd07
Compare
🧪 E2E Test✅ Bot responded: yes Test repo → gh-simili-bot/simili-e2e-22384346410 Auto-generated by E2E pipeline |
There was a problem hiding this comment.
Pull request overview
This pull request expands the auto-closer's human activity detection to address issues #54 and #55. The auto-closer automatically closes issues labeled as "potential-duplicate" after a grace period, but only if no human activity is detected. This PR adds two new detection mechanisms: checking for negative reactions on the bot's triage comment, and detecting when a human reopens an issue after the label was applied.
Changes:
- Added
ListIssueCommentReactionsmethod to GitHub client for paginated reaction retrieval - Enhanced
hasHumanActivityto check for negative reactions (👎, 😕) on bot's triage comment - Enhanced
hasHumanActivityto detect human-initiated issue reopens after labeling - Added unit tests for
isBotComment, reaction detection logic, and reopen detection logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/integrations/github/client.go | Adds paginated ListIssueCommentReactions method following existing client patterns |
| internal/steps/auto_closer.go | Expands hasHumanActivity with reaction check (Check A) and reopen check (Check B); adds fetchAllComments helper |
| internal/steps/auto_closer_test.go | Adds three new table-driven tests: TestIsBotComment, TestHasNegativeReaction, TestReopenedByHuman |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestHasNegativeReaction(t *testing.T) { | ||
| botUsers := []string{"my-ci-bot"} | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| content string | ||
| user string | ||
| wantSkip bool // true means this reaction should trigger human activity | ||
| }{ | ||
| {"thumbs down from human", "-1", "john-doe", true}, | ||
| {"confused from human", "confused", "jane-doe", true}, | ||
| {"thumbs up from human", "+1", "john-doe", false}, | ||
| {"heart from human", "heart", "john-doe", false}, | ||
| {"thumbs down from bot", "-1", "dependabot[bot]", false}, | ||
| {"confused from bot", "confused", "gh-simili-worker", false}, | ||
| {"thumbs down from configured bot", "-1", "my-ci-bot", false}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| isNegative := (tt.content == "-1" || tt.content == "confused") | ||
| isHuman := !isBotUser(tt.user, botUsers) | ||
| got := isNegative && isHuman | ||
| if got != tt.wantSkip { | ||
| t.Errorf("reaction check: content=%q user=%q => %v, want %v", tt.content, tt.user, got, tt.wantSkip) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestReopenedByHuman(t *testing.T) { | ||
| botUsers := []string{} | ||
| since := time.Now().Add(-48 * time.Hour) | ||
|
|
||
| type eventInput struct { | ||
| eventType string | ||
| actor string | ||
| createdAt time.Time | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| event eventInput | ||
| want bool | ||
| }{ | ||
| { | ||
| name: "reopened by human after since", | ||
| event: eventInput{"reopened", "john-doe", since.Add(time.Hour)}, | ||
| want: true, | ||
| }, | ||
| { | ||
| name: "reopened by bot after since", | ||
| event: eventInput{"reopened", "dependabot[bot]", since.Add(time.Hour)}, | ||
| want: false, | ||
| }, | ||
| { | ||
| name: "reopened by human before since", | ||
| event: eventInput{"reopened", "john-doe", since.Add(-time.Hour)}, | ||
| want: false, | ||
| }, | ||
| { | ||
| name: "closed by human after since", | ||
| event: eventInput{"closed", "john-doe", since.Add(time.Hour)}, | ||
| want: false, | ||
| }, | ||
| { | ||
| name: "labeled by human after since", | ||
| event: eventInput{"labeled", "john-doe", since.Add(time.Hour)}, | ||
| want: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| e := tt.event | ||
| isReopened := e.eventType == "reopened" | ||
| isAfterSince := e.createdAt.After(since) | ||
| isHuman := !isBotUser(e.actor, botUsers) | ||
| got := isReopened && isAfterSince && isHuman | ||
| if got != tt.want { | ||
| t.Errorf("reopen check: event=%q actor=%q at=%v => %v, want %v", | ||
| e.eventType, e.actor, e.createdAt, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests TestHasNegativeReaction and TestReopenedByHuman replicate the logic inline rather than testing the actual implementation in hasHumanActivity. These tests verify that the test logic itself is correct, but they don't validate the actual production code. Consider refactoring these as integration tests that call hasHumanActivity with mock data, or extract the reaction-checking and reopen-checking logic into separate testable functions that can be unit tested independently.
| for _, r := range reactions { | ||
| content := r.GetContent() | ||
| user := "" | ||
| if r.User != nil { | ||
| user = r.User.GetLogin() | ||
| } | ||
| if (content == "-1" || content == "confused") && !isBotUser(user, ac.cfg.BotUsers) { | ||
| if ac.verbose { | ||
| log.Printf("[auto-closer] #%d: negative reaction %q by %q on triage comment", number, content, user) | ||
| } | ||
| return true, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Check A does not verify when reactions were added to the triage comment. If a user added a negative reaction before the potential-duplicate label was applied (i.e., before 'since'), it would still be counted as human activity. This could incorrectly prevent auto-closing of issues. Consider adding a timestamp check for reactions, similar to how Check B filters events by CreatedAt. However, note that the GitHub API may not provide reaction timestamps directly, which could make this difficult to implement correctly.
internal/steps/auto_closer.go
Outdated
| // hasHumanActivity checks for any of these signals since the issue was labeled: | ||
| // 1. Negative reactions (👎 or 😕) on the bot's triage comment by a non-bot user |
There was a problem hiding this comment.
The documentation states that hasHumanActivity checks for signals "since the issue was labeled," but Check 1 (negative reactions) doesn't verify when reactions were added. The comment should either clarify that negative reactions are checked regardless of timing, or the implementation should filter reactions by CreatedAt timestamp to match the documented behavior for checks 2 and 3.
| // hasHumanActivity checks for any of these signals since the issue was labeled: | |
| // 1. Negative reactions (👎 or 😕) on the bot's triage comment by a non-bot user | |
| // hasHumanActivity checks for the following signals related to the issue's lifecycle: | |
| // 1. Negative reactions (👎 or 😕) on the bot's triage comment by a non-bot user (regardless of when the reaction was added) |
|
|
||
| for _, comment := range allComments { | ||
| if comment.User == nil || comment.Body == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Check A fetches all comments on the issue without any time filter, which is inefficient for high-traffic issues. Since the bot's triage comment is typically posted around the time the label is applied (or before), consider adding a time-based optimization. For example, you could fetch comments with a Since parameter set to a reasonable time before the label was applied (e.g., since.Add(-24*time.Hour)) to reduce the number of comments retrieved while still ensuring the triage comment is captured.
| for _, comment := range allComments { | |
| if comment.User == nil || comment.Body == nil { | |
| continue | |
| } | |
| // Optimization: only consider comments around the time the label was applied. | |
| // We look back 24 hours before 'since' to ensure we still capture the triage comment. | |
| windowStart := since.Add(-24 * time.Hour) | |
| for _, comment := range allComments { | |
| if comment.User == nil || comment.Body == nil { | |
| continue | |
| } | |
| createdAt := comment.GetCreatedAt() | |
| if !createdAt.IsZero() && createdAt.Before(windowStart) { | |
| // Comment is too old to be relevant for triage-related activity. | |
| continue | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/steps/auto_closer.go (1)
235-323:⚠️ Potential issue | 🟠 Major
sinceis not consistently enforced in activity detection.The function advertises “since labeled” checks, but Check A has no time gate and Check C does not explicitly enforce
CreatedAt > since. This can classify older activity as current and skip closures incorrectly.Suggested fix
for _, comment := range allComments { if comment.User == nil || comment.Body == nil { continue } + if comment.CreatedAt == nil || !comment.CreatedAt.Time.After(since) { + continue + } if !isBotUser(comment.User.GetLogin(), ac.cfg.BotUsers) { continue } @@ for { comments, resp, err := ac.github.ListComments(ctx, org, repo, number, opts) if err != nil { return false, err } for _, comment := range comments { - if comment.User == nil { + if comment.User == nil || comment.CreatedAt == nil || !comment.CreatedAt.Time.After(since) { continue } author := comment.User.GetLogin()🤖 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 235 - 323, Check A and C aren't enforcing the labeled-at time gate: in the loop that scans bot triage comments from fetchAllComments (the block that calls ac.github.ListIssueCommentReactions) only treat a comment as the triage comment if its CreatedAt exists and comment.CreatedAt.Time.After(since); likewise, in the Check C loop over ListComments add an explicit guard that comment.CreatedAt != nil and comment.CreatedAt.Time.After(since) before treating it as recent human activity (don’t rely solely on the ListComments Since filter). Update the checks in auto_closer.go around fetchAllComments/ListIssueCommentReactions and ListComments to apply these CreatedAt time comparisons.
🧹 Nitpick comments (1)
internal/steps/auto_closer_test.go (1)
105-190: Consider behavior-level tests forhasHumanActivityinstead of predicate mirroring.These cases currently re-derive the boolean logic locally, so they won’t catch regressions in the actual method’s control flow and API interaction paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/steps/auto_closer_test.go` around lines 105 - 190, The tests TestHasNegativeReaction and TestReopenedByHuman duplicate the predicate logic instead of exercising the real implementation; replace these predicate-mirroring checks with behavior-level tests that call hasHumanActivity (and any helper that wraps it) using controlled inputs/mocks for events, timestamps, and botUsers so you exercise its control flow and API interactions; specifically, update the tests to invoke hasHumanActivity with sample reactions/events (referencing hasHumanActivity and isBotUser) and assert the returned result, rather than recomputing isNegative/isReopened/isAfterSince/isHuman locally.
🤖 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/integrations/github/client.go`:
- Around line 226-229: The code unconditionally accesses resp.NextPage; guard
against a nil resp before dereferencing by checking if resp == nil and handling
that case (e.g., break the pagination loop or return an error) before the
existing lines that reference resp.NextPage; update the block around where resp
and opts are used (the pagination loop that sets opts.Page) so it first does "if
resp == nil { break }" or similar safe handling to avoid a nil-pointer panic.
---
Outside diff comments:
In `@internal/steps/auto_closer.go`:
- Around line 235-323: Check A and C aren't enforcing the labeled-at time gate:
in the loop that scans bot triage comments from fetchAllComments (the block that
calls ac.github.ListIssueCommentReactions) only treat a comment as the triage
comment if its CreatedAt exists and comment.CreatedAt.Time.After(since);
likewise, in the Check C loop over ListComments add an explicit guard that
comment.CreatedAt != nil and comment.CreatedAt.Time.After(since) before treating
it as recent human activity (don’t rely solely on the ListComments Since
filter). Update the checks in auto_closer.go around
fetchAllComments/ListIssueCommentReactions and ListComments to apply these
CreatedAt time comparisons.
---
Nitpick comments:
In `@internal/steps/auto_closer_test.go`:
- Around line 105-190: The tests TestHasNegativeReaction and TestReopenedByHuman
duplicate the predicate logic instead of exercising the real implementation;
replace these predicate-mirroring checks with behavior-level tests that call
hasHumanActivity (and any helper that wraps it) using controlled inputs/mocks
for events, timestamps, and botUsers so you exercise its control flow and API
interactions; specifically, update the tests to invoke hasHumanActivity with
sample reactions/events (referencing hasHumanActivity and isBotUser) and assert
the returned result, rather than recomputing
isNegative/isReopened/isAfterSince/isHuman locally.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/integrations/github/client.gointernal/steps/auto_closer.gointernal/steps/auto_closer_test.go
…E coverage Fixes from CodeRabbit / Copilot review on PR #88: - client.go: guard resp == nil before resp.NextPage in ListIssueCommentReactions to prevent a nil-pointer panic when the GitHub API returns no pagination header - auto_closer.go: update hasHumanActivity doc comment to clarify that reaction timestamps are unavailable via the REST API (GitHub does not expose them), so Check A cannot filter reactions by time — document this explicitly - auto_closer.go: add a 24-hour look-back window when scanning for the bot triage comment in Check A, avoiding a full comment-history scan on busy issues New: auto-close CLI command (cmd/simili/commands/auto_close.go) The auto-close.yml workflow already referenced `simili auto-close` but the subcommand was missing. Adds it with --repo, --dry-run, --grace-period-minutes flags; the last flag enables instant-expiry (≥0 → 1 min internally) for E2E and local testing without waiting 72 hours. New: auto-closer E2E coverage in .github/workflows/e2e-test.yml - Step 7b: creates a dedicated AC test issue and immediately labels it `potential-duplicate` so the grace period clock starts before the main triage test runs (~6-8 min total before the auto-closer step executes). - Step 11b: runs `simili auto-close --dry-run --grace-period-minutes 1` against the test repo, parses the JSON output with jq, and verifies the command exits cleanly and reports processed issues. Results (processed/closed/grace/human) are surfaced in the PR comment and job summary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/steps/auto_closer.go (1)
324-335:⚠️ Potential issue | 🟡 MinorPotential nil-pointer dereference on
comment.CreatedAt.Line 332 accesses
comment.CreatedAt.Timebut there is no nil guard forCreatedAt. While the GitHub API typically populates this field, Check A (line 254) already demonstrates defensive coding by checkingcomment.CreatedAt != nil. Apply the same guard here for consistency and safety.🛡️ Proposed fix
for _, comment := range comments { - if comment.User == nil { + if comment.User == nil || comment.CreatedAt == nil { continue }🤖 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 324 - 335, The loop that inspects comments may dereference comment.CreatedAt without checking for nil; update the block in the comments loop (where comment.User is checked and author := comment.User.GetLogin() is used) to guard comment.CreatedAt != nil before accessing comment.CreatedAt.Time (same pattern as the earlier check at comment.CreatedAt != nil), and only log/return after verifying CreatedAt is non-nil to avoid a potential nil-pointer dereference.
🧹 Nitpick comments (4)
internal/steps/auto_closer.go (3)
347-365:fetchAllCommentsis only used by Check A and can be eliminated.If Check A switches to a
Since-filtered call (as suggested above), this helper becomes unnecessary. Even if kept, it duplicates the pagination pattern already present in Check C (lines 318-342). Consider a single shared helper that accepts*IssueListCommentsOptions.🤖 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 347 - 365, Remove the unneeded fetchAllComments method on AutoCloser (only used by "Check A") and either change Check A to call the GitHub ListComments with a Since-filtered IssueListCommentsOptions like Check C already does, or replace both with one shared pagination helper that accepts *githubapi.IssueListCommentsOptions and returns []*githubapi.IssueComment; update Check A to call that new helper (or the Since-filtered call) and remove duplicate pagination logic in fetchAllComments. Ensure references to fetchAllComments are deleted and Check C's pagination pattern or the new helper is reused for consistency.
286-308: RedundantListIssueEventscall — events are already fetched infindLabeledTime.
ListIssueEventsis called once infindLabeledTime(line 211) and again here for Check B. For issues with many events this doubles the API cost and rate-limit consumption. Consider fetching events once and passing the slice into both checks, or caching the result on theAutoCloser.🤖 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 286 - 308, The code redundantly calls AutoCloser.github.ListIssueEvents twice (once in findLabeledTime and again in the "Check B" block), increasing API usage; modify findLabeledTime to return the fetched events slice (or store it on the AutoCloser instance) and change the "Check B" logic to reuse that events slice instead of calling ListIssueEvents again—locate symbols findLabeledTime, AutoCloser.github.ListIssueEvents, and the "Check B" loop where isBotUser is used to accept the passed/cached events and remove the second API call.
240-284:fetchAllCommentsfetches the entire comment history; use aSincefilter instead.The doc comment on lines 241-242 says "only consider comments within a 24-hour window before
since", butfetchAllComments(line 244) retrieves all comments and filters in-memory at line 254. For high-traffic issues this is wasteful—IssueListCommentsOptionsalready supports aSincefield, which would let the API do the filtering.Also, this is a second paginated listing of the same issue's comments; Check C (line 319) does a third call. Consider fetching once and reusing.
♻️ Suggested: pass `triageWindow` as `Since` to avoid fetching the full history
Replace
fetchAllCommentsusage with a time-filtered call:- allComments, err := ac.fetchAllComments(ctx, org, repo, number) + triageOpts := &githubapi.IssueListCommentsOptions{ + Since: &triageWindow, + ListOptions: githubapi.ListOptions{PerPage: 100}, + } + allComments, err := ac.fetchComments(ctx, org, repo, number, triageOpts)And unify
fetchAllCommentsinto a general helper that accepts options, which Check C can also reuse.🤖 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 240 - 284, The code currently calls fetchAllComments and filters in-memory by triageWindow; change this to call the GitHub comments list with Since=triageWindow (use IssueListCommentsOptions) so the API does the time filtering (update or add a helper like fetchCommentsWithOptions that accepts a Since/time.Time), use that helper in the triage-check loop (where triageWindow is computed) and reuse the fetched comments for Check C instead of making a separate paginated fetch; keep the existing logic that inspects each comment, calls ac.github.ListIssueCommentReactions for the bot triage comment, and preserves the break/return behavior.cmd/simili/commands/auto_close.go (1)
86-109: Config load failure is downgraded to a warning — verify this is intentional for production use.When
LoadWithInheritancefails (line 98), the error is printed as a warning and a zero-valueconfig.Config{}is used. This means the auto-closer silently runs with noBotUsers, noAutoClosesettings, and default grace period. For a CI/E2E context this is convenient, but in production it could mask misconfiguration and lead to unintended issue closures.Consider making this a hard error (or at least a
--strictmode) outside of dry-run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/simili/commands/auto_close.go` around lines 86 - 109, The code currently swallows LoadWithInheritance errors and falls back to a zero-value cfg; change this so a failed config load is a hard error in production by either (A) returning/exit non-zero on err from config.LoadWithInheritance (instead of assigning cfg = &config.Config{}), or (B) add a new flag (e.g., --strict) and, when strict is set (or when not in dry-run), treat err from config.LoadWithInheritance as fatal: log the error via fmt.Fprintf(os.Stderr, ...) and os.Exit(1); keep the existing warning/fallback only when strict is false and you explicitly want permissive behavior; update the handling around actualCfgPath, cfg, verbose and LoadWithInheritance accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/simili/commands/auto_close.go`:
- Around line 54-55: The flag help is wrong: the flag autoCloseCmd uses
autoCloseGracePeriodMins but the code maps 0→1 minute; update the flag
description to reflect that behavior (e.g., "0 is treated as 1 minute (smallest
positive value); use 0 only for testing" or similar) so users aren't misled, or
alternatively change the handling so 0 is allowed to propagate into
auto_closer.go and implement instant-expire handling there; reference
autoCloseCmd, autoCloseGracePeriodMins and auto_closer.go when making the
change.
In `@internal/steps/auto_closer.go`:
- Around line 270-276: The loop over reactions treats a nil r.User as a human
because user remains "" and isBotUser("") returns false; update the reactions
iteration in auto_closer.go to skip any reaction where r.User == nil (i.e.,
continue the loop early) before computing user or calling isBotUser, ensuring
only reactions with non-nil r.User are considered when checking for negative
reactions.
---
Outside diff comments:
In `@internal/steps/auto_closer.go`:
- Around line 324-335: The loop that inspects comments may dereference
comment.CreatedAt without checking for nil; update the block in the comments
loop (where comment.User is checked and author := comment.User.GetLogin() is
used) to guard comment.CreatedAt != nil before accessing comment.CreatedAt.Time
(same pattern as the earlier check at comment.CreatedAt != nil), and only
log/return after verifying CreatedAt is non-nil to avoid a potential nil-pointer
dereference.
---
Nitpick comments:
In `@cmd/simili/commands/auto_close.go`:
- Around line 86-109: The code currently swallows LoadWithInheritance errors and
falls back to a zero-value cfg; change this so a failed config load is a hard
error in production by either (A) returning/exit non-zero on err from
config.LoadWithInheritance (instead of assigning cfg = &config.Config{}), or (B)
add a new flag (e.g., --strict) and, when strict is set (or when not in
dry-run), treat err from config.LoadWithInheritance as fatal: log the error via
fmt.Fprintf(os.Stderr, ...) and os.Exit(1); keep the existing warning/fallback
only when strict is false and you explicitly want permissive behavior; update
the handling around actualCfgPath, cfg, verbose and LoadWithInheritance
accordingly.
In `@internal/steps/auto_closer.go`:
- Around line 347-365: Remove the unneeded fetchAllComments method on AutoCloser
(only used by "Check A") and either change Check A to call the GitHub
ListComments with a Since-filtered IssueListCommentsOptions like Check C already
does, or replace both with one shared pagination helper that accepts
*githubapi.IssueListCommentsOptions and returns []*githubapi.IssueComment;
update Check A to call that new helper (or the Since-filtered call) and remove
duplicate pagination logic in fetchAllComments. Ensure references to
fetchAllComments are deleted and Check C's pagination pattern or the new helper
is reused for consistency.
- Around line 286-308: The code redundantly calls
AutoCloser.github.ListIssueEvents twice (once in findLabeledTime and again in
the "Check B" block), increasing API usage; modify findLabeledTime to return the
fetched events slice (or store it on the AutoCloser instance) and change the
"Check B" logic to reuse that events slice instead of calling ListIssueEvents
again—locate symbols findLabeledTime, AutoCloser.github.ListIssueEvents, and the
"Check B" loop where isBotUser is used to accept the passed/cached events and
remove the second API call.
- Around line 240-284: The code currently calls fetchAllComments and filters
in-memory by triageWindow; change this to call the GitHub comments list with
Since=triageWindow (use IssueListCommentsOptions) so the API does the time
filtering (update or add a helper like fetchCommentsWithOptions that accepts a
Since/time.Time), use that helper in the triage-check loop (where triageWindow
is computed) and reuse the fetched comments for Check C instead of making a
separate paginated fetch; keep the existing logic that inspects each comment,
calls ac.github.ListIssueCommentReactions for the bot triage comment, and
preserves the break/return behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/e2e-test.ymlcmd/simili/commands/auto_close.gointernal/integrations/github/client.gointernal/steps/auto_closer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/integrations/github/client.go
| autoCloseCmd.Flags().IntVar(&autoCloseGracePeriodMins, "grace-period-minutes", 0, | ||
| "Override grace period in minutes (0 means instant-expire; use for testing)") |
There was a problem hiding this comment.
Flag description says "0 means instant-expire" but 0 is silently mapped to 1 minute.
The help text on line 55 states 0 means instant-expire; use for testing, but lines 116-117 convert 0 to 1 minute. A user passing --grace-period-minutes 0 would expect truly instant expiry, not a 1-minute window. Either update the help text to say "0 → 1 min (smallest positive value)" or allow 0 to propagate and handle it in auto_closer.go.
📝 Proposed fix: update the flag description
autoCloseCmd.Flags().IntVar(&autoCloseGracePeriodMins, "grace-period-minutes", 0,
- "Override grace period in minutes (0 means instant-expire; use for testing)")
+ "Override grace period in minutes (0 is rounded up to 1; use for testing)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| autoCloseCmd.Flags().IntVar(&autoCloseGracePeriodMins, "grace-period-minutes", 0, | |
| "Override grace period in minutes (0 means instant-expire; use for testing)") | |
| autoCloseCmd.Flags().IntVar(&autoCloseGracePeriodMins, "grace-period-minutes", 0, | |
| "Override grace period in minutes (0 is rounded up to 1; use for testing)") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/simili/commands/auto_close.go` around lines 54 - 55, The flag help is
wrong: the flag autoCloseCmd uses autoCloseGracePeriodMins but the code maps 0→1
minute; update the flag description to reflect that behavior (e.g., "0 is
treated as 1 minute (smallest positive value); use 0 only for testing" or
similar) so users aren't misled, or alternatively change the handling so 0 is
allowed to propagate into auto_closer.go and implement instant-expire handling
there; reference autoCloseCmd, autoCloseGracePeriodMins and auto_closer.go when
making the change.
| for _, r := range reactions { | ||
| content := r.GetContent() | ||
| user := "" | ||
| if r.User != nil { | ||
| user = r.User.GetLogin() | ||
| } | ||
| if (content == "-1" || content == "confused") && !isBotUser(user, ac.cfg.BotUsers) { |
There was a problem hiding this comment.
A reaction with a nil User is treated as human activity (false positive).
When r.User is nil, user stays "", and isBotUser("", …) returns false. A negative reaction with no user attribution would incorrectly signal human activity, preventing auto-close.
🛡️ Proposed fix: skip reactions with nil user
for _, r := range reactions {
content := r.GetContent()
- user := ""
- if r.User != nil {
- user = r.User.GetLogin()
- }
- if (content == "-1" || content == "confused") && !isBotUser(user, ac.cfg.BotUsers) {
+ if r.User == nil {
+ continue
+ }
+ user := r.User.GetLogin()
+ if (content == "-1" || content == "confused") && !isBotUser(user, ac.cfg.BotUsers) {🤖 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 270 - 276, The loop over
reactions treats a nil r.User as a human because user remains "" and
isBotUser("") returns false; update the reactions iteration in auto_closer.go to
skip any reaction where r.User == nil (i.e., continue the loop early) before
computing user or calling isBotUser, ensuring only reactions with non-nil r.User
are considered when checking for negative reactions.
🧪 E2E Test✅ Bot responded: yes Test repo → gh-simili-bot/simili-e2e-22389003728 Auto-generated by E2E pipeline |
2 participants
Summary
-1or 😕confused) by a non-bot user on the bot's triage comment. The triage comment is identified via the<!-- simili-bot-report -->HTML marker using the existingisBotComment()function.potential-duplicatelabel was applied, by scanningListIssueEventsforreopenedevents from non-bot actors.ListIssueCommentReactions(paginated) tointernal/integrations/github/client.go.TestIsBotComment,TestHasNegativeReaction, andTestReopenedByHumanas table-driven unit tests with no external dependencies.Test plan
go test ./internal/steps/... -run TestIsBotCommentpassesgo test ./internal/steps/... -run TestHasNegativeReactionpassesgo test ./internal/steps/... -run TestReopenedByHumanpassesgo test ./...— full suite greengo build ./...— clean buildskipped_human🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores