Add HITL workflow with context review and plan loop - #3
Merged
Conversation
Implements two-stage human-in-the-loop verification before agent implementation: - Context Review: User approves enriched context before launching planner - Plan Loop: User approves/iterates on implementation plan before execution Key changes: - New HITLWorkflow type and KV store methods for workflow state management - Phase constants: context_review, planning, plan_review, implementing, rejected, complete - HITL orchestration functions in server/hitl.go with comprehensive tests - Attachment builders for context and plan review UI (PostAction buttons + thread replies) - Configuration flags: EnableContextReview, EnablePlanLoop (global + user settings + per-mention) - Parser flags: --no-review, --no-plan, --direct for opt-out control - Thread mapping convention: values starting with "hitl:" are workflow IDs - Plan iteration creates NEW planner agents (follow-ups only work on RUNNING agents) - Planner constraints: autoCreatePr:false, autoBranch:false, read-only system prompt - PhaseBadge and PhaseProgress components for RHS visualization - WebSocket event handling for phase transitions - Full test coverage including ESLint fixes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Human-In-The-Loop (HITL) workflow: introduces workflow orchestration, KV persistence and mappings, API endpoints (archive/unarchive, workflows, HITL responses), parser flags for HITL, message attachment builders, planner/implementer agent lifecycle, and frontend UI/state updates to visualize and manage workflow phases. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Thread as "Mattermost Thread"
participant Plugin as "Cursor Plugin"
participant KV as "KV Store"
participant Planner as "Planner Agent"
participant Impl as "Implementer Agent"
participant CursorAPI as "Cursor API"
User->>Thread: Mention `@cursor` request
Thread->>Plugin: Deliver post event
Plugin->>Plugin: parse mention, resolve HITL flags
alt Context review required
Plugin->>KV: Save HITLWorkflow (context_review)
Plugin->>Thread: Post context review attachment
User->>Thread: Click Accept/Reject
Thread->>Plugin: Button action
Plugin->>KV: Update workflow phase
alt Plan loop enabled
Plugin->>Plugin: startPlanLoop
Plugin->>Planner: Launch planner agent
Planner->>CursorAPI: Run planning agent
Planner->>Plugin: Return plan
Plugin->>Thread: Post plan review attachment
User->>Thread: Accept plan
Plugin->>KV: Set phase = implementing
Plugin->>Impl: Launch implementer agent
else Plan loop skipped
Plugin->>Impl: Launch implementer agent
end
else Skip context review
Plugin->>Planner/Impl: Launch directly (plan or implement)
end
Impl->>CursorAPI: Execute implementation
Impl->>Plugin: Finish
Plugin->>KV: Mark workflow complete
Plugin->>Thread: Post completion & publish websocket phase change
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/parser/parser.go (1)
92-99:⚠️ Potential issue | 🟠 MajorTrim remainder after flag stripping to keep bracketed options working.
If a flag appears before a bracketed block (e.g.,--no-review [repo=...]), the leading space preventsbracketedRefrom matching. A quick trim fixes this.🔧 Suggested fix
- remainder = extractFlags(remainder, result) + remainder = extractFlags(remainder, result) + remainder = strings.TrimSpace(remainder)
🤖 Fix all issues with AI agents
In `@server/api.go`:
- Around line 674-705: The code paths inside the outer switch on action lack
default handlers for unexpected phase values, so add default cases in the inner
switches for both "accept" and "reject" to log the unknown phase (use
p.API.LogError with context like "Unknown HITL phase" and include phase and
action) and call p.writePostActionResponseAttachment(w, nil) to return a no-op
response; locate the inner switches that check kvstore.PhaseContextReview and
kvstore.PhasePlanReview and add these default branches next to the existing
cases so the request always gets a response even for unexpected phase values.
- Around line 476-496: The current flow archives and marks the agent stopped
even if p.getCursorClient() returns nil or cursorClient.StopAgent fails; change
the logic in the handler so you require a successful stop before mutating
record.Status or record.Archived and before saving: call p.getCursorClient(), if
cursorClient is nil log the error via p.API.LogError and return an HTTP 500
without modifying record; otherwise call cursorClient.StopAgent(ctx, agentID)
and if apiErr != nil log and return HTTP 500 (do not set record.Status or
record.Archived); only after StopAgent returns nil set record.Status =
string(cursor.AgentStatusStopped), set record.Archived = true, update UpdatedAt,
then call p.kvstore.SaveAgent and handle its error as before (p.API.LogError and
return 500).
- Around line 725-732: In sendEphemeralToActionUser, fetch the post for
request.PostId (use p.API.GetPost or equivalent) and if the returned post has a
non-empty RootId use that as the RootId for the ephemeral post; otherwise fall
back to request.PostId; ensure you handle a nil/error response from GetPost
gracefully (log or ignore) and still send the ephemeral using the chosen rootId,
leaving UserId, ChannelId and Message as before.
In `@server/attachments/attachments.go`:
- Around line 179-191: The Fallback field on the Slack attachment currently uses
the unbounded plan string while Text is truncated (maxPlanLen), which can exceed
post size limits; update the code that builds the attachment (the block using
maxPlanLen and returning &model.SlackAttachment) to produce a truncated fallback
matching Text (e.g., apply the same truncation logic to create fallbackText and
use Fallback: "Plan review: "+fallbackText) so both Text and Fallback are safely
capped and include the "... (plan truncated ...)" marker when truncated.
In `@server/cursor/types.go`:
- Around line 56-62: The Target struct's JSON tags are inconsistent:
AutoCreatePr currently has `omitempty` while AutoBranch does not, causing false
boolean values (AutoCreatePr: false) to be omitted when marshaling; update the
struct definition for Target by removing `omitempty` from the AutoCreatePr
field's json tag so both AutoCreatePr and AutoBranch are always emitted (refer
to the Target type and the fields AutoCreatePr and AutoBranch to locate the
change).
In `@server/hitl.go`:
- Around line 425-437: When GetConversation returns an error you update
workflow.Phase and save with p.kvstore.SaveWorkflow but never notify clients,
leaving the UI stale; after saving the workflow (in the error branch where you
call p.postBotReplyInThread and _ = p.kvstore.SaveWorkflow(workflow)) call the
same websocket/notification publish function used elsewhere in this file (the
routine that broadcasts workflow state updates) to emit the updated workflow to
clients so the UI reflects PhasePlanReview—i.e., save the workflow then publish
the workflow update via the existing publish/broadcast method.
In `@server/parser/parser.go`:
- Around line 48-53: The in-repo regex inRepoRe no longer accepts short repo
names (e.g., "in backend-api"); update its pattern so it captures either
owner/repo or a single short name by allowing an optional "/owner" segment
(e.g., change inRepoRe to match [a-zA-Z0-9._-]+(?:/[a-zA-Z0-9._-]+)?), then
run/update the tests that assert natural-language "in <repo>" parsing to include
short-name cases; ensure the Repository field assignment logic that reads
matches from inRepoRe continues to work unchanged.
In `@server/poller.go`:
- Around line 299-305: The code silently ignores errors from
p.kvstore.SaveWorkflow in the PhaseImplementing case which can lead to UI/state
mismatch if publishWorkflowPhaseChange succeeds; change it to capture the error
returned by p.kvstore.SaveWorkflow(workflow), log the error (using the same
logger pattern used elsewhere in this file) and handle it consistently (e.g.,
return early or still publish but include error context) so persistence failures
are observable; reference p.kvstore.SaveWorkflow, workflow, and
p.publishWorkflowPhaseChange when making the change.
🧹 Nitpick comments (6)
server/dialog.go (1)
78-95: Consider extracting a helper to reduce duplication.The switch pattern for parsing bool pointers is duplicated. A small helper would make this DRY and easier to maintain if more toggles are added.
♻️ Optional refactor
+// parseBoolPointer converts "true"/"false" strings to *bool, returning nil for empty/other values. +func parseBoolPointer(s string) *bool { + switch s { + case "true": + b := true + return &b + case "false": + b := false + return &b + default: + return nil + } +} func (p *Plugin) handleSettingsDialogSubmission(w http.ResponseWriter, r *http.Request) { // ... existing code ... - switch userEnableContextReview { - case "true": - b := true - userSettingsToSave.EnableContextReview = &b - case "false": - b := false - userSettingsToSave.EnableContextReview = &b - } - // default: nil (use global default) - - switch userEnablePlanLoop { - case "true": - b := true - userSettingsToSave.EnablePlanLoop = &b - case "false": - b := false - userSettingsToSave.EnablePlanLoop = &b - } + userSettingsToSave.EnableContextReview = parseBoolPointer(userEnableContextReview) + userSettingsToSave.EnablePlanLoop = parseBoolPointer(userEnablePlanLoop)webapp/src/components/rhs/AgentCard.tsx (1)
109-125: Use native Mattermost button classes instead of custom CSS.The archive/unarchive buttons use a custom
cursor-agent-card-archive-btnclass. Per coding guidelines, use only native Mattermost button CSS classes.Suggested fix
- <button - className='cursor-agent-card-archive-btn' - onClick={onArchive} - title='Archive agent' - > - {'Archive'} - </button> + <button + className='btn btn-link' + onClick={onArchive} + title='Archive agent' + > + {'Archive'} + </button>Apply the same change to the Unarchive button.
As per coding guidelines: "Use only native Mattermost button CSS classes: btn btn-primary, btn btn-danger, btn btn-link, btn btn-tertiary; do not create custom button CSS"
server/poller_test.go (1)
380-385: Remove duplicate GetConfig mock.
GetConfigis already mocked insetupPollerPlugin(lines 48-52). This duplicate mock is unnecessary.Suggested fix
func TestPoller_PlannerFinished_RoutesToWorkflow(t *testing.T) { p, api, cursorClient, store := setupPollerPlugin(t) - siteURL := "http://localhost:8065" - api.On("GetConfig").Return(&model.Config{ - ServiceSettings: model.ServiceSettings{ - SiteURL: &siteURL, - }, - }).Maybe() - record := &kvstore.AgentRecord{server/handlers.go (1)
259-268: Consider rollback on SetThreadWorkflow failure.If
SetThreadWorkflowfails afterSaveWorkflowsucceeds (lines 263-265), the workflow is saved but not mapped to the thread. This creates an orphaned workflow that users cannot interact with.Consider deleting the workflow if the thread mapping fails:
Suggested fix
if err := p.kvstore.SaveWorkflow(workflow); err != nil { p.API.LogError("Failed to save HITL workflow for plan loop", "error", err.Error()) // Fall through to direct launch. } else { if err := p.kvstore.SetThreadWorkflow(rootID, workflow.ID); err != nil { p.API.LogError("Failed to set thread workflow mapping", "error", err.Error()) + // Clean up orphaned workflow + _ = p.kvstore.DeleteWorkflow(workflow.ID) + // Fall through to direct launch + } else { + p.startPlanLoop(workflow) + return } - p.startPlanLoop(workflow) - return }webapp/src/components/common/styles.css (1)
247-260: Remove custom archive button styles if using native classes.This custom button styling should be removed if
AgentCard.tsxis updated to use native Mattermost button classes (btn btn-link).server/hitl.go (1)
661-668: Consider prefixing HITL branch names to avoid collisions.
/cursorlaunches use acursor/prefix; reusing that here keeps branch naming consistent and reduces conflicts with existing branches.🛠️ Suggested tweak
- BranchName: sanitizeBranchName(workflow.OriginalPrompt), + BranchName: fmt.Sprintf("cursor/%s", sanitizeBranchName(workflow.OriginalPrompt)),
Comment on lines
+48
to
53
| inlineOptRe = regexp.MustCompile(`(?i)\b(repo|branch|model|autopr|review|plan)=(\S+)`) | ||
| inRepoRe = regexp.MustCompile(`(?i)\bin\s+([a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+)\s*,?`) | ||
| withModelRe = regexp.MustCompile(`(?i)\bwith\s+([a-zA-Z0-9._-]+)\s*,?`) | ||
| multiSpace = regexp.MustCompile(`\s{2,}`) | ||
| flagRe = regexp.MustCompile(`(?i)--(?:no-review|no-plan|direct)\b`) | ||
| ) |
There was a problem hiding this comment.
Natural-language repo parsing no longer supports short names.
The new regex only matches owner/repo, so in backend-api is no longer recognized. That contradicts the parsing requirement to support short names. Consider restoring short-name support and updating tests accordingly.
As per coding guidelines: “Extract natural language 'in ' pattern to set Repository field (supports both 'owner/repo' and short names)”.
🔧 Suggested fix (restore short-name support)
- inRepoRe = regexp.MustCompile(`(?i)\bin\s+([a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+)\s*,?`)
+ inRepoRe = regexp.MustCompile(`(?i)\bin\s+([a-zA-Z0-9._-]+(?:/[a-zA-Z0-9._-]+)?)\s*,?`)📝 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
| inlineOptRe = regexp.MustCompile(`(?i)\b(repo|branch|model|autopr|review|plan)=(\S+)`) | |
| inRepoRe = regexp.MustCompile(`(?i)\bin\s+([a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+)\s*,?`) | |
| withModelRe = regexp.MustCompile(`(?i)\bwith\s+([a-zA-Z0-9._-]+)\s*,?`) | |
| multiSpace = regexp.MustCompile(`\s{2,}`) | |
| flagRe = regexp.MustCompile(`(?i)--(?:no-review|no-plan|direct)\b`) | |
| ) | |
| inlineOptRe = regexp.MustCompile(`(?i)\b(repo|branch|model|autopr|review|plan)=(\S+)`) | |
| inRepoRe = regexp.MustCompile(`(?i)\bin\s+([a-zA-Z0-9._-]+(?:/[a-zA-Z0-9._-]+)?)\s*,?`) | |
| withModelRe = regexp.MustCompile(`(?i)\bwith\s+([a-zA-Z0-9._-]+)\s*,?`) | |
| multiSpace = regexp.MustCompile(`\s{2,}`) | |
| flagRe = regexp.MustCompile(`(?i)--(?:no-review|no-plan|direct)\b`) | |
| ) |
🤖 Prompt for AI Agents
In `@server/parser/parser.go` around lines 48 - 53, The in-repo regex inRepoRe no
longer accepts short repo names (e.g., "in backend-api"); update its pattern so
it captures either owner/repo or a single short name by allowing an optional
"/owner" segment (e.g., change inRepoRe to match
[a-zA-Z0-9._-]+(?:/[a-zA-Z0-9._-]+)?), then run/update the tests that assert
natural-language "in <repo>" parsing to include short-name cases; ensure the
Repository field assignment logic that reads matches from inRepoRe continues to
work unchanged.
This commit addresses all critical CodeRabbit feedback and fixes the go mod tidy issue: **Critical fixes:** - api.go: Require successful StopAgent before archiving agents (prevent state corruption) - api.go: Add default cases in HITL phase switches (handle unexpected phase values) - api.go: Fix ephemeral messages to respect thread RootId (proper thread context) - attachments.go: Truncate Fallback field to match Text (prevent post size limit issues) - types.go: Remove omitempty from AutoCreatePr (ensure false booleans are marshaled) - hitl.go: Publish workflow phase change after GetConversation error (keep UI in sync) - parser.go: Add strings.TrimSpace after flag extraction (fix bracketed option matching) - poller.go: Handle SaveWorkflow error in implementing phase (log persistence failures) **Test improvements:** - poller_test.go: Remove duplicate GetConfig mock (already in setupPollerPlugin) - api_test.go: Add GetPost mock for sendEphemeralToActionUser (fix test failures) **Linter fix:** - go.mod: Run go mod tidy (move google/uuid to direct dependencies) **Nitpicks addressed:** - hitl.go: Add cursor/ prefix to branch names (consistency with direct launches) **Rejected CodeRabbit suggestions:** - parser.go: Did NOT allow short repo names in natural language patterns (would introduce false positives like "in production" or "in compact mode") Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This was referenced Feb 14, 2026
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
1 participant
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
Implements Milestone 3 HITL (Human-In-The-Loop) verification workflow, adding two optional stages before agent implementation:
Both stages are independently configurable via:
EnableContextReview,EnablePlanLoop--no-review,--no-plan,--directKey Changes
Backend (Go)
HITLWorkflowtype for multi-stage workflow state managementcontext_review,planning,plan_review,implementing,rejected,completeserver/hitl.go) with 15+ workflow functionshitl:andhitlagent:prefixes, workflow CRUD operationshitl:are workflow IDs (not agent IDs)autoCreatePr: false,autoBranch: false, read-only system prompthitl_test.go, KV store, attachments, handlersFrontend (React/TypeScript)
Configuration & Parser
EnableContextReview,EnablePlanLoopwith help text--no-review,--no-plan,--directfor opt-out controlDocumentation
.claude/commands/guides to reference HITL patternsserver/store/CLAUDE.mdexpanded with HITL KV conventionsTesting
✅ All linter checks pass (
make check-style)✅ All Go tests pass (
go test ./server/...)✅ All webapp tests pass (5 suites, 39 tests)
✅ 100+ new tests added for HITL functionality
Implementation Notes
FINISHEDstate, iteration requires launching a new planner with accumulated context (follow-ups only work onRUNNINGagents)planningphase queues inPendingFeedbackand auto-triggers new iteration when planner finishesHITLWorkflowKV interface methodsMigration Path
Fully backward compatible:
thread:mappings withouthitl:prefix work as before🤖 Generated with Claude Code
Summary by CodeRabbit