Filter the account-wide task listings by assignee and due date - #612
Conversation
github-actions
Bot
added
commands
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: 642857c6d5
ℹ️ 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".
642857c to
5249c1c
Compare
|
Rebased onto the corrected #611 head ( The skill defectThree recipes labelled (cross-project) omitted
plus the narrative at the bottom of the assignments section, which repeated the This is not cosmetic. All four now carry Note on CI#611 now targets |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5249c1ced3
ℹ️ 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".
5249c1c to
7fe6753
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fe6753666
ℹ️ 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".
7fe6753 to
7cb71fc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/commands/accountwide.go:310
- This loop duplicates
resolveAssigneeFilterIDs(todos.go:860), which this same PR introduces to do exactly this: iterate the repeatable--assigneevalues and resolve each viaresolvePersonRoleIDs(ctx, app, assignee, "Assignee"). Reusing it keeps the two assignee-resolution paths in sync (both files are in packagecommands). Optional, but avoids a second copy of the same logic drifting from the first.
for _, assignee := range assignees {
// Each value may itself be a comma-separated list, so --assignee is
// repeatable and comma-separated both, matching how the other
// people-taking flags already behave.
ids, err := resolvePersonRoleIDs(ctx, app, assignee, "Assignee")
if err != nil {
return nil, err
}
filters.AssigneeIDs = append(filters.AssigneeIDs, ids...)
}
internal/commands/accountwide.go:195
- This comment points readers to
filterTodosByAssignees, but no such function or note exists in the codebase. The project-scoped assignee logic lives intodoMatchesAnyAssignee(andresolveAssigneeFilterIDs) intodos.go. The dangling reference will mislead anyone who tries to follow it.
This issue also appears on line 301 of the same file.
// Project-scoped --assignee is a different animal — see the note on
// filterTodosByAssignees.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cb71fcd86
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/commands/accountwide.go:311
- Optional (maintainability): this loop re-implements the exact assignee-resolution that
resolveAssigneeFilterIDs(intodos.go) already provides. Reusing that helper keeps a single place that resolves the repeatable/comma-separated--assigneeinto IDs. This is wire-safe: the SDK marksAssigneeIDsomitempty(seeinternal/tui/workspace/data/hub.go:1009), so a--due-only call still sends noassignee_ids[].
filters := &basecamp.EverythingTaskFilters{Due: due}
for _, assignee := range assignees {
// Each value may itself be a comma-separated list, so --assignee is
// repeatable and comma-separated both, matching how the other
// people-taking flags already behave.
ids, err := resolvePersonRoleIDs(ctx, app, assignee, "Assignee")
if err != nil {
return nil, err
}
filters.AssigneeIDs = append(filters.AssigneeIDs, ids...)
}
return filters, nil
7cb71fc to
4c07e98
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c07e986e1
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/commands/accountwide.go:195
- This comment points readers to "the note on
filterTodosByAssignees", but no such function exists anywhere in the package. The project-scoped--assigneebehavior it references is actually documented onlistTodosInList(see the "same spelling, very different cost" note intodos.go), with the any-match semantics intodoMatchesAnyAssignee. The dangling name will send future maintainers searching for a symbol that isn't there.
// Project-scoped --assignee is a different animal — see the note on
// filterTodosByAssignees.
This is the feature the v0.12.0 signature change exists for. --assignee and --due map onto EverythingTaskFilters, which 11 of the 16 aggregate methods now accept. --assignee is repeatable (and comma-separated within a value) and matches a task assigned to any of the named people. On todos it already existed project-scoped and was *rejected* account-wide, because the aggregates had no parameter to map it onto; that rejection is gone. On cards it did not exist at all — the group's agent note said so — and is added account-wide only. The same flag now means two different things by scope, and the help text says so rather than papering over it. Account-wide it is a real assignee_ids[] query parameter: the server narrows before it paginates, so the filter never turns the bounded walk into a full crawl. Project-scoped there is no server-side assignee parameter at all, so it filters client-side over a deliberately unlimited fetch. Same results, very different cost. The semantics are matched on purpose — the project-scoped filter now matches any of the named people, the way assignee_ids[] does. Note what the bounded walk does *not* promise: identical request counts. The cap counts items, so a narrower filter returns fewer per page and can need one more page to reach it. Soaked against account 2914079, unfiltered took 2 requests for 100 todos and --assignee took 3 for its 21. The test pins the invariant that actually matters — given identical page contents, filtering does not change the walk. Widening todos' --assignee from StringVar to StringArrayVar changes its .surface type line. TestSurfaceSnapshot compares whole lines, so the old one reads as a removal and is acknowledged in .surface-breaking. --due takes with, without, or overdue. These are category tokens rather than dates, so internal/dateparse is deliberately not involved. Two combinations are refused before any request, and the tests assert the transport stays untouched: --assignee with --unassigned is necessarily empty. The server builds that selector as todos_recordings.remaining.not_assigned over a relation the assignee filter has already narrowed, so nothing can satisfy both. Answered rather than refused, it would return zero rows that look like a real answer. --due with --overdue or --no-due-date asks two endpoints for one answer: each of those selects its own listing on the same axis --due narrows. The SDK's own caveat rides along into the help text — the filter matches a task's own assignees, and assignees on nested steps are not considered.
4c07e98 to
20506d7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/commands/accountwide.go:202
- This doc comment starts with
rejectEmptyTaskFilterValues, but the function it documents is namedvalidateTaskFilterValues. Every other function comment in this file leads with the function's own name (e.g.validateDueFilter,rejectAssigneeWithUnassigned,accountWideTaskFilters), so this stale name breaks the convention and would confuse anyone grepping for the referenced symbol. Update the comment to begin with the actual function name.
// rejectEmptyTaskFilterValues refuses an explicitly empty --due or --assignee.
internal/commands/accountwide.go:195
- This comment points readers to a symbol named
filterTodosByAssignees, but no such function exists anywhere in the codebase. The project-scoped assignee matching described here actually lives intodoMatchesAnyAssignee(andlistTodosInList) intodos.go. The dangling reference sends readers looking for something they will never find; point it at the real symbol instead.
// Project-scoped --assignee is a different animal — see the note on
// filterTodosByAssignees.
accountwide.go referred readers to filterTodosByAssignees, which is not a function in this repository — the project-scoped assignee note lives in listTodosInList. Point there, and say what the note actually says so the reference is useful without following it. The second is mine from #612: I renamed rejectEmptyTaskFilterValues to validateTaskFilterValues when it took on the --due token check, and left its doc comment naming the old function.
Adds two filters to the account-wide task listings:
--assignee— repeatable, server-side (assignee_ids[])--due—with/without/overdue, server-side (due=)Both ride the bounded walk reworked in #590 rather than reintroducing an
unbounded account-wide fetch.
This targets
maindirectly. It is an ordinary PR, not part of a stack:#611 merged as
ebeecc13(tree-identical to its reviewed headad39cd8c), and#610 was closed as superseded. Earlier revisions of this description said it was
stacked on
bc5-command-surfaceand received almost no CI — both are obsolete.Review findings, fixed
Explicit
--due=was treated as omission. Every check tested the flag'svalue, so an empty string was indistinguishable from never passing the flag:
it slipped past the project-scoped guard, built no account-wide filter, and
returned a full unfiltered listing to a caller who believed they had narrowed
it. Now rejected on presence (
Flags().Changed).--assignee=had themirror-image bug —
StringArrayVarappends the empty string, solen(...) > 0sent a filter naming nobody — and is rejected the same way.
Filter usage errors demanded an account. The
--duetoken check livedinside the account-wide path, which runs after
ensureAccount. With nothingconfigured,
todos list --due tomorrowhit account resolution first: aninteractive session got the account picker, a noninteractive one got
--account is required, and the actual mistake was never reported. Both thetoken and emptiness checks now run in
validateTaskFilterValuesbefore accountresolution — neither depends on the account or the scope.
TestTaskFilterUsageErrorsPrecedeAccountResolutionuses an app with noconfigured account and asserts on the message; asserting only "no requests
were made" would have passed before the fix, since account resolution fails
without issuing one.
The pagination claim was wrong in four places.
accountwide.go,todos.go,cards.goand the test file header all said filtering leaves the page/requestcount untouched. The production soak disproved that: 2 requests for 100
unfiltered todos vs 3 for
--assignee's 21, because the bounded walk's capcounts items, so a narrower result can need another page. Only the walk's
algorithm is unchanged — which is what the same-fixture test actually pins;
its body already said so correctly while the header contradicted it.
Four skill/annotation contradictions. The
todos listagent note still said--assigneerequires a project and redirected cross-project work toreports assigned, and claimed--assigneeworks on todos "not cards" — which this PRmakes false too. The skill separately said "Cards do NOT support
--assignee"in two places (true project-scoped, false account-wide now), and Smart Defaults
advertised
--due tomorrownatural dates — valid when setting a due date,rejected on a listing, where
--dueis a different flag takingwith|without|overdue. All corrected so the skill presents one contract.Also fixed earlier: the four
(cross-project)recipes that omitted--all-projects.accountWide := flags.allProjects || !projectKnown(...), so aconfigured default project counts as in scope — those recipes returned different
results depending on the reader's config.
Verification at
20506d700 failures.
bin/ciexit 0 on the same SHA.own test and nothing else.