Repository navigation
fix(cli): don't mistake terraform error output for a missing-variable prompt - #267
Conversation
There was a problem hiding this comment.
How the regression was introduced
@jsteinich — this intersects with your PR #55 (Replace node-pty with simple interactive process). The fix here is correct, but I wanted to flag the full history so you can verify the regex approach aligns with your intent for that refactor.
Timeline of the var. check
1. Upstream original (commit 1c8bc5446, 2023-01-03 — Daniel Schmidt):
} else if (
noColorLine.includes("var.") &&
noColorLine.includes("Enter a value:")
) {
hideLine = true;
const variableName = extractVariableNameFromPrompt(line);
send({ type: "LINE_RECEIVED", line: missingVariable(variableName) });
send({ type: "VARIABLE_MISSING", variableName });
}Both conditions in the same if — correct, but assumed both strings appear in the same line.
2. bufferUnterminatedLines introduced (upstream 2060f2f99 — #2827):
var.mirror
Enter a value:
After this, terraform's prompt arrives as two separate calls to handleLineReceived. The combined This sat silently because in practice, a missing-variable prompt still failed at the && check was now dead — var. and Enter a value: were never in the same line anymore.var. line (terraform exits), so nobody noticed the detection was broken.
3. PR #55 (commit 46ab9fb86, 2026-04-01 — @jsteinich):
- } else if (
- noColorLine.includes("var.") &&
- noColorLine.includes("Enter a value:")
- ) {
+ } else if (noColorLine.includes("var.")) {
hideOutput = true;
const variableName = extractVariableNameFromPrompt(output);
send({ type: "OUTPUT_RECEIVED", output: missingVariable(variableName) });
send({ type: "VARIABLE_MISSING", variableName });
+ } else if (noColorLine.includes("Enter a value:")) {
+ // This comes along with above block, but is a separate line
+ hideOutput = true;
}The split correctly recognized the two-line reality, but VARIABLE_MISSING was kept on the var. side — so it fires on any line containing var., including terraform error output like │ 92: mirror = var.mirror.
This PR's fix
The regex /^\s*var\.([A-Za-z_][A-Za-z0-9_-]*)\s*$/ matches only standalone var.<name> prompt lines. It correctly handles the two-line reality without false-matching error output — arguably better than the original upstream code, which was already broken by bufferUnterminatedLines before PR #55 touched it.
Verification
Reproduced with the exact Gist from #265. Before: Missing variable: 'mirror'. After: real Unsupported argument errors forwarded correctly. All 17 test suites pass (cli-core + cdktn-cli), including the new regression test.
|
@sakul-learning thanks for the thorough history — the regression timeline is spot on. One correction on the dead-code call so we don't remove a load-bearing branch:
Both bullets are correct, but they only cover the missing-variable path. Since PR #55 switched to You can confirm on your machine with this standalone repro (no build needed — it inlines Output: So in the approval (and likewise sentinel-override) flow the branch is reached and hides the Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
… prompt handleLineReceived matched any line containing the substring "var.", so terraform error output that merely references a module variable (e.g. "92: mirror = var.mirror" from an Unsupported argument error) was transformed into a misleading "Missing variable" error, swallowing the real failure during `cdktn deploy`. Scope the detection to terraform's actual interactive variable prompt (a line that is exactly `var.<name>`) via a regex, and reuse it in extractVariableNameFromPrompt. Fixes #265
c9e015c to
6ca8ece
Compare
🤖 Release PR — merge to cut a new release. Kept open and rebased as commits land on `main`. --- <details><summary>0.23.4</summary> ## [0.23.4](v0.23.3...v0.23.4) (2026-07-02) ### Features * **cli:** validate Terraform CLI feature versions ([#237](#237)) ([32d434c](32d434c)) * declared Terraform/OpenTofu target versions (validation foundation) ([#269](#269)) ([0aa2343](0aa2343)) * **lib:** S3-backend - Add `useLockfile` field and update `dynamodbTable` deprecated JSDoc ([#234](#234)) ([32a408f](32a408f)) * **lib:** validate Fn usage against Terraform/OpenTofu versions ([#268](#268)) ([cdf8115](cdf8115)) ### Bug Fixes * **cli:** don't mistake terraform error output for a missing-variable prompt ([#267](#267)) ([0ea946f](0ea946f)) * **cli:** release terraform state lock when interrupting diff/deploy ([#284](#284)) ([de617db](de617db)) * **cli:** resolve prebuilt Java provider versions via repo1.maven.org ([#285](#285)) ([d5af07d](d5af07d)) * **gha:** Adding CHANGELOG.md to prettier ignore list ([#258](#258)) ([5160469](5160469)) * **gha:** pull jsii-terraform CI image from GHCR instead of Docker Hub ([#261](#261)) ([f19d34b](f19d34b)) * **provider-generator:** copy only publishable files into the jsii compile bundle ([#292](#292)) ([562d4c7](562d4c7)) * **provider-generator:** match shorthand provider sources against OpenTofu schema keys ([#293](#293)) ([fda0103](fda0103)) * **release:** add Changelog title for correct release-please insertion ([#255](#255)) ([004818a](004818a)) * **release:** drop devEngines.packageManager that broke npm publish ([ef508a6](ef508a6)) * **tools:** repair generate-function-bindings tool ([#273](#273)) ([821e1cf](821e1cf)) ### Miscellaneous Chores * Add Gradle to Mise config ([#271](#271)) ([1578270](1578270)) * add pnpm to mise ([#266](#266)) ([48ab91f](48ab91f)) * add uv to mise ([#260](#260)) ([163cf3d](163cf3d)) * **deps:** bump nrwl/nx-set-shas from 38457b511e60ee4e1da09255b658e480748a1af4 to afb73a62d26e41464e9254689e1fd6122ee683c1 ([#236](#236)) ([b5ac018](b5ac018)) * **deps:** bump the github-actions-backward-compatible group across 1 directory with 7 updates ([#235](#235)) ([de67d9f](de67d9f)) * **deps:** replace uuid dependency with node:crypto.randomUUID ([#291](#291)) ([6d3657a](6d3657a)) * ESLint ignore test edge-provider-bindings ([#270](#270)) ([ba407bb](ba407bb)) * Fix Verdaccio logging config ([#272](#272)) ([aded64b](aded64b)) * **gha:** parallelize provider binding generation for documentation examples ([#244](#244)) ([0a9c955](0a9c955)) * Migrate package manager from yarn to pnpm ([#170](#170)) ([d23da4e](d23da4e)) * migrate planning artifacts to cdktn-planning [skip ci] ([968c358](968c358)) * **release:** add release-please automation for versioning ([#243](#243)) ([c759eec](c759eec)) * Remove dependency-pinner tool ([#250](#250)) ([42c27d5](42c27d5)) * Remove unused dependencies ([#281](#281)) ([c7de65e](c7de65e)) * **tests:** run verdaccio in-process ([#248](#248)) ([c17677d](c17677d)) * Update JSII version constraint ([#245](#245)) ([d89558f](d89558f)) * Update scheduled workflows to only run on open-constructs/cdk-terrain ([#256](#256)) ([ba408cf](ba408cf)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: open-constructs-cdktn[bot] <291052431+open-constructs-cdktn[bot]@users.noreply.github.com>
|
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Fixes #265
Problem
cdktn deploy(anddestroy) transformed real terraform errors into a misleadingMissing variableerror, swallowing the actual failure.In
packages/@cdktn/cli-core/src/lib/models/deploy-machine.ts,handleLineReceiveddetected terraform's interactive variable prompt with:That substring check fires on any terraform output that references a module variable — including
Unsupported argumenterror lines such as92: mirror = var.mirror. As a result, the real error was hidden behind:(see #265 — this was a regression vs.
0.22.0).Fix
Scope the detection to terraform's actual interactive prompt, which puts the variable name on its own line:
isMissingVariablePrompt()helper backed by/^\s*var\.([A-Za-z_][A-Za-z0-9_-]*)\s*$/replaces the looseincludes("var.")check.extractVariableNameFromPromptreuses the same regex and returns the captured group instead ofsplit("var.").Now genuine missing-variable prompts are still detected, but error output that merely references a variable is forwarded to the user unchanged.
Before / After
Before (
0.23.3):After:
Tests
should not treat terraform error output referencing var. as a missing variable (#265)feeding the exactUnsupported argumentblock from the issue, asserting noVARIABLE_MISSINGevent fires and the real error is still forwarded.extractVariableNameFromPromptcases (var.content,var.SCREAM_CASE,var.nested_content_578CD0EA,var.with-dashes) and thevar.my_varprompt test still pass under the new regex.Note
As the issue diagnosis points out, there is a second, separate problem: the reporter's module references
mirror, which doesn't exist ongitlabhq/gitlab ~> 19.0.0. That's a module/provider version mismatch, not a cdktn bug — this PR simply makes cdktn surface that real terraform error instead of masking it.https://claude.ai/code/session_01HYP7w7BmQM9F8xZTc9qoHD
Generated by Claude Code