Conversation
github-actions
bot
added
commands
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
This PR prepares the Basecamp CLI for the upcoming SDK sync by updating existing command integrations to match new SDK signatures and by adding several new service-focused command groups (accounts, people settings, gauges, notifications, reports “mine” views), along with accompanying unit and smoke coverage.
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:
- Updated card move wiring to the SDK’s new
Move(..., opts)signature and position option handling. - Added new CLI surfaces:
accountsmanagement,peopleprofile/preferences/out-of-office,gauges,notifications, and personal assignment reports underreports. - Added/updated unit tests and introduced an SDK-sync-focused smoke suite for the new surfaces.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/workspace/data/mutations_card.go | Updates TUI card move mutation to new SDK move signature. |
| internal/commands/schedule.go | Minor adjustment to schedule update flow around attachment embedding. |
| internal/commands/reports.go | Adds reports mine/completed/due commands + scope validation. |
| internal/commands/reports_test.go | Adds tests for the new “my assignments” report commands. |
| internal/commands/people.go | Adds people profile, preferences, and out-of-office subcommands. |
| internal/commands/people_test.go | Extends people mock server + adds tests for new people subcommands. |
| internal/commands/notifications.go | Introduces notifications list and notifications read. |
| internal/commands/notifications_test.go | Adds tests for notifications list/read request behavior. |
| internal/commands/gauges.go | Introduces gauges command group (list, needles, CRUD, enable/disable). |
| internal/commands/gauges_test.go | Adds tests validating gauges request routing and payloads. |
| internal/commands/forwards.go | Adds forward listing sort controls (--sort, --reverse). |
| internal/commands/forwards_test.go | Adds tests for forwards sorting validation and query formation. |
| internal/commands/commands.go | Registers new commands in command-category metadata. |
| internal/commands/commands_test.go | Ensures root command includes new command groups under test harness. |
| internal/commands/cards.go | Updates card move implementation for SDK signature + position options. |
| internal/commands/cards_test.go | Updates move-position tests to match new endpoint/payload contract. |
| internal/commands/accounts.go | Adds accounts show/rename/logo subcommands + logo validation. |
| internal/commands/accounts_test.go | Adds tests for new accounts subcommands (including multipart upload). |
| internal/cli/root.go | Registers new top-level commands in CLI root. |
| e2e/smoke/smoke_sdk_sync.bats | Adds smoke coverage for new SDK-sync surfaces (with scoped skips). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if app == nil { | ||
| return fmt.Errorf("app not initialized") | ||
| } | ||
| if err := app.RequireAccount(); err != nil { |
There was a problem hiding this comment.
In accounts show/rename/logo subcommands you call app.RequireAccount() directly, which skips the interactive account resolution behavior provided by ensureAccount (and doesn’t sync app.Names with the resolved account). This makes these commands fail when no account is configured, while most other commands will prompt/resolve. Consider switching these to ensureAccount(cmd, app) for consistent CLI behavior.
| if err := app.RequireAccount(); err != nil { | |
| if err := ensureAccount(cmd, app); err != nil { |
| if positionSet && position > 0 && cardTableIDVal == "" { | ||
| cardTableIDVal, err = getCardTableID(cmd, app, resolvedProjectID, *cardTable) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
cards move still resolves a card table ID when --position is set and the destination column is already numeric (via --to <column-id>). With the new SDK move endpoint, the card table ID is no longer required for positioned moves, so this extra lookup can introduce a regression on projects with multiple card tables (ambiguous prompt/error) for an otherwise-valid command. Consider only resolving the card table when it’s actually needed (e.g., when --to is a column name) and keep the ID lookup strictly for breadcrumbs/UI.
| if reverse { | ||
| opts.Direction = "desc" | ||
| } else { | ||
| opts.Direction = "asc" | ||
| } | ||
| } |
There was a problem hiding this comment.
--reverse currently only affects the request when --sort is also provided (because opts.Direction is set inside the if sortField != "" block). If a user passes --reverse alone, it silently has no effect. Consider either rejecting --reverse without --sort, or applying Direction to the default sort order as well.
| if reverse { | |
| opts.Direction = "desc" | |
| } else { | |
| opts.Direction = "asc" | |
| } | |
| } | |
| if !reverse { | |
| // Preserve existing behavior: explicit sort without --reverse is ascending. | |
| opts.Direction = "asc" | |
| } | |
| } | |
| if reverse { | |
| // Apply reverse to whichever sort order (explicit or default) is in effect. | |
| opts.Direction = "desc" | |
| } |
| if !enabled { | ||
| summaryVerb = "Disabled" | ||
| } | ||
|
|
||
| return &cobra.Command{ | ||
| Use: use, | ||
| Short: strings.ToLower(summaryVerb) + " a project gauge", | ||
| Long: summaryVerb + " the gauge for a project.", |
There was a problem hiding this comment.
The enable/disable toggle command help text is grammatically off: Short becomes "enabled a project gauge" / "disabled a project gauge" and Long is past tense. Cobra command descriptions are typically imperative ("Enable a project gauge" / "Disable a project gauge"). Consider adjusting the verb forms so the help output reads correctly.
| if !enabled { | |
| summaryVerb = "Disabled" | |
| } | |
| return &cobra.Command{ | |
| Use: use, | |
| Short: strings.ToLower(summaryVerb) + " a project gauge", | |
| Long: summaryVerb + " the gauge for a project.", | |
| descriptionVerb := "Enable" | |
| if !enabled { | |
| summaryVerb = "Disabled" | |
| descriptionVerb = "Disable" | |
| } | |
| return &cobra.Command{ | |
| Use: use, | |
| Short: descriptionVerb + " a project gauge", | |
| Long: descriptionVerb + " the gauge for a project.", |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 21 files
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="internal/commands/gauges.go">
<violation number="1" location="internal/commands/gauges.go:257">
P3: Use a flag-specific usage error for missing `--description` instead of `missingArg`, which is intended for argument-missing paths.
(Based on your team's feedback about using ErrUsage for missing required flags.) [FEEDBACK_USED]</violation>
</file>
<file name="internal/commands/forwards.go">
<violation number="1" location="internal/commands/forwards.go:145">
P2: `--reverse` direction for forwards sorting is inverted versus existing CLI sort behavior for created/updated fields.</violation>
</file>
<file name="internal/commands/people.go">
<violation number="1" location="internal/commands/people.go:691">
P2: Treat whitespace-only `--from`/`--to` values as missing so required-flag validation works correctly.</violation>
</file>
<file name="internal/commands/accounts.go">
<violation number="1" location="internal/commands/accounts.go:363">
P2: AVIF/HEIC are listed as supported logo formats, but current MIME detection won’t reliably classify them, so valid files can be rejected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } else { | ||
| opts.Sort = "updated_at" | ||
| } | ||
| if reverse { |
There was a problem hiding this comment.
P2: --reverse direction for forwards sorting is inverted versus existing CLI sort behavior for created/updated fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/commands/forwards.go, line 145:
<comment>`--reverse` direction for forwards sorting is inverted versus existing CLI sort behavior for created/updated fields.</comment>
<file context>
@@ -127,6 +136,18 @@ func runForwardsList(cmd *cobra.Command, project, inboxID string, limit, page in
+ } else {
+ opts.Sort = "updated_at"
+ }
+ if reverse {
+ opts.Direction = "desc"
+ } else {
</file context>
| Long: "Enable out-of-office status for a person. Defaults to the current user.", | ||
| Args: cobra.MaximumNArgs(1), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if startDate == "" || endDate == "" { |
There was a problem hiding this comment.
P2: Treat whitespace-only --from/--to values as missing so required-flag validation works correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/commands/people.go, line 691:
<comment>Treat whitespace-only `--from`/`--to` values as missing so required-flag validation works correctly.</comment>
<file context>
@@ -364,6 +368,407 @@ func runPeopleShow(cmd *cobra.Command, args []string) error {
+ Long: "Enable out-of-office status for a person. Defaults to the current user.",
+ Args: cobra.MaximumNArgs(1),
+ RunE: func(cmd *cobra.Command, args []string) error {
+ if startDate == "" || endDate == "" {
+ return output.ErrUsage("--from and --to are required")
+ }
</file context>
| if startDate == "" || endDate == "" { | |
| if strings.TrimSpace(startDate) == "" || strings.TrimSpace(endDate) == "" { |
| } | ||
| contentType := richtext.DetectMIME(path) | ||
| switch contentType { | ||
| case "image/png", "image/jpeg", "image/gif", "image/webp", "image/avif", "image/heic": |
There was a problem hiding this comment.
P2: AVIF/HEIC are listed as supported logo formats, but current MIME detection won’t reliably classify them, so valid files can be rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/commands/accounts.go, line 363:
<comment>AVIF/HEIC are listed as supported logo formats, but current MIME detection won’t reliably classify them, so valid files can be rejected.</comment>
<file context>
@@ -161,3 +205,164 @@ func newAccountsUseCmd() *cobra.Command {
+ }
+ contentType := richtext.DetectMIME(path)
+ switch contentType {
+ case "image/png", "image/jpeg", "image/gif", "image/webp", "image/avif", "image/heic":
+ return nil
+ default:
</file context>
| Args: cobra.ExactArgs(1), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if description == "" { | ||
| return missingArg(cmd, "--description") |
There was a problem hiding this comment.
P3: Use a flag-specific usage error for missing --description instead of missingArg, which is intended for argument-missing paths.
(Based on your team's feedback about using ErrUsage for missing required flags.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/commands/gauges.go, line 257:
<comment>Use a flag-specific usage error for missing `--description` instead of `missingArg`, which is intended for argument-missing paths.
(Based on your team's feedback about using ErrUsage for missing required flags.) </comment>
<file context>
@@ -0,0 +1,463 @@
+ Args: cobra.ExactArgs(1),
+ RunE: func(cmd *cobra.Command, args []string) error {
+ if description == "" {
+ return missingArg(cmd, "--description")
+ }
+
</file context>
|
Closing due to newer #373 |
Summary
This draft prepares the CLI for the upcoming SDK sync branch and keeps the work split into service-focused commits so reviewers can walk the changes commit by commit.
This branch was developed and validated locally against the SDK sync branch at
97fde17via a temporarygo.workoverride. The final SDK bump is intentionally not included here yet.Dependency Status
This PR is draft-only until the SDK work lands.
Blocking items before this can merge:
make bump-sdkfollow-upinternal/version/sdk-provenance.jsonandAPI-COVERAGE.mdshould be updated once the SDK ref is finalBecause the SDK bump is intentionally deferred, reviewers should treat this as a prepared CLI implementation branch rather than a merge-ready PR.
What Changed
Compatibility updates for existing CLI behavior
Cards().Move(..., opts)signature--sort created|updatedand--reverseAccount management
Added account-scoped management commands under
accounts:basecamp accounts showbasecamp accounts rename <name>basecamp accounts logo set <path>basecamp accounts logo removeI kept these under
accountsrather than introducing a new top-levelaccountgroup becauseaccountis already an alias foraccounts.People profile, preferences, and out-of-office
Extended
peoplewith current-user settings and out-of-office operations:basecamp people profile showbasecamp people profile update ...basecamp people preferences showbasecamp people preferences update ...basecamp people out-of-office show [person]basecamp people out-of-office enable --from <date> --to <date> [person]basecamp people out-of-office disable [person]Gauges
Added a new top-level
gaugescommand group covering both account-wide listing and project-scoped needle management:basecamp gauges listbasecamp gauges needles --in <project>basecamp gauges show <needle-id|url>basecamp gauges create --position <0-100> --in <project> ...basecamp gauges update <needle-id|url> --description <text>basecamp gauges delete <needle-id|url>basecamp gauges enable --in <project>basecamp gauges disable --in <project>Notifications
Added a new top-level
notificationscommand group:basecamp notifications list [--page]basecamp notifications read <readable-sgid>...One live-data issue showed up while testing this surface: notification creators can be local people with string IDs like
"bulletins"or"onboarding". That turned out to be an SDK bug rather than a CLI bug, which is why the stacked SDK PR above exists.My assignments
Extended
reportswith current-user assignment views:basecamp reports minebasecamp reports completedbasecamp reports due [--scope overdue|due_today|due_tomorrow|due_later_this_week|due_next_week|due_later]I kept these under
reportsrather than adding another top-level command so they stay discoverable alongside the existing account-wide report commands.Review Guide
This branch is intentionally organized as service-focused commits:
176d049Adapt card moves to SDK sync branch3389677Add account management commands9965106Add people profile and preferences commands16eef9dAdd gauges command groupbe5f8aeAdd notifications command groupdbd9c60Add my assignments reports commands489fa2dAdd forwards sorting options7497f40Fix lint regressions after main rebase10c9177Refresh surface baseline and smoke coverageThe intent is that a reviewer can read this top-down and see the CLI surface expand one service at a time.
Validation
Local validation on this branch was done with the SDK sync worktree wired in through
GOWORK=/tmp/basecamp-cli-sdk-sync.go.work:bin/cimake check-surfaceI also ran live read-only checks against account
5978679for the new surfaces:Intentionally Deferred
These are intentionally not in this draft yet because they should reflect the final merged SDK ref rather than the local prep setup:
make bump-sdkinternal/version/sdk-provenance.jsonAPI-COVERAGE.mdSummary by cubic
Prepares the CLI for the upcoming
basecamp-sdksync by adding new surfaces (accounts, people, gauges, notifications, my assignments) and adapting card and todo moves to the new SDK APIs. The SDK bump is intentionally deferred; this is the CLI implementation only.New Features
Cards().Move(..., opts)with position support and overflow guard; update TUI move call.Todos().Reposition(..., opts).Dependencies
basecamp-sdk.make bump-sdk, then updateinternal/version/sdk-provenance.jsonandAPI-COVERAGE.md.Written for commit 5810125. Summary will update on new commits.