feat(cli): PAD_TOKEN environment override for stored credentials (#879) - #1160
feat(cli): PAD_TOKEN environment override for stored credentials (#879)#1160b4rk13 wants to merge 2 commits into
Conversation
…petualSoftware#879) Layer 1 of PerpetualSoftware#879: if PAD_TOKEN is set, the CLI uses it as the bearer token and skips the credential-store lookup — gh's GH_TOKEN convention. Reads never write credentials.json, so a read-only override sidesteps the multi-agent identity contention completely; the store is never touched under the override. Per the acceptance grounding notes: - NewClientFromURL resolves PAD_TOKEN before the per-server store lookup (the single token-attachment chokepoint). - whoami no longer lies under the override: it skips the store short-circuit and reports the effective identity via a real /me fetch, with an 'Auth: PAD_TOKEN environment override' line. - auth login/logout print a gh-style stderr notice when the override is active. logout additionally pins its server-side session invalidation to the STORED token — an unpinned Logout() after the constructor change would have invalidated the env token's session — and skips the server call when there is no stored session. - pad init's status line and server info's report disclose the override (env_token_override field; the auth probe uses the token every other command would use). Zero behaviour change when PAD_TOKEN is unset. Token minting stays web-only; a minimal 'pad token' CLI is offered as a follow-up. Co-Authored-By: Claude Fable 5 <[email protected]>
xarmian
left a comment
There was a problem hiding this comment.
Thank you — the core of this is exactly the contract from the #879 thread, and the care shows: the auth-resolution comment in client.go explains why, logout pins to the stored token so it can't invalidate the env token's session (a subtlety most implementations would miss), the rejection message is distinct, and the leak surface is clean (we grepped every EnvToken()/authToken site — the token only ever travels in the Authorization header). Tests ran green locally (internal/cli, cmd/pad). Two identity-confusion bugs to fix — both places where the override didn't get the same treatment you gave whoami/logout:
Bug 1 (blocking): pad init silently falls back to the stored user when PAD_TOKEN is rejected — then misreports it. Step 4 (init.go ~260) is pre-existing code with no cli.EnvToken() guard: session.Authenticated=false under a rejected env token flows into the stored-credential fallback → goto authenticated, so the process quietly runs as whatever human's session is on the machine. And since printInitStatus decides its "Auth: PAD_TOKEN environment override" line by env presence (init.go:617), not by which token actually authenticated, the final status then claims the override was active when it wasn't — the worst combination for the multi-agent use case this feature exists for. Suggested shape: in Step 4, when cli.EnvToken() != "", fail with the distinct rejected-token message instead of falling back (mirroring your whoami behavior), which also makes the status line truthful again.
Bug 2: login contradicts its own notice. The new envTokenNotice() prints "PAD_TOKEN overrides stored credentials for all API calls," but the store-shortcut right below still runs unconditionally and prints "Already logged in as " — even when PAD_TOKEN is a valid token for a different user. logout got the real logic fix; login only got the notice. Same guard pattern applies, and env_token_override_test.go covers whoami/logout but not login — worth a case.
One documentation ask (not a bug): under PAD_TOKEN, logout now (correctly) never invalidates the env token's own session — the env token's lifecycle belongs to whoever minted it, same as GH_TOKEN. That asymmetry is right per the reads-only contract, but it should be stated in env_token.go's doc comment or the README section so nobody reads it as an oversight later.
With those two fixed this is mergeable from my side. The whoami/logout treatment shows you already had the right instinct — these are just the two paths it didn't reach. Looking forward to layer 2.
…shortcut skipped under the override; logout asymmetry documented Per the PR PerpetualSoftware#1160 round-1 review: - Bug 1: pad init's auth step no longer falls back to stored credentials when a set PAD_TOKEN is rejected — it fails with the distinct rejected-token message (mirroring whoami), which also makes the status line's override disclosure truthful. Test drives the real padInitCmd flow and asserts the stored identity is never consulted. - Bug 2: login's 'Already logged in as <stored user>' shortcut is skipped when the override is active — it reads the store, and firing it right after envTokenNotice contradicted the notice. A second test pins the unchanged no-override shortcut behaviour. - Doc ask: the deliberate logout asymmetry (the env token's own session is never invalidated; its lifecycle belongs to the minter, GH_TOKEN posture) is now stated in env_token.go's doc comment and the README PAD_TOKEN section. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Both fixed in Bug 1 (init fallback) — took your suggested shape: when Bug 2 (login shortcut) — the store-shortcut is now gated on Doc ask — the logout asymmetry is now stated in both places: Zero new failures by name across |
What does this PR do?
Layer 1 of #879, in the order you accepted:
PAD_TOKENas a per-process bearer-token override,gh'sGH_TOKENconvention. Named profiles (v3 store) come separately as layer 2.Part of #879 (layer 1 of 2 — please don't auto-close the issue on merge).
Your three grounding notes, taken as contract:
whoamiunderPAD_TOKENskips the store lookup and reports the effective identity via a real/mefetch, adding anAuth: PAD_TOKEN environment overrideline (and a distinct message when the server rejects the env token).pad init's status output andpad server infodisclose the override too —server infogains an additiveenv_token_overrideJSON field, and its auth probe now uses the token every other command would use.credentials.json— pinned by a test asserting the file is byte-identical across client construction underPAD_TOKEN.auth login/logoutwarn (one stderr line, gh-style) when the override is active. One behaviour I want your eyes on:logoutnow pins its server-side session invalidation to the stored token — after the constructor change, an unpinnedclient.Logout()would have invalidated the env token's session, which is the one thing logout must never do — and skips the server call entirely when there's no stored session. Unset-PAD_TOKENbehaviour is unchanged.The override lands at the
NewClientFromURLchokepoint you identified: env token first, then the per-server store lookup. Zero behaviour change when the variable is unset.Out of scope, as flagged on the issue: the minimal
pad token create/list/revokeCLI — immediate follow-up PR unless you'd rather have it here.How to test
go test ./internal/cli/ ./cmd/pad/ -run 'EnvToken|Whoami_|Logout_UnderEnv|NewClientFromURL_' -count=1— the new suite (written first, watched fail). The command-level tests drive realwhoamiCmd/logoutCmdagainst anhttpteststub and assert which bearer token each request carried.pad auth whoami→ stored user;PAD_TOKEN=<other user's API token> pad auth whoami→ that user + theAuth:line;PAD_TOKEN=... pad item list --workspace <slug>→ acts as the token's user;PAD_TOKEN=... pad auth logout→ warns, deletes the stored entry, env identity keeps working.USERPROFILEalongsideHOME—os.UserHomeDirreadsUSERPROFILEon Windows, soHOME-only isolation (the existing convention) points Windows runs at the developer's real~/.pad. Happy to file that separately for the existing tests if useful.Checklist
make buildpasses (go build ./...; web unchanged)make testpasses — same caveat as fix(mcp): accept afieldsobject on pad_item create/update; reject undeclared input keys (#1066) #1159: the Windows dev box has a pre-existing red baseline; name-level failure comparison before/after this change is identical, zero new failuresPAD_TOKEN)🤖 Generated with Claude Code