fix(installer): unbreak pnpm local installs + stop self-fulfilling editor detection#501
fix(installer): unbreak pnpm local installs + stop self-fulfilling editor detection#501filip131311 wants to merge 2 commits into
Conversation
…itor detection Reported: on pnpm setups `npx @swmansion/argent init --local` left package.json without the dep, reruns died with 'Local install failed.', and init kept 'detecting' Cursor/Codex on a machine that never had them. Root causes (each reproduced, backed by installer telemetry): - a stale global argent (0.14, pre --local) is what bare npx actually runs; it silently ignored the unknown flag and did a global init - detectProjectPackageManager missed pnpm's modern markers: pnpm 10/11 'pnpm init' writes only devEngines.packageManager and no lockfile exists before the first install, so detection fell back to npm and npm's own devEngines gate insta-rejected the install (EBADDEVENGINES, the 221-537ms INSTALL_LOCAL_PACKAGE_FAILED cluster) - a missing pm binary surfaced as raw 'spawn pnpm ENOENT' plus advice that fails identically in the user's shell - Cursor/Codex detect() keyed on bare ~/.cursor / ~/.codex dirs, which argent itself creates — self-fulfilling detection Fixes: - strict init flag parsing: unknown/malformed flags abort (exit 2, INSTALL_UNKNOWN_FLAG) and point at npx @swmansion/argent@latest; --from=path accepted, dangling --from aborts, first --from wins - pm detection honors devEngines.packageManager (object/array) and pnpm-workspace.yaml — the latter below any real lockfile so a stray file can't outrank yarn.lock after a migration - missing-binary failures name the real problem (with corepack hint; Windows detected via cmd.exe 'is not recognized' since shell:true never ENOENTs); other pm failures retry once (transient registry / pnpm first-run state errors) - editor detection requires evidence beyond argent's own artifacts (mcp.json argent entry, permissions.json allowlist, managed developer_instructions, bundled rules/agents/skills); corrupt, unparseable, or empty content conservatively counts as user evidence; the stale-config sweep scans ALL adapters so dead entries in no-longer-detected dirs still get pruned - README: recommend @latest (defeats the stale-global hijack) and a pnpm dlx alternative for fresh pnpm-init projects
Aborting with exit 2 on unrecognized flags could break existing scripts/setups that pass flags an installed version doesn't know. Restore the original silent-ignore semantics; keep the --from=path spelling and first---from-wins parsing.
| detect(): boolean { | ||
| return ( | ||
| dirExists(path.join(homedir(), ".cursor")) || dirExists(path.join(process.cwd(), ".cursor")) | ||
| dirHasEditorEvidence(path.join(homedir(), ".cursor"), cursorDirLooksArgentOnly) || | ||
| dirHasEditorEvidence(path.join(process.cwd(), ".cursor"), cursorDirLooksArgentOnly) | ||
| ); |
There was a problem hiding this comment.
Once a .cursor holding only argent's own files stops counting as evidence, a project that was previously init'd for Cursor alone detects nothing — and chooseAdapters then falls back to detected.length > 0 ? detected : eligible (init-adapters.ts:38), i.e. every eligible editor. That fallback is unchanged here, but this is what now routes into it.
Concretely: a teammate clones a repo where argent init --local was run for Cursor only, so .cursor/mcp.json + .cursor/rules/argent.md are committed — exactly what the local-mode "Team Setup" note tells them to commit — and their machine has no editors installed. Running argent init --local -y there now reports
● Editors: Cursor, Claude Code, VS Code, Zed, Gemini, Codex, opencode, Kiro
and drops ~40 files into their repo: .claude/, .codex/, .gemini/, .kiro/, .mcp.json, .vscode/, .zed/, .opencode/, opencode.json, .agents/skills/ (14 skills) and skills-lock.json — for seven editors they never asked for, all of it showing up in git status. Before this change dirExists('.cursor') was true, detected was [Cursor], and only .cursor/mcp.json was rewritten.
Reproduced against the built CLI with an isolated HOME and a PATH carrying no editors.
| // ── Installed-editor evidence ───────────────────────────────────────────────── | ||
| // A bare `.cursor` / `.codex` directory is NOT proof the editor is installed: | ||
| // argent itself creates those directories when it writes an MCP config, an | ||
| // allowlist, rules, agents, or skills there. Counting them as detection made | ||
| // every later `init` "detect" editors the user never installed | ||
| // (self-fulfilling detection). A directory is evidence only when it holds | ||
| // something argent doesn't write, or an argent-writable file carries | ||
| // non-argent content. Anything empty, unreadable, or unparseable counts as |
There was a problem hiding this comment.
The loop described here is closed for Cursor and Codex only. Claude Code (.mcp.json / .claude), VS Code (.vscode), Gemini (.gemini), Kiro (.kiro), Windsurf (~/.codeium/windsurf), Hermes (~/.hermes) and Zed (~/.config/zed) all still detect on a bare dirExists of the exact directory argent itself creates when it writes their config, allowlist or rules. (opencode is the one client that never had the problem — it probes for the binary instead of a directory.)
On a machine with no editors installed at all, one argent init -y — which, with nothing detected, configures every eligible editor — creates those directories. Re-running detection against the same pristine HOME and project immediately afterwards:
not detected Cursor <- fixed here
not detected Codex <- fixed here
DETECTED Claude Code
DETECTED VS Code
DETECTED Windsurf
DETECTED Gemini
DETECTED Hermes
DETECTED Kiro
So the reported symptom — init "persistently detects" editors on a machine that never had them — still reproduces verbatim for six clients. And because the stricter Cursor/Codex evidence check makes detected come back empty more often, the "nothing detected → configure everything" fallback that creates these directories now fires more often, not less.
| if (!landed && installError && !missingBinary) { | ||
| // The package manager ran and failed. Retry once before giving up: first | ||
| // attempts fail on transient registry/network errors (argent is a large | ||
| // download) and on pnpm's own first-run state mutations (e.g. it may write | ||
| // build-script policy stubs and exit non-zero), where an identical rerun | ||
| // succeeds. | ||
| spinner.message(`${pm} failed — retrying once...`); | ||
| ({ landed, exitError: installError } = await attempt()); | ||
| } |
There was a problem hiding this comment.
This retry treats a cancellation signal as a transient failure, so an interrupted install silently runs a second time.
Argent registers no SIGINT/SIGTERM handler of its own, and p.spinner() (clack 1.6.0) registers one that merely prints "Canceled" and never exits — while registering it also suppresses Node's default terminate-on-signal. So when the install is signalled, the package manager dies, runShellCommand rejects with Command exited with code null, missingBinary is false, and this block spawns a second full install with piped (invisible) stdio while the UI already reads "Canceled". Before this change the same signal produced "Local install failed." and process.exit(1).
Reproduced on the built cli.js init --local -y with a counting pnpm shim on PATH and one kill -INT -<pgid> — two add -D @swmansion/argent invocations from a single signal, with the argent process still alive afterwards.
To be precise about the trigger, since I initially got this wrong: a human Ctrl-C at an interactive terminal does not take this path. @clack/core's block() puts a tty stdin into raw mode, which clears ISIG, so Ctrl-C arrives as a 0x03 keypress and clack's own handler exits — I confirmed a pty Ctrl-C leaves exactly one install. The path that does fire is any signal-delivered SIGINT/SIGTERM: stdin not an interactive tty (CI, piped, a devcontainer/provisioning script, an agent harness), or kill / a supervisor / a timeout wrapper. Those are also the contexts where -y is most likely used, and where nobody is watching the second install run.
| const isMissingBinaryError = (err: Error | null): boolean => | ||
| err !== null && | ||
| ((err as NodeJS.ErrnoException).code === "ENOENT" || | ||
| /is not recognized as an internal or external command/i.test(err.message)); |
There was a problem hiding this comment.
On Windows this regex is the only missing-binary signal — with shell: true only cmd.exe is ever spawned, so the ENOENT branch above cannot fire there — and cmd.exe's "is not recognized as an internal or external command" text is localized. On a German, Polish or Japanese Windows the match fails, so missingBinary stays false, the retry below burns a second identical launch, and the user ends up on the generic Install manually with: cd <root> && pnpm add -D @swmansion/argent — precisely the advice the comment above says "fails the same way in the user's shell". The locale-independent signal the comment cites (exit code 9009) never reaches the predicate: runShellCommand discards the exit code and rejects with new Error(stderr) whenever stderr is non-empty.
The match is also applied to the install's entire captured stderr rather than to a launch failure specifically, so an install that prints that phrase for any other reason and then fails to land would be reported as the package manager "not found on PATH".
On POSIX the path is sound — I confirmed the spawn ENOENT survives runTrustingDisk's re-wrap with .code intact.
| const attempt = (): Promise<{ landed: boolean; exitError: Error | null }> => | ||
| runTrustingDisk( | ||
| () => runShellCommand(cmd, { cwd: projectRoot }), | ||
| () => isLocallyInstalled(projectRoot) || isYarnPnp(projectRoot) | ||
| ); | ||
| let { landed, exitError: installError } = await attempt(); |
There was a problem hiding this comment.
None of the new failure handling in this file is covered by a test. Searching every package's test directory for install-runner, installLocally, isMissingBinaryError, retrying once or not found on PATH returns nothing — so the retry-once semantics, the "don't retry a missing binary" rule, the ENOENT predicate and the Windows branch are all unexercised. The PR's new suites cover init-args, mcp-configs and utils only.
| const devEnginesPm = pkg.devEngines?.packageManager; | ||
| for (const entry of Array.isArray(devEnginesPm) ? devEnginesPm : [devEnginesPm]) { | ||
| const name = asKnownPm(entry?.name); | ||
| if (name) return name; | ||
| } |
There was a problem hiding this comment.
An array devEngines.packageManager is an OR-set, not an ordered preference, so the first entry is not the project's package manager. npm's own validator only errors when every entry fails (npm-install-checks/lib/dev-engines.js: const invalid = depErrors.length === dependencies.length), and the spec reads "the first acceptable option would be used" — acceptable meaning "validates against the manager currently running". So [{name: "npm"}, {name: "pnpm"}] says "npm or pnpm is fine here" and carries no information about which one the project actually uses. The lockfile does, and taking [0] discards it.
Running the built detectProjectPackageManager over fixtures:
npm <- devEngines [{npm},{pnpm}] + pnpm-lock.yaml
pnpm <- devEngines [{pnpm},{npm}] + pnpm-lock.yaml
Merely reordering the array flips the answer, which is the point. In the first layout — a pnpm project that also lists npm as acceptable, which is the common workaround for npm ≥10.9 rejecting npx in a pnpm-declaring project with EBADDEVENGINES, i.e. precisely the users this PR targets — argent init --local runs npm install --save-dev @swmansion/argent in a pnpm repo, writing a package-lock.json and leaving pnpm-lock.yaml stale. The project's own CI (pnpm install --frozen-lockfile) then fails with ERR_PNPM_OUTDATED_LOCKFILE.
This detector also drives uninstall.ts (localUninstallCommand) and update.ts, both of which mutate the manifest and lockfile, so a misdetection is not confined to a failed install.
| // corepack's `packageManager` field ("[email protected]") is the project's own | ||
| // declaration — the strongest signal there is. | ||
| // declaration — the strongest signal there is. `devEngines.packageManager` is | ||
| // its modern equivalent and MUST be honored too: `pnpm init` (pnpm 10+) writes | ||
| // ONLY devEngines, and such a fresh project has no lockfile yet either — so | ||
| // without this, detection fell through to npm, whose own devEngines gate then | ||
| // instantly rejected the install (EBADDEVENGINES) as "Local install failed." |
There was a problem hiding this comment.
Putting devEngines in this function ranks it above every real lockfile (detectProjectPackageManager is pmFromPackageManagerField(dir) ?? pmFromLockfile(dir)), which is the opposite of the reasoning applied to pnpm-workspace.yaml a few lines below — that one is deliberately ranked under the lockfiles because "a stray copy can survive a migration to yarn/bun, and the lockfile is what actually records the project's dependency state", with a test pinning it.
devEngines is strictly more stale-prone than that, because yarn and bun neither read nor update it: bun install in a project declaring devEngines.packageManager: {name: "pnpm"} exits 0 and leaves the field untouched. So a project scaffolded by pnpm init (pnpm 11 writes devEngines and no packageManager) and later moved to bun or yarn keeps the declaration forever, next to the authoritative lockfile. Verified against the built detector:
pnpm <- devEngines {pnpm} + yarn.lock
pnpm <- devEngines {pnpm} + bun.lock
yarn <- pnpm-workspace.yaml + yarn.lock (ranked correctly)
In the bun case argent init --local runs pnpm add -D, creating a pnpm-lock.yaml and staling bun.lock so bun install --frozen-lockfile fails; where pnpm is the corepack shim (Node's default) it instead hard-fails with Invalid package manager specification in package.json (pnpm@^11.13.0); expected a semver version, and a bun-only machine has no pnpm at all. Note the motivating scenario itself never exercises this precedence: a fresh pnpm init has no lockfile present for devEngines to outrank.
.onFail is also never read, so even an explicitly advisory onFail: "ignore" declaration outranks the lockfile.
|
|
||
| ```bash | ||
| npx @swmansion/argent init | ||
| npx @swmansion/argent@latest init |
There was a problem hiding this comment.
npm's devEngines gate rejects the outer npx before argent ever runs, so this quick-start command is itself blocked in a freshly pnpm init-ed project — not only the --local form that the new note below is attached to.
Verified: npx pnpm@11 init writes a package.json with only devEngines.packageManager ({name: "pnpm", version: "^11.13.0", onFail: "download"}), no packageManager field and no lockfile. In that directory:
$ npx @swmansion/argent@latest init
npm error code EBADDEVENGINES
npm error EBADDEVENGINES Invalid name "pnpm" does not match "npm" for "packageManager"
This is the first command in the README, and the same un-noted form appears again under "From your project root" at line 101 — so a user in exactly the environment this PR is fixing hits the wall with the pnpm dlx alternative nowhere in view.
|
|
||
| ```bash | ||
| npx @swmansion/argent init | ||
| npx @swmansion/argent@latest init |
There was a problem hiding this comment.
The @latest pinning lands on the README's three commands, but the same command still ships un-pinned in packages/skills/rules/argent.md:35:
Argent isn't installed in this environment. … run
npx @swmansion/argent init -y(ornpm i -g @swmansion/argent && argent init -y).
That file is published in the tarball, copied into ~/.cursor/rules and ~/.claude/rules, and injected into Codex's developer_instructions — so it is the copy an assistant actually reads and runs. Given the stale-global hijack described in the PR body is triggered by exactly this un-pinned invocation, the highest-traffic instance of it is the one aimed at the agent.
| detect(): boolean { | ||
| return ( | ||
| dirExists(path.join(homedir(), CODEX_FILENAME)) || | ||
| dirExists(path.join(process.cwd(), CODEX_FILENAME)) | ||
| dirHasEditorEvidence(path.join(homedir(), CODEX_FILENAME), codexDirLooksArgentOnly) || | ||
| dirHasEditorEvidence(path.join(process.cwd(), CODEX_FILENAME), codexDirLooksArgentOnly) | ||
| ); | ||
| }, |
There was a problem hiding this comment.
A knock-on of the asymmetry between the fixed and unfixed adapters: a second init in the same project silently stops maintaining the two configs this PR actually fixed.
Because .claude, .vscode, .gemini and .kiro are argent-created but still self-detect, detected comes back non-empty on the second run, so chooseAdapters returns it instead of falling through to eligible — and the argent-only .cursor / .codex are excluded by the new evidence check. Same repo, same isolated HOME, identical command:
run 1: ● Editors: Cursor, Claude Code, VS Code, Zed, Gemini, Codex, opencode, Kiro
run 2: ● Editors: Claude Code, VS Code, Gemini, Kiro
.cursor/mcp.json and .codex/config.toml are not deleted, but they are never rewritten again (mtime unchanged across run 2). So on a version bump that changes the MCP command or args, re-running init refreshes four editors' configs and leaves Cursor's and Codex's stale — the clients that still self-detect keep getting maintained, while the two the evidence check correctly fixed get orphaned. (argent update still reaches them, since it keys off hasArgentEntry rather than detect(), so this is confined to init re-runs.)
| ); | ||
| p.log.info(`Install manually with: ${pc.cyan(`cd ${projectRoot} && ${cmdStr}`)}`); | ||
| } | ||
| await tel.trackPackageAction("fresh_install", startedAt, false, INSTALL_LOCAL_PACKAGE_FAILED); |
There was a problem hiding this comment.
The retry is not observable, so the telemetry this PR is argued from can no longer measure it.
trackPackageAction emits only { action, is_success, duration_ms, ...failureSignal }, with duration_ms = performance.now() - startedAt and startedAt captured once at line 104, before the first attempt. There is no retry/attempt field anywhere in init-telemetry.ts or packages/telemetry/src/events.ts.
Two consequences. A retried-and-succeeded install is reported as fresh_install / is_success: true, indistinguishable from a first-try success that happened to be slow — so the transient-failure rate the PR cites as its justification ("a ~19s failure shape where an identical manual retry succeeds") becomes invisible the moment the retry ships, and there is no way to tell from PostHog how often it fires, whether it helps, or whether it should be kept. And on the failure path here, duration_ms now spans both attempts, so the fast-fail duration signature the PR used to identify this bug in the first place ("the 221-537 ms INSTALL_LOCAL_PACKAGE_FAILED cluster") roughly doubles for any remaining fast-fail cause and stops being a usable fingerprint.
| it("detect() returns true when the local .codex dir holds user content", () => { | ||
| // A bare/argent-only .codex dir is NOT detection evidence (see the | ||
| // detection-evidence suite); real user content in it is. | ||
| const localCodex = path.join(process.cwd(), ".codex"); | ||
| const existed = fs.existsSync(localCodex); | ||
| const marker = path.join(localCodex, "history.jsonl"); | ||
| const markerExisted = fs.existsSync(marker); | ||
| if (!existed) fs.mkdirSync(localCodex, { recursive: true }); | ||
| if (!markerExisted) fs.writeFileSync(marker, ""); | ||
| try { |
There was a problem hiding this comment.
This test only exercises the branch it is named for by accident of a leaked global, so it does not actually pin the new behavior.
codexAdapter.detect() is evidence(homedir()/.codex) || evidence(cwd/.codex), and this test never sets homedirOverride. It currently works only because an earlier Gemini test assigns homedirOverride = path.join(tmpDir, "home") inside its it() body (around line 763) and nothing ever resets it — the "Codex adapter" describe has no beforeEach/afterEach for it. By the time this test runs, homedir() therefore points at a stale, already-removed tmpdir, the home branch is inert, and the history.jsonl marker in cwd is what gets asserted.
Remove that leak — reorder the file, run this test on its own, or add the afterEach the other describes have — and on any machine with a real ~/.codex the assertion is satisfied by the home branch before the marker is ever consulted. That is not hypothetical: on my machine ~/.codex holds .codex-global-state.json, which codexDirLooksArgentOnly classifies as user evidence (it is neither config.toml nor rules/agents/skills, so the .every() callback returns false), making detect() true regardless of the project dir. The test would keep passing even if the cwd/.codex evidence path were completely broken.
Every other detection-evidence suite added in this PR sets homedirOverride in a beforeEach and clears it in an afterEach; this one is the exception.
2 participants
Problem
Field report (pnpm setup):
npx @swmansion/argent init --local.mcp.json+skills-lock.jsonpackage.jsonLocal install failed.+ advice to runpnpm add -D ...manuallyRoot causes (each reproduced; confirmed against installer telemetry)
npx @swmansion/argentprefers an existing global install over the registry. A global 0.14 predates--localandparseInitArgssilently ignored unknown flags, so run 1 was a plain global-mode 0.14 init: it offered the 14→15 update, wrote mcp/skills files, and never touched package.json. That's symptoms 1–3 in one.pnpm init(pnpm 10/11) writes onlydevEngines.packageManager, and no lockfile exists before the first install. Detection fell back to npm, whose own devEngines gate instantly rejects the install (EBADDEVENGINES, ~250 ms). This is the 221–537 msINSTALL_LOCAL_PACKAGE_FAILEDcluster in PostHog (8 events / 7 users last week).spawn pnpm ENOENTplus manual advice that fails identically in the user's shell.detect()keyed on bare~/.cursor/~/.codexdirs, which argent itself creates (aninit -ywith nothing detected configures all editors; allowlist/skills writes too).Telemetry also shows a ~19 s failure shape where an identical manual retry succeeds (transient registry / pnpm first-run state errors).
Fixes
--from=pathspelling, ignores a dangling--from, and keeps first---from-wins semantics. The stale-binary hijack is addressed on the docs side (@latestin the README) rather than by a hard abort.detectProjectPackageManagerhonorsdevEngines.packageManager(object + array forms) andpnpm-workspace.yaml— the latter ranked below real lockfiles so a stray file left by a pnpm→yarn migration can't outrankyarn.lock.shell: truenever ENOENTs there. Other pm failures retry once..cursor/.codexdir counts only if it holds something beyond argent's own artifacts (argent mcp.json entry,permissions.jsonallowlist, manageddeveloper_instructions, bundled rules/agents/skills). Corrupt, unparseable, or empty content conservatively counts as user evidence. The stale-config sweep now scans all adapters so dead entries in no-longer-detected dirs still get pruned.npx @swmansion/argent@latest initand apnpm dlxalternative (npm's devEngines gate blocks the outer npx in fresh pnpm-init projects).Testing
--from=spelling, dangling--from), retry-on-transient-failure (flaky pm shim), detection flips (argent-only dirs incl. permissions.json → not detected;argv.json/sessions//corrupt configs → detected), plus regressions: npm project local init, global-modeinit -y, recovery of the broken post-0.14 state.--from=rejected, Windows ENOENT never firing, stale-sweep interaction, …) — all fixed and re-verified.🤖 Generated with Claude Code