Bump SDK to d15f023f and collapse todos update onto TodosService.Edit#543
Open
jeremy wants to merge 2 commits into
Open
Bump SDK to d15f023f and collapse todos update onto TodosService.Edit#543jeremy wants to merge 2 commits into
jeremy wants to merge 2 commits into
Conversation
Pins github.com/basecamp/basecamp-sdk/go at v0.7.4-0.20260718061625-d15f023f64dc (basecamp/basecamp-sdk#375), which makes TodosService.Update merge-safe (GET -> overlay -> full PUT), adds TodosService.Edit/Replace, and round-trips completion subscribers on the public Todo model. Edit and Replace are alternate write semantics for the already-covered todo update endpoint - no new CLI command, no API-COVERAGE change. Compatibility fallout absorbed here so this commit is green standalone: - OAuth discovery (SDK #369): Config.AuthorizationEndpoint, DeviceAuthorizationEndpoint, and RegistrationEndpoint became *string and discovery now requires an issuer bound code-point-exact to the base URL. Login asserts authorization-code capability immediately after discovery - before scope handling and Dynamic Client Registration - so a device-only server fails cleanly without DCR side effects. buildAuthURL keeps a defensive nil check for other callers. Test discovery fixtures gain the required issuer field. - Credential-origin guard: the SDK now refuses to attach credentials to requests targeting a different origin than BaseURL. Mock-transport tests previously built clients with an empty BaseURL, producing path-only URLs the guard rejects; they now set a test BaseURL. - Merge-safe Update: sparse UpdateTodoRequest callers (assign batch, card steps, TUI hub) silently become merge-safe - the SDK GETs full state before the PUT, at the cost of one extra GET per update. Assign batch fixtures gain the now-required content field, and the lazy resolution test pins the full order: validation GET -> person resolution -> SDK merge GET -> PUT. - todos update: subscriber clearing now routes through the raw-PUT clear branch. The sparse-Update branch cleared subscribers by omission, but merge-safe Update always sends completion_subscriber_ids, so omission no longer clears. Interim plumbing; collapsed onto Edit next. Also fixes scripts/bump-sdk.sh reading api.synced_at: the SDK provenance file exposes .bc3.date, not .bc3.synced_at, so the field landed empty.
Replaces the two divergent update branches from #540 - the raw-PUT clear branch and the typed merge branch, each fed by a raw flat-route preservation GET - with a single TodosService.Edit call: the SDK GETs full state (completion subscribers included, per basecamp/basecamp-sdk#375), hands the closure the complete writable representation, and PUTs the whole thing back. Exactly one typed GET and one typed PUT; the clear branch's post-PUT re-Get is gone. Fail-closed posture split: field presence is now the server/SDK contract (BC3 always serializes completion_subscribers and the SDK model pins nil-vs-empty), so the missing-key guard is gone - but the CLI still refuses to write back preserved subscriber IDs it can't trust; the Edit closure aborts before the PUT on a missing or invalid id. Accepted wire deltas, defined as clears by the Edit contract: - description clear is now sent as "description": "" (was omitted) - subscriber clear is now sent as "completion_subscriber_ids": [] (was omitted) Date clears remain omission (fullBody sends dates only when non-empty). Behavior improvement: --due/--starts-on values are validated with time.Parse before any HTTP. dateparse passes unrecognized input through as-is, so garbage (or an impossible date like 2026-99-99) previously reached the server; it now fails as a usage error with zero requests. Image uploads for --description move inside the Edit closure, after Edit's GET has confirmed the todo exists - a missing todo can no longer orphan uploaded attachments.
github-actions
Bot
added
commands
There was a problem hiding this comment.
Pull request overview
Updates the Basecamp Go SDK dependency and adapts the CLI to upstream breaking changes, while simplifying todos update to a single typed read-modify-write path via TodosService.Edit.
Changes:
- Bump
github.com/basecamp/basecamp-sdk/gotov0.7.4-0.20260718061625-d15f023f64dcand adjust CLI/tests for SDK behavior changes (OAuth endpoint pointers, issuer binding, credential-origin guard). - Collapse
basecamp todos updateontoTodosService.Edit, including updated clear semantics and new pre-flight date validation to avoid network calls on invalid dates. - Update provenance scripting and fixtures/tests to align with SDK/API metadata fields and request routing.
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.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/bump-sdk.sh | Read API provenance date from .bc3.date fallback when .bc3.synced_at is absent. |
| internal/version/sdk-provenance.json | Update embedded SDK/API revision metadata to the bumped SDK + latest API revision. |
| internal/tui/workspace/data/hub_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/tui/resolve/dock_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/tui/resolve/account_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/todos.go | Refactor todos update to use TodosService.Edit and add pre-HTTP date validation. |
| internal/commands/todos_test.go | Update todo-update transport expectations for typed GET+PUT and new clear semantics; add date validation tests. |
| internal/commands/show_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/search_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/recordings_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/quickstart_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/projects_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/profile_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/people_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/messages_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/hillcharts_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/helpers_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/doctor_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/comment_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/chat_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/cards_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/boost_test.go | Set a test BaseURL to satisfy the SDK credential-origin guard in mocks. |
| internal/commands/assign_test.go | Update fixtures and ordering assertions to match merge-safe SDK Update/Edit behavior. |
| internal/auth/auth.go | Handle OAuth endpoint fields as pointers, add early auth-code capability check, and adjust DCR gating. |
| internal/auth/auth_test.go | Update OAuth fixtures/tests for endpoint pointer fields and issuer requirements; add device-only server regression test. |
| go.sum | Update dependency checksums for the bumped SDK version. |
| go.mod | Pin the Basecamp SDK module to the new pseudo-version. |
💡 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}
![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: b94667355c
ℹ️ 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".
| charm.land/bubbletea/v2 v2.0.8 | ||
| charm.land/lipgloss/v2 v2.0.5 | ||
| github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260629111348-cc8e9772e729 | ||
| github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260718061625-d15f023f64dc |
There was a problem hiding this comment.
With this SDK bump, oauth.Discover binds the metadata issuer to the exact string passed by discoverOAuth. When a user configures an otherwise valid base_url/BASECAMP_BASE_URL with a trailing slash, the CLI still passes that raw value, so a metadata issuer like https://3.basecampapi.com mismatches https://3.basecampapi.com/ and the login path silently falls back to Launchpad. Trim/normalize the BaseURL before discovery so valid spellings do not switch issuers.
Useful? React with 👍 / 👎.
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.
Closes #541. Lineage: #538 → #540 (interim fix) → this PR (Phase 2 collapse), enabled by basecamp/basecamp-sdk#375 (merged as
d15f023f).Commit 1: SDK bump + compatibility
Pins
github.com/basecamp/basecamp-sdk/goatv0.7.4-0.20260718061625-d15f023f64dc. The range brings three breaking changes the CLI absorbs here, keeping the commit green standalone:AuthorizationEndpoint/DeviceAuthorizationEndpoint/RegistrationEndpointbecame*string, and discovery now requires anissuerbound code-point-exact to the base URL.Loginasserts authorization-code capability immediately after discovery — before scope handling and DCR — so a device-only server fails cleanly with zero registration requests (new test pins this). Discovery test fixtures gain the requiredissuer.BaseURL. Mock-transport tests built clients with an emptyBaseURL(path-only URLs); they now set a test BaseURL.Update: sparseUpdateTodoRequestcallers (assign batch, card steps, TUI hub) silently become merge-safe, at the cost of one extra GET per update. Assign fixtures gain the now-requiredcontent; the lazy-resolution test pins the full order (validation GET → person resolution → SDK merge GET → PUT). Subscriber clearing is routed through the interim raw-PUT branch, since merge-safeUpdatecan no longer clear by omission.Also fixes
scripts/bump-sdk.shreadingapi.synced_at(the SDK exposes.bc3.date, not.bc3.synced_at).Commit 2: single-path Edit collapse
todos updatedrops both #540 branches (raw-PUT clear + typed merge, each fed by a raw flat-route preservation GET) for oneTodosService.Editcall: exactly one typed GET and one typed PUT, no post-PUT re-Get.TestTodosUpdateSingleTypedRoundTrippins the exact wire sequence.Fail-closed posture split — field presence is now the server/SDK contract (BC3 always serializes
completion_subscribers; the SDK model pins nil-vs-empty), so the missing-key guard is consciously deleted. The CLI still refuses to write back preserved subscriber IDs it can't trust: the Edit closure aborts before the PUT on a missing/invalid id.Accepted wire deltas (clears per the Edit contract; previously omission):
"description": """completion_subscriber_ids": []Behavior improvement:
--due/--starts-onare validated withtime.Parsebefore any HTTP — garbage and impossible dates (2026-99-99) previously reached the server; they now fail as usage errors with zero requests. Image uploads for--descriptionmove inside the Edit closure, after the GET has confirmed the todo exists, so a missing todo can't orphan uploads.Verification
bin/cigreen on the final tree.BASECAMP_TOKEN): all todos-write scenarios pass, including "todos update preserves completion subscribers" and the--no-dueclear leg; all assign/unassign scenarios pass on the merge-safe Update path. 10 unrelated failures are pre-existing smoke↔CLI drift (files update --description,subscriptions add --personnever existed on main; staleattachoutput-shape expectation) and server-side environment issues (message board disabled in QA project, webhook 400, timesheet/group-position 404, visibility 403).api get:--no-descriptioncleared the description (sent as"") while preserving due date and completion subscribers;--no-notify-on-completioncleared subscribers (sent as[]) while preserving content and due date. QA todo trashed afterward.Summary by cubic
Upgrades the Basecamp SDK and refactors
todos updateto a single typed GET+PUT usingTodosService.Edit, reducing requests and simplifying clears. Also improves OAuth compatibility and safety checks.Refactors
todos updateontoTodosService.Edit(exactly one typed GET then one PUT; no raw preservation read or re-fetch)."",completion_subscriber_idssends[], date clears remain omission; preserved subscribers are kept only when IDs are valid.--due/--starts-onwithtime.Parsebefore any HTTP; move description image uploads inside the Edit closure to avoid orphaned uploads if the todo is missing.Dependencies
github.com/basecamp/basecamp-sdk/goto a version with merge-safeUpdate, OAuth pointer endpoints and issuer binding, and a credential-origin guard (tests now setBaseURL).scripts/bump-sdk.shto read.bc3.datefor API provenance.Written for commit b946673. Summary will update on new commits.