From 5b395944b5d5152311e0f32e7edcde6b5e8dcbe2 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 4 Aug 2026 11:22:02 -0700 Subject: [PATCH 1/3] Stop echoing back step fields the caller never changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bc3#12521 made JSON step updates presence-aware: omitting a parameter now leaves it unchanged, an explicit null clears the due date, and an explicit empty assignee list still removes everyone. The API docs say so, and a live check against production confirms it. That retires the title workaround at three call sites. All three re-sent the step's current title because the old controller rebuilt the recordable from scratch and rejected an update without one: - cards step update carried a dedicated CardSteps().Get purely to fetch a title it would send straight back. That read is gone. - assign --step and unassign --step already fetch the step for its assignee list, so no request is saved there — but they stop asserting a value the caller never asked to change, which is the actual bug from #604: a concurrent title edit would be silently reverted. TestCardsStepUpdate{Assignees,Due}OnlyCarriesTitle asserted the old behaviour, so they are inverted rather than supplemented — getCount == 0 is what proves the extra read is gone, since the request body alone cannot. New coverage pins the no-echo contract for assign/unassign --step, including that unassigning the last person sends an explicit empty list. Refs #604 --- internal/commands/assign.go | 6 --- internal/commands/assign_test.go | 89 +++++++++++++++++++++++++++++++- internal/commands/cards.go | 8 --- internal/commands/cards_test.go | 23 +++++---- 4 files changed, 100 insertions(+), 26 deletions(-) diff --git a/internal/commands/assign.go b/internal/commands/assign.go index dc069b479..072b4db19 100644 --- a/internal/commands/assign.go +++ b/internal/commands/assign.go @@ -644,10 +644,7 @@ func doAssignStep(cmd *cobra.Command, app *appctx.App, stepIDStr, assigneeID str } assigneeIDs = append(assigneeIDs, assigneeIDInt) - // The API rejects step updates without a title, so carry over the - // current one. updated, err := app.Account().CardSteps().Update(cmd.Context(), stepID, &basecamp.UpdateStepRequest{ - Title: step.Title, AssigneeIDs: assigneeIDs, }) if err != nil { @@ -749,10 +746,7 @@ func doUnassignStep(cmd *cobra.Command, app *appctx.App, stepIDStr string, assig assigneeIDs := removeID(existingAssigneeIDs(step.Assignees), assigneeIDInt) - // The API rejects step updates without a title, so carry over the - // current one. updated, err := app.Account().CardSteps().Update(cmd.Context(), stepID, &basecamp.UpdateStepRequest{ - Title: step.Title, AssigneeIDs: assigneeIDs, }) if err != nil { diff --git a/internal/commands/assign_test.go b/internal/commands/assign_test.go index 99ba67ff1..03542be0c 100644 --- a/internal/commands/assign_test.go +++ b/internal/commands/assign_test.go @@ -444,7 +444,7 @@ func (s *stepPathTransport) RoundTrip(req *http.Request) (*http.Response, error) return nil, fmt.Errorf("unexpected HTTP request: %s %s", req.Method, path) } -func setupStepPathTestApp(t *testing.T, transport *stepPathTransport) *appctx.App { +func setupStepPathTestApp(t *testing.T, transport http.RoundTripper) *appctx.App { t.Helper() t.Setenv("BASECAMP_NO_KEYRING", "1") @@ -793,3 +793,90 @@ func TestUnassignStepCarriesTitle(t *testing.T) { assert.Equal(t, "Existing step", body["title"]) assert.Equal(t, []any{}, body["assignee_ids"]) } + +// stepUpdateBodyTransport serves the step and the current user, and captures +// the PUT body so tests can assert exactly which fields reach the wire. +type stepUpdateBodyTransport struct { + stepAssignees string // JSON array for the step's current assignees + capturedPut []byte + getCount int +} + +func (s *stepUpdateBodyTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + respond := func(body string) (*http.Response, error) { + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(body)), + Header: header, + }, nil + } + + path := req.URL.Path + assignees := s.stepAssignees + if assignees == "" { + assignees = "[]" + } + stepJSON := fmt.Sprintf(`{"id": 456, "title": "Test Step", "due_on": "2026-07-04", "assignees": %s}`, assignees) + + switch { + case req.Method == "GET" && strings.Contains(path, "/projects.json"): + return respond(`[{"id": 123, "name": "Test Project"}]`) + case req.Method == "GET" && strings.Contains(path, "/my/profile.json"): + return respond(`{"id": 999, "name": "Test User"}`) + case req.Method == "GET" && strings.Contains(path, "card_tables/steps/456"): + s.getCount++ + return respond(stepJSON) + case req.Method == "PUT" && strings.Contains(path, "card_tables/steps/456"): + body, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + s.capturedPut = body + if err := req.Body.Close(); err != nil { + return nil, err + } + return respond(stepJSON) + } + + return nil, fmt.Errorf("unexpected HTTP request: %s %s", req.Method, path) +} + +// TestAssignStepSendsOnlyAssignees verifies that assigning someone to a step +// sends assignee_ids and nothing else. The server preserves attributes the +// request omits, so echoing back the title or due date would be the CLI +// re-asserting values the caller never asked to change. +func TestAssignStepSendsOnlyAssignees(t *testing.T) { + transport := &stepUpdateBodyTransport{} + app := setupStepPathTestApp(t, transport) + + cmd := NewAssignCmd() + require.NoError(t, executeAssignCommand(cmd, app, "456", "--step", "--to", "me", "-p", "123")) + + var body map[string]any + require.NotEmpty(t, transport.capturedPut, "step update was never sent") + require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) + assert.NotContains(t, body, "title") + assert.NotContains(t, body, "due_on") + assert.Equal(t, []any{float64(999)}, body["assignee_ids"]) +} + +// TestUnassignStepSendsEmptyAssigneeList verifies that removing the last +// assignee sends an explicit empty list — which is what clears assignees — +// and still says nothing about title or due date. +func TestUnassignStepSendsEmptyAssigneeList(t *testing.T) { + transport := &stepUpdateBodyTransport{stepAssignees: `[{"id": 999, "name": "Test User"}]`} + app := setupStepPathTestApp(t, transport) + + cmd := NewUnassignCmd() + require.NoError(t, executeAssignCommand(cmd, app, "456", "--step", "--from", "me", "-p", "123")) + + var body map[string]any + require.NotEmpty(t, transport.capturedPut, "step update was never sent") + require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) + assert.NotContains(t, body, "title") + assert.NotContains(t, body, "due_on") + assert.Equal(t, []any{}, body["assignee_ids"], "an empty list is how the API is told to clear assignees") +} diff --git a/internal/commands/cards.go b/internal/commands/cards.go index d53d1e88f..78c2ea2f2 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -2938,14 +2938,6 @@ You can pass either a step ID or a Basecamp URL: req := &basecamp.UpdateStepRequest{} if title != "" { req.Title = title - } else { - // The API rejects step updates without a title, so carry - // over the current one when only other fields change. - current, err := app.Account().CardSteps().Get(cmd.Context(), stepID) - if err != nil { - return convertSDKError(err) - } - req.Title = current.Title } if dueOn != "" { req.DueOn = dateparse.Parse(dueOn) diff --git a/internal/commands/cards_test.go b/internal/commands/cards_test.go index c13dd7d70..3b16f203a 100644 --- a/internal/commands/cards_test.go +++ b/internal/commands/cards_test.go @@ -241,10 +241,11 @@ func (t *mockStepUpdateTransport) RoundTrip(req *http.Request) (*http.Response, }, nil } -// TestCardsStepUpdateAssigneesOnlyCarriesTitle verifies that updating only -// assignees fetches the current step and includes its title in the request — -// the API rejects step updates without a title. -func TestCardsStepUpdateAssigneesOnlyCarriesTitle(t *testing.T) { +// TestCardsStepUpdateAssigneesOnlySendsNoTitle verifies that updating only +// assignees sends assignees and nothing else. The server preserves attributes +// the request omits, so there is no title to carry over — and getCount is what +// proves the extra read is gone, since the body alone cannot. +func TestCardsStepUpdateAssigneesOnlySendsNoTitle(t *testing.T) { transport := &mockStepUpdateTransport{} app := setupCardsMockApp(t, transport) @@ -252,17 +253,17 @@ func TestCardsStepUpdateAssigneesOnlyCarriesTitle(t *testing.T) { err := executeCommand(cmd, app, "456", "--assignees", "789") require.NoError(t, err) - assert.Equal(t, 1, transport.getCount) + assert.Equal(t, 0, transport.getCount, "expected no read-before-write") var body map[string]any require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) - assert.Equal(t, "Current title", body["title"]) + assert.NotContains(t, body, "title", "must not echo back a field the caller never changed") assert.Equal(t, []any{float64(789)}, body["assignee_ids"]) } -// TestCardsStepUpdateDueOnlyCarriesTitle verifies that updating only the due -// date fetches the current step and includes its title in the request. -func TestCardsStepUpdateDueOnlyCarriesTitle(t *testing.T) { +// TestCardsStepUpdateDueOnlySendsNoTitle verifies the same for a due-date-only +// update. +func TestCardsStepUpdateDueOnlySendsNoTitle(t *testing.T) { transport := &mockStepUpdateTransport{} app := setupCardsMockApp(t, transport) @@ -270,11 +271,11 @@ func TestCardsStepUpdateDueOnlyCarriesTitle(t *testing.T) { err := executeCommand(cmd, app, "456", "--due", "2026-07-04") require.NoError(t, err) - assert.Equal(t, 1, transport.getCount) + assert.Equal(t, 0, transport.getCount, "expected no read-before-write") var body map[string]any require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) - assert.Equal(t, "Current title", body["title"]) + assert.NotContains(t, body, "title", "must not echo back a field the caller never changed") assert.Equal(t, "2026-07-04", body["due_on"]) } From df2b92bf25ebf0cc57d7a0c7e66ccfc47fade889 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 4 Aug 2026 11:34:32 -0700 Subject: [PATCH 2/3] Invert the two assign_test cases that pinned the old title echo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestAssignStepCarriesTitle and TestUnassignStepCarriesTitle asserted the workaround this branch removes. I missed them and added a near-duplicate pair instead; CI caught it. Invert the originals in place — they already have a transport that covers both directions and resolves people — give that transport's step a due_on so the no-echo assertion covers the due date too, and drop my duplicates. --- internal/commands/assign_test.go | 112 +++++-------------------------- 1 file changed, 16 insertions(+), 96 deletions(-) diff --git a/internal/commands/assign_test.go b/internal/commands/assign_test.go index 03542be0c..661de045c 100644 --- a/internal/commands/assign_test.go +++ b/internal/commands/assign_test.go @@ -444,7 +444,7 @@ func (s *stepPathTransport) RoundTrip(req *http.Request) (*http.Response, error) return nil, fmt.Errorf("unexpected HTTP request: %s %s", req.Method, path) } -func setupStepPathTestApp(t *testing.T, transport http.RoundTripper) *appctx.App { +func setupStepPathTestApp(t *testing.T, transport *stepPathTransport) *appctx.App { t.Helper() t.Setenv("BASECAMP_NO_KEYRING", "1") @@ -726,7 +726,7 @@ func (m *mockStepAssignTransport) RoundTrip(req *http.Request) (*http.Response, header := make(http.Header) header.Set("Content-Type", "application/json") - stepJSON := `{"id": 456, "title": "Existing step", "completed": false, "assignees": [{"id": 11, "name": "Existing Person"}]}` + stepJSON := `{"id": 456, "title": "Existing step", "due_on": "2026-09-01", "completed": false, "assignees": [{"id": 11, "name": "Existing Person"}]}` switch req.Method { case "GET": @@ -762,9 +762,12 @@ func (m *mockStepAssignTransport) RoundTrip(req *http.Request) (*http.Response, } } -// TestAssignStepCarriesTitle verifies that assigning a person to a step sends -// the current title in the update, which the API requires. -func TestAssignStepCarriesTitle(t *testing.T) { +// TestAssignStepSendsOnlyAssignees verifies that assigning a person to a step +// sends assignee_ids and nothing else. The server preserves attributes the +// request omits, so echoing back the title or due date would be the CLI +// re-asserting values the caller never changed — and reverting a concurrent +// edit to them. +func TestAssignStepSendsOnlyAssignees(t *testing.T) { transport := &mockStepAssignTransport{} app := setupCardsMockApp(t, transport) @@ -774,13 +777,16 @@ func TestAssignStepCarriesTitle(t *testing.T) { var body map[string]any require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) - assert.Equal(t, "Existing step", body["title"]) + assert.NotContains(t, body, "title") + assert.NotContains(t, body, "due_on") assert.Equal(t, []any{float64(11), float64(99)}, body["assignee_ids"]) } -// TestUnassignStepCarriesTitle verifies that removing a person from a step -// also sends the current title in the update. -func TestUnassignStepCarriesTitle(t *testing.T) { +// TestUnassignStepSendsEmptyAssigneeList verifies the same for removal, and +// that taking off the last assignee sends an explicit empty list — omitting +// the key would now mean "leave assignees alone", so the clear must be said +// out loud. +func TestUnassignStepSendsEmptyAssigneeList(t *testing.T) { transport := &mockStepAssignTransport{} app := setupCardsMockApp(t, transport) @@ -790,93 +796,7 @@ func TestUnassignStepCarriesTitle(t *testing.T) { var body map[string]any require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) - assert.Equal(t, "Existing step", body["title"]) - assert.Equal(t, []any{}, body["assignee_ids"]) -} - -// stepUpdateBodyTransport serves the step and the current user, and captures -// the PUT body so tests can assert exactly which fields reach the wire. -type stepUpdateBodyTransport struct { - stepAssignees string // JSON array for the step's current assignees - capturedPut []byte - getCount int -} - -func (s *stepUpdateBodyTransport) RoundTrip(req *http.Request) (*http.Response, error) { - header := make(http.Header) - header.Set("Content-Type", "application/json") - - respond := func(body string) (*http.Response, error) { - return &http.Response{ - StatusCode: 200, - Body: io.NopCloser(strings.NewReader(body)), - Header: header, - }, nil - } - - path := req.URL.Path - assignees := s.stepAssignees - if assignees == "" { - assignees = "[]" - } - stepJSON := fmt.Sprintf(`{"id": 456, "title": "Test Step", "due_on": "2026-07-04", "assignees": %s}`, assignees) - - switch { - case req.Method == "GET" && strings.Contains(path, "/projects.json"): - return respond(`[{"id": 123, "name": "Test Project"}]`) - case req.Method == "GET" && strings.Contains(path, "/my/profile.json"): - return respond(`{"id": 999, "name": "Test User"}`) - case req.Method == "GET" && strings.Contains(path, "card_tables/steps/456"): - s.getCount++ - return respond(stepJSON) - case req.Method == "PUT" && strings.Contains(path, "card_tables/steps/456"): - body, err := io.ReadAll(req.Body) - if err != nil { - return nil, err - } - s.capturedPut = body - if err := req.Body.Close(); err != nil { - return nil, err - } - return respond(stepJSON) - } - - return nil, fmt.Errorf("unexpected HTTP request: %s %s", req.Method, path) -} - -// TestAssignStepSendsOnlyAssignees verifies that assigning someone to a step -// sends assignee_ids and nothing else. The server preserves attributes the -// request omits, so echoing back the title or due date would be the CLI -// re-asserting values the caller never asked to change. -func TestAssignStepSendsOnlyAssignees(t *testing.T) { - transport := &stepUpdateBodyTransport{} - app := setupStepPathTestApp(t, transport) - - cmd := NewAssignCmd() - require.NoError(t, executeAssignCommand(cmd, app, "456", "--step", "--to", "me", "-p", "123")) - - var body map[string]any - require.NotEmpty(t, transport.capturedPut, "step update was never sent") - require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) assert.NotContains(t, body, "title") assert.NotContains(t, body, "due_on") - assert.Equal(t, []any{float64(999)}, body["assignee_ids"]) -} - -// TestUnassignStepSendsEmptyAssigneeList verifies that removing the last -// assignee sends an explicit empty list — which is what clears assignees — -// and still says nothing about title or due date. -func TestUnassignStepSendsEmptyAssigneeList(t *testing.T) { - transport := &stepUpdateBodyTransport{stepAssignees: `[{"id": 999, "name": "Test User"}]`} - app := setupStepPathTestApp(t, transport) - - cmd := NewUnassignCmd() - require.NoError(t, executeAssignCommand(cmd, app, "456", "--step", "--from", "me", "-p", "123")) - - var body map[string]any - require.NotEmpty(t, transport.capturedPut, "step update was never sent") - require.NoError(t, json.Unmarshal(transport.capturedPut, &body)) - assert.NotContains(t, body, "title") - assert.NotContains(t, body, "due_on") - assert.Equal(t, []any{}, body["assignee_ids"], "an empty list is how the API is told to clear assignees") + assert.Equal(t, []any{}, body["assignee_ids"]) } From 8e5787fc32e3c0809fbe3ab5b1f7e35b882bf72c Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 4 Aug 2026 12:29:26 -0700 Subject: [PATCH 3/3] Retire the resend-the-title advice for raw step updates too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skill's todo-subtask section still told agents to include the current title on every raw step update and warned that omitting it "may reset the step title to Untitled". Under bc3#12521 that is exactly backwards: echoing a title you did not mean to change is what reverts a concurrent edit — the same failure this branch removes from the CLI's own code paths. Todo subtasks are not a separate contract. PUT card_tables/steps/:id routes to StepsController#update for both spellings (config/routes.rb:1158), so the presence-aware behaviour is identical. Verified on a live todo-backed subtask: PUT with only assignee_ids preserved title and due_on; PUT with only due_on preserved title and assignees. Document the clears explicitly ("due_on": null, "assignee_ids": []) and keep the note that assignee_ids replaces the whole list. --- skills/basecamp/SKILL.md | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 44f79ac25..44591cdf6 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -547,10 +547,10 @@ PARENT_TODO_ID= \ basecamp recordings list --in --type Kanban::Step --all \ --jq '.data[] | select(.parent.id==(env.PARENT_TODO_ID | tonumber)) | {id,title,status,parent:.parent.id,url}' -# Assign or set a due date. -# Include the current title and every person who should remain assigned. +# Assign or set a due date. Send only what you're changing — omitted fields are +# left alone. `assignee_ids` replaces the whole list, so name everyone who stays. basecamp api put /buckets//card_tables/steps/.json \ - --data '{"title":"Current subtask title","assignee_ids":[,],"due_on":""}' \ + --data '{"assignee_ids":[,],"due_on":""}' \ --json # Complete or reopen a subtask @@ -589,10 +589,20 @@ returned `not_found`: subtasks, add `--status trashed`; archived parents may require `--status archived`. -When updating a todo subtask with the raw API, include the existing `title` along -with metadata changes; omitting it may reset the step title to `Untitled`. -`assignee_ids` sets the full assignee list for the step, so include every person -who should remain assigned. The generic +**Raw step updates are partial.** `PUT .../card_tables/steps/.json` leaves +every parameter you omit unchanged, so send only the fields you are changing. +Echoing back a `title` you did not mean to change is not merely redundant — it +reverts anyone who edited the title between your read and your write. To clear a +value, say so explicitly: `"due_on": null` clears the due date, `"assignee_ids": +[]` removes everyone. `assignee_ids` always replaces the whole list rather than +adding to it, so name every person who should remain assigned. + +(This is bc3#12521. Before it, an omitted field *was* cleared and a title-less +update was rejected, which is why older guidance said to resend the title. Todo +subtasks and card steps share one endpoint and one contract — `PUT +card_tables/steps/:id` routes to the same controller for both.) + +The generic `basecamp assign --step ...` command is intended for card steps and may fail with `Bad Request` for todo-backed steps, so prefer `assignee_ids` on the raw step update endpoint for todo subtasks.