Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions internal/commands/assign.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
25 changes: 16 additions & 9 deletions internal/commands/assign_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand All @@ -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")
Comment thread
jeremy marked this conversation as resolved.
assert.NotContains(t, body, "due_on")
assert.Equal(t, []any{}, body["assignee_ids"])
}
8 changes: 0 additions & 8 deletions internal/commands/cards.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 12 additions & 11 deletions internal/commands/cards_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,40 +241,41 @@ 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)

cmd := newCardsStepUpdateCmd()
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")
Comment thread
jeremy marked this conversation as resolved.
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)

cmd := newCardsStepUpdateCmd()
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"])
}

Expand Down
24 changes: 17 additions & 7 deletions skills/basecamp/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -547,10 +547,10 @@ PARENT_TODO_ID=<parent_todo_id> \
basecamp recordings list --in <project> --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/<project_id>/card_tables/steps/<step_id>.json \
--data '{"title":"Current subtask title","assignee_ids":[<person_id>,<existing_person_id>],"due_on":"<YYYY-MM-DD>"}' \
--data '{"assignee_ids":[<person_id>,<existing_person_id>],"due_on":"<YYYY-MM-DD>"}' \
--json

# Complete or reopen a subtask
Expand Down Expand Up @@ -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/<id>.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_id> --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.
Expand Down
Loading