acdl/.ciagent/REVIEW.md

ACDL v1.9 Milestone — Multi-Persona Code Review

Reviewer: ci-code-reviewer (model: glm-5.2) Scope: v1.9 milestone — Phases 39–42 (tags v1.8.1..v1.8.4), diff v1.8.0..HEAD Date: 2026-07-23 Verdict: READY TO SHIP — 1 P0 auto-fixed, 1 P1 auto-fixed, 3 P1 flagged for post-hoc

Note (D-086): This REVIEW.md was reconstructed at v1.9 complete. The previous content was the v1.2 milestone review (v1.3–v1.8 reviews were not persisted to this file). No git history was rewritten; the v1.2 review is preserved in git history at the v1.2 review commit.

Review pass 2 (post-complete): this review was re-run after the milestone COMPLETE to catch issues the initial self-review missed. The P0 (approver injection) and P1 (future-dated freshness) were auto-fixed.

---

Summary

v1.9 closes four gaps left by v1.8 (user-directed, 2026-07-23): stale design docs, no contract interpolation, promotion requires editing the environment field, and unimplemented stubs. It also closes P1-1 (adapter hardcoded defaults, deferred from v1.2). 4 phases shipped (39–42): design-doc refresh + P1-1 parameterization, contract interpolation + env schema, per-environment CI jobs, stub implementation.

P0 issues

P0-INJECT (auto-fixed)

Shell→Python code injection via GITHUB_ACTOR in scripts/run_platform.sh Step 7b (HITL gate). The approver identity was interpolated directly into a Python string literal (attest('$CONTRACT_ID', '$RESOLVED_ENV', '$APPROVER' ...)). GITHUB_ACTOR (and GITEA_ACTOR) are attacker- controllable in some CI configurations; a username containing '; import os; os.system(...); y=' would execute arbitrary Python.

Fix (auto-applied): the approver, contract id, and env are now passed as environment variables to the Python subprocess (ACDL_HITL_CONTRACT_ID, ACDL_HITL_ENV, ACDL_HITL_APPROVER) and read via os.environ[...] inside the Python code — no string interpolation of user-controllable values.

P1 issues

P1-FRESHNESS (auto-fixed)

core/attestation_matrix.py _is_fresh accepted future-dated artifacts. A timestamp in the future produced a negative age, and age.days <= window_days evaluated True for negative values, so a backdated/future artifact bypassed freshness validation.

Fix (auto-applied): added a age.total_seconds() < 0 guard that rejects future-dated artifacts. Test added (test_freshness_rejects_future_dated_artifact).

P1-WIZ-ERRORS (flagged for post-hoc)

adapters/wiz/wiz_adapter.py WizClient._post does not check for GraphQL errors in the response. A GraphQL API returns {data: ..., errors: [...]}; if errors is present, data.issues can be null and .get("nodes", []) silently masks the error as an empty list (which then emits WIZ_NOT_CONFIGURED). Should surface GraphQL errors as a failed PolicyCheckResult or raise.

P1-WIZ-SSRF (flagged for post-hoc)

WizClient._post performs no SSRF validation on WIZ_API_URL. A malicious WIZ_API_URL env var could target an internal endpoint. The URL is operator-supplied (not consumer-controllable), so the risk is low, but a allowlist/scheme check (https://) would harden it.

P1-OBSOLETE-CHECK (flagged for post-hoc)

core/contract_resolver.py _load_env duplicates core/environment_check.load. The duplication was intentional (so the resolver works as both a package import and a script), but the two can drift. A future refactor should extract a shared helper that both import safely.

Per-lens review

Correctness

• The contract interpolation (_expand_vars) is recursive over dicts/lists/strings; unknown tokens raise ValueError (fail loud). Expansion is post-schema-validation, pre-IR-resolution — the schema sees raw tokens (valid strings), the resolver sees concrete values.
• The environment_override (D-088) is applied BEFORE schema validation so the interpolation context is consistent.
• P1-1: the adapter reads desired_count, launch_type, family, target_type, load_balancer_type from inputs (with interface defaults). The resolver's child_input_map routes wires to the sub-resource that declares the input (desired_count → aws:ecs:service, family → aws:ecs:task_definition). The v1.1 S3 regression is preserved (byte-identical main.tf for S3-only stacks).
• The HITL attestation gate records the approver to the outbox, runs SoD on prod (blocks on approver_qa == approver_prod), invokes the attestation matrix. Dev skips (autonomous).
• The attestation matrix's freshness validation uses the §10.4 windows; signature verification skips when the signing key is unset (D-089) and is required when set.
• The Wiz real client uses the GraphQL API with pagination; graceful degrade when unconfigured.
• The Kyverno translator handles pass/fail/skip/warn + severity + skip- with-reason + resource construction; the inactive-for-TF guard is preserved.

Testing

• 493 offline tests (was 350 at v1.8 → 493 at v1.9, +143 new). Each new feature has dedicated tests:
  • P1-1: test_p1_1_adapter_parameterization.py (override + default + regression).
  • Design docs: test_design_docs_current.py (no stale framing).
  • Interpolation: test_interpolation.py + test_sample_contracts_interpolate.py
    • test_environment_schema.py.
  • Per-env jobs: test_per_env_contracts.py + test_deploy_workflow_env_input.py
    • test_consumer_guide_per_env_section.py.
  • Stubs: test_route_halt_artifact.py + test_hitl_gates.py + test_attestation_matrix.py + test_wiz_adapter_real_client.py + expanded test_kyverno_adapter.py.
• run_ci.sh exits 0; run_platform.sh --check-only exits 0.

Security

• No credentials introduced. The SNS topic is KMS-encrypted.
• SoD blocks on identity equality; the halt artifact is in the audit chain.
• The attestation matrix fails loud on missing/expired evidence for prod/dr.
• Signature verification is required when the signing key is set.
• The adapter has no hardcoded resource defaults (P1-1 closed) — defaults live in the L1 interface, not the adapter.

Performance

• N/A (this milestone is about correctness + design-doc accuracy + stub implementation, not perf).

Maintainability

• The interpolation is a single recursive walker; the env context is loaded via a self-contained _load_env (works as script + package import).
• The child_input_map makes multi-resource L1 wire routing deterministic (the sub-resource that declares the input receives the value).
• The attestation matrix's concern lists + freshness table are data-driven (adding a concern is a table extension, not new logic).
• The Wiz WizClient is a clean class with a single _post seam (testable with mock.patch.object).

Adversarial

• The interpolation fail-loud (ValueError on unknown tokens) prevents silent mis-resolution — a typo in a token name surfaces immediately, not as a stale literal in the emitted Terraform.
• The environment_override is applied before schema validation, so a contract with environment: dev cannot silently interpolate against the dev env when the workflow passes environment: prod — the override is authoritative.
• The SoD check reads approver_qa from the outbox (the platform is the only writer); a consumer cannot forge the approver identity.
• The attestation matrix's signature skip is explicit + logged (not silent).

Conclusion

v1.9 is READY TO SHIP after the review auto-fixes. 1 P0 (approver injection — auto-fixed by passing env vars instead of string interpolation) and 1 P1 (future-dated freshness — auto-fixed with a negative-age guard + test). 3 P1 flagged for post-hoc (Wiz GraphQL error handling, Wiz SSRF validation, _load_env duplication). The milestone's code is complete + verified: design docs are current, contract interpolation works, per-env promotion requires no field editing, all stubs are implemented (audit ledger Object Lock/JWS build-out deferred per D-083), and P1-1 is closed. Ship tag: v1.9.0 (feature milestone, next minor per run.md — v1.8 shipped v1.8.0).

494 offline tests pass (was 350 at v1.8, +144 new); run_ci.sh + run_platform.sh --check-only green.