fix(cli): scope upgrade command to detected install mode#896
Open
rohitg00 wants to merge 4 commits into
Open
Conversation
agentmemory upgrade ran npm install in whatever cwd had a package.json, which mutated unrelated user projects and added iii-sdk as a dependency there, while never upgrading agentmemory itself for npx or global users. Add src/cli/install-mode.ts with a pure detectInstallMode helper that classifies the invocation as local-dev (cwd package.json names @agentmemory/agentmemory), npx, or global, and extract the existing npx heuristic into isNpxInvocation so the CLI delegates to one shared implementation. runUpgrade now keeps the dependency refresh and iii-sdk pin only in local-dev mode, prints upgrade instructions in npx mode without mutating anything, and runs npm install -g @agentmemory/agentmemory@latest in global mode. The engine confirm, pinned iii installer, and pinned Docker pull are unchanged. The closing note now reminds users to re-run agentmemory connect claude-code --with-hooks so hook entries pointing at absolute plugin paths get refreshed after the upgrade. Covered by test/install-mode.test.ts with the full mode matrix.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an install-mode detection module (local-dev, npx, global), integrates it into CLI npx detection and the upgrade command to choose mode-appropriate messaging/behavior, and adds Vitest coverage for the new utilities. ChangesInstall-mode detection and upgrade path selection
Sequence Diagram(s)No sequence diagrams generated. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The skills reference generator scans source for AGENTMEMORY_ prefixed tokens to build the recognized env var list, and the exported package name constant matched the pattern, documenting a config variable that does not exist.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli.ts (2)
2362-2370:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the follow-up commands mode-aware too.
The npx branch on Line 2315 says there is no installed CLI, but this final note still tells that same user to run bare
agentmemory statusandagentmemory connect .... Inmode === "npx", those next steps can fail immediately; build the displayed command prefix frommodehere as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 2362 - 2370, The follow-up note shown via p.note currently prints bare commands regardless of CLI mode; make it mode-aware by constructing a command prefix based on the existing mode variable (e.g., use "npx agentmemory" when mode === 'npx' versus "agentmemory" for installed mode) and use that prefix for the displayed commands (the entries passed into p.note). Update the logic that builds the array for p.note to compute and insert this prefix before "status" and "connect claude-code --with-hooks" so the printed next steps match the actual invocation method.
2292-2314:⚠️ Potential issue | 🟠 MajorAbort instead of switching from a detected
pnpm-lock.yamlcheckout to npm
Insrc/cli.ts(local-dev branch),pnpm-lock.yamlonly triggers thepnpm installpath whenpnpmBinis present; ifpnpmis missing butnpmexists, the code falls through to thenpm install/iii-sdkpin path. That runs the wrong package-manager tooling against a pnpm-managed repo and can dirtynode_modulesand lockfiles.Suggested fix
if (mode === "local-dev") { const hasPnpmLock = existsSync(join(cwd, "pnpm-lock.yaml")); - if (pnpmBin && hasPnpmLock) { + if (hasPnpmLock) { + if (!pnpmBin) { + p.log.error("pnpm-lock.yaml detected but `pnpm` is not on PATH."); + process.exit(1); + } const installOk = runCommand(pnpmBin, ["install"], { label: "Refreshing dependencies (pnpm install)", }); requireSuccess(installOk, "pnpm install"); runCommand(pnpmBin, ["up", "iii-sdk@0.11.2"], { label: "Pinning iii-sdk@0.11.2", optional: true, }); } else if (npmBin) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 2292 - 2314, The local-dev block currently falls back to npm when a pnpm-managed repo is detected (hasPnpmLock) but pnpmBin is missing; change the logic in the mode === "local-dev" branch so that if hasPnpmLock is true and pnpmBin is falsy you abort instead of using npm (do not proceed to the npmBin path). Specifically, update the conditional around hasPnpmLock/pnpmBin and npmBin (the code that calls runCommand and requireSuccess) to: when hasPnpmLock && !pnpmBin, log a clear error via p.log.error (or throw/exit similarly) and stop execution; only allow npm paths when no pnpm-lock is present. Adjust handling around pnpmBin, hasPnpmLock, npmBin, runCommand and requireSuccess accordingly.
🧹 Nitpick comments (1)
src/cli/install-mode.ts (1)
18-24: pnpm-as-npx-like is intentional—don’t tighten the UA match without changing tests
isNpxInvocation()treats pnpm user agents that embed" npm/"as npx-like (ua.includes(" npm/")), andtest/install-mode.test.tsexplicitly asserts"pnpm/9.0.0 npm/? node/v22.0.0"returnstrue.- This result is used in
isInvokedViaNpx()to drive the first-run global-install prompt (maybeOfferGlobalInstall()), so widening the heuristic is likely deliberate.- If anything, rename for clarity (e.g., “isNpxLikeInvocation”) since the logic is broader than “npx” naming suggests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/install-mode.ts` around lines 18 - 24, The function isNpxInvocation has broader heuristics (accepts pnpm user-agents containing " npm/") so rename it to isNpxLikeInvocation for clarity and update all call sites and tests accordingly: change the exported function name isNpxInvocation to isNpxLikeInvocation, update any imports/usages such as isInvokedViaNpx and maybeOfferGlobalInstall to call the new name, and update test expectations in test/install-mode.test.ts (and any other tests) to reference the renamed function so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli.ts`:
- Around line 2272-2277: The install-mode detection misclassifies normal npm
runs as "npx" because we pass process.env["npm_config_user_agent"] into
detectInstallMode; update the callsite or the detector so npx is identified only
via explicit markers: remove npmUserAgent from the argument list (i.e., stop
passing process.env["npm_config_user_agent"] to detectInstallMode) or change
detectInstallMode to only treat npx when npm_lifecycle_event === "npx", argv1
contains "_npx", or the user-agent explicitly indicates npx (not just any
"npm/"); reference detectInstallMode, readCwdPackageName, argv1, and
process.env["npm_lifecycle_event"] when making the change.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 2362-2370: The follow-up note shown via p.note currently prints
bare commands regardless of CLI mode; make it mode-aware by constructing a
command prefix based on the existing mode variable (e.g., use "npx agentmemory"
when mode === 'npx' versus "agentmemory" for installed mode) and use that prefix
for the displayed commands (the entries passed into p.note). Update the logic
that builds the array for p.note to compute and insert this prefix before
"status" and "connect claude-code --with-hooks" so the printed next steps match
the actual invocation method.
- Around line 2292-2314: The local-dev block currently falls back to npm when a
pnpm-managed repo is detected (hasPnpmLock) but pnpmBin is missing; change the
logic in the mode === "local-dev" branch so that if hasPnpmLock is true and
pnpmBin is falsy you abort instead of using npm (do not proceed to the npmBin
path). Specifically, update the conditional around hasPnpmLock/pnpmBin and
npmBin (the code that calls runCommand and requireSuccess) to: when hasPnpmLock
&& !pnpmBin, log a clear error via p.log.error (or throw/exit similarly) and
stop execution; only allow npm paths when no pnpm-lock is present. Adjust
handling around pnpmBin, hasPnpmLock, npmBin, runCommand and requireSuccess
accordingly.
---
Nitpick comments:
In `@src/cli/install-mode.ts`:
- Around line 18-24: The function isNpxInvocation has broader heuristics
(accepts pnpm user-agents containing " npm/") so rename it to
isNpxLikeInvocation for clarity and update all call sites and tests accordingly:
change the exported function name isNpxInvocation to isNpxLikeInvocation, update
any imports/usages such as isInvokedViaNpx and maybeOfferGlobalInstall to call
the new name, and update test expectations in test/install-mode.test.ts (and any
other tests) to reference the renamed function so behavior remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63844e97-f778-42d0-9792-581df3c55f24
📒 Files selected for processing (3)
src/cli.tssrc/cli/install-mode.tstest/install-mode.test.ts
npm_config_user_agent starts with npm/ for every npm-mediated run including npm run scripts and lifecycle hooks, so matching it misclassified globally installed users invoking upgrade through an npm script and printed instructions instead of upgrading. npx detection now requires the npx lifecycle event, the _npx cache directory in argv[1], or a user agent that names npx itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
agentmemory upgrade ran npm install in whatever cwd had a package.json, which mutated unrelated user projects and added iii-sdk as a dependency there, while never upgrading agentmemory itself for npx or global users.
New src/cli/install-mode.ts classifies the invocation as local-dev (cwd package.json names @agentmemory/agentmemory), npx, or global, and the existing npx heuristic now delegates to the shared helper. runUpgrade keeps the dependency refresh and iii-sdk pin only in local-dev mode, prints upgrade instructions in npx mode without mutating anything, and runs npm install -g @agentmemory/agentmemory@latest in global mode. The closing note reminds users to re-run agentmemory connect claude-code --with-hooks so hook entries pointing at versioned absolute plugin paths get refreshed after the upgrade.
Tested by test/install-mode.test.ts with the full mode matrix.
Summary by CodeRabbit
agentmemory connect claude-code --with-hooksstep.