Search: model content/description as required and nullable, add the plain-text excerpts - #487
Conversation
jeremy
added
bug
github-actions
Bot
added
typescript
Spec Change ImpactSummary of Changes
SDK RegenerationAll SDKs need to be regenerated to reflect the updated spec. Breaking API Change?Yes, this is a breaking change because the SDK Update Checklist
|
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Models the search-result projection more faithfully by making SearchResult.content / SearchResult.description required and nullable (keys always present, values always null) and by adding the missing highlighted excerpt fields plain_text_content / plain_text_description (optional, non-nullable).
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:
- Update
SearchResult.contentandSearchResult.descriptionto be required-on-wire while allowingnullacross SDKs. - Add
plain_text_content/plain_text_descriptionas optional excerpt fields and document their highlighted HTML-fragment semantics. - Update fixtures and add/adjust tests to pin required-vs-optional decoding behavior and new excerpt fields.
Reviewed changes
Copilot reviewed 7 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
spec/basecamp.smithy |
Marks content/description as @required and adds plain_text_* members with contract documentation. |
spec/smithy-build.json |
Applies OpenAPI patching so content/description become ["string","null"] with x-go-type: "*string". |
openapi.json |
Regenerated OpenAPI reflecting required+nullable content/description and new plain_text_* fields. |
spec/fixtures/search/results.json |
Fixes the search results fixture to match the real projection: null content/description plus appropriate plain_text_*. |
go/pkg/generated/client.gen.go |
Regenerated Go model: Content/Description become *string (non-omitempty) and adds PlainText* fields. |
go/pkg/basecamp/search.go |
Updates Go wrapper SearchResult and conversion to preserve required-null semantics and surface the excerpt fields. |
go/pkg/basecamp/search_test.go |
Updates Go tests to assert Content/Description are nil and excerpts contain <mark class="circled-text">. |
python/src/basecamp/generated/types.py |
Regenerated Python TypedDict: content/description required as `str |
ruby/lib/basecamp/generated/types.rb |
Regenerated Ruby type: content/description added to required_fields and preserved as nil on serialization. |
ruby/lib/basecamp/generated/metadata.json |
Regenerated Ruby metadata timestamp. |
typescript/src/generated/openapi-stripped.json |
Regenerated TS OpenAPI snapshot reflecting the new schema requirements and fields. |
typescript/src/generated/schema.d.ts |
Regenerated TS types: `content: string |
typescript/src/generated/metadata.ts |
Regenerated TS metadata timestamp. |
swift/Sources/Basecamp/Generated/Models/SearchResult.swift |
Regenerated Swift model: content/description required init params (String?) and optional plainText* fields. |
swift/Tests/BasecampTests/SearchResultDecodeTests.swift |
Adds Swift decoding/encoding tests that pin required-key semantics for content/description and optional excerpts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 84b8cac5dd
ℹ️ 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".
| /// Optional and non-nullable: emitted only when the underlying recordable | ||
| /// responds to `content`, so a result whose type has no content attribute | ||
| /// omits the key entirely rather than sending null. | ||
| plain_text_content: String |
There was a problem hiding this comment.
This field addition synchronizes the Smithy model with BC3's search projection, but the commit leaves spec/api-provenance.json and its synchronized copies unchanged at the previous revision. That makes the conformance baseline omit the upstream revision audited for this contract; update the BC3 revision/date and run make provenance-sync and the version synchronization required for an upstream sync.
AGENTS.md reference: AGENTS.md:L263-L270
Useful? React with 👍 / 👎.
| @required | ||
| content: String |
There was a problem hiding this comment.
Once content and description become required, existing nonempty /search.json response stubs such as typescript/tests/services/search.test.ts:28-64 and ruby/test/basecamp/services/search_service_test.rb:13-17,33 describe payloads that violate the new contract because they omit both keys. These tests will continue passing against impossible responses and do not verify that the required null projections flow through; add content: null and description: null to every affected response stub.
AGENTS.md reference: AGENTS.md:L310-L315
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="swift/Sources/Basecamp/Generated/Models/SearchResult.swift">
<violation number="1" location="swift/Sources/Basecamp/Generated/Models/SearchResult.swift:114">
P2: A response containing `plain_text_content: null` or `plain_text_description: null` is accepted as if the field were absent, despite both fields being non-nullable in the SearchResult schema. Allow missing keys explicitly, but use `decode(String.self, forKey:)` when a key is present so an explicit null fails decoding.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| self.plainTextContent = try container.decodeIfPresent(String.self, forKey: .plainTextContent) | ||
| self.plainTextDescription = try container.decodeIfPresent(String.self, forKey: .plainTextDescription) |
There was a problem hiding this comment.
P2: A response containing plain_text_content: null or plain_text_description: null is accepted as if the field were absent, despite both fields being non-nullable in the SearchResult schema. Allow missing keys explicitly, but use decode(String.self, forKey:) when a key is present so an explicit null fails decoding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At swift/Sources/Basecamp/Generated/Models/SearchResult.swift, line 114:
<comment>A response containing `plain_text_content: null` or `plain_text_description: null` is accepted as if the field were absent, despite both fields being non-nullable in the SearchResult schema. Allow missing keys explicitly, but use `decode(String.self, forKey:)` when a key is present so an explicit null fails decoding.</comment>
<file context>
@@ -3,64 +3,144 @@ import Foundation
+ self.descriptionAttachments = try container.decodeIfPresent([RichTextAttachment].self, forKey: .descriptionAttachments)
+ self.inheritsStatus = try container.decodeIfPresent(Bool.self, forKey: .inheritsStatus)
+ self.parent = try container.decodeIfPresent(RecordingParent.self, forKey: .parent)
+ self.plainTextContent = try container.decodeIfPresent(String.self, forKey: .plainTextContent)
+ self.plainTextDescription = try container.decodeIfPresent(String.self, forKey: .plainTextDescription)
+ self.status = try container.decodeIfPresent(String.self, forKey: .status)
</file context>
| self.plainTextContent = try container.decodeIfPresent(String.self, forKey: .plainTextContent) | |
| self.plainTextDescription = try container.decodeIfPresent(String.self, forKey: .plainTextDescription) | |
| if container.contains(.plainTextContent) { | |
| self.plainTextContent = try container.decode(String.self, forKey: .plainTextContent) | |
| } else { | |
| self.plainTextContent = nil | |
| } | |
| if container.contains(.plainTextDescription) { | |
| self.plainTextDescription = try container.decode(String.self, forKey: .plainTextDescription) | |
| } else { | |
| self.plainTextDescription = nil | |
| } |
84b8cac to
3e36e73
Compare
github-actions
Bot
added
enhancement
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd43995d6c
ℹ️ 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".
| @@ -0,0 +1 @@ | |||
| /Users/jeremy/Worktrees/basecamp/basecamp-sdk/feat/wormholes/typescript/node_modules No newline at end of file | |||
There was a problem hiding this comment.
On every checkout outside the author's workstation, this absolute symlink points to a nonexistent directory. TypeScript commands cannot use it until npm ci removes and replaces the tracked path, which then leaves the working tree modified; commands that check for typescript/node_modules before installation also treat it as missing. Keep dependencies untracked rather than committing a link to /Users/jeremy/....
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ruby/test/basecamp/services/search_service_test.rb">
<violation number="1" location="ruby/test/basecamp/services/search_service_test.rb:45">
P2: `assert_nil result[1]["plain_text_content"]` passes even if `plain_text_content` were present with value nil — it doesn't verify the field is actually absent from the payload. The contract says plain_text_* fields are optional and non-nullable (emitted only when available, never null), so absent is the correct state to assert.</violation>
</file>
<file name="typescript/node_modules">
<violation number="1" location="typescript/node_modules:1">
P2: This adds a machine-local absolute symlink target for `typescript/node_modules`, which will be broken on other checkouts and can leave the working tree dirty after dependency installation. It would be safer to remove this tracked path and let `npm ci`/package manager setup create `node_modules` locally.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| assert_nil r["description"] | ||
| end | ||
| assert_includes result[0]["plain_text_content"], "circled-text" | ||
| assert_nil result[1]["plain_text_content"] |
There was a problem hiding this comment.
P2: assert_nil result[1]["plain_text_content"] passes even if plain_text_content were present with value nil — it doesn't verify the field is actually absent from the payload. The contract says plain_text_* fields are optional and non-nullable (emitted only when available, never null), so absent is the correct state to assert.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ruby/test/basecamp/services/search_service_test.rb, line 45:
<comment>`assert_nil result[1]["plain_text_content"]` passes even if `plain_text_content` were present with value nil — it doesn't verify the field is actually absent from the payload. The contract says plain_text_* fields are optional and non-nullable (emitted only when available, never null), so absent is the correct state to assert.</comment>
<file context>
@@ -27,6 +33,16 @@ def test_search
+ assert_nil r["description"]
+ end
+ assert_includes result[0]["plain_text_content"], "circled-text"
+ assert_nil result[1]["plain_text_content"]
end
</file context>
| assert_nil result[1]["plain_text_content"] | |
| refute result[1].key?("plain_text_content"), "plain_text_content should be absent when not available" |
bd43995 to
bdbc370
Compare
|
Fixed the stub gap — you were right that green proved nothing here. The TypeScript and Ruby search stubs omitted Both stubs now carry Verified the assertions bite. Dropping the two keys back out of the TypeScript stub fails with Rebased onto the post-#483 |
| // Despite the name, this is an HTML fragment: matches are wrapped in | ||
| // <mark class="circled-text"> and the whole thing is truncated to 300 | ||
| // characters by BC3. | ||
| XCTAssertTrue(result.plainTextContent?.contains(#"<mark class="circled-text">"#) ?? false) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
ruby/test/basecamp/services/search_service_test.rb:49
plain_text_contentis documented/treated as an optional omitted key when absent, but this test usesassert_nil result[1]["plain_text_content"], which also passes if the key is present with an explicitnullvalue. To actually pin the optional+non-nullable contract, assert the key is omitted (not present with nil).
assert_includes result[0]["plain_text_content"], "circled-text"
assert_nil result[1]["plain_text_content"]
ruby/test/basecamp/generated_types_test.rb:153
- This test asserts that
plain_text_contentis present, but it doesn’t assert thatplain_text_descriptionis omitted when absent (as opposed to present withnil). Adding an explicitkey?assertion would better pin the optional+non-nullable contract for the newplain_text_descriptionfield.
# The excerpt is the opposite contract: optional and non-nullable.
assert_includes hash["plain_text_content"], "circled-text"
assert_equal %i[app_url content description id title type url],
Basecamp::Types::SearchResult.required_fields
…lain-text excerpts api/searches/show.json.jbuilder renders the recording's own partial and then unconditionally overwrites content and description with nil, to keep the large HTML body out of the search payload. The keys are therefore always present and always null. Both were modeled optional and non-nullable, which is wrong in an observable way: it accepts a payload omitting them, which BC3 never sends. The fields that carry the searchable text — plain_text_content and plain_text_description — were not modeled at all, so consumers had typed, permanently-empty fields and no typed access to what BC3 returns. content and description become required and nullable, following the SearchType.key precedent in smithy-build.json (jsonAdd with type: [string, null] plus x-go-type: *string). The two excerpt fields are added as optional and non-nullable, matching their conditional emission. Despite the name, plain_text_content is not plain text. excerpt_and_highlight_matches converts the rich text with to_plain_text, escapes it with html_escape_once, wraps each query match in <mark class="circled-text"><span></span>…</mark>, and truncates to SNIPPET_SIZE = 300. The doc comments say so on every SDK. The fixture was false: result 0 carried a full HTML content, result 1 a real description, result 2 a full HTML content, and no result carried plain_text_* — the exact shape the jbuilder makes impossible. Rewritten so every result has both nulls plus the appropriate highlighted excerpt. Breaking. Go's Content becomes *string with no omitempty; Python's becomes str | None; TypeScript's string | null; Ruby lists them in required_fields; Swift moves them into the required init position, so positional SearchResult(...) calls break. Kotlin is unaffected — it has no generated SearchResult type, search returns ListResult<JsonElement>. The @deprecated question on content/description is deferred: they are live and meaningful on every other recording shape. Closes #465.
The TypeScript and Ruby search-service stubs omitted content and description entirely. Under the corrected contract those keys are REQUIRED and nullable — BC3 emits json.content nil / json.description nil unconditionally on every result — so the stubs described a payload the API cannot produce. Both suites still passed, because neither SDK validates required fields at runtime: TypeScript's types are compile-time only and Ruby just reads keys. Green therefore proved nothing about the change this PR makes. Stubs now carry content: null and description: null, one result in each carries a plain_text_* excerpt, and both suites assert the discriminating property — the keys are PRESENT and null, not absent — plus that the excerpt is only there for the result type that has the underlying attribute. Verified the assertions bite: dropping the two keys back out of the TypeScript stub fails with "expected undefined to be null".
… stubs I linked typescript/node_modules into this worktree to run the TypeScript suite and then committed it. The link is absolute and points at my own machine, so it is worse than useless to anyone else — and it propagated to the two PRs stacked on this one. It slipped past .gitignore because both rules were written with a trailing slash (`node_modules/`), which matches a directory but not a symlink of the same name. Dropped the slash in typescript/.gitignore and the root conformance rule, and verified: recreating the symlink now leaves `git status` clean. Separately, the Ruby search stubs were still impossible payloads even after gaining content/description. SearchResult.required_fields is [app_url content description id title type url] and the stubs carried neither url nor app_url. Both added. Added test_search_result_preserves_null_content_and_description, mirroring the existing SearchType.key and Wormhole.color/destination_url tests: to_h must keep the explicit nulls rather than compacting them, since a consumer has to tell "the projection stripped this" from "absent". It also pins required_fields itself, so a regeneration that relaxed @required fails here.
dbbe7b2 to
fba66dd
Compare
github-actions
Bot
added
bug
Closes #465. Stacked on #486 → #484.
The contract
app/views/api/searches/show.json.jbuilderrenders the recording's own partial, then:Two different shapes, and the difference matters:
content/description— overwritten unconditionally. Keys always present, values always null → required and nullable. Optional+nullable would accept a payload that omits them, which BC3 never sends.plain_text_content/plain_text_description— emitted conditionally → optional and non-nullable. A result whose recordable has no such attribute omits the key rather than sending null.Nullability follows the
SearchType.keyprecedent inspec/smithy-build.json(jsonAdd:type: ["string","null"]+x-go-type: "*string").The name lies, and the docs say so
plain_text_contentis not plain text.excerpt_and_highlight_matches(searches_helper.rb:49) doesto_plain_text→html_escape_once→ wraps each query match in<mark class="circled-text"><span></span>…</mark>(mark_hits_in, skipped for stop-word-only matches) → truncates toSNIPPET_SIZE = 300. It is an HTML fragment. Every SDK's doc comment states this.This is breaking — #465 said additive
The issue's point 3 claimed "Additive and read-only, so no wire change and no breaking-change label". That holds for the two new fields, not for adding
@requiredto two existing ones. Correction posted on the issue.Content string \json:"content,omitempty"``Content *string \json:"content"``content: NotRequired[str]content: str | Nonecontent?: stringcontent: string | nullrequired_fieldspublic var content: String?, defaultedpublic let content: String?— moves into the required init positionSearchResult;searchreturnsListResult<JsonElement>(pre-existing typing gap)The fixture was false
spec/fixtures/search/results.jsonhad result 0 with full HTMLcontent, result 1 with a realdescription, result 2 with full HTMLcontent, and noplain_text_*anywhere — the exact shape the jbuilder makes impossible. Anything asserting against it was pinning fiction.Rewritten so all three results carry
content: nullanddescription: nullplus the appropriate highlighted excerpt:plain_text_contentfor the Message and Comment,plain_text_descriptionfor the Todo, each with real<mark class="circled-text">markup.Verification
go/pkg/basecamp/search.gois a hard gate —make go-check-wrapper-driftrequires every generated JSON tag on the wrapper and every field assigned insearchResultFromGenerated. Wrapper updated with both new fields and the pointer change; gate reports83 pairs walked, 1119 generated fields verified.Content == nil/Description == niland that the excerpts carry<mark class="circled-text">, rather than the old full-HTML strings.swift/Tests/BasecampTests/SearchResultDecodeTests.swiftpins the discriminating half of required-vs-optional: a payload omittingcontent(ordescription) must fail to decode withDecodingError.keyNotFoundnaming that key — an assertion that would pass vacuously under optional+nullable. Also pins that re-encoding keeps the nulls on the wire and thatplain_text_*stay absent when not sent. Swift generates exactly the right thing here:try container.decode(String?.self, forKey:)(key required, value nullable) versusdecodeIfPresentfor the excerpts.make checkexits 0.Deferred
The
@deprecatedquestion oncontent/description. They are permanently null in this projection but live and meaningful on every other recording shape; worth its own look rather than riding along.Summary by cubic
Models search results so
contentanddescriptionare required and nullable, and adds optionalplain_text_content/plain_text_descriptionwith 300‑char highlighted excerpts. Updates OpenAPI/Smithy, fixtures, tests, and all SDKs; breaking for consumers treatingcontent/descriptionas optional.Migration
plain_text_content/plain_text_descriptionfor snippets; they are HTML fragments with<mark class="circled-text">highlights.Content/Descriptionare*string(required); readPlainTextContent/PlainTextDescription.content/descriptionare requiredstring | null;plain_text_*are optional.content/descriptionare inrequired_fields;to_hpreserves explicit nulls.content/descriptionare required init params; payloads omitting them fail to decode;plainText*remain optional.Bug Fixes
content: null/description: null; tests assert highlighted excerpts and correct omission when not applicable.url/app_url; new test pinsrequired_fieldsand keeps nulls into_h.node_modulessymlink; fixed.gitignorerules to avoid re-adding it.Written for commit fba66dd. Summary will update on new commits.