Adopt basecamp-sdk v0.12.0 and cover the surface it opened up - #611
Merged
Conversation
Eleven Everything() aggregate methods gained a trailing *EverythingTaskFilters parameter: the nine paginated todo and card selectors, plus the two unpaginated overdue endpoints. Pass nil at every call site, so this bump is behavior-preserving — no test changes, and the account-wide listings issue exactly the requests they did before. The filters themselves are the point of the signature change, but threading real values through belongs with the flags that produce them. This commit is the mechanical half. SDK v0.12.0 is 7e2925d25078; the API provenance moves to bc3 d0edc1283b23.
Both are /my/ listings: private to the authenticated user, spanning every project, with no project to scope them to and so no --project flag. They are bounded like the account-wide listings, and for the same reason. Their List methods take a page where 0 means "follow the Link header across every page", so a default of "fetch page 0, then trim to --limit" would re-import the fetch-everything-then-truncate defect PR #590 spent a release removing. Instead both reuse accountWideCollect: the default walks positive pages to a cap of 100, --limit walks to N, --page N is exactly one request, and --all is the only path that reaches page 0. Both listings flatten their own display rows. The generic renderer skips nested objects during column detection, so a generic render of a Bookmark shows an id and a timestamp while dropping the bookmarked recording entirely, and a Draft loses the project it belongs to. A draft's nil parent and nil scheduled time are display states rather than gaps, and render as "project root" and "not scheduled" instead of blanks. bookmarks check reports its answer rather than signalling it through the exit code: both true and false exit 0. Exit codes here classify failures, so spending a nonzero code on "not bookmarked" would make a successful answer indistinguishable from a real error. add and remove are both idempotent server-side, which the help text says rather than making callers check first. The pagination combination rules move into a shared validator. The eight existing listings keep their inline copies — rewriting working call sites to prove a helper is a bigger diff than the helper earns.
Both are singletons in their own way, and neither fits the list/show/create
shape the other groups use.
notes is one private scratchpad per person, addressed by no id at all —
hence show/set rather than list/create/update. The note record does not
exist until the first write, so a fresh account gets a nil id and empty
content back. That is an empty note, not a missing one, and it renders as
empty rather than failing: every account starts there.
set writes through richtext.MarkdownToHTML like every other
content-writing command, because the field is rich text (my_notes.md:
content | HTML). Passing the raw string would store escaped markup.
Content comes from an argument, --file, or piped stdin; naming two sources
is a usage error rather than a silent precedence rule, and empty content is
refused outright, since set replaces the whole note and a mistyped path
would otherwise erase it. Attachments are out of scope — this writes the
body only.
calendars has no index endpoint, so there is no 'calendars list' to add and
the discovery path is a pasted URL, which extractID already handles.
Its color is validated client-side, which is a requirement rather than a
courtesy. At v0.12.0 the SDK's parseErrorBody reads only
error/error_description, so a 422 carrying {"errors":{"color":[...]}}
arrives as a bare "validation error" naming neither the field, the value,
nor the alternatives. Checking the eleven colors before the request is what
makes the failure actionable, and the tests assert no request is issued.
prioritize, deprioritize and reorder manage the Up Next list. The listing gains priority_recording_id in its display rows, and that is not a separate nicety — it is what makes two of the three verbs usable. Which id to send depends on what is being addressed, and there are three cases rather than two: a to-do, or a card itself the entry's own id a step not yet prioritized the step's id, from the card's children a step already prioritized the entry's priority_recording_id The third is the one that bites. Once a step is prioritized the listing normalizes it under its parent card, so the entry's top-level id is the card's and only priority_recording_id names the step. That value appears in no URL and in no other command's output, so without it deprioritize and reorder have no way to name a card-step target — and the failure is silent, since the server answers 204 whether or not anything matched. All three verbs carry the rule in their help text, including the sharp edge that two prioritized steps on one card collapse to a single addressable entry. reorder is 1-based and refuses a position it cannot honor rather than clamping: serving a different slot than the one asked for would move the item somewhere the caller did not choose. The SDK deliberately never retries it, so a transient failure surfaces as a plain error rather than being replayed into a different position.
todos create --loose creates directly on the project's to-do set, outside any list. The flag is not spelled --todoset because that name is taken and means something else: which to-do set, versus no list at all. It resolves the set with ensureTodoset and skips todolist resolution entirely — no prompt, no config fallback, no name lookup — and refuses --list, since the two flags ask for opposite things. Creates are not idempotent and the SDK does not retry them, so a transient failure stays a failure rather than risking a duplicate. checkins gains pause/resume, notify, answerers, and an account-wide reminders feed. notify's two settings are tri-state, and the flags are in pairs for that reason: an unpassed flag stays out of the request so the server leaves that setting alone, while an explicit --no-... sends false. A single bool cannot distinguish "off" from "not mentioned" and would silently overwrite a setting nobody named. Naming no setting at all is refused rather than sent as an empty update. Neither answerers nor reminders registers --page. Both option structs document that the page number is not honored, so the flag would accept a value it could not act on — the defect the pagination contract exists to prevent. --limit is a real SDK-side bound and stays. A test asserts the absence, so a later "consistency" pass cannot add it back by reflex. The reminder feed flattens its rows: it nests the question, and the generic renderer skips nested objects, so a generic render would say when something is due without saying what or where.
my_bookmarks, drafts, my_notes, calendars and question_reminders were the five BC5 sections bc-api#410 introduced and this matrix never tracked. They are now tracked and implemented, so the sentence saying they remain outside the matrix is gone rather than softened. Two counts were wrong in a way worth naming. The questions row claimed 5 endpoints while listing four actions, and the section also carries pause, resume, notification settings and answerers — so the 100%-of-tracked claim was resting on an undercount of the very section it counted. It now reads 8. And my_assignments was 3, missing the Up Next verbs. card_table_columns gained Subscribe/Unsubscribe in the SDK, which the CLI deliberately does not ship. 'cards column watch|unwatch' already performs the same action through the generic recording-subscription endpoint and returns the subscription details the specific endpoint does not, so the row records the alternate transport instead of the CLI growing a second spelling for one action. ACCOUNT-WIDE-LISTINGS.md picks up bookmarks and drafts as personal-feed rows. They are not EverythingService methods and have no --all-projects, but their List methods take the same page-0-means-every-page parameter, so they are exposed to the same trap and follow the same contract. Recording them is what gives the next /my/ feed a precedent to copy. checkins reminders is noted as sitting outside the table: its options struct honors no page number, so it takes --limit and no --page. SKILL.md gains the new groups in its frontmatter, triggers, Quick Reference and prose — including the Up Next id rule, which is the thing an agent is most likely to get wrong, since a wrong id there returns a successful 204 and changes nothing.
github-actions
Bot
added
commands
Member
Author
CI coverage caveat for this stacked PR
Substituted verification:
Flagging it so the checks list is not read as more than it is. |
![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: 05ae244291
ℹ️ 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".
`notes set` could silently discard piped content. notesContent checked stdin only after an argument and --file had been ruled out, so `generate | basecamp notes set --file fallback.md` overwrote the note from the file and threw the generated body away. In the one command that replaces everything, that is the exact failure the function's own doc comment claims it prevents. All three sources are now detected before any is chosen; naming two is a usage error. An empty pipe stays a non-source, so a redirected-but-empty stdin does not break a valid --file call. `checkins reminders` silently ignored explicit scope. --project, --in and --questionnaire are persistent flags on the checkins parent, so cobra accepts them on a subcommand whose handler always calls the account-wide endpoint. The caller believed the request was scoped and got every project's reminders back with nothing saying otherwise. All three are now rejected before the request. Correct the 400 exit-code claim in API-COVERAGE.md. It said a 400's exit code moves from 7 to 9. It does not. `internal/output` defines no `validation` mapping and clioutput defaults an unrecognised code to ExitAPI, so a 400 still exits 7 — only its JSON `code` changed `api_error` to `validation`. Exit 9 is not reachable from the CLI.
jeremy
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4577db3728
ℹ️ 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".
--ids-only prints a row's `id` field and nothing else, so `id` decides what a piped pipeline actually addresses. Two of the new listings got it wrong in opposite directions. assignments list exposed the parent card as `id` while the prioritized step lived only in priority_recording_id. So `assignments list --ids-only | xargs -n1 basecamp assignments deprioritize` addressed the card, and deprioritize answers 204 whether or not anything matched — the pipeline reported success and changed nothing. The prioritized entry now enumerates as the value the Up Next verbs take, with the parent card kept as recording_id. checkins reminders had the inverse problem: it exposed question_id but no `id` at all, so `checkins reminders --ids-only` printed nothing. The question id is the actionable one — answering takes `checkins answer <question-id>` — so it becomes `id`, and the reminder's own id is kept as reminder_id. Both are covered by tests that render through the ids-only writer rather than only asserting on the row map, since the row map was never the thing that was broken. Also document the note-clearing gap rather than leaving it implicit: `notes set` refuses empty content in every form, which means the CLI cannot clear the note at all. An explicit --clear would close that without weakening the guard, and is deliberately deferred to its own change — a destructive verb should not ride along on a bump.
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.
Supersedes #610, which is closed. This is now one complete SDK landing unit:
the bump and the coverage that documents it, targeting
main.#610 split the bump into its own PR. Review correctly rejected that as an
invalid landing unit — at
4e232e9e,go.modpinned v0.12.0 whileAPI-COVERAGE.mdstill said v0.11.0 and none of the 20 new methods werecovered, contrary to AGENTS.md's completeness bar. Merging the two avoids that
without rewriting any commits.
Contents
4e232e9enilat the 11 changedEverything()call sites +vendorHash746201f2bookmarksanddrafts— the two personal feeds1d549e5enotesandcalendarsc2de7f67bb89073atodosandcheckins05ae24424577db37Every new SDK service method gets the full treatment: command file, catalog
entry, registration in
root.goandcommands_test.go, API-COVERAGE.md row.Defects review found, now fixed (
4577db37)notes setcould silently discard piped content.notesContentcheckedstdin only after an argument and
--filehad been ruled out, sogenerate | basecamp notes set --file fallback.mdoverwrote the note from thefile and threw the generated body away — in the one command that replaces
everything, and against the function's own doc comment. All three sources are
now detected before any is chosen; naming two is a usage error. An empty pipe
stays a non-source so a redirected-but-empty stdin does not break a valid
--filecall.checkins reminderssilently ignored explicit scope.--project,--inand
--questionnaireare persistent flags on thecheckinsparent, so cobraaccepted them on a subcommand whose handler always calls the account-wide
endpoint. The caller believed the request was scoped and got every project's
reminders back with nothing saying otherwise. All three are now rejected before
the request.
The 400 exit-code claim in API-COVERAGE.md was wrong. It said a 400's exit
code moves from 7 to 9. It does not:
internal/outputdefines novalidationmapping and
clioutputdefaults an unrecognised code toExitAPI, so a 400still exits 7 — only its JSON
codechangedapi_error→validation.Exit 9 is not reachable from the CLI. This correction had been established
before and never reached the repository.
Both behavioural fixes have no-request tests (pipe+argument, pipe+file, and each
of the three scope flags), and both were mutation-checked — reverting either
guard fails exactly its own test and nothing else.
Verification
bin/ciexit 0 at this head.main, so the full remote suite runs on this PR — including theNix build that gates
vendorHash. That was not true of the stackedarrangement:
test.ymlispull_request: branches: [main], so Adopt basecamp-sdk v0.12.0 and cover the surface it opened up #611 and Filter the account-wide task listings by assignee and due date #612previously ran only
labeland the sensitive-change gate.notesrenders throughrichtext.MarkdownToHTML. Rendering one corpus (headings, nested lists,tables, fenced code, footnotes, emoji, raw HTML, hard breaks, entities) under
both versions gives byte-identical output. Live
notes set --file→notes showround-trip against production also clean; test note restoredbyte-for-byte.
Backups:
backup/sdk-0.12.0-pre-rebase,backup/bc5-command-surface-pre-rebase.