Add hill chart commands (show, track, untrack)#347
Conversation
github-actions
bot
added
commands
There was a problem hiding this comment.
Pull request overview
This PR adds hill chart management capabilities to the Basecamp CLI by introducing the hillcharts command group with show, track, and untrack subcommands. The implementation includes smart error handling that detects when hill charts are disabled and provides actionable hints to users. The SDK dependency is bumped to v0.6.1-0.20260318201020-3ed854104191 to pick up the new HillChartsService functionality.
Changes:
- Added
internal/commands/hillcharts.gowith three subcommands for managing hill charts - Implemented error detection for 403 Forbidden responses with contextual hints based on todolist status
- Updated SDK dependency and CLI integration points in root.go and commands_test.go
- Added comprehensive E2E and smoke tests
- Updated API coverage documentation and surface file
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/version/sdk-provenance.json | Updated SDK version to v0.6.1-0.20260318201020-3ed854104191 |
| internal/commands/hillcharts.go | New file implementing hillcharts command group with show, track, and untrack subcommands |
| internal/commands/commands.go | Added hillcharts to command metadata list |
| internal/commands/commands_test.go | Registered NewHillchartsCmd() in test command builder |
| internal/cli/root.go | Registered NewHillchartsCmd() in main CLI root |
| go.mod, go.sum | Updated basecamp-sdk/go dependency |
| e2e/smoke/smoke_hillcharts.bats | Added smoke tests for basic hillcharts operations |
| e2e/hillcharts.bats | Added E2E tests for error handling and help |
| API-COVERAGE.md | Updated API coverage metrics (38 sections, 138 endpoints) with Hill Charts section |
| .surface | Generated surface file entries for hillcharts commands and flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 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="e2e/smoke/smoke_hillcharts.bats">
<violation number="1" location="e2e/smoke/smoke_hillcharts.bats:20">
P2: These tests always skip unconditionally, so they verify nothing — they can't fail even if the command is broken. If you can't assert a specific outcome, skip *before* `run_smoke` so you're not hiding a false sense of coverage. Alternatively, assert that the command at least exits (success or a known API error) before skipping:
```bash
run_smoke basecamp hillcharts track 999999 -p "$QA_PROJECT" --json
# Accept success or a not-found API error; fail on client-side crash
assert_success || assert_json_value '.error.type' 'not_found'
mark_unverifiable "Requires valid todolist ID"
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
robzolkos
force-pushed
the
add-hill-chart-commands
branch
from
5e8c21a to
61854a0
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for managing Basecamp hill charts from the CLI, enabling users to view and track todolists on their project's hill chart. The changes include a new basecamp hillcharts command group with three subcommands (show, track, untrack) that interact with the newly available HillChartsService in the updated SDK.
Changes:
- Adds
basecamp hillchartscommand group withshow,track, anduntracksubcommands for managing project hill charts - Upgrades the basecamp-sdk/go dependency to v0.6.1-0.20260318201020-3ed854104191 to include HillChartsService
- Implements intelligent error handling for 403 responses that suggests next steps based on whether todolists exist in the project
- Updates API coverage documentation, command registry, and CLI infrastructure to include the new commands
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/commands/hillcharts.go | New command implementation with show, track, and untrack subcommands and intelligent error handling for disabled/unavailable hill charts |
| internal/commands/commands.go | Registers hillcharts in the command catalog |
| internal/commands/commands_test.go | Adds hillcharts command to test root |
| internal/cli/root.go | Registers hillcharts command in the CLI root |
| go.mod/go.sum | Updates basecamp-sdk/go dependency version |
| internal/version/sdk-provenance.json | Updates SDK version metadata |
| API-COVERAGE.md | Documents hill charts as fully implemented (2 endpoints) |
| .surface | Registers new commands in the surface file |
| e2e/smoke/smoke_hillcharts.bats | Smoke tests for hill chart operations |
| e2e/hillcharts.bats | E2E tests for error handling and help |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR adds hill chart management capabilities to the Basecamp CLI by implementing basecamp hillcharts commands (show, track, untrack). The implementation bumps the SDK to get HillChartsService support and includes intelligent error handling for the 403 response BC3 returns when hill charts are disabled (no tracked todolists). When this occurs, the CLI detects whether the project has todolists and provides an actionable hint, enabling users to either create todolists first or start tracking existing ones.
Changes:
- Adds three new
hillchartssubcommands to manage project hill charts - Updates SDK to v0.6.1-0.20260318201020-3ed854104191 for Hill Charts support
- Implements 403 error detection with todoset sniffing to distinguish "disabled" from "access denied"
- Includes comprehensive e2e test coverage for error handling and smoke tests
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/commands/hillcharts.go | New file implementing show, track, and untrack commands with smart error handling |
| go.mod & go.sum | SDK dependency bumped to v0.6.1-0.20260318201020-3ed854104191 |
| internal/commands/commands.go | Added hillcharts to command catalog |
| internal/commands/commands_test.go | Registered NewHillchartsCmd in test builder |
| internal/cli/root.go | Registered NewHillchartsCmd in main CLI |
| internal/version/sdk-provenance.json | Updated SDK version and API revision references |
| API-COVERAGE.md | Updated to reflect Hill Charts section with 2 new endpoints |
| .surface | Added command definitions for hillcharts group and subcommands |
| e2e/smoke/smoke_hillcharts.bats | Added smoke tests for hillcharts operations |
| e2e/hillcharts.bats | Added e2e tests for error handling and help text |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
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="internal/commands/hillcharts.go">
<violation number="1" location="internal/commands/hillcharts.go:291">
P2: Silently discarded `strconv.ParseInt` error can produce a misleading ownership error (claiming todoset belongs to project `0`) if `resolvedProjectID` is ever non-numeric.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Bump SDK to pick up HillChartsService (basecamp/basecamp-sdk#195) and add `basecamp hillcharts` with three subcommands: - show: display hill chart state for a todoset - track: add todolists to the hill chart - untrack: remove todolists from the hill chart When `show` gets a 403 (BC3 returns forbidden for disabled hill charts), the CLI detects whether the project has todolists and returns an actionable hint instead of a generic "access denied".
- The help example showed --todoset as standalone but it always requires --project; updated the example to show both flags together and added an e2e test covering the --todoset-without-project path. - BC3 returns 403 when hill charts are disabled, never a 200 with enabled: false, so the "Hill chart disabled" summary was dead code. Added a FIXME noting the upstream 403 behavior.
Change <id> to <todolist-ids> in the 403 hint message to be consistent with the track subcommand's usage string.
Gate project resolution: when --todoset is explicit, skip project resolution entirely (DockTool short-circuits anyway). When both --project and --todoset are provided, validate the todoset belongs to the stated project via Todosets().Get() bucket check. Narrow the 403 heuristic to only replace the error when TodolistsCount == 0 (unambiguous: can't have a hill chart with no todolists). The TodolistsCount > 0 case was ambiguous between "disabled" and genuine access denial — let it fall through. Breadcrumbs and hints now use --todoset when the todoset was explicitly provided, --in when auto-detected from project.
Unit tests cover 403 heuristic (empty/non-empty/probe-fail), project-todoset mismatch validation, todoset-only success paths for show/track/untrack, and the scope/hint helpers. E2e tests verify --todoset bypasses project requirement for all three subcommands (show, track, untrack).
jeremy
force-pushed
the
add-hill-chart-commands
branch
from
5133877 to
48ee3c7
Compare
Return a usage error instead of silently defaulting to 0, which would produce a misleading ownership mismatch message.
There was a problem hiding this comment.
Pull request overview
Adds first-class CLI support for Basecamp hill charts by introducing a new basecamp hillcharts command group (show/track/untrack) backed by the updated SDK, including improved 403 handling when hill charts are disabled due to empty todosets.
Changes:
- Introduce
basecamp hillcharts {show,track,untrack}with--project/--inand optional--todosetscoping. - Add a 403 heuristic in
showto emit a more actionable error when the todoset has zero todolists. - Add unit tests + e2e coverage, and update CLI surface + API coverage documentation.
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.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/commands/hillcharts.go | Implements the new hillcharts command group, argument/flag handling, breadcrumbs, and 403 empty-todoset hinting. |
| internal/commands/hillcharts_test.go | Adds unit tests for show 403 behavior, todoset-only flows, and helpers. |
| internal/commands/commands.go | Registers hillcharts in the command category list for discovery/help. |
| internal/commands/commands_test.go | Ensures the all-commands test root includes NewHillchartsCmd(). |
| internal/cli/root.go | Wires the new command group into the CLI root command. |
| e2e/smoke/smoke_hillcharts.bats | Adds smoke coverage for hillcharts show/track/untrack reaching the API. |
| e2e/hillcharts.bats | Adds e2e checks for help and missing-project behavior, including todoset-only invocation. |
| API-COVERAGE.md | Updates API coverage counts and documents Hill Charts section coverage. |
| .surface | Updates the CLI surface snapshot to include the new commands and flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use ErrUsageHint helper instead of struct literal for the empty-todoset error. Fix resolveTodolistInTodoset to omit --in when projectID is empty, avoiding broken hints like '--in --todoset'. Add tests for comma-separated ID parsing and todolist name resolution.
Summary
HillChartsServicefrom Add hill chart API support (GetHillChart, UpdateHillChartSettings) basecamp-sdk#195basecamp hillchartscommand group withshow,track, anduntracksubcommandsshowhits a 403 (BC3 returns forbidden for disabled hill charts), detects whether the project has todolists and returns an actionable hint instead of a generic "access denied"Closes the gap that prevented Claude from enabling hill chart tracking via the CLI.
Companion PR Fix for API https://github.com/basecamp/bc3/pull/10007
Companion SDK PR basecamp/basecamp-sdk#203
Summary by cubic
Adds
basecamp hillchartswithshow,track, anduntrackto manage project hill charts from the CLI. Supports--todosetwithout--projectand shows clearer 403 guidance with actionable hints.New Features
basecamp hillchartsgroup:show,track,untrack.--todosetdirectly; when both--projectand--todosetare given, validates the todoset belongs to the project.track/untrackaccept comma-separated todolist IDs or names.<todolist-ids>and breadcrumbs/hints use--todosetor--into match scope.github.com/basecamp/basecamp-sdk/go@v0.6.1-0.20260318201020-3ed854104191; API coverage now includes Hill Charts (139/139 in-scope).Bug Fixes
--projectvalues during todoset ownership checks now return a usage error instead of defaulting to 0.--inwhen no project is provided, avoiding broken suggestions like--in --todoset.Written for commit 787d6e7. Summary will update on new commits.