feat: thread VPC config env vars into reusable deploy jobs

[alinarublea]

Summary

Adds opt-in declarative VPC attachment to service-ci.yaml so consumers using helix-deploy 13.4+ CLI flags (adobe/helix-deploy#898) can stop clicking VPC config in the AWS console.

Versioning

Shipping as a new v2 tag rather than moving v1:

• v1 stays at its current commit — existing consumers unaffected.
• v2 will be cut at the merge commit of this PR and is where the opt-in behavior lives.
• Consumers that don't need VPC may stay on @v1 indefinitely.

Opt-in via vpc-enabled input

Defaults to false. On v2, a caller opts in by:

jobs:
  ci:
    uses: adobe/mysticat-ci/.github/workflows/service-ci.yaml@v2
    with:
      service-name: <service>
      vpc-enabled: true
    secrets: inherit

What vpc-enabled: true activates

Per deploy job (branch-deploy, deploy-stage, semantic-release):

1. environment: dev|stage|prod — enables env-scoped secrets
2. Validate VPC config present — errors loudly if VPC_SUBNET_1/2/VPC_SG_ID are unset (blocks the silent "detach-VPC" fallthrough)
3. VPC_* env vars exported to the helix-deploy step
4. Verify Lambda VPC attachment — asserts via aws lambda get-function-configuration that the applied VpcConfig matches the expected subnets + SG
5. permissions: { contents: read, id-token: write } — explicit scoping

Workflow signature

Declared secrets: block for VPC_SUBNET_1, VPC_SUBNET_2, VPC_SG_ID (all required: false; enforced when vpc-enabled: true by the Validate step).

Docs

docs/vpc-config.md covers consumer migration (4 steps: Environments, secrets, workflow opt-in at @v2, package.json hlx), environment-protection recommendations, rollback, and egress caveats. Header comment on service-ci.yaml links to it and warns against duplicating AWS_ACCOUNT_ID_* into env-scoped secrets.

Review items addressed

• Critical test: smoke test workflow syntax #1 — opt-in via vpc-enabled, shipped under new v2 tag; v1 untouched
• Critical fix: use exact-match API endpoint for tag existence check #2 — fail-fast Validate VPC config present
• Critical feat: add opt-in integration test jobs (DynamoDB + PostgreSQL) #3 — env protection recommendations documented; defaults preserve current cadence
• Critical refactor: remove it-dynamo input and job #4 — post-deploy Verify Lambda VPC attachment
• Important chore: remove unused S3 artifact upload job #5 — declared secrets: block
• Important chore: Configure Renovate #6 — per-job permissions:
• Important fix: remove pull-requests write from reusable workflow permissions #9 — AWS_ACCOUNT_ID_* shadowing warning in header comment
• Important fix: add pull-requests read to branch-deploy permissions #10 — rollback + egress in docs/vpc-config.md
• Important fix: use status field instead of ahead_by for behind-main check #11 — caller-side coupling + 4-step migration documented

Deferred (follow-ups)

• Important feat: thread VPC config env vars into reusable deploy jobs #7 — split semantic-release so publish/release run ungated
• Important feat: skip DEV deploy when branch is behind main #8 — SHA-pin shared actions
• Minor fix(ci): fix inverted branch-behind-main comparison direction #12 — move VPC identifiers from secrets → vars
• Minor hotfix(ci): disable branch-behind-main check until proper fix ships #13 — VPC_SUBNET_1/2 → comma-separated VPC_SUBNET_IDS
• Minor feat(service-ci): opt-in npm OIDC Trusted Publishers #14-17 — docs & log polish

Pilot

adobe/spacecat-jobs-dispatcher#694 will be switched to @v2 + vpc-enabled: true after merge. GitHub Environments and secrets are already provisioned on that repo and on the other 10 spacecat-* repos with open VPC PRs.

Post-merge checklist

• Cut v2 and v2.0.0 tags at the merge commit
• Switch pilot (spacecat-jobs-dispatcher#694) to @v2 + vpc-enabled: true
• Verify branch-deploy → dev attaches VPC and Verify step passes
• After soak, promote to stage + prod
• Roll out the remaining VPC PRs in waves of 3-5

🤖 Generated with Claude Code

[solaris007]

Hey @alinarublea,

Thanks for centralizing VPC wiring in the reusable workflow - the direction (declarative VPC attachment via helix-deploy, replacing console-clicks) is clearly right. However, this file is consumed by 20+ spacecat-* repos and the plan to move v1 means the change fans out to all of them on their next push. The current shape has several silent-regression vectors and missing guardrails that should be addressed first.

Strengths

• Tight, symmetric diff across the three deploy jobs - easy to reason about
• GitHub Environments are the right primitive for per-env secret scoping and future protection rules
• Pilot-first rollout via spacecat-jobs-dispatcher#694 is the correct instinct
• Existing workflow already uses OIDC for AWS auth and persist-credentials: false on checkouts - good security baseline to build on

Issues

Critical (Must Fix)

1. Moving v1 is an uncontrolled breaking change for 20+ consumers.
service-ci.yaml:125, 160, 191 (the three environment: additions). The PR description says "move the v1 tag so caller workflows pick up the change." Today v1 resolves to the same commit as v1.3.0 (2e8ed22), so callers pinning @v1 inherit the new behavior instantly on their next run - no opt-in, no soak, no per-repo pin that lets them defer. Any consumer repo that has not yet (a) created dev/stage/prod GitHub Environments, (b) populated VPC_SUBNET_1/VPC_SUBNET_2/VPC_SG_ID, and (c) added awsVpcSubnetIds/awsVpcSecurityGroupIds to package.json hlx, will ship a broken or silently wrong deploy on their next push (see finding 2).
Fix: cut a new major (v2) and migrate callers deliberately, or add a vpc-enabled: boolean workflow input defaulting to false so the change is backwards-compatible under v1. Either way, do not move v1 as-is.

2. Empty/missing VPC_* secrets fall through silently.
service-ci.yaml:152-154, 183-185, 213-215. GitHub Actions resolves a missing secret to empty string without failing. The consumer's package.json interpolation "awsVpcSubnetIds": ["${env.VPC_SUBNET_1}", "${env.VPC_SUBNET_2}"] then hands helix-deploy ["", ""] - semantically "detach VPC" per helix-deploy#898, not "no-op." Possible outcomes: Lambda deploys without VPC attachment (silent regression - service looks deployed but cannot reach private Postgres), or mid-deploy failure with a confusing AWS SDK error. Partial configs (subnets set, SG missing) are the single most likely onboarding mistake.
Fix: add a fail-fast guard step before each npm run deploy-* (and opt-in via finding 1):

- name: Validate VPC config present
  if: inputs.vpc-enabled
  env:
    VPC_SUBNET_1: ${{ secrets.VPC_SUBNET_1 }}
    VPC_SUBNET_2: ${{ secrets.VPC_SUBNET_2 }}
    VPC_SG_ID: ${{ secrets.VPC_SG_ID }}
  run: |
    [ -n "$VPC_SUBNET_1" ] && [ -n "$VPC_SUBNET_2" ] && [ -n "$VPC_SG_ID" ] || {
      echo "::error::VPC_SUBNET_1/2 and VPC_SG_ID must all be set in the '${{ github.job }}' environment"; exit 1; }

3. branch-deploy + environment: dev has ambiguous security/UX semantics.
service-ci.yaml:191. The branch-deploy job fires on any push to any non-main branch. Two failure modes depending on consumer config:

• If the dev environment has no protection rules (default), anyone with write to a consumer repo can push a feature branch and deploy arbitrary code to the dev AWS account - environment labels without deployment-branch policies do not gate anything.
• If a consumer later adds required-reviewer rules or a deployment-branch policy to dev, every feature-branch push either blocks on manual approval or fails outright, regressing dev cadence with no warning.

Fix: decide the intended contract and document it as a consumer prerequisite. At minimum, ship a migration checklist that requires each consumer repo to configure dev environment protection (branch policy and/or required reviewer) before adopting. Consider splitting branch-deploy off to a policy-free environment like dev-branches if the UX regression is the bigger worry.

4. Stage and prod have no post-deploy VPC verification.
service-ci.yaml - deploy-stage job and semantic-release job do not run npm run test-postdeploy or any equivalent. Only branch-deploy does (line 217-218). VPC misattachment is exactly the class of failure a static deploy exit code cannot catch - the Lambda deploys green, then times out on first invocation because it cannot reach Postgres, IMS, or external APIs. For a VPC-attachment change, stage/prod need at least aws lambda get-function-configuration --query VpcConfig to assert the applied VPC matches intent, and ideally a real invocation through API Gateway.
Fix: add a post-deploy verification step in both deploy-stage and semantic-release that asserts the Lambda's VpcConfig matches the configured subnets and SG, and (where each service has one) runs npm run test-postdeploy.

Important (Should Fix)

5. workflow_call has no explicit secrets: declaration.
service-ci.yaml:3-22. The contract for what secrets a caller must provide lives only in the PR description. Declare them in the workflow signature so the contract is discoverable and misconfigured callers fail fast:

on:
  workflow_call:
    inputs: ...
    secrets:
      VPC_SUBNET_1: { required: false, description: "Private subnet ID (from spacecat-infrastructure private_subnet_ids[0])" }
      VPC_SUBNET_2: { required: false }
      VPC_SG_ID:    { required: false }

Making them explicit is compatible with secrets: inherit; it simply documents the interface.

6. No explicit permissions: block on deploy jobs.
service-ci.yaml - build, it-postgres, semantic-release, deploy-stage, branch-deploy inherit the caller's default GITHUB_TOKEN scopes, which on older repos is often write-all. Only protect-nyc-config and validate-pr-title have scoped permissions. Add permissions: { contents: read, id-token: write } on each deploy job (id-token: write is required for OIDC to AWS).

7. semantic-release runs publish + GitHub release + Lambda deploy under one environment: prod gate.
service-ci.yaml:125. The job calls npm run semantic-release, which (via semantic-release-plugin-helix-deploy) publishes npm, creates the GitHub release, AND deploys to Lambda. With environment: prod attached, any future protection rule (required reviewers, wait timer) gates all three together. Consumers intending "gate the prod Lambda deploy" will inadvertently gate npm publish and release creation too.
Fix: either document this explicitly in the migration notes, or split the prod deploy out of semantic-release into its own job with its own environment so publish/release can run ungated.

8. Third-party actions pinned to moving tags rather than SHAs.
service-ci.yaml uses actions/checkout@v6, aws-actions/configure-aws-credentials@v5, aws-actions/amazon-ecr-login@v2, codecov/codecov-action@v5, amannn/action-semantic-pull-request@v6. Codecov's bash uploader was compromised in 2021 - if any moving tag is repointed to a malicious SHA, every consumer's next run exfiltrates whatever is in the runner env (now including VPC identifiers and the AWS OIDC token). Pin to full SHAs, track via Renovate/Dependabot with pinVersions: true. Same applies to the internal adobe/mysticat-ci/.github/actions/*@v1 references - same-org trust is still trust, and those v1 tags move too.

9. AWS_ACCOUNT_ID_ env-secret shadowing risk.*
With environment: prod active, environment-scoped secrets shadow repo-scoped ones of the same name. If anyone adds AWS_ACCOUNT_ID_PROD to the prod environment (accidental duplicate during onboarding, wrong value), deploys switch accounts silently. Add a comment in the workflow header or migration doc stating: "Do not duplicate AWS_ACCOUNT_ID_* into environment secrets; they must remain repo-scoped."

10. Rollback playbook and egress strategy are undocumented.
Two distinct operational gaps:

• If a VPC attachment is wrong in prod, there is no documented fast path. Reverting this PR and moving v1 back does not detach existing Lambdas (AWS state persists). Clearing env secrets and redeploying hits helix-deploy's "detach" path, which has not been tested under incident pressure.
• VPC-attached Lambdas lose default internet egress. Outbound to SSM, DynamoDB, S3, SQS, IMS, GSC/Ahrefs, npm runtime requires either NAT gateway (cost + per-AZ failure mode) or VPC endpoints. The PR references spacecat-infrastructure TF output but does not document which egress strategy is in place per AWS account or whether it has been load-tested.

Fix: land a docs/vpc-rollback.md (or equivalent) before the tag moves, and inventory egress dependencies per account - at minimum confirm NAT/VPC endpoint coverage for the pilot service under production-like load.

11. Caller-side package.json change is unchecked and order-dependent.
If a consumer adopts @v2 (or v1 after the move) without also adding awsVpcSubnetIds/awsVpcSecurityGroupIds to package.json, the workflow succeeds green and the Lambda deploys without VPC - silent regression. Conversely if the package.json lands first on an older tag, helix-deploy sees unresolved ${env.VPC_SUBNET_1} templates. Document the coupling explicitly in the migration note ("bump tag AND update package.json in the same PR"), and consider adding a grep-check step that fails if inputs.vpc-enabled: true but awsVpcSubnetIds is missing from package.json.

Minor (Nice to Have)

12. VPC identifiers are configuration, not secrets.
service-ci.yaml:152-154, 183-185, 213-215. Subnet IDs and SG IDs are not sensitive. Modelling them as secrets.* puts them through the redaction pipeline (harder debugging) and the rotation tooling (phantom rotations). GitHub vars.* scoped to the environment is a better fit.

13. Two-subnet hard-coding.
service-ci.yaml:152-153, 183-184, 213-214. VPC_SUBNET_1/VPC_SUBNET_2 forces exactly two-AZ deployment. Adobe prod VPCs typically span 3 AZs. A single comma-separated VPC_SUBNET_IDS secret (or vars) that callers split in package.json is future-proof without being more complex.

14. No in-file documentation of consumer requirements.
Consumer requirements (three environments, specific secret names, secrets: inherit, package.json hlx pattern) exist only in this PR description. Add a header comment block with a link to docs/MIGRATION.md or similar so the next reader discovers the contract without archaeology.

15. No CHANGELOG / release-notes entry for the tag move.
For a shared workflow used by 20+ repos, a CHANGELOG.md entry describing the breaking change, the required consumer steps, and the new v2 tag (or the opt-in input) is cheap insurance for whoever hits this next.

16. Log applied VPC config at deploy time.
At 3am during an incident, a small echo step per deploy job showing which VPC targets were applied means CI logs surface the attachment without a console trip.

17. HLX_AWS_REGION: ${{ secrets.AWS_REGION }}.
service-ci.yaml:29. Region is not sensitive - using secrets.* adds it to the redaction list and confuses inventory. Move to vars.* or plain env:.

Recommendations

1. Adopt an opt-in model: either a new v2 tag or a vpc-enabled: boolean input defaulting to false. Either defuses findings 1, 3, 4, 7 and lets consumers migrate on their own cadence.
2. Before moving any tag, test the missing-secret failure path in a throwaway repo with environment: dev defined but VPC_SUBNET_1 intentionally unset - capture the actual helix-deploy behavior and document it in this PR.
3. Land docs/MIGRATION.md describing the four consumer steps (environments, secrets/vars, secrets: inherit, package.json hlx) and link it from the workflow header. Couple with a docs/vpc-rollback.md.
4. Produce a consumer-rollout tracker (repos times environment-created, secrets-populated, package.json-updated, pilot-deployed, owner). Roll out in waves of 3-5 with a soak between waves.
5. Open follow-ups: SHA-pin all actions via Renovate; migrate VPC identifiers from secrets to vars; collapse the 2-subnet hard-coding into a list.

Assessment

Ready to merge? No - with fixes.
Reasoning: The change itself is small and technically correct, but moving v1 fans it out to 20+ production services with silent-failure paths on missing/empty secrets, no stage/prod verification, no rollback plan, and ambiguous environment-gate semantics on branch-deploy. Path to yes: ship as v2 (or behind an opt-in input), add the fail-fast guard and stage/prod post-deploy verification, document the rollback and migration, and pilot jobs-dispatcher for 1-2 weeks before rolling further.

Next Steps

1. Address the four critical issues first (opt-in rollout, missing-secret guard, branch-deploy semantics, stage/prod post-deploy).
2. Add the secrets: and permissions: declarations - these are high-leverage and fit in this PR.
3. Treat items 8-11 as part of the rollout plan (pinning, account-shadowing note, rollback/egress docs, caller-side coupling), not necessarily blockers for this specific file.
4. Minor items (12-17) can land in a follow-up.

[solaris007]

Important (finding 7): semantic-release does npm publish + GitHub release + Lambda deploy (via semantic-release-plugin-helix-deploy). Attaching environment: prod gates all three together under any future protection rule. Either document this coupling or split the prod deploy into its own job.

[alinarublea]

Thanks for the thorough review @solaris007 — extremely useful framing. I pushed bc5aeba addressing the critical and high-leverage items. Summary of what changed:

In this PR (critical + important):

• test: smoke test workflow syntax #1 vpc-enabled: boolean input defaulting to false. v1 stays backwards-compatible; nothing activates until a caller explicitly opts in. The environment:, VPC env exports, validate and verify steps are all gated on the input.
• fix: use exact-match API endpoint for tag existence check #2 Validate VPC config present step runs before each deploy when opted in; fails the job with a clear error if any of VPC_SUBNET_1/2/VPC_SG_ID is empty, so the silent detach-VPC path is closed.
• feat: add opt-in integration test jobs (DynamoDB + PostgreSQL) #3 docs/vpc-config.md documents environment protection recommendations (deployment-branch policy, required reviewers for stage/prod) as a consumer step after pilot soak. Defaults stay permissive so current dev cadence is preserved.
• refactor: remove it-dynamo input and job #4 Verify Lambda VPC attachment step after each deploy asserts via aws lambda get-function-configuration that the applied VpcConfig.SubnetIds / SecurityGroupIds match expected values.
• chore: remove unused S3 artifact upload job #5 Declared secrets: block on the workflow_call signature (all required: false because activation is gated by the input).
• chore: Configure Renovate #6 Per-deploy-job permissions: { contents: read, id-token: write }.
• fix: remove pull-requests write from reusable workflow permissions #9 Header comment warning against duplicating AWS_ACCOUNT_ID_* into environment secrets.
• fix: add pull-requests read to branch-deploy permissions #10 docs/vpc-config.md includes rollback (flip the input → helix-deploy detach path, manual recovery fallback) and egress notes (NAT/VPC endpoint dependencies per account).
• fix: use status field instead of ahead_by for behind-main check #11 Caller-side coupling documented; vpc-enabled: true without matching package.json hlx entries fails the verify step immediately.

Deferred to follow-ups (not in this PR):

• feat: thread VPC config env vars into reusable deploy jobs #7 Splitting semantic-release so npm publish / GitHub release run outside the prod environment gate. Agree this is cleaner — treating as a separate refactor so this PR stays focused on VPC.
• feat: skip DEV deploy when branch is behind main #8 SHA-pinning shared actions — will open a Renovate-config follow-up covering third-party and same-org @v* refs together.
• fix(ci): fix inverted branch-behind-main comparison direction #12 Migrate VPC identifiers from secrets to vars.
• hotfix(ci): disable branch-behind-main check until proper fix ships #13 Generalize from VPC_SUBNET_1/2 to a single comma-separated VPC_SUBNET_IDS (unblocks 3-AZ).
• feat(service-ci): opt-in npm OIDC Trusted Publishers #14–17 Log-on-deploy, CHANGELOG, additional in-file pointers.

On the v1 tag move: I'd like to keep v1 where it is and have the pilot (spacecat-jobs-dispatcher#694) pin to the merge commit (or a fresh v1.next) until soak completes, then retag deliberately. Flagging as open for your preference — I'm happy to ship a v2 tag instead if you'd rather the opt-in require a version bump as well.

PTAL when you have a moment.

[alinarublea]

Decision: shipping as a new v2 tag rather than moving v1.

• v1 stays at its current commit, unchanged. Existing consumers are unaffected.
• After this PR merges, I'll cut v2 / v2.0.0 at the merge commit.
• Consumers that want VPC attachment pin @v2 AND set vpc-enabled: true. Consumers that don't need VPC can stay on @v1 indefinitely.

Pushed 5ae8cbd updating docs/vpc-config.md so the consumer example pins @v2.

Internal action refs inside service-ci.yaml (e.g. adobe/mysticat-ci/.github/actions/setup-node-npm@v1) stay at @v1 — they're unchanged by this PR and are covered separately by the SHA-pinning follow-up (your #8).

[solaris007]

Hey @alinarublea,

Thanks for the thorough rework. The opt-in default-false design is the right shape, and the new validate + verify steps + docs/vpc-config.md meaningfully lower operational risk for pilot adoption. Prior Critical findings 1-4 and Important findings 5, 6, 9, 10, 11 are all genuinely resolved. Also noticed commit 5ae8cbd came in while reviewers were working - pinning docs to @v2 and committing to the v2 tag strategy, which closes a documentation gap one reviewer raised.

Because vpc-enabled defaults to false and every new behavior is gated on it, merging this is byte-identical for existing @v1 consumers. The remaining items below are pilot-readiness polish, not merge-blockers for non-opted-in callers. Still worth fixing before the jobs-dispatcher pilot flips to vpc-enabled: true.

Strengths (Previously flagged, now addressed)

• Critical 1 (v1 tag-move blast radius): resolved via vpc-enabled: false default plus the new v2 tag strategy. Existing callers pinned to @v1 behave identically.
• Critical 2 (silent VPC_ fall-through)*: Validate VPC config present step in all three deploy jobs fails loud if any of the three secrets are empty.
• Critical 3 (branch-deploy env semantics): environment: label only applied when opted in. Docs explicitly recommend protection tiers (dev optional, stage=main-only, prod=main + required reviewer) after pilot soak.
• Critical 4 (no post-deploy VPC verification): Verify Lambda VPC attachment step on all three deploy jobs using aws lambda get-function-configuration + jq -e. Also catches the "consumer forgot awsVpcSubnetIds in package.json hlx" regression, since a missing attachment produces empty VpcConfig.SubnetIds which fails the match.
• Important 5, 6, 9, 10, 11: explicit secrets: block on workflow_call, per-job permissions: { contents: read, id-token: write }, AWS_ACCOUNT_ID_* shadowing warning in the workflow header and docs, rollback and egress sections in docs/vpc-config.md, 4-step consumer migration documented.
• Safety of the ternary idiom: environment: ${{ inputs.vpc-enabled && 'prod' || '' }} is a known GH Actions pattern that treats empty string as "no environment." Not formally spec'd but stable on github.com-hosted runners.
• Shell-injection surface on Verify: FUNC is interpolated into an env var value, not into the shell script body; the script reads it as quoted "$FUNC" and passes to aws CLI as a single argument. Safe.

Issues

Important (Should Fix before pilot flips vpc-enabled: true)

1. Hardcoded Lambda function name spacecat-services--${{ inputs.service-name }}.
service-ci.yaml:203, 256, 311 (the three Verify steps). mysticat-ci is a platform artifact positioned for any Mysticat/SpaceCat service; services whose helix-deploy hlx.name diverges from spacecat-services--<service-name> (mystique-*, custom renames, future non-spacecat consumers) will get ResourceNotFoundException from verify. The deploy itself will have succeeded, so you get red CI on a good deploy - confusing.
Fix: add a lambda-function-name workflow input defaulting to spacecat-services--${{ inputs.service-name }}, reference the input in the three Verify steps, document the override path in vpc-config.md.

2. Validate step error does not name which secret is missing.
service-ci.yaml:185, 238, 291. The current error "VPC_SUBNET_1/VPC_SUBNET_2/VPC_SG_ID are not all set in the 'prod' environment" forces the operator to guess which of the three is empty, and gives no hint that the cause may be a missing secrets: inherit on the caller. Low-cost improvement:

missing=()
[ -z "$VPC_SUBNET_1" ] && missing+=("VPC_SUBNET_1")
[ -z "$VPC_SUBNET_2" ] && missing+=("VPC_SUBNET_2")
[ -z "$VPC_SG_ID" ]    && missing+=("VPC_SG_ID")
if [ ${#missing[@]} -gt 0 ]; then
  echo "::error::Missing in 'prod' environment: ${missing[*]}. Check environment secrets and that the caller passes 'secrets: inherit'."
  exit 1
fi

3. Verify step does not set AWS_REGION explicitly and gives poor diagnostics on AWS CLI failure.
service-ci.yaml:200-215, 253-268, 311-323. The aws lambda get-function-configuration call relies on AWS_REGION being set by the configure-aws@v1 composite as a side-effect. The three Deploy steps above all set AWS_REGION: us-east-1 explicitly for defense-in-depth; Verify should do the same. Additionally, when AWS CLI fails (role lacks lambda:GetFunctionConfiguration, function not found, transient outage), GitHub Actions exits the script via set -e (which is on by default), but the operator sees "Process completed with exit code 1" with the aws CLI stderr possibly scrolled off. Add explicit region + capture the CLI failure for a tailored error:

set -euo pipefail
if ! VPC=$(aws lambda get-function-configuration --function-name "$FUNC" --query 'VpcConfig' --output json 2>&1); then
  echo "::error::aws lambda get-function-configuration failed for $FUNC: $VPC (check that role has lambda:GetFunctionConfiguration and that $FUNC exists)"
  exit 1
fi

And add AWS_REGION: us-east-1 to the Verify step's env block.

4. Pre-flight: confirm lambda:GetFunctionConfiguration is granted on spacecat-role-github-actions in all three AWS accounts.
Not a code change - an ops check. If the role's current policy only covers helix-deploy's update-side permissions, the first pilot Verify run fails with AccessDenied. Confirm with spacecat-infrastructure owner before the pilot flips. If missing, the Terraform change is a few lines but has its own deploy cycle to coordinate.

5. Pre-flight: dry-run the rollback path on the pilot.
docs/vpc-config.md rollback step 1 states helix-deploy interprets awsVpcSubnetIds: ["", ""] as detach. This is an untested claim about helix-deploy#898 behavior. If helix-deploy passes empty strings through to AWS, Lambda rejects with InvalidParameterValue and the rollback path is broken in exactly the moment you need it most. Dry-run the round-trip (vpc-enabled: true -> false -> true) on the pilot service after initial opt-in succeeds, and update the doc to match observed behavior.

Minor (Nice to Have)

6. build and it-postgres jobs have no permissions: block. Pre-existing defense-in-depth gap; now that deploy jobs are explicitly scoped, the inconsistency stands out. Follow-up, not this PR: build: permissions: { contents: read }, it-postgres: permissions: { contents: read, id-token: write }.

7. Validate runs after Configure AWS for *. OIDC token minted before validate fails. Move Validate earlier to fail faster and avoid the wasted exchange.

8. Verify + Validate duplicated across 3 deploy jobs. About 80 lines of near-identical YAML+shell. Strong candidate for a composite action (adobe/mysticat-ci/.github/actions/vpc-validate-verify@v1) once behavior stabilizes after pilot soak.

9. One-line comment above the ternary idiom. environment: ${{ inputs.vpc-enabled && 'prod' || '' }} is correct but non-obvious; a comment like "empty string -> no environment (standard GHA conditional-env idiom)" prevents a future reader from 'fixing' it.

10. Verify uses subset-match, not equality. index($s1) and index($s2) and index($sg) passes if extra subnets or SGs are attached. Fine for pilot. If strict parity is wanted later, switch to (.SubnetIds | sort) == ([$s1,$s2] | sort).

11. Validate's non-empty check could also sanity-check format. Catching trailing whitespace, placeholder strings ("TODO", "<missing>"), or swapped IDs is realistic for human-populated secrets across 11+ repos: [[ "$VPC_SUBNET_1" =~ ^subnet-[0-9a-f]+$ ]] etc. Opinionated, defer if you prefer.

12. Verify step has no alerting on prod failure. Red CI badge is the only signal when a prod deploy succeeds but Verify catches drift. Follow-up: post to Slack (SpaceCat alerts channel) when failure() && inputs.vpc-enabled && github.ref == 'refs/heads/main'. Auto-rollback is more work but worth tracking.

13. Egress doc does not confirm NAT exists in all 3 accounts. docs/vpc-config.md points at spacecat-infrastructure/modules/vpc/main.tf for NAT but leaves it as reader-exercise. Explicit "NAT is already provisioned in spacecat-dev/stage/prod as of " tightens the doc.

14. Deferred items 7 (semantic-release publish + deploy under one env gate) and 8 (SHA-pin third-party actions). Fine to defer. The opt-in model isolates 7's blast radius to pilot callers for now. 8 is CI hygiene orthogonal to this PR. File both as follow-up issues so they don't drop.

Recommendations

1. Fix Important 1, 2, 3 in this PR - small, mechanical, all in the new code path.
2. Treat Important 4 (IAM pre-flight) and 5 (rollback dry-run) as checklist items before the pilot flips vpc-enabled: true. Neither is a code change.
3. File follow-up issues for 6, 8, 12, and deferred 7/8. Track in a rollout milestone.
4. Confirm on a throwaway branch that environment: '' with vpc-enabled: false does not produce a phantom deployment in the GH UI before merge. If it does, the fallback is splitting into two jobs or using fromJSON.

Assessment

Ready to merge? With fixes.

Reasoning: The default-off gating makes the merge itself byte-safe for existing callers - even landing this as-is does not regress any of the 20+ consumers. The Important items above are pilot-readiness polish that should land before the first consumer sets vpc-enabled: true. Substantive progress on the prior review: all four Criticals and five of seven Importants fully resolved.

Next Steps

1. Land the three small code fixes (Lambda name input, Validate error, Verify AWS_REGION + CLI diagnostic) in this PR.
2. Run the two pre-flight checks (IAM grant, rollback dry-run) before cutting over the jobs-dispatcher pilot.
3. File follow-up issues for the remaining deferred and minor items so they don't drop.
4. Smoke-test environment: '' behavior on a throwaway branch before merge.

[solaris007]

Important (finding 1): spacecat-services--${{ inputs.service-name }} bakes helix-deploy's naming convention into the platform workflow. Consumers whose hlx.name diverges (mystique-*, custom renames) get ResourceNotFoundException here on a successful deploy. Add a lambda-function-name input defaulting to this value so odd-one-out callers can override.

[solaris007]

Important (finding 2): this error forces the operator to guess which secret is missing. Accumulate the list of empty names and print them explicitly, plus a hint about secrets: inherit (see review body for the snippet).

[solaris007]

Important (finding 3): two improvements: (a) add AWS_REGION: us-east-1 to this step's env block (the three Deploy steps above all set it explicitly; relying on configure-aws side-effect is fragile). (b) capture aws CLI stderr and emit a tailored ::error:: for common failure modes (AccessDenied, ResourceNotFoundException) so the operator isn't misled by the generic 'VpcConfig does not match' further down.

[alinarublea]

Self-re-reviewed acting as staff eng + senior SRE. Pushed 70da88b addressing six findings:

Correctness

1. Verify raced with async AWS attach. AWS Lambda VPC updates take 60–90s for ENI allocation; the verify step could read transitional state. Added aws lambda wait function-updated before get-function-configuration.

2. Inverse trap: vpc-enabled: false + VPC refs already in package.json would export empty VPC_* env vars → helix-deploy resolves ["",""] → silent VPC detach. The one-sided "Validate" step is now a 2×2 sanity check that runs always (not gated on vpc-enabled) and fails fast in either direction:

   vpc-enabled	awsVpcSubnetIds in pkg.json	Result
   true	yes	proceed after secret check
   true	no	fail — opt-in without hlx fields is a no-op
   false	yes	fail — would silently detach VPC
   false	no	proceed (non-VPC deploy)

3. Shell robustness. set -euo pipefail on every multi-line run: block.

4. jq truthiness. index($x) returns 0 on first element (truthy in jq, unlike shell). Switched to explicit != null for clarity / refactor-safety.

Operability
5. Function-name hardcoded. Added optional lambda-function-name input; defaults to spacecat-services--<service-name>.
6. Log applied VPC info. Echo subnet/SG count on deploy (values stay redacted) so incident responders see the config in CI logs.

Not in this PR (deferred, tracked via solaris007's review)

• feat: thread VPC config env vars into reusable deploy jobs #7 split semantic-release from npm publish / GitHub release
• feat: skip DEV deploy when branch is behind main #8 SHA-pin shared actions
• fix(ci): fix inverted branch-behind-main comparison direction #12 move VPC identifiers from secrets → vars
• hotfix(ci): disable branch-behind-main check until proper fix ships #13 support >2 subnets via comma-separated VPC_SUBNET_IDS

Docs (docs/vpc-config.md) updated with the guardrail table and verify-step behavior. Ready for another look.

[alinarublea]

Went back and fixed #3 properly (6151070) instead of the doc-only workaround.

Change: branch-deploy now uses a dedicated dev-branches environment instead of dev. This decouples feature-branch cadence from any future protection rules on dev:

• dev-branches — feature-branch deploys, stays policy-free by design. Who-can-push is gated via git-side branch protection on the consumer repo, not env protection.
• stage — deployment-branch policy restricted to main (recommended).
• prod — deployment-branch policy restricted to main + required reviewer (recommended).

Migration: I've already created the dev-branches environment on all 12 VPC-PR repos (pilot + 11 others) and populated VPC_SUBNET_1/VPC_SUBNET_2/VPC_SG_ID with the dev-account Terraform values. The previously-created dev environment is now unused; leaving it in place as a no-op (happy to prune if you'd rather).

Docs (§ 1 Create GitHub Environments, § 2 Populate secrets, § Environment protection) updated to reflect the split.

Updated status across your 17 findings:

• All Critical (test: smoke test workflow syntax #1–4): addressed in code
• Important chore: remove unused S3 artifact upload job #5, chore: Configure Renovate #6, fix: remove pull-requests write from reusable workflow permissions #9, fix: add pull-requests read to branch-deploy permissions #10, fix: use status field instead of ahead_by for behind-main check #11: addressed
• Deferred to follow-ups: feat: thread VPC config env vars into reusable deploy jobs #7 (split semantic-release), feat: skip DEV deploy when branch is behind main #8 (SHA-pin), fix(ci): fix inverted branch-behind-main comparison direction #12 (secrets → vars), hotfix(ci): disable branch-behind-main check until proper fix ships #13 (N subnets)
• Not done: fix(it-postgres): set IT-only env vars at process level for module-scope initializers #15 (CHANGELOG), feat(service-ci): add opt-in bundle-build Lambda smoke check #17 (HLX_AWS_REGION → non-secret) — happy to fold into this PR or ship separately, your call

Re-requesting review. PTAL.

[solaris007]

Hey @alinarublea,

Round 3. Big improvements: aws lambda wait function-updated closes the eventual-consistency race, the unconditional VPC config sanity check with inverse-trap + forgot-hlx coverage is a strong correctness addition, lambda-function-name breaks the hardcoded Lambda-name coupling, dev-branches cleanly decouples feature-branch cadence from any future dev protection, and the docs truth table makes the 4-way contract unambiguous. Reviewers converged on "merge" with a handful of small pilot-readiness nits still worth fixing in this PR.

Strengths (Previously flagged, now resolved)

• Eventual-consistency race closed: aws lambda wait function-updated before the verify read (service-ci.yaml:235, 319, 405). Right tool over a fixed sleep.
• lambda-function-name input: service-ci.yaml:40-45 + ternary-with-format() at :230, 314, 400. Non-spacecat-services consumers have an escape hatch. Idiomatic GH Actions expression.
• Unconditional sanity check with inverse and forgot-hlx traps (service-ci.yaml:188-215, 274-301, 360-387). Catches the worst regression modes before deploy: opt-in without hlx fields no-ops; opt-out with hlx fields silently detaches. Docs truth table aligns with the code.
• set -euo pipefail in all shell blocks: shell-safety floor across the new steps.
• jq index($X) != null: explicit null check, correct array-element-match semantics (the old index() implicit-truthy worked because 0 is truthy in jq, but explicit is better).
• environment: 'dev-branches' (service-ci.yaml:334) + rationale comment + docs update. Future-proof against later dev env protection rules.
• Docs Guardrails section with the 4-way truth table, environment table in step 1, and dev-branches rationale. Durable shape.

Issues

Important (Should Fix in this PR - small edits)

1. Docs reference environment: dev (should be dev-branches).
docs/vpc-config.md:77. "branch-deploy will run under environment: dev, validate..." - but the workflow uses dev-branches. First adopter reading step 4 in isolation will create a dev environment, then find their push auto-creating dev-branches with no protection and no secrets. One-word fix.

2. Verify step has no explicit AWS_REGION.
service-ci.yaml:237, 321, 407. The three Deploy steps set AWS_REGION: us-east-1 inline. The Verify step relies on adobe/mysticat-ci/.github/actions/configure-aws@v1 having exported it via $GITHUB_ENV earlier in the job. If that composite ever changes (it's pinned to mutable @v1), aws lambda ... falls back to the SDK region resolution chain and you get an unrelated error or silent success against the wrong region. Three-line fix to each Verify step's env block.

3. Sanity check probes only awsVpcSubnetIds, not awsVpcSecurityGroupIds.
service-ci.yaml:193, 279, 365. A package.json that declares subnets but forgets the SG field passes the sanity check and deploys with partial VPC config. Verify catches it post-deploy, but only after the Lambda is already in a degraded state. Also, "awsVpcSubnetIds": [] (explicit empty) currently passes grep and lets helix-deploy deploy with empty arrays. Tighter check:

if [ -f package.json ] \
   && jq -e '.hlx.awsVpcSubnetIds | arrays | length > 0' package.json >/dev/null 2>&1 \
   && jq -e '.hlx.awsVpcSecurityGroupIds | arrays | length > 0' package.json >/dev/null 2>&1; then
  HAS_VPC_HLX=1
fi

4. Sanity-check "missing secrets" error doesn't name which secret.
service-ci.yaml:198, 284, 370. Still says "VPC_SUBNET_1/VPC_SUBNET_2/VPC_SG_ID are not all set." During pilot onboarding this will fire repeatedly; naming the missing field saves a manual check each time:

MISSING=()
[ -z "$VPC_SUBNET_1" ] && MISSING+=("VPC_SUBNET_1")
[ -z "$VPC_SUBNET_2" ] && MISSING+=("VPC_SUBNET_2")
[ -z "$VPC_SG_ID" ]    && MISSING+=("VPC_SG_ID")
if [ ${#MISSING[@]} -gt 0 ]; then
  echo "::error::vpc-enabled=true but missing in '<env>' environment: ${MISSING[*]}"
  exit 1
fi

Minor (Follow-ups)

5. Verify step should log the aws CLI failure directly.
service-ci.yaml:237, 321, 407. set -euo pipefail means the script exits on aws lambda get-function-configuration failure, but the step just shows "Process completed with exit code 1" with the CLI stderr possibly scrolled off. Capturing stderr and emitting a tailored ::error:: (naming the likely cause: AccessDenied vs ResourceNotFoundException) speeds operator response. Same for the aws lambda wait function-updated timeout - a 5-minute ENI provisioning wait that hits the default timeout will look like a generic waiter timeout; a wrapper or ::notice:: before the wait makes that failure mode visible.

6. Hardcoded SUBNETS=2 / SGS=1 log line is misleading (service-ci.yaml:204-206, 290-292, 376-378). Reads as if computed, actually literal. An operator trusting the log during an incident sees "2 subnets, 1 security group(s)" regardless of what was applied. Either drop the line or echo the actual IDs (non-secret): echo "Applying VPC config: SUBNETS=[$VPC_SUBNET_1, $VPC_SUBNET_2] SG=$VPC_SG_ID".

7. aws lambda wait function-updated default timeout = 5 min. ENI provisioning under Hyperplane pressure can push past 3 min. Not a regression, but documenting the ceiling (or overriding with --cli-read-timeout + a timeout 420 wrapper) reduces pager-fatigue on slow ENI days.

8. Pre-flight items still offline. Need to happen before jobs-dispatcher flips vpc-enabled: true, not visible from code:

• Confirm spacecat-role-github-actions has lambda:GetFunctionConfiguration (plus the ec2:Describe* suite Lambda itself needs for VPC attach) in dev/stage/prod
• Rollback dry-run (flip vpc-enabled: true -> false on the pilot and confirm detach works as documented)
• NAT / VPC endpoints confirmed in all three accounts
  Expand the PR's post-merge checklist to include these.

9. Validate runs after Configure AWS. Wasted OIDC exchange on sanity-check failure. Swap ordering; 5-minute move.

10. Code duplication across 3 deploy jobs is now ~120 lines. sanity check + verify blocks are essentially identical. Strong candidate for adobe/mysticat-ci/.github/actions/vpc-guardrails@v2 composite before broad v2 rollout (not before merge). Flag so it doesn't drop.

11. v1 deprecation plan. Docs say "consumers that do not need VPC attachment can stay on @v1 indefinitely." Non-VPC callers on v1 do not get inverse-trap protection - which is one of the most valuable new behaviors for the broader fleet. After pilot soak, open a tracking issue to move non-VPC callers to v2 under a platform-owned sweep. Assigns an owner and a sunset date.

12. semantic-release env-gate still overloads three responsibilities. environment: prod on the semantic-release job gates npm publish + GitHub release + Lambda deploy behind a single approval. Becomes a blocker the moment prod: required reviewer is adopted per the docs recommendation. Track; must precede the env-protection rollout.

13. No alerting on verify-fail in prod. Red CI on Friday at 18:00 gets noticed Monday. At 20+ services this is material - add Slack (SpaceCat alerts channel) or Coralogix alert on vpc-enabled && failure() for stage/prod. Follow-up.

14. Supply-chain: third-party and internal actions still on moving tags. Pre-existing; track in a cross-org sweep not in this PR.

15. build and it-postgres lack explicit permissions: blocks. Pre-existing; fold into the hardening sweep.

16. wait function-updated vs wait function-active. function-updated returns immediately if the function was just CREATED (LastUpdateStatus defaults to Successful). Non-issue for existing spacecat services, but a one-line note in docs/vpc-config.md under "known waiter behavior" helps future brand-new-Lambda adopters.

Recommendations

1. Land Important 1-4 in this PR - four small inline edits, high pilot-readiness payoff.
2. Pre-flight items 8 must happen before flipping vpc-enabled: true on jobs-dispatcher. Pin the checklist to the PR description or a pilot-runbook doc.
3. File follow-up issues for Important 10 (composite action), 11 (v1 deprecation), 12 (semantic-release split), 13 (alerting) so none drop. Each has an implicit deadline tied to "before broad v2 rollout" or "before prod gains required-reviewer rule."
4. Pilot a VPC-enabled AND a non-VPC v2 caller early. The non-VPC case exercises the inverse-trap coverage, which is arguably the most valuable new behavior for the broader fleet.

Assessment

Ready to merge? With fixes.

Reasoning: Three iterations have converged on a coherent, defensively-sound design. Core correctness items (race fix, inverse trap, naming override, verify logic, dedicated env) are in and correct. Remaining items are four tiny inline edits (1 word of docs, 3 lines of YAML, one small bash refactor, one jq probe) plus a solid set of follow-ups to track. Three of five reviewers are at "Yes"; two ask for these small inline fixes before pilot. Default-off gating keeps the merge itself byte-safe for existing consumers.

Next Steps

1. Land Important 1-4 inline (4 small edits).
2. Complete pre-flight checks 8 before the pilot flips the input.
3. Open follow-up issues for 10-15 so they don't drop before broad rollout.
4. Run the pilot with both a VPC and non-VPC consumer.

[solaris007]

Important (finding 1): This says environment: dev but the workflow uses environment: dev-branches on branch-deploy. First-time adopters reading this paragraph in isolation will create the wrong environment name. One-word fix.

[solaris007]

Important (finding 2): add AWS_REGION: us-east-1 to this step's env block (and the deploy-stage + branch-deploy verify steps at lines 321 and 407). The three Deploy steps set it explicitly; relying on configure-aws@v1 to export it as a side effect is fragile given the composite is pinned to a mutable @v1 tag.

[solaris007]

Important (finding 3): this only probes awsVpcSubnetIds. A package.json with subnets but no awsVpcSecurityGroupIds passes the sanity check and deploys with partial VPC config; verify catches it only after the Lambda is degraded. Also "awsVpcSubnetIds": [] passes grep. Switch to a jq check of both fields with non-empty arrays (see review body for the snippet). Apply the same change at the duplicates on lines 279 and 365.

[solaris007]

Important (finding 4): error doesn't name which secret is missing. During pilot onboarding this will fire repeatedly. Accumulate the empty names and print them (see review body for the snippet). Apply to the deploy-stage and branch-deploy duplicates at lines 284 and 370.

[alinarublea]

Thanks @solaris007 — pushed c42750e for round-3 Important 1-4:

1. Doc mismatch fixed: docs/vpc-config.md now says environment: dev-branches in step 4.
2. Verify step AWS_REGION: explicit AWS_REGION: us-east-1 added to the env block of all three Verify steps, so verify is no longer coupled to whatever the configure-aws composite exports.
3. Sanity check: jq probes for both fields, non-empty arrays. Replaced the grep-based check with:

   if [ -f package.json ] \
      && jq -e '.hlx.awsVpcSubnetIds | arrays | length > 0' package.json >/dev/null 2>&1 \
      && jq -e '.hlx.awsVpcSecurityGroupIds | arrays | length > 0' package.json >/dev/null 2>&1; then
     HAS_VPC_HLX=1
   fi

   Partial configs (subnets only, empty arrays) now trip the guard pre-deploy.
4. Named missing secrets: error message now lists which secrets are unset.

Also dropped the hardcoded SUBNETS=2 / SGS=1 log line (misleading as noted) and replaced with Applying VPC config: SUBNETS=[$V1,$V2] SG=$SG (GH masks the actual values since they're secrets; intent is clearer).

Follow-ups tracked separately (will file issues after merge): #5 (CLI stderr tailoring), #6 (superseded by the real-ID echo), #7 (wait-function-updated ceiling), #8 (pre-flight IAM / rollback dry-run / NAT+endpoints), #9 (Configure-AWS-before-sanity-check ordering), #10 (composite action extraction), #11 (v1 deprecation sweep), #12 (semantic-release split), #13 (prod verify-fail alerting), #14 (SHA-pin shared actions), #15 (build/it-postgres permissions), #16 (wait-function-updated note for brand-new Lambdas).

In flight: pilot run on spacecat-jobs-dispatcher#694 pinned at c42750e + vpc-enabled: true to validate end-to-end before merge.

[solaris007]

Hey @alinarublea,

All four inline items from round 3 landed cleanly:

• docs/vpc-config.md:77 - environment: dev -> environment: dev-branches. Typo fixed.
• service-ci.yaml:233, 322, 413 - AWS_REGION: us-east-1 explicit on all three Verify step env blocks. No more hidden coupling to configure-aws@v1.
• service-ci.yaml:193-195, 284-286, 375-377 - jq probe replaces the grep. Checks .hlx.awsVpcSubnetIds | arrays | length > 0 AND .hlx.awsVpcSecurityGroupIds | arrays | length > 0. The arrays filter also closes the "awsVpcSubnetIds": [] corner case I flagged.
• service-ci.yaml:198-205, 289-296, 380-387 - missing-secret error now names the specific empty fields via a MISSING accumulator. Onboarding pilot will save real time on this.
• Bonus: the hardcoded SUBNETS=2/SGS=1 log line is now Applying VPC config: SUBNETS=[...] SG=... with the real IDs. Also addresses round-3 minor 6.
• Inverse trap and forgot-hlx error messages tightened to reference both awsVpcSubnetIds/awsVpcSecurityGroupIds.

Four iterations have converged on a defensively-sound design. Opt-in default-off, explicit guardrails, post-deploy verification with eventual-consistency handling, configurable Lambda name, dedicated dev-branches env, full docs with truth table. Merge-safe for existing consumers; ready for pilot adoption on a v2 tag.

Follow-ups to track (not blocking)

These were flagged across rounds 1-3 as deferred and remain open:

1. Composite action to collapse the ~120 lines of duplicated sanity-check + verify logic across the three deploy jobs. Worth landing before broad v2 rollout.
2. Pre-pilot ops checks: confirm lambda:GetFunctionConfiguration (plus ec2:Describe* suite Lambda needs for VPC attach) granted to spacecat-role-github-actions in dev/stage/prod; rollback dry-run on the pilot service; NAT/VPC endpoint coverage in all three accounts.
3. Slack alerting on verify-fail in stage/prod (Coralogix or Slack channel) before more than 2-3 services opt in.
4. v1 deprecation plan with owner and sunset date - non-VPC callers on v1 don't get inverse-trap protection.
5. semantic-release env-gate split (publish + GitHub release ungated, prod deploy gated) - becomes a blocker the moment any consumer adds a required-reviewer rule on prod.
6. Supply-chain: SHA-pin third-party and internal @v1/@v2 actions. Track in a cross-org sweep.
7. build/it-postgres jobs lack explicit permissions: blocks. Fold into the hardening sweep.
8. Wait-timeout handling on aws lambda wait function-updated (5 min default may be tight on slow ENI provisioning days).
9. Sanity-check step ordering (currently runs after Configure AWS for *, wastes an OIDC exchange on failure).
10. One-line note in docs/vpc-config.md about wait function-updated returning immediately on brand-new functions (use wait function-active for first-ever deploy).

Assessment

Ready to merge? Yes.

Reasoning: Four iterations, all flagged correctness items resolved. The remaining items are explicit follow-ups (most of which are pre-pilot ops checks or post-pilot hardening), not defects in what ships. Default-off opt-in keeps the merge byte-safe for existing consumers; v2 adopters get a defensively-sound contract.