review(v1.1): READY TO SHIP — multi-persona code review
---ci--- project: acdl phase: 0 milestone: v1.1 status: review verdict: READY TO SHIP p0: 0 p1: 1 ---/ci---
This commit is contained in:
Jon Chery
@@ -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.
|
||||
Reference in New Issue
Block a user