Fix todos update silently clearing completion subscribers#540
Merged
Conversation
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 <names or ids> 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.
github-actions
Bot
added
commands
There was a problem hiding this comment.
Pull request overview
Fixes basecamp todos update unintentionally clearing “When done, notify…” completion subscribers by preserving existing subscriber IDs across updates (despite the current SDK model dropping completion_subscribers), and adds explicit flags to set/clear them.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Preserve completion subscribers on update by reading
/todos/{id}.jsonand carryingcompletion_subscriber_idsinto the PUT (fail-closed on read/parse/missing-key). - Add
--notify-on-completion(create/update) and--no-notify-on-completion(update) for explicit subscriber control. - Extend unit tests + smoke coverage and document the new flags in the skill docs and
.surface.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Documents new completion-subscriber flags and preservation behavior. |
| internal/commands/todos.go | Implements preservation read + new flags for setting/clearing completion subscribers. |
| internal/commands/todos_test.go | Adds unit coverage for preservation, explicit set/clear, and fail-closed behavior. |
| e2e/smoke/smoke_todos_write.bats | Adds a smoke scenario exercising preserve + clear behavior end-to-end. |
| .surface | Records the new CLI flags for drift/surface tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
--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.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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.
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.
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.
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.
Fixes #538.
Problem
Every
basecamp todos updatewiped the todo's completion subscribers ("When done, notify…"). The BC3 todos PUT has replace semantics — omitted field = cleared — and neither update branch includedcompletion_subscriber_ids. The #412/#413 read-modify-write merge couldn't preserve them because the SDK'sTodomodel dropscompletion_subscribersintodoFromGenerated(basecamp/basecamp-sdk#355), so the merge had nothing to carry forward.Fix
Preservation — a plain update now reads the current subscriber ids via a raw GET of the flat
/todos/{id}.jsonroute (which serves the field the SDK model drops) and carries them into the PUT, in both the typed-merge and raw-clear branches. The read fails closed: an HTTP error, malformed JSON, or a response missing thecompletion_subscriberskey aborts before any PUT — the command never reaches the write without either explicit user intent or a verified read of current state. ThecompletionSubscriberIDshelper is commented as temporary; it and both call sites go away once the SDK round-trips the field and the CLI collapses onto the SDK's merge-safe update.Explicit control — subscribers become directly editable:
todos create --notify-on-completion <names or ids>andtodos update --notify-on-completion <names or ids>set them (comma-separated, people-name tab completion)todos update --no-notify-on-completionclears them (clearing works by omission; explicit intent skips the preservation read entirely)Tests
--no-dueupdates; flat-route assertion (no/buckets/read); explicit set/clear with no preservation GET; a fail-closed matrix (missing key / malformed JSON / HTTP 500 × merge / clear branch) asserting no PUT occurs; flag-conflict usage error;completion_subscriber_idsin the create body.--no-duepreserves →--no-notify-on-completionclears → trash. Not yet run against a live server (no QA credentials in the authoring session); needs amake smokepass before this counts as runtime-verified.bin/cifully green (fmt, vet, lint, unit, replay e2e, naming, surface, skill drift, bare groups, smoke coverage, provenance, tidy).Bookkeeping
skills/basecamp/SKILL.mddocuments the new flags and that plain updates preserve subscribers..surfacegains the three new flag records, hand-inserted and verified identical to fresh generation. A full snapshot regeneration is deliberately deferred: the committed snapshot has ~9k lines of unrelated drift since Fix --version over-propagation in check-cli-surface.sh #499 changed the generator — tracked separately in Reconcile .surface snapshot with the post-#499 generator #539.Summary by cubic
Stops
basecamp todos updatefrom wiping "When done, notify…" subscribers. Preserves existing subscribers and adds flags to set or clear them explicitly.Bug Fixes
completion_subscribersfrom/todos/{id}.jsonand sendingcompletion_subscriber_idsin PUTs (both merge and clear paths).convertSDKErrorto keep structured SDK codes/hints.--notify-on-completionerrors; person resolution is parameterized so messages reference subscribers, not assignees.New Features
todos create --notify-on-completion <names|ids>andtodos update --notify-on-completion <names|ids>set subscribers (with people‑name completion).todos update --no-notify-on-completionclears subscribers; explicit set/clear skips the preservation read..surface.Written for commit de1f3c4. Summary will update on new commits.