diff --git a/loopx/canary/module_metric_baseline.json b/loopx/canary/module_metric_baseline.json index 5bed5aa2d..4543a99b0 100644 --- a/loopx/canary/module_metric_baseline.json +++ b/loopx/canary/module_metric_baseline.json @@ -93,7 +93,7 @@ "loopx/todos.py": { "any_count": 48, "dict_any_count": 0, - "lines": 2142 + "lines": 2318 }, "loopx/worker_bridge.py": { "any_count": 28, diff --git a/loopx/capabilities/issue_fix/acceptance_loop.py b/loopx/capabilities/issue_fix/acceptance_loop.py index 4d5f4bd7f..4d580b9af 100644 --- a/loopx/capabilities/issue_fix/acceptance_loop.py +++ b/loopx/capabilities/issue_fix/acceptance_loop.py @@ -2,7 +2,6 @@ import hashlib import os -import shlex import shutil import subprocess import sys @@ -13,6 +12,10 @@ from typing import Any from .intake_surface import build_content_ops_issue_fix_metadata_preview_packet +from ...control_plane.runtime.validation_command import ( + run_caller_validation as _run_caller_validation, + require_validation_passed as _require_passed, +) ISSUE_FIX_ACCEPTANCE_LOOP_SCHEMA_VERSION = "issue_fix_acceptance_loop_v0" @@ -247,39 +250,9 @@ def add_lines(lines: list[str]) -> None: return files[:20], truncated -def _run_caller_validation( - workspace: Path, - *, - validation_command: str, - validation_label: str, - timeout_seconds: int, -) -> dict[str, Any]: - argv = shlex.split(validation_command) - if not argv: - raise ValueError("validation_command must not be empty") - result = subprocess.run( - argv, - cwd=workspace, - env={**os.environ, "PYTHONDONTWRITEBYTECODE": "1"}, - text=True, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - timeout=timeout_seconds, - ) - return { - "schema_version": "issue_fix_validation_command_v0", - "command_label": validation_label or "caller-declared validation", - "exit_code": result.returncode, - "passed": result.returncode == 0, - "stdout_captured": False, - "stderr_captured": False, - "local_path_captured": False, - } - - -def _require_passed(step: Mapping[str, Any]) -> None: - if step.get("passed") is not True: - raise RuntimeError(f"{step.get('command_label')} failed with exit code {step.get('exit_code')}") +# Caller-validation execution (_run_caller_validation / _require_passed) is +# imported from control_plane.runtime.validation_command so the todo-completion +# path can reuse the same privacy-safe handler. def _remove_temporary_git_workspace(workspace: Path) -> None: diff --git a/loopx/cli_commands/todo.py b/loopx/cli_commands/todo.py index 4ad023de7..d35d73cfa 100644 --- a/loopx/cli_commands/todo.py +++ b/loopx/cli_commands/todo.py @@ -111,6 +111,19 @@ def register_todo_command( todo_parser.add_argument("--status", choices=["open", "done", "blocked", "deferred"], help="For todo add/update, set the lifecycle status.") todo_parser.add_argument("--note", help="Public-safe note to attach to a lifecycle transition.") todo_parser.add_argument("--evidence", help="Public-safe evidence pointer or short result for complete/update.") + todo_parser.add_argument( + "--validation-command", + help=( + "Caller-approved validation command (no shell) to run before a " + "todo's completion commits, e.g. 'pytest -q tests/test_x.py'. Set " + "on `todo add`; completion runs it independently and blocks on a " + "non-zero exit." + ), + ) + todo_parser.add_argument( + "--validation-label", + help="Optional public-safe label for the validation receipt.", + ) todo_parser.add_argument("--reason", help="Public-safe reason for blocked/deferred/supersede transitions.") todo_parser.add_argument( "--authority-reason", @@ -502,6 +515,8 @@ def handle_todo_command( agent_id=args.agent_id, unblocks_todo_id=args.unblocks_todo_id, resume_when=args.resume_when, + validation_command=args.validation_command, + validation_label=args.validation_label, monitor_metadata={ "target_key": args.monitor_target_key, "cadence": args.cadence, diff --git a/loopx/cli_commands/todo_argument_validation.py b/loopx/cli_commands/todo_argument_validation.py index 7f00ca386..871efbb5b 100644 --- a/loopx/cli_commands/todo_argument_validation.py +++ b/loopx/cli_commands/todo_argument_validation.py @@ -45,6 +45,8 @@ ("--unblocks-todo-id", "unblocks_todo_id"), ("--successor-todo-id", "successor_todo_ids"), ("--resume-when", "resume_when"), + ("--validation-command", "validation_command"), + ("--validation-label", "validation_label"), ("--clear-resume-when", "clear_resume_when"), ("--target-key", "monitor_target_key"), ("--cadence", "cadence"), diff --git a/loopx/control_plane/runtime/validation_command.py b/loopx/control_plane/runtime/validation_command.py new file mode 100644 index 000000000..0020f2222 --- /dev/null +++ b/loopx/control_plane/runtime/validation_command.py @@ -0,0 +1,58 @@ +from __future__ import annotations + +import os +import shlex +import subprocess +from collections.abc import Mapping +from pathlib import Path +from typing import Any + +# Frozen wire id shared with capabilities/issue_fix. Kept verbatim for wire +# compatibility with existing caller-repo-branch receipts. +CALLER_VALIDATION_RECEIPT_SCHEMA_VERSION = "issue_fix_validation_command_v0" + + +def run_caller_validation( + workspace: Path, + *, + validation_command: str, + validation_label: str, + timeout_seconds: int, +) -> dict[str, Any]: + """Run a caller-approved validation command and return a privacy-safe receipt. + + The command is ``shlex.split`` (no shell) and run with ``cwd=workspace``. + Only ``exit_code`` and the boolean ``passed`` are recorded; command stdout, + stderr, and local paths are deliberately not captured. A timeout raises + ``subprocess.TimeoutExpired``; callers decide whether to convert that into + a failure receipt. + """ + argv = shlex.split(validation_command) + if not argv: + raise ValueError("validation_command must not be empty") + result = subprocess.run( + argv, + cwd=workspace, + env={**os.environ, "PYTHONDONTWRITEBYTECODE": "1"}, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + timeout=timeout_seconds, + ) + return { + "schema_version": CALLER_VALIDATION_RECEIPT_SCHEMA_VERSION, + "command_label": validation_label or "caller-declared validation", + "exit_code": result.returncode, + "passed": result.returncode == 0, + "stdout_captured": False, + "stderr_captured": False, + "local_path_captured": False, + } + + +def require_validation_passed(step: Mapping[str, Any]) -> None: + """Raise ``RuntimeError`` unless the validation step ``passed``.""" + if step.get("passed") is not True: + raise RuntimeError( + f"{step.get('command_label')} failed with exit code {step.get('exit_code')}" + ) diff --git a/loopx/control_plane/todos/contract.py b/loopx/control_plane/todos/contract.py index 95aa2a005..5ab4c1852 100644 --- a/loopx/control_plane/todos/contract.py +++ b/loopx/control_plane/todos/contract.py @@ -1090,6 +1090,8 @@ def _metadata_value_is_present(value: Any) -> bool: "completed_at", "updated_at", "completion_turn_key", + "validation_command", + "validation_label", ) ), _TodoMetadataField( @@ -1225,6 +1227,8 @@ def format_todo_metadata_line( note: str | None = None, evidence: str | None = None, completion_turn_key: str | None = None, + validation_command: str | None = None, + validation_label: str | None = None, reason: str | None = None, completed_at: str | None = None, updated_at: str | None = None, diff --git a/loopx/control_plane/todos/markdown.py b/loopx/control_plane/todos/markdown.py index a778e256a..80798aff8 100644 --- a/loopx/control_plane/todos/markdown.py +++ b/loopx/control_plane/todos/markdown.py @@ -179,4 +179,21 @@ def render_todo_markdown(payload: dict[str, Any]) -> str: f"- status: `{apply_result.get('status')}`", ] ) + validation = payload.get("validation") + if isinstance(validation, dict): + lines.extend( + [ + "", + "## Validation", + "", + f"- validation_blocked_completion: `{payload.get('validation_blocked_completion')}`", + f"- command_label: `{validation.get('command_label')}`", + f"- passed: `{validation.get('passed')}`", + f"- status: `{validation.get('status')}`", + f"- exit_code: `{validation.get('exit_code')}`", + ] + ) + summary = validation.get("summary") + if summary: + lines.append(f"- summary: {summary}") return "\n".join(lines) diff --git a/loopx/todos.py b/loopx/todos.py index 982d45f96..c327e2450 100644 --- a/loopx/todos.py +++ b/loopx/todos.py @@ -1,5 +1,6 @@ from __future__ import annotations +import subprocess from contextlib import ExitStack from pathlib import Path from typing import Any @@ -8,8 +9,13 @@ from .file_lock import exclusive_file_lock from .history import load_registry from .paths import resolve_runtime_root +from .materials import find_registry_goal, goal_repo from .rollout_event_log import load_rollout_events, rollout_event_log_path from .control_plane.runtime.local_state_write_correctness import build_todo_write_correctness_dry_run_packet +from .control_plane.runtime.validation_command import ( + CALLER_VALIDATION_RECEIPT_SCHEMA_VERSION, + run_caller_validation, +) from .state_refresh import now_local, resolve_goal_state from .status import ( MAX_ACTIVE_DONE_TODOS_BEFORE_ARCHIVE, @@ -622,6 +628,8 @@ def add_todo_to_lines( global_gate: bool | None = None, unblocks_todo_id: str | None = None, resume_when: str | None = None, + validation_command: str | None = None, + validation_label: str | None = None, monitor_metadata: dict[str, Any] | None = None, evidence: str | None = None, updated_at: str | None = None, @@ -700,6 +708,8 @@ def add_todo_to_lines( global_gate=global_gate, unblocks_todo_id=unblocks_todo_id, resume_when=normalized_resume_when, + validation_command=validation_command, + validation_label=validation_label, **normalized_monitor_metadata, evidence=evidence, updated_at=updated_at, @@ -877,6 +887,8 @@ def add_goal_todo( agent_id: str | None = None, unblocks_todo_id: str | None = None, resume_when: str | None = None, + validation_command: str | None = None, + validation_label: str | None = None, monitor_metadata: dict[str, Any] | None = None, project: Path | None = None, state_file: Path | None = None, @@ -1054,6 +1066,8 @@ def add_goal_todo( global_gate=True if global_gate else None, unblocks_todo_id=normalized_unblocks_todo_id, resume_when=normalized_resume_when, + validation_command=validation_command, + validation_label=validation_label, monitor_metadata=normalized_monitor_metadata, updated_at=updated_at, ) @@ -1504,6 +1518,134 @@ def update_goal_todo( ) +# Kept safely under the 30s outer CLI/MCP subprocess budget so a timed-out +# validation still produces a typed receipt before the outer call is killed. +_COMPLETION_VALIDATION_TIMEOUT_SECONDS = 20 + + +def _resolve_goal_repo_workspace(registry_path: Path, goal_id: str) -> Path | None: + """Resolve the goal's repository directory to use as the validation workspace.""" + goal = find_registry_goal(load_registry(registry_path), goal_id) + if goal is None: + return None + repo = goal_repo(goal) + if repo is None or not repo.is_dir(): + return None + return repo + + +def _read_declared_validation( + *, state_file: Path, todo_id: str, role: str | None +) -> tuple[str | None, str | None, bool]: + """Pre-read a todo's declared validation command without the mutation lock. + + Returns ``(validation_command, validation_label, already_completed)`` from + the markdown state file, or ``(None, None, False)`` when the todo is not + materialized in markdown. Read-only; safe to call before acquiring the + state-file lock so a slow validation command does not block concurrent todo + operations on the same goal (the MUTATION lock deadline is 5s). + ``validation_command`` is set only at ``todo add`` and has no update path, + so the value cannot drift between this pre-read and the in-lock commit. + """ + try: + lines = state_file.read_text(encoding="utf-8").splitlines() + except FileNotFoundError: + return None, None, False + match = find_todo_block(lines, todo_id=todo_id, role=role) + if not match: + return None, None, False + _role, _section, _start, _end, block = match + return ( + block.get("validation_command") or None, + block.get("validation_label") or None, + block.get("status") == TODO_STATUS_DONE, + ) + + +def _run_declared_completion_validation( + *, + validation_command: str | None, + validation_label: str | None, + registry_path: Path, + goal_id: str, +) -> dict[str, Any] | None: + """Run a todo's declared caller-approved validation command. + + Returns ``None`` when no ``validation_command`` is declared (the unchanged + fast path). Otherwise always returns a privacy-safe receipt whose + ``passed`` is True only when the command ran and exited zero; setup + failures (no repository workspace), timeouts, missing executables, and + malformed commands are all reported as ``passed=False`` receipts rather + than raised, so completion can surface a typed failure without committing. + """ + if not validation_command: + return None + label = validation_label or "todo completion validation" + workspace = _resolve_goal_repo_workspace(registry_path, goal_id) + if workspace is None: + return { + "schema_version": CALLER_VALIDATION_RECEIPT_SCHEMA_VERSION, + "command_label": label, + "exit_code": None, + "passed": False, + "status": "workspace_unavailable", + "summary": ( + "validation_command is declared but the goal has no " + "repository workspace to run it in" + ), + "stdout_captured": False, + "stderr_captured": False, + "local_path_captured": False, + } + try: + return run_caller_validation( + workspace, + validation_command=str(validation_command), + validation_label=label, + timeout_seconds=_COMPLETION_VALIDATION_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired: + return { + "schema_version": CALLER_VALIDATION_RECEIPT_SCHEMA_VERSION, + "command_label": label, + "exit_code": None, + "passed": False, + "status": "timeout", + "summary": ( + f"validation command timed out after " + f"{_COMPLETION_VALIDATION_TIMEOUT_SECONDS}s" + ), + "stdout_captured": False, + "stderr_captured": False, + "local_path_captured": False, + } + except (FileNotFoundError, PermissionError) as exc: + return { + "schema_version": CALLER_VALIDATION_RECEIPT_SCHEMA_VERSION, + "command_label": label, + "exit_code": None, + "passed": False, + "status": "command_not_run", + "summary": f"validation command could not be launched: {exc}", + "stdout_captured": False, + "stderr_captured": False, + "local_path_captured": False, + } + except ValueError as exc: + # shlex.split rejects malformed (e.g. unbalanced-quote) commands. + return { + "schema_version": CALLER_VALIDATION_RECEIPT_SCHEMA_VERSION, + "command_label": label, + "exit_code": None, + "passed": False, + "status": "command_malformed", + "summary": f"validation command could not be parsed: {exc}", + "stdout_captured": False, + "stderr_captured": False, + "local_path_captured": False, + } + + def complete_goal_todo( *, registry_path: Path, @@ -1550,6 +1692,42 @@ def complete_goal_todo( project=project, state_file=state_file, ) + # Run caller-approved validation BEFORE acquiring the mutation lock. The + # validation command runs in the goal's repo workspace, not the state file, + # so it needs no state lock; running it here keeps the lock held only for + # the millisecond-scale read-modify-write (the MUTATION lock deadline is + # 5s) instead of across a multi-second subprocess. + validation_command_declared, validation_label_declared, already_completed = ( + _read_declared_validation( + state_file=resolved_state_file, todo_id=todo_id, role=role + ) + ) + # Skip validation on dry_run and on a terminal replay (todo already done) — + # the in-lock completed_todo_replay short-circuit owns idempotent replays. + completion_validation = ( + _run_declared_completion_validation( + validation_command=validation_command_declared, + validation_label=validation_label_declared, + registry_path=registry_path, + goal_id=goal_id, + ) + if not dry_run and not already_completed + else None + ) + if ( + completion_validation is not None + and completion_validation.get("passed") is not True + ): + return { + "ok": False, + "dry_run": dry_run, + "completed": False, + "goal_id": goal_id, + "todo_id": todo_id, + "changed": False, + "validation": completion_validation, + "validation_blocked_completion": True, + } with exclusive_file_lock( resolved_state_file, agent_id=agent_id or claimed_by, diff --git a/tests/control_plane/test_todo_completion_validation.py b/tests/control_plane/test_todo_completion_validation.py new file mode 100644 index 000000000..4102f1f14 --- /dev/null +++ b/tests/control_plane/test_todo_completion_validation.py @@ -0,0 +1,277 @@ +from __future__ import annotations + +import json +import shlex +import sys +from pathlib import Path + +import pytest + +import loopx.todos as todos_module +from loopx.status import parse_active_state_todos +from loopx.todos import add_goal_todo, complete_goal_todo + +GOAL_ID = "todo-completion-validation" +AGENT = "codex-author" + +_PASS_COMMAND = f'{shlex.quote(sys.executable)} -c "raise SystemExit(0)"' +_FAIL_COMMAND = f'{shlex.quote(sys.executable)} -c "raise SystemExit(1)"' +_SLEEP_COMMAND = f'{shlex.quote(sys.executable)} -c "import time; time.sleep(30)"' + + +def _write_fixture(tmp_path: Path) -> tuple[Path, Path]: + repo = tmp_path / "repo" + repo.mkdir() + state = repo / "ACTIVE_GOAL_STATE.md" + state.write_text( + "\n".join( + [ + "---", + f"goal_id: {GOAL_ID}", + "updated_at: 2026-08-12T00:00:00+00:00", + "---", + "", + "## Agent Todo", + "", + ] + ) + + "\n", + encoding="utf-8", + ) + registry = tmp_path / "registry.global.json" + registry.write_text( + json.dumps( + { + "common_runtime_root": str(tmp_path / "runtime"), + "goals": [ + { + "id": GOAL_ID, + "domain": "harness_self_improvement", + "status": "active", + "repo": str(repo), + "state_file": state.name, + "adapter": {"kind": "harness_self_improvement"}, + "coordination": { + "agent_model": "peer_v1", + "registered_agents": [AGENT], + }, + } + ], + } + ), + encoding="utf-8", + ) + return registry, state + + +def _agent_todo(state: Path, todo_id: str) -> dict: + todos = parse_active_state_todos(state.read_text(encoding="utf-8")) + return next( + item + for item in todos["agent_todos"]["items"] + if item["todo_id"] == todo_id + ) + + +def _add_todo( + registry: Path, + *, + validation_command: str | None = None, + validation_label: str | None = None, +) -> dict: + return add_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + role="agent", + text="Deliver one bounded change.", + task_class="advancement_task", + claimed_by=AGENT, + validation_command=validation_command, + validation_label=validation_label, + ) + + +def test_validation_command_declared_and_passing_commits_completion( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + registry, state = _write_fixture(tmp_path) + todo = _add_todo( + registry, + validation_command=_PASS_COMMAND, + validation_label="caller-declared smoke", + ) + # Spy on the executor so the test fails if the gate is silently skipped. + original_runner = todos_module.run_caller_validation + calls = {"count": 0} + + def counting_runner(*args, **kwargs): # type: ignore[no-untyped-def] + calls["count"] += 1 + return original_runner(*args, **kwargs) + + monkeypatch.setattr(todos_module, "run_caller_validation", counting_runner) + + result = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="validated completion", + ) + assert calls["count"] == 1 # the gate actually ran the declared command + assert result["ok"] is True + assert result["changed"] is True + assert "validation_blocked_completion" not in result + assert _agent_todo(state, str(todo["todo_id"]))["status"] == "done" + + +def test_missing_validation_executable_returns_typed_receipt( + tmp_path: Path, +) -> None: + registry, state = _write_fixture(tmp_path) + todo = _add_todo(registry, validation_command="nonexistent-binary-xyz-12345") + result = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="claim of completion", + ) + assert result["ok"] is False + assert result["validation_blocked_completion"] is True + receipt = result["validation"] + assert receipt["passed"] is False + assert receipt["status"] == "command_not_run" + assert _agent_todo(state, str(todo["todo_id"]))["status"] == "open" + + +def test_malformed_validation_command_returns_typed_receipt( + tmp_path: Path, +) -> None: + registry, state = _write_fixture(tmp_path) + # Unbalanced quote -> shlex.split raises ValueError -> typed receipt. + todo = _add_todo(registry, validation_command="echo 'unbalanced") + result = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="claim of completion", + ) + assert result["ok"] is False + assert result["validation_blocked_completion"] is True + receipt = result["validation"] + assert receipt["passed"] is False + assert receipt["status"] == "command_malformed" + assert _agent_todo(state, str(todo["todo_id"]))["status"] == "open" + + +def test_validation_command_declared_and_failing_blocks_completion( + tmp_path: Path, +) -> None: + registry, state = _write_fixture(tmp_path) + todo = _add_todo( + registry, + validation_command=_FAIL_COMMAND, + validation_label="caller-declared smoke", + ) + result = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="claim of completion", + ) + # Completion is blocked: nothing committed, evidence stays only a claim. + assert result["ok"] is False + assert result["completed"] is False + assert result["changed"] is False + assert result["validation_blocked_completion"] is True + receipt = result["validation"] + assert receipt["passed"] is False + assert receipt["exit_code"] == 1 + assert receipt["command_label"] == "caller-declared smoke" + # Privacy invariant preserved. + assert receipt["stdout_captured"] is False + assert receipt["stderr_captured"] is False + assert receipt["local_path_captured"] is False + # State is unchanged: the todo is still open. + assert _agent_todo(state, str(todo["todo_id"]))["status"] == "open" + + +def test_no_validation_command_keeps_fast_path_unchanged(tmp_path: Path) -> None: + registry, state = _write_fixture(tmp_path) + todo = _add_todo(registry) # no validation_command declared + result = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="plain completion", + ) + assert result["ok"] is True + assert result["changed"] is True + assert "validation_blocked_completion" not in result + assert _agent_todo(state, str(todo["todo_id"]))["status"] == "done" + + +def test_validation_timeout_blocks_completion( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr( + todos_module, "_COMPLETION_VALIDATION_TIMEOUT_SECONDS", 0.5 + ) + registry, state = _write_fixture(tmp_path) + todo = _add_todo(registry, validation_command=_SLEEP_COMMAND) + result = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="claim of completion", + ) + assert result["ok"] is False + assert result["validation_blocked_completion"] is True + receipt = result["validation"] + assert receipt["passed"] is False + assert receipt["status"] == "timeout" + assert _agent_todo(state, str(todo["todo_id"]))["status"] == "open" + + +def test_terminal_replay_short_circuits_before_validation( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + registry, state = _write_fixture(tmp_path) + todo = _add_todo(registry, validation_command=_PASS_COMMAND) + + original_runner = todos_module.run_caller_validation + calls = {"count": 0} + + def counting_runner(*args, **kwargs): # type: ignore[no-untyped-def] + calls["count"] += 1 + return original_runner(*args, **kwargs) + + monkeypatch.setattr(todos_module, "run_caller_validation", counting_runner) + + first = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="validated completion", + ) + assert first["ok"] is True + assert calls["count"] == 1 # validation ran once on the real completion + + replay = complete_goal_todo( + registry_path=registry, + goal_id=GOAL_ID, + todo_id=str(todo["todo_id"]), + agent_id=AGENT, + evidence="duplicate completion", + ) + # Replay short-circuits before the validation gate; the command is not re-run. + assert calls["count"] == 1 + assert replay["ok"] is True