[IAmJSD]

Root cause

terraform/environments/aws/ci-cd-permissions/outputs.tf builds policy_arns by hand. Its description says it holds the ARNs of all attached managed policies, but nothing tied it to the attachments in role.tf. #1094 later added monitoring to both, but compute_b and iam were never listed. On current main, role.tf attaches six policies (networking, compute, compute_b, data, iam, monitoring) and the output lists four. The issue says "three of five" because it was filed before monitoring was added.

Anyone who reviews or detaches the role's policies from terraform output -json policy_arns misses compute_b and iam, which are the policies that carry the #1705 boundary-delegation half of the role.

Fix

• outputs.tf: add compute_b and iam, so the map has one entry per aws_iam_role_policy_attachment, keyed by attachment name. The description now says which role and how the map is keyed. A comment explains why workload_boundary is left out: it is the permissions boundary for roles the deploy role creates, not a policy attached to the deploy role.
• outputs_guard_test.go (new, same cicdpermissions_test package and helpers as policy_guard_test.go): reads every aws_iam_role_policy_attachment from this root's *.tf files, comment lines stripped, and requires policy_arns to match exactly in both directions: every attachment has an entry with the same key and the same policy_arn expression, and every entry matches an attachment. It fails closed if the scan finds fewer than 6 attachments, or if an attachment or map entry has a shape it can't read.
• README.md: add terraform output -json policy_arns to the outputs section. I found no workflow, script or doc that reads policy_arns (git grep policy_arns only matches the audit report and outputs.tf), so nothing downstream changes.

Why not derive the map from the attachments? Terraform can't list every resource of one type. The only real derivation would merge the six attachments into one for_each resource, which changes their state addresses (aws_iam_role_policy_attachment.networking would become ...deploy["networking"]) in a root that a human applies by hand. That needs moved blocks and a carefully reviewed apply, which is too much for a p3/xs output fix. The list stays explicit, and the Go guard does the drift check instead.

The new check is a Go test rather than a terraform test. Adding a tests/ dir would turn on the "Test CI/CD trust" CI step for AWS (init + test with mocked providers) and widen this change. The Go test runs in the existing unit-test job with no provider download, and .terraform.lock.hcl is untouched.

Closes #309

Verification

All local on macOS (darwin_arm64). This is static source evidence plus terraform validate. Nothing ran plan/apply and no cloud credentials were used (AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null throughout). Gap: I did not compare the output against a real applied state (terraform output -json policy_arns against the live root).

Fail before (test added, outputs.tf as on main):

$ go test -count=1 -run TestPolicyARNsOutputListsEveryAttachedPolicy ./terraform/environments/aws/ci-cd-permissions/
--- FAIL: TestPolicyARNsOutputListsEveryAttachedPolicy (0.00s)
    outputs_guard_test.go:141: aws_iam_role_policy_attachment.compute_b attaches aws_iam_policy.compute_b.arn to the deploy role, but outputs.tf's policy_arns output has no "compute_b" entry; operators enumerating the output would miss it (#309)
    outputs_guard_test.go:141: aws_iam_role_policy_attachment.iam attaches aws_iam_policy.iam.arn to the deploy role, but outputs.tf's policy_arns output has no "iam" entry; operators enumerating the output would miss it (#309)
FAIL

Pass after: go test -count=1 ./terraform/environments/aws/ci-cd-permissions/ passes (the whole package, existing guards included).

Mutation probes, applied one at a time to the fixed tree and then reverted. Each one fails the test with a specific message:

Mutation	Result
drop the iam entry	FAIL: no "iam" entry
add workload_boundary = aws_iam_policy.workload_boundary.arn	FAIL: listed but not attached
compute_b = aws_iam_policy.compute.arn (wrong value)	FAIL: value mismatch
new aws_iam_role_policy_attachment.extra in role.tf with no output entry	FAIL: no "extra" entry
comment out the compute entry	FAIL: no "compute" entry
rename key data to db	FAIL: both directions

Other checks:

• terraform fmt -check -recursive in the module dir: clean
• TF_DATA_DIR=$(mktemp -d)/tfdata terraform init -backend=false -lockfile=readonly -input=false then terraform validate: "Success! The configuration is valid." Lock file unchanged.
• go vet ./terraform/environments/aws/ci-cd-permissions/: clean
• golangci-lint run --allow-parallel-runners ./terraform/environments/aws/ci-cd-permissions/... (v2.10.1): 0 issues
• gocyclo -over 10 -ignore "_test\.go" on the package: silent (the new test's helpers are under 10 too)
• go test -count=1 ./internal/mocks/: ok
• bash scripts/gosec-hook.sh <new test file>: clean
• git-secrets 1.3.0 (--register-aws plus scripts/setup-git-secrets.sh and .gitallowed), run in a fresh git init repo with an isolated HOME, over the three changed files: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL
• markdownlint v0.47.0 on the README: clean

Out of scope

• Turning the attachments into a single for_each resource with moved blocks (see above). This could be a follow-up if the maintainer wants the output to be derived structurally.
• A separate output for workload_boundary (the issue mentions it as optional). It isn't attached to the deploy role, so it doesn't belong in a map described as attached policies. If an inventory output for it is wanted, it should be its own output.

Labels

Could a maintainer mirror #309'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 3 billable files and costs up to $0.75.}

Or wait 59 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: 4170465
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5fa35dab-9a8f-4f95-90c5-7c1468d5c03f

Commits

Reviewing files that changed from the base of the PR and between 4170465 and 2e5acc8.

Files selected for processing: 3

• terraform/environments/aws/ci-cd-permissions/README.md
• terraform/environments/aws/ci-cd-permissions/outputs.tf
• terraform/environments/aws/ci-cd-permissions/outputs_guard_test.go

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

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

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 2e5acc8

Findings (none blocking):

• [non-blocking] terraform/environments/aws/ci-cd-permissions/outputs_guard_test.go:30-34,60-71 - The guard counts every aws_iam_role_policy_attachment in the root and never checks that its role = is aws_iam_role.cudly_deploy.
  • An attachment to another role, listed in the output, would pass (probe M6), although the description says "attached to the deploy role".
  • Today the root has one role (role.tf:3, cudly_deploy), and all six attachments target it.
• [non-blocking, cosmetic] outputs.tf:12 - The comment says "in role.tf", but the guard scans every *.tf file.

Against #309:

• role.tf:151-179 declares six attachments: networking, compute, compute_b, data, iam and monitoring.
• Base outputs.tf listed four of them. Head lists all six, with matching expressions.
• Leaving workload_boundary out is correct. policy_boundary.tf:79 is the boundary for roles the deploy role creates, and no attachment references it.
• Not using for_each is justified, since it would change state addresses in a root that is applied by hand.
• The README addition is accurate.

Blast radius:

• git grep policy_arns on origin/main (terraform, .github and scripts) finds only outputs.tf.
• The change only adds map keys, so no consumer breaks.

Reuse and scope:

• The guard reuses readPolicySource (:441), balancedBraceEnd (:489) and stripCommentLines (:510) from policy_guard_test.go, and duplicates none of them.
• Nothing in it is over-built. The min-count floor of 6 and the fail-closed shape checks are what keep the scan from passing vacuously.

Drift:

• merge-tree --write-tree against origin/main f5ad8c6 is clean.
• ci-cd-permissions is unchanged on main since the base. Only go.mod moved (go 1.26.9 to 1.26.10), so I built and tested the merged tree too.
• No migrations.

Attacked:

• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, the iam entry dropped: FAILS (no "iam" entry, :141).
  • M2, compute_b = aws_iam_policy.compute.arn: FAILS (value mismatch, :145).
  • M3, an unlisted attachment extra added: FAILS.
  • M4, a trailing # note on a map entry: fails closed ("not a plain key = expression line", :135).
  • M5, workload_boundary added to the map: FAILS (listed but not attached, :151).
  • M6, an attachment to another role, listed: passes. This is the gap above.

Verified locally, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• Head:
  • go build ./..., go vet and go test -count=1 on ./terraform/environments/aws/ci-cd-permissions/: ok.
  • gofmt -l: clean.
  • terraform fmt -check -recursive, init -backend=false -lockfile=readonly and validate: pass. No plan or apply.
• Fail-before: on base, TestPolicyARNsOutputListsEveryAttachedPolicy FAILS with exactly two errors, missing compute_b and missing iam.
• Merged tree: build, vet and the package tests pass.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 2e5acc8

Independent adversarial review of #1211 at 2e5acc8 (single commit on origin/main). The fix closes #309: on this head, role.tf:151-179 declares six aws_iam_role_policy_attachment resources (networking, compute, compute_b, data, iam, monitoring) and outputs.tf:17-24 now lists exactly those six, each pointing at the same aws_iam_policy..arn the attachment uses. Leaving workload_boundary out is correct. It is referenced only as the iam:PermissionsBoundary condition value (policy_iam.tf:108), and no attachment puts it on the deploy role.

Findings (none blocking):

• nit terraform/environments/aws/ci-cd-permissions/outputs_guard_test.go:59-75 - the scan counts every aws_iam_role_policy_attachment in the root and does not check role = aws_iam_role.cudly_deploy.name. If someone adds an attachment for a different role, the test demands a policy_arns entry and the message calls it "the deploy role". This fails loud rather than silently, and no such attachment exists today, so no change is needed.
• nit outputs_guard_test.go:43 - mapEntryPattern rejects quoted keys and trailing inline comments. Both shapes fail closed with a "not a plain key = expression line" message (probes below), so this is a format restriction, not a hole.

Blast radius: git grep policy_arns on origin/main (docs excluded) matches only outputs.tf. No workflow, script or module reads the output, so adding keys to the map breaks nothing downstream. No resource addresses change, and nothing needs a plan diff beyond the output value.

Attacked:

• go test -count=1 ./terraform/environments/aws/ci-cd-permissions/ in a scratch export of the head: ok (existing policy guards included). go vet: clean. terraform fmt -check outputs.tf: clean.
• Every aws_iam_policy resource referenced by the new entries exists (policy_compute_b.tf:70, policy_iam.tf:41).
• Mutation probes, each applied to the scratch copy and reverted:
  1. Restore main's map (drop compute_b, iam): FAIL with both missing entries named.
  2. Change compute_b's value to aws_iam_policy.compute.arn: FAIL with a value mismatch.
  3. Quote a key ("iam" = ...): FAIL, fails closed on an unreadable entry.
  4. Add a trailing # note comment to an entry: FAIL, fails closed.
  5. Add an attachment for another role: FAIL, demands an output entry (nit above).
  6. Append a commented-out attachment: passes, which is correct because comment lines are stripped by readPolicySource (policy_guard_test.go:441-448).
• The fail-closed floor (minDeployRoleAttachments = 6) matches the current count, so a scan that stops finding attachments cannot pass trivially.
• Scope: three files (outputs.tf, README.md line 108, the new test). No lock file, backend or workflow changes.

Not verified here: no terraform plan or terraform output against a real applied state.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• The fix takes effect on the next apply. terraform output reads the stored state, so the README's new terraform output -json policy_arns line keeps returning the old four-key map until someone re-applies this hand-applied root (terraform apply, or apply -refresh-only). The PR body says it was not checked against a live state. A short note in the README, or in the deploy-notes for whoever applies next, would stop an auditor relying on the output in the meantime.
• outputs_guard_test.go deployRoleAttachments doesn't check which role an attachment targets. A future aws_iam_role_policy_attachment on a different role in this root would be required in a map whose description says "attached to the deploy role". There's only one role here today, so this is theoretical.

Attacked:

• Completeness: I enumerated every aws_iam_policy (7) and aws_iam_role_policy_attachment (6) in the root. There's no managed_policy_arns and no inline aws_iam_role_policy. workload_boundary is used only as the iam:PermissionsBoundary condition value in policy_iam.tf. It is not the deploy role's own boundary, so leaving it out matches the new description. compute_b and iam resolve to existing resources. Closes fix(iac/aws): ci-cd-permissions policy_arns output lists three of five policies #309 is right: the issue's two missing entries are added.
• Fail-before: with only outputs.tf reverted to origin/main, the new test FAILS with the two missing-entry errors for compute_b and iam. Restored: passes.
• On ./terraform/environments/aws/ci-cd-permissions/: go vet with and without -tags integration; go test -race -short -count=1 ok (whole package, existing guards included); golangci-lint v2.10.1 with and without --build-tags integration: 0 issues; terraform fmt -check: clean. I didn't re-run terraform validate. The diff only adds two map entries that reference existing resources.

Body-stale:

• none

_{Reviewer: rA2. 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 2e5acc86e526742c7d35104fcc892048c58ff684 merged onto main (2514c01a), together with #1203, #1207, #1211, #1214, #1216 in that order (combined tree 995207c1, 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 2 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: 2 approving independent review(s) on 2e5acc86 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 2e5acc86e526742c7d35104fcc892048c58ff684.