SKILL.md

name	reviewing-infracode
description	Use when the user asks to "review this branch / PR / diff" and the changes touch infrastructure-as-code — Terraform, CloudFormation, mise tasks, observability configs (Prometheus, YACE, Grafana), or anything under `infra/`. Focuses on maintainability, readability, similarity to sibling stacks, and the quiet-but-expensive Terraform pitfalls that don't show up in `plan`.

Reviewing Infracode

When to use

The user asks for a review of a branch, PR, or diff, AND the changes are infrastructure-as-code: Terraform (*.tf), CloudFormation (*.yaml/*.yml under infra/), mise task definitions (mise*.toml), observability configs (prometheus.yml, yace.yml, alert rules, dashboards), or anything else under an infra/ tree.

This skill complements general code review — for application code (Rust, Go, TypeScript, etc.) use the regular code-review flow. The failure modes here are different: most infracode bugs only manifest at terraform apply time as silent destroys, downtime, or unbounded blast radius.

What to focus on (in order)

1. Will this cause downtime? Resource renames, force-replace attributes, destroy-create on stateful resources, in-place edits that aren't safe under traffic.
2. Did the blast radius grow? New IAM permissions on existing roles, broadened Resource scopes, new principals, cross-account trust changes, new public surfaces.
3. Is it consistent with sibling stacks? Most infra repos contain near-duplicate stacks per service or per region — divergence between them is technical debt and a future incident magnet.
4. Maintainability — structure, tests, docs, copy-paste, single-source-of-truth.
5. Readability — clear naming, ordering, comments where the why is non-obvious.
6. Scope creep — especially "while I was here, I tightened the IAM" or "I added a security improvement". Bundled changes hide risk and dilute the PR's stated purpose.

Discovery order (do this before forming opinions)

# 1. Read the PR description as the author intended it
gh pr view <num> --repo <org>/<repo> --json title,body,headRefName,baseRefName,additions,deletions,changedFiles

# 2. Commit-by-commit narrative — bundled changes show up here
git log --oneline main..<branch>

# 3. Full diff against the base branch
git diff main..<branch>

# 4. Identify sibling stacks (other services / other regions in the same repo)
#    e.g. infra/cts/observability/ has a sibling at infra/zerokms/observability/
ls infra/

# 5. Diff the changed files directly against the sibling
diff <(sed -n '/<section>/,$p' infra/<sibling>/.../<file>) <(sed -n '/<section>/,$p' infra/<changed>/.../<file>)

# 6. Check whether the changed files have CI/test wiring
grep -rn "<changed-file>" --include="*.tf" --include="*.toml" --include="*.sh" --include="*.yml" infra/ .github/

# 7. Check whether READMEs document the thing being changed
grep -nE "<topic>" infra/<changed-area>/README.md

When citing a finding, always use path/to/file.ext:linenum. Reviewers click these. Cite post-PR line numbers (the file as it appears on the branch), not the diff hunk's @@ numbers.

Terraform-specific failure modes to look for

These are the ones that bite quietly:

Renames that destroy and recreate

A rename like aws_iam_role.foo → aws_iam_role.bar is a destroy + create, not a rename. For stateful or sticky resources that's downtime or worse:

• aws_ecs_service — drops to zero tasks during the gap
• aws_service_discovery_service — DNS gap; can also block destroy if instances are running
• aws_cloudwatch_log_group — log history wiped
• aws_db_instance / aws_rds_cluster — data loss
• aws_s3_bucket — fails if non-empty, or wipes versioned data
• aws_iam_role — breaks every active session and cross-account assume-role chain that references the ARN
• aws_security_group — references in other SGs/SGRs may fail to detach cleanly

If you see a rename, check whether the resource is in the state file and whether moved {} blocks (Terraform ≥1.1) or terraform state mv are being used to make it a rename rather than a destroy.

Force-replace on in-place-able resources

Some attribute changes silently force replacement. Check the diff for changes to:

• aws_ecs_task_definition — every change creates a new revision (this is fine and expected); but a change to family is destroy-create
• aws_iam_role.assume_role_policy — in-place ✓
• aws_iam_role.name — destroy-create
• aws_security_group.name — destroy-create
• aws_subnet.cidr_block — destroy-create, and breaks anything in the subnet
• VPC cidr_block, RDS engine_version (sometimes), Lambda function_name

When unsure, run terraform plan and look for # forces replacement.

IAM privilege bloat

Watch for:

• New Action entries on existing inline policies — easy to over-grant ("just add s3:* for now")
• Resource = "*" where a narrower ARN would do
• New Principal entries broadening assume-role trust (especially * or root account ARNs without Condition constraints)
• Cross-account sts:AssumeRole without a Condition on aws:SourceAccount or aws:SourceArn
• iam:PassRole without a resource constraint
• Wildcard managed-policy attachments (AdministratorAccess, PowerUserAccess)

For each new permission, ask: what specific operation in the diff requires this? If you can't answer, it's bloat.

IAM inconsistencies across sibling stacks

If infra/cts/observability/yace.tf and infra/zerokms/observability/yace.tf describe the same service, their YACE task IAM policies should be identical. Check:

• Same Action list, same order
• Same Resource scoping
• Same Condition blocks
• Same assume_role_policy shape (same trust principals, same conditions)

Divergence is either (a) one was updated and the other wasn't, or (b) intentional but undocumented. Flag it; ask which.

"Helpful" scope creep

Especially watch for changes whose stated purpose is X but that also:

• Tighten or rotate IAM permissions ("while I was here, I dropped some unused actions")
• Rename resources for clarity
• Refactor a for_each to a count or vice versa
• Reformat / lint-fix unrelated files
• Add a "security improvement" — enable_key_rotation, force_destroy = false, public-access blocks, encryption-at-rest toggles

Each of these can be the right thing to do, but bundling them with an unrelated change makes the diff harder to reason about and raises the chance that one of them is wrong. Recommend splitting unless the change is genuinely atomic with the stated purpose.

Cross-account / cross-region references

• Hardcoded account IDs in role ARNs (arn:aws:iam::471112718864:role/...) — search for them; flag if a sibling stack uses a different ID without explanation
• Hardcoded VPC IDs / subnet IDs — region-coupled, breaks portability
• Hardcoded AMIs — region-coupled
• Region strings inside YAML configs — same DRY problem as Terraform locals

Sibling-stack consistency checks

For every changed file, look for the equivalent file in the sibling stack(s) and diff:

diff <path-to-sibling-equivalent> <path-to-changed-file>

Expect a small, explainable set of differences (account ID, region, resource ARN, service name). Anything beyond that is either (a) the sibling needs updating too, or (b) the divergence needs documenting.

When the PR description says "brings X in line with Y" — verify that claim by diffing X and Y after the change. It's common for "in line with" to actually leave one side ahead of the other.

Output shape

Default to this structure. Skip sections that have no findings.

# PR #<num> Review — `<title>`

**Scope:** <N> files, +<adds> / -<dels> — <one-line description of what changed>. <Optional: notable absences, e.g. "No Terraform resources renamed, no IAM changes.">

**Verdict:** <Approve / Approve with follow-ups / Request changes / Block>. <One sentence why.>

---

## What's clean

- <Things the reviewer should know are correct, not just absent. Cite line numbers.>

## <Severity> concerns

### N. <Short title>

<Body. Cite specific lines using `path/to/file.ext:linenum`. Suggest the fix concretely, ideally with a one-line code sketch.>

## Operational watch-items

<Things that aren't blockers but are worth keeping an eye on after merge. Memory limits, throttling risk, gradual cost growth, etc.>

## Summary

| Category | Finding |
|---|---|
| Resource renames / downtime risk | <None / specific> |
| IAM privilege bloat | <None / specific> |
| IAM inconsistencies introduced | <None / specific> |
| Scope creep in adjacent areas | <None / specific> |
| Maintainability (DRY) | <...> |
| Maintainability (tests) | <...> |
| Maintainability (docs) | <...> |
| Cross-stack consistency | <...> |
| Readability | <...> |

<Closing line: "Ship it; queue the follow-ups." / "Block on N; rest can ship." / etc.>

Severity ladder

• Block — will cause downtime, data loss, security regression, or breaks compliance. Must fix before merge.
• Request changes — wrong default, will cause real pain in operation, or violates a stated invariant of the codebase. Strongly recommend fix.
• High-value follow-up — correctness or maintainability issue worth a separate PR soon.
• Medium / Low — improvements; nice to have; document for the next time someone touches this.

Don't inflate severity. The user's goal is to ship. Reserve "block" for things that are actually unsafe.

Anti-patterns to avoid in the review itself

• Don't review the test plan as code. PR descriptions claim things were tested; you can't verify that from a diff. Treat the screenshot/dashboard as evidence, not proof.
• Don't list every micro-nit. A good infracode review names 3–8 things that matter. Twenty bullet-pointed nits dilute the signal and the author will skim.
• Don't say "consider X" without sketching X. If the suggestion is "use a YAML anchor" or "use templatefile()", show what that looks like in one line.
• Don't recommend large refactors as part of this review. Note them as follow-ups. The PR's purpose is the PR's purpose.
• Don't quote the entire diff back at the user. They have it. Cite specific lines and trust them to look.
• Don't review the prose of the PR description. Review the change.

What to leave out

• Process narrative ("first I checked X, then I diffed Y").
• Repetition of the PR description.
• Blanket statements about Terraform best practices that aren't anchored to a specific line.
• Praise that isn't actionable ("nice work", "looks good") — if there's nothing to flag, say so plainly and move on.

Self-check before delivering

• Every finding cites at least one path/to/file.ext:linenum.
• Every "concern" has a concrete suggested fix or follow-up.
• The verdict matches the severity of the findings — don't say "Approve" then list three blockers.
• The summary table is filled in; "None" is a valid and useful answer.
• Total output fits on roughly one screen for a small PR; longer for big PRs, but never longer than the diff itself.