POK-387: Update active agent installations and verify outcomes - #48
Conversation
Co-authored-by: multica-agent <github@multica.ai>
saheljalal
left a comment
saheljalal
left a comment
There was a problem hiding this comment.
Review — POK-387 / PR #48
Verdict: Request changes — the root-cause analysis is right and the detection work is strong, but the new "verify or fail" policy collapses unknown into failed, which makes aikit update report a failure and exit 1 on a healthy, up-to-date Hermes. Two smaller correctness issues (native-first over package managers, -1 suppressing the manager fallback) and an npm-prefix discovery regression should be resolved too.
What's good: the reproduction doc is evidence-based and honest about its limits; tests/test_aikit_updates.py uses real disposable executables and package trees rather than mocks, and the regression-first methodology (five tests failing on the original code) is exactly right. I ran the suite on this branch — 575 passed, 1 skipped — and git diff --check is clean. The codex update correction, the ownership-evidence rules (pacman DB over "we're on Linux", /Cellar/ over /homebrew/, npm prefix from the active tree), and the 2.0.0 bump under the repo's exit-code rule are all well judged.
Blocker
aikit:2288—execute_agent_updaterewrites every non-upgraded/up_to_dateoutcome tofailed, sounchangedandunchanged_outdatedcan never reach_do_update_impl; those display branches and theunchanged_outdated/unchangedterms in the exit-code expression are dead. An inconclusive check (available is None) on a current agent therefore reports FAILED and exits 1. Reproduced on this branch against a stub Hermes.
Should-fix
aikit:2293—run()returns-1for a spawn failure as well as a timeout, sobreakalso suppresses the manager fallback when nothing ran at all.aikit:1910— native updater now runs before every detected manager, including brew/mise/pipx/uv/cargo/pacman, reversing the invariant the old comment recorded.aikit:133—_maintenance_env()in_run_npmstripsNPM_CONFIG_PREFIXfrom the discovery callnpm config get prefix, breaking_npm_global_prefixes()on env-configured prefixes.aikit:1813—sudo -n pacman -S --needed --noconfirmsilently escalates and performs a partial upgrade on the reported platform.aikit:1788—_manager_installationnow spawns subprocesses and is uncached;/api/agentscalls it once per agent per request.
Nit
aikit:5928— the[:500]bound on the dashboard'sstderrwas dropped.
Details and suggested fixes are in the inline comments.
| manager=plan["manager"], path_before=str(before), path_after=str(after), attempts=list(attempts)) | ||
| if outcome["status"] in ("upgraded", "up_to_date"): | ||
| return outcome, "\n".join(outputs), "\n".join(errors) | ||
| outcome["status"] = "failed" |
There was a problem hiding this comment.
Blocker — an inconclusive update check turns a healthy install into a hard failure.
Every outcome that is not upgraded/up_to_date is rewritten to failed here, so the unchanged and unchanged_outdated statuses that classify_update_outcome produces can never reach _do_update_impl. That makes its elif status == "unchanged_outdated" / elif status == "unchanged" branches and both summary lines dead code, and reduces return 1 if failed or unchanged_outdated or unchanged else 0 to 1 if failed.
The user-visible consequence: _text_update_availability now returns None for any phrasing it does not recognise, should_skip_update does not skip on None, the updater runs, the version legitimately does not change — and aikit reports FAILED and exits 1 for an agent that is perfectly current. Reproduced against this branch with a stub Hermes that is up to date but words its check differently:
❌ ☤ Hermes Agent update failed: Updater completed without a verified update; command: …/bin/hermes update; manager: native/installer; active executable: …/bin/hermes; version: 2.24.0; updater output: Nothing to do.
❌ Failed (1): Hermes Agent
do_update(...) == 1
Hermes is the agent from the original report, and its version_check is cmd_text with available_substr: "Update available". The negative regex only recognises already up to date / no updates available / you are up to date; anything else ("you're on the latest release", "latest version already installed") becomes a permanent false failure on every run, and breaks any script that checks aikit's exit status — which is the contract this PR just made a major version out of.
Suggested fix: preserve the classification from the last attempt and only force failed when something actually failed — exit_code != 0, a version regression, a missing version, or check.get("available") is True. Leave unchanged as unchanged (it already has a sensible presentation and its own exit-code slot), and widen the negative-verdict patterns in _text_update_availability while you're there. If "unknown" really should be non-zero, it should still not print and summarise as a failure.
| outcome["error"] = _update_failure_detail(agent_key, command, before, after, outcome, stderr or stdout, plan["manager"]) | ||
| # A timeout may have interrupted a partial mutation. Do not start a | ||
| # second updater after an uncertain termination. | ||
| if exit_code == -1: |
There was a problem hiding this comment.
Should-fix — -1 also means "could not launch", so this suppresses the manager fallback when nothing ran.
run() returns -1 from two places: the timeout path and except Exception: return -1, "", str(e) around Popen. Confirmed on this branch:
run(["/nonexistent/bin/hermes", "update"], shell=False, timeout=5)[0] # -> -1The native command is bound to an absolute path by _bind_agent_command, so a shim that a previous self-update removed/replaced, or one that is not executable, fails to spawn, returns -1, and this break skips the detected-manager retry — exactly the recovery case this PR is built for. The comment's rationale (a timeout may have left a partial mutation) only applies to the timeout.
Suggested fix: distinguish the two — either a separate sentinel/flag from run() for spawn failure, or gate the break on the timeout marker it already writes into stderr (Command timed out after …). Note test_timeout_does_not_start_fallback_manager stubs run to return -1 directly, so it passes for both cases and won't catch this.
| primary = agent.get("bin", "") | ||
| native = registry if registry and (registry == primary or registry.startswith(primary + " ")) else None | ||
| commands = [] | ||
| if native: |
There was a problem hiding this comment.
Should-fix — native-first now applies to package-manager-owned installs too, reversing a deliberate invariant.
native is always attempted before managed_cmd, whatever the owner is. The code this replaces did the opposite on purpose: "The detected command takes precedence over the registry's update_cmd so agents installed via mise/brew/pipx/uv are upgraded by that manager instead of self-updating out from under it" — and test_aikit_detect_install_manager_mise was rewritten in this PR to assert the new order.
For npm/bun/installer-owned binaries native-first is right and is what fixes the reported Hermes/Grok bug. For brew/mise/pipx/uv/cargo/pacman-owned binaries a successful native self-update writes over manager-managed files and leaves the manager's version state stale, so a later brew upgrade / mise upgrade reinstalls the old build on top. For pacman-owned binaries under /usr/bin it is usually just a wasted attempt, but a self-updater that falls back to ~/.local/bin will quietly create the second copy this PR is otherwise careful to avoid.
Suggested fix: order by owner — native first when the manager is npm/bun/None, manager first (with native as the fallback) for mise/brew/pipx/uv/cargo/pacman/aur. That keeps the fix for the reported agents without reversing the invariant.
| cache, env = _npm_subprocess_env() | ||
| try: | ||
| return run(cmd, timeout=timeout, env=env) | ||
| return run(cmd, timeout=timeout, env={**_maintenance_env(), **env}) |
There was a problem hiding this comment.
Should-fix — sanitising _run_npm breaks npm prefix discovery, not just mutation.
_maintenance_env() strips NPM_CONFIG_PREFIX (and the lowercase npm_config_prefix) — verified on this branch, both keys come back mapped to None. But _run_npm is also the discovery path: _npm_global_prefixes() (aikit:1455) calls npm config get prefix through it. On a machine that sets the global prefix via the environment, that call now reports npm's built-in default instead of the user's actual prefix, so _npm_global_prefixes() no longer knows about the real global tree.
Knock-on effects outside the update path:
resolve_agent_bin(aikit:1554, via_npm_global_bin_dirs) can no longer find an agent installed under that prefix when its bin dir isn't on PATH — which weakens exactly the detection this PR is about.npm_uninstall_cmd(aikit:1486) finds no matching prefix and falls back to--prefix ~/.local, soaikit uninstallreports success while removing nothing.
The update path doesn't need the strip: _node_installation already pins --prefix/--global-dir from the resolved binary. Suggested fix: apply _maintenance_env() only to the mutating npm invocations, or have _run_npm re-add the prefix variables.
| if not helper: | ||
| return "aur", None, {}, f"AUR package {package} owns {raw}; install paru/yay or update it with your AUR build workflow" | ||
| return "aur", [helper, "-S", "--needed", "--noconfirm", package], {}, "" | ||
| prefix = ["sudo", "-n"] if hasattr(os, "geteuid") and os.geteuid() != 0 else [] |
There was a problem hiding this comment.
Should-fix — this escalates privileges and performs a partial upgrade, neither of which aikit update advertises.
sudo -n pacman -S --needed --noconfirm <pkg> mutates system packages without asking. Beyond the elevation itself, pacman -S <single-package> against the local sync database is a partial upgrade — the case Arch explicitly warns against, since it can pull newer dependencies into an otherwise un-upgraded system. The aur branch below has the same shape plus a source build (paru/yay -S --needed --noconfirm).
Omarchy is the platform in the report, so this is the path most likely to be hit in practice. The docs already say aikit "does not refresh system databases or perform a whole-system upgrade" — that reads as a safety property, but installing a single package is the unsafe half of it.
Suggested fix: for pacman/aur owners, report the owning package and direct the user to sudo pacman -Syu (consistent with the "repository metadata may lag" hint already in _update_failure_detail), or keep the automated path but put it behind an explicit opt-in flag / confirmation rather than --noconfirm under sudo -n.
| return None, None | ||
|
|
||
|
|
||
| def _manager_installation(agent_key): |
There was a problem hiding this comment.
Should-fix (performance) — this is now subprocess-backed and uncached, but it's called per agent per dashboard request.
Detection used to be pure path-string inspection with zero subprocesses. It now runs pacman -Qoq (up to twice), possibly pacman -Qm, and _python_installation's interpreter probe — each with a 10s timeout — and there's no memoisation.
/api/agents (aikit:5827) calls resolve_update_cmd(key) for every agent on every request, and /api/agents/<key> (aikit:5860) does it again. On an Arch box with a dozen agents installed, one dashboard load is now ~2 pacman invocations per installed agent, serially, before the page renders. _update_plan also calls _installed_bin_path three more times on top of the one _manager_installation already did.
Suggested fix: memoise per agent key for the process lifetime (same pattern as UPDATE_CHECK_CACHE), and/or drop update_cmd from the list endpoint and resolve it only in the detail endpoint.
| "latest_version": outcome.get("latest") if outcome else None, | ||
| "stdout": stdout[:500] if stdout else "", | ||
| "stderr": (outcome.get("error") or stderr)[:500] if (outcome or stderr) else "", | ||
| "stderr": ((outcome or {}).get("error") or stderr) if (outcome or stderr) else "", |
There was a problem hiding this comment.
Nit — the [:500] bound was dropped here while stdout above keeps it. _update_failure_detail can get long: the PATH enumeration, up to 500 chars of updater output, plus command/owner/paths. Worth keeping a bound on the API response.
Co-authored-by: multica-agent <github@multica.ai>
saheljalal
left a comment
saheljalal
left a comment
There was a problem hiding this comment.
Re-review — c69a106
Verdict: Approve — all seven findings from the first round are resolved, each with a regression test that pins the behavior. Merge state is CLEAN, CI test is green, and I reproduced the fixes locally (597 passed, 1 skipped; py_compile and git diff --check clean).
Prior findings — verified on this commit
| # | Finding | Status |
|---|---|---|
| Blocker | unchanged collapsed into failed |
Fixed. execute_agent_update now only rewrites failed/unchanged_outdated, so _do_update_impl's unchanged branch is live again and return 1 if failed or unchanged_outdated no longer counts it. My original repro — a current Hermes whose check phrasing isn't recognized — now prints version unchanged; update check inconclusive (2.24.0) and returns 0. I also re-ran it with a phrasing outside the widened regex entirely (Nothing to upgrade, mate.): still unchanged, still exit 0, so the fix doesn't depend on the pattern list being exhaustive. Dashboard returns success: true / exit_code: 0 and the JS no longer claims "updated!". |
| Should-fix | -1 conflated spawn failure with timeout |
Fixed. Pre-spawn failures return -127; the post-communicate exception path correctly keeps -1 (a child did start). Confirmed the manager retry now happens on a spawn failure and still does not happen on a real timeout — and test_real_timeout_stops_before_manager_retry uses an actual 30s sleep against a 0.03s timeout rather than a stubbed return value, which is what makes the two cases genuinely distinguishable. |
| Should-fix | native-first over every manager | Fixed. if native and manager in (None, "npm", "bun"). Verified a mise-owned claude plans exactly [["mise", "upgrade", "claude"]] with no native attempt, and the characterization test's original intent is restored. Going owner-exclusive rather than owner-first is stricter than I suggested; the rationale in the code comment (a post-failure native write still desyncs tracked files) is sound and test_receipt_owner_handles_updates_without_native_overwrite covers the failed-manager case across all six receipt managers. |
| Should-fix | _maintenance_env() broke npm prefix discovery |
Fixed. _run_npm restores NPM_CONFIG_PREFIX/NPM_CONFIG_USERCONFIG in both spellings while still dropping injection vars — confirmed PYTHONPATH is still removed on the same call. The regression test asserts both the _npm_global_prefixes() and npm_uninstall_cmd consequences, not just the env dict. |
| Should-fix | sudo -n pacman -S --noconfirm |
Fixed. pacman/AUR are now query-only. Verified with a recording pacman stub that only -Qoq and -Qm are ever invoked and attempts is empty, and the reported reason names the owning package plus the full-system workflow. |
| Should-fix | uncached subprocess probes per dashboard request | Fixed. INSTALL_MANAGER_CACHE keyed on the resolved path + stat fingerprint + discovery env, 60s TTL. Verified 5 calls → 1 probe, invalidated by a binary change, and invalidated before and after every update attempt. Returning list(command) / dict(env) copies is the right call given _update_plan mutates env for installer_bin_dir_env. |
| Nit | dashboard stderr unbounded |
Fixed, with a test asserting exactly 500. |
Notes (non-blocking, no action needed for merge)
- An agent that is genuinely outdated but whose check is inconclusive now exits 0 as
unchanged. That's the correct trade against the false-failure blocker anddocs/aikit.mdsays so explicitly ("This does not claim that the installed version is current") — worth keeping in mind if the phrasing list ever needs to grow. - The
unchanged_outdatedCLI line now prints the full_update_failure_detail, so the original report's three-agent case will be three paragraphs rather than three one-liners. Fine given the diagnostics are the point; only worth revisiting if it reads as noise in practice. -127reads as a magic number ataikit:1420. No collision risk (real signal-based returncodes don't reach 127), but a named constant would document the contract thatexecute_agent_update:2335depends on.
Nice work on the round — the regression suite in tests/test_aikit_update_review.py tests the actual mechanisms (real subprocesses, real timeouts, a recording package-manager stub, probe counting via a marker file) rather than the fix's shape, which is why I could verify each item independently instead of taking the tests' word for it.
Live Omarchy and native Windows/macOS verification is the remaining gap and is going to QA rather than blocking merge.
Co-authored-by: multica-agent <github@multica.ai>
saheljalal
left a comment
saheljalal
left a comment
There was a problem hiding this comment.
Re-review — Bun delta eec24d8
Verdict: Approve. The invocation is correct against real Bun, the regression pins it properly, and the change is confined to the Bun branch.
Verified against real Bun 1.3.14 (not just the stub)
The previous round's lesson was that a permissive stub can bless an invocation the real tool rejects, so I checked the variable names against the actual binary rather than the docs link:
BUN_INSTALL_GLOBAL_DIR="/tmp/.../pkg dir" BUN_INSTALL_BIN="/tmp/.../bin dir" BUN_INSTALL=<decoy> bun install -g cowsay@latest→ packages landed inpkg dir, andcowsay/cowthinksymlinks landed inbin dir. The decoyBUN_INSTALLwas never used, confirming both variables take precedence over$BUN_INSTALLderivation — which is whataikit:1736-1739relies on.- The old form reproduces QA's failure exactly:
bun install -g is-odd@latest --global-dir ... --global-bin-dir ...→error: Could not find package.json for "file:/tmp/.../bin dir" dependency. So the flags were indeed parsed as package specs; removing them is the right fix, not a workaround.
Verified in aikit itself
Independent probe (not the PR's test), default and relocated layouts, directory names containing spaces, with hostile inherited environment (BUN_INSTALL, BUN_INSTALL_GLOBAL_DIR, BUN_INSTALL_BIN all pointing at decoys, plus NODE_OPTIONS/PYTHONPATH injection):
- plan →
manager: bun,commands: [["bun", "install", "-g", "@oh-my-pi/pi-coding-agent@latest"]]— no flags at all - the subprocess received
BUN_INSTALL_GLOBAL_DIR/BUN_INSTALL_BINpinned to the detected install,NODE_OPTIONS/PYTHONPATHunset,HOMEintact - 18.1.15 → 18.1.17, exit 0, same active executable path afterwards; none of the decoy directories were created
aikit:2306 passes plan["env"] into run(), which merges over os.environ and drops None sentinels, so the pin survives _maintenance_env() sanitization — the ordering in _update_plan:1957 ({**_maintenance_env(), **env}) is the part that makes this work, and it's correct.
Code-level notes
_node_installationgained a third return value;aikit:1852is the only caller and it was updated, and the npm branch correctly returns{}rather than leaking a mutable default._manager_installationalready copies withdict(env)before returning, so the 60s probe cache can't be poisoned by_update_planmutatingenvforinstaller_bin_dir_env.- Cache signature already includes
BUN_INSTALL_GLOBAL_DIR(aikit:1808), which is the only environment variable the Bun detection reads — so a relocation invalidates the probe.BUN_INSTALL_BINdoesn't need to be in the signature because the bin dir is derived from the resolved binary path, which is already fingerprinted. - The regression is genuinely strict: the stub rejects any argv that isn't exactly
install -g <pkg>@latestand independently asserts both destination variables, with a conflicting inheritedBUN_INSTALL_GLOBAL_DIRin the default-layout case. That's the shape that would have caught the original defect.
Nit (non-blocking): resolve_update_cmd returns only the argv, so the dashboard's update_cmd field (aikit:5867, aikit:5900) now shows bun install -g <pkg>@latest with no indication of the destination. Harmless for aikit's own execution, but a user copy-pasting it for a relocated Bun layout would install to the default location. Worth surfacing the pinned env alongside it if that field is ever presented as a runnable command.
Suite green at eec24d8 (599 passed, 1 skipped), py_compile and git diff --check clean, CI test passing, version correctly coalesced at 2.0.0 with the changelog bullet appended to the existing section.
1 participant
Closes POK-387
aikit updatecould update a different installation prefix, invoke the nonexistentcodex updatecommand, or misclassify Hermes checks. A successful process exit also counted as success when the executable on PATH stayed outdated.Updates now detect the active installation from npm/Bun paths, pip interpreter/RECORD ownership, pipx/uv roots, Cargo receipts, mise, Homebrew, or pacman/AUR ownership. Standalone/npm/Bun installs prefer registered native updaters with manager fallback; other managed installations use their owner exclusively to keep package receipts consistent. pacman/AUR installations report the owning package and require a full system update workflow instead of running elevated partial upgrades.
Maintenance subprocesses clear injection and destination overrides while preserving identity/network settings. npm discovery retains configured prefixes. Ownership probes are cached for unchanged binaries for up to 60 seconds and invalidated before/after updates. Spawn failures allow manager fallback; timeouts stop. Every attempted update re-resolves and version-checks the executable. Bun uses plain
bun install -gwithBUN_INSTALL_GLOBAL_DIRandBUN_INSTALL_BINpinned from the active installation, avoiding unsupported destination flags.Updater errors, missing/regressed versions, and confirmed outdated results return CLI exit 1 and dashboard
success: false, with bounded diagnostics including command, owner, active path, and other PATH copies. A successful updater with an unchanged version and inconclusive check remains unchanged, exits 0, and is displayed as inconclusive. This exit-code/API correction is versioned as aikit 2.0.0 under the repository rules.Validation:
git diff --checkpassed.docs/aikit-update-reproduction.md.Tests ran on Linux. Independent QA verified real Arch pacman/AUR ownership, npm Codex upgrades, standalone Grok, mise, Linuxbrew Cask routing, and dashboard outcomes. Native Windows/macOS and true Omarchy hardware remain unverified. System-managed packages require the full system update workflow outside aikit; custom Cargo sources and unproved package ownership receive explicit instructions instead of a guessed migration.