Repository navigation
Let the Recordings tag reach the Rust generator - #928
Merged
Merged
Conversation
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. |
There was a problem hiding this comment.
🟢 Approved
The six operations correctly resolve through their Recordings tags without changing generated service placement.
Pull request overview
Removes redundant Rust generator overrides so recording operations derive their service from the shared OpenAPI tag.
Changes:
- Deletes six
Recordingsoperation-service overrides. - Aligns Rust service resolution with other SDK generators.
[!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 |
|---|---|
rust/generator/names.toml |
Removes redundant recording service overrides. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
The one red check here is inherited from
That is byte-for-byte the failure Deliberately not fixing it in this PR: both would add the same registry row and the second to merge would conflict, and a hotfix for a red The Rust change's own gates are green — |
Rust's service_for consults names.toml [operation_services] before the tag, so the six recording overrides meant Rust alone kept deciding these operations' service from a hand-written table while the other five generators followed the tag #922 introduced. The values agree today, so nothing moves; the next retag of these operations would have moved five SDKs and left Rust behind. #922 removed the equivalent dead splits from the tag-keyed generators and left these in place, reasoning that they were live rather than dead. They are live — that was the problem, not the reason to keep them. Regenerating without them is byte-identical, and the operation-assignment and service-inventory parity gates stay clean.
jeremy
force-pushed
the
sdk/rust-recordings-tag-precedence
branch
from
September 18, 2026 07:31
6aaa411 to
776ec8d
Compare
This was referenced Sep 18, 2026
jeremy
added a commit
that referenced
this pull request
Sep 18, 2026
* Tag a recording's event history as Recordings, not Automation #922 gave the six recording-lifecycle operations their own Recordings tag because basecamp/mcp's catalog.Load builds one MCP domain tool per OpenAPI tag, so a recording operation tagged Automation folds basecamp_recordings into basecamp_admin. ListEvents is the same operation class and the sweep missed it. GET /{accountId}/recordings/{recordingId}/events.json is a recording's own change history — the timeline of the very lifecycle transitions #922 moved (spotlight, trash, archive). It is one domain with them, and it is not automation: nothing schedules it, nothing subscribes to it, it reports what already happened to one recording. The URL prefix is not the argument, and should not be read as one. Operations under /recordings/ are tagged by what they are, not by where they sit: the same prefix carries Bookmarks, Boosts, BubbleUps, ClientFeatures, Messages, People and Schedule operations. ListEventBoosts, at /recordings/{recordingId}/events/{eventId}/boosts.json, is tagged Boosts even though it hangs off this very sub-resource — which is the split working as intended, per thing rather than per path. Three operations under the prefix deliberately stay Automation. EnableTool, DisableTool and RepositionTool sit at /{accountId}/recordings/{toolId}/position.json: the path label is toolId, not recordingId. They are dock-tool operations that happen to share the prefix. The Events service survives the retag rather than dissolving into it. Each tag-keyed generator looks an operation's service up in SERVICE_SPLITS under its tag, so 'Events' => %w[ListEvents] under Automation goes unreachable the moment the tag changes; the entry moves to a Recordings key in all five (ruby, python, typescript, swift, kotlin) rather than being deleted as #922's Recordings entries were. Deleting it is not a rename: ListEvents would fall through to the tag-derived Recordings service, EventsService would stop being emitted entirely, and RecordingsService would carry two `list` methods. That was checked by regenerating Ruby without the entry, not inferred. Rust needs no change, and the reason is the inverse of #928's. Its Naming::service_for consults names.toml's [operation_services] before the tag, so #928 removed the six Recordings rows there because they merely restated the tag. `ListEvents = "Events"` states something the tag cannot: the service differs from the tag both before and after this change, which is exactly what that table is for. Regenerated with make generate. All seven SDKs come back byte-identical — ruby, python, typescript, swift, kotlin, rust and go drift checks are clean, as are the operation-assignment and service-inventory parity gates. The artifacts that copy the tag verbatim carry the whole diff: openapi.json, TypeScript's vendored openapi-stripped.json, the go route table's resource field, and catalog.json's tag for this one operation. behavior-model.json is unchanged; tags live only in the OpenAPI projection. The Automation tag remains a catch-all — templates, webhooks, dock tools, questionnaires, lineup markers and search share it. Whether that bucket should be split is a real question and not this one; this PR moves the operation #922's own premise already covers. * Pin the events route's resource label in the router test Match.Resource is public and comes straight from the route table's resource field, which is the tag. #922 added a wantResource column and filled it in for three recording routes for exactly this reason; the events route moved Automation -> Recordings in the same way and nothing asserted it. Reverting the label in url-routes.json fails this case and only this case. * Say which generators reach the Events service through the tag, and which does not The note read as if every generator resolved ListEvents through SERVICE_SPLITS. Rust does not: names.toml's [operation_services] is consulted by operationId first, which is why its row is unchanged here and still load-bearing. Comment only — openapi.json is unaffected. * Scope #922's paragraph to the six it describes Adding ListEvents to the tag's heading line made "these ops" in the paragraph below read as covering it, which is wrong in the one way that matters: the six route to the Recordings service, ListEvents keeps its own Events service. Say "those six" and let the ListEvents paragraph carry its own case.
jeremy
added a commit
that referenced
this pull request
Sep 18, 2026
Automation carried 37 operations across templates, the template library, webhooks, dock tools, questionnaires, lineup markers and search — the widest and least coherent tag in the spec, and about to be frozen. basecamp/mcp's catalog.Load builds one MCP domain tool per tag, so at cutover this becomes one tool spanning all seven surfaces, and retagging afterwards moves a tool people are already using. The six new tags are the service grouping the SDKs already generate. Every tag-keyed generator resolves an operation's service through SERVICE_SPLITS under its tag, and each of these already had its own entry there, so the tag was the only thing holding them together. Tags move; services do not. All seven SDKs regenerate byte-identical. Checkins is a merge, not a new tag. #922 gave six question operations that tag and noted that folding in the questionnaire/question/answer CRUD was a follow-up left to the maintainers; this is it. Both halves already generated into one CheckinsService, so only the tag disagreed. Dock is named for the project tool strip rather than for the Tools service these route to: a consumer building one tool per tag would otherwise get a tool called "tools", and the operations are about the strip, not about the message board or to-do set behind each entry. ListLineupMarkers keeps its Automation service. It was the one operation the tag reached without a SERVICE_SPLITS entry, so it fell through to the tag-derived service while its three siblings were split out, and it has always generated into AutomationService. Retagging it silently would move a public method between services, so each generator now carries an explicit 'Lineup' => { 'Automation' => [ListLineupMarkers] } and Rust an explicit ListLineupMarkers = "Automation" row. Merging it into LineupService is a breaking change and a separate decision. Restating entries are deleted, load-bearing ones are not. The Templates, Webhooks, Search, Lineup and Checkins entries now name their own tag and go dead, as #928's six did; the Tools entry names a service that differs from its new Dock tag and moves with it, as #929's Events entry did. The same distinction drives Rust: 29 names.toml rows restate their tag and are removed, the seven Tools rows do not and stay. The Automation tag no longer exists. openapi.json has no top-level tag registry, so nothing declares it and nothing is left empty.
jeremy
added a commit
that referenced
this pull request
Sep 18, 2026
* Split the Automation catch-all into six real domains Automation carried 37 operations across templates, the template library, webhooks, dock tools, questionnaires, lineup markers and search — the widest and least coherent tag in the spec, and about to be frozen. basecamp/mcp's catalog.Load builds one MCP domain tool per tag, so at cutover this becomes one tool spanning all seven surfaces, and retagging afterwards moves a tool people are already using. The six new tags are the service grouping the SDKs already generate. Every tag-keyed generator resolves an operation's service through SERVICE_SPLITS under its tag, and each of these already had its own entry there, so the tag was the only thing holding them together. Tags move; services do not. All seven SDKs regenerate byte-identical. Checkins is a merge, not a new tag. #922 gave six question operations that tag and noted that folding in the questionnaire/question/answer CRUD was a follow-up left to the maintainers; this is it. Both halves already generated into one CheckinsService, so only the tag disagreed. Dock is named for the project tool strip rather than for the Tools service these route to: a consumer building one tool per tag would otherwise get a tool called "tools", and the operations are about the strip, not about the message board or to-do set behind each entry. ListLineupMarkers keeps its Automation service. It was the one operation the tag reached without a SERVICE_SPLITS entry, so it fell through to the tag-derived service while its three siblings were split out, and it has always generated into AutomationService. Retagging it silently would move a public method between services, so each generator now carries an explicit 'Lineup' => { 'Automation' => [ListLineupMarkers] } and Rust an explicit ListLineupMarkers = "Automation" row. Merging it into LineupService is a breaking change and a separate decision. Restating entries are deleted, load-bearing ones are not. The Templates, Webhooks, Search, Lineup and Checkins entries now name their own tag and go dead, as #928's six did; the Tools entry names a service that differs from its new Dock tag and moves with it, as #929's Events entry did. The same distinction drives Rust: 29 names.toml rows restate their tag and are removed, the seven Tools rows do not and stay. The Automation tag no longer exists. openapi.json has no top-level tag registry, so nothing declares it and nothing is left empty. * Stop naming Automation as an example of a coarse tag SPEC section 5.1 opens by naming three coarse tags the generators split. Automation is no longer one of them. Card Tables is — five services out of one tag — so the example keeps its point. * Move ListLineupMarkers onto LineupService, closing the Automation service The previous commit retagged this operation Lineup but carried an explicit 'Lineup' => { 'Automation' => [ListLineupMarkers] } entry in every generator to hold its service still, because moving a public method between services is a break. Taking the break instead. AutomationService held one method and held it by accident. ListLineupMarkers was the only operation the Automation tag reached without a SERVICE_SPLITS entry, so it fell through to the tag-derived service while its three siblings were split into Lineup. Nothing decided that. With no entry carrying it, the retag lands it on Lineup with them and AutomationService stops being emitted: account.automation.list_lineup_markers -> account.lineup.list_lineup_markers in ruby, python, typescript, kotlin, swift and rust. The method name does not change, only the service. Every case is a compile error or a NoMethodError on first call, never a silent wrong answer. MIGRATING.md carries the table. Go is unaffected and is the reason to be confident. Its hand-written surface never had an Automation() accessor — ListLineupMarkers has always been LineupService.ListMarkers there — and that divergence was recorded as a :folded carve-out in check-service-inventory-parity, which synthesized `automation` into Go's inventory so the sets would match. The other six catching up closes the fold for real, so the carve-out goes, along with its rows in SPEC 5.1 and Appendix F. The gate reported it itself the moment the service stopped being generated. That gate's self-test used `automation` as its example of a stale fold carve-out in cases 7 and 15. Those now exercise client_visibility, the surviving one; the mechanism under test is unchanged and only the example moved. The hand-written client wiring moves with the generated services: ruby's Client#automation, python's sync and async accessors, typescript's import, field and defineService call, and typescript's LineupMarker re-export, which now comes from lineup.js. The AutomationService tests fold into the LineupService ones in ruby, typescript and rust rather than being dropped. SPEC section 5 and the Kotlin and Swift READMEs move 55 -> 54 services through doc-constants. Nothing anywhere is named Automation now. * Carry the service drop through Appendix F and two stale asides Appendix F states each SDK's account-scoped surface as plain prose, so only the Swift and Kotlin rows are marked spans doc-constants rewrites. Ruby, TypeScript, Python and Rust still read 55; they lose AutomationService here like everyone else and are 54. sync-doc-constants' rationale still said Python was two short. #732 wired gauges and my_notifications, Appendix F's own Python row says so, and the parity gate counts Python at the full set. Dropped rather than left contradicting the row three lines away. The tags.smithy note said the method becomes LineupService#list. It does not — list_lineup_markers keeps its name and changes only the service it hangs off, which is what the generated services and MIGRATING.md both say. * Document the two public fields that carry the tag Match.Resource and catalog.Operation.Tag both restate an operation's tag verbatim, so both change value for all 37 retagged operations with no compile error anywhere. A Go caller switching on Resource == "Automation" simply stops matching. MIGRATING.md only covered the ListLineupMarkers service move, which is the loud half. Also drop Lineup from README's Automation coverage row. It is listed under Scheduling already, and adding it to a row this change empties implies an Automation grouping still exposes it. * Point the v0.19.0 roster at where that accessor went Its list of unpaginated Python operations names automation.list_lineup_markers, which no longer exists. Renaming it in place would make the v0.19.0 section describe an API v0.19.0 did not have, and that section is read by someone upgrading TO v0.19.0. Naming both instead: what it was there, and where it is now.
This was referenced Sep 18, 2026
Merged
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.
Follow-up to #922, from a Codex review that landed on its final head a few minutes after it merged.
The finding
#922 retagged the six recording operations
Automation→Recordingsand removed the now-unreachableAutomationservice splits from the five tag-keyed generators. It deliberately leftrust/generator/names.toml's[operation_services]entries alone, on the reasoning that Rust's table keys off operationId rather than the tag, so those entries were live rather than dead.They are live. That was the problem, not the reason to keep them.
Naming::service_for(rust/generator/src/naming.rs:152) consultsoperation_servicesfirst and returns on a hit, reaching the tag table only when there is none. So for these six operations Rust alone kept deciding the service from a hand-written table while the other five generators followed the tag. The values agree today, which is why nothing was visibly wrong — but the next correction or retag of these operations would move five SDKs and silently leave Rust behind, which is the drift #922 exists to close.The change
Remove the six overrides. Rust then resolves them through the tag like everyone else.
Verified rather than assumed:
scripts/check-rust-service-drift.shregenerates byte-identical, andcheck-operation-assignment-parityandcheck-service-inventory-paritystay clean.Note on scope
This could not go into #922 — that PR had already merged when the review arrived. Filed here instead of being left as an unresolved thread on a merged PR, where nobody would have picked it up.
Summary by cubic
Removes the six
Recordingsoperation service overrides in the Rust generator so Rust resolves those operations through theirRecordingstag like the other generators.Rust consults
[operation_services]inrust/generator/names.tomlbefore falling back to the tag, so these overrides let Rust drift from the tag silently on a future retag. The values match today, so generated output stays byte-identical and no behavior changes.Written for commit 776ec8d. Summary will update on new commits.