Validate item before person picker in assign/unassign#279
Merged
Conversation
When assigning with an invalid ID, the user would go through the
interactive person picker only to hit a "resource not found" error
afterward. Now the item is fetched first, surfacing a friendly
type-specific error ("to-do #123 not found", "card #456 not found",
"step #789 not found") before any interactive prompts.
There was a problem hiding this comment.
Pull request overview
This PR updates the assign/unassign CLI flows to validate that the target item (to-do/card/step) exists before prompting the user with the interactive person picker, so invalid IDs fail fast with a friendlier error.
Changes:
- Resolve project ID first, then fetch/validate the target item before resolving/prompting for the assignee.
- Introduce
validateItem+itemNotFoundOrErrorto centralize “fetch + friendly not-found” handling and avoid duplicate API calls. - Refactor assign/unassign helpers to accept the already-fetched item (to-do/card/step) rather than re-fetching internally.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Replace validateItem/any with typed validateTodo/validateCard/validateStep functions, eliminating unchecked type assertions and nolint suppressions - Fix bug where getStep's convertSDKError call prevented step 404s from producing the friendly "step not found" error - Add tests for notFoundOrConvert covering all three item types and non-404 passthrough
github-actions
bot
added
tests
The validate-before-picker change moved item lookup before resolveAssignee, so non-interactive callers missing --to/--from now hit a network call before the cheap ErrUsageHint. Add an early guard after resolveProjectID that short-circuits with the usage hint in non-interactive mode, preserving the interactive person-picker flow unchanged.
The guard tests relied on os.Stdout not being a TTY to make IsInteractive() return false. Under a PTY (e.g. script(1)), the tests took the interactive path and failed. Set Flags.JSON in the helper to make the non-interactive state explicit.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/assign_test.go">
<violation number="1" location="internal/commands/assign_test.go:255">
P3: The `t *testing.T` field on `assignGuardTransport` is stored but never read. If `t` was intended for calling `t.Fatal()` inside `RoundTrip`, it's not being used; if it's not needed, remove it to avoid confusion.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The field was left over from an earlier iteration that used t.Fatalf; the transport now returns an error instead. Update comment and helper doc to match.
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.
Summary
assignandunassignacross all three item types (to-do, card, step)Summary by cubic
Validate the item exists before opening the person picker in
assign/unassign, so invalid IDs fail fast with clear not-found errors. Preserve non-interactive behavior by returning usage hints before any network calls; applies to to-dos, cards, and steps.Bug Fixes
--to/--fromshort-circuit with a usage hint before any network calls.Refactors
validateTodo/validateCard/validateStepand centralized 404 handling vianotFoundOrConvert; helpers accept the fetched item to avoid extra API calls.resolveAssignee.notFoundOrConvertand the non-interactive guard; force non-interactive mode to avoid TTY flakiness; cleaned up test transport comments and removed an unused field.Written for commit e16d647. Summary will update on new commits.