From 9db08e7d597cb5671a137b77c966fca1f94d2036 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Fri, 24 Jul 2026 15:41:49 -0500 Subject: [PATCH 1/3] feat!: stop the installer from clobbering configured AuthKit projects `npx workos` provisioned a fresh WorkOS environment and rewrote the project's env file before the installer state machine existed, then read back its own write when it later checked for existing credentials. In a project that already had AuthKit wired up, that replaced the real credentials and destroyed every comment in the file, and a failed run left all of it in place with no rollback. Reported against 0.18.0 by a customer whose Next.js app already had @workos-inc/authkit-nextjs, custom middleware, and SCIM/SSO reconciliation wired up. - Add a preflight guard (src/lib/preflight-authkit.ts) ahead of credential resolution in all three install entry points: it prompts on an interactive TTY, exits non-zero in agent/CI/JSON mode, and takes `--force` as the override. Matches AUTHKIT_PACKAGES only, so @workos-inc/node alone does not trip it. - Read the project's env file before provisioning, and refuse again at the write site, so a project that already has WORKOS_API_KEY is never provisioned over even by a direct caller. - Preserve comments, blank lines, and key order on every env write, and leave one git-ignored backup of the pre-CLI file. The backup is gitignored before it is written: `stageAndCommit` runs `git add -A`, so an unignored backup would commit a live API key. - Read the homepage URL before overwriting it, and state credential provenance in the dashboard-config output, so an unclaimed throwaway environment cannot be mistaken for production. - Classify deterministic gateway failures separately from transient ones, in one shared classifier instead of three duplicated regexes, so users stop being told to retry a failure that recurs every time. - Raise the 120s upstream socket timeout to 600s in both proxy factories. The reported path uses the claim-token proxy, so fixing only one was fixing neither. BREAKING CHANGE: `workos install` now stops in a project that already has an AuthKit SDK installed. Pass `--force` to continue. --- src/bin.ts | 31 +- src/doctor/checks/sdk.ts | 8 +- src/lib/adapters/cli-adapter.spec.ts | 50 +++ src/lib/adapters/cli-adapter.ts | 45 +-- src/lib/adapters/headless-adapter.ts | 48 +-- src/lib/agent-interface.spec.ts | 34 +- src/lib/agent-interface.ts | 40 ++- src/lib/credential-proxy.spec.ts | 107 +++++++ src/lib/credential-proxy.ts | 14 +- src/lib/env-writer.spec.ts | 330 ++++++++++++++++++-- src/lib/env-writer.ts | 136 +++++--- src/lib/failure-classifier.spec.ts | 102 ++++++ src/lib/failure-classifier.ts | 144 +++++++++ src/lib/preflight-authkit.spec.ts | 192 ++++++++++++ src/lib/preflight-authkit.ts | 79 +++++ src/lib/project-env.ts | 46 +++ src/lib/resolve-install-credentials.spec.ts | 86 +++++ src/lib/resolve-install-credentials.ts | 15 +- src/lib/run-with-core.ts | 24 +- src/lib/unclaimed-env-provision.spec.ts | 45 +++ src/lib/unclaimed-env-provision.ts | 13 + src/lib/workos-management.spec.ts | 282 +++++++++++++++++ src/lib/workos-management.ts | 83 ++++- 23 files changed, 1777 insertions(+), 177 deletions(-) create mode 100644 src/lib/credential-proxy.spec.ts create mode 100644 src/lib/failure-classifier.spec.ts create mode 100644 src/lib/failure-classifier.ts create mode 100644 src/lib/preflight-authkit.spec.ts create mode 100644 src/lib/preflight-authkit.ts create mode 100644 src/lib/project-env.ts create mode 100644 src/lib/workos-management.spec.ts diff --git a/src/bin.ts b/src/bin.ts index a41d3922..2e723e43 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -145,6 +145,20 @@ const insecureStorageOption = { }, } as const; +/** + * Shared override for the "AuthKit is already installed" preflight guard. + * + * Distinct from `--force-install` below, which only relaxes peer-dependency + * checks during package installation. + */ +const forceOption = { + force: { + default: false, + describe: 'Continue even if AuthKit is already installed in this project', + type: 'boolean' as const, + }, +} as const; + const installerOptions = { direct: { alias: 'D', @@ -251,6 +265,7 @@ const installerOptions = { describe: 'Next.js router to target when detection is ambiguous (app or pages)', type: 'string' as const, }, + ...forceOption, }; // Check for updates (blocks up to 500ms, skip in JSON/non-human modes to keep machine streams clean) @@ -2551,6 +2566,11 @@ async function runCli(): Promise { (yargs) => yargs.options(installerOptions), async (argv) => { await applyInsecureStorage(argv.insecureStorage); + // MUST run before credential resolution below: that provisions a WorkOS + // environment and writes its credentials into the project's env file, + // so a guard placed after it is no guard at all. + const preflight = await import('./lib/preflight-authkit.js'); + await preflight.assertNoExistingAuthKit({ installDir: argv.installDir ?? process.cwd(), force: argv.force }); await resolveInstallCredentials(argv.apiKey, argv.installDir, argv.skipAuth, ensureAuthenticated); const { handleInstall } = await import('./commands/install.js'); await handleInstall(argv); @@ -2750,6 +2770,9 @@ async function runCli(): Promise { (yargs) => yargs.options(installerOptions), async (argv) => { await applyInsecureStorage(argv.insecureStorage); + // Guard first, before credential resolution — see the `install` handler above. + const preflight = await import('./lib/preflight-authkit.js'); + await preflight.assertNoExistingAuthKit({ installDir: argv.installDir ?? process.cwd(), force: argv.force }); await resolveInstallCredentials(argv.apiKey, argv.installDir, argv.skipAuth, ensureAuthenticated); const { handleInstall } = await import('./commands/install.js'); await handleInstall({ ...argv, dashboard: true }); @@ -2758,7 +2781,9 @@ async function runCli(): Promise { .command( ['$0'], 'WorkOS AuthKit CLI', - (yargs) => yargs.options(insecureStorageOption), + // `--force` must be registered here too: this parser is .strict(), so + // `npx workos --force` would die as an unknown argument otherwise. + (yargs) => yargs.options({ ...insecureStorageOption, ...forceOption }), async (argv) => { // Non-human modes: emit machine-readable command tree (JSON) or the // fully-configured parser help (human non-TTY edge) instead of prompting. @@ -2782,6 +2807,10 @@ async function runCli(): Promise { } await applyInsecureStorage(argv.insecureStorage); + // After the confirm above (two prompts back to back is worse UX), but + // still before credential resolution touches the project. + const preflight = await import('./lib/preflight-authkit.js'); + await preflight.assertNoExistingAuthKit({ installDir: process.cwd(), force: argv.force }); await resolveInstallCredentials(undefined, undefined, false, ensureAuthenticated); const { handleInstall } = await import('./commands/install.js'); diff --git a/src/doctor/checks/sdk.ts b/src/doctor/checks/sdk.ts index b905e175..92b9dbda 100644 --- a/src/doctor/checks/sdk.ts +++ b/src/doctor/checks/sdk.ts @@ -18,7 +18,13 @@ const SDK_PACKAGES = [ 'workos', // very old legacy ] as const; -const AUTHKIT_PACKAGES = new Set([ +/** + * AuthKit SDKs only — deliberately excludes `@workos-inc/node` and legacy + * `workos`, which are present in plenty of projects that never installed + * AuthKit. Consumed by the install preflight guard (`lib/preflight-authkit.ts`), + * where matching the base SDK would block legitimate first-time installs. + */ +export const AUTHKIT_PACKAGES = new Set([ '@workos/authkit-tanstack-react-start', '@workos/authkit-sveltekit', '@workos-inc/authkit-nextjs', diff --git a/src/lib/adapters/cli-adapter.spec.ts b/src/lib/adapters/cli-adapter.spec.ts index 3d3981ab..6ef14446 100644 --- a/src/lib/adapters/cli-adapter.spec.ts +++ b/src/lib/adapters/cli-adapter.spec.ts @@ -366,6 +366,56 @@ describe('CLIAdapter', () => { }); }); + describe('error rendering', () => { + async function renderError(message: string): Promise { + await adapter.start(); + const ui = await import('../../utils/ui.js'); + + emitter.emit('error', { message, stack: undefined }); + + return [...vi.mocked(ui.default.log.error).mock.calls, ...vi.mocked(ui.default.log.info).mock.calls] + .map((c) => String(c[0])) + .join('\n'); + } + + // The gateway's generic 500 fires for deterministic request failures too, so + // this is the interactive copy the reporter hit: four more minutes, same 500. + it('does not advise waiting for the gateway generic 500', async () => { + const output = await renderError( + 'API Error: 500 {"error":{"type":"internal_error","message":"An unexpected error occurred"}}', + ); + + expect(output).not.toMatch(/temporarily unavailable/i); + expect(output).not.toMatch(/few minutes|try again shortly|wait a minute/i); + expect(output).toMatch(/could not complete this request/i); + }); + + // installer-core re-emits runAgent's already-rendered message, so the + // adapter classifies our own copy on the real path — not the raw SDK text. + it('keeps the deterministic copy when handed an already-rendered message', async () => { + const output = await renderError( + 'The AI service could not complete this request. The same request is likely to fail the same way, so waiting will not help. Re-run with --debug to see the underlying error.', + ); + + expect(output).not.toMatch(/temporarily unavailable/i); + expect(output).toMatch(/could not complete this request/i); + }); + + it('still shows the transient copy for a real 503', async () => { + const output = await renderError('API Error: 503 Service Unavailable'); + + expect(output).toMatch(/temporarily unavailable/i); + expect(output).toMatch(/few minutes/i); + }); + + it('renders the raw message when nothing matches', async () => { + const output = await renderError('Something broke'); + + expect(output).toMatch(/Something broke/); + expect(output).toMatch(/--debug/); + }); + }); + describe('staging success copy', () => { it('device path announces a fresh environment without "retrieved"', async () => { await adapter.start(); diff --git a/src/lib/adapters/cli-adapter.ts b/src/lib/adapters/cli-adapter.ts index 4e975013..d8a52883 100644 --- a/src/lib/adapters/cli-adapter.ts +++ b/src/lib/adapters/cli-adapter.ts @@ -6,7 +6,7 @@ import chalk from 'chalk'; import { getConfig } from '../settings.js'; import { ProgressTracker } from '../progress-tracker.js'; import { renderCompletionSummary, renderBrandMark } from '../../utils/summary-box.js'; -import { formatWorkOSCommand } from '../../utils/command-invocation.js'; +import { classifyAgentFailure, describeAgentFailure } from '../failure-classifier.js'; /** * CLI adapter that renders wizard events via ui. @@ -548,41 +548,16 @@ export class CLIAdapter implements InstallerAdapter { this.stopSpinner('Failed', 1); // Rewrite raw API/SDK errors into user-friendly messages with a next step. - // Matching is word-boundary / code-based so 'author' doesn't read as 'auth' - // and 'Module not found' doesn't read as a missing-directory error. - const isServiceError = - /\b50[0-9]\b/.test(message) || /server_error|internal_error|overloaded|service.*unavailable/i.test(message); - const isRateLimit = /\b429\b/.test(message) || /\brate.?limit/i.test(message); - const isNetworkError = /ECONNREFUSED|ETIMEDOUT|ENOTFOUND|fetch failed/i.test(message); - const isProcessExit = /process exited with code/i.test(message); - const isAuthError = /\b(401|403|unauthorized|forbidden|authentication|authorization)\b/i.test(message); - const isMissingPath = /\bENOENT\b/.test(message); - - if (isServiceError) { - ui.log.error('The AI service is temporarily unavailable.'); - ui.log.info('This is usually resolved within a few minutes. Please try again shortly.'); - } else if (isRateLimit) { - ui.log.error('The AI service is currently rate-limited.'); - ui.log.info('Please wait a minute and try again.'); - } else if (isNetworkError) { - ui.log.error('Could not connect to the AI service.'); - ui.log.info('Check your internet connection and try again.'); - } else if (isProcessExit) { - ui.log.error('The AI agent process exited unexpectedly.'); - ui.log.info('Try running again. If this persists, run with --debug for details.'); - } else if (isAuthError) { - ui.log.error('Authentication failed.'); - ui.log.info(`Try running: ${formatWorkOSCommand('auth logout')} && ${formatWorkOSCommand('install')}`); - } else if (isMissingPath) { - ui.log.error(message); - ui.log.info('Make sure you are running this in your project directory.'); - } else { - // Unknown error: still give the user somewhere to go next. - ui.log.error(message); - ui.log.info( + // The verdict comes from the shared classifier so this path cannot drift + // from the headless adapter or from runAgent's own error message. + const { headline, detail } = describeAgentFailure(classifyAgentFailure(message)); + + ui.log.error(headline ?? message); + ui.log.info( + // No specific advice for this kind: still give the user somewhere to go next. + detail ?? `Re-run with ${chalk.cyan('--debug')} for details, or report it at ${chalk.cyan('https://github.com/workos/cli/issues')}`, - ); - } + ); if (stack && this.debug) { this.debugLog(stack); diff --git a/src/lib/adapters/headless-adapter.ts b/src/lib/adapters/headless-adapter.ts index fe07c3dd..cdc4e547 100644 --- a/src/lib/adapters/headless-adapter.ts +++ b/src/lib/adapters/headless-adapter.ts @@ -2,6 +2,20 @@ import type { InstallerAdapter, AdapterConfig } from './types.js'; import type { InstallerEventEmitter, InstallerEvents } from '../events.js'; import { writeNDJSON } from '../../utils/ndjson.js'; import { ExitCode } from '../../utils/exit-codes.js'; +import { classifyAgentFailure, formatAgentFailure, type AgentFailureKind } from '../failure-classifier.js'; + +/** + * Structured `code` emitted per failure kind. These are part of the NDJSON + * contract, so the existing codes must not be renamed; kinds without a + * dedicated code stay on `installer_error` and stream the raw message. + */ +const ERROR_CODES: Partial> = { + service_outage: 'service_unavailable', + rate_limited: 'rate_limited', + deterministic: 'deterministic_error', + network: 'network_error', + process_exit: 'process_error', +}; /** * Options controlling headless adapter behavior. @@ -412,30 +426,18 @@ export class HeadlessAdapter implements InstallerAdapter { }; private handleError = ({ message, stack }: InstallerEvents['error']): void => { - const isServiceError = - /\b50[0-9]\b/.test(message) || /server_error|internal_error|overloaded|service.*unavailable/i.test(message); - const isRateLimit = /\b429\b/.test(message) || /rate.limit/i.test(message); - const isNetworkError = /ECONNREFUSED|ETIMEDOUT|ENOTFOUND|fetch failed/i.test(message); - const isProcessExit = /process exited with code/i.test(message); - - let code = 'installer_error'; - let displayMessage = message; - - if (isServiceError) { - code = 'service_unavailable'; - displayMessage = 'The AI service is temporarily unavailable. Please try again in a few minutes.'; - } else if (isRateLimit) { - code = 'rate_limited'; - displayMessage = 'The AI service is currently rate-limited. Please wait a minute and try again.'; - } else if (isNetworkError) { - code = 'network_error'; - displayMessage = 'Could not connect to the AI service. Check your internet connection and try again.'; - } else if (isProcessExit) { - code = 'process_error'; - displayMessage = 'The AI agent process exited unexpectedly. Try running again with --debug for details.'; - } + // The verdict comes from the shared classifier so JSON consumers and the + // interactive path cannot disagree about whether a retry is worth trying. + const kind = classifyAgentFailure(message); + const code = ERROR_CODES[kind]; - writeNDJSON({ type: 'error', code, message: displayMessage }); + writeNDJSON({ + type: 'error', + code: code ?? 'installer_error', + // Kinds with no dedicated code keep streaming the raw message — JSON + // consumers depend on it where we have nothing better to say. + message: code ? formatAgentFailure(kind, message) : message, + }); this.debugLog(stack ?? ''); }; } diff --git a/src/lib/agent-interface.spec.ts b/src/lib/agent-interface.spec.ts index bc8fc5ae..2ab3883e 100644 --- a/src/lib/agent-interface.spec.ts +++ b/src/lib/agent-interface.spec.ts @@ -300,12 +300,41 @@ describe('service unavailability handling', () => { }) as typeof emitter.emit; }); - it('detects is_error result with API 500 as SERVICE_UNAVAILABLE', async () => { + // The gateway's generic 500 branch also fires for request-shape failures, so + // this text is NOT evidence of an outage. Telling the user to wait a few + // minutes costs them another four-minute run that fails identically. + it('treats the gateway generic 500 as deterministic, not a transient outage', async () => { const apiErrorText = 'API Error: 500 {"error":{"type":"internal_error","message":"An unexpected error occurred"}}'; mockQuery.mockImplementation(createMockSDKResponse([{ text: apiErrorText, is_error: true }])); const result = await runAgent(makeAgentConfig(), 'Test prompt', makeOptions(), undefined, emitter); + expect(result.error).toBe(AgentErrorType.EXECUTION_ERROR); + expect(result.errorMessage).not.toMatch(/temporarily unavailable/); + expect(result.errorMessage).not.toMatch(/few minutes|try again shortly/i); + expect(result.errorMessage).toMatch(/likely to fail the same way/); + }); + + it("treats the proxy's own upstream_timeout as deterministic", async () => { + mockQuery.mockImplementation( + createMockSDKResponse([ + { text: '{"error":"upstream_timeout","message":"Upstream server timed out"}', is_error: true }, + ]), + ); + + const result = await runAgent(makeAgentConfig(), 'Test prompt', makeOptions(), undefined, emitter); + + expect(result.error).toBe(AgentErrorType.EXECUTION_ERROR); + expect(result.errorMessage).not.toMatch(/temporarily unavailable/); + }); + + it('detects a 503 as SERVICE_UNAVAILABLE', async () => { + mockQuery.mockImplementation( + createMockSDKResponse([{ text: 'API Error: 503 Service Unavailable', is_error: true }]), + ); + + const result = await runAgent(makeAgentConfig(), 'Test prompt', makeOptions(), undefined, emitter); + expect(result.error).toBe(AgentErrorType.SERVICE_UNAVAILABLE); expect(result.errorMessage).toMatch(/temporarily unavailable/); }); @@ -328,8 +357,7 @@ describe('service unavailability handling', () => { }); it('skips validation retries when service is unavailable', async () => { - const apiErrorText = 'API Error: 500 {"error":{"type":"internal_error","message":"An unexpected error occurred"}}'; - mockQuery.mockImplementation(createMockSDKResponse([{ text: apiErrorText, is_error: true }])); + mockQuery.mockImplementation(createMockSDKResponse([{ text: 'API Error: 503 overloaded_error', is_error: true }])); const validateAndFormat = vi.fn().mockResolvedValue('Still broken'); diff --git a/src/lib/agent-interface.ts b/src/lib/agent-interface.ts index 2db6c170..5e8d31ae 100644 --- a/src/lib/agent-interface.ts +++ b/src/lib/agent-interface.ts @@ -19,6 +19,7 @@ import type { InstallerEventEmitter } from './events.js'; import { startCredentialProxy, startClaimTokenProxy, type CredentialProxyHandle } from './credential-proxy.js'; import { getActiveEnvironment, isUnclaimedEnvironment } from './config-store.js'; import { getAuthkitDomain, getCliAuthClientId } from './settings.js'; +import { classifyAgentFailure, formatAgentFailure } from './failure-classifier.js'; import type { SDKMessage, SDKUserMessage, @@ -63,6 +64,9 @@ const SERVICE_UNAVAILABLE_PREFIX = '__SERVICE_UNAVAILABLE__'; /** Internal prefix used to tag rate-limit errors from handleSDKMessage */ const RATE_LIMITED_PREFIX = '__RATE_LIMITED__'; +/** Internal prefix used to tag failures that will recur on every retry */ +const DETERMINISTIC_PREFIX = '__DETERMINISTIC__'; + /** * Error types that can be returned from agent execution. * These correspond to the error signals that the agent emits. @@ -702,7 +706,7 @@ export async function runAgent( logError('AI service unavailable:', detail); return { error: AgentErrorType.SERVICE_UNAVAILABLE, - errorMessage: 'The AI service is temporarily unavailable. Please try again in a few minutes.', + errorMessage: formatAgentFailure('service_outage', detail), }; } if (sdkError.startsWith(RATE_LIMITED_PREFIX)) { @@ -710,7 +714,18 @@ export async function runAgent( logError('AI service rate-limited:', detail); return { error: AgentErrorType.SERVICE_UNAVAILABLE, - errorMessage: 'The AI service is currently rate-limited. Please wait a minute and try again.', + errorMessage: formatAgentFailure('rate_limited', detail), + }; + } + if (sdkError.startsWith(DETERMINISTIC_PREFIX)) { + const detail = sdkError.slice(DETERMINISTIC_PREFIX.length); + // A gateway 500 is not automatically an outage: its generic branch also + // fires for request-shape failures, which recur on every attempt. Say so + // instead of sending the user round another four-minute run. + logError('Agent failure will recur on retry:', detail); + return { + error: AgentErrorType.EXECUTION_ERROR, + errorMessage: formatAgentFailure('deterministic', detail), }; } logError('Agent SDK error:', sdkError); @@ -922,16 +937,19 @@ function handleSDKMessage( const resultText = typeof message.result === 'string' ? message.result : ''; logError('Agent result marked as error:', resultText); - // Detect rate limiting (429) — check before 5xx so it gets distinct messaging - if (/\b429\b/.test(resultText) || /rate.limit/i.test(resultText)) { - return `${RATE_LIMITED_PREFIX}${resultText}`; - } - - // Detect service unavailability (API 500, upstream outage) - if (/\b50[0-9]\b/.test(resultText) || /server_error|internal_error|overloaded/.test(resultText)) { - return `${SERVICE_UNAVAILABLE_PREFIX}${resultText}`; + // One shared classifier owns the transient-vs-deterministic decision + // (failure-classifier.ts). This only maps its verdict onto the prefix + // protocol runAgent reads back below. + switch (classifyAgentFailure(resultText)) { + case 'rate_limited': + return `${RATE_LIMITED_PREFIX}${resultText}`; + case 'service_outage': + return `${SERVICE_UNAVAILABLE_PREFIX}${resultText}`; + case 'deterministic': + return `${DETERMINISTIC_PREFIX}${resultText}`; + default: + return resultText || 'Agent execution failed'; } - return resultText || 'Agent execution failed'; } if (message.subtype === 'success') { diff --git a/src/lib/credential-proxy.spec.ts b/src/lib/credential-proxy.spec.ts new file mode 100644 index 00000000..3c251bc0 --- /dev/null +++ b/src/lib/credential-proxy.spec.ts @@ -0,0 +1,107 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; + +vi.mock('../utils/debug.js', () => ({ + logInfo: vi.fn(), + logWarn: vi.fn(), + logError: vi.fn(), +})); + +vi.mock('../utils/analytics.js', () => ({ + analytics: { capture: vi.fn() }, +})); + +vi.mock('./credentials.js', () => ({ + getCredentials: vi.fn(() => ({ + accessToken: 'access-token', + expiresAt: Date.now() + 3_600_000, + userId: 'user_x', + })), + updateTokens: vi.fn(), +})); + +vi.mock('./token-refresh-client.js', () => ({ + refreshAccessToken: vi.fn(), +})); + +vi.mock('./host-probe.js', () => ({ + observeHostFailure: vi.fn(), +})); + +import { startCredentialProxy, startClaimTokenProxy, type CredentialProxyHandle } from './credential-proxy.js'; + +/** + * The socket timeout is the only knob standing between a long agent turn and a + * spurious 504. The gateway aggregates streams internally, so a non-streaming + * client can sit silent for minutes; 120s used to kill exactly those requests. + * Both factories are covered because the unclaimed-environment install path + * goes through the claim-token proxy, not the credential proxy. + */ +const EXPECTED_TIMEOUT_MS = 600_000; + +/** A throwaway upstream so the proxied request completes instead of hanging. */ +async function startUpstream(): Promise<{ url: string; close: () => Promise }> { + const server = http.createServer((_req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end('{"ok":true}'); + }); + + await new Promise((resolve) => server.listen(0, '127.0.0.1', () => resolve())); + const { port } = server.address() as AddressInfo; + + return { + url: `http://127.0.0.1:${port}`, + close: () => new Promise((resolve) => server.close(() => resolve())), + }; +} + +describe('credential proxy upstream timeout', () => { + let capturedOptions: http.RequestOptions[]; + let upstream: Awaited>; + let proxy: CredentialProxyHandle | null = null; + + beforeEach(async () => { + capturedOptions = []; + upstream = await startUpstream(); + + // Capture what the proxy asks the transport for, then let the real request + // run so the response pipeline (and the test's fetch) actually completes. + const realRequest = http.request; + vi.spyOn(http, 'request').mockImplementation(((...args: Parameters) => { + capturedOptions.push(args[0] as http.RequestOptions); + return realRequest(...args); + }) as typeof http.request); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await proxy?.stop(); + proxy = null; + await upstream.close(); + }); + + it('startCredentialProxy forwards with a 600s socket timeout', async () => { + proxy = await startCredentialProxy({ upstreamUrl: upstream.url }); + + const res = await fetch(`${proxy.url}/v1/messages`, { method: 'POST', body: '{}' }); + + expect(res.status).toBe(200); + expect(capturedOptions).toHaveLength(1); + expect(capturedOptions[0].timeout).toBe(EXPECTED_TIMEOUT_MS); + }); + + it('startClaimTokenProxy forwards with a 600s socket timeout', async () => { + proxy = await startClaimTokenProxy({ + upstreamUrl: upstream.url, + claimToken: 'claim_xyz', + clientId: 'client_x', + }); + + const res = await fetch(`${proxy.url}/v1/messages`, { method: 'POST', body: '{}' }); + + expect(res.status).toBe(200); + expect(capturedOptions).toHaveLength(1); + expect(capturedOptions[0].timeout).toBe(EXPECTED_TIMEOUT_MS); + }); +}); diff --git a/src/lib/credential-proxy.ts b/src/lib/credential-proxy.ts index 606e0b99..f2d98141 100644 --- a/src/lib/credential-proxy.ts +++ b/src/lib/credential-proxy.ts @@ -44,6 +44,16 @@ export interface CredentialProxyHandle { stop: () => Promise; } +/** + * Socket inactivity timeout for gateway requests. Node's `timeout` option is + * already an idle timer, so this is a value change, not a mechanism change. + * 600s matches the Anthropic SDK's own 10-minute non-streaming ceiling. It must + * stay well above a single agent turn: when the gateway aggregates a stream + * internally, a non-streaming client receives no bytes until the turn completes, + * and the old 120s cut those requests off mid-flight. + */ +const UPSTREAM_IDLE_TIMEOUT_MS = 600_000; + // Hop-by-hop headers that must not be forwarded by proxies (RFC 7230 §6.1) const HOP_BY_HOP_HEADERS = new Set([ 'connection', @@ -311,7 +321,7 @@ async function handleRequest( path: finalPath, method: req.method, headers, - timeout: 120_000, // 2 minute timeout + timeout: UPSTREAM_IDLE_TIMEOUT_MS, }; const transport = useHttps ? https : http; @@ -398,7 +408,7 @@ export async function startClaimTokenProxy(options: { path: finalPath, method: req.method, headers, - timeout: 120_000, + timeout: UPSTREAM_IDLE_TIMEOUT_MS, }, (proxyRes) => { res.writeHead(proxyRes.statusCode || 500, filterHeaders(proxyRes.headers)); diff --git a/src/lib/env-writer.spec.ts b/src/lib/env-writer.spec.ts index 3af0878c..547b5bf3 100644 --- a/src/lib/env-writer.spec.ts +++ b/src/lib/env-writer.spec.ts @@ -1,8 +1,8 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { existsSync, readFileSync, unlinkSync, writeFileSync, mkdtempSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { writeEnvLocal } from './env-writer.js'; +import { writeCredentialsEnv, writeEnvLocal } from './env-writer.js'; describe('writeEnvLocal', () => { let testDir: string; @@ -12,12 +12,7 @@ describe('writeEnvLocal', () => { }); afterEach(() => { - for (const file of ['.env.local', '.gitignore']) { - const filePath = join(testDir, file); - if (existsSync(filePath)) { - unlinkSync(filePath); - } - } + rmSync(testDir, { recursive: true, force: true }); }); it('creates .env.local when none exists', () => { @@ -34,6 +29,16 @@ describe('writeEnvLocal', () => { expect(content).toContain('WORKOS_REDIRECT_URI=http://localhost:3000/callback'); }); + it('writes a fresh file with no leading blank line', () => { + writeEnvLocal(testDir, { + WORKOS_CLIENT_ID: 'client_123', + WORKOS_COOKIE_PASSWORD: 'pw', + }); + + const content = readFileSync(join(testDir, '.env.local'), 'utf-8'); + expect(content).toBe('WORKOS_CLIENT_ID=client_123\nWORKOS_COOKIE_PASSWORD=pw\n'); + }); + it('generates cookie password when not provided', () => { writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_123', @@ -55,6 +60,16 @@ describe('writeEnvLocal', () => { expect(content).toContain('WORKOS_COOKIE_PASSWORD=my-existing-password'); }); + it('does not regenerate a cookie password already in the file', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, 'WORKOS_COOKIE_PASSWORD=already_in_file\n'); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_123' }); + + const content = readFileSync(envPath, 'utf-8'); + expect(content).toBe('WORKOS_COOKIE_PASSWORD=already_in_file\nWORKOS_CLIENT_ID=client_123\n'); + }); + it('merges with existing .env.local without overwriting', () => { const envPath = join(testDir, '.env.local'); writeFileSync(envPath, 'EXISTING_VAR=existing_value\nOTHER_VAR=other\n'); @@ -90,50 +105,226 @@ describe('writeEnvLocal', () => { writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_123', - WORKOS_REDIRECT_URI: 'http://localhost:3000/callback', + WORKOS_COOKIE_PASSWORD: 'pw', }); const content = readFileSync(envPath, 'utf-8'); - expect(content).toContain('WORKOS_CLIENT_ID=client_123'); + expect(content).toBe('WORKOS_CLIENT_ID=client_123\nWORKOS_COOKIE_PASSWORD=pw\n'); }); - it('handles comments in existing .env file', () => { - const envPath = join(testDir, '.env.local'); - writeFileSync(envPath, '# This is a comment\nEXISTING=value\n# Another comment\n'); - + it('includes API key when provided', () => { writeEnvLocal(testDir, { + WORKOS_API_KEY: 'sk_test_123', WORKOS_CLIENT_ID: 'client_123', WORKOS_REDIRECT_URI: 'http://localhost:3000/callback', }); - const content = readFileSync(envPath, 'utf-8'); - expect(content).toContain('EXISTING=value'); - expect(content).toContain('WORKOS_CLIENT_ID=client_123'); - // Comments are not preserved in simple parser, which is fine + const content = readFileSync(join(testDir, '.env.local'), 'utf-8'); + expect(content).toContain('WORKOS_API_KEY=sk_test_123'); }); - it('handles values containing equals sign', () => { - const envPath = join(testDir, '.env.local'); - writeFileSync(envPath, 'KEY_WITH_EQUALS=value=with=equals\n'); + describe('line preservation', () => { + it('rewrites values in place, preserving comments, blanks, and key order', () => { + const envPath = join(testDir, '.env.local'); + const original = [ + '# WorkOS credentials', + '', + 'WORKOS_CLIENT_ID=client_old', + 'WORKOS_API_KEY=sk_old', + 'WORKOS_COOKIE_PASSWORD=keep_me', + '', + '# Unrelated app config', + 'DATABASE_URL=postgres://localhost/dev', + '', + ].join('\n'); + writeFileSync(envPath, original); + + writeEnvLocal(testDir, { + WORKOS_CLIENT_ID: 'client_new', + WORKOS_API_KEY: 'sk_new', + }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + [ + '# WorkOS credentials', + '', + 'WORKOS_CLIENT_ID=client_new', + 'WORKOS_API_KEY=sk_new', + 'WORKOS_COOKIE_PASSWORD=keep_me', + '', + '# Unrelated app config', + 'DATABASE_URL=postgres://localhost/dev', + '', + ].join('\n'), + ); + }); - writeEnvLocal(testDir, { - WORKOS_CLIENT_ID: 'client_123', - WORKOS_REDIRECT_URI: 'http://localhost:3000/callback', + it('appends genuinely new keys at the end and touches nothing else', () => { + const envPath = join(testDir, '.env.local'); + const original = '# top\n\nDATABASE_URL=postgres://localhost/dev\n\n# bottom\n'; + writeFileSync(envPath, original); + + writeEnvLocal(testDir, { + WORKOS_CLIENT_ID: 'client_123', + WORKOS_COOKIE_PASSWORD: 'pw', + }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + original + 'WORKOS_CLIENT_ID=client_123\nWORKOS_COOKIE_PASSWORD=pw\n', + ); }); - const content = readFileSync(envPath, 'utf-8'); - expect(content).toContain('KEY_WITH_EQUALS=value=with=equals'); + it('preserves a comments-only file', () => { + const envPath = join(testDir, '.env.local'); + const original = '# This is a comment\n# Another comment\n'; + writeFileSync(envPath, original); + + writeEnvLocal(testDir, { + WORKOS_CLIENT_ID: 'client_123', + WORKOS_COOKIE_PASSWORD: 'pw', + }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + original + 'WORKOS_CLIENT_ID=client_123\nWORKOS_COOKIE_PASSWORD=pw\n', + ); + }); + + it('preserves values containing an equals sign', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, 'KEY_WITH_EQUALS=value=with=equals\nWORKOS_COOKIE_PASSWORD=a=b=c\n'); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_123' }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + 'KEY_WITH_EQUALS=value=with=equals\nWORKOS_COOKIE_PASSWORD=a=b=c\nWORKOS_CLIENT_ID=client_123\n', + ); + }); + + it('writes a value containing an equals sign verbatim', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, 'WORKOS_COOKIE_PASSWORD=old\n'); + + writeEnvLocal(testDir, { + WORKOS_CLIENT_ID: 'client_123', + WORKOS_COOKIE_PASSWORD: 'a=b=c', + }); + + expect(readFileSync(envPath, 'utf-8')).toBe('WORKOS_COOKIE_PASSWORD=a=b=c\nWORKOS_CLIENT_ID=client_123\n'); + }); + + it('adds exactly one trailing newline to a file that lacked one', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, '# top\nWORKOS_COOKIE_PASSWORD=pw'); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_123' }); + + expect(readFileSync(envPath, 'utf-8')).toBe('# top\nWORKOS_COOKIE_PASSWORD=pw\nWORKOS_CLIENT_ID=client_123\n'); + }); }); - it('includes API key when provided', () => { - writeEnvLocal(testDir, { - WORKOS_API_KEY: 'sk_test_123', + describe('backup', () => { + const envVars = { WORKOS_CLIENT_ID: 'client_123', - WORKOS_REDIRECT_URI: 'http://localhost:3000/callback', + WORKOS_COOKIE_PASSWORD: 'pw', + }; + + it('does not create a backup when no env file exists', () => { + writeEnvLocal(testDir, envVars); + + expect(existsSync(join(testDir, '.env.local.bak'))).toBe(false); }); - const content = readFileSync(join(testDir, '.env.local'), 'utf-8'); - expect(content).toContain('WORKOS_API_KEY=sk_test_123'); + it('backs up the pre-CLI file before the first mutation', () => { + const envPath = join(testDir, '.env.local'); + const original = '# mine\nDATABASE_URL=postgres://localhost/dev\n'; + writeFileSync(envPath, original); + + writeEnvLocal(testDir, envVars); + + expect(readFileSync(join(testDir, '.env.local.bak'), 'utf-8')).toBe(original); + }); + + it('keeps the original backup across two writes in the same run', () => { + const envPath = join(testDir, '.env.local'); + const original = '# mine\nDATABASE_URL=postgres://localhost/dev\n'; + writeFileSync(envPath, original); + + writeEnvLocal(testDir, envVars); + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_456', WORKOS_COOKIE_PASSWORD: 'pw' }); + + // The backup is the pre-CLI file, not the state after the first write + expect(readFileSync(join(testDir, '.env.local.bak'), 'utf-8')).toBe(original); + expect(readFileSync(envPath, 'utf-8')).toContain('WORKOS_CLIENT_ID=client_456'); + }); + + it('never overwrites a backup from an earlier run', () => { + const envPath = join(testDir, '.env.local'); + const backupPath = join(testDir, '.env.local.bak'); + writeFileSync(envPath, 'WORKOS_CLIENT_ID=client_from_run_one\n'); + writeFileSync(backupPath, '# pristine from run one\n'); + + writeEnvLocal(testDir, envVars); + + expect(readFileSync(backupPath, 'utf-8')).toBe('# pristine from run one\n'); + }); + + it('git-ignores the backup before writing it, in a project with no .gitignore', () => { + writeFileSync(join(testDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); + + writeEnvLocal(testDir, envVars); + + expect(existsSync(join(testDir, '.env.local.bak'))).toBe(true); + expect(readFileSync(join(testDir, '.gitignore'), 'utf-8')).toBe('.env.local.bak\n.env.local\n'); + }); + + it('git-ignores the backup in a project whose .gitignore only has .env*.local', () => { + // The Next.js default: .env*.local covers .env.local but NOT .env.local.bak, + // and `git add -A` would otherwise commit a live API key and claim token. + const gitignorePath = join(testDir, '.gitignore'); + writeFileSync(gitignorePath, 'node_modules\n.env*.local\n'); + writeFileSync(join(testDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); + + writeEnvLocal(testDir, envVars); + + expect(existsSync(join(testDir, '.env.local.bak'))).toBe(true); + expect(readFileSync(gitignorePath, 'utf-8')).toBe('node_modules\n.env*.local\n.env.local.bak\n'); + }); + + it('leaves .gitignore alone when .env* already covers the backup', () => { + const gitignorePath = join(testDir, '.gitignore'); + writeFileSync(gitignorePath, '.env*\n'); + writeFileSync(join(testDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); + + writeEnvLocal(testDir, envVars); + + expect(existsSync(join(testDir, '.env.local.bak'))).toBe(true); + expect(readFileSync(gitignorePath, 'utf-8')).toBe('.env*\n'); + }); + + it('leaves .gitignore alone when *.bak already covers the backup', () => { + const gitignorePath = join(testDir, '.gitignore'); + writeFileSync(gitignorePath, '*.bak\n.env.local\n'); + writeFileSync(join(testDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); + + writeEnvLocal(testDir, envVars); + + expect(existsSync(join(testDir, '.env.local.bak'))).toBe(true); + expect(readFileSync(gitignorePath, 'utf-8')).toBe('*.bak\n.env.local\n'); + }); + + it('does not write the backup when .gitignore cannot be updated', () => { + // .gitignore as a directory makes the read/write throw. Proves ordering: + // an un-ignorable backup must never reach disk. + mkdirSync(join(testDir, '.gitignore')); + const original = 'WORKOS_API_KEY=sk_live_secret\n'; + writeFileSync(join(testDir, '.env.local'), original); + + expect(() => writeEnvLocal(testDir, envVars)).toThrow(); + + expect(existsSync(join(testDir, '.env.local.bak'))).toBe(false); + // The env write happens after the backup, so the user's file is intact + expect(readFileSync(join(testDir, '.env.local'), 'utf-8')).toBe(original); + }); }); describe('gitignore handling', () => { @@ -214,3 +405,78 @@ describe('writeEnvLocal', () => { }); }); }); + +describe('writeCredentialsEnv', () => { + let testDir: string; + + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), 'env-writer-cred-test-')); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + }); + + const envVars = { + WORKOS_API_KEY: 'sk_test_new', + WORKOS_CLIENT_ID: 'client_new', + }; + + it('delegates to .env.local when package.json is present', () => { + writeFileSync(join(testDir, 'package.json'), '{}\n'); + + writeCredentialsEnv(testDir, envVars); + + expect(existsSync(join(testDir, '.env.local'))).toBe(true); + expect(existsSync(join(testDir, '.env'))).toBe(false); + }); + + it('rewrites .env values in place, preserving comments, blanks, and order', () => { + const envPath = join(testDir, '.env'); + const original = [ + '# WorkOS', + '', + 'WORKOS_CLIENT_ID=client_old', + 'WORKOS_API_KEY=sk_old', + '', + '# Django', + 'DJANGO_SECRET_KEY=abc=def', + '', + ].join('\n'); + writeFileSync(envPath, original); + + writeCredentialsEnv(testDir, envVars); + + expect(readFileSync(envPath, 'utf-8')).toBe( + [ + '# WorkOS', + '', + 'WORKOS_CLIENT_ID=client_new', + 'WORKOS_API_KEY=sk_test_new', + '', + '# Django', + 'DJANGO_SECRET_KEY=abc=def', + '', + ].join('\n'), + ); + }); + + it('does not generate a cookie password outside the JS branch', () => { + writeCredentialsEnv(testDir, envVars); + + const content = readFileSync(join(testDir, '.env'), 'utf-8'); + expect(content).toBe('WORKOS_API_KEY=sk_test_new\nWORKOS_CLIENT_ID=client_new\n'); + }); + + it('backs up .env once and git-ignores the backup', () => { + const envPath = join(testDir, '.env'); + const original = '# mine\nDJANGO_SECRET_KEY=abc\n'; + writeFileSync(envPath, original); + + writeCredentialsEnv(testDir, envVars); + writeCredentialsEnv(testDir, { WORKOS_CLIENT_ID: 'client_third' }); + + expect(readFileSync(join(testDir, '.env.bak'), 'utf-8')).toBe(original); + expect(readFileSync(join(testDir, '.gitignore'), 'utf-8')).toBe('.env.bak\n.env\n'); + }); +}); diff --git a/src/lib/env-writer.ts b/src/lib/env-writer.ts index 9f07ad9d..5784362a 100644 --- a/src/lib/env-writer.ts +++ b/src/lib/env-writer.ts @@ -1,18 +1,17 @@ import { existsSync, readFileSync, writeFileSync } from 'fs'; -import { join } from 'path'; +import { basename, join } from 'path'; import { parseEnvFile } from '../utils/env-parser.js'; const ENV_LOCAL_COVERING_PATTERNS = ['.env.local', '.env*.local', '.env*']; const ENV_COVERING_PATTERNS = ['.env', '.env*']; /** - * Ensure the given env filename is in .gitignore. + * Ensure the given filename is in .gitignore. * Creates .gitignore if it doesn't exist. - * No-ops if a covering pattern is already present. + * No-ops if one of `coveringPatterns` is already present. */ -function ensureGitignore(installDir: string, filename: '.env' | '.env.local'): void { +function ensureGitignore(installDir: string, filename: string, coveringPatterns: string[]): void { const gitignorePath = join(installDir, '.gitignore'); - const coveringPatterns = filename === '.env' ? ENV_COVERING_PATTERNS : ENV_LOCAL_COVERING_PATTERNS; if (!existsSync(gitignorePath)) { writeFileSync(gitignorePath, `${filename}\n`); @@ -50,37 +49,103 @@ function generateCookiePassword(): string { return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); } +/** + * Set `vars` in `content` without disturbing comments, blank lines, or key order. + * Existing keys are rewritten in place; new keys are appended. + * + * An existing key is rewritten as `key=value` with no leading whitespace even if + * the original line was indented — indented env lines are vanishingly rare and + * preserving the indent adds branching for no real benefit. + * + * Key extraction splits on the first `=`, matching `parseEnvFile`, so values + * containing `=` survive. Duplicate keys in a malformed source file: only the + * first occurrence is rewritten (`parseEnvFile` reads last-wins, so a duplicate + * still shadows the update — a pre-existing pathology, not handled here). + */ +function upsertEnvLines(content: string, vars: Record): string { + const hadTrailingNewline = content.endsWith('\n'); + const body = hadTrailingNewline ? content.slice(0, -1) : content; + const pending = new Map(Object.entries(vars)); + + // An empty body has no lines at all — splitting it would invent a blank one. + const lines = + body === '' + ? [] + : body.split(/\r?\n/).map((line) => { + const trimmed = line.trim(); + if (!trimmed || trimmed.startsWith('#')) return line; + const eq = trimmed.indexOf('='); + if (eq === -1) return line; + const key = trimmed.slice(0, eq); + if (!pending.has(key)) return line; + const value = pending.get(key)!; + pending.delete(key); + return `${key}=${value}`; + }); + + for (const [key, value] of pending) { + lines.push(`${key}=${value}`); + } + + // Always exactly one trailing newline, matching the previous writer. + return lines.join('\n') + '\n'; +} + +/** + * Copy `envPath` to `{envPath}.bak` before the first CLI mutation. + * + * Stateless: existence of the backup IS the "already backed up" flag, so a + * second write in the same run — or a later run — never overwrites the original + * pre-CLI file. Deliberately not a module-level flag: this module is otherwise + * pure, and the writers have four call sites across the codebase. + * + * `ensureGitignore` runs BEFORE the write so a crash between the two cannot + * leave an unignored secret on disk: `stageAndCommit` runs `git add -A`, and the + * env file holds a live API key and claim token. + */ +function backupEnvFile(installDir: string, envPath: string): void { + if (!existsSync(envPath)) return; // nothing to back up + const backupPath = `${envPath}.bak`; + if (existsSync(backupPath)) return; // never overwrite an earlier backup + + const backupName = basename(backupPath); + ensureGitignore(installDir, backupName, [backupName, '.env*', '*.bak']); + writeFileSync(backupPath, readFileSync(envPath, 'utf-8')); +} + +/** Drop keys whose value is undefined so they never serialize as `KEY=undefined`. */ +function definedVars(envVars: Partial): Record { + const result: Record = {}; + for (const [key, value] of Object.entries(envVars)) { + if (value !== undefined) result[key] = value; + } + return result; +} + /** * Write environment variables to .env.local before agent runs. - * Merges with existing .env.local if present (new vars take precedence). - * Auto-generates WORKOS_COOKIE_PASSWORD if not provided. + * Upserts into an existing .env.local if present (new vars take precedence), + * preserving comments, blank lines, and key order. + * Auto-generates WORKOS_COOKIE_PASSWORD if not already set. */ export function writeEnvLocal(installDir: string, envVars: Partial): void { const envPath = join(installDir, '.env.local'); - // Read existing env if present - let existingEnv: Record = {}; - if (existsSync(envPath)) { - const content = readFileSync(envPath, 'utf-8'); - existingEnv = parseEnvFile(content); - } + backupEnvFile(installDir, envPath); - // Merge with new vars (new vars take precedence) - const merged = { ...existingEnv, ...envVars }; + const existingContent = existsSync(envPath) ? readFileSync(envPath, 'utf-8') : ''; + const existingEnv = parseEnvFile(existingContent); - // Generate cookie password if not provided - if (!merged.WORKOS_COOKIE_PASSWORD) { - merged.WORKOS_COOKIE_PASSWORD = generateCookiePassword(); - } + const vars = definedVars(envVars); - // Write back - const content = Object.entries(merged) - .map(([key, value]) => `${key}=${value}`) - .join('\n'); + // Generate cookie password only when neither the caller nor the file has one + if (!vars.WORKOS_COOKIE_PASSWORD && !existingEnv.WORKOS_COOKIE_PASSWORD) { + vars.WORKOS_COOKIE_PASSWORD = generateCookiePassword(); + } - ensureGitignore(installDir, '.env.local'); + ensureGitignore(installDir, '.env.local', ENV_LOCAL_COVERING_PATTERNS); - writeFileSync(envPath, content + '\n'); + writeFileSync(envPath, upsertEnvLines(existingContent, vars)); } /** @@ -91,6 +156,11 @@ export function writeEnvLocal(installDir: string, envVars: Partial): vo * * Used by pre-detection flows that write credentials before the framework * integration is known (unclaimed env provisioning). + * + * KEEP IN SYNC: `resolveProjectEnvPath` in ./project-env.ts mirrors the + * package.json test below so the no-clobber check reads the same file this + * writes. If they disagree, the installer can check one file and overwrite + * another — the original bug. */ export function writeCredentialsEnv(installDir: string, envVars: Partial): void { const hasPackageJson = existsSync(join(installDir, 'package.json')); @@ -100,18 +170,12 @@ export function writeCredentialsEnv(installDir: string, envVars: Partial = {}; - if (existsSync(envPath)) { - const content = readFileSync(envPath, 'utf-8'); - existingEnv = parseEnvFile(content); - } - const merged = { ...existingEnv, ...envVars }; - const content = Object.entries(merged) - .map(([key, value]) => `${key}=${value}`) - .join('\n'); + backupEnvFile(installDir, envPath); + + const existingContent = existsSync(envPath) ? readFileSync(envPath, 'utf-8') : ''; - ensureGitignore(installDir, '.env'); + ensureGitignore(installDir, '.env', ENV_COVERING_PATTERNS); - writeFileSync(envPath, content + '\n'); + writeFileSync(envPath, upsertEnvLines(existingContent, definedVars(envVars))); } diff --git a/src/lib/failure-classifier.spec.ts b/src/lib/failure-classifier.spec.ts new file mode 100644 index 00000000..b70738de --- /dev/null +++ b/src/lib/failure-classifier.spec.ts @@ -0,0 +1,102 @@ +import { describe, it, expect } from 'vitest'; +import { + classifyAgentFailure, + describeAgentFailure, + formatAgentFailure, + type AgentFailureKind, +} from './failure-classifier.js'; + +/** + * The verbatim strings from the incident, plus the neighbouring shapes that + * must NOT reclassify. The ordering inside classifyAgentFailure is the entire + * fix: every `deterministic` row below also matches a transient pattern, so a + * careless reorder turns them all back into "try again in a few minutes". + */ +const CASES: Array<[message: string, expected: AgentFailureKind]> = [ + // --- The incident. Both of these carry 5xx-shaped text. --- + ['API Error: 500 {"error":{"type":"internal_error","message":"An unexpected error occurred"}}', 'deterministic'], + ['API Error: 500 An unexpected error occurred (request id: req_x)', 'deterministic'], + ['{"error":"upstream_timeout","message":"Upstream server timed out"}', 'deterministic'], + ['504 Upstream server timed out', 'deterministic'], + ['Streaming is strongly recommended for operations that may take longer than 10 minutes', 'deterministic'], + ['400 max_tokens: 64000 > 32000, which is the maximum allowed', 'deterministic'], + + // --- Genuinely transient. This copy is correct and must not regress. --- + ['503 Service Unavailable', 'service_outage'], + ['{"type":"overloaded_error","message":"Overloaded"}', 'service_outage'], + ['server_error: service overloaded', 'service_outage'], + ['API Error: 502 Bad Gateway', 'service_outage'], + + // --- Rate limiting, checked before any 5xx branch. --- + ['API Error: 429 rate_limit_exceeded', 'rate_limited'], + ['429 Too Many Requests', 'rate_limited'], + ['This organization has exceeded its rate limit', 'rate_limited'], + + // --- Everything else keeps its previous verdict. --- + ['fetch failed', 'network'], + ['connect ECONNREFUSED 127.0.0.1:8000', 'network'], + ['process exited with code 1', 'process_exit'], + ['API Error: 401 unauthorized', 'auth'], + ["ENOENT: no such file or directory, open '/proj/package.json'", 'missing_path'], + ['Some other failure', 'unknown'], + ['', 'unknown'], +]; + +describe('classifyAgentFailure', () => { + for (const [message, expected] of CASES) { + it(`classifies ${JSON.stringify(message.slice(0, 60))} as ${expected}`, () => { + expect(classifyAgentFailure(message)).toBe(expected); + }); + } + + it('prefers rate_limited when a message carries both 429 and 500', () => { + expect(classifyAgentFailure('API Error: 429 after retrying a 500 An unexpected error occurred')).toBe( + 'rate_limited', + ); + }); + + it('prefers deterministic when a message carries both the generic-500 text and internal_error', () => { + // This is the exact shape that used to fall into service_outage: the + // deterministic phrase and the transient token arrive in the same body. + expect(classifyAgentFailure('500 internal_error: An unexpected error occurred')).toBe('deterministic'); + }); + + it('does not read "author" as an auth failure or "Module not found" as a missing path', () => { + expect(classifyAgentFailure('author field is required in package.json')).toBe('unknown'); + expect(classifyAgentFailure('Module not found: Cannot resolve "@workos-inc/authkit-nextjs"')).toBe('unknown'); + }); +}); + +describe('describeAgentFailure', () => { + it('never advises waiting for a deterministic failure', () => { + const { headline, detail } = describeAgentFailure('deterministic'); + + expect(headline).not.toMatch(/temporarily unavailable/i); + expect(`${headline} ${detail}`).not.toMatch(/few minutes|try again shortly|wait a minute/i); + expect(detail).toMatch(/--debug/); + }); + + it('keeps the transient copy for a real outage', () => { + expect(describeAgentFailure('service_outage').headline).toMatch(/temporarily unavailable/i); + expect(describeAgentFailure('service_outage').detail).toMatch(/few minutes/i); + }); + + it('falls back to the raw message for kinds with no better copy', () => { + expect(describeAgentFailure('unknown').headline).toBeNull(); + expect(describeAgentFailure('missing_path').headline).toBeNull(); + expect(formatAgentFailure('unknown', 'Something broke')).toBe('Something broke'); + }); + + /** + * installer-core's emitError re-emits the *rendered* message onto the `error` + * event, so the adapters classify our own copy rather than the raw SDK text. + * If rendering does not round-trip, the deterministic verdict is lost between + * runAgent and the terminal — which is the bug this phase exists to fix. + */ + it.each(['deterministic', 'service_outage', 'rate_limited'] as const)( + 'round-trips its own rendered copy for %s', + (kind) => { + expect(classifyAgentFailure(formatAgentFailure(kind, 'raw'))).toBe(kind); + }, + ); +}); diff --git a/src/lib/failure-classifier.ts b/src/lib/failure-classifier.ts new file mode 100644 index 00000000..b2be5270 --- /dev/null +++ b/src/lib/failure-classifier.ts @@ -0,0 +1,144 @@ +/** + * Single source of truth for what an agent failure means and what to tell the + * user about it. + * + * Three call sites used to re-derive this independently — `handleSDKMessage` in + * `agent-interface.ts`, `CLIAdapter.handleError`, `HeadlessAdapter.handleError` — + * each with its own copy of a "5xx means transient" regex. All three were wrong + * about the same thing: the WorkOS LLM gateway returns a generic 500 for + * failures that are entirely deterministic (its `toApiErrorResponse` fallback + * fires whenever the Anthropic SDK throws a status-less error, including the + * `max_tokens` guard), so "try again in a few minutes" sent users round a + * four-minute loop that could never succeed. + */ + +import { formatWorkOSCommand } from '../utils/command-invocation.js'; + +export type AgentFailureKind = + /** Retry helps, after a wait. */ + | 'rate_limited' + /** Retry helps; a transient upstream problem. */ + | 'service_outage' + /** Retry does NOT help; same input, same failure. */ + | 'deterministic' + | 'network' + | 'process_exit' + | 'auth' + | 'missing_path' + | 'unknown'; + +/** + * Headline for the `deterministic` kind, kept as a named constant because + * `classifyAgentFailure` also matches it: `installer-core`'s `emitError` + * re-emits the *already rendered* message onto the `error` event, so the + * adapters classify our own copy. Rendering has to round-trip or the + * deterministic verdict is silently lost on the way to the terminal. + */ +const DETERMINISTIC_HEADLINE = 'The AI service could not complete this request.'; + +/** + * Classify an agent failure message. The key distinction is whether retrying + * can plausibly succeed. A gateway 500 is NOT automatically transient: the + * WorkOS LLM gateway returns a generic 500 for client-side SDK validation + * failures (`llm-gateway.controller.ts` `toApiErrorResponse`), which recur on + * every attempt. + * + * Ordering is the whole design. The deterministic patterns are tested *before* + * the generic 5xx branch; flip them and the gateway's generic-500 text falls + * back into `service_outage` exactly as it did before this function existed. + * + * Matching is word-boundary / code-based so `author` does not read as `auth` + * and `Module not found` does not read as a missing directory. + */ +export function classifyAgentFailure(message: string): AgentFailureKind { + // Rate limiting first — it is a 429, and it is the one case with specific advice. + if (/\b429\b/.test(message) || /\brate.?limit/i.test(message)) return 'rate_limited'; + + // Deterministic failures that WEAR a 5xx. Must stay above the generic 5xx branch. + // + // "An unexpected error occurred" is the gateway's generic-branch message + // (llm-gateway.controller.ts, toApiErrorResponse) and is the precise + // signature of "the SDK threw something with no status". + if (/an unexpected error occurred/i.test(message)) return 'deterministic'; + // The proxy's own 504 body (credential-proxy.ts). A request that blew the + // socket timeout will blow it again; that is not an outage. + if (/upstream_timeout|upstream server timed out/i.test(message)) return 'deterministic'; + // Request-shape rejections from the Anthropic SDK / gateway guard rails. + if (/streaming is (strongly recommended|required)|max_tokens/i.test(message)) return 'deterministic'; + // Our own rendered copy, so a re-classification downstream round-trips. + if (message.includes(DETERMINISTIC_HEADLINE)) return 'deterministic'; + + // Genuinely transient upstream states. + if (/overloaded/i.test(message) || /\b503\b/.test(message)) return 'service_outage'; + if (/server_error|internal_error|service.*unavailable/i.test(message)) return 'service_outage'; + if (/\b50[0-9]\b/.test(message)) return 'service_outage'; + + if (/ECONNREFUSED|ETIMEDOUT|ENOTFOUND|fetch failed/i.test(message)) return 'network'; + if (/process exited with code/i.test(message)) return 'process_exit'; + if (/\b(401|403|unauthorized|forbidden|authentication|authorization)\b/i.test(message)) return 'auth'; + if (/\bENOENT\b/.test(message)) return 'missing_path'; + return 'unknown'; +} + +/** + * User-facing copy per kind. Adapters render these; they do not re-derive them. + * + * A `null` headline means "there is nothing better to say than the raw message" — + * the caller renders the message it was given. A `null` detail means the caller + * supplies its own next step (the CLI adapter's `--debug` hint). + */ +export function describeAgentFailure(kind: AgentFailureKind): { headline: string | null; detail: string | null } { + switch (kind) { + case 'rate_limited': + return { + headline: 'The AI service is currently rate-limited.', + detail: 'Please wait a minute and try again.', + }; + case 'service_outage': + return { + headline: 'The AI service is temporarily unavailable.', + detail: 'This is usually resolved within a few minutes. Please try again shortly.', + }; + case 'deterministic': + // Neither "wait a few minutes" (it will fail identically) nor a flat "do + // not retry" (a real outage can wear this shape) — say what is true. + return { + headline: DETERMINISTIC_HEADLINE, + detail: + 'The same request is likely to fail the same way, so waiting will not help. Re-run with --debug to see the underlying error.', + }; + case 'network': + return { + headline: 'Could not connect to the AI service.', + detail: 'Check your internet connection and try again.', + }; + case 'process_exit': + return { + headline: 'The AI agent process exited unexpectedly.', + detail: 'Try running again. If this persists, run with --debug for details.', + }; + case 'auth': + return { + headline: 'Authentication failed.', + detail: `Try running: ${formatWorkOSCommand('auth logout')} && ${formatWorkOSCommand('install')}`, + }; + case 'missing_path': + return { + headline: null, + detail: 'Make sure you are running this in your project directory.', + }; + case 'unknown': + return { headline: null, detail: null }; + } +} + +/** + * Flatten a classified failure into one string, for the paths that carry a + * single error message (`runAgent`'s `errorMessage`, the headless NDJSON + * `message`) rather than a headline plus a hint. + */ +export function formatAgentFailure(kind: AgentFailureKind, message: string): string { + const { headline, detail } = describeAgentFailure(kind); + if (!headline) return message; + return detail ? `${headline} ${detail}` : headline; +} diff --git a/src/lib/preflight-authkit.spec.ts b/src/lib/preflight-authkit.spec.ts new file mode 100644 index 00000000..c613f6af --- /dev/null +++ b/src/lib/preflight-authkit.spec.ts @@ -0,0 +1,192 @@ +import { mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { CliExit } from '../utils/cli-exit.js'; +import { resetInteractionModeForTests, setInteractionMode } from '../utils/interaction-mode.js'; + +// Mock the UI facade — the interactive branch prompts, which has no place in a unit test. +const mockConfirm = vi.fn(); +const mockUi = { + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), step: vi.fn(), success: vi.fn() }, + rows: vi.fn(), + confirm: (...args: unknown[]) => mockConfirm(...args), + // Mirrors the real facade: only the CANCEL symbol counts as a cancellation. + isCancel: (value: unknown) => typeof value === 'symbol', +}; +vi.mock('../utils/ui.js', () => ({ default: mockUi })); + +const { assertNoExistingAuthKit, detectExistingAuthKit } = await import('./preflight-authkit.js'); + +function writePackageJson(dir: string, deps: Record, devDeps?: Record): void { + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ name: 'fixture', dependencies: deps, devDependencies: devDeps ?? {} }), + ); +} + +describe('preflight-authkit', () => { + let testDir: string; + let errorSpy: ReturnType; + + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), 'preflight-authkit-test-')); + vi.clearAllMocks(); + resetInteractionModeForTests(); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + resetInteractionModeForTests(); + errorSpy.mockRestore(); + }); + + describe('detectExistingAuthKit', () => { + it('returns [] when the project has no package.json', () => { + expect(detectExistingAuthKit(testDir)).toEqual([]); + }); + + it('returns [] when package.json is malformed (parse failure must not block a run)', () => { + writeFileSync(join(testDir, 'package.json'), '{ not json'); + + expect(detectExistingAuthKit(testDir)).toEqual([]); + }); + + it('returns [] for a framework project with no WorkOS packages', () => { + writePackageJson(testDir, { next: '15.0.0', react: '19.0.0' }); + + expect(detectExistingAuthKit(testDir)).toEqual([]); + }); + + it('detects an installed AuthKit SDK with its version range', () => { + writePackageJson(testDir, { next: '15.0.0', '@workos-inc/authkit-nextjs': '^2.6.0' }); + + expect(detectExistingAuthKit(testDir)).toEqual([{ name: '@workos-inc/authkit-nextjs', version: '^2.6.0' }]); + }); + + it('detects AuthKit declared as a devDependency', () => { + writePackageJson(testDir, { next: '15.0.0' }, { '@workos-inc/authkit-js': '1.0.0' }); + + expect(detectExistingAuthKit(testDir)).toEqual([{ name: '@workos-inc/authkit-js', version: '1.0.0' }]); + }); + + // False-positive guard: the base SDK is in plenty of projects that have + // never installed AuthKit, and those must stay installable. + it('does NOT trip on @workos-inc/node alone', () => { + writePackageJson(testDir, { '@workos-inc/node': '^7.0.0' }); + + expect(detectExistingAuthKit(testDir)).toEqual([]); + }); + + it('does NOT trip on the legacy workos package alone', () => { + writePackageJson(testDir, { workos: '^4.0.0' }); + + expect(detectExistingAuthKit(testDir)).toEqual([]); + }); + + it('ignores @workos-inc/node when it sits alongside AuthKit', () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0', '@workos-inc/node': '^7.0.0' }); + + expect(detectExistingAuthKit(testDir)).toEqual([{ name: '@workos-inc/authkit-nextjs', version: '^2.6.0' }]); + }); + + it('lists every AuthKit SDK found, not just the first', () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0', '@workos-inc/authkit-react': '^1.2.0' }); + + expect(detectExistingAuthKit(testDir)).toEqual([ + { name: '@workos-inc/authkit-nextjs', version: '^2.6.0' }, + { name: '@workos-inc/authkit-react', version: '^1.2.0' }, + ]); + }); + }); + + describe('assertNoExistingAuthKit', () => { + it('is a no-op when no AuthKit is present', async () => { + writePackageJson(testDir, { next: '15.0.0' }); + + await expect(assertNoExistingAuthKit({ installDir: testDir })).resolves.toBeUndefined(); + expect(mockConfirm).not.toHaveBeenCalled(); + }); + + it('throws CliExit (exit 1) naming the packages and doctor in non-interactive mode', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + setInteractionMode({ mode: 'agent', source: 'env' }); + + const error = await assertNoExistingAuthKit({ installDir: testDir }).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(CliExit); + const exit = error as InstanceType; + expect(exit.exitCode).toBe(1); + expect(exit.context?.errorCode).toBe('authkit_already_installed'); + expect(exit.context?.reason).toBe('validation_error'); + + const printed = errorSpy.mock.calls.map((call) => String(call[0])).join('\n'); + expect(printed).toContain('@workos-inc/authkit-nextjs ^2.6.0'); + expect(printed).toContain('doctor'); + expect(printed).toContain('--force'); + // Never prompt in a non-interactive session. + expect(mockConfirm).not.toHaveBeenCalled(); + }); + + it('also refuses in CI mode', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-remix': '^1.0.0' }); + setInteractionMode({ mode: 'ci', source: 'ci_env' }); + + await expect(assertNoExistingAuthKit({ installDir: testDir })).rejects.toBeInstanceOf(CliExit); + }); + + it('returns without throwing when force is set in non-interactive mode', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + setInteractionMode({ mode: 'agent', source: 'env' }); + + await expect(assertNoExistingAuthKit({ installDir: testDir, force: true })).resolves.toBeUndefined(); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('does not even read package.json when force is set', async () => { + // force short-circuits before detection, so a malformed manifest is irrelevant. + writeFileSync(join(testDir, 'package.json'), '{ not json'); + + await expect(assertNoExistingAuthKit({ installDir: testDir, force: true })).resolves.toBeUndefined(); + }); + + it('returns normally when an interactive user accepts', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + mockConfirm.mockResolvedValueOnce(true); + + await expect(assertNoExistingAuthKit({ installDir: testDir })).resolves.toBeUndefined(); + expect(mockConfirm).toHaveBeenCalledTimes(1); + expect(mockUi.log.warn).toHaveBeenCalledWith(expect.stringContaining('already installed')); + expect(mockUi.rows).toHaveBeenCalledWith([ + { key: '@workos-inc/authkit-nextjs', value: '^2.6.0', statusKind: 'muted' }, + ]); + // Names the exact file at risk — .env.local here, since package.json exists. + expect(mockUi.log.info).toHaveBeenCalledWith(expect.stringContaining(join(testDir, '.env.local'))); + }); + + it('throws CliExit (exit 2) and writes nothing when an interactive user declines', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + mockConfirm.mockResolvedValueOnce(false); + + const error = await assertNoExistingAuthKit({ installDir: testDir }).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(CliExit); + const exit = error as InstanceType; + expect(exit.exitCode).toBe(2); + expect(exit.context?.reason).toBe('cancelled'); + // No env file, no .gitignore — the guard runs before anything is written. + expect(readdirSync(testDir)).toEqual(['package.json']); + }); + + it('throws CliExit (exit 2) when the prompt is cancelled', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + mockConfirm.mockResolvedValueOnce(Symbol('workos.prompt.cancel')); + + const error = await assertNoExistingAuthKit({ installDir: testDir }).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(CliExit); + expect((error as InstanceType).exitCode).toBe(2); + }); + }); +}); diff --git a/src/lib/preflight-authkit.ts b/src/lib/preflight-authkit.ts new file mode 100644 index 00000000..4fbcb553 --- /dev/null +++ b/src/lib/preflight-authkit.ts @@ -0,0 +1,79 @@ +/** + * Install preflight: refuse to run over an existing AuthKit install. + * + * `workos install` provisions a fresh WorkOS environment and writes its + * credentials into the project's env file before the installer state machine + * exists. In a project that already has AuthKit wired up that is data loss, so + * this guard runs first — prompting on an interactive TTY and hard-failing in + * agent/CI/JSON mode, with `--force` as the escape hatch. + */ + +import { AUTHKIT_PACKAGES } from '../doctor/checks/sdk.js'; +import { formatWorkOSCommand } from '../utils/command-invocation.js'; +import { ExitCode, exitWithCode } from '../utils/exit-codes.js'; +import { isPromptAllowed } from '../utils/interaction-mode.js'; +import { exitWithError } from '../utils/output.js'; +import { getPackageVersion, readPackageJson } from '../utils/package-json.js'; +import ui from '../utils/ui.js'; + +export interface DetectedAuthKitPackage { + name: string; + version: string; +} + +/** + * Every AuthKit SDK present in the project, not just the first — the warning + * should name everything it found. + * + * An unreadable or malformed package.json reads as "no AuthKit": a parse + * failure must not block a run. + */ +export function detectExistingAuthKit(installDir: string): DetectedAuthKitPackage[] { + const packageJson = readPackageJson(installDir); + if (!packageJson) return []; + + return [...AUTHKIT_PACKAGES] + .map((name) => ({ name, version: getPackageVersion(name, packageJson) })) + .filter((pkg): pkg is DetectedAuthKitPackage => !!pkg.version); +} + +/** + * Stop the install before anything is written when AuthKit is already present. + * + * Must be called BEFORE credential resolution — provisioning writes the env + * file, so a guard that runs afterwards is no guard at all. + */ +export async function assertNoExistingAuthKit(opts: { installDir: string; force?: boolean }): Promise { + if (opts.force) return; + + const found = detectExistingAuthKit(opts.installDir); + if (found.length === 0) return; + + const summary = found.map((pkg) => `${pkg.name} ${pkg.version}`).join(', '); + + // Non-interactive: never prompt, never write. + if (!isPromptAllowed()) { + exitWithError({ + code: 'authkit_already_installed', + message: + `AuthKit is already installed in ${opts.installDir} (${summary}). ` + + `Continuing would provision a new WorkOS environment and rewrite your env file. ` + + `Run \`${formatWorkOSCommand('doctor')}\` to inspect the existing install, or pass --force to override.`, + }); + } + + // Interactive: show what was found, then ask. Name the env file explicitly — + // it is exactly what is at risk. + const { resolveProjectEnvPath } = await import('./project-env.js'); + ui.log.warn('AuthKit is already installed here'); + ui.rows(found.map((pkg) => ({ key: pkg.name, value: pkg.version, statusKind: 'muted' as const }))); + ui.log.info( + `Continuing will provision a new WorkOS environment and rewrite ${resolveProjectEnvPath(opts.installDir)}.`, + ); + ui.log.info(`To inspect the existing install instead: ${formatWorkOSCommand('doctor')}`); + + const proceed = await ui.confirm({ message: 'Continue anyway?' }); + if (ui.isCancel(proceed) || !proceed) { + exitWithCode(ExitCode.CANCELLED, { code: 'cancelled', message: 'Install cancelled.' }); + } +} diff --git a/src/lib/project-env.ts b/src/lib/project-env.ts new file mode 100644 index 00000000..2d0351e3 --- /dev/null +++ b/src/lib/project-env.ts @@ -0,0 +1,46 @@ +/** + * Reader for the project's WorkOS env file. + * + * One reader that resolves the same file `writeCredentialsEnv` would write, so + * the "is this project already configured?" check and the write can never + * disagree. Kept separate from `run-with-core.ts` so the credential path does + * not pull xstate and the whole installer graph in behind it. + */ + +import { existsSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { parseEnvFile } from '../utils/env-parser.js'; + +/** + * The env file the CLI would write for this project. + * + * Mirrors `writeCredentialsEnv` (`lib/env-writer.ts`): `.env.local` for JS + * projects (package.json present), `.env` for everything else. If that rule + * changes, both sides must change together. + */ +export function resolveProjectEnvPath(installDir: string): string { + return existsSync(join(installDir, 'package.json')) ? join(installDir, '.env.local') : join(installDir, '.env'); +} + +/** + * WorkOS credentials already present in the project's env file. + * + * Read failures resolve to `{}` — a malformed env file should not crash the + * installer, it should just look unconfigured. + */ +export function readProjectEnvCredentials(installDir: string): { apiKey?: string; clientId?: string } { + const envPath = resolveProjectEnvPath(installDir); + if (!existsSync(envPath)) { + return {}; + } + + try { + const envVars = parseEnvFile(readFileSync(envPath, 'utf-8')); + return { + apiKey: envVars.WORKOS_API_KEY || undefined, + clientId: envVars.WORKOS_CLIENT_ID || undefined, + }; + } catch { + return {}; + } +} diff --git a/src/lib/resolve-install-credentials.spec.ts b/src/lib/resolve-install-credentials.spec.ts index eb645483..9098cb67 100644 --- a/src/lib/resolve-install-credentials.spec.ts +++ b/src/lib/resolve-install-credentials.spec.ts @@ -1,4 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; // Mock config-store const mockGetActiveEnvironment = vi.fn(); @@ -25,13 +28,22 @@ const { resolveInstallCredentials } = await import('./resolve-install-credential describe('resolveInstallCredentials', () => { const mockAuthenticate = vi.fn(); const originalEnv = process.env.WORKOS_API_KEY; + // The default install dir is process.cwd(), and credential resolution now + // reads the project's env file. Point cwd at an empty dir so these specs + // don't depend on whatever .env.local happens to sit in the repo root. + let emptyCwd: string; + let cwdSpy: ReturnType; beforeEach(() => { vi.clearAllMocks(); delete process.env.WORKOS_API_KEY; + emptyCwd = mkdtempSync(join(tmpdir(), 'resolve-install-credentials-cwd-')); + cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(emptyCwd); }); afterEach(() => { + cwdSpy.mockRestore(); + rmSync(emptyCwd, { recursive: true, force: true }); if (originalEnv !== undefined) { process.env.WORKOS_API_KEY = originalEnv; } else { @@ -146,4 +158,78 @@ describe('resolveInstallCredentials', () => { expect(mockTryProvisionUnclaimedEnv).toHaveBeenCalledWith({ installDir: process.cwd() }); }); + + // Provisioning writes credentials into the project's env file. If a key is + // already there, provisioning would clobber it — the reported data-loss bug. + describe('project env file already has credentials', () => { + let projectDir: string; + + beforeEach(() => { + projectDir = mkdtempSync(join(tmpdir(), 'resolve-install-credentials-test-')); + mockGetActiveEnvironment.mockReturnValue(null); + mockTryProvisionUnclaimedEnv.mockResolvedValue(true); + }); + + afterEach(() => { + rmSync(projectDir, { recursive: true, force: true }); + }); + + it('skips provisioning and falls back to login when .env.local has WORKOS_API_KEY (JS project)', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env.local'), 'WORKOS_API_KEY=sk_a\nWORKOS_CLIENT_ID=client_a\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).not.toHaveBeenCalled(); + expect(mockAuthenticate).toHaveBeenCalled(); + }); + + it('skips provisioning when .env has WORKOS_API_KEY and there is no package.json (non-JS project)', async () => { + writeFileSync(join(projectDir, '.env'), 'WORKOS_API_KEY=sk_a\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).not.toHaveBeenCalled(); + expect(mockAuthenticate).toHaveBeenCalled(); + }); + + it('does not authenticate when skipAuth is set and the project env already has a key', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env.local'), 'WORKOS_API_KEY=sk_a\n'); + + await resolveInstallCredentials(undefined, projectDir, true, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).not.toHaveBeenCalled(); + expect(mockAuthenticate).not.toHaveBeenCalled(); + }); + + it('provisions when .env.local exists but carries no WorkOS keys', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).toHaveBeenCalledWith({ installDir: projectDir }); + expect(mockAuthenticate).not.toHaveBeenCalled(); + }); + + it('provisions when the project has no env file at all', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).toHaveBeenCalledWith({ installDir: projectDir }); + }); + + // A JS project's .env is NOT the file the CLI would write, so it must not + // suppress provisioning — the check has to mirror writeCredentialsEnv exactly. + it('ignores a stale .env in a JS project (writeCredentialsEnv targets .env.local there)', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env'), 'WORKOS_API_KEY=sk_stale\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).toHaveBeenCalledWith({ installDir: projectDir }); + }); + }); }); diff --git a/src/lib/resolve-install-credentials.ts b/src/lib/resolve-install-credentials.ts index 744ac975..1ba4f313 100644 --- a/src/lib/resolve-install-credentials.ts +++ b/src/lib/resolve-install-credentials.ts @@ -39,9 +39,22 @@ export async function resolveInstallCredentials( return; } + const dir = installDir ?? process.cwd(); + + // The CLI has no credentials, but the project might: provisioning writes to + // the project's env file, so a key already sitting there means we must not + // provision. Fall back to login instead — the project has a key, we just + // have no gateway auth. + const { readProjectEnvCredentials } = await import('./project-env.js'); + if (readProjectEnvCredentials(dir).apiKey) { + const { logInfo } = await import('../utils/debug.js'); + logInfo('[resolve-install-credentials] Project env already has WORKOS_API_KEY — skipping provisioning'); + if (!skipAuth) await authenticate(); + return; + } + // No existing credentials — try unclaimed env provisioning const { tryProvisionUnclaimedEnv } = await import('./unclaimed-env-provision.js'); - const dir = installDir ?? process.cwd(); const provisioned = await tryProvisionUnclaimedEnv({ installDir: dir }); if (!provisioned) { // Unclaimed env provisioning failed — fall back to login diff --git a/src/lib/run-with-core.ts b/src/lib/run-with-core.ts index 1d8e2f3c..64497344 100644 --- a/src/lib/run-with-core.ts +++ b/src/lib/run-with-core.ts @@ -1,7 +1,5 @@ import { createActor, fromPromise } from 'xstate'; import open from 'open'; -import { existsSync, readFileSync } from 'fs'; -import { join } from 'path'; import { installerMachine } from './installer-core.js'; import { createInstallerEventEmitter } from './events.js'; import type { CompletionData } from './events.js'; @@ -24,7 +22,7 @@ import type { } from './installer-core.types.js'; import { isScaffoldableEmptyDir, resolvePackageManager, runCreateNextApp } from './scaffold/index.js'; import type { Integration } from './constants.js'; -import { parseEnvFile } from '../utils/env-parser.js'; +import { readProjectEnvCredentials } from './project-env.js'; import { enableDebugLogs, initLogFile, logInfo, logError } from '../utils/debug.js'; import { getAccessToken, saveCredentials, getStagingCredentials, saveStagingCredentials } from './credentials.js'; @@ -64,24 +62,6 @@ async function runIntegrationInstallerFn(integration: Integration, options: Inst return mod.run(options); } -function readExistingCredentials(installDir: string): { apiKey?: string; clientId?: string } { - const envPath = join(installDir, '.env.local'); - if (!existsSync(envPath)) { - return {}; - } - - try { - const content = readFileSync(envPath, 'utf-8'); - const envVars = parseEnvFile(content); - return { - apiKey: envVars.WORKOS_API_KEY || undefined, - clientId: envVars.WORKOS_CLIENT_ID || undefined, - }; - } catch { - return {}; - } -} - async function detectIntegrationFn(options: Pick): Promise { const registry = await getRegistry(); const configs = registry.detectionOrder(); @@ -192,7 +172,7 @@ export async function runWithCore(options: InstallerOptions): Promise { const gatewayUrl = getTelemetryUrl(); analytics.setGatewayUrl(gatewayUrl); - const existingCreds = readExistingCredentials(options.installDir); + const existingCreds = readProjectEnvCredentials(options.installDir); const augmentedOptions: InstallerOptions = { ...options, apiKey: options.apiKey || existingCreds.apiKey, diff --git a/src/lib/unclaimed-env-provision.spec.ts b/src/lib/unclaimed-env-provision.spec.ts index 1014f18b..3b8717d0 100644 --- a/src/lib/unclaimed-env-provision.spec.ts +++ b/src/lib/unclaimed-env-provision.spec.ts @@ -246,6 +246,51 @@ describe('unclaimed-env-provision', () => { expect(result).toBe(false); }); + // Write-site enforcement of the no-clobber invariant: whatever the caller + // did or did not check, the function that writes refuses to overwrite + // credentials the project already has. + describe('refuses when the project env file already has WORKOS_API_KEY', () => { + it('returns false and never calls the provisioning API (JS project, .env.local)', async () => { + writeFileSync(join(testDir, 'package.json'), '{}'); + writeFileSync(join(testDir, '.env.local'), 'WORKOS_API_KEY=sk_existing\nWORKOS_CLIENT_ID=client_existing\n'); + + const result = await tryProvisionUnclaimedEnv({ installDir: testDir }); + + expect(result).toBe(false); + expect(mockProvisionUnclaimedEnvironment).not.toHaveBeenCalled(); + expect(mockSaveConfig).not.toHaveBeenCalled(); + // The existing credentials are still byte-for-byte intact. + expect(readFileSync(join(testDir, '.env.local'), 'utf-8')).toBe( + 'WORKOS_API_KEY=sk_existing\nWORKOS_CLIENT_ID=client_existing\n', + ); + expect(mockUi.log.warn).toHaveBeenCalledWith(expect.stringContaining('already has WORKOS_API_KEY')); + }); + + it('returns false for a non-JS project whose .env already has a key', async () => { + writeFileSync(join(testDir, '.env'), 'WORKOS_API_KEY=sk_existing\n'); + + const result = await tryProvisionUnclaimedEnv({ installDir: testDir }); + + expect(result).toBe(false); + expect(mockProvisionUnclaimedEnvironment).not.toHaveBeenCalled(); + expect(readFileSync(join(testDir, '.env'), 'utf-8')).toBe('WORKOS_API_KEY=sk_existing\n'); + }); + + it('still provisions when the env file exists without a WorkOS key', async () => { + writeFileSync(join(testDir, 'package.json'), '{}'); + writeFileSync(join(testDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); + mockProvisionUnclaimedEnvironment.mockResolvedValueOnce(validProvisionResult); + + const result = await tryProvisionUnclaimedEnv({ installDir: testDir }); + + expect(result).toBe(true); + expect(mockProvisionUnclaimedEnvironment).toHaveBeenCalled(); + const content = readFileSync(join(testDir, '.env.local'), 'utf-8'); + expect(content).toContain('DATABASE_URL=postgres://localhost/dev'); + expect(content).toContain('WORKOS_API_KEY=sk_test_oneshot'); + }); + }); + it('writes redirect URI to .env.local when provided (JS project)', async () => { writeFileSync(join(testDir, 'package.json'), '{}'); mockProvisionUnclaimedEnvironment.mockResolvedValueOnce(validProvisionResult); diff --git a/src/lib/unclaimed-env-provision.ts b/src/lib/unclaimed-env-provision.ts index 8dbf0071..f3d828a9 100644 --- a/src/lib/unclaimed-env-provision.ts +++ b/src/lib/unclaimed-env-provision.ts @@ -37,6 +37,19 @@ export async function tryProvisionUnclaimedEnv(options: UnclaimedEnvProvisionOpt try { logInfo('[unclaimed-env-provision] Attempting unclaimed environment provisioning'); + // No-clobber invariant, enforced by the function that does the writing: + // never overwrite credentials the project already has. Keyed on credentials + // only (never on AuthKit detection) so `install --force` still works. + // Checked before provisioning so no environment is created then abandoned. + const { readProjectEnvCredentials, resolveProjectEnvPath } = await import('./project-env.js'); + if (readProjectEnvCredentials(options.installDir).apiKey) { + logInfo('[unclaimed-env-provision] Refusing to provision: project env already has WORKOS_API_KEY'); + ui.log.warn( + `${resolveProjectEnvPath(options.installDir)} already has WORKOS_API_KEY — not provisioning a new environment.`, + ); + return false; + } + const result = await provisionUnclaimedEnvironment(); // Write .env.local first — if this fails, config stays clean (no orphan entries) diff --git a/src/lib/workos-management.spec.ts b/src/lib/workos-management.spec.ts new file mode 100644 index 00000000..52aca27a --- /dev/null +++ b/src/lib/workos-management.spec.ts @@ -0,0 +1,282 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import type { EnvironmentConfig } from './config-store.js'; + +// Provenance is derived in-module from the config store, so the store is the +// only seam the tests need to drive. `isUnclaimedEnvironment` keeps its real +// (trivial) behavior so a fixture's `type` alone decides the branch. +const getActiveEnvironment = vi.fn<() => EnvironmentConfig | null>(() => null); + +vi.mock('./config-store.js', () => ({ + getActiveEnvironment: () => getActiveEnvironment(), + isUnclaimedEnvironment: (env: EnvironmentConfig) => env.type === 'unclaimed', +})); + +vi.mock('../utils/analytics.js', () => ({ + analytics: { capture: vi.fn(), captureException: vi.fn() }, +})); + +const { analytics } = await import('../utils/analytics.js'); +const ui = (await import('../utils/ui.js')).default; +const { autoConfigureWorkOSEnvironment } = await import('./workos-management.js'); + +const API_KEY = 'sk_test_123'; +const HOMEPAGE_ENDPOINT = 'https://api.workos.com/user_management/app_homepage_url'; +/** `autoConfigureWorkOSEnvironment(apiKey, integration, port)` — port drives every URL. */ +const PORT = 4343; +const BASE_URL = `http://localhost:${PORT}`; + +type FetchCall = { url: string; method: string }; + +function jsonResponse(status: number, body: unknown): Response { + return { + ok: status >= 200 && status < 300, + status, + json: async () => body, + } as unknown as Response; +} + +/** A response whose body is not JSON — `.json()` rejects, like the real thing. */ +function unparseableResponse(status: number): Response { + return { + ok: status >= 200 && status < 300, + status, + json: async () => { + throw new SyntaxError('Unexpected token < in JSON'); + }, + } as unknown as Response; +} + +/** + * Stub `fetch` for the three parallel calls `autoConfigureWorkOSEnvironment` + * makes. `homepage` decides what the newly added GET returns; the two POSTs + * always succeed so failures can only come from the path under test. + */ +function stubFetch(homepage: (method: string) => Response | Promise): { + calls: FetchCall[]; +} { + const calls: FetchCall[] = []; + const stub = vi.fn(async (url: string, init: { method: string }) => { + calls.push({ url, method: init.method }); + if (url === HOMEPAGE_ENDPOINT) return homepage(init.method); + return jsonResponse(201, {}); + }); + vi.stubGlobal('fetch', stub); + return { calls }; +} + +function homepageCalls(calls: FetchCall[], method: string): FetchCall[] { + return calls.filter((c) => c.url === HOMEPAGE_ENDPOINT && c.method === method); +} + +/** The rows array passed to the single `ui.rows` call. */ +function capturedRows(): Array<{ key: string; value: string; status?: string; statusKind?: string }> { + const spy = vi.mocked(ui.rows); + expect(spy).toHaveBeenCalledTimes(1); + return spy.mock.calls[0]![0]; +} + +function rowFor(key: string) { + const row = capturedRows().find((r) => r.key === key); + expect(row, `no "${key}" row was rendered`).toBeDefined(); + return row!; +} + +const unclaimedEnv: EnvironmentConfig = { + name: 'unclaimed-2', + apiKey: API_KEY, + type: 'unclaimed', + clientId: 'client_123', + claimToken: 'tok_123', +}; + +const claimedEnv: EnvironmentConfig = { + name: 'sandbox', + apiKey: API_KEY, + type: 'sandbox', +}; + +/** `Integration` is a plain string identifier (constants.ts:8). */ +const INTEGRATION = 'nextjs'; + +describe('workos-management', () => { + beforeEach(() => { + getActiveEnvironment.mockReset(); + getActiveEnvironment.mockReturnValue(null); + vi.mocked(analytics.capture).mockClear(); + vi.spyOn(ui, 'rows').mockImplementation(() => {}); + vi.spyOn(ui.log, 'step').mockImplementation(() => {}); + vi.spyOn(ui.log, 'success').mockImplementation(() => {}); + vi.spyOn(ui.log, 'info').mockImplementation(() => {}); + vi.spyOn(ui.log, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + describe('setHomepageUrl read-then-write', () => { + it('skips the PUT when the current homepage URL already matches', async () => { + const { calls } = stubFetch(() => jsonResponse(200, { url: BASE_URL })); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(homepageCalls(calls, 'GET')).toHaveLength(1); + expect(homepageCalls(calls, 'PUT')).toHaveLength(0); + expect(result?.homepageUrl).toEqual({ success: true, alreadyExists: true }); + expect(rowFor('Homepage URL')).toMatchObject({ status: 'already set', statusKind: 'muted' }); + }); + + it('issues the PUT when the current homepage URL differs', async () => { + const { calls } = stubFetch((method) => + method === 'GET' ? jsonResponse(200, { url: 'https://app.example.com' }) : jsonResponse(200, {}), + ); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(homepageCalls(calls, 'PUT')).toHaveLength(1); + expect(result?.homepageUrl).toEqual({ success: true, alreadyExists: false }); + expect(rowFor('Homepage URL')).toMatchObject({ value: BASE_URL, status: 'updated', statusKind: 'ok' }); + }); + + it('compares against the caller-supplied homepage URL, not the base URL', async () => { + const custom = 'https://staging.example.com'; + const { calls } = stubFetch(() => jsonResponse(200, { url: custom })); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT, { homepageUrl: custom }); + + expect(homepageCalls(calls, 'PUT')).toHaveLength(0); + expect(result?.homepageUrl.alreadyExists).toBe(true); + }); + + it.each([404, 500])('falls through to the PUT when the GET returns %i', async (status) => { + const { calls } = stubFetch((method) => + method === 'GET' ? jsonResponse(status, { message: 'nope' }) : jsonResponse(200, {}), + ); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(homepageCalls(calls, 'PUT')).toHaveLength(1); + expect(result?.homepageUrl.alreadyExists).toBe(false); + }); + + it('falls through to the PUT when the GET body is not JSON', async () => { + const { calls } = stubFetch((method) => (method === 'GET' ? unparseableResponse(200) : jsonResponse(200, {}))); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(homepageCalls(calls, 'PUT')).toHaveLength(1); + expect(result?.homepageUrl.alreadyExists).toBe(false); + }); + + it('falls through to the PUT when the GET rejects', async () => { + const { calls } = stubFetch((method) => { + if (method === 'GET') return Promise.reject(new TypeError('fetch failed')); + return jsonResponse(200, {}); + }); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(homepageCalls(calls, 'PUT')).toHaveLength(1); + expect(result?.homepageUrl.alreadyExists).toBe(false); + }); + + it('omits a request body on the GET so the read cannot be mistaken for a write', async () => { + const bodies: Array = []; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string, init: { method: string; body?: string }) => { + if (url === HOMEPAGE_ENDPOINT && init.method === 'GET') bodies.push(init.body); + return jsonResponse(200, { url: BASE_URL }); + }), + ); + + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(bodies).toEqual([undefined]); + }); + + it('warns instead of aborting when the PUT fails', async () => { + stubFetch((method) => + method === 'GET' ? jsonResponse(404, {}) : jsonResponse(403, { message: 'API key lacks permission' }), + ); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(result).toBeNull(); + expect(ui.log.warn).toHaveBeenCalledWith(expect.stringContaining('Could not configure WorkOS dashboard')); + expect(ui.rows).not.toHaveBeenCalled(); + }); + + it('reports homepage no-op vs. overwrite to analytics', async () => { + stubFetch(() => jsonResponse(200, { url: BASE_URL })); + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + expect(analytics.capture).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ homepageUrl: 'existed' }), + ); + + vi.mocked(analytics.capture).mockClear(); + vi.mocked(ui.rows).mockClear(); + stubFetch((method) => (method === 'GET' ? jsonResponse(404, {}) : jsonResponse(200, {}))); + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + expect(analytics.capture).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ homepageUrl: 'updated' }), + ); + }); + }); + + describe('credential provenance row', () => { + beforeEach(() => { + stubFetch(() => jsonResponse(200, { url: BASE_URL })); + }); + + it('renders Environment as the first row', async () => { + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(capturedRows()[0]?.key).toBe('Environment'); + expect(capturedRows().map((r) => r.key)).toEqual(['Environment', 'Redirect URI', 'CORS origin', 'Homepage URL']); + }); + + it('names an unclaimed environment and points at `env claim`', async () => { + getActiveEnvironment.mockReturnValue(unclaimedEnv); + + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + const value = rowFor('Environment').value; + expect(value).toContain('unclaimed'); + expect(value).toContain('unclaimed-2'); + expect(value).toContain('env claim'); + }); + + it('names a claimed active environment', async () => { + getActiveEnvironment.mockReturnValue(claimedEnv); + + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + const value = rowFor('Environment').value; + expect(value).toBe('your active environment (sandbox)'); + expect(value).not.toContain('env claim'); + }); + + it('falls back to the supplied-key wording with no active environment', async () => { + getActiveEnvironment.mockReturnValue(null); + + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(rowFor('Environment').value).toBe('the API key supplied to this run'); + }); + + it('degrades to the supplied-key wording when the config store throws', async () => { + getActiveEnvironment.mockImplementation(() => { + throw new Error('keyring locked'); + }); + + const result = await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(result).not.toBeNull(); + expect(rowFor('Environment').value).toBe('the API key supplied to this run'); + }); + }); +}); diff --git a/src/lib/workos-management.ts b/src/lib/workos-management.ts index be8cd6df..15eb3a63 100644 --- a/src/lib/workos-management.ts +++ b/src/lib/workos-management.ts @@ -1,15 +1,23 @@ import type { Integration } from './constants.js'; +import type { EnvironmentConfig } from './config-store.js'; import { INSTALLER_INTERACTION_EVENT_NAME } from './constants.js'; import { analytics } from '../utils/analytics.js'; +import { formatWorkOSCommand } from '../utils/command-invocation.js'; import ui from '../utils/ui.js'; +import { getActiveEnvironment, isUnclaimedEnvironment } from './config-store.js'; import { getCallbackPath } from './port-detection.js'; const WORKOS_API_BASE = 'https://api.workos.com'; +const HOMEPAGE_URL_ENDPOINT = '/user_management/app_homepage_url'; + +/** Provenance wording when no stored environment can be named. */ +const SUPPLIED_KEY_PROVENANCE = 'the API key supplied to this run'; + export interface AutoConfigResult { redirectUri: { success: boolean; alreadyExists: boolean }; corsOrigin: { success: boolean; alreadyExists: boolean }; - homepageUrl: { success: boolean }; + homepageUrl: { success: boolean; alreadyExists: boolean }; } interface FetchError { @@ -19,10 +27,10 @@ interface FetchError { } async function workosRequest( - method: 'POST' | 'PUT', + method: 'GET' | 'POST' | 'PUT', endpoint: string, apiKey: string, - body: Record, + body?: Record, ): Promise { return fetch(`${WORKOS_API_BASE}${endpoint}`, { method, @@ -30,7 +38,7 @@ async function workosRequest( Authorization: `Bearer ${apiKey}`, 'Content-Type': 'application/json', }, - body: JSON.stringify(body), + ...(body ? { body: JSON.stringify(body) } : {}), }); } @@ -89,17 +97,64 @@ async function createCorsOrigin(apiKey: string, origin: string): Promise<{ succe } /** - * Set the app homepage URL in WorkOS. + * Set the app homepage URL in WorkOS, skipping the write when it already matches. + * + * The homepage URL is a single-valued setting, so an unconditional PUT silently + * overwrites whatever a logged-in user already had configured. Reading first + * makes the common case a no-op that reports itself honestly. + * + * A read failure is not fatal: fall through to the PUT, which is the old + * behavior, rather than abandoning configuration over a GET we just added. */ -async function setHomepageUrl(apiKey: string, url: string): Promise<{ success: boolean }> { - const response = await workosRequest('PUT', '/user_management/app_homepage_url', apiKey, { url }); +async function setHomepageUrl(apiKey: string, url: string): Promise<{ success: boolean; alreadyExists: boolean }> { + try { + const current = await workosRequest('GET', HOMEPAGE_URL_ENDPOINT, apiKey); + if (current.ok) { + const data = (await current.json()) as { url?: string } | null; + if (data?.url === url) { + return { success: true, alreadyExists: true }; + } + } + } catch { + // Read failed (endpoint missing, non-JSON body, network error) — fall + // through to the write so behavior is never worse than before. + } + + const response = await workosRequest('PUT', HOMEPAGE_URL_ENDPOINT, apiKey, { url }); if (!response.ok) { const error = await parseFetchError(response); throw new Error(error.message || `HTTP ${error.status}`); } - return { success: true }; + return { success: true, alreadyExists: false }; +} + +/** + * Where the credentials being used came from, so the rows below say *where* the + * writes landed and not just what was written. + * + * Derived locally: the WorkOS API exposes no environment identity this module + * can reach (`workosRequest` is GET/POST/PUT against user_management only, and + * the config store holds no environment id). `activeEnvironment.name` is a + * local label, so it is only ever a parenthetical, never an authoritative id. + */ +function describeCredentialProvenance(): string { + let activeEnv: EnvironmentConfig | null = null; + try { + activeEnv = getActiveEnvironment(); + } catch { + // Keyring locked or unavailable — a label is not worth aborting over. + return SUPPLIED_KEY_PROVENANCE; + } + + if (!activeEnv) return SUPPLIED_KEY_PROVENANCE; + + if (isUnclaimedEnvironment(activeEnv)) { + return `a new unclaimed environment (${activeEnv.name}) — run \`${formatWorkOSCommand('env claim')}\` to keep it`; + } + + return `your active environment (${activeEnv.name})`; } export interface AutoConfigOptions { @@ -148,12 +203,15 @@ export async function autoConfigureWorkOSEnvironment( port, redirectUri: redirectUri.alreadyExists ? 'existed' : 'created', corsOrigin: corsOrigin.alreadyExists ? 'existed' : 'created', + homepageUrl: homepageUrl.alreadyExists ? 'existed' : 'updated', }); // Aligned key/value feedback: value in accent, a dim status for "already - // existed" vs. a green status for a fresh create/update. + // existed" vs. a green status for a fresh create/update. The provenance row + // comes first — it is the context for the three rows below it. ui.log.success('WorkOS dashboard configured'); ui.rows([ + { key: 'Environment', value: describeCredentialProvenance(), statusKind: 'muted' }, { key: 'Redirect URI', value: callbackUrl, @@ -166,7 +224,12 @@ export async function autoConfigureWorkOSEnvironment( status: corsOrigin.alreadyExists ? 'already set' : 'created', statusKind: corsOrigin.alreadyExists ? 'muted' : 'ok', }, - { key: 'Homepage URL', value: homepageUrlValue, status: 'updated', statusKind: 'ok' }, + { + key: 'Homepage URL', + value: homepageUrlValue, + status: homepageUrl.alreadyExists ? 'already set' : 'updated', + statusKind: homepageUrl.alreadyExists ? 'muted' : 'ok', + }, ]); return results; From 057f58bfe2cd932e5499d34412ee6ea09321005b Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Fri, 24 Jul 2026 21:27:25 -0500 Subject: [PATCH 2/3] fix: widen the no-clobber check to every project env file Review found the no-clobber refusal only inspected one env file (`.env.local` whenever package.json exists), while credential-discovery already treats four files as WorkOS credential sources. A JS project whose real key lives in `.env`, with no AuthKit SDK yet, therefore still got a throwaway environment provisioned and `.env.local` written with it. `.env.local` outranks `.env` in Next.js/Vite/Remix/SvelteKit load order, so the app silently authenticated against an empty environment while the real key sat on disk looking correct: the read-back-your-own- write bug, surviving in the dashboard-first setup flow. `readProjectEnvCredentials` now scans all four files in framework precedence order and tolerates `export ` prefixes and indentation. `resolveProjectEnvPath` is unchanged: it is the write target, and conflating "which file would I clobber" with "does this project already have credentials" was the defect. Also from review: - Gate the preflight guard on `isJsonMode()` as well as `isPromptAllowed()`. `--json` on a real TTY took the interactive branch, printing prose into the machine-readable stream and failing as `prompt_unavailable` rather than `authkit_already_installed`. - Mirror the source file's permission bits onto the env backup, and birth new env files 0600. A `chmod 600 .env.local` was getting a world-readable twin holding a live API key, outside the `.env*` glob most secret scanners watch. - Preserve CRLF in env upserts instead of rewriting a Windows file to LF. - Explain the refusal on the path that actually fires. It previously logged to the debug file only, so the user saw a bare exit-4 `auth_required` indistinguishable from a provisioning failure. - Don't promise to rewrite an env file whose credentials will be kept. - Thread the API key into the provenance row, so a `--api-key` run stops naming a stored environment that the writes never touched. - Register `--force` in the machine-readable command tree, since the new error tells agents to pass it. - Change installer-core's empty-message fallback, which was byte- identical to the classifier's deterministic signature and would have rendered a git or filesystem failure as an AI-service failure. Test hardening for three assertions that passed with the fix reverted: the headless adapter had no coverage of the shared classifier at all, and the upstream_timeout and cli-adapter cases asserted only pre-existing behavior. --- src/lib/adapters/cli-adapter.spec.ts | 8 +- src/lib/adapters/headless-adapter.spec.ts | 33 ++++++++ src/lib/agent-interface.spec.ts | 8 +- src/lib/env-writer.spec.ts | 72 +++++++++++++++- src/lib/env-writer.ts | 43 ++++++++-- src/lib/installer-core.ts | 6 +- src/lib/preflight-authkit.spec.ts | 47 +++++++++++ src/lib/preflight-authkit.ts | 20 +++-- src/lib/project-env.ts | 94 ++++++++++++++++----- src/lib/resolve-install-credentials.spec.ts | 87 ++++++++++++++++++- src/lib/resolve-install-credentials.ts | 17 +++- src/lib/workos-management.spec.ts | 23 +++++ src/lib/workos-management.ts | 11 ++- src/utils/help-json.ts | 11 +++ 14 files changed, 431 insertions(+), 49 deletions(-) diff --git a/src/lib/adapters/cli-adapter.spec.ts b/src/lib/adapters/cli-adapter.spec.ts index 6ef14446..07fd6654 100644 --- a/src/lib/adapters/cli-adapter.spec.ts +++ b/src/lib/adapters/cli-adapter.spec.ts @@ -398,7 +398,13 @@ describe('CLIAdapter', () => { ); expect(output).not.toMatch(/temporarily unavailable/i); - expect(output).toMatch(/could not complete this request/i); + + // The headline must be re-derived, not echoed: a pass-through of the + // already-rendered string would put all three sentences in log.error. + const ui = await import('../../utils/ui.js'); + expect(vi.mocked(ui.default.log.error).mock.calls[0]?.[0]).toBe( + 'The AI service could not complete this request.', + ); }); it('still shows the transient copy for a real 503', async () => { diff --git a/src/lib/adapters/headless-adapter.spec.ts b/src/lib/adapters/headless-adapter.spec.ts index ec1c2e6f..e053a996 100644 --- a/src/lib/adapters/headless-adapter.spec.ts +++ b/src/lib/adapters/headless-adapter.spec.ts @@ -455,6 +455,39 @@ describe('HeadlessAdapter', () => { }); await adapter.stop(); }); + + // The gateway's generic 500 branch also fires for request-shape failures, so + // agents and CI must not be told to wait it out: the retry fails identically. + it('codes the gateway generic 500 as deterministic, not a transient outage', async () => { + const adapter = createAdapter(); + await adapter.start(); + + emitter.emit('error', { message: 'API Error: 500 An unexpected error occurred', stack: undefined }); + + expect(mockWriteNDJSON).toHaveBeenCalledWith({ + type: 'error', + code: 'deterministic_error', + message: expect.stringContaining('likely to fail the same way'), + }); + expect(mockWriteNDJSON).not.toHaveBeenCalledWith( + expect.objectContaining({ message: expect.stringContaining('temporarily unavailable') }), + ); + await adapter.stop(); + }); + + it('still codes a real 503 as service_unavailable', async () => { + const adapter = createAdapter(); + await adapter.start(); + + emitter.emit('error', { message: 'API Error: 503 Service Unavailable', stack: undefined }); + + expect(mockWriteNDJSON).toHaveBeenCalledWith({ + type: 'error', + code: 'service_unavailable', + message: expect.stringContaining('temporarily unavailable'), + }); + await adapter.stop(); + }); }); describe('staging events', () => { diff --git a/src/lib/agent-interface.spec.ts b/src/lib/agent-interface.spec.ts index 2ab3883e..24fc5a03 100644 --- a/src/lib/agent-interface.spec.ts +++ b/src/lib/agent-interface.spec.ts @@ -315,10 +315,15 @@ describe('service unavailability handling', () => { expect(result.errorMessage).toMatch(/likely to fail the same way/); }); + // The proxy answers its own socket timeout with a 504, so the message carries a + // 5xx that the old "any 5xx is transient" rule misread as an outage. it("treats the proxy's own upstream_timeout as deterministic", async () => { mockQuery.mockImplementation( createMockSDKResponse([ - { text: '{"error":"upstream_timeout","message":"Upstream server timed out"}', is_error: true }, + { + text: 'API Error: 504 {"error":"upstream_timeout","message":"Upstream server timed out"}', + is_error: true, + }, ]), ); @@ -326,6 +331,7 @@ describe('service unavailability handling', () => { expect(result.error).toBe(AgentErrorType.EXECUTION_ERROR); expect(result.errorMessage).not.toMatch(/temporarily unavailable/); + expect(result.errorMessage).toMatch(/likely to fail the same way/); }); it('detects a 503 as SERVICE_UNAVAILABLE', async () => { diff --git a/src/lib/env-writer.spec.ts b/src/lib/env-writer.spec.ts index 547b5bf3..4412238a 100644 --- a/src/lib/env-writer.spec.ts +++ b/src/lib/env-writer.spec.ts @@ -1,9 +1,15 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { writeCredentialsEnv, writeEnvLocal } from './env-writer.js'; +/** POSIX permission bits of `path`. */ +const modeOf = (path: string): number => statSync(path).mode & 0o777; + +// Windows has no meaningful POSIX permission bits. +const itPosix = it.skipIf(process.platform === 'win32'); + describe('writeEnvLocal', () => { let testDir: string; @@ -220,6 +226,70 @@ describe('writeEnvLocal', () => { expect(readFileSync(envPath, 'utf-8')).toBe('# top\nWORKOS_COOKIE_PASSWORD=pw\nWORKOS_CLIENT_ID=client_123\n'); }); + + it('keeps CRLF line endings on touched, untouched, and appended lines', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, '# top\r\nWORKOS_CLIENT_ID=client_old\r\nDATABASE_URL=postgres://localhost/dev\r\n'); + + writeEnvLocal(testDir, { + WORKOS_CLIENT_ID: 'client_new', + WORKOS_COOKIE_PASSWORD: 'pw', + }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + '# top\r\nWORKOS_CLIENT_ID=client_new\r\nDATABASE_URL=postgres://localhost/dev\r\nWORKOS_COOKIE_PASSWORD=pw\r\n', + ); + }); + + it('keeps CRLF endings on a file that lacked a trailing newline', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, '# top\r\nWORKOS_COOKIE_PASSWORD=pw'); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_123' }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + '# top\r\nWORKOS_COOKIE_PASSWORD=pw\r\nWORKOS_CLIENT_ID=client_123\r\n', + ); + }); + }); + + describe('file permissions', () => { + const envVars = { + WORKOS_CLIENT_ID: 'client_123', + WORKOS_COOKIE_PASSWORD: 'pw', + }; + + itPosix('mirrors the source file mode onto the backup', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, 'WORKOS_API_KEY=sk_live_secret\n'); + chmodSync(envPath, 0o600); + + writeEnvLocal(testDir, envVars); + + expect(modeOf(join(testDir, '.env.local.bak'))).toBe(0o600); + }); + + itPosix('creates a new env file as 0600', () => { + writeEnvLocal(testDir, envVars); + + expect(modeOf(join(testDir, '.env.local'))).toBe(0o600); + }); + + itPosix('leaves the permissions of an existing env file alone', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, 'DATABASE_URL=postgres://localhost/dev\n'); + chmodSync(envPath, 0o640); + + writeEnvLocal(testDir, envVars); + + expect(modeOf(envPath)).toBe(0o640); + }); + + itPosix('creates a new .env as 0600 outside the JS branch', () => { + writeCredentialsEnv(testDir, envVars); + + expect(modeOf(join(testDir, '.env'))).toBe(0o600); + }); }); describe('backup', () => { diff --git a/src/lib/env-writer.ts b/src/lib/env-writer.ts index 5784362a..ad081766 100644 --- a/src/lib/env-writer.ts +++ b/src/lib/env-writer.ts @@ -1,4 +1,4 @@ -import { existsSync, readFileSync, writeFileSync } from 'fs'; +import { existsSync, readFileSync, statSync, writeFileSync } from 'fs'; import { basename, join } from 'path'; import { parseEnvFile } from '../utils/env-parser.js'; @@ -61,10 +61,18 @@ function generateCookiePassword(): string { * containing `=` survive. Duplicate keys in a malformed source file: only the * first occurrence is rewritten (`parseEnvFile` reads last-wins, so a duplicate * still shadows the update — a pre-existing pathology, not handled here). + * + * The file's dominant line terminator is detected once and reused, so a CRLF + * file is not silently rewritten to LF (which would turn a two-line change into + * a whole-file diff and leave mixed endings behind). */ function upsertEnvLines(content: string, vars: Record): string { - const hadTrailingNewline = content.endsWith('\n'); - const body = hadTrailingNewline ? content.slice(0, -1) : content; + const eol = content.includes('\r\n') ? '\r\n' : '\n'; + const body = content.endsWith('\r\n') + ? content.slice(0, -2) + : content.endsWith('\n') + ? content.slice(0, -1) + : content; const pending = new Map(Object.entries(vars)); // An empty body has no lines at all — splitting it would invent a blank one. @@ -88,7 +96,17 @@ function upsertEnvLines(content: string, vars: Record): string { } // Always exactly one trailing newline, matching the previous writer. - return lines.join('\n') + '\n'; + return lines.join(eol) + eol; +} + +/** + * Write a secrets-bearing file, birthing a NEW file as 0600. + * + * `mode` is only honored on create, so an existing file's permissions are the + * user's business and are left exactly as they are. + */ +function writeSecretFile(path: string, contents: string, existed: boolean): void { + writeFileSync(path, contents, existed ? {} : { mode: 0o600 }); } /** @@ -102,6 +120,11 @@ function upsertEnvLines(content: string, vars: Record): string { * `ensureGitignore` runs BEFORE the write so a crash between the two cannot * leave an unignored secret on disk: `stageAndCommit` runs `git add -A`, and the * env file holds a live API key and claim token. + * + * The copy mirrors the source's permission bits: a `chmod 600 .env.local` must + * not gain a world-readable twin, and `.bak` sits outside the `.env*` glob most + * secret-scanning and editor-hiding tooling watches. The `existsSync` guard + * above guarantees this write is always a create, so `mode` is honored. */ function backupEnvFile(installDir: string, envPath: string): void { if (!existsSync(envPath)) return; // nothing to back up @@ -110,7 +133,7 @@ function backupEnvFile(installDir: string, envPath: string): void { const backupName = basename(backupPath); ensureGitignore(installDir, backupName, [backupName, '.env*', '*.bak']); - writeFileSync(backupPath, readFileSync(envPath, 'utf-8')); + writeFileSync(backupPath, readFileSync(envPath, 'utf-8'), { mode: statSync(envPath).mode & 0o777 }); } /** Drop keys whose value is undefined so they never serialize as `KEY=undefined`. */ @@ -133,7 +156,8 @@ export function writeEnvLocal(installDir: string, envVars: Partial): vo backupEnvFile(installDir, envPath); - const existingContent = existsSync(envPath) ? readFileSync(envPath, 'utf-8') : ''; + const envExisted = existsSync(envPath); + const existingContent = envExisted ? readFileSync(envPath, 'utf-8') : ''; const existingEnv = parseEnvFile(existingContent); const vars = definedVars(envVars); @@ -145,7 +169,7 @@ export function writeEnvLocal(installDir: string, envVars: Partial): vo ensureGitignore(installDir, '.env.local', ENV_LOCAL_COVERING_PATTERNS); - writeFileSync(envPath, upsertEnvLines(existingContent, vars)); + writeSecretFile(envPath, upsertEnvLines(existingContent, vars), envExisted); } /** @@ -173,9 +197,10 @@ export function writeCredentialsEnv(installDir: string, envVars: Partial { - const message = context.error?.message ?? 'An unexpected error occurred'; + // Must NOT be 'An unexpected error occurred': that is the LLM gateway's + // generic-500 signature, which the failure classifier reads as a + // deterministic AI failure. A git/fs/detection error reaching the error + // state without an assigned `error` would otherwise be blamed on the AI. + const message = context.error?.message ?? 'The installer failed for an unknown reason'; context.emitter.emit('error', { message, stack: context.error?.stack }); context.emitter.emit('complete', { success: false }); }, diff --git a/src/lib/preflight-authkit.spec.ts b/src/lib/preflight-authkit.spec.ts index c613f6af..7aa3be2d 100644 --- a/src/lib/preflight-authkit.spec.ts +++ b/src/lib/preflight-authkit.spec.ts @@ -4,6 +4,7 @@ import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { CliExit } from '../utils/cli-exit.js'; import { resetInteractionModeForTests, setInteractionMode } from '../utils/interaction-mode.js'; +import { setOutputMode } from '../utils/output.js'; // Mock the UI facade — the interactive branch prompts, which has no place in a unit test. const mockConfirm = vi.fn(); @@ -39,6 +40,7 @@ describe('preflight-authkit', () => { afterEach(() => { rmSync(testDir, { recursive: true, force: true }); resetInteractionModeForTests(); + setOutputMode('human'); errorSpy.mockRestore(); }); @@ -129,6 +131,24 @@ describe('preflight-authkit', () => { expect(mockConfirm).not.toHaveBeenCalled(); }); + // --json on a real TTY: output mode is json while interaction mode stays + // human, so isPromptAllowed() alone is not enough of a gate. Prompting here + // would put plain text in the JSON stream and report prompt_unavailable. + it('refuses without prompting or printing under --json on a TTY (interaction mode human)', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + setInteractionMode({ mode: 'human', source: 'default' }); + setOutputMode('json'); + + const error = await assertNoExistingAuthKit({ installDir: testDir }).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(CliExit); + expect((error as InstanceType).context?.errorCode).toBe('authkit_already_installed'); + expect(mockConfirm).not.toHaveBeenCalled(); + expect(mockUi.log.warn).not.toHaveBeenCalled(); + expect(mockUi.log.info).not.toHaveBeenCalled(); + expect(mockUi.rows).not.toHaveBeenCalled(); + }); + it('also refuses in CI mode', async () => { writePackageJson(testDir, { '@workos-inc/authkit-remix': '^1.0.0' }); setInteractionMode({ mode: 'ci', source: 'ci_env' }); @@ -165,6 +185,33 @@ describe('preflight-authkit', () => { expect(mockUi.log.info).toHaveBeenCalledWith(expect.stringContaining(join(testDir, '.env.local'))); }); + // The consent copy must not promise an outcome that cannot happen: with a key + // already on disk, credential resolution refuses to provision and logs in. + it('promises credentials will be KEPT (not rewritten) when the project env already has a key', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + writeFileSync(join(testDir, '.env.local'), 'WORKOS_API_KEY=sk_existing\n'); + mockConfirm.mockResolvedValueOnce(true); + + await expect(assertNoExistingAuthKit({ installDir: testDir })).resolves.toBeUndefined(); + + const printed = mockUi.log.info.mock.calls.map((call) => String(call[0])).join('\n'); + expect(printed).toContain('will be kept'); + expect(printed).not.toContain('provision a new WorkOS environment'); + }); + + it('names the file the key actually lives in, even when it is not the write target', async () => { + writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); + // JS project, so the write target is .env.local — but the real key is in .env. + writeFileSync(join(testDir, '.env'), 'WORKOS_API_KEY=sk_existing\n'); + mockConfirm.mockResolvedValueOnce(true); + + await expect(assertNoExistingAuthKit({ installDir: testDir })).resolves.toBeUndefined(); + + const printed = mockUi.log.info.mock.calls.map((call) => String(call[0])).join('\n'); + expect(printed).toContain(join(testDir, '.env')); + expect(printed).not.toContain(join(testDir, '.env.local')); + }); + it('throws CliExit (exit 2) and writes nothing when an interactive user declines', async () => { writePackageJson(testDir, { '@workos-inc/authkit-nextjs': '^2.6.0' }); mockConfirm.mockResolvedValueOnce(false); diff --git a/src/lib/preflight-authkit.ts b/src/lib/preflight-authkit.ts index 4fbcb553..c3f8d485 100644 --- a/src/lib/preflight-authkit.ts +++ b/src/lib/preflight-authkit.ts @@ -12,7 +12,7 @@ import { AUTHKIT_PACKAGES } from '../doctor/checks/sdk.js'; import { formatWorkOSCommand } from '../utils/command-invocation.js'; import { ExitCode, exitWithCode } from '../utils/exit-codes.js'; import { isPromptAllowed } from '../utils/interaction-mode.js'; -import { exitWithError } from '../utils/output.js'; +import { exitWithError, isJsonMode } from '../utils/output.js'; import { getPackageVersion, readPackageJson } from '../utils/package-json.js'; import ui from '../utils/ui.js'; @@ -51,8 +51,12 @@ export async function assertNoExistingAuthKit(opts: { installDir: string; force? const summary = found.map((pkg) => `${pkg.name} ${pkg.version}`).join(', '); - // Non-interactive: never prompt, never write. - if (!isPromptAllowed()) { + // Non-interactive: never prompt, never write. `--json` on a real TTY leaves + // interaction mode human (so isPromptAllowed() is still true) while output is + // machine-readable, so it has to be excluded too — otherwise the prompt below + // writes plain text into the JSON stream and fails as `prompt_unavailable` + // instead of `authkit_already_installed`. + if (!isPromptAllowed() || isJsonMode()) { exitWithError({ code: 'authkit_already_installed', message: @@ -64,11 +68,17 @@ export async function assertNoExistingAuthKit(opts: { installDir: string; force? // Interactive: show what was found, then ask. Name the env file explicitly — // it is exactly what is at risk. - const { resolveProjectEnvPath } = await import('./project-env.js'); + const { readProjectEnvCredentials, resolveProjectEnvPath } = await import('./project-env.js'); + const projectEnv = readProjectEnvCredentials(opts.installDir); ui.log.warn('AuthKit is already installed here'); ui.rows(found.map((pkg) => ({ key: pkg.name, value: pkg.version, statusKind: 'muted' as const }))); + // Only promise a rewrite when one can actually happen: with a key already on + // disk, credential resolution refuses to provision and logs in instead, so + // continuing changes code only. ui.log.info( - `Continuing will provision a new WorkOS environment and rewrite ${resolveProjectEnvPath(opts.installDir)}.`, + projectEnv.apiKey + ? `Your existing WorkOS credentials in ${projectEnv.apiKeyPath ?? resolveProjectEnvPath(opts.installDir)} will be kept — continuing changes code only.` + : `Continuing will provision a new WorkOS environment and rewrite ${resolveProjectEnvPath(opts.installDir)}.`, ); ui.log.info(`To inspect the existing install instead: ${formatWorkOSCommand('doctor')}`); diff --git a/src/lib/project-env.ts b/src/lib/project-env.ts index 2d0351e3..4e0e27fd 100644 --- a/src/lib/project-env.ts +++ b/src/lib/project-env.ts @@ -1,15 +1,46 @@ /** - * Reader for the project's WorkOS env file. + * Reader for the project's WorkOS env files. * - * One reader that resolves the same file `writeCredentialsEnv` would write, so - * the "is this project already configured?" check and the write can never - * disagree. Kept separate from `run-with-core.ts` so the credential path does - * not pull xstate and the whole installer graph in behind it. + * Two halves that must not be confused: + * - `resolveProjectEnvPath` is the WRITE target — the single file + * `writeCredentialsEnv` would create, so the write and the warning copy can + * never disagree. + * - `readProjectEnvCredentials` is the READ side — every file that counts as an + * existing WorkOS configuration, because provisioning must refuse whenever the + * project already has a key, wherever it happens to live. + * + * Kept separate from `run-with-core.ts` so the credential path does not pull + * xstate and the whole installer graph in behind it. */ import { existsSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; -import { parseEnvFile } from '../utils/env-parser.js'; + +/** + * Env files that count as an existing WorkOS configuration, in the precedence + * order frameworks load them (earlier wins). + * + * KEEP IN SYNC: `ENV_FILE_NAMES` in ./credential-discovery.ts, which already + * treats all four as WorkOS credential sources. A file that discovery reads a + * key out of must also block provisioning: `.env.local` outranks `.env` in + * Next.js / Vite / Remix / SvelteKit, so provisioning into `.env.local` over a + * project configured through `.env` silently points the app at an empty + * throwaway environment while the real key sits on disk looking correct. + */ +export const ENV_FILE_NAMES = ['.env.local', '.env.development.local', '.env.development', '.env'] as const; + +// Leading indentation and an `export ` prefix are both common in env files that +// people also `source`, so both are tolerated. A `#`-commented line can never +// match: only spaces and tabs are allowed before the name. +const WORKOS_API_KEY_PATTERN = /^[ \t]*(?:export[ \t]+)?WORKOS_API_KEY[ \t]*=[ \t]*["']?([^"'\s#]+)["']?/m; +const WORKOS_CLIENT_ID_PATTERN = /^[ \t]*(?:export[ \t]+)?WORKOS_CLIENT_ID[ \t]*=[ \t]*["']?([^"'\s#]+)["']?/m; + +export interface ProjectEnvCredentials { + apiKey?: string; + clientId?: string; + /** Absolute path of the file `apiKey` was read from, when one was found. */ + apiKeyPath?: string; +} /** * The env file the CLI would write for this project. @@ -17,30 +48,49 @@ import { parseEnvFile } from '../utils/env-parser.js'; * Mirrors `writeCredentialsEnv` (`lib/env-writer.ts`): `.env.local` for JS * projects (package.json present), `.env` for everything else. If that rule * changes, both sides must change together. + * + * This is the write target only — it is NOT the set of files that count as + * already-configured. Use `readProjectEnvCredentials` for that. */ export function resolveProjectEnvPath(installDir: string): string { return existsSync(join(installDir, 'package.json')) ? join(installDir, '.env.local') : join(installDir, '.env'); } /** - * WorkOS credentials already present in the project's env file. + * WorkOS credentials already present in any of the project's env files. + * + * Scans `ENV_FILE_NAMES` in precedence order and takes the first value found + * for each key — the same per-key, first-file-wins merge the frameworks + * themselves perform, so what we read is what the app would see. * - * Read failures resolve to `{}` — a malformed env file should not crash the - * installer, it should just look unconfigured. + * Read failures are skipped per file — a malformed or unreadable env file should + * not crash the installer, it should just look unconfigured. */ -export function readProjectEnvCredentials(installDir: string): { apiKey?: string; clientId?: string } { - const envPath = resolveProjectEnvPath(installDir); - if (!existsSync(envPath)) { - return {}; - } +export function readProjectEnvCredentials(installDir: string): ProjectEnvCredentials { + const found: ProjectEnvCredentials = {}; + + for (const fileName of ENV_FILE_NAMES) { + const envPath = join(installDir, fileName); + if (!existsSync(envPath)) continue; - try { - const envVars = parseEnvFile(readFileSync(envPath, 'utf-8')); - return { - apiKey: envVars.WORKOS_API_KEY || undefined, - clientId: envVars.WORKOS_CLIENT_ID || undefined, - }; - } catch { - return {}; + let content: string; + try { + content = readFileSync(envPath, 'utf-8'); + } catch { + continue; + } + + if (!found.apiKey) { + const apiKey = content.match(WORKOS_API_KEY_PATTERN)?.[1]; + if (apiKey) { + found.apiKey = apiKey; + found.apiKeyPath = envPath; + } + } + found.clientId ??= content.match(WORKOS_CLIENT_ID_PATTERN)?.[1]; + + if (found.apiKey && found.clientId) break; } + + return found; } diff --git a/src/lib/resolve-install-credentials.spec.ts b/src/lib/resolve-install-credentials.spec.ts index 9098cb67..5bc6026d 100644 --- a/src/lib/resolve-install-credentials.spec.ts +++ b/src/lib/resolve-install-credentials.spec.ts @@ -23,7 +23,14 @@ vi.mock('./unclaimed-env-provision.js', () => ({ tryProvisionUnclaimedEnv: (...args: unknown[]) => mockTryProvisionUnclaimedEnv(...args), })); +// Mock the UI facade — the no-clobber branch now explains itself out loud. +const mockUi = { + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), step: vi.fn(), success: vi.fn(), hint: vi.fn() }, +}; +vi.mock('../utils/ui.js', () => ({ default: mockUi })); + const { resolveInstallCredentials } = await import('./resolve-install-credentials.js'); +const { setOutputMode } = await import('../utils/output.js'); describe('resolveInstallCredentials', () => { const mockAuthenticate = vi.fn(); @@ -43,6 +50,7 @@ describe('resolveInstallCredentials', () => { afterEach(() => { cwdSpy.mockRestore(); + setOutputMode('human'); rmSync(emptyCwd, { recursive: true, force: true }); if (originalEnv !== undefined) { process.env.WORKOS_API_KEY = originalEnv; @@ -221,15 +229,86 @@ describe('resolveInstallCredentials', () => { expect(mockTryProvisionUnclaimedEnv).toHaveBeenCalledWith({ installDir: projectDir }); }); - // A JS project's .env is NOT the file the CLI would write, so it must not - // suppress provisioning — the check has to mirror writeCredentialsEnv exactly. - it('ignores a stale .env in a JS project (writeCredentialsEnv targets .env.local there)', async () => { + // The read side must cover every file the CLI itself treats as a credential + // source (credential-discovery.ts), not just the write target. `.env.local` + // outranks `.env` in Next.js/Vite/Remix/SvelteKit, so provisioning here would + // point the app at an empty throwaway env while the real key sits in `.env`. + it('refuses to provision when a JS project keeps its key in .env and has no .env.local', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env'), 'WORKOS_API_KEY=sk_real\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).not.toHaveBeenCalled(); + expect(mockAuthenticate).toHaveBeenCalled(); + }); + + it('refuses to provision when the key is in .env.development.local', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env.development.local'), 'WORKOS_API_KEY=sk_real\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).not.toHaveBeenCalled(); + expect(mockAuthenticate).toHaveBeenCalled(); + }); + + it('refuses to provision when the key is in .env.development', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env.development'), 'WORKOS_API_KEY=sk_real\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).not.toHaveBeenCalled(); + expect(mockAuthenticate).toHaveBeenCalled(); + }); + + // `export`-prefixed and indented lines are common in env files people `source`. + it('refuses to provision for `export WORKOS_API_KEY=` syntax with leading whitespace', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env.local'), ' export WORKOS_API_KEY="sk_real"\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockTryProvisionUnclaimedEnv).not.toHaveBeenCalled(); + expect(mockAuthenticate).toHaveBeenCalled(); + }); + + it('still provisions when no env file anywhere carries a WorkOS key', async () => { writeFileSync(join(projectDir, 'package.json'), '{}'); - writeFileSync(join(projectDir, '.env'), 'WORKOS_API_KEY=sk_stale\n'); + writeFileSync(join(projectDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); + writeFileSync(join(projectDir, '.env.development'), '# WORKOS_API_KEY=sk_commented_out\n'); + writeFileSync(join(projectDir, '.env'), 'NEXT_PUBLIC_URL=http://localhost:3000\n'); await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); expect(mockTryProvisionUnclaimedEnv).toHaveBeenCalledWith({ installDir: projectDir }); + expect(mockAuthenticate).not.toHaveBeenCalled(); + }); + + // Exit 4 from the login that follows is byte-identical to a provisioning + // network failure, so the refusal has to say why out loud. + it('tells the user the existing key was found and kept, naming the file it is in', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env'), 'WORKOS_API_KEY=sk_real\n'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + const printed = mockUi.log.info.mock.calls.map((call) => String(call[0])).join('\n'); + // The file the key actually lives in, not the write target (.env.local here). + expect(printed).toContain(`${join(projectDir, '.env')} already has WORKOS_API_KEY`); + expect(printed).toContain('Signing you in'); + }); + + it('stays silent in JSON mode so the machine-readable stream is not corrupted', async () => { + writeFileSync(join(projectDir, 'package.json'), '{}'); + writeFileSync(join(projectDir, '.env.local'), 'WORKOS_API_KEY=sk_real\n'); + setOutputMode('json'); + + await resolveInstallCredentials(undefined, projectDir, undefined, mockAuthenticate); + + expect(mockUi.log.info).not.toHaveBeenCalled(); + expect(mockAuthenticate).toHaveBeenCalled(); }); }); }); diff --git a/src/lib/resolve-install-credentials.ts b/src/lib/resolve-install-credentials.ts index 1ba4f313..1c352f8e 100644 --- a/src/lib/resolve-install-credentials.ts +++ b/src/lib/resolve-install-credentials.ts @@ -45,10 +45,23 @@ export async function resolveInstallCredentials( // the project's env file, so a key already sitting there means we must not // provision. Fall back to login instead — the project has a key, we just // have no gateway auth. - const { readProjectEnvCredentials } = await import('./project-env.js'); - if (readProjectEnvCredentials(dir).apiKey) { + const { readProjectEnvCredentials, resolveProjectEnvPath } = await import('./project-env.js'); + const projectEnv = readProjectEnvCredentials(dir); + if (projectEnv.apiKey) { const { logInfo } = await import('../utils/debug.js'); logInfo('[resolve-install-credentials] Project env already has WORKOS_API_KEY — skipping provisioning'); + + // Say it out loud. This is the branch that actually fires, and without a + // line here the login that follows looks identical to a provisioning + // network failure — the user never learns their key was found and kept. + const { isJsonMode } = await import('../utils/output.js'); + if (!isJsonMode()) { + const ui = (await import('../utils/ui.js')).default; + const envPath = projectEnv.apiKeyPath ?? resolveProjectEnvPath(dir); + ui.log.info(`${envPath} already has WORKOS_API_KEY — keeping it.`); + if (!skipAuth) ui.log.info('Signing you in so the AI installer can run.'); + } + if (!skipAuth) await authenticate(); return; } diff --git a/src/lib/workos-management.spec.ts b/src/lib/workos-management.spec.ts index 52aca27a..691b17ba 100644 --- a/src/lib/workos-management.spec.ts +++ b/src/lib/workos-management.spec.ts @@ -268,6 +268,29 @@ describe('workos-management', () => { expect(rowFor('Environment').value).toBe('the API key supplied to this run'); }); + it('does not name a stored environment whose key did not do the writes', async () => { + // `--api-key sk_live_prod...` bypasses the store: the writes landed in the + // supplied key's environment, not the stored active one. + getActiveEnvironment.mockReturnValue(unclaimedEnv); + + await autoConfigureWorkOSEnvironment('sk_live_prod_999', INTEGRATION, PORT); + + const value = rowFor('Environment').value; + expect(value).toBe('the API key supplied to this run'); + expect(value).not.toContain('unclaimed-2'); + expect(value).not.toContain('env claim'); + }); + + it('does not name a claimed stored environment whose key did not do the writes', async () => { + getActiveEnvironment.mockReturnValue(claimedEnv); + + await autoConfigureWorkOSEnvironment('sk_live_prod_999', INTEGRATION, PORT); + + const value = rowFor('Environment').value; + expect(value).toBe('the API key supplied to this run'); + expect(value).not.toContain('sandbox'); + }); + it('degrades to the supplied-key wording when the config store throws', async () => { getActiveEnvironment.mockImplementation(() => { throw new Error('keyring locked'); diff --git a/src/lib/workos-management.ts b/src/lib/workos-management.ts index 15eb3a63..0535bf2d 100644 --- a/src/lib/workos-management.ts +++ b/src/lib/workos-management.ts @@ -138,8 +138,13 @@ async function setHomepageUrl(apiKey: string, url: string): Promise<{ success: b * can reach (`workosRequest` is GET/POST/PUT against user_management only, and * the config store holds no environment id). `activeEnvironment.name` is a * local label, so it is only ever a parenthetical, never an authoritative id. + * + * The stored active environment is only named when its key is the one that + * actually did the writes — `--api-key` (or `WORKOS_API_KEY`) bypasses the + * store entirely, and naming an untouched environment is exactly the confusion + * this row exists to prevent. */ -function describeCredentialProvenance(): string { +function describeCredentialProvenance(apiKey: string): string { let activeEnv: EnvironmentConfig | null = null; try { activeEnv = getActiveEnvironment(); @@ -148,7 +153,7 @@ function describeCredentialProvenance(): string { return SUPPLIED_KEY_PROVENANCE; } - if (!activeEnv) return SUPPLIED_KEY_PROVENANCE; + if (!activeEnv || activeEnv.apiKey !== apiKey) return SUPPLIED_KEY_PROVENANCE; if (isUnclaimedEnvironment(activeEnv)) { return `a new unclaimed environment (${activeEnv.name}) — run \`${formatWorkOSCommand('env claim')}\` to keep it`; @@ -211,7 +216,7 @@ export async function autoConfigureWorkOSEnvironment( // comes first — it is the context for the three rows below it. ui.log.success('WorkOS dashboard configured'); ui.rows([ - { key: 'Environment', value: describeCredentialProvenance(), statusKind: 'muted' }, + { key: 'Environment', value: describeCredentialProvenance(apiKey), statusKind: 'muted' }, { key: 'Redirect URI', value: callbackUrl, diff --git a/src/utils/help-json.ts b/src/utils/help-json.ts index 5037c570..b057941a 100644 --- a/src/utils/help-json.ts +++ b/src/utils/help-json.ts @@ -1424,6 +1424,17 @@ const commands: CommandSchema[] = [ default: false, hidden: false, }, + // Distinct from --force-install above: this one overrides the + // "AuthKit is already installed" preflight guard, and the guard's own + // error tells agents to pass it, so it has to be discoverable here. + { + name: 'force', + type: 'boolean', + description: 'Continue even if AuthKit is already installed in this project', + required: false, + default: false, + hidden: false, + }, { name: 'dashboard', type: 'boolean', From 3ad47a68d1cb5818fe87244505dd0fe761b8b9d4 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Fri, 24 Jul 2026 21:55:55 -0500 Subject: [PATCH 3/3] fix: align the env writer's key matching with the no-clobber reader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings from Greptile on #205. `upsertEnvLines` extracted the key with `trimmed.slice(0, eq)`, so any assignment the no-clobber reader recognizes but that form did not — `WORKOS_API_KEY = x` (spaces around `=`) or `export WORKOS_API_KEY=x` — failed the `pending` lookup. The old line kept its stale value and the new one was appended below it, leaving the file holding the key twice with conflicting values. The reader and writer now recognize the same shapes, and an `export ` prefix is preserved rather than dropped, since removing it would stop the var being exported when the file is sourced. The refusal message in `tryProvisionUnclaimedEnv` named `resolveProjectEnvPath` — the file the CLI *would* write — instead of the file the key was actually found in. In a JS project whose key lives in `.env`, that pointed the user at a `.env.local` that may not exist. Matches the idiom already used at `resolve-install-credentials.ts:60`. `workosRequest` sent `Content-Type: application/json` on every request, including the bodyless GET added for the homepage-URL read. Verified load-bearing by reverting each fix and observing the new tests go red: the two key-shape cases, the message-path case, and the header case. The indentation and commented-out-assignment cases are regression guards for behavior the refactor had to preserve, and pass either way. --- src/lib/env-writer.spec.ts | 49 +++++++++++++++++++++++++ src/lib/env-writer.ts | 33 ++++++++++++----- src/lib/unclaimed-env-provision.spec.ts | 15 ++++++++ src/lib/unclaimed-env-provision.ts | 11 ++++-- src/lib/workos-management.spec.ts | 21 +++++++++++ src/lib/workos-management.ts | 4 +- 6 files changed, 118 insertions(+), 15 deletions(-) diff --git a/src/lib/env-writer.spec.ts b/src/lib/env-writer.spec.ts index 4412238a..1c8aef55 100644 --- a/src/lib/env-writer.spec.ts +++ b/src/lib/env-writer.spec.ts @@ -218,6 +218,55 @@ describe('writeEnvLocal', () => { expect(readFileSync(envPath, 'utf-8')).toBe('WORKOS_COOKIE_PASSWORD=a=b=c\nWORKOS_CLIENT_ID=client_123\n'); }); + // The writer has to recognize every line shape `readProjectEnvCredentials` + // treats as already-configured. Where it doesn't, the old line survives and + // the new value lands beneath it — the file then holds the key twice with + // conflicting values, which is how a stale key outlives its own update. + it('updates a key written with spaces around the equals sign, without duplicating it', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, 'WORKOS_CLIENT_ID = client_old\nWORKOS_COOKIE_PASSWORD=pw\n'); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_new' }); + + expect(readFileSync(envPath, 'utf-8')).toBe('WORKOS_CLIENT_ID=client_new\nWORKOS_COOKIE_PASSWORD=pw\n'); + }); + + it('updates an `export `-prefixed key in place, keeping the prefix', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync( + envPath, + '# top\nexport WORKOS_CLIENT_ID=client_old\nWORKOS_COOKIE_PASSWORD=pw\nDATABASE_URL=postgres://localhost/dev\n', + ); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_new' }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + '# top\nexport WORKOS_CLIENT_ID=client_new\nWORKOS_COOKIE_PASSWORD=pw\nDATABASE_URL=postgres://localhost/dev\n', + ); + }); + + it('rewrites an indented key exactly once', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, ' WORKOS_CLIENT_ID=client_old\nWORKOS_COOKIE_PASSWORD=pw\n'); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_new' }); + + const content = readFileSync(envPath, 'utf-8'); + expect(content.match(/^\s*WORKOS_CLIENT_ID=/gm)).toHaveLength(1); + expect(content).toBe('WORKOS_CLIENT_ID=client_new\nWORKOS_COOKIE_PASSWORD=pw\n'); + }); + + it('leaves a commented-out assignment commented, appending the real one', () => { + const envPath = join(testDir, '.env.local'); + writeFileSync(envPath, '# WORKOS_CLIENT_ID=do_not_touch\nWORKOS_COOKIE_PASSWORD=pw\n'); + + writeEnvLocal(testDir, { WORKOS_CLIENT_ID: 'client_new' }); + + expect(readFileSync(envPath, 'utf-8')).toBe( + '# WORKOS_CLIENT_ID=do_not_touch\nWORKOS_COOKIE_PASSWORD=pw\nWORKOS_CLIENT_ID=client_new\n', + ); + }); + it('adds exactly one trailing newline to a file that lacked one', () => { const envPath = join(testDir, '.env.local'); writeFileSync(envPath, '# top\nWORKOS_COOKIE_PASSWORD=pw'); diff --git a/src/lib/env-writer.ts b/src/lib/env-writer.ts index ad081766..a7c67a38 100644 --- a/src/lib/env-writer.ts +++ b/src/lib/env-writer.ts @@ -49,6 +49,10 @@ function generateCookiePassword(): string { return Array.from(array, (byte) => byte.toString(16).padStart(2, '0')).join(''); } +// Captures the optional `export ` prefix (group 1) and the variable name (group 2) +// of an assignment line. Deliberately mirrors the patterns in `lib/project-env.ts`. +const ENV_ASSIGNMENT = /^[ \t]*(export[ \t]+)?([A-Za-z_][A-Za-z0-9_]*)[ \t]*=/; + /** * Set `vars` in `content` without disturbing comments, blank lines, or key order. * Existing keys are rewritten in place; new keys are appended. @@ -57,10 +61,17 @@ function generateCookiePassword(): string { * the original line was indented — indented env lines are vanishingly rare and * preserving the indent adds branching for no real benefit. * - * Key extraction splits on the first `=`, matching `parseEnvFile`, so values - * containing `=` survive. Duplicate keys in a malformed source file: only the - * first occurrence is rewritten (`parseEnvFile` reads last-wins, so a duplicate - * still shadows the update — a pre-existing pathology, not handled here). + * Key extraction recognizes exactly the line shapes `readProjectEnvCredentials` + * (`lib/project-env.ts`) recognizes — optional indentation, an optional `export ` + * prefix, and whitespace around `=`. The two must agree: a line the reader counts + * as configured but the writer fails to match would be left holding its old value + * while the new one is appended below it, so the file would carry the key twice + * with conflicting values. Only the text up to the first `=` is matched, so + * values containing `=` are irrelevant here. + * + * Duplicate keys in a malformed source file: only the first occurrence is + * rewritten (`parseEnvFile` reads last-wins, so a duplicate still shadows the + * update — a pre-existing pathology, not handled here). * * The file's dominant line terminator is detected once and reused, so a CRLF * file is not silently rewritten to LF (which would turn a two-line change into @@ -80,15 +91,17 @@ function upsertEnvLines(content: string, vars: Record): string { body === '' ? [] : body.split(/\r?\n/).map((line) => { - const trimmed = line.trim(); - if (!trimmed || trimmed.startsWith('#')) return line; - const eq = trimmed.indexOf('='); - if (eq === -1) return line; - const key = trimmed.slice(0, eq); + // Blank lines and `#` comments cannot match: only spaces and tabs may + // precede the name, so no separate guard for them is needed. + const match = ENV_ASSIGNMENT.exec(line); + if (!match) return line; + const [, exportPrefix, key] = match; if (!pending.has(key)) return line; const value = pending.get(key)!; pending.delete(key); - return `${key}=${value}`; + // Keep `export ` — dropping it would stop the var being exported when + // the file is sourced, which is why the author wrote it. + return `${exportPrefix ?? ''}${key}=${value}`; }); for (const [key, value] of pending) { diff --git a/src/lib/unclaimed-env-provision.spec.ts b/src/lib/unclaimed-env-provision.spec.ts index 3b8717d0..044cd5e1 100644 --- a/src/lib/unclaimed-env-provision.spec.ts +++ b/src/lib/unclaimed-env-provision.spec.ts @@ -276,6 +276,21 @@ describe('unclaimed-env-provision', () => { expect(readFileSync(join(testDir, '.env'), 'utf-8')).toBe('WORKOS_API_KEY=sk_existing\n'); }); + // The write target and the file holding the key are not the same thing: a + // JS project writes `.env.local`, but the key may already sit in `.env`. + // Naming the write target would send the user to edit the wrong file. + it('names the file the key was found in, not the file it would have written', async () => { + writeFileSync(join(testDir, 'package.json'), '{}'); + writeFileSync(join(testDir, '.env'), 'WORKOS_API_KEY=sk_existing\n'); + + const result = await tryProvisionUnclaimedEnv({ installDir: testDir }); + + expect(result).toBe(false); + expect(mockUi.log.warn).toHaveBeenCalledWith( + `${join(testDir, '.env')} already has WORKOS_API_KEY — not provisioning a new environment.`, + ); + }); + it('still provisions when the env file exists without a WorkOS key', async () => { writeFileSync(join(testDir, 'package.json'), '{}'); writeFileSync(join(testDir, '.env.local'), 'DATABASE_URL=postgres://localhost/dev\n'); diff --git a/src/lib/unclaimed-env-provision.ts b/src/lib/unclaimed-env-provision.ts index f3d828a9..15a883a0 100644 --- a/src/lib/unclaimed-env-provision.ts +++ b/src/lib/unclaimed-env-provision.ts @@ -42,11 +42,14 @@ export async function tryProvisionUnclaimedEnv(options: UnclaimedEnvProvisionOpt // only (never on AuthKit detection) so `install --force` still works. // Checked before provisioning so no environment is created then abandoned. const { readProjectEnvCredentials, resolveProjectEnvPath } = await import('./project-env.js'); - if (readProjectEnvCredentials(options.installDir).apiKey) { + const projectEnv = readProjectEnvCredentials(options.installDir); + if (projectEnv.apiKey) { logInfo('[unclaimed-env-provision] Refusing to provision: project env already has WORKOS_API_KEY'); - ui.log.warn( - `${resolveProjectEnvPath(options.installDir)} already has WORKOS_API_KEY — not provisioning a new environment.`, - ); + // Name the file the key was actually found in, not the file we would have + // written: the key can live in any of `ENV_FILE_NAMES`, and pointing at the + // write target sends people to edit a file that may not even exist. + const envPath = projectEnv.apiKeyPath ?? resolveProjectEnvPath(options.installDir); + ui.log.warn(`${envPath} already has WORKOS_API_KEY — not provisioning a new environment.`); return false; } diff --git a/src/lib/workos-management.spec.ts b/src/lib/workos-management.spec.ts index 691b17ba..a328ed38 100644 --- a/src/lib/workos-management.spec.ts +++ b/src/lib/workos-management.spec.ts @@ -196,6 +196,27 @@ describe('workos-management', () => { expect(bodies).toEqual([undefined]); }); + it('declares Content-Type only on the requests that carry a body', async () => { + const seen: Array<{ method: string; contentType?: string }> = []; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string, init: { method: string; headers: Record }) => { + if (url === HOMEPAGE_ENDPOINT) { + seen.push({ method: init.method, contentType: init.headers['Content-Type'] }); + } + // Differing URL forces the PUT, so both methods are observed. + return jsonResponse(200, { url: 'https://app.example.com' }); + }), + ); + + await autoConfigureWorkOSEnvironment(API_KEY, INTEGRATION, PORT); + + expect(seen).toEqual([ + { method: 'GET', contentType: undefined }, + { method: 'PUT', contentType: 'application/json' }, + ]); + }); + it('warns instead of aborting when the PUT fails', async () => { stubFetch((method) => method === 'GET' ? jsonResponse(404, {}) : jsonResponse(403, { message: 'API key lacks permission' }), diff --git a/src/lib/workos-management.ts b/src/lib/workos-management.ts index 0535bf2d..8d6ae1f1 100644 --- a/src/lib/workos-management.ts +++ b/src/lib/workos-management.ts @@ -36,7 +36,9 @@ async function workosRequest( method, headers: { Authorization: `Bearer ${apiKey}`, - 'Content-Type': 'application/json', + // Only declare a body's type when there is a body. A bodyless GET that + // claims `application/json` is malformed, and strict proxies may reject it. + ...(body ? { 'Content-Type': 'application/json' } : {}), }, ...(body ? { body: JSON.stringify(body) } : {}), });