Promote dev: public-repo log hygiene + demo/admin user policy

[Atom-oh]

Promotes the merged #28 chain: role-ARN secrets (log masking), mask-aws-account-id, encrypted tfplan artifact, per-stack demo user (opt-in create_demo_user), admin user out of Terraform management (out-of-band CLI provisioning), IAM findings admin-gated in the web BFF, and persistent-runner cleanup steps.

🤖 Generated with Claude Code

[github-actions]

🤖 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.

Final Review — PR #29: Promote dev: public-repo log hygiene + demo/admin user policy

Summary

This PR moves role ARNs from repo variables into masked secrets, adds mask-aws-account-id, encrypts the tfplan artifact fail-closed, adds always-run runner cleanup steps, replaces the Terraform-managed Cognito admin with an out-of-band-provisioned admin plus a gated shared demo user, and adds an admin-only gate to the iam_no_mfa security check. The CI hygiene and web-side changes verified cleanly against base (the terraform.yml job-level defaults.run.working-directory: terraform/foundation makes the new steps correct; web/lib/admin.ts, verifyUser().groups, and CHECKS all exist in base; no stale references to the removed admin_password variable remain). One MAJOR remains: deleting the aws_cognito_user.admin resource block destroys the live admin user on the next apply when Terraform's removed block would achieve the stated "not TF-managed" policy without destruction.

Panel note: each lens was seen by a single model (codex), so cross-model agreement signals are unavailable; all adopted findings were independently verified against the base checkout and diff.

Issues per lens

L2 — Code correctness

Panel: no findings. Chair verification concurs.

• web/app/api/security/route.ts filtering, the checks response field, the page's visibleChecks fallback (?? CHECKS for older API responses) and tab-snap effect are all coherent; tests match the query-call ordering (iam_no_mfa never queried for non-admins).
• The new cleanup steps in deploy-web.yml/deploy-agentcore.yml are placed after the last step that reads terraform.tfvars/backend.hcl (verified against base step ordering), and terraform.yml's encrypt/decrypt/cleanup steps inherit the correct working directory from job defaults.
• make agentcore runs a node provisioner, not terraform plan/apply, so scoping TF_VAR_demo_password to terraform.yml's plan step only is sufficient (the apply job consumes the saved plan with values embedded).

No CRITICAL/MAJOR/MINOR.

L3 — Security / AWS mutation safety

MAJOR — Removing the aws_cognito_user.admin block destroys the live admin user instead of orphaning it (terraform/foundation/auth.tf). Flagged by the panel (1/1 model on this lens); chair-verified as real behavior. Deleting the resource block makes the first post-merge apply destroy the existing admin on every stack, production included — an AWS-mutating destroy of a live credential shipped inside a hygiene PR. The PR does document this as intentional (runbook + auth.tf comment), but two things keep it MAJOR rather than an accepted trade-off: (1) recreation via admin-create-user issues a new Cognito sub, and sub-keyed ownership rows (e.g. worker_jobs.requested_by, which ADR-009's backfill deliberately migrated to subs) become orphaned — the ownership-checked routes will deny the recreated admin access to their prior jobs; this consequence is not acknowledged anywhere in the PR. (2) Terraform's designed mechanism for "stop managing without destroying" — a removed { from = aws_cognito_user.admin; lifecycle { destroy = false } } block (supported at the repo's required TF ≥ 1.15) — implements the stated policy strictly better, with no admin-access gap and no sub churn. Also, the auth.tf comment "intentionally removed from state on the next apply" is inaccurate: without a removed block it is a destroy, not a state removal (the runbook wording is correct).

MINOR (downgraded from panel MAJOR) — Shared demo password persists in Terraform state and the plan file. The panel's claim is factually true (sensitive = true does not keep values out of state/plan), but overstated as MAJOR: state lives in the private awsops-v2-tfstate S3 bucket (not a public artifact), the plan artifact is now encrypted fail-closed, the demo user is non-admin, and create_demo_user defaults to false (fail-closed, opt-in per dev-tier stack). The repo's hygiene rule targets public logs/artifacts, which this PR addresses. Residual risk worth recording: any principal with state-bucket read can recover the shared demo password, and sharing one credential across stacks widens the rotation blast radius — acceptable for a low-privilege demo credential, by design.

Positive L3 notes (verified): role-ARN secrets migration + mask-aws-account-id are applied consistently across all four workflows; the admin_password grep guard is fail-closed in both plan and apply jobs; plan-artifact encryption refuses plaintext upload when the key is unset; TF_VAR_demo_password is step-scoped; no IAM policy is broadened (diff IAM changes are formatting-only).

L4 — Observability / data-integration correctness

Panel: no findings. Chair concurs — no Steampipe query, CIS-check, or diagnosis-logic semantics changed (steampipe.tf/workers.tf hunks are terraform fmt whitespace only; the iam_no_mfa change is access gating, with the check's SQL untouched).

No CRITICAL/MAJOR/MINOR.

L5 — Docs / ADR consistency

MINOR — demo_email description contradicts the gating policy (terraform/foundation/variables.tf): "created in every stack" vs. create_demo_user defaulting to false with explicit per-stack opt-in. Panel finding (1/1), verified against the diff. Update to "created only when create_demo_user = true".

MINOR (chair, in-diff) — auth.tf comment misdescribes the destroy as a state removal — see the L3 MAJOR; if the removed-block suggestion is adopted, the comment becomes accurate as written.

Out-of-gate note (out of diff scope): terraform/CLAUDE.md still describes staging.tfvars as containing "plaintext admin_email/admin_password" — stale once this PR lands (the variable no longer exists and the guard hard-fails on it). Recorded as a suggestion only, per scope rules.

Otherwise the runbook additions accurately describe the implemented mechanics (secret names, fail-closed defaults, encryption flow, per-stack override precedence) — docs/implementation consistency is good.

Suggestions

1. Replace the silent deletion of aws_cognito_user.admin with a removed { from = aws_cognito_user.admin; lifecycle { destroy = false } } block: the existing admin (and its sub, and its sub-keyed ownership rows) survives, the state stops managing it, and the runbook's "recreate when needed" step becomes unnecessary for existing stacks. If destruction is truly desired per stack, do it out-of-band, not as a merge side effect.
2. Fix the demo_email description ("created only when create_demo_user = true") and the auth.tf "removed from state" wording.
3. Update terraform/CLAUDE.md's staging.tfvars description to drop admin_password (follow-up commit is fine).
4. Optional hardening: the deploy workflows' cleanup removes terraform.tfvars/backend.hcl but .terraform/terraform.tfstate (cached backend config) survives on the persistent runner — consider rm -rf .terraform too if backend config is considered sensitive.
5. Consider documenting a rotation procedure for TF_VAR_DEMO_PASSWORD (rotating it requires a re-plan/apply per opted-in stack since the value is baked into state).

Verdict

One verified MAJOR (destructive admin-user removal with an unacknowledged sub/ownership side effect and a strictly-better non-destructive alternative) blocks promotion; everything else is MINOR or clean.

---

Triggered by commit 46ef9fa07758d5318c55bad185789e9ec02e7ea4 · workflow: .github/workflows/pr-review.yml