acdl/.ciagent/REVIEW.md

# ACDL v1.11 — Multi-Persona Code Review (P60–P65 retrofit + new work)

**Reviewer:** ci-code-reviewer (model: glm-5.2)
**Scope:** v1.11 milestone, branch `milestone/v1.11-restart` — 22 commits
(e1bb214..8c09580), 25 files, +790/-142 lines
**Date:** 2026-07-29

## Commits reviewed

| Commit | Phase | Type | Summary |
|--------|-------|------|---------|
| e1bb214 | 60 | docs | retrofit plan — L1 lifecycle pipeline live-run |
| bc9058f | 60 | feat | L1 module lifecycle live run — module fixes (retrofit) |
| bb3ac7c | 60 | fix | WAF scope case + VPC modify DependencyViolation |
| 0c5c4d1 | 61 | docs | create phase plan — L2 lifecycle pipeline author |
| 361fe60 | 61 | feat | L2 lifecycle pipeline — extend matrix + workflows + tests |
| 9ac5720 | 61 | verify | 4-layer gate — PASS |
| 6441633 | 62 | docs | create phase plan — L2 lifecycle pipeline live run |
| 4dad967 | 60 | fix | ALB target group name_prefix — avoid orphaned conflicts |
| adfcf86 | 63 | docs | create phase plan — regression registry + cost docs |
| b71e63c | 63 | feat | CAP-017..022 regression registry + COST.md |
| beac2ef | 63 | verify | 4-layer gate — PASS |
| 06f4fc7 | 60 | fix | free disk space in lifecycle jobs |
| 92bb03e | 64 | docs | create phase plan — pre-mortem + teardown |
| 186cdde | 64 | feat | pre-mortem — v1.10 post-mortem + forward pre-mortem |
| 4102950 | 64 | feat | pre-mortem + teardown plan — HITL escalation CHG0680001 |
| 7c4fc1f | 64 | feat | teardown complete — zero live ACDL resources remain |
| a52f8a5 | 64 | verify | 4-layer gate — PASS |
| a03c019 | 60/62 | fix | ALB name_prefix + adapter dedup + L2 composition wiring |
| 93a6598 | 65 | docs | create phase plan — rewrite caps + decks |
| 6394801 | 65 | feat | rewrite caps — CAP-017..022 Verified via lifecycle pipeline |
| fc91f24 | 65 | verify | 4-layer gate — PASS |
| 8c09580 | 65 | docs | update v1.11 status — all phases complete |

## P0 issues (0)

No blocking issues found. The targeted fixes are correct for their stated
purposes. The 447 fast offline tests pass (485/490 collected; 5 slow
deselected, including 2 slow regression-integration tests that exercise the
CAPABILITY_REGISTRY against the live codebase).

## P1 issues (5 — should fix)

### P1-1: Adapter dedup silently drops resources whose module is not in the registry
[correctness] `adapters/terraform/adapter.py:159-170`

The new dedup loop only adds resources to `seen` when `tf_dir` is truthy
(in the registry). A resource whose module is missing from the registry is
**silently dropped** from `merged` — it never reaches `_emit_module_block`,
so no error is raised. The pre-dedup code (`parts.extend(... for r in
resources)`) would have raised `ValueError("no terraform_dir in registry
for module ...")` via `_emit_module_block`, surfacing the misconfiguration.

Confirmed by simulation: two resources, one with `module: nonexistent@1.0.0`,
produces a `merged` list of length 1 — the unknown-module resource vanishes
without diagnostic.

**Recommendation:** in the dedup loop, when `tf_dir` is `None`, either
(a) raise immediately (preserving the prior contract), or (b) append the
resource to a separate `unknown` list and extend `parts` with it so
`_emit_module_block` raises the descriptive error. As written, a typo in
a composition's `module` field (e.g. `iam-role@1.0.0` vs `iam_roles@1.0.0`)
will silently omit a resource from the emitted terraform — a class of
defect the v1.10 sweep was specifically created to catch.

### P1-2: L2 static-assets "modify" example is a no-op — complex ≡ simple
[correctness] `modules/l2/static-assets/examples/complex.yml`,
`modules/l2/static-assets/composition.json`

The complex.yml comment claims "Modify variant: same bucket_name as simple
(in-place modify, adds CDN + WAF)". But resolving both examples yields
**identical** resource sets: `['s3','cloudfront-distribution',
'cloudfront-originaccesscontrol','waf','kms']`. The CDN and WAF are
**always present** in the static-assets composition (they are unconditional
children + wires); the `waf_enabled`, `default_ttl`, `max_ttl`,
`price_class`, `viewer_protocol_policy` inputs in complex.yml have **no
corresponding wires** in composition.json and are silently dropped at
resolve time. So the L2 static-assets lifecycle cell's "modify" step
applies a contract that produces the same terraform as "simple" — it
exercises `terraform apply` twice with no change, not a true modify.

This is not a regression (the inputs were never wired), but the
CAPABILITY_INVENTORY claim "CAP-020 Verified live-aws via L2 static-assets
lifecycle pipeline (apply/modify/destroy exit 0)" overstates what the
modify step proves: it proves idempotent re-apply, not in-place modify.

**Recommendation:** either (a) wire `waf_enabled`/`default_ttl`/etc. in
composition.json so the complex contract genuinely differs, or (b) correct
the comment + CAPABILITY_INVENTORY wording to "apply + idempotent re-apply
+ destroy" rather than "apply/modify/destroy". The microservice complex
example, by contrast, is a real modify (desired_count 1→2) — that one is
fine.

### P1-3: L2 lifecycle scripts ignore the ci-vpc-outputs.json argument
[correctness] `scripts/run_l2_lifecycle_test.sh:14`,
`scripts/run_l2_lifecycle_destroy.sh:12`

Both L2 scripts declare `Usage: ... <module> <example> [ci-vpc-outputs.json]`
but neither reads `$3`/`$2`. The microservice composition references the
platform VPC via `terraform_remote_state` (data source), and the script
sets `ACDL_REMOTE_STATE_KEY=spike/ci-vpc/terraform.tfstate` so the data
source reads from the CI VPC state — that part is correct. But the
`ci-vpc-outputs.json` argument is positional noise: the workflow passes
it (`run_l2_lifecycle_test.sh ${{ matrix.module }} simple
/tmp/ci-vpc-outputs.json`) and it is silently ignored. The L1 scripts
(`run_lifecycle_test.sh`) inject VPC outputs by rewriting the contract in
Python; the L2 path takes a different approach (remote state) and does not
need the file, so the argument is vestigial, not a bug — but the usage
string advertises a feature the script does not provide, which will
confuse a future maintainer who assumes parity with the L1 scripts.

**Recommendation:** remove the `[ci-vpc-outputs.json]` token from the
usage strings (or add a comment explaining the L2 path uses remote state
and the arg is accepted-but-ignored for workflow-argument parity).

### P1-4: CAPABILITY_INVENTORY summary table is stale (says 16, body lists 22)
[maintainability] `.ciagent/CAPABILITY_INVENTORY.md:9-16`

The Summary table still reads "Verified 16 / Decayed 0 / Broken 0 / Total
16" — the v1.10 sweep count. The body (lines 93-110) now lists CAP-017..022
as **Verified** via the lifecycle pipeline, bringing the real total to 22.
The two counts disagree: a reader scanning the summary sees 16 Verified; a
reader scanning the inventory body sees 22 Verified. The PRE_MORTEM
(lines 82-83) and CAPABILITY_INVENTORY prose both assert all 22 are
Verified, but the headline table was not updated in the P65 rewrite.

**Recommendation:** update the Summary table to "Verified 22 / Decayed 0
/ Broken 0 / Total 22" and add CAP-017..022 rows to the Inventory table
(the body section "Cloud capabilities NOT re-verified..." is now
mis-titled — they ARE verified, just via the lifecycle-pipeline tier).

### P1-5: CAP-017..022 regression checks are offline proxies, not pipeline evidence
[adversarial] `core/regression_verify.py:432-519`,
`.ciagent/CAPABILITY_INVENTORY.md:93-110`

The CAP-017..022 checks (`_check_cap_017_dynamodb` etc.) call
`_check_lifecycle_module_terraform` / `_check_lifecycle_l2_module`, which
verify only that (a) the terraform dir + required files exist and (b) the
example contracts **resolve** (resolver exit 0). They do **not** run
`terraform validate`, do not run apply/modify/destroy, and do not query
the pipeline's actual green/red status. The CAPABILITY_INVENTORY claims
"Evidence = L1 rds module lifecycle pipeline green (terraform validate +
contracts resolve)" — but the check does not run terraform validate, and
"lifecycle pipeline green" is asserted, not verified by the regression
gate.

This means the lifecycle-pipeline evidence CAN be faked at the regression
tier: a module whose terraform is syntactically broken (e.g.
`scope = upper(var.scope)` removed, or a missing required variable) would
still pass `_check_lifecycle_module_terraform` as long as the files exist
and the resolver runs. The real green/red evidence lives only in the
workflow run history (Gitea/GitHub Actions), which the regression gate does
not read.

**Mitigation context:** the modules-lifecycle workflow IS the live
evidence — when it runs on a PR, the cells genuinely apply/modify/destroy
against live AWS. The gap is that the *regression gate* (which gates
milestone COMPLETE) trusts the workflow will be run, rather than proving it
was run and passed. A milestone could in principle be marked COMPLETE with
CAP-017..022 "Verified" if the regression gate runs but the workflow was
never executed (e.g. workflow_dispatch never triggered, or the PR was
merged without the workflow running).

**Recommendation:** (a) tighten the CAP-017..022 check docstrings + the
CAPABILITY_INVENTORY wording to "terraform files present + contracts
resolve (offline proxy; live apply/modify/destroy verified by the
modules-lifecycle workflow run, not by this gate)"; and/or (b) add a
`terraform validate` step to `_check_lifecycle_module_terraform` (slow but
cheap relative to init+apply) so at least HCL syntax is verified at the
gate. The teardown trustworthiness (P64) is good — `ci-vpc-destroy` runs
`if: always()` and the decommission `---ci---` block is the audit trail.

## P2 issues (4 — post-hoc)

### P2-1: ALB `name_prefix = "tg-ci-"` discards `var.name` entirely
[maintainability] `modules/l1/alb/terraform/main.tf:9`

The fix replaces `name = var.name` with `name_prefix = "tg-ci-"` (a
hardcoded literal). This is the correct terraform pattern for
create_before_destroy resources with name-uniqueness constraints, and the
commit message explains the orphaned-resource motivation well. However
the target group name is now non-configurable (always `tg-ci-<random>`),
and the `var.name` variable is no longer used by the target group at all
(it is still used by `aws_lb.this.name`). A consumer who sets `name:
my-app` gets an LB named `my-app` but a target group named `tg-ci-...` —
inconsistent tagging. Consider `name_prefix = "${var.name}-"` to keep the
consumer's name as a prefix while preserving uniqueness. Post-hoc: not
blocking; the lifecycle pipeline is the only current consumer and `tg-ci-`
is fine for CI.

### P2-2: No test covers the new dedup merge behavior or `ACDL_REMOTE_STATE_KEY`
[testing] `tests/test_adapter.py`, `tests/test_pipeline_contract.py`

The adapter gained (a) a dedup-merge loop for multi-resource L1s sharing a
terraform dir and (b) `ACDL_REMOTE_STATE_KEY` env override for the remote
state data block. Neither has a unit test:
- No test asserts that two resources with the same `module` collapse to one
  `module "<first_id>" { ... }` block with merged inputs.
- No test asserts that `ACDL_REMOTE_STATE_KEY` overrides the default
  `platform/terraform.tfstate` key in the emitted `data
  terraform_remote_state` block.
- No test covers the L2 lifecycle scripts (`run_l2_lifecycle_test.sh` /
  `run_l2_lifecycle_destroy.sh`) — the L1 equivalents are also untested at
  the script level, so this is consistent with existing practice, but the
  L2 scripts are new in this session and the `ACDL_REMOTE_STATE_KEY` wiring
  is the load-bearing correctness mechanism for the microservice lifecycle.

The 485 offline tests adequately cover the *contract* (pipeline schema,
byte-identical workflows, matrix membership, job needs) — the
`TestModulesLifecyclePipeline` class is solid (89 tests pass). The gap is
adapter *behavior* at the unit level.

**Recommendation:** add a `test_adapter_dedup_merges_same_module` and a
`test_adapter_remote_state_key_override` to `tests/test_adapter.py`.

### P2-3: `waf` complex example uses `scope: CLOUDFRONT` but WAF scope is now `upper()`'d
[correctness] `modules/l1/waf/examples/complex.yml:8`,
`modules/l1/waf/terraform/locals.tf:3`

The `locals.tf` change `scope = upper(var.scope)` is the correct defensive
fix (the AWS provider requires `CLOUDFRONT`/`REGIONAL` regardless of input
case). The complex.yml was simultaneously changed from `scope: cloudfront`
to `scope: CLOUDFRONT`. Both are now correct, but the example's uppercase
value is now redundant with the `upper()` — a future reader may wonder
which is authoritative. Minor; the defensive `upper()` is the right call
and the example matching it is fine. Post-hoc only.

### P2-4: COST.md reproducibility snippet could leak the account ID via CloudTrail
[security] `.ciagent/COST.md:106`

COST.md contains the AWS account ID `581513795199` in multiple places
(summary, S3 bucket name, methodology). This is consistent with the rest of
the repo (the bucket name `acdl-tfstate-581513795199-us-east-1` is hardcoded
in `adapter.py:130` and `adapter.py:146`), so it is not new leakage and not
a regression. No actual secret material (access keys, secret access keys)
appears in COST.md, PRE_MORTEM.md, CAPABILITY_INVENTORY.md, or the workflow
files — all credential references use `${{ secrets.ACDL_AWS_* }}` or env
var names only. The `.ciagent/PROJECT.md:731` reference to a deactivated
root key is redacted (`AKIA…ROOT-DEACTIVATED`). **No credential leakage
found.** The P2 is only that the account ID is published; if the account
is meant to be opaque, this is an accepted exposure (the bucket name
already requires it).

## What is correct

- **WAF scope fix (`upper(var.scope)`):** correct and defensive; AWS
  provider v5 requires uppercase. The `local.scope` indirection is clean.
- **VPC `create_before_destroy` + same-CIDR complex example:** correct
  fix for the DependencyViolation on modify. Using the same CIDR means
  terraform modifies in-place rather than replacing the VPC (which would
  cascade-fail on dependent subnets/IGW). The `create_before_destroy`
  lifecycle is the right guard.
- **ALB `name_prefix`:** correct terraform pattern for
  create_before_destroy + name-uniqueness; well-documented commit message.
- **Adapter dedup (for the registered-module case):** correct —
  multi-resource L1s like cloudfront (distribution + OAC) correctly merge
  into one `module "cloudfront-distribution" { ... }` block. The merge
  preserves first-resource inputs and union of outputs. (The
  unregistered-module drop is P1-1, a separate concern.)
- **L2 composition wiring (`ecr.inputs.name`, `roles.inputs.role_name`):**
  correct. Resolving microservice complex now shows `ecr.inputs.name =
  "app-repo"` and `roles.inputs.role_name = "app-role"` (defaults applied
  since the contract doesn't set `name`). Previously these would have hit
  the "missing required arg" defect class from the v1.10 sweep.
- **Microservice complex = real modify:** `desired_count: 2` (vs simple's
  default 1) is a genuine in-place modify — confirmed by resolving both
  and diffing `service-service.inputs.desired_count`.
- **`ACDL_REMOTE_STATE_KEY` plumbing:** correct end-to-end — the L2 scripts
  export it, the adapter reads it with a sensible default, and the
  microservice composition's `terraform_remote_state` data block picks it
  up. This cleanly separates the short-lived CI VPC state from the
  long-lived platform VPC state.
- **Workflow structure:** `l2-lifecycle` correctly `needs: ci-vpc-apply`;
  `ci-vpc-destroy` correctly `needs: [lifecycle, l2-lifecycle]` and
  `if: always()`. The 7 new L2 pipeline-contract tests assert all of this.
- **Byte-identical workflows:** `.gitea` and `.github` modules-lifecycle.yml
  are byte-identical (test asserts this); the `test_workflow_has_four_jobs`
  rename from three→four is correct.
- **Adapter line count:** 194 lines — under the 200-line ceiling, still a
  clean stateless assembler. The dedup logic added ~16 lines without
  bloating.
- **Teardown verification (P64):** trustworthy in structure — the
  `ci-vpc-destroy` job runs unconditionally and the decommission
  `---ci---` block is the audit trail. The adversarial concern (P1-5) is
  about the regression gate trusting the workflow ran, not about the
  teardown itself being fakeable.
- **Security:** no credential leakage in any reviewed file. All AWS auth
  in workflows uses `${{ secrets.* }}`; COST.md references only env var
  names and a redacted/deactivated root key ID.

## Test coverage assessment (485 offline tests)

- **Adequate:** pipeline contract (89 tests), schema validation, contract
  resolution, adapter emission (basic), confidence signal, outbox,
  interpolation, local emulators, module-standards file presence, design-doc
  currency.
- **Gaps (post-hoc):**
  1. Adapter dedup merge behavior (P2-2) — no unit test.
  2. `ACDL_REMOTE_STATE_KEY` override (P2-2) — no unit test.
  3. CAP-017..022 regression checks (P1-5) — not exercised at the unit
     level; the 2 slow tests in `test_verify_regression_mode.py` run the
     full registry but are `@pytest.mark.slow` and deselected from the
     fast suite, so a CI run of the 485 fast tests does not verify
     CAP-017..022 even at the offline-proxy level.
  4. WAF `upper()` scope — no test asserts the locals transform; relies
     on the lifecycle pipeline cell to catch a regression.
  5. ALB `name_prefix` — no test asserts the target group uses
     `name_prefix` (P2-1 context).

The 485 count is honest (447 pass fast, 5 deselected slow, 485/490
collected). The gap is behavioral coverage of the new adapter + module
logic, not contract/schema coverage.

## Verdict

**PASS with P1 flags for post-hoc review.** No P0 fixes applied. The
milestone's structural controls (regression gate, mandatory teardown,
byte-identical workflows, byte-identical contract↔workflow tests) are
sound. The most material finding is P1-5 (the regression gate's
CAP-017..022 evidence is an offline proxy, not live pipeline evidence) —
this is a repeat of the v1.10 "VERIFY was diff-scoped" structural defect
in a milder form: the gate trusts the workflow was run rather than proving
it. The mitigations in PRE_MORTEM (FM-1..FM-4) acknowledge related risks;
P1-5 is the specific instance for the lifecycle-pipeline tier.