Repository navigation
fix(ci): test stdin handling on Node 20.12 - #2777
Conversation
Package TarballHow to installgh release download pr-2777-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.2.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Checked against base v1.x. Both changes hold up:
.github/workflows/verify.yml— the pin (20.12.0) matchesengines.node(>=20.12.0), the action SHA (949feb2…/ v7.1.0) is the same one already used incanary.yml,e2e-test.yml, andpr-automation.yml, andsetup-nodeis already proven on the sameimage:windows-1.0CodeBuild runner class bye2e-test.yml. Nothing else in the job depends on a newer Node — build/pack/compile/smoke-test all run through Bun, and the Codecov action brings its own runtime — so dropping the ambient Node to 20.12 only affects the tests that spawnnode(src/io/source.test.ts,src/io/exec.test.ts,src/io/exec-arguments.test.ts), none of which use post-20 syntax or APIs. Bothverifycallers (ci.yml,release-main-and-preview.yml) pick up the pin, and both jobs in the file were updated.src/io/source.test.ts— gating on theREADYmarker instead of the firststderrchunk is strictly more correct than the oldonce(child.stderr, "data"), and rejecting on earlyclose/errorturns a would-be timeout into a diagnosable failure with the captured stderr. The late-rejection of an already-settled promise by thecloselistener is a harmless no-op, and the subsequentonce(child, "close")still gets its event.
Two non-blocking observations, take or leave:
expect(errors).toContain("READY\n")(src/io/source.test.ts) is now tautological —await readyonly resolves whenerrorsincludes that marker, so the assertion cannot fail. The real regression signal is stillcode === 0andoutput === "late value", which do catch a child that threw after printingREADY, so coverage is intact; the line is just redundant.- The comment "Node 20 can warn while importing Ink" doesn't name the warning. Naming it (or linking the issue) would let a future reader tell when it's safe to tighten the assertion back to an exact match, especially given the PR frames the Node-version bump as the long-term fix.
One coverage trade-off worth being aware of: verify is the only workflow that runs bun test, so after this PR the node branch of this regression test runs only on 20.12.0 — the newer Node the runners previously provided is no longer exercised by it (e2e runs Node 24, but not this test). If cross-version behavior of the stdin path matters, a two-entry Node matrix would be the follow-up; the interim unblock as written is fine.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v1.x #2777 +/- ##
=======================================
Coverage 96.50% 96.50%
=======================================
Files 709 709
Lines 58865 58865
=======================================
+ Hits 56805 56806 +1
+ Misses 2060 2059 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
Pin verification to the CLI minimum Node version, 20.12.0. Update the stdin regression test to wait for the READY marker and tolerate the warning Node 20 may print before it.
Upgrading minimum Node version should be considered as the long term fix for this issue. This PR aims to unblock the workflow in the interim
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.