From f03523240470aaf76ab42cf2de3b515e09a96837 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 17 Jul 2026 18:03:55 -0700 Subject: [PATCH 1/5] Fix todos update silently clearing completion subscribers (#538) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The BC3 todos PUT has replace semantics: any field omitted from the body is cleared. Neither branch of todos update included completion_subscriber_ids, so every update wiped the todo's "When done, notify" list. The #412/#413 read-modify-write merge couldn't preserve it because the SDK's Todo model drops completion_subscribers in todoFromGenerated (basecamp/basecamp-sdk#355) — the merge had nothing to carry forward. Preservation now works via a raw GET of the flat /todos/{id}.json route (which serves the field the SDK model drops), feeding the current subscriber ids into both the typed-merge and raw-clear PUT paths. The read fails closed: an HTTP error, malformed JSON, or a response missing the completion_subscribers key aborts the command before any PUT rather than risking a silent clear. The helper is commented as temporary and goes away once the SDK round-trips the field. Alongside the fix, subscribers become directly editable: - todos create/update --notify-on-completion sets them (comma-separated, with people-name tab completion) - todos update --no-notify-on-completion clears them (by omission — explicit intent skips the preservation read entirely) Unit tests cover preservation in both branches, the flat-route assertion, explicit set/clear bypassing the read, the fail-closed matrix (missing key / malformed JSON / HTTP 500 in each branch), flag conflicts, and the create body. A live smoke test exercises the full sequence: create with subscriber, title-only update preserves, --no-due preserves, --no-notify-on-completion clears. .surface gains the three new flag records by hand (verified identical to fresh generation); a full regeneration is deferred because the snapshot has unrelated drift since #499 changed the generator script. --- .surface | 3 + e2e/smoke/smoke_todos_write.bats | 49 +++++++++ internal/commands/todos.go | 107 +++++++++++++++++++- internal/commands/todos_test.go | 165 ++++++++++++++++++++++++++++++- skills/basecamp/SKILL.md | 8 ++ 5 files changed, 326 insertions(+), 6 deletions(-) diff --git a/.surface b/.surface index cd83c7492..a42b363f1 100644 --- a/.surface +++ b/.surface @@ -13091,6 +13091,7 @@ FLAG basecamp todos create --markdown type=bool FLAG basecamp todos create --md type=bool FLAG basecamp todos create --no-hints type=bool FLAG basecamp todos create --no-stats type=bool +FLAG basecamp todos create --notify-on-completion type=string FLAG basecamp todos create --profile type=string FLAG basecamp todos create --project type=string FLAG basecamp todos create --quiet type=bool @@ -13359,9 +13360,11 @@ FLAG basecamp todos update --md type=bool FLAG basecamp todos update --no-description type=bool FLAG basecamp todos update --no-due type=bool FLAG basecamp todos update --no-hints type=bool +FLAG basecamp todos update --no-notify-on-completion type=bool FLAG basecamp todos update --no-starts-on type=bool FLAG basecamp todos update --no-stats type=bool FLAG basecamp todos update --notify type=bool +FLAG basecamp todos update --notify-on-completion type=string FLAG basecamp todos update --profile type=string FLAG basecamp todos update --project type=string FLAG basecamp todos update --quiet type=bool diff --git a/e2e/smoke/smoke_todos_write.bats b/e2e/smoke/smoke_todos_write.bats index 752dba34e..56ad9db1a 100644 --- a/e2e/smoke/smoke_todos_write.bats +++ b/e2e/smoke/smoke_todos_write.bats @@ -138,6 +138,55 @@ setup_file() { run_smoke basecamp todos trash "$todo_id" -p "$QA_PROJECT" --json } +@test "todos update preserves completion subscribers" { + # Own person ID for use as a completion subscriber + run_smoke basecamp api get /my/profile.json --json + assert_success + local person_id + person_id=$(echo "$output" | jq -r '.data.id') + [[ -n "$person_id" && "$person_id" != "null" ]] || mark_unverifiable "Cannot resolve own person ID" + + # Create with a due date and a completion subscriber + run_smoke basecamp todos create "Subscriber preservation test $(date +%s)" \ + --list "$QA_TODOLIST" -p "$QA_PROJECT" \ + --due "2026-08-15" \ + --notify-on-completion "$person_id" --json + assert_success + local todo_id + todo_id=$(echo "$output" | jq -r '.data.id') + + # Title-only update (typed merge branch) must preserve the subscriber + run_smoke basecamp todos update "$todo_id" --title "Renamed, subscriber kept" --json + assert_success + + run_smoke basecamp api get "/todos/$todo_id.json" --json + assert_success + local subs + subs=$(echo "$output" | jq -r '[.data.completion_subscribers[].id] | join(",")') + [[ "$subs" == "$person_id" ]] || { echo "subscriber lost after title update: [$subs]"; return 1; } + + # --no-due (raw clear branch) must also preserve the subscriber + run_smoke basecamp todos update "$todo_id" --no-due --json + assert_success + + run_smoke basecamp api get "/todos/$todo_id.json" --json + assert_success + subs=$(echo "$output" | jq -r '[.data.completion_subscribers[].id] | join(",")') + [[ "$subs" == "$person_id" ]] || { echo "subscriber lost after --no-due: [$subs]"; return 1; } + + # --no-notify-on-completion clears the subscribers + run_smoke basecamp todos update "$todo_id" --no-notify-on-completion --json + assert_success + + run_smoke basecamp api get "/todos/$todo_id.json" --json + assert_success + subs=$(echo "$output" | jq -r '.data.completion_subscribers | length') + [[ "$subs" == "0" ]] || { echo "subscribers not cleared: $subs remain"; return 1; } + + # Clean up + run_smoke basecamp todos trash "$todo_id" -p "$QA_PROJECT" --json +} + @test "todos trash trashes a todo" { local id_file="$BATS_FILE_TMPDIR/direct_todo_id" [[ -f "$id_file" ]] || mark_unverifiable "No direct todo created in prior test" diff --git a/internal/commands/todos.go b/internal/commands/todos.go index ccc055ac1..cae5bd20d 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -766,6 +766,7 @@ func newTodosCreateCmd() *cobra.Command { var due string var description string var attachFiles []string + var notifyOnCompletion string cmd := &cobra.Command{ Use: "create ", @@ -881,6 +882,13 @@ func newTodosCreateCmd() *cobra.Command { assigneeIDInt, _ := strconv.ParseInt(assigneeID, 10, 64) req.AssigneeIDs = []int64{assigneeIDInt} } + if strings.TrimSpace(notifyOnCompletion) != "" { + subscriberIDs, err := resolveAssigneeIDs(cmd.Context(), app, notifyOnCompletion) + if err != nil { + return err + } + req.CompletionSubscriberIDs = subscriberIDs + } todolistID, err := strconv.ParseInt(resolvedTodolist, 10, 64) if err != nil { @@ -925,6 +933,7 @@ func newTodosCreateCmd() *cobra.Command { cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (YYYY-MM-DD)") cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + cmd.Flags().StringVar(¬ifyOnCompletion, "notify-on-completion", "", "People to notify when done (names or IDs, comma-separated)") // Register tab completion for flags completer := completion.NewCompleter(nil) @@ -932,6 +941,7 @@ func newTodosCreateCmd() *cobra.Command { _ = cmd.RegisterFlagCompletionFunc("in", completer.ProjectNameCompletion()) _ = cmd.RegisterFlagCompletionFunc("assignee", completer.PeopleNameCompletion()) _ = cmd.RegisterFlagCompletionFunc("to", completer.PeopleNameCompletion()) + _ = cmd.RegisterFlagCompletionFunc("notify-on-completion", completer.PeopleNameCompletion()) return cmd } @@ -946,6 +956,8 @@ func newTodosUpdateCmd() *cobra.Command { var noDue bool var noStartsOn bool var noDescription bool + var notifyOnCompletion string + var noNotifyOnCompletion bool cmd := &cobra.Command{ Use: "update [title]", @@ -961,7 +973,11 @@ You can pass either a todo ID or a Basecamp URL: Clear a field by passing its --no- flag or an empty value: basecamp todos update 789 --no-due basecamp todos update 789 --due "" - basecamp todos update 789 --no-description`, + basecamp todos update 789 --no-description + +Set or clear the people notified when the todo is completed: + basecamp todos update 789 --notify-on-completion "Jane Smith,Bob" + basecamp todos update 789 --no-notify-on-completion`, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 0 { return missingArg(cmd, "") @@ -977,11 +993,17 @@ Clear a field by passing its --no- flag or an empty value: if noDescription && strings.TrimSpace(description) != "" { return output.ErrUsage("--no-description and --description cannot be used together") } + if noNotifyOnCompletion && strings.TrimSpace(notifyOnCompletion) != "" { + return output.ErrUsage("--no-notify-on-completion and --notify-on-completion cannot be used together") + } // Detect clear intent: explicit --no-X flag or empty value via --X "" clearDue := noDue || (cmd.Flags().Changed("due") && strings.TrimSpace(due) == "") clearStarts := noStartsOn || (cmd.Flags().Changed("starts-on") && strings.TrimSpace(startsOn) == "") clearDescription := noDescription || (cmd.Flags().Changed("description") && strings.TrimSpace(description) == "") needsClear := clearDue || clearStarts || clearDescription + // Subscriber clearing works by omission in both branches, so it + // doesn't force the raw-PUT clear branch. + clearSubscribers := noNotifyOnCompletion || (cmd.Flags().Changed("notify-on-completion") && strings.TrimSpace(notifyOnCompletion) == "") // Clearing due while setting starts is contradictory (Basecamp enforces starts <= due) if clearDue && strings.TrimSpace(startsOn) != "" { @@ -999,12 +1021,13 @@ Clear a field by passing its --no- flag or an empty value: // No-op guard: at least one effective field required assigneeChanged := (cmd.Flags().Changed("assignee") || cmd.Flags().Changed("to")) && strings.TrimSpace(assignee) != "" + subscribersChanged := cmd.Flags().Changed("notify-on-completion") && strings.TrimSpace(notifyOnCompletion) != "" if strings.TrimSpace(effectiveTitle) == "" && strings.TrimSpace(description) == "" && strings.TrimSpace(due) == "" && strings.TrimSpace(startsOn) == "" && - !assigneeChanged && + !assigneeChanged && !subscribersChanged && (!cmd.Flags().Changed("notify") || !notify) && - !needsClear { + !needsClear && !clearSubscribers { return noChanges(cmd) } @@ -1100,6 +1123,26 @@ Clear a field by passing its --no- flag or an empty value: body["assignee_ids"] = ids } + // Completion subscribers: the SDK's Todo model drops them, so + // existingTodo can't supply them — set explicitly, clear by + // omission, or preserve via a raw read that must succeed + // before any PUT (fail closed). + if subscribersChanged { + subscriberIDs, err := resolveAssigneeIDs(cmd.Context(), app, notifyOnCompletion) + if err != nil { + return err + } + body["completion_subscriber_ids"] = subscriberIDs + } else if !clearSubscribers { + subscriberIDs, err := completionSubscriberIDs(cmd.Context(), app, todoID) + if err != nil { + return err + } + if len(subscriberIDs) > 0 { + body["completion_subscriber_ids"] = subscriberIDs + } + } + if cmd.Flags().Changed("notify") && notify { body["notify"] = true } @@ -1166,6 +1209,25 @@ Clear a field by passing its --no- flag or an empty value: } req.AssigneeIDs = assigneeIDs } + // Completion subscribers: the SDK's Todo model drops them, so + // existingTodo can't supply them — set explicitly, clear by + // omission (nil), or preserve via a raw read that must succeed + // before any PUT (fail closed). + if subscribersChanged { + subscriberIDs, err := resolveAssigneeIDs(cmd.Context(), app, notifyOnCompletion) + if err != nil { + return err + } + req.CompletionSubscriberIDs = subscriberIDs + } else if !clearSubscribers { + subscriberIDs, err := completionSubscriberIDs(cmd.Context(), app, todoID) + if err != nil { + return err + } + if len(subscriberIDs) > 0 { + req.CompletionSubscriberIDs = subscriberIDs + } + } if cmd.Flags().Changed("notify") && notify { req.Notify = true } @@ -1205,15 +1267,52 @@ Clear a field by passing its --no- flag or an empty value: cmd.Flags().BoolVar(&noDue, "no-due", false, "Clear the due date") cmd.Flags().BoolVar(&noStartsOn, "no-starts-on", false, "Clear the start date") cmd.Flags().BoolVar(&noDescription, "no-description", false, "Clear the description") + cmd.Flags().StringVar(¬ifyOnCompletion, "notify-on-completion", "", "People to notify when done (names or IDs, comma-separated)") + cmd.Flags().BoolVar(&noNotifyOnCompletion, "no-notify-on-completion", false, "Clear the people notified when done") - // Register tab completion for assignee flags + // Register tab completion for people flags completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("assignee", completer.PeopleNameCompletion()) _ = cmd.RegisterFlagCompletionFunc("to", completer.PeopleNameCompletion()) + _ = cmd.RegisterFlagCompletionFunc("notify-on-completion", completer.PeopleNameCompletion()) return cmd } +// completionSubscriberIDs reads the current completion subscriber ids via a raw +// GET of /todos/{id}.json. The SDK's Todo model drops completion_subscribers in +// todoFromGenerated (basecamp/basecamp-sdk#355), so a raw read is the only way +// to preserve them across the replace-semantics PUT (#538). Delete once the SDK +// round-trips the field. +// +// Fails closed: a missing or null completion_subscribers key, a malformed +// response, or a failed GET all return an error — proceeding without knowing +// the current subscribers would silently clear them. +func completionSubscriberIDs(ctx context.Context, app *appctx.App, todoID int64) ([]int64, error) { + resp, err := app.Account().Get(ctx, fmt.Sprintf("/todos/%d.json", todoID)) + if err != nil { + return nil, fmt.Errorf("failed to read current completion subscribers for todo %d: %w", todoID, err) + } + + var raw struct { + CompletionSubscribers *[]struct { + ID int64 `json:"id"` + } `json:"completion_subscribers"` + } + if err := resp.UnmarshalData(&raw); err != nil { + return nil, fmt.Errorf("failed to parse completion subscribers for todo %d: %w", todoID, err) + } + if raw.CompletionSubscribers == nil { + return nil, fmt.Errorf("cannot verify current completion subscribers for todo %d: response missing completion_subscribers", todoID) + } + + ids := make([]int64, len(*raw.CompletionSubscribers)) + for i, s := range *raw.CompletionSubscribers { + ids[i] = s.ID + } + return ids, nil +} + func newTodosCompleteCmd() *cobra.Command { cmd := &cobra.Command{ Use: "complete ...", diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 82763f6c4..573e5334b 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -405,7 +405,7 @@ func TestTodosCreateContentIsPlainText(t *testing.T) { cmd := NewTodosCmd() plainTextContent := "Fix the authentication bug" - err := executeTodosCommand(cmd, app, "create", plainTextContent) + err := executeTodosCommand(cmd, app, "create", plainTextContent, "--notify-on-completion", "7,8") require.NoError(t, err, "command should succeed with mock transport") require.NotEmpty(t, transport.capturedBody, "expected request body to be captured") @@ -419,6 +419,9 @@ func TestTodosCreateContentIsPlainText(t *testing.T) { // The content should be exactly what was passed in - plain text, no HTML wrapping assert.Equal(t, plainTextContent, content, "Todo content should be plain text, not HTML-wrapped") + + assert.Equal(t, []any{float64(7), float64(8)}, requestBody["completion_subscriber_ids"], + "--notify-on-completion must map to completion_subscriber_ids") } func TestTodosListAssigneeWithoutProjectErrors(t *testing.T) { @@ -1532,15 +1535,43 @@ func TestSweepCommentLocalImageErrors(t *testing.T) { // Todos Update Tests // ============================================================================= -// mockTodoUpdateTransport handles GET and PUT for todo update tests. +// mockTodoUpdateTransport handles GET and PUT for todo update tests. It +// records every request as "METHOD path" so tests can assert which routes +// were hit and in what order. The raw preservation GET (flat route +// /todos/{id}.json, distinct from the typed generated route /todos/{id}) +// serves a configurable body/status so tests can exercise the fail-closed +// contract. type mockTodoUpdateTransport struct { capturedBody []byte + requests []string + + rawGetStatus int // 0 → 200 + rawGetBody string // "" → default with completion_subscribers [7, 8] } func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.requests = append(t.requests, req.Method+" "+req.URL.Path) + header := make(http.Header) header.Set("Content-Type", "application/json") + if req.Method == "GET" && strings.HasSuffix(req.URL.Path, ".json") { + // Raw preservation GET of the flat /todos/{id}.json route. + status := t.rawGetStatus + if status == 0 { + status = 200 + } + body := t.rawGetBody + if body == "" { + body = `{"id": 999, "completion_subscribers": [{"id": 7}, {"id": 8}]}` + } + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), + Header: header, + }, nil + } + if req.Method == "GET" { mockTodo := `{"id": 999, "title": "Test", "content": "Test todo", "status": "active", "completed": false, "description": "Existing desc", "due_on": "2026-04-01", "starts_on": "2026-03-25", "bucket": {"id": 456, "name": "Test Project", "type": "Project"}, "assignees": [{"id": 42, "name": "Test User"}]}` return &http.Response{ @@ -1570,6 +1601,17 @@ func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, return nil, errors.New("unexpected request") } +// hasRequest reports whether any recorded request matches the given method +// and path predicate. +func (t *mockTodoUpdateTransport) hasRequest(method, pathSubstr string) bool { + for _, r := range t.requests { + if strings.HasPrefix(r, method+" ") && strings.Contains(r, pathSubstr) { + return true + } + } + return false +} + func setupTodoUpdateApp(t *testing.T, transport http.RoundTripper) *appctx.App { t.Helper() t.Setenv("BASECAMP_NO_KEYRING", "1") @@ -1924,6 +1966,9 @@ func TestTodosUpdateDueDatePreservesExistingFields(t *testing.T) { require.True(t, ok, "assignee_ids must be preserved") require.Len(t, ids, 1) assert.Equal(t, float64(42), ids[0]) + + assert.Equal(t, []any{float64(7), float64(8)}, body["completion_subscriber_ids"], + "completion subscribers must be preserved") } func TestTodosUpdateTitlePreservesExistingFields(t *testing.T) { @@ -1943,6 +1988,122 @@ func TestTodosUpdateTitlePreservesExistingFields(t *testing.T) { assert.Equal(t, "Existing desc", body["description"]) assert.Equal(t, "2026-04-01", body["due_on"]) assert.Equal(t, "2026-03-25", body["starts_on"]) + assert.Equal(t, []any{float64(7), float64(8)}, body["completion_subscriber_ids"], + "completion subscribers must be preserved") +} + +func TestTodosUpdatePreservationReadUsesFlatRoute(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "New title") + require.NoError(t, err) + + assert.True(t, transport.hasRequest("GET", "/todos/999.json"), + "preservation read must hit the flat /todos/{id}.json route, got: %v", transport.requests) + for _, r := range transport.requests { + assert.NotContains(t, r, "/buckets/", "no bucket-scoped request expected") + } +} + +func TestTodosUpdateExplicitSubscribersSkipsPreservationRead(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "--notify-on-completion", "42") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + err = json.Unmarshal(transport.capturedBody, &body) + require.NoError(t, err) + + assert.Equal(t, []any{float64(42)}, body["completion_subscriber_ids"]) + assert.False(t, transport.hasRequest("GET", ".json"), + "explicit --notify-on-completion must not trigger a preservation read, got: %v", transport.requests) +} + +func TestTodosUpdateNoNotifyOnCompletionClearsSubscribers(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "--no-notify-on-completion") + require.NoError(t, err, "--no-notify-on-completion alone must pass the no-op guard") + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + err = json.Unmarshal(transport.capturedBody, &body) + require.NoError(t, err) + + _, exists := body["completion_subscriber_ids"] + assert.False(t, exists, "completion_subscriber_ids must be omitted to clear") + assert.False(t, transport.hasRequest("GET", ".json"), + "clearing subscribers must not trigger a preservation read, got: %v", transport.requests) +} + +func TestTodosUpdateNoDuePreservesSubscribers(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "--no-due") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + err = json.Unmarshal(transport.capturedBody, &body) + require.NoError(t, err) + + assert.Equal(t, []any{float64(7), float64(8)}, body["completion_subscriber_ids"], + "completion subscribers must be preserved in the clear branch") +} + +func TestTodosUpdateSubscriberReadFailsClosed(t *testing.T) { + // A preservation read that fails — or succeeds without an unambiguous + // completion_subscribers key — must abort the update before any PUT. + cases := map[string]struct { + rawGetStatus int + rawGetBody string + }{ + "missing key": {rawGetBody: `{"id": 999}`}, + "malformed json": {rawGetBody: `{not json`}, + "http error": {rawGetStatus: 500, rawGetBody: `{}`}, + } + branches := map[string][]string{ + "merge branch": {"update", "999", "New title"}, + "clear branch": {"update", "999", "--no-due"}, + } + + for branchName, cmdArgs := range branches { + for caseName, tc := range cases { + t.Run(branchName+"/"+caseName, func(t *testing.T) { + transport := &mockTodoUpdateTransport{ + rawGetStatus: tc.rawGetStatus, + rawGetBody: tc.rawGetBody, + } + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, cmdArgs...) + require.Error(t, err) + assert.Contains(t, err.Error(), "completion subscribers") + assert.False(t, transport.hasRequest("PUT", "/"), + "no PUT may occur when the preservation read fails, got: %v", transport.requests) + }) + } + } +} + +func TestTodosUpdateConflictingNotifyOnCompletionFlags(t *testing.T) { + app, _ := setupTodosTestApp(t) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "--no-notify-on-completion", "--notify-on-completion", "42") + require.Error(t, err) + assert.Contains(t, err.Error(), "--no-notify-on-completion and --notify-on-completion cannot be used together") } // ============================================================================= diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index bd3dc0d56..d6a4db157 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -455,10 +455,18 @@ basecamp unassign [id...] --step --from --in # Remove st basecamp todos position --to 1 # Move to top basecamp todos position --to 1 --list # Move to different list basecamp todos sweep --overdue --complete --comment "Done" --in +basecamp todos create "Task" --in --list --notify-on-completion "Jane,Bob" # Notify when done +basecamp todos update --notify-on-completion "Jane" # Set who's notified on completion +basecamp todos update --no-notify-on-completion # Clear completion notifications ``` **Flags:** `--assignee` (todos only - not available on cards/messages), `--status` (completed/incomplete/archived/trashed), `--overdue`, `--list`, `--due`, `--limit`, `--all` +**Completion subscribers** ("When done, notify…"): set with +`--notify-on-completion ` on `todos create` and +`todos update`; clear with `--no-notify-on-completion` on `todos update`. +Plain updates (title, due date, etc.) preserve existing completion subscribers. + **Todo Subtasks (checklist steps):** Basecamp to-do subtasks are stored as `Kanban::Step` records, even when their parent is a normal `Todo`. The regular `basecamp todos show` response may not include them; use From 6aa92c1fb4d000a34015546a98d0e17aaec66926 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 17 Jul 2026 22:01:53 -0700 Subject: [PATCH 2/5] Use completion-subscriber wording in --notify-on-completion errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --notify-on-completion resolved people through resolveAssigneeIDs, so failures surfaced as assignee errors ("No valid assignees provided", "Assignee ID must be a positive number") — misleading for a flag that sets completion subscribers. Parameterize the resolver with a role label: resolvePersonRoleID(s) carry the wording, and resolveAssigneeID(s) stay as assignee-labeled wrappers so existing call sites and messages are unchanged. The subscriber call sites now go through resolveCompletionSubscriberIDs. Tests cover both subscriber-worded messages and assert no PUT occurs on resolution failure. --- internal/commands/cards.go | 25 +++++++++++++++++++------ internal/commands/todos.go | 12 +++++++++--- internal/commands/todos_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 7d8c2e30e..3403076a9 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -347,27 +347,34 @@ You can pass either a card ID or a Basecamp URL: } func resolveAssigneeID(ctx context.Context, app *appctx.App, input string) (int64, error) { + return resolvePersonRoleID(ctx, app, input, "Assignee") +} + +// resolvePersonRoleID resolves a single person name or ID. The role label +// ("Assignee", "Completion subscriber", …) names the person's role in error +// messages so callers surface the flag the user actually passed. +func resolvePersonRoleID(ctx context.Context, app *appctx.App, input, role string) (int64, error) { input = strings.TrimSpace(input) if input == "" { - return 0, output.ErrUsage("Assignee cannot be empty") + return 0, output.ErrUsage(fmt.Sprintf("%s cannot be empty", role)) } if id, err := strconv.ParseInt(input, 10, 64); err == nil { if id <= 0 { - return 0, output.ErrUsage("Assignee ID must be a positive number") + return 0, output.ErrUsage(fmt.Sprintf("%s ID must be a positive number", role)) } return id, nil } resolvedID, _, err := app.Names.ResolvePerson(ctx, input) if err != nil { - return 0, fmt.Errorf("failed to resolve assignee '%s': %w", input, err) + return 0, fmt.Errorf("failed to resolve %s '%s': %w", strings.ToLower(role), input, err) } id, err := strconv.ParseInt(resolvedID, 10, 64) if err != nil { return 0, fmt.Errorf("invalid resolved ID '%s': %w", resolvedID, err) } if id <= 0 { - return 0, fmt.Errorf("resolved assignee ID for '%s' is not valid: %d", input, id) + return 0, fmt.Errorf("resolved %s ID for '%s' is not valid: %d", strings.ToLower(role), input, id) } return id, nil } @@ -2460,6 +2467,12 @@ func resolveColumn(columns []basecamp.CardColumn, identifier string) int64 { } func resolveAssigneeIDs(ctx context.Context, app *appctx.App, input string) ([]int64, error) { + return resolvePersonRoleIDs(ctx, app, input, "Assignee") +} + +// resolvePersonRoleIDs resolves a comma-separated list of person names or IDs, +// labeling errors with the given role (see resolvePersonRoleID). +func resolvePersonRoleIDs(ctx context.Context, app *appctx.App, input, role string) ([]int64, error) { parts := strings.Split(input, ",") ids := make([]int64, 0, len(parts)) @@ -2469,7 +2482,7 @@ func resolveAssigneeIDs(ctx context.Context, app *appctx.App, input string) ([]i continue } - id, err := resolveAssigneeID(ctx, app, part) + id, err := resolvePersonRoleID(ctx, app, part, role) if err != nil { return nil, err } @@ -2477,7 +2490,7 @@ func resolveAssigneeIDs(ctx context.Context, app *appctx.App, input string) ([]i } if len(ids) == 0 { - return nil, output.ErrUsage("No valid assignees provided") + return nil, output.ErrUsage(fmt.Sprintf("No valid %ss provided", strings.ToLower(role))) } return ids, nil diff --git a/internal/commands/todos.go b/internal/commands/todos.go index cae5bd20d..8ccb33049 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -883,7 +883,7 @@ func newTodosCreateCmd() *cobra.Command { req.AssigneeIDs = []int64{assigneeIDInt} } if strings.TrimSpace(notifyOnCompletion) != "" { - subscriberIDs, err := resolveAssigneeIDs(cmd.Context(), app, notifyOnCompletion) + subscriberIDs, err := resolveCompletionSubscriberIDs(cmd.Context(), app, notifyOnCompletion) if err != nil { return err } @@ -1128,7 +1128,7 @@ Set or clear the people notified when the todo is completed: // omission, or preserve via a raw read that must succeed // before any PUT (fail closed). if subscribersChanged { - subscriberIDs, err := resolveAssigneeIDs(cmd.Context(), app, notifyOnCompletion) + subscriberIDs, err := resolveCompletionSubscriberIDs(cmd.Context(), app, notifyOnCompletion) if err != nil { return err } @@ -1214,7 +1214,7 @@ Set or clear the people notified when the todo is completed: // omission (nil), or preserve via a raw read that must succeed // before any PUT (fail closed). if subscribersChanged { - subscriberIDs, err := resolveAssigneeIDs(cmd.Context(), app, notifyOnCompletion) + subscriberIDs, err := resolveCompletionSubscriberIDs(cmd.Context(), app, notifyOnCompletion) if err != nil { return err } @@ -1279,6 +1279,12 @@ Set or clear the people notified when the todo is completed: return cmd } +// resolveCompletionSubscriberIDs resolves --notify-on-completion values +// (comma-separated names or IDs) with completion-subscriber wording in errors. +func resolveCompletionSubscriberIDs(ctx context.Context, app *appctx.App, input string) ([]int64, error) { + return resolvePersonRoleIDs(ctx, app, input, "Completion subscriber") +} + // completionSubscriberIDs reads the current completion subscriber ids via a raw // GET of /todos/{id}.json. The SDK's Todo model drops completion_subscribers in // todoFromGenerated (basecamp/basecamp-sdk#355), so a raw read is the only way diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 573e5334b..03ab4a0f0 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -2097,6 +2097,32 @@ func TestTodosUpdateSubscriberReadFailsClosed(t *testing.T) { } } +func TestTodosUpdateSubscriberErrorsUseSubscriberWording(t *testing.T) { + // Resolution failures for --notify-on-completion must talk about + // completion subscribers, not assignees. + t.Run("invalid id", func(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "--notify-on-completion", "0") + require.Error(t, err) + assert.Contains(t, err.Error(), "Completion subscriber ID must be a positive number") + assert.False(t, transport.hasRequest("PUT", "/"), "no PUT on resolution failure") + }) + + t.Run("no valid people", func(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "--notify-on-completion", ",") + require.Error(t, err) + assert.Contains(t, err.Error(), "No valid completion subscribers provided") + assert.False(t, transport.hasRequest("PUT", "/"), "no PUT on resolution failure") + }) +} + func TestTodosUpdateConflictingNotifyOnCompletionFlags(t *testing.T) { app, _ := setupTodosTestApp(t) From 33b19addec452b94f0ad7dc2a653be4f172533a7 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 17 Jul 2026 23:02:06 -0700 Subject: [PATCH 3/5] Test the name-resolution failure path for --notify-on-completion The subscriber-wording tests covered the numeric-validation and empty-list errors but not the ResolvePerson miss, so the "failed to resolve completion subscriber" formatting was only exercised indirectly. The update mock now matches the flat preservation route exactly (/todos/999.json) instead of any .json GET, and serves /people routes an empty directory so name resolution deterministically misses. New subtest asserts the subscriber-worded resolve error and that no PUT occurs. The explicit set/clear tests tighten their no-preservation-read assertions to the exact route. --- internal/commands/todos_test.go | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 03ab4a0f0..79863d075 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -1555,7 +1555,7 @@ func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, header := make(http.Header) header.Set("Content-Type", "application/json") - if req.Method == "GET" && strings.HasSuffix(req.URL.Path, ".json") { + if req.Method == "GET" && strings.HasSuffix(req.URL.Path, "/todos/999.json") { // Raw preservation GET of the flat /todos/{id}.json route. status := t.rawGetStatus if status == 0 { @@ -1572,6 +1572,15 @@ func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, }, nil } + if req.Method == "GET" && strings.Contains(req.URL.Path, "/people") { + // Empty people directory so name resolution deterministically misses. + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`[]`)), + Header: header, + }, nil + } + if req.Method == "GET" { mockTodo := `{"id": 999, "title": "Test", "content": "Test todo", "status": "active", "completed": false, "description": "Existing desc", "due_on": "2026-04-01", "starts_on": "2026-03-25", "bucket": {"id": 456, "name": "Test Project", "type": "Project"}, "assignees": [{"id": 42, "name": "Test User"}]}` return &http.Response{ @@ -2021,7 +2030,7 @@ func TestTodosUpdateExplicitSubscribersSkipsPreservationRead(t *testing.T) { require.NoError(t, err) assert.Equal(t, []any{float64(42)}, body["completion_subscriber_ids"]) - assert.False(t, transport.hasRequest("GET", ".json"), + assert.False(t, transport.hasRequest("GET", "/todos/999.json"), "explicit --notify-on-completion must not trigger a preservation read, got: %v", transport.requests) } @@ -2040,7 +2049,7 @@ func TestTodosUpdateNoNotifyOnCompletionClearsSubscribers(t *testing.T) { _, exists := body["completion_subscriber_ids"] assert.False(t, exists, "completion_subscriber_ids must be omitted to clear") - assert.False(t, transport.hasRequest("GET", ".json"), + assert.False(t, transport.hasRequest("GET", "/todos/999.json"), "clearing subscribers must not trigger a preservation read, got: %v", transport.requests) } @@ -2121,6 +2130,17 @@ func TestTodosUpdateSubscriberErrorsUseSubscriberWording(t *testing.T) { assert.Contains(t, err.Error(), "No valid completion subscribers provided") assert.False(t, transport.hasRequest("PUT", "/"), "no PUT on resolution failure") }) + + t.Run("unresolvable name", func(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "--notify-on-completion", "nonexistent") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to resolve completion subscriber 'nonexistent'") + assert.False(t, transport.hasRequest("PUT", "/"), "no PUT on resolution failure") + }) } func TestTodosUpdateConflictingNotifyOnCompletionFlags(t *testing.T) { From 5e0a46ec71bc8196d192e1a7644c3463d44d1b60 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 17 Jul 2026 23:18:40 -0700 Subject: [PATCH 4/5] Address review: SDK error conversion + exact mock route match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preservation read's GET error now runs through convertSDKError before wrapping, so structured codes/hints (rate limit, circuit breaker) survive with the subscriber-specific context — matching every other raw Account() call site. The update mock's preservation branch now matches the account-scoped flat route exactly (/99999/todos/999.json) instead of by suffix, so a bucket-scoped GET can never satisfy it. --- internal/commands/todos.go | 2 +- internal/commands/todos_test.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 8ccb33049..065951125 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1297,7 +1297,7 @@ func resolveCompletionSubscriberIDs(ctx context.Context, app *appctx.App, input func completionSubscriberIDs(ctx context.Context, app *appctx.App, todoID int64) ([]int64, error) { resp, err := app.Account().Get(ctx, fmt.Sprintf("/todos/%d.json", todoID)) if err != nil { - return nil, fmt.Errorf("failed to read current completion subscribers for todo %d: %w", todoID, err) + return nil, fmt.Errorf("failed to read current completion subscribers for todo %d: %w", todoID, convertSDKError(err)) } var raw struct { diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 79863d075..8707b1387 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -1555,8 +1555,9 @@ func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, header := make(http.Header) header.Set("Content-Type", "application/json") - if req.Method == "GET" && strings.HasSuffix(req.URL.Path, "/todos/999.json") { - // Raw preservation GET of the flat /todos/{id}.json route. + if req.Method == "GET" && req.URL.Path == "/99999/todos/999.json" { + // Raw preservation GET of the flat account-scoped /todos/{id}.json + // route — exact match so bucket-scoped paths can never satisfy it. status := t.rawGetStatus if status == 0 { status = 200 From de1f3c4f919dc2af24f233d9272f793a3a91fd48 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 17 Jul 2026 23:29:03 -0700 Subject: [PATCH 5/5] Reject missing or invalid subscriber ids in the preservation read A completion_subscribers element without a positive id would have put a zero into completion_subscriber_ids and reached the PUT, slipping past the fail-closed contract. The preservation read now errors on any non-positive id, and the fail-closed test matrix covers the case. --- internal/commands/todos.go | 3 +++ internal/commands/todos_test.go | 7 ++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 065951125..487f9ef52 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1314,6 +1314,9 @@ func completionSubscriberIDs(ctx context.Context, app *appctx.App, todoID int64) ids := make([]int64, len(*raw.CompletionSubscribers)) for i, s := range *raw.CompletionSubscribers { + if s.ID <= 0 { + return nil, fmt.Errorf("cannot verify current completion subscribers for todo %d: subscriber with missing or invalid id", todoID) + } ids[i] = s.ID } return ids, nil diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 8707b1387..4243c6cc3 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -2078,9 +2078,10 @@ func TestTodosUpdateSubscriberReadFailsClosed(t *testing.T) { rawGetStatus int rawGetBody string }{ - "missing key": {rawGetBody: `{"id": 999}`}, - "malformed json": {rawGetBody: `{not json`}, - "http error": {rawGetStatus: 500, rawGetBody: `{}`}, + "missing key": {rawGetBody: `{"id": 999}`}, + "malformed json": {rawGetBody: `{not json`}, + "http error": {rawGetStatus: 500, rawGetBody: `{}`}, + "invalid subscriber id": {rawGetBody: `{"id": 999, "completion_subscribers": [{"name": "No ID"}]}`}, } branches := map[string][]string{ "merge branch": {"update", "999", "New title"},