Repository navigation
Split the Automation catch-all into six real domains - #930
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. |
|
@codex review |
github-actions
Bot
added
typescript
There was a problem hiding this comment.
🟢 Approved
The tag artifacts and generator overrides are consistent, preserve existing service placement, and include focused regression coverage.
Pull request overview
Splits the broad Automation tag into six coherent domains while preserving existing SDK service placement.
Changes:
- Retags 37 operations across Smithy and generated metadata.
- Updates all service generators, preserving
ListLineupMarkersinAutomationService. - Adds Go router coverage for every new resource label.
[!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 six domain boundaries. |
openapi.json |
Projects the updated operation tags. |
typescript/src/generated/openapi-stripped.json |
Synchronizes TypeScript OpenAPI metadata. |
typescript/scripts/generate-services.ts |
Preserves TypeScript service assignments. |
ruby/scripts/generate-services.rb |
Preserves Ruby service assignments. |
python/scripts/generate_services.py |
Preserves Python service assignments. |
swift/Sources/BasecampGenerator/ServiceGrouper.swift |
Preserves Swift service assignments. |
kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/Config.kt |
Preserves Kotlin service assignments. |
rust/generator/names.toml |
Preserves Rust’s lineup service override. |
go/pkg/basecamp/catalog/catalog.json |
Updates catalog domain tags. |
go/pkg/basecamp/url-routes.json |
Updates public router resource labels. |
go/pkg/basecamp/url_test.go |
Covers each new router label. |
Review details
- Files reviewed: 11/12 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.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
jeremy
left a comment
jeremy
left a comment
There was a problem hiding this comment.
Merging ListLineupMarkers into LineupService is where it obviously belongs, but that is a breaking change and a separate decision. Flagging it rather than taking it.
Get it right; make the breaking change.
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.
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.
…vice
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.
40f62c5 to
b9a9d06
Compare
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
Service-count documentation and a generated-method reference remain stale after the Automation service removal.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 30/50 changed files
- Comments generated: 3
- Review effort level: Balanced
![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: b9a9d06b6c
ℹ️ 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".
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.
|
@codex review |
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.
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
The migration guide still references the removed automation.list_lineup_markers accessor in its later operation roster.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 29/49 changed files
- Comments generated: 1
- Review effort level: Balanced
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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
11 version files and 8 lockfiles, written by scripts/bump-version.sh. MIGRATING.md's '# Unreleased' section is promoted to '# v0.20.0'. Thirteen commits since v0.19.0 (2026-09-16), the substantial ones being the #925 chain — #931 finding operations by exclusion so no HTTP verb can drop one in silence, #932 stopping a refusal from destroying generated output, and #938 deriving gen-catalog's emission bound from the shared declaration — plus #926's embedded tool Catalog, #930's Automation split, #924's event-feed body validation at the wrapper's decode, and #937's anyio bump for CVE-2026-63374. Ran the release's own preflight here rather than discovering it at tag time: promote-migrating --check passes, rs-publish-check packages and dry-run-uploads 0.20.0 clean, and all four lockfile freshness assertions hold.
Automationcarries 37 operations spanning templates, the template library, webhooks, dock tools, questionnaires, lineup markers and search. basecamp/mcp'scatalog.Loadbuilds one MCP domain tool per OpenAPI tag, so at cutover that becomes the widest and least coherent tool in the set — and a retag afterwards moves a tool people are already using. #929 named this as a real question and explicitly deferred it; this is the answer.The boundaries, and why each falls where it does
The six domains are not invented here. Every tag-keyed generator resolves an operation's service through
SERVICE_SPLITSunder its tag, and all 37 of these already had entries there routing them to six different services. The split was already made; only the tag was holding them together.TemplatesTemplatesWebhooksWebhooksDockToolsLineupLineup(3),Automation(1)SearchSearchCheckinsCheckinsTemplates stays one domain, not two. A template is a stored project shape; a project construction is the async job that builds a project from one. The template library is the same act from a different source — the account's shared catalogue instead of a template you own — and a library copy is its construction.
/template_library/looks like a separate prefix, but splitting on it would put the two halves of "make a new project from something stored" under different tools.CreateProjectFromTemplateandCreateTemplateLibraryCopyare the same verb.Dock, notTools. These operations are about the project's tool strip: which tools a project has, what they are called, what order they sit in, whether they are on. Naming the tagToolswould hand a tag-per-tool consumer a tool called "tools", and would name the operations after the message board or to-do set behind each dock entry rather than after the strip they actually manipulate. The tag differs from theToolsservice, which is the #929 shape: the split entry moves from theAutomationkey to aDockkey and stays load-bearing.Checkinsis the one collision, and it is the point. #922 created that tag for six question operations that had none, and its own note said consolidating the rest of the check-in family under it was "a follow-up editorial call left to the maintainers." This is that follow-up. The questionnaire/question/answer CRUD already generated into the sameCheckinsServiceas those six — only the tag disagreed, so one domain was being offered to tag-keyed consumers as two. After this the tag and the service are 1:1 at 15 operations.The downstream consumer reviewed these boundaries and endorsed all three contested calls. The basecamp-mcp-server catalog work — which turns each tag into one
basecamp_<domain>tool — reports thatbasecamp_toolswould collide with "tool" in the MCP sense on top of the mislabelling, soDockis the better handle; thatdockis Basecamp jargon an outside caller will not know cold, but an MCP tool carries a description beside its name, and that description will carry the weight ("the project's dock — the strip of tools like Message Board, To-dos, Schedule: enable/disable, rename, reorder"). It readsTemplatesas one domain for the same reason given here — splitting on/template_library/would fracture one caller intent across two tools — andCheckinsconsolidation as strictly better, since oneCheckinsServicesurfacing as two tags scatters the questionnaire/question/answer family.Searchstays separate fromEverything.Everythingis Basecamp's pre-defined cross-project screens (/todos/overdue.json,/cards/open.json);Searchis a free-text query plus the metadata describing what can be filtered on. Folding one into the other would force a service rename for no gain.Nothing stays
Automation. Webhooks are the closest — they are what automation is built on — but a webhook is a subscription resource, andbasecamp_webhookssays more thanbasecamp_automationever did. The tag ends at zero operations.openapi.jsonhas no top-leveltagsregistry, so there is nothing left declared and nothing empty: the tag simply ceases to exist. Tag count goes 28 → 32.The one break:
ListLineupMarkersmoves service as well as tagAutomationServiceis gone. It held exactly one method, and it held it by accident:ListLineupMarkerswas the only operation theAutomationtag reached without aSERVICE_SPLITSentry, so it fell through to the tag-derived service while its three siblings were split intoLineup. Nothing decided that; the table just did not name it.With the tag gone it lands on
Lineupwith the other three, and the service goes with it. That is a public break in six SDKs, taken deliberately and written down inMIGRATING.md:account.automation.list_lineup_markersaccount.lineup.list_lineup_markersaccount.automation.list_lineup_markers()account.lineup.list_lineup_markers()client.automation.listLineupMarkers()client.lineup.listLineupMarkers()account.automation.listLineupMarkers()account.lineup.listLineupMarkers()account.automation().list_lineup_markers()account.lineup().list_lineup_markers()account.Lineup().ListMarkers(ctx)The method name does not change — only the service it hangs off — and every one of these is a compile error or a
NoMethodError/AttributeErroron first call. There is no version of this that silently returns the wrong thing.Go is the reason to be confident this is right. Go's hand-written surface never had an
Automation()accessor; it has always exposed this operation asLineupService.ListMarkers. That divergence was recorded as a:foldedcarve-out incheck-service-inventory-parity, which synthesizedautomationinto Go's inventory so the sets would still match. Bringing the other six into line closes the fold for real — so the carve-out is deleted, along with its rows in SPEC §5.1 and Appendix F. The gate found this itself: it failed with "automationis emitted by go-accessors but NOT by [the other nine]" the moment the service stopped being generated, which is exactly the staleness check it was built for.Its own self-test used
automationas the example of a stale fold carve-out (cases 7 and 15). Those now exerciseclient_visibility, the surviving one — the mechanism under test is unchanged, only the example moved.Dead entries vs. load-bearing ones
Both halves of the #922/#928/#929 distinction show up here, so each one was checked rather than reasoned about.
Deleted (restating — the entry names its own tag):
Templates,Webhooks,Search,Lineup(the three markers) andCheckinssplit entries. Re-adding all five and regenerating Ruby produces a byte-identical tree, which is what "dead" means.Kept (load-bearing — the service differs from the tag): the
Toolsentry alone, moved from theAutomationkey toDock. Deleting it deletestools_service.rb(83 lines) and emits aDockServicein its place. It is the only entry in the split that survives.Rust.
Naming::service_forconsults[operation_services]by operationId before the tag, so a row is dead exactly when its service equals the tag-derived name. 29 rows become restating and are removed — the 9 Checkins, 3 Lineup, 2 Search, 10 Templates and 5 Webhooks rows. The 7Toolsrows stay: their service differs fromDockthe same wayListEvents = "Events"differs fromRecordings. No row is added:ListLineupMarkersresolves through the tag like its siblings.TAG_TO_SERVICE's'Automation' => 'Automation'row (and Rust's[services]equivalent) is removed in all six generators. It was an identity mapping for a tag that no longer exists, and with the service gone too there is nothing left anywhere namedAutomation.Deliberately not touched: the
'Untagged' => { 'Checkins' => [...] }entries and their six Rust rows. Those six operations have carried theCheckinstag since #922, so the entries are already dead — but the wholeUntaggedkey is dead for the same reason (check-required-tagsguarantees every operation has exactly one tag), and cleaning out one block of it while leavingTimeline,Reports,Todos,PeopleandCardColumnswould make the six hand-transcribed tables harder to diff by eye, which is the propertynames.toml's header depends on. That is a separate sweep.What changed, and what did not
36 of the 37 operations change tag and nothing else. For those,
make generatecomes back byte-identical across ruby, python, typescript, swift, kotlin, rust and go — no generated service file moves, no method is renamed, no signature changes. That is the design: tags and services are different axes, and #929 proved the move is free when they are kept so.The 37th,
ListLineupMarkers, changes both, and that accounts for every generated-code diff in this PR:AutomationServicedeleted in six SDKs, the method appended toLineupService, and the accessor barrels and hand-written client wiring updated to match. Nothing else moved with it.The tag-carrying artifacts show the split itself, and nothing but it:
openapi.json— 37 lines, one per operation, all tag stringstypescript/src/generated/openapi-stripped.json— the same 37go/pkg/basecamp/catalog/catalog.json— 37"tag"fields, no other keygo/pkg/basecamp/url-routes.json— 21"resource"fields (one per route pattern), no other keybehavior-model.jsonis unchanged; tags live only in the OpenAPI projection.One prose fix rides along: SPEC §5.1 opened by naming
Automationas an example of a coarse tag, which this PR makes false.Card Tables— five services out of one tag — takes its place.Router labels are pinned
Match.Resourceis public and comes straight from the route table's resource field, which is the tag — so all six labels moved and nothing asserted any of them.url_test.gonow pins one route per new label, matching what #922 and #929 did for the recording routes.Mutation-checked one label at a time by reverting it to
Automationinurl-routes.json:Dock(3 routes) fails 2 assertions,Templates(7) fails 2, andWebhooks(2),Lineup(2),Search(2) andCheckins(10) each fail 1. Every new label is held by at least one case.Gates
make checkclean: the seven per-language drift checks,check-operation-assignment-parity,check-service-inventory-parity(and its self-test),check-grouped-client-coverage,check-required-tags,url-routes-check,catalog-check,smithy-checkanddoc-constants-check.Rebased onto #931, which replaced each generator's hard-coded five-verb list with a
NON_OPERATION_FIELDSdeny-list. It rebased without conflict — #931 changed how operations are found, this changes how they are grouped, and the two do not touch the same lines — and the regeneration after the rebase produced nothing beyond what is described above. The service count in SPEC §5 and the Kotlin and Swift READMEs moves 55 → 54 on its own throughdoc-constants.