diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index 31c4bf68ae85..54c9744ac94f 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -1894,13 +1894,19 @@ def _cmd_complete(args: argparse.Namespace) -> int: failed: list[str] = [] with kb.connect_closing() as conn: for tid in ids: - if not kb.complete_task( - conn, tid, - result=args.result, - summary=summary, - metadata=metadata, - expected_run_id=_worker_run_id_for(tid), - ): + try: + ok = kb.complete_task( + conn, tid, + result=args.result, + summary=summary, + metadata=metadata, + expected_run_id=_worker_run_id_for(tid), + ) + except (kb.CompletionGateError, kb.InvalidOptOutError) as gate_err: + failed.append(tid) + print(f"{tid}: {gate_err}", file=sys.stderr) + continue + if not ok: failed.append(tid) print(f"cannot complete {tid} (unknown id or terminal state)", file=sys.stderr) else: diff --git a/hermes_cli/kanban_completion_gates.py b/hermes_cli/kanban_completion_gates.py new file mode 100644 index 000000000000..ed541ac04328 --- /dev/null +++ b/hermes_cli/kanban_completion_gates.py @@ -0,0 +1,429 @@ +"""Verification gates run before `complete_task` writes `status=done`. + +Each gate is a pure function that takes structured inputs and returns either +``None`` (pass) or a violation dataclass describing why the completion should +be rejected. The caller (``complete_task`` in ``kanban_db.py``) collects any +violation, emits an auditable event, and raises so the worker layer surfaces a +structured retry message. + +Pattern mirrors the existing ``_verify_created_cards`` / +``HallucinatedCardsError`` flow — gates fire BEFORE the write transaction so +state is unchanged on rejection and the worker can simply retry with corrected +output. + +Three gates ship today (Tranche 1 of v6.7, closes #28, #62, #64): + +1. :func:`verify_runtime_floor` — per-role floor on + ``completed_at - started_at``. Catches Tony's 20-second "approve" verdicts + and Friday's 59-second "implemented 7 dispatcher gates" claims. + +2. :func:`verify_workspace_diff` — when a non-review worker on a + ``dir`` / ``worktree`` workspace claims to have produced code, the workspace + must show a real diff against its tracking base. Catches Friday's "Wave A + gates implemented" with zero changes on the branch. + +3. :func:`verify_no_stray_artifacts` — reject untracked artifacts matching + patterns the swarm has historically committed by accident + (``*evidence*``, ``commit-hash*``, ``triage/*``, ``tmp-*``, and tracked + files with no extension and no shebang — the "all prior block evidence + files" failure mode). + +See hermes-jarvis#61 for the bootstrap-paradox case study that motivates +these gates. +""" +from __future__ import annotations + +import os +import re +import shlex +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Optional + + +# ===================================================================== +# Per-role runtime floors (#64) +# ===================================================================== + +# Empirically derived from the 2026-06-09 build-chain failure: any number +# below the floor on a non-orchestration task is more likely fabrication +# than fast work. Workers may opt out per-call via the +# ``x_fast_justified`` metadata field, surfaced through ``allow_below_floor``. +ROLE_RUNTIME_FLOORS_SECONDS: dict[str, int] = { + # Build / implementation roles — real code changes don't ship in <5 min + "friday": 5 * 60, + "shuri": 5 * 60, + "build-engineer": 5 * 60, + # Review roles — even a tiny review needs to read the diff + "tony": 90, + "tchalla": 90, + "vision": 90, + "reviewer": 90, + # Orchestration roles — JARVIS umbrella spawn can be fast and correct + "jarvis": 0, + "pepper": 0, + "banner": 0, +} + + +@dataclass(frozen=True) +class RuntimeFloorViolation: + role: str + started_at: int + completed_at: int + floor_seconds: int + actual_seconds: int + + def message(self) -> str: + return ( + f"runtime-floor: {self.role} completed in {self.actual_seconds}s, " + f"below the {self.floor_seconds}s floor for this role. " + f"Either keep working (add evidence and re-call kanban_complete after " + f"the floor passes) or, if the work was genuinely trivial, set " + f"metadata={{\"x_fast_justified\": \"\"}} on the " + f"completion call." + ) + + +def verify_runtime_floor( + assignee: Optional[str], + started_at: Optional[int], + completed_at: int, + *, + allow_below_floor: bool = False, +) -> Optional[RuntimeFloorViolation]: + """Return a violation if the worker's runtime is below its role floor. + + ``started_at`` is the timestamp the dispatcher recorded when the worker + claimed the task (NOT the run-row creation time). ``completed_at`` is + "now" from the dispatcher's perspective when ``complete_task`` runs. + + A floor of 0 (or an unknown assignee, or a missing ``started_at``) is a + pass — we never invent floors for roles we don't know. + """ + if allow_below_floor: + return None + if not assignee or started_at is None: + return None + floor = ROLE_RUNTIME_FLOORS_SECONDS.get(assignee.lower()) + if not floor: + return None + actual = max(0, completed_at - int(started_at)) + if actual >= floor: + return None + return RuntimeFloorViolation( + role=assignee, started_at=int(started_at), completed_at=completed_at, + floor_seconds=floor, actual_seconds=actual, + ) + + +# ===================================================================== +# Workspace-diff gate (#62) +# ===================================================================== + +REVIEW_ROLES = {"tony", "tchalla", "vision", "reviewer"} +ORCHESTRATION_ROLES = {"jarvis", "pepper", "banner"} + + +@dataclass(frozen=True) +class WorkspaceDiffViolation: + assignee: str + workspace_path: str + summary_excerpt: str + diff_stat: str # may be empty string if no changes + + def message(self) -> str: + diff_preview = self.diff_stat.strip() or "(no changes against tracking base)" + return ( + f"workspace-diff: {self.assignee} summary claims implementation " + f"work ({self.summary_excerpt!r}) but `git diff` in " + f"{self.workspace_path} shows: {diff_preview}. " + f"Either produce the changes the summary describes, or block " + f"with an honest reason. To skip this check on a doc-only or " + f"genuinely-no-code task, set metadata={{\"x_no_code\": true}}." + ) + + +def _git_diff_stat_against_base(workspace_path: str) -> str: + """Return `git diff --stat` against the workspace's tracking base. + + Tracking base is, in order: ``@{upstream}`` if it exists, else + ``origin/main`` if it exists, else ``main``. If git rejects all three, + returns the empty string (gate treats as "no diff"). + + Subprocess calls use a hard 10s wallclock so a hung git can't stall the + dispatcher. + """ + if not workspace_path or not os.path.isdir(workspace_path): + return "" + if not os.path.isdir(os.path.join(workspace_path, ".git")): + # Worktree-backed dirs have .git as a file pointer; that's fine. + if not os.path.isfile(os.path.join(workspace_path, ".git")): + return "" + + def _run(args: list[str]) -> Optional[str]: + try: + out = subprocess.run( + args, cwd=workspace_path, capture_output=True, + text=True, timeout=10, check=False, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): + return None + if out.returncode != 0: + return None + return out.stdout + + for base_spec in ("@{upstream}", "origin/main", "main"): + # First check the base exists (cheap), then diff against it. + if _run(["git", "rev-parse", "--verify", base_spec]) is None: + continue + stat = _run(["git", "diff", "--stat", base_spec, "HEAD"]) + if stat is not None: + return stat + return "" + + +def verify_workspace_diff( + assignee: Optional[str], + workspace_kind: Optional[str], + workspace_path: Optional[str], + summary: Optional[str], + *, + allow_no_code: bool = False, +) -> Optional[WorkspaceDiffViolation]: + """Reject completions that claim code work but show no diff. + + Skipped (returns None) when: + - assignee is a review or orchestration role + - workspace is scratch (no diff target) + - workspace_path is missing or not a directory + - summary doesn't claim implementation + - caller opted out via ``allow_no_code=True`` + """ + if allow_no_code: + return None + if not assignee: + return None + role = assignee.lower() + if role in REVIEW_ROLES or role in ORCHESTRATION_ROLES: + return None + if (workspace_kind or "scratch") not in {"dir", "worktree"}: + return None + if not workspace_path or not os.path.isdir(workspace_path): + # Wrong / typo'd path is the dispatcher's problem to surface + # elsewhere — we don't punish the worker for it. + return None + # Build-role workers on a real workspace ALWAYS need a non-empty + # diff. The earlier implementation only fired when the summary used + # a trigger verb ("implemented X"), which made the gate trivially + # bypassable: a worker that wrote "Per spec, the changes land in + # hermes_cli and tests pass" had no trigger verb and passed even + # with an empty branch. Build-role + workspace=dir/worktree implies + # code work; if the worker honestly produced no code, they should + # call ``kanban_block`` with a reason or opt out via + # ``x_no_code`` with a string justification. See PR-#11 self- + # review notes in hermes-jarvis#61. + diff_stat = _git_diff_stat_against_base(workspace_path) + # A real implementation produces SOME change line. We only reject when + # the diff is empty / whitespace. + if diff_stat and diff_stat.strip(): + return None + # Defensive — empty/None summary used to crash here on + # splitlines()[0] before the gate could surface its violation. + # Fall back to an empty excerpt so the violation is still + # constructed cleanly (the gate's CompletionGateError message still + # tells the worker what to do). + summary_lines = (summary or "").strip().splitlines() + summary_excerpt = summary_lines[0][:200] if summary_lines else "" + return WorkspaceDiffViolation( + assignee=assignee, workspace_path=workspace_path, + summary_excerpt=summary_excerpt, diff_stat=diff_stat, + ) + + +# ===================================================================== +# Repo-hygiene gate (#28) +# ===================================================================== + +# Patterns that mark a path as "stray orchestration artifact" rather +# than real source. Matched against the path relative to the repo +# root, case insensitive. Tightened after a self-review false-positive +# audit: matching `evidence-types.md` or `LICENSE` or `Dockerfile` +# would block every Friday completion. Patterns now match only +# segments that are exactly the stray token (not legitimate filenames +# that contain the token as a substring). +# +# Each pattern matches a PATH SEGMENT or a FULL BASENAME and only the +# specific shapes we've seen as accidental commits in prior chains: +# agent-dashboard PR #1's "all prior block evidence files", `commit- +# hash.txt`, the `evidence/` artifact dirs under `changes/`, and +# `triage/` report drops. +_STRAY_PATH_PATTERNS = [ + # `evidence` as a directory segment, or basename `block-*-evidence.*` + # or `*-evidence.json/png/log/txt`. Excludes `evidence-types.md` (a + # legitimate source doc) by requiring the segment END at `evidence`. + re.compile(r"(^|/)evidence(/|$)", re.IGNORECASE), + re.compile( + r"(^|/)[^/]*-evidence\.(json|png|log|txt|md|yaml|yml)$", + re.IGNORECASE, + ), + re.compile(r"(^|/)commit-hash(\.[a-z]+)?$", re.IGNORECASE), + re.compile(r"(^|/)triage(/|$)", re.IGNORECASE), + re.compile(r"(^|/)tmp-[^/]+$", re.IGNORECASE), + re.compile(r"(^|/)all prior block evidence files$", re.IGNORECASE), +] + +# Tracked basenames that look like they MIGHT be stray (no extension) +# but are universally legitimate source files in many repos. The +# untracked-only scoping below already protects these in practice, but +# we keep the allowlist as defense-in-depth for repos whose history +# includes these as tracked files long before any swarm activity. +_LEGITIMATE_NO_EXT_BASENAMES = { + "LICENSE", "LICENCE", "COPYING", "NOTICE", "AUTHORS", + "CHANGELOG", "CONTRIBUTORS", "MAINTAINERS", "OWNERS", "CODEOWNERS", + "Dockerfile", "Makefile", "Vagrantfile", "Procfile", "Brewfile", + "Rakefile", "Gemfile", "Guardfile", "Capfile", "Jenkinsfile", + "Containerfile", "Earthfile", "README", +} + + +@dataclass(frozen=True) +class StrayArtifactViolation: + workspace_path: str + stray_paths: tuple[str, ...] + + def message(self) -> str: + listing = "\n ".join(self.stray_paths) + return ( + f"repo-hygiene: workspace {self.workspace_path} contains files " + f"that look like leftover orchestration artifacts:\n {listing}\n" + f"Delete (or .gitignore) them before calling kanban_complete. " + f"If a stray-looking path is intentional, prefix it with a real " + f"file extension and add a one-line comment explaining why it's " + f"in the repo." + ) + + +def _has_shebang(path: str) -> bool: + try: + with open(path, "rb") as f: + head = f.read(2) + return head == b"#!" + except (OSError, IOError): + return False + + +def _stray_path_score(repo_root: str, rel_path: str, *, is_tracked: bool) -> bool: + """True if ``rel_path`` looks like a stray artifact. + + The specific stray patterns apply to BOTH tracked and untracked + files (a worker who actually committed ``commit-hash.txt`` is just + as wrong as one who left it untracked). The fuzzy + no-extension/no-shebang heuristic only applies to UNTRACKED files — + repos legitimately track LICENSE / Dockerfile / Makefile, and + blaming a worker for files that were in main before they started is + a false positive that teaches the swarm to opt out reflexively. + """ + norm = rel_path.replace("\\", "/") + if any(p.search(norm) for p in _STRAY_PATH_PATTERNS): + return True + if is_tracked: + return False + base = os.path.basename(norm) + if base in _LEGITIMATE_NO_EXT_BASENAMES: + return False + if "." not in base and not _has_shebang(os.path.join(repo_root, rel_path)): + return True + return False + + +def _list_workspace_files_split( + workspace_path: str, +) -> tuple[list[str], list[str]]: + """Return ``(tracked, untracked)`` lists of paths in ``workspace_path``. + + Untracked respects .gitignore. Empty lists on any git error. + """ + if not workspace_path or not os.path.isdir(workspace_path): + return [], [] + + def _run(args: list[str]) -> Optional[str]: + try: + out = subprocess.run( + args, cwd=workspace_path, capture_output=True, + text=True, timeout=10, check=False, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): + return None + if out.returncode != 0: + return None + return out.stdout + + def _split(blob: Optional[str]) -> list[str]: + if not blob: + return [] + return [ln.strip() for ln in blob.splitlines() if ln.strip()] + + tracked = _split(_run(["git", "ls-files"])) + untracked = _split(_run(["git", "ls-files", "--others", "--exclude-standard"])) + return tracked, untracked + + +def verify_no_stray_artifacts( + workspace_kind: Optional[str], + workspace_path: Optional[str], + *, + allow_stray: bool = False, +) -> Optional[StrayArtifactViolation]: + """Reject completions where the workspace tree contains stray files. + + Skipped (returns None) when: + - workspace is scratch + - workspace_path is missing or not a directory + - caller opted out via ``allow_stray=True`` + """ + if allow_stray: + return None + if (workspace_kind or "scratch") not in {"dir", "worktree"}: + return None + if not workspace_path or not os.path.isdir(workspace_path): + return None + tracked, untracked = _list_workspace_files_split(workspace_path) + stray: list[str] = [] + for p in tracked: + if _stray_path_score(workspace_path, p, is_tracked=True): + stray.append(p) + for p in untracked: + if _stray_path_score(workspace_path, p, is_tracked=False): + stray.append(p) + stray = sorted(set(stray)) + if not stray: + return None + return StrayArtifactViolation( + workspace_path=workspace_path, stray_paths=tuple(stray), + ) + + +# ===================================================================== +# Exception class for the integration in `complete_task` +# ===================================================================== + +class CompletionGateError(ValueError): + """Raised by ``complete_task`` when one or more v6.7 gates reject. + + ``violations`` is a list of dataclasses (one per failed gate). Each has + a ``.message()`` returning a worker-actionable string. Subclass of + ``ValueError`` so existing tool-error handlers treat this as a + recoverable user error (same convention as + :class:`HallucinatedCardsError`). + """ + + def __init__(self, violations: list, completing_task_id: str): + self.violations = list(violations) + self.completing_task_id = completing_task_id + lines = [v.message() for v in self.violations] + super().__init__( + "kanban_complete blocked by v6.7 gates:\n- " + + "\n- ".join(lines) + ) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c8c53dba7ecb..054b5c1d4998 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -90,6 +90,13 @@ from toolsets import get_toolset_names +from hermes_cli.kanban_completion_gates import ( + CompletionGateError, + verify_no_stray_artifacts, + verify_runtime_floor, + verify_workspace_diff, +) + _log = logging.getLogger(__name__) @@ -3559,6 +3566,131 @@ def __init__(self, phantom: list[str], completing_task_id: str): ) +_V67_OPT_OUT_MIN_REASON_LEN = 20 + + +class InvalidOptOutError(ValueError): + """Raised when a v6.7 gate opt-out (``x_fast_justified`` / + ``x_no_code`` / ``x_stray_ok``) is set to a non-string or too-short + value. + + Each opt-out is an explicit auditable bypass — it MUST be a string + of at least :data:`_V67_OPT_OUT_MIN_REASON_LEN` non-whitespace + characters explaining WHY the bypass is justified. Truthy booleans + or empty strings (``True``, ``"x"``, ``"ok"``) get rejected so the + opt-out is not a free keyword bypass for the gate. See review of + PR #11 in hermes-jarvis#61 thread. + """ + + def __init__(self, completing_task_id: str, key: str, value): + self.completing_task_id = completing_task_id + self.key = key + self.value = value + kind = type(value).__name__ + super().__init__( + f"kanban_complete blocked: metadata.{key} must be a string of " + f"at least {_V67_OPT_OUT_MIN_REASON_LEN} non-whitespace characters " + f"explaining the bypass; got {kind} {value!r}. " + f"Either drop {key} and address the gate's finding, or pass " + f"a real justification like '{key}: trivially-one-line-rename " + f"verified by smoke test'." + ) + + +def _validate_opt_out(task_id: str, key: str, raw) -> Optional[str]: + """Return a normalized opt-out reason, or ``None`` when the key isn't + set. Raises :class:`InvalidOptOutError` when the value is truthy but + not a substantive string. + """ + if raw is None or raw is False: + return None + if not isinstance(raw, str) or len(raw.strip()) < _V67_OPT_OUT_MIN_REASON_LEN: + raise InvalidOptOutError(task_id, key, raw) + return raw.strip() + + +def _v6_7_run_completion_gates( + conn: sqlite3.Connection, + task_id: str, + *, + summary: Optional[str], + metadata: Optional[dict], + now: int, +) -> list: + """Run the v6.7 Tranche 1 completion gates and return any violations. + + Reads task assignee / workspace / started_at from the tasks row and + delegates to the pure gate functions in ``kanban_completion_gates``. + Returns an empty list when all gates pass. + + Workers may opt out of individual gates via per-call metadata keys. + Each opt-out value MUST be a non-empty string of at least + :data:`_V67_OPT_OUT_MIN_REASON_LEN` chars explaining the bypass + (truthy bools or short strings are rejected with + :class:`InvalidOptOutError`). Opt-outs that ARE accepted get + emitted as a ``completion_opt_out_used`` event so the bypass is + auditable downstream. + + Opt-out keys (string only): + - ``x_fast_justified`` — skip runtime-floor (#64) + - ``x_no_code`` — skip workspace-diff (#62) + - ``x_stray_ok`` — skip repo-hygiene (#28) + """ + row = conn.execute( + "SELECT assignee, workspace_kind, workspace_path, started_at " + " FROM tasks WHERE id = ?", + (task_id,), + ).fetchone() + if row is None: + return [] + md = metadata or {} + fast_ok_reason = _validate_opt_out(task_id, "x_fast_justified", md.get("x_fast_justified")) + no_code_reason = _validate_opt_out(task_id, "x_no_code", md.get("x_no_code")) + stray_ok_reason = _validate_opt_out(task_id, "x_stray_ok", md.get("x_stray_ok")) + accepted_opt_outs = { + k: v for k, v in ( + ("x_fast_justified", fast_ok_reason), + ("x_no_code", no_code_reason), + ("x_stray_ok", stray_ok_reason), + ) if v is not None + } + if accepted_opt_outs: + with write_txn(conn): + _append_event( + conn, task_id, "completion_opt_out_used", + {"opt_outs": accepted_opt_outs}, + ) + fast_ok = fast_ok_reason is not None + no_code = no_code_reason is not None + stray_ok = stray_ok_reason is not None + violations: list = [] + floor = verify_runtime_floor( + assignee=row["assignee"], + started_at=row["started_at"], + completed_at=now, + allow_below_floor=fast_ok, + ) + if floor is not None: + violations.append(floor) + diff = verify_workspace_diff( + assignee=row["assignee"], + workspace_kind=row["workspace_kind"], + workspace_path=row["workspace_path"], + summary=summary, + allow_no_code=no_code, + ) + if diff is not None: + violations.append(diff) + stray = verify_no_stray_artifacts( + workspace_kind=row["workspace_kind"], + workspace_path=row["workspace_path"], + allow_stray=stray_ok, + ) + if stray is not None: + violations.append(stray) + return violations + + def complete_task( conn: sqlite3.Connection, task_id: str, @@ -3626,6 +3758,30 @@ def complete_task( else: verified_cards = [] + # v6.7 Tranche 1: kanban_complete verification gates. + # See hermes-jarvis#61, #62, #28, #64. Same pre-write-txn pattern as + # _verify_created_cards: any violation raises before state changes, so + # the worker can retry after fixing the underlying issue. + _violations = _v6_7_run_completion_gates( + conn, task_id, summary=summary, metadata=metadata, now=now, + ) + if _violations: + with write_txn(conn): + _append_event( + conn, task_id, "completion_blocked_v6_7_gates", + { + "violations": [ + {"kind": type(v).__name__, "message": v.message()} + for v in _violations + ], + "summary_preview": ( + (summary or result or "").strip().splitlines()[0][:200] + if (summary or result) else None + ), + }, + ) + raise CompletionGateError(_violations, task_id) + with write_txn(conn): if expected_run_id is None: cur = conn.execute( diff --git a/plugins/kanban/dashboard/plugin_api.py b/plugins/kanban/dashboard/plugin_api.py index 0a49172a86b8..98401eaeb714 100644 --- a/plugins/kanban/dashboard/plugin_api.py +++ b/plugins/kanban/dashboard/plugin_api.py @@ -843,14 +843,25 @@ def update_task(task_id: str, payload: UpdateTaskBody, board: Optional[str] = Qu s = payload.status ok = True if s == "done": - ok = kanban_db.complete_task( - conn, task_id, - result=payload.result, - summary=payload.summary, - metadata=payload.metadata, - ) + try: + ok = kanban_db.complete_task( + conn, task_id, + result=payload.result, + summary=payload.summary, + metadata=payload.metadata, + ) + except ( + kanban_db.CompletionGateError, + kanban_db.InvalidOptOutError, + ) as gate_err: + # v6.7 gates rejected; surface as 409 so the dashboard + # can show the user a structured retry hint. + raise HTTPException(status_code=409, detail=str(gate_err)) from gate_err elif s == "blocked": - ok = kanban_db.block_task(conn, task_id, reason=payload.block_reason) + try: + ok = kanban_db.block_task(conn, task_id, reason=payload.block_reason) + except kanban_db.FabricatedAuthClaimError as fab_err: + raise HTTPException(status_code=409, detail=str(fab_err)) from fab_err elif s == "scheduled": ok = kanban_db.schedule_task(conn, task_id, reason=payload.block_reason) elif s == "ready": @@ -1186,14 +1197,27 @@ def bulk_update(payload: BulkTaskBody, board: Optional[str] = Query(None)): if payload.status is not None and not payload.archive: s = payload.status if s == "done": - ok = kanban_db.complete_task( - conn, tid, - result=payload.result, - summary=payload.summary, - metadata=payload.metadata, - ) + try: + ok = kanban_db.complete_task( + conn, tid, + result=payload.result, + summary=payload.summary, + metadata=payload.metadata, + ) + except ( + kanban_db.CompletionGateError, + kanban_db.InvalidOptOutError, + ) as gate_err: + entry.update(ok=False, error=str(gate_err)) + results.append(entry) + continue elif s == "blocked": - ok = kanban_db.block_task(conn, tid) + try: + ok = kanban_db.block_task(conn, tid) + except kanban_db.FabricatedAuthClaimError as fab_err: + entry.update(ok=False, error=str(fab_err)) + results.append(entry) + continue elif s == "ready": cur = kanban_db.get_task(conn, tid) if cur and cur.status in ("blocked", "scheduled"): diff --git a/tests/cli/test_kanban_completion_gates.py b/tests/cli/test_kanban_completion_gates.py new file mode 100644 index 000000000000..a5c39db86b31 --- /dev/null +++ b/tests/cli/test_kanban_completion_gates.py @@ -0,0 +1,598 @@ +"""Tests for hermes_cli.kanban_completion_gates — v6.7 Tranche 1. + +Closes hermes-jarvis#62 (workspace-diff verification), #28 (repo hygiene +gate), and #64 (per-role runtime floor). See hermes-jarvis#61 for the +bootstrap-paradox case study where a v6.7 swarm build chain rubber-stamped +9 tasks done in ~10 minutes with zero real deliverables. + +Each gate is a pure function and gets a focused test that pins the exact +failure modes the 2026-06-09 chain demonstrated. +""" +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from hermes_cli.kanban_completion_gates import ( + RuntimeFloorViolation, + StrayArtifactViolation, + WorkspaceDiffViolation, + verify_no_stray_artifacts, + verify_runtime_floor, + verify_workspace_diff, +) + + +# ===================================================================== +# verify_runtime_floor — #64 +# ===================================================================== + + +class TestRuntimeFloor: + def test_tony_20s_review_is_below_floor(self) -> None: + """The exact case from 2026-06-09: Tony approved Wave A in 20s.""" + v = verify_runtime_floor("tony", started_at=1000, completed_at=1020) + assert isinstance(v, RuntimeFloorViolation) + assert v.actual_seconds == 20 + assert v.floor_seconds == 90 + assert "tony" in v.message().lower() + assert "below" in v.message().lower() + + def test_friday_59s_implementation_is_below_floor(self) -> None: + """Friday claimed 7 dispatcher gates implemented in 59s.""" + v = verify_runtime_floor("friday", started_at=1000, completed_at=1059) + assert isinstance(v, RuntimeFloorViolation) + assert v.floor_seconds == 300 + + def test_tony_91s_review_passes(self) -> None: + """One second above the floor is a pass — the floor is the floor.""" + assert verify_runtime_floor("tony", 1000, 1091) is None + + def test_jarvis_orchestration_has_no_floor(self) -> None: + """Orchestration roles routinely complete in seconds and that's fine.""" + assert verify_runtime_floor("jarvis", 1000, 1001) is None + + def test_unknown_assignee_skips(self) -> None: + """Don't invent floors for roles we haven't categorized.""" + assert verify_runtime_floor("rando-profile", 1000, 1001) is None + + def test_missing_assignee_skips(self) -> None: + assert verify_runtime_floor(None, 1000, 1001) is None + + def test_missing_started_at_skips(self) -> None: + """If the dispatcher never recorded started_at the gate can't fire.""" + assert verify_runtime_floor("tony", None, 1100) is None + + def test_allow_below_floor_opt_out(self) -> None: + """Workers can justify fast completions via metadata.""" + assert ( + verify_runtime_floor("tony", 1000, 1020, allow_below_floor=True) + is None + ) + + def test_completed_before_started_is_zero(self) -> None: + """Clock skew / wrong order doesn't crash — actual=0, still below floor.""" + v = verify_runtime_floor("tony", 1100, 1000) + assert v is not None + assert v.actual_seconds == 0 + + def test_case_insensitive_role_match(self) -> None: + """Profile names sometimes capitalize differently — match insensitively.""" + v = verify_runtime_floor("Tony", 1000, 1020) + assert v is not None + + +# ===================================================================== +# verify_workspace_diff — #62 +# ===================================================================== + + +@pytest.fixture +def git_workspace(tmp_path: Path) -> Path: + """A real git repo with one committed file on main, no other changes.""" + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=tmp_path, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], + cwd=tmp_path, check=True, + ) + subprocess.run( + ["git", "config", "user.name", "test"], + cwd=tmp_path, check=True, + ) + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "src.py"], cwd=tmp_path, check=True) + subprocess.run( + ["git", "commit", "-q", "-m", "init"], cwd=tmp_path, check=True, + ) + return tmp_path + + +class TestWorkspaceDiff: + def test_friday_empty_diff_with_implementation_claim_rejects( + self, git_workspace: Path, + ) -> None: + """The exact case from 2026-06-09: Friday's branch had no new commits + but his summary claimed "Wave A dispatcher discipline gates + implemented; tests cover #28-#34". + """ + v = verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Wave A dispatcher discipline gates implemented; tests cover #28-#34", + ) + assert isinstance(v, WorkspaceDiffViolation) + assert "friday" in v.message().lower() + assert "implementation" in v.message().lower() or "implement" in v.message().lower() + + def test_real_diff_with_implementation_claim_passes( + self, git_workspace: Path, + ) -> None: + """A worker who actually did work and committed it gets through.""" + # Make a second commit so HEAD differs from main's first commit + # but we still test against HEAD's diff against base. Setup: detach, + # add a new commit, then diff stat will be non-empty against `main` + # if HEAD has more. + new_file = git_workspace / "feature.py" + new_file.write_text("def real(): pass\n") + subprocess.run(["git", "checkout", "-q", "-b", "feature"], cwd=git_workspace, check=True) + subprocess.run(["git", "add", "feature.py"], cwd=git_workspace, check=True) + subprocess.run( + ["git", "commit", "-q", "-m", "feat"], cwd=git_workspace, check=True, + ) + v = verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Implemented feature module per spec", + ) + assert v is None + + def test_review_role_skipped(self, git_workspace: Path) -> None: + """Tony's deliverable is a verdict, not code — skip the diff gate.""" + assert ( + verify_workspace_diff( + assignee="tony", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="approve - implementation matches spec", + ) + is None + ) + + def test_orchestration_role_skipped(self, git_workspace: Path) -> None: + """JARVIS umbrella spawn doesn't ship code, even when body says + 'implemented chain'.""" + assert ( + verify_workspace_diff( + assignee="jarvis", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Spawned and implemented the build chain", + ) + is None + ) + + def test_scratch_workspace_skipped(self) -> None: + """scratch workspaces have no diff target.""" + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="scratch", + workspace_path=None, + summary="implemented thing", + ) + is None + ) + + def test_build_role_with_no_diff_rejects_regardless_of_summary_verb( + self, git_workspace: Path, + ) -> None: + """After the PR-#11 self-review fix: build-role + dir/worktree + workspace REQUIRES a non-empty diff. The earlier verb-trigger + version was bypassed by writing the summary without + implementation verbs. Now even a verb-free summary fails when + no diff exists. + """ + v = verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Investigated the issue; recommendations in comment.", + ) + assert v is not None + + def test_x_no_code_opt_out(self, git_workspace: Path) -> None: + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Implemented the docs reshuffle", + allow_no_code=True, + ) + is None + ) + + def test_nonexistent_workspace_path_skipped(self) -> None: + """We don't crash when workspace_path is wrong; we just skip.""" + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path="/tmp/this-does-not-exist-v67", + summary="implemented thing", + ) + is None + ) + + def test_empty_summary_does_not_crash(self, git_workspace: Path) -> None: + """Regression: empty/None summary on a build role with no diff + previously crashed `splitlines()[0]` before the gate could + surface the violation. Now the violation lands cleanly.""" + v_empty = verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="", + ) + assert isinstance(v_empty, WorkspaceDiffViolation) + v_none = verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary=None, + ) + assert isinstance(v_none, WorkspaceDiffViolation) + + +# ===================================================================== +# verify_no_stray_artifacts — #28 +# ===================================================================== + + +def _git_init(tmp_path: Path) -> None: + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], cwd=tmp_path, check=True, + ) + subprocess.run( + ["git", "config", "user.name", "test"], cwd=tmp_path, check=True, + ) + + +class TestStrayArtifacts: + def test_pr1_all_prior_block_evidence_files(self, tmp_path: Path) -> None: + """The literal failure mode from agent-dashboard PR #1: a file + named 'all prior block evidence files' (no extension) committed + because the evidence-path gate took a descriptive phrase + literally. + """ + _git_init(tmp_path) + stray = tmp_path / "all prior block evidence files" + stray.write_text("nothing\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run( + ["git", "add", "all prior block evidence files", "src.py"], + cwd=tmp_path, check=True, + ) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert isinstance(v, StrayArtifactViolation) + assert "all prior block evidence files" in v.stray_paths + + def test_commit_hash_txt_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "commit-hash.txt").write_text("abc123\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert "commit-hash.txt" in v.stray_paths + + def test_triage_dir_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + td = tmp_path / "triage" + td.mkdir() + (td / "v6.4-report.md").write_text("notes\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert any("triage/" in p for p in v.stray_paths) + + def test_evidence_subdir_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + ed = tmp_path / "changes" / "fix-14" / "evidence" + ed.mkdir(parents=True) + (ed / "out.json").write_text("{}\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert any("evidence" in p for p in v.stray_paths) + + def test_clean_repo_passes(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "src.py").write_text("print('hi')\n") + (tmp_path / "README.md").write_text("# hi\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert verify_no_stray_artifacts("dir", str(tmp_path)) is None + + def test_shebang_file_without_extension_is_ok(self, tmp_path: Path) -> None: + """Real scripts have shebangs — those aren't stray.""" + _git_init(tmp_path) + (tmp_path / "bin").mkdir() + script = tmp_path / "bin" / "deploy" + script.write_text("#!/bin/bash\necho hi\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert verify_no_stray_artifacts("dir", str(tmp_path)) is None + + def test_scratch_workspace_skipped(self) -> None: + assert verify_no_stray_artifacts("scratch", None) is None + + def test_x_stray_ok_opt_out(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "all prior block evidence files").write_text("x\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert ( + verify_no_stray_artifacts("dir", str(tmp_path), allow_stray=True) + is None + ) + + def test_nonexistent_workspace_path_skipped(self) -> None: + assert ( + verify_no_stray_artifacts("dir", "/tmp/does-not-exist-v67") is None + ) + + def test_tmp_prefixed_files_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "tmp-scratch").write_text("x\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert "tmp-scratch" in v.stray_paths + + # === post-self-review fixes === + + def test_tracked_LICENSE_and_Dockerfile_not_flagged( + self, tmp_path: Path, + ) -> None: + """The PR-#11 self-review found that the original gate flagged + every repo's tracked LICENSE / Dockerfile / Makefile as stray + because they have no extension. Real repos legitimately track + these — they predate the worker by years. + """ + _git_init(tmp_path) + (tmp_path / "LICENSE").write_text("MIT License\n") + (tmp_path / "Dockerfile").write_text("FROM alpine\n") + (tmp_path / "Makefile").write_text("all:\n\techo hi\n") + (tmp_path / "Vagrantfile").write_text("config\n") + (tmp_path / "README").write_text("project\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert verify_no_stray_artifacts("dir", str(tmp_path)) is None + + def test_evidence_substring_in_legitimate_filename_not_flagged( + self, tmp_path: Path, + ) -> None: + """The original `evidence|.*-evidence|.*_evidence` regex was so + broad it matched `evidence-types.md` (a legitimate doc in the + security skills tree) and `scripts/evidence-store.py` (a + legitimate source file). After tightening, those paths pass.""" + _git_init(tmp_path) + d = tmp_path / "optional-skills" / "security" / "references" + d.mkdir(parents=True) + (d / "evidence-types.md").write_text("# Evidence types\n") + scripts = tmp_path / "scripts" + scripts.mkdir() + (scripts / "evidence-store.py").write_text("def store(): pass\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert verify_no_stray_artifacts("dir", str(tmp_path)) is None + + def test_untracked_no_extension_file_still_flagged( + self, tmp_path: Path, + ) -> None: + """Untracked files with no extension and no shebang remain + stray. (Tracked ones we trust; untracked ones the worker added + this run.)""" + _git_init(tmp_path) + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "src.py"], cwd=tmp_path, check=True) + # Add the literal failure-mode file as UNTRACKED. + (tmp_path / "all prior block evidence files").write_text("x\n") + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert "all prior block evidence files" in v.stray_paths + + def test_evidence_dir_under_changes_still_flagged( + self, tmp_path: Path, + ) -> None: + """The agent-dashboard PR #1 failure: `changes/fix-14/evidence/` + subdirectory artifacts get flagged. Tightening the regex + shouldn't have lost this case.""" + _git_init(tmp_path) + ed = tmp_path / "changes" / "v6-6" / "evidence" + ed.mkdir(parents=True) + (ed / "block-c-test-output.json").write_text("{}\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert any("evidence" in p for p in v.stray_paths) + + def test_block_evidence_basename_still_flagged( + self, tmp_path: Path, + ) -> None: + """Files matching ``*-evidence.`` at any depth are flagged + even when their extension is legitimate. Catches the v6.6 + artifact basenames without false-positive-ing on + evidence-types.md.""" + _git_init(tmp_path) + d = tmp_path / "tests" / "data" + d.mkdir(parents=True) + (d / "block-c-evidence.json").write_text("{}\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert any("block-c-evidence.json" in p for p in v.stray_paths) + + +# ===================================================================== +# Opt-out audit + integration through complete_task — PR-#11 self-review +# ===================================================================== + + +import hermes_cli.kanban_db as kb +from hermes_cli.kanban_db import InvalidOptOutError + + +@pytest.fixture +def board_conn_with_task(tmp_path, monkeypatch): + """A board with a single running task with a scratch workspace and + a recently-claimed started_at so runtime-floor doesn't fire.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("HERMES_KANBAN_DB", str(tmp_path / "k.db")) + conn = kb.connect(board="default") + import time + now = int(time.time()) + started = now - 1000 # well above any floor + conn.execute( + "INSERT INTO tasks (id, title, status, assignee, started_at, " + " created_at, workspace_kind, workspace_path) " + "VALUES ('t_int', 'integration test task', 'running', 'jarvis', ?, " + " ?, 'scratch', NULL)", + (started, now), + ) + conn.commit() + yield conn + conn.close() + + +class TestOptOutAudit: + def test_truthy_bool_opt_out_rejected(self, board_conn_with_task) -> None: + """``x_fast_justified: true`` (a literal bool) must NOT be a free + bypass — the gate requires a substantive string reason.""" + with pytest.raises(InvalidOptOutError) as excinfo: + kb.complete_task( + board_conn_with_task, "t_int", + summary="quick", result="done", + metadata={"x_fast_justified": True}, + ) + assert excinfo.value.key == "x_fast_justified" + + def test_short_string_opt_out_rejected(self, board_conn_with_task) -> None: + """Reason shorter than 20 chars (after strip) rejected.""" + with pytest.raises(InvalidOptOutError): + kb.complete_task( + board_conn_with_task, "t_int", + summary="quick", result="done", + metadata={"x_no_code": "ok"}, + ) + + def test_whitespace_only_opt_out_rejected( + self, board_conn_with_task, + ) -> None: + """A reason that's just whitespace can't satisfy the audit.""" + with pytest.raises(InvalidOptOutError): + kb.complete_task( + board_conn_with_task, "t_int", + summary="quick", result="done", + metadata={"x_stray_ok": " "}, + ) + + def test_real_reason_opt_out_accepted_and_audited( + self, board_conn_with_task, + ) -> None: + """A real string reason ≥20 chars is accepted and emits a + ``completion_opt_out_used`` event with the verbatim reason.""" + ok = kb.complete_task( + board_conn_with_task, "t_int", + summary="docs-only reshuffle", result="done", + metadata={"x_fast_justified": + "one-line rename verified by smoke test"}, + ) + assert ok + events = board_conn_with_task.execute( + "SELECT kind, payload FROM task_events WHERE task_id = 't_int' " + "AND kind = 'completion_opt_out_used'" + ).fetchall() + assert len(events) == 1 + import json as _j + payload = _j.loads(events[0]["payload"]) + assert ( + payload["opt_outs"]["x_fast_justified"] + == "one-line rename verified by smoke test" + ) + + def test_false_opt_out_not_rejected_no_event( + self, board_conn_with_task, + ) -> None: + """``False`` and ``None`` are the natural absent values — they + skip validation and emit no opt-out event.""" + ok = kb.complete_task( + board_conn_with_task, "t_int", + summary="done", result="done", + metadata={"x_fast_justified": False, "x_no_code": None}, + ) + assert ok + events = board_conn_with_task.execute( + "SELECT count(*) AS n FROM task_events WHERE task_id = 't_int' " + "AND kind = 'completion_opt_out_used'" + ).fetchone() + assert events["n"] == 0 + + +class TestCompleteTaskIntegration: + """End-to-end coverage through the public complete_task entrypoint. + These tests would have caught wiring regressions in PR #11 / #12 + that the pure-function tests miss. + """ + + def test_friday_below_floor_blocks_and_emits_event( + self, board_conn_with_task, + ) -> None: + # Re-claim as friday with a recent started_at to trip the floor. + board_conn_with_task.execute( + "UPDATE tasks SET assignee = 'friday', started_at = ? " + "WHERE id = 't_int'", + (int(__import__("time").time()) - 10,), # 10s ago, well below 5min + ) + board_conn_with_task.commit() + with pytest.raises(kb.CompletionGateError): + kb.complete_task( + board_conn_with_task, "t_int", + summary="implemented thing", result="done", + ) + # Task state preserved + row = board_conn_with_task.execute( + "SELECT status FROM tasks WHERE id = 't_int'" + ).fetchone() + assert row["status"] == "running" + events = board_conn_with_task.execute( + "SELECT kind FROM task_events WHERE task_id = 't_int' " + "ORDER BY id DESC LIMIT 1" + ).fetchone() + assert events["kind"] == "completion_blocked_v6_7_gates" + + def test_clean_completion_passes_all_gates( + self, board_conn_with_task, + ) -> None: + """Default jarvis (no floor) + scratch workspace (no diff/stray + gate) + no opt-outs = clean pass.""" + ok = kb.complete_task( + board_conn_with_task, "t_int", + summary="spawned chain", result="done", + ) + assert ok + row = board_conn_with_task.execute( + "SELECT status FROM tasks WHERE id = 't_int'" + ).fetchone() + assert row["status"] == "done" diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 20a522f90a4b..c87dd2906d1e 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -580,6 +580,24 @@ def _handle_complete(args: dict, **kw) -> str: f"and either drop these ids from created_cards, or pass " f"created_cards=[] to skip the card-claim check entirely." ) + except kb.CompletionGateError as gate_err: + # v6.7 verification gates rejected. Task state unchanged — + # the worker retries kanban_complete after fixing the + # underlying issue, OR calls kanban_block honestly. See + # hermes-jarvis#61 for why these gates exist. + return tool_error( + f"{gate_err}\n" + f"Your task is still in-flight (no state change). " + f"Address each violation above and retry kanban_complete." + ) + except kb.InvalidOptOutError as opt_err: + # v6.7 opt-out validation rejected — the worker passed a + # truthy-but-non-substantive value for an opt-out key. + # Forces an audit-friendly string reason for every bypass. + return tool_error( + f"{opt_err}\n" + f"Your task is still in-flight (no state change)." + ) if not ok: return tool_error( f"could not complete {tid} (unknown id or already terminal)"