stack-cli: explicit --cloud-project-id / --config-file across exec, config, project#1422
Open
BilalG1 wants to merge 9 commits into
Open
stack-cli: explicit --cloud-project-id / --config-file across exec, config, project#1422BilalG1 wants to merge 9 commits into
BilalG1 wants to merge 9 commits into
Conversation
Local-first dev workflow: `stack exec` now signs in as the emulator admin via the well-known shared credentials and the run-dir PCK, falling back to the cloud only when --cloud is passed (or STACK_EXEC_DEFAULT_TARGET=cloud is set). Also: STACK_EMULATOR_*_PORT env vars take precedence over the legacy unprefixed names; emulator paths/ports/PCK polling extracted to lib/emulator-paths.ts; shared local-emulator admin creds hoisted to stack-shared so backend, dashboard auto-login, and CLI agree.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughCentralizes local-emulator admin credentials in the shared package, adds emulator path/port utilities and PCK polling, implements local-emulator client/auth with retries, refactors CLI auth to be flag-less, adds explicit cloud/config targeting for exec, and updates tests and app/dashboard wiring. ChangesLocal Emulator CLI Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/stack-cli/src/lib/emulator-paths.ts (1)
79-91: ⚡ Quick winReplace
Date.now()withperformance.now()for elapsed-time measurement.
Date.now()is used here to compute a deadline and measure remaining time — exactly the use-case forperformance.now().As per coding guidelines: "Use
performance.now()instead ofDate.now()for measuring elapsed real time."
performanceis a global in Node.js 16+; if the project must support earlier Node, import it fromperf_hooks.♻️ Proposed fix
export async function pollInternalPck(timeoutMs: number): Promise<string | null> { const pckPath = internalPckPath(); - const deadline = Date.now() + timeoutMs; + const deadline = performance.now() + timeoutMs; let delay = 50; while (true) { try { const contents = readFileSync(pckPath, "utf-8").trim(); if (contents) return contents; } catch (e) { if ((e as NodeJS.ErrnoException).code !== "ENOENT") throw e; } - if (Date.now() >= deadline) return null; - const remaining = deadline - Date.now(); + if (performance.now() >= deadline) return null; + const remaining = deadline - performance.now(); await new Promise((r) => setTimeout(r, Math.min(delay, remaining))); delay = Math.min(delay * 2, 2000); } }🤖 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/stack-cli/src/lib/emulator-paths.ts` around lines 79 - 91, Replace the uses of Date.now() used for elapsed-time/deadline calculation in the polling loop inside emulator-paths.ts (the code that sets deadline = Date.now() + timeoutMs and checks Date.now() >= deadline and computes remaining) with performance.now() to measure elapsed time; ensure you either use the global performance (Node 16+) or import { performance } from "perf_hooks" at the top if globals are not available, and update all references (deadline, remaining calculation, and delay scheduling) so they consistently use performance.now() while leaving readFileSync(pckPath, "utf-8") and the retry/backoff logic unchanged.packages/stack-cli/src/commands/init.ts (1)
274-287: 💤 Low valueDrop the unused
_flagsparameter from these handlers.
_flagsis no longer threaded into auth resolution, and both call sites inrunInitalready passflagsonly because the signature still demands it. Removing the parameter (and the correspondingflagsargument at the call sites) makes the obsolete data flow disappear instead of relying on the underscore convention.♻️ Proposed fix
-async function handleCreateCloud(_flags: Record<string, unknown>, opts: InitOptions, outputDir: string): Promise<{ configPath?: string }> { +async function handleCreateCloud(opts: InitOptions, outputDir: string): Promise<{ configPath?: string }> { const sessionAuth = await ensureLoggedInSession(); ... } -async function handleLinkFromCloud(_flags: Record<string, unknown>, opts: InitOptions, outputDir: string): Promise<{ configPath?: string }> { +async function handleLinkFromCloud(opts: InitOptions, outputDir: string): Promise<{ configPath?: string }> { const sessionAuth = await ensureLoggedInSession(); ... }And update the call sites in
runInit(lines 130/140) to dropflags. Ifflagsis otherwise unused inrunInit, you can also drop theconst flags = program.opts();line.🤖 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/stack-cli/src/commands/init.ts` around lines 274 - 287, Remove the unused _flags parameter from both handler signatures (handleCreateCloud and handleLinkFromCloud) and update their call sites in runInit to stop passing flags; specifically, delete the first parameter in each function definition and remove the corresponding argument where they are invoked (and if runInit no longer uses flags at all, also remove the const flags = program.opts() declaration). Ensure only these two function names are changed so auth resolution no longer receives obsolete flag data.packages/stack-cli/src/lib/auth.ts (1)
217-220: 💤 Low valueAvoid silent catch-all on
res.text().
.catch(() => "")swallows any failure (transport error mid-stream, abort, etc.) without observability. Since you're already inside an error-reporting branch, prefer explicit handling that surfaces the read failure in the resulting message instead of pretending the body was empty.♻️ Proposed fix
if (!res.ok) { - const body = await res.text().catch(() => ""); - throw new AuthError(`Local emulator sign-in failed (${res.status} ${res.statusText})${body ? `: ${body}` : ""}. Make sure the emulator is running with NEXT_PUBLIC_STACK_IS_LOCAL_EMULATOR=true.`); + let body = ""; + let bodyReadError: unknown = null; + try { + body = await res.text(); + } catch (err) { + bodyReadError = err; + } + const detail = body + ? `: ${body}` + : bodyReadError + ? ` (failed to read response body: ${bodyReadError instanceof Error ? bodyReadError.message : String(bodyReadError)})` + : ""; + throw new AuthError(`Local emulator sign-in failed (${res.status} ${res.statusText})${detail}. Make sure the emulator is running with NEXT_PUBLIC_STACK_IS_LOCAL_EMULATOR=true.`); }As per coding guidelines, "NEVER try-catch-all, NEVER void a promise, and NEVER use
.catch(console.error)." and "errors are never silently swallowed".🤖 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/stack-cli/src/lib/auth.ts` around lines 217 - 220, The current catch-all on res.text() silences read failures; change the logic around the failed response branch so you attempt to read the body but surface any body-read error in the thrown AuthError instead of returning an empty string. Concretely, wrap the await res.text() call in a try/catch that captures the thrown error (e.g., bodyReadError) and include either the body or the bodyReadError.message/stack in the new AuthError message (reference res, res.text(), and AuthError) so transport/stream errors are visible in logs rather than being silently swallowed.
🤖 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 `@apps/e2e/tests/general/cli.test.ts`:
- Around line 325-371: Add a guard assertion to both negative emulator tests
("local-default exec errors when emulator PCK file is missing" and
"local-default exec errors when emulator API is unreachable"): before calling
runCli, add expect(createdProjectId).toBeDefined() (the same check used in the
positive create-project test) so the tests fail fast if createdProjectId is
undefined and we don't mis-interpret a missing project ID as an emulator error;
update the tests that reference createdProjectId and runCli to include this
single-line guard.
In `@packages/stack-cli/src/lib/auth.ts`:
- Around line 179-202: Replace the wall-clock based timing in
localEmulatorSignInWithRetry with a monotonic clock: change all uses of
Date.now() that compute deadline, remainingForRequest, remaining, and the
deadline comparison to use performance.now() (keep units in milliseconds and the
same arithmetic with totalTimeoutMs), e.g. compute deadline = performance.now()
+ totalTimeoutMs and use performance.now() in the checks and mins for
perRequestTimeoutMs and sleep calculation; ensure lastError logic and thrown
message remain unchanged and no other behavior is altered.
---
Nitpick comments:
In `@packages/stack-cli/src/commands/init.ts`:
- Around line 274-287: Remove the unused _flags parameter from both handler
signatures (handleCreateCloud and handleLinkFromCloud) and update their call
sites in runInit to stop passing flags; specifically, delete the first parameter
in each function definition and remove the corresponding argument where they are
invoked (and if runInit no longer uses flags at all, also remove the const flags
= program.opts() declaration). Ensure only these two function names are changed
so auth resolution no longer receives obsolete flag data.
In `@packages/stack-cli/src/lib/auth.ts`:
- Around line 217-220: The current catch-all on res.text() silences read
failures; change the logic around the failed response branch so you attempt to
read the body but surface any body-read error in the thrown AuthError instead of
returning an empty string. Concretely, wrap the await res.text() call in a
try/catch that captures the thrown error (e.g., bodyReadError) and include
either the body or the bodyReadError.message/stack in the new AuthError message
(reference res, res.text(), and AuthError) so transport/stream errors are
visible in logs rather than being silently swallowed.
In `@packages/stack-cli/src/lib/emulator-paths.ts`:
- Around line 79-91: Replace the uses of Date.now() used for
elapsed-time/deadline calculation in the polling loop inside emulator-paths.ts
(the code that sets deadline = Date.now() + timeoutMs and checks Date.now() >=
deadline and computes remaining) with performance.now() to measure elapsed time;
ensure you either use the global performance (Node 16+) or import { performance
} from "perf_hooks" at the top if globals are not available, and update all
references (deadline, remaining calculation, and delay scheduling) so they
consistently use performance.now() while leaving readFileSync(pckPath, "utf-8")
and the retry/backoff logic unchanged.
🪄 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: d7d488bb-4abd-4f8e-b7d7-f09370f56e56
📒 Files selected for processing (17)
apps/backend/src/lib/local-emulator.tsapps/dashboard/src/app/(main)/(protected)/layout-client.tsxapps/e2e/tests/general/cli.test.tspackages/stack-cli/src/commands/emulator.test.tspackages/stack-cli/src/commands/emulator.tspackages/stack-cli/src/commands/exec.test.tspackages/stack-cli/src/commands/exec.tspackages/stack-cli/src/commands/init.tspackages/stack-cli/src/commands/login.tspackages/stack-cli/src/commands/project.tspackages/stack-cli/src/lib/app.tspackages/stack-cli/src/lib/auth.test.tspackages/stack-cli/src/lib/auth.tspackages/stack-cli/src/lib/config.tspackages/stack-cli/src/lib/emulator-paths.test.tspackages/stack-cli/src/lib/emulator-paths.tspackages/stack-shared/src/local-emulator.ts
Contributor
Greptile SummaryThis PR makes
Confidence Score: 4/5Safe to merge after verifying the backend grants the emulator admin access to all local-emulator projects. The auth-flow refactor is well-structured and the unit/e2e test coverage is thorough. The two concerns worth a second look: (1) the positive e2e test signs in as the emulator admin but looks up a project created by the test user — if the backend's listOwnedProjects does not give the admin superuser project access, that test will always fail in local-emulator CI; (2) the per-phase timeout budget is applied independently to PCK polling and sign-in, so the actual wait can silently reach double the configured value, and the 0ms fast-fail path still fires one 1ms-timeout network attempt that can produce an 'aborted' message rather than a connection-specific one. Neither blocks the cloud path or any existing functionality.
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant exec.ts
participant auth.ts
participant emulator-paths.ts
participant LocalEmulator
User->>exec.ts: stack exec [--cloud] "js"
exec.ts->>exec.ts: resolveExecTarget(opts, env)
alt "--cloud or STACK_EXEC_DEFAULT_TARGET=cloud"
exec.ts->>auth.ts: resolveAuth(flags)
auth.ts-->>exec.ts: ProjectAuthWithRefreshToken (cloud creds)
else default: local emulator
exec.ts->>auth.ts: resolveLocalEmulatorAuth(flags)
auth.ts->>emulator-paths.ts: pollInternalPck(readyTimeoutMs)
emulator-paths.ts-->>auth.ts: internalPck (or null - throw AuthError)
auth.ts->>LocalEmulator: POST /api/v1/auth/password/sign-in (retry loop)
LocalEmulator-->>auth.ts: refresh_token
auth.ts-->>exec.ts: ProjectAuthWithRefreshToken (local creds)
end
exec.ts->>exec.ts: getAdminProject(auth)
exec.ts->>LocalEmulator: listOwnedProjects - find projectId
exec.ts->>exec.ts: new AsyncFunction(stackServerApp, js)
exec.ts-->>User: JSON result or error
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
- The positive happy-path test minted a project owned by the test user's team, but the CLI signs in as the local-emulator admin whose listOwnedProjects() only returns LOCAL_EMULATOR_OWNER_TEAM_ID-owned projects. Mint the project via /internal/local-emulator/project so it shows up under the admin's team. - Surface stderr when the positive test exits non-zero so future regressions report the real CLI error instead of a bare exit-code mismatch. - Add expect(createdProjectId).toBeDefined() guards to the two negative emulator tests for parity with the positive test. - Use performance.now() instead of Date.now() for the local-emulator sign-in retry deadline so wall-clock skew can't break the loop.
# Conflicts: # packages/stack-cli/src/commands/init.ts
- Use performance.now() in pollInternalPck for the polling deadline so
wall-clock skew can't break the loop, mirroring the same change in
localEmulatorSignInWithRetry.
- Surface response-body read failures in resolveLocalEmulatorAuth instead
of swallowing them with .catch(() => ""). The original message
("Local emulator sign-in failed (status text)") loses all diagnostic
info when res.text() itself throws; now we throw an AuthError that
includes the read error.
…urfaces Replaces the global --project-id / STACK_PROJECT_ID surface (and the local-default exec path added in this branch) with explicit per-command flags: - `stack exec` now requires exactly one of `--cloud-project-id <id>` or `--config-file <path>`. `--cloud` and STACK_EXEC_DEFAULT_TARGET are removed. `--config-file` resolves the local emulator project by absolute path via the existing GET /api/latest/internal/local-emulator/project endpoint. - `stack config pull/push` take `--cloud-project-id` instead of the global flag. `config pull --config-file` is optional and defaults to ./stack.config.ts in cwd, erroring with a clear hint when neither is present. - `stack project list` lists cloud + dev by default with a `target` field on each entry; `--cloud` / `--dev` filter to one source (mutually exclusive). Unreachable emulator on the default path emits a single stderr warning rather than failing. - `stack project create` now requires `--cloud` to make cloud-vs-local explicit. - Bumps the LIMIT on local-emulator project listing from 20 to 100 so `project list --dev` doesn't silently truncate.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/e2e/tests/general/cli.test.ts (1)
220-222: ⚡ Quick winAvoid introducing
anyin the new project-list assertion path.Type the parsed list once and let
findinfer item type instead of(p: any).♻️ Proposed fix
- const projects = JSON.parse(stdout); - const found = projects.find((p: any) => p.id === createdProjectId); + const projects: Array<{ id: string; displayName: string; target: "cloud" | "dev" }> = JSON.parse(stdout); + const found = projects.find((p) => p.id === createdProjectId);As per coding guidelines, "Try to avoid the
anytype. Whenever you need to useany, leave a comment explaining why you're using it..."🤖 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 `@apps/e2e/tests/general/cli.test.ts` around lines 220 - 222, The test currently uses an explicit any in the find callback; instead parse stdout into a typed array (e.g., assign JSON.parse(stdout) to a typed variable like projects: Project[] or inferred type) so the subsequent projects.find((p) => p.id === createdProjectId) can infer the item type and avoid (p: any). Update the declaration of projects (and import or declare the Project/test DTO type if needed) so the expect(found).toBeDefined() assertion uses a properly typed list rather than any.
🤖 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 `@apps/e2e/tests/general/cli.test.ts`:
- Around line 220-222: The test currently uses an explicit any in the find
callback; instead parse stdout into a typed array (e.g., assign
JSON.parse(stdout) to a typed variable like projects: Project[] or inferred
type) so the subsequent projects.find((p) => p.id === createdProjectId) can
infer the item type and avoid (p: any). Update the declaration of projects (and
import or declare the Project/test DTO type if needed) so the
expect(found).toBeDefined() assertion uses a properly typed list rather than
any.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b751821-599a-40d2-aa35-3f50d90cf379
📒 Files selected for processing (12)
apps/backend/src/app/api/latest/internal/local-emulator/project/route.tsxapps/e2e/tests/general/cli.test.tspackages/stack-cli/src/commands/config-file.test.tspackages/stack-cli/src/commands/config-file.tspackages/stack-cli/src/commands/emulator.tspackages/stack-cli/src/commands/exec.test.tspackages/stack-cli/src/commands/exec.tspackages/stack-cli/src/commands/project.test.tspackages/stack-cli/src/commands/project.tspackages/stack-cli/src/index.tspackages/stack-cli/src/lib/auth.tspackages/stack-cli/src/lib/local-emulator-client.ts
💤 Files with no reviewable changes (1)
- packages/stack-cli/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stack-cli/src/commands/emulator.ts
BilalG1
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reworks the
stackCLI surface so the cloud-vs-local choice is explicit at every invocation, removing the global--project-id/STACK_PROJECT_IDenv var and the local-defaultexecbehavior introduced earlier in this branch.stack exec--cloud,STACK_EXEC_DEFAULT_TARGET, and the implicit local default. The CLI now requires exactly one of:--cloud-project-id <id>— run against the Stack Auth cloud API--config-file <path>— run against the local emulator project mapped to that absolute config-file path--config-filebranch resolves the project id by calling the existingGET /api/latest/internal/local-emulator/projectendpoint and matchingabsolute_file_pathclient-side. No new backend endpoint introduced.stack config pull/stack config push--cloud-project-id <id>per-command instead of the global flag /STACK_PROJECT_IDenv.config pull --config-fileis optional: when omitted, the CLI uses./stack.config.tsfrom the current directory. If neither flag nor cwd file is present, it exits with a clear hint to pass--config-fileorcdinto a directory containingstack.config.ts.stack project listtarget: "cloud" | "dev"field (text format:<id>\t<displayName>\t[<target>]).--cloud/--devfilter to a single source (mutually exclusive — passing both errors).warning: skipping dev projects — local emulator not reachable …) and the command still succeeds with cloud results. With--devexplicit, the unreachable case hard-errors.stack project create--cloudto make the cloud-vs-local choice explicit. There is no local alternative today; the flag exists to surface the decision so a future local-project create doesn't silently change behavior.Backend
LIMITonGET /api/latest/internal/local-emulator/projectfrom 20 → 100 soproject list --devdoesn't silently truncate.Refactors (from earlier in this branch, unchanged here)
packages/stack-cli/src/lib/emulator-paths.ts.packages/stack-shared/src/local-emulator.ts.resolveAuth/resolveLocalEmulatorAuthtake an explicitprojectId: string(no moreFlagsparameter).packages/stack-cli/src/lib/local-emulator-client.tsencapsulates the GET-and-match flow used by bothexec --config-fileandproject list --dev.Breaking changes
Scripts that relied on any of the following must be updated:
--project-id <id>flag--cloud-project-id <id>STACK_PROJECT_IDenv var--cloud-project-id <id>stack exec --cloudstack exec --cloud-project-id <id>STACK_EXEC_DEFAULT_TARGET=cloud|local--cloud-project-id <id>or--config-file <path>stack execdefaulting to local emulator--config-file <path>requiredstack project createwithout a flagstack project create --cloud …requiredTest plan
pnpm lint(stack-cli, backend, e2e) — cleanpnpm --filter @stackframe/stack-cli typecheck— cleanpnpm --filter @stackframe/stack-cli exec vitest run— 72/72 passing (new unit tests:parseExecTarget,resolveConfigFilePathForPull,resolveProjectListSources,formatProjectList)pnpm test run apps/e2e/tests/general/cli.test.ts— 73 passing, 4 skipped, 0 failing. New e2e cases cover:execwith neither flag → errors with "Specify a target"execwith both flags → errors with "not both"exec --config-filewith missing file / missing PCK / unreachable APIexec --config-filehappy path against a real local-emulator backend (gated onNEXT_PUBLIC_STACK_IS_LOCAL_EMULATOR=true)config pullcwd fallback to./stack.config.tsconfig pullwith no--config-fileand no cwdstack.config.ts→ errors withPass --config-file …project list --cloud --devtogether → errorsproject listdefault with unreachable emulator → cloud results + single stderr warningproject createwithout--cloud→ errors--cloudexec cases ported to--cloud-project-idstack exec --help,stack project list --cloud --dev,stack project createall emit the expected friendly errors / help text.Summary by CodeRabbit
Release Notes
New Features
exec,config, andprojectcommands now require explicit targeting via--cloud-project-id(cloud) or--config-file(local emulator).project listnow supports--cloudand--devflags to display projects from both sources with target indicators.Bug Fixes
project listnow gracefully handles unreachable emulator with warning fallback instead of failure.Tests