feat(cli): non-interactive / agent-friendly auth + region flags#569
Conversation
🦋 Changeset detectedLatest commit: f55226a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThis PR adds non-interactive/agent-friendly behavior to ChangesNon-interactive CLI region and auth JSON support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User as CLI User/Agent
participant CLI as stash CLI
participant Region as resolveRegion
participant Auth as AuthAPI (device code)
participant Events as events.ts
User->>CLI: stash auth login --region us-east-1 --json
CLI->>Region: resolveRegion({regionFlag, json})
Region-->>CLI: normalized region or failRegion(exit 1)
CLI->>Auth: beginDeviceCodeFlow(region)
Auth-->>CLI: pending device code + verification URL
CLI->>Events: emitJsonEvent(authorization_required)
CLI->>Auth: poll for authorization
Auth-->>CLI: authorized token
CLI->>Events: emitJsonEvent(authorized)
CLI->>Auth: bindDevice()
Auth-->>CLI: bound
CLI->>Events: emitJsonEvent(device_bound)
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Test coverage review — PR #569 (non-interactive / agent-friendly auth CLI)
Verdict: Solid coverage on the parts that were factored out to be testable. region.ts (normalizeRegion, regionSlugs, regionList, resolveRegion) is exercised across happy paths, precedence, invalid/missing regions, TTY/CI/json branches — nicely done. The E2E suite covers auth regions, help text, and the pre-network region-resolution failures.
The one real gap is the new JSON-event emission and error-mapping logic in login.ts (login() and bindDevice()). The E2E header explicitly (and reasonably) skips the login happy path because it hits the network — but the JSON serialization, the expiresAt→ISO conversion, and the begin_failed/poll_failed/bind_failed error mapping are all pure logic that can be unit-tested by mocking @cipherstash/auth the same way region.test.ts mocks @clack/prompts (the mock replaces the module before load, so no native binary is pulled into the fast suite). Two inline sketches below cover it.
Additional coverage gaps not posted inline
authErrorCodefallback (login.ts:45) — the?? 'begin_failed'/poll_failed/bind_faileddefaults kick in only when the thrown error lacks a.code. The two sketches below should assert both anAuthError-shaped error ({ code: 'INVALID_CLIENT' }→ payloadcodeechoes it) and a plainError(→ the per-call default), so the branch inauthErrorCodeis actually exercised. Classic lopsided-negative: right now neither the code-present nor code-absent arm is covered.--no-open/openInBrowser()(login.ts:96) — theopenflag and the!opened && !jsonwarn branch are untested. Lower value (browser-opening is UI-adjacent), but a mockedopenInBrowser: () => falsein non-json mode would confirm the warn fires and stays silent in json mode.- init
--regionthreading (init/index.ts:80,authenticate.ts:48) —initCommandnow copiesvalues.regionintostate.regionFlag, consumed byresolveRegionin the authenticate step. No test asserts this wiring; it's integration-level (full init flow) so likely not worth a unit test, but worth a mention.
No crypto/security concerns noted in the diff.
|
Addressed the code-review findings and the test-gap review in Code review fixes
Test gaps
Verification: 360/360 unit tests and 12/12 auth e2e tests pass; typecheck introduces no new errors (the pre-existing |
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
Test-coverage review (consolidated)
Solid PR. Coverage on the new non-interactive surface is genuinely strong: region.test.ts exercises resolveRegion precedence/gating exhaustively, login.test.ts covers the JSON event stream on both error axes, and the E2E suite guards the anti-hang regression end-to-end. The remaining gaps are all on the interactive (non-json) arms of login.ts and on the command-wrapper plumbing (authCommand / initCommand), neither of which is exercised today. None are correctness bugs — this is a COMMENT, not a change request — but each is a branch a future refactor could silently break.
Both reviewers independently landed on the interactive browser-open gap; the rest are single-source but verified against the diff.
Review stats
| Source | Raw findings | Survived |
|---|---|---|
| codex (gpt-5.5) [test-gap] | 3 | 3 |
| claude (claude-opus-4-8) [test-gap] | 3 | 3 |
- Kept findings: 5 (the two
login.ts:80findings merged into one) - Cross-model corroboration: 1 kept finding (interactive browser-open branch) was raised by 2+ models.
- Dropped as unverifiable: 0
Addresses 5 gaps Lindsay flagged: - login: interactive (non-json) browser-open branch — opens once, respects open:false, warns when openInBrowser() returns false - login: non-json rethrow arms for begin + poll failures propagate the original error and do NOT call process.exit - bindDevice: code-present error branch (authErrorCode pass-through), matching login's code-present/absent coverage - authCommand: new index.test.ts asserts values.region / json / open: !json && !--no-open / referrer are forwarded into resolveRegion, login, bindDevice (+ valueless --region fails fast before login) - initCommand: new init-command.test.ts asserts state.regionFlag is seeded from values.region before the authenticate step runs login.test.ts's clack mock is hoisted so the spinner + p.log.warn are observable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli/src/commands/auth/region.ts (1)
21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making
regionsimmutable.The
regionsarray is exported and mutable. SinceregionList()andregionSlugs()derive from it, accidental mutation would silently corrupt downstream behavior. Addingas const(or at minimumreadonly) would prevent this at compile time.♻️ Optional: harden the regions array
-export const regions = [ +export const regions = [ + { value: 'us-east-1.aws', label: 'us-east-1 (Virginia, USA)' }, + { value: 'us-east-2.aws', label: 'us-east-2 (Ohio, USA)' }, + { value: 'us-west-1.aws', label: 'us-west-1 (California, USA)' }, + { value: 'us-west-2.aws', label: 'us-west-2 (Oregon, USA)' }, + { value: 'ap-southeast-2.aws', label: 'ap-southeast-2 (Sydney, Australia)' }, + { value: 'eu-central-1.aws', label: 'eu-central-1 (Frankfurt, Germany)' }, + { value: 'eu-west-1.aws', label: 'eu-west-1 (Dublin, Ireland)' }, +] as const🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/auth/region.ts` around lines 21 - 29, The exported regions list is mutable, which can let accidental changes affect regionList() and regionSlugs() behavior. Make the regions declaration immutable in region.ts by using a readonly/const assertion on the regions array so its entries cannot be modified at compile time, and keep the existing regionList and regionSlugs helpers reading from that immutable source.packages/cli/src/commands/auth/login.ts (1)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the warn string to
messages.ts— it's asserted in tests.
'Could not open browser — please visit the URL above manually.'is a user-facing string asserted viaexpect.stringContaining('Could not open browser')inlogin.test.ts:233-234. Per coding guidelines, assertion-stable user-facing strings should live insrc/messages.tsso both production code and tests consume the same copy. As per coding guidelines: "Put assertion-stable user-facing strings insrc/messages.tsinstead of hard-coding new wording directly in tests; update the constant there so both production code and tests consume the same copy."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/auth/login.ts` around lines 83 - 85, Move the hard-coded browser warning in login’s warn call to src/messages.ts and have login.ts reference that shared constant instead. The string is assertion-stable in login.test.ts, so define it in messages.ts with the existing user-facing wording, then update the login flow to import and use that message so production code and tests consume the same copy.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/commands/auth/login.ts`:
- Around line 83-85: Move the hard-coded browser warning in login’s warn call to
src/messages.ts and have login.ts reference that shared constant instead. The
string is assertion-stable in login.test.ts, so define it in messages.ts with
the existing user-facing wording, then update the login flow to import and use
that message so production code and tests consume the same copy.
In `@packages/cli/src/commands/auth/region.ts`:
- Around line 21-29: The exported regions list is mutable, which can let
accidental changes affect regionList() and regionSlugs() behavior. Make the
regions declaration immutable in region.ts by using a readonly/const assertion
on the regions array so its entries cannot be modified at compile time, and keep
the existing regionList and regionSlugs helpers reading from that immutable
source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1f9a7ff-ac45-48d7-8623-08a8c5cfec8b
📒 Files selected for processing (20)
.changeset/stash-non-interactive-agent-cli.mdpackages/cli/AGENTS.mdpackages/cli/README.mdpackages/cli/src/bin/main.tspackages/cli/src/commands/auth/__tests__/index.test.tspackages/cli/src/commands/auth/__tests__/login.test.tspackages/cli/src/commands/auth/__tests__/region.test.tspackages/cli/src/commands/auth/events.tspackages/cli/src/commands/auth/index.tspackages/cli/src/commands/auth/login.tspackages/cli/src/commands/auth/region.tspackages/cli/src/commands/init/__tests__/init-command.test.tspackages/cli/src/commands/init/index.tspackages/cli/src/commands/init/steps/authenticate.tspackages/cli/src/commands/init/types.tspackages/cli/src/config/database-url.tspackages/cli/src/config/tty.tspackages/cli/src/messages.tspackages/cli/tests/e2e/auth-login-cancel.e2e.test.tspackages/cli/tests/e2e/auth-non-interactive.e2e.test.ts
Thanks! All addressed. |
Additive escape hatches so `stash init` and `stash auth login` can run without a TTY (agents, CI, pipes). Nothing changes for interactive humans — the region picker still renders in a real terminal. - `--region <slug>` + `STASH_REGION` on `auth login` and `init` skip the interactive region picker. An unknown or missing region in a non-TTY context now exits with an actionable message instead of hanging on the picker (previously the only prompt with no escape hatch). - `stash auth login --json` emits newline-delimited device-code events. The first event (`authorization_required`) carries the verification URL so an agent can *trigger* auth and hand the browser step to a human — only a human completes it in the browser. `--no-open` suppresses the browser launch for headless contexts. - Region resolution mirrors the DATABASE_URL resolver (TTY + !CI gate) and lives in a native-free module (`src/commands/auth/region.ts`) so its pure helpers and policy have fast unit coverage. New E2E covers the non-interactive paths; the interactive-cancel E2E now overrides CI to keep exercising the picker. Docs updated in packages/cli README + AGENTS.md.
First-contact affordance so an agent (or human) can discover valid
`--region` / `STASH_REGION` values up front instead of learning them
reactively from an error. Grouped under `auth` (region is an auth concern,
next to `stash auth login`), matching how AWS/gcloud/Fly group region lists
under their domain.
- `stash auth regions` prints the region labels (plain stdout, pipe-friendly).
- `stash auth regions --json` emits `[{ slug, label }]` for agents.
- Backed by a new `regionList()` in the native-free region module; unit + E2E
covered. Docs + help updated.
Code review fixes: - --json no longer auto-opens a browser on the agent host (open defaults to !json; help/README updated) - valueless --region now fails with an actionable message instead of a misleading region_required - empty/whitespace --region falls back to STASH_REGION (matches init's guard) - extract shared isCiEnv() into config/tty.ts (dedup vs database-url resolver) - remove login.ts re-export shim; import region symbols from region.js directly - share the NDJSON error envelope via commands/auth/events.ts - derive regionSlugs() from regionList() Test gaps: - new login.test.ts covers login/bindDevice json paths + authErrorCode branches - unit coverage for the empty-flag fallback; e2e coverage for the bare --region guard
Addresses 5 gaps Lindsay flagged: - login: interactive (non-json) browser-open branch — opens once, respects open:false, warns when openInBrowser() returns false - login: non-json rethrow arms for begin + poll failures propagate the original error and do NOT call process.exit - bindDevice: code-present error branch (authErrorCode pass-through), matching login's code-present/absent coverage - authCommand: new index.test.ts asserts values.region / json / open: !json && !--no-open / referrer are forwarded into resolveRegion, login, bindDevice (+ valueless --region fails fast before login) - initCommand: new init-command.test.ts asserts state.regionFlag is seeded from values.region before the authenticate step runs login.test.ts's clack mock is hoisted so the spinner + p.log.warn are observable.
…-3431)
Phase 1 of docs/plans/cli-help-and-manifest.md: a single source of truth for
command metadata so help/docs can't drift from the real command set.
- src/cli/registry.ts: Flag/CommandDescriptor/CommandGroup types + the full
command surface (groups, summaries, flags; long/examples for key commands),
populated from the existing HELP surface and per-command flag parsing.
- src/cli/manifest.ts: buildManifest(version) → { name, version, groups[] },
the exact contract the docs generator (cipherstash/docs#45) targets. version
comes from the CLI's package.json.
- `stash manifest --json` emits the structured surface; `stash manifest` prints
a grouped human-readable list. Pure metadata — no native binary required.
Additive and non-breaking; the top-level HELP string is untouched. Rendering
help from the registry (phases 2–4) is the documented follow-on.
Tests: unit coverage for buildManifest shape/version/hidden-exclusion; e2e for
`manifest --json`, `manifest`, and `--help` listing the command.
Registry-vs-reality fixes (the drift this PR exists to prevent): - encrypt status/plan: drop --table/--column — both dispatch to zero-arg commands (statusCommand()/planCommand()) that ignore them - eql install: add the real --name/--out flags (forwarded to the drizzle path) - db migrate: mark summary "(not yet implemented)" and drop the --database-url flag it never reads (dispatch only prints a not-implemented warning) Cleanup / correctness: - manifest: toManifestCommand now defensively copies examples/flags so the manifest can't alias the registry's shared flag singletons (mutation safety) - registry: extract TABLE/COLUMN/DRY_RUN/EXCLUDE_OPERATOR_FAMILY/SUPABASE_COMPAT shared flag consts (were copy-pasted 3-5x), matching the DATABASE_URL_FLAG pattern - manifest: drop the single-use `ManifestFlag = Flag` alias - soften the "single source of truth" claim (registry docstring + changeset): it's additive; HELP stays authoritative for --help until phase 2 Tests: - buildManifest gains a `groups` injection seam so the hidden-command filter is driven by a stub (was vacuously green against the real registry) - add positive assertions for examples passthrough and the defensive-copy fix
coderdan
force-pushed
the
agent-friendly-cli
branch
from
7b0bf68 to
4a523f1
Compare
Rebase follow-up: login.ts was merged onto @cipherstash/auth 0.41's Result API
(begin/poll/openInBrowser/bindClientDevice return { data }/{ failure }, and
non-json failures p.log.error + exit(1) instead of throwing). The login tests
still used the old throwing-API mocks — rewrite them to the Result shape
(failure code from AuthFailure.type) and flip the two non-json cases from
rethrow to error+exit(1).
2 participants
Why
Driving the CLI as an agent (or in CI / over a pipe) hits one hard wall: the
region picker in
stash auth login— and, transitively,stash init— is aninteractive
p.selectwith no non-interactive escape hatch, so it blocks.Everything else in
initalready has one (--database-url/DATABASE_URL,--proxy/--no-proxy,--target), so region was the last thing standingbetween an agent and a scripted
stash init.This PR closes that gap additively — nothing changes for an interactive
human (the picker still renders in a real terminal); we only add flags, env
vars, and a machine-readable output mode.
What changed
1. Region without a prompt —
--region/STASH_REGIONstash auth loginandstash init.us-east-1) or canonical form (us-east-1.aws),case-insensitively.
DATABASE_URLresolver:--region→STASH_REGION→ interactive picker (only whenstdin.isTTYand notCI) → cleanexit(1)with an actionable message (never a hang).src/commands/auth/region.tsso the purehelpers (
normalizeRegion,regionSlugs) and the resolver policy get fastunit coverage without loading the
@cipherstash/authbinary.login.tsre-exports the names, so existing call sites are untouched.
2. Agent-triggerable device-code auth —
stash auth login --jsonThe device-code flow is the natural fit for "agent triggers, human
completes". With
--json,auth loginemits newline-delimited events:{"status":"authorization_required","userCode":"ABCD-1234","verificationUri":"https://…/activate","verificationUriComplete":"https://…/activate?user_code=ABCD-1234","expiresIn":899} // … blocks polling until a human authorizes in the browser … {"status":"authorized","expiresAt":1751990400,"expiresAtIso":"2025-07-08T12:00:00.000Z"} {"status":"device_bound"}The intended agent recipe: run it in the background, read the first line,
surface
verificationUriCompleteto the user, let them finish in the browser.The agent never completes auth.
--no-opensuppresses the browser launch forheadless boxes. Errors are
{"status":"error","code":"…","message":"…"}+ anon-zero exit; a missing region in
--jsonmode returnscode:"region_required"immediately rather than hanging.
3. Docs + help
--help(top-level andauth), the package README, andAGENTS.mdalldocument the new flags, the env var, and the agent auth recipe.
Verification
pnpm --filter stash test→ 349 passed (15 new for the region module).pnpm --filter stash test:e2e→ 35 passed (6 new non-interactivecases via the piped/non-TTY harness; the interactive-cancel test still
exercises the picker).
pnpm --filter stash lint→ clean.--no-open, bounded to 12s):stash auth login --region us-east-1 --json --no-openemitted a realauthorization_requiredevent and then blocked on polling exactly asdesigned (only a human can complete it).
Notes for the maintainer
.changeset/, outsidepackages/cli, and I was asked to confine changes to the CLI. Please add onebefore release:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation