Repository navigation
Go: return an empty list, not nil, when a listing has no items - #990
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approved
The narrow initialization changes correct empty-result serialization without changing populated-result processing, and no blocking issues remain.
0 open findings
What changed in this PR
Fixes Go SDK empty-list serialization so consumers receive [] instead of null.
Changes:
- Initializes list results as non-nil empty slices across 31 service files.
- Adds reflective regression coverage for empty list results.
- Leaves request bodies, nested model fields, and generated code unchanged.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| go/pkg/basecamp/webhooks.go | Initializes empty webhook lists. |
| go/pkg/basecamp/vaults.go | Initializes empty vault, document, upload, and version lists. |
| go/pkg/basecamp/todos.go | Initializes empty todo lists. |
| go/pkg/basecamp/todolists.go | Initializes empty todolist lists. |
| go/pkg/basecamp/todolist_groups.go | Initializes empty group lists. |
| go/pkg/basecamp/timesheet.go | Initializes empty report entries. |
| go/pkg/basecamp/timeline.go | Initializes empty timeline events. |
| go/pkg/basecamp/templates.go | Initializes empty template lists. |
| go/pkg/basecamp/subtasks.go | Initializes empty subtask lists. |
| go/pkg/basecamp/search.go | Initializes empty search results. |
| go/pkg/basecamp/schedules.go | Initializes empty schedule entries. |
| go/pkg/basecamp/recordings.go | Initializes empty recording lists. |
| go/pkg/basecamp/projects.go | Initializes empty project lists. |
| go/pkg/basecamp/people.go | Initializes empty people lists. |
| go/pkg/basecamp/messages.go | Initializes empty message lists. |
| go/pkg/basecamp/message_types.go | Initializes empty message-type lists. |
| go/pkg/basecamp/list_results_empty_test.go | Adds reflective non-nil list checks and a coverage floor. |
| go/pkg/basecamp/lineup.go | Initializes empty marker lists. |
| go/pkg/basecamp/forwards.go | Initializes empty forward and reply lists. |
| go/pkg/basecamp/folders.go | Initializes empty folder lists. |
| go/pkg/basecamp/everything.go | Initializes empty aggregate listing results. |
| go/pkg/basecamp/events.go | Initializes empty event lists. |
| go/pkg/basecamp/drafts.go | Initializes empty draft lists. |
| go/pkg/basecamp/comments.go | Initializes empty comment lists. |
| go/pkg/basecamp/client_replies.go | Initializes empty client reply lists. |
| go/pkg/basecamp/client_correspondences.go | Initializes empty correspondence lists. |
| go/pkg/basecamp/client_approvals.go | Initializes empty approval lists. |
| go/pkg/basecamp/checkins.go | Initializes empty question, answer, people, and reminder lists. |
| go/pkg/basecamp/cards.go | Initializes empty card lists. |
| go/pkg/basecamp/campfires.go | Initializes empty Campfire, line, upload, and chatbot lists. |
| go/pkg/basecamp/boosts.go | Initializes empty boost lists. |
| go/pkg/basecamp/bookmarks.go | Initializes empty bookmark lists. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Open for a decision: four checks fail on d7c8053, none caused by this diff:
Fixing them means bumping the CI Go toolchain and rustls, which is separate dependency work outside this Go-only change. Someone needs to decide whether to land that first or merge over it. |
A list method that found nothing returned a nil slice, which encodes as JSON null. A consumer that serializes the result (the MCP server, the CLI's JSON output) reported an empty folder listing as null rather than []. Each listing now starts from an empty slice. A reflective test drives every AccountClient service method that returns a list against a server answering [] and asserts the list is non-nil, so a listing added later is covered without being named; a floor fails the test if the harness stops reaching them. Nested model fields are untouched: those that distinguish absent from empty (completion_subscribers) or omit when empty (assignees) do so on purpose.
d7c8053 to
7947e39
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Nothing open on 7947e39. |
Problem
Go service-layer list methods built their result with
var xs []Tand appended per item, so a listing with no items returned a nil slice. A nil slice encodes as JSONnull, so consumers that serialize SDK results reported an empty listing asnullrather than[]. Observed in production through basecamp-mcp-server:list_folderson a project with no folders answered with the bare textnull(and no structured result).Change
xs := []T{}), across 31 service files (53 sites). Behavior is otherwise unchanged; the populated path appends exactly as before.list_results_empty_test.godrives everyAccountClientservice method that returns a list (a slice, or a pointer to a struct carrying one) against a server that answers[], and asserts the list is non-nil. It enumerates by reflection, so a list method added later is covered without anyone naming it. A floor (50; 70 are exercised today) fails the test if the harness stops reaching them. Againstmainthe test reports 62 nil lists; with this change, none.Not changed, deliberately
Todo.CompletionSubscribers) or omit when empty (Todo.Assignees,omitempty) do so on purpose.BucketIDs,Types,Days, ...): turning a nil into[]there would change request bodies.json.Unmarshalthe body directly (gauges, my assignments, notifications, everything files):[]already decodes to an empty, non-nil slice.[]to an empty array.No spec, OpenAPI or generated-code change.
make go-check go-check-drift go-check-generated-drift go-check-wrapper-driftpasses.Consumer: basecamp/basecamp-mcp-server will bump to the release carrying this.
Overlap
#991 (fix-pass-api-semantics) touches
go/pkg/basecamp/cards.go,checkins.go,lineup.goandprojects.gotoo. Both are rebased on main independently and merge with each other without a textual conflict; the merged tree passesgo build,go vetandgo test ./.... Whichever lands second needs a small rebase onto the new main.Summary by cubic
Fixes the Go SDK's list methods returning a nil slice — which encodes as JSON
null— when a listing has no items, so empty listings now serialize as[]instead ofnull. This was observed in production throughbasecamp-mcp-server, wherelist_folderson a project with no folders answered barenull.var xs []Ttoxs := []T{}.AccountClientservice method returning a list against a server answering[], asserting the result is non-nil; a floor keeps the harness honest as new list methods are added.basecamp/basecamp-mcp-serverwill bump to the release carrying this.Written for commit 7947e39. Summary will update on new commits.