Skip to content

Bump SDK to v0.10.0 and wire its account-wide aggregates - #584

Merged
jeremy merged 27 commits into
mainfrom
sdk-0.10.0
Jul 29, 2026
Merged

Bump SDK to v0.10.0 and wire its account-wide aggregates#584
jeremy merged 27 commits into
mainfrom
sdk-0.10.0

Conversation

@jeremy

@jeremy jeremy commented Jul 29, 2026

Copy link
Copy Markdown
Member

Moves the SDK pin from bb363c84 to the v0.10.0 tag, ahead of cutting release N.

Breaking Go surface, and how it was adapted

UpdateCardRequest fields became *string (basecamp/basecamp-sdk#489), where nil now means "leave unchanged" instead of the field being merge-unsafe. The three call sites (cards update, plus the workspace TUI's due-date and title editors) already guarded on non-empty values, so they take addresses and preserve the same "only send what was asked for" behavior — now with merge-safe semantics underneath.

SearchResult.Content and .Description became *string and always arrive null (basecamp/basecamp-sdk#487). The server's highlighted excerpt moved to PlainTextContent/PlainTextDescription. The workspace search view now reads those. This one is worth a second look in review: fixing the compile error by dereferencing the old fields would have compiled cleanly and produced permanently blank excerpts. The search command itself never read those fields (it emits id/title/type/project/created), so CLI output is unaffected.

Behavior riding along

EverythingService is now wired

v0.10.0 adds EverythingService via AccountClient.Everything() (basecamp/basecamp-sdk#435, #438): 17 methods covering account-wide messages, comments, checkins, forwards, boosts, files, and the open/completed/unassigned/overdue/no-due-date todo and card rollups. All 17 are reachable.

They are not a new command group. Everything is web-UI vocabulary; each aggregate is the account-wide variant of a listing the CLI already owned, so it lands on the group that owns its noun, reached by that group's existing leaf command. --all-projects pins the intent and overrides a configured project; with nothing in scope, these commands list account-wide instead of prompting for a project.

The full contract — dispatch, pagination, sorting, output shape, and the complete inventory of added flags — is ACCOUNT-WIDE-LISTINGS.md, added here. It exists because two earlier attempts at this work were reverted for the same defect: a flag accepted and silently ignored. Everything the aggregates accept either changes the result or returns an actionable ErrUsage.

Behavior changes to note at release

  • The interactive project prompt no longer fires on todos list, cards list, messages list, comments list, files list, forwards list, boost list, or checkins answers when no project is configured — they list account-wide instead.
  • todos list --overdue with no project used to error with a redirect to reports overdue. It now returns results. --assignee still errors, still pointing at reports assigned.
  • comments list, boost list, and checkins answers take an optional item ID now instead of a required one. Acknowledged in .surface-breaking: an optional positional accepts everything the required one did.
  • files list --json changes shape when it goes account-wide — the feed returns the SDK's EverythingFile superset (title), where the project-scoped path returns the CLI's own row struct (name).
  • vaults list / folders list are aliases of files list, and the account-wide feed carries no folder variant, so those spellings return no folders across projects.

Verification

bin/ci green: build, vet, lint, unit + 346 e2e tests, naming, CLI surface snapshot up to date, skill drift, bare-group convention, leaf-command coverage (0 uncovered), SDK provenance OK at v0.10.0, go mod tidy.


Summary by cubic

Upgrades SDK to github.com/basecamp/basecamp-sdk/go v0.10.0 and wires its EverythingService into the CLI to enable account‑wide listings. Card updates and search excerpts keep working; HTTP 400 now returns the validation exit code, and account‑wide outputs flatten and include the project, including overdue lists.

  • New Features

    • --all-projects added to eight list commands; runs account‑wide when no project is in scope.
    • boost list, comments list, and checkins answers accept an optional ID; bare calls list account‑wide.
    • Cards/Todos: new selectors (--status completed, --unassigned, --no-due-date, --not-now, --overdue) for account‑wide listings.
    • Files: --kind and --person filters for account‑wide listings; docs/documents imply --kind documents and reject an explicit --kind; vaults/folders refuse account‑wide and point to project‑scoped listing.
    • Styled and markdown output flatten all account‑wide payloads and include the project; machine formats keep raw payloads.
    • Search excerpts read PlainTextContent/PlainTextDescription and strip highlight markup.
  • Dependencies

    • SDK v0.10.0 adopted; EverythingService reached via existing commands and documented in ACCOUNT-WIDE-LISTINGS.md and API-COVERAGE.md.
    • Model: UpdateCardRequest fields are *string; CLI and workspace TUI updated.
    • Transport: HTTP 400 maps to validation; retries honor per‑op max, replay bodies, and respect retry_on.
    • Pagination: out‑of‑range --page is now rejected across account‑wide listings (was clamped); exit codes unchanged otherwise. 400 now exits with 9 (was 7).

Written for commit f8a1465. Summary will update on new commits.

Review in cubic

jeremy added 2 commits July 28, 2026 23:34
Moves the pin from bb363c84 to the v0.10.0 tag. Two adaptations were
forced by the new surface:

UpdateCardRequest's Title, Content, and DueOn are now *string, where nil
leaves the field unchanged. The call sites already guarded on non-empty
values, so they now take addresses and preserve the same "only send what
was asked for" behavior — with the SDK's merge-safe semantics underneath.

SearchResult.Content and .Description became *string and always arrive
null; the server's highlighted excerpt now lands in PlainTextContent and
PlainTextDescription. The workspace search view reads those instead, so
excerpts keep rendering rather than silently going blank.

Also rides along from the SDK: HTTP 400 now maps to the validation error
code rather than api_error, which moves the exit code for a 400 from 7 to
9. 422 was already validation, so the two agree now.
Notes the new EverythingService aggregate family as present in the SDK but
not yet surfaced by any CLI command, so it stays untracked and out of the
parity totals rather than being counted as covered. Also records the
transport and model changes that ride along with the bump.
Copilot AI review requested due to automatic review settings July 29, 2026 06:44
@github-actions github-actions Bot added commands CLI command implementations tui Terminal UI sdk SDK wrapper and provenance docs deps labels Jul 29, 2026
@github-actions github-actions Bot added the bug Something isn't working label Jul 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR bumps the Basecamp Go SDK dependency to v0.10.0 and updates Basecamp CLI call sites to match the SDK’s breaking surface changes (notably pointer fields on card updates and updated search excerpt fields), while recording the new SDK/API provenance and updating API coverage notes.

Changes:

  • Update github.com/basecamp/basecamp-sdk/go dependency to v0.10.0 (go.mod/go.sum) and refresh SDK provenance.
  • Adapt card update call sites to the SDK’s *string UpdateCardRequest fields to preserve “only send what was asked for” semantics.
  • Update TUI search excerpt extraction to use PlainTextContent / PlainTextDescription per SDK change, and document the new coverage notes in API-COVERAGE.md.

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 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/version/sdk-provenance.json Records updated SDK version/revision and synced API revision.
internal/tui/workspace/views/search.go Switches TUI search excerpt source to the new plain-text excerpt fields.
internal/tui/workspace/views/detail.go Updates card due date/title edits to send pointer fields in UpdateCardRequest.
internal/commands/cards.go Updates cards update request construction to use pointer fields and preserve partial update behavior.
go.sum Updates dependency checksums for the bumped SDK version.
go.mod Pins the Basecamp SDK dependency to v0.10.0.
API-COVERAGE.md Documents SDK v0.10.0 behavior changes and notes EverythingService as present-but-unsurfaced.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/tui/workspace/views/search.go Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4c40a9c153

ℹ️ 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".

Comment thread API-COVERAGE.md Outdated
…vior

The absolute claim about nulls on the wire would go stale if the server
changed; what is durable is that the SDK made the fields nullable and moved
the excerpt to the plain-text variants.
Copilot AI review requested due to automatic review settings July 29, 2026 06:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

jeremy added 13 commits July 29, 2026 07:37
The v0.10.0 excerpt fields are named PlainText* but the SDK documents them as
HTML fragments: BC3 wraps each query match in <mark class="circled-text">.
Passing PlainTextDescription straight to the list rendered those tags
literally, since only the content fallback went through truncateExcerpt.

Select the source first, then normalize both branches through truncateExcerpt,
which strips markup. Tests cover both branches, the preference order, and the
no-excerpt case.
The SDK paragraph enumerated only part of the delta. It now covers the
BubbleUpURL spread to Recording, SearchResult, Todolist, and TodolistGroup
(always serialized on the latter two), the participant_ids provenance repin,
and the machine-output consequence: search serializes raw SDK structs for
--json/--agent/--md, so content and description now appear as explicit null
where the old empty strings were omitted, alongside the new plain_text_* and
bubble_up_url keys. Styled output is unchanged.

Adds a Known uncovered SDK surface section so the 17 unreached
EverythingService methods are stated outright rather than implied by their
absence from the totals.
The design was scattered across a PR description that said Everything was
deliberately unwired, an issue that called the command shape an open question,
and a release plan pinned to the previous SDK. None of them carried the
invariants the implementation has to satisfy.

Records the full 17-method matrix and the invariants: scope is selected before
scope-specific validation, --all-projects overrides configured scope and
conflicts with explicit scope, no accepted flag may be silently ignored,
--sort is rejected for grouped aggregates, the documented limit cap survives,
and machine output keeps grouping while styled output is flattened.

Also records the page-0 fetch cost as an accepted tradeoff rather than leaving
it implicit.
Two implementation attempts were reverted for the same defect — a flag accepted
and silently ignored — and both traced back to text that left room for it.

I2 contradicted itself: the general precedence said a configured project selects
project scope, but comments, boost, and checkins answers cannot produce a
listing from a project alone. Give the per-item groups their own truth table and
say plainly that ambient config is ignored there because it cannot scope the
operation.

I5's "the command's documented default" left eight implementers to reinterpret
it. Pin every default in a table, forbid new pagination flags where none exist,
and settle the grouped-payload questions: --limit counts inner items, TotalCount
counts groups so accountWideRespOpts must not be used for item notices, and an
explicit --page 0 is a usage error rather than a silent full-account crawl.

I4 states what was already true — sorting flags are reused, never added — so a
feed without --sort stays without one.

The one new flag pair, files list --kind/--person, is recorded as a deliberate
exception with its own rejection rule: the project-scoped path has no equivalent
filter, so passing either with a project in scope is a usage error.
The method matrix reaches eleven distinct todo and card endpoints, but the flags
that pick among them mostly do not exist: cards list has no --status,
--unassigned, --no-due-date, --not-now, or --overdue, and todos list has no
--unassigned or --no-due-date. Left unsaid, "no new flags" and the matrix
contradict each other, and an implementer resolves it by guessing.

List every added flag in one table and give the new ones the same scope rule the
files filters already have: account-wide only, so passing one with a project in
scope is a usage error rather than a silent no-op. The selectors are mutually
exclusive because no endpoint combines two.
accountWidePage narrows a user-supplied int to the int32 the endpoints take.
On 64-bit platforms that conversion silently wraps, so --page 4294967297 would
have become page 1 rather than an out-of-range page. gosec flags it as G115.

Clamp at MaxInt32. A page number that large has no results behind it either
way, but wrapping to a valid-looking page is the worse failure.
`forwards list` resolved a project from --project/--in, the root flag, or
config, and prompted for one when none of those answered. There is nothing to
prompt for: the account-wide forwards feed answers the same question across
every accessible project, so an empty scope lists that instead. --all-projects
overrides a configured project and conflicts with an explicit one, so identical
scripts do not change behavior with ambient config.

The feed takes a page number where 0 follows every page. This command's
project-scoped default is already "0 = all", so the account-wide default
follows every page too rather than quietly growing a cap. --all lands on the
same page 0, --page N returns exactly page N — the aggregate serves any
positive page, unlike the project-scoped inbox — and --limit truncates
client-side with a notice naming the full total.

Flags that cannot mean anything account-wide are rejected instead of ignored:
--inbox names a container inside one project, and an explicit --page 0, a
negative page, or a negative limit would each hand back something other than
what was asked for.
Boosts hang off a single recording, so `boost list` demanded an item ID and
fell back to prompting for a project it could not use anyway — a project alone
never produced a listing. The account-wide /boosts.json feed answers the
question the bare invocation was really asking, so `boost list` with no ID now
lists boosts from every accessible project, newest first, and a configured
project is ignored rather than turned into a prompt or an error: it could not
have scoped this listing either way. An explicitly named project still asks for
an ID, since naming a project is a statement of intent that only an item ID can
satisfy, and --all-projects pins the account-wide intent while conflicting with
both an ID and an explicit project.

The command has no pagination flags and gains none, so it stays on the first
page of the feed rather than growing a parallel flag surface. --event names an
event inside one item and has no account-wide equivalent, so it is rejected by
name instead of being quietly dropped.

Each boost nests the recording it sits on, which renders as an unreadable cell,
so styled output gets flat rows — project, booster, content, and the boosted
recording's title and type — while machine formats keep the raw payload.
A message board belongs to one project, so `messages list` has always
needed a project and prompted for one when it could not find it. The
account-wide messages feed answers the same question across every
accessible project, so listing that instead of prompting is both more
useful and the only reading of "list messages" that does not require the
caller to already know which project they meant.

Scope is chosen before it is validated. An explicit --project/--in wins,
whether it arrives after the group noun or from the root-level flag that
never sets Changed; a configured project comes next; with neither, the
account-wide feed answers. --all-projects pins that intent, so it
overrides a configured project and conflicts with an explicit one. The
project path's "only --page 1 is supported" rule stays on the project
branch, because the account-wide feed serves any positive page.

Every flag the command takes either changes the account-wide result or
says why it cannot. --message-board names one project's board and is
rejected by name. --sort/--reverse are honored: the feed is a flat
[]Recording, so the existing field set sorts it, and the sort runs
before the cap so a truncated listing is the top of the whole account
rather than the top of an arbitrary window. --limit and the default cap
of 100 collect one page at a time and stop once they have enough,
instead of crawling the account and discarding most of it. --all follows
every page. An explicit --page 0, a negative page, and a negative limit
are usage errors rather than a full-account crawl nobody asked for.

The feed needs no flattening: each recording carries its own bucket, and
the styled renderer already gives []Recording one row per item, the way
recordings list does.
Bare `files list` prompted for a project whenever none was configured, so the
one listing that could answer "what files are there?" across an account
insisted on being pointed at a single one first. The SDK's account-wide files
feed answers exactly that question, and it was adopted but unreachable.

With no project from a flag, the root flag, or config, the listing now runs
account-wide instead of prompting. `--all-projects` reaches it from a
configured project too, and conflicts with an explicit --project/--in rather
than silently picking a winner.

The feed carries filters the project-scoped path has no equivalent for, so
`--kind` and `--person` are account-wide only: passing either with a project in
scope is a usage error, as is `--vault`/`--folder` account-wide. A flag that
cannot change the result must say so rather than be dropped.

`EverythingFile` is an all-pointer superset over the upload, document, and
attachment variants, so styled output gets nil-checked flattened rows while
machine formats keep the raw payload. The listing has no pagination flags
project-scoped and gains none here; it follows every page.
`comments list` demanded an item ID, so the only way to see what people
had been saying lately was to already know which recording to ask about.
The account-wide comments feed answers that directly, so make the item
optional and route the bare invocation there.

Comments are per-item, which makes the usual project precedence a poor
fit: a project cannot narrow a comment listing at all. So an ambient
configured project is ignored rather than turned into an error — it is
not a scope this command could honor even if we wanted it to — while an
explicit --project asks for an item ID instead of silently listing
something the flag did not select. --all-projects states the account-wide
intent outright and conflicts with both an item ID and an explicit
project. --todolist is rejected by name for the same reason a project is:
it names a container inside one project.

Scope is chosen before it is validated. The item feed still permits only
page 1; the aggregate accepts any positive page, and --all maps onto the
page-0 "follow every page" the SDK takes. The default keeps the 100-comment
cap the item listing already documents — dropping the ID must not quietly
promote a bounded command into a full-account crawl — and reaches it by
walking positive pages until the cap is met rather than downloading the
whole account to throw most of it away. An explicit --page 0, a negative
page, and a negative limit are usage errors: --page 0 means "unset" to
Cobra but "every page" to the SDK, and accepting it would hand back a
crawl nobody asked for.

The payload is []Recording, which the styled renderer already takes as-is
from `recordings list`, so there is nothing to flatten.
`checkins answers` lists the children of one question, so a project on its
own could never produce a listing — without a question ID the command
prompted for a project and then still had nothing to ask for. The account-wide
checkins aggregate answers the question the user was actually asking: every
check-in answer across every project they can reach, newest first.

The question ID becomes optional. With one, nothing changes. Without one, an
explicit --project/--in is a usage error asking for an ID, because a project
cannot name a question; a merely configured project is ignored rather than
turned into an error, since ambient config should not decide whether this
command works at all. --all-projects states the intent outright and conflicts
with either an explicit project or a question ID.

Scope is chosen before the pagination flags are validated: the aggregate
serves any positive page, while the per-question listing only serves page 1.
The default stays what it already was project-scoped — every answer — so the
account-wide path follows every page unless --page narrows it. --limit
truncates client-side and says so. --by and --questionnaire name a person
filter and a container that the aggregate has no equivalent for, so both are
rejected with a pointer at the per-question form rather than accepted and
dropped on the floor.

The payload is []Recording, the same shape `recordings list` already hands the
styled renderer, so it needs no format-dependent flattening.
The truth table listed five of the six input combinations. An omitted ID
alongside both an explicit project and --all-projects had no row, leaving the
one case where two rules could each claim it undefined. The general I2 rule
decides it: --all-projects conflicts with an explicit project regardless of
whether an ID is present.
jeremy added 4 commits July 29, 2026 08:22
The matrix carried a section whose whole purpose was to state that 17 adopted
SDK methods reached no CLI command. They all reach one now, so the section
becomes a coverage table: every invocation, the method behind it, and the
payload shape it returns.

The rows stay out of the tracked totals on purpose. These are not new endpoints
in the matrix — each is the account-wide variant of a listing already counted,
reached through the owning group's existing leaf command.

Also drops the same 'always arrive null' absolute from the search-excerpt note
that the code comment shed: what is durable is that the SDK made the fields
nullable and moved the excerpt to the plain-text variants.
The skill told agents twice that project scope is mandatory and enumerated the
cross-project exceptions. Eight commands just joined that list, so the guidance
was wrong in the direction that matters: an agent reading it would reach for
recordings or reports when the command it already knows now answers the
question directly.

The drift check cannot catch this — it verifies that referenced flags exist,
not that the prose still describes what the CLI does.
The positional is optional now, so a bare invocation lists across every project
instead of erroring. The e2e test asserting 'ID required' was asserting the
behavior this change removes, and with a configured project it went to the
network rather than failing locally.

Replace it with the two cases the contract actually pins: an explicit project
without a question ID cannot select a listing, and an ID cannot be combined
with --all-projects. Both fail locally, so the suite stays offline.

Also drops the --by parameter that the account-wide path never read; it detects
the flag through Changed, which catches an explicitly blank value too.
Four reviewers went at the eight implementations looking specifically for the
defect that reverted the two prior attempts — a flag accepted and silently
ignored. Two live instances survived, and both are fixed here.

--kind "" fell through every check in files list, because the guards tested the
value rather than the flag. An explicitly empty value is still the user asking
for a filter. Detect it with Changed, like --person already effectively did.

The root --todolist is a global, so it reaches all eight listings whether or not
the command has any notion of a todolist. Only comments and todos rejected it;
the other six accepted and dropped it. I3 names it, so all eight reject it now,
through one shared helper.

--questionnaire was threaded into the check-in answers command by this work and
wired to only one of the two branches: account-wide rejected it, the item path
kept dropping it. A question ID already names its questionnaire, so say that.

Beyond the flag layer, the grouped todo and card listings lost data outright in
three output modes. Branching on EffectiveFormat fed flat rows to styled output
only, leaving --md to render a heading and no table, --ids to find no ids (a
project group has none), and --count to report the number of projects while the
summary one line above reported todos. Supplying the flat rows through
WithDisplayData feeds all four consumers and still leaves --json and --agent the
raw grouping they want; the generic renderers and the two counters now honor it
the way the schema-aware presenters already did.

The recording feeds had the mirror-image problem: rendered as-is, the renderer
drops the nested bucket, so an account-wide listing gave no way to tell which
project a row came from. They flatten now, with the project named and Subject
preferred over the generic recording title — which also means --sort title on
messages orders by the column the user can actually see.

Smaller repairs: todos rejects an out-of-range --page instead of clamping it to
a page nobody asked for, reports the SDK's truncation signal instead of passing
a partial listing off as complete, and carries a hint on its conflict error like
the other seven. The shared summary singularizes, and its truncation notice
stops recommending --all to the two commands the contract denies that flag.

The contract's I6 table was the source of the recording-feed gap — it said
render as-is on the strength of a project-scoped precedent that does not
transfer. Corrected, along with the mechanism it now prescribes.
Copilot AI review requested due to automatic review settings July 29, 2026 15:43
@github-actions github-actions Bot added tests Tests (unit and e2e) skills Agent skills output Output formatting and presentation enhancement New feature or request and removed bug Something isn't working labels Jul 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/commands/accountwide.go:46

  • accountWidePage currently clamps page > math.MaxInt32 down to math.MaxInt32 (lines 43-45). Several new account-wide branches call this helper without their own upper-bound validation (e.g., messages/comments/forwards/checkins), so an out-of-range --page becomes a silently different request rather than an actionable usage error.

Consider making out-of-range pages fail consistently (either by changing accountWidePage to return (int32, error) and returning ErrUsage here, or by removing clamping and adding explicit page > math.MaxInt32 validation in every caller before converting to int32).

// accountWidePage maps a group's existing --page/--all pair onto the page
// number the account-wide endpoints take. Groups that only support page 1
// pass their validated page through unchanged.
func accountWidePage(page int, all bool) int32 {
	if all {
		return 0
	}
	if page < 1 {
		return 1
	}
	if page > math.MaxInt32 {
		return math.MaxInt32
	}
	return int32(page)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8224a2359

ℹ️ 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".

Comment thread internal/commands/boost.go Outdated
Comment thread internal/commands/files.go Outdated
Comment thread internal/commands/accountwide.go Outdated
vaults/folders and docs/documents are NewFilesCmd under different names, so they
share the list leaf. That was fine while the leaf only ever listed one project's
folder. It is not fine account-wide: the aggregate feed carries Uploads,
Documents, and Attachments and no folder variant at all, so `folders list
--all-projects` returned a listing containing none of the thing the command is
named for, and said nothing about it.

Each spelling now means what it says. The folder spellings refuse account-wide
and point at the project-scoped form and at `files list --all-projects`. The
document spellings pin --kind documents, which is a real answer the feed can
give, and reject an explicit --kind because the command name already chose one.
Bare `files list` is unchanged, and so is the project-scoped behavior of all
three.

Also finishes the DisplayData conversion the previous commit left half-done:
boost and files were still branching on EffectiveFormat, so --md handed the
generic renderer a nested EverythingBoost or an all-pointer EverythingFile and
it skipped the nested maps — dropping the project column from one and the
recording context from the other.

And the truncation notice now depends on what caused the shortfall. It told
anyone who passed --limit to use --all, which those commands reject as mutually
exclusive with --limit — an unactionable hint naming a combination the command
refuses.
Copilot AI review requested due to automatic review settings July 29, 2026 16:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/commands/accountwide.go:46

  • accountWidePage clamps page > math.MaxInt32 down to math.MaxInt32. That means a user can ask for (e.g.) --page 3000000000 on an account-wide listing (valid int on 64-bit) and silently get a different page than requested, which is a correctness issue (and differs from todos list, which explicitly errors when the page is out of range).

It’d be safer to treat page > math.MaxInt32 as an actionable usage error (or have accountWidePage return (int32, error) so callers can fail fast) rather than clamping to a different page.

// accountWidePage maps a group's existing --page/--all pair onto the page
// number the account-wide endpoints take. Groups that only support page 1
// pass their validated page through unchanged.
func accountWidePage(page int, all bool) int32 {
	if all {
		return 0
	}
	if page < 1 {
		return 1
	}
	if page > math.MaxInt32 {
		return math.MaxInt32
	}
	return int32(page)

internal/commands/forwards.go:233

  • On the account-wide branch, --limit is enforced only after calling Everything().Forwards(..., sdkPage) with sdkPage == 0 by default, which (per the surrounding comment) causes the SDK to follow the Link header across every page. For large accounts, basecamp forwards list --limit N will still download the full feed and then truncate locally, which can be unnecessarily slow and resource-heavy.

Consider fetching positive pages in a loop until the limit is met (similar to fetchAccountWideComments), and only using page 0 when the user explicitly asks for the complete listing (e.g. --all) or explicitly requests a specific --page.

	// 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 {
		sdkPage = accountWidePage(page, all)
	}

	result, err := app.Account().Everything().Forwards(cmd.Context(), sdkPage)
	if err != nil {
		return convertSDKError(err)
	}

	// --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]
	}

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4733cfeedf

ℹ️ 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".

Comment thread internal/commands/cards.go
todos and cards --overdue return items from every project, and their project
arrives in a nested bucket that both generic renderers skip by name. Handed the
raw slice, styled and Markdown output rendered rows that cannot be attributed to
a project at all — two identically-titled overdue todos are indistinguishable.

The overdue todo listing also carried WithEntity("todo"), whose schema renders a
task list. That format has no column for a project, so honoring it would have
lost the attribution a different way. It goes.

This is the third time the same reasoning error produced the same defect, so the
contract now states the rule rather than a payload-by-payload verdict: every
account-wide payload flattens, because render.go skips bucket by name, and
"renders fine project-scoped" is never the test — project-scoped output does not
need a project column.
Copilot AI review requested due to automatic review settings July 29, 2026 17:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/commands/accountwide.go:44

  • accountWidePage currently clamps page values larger than math.MaxInt32 down to MaxInt32. For user-provided --page, that means we silently serve a different page than requested, which violates the account-wide contract (no silently ignored/altered flags) and is inconsistent with todos list, which explicitly errors when --page is out of range. Consider changing this helper (and its callers) to return/propagate an ErrUsage when the user asks for a page > MaxInt32, rather than clamping.
	if page < 1 {
		return 1
	}
	if page > math.MaxInt32 {
		return math.MaxInt32

internal/commands/comment.go:203

  • This comment says there is “nothing to flatten”, but the implementation intentionally sets DisplayData via flattenAccountWideRecordings so human-oriented outputs keep a project column (the generic renderers drop nested bucket fields). Updating the comment would prevent future confusion/regressions.
// runCommentsListAccountWide lists every comment in the account, newest first.
// The payload is []Recording, which the styled renderer already handles as-is
// (`recordings list` hands it the same type), so there is nothing to flatten
// and no format branch to make.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f7f587b38b

ℹ️ 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".

Comment thread internal/commands/accountwide.go Outdated
accountWidePage clamped a page beyond int32 down to MaxInt32 and carried on, so
--page 2147483648 quietly became a request for a different page. That reached
messages, comments, checkins, and forwards, which relied on the helper for their
upper bound; todos and cards validated it themselves and errored, so the same
flag behaved two different ways depending on which listing you asked for.

The clamp was mine, added to silence a gosec integer-overflow warning. It traded
a lint finding for a contract violation: I3 forbids silently altering a flag as
much as silently dropping one. The helper returns an error now and the five
callers propagate it.

Also corrects two comments that still described []Recording as needing no
flattening. That stopped being true when the recording feeds started carrying a
project column, and a comment claiming otherwise invites the regression back.
Copilot AI review requested due to automatic review settings July 29, 2026 17:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.

Comment thread internal/commands/todos_test.go
@jeremy jeremy changed the title Bump SDK to v0.10.0 Bump SDK to v0.10.0 and wire its account-wide aggregates Jul 29, 2026
@jeremy
jeremy merged commit e1059fb into main Jul 29, 2026
28 checks passed
@jeremy
jeremy deleted the sdk-0.10.0 branch July 29, 2026 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands CLI command implementations deps docs enhancement New feature or request output Output formatting and presentation sdk SDK wrapper and provenance skills Agent skills tests Tests (unit and e2e) tui Terminal UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants