fix(cli-internal): guard confirmPrompt against node 24 readline crash#14949
Draft
sarayev wants to merge 1 commit into
Draft
fix(cli-internal): guard confirmPrompt against node 24 readline crash#14949sarayev wants to merge 1 commit into
sarayev wants to merge 1 commit into
Conversation
Node 24 changed readline.Interface#pause() to throw ERR_USE_AFTER_CLOSE when the interface is already closed; Node 22 and earlier treated the call as a no-op. inquirer@7's baseUI.close() calls rl.pause() unconditionally during prompt teardown, so when stdin closes (EOF / piped input) as a confirm prompt finishes, the throw rejects the prompt promise and bubbles up. This is what makes `amplify add function` die with the generic "There was an error adding the function resource" on Node 24: its openEditor() step asks a confirm question through context.amplify.confirmPrompt(). Guard the legacy confirmPrompt helper (the implementation behind context.amplify.confirmPrompt) so an ERR_USE_AFTER_CLOSE thrown during teardown is caught. The answer is captured on the prompt UI before teardown runs, so we recover it from the prompt promise's ui.answers and return it, restoring the pre-Node-24 behavior where the teardown was a silent no-op. If the prompt closed before an answer was recorded, we fall back to the supplied default. confirmPrompt has ~28 callers. Re-implementing it on top of amplify-prompts' enquirer-based prompter would change --yes and non-interactive behavior for all of them (callers such as s0-analyzeProject already gate --yes themselves), so the narrow guard is the lower-risk fix and covers every caller, including openEditor, at once. Testing: added regression tests that simulate inquirer's teardown crash (answer captured on ui.answers, then the promise rejects with ERR_USE_AFTER_CLOSE) and assert confirmPrompt returns the real answer instead of throwing; also verified against the real inquirer@7 that the answer is present on the prompt promise's ui.answers after the crash. Built with `tsc -b` and ran the amplify-helpers jest suite (40 suites / 155 tests pass); lint clean. A separate amplify-category-api change patches the e2e harness's installed inquirer as a stopgap; that patch can be retired once this product fix ships in a cli-internal release. --- Prompt: Root-fix the Node 24 `amplify add function` crash in the PUBLISHED Gen1 CLI (repo: amplify-cli; PRs target the `dev` branch). ROOT CAUSE: Node 24 made readline.Interface THROW ERR_USE_AFTER_CLOSE when pause() is called after the readline is closed (Node <=22 silently no-op'd). The legacy helper confirm-prompt.ts uses inquirer@7.3.3; inquirer's baseUI.js close() calls this.rl.pause() unconditionally. When stdin closes (EOF / piped input) right as a confirm prompt finishes, Node 24 throws and `amplify add function` dies with the generic "There was an error adding the function resource". The crash path is amplify-category-function openEditor() -> context.amplify.confirmPrompt(...). confirmPrompt is @deprecated in favor of confirmContinue from amplify-prompts (enquirer-based), which does not have this bug. Produce the durable fix in amplify-cli: prefer making confirmPrompt Node-24-safe at the source so every caller benefits, either by re-implementing on amplify-prompts' prompter while preserving the exact public contract, or, if that risks changing behavior for the many callers, by guarding the inquirer path against the closed-readline crash (the minimal change that restores pre-Node-24 behavior for all callers). Also ensure openEditor() no longer crashes. Add a regression test that reproduces the closed-readline / EOF-after-confirm scenario and asserts it no longer throws. Build + test the touched package(s). Follow the repo commit rules and open a draft PR targeting `dev`, noting that amplify-category-api #3503's harness patch is a stopgap that can be retired once this ships in a cli-internal release.
6 tasks
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
1 participant
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 of changes
amplify add functioncrashes on Node 24 with the generic🛑 There was an error adding the function resource. This restores thepre-Node-24 behavior.
Root cause
Node 24 changed
readline.Interface#pause()to throwERR_USE_AFTER_CLOSEwhen the interface is already closed; Node 22 and earlier treated the call as a
no-op.
inquirer@7'sbaseUI.close()callsrl.pause()unconditionally duringprompt teardown. So when stdin closes (EOF / piped input) just as a confirm
prompt finishes, the
pause()call throws, the prompt promise rejects, and theerror bubbles up and aborts the command.
The user-visible crash path is
amplify-category-function'sopenEditor(),which asks "Do you want to edit the local … file now?" through
context.amplify.confirmPrompt(...). That routes to the legacyconfirm-prompt.tshelper, which is built oninquirer@7and therefore hitsthe throw.
Fix
Guard the legacy
confirmPrompthelper (the implementation behindcontext.amplify.confirmPrompt) so anERR_USE_AFTER_CLOSEraised duringteardown is caught. inquirer records the user's answer on the prompt UI
before teardown runs, so the helper recovers it from the prompt promise's
ui.answersand returns it — exactly what the caller would have received onNode ≤22. If the prompt closed before any answer was recorded, it falls back to
the supplied default. Any other error is re-thrown unchanged.
Because every caller of
context.amplify.confirmPromptfunnels through this onehelper, this fixes
amplify add functionon Node 24 and the same latent crashfor all ~28 other callers, without changing behavior on older Node versions.
Why guard instead of re-implementing on
amplify-promptsconfirmPromptis@deprecatedin favor ofamplify-prompts' enquirer-basedprompter, and re-pointing it there was considered. It was rejected as toorisky for a bug fix: the
prompterthrows in non-interactive (non-TTY) shellsand owns
--yeshandling, whereas several existingconfirmPromptcallers(e.g.
s0-analyzeProject) already gate--yesthemselves and rely on thecurrent non-interactive semantics. The narrow guard restores the exact
pre-Node-24 contract for every caller and is the lower-risk change. Migrating
callers to
amplify-promptsremains a good future cleanup, separate from thisfix.
Relation to the e2e harness stopgap
A separate
amplify-category-apichange (#3503) patches the e2e testharness's installed
inquireras a stopgap so suites stop crashing on Node24. This PR is the actual product fix; once it ships in a
cli-internalrelease, that harness patch can be retired.
Issue #, if available
N/A. Related stopgap: aws-amplify/amplify-category-api#3503 (e2e harness patch,
can be retired once this ships).
Description of how you validated changes
packages/amplify-cli/src/__tests__/extensions/amplify-helpers/confirm-prompt.test.tsthat simulate the Node 24 teardown crash: the answer is captured on
ui.answersand the prompt promise then rejects withERR_USE_AFTER_CLOSE.The tests assert
confirmPromptreturns the real answer (yes/no) instead ofthrowing, falls back to the default when no answer was captured, and still
re-throws unrelated errors.
inquirer@7that the answer ispresent on the prompt promise's
ui.answersafter the simulated crash, so therecovery reads a real value rather than a test artifact.
tsc -b(clean) and ran theamplify-helpersJest suite:40 suites / 155 tests pass. Lint clean on both touched files.
Checklist
yarn testpasses (touchedamplify-helperssuite: 40 suites / 155 tests)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.