Register --in/--project flags on comments commands#249
Conversation
github-actions
bot
added
commands
![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 3 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/comment.go">
<violation number="1" location="internal/commands/comment.go:22">
P1: Dead code: The `project` variable is declared but never used in `NewCommentCmd`. The flags `--in`/`--project` are registered but have no effect since the underlying `newCommentsCreateCmd()` function doesn't access them.</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
Adds --in / --project flag registration to the comments command group and the top-level comment shortcut to match other command groups, and updates the CLI surface snapshot accordingly.
Changes:
- Register persistent
--in/--projectflags onNewCommentsCmd(inherited by subcommands). - Register local
--in/--projectflags onNewCommentCmd(shortcut command). - Add tests to ensure the new flags are accepted; update
.surfacesnapshot.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/commands/comment.go | Adds --in/--project flags to comments and comment. |
| internal/commands/comment_test.go | Adds tests asserting the new flags are accepted. |
| .surface | Records newly surfaced --in flags for comment/comments commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
force-pushed
the
fix/flag-alias-required
branch
from
f691522 to
f08648a
Compare
jeremy
force-pushed
the
fix/comments-project-flags
branch
from
8c0b7f2 to
65a2954
Compare
jeremy
force-pushed
the
fix/comments-project-flags
branch
from
65a2954 to
8251716
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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/names/resolver.go">
<violation number="1" location="internal/names/resolver.go:213">
P2: A `getPingable` failure turns a "not found" with helpful suggestions into a raw API error. Since this is a fallback path, consider degrading gracefully (e.g., treat the error as an empty pingable list) so the suggestions from the already-fetched `people` list are still shown.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
github-actions
bot
added
bug
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The comments group and comment shortcut were the only command groups missing --in/--project flags, breaking consistency with every other command group. Add persistent flags on NewCommentsCmd and local flags on NewCommentCmd (which is a top-level shortcut that doesn't inherit from the group's persistent flags).
ResolvePerson only searched /people.json, missing people findable via /people/pingable.json (clients, external collaborators). After the primary people list fails to match, fall back to the pingable endpoint for both email and name resolution before returning not-found. Propagate getPingable errors so transient API failures aren't masked as not-found. Deduplicate suggestions by ID to avoid showing the same name twice when a person appears in both lists.
If getPingable errors, the people list was already fetched successfully and can still provide suggestions. Swallow the error instead of replacing the "not found" with a raw API error.
jeremy
force-pushed
the
fix/comments-project-flags
branch
from
eef066d to
3ae5aec
Compare
Summary
commentsgroup andcommentshortcut were the only command groups missing--in/--projectflagsNewCommentsCmd(inherited by subcommands)NewCommentCmd(top-level shortcut that doesn't inherit from the group)Stacked on #248
Test plan
TestCommentShortcutAcceptsInFlag—comment --inacceptedTestCommentShortcutAcceptsProjectFlag—comment -pacceptedTestCommentsGroupAcceptsInFlag—comments list --inacceptedmake checkgreenSummary by cubic
Add
--in/--projectsupport to thecommentsgroup andcommentshortcut for project scoping. Improve person resolution by falling back to pingable users and deduplicating suggestions.--in/--project (-p)onNewCommentsCmd(persistent) andNewCommentCmd(local); update.surfaceand add tests.ResolvePersonto query/people/pingable.jsonon miss, prefer primary people, deduplicate suggestions, clear pingable cache on account changes, degrade gracefully if pingable fetch fails, and add tests.Written for commit 3ae5aec. Summary will update on new commits.