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