diff --git a/internal/commands/assign.go b/internal/commands/assign.go index dc069b47..072b4db1 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 99ba67ff..661de045 100644 --- a/internal/commands/assign_test.go +++ b/internal/commands/assign_test.go @@ -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,6 +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.NotContains(t, body, "title") + assert.NotContains(t, body, "due_on") assert.Equal(t, []any{}, body["assignee_ids"]) } diff --git a/internal/commands/cards.go b/internal/commands/cards.go index d53d1e88..78c2ea2f 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 c13dd7d7..3b16f203 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"]) } diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 44f79ac2..44591cdf 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.