Fix "me" resolution: stop conflating identity IDs with person IDs by jorgemanrubia · Pull Request #202 · basecamp/basecamp-cli

jorgemanrubia · 2026-03-06T04:33:32Z

Problem

ResolvePerson("me") resolved to the Launchpad identity ID (cross-account, from /authorization.json) instead of the account-scoped person ID (from /people.json). These are different ID spaces — using an identity ID on account-scoped endpoints like /people/{id} or /reports/todos/assigned/{id} produces 404s.

The bug had two parts:

basecamp me stored the identity ID as UserID via SetUserIdentity
ResolvePerson("me") fell back to this stored identity ID when email matching failed, silently returning the wrong-namespace ID

Fix

basecamp me now calls SetUserEmail instead of SetUserIdentity, storing only the email (the identity ID is never written to credentials)
ResolvePerson("me") matches the stored email against the account people list — the identity email and person email are the same, making email a reliable join key
The GetUserID fallback is removed — returning a wrong-namespace ID silently is worse than an error
Dead GetUserID / SetUserID methods removed (zero production callers after the fix)
Error messages differentiate between no stored email (re-login needed) and email not found in account (wrong account selected)

Other callsites (auth login, wizard, profile) already store the correct person ID via SetUserIdentity from /my/profile.json — those are unchanged.

Test plan

TestResolverResolvePerson_Me_IdentityID_NotLeaked — regression test: identity ID stored as UserID + email not in people list → returns error, NOT the identity ID. Verified this test fails when the old fallback is restored.
TestResolverResolvePerson_Me_MatchesByEmail — email matches person → returns person ID
TestResolverResolvePerson_Me_CaseInsensitiveEmail — case-insensitive matching works
TestResolverResolvePerson_Me_NoEmail — no email stored → auth error
TestResolverResolvePerson_Me_EmailOnly_NoUserID — new happy path (email only, no UserID)
TestSetUserEmail — stores email without touching UserID
make check passes (fmt, vet, lint, test, test-e2e)

Files changed

File	What
`internal/commands/people.go`	`basecamp me`: `SetUserIdentity` → `SetUserEmail`
`internal/names/resolver.go`	Remove `GetUserID` fallback; surface `getPeople` errors; differentiate error messages
`internal/auth/auth.go`	Add `SetUserEmail`, `SetStore`; remove `GetUserID`, `SetUserID`
`internal/auth/auth_test.go`	Replace `TestGetUserID`/`TestSetUserID` with `TestSetUserEmail`; check Save errors
`internal/names/resolver_test.go`	5 new "me" resolution tests including regression test

ResolvePerson("me") was returning the Launchpad identity ID instead of the account-scoped person ID. These are different ID spaces — the identity ID is cross-account (from /authorization.json) while the person ID is per-account (from /people.json). The identity ID never matches any person in the account, so every API call using the resolved ID 404'd.

jorgemanrubia · 2026-03-06T04:34:43Z

Closed since resolving me by paginating all the people is not a sustainable strategy. Discussing https://app.3.basecamp.com/2914079/buckets/46292715/todos/9651070678/

Copilot

Pull request overview

Fixes incorrect "me" person resolution by joining the stored Launchpad identity to an account person via email, and updates related commands to store/use that email so person-scoped endpoints stop 404’ing.

Changes:

Persist UserEmail in stored credentials and add GetUserEmail / SetUserIdentity(id, email) APIs.
Update "me" resolution to match stored email against the account people list (and switch people/projects fetching to paginated GetAll).
Route timeline watch me through ResolvePerson("me") and update login/wizard/profile/me flows to store email.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/names/resolver.go	Email-based `"me"` resolution; switch people/projects fetching to `GetAll` pagination.
internal/commands/wizard.go	Store user identity + email after fetching `/my/profile.json`.
internal/commands/timeline.go	Use `ResolvePerson("me")` instead of `GetUserID()` for personal timeline.
internal/commands/profile.go	Store user identity + email after profile creation login flow.
internal/commands/people.go	`basecamp me` stores identity ID + email via `SetUserIdentity`.
internal/commands/auth.go	`auth login` stores identity ID + email via `SetUserIdentity`.
internal/auth/keyring.go	Add `UserEmail` to `Credentials` for persistence.
internal/auth/auth_test.go	Add test coverage for `SetUserIdentity`.
internal/auth/auth.go	Implement `GetUserEmail()` and `SetUserIdentity(userID, email)`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/names/resolver.go

`basecamp me` fetches from /authorization.json which returns a cross-account identity ID, not an account-scoped person ID. Storing this identity ID as UserID causes 404s when later used with account-scoped endpoints. Add SetUserEmail to auth.Manager and use it in `basecamp me` so only the email is stored — the identity ID is no longer written to credentials.

The fallback returned a stored identity ID (cross-account) when email matching failed. Since all consumers need account-scoped person IDs, returning a wrong-namespace ID silently is worse than an error.

After the previous two commits, these have zero callers. Replace their tests with TestSetUserEmail which verifies the new method stores email without touching UserID.

Test the full "me" resolution matrix: - Email matches a person in the account → returns person ID - Case-insensitive email matching works - Email stored but not in people list, with identity ID in UserID → returns error, NOT the identity ID (the regression Jorge identified) - No email stored at all → returns auth error - Email-only stored (no UserID) → resolves correctly The key regression test (IdentityID_NotLeaked) fails when the old GetUserID fallback is present, proving the fix is load-bearing. Also adds SetStore to auth.Manager as a test seam for cross-package tests that need to inject a file-backed store.

cubic-dev-ai

No issues found across 10 files

Since this is your first cubic review, here's how it works:

cubic automatically reviews your code and comments on bugs and improvements
Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
Add one-off context when rerunning by tagging @cubic-dev-ai with guidance or docs links (including llms.txt)
Ask questions if you need clarification on any suggestion

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/names/resolver.go

internal/names/resolver_test.go

internal/auth/auth_test.go

Address PR review feedback: - ResolvePerson("me") now returns the getPeople() error instead of swallowing it and returning a misleading auth error - Check store.Save errors in test setup with require.NoError

cubic-dev-ai

1 issue found across 3 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:148">
P2: Misleading error message when email exists but doesn't match any person in the account. After the restructuring, this `return` is only reached when the user has a stored email but it doesn't match anyone in the current account's people list. Telling them to `basecamp auth login` won't help — the issue is account membership, not authentication. Consider a more specific message like `"Your email (%s) was not found in this account's people list. You may not be a member of this account."` or at minimum differentiate it from the empty-email case above.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

internal/names/resolver.go

When ResolvePerson("me") fails, distinguish between no stored email (suggests re-login) and email not found in the account (suggests checking account selection).

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/names/resolver.go

internal/names/resolver_test.go

internal/auth/auth_test.go

Using make() instead of var ensures the cache sentinel (nil check) recognizes an empty result as populated, preventing re-fetches on every call when an account has zero projects or people.

internal/names/resolver.go

Copilot AI review requested due to automatic review settings March 6, 2026 04:33

github-actions bot added commands CLI command implementations tests Tests (unit and e2e) auth OAuth authentication bug Something isn't working labels Mar 6, 2026

Copilot started reviewing on behalf of jorgemanrubia March 6, 2026 04:34 View session

jorgemanrubia closed this Mar 6, 2026

Copilot AI reviewed Mar 6, 2026

View reviewed changes

internal/names/resolver.go Show resolved Hide resolved

internal/names/resolver.go Show resolved Hide resolved

internal/names/resolver.go Outdated Show resolved Hide resolved

internal/names/resolver.go Outdated Show resolved Hide resolved

jeremy reopened this Mar 6, 2026

jeremy added 4 commits March 5, 2026 21:41

Remove identity ID fallback from ResolvePerson("me")

be88f05

The fallback returned a stored identity ID (cross-account) when email matching failed. Since all consumers need account-scoped person IDs, returning a wrong-namespace ID silently is worse than an error.

Remove dead GetUserID and SetUserID methods

5913653

After the previous two commits, these have zero callers. Replace their tests with TestSetUserEmail which verifies the new method stores email without touching UserID.

jeremy marked this pull request as ready for review March 6, 2026 06:05

Copilot AI review requested due to automatic review settings March 6, 2026 06:05

Copilot started reviewing on behalf of jeremy March 6, 2026 06:05 View session

cubic-dev-ai bot reviewed Mar 6, 2026

View reviewed changes

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Surface getPeople errors in "me" resolution; check Save in tests

bf3dc6b

Address PR review feedback: - ResolvePerson("me") now returns the getPeople() error instead of swallowing it and returning a misleading auth error - Check store.Save errors in test setup with require.NoError

cubic-dev-ai bot reviewed Mar 6, 2026

View reviewed changes

internal/names/resolver.go Show resolved Hide resolved

Differentiate error messages for missing email vs unmatched email

5aca5fc

When ResolvePerson("me") fails, distinguish between no stored email (suggests re-login) and email not found in the account (suggests checking account selection).

Copilot AI review requested due to automatic review settings March 6, 2026 06:37

Copilot started reviewing on behalf of jeremy March 6, 2026 06:37 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

internal/names/resolver.go Show resolved Hide resolved

internal/names/resolver_test.go Show resolved Hide resolved

internal/auth/auth_test.go Show resolved Hide resolved

jeremy changed the title ~~Fix "me" resolution: use email matching instead of broken identity ID~~ Fix "me" resolution: stop conflating identity IDs with person IDs Mar 6, 2026

Initialize slices in getProjects/getPeople so empty results are cached

0fbea33

Using make() instead of var ensures the cache sentinel (nil check) recognizes an empty result as populated, preventing re-fetches on every call when an account has zero projects or people.

jeremy merged commit 56e233e into main Mar 6, 2026
22 checks passed

jeremy deleted the fix-resolve-person-me branch March 6, 2026 07:00

jorgemanrubia commented Mar 6, 2026

View reviewed changes

internal/names/resolver.go Show resolved Hide resolved

Conversation

jorgemanrubia commented Mar 6, 2026 • edited by jeremy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Test plan

Files changed

Uh oh!

jorgemanrubia commented Mar 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jorgemanrubia commented Mar 6, 2026 •

edited by jeremy

Loading