fix(deploy): forward extra terraform args, drop vestigial targets

[cristim]

Problem

Code-review finding HYG-04 (#1172): the Makefile.terraform targets docker-build, docker-skip and frontend-skip passed -var="..." flags as a 4th argument to scripts/tf-deploy.sh, but the script only consumed $1..$3 and ran a plain terraform apply -var-file=.... The flags were silently discarded, so e.g. make docker-skip performed a full deploy including the Docker build.

The verifier also found the targets were never functional at all: none of the named variables (skip_docker_push, skip_docker_build, enable_frontend_build) is a declared root-module variable in any of the three cloud environments (they exist only at module level, hardcoded false in each root build.tf), and nothing in the repo references the targets.

Fix

• Delete the three vestigial targets (and their .PHONY entries) rather than plumbing dead variables through three cloud root modules. They now fail loudly with No rule to make target instead of silently running a full apply. frontend-only (which works via -target=module.frontend) is kept.
• Fix the root cause in scripts/tf-deploy.sh: arguments after <provider> <profile> <action> are now forwarded verbatim to terraform (e.g. -target=..., -var=...), so this class of silent argument drop cannot recur. The empty-array expansion uses the ${EXTRA_ARGS[@]+...} guard to stay set -u safe on bash 3.2 (macOS default).
• Make the usage check reachable (also called out in the finding): PROVIDER=$1 aborted with unbound variable under set -u before the intended usage message could print; now uses ${1:-}/${2:-} and prints the usage text with exit 1.

Test evidence

Verified per the finding's guidance with make -n dry-runs and a stub terraform binary on PATH that records its exact invocation:

• Pre-fix: ./scripts/tf-deploy.sh aws <profile> plan -var="skip_docker_build=true" invoked terraform plan -var-file=<profile.tfvars> with the -var flag silently dropped; ./scripts/tf-deploy.sh with no args died with line 34: $1: unbound variable.
• Post-fix: the stub records plan -var-file=<profile.tfvars> -var=skip_docker_build=true -target=module.frontend (extras forwarded verbatim); a plain invocation renders identically to before (plan -var-file=<profile.tfvars>); no-args prints the usage message and exits 1.
• make -n -f Makefile.terraform docker-build|docker-skip|frontend-skip now fail with No rule to make target; deploy, plan and frontend-only render unchanged.
• bash -n scripts/tf-deploy.sh clean.

Closes #1172

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 43 minutes and 15 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 67660958-5a8b-4f33-975a-6270cbdc40f0

📥 Commits

Reviewing files that changed from the base of the PR and between 451a70f and 6a92d60.

📒 Files selected for processing (2)

• Makefile.terraform
• scripts/tf-deploy.sh

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/hyg-04-fix

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 5 minutes and 45 seconds before sending another message.

[cristim]

Adversarial review — PR #1244

Reviewed against the stated risk surfaces (argument forwarding, injection, -target/-replace, vestigial-target removal, default-arg precedence, state-file safety, provider-version interaction). Net: no blocking findings. Two out-of-scope follow-ups filed.

What I checked

• Argument forwarding correctness. The idiom EXTRA_ARGS=("${@:4}") then terraform <verb> ... ${EXTRA_ARGS[@]+"${EXTRA_ARGS[@]}"} is the canonical bash-3.2 / set -u safe pattern for "expand array only if non-empty". The inner "${EXTRA_ARGS[@]}" is quoted, so array element boundaries (and embedded spaces in operator-quoted args like '-var=key=hello world') survive intact. The PR description's stub-binary recording (apply -var-file=... -var=skip_docker_build=true -target=module.frontend) matches what I expected. Inline comment on the bash-3.2 rationale is a nice touch.
• Quoting fix in the catch-all branch. terraform $ACTION -var-file=... → terraform "$ACTION" -var-file=... quietly fixes a pre-existing word-splitting hole. Good.
• ${1:-} / ${2:-} + usage check. Confirmed: before the PR, PROVIDER=$1 under set -u aborted with line 34: $1: unbound variable before the intended "Usage:" message could ever print. PR fixes that. The exit-1 path is reachable now.
• Injection risk. $ACTION is now quoted in every branch (literal in 6 case arms, "$ACTION" in the catch-all). EXTRA_ARGS elements are already shell-parsed words handed to terraform; the script never evals them. Operator-only path; no CI workflow forwards user-supplied args into this script.
• -target / -replace blast-radius. These are now first-class via the forward, which is the explicit goal. Risk is acceptable because the script is operator-invoked, terraform itself enforces the state lock, and the Makefile targets that wrap the script do NOT forward args — so make deploy stays safe. Worth a one-line warning in the script's usage block at some point, but not blocking.
• Vestigial targets removal. Verified the PR's central claim:

  $ grep -rnE 'variable\s+"(skip_docker_build|skip_docker_push|enable_frontend_build)"' terraform/environments/
  # (no matches)
  

  Those names exist only at module level (terraform/modules/build/variables.tf, terraform/modules/frontend/variables.tf); each environment's build.tf wires them to hardcoded false. So the deleted docker-build / docker-skip / frontend-skip Makefile targets were sending undeclared -var= flags that terraform silently warns-and-ignores, and the user got a full apply anyway — exactly the silent-fallback pattern called out in the project's memory garden. No rule to make target is the right failure mode. Confirmed no references to the removed targets in *.md / *.yml / Makefile* / *.sh / *.tf (the docker-build target in the top-level Makefile is a separate Docker-image build, unrelated).
• Default-arg precedence. -var-file=<PROFILE_FILE> is positioned first, EXTRA_ARGS last. Terraform's CLI is last-wins for -var and merges -var-file left-to-right with later files overriding — both behaviours match operator expectation that "what I type wins".
• State-file safety. No new code path mutates state. State lock continues to be terraform's responsibility. No concurrent-write window introduced.
• Per memory feedback_pr_workflow.md. Base is main ✓.
• Per memory feedback_tf_provider_version_align.md. No provider-version interaction — neither file changes a versions.tf or a constraint.
• feedback_no_silent_fallbacks.md. Replacing a silently-broken target with a hard No rule to make target failure is exactly the right move.

CI state

The three failed checks on this PR (Lint Code, Security Scanning, Integration Tests) are pre-existing repo-wide failures unrelated to this PR's two-file diff (Go lint debt across internal/api, npm-audit findings in the frontend lockfile, and an internal/config integration panic in TestPostgresStore_PurchaseExecutions). Same failures exist on main. From a code-correctness standpoint this PR is mergeable.

Out-of-scope follow-ups filed

• Stale profile README docs three non-functional tfvars (skip_docker_build/push, enable_frontend_build) #1296 — terraform/profiles/README.md still documents enable_frontend_build, skip_docker_build, skip_docker_push as profile-tfvars settings, but none are declared at root, so the docs send users down a silent-no-op path. (type/docs, type/bug, priority/p2, severity/low)
• Route Makefile.terraform frontend-only through scripts/tf-deploy.sh (unify deploy paths) #1298 — Now that the script supports -target=..., the frontend-only Makefile target can drop its direct-terraform path and route through scripts/tf-deploy.sh like every other target, eliminating duplicated ../../../profiles/... arithmetic and giving frontend-only the same colored output / profile validation / auto-init. (type/chore, priority/p3, effort/xs)

Minor pre-existing nit (no fix required, not blocking)

The if [ $? -eq 0 ] && [ "$ACTION" = "apply" ] after the case is effectively dead-code on the failure branch — set -e would have already exited on terraform failure, so $? -eq 0 is always true if reached. Pre-existing; safe to ignore.

Verdict

No fix needed on PR #1244 itself. Recommend re-requesting CR (it was rate-limited on the last ping) and merging once CR is clean and the unrelated CI failures are addressed at the repo level.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.