From 0bf0015248d5628111188fb296cc671d77217022 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 29 Jul 2026 14:23:16 -0700 Subject: [PATCH 1/9] Bound the account-wide listings instead of crawling the account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Soaking v0.8.0-rc.1 against a ~80-project account showed the account-wide listings are unusable at real scale. The cause is a contract decision, not a bug: I5 pinned each command's account-wide default to its project-scoped default. But project-scoped "all" is one project's items and account-wide "all" is the whole account, so the same word bought very different work. files list --all-projects HTTP 500 mid-crawl, no flag to recover with cards list --all-projects --status completed timed out at 94s (--page 1: 24s) checkins answers --all-projects timed out (one page: 1s) I5 also blessed "fetch page 0, then truncate", which made --limit useless exactly when it mattered: cards --limit 3 fetched all 2142 cards before trimming. Add accountWideCollect, one bounded walk over positive pages shared by all eight paths. It stops on an empty page, a page that adds no items, the cap, or the server's total. Two rules callers must know, so they are stated on the helper: Meta.TotalCount compares against len(items) — the top-level elements, which for a grouped payload are the groups, not the items nested inside — and the walk does not trim, so every caller cuts to the exact cap itself. Default to 100 for the paginated listings; --all is how you ask for the account. This changes the default for cards, checkins, forwards, and files. Project-scoped defaults are untouched. Give boost list and bare files list the --limit/--page/--all they never had. A listing with no escape hatch cannot recover from a server error mid-crawl, which is exactly how the files 500 became unrecoverable. Where a path has no pagination to thread — project-scoped files list, item-scoped boost list — it rejects all three by name rather than accepting and ignoring them. Both overdue paths now accept --all and reject --page. They are unpaginated, so --page addresses nothing, but capping at 100 while also rejecting --all left item 101 unreachable by any combination of flags. boost keeps a first-page default: /boosts.json?page=1 alone measured 93s, so walking toward 100 by default would multiply an already unacceptable wait. The exemption covers the default only — its flags walk like everywhere else — and its notices are per-mode, since "Showing first page" is false for --page 2. The slowness is server-side and tracked there. Dropping accountWideRespOpts's moreFlag parameter falls out of this: it existed so the two flagless commands would not be told to use an --all they did not have. They have one now. --- .surface | 27 ++++ ACCOUNT-WIDE-LISTINGS.md | 170 ++++++++++++++++++++------ internal/commands/accountwide.go | 139 +++++++++++++++++++-- internal/commands/accountwide_test.go | 159 ++++++++++++++++++++++++ internal/commands/boost.go | 147 +++++++++++++++++++--- internal/commands/boost_test.go | 85 +++++++++++-- internal/commands/cards.go | 116 +++++++++++------- internal/commands/cards_test.go | 96 +++++++++++---- internal/commands/checkins.go | 56 ++++++--- internal/commands/checkins_test.go | 16 ++- internal/commands/comment.go | 40 +++--- internal/commands/files.go | 100 +++++++++++++-- internal/commands/files_test.go | 84 ++++++++++--- internal/commands/forwards.go | 57 ++++++--- internal/commands/forwards_test.go | 8 +- internal/commands/messages.go | 26 +--- internal/commands/messages_test.go | 16 ++- internal/commands/recording_test.go | 43 ++++++- internal/commands/todos.go | 62 +++------- internal/commands/todos_test.go | 25 +++- skills/basecamp/SKILL.md | 6 + 21 files changed, 1170 insertions(+), 308 deletions(-) diff --git a/.surface b/.surface index 8ce5ca25..5a39e551 100644 --- a/.surface +++ b/.surface @@ -2061,6 +2061,7 @@ FLAG basecamp boost delete --todolist type=string FLAG basecamp boost delete --verbose type=count FLAG basecamp boost list --account type=string FLAG basecamp boost list --agent type=bool +FLAG basecamp boost list --all type=bool FLAG basecamp boost list --all-projects type=bool FLAG basecamp boost list --cache-dir type=string FLAG basecamp boost list --count type=bool @@ -2071,10 +2072,12 @@ FLAG basecamp boost list --ids-only type=bool FLAG basecamp boost list --in type=string FLAG basecamp boost list --jq type=string FLAG basecamp boost list --json type=bool +FLAG basecamp boost list --limit type=int FLAG basecamp boost list --markdown type=bool FLAG basecamp boost list --md type=bool FLAG basecamp boost list --no-hints type=bool FLAG basecamp boost list --no-stats type=bool +FLAG basecamp boost list --page type=int FLAG basecamp boost list --profile type=string FLAG basecamp boost list --project type=string FLAG basecamp boost list --quiet type=bool @@ -2169,6 +2172,7 @@ FLAG basecamp boosts delete --todolist type=string FLAG basecamp boosts delete --verbose type=count FLAG basecamp boosts list --account type=string FLAG basecamp boosts list --agent type=bool +FLAG basecamp boosts list --all type=bool FLAG basecamp boosts list --all-projects type=bool FLAG basecamp boosts list --cache-dir type=string FLAG basecamp boosts list --count type=bool @@ -2179,10 +2183,12 @@ FLAG basecamp boosts list --ids-only type=bool FLAG basecamp boosts list --in type=string FLAG basecamp boosts list --jq type=string FLAG basecamp boosts list --json type=bool +FLAG basecamp boosts list --limit type=int FLAG basecamp boosts list --markdown type=bool FLAG basecamp boosts list --md type=bool FLAG basecamp boosts list --no-hints type=bool FLAG basecamp boosts list --no-stats type=bool +FLAG basecamp boosts list --page type=int FLAG basecamp boosts list --profile type=string FLAG basecamp boosts list --project type=string FLAG basecamp boosts list --quiet type=bool @@ -5062,6 +5068,7 @@ FLAG basecamp docs folders list --vault type=string FLAG basecamp docs folders list --verbose type=count FLAG basecamp docs list --account type=string FLAG basecamp docs list --agent type=bool +FLAG basecamp docs list --all type=bool FLAG basecamp docs list --all-projects type=bool FLAG basecamp docs list --cache-dir type=string FLAG basecamp docs list --count type=bool @@ -5073,10 +5080,12 @@ FLAG basecamp docs list --in type=string FLAG basecamp docs list --jq type=string FLAG basecamp docs list --json type=bool FLAG basecamp docs list --kind type=string +FLAG basecamp docs list --limit type=int FLAG basecamp docs list --markdown type=bool FLAG basecamp docs list --md type=bool FLAG basecamp docs list --no-hints type=bool FLAG basecamp docs list --no-stats type=bool +FLAG basecamp docs list --page type=int FLAG basecamp docs list --person type=stringArray FLAG basecamp docs list --profile type=string FLAG basecamp docs list --project type=string @@ -5973,6 +5982,7 @@ FLAG basecamp documents folders list --vault type=string FLAG basecamp documents folders list --verbose type=count FLAG basecamp documents list --account type=string FLAG basecamp documents list --agent type=bool +FLAG basecamp documents list --all type=bool FLAG basecamp documents list --all-projects type=bool FLAG basecamp documents list --cache-dir type=string FLAG basecamp documents list --count type=bool @@ -5984,10 +5994,12 @@ FLAG basecamp documents list --in type=string FLAG basecamp documents list --jq type=string FLAG basecamp documents list --json type=bool FLAG basecamp documents list --kind type=string +FLAG basecamp documents list --limit type=int FLAG basecamp documents list --markdown type=bool FLAG basecamp documents list --md type=bool FLAG basecamp documents list --no-hints type=bool FLAG basecamp documents list --no-stats type=bool +FLAG basecamp documents list --page type=int FLAG basecamp documents list --person type=stringArray FLAG basecamp documents list --profile type=string FLAG basecamp documents list --project type=string @@ -6887,6 +6899,7 @@ FLAG basecamp file folders list --vault type=string FLAG basecamp file folders list --verbose type=count FLAG basecamp file list --account type=string FLAG basecamp file list --agent type=bool +FLAG basecamp file list --all type=bool FLAG basecamp file list --all-projects type=bool FLAG basecamp file list --cache-dir type=string FLAG basecamp file list --count type=bool @@ -6898,10 +6911,12 @@ FLAG basecamp file list --in type=string FLAG basecamp file list --jq type=string FLAG basecamp file list --json type=bool FLAG basecamp file list --kind type=string +FLAG basecamp file list --limit type=int FLAG basecamp file list --markdown type=bool FLAG basecamp file list --md type=bool FLAG basecamp file list --no-hints type=bool FLAG basecamp file list --no-stats type=bool +FLAG basecamp file list --page type=int FLAG basecamp file list --person type=stringArray FLAG basecamp file list --profile type=string FLAG basecamp file list --project type=string @@ -7777,6 +7792,7 @@ FLAG basecamp files folders list --vault type=string FLAG basecamp files folders list --verbose type=count FLAG basecamp files list --account type=string FLAG basecamp files list --agent type=bool +FLAG basecamp files list --all type=bool FLAG basecamp files list --all-projects type=bool FLAG basecamp files list --cache-dir type=string FLAG basecamp files list --count type=bool @@ -7788,10 +7804,12 @@ FLAG basecamp files list --in type=string FLAG basecamp files list --jq type=string FLAG basecamp files list --json type=bool FLAG basecamp files list --kind type=string +FLAG basecamp files list --limit type=int FLAG basecamp files list --markdown type=bool FLAG basecamp files list --md type=bool FLAG basecamp files list --no-hints type=bool FLAG basecamp files list --no-stats type=bool +FLAG basecamp files list --page type=int FLAG basecamp files list --person type=stringArray FLAG basecamp files list --profile type=string FLAG basecamp files list --project type=string @@ -8667,6 +8685,7 @@ FLAG basecamp folders folders list --vault type=string FLAG basecamp folders folders list --verbose type=count FLAG basecamp folders list --account type=string FLAG basecamp folders list --agent type=bool +FLAG basecamp folders list --all type=bool FLAG basecamp folders list --all-projects type=bool FLAG basecamp folders list --cache-dir type=string FLAG basecamp folders list --count type=bool @@ -8678,10 +8697,12 @@ FLAG basecamp folders list --in type=string FLAG basecamp folders list --jq type=string FLAG basecamp folders list --json type=bool FLAG basecamp folders list --kind type=string +FLAG basecamp folders list --limit type=int FLAG basecamp folders list --markdown type=bool FLAG basecamp folders list --md type=bool FLAG basecamp folders list --no-hints type=bool FLAG basecamp folders list --no-stats type=bool +FLAG basecamp folders list --page type=int FLAG basecamp folders list --person type=stringArray FLAG basecamp folders list --profile type=string FLAG basecamp folders list --project type=string @@ -14908,6 +14929,7 @@ FLAG basecamp vault folders list --vault type=string FLAG basecamp vault folders list --verbose type=count FLAG basecamp vault list --account type=string FLAG basecamp vault list --agent type=bool +FLAG basecamp vault list --all type=bool FLAG basecamp vault list --all-projects type=bool FLAG basecamp vault list --cache-dir type=string FLAG basecamp vault list --count type=bool @@ -14919,10 +14941,12 @@ FLAG basecamp vault list --in type=string FLAG basecamp vault list --jq type=string FLAG basecamp vault list --json type=bool FLAG basecamp vault list --kind type=string +FLAG basecamp vault list --limit type=int FLAG basecamp vault list --markdown type=bool FLAG basecamp vault list --md type=bool FLAG basecamp vault list --no-hints type=bool FLAG basecamp vault list --no-stats type=bool +FLAG basecamp vault list --page type=int FLAG basecamp vault list --person type=stringArray FLAG basecamp vault list --profile type=string FLAG basecamp vault list --project type=string @@ -15798,6 +15822,7 @@ FLAG basecamp vaults folders list --vault type=string FLAG basecamp vaults folders list --verbose type=count FLAG basecamp vaults list --account type=string FLAG basecamp vaults list --agent type=bool +FLAG basecamp vaults list --all type=bool FLAG basecamp vaults list --all-projects type=bool FLAG basecamp vaults list --cache-dir type=string FLAG basecamp vaults list --count type=bool @@ -15809,10 +15834,12 @@ FLAG basecamp vaults list --in type=string FLAG basecamp vaults list --jq type=string FLAG basecamp vaults list --json type=bool FLAG basecamp vaults list --kind type=string +FLAG basecamp vaults list --limit type=int FLAG basecamp vaults list --markdown type=bool FLAG basecamp vaults list --md type=bool FLAG basecamp vaults list --no-hints type=bool FLAG basecamp vaults list --no-stats type=bool +FLAG basecamp vaults list --page type=int FLAG basecamp vaults list --person type=stringArray FLAG basecamp vaults list --profile type=string FLAG basecamp vaults list --project type=string diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index 7712404c..3b5e5d63 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -105,10 +105,15 @@ values) are rejected, pointing at the command that does answer the question (e.g. `reports assigned`). **No new flags** beyond `--all-projects`, the endpoint selectors the method -matrix names, and one recorded exception. The account-wide path reuses the -filter, sort, and pagination flags a command already has; it does not grow a -parallel flag surface. The exception is `files list`, which today has no filters -at all and gains two account-wide-only ones — see I5. +matrix names, the `files list` filters, and the pagination flags the two +flagless commands needed. The account-wide path reuses the filter and sort flags +a command already has; it does not grow a parallel flag surface for its own +sake. + +Pagination is the one place where "reuse only" turned out to be wrong. A command +with no `--limit`/`--page`/`--all` has no way to recover from a server error +mid-crawl and no way to reach past a bounded default, so `boost list` and bare +`files list` gained all three — see I5. #### Endpoint selectors @@ -127,6 +132,8 @@ flags added by this work — anything not listed here is reuse: | `cards list` | `--not-now` | `NotNowCards` | account-wide only | | `cards list` | `--overdue` | `OverdueCards` | account-wide only | | `files list` | `--kind`, `--person` | filters on `Files` | account-wide only — see I5 | +| `files list` | `--limit`/`-n`, `--page`, `--all` | pagination on `Files` | account-wide only — see I5 | +| `boost list` | `--limit`/`-n`, `--page`, `--all` | pagination on `Boosts` | account-wide only — see I5 | **Account-wide only** means exactly what it means for the `files list` filters: the project-scoped path has no equivalent, so passing one with a project in @@ -167,36 +174,85 @@ exactly page `N`. | Flag | Mapping | |---|---| -| `--all` | page 0 | +| `--all` | page 0 — the whole account | | `--page N` | page N (any positive N) | -| `--limit N` | client-side truncation to N, with an honest truncation notice | -| default | the command's documented default, per the table below | +| `--limit N` | **bounds the fetch**: walks positive pages until N items are collected, then trims to exactly N | +| default | a bounded cap, per the table below | + +`--limit` bounding the fetch is the point. Fetching page 0 and then truncating +is correct and useless: it downloads the entire account to keep the first +hundred rows, which is what made `cards list --limit 3` take 16s while +`todos list --limit 3` took none. `accountWideCollect` is the shared walk. + +**Three deliberate exceptions**, each with a reason that is not "we didn't get +to it": + +1. **Sorted `messages list`** fetches every page before capping. The cap applies + after the sort (I4), so every page has to be in hand first. Pinned by + `TestMessagesListAccountWideSortsBeforeTruncating`. +2. **The overdue endpoints** (`todos list --overdue`, `cards list --overdue`) + are unpaginated — one request returns everything — so `--limit` trims + locally. There is no walk to bound. +3. **`boost list`** keeps a first-page default rather than the cap. Its flags + still walk normally; only the default differs. See below. #### Per-command defaults -"The command's documented default" is not left to interpretation. Each -command's account-wide default matches what it already does project-scoped: +Pinning each account-wide default to its project-scoped default was a mistake, +and it is the mistake this section exists to correct. Project-scoped "all" is +one project's items; account-wide "all" is the whole account. The same word +describes two very different amounts of work, and on a ~80-project account the +account-wide reading of it timed out. + +**The default is a uniform cap of 100** (`accountWideDefaultLimit`), with two +rows that differ for stated reasons. `--all` is how you ask for the account. -| Command | Existing default | Account-wide contract | +| Command | Account-wide default | Notes | |---|---|---| -| `messages list` | 100 | cap 100 | -| `comments list` | 100 | cap 100 | -| `checkins answers` | all | all | -| `forwards list` | all | all | -| `boost list` | no paging flags | first page only | -| `files list` | no paging flags | all pages | -| `todos list` | 100 | cap 100 | -| `cards list` | all | all | - -The default must remain the documented default. Degrading a documented cap to -"one raw SDK page" is a silent contract change; so is promoting an unpaged -command to a capped one. +| `messages list` | cap 100 | unchanged | +| `comments list` | cap 100 | unchanged | +| `todos list` | cap 100 | unchanged | +| `cards list` | cap 100 | **changed** from "all pages" | +| `checkins answers` | cap 100 | **changed** from "all pages" | +| `forwards list` | cap 100 | **changed** from "all pages" | +| `files list` | cap 100 | **changed** from "all pages" | +| `todos list --overdue` | cap 100 | unpaginated endpoint; accepts `--all`, rejects `--page` | +| `cards list --overdue` | cap 100 | **changed** from uncapped; same rules as above | +| `boost list` | **first page only** | the explicit exception — see below | + +Project-scoped defaults are untouched throughout. + +**The two overdue rows.** Both endpoints are unpaginated, so `--page` has +nothing to address and stays an error. `--all` is a different question: it means +"skip the cap", and since the complete array is already in hand it costs no +extra request. Previously `todos --overdue` capped at 100 *and* rejected `--all`, +which left item 101 unreachable by any flag combination — capped with no escape +hatch. `cards --overdue` was uncapped and also rejected `--all`. + +**Changing a default is allowed when the old one was wrong.** The earlier +version of this section forbade moving a default in either direction. That rule +protected a contract that could not be honored at scale. What must not happen is +a default changing *silently*: each row marked **changed** above is a documented +behavior change and belongs in the release notes. #### Rules -- **No new pagination flags where none exist.** `boost list` and bare - `files list` have no `--limit`/`--page`/`--all` today and gain none. Their - account-wide behavior is fixed by the table above. +- **Every account-wide listing needs a bounded default and a way to ask for + more.** A listing with no escape hatch cannot recover from a server error + mid-crawl: when `files list --all-projects` returned a 500 partway through the + full-account crawl, there was no flag that could step around the failing page, + because the command had none. A bounded default without a way past it is the + same defect from the other side. +- **A registered flag must work on every path that accepts it.** Where a path + genuinely has no pagination, it rejects all three by name rather than + accepting and ignoring them (`rejectScopedPaginationFlags`): + - **Project-scoped `files list`** rejects `--limit`/`--page`/`--all`. That + path passes `nil` to `Vaults().List`, `Uploads().List`, and + `Documents().List` — three unpaginated calls with nowhere to put a page. + - **Item-scoped `boost list ` and `--event`** reject all three. The SDK + documents `BoostListOptions.Page` as not honoring the page number at all: + setting `Page=2` does not fetch page 2. Rejecting is honest; implementing + would lie. - **`--limit N` on grouped todo/card responses counts inner todos/cards, not outer project groups.** Truncating groups would silently drop whole projects. - **`Meta.TotalCount` counts groups, not items,** for the grouped responses. @@ -208,6 +264,56 @@ command to a capped one. crawl they did not ask for, and that is the same no-silent-flags defect I3 names. `--all` is the spelling for every page. +#### `boost list` — the default exception, and its server-side cause + +`boost list` is the one account-wide listing exempt from the cap-100 row. It +returns **exactly the first page** when no pagination flag is given. + +The reason is measured, not stylistic: `/boosts.json?page=1` — one page, no +crawling — took **93s** against a large production account. A default that +walked toward 100 items would multiply an already unacceptable wait. + +**The exemption covers the default only.** Once a flag is passed, `boost list` +behaves like every other bounded listing. The four modes are exhaustive: + +| Invocation | Fetch behavior | +|---|---| +| no pagination flag | exactly page 1 — one request, no walk | +| `--page N` | exactly page N — one request | +| `--limit N` | bounded walk over positive page numbers, then exact trim | +| `--all` | full traversal (page 0 / omit `page=`) | + +`--limit` walks **positive** pages (1, 2, 3, …). It must never fall back to the +page-0 full-crawl form, which is the behavior this whole section exists to +remove. + +**Notices are mode-specific.** "Showing first page" is false for `--page 2`, so +a single fixed string would state something untrue. Every mode keeps the +account-wide total: now that `--page` and `--all` make the rest of the feed +genuinely reachable — just slow — suppressing the total would understate what +exists. + +| Mode | Notice | +|---|---| +| default | `Showing first page, N of M; additional pages may be slow on large accounts` | +| `--page P` | `Showing page P, N of M; additional pages may be slow on large accounts` | +| `--limit` | the standard truncation notice used by the other bounded listings | +| `--all` | none — the traversal is complete | + +When the response is the whole listing there is no notice at all; there are no +additional pages to be slow. + +**The slowness is server-side and is not fixed here.** The CLI cannot make this +feed fast; the flags make its data reachable. `Everything::BoostsController#index` +already preloads the booster and boostable associations and uses `UNION ALL` +deliberately, so the obvious N+1 hypothesis is wrong. The remaining suspect — +**unconfirmed** — is that it wraps that union in +`Boost.from("(...) AS boosts").order(created_at: :desc, id: :desc)`; ordering a +derived table cannot use an index, which would force MySQL to materialize and +sort every accessible boost before `OFFSET`/`LIMIT` applies, making page 1 cost +the same as page N. That needs an `EXPLAIN` to confirm and is tracked +server-side, not here. + #### The `files list` filter exception `EverythingFilesOptions` carries filters the project-scoped path has no @@ -218,15 +324,15 @@ kind nor a creator. Rather than leave adopted SDK surface unreachable, bare | Flag | Value | Maps to | |---|---|---| | `--kind` | `all`, `images`, `pdfs`, `documents`, `videos` | `EverythingFilesOptions.Kind` | -| `--person` | repeatable; name, email, ID, or `me`, resolved via `resolveAssigneeID` | `EverythingFilesOptions.PeopleIDs` | +| `--person` | repeatable; name, email, ID, or `me`, resolved via `resolvePersonRoleID(ctx, app, person, "Person")` | `EverythingFilesOptions.PeopleIDs` | Both are **account-wide-only**. Passing either with a project in scope — explicit, configured, or via `--vault`/`--folder` — is `ErrUsage`, because the project-scoped path would have to ignore them, and I3 forbids that. An unrecognized `--kind` value is `ErrUsage` listing the accepted set. -This is the one deliberate exception to I3's "no new flags"; it is recorded here -so it stays an exception rather than a precedent. +These filters are account-wide-only by nature rather than by policy: the +project-scoped path has nothing to map them onto. #### The `files` group's alias spellings @@ -241,18 +347,12 @@ therefore gets its own account-wide meaning: | Spelling | Account-wide behavior | |---|---| -| `files list` | the whole feed, `--kind` free | +| `files list` | the feed, `--kind` free, bounded by the cap and its pagination flags | | `vaults` / `folders list` | **ErrUsage** — folders have no account-wide listing; points at the project-scoped form and at `files list --all-projects` | | `docs` / `documents list` | the feed pinned to `--kind documents`; an explicit `--kind` is ErrUsage, since the command name already chose | The project-scoped behavior of all three is unchanged. -**Accepted tradeoff.** Honoring a 100-item cap by fetching page 0 downloads -every page before truncating, which is correct but potentially expensive on -large accounts. Where sorting is not in play, a bounded loop over positive pages -that stops once the limit is collected is the cheaper equivalent and is -preferred. Recorded here so the cost is a decision rather than an accident. - ### I6 — Output contract Project-scoped paths return concrete types (`[]Message`, `[]Todo`). Aggregate diff --git a/internal/commands/accountwide.go b/internal/commands/accountwide.go index 482e14f9..bceb7e87 100644 --- a/internal/commands/accountwide.go +++ b/internal/commands/accountwide.go @@ -4,6 +4,8 @@ import ( "fmt" "math" + "github.com/spf13/cobra" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/basecamp/basecamp-cli/internal/appctx" @@ -18,9 +20,15 @@ import ( // endpoints answer the same question across every accessible project, so the // groups list account-wide instead of prompting. // -// Groups keep their own pagination flags rather than gaining new ones; each -// maps them onto the page number these endpoints take, where 0 means "follow -// the Link header across every page". +// Every account-wide listing needs a bounded default and a way to ask for more. +// A listing with no escape hatch cannot recover from a server error mid-crawl, +// which is why the two commands that once had no pagination flags now have +// them. Each command maps --page/--all onto the page number these endpoints +// take, where 0 means "follow the Link header across every page". +// +// The default is bounded because project-scoped "all" is one project's items +// while account-wide "all" is the whole account: the same number means very +// different work. --all is how you ask for the account. // projectKnown reports whether a project is available without prompting. func projectKnown(app *appctx.App, projectFlag string) bool { @@ -50,6 +58,115 @@ func accountWidePage(page int, all bool) (int32, error) { return int32(page), nil } +// accountWideDefaultLimit bounds every paginated account-wide listing that has +// no reason to differ. Project-scoped defaults are untouched: capping one +// project's items and capping the whole account are not the same promise. +const accountWideDefaultLimit = 100 + +// accountWideCollect walks positive pages until it has collected at least limit +// items, which is far cheaper than fetching every page only to throw most of it +// away. It is the shared engine behind every bounded account-wide listing. +// +// fetch returns one page. count reports how many items a collected slice holds: +// for a flat payload that is len, but for a payload grouped by project it is +// the number of items nested inside the groups, since capping groups would drop +// whole projects from the listing. +// +// The walk stops on the first of: an empty page, a page that adds no items, +// reaching the cap, or exhausting the server's reported total. +// +// Two things callers must know: +// +// - Meta.TotalCount is compared against len(items) — the top-level elements, +// which for a grouped payload are the groups, never the inner-item counter. +// The two counters measure different things and mixing them ends the walk +// early on grouped feeds. +// - This does not trim. It stops at the first page boundary at or past the +// cap, so it can overshoot by up to one page. Every caller trims exactly +// afterwards. +// +// capped reports that collection stopped at the cap rather than at the end of +// the listing, which is what lets a caller say "more may exist" honestly. meta +// is the first page's, whose X-Total-Count is the account-wide total. +func accountWideCollect[T any]( + fetch func(page int32) ([]T, basecamp.ListMeta, error), + count func([]T) int, + limit int, +) ([]T, bool, basecamp.ListMeta, error) { + var ( + items []T + meta basecamp.ListMeta + collected int + ) + + for page := int32(1); ; page++ { + pageItems, pageMeta, err := fetch(page) + if err != nil { + return nil, false, basecamp.ListMeta{}, err + } + if page == 1 { + meta = pageMeta + } + if len(pageItems) == 0 { + return items, false, meta, nil + } + + items = append(items, pageItems...) + + // A page that carries only empty groups makes no progress toward the + // cap; stop rather than request the same page shape forever. + n := count(items) + if n == collected { + return items, false, meta, nil + } + collected = n + + if collected >= limit { + return items, true, meta, nil + } + if pageMeta.TotalCount > 0 && len(items) >= pageMeta.TotalCount { + return items, false, meta, nil + } + } +} + +// accountWideFlatCount is the count function for the flat listings, where an +// item is its own unit and the top-level length is the item total. +func accountWideFlatCount[T any](items []T) int { return len(items) } + +// accountWideCapNotice reports that the bounded walk stopped at the cap rather +// than at the end of the listing. Without it a trimmed listing reads as a +// complete one. +// +// It defers to accountWideRespOpts whenever the server reported a total, since +// "N of M" is strictly more informative than "more may exist". The walk stops +// early by design, so on the feeds that withhold X-Total-Count the count is all +// there is to say. +func accountWideCapNotice(capped bool, meta basecamp.ListMeta, count int, plural string) string { + if !capped || meta.TotalCount > count { + return "" + } + return fmt.Sprintf( + "Showing the first %d %s; more may exist (use --all for every page, or --limit to raise the cap)", + count, plural) +} + +// rejectScopedPaginationFlags refuses --limit/--page/--all on a path that has +// no pagination to thread them onto. +// +// A flag registered on a command but ignored on one of its paths is exactly the +// defect this contract exists to prevent: the operator gets a listing that +// silently disregards what they asked for. Saying so by name costs one error +// and keeps the flag honest everywhere it is accepted. +func rejectScopedPaginationFlags(cmd *cobra.Command, scope, hint string) error { + for _, name := range []string{"limit", "page", "all"} { + if cmd.Flags().Changed(name) { + return output.ErrUsageHint(fmt.Sprintf("--%s does not apply to %s", name, scope), hint) + } + } + return nil +} + // flattenAccountWideRecordings builds the display rows for the four aggregate // feeds that return []Recording. The renderer's generic column detection drops // the nested bucket, which on an account-wide listing removes the one thing @@ -98,15 +215,16 @@ func rejectAccountWideTodolist(app *appctx.App, noun string) error { // account-wide listings. singular/plural are the noun in both forms, since a // one-item account-wide listing is common enough that "1 boosts" shows up. // -// moreFlag names the flag that widens the listing, or is empty for the -// commands I5 gives no pagination flags at all — recommending --all to a -// command that has no --all is worse than saying nothing. +// Every account-wide listing now has an --all, so the recovery advice can name +// it unconditionally. It did not always: the two flagless commands used to get +// no advice at all, since recommending --all to a command that has none is +// worse than saying nothing. // // explicitLimit reports whether the user asked for the cap. It changes the // recovery advice rather than the fact: these commands reject --all alongside // --limit, so telling someone who passed --limit to add --all names a // combination the command refuses. -func accountWideRespOpts(count int, singular, plural string, meta basecamp.ListMeta, moreFlag string, explicitLimit bool) []output.ResponseOption { +func accountWideRespOpts(count int, singular, plural string, meta basecamp.ListMeta, explicitLimit bool) []output.ResponseOption { noun := plural if count == 1 { noun = singular @@ -116,11 +234,10 @@ func accountWideRespOpts(count int, singular, plural string, meta basecamp.ListM } if meta.TotalCount > count { notice := fmt.Sprintf("Showing %d of %d results", count, meta.TotalCount) - switch { - case explicitLimit: + if explicitLimit { notice += " (raise or drop --limit for more)" - case moreFlag != "": - notice += fmt.Sprintf(" (use %s for the complete list)", moreFlag) + } else { + notice += " (use --all for the complete list)" } respOpts = append(respOpts, output.WithNotice(notice)) } diff --git a/internal/commands/accountwide_test.go b/internal/commands/accountwide_test.go index 5294d737..7b6041bb 100644 --- a/internal/commands/accountwide_test.go +++ b/internal/commands/accountwide_test.go @@ -13,6 +13,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/output" ) @@ -162,3 +164,160 @@ func TestAccountWidePageRejectsOutOfRange(t *testing.T) { require.NoError(t, err, "--all wins before the range check") assert.Equal(t, int32(0), all) } + +// --- accountWideCollect ------------------------------------------------- +// +// The bounded walk is the whole reason the account-wide listings are usable at +// account scale, so each guard gets its own test. The request count is asserted +// as often as the payload: "stopped early" and "returned the right items" are +// different claims, and only the first one is what makes these commands fast. + +// collectGroup stands in for the project-grouped payloads, where the top-level +// element is a project and the items nest inside it. +type collectGroup struct{ items []int } + +func countCollectGroups(groups []collectGroup) int { + total := 0 + for _, g := range groups { + total += len(g.items) + } + return total +} + +// collectPager serves canned pages and records how many were asked for. +type collectPager[T any] struct { + pages [][]T + metas []basecamp.ListMeta + err error + errOn int32 + requests int + asked []int32 +} + +func (p *collectPager[T]) fetch(page int32) ([]T, basecamp.ListMeta, error) { + p.requests++ + p.asked = append(p.asked, page) + if p.err != nil && page == p.errOn { + return nil, basecamp.ListMeta{}, p.err + } + idx := int(page - 1) + var meta basecamp.ListMeta + if idx < len(p.metas) { + meta = p.metas[idx] + } + if idx >= len(p.pages) { + return nil, meta, nil + } + return p.pages[idx], meta, nil +} + +func TestAccountWideCollectStopsOnEmptyPage(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1, 2, 3}, {}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, []int{1, 2, 3}, items) + assert.False(t, capped, "running out of listing is not hitting the cap") + assert.Equal(t, 2, pager.requests, "stops at the empty page rather than walking on") +} + +// A page can be non-empty at the top level while adding no items, when every +// group on it is empty. Counting groups would call that progress and loop. +func TestAccountWideCollectStopsWhenAPageAddsNoItems(t *testing.T) { + pager := &collectPager[collectGroup]{pages: [][]collectGroup{ + {{items: []int{1, 2}}}, + {{items: nil}}, + {{items: []int{3}}}, + }} + + items, capped, _, err := accountWideCollect(pager.fetch, countCollectGroups, 100) + + require.NoError(t, err) + assert.Equal(t, 2, countCollectGroups(items)) + assert.False(t, capped) + assert.Equal(t, 2, pager.requests, "the no-progress page ends the walk; page 3 is never asked for") +} + +// The helper deliberately does not trim: it stops at the first page boundary at +// or past the cap and leaves the exact cut to the caller. +func TestAccountWideCollectOvershootsToThePageBoundary(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1, 2, 3}, {4, 5, 6}, {7, 8, 9}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 4) + + require.NoError(t, err) + assert.True(t, capped) + assert.Equal(t, []int{1, 2, 3, 4, 5, 6}, items, "overshoots by up to one page; the caller trims") + assert.Equal(t, 2, pager.requests, "stops as soon as the cap is met") +} + +func TestAccountWideCollectStopsExactlyAtAPageBoundary(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1, 2, 3, 4, 5}, {6, 7, 8, 9, 10}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 5) + + require.NoError(t, err) + assert.True(t, capped) + assert.Len(t, items, 5) + assert.Equal(t, 1, pager.requests, "a cap met by page 1 must not fetch page 2") +} + +// TotalCount bounds the walk against len(items) — the top-level elements. On a +// grouped feed those are groups, not the todos or cards inside them. Comparing +// it against the inner counter would end the walk on the first page here. +func TestAccountWideCollectBoundsTotalCountAgainstTopLevelLength(t *testing.T) { + meta := basecamp.ListMeta{TotalCount: 2} + pager := &collectPager[collectGroup]{ + pages: [][]collectGroup{ + {{items: []int{1, 2, 3, 4, 5}}}, + {{items: []int{6, 7, 8, 9, 10}}}, + {{items: []int{11}}}, + }, + metas: []basecamp.ListMeta{meta, meta, meta}, + } + + items, capped, _, err := accountWideCollect(pager.fetch, countCollectGroups, 100) + + require.NoError(t, err) + assert.False(t, capped, "exhausting the listing is not hitting the cap") + assert.Len(t, items, 2, "two groups is the whole listing TotalCount promised") + assert.Equal(t, 10, countCollectGroups(items)) + assert.Equal(t, 2, pager.requests, + "TotalCount counts groups; comparing it against the 5 items on page 1 would stop a page early") +} + +// X-Total-Count is the account-wide total on every page, so the first page's +// meta is what the truncation notice should report. +func TestAccountWideCollectReportsFirstPageMeta(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2, 3}, {}}, + metas: []basecamp.ListMeta{{TotalCount: 42}, {TotalCount: 7}}, + } + + _, _, meta, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, 42, meta.TotalCount, "later pages must not overwrite the account-wide total") +} + +func TestAccountWideCollectWalksPositivePagesOnly(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{1}, {2}, {3}, {}}} + + _, _, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, []int32{1, 2, 3, 4}, pager.asked, + "page 0 is the full-account crawl this helper exists to avoid") +} + +func TestAccountWideCollectPropagatesFetchError(t *testing.T) { + boom := errors.New("boom") + pager := &collectPager[int]{pages: [][]int{{1, 2, 3}, {4}}, err: boom, errOn: 2} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.ErrorIs(t, err, boom) + assert.Nil(t, items) + assert.False(t, capped) +} diff --git a/internal/commands/boost.go b/internal/commands/boost.go index 14f111cf..b13f7f12 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -2,6 +2,7 @@ package commands import ( "fmt" + "math" "strconv" "unicode/utf8" @@ -51,6 +52,8 @@ Tip: In the TUI, press 'b' on any item to boost interactively.`, func newBoostListCmd(project *string) *cobra.Command { var eventID string var allProjects bool + var limit, page int + var all bool cmd := &cobra.Command{ Use: "list [id|url]", @@ -64,9 +67,15 @@ You can pass either an ID or a Basecamp URL: Use --event to list boosts on a specific event within the item. Without an ID, boosts from every accessible project are listed, newest -first — the first page of the feed: - basecamp boost list - basecamp boost list --all-projects`, +first. That feed is slow on large accounts, so it returns the first page +unless you ask for more: + basecamp boost list # first page + basecamp boost list --page 2 # exactly page 2 + basecamp boost list --limit 250 # walk pages until 250 collected + basecamp boost list --all # every page (slowest) + +--limit/--page/--all apply to that account-wide feed only; an item's own +boosts arrive in a single unpaginated response.`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) @@ -77,17 +86,20 @@ first — the first page of the feed: if len(args) > 0 { recording = args[0] } - return runBoostList(cmd, app, recording, *project, eventID, allProjects) + return runBoostList(cmd, app, recording, *project, eventID, allProjects, limit, page, all) }, } cmd.Flags().StringVar(&eventID, "event", "", "Event ID (for event-specific boosts)") cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List boosts across every project") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Account-wide only: maximum boosts to fetch (0 = first page)") + cmd.Flags().IntVar(&page, "page", 0, "Account-wide only: fetch exactly this page") + cmd.Flags().BoolVar(&all, "all", false, "Account-wide only: fetch every page (slow)") return cmd } -func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string, allProjects bool) error { +func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string, allProjects bool, limit, page int, all bool) error { // Boosts hang off a single recording, so a project cannot scope this // listing on its own — only an item ID can. Without one, the account-wide // feed answers instead, and a configured project is ignored rather than @@ -106,7 +118,7 @@ func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, event return output.ErrUsageHint("Boosts belong to an item, so --project alone cannot list them", fmt.Sprintf("Pass the item's ID or URL: basecamp boost list --project %s", explicitProject)) } - return runBoostListAccountWide(cmd, app, eventID) + return runBoostListAccountWide(cmd, app, eventID, limit, page, all) } if allProjects { @@ -114,6 +126,15 @@ func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, event "Drop the ID to list boosts across every project, or drop --all-projects to list that item's boosts") } + // An item's boosts come back in one unpaginated response, and the SDK's + // Page option on that feed is documented not to honor the page number at + // all. Accepting these flags here would promise addressability the endpoint + // cannot deliver. + if err := rejectScopedPaginationFlags(cmd, "an item's boosts", + "Drop the item ID to page through boosts across every project."); err != nil { + return err + } + recordingID, urlProjectID := extractWithProject(recording) projectID := project @@ -187,10 +208,29 @@ func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, event ) } -// runBoostListAccountWide lists boosts across every accessible project. The -// feed takes a page number and boost list has no paging flags to map onto it, -// so it stays on the first page rather than growing a parallel flag surface. -func runBoostListAccountWide(cmd *cobra.Command, app *appctx.App, eventID string) error { +// boostListMode names which slice of the account-wide feed was asked for. The +// notice differs per mode: "Showing first page" is simply false for --page 2, +// and a single fixed string would state it anyway. +type boostListMode int + +const ( + boostListFirstPage boostListMode = iota // no pagination flag + boostListPage // --page N + boostListLimit // --limit N + boostListAll // --all +) + +// runBoostListAccountWide lists boosts across every accessible project. +// +// This feed is the one account-wide listing that keeps a first-page default +// rather than the shared cap. /boosts.json?page=1 — a single page, no crawling +// — measured 93s against a large production account, so a default that walked +// toward 100 items would multiply an already unacceptable wait. The flags exist +// for addressability, not because they make it fast: the cost is server-side +// and tracked separately. +// +// The four modes are exhaustive and each maps to exactly one fetch shape. +func runBoostListAccountWide(cmd *cobra.Command, app *appctx.App, eventID string, limit, page int, all bool) error { if err := rejectAccountWideTodolist(app, "boost"); err != nil { return err } @@ -198,18 +238,76 @@ func runBoostListAccountWide(cmd *cobra.Command, app *appctx.App, eventID string return output.ErrUsageHint("--event names an event inside one item, which the account-wide feed has no equivalent for", "Pass the item's ID alongside --event, or drop --event to list boosts across every project") } + if limit < 0 { + return output.ErrUsage("--limit must be zero or positive") + } + if all && limit > 0 { + return output.ErrUsage("--all and --limit are mutually exclusive") + } + if page > 0 && (all || limit > 0) { + return output.ErrUsage("--page cannot be combined with --all or --limit") + } + if cmd.Flags().Changed("page") && page < 1 { + return output.ErrUsageHint( + "--page must be a positive page number", + "Omit --page for the first page, or pass --all to follow every page") + } + if page > math.MaxInt32 { + return output.ErrUsage("--page is out of range") + } + + fetch := func(p int32) ([]basecamp.EverythingBoost, basecamp.ListMeta, error) { + result, err := app.Account().Everything().Boosts(cmd.Context(), p) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return result.Boosts, result.Meta, nil + } - result, err := app.Account().Everything().Boosts(cmd.Context(), 1) + var ( + boosts []basecamp.EverythingBoost + meta basecamp.ListMeta + mode boostListMode + err error + ) + switch { + case all: + mode = boostListAll + boosts, meta, err = fetch(0) + case limit > 0: + mode = boostListLimit + boosts, _, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.EverythingBoost], limit) + // The walk stops at a page boundary, so trim to the exact cap. + if err == nil && len(boosts) > limit { + boosts = boosts[:limit] + } + case page > 0: + mode = boostListPage + boosts, meta, err = fetch(int32(page)) //nolint:gosec // bounded above by the --page range check + default: + mode = boostListFirstPage + boosts, meta, err = fetch(1) + } if err != nil { - return convertSDKError(err) + return err + } + + noun := "boosts" + if len(boosts) == 1 { + noun = "boost" } // The boosted recording is nested, so every consumer but --json and // --agent reads the flat rows: the generic renderer skips nested maps, // which would drop the project and the item title that make a boost row // mean anything. - respOpts := accountWideRespOpts(len(result.Boosts), "boost", "boosts", result.Meta, "", false) - respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideBoosts(result.Boosts))) + respOpts := []output.ResponseOption{ + output.WithSummary(fmt.Sprintf("%d %s across all projects", len(boosts), noun)), + } + if notice := boostAccountWideNotice(mode, page, len(boosts), meta.TotalCount); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) + } + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideBoosts(boosts))) respOpts = append(respOpts, output.WithBreadcrumbs( output.Breadcrumb{ Action: "show", @@ -218,7 +316,26 @@ func runBoostListAccountWide(cmd *cobra.Command, app *appctx.App, eventID string }, )) - return app.OK(result.Boosts, respOpts...) + return app.OK(boosts, respOpts...) +} + +// boostAccountWideNotice states which slice of the feed came back, in the +// wording that mode actually earns. Every mode keeps the account-wide total: +// now that --page and --all make the rest reachable, suppressing the total +// would understate what exists. When the response is the whole listing there is +// nothing left to warn about, so there is no notice at all. +func boostAccountWideNotice(mode boostListMode, page, count, total int) string { + if mode == boostListAll || total <= count { + return "" + } + if mode == boostListLimit { + return fmt.Sprintf("Showing %d of %d results (raise or drop --limit for more)", count, total) + } + where := "first page" + if mode == boostListPage { + where = fmt.Sprintf("page %d", page) + } + return fmt.Sprintf("Showing %s, %d of %d; additional pages may be slow on large accounts", where, count, total) } // flattenAccountWideBoosts turns the account-wide feed into flat rows for the diff --git a/internal/commands/boost_test.go b/internal/commands/boost_test.go index 9cbdfa10..224211f4 100644 --- a/internal/commands/boost_test.go +++ b/internal/commands/boost_test.go @@ -223,13 +223,18 @@ func accountWideBoostsRoute() stubRoute { method: http.MethodGet, path: "/99999/boosts.json", status: http.StatusOK, - body: `[{"id":1,"content":"🎉","created_at":"2024-01-01T00:00:00Z", - "booster":{"id":10,"name":"Alice"}, - "recording":{"id":456,"title":"Ship it","type":"Todo", - "bucket":{"id":123,"name":"Test Project","type":"Project"}}}]`, + body: accountWideBoostsBody, + // --limit walks positive pages; the fixture needs an end so the walk + // can stop somewhere other than the cap. + pages: []string{accountWideBoostsBody}, } } +const accountWideBoostsBody = `[{"id":1,"content":"🎉","created_at":"2024-01-01T00:00:00Z", + "booster":{"id":10,"name":"Alice"}, + "recording":{"id":456,"title":"Ship it","type":"Todo", + "bucket":{"id":123,"name":"Test Project","type":"Project"}}}]` + // recordingBoostsRoute serves the item-scoped boost listing. func recordingBoostsRoute() stubRoute { return stubRoute{ @@ -279,7 +284,52 @@ func TestBoostListWithoutIDListsAccountWide(t *testing.T) { last := transport.last(t) assert.Equal(t, "/99999/boosts.json", last.Path) - assert.Equal(t, "page=1", last.Query, "boost list has no paging flags, so it stays on the first page") + assert.Equal(t, "page=1", last.Query, "the default is exactly the first page — this feed is slow enough that walking it by default is not viable") +} + +// The four account-wide modes are exhaustive and each maps to exactly one fetch +// shape. Boost keeps a first-page default because /boosts.json?page=1 alone +// measured 93s on a large account — but the default being an exception does not +// make the flags one, and --limit walks here exactly as it does everywhere else. +func TestBoostListAccountWideFourModes(t *testing.T) { + cases := []struct { + name string + args []string + queries []string + }{ + {"no pagination flag is exactly page 1", nil, []string{"page=1"}}, + {"--page N is exactly page N", []string{"--page", "3"}, []string{"page=3"}}, + {"--limit walks positive pages", []string{"--limit", "5"}, []string{"page=1", "page=2"}}, + {"--all is the full traversal", []string{"--all"}, []string{""}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cmd, app, transport := setupBoostListTest(t, nil) + + require.NoError(t, executeBoostCommand(cmd, app, append([]string{"list"}, tc.args...)...)) + assert.Equal(t, tc.queries, transport.queriesFor("/99999/boosts.json")) + }) + } +} + +// "Showing first page" is simply false for --page 2, so the notice is +// mode-specific. Every mode keeps the total: with --page and --all registered +// the rest of the feed is genuinely reachable, just slow, so suppressing it +// would understate what exists. +func TestBoostListAccountWideNoticeIsModeSpecific(t *testing.T) { + assert.Equal(t, + "Showing first page, 1 of 40; additional pages may be slow on large accounts", + boostAccountWideNotice(boostListFirstPage, 0, 1, 40)) + assert.Equal(t, + "Showing page 2, 1 of 40; additional pages may be slow on large accounts", + boostAccountWideNotice(boostListPage, 2, 1, 40)) + assert.Equal(t, + "Showing 1 of 40 results (raise or drop --limit for more)", + boostAccountWideNotice(boostListLimit, 0, 1, 40)) + assert.Empty(t, boostAccountWideNotice(boostListAll, 0, 40, 40), + "a complete traversal has nothing to warn about") + assert.Empty(t, boostAccountWideNotice(boostListFirstPage, 0, 40, 40), + "when the first page is the whole listing there are no additional pages to be slow") } // TestBoostListIgnoresConfiguredProject verifies that a configured project — @@ -356,12 +406,33 @@ func TestBoostListAccountWideRejectsEvent(t *testing.T) { // TestBoostListHasNoPaginationFlags verifies that the account-wide path did not // grow a parallel pagination surface. -func TestBoostListHasNoPaginationFlags(t *testing.T) { +// The account-wide boost feed is slow, not unaddressable. Keeping it flagless +// meant the only reachable data was page 1; the flags do not make it fast, they +// make the rest of it reachable at all. +func TestBoostListHasPaginationFlags(t *testing.T) { list, _, err := NewBoostsCmd().Find([]string{"list"}) require.NoError(t, err) for _, name := range []string{"limit", "page", "all"} { - assert.Nil(t, list.Flags().Lookup(name), "boost list must not gain --%s", name) + assert.NotNil(t, list.Flags().Lookup(name), "boost list must carry --%s", name) + } + assert.Equal(t, "n", list.Flags().Lookup("limit").Shorthand) +} + +// An item's boosts arrive in one unpaginated response, and the SDK documents +// BoostListOptions.Page as not honoring the page number at all — setting Page=2 +// does not fetch page 2. Rejecting is honest; implementing would lie. +func TestBoostListItemScopedRejectsPaginationFlags(t *testing.T) { + for _, flag := range []string{"--limit=5", "--page=2", "--all"} { + t.Run(flag, func(t *testing.T) { + buf := &bytes.Buffer{} + cmd, app, _ := setupBoostListTest(t, buf) + + err := executeBoostCommand(cmd, app, "list", "789", "--project", "123", flag) + + require.Error(t, err, "an item's boosts have no page to address") + assert.Contains(t, err.Error(), "does not apply to an item's boosts") + }) } } diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 21d30e84..965c7a01 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -276,10 +276,13 @@ func runCardsListAccountWide(cmd *cobra.Command, app *appctx.App, opts cardsList } if selector == cardsSelectorOverdue { - if opts.pageSet || opts.all { + // --page has nothing to address on an unpaginated endpoint, but --all + // does: the listing is capped by default, and --all is how you reach + // past the cap. Rejecting both would leave card 101 unreachable. + if opts.pageSet { return output.ErrUsageHint( - "--overdue returns every overdue card in one unpaginated list", - "Drop --page/--all; use --limit to cap the result.", + "--page is not supported with --overdue (the overdue listing is not paginated)", + "Cap the results instead: basecamp cards list --overdue --limit ", ) } return runCardsListOverdue(cmd, app, opts) @@ -294,55 +297,77 @@ func runCardsListAccountWide(cmd *cobra.Command, app *appctx.App, opts cardsList ) } - // `cards list` fetches everything by default, so the account-wide default - // follows every page. accountWidePage is not used here: it defaults to page - // 1, which would silently cap a listing documented as unbounded. - apiPage := int32(0) - if opts.page > 0 { - apiPage = int32(opts.page) //nolint:gosec // bounded above by the --page range check + // Account-wide "all" is the whole account, not one project's cards, so the + // default is bounded and --all is how you ask for the rest. Walking pages + // to the cap beats fetching every page and discarding most of it. + limit := opts.limit + if limit == 0 { + limit = accountWideDefaultLimit } everything := app.Account().Everything() + fetch := func(page int32) ([]basecamp.BucketCardsGroup, basecamp.ListMeta, error) { + var ( + groupsPage *basecamp.BucketCardsGroupsPage + err error + ) + switch selector { + case cardsSelectorCompleted: + groupsPage, err = everything.CompletedCards(cmd.Context(), page) + case cardsSelectorUnassigned: + groupsPage, err = everything.UnassignedCards(cmd.Context(), page) + case cardsSelectorNoDueDate: + groupsPage, err = everything.NoDueDateCards(cmd.Context(), page) + case cardsSelectorNotNow: + groupsPage, err = everything.NotNowCards(cmd.Context(), page) + default: + groupsPage, err = everything.OpenCards(cmd.Context(), page) + } + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return groupsPage.Groups, groupsPage.Meta, nil + } + var ( - groupsPage *basecamp.BucketCardsGroupsPage - err error + groups []basecamp.BucketCardsGroup + capped bool + truncated bool ) - switch selector { - case cardsSelectorCompleted: - groupsPage, err = everything.CompletedCards(cmd.Context(), apiPage) - case cardsSelectorUnassigned: - groupsPage, err = everything.UnassignedCards(cmd.Context(), apiPage) - case cardsSelectorNoDueDate: - groupsPage, err = everything.NoDueDateCards(cmd.Context(), apiPage) - case cardsSelectorNotNow: - groupsPage, err = everything.NotNowCards(cmd.Context(), apiPage) - default: - groupsPage, err = everything.OpenCards(cmd.Context(), apiPage) - } - if err != nil { - return convertSDKError(err) + if opts.all || opts.page > 0 { + sdkPage, err := accountWidePage(opts.page, opts.all) + if err != nil { + return err + } + pageGroups, meta, err := fetch(sdkPage) + if err != nil { + return err + } + groups, truncated = pageGroups, meta.Truncated + } else { + // --limit counts cards, not project groups: truncating groups would + // drop whole projects. The walk stops at a page boundary, so trim to + // the exact cap afterwards. + collected, more, _, err := accountWideCollect(fetch, countAccountWideCards, limit) + if err != nil { + return err + } + groups, capped = truncateAccountWideCards(collected, limit), more } - // --limit counts cards, not project groups: truncating groups would drop - // whole projects. Meta.TotalCount counts groups, so the item total is ours - // to compute. - groups := groupsPage.Groups - total := countAccountWideCards(groups) - shown := total - if opts.limit > 0 && total > opts.limit { - groups = truncateAccountWideCards(groups, opts.limit) - shown = opts.limit - } + // Meta.TotalCount counts project groups rather than cards, so the item + // total is ours to compute. + count := countAccountWideCards(groups) respOpts := []output.ResponseOption{ - output.WithSummary(fmt.Sprintf("%d %s cards across %d projects", shown, selector, len(groups))), + output.WithSummary(fmt.Sprintf("%d %s cards across %d projects", count, selector, len(groups))), output.WithBreadcrumbs(cardsAccountWideBreadcrumbs()...), } switch { - case shown < total: + case capped: respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( - "Showing the first %d of %d cards; drop --limit to see them all", shown, total))) - case groupsPage.Meta.Truncated: + "Showing the first %d cards; more may exist (use --all for every page, or --limit to raise the cap)", count))) + case truncated: respOpts = append(respOpts, output.WithNotice("More pages are available; results were truncated")) } @@ -373,9 +398,15 @@ func runCardsListOverdue(cmd *cobra.Command, app *appctx.App, opts cardsListOpti sortCards(cards, opts.sortField, opts.reverse) } + // The endpoint is unpaginated, so the complete array is already in hand and + // --all costs nothing beyond skipping the cap. --limit trims locally. total := len(cards) - if opts.limit > 0 && total > opts.limit { - cards = cards[:opts.limit] + limit := opts.limit + if limit == 0 { + limit = accountWideDefaultLimit + } + if !opts.all && total > limit { + cards = cards[:limit] } respOpts := []output.ResponseOption{ @@ -385,7 +416,8 @@ func runCardsListOverdue(cmd *cobra.Command, app *appctx.App, opts cardsListOpti } if len(cards) < total { respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( - "Showing the first %d of %d cards; drop --limit to see them all", len(cards), total))) + "Showing the first %d of %d overdue cards (use --all for the complete list, or --limit to raise the cap)", + len(cards), total))) } return app.OK(cards, respOpts...) diff --git a/internal/commands/cards_test.go b/internal/commands/cards_test.go index ddf81db6..c13dd7d7 100644 --- a/internal/commands/cards_test.go +++ b/internal/commands/cards_test.go @@ -2446,9 +2446,19 @@ func cardsAggregateRoute(name, body string) stubRoute { path: fmt.Sprintf("/99999/cards/%s.json", name), status: http.StatusOK, body: body, + // The whole fixture is page 1; page 2 is where the listing runs out. + // Without a page-aware route the bounded walk would never see an empty + // page and would keep asking until it hit the cap. + pages: []string{body}, } } +// cardsAggregatePath is the path cardsAggregateRoute serves, for tests that +// assert on the request sequence. +func cardsAggregatePath(name string) string { + return fmt.Sprintf("/99999/cards/%s.json", name) +} + func cardsOpenRoute() stubRoute { return cardsAggregateRoute("open", cardsGroupsFixture) } // cardsAllAggregateRoutes serves every account-wide card endpoint, so a test @@ -2489,7 +2499,7 @@ func cardsUsageError(t *testing.T, err error) *output.Error { func TestCardsListProjectScopedUnchanged(t *testing.T) { app, transport := setupRecordingTestApp(t, projectsRoute(), - stubRoute{http.MethodGet, "/99999/card_tables/lists/12345/cards.json", http.StatusOK, `[]`}, + stubRoute{method: http.MethodGet, path: "/99999/card_tables/lists/12345/cards.json", status: http.StatusOK, body: `[]`}, ) require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--project", "123", "--column", "12345")) @@ -2613,22 +2623,44 @@ func TestCardsListAccountWideRejectsScopeChildren(t *testing.T) { } } -// TestCardsListAccountWidePagination tests the page mapping: the default and -// --all follow every page, --page N asks for exactly N. +// TestCardsListAccountWidePagination pins the request sequence, not just the +// last call. The old default asked for page 0 — one request that made the +// server walk the whole account — and --limit did the same before throwing most +// of the result away. Both now walk positive pages and stop as soon as they +// can, so the sequence is the behavior under test. func TestCardsListAccountWidePagination(t *testing.T) { cases := []struct { - args []string - query string + name string + args []string + queries []string }{ - {[]string{"--all-projects"}, ""}, - {[]string{"--all-projects", "--all"}, ""}, - {[]string{"--all-projects", "--page", "3"}, "page=3"}, - {[]string{"--all-projects", "--limit", "2"}, ""}, + { + "the default walks positive pages until the listing runs out", + []string{"--all-projects"}, + []string{"page=1", "page=2"}, + }, + { + "--all is the only spelling that asks for the full crawl", + []string{"--all-projects", "--all"}, + []string{""}, + }, + { + "--page N asks for exactly N", + []string{"--all-projects", "--page", "3"}, + []string{"page=3"}, + }, + { + "--limit stops at the first page that satisfies it", + []string{"--all-projects", "--limit", "2"}, + []string{"page=1"}, + }, } for _, tc := range cases { - app, transport := setupRecordingTestApp(t, cardsOpenRoute()) - require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, tc.args...)...), tc.args) - assert.Equal(t, tc.query, transport.last(t).Query, tc.args) + t.Run(tc.name, func(t *testing.T) { + app, transport := setupRecordingTestApp(t, cardsOpenRoute()) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, tc.args...)...)) + assert.Equal(t, tc.queries, transport.queriesFor(cardsAggregatePath("open"))) + }) } } @@ -2652,17 +2684,32 @@ func TestCardsListAccountWideRejectsBadPaging(t *testing.T) { } } -// TestCardsListOverdueRejectsPaging tests that the unpaginated endpoint refuses -// page flags instead of accepting and dropping them. -func TestCardsListOverdueRejectsPaging(t *testing.T) { - for _, args := range [][]string{ - {"--all-projects", "--overdue", "--page", "1"}, - {"--all-projects", "--overdue", "--all"}, - } { - app, _ := setupRecordingTestApp(t, cardsAllAggregateRoutes()...) - err := executeRecordingCommand(NewCardsCmd(), app, append([]string{"list"}, args...)...) - assert.Contains(t, cardsUsageError(t, err).Message, "one unpaginated list", args) +// TestCardsListOverdueRejectsPage tests that --page is refused rather than +// accepted and dropped: there is no page to address on an unpaginated endpoint. +func TestCardsListOverdueRejectsPage(t *testing.T) { + app, _ := setupRecordingTestApp(t, cardsAllAggregateRoutes()...) + err := executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--overdue", "--page", "1") + assert.Contains(t, cardsUsageError(t, err).Message, "--page is not supported with --overdue") +} + +// --all is a different question from --page. The overdue listing is capped at +// 100 by default, so rejecting --all as well would leave card 101 unreachable — +// capped with no escape hatch is the defect, not the fix. The endpoint is +// unpaginated, so honoring --all costs nothing: the complete array is already +// in hand. +func TestCardsListOverdueAcceptsAll(t *testing.T) { + app, transport, buf := setupCardsAccountWideApp(t, output.FormatJSON, cardsAllAggregateRoutes()...) + require.NoError(t, executeRecordingCommand(NewCardsCmd(), app, "list", "--all-projects", "--overdue", "--all")) + + var envelope struct { + Data []basecamp.Card `json:"data"` + Notice string `json:"notice"` } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.Len(t, envelope.Data, 2, "--all returns the complete array") + assert.Empty(t, envelope.Notice, "nothing was withheld, so there is nothing to warn about") + assert.Len(t, transport.queriesFor(cardsAggregatePath("overdue")), 1, + "--all costs no extra request on an unpaginated endpoint") } // TestCardsListAccountWideSorting tests that sorting is rejected for the @@ -2716,7 +2763,10 @@ func TestCardsListAccountWideLimitCountsCards(t *testing.T) { assert.Equal(t, "Alpha", envelope.Data[0].Bucket.Name) assert.Len(t, envelope.Data[1].Cards, 1) assert.Contains(t, envelope.Summary, "3 open cards across 2 projects") - assert.Contains(t, envelope.Notice, "first 3 of 4") + // The walk stops at the cap, so the account-wide total is unknown by + // construction — claiming "of 4" would be reporting the size of the one + // page that happened to be fetched. + assert.Contains(t, envelope.Notice, "Showing the first 3 cards; more may exist") } // TestCardsListAccountWideOutputBranches tests that machine formats keep the diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index 54412122..690af312 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -748,29 +748,49 @@ func runCheckinsAnswersAccountWide(cmd *cobra.Command, app *appctx.App, question return output.ErrUsage("--page cannot be combined with --all or --limit") } - // The project-scoped default is "0 = all", so the account-wide default - // follows every page too. Only an explicit --page narrows it to one. - sdkPage, err := accountWidePage(page, all || page == 0) - if err != nil { - return err - } - answersPage, err := app.Account().Everything().Checkins(cmd.Context(), sdkPage) - if err != nil { - return convertSDKError(err) + fetch := func(p int32) ([]basecamp.Recording, basecamp.ListMeta, error) { + answersPage, err := app.Account().Everything().Checkins(cmd.Context(), p) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return answersPage.Recordings, answersPage.Meta, nil } - answers := answersPage.Recordings - fetched := len(answers) - if limit > 0 && fetched > limit { - answers = answers[:limit] + // Account-wide "all" is every answer in the account, which on a large one + // is a crawl nobody asked for. Bound the default and let --all say it. + var ( + answers []basecamp.Recording + meta basecamp.ListMeta + capped bool + ) + if all || page > 0 { + sdkPage, err := accountWidePage(page, all) + if err != nil { + return err + } + if answers, meta, err = fetch(sdkPage); err != nil { + return err + } + } else { + effectiveLimit := limit + if effectiveLimit == 0 { + effectiveLimit = accountWideDefaultLimit + } + var err error + if answers, capped, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], effectiveLimit); err != nil { + return err + } + // The walk stops at a page boundary, so trim to the exact cap. + if len(answers) > effectiveLimit { + answers = answers[:effectiveLimit] + } } - respOpts := accountWideRespOpts(len(answers), "check-in answer", "check-in answers", answersPage.Meta, "--all", limit > 0) - respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(answers))) - if len(answers) < fetched { - respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( - "Showing %d of %d fetched check-in answers (--limit); drop --limit for all of them", len(answers), fetched))) + respOpts := accountWideRespOpts(len(answers), "check-in answer", "check-in answers", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(answers), "check-in answers"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) } + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(answers))) respOpts = append(respOpts, output.WithBreadcrumbs( output.Breadcrumb{ Action: "answer", diff --git a/internal/commands/checkins_test.go b/internal/commands/checkins_test.go index 16ff220b..ea8c2fa9 100644 --- a/internal/commands/checkins_test.go +++ b/internal/commands/checkins_test.go @@ -298,6 +298,10 @@ func checkinsAccountWideRoute() stubRoute { path: checkinsAccountWidePath, status: http.StatusOK, body: checkinsAccountWideBody, + // The bounded default walks positive pages until one comes back empty, + // so the fixture has to have an end. Serving the same body for every + // page would walk to the cap instead. + pages: []string{checkinsAccountWideBody}, } } @@ -338,9 +342,10 @@ func TestCheckinsAnswersWithoutQuestionListsAccountWide(t *testing.T) { transport, err := runCheckinsAnswersAccountWideCmd(t) require.NoError(t, err) - last := transport.last(t) - assert.Equal(t, checkinsAccountWidePath, last.Path) - assert.Empty(t, last.Query, "the default follows every page, which sends no page param") + // The default used to send no page param at all, which asked the server to + // crawl the whole account — the shape that timed out in production. It now + // walks positive pages and stops at the first empty one. + assert.Equal(t, []string{"page=1", "page=2"}, transport.queriesFor(checkinsAccountWidePath)) } // A configured project cannot scope a question's answers, so it is ignored @@ -479,7 +484,10 @@ func TestCheckinsAnswersAccountWideLimitTruncatesWithNotice(t *testing.T) { require.NoError(t, json.Unmarshal(buf.Bytes(), &resp)) assert.Len(t, resp.Data, 1) assert.Equal(t, "1 check-in answer across all projects", resp.Summary) - assert.Contains(t, resp.Notice, "Showing 1 of 2 fetched check-in answers") + // "of 2 fetched" was an artifact of fetching everything and then trimming. + // The walk now stops at the cap, so what it can honestly report is that + // more may exist — not a total it never saw. + assert.Contains(t, resp.Notice, "Showing the first 1 check-in answers; more may exist") } // []Recording is what `recordings list` already hands the styled renderer, so diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 1172187c..bb32832c 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -192,11 +192,6 @@ func runCommentsListForItem(cmd *cobra.Command, app *appctx.App, recordingArg st return app.OK(comments, respOpts...) } -// defaultAccountWideCommentLimit caps the account-wide feed at the same 100 -// comments `comments list ` defaults to. Dropping the item must not -// silently promote a bounded command into a full-account crawl. -const defaultAccountWideCommentLimit = 100 - // runCommentsListAccountWide lists every comment in the account, newest first. // The payload is []Recording, which machine formats get raw. Human-facing // output gets flattened rows instead: the generic renderers skip the nested @@ -245,7 +240,7 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page } comments, meta = result.Recordings, result.Meta default: - effectiveLimit := defaultAccountWideCommentLimit + effectiveLimit := accountWideDefaultLimit if limit > 0 { effectiveLimit = limit } @@ -256,7 +251,7 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page } } - respOpts := append(accountWideRespOpts(len(comments), "comment", "comments", meta, "--all", limit > 0), + respOpts := append(accountWideRespOpts(len(comments), "comment", "comments", meta, limit > 0), output.WithDisplayData(flattenAccountWideRecordings(comments)), output.WithBreadcrumbs( output.Breadcrumb{ @@ -280,26 +275,21 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page // before truncating — correct, but it downloads every comment to keep 100. // Each iteration adds at least one comment, so the walk always terminates. func fetchAccountWideComments(cmd *cobra.Command, app *appctx.App, limit int) ([]basecamp.Recording, basecamp.ListMeta, error) { - var ( - comments []basecamp.Recording - meta basecamp.ListMeta + comments, _, meta, err := accountWideCollect( + func(page int32) ([]basecamp.Recording, basecamp.ListMeta, error) { + result, err := app.Account().Everything().Comments(cmd.Context(), page) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return result.Recordings, result.Meta, nil + }, + accountWideFlatCount[basecamp.Recording], + limit, ) - for page := int32(1); len(comments) < limit; page++ { - result, err := app.Account().Everything().Comments(cmd.Context(), page) - if err != nil { - return nil, meta, convertSDKError(err) - } - if page == 1 { - // X-Total-Count is the account-wide total on every page; take it - // from the first so the truncation notice is honest about how much - // this walk left behind. - meta = result.Meta - } - if len(result.Recordings) == 0 { - break - } - comments = append(comments, result.Recordings...) + if err != nil { + return nil, basecamp.ListMeta{}, err } + // The walk stops at a page boundary, so trim to the exact cap. if len(comments) > limit { comments = comments[:limit] } diff --git a/internal/commands/files.go b/internal/commands/files.go index 910f9f57..1f42f742 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -3,6 +3,7 @@ package commands import ( "fmt" "io" + "math" "net/url" "os" "path/filepath" @@ -135,6 +136,8 @@ func newFilesListCmd(project, vaultID *string) *cobra.Command { var allProjects bool var kind string var people []string + var limit, page int + var all bool cmd := &cobra.Command{ Use: "list", @@ -146,23 +149,31 @@ instead of asking which project to use. Pass --all-projects to list account-wide even when a project is configured. --kind and --person filter the account-wide listing only; the project-scoped -folder listing has no equivalent for them.`, +folder listing has no equivalent for them. + +--limit/--page/--all page through the account-wide listing, which returns +the first 100 files by default. A project's folder listing is a single +unpaginated response, so it rejects all three.`, Example: ` basecamp files list --in my-project basecamp files list --all-projects --kind images - basecamp files list --all-projects --person me`, + basecamp files list --all-projects --person me + basecamp files list --all-projects --limit 500`, RunE: func(cmd *cobra.Command, args []string) error { - return runFilesList(cmd, *project, *vaultID, allProjects, kind, people) + return runFilesList(cmd, *project, *vaultID, allProjects, kind, people, limit, page, all) }, } cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List files across every accessible project") cmd.Flags().StringVar(&kind, "kind", "", fmt.Sprintf("Account-wide only: filter by file kind (%s)", strings.Join(filesAccountWideKinds, ", "))) cmd.Flags().StringArrayVar(&people, "person", nil, "Account-wide only: filter by creator (name, email, ID, or \"me\"; repeatable)") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Account-wide only: maximum files to fetch (0 = default 100)") + cmd.Flags().IntVar(&page, "page", 0, "Account-wide only: fetch a single page (use --all for everything)") + cmd.Flags().BoolVar(&all, "all", false, "Account-wide only: fetch every page") return cmd } -func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, kind string, people []string) error { +func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, kind string, people []string, limit, page int, all bool) error { app := appctx.FromContext(cmd.Context()) // Scope is settled before any scope-specific validation, and an explicit @@ -184,7 +195,16 @@ func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, // any layer keeps the folder listing. With nothing to scope to, list // account-wide rather than prompting. if allProjects || !projectKnown(app, project) { - return runFilesListAccountWide(cmd, app, vaultID, kind, people) + return runFilesListAccountWide(cmd, app, vaultID, kind, people, limit, page, all) + } + + // A project's folder listing is three unpaginated calls — Vaults, Uploads, + // and Documents each get a nil options struct, so there is no page number + // to thread. Accepting a cap here and ignoring it is the defect these + // scope rules exist to prevent. + if err := rejectScopedPaginationFlags(cmd, "a project's folder listing", + "Drop --project/--in (and pass --all-projects if a project is configured) to page through files across every project."); err != nil { + return err } // The account-wide filters have no project-scoped equivalent, so a project @@ -320,7 +340,7 @@ func runFilesList(cmd *cobra.Command, project, vaultID string, allProjects bool, // runFilesListAccountWide lists every file across all accessible projects. // The bare files listing has no --limit/--page/--all project-scoped and gains // none here, so it always follows the Link header across every page. -func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind string, people []string) error { +func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind string, people []string, limit, page int, all bool) error { if err := rejectAccountWideTodolist(app, "file"); err != nil { return err } @@ -354,21 +374,77 @@ func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind ) } + if limit < 0 { + return output.ErrUsage("--limit must be zero or positive") + } + if all && limit > 0 { + return output.ErrUsage("--all and --limit are mutually exclusive") + } + if page > 0 && (all || limit > 0) { + return output.ErrUsage("--page cannot be combined with --all or --limit") + } + if cmd.Flags().Changed("page") && page < 1 { + return output.ErrUsageHint( + "--page must be a positive page number", + "Omit --page, or pass --all, to follow every page") + } + if page > math.MaxInt32 { + return output.ErrUsage("--page is out of range") + } + opts, err := filesAccountWideOptions(cmd, app, kind, people) if err != nil { return err } - page, err := app.Account().Everything().Files(cmd.Context(), 0, opts) - if err != nil { - return convertSDKError(err) + fetch := func(p int32) ([]basecamp.EverythingFile, basecamp.ListMeta, error) { + result, err := app.Account().Everything().Files(cmd.Context(), p, opts) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) + } + return result.Files, result.Meta, nil + } + + // The default used to follow the Link header across the whole account, + // which on a large one is both slow and fragile: a single failing page + // takes down the entire listing, and before these flags existed there was + // no way to step around it. Bound the default and let --page/--all reach + // the rest. + var ( + files []basecamp.EverythingFile + meta basecamp.ListMeta + capped bool + ) + if all || page > 0 { + sdkPage, err := accountWidePage(page, all) + if err != nil { + return err + } + if files, meta, err = fetch(sdkPage); err != nil { + return err + } + } else { + effectiveLimit := limit + if effectiveLimit == 0 { + effectiveLimit = accountWideDefaultLimit + } + if files, capped, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.EverythingFile], effectiveLimit); err != nil { + return err + } + // The walk stops at a page boundary, so trim to the exact cap. + if len(files) > effectiveLimit { + files = files[:effectiveLimit] + } } // EverythingFile is an all-pointer superset far too wide to render raw, and // its bucket is nested, so every consumer but --json and --agent reads the // flat rows. - respOpts := accountWideRespOpts(len(page.Files), "file", "files", page.Meta, "", false) - respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideFiles(page.Files))) + respOpts := accountWideRespOpts(len(files), "file", "files", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(files), "files"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) + } + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideFiles(files))) respOpts = append(respOpts, output.WithBreadcrumbs( output.Breadcrumb{ Action: "kind", @@ -382,7 +458,7 @@ func runFilesListAccountWide(cmd *cobra.Command, app *appctx.App, vaultID, kind }, )) - return app.OK(page.Files, respOpts...) + return app.OK(files, respOpts...) } // filesAccountWideOptions validates --kind and resolves --person into the diff --git a/internal/commands/files_test.go b/internal/commands/files_test.go index 4557e4bd..78f881bb 100644 --- a/internal/commands/files_test.go +++ b/internal/commands/files_test.go @@ -922,6 +922,8 @@ func filesAccountWideRoute(body string) stubRoute { path: "/99999/files.json", status: http.StatusOK, body: body, + // The bounded default walks positive pages until one comes back empty. + pages: []string{body}, } } @@ -930,12 +932,12 @@ func filesAccountWideRoute(body string) stubRoute { func filesProjectScopedRoutes() []stubRoute { return []stubRoute{ projectsRoute(), - {http.MethodGet, "/99999/projects/123.json", http.StatusOK, - `{"id":123,"name":"Test Project","dock":[{"id":555,"name":"vault","title":"Docs & Files","enabled":true}]}`}, - {http.MethodGet, "/99999/vaults/555", http.StatusOK, `{"id":555,"title":"Docs & Files"}`}, - {http.MethodGet, "/99999/vaults/555/vaults.json", http.StatusOK, `[]`}, - {http.MethodGet, "/99999/vaults/555/uploads.json", http.StatusOK, `[]`}, - {http.MethodGet, "/99999/vaults/555/documents.json", http.StatusOK, `[]`}, + {method: http.MethodGet, path: "/99999/projects/123.json", status: http.StatusOK, + body: `{"id":123,"name":"Test Project","dock":[{"id":555,"name":"vault","title":"Docs & Files","enabled":true}]}`}, + {method: http.MethodGet, path: "/99999/vaults/555", status: http.StatusOK, body: `{"id":555,"title":"Docs & Files"}`}, + {method: http.MethodGet, path: "/99999/vaults/555/vaults.json", status: http.StatusOK, body: `[]`}, + {method: http.MethodGet, path: "/99999/vaults/555/uploads.json", status: http.StatusOK, body: `[]`}, + {method: http.MethodGet, path: "/99999/vaults/555/documents.json", status: http.StatusOK, body: `[]`}, } } @@ -1068,8 +1070,7 @@ func TestFilesListKindReachesTheQuery(t *testing.T) { func TestFilesListPersonResolvesAndReachesTheQuery(t *testing.T) { app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`), - stubRoute{http.MethodGet, "/99999/people.json", http.StatusOK, - `[{"id":77,"name":"Ann Perkins"},{"id":88,"name":"Bo Diaz"}]`}, + stubRoute{method: http.MethodGet, path: "/99999/people.json", status: http.StatusOK, body: `[{"id":77,"name":"Ann Perkins"},{"id":88,"name":"Bo Diaz"}]`}, ) require.NoError(t, runFilesListCmd(t, app, "--person", "Ann Perkins", "--person", "88")) @@ -1078,20 +1079,71 @@ func TestFilesListPersonResolvesAndReachesTheQuery(t *testing.T) { assert.Contains(t, query, "88") } -func TestFilesListAccountWideFollowsEveryPage(t *testing.T) { +// The old default asked for page 0 — the full-account crawl — and that is the +// request that returned a 500 mid-listing with no flag available to step around +// it. The default is now a bounded walk over positive pages. +func TestFilesListAccountWideDefaultWalksBoundedPages(t *testing.T) { app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) require.NoError(t, runFilesListCmd(t, app)) - assert.NotContains(t, transport.last(t).Query, "page=", - "bare files list is pinned at every page, so it sends no page argument") + assert.Equal(t, []string{"page=1"}, transport.queriesFor("/99999/files.json"), + "an empty first page ends the walk immediately") } -func TestFilesListGainsNoPaginationFlags(t *testing.T) { - listCmd, _, err := NewFilesCmd().Find([]string{"list"}) - require.NoError(t, err) +// --all is the one spelling that still asks for the whole account. +func TestFilesListAccountWideAllFollowsEveryPage(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + require.NoError(t, runFilesListCmd(t, app, "--all")) + assert.Equal(t, []string{""}, transport.queriesFor("/99999/files.json"), + "the endpoint spells the full crawl as an absent page") +} + +func TestFilesListAccountWidePageAsksForExactlyThatPage(t *testing.T) { + app, transport := setupRecordingTestApp(t, filesAccountWideRoute(`[]`)) + + require.NoError(t, runFilesListCmd(t, app, "--page", "4")) + assert.Equal(t, []string{"page=4"}, transport.queriesFor("/99999/files.json")) +} + +// Every account-wide listing needs a bounded default and a way to ask for more. +// files list had neither, so a 500 mid-crawl left no way to step around the bad +// page. The three group constructors share newFilesListCmd, so the flags have +// to be present under all six spellings, not just the one they were added for. +func TestFilesListHasPaginationFlagsUnderEverySpelling(t *testing.T) { + spellings := map[string]func() *cobra.Command{ + "files": NewFilesCmd, + "vaults": NewVaultsCmd, + "docs": NewDocsCmd, + } - for _, name := range []string{"limit", "page", "all"} { - assert.Nil(t, listCmd.Flags().Lookup(name), "files list must not grow --%s", name) + for spelling, ctor := range spellings { + t.Run(spelling, func(t *testing.T) { + listCmd, _, err := ctor().Find([]string{"list"}) + require.NoError(t, err) + + for _, name := range []string{"limit", "page", "all"} { + assert.NotNil(t, listCmd.Flags().Lookup(name), "%s list must carry --%s", spelling, name) + } + assert.Equal(t, "n", listCmd.Flags().Lookup("limit").Shorthand) + }) + } +} + +// The flags are account-wide only: a project's folder listing is three +// unpaginated SDK calls with nowhere to put a page number. Accepting a cap and +// then ignoring it is precisely the defect the contract forbids. +func TestFilesListProjectScopedRejectsPaginationFlags(t *testing.T) { + for _, flag := range []string{"--limit=5", "--page=2", "--all"} { + t.Run(flag, func(t *testing.T) { + app, _ := setupMessagesTestApp(t) + cmd := NewFilesCmd() + + err := executeMessagesCommand(cmd, app, "list", "--in", "123", flag) + + require.Error(t, err, "a project-scoped folder listing has no pagination to thread") + assert.Contains(t, err.Error(), "does not apply to a project's folder listing") + }) } } diff --git a/internal/commands/forwards.go b/internal/commands/forwards.go index cce352b6..01a4c0e5 100644 --- a/internal/commands/forwards.go +++ b/internal/commands/forwards.go @@ -211,31 +211,50 @@ func runForwardsListEverywhere(cmd *cobra.Command, app *appctx.App, inboxID stri return err } - // The endpoint spells "follow every page" as page 0, which is where both - // --all and the default land: this command's project-scoped default is - // already "0 = all", so listing account-wide does not silently start - // capping. Only an explicit positive --page narrows to a single page. - var sdkPage int32 - if page > 0 { - var err error - if sdkPage, err = accountWidePage(page, all); err != nil { - return err + fetch := func(p int32) ([]basecamp.Recording, basecamp.ListMeta, error) { + result, err := app.Account().Everything().Forwards(cmd.Context(), p) + if err != nil { + return nil, basecamp.ListMeta{}, convertSDKError(err) } + return result.Recordings, result.Meta, nil } - result, err := app.Account().Everything().Forwards(cmd.Context(), sdkPage) - if err != nil { - return convertSDKError(err) + // The endpoint spells "follow every page" as page 0, which is where --all + // lands. The default no longer goes there: one project's inbox and every + // project's inboxes are different amounts of work, so the account-wide + // default is bounded and --all is how you ask for the rest. + var ( + forwards []basecamp.Recording + meta basecamp.ListMeta + capped bool + ) + if all || page > 0 { + sdkPage, err := accountWidePage(page, all) + if err != nil { + return err + } + if forwards, meta, err = fetch(sdkPage); err != nil { + return err + } + } else { + effectiveLimit := limit + if effectiveLimit == 0 { + effectiveLimit = accountWideDefaultLimit + } + var err error + if forwards, capped, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], effectiveLimit); err != nil { + return err + } + // The walk stops at a page boundary, so trim to the exact cap. + if len(forwards) > effectiveLimit { + forwards = forwards[:effectiveLimit] + } } - // --limit truncates client-side; accountWideRespOpts reports the shortfall - // against the server's total so the trim is visible. - forwards := result.Recordings - if limit > 0 && len(forwards) > limit { - forwards = forwards[:limit] + respOpts := accountWideRespOpts(len(forwards), "forward", "forwards", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(forwards), "forwards"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) } - - respOpts := accountWideRespOpts(len(forwards), "forward", "forwards", result.Meta, "--all", limit > 0) respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(forwards))) respOpts = append(respOpts, output.WithBreadcrumbs( diff --git a/internal/commands/forwards_test.go b/internal/commands/forwards_test.go index f0a0820b..4192bc24 100644 --- a/internal/commands/forwards_test.go +++ b/internal/commands/forwards_test.go @@ -42,6 +42,8 @@ func forwardsAccountWideRoute(n int) stubRoute { path: forwardsAccountWidePath, status: http.StatusOK, body: forwardsFeedBody(n), + // The bounded default walks positive pages until one comes back empty. + pages: []string{forwardsFeedBody(n)}, } } @@ -92,9 +94,9 @@ func TestForwardsListWithoutProjectListsAccountWide(t *testing.T) { err := executeRecordingCommand(NewForwardsCmd(), app, "list") require.NoError(t, err) - last := transport.last(t) - assert.Equal(t, forwardsAccountWidePath, last.Path) - assert.Empty(t, last.Query, "the default follows every page, which the endpoint spells as an absent page") + // The default is bounded now: one project's inbox and every project's + // inboxes are different amounts of work, so only --all asks for the latter. + assert.Equal(t, []string{"page=1", "page=2"}, transport.queriesFor(forwardsAccountWidePath)) } func TestForwardsListAllProjectsOverridesConfiguredProject(t *testing.T) { diff --git a/internal/commands/messages.go b/internal/commands/messages.go index aca72e03..d77d36e8 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -197,11 +197,6 @@ func messagesListBreadcrumbs(resolvedProjectID string) []output.Breadcrumb { } } -// messagesAccountWideLimit is the account-wide default cap. It matches what the -// project-scoped list already returns by default, so dropping the project does -// not quietly change how much a caller gets back. -const messagesAccountWideLimit = 100 - // runMessagesListAccountWide lists every message across all accessible // projects. The feed is flat []Recording, and each item carries its own // bucket — which the generic renderers skip by name, so human-facing output @@ -232,7 +227,7 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar capped := !all && page == 0 wanted := limit if wanted == 0 { - wanted = messagesAccountWideLimit + wanted = accountWideDefaultLimit } recordings, meta, err := messagesAccountWideFetch(cmd.Context(), app, wanted, page, all, sortField != "") @@ -249,7 +244,7 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar recordings = recordings[:wanted] } - respOpts := accountWideRespOpts(len(recordings), "message", "messages", meta, "--all", limit > 0) + respOpts := accountWideRespOpts(len(recordings), "message", "messages", meta, limit > 0) respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(recordings))) respOpts = append(respOpts, output.WithBreadcrumbs(messagesAccountWideBreadcrumbs()...)) @@ -286,20 +281,9 @@ func messagesAccountWideFetch(ctx context.Context, app *appctx.App, wanted, page return fetch(0) } - var recordings []basecamp.Recording - var meta basecamp.ListMeta - for p := int32(1); len(recordings) < wanted; p++ { - pageRecordings, pageMeta, err := fetch(p) - if err != nil { - return nil, basecamp.ListMeta{}, err - } - if p == 1 { - meta = pageMeta - } - if len(pageRecordings) == 0 { - break - } - recordings = append(recordings, pageRecordings...) + recordings, _, meta, err := accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], wanted) + if err != nil { + return nil, basecamp.ListMeta{}, err } return recordings, meta, nil } diff --git a/internal/commands/messages_test.go b/internal/commands/messages_test.go index bb15bb50..6e47a1fb 100644 --- a/internal/commands/messages_test.go +++ b/internal/commands/messages_test.go @@ -658,10 +658,8 @@ func requireMessagesUsageError(t *testing.T, err error, want string) { func TestMessagesListProjectScopedUnchanged(t *testing.T) { app, transport := setupRecordingTestApp(t, projectsRoute(), - stubRoute{http.MethodGet, "/99999/projects/123.json", http.StatusOK, - `{"id":123,"dock":[{"name":"message_board","id":777,"enabled":true}]}`}, - stubRoute{http.MethodGet, "/99999/message_boards/777/messages.json", http.StatusOK, - `[{"id":1,"subject":"Board message"}]`}, + stubRoute{method: http.MethodGet, path: "/99999/projects/123.json", status: http.StatusOK, body: `{"id":123,"dock":[{"name":"message_board","id":777,"enabled":true}]}`}, + stubRoute{method: http.MethodGet, path: "/99999/message_boards/777/messages.json", status: http.StatusOK, body: `[{"id":1,"subject":"Board message"}]`}, ) app.Config.ProjectID = "123" @@ -674,11 +672,11 @@ func TestMessagesListProjectScopedUnchanged(t *testing.T) { // TestMessagesListAccountWideWithoutAnyProject verifies that with no project // anywhere the list goes account-wide instead of prompting for one (I7). func TestMessagesListAccountWideWithoutAnyProject(t *testing.T) { - app, transport := setupRecordingTestApp(t, messagesFeedRouteOfSize(messagesAccountWideLimit)) + app, transport := setupRecordingTestApp(t, messagesFeedRouteOfSize(accountWideDefaultLimit)) data := runMessagesListJSON(t, app) - require.Len(t, data, messagesAccountWideLimit) + require.Len(t, data, accountWideDefaultLimit) require.Len(t, messagesRequestsTo(transport, messagesAccountWidePath), 1) assert.Empty(t, messagesRequestsTo(transport, "/99999/projects.json"), "account-wide listing should not resolve a project") @@ -687,12 +685,12 @@ func TestMessagesListAccountWideWithoutAnyProject(t *testing.T) { // TestMessagesListAllProjectsOverridesConfiguredProject verifies --all-projects // wins over ambient config rather than being ignored. func TestMessagesListAllProjectsOverridesConfiguredProject(t *testing.T) { - app, transport := setupRecordingTestApp(t, projectsRoute(), messagesFeedRouteOfSize(messagesAccountWideLimit)) + app, transport := setupRecordingTestApp(t, projectsRoute(), messagesFeedRouteOfSize(accountWideDefaultLimit)) app.Config.ProjectID = "123" data := runMessagesListJSON(t, app, "--all-projects") - require.Len(t, data, messagesAccountWideLimit) + require.Len(t, data, accountWideDefaultLimit) require.Len(t, messagesRequestsTo(transport, messagesAccountWidePath), 1) assert.Empty(t, messagesRequestsTo(transport, "/99999/projects/123.json"), "--all-projects should override the configured project") @@ -801,7 +799,7 @@ func TestMessagesListAccountWideDefaultCapsAt100(t *testing.T) { data := runMessagesListJSON(t, app) - assert.Len(t, data, messagesAccountWideLimit) + assert.Len(t, data, accountWideDefaultLimit) calls := messagesRequestsTo(transport, messagesAccountWidePath) require.Len(t, calls, 2, "expected the cap to be filled from two pages") assert.Contains(t, calls[0].Query, "page=1") diff --git a/internal/commands/recording_test.go b/internal/commands/recording_test.go index 71ddd08a..0deced55 100644 --- a/internal/commands/recording_test.go +++ b/internal/commands/recording_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "strconv" "strings" "sync" "testing" @@ -41,6 +42,28 @@ type stubRoute struct { path string status int body string + + // pages, when set, makes the route page-aware: page=N serves pages[N-1] + // and anything past the end serves an empty array, while an absent or zero + // page serves body — the whole listing, which is what the real endpoints + // return for the "follow every page" form. + // + // Account-wide fixtures need this. The bounded walk stops on the first + // empty page, so a route that served the same body for every page number + // would walk all the way to the cap and hide the behavior under test. + pages []string +} + +// pagedBody picks the response for a page-aware route. +func pagedBody(route stubRoute, rawPage string) string { + if rawPage == "" || rawPage == "0" { + return route.body + } + n, err := strconv.Atoi(rawPage) + if err != nil || n < 1 || n > len(route.pages) { + return "[]" + } + return route.pages[n-1] } // recordingTransport is an http.RoundTripper that records every request and @@ -75,10 +98,14 @@ func (t *recordingTransport) RoundTrip(req *http.Request) (*http.Response, error for _, route := range t.routes { if route.method == req.Method && route.path == req.URL.Path { + responseBody := route.body + if route.pages != nil { + responseBody = pagedBody(route, req.URL.Query().Get("page")) + } return &http.Response{ StatusCode: route.status, Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: io.NopCloser(strings.NewReader(route.body)), + Body: io.NopCloser(strings.NewReader(responseBody)), Request: req, }, nil } @@ -100,6 +127,20 @@ func (t *recordingTransport) recorded() []recordedCall { return append([]recordedCall(nil), t.requests...) } +// queriesFor returns the raw query string of every recorded request to path, in +// order. The account-wide tests assert on the whole sequence rather than the +// last call: the request count, and whether any request omitted page= at all, +// are exactly what separates a bounded walk from a full-account crawl. +func (t *recordingTransport) queriesFor(path string) []string { + queries := []string{} + for _, call := range t.recorded() { + if call.Path == path { + queries = append(queries, call.Query) + } + } + return queries +} + // last returns the most recent request, failing the test if none were made. func (t *recordingTransport) last(tb testing.TB) recordedCall { tb.Helper() diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 7264bc3f..b0003186 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -226,10 +226,6 @@ const ( todosFilterOverdue ) -// todosAccountWideDefaultLimit mirrors the project-scoped default: 100 todos -// unless --limit or --all says otherwise. -const todosAccountWideDefaultLimit = 100 - // listTodosAcrossProjects answers `todos list` from the account-wide aggregates // when no project is in scope, or when --all-projects overrides a configured // one. Flags that only mean something inside one project are rejected here @@ -356,7 +352,7 @@ func selectAccountWideTodosFilter(flags todosListFlags) (todosAccountWideFilter, func listGroupedTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags todosListFlags, filter todosAccountWideFilter) error { limit := flags.limit if limit == 0 { - limit = todosAccountWideDefaultLimit + limit = accountWideDefaultLimit } var groups []basecamp.BucketTodosGroup @@ -427,11 +423,6 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t "--page is not supported with --overdue (the overdue listing is not paginated)", "Cap the results instead: basecamp todos list --overdue --limit ") } - if flags.all { - return output.ErrUsageHint( - "--all is not supported with --overdue (the overdue listing is already complete)", - "Raise the cap instead: basecamp todos list --overdue --limit ") - } if flags.sortField == "position" { return output.ErrUsage("--sort position requires --list (position is per-todolist)") } @@ -447,11 +438,13 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t sortTodos(todos, flags.sortField, flags.reverse) } + // The endpoint is unpaginated, so the complete array is already in hand and + // --all costs nothing beyond skipping the cap. --limit trims locally. limit := flags.limit if limit == 0 { - limit = todosAccountWideDefaultLimit + limit = accountWideDefaultLimit } - if len(todos) > limit { + if !flags.all && len(todos) > limit { todos = todos[:limit] } @@ -477,7 +470,8 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t } if total > len(todos) { respOpts = append(respOpts, output.WithNotice(fmt.Sprintf( - "Showing %d of %d overdue todos (use --limit to raise the cap)", len(todos), total))) + "Showing %d of %d overdue todos (use --all for the complete list, or --limit to raise the cap)", + len(todos), total))) } return app.OK(todos, respOpts...) @@ -488,36 +482,18 @@ func listOverdueTodosAcrossProjects(cmd *cobra.Command, app *appctx.App, flags t // truncate. The second return reports that collection stopped at the cap rather // than at the end of the listing. func collectAccountWideTodoGroups(ctx context.Context, app *appctx.App, filter todosAccountWideFilter, limit int) ([]basecamp.BucketTodosGroup, bool, error) { - var groups []basecamp.BucketTodosGroup - count := 0 - - for page := int32(1); ; page++ { - result, err := fetchAccountWideTodoGroups(ctx, app, filter, page) - if err != nil { - return nil, false, err - } - if len(result.Groups) == 0 { - return groups, false, nil - } - - groups = append(groups, result.Groups...) - fetched := countAccountWideTodos(groups) - if fetched == count { - // A page of empty groups makes no progress toward the cap; - // stop rather than request the same page shape forever. - return groups, false, nil - } - count = fetched - - if count >= limit { - return groups, true, nil - } - // TotalCount counts groups, so it bounds the walk even though it - // cannot bound the todos. - if result.Meta.TotalCount > 0 && len(groups) >= result.Meta.TotalCount { - return groups, false, nil - } - } + groups, capped, _, err := accountWideCollect( + func(page int32) ([]basecamp.BucketTodosGroup, basecamp.ListMeta, error) { + result, err := fetchAccountWideTodoGroups(ctx, app, filter, page) + if err != nil { + return nil, basecamp.ListMeta{}, err + } + return result.Groups, result.Meta, nil + }, + countAccountWideTodos, + limit, + ) + return groups, capped, err } // fetchAccountWideTodoGroups calls the aggregate the filter selects. Page 0 diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 3cc46906..4402c45a 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -102,7 +102,7 @@ func TestTodosShowsHelp(t *testing.T) { // project anywhere lists account-wide instead of prompting for one. func TestTodosListWithoutProjectListsAcrossProjects(t *testing.T) { app, transport := setupRecordingTestApp(t, - accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(todosAccountWideDefaultLimit))) + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(accountWideDefaultLimit))) err := executeRecordingCommand(NewTodosCmd(), app, "list") require.NoError(t, err) @@ -3126,7 +3126,7 @@ func TestTodosListWithProjectStaysProjectScoped(t *testing.T) { func TestTodosListAllProjectsOverridesConfiguredProject(t *testing.T) { app, transport := setupRecordingTestApp(t, - accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(todosAccountWideDefaultLimit))) + accountWideTodosRoute("/99999/todos/open.json", todosGroupsBody(accountWideDefaultLimit))) app.Config.ProjectID = "123" err := executeRecordingCommand(NewTodosCmd(), app, "list", "--all-projects") @@ -3148,7 +3148,7 @@ func TestTodosListAccountWideSelectsEndpointPerFilter(t *testing.T) { assertEndpoint := func(path string, args ...string) { t.Helper() - app, transport := setupRecordingTestApp(t, accountWideTodosRoute(path, todosGroupsBody(todosAccountWideDefaultLimit))) + app, transport := setupRecordingTestApp(t, accountWideTodosRoute(path, todosGroupsBody(accountWideDefaultLimit))) require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, append([]string{"list"}, args...)...)) assert.Equal(t, path, transport.last(t).Path) } @@ -3340,7 +3340,24 @@ func TestTodosListAccountWideOverdueIsUnpaginated(t *testing.T) { app, _ := setupRecordingTestApp(t) requireTodosListUsageError(t, app, "--page is not supported with --overdue", "--overdue", "--page", "2") - requireTodosListUsageError(t, app, "--all is not supported with --overdue", "--overdue", "--all") +} + +// --all is a different question from --page. The overdue listing caps at 100 by +// default, so refusing --all too would leave todo 101 unreachable. The endpoint +// is unpaginated, so honoring it costs nothing: the array is already in hand. +func TestTodosListAccountWideOverdueAcceptsAll(t *testing.T) { + body := `[{"id":1,"title":"Bravo","due_on":"2020-03-01"}, + {"id":2,"title":"Alpha","due_on":"2020-01-01"}]` + app, transport, buf := setupAccountWideTodosApp(t, output.FormatJSON, + accountWideTodosRoute("/99999/todos/overdue.json", body)) + + require.NoError(t, executeRecordingCommand(NewTodosCmd(), app, "list", "--overdue", "--all")) + + envelope := decodeTodosEnvelope(t, buf) + assert.Equal(t, "2 overdue todos across all projects", envelope["summary"]) + assert.Empty(t, envelope["notice"], "nothing was withheld, so there is nothing to warn about") + assert.Len(t, transport.queriesFor("/99999/todos/overdue.json"), 1, + "--all costs no extra request on an unpaginated endpoint") } func TestTodosListAccountWideOverdueSortsBeforeTruncating(t *testing.T) { diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 0b146de0..d3cf3008 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -105,6 +105,8 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, ``` 6. **Project scope is mandatory for most commands** — via `--in ` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings ` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the eight list commands covered in item 7. 7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, `boost list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. + Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected), `--page N` for exactly one page, or `--all` for everything. + `boost list --all-projects` is the exception: it returns only the first page by default, because that feed is slow server-side (a single page measured ~93s on a large account). Its `--page`/`--limit`/`--all` work normally, but expect them to be slow. ### Output Modes @@ -720,6 +722,10 @@ basecamp comments update "Updated" --in ```bash basecamp files list --in --json # List all (folders, files, docs) basecamp files list --vault --in # List folder contents +basecamp files list --all-projects --json # Across every project (first 100) +basecamp files list --all-projects --limit 500 # Walk pages until 500 collected +basecamp files list --all-projects --page 2 # Exactly page 2 +basecamp files list --all-projects --all # Every page (slow on big accounts) basecamp files show --in # Show item (auto-detects type) basecamp files download --in # Download file basecamp files download --out ./dir # Download to specific dir From 0558501a9362737156cd14c63775cf69b2195871 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 29 Jul 2026 14:57:42 -0700 Subject: [PATCH 2/9] Record the measured cause of the boost feed's latency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contract doc carried a hypothesis; production tracing settles it. Server timings for one page=40 request, 44,630ms total: Boost Load 22,097.6ms the paginated SELECT ~60 preloads <2.3ms each Boost Count 22,265.7ms GearedPagination's total-count query Completed 200 OK in 44630ms (Views: 244.7ms | ActiveRecord: 44370.9ms) 99.5% database time, in two statements of near-identical cost. That corrects the record twice over: the N+1 hypothesis is dead — the controller's preloads work, and 60 of 64 queries are trivial — and ordering the derived table, which the doc named as the suspect, is only half the story. The count pays the same materialization again for pagination metadata, so fixing the ORDER BY alone would leave roughly half the latency behind. The 93s figure was also wrong: that was the client giving up after three 30s attempts. A page costs ~44s, and page 40 costs the same as page 1 — the flat cost curve the materialization predicts. Since ~44s exceeds the SDK's 30s client timeout, boost list --all-projects does not merely run slow, it cannot complete. Filed as basecamp/bc3#12458, tracked alongside the files.json 500 (basecamp/bc3#12459) in basecamp-cli#589. --- ACCOUNT-WIDE-LISTINGS.md | 47 ++++++++++++++++++++++++++++++---------- skills/basecamp/SKILL.md | 2 +- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index 3b5e5d63..ac991afe 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -270,8 +270,10 @@ behavior change and belongs in the release notes. returns **exactly the first page** when no pagination flag is given. The reason is measured, not stylistic: `/boosts.json?page=1` — one page, no -crawling — took **93s** against a large production account. A default that -walked toward 100 items would multiply an already unacceptable wait. +crawling — takes **~44s** against a large production account. (An earlier +figure of 93s was the client giving up: three 30s attempts plus backoff.) A +default that walked toward 100 items would multiply an already unacceptable +wait. **The exemption covers the default only.** Once a flag is passed, `boost list` behaves like every other bounded listing. The four modes are exhaustive: @@ -303,16 +305,37 @@ exists. When the response is the whole listing there is no notice at all; there are no additional pages to be slow. -**The slowness is server-side and is not fixed here.** The CLI cannot make this -feed fast; the flags make its data reachable. `Everything::BoostsController#index` -already preloads the booster and boostable associations and uses `UNION ALL` -deliberately, so the obvious N+1 hypothesis is wrong. The remaining suspect — -**unconfirmed** — is that it wraps that union in -`Boost.from("(...) AS boosts").order(created_at: :desc, id: :desc)`; ordering a -derived table cannot use an index, which would force MySQL to materialize and -sort every accessible boost before `OFFSET`/`LIMIT` applies, making page 1 cost -the same as page N. That needs an `EXPLAIN` to confirm and is tracked -server-side, not here. +**The slowness is server-side and is not fixed here** — and as of this writing +it is bad enough that the account-wide feed does not work at all. Measured +against a large production account, every page takes ~44s, which exceeds the +SDK's 30s client timeout: the request is retried three times and then fails. +`boost list --all-projects` is therefore held out of the release rather than +shipped broken. Tracked in **basecamp/bc3#12458** and **basecamp-cli#589**. + +The cause is measured, not guessed. Server-side timings for one `page=40` +request (44,630ms total): + +``` +Boost Load 22,097.6ms the paginated SELECT +~60 preloads <2.3ms each +Boost Count 22,265.7ms GearedPagination's total-count query +Completed 200 OK in 44630ms (Views: 244.7ms | ActiveRecord: 44370.9ms, 64 queries) +``` + +99.5% of the request is database time, in two statements of almost exactly equal +cost. Three things follow, and they are worth recording because two of them +contradict what this document previously assumed: + +- **It is not an N+1.** `Everything::BoostsController#index` preloads booster and + boostable thoroughly and the log confirms it works — 60 of 64 queries are + trivial. That hypothesis is dead. +- **Ordering the derived table is only half of it.** The controller wraps its + `UNION ALL` in `Boost.from("(...) AS boosts").order(created_at: :desc, id: + :desc)`; ordering a derived table cannot use an index, so MySQL materializes + and sorts every accessible boost before `OFFSET`/`LIMIT` applies. That + explains `Boost Load`, and it explains why page 1 costs the same as page 40. +- **The count pays the same cost again**, purely for pagination metadata. Fixing + only the `ORDER BY` would leave roughly half the latency in place. #### The `files list` filter exception diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index d3cf3008..95c066e8 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -106,7 +106,7 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, 6. **Project scope is mandatory for most commands** — via `--in ` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings ` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the eight list commands covered in item 7. 7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, `boost list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected), `--page N` for exactly one page, or `--all` for everything. - `boost list --all-projects` is the exception: it returns only the first page by default, because that feed is slow server-side (a single page measured ~93s on a large account). Its `--page`/`--limit`/`--all` work normally, but expect them to be slow. + `boost list --all-projects` is the exception: it returns only the first page by default. That feed is slow server-side — ~44s per page on a large account, which exceeds the client timeout — so on large accounts it currently fails outright (basecamp-cli#589). Item-scoped `boost list ` is unaffected. ### Output Modes From e13bb1436db4c85e0dbafe0ae5cdc3e0ce8df723 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 29 Jul 2026 16:42:40 -0700 Subject: [PATCH 3/9] Stop reporting a listing that ends at the cap as truncated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit accountWideCollect checked the cap before exhaustion, so a listing whose total equalled the limit came back capped=true and the callers turned that into "more may exist" about a listing with nothing left in it. Test the known total first, and let the trim decide: holding every item still counts as capped when the caller is about to cut down to the cap. The exhaustion bound also read TotalCount from the current page while the helper documents the first page's as authoritative. A later page omitting X-Total-Count switched the bound off mid-walk and kept requesting past the declared end of the listing. messages and comments were discarding capped entirely, which made truncation silent on exactly the feeds that withhold a total — the case the notice exists for. messages also had a local `capped` meaning "this invocation applies a cap", which is a different question from "the walk stopped short"; it is now `bounded`, and the sorted path sets capped when its own trim removes anything. --- internal/commands/accountwide.go | 19 ++++++++-- internal/commands/accountwide_test.go | 50 +++++++++++++++++++++++++++ internal/commands/comment.go | 17 +++++---- internal/commands/messages.go | 35 +++++++++++++------ 4 files changed, 102 insertions(+), 19 deletions(-) diff --git a/internal/commands/accountwide.go b/internal/commands/accountwide.go index bceb7e87..7518b8f8 100644 --- a/internal/commands/accountwide.go +++ b/internal/commands/accountwide.go @@ -121,12 +121,25 @@ func accountWideCollect[T any]( } collected = n + // Exhaustion is tested before the cap, and against the first page's + // total rather than this page's. + // + // Order matters: a listing that ends exactly at the cap is complete, + // not truncated, and checking the cap first would report "more may + // exist" about a listing with nothing left in it. + // + // The total comes from meta because the first page's is the one this + // helper documents as authoritative. Reading it from pageMeta let a + // later page that omits X-Total-Count silently switch the bound off + // and walk past the declared end of the listing. + if meta.TotalCount > 0 && len(items) >= meta.TotalCount { + // Everything the listing has is in hand. The caller still trims to + // the cap, so it is only complete if the trim drops nothing. + return items, collected > limit, meta, nil + } if collected >= limit { return items, true, meta, nil } - if pageMeta.TotalCount > 0 && len(items) >= pageMeta.TotalCount { - return items, false, meta, nil - } } } diff --git a/internal/commands/accountwide_test.go b/internal/commands/accountwide_test.go index 7b6041bb..42f4d76e 100644 --- a/internal/commands/accountwide_test.go +++ b/internal/commands/accountwide_test.go @@ -321,3 +321,53 @@ func TestAccountWideCollectPropagatesFetchError(t *testing.T) { assert.Nil(t, items) assert.False(t, capped) } + +// A listing that ends exactly at the cap is complete, not truncated. Checking +// the cap before exhaustion reported capped=true here, and the caller turned +// that into "more may exist" about a listing with nothing left in it. +func TestAccountWideCollectExactCapIsNotCapped(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2, 3, 4, 5}}, + metas: []basecamp.ListMeta{{TotalCount: 5}}, + } + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 5) + + require.NoError(t, err) + assert.Len(t, items, 5) + assert.False(t, capped, "TotalCount == limit means the cap and the end of the listing coincide") + assert.Equal(t, 1, pager.requests) +} + +// Exhaustion still has to lose to the trim: holding the whole listing does not +// make a result complete when the caller is about to cut it down to the cap. +func TestAccountWideCollectExhaustedButOvershootingIsCapped(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2, 3, 4, 5}}, + metas: []basecamp.ListMeta{{TotalCount: 5}}, + } + + _, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 3) + + require.NoError(t, err) + assert.True(t, capped, "the caller's trim to 3 drops two items that exist") +} + +// X-Total-Count is the account-wide total on every page that carries it, but +// not every page carries it. The bound comes from page 1 so that a later page +// omitting the header cannot switch it off mid-walk. +func TestAccountWideCollectBoundsOnFirstPageTotalOnly(t *testing.T) { + pager := &collectPager[int]{ + pages: [][]int{{1, 2}, {3, 4}, {5, 6}, {7, 8}}, + metas: []basecamp.ListMeta{{TotalCount: 4}, {}, {}, {}}, + } + + items, capped, meta, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + assert.Equal(t, []int{1, 2, 3, 4}, items, "page 1 declared four items; the walk stops there") + assert.False(t, capped) + assert.Equal(t, 4, meta.TotalCount) + assert.Equal(t, 2, pager.requests, + "pages 3 and 4 omit the header; reading the total from them would walk past the declared end") +} diff --git a/internal/commands/comment.go b/internal/commands/comment.go index bb32832c..05ec3656 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -224,6 +224,7 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page var ( comments []basecamp.Recording meta basecamp.ListMeta + capped bool ) switch { case all || page > 0: @@ -245,13 +246,17 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page effectiveLimit = limit } var err error - comments, meta, err = fetchAccountWideComments(cmd, app, effectiveLimit) + comments, capped, meta, err = fetchAccountWideComments(cmd, app, effectiveLimit) if err != nil { return err } } - respOpts := append(accountWideRespOpts(len(comments), "comment", "comments", meta, limit > 0), + respOpts := accountWideRespOpts(len(comments), "comment", "comments", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(comments), "comments"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) + } + respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(comments)), output.WithBreadcrumbs( output.Breadcrumb{ @@ -274,8 +279,8 @@ func runCommentsListAccountWide(cmd *cobra.Command, app *appctx.App, limit, page // Asking for page 0 would follow the Link header to the end of the account // before truncating — correct, but it downloads every comment to keep 100. // Each iteration adds at least one comment, so the walk always terminates. -func fetchAccountWideComments(cmd *cobra.Command, app *appctx.App, limit int) ([]basecamp.Recording, basecamp.ListMeta, error) { - comments, _, meta, err := accountWideCollect( +func fetchAccountWideComments(cmd *cobra.Command, app *appctx.App, limit int) ([]basecamp.Recording, bool, basecamp.ListMeta, error) { + comments, capped, meta, err := accountWideCollect( func(page int32) ([]basecamp.Recording, basecamp.ListMeta, error) { result, err := app.Account().Everything().Comments(cmd.Context(), page) if err != nil { @@ -287,13 +292,13 @@ func fetchAccountWideComments(cmd *cobra.Command, app *appctx.App, limit int) ([ limit, ) if err != nil { - return nil, basecamp.ListMeta{}, err + return nil, false, basecamp.ListMeta{}, err } // The walk stops at a page boundary, so trim to the exact cap. if len(comments) > limit { comments = comments[:limit] } - return comments, meta, nil + return comments, capped, meta, nil } func newCommentsShowCmd() *cobra.Command { diff --git a/internal/commands/messages.go b/internal/commands/messages.go index d77d36e8..82014f67 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -224,13 +224,17 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar "use --all to follow every page") } - capped := !all && page == 0 + // bounded reports that this invocation applies a cap at all; capped + // reports that the walk actually stopped short of the listing. They are + // different questions — --all is unbounded, and a bounded walk that + // reaches the end of a short listing is not truncated. + bounded := !all && page == 0 wanted := limit if wanted == 0 { wanted = accountWideDefaultLimit } - recordings, meta, err := messagesAccountWideFetch(cmd.Context(), app, wanted, page, all, sortField != "") + recordings, capped, meta, err := messagesAccountWideFetch(cmd.Context(), app, wanted, page, all, sortField != "") if err != nil { return err } @@ -240,11 +244,17 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar if sortField != "" { sortMessagesAccountWide(recordings, sortField, reverse) } - if capped && len(recordings) > wanted { + if bounded && len(recordings) > wanted { + // A sorted listing arrives whole and is capped here, so the trim is + // itself the truncation the notice has to report. + capped = true recordings = recordings[:wanted] } respOpts := accountWideRespOpts(len(recordings), "message", "messages", meta, limit > 0) + if notice := accountWideCapNotice(capped, meta, len(recordings), "messages"); notice != "" { + respOpts = append(respOpts, output.WithNotice(notice)) + } respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideRecordings(recordings))) respOpts = append(respOpts, output.WithBreadcrumbs(messagesAccountWideBreadcrumbs()...)) @@ -259,7 +269,7 @@ func runMessagesListAccountWide(cmd *cobra.Command, app *appctx.App, messageBoar // rather than crawling the whole account and discarding most of it. A sorted // listing cannot do that — the cap applies after the sort, so every page has to // be in hand first. -func messagesAccountWideFetch(ctx context.Context, app *appctx.App, wanted, page int, all, sorted bool) ([]basecamp.Recording, basecamp.ListMeta, error) { +func messagesAccountWideFetch(ctx context.Context, app *appctx.App, wanted, page int, all, sorted bool) ([]basecamp.Recording, bool, basecamp.ListMeta, error) { everything := app.Account().Everything() fetch := func(p int32) ([]basecamp.Recording, basecamp.ListMeta, error) { @@ -270,22 +280,27 @@ func messagesAccountWideFetch(ctx context.Context, app *appctx.App, wanted, page return result.Recordings, result.Meta, nil } + // An explicit page or --all is exactly what was asked for, and a sorted + // listing needs every page before the cap can apply (I4). Neither is a + // bounded walk, so neither can report one stopping short. if page > 0 || all { sdkPage, err := accountWidePage(page, all) if err != nil { - return nil, basecamp.ListMeta{}, err + return nil, false, basecamp.ListMeta{}, err } - return fetch(sdkPage) + recordings, meta, err := fetch(sdkPage) + return recordings, false, meta, err } if sorted { - return fetch(0) + recordings, meta, err := fetch(0) + return recordings, false, meta, err } - recordings, _, meta, err := accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], wanted) + recordings, capped, meta, err := accountWideCollect(fetch, accountWideFlatCount[basecamp.Recording], wanted) if err != nil { - return nil, basecamp.ListMeta{}, err + return nil, false, basecamp.ListMeta{}, err } - return recordings, meta, nil + return recordings, capped, meta, nil } func messagesAccountWideBreadcrumbs() []output.Breadcrumb { From 2782cfa5a9b9f2a26578aef36f69aacad78094aa Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 29 Jul 2026 16:42:40 -0700 Subject: [PATCH 4/9] Separate what the boost tracing measured from what it infers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The doc claimed the derived-table materialization as established. The tracing established the shape — two ~22s statements, 99.5% db time, N+1 disproved — but not the query plan behind it, and bc3#12458 correctly asks for an EXPLAIN rather than asserting one. Say the same thing here. --- ACCOUNT-WIDE-LISTINGS.md | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index ac991afe..3a0072d9 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -323,19 +323,22 @@ Completed 200 OK in 44630ms (Views: 244.7ms | ActiveRecord: 44370.9ms, 64 querie ``` 99.5% of the request is database time, in two statements of almost exactly equal -cost. Three things follow, and they are worth recording because two of them -contradict what this document previously assumed: - -- **It is not an N+1.** `Everything::BoostsController#index` preloads booster and - boostable thoroughly and the log confirms it works — 60 of 64 queries are - trivial. That hypothesis is dead. -- **Ordering the derived table is only half of it.** The controller wraps its - `UNION ALL` in `Boost.from("(...) AS boosts").order(created_at: :desc, id: - :desc)`; ordering a derived table cannot use an index, so MySQL materializes - and sorts every accessible boost before `OFFSET`/`LIMIT` applies. That - explains `Boost Load`, and it explains why page 1 costs the same as page 40. -- **The count pays the same cost again**, purely for pagination metadata. Fixing - only the `ORDER BY` would leave roughly half the latency in place. +cost. Separating what is measured from what is inferred: + +**Measured.** It is not an N+1 — `Everything::BoostsController#index` preloads +booster and boostable thoroughly and the log confirms that works, with 60 of 64 +queries trivial and views at 244.7ms. That hypothesis is dead. The cost is two +statements: the paginated `Boost Load` and GearedPagination's `Boost Count`, at +roughly 22s each. Page 1 costs the same as page 40. Whatever the fix is, it has +to address **both** statements: halving one leaves ~22s behind. + +**Inferred, pending `EXPLAIN`.** The controller wraps its `UNION ALL` in +`Boost.from("(...) AS boosts").order(created_at: :desc, id: :desc)`. Ordering a +derived table cannot use an index, which would force MySQL to materialize and +sort every accessible boost before `OFFSET`/`LIMIT` applies — consistent with +both the flat cost curve and the count costing as much as the fetch. That is a +coherent explanation for the numbers, not a verified query plan, and +basecamp/bc3#12458 asks for the `EXPLAIN` rather than asserting it. #### The `files list` filter exception From 23a794a75bb278fd37be60df3dbaa1207e34686e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 29 Jul 2026 19:13:00 -0700 Subject: [PATCH 5/9] Drop the account-wide boost listing rather than page around it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /boosts.json aggregate is an easter egg: unlinked from the Basecamp web UI, and served by a query that spends ~44s in MySQL on every page regardless of depth (basecamp/bc3#12458). It is being withdrawn server-side, so the CLI stops calling it instead of building a contract on top of that. boost list now requires an item ID. It loses --all-projects along with the --limit/--page/--all this branch had given it: an item's boosts arrive in one unpaginated response, and the SDK documents BoostListOptions.Page as not honoring a page number, so there was never anything for those flags to address once the aggregate went away. rejectScopedPaginationFlags is no longer needed here — flags that do not exist cannot be silently ignored. Everything().Boosts() stays in the SDK. Whether it should is the SDK's call; this repo just stops reaching for it, and API-COVERAGE.md now records 16 of the 17 aggregates as surfaced with the reason for the sixteenth. Seven commands list account-wide, not eight. On the surface baseline: --limit/--page/--all on boost were added and removed inside this unmerged branch, so they never shipped and recording them in .surface-breaking would claim a break that never happened. They are rewound out of the baseline instead. The two changes that did ship in v0.8.0-rc.1 -- `--all-projects`, and the argument going from optional to required -- are acknowledged as real removals. Net surface change against rc.1 is exactly those two. --- .surface | 12 +- .surface-breaking | 7 +- ACCOUNT-WIDE-LISTINGS.md | 133 ++++--------- API-COVERAGE.md | 12 +- internal/commands/accountwide_test.go | 4 +- internal/commands/boost.go | 241 ++--------------------- internal/commands/boost_test.go | 264 +++++--------------------- skills/basecamp/SKILL.md | 7 +- 8 files changed, 123 insertions(+), 557 deletions(-) diff --git a/.surface b/.surface index 5a39e551..c0273e93 100644 --- a/.surface +++ b/.surface @@ -19,12 +19,12 @@ ARG basecamp bonfire split 00 ARG basecamp boost create 00 ARG basecamp boost create 01 ARG basecamp boost delete 00 -ARG basecamp boost list 00 [id|url] +ARG basecamp boost list 00 ARG basecamp boost show 00 ARG basecamp boosts create 00 ARG basecamp boosts create 01 ARG basecamp boosts delete 00 -ARG basecamp boosts list 00 [id|url] +ARG basecamp boosts list 00 ARG basecamp boosts show 00 ARG basecamp campfire delete 00 ARG basecamp campfire line 00 @@ -2061,8 +2061,6 @@ FLAG basecamp boost delete --todolist type=string FLAG basecamp boost delete --verbose type=count FLAG basecamp boost list --account type=string FLAG basecamp boost list --agent type=bool -FLAG basecamp boost list --all type=bool -FLAG basecamp boost list --all-projects type=bool FLAG basecamp boost list --cache-dir type=string FLAG basecamp boost list --count type=bool FLAG basecamp boost list --event type=string @@ -2072,12 +2070,10 @@ FLAG basecamp boost list --ids-only type=bool FLAG basecamp boost list --in type=string FLAG basecamp boost list --jq type=string FLAG basecamp boost list --json type=bool -FLAG basecamp boost list --limit type=int FLAG basecamp boost list --markdown type=bool FLAG basecamp boost list --md type=bool FLAG basecamp boost list --no-hints type=bool FLAG basecamp boost list --no-stats type=bool -FLAG basecamp boost list --page type=int FLAG basecamp boost list --profile type=string FLAG basecamp boost list --project type=string FLAG basecamp boost list --quiet type=bool @@ -2172,8 +2168,6 @@ FLAG basecamp boosts delete --todolist type=string FLAG basecamp boosts delete --verbose type=count FLAG basecamp boosts list --account type=string FLAG basecamp boosts list --agent type=bool -FLAG basecamp boosts list --all type=bool -FLAG basecamp boosts list --all-projects type=bool FLAG basecamp boosts list --cache-dir type=string FLAG basecamp boosts list --count type=bool FLAG basecamp boosts list --event type=string @@ -2183,12 +2177,10 @@ FLAG basecamp boosts list --ids-only type=bool FLAG basecamp boosts list --in type=string FLAG basecamp boosts list --jq type=string FLAG basecamp boosts list --json type=bool -FLAG basecamp boosts list --limit type=int FLAG basecamp boosts list --markdown type=bool FLAG basecamp boosts list --md type=bool FLAG basecamp boosts list --no-hints type=bool FLAG basecamp boosts list --no-stats type=bool -FLAG basecamp boosts list --page type=int FLAG basecamp boosts list --profile type=string FLAG basecamp boosts list --project type=string FLAG basecamp boosts list --quiet type=bool diff --git a/.surface-breaking b/.surface-breaking index 8539d556..4da82f27 100644 --- a/.surface-breaking +++ b/.surface-breaking @@ -1,8 +1,7 @@ - ARG basecamp assign 00 ARG basecamp assign 00 -ARG basecamp boost list 00 -ARG basecamp boosts list 00 +ARG basecamp boost list 00 [id|url] +ARG basecamp boosts list 00 [id|url] ARG basecamp card 00 ARG basecamp card 01 [body] ARG basecamp card move 00 <id|url> @@ -134,6 +133,8 @@ CMD basecamp uploads vault list CMD basecamp uploads vaults CMD basecamp uploads vaults create CMD basecamp uploads vaults list +FLAG basecamp boost list --all-projects type=bool +FLAG basecamp boosts list --all-projects type=bool FLAG basecamp campfire --account type=string FLAG basecamp campfire --agent type=bool FLAG basecamp campfire --cache-dir type=string diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index 3a0072d9..9f6ea7b0 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -22,7 +22,6 @@ All 17 methods, and the invocation that reaches each. | `comments list --all-projects` | `Comments` | `.Recordings` | yes | | `checkins answers --all-projects` | `Checkins` | `.Recordings` | yes | | `forwards list --all-projects` | `Forwards` | `.Recordings` | yes | -| `boost list --all-projects` | `Boosts` | `.Boosts` | yes | | `files list --all-projects` | `Files` | `.Files` | yes | | `todos list --all-projects` | `OpenTodos` | `.Groups` | yes | | `todos list --all-projects --status completed` | `CompletedTodos` | `.Groups` | yes | @@ -71,10 +70,10 @@ ambient config. #### Per-item dispatch -The per-item groups (`comments`, `boost`, `checkins answers`) list the children -of one recording, so a project alone cannot produce a listing — only an item ID -can. Their dispatch is therefore its own truth table, and it overrides the -general precedence above: +The per-item groups (`comments`, `checkins answers`) list the children of one +recording, so a project alone cannot produce a listing — only an item ID can. +Their dispatch is therefore its own truth table, and it overrides the general +precedence above: | ID/URL | Explicit project | `--all-projects` | Result | |---|---|---|---| @@ -86,8 +85,13 @@ general precedence above: | absent | absent | present | account-wide, intent pinned | The fourth row is the deliberate exception to "a configured project selects -project scope": for these three commands a configured project is not a scope -that can be honored, so it is ignored rather than turned into an error. +project scope": for these two commands a configured project is not a scope that +can be honored, so it is ignored rather than turned into an error. + +`boost list` is per-item in the same way but has **no account-wide row at all** +— the aggregate behind it is being withdrawn server-side. Every absent-ID case +asks for an ID, and it carries no `--all-projects`. See "`boost list` — +withdrawn" under I5. ### I3 — No flag is silently ignored @@ -112,8 +116,9 @@ sake. Pagination is the one place where "reuse only" turned out to be wrong. A command with no `--limit`/`--page`/`--all` has no way to recover from a server error -mid-crawl and no way to reach past a bounded default, so `boost list` and bare -`files list` gained all three — see I5. +mid-crawl and no way to reach past a bounded default, so bare `files list` +gained all three — see I5. (`boost list` gained them too, then lost them along +with the account-wide boost feed itself — see below.) #### Endpoint selectors @@ -123,7 +128,7 @@ flags added by this work — anything not listed here is reuse: | Command | New flag | Selects | Scope | |---|---|---|---| -| all eight | `--all-projects` | account-wide | — | +| all seven | `--all-projects` | account-wide | — | | `todos list` | `--unassigned` | `UnassignedTodos` | account-wide only | | `todos list` | `--no-due-date` | `NoDueDateTodos` | account-wide only | | `cards list` | `--status` (only `completed`) | `CompletedCards` | account-wide only | @@ -133,7 +138,6 @@ flags added by this work — anything not listed here is reuse: | `cards list` | `--overdue` | `OverdueCards` | account-wide only | | `files list` | `--kind`, `--person` | filters on `Files` | account-wide only — see I5 | | `files list` | `--limit`/`-n`, `--page`, `--all` | pagination on `Files` | account-wide only — see I5 | -| `boost list` | `--limit`/`-n`, `--page`, `--all` | pagination on `Boosts` | account-wide only — see I5 | **Account-wide only** means exactly what it means for the `files list` filters: the project-scoped path has no equivalent, so passing one with a project in @@ -150,7 +154,7 @@ pair. **Sorting flags are reused where they exist and never added.** Verified current state: among these commands only `messages list`, `todos list`, and `cards list` expose `--sort`/`--reverse`. `comments list`, `checkins answers`, -`forwards list`, `boost list`, and bare `files list` expose neither and gain +`forwards list`, and bare `files list` expose neither and gain neither — an unsorted account-wide feed is the honest result, not a gap to paper over. @@ -193,8 +197,8 @@ to it": 2. **The overdue endpoints** (`todos list --overdue`, `cards list --overdue`) are unpaginated — one request returns everything — so `--limit` trims locally. There is no walk to bound. -3. **`boost list`** keeps a first-page default rather than the cap. Its flags - still walk normally; only the default differs. See below. +3. ~~**`boost list`** keeps a first-page default.~~ Withdrawn — there is no + account-wide boost listing at all now. See below. #### Per-command defaults @@ -218,7 +222,6 @@ rows that differ for stated reasons. `--all` is how you ask for the account. | `files list` | cap 100 | **changed** from "all pages" | | `todos list --overdue` | cap 100 | unpaginated endpoint; accepts `--all`, rejects `--page` | | `cards list --overdue` | cap 100 | **changed** from uncapped; same rules as above | -| `boost list` | **first page only** | the explicit exception — see below | Project-scoped defaults are untouched throughout. @@ -249,10 +252,10 @@ behavior change and belongs in the release notes. - **Project-scoped `files list`** rejects `--limit`/`--page`/`--all`. That path passes `nil` to `Vaults().List`, `Uploads().List`, and `Documents().List` — three unpaginated calls with nowhere to put a page. - - **Item-scoped `boost list <id>` and `--event`** reject all three. The SDK - documents `BoostListOptions.Page` as not honoring the page number at all: - setting `Page=2` does not fetch page 2. Rejecting is honest; implementing - would lie. + - **`boost list`** carries none of the three at all. The SDK documents + `BoostListOptions.Page` as not honoring the page number — setting `Page=2` + does not fetch page 2 — and the account-wide feed they were added for is + gone. - **`--limit N` on grouped todo/card responses counts inner todos/cards, not outer project groups.** Truncating groups would silently drop whole projects. - **`Meta.TotalCount` counts groups, not items,** for the grouped responses. @@ -264,81 +267,28 @@ behavior change and belongs in the release notes. crawl they did not ask for, and that is the same no-silent-flags defect I3 names. `--all` is the spelling for every page. -#### `boost list` — the default exception, and its server-side cause +#### `boost list` — withdrawn -`boost list` is the one account-wide listing exempt from the cap-100 row. It -returns **exactly the first page** when no pagination flag is given. +There is **no account-wide boost listing.** `boost list` requires an item ID. -The reason is measured, not stylistic: `/boosts.json?page=1` — one page, no -crawling — takes **~44s** against a large production account. (An earlier -figure of 93s was the client giving up: three 30s attempts plus backoff.) A -default that walked toward 100 items would multiply an already unacceptable -wait. +The `/boosts.json` aggregate this section used to document was an easter egg: +unlinked from the Basecamp web UI, and served by a query that spent ~44s in +MySQL on every page regardless of depth (basecamp/bc3#12458 has the numbers). +Rather than build a CLI contract on top of pathological database performance, +the endpoint is being removed server-side, so the CLI stops calling it. -**The exemption covers the default only.** Once a flag is passed, `boost list` -behaves like every other bounded listing. The four modes are exhaustive: +What that means here: -| Invocation | Fetch behavior | -|---|---| -| no pagination flag | exactly page 1 — one request, no walk | -| `--page N` | exactly page N — one request | -| `--limit N` | bounded walk over positive page numbers, then exact trim | -| `--all` | full traversal (page 0 / omit `page=`) | - -`--limit` walks **positive** pages (1, 2, 3, …). It must never fall back to the -page-0 full-crawl form, which is the behavior this whole section exists to -remove. - -**Notices are mode-specific.** "Showing first page" is false for `--page 2`, so -a single fixed string would state something untrue. Every mode keeps the -account-wide total: now that `--page` and `--all` make the rest of the feed -genuinely reachable — just slow — suppressing the total would understate what -exists. - -| Mode | Notice | -|---|---| -| default | `Showing first page, N of M; additional pages may be slow on large accounts` | -| `--page P` | `Showing page P, N of M; additional pages may be slow on large accounts` | -| `--limit` | the standard truncation notice used by the other bounded listings | -| `--all` | none — the traversal is complete | - -When the response is the whole listing there is no notice at all; there are no -additional pages to be slow. - -**The slowness is server-side and is not fixed here** — and as of this writing -it is bad enough that the account-wide feed does not work at all. Measured -against a large production account, every page takes ~44s, which exceeds the -SDK's 30s client timeout: the request is retried three times and then fails. -`boost list --all-projects` is therefore held out of the release rather than -shipped broken. Tracked in **basecamp/bc3#12458** and **basecamp-cli#589**. - -The cause is measured, not guessed. Server-side timings for one `page=40` -request (44,630ms total): - -``` -Boost Load 22,097.6ms the paginated SELECT -~60 preloads <2.3ms each -Boost Count 22,265.7ms GearedPagination's total-count query -Completed 200 OK in 44630ms (Views: 244.7ms | ActiveRecord: 44370.9ms, 64 queries) -``` - -99.5% of the request is database time, in two statements of almost exactly equal -cost. Separating what is measured from what is inferred: - -**Measured.** It is not an N+1 — `Everything::BoostsController#index` preloads -booster and boostable thoroughly and the log confirms that works, with 60 of 64 -queries trivial and views at 244.7ms. That hypothesis is dead. The cost is two -statements: the paginated `Boost Load` and GearedPagination's `Boost Count`, at -roughly 22s each. Page 1 costs the same as page 40. Whatever the fix is, it has -to address **both** statements: halving one leaves ~22s behind. - -**Inferred, pending `EXPLAIN`.** The controller wraps its `UNION ALL` in -`Boost.from("(...) AS boosts").order(created_at: :desc, id: :desc)`. Ordering a -derived table cannot use an index, which would force MySQL to materialize and -sort every accessible boost before `OFFSET`/`LIMIT` applies — consistent with -both the flat cost curve and the count costing as much as the fetch. That is a -coherent explanation for the numbers, not a verified query plan, and -basecamp/bc3#12458 asks for the `EXPLAIN` rather than asserting it. +- `boost list` takes an item ID, and rejecting a bare invocation is the honest + answer rather than a fallback to something slower. +- It carries no `--all-projects`, `--limit`, `--page`, or `--all`. An item's + boosts arrive in one unpaginated response, and the SDK documents + `BoostListOptions.Page` as not honoring a page number, so there would be + nothing for those flags to address even if the aggregate had survived. +- The SDK still exposes `Everything().Boosts()`. The CLI simply does not call + it; removing it is the SDK's decision to make, not this repo's. + +Seven commands list account-wide, not eight. #### The `files list` filter exception @@ -393,14 +343,13 @@ precedent in `internal/commands/search.go`: - **styled**: flattened rows carrying at least project name, id, and title/subject, plus status and due date where applicable. -Which payloads actually need flattening, so the eight commands do not each +Which payloads actually need flattening, so the seven commands do not each answer this differently: | Payload | Commands | Styled treatment | |---|---|---| | `[]Recording` | messages, comments, checkins answers, forwards | flatten — the generic renderer drops the nested `bucket`, and a project column is exactly what an account-wide row needs | | `[]Todo`, `[]Card` (flat overdue) | todos, cards `--overdue` | flatten — same reason as `[]Recording`; the items come from every project and `bucket` is skipped by name | -| `[]EverythingBoost` | boost | flatten — the boosted `*Recording` is nested | | `[]EverythingFile` | files | flatten — all-pointer superset, too wide to render raw | | `[]BucketTodosGroup`, `[]BucketCardsGroup` | todos, cards | flatten — nested groups render as unreadable cells | diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 4fcf3195..029db0ae 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -23,9 +23,12 @@ Out-of-scope sections are excluded from parity totals and scripts: chatbots (dif **SDK version:** v0.10.0 — adds `EverythingService` (`AccountClient.Everything()`, basecamp/basecamp-sdk#435 and #438), a 17-method account-wide aggregate family covering cross-project messages, comments, checkins, forwards, boosts, files, and -the open/completed/unassigned/overdue/no-due-date todo and card rollups. All 17 -are reached from the CLI — see [Account-wide -aggregates](#account-wide-aggregates). They are not a new command group and add +the open/completed/unassigned/overdue/no-due-date todo and card rollups. **16 of +the 17 are reached from the CLI** — see [Account-wide +aggregates](#account-wide-aggregates). `Everything().Boosts()` is deliberately +not called: the `/boosts.json` aggregate behind it was an unlinked easter egg +served by a query costing ~44s per page (basecamp/bc3#12458), and it is being +withdrawn server-side, so the CLI does not surface it. They are not a new command group and add no endpoints to the tracked totals above: each aggregate is the account-wide variant of a listing the CLI already owned, reached through that group's existing leaf command. The contract is `ACCOUNT-WIDE-LISTINGS.md`. @@ -79,7 +82,6 @@ instead of prompting for a project. | `comments list --all-projects` | `Comments` | `[]Recording` | | `checkins answers --all-projects` | `Checkins` | `[]Recording` | | `forwards list --all-projects` | `Forwards` | `[]Recording` | -| `boost list --all-projects` | `Boosts` | `[]EverythingBoost` | | `files list --all-projects` | `Files` | `[]EverythingFile` | | `todos list --all-projects` | `OpenTodos` | bucket groups | | `todos list --all-projects --status completed` | `CompletedTodos` | bucket groups | @@ -126,7 +128,7 @@ The **Since** column tags each row with the Basecamp version that introduced its | message_types | 5 | `messagetypes` | ✅ | BC4 | - | list, show, create, update, delete. Bucket-scoped (`/buckets/{id}/categories…`); commands are project-scoped via `--in`/`--project` | | campfires | 14 | `chat` | ✅ | BC4 | - | list, messages, post, line show/update/delete. @mentions in content | | comments | 8 | `comment`, `comments` | ✅ | BC4 | - | list, show, thread, create, update. @mentions in content. `show` surfaces `reply_target` + paste-ready `mention` from its single Get (no new calls). `thread` composes Get + parent recording (via type endpoint) + List into a deterministic reply-ready context (no new endpoints) | -| boosts | 6 | `boost`, `react` | ✅ | BC4 | - | list (recording + event), show, create (recording + event), delete | +| boosts | 6 | `boost`, `react` | ✅ | BC4 | - | list (recording + event), show, create (recording + event), delete. No account-wide listing — the `/boosts.json` aggregate is being withdrawn server-side (basecamp/bc3#12458) | | notifications | 2 | `notifications` | ✅ | BC4 | - | list, mark as read (BC5: `bubble_ups`/`scheduled_bubble_ups` sections; `memories` is BC4-only) | | bubble_ups | 1 | `notifications bubbleups` | ✅ | BC5 | - | Dedicated Bubble Ups list (`GET /my/readings/bubble_ups.json`, paginated) plus the `limit_bubble_ups` variant behind `notifications list --limit-bubble-ups` | | **Cards (Kanban)** | diff --git a/internal/commands/accountwide_test.go b/internal/commands/accountwide_test.go index 42f4d76e..4e9f723d 100644 --- a/internal/commands/accountwide_test.go +++ b/internal/commands/accountwide_test.go @@ -21,6 +21,9 @@ import ( // The root --todolist is a global, so it reaches every account-wide listing // whether or not the command has any notion of a todolist. I3 lists it among // the scope-child flags that must be rejected by name rather than dropped. +// +// boost list is absent: it has no account-wide listing to scope, so a bare +// invocation asks for an item ID before any flag rejection is reached. func TestAccountWideListingsRejectRootTodolist(t *testing.T) { cases := []struct { name string @@ -29,7 +32,6 @@ func TestAccountWideListingsRejectRootTodolist(t *testing.T) { }{ {"messages", NewMessagesCmd, []string{"list"}}, {"comments", NewCommentsCmd, []string{"list"}}, - {"boost", NewBoostsCmd, []string{"list"}}, {"forwards", NewForwardsCmd, []string{"list"}}, {"checkins", NewCheckinsCmd, []string{"answers"}}, {"files", NewFilesCmd, []string{"list"}}, diff --git a/internal/commands/boost.go b/internal/commands/boost.go index b13f7f12..aa091608 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -2,14 +2,11 @@ package commands import ( "fmt" - "math" "strconv" "unicode/utf8" "github.com/spf13/cobra" - "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" - "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" ) @@ -51,13 +48,10 @@ Tip: In the TUI, press 'b' on any item to boost interactively.`, func newBoostListCmd(project *string) *cobra.Command { var eventID string - var allProjects bool - var limit, page int - var all bool cmd := &cobra.Command{ - Use: "list [id|url]", - Short: "List boosts on an item, or across every project", + Use: "list <id|url>", + Short: "List boosts on an item", Long: `List boosts on an item. You can pass either an ID or a Basecamp URL: @@ -66,75 +60,32 @@ You can pass either an ID or a Basecamp URL: Use --event to list boosts on a specific event within the item. -Without an ID, boosts from every accessible project are listed, newest -first. That feed is slow on large accounts, so it returns the first page -unless you ask for more: - basecamp boost list # first page - basecamp boost list --page 2 # exactly page 2 - basecamp boost list --limit 250 # walk pages until 250 collected - basecamp boost list --all # every page (slowest) - ---limit/--page/--all apply to that account-wide feed only; an item's own -boosts arrive in a single unpaginated response.`, - Args: cobra.MaximumNArgs(1), +Boosts hang off a single item, so an item ID is required: there is no +account-wide boost listing. An item's boosts arrive in one unpaginated +response, so there is nothing to page through either.`, + Example: ` basecamp boost list 789 --project my-project`, + Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + if len(args) == 0 { + return missingArg(cmd, "<id|url>") + } if err := ensureAccount(cmd, app); err != nil { return err } - recording := "" - if len(args) > 0 { - recording = args[0] - } - return runBoostList(cmd, app, recording, *project, eventID, allProjects, limit, page, all) + return runBoostList(cmd, app, args[0], *project, eventID) }, } cmd.Flags().StringVar(&eventID, "event", "", "Event ID (for event-specific boosts)") - cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List boosts across every project") - cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Account-wide only: maximum boosts to fetch (0 = first page)") - cmd.Flags().IntVar(&page, "page", 0, "Account-wide only: fetch exactly this page") - cmd.Flags().BoolVar(&all, "all", false, "Account-wide only: fetch every page (slow)") return cmd } -func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string, allProjects bool, limit, page int, all bool) error { - // Boosts hang off a single recording, so a project cannot scope this - // listing on its own — only an item ID can. Without one, the account-wide - // feed answers instead, and a configured project is ignored rather than - // turned into an error, since it could never have produced a listing here. - explicitProject := project - if explicitProject == "" { - explicitProject = app.Flags.Project - } - - if recording == "" { - switch { - case explicitProject != "" && allProjects: - return output.ErrUsageHint("Cannot combine --all-projects with --project", - "--all-projects lists boosts from every project; drop --project, or pass an item ID to list one item's boosts") - case explicitProject != "": - return output.ErrUsageHint("Boosts belong to an item, so --project alone cannot list them", - fmt.Sprintf("Pass the item's ID or URL: basecamp boost list <id> --project %s", explicitProject)) - } - return runBoostListAccountWide(cmd, app, eventID, limit, page, all) - } - - if allProjects { - return output.ErrUsageHint("Cannot combine --all-projects with an item ID", - "Drop the ID to list boosts across every project, or drop --all-projects to list that item's boosts") - } - - // An item's boosts come back in one unpaginated response, and the SDK's - // Page option on that feed is documented not to honor the page number at - // all. Accepting these flags here would promise addressability the endpoint - // cannot deliver. - if err := rejectScopedPaginationFlags(cmd, "an item's boosts", - "Drop the item ID to page through boosts across every project."); err != nil { - return err - } - +func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string) error { + // Boosts hang off a single recording, so only an item ID can scope this + // listing — a project cannot, and there is no account-wide equivalent to + // fall back to. recordingID, urlProjectID := extractWithProject(recording) projectID := project @@ -208,168 +159,6 @@ func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, event ) } -// boostListMode names which slice of the account-wide feed was asked for. The -// notice differs per mode: "Showing first page" is simply false for --page 2, -// and a single fixed string would state it anyway. -type boostListMode int - -const ( - boostListFirstPage boostListMode = iota // no pagination flag - boostListPage // --page N - boostListLimit // --limit N - boostListAll // --all -) - -// runBoostListAccountWide lists boosts across every accessible project. -// -// This feed is the one account-wide listing that keeps a first-page default -// rather than the shared cap. /boosts.json?page=1 — a single page, no crawling -// — measured 93s against a large production account, so a default that walked -// toward 100 items would multiply an already unacceptable wait. The flags exist -// for addressability, not because they make it fast: the cost is server-side -// and tracked separately. -// -// The four modes are exhaustive and each maps to exactly one fetch shape. -func runBoostListAccountWide(cmd *cobra.Command, app *appctx.App, eventID string, limit, page int, all bool) error { - if err := rejectAccountWideTodolist(app, "boost"); err != nil { - return err - } - if eventID != "" { - return output.ErrUsageHint("--event names an event inside one item, which the account-wide feed has no equivalent for", - "Pass the item's ID alongside --event, or drop --event to list boosts across every project") - } - if limit < 0 { - return output.ErrUsage("--limit must be zero or positive") - } - if all && limit > 0 { - return output.ErrUsage("--all and --limit are mutually exclusive") - } - if page > 0 && (all || limit > 0) { - return output.ErrUsage("--page cannot be combined with --all or --limit") - } - if cmd.Flags().Changed("page") && page < 1 { - return output.ErrUsageHint( - "--page must be a positive page number", - "Omit --page for the first page, or pass --all to follow every page") - } - if page > math.MaxInt32 { - return output.ErrUsage("--page is out of range") - } - - fetch := func(p int32) ([]basecamp.EverythingBoost, basecamp.ListMeta, error) { - result, err := app.Account().Everything().Boosts(cmd.Context(), p) - if err != nil { - return nil, basecamp.ListMeta{}, convertSDKError(err) - } - return result.Boosts, result.Meta, nil - } - - var ( - boosts []basecamp.EverythingBoost - meta basecamp.ListMeta - mode boostListMode - err error - ) - switch { - case all: - mode = boostListAll - boosts, meta, err = fetch(0) - case limit > 0: - mode = boostListLimit - boosts, _, meta, err = accountWideCollect(fetch, accountWideFlatCount[basecamp.EverythingBoost], limit) - // The walk stops at a page boundary, so trim to the exact cap. - if err == nil && len(boosts) > limit { - boosts = boosts[:limit] - } - case page > 0: - mode = boostListPage - boosts, meta, err = fetch(int32(page)) //nolint:gosec // bounded above by the --page range check - default: - mode = boostListFirstPage - boosts, meta, err = fetch(1) - } - if err != nil { - return err - } - - noun := "boosts" - if len(boosts) == 1 { - noun = "boost" - } - - // The boosted recording is nested, so every consumer but --json and - // --agent reads the flat rows: the generic renderer skips nested maps, - // which would drop the project and the item title that make a boost row - // mean anything. - respOpts := []output.ResponseOption{ - output.WithSummary(fmt.Sprintf("%d %s across all projects", len(boosts), noun)), - } - if notice := boostAccountWideNotice(mode, page, len(boosts), meta.TotalCount); notice != "" { - respOpts = append(respOpts, output.WithNotice(notice)) - } - respOpts = append(respOpts, output.WithDisplayData(flattenAccountWideBoosts(boosts))) - respOpts = append(respOpts, output.WithBreadcrumbs( - output.Breadcrumb{ - Action: "show", - Cmd: "basecamp boost show <boost-id>", - Description: "Show a boost", - }, - )) - - return app.OK(boosts, respOpts...) -} - -// boostAccountWideNotice states which slice of the feed came back, in the -// wording that mode actually earns. Every mode keeps the account-wide total: -// now that --page and --all make the rest reachable, suppressing the total -// would understate what exists. When the response is the whole listing there is -// nothing left to warn about, so there is no notice at all. -func boostAccountWideNotice(mode boostListMode, page, count, total int) string { - if mode == boostListAll || total <= count { - return "" - } - if mode == boostListLimit { - return fmt.Sprintf("Showing %d of %d results (raise or drop --limit for more)", count, total) - } - where := "first page" - if mode == boostListPage { - where = fmt.Sprintf("page %d", page) - } - return fmt.Sprintf("Showing %s, %d of %d; additional pages may be slow on large accounts", where, count, total) -} - -// flattenAccountWideBoosts turns the account-wide feed into flat rows for the -// styled renderer: each boost nests the recording it sits on, which renders as -// an unreadable cell. Machine formats keep the nested payload. Booster and -// Recording are both optional, so every row carries the same keys whether or -// not the feed populated them. -func flattenAccountWideBoosts(boosts []basecamp.EverythingBoost) []map[string]any { - rows := make([]map[string]any, 0, len(boosts)) - for _, boost := range boosts { - booster := "" - if boost.Booster != nil { - booster = boost.Booster.Name - } - project, title, recordingType := "", "", "" - if boost.Recording != nil { - title = boost.Recording.Title - recordingType = simplifyType(boost.Recording.Type) - if boost.Recording.Bucket != nil { - project = boost.Recording.Bucket.Name - } - } - rows = append(rows, map[string]any{ - "id": boost.ID, - "project": project, - "booster": booster, - "content": boost.Content, - "title": title, - "type": recordingType, - }) - } - return rows -} - func newBoostShowCmd(project *string) *cobra.Command { cmd := &cobra.Command{ Use: "show <boost-id|url>", diff --git a/internal/commands/boost_test.go b/internal/commands/boost_test.go index 224211f4..bdeab729 100644 --- a/internal/commands/boost_test.go +++ b/internal/commands/boost_test.go @@ -215,25 +215,7 @@ func TestBoostCreateAcceptsMaxContent(t *testing.T) { assert.Equal(t, "POST", transport.capturedMethod) } -// --- account-wide boost listing --- - -// accountWideBoostsRoute serves the /boosts.json aggregate feed. -func accountWideBoostsRoute() stubRoute { - return stubRoute{ - method: http.MethodGet, - path: "/99999/boosts.json", - status: http.StatusOK, - body: accountWideBoostsBody, - // --limit walks positive pages; the fixture needs an end so the walk - // can stop somewhere other than the cap. - pages: []string{accountWideBoostsBody}, - } -} - -const accountWideBoostsBody = `[{"id":1,"content":"🎉","created_at":"2024-01-01T00:00:00Z", - "booster":{"id":10,"name":"Alice"}, - "recording":{"id":456,"title":"Ship it","type":"Todo", - "bucket":{"id":123,"name":"Test Project","type":"Project"}}}]` +// --- item-scoped boost listing --- // recordingBoostsRoute serves the item-scoped boost listing. func recordingBoostsRoute() stubRoute { @@ -249,7 +231,7 @@ func recordingBoostsRoute() stubRoute { func setupBoostListTest(t *testing.T, buf *bytes.Buffer) (*cobra.Command, *appctx.App, *recordingTransport) { t.Helper() - app, transport := setupRecordingTestApp(t, projectsRoute(), accountWideBoostsRoute(), recordingBoostsRoute()) + app, transport := setupRecordingTestApp(t, projectsRoute(), recordingBoostsRoute()) if buf != nil { app.Output = output.New(output.Options{Format: output.FormatJSON, Writer: buf}) } @@ -266,8 +248,8 @@ func requireBoostUsageError(t *testing.T, err error, want string) { assert.Contains(t, e.Message, want) } -// TestBoostListWithIDStaysItemScoped verifies that passing an ID still lists -// that item's boosts, unchanged by the account-wide path. +// TestBoostListWithIDStaysItemScoped verifies that passing an ID lists that +// item's boosts. func TestBoostListWithIDStaysItemScoped(t *testing.T) { cmd, app, transport := setupBoostListTest(t, nil) @@ -275,219 +257,69 @@ func TestBoostListWithIDStaysItemScoped(t *testing.T) { assert.Equal(t, "/99999/recordings/456/boosts.json", transport.last(t).Path) } -// TestBoostListWithoutIDListsAccountWide verifies that a bare list with no -// project anywhere reaches the account-wide feed instead of prompting. -func TestBoostListWithoutIDListsAccountWide(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) - - require.NoError(t, executeBoostCommand(cmd, app, "list")) - - last := transport.last(t) - assert.Equal(t, "/99999/boosts.json", last.Path) - assert.Equal(t, "page=1", last.Query, "the default is exactly the first page — this feed is slow enough that walking it by default is not viable") -} - -// The four account-wide modes are exhaustive and each maps to exactly one fetch -// shape. Boost keeps a first-page default because /boosts.json?page=1 alone -// measured 93s on a large account — but the default being an exception does not -// make the flags one, and --limit walks here exactly as it does everywhere else. -func TestBoostListAccountWideFourModes(t *testing.T) { - cases := []struct { - name string - args []string - queries []string - }{ - {"no pagination flag is exactly page 1", nil, []string{"page=1"}}, - {"--page N is exactly page N", []string{"--page", "3"}, []string{"page=3"}}, - {"--limit walks positive pages", []string{"--limit", "5"}, []string{"page=1", "page=2"}}, - {"--all is the full traversal", []string{"--all"}, []string{""}}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) - - require.NoError(t, executeBoostCommand(cmd, app, append([]string{"list"}, tc.args...)...)) - assert.Equal(t, tc.queries, transport.queriesFor("/99999/boosts.json")) - }) - } -} - -// "Showing first page" is simply false for --page 2, so the notice is -// mode-specific. Every mode keeps the total: with --page and --all registered -// the rest of the feed is genuinely reachable, just slow, so suppressing it -// would understate what exists. -func TestBoostListAccountWideNoticeIsModeSpecific(t *testing.T) { - assert.Equal(t, - "Showing first page, 1 of 40; additional pages may be slow on large accounts", - boostAccountWideNotice(boostListFirstPage, 0, 1, 40)) - assert.Equal(t, - "Showing page 2, 1 of 40; additional pages may be slow on large accounts", - boostAccountWideNotice(boostListPage, 2, 1, 40)) - assert.Equal(t, - "Showing 1 of 40 results (raise or drop --limit for more)", - boostAccountWideNotice(boostListLimit, 0, 1, 40)) - assert.Empty(t, boostAccountWideNotice(boostListAll, 0, 40, 40), - "a complete traversal has nothing to warn about") - assert.Empty(t, boostAccountWideNotice(boostListFirstPage, 0, 40, 40), - "when the first page is the whole listing there are no additional pages to be slow") -} +// Boosts hang off a single item, so an ID is required. The account-wide feed +// that used to answer a bare `boost list` was an unlinked easter egg on the +// web side and has been withdrawn, so there is nothing to fall back to. +// +// A machine-output invocation gets a structured usage error; an interactive one +// gets help. Either way the point is that nothing is fetched — silently listing +// something else is the failure mode worth pinning. +func TestBoostListWithoutIDAsksForAnID(t *testing.T) { + // missingArg shows help interactively and errors otherwise; pin the + // non-interactive branch so the assertion is about the contract, not + // about whether the test process happens to look like a terminal. + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + buf := &bytes.Buffer{} + cmd, app, transport := setupBoostListTest(t, buf) -// TestBoostListIgnoresConfiguredProject verifies that a configured project — -// which cannot scope a per-item listing — is ignored rather than errored on. -func TestBoostListIgnoresConfiguredProject(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) - app.Config.ProjectID = "123" + err := executeBoostCommand(cmd, app, "list") - require.NoError(t, executeBoostCommand(cmd, app, "list")) - assert.Equal(t, "/99999/boosts.json", transport.last(t).Path) + requireBoostUsageError(t, err, "<id|url> required") + assert.Empty(t, transport.recorded(), "a missing argument must not reach the API") } -// TestBoostListAllProjectsOverridesConfiguredProject verifies that -// --all-projects pins account-wide intent over ambient config. -func TestBoostListAllProjectsOverridesConfiguredProject(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) +// A configured project cannot scope a per-item listing, so it must not be +// silently promoted into one. +func TestBoostListConfiguredProjectStillNeedsAnID(t *testing.T) { + // missingArg shows help interactively and errors otherwise; pin the + // non-interactive branch so the assertion is about the contract, not + // about whether the test process happens to look like a terminal. + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + buf := &bytes.Buffer{} + cmd, app, transport := setupBoostListTest(t, buf) app.Config.ProjectID = "123" - require.NoError(t, executeBoostCommand(cmd, app, "list", "--all-projects")) - assert.Equal(t, "/99999/boosts.json", transport.last(t).Path) -} - -// TestBoostListExplicitProjectWithoutIDAsksForID verifies that an explicit -// project without an ID is a usage error — through --project, its --in alias, -// and the root-level form that lands in app.Flags.Project. -func TestBoostListExplicitProjectWithoutIDAsksForID(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) - requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--project", "123"), - "--project alone cannot list them") + err := executeBoostCommand(cmd, app, "list") - cmd, app, _ = setupBoostListTest(t, nil) - requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--in", "123"), - "--project alone cannot list them") - - cmd, app, _ = setupBoostListTest(t, nil) - app.Flags.Project = "123" - requireBoostUsageError(t, executeBoostCommand(cmd, app, "list"), - "--project alone cannot list them") - - assert.Empty(t, transport.recorded(), "a usage error must not reach the API") -} - -// TestBoostListExplicitProjectWithAllProjectsConflicts verifies that -// --all-projects conflicts with an explicitly named project. -func TestBoostListExplicitProjectWithAllProjectsConflicts(t *testing.T) { - cmd, app, _ := setupBoostListTest(t, nil) - requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--project", "123", "--all-projects"), - "Cannot combine --all-projects with --project") - - cmd, app, _ = setupBoostListTest(t, nil) - app.Flags.Project = "123" - requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--all-projects"), - "Cannot combine --all-projects with --project") -} - -// TestBoostListIDWithAllProjectsConflicts verifies that an item ID and -// --all-projects name two different listings. -func TestBoostListIDWithAllProjectsConflicts(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) - - requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "456", "--all-projects"), - "Cannot combine --all-projects with an item ID") - assert.Empty(t, transport.recorded()) -} - -// TestBoostListAccountWideRejectsEvent verifies that --event, which names an -// event inside one item, is rejected rather than silently ignored. -func TestBoostListAccountWideRejectsEvent(t *testing.T) { - cmd, app, transport := setupBoostListTest(t, nil) - - requireBoostUsageError(t, executeBoostCommand(cmd, app, "list", "--event", "5"), "--event") + requireBoostUsageError(t, err, "<id|url> required") assert.Empty(t, transport.recorded()) } -// TestBoostListHasNoPaginationFlags verifies that the account-wide path did not -// grow a parallel pagination surface. -// The account-wide boost feed is slow, not unaddressable. Keeping it flagless -// meant the only reachable data was page 1; the flags do not make it fast, they -// make the rest of it reachable at all. -func TestBoostListHasPaginationFlags(t *testing.T) { +// The pagination flags existed only for the account-wide feed. With that gone +// they must not linger: an item's boosts arrive in one unpaginated response, +// and the SDK documents BoostListOptions.Page as not honoring a page number. +func TestBoostListHasNoPaginationFlags(t *testing.T) { list, _, err := NewBoostsCmd().Find([]string{"list"}) require.NoError(t, err) - for _, name := range []string{"limit", "page", "all"} { - assert.NotNil(t, list.Flags().Lookup(name), "boost list must carry --%s", name) + for _, name := range []string{"limit", "page", "all", "all-projects"} { + assert.Nil(t, list.Flags().Lookup(name), "boost list must not carry --%s", name) } - assert.Equal(t, "n", list.Flags().Lookup("limit").Shorthand) } -// An item's boosts arrive in one unpaginated response, and the SDK documents -// BoostListOptions.Page as not honoring the page number at all — setting Page=2 -// does not fetch page 2. Rejecting is honest; implementing would lie. -func TestBoostListItemScopedRejectsPaginationFlags(t *testing.T) { - for _, flag := range []string{"--limit=5", "--page=2", "--all"} { - t.Run(flag, func(t *testing.T) { - buf := &bytes.Buffer{} - cmd, app, _ := setupBoostListTest(t, buf) - - err := executeBoostCommand(cmd, app, "list", "789", "--project", "123", flag) - - require.Error(t, err, "an item's boosts have no page to address") - assert.Contains(t, err.Error(), "does not apply to an item's boosts") - }) - } -} - -// TestBoostListAccountWideMachineOutputKeepsPayload verifies that machine -// formats get the raw feed, nesting intact. -func TestBoostListAccountWideMachineOutputKeepsPayload(t *testing.T) { - buf := &bytes.Buffer{} - cmd, app, _ := setupBoostListTest(t, buf) - - require.NoError(t, executeBoostCommand(cmd, app, "list")) - - var envelope struct { - Data []struct { - ID int64 `json:"id"` - Recording *struct { - Title string `json:"title"` - } `json:"recording"` - } `json:"data"` - Summary string `json:"summary"` - } - require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) - require.Len(t, envelope.Data, 1) - require.NotNil(t, envelope.Data[0].Recording) - assert.Equal(t, "Ship it", envelope.Data[0].Recording.Title) - assert.Equal(t, "1 boost across all projects", envelope.Summary) -} - -// TestBoostListAccountWideStyledOutputFlattens verifies that styled output gets -// flat rows rather than a nested recording cell. -func TestBoostListAccountWideStyledOutputFlattens(t *testing.T) { +// --event names an event inside the item, so it still needs that item's ID. +func TestBoostListEventWithoutIDAsksForAnID(t *testing.T) { + // missingArg shows help interactively and errors otherwise; pin the + // non-interactive branch so the assertion is about the contract, not + // about whether the test process happens to look like a terminal. + t.Setenv("BASECAMP_NONINTERACTIVE", "1") buf := &bytes.Buffer{} - cmd, app, _ := setupBoostListTest(t, nil) - app.Output = output.New(output.Options{Format: output.FormatStyled, Writer: buf}) + cmd, app, transport := setupBoostListTest(t, buf) - require.NoError(t, executeBoostCommand(cmd, app, "list")) + err := executeBoostCommand(cmd, app, "list", "--event", "999") - rendered := buf.String() - assert.Contains(t, rendered, "Test Project") - assert.Contains(t, rendered, "Alice") - assert.Contains(t, rendered, "Ship it") - assert.NotContains(t, rendered, "Recording", "the nested recording must be flattened away, not rendered as a cell") -} - -// TestFlattenAccountWideBoostsNilPointers verifies that a boost missing its -// booster and recording still yields a row with every column. -func TestFlattenAccountWideBoostsNilPointers(t *testing.T) { - rows := flattenAccountWideBoosts([]basecamp.EverythingBoost{{ID: 7, Content: "👍"}}) - - require.Len(t, rows, 1) - assert.Equal(t, int64(7), rows[0]["id"]) - assert.Equal(t, "👍", rows[0]["content"]) - assert.Equal(t, "", rows[0]["booster"]) - assert.Equal(t, "", rows[0]["project"]) - assert.Equal(t, "", rows[0]["title"]) - assert.Equal(t, "", rows[0]["type"]) + requireBoostUsageError(t, err, "<id|url> required") + assert.Empty(t, transport.recorded()) } // mockBoostNilBoosterTransport returns a boost with no booster field. diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 95c066e8..d8cc8ca0 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -103,10 +103,9 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, ```bash printf '%s\n' '海报 mockup 方向稿:' '' '<bc-attachment ...>' | basecamp comments create <recording_id> - --in <project> --json ``` -6. **Project scope is mandatory for most commands** — via `--in <project>` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings <type>` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the eight list commands covered in item 7. -7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, `boost list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. +6. **Project scope is mandatory for most commands** — via `--in <project>` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings <type>` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the seven list commands covered in item 7. +7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected), `--page N` for exactly one page, or `--all` for everything. - `boost list --all-projects` is the exception: it returns only the first page by default. That feed is slow server-side — ~44s per page on a large account, which exceeds the client timeout — so on large accounts it currently fails outright (basecamp-cli#589). Item-scoped `boost list <id>` is unaffected. ### Output Modes @@ -164,7 +163,7 @@ basecamp <cmd> --page 1 # First page only, no auto-pagination > **Note:** Most queries require project scope (via `--in <project>` or `.basecamp/config.json`). Cross-project exceptions: `basecamp reports assigned`, `basecamp assignments`, `basecamp reports overdue`, `basecamp reports schedule`, `basecamp recordings <type>`, `basecamp notifications`, `basecamp gauges list`. > -> Eight list commands also list account-wide: `basecamp todos list --all-projects --json`, and likewise `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, `boost list`, and `checkins answers`. +> Seven list commands also list account-wide: `basecamp todos list --all-projects --json`, and likewise `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, and `checkins answers`. | Task | Command | |------|---------| From e63327f0382d752cd28acd15a39268cda34f5cde Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 30 Jul 2026 09:09:19 -0700 Subject: [PATCH 6/9] Record that the boosts withdrawal is temporary, and BC3's own cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BC3's brief supersedes the guesswork in the previous commit on two points, both worth having in the tree rather than only in an issue thread. The cause is not the derived-table materialization this repo inferred: the query's cost is proportional to the account's accessible recordings rather than to its boosts. That explains the ~44s and the flat cost curve without needing the EXPLAIN we asked for. And the withdrawal is temporary. The feed returns on a boost-proportional query via a boosts.bucket_id denormalization (basecamp/bc3#12463); the withdrawal itself is basecamp/bc3#12464, with public notice at basecamp/bc-api#427. So the docs now describe a listing that is gone for now, not one judged a bad idea — and boost.go says where the account-wide branch goes back when it returns. The SDK keeps Everything().Boosts() deliberately: removing a generated operation for a temporary withdrawal would churn every generated client twice. It 404s in the interim; the CLI simply does not call it. e2e/boost.bats needed no change — it covers help only, with no account-wide case to remove. --- ACCOUNT-WIDE-LISTINGS.md | 34 ++++++++++++++++++++++++--------- API-COVERAGE.md | 12 ++++++++---- internal/commands/boost.go | 7 ++++++- internal/commands/boost_test.go | 4 +++- 4 files changed, 42 insertions(+), 15 deletions(-) diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index 9f6ea7b0..956b2f71 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -271,24 +271,40 @@ behavior change and belongs in the release notes. There is **no account-wide boost listing.** `boost list` requires an item ID. -The `/boosts.json` aggregate this section used to document was an easter egg: -unlinked from the Basecamp web UI, and served by a query that spent ~44s in -MySQL on every page regardless of depth (basecamp/bc3#12458 has the numbers). -Rather than build a CLI contract on top of pathological database performance, -the endpoint is being removed server-side, so the CLI stops calling it. +The `/boosts.json` aggregate this section used to document was an easter egg — +unlinked from the Basecamp web UI, ~2,250 requests per 30 days globally, and +this CLI its only known consumer. BC5 has **withdrawn it** (bc3#12464); the +path 404s on both the web and API hosts once that deploys, so the CLI stops +calling it. + +The cause, from BC3's own diagnosis rather than inference: the query's cost is +proportional to the account's **accessible recordings, not its boosts**. That +is why it measured ~44s on the largest account and why page 40 cost the same as +page 1 (bc3#12458 has the timings). + +**The withdrawal is temporary.** The feed is expected back on a +boost-proportional query, via a `boosts.bucket_id` denormalization, with the +design record in bc3#12463. Public notice: basecamp/bc-api#427. So this section +describes a listing that is gone for now, not one that was judged a bad idea. What that means here: - `boost list` takes an item ID, and rejecting a bare invocation is the honest - answer rather than a fallback to something slower. + answer rather than a fallback to an endpoint that will 404. - It carries no `--all-projects`, `--limit`, `--page`, or `--all`. An item's boosts arrive in one unpaginated response, and the SDK documents `BoostListOptions.Page` as not honoring a page number, so there would be nothing for those flags to address even if the aggregate had survived. -- The SDK still exposes `Everything().Boosts()`. The CLI simply does not call - it; removing it is the SDK's decision to make, not this repo's. +- **The SDK keeps `Everything().Boosts()` on purpose.** Removing a generated + operation for a temporary withdrawal would churn every generated client + twice, so the operation stays and simply 404s until reintroduction. The CLI + not calling it is this repo's decision; the operation's existence is not. -Seven commands list account-wide, not eight. +Everything else is untouched: the other Everything feeds, the bucket-scoped +boosts endpoints, and the `boosts_count`/`boosts_url` recording attributes. + +Seven commands list account-wide, not eight — until the feed returns, at which +point this is the section to revisit. #### The `files list` filter exception diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 029db0ae..8e8b6960 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -26,9 +26,13 @@ covering cross-project messages, comments, checkins, forwards, boosts, files, an the open/completed/unassigned/overdue/no-due-date todo and card rollups. **16 of the 17 are reached from the CLI** — see [Account-wide aggregates](#account-wide-aggregates). `Everything().Boosts()` is deliberately -not called: the `/boosts.json` aggregate behind it was an unlinked easter egg -served by a query costing ~44s per page (basecamp/bc3#12458), and it is being -withdrawn server-side, so the CLI does not surface it. They are not a new command group and add +not called: BC5 has withdrawn the `/boosts.json` aggregate behind it +(basecamp/bc3#12464), because its cost was proportional to the account's +accessible recordings rather than its boosts (~44s per page — +basecamp/bc3#12458). The withdrawal is **temporary** — the feed returns on a +boost-proportional query, tracked in basecamp/bc3#12463 — so the SDK operation +is intentionally retained rather than removed, and the CLI simply does not +surface it in the meantime. They are not a new command group and add no endpoints to the tracked totals above: each aggregate is the account-wide variant of a listing the CLI already owned, reached through that group's existing leaf command. The contract is `ACCOUNT-WIDE-LISTINGS.md`. @@ -128,7 +132,7 @@ The **Since** column tags each row with the Basecamp version that introduced its | message_types | 5 | `messagetypes` | ✅ | BC4 | - | list, show, create, update, delete. Bucket-scoped (`/buckets/{id}/categories…`); commands are project-scoped via `--in`/`--project` | | campfires | 14 | `chat` | ✅ | BC4 | - | list, messages, post, line show/update/delete. @mentions in content | | comments | 8 | `comment`, `comments` | ✅ | BC4 | - | list, show, thread, create, update. @mentions in content. `show` surfaces `reply_target` + paste-ready `mention` from its single Get (no new calls). `thread` composes Get + parent recording (via type endpoint) + List into a deterministic reply-ready context (no new endpoints) | -| boosts | 6 | `boost`, `react` | ✅ | BC4 | - | list (recording + event), show, create (recording + event), delete. No account-wide listing — the `/boosts.json` aggregate is being withdrawn server-side (basecamp/bc3#12458) | +| boosts | 6 | `boost`, `react` | ✅ | BC4 | - | list (recording + event), show, create (recording + event), delete. No account-wide listing — BC5 withdrew `/boosts.json` (basecamp/bc3#12464); temporary, returns via basecamp/bc3#12463 | | notifications | 2 | `notifications` | ✅ | BC4 | - | list, mark as read (BC5: `bubble_ups`/`scheduled_bubble_ups` sections; `memories` is BC4-only) | | bubble_ups | 1 | `notifications bubbleups` | ✅ | BC5 | - | Dedicated Bubble Ups list (`GET /my/readings/bubble_ups.json`, paginated) plus the `limit_bubble_ups` variant behind `notifications list --limit-bubble-ups` | | **Cards (Kanban)** | diff --git a/internal/commands/boost.go b/internal/commands/boost.go index aa091608..46a4c78c 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -85,7 +85,12 @@ response, so there is nothing to page through either.`, func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, eventID string) error { // Boosts hang off a single recording, so only an item ID can scope this // listing — a project cannot, and there is no account-wide equivalent to - // fall back to. + // fall back to: BC5 withdrew /boosts.json (basecamp/bc3#12464) because its + // cost scaled with accessible recordings rather than with boosts. + // + // That withdrawal is temporary — the feed is expected back on a + // boost-proportional query (basecamp/bc3#12463). When it returns, this is + // where the account-wide branch goes back, along with --all-projects. recordingID, urlProjectID := extractWithProject(recording) projectID := project diff --git a/internal/commands/boost_test.go b/internal/commands/boost_test.go index bdeab729..79023dda 100644 --- a/internal/commands/boost_test.go +++ b/internal/commands/boost_test.go @@ -259,7 +259,9 @@ func TestBoostListWithIDStaysItemScoped(t *testing.T) { // Boosts hang off a single item, so an ID is required. The account-wide feed // that used to answer a bare `boost list` was an unlinked easter egg on the -// web side and has been withdrawn, so there is nothing to fall back to. +// web side and BC5 has withdrawn it (basecamp/bc3#12464), so there is nothing +// to fall back to. If it returns (basecamp/bc3#12463), this test is the one +// that says so. // // A machine-output invocation gets a structured usage error; an interactive one // gets help. Either way the point is that nothing is fetched — silently listing From 3522e608cb8279e396f3a6002e62af56aeb6707e Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 30 Jul 2026 10:13:10 -0700 Subject: [PATCH 7/9] The SDK is dropping the boosts aggregate too, not just the CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The withdrawal brief asked that Everything().Boosts() stay in the SDK, on the grounds that churning every generated client twice for a temporary withdrawal was worse than letting it 404. That call has changed: the endpoint is going away for real server-side, so shipping an operation that cannot work is worse than the churn. Nothing to do here beyond correcting the docs, which said the opposite. The CLI carries zero references to the aggregate — verified across code and tests, not just assumed — so whichever SDK release removes it lands as a no-op, and the next bump needs no CLI change. API-COVERAGE.md records that its "16 of 17" becomes 16 of 16 once the CLI pins an SDK without it, rather than pre-claiming a version that isn't pinned yet. Also drops a stale I6 line about EverythingBoost's pointer fields: nothing flattens that payload any more. --- ACCOUNT-WIDE-LISTINGS.md | 12 +++++++----- API-COVERAGE.md | 17 +++++++++-------- internal/commands/boost.go | 8 +++++--- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/ACCOUNT-WIDE-LISTINGS.md b/ACCOUNT-WIDE-LISTINGS.md index 956b2f71..03012781 100644 --- a/ACCOUNT-WIDE-LISTINGS.md +++ b/ACCOUNT-WIDE-LISTINGS.md @@ -295,10 +295,13 @@ What that means here: boosts arrive in one unpaginated response, and the SDK documents `BoostListOptions.Page` as not honoring a page number, so there would be nothing for those flags to address even if the aggregate had survived. -- **The SDK keeps `Everything().Boosts()` on purpose.** Removing a generated - operation for a temporary withdrawal would churn every generated client - twice, so the operation stays and simply 404s until reintroduction. The CLI - not calling it is this repo's decision; the operation's existence is not. +- **`Everything().Boosts()` is being removed from the SDK too.** The initial + brief asked that it stay — churning every generated client twice for a + temporary withdrawal seemed worse than letting it 404 — but the endpoint is + going away for real on the server, so the SDK drops it rather than ship an + operation that cannot work. That is the SDK's call; this repo only has to not + call it, which it already does. The CLI carries **zero** references to the + aggregate, so the SDK bump that removes it needs no CLI change. Everything else is untouched: the other Everything feeds, the bucket-scoped boosts endpoints, and the `boosts_count`/`boosts_url` recording attributes. @@ -389,7 +392,6 @@ groups and `--ids` finds no ids at all, both silently. `EverythingFile` is an all-pointer superset over the Upload, Document, and Attachment variants — every field read during flattening must be nil-checked. -`EverythingBoost.Booster` and `.Recording` are pointers too. ### I7 — No interactive prompt diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 8e8b6960..59d6432a 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -25,14 +25,15 @@ basecamp/basecamp-sdk#435 and #438), a 17-method account-wide aggregate family covering cross-project messages, comments, checkins, forwards, boosts, files, and the open/completed/unassigned/overdue/no-due-date todo and card rollups. **16 of the 17 are reached from the CLI** — see [Account-wide -aggregates](#account-wide-aggregates). `Everything().Boosts()` is deliberately -not called: BC5 has withdrawn the `/boosts.json` aggregate behind it -(basecamp/bc3#12464), because its cost was proportional to the account's -accessible recordings rather than its boosts (~44s per page — -basecamp/bc3#12458). The withdrawal is **temporary** — the feed returns on a -boost-proportional query, tracked in basecamp/bc3#12463 — so the SDK operation -is intentionally retained rather than removed, and the CLI simply does not -surface it in the meantime. They are not a new command group and add +aggregates](#account-wide-aggregates). `Everything().Boosts()` is not called: +BC5 has withdrawn the `/boosts.json` aggregate behind it (basecamp/bc3#12464), +because its cost was proportional to the account's accessible recordings rather +than its boosts (~44s per page — basecamp/bc3#12458). The feed is expected back +later on a boost-proportional query (basecamp/bc3#12463), but the endpoint is +genuinely gone server-side in the meantime, so **the SDK is dropping the +operation as well**. The CLI holds no references to it, so that SDK removal +lands here as a no-op. Once the CLI pins an SDK without it, this line becomes +16 of 16. They are not a new command group and add no endpoints to the tracked totals above: each aggregate is the account-wide variant of a listing the CLI already owned, reached through that group's existing leaf command. The contract is `ACCOUNT-WIDE-LISTINGS.md`. diff --git a/internal/commands/boost.go b/internal/commands/boost.go index 46a4c78c..3d70a0e4 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -88,9 +88,11 @@ func runBoostList(cmd *cobra.Command, app *appctx.App, recording, project, event // fall back to: BC5 withdrew /boosts.json (basecamp/bc3#12464) because its // cost scaled with accessible recordings rather than with boosts. // - // That withdrawal is temporary — the feed is expected back on a - // boost-proportional query (basecamp/bc3#12463). When it returns, this is - // where the account-wide branch goes back, along with --all-projects. + // The feed is expected back later on a boost-proportional query + // (basecamp/bc3#12463), but the endpoint is genuinely gone in the meantime + // and the SDK is dropping Everything().Boosts() with it. When it returns, + // this is where the account-wide branch goes back, along with + // --all-projects. recordingID, urlProjectID := extractWithProject(recording) projectID := project From abcfd3000a06eb3d6644502a00bbd34ba6cc3019 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 30 Jul 2026 16:18:09 -0700 Subject: [PATCH 8/9] Stop the bounded walk dropping every page after a sparse one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings, two of them correctness. A grouped page whose groups all happen to be empty is sparse, not terminal, and the walk returned on it — silently dropping every project on every later page, and suppressing the truncation notice while doing so. My own test enshrined the data loss, asserting page 3 was never requested. It now asserts the opposite. The justification for that guard was wrong on its face: it claimed to prevent requesting "the same page shape forever", but the loop increments page, so it was never at risk of re-requesting anything. The empty page is the server's actual end-of-listing signal and is now the only content-based stop. A page ceiling replaces the guard as the runaway backstop, and hitting it reports capped rather than passing a bounded walk off as a complete listing. The collected slice started nil, so an empty first page marshalled to "data": null while the SDK-backed --all path returns []. Consumers iterating .data[] had to handle both. Verified against production: an account-wide listing with no matches now returns []. Three contract defects alongside them. cards, checkins and forwards still advertised "--limit 0 = all" while their account-wide default had become 100. accountWideRespOpts told users to "drop --limit for more", which returns fewer once the explicit limit exceeds the default. And accountWideCapNotice claimed to defer whenever the server reported a total but only deferred when that total exceeded the count, so it could contradict the server by saying "more may exist" when the server said there was nothing more. --- internal/commands/accountwide.go | 49 +++++++++++++------- internal/commands/accountwide_test.go | 67 +++++++++++++++++++++++++-- internal/commands/cards.go | 2 +- internal/commands/checkins.go | 2 +- internal/commands/forwards.go | 2 +- 5 files changed, 97 insertions(+), 25 deletions(-) diff --git a/internal/commands/accountwide.go b/internal/commands/accountwide.go index 7518b8f8..3647b3a4 100644 --- a/internal/commands/accountwide.go +++ b/internal/commands/accountwide.go @@ -63,6 +63,12 @@ func accountWidePage(page int, all bool) (int32, error) { // project's items and capping the whole account are not the same promise. const accountWideDefaultLimit = 100 +// accountWideMaxPages bounds the walk so a server that never returns an empty +// page cannot spin forever. It is a runaway backstop, not a tuning knob: the +// default cap of 100 items is met within a handful of pages, and hitting this +// ceiling is reported as a capped result rather than a complete one. +const accountWideMaxPages = 1000 + // accountWideCollect walks positive pages until it has collected at least limit // items, which is far cheaper than fetching every page only to throw most of it // away. It is the shared engine behind every bounded account-wide listing. @@ -72,8 +78,8 @@ const accountWideDefaultLimit = 100 // the number of items nested inside the groups, since capping groups would drop // whole projects from the listing. // -// The walk stops on the first of: an empty page, a page that adds no items, -// reaching the cap, or exhausting the server's reported total. +// The walk stops on the first of: an empty page, reaching the cap, exhausting +// the server's reported total, or the page ceiling. // // Two things callers must know: // @@ -93,13 +99,13 @@ func accountWideCollect[T any]( count func([]T) int, limit int, ) ([]T, bool, basecamp.ListMeta, error) { - var ( - items []T - meta basecamp.ListMeta - collected int - ) + // Non-nil so an empty first page renders as [] rather than null. The + // SDK-backed --all path returns an empty slice, and consumers iterating + // .data[] should not have to handle both. + items := make([]T, 0) + var meta basecamp.ListMeta - for page := int32(1); ; page++ { + for page := int32(1); page <= accountWideMaxPages; page++ { pageItems, pageMeta, err := fetch(page) if err != nil { return nil, false, basecamp.ListMeta{}, err @@ -107,19 +113,16 @@ func accountWideCollect[T any]( if page == 1 { meta = pageMeta } + // An empty page is the server saying the listing has ended. It is the + // only content-based stop: a page carrying groups that happen to hold + // no items is sparse, not terminal, and later pages may still have + // results. Stopping on one used to drop every project after it. if len(pageItems) == 0 { return items, false, meta, nil } items = append(items, pageItems...) - - // A page that carries only empty groups makes no progress toward the - // cap; stop rather than request the same page shape forever. - n := count(items) - if n == collected { - return items, false, meta, nil - } - collected = n + collected := count(items) // Exhaustion is tested before the cap, and against the first page's // total rather than this page's. @@ -141,6 +144,11 @@ func accountWideCollect[T any]( return items, true, meta, nil } } + + // Ran out of page budget with the server still handing back items. Report + // it as capped: whatever remains is unreached, and saying so beats + // presenting a bounded walk as a complete listing. + return items, true, meta, nil } // accountWideFlatCount is the count function for the flat listings, where an @@ -156,7 +164,10 @@ func accountWideFlatCount[T any](items []T) int { return len(items) } // early by design, so on the feeds that withhold X-Total-Count the count is all // there is to say. func accountWideCapNotice(capped bool, meta basecamp.ListMeta, count int, plural string) string { - if !capped || meta.TotalCount > count { + // Any server-reported total wins, not just one larger than the count. + // When the total equals or trails the count there is demonstrably nothing + // more to show, and "more may exist" would contradict the server. + if !capped || meta.TotalCount > 0 { return "" } return fmt.Sprintf( @@ -248,7 +259,9 @@ func accountWideRespOpts(count int, singular, plural string, meta basecamp.ListM if meta.TotalCount > count { notice := fmt.Sprintf("Showing %d of %d results", count, meta.TotalCount) if explicitLimit { - notice += " (raise or drop --limit for more)" + // Not "drop --limit": with an explicit limit above the default, + // dropping it falls back to 100 and returns fewer. + notice += " (raise --limit, or use --all for the complete list)" } else { notice += " (use --all for the complete list)" } diff --git a/internal/commands/accountwide_test.go b/internal/commands/accountwide_test.go index 4e9f723d..b5218e5f 100644 --- a/internal/commands/accountwide_test.go +++ b/internal/commands/accountwide_test.go @@ -225,8 +225,10 @@ func TestAccountWideCollectStopsOnEmptyPage(t *testing.T) { } // A page can be non-empty at the top level while adding no items, when every -// group on it is empty. Counting groups would call that progress and loop. -func TestAccountWideCollectStopsWhenAPageAddsNoItems(t *testing.T) { +// group on it happens to be empty. That is sparsity, not the end of the +// listing, and the walk used to stop dead on it — silently dropping every +// project on every later page. +func TestAccountWideCollectContinuesPastAPageWithNoInnerItems(t *testing.T) { pager := &collectPager[collectGroup]{pages: [][]collectGroup{ {{items: []int{1, 2}}}, {{items: nil}}, @@ -236,9 +238,66 @@ func TestAccountWideCollectStopsWhenAPageAddsNoItems(t *testing.T) { items, capped, _, err := accountWideCollect(pager.fetch, countCollectGroups, 100) require.NoError(t, err) - assert.Equal(t, 2, countCollectGroups(items)) + assert.Equal(t, 3, countCollectGroups(items), "the item on page 3 survives the empty group on page 2") + assert.Len(t, items, 3, "all three groups are kept") + assert.False(t, capped, "the listing ran out; it was not truncated") + assert.Equal(t, 4, pager.requests, "walks to the empty page that actually ends the listing") +} + +// The empty page is the only content-based stop. Everything else — cap, server +// total, page ceiling — is a bound we impose. +func TestAccountWideCollectStopsOnlyOnATrulyEmptyPage(t *testing.T) { + pager := &collectPager[collectGroup]{pages: [][]collectGroup{ + {{items: nil}}, + {{items: nil}}, + {{items: []int{1}}}, + }} + + items, _, _, err := accountWideCollect(pager.fetch, countCollectGroups, 100) + + require.NoError(t, err) + assert.Equal(t, 1, countCollectGroups(items), + "two consecutive item-less pages must not hide the third") +} + +// A server that never returns an empty page must not spin forever. The ceiling +// is a runaway backstop, and hitting it is reported as capped rather than +// passed off as a complete listing. +func TestAccountWideCollectStopsAtThePageCeiling(t *testing.T) { + pager := &endlessPager{} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 1<<30) + + require.NoError(t, err) + assert.True(t, capped, "the walk was cut short, so more may exist") + assert.Equal(t, accountWideMaxPages, pager.requests) + assert.Len(t, items, accountWideMaxPages) +} + +// endlessPager always returns one item and never an empty page. +type endlessPager struct{ requests int } + +func (p *endlessPager) fetch(int32) ([]int, basecamp.ListMeta, error) { + p.requests++ + return []int{p.requests}, basecamp.ListMeta{}, nil +} + +// An empty first page must render as [] rather than null: the SDK-backed --all +// path returns an empty slice, and consumers iterating .data[] should not have +// to handle both shapes. +func TestAccountWideCollectReturnsEmptySliceNotNil(t *testing.T) { + pager := &collectPager[int]{pages: [][]int{{}}} + + items, capped, _, err := accountWideCollect(pager.fetch, accountWideFlatCount[int], 100) + + require.NoError(t, err) + require.NotNil(t, items, "a nil slice marshals to null") + assert.Empty(t, items) assert.False(t, capped) - assert.Equal(t, 2, pager.requests, "the no-progress page ends the walk; page 3 is never asked for") + + encoded, err := json.Marshal(items) + require.NoError(t, err) + assert.Equal(t, "[]", string(encoded)) } // The helper deliberately does not trim: it stops at the first page boundary at diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 965c7a01..95facaef 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -106,7 +106,7 @@ func newCardsListCmd(project, cardTable *string) *cobra.Command { } cmd.Flags().StringVarP(&opts.column, "column", "c", "", "Filter by column ID or name") - cmd.Flags().IntVarP(&opts.limit, "limit", "n", 0, "Maximum number of cards to fetch (0 = all)") + cmd.Flags().IntVarP(&opts.limit, "limit", "n", 0, "Maximum number of cards to fetch (0 = all in one project, 100 account-wide; use --all for every page)") cmd.Flags().BoolVar(&opts.all, "all", false, "Fetch all cards (no limit)") cmd.Flags().IntVar(&opts.page, "page", 0, "Fetch a single page (use --all for everything)") cmd.Flags().StringVar(&opts.sortField, "sort", "", "Sort by field (title, created, updated, position, due)") diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index 690af312..fc423fbc 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -603,7 +603,7 @@ on its own cannot name a question; --all-projects states that intent outright: }, } - cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of answers to fetch (0 = all)") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of answers to fetch (0 = all for one question, 100 account-wide; use --all for every page)") cmd.Flags().BoolVar(&all, "all", false, "Fetch all answers (no limit)") cmd.Flags().IntVar(&page, "page", 0, "Fetch a single page (use --all for everything)") cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List check-in answers across every project (no question ID)") diff --git a/internal/commands/forwards.go b/internal/commands/forwards.go index 01a4c0e5..dbed16eb 100644 --- a/internal/commands/forwards.go +++ b/internal/commands/forwards.go @@ -65,7 +65,7 @@ override a configured one — lists forwards across every accessible project.`, }, } - cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of forwards to fetch (0 = all)") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of forwards to fetch (0 = all in one project, 100 account-wide; use --all for every page)") cmd.Flags().BoolVar(&all, "all", false, "Fetch all forwards (no limit)") cmd.Flags().IntVar(&page, "page", 0, "Fetch a single page (use --all for everything)") cmd.Flags().BoolVar(&allProjects, "all-projects", false, "List forwards across every project") From 186feb1639f7512e3d1015060b11151fa8669004 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 30 Jul 2026 17:08:50 -0700 Subject: [PATCH 9/9] Stop telling agents to use --page on the overdue listings SKILL.md said every account-wide listing takes --page. The two overdue variants come from unpaginated endpoints and reject it, so an agent following the skill would generate a command the CLI refuses. Scope --page to the paginated listings and name the exception, with what the overdue paths do accept: --limit and --all. Verified against production rather than read off the source: both overdue variants reject --page 2 and return rows for --all. Note the drift checker cannot catch this class of error. It validates that a flag exists on a command, and --page does exist on todos list and cards list; it is rejected at runtime only in combination with --overdue. The prose is the only guard, which is the reason to keep it exact. --- skills/basecamp/SKILL.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index d8cc8ca0..fbf4a58c 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -105,7 +105,8 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, ``` 6. **Project scope is mandatory for most commands** — via `--in <project>` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings <type>` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the seven list commands covered in item 7. 7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. - Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected), `--page N` for exactly one page, or `--all` for everything. + Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected) or `--all` for everything. `--page N` fetches exactly one page, but only on the paginated listings. + The two overdue variants — `basecamp todos list --all-projects --overdue` and `basecamp cards list --all-projects --overdue` — come from unpaginated endpoints. They accept `--limit` and `--all` but **reject `--page`**, so do not generate `--page` against them. ### Output Modes