acdl/.ciagent/REVIEW.md

ACDL v1.10 — Multi-Persona Code Review

Reviewer: ci-code-reviewer (model: glm-5.2) Scope: v1.10 milestone — 6 commits (772ac72..5274bc4), 23 files, +2458/-419 lines Date: 2026-07-27

Commits reviewed

Commit	Phase	Type	Summary
772ac72	52	docs	v1.10 milestone plan (PLAN stage)
9897df0	52	fix	regression-class VERIFY (D-091)
217653d	53	feat	local emulating adapters (D-092)
44d1d19	54	fix	capability re-verification sweep — 7 adapter defects fixed
950db56	55	docs	rewrite PROJECT/ROADMAP/decks to verified reality
5274bc4	0	verify	4-layer milestone gate — PASS

P0 issues (1 — auto-fixed)

P0-1: TOCTOU race in LocalEcsEmulator.deploy() — FIXED

Persona: Correctness + Adversarial File: core/local_emulators.py:180-186 (pre-fix) Finding: deploy() opened a socket to find a free port, closed it, then bound TCPServer to that port. Between sock.close() and TCPServer(...), another process could grab the port (TOCTOU race), causing serve_forever to fail with OSError: Address already in use. This made the local E2E test flaky under port contention. Fix: Bind TCPServer directly to port 0 (the OS assigns a free port atomically); read the assigned port back from server_address[1]. No race window. Status: Auto-applied. All 13 local-emulator tests pass; 513 fast tests pass.

P1 issues (1 — flagged for post-hoc)

P1-1: run_local_e2e() os.chdir side-effect — FIXED (upgraded from P1)

Persona: Maintainability File: core/local_emulators.py:411 (pre-fix) Finding: run_local_e2e() called os.chdir(str(root)) as a side-effect without restoring the prior CWD. If called from a context that expects a specific CWD (e.g. a test runner), it would break subsequent tests. Fix: Wrapped the body in a try/finally that restores prior_cwd on exit. Status: Auto-applied (upgraded from P1 to P0-equivalent because it's a clear correctness issue with a trivial fix). All tests pass.

P2 issues (2 — flagged for post-hoc)

P2-1: Regression registry coverage gap (uptime-kuma + RDS)

Persona: Testing Finding: The regression registry covers microservice + static-assets stacks but not uptime-kuma or RDS. The adapter fixes in Phase 54 could theoretically regress those stacks without the gate catching it. Recommendation: Add uptime-kuma + RDS contracts to the regression registry in a future patch.

P2-2: f-string path interpolation in _check_outbox_writer

Persona: Maintainability File: core/regression_verify.py:236 Finding: _check_outbox_writer uses an f-string to embed a temp path into a python3 -c command (open('{event_path}')). Safe in practice (Linux temp paths have no single quotes) but fragile by design. Recommendation: Use -- arg passing or sys.argv instead of f-string interpolation in a future refactor.

Persona findings

Correctness — PASS (1 P0 auto-fixed)

• 7 adapter defects fixed in Phase 54; each traceable to a terraform validate/plan error.
• No duplicate outputs after the dedup fix (verified for both contracts).
• assume_role_policy JSON is valid (verified: inner JSON parses correctly).
• TOCTOU race in LocalEcsEmulator.deploy() — auto-fixed (P0-1).
• os.chdir side-effect in run_local_e2e — auto-fixed (P1-1, upgraded).

Testing — PASS (1 P2 flagged)

• 24 new tests (11 regression-mode + 13 local-emulator). All pass.
• Coverage: outbox write/chain/broken-chain/resume; ECS HTTP 200/destroy; S3 backend rewrite/state path; Lambda stub happy/missing-field; is_local_tier flag; full local E2E for both stacks.
• Gap: uptime-kuma + RDS not in registry (P2-1).

Security — PASS

• No AWS credentials logged (0 cred strings in reports; verified by grep).
• Local ECS binds 127.0.0.1 only (loopback; no external exposure).
• Local Lambda stub patches urllib.urlopen to a fake response (no network egress).
• No eval/exec/subprocess injection vectors in adapter changes (verified by diff grep).
• All STRIDE threats low-severity (auto-accepted per config).

Performance — PASS

• Regression run ~60s (16 capabilities). Slow checks (pytest, run_ci, terraform plan) are the bulk; acceptable for a milestone gate.
• Local ECS emulator: free port, daemon thread, clean destroy. No resource leak.
• No O(n^2) patterns in new code.

Maintainability — PASS (1 P1 auto-fixed, 1 P2 flagged)

• regression_verify.py (532 lines) well-structured: dataclass report, registry, run_regression entrypoint, write_report helper. Adding a capability = 1 function + 1 registry entry.
• local_emulators.py (489 lines) organized as 4 independent adapter classes + run_local_e2e convenience function.
• os.chdir side-effect fixed (P1-1).
• f-string path interpolation is fragile (P2-2).

Adversarial — PASS (1 P0 auto-fixed)

• Could the regression gate be bypassed? No — env vars (ACDL_REGRESSION_MILESTONE/PHASE) only affect metadata, not pass/fail.
• Could the local E2E mutate cloud? No — no terraform apply, no real put_item (only the flat-file stub).
• Could the TOCTOU race be exploited? The race window is small but real under port contention — fixed (P0-1).
• Could the adapter fixes regress an untested stack? Possible — P2-1 flagged.

Verdict

READY TO SHIP — 1 P0 auto-fixed (TOCTOU race), 1 P1 auto-fixed (os.chdir side-effect), 2 P2 flagged for post-hoc (regression registry coverage gap; f-string path interpolation). 513 fast tests + 5 slow local E2E tests pass after fixes. The v1.10 milestone is sound.