POK-314: Audit curl-install + npm-version_check agent family for explicit uninstall (qwen, blackbox, amp, continue, openclaw) - #49
Conversation
saheljalal
left a comment
There was a problem hiding this comment.
Review — POK-314 (Code Reviewer)
Verdict: Approve — the diff does what the issue asks, is evidence-backed and well tested; findings below are non-blocking. (Submitted as a COMMENT review because GitHub rejects APPROVE from this token identity — "can not approve your own pull request", both agents share the workspace GitHub App.)
Verified independently: family enumeration (computed (install starts with curl) AND version_check.type == npm against the live registry → exactly the nine pinned members, none missed), qwen installer evidence (INSTALL_LIB_DIR=~/.local/lib/qwen-code, wrapper ~/.local/bin/qwen, ~/.qwen/source.json, install_npm() fallback — all covered by the new uninstall), blackbox vendor dirs match discover_auth (~/.blackboxai/~/.blackboxcli), continue's prefix-repair uses persistent npm config set prefix ~/.npm-global so _npm_global_prefixes() detects it, qoder refactor is output-identical, --dry-run smoke for all nine runs clean (exit 0, no prompts/execution), 600 passed + 1 pre-existing env skip locally (tomli_w), CI test green on head, single clean commit, no merge commits.
Should-fix
tests/test_tools_characterization.py:1050— family membership is hardcoded; a future curl+npm agent silently escapes the contract (the same drift that letroo/qoderpast POK-313's version). Add a dynamic completeness assertion (suggestion inline).
Nit
aikit:3087—--dry-runwith no agent keys still opens the interactive picker; "prompts nothing" holds only when keys are passed.
Praise
_npm_channel_cleanup_cmdscorrectly omits the always-emit fallbacknpm_uninstall_cmdhas — exactly the right semantic split between curl-primary (conditional npm cleanup) and npm-wrapping installers (unconditional removal).- Monkeypatching
_npm_global_prefixesin the contract test keeps it deterministic on machines that happen to have one of these npm-installed.
| # derived npm uninstall" assertion cannot flake on a machine that happens | ||
| # to have one of them npm-installed. | ||
| monkeypatch.setattr(m, "_npm_global_prefixes", lambda: []) | ||
| cases = [ |
There was a problem hiding this comment.
Should-fix (non-blocking): the family membership here is a hardcoded list, so the next curl-installed + npm-version_check agent added to AGENTS silently escapes this contract — exactly how roo/qoder drifted past the POK-313 version of this test and needed a separate fix. A dynamic completeness assertion makes the test fail until a new member is classified:
def _install_cmds(agent):
ic = agent.get("install", "")
return [str(c) for c in ic.values()] if isinstance(ic, dict) else [str(ic)]
enumerated = {k for k, a in m.AGENTS.items()
if any(c.startswith("curl") for c in _install_cmds(a))
and a.get("version_check", {}).get("type") == "npm"}
assert enumerated == {c[0] for c in cases} | {"continue", "openclaw"}, enumeratedI verified the current registry enumerates to exactly these nine, so this is purely drift protection. Happy to see it as a fast-follow.
|
|
||
|
|
||
| def _do_uninstall_impl(agent_keys, yes): | ||
| def _do_uninstall_impl(agent_keys, yes, dry_run=False): |
There was a problem hiding this comment.
Nit: aikit uninstall --dry-run with no agent keys still opens the interactive picker (the if not agent_keys: branch below), so "prompts nothing" holds only when keys are passed. Fine if intended (the picker selects what to preview) — if so, consider hinting in the --dry-run help text that keys skip the picker, or auto-previewing all installed agents in dry-run mode.
…nstall --dry-run (POK-314)
Audit of every AGENTS entry with a curl installer and an npm version_check,
verified against each vendor's install script:
- qwen: standalone tree at ~/.local/lib/qwen-code + wrapper; npm only a
fallback channel -> remove tree/wrapper/~/.qwen state, npm cleanup only
when a global copy exists
- blackbox: ~/.blackbox-cli-v2 tree vendors its npm deps internally; both
~/.local/bin/blackbox{,.mjs} wrappers + config dirs removed
- amp: ~/.amp/bin/amp binary + PATH symlinks (~/.local/bin, ~/bin, ~/.bin)
+ ~/.config/amp
- continue: install.sh wraps npm install -g -> npm uninstall kept, adds
~/.continue cleanup
- openclaw: install.sh wraps npm install -g -> npm uninstall added before
the shim/vendor cleanup it previously lacked
Contract test now pins all nine family members (mimo, omp, roo, qoder, qwen,
blackbox, amp + the npm-wrapping continue/openclaw sub-family); qoder's
conditional npm-channel cleanup factored into a shared helper. New
`aikit uninstall --dry-run` previews resolved commands without executing.
Co-authored-by: multica-agent <github@multica.ai>
15c00bd to
b6ccfec
Compare
saheljalal
left a comment
There was a problem hiding this comment.
Re-review — POK-314 (Code Reviewer)
Verdict: Approve — both findings addressed as suggested; rebase resolution verified clean. Ready for human merge. (Still a COMMENT review — the shared App token can't approve its own PR.)
Verified independently:
- Should-fix (dynamic family assertion) —
tests/test_tools_characterization.py:1060now recomputes the curl+npm family from the live registry and asserts equality with the pinned nine. I confirmed it actually fires: injecting a fake curl+npm entry intoAGENTSbreaks the equality ({'fakeagent'}), the clean registry passes. - Nit (dry-run without keys) —
aikit:3090auto-previews all installed agents instead of opening the picker; help text +docs/aikit.mdupdated. Smoke:./aikit uninstall --dry-run </dev/nullpreviews 10 installed agents, exit 0, no picker. Both new tests are meaningful (picker monkeypatched to fail if invoked; none-installed path asserted). - Rebase onto main's 2.0.1 — version/docstring correctly 2.1.0; CHANGELOG stacks 2.1.0 above 2.0.1; POK-392's change survives intact (
print_agent_tablestill suppresses the collision warning;agent_bin_collision_warnings()def + install/doctor call sites present); PR-vs-main diff contains only POK-314 content. - Gates — 602 passed + 1 pre-existing env skip (
tomli_w) locally, consistent with the reported 603; CItestpass on headb6ccfec; MERGEABLE/CLEAN; single commit, no merge-from-main.
One no-action observation: the changelog now reads 2.1.0 — 2026-09-10 above 2.0.1 — 2026-09-11 (the branch's entry predates main's 2.0.1 landing). Semver order is correct and the dates reflect authoring order — nothing to change, just noting it reads oddly.
Closes POK-314
Summary
POK-313 left
mimo/omp(laterroo/qoder) with explicit uninstalls and a contract test pinning the class. This PR audits the entire curl-install + npm-version_checkfamily by reading each vendor's install script, fixes the remaining members, and addsaikit uninstall --dry-run(named in the issue's acceptance criteria; the flag didn't exist). aikit 2.0.0 → 2.1.0 (MINOR — new flag dominates the PATCH-level fixes).Audit (AC1 + AC2)
Nine
AGENTSentries match(install starts with curl) AND (version_check.type == npm):qwen~/.local/lib/qwen-code/(self-containedbin/qwen+ bundled node runtime), wrapper~/.local/bin/qwen, writes~/.qwen/source.json; npm is only the installer's fallback channelnpm uninstall -gno-op'dblackbox~/.blackbox-cli-v2/with npm deps vendored inside the tree (never a global npm install); wrappers~/.local/bin/blackbox+blackbox.mjsamp~/.amp/bin/amp+ symlink into~/.local/bin(or~/bin,~/.bin)continuenpm install -g @continuedev/cli(may repair npm prefix to~/.npm-global)~/.continuesurvivedopenclawnpm install -g openclawmimo,omproo,qoderChanges (AC3 + AC4)
qwen_uninstall_cmd,blackbox_uninstall_cmd,amp_uninstall_cmd,openclaw_uninstall_cmd+ explicituninstall_cmdentries;continuegets the jules/codebuff-shaped lambda (npm uninstall -g+rm -rf ~/.continue). Vendor dirs mirror whatdiscover_authreads (~/.qwen,~/.blackbox{,-cli-v2,/ai,/cli},~/.amp,~/.config/amp,~/.continue, …), souninstall_removes_vendor_datais True for all._npm_channel_cleanup_cmds()(output-identical) and reused by qwen.test_aikit_curl_installed_npm_version_check_agents_have_explicit_uninstallnow pins all nine members (with a monkeypatched empty npm-prefix set for determinism) plus the npm-wrappingcontinue/openclawsub-family;test_aikit_openclaw_registry_entryupdated to the corrected contract (intentional behavior change, documented in-test); the four fixed entries removed fromtest_aikit_npm_agent_uninstall_derived_from_version_check.aikit uninstall --dry-run(parser +do_uninstall(dry_run=)): prints each resolved command with a vendor-data note, executes nothing, prompts nothing.Verification (AC5)
Installer evidence snapshots:
qwen(install-qwen-standalone.sh:INSTALL_LIB_DIR=~/.local/lib/qwen-code,install_npm()fallback),blackbox(BLACKBOX_INSTALL_DIR=~/.blackbox-cli-v2,npm ciinsidepackages/*/dist),amp(AMP_HOME=~/.amp, BIN_DIR symlink),continue(install_cli()→npm install -g "$PACKAGE_NAME"),openclaw(run_npm_global_install→install -g).Files
aikit— 4 new uninstall functions + shared helper, 5 registry entries,--dry-run, version bump 2.1.0tests/test_tools_characterization.py— family-wide contract test, dry-run test, openclaw contract updateCHANGELOG.md,docs/aikit.md— 2.1.0 entry, uninstall row documents--dry-run