Skip to content

Add browser-based CLI login support#53

Merged
etbyrd merged 8 commits into
mainfrom
jet/browser-cli-login
May 6, 2026
Merged

Add browser-based CLI login support#53
etbyrd merged 8 commits into
mainfrom
jet/browser-cli-login

Conversation

@jetpham
Copy link
Copy Markdown
Contributor

@jetpham jetpham commented May 6, 2026

Summary

  • Document CLI login/logout endpoints in OpenAPI and regenerate Node, Python, and Go SDK artifacts.
  • Add Node CLI primitive login and primitive logout with file-backed saved credentials.
  • Let CLI commands use saved login credentials and self-heal stale saved credentials on unauthorized responses.

Verification

  • pnpm --dir sdk-node typecheck
  • pnpm --dir sdk-node test
  • pnpm --dir sdk-node build:oclif
  • biome check --error-on-warnings src/oclif tests/oclif
  • git diff --check

Pre-Merge

  • Preview/deployed E2E should be run with the API/Web PR before merging.

Expose the CLI login API in generated SDKs and add Node CLI login/logout commands so saved credentials can be managed end to end.
@jetpham
Copy link
Copy Markdown
Contributor Author

jetpham commented May 6, 2026

@greptile review

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 6, 2026

Greptile Summary

This PR adds browser-based primitive login / primitive logout commands to the Node CLI, backed by a new auth.ts credential-storage module (atomic writes, directory-based locking, resolveCliAuth priority chain). It also documents the underlying /cli/login/start, /cli/login/poll, and /cli/logout endpoints in the OpenAPI spec and regenerates the Go, Python, and Node SDK artifacts.

  • auth.ts: New credential module with atomic temp-file writes (PID + UUID temp name, renameSync), a 30-minute stale-lock recovery policy, and resolveCliAuth that prefers explicit flags/env over saved credentials.
  • login.ts / logout.ts: Full device-code flow with browser opener, polling loop, slow_down/authorization_pending/expired/denied handling, and a checkExistingLogin guard that auto-removes stale credentials before starting a fresh flow.
  • api-command.ts and existing commands: All authenticated commands now call resolveCliAuth to pick up saved credentials and removeStaleSavedCredentialOnUnauthorized to self-heal on 401 \u2014 but this cleanup helper deletes credentials without holding the credential lock (see inline comment).

Confidence Score: 4/5

Safe to merge with awareness of a race condition in credential cleanup that can silently undo a successful login when two CLI processes run concurrently against stale credentials.

The new removeStaleSavedCredentialOnUnauthorized helper deletes the credentials file without acquiring the directory-based lock that login and logout use. If a concurrent login completes and writes fresh credentials during the window between another command resolving its auth and that command's 401 handler firing, the freshly-saved credentials are silently deleted. The rest of the change is solid and well-tested.

sdk-node/src/oclif/api-command.ts — the removeStaleSavedCredentialOnUnauthorized function and every call site that reaches it without holding the credential lock.

Important Files Changed

Filename Overview
sdk-node/src/oclif/api-command.ts Added resolveCliAuth integration for all operation commands and new removeStaleSavedCredentialOnUnauthorized helper. The helper deletes credentials without acquiring the credential lock, creating a race condition where a concurrent successful login's credentials can be silently deleted by a parallel command's 401 error handler.
sdk-node/src/oclif/auth.ts New module for CLI credential storage: atomic temp-file write, directory-based lock, and resolveCliAuth priority chain. Well-structured with good error messages.
sdk-node/src/oclif/commands/login.ts New LoginCommand: device-code flow with browser opener, polling loop with slow_down/authorization_pending/expired_token/access_denied handling, credential lock, and checkExistingLogin guard. Logic is sound and tests are thorough.
sdk-node/src/oclif/commands/logout.ts New LogoutCommand: revokes the saved API key via the server and deletes local credentials. Handles unauthorized/not_found responses gracefully. Lock acquisition is correct.
sdk-node/tests/oclif/auth.test.ts New test suite covering credential save/load/delete, file permissions, lock acquisition, stale lock recovery, and resolveCliAuth priority chain. Coverage is comprehensive.
sdk-node/tests/oclif/login.test.ts New tests for checkExistingLogin covering valid login, stale credential removal, base-URL mismatch blocking, non-auth error blocking, and trailing-slash normalization.
openapi/primitive-api.yaml Added /cli/login/start, /cli/login/poll, and /cli/logout endpoints with schemas, response envelopes, and error examples. New CLI error codes added to ErrorResponse enum.

Comments Outside Diff (1)

  1. sdk-node/src/oclif/api-command.ts, line 62-92 (link)

    P1 Credential deletion without lock can clobber a freshly-saved login

    removeStaleSavedCredentialOnUnauthorized calls deleteCliCredentials without ever acquiring acquireCliCredentialsLock. The login command holds that lock for the entire browser-approval polling cycle, but any other command (whoami, send, emails-latest, or a generated operation command) does not hold it when it calls this function.

    Failure scenario: a user has stale saved credentials and runs primitive login in one shell while a second shell runs primitive whoami (or a script runs multiple commands in parallel). login succeeds, saves a fresh API key, and releases the lock — all while whoami's API call is still in-flight against the old key. When that 401 comes back, removeStaleSavedCredentialOnUnauthorized deletes the now-valid credentials login just wrote. The user sees "Removed saved Primitive CLI credentials because the backing API key is no longer valid" and their successful login is silently undone.

    A targeted fix is to re-read the credential file inside removeStaleSavedCredentialOnUnauthorized and compare the stored key_id against params.auth.credentials.key_id before deleting — if they differ, the credentials have already been replaced and should be left alone.

Reviews (8): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Comment thread sdk-node/src/oclif/commands/login.ts
@jetpham
Copy link
Copy Markdown
Contributor Author

jetpham commented May 6, 2026

@greptile review

Commit the Go artifacts produced by the CI generator so generated-file checks stay in sync.
@jetpham
Copy link
Copy Markdown
Contributor Author

jetpham commented May 6, 2026

@greptile review

@jetpham jetpham requested a review from etbyrd May 6, 2026 01:10
@jetpham jetpham self-assigned this May 6, 2026
Move slow_down to the RFC-compatible 400 response and document the CLI login metadata size limit across generated SDK artifacts.
@jetpham
Copy link
Copy Markdown
Contributor Author

jetpham commented May 6, 2026

@greptile review

@etbyrd
Copy link
Copy Markdown
Member

etbyrd commented May 6, 2026

A few things from a deeper pass. Reviewed the hand-written surface (openapi/primitive-api.yaml, the new auth.ts / commands/login.ts / commands/logout.ts, and the api-command.ts / whoami / emails-latest / send updates); skimmed the regenerated SDK output.

Should fix

  1. primitive login is unusable when the API is temporarily unreachable. checkExistingLogin calls getAccount to validate saved creds. If the response is anything other than success or unauthorized (network failure, 5xx, timeout), it returns blocked with "Run primitive logout before logging in again." A user with stored creds cannot re-login during a Primitive outage without first nuking working credentials. Either skip the existence check when the probe fails to connect, or add a --force flag that overwrites without verification. The "already logged in" branch should also accept --force.

  2. Phone-home metadata is sent without disclosure (login.ts:175). metadata: { arch, platform, version } is sent on every login start, persisted to cli_login_sessions.metadata, and then merged into the long-lived api_keys.metadata server-side. Not PII, but the user isn't told and there's no opt-out. Either drop it (the server already captures user_agent and created_ip, which covers most of this) or surface it in the login output with a --no-metadata opt-out.

  3. Spec/SDK drift on slow_down if the paired API PR adopts the RFC fix. I asked on the API/web PR to consider returning 400 + error: "slow_down" per RFC 8628 instead of 429 + Retry-After. The CLI happens to handle both (it switches on code not status, and retryAfterSeconds returns null on 400 and falls back to interval + 5). But this OpenAPI spec documents 429 + Retry-After explicitly. If the paired change goes through, the spec, response examples, and regenerated SDK types all need updating. Worth a Pre-Merge note that these two PRs ship together.

Worth addressing

  1. Concurrent primitive login runs can clobber each other. saveCliCredentials writes the file non-atomically. Two parallel logins (race after both pass loadCliCredentials() == null) could result in one CLI's saved key shadowing another's still-valid key. Use writeFile to a tempfile + rename, or grab a process-level lock.

  2. interval can grow without bound on repeated slow_down (login.ts:233). Capped only by the 15-min deadline. Add a hard ceiling (60s) so a buggy server can't push the CLI into one-poll-per-minute territory.

  3. Error codes are matched as string literals scattered across files (login.ts, logout.ts, api-command.ts, send.ts). The OpenAPI spec already adds these to the error_code enum. Pull them from types.gen.ts so a typo or future rename is a type error instead of a silent break.

  4. Login doesn't tell the user where credentials landed. Add a final line: Saved credentials to ${credentialsPath(this.config.configDir)}. Helpful for debugging and for users who want to copy the file to another machine (or revoke by deletion).

  5. Initial poll waits a full interval (5s) before the first call (login.ts:225). If the user approves in the browser instantly the CLI still waits 5s. Cheap fix: do one quick poll at ~1s, then settle into the server-suggested interval.

Minor

  1. Windows openBrowser uses cmd /c start "" url. URLs with & or ^ can be reinterpreted by cmd.exe even with spawn. The URL comes from our own API so it's controlled today, but worth a Windows smoke test in CI before relying on this. The current URL pattern (/cli/login?code=XXXX-YYYY) is safe.

  2. parseCredentials throws the same generic message for every shape error. Fine for users (action is the same: logout + login), but include the field name to make future cred format changes easier to debug.

  3. Public-repo hygiene: the commit message address greptile cli login feedback lands in git log on a public repo. Not damning, but advertises which automated review tool the team uses. Squash on merge if you care.

What's good

  • File mode 0o600, dir 0o700. writeFileSync with mode + redundant chmodSync covers filesystems that ignore creation mode.
  • removeStaleSavedCredentialOnUnauthorized correctly refuses to delete creds when the base URL was overridden. Tests cover the matching and mismatching base URL cases explicitly.
  • logout distinguishes "key already gone server-side" (delete local + exit 1) from "network failure" (preserve local + throw). Right call.
  • spawn (not exec) for browser open: no shell injection surface even if the verification URL ever held something weird.
  • Test coverage is thorough: auth round-trip including 0o600 verification, all four checkExistingLogin branches, removeStale* with base-URL guard.
  • Pulling auth resolution into a single resolveCliAuth and threading it through every command via the same removeStale* helper is the right shape: no command can forget the self-heal step.

@jetpham
Copy link
Copy Markdown
Contributor Author

jetpham commented May 6, 2026

@greptile review

@etbyrd
Copy link
Copy Markdown
Member

etbyrd commented May 6, 2026

Re-review of 163d10f + 55ee930. 8 of 10 prior items resolved cleanly (spec + slow_down, metadata removed, lockfile + atomic write, interval cap, typed API_ERROR_CODES, --force, "Saved credentials to..." line, faster first poll, field-specific parse errors). Two small things worth closing before merge so we don't revisit:

1. Stale lock on crash. acquireCliCredentialsLock uses mkdirSync for atomicity, which is right, but if the login process crashes or is killed mid-flow the lock dir persists. The next primitive login fails with "Another Primitive CLI credential operation is already in progress." With a 15-min device-code window the user can hit a real blackout from a single ctrl-C. Two reasonable options:

  • Write process.pid (and optionally start timestamp) into a file inside the lock dir on acquire. On EEXIST, read it, check if the PID is alive (process.kill(pid, 0) throws ESRCH if not), and if dead, remove the lock and retry once.
  • Or, if that's more than you want to own, mention in the error message that the user can remove the lock manually: "If no other primitive process is running, remove ~/.config/.../credentials.lock and retry."

PID-based recovery is the kinder UX and isn't much code.

2. logout doesn't acquire the lock. Concurrent login + logout can race: login writes credentials.json, logout deletes it a moment later, login's saved creds vanish silently. Probably rare in practice, but the lock helper already exists and wrapping logout in it is two lines. If we're investing in serialization at all, finishing the job is cheaper than explaining the gap later.

Otherwise looks great. LGTM with these two.

@jetpham
Copy link
Copy Markdown
Contributor Author

jetpham commented May 6, 2026

@greptile review

@jetpham
Copy link
Copy Markdown
Contributor Author

jetpham commented May 6, 2026

@greptile review

@etbyrd etbyrd merged commit 3856aa9 into main May 6, 2026
9 checks passed
@etbyrd etbyrd deleted the jet/browser-cli-login branch May 6, 2026 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants