mirror of
https://github.com/multica-ai/multica.git
synced 2026-06-17 03:38:32 +02:00
fix(agent/comments): prevent resumed sessions from reusing stale --parent UUID (#1374)
* 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.
This commit is contained in:
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
24
server/internal/daemon/execenv/reply_instructions.go
Normal file
24
server/internal/daemon/execenv/reply_instructions.go
Normal file
@@ -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,
|
||||
)
|
||||
}
|
||||
66
server/internal/daemon/execenv/reply_instructions_test.go
Normal file
66
server/internal/daemon/execenv/reply_instructions_test.go
Normal file
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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 <timestamp>` 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 {
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user