acdl/.ciagent/REVIEW.md

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