Stop echoing back step fields the caller never changed - #620
Merged
Conversation
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
github-actions
Bot
added
commands
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b395944b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bc3#12521 (
4e34dc83eb, onmaster) made JSON step and card updates presence-aware.steps#updatenowdoes
@recording.recordable.changing(step_update_params)and replacesassignees only
if update_assignees?, so omitting a parameter leaves itunchanged; an explicit
nullclears the due date and an explicit[]stillremoves everyone. The public docs say the same thing now.
That retires the title workaround at all three call sites. Each re-sent the
step's current title because the old controller rebuilt the recordable from
scratch and rejected an update without one:
cards step updatecarried a dedicatedCardSteps().Getpurely to fetcha title it would send straight back. That extra read is gone.
assign --step/unassign --stepalready fetch the step for itsassignee list, so no request is saved there. What goes away is the CLI
asserting a value the caller never asked to change — which is the actual bug
in Card step updates wholesale-replace fields:
assign --stepwipes due_on;step update --duewipes assignees #604: a title edited concurrently would be silently reverted by anunrelated assign.
Tests
TestCardsStepUpdate{Assignees,Due}OnlyCarriesTitleasserted the oldbehaviour, so they're inverted rather than supplemented.
getCount == 0is whatproves the extra read is gone — asserting on the request body alone would not.
New coverage pins the no-echo contract for
assign/unassign --step, includingthat unassigning the last person sends an explicit empty list rather than
omitting the key.
Verified live
Against production (QA sandbox project, card created and trashed afterward):
assign --stepassignee_idsonlydue_onsurvivedcards step update --duedue_ononlyunassign --step(last one)assignee_ids: []due_onsurvivedRows 1 and 2 are exactly the two data-loss bugs reported in #604 — both are
fixed server-side, with no client release needed. Row 3 also settles a claim I
made and then withdrew on the record: unassigning the last person does not
silently no-op. The SDK builds
body := map[string]any{}and guards onreq.AssigneeIDs != nil, and the CLI'sremoveIDreturns a non-nil emptyslice, so
assignee_ids: []does reach the wire.bin/ciis green.Not in scope
Explicit due-date clears. The CLI can't express one —
cards.goonly setsreq.DueOnwhen--dueis non-empty, so--due ""is indistinguishable fromomitting the flag. Separately, SDK v0.12.0 encodes a card's due-date clear by
omitting
due_on, which post-#12521 means "leave unchanged"; that's alreadyfixed upstream in basecamp-sdk#647 and needs an SDK release to reach us.
Refs #604 — closing that issue is a separate step, once this merges.
Summary by cubic
Send only changed fields when updating card steps to avoid clobbering titles or due dates and remove the extra read in
cards step update. Also updatesSKILL.mdto document presence-aware step updates (omit unchanged fields; usenull/[]to clear).assign --stepandunassign --stepnow send onlyassignee_ids(notitle/due_on), preventing silent reverts from concurrent edits (refs Card step updates wholesale-replace fields:assign --stepwipes due_on;step update --duewipes assignees #604). Tests inverted to assert no echo and that clearing usesassignee_ids: [].cards step updatestops reading the step to carrytitle; due-only and assignee-only updates send only the changed field. Tests assert zero extra GETs and correct request bodies.Written for commit 8e5787f. Summary will update on new commits.