Harden AI review loop and add reviewer-specific feedback extraction - #10
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fter-save bug Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Automate code review cycles for PRs created by Cursor Cloud Agents. When an agent finishes and opens a PR, the plugin requests AI reviews (CodeRabbit + Copilot), collects feedback, posts @cursor comments to have the agent fix issues, and repeats until CodeRabbit approves. Key features: - GitHub client wrapper using google/go-github v68 - ReviewLoop KV store entity with state machine - Webhook-driven review cycle (PR reviews + synchronize events) - Inline thread UX updates (no spam, quiet loop / loud milestones) - Webapp dashboard with review loop phase, timeline, and who's-up - Safety valve via MaxReviewIterations config - Fully gated behind EnableAIReviewLoop toggle Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a complete AI-driven PR review loop: new kvstore entities and indexes, ghclient wrapper for GitHub, review-loop state machine (start/iterate/complete), webhook and poller integrations, UI/websocket support, configuration for GitHub PAT and AI reviewers, extensive tests, and attachment/UI updates. Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent/Workflow
participant Poller as Poller
participant KV as KV Store
participant Plugin as Plugin
participant GH as GitHub API
participant Cursor as Cursor Client
participant MM as Mattermost
Agent->>Poller: finish with PR
Poller->>KV: Get/Save ReviewLoop index
KV-->>Plugin: ReviewLoop instance
Plugin->>GH: MarkPRReadyForReview / RequestReviewers
Plugin->>MM: post review status
GH->>Plugin: pull request review webhook
Plugin->>GH: ListReviews / ListReviewComments
Plugin->>Plugin: classify feedback
alt Approved by AI bots
Plugin->>KV: transition approved/complete
Plugin->>MM: post approval
else Needs fixes
Plugin->>Cursor: AddFollowup (dispatch fixes)
Plugin->>KV: transition cursor_fixing
Plugin->>MM: post fix request
end
GH->>Plugin: PR synchronize (new commit)
Plugin->>KV: update last commit SHA
Plugin->>Plugin: re-trigger awaiting_review
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
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 |
nickmisasi
marked this pull request as ready for review
February 17, 2026 14:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
server/poller.go (1)
369-373: Remove the incorrectnolint:unusedcomment.The
cleanupStaleAgentsfunction is called on line 26, so this nolint directive is unnecessary.Suggested fix
// cleanupStaleAgents marks agents stuck in CREATING or RUNNING state for longer // than maxAge as STOPPED and notifies users via thread messages. -// -//nolint:unused // Called from scheduled maintenance, not from the main poll loop. func (p *Plugin) cleanupStaleAgents(agents []*kvstore.AgentRecord, maxAge time.Duration) int {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/poller.go` around lines 369 - 373, Remove the erroneous nolint:unused directive above the cleanupStaleAgents function: this function (cleanupStaleAgents in type Plugin) is actually referenced elsewhere, so delete the "//nolint:unused" comment so the linter won't be misled; ensure no other change to the function signature or surrounding comment is made.server/poller_test.go (1)
626-627: Move mockGitHubClient to server/testhelpers_test.go for shared use across test files.The mock is currently defined in
server/reviewloop_test.go(line 20) but used in bothreviewloop_test.goandpoller_test.go(line 626). Centralizing it intesthelpers_test.gofollows the established pattern for shared test mocks in this codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/poller_test.go` around lines 626 - 627, The mock definition for mockGitHubClient is duplicated; move its type and helper constructors into a shared test helper file (testhelpers_test.go) so both tests reuse it: create mockGitHubClient in testhelpers_test.go (keeping the same type name and methods), remove the local mockGitHubClient definition from reviewloop_test.go, and update poller_test.go/reviewloop_test.go to use the shared mock by assigning p.githubClient = &mockGitHubClient{} where present; ensure package names match so test files can see the type and run `go test` to verify.webapp/src/components/common/PhaseBadge.tsx (1)
3-9: Consider using a discriminated union for type safety.Changing
PHASE_CONFIGfromRecord<WorkflowPhase, ...>toRecord<string, ...>loses compile-time exhaustiveness checking. However, the fallback on line 30 handles missing phases gracefully at runtime.For future type safety, consider defining a combined phase type:
type AllPhases = WorkflowPhase | ReviewLoopPhase; const PHASE_CONFIG: Record<AllPhases, {label: string; className: string}> = { ... };This is a minor type-safety improvement and not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/common/PhaseBadge.tsx` around lines 3 - 9, PHASE_CONFIG is typed as Record<string,...> which loses compile-time exhaustiveness for the phases; define a union type (e.g., type AllPhases = WorkflowPhase | ReviewLoopPhase) and change PHASE_CONFIG to Record<AllPhases, {label: string; className: string}> so the compiler will check keys against the combined phase types (update the PHASE_CONFIG declaration and keep existing runtime fallback in the PhaseBadge rendering logic that uses Props.phase).webapp/src/components/rhs/AgentDetail.tsx (2)
304-307: Consider using timestamp as React key instead of array index.Using array index as the React key is generally discouraged. Since history is append-only and timestamps should be unique, using
event.timestampas the key would be more stable.Proposed fix
{reviewLoop.history.map((event, index) => ( <div - key={index} + key={event.timestamp} className='cursor-review-loop-timeline-item' >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/rhs/AgentDetail.tsx` around lines 304 - 307, In the map over reviewLoop.history in AgentDetail.tsx (the block using reviewLoop.history.map and rendering elements with className 'cursor-review-loop-timeline-item'), replace the unstable key={index} with a stable unique key using event.timestamp (e.g., key={event.timestamp}); if timestamps might be missing or non-unique, fall back to a deterministic composite key (like `${event.timestamp}-${event.id}` or `${event.timestamp}-${index}`) to ensure uniqueness.
76-91:getElapsedTimevalues won't update without re-render.The elapsed time strings (e.g., "5m ago") are computed once when the component renders and won't update dynamically. For long-lived sessions, users may see stale timestamps. This is a minor UX consideration—if real-time updates are desired, consider using a timer to periodically trigger re-renders or using a library like
date-fnswith relative time formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/rhs/AgentDetail.tsx` around lines 76 - 91, The elapsed strings produced by getElapsedTime are computed only on render and will become stale; update AgentDetail to re-render periodically (e.g., add a local tick state and a useEffect that sets up a setInterval/clearInterval) so getElapsedTime is called again on each tick; choose an interval granularity appropriate for your needs (every minute for "Xm ago", or every 10s/1s for finer updates), ensure you clear the interval in the effect cleanup, and reference getElapsedTime and the AgentDetail component when applying the change.server/attachments/attachments.go (1)
366-374: Consider using yellow forhuman_reviewphase.The
human_reviewphase falls through to the green default, but this phase represents "waiting for human reviewer" which is an in-progress/waiting state. Consider usingColorYellowfor consistency with other HITL review states.Proposed fix
switch reviewPhase { case "requesting_review", "awaiting_review", "cursor_fixing": color = ColorBlue + case "human_review": + color = ColorYellow case "max_iterations": color = ColorGrey case "failed": color = ColorRed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/attachments/attachments.go` around lines 366 - 374, The switch that sets the attachment card color based on reviewPhase currently falls through to ColorGreen for "human_review"; update the switch (the block that assigns color based on the reviewPhase variable) to add a case for "human_review" and set color = ColorYellow so the waiting/in-progress HITL state is shown as yellow; ensure you reference the same ColorYellow constant used elsewhere and keep other cases unchanged.server/reviewloop.go (2)
26-31: Discarded error may mask storage failures.The error from
GetReviewLoopByPRURLis silently discarded. If the KV store returns an error other than "not found", this could lead to unintended duplicate review loops being created.♻️ Proposed fix
// Idempotency: check for existing review loop for this PR. - existing, _ := p.kvstore.GetReviewLoopByPRURL(record.PrURL) - if existing != nil { + existing, err := p.kvstore.GetReviewLoopByPRURL(record.PrURL) + if err != nil { + p.API.LogWarn("Failed to check for existing review loop", "error", err.Error(), "pr_url", record.PrURL) + // Proceed anyway - worst case we may create a duplicate that gets deduped later. + } + if existing != nil { p.API.LogDebug("Review loop already exists for PR, skipping", "pr_url", record.PrURL, "review_loop_id", existing.ID) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/reviewloop.go` around lines 26 - 31, The error returned from p.kvstore.GetReviewLoopByPRURL is currently discarded which can hide storage failures; change the call to capture the error (e.g., existing, err := p.kvstore.GetReviewLoopByPRURL(record.PrURL)), then if err != nil handle it: if the KV layer exposes a "not found" sentinel check for that and continue, otherwise log the error (use p.API.LogError or similar) with context ("pr_url", record.PrURL) and return the error to stop creating a duplicate review loop; keep the existing early-return when existing != nil and preserve the p.API.LogDebug call that logs existing.ID.
179-179: Consider logging errors from SaveReviewLoop in terminal state paths.The error is discarded here. While this is a terminal state, logging the failure would aid debugging if state persistence issues occur.
♻️ Suggested improvement
- _ = p.kvstore.SaveReviewLoop(loop) + if err := p.kvstore.SaveReviewLoop(loop); err != nil { + p.API.LogError("Failed to save review loop at max iterations", "error", err.Error(), "review_loop_id", loop.ID) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/reviewloop.go` at line 179, Capture and check the error returned from p.kvstore.SaveReviewLoop(loop) instead of discarding it; if non-nil, log it with context (e.g., loop ID and current state) so failures to persist the terminal state are visible for debugging. Use the existing logger on the receiver (e.g., p.logger.Errorf or similar) or fall back to log.Printf if no logger field exists, and include the error value and a short descriptive message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/reviewloop_test.go`:
- Around line 125-127: The AddReaction mocks currently return nil which causes a
dereference panic in pluginapi.PostService.AddReaction; update each mock Return
to return a non-nil Reaction object (e.g., replace Return(nil, nil) with
Return(&model.Reaction{}, nil)) for the AddReaction calls shown (the
api.On("AddReaction", mock.MatchedBy(...)) invocations at the locations
referenced, including the instances around the symbols in this file), and apply
the same change to the other occurrences mentioned so the mocked AddReaction
always returns &model.Reaction{} as the first return value.
In `@server/reviewloop.go`:
- Around line 224-244: The handler handlePRSynchronize unconditionally sets
loop.Phase to kvstore.ReviewPhaseAwaitingReview which can regress terminal
states; add a state guard that only performs the transition (and appends the
history event, updates UpdatedAt, saves, and publishes) when loop.Phase is the
expected non-terminal state (e.g., the "cursor fixing" phase) and explicitly
skip/return early if loop.Phase is one of the terminal states (e.g.,
kvstore.ReviewPhaseComplete, kvstore.ReviewPhaseMaxIterations,
kvstore.ReviewPhaseFailed); ensure you still persist and publish only when the
phase changes and keep using p.kvstore.SaveReviewLoop,
p.updateReviewLoopInlineStatus, and p.publishReviewLoopChange as shown.
- Around line 72-79: The code calls getGitHubClient() and immediately uses
ghClient.RequestReviewers without checking for nil; add a nil-check after
ghClient := p.getGitHubClient() (e.g., if ghClient == nil { /* log or
return/skip */ }) and handle the missing client by logging a clear message and
returning or skipping the reviewer request so RequestReviewers is never invoked
on a nil pointer; update any surrounding error handling accordingly for the
prRef/RequestReviewers call.
In `@webapp/src/components/common/styles.css`:
- Around line 518-663: The timeline dot (.cursor-review-loop-timeline-dot)
currently uses a 6px size and 5px top margin which breaks the 4px grid; change
width and height to 4px and set margin-top to 4px (keep margin-left as-is or
adjust to a 4px-grid value such as -8px or -12px if visual alignment requires
it) so the dot sizing and spacing conform to the 4px grid system.
---
Nitpick comments:
In `@server/attachments/attachments.go`:
- Around line 366-374: The switch that sets the attachment card color based on
reviewPhase currently falls through to ColorGreen for "human_review"; update the
switch (the block that assigns color based on the reviewPhase variable) to add a
case for "human_review" and set color = ColorYellow so the waiting/in-progress
HITL state is shown as yellow; ensure you reference the same ColorYellow
constant used elsewhere and keep other cases unchanged.
In `@server/poller_test.go`:
- Around line 626-627: The mock definition for mockGitHubClient is duplicated;
move its type and helper constructors into a shared test helper file
(testhelpers_test.go) so both tests reuse it: create mockGitHubClient in
testhelpers_test.go (keeping the same type name and methods), remove the local
mockGitHubClient definition from reviewloop_test.go, and update
poller_test.go/reviewloop_test.go to use the shared mock by assigning
p.githubClient = &mockGitHubClient{} where present; ensure package names match
so test files can see the type and run `go test` to verify.
In `@server/poller.go`:
- Around line 369-373: Remove the erroneous nolint:unused directive above the
cleanupStaleAgents function: this function (cleanupStaleAgents in type Plugin)
is actually referenced elsewhere, so delete the "//nolint:unused" comment so the
linter won't be misled; ensure no other change to the function signature or
surrounding comment is made.
In `@server/reviewloop.go`:
- Around line 26-31: The error returned from p.kvstore.GetReviewLoopByPRURL is
currently discarded which can hide storage failures; change the call to capture
the error (e.g., existing, err := p.kvstore.GetReviewLoopByPRURL(record.PrURL)),
then if err != nil handle it: if the KV layer exposes a "not found" sentinel
check for that and continue, otherwise log the error (use p.API.LogError or
similar) with context ("pr_url", record.PrURL) and return the error to stop
creating a duplicate review loop; keep the existing early-return when existing
!= nil and preserve the p.API.LogDebug call that logs existing.ID.
- Line 179: Capture and check the error returned from
p.kvstore.SaveReviewLoop(loop) instead of discarding it; if non-nil, log it with
context (e.g., loop ID and current state) so failures to persist the terminal
state are visible for debugging. Use the existing logger on the receiver (e.g.,
p.logger.Errorf or similar) or fall back to log.Printf if no logger field
exists, and include the error value and a short descriptive message.
In `@webapp/src/components/common/PhaseBadge.tsx`:
- Around line 3-9: PHASE_CONFIG is typed as Record<string,...> which loses
compile-time exhaustiveness for the phases; define a union type (e.g., type
AllPhases = WorkflowPhase | ReviewLoopPhase) and change PHASE_CONFIG to
Record<AllPhases, {label: string; className: string}> so the compiler will check
keys against the combined phase types (update the PHASE_CONFIG declaration and
keep existing runtime fallback in the PhaseBadge rendering logic that uses
Props.phase).
In `@webapp/src/components/rhs/AgentDetail.tsx`:
- Around line 304-307: In the map over reviewLoop.history in AgentDetail.tsx
(the block using reviewLoop.history.map and rendering elements with className
'cursor-review-loop-timeline-item'), replace the unstable key={index} with a
stable unique key using event.timestamp (e.g., key={event.timestamp}); if
timestamps might be missing or non-unique, fall back to a deterministic
composite key (like `${event.timestamp}-${event.id}` or
`${event.timestamp}-${index}`) to ensure uniqueness.
- Around line 76-91: The elapsed strings produced by getElapsedTime are computed
only on render and will become stale; update AgentDetail to re-render
periodically (e.g., add a local tick state and a useEffect that sets up a
setInterval/clearInterval) so getElapsedTime is called again on each tick;
choose an interval granularity appropriate for your needs (every minute for "Xm
ago", or every 10s/1s for finer updates), ensure you clear the interval in the
effect cleanup, and reference getElapsedTime and the AgentDetail component when
applying the change.
Comment on lines
+125
to
+127
| api.On("AddReaction", mock.MatchedBy(func(r *model.Reaction) bool { | ||
| return r.PostId == "trigger-1" && r.EmojiName == "eyes" | ||
| })).Return(nil, nil) |
There was a problem hiding this comment.
AddReaction mock returns nil instead of &model.Reaction{}.
Per coding guidelines, AddReaction mocks should return &model.Reaction{} not nil since pluginapi.PostService.AddReaction dereferences the result. This pattern appears in multiple places in this file (lines 127, 257, 299, 419, 647).
Proposed fix
api.On("AddReaction", mock.MatchedBy(func(r *model.Reaction) bool {
return r.PostId == "trigger-1" && r.EmojiName == "eyes"
- })).Return(nil, nil)
+ })).Return(&model.Reaction{}, nil)Apply similar changes to lines 257, 299, 419, and 647.
As per coding guidelines: "When mocking AddReaction in command tests using pluginapi.Client, return &model.Reaction{} not nil since pluginapi.PostService.AddReaction dereferences the result"
📝 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.
Suggested change
| api.On("AddReaction", mock.MatchedBy(func(r *model.Reaction) bool { | |
| return r.PostId == "trigger-1" && r.EmojiName == "eyes" | |
| })).Return(nil, nil) | |
| api.On("AddReaction", mock.MatchedBy(func(r *model.Reaction) bool { | |
| return r.PostId == "trigger-1" && r.EmojiName == "eyes" | |
| })).Return(&model.Reaction{}, nil) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/reviewloop_test.go` around lines 125 - 127, The AddReaction mocks
currently return nil which causes a dereference panic in
pluginapi.PostService.AddReaction; update each mock Return to return a non-nil
Reaction object (e.g., replace Return(nil, nil) with Return(&model.Reaction{},
nil)) for the AddReaction calls shown (the api.On("AddReaction",
mock.MatchedBy(...)) invocations at the locations referenced, including the
instances around the symbols in this file), and apply the same change to the
other occurrences mentioned so the mocked AddReaction always returns
&model.Reaction{} as the first return value.
Comment on lines
+72
to
+79
| ghClient := p.getGitHubClient() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| defer cancel() | ||
|
|
||
| err := ghClient.RequestReviewers(ctx, prRef.Owner, prRef.Repo, prRef.Number, github.ReviewersRequest{ | ||
| Reviewers: botUsernames, | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
Missing nil-check for GitHub client before use.
The getGitHubClient() result is used directly without verifying it's non-nil. If the GitHub client is not configured, this will cause a nil pointer dereference on line 76.
🔧 Proposed fix
} else {
ghClient := p.getGitHubClient()
+ if ghClient == nil {
+ p.API.LogError("GitHub client is not configured, cannot request reviewers")
+ // Continue to awaiting_review state; webhooks may still pick up reviews.
+ } else {
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
defer cancel()
err := ghClient.RequestReviewers(ctx, prRef.Owner, prRef.Repo, prRef.Number, github.ReviewersRequest{
Reviewers: botUsernames,
})
if err != nil {
// ... existing error handling ...
}
+ }
}As per coding guidelines: "Always nil-check the Cursor client before use, as it is nil when no API key is configured" - the same defensive pattern should apply to the GitHub client.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/reviewloop.go` around lines 72 - 79, The code calls getGitHubClient()
and immediately uses ghClient.RequestReviewers without checking for nil; add a
nil-check after ghClient := p.getGitHubClient() (e.g., if ghClient == nil { /*
log or return/skip */ }) and handle the missing client by logging a clear
message and returning or skipping the reviewer request so RequestReviewers is
never invoked on a nil pointer; update any surrounding error handling
accordingly for the prRef/RequestReviewers call.
Comment on lines
+224
to
+244
| func (p *Plugin) handlePRSynchronize(loop *kvstore.ReviewLoop, pr ghPullRequest) error { | ||
| if pr.Head.SHA != "" { | ||
| loop.LastCommitSHA = pr.Head.SHA | ||
| } | ||
|
|
||
| loop.Phase = kvstore.ReviewPhaseAwaitingReview | ||
| loop.History = append(loop.History, kvstore.ReviewLoopEvent{ | ||
| Phase: kvstore.ReviewPhaseAwaitingReview, | ||
| Timestamp: time.Now().UnixMilli(), | ||
| Detail: "Cursor pushed fixes", | ||
| }) | ||
| loop.UpdatedAt = time.Now().UnixMilli() | ||
|
|
||
| if err := p.kvstore.SaveReviewLoop(loop); err != nil { | ||
| return fmt.Errorf("failed to save review loop: %w", err) | ||
| } | ||
|
|
||
| p.updateReviewLoopInlineStatus(loop) | ||
| p.publishReviewLoopChange(loop) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing state guard could allow invalid transitions.
The comment states this transitions from cursor_fixing → awaiting_review, but the code doesn't verify the current phase. If a synchronize webhook arrives after the loop has reached a terminal state (Complete, MaxIterations, Failed), this would incorrectly revert it.
🛡️ Proposed fix
func (p *Plugin) handlePRSynchronize(loop *kvstore.ReviewLoop, pr ghPullRequest) error {
+ // Only transition if we're in cursor_fixing state.
+ if loop.Phase != kvstore.ReviewPhaseCursorFixing {
+ p.API.LogDebug("Ignoring PR synchronize for non-cursor_fixing state",
+ "review_loop_id", loop.ID,
+ "current_phase", loop.Phase,
+ )
+ return nil
+ }
+
if pr.Head.SHA != "" {
loop.LastCommitSHA = pr.Head.SHA
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/reviewloop.go` around lines 224 - 244, The handler handlePRSynchronize
unconditionally sets loop.Phase to kvstore.ReviewPhaseAwaitingReview which can
regress terminal states; add a state guard that only performs the transition
(and appends the history event, updates UpdatedAt, saves, and publishes) when
loop.Phase is the expected non-terminal state (e.g., the "cursor fixing" phase)
and explicitly skip/return early if loop.Phase is one of the terminal states
(e.g., kvstore.ReviewPhaseComplete, kvstore.ReviewPhaseMaxIterations,
kvstore.ReviewPhaseFailed); ensure you still persist and publish only when the
phase changes and keep using p.kvstore.SaveReviewLoop,
p.updateReviewLoopInlineStatus, and p.publishReviewLoopChange as shown.
Comment on lines
+518
to
+663
|
|
||
| /* --- Review Loop Phase Badges --- */ | ||
|
|
||
| .cursor-phase-rl-requesting { | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | ||
| color: var(--button-bg); | ||
| border: 1px solid var(--button-bg); | ||
| } | ||
|
|
||
| .cursor-phase-rl-awaiting { | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | ||
| color: var(--button-bg); | ||
| border: 1px solid var(--button-bg); | ||
| } | ||
|
|
||
| .cursor-phase-rl-fixing { | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | ||
| color: var(--away-indicator); | ||
| border: 1px solid var(--away-indicator); | ||
| } | ||
|
|
||
| .cursor-phase-rl-approved { | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | ||
| color: var(--online-indicator); | ||
| border: 1px solid var(--online-indicator); | ||
| } | ||
|
|
||
| .cursor-phase-rl-human { | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | ||
| color: var(--away-indicator); | ||
| border: 1px solid var(--away-indicator); | ||
| } | ||
|
|
||
| .cursor-phase-rl-maxiter { | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | ||
| color: var(--dnd-indicator); | ||
| border: 1px solid var(--dnd-indicator); | ||
| } | ||
|
|
||
| .cursor-phase-rl-failed { | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | ||
| color: var(--dnd-indicator); | ||
| border: 1px solid var(--dnd-indicator); | ||
| } | ||
|
|
||
| /* --- Review Loop Section in AgentDetail --- */ | ||
|
|
||
| .cursor-review-loop-section { | ||
| margin-top: 4px; | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup { | ||
| margin-bottom: 8px; | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup-label { | ||
| font-size: 13px; | ||
| font-weight: 600; | ||
| color: var(--center-channel-color); | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup--active .cursor-review-loop-whosup-label, | ||
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--active { | ||
| color: var(--button-bg); | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--approved { | ||
| color: var(--online-indicator); | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--waiting { | ||
| color: var(--away-indicator); | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--complete { | ||
| color: var(--online-indicator); | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--warning { | ||
| color: var(--dnd-indicator); | ||
| } | ||
|
|
||
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--failed { | ||
| color: var(--dnd-indicator); | ||
| } | ||
|
|
||
| .cursor-review-loop-iteration { | ||
| font-size: 12px; | ||
| color: rgba(var(--center-channel-color-rgb), 0.56); | ||
| margin-bottom: 8px; | ||
| } | ||
|
|
||
| .cursor-review-loop-pr { | ||
| margin-bottom: 8px; | ||
| } | ||
|
|
||
| /* --- Review Loop Timeline --- */ | ||
|
|
||
| .cursor-review-loop-timeline { | ||
| margin-top: 8px; | ||
| padding-left: 8px; | ||
| border-left: 2px solid rgba(var(--center-channel-color-rgb), 0.12); | ||
| } | ||
|
|
||
| .cursor-review-loop-timeline-item { | ||
| display: flex; | ||
| align-items: flex-start; | ||
| gap: 8px; | ||
| padding: 4px 0; | ||
| position: relative; | ||
| } | ||
|
|
||
| .cursor-review-loop-timeline-dot { | ||
| width: 6px; | ||
| height: 6px; | ||
| border-radius: 50%; | ||
| background-color: rgba(var(--center-channel-color-rgb), 0.3); | ||
| flex-shrink: 0; | ||
| margin-top: 5px; | ||
| margin-left: -12px; | ||
| } | ||
|
|
||
| .cursor-review-loop-timeline-content { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| align-items: baseline; | ||
| gap: 4px; | ||
| font-size: 12px; | ||
| line-height: 1.4; | ||
| } | ||
|
|
||
| .cursor-review-loop-timeline-phase { | ||
| color: var(--center-channel-color); | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .cursor-review-loop-timeline-detail { | ||
| color: rgba(var(--center-channel-color-rgb), 0.72); | ||
| } | ||
|
|
||
| .cursor-review-loop-timeline-time { | ||
| color: rgba(var(--center-channel-color-rgb), 0.4); | ||
| font-size: 11px; | ||
| margin-left: auto; | ||
| white-space: nowrap; | ||
| } |
There was a problem hiding this comment.
Align review-loop timeline dot sizing/margins to the 4px grid.
Line 631-637 uses a 6px dot and 5px top margin, which breaks the 4px spacing system.
♻️ Proposed adjustment
.cursor-review-loop-timeline-dot {
- width: 6px;
- height: 6px;
+ width: 8px;
+ height: 8px;
border-radius: 50%;
background-color: rgba(var(--center-channel-color-rgb), 0.3);
flex-shrink: 0;
- margin-top: 5px;
+ margin-top: 4px;
margin-left: -12px;
}As per coding guidelines: Use 4px grid spacing system: 4, 8, 12, 16, 20, 24, 32, 40px.
📝 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.
Suggested change
| /* --- Review Loop Phase Badges --- */ | |
| .cursor-phase-rl-requesting { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--button-bg); | |
| border: 1px solid var(--button-bg); | |
| } | |
| .cursor-phase-rl-awaiting { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--button-bg); | |
| border: 1px solid var(--button-bg); | |
| } | |
| .cursor-phase-rl-fixing { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--away-indicator); | |
| border: 1px solid var(--away-indicator); | |
| } | |
| .cursor-phase-rl-approved { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--online-indicator); | |
| border: 1px solid var(--online-indicator); | |
| } | |
| .cursor-phase-rl-human { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--away-indicator); | |
| border: 1px solid var(--away-indicator); | |
| } | |
| .cursor-phase-rl-maxiter { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--dnd-indicator); | |
| border: 1px solid var(--dnd-indicator); | |
| } | |
| .cursor-phase-rl-failed { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--dnd-indicator); | |
| border: 1px solid var(--dnd-indicator); | |
| } | |
| /* --- Review Loop Section in AgentDetail --- */ | |
| .cursor-review-loop-section { | |
| margin-top: 4px; | |
| } | |
| .cursor-review-loop-whosup { | |
| margin-bottom: 8px; | |
| } | |
| .cursor-review-loop-whosup-label { | |
| font-size: 13px; | |
| font-weight: 600; | |
| color: var(--center-channel-color); | |
| } | |
| .cursor-review-loop-whosup--active .cursor-review-loop-whosup-label, | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--active { | |
| color: var(--button-bg); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--approved { | |
| color: var(--online-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--waiting { | |
| color: var(--away-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--complete { | |
| color: var(--online-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--warning { | |
| color: var(--dnd-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--failed { | |
| color: var(--dnd-indicator); | |
| } | |
| .cursor-review-loop-iteration { | |
| font-size: 12px; | |
| color: rgba(var(--center-channel-color-rgb), 0.56); | |
| margin-bottom: 8px; | |
| } | |
| .cursor-review-loop-pr { | |
| margin-bottom: 8px; | |
| } | |
| /* --- Review Loop Timeline --- */ | |
| .cursor-review-loop-timeline { | |
| margin-top: 8px; | |
| padding-left: 8px; | |
| border-left: 2px solid rgba(var(--center-channel-color-rgb), 0.12); | |
| } | |
| .cursor-review-loop-timeline-item { | |
| display: flex; | |
| align-items: flex-start; | |
| gap: 8px; | |
| padding: 4px 0; | |
| position: relative; | |
| } | |
| .cursor-review-loop-timeline-dot { | |
| width: 6px; | |
| height: 6px; | |
| border-radius: 50%; | |
| background-color: rgba(var(--center-channel-color-rgb), 0.3); | |
| flex-shrink: 0; | |
| margin-top: 5px; | |
| margin-left: -12px; | |
| } | |
| .cursor-review-loop-timeline-content { | |
| display: flex; | |
| flex-wrap: wrap; | |
| align-items: baseline; | |
| gap: 4px; | |
| font-size: 12px; | |
| line-height: 1.4; | |
| } | |
| .cursor-review-loop-timeline-phase { | |
| color: var(--center-channel-color); | |
| font-weight: 600; | |
| } | |
| .cursor-review-loop-timeline-detail { | |
| color: rgba(var(--center-channel-color-rgb), 0.72); | |
| } | |
| .cursor-review-loop-timeline-time { | |
| color: rgba(var(--center-channel-color-rgb), 0.4); | |
| font-size: 11px; | |
| margin-left: auto; | |
| white-space: nowrap; | |
| } | |
| /* --- Review Loop Phase Badges --- */ | |
| .cursor-phase-rl-requesting { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--button-bg); | |
| border: 1px solid var(--button-bg); | |
| } | |
| .cursor-phase-rl-awaiting { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--button-bg); | |
| border: 1px solid var(--button-bg); | |
| } | |
| .cursor-phase-rl-fixing { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--away-indicator); | |
| border: 1px solid var(--away-indicator); | |
| } | |
| .cursor-phase-rl-approved { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--online-indicator); | |
| border: 1px solid var(--online-indicator); | |
| } | |
| .cursor-phase-rl-human { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--away-indicator); | |
| border: 1px solid var(--away-indicator); | |
| } | |
| .cursor-phase-rl-maxiter { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--dnd-indicator); | |
| border: 1px solid var(--dnd-indicator); | |
| } | |
| .cursor-phase-rl-failed { | |
| background-color: rgba(var(--center-channel-color-rgb), 0.08); | |
| color: var(--dnd-indicator); | |
| border: 1px solid var(--dnd-indicator); | |
| } | |
| /* --- Review Loop Section in AgentDetail --- */ | |
| .cursor-review-loop-section { | |
| margin-top: 4px; | |
| } | |
| .cursor-review-loop-whosup { | |
| margin-bottom: 8px; | |
| } | |
| .cursor-review-loop-whosup-label { | |
| font-size: 13px; | |
| font-weight: 600; | |
| color: var(--center-channel-color); | |
| } | |
| .cursor-review-loop-whosup--active .cursor-review-loop-whosup-label, | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--active { | |
| color: var(--button-bg); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--approved { | |
| color: var(--online-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--waiting { | |
| color: var(--away-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--complete { | |
| color: var(--online-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--warning { | |
| color: var(--dnd-indicator); | |
| } | |
| .cursor-review-loop-whosup-label.cursor-review-loop-whosup--failed { | |
| color: var(--dnd-indicator); | |
| } | |
| .cursor-review-loop-iteration { | |
| font-size: 12px; | |
| color: rgba(var(--center-channel-color-rgb), 0.56); | |
| margin-bottom: 8px; | |
| } | |
| .cursor-review-loop-pr { | |
| margin-bottom: 8px; | |
| } | |
| /* --- Review Loop Timeline --- */ | |
| .cursor-review-loop-timeline { | |
| margin-top: 8px; | |
| padding-left: 8px; | |
| border-left: 2px solid rgba(var(--center-channel-color-rgb), 0.12); | |
| } | |
| .cursor-review-loop-timeline-item { | |
| display: flex; | |
| align-items: flex-start; | |
| gap: 8px; | |
| padding: 4px 0; | |
| position: relative; | |
| } | |
| .cursor-review-loop-timeline-dot { | |
| width: 8px; | |
| height: 8px; | |
| border-radius: 50%; | |
| background-color: rgba(var(--center-channel-color-rgb), 0.3); | |
| flex-shrink: 0; | |
| margin-top: 4px; | |
| margin-left: -12px; | |
| } | |
| .cursor-review-loop-timeline-content { | |
| display: flex; | |
| flex-wrap: wrap; | |
| align-items: baseline; | |
| gap: 4px; | |
| font-size: 12px; | |
| line-height: 1.4; | |
| } | |
| .cursor-review-loop-timeline-phase { | |
| color: var(--center-channel-color); | |
| font-weight: 600; | |
| } | |
| .cursor-review-loop-timeline-detail { | |
| color: rgba(var(--center-channel-color-rgb), 0.72); | |
| } | |
| .cursor-review-loop-timeline-time { | |
| color: rgba(var(--center-channel-color-rgb), 0.4); | |
| font-size: 11px; | |
| margin-left: auto; | |
| white-space: nowrap; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/common/styles.css` around lines 518 - 663, The timeline
dot (.cursor-review-loop-timeline-dot) currently uses a 6px size and 5px top
margin which breaks the 4px grid; change width and height to 4px and set
margin-top to 4px (keep margin-left as-is or adjust to a 4px-grid value such as
-8px or -12px if visual alignment requires it) so the dot sizing and spacing
conform to the 4px grid system.
…ation agents Without AutoBranch=true, the Cursor API pushes directly to the source ref (master) when AutoCreatePr is false, instead of creating a branch. This was causing agents to commit directly to the default branch. The fix adds AutoBranch=true to all three implementation agent launch sites (mention handler, HITL implementation, slash command). The planner agent intentionally keeps AutoBranch=false to avoid orphan branches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two critical bugs preventing the AI review loop from activating: 1. Bool config fields (EnableAIReviewLoop, AutoCreatePR, etc.) stored as strings by Mattermost plugin API but defined as bool in Go struct. LoadPluginConfiguration fails silently, leaving all config at defaults. Fix: Change to string fields with boolFromStr() helper. 2. Cursor API creates PRs as drafts. CodeRabbit skips draft PRs. Fix: Add MarkPRReadyForReview to GitHub client, call it in startReviewLoop before requesting reviewers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CodeRabbit review bodies contain HTML tags (<details>, <summary>, <blockquote>) that Mattermost doesn't render. Add sanitizeReviewBody() to convert HTML to Markdown before posting review notifications. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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)
server/hitl.go (1)
678-685:⚠️ Potential issue | 🔴 CriticalRemove
AutoBranch: truewhen explicitBranchNameis provided—they conflict.The Cursor API documentation specifies that
target.branchNameis optional. When provided, Cursor uses that branch name; when omitted, Cursor generates one. TheAutoBranchfield does not exist in the official API schema. Setting bothAutoBranch: trueand an explicitBranchNameviolates the API contract. Either provideBranchNamealone (removeAutoBranch), or omitBranchNameto let Cursor auto-generate.This applies to lines 678–685 in
server/hitl.go, and identical issues exist inserver/handlers.go:310andserver/command/command.go:193.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/hitl.go` around lines 678 - 685, The LaunchAgentRequest is setting both a specific branch name and AutoBranch which conflicts with the Cursor API; in the cursor.Target literal (see cursor.LaunchAgentRequest and cursor.Target created in function using sanitizeBranchName(workflow.OriginalPrompt)) remove the AutoBranch: true field when BranchName is explicitly set and keep AutoCreatePr: workflow.AutoCreatePR and BranchName: fmt.Sprintf("cursor/%s", sanitizeBranchName(workflow.OriginalPrompt)); apply the same change in the equivalent targets in server/handlers.go (around the handler that builds LaunchAgentRequest) and server/command/command.go (around the LaunchAgentRequest construction) so only BranchName is sent when explicit and AutoBranch is not included.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/reviewloop_test.go`:
- Around line 66-68: The mock for PublishWebSocketEvent uses the short event
name "review_loop_changed" but must use the full custom prefix; update the
mocked call(s) to use the production event name
"custom_com.mattermost.plugin-cursor_review_loop_changed" wherever
PublishWebSocketEvent is mocked (e.g., the call in reviewloop_test.go and the
other similar mocks referencing "review_loop_changed") so the tests align with
the required WebSocket event naming convention.
In `@server/reviewloop.go`:
- Around line 456-470: The WebSocket event name in publishReviewLoopChange must
use the required custom prefix; update the call in publishReviewLoopChange (the
p.API.PublishWebSocketEvent invocation) to replace the literal
"review_loop_changed" with
"custom_com.mattermost.plugin-cursor_review_loop_changed" (and ensure any other
WebSocket publishes for review_loop_changed use the same full prefix) so the
event follows the "custom_com.mattermost.plugin-cursor_<event_name>" convention.
---
Outside diff comments:
In `@server/hitl.go`:
- Around line 678-685: The LaunchAgentRequest is setting both a specific branch
name and AutoBranch which conflicts with the Cursor API; in the cursor.Target
literal (see cursor.LaunchAgentRequest and cursor.Target created in function
using sanitizeBranchName(workflow.OriginalPrompt)) remove the AutoBranch: true
field when BranchName is explicitly set and keep AutoCreatePr:
workflow.AutoCreatePR and BranchName: fmt.Sprintf("cursor/%s",
sanitizeBranchName(workflow.OriginalPrompt)); apply the same change in the
equivalent targets in server/handlers.go (around the handler that builds
LaunchAgentRequest) and server/command/command.go (around the LaunchAgentRequest
construction) so only BranchName is sent when explicit and AutoBranch is not
included.
---
Duplicate comments:
In `@server/reviewloop_test.go`:
- Around line 132-134: The mock for pluginapi.PostService.AddReaction is
returning (nil, nil) which can cause a panic because AddReaction callers
dereference the returned *model.Reaction; update each api.On("AddReaction", ...)
mock to return a non-nil *model.Reaction instance (e.g., &model.Reaction{PostId:
"trigger-1", EmojiName: "eyes", UserId: "<test-user-id>"} , nil) and do the same
for the other occurrences referenced (lines ~265-267, ~427-429, ~655-657) so the
tests supply a valid Reaction object instead of nil.
In `@server/reviewloop.go`:
- Around line 65-111: The code assumes ghClient returned by p.getGitHubClient()
is non-nil; guard against ghClient == nil before calling MarkPRReadyForReview or
RequestReviewers: if nil, log a warning (use p.API.LogWarn) that no GitHub
PAT/client is configured and skip reviewer requests, then set loop state to
awaiting_review or leave it unchanged per existing flow (ensure you still call
p.updateReviewLoopInlineStatus(loop) and p.publishReviewLoopChange(loop) so
webhooks can pick up the PR) instead of invoking methods on a nil pointer; apply
this check around the usages of ghClient (MarkPRReadyForReview and
RequestReviewers) found in the shown block.
- Around line 233-255: handlePRSynchronize can regress a terminal review loop
back to ReviewPhaseAwaitingReview; add a guard at the top of the function that
checks loop.Phase and returns early (no state mutation, no SaveReviewLoop, no
updateReviewLoopInlineStatus, no publishReviewLoopChange) when the loop is in a
terminal phase (e.g., canceled/completed/closed) so only non-terminal loops are
transitioned; keep existing updates to LastCommitSHA only if you still want to
track commits for terminal loops or skip that as well per desired behavior, and
reference ReviewPhaseAwaitingReview, handlePRSynchronize,
p.kvstore.SaveReviewLoop, p.updateReviewLoopInlineStatus, and
p.publishReviewLoopChange when making the change.
Comment on lines
+66
to
+68
| // Default mock for publishReviewLoopChange WebSocket events. | ||
| api.On("PublishWebSocketEvent", "review_loop_changed", mock.Anything, mock.Anything).Return().Maybe() | ||
|
|
There was a problem hiding this comment.
Align WebSocket event name with the required custom prefix.
Update the mocked event name to match the production format.
Proposed fix
- api.On("PublishWebSocketEvent", "review_loop_changed", mock.Anything, mock.Anything).Return().Maybe()
+ api.On("PublishWebSocketEvent", "custom_com.mattermost.plugin-cursor_review_loop_changed", mock.Anything, mock.Anything).Return().Maybe()- api.On("PublishWebSocketEvent",
- "review_loop_changed",
+ api.On("PublishWebSocketEvent",
+ "custom_com.mattermost.plugin-cursor_review_loop_changed",
mock.MatchedBy(func(data map[string]any) bool {As per coding guidelines: “WebSocket event names must use the full format custom_com.mattermost.plugin-cursor_<event_name>, for events like agent_status_change and agent_created”.
Also applies to: 601-603
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/reviewloop_test.go` around lines 66 - 68, The mock for
PublishWebSocketEvent uses the short event name "review_loop_changed" but must
use the full custom prefix; update the mocked call(s) to use the production
event name "custom_com.mattermost.plugin-cursor_review_loop_changed" wherever
PublishWebSocketEvent is mocked (e.g., the call in reviewloop_test.go and the
other similar mocks referencing "review_loop_changed") so the tests align with
the required WebSocket event naming convention.
Comment on lines
+456
to
+470
| // publishReviewLoopChange publishes a WebSocket event when a review loop phase changes. | ||
| func (p *Plugin) publishReviewLoopChange(loop *kvstore.ReviewLoop) { | ||
| p.API.PublishWebSocketEvent( | ||
| "review_loop_changed", | ||
| map[string]any{ | ||
| "review_loop_id": loop.ID, | ||
| "agent_record_id": loop.AgentRecordID, | ||
| "phase": loop.Phase, | ||
| "iteration": fmt.Sprintf("%d", loop.Iteration), | ||
| "pr_url": loop.PRURL, | ||
| "updated_at": fmt.Sprintf("%d", loop.UpdatedAt), | ||
| }, | ||
| &model.WebsocketBroadcast{UserId: loop.UserID}, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Use the required custom WebSocket event prefix.
Event names must use the full custom_com.mattermost.plugin-cursor_<event_name> format.
Proposed fix
- p.API.PublishWebSocketEvent(
- "review_loop_changed",
+ p.API.PublishWebSocketEvent(
+ "custom_com.mattermost.plugin-cursor_review_loop_changed",
map[string]any{
"review_loop_id": loop.ID,
"agent_record_id": loop.AgentRecordID,
"phase": loop.Phase,
"iteration": fmt.Sprintf("%d", loop.Iteration),
"pr_url": loop.PRURL,
"updated_at": fmt.Sprintf("%d", loop.UpdatedAt),
},
&model.WebsocketBroadcast{UserId: loop.UserID},
)As per coding guidelines: “WebSocket event names must use the full format custom_com.mattermost.plugin-cursor_<event_name>, for events like agent_status_change and agent_created”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/reviewloop.go` around lines 456 - 470, The WebSocket event name in
publishReviewLoopChange must use the required custom prefix; update the call in
publishReviewLoopChange (the p.API.PublishWebSocketEvent invocation) to replace
the literal "review_loop_changed" with
"custom_com.mattermost.plugin-cursor_review_loop_changed" (and ensure any other
WebSocket publishes for review_loop_changed use the same full prefix) so the
event follows the "custom_com.mattermost.plugin-cursor_<event_name>" convention.
This routes review-loop feedback by reviewer/source, strictly keeps CodeRabbit AI-agent prompt sections, drops non-inline non-CodeRabbit noise with explicit drop-reason logging, and adds fixture-backed regressions to keep classification/dispatch behavior stable. Co-authored-by: Cursor <cursoragent@cursor.com>
nickmisasi
changed the title
Resolve the poller merge conflict by keeping the current stale-agent cleanup path and removing obsolete conflict-era annotations while preserving review-loop handling behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/poller.go (1)
350-365:⚠️ Potential issue | 🟠 MajorPrefix agent WebSocket event names.
Current event names omit the required custom prefix, so frontend handlers won’t receive updates.
🛠️ Proposed fix
- "agent_status_change", + "custom_com.mattermost.plugin-cursor_agent_status_change",- "agent_created", + "custom_com.mattermost.plugin-cursor_agent_created",As per coding guidelines: “WebSocket event names must use the full format custom_com.mattermost.plugin-cursor_<event_name>, for events like agent_status_change and agent_created”.
Also applies to: 401-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/poller.go` around lines 350 - 365, The WebSocket event names (e.g., in publishAgentStatusChange) are missing the required custom prefix; update the first argument passed to p.API.PublishWebSocketEvent from "agent_status_change" (and any other bare names like "agent_created") to the full format "custom_com.mattermost.plugin-cursor_<event_name>" so frontend handlers receive the events—search for PublishWebSocketEvent calls in this plugin (notably publishAgentStatusChange and the agent creation emitter) and replace the raw event names with the prefixed form.server/store/kvstore/store.go (2)
12-28:⚠️ Potential issue | 🟠 MajorNew KV prefixes need to be added to the allowed list.
The added
reviewloop:,rlbypr:,rlbyagent:, andfinishedpr:keys are outside the documented prefix allowlist. Please update the KV prefix documentation/allowlist (or align names) so tooling/cleanup rules remain consistent. As per coding guidelines: "KV keys must use one of the defined prefixes:agent:,thread:,channel:,user:,agentidx:,useragentidx:,prurlidx:,branchidx:,ghdelivery:,hitl:, orhitlagent:".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/store/kvstore/store.go` around lines 12 - 28, The new KV prefixes defined as constants prefixReviewLoop, prefixRLByPR, prefixRLByAgent, and prefixFinishedWithPR are not included in the project's documented/validated KV-prefix allowlist; update that allowlist or documentation to include "reviewloop:", "rlbypr:", "rlbyagent:", and "finishedpr:" (or rename the constants to match existing allowed prefixes) so tooling and cleanup rules recognize them; locate the allowlist/validation code or docs that enumerate allowed prefixes (the place that enforces the rule "KV keys must use one of the defined prefixes") and add these four entries, ensuring the constant names prefixReviewLoop, prefixRLByPR, prefixRLByAgent, and prefixFinishedWithPR remain consistent with the updated list.
107-120:⚠️ Potential issue | 🟠 MajorDeleteAgent should also remove PR URL and branch indexes.
The method already loads the record; please also delete the
prurlidx:andbranchidx:entries to avoid stale lookups. Based on learnings: "InDeleteAgent(), clean up all index entries includingagentidx:,useragentidx:,prurlidx:, andbranchidx:keys".🛠️ Proposed fix
_ = s.client.KV.Delete(prefixAgentIdx + cursorAgentID) _ = s.client.KV.Delete(prefixFinishedWithPR + cursorAgentID) - if record != nil && record.UserID != "" { - _ = s.client.KV.Delete(prefixUserAgentIdx + record.UserID + ":" + cursorAgentID) - } + if record != nil { + if record.UserID != "" { + _ = s.client.KV.Delete(prefixUserAgentIdx + record.UserID + ":" + cursorAgentID) + } + if record.PrURL != "" { + _ = s.client.KV.Delete(prefixPRURLIdx + normalizeURL(record.PrURL)) + } + if record.TargetBranch != "" { + _ = s.client.KV.Delete(prefixBranchIdx + record.TargetBranch) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/store/kvstore/store.go` around lines 107 - 120, DeleteAgent currently removes agent, agent index, finished-with-PR and user-agent index entries but omits PR URL and branch indexes; update the store.DeleteAgent function to also delete the PR URL and branch index keys (use the same key patterns as other deletes). Specifically call s.client.KV.Delete(prefixPrURLIdx + cursorAgentID) and s.client.KV.Delete(prefixBranchIdx + cursorAgentID), and if record != nil remove any keyed indexes that include record.PrURL or record.Branch (e.g. prefixPrURLIdx + record.PrURL + ":" + cursorAgentID and prefixBranchIdx + record.Branch + ":" + cursorAgentID) consistent with how prefixUserAgentIdx is constructed.
🧹 Nitpick comments (6)
server/reviewloop_feedback.go (3)
459-465:isCodeRabbitFenceStartLineandisCodeRabbitFenceEndLineare byte-for-byte identical.Both return
strings.HasPrefix(strings.TrimSpace(line), "``\")`. While semantically distinct names are fine (Markdown fences use the same syntax for open and close), the duplication is surprising and could mislead future readers into thinking they diverge.♻️ Simplest fix: have one delegate to the other
func isCodeRabbitFenceStartLine(line string) bool { return strings.HasPrefix(strings.TrimSpace(line), "```") } func isCodeRabbitFenceEndLine(line string) bool { - return strings.HasPrefix(strings.TrimSpace(line), "```") + return isCodeRabbitFenceStartLine(line) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/reviewloop_feedback.go` around lines 459 - 465, The two functions isCodeRabbitFenceStartLine and isCodeRabbitFenceEndLine are identical; replace the duplication by having isCodeRabbitFenceEndLine delegate to isCodeRabbitFenceStartLine (or consolidate into a single exported helper) so only one implementation of the fence check exists, updating calls if needed; target the functions isCodeRabbitFenceStartLine and isCodeRabbitFenceEndLine to perform this refactor.
588-589:classifyFeedbacksilently mutatesloop.Findings— document the side effect.Line 738 writes directly to the caller's
loop.Findingspointer. The function signature gives no indication of this mutation; callers must know to persist the loop afterward or risk divergence. A doc comment would clarify intent.♻️ Suggested doc comment
+// classifyFeedback merges new review feedback candidates into the loop's finding +// history and returns a classification of new/repeated/resolved/superseded findings. +// As a side effect it updates loop.Findings with the bounded, merged result; callers +// are expected to persist the loop after this call. func classifyFeedback(loop *kvstore.ReviewLoop, candidates []reviewFeedbackCandidate, now int64) reviewFeedbackClassification {Also applies to: 738-739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/reviewloop_feedback.go` around lines 588 - 589, The classifyFeedback function silently mutates the caller's loop.Findings field; update the code by adding a doc comment above func classifyFeedback that clearly states this side effect (that classifyFeedback writes into the provided *kvstore.ReviewLoop's Findings and that callers are responsible for persisting the loop afterward to avoid divergence). Mention the affected symbols (classifyFeedback and loop.Findings) and note that callers should call the appropriate persist/save method after invoking classifyFeedback.
147-154:collectFeedbackCandidatesignores caller cancellation — consider accepting acontext.Contextparameter.The function creates a fresh
context.Background()with a hardcoded 15-second timeout, making it impossible for callers (e.g., the review-loop orchestrator) to propagate cancellation or a shorter deadline. If the plugin is being deactivated, this function can hold for the full 15 seconds.♻️ Proposed refactor: accept a context and compose with timeout
-func (p *Plugin) collectFeedbackCandidates(loop *kvstore.ReviewLoop) ([]reviewFeedbackCandidate, error) { +func (p *Plugin) collectFeedbackCandidates(ctx context.Context, loop *kvstore.ReviewLoop) ([]reviewFeedbackCandidate, error) { ghClient := p.getGitHubClient() if ghClient == nil { return nil, fmt.Errorf("GitHub client is not configured") } - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(ctx, 15*time.Second) defer cancel()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/reviewloop_feedback.go` around lines 147 - 154, collectFeedbackCandidates currently ignores caller cancellation by creating its own context.Background() with a fixed 15s timeout; change the function signature to accept a context.Context (e.g., func (p *Plugin) collectFeedbackCandidates(ctx context.Context, loop *kvstore.ReviewLoop) ([]reviewFeedbackCandidate, error)) and inside compose the caller context with a timeout using context.WithTimeout(ctx, 15*time.Second) and defer cancel(), then use that composed ctx for all GitHub/client calls; update all call sites (the review-loop orchestrator and any tests) to pass through their context so cancellations and shorter deadlines are honored.server/ghclient/client.go (2)
101-129:MarkPRReadyForReviewandGetPullRequestByBranchlack test coverage.These are the two most complex/stateful methods in this client —
MarkPRReadyForReviewhas a REST → verify → GraphQL fallback path andGetPullRequestByBranchhas a branch-scoped lookup with pagination edge cases. Both are currently untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/ghclient/client.go` around lines 101 - 129, Add unit tests for clientImpl.MarkPRReadyForReview and GetPullRequestByBranch covering all stateful paths: for MarkPRReadyForReview write tests that simulate (1) REST PullRequests.Edit succeeds and verification Get returns draft=false, (2) REST Edit returns nil but verification still shows draft=true so graphqlMarkReady is called, (3) REST Edit fails and PR has empty NodeID to assert the specific error, and (4) successful GraphQL fallback via graphqlMarkReady; stub/mock github.PullRequests.Edit, PullRequests.Get and the graphqlMarkReady call to assert behavior. For GetPullRequestByBranch add tests for single-page hit, multi-page pagination where the PR appears on a later page, no-match result, and multiple matches edge cases by mocking List/Pages responses (or using an httptest server/mock client) to exercise pagination logic and ensure expected returns.
300-313: Document the same-repo-only assumption inGetPullRequestByBranch.The
Head: owner + ":" + branchfilter only matches PRs where the head branch is in the sameownernamespace. Fork-originated PRs (where head is in a different owner's fork) would return no results. This is the correct behavior for Cursor's workflow—which only creates PRs in the configured repository—but should be documented for clarity.Consider adding a comment:
func (c *clientImpl) GetPullRequestByBranch(ctx context.Context, owner, repo, branch string) (*github.PullRequest, error) { prs, _, err := c.gh.PullRequests.List(ctx, owner, repo, &github.PullRequestListOptions{ - Head: owner + ":" + branch, + Head: owner + ":" + branch, // Only matches same-repo head branches; fork PRs are not supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/ghclient/client.go` around lines 300 - 313, Document in GetPullRequestByBranch that using Head: owner + ":" + branch intentionally limits results to PRs whose head branch lives in the same owner namespace (i.e., same-repo branches) and therefore will not match fork-originated PRs; add a short comment above the GetPullRequestByBranch function mentioning this same-repo-only assumption and that returning nil for no results is expected for forked PRs.server/ghclient/client_test.go (1)
108-121:MarkPRReadyForReviewandGetPullRequestByBranchstill need tests;ListReviewCommentspagination is also untested.The test suite is well-structured, but three methods have no coverage:
MarkPRReadyForReview: REST-first, verify, GraphQL fallback — the most complex method in the client.GetPullRequestByBranch: branch-scoped PR lookup.ListReviewCommentspagination: unlikeListReviewsandListIssueComments, this is only exercised with a single page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/ghclient/client_test.go` around lines 108 - 121, Add unit tests covering three missing flows: (1) extend TestListReviewComments with pagination by mocking two pages on "/repos/owner/repo/pulls/42/comments" (first response returns a Link header pointing to page=2 and contains one comment, second response returns another comment) then call ListReviewComments and assert aggregated length and contents; (2) add tests for GetPullRequestByBranch by mocking the branch-scoped endpoint (e.g., "/repos/owner/repo/pulls" or the specific query used by GetPullRequestByBranch) to return a matching PR and assert the returned PR fields and no error; (3) add tests for MarkPRReadyForReview exercising the REST-first path (mock the REST readiness/change endpoint to succeed and verify client behavior), the "verify" check path (mock any verification endpoint the method calls), and the GraphQL fallback path by making the REST call return an error/status that triggers the GraphQL branch (mock the GraphQL handler used by MarkPRReadyForReview) and assert the final result and that the correct endpoints were invoked; for each test use the existing setup helper (client, mux) and require/assert helpers to check requests, responses, and that pagination/fallback logic aggregates/returns expected data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/poller.go`:
- Around line 26-46: The early return after checking len(activeAgents) prevents
janitorSweep from running when there are zero active agents; move or call
p.janitorSweep() before the return so GitHub-related reconciliation runs even
when no agents are active. Specifically, in the function that calls
cleanupStaleAgents and iterates over activeAgents, ensure p.janitorSweep() is
invoked unconditionally (or at least before the early return) while keeping the
existing cleanupStaleAgents, pollSingleAgent loop, and debug logging intact.
In `@server/reviewloop.go`:
- Around line 318-323: Replace the direct p.API.LogDebug call with the
p.logDebug helper so debug output respects EnableDebugLogging; specifically
update the call in the non-CodeRabbit branch (currently invoking p.API.LogDebug
with "Non-CodeRabbit AI review received..." and fields "reviewer" ->
review.User.Login and "review_loop_id" -> loop.ID) to use p.logDebug with the
same message and key/value pairs, and also change the earlier direct
p.API.LogDebug in startReviewLoop to p.logDebug so all debug logging passes
through the config-checked helper.
In `@server/store/kvstore/store_test.go`:
- Around line 837-845: Add the four missing KV prefix entries to the "Key Prefix
Conventions" table in server/store/CLAUDE.md: document prefixReviewLoop
("reviewloop:") as "ReviewLoop record storage", prefixRLByPR ("rlbypr:") as "PR
URL → ReviewLoop ID index", prefixRLByAgent ("rlbyagent:") as "Agent record ID →
ReviewLoop ID index", and prefixFinishedWithPR ("finishedpr:") as "Index for
finished agents with PR URL (janitor cleanup)"; reference the existing constants
like ReviewPhaseCursorFixing when locating the review-loop related code to
ensure names and descriptions match the defined identifiers.
In `@server/webhook_test.go`:
- Line 354: Tests and server code are emitting unprefixed WebSocket event names
like "agent_status_change" but listeners expect prefixed names
("custom_com.mattermost.plugin-cursor_<event_name>"); update the four publisher
sites (the functions that call api.PublishWebSocketEvent in poller.go,
reviewloop.go, and hitl.go—and any helper that constructs event names) to use
the prefixed format (prefix = "custom_com.mattermost.plugin-cursor_") when
publishing events (agent_status_change, review_loop_changed, agent_created,
workflow_phase_change), and change all test mocks that expect
api.On("PublishWebSocketEvent", ...) in webhook_test.go, reviewloop_test.go,
handlers_test.go, api_test.go, hitl_test.go, and poller_test.go to use the new
full prefixed event names so tests match runtime behavior.
---
Outside diff comments:
In `@server/poller.go`:
- Around line 350-365: The WebSocket event names (e.g., in
publishAgentStatusChange) are missing the required custom prefix; update the
first argument passed to p.API.PublishWebSocketEvent from "agent_status_change"
(and any other bare names like "agent_created") to the full format
"custom_com.mattermost.plugin-cursor_<event_name>" so frontend handlers receive
the events—search for PublishWebSocketEvent calls in this plugin (notably
publishAgentStatusChange and the agent creation emitter) and replace the raw
event names with the prefixed form.
In `@server/store/kvstore/store.go`:
- Around line 12-28: The new KV prefixes defined as constants prefixReviewLoop,
prefixRLByPR, prefixRLByAgent, and prefixFinishedWithPR are not included in the
project's documented/validated KV-prefix allowlist; update that allowlist or
documentation to include "reviewloop:", "rlbypr:", "rlbyagent:", and
"finishedpr:" (or rename the constants to match existing allowed prefixes) so
tooling and cleanup rules recognize them; locate the allowlist/validation code
or docs that enumerate allowed prefixes (the place that enforces the rule "KV
keys must use one of the defined prefixes") and add these four entries, ensuring
the constant names prefixReviewLoop, prefixRLByPR, prefixRLByAgent, and
prefixFinishedWithPR remain consistent with the updated list.
- Around line 107-120: DeleteAgent currently removes agent, agent index,
finished-with-PR and user-agent index entries but omits PR URL and branch
indexes; update the store.DeleteAgent function to also delete the PR URL and
branch index keys (use the same key patterns as other deletes). Specifically
call s.client.KV.Delete(prefixPrURLIdx + cursorAgentID) and
s.client.KV.Delete(prefixBranchIdx + cursorAgentID), and if record != nil remove
any keyed indexes that include record.PrURL or record.Branch (e.g.
prefixPrURLIdx + record.PrURL + ":" + cursorAgentID and prefixBranchIdx +
record.Branch + ":" + cursorAgentID) consistent with how prefixUserAgentIdx is
constructed.
---
Duplicate comments:
In `@server/reviewloop_test.go`:
- Around line 124-126: The mock WebSocket event name uses the short identifier
"review_loop_changed"; update all occurrences of api.On(...,
"review_loop_changed", ...) to use the full custom prefix format
"custom_com.mattermost.plugin-cursor_review_loop_changed" (and likewise for any
other events referenced nearby such as agent_status_change/agent_created), so
change the string literal in the PublishWebSocketEvent mocks (e.g., the
api.On("PublishWebSocketEvent", ...) calls) to the full names to match the
required event naming convention.
In `@server/reviewloop.go`:
- Around line 326-348: handlePRSynchronize unconditionally transitions the
review loop to AwaitingReview; add an explicit phase guard so the transition
only occurs when loop.Phase == kvstore.ReviewPhaseCursorFixing (leave the
pr.Head.SHA update intact), e.g. early-return without side effects if the phase
is not CursorFixing; when the guard passes, set loop.Phase =
kvstore.ReviewPhaseAwaitingReview, append the ReviewLoopEvent with Detail
"Cursor pushed fixes", update UpdatedAt, save via
p.kvstore.SaveReviewLoop(loop), and then call
p.updateReviewLoopInlineStatus(loop) and p.publishReviewLoopChange(loop).
- Around line 61-166: startReviewLoop currently calls p.getGitHubClient() and
immediately uses it, which can panic if it returns nil; add a defensive
nil-check after ghClient := p.getGitHubClient() and before any ghClient method
calls (e.g., MarkPRReadyForReview, RequestReviewers) and return a clear error
(or log and return) if ghClient == nil; ensure the error message references the
PR URL and the function name (startReviewLoop) so callers and logs can trace the
failure, and keep the rest of the flow unchanged (including
cleanup/DeleteReviewLoop) when aborting.
- Around line 711-725: The WebSocket event name in publishReviewLoopChange must
follow the full custom format; update the first argument passed to
p.API.PublishWebSocketEvent in function publishReviewLoopChange from
"review_loop_changed" to
"custom_com.mattermost.plugin-cursor_review_loop_changed" so the event name
matches the required naming convention while leaving the payload and
WebsocketBroadcast call unchanged.
---
Nitpick comments:
In `@server/ghclient/client_test.go`:
- Around line 108-121: Add unit tests covering three missing flows: (1) extend
TestListReviewComments with pagination by mocking two pages on
"/repos/owner/repo/pulls/42/comments" (first response returns a Link header
pointing to page=2 and contains one comment, second response returns another
comment) then call ListReviewComments and assert aggregated length and contents;
(2) add tests for GetPullRequestByBranch by mocking the branch-scoped endpoint
(e.g., "/repos/owner/repo/pulls" or the specific query used by
GetPullRequestByBranch) to return a matching PR and assert the returned PR
fields and no error; (3) add tests for MarkPRReadyForReview exercising the
REST-first path (mock the REST readiness/change endpoint to succeed and verify
client behavior), the "verify" check path (mock any verification endpoint the
method calls), and the GraphQL fallback path by making the REST call return an
error/status that triggers the GraphQL branch (mock the GraphQL handler used by
MarkPRReadyForReview) and assert the final result and that the correct endpoints
were invoked; for each test use the existing setup helper (client, mux) and
require/assert helpers to check requests, responses, and that
pagination/fallback logic aggregates/returns expected data.
In `@server/ghclient/client.go`:
- Around line 101-129: Add unit tests for clientImpl.MarkPRReadyForReview and
GetPullRequestByBranch covering all stateful paths: for MarkPRReadyForReview
write tests that simulate (1) REST PullRequests.Edit succeeds and verification
Get returns draft=false, (2) REST Edit returns nil but verification still shows
draft=true so graphqlMarkReady is called, (3) REST Edit fails and PR has empty
NodeID to assert the specific error, and (4) successful GraphQL fallback via
graphqlMarkReady; stub/mock github.PullRequests.Edit, PullRequests.Get and the
graphqlMarkReady call to assert behavior. For GetPullRequestByBranch add tests
for single-page hit, multi-page pagination where the PR appears on a later page,
no-match result, and multiple matches edge cases by mocking List/Pages responses
(or using an httptest server/mock client) to exercise pagination logic and
ensure expected returns.
- Around line 300-313: Document in GetPullRequestByBranch that using Head: owner
+ ":" + branch intentionally limits results to PRs whose head branch lives in
the same owner namespace (i.e., same-repo branches) and therefore will not match
fork-originated PRs; add a short comment above the GetPullRequestByBranch
function mentioning this same-repo-only assumption and that returning nil for no
results is expected for forked PRs.
In `@server/reviewloop_feedback.go`:
- Around line 459-465: The two functions isCodeRabbitFenceStartLine and
isCodeRabbitFenceEndLine are identical; replace the duplication by having
isCodeRabbitFenceEndLine delegate to isCodeRabbitFenceStartLine (or consolidate
into a single exported helper) so only one implementation of the fence check
exists, updating calls if needed; target the functions
isCodeRabbitFenceStartLine and isCodeRabbitFenceEndLine to perform this
refactor.
- Around line 588-589: The classifyFeedback function silently mutates the
caller's loop.Findings field; update the code by adding a doc comment above func
classifyFeedback that clearly states this side effect (that classifyFeedback
writes into the provided *kvstore.ReviewLoop's Findings and that callers are
responsible for persisting the loop afterward to avoid divergence). Mention the
affected symbols (classifyFeedback and loop.Findings) and note that callers
should call the appropriate persist/save method after invoking classifyFeedback.
- Around line 147-154: collectFeedbackCandidates currently ignores caller
cancellation by creating its own context.Background() with a fixed 15s timeout;
change the function signature to accept a context.Context (e.g., func (p
*Plugin) collectFeedbackCandidates(ctx context.Context, loop
*kvstore.ReviewLoop) ([]reviewFeedbackCandidate, error)) and inside compose the
caller context with a timeout using context.WithTimeout(ctx, 15*time.Second) and
defer cancel(), then use that composed ctx for all GitHub/client calls; update
all call sites (the review-loop orchestrator and any tests) to pass through
their context so cancellations and shorter deadlines are honored.
Comment on lines
+26
to
+46
|
|
||
| // Janitor sweep: reconcile GitHub-related state for finished agents. | ||
| p.janitorSweep() | ||
| } |
There was a problem hiding this comment.
Don’t skip janitorSweep when there are zero active agents.
The early return at Line 31 prevents reconciliation when the system is idle, so finished PRs with missed webhooks never get bootstrapped into review loops. Call janitorSweep() before returning.
🛠️ Proposed fix
- if len(activeAgents) == 0 {
- return
- }
+ if len(activeAgents) == 0 {
+ p.janitorSweep()
+ return
+ }📝 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.
Suggested change
| cleaned := p.cleanupStaleAgents(activeAgents, staleAgentMaxAge) | |
| if cleaned > 0 { | |
| p.API.LogInfo("Cleaned up stale agents", "count", cleaned, "max_age", staleAgentMaxAge.String()) | |
| } | |
| if len(activeAgents) == 0 { | |
| return | |
| } | |
| p.API.LogDebug("Polling agent statuses", "count", len(activeAgents)) | |
| for _, record := range activeAgents { | |
| if cursor.AgentStatus(record.Status).IsTerminal() { | |
| continue | |
| } | |
| p.pollSingleAgent(record) | |
| } | |
| // Janitor sweep: reconcile GitHub-related state for finished agents. | |
| p.janitorSweep() | |
| } | |
| cleaned := p.cleanupStaleAgents(activeAgents, staleAgentMaxAge) | |
| if cleaned > 0 { | |
| p.API.LogInfo("Cleaned up stale agents", "count", cleaned, "max_age", staleAgentMaxAge.String()) | |
| } | |
| if len(activeAgents) == 0 { | |
| p.janitorSweep() | |
| return | |
| } | |
| p.API.LogDebug("Polling agent statuses", "count", len(activeAgents)) | |
| for _, record := range activeAgents { | |
| if cursor.AgentStatus(record.Status).IsTerminal() { | |
| continue | |
| } | |
| p.pollSingleAgent(record) | |
| } | |
| // Janitor sweep: reconcile GitHub-related state for finished agents. | |
| p.janitorSweep() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/poller.go` around lines 26 - 46, The early return after checking
len(activeAgents) prevents janitorSweep from running when there are zero active
agents; move or call p.janitorSweep() before the return so GitHub-related
reconciliation runs even when no agents are active. Specifically, in the
function that calls cleanupStaleAgents and iterates over activeAgents, ensure
p.janitorSweep() is invoked unconditionally (or at least before the early
return) while keeping the existing cleanupStaleAgents, pollSingleAgent loop, and
debug logging intact.
Comment on lines
+318
to
+323
| // Non-CodeRabbit bot reviews are informational only. | ||
| p.API.LogDebug("Non-CodeRabbit AI review received, not driving state transition", | ||
| "reviewer", review.User.Login, | ||
| "review_loop_id", loop.ID, | ||
| ) | ||
| return nil |
There was a problem hiding this comment.
Use p.logDebug instead of p.API.LogDebug for debug logging.
Direct LogDebug calls bypass the EnableDebugLogging gate. Please route these through p.logDebug (also applies to the earlier LogDebug in startReviewLoop). As per coding guidelines: "Debug logging is toggled via EnableDebugLogging config setting. Use p.logDebug() helper that wraps p.API.LogDebug with config check".
🔧 Suggested adjustment
- p.API.LogDebug("Non-CodeRabbit AI review received, not driving state transition",
+ p.logDebug("Non-CodeRabbit AI review received, not driving state transition",
"reviewer", review.User.Login,
"review_loop_id", loop.ID,
)📝 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.
Suggested change
| // Non-CodeRabbit bot reviews are informational only. | |
| p.API.LogDebug("Non-CodeRabbit AI review received, not driving state transition", | |
| "reviewer", review.User.Login, | |
| "review_loop_id", loop.ID, | |
| ) | |
| return nil | |
| // Non-CodeRabbit bot reviews are informational only. | |
| p.logDebug("Non-CodeRabbit AI review received, not driving state transition", | |
| "reviewer", review.User.Login, | |
| "review_loop_id", loop.ID, | |
| ) | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/reviewloop.go` around lines 318 - 323, Replace the direct
p.API.LogDebug call with the p.logDebug helper so debug output respects
EnableDebugLogging; specifically update the call in the non-CodeRabbit branch
(currently invoking p.API.LogDebug with "Non-CodeRabbit AI review received..."
and fields "reviewer" -> review.User.Login and "review_loop_id" -> loop.ID) to
use p.logDebug with the same message and key/value pairs, and also change the
earlier direct p.API.LogDebug in startReviewLoop to p.logDebug so all debug
logging passes through the config-checked helper.
Comment on lines
+837
to
+845
| func TestReviewPhaseConstants(t *testing.T) { | ||
| assert.Equal(t, "requesting_review", ReviewPhaseRequestingReview) | ||
| assert.Equal(t, "awaiting_review", ReviewPhaseAwaitingReview) | ||
| assert.Equal(t, "cursor_fixing", ReviewPhaseCursorFixing) | ||
| assert.Equal(t, "approved", ReviewPhaseApproved) | ||
| assert.Equal(t, "human_review", ReviewPhaseHumanReview) | ||
| assert.Equal(t, "complete", ReviewPhaseComplete) | ||
| assert.Equal(t, "max_iterations", ReviewPhaseMaxIterations) | ||
| assert.Equal(t, "failed", ReviewPhaseFailed) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Find ReviewPhaseCursorFixing definition
echo "=== Searching for ReviewPhaseCursorFixing definition ==="
rg -n "ReviewPhaseCursorFixing\s*=" --type=go
# 2. Find new KV prefix definitions
echo ""
echo "=== Searching for new KV prefix definitions ==="
rg -n "prefixReviewLoop|prefixRLByPR|prefixRLByAgent|prefixFinishedWithPR" --type=go -B 1 -A 1
# 3. Check server/store/kvstore/CLAUDE.md for prefix documentation
echo ""
echo "=== Checking CLAUDE.md for prefix list ==="
if [ -f "server/store/kvstore/CLAUDE.md" ]; then
cat server/store/kvstore/CLAUDE.md
else
echo "CLAUDE.md not found in server/store/kvstore/"
fi
# 4. Check server/store/CLAUDE.md
echo ""
echo "=== Checking server/store/CLAUDE.md ==="
if [ -f "server/store/CLAUDE.md" ]; then
cat server/store/CLAUDE.md
else
echo "CLAUDE.md not found in server/store/"
fiRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 18646
Update server/store/CLAUDE.md to document the four new KV prefixes.
The constants ReviewPhaseCursorFixing, prefixReviewLoop, prefixRLByPR, prefixRLByAgent, and prefixFinishedWithPR are all properly defined in the codebase. However, the four review loop–related prefixes (reviewloop:, rlbypr:, rlbyagent:, finishedpr:) are not documented in the "Key Prefix Conventions" table in server/store/CLAUDE.md. Per the coding guidelines, all KV prefixes must be documented there. Add these prefixes to the table:
reviewloop:— ReviewLoop record storagerlbypr:— PR URL → ReviewLoop ID indexrlbyagent:— Agent record ID → ReviewLoop ID indexfinishedpr:— Index for finished agents with PR URL (janitor cleanup)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/store/kvstore/store_test.go` around lines 837 - 845, Add the four
missing KV prefix entries to the "Key Prefix Conventions" table in
server/store/CLAUDE.md: document prefixReviewLoop ("reviewloop:") as "ReviewLoop
record storage", prefixRLByPR ("rlbypr:") as "PR URL → ReviewLoop ID index",
prefixRLByAgent ("rlbyagent:") as "Agent record ID → ReviewLoop ID index", and
prefixFinishedWithPR ("finishedpr:") as "Index for finished agents with PR URL
(janitor cleanup)"; reference the existing constants like
ReviewPhaseCursorFixing when locating the review-loop related code to ensure
names and descriptions match the defined identifiers.
| })).Return(nil) | ||
|
|
||
| // WebSocket event for RHS update. | ||
| api.On("PublishWebSocketEvent", "agent_status_change", mock.Anything, mock.Anything).Return() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the webhook_test.go file and check the specified lines
fd webhook_test.goRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 101
🏁 Script executed:
# Check specific lines in webhook_test.go where the issue is reported
cat -n server/webhook_test.go | sed -n '350,360p;400,410p;1020,1030p;1134,1144p;1191,1201p;1304,1314p;1414,1424p'Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 4125
🏁 Script executed:
# Search for all PublishWebSocketEvent calls in server code
rg -n 'PublishWebSocketEvent' server/ --type goRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 4910
🏁 Script executed:
# Search for the actual event names being used in server implementations
rg -n 'agent_status_change|review_loop_changed|agent_created' server/ --type go -B2 -A2Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 7224
🏁 Script executed:
# Search for WebSocket event listeners in webapp
find webapp -type f -name "*.ts" -o -name "*.tsx" | head -20
rg -n 'agent_status_change|review_loop_changed|agent_created' webapp/ --type ts --type tsx -B2 -A2Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 834
🏁 Script executed:
# Search for WebSocket listener registration patterns in webapp
rg -n 'addEventListener|on\(' webapp/ --type ts --type tsx | grep -i 'websocket\|event' | head -20Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 106
🏁 Script executed:
# Check for custom_com.mattermost patterns in webapp
rg -n 'custom_com.mattermost' webapp/ --type ts --type tsxRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 106
🏁 Script executed:
# Search for event names in webapp without type filter
rg -n 'agent_status_change|review_loop_changed|agent_created' webapp/ -B2 -A2Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 2746
🏁 Script executed:
# Search for custom_com.mattermost pattern
rg -n 'custom_com\.mattermost' webapp/ -B2 -A2Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 430
🏁 Script executed:
# Look for websocket event listeners in webapp
rg -n 'registerWebSocketEvents|addEventListener|WebSocketClient' webapp/ -B2 -A2 | head -50Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 61
🏁 Script executed:
# Search for how events are handled in webapp
rg -n 'PublishWebSocketEvent|WebSocketClient|websocket' webapp/src/ -B1 -A1 | head -40Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 2319
🏁 Script executed:
# Check manifest to understand the plugin ID
cat webapp/src/manifest.ts 2>/dev/null || cat webapp/src/manifest.js 2>/dev/null || rg -n 'manifest.*id' webapp/src/ | head -10Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 984
🏁 Script executed:
# Verify the complete event name construction
cat webapp/src/websocket.tsRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 1610
🏁 Script executed:
# Get manifest.id value to confirm the full event name format
rg -n "id:" webapp/src/manifest.* -A1 | head -5Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 61
🏁 Script executed:
# Check server code for where events are being emitted
rg -n 'PublishWebSocketEvent\(' server/ --type go -B3 -A3Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 1770
🏁 Script executed:
# Check the manifest file for the plugin ID
cat webapp/src/manifest.ts 2>/dev/null || find webapp -name manifest.ts -o -name manifest.js | xargs catRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 61
🏁 Script executed:
# Search for all event publishing calls to see the complete picture
rg -n 'publishAgentStatusChange|publishAgentCreated|publishReviewLoopChange|publishWorkflowPhaseChange' server/ --type goRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 3246
🏁 Script executed:
# Verify no server code is already using the prefixed format
rg -n 'custom_com\.mattermost\.plugin-cursor' server/ --type goRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 61
🏁 Script executed:
# Count how many functions emit unprefixed events
rg -n 'PublishWebSocketEvent' server/*.go | grep -v testRepository: nickmisasi/mattermost-plugin-cursor
Length of output: 284
🏁 Script executed:
# Verify the exact impact by checking all four event publishing functions
cat server/poller.go | sed -n '350,360p;401,411p'
cat server/reviewloop.go | sed -n '710,720p'
cat server/hitl.go | sed -n '630,640p'Repository: nickmisasi/mattermost-plugin-cursor
Length of output: 1920
Server WebSocket events must use the prefixed event name format.
The server emits unprefixed event names (agent_status_change, review_loop_changed, agent_created, workflow_phase_change), but webapp listeners expect the full format (custom_com.mattermost.plugin-cursor_<event_name>). This breaks real-time updates. Update the four event publishing functions in poller.go, reviewloop.go, and hitl.go to emit prefixed names, and align test mocks accordingly across all test files (webhook_test.go, reviewloop_test.go, handlers_test.go, api_test.go, hitl_test.go, poller_test.go).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/webhook_test.go` at line 354, Tests and server code are emitting
unprefixed WebSocket event names like "agent_status_change" but listeners expect
prefixed names ("custom_com.mattermost.plugin-cursor_<event_name>"); update the
four publisher sites (the functions that call api.PublishWebSocketEvent in
poller.go, reviewloop.go, and hitl.go—and any helper that constructs event
names) to use the prefixed format (prefix =
"custom_com.mattermost.plugin-cursor_") when publishing events
(agent_status_change, review_loop_changed, agent_created,
workflow_phase_change), and change all test mocks that expect
api.On("PublishWebSocketEvent", ...) in webhook_test.go, reviewloop_test.go,
handlers_test.go, api_test.go, hitl_test.go, and poller_test.go to use the new
full prefixed event names so tests match runtime behavior.
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)
server/poller.go (1)
428-449:⚠️ Potential issue | 🟡 MinorUI side-effects fire before
SaveAgent, causing duplicate "stopped" notifications on KV failure.
handleAgentStopped(agent)(posts thread message, swaps reactions, updates bot-reply attachment) runs at line 440 beforep.kvstore.SaveAgent(agent)at line 441. IfSaveAgentreturns an error, the thread message has already been posted but the KV record remains in CREATING/RUNNING. On the next poll,cleanupStaleAgentstreats the agent as still stale and fireshandleAgentStoppedagain — users see a duplicate "Agent was stopped." message.Fix: persist first, then emit notifications:
🛠️ Proposed fix
agent.Status = string(cursor.AgentStatusStopped) agent.UpdatedAt = now.UnixMilli() - p.handleAgentStopped(agent) if err := p.kvstore.SaveAgent(agent); err != nil { p.API.LogError("Failed to mark stale agent as stopped", "agent_id", agent.CursorAgentID, "error", err.Error()) continue } + p.handleAgentStopped(agent) p.updateBotPostProps(agent) p.publishAgentStatusChange(agent) cleaned++Additionally, the staleness check at line 433–435 uses
agent.CreatedAtas the age basis. An agent that was queued for a while before transitioning to RUNNING (e.g.,CreatedAtis 25 h ago, lastUpdatedAtis 30 min ago) will be incorrectly stopped mid-run. Considermax(CreatedAt, UpdatedAt)orUpdatedAtalone as the epoch for the age comparison.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/poller.go` around lines 428 - 449, The UI side-effects (handleAgentStopped, updateBotPostProps, publishAgentStatusChange) are executed before persisting the stopped state (p.kvstore.SaveAgent), causing duplicate notifications if SaveAgent fails — change the order so you call p.kvstore.SaveAgent(agent) immediately after setting agent.Status and agent.UpdatedAt and only call handleAgentStopped/updateBotPostProps/publishAgentStatusChange after SaveAgent succeeds (fall back to logging/continue on SaveAgent error). Also update the staleness test to use the later of agent.CreatedAt and agent.UpdatedAt (or just agent.UpdatedAt) when comparing against maxAge so recently-updated agents aren’t incorrectly treated as stale in cleanupStaleAgents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/poller.go`:
- Around line 70-73: The call to p.kvstore.GetReviewLoopByPRURL is ignoring its
error which can make existing review loops invisible on transient KV errors;
update the logic around GetReviewLoopByPRURL in poller.go to check the returned
error (from p.kvstore.GetReviewLoopByPRURL(agent.PrURL)) and handle non-nil
errors before calling startReviewLoop: on transient/read errors log the error
(or return/retry/skip this agent iteration) and avoid falling through to
startReviewLoop, ensuring you only start a new loop when existing != nil is
conclusively false; reference GetReviewLoopByPRURL, p.kvstore, and
startReviewLoop when making the change.
---
Outside diff comments:
In `@server/poller.go`:
- Around line 428-449: The UI side-effects (handleAgentStopped,
updateBotPostProps, publishAgentStatusChange) are executed before persisting the
stopped state (p.kvstore.SaveAgent), causing duplicate notifications if
SaveAgent fails — change the order so you call p.kvstore.SaveAgent(agent)
immediately after setting agent.Status and agent.UpdatedAt and only call
handleAgentStopped/updateBotPostProps/publishAgentStatusChange after SaveAgent
succeeds (fall back to logging/continue on SaveAgent error). Also update the
staleness test to use the later of agent.CreatedAt and agent.UpdatedAt (or just
agent.UpdatedAt) when comparing against maxAge so recently-updated agents aren’t
incorrectly treated as stale in cleanupStaleAgents.
---
Duplicate comments:
In `@server/poller.go`:
- Around line 26-45: The janitorSweep() call is skipped when activeAgents is
empty because of the early return after checking len(activeAgents) == 0; move or
duplicate the janitorSweep invocation so it always runs after cleanupStaleAgents
regardless of activeAgents length. Specifically, ensure janitorSweep() is
invoked before the early return (or remove the early return and only skip the
polling loop), keeping the existing cleanupStaleAgents(...) call, the
len(activeAgents) check used to decide whether to poll with
pollSingleAgent(...), and the terminal-status check
cursor.AgentStatus(...).IsTerminal() logic intact.
Comment on lines
+70
to
+73
| existing, _ := p.kvstore.GetReviewLoopByPRURL(agent.PrURL) | ||
| if existing != nil { | ||
| continue // Loop already exists; nothing to reconcile. | ||
| } |
There was a problem hiding this comment.
Silently discarding the GetReviewLoopByPRURL error can cause a duplicate review loop.
existing, _ := p.kvstore.GetReviewLoopByPRURL(agent.PrURL)If the KV store returns a transient error, existing is nil and the code falls through to startReviewLoop. This may create a second review loop for a PR that already has one, depending on what startReviewLoop does with a conflicting state.
🛠️ Proposed fix
- existing, _ := p.kvstore.GetReviewLoopByPRURL(agent.PrURL)
+ existing, err := p.kvstore.GetReviewLoopByPRURL(agent.PrURL)
+ if err != nil {
+ p.API.LogError("Janitor: failed to check existing review loop",
+ "error", err.Error(),
+ "pr_url", agent.PrURL,
+ )
+ continue
+ }
if existing != nil {
continue // Loop already exists; nothing to reconcile.
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/poller.go` around lines 70 - 73, The call to
p.kvstore.GetReviewLoopByPRURL is ignoring its error which can make existing
review loops invisible on transient KV errors; update the logic around
GetReviewLoopByPRURL in poller.go to check the returned error (from
p.kvstore.GetReviewLoopByPRURL(agent.PrURL)) and handle non-nil errors before
calling startReviewLoop: on transient/read errors log the error (or
return/retry/skip this agent iteration) and avoid falling through to
startReviewLoop, ensuring you only start a new loop when existing != nil is
conclusively false; reference GetReviewLoopByPRURL, p.kvstore, and
startReviewLoop when making the change.
nickmisasi
commented
Feb 19, 2026
| @@ -0,0 +1,1020 @@ | |||
| package main | |||
Collaborator
Author
There was a problem hiding this comment.
I built this out in the name of token saving.. But it's pretty complex and I don't love it. I am thinking the best bet will be to refactor this to just ask Agents to parse out the feedback to be sent along. Will dogfood this implementation a bit first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prompt for AI AgentsandPrompt for all review comments with AI agentssections only.review_commentpass-through with light normalization.review_body,issue_comment) with explicit debug drop-reason logging.Key Changes
ReviewLooplifecycle/state wiring across poller + webhook + API surfaces.server/reviewloop_feedback.go.server/reviewloop.go.Test Plan
go test ./server/... -count=1cd webapp && npm test -- --watchAll=falsemake check-styleSummary by CodeRabbit
New Features
Bug Fixes
Chores