feat(update): port CLI self-updater command to refactor architecture - #2151
feat(update): port CLI self-updater command to refactor architecture#2151jariy17 wants to merge 1 commit into
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #2151 +/- ##
============================================
- Coverage 97.16% 97.14% -0.03%
============================================
Files 495 497 +2
Lines 32676 32769 +93
============================================
+ Hits 31751 31834 +83
- Misses 925 935 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Clean port of agentcore update into the refactored Handler/Router architecture. Nicely scoped:
- Business logic (
fetchLatestVersion,compareVersions,handleUpdate) is separated from the handler shell. ProcessRunneris injected so the install path is covered without spawning realnpm— mocking is at the true I/O boundary (matches the repo's guidance).fetchis spied viaspyOn(globalThis, "fetch")and restored inafterEach— appropriately hermetic.renderJsonis called before theSilentCLIErrorthrow, so the JSON result reaches stdout even on a failed install, and the non-zero exit still surfaces to scripts.- Telemetry is auto-instrumented at the router (
cli.command_runwithcommand_path), so no per-handler wiring is needed.
Known follow-ups already called out in the PR description (which I agree are out of scope for a "clean port"):
installArgv()usesdistTag()(@previewvs@latest), butfetchLatestVersion()always queries the/latestendpoint. For a preview build the check and the install target disagree — the check compares against stable, then installs frompreview.- No guard against running
npm install -gin non-interactive/CI contexts, and the registry JSON isn't schema-validated.
Non-blocking observations for a future pass:
PACKAGE_NAME = "@aws/agentcore"is hardcoded while the currentpackage.jsonname onrefactorisagentcore. Fine if this reflects the intended published name, but worth confirming before the branch actually ships.- Test file is
update.test.tswhile the rest ofsrc/handlers/uses co-locatedindex.test.ts. Minor. - In the
"newer-local"test (handleUpdate(false)with no injected runner), the branch returns before invoking the runner, so it's safe today, but passing a mock runner would make the test robust to future refactors ofhandleUpdate.
Nothing here needs to block merge.
c07c1ce to
26875ea
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Ports the `agentcore update` command from the old CLI into the refactored Handler/Router (Bun) architecture. It checks the npm registry for a newer @aws/agentcore and runs `npm install -g` (`--check` reports without installing). - src/handlers/update/action.ts: fetchLatestVersion + compareVersions + handleUpdate, with an injectable ProcessRunner (defaults to the shared runProcess) so the install path is testable without spawning npm. - src/handlers/update/index.tsx: createUpdateHandler, always renders the UpdateResult as JSON (resource-command convention); npm progress streams to stderr so stdout stays pipeable. - Mounted in src/handlers/index.tsx. - 21 bun tests (compareVersions table, fetch spy, injected runner); tsc clean.
26875ea to
3003400
Compare
|
Claude Security Review: no high-confidence findings. (run) |
What
Ports the
agentcore updatecommand from the old CLI (main) into the refactored Handler/Router (Bun) architecture.updatechecks the npm registry for a newer@aws/agentcoreand runsnpm install -g;update --checkreports availability without installing.The
refactorbranch had resource-levelupdatesubcommands (gateway, harness, eval…) but no top-level self-updater — this adds it.Changes
src/handlers/update/action.ts(new) —fetchLatestVersion,compareVersions,installArgv,handleUpdate. The install runs through an injectableProcessRunner(defaults to the sharedrunProcessfromsrc/io/exec.ts), so the install path is testable without spawning a real npm.src/handlers/update/index.tsx(new) —createUpdateHandler; always renders theUpdateResultas JSON (matching the resource-command convention); npm progress streams toio.stderrso stdout stays a clean, pipeable JSON result;SilentCLIErrorfor a non-zero exit on failed install.src/handlers/index.tsx— mounts the handler at the root (2 lines).src/handlers/update/update.test.ts(new) —compareVersionstable,fetchLatestVersionfetch spy (200/404), andhandleUpdatebranches (up-to-date / newer-local / update-available / updated / update-failed) via an injected fake runner.CLI surface
Verification
bun test src/handlers/update/→ 21 pass / 0 failbunx tsc --noEmit→ cleanupdate --help,update --check,update --check --jsonall correctNotes
Faithful port of the old command's behavior (bare
updateinstalls;--checkonly checks). A separate adversarial bug bash surfaced pre-existing gaps carried over from the old CLI (dist-tag check/install channel mismatch, no CI/TTY install guard, unvalidated registry JSON) — not addressed here to keep this a clean port; happy to follow up.