Jon Chery
28d4645a0c
Multi-persona review of the v1.10 milestone (6 commits, 23 files).
P0-1 (auto-fixed): TOCTOU race in LocalEcsEmulator.deploy() — opened a
socket to find a free port, closed it, then bound TCPServer to that
port. Between close and bind, another process could grab the port,
causing serve_forever to fail with OSError: Address already in use.
Fix: bind TCPServer directly to port 0 (OS assigns a free port
atomically); read the assigned port back from server_address[1].
P1-1 (auto-fixed, upgraded): run_local_e2e() called os.chdir() as a
side-effect without restoring the prior CWD. Fix: wrapped the body in
try/finally that restores prior_cwd on exit.
P2-1 (flagged): regression registry covers microservice + static-assets
but not uptime-kuma or RDS stacks. Recommend adding in a future patch.
P2-2 (flagged): _check_outbox_writer uses an f-string to embed a temp
path into a python3 -c command. Safe in practice but fragile by design.
Verified after fixes: 513 fast tests + 5 slow local E2E tests pass.
No regressions.
---ci---
project: acdl
phase: 0
milestone: v1.10
status: verify
lessons:
- P0 fix: TOCTOU race in LocalEcsEmulator.deploy() — bind to port 0
directly instead of open/close/rebind.
- P1 fix: os.chdir side-effect in run_local_e2e() — restore prior
CWD in a finally block.
- The regression registry should be expanded to cover all L2 stacks
(uptime-kuma, RDS) to prevent untested-stack regressions.
---/ci---
89 lines
5.5 KiB
Markdown
89 lines
5.5 KiB
Markdown
# 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. |