Conversation
github-actions
bot
added
commands
![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: 9c06abb4d1
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
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.
This PR upgrades the Basecamp Go SDK to v0.7.0 and expands the CLI surface to cover newly supported API areas (gauges, my assignments, my notifications, and additional account management actions).
Changes:
- Bump
github.com/basecamp/basecamp-sdk/gotov0.7.0and update affected call sites (Cards.Move,Todos.Reposition) for the new optional-params signatures. - Add new CLI command groups:
gauges,assignments, andnotifications, plus extendaccountswithshow,update, andlogosubcommands. - Update CLI catalog/surface snapshots, API coverage docs, and add smoke tests for the new commands.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/check-bare-groups.sh | Allowlist new “shortcut-style” command parents that intentionally run on bare invocation. |
| internal/version/sdk-provenance.json | Update recorded SDK/API revisions for provenance tracking. |
| internal/tui/workspace/data/mutations_card.go | Adjust TUI card move mutation to new SDK Move signature. |
| internal/commands/todos.go | Adjust Todos.Reposition call to new SDK signature. |
| internal/commands/notifications.go | New notifications command (list + mark-as-read). |
| internal/commands/gauges.go | New gauges command group (list/needles CRUD + enable/disable). |
| internal/commands/commands_test.go | Register new commands in the test root to keep catalog/surface tests aligned. |
| internal/commands/commands.go | Add new commands to the command catalog categories/actions list. |
| internal/commands/cards.go | Adjust Cards.Move calls to new SDK signature. |
| internal/commands/assignments.go | New assignments command group (list/completed/due). |
| internal/commands/accounts.go | Extend accounts with show/update/logo upload/remove operations. |
| internal/cli/root.go | Register new commands in the real CLI root. |
| go.mod | Bump basecamp-sdk and indirect runtime dependency versions. |
| go.sum | Update checksums for bumped dependencies. |
| e2e/smoke/smoke_notifications.bats | Add smoke coverage skeleton for notifications commands. |
| e2e/smoke/smoke_gauges.bats | Add smoke coverage for gauges commands (with environment-dependent skips). |
| e2e/smoke/smoke_assignments.bats | Add smoke coverage for assignments commands. |
| e2e/smoke/smoke_account.bats | Add smoke coverage skeleton for new accounts subcommands. |
| API-COVERAGE.md | Update documented endpoint coverage totals and list newly covered sections. |
| .surface | Update CLI surface snapshot for new commands/subcommands/flags/args. |
💡 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}
There was a problem hiding this comment.
4 issues found across 20 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/accounts.go">
<violation number="1" location="internal/commands/accounts.go:283">
P2: `accounts logo upload` advertises a 5MB limit, but the current validation allows up to 100MB.</violation>
</file>
<file name="internal/commands/gauges.go">
<violation number="1" location="internal/commands/gauges.go:226">
P2: Remove `MarkFlagRequired` and validate the required flag manually in `RunE` using `output.ErrUsage("--position is required")`.
(Based on your team's feedback about using output.ErrUsage to report missing required flag values.) [FEEDBACK_USED]</violation>
<violation number="2" location="internal/commands/gauges.go:229">
P2: The `--notify custom` mode is exposed, but there's no way to provide the required subscriptions (people IDs). Consider adding a `--subscribe` flag or removing `custom` from the allowed values.</violation>
</file>
<file name="e2e/smoke/smoke_account.bats">
<violation number="1" location="e2e/smoke/smoke_account.bats:75">
P0: `mark_unverifiable` must be called before executing the command to prevent actually deleting the account logo in the smoke environment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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/notifications.go">
<violation number="1" location="internal/commands/notifications.go:106">
P3: The `read` command help text says the default is page 1, but the `--page` flag default is 0. Update the text to match the actual default (or change the default).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac1a1b2b3d
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
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:195">
P2: Validate `--position` before account/project resolution so missing-flag usage errors are returned immediately instead of after auth/project lookup side effects.</violation>
</file>
<file name="e2e/smoke/smoke_account.bats">
<violation number="1" location="e2e/smoke/smoke_account.bats:79">
P2: `run_smoke` is unreachable here because `mark_unverifiable` immediately skips the test. This leaves dead code and makes the test body misleading.</violation>
</file>
<file name="skills/basecamp/SKILL.md">
<violation number="1" location="skills/basecamp/SKILL.md:84">
P2: `notifications read` is documented as unblocked, but it first calls `MyNotifications().Get` to resolve IDs, so it is still blocked by the notification deserialization issue.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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="e2e/smoke/smoke_notifications.bats">
<violation number="1" location="e2e/smoke/smoke_notifications.bats:19">
P2: Use structured JSON assertions instead of substring matching for `--json` output.
(Based on your team's feedback about structured JSON assertions in smoke tests.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83bae399c4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70493aa5bf
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
internal/commands/schedule.go:688
- The new attachments-only handling runs whenever
description == ""and--attachis used. If the user updates other fields (e.g.--summary) and also attaches files without providing--description, this will overwrite the existing description with a description that contains only embedded attachments. Consider restoring the previous!hasChanges && len(attachFiles) > 0guard (or otherwise ensuring you don't clobber Description unless the user explicitly intends to change it).
// Handle attachments when no description text provided
if description == "" && len(attachFiles) > 0 {
refs, attachErr := uploadAttachments(cmd, app, attachFiles)
if attachErr != nil {
return attachErr
}
req.Description = richtext.EmbedAttachments("", refs)
hasChanges = true
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88ad9e0ebf
ℹ️ 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".
jeremy
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/commands/schedule.go:688
- When updating a schedule entry with other changes (e.g., summary/starts/ends/etc.) and
--attachbut no--description, this block will overwrite the entry description with only the embedded attachments (clearing any existing description). Previously this ran only for attachments-only updates. Consider preserving the existing description (fetch current entry + embed), or require--descriptionwhen using--attachfor updates that should not wipe the description.
// Handle attachments when no description text provided
if description == "" && len(attachFiles) > 0 {
refs, attachErr := uploadAttachments(cmd, app, attachFiles)
if attachErr != nil {
return attachErr
}
req.Description = richtext.EmbedAttachments("", refs)
hasChanges = true
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
basecamp-cli/internal/commands/schedule.go
Line 686 in 6ea302a
Preserve existing description when attaching on update
Setting req.Description to richtext.EmbedAttachments("", refs) in the no-description attach path will replace any existing schedule-entry description with attachment-only HTML. This now affects updates that change other fields (for example --summary + --attach) because the branch runs whenever description == "", so users can unintentionally lose prior description text while making unrelated edits. Consider fetching/merging the current description or only mutating Description when the user explicitly updates it.
ℹ️ 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".
|
Fixed both:
|
Fixes Person.Id deserialization (normalizeJSON pre-unmarshal for notification responses) and Account.Logo type (string → AccountLogo object). Adapts two breaking changes: Cards().Move() and Todos().Reposition() now take optional params. Adds AVIF/HEIC to richtext MIME extension map for logo upload support.
Subcommands: list (account-wide), needles/needle (project-scoped), create/update/delete, enable/disable. Create validates --position (0-100), --color, --notify (requires --subscriptions for custom mode).
Subcommands: list, completed, due (with scope filter). Shortcut pattern — bare invocation shows priorities + non-priorities.
Notifications: list (paginated with dynamic next-page breadcrumb) and read (resolves IDs to SGIDs from --page). Accounts: show, update, logo upload (5MB + image type validation) and remove. Registers all new commands in root.go, catalog, and test harness.
API-COVERAGE.md: 42 sections, 155 endpoints, SDK v0.7.1. SKILL.md: gauges, assignments, notifications, accounts sections. .surface: 148 commands, 422 flags.
Summary
Bumps basecamp-sdk to v0.7.1 and adds CLI commands for all four new services.
gauges: project progress tracking with needles (0-100 scale, green/yellow/red). Subcommands: list, needles, needle, create, update, delete, enable, disableassignments: view current assignments with priority grouping. Subcommands: list, completed, due (with scope filter)notifications: view and mark-as-read notifications.readresolves notification IDs to SGIDs transparently, with--pageto match the page listedaccountsextensions: show (details/limits/subscription), update (rename), logo upload (5MB/image-type validation) and removeTest plan
bin/cipasses (all checks green except pre-existing smoke coverage gaps)basecamp gauges listreturns gaugesbasecamp gauges needles --in <project>returns needlesbasecamp assignmentsshows priorities and non-prioritiesbasecamp assignments due overduefilters correctlybasecamp notificationslists unread/read/memoriesbasecamp notifications read 999999returns proper "not found" errorbasecamp accounts showreturns account details with logo objectbasecamp accounts logo uploadvalidates 5MB size + image MIME type