feat: auto-claim keyless applications on login#157
feat: auto-claim keyless applications on login#157rafa-thayto wants to merge 10 commits intomainfrom
Conversation
rafa-thayto
force-pushed
the
rafa-thayto/auto-claim
branch
from
a56b83d to
d338623
Compare
🦋 Changeset detectedLatest commit: a688ed0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis pull request introduces automatic claiming of keyless applications during the login flow. After a keyless app is created during initialization (with temporary keys persisted via a breadcrumb file), the new Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
rafa-thayto
force-pushed
the
rafa-thayto/auto-claim
branch
from
d338623 to
76b9642
Compare
Add machine-readable error code for autoclaim failures, available for agent mode and CI error classification.
Add PLAPI client function for POST /v1/platform/accountless_applications/claim. Sends claim token and app name, returns the claimed Application.
Extract profile-building logic from autolink() into a reusable linkApp() function. This allows both autolink (key detection) and autoclaim (keyless claim) to share the same profile persistence code.
Add keyless.ts for managing accountless Clerk applications: - createAccountlessApp(): calls BAPI to create app with no auth - writeKeysToEnvFile(): writes framework-specific keys to .env.local - parseClaimToken(): extracts token from claim URL - Breadcrumb I/O: read/write/clear .clerk/keyless.json Includes 15 unit tests covering all functions and edge cases.
Add autoclaim.ts that detects keyless projects and claims them automatically after clerk auth login. Never throws — returns a discriminated union (claimed|not_found|already_claimed|failed|not_keyless) so the login flow is never interrupted. Key behaviors: - Truncates app name to 50 chars (backend limit) - Transient errors (5xx) preserve breadcrumb for retry - Terminal errors (404/403) clear breadcrumb Includes 8 unit tests covering all status paths.
When bootstrap mode skips authentication, create an accountless app via BAPI, write keys to .env.local, and store the claim token in .clerk/keyless.json for autoclaim on next login. Also simplifies the keyless info message to promote the autoclaim flow instead of requiring manual clerk link + env pull.
Wire autoclaim into the login flow with improved UX: - Specific warning messages per failure cause (expired token, missing org, transient error) instead of generic fallback - Contextual next-steps based on claim result: manual link for terminal failures, retry guidance for transient errors - Mock autoclaim in login tests to isolate from transitive deps
After a successful autoclaim, automatically run `env pull` to refresh .env.local with the claimed app's keys — no manual step needed.
rafa-thayto
force-pushed
the
rafa-thayto/auto-claim
branch
from
76b9642 to
55ea73e
Compare
- Extract shared plapiRequest<T> helper; collapse 6 PLAPI functions to 2-3 lines each (6 near-duplicate fetch blocks → one) - Promote errorMessage() from doctor/checks.ts to lib/errors.ts so it can be shared with autoclaim.ts and init/index.ts - Replace two TOCTOU file.exists()+read patterns in keyless.ts with atomic .text()/.json() catch-ENOENT variants - Parallelize detectPublishableKeyName + detectSecretKeyName in writeKeysToEnvFile (independent I/O) - Table-drive classifyClaimError (404/403 status → result mapping) - Drop file-level docstrings, section-header banners, and WHAT/ narration comments across new files per house style
wyattjoh
left a comment
wyattjoh
left a comment
There was a problem hiding this comment.
Code Review — PR #157
Reviewed with Opus + Codex second-opinion validation.
Blocker
B1. .clerk/keyless.json stores a live claim token with no .gitignore protection (codex confirmed)
packages/cli-core/src/lib/keyless.ts:88-106 writes claimToken under .clerk/ in the user's project. Nothing in this PR (or clerk init today) appends .clerk/ to the project's .gitignore. A developer can git add . and commit the token to a public repo, at which point any third party can claim the app before the original developer logs in, hijacking an app that will later hold production workloads.
Suggestion: in setupKeylessApp or writeKeylessBreadcrumb, append .clerk/ to .gitignore if not already present (mirroring what create-next-app and similar tools do). Add a test that verifies the .gitignore update.
B2. Keyless init writes .env.local but post-claim pull writes the framework default, producing two env files with drifting keys
This is a cross-file interaction with existing code rather than a change in this PR alone, but the PR is where the regression becomes user-visible:
packages/cli-core/src/lib/keyless.ts:57-76hardcodesconst targetFile = join(cwd, ".env.local").packages/cli-core/src/lib/framework.ts(unchanged) declaresenvFile: ".env"for Next.js, Astro, and Nuxt.packages/cli-core/src/commands/env/pull.ts:30-46(resolveTargetFile) prefers the framework-detected file.
Net effect on a Next.js project: .env.local written at init, .env written by pull after claim, same keys, different values.
Suggestion: use detectEnvFile(cwd) in writeKeysToEnvFile so there is one canonical target. Also update the JSDoc that references .env.local.
B3. Missing changeset
git diff main..HEAD -- .changeset/ is empty. This is a feat: PR touching @clerk/cli-core runtime. The Enforce Changeset workflow will block merge.
Major
M1. App name sent to backend uses basename(cwd).slice(0, 50) with no sanitization (codex confirmed)
packages/cli-core/src/lib/autoclaim.ts:32:
- Leaks potentially-sensitive directory names to the Clerk platform API.
.slice(50)truncates on UTF-16 code units; an emoji or non-BMP char at the boundary produces an orphaned surrogate (400 on strict UTF-8 validators).- Backend-invalid chars are not sanitized.
Suggestion: derive the name from package.json#name or the git repo name with basename(cwd) as a fallback, and sanitize + byte-length-truncate before sending.
M2. attemptAutoclaim(cwd) threads cwd but tryPullEnv ignores it
autoclaim.ts:37-50 accepts a cwd argument for the breadcrumb but calls pull({}), which uses process.cwd() internally (pull.ts:53). Today's single call site passes process.cwd() so it is benign, but this is fragile. Either thread cwd through pull options or assert at entry.
M3. createAccountlessApp has no timeout or abort; clerk init hangs on captive portal / slow BAPI (codex confirmed)
packages/cli-core/src/lib/keyless.ts:35-52 and packages/cli-core/src/commands/init/index.ts:272-287: fetch(...) has no AbortController. On slow/broken networks, withSpinner hangs indefinitely. log.debug is only visible with --verbose, so users see nothing.
M4. setupKeylessApp swallows all errors while success copy still renders (codex confirmed)
commands/init/index.ts:270-287 wraps the body in try { ... } catch { log.debug(...) }. Even on filesystem errors, parseClaimToken throws, or the fetch fails, printKeylessInfo still tells the user "Your app is ready with development keys in .env.local". Distinguish expected network failures from unexpected bugs and suppress the success message on failure.
M5. classifyClaimError semantic mismatch and aggressive breadcrumb-clear
autoclaim.ts:62-74 maps 403 to already_claimed, but the message at login.ts:128-132 says "your account does not have an active organization". Name and message disagree; rename the status (e.g. no_organization). Also, 404 unconditionally clears the breadcrumb; a transient DNS blip that returns 404 would destroy the retry signal.
Minor
readKeylessBreadcrumb(keyless.ts:108-119) has no schema validation; a malformed-but-JSON file yields{claimToken: undefined}andautoclaim.ts:33will POST{token: undefined}.parseClaimTokenuseshttps://placeholder.comas the synthetic URL base (keyless.ts:81-87); preferhttps://example.invalid(RFC 6761).- Em-dashes added in
login.ts:130-131,163andautoclaim.ts:7(global style rule). handleAutoclaimprintslog.successthenlog.warnthenoutro(login.ts:117-124,135-148); noisy ordering.- No retry for 5xx / 429 despite the design claim that "transient failures preserve breadcrumb for retry on next login" (
autoclaim.ts:28-36); a short in-process retry with backoff would save users a manual re-login.
Nits
CLAIM_WARNINGScould be an explicit object-literal map of the three warning statuses.log.info("Environment variables written to .env.local")will be wrong once B2 is fixed.- Test
expect(parsed.name).toBeTruthy()(autoclaim.test.ts:164) is too loose; assert the exact truncation. - CI refactor (composite action,
workflow_call) is bundled into afeat:PR; splitting would simplify bisect.
Positives
AutoclaimResult discriminated union is cleanly typed and enables exhaustive handling in loginNextSteps. attemptAutoclaim never throws (the type system enforces it). linkApp extraction in autolink.ts is a good refactor. Test coverage for the orchestrator is thorough: all status paths, breadcrumb clear/preserve semantics, env-pull success/failure, linkApp invocation, and request body shape.
B1: add .clerk/ to .gitignore in writeKeylessBreadcrumb to prevent
claim token from being committed to public repos
B2: use detectEnvFile(cwd) in writeKeysToEnvFile so init and claim
write to the same env file (e.g. .env for Next.js, not .env.local)
B3: add changeset (minor) for the autoclaim feature
M1: derive app name from package.json#name with basename fallback;
byte-length-truncate to 50 to avoid surrogate issues
M2: thread cwd through tryPullEnv -> pull() instead of relying on
process.cwd() internally
M3: add 15s AbortController timeout to createAccountlessApp to
prevent hanging on captive portal / slow BAPI
M4: move printKeylessInfo() inside try block so success copy is
suppressed on failure
M5: rename already_claimed -> no_organization to match the actual
403 semantic (no active organization); fix em-dashes in messages
nit: use https://example.invalid instead of https://placeholder.com
nit: add schema validation to readKeylessBreadcrumb
nit: tighten autoclaim test name assertion (exact value, not truthy)
nit: add gitignore idempotency test + wrong-shape breadcrumb test
docs(changeset): Automatically claim and link keyless applications on clerk auth login
wyattjoh
left a comment
wyattjoh
left a comment
There was a problem hiding this comment.
Code review
Substantial new surface (keyless.ts, autoclaim.ts) plus a clean plapi.ts refactor. Core design — discriminated AutoclaimResult, never-throws contract, breadcrumb-preserve-on-transient — is sound. A handful of issues worth tightening before merge: one user-facing inaccuracy (.env.local message), one misleading API parameter (pull({ cwd })), and a few error-handling edges (retry loops, corrupted breadcrumb recovery, silent keyless creation failures).
| " clerk auth login", | ||
| " clerk link", | ||
| " clerk env pull", | ||
| "\n Your app is ready with development keys in .env.local.", |
There was a problem hiding this comment.
[HIGH] printKeylessInfo hardcodes .env.local, but Next.js/Astro/Nuxt write to .env
The new message says Your app is ready with development keys in .env.local. But writeKeysToEnvFile (in lib/keyless.ts) resolves the target via detectEnvFile(cwd), which returns .env for Next.js, Astro, and Nuxt per FRAMEWORK_MAP. A Next.js user following this output will look in the wrong file.
Consider accepting the resolved envFile as a parameter (the init flow already has it in ctx.envFile) and interpolating it into the message. At minimum, replace the hardcoded .env.local with a generic phrase.
Evidence
heuristics.ts:
"\n Your app is ready with development keys in .env.local.",
framework.ts FRAMEWORK_MAP:
{ dep: "next", envFile: ".env", supportsKeyless: true },
{ dep: "astro", envFile: ".env", supportsKeyless: true },
{ dep: "nuxt", envFile: ".env", supportsKeyless: true },
| export async function pull(options: EnvPullOptions): Promise<void> { | ||
| const ctx = await resolveAppContext(options); | ||
| const cwd = process.cwd(); | ||
| const cwd = options.cwd ?? process.cwd(); |
There was a problem hiding this comment.
[HIGH] pull({ cwd }) threads cwd partially — resolveAppContext still uses process.cwd()
The new cwd option is honored for detectEnvFile, resolveTargetFile, and detectPublishableKeyName, but resolveAppContext(options) at line 52 still calls resolveProfile(process.cwd()) internally (see lib/config.ts:287). If a future caller passes a cwd that differs from process.cwd(), profile lookup silently uses the wrong directory.
Today the only consumer (autoclaim.attemptAutoclaim) passes process.cwd(), so there is no live bug — but the API is misleading. Either plumb cwd through resolveAppContext → resolveProfile, or drop the cwd option entirely and document that pull uses process.cwd(). The lean fix is the latter since the only caller happens to be same-cwd.
Evidence
pull.ts:
export async function pull(options: EnvPullOptions): Promise {
const ctx = await resolveAppContext(options); // <-- options.cwd ignored here
const cwd = options.cwd ?? process.cwd();
...
config.ts (resolveAppContext):
const resolved = await resolveProfile(process.cwd()); // hardcoded
|
|
||
| const APP_NAME_MAX_BYTES = 50; | ||
|
|
||
| const TERMINAL_BY_STATUS: Record<number, Terminal["status"]> = { |
There was a problem hiding this comment.
[MEDIUM] Non-404/403 PlapiErrors preserve breadcrumb → infinite retry on terminal 4xx
classifyClaimError only treats 404 and 403 as terminal. A 400 (invalid token), 409 (already claimed elsewhere), or 422 (validation) all fall through to failed, which preserves the breadcrumb and retries on every subsequent clerk auth login. The user sees the same "Auto-claim failed due to a temporary error" warning repeatedly with no path to recovery short of manually deleting .clerk/keyless.json.
Broaden TERMINAL_BY_STATUS to cover all 4xx except 408/429, or explicitly list 400, 409, 422 alongside 403/404. 5xx and network errors should remain retryable.
Evidence
const TERMINAL_BY_STATUS: Record<number, Terminal["status"]> = {
404: "not_found",
403: "no_organization",
};
function classifyClaimError(error: unknown): Terminal | Failed {
if (error instanceof PlapiError && error.status in TERMINAL_BY_STATUS) { ... }
// everything else -> Failed, breadcrumb preserved
return { status: "failed", error: err };
}
| export async function readKeylessBreadcrumb(cwd: string): Promise<KeylessBreadcrumb | undefined> { | ||
| try { | ||
| const data: unknown = await Bun.file(breadcrumbPath(cwd)).json(); | ||
| return isKeylessBreadcrumb(data) ? data : undefined; |
There was a problem hiding this comment.
[MEDIUM] Corrupted keyless.json is silently ignored forever
readKeylessBreadcrumb swallows parse errors and shape-validation failures, returning undefined. A corrupted keyless.json (truncated write, user edit, wrong shape) will cause every subsequent clerk auth login to report not_keyless silently — the user never learns why auto-claim is skipped.
When isKeylessBreadcrumb(data) returns false, call clearKeylessBreadcrumb(cwd) (and log.debug the reason). That way the file is recoverable by re-running clerk init --keyless, and the state doesn't persist indefinitely.
Evidence
export async function readKeylessBreadcrumb(cwd: string): Promise<KeylessBreadcrumb | undefined> {
try {
const data: unknown = await Bun.file(breadcrumbPath(cwd)).json();
return isKeylessBreadcrumb(data) ? data : undefined; // malformed file stays on disk
} catch {
return undefined;
}
}
| } catch (error) { | ||
| log.debug(`Could not create accountless app: ${errorMessage(error)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
[MEDIUM] setupKeylessApp swallows creation failures at debug log level
When createAccountlessApp fails (BAPI down, network error, timeout), the catch block only emits log.debug, which is suppressed without --verbose. The user sees zero output for the keyless step: no warning, no next-steps, nothing. printKeylessInfo() is never reached. The outro("Done") still prints. The user then runs bun dev and wonders why auth doesn't work.
Escalate the failure case to log.warn and print a brief recovery hint (clerk auth login then clerk link). log.debug is fine for the underlying error detail, but the user needs to know the keyless step failed.
Evidence
async function setupKeylessApp(cwd: string, frameworkDep: string): Promise {
try {
const app = await withSpinner("Creating development application...", () => ...);
await writeKeysToEnvFile(...);
await writeKeylessBreadcrumb(...);
printKeylessInfo();
} catch (error) {
log.debug(Could not create accountless app: ${errorMessage(error)}); // silent to the user
}
}
|
|
||
| if (!response.ok) { | ||
| const text = await response.text(); | ||
| throw new Error(`Failed to create accountless application (${response.status}): ${text}`); |
There was a problem hiding this comment.
[MEDIUM] createAccountlessApp bypasses the project's error-handling protocol
The BAPI call throws a plain new Error(...) with the raw status/body. Per .claude/rules/errors.md, API errors should use a typed error class that the global handler recognizes (BapiError/PlapiError) and be wrapped with withApiContext for user-facing context. As written, a non-OK response surfaces as Failed to create accountless application (500): <raw body> which is both unstyled and leaks response body directly.
Prefer BapiError if one exists (the rules doc references src/commands/api/bapi.ts), or wrap with an error class the global handler can format.
Evidence
if (!response.ok) {
const text = await response.text();
throw new Error(Failed to create accountless application (${response.status}): ${text});
}
| DOCTOR_FAILED: "doctor_failed", | ||
| /** Autoclaim of a keyless application failed. */ | ||
| AUTOCLAIM_FAILED: "autoclaim_failed", | ||
| } as const; |
There was a problem hiding this comment.
[LOW] AUTOCLAIM_FAILED error code added but never thrown
ERROR_CODE.AUTOCLAIM_FAILED is added to the enum but nothing in autoclaim.ts throws a CliError with it. The autoclaim design is explicitly "never throws" and uses a discriminated-union return type, so there is no consumer for this code. Dead code — either remove it or wire it into the failed branch's Error.
Evidence
errors.ts:
/** Autoclaim of a keyless application failed. */
AUTOCLAIM_FAILED: "autoclaim_failed",
autoclaim.ts never references ERROR_CODE.AUTOCLAIM_FAILED.
| } | ||
| return basename(cwd); | ||
| } | ||
|
|
There was a problem hiding this comment.
[LOW] truncateToByteLength uses byte-slice + lenient decode; consider char-based truncation
The current impl slices UTF-8 bytes, decodes with fatal: false, and strips a trailing U+FFFD replacement char. This works but is brittle: it creates an extra buffer copy and the intent is non-obvious. Per the PR body the backend limit is "50 chars". If that is codepoints (likely) rather than UTF-8 bytes, Array.from(str).slice(0, 50).join('') is both simpler and matches the "chars" framing.
Worth confirming with the backend team what the 50 actually counts — bytes, codepoints, or JS-string-length — and matching that exactly. Emoji/CJK app names are the edge case.
Evidence
const APP_NAME_MAX_BYTES = 50;
function truncateToByteLength(str: string, maxBytes: number): string {
const encoded = new TextEncoder().encode(str);
if (encoded.length <= maxBytes) return str;
return new TextDecoder("utf-8", { fatal: false })
.decode(encoded.slice(0, maxBytes))
.replace(/\uFFFD$/, "");
}
| } | ||
|
|
||
| export function parseClaimToken(claimUrl: string): string { | ||
| const base = claimUrl.startsWith("http") ? undefined : "https://example.invalid"; |
There was a problem hiding this comment.
[LOW] example.invalid base URL sentinel deserves a one-line comment
parseClaimToken uses https://example.invalid as a base URL so WHATWG URL can parse relative paths like /apps/claim?token=abc. The pattern is correct but the intent is non-obvious to readers who haven't hit the WHATWG URL relative-parsing quirk. A one-liner (// Need a dummy base because WHATWG URL rejects relative URLs without one) would save the next reader a confused minute.
Evidence
export function parseClaimToken(claimUrl: string): string {
const base = claimUrl.startsWith("http") ? undefined : "https://example.invalid";
const token = new URL(claimUrl, base).searchParams.get("token");
...
}
|
|
||
| return response.json() as Promise<Application>; | ||
| } | ||
|
|
There was a problem hiding this comment.
[LOW] include_secret_keys=true now serializes as the string "true"
fetchApplication previously called url.searchParams.set("include_secret_keys", "true"). The refactored version passes search: { include_secret_keys: true }, and applySearch handles value === true by writing "true". Behavior is identical to the old code (both produce ?include_secret_keys=true), but worth a targeted spot-check against a live PLAPI instance to confirm the backend parses the query string identically after the refactor — especially for the destructive flag in sendInstanceConfig, which is only set when truthy and now skipped via the value === undefined || value === false short-circuit in applySearch.
Evidence
function applySearch(url: URL, params: Record<string, SearchValue>): void {
for (const [key, value] of Object.entries(params)) {
if (value === undefined || value === false) continue;
...
url.searchParams.set(key, value === true ? "true" : value);
}
}
2 participants
Summary
keyless.ts): create accountless apps via BAPI, write framework-specific keys to.env.local, manage.clerk/keyless.jsonclaim token breadcrumbautoclaim.ts): afterclerk auth login, detect keyless breadcrumb and automatically claim the application via PLAPI — never throws, returns discriminated union so login is never interruptedclerk env pullstep neededlinkApp()fromautolink.tsfor reuse by both autolink (key detection) and autoclaim (keyless claim)claimApplication()PLAPI endpoint (POST /v1/platform/accountless_applications/claim)clerk initbootstrap when user skips authclerk auth loginwith contextual warnings and next-steps per failure modeHow it works
clerk init(skip auth) → creates accountless app, writes keys to.env.local, stores claim token in.clerk/keyless.jsonclerk auth login→ after OAuth, reads breadcrumb, calls claim endpoint, links app to account, auto-pulls env vars, clears breadcrumbCleanup pass (commit 6cda4f5)
Applied KISS/DRY simplification across the new surface and touched neighboring code where it shared the same patterns. Net: -121 lines (107 insertions, 228 deletions).
plapi.ts: extracted sharedplapiRequest<T>helper; 6 near-identical fetch-with-auth blocks (20 lines each) collapsed to 2–3 line call sites. Helper normalizes URL construction, query-param serialization, auth headers,PlapiErrorthrow, and JSON parsing.errors.ts: promotederrorMessage(unknown): stringout ofcommands/doctor/checks.tssoautoclaim.tsandinit/index.tscan reuse it instead of re-implementingerror instanceof Error ? error.message : String(error)inline.keyless.ts: replaced two TOCTOUBun.file(...).exists()+ read patterns with atomic.text().catch(() => "")/.json().catch(...); parallelized independentdetectPublishableKeyName+detectSecretKeyNamewithPromise.all; factored.env.local/.clerk/keyless.jsoninto named constants.autoclaim.ts: table-drivenclassifyClaimError— replaced back-to-backerror instanceof PlapiError && error.status === Nbranches with aTERMINAL_BY_STATUS: Record<number, Terminal["status"]>lookup.as never→as Profile, dropped thespies[]array in favor of explicit per-spymockRestore(), deleted redundant setup commentary.Test plan
clerk init -y→ skip auth → verify.env.localand.clerk/keyless.jsoncreated →clerk auth login→ verify app claimed, linked, and env vars refreshed