From 28d4645a0cedfae17d6df1c15f70d94e3ebc4111 Mon Sep 17 00:00:00 2001 From: Jon Chery Date: Mon, 27 Jul 2026 18:46:05 +0000 Subject: [PATCH] =?UTF-8?q?verify(v1.10):=20code=20review=20=E2=80=94=201?= =?UTF-8?q?=20P0=20auto-fixed,=201=20P1=20auto-fixed,=202=20P1+=20flagged?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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--- --- .ciagent/REVIEW.md | 125 +++++++++++++++++----------------- core/local_emulators.py | 147 +++++++++++++++++++++------------------- 2 files changed, 137 insertions(+), 135 deletions(-) diff --git a/.ciagent/REVIEW.md b/.ciagent/REVIEW.md index 2d47e88..36d1773 100644 --- a/.ciagent/REVIEW.md +++ b/.ciagent/REVIEW.md @@ -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. \ No newline at end of file +**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. \ No newline at end of file diff --git a/core/local_emulators.py b/core/local_emulators.py index 493bd8e..6c272fc 100644 --- a/core/local_emulators.py +++ b/core/local_emulators.py @@ -177,14 +177,15 @@ class LocalEcsEmulator: def log_message(self, *a, **k): pass # silence - # Bind to a free port. - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.bind(("127.0.0.1", 0)) - self._port = sock.getsockname()[1] - sock.close() + # Bind directly to port 0 (the OS assigns a free port atomically). + # The prior approach (open a socket, read the port, close, then + # bind TCPServer) was a TOCTOU race: another process could grab + # the port between close and bind. Binding to port 0 avoids the + # race entirely. self._server = socketserver.TCPServer( - ("127.0.0.1", self._port), Handler) + ("127.0.0.1", 0), Handler) self._server.allow_reuse_address = True + self._port = self._server.server_address[1] self._thread = threading.Thread( target=self._server.serve_forever, daemon=True) self._thread.start() @@ -408,78 +409,82 @@ def run_local_e2e(contract_path: str, repo_root: Optional[Path] = None) -> Dict[ Returns a dict of results. Raises AssertionError on any failure. """ root = Path(repo_root) if repo_root else ROOT + prior_cwd = os.getcwd() os.chdir(str(root)) - sys.path.insert(0, str(root)) - from core.contract_resolver import resolve - import adapters.terraform.adapter as adapter + try: + sys.path.insert(0, str(root)) + from core.contract_resolver import resolve + import adapters.terraform.adapter as adapter - stack = resolve(contract_path, str(root)) - stack_name = stack["stack"]["name"] - work = Path(tempfile.mkdtemp(prefix="acdl_local_e2e_")) - tf_dir = work / "tf" - tf_dir.mkdir(exist_ok=True) - adapter.adapt(stack, str(tf_dir)) + stack = resolve(contract_path, str(root)) + stack_name = stack["stack"]["name"] + work = Path(tempfile.mkdtemp(prefix="acdl_local_e2e_")) + tf_dir = work / "tf" + tf_dir.mkdir(exist_ok=True) + adapter.adapt(stack, str(tf_dir)) - # 3. Local S3 state backend rewrite. - backend = LocalS3StateBackend.create(dir=work / "tfstate") - tf_tf = tf_dir / "terraform.tf" - backend.rewrite_terraform_tf(tf_tf, stack_name) - assert "backend \"local\"" in tf_tf.read_text(), "backend not rewritten" + # 3. Local S3 state backend rewrite. + backend = LocalS3StateBackend.create(dir=work / "tfstate") + tf_tf = tf_dir / "terraform.tf" + backend.rewrite_terraform_tf(tf_tf, stack_name) + assert "backend \"local\"" in tf_tf.read_text(), "backend not rewritten" - # 4. Local ECS emulator (only if the stack has an ECS service). - ecs_result = None - has_ecs = any(r["type"] == "aws:ecs:service" for r in stack["resources"]) - if has_ecs: - ecs = LocalEcsEmulator( - service_name=stack_name, - service_definition={"desired_count": 1}, - ) - deploy_meta = ecs.deploy() - ok, status = ecs.health_check(deploy_meta["endpoint"]) - assert ok, f"ECS emulator health check failed: status={status}" - ecs_result = deploy_meta - ecs.destroy() + # 4. Local ECS emulator (only if the stack has an ECS service). + ecs_result = None + has_ecs = any(r["type"] == "aws:ecs:service" for r in stack["resources"]) + if has_ecs: + ecs = LocalEcsEmulator( + service_name=stack_name, + service_definition={"desired_count": 1}, + ) + deploy_meta = ecs.deploy() + ok, status = ecs.health_check(deploy_meta["endpoint"]) + assert ok, f"ECS emulator health check failed: status={status}" + ecs_result = deploy_meta + ecs.destroy() - # 5. Flat-file outbox: write a CONFIDENCE_COMPUTED event + verify chain. - outbox = FlatFileOutbox.create(dir=work / "outbox") - event = { - "contractId": "local-e2e-test", - "eventType": "CONFIDENCE_COMPUTED", - "ts": datetime.datetime.now(datetime.timezone.utc) - .strftime("%Y-%m-%dT%H:%M:%SZ"), - "environment": "dev", - "stack": stack_name, - "score": 0.9, - "band": "pass", - "prev_event_hash": "GENESIS", - } - item = outbox.write_event(event) - assert item["hash"], "outbox item missing hash" - assert outbox.verify_chain(), "outbox hash chain broken" + # 5. Flat-file outbox: write a CONFIDENCE_COMPUTED event + verify chain. + outbox = FlatFileOutbox.create(dir=work / "outbox") + event = { + "contractId": "local-e2e-test", + "eventType": "CONFIDENCE_COMPUTED", + "ts": datetime.datetime.now(datetime.timezone.utc) + .strftime("%Y-%m-%dT%H:%M:%SZ"), + "environment": "dev", + "stack": stack_name, + "score": 0.9, + "band": "pass", + "prev_event_hash": "GENESIS", + } + item = outbox.write_event(event) + assert item["hash"], "outbox item missing hash" + assert outbox.verify_chain(), "outbox hash chain broken" - # 6. Local Lambda stub: invoke the contract_ingestor handler. - lambda_stub = LocalLambdaStub(outbox=outbox) - lambda_result = lambda_stub.invoke({ - "action": "submit_contract", - "consumerRepo": "local-test/consumer", - "contractId": "local-e2e-test", - "contract": {"module": stack_name, "environment": "dev"}, - "environment": "dev", - }) - assert lambda_result["statusCode"] == 200, ( - f"lambda stub returned {lambda_result['statusCode']}: {lambda_result.get('body')}") + # 6. Local Lambda stub: invoke the contract_ingestor handler. + lambda_stub = LocalLambdaStub(outbox=outbox) + lambda_result = lambda_stub.invoke({ + "action": "submit_contract", + "consumerRepo": "local-test/consumer", + "contractId": "local-e2e-test", + "contract": {"module": stack_name, "environment": "dev"}, + "environment": "dev", + }) + assert lambda_result["statusCode"] == 200, ( + f"lambda stub returned {lambda_result['statusCode']}: {lambda_result.get('body')}") - return { - "stack_name": stack_name, - "tier": "local-emulator", - "tf_dir": str(tf_dir), - "backend": "local", - "ecs": ecs_result, - "outbox_dir": str(outbox.dir), - "outbox_events": len(outbox.read_all()), - "outbox_chain_verified": True, - "lambda_status": lambda_result["statusCode"], - } + return { + "stack_name": stack_name, + "tier": "local-emulator", + "tf_dir": str(tf_dir), + "backend": "local", + "ecs": ecs_result, + "outbox_dir": str(outbox.dir), + "outbox_events": len(outbox.read_all()), + "outbox_chain_verified": True, + "lambda_status": lambda_result["statusCode"], + } + finally: + os.chdir(prior_cwd) if __name__ == "__main__":