Add pingable fallback to ResolvePerson#250
Merged
jeremy merged 1 commit intofix/comments-project-flagsfrom Mar 11, 2026
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a secondary “pingable people” lookup path to names.Resolver.ResolvePerson so the CLI can resolve people who aren’t returned by /people.json (e.g., clients/external collaborators), while caching the pingable list per session and clearing it on account switch.
Changes:
- Add
Resolver.pingablecache and clear it viaSetAccountIDandClearCache. - Extend
ResolvePersonto fall back to/people/pingable.jsonfor email/name resolution and include both lists in suggestions. - Add unit tests covering pingable fallback behavior and precedence of
/people.json.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/names/resolver.go | Adds pingable cache + fetcher and uses it as a fallback resolution source for people. |
| internal/names/resolver_test.go | Adds mock support for pingable cache and tests for pingable fallback scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/names/resolver.go">
<violation number="1" location="internal/names/resolver.go:239">
P2: `allPeople` can contain duplicates when a person appears in both `/people.json` and `/people/pingable.json` (which is common — pingable is a superset). Since `suggest` doesn't deduplicate, the "Did you mean" hint can show the same name twice.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 9dfb150813
ℹ️ 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".
jeremy
force-pushed
the
fix/resolver-pingable-fallback
branch
from
9dfb150 to
30d1e2b
Compare
jeremy
force-pushed
the
fix/comments-project-flags
branch
from
8c0b7f2 to
65a2954
Compare
jeremy
force-pushed
the
fix/resolver-pingable-fallback
branch
from
30d1e2b to
82eb829
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 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
jeremy
force-pushed
the
fix/resolver-pingable-fallback
branch
from
82eb829 to
f181cfc
Compare
jeremy
added a commit
that referenced
this pull request
Mar 11, 2026
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.
jeremy
added a commit
that referenced
this pull request
Mar 11, 2026
* Register --in/--project flags on comments and comment commands 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). * Add pingable fallback to ResolvePerson (#250) 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. * Degrade gracefully when pingable fallback fails 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.
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
ResolvePersononly searched/people.json, missing people findable via/people/pingable.json(clients, external collaborators)Stacked on #249
Test plan
TestResolverResolvePerson_PingableFallback_ByName— name match via pingableTestResolverResolvePerson_PingableFallback_ByEmail— email match via pingableTestResolverResolvePerson_PeoplePreferredOverPingable— people list takes priorityTestResolverResolvePerson_PingableFallback_NotFound— not-found in both listsmake checkgreenSummary by cubic
Fixes person resolution by falling back to
/people/pingable.jsonwhen/people.jsonhas no match, so clients and external collaborators resolve correctly. Adds per-session caching, deduped suggestions, and proper error propagation./people/pingable.jsonfor email and name when/people.jsonhas no match; prefer the main people list if both match.ClearCache().Written for commit f181cfc. Summary will update on new commits.