fix(claude-apps-gateway): make deploy.sh safe to re-run — status-aware pass 1 + destructive-change gate

[peepeepopapapeepeepo]

Fixes #287.

Problem

cdk/scripts/deploy.sh ran CDK pass 1 (-c imageReady=false) unconditionally. Pass 1 synthesizes a template containing only the ECR repository, so re-running the script against a live stack updates it to that template — CloudFormation removes the VPC, RDS, ALB, ECS service, secrets, and DNS record. Details and repro in #287.

Changes (all in claude-apps-gateway/cdk/scripts/deploy.sh)

1. Status-aware step 1. describe-stacks now feeds a case on StackStatus instead of running pass 1 blindly:
   • stack absent → run pass 1 (first deploy, unchanged behaviour)
   • CREATE_COMPLETE / UPDATE_COMPLETE / UPDATE_ROLLBACK_COMPLETE → skip pass 1, go straight to build + pass 2
   • ROLLBACK_COMPLETE (failed first deploy — not updatable) → fail early with the exact delete-stack + wait recovery commands
   • any in-progress state → fail with a wait-and-retry message
2. EcrRepositoryUri output guard. A partial/rolled-back stack returns ""/None; previously that flowed into the CodeBuild IAM policy (repository/None) and failed far from the cause. Now it fails immediately with a pointer to the console.
3. Terminal build states. The CodeBuild poll treats FAULT and TIMED_OUT as failures; previously they spun the loop forever.
4. Destructive-change gate on pass 2. cdk diff runs first; the deploy aborts if the change set would remove (^\[-\]) or replace (requires replacement / may be replaced) any resource. FORCE_DESTRUCTIVE=1 overrides for intentional changes (e.g. dropping VPC endpoints after CREATE_VPC_ENDPOINTS=false). The match is deliberately narrow — a bare replace grep would trip on cdk's own boilerplate line on every run. Synth errors are echoed before exiting instead of dying silently under set -e (cdk diff exits 0 on mere differences unless --fail; verified on 2.1133.0).

Testing

• bash -n clean.
• cdk suite: npm test — 17/17 pass (no template changes; script-only).
• Branch logic exercised for NONE, all three healthy states, ROLLBACK_COMPLETE, and in-progress states.
• Live re-run against an UPDATE_COMPLETE deployment (ap-southeast-7) lands on the skip path and proceeds to build + pass 2.

By submitting this pull request, I confirm that my contribution is made under the terms of the repository's license.

[bluedoors]

Thanks for working on this. This is a bug i'd identfied previously and raise a PR to resolve so this overlaps directly with #266 in the same deploy.sh section, so the two changes will need to be reconciled before either merges unchanged. That is a maintainer integration issue rather than a reason to prefer one contribution here.

In terms of this PR, I found two blocking issues.

The status query fails open

STACK_STATUS=$(... 2>/dev/null || echo "NONE")

This maps every describe-stacks failure to the branch that runs the repo-only deployment. A transient throttle, network failure, or refreshed credential can make the status call fail and the following CDK call succeed against a live stack. Since this query is the safety control, it needs to fail closed: treat only an explicit CloudFormation "stack does not exist" response as absence, and stop with the original error for everything else.

The catch-all status branch also gives incorrect recovery advice. REVIEW_IN_PROGRESS is a stable change-set-only state left by --no-execute or an interrupted deployment; it will not settle by itself. Terminal failures such as UPDATE_ROLLBACK_FAILED, IMPORT_ROLLBACK_FAILED, DELETE_FAILED, and CREATE_FAILED also require operator action rather than waiting.

The safest shape is to put the narrow allowlist on the destructive branch: run pass 1 only when the stack is absent or known to contain no deployed resources, and skip pass 1 for every other state. That avoids having to maintain a list of every safe and future CloudFormation status.

The pass-2 gate rejects routine task-definition replacements

AWS::ECS::TaskDefinition is immutable. Any ContainerDefinitions change creates a new revision, which CDK reports as requires replacement even though this is the normal ECS deployment mechanism.

This local repro needs 2>&1 because the pinned CDK CLI writes its diff to stderr:

cd claude-apps-gateway/cdk
CTX="-c region=us-east-1 -c gatewayName=claude-gateway -c publicUrl=https://gw.example.com \
 -c zoneName=example.com -c zoneId=Z123456789ABCDEFGHIJK -c ingressCidr=10.0.0.0/8 \
 -c certArn=arn:aws:acm:us-east-1:111122223333:certificate/abc"

npx cdk synth --quiet -c imageReady=true $CTX -c imageTag=latest -o /tmp/p2
NO_COLOR=1 npx cdk diff --template /tmp/p2/ClaudeGatewayStack.template.json \
  -c imageReady=true $CTX -c imageTag=v2 2>&1 |
  grep -Ec '^\[-\]|requires replacement|may be replaced'

The results were:

image tag change: 1
no-op:            0
ingressCidr:      0
pass 2 to pass 1: 62

I also synthesized the admin API edit from root README section 5. Adding GATEWAY_ADMIN_WRITE_KEY and GATEWAY_ADMIN_READ_KEY to the container's secrets map produces:

[~] AWS::ECS::TaskDefinition Gateway/TaskDef ... replace
 └─ [~] ContainerDefinitions (requires replacement)

CPU, memory, environment, and other legitimate task-definition edits have the same result. The current script avoids the image-tag case only because it pins the mutable :latest tag, so the task definition itself does not change.

The gate also runs after pass 1, while the destructive failure in #287 happens during pass 1. The status decision is the mechanism that fixes #287; the gate cannot protect its own fail-open path.

I would drop the gate from this PR. If replacement protection is retained, it needs to distinguish routine task-definition revisions from replacement of stateful resources. Matching only ^\[-\] is not enough because it would allow replacement of RDS, secrets, or ECR. A structured CloudFormation change-set check would be safer than parsing CDK's human-readable output.

Other changes

The EcrRepositoryUri guard and the FAULT/TIMED_OUT handling are correct and should stay.

Please add a committed regression test for the status decision. The sample already keeps dependency-free shell tests under claude-apps-gateway/test/, separate from the Jest suite in cdk/test/, and CLAUDE.md asks for a test when a deployment trap is fixed. Cases should cover absent, healthy, ROLLBACK_COMPLETE, REVIEW_IN_PROGRESS, a terminal failure, an in-progress state, and a failed status query.

Finally, please document the new rerun behavior and any retained override in the script's Usage block and the CDK README. While you are in the docs, the "its own inline Dockerfile and config" clause at docs/deployment.md:222 is now stale — deploy.sh stamps from gateway.yaml.template and uploads the tracked Dockerfile. The rest of that note, about the SHA-verified binary, still holds.

[bsnehanshu]

Withdrawing this review. Deferring to @bluedoors' review above.

Withdrawn — premature approval; deferring to @bluedoors' blocking review.

[bluedoors]

Thanks for the careful write-up on this. The FAULT/TIMED_OUT find is real and is now carried on #266 as 7384fc7, with you as co-author — buildStatus has six values and the loop only treated three as terminal, so a faulted or timed-out build polled forever.

Taking #266 for the rest, for three reasons:

• The pass-2 cdk diff gate matches [-] Output NextStep on a first deploy — pass 1 emits that output and pass 2 drops it — so every fresh install aborts at Step 4/5.
• Any ContainerDefinitions edit reports requires replacement on AWS::ECS::TaskDefinition, which is how ECS revisions work; the gate blocks routine config changes. Only the :latest pin hides this today.
• 2>/dev/null || echo "NONE" maps a throttle, an expired credential, or a network blip to the branch that runs the repo-only deploy against a live stack — the safety control fails open into the bug it is meant to prevent.

#266 resolves the pass-1 destruction with a fail-closed status query, a narrow allowlist on the destructive branch, the same EcrRepositoryUri guard, and test/deploy-helpers.test.sh covering the status matrix.

Superseded by #266 — I don't have triage rights on this repo to close it here, so a maintainer or you can close it. #287 stays open until #266 merges.