diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 4784d73..d23ebd1 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -533,7 +533,11 @@ paths: type: string description: Where it goes when the text names no project. responses: - "201": { $ref: "#/components/responses/Task" } + "201": + description: Created + content: + application/json: + schema: { $ref: "#/components/schemas/QuickAddResult" } "422": { $ref: "#/components/responses/Validation" } /api/v1/tasks/quick-add/preview: @@ -2135,7 +2139,12 @@ components: TaskInput: type: object properties: - project_id: { type: string, description: "Omitted on create, the task goes to the Inbox." } + project_id: + type: string + description: | + Omitted on create, the task goes to the Inbox. On update it moves the + task — which needs editor rights on the destination. + section_id: { type: string } parent_id: { type: string } content: { type: string } @@ -2151,6 +2160,19 @@ components: items: { type: string } sort_order: { type: number } + QuickAddResult: + allOf: + - $ref: "#/components/schemas/Task" + - type: object + properties: + unknown_project: + type: string + description: | + Present when the text named a `#project` that does not exist. The + task is still created, in the Inbox — a thought is not thrown away + over a typo — but the caller is told, because the alternative is + that the name vanishes from the title and nothing says why. + ParsedTask: type: object description: What the quick-add parser made of a line. diff --git a/internal/httpapi/integrations_test.go b/internal/httpapi/integrations_test.go index 6547790..24f2209 100644 --- a/internal/httpapi/integrations_test.go +++ b/internal/httpapi/integrations_test.go @@ -983,3 +983,173 @@ func TestWellKnownCalDAVStillRedirects(t *testing.T) { t.Error("CalDAV discovery was caught by the well-known rule") } } + +// --- what the API accepts, it must actually do ------------------------------------- + +// project_id on an update used to be validated and then dropped: the destination +// was permission-checked, which reads as "understood", and the task never moved +// while the answer said OK. Both the REST path and the MCP tool did it. +func TestUpdateMovesTheTaskToAnotherProject(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]string{"name": "Firma"}) + projectID := project["id"].(string) + + _, task := ts.do(t, "POST", "/api/v1/tasks", map[string]any{"content": "betal moms"}) + taskID := task["id"].(string) + if task["project_id"] == projectID { + t.Fatal("the task started in the destination; the test proves nothing") + } + + resp, updated := ts.do(t, "PATCH", "/api/v1/tasks/"+taskID, + map[string]any{"project_id": projectID}) + if resp.StatusCode != http.StatusOK { + t.Fatalf("patch: status %d, body %v", resp.StatusCode, updated) + } + if updated["project_id"] != projectID { + t.Errorf("project_id = %v, want %v — the move was accepted and not made", + updated["project_id"], projectID) + } + + // And it really is there, not merely reported as there. + _, reread := ts.do(t, "GET", "/api/v1/tasks/"+taskID, nil) + if reread["project_id"] != projectID { + t.Errorf("after re-reading, project_id = %v", reread["project_id"]) + } +} + +func TestMCPUpdateMovesTheTaskToAnotherProject(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]string{"name": "Firma"}) + projectID := project["id"].(string) + + created := ts.callTool(t, "create_task", map[string]any{"text": "betal moms"}) + taskID := created["id"].(string) + + moved := ts.callTool(t, "update_task", map[string]any{ + "task_id": taskID, "project_id": projectID, + }) + if moved["project_id"] != projectID { + t.Errorf("project_id = %v, want %v", moved["project_id"], projectID) + } +} + +// A project you cannot write to is refused rather than quietly ignored. +func TestUpdateRefusesAProjectYouCannotWriteTo(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + other := ts.newUser(t, "anden@example.dk", "Anden") + _, theirs := other.do(t, "POST", "/api/v1/projects", map[string]string{"name": "Deres"}) + + _, task := ts.do(t, "POST", "/api/v1/tasks", map[string]any{"content": "min opgave"}) + taskID := task["id"].(string) + mine := task["project_id"] + + resp, _ := ts.do(t, "PATCH", "/api/v1/tasks/"+taskID, + map[string]any{"project_id": theirs["id"]}) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("status %d, want 404", resp.StatusCode) + } + + _, reread := ts.do(t, "GET", "/api/v1/tasks/"+taskID, nil) + if reread["project_id"] != mine { + t.Error("the task moved into somebody else's project") + } +} + +// An unknown #project is still not an error — the thought is kept rather than +// thrown away over a typo — but it is no longer silent. It used to vanish from +// the title while the task went to the Inbox, with nothing saying why. +func TestQuickAddSaysWhenTheProjectDoesNotExist(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + resp, body := ts.do(t, "POST", "/api/v1/tasks/quick-add", + map[string]any{"text": "Lav plakat #3Dekoration"}) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status %d, want 201 — the task should still be created", resp.StatusCode) + } + if body["unknown_project"] != "3Dekoration" { + t.Errorf("unknown_project = %v, want %q", body["unknown_project"], "3Dekoration") + } + if body["content"] != "Lav plakat" { + t.Errorf("content = %v", body["content"]) + } +} + +func TestQuickAddSaysNothingWhenTheProjectExists(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + ts.do(t, "POST", "/api/v1/projects", map[string]string{"name": "Dekoration"}) + + _, body := ts.do(t, "POST", "/api/v1/tasks/quick-add", + map[string]any{"text": "Lav plakat #Dekoration"}) + if _, present := body["unknown_project"]; present { + t.Errorf("reported an unknown project for one that exists: %v", body) + } +} + +func TestMCPCreateSaysWhenTheProjectDoesNotExist(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + created := ts.callTool(t, "create_task", map[string]any{"text": "Lav plakat #3Dekoration"}) + note, _ := created["note"].(string) + if !strings.Contains(note, "3Dekoration") { + t.Errorf("note = %q, want it to name the project that does not exist", note) + } +} + +// Sub-tasks were unreachable over MCP: create_task had no parent_id, so a parent +// with seven children arrived as eight unrelated tasks. +func TestMCPCreateMakesSubtasks(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]string{"name": "Firma"}) + parent := ts.callTool(t, "create_task", map[string]any{ + "content": "Runen v2", "project_id": project["id"], + }) + parentID := parent["id"].(string) + + child := ts.callTool(t, "create_task", map[string]any{ + "content": "Tegn ikonet", "parent_id": parentID, + }) + if child["parent_id"] != parentID { + t.Errorf("parent_id = %v, want %v", child["parent_id"], parentID) + } + // A sub-task joins its parent's project whatever else was asked for. + if child["project_id"] != project["id"] { + t.Errorf("project_id = %v, want the parent's %v", child["project_id"], project["id"]) + } + + _, listed := ts.do(t, "GET", "/api/v1/tasks?parent_id="+parentID, nil) + if tasks, _ := listed["tasks"].([]any); len(tasks) != 1 { + t.Errorf("the parent has %d sub-tasks, want 1", len(tasks)) + } +} + +func TestMCPCreateRefusesASecondLevelOfSubtask(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + parent := ts.callTool(t, "create_task", map[string]any{"content": "Runen v2"}) + child := ts.callTool(t, "create_task", map[string]any{ + "content": "Tegn ikonet", "parent_id": parent["id"], + }) + + // A tool that refuses reports it inside the result with isError, not as a + // JSON-RPC error — the call itself was well formed, the arguments were not. + response := ts.rpc(t, "tools/call", map[string]any{ + "name": "create_task", + "arguments": map[string]any{"content": "Endnu dybere", "parent_id": child["id"]}, + }) + result, _ := response["result"].(map[string]any) + if result == nil || result["isError"] != true { + t.Errorf("a sub-task of a sub-task was accepted: %v", response) + } +} diff --git a/internal/httpapi/mcp_handlers.go b/internal/httpapi/mcp_handlers.go index 846aeae..acc00c6 100644 --- a/internal/httpapi/mcp_handlers.go +++ b/internal/httpapi/mcp_handlers.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "strings" @@ -57,7 +58,8 @@ func (s *Server) buildMCP() *mcp.Server { InputSchema: mcp.Schema(map[string]any{ "text": mcp.Str("The task as a sentence, parsed for date, time, priority, #project, @label and recurrence."), "content": mcp.Str("The task title, if not using `text`."), - "project_id": mcp.Str("Which project. Defaults to the Inbox."), + "project_id": mcp.Str("Which project. Defaults to the Inbox. Naming a project that does not exist is not an error — the task goes to the Inbox and the answer says so."), + "parent_id": mcp.Str("Make this a sub-task of that task. It joins the parent's project. One level deep."), "due_date": mcp.Str("A date as YYYY-MM-DD."), "priority": mcp.Int("1 (highest) to 4 (none)."), "description": mcp.Str("Longer notes on the task."), @@ -253,6 +255,7 @@ func (s *Server) mcpCreateTask(ctx context.Context, userID string, args json.Raw Text string `json:"text"` Content string `json:"content"` ProjectID string `json:"project_id"` + ParentID string `json:"parent_id"` DueDate string `json:"due_date"` Priority int `json:"priority"` Description string `json:"description"` @@ -268,6 +271,11 @@ func (s *Server) mcpCreateTask(ctx context.Context, userID string, args json.Raw task := &store.Task{CreatedBy: userID, Priority: 4} labels := params.Labels + // Reported below rather than swallowed. "#Dekoration" used to vanish from the + // title while the task went to the Inbox, with nothing anywhere saying so — + // invisible in a chat, where nobody sees the row that was written. + unknownProject := "" + if text := strings.TrimSpace(params.Text); text != "" { parsed := quickadd.Parse(text, time.Now().In(userLocation(user.Timezone)), user.Locale) task.Content = parsed.Content @@ -280,6 +288,8 @@ func (s *Server) mcpCreateTask(ctx context.Context, userID string, args json.Raw if parsed.Project != "" && params.ProjectID == "" { if id, err := s.db.ProjectByName(ctx, userID, parsed.Project); err == nil { task.ProjectID = id + } else { + unknownProject = parsed.Project } } } @@ -305,6 +315,23 @@ func (s *Server) mcpCreateTask(ctx context.Context, userID string, args json.Raw if params.ProjectID != "" { task.ProjectID = params.ProjectID } + + // A sub-task belongs to its parent's project, whatever else was asked for: + // the two cannot disagree, and the parent is the more specific instruction. + // Without this the tools could not build a tree at all — an import of a + // parent with seven children arrived as eight unrelated tasks. + if params.ParentID != "" { + parent, err := s.db.GetTask(ctx, params.ParentID, userID) + if err != nil { + return nil, mcp.ArgError("no task with id %q to hang this under", params.ParentID) + } + if parent.ParentID != "" { + return nil, mcp.ArgError("sub-tasks are one level deep; %q is already a sub-task", params.ParentID) + } + task.ParentID = parent.ID + task.ProjectID = parent.ProjectID + } + if task.ProjectID == "" { if task.ProjectID, err = s.db.InboxID(ctx, userID); err != nil { return nil, err @@ -319,10 +346,23 @@ func (s *Server) mcpCreateTask(ctx context.Context, userID string, args json.Raw } s.hub.Publish(task.ProjectID, "task.created", toTaskJSON(*task)) - return map[string]any{ + + out := map[string]any{ "id": task.ID, "content": task.Content, "priority": task.Priority, "due_date": task.DueDate, "project_id": task.ProjectID, - }, nil + } + if task.ParentID != "" { + out["parent_id"] = task.ParentID + } + if unknownProject != "" { + // Phrased for a model to relay: it says what happened and what to do, + // so the person is told rather than left with a task in the wrong place. + out["note"] = fmt.Sprintf( + "There is no project called %q, so this went to the Inbox. "+ + "Create the project and move it, or say which project you meant.", + unknownProject) + } + return out, nil } func (s *Server) mcpUpdateTask(ctx context.Context, userID string, args json.RawMessage) (any, error) { @@ -364,8 +404,11 @@ func (s *Server) mcpUpdateTask(ctx context.Context, userID string, args json.Raw if params.Labels != nil { update.SetLabels, update.Labels = true, *params.Labels } + // Moved before the field update, and through the same helper the REST handler + // uses. This used to check the caller's rights on the destination and then + // drop the argument, so the task never moved and the answer still said OK. if params.ProjectID != nil { - if _, err := store.RequireProjectRole(ctx, s.db, *params.ProjectID, userID, store.RoleEditor); err != nil { + if err := s.moveTaskToProject(ctx, params.TaskID, *params.ProjectID, userID); err != nil { return nil, mcp.ArgError("no project with id %q that you can write to", *params.ProjectID) } } @@ -379,9 +422,13 @@ func (s *Server) mcpUpdateTask(ctx context.Context, userID string, args json.Raw } s.hub.Publish(task.ProjectID, "task.updated", toTaskJSON(*task)) + // project_id is in the answer because it is one of the things that can be + // changed here: a caller that asked for a move can see whether it happened + // rather than taking "OK" on trust. return map[string]any{ "id": task.ID, "content": task.Content, "priority": task.Priority, "due_date": task.DueDate, "labels": task.Labels, + "project_id": task.ProjectID, }, nil } diff --git a/internal/httpapi/tasks_handlers.go b/internal/httpapi/tasks_handlers.go index 274df71..c81ef90 100644 --- a/internal/httpapi/tasks_handlers.go +++ b/internal/httpapi/tasks_handlers.go @@ -1,6 +1,8 @@ package httpapi import ( + "context" + "errors" "net/http" "strconv" "strings" @@ -249,6 +251,16 @@ func (s *Server) handleUpdateTask(w http.ResponseWriter, r *http.Request) { return } + // A change of project is a move, not a field: it has to renumber the task + // among its new neighbours. Done before the rest so a request that both moves + // and edits ends up entirely applied or entirely refused. + if req.ProjectID != nil { + if err := s.moveTaskToProject(r.Context(), taskID, *req.ProjectID, user.ID); err != nil { + writeError(w, http.StatusNotFound, CodeNotFound, err.Error()) + return + } + } + u := store.TaskUpdate{ Description: req.Description, Priority: req.Priority, SectionID: req.SectionID, ParentID: req.ParentID, @@ -473,11 +485,18 @@ func (s *Server) handleQuickAdd(w http.ResponseWriter, r *http.Request) { } projectID := req.ProjectID + // Set when the text named a project that does not exist. The task is still + // created — see below — but the caller is told, because the alternative is + // what it used to be: "#Dekoration" disappears from the title, the task lands + // somewhere else, and nothing anywhere says why. + unknownProject := "" if parsed.Project != "" { // An unknown "#name" is not an error: the task still gets created, in the // Inbox, rather than a thought being thrown away over a typo. if id, err := s.db.ProjectByName(r.Context(), user.ID, parsed.Project); err == nil { projectID = id + } else { + unknownProject = parsed.Project } } if projectID == "" { @@ -510,6 +529,16 @@ func (s *Server) handleQuickAdd(w http.ResponseWriter, r *http.Request) { t.Labels = parsed.Labels s.activity(r, t.ProjectID, t.ID, "task.created", map[string]any{"content": t.Content}) s.publish(t.ProjectID, "task.created", toTaskJSON(*t)) + + if unknownProject != "" { + // Alongside the task rather than instead of it: the task was created, and + // this says what could not be honoured while creating it. + writeJSON(w, http.StatusCreated, struct { + taskJSON + UnknownProject string `json:"unknown_project"` + }{toTaskJSON(*t), unknownProject}) + return + } writeJSON(w, http.StatusCreated, toTaskJSON(*t)) } @@ -525,6 +554,42 @@ func (s *Server) handleQuickAddPreview(w http.ResponseWriter, r *http.Request) { // --- helpers -------------------------------------------------------------------- +// moveTaskToProject moves a task to another project, if that is really what was +// asked for. +// +// Shared by the REST handler and the MCP tool, because they had drifted: both +// accepted project_id on an update, both checked the caller's rights on the +// destination — which reads as "understood" — and then neither passed it on, +// because store.TaskUpdate has no such field. The task stayed where it was and +// the response said OK. An argument that is validated and then dropped is worse +// than one that is refused. +// +// A move rather than a field assignment: sort_order is per project, so landing +// in a new one means being given a position among its tasks. +func (s *Server) moveTaskToProject(ctx context.Context, taskID, projectID, userID string) error { + if projectID == "" { + return nil + } + current, err := s.db.GetTask(ctx, taskID, userID) + if err != nil { + return errors.New("not found") + } + if current.ProjectID == projectID { + return nil // already there; not worth a write or a renumber + } + if _, err := store.RequireProjectRole(ctx, s.db, projectID, userID, store.RoleEditor); err != nil { + return errors.New("not found") + } + + // No neighbours: it lands at the start of the destination, which is the only + // answer available when the caller did not say where. Section is cleared — + // a section belongs to the project being left behind. + if _, err := s.db.MoveTask(ctx, taskID, projectID, "", "", ""); err != nil { + return errors.New("not found") + } + return nil +} + func validateTaskContent(content string, priority *int) map[string]string { fields := map[string]string{} switch {