verify(v1.10): code review — 1 P0 auto-fixed, 1 P1 auto-fixed, 2 P1+ flagged
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---
This commit is contained in:
+61
-64
@@ -1,7 +1,8 @@
|
||||
# ACDL v1.10 — Review
|
||||
# ACDL v1.10 — Multi-Persona Code Review
|
||||
|
||||
> Review date: 2026-07-27. Reviewer: ci-code-reviewer. Milestone: v1.10.
|
||||
> Scope: 5 commits (772ac72..950db56), 22 files, +2281/-256 lines.
|
||||
**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
|
||||
|
||||
@@ -12,81 +13,77 @@
|
||||
| 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
|
||||
## P0 issues (1 — auto-fixed)
|
||||
|
||||
**0 P0.** No correctness, security, or data-loss issues found.
|
||||
### 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
|
||||
## P1 issues (1 — flagged for post-hoc)
|
||||
|
||||
**0 P1.** No maintainability or design issues requiring post-hoc review.
|
||||
### 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.
|
||||
|
||||
## Correctness
|
||||
## P2 issues (2 — flagged for post-hoc)
|
||||
|
||||
- The regression-class VERIFY (D-091) correctly fails closed on any
|
||||
non-Verified capability. The decay-surfacing test proves the gate
|
||||
catches Broken. The 16-capability registry covers both local and
|
||||
live-AWS tiers.
|
||||
- The 7 adapter defect fixes are each traceable to a specific
|
||||
`terraform validate/plan` error they resolved. The fixes are
|
||||
defensive (skip-in-generic-loop + emit-in-type-specific-block) and
|
||||
do not regress the static-assets stack (verified: terraform plan
|
||||
passes for both contracts).
|
||||
- The local emulators (D-092) correctly emulate ECS, outbox, S3 state,
|
||||
and Lambda without cloud credentials. The headline E2E runs
|
||||
end-to-end against the local tier.
|
||||
**PASS.**
|
||||
### 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.
|
||||
|
||||
## Security
|
||||
### 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.
|
||||
|
||||
- No AWS credentials logged. The regression module reads
|
||||
`.env.secrets` and passes creds via env vars to subprocesses only.
|
||||
- The local ECS emulator binds to 127.0.0.1 (loopback) only.
|
||||
- The local Lambda stub patches `urllib.urlopen` to a fake response
|
||||
so `report_error` does not hit the network.
|
||||
- No new network calls or cloud mutations introduced (plan-only).
|
||||
**PASS.**
|
||||
## Persona findings
|
||||
|
||||
## Performance
|
||||
### 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).
|
||||
|
||||
- The regression run completes in ~60s (16 capabilities). The slow
|
||||
checks (CAP-009 pytest, CAP-010 run_ci, CAP-013/014 terraform plan)
|
||||
are the bulk; acceptable for a milestone gate.
|
||||
**PASS.**
|
||||
### 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).
|
||||
|
||||
## Maintainability
|
||||
### 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).
|
||||
|
||||
- `core/regression_verify.py` (532 lines) is well-structured: a
|
||||
dataclass report, a registry of capability checks, a `run_regression`
|
||||
entrypoint, and a `write_report` helper. Adding a new capability is a
|
||||
single function + registry entry.
|
||||
- `core/local_emulators.py` (489 lines) is organized as four
|
||||
independent adapter classes + a `run_local_e2e` convenience function.
|
||||
- The adapter defect fixes are localized (skip lists + type-specific
|
||||
default blocks); no large refactors.
|
||||
**PASS.**
|
||||
### 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.
|
||||
|
||||
## Adversarial
|
||||
### 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).
|
||||
|
||||
- Could the regression gate pass while cloud resources are actually
|
||||
broken? No — the live-AWS checks (CAP-013..CAP-016) probe the real
|
||||
AWS account. The 6 IAM-gated resources are explicitly escalated, not
|
||||
silently passed.
|
||||
- Could the local emulators mask a real cloud failure? No — the local
|
||||
tier is additive; the live-AWS tier (CAP-013/014) runs the real
|
||||
terraform plan. The emulators prove runtime behavior; the live plan
|
||||
proves deployment correctness.
|
||||
- Could the adapter fixes introduce a regression in a stack not tested?
|
||||
Possible — the registry covers microservice + static-assets. The
|
||||
uptime-kuma and RDS stacks are not in the registry. **P2
|
||||
(post-hoc):** add uptime-kuma + RDS contracts to the regression
|
||||
registry in a future patch.
|
||||
**PASS (1 P2 flagged for post-hoc).**
|
||||
### 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** — 0 P0, 0 P1, 1 P2 (post-hoc: expand regression
|
||||
registry to uptime-kuma + RDS stacks). The v1.10 milestone achieves
|
||||
its goal: the pipeline regression gap is fixed, the platform is fully
|
||||
locally testable, every advertised capability is re-verified, and the
|
||||
docs/decks match verified reality.
|
||||
**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.
|
||||
Reference in New Issue
Block a user