Skip to content

feat!: stop the installer from clobbering configured AuthKit projects - #205

Merged
nicknisi merged 3 commits into
mainfrom
nicknisi/dax-feedback
Aug 7, 2026
Merged

feat!: stop the installer from clobbering configured AuthKit projects#205
nicknisi merged 3 commits into
mainfrom
nicknisi/dax-feedback

Conversation

@nicknisi

Copy link
Copy Markdown
Member

Summary

A customer ran npx workos@latest against a Next.js app that already had AuthKit fully wired up (@workos-inc/authkit-nextjs, custom middleware, SCIM/SSO reconciliation). The CLI provisioned a brand new unclaimed WorkOS environment, rewrote his .env.local with those credentials, then died four minutes later on an opaque HTTP 500 from the LLM gateway and left everything it had written in place.

He asked for "a check that detects an existing AuthKit install and stops before touching dashboard config." There wasn't one, and two of us thought there was. The check cited internally (run-with-core.ts:252) is the checkAuthentication actor: it reads the CLI's own keyring config store and never looks at the project. detectSingleIntegration only answers which framework is present, and no @workos-inc/* package appears in it. The only AuthKit-aware detector we own (doctor/checks/sdk.ts) is reachable during install only after the agent has already run.

The root cause is ordering, not a missing feature. resolveInstallCredentials runs in the bin.ts handlers before the installer state machine exists, provisions an unclaimed environment, and writes credentials into the project's env file. readExistingCredentials then reads that same file and finds the key the CLI just wrote. Two individually correct functions in the wrong order, where the guard silently reads back its own write. The terminal error state has no rollback.

What changed

Area Change
Preflight guard New src/lib/preflight-authkit.ts, wired ahead of credential resolution in all three install entry points (install, dashboard, $0). Prompts on an interactive TTY, exits non-zero in agent/CI/JSON mode, --force overrides.
Ordering New src/lib/project-env.ts reads whichever env file the CLI would write (.env.local when package.json exists, else .env) before provisioning. tryProvisionUnclaimedEnv refuses again at the write site, so a direct caller can't bypass it.
Env safety writeEnvLocal and the .env branch of writeCredentialsEnv now do line-preserving upserts, so comments, blank lines, and key order survive. One git-ignored backup of the pre-CLI file, written once and never overwritten.
Dashboard truthfulness setHomepageUrl reads before writing and reports already set instead of an unconditional updated. Output now states credential provenance, so an unclaimed throwaway can't be mistaken for production.
Error surfacing One shared failure-classifier.ts replaces three duplicated 5xx regexes (agent-interface.ts, cli-adapter.ts, headless-adapter.ts). Deterministic gateway failures no longer render as "temporarily unavailable, try again in a few minutes."
Proxy timeout 120s → 600s in both startCredentialProxy and startClaimTokenProxy. The reported path uses the claim-token proxy, so fixing one was fixing neither.

Two details worth a reviewer's eye:

  • The backup had to be gitignored before it is written. ensureGitignore's existing covering patterns (.env.local, .env*.local, .env*) never match a .bak; a Next.js project ships .env*.local so the old guard no-ops; and stageAndCommit runs git add -A with commit defaulting to true. An unignored backup would have committed a live WORKOS_API_KEY and WORKOS_CLAIM_TOKEN.
  • installer-core.ts re-emits the already-rendered message onto the error event, so the adapters classify our own copy rather than the raw SDK text. Today's transient path only round-trips by accident ("temporarily unavailable" happens to match service.*unavailable). Hence the named DETERMINISTIC_HEADLINE constant and its round-trip test: without it the new verdict would be computed and then silently discarded one hop later.

run-with-core.ts also now reads .env for non-JS projects, where the deleted private helper only ever read .env.local. That is the "close the read-back loop" half of the fix, but it is a behavior change beyond a pure refactor: a Django/Rails project with WORKOS_API_KEY in .env will now have it picked up as existing credentials.

Test plan

Unit tests only, by design, so the tests carry the whole guarantee. Several were mutation-checked (the fix was temporarily disabled to confirm the new assertions actually go red).

pnpm typecheck                 # clean
pnpm test                      # 136 files, 2084 tests pass
pnpm build                     # clean
pnpm lint && pnpm format:check # clean

# The load-bearing structural checks
grep -c 'assertNoExistingAuthKit' src/bin.ts   # 3 (one per entry point)
grep -c 'forceOption' src/bin.ts               # 3 (definition + both option sets)
grep -c '120_000' src/lib/credential-proxy.ts  # 0
grep -c '50\[0-9\]' src/lib/agent-interface.ts src/lib/adapters/*.ts  # 0 each

The three grep checks are deliberate. bin.ts runs runCli() at import and has no test seams, so the ordering invariant that this whole PR turns on cannot be asserted by a unit test. Guard placement relative to resolveInstallCredentials is worth checking by eye in review.

End-to-end sanity, run by hand:

  • WORKOS_MODE=agent workos install --install-dir <fixture with @workos-inc/authkit-nextjs> → exit 1, {"error":{"code":"authkit_already_installed",...}}, and the fixture directory is untouched (no .env, no .env.local, no .gitignore written).
  • Positive control: a fixture with @workos-inc/node only sails past the guard, confirming no false positive on the base SDK.
  • workos --force --help parses on the .strict() $0 parser.

Not in this PR

  • The server-side half. LlmGatewayService.createMessage calls messages.create non-streaming, and the pinned @anthropic-ai/sdk@0.40.0 throws a status-less AnthropicError for any max_tokens > 21,333, which toApiErrorResponse turns into an opaque 500. That is a separate PR against workos/workos (packages/api), independently mergeable. Its deploy should land with or after this release: once the gateway aggregates a stream internally, a non-streaming client receives no bytes until the turn completes, which the old 120s timeout would have killed.
  • --repair for projects the guard now turns away. The guard points at workos doctor.
  • The duplicated .env writers in the python and go integrations still destroy comments.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@greptile-apps

greptile-apps Bot commented Jul 24, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a credential-clobbering data-loss bug: workos install was provisioning a fresh WorkOS environment and writing its credentials into the project's env file before any AuthKit detection ran, so a guard placed after it was no guard at all.

  • Preflight guard (preflight-authkit.ts): assertNoExistingAuthKit runs before resolveInstallCredentials in all three entry points (install, dashboard, $0), hard-exits in non-interactive/JSON mode, prompts interactively, and obeys --force.
  • Ordering + dual guard: readProjectEnvCredentials (new project-env.ts) scans all four env files in framework-precedence order; resolveInstallCredentials refuses provisioning when a key is already present; tryProvisionUnclaimedEnv enforces the same check at the write site so direct callers can't bypass it.
  • Env safety: upsertEnvLines replaces destructive rewrites with line-preserving upserts; backupEnvFile writes a gitignored .bak before the first mutation; writeSecretFile creates new files as 0600.
  • Failure classification: failure-classifier.ts consolidates three duplicated 5xx regexes, correctly distinguishes deterministic gateway failures (which should not be retried) from transient outages, and the DETERMINISTIC_HEADLINE constant enables round-trip re-classification through installer-core's re-emit.
  • Proxy timeout: increased from 120 s to 600 s in both proxy start functions via a shared constant, fixing the reported four-minute silent kill.

Confidence Score: 5/5

  • Safe to merge. The changes are well-scoped, deeply tested (136 files, 2084 tests), and the structural ordering invariants are explicitly verified by the test plan's grep checks.
  • The core fix — moving assertNoExistingAuthKit before resolveInstallCredentials in all three entry points — is verified to be in place. The dual-enforcement pattern (preflight check + write-site check in tryProvisionUnclaimedEnv) is a strong defense-in-depth approach. The failure-classifier consolidation removes real duplicated logic that was producing wrong user advice. All comments are non-blocking style observations.
  • No files require special attention. The PR description calls out the three grep invariants (assertNoExistingAuthKit count, forceOption count, absence of 120_000) as the structural checks worth verifying by eye in review.

Important Files Changed

Filename Overview
src/bin.ts Three install entry points (install, dashboard, $0) now call assertNoExistingAuthKit before resolveInstallCredentials; --force registered on all three parsers including the .strict() $0 handler; ordering invariant matches the PR's core fix.
src/lib/preflight-authkit.ts New preflight guard: detects AUTHKIT_PACKAGES in package.json, hard-exits non-interactively (including --json-on-TTY), prompts interactively, --force short-circuits before any detection. Well-tested and correctly handles the JSON mode edge case.
src/lib/project-env.ts New reader module: readProjectEnvCredentials scans all four env file names in precedence order; resolveProjectEnvPath mirrors writeCredentialsEnv's JS/non-JS branch. Replaces the private .env.local-only helper that caused the read-back loop bug.
src/lib/env-writer.ts upsertEnvLines replaces destructive rewrite with line-preserving upsert; backupEnvFile writes a gitignored .bak before the first mutation; writeSecretFile creates files 0600. The key-extraction regex strips leading indentation on rewrite (documented). The trailing-spaces-before-= key matching issue (noted in previous thread) still applies.
src/lib/failure-classifier.ts New consolidated classifier replaces three duplicated 5xx regexes; ordering (rate_limited → deterministic → service_outage) is intentional and well-documented; DETERMINISTIC_HEADLINE constant enables correct round-trip classification. Clean separation of classification from rendering.
src/lib/resolve-install-credentials.ts Now reads project env files before attempting provisioning; falls back to authenticate() instead of provisioning when a key is already present; logs the key-found message so users understand why the flow changed.
src/lib/unclaimed-env-provision.ts Added a second no-clobber check at the write site using readProjectEnvCredentials, so a direct caller that bypasses resolveInstallCredentials cannot overwrite existing credentials. Correctly names the file the key was found in, not the write target.
src/lib/workos-management.ts setHomepageUrl now reads before writing, reporting already set vs updated honestly; describeCredentialProvenance names the active environment only when its key matches the writes; GET requests no longer send Content-Type (previous thread). AutoConfigResult.homepageUrl gains alreadyExists.
src/lib/credential-proxy.ts Timeout increased from 120s to 600s in both startCredentialProxy and startClaimTokenProxy via a shared constant. The 10-minute ceiling matches the Anthropic SDK's own non-streaming timeout.
src/lib/agent-interface.ts handleSDKMessage now uses the shared classifyAgentFailure instead of duplicated regexes; adds DETERMINISTIC_PREFIX for failures that won't benefit from retry; runAgent returns EXECUTION_ERROR (not SERVICE_UNAVAILABLE) for deterministic failures.
src/lib/run-with-core.ts Replaced the private readExistingCredentials helper (which only read .env.local) with readProjectEnvCredentials (which reads all four env file names). This is the read-back-loop close described in the PR.
src/lib/installer-core.ts emitError fallback message changed from 'An unexpected error occurred' (which matched the gateway's deterministic error signature) to 'The installer failed for an unknown reason', preventing false-positive deterministic classification.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[workos install / dashboard / $0] --> B[assertNoExistingAuthKit]
    B -->|AuthKit package found, non-interactive| C[exitWithError\nauthkit_already_installed]
    B -->|AuthKit package found, interactive| D{User confirm?}
    D -->|No| E[exit 2 CANCELLED]
    D -->|Yes or --force| F[resolveInstallCredentials]
    B -->|No AuthKit found| F

    F --> G{CLI has stored apiKey?}
    G -->|Yes| K[proceed]
    G -->|No| H{Project env file\nhas WORKOS_API_KEY?}
    H -->|Yes| I[log: key kept\nauthenticate if not skipAuth]
    I --> K
    H -->|No| J[tryProvisionUnclaimedEnv]
    J --> JJ{Project env file\nhas WORKOS_API_KEY?}
    JJ -->|Yes| JK[warn + return false]
    JJ -->|No| L[provisionUnclaimedEnvironment]
    L --> M[writeCredentialsEnv\nupsertEnvLines + backup]
    M --> K
    K --> N[runWithCore / handleInstall]
    N --> O[autoConfigureWorkOSEnvironment]
    O --> P[GET homepage URL]
    P -->|matches| Q[skip PUT, report already set]
    P -->|differs or error| R[PUT new URL]
    O --> S[createRedirectUri + createCorsOrigin in parallel]
    N --> T[LLM installer agent via credential proxy\n600s idle timeout]
    T -->|error| U[classifyAgentFailure]
    U -->|deterministic| V[DETERMINISTIC_PREFIX\ndo not retry]
    U -->|service_outage| W[SERVICE_UNAVAILABLE_PREFIX\nretry OK]
    U -->|rate_limited| X[RATE_LIMITED_PREFIX\nwait then retry]
Loading

Reviews (4): Last reviewed commit: "fix: align the env writer's key matching..." | Re-trigger Greptile

Comment thread src/lib/env-writer.ts Outdated
Comment thread src/lib/workos-management.ts
nicknisi added a commit that referenced this pull request Jul 25, 2026
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.
`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.
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.
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.
@nicknisi
nicknisi force-pushed the nicknisi/dax-feedback branch from 3ad47a6 to ff747eb Compare August 7, 2026 18:54
@nicknisi
nicknisi merged commit 04896f6 into main Aug 7, 2026
5 checks passed
@nicknisi
nicknisi deleted the nicknisi/dax-feedback branch August 7, 2026 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant