Skip to content

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

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

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

Conversation

@jeremy
Copy link
Copy Markdown
Member

@jeremy jeremy commented Mar 24, 2026

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
Copilot AI review requested due to automatic review settings March 24, 2026 19:19
@github-actions github-actions bot added commands CLI command implementations tests Tests (unit and e2e) bug Something isn't working labels Mar 24, 2026
Copy link
Copy Markdown

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

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

Copy link
Copy Markdown

Copilot AI left a comment

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.

File Description
internal/commands/todos.go Wires --assignee into the --list path and adds client-side filtering + limit handling changes.
internal/commands/todos_test.go Adds 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.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

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.

@jeremy jeremy merged commit 994b977 into main Mar 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working commands CLI command implementations tests Tests (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