diff --git a/COMMUNIQUE-card-step-partial-updates.md b/COMMUNIQUE-card-step-partial-updates.md deleted file mode 100644 index 21fba968..00000000 --- a/COMMUNIQUE-card-step-partial-updates.md +++ /dev/null @@ -1,203 +0,0 @@ -# Communique: Presence-Aware Partial Updates for Card Steps - -**From:** Basecamp CLI team -**To:** BC3 Rails development -**Re:** `steps#update` clears `due_on` and assignees when they are omitted -**Context:** CLI issue #604 — live data loss on released v0.8.0 / v0.8.1 - ---- - -## The problem - -A single-field update to a card step destroys the fields it did not mention. -Reproduced against production on released CLI v0.8.0: - -``` -$ basecamp cards step create "Ship the thing" --card --in --due 2026-08-14 -# step: title set, due_on 2026-08-14 - -$ basecamp assign --step --to me --in -# ok: true — assignee set, and due_on is now null - -$ basecamp cards step update --in --due 2026-08-14 -# ok: true — due_on restored, and assignees are now [] -``` - -There is no client-side spelling of these commands that avoids it, which is -what makes this a contract question rather than a CLI bug. - -## Why the client cannot fix it - -`app/controllers/steps_controller.rb`: - -```ruby -def update - @recording.update! recordable: new_step - @recording.replace_assignees find_assignees, notify: true - - render :show -end - -private - def new_step - Step.new step_params - end - - def step_params - normalized_params.expect(step: [ :title, :due_on ]).tap do |p| - p[:title]&.strip! - end - end - - def assignee_ids - normalized_params.dig(:step, :assignee_ids) || normalized_params.dig(:step, :assignees).to_s.split(",") - end -``` - -Two independent clears, both unconditional: - -1. **`Step.new step_params`** constructs a *fresh* `Step` from whatever the - request carried and swaps it in as the recordable. An omitted `due_on` - yields a `Step` that has none. This is also why the CLI already has to - re-send `title` on every update — omitting it produces a titleless `Step`, - which fails validation with a 400 (CLI #496). -2. **`replace_assignees find_assignees`** runs on every request, not only when - the request mentioned assignees. With `assignee_ids` absent, `assignee_ids` - falls through to `"".split(",")` → `[]`, so `find_assignees` resolves to an - empty relation and every assignee is removed. - -Both clears happen server-side, from the *absence* of a key. No client change -reaches them. Omitting a field and explicitly clearing it are the same request -on the wire today, so a client cannot express "leave this alone" at all — -including a client that made every field nullable specifically to draw that -distinction. - -The only client-side workaround is read-modify-write: `GET` the step and echo -back every field on every mutation. That is racy (it clobbers a concurrent -edit between the read and the write), it doubles the request count for every -step mutation, and each API consumer has to reimplement it. We would rather -not, and #604's original framing — that the CLI owns preservation — was wrong. - -## What `cards#update` already does - -`app/controllers/kanban/cards_controller.rb` has half of the answer already: - -```ruby -def update - @recording.update! recordable: @recording.recordable.changing(card_update_params) - @recording.replace_assignees(find_assignees, notify: notify_assignees_param) if update_assignees? -end - -private - def update_assignees? - params[:kanban_card].has_key?(:assignee_ids) || params[:kanban_card].has_key?(:assignees) - end - - def card_update_params - # sets due_on to nil if it's not included in the params - { due_on: nil }.merge(card_params) - end -``` - -Two things differ from steps: - -- **Assignees are presence-aware.** `update_assignees?` tests `has_key?`, so an - omitted key leaves assignees untouched while an explicit `assignee_ids: []` - still clears them. This is exactly the behaviour steps need, and the - mechanism already exists in the codebase. -- **The recordable is mutated, not replaced.** `recordable.changing(...)` - preserves attributes the request did not mention, so `title` and `content` - round-trip on cards. Steps construct a new `Step`, so nothing round-trips. - -`due_on` is the exception: cards nil it deliberately when omitted, per the -comment on `card_update_params`. So cards have the same `due_on` behaviour that -#604 reports for steps — it is intentional there, presumably because the card -form always submits the field. We are not assuming that intent extends to a -JSON API client, and we would rather ask than guess. - -## What we are asking for - -**Make `steps#update` presence-aware, on both fields.** - -```ruby -def update - @recording.update! recordable: @recording.recordable.changing(step_params) - @recording.replace_assignees(find_assignees, notify: true) if update_assignees? - - render :show -end - -private - def update_assignees? - normalized_params[:step].has_key?(:assignee_ids) || normalized_params[:step].has_key?(:assignees) - end -``` - -Resulting contract, for a `PUT`/`PATCH` to a step: - -| Request contains | `due_on` | assignees | -|---|---|---| -| neither key | unchanged | unchanged | -| `due_on: "2026-08-14"` | set | unchanged | -| `due_on: null` | cleared | unchanged | -| `assignee_ids: [1,2]` | unchanged | set to 1,2 | -| `assignee_ids: []` | unchanged | cleared | - -Omission means "leave alone"; an explicit `null` / `[]` means "clear". That is -the distinction the wire format can carry and the current controller cannot. - -If `changing` on the recordable is the wrong shape for `Step`, merging the -permitted params over the current recordable's attributes gets to the same -place — the requirement is only that an absent key does not participate. - -### Tests we would ask for - -Three cases, because two of them are the reported bug and the third is the one -a naive fix breaks: - -1. **Due-only preservation** — `PUT` with `assignee_ids` and no `due_on`; the - existing `due_on` survives. -2. **Assignee-only preservation** — `PUT` with `due_on` and no assignee key; - the existing assignees survive. -3. **Explicit clear still clears** — `PUT` with `due_on: null` clears the due - date; `PUT` with `assignee_ids: []` removes every assignee. A fix that - guards on blankness rather than key presence passes 1 and 2 and silently - breaks this one. - -### Would `title` come along? - -`changing` would also make `title` optional on update, which would let the CLI -drop the extra `GET` that #496 added purely to re-send an unchanged title. Not -required, and not what this is about — but it falls out of the same change, and -we would happily delete that workaround. - -## Why this is worth doing server-side - -A deployed BC3 fix stops the data loss for every already-installed v0.8.x -client, with no CLI release, no SDK release, and nothing for users to upgrade. -The reverse is not true: no client release can fix it at all. - -## Impact on existing behavior - -For any client that sends the full object on every update — which is what the -web UI's form does — nothing changes. What changes is that a client which -*omits* a field stops having it cleared. Any caller relying on omission-clears -would be affected; we do not know of one, and it is not a documented behaviour -in [bc3-api](https://github.com/basecamp/bc3-api). - -## Follow-on, on our side - -Once the server honours the distinction, we would pointerize `due_on` on the -SDK's `UpdateStepRequest` — mirroring `UpdateCardRequest`, which was -pointerized in SDK v0.10.0 — so a client can *express* omit-versus-clear. That -change is inert until the server-side fix lands: a nil `*string` is omitted -from the JSON either way, and today's controller clears on that omission -regardless. - ---- - -**Question for BC3:** Is presence-awareness the shape you want here, or is the -wholesale replace in `steps#update` load-bearing for something we cannot see -from outside? And is `card_update_params`' deliberate `due_on: nil` still -intended for JSON API callers, or is that form-shaped behaviour that cards -would also want revisited? diff --git a/COMMUNIQUE-inline-attachments-api.md b/COMMUNIQUE-inline-attachments-api.md deleted file mode 100644 index 8a3354a2..00000000 --- a/COMMUNIQUE-inline-attachments-api.md +++ /dev/null @@ -1,156 +0,0 @@ -# Communique: Inline Attachment Metadata in Recording API Responses - -**From:** Basecamp CLI team -**To:** BC3 Rails development -**Re:** Surfacing `` metadata as structured data in API responses -**Context:** CLI PR #326 — `basecamp attachments download` - ---- - -## The problem - -The CLI needs to let agents and humans download inline file attachments -(images, PDFs, etc.) embedded in messages, todos, cards, and documents. -Today the only way to discover these attachments is to parse the HTML body -and regex out `` elements: - -```html - - -``` - -This works, but it's fragile client-side work that every API consumer has -to replicate independently — and it depends on Trix/Basecamp internal HTML -conventions that aren't part of the documented API contract. - -## What we're doing now (client-side) - -The CLI's `richtext.ExtractAttachments(html)` function: - -1. Regex-scans for `` opening tags -2. Skips mentions (`content-type="application/vnd.basecamp.mention"`) -3. Skips tags without `href` (not downloadable) -4. Extracts `href`, `filename`, `filesize`, `content-type`, `sgid` - -This produces an `[]InlineAttachment` array that the CLI surfaces as an -`inline_attachments` field in show command responses and uses as the -download manifest for `basecamp attachments download`. - -### Pain points - -**Content field ambiguity.** The rich-text body lives in different fields -depending on recording type: - -| Type | Plain-text field | Rich HTML field | -|----------|-----------------|-----------------| -| Todo | `content` | `description` | -| Message | `subject` | `content` | -| Card | — | `content` | -| Document | `title` | `content` | - -The CLI has to sniff both `content` and `description`, check which one -contains HTML, and pick the right one. This is the kind of thing that -breaks silently. - -**Storage URL opacity.** The `href` values in `` tags are -storage URLs (`https://storage.3.basecamp.com/...`). The SDK rewrites -these through the API host for auth, then follows a redirect to a signed -S3 URL. This two-hop dance works, but the URLs aren't guaranteed stable — -they're an implementation detail of how Trix stores blob references. - -**No discoverability.** An API consumer can't tell whether a recording has -inline attachments without fetching and parsing the full HTML body. For -agents that want to decide whether to download images for multimodal -analysis, this is a wasted round-trip. - -## Proposed API enhancement - -Add an `inline_attachments` array to recording responses that contain rich -text. Return it alongside the existing `content`/`description` fields. - -```json -{ - "id": 789, - "type": "Message", - "subject": "Q4 Report", - "content": "

See attached: report.pdf

", - "inline_attachments": [ - { - "sgid": "BAh7CEk", - "filename": "report.pdf", - "content_type": "application/pdf", - "byte_size": 12345, - "download_url": "https://3.basecampapi.com/123/blobs/abc/download/report.pdf" - } - ] -} -``` - -Fields: - -- **`sgid`** — the signed global ID (already in the HTML, used for - ActionText references) -- **`filename`** — original upload filename -- **`content_type`** — MIME type -- **`byte_size`** — integer, not string (the HTML `filesize` attribute is - a string today) -- **`download_url`** — a stable API-routable URL that the client can GET - with auth headers, rather than a raw storage URL that requires - rewriting. Ideally the same URL shape that `Upload#download_url` already - returns. - -### Scope - -Only file attachments. Mentions (`application/vnd.basecamp.mention`) are -excluded. Tags without a downloadable blob reference are excluded. - -### Which recording types - -Any type whose API response includes a rich-text HTML body: - -- `Message` (field: `content`) -- `Todo` (field: `description`) -- `Kanban::Card` (field: `content`) -- `Document` (field: `content`) -- `Comment` (field: `content`) -- `Question::Answer` (field: `content`) - -The field would appear only when attachments are present (empty array or -omitted when none). - -## What this unblocks - -- **CLI:** Drop the regex parser, use structured data, remove the - content-vs-description sniffing -- **SDK:** Add `InlineAttachments` field to recording structs -- **Agents:** Discover attachments from list/show responses without - parsing HTML — enables "does this message have images I should look at?" - decisions -- **Third-party integrations:** Any API consumer that wants to mirror or - process attachments gets a stable contract instead of HTML scraping - -## Impact on existing behavior - -Additive. The HTML body continues to contain `` elements -as before. The new field is additional structured metadata derived from -the same source. No breaking changes. - -## Migration path - -If this ships, the CLI can: - -1. Check for `inline_attachments` in the API response -2. Fall back to `richtext.ExtractAttachments(html)` when absent (older - API versions, or types not yet covered) -3. Eventually remove the regex path once the API field is universal - ---- - -**Question for BC3:** Is this something that could be derived at the -serializer level (walking the ActionText body's `` nodes -and emitting structured metadata), or does it need deeper plumbing through -the blob/attachment infrastructure?