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.