Skip to content

Promote dev to main: user-branch stack CI + DNS prerequisite docs - #25

Merged
Atom-oh merged 2 commits into
mainfrom
dev
Sep 4, 2026
Merged

Atom-oh merged 2 commits into
mainfrom
dev

Conversation

@Atom-oh

@Atom-oh Atom-oh commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Promotes PR #24.

🤖 Generated with Claude Code

…e documented

- terraform.yml: plan on user-branch pushes and apply/agentcore dispatches
  from user branches, against that user's TF_*_PREVIEW_<USER> pair (fail-closed
  case selection; development environment + dev deployer role)
- branch-strategy runbook: the awsops-dev.whchoi.net zone lives in the samples
  account but its NS delegation from the live whchoi.net zone (another
  account) is PENDING — exact record listed; every stack's ACM validation
  waits on it
CI: user branches manage their own stacks; DNS delegation prerequisite
@Atom-oh
Atom-oh merged commit 47a93fa into main Sep 4, 2026
3 checks passed
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 AI Code Review (Claude Fable 5 chair · lens×model matrix)

_Cells (model/lens): codex/L2 codex/L3 codex/L4 codex/L5 _

Status: BLOCKED — CRITICAL/MAJOR issues must be resolved before merge

🛑 Forced FAIL due to coverage collapse: at most one vendor survived, so cross-verification across the lens x model matrix cannot happen — fail-closed regardless of the chair's judgment.

⚠️ Coverage degraded: model(s) [kiro-opus, kiro-gpt] had no response across every lens (invalid flag/missing binary/auth failure, etc.) — the review below was synthesized without them.

All base files verified. Key facts established: (1) the shared DEV_ROLE for user branches already exists in base deploy-web.yml and is documented as intentional in dev-repo-setup.md §2; (2) I found an inconsistency the panel missed — the plan role's documented OIDC trust (dev-repo-setup.md:53) only covers main/dev/pull_request, but this diff adds user-branch pushes to the plan trigger without updating that matrix.

PR #25 Review — user-branch stack CI + DNS prerequisite docs (Chair synthesis)

1. Summary

This PR extends the per-user preview-stack CI pattern (already live in base deploy-web.yml) to terraform.yml and deploy-agentcore.yml — user branches (atomoh/ssminji/whchoi) get automatic plans on push and dispatch-gated apply/agentcore under the dev-tier role and per-user TF_*_PREVIEW_<USER> secrets — and documents the pending NS delegation for awsops-dev.whchoi.net in the branch-strategy runbook. The workflow changes faithfully replicate the base's fail-closed, no-cross-branch-fallback pattern, but the PR enables user-branch plan runs whose OIDC trust the repo's own bring-up runbook says does not exist, and does not update that runbook.

2. Issues per lens

L2 — Code correctness (panel: codex/L2)

No issues — panel found none; I concur. No TS/React or Python runtime code is touched. Two things I verified rather than assumed:

  • The secrets[format('TF_BACKEND_HCL_PREVIEW_{0}', github.ref_name)] lookup uses lowercase branch names against secrets registered "uppercased" (branch-strategy.md step 3) — this works because GitHub Actions context property lookup is case-insensitive, and the identical pattern is already live in base deploy-web.yml:66. Not a defect.
  • The case statements keep the explicit hard-fail semantics (no &&/|| ternary fallthrough) that terraform.yml's header comment mandates. Correct.

L3 — Security / AWS mutation safety (panel: codex/L3)

MINOR (downgraded from panel MAJOR) — codex/L3 (1/1 model on this lens) flagged as MAJOR that personal branches assume the shared mutation-capable DEV_ROLE, so a branch author could target other previews or shared dev resources. Verified against base: this is pre-existing, documented design, not introduced by this diff. Base deploy-web.yml:76 already grants DEV_ROLE to dev|atomoh|ssminji|whchoi, and dev-repo-setup.md:52 explicitly documents sample-awsops-dev-ci-deployer as scoped to "dev + user stacks' ECS/ECR-pin/apply — never production", with branch-strategy.md:119-122 describing the environment: development branch-policy gate. The residual risk (one user's branch can reach another user's stack or the shared dev stack) is real but is an accepted tradeoff among the three named internal collaborators, isolated from production by distinct roles, distinct secret names, and the production environment reviewer gate. Excluded from the gate as out of diff scope; worth a note (see Suggestions).

No ADR-005 concern: apply remains saved-plan-only (apply tfplan, exact artifact from plan_run_id), dispatch-only, environment-gated — no -auto-approve, no new autonomous mutation path. No hardcoded secrets in the diff (secrets flow via secrets[] context).

L4 — Observability / data-integration (panel: codex/L4)

No issues — panel found none; I concur. No Steampipe, CIS, or AgentCore diagnosis logic is touched.

L5 — Docs / ADR consistency (panel: codex/L5)

MAJOR (chair finding — 0/1 panel models caught it) — The plan role's documented OIDC trust does not cover the user-branch pushes this PR enables, and the PR doesn't update it. The diff adds atomoh, ssminji, whchoi to terraform.yml's push: trigger and advertises in branch-strategy.md that "terraform.yml plans your stack on terraform-path pushes." But the plan job assumes sample-awsops-ci-terraform-plan via a ref-based trust (no environment: on the plan job), and docs/runbooks/dev-repo-setup.md:53 — the CI/OIDC bring-up runbook this very diff points readers to — lists that role's trust as StringLike: ...:ref:refs/heads/main, ...:ref:refs/heads/dev, ...:pull_request only. A push from a user branch presents sub ref:refs/heads/<user>, which that trust rejects. So post-merge, either (a) the live trust was already widened out-of-band and the runbook is now wrong on an IAM trust matrix (its stated source-of-truth purpose), or (b) the trust matches the docs and the PR's headline feature fails at AssumeRoleWithWebIdentity on every user-branch terraform push. Either way a defect exists in the merged tree; the live-IAM side is unverifiable from here, but the docs contradiction is reproducible against base. (Apply/agentcore are not affected — they carry environment: development, whose trust is environment-sub-based and whose branch policy already lists the user branches.)

MINOR — The diff embeds the Route53 hosted zone ID (Z05356393HGNKULJIZ69V) and NS hostnames in branch-strategy.md. docs/runbooks/CLAUDE.md says runbooks must not embed secrets/account IDs/ARNs/live domains; a zone ID isn't in that enumerated list and NS records become public DNS data once delegated (and this file already carries live domains under the repo's documented "public-safe content" acceptance), but it's worth a conscious call rather than an accident.

Bilingual convention: satisfied — the new NS-delegation warning carries a Korean paragraph, per the runbook-body bilingual rule.

Panel agreement notes

Each lens had a single model, so no cross-model agreement signal is available. The one panel MAJOR (L3 shared role) did not survive base verification as a diff-scope finding; the one gate-relevant finding (L5 trust matrix) was found by no panel model and verified directly by the chair against dev-repo-setup.md and terraform.yml in base.

3. Suggestions

  1. (Fixes the MAJOR) Update dev-repo-setup.md §2's sample-awsops-ci-terraform-plan row to include the three user-branch refs (...:ref:refs/heads/atomoh|ssminji|whchoi), and confirm the live role trust actually includes them before merging — otherwise user-branch plans fail at the credentials step.
  2. Consider a one-line note in branch-strategy.md acknowledging that the three user stacks share the dev-tier deployer role (mutual reachability between user/dev stacks is accepted; production is isolated) — this pre-empts the L3-style flag in future reviews and matches the repo's habit of documenting accepted tradeoffs.
  3. Decide explicitly whether hosted zone IDs belong in public runbooks; if yes, consider amending docs/runbooks/CLAUDE.md's prohibition list wording so the convention and practice agree.

4. Verdict

One MAJOR (L5): the PR enables user-branch terraform plans whose OIDC trust the repo's own bring-up runbook says doesn't exist, and leaves that runbook's trust matrix stale. Everything else is clean or pre-existing documented design.


Triggered by commit 7ed356b2f20c6f33480ce64548ac727b846612e3 · workflow: .github/workflows/pr-review.yml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant