From 951f51408a84c3a0ea4bdf413d85b48be294f21e Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:56:16 +0800 Subject: [PATCH] fix(agent/comments): prevent resumed sessions from reusing stale --parent UUID (#1374) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agent/comments): re-emit trigger comment id every turn + server-side parent_id guard Resumed Claude sessions keep prior turns' tool calls in context, so a comment-triggered task could reuse the PREVIOUS turn's --parent UUID instead of the current trigger's. The reply landed in the wrong thread (MUL-1125): backend stored exactly what the agent sent, but the agent pulled a stale UUID from its own conversation memory. Two layers of defense: 1. Extract BuildCommentReplyInstructions so daemon.buildCommentPrompt and execenv.InjectRuntimeConfig emit the same "use this exact --parent, do not reuse values from previous turns" block. The per-turn prompt now carries the current TriggerCommentID, which it previously relied on CLAUDE.md for (and CLAUDE.md isn't re-read mid-session). 2. Handler-side guard in CreateComment: when an agent posts from inside a comment-triggered task (X-Agent-ID + X-Task-ID, task has TriggerCommentID), require parent_id == task.TriggerCommentID or return 409. Assignment-triggered tasks are untouched. * fix(agent/comments): scope parent_id guard to the task's own issue Two issues from CI + GPT-Boy's review: 1. Guard was too broad: the CLI stamps X-Task-ID on every request, so an agent legitimately commenting on a different issue while its current task was comment-triggered would get 409'd with the wrong issue's trigger comment id. Narrow the guard to fire only when the request's issue matches the task's own issue — cross-issue agent activity stays unblocked. 2. The integration test tried to insert a second queued task for the same (agent, issue), which hits the idx_one_pending_task_per_issue_agent unique index. Replace the assignment-triggered-task sub-case with a cross-issue regression test (the scenario we now need to cover anyway): post on issue B while X-Task-ID points at a comment-triggered task on issue A, expect 201. --- server/internal/daemon/daemon_test.go | 10 +- .../daemon/execenv/reply_instructions.go | 24 ++++ .../daemon/execenv/reply_instructions_test.go | 66 ++++++++++ .../internal/daemon/execenv/runtime_config.go | 3 +- server/internal/daemon/prompt.go | 8 +- server/internal/handler/activity_test.go | 117 ++++++++++++++++++ server/internal/handler/comment.go | 23 ++++ 7 files changed, 247 insertions(+), 4 deletions(-) create mode 100644 server/internal/daemon/execenv/reply_instructions.go create mode 100644 server/internal/daemon/execenv/reply_instructions_test.go diff --git a/server/internal/daemon/daemon_test.go b/server/internal/daemon/daemon_test.go index 25883566f..8e733f915 100644 --- a/server/internal/daemon/daemon_test.go +++ b/server/internal/daemon/daemon_test.go @@ -112,14 +112,20 @@ func TestBuildPromptCommentTriggered(t *testing.T) { Agent: &AgentData{Name: "Test"}, }) - // Prompt should contain the comment content directly. + // Prompt should contain the comment content, the trigger comment id, and + // the full reply command with --parent. Re-emitting --parent on every turn + // is what prevents resumed sessions from reusing the previous turn's + // --parent UUID. for _, want := range []string{ issueID, commentContent, "comment that triggered this task", + commentID, + "multica issue comment add " + issueID + " --parent " + commentID, + "do NOT reuse --parent values from previous turns", } { if !strings.Contains(prompt, want) { - t.Fatalf("prompt missing %q", want) + t.Fatalf("prompt missing %q\n---\n%s", want, prompt) } } diff --git a/server/internal/daemon/execenv/reply_instructions.go b/server/internal/daemon/execenv/reply_instructions.go new file mode 100644 index 000000000..a541ed899 --- /dev/null +++ b/server/internal/daemon/execenv/reply_instructions.go @@ -0,0 +1,24 @@ +package execenv + +import "fmt" + +// BuildCommentReplyInstructions returns the canonical block telling an agent +// how to post its reply for a comment-triggered task. Both the per-turn +// prompt (daemon.buildCommentPrompt) and the CLAUDE.md workflow +// (InjectRuntimeConfig) call this so the trigger comment ID and the +// --parent value cannot drift between surfaces. +// +// The explicit "do not reuse --parent from previous turns" wording exists +// because resumed Claude sessions keep prior turns' tool calls in context +// and will otherwise copy the old --parent UUID forward. +func BuildCommentReplyInstructions(issueID, triggerCommentID string) string { + if triggerCommentID == "" { + return "" + } + return fmt.Sprintf( + "Reply by running exactly this command — always use the trigger comment ID below, "+ + "do NOT reuse --parent values from previous turns in this session:\n\n"+ + " multica issue comment add %s --parent %s --content \"...\"\n", + issueID, triggerCommentID, + ) +} diff --git a/server/internal/daemon/execenv/reply_instructions_test.go b/server/internal/daemon/execenv/reply_instructions_test.go new file mode 100644 index 000000000..03e1a245d --- /dev/null +++ b/server/internal/daemon/execenv/reply_instructions_test.go @@ -0,0 +1,66 @@ +package execenv + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestBuildCommentReplyInstructionsIncludesTriggerID(t *testing.T) { + t.Parallel() + + issueID := "11111111-1111-1111-1111-111111111111" + triggerID := "22222222-2222-2222-2222-222222222222" + + got := BuildCommentReplyInstructions(issueID, triggerID) + + for _, want := range []string{ + "multica issue comment add " + issueID + " --parent " + triggerID, + "do NOT reuse --parent values from previous turns", + } { + if !strings.Contains(got, want) { + t.Fatalf("reply instructions missing %q\n---\n%s", want, got) + } + } +} + +func TestBuildCommentReplyInstructionsEmptyWhenNoTrigger(t *testing.T) { + t.Parallel() + + if got := BuildCommentReplyInstructions("issue-id", ""); got != "" { + t.Fatalf("expected empty string when triggerCommentID is empty, got %q", got) + } +} + +func TestInjectRuntimeConfigCommentTriggerUsesHelper(t *testing.T) { + t.Parallel() + dir := t.TempDir() + + issueID := "11111111-1111-1111-1111-111111111111" + triggerID := "22222222-2222-2222-2222-222222222222" + + ctx := TaskContextForEnv{ + IssueID: issueID, + TriggerCommentID: triggerID, + } + if err := InjectRuntimeConfig(dir, "claude", ctx); err != nil { + t.Fatalf("InjectRuntimeConfig failed: %v", err) + } + + content, err := os.ReadFile(filepath.Join(dir, "CLAUDE.md")) + if err != nil { + t.Fatalf("read CLAUDE.md: %v", err) + } + + s := string(content) + for _, want := range []string{ + triggerID, + "multica issue comment add " + issueID + " --parent " + triggerID, + "do NOT reuse --parent values from previous turns", + } { + if !strings.Contains(s, want) { + t.Errorf("CLAUDE.md missing %q", want) + } + } +} diff --git a/server/internal/daemon/execenv/runtime_config.go b/server/internal/daemon/execenv/runtime_config.go index 7e20345a9..49a84d7b8 100644 --- a/server/internal/daemon/execenv/runtime_config.go +++ b/server/internal/daemon/execenv/runtime_config.go @@ -129,7 +129,8 @@ func buildMetaSkillContent(provider string, ctx TaskContextForEnv) string { fmt.Fprintf(&b, "2. Run `multica issue comment list %s --output json` to read the conversation\n", ctx.IssueID) b.WriteString(" - If the output is very large or truncated, use pagination: `--limit 30` to get the latest 30 comments, or `--since ` to fetch only recent ones\n") fmt.Fprintf(&b, "3. Find the triggering comment (ID: `%s`) and understand what is being asked — do NOT confuse it with previous comments\n", ctx.TriggerCommentID) - fmt.Fprintf(&b, "4. Reply: `multica issue comment add %s --parent %s --content \"...\"`\n", ctx.IssueID, ctx.TriggerCommentID) + b.WriteString("4. ") + b.WriteString(BuildCommentReplyInstructions(ctx.IssueID, ctx.TriggerCommentID)) b.WriteString("5. If the comment requests code changes or further work, do the work first, then reply with your results\n") b.WriteString("6. Do NOT change the issue status unless the comment explicitly asks for it\n\n") } else { diff --git a/server/internal/daemon/prompt.go b/server/internal/daemon/prompt.go index a3ec91a51..1efab84d4 100644 --- a/server/internal/daemon/prompt.go +++ b/server/internal/daemon/prompt.go @@ -3,6 +3,8 @@ package daemon import ( "fmt" "strings" + + "github.com/multica-ai/multica/server/internal/daemon/execenv" ) // BuildPrompt constructs the task prompt for an agent CLI. @@ -25,6 +27,9 @@ func BuildPrompt(task Task) string { // buildCommentPrompt constructs a prompt for comment-triggered tasks. // The triggering comment content is embedded directly so the agent cannot // miss it, even when stale output files exist in a reused workdir. +// The reply instructions (including the current TriggerCommentID as --parent) +// are re-emitted on every turn so resumed sessions cannot carry forward a +// previous turn's --parent UUID. func buildCommentPrompt(task Task) string { var b strings.Builder b.WriteString("You are running as a local coding agent for a Multica workspace.\n\n") @@ -33,7 +38,8 @@ func buildCommentPrompt(task Task) string { b.WriteString("[NEW COMMENT] A user just left a new comment that triggered this task. You MUST respond to THIS comment, not any previous ones:\n\n") fmt.Fprintf(&b, "> %s\n\n", task.TriggerCommentContent) } - fmt.Fprintf(&b, "Start by running `multica issue get %s --output json` to understand your task, then complete it.\n", task.IssueID) + fmt.Fprintf(&b, "Start by running `multica issue get %s --output json` to understand your task, then complete it.\n\n", task.IssueID) + b.WriteString(execenv.BuildCommentReplyInstructions(task.IssueID, task.TriggerCommentID)) return b.String() } diff --git a/server/internal/handler/activity_test.go b/server/internal/handler/activity_test.go index f20911818..0bf1f1a7e 100644 --- a/server/internal/handler/activity_test.go +++ b/server/internal/handler/activity_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" db "github.com/multica-ai/multica/server/pkg/db/generated" @@ -206,6 +207,122 @@ func TestCreateComment_WithParentID(t *testing.T) { } } +func TestCreateComment_AgentWithWrongParentRejected(t *testing.T) { + ctx := context.Background() + + // Find the fixture agent + its runtime. + var agentID, runtimeID string + if err := testPool.QueryRow(ctx, + `SELECT id, runtime_id FROM agent WHERE workspace_id = $1 AND name = $2`, + testWorkspaceID, "Handler Test Agent", + ).Scan(&agentID, &runtimeID); err != nil { + t.Fatalf("find test agent: %v", err) + } + + // Two issues: A hosts the comment-triggered task; B exists to prove the + // guard is scoped to the task's own issue and does not block cross-issue + // agent activity. (The CLI stamps X-Task-ID on every request, so an agent + // legitimately commenting on another issue must still succeed.) + createIssue := func(title string) string { + w := httptest.NewRecorder() + r := newRequest("POST", "/api/issues?workspace_id="+testWorkspaceID, map[string]any{"title": title}) + testHandler.CreateIssue(w, r) + if w.Code != http.StatusCreated { + t.Fatalf("CreateIssue(%s): %d: %s", title, w.Code, w.Body.String()) + } + var issue IssueResponse + json.NewDecoder(w.Body).Decode(&issue) + return issue.ID + } + issueA := createIssue("agent parent guard test — issue A") + issueB := createIssue("agent parent guard test — issue B") + + var freshTaskID string + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM agent_task_queue WHERE id = $1`, freshTaskID) + testPool.Exec(ctx, `DELETE FROM comment WHERE issue_id IN ($1, $2)`, issueA, issueB) + testPool.Exec(ctx, `DELETE FROM issue WHERE id IN ($1, $2)`, issueA, issueB) + }) + + postComment := func(t *testing.T, issueID string, body map[string]any, headers map[string]string) *httptest.ResponseRecorder { + t.Helper() + w := httptest.NewRecorder() + r := newRequest("POST", "/api/issues/"+issueID+"/comments", body) + r = withURLParam(r, "id", issueID) + for k, v := range headers { + r.Header.Set(k, v) + } + testHandler.CreateComment(w, r) + return w + } + + w := postComment(t, issueA, map[string]any{"content": "stale comment"}, nil) + if w.Code != http.StatusCreated { + t.Fatalf("create stale parent: %d: %s", w.Code, w.Body.String()) + } + var staleParent CommentResponse + json.NewDecoder(w.Body).Decode(&staleParent) + + w = postComment(t, issueA, map[string]any{"content": "fresh comment"}, nil) + if w.Code != http.StatusCreated { + t.Fatalf("create fresh parent: %d: %s", w.Code, w.Body.String()) + } + var freshParent CommentResponse + json.NewDecoder(w.Body).Decode(&freshParent) + + // Comment-triggered task bound to issueA. + if err := testPool.QueryRow(ctx, + `INSERT INTO agent_task_queue (agent_id, runtime_id, issue_id, status, priority, trigger_comment_id) + VALUES ($1, $2, $3, 'queued', 0, $4) RETURNING id`, + agentID, runtimeID, issueA, freshParent.ID, + ).Scan(&freshTaskID); err != nil { + t.Fatalf("insert fresh task: %v", err) + } + + agentHeaders := map[string]string{"X-Agent-ID": agentID, "X-Task-ID": freshTaskID} + + // Same issue + wrong parent → 409. + w = postComment(t, issueA, + map[string]any{"content": "drifted reply", "parent_id": staleParent.ID}, + agentHeaders, + ) + if w.Code != http.StatusConflict { + t.Fatalf("expected 409 when agent replies with wrong parent, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), freshParent.ID) { + t.Fatalf("expected error body to reference the correct trigger comment id, got %s", w.Body.String()) + } + + // Same issue + no parent → 409 (must reply to trigger). + w = postComment(t, issueA, + map[string]any{"content": "no parent"}, + agentHeaders, + ) + if w.Code != http.StatusConflict { + t.Fatalf("expected 409 when agent replies with no parent, got %d", w.Code) + } + + // Same issue + correct parent → 201. + w = postComment(t, issueA, + map[string]any{"content": "correct reply", "parent_id": freshParent.ID}, + agentHeaders, + ) + if w.Code != http.StatusCreated { + t.Fatalf("expected 201 when agent replies with matching parent, got %d: %s", w.Code, w.Body.String()) + } + + // Cross-issue: agent carries X-Task-ID (bound to issueA) but comments on + // issueB. The guard must NOT fire — this is the cross-issue regression + // covering the fix for gpt-boy's review. + w = postComment(t, issueB, + map[string]any{"content": "cross-issue note"}, + agentHeaders, + ) + if w.Code != http.StatusCreated { + t.Fatalf("agent posting on a different issue should not be blocked by its current task's trigger, got %d: %s", w.Code, w.Body.String()) + } +} + func TestCommentWithParentID_AppearsInTimeline(t *testing.T) { ctx := context.Background() diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index 2060f72b6..71577f7e2 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -216,6 +216,29 @@ func (h *Handler) CreateComment(w http.ResponseWriter, r *http.Request) { // Determine author identity: agent (via X-Agent-ID header) or member. authorType, authorID := h.resolveActor(r, userID, uuidToString(issue.WorkspaceID)) + // Defense against resumed-session drift: when an agent posts from inside a + // comment-triggered task AND the comment is being posted on that same + // issue, the parent_id must exactly match the task's trigger comment. + // Resumed Claude sessions otherwise carry forward a previous turn's + // --parent UUID and silently misplace the reply. + // + // The task.IssueID scope is important: the CLI stamps X-Task-ID on every + // request, so an agent legitimately commenting on a different issue must + // not be blocked by its current task's trigger. Assignment-triggered + // tasks (no TriggerCommentID) are also unaffected. + if authorType == "agent" { + if taskIDHeader := r.Header.Get("X-Task-ID"); taskIDHeader != "" { + task, err := h.Queries.GetAgentTask(r.Context(), parseUUID(taskIDHeader)) + if err == nil && task.TriggerCommentID.Valid && uuidToString(task.IssueID) == uuidToString(issue.ID) { + if uuidToString(parentID) != uuidToString(task.TriggerCommentID) { + writeError(w, http.StatusConflict, + "parent_id must equal this task's trigger comment id ("+uuidToString(task.TriggerCommentID)+")") + return + } + } + } + } + // Expand bare issue identifiers (e.g. MUL-117) into mention links. req.Content = mention.ExpandIssueIdentifiers(r.Context(), h.Queries, issue.WorkspaceID, req.Content)