mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
feat(comments): agents can edit and delete comments from the CLI and every MCP transport (TASK-2695) (#1607)
Web users could edit and delete comments since PLAN-1662. Agents, who write
most comments, could only append corrections.
- Server: new item-scoped routes, PATCH/DELETE
/workspaces/{ws}/items/{ref}/comments/{id}. They answer 404 unless the
comment is on {ref}, then run the existing handlers unchanged (edit is
author-only, delete needs item-edit). GET /server/capabilities advertises
them as item_scoped_comment_writes.
- CLI: `pad item comment-edit <ref> <comment-id> [message|--stdin]` and
`pad item comment-delete <ref> <comment-id>`. Both refuse to send to a
server without the capability, whose bare 404 would otherwise read as
"comment not found". `pad item comments` now prints each comment's id,
plus "edited" once its body has changed.
- models.Comment serialises a derived `edited` bool, true when
updated_at > created_at (the web timeline's rule).
- MCP 0.57: pad_item edit-comment / delete-comment and a comment_id param,
on stdio, remote /mcp and WebMCP. All three use the item-scoped routes,
so the check that the comment is on ref happens on the server for every
transport. Remote packages a delete's 204 as the CLI's JSON.
Found along the way, and out of scope: deleting a comment that has replies
answers 500, because parent_id has no ON DELETE. Filed as BUG-3252.
Claude-Session: https://claude.ai/code/session_016PXRzXhokr61CLbbu3Txj9
This commit is contained in:
+104
-3
@@ -3077,6 +3077,98 @@ func commentsCmd() *cobra.Command {
|
||||
}
|
||||
}
|
||||
|
||||
// errItemScopedCommentWritesUnsupported is returned before sending when the
|
||||
// server does not advertise item_scoped_comment_writes (TASK-2695). An older
|
||||
// build has no such route, and its bare 404 would read as "comment not found".
|
||||
var errItemScopedCommentWritesUnsupported = errors.New("this server does not support editing or deleting comments from the CLI " +
|
||||
"(it does not advertise item_scoped_comment_writes); upgrade the server, or use the web UI")
|
||||
|
||||
func commentEditCmd() *cobra.Command {
|
||||
var useStdin bool
|
||||
|
||||
cmd := &cobra.Command{
|
||||
Use: "comment-edit <ref> <comment-id> [message]",
|
||||
Short: "Replace the body of a comment on an item",
|
||||
Long: `Replace the body of a comment on an item.
|
||||
|
||||
Only the comment's author may edit it; an admin may too, but only from a
|
||||
browser session. Get comment IDs from "pad item comments <ref>". An edited
|
||||
comment is shown with "edited" in that listing.
|
||||
|
||||
pad item comment-edit TASK-5 3f2a... "corrected threshold is 0.7"
|
||||
pad item comment-edit TASK-5 3f2a... --stdin < note.md`,
|
||||
Args: func(cmd *cobra.Command, args []string) error {
|
||||
if useStdin {
|
||||
return cobra.ExactArgs(2)(cmd, args)
|
||||
}
|
||||
return cobra.ExactArgs(3)(cmd, args)
|
||||
},
|
||||
RunE: func(cmd *cobra.Command, args []string) error {
|
||||
var body string
|
||||
if useStdin {
|
||||
b, err := readStdinBody("comment edit")
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
body = b
|
||||
} else {
|
||||
body = args[2]
|
||||
}
|
||||
|
||||
client, _ := getClient()
|
||||
ws := getWorkspace()
|
||||
if !client.ServerSupportsItemScopedCommentWrites() {
|
||||
return errItemScopedCommentWritesUnsupported
|
||||
}
|
||||
|
||||
comment, err := client.UpdateComment(ws, args[0], args[1], body)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if formatFlag == "json" {
|
||||
return cli.PrintJSON(comment)
|
||||
}
|
||||
|
||||
fmt.Printf("Comment %s on %s updated\n", args[1], args[0])
|
||||
return nil
|
||||
},
|
||||
}
|
||||
|
||||
cmd.Flags().BoolVar(&useStdin, "stdin", false, "read the new body from stdin instead of the message argument (a blank read is refused)")
|
||||
return cmd
|
||||
}
|
||||
|
||||
func commentDeleteCmd() *cobra.Command {
|
||||
return &cobra.Command{
|
||||
Use: "comment-delete <ref> <comment-id>",
|
||||
Short: "Delete a comment from an item",
|
||||
Long: `Delete a comment from an item.
|
||||
|
||||
Anyone who may edit the item may delete its comments. Get comment IDs from
|
||||
"pad item comments <ref>".`,
|
||||
Args: cobra.ExactArgs(2),
|
||||
RunE: func(cmd *cobra.Command, args []string) error {
|
||||
client, _ := getClient()
|
||||
ws := getWorkspace()
|
||||
if !client.ServerSupportsItemScopedCommentWrites() {
|
||||
return errItemScopedCommentWritesUnsupported
|
||||
}
|
||||
|
||||
if err := client.DeleteComment(ws, args[0], args[1]); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if formatFlag == "json" {
|
||||
return cli.PrintJSON(map[string]any{"deleted": true, "ref": args[0], "comment_id": args[1]})
|
||||
}
|
||||
|
||||
fmt.Printf("Comment %s deleted from %s\n", args[1], args[0])
|
||||
return nil
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
// renderCommentsMarkdown is the markdown counterpart of cli.PrintCommentTable.
|
||||
// Comments are not tabular — the terminal form prints an attribution line then
|
||||
// the body — so the markdown form keeps that shape rather than forcing a table.
|
||||
@@ -3095,10 +3187,16 @@ func renderCommentsMarkdown(w io.Writer, comments []models.Comment) {
|
||||
if c.Author != "" && c.Author != c.CreatedBy {
|
||||
badge = c.Author + " (" + c.CreatedBy + ")"
|
||||
}
|
||||
fmt.Fprintf(w, "**%s** · %s via %s\n\n",
|
||||
edited := ""
|
||||
if c.IsEdited() {
|
||||
edited = " · edited"
|
||||
}
|
||||
fmt.Fprintf(w, "**%s** · %s via %s%s · id `%s`\n\n",
|
||||
cli.SanitizeMarkdownText(badge),
|
||||
cli.SanitizeMarkdownText(cli.RelativeTime(c.CreatedAt)),
|
||||
cli.SanitizeMarkdownText(c.Source))
|
||||
cli.SanitizeMarkdownText(c.Source),
|
||||
edited,
|
||||
strings.ReplaceAll(c.ID, "`", ""))
|
||||
fmt.Fprintln(w, c.Body)
|
||||
if i < len(comments)-1 {
|
||||
fmt.Fprintln(w)
|
||||
@@ -4363,8 +4461,11 @@ func readStdinBody(verb string) (string, error) {
|
||||
}
|
||||
if strings.TrimSpace(string(data)) == "" {
|
||||
hint := "pass a body, or drop --stdin"
|
||||
if verb == "update" {
|
||||
switch verb {
|
||||
case "update":
|
||||
hint = "pass a body, or use --clear-content to empty the body on purpose"
|
||||
case "comment edit":
|
||||
hint = "pass a body, or use comment-delete to remove the comment"
|
||||
}
|
||||
return "", fmt.Errorf("--stdin: the body read from stdin is empty or whitespace-only, so the %s was refused and nothing was sent (a lost heredoc looks like this); %s", verb, hint)
|
||||
}
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"github.com/spf13/cobra"
|
||||
)
|
||||
|
||||
// TASK-2695: `pad item comment-edit` / `comment-delete` send only to a server
|
||||
// that advertises item_scoped_comment_writes. An older build has no such
|
||||
// route and would answer a bare 404, which reads as "comment not found".
|
||||
// Every leg counts the writes, since what the gate prevents is a write.
|
||||
|
||||
type commentStub struct {
|
||||
caps string // "new": advertises; "old": 404; "flaky": 500
|
||||
|
||||
mu sync.Mutex
|
||||
writes []string // "METHOD path"
|
||||
bodies []map[string]any
|
||||
}
|
||||
|
||||
func (s *commentStub) handler(w http.ResponseWriter, r *http.Request) {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
if strings.HasSuffix(r.URL.Path, "/server/capabilities") {
|
||||
switch s.caps {
|
||||
case "old":
|
||||
http.NotFound(w, r)
|
||||
case "flaky":
|
||||
w.WriteHeader(http.StatusInternalServerError)
|
||||
default:
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{"item_scoped_comment_writes": true})
|
||||
}
|
||||
return
|
||||
}
|
||||
s.writes = append(s.writes, r.Method+" "+r.URL.Path)
|
||||
if r.Method == http.MethodDelete {
|
||||
w.WriteHeader(http.StatusNoContent)
|
||||
return
|
||||
}
|
||||
var body map[string]any
|
||||
_ = json.NewDecoder(r.Body).Decode(&body)
|
||||
s.bodies = append(s.bodies, body)
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{"id": "c-1", "body": body["body"]})
|
||||
}
|
||||
|
||||
func runCommentCmd(t *testing.T, stub *commentStub, cmd *cobra.Command, args ...string) error {
|
||||
t.Helper()
|
||||
setupPushTest(t, http.HandlerFunc(stub.handler))
|
||||
cmd.SetArgs(args)
|
||||
cmd.SilenceUsage = true
|
||||
cmd.SilenceErrors = true
|
||||
var err error
|
||||
captureStdout(t, func() { err = cmd.Execute() })
|
||||
return err
|
||||
}
|
||||
|
||||
func TestCommentEditDelete_NewServer_ItemScopedRoute(t *testing.T) {
|
||||
stub := &commentStub{caps: "new"}
|
||||
if err := runCommentCmd(t, stub, commentEditCmd(), "TASK-5", "c-1", "fixed text"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := runCommentCmd(t, stub, commentDeleteCmd(), "TASK-5", "c-1"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(stub.writes) != 2 {
|
||||
t.Fatalf("writes = %v, want one PATCH and one DELETE", stub.writes)
|
||||
}
|
||||
for i, method := range []string{"PATCH", "DELETE"} {
|
||||
if !strings.HasPrefix(stub.writes[i], method+" ") || !strings.HasSuffix(stub.writes[i], "/items/TASK-5/comments/c-1") {
|
||||
t.Errorf("write %d = %q, want %s to the item-scoped route", i, stub.writes[i], method)
|
||||
}
|
||||
}
|
||||
if len(stub.bodies) != 1 || stub.bodies[0]["body"] != "fixed text" {
|
||||
t.Errorf("PATCH body = %v, want {body: fixed text}", stub.bodies)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCommentEditDelete_UnadvertisedServer_SendsNothing(t *testing.T) {
|
||||
for _, caps := range []string{"old", "flaky"} {
|
||||
stub := &commentStub{caps: caps}
|
||||
errEdit := runCommentCmd(t, stub, commentEditCmd(), "TASK-5", "c-1", "fixed text")
|
||||
errDelete := runCommentCmd(t, stub, commentDeleteCmd(), "TASK-5", "c-1")
|
||||
for name, err := range map[string]error{"edit": errEdit, "delete": errDelete} {
|
||||
if err == nil || !strings.Contains(err.Error(), "item_scoped_comment_writes") {
|
||||
t.Errorf("%s/%s: want the capability refusal, got %v", caps, name, err)
|
||||
}
|
||||
}
|
||||
if len(stub.writes) != 0 {
|
||||
t.Errorf("%s: sent %v to a server that did not advertise the routes", caps, stub.writes)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -124,6 +124,8 @@ func itemCmd() *cobra.Command {
|
||||
bulkUpdateCmd(),
|
||||
commentCmd(),
|
||||
commentsCmd(),
|
||||
commentEditCmd(),
|
||||
commentDeleteCmd(),
|
||||
remindCmd(),
|
||||
remindersCmd(),
|
||||
setAsideCmd(),
|
||||
|
||||
+30
-2
@@ -52,6 +52,7 @@ type serverCapabilityFlags struct {
|
||||
CollectionResolution bool `json:"collection_resolution"`
|
||||
ItemFieldAppend bool `json:"item_field_append"`
|
||||
SearchCollectionResolution bool `json:"search_collection_resolution"`
|
||||
ItemScopedCommentWrites bool `json:"item_scoped_comment_writes"`
|
||||
}
|
||||
|
||||
func NewClient(host string, port int) *Client {
|
||||
@@ -782,8 +783,35 @@ func (c *Client) CreateComment(wsSlug, itemSlug string, input models.CommentCrea
|
||||
return &result, wrapItemNotFound(err, itemSlug, wsSlug)
|
||||
}
|
||||
|
||||
func (c *Client) DeleteComment(wsSlug, commentID string) error {
|
||||
return c.delete("/workspaces/" + wsSlug + "/comments/" + commentID)
|
||||
// UpdateComment replaces a comment's body through the item-scoped route
|
||||
// (TASK-2695), which refuses a comment that is not on itemRef. The server's
|
||||
// not_found is passed through as-is: it already says whether the ITEM or the
|
||||
// COMMENT was missing, and wrapItemNotFound would relabel both as the item.
|
||||
func (c *Client) UpdateComment(wsSlug, itemRef, commentID, body string) (*models.Comment, error) {
|
||||
var result models.Comment
|
||||
err := c.patch(itemCommentPath(wsSlug, itemRef, commentID), map[string]string{"body": body}, &result)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return &result, nil
|
||||
}
|
||||
|
||||
// DeleteComment removes a comment through the item-scoped route (TASK-2695).
|
||||
func (c *Client) DeleteComment(wsSlug, itemRef, commentID string) error {
|
||||
return c.delete(itemCommentPath(wsSlug, itemRef, commentID))
|
||||
}
|
||||
|
||||
func itemCommentPath(wsSlug, itemRef, commentID string) string {
|
||||
return "/workspaces/" + wsSlug + "/items/" + url.PathEscape(itemRef) + "/comments/" + url.PathEscape(commentID)
|
||||
}
|
||||
|
||||
// ServerSupportsItemScopedCommentWrites reports whether this server routes
|
||||
// comment PATCH/DELETE under the item (TASK-2695). An indeterminate probe
|
||||
// answers false: sending anyway would turn an older server's unrouted 404
|
||||
// into a false "comment not found".
|
||||
func (c *Client) ServerSupportsItemScopedCommentWrites() bool {
|
||||
caps, definitive := c.serverCapabilities()
|
||||
return definitive && caps.ItemScopedCommentWrites
|
||||
}
|
||||
|
||||
// --- Dashboard ---
|
||||
|
||||
@@ -644,7 +644,11 @@ func PrintCommentTable(comments []models.Comment) {
|
||||
if c.Author != "" && c.Author != c.CreatedBy {
|
||||
badge = c.Author + " (" + c.CreatedBy + ")"
|
||||
}
|
||||
fmt.Printf("💬 %s • %s via %s\n", badge, RelativeTime(c.CreatedAt), c.Source)
|
||||
edited := ""
|
||||
if c.IsEdited() {
|
||||
edited = " • edited"
|
||||
}
|
||||
fmt.Printf("💬 %s • %s via %s%s • id %s\n", badge, RelativeTime(c.CreatedAt), c.Source, edited, c.ID)
|
||||
fmt.Println(c.Body)
|
||||
if i < len(comments)-1 {
|
||||
fmt.Println()
|
||||
|
||||
@@ -107,6 +107,10 @@ var padItemTool = ToolDef{
|
||||
// the `reply_to` parameter.
|
||||
"comment": passThrough([]string{"item", "comment"}),
|
||||
"list-comments": passThrough([]string{"item", "comments"}),
|
||||
// TASK-2695: edit/delete ride the item-scoped comment routes, which
|
||||
// refuse a comment_id that is not on `ref` on every transport.
|
||||
"edit-comment": passThrough([]string{"item", "comment-edit"}),
|
||||
"delete-comment": passThrough([]string{"item", "comment-delete"}),
|
||||
|
||||
// Backlinks ("Mentioned in") — PLAN-1593 / TASK-1596.
|
||||
// Returns inbound `[[...]]` references to the given item.
|
||||
@@ -156,7 +160,7 @@ var padItemTool = ToolDef{
|
||||
// keeping the schema simple to maintain.
|
||||
var padItemSchemaParams = []ParamDef{
|
||||
// ── Targeting ──
|
||||
{Name: "ref", Type: "string", Description: "Item reference (e.g. TASK-5, IDEA-12, PLAYB-3, CONVE-7). Required for: update, delete, restore, get, move, link, unlink, deps, star, unstar, comment, list-comments, note, decide, export, remind, claim, release. NOT used for ack-reminder (which addresses a REMINDER by `reminder_id`, since an item can carry several) and NOT used for bulk-update — pass `refs` (array) instead."},
|
||||
{Name: "ref", Type: "string", Description: "Item reference (e.g. TASK-5, IDEA-12, PLAYB-3, CONVE-7). Required for: update, delete, restore, get, move, link, unlink, deps, star, unstar, comment, list-comments, edit-comment, delete-comment, note, decide, export, remind, claim, release. NOT used for ack-reminder (which addresses a REMINDER by `reminder_id`, since an item can carry several) and NOT used for bulk-update — pass `refs` (array) instead."},
|
||||
{Name: "refs", Type: "array<string>", Description: "Item references for batch operations. Required for: bulk-update (one or more refs)."},
|
||||
{Name: "target", Type: "string", Description: "The OTHER end of a relationship. Required for: link, unlink (paired with `ref` and `link_type`). For link_type=blocks, target is the item being blocked; for blocked-by it's the blocker; for supersedes it's the superseded item; etc."},
|
||||
{Name: "link_type", Type: "string", Description: "Type of relationship for action=link/unlink.", Enum: []string{"blocks", "blocked-by", "supersedes", "implements", "split-from"}},
|
||||
@@ -270,7 +274,8 @@ var padItemSchemaParams = []ParamDef{
|
||||
{Name: "target_collection", Type: "string", Description: "Destination collection slug for action=move. Required for: move."},
|
||||
|
||||
// ── Comments ──
|
||||
{Name: "message", Type: "string", Description: "Comment body. Required for: comment."},
|
||||
{Name: "message", Type: "string", Description: "Comment body. Required for: comment, edit-comment."},
|
||||
{Name: "comment_id", Type: "string", Description: "ID of a comment on `ref` (from list-comments). Required for: edit-comment, delete-comment."},
|
||||
{Name: "reply_to", Type: "string", Description: "Parent comment ID for threading replies. Optional for: comment."},
|
||||
{Name: "comment", Type: "string", Description: "Audit comment explaining the change. Optional for: update."},
|
||||
|
||||
@@ -411,8 +416,13 @@ Actions:
|
||||
comment — Add a comment.
|
||||
Required: ref, message.
|
||||
Optional: reply_to (comment ID for threaded reply).
|
||||
list-comments — List comments on an item.
|
||||
list-comments — List comments on an item. Each carries its id and
|
||||
edited (true once its body has been changed).
|
||||
Required: ref.
|
||||
edit-comment — Replace a comment's body. Only its author may.
|
||||
Required: ref, comment_id, message.
|
||||
delete-comment — Delete a comment. Anyone who may edit the item may.
|
||||
Required: ref, comment_id.
|
||||
backlinks — List inbound [[...]] references to an item ("Mentioned in").
|
||||
Required: ref.
|
||||
Optional: limit (default 50, max 300), offset.
|
||||
|
||||
@@ -1710,7 +1710,8 @@ func TestFieldConflictProperty_SourcesDerivedFromTheDeclaredSchema(t *testing.T)
|
||||
"summary": true, "details": true, // action=note
|
||||
"decision": true, "rationale": true, // action=decide
|
||||
"message": true, "reply_to": true, // action=comment
|
||||
"comment": true, // the audit note on update
|
||||
"comment_id": true, // action=edit-comment / delete-comment (TASK-2695): addresses a comment row, never an item field
|
||||
"comment": true, // the audit note on update
|
||||
|
||||
// action=remind / ack-reminder (IDEA-2641). Neither writes an item
|
||||
// FIELD: a reminder is a row in its own table addressed by its own
|
||||
|
||||
@@ -115,28 +115,30 @@ func TestReadOnlyCatalog_ActionsMatchCmdhelp(t *testing.T) {
|
||||
// pad_item passThrough actions. link/unlink are excluded —
|
||||
// they fan out to per-link_type cmdPaths and are tested in
|
||||
// catalog_item_test.go instead.
|
||||
{"pad_item", "create"}: {"item", "create"},
|
||||
{"pad_item", "update"}: {"item", "update"},
|
||||
{"pad_item", "delete"}: {"item", "delete"},
|
||||
{"pad_item", "get"}: {"item", "show"},
|
||||
{"pad_item", "list"}: {"item", "list"},
|
||||
{"pad_item", "move"}: {"item", "move"},
|
||||
{"pad_item", "restore"}: {"item", "restore"},
|
||||
{"pad_item", "deps"}: {"item", "deps"},
|
||||
{"pad_item", "remind"}: {"item", "remind"},
|
||||
{"pad_item", "ack-reminder"}: {"item", "ack"},
|
||||
{"pad_item", "claim"}: {"item", "claim"},
|
||||
{"pad_item", "release"}: {"item", "release"},
|
||||
{"pad_item", "star"}: {"item", "star"},
|
||||
{"pad_item", "unstar"}: {"item", "unstar"},
|
||||
{"pad_item", "starred"}: {"item", "starred"},
|
||||
{"pad_item", "comment"}: {"item", "comment"},
|
||||
{"pad_item", "list-comments"}: {"item", "comments"},
|
||||
{"pad_item", "bulk-update"}: {"item", "bulk-update"},
|
||||
{"pad_item", "note"}: {"item", "note"},
|
||||
{"pad_item", "decide"}: {"item", "decide"},
|
||||
{"pad_item", "backlinks"}: {"item", "backlinks"},
|
||||
{"pad_item", "history"}: {"item", "history"},
|
||||
{"pad_item", "create"}: {"item", "create"},
|
||||
{"pad_item", "update"}: {"item", "update"},
|
||||
{"pad_item", "delete"}: {"item", "delete"},
|
||||
{"pad_item", "get"}: {"item", "show"},
|
||||
{"pad_item", "list"}: {"item", "list"},
|
||||
{"pad_item", "move"}: {"item", "move"},
|
||||
{"pad_item", "restore"}: {"item", "restore"},
|
||||
{"pad_item", "deps"}: {"item", "deps"},
|
||||
{"pad_item", "remind"}: {"item", "remind"},
|
||||
{"pad_item", "ack-reminder"}: {"item", "ack"},
|
||||
{"pad_item", "claim"}: {"item", "claim"},
|
||||
{"pad_item", "release"}: {"item", "release"},
|
||||
{"pad_item", "star"}: {"item", "star"},
|
||||
{"pad_item", "unstar"}: {"item", "unstar"},
|
||||
{"pad_item", "starred"}: {"item", "starred"},
|
||||
{"pad_item", "comment"}: {"item", "comment"},
|
||||
{"pad_item", "list-comments"}: {"item", "comments"},
|
||||
{"pad_item", "edit-comment"}: {"item", "comment-edit"},
|
||||
{"pad_item", "delete-comment"}: {"item", "comment-delete"},
|
||||
{"pad_item", "bulk-update"}: {"item", "bulk-update"},
|
||||
{"pad_item", "note"}: {"item", "note"},
|
||||
{"pad_item", "decide"}: {"item", "decide"},
|
||||
{"pad_item", "backlinks"}: {"item", "backlinks"},
|
||||
{"pad_item", "history"}: {"item", "history"},
|
||||
|
||||
// pad_playbook actions (PLAN-1377 / TASK-1381). All three
|
||||
// passThrough to `pad playbook <subcommand>`.
|
||||
@@ -275,27 +277,29 @@ func TestReadOnlyCatalog_ActionsDispatchExpectedCmdPath(t *testing.T) {
|
||||
|
||||
// pad_item passThrough actions. link/unlink excluded — see
|
||||
// catalog_item_test.go for their dispatch test.
|
||||
{"pad_item", "create"}: {"item", "create"},
|
||||
{"pad_item", "update"}: {"item", "update"},
|
||||
{"pad_item", "delete"}: {"item", "delete"},
|
||||
{"pad_item", "get"}: {"item", "show"},
|
||||
{"pad_item", "list"}: {"item", "list"},
|
||||
{"pad_item", "move"}: {"item", "move"},
|
||||
{"pad_item", "restore"}: {"item", "restore"},
|
||||
{"pad_item", "deps"}: {"item", "deps"},
|
||||
{"pad_item", "remind"}: {"item", "remind"},
|
||||
{"pad_item", "ack-reminder"}: {"item", "ack"},
|
||||
{"pad_item", "claim"}: {"item", "claim"},
|
||||
{"pad_item", "release"}: {"item", "release"},
|
||||
{"pad_item", "star"}: {"item", "star"},
|
||||
{"pad_item", "unstar"}: {"item", "unstar"},
|
||||
{"pad_item", "starred"}: {"item", "starred"},
|
||||
{"pad_item", "comment"}: {"item", "comment"},
|
||||
{"pad_item", "list-comments"}: {"item", "comments"},
|
||||
{"pad_item", "bulk-update"}: {"item", "bulk-update"},
|
||||
{"pad_item", "note"}: {"item", "note"},
|
||||
{"pad_item", "decide"}: {"item", "decide"},
|
||||
{"pad_item", "backlinks"}: {"item", "backlinks"},
|
||||
{"pad_item", "create"}: {"item", "create"},
|
||||
{"pad_item", "update"}: {"item", "update"},
|
||||
{"pad_item", "delete"}: {"item", "delete"},
|
||||
{"pad_item", "get"}: {"item", "show"},
|
||||
{"pad_item", "list"}: {"item", "list"},
|
||||
{"pad_item", "move"}: {"item", "move"},
|
||||
{"pad_item", "restore"}: {"item", "restore"},
|
||||
{"pad_item", "deps"}: {"item", "deps"},
|
||||
{"pad_item", "remind"}: {"item", "remind"},
|
||||
{"pad_item", "ack-reminder"}: {"item", "ack"},
|
||||
{"pad_item", "claim"}: {"item", "claim"},
|
||||
{"pad_item", "release"}: {"item", "release"},
|
||||
{"pad_item", "star"}: {"item", "star"},
|
||||
{"pad_item", "unstar"}: {"item", "unstar"},
|
||||
{"pad_item", "starred"}: {"item", "starred"},
|
||||
{"pad_item", "comment"}: {"item", "comment"},
|
||||
{"pad_item", "list-comments"}: {"item", "comments"},
|
||||
{"pad_item", "edit-comment"}: {"item", "comment-edit"},
|
||||
{"pad_item", "delete-comment"}: {"item", "comment-delete"},
|
||||
{"pad_item", "bulk-update"}: {"item", "bulk-update"},
|
||||
{"pad_item", "note"}: {"item", "note"},
|
||||
{"pad_item", "decide"}: {"item", "decide"},
|
||||
{"pad_item", "backlinks"}: {"item", "backlinks"},
|
||||
|
||||
// pad_playbook actions (PLAN-1377 / TASK-1381).
|
||||
{"pad_playbook", "list"}: {"playbook", "list"},
|
||||
@@ -335,6 +339,9 @@ func TestReadOnlyCatalog_ActionsDispatchExpectedCmdPath(t *testing.T) {
|
||||
"remind_at": "2026-08-01T09:00:00Z",
|
||||
// pad_playbook.match needs a `text` positional (TASK-3120).
|
||||
"text": "test text",
|
||||
// pad_item.edit-comment / delete-comment address a COMMENT on `ref`
|
||||
// (TASK-2695).
|
||||
"comment_id": "c-1",
|
||||
}
|
||||
|
||||
for _, def := range Catalog {
|
||||
@@ -708,6 +715,16 @@ func liveCmdhelpDoc(t *testing.T) *cmdhelp.Document {
|
||||
Args: mkArgs("ref"),
|
||||
Flags: mkFlags("workspace"),
|
||||
},
|
||||
"item comment-edit": {
|
||||
Summary: "edit comment",
|
||||
Args: append(mkArgs("ref", "comment-id"), cmdhelp.Arg{Name: "message"}),
|
||||
Flags: mkFlags("workspace"),
|
||||
},
|
||||
"item comment-delete": {
|
||||
Summary: "delete comment",
|
||||
Args: mkArgs("ref", "comment-id"),
|
||||
Flags: mkFlags("workspace"),
|
||||
},
|
||||
"item backlinks": {
|
||||
Summary: "list inbound [[...]] references",
|
||||
Args: mkArgs("ref"),
|
||||
|
||||
@@ -453,6 +453,7 @@ func (d *HTTPHandlerDispatcher) buildSpecialRoutes() map[string]specialDispatchF
|
||||
"item note": d.dispatchItemNote,
|
||||
"item decide": d.dispatchItemDecide,
|
||||
"item import": d.dispatchItemImport,
|
||||
"item comment-delete": d.dispatchItemCommentDelete,
|
||||
"project ready": d.dispatchProjectReady,
|
||||
"project stale": d.dispatchProjectStale,
|
||||
"project next": d.dispatchProjectNext,
|
||||
|
||||
@@ -49,6 +49,7 @@ func parityFixtureInput() map[string]any {
|
||||
"title": "test title",
|
||||
"target_collection": "ideas",
|
||||
"message": "test message",
|
||||
"comment_id": "c-1",
|
||||
"summary": "test summary",
|
||||
"decision": "test decision",
|
||||
"code": "123456",
|
||||
|
||||
@@ -258,6 +258,10 @@ func init() {
|
||||
method: http.MethodGet,
|
||||
pathTemplate: "/api/v1/workspaces/{workspace}/items/{ref}/comments",
|
||||
}.toRouteMapper(),
|
||||
// TASK-2695: the item-scoped route, so the comment-is-on-ref check
|
||||
// is the server's on this transport as on the CLI's. Delete is a
|
||||
// special route (dispatchItemCommentDelete) for its 204.
|
||||
"item comment-edit": mapItemCommentEdit,
|
||||
|
||||
// `item backlinks` (BUG-2304) — the reverse [[...]] index
|
||||
// (PLAN-1593). The CLI's --format json emits the endpoint's
|
||||
@@ -1714,6 +1718,66 @@ func cloneStringMap(m map[string]any) map[string]any {
|
||||
return out
|
||||
}
|
||||
|
||||
// itemCommentTarget reads the workspace, ref and comment_id every
|
||||
// item-scoped comment write needs and builds its path (TASK-2695).
|
||||
func itemCommentTarget(input map[string]any) (string, error) {
|
||||
workspace, _ := input["workspace"].(string)
|
||||
ref, _ := input["ref"].(string)
|
||||
commentID, _ := input["comment_id"].(string)
|
||||
if workspace == "" {
|
||||
return "", fmt.Errorf("workspace is required")
|
||||
}
|
||||
if ref == "" {
|
||||
return "", fmt.Errorf("ref is required")
|
||||
}
|
||||
if commentID == "" {
|
||||
return "", fmt.Errorf("comment_id is required")
|
||||
}
|
||||
return fmt.Sprintf("/api/v1/workspaces/%s/items/%s/comments/%s",
|
||||
url.PathEscape(workspace), url.PathEscape(ref), url.PathEscape(commentID)), nil
|
||||
}
|
||||
|
||||
// mapItemCommentEdit dispatches `pad item comment-edit <ref> <comment-id>
|
||||
// <message>` as PATCH {body: <message>}, the rename mapItemComment also makes.
|
||||
func mapItemCommentEdit(input map[string]any) (string, string, []byte, error) {
|
||||
urlPath, err := itemCommentTarget(input)
|
||||
if err != nil {
|
||||
return "", "", nil, err
|
||||
}
|
||||
message, _ := input["message"].(string)
|
||||
if message == "" {
|
||||
return "", "", nil, fmt.Errorf("message is required")
|
||||
}
|
||||
body, err := json.Marshal(map[string]any{"body": message})
|
||||
if err != nil {
|
||||
return "", "", nil, fmt.Errorf("encode body: %w", err)
|
||||
}
|
||||
return http.MethodPatch, urlPath, body, nil
|
||||
}
|
||||
|
||||
// dispatchItemCommentDelete handles `pad item comment-delete <ref>
|
||||
// <comment-id>`. The handler answers 204 with no body, which would reach the
|
||||
// agent as an empty result, so a success is packaged as the CLI's
|
||||
// `--format json` object instead (the dispatchDeleteItemLink precedent).
|
||||
func (d *HTTPHandlerDispatcher) dispatchItemCommentDelete(
|
||||
ctx context.Context,
|
||||
input map[string]any,
|
||||
user *models.User,
|
||||
) (*mcp.CallToolResult, error) {
|
||||
const cmdKey = "item comment-delete"
|
||||
urlPath, err := itemCommentTarget(input)
|
||||
if err != nil {
|
||||
return validationFailedResult(cmdKey, err.Error(), "Pass ref and comment_id (ids come from list-comments)."), nil
|
||||
}
|
||||
res, err := d.executeRequest(ctx, cmdKey, user, http.MethodDelete, urlPath, nil)
|
||||
if err != nil || res.IsError {
|
||||
return res, err
|
||||
}
|
||||
ref, _ := input["ref"].(string)
|
||||
commentID, _ := input["comment_id"].(string)
|
||||
return packageStructuredResponse(cmdKey, map[string]any{"deleted": true, "ref": ref, "comment_id": commentID})
|
||||
}
|
||||
|
||||
// mapItemComment dispatches `pad item comment <ref> <message>`.
|
||||
//
|
||||
// POST /api/v1/workspaces/{ws}/items/{ref}/comments with body shape
|
||||
|
||||
File diff suppressed because one or more lines are too long
@@ -0,0 +1,153 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
"github.com/PerpetualSoftware/pad/internal/server"
|
||||
"github.com/PerpetualSoftware/pad/internal/store/storetest"
|
||||
mcpserver "github.com/mark3labs/mcp-go/server"
|
||||
)
|
||||
|
||||
// TASK-2695. edit-comment / delete-comment through the REMOTE door, end to
|
||||
// end: raw JSON-RPC → catalog → HTTPHandlerDispatcher → the real handler
|
||||
// chain → the store. The wrong-item rows are the point: the comment-is-on-ref
|
||||
// check lives on the server, so a dispatcher that addressed the
|
||||
// workspace-scoped route instead would pass the happy rows and fail these.
|
||||
func TestCommentEditDelete_Remote(t *testing.T) {
|
||||
s := storetest.NewSQLite(t)
|
||||
srv := server.New(s)
|
||||
t.Cleanup(srv.Stop)
|
||||
owner, err := s.CreateUser(models.UserCreate{Email: "o@example.com", Name: "O", Password: "correct-horse-battery-staple"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "W", Slug: "w2695", OwnerID: owner.ID})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := s.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
coll, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Tasks", Slug: "tasks", Prefix: "TASK", Schema: `{"fields":[]}`})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
itemA, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "A"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
itemB, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "B"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
onA, err := s.CreateComment(ws.ID, itemA.ID, owner.ID, models.CommentCreate{Body: "on A", CreatedBy: "agent"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
onB, err := s.CreateComment(ws.ID, itemB.ID, owner.ID, models.CommentCreate{Body: "on B", CreatedBy: "agent"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
m := mcpserver.NewMCPServer("t", "1", mcpserver.WithToolCapabilities(true))
|
||||
d := &HTTPHandlerDispatcher{Handler: srv, UserResolver: func(context.Context) *models.User { return owner }}
|
||||
if _, err := RegisterCatalog(m, CatalogOptions{Doc: liveCmdhelpDoc(t), Workspace: NewWorkspaceState(ws.Slug), Dispatcher: d, PadVersion: "test"}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
call := func(id int, args string) string {
|
||||
t.Helper()
|
||||
return respText(t, m.HandleMessage(context.Background(), toolsCall(id, args)))
|
||||
}
|
||||
body := func(id string) string {
|
||||
t.Helper()
|
||||
c, err := s.GetComment(id)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if c == nil {
|
||||
return "<deleted>"
|
||||
}
|
||||
return c.Body
|
||||
}
|
||||
|
||||
refA := fmt.Sprintf("TASK-%d", *itemA.ItemNumber)
|
||||
|
||||
resp := call(1, fmt.Sprintf(`{"action":"edit-comment","ref":%q,"comment_id":%q,"message":"on A, fixed"}`, refA, onA.ID))
|
||||
if strings.Contains(resp, `"isError":true`) {
|
||||
t.Fatalf("edit through the right item failed: %s", resp)
|
||||
}
|
||||
if got := body(onA.ID); got != "on A, fixed" {
|
||||
t.Fatalf("edit did not land: body = %q", got)
|
||||
}
|
||||
|
||||
resp = call(2, fmt.Sprintf(`{"action":"edit-comment","ref":%q,"comment_id":%q,"message":"hijacked"}`, refA, onB.ID))
|
||||
if !strings.Contains(resp, `"isError":true`) {
|
||||
t.Fatalf("edit of B's comment through A succeeded: %s", resp)
|
||||
}
|
||||
if got := body(onB.ID); got != "on B" {
|
||||
t.Fatalf("B's comment changed through A: %q", got)
|
||||
}
|
||||
|
||||
resp = call(3, fmt.Sprintf(`{"action":"delete-comment","ref":%q,"comment_id":%q}`, refA, onB.ID))
|
||||
if !strings.Contains(resp, `"isError":true`) {
|
||||
t.Fatalf("delete of B's comment through A succeeded: %s", resp)
|
||||
}
|
||||
if got := body(onB.ID); got != "on B" {
|
||||
t.Fatalf("B's comment deleted through A: %q", got)
|
||||
}
|
||||
|
||||
resp = call(4, fmt.Sprintf(`{"action":"list-comments","ref":%q}`, refA))
|
||||
if !strings.Contains(resp, `\"edited\":`) {
|
||||
t.Fatalf("list-comments rows carry no edited key: %s", resp)
|
||||
}
|
||||
|
||||
resp = call(5, fmt.Sprintf(`{"action":"delete-comment","ref":%q,"comment_id":%q}`, refA, onA.ID))
|
||||
if strings.Contains(resp, `"isError":true`) {
|
||||
t.Fatalf("delete through the right item failed: %s", resp)
|
||||
}
|
||||
if !strings.Contains(resp, `\"deleted\":true`) || !strings.Contains(resp, onA.ID) {
|
||||
t.Fatalf("delete result is not the CLI's JSON shape: %s", resp)
|
||||
}
|
||||
if got := body(onA.ID); got != "<deleted>" {
|
||||
t.Fatalf("comment still present after delete: %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCommentEditDelete_Stdio pins the local door's argv: the actions reach
|
||||
// the CLI verbs with ref, comment id and body as positionals behind `--`, so
|
||||
// a body that starts with "-" is not parsed as a flag (the v0.42 rule).
|
||||
func TestCommentEditDelete_Stdio(t *testing.T) {
|
||||
fd := &fakeDispatcher{}
|
||||
m := mcpserver.NewMCPServer("t", "1", mcpserver.WithToolCapabilities(true))
|
||||
if _, err := RegisterCatalog(m, CatalogOptions{Doc: liveCmdhelpDoc(t), Workspace: NewWorkspaceState("ws"), Dispatcher: fd, PadVersion: "test"}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
resp := respText(t, m.HandleMessage(context.Background(),
|
||||
toolsCall(1, `{"action":"edit-comment","ref":"TASK-1","comment_id":"c-1","message":"- corrected"}`)))
|
||||
if strings.Contains(resp, `"isError":true`) {
|
||||
t.Fatalf("edit-comment failed: %s", resp)
|
||||
}
|
||||
if got := strings.Join(fd.gotPath, " "); got != "item comment-edit" {
|
||||
t.Fatalf("edit-comment dispatched to %q", got)
|
||||
}
|
||||
if got := strings.Join(fd.gotArgs, "\x00"); !strings.HasSuffix(got, strings.Join([]string{"--", "TASK-1", "c-1", "- corrected"}, "\x00")) {
|
||||
t.Fatalf("edit-comment argv = %q", fd.gotArgs)
|
||||
}
|
||||
|
||||
resp = respText(t, m.HandleMessage(context.Background(),
|
||||
toolsCall(2, `{"action":"delete-comment","ref":"TASK-1","comment_id":"c-1"}`)))
|
||||
if strings.Contains(resp, `"isError":true`) {
|
||||
t.Fatalf("delete-comment failed: %s", resp)
|
||||
}
|
||||
if got := strings.Join(fd.gotPath, " "); got != "item comment-delete" {
|
||||
t.Fatalf("delete-comment dispatched to %q", got)
|
||||
}
|
||||
if got := strings.Join(fd.gotArgs, "\x00"); !strings.HasSuffix(got, strings.Join([]string{"--", "TASK-1", "c-1"}, "\x00")) {
|
||||
t.Fatalf("delete-comment argv = %q", fd.gotArgs)
|
||||
}
|
||||
}
|
||||
+19
-1
@@ -1651,6 +1651,24 @@ const CmdhelpVersion = "0.1"
|
||||
// this one either, and it is not a promise that the row will catch
|
||||
// up at all.
|
||||
//
|
||||
// 0.57 — TASK-2695. ADDITIVE bump (the v0.28 / v0.13 disposition): two
|
||||
// `pad_item` actions, `edit-comment` (ref, comment_id, message) and
|
||||
// `delete-comment` (ref, comment_id), and the `comment_id` param. Web
|
||||
// users could edit and delete comments since PLAN-1662; agents could
|
||||
// only append corrections. Both ride new item-scoped routes,
|
||||
// PATCH/DELETE /items/{ref}/comments/{id}, which answer 404 when the
|
||||
// comment is not on `ref`. That check is the server's on every
|
||||
// transport, and the ACL is the workspace route's unchanged: edit is
|
||||
// author-only (a bearer admin gets no bypass, BUG-1919), delete is
|
||||
// anyone who may edit the item. Remote packages a delete's 204 as the
|
||||
// CLI's JSON, `{deleted, ref, comment_id}`. Every serialised comment
|
||||
// (list-comments on all transports, and the API) now carries a derived
|
||||
// `edited` bool, true once the body changed (updated_at > created_at,
|
||||
// the web timeline's rule). That key is new, not a change to an
|
||||
// existing one. Stdio reaches the actions through the new CLI verbs
|
||||
// `pad item comment-edit` / `comment-delete`, which refuse against a
|
||||
// server that does not advertise `item_scoped_comment_writes`.
|
||||
//
|
||||
// Post-0.37, deliberately NO bump (BUG-3098): `workspace_member_limit`
|
||||
// joins the structured 403 allow-list, as ErrWorkspaceMemberLimit with
|
||||
// its own hint (not the plan-limit "upgrade" hint). The server emits it
|
||||
@@ -1659,7 +1677,7 @@ const CmdhelpVersion = "0.1"
|
||||
// this surface can receive it; the entry exists so a future action does
|
||||
// not collapse it to permission_denied. When an action that can reach
|
||||
// it is added, that addition is the contract change and owns the bump.
|
||||
const ToolSurfaceVersion = "0.56"
|
||||
const ToolSurfaceVersion = "0.57"
|
||||
|
||||
// MetaVersionURI is the canonical URI of the queryable version document.
|
||||
// Lives outside the pad://workspace/{ws}/... namespace because it's a
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
package models
|
||||
|
||||
import "time"
|
||||
import (
|
||||
"encoding/json"
|
||||
"time"
|
||||
)
|
||||
|
||||
// Comment represents a comment on an item.
|
||||
type Comment struct {
|
||||
@@ -44,6 +47,28 @@ type Comment struct {
|
||||
Reactions []Reaction `json:"reactions,omitempty"`
|
||||
}
|
||||
|
||||
// IsEdited reports whether the body changed after creation: create stamps
|
||||
// created_at and updated_at with one value, and only an edit moves updated_at
|
||||
// (reactions live in their own table). Timestamps are second-precision, so an
|
||||
// edit inside the creation second is not seen. Same rule as the web timeline's
|
||||
// isEdited (TimelineCommentCard.svelte); TASK-2695 brought it to the CLI.
|
||||
func (c Comment) IsEdited() bool {
|
||||
return !c.CreatedAt.IsZero() && c.UpdatedAt.After(c.CreatedAt)
|
||||
}
|
||||
|
||||
// MarshalJSON adds a derived, read-only `edited` (IsEdited) to every
|
||||
// serialised comment, so an agent reading JSON on either MCP transport sees
|
||||
// the same marker the CLI table and the web timeline show without comparing
|
||||
// timestamps itself (TASK-2695). It is not stored and not read back: Comment
|
||||
// has no field for it, so a decode ignores it.
|
||||
func (c Comment) MarshalJSON() ([]byte, error) {
|
||||
type plain Comment
|
||||
return json.Marshal(struct {
|
||||
plain
|
||||
Edited bool `json:"edited"`
|
||||
}{plain(c), c.IsEdited()})
|
||||
}
|
||||
|
||||
// CommentCreate is the input for creating a new comment.
|
||||
type CommentCreate struct {
|
||||
Author string `json:"author,omitempty"`
|
||||
|
||||
@@ -0,0 +1,47 @@
|
||||
package models
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// TASK-2695: `edited` is derived from the timestamps and emitted on every
|
||||
// serialised comment, nested replies included.
|
||||
func TestCommentEditedMarker(t *testing.T) {
|
||||
created := time.Date(2026, 9, 27, 12, 0, 0, 0, time.UTC)
|
||||
fresh := Comment{ID: "c1", CreatedAt: created, UpdatedAt: created}
|
||||
edited := Comment{ID: "c2", CreatedAt: created, UpdatedAt: created.Add(time.Second)}
|
||||
|
||||
if fresh.IsEdited() {
|
||||
t.Error("a comment whose timestamps match is not edited")
|
||||
}
|
||||
if !edited.IsEdited() {
|
||||
t.Error("a comment updated after creation is edited")
|
||||
}
|
||||
if (Comment{}).IsEdited() {
|
||||
t.Error("a zero comment is not edited")
|
||||
}
|
||||
|
||||
fresh.Replies = []Comment{edited}
|
||||
b, err := json.Marshal(fresh)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
got := string(b)
|
||||
if !strings.Contains(got, `"edited":false`) || !strings.Contains(got, `"edited":true`) {
|
||||
t.Fatalf("want edited on the comment and on its reply, got %s", got)
|
||||
}
|
||||
if !strings.Contains(got, `"id":"c1"`) || !strings.Contains(got, `"replies":[`) {
|
||||
t.Fatalf("the ordinary fields must still serialise: %s", got)
|
||||
}
|
||||
|
||||
var back Comment
|
||||
if err := json.Unmarshal(b, &back); err != nil {
|
||||
t.Fatalf("a serialised comment must still decode: %v", err)
|
||||
}
|
||||
if back.ID != "c1" || len(back.Replies) != 1 {
|
||||
t.Fatalf("round trip lost fields: %+v", back)
|
||||
}
|
||||
}
|
||||
@@ -43,6 +43,12 @@ type serverCapabilities struct {
|
||||
// literal slug, so a shorthand (`task` for `tasks`) finds nothing and the
|
||||
// client must keep normalising.
|
||||
SearchCollectionResolution bool `json:"search_collection_resolution"`
|
||||
|
||||
// ItemScopedCommentWrites is true when PATCH and DELETE are routed at
|
||||
// /items/{ref}/comments/{id} (TASK-2695). An older build has no such
|
||||
// route and answers a bare 404, which a client would read as "comment
|
||||
// not found", so the CLI refuses instead of sending when this is absent.
|
||||
ItemScopedCommentWrites bool `json:"item_scoped_comment_writes"`
|
||||
}
|
||||
|
||||
// WHAT A BUILD THAT CANNOT DECODE A FORMAT ACTUALLY COSTS THE READER
|
||||
@@ -85,7 +91,7 @@ type serverCapabilities struct {
|
||||
// rather than 500-ing — that signals to the editor "uploads still work,
|
||||
// but disable transformation tools."
|
||||
func (s *Server) handleServerCapabilities(w http.ResponseWriter, r *http.Request) {
|
||||
resp := serverCapabilities{CollectionResolution: true, ItemFieldAppend: true, SearchCollectionResolution: true}
|
||||
resp := serverCapabilities{CollectionResolution: true, ItemFieldAppend: true, SearchCollectionResolution: true, ItemScopedCommentWrites: true}
|
||||
if s.imageProcessor != nil {
|
||||
resp.Image = s.imageProcessor.Capabilities()
|
||||
} else {
|
||||
|
||||
@@ -259,6 +259,46 @@ func (s *Server) handleUpdateComment(w http.ResponseWriter, r *http.Request) {
|
||||
writeJSON(w, http.StatusOK, updated)
|
||||
}
|
||||
|
||||
// itemScopedComment adapts a /comments/{commentID} handler to the item-scoped
|
||||
// route /items/{itemSlug}/comments/{commentID} (TASK-2695). It resolves the
|
||||
// item and answers 404 unless the comment is on it, then runs next unchanged,
|
||||
// so the ACL, validation and events stay the workspace-scoped handler's own.
|
||||
//
|
||||
// The item-scoped form exists for the agent doors (CLI, both MCP transports),
|
||||
// which name a comment by item ref plus id: checking the pairing here, rather
|
||||
// than in each client, is what makes the three doors land on one check. The
|
||||
// remote MCP dispatcher maps an action to a single HTTP call, so a
|
||||
// client-side check could not reach it.
|
||||
//
|
||||
// A comment that exists on another item answers the same 404 as one that
|
||||
// does not exist, so the route says nothing about comments elsewhere.
|
||||
func (s *Server) itemScopedComment(next http.HandlerFunc) http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
workspaceID, ok := s.getWorkspaceID(w, r)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
item, err := s.store.ResolveItemIncludeDeleted(workspaceID, chi.URLParam(r, "itemSlug"))
|
||||
if err != nil {
|
||||
writeInternalError(w, err)
|
||||
return
|
||||
}
|
||||
if item == nil {
|
||||
writeError(w, http.StatusNotFound, "not_found", "Item not found")
|
||||
return
|
||||
}
|
||||
if !s.requireItemVisible(w, r, workspaceID, item) {
|
||||
return
|
||||
}
|
||||
comment, cerr := s.store.GetComment(chi.URLParam(r, "commentID"))
|
||||
if cerr != nil || comment == nil || comment.WorkspaceID != workspaceID || comment.ItemID != item.ID {
|
||||
writeError(w, http.StatusNotFound, "not_found", "Comment not found")
|
||||
return
|
||||
}
|
||||
next(w, r)
|
||||
}
|
||||
}
|
||||
|
||||
// canEditComment reports whether the requester may edit the given comment:
|
||||
// the authenticated author (matching user_id) or a platform admin. A comment
|
||||
// with an empty user_id has no provable author, so only admins can edit it.
|
||||
|
||||
@@ -0,0 +1,142 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestItemScopedCommentWrites pins the item-scoped comment routes the agent
|
||||
// doors use (TASK-2695): PATCH and DELETE /items/{ref}/comments/{id} refuse a
|
||||
// comment that is not on {ref}, and otherwise behave exactly like the
|
||||
// workspace-scoped routes they delegate to, ACL included.
|
||||
func TestItemScopedCommentWrites(t *testing.T) {
|
||||
t.Parallel()
|
||||
env := setupRBACEnv(t)
|
||||
ws := "/api/v1/workspaces/" + env.wsSlug
|
||||
|
||||
newItem := func(title string) string {
|
||||
t.Helper()
|
||||
rr := doRequestWithCookie(env.srv, "POST", ws+"/collections/docs/items",
|
||||
map[string]any{"title": title}, env.ownerToken)
|
||||
if rr.Code != http.StatusCreated {
|
||||
t.Fatalf("create item: %d %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
var item map[string]any
|
||||
parseJSON(t, rr, &item)
|
||||
return item["slug"].(string)
|
||||
}
|
||||
newComment := func(slug, body, token string) string {
|
||||
t.Helper()
|
||||
rr := doRequestWithCookie(env.srv, "POST", ws+"/items/"+slug+"/comments",
|
||||
map[string]any{"body": body}, token)
|
||||
if rr.Code != http.StatusCreated {
|
||||
t.Fatalf("post comment: %d %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
var c map[string]any
|
||||
parseJSON(t, rr, &c)
|
||||
if c["edited"] != false {
|
||||
t.Fatalf("fresh comment: want edited=false, got %v", c["edited"])
|
||||
}
|
||||
return c["id"].(string)
|
||||
}
|
||||
listBodies := func(slug string) map[string]map[string]any {
|
||||
t.Helper()
|
||||
rr := doRequestWithCookie(env.srv, "GET", ws+"/items/"+slug+"/comments", nil, env.ownerToken)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("list comments: %d %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
var list []map[string]any
|
||||
parseJSON(t, rr, &list)
|
||||
out := map[string]map[string]any{}
|
||||
for _, c := range list {
|
||||
out[c["id"].(string)] = c
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
itemA := newItem("Scoped A")
|
||||
itemB := newItem("Scoped B")
|
||||
onA := newComment(itemA, "on A", env.editorToken)
|
||||
onB := newComment(itemB, "on B", env.editorToken)
|
||||
|
||||
t.Run("edit through the right item", func(t *testing.T) {
|
||||
rr := doRequestWithCookie(env.srv, "PATCH", ws+"/items/"+itemA+"/comments/"+onA,
|
||||
map[string]any{"body": "on A, corrected"}, env.editorToken)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("want 200, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
got := listBodies(itemA)[onA]
|
||||
if got["body"] != "on A, corrected" {
|
||||
t.Fatalf("body not updated: %v", got["body"])
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("edit through the wrong item is refused and writes nothing", func(t *testing.T) {
|
||||
rr := doRequestWithCookie(env.srv, "PATCH", ws+"/items/"+itemA+"/comments/"+onB,
|
||||
map[string]any{"body": "hijacked"}, env.editorToken)
|
||||
if rr.Code != http.StatusNotFound || !strings.Contains(rr.Body.String(), "Comment not found") {
|
||||
t.Fatalf("want 404 Comment not found, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
if got := listBodies(itemB)[onB]["body"]; got != "on B" {
|
||||
t.Fatalf("comment on B changed through A: %v", got)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("unknown item is an item 404", func(t *testing.T) {
|
||||
rr := doRequestWithCookie(env.srv, "PATCH", ws+"/items/no-such-item/comments/"+onA,
|
||||
map[string]any{"body": "x"}, env.editorToken)
|
||||
if rr.Code != http.StatusNotFound || !strings.Contains(rr.Body.String(), "Item not found") {
|
||||
t.Fatalf("want 404 Item not found, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("the edit ACL is the workspace route's", func(t *testing.T) {
|
||||
rr := doRequestWithCookie(env.srv, "PATCH", ws+"/items/"+itemA+"/comments/"+onA,
|
||||
map[string]any{"body": "not mine"}, env.viewerToken)
|
||||
if rr.Code != http.StatusForbidden {
|
||||
t.Fatalf("non-author edit: want 403, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("delete through the wrong item is refused and deletes nothing", func(t *testing.T) {
|
||||
rr := doRequestWithCookie(env.srv, "DELETE", ws+"/items/"+itemA+"/comments/"+onB, nil, env.editorToken)
|
||||
if rr.Code != http.StatusNotFound {
|
||||
t.Fatalf("want 404, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
if _, ok := listBodies(itemB)[onB]; !ok {
|
||||
t.Fatal("comment on B was deleted through A")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("the delete ACL is the workspace route's", func(t *testing.T) {
|
||||
rr := doRequestWithCookie(env.srv, "DELETE", ws+"/items/"+itemB+"/comments/"+onB, nil, env.viewerToken)
|
||||
if rr.Code != http.StatusForbidden {
|
||||
t.Fatalf("viewer delete: want 403, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("delete through the right item", func(t *testing.T) {
|
||||
rr := doRequestWithCookie(env.srv, "DELETE", ws+"/items/"+itemB+"/comments/"+onB, nil, env.editorToken)
|
||||
if rr.Code != http.StatusNoContent {
|
||||
t.Fatalf("want 204, got %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
if _, ok := listBodies(itemB)[onB]; ok {
|
||||
t.Fatal("comment still listed after delete")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestServerCapabilitiesAdvertisesItemScopedCommentWrites(t *testing.T) {
|
||||
t.Parallel()
|
||||
srv := testServer(t)
|
||||
rr := doRequest(srv, "GET", "/api/v1/server/capabilities", nil)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("capabilities: %d %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
var caps map[string]any
|
||||
parseJSON(t, rr, &caps)
|
||||
if caps["item_scoped_comment_writes"] != true {
|
||||
t.Fatalf("want item_scoped_comment_writes=true, got %v", caps["item_scoped_comment_writes"])
|
||||
}
|
||||
}
|
||||
@@ -1933,6 +1933,8 @@ func (s *Server) setupRouter() {
|
||||
r.Post("/links", s.handleCreateItemLink)
|
||||
r.Get("/comments", s.handleListComments)
|
||||
r.Post("/comments", s.handleCreateComment)
|
||||
r.Patch("/comments/{commentID}", s.itemScopedComment(s.handleUpdateComment))
|
||||
r.Delete("/comments/{commentID}", s.itemScopedComment(s.handleDeleteComment))
|
||||
r.Get("/timeline", s.handleListItemTimeline)
|
||||
r.Get("/children", s.handleGetItemChildren)
|
||||
r.Get("/progress", s.handleGetItemProgress)
|
||||
|
||||
+3
-1
@@ -206,7 +206,9 @@ pad item remind TASK-5 --remind-at 2026-08-01T09:00:00Z # arm a one-shot remin
|
||||
pad item reminders TASK-5 # armed / fired / acknowledged
|
||||
pad item ack <reminder-id> # acknowledge a fired reminder
|
||||
pad item unremind <reminder-id>
|
||||
pad item comments TASK-5
|
||||
pad item comments TASK-5 # each row shows its id, and "edited" once changed
|
||||
pad item comment-edit TASK-5 <comment-id> "..." [--stdin] # correct your own comment in place instead of appending
|
||||
pad item comment-delete TASK-5 <comment-id>
|
||||
pad item note TASK-5 "what you did" [--details "..." | --stdin]
|
||||
pad item decide TASK-5 "what you chose" [--rationale "..." | --stdin]
|
||||
pad item bulk-update --status X TASK-5 TASK-8 ...
|
||||
|
||||
@@ -2108,6 +2108,21 @@ export const api = {
|
||||
method: 'DELETE'
|
||||
}),
|
||||
|
||||
// Item-scoped edit/delete (TASK-2695): the server answers 404 unless
|
||||
// the comment is on itemRef. WebMCP uses these so its edit-comment /
|
||||
// delete-comment land on the same check as the CLI and /mcp.
|
||||
updateOnItem: (ws: string, itemRef: string, commentId: string, data: { body: string }) =>
|
||||
request<Comment>(
|
||||
`/workspaces/${ws}/items/${encodeURIComponent(itemRef)}/comments/${encodeURIComponent(commentId)}`,
|
||||
{ method: 'PATCH', body: JSON.stringify(data) }
|
||||
),
|
||||
|
||||
deleteOnItem: (ws: string, itemRef: string, commentId: string) =>
|
||||
request<void>(
|
||||
`/workspaces/${ws}/items/${encodeURIComponent(itemRef)}/comments/${encodeURIComponent(commentId)}`,
|
||||
{ method: 'DELETE' }
|
||||
),
|
||||
|
||||
reply: (ws: string, commentId: string, data: CommentCreate) =>
|
||||
request<Comment>(`/workspaces/${ws}/comments/${commentId}/replies`, {
|
||||
method: 'POST',
|
||||
|
||||
@@ -1574,6 +1574,8 @@ export interface Comment {
|
||||
parent_id?: string;
|
||||
created_at: string;
|
||||
updated_at: string;
|
||||
/** Derived server-side: the body changed after creation (TASK-2695). */
|
||||
edited?: boolean;
|
||||
item_title?: string;
|
||||
item_slug?: string;
|
||||
/**
|
||||
|
||||
@@ -31,6 +31,8 @@ function mockApi() {
|
||||
comments: {
|
||||
list: vi.fn(async () => []),
|
||||
create: vi.fn(async () => ({ id: 'c-1' })),
|
||||
updateOnItem: vi.fn(async () => ({ id: 'c-1', body: 'fixed', edited: true })),
|
||||
deleteOnItem: vi.fn(async () => undefined),
|
||||
},
|
||||
dashboard: { get: vi.fn(async () => ({ ok: true })) },
|
||||
next: vi.fn(async () => [{ item_slug: 'task-1', item_title: 'Do it', collection: 'tasks', reason: 'high priority' }]),
|
||||
@@ -104,7 +106,7 @@ const READ = new Set([
|
||||
const isReadOnly = (tool: string, action: string): boolean | undefined => {
|
||||
if (READ.has(`${tool}:${action}`)) return true;
|
||||
if (
|
||||
['create', 'update', 'delete', 'import', 'comment', 'link', 'unlink', 'move',
|
||||
['create', 'update', 'delete', 'import', 'comment', 'edit-comment', 'delete-comment', 'link', 'unlink', 'move',
|
||||
'restore', 'star', 'unstar', 'bulk-update', 'activate'].includes(action)
|
||||
)
|
||||
return false;
|
||||
@@ -359,6 +361,43 @@ describe('dispatch — pad_item writes', () => {
|
||||
);
|
||||
});
|
||||
|
||||
// TASK-2695: the item-scoped client methods, so the server refuses a
|
||||
// comment_id that is not on `ref`, as it does for the CLI and /mcp.
|
||||
it('edit-comment maps ref, comment_id and message to the item-scoped update', async () => {
|
||||
const api = mockApi();
|
||||
const res = parse(
|
||||
await run(api, 'pad_item', {
|
||||
action: 'edit-comment',
|
||||
ref: 'TASK-1',
|
||||
comment_id: 'c-1',
|
||||
message: 'fixed',
|
||||
}),
|
||||
);
|
||||
expect(res.isError).toBe(false);
|
||||
expect(api.comments.updateOnItem).toHaveBeenCalledWith(WS, 'TASK-1', 'c-1', { body: 'fixed' });
|
||||
});
|
||||
|
||||
it('edit-comment refuses a missing comment_id or message without calling the api', async () => {
|
||||
const api = mockApi();
|
||||
const noId = parse(await run(api, 'pad_item', { action: 'edit-comment', ref: 'TASK-1', message: 'x' }));
|
||||
const noBody = parse(await run(api, 'pad_item', { action: 'edit-comment', ref: 'TASK-1', comment_id: 'c-1' }));
|
||||
expect(noId.isError).toBe(true);
|
||||
expect(noId.text).toContain("'comment_id'");
|
||||
expect(noBody.isError).toBe(true);
|
||||
expect(noBody.text).toContain("'message'");
|
||||
expect(api.comments.updateOnItem).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('delete-comment uses the item-scoped delete and returns the CLI-shaped result', async () => {
|
||||
const api = mockApi();
|
||||
const res = parse(
|
||||
await run(api, 'pad_item', { action: 'delete-comment', ref: 'TASK-1', comment_id: 'c-1' }),
|
||||
);
|
||||
expect(res.isError).toBe(false);
|
||||
expect(api.comments.deleteOnItem).toHaveBeenCalledWith(WS, 'TASK-1', 'c-1');
|
||||
expect(JSON.parse(res.text)).toEqual({ deleted: true, ref: 'TASK-1', comment_id: 'c-1' });
|
||||
});
|
||||
|
||||
it('star/unstar map to the right client methods', async () => {
|
||||
const api = mockApi();
|
||||
await run(api, 'pad_item', { action: 'star', ref: 'TASK-1' });
|
||||
|
||||
@@ -522,6 +522,18 @@ const HANDLERS: Record<string, Handler> = {
|
||||
parent_id: str(args, 'reply_to'),
|
||||
source: 'web',
|
||||
}),
|
||||
// TASK-2695: item-scoped, so a comment_id not on `ref` is refused here
|
||||
// exactly as on the CLI and /mcp. Delete mirrors their JSON result.
|
||||
'pad_item:edit-comment': (api, ws, args) =>
|
||||
api.comments.updateOnItem(ws, requireRef(args), requireArg(args, 'comment_id'), {
|
||||
body: requireArg(args, 'message'),
|
||||
}),
|
||||
'pad_item:delete-comment': async (api, ws, args) => {
|
||||
const ref = requireRef(args);
|
||||
const commentId = requireArg(args, 'comment_id');
|
||||
await api.comments.deleteOnItem(ws, ref, commentId);
|
||||
return { deleted: true, ref, comment_id: commentId };
|
||||
},
|
||||
'pad_item:bulk-update': (api, ws, args) => {
|
||||
const ids = strArray(args, 'refs');
|
||||
if (!ids || ids.length === 0) {
|
||||
|
||||
Reference in New Issue
Block a user