Repository navigation
Tag the recordings operations as their own domain, and require a tag on every operation - #922
Merged
Merged
Conversation
…s own domain basecamp/mcp's catalog.Load groups SDK operations into MCP domain tools by their OpenAPI tag, one tag per operation. The six recording-lifecycle operations — ListRecordings, SpotlightRecording, UnspotlightRecording, TrashRecording, ArchiveRecording, UnarchiveRecording — carried the Automation tag, so adopting the SDK's native tag mapping would fold basecamp_recordings into basecamp_admin and silently drop the deliberately-designed recordings tool surface. (The account event-feed surface already got its own EventFeed tag in #898; this closes the matching gap for recordings.) Retag the six under a new Recordings tag, mirroring the Recordings service every SDK generator already emits for exactly these operations. Recording boosts stay under Boosts and the recording timesheet stays under Schedule -> Timesheets, matching the SDK service groupings. Each generator assigns services from its own split table rather than from the OpenAPI tag, so all seven SDKs regenerate byte-identical; the artifacts that copy the tag verbatim change: openapi.json, the go url-routes resource field (which now reads "Recordings" for these routes instead of the stale "Automation"), and the TypeScript vendored openapi-stripped.json. Regenerated via make smithy-build, make url-routes, and the TypeScript generator. behavior-model.json is unchanged: tags live only in the OpenAPI projection.
…e tag The recordings-into-Automation fold happened because nothing enforced that every operation carries its own tag: an operation added to the Smithy model without a matching entry in spec/overlays/tags.smithy reaches openapi.json untagged, and catalog.Load then drops it or folds it into an unrelated domain. This is an accident-class guard — anyone who can add an operation can add a tag; the check exists so they cannot do the first and forget the second. check-required-tags.rb reads the committed openapi.json — the same artifact catalog.Load consumes — and fails if any operation has zero tags or more than one. The allowlist is empty and documented: every Basecamp API operation is a catalog operation and must carry a domain tag; a genuinely tag-less operation would be added by operationId with a reason rather than by weakening the check. It fails closed on a spec that yields no operations, so a truncated openapi.json cannot pass vacuously. The smithy-verify "OpenAPI is up to date" step already proves openapi.json is regenerated from the model, so a dropped tag reaches this gate rather than hiding behind a stale file. test-check-required-tags.rb drives the check with crafted openapi documents (untagged, empty-tagged, multi-tagged, allowlisted, empty, and path-level parameters) since the live run only ever exercises the passing case. Both are wired into make check and the spec-gates CI job (bash/ruby, no toolchain).
Contributor
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
github-actions
Bot
added
github-actions
jeremy
marked this pull request as ready for review
September 18, 2026 06:12
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.
🟡 Changes recommended
The guard ignores valid HEAD, OPTIONS, and TRACE operations, allowing untagged operations to pass.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Retags recording lifecycle operations into their own domain and adds CI enforcement requiring one tag per OpenAPI operation.
Changes:
- Retags six recording operations as
Recordings. - Regenerates OpenAPI-derived artifacts and Go route metadata.
- Adds a required-tags guard, self-tests, and CI integration.
[!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 |
|---|---|
spec/overlays/tags.smithy |
Defines the new Recordings domain tag. |
openapi.json |
Updates six generated operation tags. |
typescript/src/generated/openapi-stripped.json |
Mirrors generated tag changes. |
go/pkg/basecamp/url-routes.json |
Updates recording route resource labels. |
scripts/check-required-tags.rb |
Adds the exactly-one-tag guard. |
scripts/test-check-required-tags.rb |
Tests positive and negative guard behavior. |
Makefile |
Adds guard and self-test targets. |
.github/workflows/test.yml |
Runs both checks in CI. |
Review details
- Files reviewed: 7/8 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.
![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: de867047b7
ℹ️ 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".
The tag gate only visited get/post/put/patch/delete, so an operation on options, head or trace reached openapi.json unvisited: a tagged GET beside an untagged HEAD passed and reported one operation. Visit all eight methods an OpenAPI path item may carry, matching check-projected-examples.rb. The self-test drives the tagged-GET-plus-untagged-HEAD document that used to pass, and asserts an untagged operation is caught on each of the eight.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8cbe60a1f
ℹ️ 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".
Enumerating the operation verbs made a fail-closed gate fail open in the one dimension it exists to hold: the list is only ever as complete as its last edit. It missed options/head/trace, and lengthening it would not have been a fix. OpenAPI 3.2 adds query and additionalOperations, and Smithy's @http trait takes the method as a free-form string it performs no validation on, so this repo's own generator can put a key in a path item that no list anticipates. A Path Item Object's non-operation fields are a closed, spec-defined set and its extensions are x- prefixed. Skip those; everything else is an operation. A field that is neither is now reported by name instead of stepped over. With the verb list restored to eight, the self-test still fails five cases.
An operation tagged [""] satisfied exactly-one while naming nothing, and the generators do not agree what that means: TypeScript reads "" as falsy and files the operation under Miscellaneous, while Ruby and Python read it as truthy and derive a service from the empty string. One blank tag would split the SDKs rather than just mis-file an operation. The sole tag must now be a non-blank string. A non-array tags value counts as untagged instead of raising. Surrounding whitespace still passes, so the rule is about naming a domain and not about trimming.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 18, 2026 06:39
There was a problem hiding this comment.
🔵 Needs a closer look
Generator split mappings remain stale, and the changed Go router classification lacks regression coverage.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
go/pkg/basecamp/url-routes.json:1540
- This changes the public
Match.Resourceclassification returned by the embedded Go router, butgo/pkg/basecamp/url_test.go:7-337does not assertResourcefor any route. Add a recording URL case with expected resourceRecordings(and awantResourceassertion) so this behavioral change is covered rather than only snapshot-regenerated.
spec/overlays/tags.smithy:332 - Retagging these operations makes the existing
Automation -> Recordingsentries unreachable: each tag-based generator looks upSERVICE_SPLITSusing the operation's current tag, then falls back to the tag-derivedRecordingsservice (for example,typescript/scripts/generate-services.ts:904-911). The old recording lists remain in the TypeScript, Ruby, Python, Kotlin, and SwiftAutomationsplit tables, leaving the generator mappings stale even though output happens to stay byte-identical. Remove those dead split entries as part of this retag so the service-assignment configuration remains current.
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…in the router's resource Each tag-keyed generator looked the recording operations up under the Automation split; they carry the Recordings tag now, so the lookup misses and the tag-derived service name produces the same output. The entries are dead configuration describing an assignment that no longer happens. Regenerating without them is byte-identical in all five: ruby, python, typescript, swift and kotlin drift checks all clean, as are the operation-assignment and service-inventory parity gates. Rust's names.toml keys off operationId rather than the tag, so its entries stay live and unchanged. The route table's resource label for recordings went Automation -> Recordings in the same retag, and that field is public on Match while no test asserted it. TestRouterMatch now carries a wantResource column, filled in for the two existing bucket cases and for three recording routes. Reverting the five route labels fails those three.
github-actions
Bot
added
ruby
Member
Author
|
@codex review |
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 18, 2026 07:02
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
There was a problem hiding this comment.
🟢 Approved
The implementation is sound; identified issues are limited to non-blocking documentation and reporting accuracy.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/workflows/test.yml:199
- The PR description says a required-tags failure will not block merging because
Spec Gatesis not a required context. However, this job feeds the requiredConformance Testsfan-in at.github/workflows/test.yml:1206-1225, and that job explicitly fails unlessspec-gatessucceeds. The new check therefore does block merging indirectly; please update the PR description so its operational status matches the workflow.
scripts/check-required-tags.rb:213 - This success message is false when the supported allowlist path is used: case 5 passes an untagged
SomeOp, yet the check reports that all operations carry exactly one tag. Report the guarantee for non-allowlisted operations so future production exceptions do not produce a misleading green result.
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Balanced
The header still said the check reads paths only and would miss webhooks and components.pathItems. It reads both places path items live now; what remains is that it refuses a $ref rather than following one. Say that, and point the generator hole at its issue.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 18, 2026 07:15
There was a problem hiding this comment.
🟢 Approved
The retagging, generated artifacts, generator mappings, router coverage, validation logic, and CI wiring are consistent and no unresolved issues were found.
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- 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: 44e0befbee
ℹ️ 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".
This was referenced Sep 18, 2026
jeremy
added a commit
that referenced
this pull request
Sep 18, 2026
#922 landed a comment reading "tracked in #925" without the registry row that sentence requires, so check-known-defect-issues-open has been failing on main since that merge. The row was written during the review but the PR merged before it was pushed. The gate behaved exactly as designed: a prose promise that an issue owns a gap now fails closed until it has a row that will keep failing if the issue closes while the gap does not.
jeremy
added a commit
that referenced
this pull request
Sep 18, 2026
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
added a commit
that referenced
this pull request
Sep 18, 2026
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
added a commit
that referenced
this pull request
Sep 18, 2026
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
* 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.
This was referenced Sep 18, 2026
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 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.
Why
basecamp-mcp-server builds its MCP tool catalog by grouping SDK operations by their OpenAPI tag — one tag per operation, each tag becoming one domain tool (
basecamp_recordings,basecamp_events, …). An operation with no tag of its own gets dropped or folded into an unrelated domain.Two recently-shipped, deliberately-designed surfaces were exposed to this:
AccountClient.EventFeed()service added in 0.19.0). Already fixed — Account event feed, inbox, and stream tickets: spec operations and generated clients #898 taggedPollEvents/PollInbox/CreateStreamTicketwithEventFeed. No change needed here; noted for completeness.Automation, so adopting the SDK's native tag mapping would foldbasecamp_recordingsintobasecamp_admin. This PR fixes that.What was untagged / mis-tagged
All 265 operations already carry exactly one tag in
openapi.json; the recordings gap was mis-tagging, not a literal absent tag. These six carriedAutomation:ListRecordings,SpotlightRecording,UnspotlightRecording,TrashRecording,ArchiveRecording,UnarchiveRecordingThe tags added
Retagged those six under a new
Recordingstag inspec/overlays/tags.smithy, mirroring theRecordingsservice every SDK generator already emits for exactly these ops. Recording boosts stay underBoostsand the recording timesheet stays underSchedule → Timesheets, matching the SDK service groupings — this is the coherent recordings domain, nothing more.Regeneration
make smithy-build,make url-routes, and the TypeScript generator. Because each SDK generator assigns services from its own split table (not the OpenAPI tag), all seven SDKs regenerate byte-identical — verified: ruby regen produced zero diff, andcheck-operation-assignment-parity(all 6 SDKs),check-service-inventory-parity, andkt-check-driftare all clean. The only committed artifacts that change are the three that copy the tag verbatim:openapi.json(6 operation tags:Automation→Recordings)go/pkg/basecamp/url-routes.json(theresourcefield for recording routes now readsRecordingsinstead of the staleAutomation— a latent consistency fix)typescript/src/generated/openapi-stripped.jsonbehavior-model.jsonis unchanged — tags live only in the OpenAPI projection.Require-tags guard (fail-closed) — implemented
Survey first: 0 untagged operations, 0 multi-tagged — all 265 carry exactly one tag. Because the exception set is empty, the fail-closed check is clean to add, so I implemented it rather than deferring.
scripts/check-required-tags.rbreads the committedopenapi.json(the artifactcatalog.Loadconsumes) and fails if any operation has zero tags or more than one. It's an accident-class guard: it stops a new operation added to the Smithy model without a matchingtags.smithyentry from shipping untagged. Allowlist is empty and documented — every Basecamp API operation is a catalog operation; a genuinely tag-less op would be added by operationId with a reason rather than by weakening the check. It also fails closed on a spec that yields no operations (a truncatedopenapi.jsoncan't pass vacuously).scripts/test-check-required-tags.rbdrives it with crafted specs (untagged, empty-tagged, multi-tagged, allowlisted, empty, path-level parameters) since the live run only exercises the passing case. Both are wired intomake check(check-targets) and the spec-gates CI job (bash/ruby, no toolchain). Adversarial check: stripping a tag from a copy ofopenapi.jsonmakes the gate exit 1 and name the offender.How the guard decides what an operation is
Review caught the first version enumerating five HTTP verbs, so
options,headandtracewere never visited: a tagged GET beside an untagged HEAD passed and reported one operation. The obvious repair is to lengthen the list to the eight verbs OpenAPI 3.1 names. That repairs the three misses and keeps the structure that produced them, so the guard identifies operations the other way round instead.A Path Item Object's non-operation fields are a closed, spec-defined set —
$ref,summary,description,servers,parameters— and its extensions arex-prefixed. The check skips those; every other field is an operation. A field that is neither a known non-operation field nor a readable operation object is reported by name rather than skipped.Why the inversion rather than the longer list, on the specifics of this repo:
trace; 3.2 addsqueryandadditionalOperations.@httptrait takesmethodas a free-form string that Smithy "will use literally and will perform no validation on". The generator that produces this artifact can therefore put a key in a path item that no list anticipates, and a list would skip it in silence.openapi.jsoncontains exactly four path-item keys today (get,post,put,delete) and no non-operation fields at all.The self-test pins it by mutation. Restored to the original five verbs it fails 10 cases; restored to the eight-verb list the reviewers asked for it still fails 5, including untagged
query/purge/notifyand the unreadable-field cases. That gap is the whole argument for the inversion, so it is a test rather than a paragraph.One case is known and deliberately not built for: 3.2's
additionalOperationsis a map of method to Operation, so the check would read it as a single untagged operation and fail naming the field. This artifact declares3.1.0, so it is unreachable; the failure is loud and the remedy is one line, recorded in the script header.What counts as a tag
Review also found that
tags: [""]satisfied "exactly one" while naming no domain. The three generators do not merely mis-file such an operation, they disagree about it: TypeScript'stags?.[0] || "Untagged"reads""as falsy and folds it intoMiscellaneous, while the Ruby and Python equivalents read""as truthy and derive a service from the empty string. A single blank tag would therefore split the SDKs rather than just mis-file an operation.The sole tag must now be a non-blank string; a non-string entry and a non-array
tagsvalue are rejected with it. A tag with surrounding whitespace still passes, pinned by a test — the rule is that the tag names a domain, not that it is trimmed, and stray formatting belongs in the Smithy overlay rather than in this gate.What the guard cannot see
Stated plainly, since "every operation" is the claim:
pathsandwebhooks, the two places a 3.1 document holds path items. A path item behind a$refis refused by name rather than skipped, since its operations cannot be counted without resolving the reference; that also closescomponents.pathItems, which is reachable only through such a reference. Resolving local references is the better end state, but refusing cannot under-count while waiting for it.Automationmis-tagging this PR fixes.openapi.json, not the Smithy model, and relies on smithy-verify's "OpenAPI is up to date" step to keep that artifact honest.@http's method is unvalidated, a model author writingmethod: "HEAD"would have produced an untagged operation that shipped silently. But the six per-language generators each iterate a five-verb list of their own, so such an operation would be dropped from the generated services outright — a larger hole than the tag one, and directly upstream of anything that builds a catalog from SDK operations. Deliberately out of scope here and filed as Generators iterate a five-verb list, so an operation on any other HTTP method is silently dropped from every SDK #925; this guard would at least name such an operation.Interaction with the shadow-mode check
Worth separating, because the two are easy to conflate: the shadow-mode notice on this PR belongs to the Sensitive Change Gate, which fires on control-plane paths (here,
.github/workflows/test.yml) and is informational until activated. It says nothing about the required-tags gate, which is a live step in the Spec Gates job and reds the PR on a real failure. What is true is narrower:mainis protected by themain-gateruleset, andSpec Gatesis not among its required status check contexts as of this PR. So the required-tags gate reds a pull request without blocking its merge — which argues for the strict reading, since a guard that can be walked past should at least be loud and correct when it fires. Making it binding means addingSpec Gatesto that ruleset's required contexts, a repository settings change this PR cannot make.Dead split entries and router coverage
Two consequences of the retag itself, both found in review:
Automationservice split. They carry theRecordingstag now, so that lookup misses and the tag-derived name produces the same service — the entries were dead configuration describing an assignment that no longer happens. Removed from ruby, python, typescript, swift and kotlin; all five drift checks regenerate byte-identical, as docheck-operation-assignment-parity,check-service-inventory-parityandcheck-grouped-client-coverage. Rust'snames.tomlkeys off operationId rather than the tag, so its entries stay live and unchanged.Match.Resourceis public on the Go router and moves fromAutomationtoRecordingshere, buturl_test.go'swantResassertsResourceID()— the ID — and nothing asserted the classification.TestRouterMatchnow carries awantResourcecolumn, filled in for the two existing bucket cases and three recording routes. Reverting the five route labels fails exactly those three.Follow-up linkage
After this merges and a new SDK release is cut, mcp-server's #140 / M2 can consume the
Recordings(andEventFeed) tags sobasecamp_recordingsandbasecamp_eventskeep their own MCP domains instead of folding intobasecamp_admin/basecamp_schedules.Summary by cubic
Retags the six recording lifecycle operations from
AutomationtoRecordingsso MCP catalog generation gives them their ownbasecamp_recordingsdomain, and adds a fail-closed check that every operation inopenapi.jsoncarries exactly one tag.openapi.json, Gourl-routes.json, and TypeScriptopenapi-stripped.json, and Go's routerMatch.Resourcefor recording routes is nowRecordings(pinned by new tests).Recordingsservice; boosts stay underBoostsand the recording timesheet underSchedule.scripts/check-required-tags.rbrejects operations with zero, multiple, or blank tags; treats unknown path-item fields as operations so a new HTTP verb can't slip past; refuses$refpath items it can't resolve; scanswebhookstoo, reporting malformed values by name; and fails closed on a spec with no operations.make checkand the spec-gates CI job; a self-test covers the negative cases, including an untaggedHEADbeside a taggedGET.Written for commit 44e0bef. Summary will update on new commits.