Repository navigation
fix: stop with PACKAGE_NOT_INSTALLED when the package to upgrade is not installed (UPT-136) - #273
Conversation
…lled (UPT-136) The install guard before check and fix walked up from each workspace like Node resolution does, past the project root. A node_modules in a parent directory (here /private/tmp/claude-502/node_modules with express 5.3.0) satisfied it, the analysis read that copy as installed, found express "up to date", and fix failed with "express has no upgrade to 5.3.0" (ANALYSIS_FAILED, run.ts:502). - The guard stops at the project root. - The refusal carries NOT_INSTALLED (fix --json error.code), names the package.json that declares the package, says whether nothing is installed or only that package is missing, and keeps the frozen install command for the lockfile. - In fix, an empty check with the package not installed throws NOT_INSTALLED with the install command instead of "has no upgrade"; "has no upgrade" stays for a package already at the target. Nothing is installed anywhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Lucas Braum <lucasbraum@gmail.com>
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
🧹 Nitpicks (1) — 🟢 1 low
This PR introduces an installed-package preflight: before running any fix, the core checks whether the package is present in node_modules (stopping at the repo root so parent-directory installs are excluded) and throws a new NOT_INSTALLED error, which the CLI maps to a user-facing failure with a remediation message.
Reviewed commit: 2790c95 |
There was a problem hiding this comment.
Adds a NOT_INSTALLED preflight check that aborts fix runs when uptide is missing from node_modules, with new CLI failure reporting, exit codes, and tests.
Key findings
- 🟡
NOT_INSTALLEDaborts fixes on partial installs — run.ts:502
| const missing = declared.find((p) => p.installed === 'unknown'); | ||
| if (missing) | ||
| throw new UptideError( | ||
| 'NOT_INSTALLED', | ||
| `${pack.name} is declared in ${join(missing.workspace, 'package.json')} but not installed. Install the dependencies first: ${frozenInstall(packageManager(root))}, then run uptide fix ${pack.name} again.`, | ||
| ); | ||
| const behind = declared.filter((p) => compareVersions(p.installed, target) < 0); |
There was a problem hiding this comment.
🟡 bug · medium
NOT_INSTALLED aborts fixes on partial installs
The new guard throws NOT_INSTALLED when any entry returned by fallbackReport has installed === 'unknown', but fallbackReport emits one entry per workspace that declares the package (packages/core/src/fix/feedback.ts:158-188), so an uninstalled entry in one workspace aborts the whole run. The old code filtered unknown entries out and continued with the installed ones (packages/core/src/fix/run.ts:508). A repo where the package is installed and behind in one workspace but absent in another — e.g. a platform-specific optional dependency — turns a previously working upgrade into a hard error telling the user to run a frozen install. The same unknown value also covers an installed package whose manifest has no readable version, which reinstalling would not fix.
📋 Prompt for AI Agents
In packages/core/src/fix/run.ts around lines 501-508, change the NOT_INSTALLED guard to fire only when the package is uninstalled in every declaring workspace: replace const missing = declared.find((p) => p.installed === 'unknown') with declared.length > 0 && declared.every((p) => p.installed === 'unknown'), and add p.installed !== 'unknown' && back into the behind filter on line 508. Why: fallbackReport returns one entry per workspace and sets installed to installedManifest(...)?.version ?? 'unknown' (feedback.ts:177), so optional/platform-specific dependencies, an uninstalled workspace, or a manifest without a version all read 'unknown' and currently abort fixes that previously worked.
… (UPT-136) Review answers on #273: - One code: the refusal uses the existing PACKAGE_NOT_INSTALLED in fix, check and --json; NOT_INSTALLED is gone. Both message variants stay. - check --json answers every refusal on stdout with {"error": {"code", "message"}}, the same shape and exit codes as fix. - When everything declared is at or past the target, fix says "<pkg> is already at <installed> (target <target>)" instead of "has no upgrade to <target>". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Lucas Braum <lucasbraum@gmail.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## uptide@0.18.3 ### Patch Changes - [#271](#271) [`b66598e`](b66598e) Thanks [@lucasbraum](https://github.com/lucasbraum)! - `fix` no longer hides an agent session that could not work. When the Anthropic API refuses the request (an invalid key, an empty credit balance, a model the key cannot use), the Agent SDK ends the session after one turn with nothing spent and the API's message; Uptide dropped that message, recorded "made no change" for every site and ran the feedback rounds into the same error. Now the task records the error (`llm.tasks[].error`, gate `error`), no further session starts, and every task that leaves nothing says why in `llm.tasks[].noChange`: the API's error, no turn, no output, a refusal, a stop, or every edit rejected. When nothing the agent did stayed, `report.json` (`llm.agentIdle`), the terminal and the PR body say "The agent did not change anything: <reason>", and the PR body no longer says the code was migrated when every site is still manual. - [#271](#271) [`b66598e`](b66598e) Thanks [@lucasbraum](https://github.com/lucasbraum)! - The lockfile check no longer stops an upgrade when the package manager moves an unchanged package's dependency onto the copy the upgrade installs. Upgrading express 4 to 5, pnpm can move another package's `qs` or `iconv-lite` onto the newer copy express 5 brings: the old copy leaves the lockfile, and every package depending on it read as "same version, different resolution", so the run fell back to an agent-led draft. Such a move is now in scope when it stays in the same major (the same minor below 1.0) and never goes back, and the copy it left goes with it when nothing else needs it; the PR body and the terminal list each one as lockfile housekeeping ("onto the upgrade's copy"). A move to a release the upgrade did not bring, a downgrade, a major apart, or a change to what the repository declares itself is still refused. The same applies to npm (a nested copy dropped for the hoisted one) and Yarn Berry (a descriptor that now resolves to the upgrade's copy). - [#273](#273) [`51d0370`](51d0370) Thanks [@lucasbraum](https://github.com/lucasbraum)! - `fix` and `check` in a checkout whose dependencies are not installed now stop before any analysis with `PACKAGE_NOT_INSTALLED` (exit 2), name the `package.json` that declares the package, say whether nothing is installed or only that package is missing, and give the frozen install for the lockfile (`npm ci`, `pnpm install --frozen-lockfile`, `yarn install --frozen-lockfile`, `yarn install --immutable`). Before, a `node_modules` in a directory above the project counted as the project's install, so a fresh clone under such a directory got past the check and `fix` failed with a misleading "has no upgrade". Uptide still installs nothing in your checkout. `check --json` now answers every refusal on stdout with `{"error": {"code", "message"}}`, as `fix --json` does. When the package is already at or past the target, `fix` says "<package> is already at <version> (target <target>)". - [#271](#271) [`b66598e`](b66598e) Thanks [@lucasbraum](https://github.com/lucasbraum)! - The PR body titles each group of sites by what changed instead of an internal key: a pack rule's group by the pack's one-line summary of the rule (`report.json` keeps them as `ruleSummaries`), and sites no rule covers, keyed by the path of the declaration that changed (`e.query`), by what the compiler said there or by what changed ("`e.query` was removed in express 5.3.0"). Co-authored-by: uptide-release[bot] <338861287+uptide-release[bot]@users.noreply.github.com>
Fixes UPT-136.
uptide fix expressin a fresh clone failed withexpress has no upgrade to 5.3.0(ANALYSIS_FAILED). Nowfixandcheckstop before any analysis withPACKAGE_NOT_INSTALLEDand the install command. Nothing is installed anywhere.Root cause
A plain fresh clone was already refused:
requireInstalled(packages/cli/src/failures.ts) runs beforecheckandfixand printed "express is in the lockfile but not installed in node_modules / Next: pnpm install --frozen-lockfile". The bug needs anode_modulesabove the project root:installedIn(packages/cli/src/failures.ts:136on b66598e) walked up from each workspace like Node resolution, past the project root. The UPT-134 repro clone sat under/private/tmp/claude-502/, and/private/tmp/claude-502/node_modules/expressis 5.3.0, so the guard passed.installedManifest→installedPackageDir,packages/core/src/list/evidence.ts:27) and took 5.3.0 as installed. So the check reported express "up to date" andrun.tsfiltered it out.packages/core/src/fix/run.ts:502(496 at c64fb88) treats "installed isunknownor ≥ target" asthrow new Error('express has no upgrade to 5.3.0'). That is a plainError, so--jsonreported ANALYSIS_FAILED.The same ancestor copy also made
check expressin that clone report "✓ no impact", analyzed against a copy that was not the project's.Change
installedInstops at the project root. A copy above the project is not its install.missingNodeModulesreturns aCliErrorwith the existing codePACKAGE_NOT_INSTALLED(CliErrorgains an optionalcode, whicherrorCode()already reads). It names thepackage.jsonthat declares the package and says whether nothing is installed (no node_modules) or node_modules is there, without it. TheNext:line keeps the frozen install for the lockfile.run.ts: when the check returns nothing and a declaring workspace has nothing installed,fixthrowsUptideError('PACKAGE_NOT_INSTALLED', 'express is declared in package.json but not installed. Install the dependencies first: pnpm install --frozen-lockfile, then run uptide fix express again.'). The command comes from the newfrozenInstall(packageManager(root)). When everything declared is at or past the target,fixnow says<pkg> is already at <installed> (target <target>)instead of "has no upgrade to ".check --jsonnow answers every refusal on stdout with{"error": {"code", "message"}}, the same shape and exit codes asfix --json(jsonErrors).docs/commands/check.mdsays so.docs/troubleshooting.mdgets an entry for the message, and there is a patch changeset.Before / after
Fixtures: express 4.21.2 with npm, pnpm, Yarn classic and Yarn Berry (node-modules linker), committed lockfile, no node_modules.
fixran with a dummyANTHROPIC_API_KEY(no agent call is possible) and other keys unset.fix/checkexpress is in the lockfile but not installed in node_modules/Next: npm ci, exit 2,--jsoncode ANALYSIS_FAILEDexpress is declared in package.json but not installed: nothing is installed (no node_modules)/Next: npm ci, exit 2,--jsoncode PACKAGE_NOT_INSTALLEDNext: pnpm install --frozen-lockfileNext: pnpm install --frozen-lockfileNext: yarn install --frozen-lockfileNext: yarn install --frozen-lockfileNext: yarn install --immutableNext: yarn install --immutablefix✖ Migration of express … error: express has no upgrade to 5.3.0, exit 2✖ Repository … nothing is installed (no node_modules)/Next: pnpm install --frozen-lockfile, exit 2checkexpress 4.21.2 → 5.3.0 … ✓ no impact, exit 0 (analyzed the ancestor copy)in the lockfile but not installed in node_modules, exit 2express is declared in package.json but not installed: node_modules is there, without it, exit 2express is declared in apps/web/package.json but not installed: node_modules is there, without it, exit 2--target 4.21.2(= installed)express has no upgrade to 4.21.2express is already at 4.21.2 (target 4.21.2)--target 9.9.9(not on npm)ERR_PNPM_NO_MATCHING_VERSION … The latest release of express is "5.3.0"listwithout node_modulesThe Action
The workflow
uptide initwrites installs before the Action runs (Install dependencies: pnpm install --frozen-lockfile --ignore-scripts, thenuses: uptide-dev/uptide@…; see the__golden__/*/workflow.ymlfiles).node_modulesis at the project root inGITHUB_WORKSPACE, so the root-bounded guard passes. The Action's own engine install lives outside the workspace and was never counted. The Action is unchanged.Tests
packages/cli/src/failures.test.ts: the per-manager cases (npm, pnpm, Yarn classic, and Berry) now assert the new wording. New tests cover a partial install that namespackages/api/package.json, a copy above the project root, andfixwith nothing installed (stderr, no engine call,--jsonerror.code === 'PACKAGE_NOT_INSTALLED').check --jsonis tested on two refusals: not installed (PACKAGE_NOT_INSTALLED) and a dependency that is not there (ANALYSIS_FAILED, "stripe is not a dependency of shop").packages/core/src/fix/isolate.test.ts: an empty check without an install gives PACKAGE_NOT_INSTALLED with the pnpm command, not "has no upgrade". A package already at the target giveszod is already at 4.6.5 (target 4.6.5).packages/core/src/fix/managers/manager.test.ts:frozenInstallfor npm, pnpm, Yarn classic and Berry.codejsonErrorsoff for checkLocal CI with
ANTHROPIC_API_KEY,OPENAI_API_KEYandGEMINI_API_KEYunset:pnpm lint: exit 0 (2 warnings, 1 info, already on main)node scripts/private-material.mjswith a dummyUPTIDE_PRIVATE_DENYLIST:cleanpnpm buildandpnpm typecheck: exit 0pnpm test(turbo): 5 failures infailures.test.tsandsmoke.test.ts(no project, no lockfile, bun lockfile, container assertions). These come from the known stray/private/tmp/package.json, because turbo setsTMPDIR=/private/tmp. Main fails the same way locally. The file was left in place.Review answers (second commit, 5956f78)
check --jsonemits the error object on every refusal. Done, tested and mutation-checked.<pkg> is not declared in any package.json of this repository.PACKAGE_NOT_INSTALLEDis used everywhere andNOT_INSTALLEDis removed.Known gap
Core analysis still resolves installs above the project root (
installedPackageDirinpackages/core/src/list/evidence.ts, used by list evidence andfallbackReport). The CLI guard keeps every CLI and Action run away from it. Code that callsrunFixas a library, without the CLI, can still read an ancestor copy. Left as is, as decided.🤖 Generated with Claude Code