fix(cloud): keep EC2 UserData dist-check free of ${...} so !Sub accepts it

[iamwhatever]

Problem

Launching an EC2 crew fails immediately at stack creation:

cloudformation deploy failed: An error occurred (ValidationError) when calling the
CreateChangeSet operation: Template error: variable names in Fn::Sub syntax must
contain only alphanumeric characters, underscores, periods, and colons

The stack never gets created, so the crew can't come up at all. Regression from #2059.

Why it matters

This is a hard block on the whole dashboard EC2-launch feature: every launch dies at
CreateChangeSet before a single resource is provisioned. It slipped past CI because
cfn-lint did not flag the offending ${...} and no test validated the !Sub tokens;
only the real AWS CreateChangeSet call rejects it.

Fix

• Symptom: CreateChangeSet rejects the template with the Fn::Sub variable-name error.
• Root cause: the dashboard-build guard added in feat(cloud): durable launch jobs + owner-only API for EC2 crew setup #2059 folds the setup-log tail into
  the WaitCondition failure reason using a bash default-expansion,
  ${fe_err:-<none captured; ...>}. The entire UserData is a CloudFormation !Sub,
  which parses every ${...} as a Sub variable reference. fe_err:-<none captured; ...>
  is not a legal Sub variable name (:-<, spaces, /, ;), so the template is invalid.
  The existing code already knows this — sibling escapes use the ${!literal} form — but
  the new line used a raw brace expansion.
• Change: rewrite the dist-check to use only plain $fe_err / $LOG (no ${...}),
  with an explicit if [ -z "$fe_err" ] default instead of the brace default. Behaviour
  is unchanged; the real npm/vite/tsc error is still folded into the reason.

Tests

• New TestSubTemplateSyntax::test_every_sub_variable_is_legal scans the rendered
  template and asserts every ${...} is either the ${!...} literal escape or a valid
  Sub variable name ([A-Za-z0-9_:.]+). It fails on the pre-fix line and guards the
  whole class going forward.
• test/test_cloud_ec2.py full suite: 74 passed locally.

Manual verification

Not yet re-launched. Next EC2 launch from a gateway carrying this template should get
past CreateChangeSet; if the frontend build then fails, the WaitCondition reason now
carries the real build error (the feature #2059 intended, minus the syntax break).

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 460b390f590ed94911a4faa541b6a726d8f44155 — this comment is updated in place on each push.

Review details

The fix is clean and minimal. The offending ${fe_err:-<none captured; ...>} brace expansion (which !Sub parses as an illegal variable reference) is replaced with plain $fe_err plus an explicit if [ -z "$fe_err" ] default. The :- semantics (default on unset OR empty) are preserved by -z. The new default string is assigned inside a plain bash "" context with no ${...}, and $fe_err in the fail line is a bare-dollar reference that !Sub passes through untouched. Behavior matches the pre-fix intent; the syntax error is resolved.

No findings.

[OPUS-REVIEWED] 460b390

Verdict parsed from the review's SHA-scoped output markers for commit 460b390f590ed94911a4faa541b6a726d8f44155.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 460b390f590ed94911a4faa541b6a726d8f44155: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 460b390f590ed94911a4faa541b6a726d8f44155 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 460b390

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 460b390f590ed94911a4faa541b6a726d8f44155: <one-sentence reason>

[iamwhatever]

Disposition for 047d56e1b → 460b390f5.

GPT BLOCKING — kirocrew-ec2.yaml:492 — "comments reintroduce invalid !Sub references" — VALID, FIXED. My own incomplete fix.

Correct and reachable. The UserData is one big !Sub, and a bash # comment is still part of the Sub string — CloudFormation scans it for ${...} too. The NB comment I added to warn about this literally contained ${...} twice, so it reintroduces the exact CreateChangeSet ValidationError the commit set out to fix. Worse, my regression test missed it: ${...} has inner ..., which matched the old charset check (. is a legal Sub character), so it passed while still being a bogus reference.

Fix, two parts:

1. Reworded the comment to contain no dollar-brace token at all (describes the rule in words: "never a brace expansion … any dollar-brace token even inside a comment is parsed as a Sub variable"). Verified the only remaining ${...} tokens in the file are real refs: the ${!tail_ctx} escape, AWS:: pseudos, declared Parameters (${DashboardPort}, ${KirocrewRef}, ${SourceBucket}, ${SourceKey}, ${StackTag}, ${KirocrewRepo}), the ${WaitHandle} resource, and the ${Param} Sub variable-map key on the SSM-resolve line.
2. Hardened test_every_sub_variable_is_legal: each non-escape ${...} must now be an AWS:: pseudo or an identifier that STARTS WITH A LETTER (optionally .Attribute), not merely charset-valid. ${...} starts with . → now fails; all real refs above still pass.

Local: 74 test_cloud_ec2.py tests pass, flake8 clean.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 460b390f590ed94911a4faa541b6a726d8f44155 — updated in place on each push; does not block merge.

Both tokens in the fix are now brace-free, all remaining ${...} tokens in the template are legal Sub references or ${!...} escapes, and the new test closes the CI gap (cfn-lint missing this class) that let the regression ship — that's a root-cause fix, not just the symptom line. The change is minimal, matches the description exactly, and carries no compat or reversibility risk (the template is re-rendered per launch, no persisted data).

Design-Verdict: PASS

Root cause fixed at both levels: the illegal token is gone and a regression test now guards the whole !Sub-token class CI previously missed.

[DESIGN-REVIEWED] 460b390