From 2ed2ca6bac09e2b2c2f035e772493dbfd641d019 Mon Sep 17 00:00:00 2001 From: Jon Chery Date: Tue, 21 Jul 2026 19:44:51 +0000 Subject: [PATCH] =?UTF-8?q?review(v1.1):=20READY=20TO=20SHIP=20=E2=80=94?= =?UTF-8?q?=20multi-persona=20code=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ---ci--- project: acdl phase: 0 milestone: v1.1 status: review verdict: READY TO SHIP p0: 0 p1: 1 ---/ci--- --- .ciagent/REVIEW.md | 230 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 230 insertions(+) create mode 100644 .ciagent/REVIEW.md diff --git a/.ciagent/REVIEW.md b/.ciagent/REVIEW.md new file mode 100644 index 0000000..c523f24 --- /dev/null +++ b/.ciagent/REVIEW.md @@ -0,0 +1,230 @@ +# ACDL v1.1 Milestone — Multi-Persona Code Review + +**Reviewer:** ci-code-reviewer (model: glm-5.2) +**Scope:** v1.1 milestone — Phases 06–10 (tags v1.1.1..v1.1.5), diff `v1.1.0..HEAD` +**Date:** 2026-07-21 +**Verdict:** **READY TO SHIP** — 0 P0, 1 P1 (carried-forward), 0 P2 new + +--- + +## Lens 1 — Correctness + +The schemas + Python modules + Terraform implement what the decisions + +`ARCHITECTURE.md` committed. Spot-checks all pass. + +### Findings + +- **`schemas/ir.schema.json`** (REQ-17): resources / relationships / composition + (max-depth-5) / policy hooks (via PolicyCheckResult consumer) all present per + §12.1. Substrate-agnostic: `aws_s3_bucket` appears ONLY in `$comment` and + `description` strings (which explain the IR→Terraform mapping); it does NOT + appear in any constraining keyword (`enum`/`const`/`pattern`/`required`). The + schema body uses IR types (`aws:s3:bucket`). **Correct.** +- **`schemas/contract.schema.json`** (REQ-22, W3.E): per-env mandatory via `allOf` + if/then — qa requires `validation.e2eSuite`+`validation.loadTest`; prod requires + `runbook`+`dashboard`+`oncall`; dr requires `drDrillRef`. The `profile:agentic` + conditional is `if: {required:[profile], profile:{const:agentic}}` → + `then: {required:[naturalLanguageIntent]}` — this is the **fixed** form + (requires `profile` to be present before checking `const`), not the Phase 07 + initial bug. Verified: prod-missing-runbook rejected; agentic-without-NLI + rejected; qa-without-validation rejected; dr-without-drDrillRef rejected; + dev + agentic-with-NLI accepted. **Correct.** +- **`acdl_platform/confidence_signal.py`** (REQ-19, D-040): `WEIGHTS` sum to + 1.0 (verified: 0.30+0.25+0.10+0.15+0.10+0.10 = 1.0). `PENALTY["critical"] = None` + (hard-override sentinel). The critical-override short-circuit + (`if p is None: return Signal(0.0, "block", ...)`) returns BEFORE the + `score = max(0.0, min(1.0, base - penalty))` clamp. Dev-warn→block flip present + (`if environment == "dev" and band == "warn": band = "block"`). The `policy` + input key is read as `inputs.get("policy")` (not `policy_results`) — matches the + Phase 10 e2e `run_spike_e2e.sh` which passes `inputs = {"policy": pcr, ...}`. + Adversarial test: a critical-fail PCR → `score=0.0 band=block reasons=['CRITICAL_OVERRIDE:...']`. + **Correct.** +- **`acdl_platform/contract_resolver.py`** (REQ-27): `resolve()` loads YAML → + validates against `contract.schema.json` → looks up L2 in registry → loads + `composition.json` → maps wires → emits IR → validates against `ir.schema.json`. + Wire mapping verified: `contract.inputs.bucket_name` → + `child.inputs.bucket_name` via `wires.bucket_name.{target:s3, input:bucket_name}`. + Resolved spike IR has `resources[0].inputs = {bucket_name: acdl-spike-bucket, + region: us-east-1}`. Prod-missing-runbook raises `jsonschema.ValidationError` + (not a generic ValueError). **Correct.** +- **`acdl_platform/outbox_writer.py`** (D-044, D-P10-3): SHA-256 over canonical + JSON (`sort_keys=True, separators=(",", ":")`). `prev_event_hash` defaults to + `"GENESIS"`. DynamoDB item shape: PK `contractId` (S), SK + `eventType#eventTs` (S), TTL `expire_at` (N, now+365d). Append-only + (`put_item` only; 0 `delete_item`/`update_item`). **Correct.** +- **`adapters/terraform/adapter.py`** (REQ-26, D-P10-1): `TYPE_MAP = + {aws:s3:bucket -> aws_s3_bucket}`. Backend key derived from stack name: + `spike/l2-static-asset/terraform.tfstate` (verified). Unknown IR type raises + `ValueError`. Resources array handling is shape-driven (iterates + `ir_instance["resources"]`; works for both l1 and l2 IR). **Correct.** +- **`adapters/terraform/policy/checkov_adapter.py`** (REQ-18, D-043): `RULE_MAP` + has exactly 11 Checkov rule IDs (CKV_AWS_41/45/46/20/57/24/25/1/40/7/33). The + `ACDL_TAG_NAMING` SKIPPED record is appended (severity: info, result: + skipped). Tolerates both Checkov JSON shapes — nested + `{framework: {results: {...}}}` and legacy `{framework: {passed_checks:...}}` + (the `results = body.get("results", body)` fallback). **Correct.** + +### Verdict: PASS — no issues. + +--- + +## Lens 2 — Testing + +The verify scripts are real gates that fail on regression, not presence checks. + +### Findings + +- **`scripts/verify_phase07.sh`**: Check 2 uses + `jsonschema.Draft202012Validator.check_schema(...) || fail` — actually + validates the 3 schemas as Draft 2020-12 (fails if a schema is broken). + Check 8 cross-checks the spike contract against `contract.schema.json` via + `jsonschema.validate(...) || fail`. Check 9 cross-checks a minimal IR against + `ir.schema.json`. Every check has `|| fail`. **Real gate.** +- **`scripts/verify_phase10.sh`**: 8 checks, each with `|| fail`. Check (h) is the + REQ-28 substrate-agnostic scan. **Synthetic leak test performed:** appended + `LEAK = "aws_s3_bucket"` to `acdl_platform/separation_of_duties.py` and ran the + Check (h) grep — it caught the leak (`acdl_platform/separation_of_duties.py:44: + LEAK = "aws_s3_bucket"`), then reverted. The check also scans `modules-ir/` + JSON for `aws_*` resource-type VALUES (excluding `description`/`$comment` + strings). **Real gate.** +- **`scripts/run_spike_e2e.sh`** + **`scripts/run_spike_plan.sh`**: touch real AWS + — `terraform init/validate/plan -lock=false` + `checkov` + DynamoDB + `put_item`/`query`. NOT stubbed (the spike key is loaded from gitignored + `.env.secrets`). The e2e runner uses `|| fail` on every step, so a DynamoDB + outage or terraform failure exits 1 (verified: outbox write failure propagates + via `|| fail "outbox write failed"`). **Real e2e.** + +### Verdict: PASS — no issues. + +--- + +## Lens 3 — Security + +AWS key handling (D-034/D-039), IAM least-privilege, gitignore discipline, no +secrets in commits. All clean. + +### Findings + +- **No leaked key IDs in executable code:** + `git log v1.1.0..HEAD -p | grep -iE "AKIA[A-Z0-9]{16}" | grep -v "^#"` returns + matches ONLY inside `.ciagent/VERIFY.md` (the Phase 09 narrative — the + carried-forward P1-1). No `.py`, `.tf`, `.json`, `.yaml`, or `.sh` file + contains an `AKIA…` key ID. **Clean.** +- **No leaked secret keys:** + `git log v1.1.0..HEAD -p | grep -iE "aws_secret_access_key.*=.*[A-Za-z0-9/+=]{40}" | grep -v "^#"` + returns nothing. **Clean.** +- **`terraform/bootstrap/spike_runner_policy.json`** (REQ-23): least-privilege. + Allow actions: `s3:{PutObject,GetObject,DeleteObject,ListBucket,GetBucketLocation,GetBucketVersioning}` + + `dynamodb:{GetItem,PutItem,DeleteItem,UpdateItem,Query,Scan,DescribeTable}` + + `sts:GetCallerIdentity`. **No** `iam:*`, **no** `ec2:*`, **no** + `s3:CreateBucket`, **no** `s3:DeleteBucket`, **no** `terraform apply` + (apply is out of spike scope). `DenyEverythingElse` `NotResource` lists exactly + 3 ARNs (state bucket + bucket objects + outbox table); everything else is + denied. **Correct.** +- **Gitignore discipline:** `.env.secrets`, `terraform/bootstrap/.bootstrap_state.json`, + `terraform/spike/.terraform/`, `terraform/spike/.terraform.lock.hcl`, + `terraform/spike/tfplan`, `terraform/spike/*.tfstate*` all gitignored + (`git check-ignore` confirms each). **Correct.** +- **Outbox write is append-only:** `grep -c "delete_item|update_item" + outbox_writer.py` = 0 (only `put_item`). **Correct.** +- **E2E runner is plan-only:** `grep -c "terraform apply" run_spike_e2e.sh` = 0 + (only `init + validate + plan`). **Correct.** + +### P1 (carried-forward, NOT auto-fixed) + +- **P1-1:** The `.ciagent/VERIFY.md` Phase 09 narrative contains two AWS access + key IDs — `AKIAYOZHMKZ7RK26N66W` (the rotated spike key id) and + `AKIAYOZHMKZ772SINHFX` (the deactivated root key id). Confirmed still present + (`grep -c` returns 2). These are **public identifiers, not secret pairs**; + they live in the `.ciagent/` audit narrative, not in any executable code + path. Recommended for a future hygiene redaction pass (replace with + `AKIA…SPIKE` / `AKIA…ROOT-DEACTIVATED` placeholders). **Non-blocking for v1.2 + ship; flagged for post-hoc review.** + +### Verdict: PASS — 1 carried-forward P1 (non-blocking). + +--- + +## Lens 4 — Performance + +Not a concern for the spike (plan-only, single resource, no load). **Skipped.** + +--- + +## Lens 5 — Maintainability + +The `acdl_platform/` rename, substrate-agnostic boundary, and decision trail +are all consistent. + +### Findings + +- **`acdl_platform/` rename (Phase 08 prep, fixing the stdlib `platform` + shadow):** consistently applied across `scripts/verify_phase06.sh`, + `scripts/verify_phase07.sh`, `README.md`, and the Python imports + (`import acdl_platform.confidence_signal as c` in `run_spike_e2e.sh`). + `grep -l acdl_platform` confirms all three files reference the renamed dir. + **Consistent.** +- **Decision trail:** every schema/module cites its source. Sampled 3 files: + - `acdl_platform/confidence_signal.py` cites `REQ-19`, `D-040`, + `ARCHITECTURE.md §8`. + - `acdl_platform/contract_resolver.py` cites `ARCHITECTURE.md §12.8`. + - `schemas/ir.schema.json` cites `ARCHITECTURE.md §12.1`, `§3`, `W3.D`. + **Citations present.** +- **Spike-vs-v1.2 boundary** documented in each design doc: + `acdl_platform/audit_ledger_design.md`, `acdl_platform/hitl_matrix_design.md`, + and `.ciagent/PLAN.md` all reference `v1.2`. **Boundary documented.** +- **Substrate-agnostic boundary (REQ-28):** the adapter is the only + substrate-specific code. `acdl_platform/` Python is clean (verified by the + Check (h) grep + the synthetic leak test). `modules-ir/` JSON data files + contain only IR types (`aws:s3:bucket`); `aws_s3_bucket` appears only in + `description`/`$comment` strings that explain the mapping. **Boundary holds.** + +### Verdict: PASS — no issues. + +--- + +## Lens 6 — Adversarial + +Tried to break the spike. All failure modes handled correctly. + +### Findings + +- **`contracts/spike.yaml` with `environment: prod` (missing runbook):** the + contract schema rejects it via the `allOf` if/then (`runbook` is a required + property when `environment == "prod"`). `contract_resolver.py` raises + `jsonschema.ValidationError` (not a generic ValueError). **Handled.** +- **IR instance with a resource type not in `TYPE_MAP` (e.g. + `aws:ec2:instance`):** the adapter raises + `ValueError("unknown IR type 'aws:ec2:instance' (adapter spike handles + aws:s3:bucket only)")`. **Handled.** +- **Confidence signal gets a critical-fail `PolicyCheckResult`:** hard-overrides + to `score=0.0`, `band=block`, `reasonCodes=['CRITICAL_OVERRIDE:...']`. The + short-circuit returns BEFORE the score clamp. **Handled.** +- **Outbox write fails (DynamoDB unreachable):** `outbox_writer.py` raises + (boto3 `put_item` propagates the exception); `run_spike_e2e.sh` line 93 uses + `|| fail "outbox write failed"` → exit 1. **Handled (no silent success).** +- **Missing confidence input (e.g. `nfrs` absent):** `compute()` returns + `Signal(0.0, "block", {}, ["INPUT_MISSING:nfrs"])`. **Handled.** + +### Verdict: PASS — no issues. + +--- + +## P0 / P1 / P2 Summary + +| Severity | Count | Action | +|-----------|-------|--------| +| **P0** | 0 | none (no auto-fix needed) | +| **P1** | 1 | P1-1 (carried-forward): two AWS access key IDs in `.ciagent/VERIFY.md` Phase 09 narrative — flagged for post-hoc hygiene redaction; non-blocking | +| **P2** | 0 | none | + +--- + +## Milestone verdict + +**v1.1 milestone: READY TO SHIP** + +- 0 P0 issues (no blocking fixes). +- 1 P1 carried-forward (non-blocking; flagged for post-hoc review). +- All 5 lenses pass. REQ-16..28 satisfied. The IR commitments hold (REQ-28). +- Ready for the COMPLETE gate → ship `v1.2.0` → audit. \ No newline at end of file