fix(cli): silent killers — viewer port auto-bump, engine version-match warning, adopt-on-attach, stop --force, npx PATH hint#405
Merged
Conversation
The viewer used to log 'Viewer port 3113 already in use, skipping viewer.' and silently fail. On every fresh terminal you'd lose the viewer for the rest of the session unless you noticed the warning buried in stdout. Retry the next 10 ports (3113 -> 3122) before giving up. On fallback, log: 'Viewer started on http://localhost:3114 (fallback from 3113)'.
agentmemory pins iii-engine to v0.11.2 (see IIPINNED_VERSION in
src/cli.ts) but anyone with a different iii on PATH gets silent
behavioral drift -- EPIPE reconnect loops, empty search after save --
because we never tell them their engine doesn't match.
Add warnIfEngineVersionMismatch() and fire it from both:
- startIiiBin() before we spawn (covers fresh starts)
- main() early-return when isEngineRunning() returns true (covers
attach-to-existing)
The warn tells the operator how to silence (AGENTMEMORY_III_VERSION)
and how to downgrade (curl | tar).
runStop() correctly refuses to signal host PIDs when state file is missing and a docker-compose.yml is discoverable -- the engine might have been started by another shell via Docker compose, and SIGTERMing the lsof match would kill an unrelated host process. But when the engine actually WAS started natively (e.g. by a stale agentmemory v0.9.13 with no state-writing) and the state file is gone, the guard becomes a foot-gun: stop refuses, the engine stays up, and the operator has no escape hatch short of `lsof | kill`. Add --force. Documented in --help. The non-force path remains the default and still refuses.
When main() finds isEngineRunning() already true (e.g. a previous shell started it), we early-return without writing iii.pid or engine-state.json. Later `agentmemory stop` then hits the Docker-heuristic guard (state missing + compose discoverable) and refuses to act -- correct in principle, foot-gun in practice. Adopt on attach: write iii.pid from lsof-on-port and engine-state.json as kind=native attached=true, but only if neither already exists. Idempotent -- safe to re-run. Log: 'Attached to existing iii-engine (pid 12345)'.
After the engine is ready, if invoked via npx (detected via
npm_lifecycle_event / process.argv[1] containing _npx /
npm_config_user_agent starting with npm/), emit one line:
Tip: install globally for the bare `agentmemory` command:
npm install -g @agentmemory/agentmemory
Suppressed by ~/.agentmemory/preferences.json with skipNpxHint:true
(a sibling PR will write that file when the user opts out).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR enhances CLI engine lifecycle management with version mismatch detection against a pinned binary version, automatic adoption of already-running engines, and an optional npx installation tip controlled by user preference. The ChangesEngine Lifecycle Diagnostics and Viewer Startup
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 365-389: The adoptRunningEngine function currently marks any
reachable engine as a local/native instance; change it so it only writes native
state and a pidfile (via writeEngineState and writeEnginePidfile) and runs any
native-version checks when you can prove the service is local and native:
require that findEnginePidsByPort(getRestPort()) actually returns a PID and that
the base URL is loopback/localhost (check getBaseUrl() or the rest port binding)
before persisting { kind: "native", attached: true } or logging "Attached to
existing iii-engine"; if no local PID or not loopback, do not write native state
or pidfile and skip the native-version check/logging. Apply the same guard to
the other place that writes engine state/pid (the later
writeEngineState/writeEnginePidfile usage referenced in the review) so
remote/Docker/Windows cases are not misclassified.
🪄 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: 314c678a-9220-4874-864c-f6bc9c27495d
📒 Files selected for processing (2)
src/cli.tssrc/viewer/server.ts
Comment on lines
+365
to
+389
| function adoptRunningEngine(): void { | ||
| try { | ||
| const existingState = readEngineState(); | ||
| const existingPid = readEnginePidfile(); | ||
| if (existingState && existingPid) return; | ||
|
|
||
| const pids = findEnginePidsByPort(getRestPort()); | ||
| const enginePid = pids[0]; | ||
| if (enginePid && !existingPid) { | ||
| writeEnginePidfile(enginePid); | ||
| } | ||
| if (!existingState) { | ||
| writeEngineState({ | ||
| kind: "native", | ||
| configPath: findIiiConfig() || "", | ||
| attached: true, | ||
| }); | ||
| } | ||
| if (enginePid && !existingPid) { | ||
| p.log.info(`Attached to existing iii-engine (pid ${enginePid})`); | ||
| } | ||
| } catch (err) { | ||
| vlog(`adoptRunningEngine: ${err instanceof Error ? err.message : String(err)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Only adopt engines you can prove are local/native.
This path currently treats any reachable getBaseUrl() as a local native iii instance. With a remote AGENTMEMORY_URL, a Docker-published engine, or Windows (where findEnginePidsByPort() always returns []), it still persists { kind: "native", attached: true } and may emit a local-binary version warning for the wrong process. That changes later stop behavior from the existing Docker/ambiguity guard into native PID signaling or a dead-end “could not locate engine process”.
At minimum, don't persist native state or run the version check unless you've established that the target is loopback and you actually found a native engine PID to adopt.
Minimal guard to avoid misclassifying foreign engines
-function adoptRunningEngine(): void {
+function adoptRunningEngine(): boolean {
try {
const existingState = readEngineState();
const existingPid = readEnginePidfile();
- if (existingState && existingPid) return;
+ if (existingState && existingPid) return true;
+
+ const base = new URL(getBaseUrl());
+ const isLoopback = new Set(["localhost", "127.0.0.1", "::1"]).has(base.hostname);
+ if (!isLoopback) return false;
const pids = findEnginePidsByPort(getRestPort());
const enginePid = pids[0];
+ if (!enginePid) return false;
+
if (enginePid && !existingPid) {
writeEnginePidfile(enginePid);
}
if (!existingState) {
writeEngineState({
kind: "native",
configPath: findIiiConfig() || "",
attached: true,
});
}
if (enginePid && !existingPid) {
p.log.info(`Attached to existing iii-engine (pid ${enginePid})`);
}
+ return true;
} catch (err) {
vlog(`adoptRunningEngine: ${err instanceof Error ? err.message : String(err)}`);
+ return false;
}
}
- warnIfEngineVersionMismatch(attachedBin);
- adoptRunningEngine();
+ const adopted = adoptRunningEngine();
+ if (adopted) warnIfEngineVersionMismatch(attachedBin);Also applies to: 699-703
🤖 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 365 - 389, The adoptRunningEngine function currently
marks any reachable engine as a local/native instance; change it so it only
writes native state and a pidfile (via writeEngineState and writeEnginePidfile)
and runs any native-version checks when you can prove the service is local and
native: require that findEnginePidsByPort(getRestPort()) actually returns a PID
and that the base URL is loopback/localhost (check getBaseUrl() or the rest port
binding) before persisting { kind: "native", attached: true } or logging
"Attached to existing iii-engine"; if no local PID or not loopback, do not write
native state or pidfile and skip the native-version check/logging. Apply the
same guard to the other place that writes engine state/pid (the later
writeEngineState/writeEnginePidfile usage referenced in the review) so
remote/Docker/Windows cases are not misclassified.
7 tasks
4 tasks
rohitg00
added a commit
that referenced
this pull request
May 15, 2026
… remove, silent killers) (#407) Patch bump per the established rule: all changes additive, no breaks to MemoryProvider trait or exported types or default behaviour. New top-level subcommands (connect, remove, --reset, --force) are opt-in. PRs included since v0.9.14: #405 — silent killers: viewer port auto-bump, engine version-match warning, stop --force, adopt-on-attach, npx PATH hint #402 — agentmemory connect — automate native-plugin install for 8 agents (claude-code, codex, cursor, gemini-cli, openclaw end-to-end; hermes/pi/openhuman stubbed) #406 — interactive doctor v2 + agentmemory remove (destruction plan + two-confirmation flow) #403 — splash banner + agent grid + provider picker + smart-defaults preferences + bootLog shim (30+ lines of log spam → 10) Files bumped (9): package.json, packages/mcp/package.json, plugin/.claude-plugin/plugin.json, plugin/.codex-plugin/plugin.json, src/version.ts, src/types.ts, src/functions/export-import.ts, test/export-import.test.ts, CHANGELOG.md
6 tasks
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.
Five small DevEx fixes surfaced during v0.9.14 local testing. Each one closes a path where the CLI silently fails or refuses to act with no escape hatch.
Fixes
Viewer port auto-bump —
src/viewer/server.ts:61-148Used to log
Viewer port 3113 already in use, skipping viewer.and silently exit. Now retries up to 10 increments (3113 -> 3122) before giving up. On fallback, logsViewer started on http://localhost:3114 (fallback from 3113). Only fail-loud after all retries.Engine version-match warning —
src/cli.ts:245-258(helper) + wired atsrc/cli.ts:503(startIiiBin) andsrc/cli.ts:699-702(main()attach path).New helper
warnIfEngineVersionMismatch(iiiBinPath)callsiiiBinVersion()and prints ap.log.warnline when the version differs fromIIPINNED_VERSION(currently0.11.2). The message tells the operator how to silence it (AGENTMEMORY_III_VERSION) and how to downgrade (curl | tar). Fires once per process.stop --force—src/cli.ts:1544(flag parse) +src/cli.ts:1583-1592(guard bypass) +src/cli.ts:90-93(--help).runStop()parses--force. When true AND the Docker-heuristic guard would have fired (state missing + compose discoverable), bypasses the guard and proceeds with native pidfile + lsof. Documented in--help. The non-force path remains the default and still refuses.Adopt-on-attach —
src/cli.ts:365-389(helper) +src/cli.ts:702(wire-in).When
main()findsisEngineRunning()already true, newadoptRunningEngine()writes the engine PID (fromfindEnginePidsByPort) to~/.agentmemory/iii.pidand writes{kind:'native', configPath, attached:true}to~/.agentmemory/engine-state.json. Idempotent — never clobbers existing files. LogsAttached to existing iii-engine (pid 12345). This lets a subsequentagentmemory stopactually find the process without--force.npx PATH hint —
src/cli.ts:336-364(helpers) +src/cli.ts:703,772(wire-in).After engine is ready, detects npx via
npm_lifecycle_event,_npxinprocess.argv[1], ornpm_config_user_agentstarting withnpm/. Emits one line:```
Tip: install globally for the bare `agentmemory` command:
npm install -g @agentmemory/agentmemory
```
Suppressed when
~/.agentmemory/preferences.jsonhasskipNpxHint: true.Verification
npm run buildcleannpm test: 11 failed | 892 passed (baseline unchanged — all 11 pre-existing failures intest/mcp-standalone.test.ts)src/cli.tsandsrc/viewer/server.tstouchedCHANGELOG.md/ version bumpTest plan
agentmemory stopon an engine started by a previous shell (without state file) -> still refusesagentmemory stop --forceon the same -> proceeds with native pidfile + lsofiii v0.11.6on PATH, runagentmemory-> see version mismatch warningnpx @agentmemory/agentmemory-> see one-line install-globally tip after engine readyagentmemoryin shell B -> pidfile + state file written, attach message loggedSummary by CodeRabbit
stop --forcemode to force engine stop while bypassing engine state protection checks