Bound the account-wide listings, and drop the boost aggregate - #590
Merged
Conversation
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.
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.
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.
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.
github-actions
Bot
added
commands
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2782cfa5a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR makes account-wide listing commands scale on large Basecamp accounts by replacing implicit full-account crawls with a shared bounded page-walk (default cap 100), and by adding missing pagination flags so operators can explicitly request more (or everything) when needed.
Changes:
- Introduces
accountWideCollectto bound account-wide fetches via positive pages, with consistent stop conditions and caller-side trimming. - Standardizes account-wide defaults to a cap of 100 (with the documented
boost listdefault exception) and updates notices to match bounded vs complete results. - Adds
--limit/-n,--page,--allto previously-flagless account-wide listings (files list,boost list) and rejects these flags on truly unpaginated scoped paths.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Documents new bounded defaults and pagination flags for account-wide listings. |
| internal/commands/accountwide.go | Adds shared bounded collector + helpers, updates response option helper signature. |
| internal/commands/accountwide_test.go | Adds unit tests covering all accountWideCollect stop/guard semantics. |
| internal/commands/recording_test.go | Makes stub routes page-aware to support bounded-walk testing. |
| internal/commands/todos.go | Switches grouped todo account-wide collection to accountWideCollect; updates overdue --all behavior. |
| internal/commands/todos_test.go | Updates defaults and adds coverage for overdue --all behavior. |
| internal/commands/messages.go | Uses bounded collection for unsorted account-wide; preserves full-fetch for sorted mode; adjusts notices. |
| internal/commands/messages_test.go | Updates expectations for new default cap behavior and request sequencing. |
| internal/commands/comment.go | Switches account-wide comment collection to bounded walk + cap notice helper. |
| internal/commands/forwards.go | Bounds the default account-wide listing; supports explicit page/--all behavior. |
| internal/commands/forwards_test.go | Pins request sequences for bounded defaults via page-aware stubs. |
| internal/commands/checkins.go | Bounds default account-wide answers listing and adds cap notices consistent with bounded fetch. |
| internal/commands/checkins_test.go | Updates assertions for bounded behavior and new notice wording. |
| internal/commands/files.go | Adds pagination flags for account-wide files; rejects paging flags on project-scoped folder listing. |
| internal/commands/files_test.go | Adds coverage for new files pagination flags and project-scoped rejection behavior. |
| internal/commands/cards.go | Bounds default account-wide card listings; updates overdue behavior to accept --all and cap by default. |
| internal/commands/cards_test.go | Pins request sequences and adds tests for overdue --all semantics and updated notices. |
| internal/commands/boost.go | Adds pagination flags to account-wide boosts with first-page default; rejects paging flags for item-scoped boosts. |
| internal/commands/boost_test.go | Adds coverage for the four boost account-wide modes and new flags/rejections. |
| ACCOUNT-WIDE-LISTINGS.md | Updates the contract: bounded defaults, new pagination rules/flags, and documented exceptions. |
| .surface | Records the additive CLI surface changes for new flags across spellings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
jeremy
changed the title
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
internal/commands/accountwide.go:157
- The comment above accountWideCapNotice says it defers whenever the server reported a total, but the implementation actually defers only when
meta.TotalCount > count. Updating the comment to match the real behavior will avoid confusion for future readers.
// 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.
internal/commands/accountwide.go:253
- When
meta.TotalCount > countand the user is on the default cap (no explicit --limit), the notice currently only suggests--all. That pushes users toward a potentially slow full crawl even though--limitis also a supported (and often cheaper) way to raise the cap.
notice += " (use --all for the complete list)"
internal/commands/accountwide.go:229
- This comment references "1 boosts" as the motivating example for singular/plural handling, but account-wide boosts have been removed in this PR. Consider making the wording generic so it stays accurate even as listings come and go.
// accountWideRespOpts builds the summary and truncation notice shared by the
// 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.
internal/commands/accountwide.go:164
- accountWideCapNotice takes only a plural noun and will emit ungrammatical text for singular counts (e.g., "Showing the first 1 messages"). Consider changing the helper to accept both singular and plural (or otherwise handle singularization) and update callers accordingly.
return fmt.Sprintf(
"Showing the first %d %s; more may exist (use --all for every page, or --limit to raise the cap)",
count, plural)
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e63327f038
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
skills/basecamp/SKILL.md:108
- The new account-wide pagination summary implies
--limitalways bounds the fetch by walking pages until N items are collected. That’s not true for at least two cases in this PR:messages list --sort ...fetches all pages before applying the cap, and the overdue listings are unpaginated so--limittrims locally. This line should be softened or call out the exceptions so the skill doc matches actual CLI behavior.
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.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
skills/basecamp/SKILL.md:108
- The new note about
--limitis not always accurate: some account-wide listings (e.g.messages list --sort ...) must fetch all pages before trimming, so they don’t “walk pages until N are collected”. This reads as a contract promise for all account-wide listings.
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.
internal/commands/accountwide.go:165
accountWideCapNoticealways prints the plural noun, so a capped single-item result becomes grammatically incorrect (e.g. "Showing the first 1 check-in answers"). This is user-facing output and shows up in the new bounded listings when X-Total-Count is absent.
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)
}
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abcfd3000a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Soaking
v0.8.0-rc.1against a ~80-project production account showed the account-wide listings unusable at real scale. The cause was a contract decision rather than a bug:ACCOUNT-WIDE-LISTINGS.mdI5 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. The same word bought very different amounts of work.I5 also blessed "fetch page 0, then truncate", which made
--limituseless exactly when it mattered:cards list --limit 3fetched all 2142 cards before trimming.What changed
accountWideCollect— one bounded walk over positive pages, shared by all eight paths (todos,comment,messages,cards,checkins,forwards,files,boost). It stops on an empty page, a page that adds no items, the cap, or the server's declared total. Two rules callers must obey are stated on the helper:Meta.TotalCountcompares againstlen(items)— top-level elements, which on a grouped payload are the groups, never the inner-item counter.Uniform default cap of 100;
--allis how you ask for the account. Changes the default forcards,checkins,forwards,files. Project-scoped defaults untouched.Pagination flags for bare
files list—--limit/-n,--page,--all. A listing with no escape hatch cannot recover from a server error mid-crawl, which is exactly how thefiles500 became unrecoverable. Project-scopedfiles listhas no pagination to thread, so it rejects all three by name rather than accepting and ignoring them.The account-wide boost listing is withdrawn.
/boosts.jsonturned out to be an easter egg — unlinked from the web UI, ~2,250 req/30d globally, with this CLI its only known consumer. BC5 is withdrawing it (basecamp/bc3#12464) because the query's cost is proportional to the account's accessible recordings rather than its boosts, which is what produced ~44s per page and a flat cost curve (basecamp/bc3#12458).boost listnow requires an item ID and carries no--all-projects/--limit/--page/--all. Seven commands list account-wide, not eight.The feed is expected back later on a boost-proportional query via a
boosts.bucket_iddenormalization (basecamp/bc3#12463), public notice at basecamp/bc-api#427 — the docs andboost.gomark where the account-wide branch goes back. In the meantime the endpoint is genuinely gone, so the SDK is droppingEverything().Boosts()as well. This PR carries zero references to that aggregate, verified across code and tests, so whichever SDK release removes it lands here as a no-op.Landing this before v0.8.0 stable is the point — otherwise the release ships a command that 404s.
Both overdue paths now accept
--alland reject--page. They are unpaginated, so--pageaddresses nothing; but capping at 100 and rejecting--allhad left item 101 unreachable by any combination of flags.Soak results (account 2914079)
cards list --all-projects --status completedcheckins answers --all-projectscards list --limit 3forwards list --all-projectscomments list --all-projectsmessages list --all-projectsNo default or
--limitrequest omitspage=any more.--allstill omits it deliberately — that is the spelling for the full traversal.What this does not fix
One listing remains broken for a server-side reason, filed and holding the release (basecamp-cli#589):
files.json500s on any page holding an attachment whose parent is an autolinked image. The bounded default does not dodge it: GearedPagination ramps page sizes (15, 30, 50, …), so the first 100 items span pages 1–4 and the walk lands on the failing page.The other blocker, basecamp/bc3#12458 (
boosts.jsonat ~44s/page), is resolved here by removing the caller — see above.e2e/boost.batsneeded no change; it covers help only.Merging this is independent of #12459; the release is what waits on it.
Notes for review
accountWideRespOptslost itsmoreFlagparameter. It existed so the two flagless commands would not be advised to use an--allthey did not have — they have one now, and the linter caught the parameter going constant.stubRoute.pages). Without it a fixture serves the same body for every page number, so a bounded walk never sees an empty page and runs to the cap — which would have hidden the behavior under test.TotalCount-vs-len(items)rule, page-boundary overshoot, the exact-cap case (TotalCount == limitis complete, not truncated), and a total present only on page 1..surface: 21 additive lines (3 flags × 7files-family spellings). Boost contributes the only removals —--all-projectsand the argument going optional → required, both of which shipped inv0.8.0-rc.1, so both are acknowledged in.surface-breaking. The--limit/--page/--allthis branch briefly gave boost never shipped, so they are rewound out of the baseline rather than recorded as breaks that never happened. Net change against rc.1 is exactly the two real ones.Summary by cubic
Bound account-wide listings to a fast, bounded page walk with a 100-item default cap, replacing full-account crawls and making
--limiteffective, and fixed sparse-page truncation with consistent[]empty results. Removed the account-wide boosts aggregate;boost listis now item-scoped only and requires<id|url>, and agent docs now scope--pageoff overdue listings.New Features
todos,comments,messages,cards,checkins,forwards, and account-widefiles; walks positive pages and stops on empty page, no new items, cap, or known total; adds a page ceiling for runaway backstop.--allfor the whole account. Project-scoped defaults unchanged.--limit,--page,--allto account-widefiles list; project-scopedfiles listrejects these. Overdue listings accept--alland reject--page.boost listnow requires<id|url>. The CLI no longer callsEverything().Boosts()and the SDK will drop it; docs note the CLI will surface 16/16 aggregates once pinned.Bug Fixes
--limitno longer fetches everything before trimming; callers trim after a page-boundary overshoot.Meta.TotalCounthandling (use first-page totals), avoid false truncation when total equals the limit, don’t switch the bound off mid-walk, and ensuremessagesandcommentssurface truncation when totals are absent.[](notnull).cards,checkins, andforwardsnow reflects the 100-item account-wide default. Agent guidance inskills/basecamp/SKILL.mdnow states overdue listings reject--pageand accept--all/--limit.moreFlag; tests and stub transport are page-aware to validate bounded walks.Written for commit 186feb1. Summary will update on new commits.