Conversation
github-actions
Bot
added
commands
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
SetUserIdentity can overwrite stored credential metadata when BASECAMP_TOKEN is active.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves identity rendering and persistence for ID-only BC3 authorization responses.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
Changes:
- Resolves account-scoped person details for
basecamp me. - Preserves stored identity fields when incoming values are empty.
- Improves ID-only login labels and regression coverage.
File summaries
| File | Description |
|---|---|
internal/commands/people.go |
Adds person resolution and identity fallback summaries. |
internal/commands/people_test.go |
Tests person resolution and fallback behavior. |
internal/commands/auth.go |
Improves ID-only identity labels. |
internal/commands/auth_identity_test.go |
Tests identity label variants. |
internal/auth/auth.go |
Treats empty identity fields as omissions. |
internal/auth/auth_test.go |
Tests preservation of stored fields. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 571254171b
ℹ️ 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.
🟡 Changes recommended
The environment-token guard breaks post-login identity persistence, while partial profile responses can discard known identity data.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc57f6679f
ℹ️ 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.
🟢 Approved
The focused changes match the stated behavior and include coverage for the relevant success, fallback, and credential-storage paths.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8205233d5
ℹ️ 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".
|
Review threads: 3 resolved (3 fixed, 0 declined with the reasoning in each thread). |
An in-house (bc3) token's /authorization.json reports only identity.id. `basecamp me` rendered that as " <> - 1 Basecamp account(s)" and then stored the empty email over the one the login had written, so `auth status` forgot who the credential belonged to. The --expect-identity mismatch message opened with the same blank name. SetUserEmail and SetUserIdentity now treat an empty value as an omission. `me` names an id-only identity as "identity <id>", and when an account is configured it looks the person up there — the same call a login verifies against — for the name and email, adding them to the JSON as data.person; a failed lookup keeps the identity rendering. The label every login message renders names an id-only identity by its ids alone.
`me` now persists the person it resolves, and under BASECAMP_TOKEN that person belongs to the environment token, not to whatever credential is stored for the profile. SetUserIdentity takes the rule SetUserEmail already had: a BASECAMP_TOKEN session writes nothing.
Guarding SetUserIdentity itself also silenced the write a login makes after storing its new credential, which runs under BASECAMP_TOKEN too and would have left that credential without its verified user. The guard now sits where the environment token's identity is learned: me skips the write, logins keep it. The person record fills gaps in the authorization document rather than replacing it: an email the document named survives a record that omits its own, and the merged email is what gets stored.
…empty The merge tested the record's fields for emptiness rather than the document's, so an identity with its canonical email but no name had that email replaced by the account person's. The document's values now stay as its own; the record fills the empty ones.
What
basecamp meunder an in-house (bc3) token:/authorization.jsonreports onlyidentity.id, and the summary read<> - 1 Basecamp account(s). It now saysidentity 28142355 - 1 Basecamp account(s), and when an account is configured it looks the person up there (/{account}/my/profile.json, the same call a login verifies against) and names them:Ada Lovelace <ada@example.com> - 1 Basecamp account(s). The person is added to the JSON asdata.person(id,name,email) when fetched; a failed lookup falls back to the identity rendering.SetUserEmail/SetUserIdentitytreat an empty value as an omission and leave the stored field alone.meused to callSetUserEmail("")on every run under an in-house token, wiping theuser_emailthe login had stored.--expect-identitymismatch (and every other placeloginIdentity.label()renders) names an id-only identity asidentity 28142355instead of(identity 28142355)with a blank name.The person lookup runs only when the authorization document lacks a name or email, so a Launchpad or BC5 login with a full identity makes no extra request.
Why
Smoke-test finding "authorization.json under in-house tokens omits identity name/email; CLI me and --expect-identity show blanks". Two visible symptoms — the blank
<>summary and an--expect-identitymismatch message that opened with an empty name — and one silent one: afterme,auth statusno longer knew who the credential belonged to, because the empty email had overwritten the stored one.Before / After
Before:
After:
Borrowed from Codex
Nothing here; Codex has no equivalent identity document.
Testing
internal/commands:TestMeIdentityOnlyResolvesThePerson(identity-only payload plus a person record: summary,data.person, storeduser_id/user_email),TestMeIdentityOnlyFallsBackToTheIdentity(person 404: identity summary, stored email untouched),TestLoginIdentityLabelWithoutNameOrEmail.internal/auth:TestSetUserIdentity_EmptyValuesAreOmissions.make fmt-check vet lint,go test -tags dev ./internal/...,make check-surface check-skill-drift, and the rest ofmake check(e2e, naming, lockstep, tidy).Summary by cubic
Fixes
basecamp meand--expect-identityfor in-house (bc3) tokens: id-only identities are now named, andmeno longer blanks a stored email or overwrites it with an environment token's identity.Bug Fixes
basecamp menow showsidentity <id>when the authorization document has no name or email, and resolves the person from the configured account when possible; the person record fills only fields the document left empty.SetUserEmailandSetUserIdentitytreat empty values as omissions, andmeskips the write underBASECAMP_TOKEN; logins still record who they verified.--expect-identitymismatch message now printsidentity <id>instead of a blank name with ids in brackets.Written for commit 2cd06d4. Summary will update on new commits.