diff --git a/server/cmd/multica/cmd_issue.go b/server/cmd/multica/cmd_issue.go index 4d8895996..33369c32d 100644 --- a/server/cmd/multica/cmd_issue.go +++ b/server/cmd/multica/cmd_issue.go @@ -540,7 +540,7 @@ func init() { issueCommentAddCmd.Flags().Bool("content-stdin", false, "Read comment content from stdin (preserves multi-line content verbatim)") issueCommentAddCmd.Flags().String("content-file", "", "Read comment content from a UTF-8 file (preserves multi-line content verbatim; use this on Windows when stdin piping mangles non-ASCII bytes). The path must be inside the current working directory unless --allow-external-file is set.") issueCommentAddCmd.Flags().Bool("allow-external-file", false, "Allow --content-file / --attachment to read a path outside the current working directory. Off by default so a stale file from another run/environment can't be picked up (MUL-4252).") - issueCommentAddCmd.Flags().String("parent", "", "Parent comment ID (reply to a specific comment)") + issueCommentAddCmd.Flags().String("parent", "", "Parent comment ID to reply under. A comment-triggered agent task must reply under its trigger comment; omitting --parent to post a top-level comment is rejected") issueCommentAddCmd.Flags().StringSlice("attachment", nil, "File path(s) to attach (can be specified multiple times)") issueCommentAddCmd.Flags().String("output", "json", "Output format: table or json") diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index e8f2344a1..7715fa10e 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -1265,8 +1265,9 @@ func (h *Handler) CreateComment(w http.ResponseWriter, r *http.Request) { if err == nil && task.IssueID.Valid && uuidToString(task.IssueID) == uuidToString(issue.ID) { if task.TriggerCommentID.Valid { if !taskCoversReplyParent(task, parentID) { + // Keep this error actionable for agents (MUL-4417 / GH #5266). writeError(w, http.StatusConflict, - "parent_id must be this task's trigger comment id ("+uuidToString(task.TriggerCommentID)+") or one of the earlier comments it coalesced") + "comment-triggered tasks cannot create top-level comments; set parent_id (--parent) to "+uuidToString(task.TriggerCommentID)+" or a coalesced comment id") return } } diff --git a/server/internal/handler/comment_reply_authz_handler_test.go b/server/internal/handler/comment_reply_authz_handler_test.go new file mode 100644 index 000000000..fd124918b --- /dev/null +++ b/server/internal/handler/comment_reply_authz_handler_test.go @@ -0,0 +1,84 @@ +package handler + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestCreateComment_TriggeredTaskRejectsTopLevelComment exercises the full +// CreateComment handler path (not just taskCoversReplyParent) for the trap +// reported in MUL-4417 / GH #5266: a comment-triggered task that posts a +// parentless, top-level comment on its own issue is rejected with a 409 whose +// message names the trigger comment and states that top-level comments are not +// allowed. Pinning the message here keeps it from silently drifting away from +// the behavior the CLI help now documents. +func TestCreateComment_TriggeredTaskRejectsTopLevelComment(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + + fx := newRunningSquadLeaderTaskFixture(t) + + w := httptest.NewRecorder() + r := newRequest("POST", "/api/issues/"+fx.IssueID+"/comments", map[string]any{ + "content": "dispatching a squad from this task", + }) + r = withURLParam(r, "id", fx.IssueID) + r.Header.Set("X-Agent-ID", fx.LeaderID) + r.Header.Set("X-Task-ID", fx.TaskID) + + testHandler.CreateComment(w, r) + if w.Code != http.StatusConflict { + t.Fatalf("CreateComment top-level: expected 409, got %d: %s", w.Code, w.Body.String()) + } + if got := countAgentCommentsForIssue(t, fx.IssueID, fx.LeaderID); got != 0 { + t.Fatalf("expected rejected top-level comment not to be stored, got %d", got) + } + + var body map[string]any + if err := json.NewDecoder(w.Body).Decode(&body); err != nil { + t.Fatalf("decode error response: %v", err) + } + msg, _ := body["error"].(string) + // Pin the three semantic pieces without locking the exact wording: why it + // was rejected, the comment to reply under, and the actionable fix. The last + // one guards against the guidance being dropped in a future edit. + for _, want := range []string{ + "top-level comments", // reason + fx.TriggerCommentID, // the comment to reply under + "parent_id (--parent)", // actionable fix + } { + if !strings.Contains(msg, want) { + t.Fatalf("409 message should contain %q, got %q", want, msg) + } + } +} + +// TestCreateComment_TriggeredTaskAllowsReplyUnderTrigger is the positive half: +// the same task replying under its trigger comment succeeds, proving the guard +// rejects only the top-level case and does not lock the whole issue for +// comments (MUL-4417 / GH #5266). +func TestCreateComment_TriggeredTaskAllowsReplyUnderTrigger(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + + fx := newRunningSquadLeaderTaskFixture(t) + + w := httptest.NewRecorder() + r := newRequest("POST", "/api/issues/"+fx.IssueID+"/comments", map[string]any{ + "content": "replying under the trigger comment", + "parent_id": fx.TriggerCommentID, + }) + r = withURLParam(r, "id", fx.IssueID) + r.Header.Set("X-Agent-ID", fx.LeaderID) + r.Header.Set("X-Task-ID", fx.TaskID) + + testHandler.CreateComment(w, r) + if w.Code != http.StatusCreated { + t.Fatalf("CreateComment reply-under-trigger: expected 201, got %d: %s", w.Code, w.Body.String()) + } +}