feat(update): port CLI self-updater command to refactor architecture - #2151
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.15% -0.02%
============================================
Files 495 496 +1
Lines 32676 32734 +58
============================================
+ Hits 31751 31804 +53
- Misses 925 930 +5 ☔ 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) |
| export async function fetchLatestVersion(): Promise<string> { | ||
| const response = await fetch(`${REGISTRY_URL}/${PACKAGE_NAME}/latest`); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch latest version: ${response.statusText}`); |
There was a problem hiding this comment.
This should be an AgentCoreCLIError. I think we should create a NetworkingError to cover this and other cases.
There was a problem hiding this comment.
We already have a NetworkingError (ref). I'll just use that.
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
This is looking a little gnarly! Can we not use semver?
There was a problem hiding this comment.
I didn't realize it was a library. I'll use that instead.
|
CI note: the only red check is |
Replace the hand-rolled compareVersions with semver.compare (semver was already resolved transitively; added as a direct dep + @types/semver). Fixes the edge cases the hand-rolled parser mishandled (build metadata, v-prefix/non-numeric -> NaN treated as equal, 4th segment ignored, mixed numeric/alphanumeric prerelease precedence). Addresses review feedback.
|
Claude Security Review: no high-confidence findings. (run) |
- fetchLatestVersion throws the existing NetworkingError on both a non-OK registry response and a fetch reject (offline), instead of a raw Error / undici TypeError. - update-failed now carries the caught error message in UpdateResult.error, so the always-JSON output explains the failure even when npm produced no stderr (e.g. npm missing -> ENOENT before any output streams). Addresses review feedback.
|
Claude Security Review: no high-confidence findings. (run) |
Throw a plain AgentCoreCLIError carrying the captured failure reason instead of SilentCLIError, so a failed self-update always prints why (incl. npm missing) and exits non-zero.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
Drop the catch/rethrow around the install. runProcess already throws a ProcessFailedError (an AgentCoreCLIError carrying the command, exit code, and captured npm output), so let it bubble to the root instead of flattening it into a generic error. Removes the update-failed status and error field.
|
Claude Security Review: no high-confidence findings. (run) |
Regenerate bun.lock from the base branch so it keeps lockfileVersion 3, the nested @aws-cdk/toolkit-lib yaml override, and the pinned @opentelemetry/core 2.10.0. An older bun had rewritten all three while adding semver, which broke 'bun install --frozen-lockfile' in CI. Net change is now just the semver and @types/semver entries.
|
Claude Security Review: no high-confidence findings. (run) |
known flaky test: #2140 :( |
|
|
||
| export type UpdateStatus = "up-to-date" | "newer-local" | "update-available" | "updated"; | ||
|
|
||
| export interface UpdateResult { |
There was a problem hiding this comment.
should this be a type since it represents a concrete grouping of data?
There was a problem hiding this comment.
Yes I'll fix this in a follow up
| if (!response.ok) { | ||
| throw new NetworkingError(`Failed to fetch latest version: ${response.statusText}`); | ||
| } | ||
| const data = (await response.json()) as { version: string }; |
There was a problem hiding this comment.
should these two lines lines also be wrapped in networking error? My understanding is that fetch doesn't consume the body until the .json call.
There was a problem hiding this comment.
Yes, ill fix this in a follow up
|
|
||
| describe("fetchLatestVersion", () => { | ||
| afterEach(() => { | ||
| spyOn(globalThis, "fetch").mockRestore(); |
There was a problem hiding this comment.
could we avoid the direct mocking by making the http fetch injectable?
There was a problem hiding this comment.
Yeah, I'll look into doing that in a follow up pr.
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.