From 307e5677ea3d9658d5a310ce35fe05255686dde8 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Tue, 9 Jun 2026 14:44:51 -0700 Subject: [PATCH 1/3] =?UTF-8?q?feat(kanban):=20v6.7=20Tranche=201=20?= =?UTF-8?q?=E2=80=94=20kanban=5Fcomplete=20verification=20gates?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds three pre-write-txn gates that fire before complete_task transitions a task to done. Mirrors the existing _verify_created_cards / HallucinatedCardsError pattern: any violation is recorded as an audit event and raised, so worker state is unchanged and the worker can retry after fixing the underlying issue. Closes hermes-jarvis#28 (repo hygiene gate) Closes hermes-jarvis#62 (workspace-diff verification) Closes hermes-jarvis#64 (per-role runtime floor) Context: hermes-jarvis#61 (bootstrap-paradox case study) ## The three gates 1. verify_runtime_floor — per-role floor on completed_at - started_at. build roles 5min, review roles 90s, orchestration roles 0. Catches Tony's 20-second "approve" verdicts and Friday's 59-second "implemented 7 dispatcher gates" claims. 2. verify_workspace_diff — when a non-review worker on a dir/worktree workspace claims to have produced code, git diff against the tracking base must show actual changes. Catches Friday's "Wave A gates implemented" with an empty diff on the branch. 3. verify_no_stray_artifacts — rejects untracked or tracked 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 from agent-dashboard PR #1). ## Opt-outs Workers may bypass individual gates via per-call metadata keys: x_fast_justified → allow_below_floor x_no_code → allow_no_code x_stray_ok → allow_stray Opt-outs are recorded as part of the completed event for audit. ## Tests 28 new tests cover the exact 2026-06-09 failure modes (Tony 20s, Friday 59s + empty diff, PR-1 "all prior block evidence files") plus clean-path passes and opt-outs. 258 passed / 0 failed in the wider kanban+complete+task test suite — zero regressions. Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/kanban_completion_gates.py | 381 ++++++++++++++++++++++ hermes_cli/kanban_db.py | 89 +++++ tests/cli/test_kanban_completion_gates.py | 335 +++++++++++++++++++ tools/kanban_tools.py | 10 + 4 files changed, 815 insertions(+) create mode 100644 hermes_cli/kanban_completion_gates.py create mode 100644 tests/cli/test_kanban_completion_gates.py diff --git a/hermes_cli/kanban_completion_gates.py b/hermes_cli/kanban_completion_gates.py new file mode 100644 index 000000000000..00ce28f6c22b --- /dev/null +++ b/hermes_cli/kanban_completion_gates.py @@ -0,0 +1,381 @@ +"""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"} + +# Phrases workers used in fabricated completion summaries that should be +# backed by a real diff. Conservative — only triggers the gate when the +# worker has explicitly claimed code changes. +_IMPLEMENTATION_CLAIM_PATTERNS = [ + re.compile(r"\b(implement(?:ed|s)?|build(?:s|t)?|add(?:ed|s)?|" + r"creat(?:ed|es)?|wrote|wr(?:ites|ote)|ship(?:ped|s)?|" + r"land(?:ed|s)?|introduc(?:ed|es)?|refactor(?:ed|s)?|" + r"fix(?:ed|es)?|patch(?:ed|es)?)\b", re.IGNORECASE), +] + + +@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 _summary_claims_implementation(summary: str) -> bool: + return any(p.search(summary or "") for p in _IMPLEMENTATION_CLAIM_PATTERNS) + + +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 + if not _summary_claims_implementation(summary or ""): + return None + 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 + summary_excerpt = (summary or "").strip().splitlines()[0][:200] + 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. Aligned with the agent-dashboard PR #1 audit findings ( +# `all prior block evidence files`, `commit-hash.txt`, `triage/v6.4-*`). +_STRAY_PATH_PATTERNS = [ + re.compile(r"(^|/)(evidence|.*-evidence|.*_evidence)(/|\b)", 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), +] + + +@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) -> bool: + """True if ``rel_path`` looks like a stray artifact.""" + norm = rel_path.replace("\\", "/") + if any(p.search(norm) for p in _STRAY_PATH_PATTERNS): + return True + # Tracked file with no extension and no shebang — the "all prior block + # evidence files" failure mode. + base = os.path.basename(norm) + if "." not in base and not _has_shebang(os.path.join(repo_root, rel_path)): + return True + return False + + +def _list_workspace_files(workspace_path: str) -> list[str]: + """Return the union of `git ls-files` (tracked) and `git ls-files + --others --exclude-standard` (untracked & not gitignored), as relative + paths. Empty list 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 + + tracked = _run(["git", "ls-files"]) or "" + untracked = _run(["git", "ls-files", "--others", "--exclude-standard"]) or "" + paths = set() + for blob in (tracked, untracked): + for line in blob.splitlines(): + line = line.strip() + if line: + paths.add(line) + return sorted(paths) + + +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 + stray = [p for p in _list_workspace_files(workspace_path) + if _stray_path_score(workspace_path, p)] + 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..b7eef59eeda3 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,64 @@ def __init__(self, phantom: list[str], completing_task_id: str): ) +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 + (``x_fast_justified``, ``x_no_code``, ``x_stray_ok``) which surface as + the gate functions' ``allow_*`` kwargs. Opt-outs are recorded as part + of the completed event for audit. + """ + 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 = bool(md.get("x_fast_justified")) + no_code = bool(md.get("x_no_code")) + stray_ok = bool(md.get("x_stray_ok")) + 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 +3691,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/tests/cli/test_kanban_completion_gates.py b/tests/cli/test_kanban_completion_gates.py new file mode 100644 index 000000000000..448c49515525 --- /dev/null +++ b/tests/cli/test_kanban_completion_gates.py @@ -0,0 +1,335 @@ +"""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_no_implementation_claim_skipped(self, git_workspace: Path) -> None: + """Summary that doesn't claim code work doesn't trip the gate.""" + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Investigated the issue; recommendations in comment.", + ) + is 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 + ) + + +# ===================================================================== +# 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 diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 20a522f90a4b..562dd1e9aeb3 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -580,6 +580,16 @@ 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." + ) if not ok: return tool_error( f"could not complete {tid} (unknown id or already terminal)" From aa127bd8c831f00dd601f5d86e662b8eb4582bc7 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Tue, 9 Jun 2026 16:12:41 -0700 Subject: [PATCH 2/3] =?UTF-8?q?fix(kanban):=20v6.7=20Part=201=20self-revie?= =?UTF-8?q?w=20=E2=80=94=20opt-out=20audit,=20hygiene=20scoping,=20dead-co?= =?UTF-8?q?de=20removal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of PR #11 surfaced two P0s and several P1s that would have actively recreated the v6.7 failure mode the gates exist to prevent. This commit addresses every finding. Findings + fixes: ## P0 — Opt-outs were free keyword bypasses `bool(metadata.get("x_fast_justified"))` accepted `True`, `"x"`, `"ok"` — all truthy under bool(). A worker that learned the metadata key could pass `{"x_fast_justified": true}` and bypass every gate. The docstring also lied: it claimed "opt-outs are recorded as part of the completed event for audit" but nothing in `complete_task` copied the opt-out keys to any event. Fix: new `_validate_opt_out` requires the value be a STRING of at least 20 non-whitespace characters and raises `InvalidOptOutError` on anything else. Accepted opt-outs are emitted as a `completion_opt_out_used` event with verbatim reasons. ## P0 — Hygiene gate false-positived on legitimate tracked files `verify_no_stray_artifacts` flagged `LICENSE`, `Dockerfile`, `Makefile`, `Vagrantfile`, `README` because the no-extension rule was applied universally. The `evidence` regex also matched legitimate filenames like `optional-skills/security/oss-forensics/references/ evidence-types.md` and `scripts/evidence-store.py`. Verified by running the gate against the hermes-agent repo itself: 15 stray paths, all legitimate. Fix: - Tightened patterns: `evidence/` must be a directory segment; `*-evidence.` only matches with explicit artifact extensions (.json/.png/.log/.txt/.md/.yaml/.yml); `triage/` matches as a segment. - No-extension+no-shebang rule now applies to UNTRACKED files only. Specific patterns (commit-hash.txt, triage/, tmp-*) still match tracked files too — a worker who actually committed those is just as wrong. - Allowlist for well-known no-extension basenames (LICENSE, Dockerfile, Makefile, ...) as defense in depth. ## P1 — Workspace-diff was bypassable by avoiding trigger verbs `_summary_claims_implementation` only triggered on a fixed verb list. A worker who wrote "Per spec, the changes land in hermes_cli and tests pass" had no trigger verb and bypassed the gate even with an empty branch. Fix: build-role + dir/worktree workspace ALWAYS requires a non-empty diff. Workers who honestly produced no code should call kanban_block with a reason, or opt out via x_no_code with a string justification ≥20 chars. Dead `_summary_claims_implementation` and `_IMPLEMENTATION_CLAIM_PATTERNS` removed. ## P1 — Other production callers didn't catch the new errors `complete_task` is also called from `hermes_cli/kanban.py:_cmd_complete` (CLI) and `plugins/kanban/dashboard/plugin_api.py` (PATCH endpoints ×2). Without explicit catches, the new exceptions would have been stack traces — to stderr in the CLI, to a 500 in the dashboard. Fix: CLI prints the error and continues per task; dashboard endpoints translate to HTTP 409 (single PATCH) or per-entry error (bulk). Block-task callers updated for `FabricatedAuthClaimError` too. ## P1 — Pure-function tests missed integration regressions Added `TestOptOutAudit` (5 tests) and `TestCompleteTaskIntegration` (2 tests) that exercise the full `complete_task` → gate → event → state-unchanged path. A wiring regression in `_v6_7_run_completion_gates` would now fail in CI. ## Test results 40 passed in tests/cli/test_kanban_completion_gates.py — up from 28 in the original PR #11 (12 new + 1 replacing the verb-trigger test). Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/kanban.py | 20 +- hermes_cli/kanban_completion_gates.py | 126 +++++++---- hermes_cli/kanban_db.py | 81 ++++++- plugins/kanban/dashboard/plugin_api.py | 52 +++-- tests/cli/test_kanban_completion_gates.py | 264 +++++++++++++++++++++- tools/kanban_tools.py | 8 + 6 files changed, 471 insertions(+), 80 deletions(-) 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 index 00ce28f6c22b..69a5df7dcef9 100644 --- a/hermes_cli/kanban_completion_gates.py +++ b/hermes_cli/kanban_completion_gates.py @@ -125,16 +125,6 @@ def verify_runtime_floor( REVIEW_ROLES = {"tony", "tchalla", "vision", "reviewer"} ORCHESTRATION_ROLES = {"jarvis", "pepper", "banner"} -# Phrases workers used in fabricated completion summaries that should be -# backed by a real diff. Conservative — only triggers the gate when the -# worker has explicitly claimed code changes. -_IMPLEMENTATION_CLAIM_PATTERNS = [ - re.compile(r"\b(implement(?:ed|s)?|build(?:s|t)?|add(?:ed|s)?|" - r"creat(?:ed|es)?|wrote|wr(?:ites|ote)|ship(?:ped|s)?|" - r"land(?:ed|s)?|introduc(?:ed|es)?|refactor(?:ed|s)?|" - r"fix(?:ed|es)?|patch(?:ed|es)?)\b", re.IGNORECASE), -] - @dataclass(frozen=True) class WorkspaceDiffViolation: @@ -155,10 +145,6 @@ def message(self) -> str: ) -def _summary_claims_implementation(summary: str) -> bool: - return any(p.search(summary or "") for p in _IMPLEMENTATION_CLAIM_PATTERNS) - - def _git_diff_stat_against_base(workspace_path: str) -> str: """Return `git diff --stat` against the workspace's tracking base. @@ -228,8 +214,16 @@ def verify_workspace_diff( # Wrong / typo'd path is the dispatcher's problem to surface # elsewhere — we don't punish the worker for it. return None - if not _summary_claims_implementation(summary or ""): - 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. @@ -246,18 +240,47 @@ def verify_workspace_diff( # 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. Aligned with the agent-dashboard PR #1 audit findings ( -# `all prior block evidence files`, `commit-hash.txt`, `triage/v6.4-*`). +# 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 = [ - re.compile(r"(^|/)(evidence|.*-evidence|.*_evidence)(/|\b)", re.IGNORECASE), + # `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"(^|/)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: @@ -285,26 +308,39 @@ def _has_shebang(path: str) -> bool: return False -def _stray_path_score(repo_root: str, rel_path: str) -> bool: - """True if ``rel_path`` looks like a stray artifact.""" +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 - # Tracked file with no extension and no shebang — the "all prior block - # evidence files" failure mode. + 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(workspace_path: str) -> list[str]: - """Return the union of `git ls-files` (tracked) and `git ls-files - --others --exclude-standard` (untracked & not gitignored), as relative - paths. Empty list on any git error. +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 [] + return [], [] def _run(args: list[str]) -> Optional[str]: try: @@ -318,15 +354,14 @@ def _run(args: list[str]) -> Optional[str]: return None return out.stdout - tracked = _run(["git", "ls-files"]) or "" - untracked = _run(["git", "ls-files", "--others", "--exclude-standard"]) or "" - paths = set() - for blob in (tracked, untracked): - for line in blob.splitlines(): - line = line.strip() - if line: - paths.add(line) - return sorted(paths) + 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( @@ -348,8 +383,15 @@ def verify_no_stray_artifacts( return None if not workspace_path or not os.path.isdir(workspace_path): return None - stray = [p for p in _list_workspace_files(workspace_path) - if _stray_path_score(workspace_path, p)] + 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( diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index b7eef59eeda3..054b5c1d4998 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -3566,6 +3566,49 @@ 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, @@ -3580,10 +3623,18 @@ def _v6_7_run_completion_gates( 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 - (``x_fast_justified``, ``x_no_code``, ``x_stray_ok``) which surface as - the gate functions' ``allow_*`` kwargs. Opt-outs are recorded as part - of the completed event for audit. + 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 " @@ -3593,9 +3644,25 @@ def _v6_7_run_completion_gates( if row is None: return [] md = metadata or {} - fast_ok = bool(md.get("x_fast_justified")) - no_code = bool(md.get("x_no_code")) - stray_ok = bool(md.get("x_stray_ok")) + 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"], 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 index 448c49515525..3c070b96dd06 100644 --- a/tests/cli/test_kanban_completion_gates.py +++ b/tests/cli/test_kanban_completion_gates.py @@ -188,17 +188,22 @@ def test_scratch_workspace_skipped(self) -> None: is None ) - def test_no_implementation_claim_skipped(self, git_workspace: Path) -> None: - """Summary that doesn't claim code work doesn't trip the gate.""" - assert ( - verify_workspace_diff( - assignee="friday", - workspace_kind="dir", - workspace_path=str(git_workspace), - summary="Investigated the issue; recommendations in comment.", - ) - 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 ( @@ -333,3 +338,242 @@ def test_tmp_prefixed_files_stray(self, tmp_path: Path) -> None: 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 562dd1e9aeb3..c87dd2906d1e 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -590,6 +590,14 @@ def _handle_complete(args: dict, **kw) -> str: 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)" From 6b1bcd14f94477d8f41ea306ca6c35215f5e347e Mon Sep 17 00:00:00 2001 From: Jarvis Date: Tue, 9 Jun 2026 16:36:32 -0700 Subject: [PATCH 3/3] =?UTF-8?q?fix(kanban):=20N1=20self-review=20=E2=80=94?= =?UTF-8?q?=20empty=20summary=20no=20longer=20crashes=20workspace-diff=20g?= =?UTF-8?q?ate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Empty/None summary on a build role with no diff previously crashed `splitlines()[0]` before `verify_workspace_diff` could surface its violation. The crash bubbled past the gate, skipped the `completion_blocked_v6_7_gates` audit event, and surfaced as a 500 in the dashboard. Fix: guard the excerpt construction. Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/kanban_completion_gates.py | 8 +++++++- tests/cli/test_kanban_completion_gates.py | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/hermes_cli/kanban_completion_gates.py b/hermes_cli/kanban_completion_gates.py index 69a5df7dcef9..ed541ac04328 100644 --- a/hermes_cli/kanban_completion_gates.py +++ b/hermes_cli/kanban_completion_gates.py @@ -229,7 +229,13 @@ def verify_workspace_diff( # the diff is empty / whitespace. if diff_stat and diff_stat.strip(): return None - summary_excerpt = (summary or "").strip().splitlines()[0][:200] + # 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, diff --git a/tests/cli/test_kanban_completion_gates.py b/tests/cli/test_kanban_completion_gates.py index 3c070b96dd06..a5c39db86b31 100644 --- a/tests/cli/test_kanban_completion_gates.py +++ b/tests/cli/test_kanban_completion_gates.py @@ -229,6 +229,25 @@ def test_nonexistent_workspace_path_skipped(self) -> None: 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