Fix --version over-propagation in check-cli-surface.sh#499
Merged
robzolkos merged 1 commit intoJun 29, 2026
Conversation
The script merged ALL root flags (including local-only ones like --version) into every subcommand's flag list. Cobra adds --version as a local flag via InitDefaultVersionFlag, not a persistent one, so it does not inherit to subcommands. Fix: derive ROOT_FLAGS from a child command's inherited_flags instead of the root's .flags. This captures only the persistent subset that Cobra actually propagates to children. Closes basecamp#368
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the CLI surface-check script to detect only persistent (inherited) flags, avoiding root-only flags that don’t propagate to subcommands.
Changes:
- Switches ROOT_FLAGS extraction from
.flagson the root command to.inherited_flagson a child subcommand. - Adds logic to pick the first subcommand name from the root
--help --agentJSON output.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Collaborator
|
@sawirricardo thanks! |
This was referenced Jul 18, 2026
jeremy
added a commit
that referenced
this pull request
Jul 18, 2026
* Fix todos update silently clearing completion subscribers (#538) The BC3 todos PUT has replace semantics: any field omitted from the body is cleared. Neither branch of todos update included completion_subscriber_ids, so every update wiped the todo's "When done, notify" list. The #412/#413 read-modify-write merge couldn't preserve it because the SDK's Todo model drops completion_subscribers in todoFromGenerated (basecamp/basecamp-sdk#355) — the merge had nothing to carry forward. Preservation now works via a raw GET of the flat /todos/{id}.json route (which serves the field the SDK model drops), feeding the current subscriber ids into both the typed-merge and raw-clear PUT paths. The read fails closed: an HTTP error, malformed JSON, or a response missing the completion_subscribers key aborts the command before any PUT rather than risking a silent clear. The helper is commented as temporary and goes away once the SDK round-trips the field. Alongside the fix, subscribers become directly editable: - todos create/update --notify-on-completion <names or ids> sets them (comma-separated, with people-name tab completion) - todos update --no-notify-on-completion clears them (by omission — explicit intent skips the preservation read entirely) Unit tests cover preservation in both branches, the flat-route assertion, explicit set/clear bypassing the read, the fail-closed matrix (missing key / malformed JSON / HTTP 500 in each branch), flag conflicts, and the create body. A live smoke test exercises the full sequence: create with subscriber, title-only update preserves, --no-due preserves, --no-notify-on-completion clears. .surface gains the three new flag records by hand (verified identical to fresh generation); a full regeneration is deferred because the snapshot has unrelated drift since #499 changed the generator script. * Use completion-subscriber wording in --notify-on-completion errors --notify-on-completion resolved people through resolveAssigneeIDs, so failures surfaced as assignee errors ("No valid assignees provided", "Assignee ID must be a positive number") — misleading for a flag that sets completion subscribers. Parameterize the resolver with a role label: resolvePersonRoleID(s) carry the wording, and resolveAssigneeID(s) stay as assignee-labeled wrappers so existing call sites and messages are unchanged. The subscriber call sites now go through resolveCompletionSubscriberIDs. Tests cover both subscriber-worded messages and assert no PUT occurs on resolution failure. * Test the name-resolution failure path for --notify-on-completion The subscriber-wording tests covered the numeric-validation and empty-list errors but not the ResolvePerson miss, so the "failed to resolve completion subscriber" formatting was only exercised indirectly. The update mock now matches the flat preservation route exactly (/todos/999.json) instead of any .json GET, and serves /people routes an empty directory so name resolution deterministically misses. New subtest asserts the subscriber-worded resolve error and that no PUT occurs. The explicit set/clear tests tighten their no-preservation-read assertions to the exact route. * Address review: SDK error conversion + exact mock route match The preservation read's GET error now runs through convertSDKError before wrapping, so structured codes/hints (rate limit, circuit breaker) survive with the subscriber-specific context — matching every other raw Account() call site. The update mock's preservation branch now matches the account-scoped flat route exactly (/99999/todos/999.json) instead of by suffix, so a bucket-scoped GET can never satisfy it. * Reject missing or invalid subscriber ids in the preservation read A completion_subscribers element without a positive id would have put a zero into completion_subscriber_ids and reached the PUT, slipping past the fail-closed contract. The preservation read now errors on any non-positive id, and the fail-closed test matrix covers the case.
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.
Summary
scripts/check-cli-surface.shmerged ALL root.flags(22 entries) into every subcommand, including local-only flags like--versionthat Cobra does not propagate to children. This created a ~612-entry divergence vs the Go surface walker (github.com/basecamp/cli/surface).Fix
Derive
$ROOT_FLAGSfrom a child command'sinherited_flagsinstead of the root's.flags. This captures only the 5 persistent flags Cobra actually propagates (account,json,md,project,quiet), excluding 17 local-only flags (version,agent,verbose,cache-dir, etc.).Before → After
--versionin subcommand surfaceTesting
Closes #368
Summary by cubic
Fixes incorrect flag propagation in
scripts/check-cli-surface.shby only inheriting persistent flags for subcommands. This removes--versionand other local-only flags from subcommand surfaces and restores parity with the Go walkergithub.com/basecamp/cli/surface.ROOT_FLAGSfrom a subcommand’sinherited_flagsinstead of the root’s.flags.--versionis root-only; surface size drops from ~10,900 to ~7,750 lines.Written for commit 9e45417. Summary will update on new commits.