Repository navigation
fix(ci): test CLI on supported Node 22 floor - #2770
Draft
nborges-aws wants to merge 1 commit into
Draft
nborges-aws wants to merge 1 commit into
nborges-aws wants to merge 1 commit into
Conversation
Contributor
Package TarballHow to installgh release download pr-2770-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.1.tgz |
Contributor
|
Claude Security Review: no high-confidence findings. (run) |
Contributor
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
I verified the premise and the mechanics against the PR's base branch (v1.x):
ink@7.1.0does declareengines.node: ">=22"on npm, so the floor bump is justified and theverify.ymlcomment accurately explains why the stdin regression test needs a pinned Node.verify.ymlis the only workflow that runsbun test, and both jobs (verifymatrix andverify-macos) now getsetup-node@22, so the spawn-nodepath insrc/io/source.test.tsis covered on Linux, Windows, and macOS.- No stale reference to the old floor is left behind:
20.12only remains insrc/assets/templates/runtime-docs/typescript/*andsrc/core/project/runtimeBuild.test.ts, which are about the Node version of generated runtime projects, not the CLI itself. - The reworded publish-job comment is still correct — Node 22 ships npm 10.9.x, so
npm install -g npm@11is still needed for trusted publishing. - No
command.mdchurn, and the assertion reorder insource.test.tsgenuinely surfaces the child's stderr before the exit code, which is the useful failure message.
Two non-blocking notes for the author to decide on:
package.jsonengines.node: ">=22"is a consumer-visible support drop. The commit is typedfix(ci):and the "Breaking change" box is unchecked, so this will ship as a patch while dropping Node 20/21 installs (npm will emitEBADENGINE, and anyone withengine-strictwill fail outright).CONTRIBUTING.mdasks that breaking changes be called out. Options: retype/annotate the commit so the release notes mention the new floor, or add an explicit note in the PR description/release notes that v1 now requires Node 22..github/workflows/pr-automation.yml:181still pinsnode-version: "20.x"for the npm-lockfile path, which is now below the declaredenginesfloor. Harmless today (npm only warns), but it is the one remaining job that contradicts "we now install Node 22 in our jobs" if you wanted consistency.
Neither requires a change before merge from my side.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v1.x #2770 +/- ##
=======================================
Coverage 96.55% 96.55%
=======================================
Files 707 707
Lines 58675 58675
=======================================
+ Hits 56653 56654 +1
+ Misses 2022 2021 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nborges-aws
marked this pull request as draft
November 5, 2026 22:05
This branch has not been deployed
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
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.
Description
This PR raises the CLI's minimum Node.js version from 20.12 to 22. CLI dependencies (namely Ink 7) require Node 22 or newer, which we currently do not meet. Our windows CI runner has been failing recently, which diagnosis traces back to compatibility issue with Node 20.12. We now install Node 22 in our jobs, and update documentation to match the new Node floor.
Additionally, updated the stdin regression test (which was the failing windows case) to show stderr before sending the exit code, to make root causing during future debugging simpler.
Type of Change
Testing
How have you tested the change?
bun testbun run test:e2e, or explained why they are not applicable(CI does not run them for you)
bun run typecheckbun run lint:checkbun run format:checkbun run auditbun run secrets:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themby this PR
command.md(the release workflow regenerates it)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.