feat(cli): make stash env mint real deployment credentials (rc.2 B3)#682
Conversation
The command was a gated stub (STASH_EXPERIMENTAL_ENV_CMD) waiting on "the
CTS mint endpoint" — which turned out to already exist. It now mints
working credentials from the device-code session using the same APIs the
dashboard uses, all plain fetch against the session's own service URLs:
1. GET {cts}/api/workspaces → server-authoritative region → CS_WORKSPACE_CRN
2. POST {zerokms}/create-client → CS_CLIENT_ID / CS_CLIENT_KEY (hex-transcoded)
3. POST {cts}/api/access-keys → CS_CLIENT_ACCESS_KEY (member role, always —
the CLI deliberately cannot mint admin keys; role is omitted so the
server owns the default)
Design points:
- The ZeroKMS client is created before the access key, so a partial
failure leaves an inert client record, never an unaccounted-for live
credential.
- The key name resolves before any profile/network access, so the
non-interactive missing-name failure is credential-free — that is the
seam the new pty-less e2e exercises (minting live keys from a dev
machine's real session in tests is deliberately impossible).
- --write emits .env.production.local with mode 0600 and refuses to
overwrite non-interactively; --json emits one minted object or the
shared { status: 'error' } envelope.
Closes #663 (the wasm/edge credential gap, CIP-2997): edge runtimes that
cannot read ~/.cipherstash (containerised supabase functions serve,
Workers) now have a supported path — stash env + --env-file/secret store.
Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
🦋 Changeset detectedLatest commit: f1efc2d The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthrough
ChangesDeployment credential flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant stash env
participant Device session
participant CipherStash services
User->>stash env: Run with --name and output options
stash env->>Device session: Obtain authenticated token
Device session-->>stash env: Return bearer token
stash env->>CipherStash services: Fetch workspace and create ZeroKMS client
CipherStash services-->>stash env: Return client credentials
stash env->>CipherStash services: Create member access key
CipherStash services-->>stash env: Return access-key credentials
stash env-->>User: Print or write CS_* variables
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/cli/src/commands/env/__tests__/env.test.ts (1)
143-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
as nevercast from theprocess.exitmock.
Type the mock callback to matchprocess.exitdirectly; if the exit-code type needs widening, updateExitSignalinstead of erasing the callback type.🤖 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/env/__tests__/env.test.ts` around lines 143 - 145, Update the process.exit mock setup in the env tests to use a callback typed directly as process.exit, removing the as never cast. If the callback’s exit-code type is incompatible, widen the ExitSignal constructor or definition rather than suppressing the type mismatch.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.
Inline comments:
In `@packages/cli/src/commands/env/index.ts`:
- Around line 89-98: Move the existing `.env.production.local` write-destination
and interactive-overwrite check from the post-`mintCredentials` path to before
the credential-minting flow in the command handler. Preserve the current refusal
behavior for non-interactive existing files and declined overwrites, and ensure
these preflight failures return without invoking any API or credential-mutation
calls.
- Around line 322-335: The apiFetch helper currently allows credential requests
to hang indefinitely. Add a bounded AbortSignal timeout to the fetch call in
apiFetch, and convert timeout or abort failures into a clear MintError while
preserving normal responses and other request behavior.
- Around line 109-110: Update the file-writing flow around writeFileSync to
explicitly enforce permissions with chmodSync or fchmodSync after writing,
including when an existing 0644 file is overwritten. Add or update tests to
cover the confirmed-overwrite case and verify the resulting mode is 0600.
- Around line 47-56: Update the env command’s Clack UI calls—`p.intro`, the
spinner started after `resolveKeyName`, and the corresponding `p.outro`—so they
are only used when `isInteractive()` is true or are directed to stderr. Preserve
clean dotenv output on stdout for piped, non-JSON execution.
In `@skills/stash-cli/SKILL.md`:
- Line 502: Specify the fenced code block language in the relevant section of
SKILL.md by changing the unlabeled fence to use the dotenv language identifier,
while preserving the existing environment-variable example content.
---
Nitpick comments:
In `@packages/cli/src/commands/env/__tests__/env.test.ts`:
- Around line 143-145: Update the process.exit mock setup in the env tests to
use a callback typed directly as process.exit, removing the as never cast. If
the callback’s exit-code type is incompatible, widen the ExitSignal constructor
or definition rather than suppressing the type mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a27040b-f47e-4867-a274-17c2c10f1dc6
📒 Files selected for processing (9)
.changeset/stash-env-mint-credentials.mdpackages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/env/__tests__/env.test.tspackages/cli/src/commands/env/index.tspackages/cli/src/messages.tspackages/cli/tests/e2e/env-non-interactive.e2e.test.tsskills/stash-cli/SKILL.mdskills/stash-supabase/SKILL.md
Live smoke ✅ (as promised in the PR body)Ran against my real workspace with a disposable key name: node packages/cli/dist/bin/stash.js env --name stash-env-smoke-20260717 --jsonFirst-shot success (exit 0), minted (secrets redacted): {
"status": "minted",
"keyName": "stash-env-smoke-20260717",
"workspaceCrn": "crn:us-west-2.aws:5QSZTRROIQGIKBOH",
"clientId": "c8157833-8b28-4a1c-82d0-418a16abb576",
"clientKey": "<hex, 336 chars — REDACTED>",
"accessKey": "CSAK…<69 chars — REDACTED>"
}Then proved the credentials on the surface B3 is about — Notes:
Cleanup: the access key + ZeroKMS client |
The Configuration section listed the four CS_* variables but left how to obtain them to the dashboard. Both the stash-cli and stash-supabase skills now document `stash env`; this closes the loop in the skill the other two reference for configuration. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
There was a problem hiding this comment.
Pull request overview
This PR turns stash env from an experimental stub into a real “deployment credentials minting” command for the stash CLI, and updates the shipped Skills docs to describe the new workflow (including edge/WASM runtimes).
Changes:
- Implement
stash envto mintCS_WORKSPACE_CRN,CS_CLIENT_ID,CS_CLIENT_KEY(hex), andCS_CLIENT_ACCESS_KEYfrom the local device-session token (CTS + ZeroKMS calls). - Add unit + e2e coverage for non-interactive behavior, JSON output envelopes, ordering guarantees, and safe
--writesemantics (0600, no overwrite non-interactively). - Update
skills/*docs, CLI registry metadata, help output, and add astashchangeset for the new command behavior.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/stash-supabase/SKILL.md | Documents stash env as the recommended credential path for Supabase Edge Functions (local + deploy). |
| skills/stash-encryption/SKILL.md | Adds guidance for minting CS_* via stash env and notes edge/WASM constraints. |
| skills/stash-cli/SKILL.md | Replaces “stub” docs with real env command docs, examples, and behavioral notes. |
| packages/cli/tests/e2e/env-non-interactive.e2e.test.ts | Adds non-TTY e2e coverage for missing --name and JSON error envelope. |
| packages/cli/src/messages.ts | Introduces a stable, test-asserted missing-name message leader for stash env. |
| packages/cli/src/commands/env/index.ts | Implements the actual minting flow (workspaces → create-client → access-keys), output modes, and safety behaviors. |
| packages/cli/src/commands/env/tests/env.test.ts | Adds unit tests for happy path, ordering, error mapping, JSON mode, and --write file permissions/overwrite refusal. |
| packages/cli/src/cli/registry.ts | Promotes env into “Deployment”, adds long help, examples, and flags (--name, --write, --json). |
| packages/cli/src/bin/main.ts | Updates help text and wires --name/--json/--write into envCommand. |
| .changeset/stash-env-mint-credentials.md | Adds a minor changeset describing the new stash env behavior and constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…verified Ran the flow for real (agent starts `auth login --json --region us-west-2` backgrounded, human approves the relayed URL): authorization_required → authorized → device_bound, exit 0, session verified via getToken. The event table in stash-cli was already accurate; what was missing was the part that bites an agent — the command blocks polling for up to ~15 minutes, so it must run as a background task with a generous timeout, and success is confirmed from the event stream (never by inspecting ~/.cipherstash). stash-encryption's Local Development section now points agents at the flow. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
An 8-angle adversarial review of this branch confirmed 10 defects (plus one
skill nit); this commit fixes all of them:
1. --write's overwrite decision (refusal AND interactive confirm) now runs
BEFORE minting — a declined overwrite previously discarded a live,
shown-exactly-once access key, contradicting the file's own contract.
2. Stdout is now actually pipe-clean: every clack call routes chrome to
stderr via { output: process.stderr }, so `stash env > prod.env` and
pipes into dotenv consumers capture only the block. (clack defaults
everything to stdout — the old comment's claim was wrong.)
3. This also fixes prompts rendering into redirected stdout (isInteractive
is stdin-only): prompts on stderr stay visible on the terminal.
4. `--write <path>` is now a feature, not a parse trap: parseArgs put the
path in values.write leaving flags.write undefined, silently printing
secrets to stdout. main.ts passes values.write ?? flags.write and the
command accepts a target path.
5. `--name` followed by another flag gets its own name_requires_value
diagnostic instead of a false missing_name.
6. `stash env <positional>` is rejected with did-you-mean --name guidance
instead of silently vanishing into the subcommand slot.
7. --json + --write now compose: the file is written and the JSON
confirmation ({ status: 'written' }) is deliberately secret-free.
8. 0600 is enforced on overwrite too (writeFileSync's mode only applies on
creation) via an explicit chmod.
9. All three API responses are zod-validated; non-base64 client_key is
refused before Node's lenient decoder can transcode garbage; a service
shape change can no longer print undefined into a credentials file.
10. The member role is pinned in the request (wire value verified lowercase
in cts-common) AND asserted on the response — a non-member key is
refused with revocation guidance, so the documented 'cannot mint admin
keys' invariant is enforced, not assumed.
11. All exits go through CliExit (never deep process.exit), so run()
records env outcomes for telemetry; cancel paths use CliExit(0). The
three divergent dead-code exit trailers are gone.
12. skills/stash-cli: the bare --env-file mention is attributed to
`supabase functions serve`, per the manifest-resolution rule.
Tests: 21 env unit tests (preflight-before-mint asserted via zero fetch
calls; chrome-on-stderr; role pinning; validation refusals; json+write
secret-free event; chmod-on-overwrite) and 4 e2e cases (all credential-free
pre-mint failures, including asserting error chrome lands on stderr, not
stdout). Full suite: 534 unit / 62 e2e green, code:check exit 0.
Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Branch review → all findings fixed (
|
| # | Finding | Fix |
|---|---|---|
| 1 | --write overwrite check ran after minting → a refusal/decline discarded a live, shown-once access key |
Overwrite decision (refusal and interactive confirm) moved before the mint; tests assert zero fetch calls on the refusal paths |
| 2 | Stdout wasn't pipe-clean — clack chrome defaults to stdout, corrupting stash env > prod.env |
All chrome routed to stderr via { output: process.stderr }; e2e asserts error text lands on stderr only |
| 3 | Prompts rendered into redirected stdout (stdin-only isInteractive) |
Solved by the same stderr routing — prompts stay visible when stdout is redirected |
| 4 | --write <path> parse trap: path swallowed into values, secrets printed to stdout, exit 0 |
Now a feature — --write [path] targets that file (values.write ?? flags.write) |
| 5 | --name --json → false missing_name |
Dedicated name_requires_value diagnostic |
| 6 | stash env my-app positional silently vanished into the subcommand slot |
Rejected with did-you-mean --name my-app guidance |
| 7 | --json --write silently dropped --write, secrets into captured stdout |
They compose: file written, JSON confirmation { status: \"written\" } is deliberately secret-free |
| 8 | mode: 0600 not applied when overwriting an existing 0644 file |
Explicit chmodSync after every write |
| 9 | Bare as-casts on all three API responses → crn:undefined / CS_CLIENT_ACCESS_KEY=undefined with exit 0 |
zod-validated (loose list, strict on our workspace); non-base64 client_key refused before Node's lenient decoder can emit corrupt hex |
| 10 | 'Cannot mint admin keys' enforced only by the server default | role: 'member' pinned in the request (wire value verified lowercase in cts-common) and asserted on the response — a non-member key is refused with revocation guidance |
| 11 | Deep process.exit → env failures invisible to telemetry; 3 divergent dead-code exit idioms |
All exits via CliExit (verified unwindable to run()); cancels are CliExit(0) |
Plus the skill nit: the bare --env-file mention in stash-cli is now attributed to supabase functions serve, per the AGENTS.md manifest-resolution rule.
Coverage grew 10 → 21 unit tests + 2 → 4 e2e cases; full suite 534 unit / 62 e2e green, code:check clean, manifest resolves --name <name> / --write [path] / --json.
…doc nits) - apiFetch now carries a 30s AbortSignal.timeout; timeouts surface as request_timeout and connection failures as network_error (naming the host, and the undici cause rather than the opaque 'fetch failed'). A stalled CTS/ZeroKMS endpoint no longer hangs the CLI. (CodeRabbit) - The credential name rejects control characters (invalid_name) — a newline could break out of the dotenv comment line and inject a line into a written credentials file. Enforced pre-mint for both the flag and the interactive prompt. (Copilot) - skills/stash-cli: dotenv language on the output fence (MD040); skills/stash-supabase: keep the example command in one code span. Unit tests: 24 env tests (invalid_name pre-profile, request_timeout, network_error). Suite 537/62 green, code:check clean. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Overview
Turns stash env from a gated stub into a working credential-minter: it reuses the device-code session token to call the same CTS + ZeroKMS endpoints the dashboard uses, and emits CS_WORKSPACE_CRN / CS_CLIENT_ID / CS_CLIENT_KEY / CS_CLIENT_ACCESS_KEY to stdout, a 0600 file (--write), or NDJSON (--json). Closes the rc.2 B3 blocker (#663) and gives WASM/edge runtimes that can't read ~/.cipherstash a supported credential path.
This is security-sensitive code (it mints live credentials), and it's written with the care that warrants. Approving.
Why this is solid
- Pre-mint refusal ordering is load-bearing and correct. Every refusable condition — stray positional, missing/valueless
--name, control-char name, and the--writeoverwrite decision — resolves beforemintCredentialsruns, so a refusal can never discard a shown-exactly-once access key. The e2e suite pins that the missing-name path never touches the profile or network, which structurally prevents a CI run from minting real keys. - Mint ordering minimizes blast radius. ZeroKMS client is created before the access key, so the only partial-failure leftover is an inert client (and the error names it). Correct call sequence is asserted in tests.
- Least privilege, enforced both ways.
role: 'member'is pinned in the request and asserted on the response; there's deliberately no--role, and an over-privileged response is refused without printing the secret. Good defense against a server/default drift. - Every cross-boundary response is schema-validated (
zod) before any field reaches the env block — the comment correctly notes anas-cast would printundefinedinto a credentials file after a live key was minted. Thecreate-clientkey material is validated as padded base64 before Node's lenient decoder can transcode garbage into a plausible-but-wrong hex key. - Secret hygiene is thorough. Bearer token never reaches stdout (asserted); stdout is pipe-clean (all chrome → stderr, spinner suppressed in
--json);--json --writeconfirmation is deliberately secret-free;writeEnvFilere-chmods0600after write becausewriteFileSync'smodeonly applies on create. Control-char rejection on the name closes the dotenv-comment injection vector. CliExitinstead ofprocess.exitsorun()records telemetry; cancel paths exit 0, failures exit 1, single-emit. Timeouts (AbortSignal.timeout) and network/timeout error mapping name the host.- Tests (10 unit + 4 e2e) are excellent and cover the security-relevant invariants directly. Skills (
stash-cli,stash-encryption,stash-supabase) and the help/registry surface are updated in-PR;stashminor changeset present.
Notes — all minor, none blocking
-
CRN assembly is the one thing only a live mint validates.
crn:${region}:${workspaceId}is new, and if that string shape doesn't match what the SDK'sCS_WORKSPACE_CRNparser expects, the minted creds fail only at deploy time — past all the unit mocks. Good that the PR flags a redacted live smoke test before merge; please make sure that smoke test actually uses the emitted CRN to init a client, not just eyeballs it. -
Role assertion is skipped when the response omits
role.if (accessKeyBody.role && …)— an absentrolefield bypasses the member check. Acceptable because the request pinsmemberand the server owns the default, but since the docs promise "verified on the response," consider treating missingroleas an explicit member assumption (comment or?? 'member') so the guarantee doesn't depend on the field always being present. -
The "if the name is already taken, rerun with
--name" hint is appended to every non-403 access-key error, not just duplicates (there's no duplicate-specific status check). Harmless, but a 500 would carry a slightly misleading suggestion. Cosmetic. -
Defense-in-depth (optional): the emitted dotenv values (region-derived CRN, access key) aren't newline-validated the way the name is. A malformed server-provided
regioncould in theory break the block. Server-trust makes this very low risk, but a one-line assertion that no emitted value contains a newline would fully close the injection surface.
Verdict
Approve. Careful, least-privilege, well-tested implementation of a sensitive command; the failure/ordering design is exactly right. The notes are polish — item 1 (confirm the CRN round-trips through a real client in the pre-merge smoke) is the only one worth acting on before shipping.
- Note 2: treat an ABSENT access-key role as member (server default) rather than skipping the check — `(role ?? 'member')` — so the documented 'verified on the response' guarantee doesn't depend on the field always being present. - Note 3: only append the 'name already taken, use --name' hint on 400/409 (where duplicates land), not every non-403 — a 500 no longer carries a misleading suggestion. - Note 4 (defense-in-depth): reject any server-provided emitted value (CRN/region, client id, access key) that contains a control character, closing the dotenv line-injection surface the name guard already covered. clientKey is hex by construction. Tests: absent-role → success; 500 omits the --name hint (keeps the leftover-client note); a newline in the server region → refused, no injected line on stdout. 27 env unit tests, suite 540/62 green. Note 1 (CRN round-trip) needs no code change — the pre-merge live smoke already inits a wasm-inline client with the emitted workspaceCrn (see PR comment); replying there. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
|
Thanks James — addressed all four in 2. Absent role skips the member check → fixed. Now 3. Duplicate-name hint on every non-403 → fixed. The 'name already taken, use --name' hint is now gated on 4. Emitted values not newline-validated → fixed. After assembling the creds, any server-provided value (CRN/region, client id, access key) containing a C0/DEL control char is refused with 1. CRN round-trip — no code change needed, and I want to confirm it's genuinely covered, not eyeballed: the live smoke fed the emitted Also: CodeRabbit's one nitpick ( Suite: 27 env unit tests, 540 unit / 62 e2e green, |
What
stash envwas a gated stub (STASH_EXPERIMENTAL_ENV_CMD) whose TODO said the CTS mint endpoint "isn't wired up yet." That comment was stale — both endpoints exist and are what the dashboard itself uses. The command now mints working deployment credentials from the local device-code session and prints them as env vars:Output:
CS_WORKSPACE_CRN,CS_CLIENT_ID,CS_CLIENT_KEY,CS_CLIENT_ACCESS_KEY.How
All plain
fetch, no new native code —DeviceSessionStrategy.fromProfile().getToken()already exposes the bearer token, workspace id, CTS base URL (issuer), and ZeroKMS URL (services.zerokms):GET {cts}/api/workspaces→ the workspace's server-authoritative region → CRN (works for self-hosted CTS; no issuer-hostname parsing)POST {zerokms}/create-client→ a fresh, dedicated client id/key for the deployment (not the dev machine'ssecretkey.json) — key material transcoded base64→hex, the format every released SDK acceptsPOST {cts}/api/access-keys→ the access key, member role always:roleis omitted so the server owns the default, and the CLI deliberately has no--role— admin keys belong in the dashboard where they're visibleDeliberate ordering/safety properties:
--name;--writerefuses to overwrite non-interactively and writes 0600.Why (rc.2 B3, #663, CIP-2997)
Closes #663. The wasm-inline surface has no
~/.cipherstashfallback, and for the headline case it never can:supabase functions serveruns in a container that doesn't mount the host home dir (verified against a live edge-runtime container — no binds), and Workers have no fs.stash envgives every such runtime — plus production, per CIP-2997's original intent — one credential path: mint a key, feed it via--env-fileor the platform secret store.Verification
--writemode bits,--jsonenvelopes, token-never-on-stdoutpnpm --filter stash test523+10 green;code:checkexit 0skills/stash-cli(stub section → real docs) andskills/stash-supabase(edge-functions credential path) updated in the same PR;stash manifest --jsonresolves every flag the skill namesstashminorNot exercised here: a live mint against a real workspace (needs my session; will do as a manual smoke before merge and paste the redacted output).
https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Summary by CodeRabbit
New Features
stash envproduction-ready to mint deployment credentials and outputCS_WORKSPACE_CRN,CS_CLIENT_ID,CS_CLIENT_KEY, andCS_CLIENT_ACCESS_KEY.--writeto save credentials to.env.production.localwith owner-only permissions (0600) and safe overwrite behavior, plus--jsonNDJSON output.envunder a new Deployment section;--nameis required for non-interactive runs.Documentation
Tests
--name).