From 9ba390a595a7e66ae1e28563655430afce0400ae Mon Sep 17 00:00:00 2001 From: Aymen Hammouda Date: Sat, 3 Oct 2026 08:35:30 +0200 Subject: [PATCH] fix(vision): allow full-index reviews to finish and expose failures --- AGENT-EXECUTION-PIPELINE.md | 7 ++-- ops/vision/README.md | 9 ++++- ops/vision/control.py | 35 ++++++++++++++++--- ops/vision/install.py | 6 ++++ tests/test_vision_control.py | 68 ++++++++++++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 7 deletions(-) diff --git a/AGENT-EXECUTION-PIPELINE.md b/AGENT-EXECUTION-PIPELINE.md index 217d7b4..b5f85b0 100644 --- a/AGENT-EXECUTION-PIPELINE.md +++ b/AGENT-EXECUTION-PIPELINE.md @@ -394,11 +394,14 @@ Until activation, authenticated project operations fail closed. regressions, installation success and latency, not stars/downloads alone. Preserve this evidence in issues/PRs and `state/project.json`; revisit planned outcomes each owner cycle when their review date is due. -- Keep one implementation in flight, leaf workers, a 900-second worker budget, +- Keep one implementation in flight, leaf workers, a 900-second implementer budget, + a 1500-second independent review deadline (1560-second broker wait), the 1800-second owner deadline, and a checkpoint within 25 minutes. The broker serializes mutations and verification; two failed reviews of the same head/base open its circuit. Stop repeated attempts without a concrete new hypothesis. -- Preserve exactly two owner runs daily: 08:17 and 20:17 Europe/Paris. Reuse the +- Aymen changed the schedule on 2026-10-02: run every two hours at minute 17, + Europe/Paris. Start publication/verification with at least 27 minutes left in + the owner deadline, or checkpoint it for the next cycle. Reuse the existing job and state. Notify only meaningful results, failure or required operator action. Record a credential blocker once; do not repeat it every run. - Release only a verified main commit with passing main checks and matching diff --git a/ops/vision/README.md b/ops/vision/README.md index 07b7b86..1354cbc 100644 --- a/ops/vision/README.md +++ b/ops/vision/README.md @@ -2,7 +2,7 @@ Vision owns `ayhammouda/python-docs-mcp-server`: maintenance, issue replies, research-driven development, merges and releases. The existing owner job runs -at **08:17 and 20:17 Europe/Paris**. Account ownership stays with Aymen. +**every two hours at minute 17, Europe/Paris**. Account ownership stays with Aymen. ## Finish GitHub identity setup later @@ -96,6 +96,13 @@ only for the identical head/base and installed policy. Different code or main requires new review. Two failed reviews of identical content open the repair circuit. A global lock serializes publication, verification and merging. +Independent reviews have a 25-minute deadline so a clean three-version index can +finish. The broker waits one further minute for the CLI response. Start review +only with 27 minutes left in the 30-minute owner cycle, or checkpoint the prepared +commit for the next cycle. Use a 1620-second exec timeout with short yields and +process polling. `pdctl status` exposes failed review head/base, session and a +bounded error; subprocess output and credentials are never included. + The verifier checks the full diff and runs the locked commands itself. The required GitHub check is issued only through its separate App, then main/head are rechecked before merge. GitHub enforces all other required checks, including the CodeQL findings diff --git a/ops/vision/control.py b/ops/vision/control.py index 2aab741..a407ae3 100644 --- a/ops/vision/control.py +++ b/ops/vision/control.py @@ -35,6 +35,7 @@ SHA = re.compile(r"[0-9a-f]{40}\Z") BRANCH = re.compile(r"(?:codex|agent)/[A-Za-z0-9][A-Za-z0-9._/-]{0,150}\Z") MAX_BYTES = 8 * 1024 * 1024 +REVIEW_TIMEOUT = 1500 ENV = { "PATH": "/usr/bin:/bin:/home/linuxbrew/.linuxbrew/bin:/home/ahammouda/.local/bin", "LANG": "C.UTF-8", @@ -321,6 +322,7 @@ def review(head: str, base: str, decision: dict): if attempts["failures"] >= 2: raise ValueError("Repair circuit open for unchanged content: new revision required") reset_verifier() + session_id = str(uuid.uuid4()) try: # The reviewer is a separate OpenClaw agent with an SSH sandbox and no GitHub identity. prompt = ( @@ -353,17 +355,17 @@ def review(head: str, base: str, decision: dict): "--agent", "pd-verifier", "--session-id", - str(uuid.uuid4()), + session_id, "--message", prompt, "--json", "--timeout", - "900", + str(REVIEW_TIMEOUT), ], env=ENV, capture_output=True, text=True, - timeout=960, + timeout=REVIEW_TIMEOUT + 60, check=True, ) response = json.loads(run.stdout[run.stdout.index("{") :]) @@ -392,10 +394,28 @@ def review(head: str, base: str, decision: dict): + str(verdict.get("blockers") or "incomplete check evidence") ) checkpoint.write_text(json.dumps(verdict)) + attempts["status"] = "success" + circuit.write_text(json.dumps(attempts)) return verdict - except Exception: + except Exception as exc: + if isinstance(exc, subprocess.TimeoutExpired): + reason = f"Independent review execution timed out after {REVIEW_TIMEOUT + 60}s" + elif isinstance(exc, subprocess.CalledProcessError): + reason = f"Independent review execution failed with exit {exc.returncode}" + else: + reason = f"Independent review failed ({type(exc).__name__})" attempts["failures"] += 1 + attempts.update( + status="failure", + head_sha=head, + base_sha=base, + tree_sha=tree, + session_id=session_id, + last_error=reason, + ) circuit.write_text(json.dumps(attempts)) + if isinstance(exc, (subprocess.TimeoutExpired, subprocess.CalledProcessError)): + raise ValueError(f"{reason}; verifier session {session_id}") from None raise finally: reset_verifier() @@ -547,6 +567,13 @@ def dispatch(data: dict): "ready": (CONFIG / "activated").exists(), "authentication": "temporary-token" if temporary_auth() else "github-apps", "releases_enabled": (CONFIG / "activated").exists() and not temporary_auth(), + "review_failures": [ + result + for p in STATE.glob( + f"attempts-{hashlib.sha256(Path(__file__).read_bytes()).hexdigest()[:16]}-*.json" + ) + if (result := json.loads(p.read_text())).get("status") == "failure" + ], "blocked_verifications": [ json.loads(p.read_text()) for p in STATE.glob("verify-*.json") diff --git a/ops/vision/install.py b/ops/vision/install.py index cf3c1e7..499a3a7 100644 --- a/ops/vision/install.py +++ b/ops/vision/install.py @@ -50,6 +50,12 @@ The implementer must return a commit bundle in /var/lib/python-docs/exchange/implementation. Import that bundle into your repository, then pdctl publish its committed diff with --decision. Publication itself waits for independent review before creating a runnable GitHub ref. +Independent review can take 25 minutes while rebuilding official docs. Start +pdctl publish/verify only with at least 27 minutes left in the owner deadline; +otherwise checkpoint the prepared commit for the next cycle. Use an exec timeout +of 1620 seconds and short yields with process polling; do not kill a live review +at an earlier client timeout. Record review_failures from pdctl status, including +the verifier session ID, instead of retrying an unexplained failure. Obtain independent verification through pdctl verify, never by self-assertion. pdctl merge performs a SHA-matched merge only after the verifier succeeds. Use pdctl release COMMIT vX.Y.Z only after successful main CI; tags are immutable. diff --git a/tests/test_vision_control.py b/tests/test_vision_control.py index 0540bc5..e4ee02c 100644 --- a/tests/test_vision_control.py +++ b/tests/test_vision_control.py @@ -4,6 +4,7 @@ import hashlib import json import runpy +import subprocess from pathlib import Path import pytest @@ -11,6 +12,73 @@ CONTROL = runpy.run_path(str(Path(__file__).parents[1] / "ops/vision/control.py")) +@pytest.mark.parametrize("failure_kind", ["exit", "timeout", "rejected", "malformed"]) +def test_review_deadline_and_failure_handoff(tmp_path, monkeypatch, failure_kind): + from types import SimpleNamespace + + module = CONTROL["review"].__globals__ + monkeypatch.setitem(module, "CONFIG", tmp_path) + monkeypatch.setitem(module, "STATE", tmp_path) + monkeypatch.setitem(module, "api", lambda *_: {"tree": {"sha": "c" * 40}}) + cleanups = [] + monkeypatch.setitem(module, "reset_verifier", lambda: cleanups.append(True)) + calls = [] + + def fail(args, **kwargs): + calls.append((args, kwargs)) + if failure_kind == "timeout": + raise subprocess.TimeoutExpired(args, kwargs["timeout"], stderr="private-output") + if failure_kind == "exit": + raise subprocess.CalledProcessError(1, args, stderr="private-output") + verdict = { + "head_sha": "a" * 40, + "base_sha": "b" * 40, + "approved": False, + "blockers": ["private-output"], + "commands": [], + } + text = json.dumps(verdict) if failure_kind == "rejected" else "private-output" + return SimpleNamespace(stdout=json.dumps({"payloads": [{"text": text}]})) + + monkeypatch.setattr(module["subprocess"], "run", fail) + with pytest.raises(ValueError) as error: + CONTROL["review"]("a" * 40, "b" * 40, {}) + args, kwargs = calls[0] + assert args[args.index("--timeout") + 1] == "1500" + assert kwargs["timeout"] == 1560 + assert len(cleanups) == 2 + status = CONTROL["dispatch"]({"operation": "status"}) + failure = status["review_failures"][0] + assert failure["failures"] == 1 + assert failure["head_sha"] == "a" * 40 + assert failure["base_sha"] == "b" * 40 + assert failure["session_id"] == args[args.index("--session-id") + 1] + assert "Independent review" in failure["last_error"] + assert "private-output" not in json.dumps(status) + if failure_kind in {"exit", "timeout"}: + assert "private-output" not in str(error) + verdict = { + "head_sha": "a" * 40, + "base_sha": "b" * 40, + "approved": True, + "blockers": [], + "commands": [ + {"command": command, "exit_code": 0} + for command in ["uv sync --locked --dev", "ruff check", "pyright", "pytest"] + ], + } + monkeypatch.setattr( + module["subprocess"], + "run", + lambda *args, **kwargs: SimpleNamespace( + stdout=json.dumps({"payloads": [{"text": json.dumps(verdict)}]}) + ), + ) + assert CONTROL["review"]("a" * 40, "b" * 40, {}) == verdict + assert CONTROL["dispatch"]({"operation": "status"})["review_failures"] == [] + assert json.loads(next(tmp_path.glob("attempts-*.json")).read_text())["failures"] == 1 + + def test_api_rejects_credentials_protections_checks_merges_and_other_repositories(): validate = CONTROL["validate_api"] validate("GET", "issues?state=open", None)