diff --git a/cmd/pad/cmd_push.go b/cmd/pad/cmd_push.go new file mode 100644 index 00000000..63a9aa34 --- /dev/null +++ b/cmd/pad/cmd_push.go @@ -0,0 +1,76 @@ +package main + +import ( + "fmt" + "strings" + + "github.com/spf13/cobra" + + "github.com/PerpetualSoftware/pad/internal/cli" +) + +// maxPushMessageLenForHelp mirrors server.maxPushMessageLen +// (handlers_push.go) — KEEP IN SYNC. Duplicated rather than shared +// because the two live in different packages with no existing shared +// constants package for a single value; it exists here purely so +// `pad push --help` states the bound instead of a user only learning it +// from a 400. The server is still the enforcing source of truth — this +// value is advisory (help text), not validated client-side. +const maxPushMessageLenForHelp = 4096 + +// pushCmd is `pad push -m "message"` (IDEA-2544 Phase 1) — the +// "push this to my agent" verb: an explicit, user-authored instruction +// bound to an item, delivered to the caller's own connected monitor +// sessions over the same watch-events stream `pad watch --stream +// --for-session` consumes. Distinct from `pad watch` (a durable, item- +// scoped subscription): a push is one-shot, transient, and IS an +// instruction rather than a passive fact — see the plugin skill's +// notification-etiquette section for the receiving-agent behavior +// contract. +func pushCmd() *cobra.Command { + var message string + + cmd := &cobra.Command{ + Use: "push ", + Short: "Push an item + instruction to your own connected agent session(s)", + Long: fmt.Sprintf(`pad push -m "message" + Publish a self-addressed push notification on an item. Every one of + your OWN connected plugin-monitor sessions (pad watch --stream + --for-session) receives it — this is fire-and-forget over the + in-memory watch-events bus, with no durable inbox: a push with no + connected session listening is simply not seen (Phase 1 scope). + + -m/--message is required and must not be blank; it is the + instruction text the receiving agent acts on (load the item first, + then do what the message says — see the plugin skill's notification + section for the full contract). Newlines are collapsed to spaces: + the monitor stream is a one-line-per-event wire contract. Limited to + %d characters after that collapse — the server rejects anything + longer rather than truncating it.`, maxPushMessageLenForHelp), + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + trimmed := strings.Join(strings.Fields(message), " ") + if trimmed == "" { + return fmt.Errorf("-m/--message is required and must not be blank") + } + + client, _ := getClient() + ws := getWorkspace() + + result, err := client.PushItem(ws, args[0], trimmed) + if err != nil { + return err + } + + if formatFlag == "json" { + return cli.PrintJSON(result) + } + fmt.Printf("Pushed %s\n", args[0]) + return nil + }, + } + + cmd.Flags().StringVarP(&message, "message", "m", "", + fmt.Sprintf("instruction text to push (required, max %d characters after whitespace collapse)", maxPushMessageLenForHelp)) + return cmd +} diff --git a/cmd/pad/cmd_push_test.go b/cmd/pad/cmd_push_test.go new file mode 100644 index 00000000..2913e296 --- /dev/null +++ b/cmd/pad/cmd_push_test.go @@ -0,0 +1,138 @@ +package main + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) + +// setupPushTest mirrors setupItemOpenTest (item_open_test.go): points +// getClient()/getWorkspace() at a fake httptest server via the same +// urlFlag/workspaceFlag override the real CLI entry points read. +func setupPushTest(t *testing.T, handler http.Handler) *httptest.Server { + t.Helper() + + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + + t.Setenv("HOME", t.TempDir()) + + previousWorkspace := workspaceFlag + previousURL := urlFlag + workspaceFlag = "demo" + urlFlag = server.URL + "/" + t.Cleanup(func() { + workspaceFlag = previousWorkspace + urlFlag = previousURL + }) + + return server +} + +// TestPushCmd_SendsCollapsedMessage covers the client round trip: the +// CLI posts the trimmed, newline-collapsed message to the item's /push +// endpoint and reports success. +func TestPushCmd_SendsCollapsedMessage(t *testing.T) { + var gotPath string + var gotBody map[string]string + setupPushTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + _ = json.NewDecoder(r.Body).Decode(&gotBody) + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{"ref": "TASK-5", "pushed": true}) + })) + + var output bytes.Buffer + cmd := pushCmd() + cmd.SetOut(&output) + cmd.SetArgs([]string{"TASK-5", "-m", "triage this\nwith the triage playbook"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("execute push: %v", err) + } + + wantPath := "/api/v1/workspaces/demo/items/TASK-5/push" + if gotPath != wantPath { + t.Fatalf("path = %q, want %q", gotPath, wantPath) + } + wantMessage := "triage this with the triage playbook" + if gotBody["message"] != wantMessage { + t.Fatalf("message = %q, want %q", gotBody["message"], wantMessage) + } +} + +// TestPushCmd_FormatJSON covers the P2 finding (dispatcher review round +// 2, codex): --format json was silently ignored — the RunE hardcoded +// plain text regardless of formatFlag. Mirrors runCreateWatch's +// formatFlag == "json" branch (cmd_watch.go) and asserts the full +// PushResult shape (ref, workspace, pushed, message), not just that +// SOME JSON came out. +func TestPushCmd_FormatJSON(t *testing.T) { + setupPushTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "ref": "TASK-5", "workspace": "demo", "pushed": true, "message": "triage this", + }) + })) + prevFormat := formatFlag + formatFlag = "json" + defer func() { formatFlag = prevFormat }() + + cmd := pushCmd() + cmd.SetArgs([]string{"TASK-5", "-m", "triage this"}) + + var execErr error + out := captureStdout(t, func() { execErr = cmd.Execute() }) + if execErr != nil { + t.Fatalf("execute push: %v", execErr) + } + + var result PushResultForTest + if err := json.Unmarshal([]byte(out), &result); err != nil { + t.Fatalf("output is not valid JSON: %v\noutput: %s", err, out) + } + if result.Ref != "TASK-5" || result.Workspace != "demo" || !result.Pushed || result.Message != "triage this" { + t.Fatalf("unexpected JSON result: %+v", result) + } +} + +// PushResultForTest mirrors cli.PushResult's wire shape — cmd/pad +// doesn't import internal/cli's type back out for a test-local decode, +// so this pins the same JSON tags independently (a divergence here +// would fail this test, which is the point). +type PushResultForTest struct { + Ref string `json:"ref"` + Workspace string `json:"workspace"` + Pushed bool `json:"pushed"` + Message string `json:"message"` +} + +// TestPushCmd_RequiresNonBlankMessage covers the client-side guard: an +// absent or whitespace-only -m never reaches the server at all. +func TestPushCmd_RequiresNonBlankMessage(t *testing.T) { + called := false + setupPushTest(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + })) + + for _, args := range [][]string{ + {"TASK-5"}, + {"TASK-5", "-m", ""}, + {"TASK-5", "-m", " "}, + {"TASK-5", "-m", "\n\t "}, + } { + called = false + cmd := pushCmd() + cmd.SetArgs(args) + err := cmd.Execute() + if err == nil { + t.Fatalf("args %v: expected an error for a blank message, got nil", args) + } + if called { + t.Fatalf("args %v: expected no HTTP call for a blank message", args) + } + } +} diff --git a/cmd/pad/cmd_watch.go b/cmd/pad/cmd_watch.go index c604c163..797d5e3d 100644 --- a/cmd/pad/cmd_watch.go +++ b/cmd/pad/cmd_watch.go @@ -69,8 +69,8 @@ type watchStreamPayload struct { // formatMonitorLine renders one notification as the exact stdout-line // contract `pad watch --stream --for-session` promises (DOC-2479): -// "PAD TASK-214 → done (Dave): fix verified" — one line, facts only, no -// etiquette prose (that lives in the plugin skill per DR-4). +// "PAD demo/TASK-214 → done (Dave): fix verified" — one line, facts +// only, no etiquette prose (that lives in the plugin skill per DR-4). // // TASK-2533 plan interpretation: DOC-2479's example names a target // STATUS VALUE and reads as though it combines two facts (a status @@ -83,8 +83,26 @@ type watchStreamPayload struct { // (not a status value) as the arrow's target — it's the one thing every // kind (status-change / assignment / comment / the reserved ask) can // supply uniformly from the actual payload fields. +// +// The workspace slug prefix (IDEA-2544 Phase 1, dispatcher review round +// 2, codex P1) is UNIVERSAL — every kind, not push-specific — because +// the ambiguity it closes predates push: GET /api/v1/events/stream is +// user-scoped ACROSS every workspace the caller belongs to (a watch is +// personal, not workspace-scoped — see Store.ListWatchesForUser), so +// ANY kind can arrive for a workspace other than the one linked in the +// caller's cwd, not just push. The payload already carried Workspace +// (DOC-2479's wire contract); it was simply never rendered. Dropping it +// silently means a session linked to workspace A that receives a +// notification for workspace B resolves the wrong item (or 404s) with +// no signal in the line that anything was off. Safe to change +// universally: formatMonitorLine's only consumer is this file's own +// fmt.Println (grepped plugin/ and skills/ for anything else parsing +// "PAD ..." lines — there is none; the Claude Code plugin host ingests +// the stdout line as free-text session-notification prose, not a +// structured format any code parses), so there is no wire-format +// consumer to break by adding a field. func formatMonitorLine(p watchStreamPayload) string { - return fmt.Sprintf("PAD %s → %s (%s): %s", p.ItemRef, p.Kind, p.Actor, p.Summary) + return fmt.Sprintf("PAD %s/%s → %s (%s): %s", p.Workspace, p.ItemRef, p.Kind, p.Actor, p.Summary) } // sleepOrDone waits for d or ctx cancellation, whichever comes first. diff --git a/cmd/pad/cmd_watch_test.go b/cmd/pad/cmd_watch_test.go index d3575e42..a9417f79 100644 --- a/cmd/pad/cmd_watch_test.go +++ b/cmd/pad/cmd_watch_test.go @@ -67,18 +67,28 @@ func TestFormatMonitorLine(t *testing.T) { }{ { name: "status change", - in: watchStreamPayload{ItemRef: "TASK-214", Kind: "status-change", Actor: "Dave", Summary: "open → done"}, - want: "PAD TASK-214 → status-change (Dave): open → done", + in: watchStreamPayload{Workspace: "demo", ItemRef: "TASK-214", Kind: "status-change", Actor: "Dave", Summary: "open → done"}, + want: "PAD demo/TASK-214 → status-change (Dave): open → done", }, { name: "assignment", - in: watchStreamPayload{ItemRef: "BUG-5", Kind: "assignment", Actor: "Alice", Summary: "assigned to Alice"}, - want: "PAD BUG-5 → assignment (Alice): assigned to Alice", + in: watchStreamPayload{Workspace: "demo", ItemRef: "BUG-5", Kind: "assignment", Actor: "Alice", Summary: "assigned to Alice"}, + want: "PAD demo/BUG-5 → assignment (Alice): assigned to Alice", }, { name: "comment", - in: watchStreamPayload{ItemRef: "TASK-1", Kind: "comment", Actor: "Bob", Summary: "fix verified"}, - want: "PAD TASK-1 → comment (Bob): fix verified", + in: watchStreamPayload{Workspace: "demo", ItemRef: "TASK-1", Kind: "comment", Actor: "Bob", Summary: "fix verified"}, + want: "PAD demo/TASK-1 → comment (Bob): fix verified", + }, + { + // IDEA-2544 Phase 1, dispatcher review round 2 (codex P1): the + // workspace prefix matters most here — push carries an + // instruction, so a caller resolving it against the wrong + // linked workspace is a worse failure mode than for a passive + // fact. + name: "push, different workspace than the item ref alone would suggest", + in: watchStreamPayload{Workspace: "other-workspace", ItemRef: "TASK-9", Kind: "push", Actor: "Dave", Summary: "triage this with the triage playbook"}, + want: "PAD other-workspace/TASK-9 → push (Dave): triage this with the triage playbook", }, } for _, c := range cases { diff --git a/cmd/pad/main.go b/cmd/pad/main.go index c5d8dddf..2b2e53e4 100644 --- a/cmd/pad/main.go +++ b/cmd/pad/main.go @@ -148,6 +148,7 @@ func newRootCmd() *cobra.Command { bootstrapCmd(), playbookCmd(), watchCmdGroup(), + pushCmd(), sessionCmd(), ) diff --git a/internal/cli/agent_identity_test.go b/internal/cli/agent_identity_test.go index 93fb2c40..7fd98bd2 100644 --- a/internal/cli/agent_identity_test.go +++ b/internal/cli/agent_identity_test.go @@ -135,6 +135,44 @@ func TestClientSendsResolvedAgentHeader(t *testing.T) { } } +// TestPushItemSendsResolvedAgentHeader (IDEA-2544 Phase 1, dispatcher +// review) pins that PushItem inherits X-Pad-Agent the same as every +// other mutating client method, rather than assuming: PushItem doesn't +// build its own request, it goes through c.post -> c.newRequest like +// CreateWatch and everything else, so the BUG-2542 fix upgraded it for +// free — but "the code path implies it" and "verified against a live +// request" are different claims, and only the second belongs in a +// report. Also confirms the server side of the loop: handlePushToItem's +// actor/actorName come from actorFromRequest/actorNameFromRequest, +// which read this exact header. +func TestPushItemSendsResolvedAgentHeader(t *testing.T) { + chdir(t, t.TempDir()) + for _, k := range []string{"PAD_AGENT", "CLAUDECODE"} { + t.Setenv(k, "") + os.Unsetenv(k) + } + t.Setenv("CLAUDECODE", "1") + + var got string + var seen bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got, seen = r.Header.Get("X-Pad-Agent"), true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-1","pushed":true}`)) + })) + defer srv.Close() + + if _, err := NewClientFromURL(srv.URL).PushItem("demo", "TASK-1", "triage this"); err != nil { + t.Fatalf("PushItem: %v", err) + } + if !seen { + t.Fatal("server never saw the request") + } + if got != "claude-code" { + t.Errorf("X-Pad-Agent = %q, want %q", got, "claude-code") + } +} + // ActorKind backs the one place the client must self-describe: structured // entries inside an item's fields JSON, which the server never parses. An // earlier revision of the BUG-2542 fix removed the CLI's hardcoded "user" diff --git a/internal/cli/client.go b/internal/cli/client.go index da0983e2..a1f41574 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -334,6 +334,29 @@ func (c *Client) DeleteWatch(wsSlug, itemSlug string) error { return c.delete("/workspaces/" + wsSlug + "/items/" + itemSlug + "/watch") } +// PushResult is the response body of a successful pad push, mirroring +// server.pushResponse's wire shape. Workspace is the CANONICAL slug the +// server resolved the push against (dispatcher review round 2, codex +// P1/P2) — a JSON consumer needs it because the notification stream is +// user-scoped across every workspace the caller belongs to, not just +// the one this call happened to target. +type PushResult struct { + Ref string `json:"ref"` + Workspace string `json:"workspace"` + Pushed bool `json:"pushed"` + Message string `json:"message"` +} + +// PushItem publishes a self-addressed push notification (IDEA-2544 +// Phase 1) on an item, over the same watch-events bus/stream `pad watch +// --stream --for-session` consumes. Transient, fire-and-forget — see +// server.handlePushToItem's doc comment for the no-durability rationale. +func (c *Client) PushItem(wsSlug, itemSlug, message string) (*PushResult, error) { + body := map[string]string{"message": message} + var result PushResult + return &result, c.post("/workspaces/"+wsSlug+"/items/"+itemSlug+"/push", body, &result) +} + // ListWatches returns every watch the current user holds, across all // workspaces they belong to (TASK-2533 — a watch is personal, not // workspace-scoped; see Store.ListWatchesForUser's doc comment). diff --git a/internal/server/handlers_push.go b/internal/server/handlers_push.go new file mode 100644 index 00000000..32ea9257 --- /dev/null +++ b/internal/server/handlers_push.go @@ -0,0 +1,151 @@ +package server + +import ( + "fmt" + "net/http" + "strings" + + "github.com/go-chi/chi/v5" + + "github.com/PerpetualSoftware/pad/internal/watchevents" +) + +// pushRequest is the body of POST .../items/{itemSlug}/push. +type pushRequest struct { + Message string `json:"message"` +} + +// pushResponse is the body of a successful push (dispatcher review round +// 2, codex P2: `pad push --format json` needs a real shape, not a +// discarded response). Workspace is included per the round-2 P1 fix's +// same rationale — the stream is user-scoped across every workspace the +// caller belongs to, so a JSON consumer needs it disambiguated same as +// the monitor line does. +type pushResponse struct { + Ref string `json:"ref"` + Workspace string `json:"workspace"` + Pushed bool `json:"pushed"` + Message string `json:"message"` +} + +// maxPushMessageLen bounds a push's instruction text, measured in runes +// AFTER whitespace collapse (dispatcher review round 1). Two +// constraints in tension set this: a push message is a free-form +// instruction, not a short label — truncating one the way +// truncateForSummary shortens a comment preview would silently corrupt +// what the user actually asked for, and unlike a comment (whose full +// body is still fetchable via `pad item show`), a push has no +// persistence to recover the untruncated text from (see +// handlePushToItem's doc comment). But Notification.Summary rides a +// single stdout line into a plugin monitor / terminal session (`pad +// watch --stream --for-session`'s one-line-per-event wire contract), so +// it can't be unbounded either. 4096 runes gives several paragraphs of +// headroom — comfortably more than any reasonable single instruction — +// while keeping that one line a sane size; a message over the cap is +// rejected with a 400 rather than silently truncated. +const maxPushMessageLen = 4096 + +// handlePushToItem publishes a self-addressed watchevents.KindPush +// notification (IDEA-2544 Phase 1) — the "push this to my agent" verb: +// an explicit, user-authored instruction bound to an item, delivered to +// every one of the pushing user's OWN connected monitor sessions via +// GET /api/v1/events/stream. Unlike watch/assignment notifications, +// this has no durable backing (Dave's product call: fire-and-forget is +// acceptable for v1 — no inbox, no "no session connected" warning; the +// bus's replay buffer is the only resilience a push gets). +// +// Self-addressed only: pushing into someone else's session is a consent +// question, not a code question (IDEA-2544 plan), so TargetUserID is +// always set to the CALLER's own ID, never a request-supplied target — +// there is no cross-user push in Phase 1. +// +// POST /api/v1/workspaces/{slug}/items/{itemSlug}/push +func (s *Server) handlePushToItem(w http.ResponseWriter, r *http.Request) { + if s.watchEvents == nil { + // Unlike handleCreateWatch (a durable store write that works fine + // without the bus), a push has NO persistence to fall back on — + // if there's no bus, the message is unrecoverably lost. Fail + // loudly here rather than returning 200 for an instruction that + // silently went nowhere. + writeError(w, http.StatusServiceUnavailable, "unavailable", "Push is not available") + return + } + + // Full workspace object, not just the ID (getWorkspaceID), so the + // response can echo the CANONICAL slug — the caller may have passed + // an ID in the URL, and pushResponse.Workspace exists specifically + // to disambiguate which workspace a JSON consumer should resolve the + // ref against (same rationale as the monitor line's workspace + // prefix), so it needs to be the real slug, not an echo of whatever + // the URL happened to contain. + ws, ok := s.getWorkspace(w, r) + if !ok { + return + } + workspaceID := ws.ID + + itemSlug := chi.URLParam(r, "itemSlug") + item, err := s.store.ResolveItem(workspaceID, itemSlug) + if err != nil { + writeInternalError(w, err) + return + } + if item == nil { + s.writeItemResolveError(w, r, workspaceID, itemSlug) + return + } + if !s.requireItemVisible(w, r, workspaceID, item) { + return + } + + userID := currentUserID(r) + if userID == "" { + writeError(w, http.StatusUnauthorized, "unauthorized", "Authentication required") + return + } + + var input pushRequest + if err := decodeJSON(r, &input); err != nil { + writeError(w, http.StatusBadRequest, "bad_request", err.Error()) + return + } + // Collapse newlines to spaces, matching truncateForSummary's + // rationale in handlers_comments.go: Notification.Summary is a + // single-line wire contract (`pad watch --stream --for-session` + // prints exactly one stdout line per event). Trimmed BEFORE the + // empty check so a whitespace-only -m (" ") is rejected the same + // as a genuinely empty one, rather than publishing a blank + // instruction. + message := strings.Join(strings.Fields(input.Message), " ") + if message == "" { + writeError(w, http.StatusBadRequest, "bad_request", "message must not be empty") + return + } + if length := len([]rune(message)); length > maxPushMessageLen { + writeError(w, http.StatusBadRequest, "bad_request", + fmt.Sprintf("message must be %d characters or fewer after whitespace collapse (got %d)", maxPushMessageLen, length)) + return + } + + actor, _ := actorFromRequest(r) + actorName := actorNameFromRequest(r) + + s.watchEvents.Publish(watchevents.Notification{ + WorkspaceID: workspaceID, + ItemID: item.ID, + CollectionID: item.CollectionID, + ItemRef: item.Ref, + Kind: watchevents.KindPush, + Actor: actor, + ActorName: actorName, + Summary: message, + TargetUserID: userID, + }) + + writeJSON(w, http.StatusOK, pushResponse{ + Ref: item.Ref, + Workspace: ws.Slug, + Pushed: true, + Message: message, + }) +} diff --git a/internal/server/handlers_push_test.go b/internal/server/handlers_push_test.go new file mode 100644 index 00000000..88c95d33 --- /dev/null +++ b/internal/server/handlers_push_test.go @@ -0,0 +1,193 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/watchevents" +) + +// TestPushToItem_RequiresMessage covers the 400-on-empty(-after-trim) +// validation: an absent message and a whitespace-only one are both +// rejected, so a blank push instruction can never reach the bus. +func TestPushToItem_RequiresMessage(t *testing.T) { + t.Parallel() + srv := testServerWithWatchEvents(t) + slug, item, tok, _ := setupWatchTestUser(t, srv) + + for _, body := range [][]byte{ + nil, + []byte(`{}`), + []byte(`{"message":""}`), + []byte(`{"message":" "}`), + []byte(`{"message":"\n\t "}`), + } { + rr := bearerCall(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token, body) + if rr.Code != http.StatusBadRequest { + t.Fatalf("body %q: expected 400, got %d (body: %s)", body, rr.Code, rr.Body.String()) + } + } +} + +// TestPushToItem_ResponseCarriesWorkspaceSlug covers the P2 finding +// (dispatcher review round 2, codex): a successful push's JSON response +// must carry ref/workspace/pushed/message so `pad push --format json` +// has something real to print, and workspace must be the CANONICAL +// slug (not merely whatever the URL happened to contain) — same +// disambiguation rationale as the monitor line's workspace prefix. +func TestPushToItem_ResponseCarriesWorkspaceSlug(t *testing.T) { + t.Parallel() + srv := testServerWithWatchEvents(t) + slug, item, tok, _ := setupWatchTestUser(t, srv) + + rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token, + map[string]interface{}{"message": "triage this"}) + if rr.Code != http.StatusOK { + t.Fatalf("push: %d %s", rr.Code, rr.Body.String()) + } + + var resp pushResponse + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("parse response: %v", err) + } + if resp.Ref != item.Ref { + t.Fatalf("expected ref %q, got %q", item.Ref, resp.Ref) + } + if resp.Workspace != slug { + t.Fatalf("expected workspace %q, got %q", slug, resp.Workspace) + } + if !resp.Pushed { + t.Fatal("expected pushed: true") + } + if resp.Message != "triage this" { + t.Fatalf("expected message %q, got %q", "triage this", resp.Message) + } +} + +// TestPushToItem_RejectsOverLongMessage covers the 400-over-cap +// validation (dispatcher review round 1): a message whose COLLAPSED +// length exceeds maxPushMessageLen is rejected rather than silently +// truncated — the message is the payload, not a preview, so truncation +// would corrupt what the user asked for. Built with extra internal +// whitespace so this also pins that the cap is measured AFTER +// whitespace collapse, not before (a message that's only over-length +// due to now-collapsed whitespace must NOT be rejected). +func TestPushToItem_RejectsOverLongMessage(t *testing.T) { + t.Parallel() + srv := testServerWithWatchEvents(t) + slug, item, tok, _ := setupWatchTestUser(t, srv) + + overLong := strings.Repeat("a \n ", maxPushMessageLen) // collapses to > maxPushMessageLen chars + rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token, + map[string]interface{}{"message": overLong}) + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for an over-cap message, got %d (body: %s)", rr.Code, rr.Body.String()) + } +} + +// TestPushToItem_MessageAtCapSurvivesIntact proves a message whose +// COLLAPSED length is exactly at maxPushMessageLen is accepted and +// delivered byte-for-byte (not silently truncated at the boundary) — +// the counterpart to the over-cap rejection test above. +func TestPushToItem_MessageAtCapSurvivesIntact(t *testing.T) { + t.Parallel() + srv := testServerWithWatchEvents(t) + slug, item, tok, _ := setupWatchTestUser(t, srv) + ts := httptest.NewServer(srv) + defer ts.Close() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ch := connectWatchStream(ctx, t, ts.URL, tok.Token) + waitForWatchEvent(t, ch, 3*time.Second) // connected + + atCap := strings.Repeat("a", maxPushMessageLen) // already whitespace-free: collapse is a no-op + rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token, + map[string]interface{}{"message": atCap}) + if rr.Code != http.StatusOK { + t.Fatalf("expected 200 for a message exactly at the cap, got %d (body: %s)", rr.Code, rr.Body.String()) + } + + ev := waitForWatchEvent(t, ch, 3*time.Second) + var payload watchEventPayload + if err := json.Unmarshal([]byte(ev.Data), &payload); err != nil { + t.Fatalf("parse payload: %v", err) + } + if payload.Summary != atCap { + t.Fatalf("expected the at-cap message to survive collapse+publish intact (len %d), got len %d", len(atCap), len(payload.Summary)) + } +} + +// TestPushToItem_UnavailableWithoutBus covers the 503-on-nil-bus guard: +// unlike a durable watch, a push has no persistence to fall back on, so +// a missing bus must fail loudly rather than silently losing the +// message. +func TestPushToItem_UnavailableWithoutBus(t *testing.T) { + t.Parallel() + srv := testServer(t) // NOT testServerWithWatchEvents — bus is nil + slug, item, tok, _ := setupWatchTestUser(t, srv) + + rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token, + map[string]interface{}{"message": "triage this"}) + if rr.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503, got %d (body: %s)", rr.Code, rr.Body.String()) + } +} + +// TestPushToItem_DeliversSelfAddressed mirrors +// TestWatchEventsStream_AddressedToYou_Assignment: a push must reach the +// SAME user's own connected stream with no explicit watch required, and +// carry the collapsed message as Summary. +func TestPushToItem_DeliversSelfAddressed(t *testing.T) { + t.Parallel() + srv := testServerWithWatchEvents(t) + slug, item, tok, _ := setupWatchTestUser(t, srv) + ts := httptest.NewServer(srv) + defer ts.Close() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ch := connectWatchStream(ctx, t, ts.URL, tok.Token) + waitForWatchEvent(t, ch, 3*time.Second) // connected + + rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/"+item.Slug+"/push", tok.Token, + map[string]interface{}{"message": "triage this\nwith the triage playbook"}) + if rr.Code != http.StatusOK { + t.Fatalf("push: %d %s", rr.Code, rr.Body.String()) + } + + ev := waitForWatchEvent(t, ch, 3*time.Second) + var payload watchEventPayload + if err := json.Unmarshal([]byte(ev.Data), &payload); err != nil { + t.Fatalf("parse payload: %v", err) + } + if payload.Kind != watchevents.KindPush { + t.Fatalf("expected kind %q, got %q", watchevents.KindPush, payload.Kind) + } + if payload.ItemRef != item.Ref { + t.Fatalf("expected item_ref %q, got %q", item.Ref, payload.ItemRef) + } + if payload.Summary != "triage this with the triage playbook" { + t.Fatalf("expected newline-collapsed summary, got %q", payload.Summary) + } +} + +// TestPushToItem_InvisibleItemDenied covers the same resolve/visibility +// gate every other item-scoped handler enforces: pushing a nonexistent +// ref 404s. +func TestPushToItem_InvisibleItemDenied(t *testing.T) { + t.Parallel() + srv := testServerWithWatchEvents(t) + slug, _, tok, _ := setupWatchTestUser(t, srv) + + rr := bearerJSON(t, srv, "POST", "/api/v1/workspaces/"+slug+"/items/NOPE-999/push", tok.Token, + map[string]interface{}{"message": "triage this"}) + if rr.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d (body: %s)", rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/handlers_watch_events.go b/internal/server/handlers_watch_events.go index 8d43151d..ee08b8ea 100644 --- a/internal/server/handlers_watch_events.go +++ b/internal/server/handlers_watch_events.go @@ -405,6 +405,30 @@ func watchNotificationVisible(watches map[string]string, vis watchAccessVisibili return true } + // Push (IDEA-2544 Phase 1) is semantically DIFFERENT from assignment, + // not just a same-shaped variant, and that difference is why this + // branch RETURNS here — for either outcome — instead of falling + // through like the assignment branch above does. Assignment is an + // item-level event: it's ordinary, expected behavior for someone who + // already holds an unconditional watch on the item to also see who + // it got assigned to (`pad watch --help` promises exactly that — any + // notification on a watched item fires). A push is addressed PRIVATE + // dispatch: one specific user putting an item + instruction in front + // of their OWN session, not a fact about the item that anyone + // watching it has a claim on. Watching an item must not leak a push + // someone else addressed to themselves — that's the whole point of + // TargetUserID gating it (dispatcher review round 2, codex P1: the + // original code fell through to the watch-map check on a + // non-matching TargetUserID, so any unconditional watcher on the item + // received every push addressed to every other user, instruction + // text included). Phase 4's session targeting is expected to inherit + // this same "addressed traffic is exclusive of watch-matched + // delivery" semantics, so it's pinned explicitly here rather than + // left to fall out of the shared TargetUserID field's shape. + if n.Kind == watchevents.KindPush { + return n.TargetUserID != "" && n.TargetUserID == userID + } + predicate, watched := watches[n.ItemID] if !watched { return false diff --git a/internal/server/handlers_watch_events_test.go b/internal/server/handlers_watch_events_test.go index c0b20c93..3e9473fb 100644 --- a/internal/server/handlers_watch_events_test.go +++ b/internal/server/handlers_watch_events_test.go @@ -571,6 +571,88 @@ func TestWatchNotificationVisible_AssignmentToSomeoneElseDenied(t *testing.T) { } } +// TestWatchNotificationVisible_PushToYou mirrors +// TestWatchNotificationVisible_AddressedToYouIgnoresWatchList for +// KindPush (IDEA-2544 Phase 1): a push fires purely off TargetUserID, +// independent of any watch. +func TestWatchNotificationVisible_PushToYou(t *testing.T) { + t.Parallel() + watches := map[string]string{} // no watches at all + n := watchevents.Notification{ItemID: "item-1", Kind: watchevents.KindPush, TargetUserID: "user-1"} + if !watchNotificationVisible(watches, watchAccessVisibility{fullAccess: true}, "user-1", n) { + t.Fatal("expected a push-to-you notification to be visible with no watch") + } +} + +// TestWatchNotificationVisible_PushToSomeoneElseDenied mirrors +// TestWatchNotificationVisible_AssignmentToSomeoneElseDenied for +// KindPush: Phase 1 only ever publishes self-addressed pushes, but the +// delivery rule itself must still deny a push addressed to a different +// user, exactly like the assignment branch. +func TestWatchNotificationVisible_PushToSomeoneElseDenied(t *testing.T) { + t.Parallel() + watches := map[string]string{} + n := watchevents.Notification{ItemID: "item-1", Kind: watchevents.KindPush, TargetUserID: "user-2"} + if watchNotificationVisible(watches, watchAccessVisibility{fullAccess: true}, "user-1", n) { + t.Fatal("expected a push addressed to someone else to be denied") + } +} + +// TestWatchNotificationVisible_PushToSomeoneElseDeniedEvenWithUnconditionalWatch +// covers codex round 1's P1 finding: the original push branch returned +// true only on a match and otherwise FELL THROUGH to the watch-map +// check below it — so a non-target caller holding an unconditional +// watch on the item still received the push (instruction text +// included), because an unconditional watch matches "any notification +// on this item". TestWatchNotificationVisible_PushToSomeoneElseDenied +// used an EMPTY watch map, which never exercised that fall-through path +// at all. Push must be exclusive of watch-matched delivery: watching an +// item grants no claim on private dispatch someone else addressed to +// their own session — see the doc comment on the push branch in +// watchNotificationVisible. +func TestWatchNotificationVisible_PushToSomeoneElseDeniedEvenWithUnconditionalWatch(t *testing.T) { + t.Parallel() + watches := map[string]string{"item-1": ""} // unconditional watch on the item + n := watchevents.Notification{ItemID: "item-1", Kind: watchevents.KindPush, TargetUserID: "user-2"} + if watchNotificationVisible(watches, watchAccessVisibility{fullAccess: true}, "user-1", n) { + t.Fatal("expected a push addressed to someone else to be denied even when the caller holds an unconditional watch on the item") + } +} + +// TestWatchNotificationVisible_PushToSomeoneElseDeniedEvenWithPredicateWatch +// is the predicated-watch variant of the same fall-through blind spot — +// same shape, a non-empty predicate instead of an unconditional watch. +func TestWatchNotificationVisible_PushToSomeoneElseDeniedEvenWithPredicateWatch(t *testing.T) { + t.Parallel() + watches := map[string]string{"item-1": "status=done"} + n := watchevents.Notification{ItemID: "item-1", Kind: watchevents.KindPush, TargetUserID: "user-2"} + if watchNotificationVisible(watches, watchAccessVisibility{fullAccess: true}, "user-1", n) { + t.Fatal("expected a push addressed to someone else to be denied even when the caller holds a predicated watch on the item") + } +} + +// TestWatchNotificationVisible_PushStillGatedByAccess mirrors +// TestWatchNotificationVisible_AddressedToYouStillGatedByAccess: the +// SAME current-access check that gates every other kind must also gate +// a push-to-you notification, not just the assignment branch. +func TestWatchNotificationVisible_PushStillGatedByAccess(t *testing.T) { + t.Parallel() + watches := map[string]string{} + deny := watchAccessVisibility{} // zero value: nothing visible + n := watchevents.Notification{ + ItemID: "item-1", CollectionID: "coll-1", + Kind: watchevents.KindPush, TargetUserID: "user-1", + } + if watchNotificationVisible(watches, deny, "user-1", n) { + t.Fatal("expected a push-to-you notification to be denied when the caller has no current access to the item's collection") + } + + allow := watchAccessVisibility{visibleCollIDs: map[string]bool{"coll-1": true}} + if !watchNotificationVisible(watches, allow, "user-1", n) { + t.Fatal("expected a push-to-you notification to be visible once the caller has current access to the item's collection") + } +} + // TestWatchNotificationVisible_AddressedToYouStillGatedByAccess covers // TASK-2533 codex round 2 finding 2: the addressed-to-you branch used to // return true unconditionally, with NO access check — an item assigned diff --git a/internal/server/server.go b/internal/server/server.go index 9303f3fc..fbaec9cc 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1561,6 +1561,11 @@ func (s *Server) setupRouter() { // pipeline. `pad watch ` / `pad watch remove `. r.Post("/watch", s.handleCreateWatch) r.Delete("/watch", s.handleDeleteWatch) + // Push (IDEA-2544 Phase 1): transient, self-addressed + // human→harness dispatch over the SAME watch-events + // bus/stream — no durable row, see handlePushToItem's + // doc comment. `pad push -m "message"`. + r.Post("/push", s.handlePushToItem) }) // Links (v2) diff --git a/internal/watchevents/watchevents.go b/internal/watchevents/watchevents.go index 61e97b61..5cf4fd43 100644 --- a/internal/watchevents/watchevents.go +++ b/internal/watchevents/watchevents.go @@ -42,6 +42,15 @@ const ( // enum so the wire contract doesn't have to change when a producer // eventually exists. KindAsk = "ask" + // KindPush is IDEA-2544 Phase 1's human→harness addressed-dispatch + // event: POST .../items/{itemSlug}/push publishes exactly one of + // these, self-addressed (TargetUserID == the pushing user), any time + // a user wants to put an item + instruction in front of their own + // harness right now rather than waiting on assignment/watch + // semantics. KindAsk is push's reserved sibling in the other + // direction (harness→human) — see TargetUserID's doc comment for the + // shared envelope shape the two are meant to converge on. + KindPush = "push" ) // Notification is one watch-worthy fact: an item's status changed, it was @@ -74,6 +83,21 @@ type Notification struct { // addressed-to-you check compares this directly against the // connected caller's user ID — see server.sseWatchVisible. AssignedUserID string + // TargetUserID is IDEA-2544's generalized addressed-to field: + // populated on Kind == KindPush with the user the push is addressed + // to (Phase 1 always sets this to the pushing user's own ID — + // self-addressed only, per Dave's product call that pushing into + // someone else's session is a consent question, not a code + // question). watchNotificationVisible compares this directly against + // the connected caller's user ID, exactly like AssignedUserID's role + // for KindAssignment. Reserved KindAsk is expected to share this same + // field (harness→human addressed traffic) rather than growing its + // own, once it has a producer. Deliberately NOT part of + // watchEventPayload's wire shape (server.go) — it exists purely to + // gate delivery server-side; a client never needs to know who else a + // notification could have been addressed to, and echoing it back + // would leak a user ID for no consumer that needs it. + TargetUserID string // StatusFieldKey / ToStatus are populated on Kind == KindStatusChange, // mirroring models.ItemMutationSignal, so the `--until field=value` // watch predicate can be evaluated against a Notification directly diff --git a/internal/watchevents/watchevents_test.go b/internal/watchevents/watchevents_test.go index c7d60f7d..b371a9ae 100644 --- a/internal/watchevents/watchevents_test.go +++ b/internal/watchevents/watchevents_test.go @@ -30,6 +30,41 @@ func TestMemoryBus_PublishDeliversToSubscriber(t *testing.T) { } } +// TestMemoryBus_PublishDeliversPushWithTargetUserID covers IDEA-2544 +// Phase 1: KindPush and TargetUserID must round-trip through Publish +// like any other kind/field — the bus itself is kind-agnostic, but this +// pins that a new kind + a new addressed-to field don't need any bus +// changes to work, matching the plan's "zero formatter/bus changes" +// claim for a new kind. +func TestMemoryBus_PublishDeliversPushWithTargetUserID(t *testing.T) { + t.Parallel() + b := New() + defer b.Close() + + ch := b.Subscribe() + defer b.Unsubscribe(ch) + + b.Publish(Notification{ + WorkspaceID: "ws1", + ItemID: "item1", + Kind: KindPush, + Summary: "triage this", + TargetUserID: "user-1", + }) + + select { + case n := <-ch: + if n.Kind != KindPush { + t.Fatalf("expected kind %q, got %q", KindPush, n.Kind) + } + if n.TargetUserID != "user-1" { + t.Fatalf("expected TargetUserID %q, got %q", "user-1", n.TargetUserID) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for notification") + } +} + func TestMemoryBus_UnsubscribeClosesChannel(t *testing.T) { t.Parallel() b := New() diff --git a/plugin/monitors/monitors.json b/plugin/monitors/monitors.json index d6081216..e9f2da8a 100644 --- a/plugin/monitors/monitors.json +++ b/plugin/monitors/monitors.json @@ -2,6 +2,6 @@ { "name": "pad-events", "command": "while ! pad watch --help >/dev/null 2>&1; do sleep 3600; done; exec pad watch --stream --for-session", - "description": "Pad item events: explicit subscriptions (pad watch ) and items addressed to you (currently: assignment — ask-events are reserved in the wire contract but not yet emitted). Silent when the pad CLI isn't installed, no .pad.toml is present, or padd is unreachable." + "description": "Pad item events: explicit subscriptions (pad watch ), items addressed to you (assignment, push — see pad push -m; ask-events are reserved in the wire contract but not yet emitted). Silent when the pad CLI isn't installed, no .pad.toml is present, or padd is unreachable." } ] diff --git a/plugin/skills/pad/SKILL.md b/plugin/skills/pad/SKILL.md index 641af38a..abcf24a7 100644 --- a/plugin/skills/pad/SKILL.md +++ b/plugin/skills/pad/SKILL.md @@ -328,10 +328,11 @@ If the user's intent doesn't match any pattern above, respond helpfully. You can ## When a Pad notification arrives (plugin monitor) -This plugin runs a background monitor that delivers Pad item events as session notifications — your explicit watches (`pad watch `) and items addressed to you (assignments, asks for your role). Etiquette when one fires: +This plugin runs a background monitor that delivers Pad item events as session notifications — your explicit watches (`pad watch `), items addressed to you (assignments, asks for your role), and pushes (`pad push `). Etiquette when one fires: -- **Default: read-only, one line, park.** A notification is context, not a command. Say what happened in one line — "Pad: TASK-214 was closed by Dave" — and continue whatever the user was already doing. This is the default for every notification from an explicit watch (`pad watch `), and for any addressed-to-you event you aren't certain about. +- **Push is the first of two exceptions to the never-write rule below — read it first if this bullet is confusing on its own.** A push is user-authored and harness-addressed: someone deliberately put this item in front of *this* session right now, not a passive fact to note and park. The read-only/park default is lifted for it specifically: load the item first — **using the workspace slug from the notification line** (`PAD / → push (...): ...`), i.e. `pad --workspace item show `, not a bare `pad item show ` — for full context, then do what the message says. The monitor stream is user-scoped across every workspace you belong to, so a push can arrive for a DIFFERENT workspace than the one linked in this session's cwd; resolving against the wrong one silently loads (or 404s on) the wrong item. Like the second exception below, this lifts the *never-write* rule only — confirm-first (Key Principles #3) still applies to destructive operations, exactly as for any other item mutation. +- **Default: read-only, one line, park.** A notification is context, not a command. Say what happened in one line — "Pad: TASK-214 was closed by Dave" — and continue whatever the user was already doing. This is the default for every notification from an explicit watch (`pad watch `), and for any addressed-to-you event you aren't certain about (push excepted — see above). - **Never write to Pad just because a notification fired.** Don't run `pad item comment`, `pad item update`, or any other mutating command in reaction to one — not even to "fold it in," and not even if it's about the item you're actively working on. Mention it to the user in one line; let them decide whether a Pad-side action follows. -- **The one narrow exception:** a write action is permitted only when the event is an assignment or an ask *explicitly addressed to the session's current user* (not merely a watched item), **and** acting on it immediately is unambiguously what the user would expect. This exception lifts the *never-write* rule only — it does not lift confirm-first (Key Principles #3): show what you're about to write and get confirmation, exactly as for any other item mutation, unless the workspace's active conventions explicitly opt into autonomous capture (same rule the `/pad:capture` skill follows). When in doubt, park — the default always wins. +- **The second exception:** a write action is also permitted when the event is an assignment or an ask *explicitly addressed to the session's current user* (not merely a watched item), **and** acting on it immediately is unambiguously what the user would expect. Narrower than push's exception above — an assignment/ask doesn't itself carry an instruction, so this exception only lifts the *never-write* rule when the right action is genuinely unambiguous; it does not lift confirm-first (Key Principles #3): show what you're about to write and get confirmation, exactly as for any other item mutation, unless the workspace's active conventions explicitly opt into autonomous capture (same rule the `/pad:capture` skill follows). When in doubt, park — the default always wins. - **Never start new unrequested work from a notification.** Offer it as a follow-up at a natural pause instead. - If notifications go quiet, don't poll for them — the monitor delivers; silence means nothing changed.