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 <noreply@anthropic.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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]
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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 <subtask-id>` 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.
|
||||
|
||||
|
||||
+8
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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", "")
|
||||
|
||||
|
||||
@@ -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}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 (
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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))
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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 == "" {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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 == "" {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
@@ -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 <id> [--limit N]` and
|
||||
`kandev tasks conversation --id <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 <child>`, 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)
|
||||
Reference in New Issue
Block a user