diff --git a/apps/backend/cmd/agentctl/kandev_comment.go b/apps/backend/cmd/agentctl/kandev_comment.go index c2e68acf8f..d2d27d6835 100644 --- a/apps/backend/cmd/agentctl/kandev_comment.go +++ b/apps/backend/cmd/agentctl/kandev_comment.go @@ -25,7 +25,8 @@ func runCommentCmd(args []string) int { } } -// commentAdd posts a comment on a task. If --body is "-", reads from stdin. +// commentAdd posts a comment through the signed runtime scope. If --body is +// "-", reads from stdin. func commentAdd(args []string) int { fs := flag.NewFlagSet("comment add", flag.ContinueOnError) taskFlag := fs.String("task", "", "Task ID (defaults to $KANDEV_TASK_ID)") @@ -58,13 +59,11 @@ func commentAdd(args []string) int { } payload := map[string]string{ - "body": body, - "author_type": "agent", - "author_id": client.agentID, + "task_id": taskID, + "body": body, } - respBody, status, doErr := client.do(http.MethodPost, - fmt.Sprintf("/api/v1/office/tasks/%s/comments", taskID), payload) + respBody, status, doErr := client.do(http.MethodPost, "/api/v1/office/runtime/comments", payload) return handleResponse(respBody, status, doErr) } diff --git a/apps/backend/cmd/agentctl/kandev_test.go b/apps/backend/cmd/agentctl/kandev_test.go index 879ea21e1d..226b6a81b6 100644 --- a/apps/backend/cmd/agentctl/kandev_test.go +++ b/apps/backend/cmd/agentctl/kandev_test.go @@ -584,8 +584,12 @@ func TestOfficeSetup_ProjectThenTaskUsesReturnedProjectAndWorkspace(t *testing.T // --- Comment Tests --- func TestCommentAdd_PostsComment(t *testing.T) { - srv, captured := setupMockServer(t, 201, `{"ok":true}`) - setEnvVars(t, srv) + captured := setupMockTransport(t, http.StatusCreated, `{"ok":true}`) + t.Setenv("KANDEV_API_URL", "http://kandev.test") + t.Setenv("KANDEV_API_KEY", "signed-office-run-token") + t.Setenv("KANDEV_RUN_ID", "run-456") + t.Setenv("KANDEV_TASK_ID", "task-abc") + t.Setenv("KANDEV_AGENT_ID", "agent-789") code := runKandevCLI([]string{ "comment", "add", "--body", "This is a test comment", @@ -596,19 +600,25 @@ func TestCommentAdd_PostsComment(t *testing.T) { if captured.Method != "POST" { t.Errorf("expected POST, got %s", captured.Method) } - if captured.Path != "/api/v1/office/tasks/task-abc/comments" { + if captured.Path != "/api/v1/office/runtime/comments" { t.Errorf("unexpected path: %s", captured.Path) } + assertRunIDHeader(t, captured, "run-456") - var body map[string]string + var body map[string]any if err := json.Unmarshal([]byte(captured.Body), &body); err != nil { t.Fatalf("unmarshal body: %v", err) } if body["body"] != "This is a test comment" { - t.Errorf("unexpected body: %s", body["body"]) + t.Errorf("unexpected body: %v", body["body"]) } - if body["author_id"] != "agent-789" { - t.Errorf("expected author_id=agent-789, got %s", body["author_id"]) + if body["task_id"] != "task-abc" { + t.Errorf("expected task_id=task-abc, got %v", body["task_id"]) + } + for _, key := range []string{"author_type", "author_id", "source"} { + if _, ok := body[key]; ok { + t.Errorf("runtime comment payload must not contain %q: %#v", key, body) + } } } diff --git a/apps/backend/internal/office/configloader/skills/kandev-task-ops/references/comments.md b/apps/backend/internal/office/configloader/skills/kandev-task-ops/references/comments.md index 5246584a46..ff5e68f55c 100644 --- a/apps/backend/internal/office/configloader/skills/kandev-task-ops/references/comments.md +++ b/apps/backend/internal/office/configloader/skills/kandev-task-ops/references/comments.md @@ -12,6 +12,6 @@ Comment on another task: $KANDEV_CLI kandev tasks message --id T-42 --prompt "Blocked: the worktree has a dirty submodule. Owner: please investigate." ``` -`tasks message` writes as the current agent. `comment add` writes as the user, so only use `comment add` when acting as a test fixture or intentionally simulating user input. +Both `tasks message` and `comment add` write as the current agent. Avoid empty acknowledgements such as `Done.` unless they include useful progress. Link to file paths or commits instead of quoting long code blocks. diff --git a/apps/backend/internal/office/dashboard/handler_comments.go b/apps/backend/internal/office/dashboard/handler_comments.go index 275264e58f..4c08059b9d 100644 --- a/apps/backend/internal/office/dashboard/handler_comments.go +++ b/apps/backend/internal/office/dashboard/handler_comments.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" "github.com/google/uuid" + "github.com/kandev/kandev/internal/office/agents" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" "go.uber.org/zap" @@ -72,14 +73,20 @@ func (h *Handler) createComment(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "body is required"}) return } - authorType := req.AuthorType - if authorType == "" { - authorType = userSentinel + taskID := c.Param("id") + if agents.CallerFromContext(c) != nil || + (req.AuthorType != "" && req.AuthorType != userSentinel) { + h.logger.Warn("reject agent comment on dashboard endpoint", + zap.String("task_id", taskID)) + c.JSON(http.StatusForbidden, gin.H{ + "error": "agent comments must use the runtime comments endpoint", + }) + return } comment := &models.TaskComment{ ID: uuid.New().String(), - TaskID: c.Param("id"), - AuthorType: authorType, + TaskID: taskID, + AuthorType: userSentinel, AuthorID: userSentinel, Body: req.Body, Source: userSentinel, diff --git a/apps/backend/internal/office/dashboard/handler_comments_security_test.go b/apps/backend/internal/office/dashboard/handler_comments_security_test.go new file mode 100644 index 0000000000..23b354fcc8 --- /dev/null +++ b/apps/backend/internal/office/dashboard/handler_comments_security_test.go @@ -0,0 +1,191 @@ +package dashboard_test + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/office/agents" + "github.com/kandev/kandev/internal/office/dashboard" + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/repository/sqlite" + "github.com/kandev/kandev/internal/office/shared" + workflowrepo "github.com/kandev/kandev/internal/workflow/repository" +) + +// commentSecurityFixture wires the authenticated dashboard stack. The route +// accepts UI requests without a token, but agent tokens must use the runtime +// endpoint so capability and task-scope checks cannot be bypassed. +type commentSecurityFixture struct { + router *gin.Engine + agentsSvc *agents.AgentService + repo *sqlite.Repository +} + +func newCommentSecurityFixture(t *testing.T) *commentSecurityFixture { + t.Helper() + gin.SetMode(gin.TestMode) + + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + if _, _, err := settingsstore.Provide(db, db, nil); err != nil { + t.Fatalf("settings store: %v", err) + } + + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("new repo: %v", err) + } + + _, err = db.Exec(` + CREATE TABLE IF NOT EXISTS tasks ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL DEFAULT '', + title TEXT NOT NULL DEFAULT '', + description TEXT DEFAULT '', + state TEXT DEFAULT 'todo', + priority TEXT NOT NULL DEFAULT 'medium' CHECK (priority IN ('critical','high','medium','low')), + parent_id TEXT DEFAULT '', + project_id TEXT DEFAULT '', + assignee_agent_profile_id TEXT DEFAULT '', + labels TEXT DEFAULT '[]', + metadata TEXT DEFAULT '{}', + identifier TEXT DEFAULT '', + is_ephemeral INTEGER DEFAULT 0, + origin TEXT DEFAULT 'manual', + execution_policy TEXT DEFAULT '', + execution_state TEXT DEFAULT '', + workflow_id TEXT NOT NULL DEFAULT '', + workflow_step_id TEXT DEFAULT '', + archived_at TIMESTAMP, + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP + ) + `) + if err != nil { + t.Fatalf("create tasks table: %v", err) + } + _, err = db.Exec(`CREATE TABLE IF NOT EXISTS workspaces ( + id TEXT PRIMARY KEY, + office_workflow_id TEXT DEFAULT '' + )`) + if err != nil { + t.Fatalf("create workspaces table: %v", err) + } + _, err = db.Exec(`CREATE TABLE IF NOT EXISTS workflows ( + id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL DEFAULT '', + workflow_template_id TEXT DEFAULT '', name TEXT NOT NULL, + description TEXT DEFAULT '', created_at TIMESTAMP NOT NULL, updated_at TIMESTAMP NOT NULL + )`) + if err != nil { + t.Fatalf("create workflows: %v", err) + } + if _, err := workflowrepo.NewWithDB(db, db, nil); err != nil { + t.Fatalf("workflow repo: %v", err) + } + + log := logger.Default() + activity := shared.NewActivityLogger(repo, log) + agentsSvc := agents.NewAgentService(repo, log, nil) + agentsSvc.SetAuth(agents.NewAgentAuth("test-key")) + svc := dashboard.NewDashboardService(repo, log, activity, agentsSvc, &stubCostChecker{}) + + r := gin.New() + r.Use(agents.AgentAuthMiddleware(agentsSvc)) + group := r.Group("/api/v1/office") + dashboard.RegisterRoutes(group, svc, repo, nil, log) + + return &commentSecurityFixture{router: r, agentsSvc: agentsSvc, repo: repo} +} + +func seedCommentAgent(t *testing.T, svc *agents.AgentService, id, workspaceID string) *models.AgentInstance { + t.Helper() + a := &models.AgentInstance{ + ID: id, + WorkspaceID: workspaceID, + Name: id, + Role: models.AgentRoleWorker, + Status: models.AgentStatusIdle, + Permissions: shared.DefaultPermissions(shared.AgentRoleWorker), + } + if err := svc.CreateAgentInstance(context.Background(), a); err != nil { + t.Fatalf("create agent %q: %v", id, err) + } + return a +} + +func seedCommentTask(t *testing.T, repo *sqlite.Repository, id, workspaceID, assigneeAgentID string) { + t.Helper() + _, err := repo.ExecRaw(context.Background(), + `INSERT INTO tasks (id, workspace_id, title, assignee_agent_profile_id) VALUES (?, ?, ?, ?)`, + id, workspaceID, "task", assigneeAgentID, + ) + if err != nil { + t.Fatalf("seed task %q: %v", id, err) + } +} + +func postCommentReq(taskID, token, body string) *http.Request { + req := httptest.NewRequest( + http.MethodPost, + "/api/v1/office/tasks/"+taskID+"/comments", + bytes.NewBufferString(body), + ) + req.Header.Set("Content-Type", "application/json") + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + return req +} + +func TestCreateComment_AgentCallerMustUseRuntimeEndpoint(t *testing.T) { + f := newCommentSecurityFixture(t) + agent := seedCommentAgent(t, f.agentsSvc, "agent-a", "ws-1") + seedCommentTask(t, f.repo, "task-1", "ws-1", agent.ID) + seedCommentTask(t, f.repo, "task-2", "ws-1", "") + + // The token has no post_comment capability and is scoped to task-1. A + // dashboard request for task-2 must still be rejected before persistence. + token, err := f.agentsSvc.MintRuntimeJWT(agent.ID, "task-1", agent.WorkspaceID, "run-1", "sess-1", "") + if err != nil { + t.Fatalf("mint jwt: %v", err) + } + rec := httptest.NewRecorder() + f.router.ServeHTTP(rec, postCommentReq("task-2", token, `{"body":"hello"}`)) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) + } + comments, err := f.repo.ListTaskComments(context.Background(), "task-2") + if err != nil { + t.Fatalf("list comments: %v", err) + } + if len(comments) != 0 { + t.Fatalf("comments = %+v, want none persisted", comments) + } +} + +func TestCreateComment_RejectsAgentAuthorTypeWithoutJWT(t *testing.T) { + f := newCommentSecurityFixture(t) + seedCommentTask(t, f.repo, "task-1", "ws-1", "") + + rec := httptest.NewRecorder() + f.router.ServeHTTP(rec, postCommentReq("task-1", "", `{"body":"hello","author_type":"agent"}`)) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) + } +} diff --git a/apps/backend/internal/office/dashboard/handler_test.go b/apps/backend/internal/office/dashboard/handler_test.go index 49805857f5..cb39ac17a4 100644 --- a/apps/backend/internal/office/dashboard/handler_test.go +++ b/apps/backend/internal/office/dashboard/handler_test.go @@ -651,6 +651,38 @@ func TestCreateComment_RequiresBody(t *testing.T) { } } +// TestCreateComment_UserAuthorUnchanged locks in that the UI comment path +// (author_type unset or "user") still persists the user sentinel — the +// fix must not touch this path. +func TestCreateComment_UserAuthorUnchanged(t *testing.T) { + deps := newTestDeps(t) + + body := `{"body":"hello from the UI","author_type":"user"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/office/tasks/taskUser/comments", + strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + deps.router.ServeHTTP(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String()) + } + + var resp dashboard.CommentResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp.Comment == nil { + t.Fatal("expected comment in response") + } + if resp.Comment.AuthorID != "user" { + t.Errorf("expected authorId 'user', got %q", resp.Comment.AuthorID) + } + if resp.Comment.Source != "user" { + t.Errorf("expected source 'user', got %q", resp.Comment.Source) + } +} + func TestUpdateWorkspaceSettings_ReturnsOK(t *testing.T) { deps := newTestDeps(t) diff --git a/apps/backend/internal/office/runtime/handler_test.go b/apps/backend/internal/office/runtime/handler_test.go index 507ed5e5d7..0690bb0e1d 100644 --- a/apps/backend/internal/office/runtime/handler_test.go +++ b/apps/backend/internal/office/runtime/handler_test.go @@ -172,7 +172,7 @@ func TestRuntimeHandler_PostCommentUsesRuntimeToken(t *testing.T) { t.Fatalf("comments = %d, want 1", len(h.comments.comments)) } comment := h.comments.comments[0] - if comment.TaskID != "task-1" || comment.AuthorID != "agent-1" || comment.AuthorType != "agent" { + if comment.TaskID != "task-1" || comment.AuthorID != "agent-1" || comment.AuthorType != "agent" || comment.Source != "agent" { t.Fatalf("comment identity = %#v", comment) } assertActionRunEvent(t, h.runEvents, "post_comment", "task", "task-1") diff --git a/apps/backend/internal/office/scheduler/reactivity_test.go b/apps/backend/internal/office/scheduler/reactivity_test.go new file mode 100644 index 0000000000..e076912a44 --- /dev/null +++ b/apps/backend/internal/office/scheduler/reactivity_test.go @@ -0,0 +1,90 @@ +package scheduler + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/common/logger" +) + +// recordedQueueCall captures one queue(agentID, RunContext) invocation from +// reactToComment for assertion. +type recordedQueueCall struct { + agentID string + ctx RunContext +} + +// TestReactToComment_SelfCommentDoesNotWakeAssignee is the card's required +// regression: a comment authored by the task's own assignee (author_type +// "agent", author_id == the assignee's agent profile id) must NOT queue a +// task_comment run for that assignee. This is the guard in reactToComment +// (reactivity.go); it only works when the comment's author_id was +// persisted as the acting agent's real profile id (dashboard's +// createComment handler is the thing that used to break this by hardcoding +// author_id="user"). +func TestReactToComment_SelfCommentDoesNotWakeAssignee(t *testing.T) { + ss := &SchedulerService{logger: logger.Default()} + task := &TaskSnapshot{ + ID: "task-1", + WorkspaceID: "ws-1", + State: "IN_PROGRESS", + AssigneeAgentProfileID: "agent-a", + } + comment := &MutationComment{ + ID: "comment-1", + Body: "Landscape scan complete, no action needed.", + AuthorType: "agent", + AuthorID: "agent-a", + } + + var calls []recordedQueueCall + queue := func(agentID string, c RunContext) { + calls = append(calls, recordedQueueCall{agentID: agentID, ctx: c}) + } + + ss.reactToComment(context.Background(), task, comment, queue) + + for _, c := range calls { + if c.agentID == "agent-a" && c.ctx.Reason == RunReasonTaskComment { + t.Fatalf("agent-a was woken by its own comment: %+v", c) + } + } +} + +// TestReactToComment_OtherAuthorWakesAssignee is the positive counterpart: +// a comment from a different agent (or the user) on a task assigned to +// agent-b DOES queue a task_comment run for agent-b. Preserving this is +// explicitly required by the card — the fix must not suppress cross-agent +// wakes while closing the self-wake hole. +func TestReactToComment_OtherAuthorWakesAssignee(t *testing.T) { + ss := &SchedulerService{logger: logger.Default()} + task := &TaskSnapshot{ + ID: "task-2", + WorkspaceID: "ws-1", + State: "IN_PROGRESS", + AssigneeAgentProfileID: "agent-b", + } + comment := &MutationComment{ + ID: "comment-2", + Body: "Please take a look at this.", + AuthorType: "agent", + AuthorID: "agent-a", + } + + var calls []recordedQueueCall + queue := func(agentID string, c RunContext) { + calls = append(calls, recordedQueueCall{agentID: agentID, ctx: c}) + } + + ss.reactToComment(context.Background(), task, comment, queue) + + found := false + for _, c := range calls { + if c.agentID == "agent-b" && c.ctx.Reason == RunReasonTaskComment { + found = true + } + } + if !found { + t.Fatalf("expected agent-b to be queued a task_comment run, got calls: %+v", calls) + } +}