From c983cef17c4109ba45ef9da340851692630f47de Mon Sep 17 00:00:00 2001 From: Dimitri Krattiger Date: Thu, 23 Jul 2026 20:46:25 +0000 Subject: [PATCH] Add opt-in tarot review-artifact gate for GitHub-forge workflows Repos that set `capabilities.tarot_review` get a real, verified ITERATING responsibility (unlike every other responsibility, which is agent self-attested): a `PreToolUse` hook on `apply_operation` runs `tarot strands check` / `tarot tour check` in the container and denies `advance` on failure, with the checks' output fed back as the reason. A trivial diff (below a changed-line threshold) auto-resolves the responsibility without running the checks. Threads an optional `repo` through `Workflow.responsibilities()` and the transition methods so a workflow can vary what it promises per repo. Co-Authored-By: Claude Sonnet 5 --- AGENTS.md | 6 +- docs/repos.md | 28 ++- src/panopticon/container/hooks.py | 19 +- src/panopticon/container/tarot_gate.py | 188 ++++++++++++++++ src/panopticon/core/workflow.py | 40 +++- src/panopticon/taskservice/service.py | 13 +- src/panopticon/workflows/github_forge.py | 34 ++- tests/container/test_hooks.py | 10 + tests/container/test_tarot_gate.py | 265 +++++++++++++++++++++++ tests/core/test_workflow.py | 83 +++++++ tests/workflows/test_github_forge.py | 83 +++++++ 11 files changed, 743 insertions(+), 26 deletions(-) create mode 100644 src/panopticon/container/tarot_gate.py create mode 100644 tests/container/test_tarot_gate.py create mode 100644 tests/workflows/test_github_forge.py diff --git a/AGENTS.md b/AGENTS.md index 339639fd..dce07de7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,7 +52,11 @@ src/panopticon/ container/ # entrypoint (`python -m panopticon.container` = connect/register/slug/ # heartbeat liveness) + agent.py (`-m panopticon.container.agent` = the tmux # pane's launcher: render skills + operations, point claude at the /mcp server, - # put the workflow overview in its system prompt → exec `claude`) — the ONLY LLM pkg + # put the workflow overview in its system prompt → exec `claude`) — the ONLY LLM pkg; + # tarot_gate.py = the `PreToolUse` hook on `apply_operation` that real-verifies + # (not self-attests) the opt-in tarot review-artifact responsibility for the + # GitHub-forge workflows (`Repo.capabilities.tarot_review`, docs/repos.md) — runs + # `tarot strands check` / `tarot tour check` and denies `advance` on failure docker/Dockerfile # base task-container image (ADR 0005 base layer): python + git + bash + # the panopticon package + the `claude` CLI the agent execs; runs as the # unprivileged `panopticon` user. docker/entrypoint.sh = remap that user to the diff --git a/docs/repos.md b/docs/repos.md index 599c279d..193b4327 100644 --- a/docs/repos.md +++ b/docs/repos.md @@ -70,11 +70,29 @@ and secrets are never baked in — they're injected at run time via `env_file`. ## Capabilities `capabilities` is a JSON opt-in map for elevated container privileges the runner grants at -spawn. The first (and currently only) capability is `docker_in_docker`: set it and the runner -spawns the container `--privileged`, gives it a volume for `/var/lib/docker`, and the entrypoint -starts a nested Docker daemon. It's **off by default** because it's a trust escalation — a -privileged container is effectively host root — so a repo opts in only when its tasks genuinely -need to run Docker. +spawn. `docker_in_docker`: set it and the runner spawns the container `--privileged`, gives it a +volume for `/var/lib/docker`, and the entrypoint starts a nested Docker daemon. It's **off by +default** because it's a trust escalation — a privileged container is effectively host root — so +a repo opts in only when its tasks genuinely need to run Docker. + +### `tarot_review` — the tarot review-artifact gate + +For the GitHub-forge workflows (`github-peer-reviewed`, `github-self-reviewed`), setting +`capabilities.tarot_review` adds a real, verified ITERATING responsibility: before `advance` out +of ITERATING is allowed, the task must author `.tarot/strands.json` and (for non-trivial changes) +a tour, passing `tarot strands check` / `tarot tour check`. Unlike every other responsibility +(agent self-attested), this one is checked by an in-container hook that runs the actual commands +and denies the `advance` call — with the checks' output as the reason — on failure. A trivial +diff (below a changed-line threshold, default 20; override with the integer capability +`tarot_review_threshold`) skips the checks and resolves the responsibility automatically. + +The `tarot` CLI itself isn't installed by panopticon — a repo that opts in must add it to its own +`image_layer_file` (for example `RUN pip install tarot-review`), since installing it into the +shared workflow layer would force it onto every forge repo whether opted in or not. If `tarot` +isn't on `PATH` for an opted-in repo, the gate denies with a message pointing back here. + +Off by default, like `docker_in_docker` — a repo opts in only once it has installed `tarot` via +its image layer. ## Host hook (`hook_file`) diff --git a/src/panopticon/container/hooks.py b/src/panopticon/container/hooks.py index 8e521b69..80f70240 100644 --- a/src/panopticon/container/hooks.py +++ b/src/panopticon/container/hooks.py @@ -8,10 +8,13 @@ - **PreToolUse**/**PostToolUse** matched to the ``AskUserQuestion`` tool flip to the *user* while the agent is asking the user something (so the dashboard shows input is required) and back to the *agent* once it's answered. ``AskUserQuestion`` is a mid-turn tool call — it never fires ``Stop`` - — so without this the turn would wrongly read *agent* the whole time the question is pending. + — so without this the turn would wrongly read *agent* the whole time the question is pending; +- **PreToolUse** matched to the `apply_operation` MCP tool also runs the tarot review-artifact + gate (:mod:`panopticon.container.tarot_gate`) — a real, opt-in check (not just turn bookkeeping) + that can deny an `advance` call; see that module for what it does and why. -claude-specific (`.claude/settings.json`); M3 revisits for other CLIs. Pure — the callback the -hooks invoke is :mod:`panopticon.container.hook`. `:blocked:` is preserved by construction: the +claude-specific (`.claude/settings.json`); M3 revisits for other CLIs. Pure — the turn-flip +callback is :mod:`panopticon.container.hook`. `:blocked:` is preserved by construction: the callback only sets the turn, never the block. """ @@ -24,6 +27,8 @@ #: The command claude runs for each hook event (sets the turn via the task service). HOOK_COMMAND = "python -m panopticon.container.hook" +#: The command claude runs on every `apply_operation` call (the tarot review-artifact gate). +TAROT_GATE_COMMAND = "python -m panopticon.container.tarot_gate" def settings() -> dict[str, Any]: @@ -53,7 +58,13 @@ def run(actor: str, event: str | None = None, *, matcher: str | None = None) -> "Stop": [run("user", "stop")], "UserPromptSubmit": [run("agent", "prompt")], # The agent stops to ask the user → flip to user; once answered → back to agent. - "PreToolUse": [run("user", matcher="AskUserQuestion")], + "PreToolUse": [ + run("user", matcher="AskUserQuestion"), + { + "matcher": "mcp__panopticon__apply_operation", + "hooks": [{"type": "command", "command": TAROT_GATE_COMMAND}], + }, + ], "PostToolUse": [run("agent", matcher="AskUserQuestion")], }, "skipDangerousModePermissionPrompt": True, diff --git a/src/panopticon/container/tarot_gate.py b/src/panopticon/container/tarot_gate.py new file mode 100644 index 00000000..996a23a2 --- /dev/null +++ b/src/panopticon/container/tarot_gate.py @@ -0,0 +1,188 @@ +"""The tarot review-artifact gate — a `PreToolUse` hook on `apply_operation` (advance). + +For a repo that opts in (`Repo.capabilities["tarot_review"]`, see +:mod:`panopticon.workflows.github_forge`), every other ITERATING responsibility is agent +self-attested, but this one is real-verified: this hook intercepts the `advance` operation while +the task is in ITERATING and runs `tarot strands check` / `tarot tour check` in `/workspace`, +denying the tool call (with the checks' output as the reason, so it lands in the agent's context +like a failed test would) unless they pass. A trivial diff (below a changed-line threshold) skips +the checks and auto-resolves the responsibility instead — no tour to write for a one-line fix. + +Registered unconditionally in `container/hooks.py` (like the turn-flip hooks), regardless of +workflow — irrelevant calls (a different operation, a non-ITERATING state, a non-opted-in repo) +resolve in the first couple of checks below and allow immediately. Deterministic and LLM-free: +only subprocess + REST calls, so it's unit-tested with a fake command runner and a fake client +(no real `tarot` binary needed), the same shape as :mod:`panopticon.container.hook`. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from collections.abc import Sequence +from dataclasses import dataclass +from typing import Any, Protocol, TextIO + +import httpx + +from panopticon.client import JsonObj, TaskServiceClient +from panopticon.core.models import Status + +#: The `Repo.capabilities` key that opts a repo into this gate (mirrors `docker_in_docker`). +TAROT_REVIEW_CAPABILITY = "tarot_review" +#: Optional per-repo override (an int) of the trivial-diff line threshold, under `capabilities`. +TAROT_REVIEW_THRESHOLD_CAPABILITY = "tarot_review_threshold" +#: The ITERATING responsibility this gate verifies (see `GithubForgeWorkflow.TAROT_REVIEW_ARTIFACTS`). +RESPONSIBILITY_KEY = "tarot-review-artifacts" +#: Below this many total changed lines (git `diff --numstat`, added + removed), the diff is +#: considered trivial and the tarot checks are skipped entirely. +DEFAULT_TRIVIAL_THRESHOLD = 20 +WORKSPACE = "/workspace" + + +@dataclass(frozen=True) +class CommandResult: + """The outcome of running one external command — never raises; the caller inspects it.""" + + returncode: int + output: str # combined stdout+stderr + found: bool = True # False when the executable itself wasn't found on PATH + + +class CommandRunner(Protocol): + def __call__(self, args: Sequence[str], *, cwd: str | None = None) -> CommandResult: ... + + +def _subprocess_run(args: Sequence[str], *, cwd: str | None = None) -> CommandResult: + try: + proc = subprocess.run(list(args), cwd=cwd, capture_output=True, text=True, check=False) + except FileNotFoundError: + return CommandResult(returncode=127, output=f"{args[0]}: command not found", found=False) + return CommandResult(returncode=proc.returncode, output=proc.stdout + proc.stderr) + + +def _read_payload(stdin: TextIO) -> dict[str, Any]: + """Tolerantly parse the hook's stdin JSON; empty/invalid input yields an empty payload.""" + try: + raw = stdin.read() + except (OSError, ValueError): + return {} + if not raw or not raw.strip(): + return {} + try: + data = json.loads(raw) + except json.JSONDecodeError: + return {} + return data if isinstance(data, dict) else {} + + +def _allow() -> int: + """No stdout, exit 0 — Claude Code lets the tool call through unmodified.""" + return 0 + + +def _deny(reason: str) -> int: + """Structured `PreToolUse` denial: the reason string lands in the agent's context as the + tool call's failure, the same seam a failed test's output would use.""" + print( + json.dumps( + { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason, + } + } + ) + ) + return 0 + + +def _opted_in(repo: JsonObj) -> bool: + return bool((repo.get("capabilities") or {}).get(TAROT_REVIEW_CAPABILITY)) + + +def _threshold(repo: JsonObj) -> int: + value = (repo.get("capabilities") or {}).get(TAROT_REVIEW_THRESHOLD_CAPABILITY) + return value if isinstance(value, int) else DEFAULT_TRIVIAL_THRESHOLD + + +def _changed_line_count(run: CommandRunner, *, base_ref: str) -> int: + """Total added+removed lines between ``base_ref`` and ``HEAD`` (a `diff --numstat` sum) — the + trivial-diff heuristic. Binary files report ``-`` counts, which don't parse as digits and are + skipped rather than counted.""" + result = run(["git", "-C", WORKSPACE, "diff", "--numstat", f"{base_ref}...HEAD"]) + total = 0 + for line in result.output.splitlines(): + added, _, rest = line.partition("\t") + removed, _, _path = rest.partition("\t") + for count in (added, removed): + if count.isdigit(): + total += int(count) + return total + + +def _run_tarot_checks(run: CommandRunner) -> CommandResult | None: + """Run `tarot strands check` then `tarot tour check`; stop at the first failure (nothing to + gain running both once one has already failed). ``None`` means both passed.""" + for args in (["tarot", "strands", "check"], ["tarot", "tour", "check"]): + result = run(args, cwd=WORKSPACE) + if result.returncode != 0: + return result + return None + + +def main( + *, + client: TaskServiceClient | None = None, + stdin: TextIO | None = None, + run: CommandRunner = _subprocess_run, +) -> int: + payload = _read_payload(stdin or sys.stdin) + tool_input = payload.get("tool_input") + if not isinstance(tool_input, dict) or tool_input.get("operation") != "advance": + return _allow() + + env = os.environ + task_id = env["PANOPTICON_TASK_ID"] + client = client or TaskServiceClient(httpx.Client(base_url=env["PANOPTICON_SERVICE_URL"])) + + task = client.get_task(task_id) + if task.get("state") != "ITERATING": + return _allow() + + repo = client.get_repo(task["repo_id"]) + if not _opted_in(repo): + return _allow() + + base_ref = f"origin/{repo.get('default_base', 'main')}" + if _changed_line_count(run, base_ref=base_ref) < _threshold(repo): + client.resolve_responsibility( + task_id, + RESPONSIBILITY_KEY, + Status.MET, + comment="trivial diff — tarot review skipped", + ) + return _allow() + + failure = _run_tarot_checks(run) + if failure is None: + client.resolve_responsibility( + task_id, + RESPONSIBILITY_KEY, + Status.MET, + comment="verified by tarot strands check / tarot tour check", + ) + return _allow() + if not failure.found: + return _deny( + "`tarot` is not installed in this container. An opted-in repo must install it via " + "its `image_layer_file` (see docs/repos.md)." + ) + return _deny(failure.output) + + +if __name__ == "__main__": # pragma: no cover + raise SystemExit(main()) diff --git a/src/panopticon/core/workflow.py b/src/panopticon/core/workflow.py index ef3bcdd0..2d77eb54 100644 --- a/src/panopticon/core/workflow.py +++ b/src/panopticon/core/workflow.py @@ -17,7 +17,7 @@ from typing import ClassVar from panopticon.core.artifacts import ArtifactStore -from panopticon.core.models import Actor, HistoryEntry, Responsibility, Skill, Task, Tool +from panopticon.core.models import Actor, HistoryEntry, Repo, Responsibility, Skill, Task, Tool from panopticon.core.state import BaseState, Complete, Dropped, InitialState, State, TerminalState _ABSTRACT_BASES = (BaseState, State, TerminalState) @@ -291,8 +291,14 @@ def advanced_by(self, label: str) -> Actor: raise InvalidWorkflow(f"{self.name!r}: terminal state {label!r} does not advance") return cls.advanced_by - def responsibilities(self, label: str) -> Iterator[Responsibility]: - """Yield the obligations (PENDING definitions) the agent takes on entering ``label``.""" + def responsibilities(self, label: str, *, repo: Repo | None = None) -> Iterator[Responsibility]: + """Yield the obligations (PENDING definitions) the agent takes on entering ``label``. + + ``repo`` is the task's repo, when known — ``None`` by default (most workflows are + repo-agnostic). A subclass may override this to vary its declared responsibilities per + repo (e.g. a repo opted into an extra review convention); the base implementation ignores + it and always yields the state's static list. + """ yield from self._state_class(label).responsibilities def skills(self) -> Sequence[Skill]: @@ -487,9 +493,9 @@ def provision(self, task: Task, *, branch: str, worktree_path: str) -> None: # -- task lifecycle (deterministic: no clock, no I/O; timestamps passed in) --------- - def _promised(self, label: str) -> list[Responsibility]: + def _promised(self, label: str, *, repo: Repo | None = None) -> list[Responsibility]: """A fresh PENDING responsibility list to seed the history entry for entering ``label``.""" - return list(self.responsibilities(label)) + return list(self.responsibilities(label, repo=repo)) def start_task( self, @@ -499,12 +505,15 @@ def start_task( at: str, memo: str | None = None, initial_prompt: str | None = None, + repo: Repo | None = None, ) -> Task: """Create a task in this workflow's initial state, with turn and seed history set. The seed history entry carries the initial state's responsibilities (all ``PENDING``). ``memo`` is the optional brief one-line reminder of what the task is, collected at creation. ``initial_prompt`` is optional text prefilled into Claude's input box on first spawn. + ``repo`` is the task's repo, when known — passed through to :meth:`responsibilities` so a + workflow can vary what it promises per repo; ``None`` (the default) if not needed/known. """ state = self.initial_label return Task( @@ -522,7 +531,7 @@ def start_task( from_state=None, to_state=state, trigger="start", - responsibilities=self._promised(state), + responsibilities=self._promised(state, repo=repo), ) ], ) @@ -535,6 +544,7 @@ def apply_transition( at: str, trigger: str | None = None, note: str | None = None, + repo: Repo | None = None, ) -> Task: """Validate and apply a transition, mutating ``task`` in place and returning it. @@ -543,7 +553,7 @@ def apply_transition( is resolved (each ``MET`` or ``FAILED``-with-comment, none ``PENDING``). Dropping is always allowed and bypasses the gate. On success, appends a new history entry for the destination state — seeded with *its* responsibilities (``PENDING``) — and recomputes - the turn. + the turn. ``repo`` is passed through to :meth:`responsibilities` for the new entry. """ if self.is_terminal(task.state): raise IllegalTransition(f"task {task.id!r} is terminal ({task.state!r})") @@ -556,7 +566,7 @@ def apply_transition( raise ResponsibilitiesNotMet( f"{self.name!r}: responsibility {outstanding[0].key!r} is not resolved" ) - return self._enter(task, to_state, at=at, trigger=trigger, note=note) + return self._enter(task, to_state, at=at, trigger=trigger, note=note, repo=repo) def force_transition( self, @@ -566,6 +576,7 @@ def force_transition( at: str, trigger: str | None = None, note: str | None = None, + repo: Repo | None = None, ) -> Task: """Set the task to *any* state directly — the user's authority to move freely. @@ -575,10 +586,17 @@ def force_transition( that the target state exists. Same history/turn bookkeeping as a normal transition. """ self._state_class(to_state) # validate the target exists - return self._enter(task, to_state, at=at, trigger=trigger, note=note) + return self._enter(task, to_state, at=at, trigger=trigger, note=note, repo=repo) def _enter( - self, task: Task, to_state: str, *, at: str, trigger: str | None, note: str | None + self, + task: Task, + to_state: str, + *, + at: str, + trigger: str | None, + note: str | None, + repo: Repo | None = None, ) -> Task: """Append the entry for ``to_state`` (seeding its promises) and recompute state + turn.""" task.history.append( @@ -588,7 +606,7 @@ def _enter( to_state=to_state, trigger=trigger, note=note, - responsibilities=self._promised(to_state), + responsibilities=self._promised(to_state, repo=repo), ) ) task.state = to_state diff --git a/src/panopticon/taskservice/service.py b/src/panopticon/taskservice/service.py index bd9bf023..776d6185 100644 --- a/src/panopticon/taskservice/service.py +++ b/src/panopticon/taskservice/service.py @@ -323,7 +323,9 @@ async def create_task( if not self._workflow_visible(wf, repo): raise NotAuthorized(f"workflow {workflow_name!r} is not enabled for repo {repo_id!r}") now = self._clock() - task = wf.start_task(self._id(), repo_id, at=now, memo=memo, initial_prompt=initial_prompt) + task = wf.start_task( + self._id(), repo_id, at=now, memo=memo, initial_prompt=initial_prompt, repo=repo + ) task.governor_task_id = governor_task_id task.created_at = now task.updated_at = now # creation time = first mutation @@ -505,10 +507,15 @@ async def _commit_transition( ) -> Task: from_state = task.state _log.info("task %s: %s → %s (trigger=%s)", task.id, from_state, to_state, trigger) + repo = await self.get_repo(task.repo_id) if force: - wf.force_transition(task, to_state, at=self._clock(), trigger=trigger, note=note) + wf.force_transition( + task, to_state, at=self._clock(), trigger=trigger, note=note, repo=repo + ) else: - wf.apply_transition(task, to_state, at=self._clock(), trigger=trigger, note=note) + wf.apply_transition( + task, to_state, at=self._clock(), trigger=trigger, note=note, repo=repo + ) # Deterministic lifecycle hook (e.g. seed the plan on plan acceptance) — may touch the # task/artifacts; run before the single save so any task mutation persists with it. await wf.on_transition( diff --git a/src/panopticon/workflows/github_forge.py b/src/panopticon/workflows/github_forge.py index d78cfafc..08c4abc3 100644 --- a/src/panopticon/workflows/github_forge.py +++ b/src/panopticon/workflows/github_forge.py @@ -19,12 +19,16 @@ from __future__ import annotations -from collections.abc import Sequence +from collections.abc import Iterator, Sequence from typing import ClassVar -from panopticon.core.models import Responsibility, Skill, Tool +from panopticon.core.models import Repo, Responsibility, Skill, Tool from panopticon.workflows.planned_workflow import PlannedWorkflow +#: The `Repo.capabilities` key a repo sets to opt into the tarot review-artifact responsibility +#: (see `TAROT_REVIEW_ARTIFACTS`) — the same "per-repo opt-in map" mechanism as `docker_in_docker`. +TAROT_REVIEW_CAPABILITY = "tarot_review" + class GithubForgeWorkflow(PlannedWorkflow): """Abstract base for GitHub-forge workflows: shared `gh` tool, image layer, and forge @@ -40,6 +44,32 @@ class GithubForgeWorkflow(PlannedWorkflow): description="The PR URL is recorded on the task with the `set_url` MCP tool.", ) + #: ITERATING responsibility added only for repos that opt in (`capabilities.tarot_review`, + #: see :meth:`responsibilities`). Resolution of this key is owned by the in-container + #: `tarot_gate` PreToolUse hook, not the agent's own `resolve_responsibility` call — it runs + #: the real `tarot strands check` / `tarot tour check` and blocks `advance` on failure, so the + #: description tells the agent to do the work, not to self-attest it. + TAROT_REVIEW_ARTIFACTS: ClassVar[Responsibility] = Responsibility( + key="tarot-review-artifacts", + description=( + "For non-trivial changes: author `.tarot/strands.json` (seed via `tarot strands " + "suggest`, edit, pass `tarot strands check`) and a tour (`tarot tour scaffold " + "--from-strands`, fill the narrative, pass `tarot tour check`), committed on the " + "branch. Verified automatically when you advance — trivial diffs are skipped." + ), + ) + + def responsibilities(self, label: str, *, repo: Repo | None = None) -> Iterator[Responsibility]: + """ITERATING gains `TAROT_REVIEW_ARTIFACTS` for a repo that opts in (see + `TAROT_REVIEW_CAPABILITY`); every other state, and a non-opted-in repo, is unchanged.""" + yield from super().responsibilities(label, repo=repo) + if ( + label == "ITERATING" + and repo is not None + and repo.capabilities.get(TAROT_REVIEW_CAPABILITY) + ): + yield self.TAROT_REVIEW_ARTIFACTS + def tools(self) -> Sequence[Tool]: """`gh` is in the image (see `image_layer`); name it so the agent reaches for it.""" return ( diff --git a/tests/container/test_hooks.py b/tests/container/test_hooks.py index ae345f31..8d9ef243 100644 --- a/tests/container/test_hooks.py +++ b/tests/container/test_hooks.py @@ -29,6 +29,16 @@ def test_settings_flip_to_user_while_asking_a_question_and_back_when_answered() assert post["hooks"][0]["command"].endswith("hook agent") +def test_settings_wires_the_tarot_gate_on_apply_operation() -> None: + # Registered unconditionally (like the turn-flip hooks) — the gate script itself decides + # relevance per task/repo at runtime (see tests/container/test_tarot_gate.py). + entries = settings()["hooks"]["PreToolUse"] + tarot = next(e for e in entries if e.get("matcher") == "mcp__panopticon__apply_operation") + assert tarot["hooks"][0]["command"] == "python -m panopticon.container.tarot_gate" + # AskUserQuestion's turn-flip entry is still present alongside it. + assert any(e.get("matcher") == "AskUserQuestion" for e in entries) + + def test_settings_pre_accept_bypass_permissions_mode() -> None: # Without this, unattended claude (--dangerously-skip-permissions) hangs on the first-run # "Bypass Permissions mode" acceptance prompt — the task shows "stuck starting". diff --git a/tests/container/test_tarot_gate.py b/tests/container/test_tarot_gate.py new file mode 100644 index 00000000..99f97117 --- /dev/null +++ b/tests/container/test_tarot_gate.py @@ -0,0 +1,265 @@ +"""The tarot review-artifact gate: a `PreToolUse` hook on `apply_operation` (advance). + +Pins every branch: irrelevant calls (wrong operation, wrong state, non-opted-in repo) allow +immediately with no side effects; a trivial diff auto-resolves the responsibility without +running the tarot CLIs; a non-trivial diff runs them and either auto-resolves (both pass) or +denies with the captured output (either fails); a missing `tarot` binary denies with an +operator-facing message. Deterministic and LLM-free — a fake client and a fake command runner, +no real `tarot`/`git` needed (the same style as :mod:`tests.container.test_hooks`). +""" + +from __future__ import annotations + +import io +import json + +from panopticon.client import JsonObj +from panopticon.container import tarot_gate +from panopticon.container.tarot_gate import CommandResult + +WORKSPACE = "/workspace" + + +class _FakeClient: + def __init__(self, task: JsonObj, repo: JsonObj) -> None: + self._task = task + self._repo = repo + self.resolved: list[tuple[str, str, object, str | None]] = [] + + def get_task(self, task_id: str) -> JsonObj: + assert task_id == self._task["id"] + return self._task + + def get_repo(self, repo_id: str) -> JsonObj: + assert repo_id == self._repo["id"] + return self._repo + + def resolve_responsibility( + self, task_id: str, key: str, status: object, comment: str | None = None + ) -> JsonObj: + self.resolved.append((task_id, key, status, comment)) + return {} + + +class _FakeRun: + """Records every command it's asked to run; returns a canned result per exact argv.""" + + def __init__(self, responses: dict[tuple[str, ...], CommandResult] | None = None) -> None: + self._responses = responses or {} + self.calls: list[tuple[tuple[str, ...], str | None]] = [] + + def __call__(self, args: list[str], *, cwd: str | None = None) -> CommandResult: + key = tuple(args) + self.calls.append((key, cwd)) + return self._responses.get(key, CommandResult(returncode=0, output="")) + + +def _task(state: str = "ITERATING") -> JsonObj: + return {"id": "t1", "state": state, "repo_id": "r1"} + + +def _repo(*, opted_in: bool, threshold: int | None = None, default_base: str = "main") -> JsonObj: + capabilities: dict[str, object] = {"tarot_review": opted_in} + if threshold is not None: + capabilities["tarot_review_threshold"] = threshold + return {"id": "r1", "default_base": default_base, "capabilities": capabilities} + + +def _numstat_key(base_ref: str) -> tuple[str, ...]: + return ("git", "-C", WORKSPACE, "diff", "--numstat", f"{base_ref}...HEAD") + + +def _payload(operation: str | None) -> str: + tool_input: JsonObj = {} if operation is None else {"operation": operation} + return json.dumps({"tool_name": "mcp__panopticon__apply_operation", "tool_input": tool_input}) + + +def _run_gate( + *, client: _FakeClient, run: _FakeRun, operation: str = "advance", env: dict[str, str] +) -> tuple[int, str]: + import os + + old = dict(os.environ) + os.environ.update(env) + try: + stdin = io.StringIO(_payload(operation)) + code = tarot_gate.main(client=client, stdin=stdin, run=run) # type: ignore[arg-type] + finally: + os.environ.clear() + os.environ.update(old) + return code, "" + + +ENV = {"PANOPTICON_TASK_ID": "t1", "PANOPTICON_SERVICE_URL": "http://svc"} + + +def _stdout(capsys) -> str: # type: ignore[no-untyped-def] + return capsys.readouterr().out + + +# -- irrelevant calls allow immediately, no side effects ----------------------------- + + +def test_non_advance_operation_allows_with_no_client_calls() -> None: + stdin = io.StringIO(_payload("drop")) + # No env vars set at all — a non-advance operation never even reads them. + assert tarot_gate.main(stdin=stdin, run=_FakeRun()) == 0 # type: ignore[arg-type] + + +def test_missing_operation_field_allows() -> None: + stdin = io.StringIO(_payload(None)) + assert tarot_gate.main(stdin=stdin, run=_FakeRun()) == 0 # type: ignore[arg-type] + + +def test_non_iterating_state_allows(capsys) -> None: # type: ignore[no-untyped-def] + client = _FakeClient(_task(state="PLANNING"), _repo(opted_in=True)) + run = _FakeRun() + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + assert _stdout(capsys) == "" + assert run.calls == [] # never even checked the diff + assert client.resolved == [] + + +def test_non_opted_in_repo_allows(capsys) -> None: # type: ignore[no-untyped-def] + client = _FakeClient(_task(), _repo(opted_in=False)) + run = _FakeRun() + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + assert _stdout(capsys) == "" + assert run.calls == [] + assert client.resolved == [] + + +# -- trivial diff: auto-resolve without running the tarot CLIs ---------------------- + + +def test_trivial_diff_auto_resolves_and_skips_tarot_checks(capsys) -> None: # type: ignore[no-untyped-def] + repo = _repo(opted_in=True) + client = _FakeClient(_task(), repo) + run = _FakeRun( + {_numstat_key("origin/main"): CommandResult(returncode=0, output="3\t2\tfoo.py\n")} + ) + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + assert _stdout(capsys) == "" + assert client.resolved == [ + ( + "t1", + "tarot-review-artifacts", + tarot_gate.Status.MET, + "trivial diff — tarot review skipped", + ) + ] + assert ("tarot", "strands", "check") not in {c[0] for c in run.calls} + + +def test_trivial_diff_threshold_is_overridable_per_repo(capsys) -> None: # type: ignore[no-untyped-def] + # 25 changed lines is above the default (20) but below a repo override of 30. + repo = _repo(opted_in=True, threshold=30) + client = _FakeClient(_task(), repo) + run = _FakeRun( + {_numstat_key("origin/main"): CommandResult(returncode=0, output="20\t5\tfoo.py\n")} + ) + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + assert client.resolved and client.resolved[0][3] == "trivial diff — tarot review skipped" + + +# -- non-trivial diff: run the tarot CLIs -------------------------------------------- + + +def _big_diff() -> CommandResult: + return CommandResult(returncode=0, output="500\t20\tbig.py\n") + + +def test_non_trivial_diff_both_checks_pass_auto_resolves(capsys) -> None: # type: ignore[no-untyped-def] + repo = _repo(opted_in=True) + client = _FakeClient(_task(), repo) + run = _FakeRun( + { + _numstat_key("origin/main"): _big_diff(), + ("tarot", "strands", "check"): CommandResult(returncode=0, output="ok"), + ("tarot", "tour", "check"): CommandResult(returncode=0, output="ok"), + } + ) + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + assert _stdout(capsys) == "" + assert client.resolved == [ + ( + "t1", + "tarot-review-artifacts", + tarot_gate.Status.MET, + "verified by tarot strands check / tarot tour check", + ) + ] + + +def test_strands_check_failure_denies_with_output_and_leaves_pending(capsys) -> None: # type: ignore[no-untyped-def] + repo = _repo(opted_in=True) + client = _FakeClient(_task(), repo) + run = _FakeRun( + { + _numstat_key("origin/main"): _big_diff(), + ("tarot", "strands", "check"): CommandResult( + returncode=1, output="strands.json missing a strand for foo()" + ), + } + ) + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 # denial is exit 0 + JSON, per the PreToolUse contract + out = json.loads(_stdout(capsys)) + reason = out["hookSpecificOutput"]["permissionDecisionReason"] + assert "strands.json missing a strand for foo()" in reason + assert out["hookSpecificOutput"]["permissionDecision"] == "deny" + assert client.resolved == [] # left PENDING — never optimistically resolved + # tour check never ran — strands already failed + assert ("tarot", "tour", "check") not in {c[0] for c in run.calls} + + +def test_tour_check_failure_denies_with_its_output(capsys) -> None: # type: ignore[no-untyped-def] + repo = _repo(opted_in=True) + client = _FakeClient(_task(), repo) + run = _FakeRun( + { + _numstat_key("origin/main"): _big_diff(), + ("tarot", "strands", "check"): CommandResult(returncode=0, output="ok"), + ("tarot", "tour", "check"): CommandResult(returncode=1, output="tour missing a stop"), + } + ) + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + out = json.loads(_stdout(capsys)) + assert "tour missing a stop" in out["hookSpecificOutput"]["permissionDecisionReason"] + assert client.resolved == [] + + +def test_missing_tarot_binary_denies_with_operator_message(capsys) -> None: # type: ignore[no-untyped-def] + repo = _repo(opted_in=True) + client = _FakeClient(_task(), repo) + run = _FakeRun( + { + _numstat_key("origin/main"): _big_diff(), + ("tarot", "strands", "check"): CommandResult( + returncode=127, output="tarot: command not found", found=False + ), + } + ) + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + out = json.loads(_stdout(capsys)) + reason = out["hookSpecificOutput"]["permissionDecisionReason"] + assert "not installed" in reason and "image_layer_file" in reason + assert client.resolved == [] + + +def test_default_base_branch_is_used_for_the_diff_base() -> None: + repo = _repo(opted_in=True, default_base="develop") + client = _FakeClient(_task(), repo) + run = _FakeRun( + {_numstat_key("origin/develop"): CommandResult(returncode=0, output="1\t1\tx\n")} + ) + code, _ = _run_gate(client=client, run=run, env=ENV) + assert code == 0 + assert client.resolved # matched the develop-based numstat key, so it ran and resolved diff --git a/tests/core/test_workflow.py b/tests/core/test_workflow.py index f904271a..6c494a2b 100644 --- a/tests/core/test_workflow.py +++ b/tests/core/test_workflow.py @@ -18,6 +18,7 @@ IllegalTransition, InitialState, InvalidWorkflow, + Repo, ResponsibilitiesNotMet, Responsibility, State, @@ -256,6 +257,88 @@ def test_ungated_state_needs_no_responsibilities() -> None: assert task.state == "WORKING" +# -- responsibilities can vary by repo (optional context) --------------------------- + + +class RepoAwareWorkflow(Workflow): + """A workflow that adds an extra WORKING responsibility for a repo opted into "extra". + + A fresh `Workflow` (not a `GatedWorkflow` subclass): nested state classes are discovered via + `vars(type(self))` (not inherited-attribute lookup), so a subclass reusing a base's *states* + without redeclaring them would never resolve its own graph — this mirrors `GatedWorkflow`'s + shape directly instead. + """ + + name = "repo-aware-test" + + EXTRA: Responsibility = Responsibility(key="extra-check", description="An extra check") + + class Plan(InitialState): + label = "PLAN" + transitions = ("WORKING",) + + class Working(State): + label = "WORKING" + advanced_by = Actor.AGENT + responsibilities = ( + Responsibility(key="tests-pass", description="Tests pass"), + Responsibility(key="pr-opened", description="PR opened"), + ) + transitions = (Complete,) + + initial = Plan + + def responsibilities(self, label, *, repo=None): # type: ignore[no-untyped-def] + yield from super().responsibilities(label, repo=repo) + if label == "WORKING" and repo is not None and repo.capabilities.get("extra"): + yield self.EXTRA + + +def _repo(**capabilities: bool) -> Repo: + return Repo(id="r1", name="r1", git_url="git@example.com:r1.git", capabilities=capabilities) + + +def test_responsibilities_default_repo_is_none_and_inert() -> None: + # The base Workflow.responsibilities ignores `repo` entirely — passing one changes nothing + # unless a subclass overrides responsibilities() to look at it (see RepoAwareWorkflow below). + assert {r.key for r in WF.responsibilities("WORKING")} == {"tests-pass", "pr-opened"} + assert {r.key for r in WF.responsibilities("WORKING", repo=_repo(extra=True))} == { + "tests-pass", + "pr-opened", + } + + +def test_responsibilities_adds_the_extra_one_only_for_an_opted_in_repo() -> None: + rwf = RepoAwareWorkflow() + assert "extra-check" not in {r.key for r in rwf.responsibilities("WORKING")} + assert "extra-check" not in {r.key for r in rwf.responsibilities("WORKING", repo=_repo())} + assert "extra-check" in {r.key for r in rwf.responsibilities("WORKING", repo=_repo(extra=True))} + # unaffected states (e.g. PLAN) are unchanged regardless of repo + assert list(rwf.responsibilities("PLAN", repo=_repo(extra=True))) == [] + + +def test_apply_transition_seeds_repo_conditional_responsibility() -> None: + rwf = RepoAwareWorkflow() + task = rwf.start_task("t1", "r1", at="t0") + rwf.apply_transition(task, "WORKING", at="t1", repo=_repo(extra=True)) + assert {r.key for r in task.history[-1].responsibilities} == { + "tests-pass", + "pr-opened", + "extra-check", + } + # a non-opted-in repo doesn't get it + other = rwf.start_task("t2", "r1", at="t0") + rwf.apply_transition(other, "WORKING", at="t1", repo=_repo()) + assert {r.key for r in other.history[-1].responsibilities} == {"tests-pass", "pr-opened"} + + +def test_force_transition_seeds_repo_conditional_responsibility() -> None: + rwf = RepoAwareWorkflow() + task = rwf.start_task("t1", "r1", at="t0") + rwf.force_transition(task, "WORKING", at="t1", repo=_repo(extra=True)) + assert "extra-check" in {r.key for r in task.history[-1].responsibilities} + + # -- history ------------------------------------------------------------------------ diff --git a/tests/workflows/test_github_forge.py b/tests/workflows/test_github_forge.py new file mode 100644 index 00000000..443f9046 --- /dev/null +++ b/tests/workflows/test_github_forge.py @@ -0,0 +1,83 @@ +"""The tarot review-artifact responsibility shared by the GitHub-forge workflows. + +`GithubForgeWorkflow.responsibilities()` conditionally adds `TAROT_REVIEW_ARTIFACTS` to +ITERATING for a repo that opts in (`Repo.capabilities["tarot_review"]`) — verified here on both +concrete subclasses (`GithubPeerReviewed`, `GithubSelfReviewed`) since the override lives on the +shared base. The real verification (running `tarot strands check` / `tarot tour check` and +blocking `advance` on failure) is the in-container `tarot_gate` hook +(`tests/container/test_tarot_gate.py`), not this responsibility-declaration layer. +""" + +from __future__ import annotations + +from panopticon.core.models import Repo, Status +from panopticon.workflows import GithubPeerReviewed, GithubSelfReviewed +from panopticon.workflows.github_forge import TAROT_REVIEW_CAPABILITY, GithubForgeWorkflow + + +def _repo(**capabilities: object) -> Repo: + return Repo(id="r1", name="r1", git_url="git@example.com:r1.git", capabilities=capabilities) + + +def test_tarot_responsibility_absent_with_no_repo() -> None: + wf = GithubPeerReviewed() + assert "tarot-review-artifacts" not in {r.key for r in wf.responsibilities("ITERATING")} + + +def test_tarot_responsibility_absent_for_a_non_opted_in_repo() -> None: + wf = GithubPeerReviewed() + keys = {r.key for r in wf.responsibilities("ITERATING", repo=_repo())} + assert "tarot-review-artifacts" not in keys + + +def test_tarot_responsibility_present_for_an_opted_in_repo() -> None: + wf = GithubPeerReviewed() + repo = _repo(**{TAROT_REVIEW_CAPABILITY: True}) + responsibilities = list(wf.responsibilities("ITERATING", repo=repo)) + assert "tarot-review-artifacts" in {r.key for r in responsibilities} + tarot = next(r for r in responsibilities if r.key == "tarot-review-artifacts") + assert "tarot strands check" in tarot.description + assert "tarot tour check" in tarot.description + # every other ITERATING responsibility is untouched + assert {"plan-implemented", "tests-pass", "url-recorded"} <= {r.key for r in responsibilities} + + +def test_tarot_responsibility_only_applies_to_iterating() -> None: + wf = GithubPeerReviewed() + repo = _repo(**{TAROT_REVIEW_CAPABILITY: True}) + assert "tarot-review-artifacts" not in { + r.key for r in wf.responsibilities("PLANNING", repo=repo) + } + assert "tarot-review-artifacts" not in {r.key for r in wf.responsibilities("REVIEW", repo=repo)} + assert "tarot-review-artifacts" not in { + r.key for r in wf.responsibilities("MERGING", repo=repo) + } + + +def test_shared_across_both_forge_workflows() -> None: + repo = _repo(**{TAROT_REVIEW_CAPABILITY: True}) + for wf in (GithubPeerReviewed(), GithubSelfReviewed()): + assert "tarot-review-artifacts" in { + r.key for r in wf.responsibilities("ITERATING", repo=repo) + } + + +def test_seeded_through_a_real_transition_only_when_opted_in() -> None: + wf = GithubPeerReviewed() + task = wf.start_task("t1", "r1", at="t0") + for r in list(task.outstanding_responsibilities): + task.resolve_responsibility(key=r.key, status=Status.MET) + wf.apply_transition(task, "ITERATING", at="t1", repo=_repo(**{TAROT_REVIEW_CAPABILITY: True})) + assert "tarot-review-artifacts" in {r.key for r in task.current_entry.responsibilities} + + other = wf.start_task("t2", "r1", at="t0") + for r in list(other.outstanding_responsibilities): + other.resolve_responsibility(key=r.key, status=Status.MET) + wf.apply_transition(other, "ITERATING", at="t1", repo=_repo()) + assert "tarot-review-artifacts" not in {r.key for r in other.current_entry.responsibilities} + + +def test_tarot_review_capability_constant_matches_docker_in_docker_style() -> None: + # Same "per-repo opt-in map" mechanism as `docker_in_docker` — a plain capabilities key. + assert TAROT_REVIEW_CAPABILITY == "tarot_review" + assert GithubForgeWorkflow.TAROT_REVIEW_ARTIFACTS.key == "tarot-review-artifacts"