Repository navigation
fix(ci): pin Node 24 for Windows stdin test - #2765
nborges-aws wants to merge 1 commit into
Conversation
Package TarballHow to installgh release download pr-2765-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.1.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Changes requested
The mechanism here is fine and consistent with what the repo already does (e2e-test.yml and canary.yml already install actions/setup-node@… pinned to "24" on the same image:windows-1.0 CodeBuild runner), and the assertion reorder in src/io/source.test.ts is a genuine diagnostic improvement — expect(errors).toBe("READY\n") failing first surfaces the child's console.error(error) output instead of the masked expected 0, received 1. Two things should be resolved before this merges, though.
1. Pinning to Node 24 likely hides a real supportability gap (.github/workflows/verify.yml:48, .github/workflows/verify.yml:93)
The test that is failing spawns node -e and does await import("ink"). The installed ink is ink@7.1.1, whose own engines.node is ">=22" (confirmed in bun.lock:985 and on the registry for every 7.x release). Meanwhile this package advertises "node": ">=20.12.0" (package.json:16) and README.md:25 says "You need Node.js 20.12 or later". The npm bundle inlines ink (it is not in EXTERNAL in scripts/build.ts), so ink's code runs on the user's Node.
If the Windows image ships a Node older than 22, this test failing is not CI noise — it is the only test in the suite that exercises ink under a real Node, and it is telling you the declared floor is wrong. Pinning the runner to 24 silences that signal, and after this change bun test never exercises the minimum Node version the package claims to support on any platform.
Options:
- Confirm the root cause first (add a
node --versionstep, or temporarily pin to20.12to reproduce). The PR description says the fix is speculative, so this seems worth one CI run before locking in a version. - If the cause is "image Node predates ink 7's requirement": bump
engines.nodeto>=22and updateREADME.md:25, then pin the runner to that floor ("22") so CI tests the documented minimum rather than a version no supported user is required to have. - If you want to keep testing 24, add the floor as a second matrix dimension (or a second
bun test src/io/source.test.tsrun on the floor version) so the>=20.12/>=22claim stays covered.
2. Add a one-line reason for the new step
verify.yml is otherwise meticulous about explaining non-obvious CI decisions (the per-job runner labels, the macOS split, the Linux-only static checks). A bare setup-node in a Bun-only job reads as redundant and is an easy target for a future cleanup, which would silently re-break Windows. A comment such as "src/io/source.test.ts spawns node to verify stdin handling under the Node runtime; the image's default Node is too old for ink" would prevent that.
Neither point affects the test-file change, which is fine as is.
Description
Attempted fix at the windows failure in our verify CI job from a newly added test. Error message is masked, but likely a node and ink compatibility issue. Submitting PR as to let the test run and verify
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.