Skip to content

Fix --assignee filter ignored with --list - #369

Merged
jeremy merged 2 commits into
mainfrom
humble-bronze
Mar 24, 2026
Merged

Fix --assignee filter ignored with --list#369
jeremy merged 2 commits into
mainfrom
humble-bronze

Conversation

@jeremy

@jeremyjeremy commented Mar 24, 2026

Copy link
Copy Markdown
Member

Summary

  • --assignee was silently dropped when combined with --list because listTodosInList never received the flag
  • Forces unlimited fetch when assignee filtering is active so client-side filtering doesn't miss matches beyond the default SDK cap
  • Applies --limit after filtering, not before

Closes#285

Test plan

  • TestTodosListInListFiltersByAssignee — direct + group todos filtered by "me"
  • TestTodosListInListAssigneeNumericIDIncludesGroupTodos — numeric ID resolves across groups
  • TestTodosListInListAssigneeNoMatchReturnsEmpty — unknown assignee returns empty
  • TestTodosListInListAssigneeLimitAppliedAfterFilter--limit caps post-filter (fixture's first todo is a non-match)
  • TestTodosListInListAssigneeNoGroupsFilters — no-groups fast path filtering
  • TestTodosListInListAssigneeNoGroupsLimitAfterFilter — no-groups path limit-after-filter

The --assignee flag was silently dropped when combined with --list
because the dispatch path passed it to listTodosInList which didn't
accept or apply it. Also forces an unlimited fetch when assignee
filtering is active so client-side filtering doesn't miss matches
beyond the default SDK cap, with --limit applied after filtering.
Closes#285
CopilotAI review requested due to automatic review settings March 24, 2026 19:19
@github-actionsgithub-actionsBot added commands CLI command implementations tests Tests (unit and e2e) bug Something isn't working labels Mar 24, 2026

@chatgpt-codex-connectorchatgpt-codex-connectorBot 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:d1310b505c

ℹ️ 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 threadinternal/commands/todos.go

CopilotAI 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 fixes basecamp todos list --list … --assignee … silently ignoring the assignee filter by wiring the flag through to the single-todolist code path and applying client-side assignee filtering (including group-nested todos).

Changes:

  • Pass --assignee into listTodosInList and filter returned todos by assignee client-side.
  • Adjust fetch/limit behavior to avoid missing assignee matches beyond the SDK default cap.
  • Add focused unit tests covering assignee filtering and “limit after filter” behavior for both grouped and no-groups paths.

Reviewed changes

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

FileDescription
internal/commands/todos.goWires --assignee into the --list path and adds client-side filtering + limit handling changes.
internal/commands/todos_test.goAdds fixtures/transports and unit tests to validate assignee filtering (including group todos) and post-filter limiting.

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.

Comments suppressed due to low confidence (1)

internal/commands/todos.go:515

  • With assignee != "" you set sdkLimit = -1, but when the user does not pass --all or an explicit --limit (i.e. limit == 0), the result set becomes uncapped (default 100 no longer applies). This can unexpectedly return very large outputs and also prevents truncation notices from working as before. Consider applying the default cap (100) to the post-filter results when !all && limit == 0, while keeping totalCount as the full filtered count for notices.
	// When assignee filtering is active, fetch all so client-side filtering
// doesn't miss matches beyond the default cap.
sdkLimit := 0 // SDK default
if all || assignee != "" {
sdkLimit = -1
} else if limit > 0 {
sdkLimit = limit
}

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

Comment threadinternal/commands/todos.go

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/todos.go">
<violation number="1" location="internal/commands/todos.go:553">
P2: When `--assignee` is used without `--limit`/`--all`, the default 100-item cap is no longer applied, so this path can return unbounded results.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment threadinternal/commands/todos.go
@jeremy
jeremy merged commit 994b977 into mainMar 24, 2026
27 checks passed
@jeremy
jeremy deleted the humble-bronze branch March 24, 2026 19:42
jeremy added a commit that referenced this pull request Mar 25, 2026
Cherry-picked fixes from pending main PRs:
- Fix --assignee filter ignored with --list (#369)
- Fix chat post JSON output contract and upload error handling (#365)
- Fix schedule update silently dropping --attach without --description (#364)
- Fix blank-line-after-list suppressing paragraph break (#363)
- Fix campfire messages --limit being ignored (#367)
jeremy added a commit that referenced this pull request Mar 25, 2026
jeremy added a commit that referenced this pull request Mar 25, 2026
jeremy added a commit that referenced this pull request Mar 25, 2026
Also fixes: chat upload error now wraps converted SDK error instead of
raw error; schedule update --attach without --description now fetches
current description to append rather than replace.
jeremy added a commit that referenced this pull request Mar 25, 2026
Also fixes: chat upload error now wraps converted SDK error instead of
raw error; schedule update --attach without --description now fetches
current description to append rather than replace.
jeremy added a commit that referenced this pull request Mar 25, 2026
…accounts (#373)
* Bump basecamp-sdk to v0.7.1
Fixes Person.Id deserialization (normalizeJSON pre-unmarshal for
notification responses) and Account.Logo type (string → AccountLogo
object). Adapts two breaking changes: Cards().Move() and
Todos().Reposition() now take optional params.
Adds AVIF/HEIC to richtext MIME extension map for logo upload support.
* Merge upstream fixes (#363, #364, #365, #367, #369)
Also fixes: chat upload error now wraps converted SDK error instead of
raw error; schedule update --attach without --description now fetches
current description to append rather than replace.
* Add gauges command for project progress tracking
Subcommands: list (account-wide), needles/needle (project-scoped),
create/update/delete, enable/disable. Create validates --position
(0-100), --color, --notify (requires --subscriptions for custom mode).
* Add assignments command for viewing my assignments
Subcommands: list, completed, due (with scope filter). Shortcut
pattern — bare invocation shows priorities + non-priorities.
* Add notifications command, extend accounts, register all new commands
Notifications: list (paginated with dynamic next-page breadcrumb) and
read (resolves IDs to SGIDs from --page). Accounts: show, update, logo
upload (5MB + image type validation) and remove. Registers all new
commands in root.go, catalog, and test harness.
* Update docs and surface for v0.7.1 commands
API-COVERAGE.md: 42 sections, 155 endpoints, SDK v0.7.1.
SKILL.md: gauges, assignments, notifications, accounts sections.
.surface: 148 commands, 422 flags.
jeremy added a commit that referenced this pull request Jul 19, 2026
Pins github.com/basecamp/basecamp-sdk/go at
v0.7.4-0.20260718061625-d15f023f64dc (basecamp/basecamp-sdk#375), which
makes TodosService.Update merge-safe (GET -> overlay -> full PUT), adds
TodosService.Edit/Replace, and round-trips completion subscribers on the
public Todo model. Edit and Replace are alternate write semantics for the
already-covered todo update endpoint - no new CLI command, no
API-COVERAGE change.
Compatibility fallout absorbed here so this commit is green standalone:
- OAuth discovery (SDK #369): Config.AuthorizationEndpoint,
DeviceAuthorizationEndpoint, and RegistrationEndpoint became *string
and discovery now requires an issuer bound code-point-exact to the
base URL. Login asserts authorization-code capability immediately
after discovery - before scope handling and Dynamic Client
Registration - so a device-only server fails cleanly without DCR side
effects. buildAuthURL keeps a defensive nil check for other callers.
Test discovery fixtures gain the required issuer field.
- Credential-origin guard: the SDK now refuses to attach credentials to
requests targeting a different origin than BaseURL. Mock-transport
tests previously built clients with an empty BaseURL, producing
path-only URLs the guard rejects; they now set a test BaseURL.
- Merge-safe Update: sparse UpdateTodoRequest callers (assign batch,
card steps, TUI hub) silently become merge-safe - the SDK GETs full
state before the PUT, at the cost of one extra GET per update. Assign
batch fixtures gain the now-required content field, and the lazy
resolution test pins the full order: validation GET -> person
resolution -> SDK merge GET -> PUT.
- todos update: subscriber clearing now routes through the raw-PUT clear
branch. The sparse-Update branch cleared subscribers by omission, but
merge-safe Update always sends completion_subscriber_ids, so omission
no longer clears. Interim plumbing; collapsed onto Edit next.
Also fixes scripts/bump-sdk.sh reading api.synced_at: the SDK provenance
file exposes .bc3.date, not .bc3.synced_at, so the field landed empty.
jeremy added a commit that referenced this pull request Jul 19, 2026
…#543)
* Bump SDK to d15f023f: merge-safe Update, oauth pointer endpoints
Pins github.com/basecamp/basecamp-sdk/go at
v0.7.4-0.20260718061625-d15f023f64dc (basecamp/basecamp-sdk#375), which
makes TodosService.Update merge-safe (GET -> overlay -> full PUT), adds
TodosService.Edit/Replace, and round-trips completion subscribers on the
public Todo model. Edit and Replace are alternate write semantics for the
already-covered todo update endpoint - no new CLI command, no
API-COVERAGE change.
Compatibility fallout absorbed here so this commit is green standalone:
- OAuth discovery (SDK #369): Config.AuthorizationEndpoint,
DeviceAuthorizationEndpoint, and RegistrationEndpoint became *string
and discovery now requires an issuer bound code-point-exact to the
base URL. Login asserts authorization-code capability immediately
after discovery - before scope handling and Dynamic Client
Registration - so a device-only server fails cleanly without DCR side
effects. buildAuthURL keeps a defensive nil check for other callers.
Test discovery fixtures gain the required issuer field.
- Credential-origin guard: the SDK now refuses to attach credentials to
requests targeting a different origin than BaseURL. Mock-transport
tests previously built clients with an empty BaseURL, producing
path-only URLs the guard rejects; they now set a test BaseURL.
- Merge-safe Update: sparse UpdateTodoRequest callers (assign batch,
card steps, TUI hub) silently become merge-safe - the SDK GETs full
state before the PUT, at the cost of one extra GET per update. Assign
batch fixtures gain the now-required content field, and the lazy
resolution test pins the full order: validation GET -> person
resolution -> SDK merge GET -> PUT.
- todos update: subscriber clearing now routes through the raw-PUT clear
branch. The sparse-Update branch cleared subscribers by omission, but
merge-safe Update always sends completion_subscriber_ids, so omission
no longer clears. Interim plumbing; collapsed onto Edit next.
Also fixes scripts/bump-sdk.sh reading api.synced_at: the SDK provenance
file exposes .bc3.date, not .bc3.synced_at, so the field landed empty.
* Collapse todos update onto TodosService.Edit (#541)
Replaces the two divergent update branches from #540 - the raw-PUT clear
branch and the typed merge branch, each fed by a raw flat-route
preservation GET - with a single TodosService.Edit call: the SDK GETs
full state (completion subscribers included, per
basecamp/basecamp-sdk#375), hands the closure the complete writable
representation, and PUTs the whole thing back. Exactly one typed GET and
one typed PUT; the clear branch's post-PUT re-Get is gone.
Fail-closed posture split: field presence is now the server/SDK contract
(BC3 always serializes completion_subscribers and the SDK model pins
nil-vs-empty), so the missing-key guard is gone - but the CLI still
refuses to write back preserved subscriber IDs it can't trust; the Edit
closure aborts before the PUT on a missing or invalid id.
Accepted wire deltas, defined as clears by the Edit contract:
- description clear is now sent as "description": "" (was omitted)
- subscriber clear is now sent as "completion_subscriber_ids": [] (was
omitted)
Date clears remain omission (fullBody sends dates only when non-empty).
Behavior improvement: --due/--starts-on values are validated with
time.Parse before any HTTP. dateparse passes unrecognized input through
as-is, so garbage (or an impossible date like 2026-99-99) previously
reached the server; it now fails as a usage error with zero requests.
Image uploads for --description move inside the Edit closure, after
Edit's GET has confirmed the todo exists - a missing todo can no longer
orphan uploaded attachments.
* Normalize base URL before OAuth discovery
The bumped SDK binds the discovered issuer to the requested base URL
code-point exact (RFC 8414), so a configured BaseURL with a trailing
slash (https://host/) mismatched an issuer of https://host and
incorrectly fell back to Launchpad. Normalize with the existing
config.NormalizeBaseURL before Discover, matching credentialKey and the
authorization-endpoint builders.
Regression test drives discoverOAuth with BaseURL srv.URL + "/" and
asserts BC3 discovery succeeds without Launchpad fallback; verified to
fail without the fix.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't workingcommandsCLI command implementationstestsTests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--assignee flag ignored when listing todos with --list

2 participants

@jeremy