fix(messages): allow Update to clear a message's category - #838
Conversation
MessagesService.Update could set a category but never remove one: an omitted category_id is left unchanged since BC3 became presence-aware, and the generated *int64 category_id cannot spell the clear. Add ClearCategory to UpdateMessageRequest (additive; CategoryID int64 stays the set path) and hand-marshal the update body so a clear reaches the wire as category_id: "" — never null (SPEC §18), the one clear encoding all six SDKs express identically. BC3 blank-casts "" to nil to clear (basecamp/bc3#12521). This mirrors CardsService.UpdateVerbatim, which clears due_on the same way. ClearCategory wins over CategoryID.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![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: 74857ffd3d
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The canonical hand-marshaled-body inventories must document this new carve-out.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Go support for explicitly clearing a message category while preserving existing update semantics.
Changes:
- Adds the
ClearCategoryrequest control flag. - Sends
category_id: ""when clearing. - Adds wire-level and service-level tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
go/pkg/basecamp/messages.go |
Implements and documents category clearing. |
go/pkg/basecamp/messages_test.go |
Tests category presence semantics. |
go/pkg/basecamp/wire_bodies_test.go |
Pins exact serialized request bodies. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
SPEC.md §18 must document the newly introduced hand-marshaling carve-out.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc4abf9954
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92d67575f1
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c6921232e
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85c7e47774
ℹ️ 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".
What
MessagesService.Updatecould set a message's category but never remove one. Since BC3 became presence-aware, an omittedcategory_idis left unchanged, and the generated*int64 category_idhas no spelling that clears — leaving the TUI/CLI unable to un-categorize a message.Change
Additive public API on
UpdateMessageRequest:CategoryID int64stays the set path (0= leave unchanged), preserving the integer set-wire type. Keptint64rather than switching to*int64— the pointer change would be a breaking change to an existing exported field, and the repo prefers the additive path here.ClearCategory boolcarries the clear. It is a control flag (json:"-"), never marshaled directly.Wire behavior (
Updatenow hand-marshals its body):ClearCategory: true{"category_id":""}CategoryID: 42{"category_id":42}category_idomitted (category unchanged){"category_id":""}—ClearCategorywinsThe clear goes on the wire as
category_id: ""— nevernull. SPEC §18 forbids null-for-clear and 5/6 SDKs strip nulls structurally, so""is the only clear all six SDKs express identically. bc3 blank-casts""→nilto clear (basecamp/bc3#12521,messages_controller.rbfind_category).This mirrors
CardsService.UpdateVerbatim, which clearsdue_onthe same way via a hand-marshaled map (SPEC §18 rule-1 carve-out). No conflict precedent exists in the generated surface (Cards uses a single pointer field), so the documented rule isClearCategorywins.No spec change — the encoding is expressed entirely at the wire, so
spec/basecamp.smithyand generated code are untouched.Tests
TestMessagesUpdateBodyBytes(wire_bodies_test.go) — byte-level pins for set / clear / leave-unaddressed / clear-wins / set-alongside-other-fields.TestMessagesService_UpdateCategory(messages_test.go) — presence semantics: clear is present-and-empty (""), set is the integer, unaddressed omits the key, clear wins.Checks
make check(Go: fmt/vet/lint/test) green. Targeted cross-SDK parity gates green:go-check-drift,go-check-wrapper-drift,go-check-optional-pointers,check-write-semantics-parity,check-fixture-coverage.Refs card 10264999579.
Summary by cubic
Lets
MessagesService.Updateclear a message's category. Previously an omittedcategory_idwas left unchanged and the generated*int64field could not spell a clear, so the TUI/CLI had no way to un-categorize a message.Wire semantics
UpdateMessageRequestgainsClearCategory;CategoryIDstill sets the category (0= leave unchanged).ClearCategorysendscategory_idas""on the wire — nevernull— which BC3 blank-casts to nil to clear; the clear is Go-only since the shared contract still modelscategory_idas an integer.ClearCategorywins when both fields are set.Updatehand-marshals its body; SPEC §18 and AGENTS.md now list thecategory_idclear as a second instance of the hand-marshal carve-out alongside the date clears.Written for commit 85c7e47. Summary will update on new commits.