Stop the sparse document PUT from erasing the content (#543)

[jeremy]

Closes #543. First child of #374, and the template #547 (ScheduleEntries) copies — the shape established here is the one that wave repeats, so it is written to be copied rather than to be clever.

Was stacked on #593; that merged as 9459083e while this was in flight, so this is rebased onto main and targets main directly. After that rebase this PR carries no repin of its own — it depends on the repin already on main from #593, which is what makes it safe to ship (see below).

PUT /{accountId}/documents/{documentId} is a full replace, and every SDK shipped a sparse update over it. UpdateDocument becomes ReplaceDocument — breaking, no deprecated alias, the ReplaceTodo precedent (#375) — and the merge-safe update / edit / raw replace triad lands in all six SDKs.

Red proof — what the shipped update puts on the wire

One call, update(title: "Q3 Plan"). It never mentions content. Captured against the base branch and against this one; literal output, pasted from the capture files.

Python

--- BASE (feat/folders-and-repin, shipped generated update) ---
  PUT  /999/documents/456  body={"title": "Q3 Plan"}
        -> content ABSENT: BC3 rebuilds the recordable from the permitted params, so this ERASES the document body

--- THIS BRANCH (merge-safe composite update) ---
  GET  /999/documents/456
  PUT  /999/documents/456  body={"content": "<div>Milestones and owners.</div>", "title": "Q3 Plan"}
        -> content preserved: '<div>Milestones and owners.</div>'

Ruby

--- BASE (feat/folders-and-repin, shipped generated update) ---
  PUT  /999/documents/456  body={"title":"Q3 Plan"}
        -> content ABSENT: BC3 rebuilds the recordable from the permitted params, so this ERASES the document body

--- THIS BRANCH (merge-safe composite update) ---
  GET  /999/documents/456
  PUT  /999/documents/456  body={"content":"<div>Milestones and owners.</div>","title":"Q3 Plan"}
        -> content preserved: "<div>Milestones and owners.</div>"

Go — worth its own run, because Go's Update was a hand-written wrapper in vaults.go, not the generated shape the other five shared:

--- BASE (feat/folders-and-repin, shipped hand-written Update in vaults.go) ---
  PUT  /999/documents/456  body={"title":"Q3 Plan"}
        -> content ABSENT: BC3 rebuilds the recordable from the permitted params, so this ERASES the document body

--- THIS BRANCH (merge-safe composite Update in documents.go) ---
  GET  /999/documents/456
  PUT  /999/documents/456  body={"content":"<div>Milestones and owners.</div>","title":"Q3 Plan"}
        -> content preserved: "<div>Milestones and owners.</div>"

(Go's line is reproduced above with the HTML unescaped for readability; the raw capture prints Go's <div> escaping of the same string. Kotlin, Swift and TypeScript shipped the same generated sparse body — body.title?.let { put(...) } and its analogues — so the two structurally distinct paths are the generated one and Go's, and both are shown.)

Red proof — the tests

Every new test was run against deliberately un-fixed source, in a scratch copy of the tree rather than by mutating the tracked file:

proof	failures against un-fixed code
Python, serializer compacted to drop empties	AssertionError: assert 'content' in {'title': 'Project Overview'} — 3 failed
Python, guards removed (naive document.get(key) or "")	Failed: DID NOT RAISE <class 'basecamp.errors.ApiError'> / AttributeError: 'NoneType' object has no attribute 'get' — 29 failed, 4 passed
TypeScript, putFields with || undefined	AssertionError: expected { title: 'Project Overview' } to have property "content" — 2 failed
TypeScript, guards removed (naive ?? read)	expected the call to reject, but it resolved / AssertionError: expected TypeError: Cannot read properties of null… to be an instance of BasecampError — 25 failed, 4 passed

That last line is the point of requireRecord: un-guarded, a null envelope degrades to a raw TypeError instead of the documented statusless api_error.

The measured contract: title and content are BOTH optional

This is the one place Documents diverges from Todos and Todolists, and the reason it is stated rather than inferred — modelling either field as @required would make the SDK reject a request the server accepts.

claim	evidence
the PUT is a full replace	DocumentsController#update → @recording.update! recording_attributes.merge(recordable: new_document), where new_document is Document.new(params.require(:document).permit(:title, :content)) — a brand-new recordable from the permitted params, swapped in wholesale, so an absent field is nil on the replacement
omitting title is a 200, not a 422	Document#title is super.presence || "Untitled" (app/models/document.rb:7-9); the model declares no validates, and neither Recordable nor Recording presence-validates the recordable's title
omitting content is a 200 that clears	same rebuild; no validation on content either
a body with neither field is a 400	params.require(:document) raises ActionController::ParameterMissing; Rails wrap_parameters only synthesizes the wrapper from a flat body carrying at least one Document attribute name. Pinned upstream by test "publishing a draft document requires the full payload and preserves it" (test/api/documents_controller_api_test.rb:56)

The public API docs say it outright: "A status-only update fails with 400 Bad Request (the document parameters are required), and omitting a field clears its value."

Read-side, the asymmetry inverts

Document.title is @required on the response schema and BC3 can never render it blank, so an absent or null title in a 2xx body is a malformed response, not an empty title — coalescing it to "" and sending that in the full-replace PUT would blank the real title on a call that only touched content. All six refuse it: Kotlin and Swift get it from the decoder (non-optional String), and Go, Python, Ruby and TypeScript check explicitly, because their reads would otherwise yield the string zero value. content is optional on the response schema, so absent there is genuinely empty.

Request optionality and response requiredness are separate facts, modelled separately. Getting that backwards in either direction is a bug: @required on the input would reject a request the server accepts; a tolerant read would write a blank title nobody asked for.

So ReplaceDocumentInput marks neither field @required. Go's Replace refuses a body with neither locally rather than spending a round-trip on the 400; the other five leave it to the server.

Why this was blocked, and why it no longer is

A merge-safe composite PUTs a full representation and names neither subscriptions nor notify. BC3's notify_param defaulted to "custom", so find_subscribers ran where(id: params[:subscriptions]) → where(id: nil) → empty, and every sparse update to a drafted recording reset its subscriber list to creator + updater. The list is also unreadable over the API — only subscription_url is emitted — so the composite could not have preserved it by resending. Shipping before that fix would have preserved title and content while silently unsubscribing everyone.

Fixed in bc3 #12494 (344581a379) and #12501 (2c0dafba13) by Recording::DraftSubscribers, whose predicate is params.key?(:subscriptions) || params.key?(:notify): a request addressing neither keeps the list it found. #593's repin to 2c0dafba13 is what brings that into the SDK's provenance, and it is already on main — this PR reads that provenance rather than moving it, and spec/api-provenance.json is untouched here.

One correction to #543's body: the concern shipped as Recording::DraftSubscribers (app/controllers/concerns/recording/draft_subscribers.rb), not Recording::DraftSubscriberParams.

The overlay does not reintroduce #576

The merge-safe read is the exact defect class #597 is fixing on Todos and Cards: a value read off the GET is a value written by the PUT, on a call that never mentioned the field. Documents reads two string fields, so it uses #597's shared guards rather than a fourth private copy — require_mapping/writable_string, requireRecord/writableString, MergeSafe.require_hash/MergeSafe.writable_string. Go, Kotlin and Swift decode into typed models and need none (Kotlin's client-wide coerceInputValues scalar hole is the known cross-service gap from #597, not something a composite can close).

#597 has not merged yet, so python/src/basecamp/services/_merge_safe.py, ruby/lib/basecamp/services/merge_safe.rb and typescript/src/services/merge-safe.ts appear here as byte-identical copies — verified by shasum -a 256 against fix/composite-containment-576. When #597 lands they rebase to a zero diff. They are not forked and must not be edited here; #543 owns only the Documents call sites.

Deliberately not carried over: a Documents kill fixture. errorRaised and the six runner branches that support it are #597's, and duplicating them — including the Swift requiresHTTPSCrashProbe defect still open on that PR — would be worse than waiting. The guard is covered by native tests in all three languages here, and the shared kill case lands as a follow-up once errorRaised is on main.

Also out of scope, matching #597 exactly: the caller-side mirror. edit hands back a mutable view and a closure assigning 42 walks into the same PUT — but that is a value the caller chose, not one silently substituted for a value they asked to preserve. Ruby normalises nil → "" rather than to_s, so nothing is coerced on the way out.

Surface

SDK	placement
Go	new go/pkg/basecamp/documents.go — Update / Edit / Replace + private replaceDocument owning the hook envelope and the single ReplaceDocumentWithBodyWithResponse call site (SPEC §18 rule 1 carve-out: omitempty cannot express always-send-empty). Read/create/trash stay in vaults.go
TypeScript	src/services/documents-extensions.ts, import swapped in client.ts, split export in index.ts
Python	src/basecamp/services/documents.py — sync + async, DocumentEdit / AsyncDocumentEdit context managers
Ruby	lib/basecamp/services/documents_extensions.rb, prepended via a fourth on_load in lib/basecamp.rb
Kotlin	Documents added to EXTENSIBLE_SERVICES and HAND_WRITTEN_SERVICES; services/DocumentsService.kt; generated-compat/UpdateDocumentBody.kt (the composite's body type, not a deprecated alias — null preserves where the generated body's null omits)
Swift	Sources/Basecamp/DocumentsServiceExtensions.swift; the generated UpdateDocumentRequest.swift became ReplaceDocumentRequest.swift, freeing the name

Full-state serialization: both fields always sent, empties included. On a full-replace endpoint "" is how a clear is expressed — never JSON null (SPEC §18), and never by omission, which would hand the clear back to the server's own rebuild and read as an accident rather than an intent. Three independent attempts to "simplify" that into dropping empties were caught and reverted during this change; each one would have turned a stated clear back into the accident.

status: "active" (draft publish) stays unmodeled and is now said so in the operation doc: BC3 rejects a status-only update for the same reason it rejects an empty body, so publishing needs the full payload alongside it.

Review rounds (Codex, 13 x P2 + 1 CodeQL)

Eleven fixed, two carried to follow-ups, one declined with evidence — five rounds. The Go read-error handling took four passes, and the honest summary is that the first three were the wrong shape, not merely incomplete.

• Go and Kotlin leaked raw decoder exceptions (*json.UnmarshalTypeError, SerializationException) where SPEC §6 wants a statusless non-retryable api_error. The PUT was correctly avoided either way, but a caller switching on *Error/BasecampException would have missed it — and Swift in this same PR already wrapped DecodingError, so the three decoder-backed SDKs disagreed with each other. Both now read through a private fetchDocument (dfa5e7205). Go's is deliberately narrow: a transport or HTTP error passes through untouched, so a 404 stays a 404. Red-proofed:

  --- FAIL: TestDocumentsService_UpdateWrapsADecodeFailureAsStatuslessAPIError (0.01s)
      --- FAIL: .../title_is_a_number (0.00s)
          documents_test.go:656: expected *Error, got *json.UnmarshalTypeError: json: cannot unmarshal number into Go struct field Document.title of type string
      --- FAIL: .../content_is_an_object (0.00s)
          documents_test.go:656: expected *Error, got *json.UnmarshalTypeError: json: cannot unmarshal object into Go struct field Document.content of type string
  

• A missing required title on read — covered by the read-side section above; the guard shipped in all six before the review landed.

• Go's Replace could not express {"title": "", "content": ""} — a legal full replacement that clears both. Zero-value guards collapsed it into the all-nil request, so the length check turned a legal clear into a usage error while the other five SDKs could express it. Both fields are now *string (d4cceb6fa); nil omits, a pointer to "" sends, and the all-nil request stays the usage error standing in for BC3's 400. This is the one Go request here that departs from the zero-value convention, and SPEC §5 says why.

• The decode-error wrapper's allowlist was incomplete — twice (second review round). Document.created_at/updated_at are time.Time, whose UnmarshalJSON returns *time.ParseError, so the encoding/json allowlist leaked. Adding it does not close the class either: content_attachments carries *types.FlexInt dimensions and FlexInt.UnmarshalJSON rejects a non-integral value with a bare fmt.Errorf that is no named type at all. Against the three-type allowlist:

  --- FAIL: TestDocumentsService_UpdateWrapsADecodeFailureAsStatuslessAPIError (0.01s)
      --- FAIL: .../attachment_height_is_non-integral (0.00s)
          documents_test.go:683: expected *Error, got *errors.errorString: flexint: 1024.5 is not an integer
  

  28d6871da therefore classifies by exclusion: pass through an error already carrying the SDK taxonomy (checkResponse maps HTTP status), a transport failure, or a cancelled context; normalize everything else. An allowlist of decoder error types can only be extended, never completed. The test table spans all three shapes on purpose.

• A blank title on read was accepted in five of six. Document#title is super.presence || "Untitled", so BC3 can never render one — "" on a 2xx read is a malformed response, not an empty title, and resending it blanks the real title on a call that only touched content. Go already refused it; the other five only covered absent and null. Kotlin and Swift needed it written by hand for the same reason: their non-optional String refuses absent/null at decode, but "" decodes fine (f7707c6b7).

• A gating-hook error was misclassified as a decode failure. The classifier wrapped the whole of Get, which sits above the gate, so a circuit breaker's own sentinel came back as "does not decode" — breaking errors.Is and misreporting why nothing was sent:

  --- FAIL: TestDocumentsService_UpdatePreservesAGatingHookError (0.00s)
      documents_test.go:866: expected the gate's own error to survive, got *basecamp.Error: GetDocument returned a body that does not decode as a document: circuit breaker open
  

  Rather than teach the classifier to recognize an arbitrary sentinel, it moved below the gate, to the one call site whose only origins are the transport and the decoder. That removes the gate from the candidate set entirely, and fixes the same misclassification for direct Get callers too (f7707c6b7).

• Classifying the Go read error by inspection could not be made to work, in either direction. The allowlist of decoder error types leaked *time.ParseError (from created_at) and then types.FlexInt's bare fmt.Errorf (from an attachment dimension). Inverting it to an exclusion list then leaked a gating hook's sentinel, and then an arbitrary AuthStrategy's — the auth editor runs per request inside the generated client, so it surfaces at the same call site with no HTTP response behind it. Neither set is enumerable.

  406d16943 stops guessing: DocumentsService.Get splits gen.GetDocument from generated.ParseGetDocumentResponse — exactly the two calls GetDocumentWithResponse makes — and normalizes the decode step only, so the origin is known by construction. Everything preflight returns verbatim and errors.Is keeps working. Both are public generated methods and the hook envelope is unchanged, so SPEC §18 rule 1 still holds. The service-drift gate only recognized .gen.*WithResponse, so 2e8c29fbf teaches it the two-stage form — verified still failing when a wire call really is missing:

  === ERROR: Generated operations NOT wrapped by service layer (1) ===
  ReplaceDocument
  

• A whitespace-only title was accepted. Rails' presence delegates to blank?, so BC3 can no more render " " than "". All six now trim before the check.

• The same auth-phase defect in Kotlin and Swift — split out as Isolate the response decoder from the auth/transport phase in Kotlin and Swift BaseService #604, not fixed here. Their AuthStrategy runs inside the request path, so a custom strategy throwing SerializationException/DecodingError while loading credentials is caught by the composite's decode catch. Go could be fixed locally because GetDocumentWithResponse is literally its two halves; Kotlin and Swift have no seam in the composite — the decode happens inside BaseService.request(info, block, parse), one level down and shared by all 243 operations. Wrapping parse there is the right fix, is a strict improvement for every service, and would let Todos, Todolists, Cards and Documents all drop their per-composite catches. That is a cross-cutting change to both SDKs' decode contract and belongs on its own. Residual: a mislabelled message in a scenario needing a custom auth strategy that throws a serialization error; no PUT is issued either way.

• CodeQL java/class-name-matches-super-class — declined. It is the deliberate architecture of every extensible Kotlin service: the accessor must resolve to a subclass of the same name so callers need no import (SPEC §18 rule 5). Four identical alerts are already open on main for TodolistsService, UploadsService, TodosService and CardsService; this is the fifth instance of an accepted pattern, and if we want it gone it should be one repo-level decision covering all five.

SPEC

• §5 gains "Merge-Safe Write Surface (Documents)", including the measured optionality, the subscriber carve-out, and the Go clear-semantics asymmetry.
• §18 gains "Replace-Semantic Operation Naming": Replace* when the server clears what the body omits — including replace-with-declared-carve-outs, because a carve-out is one named field excluded from the swap while a merge is the server preserving everything omitted; Update* when it merges (Messages) or is hybrid (Cards, Cards.update silently erases due_on on every sparse update (BC3 forced-replace) #467). It also names the two operations still carrying the debt: UpdateTodolistOrGroup (method renamed via METHOD_NAME_OVERRIDES, operationId not) and UpdateScheduleEntry (neither renamed nor composited yet, ScheduleEntry write contract: preservedOnOmission trait extension + bc3 url/highlighted guard-and-emit #546/ScheduleEntries triad: ReplaceScheduleEntry + carve-out-aware composites #547).
• §18 "Current composites" gains Documents, noting it needs no METHOD_NAME_OVERRIDES entry — the wire rename makes replace fall out of the ordinary naming algorithm.

Conformance

conformance/tests/documents_write.json — update-merge, edit-clear, replace-omission-clears, dispatched in all six runners (Go / Python / Ruby / TypeScript switches, Kotlin Main.kt, Swift Dispatch.swift). A fixture whose operation has no dispatch case silently skips, so each runner's PASS: lines were read back individually rather than trusting the totals.

Follow-ups filed

• Isolate the response decoder from the auth/transport phase in Kotlin and Swift BaseService #604 — isolate the response decoder from the auth/transport phase in Kotlin and Swift BaseService, the one place the seam exists. Note the same catch-around-the-whole-get already ships on main in Swift's TodolistsServiceExtensions.swift (fetchTodolist, Stop the sparse todolist PUT from erasing the description #574), so Documents matched the shipped precedent rather than diverging from it — Isolate the response decoder from the auth/transport phase in Kotlin and Swift BaseService #604 covers all three call sites in one pass, before ScheduleEntries triad: ReplaceScheduleEntry + carve-out-aware composites #547 copies the shape a fourth time. (Kotlin and Swift composites mislabel an auth-phase decoder failure as a malformed response body #603 was a same-minute duplicate from a parallel agent and is closed in its favour.)
• A Documents kill fixture, once Refuse a malformed GET field instead of writing it back (#576) #597's errorRaised assertion is on main (see above).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 828cfe14bc

ℹ️ 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".

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c83bcae267

ℹ️ 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".

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[jeremy]

Copilot errored out on all four attempts here ("Copilot encountered an error and was unable to review this pull request"), so its pass is missing rather than clean. Codex reviewed twice and CodeQL ran; all five findings are answered above — four fixed in `dfa5e7205` and `d4cceb6fa`, one declined with the four pre-existing main alerts that show it is an accepted pattern. Re-requesting Copilot once.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4cceb6fa9

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 90a4a73f54

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e8c29fbf1

ℹ️ 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".

[Copilot]

🟡 Human review recommended

It is a breaking wire-operation rename spanning the spec, six SDKs, generated artifacts, and conformance, and it deliberately carries byte-identical shared merge-safe helper files from the still-unmerged PR #597, so a human should verify the cross-PR coupling and breaking-change coordination before approval.

Review details

• Files reviewed: 45/63 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

It is a breaking six-SDK wire-operation rename touching generated artifacts and depends on byte-identical shared guard files copied from the still-unmerged #597, whose landing order is an open coordination decision requiring human judgment.

Review details

• Files reviewed: 46/64 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.