From 44989d12e8197be2a6c40609ec9bc4012526f177 Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 26 Aug 2026 08:38:18 +0000 Subject: [PATCH] feat(office): let agents read task comments so parent tasks stop missing handoffs (#3019) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- apps/backend/cmd/agentctl/kandev_comment.go | 2 +- .../internal/backendapp/adapters_office.go | 38 ++ .../backendapp/adapters_office_test.go | 38 ++ apps/backend/internal/backendapp/helpers.go | 3 +- .../internal/backendapp/office_scope.go | 16 +- .../office_scope_completeness_test.go | 2 +- .../internal/backendapp/office_scope_test.go | 88 +++- .../internal/common/truncate/truncate.go | 15 + .../internal/common/truncate/truncate_test.go | 31 ++ .../internal/mcp/server/server_test.go | 11 +- .../backend/internal/office/agents/handler.go | 10 +- .../office/agents/handler_claims_test.go | 32 ++ .../configloader/instructions/ceo/AGENTS.md | 4 +- .../kandev-task-ops/references/comments.md | 8 + .../office/dashboard/agent_summaries_test.go | 2 +- .../internal/office/dashboard/handler.go | 20 +- .../office/dashboard/handler_comments.go | 35 +- .../handler_comments_agent_read_test.go | 178 ++++++++ .../handler_comments_security_test.go | 96 +++- .../internal/office/dashboard/handler_test.go | 2 +- .../office/dashboard/participants_test.go | 2 +- .../office/repository/sqlite/comments.go | 6 +- .../office/repository/sqlite/comments_test.go | 67 +++ .../repository/sqlite/comments_window.go | 53 +++ .../repository/sqlite/comments_window_test.go | 412 ++++++++++++++++++ apps/backend/internal/office/routes.go | 8 +- .../internal/office/runtime/actions.go | 2 +- .../internal/office/truncate/truncate.go | 26 +- .../internal/task/service/handoff_access.go | 22 +- .../task/service/handoff_access_test.go | 81 +++- .../internal/task/service/handoff_comments.go | 171 ++++++++ .../task/service/handoff_comments_test.go | 367 ++++++++++++++++ .../internal/task/service/handoff_service.go | 10 + .../web/e2e/tests/office/agentctl-cli.spec.ts | 76 ++++ docs/specs/office/README.md | 2 + .../requirements/agent-comment-reads.md | 367 ++++++++++++++++ .../system-design/agent-comment-reads-01.md | 387 ++++++++++++++++ 37 files changed, 2630 insertions(+), 60 deletions(-) create mode 100644 apps/backend/internal/common/truncate/truncate.go create mode 100644 apps/backend/internal/common/truncate/truncate_test.go create mode 100644 apps/backend/internal/office/agents/handler_claims_test.go create mode 100644 apps/backend/internal/office/dashboard/handler_comments_agent_read_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/comments_window.go create mode 100644 apps/backend/internal/office/repository/sqlite/comments_window_test.go create mode 100644 apps/backend/internal/task/service/handoff_comments.go create mode 100644 apps/backend/internal/task/service/handoff_comments_test.go create mode 100644 docs/specs/office/requirements/agent-comment-reads.md create mode 100644 docs/specs/office/system-design/agent-comment-reads-01.md diff --git a/apps/backend/cmd/agentctl/kandev_comment.go b/apps/backend/cmd/agentctl/kandev_comment.go index d2d27d6835..e8749eea29 100644 --- a/apps/backend/cmd/agentctl/kandev_comment.go +++ b/apps/backend/cmd/agentctl/kandev_comment.go @@ -71,7 +71,7 @@ func commentAdd(args []string) int { func commentList(args []string) int { fs := flag.NewFlagSet("comment list", flag.ContinueOnError) taskFlag := fs.String("task", "", "Task ID (defaults to $KANDEV_TASK_ID)") - limitFlag := fs.Int("limit", 0, "Max number of comments (0 = all)") + limitFlag := fs.Int("limit", 0, "Max number of comments (0 = server default)") if err := fs.Parse(args); err != nil { cliError("parse flags: %v", err) return 1 diff --git a/apps/backend/internal/backendapp/adapters_office.go b/apps/backend/internal/backendapp/adapters_office.go index 4b06e421a5..4b9921e510 100644 --- a/apps/backend/internal/backendapp/adapters_office.go +++ b/apps/backend/internal/backendapp/adapters_office.go @@ -10,6 +10,7 @@ import ( "github.com/kandev/kandev/internal/office/configloader" officedashboard "github.com/kandev/kandev/internal/office/dashboard" officeengineadapters "github.com/kandev/kandev/internal/office/engine_adapters" + officemodels "github.com/kandev/kandev/internal/office/models" officeonboarding "github.com/kandev/kandev/internal/office/onboarding" officesqlite "github.com/kandev/kandev/internal/office/repository/sqlite" officeroutines "github.com/kandev/kandev/internal/office/routines" @@ -20,6 +21,43 @@ import ( taskservice "github.com/kandev/kandev/internal/task/service" ) +type officeCommentWindowReader interface { + ListTaskCommentsWindow(ctx context.Context, taskID string, limit int) ([]*officemodels.TaskComment, int, error) +} + +// officeCommentReaderAdapter keeps Office persistence models at the backend +// composition boundary. The task service receives its own neutral records. +type officeCommentReaderAdapter struct { + reader officeCommentWindowReader +} + +func (a *officeCommentReaderAdapter) ListTaskCommentsWindow( + ctx context.Context, taskID string, limit int, +) ([]taskservice.CommentRecord, int, error) { + rows, total, err := a.reader.ListTaskCommentsWindow(ctx, taskID, limit) + if err != nil { + return nil, 0, err + } + records := make([]taskservice.CommentRecord, 0, len(rows)) + for _, row := range rows { + if row == nil { + continue + } + records = append(records, taskservice.CommentRecord{ + ID: row.ID, + TaskID: row.TaskID, + AuthorType: row.AuthorType, + AuthorID: row.AuthorID, + Source: row.Source, + Body: row.Body, + CreatedAt: row.CreatedAt, + }) + } + return records, total, nil +} + +var _ taskservice.CommentReader = (*officeCommentReaderAdapter)(nil) + // taskWorkspaceCreatorAdapter adapts the task service to the office // WorkspaceCreator interface for dual workspace creation. type taskWorkspaceCreatorAdapter struct { diff --git a/apps/backend/internal/backendapp/adapters_office_test.go b/apps/backend/internal/backendapp/adapters_office_test.go index 533d95af1d..9da4269d9c 100644 --- a/apps/backend/internal/backendapp/adapters_office_test.go +++ b/apps/backend/internal/backendapp/adapters_office_test.go @@ -5,12 +5,14 @@ import ( "database/sql" "path/filepath" "testing" + "time" "github.com/jmoiron/sqlx" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/events/bus" + officemodels "github.com/kandev/kandev/internal/office/models" officesqlite "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/repository" @@ -19,6 +21,42 @@ import ( "github.com/kandev/kandev/internal/worktree" ) +type fakeOfficeCommentWindowReader struct { + comments []*officemodels.TaskComment + total int +} + +func (f *fakeOfficeCommentWindowReader) ListTaskCommentsWindow( + context.Context, string, int, +) ([]*officemodels.TaskComment, int, error) { + return f.comments, f.total, nil +} + +func TestOfficeCommentReaderAdapterMapsPersistenceRows(t *testing.T) { + createdAt := time.Date(2026, 1, 1, 2, 3, 4, 0, time.UTC) + adapter := officeCommentReaderAdapter{reader: &fakeOfficeCommentWindowReader{ + comments: []*officemodels.TaskComment{{ + ID: "comment-1", TaskID: "task-1", AuthorType: "agent", AuthorID: "agent-1", + Body: "done", Source: "run", ReplyChannelID: "private", CreatedAt: createdAt, + }}, + total: 4, + }} + + rows, total, err := adapter.ListTaskCommentsWindow(context.Background(), "task-1", 2) + if err != nil { + t.Fatalf("ListTaskCommentsWindow: %v", err) + } + if total != 4 { + t.Fatalf("total = %d, want 4", total) + } + if len(rows) != 1 { + t.Fatalf("len(rows) = %d, want 1", len(rows)) + } + if rows[0].ID != "comment-1" || rows[0].Body != "done" || !rows[0].CreatedAt.Equal(createdAt) { + t.Fatalf("row = %+v, want mapped comment", rows[0]) + } +} + type adapterStartStepResolver struct { repo *tasksqlite.Repository } diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index 8b92a87a5a..0143179516 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -645,6 +645,7 @@ func registerRoutes(p routeParams) { handoffDocSvc := taskservice.NewDocumentService(p.taskRepo, p.log) handoffSvc := taskservice.NewHandoffService(p.taskRepo, p.taskRepo, handoffDocSvc, p.officeRepo, p.officeRepo, p.log) + handoffSvc.SetCommentReader(&officeCommentReaderAdapter{reader: p.officeRepo}) // Phase 6 wirings — materializer hook + disk cleaner. The // SessionWorktreeReader and WorkspaceCleaner interfaces are both // satisfied by adapters that delegate to existing services. @@ -1385,7 +1386,7 @@ func registerSecondaryRoutes( // Register office routes if p.services.OfficeSvcs != nil { - mountOfficeRoutes(p.router, p.services.OfficeSvcs, p.authSvc, p.taskSvc, p.officeRepo, p.log) + mountOfficeRoutes(p.router, p.services.OfficeSvcs, p.authSvc, p.taskSvc, p.officeRepo, handoffSvc, p.log) p.log.Debug("Registered Office handlers (HTTP)") } } diff --git a/apps/backend/internal/backendapp/office_scope.go b/apps/backend/internal/backendapp/office_scope.go index 7828fd06f2..3b6c042945 100644 --- a/apps/backend/internal/backendapp/office_scope.go +++ b/apps/backend/internal/backendapp/office_scope.go @@ -188,6 +188,13 @@ func officeWorkspaceScopeMiddleware( c.Next() return } + // The comment endpoint has its own task-relation guard for agent + // callers. Let that guard decide every target so missing, foreign, and + // unrelated tasks all produce the same forbidden response. + if isAgentCommentRead(c) { + c.Next() + return + } if err := authorizeOfficeRequest(c, taskSvc, officeRepo, resolvers); err != nil { c.AbortWithStatusJSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) return @@ -196,6 +203,12 @@ func officeWorkspaceScopeMiddleware( } } +func isAgentCommentRead(c *gin.Context) bool { + return c.Request.Method == http.MethodGet && + c.FullPath() == officeRoutePrefix+"/tasks/:id/comments" && + officeagents.ClaimsFromContext(c) != nil +} + // authorizeOfficeRequest returns nil only when the caller is allowed to reach // this route: either it is explicitly workspace-less, or every workspace the // route's ids resolve to is one the caller may access. @@ -505,10 +518,11 @@ func mountOfficeRoutes( authSvc *auth.Service, taskSvc *taskservice.Service, officeRepo *officesqlite.Repository, + handoffSvc *taskservice.HandoffService, log *logger.Logger, ) { api := router.Group(officeRoutePrefix) api.Use(officeagents.AgentAuthMiddleware(svcs.Agents)) api.Use(officeWorkspaceScopeMiddleware(authSvc, taskSvc, officeRepo)) - office.RegisterAllRoutes(api, svcs, log) + office.RegisterAllRoutes(api, svcs, handoffSvc, log) } diff --git a/apps/backend/internal/backendapp/office_scope_completeness_test.go b/apps/backend/internal/backendapp/office_scope_completeness_test.go index 3f9b2b137c..9cb6ca92b7 100644 --- a/apps/backend/internal/backendapp/office_scope_completeness_test.go +++ b/apps/backend/internal/backendapp/office_scope_completeness_test.go @@ -36,7 +36,7 @@ func registeredOfficeRoutes(t *testing.T) gin.RoutesInfo { t.Helper() gin.SetMode(gin.TestMode) engine := gin.New() - office.RegisterAllRoutes(engine.Group(officeRoutePrefix), officeTestServices(), testLogger(t)) + office.RegisterAllRoutes(engine.Group(officeRoutePrefix), officeTestServices(), nil, testLogger(t)) routes := engine.Routes() if len(routes) < 100 { diff --git a/apps/backend/internal/backendapp/office_scope_test.go b/apps/backend/internal/backendapp/office_scope_test.go index 4a4c1837af..584e694164 100644 --- a/apps/backend/internal/backendapp/office_scope_test.go +++ b/apps/backend/internal/backendapp/office_scope_test.go @@ -14,8 +14,10 @@ import ( "github.com/kandev/kandev/internal/auth/authn" "github.com/kandev/kandev/internal/common/config" officeagents "github.com/kandev/kandev/internal/office/agents" + officedashboard "github.com/kandev/kandev/internal/office/dashboard" "github.com/kandev/kandev/internal/office/models" officesqlite "github.com/kandev/kandev/internal/office/repository/sqlite" + "github.com/kandev/kandev/internal/office/shared" taskservice "github.com/kandev/kandev/internal/task/service" ) @@ -546,7 +548,7 @@ func TestOfficeRouteGroupMountsScopeGuard(t *testing.T) { c.Request.Context(), authn.Identity{UserID: officeScopeUserB, Role: authn.RoleMember})) c.Next() }) - mountOfficeRoutes(engine, officeTestServices(), h.authSvc, h.taskSvc, h.officeRepo, testLogger(t)) + mountOfficeRoutes(engine, officeTestServices(), h.authSvc, h.taskSvc, h.officeRepo, nil, testLogger(t)) // The scope guard: user B naming user A's agent must be refused before // the (zero-value, would-panic) handler runs. @@ -743,6 +745,90 @@ func TestOfficeScopeAllowsAgentJWTInsideItsOwnWorkspace(t *testing.T) { } } +// TestOfficeScopeDefersAgentCommentReadToRelationGuard mounts the production +// Office scope middleware and dashboard handler together. The comment route +// must reach HandoffService so missing, foreign, and unrelated targets all +// return the same access-denied response instead of leaking a 404 from the +// workspace resolver. +func TestOfficeScopeDefersAgentCommentReadToRelationGuard(t *testing.T) { + taskSvc, taskRepo, officeRepo := newRunSubscriptionCheckHarness(t) + log := testLogger(t) + config := &config.Config{} + config.Features.Auth = true + config.Auth.SessionTTLHours = 720 + authSvc := newEnabledAuthService(t, config) + + workspaceA := seedOwnedWorkspace(t, taskSvc, "comment-user-a") + workspaceB := seedOwnedWorkspace(t, taskSvc, "comment-user-b") + seedOfficeResources(t, officeRepo, workspaceA, "comment-a") + seedOfficeResources(t, officeRepo, workspaceB, "comment-b") + + agentSvc := officeagents.NewAgentService(officeRepo, log, nil) + agentSvc.SetAuth(officeagents.NewAgentAuth("comment-test-key")) + token, err := agentSvc.MintRuntimeJWT( + "agent-comment-a", "task-comment-a", workspaceA, "run-comment-a", "", "") + if err != nil { + t.Fatalf("mint runtime jwt: %v", err) + } + + handoff := taskservice.NewHandoffService( + taskRepo, + taskRepo, + taskservice.NewDocumentService(taskRepo, log), + officeRepo, + officeRepo, + log, + ) + handoff.SetCommentReader(&officeCommentReaderAdapter{reader: officeRepo}) + dashboardSvc := officedashboard.NewDashboardService( + officeRepo, + log, + shared.NewActivityLogger(officeRepo, log), + agentSvc, + nil, + ) + + gin.SetMode(gin.TestMode) + engine := gin.New() + group := engine.Group(officeRoutePrefix) + group.Use(officeagents.AgentAuthMiddleware(agentSvc)) + group.Use(officeWorkspaceScopeMiddleware(authSvc, taskSvc, officeRepo)) + officedashboard.RegisterRoutes(group, dashboardSvc, officeRepo, nil, handoff, log) + + cases := []struct { + name string + path string + }{ + {name: "missing", path: officeRoutePrefix + "/tasks/does-not-exist/comments"}, + {name: "foreign workspace", path: officeRoutePrefix + "/tasks/task-comment-b/comments"}, + {name: "unrelated same workspace", path: officeRoutePrefix + "/tasks/task-comment-a2/comments"}, + } + // Add an existing task in the caller workspace that is not related to the + // JWT task. The seeded comment-a task is the caller task itself. + now := time.Now().UTC() + if _, err := officeRepo.ExecRaw(context.Background(), + `INSERT INTO tasks (id, workspace_id, title, created_at, updated_at) VALUES (?, ?, ?, ?, ?)`, + "task-comment-a2", workspaceA, "unrelated", now, now); err != nil { + t.Fatalf("seed unrelated task: %v", err) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tc.path, nil) + req.Header.Set("Authorization", "Bearer "+token) + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, body=%q; want 403", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), taskservice.ErrAccessDenied.Error()) { + t.Fatalf("body = %q, want %q", rec.Body.String(), taskservice.ErrAccessDenied.Error()) + } + }) + } +} + // officeCrossWorkspaceCases are routes naming more than one resource, with // the ids deliberately drawn from two DIFFERENT workspaces that the SAME // caller owns. `path` templates {wsA1}/{wsA2} to user A's two workspaces. diff --git a/apps/backend/internal/common/truncate/truncate.go b/apps/backend/internal/common/truncate/truncate.go new file mode 100644 index 0000000000..520875666a --- /dev/null +++ b/apps/backend/internal/common/truncate/truncate.go @@ -0,0 +1,15 @@ +// Package truncate provides dependency-neutral rune-safe byte truncation. +package truncate + +// UTF8 returns s truncated to at most maxBytes, cutting at a rune boundary. +// When the cut would split a multi-byte rune, the incomplete rune is dropped. +func UTF8(s string, maxBytes int) string { + if len(s) <= maxBytes { + return s + } + cut := maxBytes + for cut > 0 && (s[cut]&0xC0) == 0x80 { + cut-- + } + return s[:cut] +} diff --git a/apps/backend/internal/common/truncate/truncate_test.go b/apps/backend/internal/common/truncate/truncate_test.go new file mode 100644 index 0000000000..b0931e2938 --- /dev/null +++ b/apps/backend/internal/common/truncate/truncate_test.go @@ -0,0 +1,31 @@ +package truncate_test + +import ( + "testing" + "unicode/utf8" + + "github.com/kandev/kandev/internal/common/truncate" +) + +func TestUTF8CutsOnlyAtRuneBoundaries(t *testing.T) { + for _, test := range []struct { + name string + in string + cap int + want string + }{ + {name: "ascii", in: "abcdef", cap: 3, want: "abc"}, + {name: "split rune", in: "a😀b", cap: 3, want: "a"}, + {name: "fits rune", in: "a😀b", cap: 5, want: "a😀"}, + } { + t.Run(test.name, func(t *testing.T) { + got := truncate.UTF8(test.in, test.cap) + if got != test.want { + t.Fatalf("UTF8(%q, %d) = %q, want %q", test.in, test.cap, got, test.want) + } + if !utf8.ValidString(got) { + t.Fatalf("UTF8(%q, %d) returned invalid UTF-8", test.in, test.cap) + } + }) + } +} diff --git a/apps/backend/internal/mcp/server/server_test.go b/apps/backend/internal/mcp/server/server_test.go index 85e58b20de..5253ae5f4d 100644 --- a/apps/backend/internal/mcp/server/server_test.go +++ b/apps/backend/internal/mcp/server/server_test.go @@ -1024,9 +1024,10 @@ func TestServerModeOffice_ToolCount(t *testing.T) { s := New(backend, "test-session", "test-task", 10005, log, "", false, ModeOffice) tools := getRegisteredToolNames(s) - // 4 plan + 1 interaction + 1 related-tasks + 3 task-documents + 1 rich-output + 1 decisions - // + 1 step_complete (ADR 0015) = 12. + // 4 plan + 1 interaction + 1 related-tasks + 3 task-documents + // + 1 rich-output + 1 decisions + 1 step_complete (ADR 0015) = 12. // (delegate_task_kandev retired in favour of `agentctl kandev task create …`). + // (list_task_comments_kandev retired in favour of `agentctl kandev comment list …`). assert.Contains(t, tools, "step_complete_kandev", "office mode must register the ADR 0015 completion signal") assert.Equal(t, 12, len(tools)) } @@ -1045,9 +1046,9 @@ func TestServerModeOffice_DisableAskQuestion(t *testing.T) { // delegate_task_kandev was retired from ModeOffice (now lives in // the agentctl CLI as `agentctl kandev task create --parent …`). assert.NotContains(t, tools, "delegate_task_kandev") - // 4 plan + 1 related-tasks + 3 task-documents + 1 rich-output + 1 decisions - // + 1 step_complete (ADR 0015) = 11 - // (no ask_user_question, no delegate) + // 4 plan + 1 related-tasks + 3 task-documents + 1 rich-output + // + 1 decisions + 1 step_complete (ADR 0015) = 11 + // (no ask_user_question, no delegate, no list_task_comments) assert.Equal(t, 11, len(tools)) } diff --git a/apps/backend/internal/office/agents/handler.go b/apps/backend/internal/office/agents/handler.go index a6f7028d9b..b2767022d4 100644 --- a/apps/backend/internal/office/agents/handler.go +++ b/apps/backend/internal/office/agents/handler.go @@ -108,9 +108,13 @@ func CallerFromContext(c *gin.Context) *models.AgentInstance { } // ClaimsFromContext exposes the validated agent JWT claims (or nil for UI -// requests). The workspace claim is what AgentAuthMiddleware compares against -// `:wsId`; the HTTP scope guard needs the same value to confine a token to -// its own workspace on by-ID routes, where there is no `:wsId` to compare. +// requests) to other office packages, without depending on the agents +// package's internal context-key constants. The workspace claim is what +// AgentAuthMiddleware compares against `:wsId`; the HTTP scope guard needs +// the same value to confine a token to its own workspace on by-ID routes, +// where there is no `:wsId` to compare. The task claim is the only source +// of caller task identity, used by callers such as the dashboard comment +// read guard to authorize an agent against the target task. func ClaimsFromContext(c *gin.Context) *AgentClaims { val, ok := c.Get(ctxKeyAgentClaims) if !ok { diff --git a/apps/backend/internal/office/agents/handler_claims_test.go b/apps/backend/internal/office/agents/handler_claims_test.go new file mode 100644 index 0000000000..092b84c53d --- /dev/null +++ b/apps/backend/internal/office/agents/handler_claims_test.go @@ -0,0 +1,32 @@ +package agents + +import ( + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" +) + +// AC-OFFICE-AGENT-COMMENT-READS-005.5: the caller task identity must come +// from validated JWT claims alone. ClaimsFromContext is the only exported +// reader of those claims outside this package. +func TestClaimsFromContext_ReturnsNilForUIRequest(t *testing.T) { + gin.SetMode(gin.TestMode) + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + + if got := ClaimsFromContext(c); got != nil { + t.Fatalf("ClaimsFromContext = %+v, want nil for a request with no validated claims", got) + } +} + +func TestClaimsFromContext_ReturnsSetClaims(t *testing.T) { + gin.SetMode(gin.TestMode) + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + want := &AgentClaims{AgentProfileID: "agent-1", TaskID: "task-42", WorkspaceID: "ws-1"} + c.Set(ctxKeyAgentClaims, want) + + got := ClaimsFromContext(c) + if got != want { + t.Fatalf("ClaimsFromContext = %+v, want %+v", got, want) + } +} diff --git a/apps/backend/internal/office/configloader/instructions/ceo/AGENTS.md b/apps/backend/internal/office/configloader/instructions/ceo/AGENTS.md index 9a319bb3e4..e4fb812390 100644 --- a/apps/backend/internal/office/configloader/instructions/ceo/AGENTS.md +++ b/apps/backend/internal/office/configloader/instructions/ceo/AGENTS.md @@ -47,7 +47,9 @@ When triaging a new task: 6. Post a comment only for a material delegation or status decision. When all subtasks are complete: -1. Review the results from each subtask. +1. Review the results from each subtask, including its comments (use + `$KANDEV_CLI kandev comment list --task ` to read the + deliverable it posted). 2. If satisfactory, mark the parent task as done. 3. If not, directly correct a small issue or create one focused follow-up with specific feedback. 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 ff5e68f55c..dc26efe261 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 @@ -15,3 +15,11 @@ $KANDEV_CLI kandev tasks message --id T-42 --prompt "Blocked: the worktree has a 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. + +Read a task's comments (all authors, agent and human) with: + +```bash +$KANDEV_CLI kandev comment list --task T-42 --limit 20 +``` + +`--task` defaults to the current task; `--limit` defaults to the server default (20, max 100). Use it to check whether a related task (parent, child, etc.) already posted the deliverable you're waiting on before proceeding. diff --git a/apps/backend/internal/office/dashboard/agent_summaries_test.go b/apps/backend/internal/office/dashboard/agent_summaries_test.go index 97c79b03c1..e3d7496d78 100644 --- a/apps/backend/internal/office/dashboard/agent_summaries_test.go +++ b/apps/backend/internal/office/dashboard/agent_summaries_test.go @@ -110,7 +110,7 @@ func withAgents(t *testing.T, deps *testDeps, agents []*models.AgentInstance) { router := gin.New() group := router.Group("/api/v1/office") - dashboard.RegisterRoutes(group, svc, deps.repo, nil, log) + dashboard.RegisterRoutes(group, svc, deps.repo, nil, nil, log) deps.svc = svc deps.router = router diff --git a/apps/backend/internal/office/dashboard/handler.go b/apps/backend/internal/office/dashboard/handler.go index e684edb9e4..7f5236cd13 100644 --- a/apps/backend/internal/office/dashboard/handler.go +++ b/apps/backend/internal/office/dashboard/handler.go @@ -12,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/office/shared" + taskservice "github.com/kandev/kandev/internal/task/service" "go.uber.org/zap" "golang.org/x/sync/errgroup" @@ -31,6 +32,7 @@ type Handler struct { gitMgr *configloader.GitManager runDetail RunDetailRepo agentSummary AgentSummaryRepository + handoff *taskservice.HandoffService logger *logger.Logger } @@ -41,12 +43,16 @@ type Handler struct { // when it satisfies those interfaces (the production *sqlite.Repository // does); a fake repo that does not implement them causes the // corresponding endpoints to respond 503. -func NewHandler(svc *DashboardService, labelRepo labelFetcher, gitMgr *configloader.GitManager, log *logger.Logger) *Handler { +// handoff may be nil in tests that never exercise the agent-caller comment +// read branch; an agent request against a nil handoff responds 503 rather +// than panicking (mirrors the runDetail/agentSummary nil-dependency pattern). +func NewHandler(svc *DashboardService, labelRepo labelFetcher, gitMgr *configloader.GitManager, handoff *taskservice.HandoffService, log *logger.Logger) *Handler { h := &Handler{ - svc: svc, - labels: labelRepo, - gitMgr: gitMgr, - logger: log.WithFields(zap.String("component", "office-dashboard-handler")), + svc: svc, + labels: labelRepo, + gitMgr: gitMgr, + handoff: handoff, + logger: log.WithFields(zap.String("component", "office-dashboard-handler")), } if r, ok := labelRepo.(RunDetailRepo); ok { h.runDetail = r @@ -58,8 +64,8 @@ func NewHandler(svc *DashboardService, labelRepo labelFetcher, gitMgr *configloa } // RegisterRoutes registers all dashboard-related routes on the given router group. -func RegisterRoutes(api *gin.RouterGroup, svc *DashboardService, labelRepo labelFetcher, gitMgr *configloader.GitManager, log *logger.Logger) { - h := NewHandler(svc, labelRepo, gitMgr, log) +func RegisterRoutes(api *gin.RouterGroup, svc *DashboardService, labelRepo labelFetcher, gitMgr *configloader.GitManager, handoff *taskservice.HandoffService, log *logger.Logger) { + h := NewHandler(svc, labelRepo, gitMgr, handoff, log) api.GET("/meta", h.getMeta) api.GET("/workspaces/:wsId/dashboard", h.getDashboard) diff --git a/apps/backend/internal/office/dashboard/handler_comments.go b/apps/backend/internal/office/dashboard/handler_comments.go index 4c08059b9d..f723d57568 100644 --- a/apps/backend/internal/office/dashboard/handler_comments.go +++ b/apps/backend/internal/office/dashboard/handler_comments.go @@ -2,7 +2,9 @@ package dashboard import ( "context" + "errors" "net/http" + "strconv" "time" "github.com/gin-gonic/gin" @@ -10,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/office/agents" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" + taskservice "github.com/kandev/kandev/internal/task/service" "go.uber.org/zap" ) @@ -21,8 +24,15 @@ import ( // declared in dto.go). func (h *Handler) listComments(c *gin.Context) { + taskID := c.Param("id") + + if claims := agents.ClaimsFromContext(c); claims != nil { + h.listCommentsForAgent(c, taskID, claims) + return + } + ctx := c.Request.Context() - comments, err := h.svc.ListComments(ctx, c.Param("id")) + comments, err := h.svc.ListComments(ctx, taskID) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return @@ -41,6 +51,29 @@ func (h *Handler) listComments(c *gin.Context) { c.JSON(http.StatusOK, CommentListResponse{Comments: dtos}) } +// listCommentsForAgent serves the guarded, bounded, reduced-projection +// comment window for an in-sandbox agent caller +// (REQ-OFFICE-AGENT-COMMENT-READS-001/002/003/004/005). The caller task +// identity comes only from the validated JWT claims, never from the +// request path or query string. +func (h *Handler) listCommentsForAgent(c *gin.Context, taskID string, claims *agents.AgentClaims) { + if h.handoff == nil { + c.JSON(http.StatusServiceUnavailable, gin.H{"error": "comment reads not configured"}) + return + } + limit, _ := strconv.Atoi(c.Query("limit")) + window, err := h.handoff.ListCommentsForCaller(c.Request.Context(), claims.TaskID, taskID, limit) + if err != nil { + if errors.Is(err, taskservice.ErrAccessDenied) { + c.JSON(http.StatusForbidden, gin.H{"error": err.Error()}) + return + } + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + c.JSON(http.StatusOK, window) +} + // fetchRunStatusForComments batches a per-comment run-status lookup. // Returns an empty map when no comments are passed or the lookup // fails — the handler degrades to the legacy run-less DTO shape so diff --git a/apps/backend/internal/office/dashboard/handler_comments_agent_read_test.go b/apps/backend/internal/office/dashboard/handler_comments_agent_read_test.go new file mode 100644 index 0000000000..b0a290c165 --- /dev/null +++ b/apps/backend/internal/office/dashboard/handler_comments_agent_read_test.go @@ -0,0 +1,178 @@ +package dashboard_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// TestListComments_AgentCallerGetsGuardedWindow covers +// REQ-OFFICE-AGENT-COMMENT-READS-001/002/003: an agent reading its own +// task's comments over the dashboard route gets the guarded, bounded, +// reduced-projection response instead of the browser's unbounded shape. +func TestListComments_AgentCallerGetsGuardedWindow(t *testing.T) { + f := newCommentSecurityFixture(t) + agent := seedCommentAgent(t, f.agentsSvc, f.repo, "agent-a", "ws-1") + seedCommentTask(t, f.repo, "task-1", "ws-1", agent.ID) + seedComment(t, f.repo, "c1", "task-1", "user", "user", "hello", "user", time.Unix(1000, 0)) + seedComment(t, f.repo, "c2", "task-1", "agent", agent.ID, "world", "agent", time.Unix(2000, 0)) + + 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, getCommentsReq("task-1", token, "")) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var window struct { + Comments []struct { + ID string `json:"id"` + TaskID string `json:"task_id"` + AuthorType string `json:"author_type"` + AuthorID string `json:"author_id"` + Source string `json:"source"` + Body string `json:"body"` + BodyTruncated bool `json:"body_truncated"` + BodyBytes int `json:"body_bytes"` + RunID string `json:"run_id"` + } `json:"comments"` + Total int `json:"total"` + Returned int `json:"returned"` + HasMore bool `json:"has_more"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &window); err != nil { + t.Fatalf("unmarshal: %v; body=%s", err, rec.Body.String()) + } + if window.Total != 2 || window.Returned != 2 || window.HasMore { + t.Fatalf("window = %+v, want total=2 returned=2 has_more=false", window) + } + if len(window.Comments) != 2 { + t.Fatalf("comments = %+v, want 2", window.Comments) + } + // AC-002.4: the agent projection omits run-lifecycle fields present on + // the browser's CommentDTO — a stray run_id in the JSON means the + // browser projection leaked onto the agent path. + for _, c := range window.Comments { + if c.RunID != "" { + t.Fatalf("comment %+v carries a run_id; agent projection must omit run lifecycle fields", c) + } + } +} + +// TestListComments_AgentCallerDeniedForUnrelatedTask covers +// REQ-OFFICE-AGENT-COMMENT-READS-005: an agent scoped to one task cannot +// read an unrelated task's comments through the dashboard route. +func TestListComments_AgentCallerDeniedForUnrelatedTask(t *testing.T) { + f := newCommentSecurityFixture(t) + agent := seedCommentAgent(t, f.agentsSvc, f.repo, "agent-a", "ws-1") + seedCommentTask(t, f.repo, "task-1", "ws-1", agent.ID) + seedCommentTask(t, f.repo, "task-2", "ws-1", "") + seedComment(t, f.repo, "c1", "task-2", "user", "user", "secret", "user", time.Unix(1000, 0)) + + 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, getCommentsReq("task-2", token, "")) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", rec.Code, rec.Body.String()) + } +} + +// TestListComments_AgentCallerLimitClampsWindow covers +// REQ-OFFICE-AGENT-COMMENT-READS-003: an agent-supplied limit bounds the +// returned window while total still reflects the full comment count. +func TestListComments_AgentCallerLimitClampsWindow(t *testing.T) { + f := newCommentSecurityFixture(t) + agent := seedCommentAgent(t, f.agentsSvc, f.repo, "agent-a", "ws-1") + seedCommentTask(t, f.repo, "task-1", "ws-1", agent.ID) + seedComment(t, f.repo, "c1", "task-1", "user", "user", "one", "user", time.Unix(1000, 0)) + seedComment(t, f.repo, "c2", "task-1", "user", "user", "two", "user", time.Unix(2000, 0)) + + 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, getCommentsReq("task-1", token, "?limit=1")) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var window struct { + Total int `json:"total"` + Returned int `json:"returned"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &window); err != nil { + t.Fatalf("unmarshal: %v; body=%s", err, rec.Body.String()) + } + if window.Total != 2 || window.Returned != 1 { + t.Fatalf("window = %+v, want total=2 returned=1", window) + } +} + +// TestListComments_BrowserCallerUnaffectedByAgentBranch pins the existing +// unguarded, unbounded browser shape: adding the agent branch must not +// change status code, response shape, or the ignored `limit` query param +// for a request carrying no agent token. +func TestListComments_BrowserCallerUnaffectedByAgentBranch(t *testing.T) { + f := newCommentSecurityFixture(t) + seedCommentTask(t, f.repo, "task-1", "ws-1", "") + seedComment(t, f.repo, "c1", "task-1", "user", "user", "hello", "user", time.Unix(1000, 0)) + seedComment(t, f.repo, "c2", "task-1", "user", "user", "world", "user", time.Unix(2000, 0)) + + rec := httptest.NewRecorder() + f.router.ServeHTTP(rec, getCommentsReq("task-1", "", "?limit=1")) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var resp struct { + Comments []json.RawMessage `json:"comments"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("unmarshal: %v; body=%s", err, rec.Body.String()) + } + if len(resp.Comments) != 2 { + t.Fatalf("comments = %d, want 2 (browser branch must ignore ?limit)", len(resp.Comments)) + } +} + +func seedComment( + t *testing.T, repo *sqlite.Repository, + id, taskID, authorType, authorID, body, source string, createdAt time.Time, +) { + t.Helper() + _, err := repo.ExecRaw(context.Background(), + `INSERT INTO task_comments (id, task_id, author_type, author_id, body, source, created_at) VALUES (?, ?, ?, ?, ?, ?, ?)`, + id, taskID, authorType, authorID, body, source, createdAt.UTC(), + ) + if err != nil { + t.Fatalf("seed comment %q: %v", id, err) + } +} + +func getCommentsReq(taskID, token, query string) *http.Request { + req := httptest.NewRequest( + http.MethodGet, + "/api/v1/office/tasks/"+taskID+"/comments"+query, + nil, + ) + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + return req +} diff --git a/apps/backend/internal/office/dashboard/handler_comments_security_test.go b/apps/backend/internal/office/dashboard/handler_comments_security_test.go index 23b354fcc8..60b2ea92ad 100644 --- a/apps/backend/internal/office/dashboard/handler_comments_security_test.go +++ b/apps/backend/internal/office/dashboard/handler_comments_security_test.go @@ -15,9 +15,11 @@ import ( "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" + officemodels "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/repository/sqlite" "github.com/kandev/kandev/internal/office/shared" + tasksqlite "github.com/kandev/kandev/internal/task/repository/sqlite" + taskservice "github.com/kandev/kandev/internal/task/service" workflowrepo "github.com/kandev/kandev/internal/workflow/repository" ) @@ -28,6 +30,41 @@ type commentSecurityFixture struct { router *gin.Engine agentsSvc *agents.AgentService repo *sqlite.Repository + handoff *taskservice.HandoffService +} + +// commentWindowReaderAdapter bridges the office repo's Office-model comment +// window read to the task service's neutral CommentReader contract — the +// same conversion internal/backendapp's officeCommentReaderAdapter performs +// in production, duplicated here because that adapter is unexported outside +// its package. +type commentWindowReaderAdapter struct { + repo *sqlite.Repository +} + +func (a *commentWindowReaderAdapter) ListTaskCommentsWindow( + ctx context.Context, taskID string, limit int, +) ([]taskservice.CommentRecord, int, error) { + rows, total, err := a.repo.ListTaskCommentsWindow(ctx, taskID, limit) + if err != nil { + return nil, 0, err + } + records := make([]taskservice.CommentRecord, 0, len(rows)) + for _, row := range rows { + if row == nil { + continue + } + records = append(records, taskservice.CommentRecord{ + ID: row.ID, + TaskID: row.TaskID, + AuthorType: row.AuthorType, + AuthorID: row.AuthorID, + Source: row.Source, + Body: row.Body, + CreatedAt: row.CreatedAt, + }) + } + return records, total, nil } func newCommentSecurityFixture(t *testing.T) *commentSecurityFixture { @@ -58,6 +95,7 @@ func newCommentSecurityFixture(t *testing.T) *commentSecurityFixture { description TEXT DEFAULT '', state TEXT DEFAULT 'todo', priority TEXT NOT NULL DEFAULT 'medium' CHECK (priority IN ('critical','high','medium','low')), + position INTEGER DEFAULT 0, parent_id TEXT DEFAULT '', project_id TEXT DEFAULT '', assignee_agent_profile_id TEXT DEFAULT '', @@ -80,7 +118,16 @@ func newCommentSecurityFixture(t *testing.T) *commentSecurityFixture { } _, err = db.Exec(`CREATE TABLE IF NOT EXISTS workspaces ( id TEXT PRIMARY KEY, - office_workflow_id TEXT DEFAULT '' + name TEXT NOT NULL DEFAULT '', + description TEXT DEFAULT '', + owner_id TEXT DEFAULT '', + default_executor_id TEXT DEFAULT '', + default_environment_id TEXT DEFAULT '', + default_agent_profile_id TEXT DEFAULT '', + default_config_agent_profile_id TEXT DEFAULT '', + office_workflow_id TEXT DEFAULT '', + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP )`) if err != nil { t.Fatalf("create workspaces table: %v", err) @@ -103,22 +150,53 @@ func newCommentSecurityFixture(t *testing.T) *commentSecurityFixture { agentsSvc.SetAuth(agents.NewAgentAuth("test-key")) svc := dashboard.NewDashboardService(repo, log, activity, agentsSvc, &stubCostChecker{}) + taskRepo, err := tasksqlite.NewWithDB(db, db, log) + if err != nil { + t.Fatalf("new task repo: %v", err) + } + // The task repo's migrations leave PRAGMA foreign_keys=ON on this shared + // connection. Restore the fixture's pre-existing FK-disabled behavior — + // tests here seed agents/tasks minimally and don't register the CLI + // tool rows agent_profiles.agent_id's FK points at. + if _, err := db.Exec(`PRAGMA foreign_keys=OFF`); err != nil { + t.Fatalf("disable foreign_keys: %v", err) + } + handoffDocSvc := taskservice.NewDocumentService(taskRepo, log) + handoff := taskservice.NewHandoffService(taskRepo, taskRepo, handoffDocSvc, repo, repo, log) + handoff.SetCommentReader(&commentWindowReaderAdapter{repo: repo}) + r := gin.New() r.Use(agents.AgentAuthMiddleware(agentsSvc)) group := r.Group("/api/v1/office") - dashboard.RegisterRoutes(group, svc, repo, nil, log) + dashboard.RegisterRoutes(group, svc, repo, nil, handoff, log) - return &commentSecurityFixture{router: r, agentsSvc: agentsSvc, repo: repo} + return &commentSecurityFixture{router: r, agentsSvc: agentsSvc, repo: repo, handoff: handoff} } -func seedCommentAgent(t *testing.T, svc *agents.AgentService, id, workspaceID string) *models.AgentInstance { +// seedCommentWorkspace inserts a minimal workspace row. Required because the +// fixture shares its sqlite connection with a task/repository/sqlite +// instance, whose migrations leave PRAGMA foreign_keys=ON on that +// connection — agents and tasks referencing an unseeded workspace_id now +// fail their FK constraint instead of silently succeeding. +func seedCommentWorkspace(t *testing.T, repo *sqlite.Repository, id string) { t.Helper() - a := &models.AgentInstance{ + _, err := repo.ExecRaw(context.Background(), + `INSERT OR IGNORE INTO workspaces (id, name) VALUES (?, ?)`, id, id, + ) + if err != nil { + t.Fatalf("seed workspace %q: %v", id, err) + } +} + +func seedCommentAgent(t *testing.T, svc *agents.AgentService, repo *sqlite.Repository, id, workspaceID string) *officemodels.AgentInstance { + t.Helper() + seedCommentWorkspace(t, repo, workspaceID) + a := &officemodels.AgentInstance{ ID: id, WorkspaceID: workspaceID, Name: id, - Role: models.AgentRoleWorker, - Status: models.AgentStatusIdle, + Role: officemodels.AgentRoleWorker, + Status: officemodels.AgentStatusIdle, Permissions: shared.DefaultPermissions(shared.AgentRoleWorker), } if err := svc.CreateAgentInstance(context.Background(), a); err != nil { @@ -153,7 +231,7 @@ func postCommentReq(taskID, token, body string) *http.Request { func TestCreateComment_AgentCallerMustUseRuntimeEndpoint(t *testing.T) { f := newCommentSecurityFixture(t) - agent := seedCommentAgent(t, f.agentsSvc, "agent-a", "ws-1") + agent := seedCommentAgent(t, f.agentsSvc, f.repo, "agent-a", "ws-1") seedCommentTask(t, f.repo, "task-1", "ws-1", agent.ID) seedCommentTask(t, f.repo, "task-2", "ws-1", "") diff --git a/apps/backend/internal/office/dashboard/handler_test.go b/apps/backend/internal/office/dashboard/handler_test.go index db33fede26..dead7e7979 100644 --- a/apps/backend/internal/office/dashboard/handler_test.go +++ b/apps/backend/internal/office/dashboard/handler_test.go @@ -238,7 +238,7 @@ func newTestDeps(t *testing.T) *testDeps { router := gin.New() group := router.Group("/api/v1/office") - dashboard.RegisterRoutes(group, svc, repo, nil, log) + dashboard.RegisterRoutes(group, svc, repo, nil, nil, log) return &testDeps{db: db, repo: repo, svc: svc, router: router, agents: agentSvc, wfRepo: wfRepo} } diff --git a/apps/backend/internal/office/dashboard/participants_test.go b/apps/backend/internal/office/dashboard/participants_test.go index 7200d17acc..0f657a1bf3 100644 --- a/apps/backend/internal/office/dashboard/participants_test.go +++ b/apps/backend/internal/office/dashboard/participants_test.go @@ -284,7 +284,7 @@ func newParticipantTestRouter(t *testing.T, caller *models.AgentInstance) *testD // Use the underlying stubAgentReader replaced with caller-aware reader // only when caller is non-nil. deps.svc = newServiceForParticipantsTest(t, deps, caller) - dashboard.RegisterRoutes(group, deps.svc, deps.repo, nil, logger.Default()) + dashboard.RegisterRoutes(group, deps.svc, deps.repo, nil, nil, logger.Default()) deps.router = router return deps } diff --git a/apps/backend/internal/office/repository/sqlite/comments.go b/apps/backend/internal/office/repository/sqlite/comments.go index f937e16958..6246c31957 100644 --- a/apps/backend/internal/office/repository/sqlite/comments.go +++ b/apps/backend/internal/office/repository/sqlite/comments.go @@ -15,7 +15,11 @@ func (r *Repository) CreateTaskComment(ctx context.Context, comment *models.Task if comment.ID == "" { comment.ID = uuid.New().String() } - comment.CreatedAt = time.Now().UTC() + if comment.CreatedAt.IsZero() { + comment.CreatedAt = time.Now().UTC() + } else { + comment.CreatedAt = comment.CreatedAt.UTC() + } _, err := r.db.ExecContext(ctx, r.db.Rebind(` INSERT INTO task_comments ( diff --git a/apps/backend/internal/office/repository/sqlite/comments_test.go b/apps/backend/internal/office/repository/sqlite/comments_test.go index 83307367a7..fb221fd97e 100644 --- a/apps/backend/internal/office/repository/sqlite/comments_test.go +++ b/apps/backend/internal/office/repository/sqlite/comments_test.go @@ -3,6 +3,7 @@ package sqlite_test import ( "context" "testing" + "time" "github.com/kandev/kandev/internal/office/models" ) @@ -41,3 +42,69 @@ func TestTaskComment_CRUD(t *testing.T) { t.Errorf("count after delete = %d, want 0", len(comments)) } } + +// AC-003.1/AC-003.2: CreateTaskComment must normalize a non-zero +// caller-supplied CreatedAt to UTC. task_comments.created_at is compared +// lexicographically (ORDER BY created_at DESC, id DESC), so mixing a +// local-offset timestamp with a UTC one on the same task can invert true +// chronological order even though both values name valid instants. +// This guards CreateTaskComment's own normalization directly; it does not +// depend on any caller already passing a UTC time. Actions.PostComment +// (internal/office/runtime/actions.go) sets CreatedAt: time.Now().UTC() +// itself, but CreateTaskComment must not regress if a future or external +// caller passes a non-UTC CreatedAt. +func TestCreateTaskComment_NormalizesNonZeroCreatedAtToUTC(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + + // olderLocal is genuinely earlier in real time (2026-01-01T01:00:00Z) + // but carries a +08:00 offset, so its wall-clock digits are larger + // than newerUTC's. + loc := time.FixedZone("test+08:00", 8*60*60) + olderLocal := &models.TaskComment{ + TaskID: "task-tz", + AuthorType: "agent", + AuthorID: "agent-1", + Body: "older-local", + Source: "run", + CreatedAt: time.Date(2026, 1, 1, 9, 0, 0, 0, loc), + } + // newerUTC is genuinely one hour later in real time. + newerUTC := &models.TaskComment{ + TaskID: "task-tz", + AuthorType: "agent", + AuthorID: "agent-1", + Body: "newer-utc", + Source: "run", + CreatedAt: time.Date(2026, 1, 1, 2, 0, 0, 0, time.UTC), + } + if err := repo.CreateTaskComment(ctx, olderLocal); err != nil { + t.Fatalf("create olderLocal: %v", err) + } + if err := repo.CreateTaskComment(ctx, newerUTC); err != nil { + t.Fatalf("create newerUTC: %v", err) + } + + if !olderLocal.CreatedAt.Equal(newerUTC.CreatedAt.Add(-time.Hour)) { + t.Fatalf("test setup invalid: olderLocal %v is not exactly 1h before newerUTC %v", + olderLocal.CreatedAt, newerUTC.CreatedAt) + } + + comments, err := repo.ListTaskComments(ctx, "task-tz") + if err != nil { + t.Fatalf("list: %v", err) + } + if len(comments) != 2 { + t.Fatalf("count = %d, want 2", len(comments)) + } + + // True chronological order is olderLocal, then newerUTC. + if comments[0].Body != "older-local" || comments[1].Body != "newer-utc" { + t.Fatalf("order = [%s, %s], want [older-local, newer-utc] (true chronological order)", + comments[0].Body, comments[1].Body) + } + + if comments[0].CreatedAt.Location() != time.UTC { + t.Errorf("olderLocal.CreatedAt location = %v, want UTC (normalized on write)", comments[0].CreatedAt.Location()) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/comments_window.go b/apps/backend/internal/office/repository/sqlite/comments_window.go new file mode 100644 index 0000000000..340a1e163b --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/comments_window.go @@ -0,0 +1,53 @@ +package sqlite + +import ( + "context" + "database/sql" + "fmt" + + "github.com/kandev/kandev/internal/office/models" +) + +// ListTaskCommentsWindow returns the newest `limit` comments for a task +// (by created_at DESC, id DESC), presented ascending by the same tiebreak +// columns, alongside the task's total comment count. The count and the +// window are read inside a single read-only transaction so a concurrent +// write is either fully visible to both or fully invisible to both +// (AC-003.7, AC-006.1/AC-006.2). ReadOnly+RepeatableRead is a no-op on +// SQLite (WAL already snapshots per-transaction) but documents intent and +// matches the internal/automation/export_store.go precedent. +func (r *Repository) ListTaskCommentsWindow(ctx context.Context, taskID string, limit int) ([]*models.TaskComment, int, error) { + tx, err := r.ro.BeginTxx(ctx, &sql.TxOptions{Isolation: sql.LevelRepeatableRead, ReadOnly: true}) + if err != nil { + return nil, 0, fmt.Errorf("begin read tx: %w", err) + } + defer func() { _ = tx.Rollback() }() + + var total int + if err := tx.QueryRowxContext(ctx, tx.Rebind( + `SELECT COUNT(*) FROM task_comments WHERE task_id = ?`), taskID).Scan(&total); err != nil { + return nil, 0, fmt.Errorf("count comments: %w", err) + } + + var comments []*models.TaskComment + if err := tx.SelectContext(ctx, &comments, tx.Rebind(` + SELECT * FROM task_comments + WHERE task_id = ? + ORDER BY created_at DESC, id DESC + LIMIT ? + `), taskID, limit); err != nil { + return nil, 0, fmt.Errorf("select comments: %w", err) + } + + if err := tx.Commit(); err != nil { + return nil, 0, fmt.Errorf("commit read tx: %w", err) + } + + for i, j := 0, len(comments)-1; i < j; i, j = i+1, j-1 { + comments[i], comments[j] = comments[j], comments[i] + } + if comments == nil { + comments = []*models.TaskComment{} + } + return comments, total, nil +} diff --git a/apps/backend/internal/office/repository/sqlite/comments_window_test.go b/apps/backend/internal/office/repository/sqlite/comments_window_test.go new file mode 100644 index 0000000000..7be9895f8e --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/comments_window_test.go @@ -0,0 +1,412 @@ +package sqlite_test + +import ( + "context" + "database/sql" + "database/sql/driver" + "path/filepath" + "sort" + "strings" + "sync" + "testing" + "time" + + "github.com/jmoiron/sqlx" + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/repository/sqlite" + sqlite3 "github.com/mattn/go-sqlite3" +) + +type commentWindowTraceState struct { + countProfiled chan struct{} + release chan struct{} + releaseOnce sync.Once + countOnce sync.Once +} + +var commentWindowTraceStateGlobal struct { + mu sync.Mutex + state *commentWindowTraceState +} + +func init() { + sql.Register("sqlite3_comments_window_test", commentWindowDriver{}) +} + +type commentWindowDriver struct{} + +func (commentWindowDriver) Open(name string) (driver.Conn, error) { + conn, err := (&sqlite3.SQLiteDriver{}).Open(name) + if err != nil { + return nil, err + } + commentWindowTraceStateGlobal.mu.Lock() + state := commentWindowTraceStateGlobal.state + commentWindowTraceStateGlobal.mu.Unlock() + return &commentWindowConn{Conn: conn, state: state}, nil +} + +type commentWindowConn struct { + driver.Conn + state *commentWindowTraceState +} + +func (c *commentWindowConn) Prepare(query string) (driver.Stmt, error) { + stmt, err := c.Conn.Prepare(query) + if err != nil || c.state == nil || !isCommentCountQuery(query) { + return stmt, err + } + return &commentWindowStmt{Stmt: stmt, state: c.state}, nil +} + +func (c *commentWindowConn) PrepareContext(ctx context.Context, query string) (driver.Stmt, error) { + preparer, ok := c.Conn.(driver.ConnPrepareContext) + if !ok { + return c.Prepare(query) + } + stmt, err := preparer.PrepareContext(ctx, query) + if err != nil || c.state == nil || !isCommentCountQuery(query) { + return stmt, err + } + return &commentWindowStmt{Stmt: stmt, state: c.state}, nil +} + +func (c *commentWindowConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) { + beginner, ok := c.Conn.(driver.ConnBeginTx) + if !ok { + legacy, ok := c.Conn.(interface { + Begin() (driver.Tx, error) + }) + if !ok { + return nil, driver.ErrSkip + } + return legacy.Begin() + } + return beginner.BeginTx(ctx, opts) +} + +func (c *commentWindowConn) QueryContext( + ctx context.Context, query string, args []driver.NamedValue, +) (driver.Rows, error) { + queryer, ok := c.Conn.(driver.QueryerContext) + if !ok { + return nil, driver.ErrSkip + } + rows, err := queryer.QueryContext(ctx, query, args) + if err != nil || c.state == nil || !isCommentCountQuery(query) { + return rows, err + } + return &commentWindowRows{Rows: rows, state: c.state}, nil +} + +func (c *commentWindowConn) ExecContext( + ctx context.Context, query string, args []driver.NamedValue, +) (driver.Result, error) { + execer, ok := c.Conn.(driver.ExecerContext) + if !ok { + return nil, driver.ErrSkip + } + return execer.ExecContext(ctx, query, args) +} + +type commentWindowRows struct { + driver.Rows + state *commentWindowTraceState +} + +func (r *commentWindowRows) Next(dest []driver.Value) error { + err := r.Rows.Next(dest) + if err == nil { + r.state.countOnce.Do(func() { + r.state.countProfiled <- struct{}{} + <-r.state.release + }) + } + return err +} + +type commentWindowStmt struct { + driver.Stmt + state *commentWindowTraceState +} + +func isCommentCountQuery(query string) bool { + return strings.Contains(strings.ToUpper(query), "SELECT COUNT(*) FROM TASK_COMMENTS") +} + +func (s *commentWindowStmt) Query(args []driver.Value) (driver.Rows, error) { + legacy, ok := s.Stmt.(interface { + Query([]driver.Value) (driver.Rows, error) + }) + if !ok { + return nil, driver.ErrSkip + } + rows, err := legacy.Query(args) + if err != nil { + return rows, err + } + return &commentWindowRows{Rows: rows, state: s.state}, nil +} + +func (s *commentWindowStmt) QueryContext( + ctx context.Context, args []driver.NamedValue, +) (driver.Rows, error) { + queryer, ok := s.Stmt.(driver.StmtQueryContext) + if !ok { + return nil, driver.ErrSkip + } + rows, err := queryer.QueryContext(ctx, args) + if err != nil { + return rows, err + } + return &commentWindowRows{Rows: rows, state: s.state}, nil +} + +func newConcurrentCommentWindowRepo(t *testing.T, state *commentWindowTraceState) *sqlite.Repository { + t.Helper() + commentWindowTraceStateGlobal.mu.Lock() + commentWindowTraceStateGlobal.state = state + commentWindowTraceStateGlobal.mu.Unlock() + t.Cleanup(func() { + state.releaseOnce.Do(func() { close(state.release) }) + commentWindowTraceStateGlobal.mu.Lock() + commentWindowTraceStateGlobal.state = nil + commentWindowTraceStateGlobal.mu.Unlock() + }) + + dsn := filepath.Join(t.TempDir(), "comments.db") + "?_journal_mode=WAL&_busy_timeout=5000" + writer, err := sqlx.Open("sqlite3_comments_window_test", dsn) + if err != nil { + t.Fatalf("open writer: %v", err) + } + reader, err := sqlx.Open("sqlite3_comments_window_test", dsn) + if err != nil { + t.Fatalf("open reader: %v", err) + } + writer.SetMaxOpenConns(4) + reader.SetMaxOpenConns(4) + t.Cleanup(func() { + _ = writer.Close() + _ = reader.Close() + }) + if _, _, err := settingsstore.Provide(writer, reader, nil); err != nil { + t.Fatalf("settings store: %v", err) + } + repo, err := sqlite.NewWithDB(writer, reader, nil) + if err != nil { + t.Fatalf("new repo: %v", err) + } + return repo +} + +// seedComment creates a comment with an explicit CreatedAt so ordering +// assertions don't depend on wall-clock timing between inserts. +func seedComment(t *testing.T, repo interface { + CreateTaskComment(ctx context.Context, c *models.TaskComment) error +}, taskID, body string, createdAt time.Time) *models.TaskComment { + t.Helper() + c := &models.TaskComment{ + TaskID: taskID, + AuthorType: "agent", + AuthorID: "agent-1", + Body: body, + Source: "run", + CreatedAt: createdAt, + } + if err := repo.CreateTaskComment(context.Background(), c); err != nil { + t.Fatalf("seed comment %q: %v", body, err) + } + return c +} + +// AC-003.1/AC-003.2/AC-003.3: the window selects the newest `limit` +// comments by (created_at DESC, id DESC) but presents them ascending by +// the same tiebreak columns. +func TestListTaskCommentsWindow_OrdersAscendingAndLimits(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + + c1 := seedComment(t, repo, "task-1", "first", base) + c2 := seedComment(t, repo, "task-1", "second", base.Add(time.Minute)) + c3 := seedComment(t, repo, "task-1", "third", base.Add(2*time.Minute)) + + comments, total, err := repo.ListTaskCommentsWindow(ctx, "task-1", 2) + if err != nil { + t.Fatalf("ListTaskCommentsWindow: %v", err) + } + if total != 3 { + t.Fatalf("total = %d, want 3", total) + } + if len(comments) != 2 { + t.Fatalf("len(comments) = %d, want 2", len(comments)) + } + // Newest 2 by created_at DESC are c3, c2; presented ascending: c2, c3. + if comments[0].ID != c2.ID || comments[1].ID != c3.ID { + t.Fatalf("order = [%s, %s], want [%s, %s]", comments[0].ID, comments[1].ID, c2.ID, c3.ID) + } + _ = c1 +} + +// AC-003.3: comments sharing an identical created_at break ties on id +// (DESC for the newest-N selection, which reverses to ASC when presented), +// so ordering is deterministic instead of depending on row/scan order. +func TestListTaskCommentsWindow_TiesBreakOnID(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + same := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + + c1 := seedComment(t, repo, "task-3", "a", same) + c2 := seedComment(t, repo, "task-3", "b", same) + c3 := seedComment(t, repo, "task-3", "c", same) + + byID := []*models.TaskComment{c1, c2, c3} + sort.Slice(byID, func(i, j int) bool { return byID[i].ID < byID[j].ID }) + wantAsc := []string{byID[0].ID, byID[1].ID, byID[2].ID} + + comments, total, err := repo.ListTaskCommentsWindow(ctx, "task-3", 20) + if err != nil { + t.Fatalf("ListTaskCommentsWindow: %v", err) + } + if total != 3 { + t.Fatalf("total = %d, want 3", total) + } + if len(comments) != 3 { + t.Fatalf("len(comments) = %d, want 3", len(comments)) + } + gotAsc := []string{comments[0].ID, comments[1].ID, comments[2].ID} + for i := range wantAsc { + if gotAsc[i] != wantAsc[i] { + t.Fatalf("order = %v, want %v (id-ascending tiebreak on equal created_at)", gotAsc, wantAsc) + } + } + + // A limit smaller than the tied set must still select the newest-by-id + // rows (DESC tiebreak for selection), presented ascending. + limited, _, err := repo.ListTaskCommentsWindow(ctx, "task-3", 2) + if err != nil { + t.Fatalf("ListTaskCommentsWindow(limit=2): %v", err) + } + if len(limited) != 2 { + t.Fatalf("len(limited) = %d, want 2", len(limited)) + } + wantLimited := []string{byID[1].ID, byID[2].ID} + gotLimited := []string{limited[0].ID, limited[1].ID} + if gotLimited[0] != wantLimited[0] || gotLimited[1] != wantLimited[1] { + t.Fatalf("limited order = %v, want %v (selects highest-id pair, presented ascending)", gotLimited, wantLimited) + } +} + +// AC-005.1: an empty result set returns a non-nil empty slice, not nil, +// so JSON marshals `[]` rather than `null`. +func TestListTaskCommentsWindow_EmptyReturnsNonNilSlice(t *testing.T) { + repo := newTestRepo(t) + comments, total, err := repo.ListTaskCommentsWindow(context.Background(), "no-such-task", 20) + if err != nil { + t.Fatalf("ListTaskCommentsWindow: %v", err) + } + if total != 0 { + t.Fatalf("total = %d, want 0", total) + } + if comments == nil { + t.Fatal("comments slice must be non-nil (empty), got nil") + } + if len(comments) != 0 { + t.Fatalf("len(comments) = %d, want 0", len(comments)) + } +} + +// AC-003.6: total reflects the full comment count on the task, independent +// of the requested limit. +func TestListTaskCommentsWindow_TotalIndependentOfLimit(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for i := 0; i < 5; i++ { + seedComment(t, repo, "task-2", "c", base.Add(time.Duration(i)*time.Minute)) + } + comments, total, err := repo.ListTaskCommentsWindow(ctx, "task-2", 1) + if err != nil { + t.Fatalf("ListTaskCommentsWindow: %v", err) + } + if total != 5 { + t.Fatalf("total = %d, want 5", total) + } + if len(comments) != 1 { + t.Fatalf("len(comments) = %d, want 1", len(comments)) + } +} + +// AC-003.7/AC-006.1/AC-006.3: the count and page share one read snapshot, +// while a concurrent writer can commit before the page query runs. The trace +// callback gives the writer a deterministic point between those two queries. +func TestListTaskCommentsWindow_SnapshotAllowsConcurrentWrite(t *testing.T) { + state := &commentWindowTraceState{ + countProfiled: make(chan struct{}, 1), + release: make(chan struct{}), + } + repo := newConcurrentCommentWindowRepo(t, state) + ctx := context.Background() + seedComment(t, repo, "task-concurrent", "before", time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)) + + type windowResult struct { + comments []*models.TaskComment + total int + err error + } + readDone := make(chan windowResult, 1) + go func() { + comments, total, err := repo.ListTaskCommentsWindow(ctx, "task-concurrent", 20) + readDone <- windowResult{comments: comments, total: total, err: err} + }() + + select { + case <-state.countProfiled: + case <-time.After(2 * time.Second): + t.Fatal("read did not reach the count query") + } + + writeDone := make(chan error, 1) + go func() { + writeDone <- repo.CreateTaskComment(ctx, &models.TaskComment{ + TaskID: "task-concurrent", + AuthorType: "agent", + AuthorID: "agent-1", + Body: "after", + Source: "run", + CreatedAt: time.Date(2026, 1, 1, 0, 1, 0, 0, time.UTC), + }) + }() + select { + case err := <-writeDone: + if err != nil { + t.Fatalf("concurrent write: %v", err) + } + case <-time.After(2 * time.Second): + state.releaseOnce.Do(func() { close(state.release) }) + t.Fatal("concurrent write blocked while the read transaction was paused") + } + state.releaseOnce.Do(func() { close(state.release) }) + + var got windowResult + select { + case got = <-readDone: + case <-time.After(2 * time.Second): + t.Fatal("read did not finish after releasing the snapshot barrier") + } + if got.err != nil { + t.Fatalf("ListTaskCommentsWindow: %v", got.err) + } + if got.total != 1 || len(got.comments) != 1 || got.comments[0].Body != "before" { + t.Fatalf("snapshot = total %d comments %v, want the complete pre-write snapshot", got.total, got.comments) + } + + comments, total, err := repo.ListTaskCommentsWindow(ctx, "task-concurrent", 20) + if err != nil { + t.Fatalf("post-write ListTaskCommentsWindow: %v", err) + } + if total != 2 || len(comments) != 2 { + t.Fatalf("post-write window = total %d comments %d, want 2", total, len(comments)) + } +} diff --git a/apps/backend/internal/office/routes.go b/apps/backend/internal/office/routes.go index 222e0998af..b091048ad9 100644 --- a/apps/backend/internal/office/routes.go +++ b/apps/backend/internal/office/routes.go @@ -17,10 +17,14 @@ import ( "github.com/kandev/kandev/internal/office/skills" "github.com/kandev/kandev/internal/office/tree_controls" "github.com/kandev/kandev/internal/office/workspaces" + taskservice "github.com/kandev/kandev/internal/task/service" ) // RegisterAllRoutes delegates route registration to each feature package. -func RegisterAllRoutes(router *gin.RouterGroup, svcs *Services, log *logger.Logger) { +// handoff wires the guarded agent-caller comment-read branch of +// dashboard.listComments; it may be nil where no HandoffService is +// available, in which case an agent request to that route responds 503. +func RegisterAllRoutes(router *gin.RouterGroup, svcs *Services, handoff *taskservice.HandoffService, log *logger.Logger) { agents.RegisterRoutes(router, svcs.Agents, log) officeruntime.RegisterRoutes(router, officeruntime.NewHandler( svcs.Agents, @@ -59,7 +63,7 @@ func RegisterAllRoutes(router *gin.RouterGroup, svcs *Services, log *logger.Logg configHandler := config.NewHandler(svcs.Config, log) config.RegisterRoutes(router, configHandler) - dashboard.RegisterRoutes(router, svcs.Dashboard, svcs.Repo, svcs.GitManager, log) + dashboard.RegisterRoutes(router, svcs.Dashboard, svcs.Repo, svcs.GitManager, handoff, log) if svcs.Documents != nil { docHandler := dashboard.NewDocumentHandler(svcs.Documents, svcs.KandevHome, log) diff --git a/apps/backend/internal/office/runtime/actions.go b/apps/backend/internal/office/runtime/actions.go index 00c6422f84..45e9199890 100644 --- a/apps/backend/internal/office/runtime/actions.go +++ b/apps/backend/internal/office/runtime/actions.go @@ -386,7 +386,7 @@ func (a *Actions) PostComment(ctx context.Context, runCtx RunContext, taskID, bo AuthorID: runCtx.AgentID, Body: body, Source: "agent", - CreatedAt: time.Now(), + CreatedAt: time.Now().UTC(), } return a.deps.Comments.CreateComment(ctx, comment) } diff --git a/apps/backend/internal/office/truncate/truncate.go b/apps/backend/internal/office/truncate/truncate.go index a2035ec154..66774719fb 100644 --- a/apps/backend/internal/office/truncate/truncate.go +++ b/apps/backend/internal/office/truncate/truncate.go @@ -1,23 +1,11 @@ -// Package truncate provides rune-safe byte truncation for office content. -// -// It exists so the cut lives in exactly one place. The implementations it -// replaces were byte-for-byte identical copies in separate packages, which -// is how a future correctness fix lands in one and silently misses another. +// Package truncate preserves the Office import path for rune-safe byte +// truncation. The implementation lives in the dependency-neutral common +// package so task and Office code share one boundary rule. package truncate -// UTF8 returns s truncated to at most maxBytes, cutting at a rune -// boundary. When the cut would split a multi-byte rune the straddling -// bytes are dropped, so the result is always valid UTF-8. +import commontruncate "github.com/kandev/kandev/internal/common/truncate" + +// UTF8 delegates to the shared implementation. func UTF8(s string, maxBytes int) string { - if len(s) <= maxBytes { - return s - } - // Walk back from maxBytes to the start of the last fully-included - // rune. UTF-8 continuation bytes match 10xxxxxx (0x80..0xBF); the - // loop terminates as soon as we land on a non-continuation byte. - cut := maxBytes - for cut > 0 && (s[cut]&0xC0) == 0x80 { - cut-- - } - return s[:cut] + return commontruncate.UTF8(s, maxBytes) } diff --git a/apps/backend/internal/task/service/handoff_access.go b/apps/backend/internal/task/service/handoff_access.go index 2f82184eac..b461fa6c85 100644 --- a/apps/backend/internal/task/service/handoff_access.go +++ b/apps/backend/internal/task/service/handoff_access.go @@ -97,17 +97,26 @@ func canWriteDocuments(ctx context.Context, repo taskLookup, currentTaskID, targ // loadAccessPair resolves both tasks and runs the always-required guards: // non-empty IDs and matching workspaces. Returns (current, target, true) -// only when access evaluation should proceed. +// only when access evaluation should proceed. A not-found task (the +// repository's ErrTaskNotFound sentinel, which embeds the raw task id) +// normalizes to a plain deny rather than propagating — callers must not +// leak the identifier via a raw error message. func loadAccessPair(ctx context.Context, repo taskLookup, currentTaskID, targetTaskID string) (*models.Task, *models.Task, bool, error) { if currentTaskID == "" || targetTaskID == "" { return nil, nil, false, nil } current, err := repo.GetTask(ctx, currentTaskID) if err != nil { + if errors.Is(err, repository.ErrTaskNotFound) { + return nil, nil, false, nil + } return nil, nil, false, err } target, err := repo.GetTask(ctx, targetTaskID) if err != nil { + if errors.Is(err, repository.ErrTaskNotFound) { + return nil, nil, false, nil + } return nil, nil, false, err } if current == nil || target == nil { @@ -150,7 +159,13 @@ func inAncestorChain(ctx context.Context, repo taskLookup, currentID, targetID s // ancestorIDs walks parent_id up from taskID, returning the chain of // ancestor IDs (excluding taskID itself). The walk stops at empty -// parent_id or after ancestorWalkHopCap hops, whichever comes first. +// parent_id or after ancestorWalkHopCap hops, whichever comes first. A +// not-found parent also stops the walk (returning the chain accumulated +// so far) rather than propagating the repository's identifier-embedding +// error — mirroring loadAccessPair's not-found normalization. A dangling +// parent_id is reachable via ordinary delete_task_kandev usage (deleting +// a task without reparenting its children), not just corrupt data, so +// this must degrade to a benign outcome rather than leak. func ancestorIDs(ctx context.Context, repo taskLookup, taskID string) ([]string, error) { var out []string current := taskID @@ -158,6 +173,9 @@ func ancestorIDs(ctx context.Context, repo taskLookup, taskID string) ([]string, for i := 0; i < ancestorWalkHopCap; i++ { t, err := repo.GetTask(ctx, current) if err != nil { + if errors.Is(err, repository.ErrTaskNotFound) { + return out, nil + } return nil, err } if t == nil || t.ParentID == "" { diff --git a/apps/backend/internal/task/service/handoff_access_test.go b/apps/backend/internal/task/service/handoff_access_test.go index 2a7fab091e..3a8bb54abb 100644 --- a/apps/backend/internal/task/service/handoff_access_test.go +++ b/apps/backend/internal/task/service/handoff_access_test.go @@ -2,9 +2,11 @@ package service import ( "context" + "fmt" "testing" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository" ) // fakeTaskLookup is a tiny in-memory implementation of taskLookup keyed @@ -13,11 +15,15 @@ type fakeTaskLookup struct { tasks map[string]*models.Task } +// GetTask mirrors the production repository shape (task/repository/sqlite +// GetTask): a missing id returns repository.ErrTaskNotFound wrapped with +// the id, never a nil/nil result. Access-guard tests rely on this to prove +// loadAccessPair normalizes not-found into a plain deny. func (f *fakeTaskLookup) GetTask(ctx context.Context, id string) (*models.Task, error) { if t, ok := f.tasks[id]; ok { return t, nil } - return nil, nil + return nil, fmt.Errorf("%w: %s", repository.ErrTaskNotFound, id) } func (f *fakeTaskLookup) GetTasksByIDs(ctx context.Context, ids []string) ([]*models.Task, error) { var out []*models.Task @@ -198,6 +204,30 @@ func TestCanReadDocuments_MissingTasksDeny(t *testing.T) { } } +// REGRESSION (AC-001.11): a not-found task must normalize to a plain deny, +// not leak the wrapped repository.ErrTaskNotFound (which embeds the raw +// task id) up through the access guard. +func TestLoadAccessPair_NotFoundNormalizesToPlainDeny(t *testing.T) { + g := newGraph(newTask("known", "", "ws-1")) + ctx := context.Background() + + current, target, ok, err := loadAccessPair(ctx, g, "known", "missing") + if err != nil { + t.Fatalf("loadAccessPair err = %v, want nil (plain deny)", err) + } + if ok || current != nil || target != nil { + t.Fatalf("loadAccessPair(known, missing) = (%v, %v, %v), want (nil, nil, false)", current, target, ok) + } + + current, target, ok, err = loadAccessPair(ctx, g, "missing", "known") + if err != nil { + t.Fatalf("loadAccessPair err = %v, want nil (plain deny)", err) + } + if ok || current != nil || target != nil { + t.Fatalf("loadAccessPair(missing, known) = (%v, %v, %v), want (nil, nil, false)", current, target, ok) + } +} + func TestCanWriteDocuments_SelfAndAncestorOnly(t *testing.T) { g := newGraph( newTask("root", "", "ws-1"), @@ -273,6 +303,55 @@ func TestAncestorIDs_CycleHandled(t *testing.T) { } } +// REGRESSION (SEC-001): a dangling parent_id is reachable via ordinary +// delete_task_kandev usage — it deletes a task via a bare +// `DELETE FROM tasks WHERE id = ?` with no reparenting or cascade, so a +// surviving child's parent_id can point at a row that no longer exists. +// ancestorIDs must treat a not-found parent the same as reaching the end +// of the chain (return what it has accumulated so far), not propagate the +// wrapped repository.ErrTaskNotFound the way loadAccessPair already +// refuses to for its own two direct GetTask calls. +func TestAncestorIDs_DanglingParentNormalizesToAccumulatedChain(t *testing.T) { + // "child" points at "deleted-parent", which does not exist in the + // graph — simulating the state left behind by delete_task_kandev + // deleting a task that still had children. The dangling id itself is + // still recorded (added to the chain before the walk discovers, on + // the next hop, that it doesn't exist) — the same + // append-before-verify shape TestAncestorIDs_CycleHandled already + // tolerates. What matters is the walk stops cleanly instead of + // propagating an error: neither loadAccessPair's `current` nor + // `target` can ever equal a dangling id (both are validated to exist + // before ancestorIDs runs), so its mere presence in this slice is + // harmless for the access decision. + g := newGraph(newTask("child", "deleted-parent", "ws-1")) + got, err := ancestorIDs(context.Background(), g, "child") + if err != nil { + t.Fatalf("ancestorIDs err = %v, want nil (dangling parent must normalize, not error)", err) + } + if len(got) != 1 || got[0] != "deleted-parent" { + t.Fatalf("ancestorIDs = %v, want [\"deleted-parent\"] (walk stops cleanly at the dangling hop)", got) + } +} + +// REGRESSION (SEC-001): the same dangling-parent scenario must not leak a +// raw internal error out of canReadDocuments either — the guard must reach +// a normal allow/deny decision instead of propagating the wrapped +// repository.ErrTaskNotFound (which embeds the deleted task's id) up to +// mapHandoffError's uncaught default branch. +func TestCanReadDocuments_DanglingParentDoesNotLeakRawError(t *testing.T) { + g := newGraph( + newTask("child", "deleted-parent", "ws-1"), + newTask("unrelated", "", "ws-1"), + ) + ok, err := canReadDocuments(context.Background(), g, nil, "child", "unrelated") + if err != nil { + t.Fatalf("canReadDocuments err = %v, want nil (dangling ancestor must not leak)", err) + } + if ok { + t.Fatal("canReadDocuments(child, unrelated) = true, want false (no real relation)") + } +} + // itoa avoids pulling in strconv. func itoa(i int) string { if i == 0 { diff --git a/apps/backend/internal/task/service/handoff_comments.go b/apps/backend/internal/task/service/handoff_comments.go new file mode 100644 index 0000000000..05d181e609 --- /dev/null +++ b/apps/backend/internal/task/service/handoff_comments.go @@ -0,0 +1,171 @@ +package service + +import ( + "context" + "errors" + "time" + + "github.com/kandev/kandev/internal/common/truncate" +) + +// commentBodyMaxBytes and commentResponseBudgetBytes are the per-comment +// and aggregate byte caps required by REQ-OFFICE-AGENT-COMMENT-READS-004. +const ( + commentBodyMaxBytes = 8192 + commentResponseBudgetBytes = 65536 +) + +// commentWindowDefaultLimit and commentWindowMaxLimit bound the comment +// window returned to callers (default 20, clamp 100). The dashboard HTTP +// handler (handler_comments.go) parses its "limit" query parameter with +// strconv.Atoi and discards the error, so a missing, non-numeric, zero, or +// negative value all reach this method as 0 or negative with no validation +// of its own — clamp here so any non-positive or oversized limit degrades to +// a bounded window instead of reaching SQLite's LIMIT clause raw (LIMIT 0 +// returns nothing, a negative LIMIT returns the entire table). +const ( + commentWindowDefaultLimit = 20 + commentWindowMaxLimit = 100 +) + +// clampCommentWindowLimit normalizes a caller-supplied limit to the +// documented [1, commentWindowMaxLimit] window, defaulting a non-positive +// value to commentWindowDefaultLimit. +func clampCommentWindowLimit(limit int) int { + if limit <= 0 { + return commentWindowDefaultLimit + } + if limit > commentWindowMaxLimit { + return commentWindowMaxLimit + } + return limit +} + +// errCommentReaderNotConfigured is returned when ListCommentsForCaller is +// called before SetCommentReader wires a backing store. Deliberately not a +// shared sentinel like ErrAccessDenied / ErrDocumentTaskRequired — a caller +// must never confuse "dependency unconfigured" with either of those +// (AC-005.2/AC-005.3). +var errCommentReaderNotConfigured = errors.New("comment reader not configured") + +// CommentRecord is the task-service-owned projection returned by a comment +// store. It contains only fields needed by the handoff read contract and does +// not expose an Office persistence model to this service. +type CommentRecord struct { + ID string + TaskID string + AuthorType string + AuthorID string + Source string + Body string + CreatedAt time.Time +} + +// CommentReader is the minimal read surface HandoffService.ListCommentsForCaller +// depends on. Persistence adapters convert their storage model to CommentRecord. +type CommentReader interface { + ListTaskCommentsWindow(ctx context.Context, taskID string, limit int) ([]CommentRecord, int, error) +} + +// CommentProjection is the wire shape of a single comment returned by +// ListCommentsForCaller. It deliberately omits ReplyChannelID and any +// per-comment run lifecycle field (AC-002.4). +type CommentProjection struct { + ID string `json:"id"` + TaskID string `json:"task_id"` + AuthorType string `json:"author_type"` + AuthorID string `json:"author_id"` + Source string `json:"source"` + Body string `json:"body"` + CreatedAt time.Time `json:"created_at"` + BodyTruncated bool `json:"body_truncated,omitempty"` + BodyBytes int `json:"body_bytes,omitempty"` +} + +// CommentWindow is the response envelope for ListCommentsForCaller. +type CommentWindow struct { + Comments []CommentProjection `json:"comments"` + Total int `json:"total"` + Returned int `json:"returned"` + HasMore bool `json:"has_more"` +} + +// ListCommentsForCaller returns targetTaskID's comments after the same +// read-access guard the document tools use. There is no "self"/empty +// sentinel: targetTaskID crosses only from the caller-supplied argument (a +// raw path segment on the HTTP transport, interpolated without validation) +// and is resolved by the guard like any other target +// (AC-OFFICE-AGENT-COMMENT-READS-005.4). A caller with no task identity +// (callerTaskID == "") is denied on this same path for every target, rather +// than surfacing a distinct missing-target validation error +// (AC-OFFICE-AGENT-COMMENT-READS-001.13). +func (s *HandoffService) ListCommentsForCaller(ctx context.Context, callerTaskID, targetTaskID string, limit int) (*CommentWindow, error) { + ok, err := canReadDocuments(ctx, repoTaskLookupAdapter{r: s.tasks}, blockerLookupAdapter{repo: s.blockers}, callerTaskID, targetTaskID) + if err != nil { + return nil, err + } + if !ok { + return nil, ErrAccessDenied + } + + if s.comments == nil { + return nil, errCommentReaderNotConfigured + } + rows, total, err := s.comments.ListTaskCommentsWindow(ctx, targetTaskID, clampCommentWindowLimit(limit)) + if err != nil { + return nil, err + } + + projected := make([]CommentProjection, 0, len(rows)) + for _, c := range rows { + projected = append(projected, projectComment(c)) + } + projected = trimCommentsToBudget(projected, commentResponseBudgetBytes) + + return &CommentWindow{ + Comments: projected, + Total: total, + Returned: len(projected), + HasMore: len(projected) < total, + }, nil +} + +// trimCommentsToBudget drops whole comments from the oldest end (index 0) +// of an ascending-ordered slice until the summed body bytes fit within +// budgetBytes, per AC-004.6/004.7/004.8. The `len(comments)-start > 1` +// guard is deliberately generic rather than relying on the current +// 8192/65536 constants: even if those change, a non-empty window is never +// reduced to empty (AC-004.9/004.10) — the single newest comment always +// survives. +func trimCommentsToBudget(comments []CommentProjection, budgetBytes int) []CommentProjection { + total := 0 + for _, c := range comments { + total += len(c.Body) + } + start := 0 + for total > budgetBytes && len(comments)-start > 1 { + total -= len(comments[start].Body) + start++ + } + return comments[start:] +} + +// projectComment maps a stored comment onto the task-service-owned wire +// projection and applies the per-body byte cap. +func projectComment(c CommentRecord) CommentProjection { + p := CommentProjection{ + ID: c.ID, + TaskID: c.TaskID, + AuthorType: c.AuthorType, + AuthorID: c.AuthorID, + Source: c.Source, + Body: c.Body, + CreatedAt: c.CreatedAt, + } + if len(c.Body) > commentBodyMaxBytes { + p.BodyBytes = len(c.Body) + p.Body = truncate.UTF8(c.Body, commentBodyMaxBytes) + p.BodyTruncated = true + } + return p +} diff --git a/apps/backend/internal/task/service/handoff_comments_test.go b/apps/backend/internal/task/service/handoff_comments_test.go new file mode 100644 index 0000000000..818e4f5427 --- /dev/null +++ b/apps/backend/internal/task/service/handoff_comments_test.go @@ -0,0 +1,367 @@ +package service + +import ( + "context" + "errors" + "strings" + "testing" + "time" + "unicode/utf8" +) + +// fakeCommentReader is a minimal in-memory CommentReader. Comments for a +// task are stored ascending by insertion order (matching the ordering the +// real repository presents); ListTaskCommentsWindow slices the newest +// `limit` while preserving that ascending order, mirroring the production +// contract without touching SQLite. +type fakeCommentReader struct { + byTask map[string][]CommentRecord + err error + lastLimit int +} + +func (f *fakeCommentReader) ListTaskCommentsWindow(_ context.Context, taskID string, limit int) ([]CommentRecord, int, error) { + f.lastLimit = limit + if f.err != nil { + return nil, 0, f.err + } + all := f.byTask[taskID] + total := len(all) + if limit <= 0 || limit > len(all) { + limit = len(all) + } + start := total - limit + if start < 0 { + start = 0 + } + window := make([]CommentRecord, len(all[start:])) + copy(window, all[start:]) + return window, total, nil +} + +func (f *fakeCommentReader) seed(taskID string, n int, bodyLen int) { + if f.byTask == nil { + f.byTask = map[string][]CommentRecord{} + } + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for i := 0; i < n; i++ { + body := strings.Repeat("x", bodyLen) + f.byTask[taskID] = append(f.byTask[taskID], CommentRecord{ + ID: "c-" + taskID + "-" + itoa(i), + TaskID: taskID, + AuthorType: "agent", + AuthorID: "agent-1", + Source: "run", + Body: body, + CreatedAt: base.Add(time.Duration(i) * time.Minute), + }) + } +} + +func TestListCommentsForCallerEnforcesReadAccess(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + reader.seed("parent", 1, 10) + reader.seed("stranger", 1, 10) + svc.SetCommentReader(reader) + ctx := context.Background() + + if _, err := svc.ListCommentsForCaller(ctx, "child-a", "parent", 20); err != nil { + t.Fatalf("ancestor read: %v", err) + } + if _, err := svc.ListCommentsForCaller(ctx, "child-a", "stranger", 20); !errors.Is(err, ErrAccessDenied) { + t.Fatalf("unrelated read = %v, want ErrAccessDenied", err) + } + if _, err := svc.ListCommentsForCaller(ctx, "child-a", "foreign", 20); !errors.Is(err, ErrAccessDenied) { + t.Fatalf("cross-workspace read = %v, want ErrAccessDenied", err) + } + // AC-001.5: a nonexistent target reports the identical access-denied + // error as an unrelated existing task. + if _, err := svc.ListCommentsForCaller(ctx, "child-a", "does-not-exist", 20); !errors.Is(err, ErrAccessDenied) { + t.Fatalf("nonexistent target = %v, want ErrAccessDenied", err) + } +} + +// AC-OFFICE-AGENT-COMMENT-READS-008.1: the feature's sole acceptance-path +// scenario is a PARENT reading a CHILD's comment (the child posted a stage +// deliverable; the parent reads it on its next run). Every other case in +// this file exercises the caller reading an ancestor ("child-a" -> "parent") +// or itself — none cover this descendant direction end-to-end through +// ListCommentsForCaller (windowing, projection, truncation), even though +// the underlying canReadDocuments guard supports it generically. +func TestListCommentsForCallerAllowsDescendantRead(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + reader.seed("child-a", 1, 10) + svc.SetCommentReader(reader) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "parent", "child-a", 20) + if err != nil { + t.Fatalf("parent reading child-a's comments: %v", err) + } + if w.Total != 1 || len(w.Comments) != 1 { + t.Fatalf("window = %+v, want exactly the one comment child-a posted", w) + } +} + +// AC-OFFICE-AGENT-COMMENT-READS-005.4: the target crosses only from the +// caller-supplied target argument. There is no "self"/empty sentinel — an +// unrecognised or empty target is resolved by the guard like any other +// target (denied, since it matches no real task) rather than substituted +// with the caller's own task. +func TestListCommentsForCallerTreatsUnresolvedTargetsLiterally(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + reader.seed("child-a", 2, 10) + svc.SetCommentReader(reader) + ctx := context.Background() + + for _, target := range []string{"", " ", "self"} { + if _, err := svc.ListCommentsForCaller(ctx, "child-a", target, 20); !errors.Is(err, ErrAccessDenied) { + t.Fatalf("target %q: err = %v, want ErrAccessDenied (no sentinel substitution)", target, err) + } + } +} + +// AC-OFFICE-AGENT-COMMENT-READS-001.13: a caller whose JWT carries no task +// claim (callerTaskID == "") is denied on this same path for every target — +// not a distinct missing-target validation error, which would let a caller +// distinguish "I have no identity" from "I am not related to this task". +func TestListCommentsForCallerDeniesEveryTargetWhenCallerTaskEmpty(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + reader.seed("parent", 1, 10) + svc.SetCommentReader(reader) + ctx := context.Background() + + for _, target := range []string{"", " ", "self", "parent"} { + if _, err := svc.ListCommentsForCaller(ctx, "", target, 20); !errors.Is(err, ErrAccessDenied) { + t.Fatalf("caller=%q target=%q: err = %v, want ErrAccessDenied", "", target, err) + } + } +} + +// AC-005.1: an accessible target with no comments succeeds with a non-nil +// empty list and total zero. +func TestListCommentsForCallerEmptySuccess(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + svc.SetCommentReader(&fakeCommentReader{}) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "child-a", "parent", 20) + if err != nil { + t.Fatalf("ListCommentsForCaller: %v", err) + } + if w.Total != 0 || w.Returned != 0 || w.HasMore { + t.Fatalf("window = %+v, want empty zero window", w) + } + if w.Comments == nil { + t.Fatal("Comments must be non-nil (empty), got nil") + } +} + +// AC-005.2/005.3: an unconfigured comment store is a distinct internal +// error, never an empty-but-successful result. +func TestListCommentsForCallerDependencyNotConfigured(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "child-a", "parent", 20) + if err == nil { + t.Fatal("want an error when the comment reader is unconfigured") + } + if errors.Is(err, ErrAccessDenied) || errors.Is(err, ErrDocumentTaskRequired) { + t.Fatalf("unconfigured dependency must not reuse the access/validation sentinels, got %v", err) + } + if w != nil { + t.Fatalf("window = %+v, want nil on error", w) + } +} + +// AC-004.1-004.5: bodies over 8192 bytes are truncated at a rune boundary +// and carry body_truncated/body_bytes; bodies at or under the cap are +// returned untouched with neither field set. +func TestListCommentsForCallerTruncatesLongBodies(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + body := strings.Repeat("x", 8191) + "é" + reader := &fakeCommentReader{byTask: map[string][]CommentRecord{ + "child-a": {{ + ID: "utf8-boundary", TaskID: "child-a", AuthorType: "agent", AuthorID: "agent-1", + Source: "run", Body: body, CreatedAt: time.Now().UTC(), + }}, + }} + svc.SetCommentReader(reader) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "child-a", "child-a", 20) + if err != nil { + t.Fatalf("ListCommentsForCaller: %v", err) + } + if len(w.Comments) != 1 { + t.Fatalf("len(comments) = %d, want 1", len(w.Comments)) + } + c := w.Comments[0] + if !c.BodyTruncated { + t.Fatal("want body_truncated = true for a 9000-byte body") + } + if c.BodyBytes != len(body) { + t.Fatalf("body_bytes = %d, want %d", c.BodyBytes, len(body)) + } + if len(c.Body) != 8191 { + t.Fatalf("truncated body len = %d, want 8191", len(c.Body)) + } + if !utf8.ValidString(c.Body) { + t.Fatal("truncated body must remain valid UTF-8") + } +} + +func TestListCommentsForCallerLeavesShortBodiesUntouched(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + reader.seed("child-a", 1, 42) + svc.SetCommentReader(reader) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "child-a", "child-a", 20) + if err != nil { + t.Fatalf("ListCommentsForCaller: %v", err) + } + c := w.Comments[0] + if c.BodyTruncated { + t.Fatal("want body_truncated = false for a 42-byte body") + } + if c.BodyBytes != 0 { + t.Fatalf("body_bytes = %d, want 0 (omitted)", c.BodyBytes) + } + if len(c.Body) != 42 { + t.Fatalf("body len = %d, want 42", len(c.Body)) + } +} + +// AC-004.5: a body of exactly 8192 bytes is the boundary case — at the cap, +// not over it, so it must pass through untouched rather than truncate. +func TestListCommentsForCallerLeavesExactCapBodyUntouched(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + reader.seed("child-a", 1, 8192) + svc.SetCommentReader(reader) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "child-a", "child-a", 20) + if err != nil { + t.Fatalf("ListCommentsForCaller: %v", err) + } + c := w.Comments[0] + if c.BodyTruncated { + t.Fatal("want body_truncated = false for an exactly-8192-byte body") + } + if c.BodyBytes != 0 { + t.Fatalf("body_bytes = %d, want 0 (omitted)", c.BodyBytes) + } + if len(c.Body) != 8192 { + t.Fatalf("body len = %d, want 8192", len(c.Body)) + } +} + +// AC-004.6-004.9: the aggregate 65536-byte body budget drops whole +// comments from the oldest end, never emptying a non-empty window. +func TestListCommentsForCallerBudgetDropsOldestNeverEmpties(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + // 10 comments at 8192 bytes each = 81920 bytes, over the 65536 budget. + reader.seed("child-a", 10, 8192) + svc.SetCommentReader(reader) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "child-a", "child-a", 20) + if err != nil { + t.Fatalf("ListCommentsForCaller: %v", err) + } + if w.Total != 10 { + t.Fatalf("total = %d, want 10", w.Total) + } + if w.Returned == 0 || w.Returned >= 10 { + t.Fatalf("returned = %d, want some comments dropped but not empty", w.Returned) + } + if !w.HasMore { + t.Fatal("want has_more = true when comments were budget-dropped") + } + if len(w.Comments) != w.Returned { + t.Fatalf("len(Comments) = %d, want == Returned (%d)", len(w.Comments), w.Returned) + } + // The newest comment (highest index) must be the one retained. + newest := "c-child-a-9" + if w.Comments[len(w.Comments)-1].ID != newest { + t.Fatalf("last retained comment = %s, want %s (the newest)", w.Comments[len(w.Comments)-1].ID, newest) + } + + var sum int + for _, c := range w.Comments { + sum += len(c.Body) + } + if sum > 65536 { + t.Fatalf("retained body bytes = %d, want <= 65536", sum) + } +} + +// AC-002.2/002.4: projection carries author_type/author_id/source and +// omits reply_channel_id (the struct has no such field to leak). +func TestListCommentsForCallerProjectsAuthorFields(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{ + byTask: map[string][]CommentRecord{ + "child-a": {{ + ID: "c1", TaskID: "child-a", AuthorType: "user", AuthorID: "user-9", + Source: "user", Body: "hi", + CreatedAt: time.Now().UTC(), + }}, + }, + } + svc.SetCommentReader(reader) + ctx := context.Background() + + w, err := svc.ListCommentsForCaller(ctx, "child-a", "child-a", 20) + if err != nil { + t.Fatalf("ListCommentsForCaller: %v", err) + } + c := w.Comments[0] + if c.AuthorType != "user" || c.AuthorID != "user-9" || c.Source != "user" { + t.Fatalf("projection = %+v", c) + } +} + +// The dashboard HTTP handler's strconv.Atoi(c.Query("limit")) performs no +// validation of its own (see handoff_comments.go), so a missing, zero, or +// negative "limit" query parameter reaches ListCommentsForCaller unclamped. +// Assert the service clamps at its own boundary so a raw, unnormalized limit +// never reaches SQLite's LIMIT clause (LIMIT 0 returns zero rows, a negative +// LIMIT returns the whole table under SQLite's "no limit" semantics). +func TestListCommentsForCallerClampsLimitAtServiceBoundary(t *testing.T) { + cases := []struct { + name string + requested int + want int + }{ + {"omitted/zero defaults", 0, commentWindowDefaultLimit}, + {"negative defaults", -1, commentWindowDefaultLimit}, + {"within range passes through", 5, 5}, + {"over max clamps", 500, commentWindowMaxLimit}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + svc, _ := newDocumentHandoffService(t, nil) + reader := &fakeCommentReader{} + reader.seed("child-a", 1, 10) + svc.SetCommentReader(reader) + ctx := context.Background() + + if _, err := svc.ListCommentsForCaller(ctx, "child-a", "child-a", tc.requested); err != nil { + t.Fatalf("ListCommentsForCaller: %v", err) + } + if reader.lastLimit != tc.want { + t.Fatalf("limit passed to ListTaskCommentsWindow = %d, want %d", reader.lastLimit, tc.want) + } + }) + } +} diff --git a/apps/backend/internal/task/service/handoff_service.go b/apps/backend/internal/task/service/handoff_service.go index 0c8cbb014b..74c50c4bc1 100644 --- a/apps/backend/internal/task/service/handoff_service.go +++ b/apps/backend/internal/task/service/handoff_service.go @@ -201,6 +201,7 @@ type HandoffService struct { eventPublisher TaskEventPublisher resourceCleaner TaskResourceCleaner taskAccessCheck func(ctx context.Context, taskID string) error + comments CommentReader logger *logger.Logger parentLock parentMutex workspaceGroupLock parentMutex @@ -293,6 +294,15 @@ func (s *HandoffService) SetTaskAccessChecker(check func(ctx context.Context, ta s.taskAccessCheck = check } +// SetCommentReader wires the read-only comment store queried by +// ListCommentsForCaller. Optional — when nil, ListCommentsForCaller reports +// an internal error rather than falling back to an empty list (AC-005.2: +// an unconfigured dependency must never look like "the task has no +// comments"). +func (s *HandoffService) SetCommentReader(r CommentReader) { + s.comments = r +} + // authorizeTask applies the configured per-user task check. No-op when unwired. func (s *HandoffService) authorizeTask(ctx context.Context, taskID string) error { if s.taskAccessCheck == nil || taskID == "" { diff --git a/apps/web/e2e/tests/office/agentctl-cli.spec.ts b/apps/web/e2e/tests/office/agentctl-cli.spec.ts index 019cf00f81..42d46eecd1 100644 --- a/apps/web/e2e/tests/office/agentctl-cli.spec.ts +++ b/apps/web/e2e/tests/office/agentctl-cli.spec.ts @@ -347,6 +347,82 @@ test.describe("agentctl kandev CLI", () => { expect((comments.comments ?? []).some((c) => c.body === body)).toBe(true); }); + test("comment list reads the caller's own task, reads a child task, and is denied on an unrelated task", async ({ + apiClient, + officeApi, + backend, + officeSeed, + }) => { + // ownTask is the caller's task. childTask is a genuine child of ownTask, + // so this also exercises AC-OFFICE-AGENT-COMMENT-READS-008.1: a parent + // task's agent reading a comment its child posted. unrelatedTask is an + // independent root task in the same workspace with no parent/child, + // sibling, or blocker relation to either. + const ownTask = await apiClient.createTask( + officeSeed.workspaceId, + `CLI Comment List Own ${Date.now()}`, + { workflow_id: officeSeed.workflowId }, + ); + const childTask = await apiClient.createTask( + officeSeed.workspaceId, + `CLI Comment List Child ${Date.now()}`, + { workflow_id: officeSeed.workflowId, parent_id: ownTask.id as string }, + ); + const unrelatedTask = await apiClient.createTask( + officeSeed.workspaceId, + `CLI Comment List Unrelated ${Date.now()}`, + { workflow_id: officeSeed.workflowId }, + ); + + const ownBody = `own-task-comment-${Date.now()}`; + const childBody = `child-task-comment-${Date.now()}`; + await officeApi.createTaskComment(ownTask.id as string, ownBody); + await officeApi.createTaskComment(childTask.id as string, childBody); + await officeApi.createTaskComment( + unrelatedTask.id as string, + `unrelated-comment-${Date.now()}`, + ); + + const run = await apiClient.seedRun({ + agentProfileId: officeSeed.agentId, + reason: "task_assigned", + status: "claimed", + taskId: ownTask.id, + }); + const { token } = await apiClient.mintRuntimeToken({ + agentProfileId: officeSeed.agentId, + workspaceId: officeSeed.workspaceId, + runId: run.run_id, + taskId: ownTask.id, + }); + + const env: NodeJS.ProcessEnv = { + KANDEV_API_URL: backend.baseUrl, + KANDEV_API_KEY: token, + KANDEV_WORKSPACE_ID: officeSeed.workspaceId, + KANDEV_AGENT_ID: officeSeed.agentId, + KANDEV_RUN_ID: run.run_id, + KANDEV_TASK_ID: ownTask.id as string, + }; + + // Same-task read succeeds and returns the guarded window. + const ownRes = runCLI(env, ["comment", "list", "--task", ownTask.id as string]); + expect(ownRes.exitCode, `stderr=${ownRes.stderr}`).toBe(0); + const ownParsed = JSON.parse(ownRes.stdout) as { comments?: Array<{ body?: string }> }; + expect((ownParsed.comments ?? []).some((c) => c.body === ownBody)).toBe(true); + + // Descendant read succeeds: the parent's agent can read the comment its + // child task posted, with no human action between the two runs. + const childRes = runCLI(env, ["comment", "list", "--task", childTask.id as string]); + expect(childRes.exitCode, `stderr=${childRes.stderr}`).toBe(0); + const childParsed = JSON.parse(childRes.stdout) as { comments?: Array<{ body?: string }> }; + expect((childParsed.comments ?? []).some((c) => c.body === childBody)).toBe(true); + + // Cross-task read on an unrelated in-workspace task is denied. + const deniedRes = runCLI(env, ["comment", "list", "--task", unrelatedTask.id as string]); + expect(deniedRes.exitCode).toBe(1); + }); + test("routines create persists a routine with the supplied name", async ({ apiClient, officeApi, diff --git a/docs/specs/office/README.md b/docs/specs/office/README.md index a954ad14b7..05d6335211 100644 --- a/docs/specs/office/README.md +++ b/docs/specs/office/README.md @@ -32,6 +32,7 @@ dashboard projections, and Office testing contracts. +- [Office: Agent Comment Reads](requirements/agent-comment-reads.md) - [Office: Agents](requirements/agents.md) - [Office: Personal Assistant Agent, Channels & Agent Memory](requirements/assistant.md) - [Automation runs — status-scoped delete all](requirements/automation-runs-delete-all-by-status.md) @@ -58,6 +59,7 @@ dashboard projections, and Office testing contracts. +- [Office: Agent Comment Reads System Design](system-design/agent-comment-reads-01.md) - [Office: Agents System Design Part 1](system-design/agents-01.md) - [Office: Agents System Design Part 2](system-design/agents-02.md) - [Office: Agents System Design Part 3](system-design/agents-03.md) diff --git a/docs/specs/office/requirements/agent-comment-reads.md b/docs/specs/office/requirements/agent-comment-reads.md new file mode 100644 index 0000000000..ebc8b5cf23 --- /dev/null +++ b/docs/specs/office/requirements/agent-comment-reads.md @@ -0,0 +1,367 @@ +--- +status: draft +system: office +created: 2026-08-25 +owners: + - Kandev +--- + +# Office: Agent Comment Reads Requirements + +## Overview + +Every Office role instruction file tells its agent to post task comments. No +Office agent can usefully read one, so a producer stage writes its deliverable +to a channel no consumer stage can open on the terms it needs, and multi-stage +delegation cannot hand off. + +The read endpoint already exists and the agent CLI already calls it, so the gap +is not a missing transport. It is a read that returns the wrong thing: it +applies no cross-task relation check, and it ignores the caller's requested +size, returning a task's whole comment history unbounded. This document makes +that endpoint safe and bounded for agent callers and leaves its human-facing +behaviour unchanged. Office owns this contract because it owns agent +coordination; the comment store and access rule are consumed, not redefined. + +## Design decision + +Three options were considered. + +1. Expose comments to the Office MCP surface as a read tool. +2. Redirect role instructions at `write_task_document_kandev`, treating + comments as human-facing discussion only. +3. Fix the existing REST read the agent CLI already calls, and teach the + instructions that CLI command. + +**Option 3 is chosen. It supersedes an earlier selection of option 1, which was +built and then withdrawn.** Office mode's MCP surface is deliberately minimal; +its own registry records that "Kanban tools are excluded because office agents +use CLI commands instead", and nine shipped skills under +`internal/office/configloader/skills/` are already built on that CLI. An MCP +tool contradicted that boundary and routed around a defective endpoint instead +of repairing it: `agentctl kandev comment list` and `agentctl kandev tasks +conversation` both already reach this endpoint, so the unguarded, unbounded read +would have survived beside a new guarded one. Option 2 stays rejected because +six role files already direct agents to post comments, making it a rewrite of +every role rather than an instruction edit, after which that text becomes +load-bearing for every future role author. A comment and a document are also not +interchangeable: a document is replaced wholesale by key, so two stages +publishing to a shared parent collide on key choice, while a comment is an +append-only attributed event. + +The access rule, byte budget, ordering guarantee, and window reporting are +unchanged; only the surface carrying them moved. + +## Prior art + +**Our own prior reasoning.** Attempted, not skipped: the vault at +`/Users/henry/Documents/henry/wiki` returned `Operation not permitted` and the +`obsidian-wiki` CLI is not installed, so this leg returned nothing. + +**What other products shipped.** Queried through the `saas-kb` `ai_sdlc` slice. +*Warp* gives parent and child agents inboxes on a durable bus, is explicit that +"agents don't read each other's transcripts or live working trees", and has each +review-swarm child post findings and exit while the parent fans them in. *Devin* +lets a coordinator "inspect any session's full event timeline". *OpenHands* +returned nothing on cross-agent handoff. + +**What we are doing differently.** Not a bus or mailbox: Office already has a +durable, per-task, append-only, attributed channel agents are told to write to, +so the gap is a missing guarded reader. We deliberately do **not** adopt Warp's +global message sequence number: it orders a bus against payload-before-state +races, and this is a read gap, not a race, which is what makes the +stable-arbitrary tiebreak in REQ-OFFICE-AGENT-COMMENT-READS-003 acceptable. +Unlike Devin, the read is scoped by task relation, not blanket access to any +session a coordinator can name. + +## Terminology + +- **Comment read endpoint:** `GET /api/v1/office/tasks/{id}/comments`, the + existing dashboard route reached by the browser UI and by both agent CLI + commands `agentctl kandev comment list` and `kandev tasks conversation`. +- **Agent caller:** A request carrying a valid Office agent JWT, for which the + office route group records an authenticated agent on the request context. +- **Browser caller:** Any other request on that route, including the human + dashboard SPA. +- **Caller task:** The task an agent caller is bound to, taken from its + validated JWT claims, never from the request path, query, or body. +- **Target task:** The task whose comments are requested, taken from the route's + path parameter. +- **Read relation:** The existing cross-task read rule shared with the task + document tools: target is the caller itself, an ancestor, descendant, sibling + with a shared non-empty parent, or blocker of the caller, both in the same + workspace. +- **Window:** The `total` / `returned` / `has_more` triple describing how much + of a task's comment history a response covers. +- **Coordinator:** The `ceo` Office role, at + `apps/backend/internal/office/configloader/instructions/ceo/AGENTS.md`. No + role directory is named `coordinator`; the six are `ceo`, `devops`, `qa`, + `reviewer`, `security`, `worker`. + +## Requirements + +### REQ-OFFICE-AGENT-COMMENT-READS-001: Read a related task's comments + +**Intent:** Let a coordinating agent read the deliverable a child stage wrote, +so a fan-in completes without a human relaying the payload, and let no agent +read a task it has no relation to. + +**User story:** As an Office coordinator agent, I want to read a child task's +comments, so I can act on a completed stage's written output. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-001.1:** The comment read endpoint shall + apply the read relation to every agent caller. No MCP surface, Office + included, shall register a comment read tool. +- **AC-OFFICE-AGENT-COMMENT-READS-001.2:** When the caller task and the target + task satisfy the read relation, the endpoint shall return the target's + comments. +- **AC-OFFICE-AGENT-COMMENT-READS-001.3:** When the target task is a descendant + of the caller task, the read relation shall be satisfied, so a parent can + read a child's comments. +- **AC-OFFICE-AGENT-COMMENT-READS-001.4:** When the caller and target do not + satisfy the read relation, the endpoint shall return a forbidden status and no + comment content, including when both tasks are in the caller's own workspace. +- **AC-OFFICE-AGENT-COMMENT-READS-001.5:** When the target task does not exist, + the endpoint shall return the same forbidden status and the same message as an + unrelated existing task, so a caller cannot distinguish the two. +- **AC-OFFICE-AGENT-COMMENT-READS-001.6:** When the caller and target are in + different workspaces, the endpoint shall return a forbidden status even if a + parent, child, sibling, or blocker edge exists between them. +- **AC-OFFICE-AGENT-COMMENT-READS-001.7:** The read relation applied by this + endpoint shall be the same rule the task document read tools apply, evaluated + by the same shared implementation, so the two surfaces cannot diverge. +- **AC-OFFICE-AGENT-COMMENT-READS-001.8:** The endpoint's read method shall + remain read-only, with no way to create, edit, or delete a comment. +- **AC-OFFICE-AGENT-COMMENT-READS-001.9:** The shared read-relation guard shall + resolve a caller task or target task that does not exist to a plain denial, + emitting no error that names the task or distinguishes a nonexistent task + from an existing unrelated one, so that + AC-OFFICE-AGENT-COMMENT-READS-001.5 holds at the guard, not at one caller. +- **AC-OFFICE-AGENT-COMMENT-READS-001.10:** The normalisation required by + AC-OFFICE-AGENT-COMMENT-READS-001.9 shall take effect for every caller of the + shared guard, including the existing task document read and write tools, so + the surfaces cannot answer does-this-task-exist differently. +- **AC-OFFICE-AGENT-COMMENT-READS-001.11:** Any in-memory task lookup used to + test the shared guard shall return the same not-found result shape the task + repository returns for an absent identifier, so a guard test cannot report + success on a branch production never reaches. +- **AC-OFFICE-AGENT-COMMENT-READS-001.12:** The denial shall be reported through + the existing shared access-denied sentinel, whose message is the literal string + `document access denied`. No second access-denied sentinel or comment-specific + message shall be introduced, so the denial a caller sees is identical across + the comment and document surfaces. +- **AC-OFFICE-AGENT-COMMENT-READS-001.13:** When an agent caller's JWT carries + no caller task, the endpoint shall return the forbidden status of + AC-OFFICE-AGENT-COMMENT-READS-001.12 for every target, including one that + exists, and shall not fall back to an unguarded read. + +### REQ-OFFICE-AGENT-COMMENT-READS-002: Return every author's comments + +**Intent:** A stage's context includes what its peers wrote and what a human +told it. Dropping either silently hides a decision. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-002.1:** The endpoint shall return comments + regardless of author, including agent-authored and human-authored comments. +- **AC-OFFICE-AGENT-COMMENT-READS-002.2:** Each returned comment shall carry + its `author_type`, `author_id`, and `source` values as recorded. +- **AC-OFFICE-AGENT-COMMENT-READS-002.3:** The endpoint shall accept no author, + author-type, or source filter, so no caller receives a silently narrowed view + of a task's history. +- **AC-OFFICE-AGENT-COMMENT-READS-002.4:** A comment returned to an agent caller + shall omit `reply_channel_id` and shall omit per-comment run lifecycle fields. + +### REQ-OFFICE-AGENT-COMMENT-READS-003: Deterministic ordering and windowing + +**Intent:** A coordinator re-reading a thread must see the same thread. An +under-determined sort makes a fan-in unrepeatable and a truncated read +indistinguishable from an empty one. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-003.1:** For an agent caller the endpoint + shall select the most recent `limit` comments for the target task, ordered by + the `created_at` column descending and, where `created_at` values are equal, + by the `id` column descending. +- **AC-OFFICE-AGENT-COMMENT-READS-003.2:** The endpoint shall return that + selected window ordered by the `created_at` column ascending and, where + `created_at` values are equal, by the `id` column ascending, so the caller + reads the thread oldest-first. +- **AC-OFFICE-AGENT-COMMENT-READS-003.3:** The tiebreak column named in + AC-OFFICE-AGENT-COMMENT-READS-003.1 and AC-OFFICE-AGENT-COMMENT-READS-003.2 + shall be the same column in both directions, so the window boundary selects + the same comment set on every call. +- **AC-OFFICE-AGENT-COMMENT-READS-003.4:** When an agent caller supplies no + `limit` query parameter, or supplies one that is empty, unparseable as an + integer, zero, or negative, the endpoint shall apply a default limit of 20 and + shall not return an error. +- **AC-OFFICE-AGENT-COMMENT-READS-003.5:** When an agent caller's `limit` + exceeds 100, the endpoint shall clamp it to 100 and shall not return an error. +- **AC-OFFICE-AGENT-COMMENT-READS-003.6:** Every response to an agent caller + shall carry a window reporting `total`, the target's comment count; + `returned`, the number of comments in this response; and `has_more`, true + exactly when `returned` is less than `total`. +- **AC-OFFICE-AGENT-COMMENT-READS-003.7:** The total count and the returned + comments shall be read within a single read transaction, so the returned + count never exceeds the reported total. +- **AC-OFFICE-AGENT-COMMENT-READS-003.8:** Two calls with identical arguments + and no intervening comment write shall return byte-identical comment + identifiers in identical order. +- **AC-OFFICE-AGENT-COMMENT-READS-003.9:** No `limit` value from an agent caller + shall produce an error from any layer; every value shall be defaulted or + clamped per AC-OFFICE-AGENT-COMMENT-READS-003.4 and + AC-OFFICE-AGENT-COMMENT-READS-003.5. +- **AC-OFFICE-AGENT-COMMENT-READS-003.10:** The window field named in + AC-OFFICE-AGENT-COMMENT-READS-003.6 shall describe only how many comments the + response omits, and shall never report whether any body was shortened. +- **AC-OFFICE-AGENT-COMMENT-READS-003.11:** For a browser caller the endpoint + shall keep its current behaviour: no default limit, the full comment list, the + existing response shape including per-comment run lifecycle fields, and no + read-relation check. Its valid positive `limit` may be honoured; absent one, + no limit shall be imposed. + +### REQ-OFFICE-AGENT-COMMENT-READS-004: Bounded bodies + +**Intent:** A comment body is unbounded text, and an unbounded read can exhaust +a consuming agent's context and lose the deliverable it was fetched for. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-004.1:** Each comment body returned to an + agent caller shall be truncated to at most 8192 bytes. +- **AC-OFFICE-AGENT-COMMENT-READS-004.2:** A truncated body shall be cut at a + rune boundary and shall remain valid UTF-8. +- **AC-OFFICE-AGENT-COMMENT-READS-004.3:** When a body is shortened, the + returned comment shall carry a `body_truncated` marker and `body_bytes`, the + original body length in bytes. +- **AC-OFFICE-AGENT-COMMENT-READS-004.4:** When a body is not shortened, the + returned comment shall carry neither a `body_truncated` marker nor a + `body_bytes` value. +- **AC-OFFICE-AGENT-COMMENT-READS-004.5:** When a body is exactly 8192 bytes, + the endpoint shall return it unchanged and shall carry no `body_truncated` + marker. +- **AC-OFFICE-AGENT-COMMENT-READS-004.6:** The endpoint shall return at most + 65536 bytes of comment body across a single response to an agent caller. +- **AC-OFFICE-AGENT-COMMENT-READS-004.7:** When the window selected by + REQ-OFFICE-AGENT-COMMENT-READS-003 would exceed the budget in + AC-OFFICE-AGENT-COMMENT-READS-004.6, the endpoint shall drop whole comments + from the oldest end of the window until the response fits, so the newest + comments are always the ones retained. +- **AC-OFFICE-AGENT-COMMENT-READS-004.8:** Comments dropped under + AC-OFFICE-AGENT-COMMENT-READS-004.7 shall be excluded from `returned` and + shall not change `total`, so `has_more` reports them. +- **AC-OFFICE-AGENT-COMMENT-READS-004.9:** When the window selected by + REQ-OFFICE-AGENT-COMMENT-READS-003 is not empty, the response shall contain at + least one comment. The budget in AC-OFFICE-AGENT-COMMENT-READS-004.6 shall + never reduce a non-empty window to an empty list, because an empty list is + read by a coordinator as "the child produced nothing" and ends the delegation. +- **AC-OFFICE-AGENT-COMMENT-READS-004.10:** Should the constants in + AC-OFFICE-AGENT-COMMENT-READS-004.1 and AC-OFFICE-AGENT-COMMENT-READS-004.6 + ever change so that one shortened body could exceed the budget, that comment + shall still be returned alone, so + AC-OFFICE-AGENT-COMMENT-READS-004.9 does not depend on their present values. +- **AC-OFFICE-AGENT-COMMENT-READS-004.11:** Neither cap shall apply to a browser + caller, whose bodies are returned whole. + +### REQ-OFFICE-AGENT-COMMENT-READS-005: Empty, denied, and failed reads are distinguishable + +**Intent:** "The child posted nothing" and "the read did not work" are different +facts leading to opposite coordinator decisions. The reported defect is a +coordinator concluding the former from evidence of neither. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-005.1:** When the target task is accessible + and has no comments, the endpoint shall succeed and return an empty comment + list, never a null list, with a total of zero. +- **AC-OFFICE-AGENT-COMMENT-READS-005.2:** When a backing dependency is + unconfigured or a storage read fails, the endpoint shall return an error + status and shall not return an empty comment list. +- **AC-OFFICE-AGENT-COMMENT-READS-005.3:** The denied outcome, the dependency + error, and the empty-but-successful result shall be mutually distinguishable + from the response status and body alone. +- **AC-OFFICE-AGENT-COMMENT-READS-005.4:** The target task shall be taken from + the route's path parameter alone. No request field shall be able to redirect + the read to another task, and no caller-supplied value shall be able to + substitute the caller task for the requested one. +- **AC-OFFICE-AGENT-COMMENT-READS-005.5:** The caller task shall be taken from + the agent caller's validated JWT claims alone, so a caller cannot claim to be + a task it is not. +- **AC-OFFICE-AGENT-COMMENT-READS-005.6:** When the target task is archived, + its comments shall remain readable on the same terms as an unarchived task, + so a coordinator can read a child that was archived on completion. + +### REQ-OFFICE-AGENT-COMMENT-READS-006: Concurrent reads and writes + +**Intent:** Comments arrive while a coordinator reads them. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-006.1:** When a comment is written to the + target task while a read is in flight, that comment shall either appear + whole in the response or be absent from it, and shall never appear partially. +- **AC-OFFICE-AGENT-COMMENT-READS-006.2:** Two concurrent reads of one target + with identical arguments shall each return a self-consistent window + satisfying AC-OFFICE-AGENT-COMMENT-READS-003.7. +- **AC-OFFICE-AGENT-COMMENT-READS-006.3:** The read shall take no lock that + blocks a concurrent comment write. + +### REQ-OFFICE-AGENT-COMMENT-READS-007: The surface is discoverable and advertised + +**Intent:** A capability an agent is never told about is not a fix. The reported +defect is partly an instruction defect: the injected Office comment reference +documents only the write. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-007.1:** The Office first-turn context shall + advertise no comment read tool, since none is registered. +- **AC-OFFICE-AGENT-COMMENT-READS-007.2:** The set of tool names advertised in + the Office first-turn context shall remain exactly equal to the set of tools + registered for the Office MCP surface. +- **AC-OFFICE-AGENT-COMMENT-READS-007.3:** The injected Office comment + reference shall document reading a related task's comments with the CLI + command, its `--task` and `--limit` flags, and the default and maximum limit, + alongside the existing write command. +- **AC-OFFICE-AGENT-COMMENT-READS-007.4:** The Coordinator role instructions, + at `apps/backend/internal/office/configloader/instructions/ceo/AGENTS.md`, + shall direct the agent to read a delegated child task's comments with that CLI + command before concluding that the child produced no output. No other role + instruction file is required to change. + +### REQ-OFFICE-AGENT-COMMENT-READS-008: Acceptance path + +**Intent:** State the end-to-end outcome as one observable behavior. + +#### Acceptance criteria + +- **AC-OFFICE-AGENT-COMMENT-READS-008.1:** When a child Office task's agent + posts its stage deliverable as a task comment and its parent task's agent + subsequently runs, the parent shall be able to obtain that comment body by + running `agentctl kandev comment list` against the child task, with no human + action between the two runs. + +## Out of scope + +- **Comment writes.** Agents keep writing through the signed Office runtime + path; this document changes no write. +- **A monotonic comment sequence column.** Ordering is deterministic but its + tiebreak is stable-arbitrary, not insertion order: `id` is random, so two + comments sharing a `created_at` are ordered repeatably and not necessarily as + written. Adding a sequence column is a schema change no consumer needs today. +- **Cursor pagination.** `has_more` reports that older comments exist and + offers no cursor to reach them. History beyond the clamped maximum or the + response budget is out of scope. +- **The unguarded task document endpoints.** The dashboard document routes apply + no relation check either. That gap pre-exists, is the same shape as the one + closed here, and is neither widened nor fixed here. +- **Comment authorship correctness.** Reviewer runs execute inside the author's + session, so `author_id` is unreliable. This document returns the recorded + value and does not repair attribution. +- **Human-facing comment UI.** No dashboard, board, or task-detail change. + Preserving that surface's behaviour is a requirement here, not a change to it. +- **Notification or wake behavior.** Reading raises no wake and changes no run + scheduling; the wake payload's five-comment window is unchanged. diff --git a/docs/specs/office/system-design/agent-comment-reads-01.md b/docs/specs/office/system-design/agent-comment-reads-01.md new file mode 100644 index 0000000000..aac83a3f01 --- /dev/null +++ b/docs/specs/office/system-design/agent-comment-reads-01.md @@ -0,0 +1,387 @@ +--- +status: draft +system: office +requirements: + - REQ-OFFICE-AGENT-COMMENT-READS-001 + - REQ-OFFICE-AGENT-COMMENT-READS-002 + - REQ-OFFICE-AGENT-COMMENT-READS-003 + - REQ-OFFICE-AGENT-COMMENT-READS-004 + - REQ-OFFICE-AGENT-COMMENT-READS-005 + - REQ-OFFICE-AGENT-COMMENT-READS-006 + - REQ-OFFICE-AGENT-COMMENT-READS-007 + - REQ-OFFICE-AGENT-COMMENT-READS-008 +--- + +# Office: Agent Comment Reads System Design + +## Purpose and boundaries + +Office owns the agent-facing office HTTP surface, which tools an Office agent +may call, and what an Office agent is told it can do. This design repairs one +existing endpoint on that surface, `GET /api/v1/office/tasks/{id}/comments`, +and removes a comment read tool from the MCP surface rather than adding one. + +It uses, and does not own: + +- the `task_comments` store and its repository, owned by Office persistence; +- the cross-task read relation, owned by the task system's handoff access + guard and shared verbatim with the task document tools; +- the byte budget, ordering, and window projection, owned by the task system's + handoff comment service, which this design re-points at rather than rewrites; +- the agent JWT middleware that authenticates the office route group. + +Two deliberate exceptions to those boundaries are claimed as in scope. + +**The shared guard's not-found handling.** This design covers a change to +`loadAccessPair` that has already landed on the branch (commit `260405a9d`); +it is recorded here because it belongs to this contract, not because it is +outstanding. The guard as originally written did not deny a missing task; it +returned the repository's not-found error, and that error embedded the task +identifier. +Fixing that only at this endpoint would leave the comment surface and the +document surface answering the does-this-task-exist question differently, which +`REQ-OFFICE-AGENT-COMMENT-READS-001` forbids in terms: the relation must be +evaluated by the same shared implementation, so the two surfaces cannot +diverge. The blast radius is bounded. `loadAccessPair` has exactly two callers, +both inside the guard file, and no existing test asserts the leaking behaviour. +The change is security-positive for the document tools as well. + +**An exported agent-claims accessor.** The office agents package exposes +`CallerFromContext`, which returns the authenticated agent instance and carries +no task identity. The caller task lives in the validated JWT's `AgentClaims`, +set on the request context under an unexported key with no exported reader. +This design adds one exported accessor beside `CallerFromContext`, on the same +justification the existing one records: so other office packages can enforce +agent-scoped authorization without depending on the package's internal +context-key constants. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-OFFICE-AGENT-COMMENT-READS-001` | [Purpose and boundaries](#purpose-and-boundaries), [Components and responsibilities](#components-and-responsibilities), [Failure and recovery](#failure-and-recovery), [Security](#security) | +| `REQ-OFFICE-AGENT-COMMENT-READS-002` | [Data and contracts](#data-and-contracts) | +| `REQ-OFFICE-AGENT-COMMENT-READS-003` | [Data and contracts](#data-and-contracts), [Ordering and windowing](#ordering-and-windowing) | +| `REQ-OFFICE-AGENT-COMMENT-READS-004` | [Data and contracts](#data-and-contracts), [Ordering and windowing](#ordering-and-windowing) | +| `REQ-OFFICE-AGENT-COMMENT-READS-005` | [Data and contracts](#data-and-contracts), [Failure and recovery](#failure-and-recovery) | +| `REQ-OFFICE-AGENT-COMMENT-READS-006` | [Persistence](#persistence) | +| `REQ-OFFICE-AGENT-COMMENT-READS-007` | [The advertised-surface contract](#the-advertised-surface-contract) | +| `REQ-OFFICE-AGENT-COMMENT-READS-008` | [Control flow](#control-flow) | + +## Components and responsibilities + +- **Comment read endpoint.** The existing dashboard handler for + `GET /api/v1/office/tasks/:id/comments`. It gains a caller branch: an agent + caller takes the guarded, bounded path; every other caller keeps today's + behaviour byte for byte. +- **Agent-claims accessor.** The new exported reader described above. It is the + only source of the caller task identity. +- **Access-checked read.** `ListCommentsForCaller` on the cross-task handoff + service: applies the shared read guard, then the window, budget, and + projection. It already exists, and the guard, window, budget, and projection + are unchanged by the transport move. One thing about it does change, and it + is not optional. Its first act is a `self`/empty-target sentinel that + substitutes the caller's own task for the requested one. Under the deleted + MCP tool that sentinel was an argument convenience; on this transport the + target is a path segment the CLI interpolates without validation, so + `--task self` arrives as a literal target and substitutes. That is what + `AC-OFFICE-AGENT-COMMENT-READS-005.4` forbids in terms, and under a token + carrying no task claim it yields a missing-target validation error where + `AC-OFFICE-AGENT-COMMENT-READS-001.13` requires the denial sentinel for + every target. The sentinel is therefore removed: an unrecognised target is + resolved by the guard like any other, and only a target the caller is + entitled to name is read. Blast radius is nil, because once the tool is + deleted this endpoint is the function's only caller. Two tests assert the + removed behaviour and are retired with it, + `TestListCommentsForCallerSelfFallback` and + `TestListCommentsForCallerRequiresTaskIDWhenNoCaller` — the second cites an + acceptance criterion this revision deleted. Build replaces them with + coverage of the contract above; it does not delete them silently, and it + does not weaken them to pass. This is the only place the guard is called for + comments. +- **Comment store.** Supplies the windowed read and the total count. +- **agentctl CLI.** `kandev comment list --task [--limit N]` and + `kandev tasks conversation --id ` both already call this endpoint and + print the response body verbatim. Neither needs a wire change. The + `--limit` flag help, which reads `0 = all`, is corrected: for an agent caller + the server defaults and clamps, so `all` is no longer reachable and the flag + is documented as "0 = server default". +- **Office first-turn context, the skills reference, and role instructions.** + Advertise the CLI command and direct the coordinator to use it. +- **Deleted.** The Office MCP comment tool, its registration, its backend + action handler, and the sysprompt line advertising it. The guarded read must + have exactly one entry point; leaving the tool beside the repaired endpoint + would be two. + +## Data and contracts + +The endpoint takes the target task from its path parameter and, for an agent +caller, the caller task from JWT claims. Its only query parameter is `limit`. + +### The dual-caller split + +This endpoint is consumed by the human dashboard SPA as well as by agents. +Applying the guard and the default limit unconditionally would break the human +Office comment view: it would lose the per-comment run lifecycle fields it +renders the Queued / Working / Failed badge from, and would silently show a +browser user the newest twenty comments of a longer thread. The split is by +caller kind, and the precedent is in the same file: `createComment` already +rejects an agent caller by testing `CallerFromContext(c) != nil`, so agent-kind +branching on this route is established, not invented here. + +An agent caller therefore receives the guard, the default and clamp, the byte +budget, the window fields, and the reduced projection. Any other caller +receives exactly what it receives today. The existing Playwright coverage under +`apps/web/e2e/tests/office/` and the SPA's `listComments` client are the +regression surface for that second half. + +Which branch each acceptance criterion binds follows from that split, and three +of them must be read with it in hand because they carry no caller qualifier. +`AC-OFFICE-AGENT-COMMENT-READS-003.7` and `-003.8` — the single read +transaction and the repeat-call ordering determinism — and +`AC-OFFICE-AGENT-COMMENT-READS-005.1`, the empty-result total, bind the agent +branch only. None can bind the browser branch without contradicting +`AC-OFFICE-AGENT-COMMENT-READS-003.11`, which preserves it: the legacy list +query sorts on `created_at` with no tiebreak, so meeting the ordering criterion +there would mean changing that query, and the browser response carries no +`total` field at all, so meeting the transaction and empty-result criteria +there would mean adding one. Each of those is precisely the change `-003.11` +exists to prevent. `REQ-OFFICE-AGENT-COMMENT-READS-003` is framed around a +coordinator re-reading a child's thread, which is the agent branch; the browser +branch is preserved by this document, not specified by it. + +The browser branch reads no query parameter at all. +`AC-OFFICE-AGENT-COMMENT-READS-003.11` says a valid positive `limit` *may* be +honoured there; this design resolves that permission to no. The handler as it +stands passes only the path parameter to the list call and never touches the +query string, so honouring `limit` would be new behaviour rather than preserved +behaviour, and it would leave the human Office comment view one query parameter +away from a silently truncated thread. A permission that no builder is required +to exercise is one no test can observe either way, so it is settled here in the +negative: the browser branch ignores `limit` entirely and returns the full +list, which is what the same criterion's own first clause already requires. + +### Where the limit decision is made + +A query string carries no types, so `limit` arrives as a string or not at all +and there is no schema layer that can reject it before the handler. Parsing is +therefore the handler's, and the contract that `limit` never produces an error +from any layer falls out of the transport rather than having to be defended +against one, which is the main simplification the CLI transport buys. + +Absent, empty, unparseable, zero, or negative takes the default of 20; above +100 clamps to 100. A fractional `7.5` does not parse as an integer, so it takes +the default rather than truncating to 7. Nothing here is an error, and nothing +here is invisible: `total` and `has_more` report every omission. The clamping +itself is not re-implemented at the handler — the handoff service already +clamps at its own boundary, so the handler passes the parsed value through and +the bound holds even if a future caller reaches the service directly. + +### The agent projection + +Each comment returned to an agent projects exactly: identifier, task +identifier, `author_type`, `author_id`, `source`, body, creation timestamp, +and — only when the body was cut — a truncation marker plus the original body +length in bytes. `reply_channel_id` and the per-comment run lifecycle fields +are not projected; the first is internal reply routing and the second is a +rendering concern for the human comment list. + +Bodies are cut with the existing shared rune-safe byte truncation helper rather +than a new cut, so the boundary rule lives in one place. The per-body cap is +8192 bytes. + +Every agent response carries the window: `total`, `returned`, and `has_more`, +which is true exactly when `returned` is below `total`. The window describes +only omitted comments. Whether an individual body was shortened is carried on +that comment as `body_truncated`, never in the window. Two names because they +are two facts, and one name for both is how a coordinator concludes it has the +whole note when it has the first half of it. + +Author fields are projected as data. No filter parameter exists for them. Two +reasons: a coordinator needs the human steer on a child task as much as the +agent note, and comment authorship is currently unreliable because reviewer +runs execute inside the author's session, so a filter keyed on authorship would +silently drop rows it should return. + +## Ordering and windowing + +The comment store's existing ascending list and descending recent-list both +sort on `created_at` with no tiebreak, and the comment DTO already records that +Office writes two timestamps inside the same second. The window here is +therefore explicitly two-phase and uses a named tiebreak in both directions: + +1. select the newest `limit` rows ordered by `created_at` descending, `id` + descending; +2. present that window ordered by `created_at` ascending, `id` ascending. + +Using `id` in both directions is what makes the window boundary deterministic: +with a tiebreak in only one phase, two comments sharing a timestamp could +straddle the boundary differently on successive calls. `id` is a random +identifier, so this ordering is repeatable but is not insertion order; the +requirement and the exclusions both say so. + +A second bound sits above `limit`: the response carries at most 65536 bytes of +body in total. At the clamped maximum the per-body cap alone would permit a +response near 800KB, which no consuming agent can use, and the point of this +read is that the result is usable. When the selected window exceeds the budget, +whole comments are dropped from the oldest end, preserving the same +newest-wins rule the window selection already uses; the dropped comments leave +`total` untouched, so `has_more` reports them. + +The budget never empties a non-empty window. At the stated constants this holds +by construction rather than by a special case: the per-body cap is 8192 bytes +and the budget is 65536, so the newest comment's shortened body always fits +alone and the drop loop can never reach it. The invariant is written down +anyway, because it is the load-bearing one — an empty list is read by a +coordinator as "the child produced nothing" and ends the delegation — and it is +stated independently of the constants, so that if either ever changes such that +one shortened body could exceed the budget, that comment is still returned +alone. + +## Control flow + +Caller agent → `agentctl kandev comment list` → office HTTP route group → agent +auth middleware → comment read endpoint → access-checked read → comment store, +and the projection back out as the response body the CLI prints. The caller +task identity crosses from validated JWT claims; the target identifier crosses +from the request path. Neither is ever read from a flag the model chose beyond +the target it is entitled to name. + +The wiring is a pass-through, not a new assembly: `registerSecondaryRoutes` +already holds the handoff service and already registers the office route group +a few lines later, so the dashboard handler receives it the way the document +handler receives its own service. Office already imports the task service in +this package, so the architecture lint that bans the reverse direction is not +engaged. + +The acceptance path: a child agent posts its deliverable through the existing +signed runtime comment path; later, the parent's run runs +`agentctl kandev comment list --task `, the descendant branch of the +read guard admits it, and the body is printed. Nothing between the two runs is +human. + +## Failure and recovery + +Three outcomes must stay distinguishable, because conflating them is the +reported defect. + +- **Accessible and empty** is a success with an empty list, an explicit empty + array rather than a null, and a zero total. +- **Not permitted** is a forbidden status carrying the existing shared + access-denied sentinel, whose message is the literal string `document access + denied`. A missing task and an unrelated task must produce that same forbidden + outcome and that same message, because distinguishing them would let a caller + enumerate task identifiers. An agent whose JWT carries no task — routine + dispatch mints exactly such a token — is denied on this same path for every + target, rather than being allowed to fall through to the unguarded read. + + That sameness was not what the guard did, and closing the gap is the one + shared change this design owns; as noted above it has already landed. + `loadAccessPair` called the task repository for the caller and the target and + returned the repository's error unchanged. On an absent identifier that + repository returns a not-found error which embeds the identifier it was + given. The guard's `current == nil || target == nil` denial + branch below it is reachable only under test, because the test double returns + a bare `(nil, nil)` where the repository returns `(nil, not-found)`, and the + guard's missing-task test discards the error, so the divergence is invisible. + Normalisation therefore happens inside `loadAccessPair`: a not-found from + either lookup becomes a plain deny, with no error, before any caller sees it. + The test double is corrected in the same change to return the repository's + shape, so the guard's tests stop passing on a branch production never reaches. + + Renaming the `document access denied` sentinel text to something + surface-neutral is deliberately out of scope. That both surfaces read + identically is what matters; the wording does not, and changing it would churn + existing assertions for nothing. +- **Dependency unconfigured or storage read failed** is an internal error. This + path must not degrade to an empty list. The adjacent per-comment run-status + lookup in the same handler does degrade to an empty map, which is right there + and wrong here: an empty comment list is read by a coordinator as "the child + produced nothing" and ends the delegation. That existing degradation stays on + the browser branch, where the cost of a missing badge is a missing badge. + +## Persistence + +The read path is read-only and needs no schema change or new index. The +existing `(task_id, created_at)` index serves the window. + +Legacy rows are a known edge, and this design excludes them deliberately rather +than repairing them in passing. `CreateTaskComment` normalises a non-zero +`created_at` to UTC on write, so every row written after that fix compares +correctly under the lexical ordering the window relies on; rows written before +it may carry another offset and do not. Backfilling those is owned by the +follow-up card *Backfill legacy non-UTC `task_comments.created_at`* +(`f5984c8c-ecdb-4e58-b326-1c742c29bfbc`) and is not a precondition of this +work. Until it lands, a row whose stored timestamp is not in the canonical UTC +representation falls outside the ordering guarantee of +`REQ-OFFICE-AGENT-COMMENT-READS-003`. It is still returned, still counted in +`total`, and still subject to the guard and the budget — nothing is hidden and +nothing errors; only its position relative to a canonical row is unspecified. +No startup backfill is described here on purpose: a partial one, whose +behaviour on a value it cannot parse would itself need stating, would reopen +the very ordering question it appears to close. + +Both the count and the page are taken inside one read transaction on the +read-only connection, so the returned count never exceeds the reported total. A +comment inserted concurrently either precedes that snapshot or follows it. No +lock is taken that a writer could block on. + +## Security + +The read relation is not re-derived here. The endpoint calls the same guard the +document reads call, so the two cannot drift into two answers to one question. +The relation admits self, ancestors, descendants, siblings sharing a non-empty +parent, and blockers, and denies any cross-workspace pair. + +Placing the guard at the endpoint rather than at a tool is what makes it +complete. The office route group authenticates agents with a workspace-scoped +JWT. The workspace-scope middleware defers this exact agent comment route to +the shared guard because the route has no `:wsId` parameter and must return one +forbidden response for missing, foreign, and unrelated targets. Other Office +routes still use the workspace-scope middleware before dispatch. Before this +change any Office agent could read any task's comments in its workspace, and +both CLI commands reached that read. One handler fix closes both entry points. +A guarded tool beside an unguarded endpoint would have closed neither, which is +why the tool is deleted rather than kept. + +Task-identifier enumeration is closed at that guard rather than at this +endpoint, by the `loadAccessPair` normalisation described in +[Failure and recovery](#failure-and-recovery). Before that change an absent +identifier produced an error naming it; after it, a missing task is +indistinguishable from an existing unrelated one on every surface using the +guard. + +The caller identity comes from validated JWT claims, so a caller cannot claim +to be a task it is not, and a token with no task claim reads nothing. + +Recorded so it is not mistaken for a new hole: the dashboard task **document** +routes apply no relation check either. That gap predates this work, is the same +shape as the one closed here, and is neither widened nor fixed here. + +## The advertised-surface contract + +The Office first-turn context is asserted to advertise exactly the tool set the +Office surface registers, so removing the tool and leaving its sysprompt line +is a build failure rather than silent drift; the assertion is what keeps the +deletion honest in both directions. + +The capability is advertised where Office agents actually read: the injected +comment reference under the task-ops skill currently documents only the write +command and gains the read, and the coordinator role instructions — the `ceo` +role, at +`apps/backend/internal/office/configloader/instructions/ceo/AGENTS.md`, the role +that creates subtasks and reviews their results — gain the direction to read a +delegated child's comments with that command before concluding the child +produced nothing. Without that, the endpoint works and the coordinator still +does not call it. + +## Observability + +Requests are visible in the office route group's existing HTTP logging, so a +coordinator's read attempts are traceable alongside its other runtime calls. No +new metric: the failure this addresses is a missing capability and an unguarded +read, not a rate to watch. + +## Related decisions + +- [ADR 0015 — explicit completion signal for auto-advance](../../../decisions/0015-explicit-completion-signal-for-auto-advance.md)