[IAmJSD]

Root cause

scripts/generate-federation-iac.go renders the same templates as the server's GET /api/federation/iac, but from its own copy of the template data struct. iacData's doc comment said "Keep in sync with any template variable changes", and nothing enforced that. By the time of this PR, #1710 had added SourceAccountID, CUDlyAPIURL and ContactEmail. One field was still missing: OIDCIssuerHost, which aws-cfn-deploy.sh.tmpl references. So --format bundle still failed for every AWS target with a non-AWS source. Reproduced on main (a14dc8f):

$ go run scripts/generate-federation-iac.go --target aws --source gcp \
    --account-name prod --account-id 123456789012 \
    --oidc-subject-claim 123456789012345678901 --format bundle --output "$T/b.zip"
Error: render internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl: template: iac:36:22: executing "iac" at <.OIDCIssuerHost>: can't evaluate field OIDCIssuerHost in type main.iacData

The existing end-to-end tests didn't catch this. They cover tfvars for every combination but bundle only for Azure targets, and TestGenerator_ShellTemplatesEscapeValues swaps in a probe for aws-cfn-deploy.sh.tmpl precisely because the real template couldn't be rendered.

Fix

1. Field. iacData gains OIDCIssuerHost. populateData derives it from OIDCIssuerURL with the same strings.TrimPrefix(..., "https://") that buildGenericIaCData uses on the server. escapeData walks every string field by reflection, so the new field is shell-escaped in .sh output without any extra code.
2. Structural drift guard (new scripts/generate_federation_iac_drift_test.go):
   • TestIaCData_MatchesServerTemplateData reads iacData and federationIaCData from source with go/parser and requires the same field set.
   • TestIaCData_CoversEveryTemplateField parses every internal/iacfiles/templates/*.tmpl with text/template, walks the parse tree, and fails if a template references a field iacData lacks. This catches a new template field before anyone runs the script. If a template ever uses with or range, the test fails with "extend templateFieldRefs" rather than guessing what dot means.
   • TestIaCData_FieldWalkerSeesKnownFields is a positive control, so a walker that finds nothing can't make the two tests above pass vacuously.
   • TestGenerator_AWSWIFBundleRendersDeployScript runs the real --format bundle for aws←gcp and aws←azure and checks the OIDCIssuerHost= value in cloudformation/deploy-cfn.sh. It only reads the rendered script and never runs it, since running it would call the AWS CLI.
3. Doc comment. It no longer claims an invariant nothing enforced. It now says the script can't import the server's type and names the tests that keep the two in step.
4. Stale test rationales (audit A14-028):
   • TestGenerator_CrossAccountDoesNotRequireSubjectClaim now passes --source-account-id, asserts exit 0, and checks that the target account ID is rendered. Before, it only checked stderr, so it passed whatever the aws→aws path returned.
   • Updated the comments on TestGenerator_AcceptsValidOIDCSubjectClaim and probeDeployScript, which said the tfvars and deploy-script paths could not render.

Why a source-level check instead of sharing one type

The issue body asks not to couple the script's struct to internal/api. Importing internal/api (or moving the type into internal/iacfiles) would make a //go:build ignore dev tool depend on a large server package. The drift tests read both files as source and import neither, so the script stays decoupled while the drift still fails CI. cristim's follow-up comment suggested this pair of checks (field-name comparison plus every-template coverage).

Closes #153

cristim's follow-ups on the issue (from cli#1691)

1. Missing SourceAccountID / full field list. In scope and fixed. SourceAccountID, CUDlyAPIURL and ContactEmail and their flags were already on main via #1710. This PR adds the remaining OIDCIssuerHost, derived as the server does, plus the drift tests above.
2. shellEscape does not escape }. Out of scope and still present on main. internal/api/handler_federation.go shellEscape (and the script's copy) still leave } alone. 8 templates still put {{.ContactEmail}} inside "${CUDLY_CONTACT_EMAIL:-...}": aws-cfn-deploy, aws-cross-account-cfn-deploy, aws-cross-account-cli, aws-wif-cli, azure-wif-cli, azure-wif-deploy, gcp-sa-impersonation-cli, gcp-wif-cli. Only the bicep deploy script was fixed, in fix(iac): keep '}' and JSON metacharacters in the Bicep/ARM contact email intact (#963 follow-ups) #992 (Follow-ups from #963 review: '}' and JSON metacharacters in the Bicep/ARM contact email #991). This is a server-side escaping change, not generator/handler drift, and it touches the server's CLI templates, so it needs its own issue. As the issue comment notes, it can't run commands, because $, `, " and \ are all escaped.
3. Render paths that bypass escaping. Already fixed on main, nothing to do. The bicep azure-wif-bicep-deploy.sh.tmpl render now goes through shellEscapeData (sec(api): shell-escape the Bicep/ARM deploy script render (#958) #963), and the .auto.tfvars render goes through hclEscapeData.

Out of scope / noted

• Running golangci-lint directly on scripts/generate-federation-iac.go reports 11 issues on lines this PR does not touch, e.g. recognise (misspell), gosec G306 on 0o644, revive error-strings. CI doesn't lint this file because it carries //go:build ignore, and none of the findings are on changed lines.

Verification

Run on this branch with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true and Go 1.26.9. This is unit and subprocess evidence: the tests run the compiled script against the repo's real templates. No cloud calls.

Fail before the fix (new tests, script unchanged):

$ (cd scripts && go test -count=1 -v -run 'TestIaCData_|TestGenerator_AWSWIFBundleRendersDeployScript' .)
--- FAIL: TestIaCData_MatchesServerTemplateData        (script lacks "OIDCIssuerHost")
--- FAIL: TestIaCData_CoversEveryTemplateField
        Should be empty, but was [OIDCIssuerHost (used by aws-cfn-deploy.sh.tmpl)]
--- PASS: TestIaCData_FieldWalkerSeesKnownFields
--- FAIL: TestGenerator_AWSWIFBundleRendersDeployScript/gcp
--- FAIL: TestGenerator_AWSWIFBundleRendersDeployScript/azure
        stderr: Error: render internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl: ... can't evaluate field OIDCIssuerHost in type main.iacData
FAIL

Pass after:

$ (cd scripts && go test -count=1 -v -run 'TestIaCData_|TestGenerator_AWSWIFBundleRendersDeployScript|TestGenerator_CrossAccountDoesNotRequireSubjectClaim' .)
--- PASS: TestIaCData_MatchesServerTemplateData
--- PASS: TestIaCData_CoversEveryTemplateField
--- PASS: TestIaCData_FieldWalkerSeesKnownFields
--- PASS: TestGenerator_AWSWIFBundleRendersDeployScript (gcp, azure)
--- PASS: TestGenerator_CrossAccountDoesNotRequireSubjectClaim
ok  	github.com/LeanerCloud/cloud-commitments-platform/scripts
$ (cd scripts && go test -count=1 .)
ok  	github.com/LeanerCloud/cloud-commitments-platform/scripts	7.253s

Mutation probes. Each was applied temporarily and reverted, and each one fails the tests:

Mutation	Caught by
drop the OIDCIssuerHost = TrimPrefix(...) line, keep the field	TestGenerator_AWSWIFBundleRendersDeployScript (gcp, azure)
add {{.NewField}} to gcp-wif.tfvars.tmpl	TestIaCData_CoversEveryTemplateField: NewField (used by gcp-wif.tfvars.tmpl)
add a field to the server's federationIaCData only	TestIaCData_MatchesServerTemplateData
add a {{with .AccountName}} block to a template	TestIaCData_CoversEveryTemplateField: "*parse.WithNode ... extend templateFieldRefs"

Other checks:

• go build ./..., go vet ./..., go vet -tags integration ./..., go vet scripts/generate-federation-iac.go: all clean.
• golangci-lint run --allow-parallel-runners ./scripts/... (v2.10.1): 0 issues.
• gocyclo -over 10 -ignore "_test\.go" scripts/generate-federation-iac.go: no output.
• go test ./internal/mocks/ -count=1: ok.

Combined-tree verification. This branch was tested together with six other queued branches on a local, never-pushed tree: queue/platform @ 77d116c6 = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-47	62feb4f
fix/issue-76	3b8c241
fix/issue-1233	e1638b7
fix/issue-153	8729074
fix/issue-532	0971624
fix/issue-64	fefdc43
fix/issue-54	91827b9

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks (changed packages): golangci-lint v2.10.1 found 0 issues, with and without --build-tags integration, and gocyclo -over 10 is silent on the changed non-test files. These ran at b61a9c30, which differs from 77d116c6 only by one .gitallowed line on fix/issue-1233.
• Integration tests, every changed package, one package at a time (-tags integration -count=1):
  • These packages pass: cmd/cleanup-lambda, internal/analytics, internal/auth, internal/database/postgres/migrations and internal/purchase.
  • internal/api passes, except two tests whose Postgres container didn't start, or whose migrations hit a timeout, under machine load. Rerun alone on the same tree, both pass: TestDeleteManagedPlanByExecutionStatus and TestNotifiedRow_RescheduleDoesNotOverwriteConcurrentCancel.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • Packages were run with a 60-minute timeout (rather than CI's 20) because the VM was shared with other test runs. Some reruns used TESTCONTAINERS_RYUK_DISABLED=true to avoid a testcontainers reaper race under load.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Labels

A maintainer, please mirror the issue's triage labels onto this PR: impact/internal, triaged, priority/p3, severity/low, urgency/eventually, effort/xs, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 56 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 503eb2e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: e7a61494-6971-4418-9cae-fa716647e0f5

Commits

Reviewing files that changed from the base of the PR and between 503eb2e and 54eb80c.

Files selected for processing: 4

• scripts/generate-federation-iac.go
• scripts/generate_federation_iac_drift_test.go
• scripts/generate_federation_iac_escape_test.go
• scripts/generate_federation_iac_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[IAmJSD]

Heads-up on an interaction with a queued branch: fix/issue-324 (IAmJSD fork, PR not yet open) removes the CloudFormation OIDCIssuerHost parameter entirely and derives the host from OIDCIssuerURL (audit A13b-014). If that lands first, this PR's OIDCIssuerHost generator field and the drift-test expectations for it become unnecessary, and this PR will be updated to drop them. If this PR lands first, #324's branch will drop the field from the generator and adjust these drift tests. Either way the combined result is a generator with no OIDCIssuerHost.

[IAmJSD]

Follow-up to my note above: the fix/issue-324 branch is now #1312. Whichever of #1256 and #1312 merges second drops the OIDCIssuerHost generator field (and this PR's drift-test expectations for it).

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 54eb80ce8fa3d443c2f7468f7c99560ae1545766

Blocking:

• none

Non-blocking:

• scripts/generate_federation_iac_drift_test.go:67: templateFieldRefs has cyclomatic complexity 16. It's a test file, so the gocyclo gate on non-test files doesn't apply, but a table of handled node types would read more easily.
• TestIaCData_MatchesServerTemplateData requires exact field-set equality. A future server-only field that no template uses (e.g. a handler-internal one) will force a matching script field. That trade-off is deliberate, and cristim's 2026-08-03 comment on the issue asks for exactly this pair of checks, which overrides the body's earlier "Not doing: reflection test". I'm only noting it.
• TestGenerator_CrossAccountDoesNotRequireSubjectClaim already passes on main once --source-account-id is supplied, so it hardens a test (audit A14-028) rather than proving this fix.

Attacked:

• Compared the derivation with the server: handler_federation.go:405 uses the same strings.TrimPrefix(OIDCIssuerURL, "https://"). Checked that escapeData picks up the new string field by reflection.
• Checked the parse-tree walker. .A.B resolves to Ident[0], else if nests under ElseList, $.X is handled, and with, range and template hit t.Fatalf rather than passing silently. The positive control (FieldWalkerSeesKnownFields) stops the walker from passing vacuously.
• Ran: go build ./..., go vet ./scripts/ (with and without -tags integration), go vet scripts/generate-federation-iac.go, go test -race -short -count=1 ./scripts/ (ok), golangci-lint v2.10.1 on ./scripts/... with and without integration tags (0 issues), gocyclo (non-test files silent), and the secrets scan ("clean; planted key flagged").
• Fail-before: I restored scripts/generate-federation-iac.go from origin/main and kept the new tests. MatchesServerTemplateData, CoversEveryTemplateField ("OIDCIssuerHost (used by aws-cfn-deploy.sh.tmpl)") and both AWSWIFBundleRendersDeployScript subtests ("can't evaluate field OIDCIssuerHost") fail. I restored the branch afterwards.

Body-stale:

• The "Combined-tree verification" section tests fix/issue-153 at 8729074, not the current head 54eb80c. That's historical evidence, not about this head.

_{Reviewer: rB4. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 54eb80ce8fa3d443c2f7468f7c99560ae1545766 merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 54eb80ce, green CI, and the combined-tree verification above. Squash-merged at exactly 54eb80ce8fa3d443c2f7468f7c99560ae1545766.