feat(cli): interactive doctor v2 + agentmemory remove#406
Merged
Conversation
Replace passive doctor with an interactive [F]ix/[S]kip/[?]more/[Q]uit prompt per diagnostic. Each check now ships a one-line problem, cause, fix-preview, and inline apply() — and we re-check only the affected diagnostic after a fix, not the whole suite. New flags: doctor --all apply every fix without prompting (CI) doctor --dry-run show what each fix would do, run nothing Catalog (src/cli/doctor-diagnostics.ts): - env-missing → runInit() - no-llm-provider-key → open .env in $EDITOR - engine-version-mismatch → runIiiInstaller + restart - viewer-unreachable → stop + restart engine - stale-pidfile → clear pid + state, restart - env-placeholder-keys → open .env in $EDITOR - iii-on-path-not-local-bin → manual (suggest installer) The catalog is exported as a pure data structure so tests can assert each entry has check/fix/message without booting clack. Passive server checks (graph populated, Claude Code hooks, flags) still run first — they need a live engine and have no one-shot inline fix.
New command tears down everything agentmemory installs: - ~/.agentmemory/iii.pid + engine-state.json (after stopping engine) - ~/.agentmemory/.env (asks separately — holds API keys) - ~/.agentmemory/preferences.json - ~/.agentmemory/backups/ (connect-PR manifest + backups) - ~/.local/bin/iii (only when iii --version matches our pin) - Any agent symlinks from the connect-manifest - ~/.agentmemory/data/ (asks separately, default keep) Surface: agentmemory remove # interactive, double-confirms agentmemory remove --force # skip generic confirms agentmemory remove --keep-data # remove binaries + symlinks only Plan building lives in src/cli/remove-plan.ts as a pure fs-inspecting function so tests sandbox a fake $HOME under tmpdir() and assert on plan shape without touching the real home directory. Always-ask items (.env, memory data, mismatched-version local-bin iii) still get per-item confirmation even with --force.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces two major CLI features: "Doctor v2," a data-driven interactive diagnostic system replacing the prior passive checklist, and a new ChangesDoctor v2 Diagnostics
Remove Command
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Doctor CLI
participant Diagnostics
participant Effects as DoctorEffects
participant Interactive as Interactive Loop
User->>CLI: doctor [--all|--dry-run]
CLI->>Effects: Build context & effects (env, pidfile, engine)
CLI->>Diagnostics: passiveServerChecks()
Diagnostics->>Effects: Query server health, flags, graph
CLI->>User: Display passive summary
loop For each diagnostic
CLI->>Diagnostics: runAllChecks()
Diagnostics->>Effects: Async check (env, keys, version, reachability)
Diagnostics-->>CLI: { diagnostic, status }
end
alt --dry-run
CLI->>Diagnostics: dryRunPlan()
Diagnostics-->>CLI: Plan of failing diagnostics
CLI->>User: Print plan & exit
else --all
loop For each failing diagnostic
CLI->>Interactive: Apply fix automatically
Interactive->>Effects: Execute fix
CLI->>Diagnostics: Re-check diagnostic
end
CLI->>User: Summary & exit code
else Interactive (default)
loop While user doesn't quit
CLI->>User: Fix/Skip/More/Quit prompt
alt User selects Fix
Interactive->>Effects: Execute fix
CLI->>Diagnostics: Re-check diagnostic
else User selects More
CLI->>User: Display moreInfo detail
end
end
CLI->>User: Summary & exit code
end
sequenceDiagram
participant User
participant CLI as Remove CLI
participant RemovePlan as buildRemovePlan
participant Effects as Execution
participant FS as Filesystem
User->>CLI: remove [--force|--keep-data]
CLI->>CLI: Load connectManifest, detect local iii version
CLI->>RemovePlan: buildRemovePlan(context, options)
RemovePlan->>FS: Probe pidfile, engine-state, env, preferences, backups, data-dir, iii
RemovePlan-->>CLI: Plan with applicability & alwaysAsk flags
CLI->>User: Display formatPlan(plan)
loop For each applicable item
alt Item marked alwaysAsk or not --force
CLI->>User: Request confirmation
User->>CLI: Confirm/Skip
end
alt User confirmed or --force (non-alwaysAsk)
alt item.id === stop-engine
Effects->>FS: Read pidfile, state, port
Effects->>Effects: Kill engine process
else item.id === delete-file/dir
Effects->>FS: rmSync(path)
end
end
end
CLI->>User: Summary & exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
🧹 Nitpick comments (1)
src/cli/remove-plan.ts (1)
1-13: 💤 Low valueReduce WHAT-comments; let names carry intent.
This file has several explanatory comments that restate behavior already conveyed by function/type names. Please trim them and keep only non-obvious rationale comments.
As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead".
Also applies to: 154-156, 183-185, 198-200, 216-217, 231-231
🤖 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/remove-plan.ts` around lines 1 - 13, The header and several inline comments in src/cli/remove-plan.ts are restating what the function and type names already convey; remove those "WHAT" comments (including the top file summary and the comment blocks around the remove-plan logic and option handling) and keep only non-obvious rationale notes. Locate the symbols like planRemove / buildRemovePlan (or any functions that construct the removal plan), the RemoveOptions / RemovePlan types, and the CLI-facing helpers for --force / --keep-data, then delete redundant explanatory comments around those symbols while preserving any comments that explain reasoning, side effects, or important invariants.
🤖 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 914-923: The doctor fix path can hard-exit because effects.runInit
calls runInit() which contains process.exit(1) branches; change the control flow
so runInit does not call process.exit directly but instead returns a
DiagnosticFixResult or throws an Error that effects.runInit can catch.
Concretely, remove/replace any process.exit(1) calls inside runInit, make
runInit return a structured result (or throw on fatal failure), and ensure
effects.runInit awaits that result and converts it into the { ok: boolean,
message: string } shape so the interactive flow is preserved and no hard process
termination occurs.
- Around line 1197-1214: The code currently ignores the d.manualOnly flag and
still calls applyFixWithReport in both the applyAll branch and the interactive
loop; update both places so that if d.manualOnly is true you never invoke
applyFixWithReport: in the applyAll branch, check d.manualOnly before calling
applyFixWithReport and increment fixed only when a fix was actually applied,
otherwise log or skip with the existing p.log.info message; in the interactive
loop, prevent the "fix" action by either filtering it out from askFixAction or
adding an explicit guard before the action === "fix" branch that informs the
user the diagnostic is manual-only and continues the loop without calling
applyFixWithReport. Ensure you reference and preserve d.id in any log messages.
In `@src/cli/doctor-diagnostics.ts`:
- Around line 45-47: Change the Diagnostic.id property from string to the
exported DiagnosticId union so invalid IDs fail at compile time: update the
Diagnostic type definition to use DiagnosticId instead of string, ensure
DiagnosticId is exported and matches the entries in DIAGNOSTIC_IDS, and update
any other type occurrences (e.g., the diagnostics array or factory at symbol
names referencing Diagnostic) to use DiagnosticId so the compiler enforces valid
IDs.
- Around line 200-202: The user-facing diagnostic string that begins "Set at
least one of: ANTHROPIC_API_KEY, OPENAI_API_KEY, GEMINI_API_KEY,
OPENROUTER_API_KEY, MINIMAX_API_KEY..." is missing GOOGLE_API_KEY while the
startup check already supports GOOGLE_API_KEY; update that string in
doctor-diagnostics.ts to include GOOGLE_API_KEY among the comma-separated env
var names so the message matches the actual check that looks for GOOGLE_API_KEY.
In `@src/cli/remove-plan.ts`:
- Around line 186-194: The loop that builds plan entries from
connectManifest.installed currently uses entry.target raw (in the block that
pushes into plan) which can be a relative path; update that code to validate and
normalize targets before creating a plan item: use Node's path.isAbsolute and
path.resolve/path.normalize to produce a normalized absoluteTarget; if
entry.target is not absolute, skip the entry or mark applicable false and log a
warning; replace uses of entry.target in applicable and sizeBytes
(pathExists(entry.target), safeSize(entry.target)) with the normalized
absoluteTarget and ensure id/path fields use the normalized value so deletions
cannot resolve against the process CWD.
---
Nitpick comments:
In `@src/cli/remove-plan.ts`:
- Around line 1-13: The header and several inline comments in
src/cli/remove-plan.ts are restating what the function and type names already
convey; remove those "WHAT" comments (including the top file summary and the
comment blocks around the remove-plan logic and option handling) and keep only
non-obvious rationale notes. Locate the symbols like planRemove /
buildRemovePlan (or any functions that construct the removal plan), the
RemoveOptions / RemovePlan types, and the CLI-facing helpers for --force /
--keep-data, then delete redundant explanatory comments around those symbols
while preserving any comments that explain reasoning, side effects, or important
invariants.
🪄 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: 347d8e96-72c9-486e-a4ac-e688ce20fe54
📒 Files selected for processing (5)
src/cli.tssrc/cli/doctor-diagnostics.tssrc/cli/remove-plan.tstest/cli-doctor-fixes.test.tstest/cli-remove.test.ts
Comment on lines
+914
to
+923
| runInit: async () => { | ||
| try { | ||
| await runInit(); | ||
| return { ok: true, message: "Wrote ~/.agentmemory/.env" }; | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| message: err instanceof Error ? err.message : String(err), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Doctor fix path can terminate the process unexpectedly.
effects.runInit delegates to runInit(), but runInit() has process.exit(1) branches. That means selecting a fix can hard-exit doctor instead of returning a DiagnosticFixResult, breaking the interactive flow.
🤖 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 914 - 923, The doctor fix path can hard-exit because
effects.runInit calls runInit() which contains process.exit(1) branches; change
the control flow so runInit does not call process.exit directly but instead
returns a DiagnosticFixResult or throws an Error that effects.runInit can catch.
Concretely, remove/replace any process.exit(1) calls inside runInit, make
runInit return a structured result (or throw on fatal failure), and ensure
effects.runInit awaits that result and converts it into the { ok: boolean,
message: string } shape so the interactive flow is preserved and no hard process
termination occurs.
Comment on lines
+1197
to
+1214
| if (d.manualOnly) { | ||
| p.log.info(`(manual fix only — see "${d.id}" docs)`); | ||
| } | ||
|
|
||
| if (applyAll) { | ||
| const r = await applyFixWithReport(d, ctx, false); | ||
| if (r.ok) fixed++; | ||
| // Re-check only this diagnostic. | ||
| const after = await d.check(ctx); | ||
| if (!after.ok) p.log.warn(`${d.id} still failing after fix.`); | ||
| continue; | ||
| } | ||
|
|
||
| // Interactive prompt loop — allow [?] More info without leaving the check. | ||
| while (true) { | ||
| const action = await askFixAction(d); | ||
| if (action === "fix") { | ||
| const r = await applyFixWithReport(d, ctx, false); |
There was a problem hiding this comment.
manualOnly is not enforced in fix execution paths.
Even when d.manualOnly is true, --all still runs applyFixWithReport(...), and interactive mode still allows fix. This contradicts the manualOnly contract and can trigger unintended side effects.
Proposed diff
- if (applyAll) {
+ if (applyAll) {
+ if (d.manualOnly) {
+ skipped++;
+ p.log.info(`${d.id} is manual-only; skipped in --all mode.`);
+ continue;
+ }
const r = await applyFixWithReport(d, ctx, false);
if (r.ok) fixed++;
// Re-check only this diagnostic.
const after = await d.check(ctx);
if (!after.ok) p.log.warn(`${d.id} still failing after fix.`);
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (d.manualOnly) { | |
| p.log.info(`(manual fix only — see "${d.id}" docs)`); | |
| } | |
| if (applyAll) { | |
| const r = await applyFixWithReport(d, ctx, false); | |
| if (r.ok) fixed++; | |
| // Re-check only this diagnostic. | |
| const after = await d.check(ctx); | |
| if (!after.ok) p.log.warn(`${d.id} still failing after fix.`); | |
| continue; | |
| } | |
| // Interactive prompt loop — allow [?] More info without leaving the check. | |
| while (true) { | |
| const action = await askFixAction(d); | |
| if (action === "fix") { | |
| const r = await applyFixWithReport(d, ctx, false); | |
| if (d.manualOnly) { | |
| p.log.info(`(manual fix only — see "${d.id}" docs)`); | |
| } | |
| if (applyAll) { | |
| if (d.manualOnly) { | |
| skipped++; | |
| p.log.info(`${d.id} is manual-only; skipped in --all mode.`); | |
| continue; | |
| } | |
| const r = await applyFixWithReport(d, ctx, false); | |
| if (r.ok) fixed++; | |
| // Re-check only this diagnostic. | |
| const after = await d.check(ctx); | |
| if (!after.ok) p.log.warn(`${d.id} still failing after fix.`); | |
| continue; | |
| } | |
| // Interactive prompt loop — allow [?] More info without leaving the check. | |
| while (true) { | |
| const action = await askFixAction(d); | |
| if (action === "fix") { | |
| const r = await applyFixWithReport(d, ctx, false); |
🤖 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 1197 - 1214, The code currently ignores the
d.manualOnly flag and still calls applyFixWithReport in both the applyAll branch
and the interactive loop; update both places so that if d.manualOnly is true you
never invoke applyFixWithReport: in the applyAll branch, check d.manualOnly
before calling applyFixWithReport and increment fixed only when a fix was
actually applied, otherwise log or skip with the existing p.log.info message; in
the interactive loop, prevent the "fix" action by either filtering it out from
askFixAction or adding an explicit guard before the action === "fix" branch that
informs the user the diagnostic is manual-only and continues the loop without
calling applyFixWithReport. Ensure you reference and preserve d.id in any log
messages.
Comment on lines
+45
to
+47
| export type Diagnostic = { | ||
| /** Stable id. Used in --json and tests. */ | ||
| id: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Constrain diagnostic IDs to the exported union type.
Diagnostic.id is currently string, which weakens the stability contract around DIAGNOSTIC_IDS. Please type it as DiagnosticId so invalid IDs fail at compile time.
Proposed diff
export type Diagnostic = {
/** Stable id. Used in --json and tests. */
- id: string;
+ id: DiagnosticId;Also applies to: 73-73
🤖 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/doctor-diagnostics.ts` around lines 45 - 47, Change the Diagnostic.id
property from string to the exported DiagnosticId union so invalid IDs fail at
compile time: update the Diagnostic type definition to use DiagnosticId instead
of string, ensure DiagnosticId is exported and matches the entries in
DIAGNOSTIC_IDS, and update any other type occurrences (e.g., the diagnostics
array or factory at symbol names referencing Diagnostic) to use DiagnosticId so
the compiler enforces valid IDs.
Comment on lines
+200
to
+202
| "Set at least one of: ANTHROPIC_API_KEY, OPENAI_API_KEY, GEMINI_API_KEY, " + | ||
| "OPENROUTER_API_KEY, MINIMAX_API_KEY. The daemon picks the first that resolves " + | ||
| "to a real (non-placeholder) value at startup.", |
There was a problem hiding this comment.
Include GOOGLE_API_KEY in the provider key guidance.
The check supports GOOGLE_API_KEY (Line 93), but the user-facing “Set at least one of” list omits it. This can send users down the wrong path.
🤖 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/doctor-diagnostics.ts` around lines 200 - 202, The user-facing
diagnostic string that begins "Set at least one of: ANTHROPIC_API_KEY,
OPENAI_API_KEY, GEMINI_API_KEY, OPENROUTER_API_KEY, MINIMAX_API_KEY..." is
missing GOOGLE_API_KEY while the startup check already supports GOOGLE_API_KEY;
update that string in doctor-diagnostics.ts to include GOOGLE_API_KEY among the
comma-separated env var names so the message matches the actual check that looks
for GOOGLE_API_KEY.
Comment on lines
+186
to
+194
| for (const entry of connectManifest.installed) { | ||
| plan.push({ | ||
| id: `connect:${entry.target}`, | ||
| description: `Remove agent connection (${entry.agent ?? "unknown"})`, | ||
| path: entry.target, | ||
| alwaysAsk: false, | ||
| applicable: pathExists(entry.target), | ||
| sizeBytes: safeSize(entry.target), | ||
| }); |
There was a problem hiding this comment.
Validate connect-manifest targets before planning deletion.
entry.target is accepted as-is. If a manifest contains a relative path, downstream removal may resolve it against the process CWD and delete unintended files. Require absolute paths (and ideally normalize) before creating applicable plan entries.
Suggested hardening
+import { isAbsolute } from "node:path";
...
if (connectManifest?.installed?.length) {
for (const entry of connectManifest.installed) {
+ const target = entry.target;
+ const validTarget = isAbsolute(target);
plan.push({
- id: `connect:${entry.target}`,
+ id: `connect:${target}`,
description: `Remove agent connection (${entry.agent ?? "unknown"})`,
- path: entry.target,
+ path: target,
alwaysAsk: false,
- applicable: pathExists(entry.target),
- sizeBytes: safeSize(entry.target),
+ applicable: validTarget && pathExists(target),
+ sizeBytes: validTarget ? safeSize(target) : -1,
});
}
}🤖 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/remove-plan.ts` around lines 186 - 194, The loop that builds plan
entries from connectManifest.installed currently uses entry.target raw (in the
block that pushes into plan) which can be a relative path; update that code to
validate and normalize targets before creating a plan item: use Node's
path.isAbsolute and path.resolve/path.normalize to produce a normalized
absoluteTarget; if entry.target is not absolute, skip the entry or mark
applicable false and log a warning; replace uses of entry.target in applicable
and sizeBytes (pathExists(entry.target), safeSize(entry.target)) with the
normalized absoluteTarget and ensure id/path fields use the normalized value so
deletions cannot resolve against the process CWD.
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
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.
Summary
Two CLI improvements that close out the "doctor told me what was wrong but didn't help me fix it" and "uninstalling agentmemory leaves orphan files everywhere" gaps.
/.local/bin/iii` only when version matches our pin. Memory data dir (`/.agentmemory/data/`) and `.env` always get a per-item confirmation, even with `--force`. Default mode double-confirms; `--force` skips generic confirms; `--keep-data` removes binaries/symlinks only.Diagnostic catalog
Architecture
Both features split the side-effect-free logic into testable modules:
`src/cli.ts` owns the clack prompt loop and side-effect implementations. Tests sandbox $HOME under tmpdir() and assert on plan shape without ever touching the real home directory.
Test plan
Coordination
Conflicts at merge time will be resolved by the orchestrator.
Summary by CodeRabbit
New Features
doctorcommand with system diagnostics that check configuration and offer fixes through Fix/Skip/More/Quit prompts; supports--allflag for automatic fixes and--dry-runto preview changes without applying them.removecommand for clean uninstallation with--forceand--keep-dataoptions; includes interactive confirmations for critical deletions and version matching for safe removal.Tests