From 675a4385dd2f424319656992a54e69868dd0d8c1 Mon Sep 17 00:00:00 2001 From: Alejandro Gonzalez Date: Sat, 19 Sep 2026 21:43:16 +0200 Subject: [PATCH] feat(policy-checks): evaluate otari hook policies locally by default otari hook now evaluates .otari-gates.yml in process by default, needing no running otari serve and no credential: the parse-and-evaluate logic that used to live inline in routes/hooks.py is now a shared, pure function (agent_runtime.domain.check.run_policy_check) that both the Hook Server route and the CLI call. POST /api/v1/hooks/check stays as an opt-in path via --url/--api-key (or OTARI_URL/OTARI_API_KEY), for a caller that wants a shared or hosted gateway to decide instead. otari hook setup no longer resolves or prompts for a credential by default; it only embeds one when --api-key is given explicitly. Regenerates docs/public/openapi.json and the Postman collection for the check_policy route's updated docstring. Co-Authored-By: Claude Sonnet 5 --- docs/agent-gates.md | 165 ++++++---- docs/index.md | 3 +- docs/public/openapi.json | 2 +- docs/public/otari.postman_collection.json | 2 +- src/gateway/AGENTS.md | 13 +- src/gateway/agent_runtime/domain/check.py | 269 ++++++++++++++++ src/gateway/api/routes/hooks.py | 365 ++++------------------ src/gateway/cli.py | 343 +++++++++++--------- tests/integration/test_hooks_route.py | 2 +- tests/unit/agent_runtime/test_check.py | 108 +++++++ tests/unit/test_hook_cli.py | 162 +++++++++- tests/unit/test_hook_setup_cli.py | 29 +- web/src/client/schema.ts | 6 + 13 files changed, 919 insertions(+), 550 deletions(-) create mode 100644 src/gateway/agent_runtime/domain/check.py create mode 100644 tests/unit/agent_runtime/test_check.py diff --git a/docs/agent-gates.md b/docs/agent-gates.md index 3723676185..a5787c2462 100644 --- a/docs/agent-gates.md +++ b/docs/agent-gates.md @@ -7,10 +7,15 @@ repository itself. Rules live in `.otari-gates.yml`, committed alongside the code they check, so they survive an agent swap and a clone the same way the rest of the repo does. -This is core Otari, evaluated by `otari serve`'s Hook Server, not a separate -package or plugin. It is not [Guardrails](guardrails.md), which checks -request input/output at inference time; a gate checks agent actions and repo -diffs. +This is core Otari, not a separate package or plugin. It is not +[Guardrails](guardrails.md), which checks request input/output at inference +time; a gate checks agent actions and repo diffs. It also needs no running +`otari serve`: the evaluator (`gateway.agent_runtime.domain.check`) is pure +Python with no filesystem, network, subprocess, database, or clock access, so +`otari hook` evaluates it in process by default, and `otari serve`'s own Hook +Server (`POST /api/v1/hooks/check`) calls the exact same function. Pointing +`otari hook` at a gateway over HTTP is an opt-in, not a requirement; see +"Calling the Hook Server" below for why you might still want to. ## Status @@ -18,10 +23,14 @@ This is the first slice. It ships: - Five gate types: `changed_path`, `command_match`, `command_if_changed`, `judge`, and `check_passed`. -- `POST /api/v1/hooks/check`, evaluated against evidence the caller submits. - `otari hook --harness claude-code` and `otari hook --harness codex`, real - installed commands that read a Claude Code or Codex hook payload and call - the endpoint above. + installed commands that read a Claude Code or Codex hook payload, collect + the evidence it implies, and evaluate the repo's own `.otari-gates.yml` + against it in process, no server or credential required. Given `--url` + and/or `--api-key` (or their `OTARI_URL`/`OTARI_API_KEY` envvars), they + instead call `POST /api/v1/hooks/check` on a gateway over HTTP. +- `POST /api/v1/hooks/check`, the same evaluation exposed over HTTP by + `otari serve`, for whoever opts a hook into it. - `otari hook setup`, which registers it: writes a `PreToolUse` and a `Stop` hook entry into the harness's own settings (`.claude/settings.local.json` for Claude Code, `.codex/hooks.json` for Codex) and, if this repo has no @@ -580,6 +589,13 @@ problems" trade a diff-scoped linter already makes. ## Calling the Hook Server +`otari hook` does not need this by default: it evaluates the local +`.otari-gates.yml` in process (see "Status" above), and reaches this endpoint +only when it is given `--url` and/or `--api-key` (or their `OTARI_URL`/ +`OTARI_API_KEY` envvars). Opting into it is for whoever wants a shared or +hosted gateway, rather than the machine the agent is running on, to be the +one deciding, or a central place a future check could report to. + `POST /api/v1/hooks/check`, authenticated like `POST /api/v1/usage/external-events`: an ordinary API key or the master key in the `Otari-Key` header. This identifies who sent the request, not whether @@ -656,9 +672,11 @@ HTTP status codes: | `401`/`403` | Missing or invalid API key / master key. | | `422` | `policy_yaml` is missing, malformed, or references an unsupported schema version or gate type. | -`otari serve` must be running: there is no offline or local-only path in this -build. A gate is only as useful as the evidence it is checked against, and -evaluating that evidence is what this endpoint does. +`otari serve` must be running for this endpoint specifically, since it is +what serves it; `otari hook` itself does not need it, by default (see +"Calling the Hook Server" above). A gate is only as useful as the evidence it +is checked against, and evaluating that evidence, wherever it runs, is the +one job both this endpoint and `otari hook`'s own local evaluation share. ## Trying it against a real Claude Code session @@ -778,9 +796,8 @@ blocking proves nothing about whether an interactive session's own 1. Run `otari hook setup`. It writes both a `PreToolUse` hook entry and a `Stop` hook entry into `.claude/settings.local.json` (personal, and this - repo's own `.gitignore` covers it specifically, since the generated - command embeds a live API key or master key: see "Registering it for - Codex" below for the same file for that harness), both pointing at this + repo's own `.gitignore` covers it specifically: see below for why it + still matters with no credential involved), both pointing at this install's own `otari hook --harness claude-code`; Claude Code passes its own `hook_event_name` in the payload, so one callback serves both. If this @@ -796,34 +813,37 @@ blocking proves nothing about whether an interactive session's own since `changed_path` always benefits from its Git-status fallback there and a `command_if_changed` gate has no other event it can resolve on. - For a credential, it tries the same automatic resolution `otari hook` - itself does at runtime (config file, `.env`, environment) before asking; - if that finds nothing, it prompts once and writes the answer into the - generated command rather than into `.env`. Re-running `otari hook setup` - updates both entries in place rather than adding duplicates, and leaves - every other hook or permission already in the file untouched. - - Safe to run again any time the policy or the credential changes. + With no `--api-key`, the generated command carries no credential at all: + `otari hook` evaluates the policy locally, and needs neither one nor a + running gateway to do it. `.claude/settings.local.json` stays gitignored + regardless, since it is still a personal file (its own registration + should not be everyone's default, and it may later carry `--api-key`). + Re-running `otari hook setup` updates both entries in place rather than + adding duplicates, and leaves every other hook or permission already in + the file untouched. Safe to run again any time the policy changes. `--harness codex` runs the same setup against Codex's own settings instead - (see "Registering it for Codex" below); `--api-key ` skips resolution - and prompting outright, for a non-interactive run. + (see "Registering it for Codex" below); `--api-key ` is the opt-in + into checking against a gateway over HTTP instead (see "Calling the Hook + Server" above) and embeds that credential in the generated command. Three things about this are temporary, not deliberate design, and all trace back to one cause: this package installs into a per-project venv today, not a single, stable, per-user location. `setup` identifies its own hook entry by the absolute path of that venv's `otari` binary, so a reinstalled or relocated environment leaves the old entry unrecognized - rather than updated in place. It has no `--config`/`-c` of its own (unlike - `otari hook` itself, see "Registering it by hand" below), so a - `master_key` living only in `config.yml` is neither found automatically - nor passed to the generated hook. And a manually entered credential is - embedded directly in the generated command: a subprocess argument visible - to anything that lists processes on the machine, not just a value in a - gitignored file. Once otari ships as a standalone install (Homebrew, most - likely) instead of a venv console script, it gains a fixed binary path to - match on and a well-known per-user config directory (`~/.config/otari` on - both macOS and Linux, not the platform-native convention) to read a - `master_key` from and write a prompted one into, closing all three without + rather than updated in place; this one applies regardless of `--api-key`. + The other two are specific to opting into the HTTP-backed mode: `setup` + has no `--config`/`-c` of its own (unlike `otari hook` itself, see + "Registering it by hand" below), so a `master_key` living only in + `config.yml` is neither found automatically nor passed to the generated + hook when opting in without `--api-key`; and a given `--api-key` is + embedded directly in the generated command, a subprocess argument + visible to anything that lists processes on the machine, not just a + value in a gitignored file. Once otari ships as a standalone install + (Homebrew, most likely) instead of a venv console script, it gains a + fixed binary path to match on and a well-known per-user config directory + (`~/.config/otari` on both macOS and Linux, not the platform-native + convention) to read a `master_key` from, closing all three without threading a flag through every entry point or ever putting a secret in argv. Fixed then, not now. @@ -850,7 +870,7 @@ explicitly, one for `PreToolUse` and one for `Stop`: "hooks": [ { "type": "command", - "command": "/abs/path/to/.venv/bin/otari hook --harness claude-code -c /abs/path/to/config.yml" + "command": "/abs/path/to/.venv/bin/otari hook --harness claude-code" } ] } @@ -860,7 +880,7 @@ explicitly, one for `PreToolUse` and one for `Stop`: "hooks": [ { "type": "command", - "command": "/abs/path/to/.venv/bin/otari hook --harness claude-code -c /abs/path/to/config.yml" + "command": "/abs/path/to/.venv/bin/otari hook --harness claude-code" } ] } @@ -869,44 +889,49 @@ explicitly, one for `PreToolUse` and one for `Stop`: } ``` -Both paths are absolute on purpose. The hook subprocess does not inherit an -activated shell's `PATH`, so a bare `otari` often will not resolve, and it -does not reliably start in the directory the credential lookup below reads -from either. +The path is absolute on purpose. The hook subprocess does not inherit an +activated shell's `PATH`, so a bare `otari` often will not resolve. -With no `--url`/`--api-key`, `otari hook` resolves both the same way every -other Otari command does, reading the gateway's own `host`/`port` and its -`master_key`. It consults the config file `-c` names, the `.env` beside that -file, the `.env` in the working directory, and the environment, with the -environment taking precedence over the config file. +That is everything the default, local evaluation needs: no `--url`, +`--api-key`, or `-c`, and no `master_key` anywhere. Opting into checking +against a gateway over HTTP instead (see "Calling the Hook Server" above) +adds `--url`/`--api-key` (or their `OTARI_URL`/`OTARI_API_KEY` envvars) to +the command, and, only in that mode, `otari hook` resolves whichever of the +two is not given the same way every other Otari command does, reading the +gateway's own `host`/`port` and its `master_key`. It consults the config +file `-c` names, the `.env` beside that file, the `.env` in the working +directory, and the environment, with the environment taking precedence over +the config file. Nothing auto-discovers a `config.yml`. Without `-c`, a `config.yml` sitting in the working directory is not read, exactly as for `otari serve`. Pass `-c` whenever your `master_key` lives in `config.yml` rather than in `.env` -or the environment, or the hook finds no credential. - -Getting that wrong is quiet. A hook with no credential does what it does for -any setup failure: prints to stderr and exits 0, and Claude Code shows a -non-blocking hook's stderr only in its own debug log. The gates stop running -and nothing in the transcript says so. After setting this up, confirm it -works by editing a forbidden path (step 2 above) rather than by seeing no -complaints. - -All of this only applies when the hook runs on the same machine as the -server. Otherwise point it at the gateway with `--url`/`--api-key`, or -`OTARI_URL`/`OTARI_API_KEY`. Prefer an ordinary API key over the `master_key` -there: this endpoint accepts either, a hook needs nothing the master key -uniquely grants, and a credential written into a settings file or a command -line is one you should be able to rotate on its own. +or the environment and you have opted into this mode without `--api-key`, or +the hook finds no credential. + +Getting that wrong is quiet. Once opted in, a hook with no credential does +what it does for any setup failure: prints to stderr and exits 0, and Claude +Code shows a non-blocking hook's stderr only in its own debug log. The gates +stop running and nothing in the transcript says so. After setting this up, +confirm it works by editing a forbidden path (step 2 above) rather than by +seeing no complaints. + +All of the credential-resolution paragraph above only applies when the hook +runs on the same machine as the server. Otherwise point it at the gateway +with `--url`/`--api-key`, or `OTARI_URL`/`OTARI_API_KEY`. Prefer an ordinary +API key over the `master_key` there: this endpoint accepts either, a hook +needs nothing the master key uniquely grants, and a credential written into +a settings file or a command line is one you should be able to rotate on its +own. ### Registering it for Codex `otari hook setup --harness codex` writes the same pair of hook blocks into `.codex/hooks.json` instead, naming this install's own `otari hook --harness -codex`. Personal, the same as `.claude/settings.local.json` above and for -the same reason: the generated command embeds a live API key or master key, -and this repo's own `.gitignore` covers this exact path specifically so it -never lands in a commit or a PR. +codex`. Personal, the same as `.claude/settings.local.json` above: its own +registration should not be everyone's default, and it may later carry an +opted-in `--api-key`, in which case this repo's own `.gitignore` covers this +exact path specifically so that credential never lands in a commit or a PR. ```json { @@ -914,11 +939,11 @@ never lands in a commit or a PR. "PreToolUse": [ { "matcher": "apply_patch|Bash|exec|code_mode_exec", - "hooks": [{"type": "command", "command": "/abs/path/to/.venv/bin/otari hook --harness codex -c /abs/path/to/config.yml"}] + "hooks": [{"type": "command", "command": "/abs/path/to/.venv/bin/otari hook --harness codex"}] } ], "Stop": [ - {"hooks": [{"type": "command", "command": "/abs/path/to/.venv/bin/otari hook --harness codex -c /abs/path/to/config.yml"}]} + {"hooks": [{"type": "command", "command": "/abs/path/to/.venv/bin/otari hook --harness codex"}]} ] } } @@ -947,9 +972,11 @@ neither depends on `PreToolUse` firing. ### Known gaps -`otari hook` is a thin, harness-specific transport, not a second copy of the -evaluator: it collects evidence and calls the endpoint above; every actual -decision still comes from `gateway.agent_runtime`. What neither command does +`otari hook` is a thin, harness-specific evidence collector, not a second +copy of the evaluator: it collects evidence and hands it to +`agent_runtime.domain.check.run_policy_check`, the exact same function the +Hook Server route calls; every actual decision comes from +`gateway.agent_runtime`, whichever caller runs it. What neither command does yet: uninstall itself, or probe whether it is correctly registered (`otari status`, not built). diff --git a/docs/index.md b/docs/index.md index 38a604f527..42f1daf42a 100644 --- a/docs/index.md +++ b/docs/index.md @@ -48,7 +48,8 @@ Calling the gateway from your own code. - [Files](files.md): file uploads and document understanding for local models. - [Guardrails](guardrails.md): request-level checks like prompt-injection detection. - [Agent Gates](agent-gates.md): repo-owned policy checks against evidence a coding - agent's session reports, served by Otari's Hook Server at `POST /api/v1/hooks/check`. + agent's session reports. `otari hook` evaluates them locally by default, with + Otari's Hook Server (`POST /api/v1/hooks/check`) as an opt-in. - [Use with Claude Code](use-with-claude-code.md): point the Claude Code CLI at Otari. - [Use with Codex](use-with-codex.md): route the Codex CLI through Otari over the Responses API, or import its usage without routing. - [Use with opencode](use-with-opencode.md): point the opencode CLI at Otari. diff --git a/docs/public/openapi.json b/docs/public/openapi.json index 78160a2dd5..b01a0a018d 100644 --- a/docs/public/openapi.json +++ b/docs/public/openapi.json @@ -21772,7 +21772,7 @@ }, "/api/v1/hooks/check": { "post": { - "description": "Evaluate a submitted policy against submitted evidence.\n\nAuthenticated with either an API key or the master key (the router-level\ngate), like ``POST /api/v1/usage/external-events``: this identifies who\nsent the request, not whether its evidence is true. `blocked` is set when\na required gate's outcome is not `pass`/`not_applicable` (an unresolved\ngate never counts as a pass).", + "description": "Evaluate a submitted policy against submitted evidence.\n\nAuthenticated with either an API key or the master key (the router-level\ngate), like ``POST /api/v1/usage/external-events``: this identifies who\nsent the request, not whether its evidence is true. `blocked` is set when\na required gate's outcome is not `pass`/`not_applicable` (an unresolved\ngate never counts as a pass).\n\nThe actual parse-and-evaluate work is\n``agent_runtime.domain.check.run_policy_check``, shared with ``otari\nhook``'s own local evaluation: this route's own job is authentication,\ntranslating that function's tri-state request fields into its own typed\nones, and turning ``PolicyCheckError`` into a 422.", "operationId": "hooks-check_policy", "requestBody": { "content": { diff --git a/docs/public/otari.postman_collection.json b/docs/public/otari.postman_collection.json index 49fac442b5..614d6eb439 100644 --- a/docs/public/otari.postman_collection.json +++ b/docs/public/otari.postman_collection.json @@ -2035,7 +2035,7 @@ }, "raw": "{\n \"policy_yaml\": \"string\"\n}" }, - "description": "Evaluate a submitted policy against submitted evidence.\n\nAuthenticated with either an API key or the master key (the router-level\ngate), like ``POST /api/v1/usage/external-events``: this identifies who\nsent the request, not whether its evidence is true. `blocked` is set when\na required gate's outcome is not `pass`/`not_applicable` (an unresolved\ngate never counts as a pass).", + "description": "Evaluate a submitted policy against submitted evidence.\n\nAuthenticated with either an API key or the master key (the router-level\ngate), like ``POST /api/v1/usage/external-events``: this identifies who\nsent the request, not whether its evidence is true. `blocked` is set when\na required gate's outcome is not `pass`/`not_applicable` (an unresolved\ngate never counts as a pass).\n\nThe actual parse-and-evaluate work is\n``agent_runtime.domain.check.run_policy_check``, shared with ``otari\nhook``'s own local evaluation: this route's own job is authentication,\ntranslating that function's tri-state request fields into its own typed\nones, and turning ``PolicyCheckError`` into a 422.", "header": [ { "key": "Content-Type", diff --git a/src/gateway/AGENTS.md b/src/gateway/AGENTS.md index 76f08ca3da..232e991f94 100644 --- a/src/gateway/AGENTS.md +++ b/src/gateway/AGENTS.md @@ -292,10 +292,15 @@ scope and not only its filter set. Nothing else narrows it, and ## Agent Gates `agent_runtime/` evaluates a caller-submitted `.otari-gates.yml` policy -against caller-submitted evidence, served by the Hook Server -(`POST /api/v1/hooks/check`, `routes/hooks.py`). Everything under it is pure: -no filesystem, network, subprocess, or clock access. Otari never reads a -caller's repository itself. See [docs/agent-gates.md](../../docs/agent-gates.md). +against caller-submitted evidence. Everything under it is pure: no +filesystem, network, subprocess, or clock access. Otari never reads a +caller's repository itself. `agent_runtime/domain/check.py`'s +`run_policy_check` is the shared orchestration (parse, budget-guard, +dispatch to each gate's evaluator): `otari hook` (`cli.py`) calls it in +process by default, needing no running gateway, and the Hook Server +(`POST /api/v1/hooks/check`, `routes/hooks.py`) calls the same function for +whoever opts a hook into checking against a gateway over HTTP instead. See +[docs/agent-gates.md](../../docs/agent-gates.md). ## Logging diff --git a/src/gateway/agent_runtime/domain/check.py b/src/gateway/agent_runtime/domain/check.py new file mode 100644 index 0000000000..5259844df5 --- /dev/null +++ b/src/gateway/agent_runtime/domain/check.py @@ -0,0 +1,269 @@ +"""Evaluate a submitted policy against submitted evidence. + +The orchestration shared by the Hook Server route (``routes/hooks.py``) and +``otari hook``'s own local evaluation: parse the policy, guard the match-cost +budgets below, dispatch every gate to its evaluator, and fold the results +into one pass/fail verdict. Pure, like every other module in this package: +no filesystem, network, subprocess, or clock access. A caller collects its +own evidence; this only ever computes over what it was given. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass + +from gateway.agent_runtime.domain.evaluators import ( + evaluate_changed_path, + evaluate_check_passed, + evaluate_command_if_changed, + evaluate_command_match, + evaluate_judge, + tokenize_commands, + tokenize_phrases, +) +from gateway.agent_runtime.domain.policy import PolicyError, parse_policy +from gateway.agent_runtime.domain.types import ( + ChangedPathEvidence, + ChangedPathGate, + CheckEvidence, + CheckPassedGate, + CheckVerdict, + CommandEvidence, + CommandIfChangedGate, + CommandMatchGate, + EvidenceScope, + GateResult, + GateSpec, + JudgeEvidence, + JudgeGate, + JudgeVerdict, +) + +# A submitted evidence list is caller-observed, not Otari-observed, but it is +# still bounded input: this caps a pathological request/run, not a real repo. +_MAX_PATH_LENGTH = 4096 +_MAX_COMMAND_LENGTH = 4096 + +# A per-match cost bound (domain/evaluators.py) does not bound the total cost +# of one check: this estimates total path-match work as +# pattern_count * total_path_length + path_count * total_pattern_length, +# which is what the matcher's own cost scales with, and rejects a policy/ +# evidence combination whose estimate is disproportionate rather than let it +# run. See routes/hooks.py's history (this module inherits its calibration +# unchanged) for the benchmarking behind these numbers. +_MAX_MATCH_WORK = 50_000_000 + +# A byte-weighted budget alone understates a check built from many *short* +# patterns and paths: each comparison costs a near-constant overhead +# regardless of how few bytes it compares, so a cheap-by-bytes check can +# still mean millions of individual calls. This bounds the raw comparison +# count directly, independent of length. +_MAX_COMPARISONS = 1_000_000 + +# command_match's per-(phrase, command) match cost is a product, not a sum, +# so it needs its own bound and its own calibration from +# total_pattern_tokens * total_command_tokens. +_MAX_COMMAND_MATCH_WORK = 2_000_000 + +# Independent of token length, for the same reason _MAX_COMPARISONS exists +# alongside _MAX_MATCH_WORK: many short phrases against many near-empty +# commands is still one comparison per pair. +_MAX_COMMAND_COMPARISONS = 500_000 + +# Tokenizing itself is not free (shlex.split costs real time per character +# regardless of content), so this caps the raw character total *before* any +# command is tokenized, using only len() (uniformly cheap regardless of +# content). +_MAX_TOTAL_COMMAND_CHARS = 2_000_000 + + +class PolicyCheckError(Exception): + """The policy, or the evidence submitted against it, cannot be evaluated. + + Covers both a malformed policy (``PolicyError``, from ``parse_policy``) + and evidence whose match cost exceeds this build's budgets, so a caller + has one exception to catch rather than two. + """ + + +@dataclass(frozen=True, slots=True) +class PolicyCheckResult: + """One evaluated policy check: every gate's result, folded into a verdict.""" + + policy_id: str + schema_version: str + results: tuple[GateResult, ...] + blocked: bool + + +def _evaluate_gate( + gate: GateSpec, + changed_path_evidence: ChangedPathEvidence | None, + command_evidence: CommandEvidence | None, + judge_evidence: JudgeEvidence | None, + check_evidence: CheckEvidence | None, + segment_cache: dict[str, list[list[str]]] | None, + phrase_cache: dict[str, list[str]] | None, +) -> GateResult: + """Dispatch one gate to its evaluator. Extend as a new gate type joins ``GateSpec``.""" + if isinstance(gate, ChangedPathGate): + return evaluate_changed_path(gate, changed_path_evidence) + if isinstance(gate, CommandMatchGate): + return evaluate_command_match(gate, command_evidence, segment_cache=segment_cache, phrase_cache=phrase_cache) + if isinstance(gate, JudgeGate): + return evaluate_judge(gate, changed_path_evidence, judge_evidence) + if isinstance(gate, CheckPassedGate): + return evaluate_check_passed(gate, changed_path_evidence, check_evidence) + return evaluate_command_if_changed( + gate, changed_path_evidence, command_evidence, segment_cache=segment_cache, phrase_cache=phrase_cache + ) + + +def run_policy_check( + policy_yaml: str, + *, + source: str, + changed_paths: Sequence[str] | None, + commands: Sequence[str] | None, + command_scope: EvidenceScope = "call", + judge_results: Sequence[JudgeVerdict] | None = None, + check_results: Sequence[CheckVerdict] | None = None, +) -> PolicyCheckResult: + """Parse ``policy_yaml`` and evaluate it against the given evidence. + + ``source`` names ``policy_yaml`` in a raised ``PolicyCheckError`` (e.g. + "request body", or a gates-file path). Every other argument mirrors + ``routes/hooks.py``'s own ``PolicyCheckRequest`` fields one for one, + including their tri-state contracts (see that model's own field docs, + and docs/agent-gates.md): ``None`` means a caller that never collects + that evidence kind at all (resolves ``unknown``/``not_applicable`` + depending on the gate type); ``()``/``[]`` means it collected some and + there is none (resolves ``not_applicable``). + """ + try: + spec = parse_policy(policy_yaml, source=source) + except PolicyError as exc: + raise PolicyCheckError(str(exc)) from exc + + for path in changed_paths or (): + if len(path) > _MAX_PATH_LENGTH: + raise PolicyCheckError(f"changed_paths entry exceeds {_MAX_PATH_LENGTH} characters.") + for command in commands or (): + if len(command) > _MAX_COMMAND_LENGTH: + raise PolicyCheckError(f"commands entry exceeds {_MAX_COMMAND_LENGTH} characters.") + + changed_path_gates = [gate for gate in spec.gates if isinstance(gate, ChangedPathGate)] + command_match_gates = [gate for gate in spec.gates if isinstance(gate, CommandMatchGate)] + command_if_changed_gates = [gate for gate in spec.gates if isinstance(gate, CommandIfChangedGate)] + judge_gates = [gate for gate in spec.gates if isinstance(gate, JudgeGate)] + check_passed_gates = [gate for gate in spec.gates if isinstance(gate, CheckPassedGate)] + + # Deduplicated once here and reused below: gate.forbidden/when_changed/ + # require are already deduplicated at parse time (domain.policy), so + # each estimate and its matching evaluation always agree on the same, + # cheaper counts. command_if_changed's, judge's, and check_passed's own + # when_changed globs are path-matching work exactly like changed_path's + # forbidden globs (evaluate_judge/evaluate_check_passed both call the + # same matched_changed_paths), so all four share the same budget. + changed_path_evidence = ( + ChangedPathEvidence(changed_paths=tuple(dict.fromkeys(changed_paths))) if changed_paths is not None else None + ) + changed_path_list = changed_path_evidence.changed_paths if changed_path_evidence is not None else () + path_globs = ( + [glob for gate in changed_path_gates for glob in gate.forbidden] + + [glob for gate in command_if_changed_gates for glob in gate.when_changed] + + [glob for gate in judge_gates for glob in gate.when_changed] + + [glob for gate in check_passed_gates for glob in gate.when_changed] + ) + pattern_count = len(path_globs) + total_pattern_length = sum(len(glob) for glob in path_globs) + path_count = len(changed_path_list) + total_path_length = sum(len(path) for path in changed_path_list) + estimated_work = pattern_count * total_path_length + path_count * total_pattern_length + comparisons = pattern_count * path_count + if estimated_work > _MAX_MATCH_WORK or comparisons > _MAX_COMPARISONS: + raise PolicyCheckError( + f"This policy and evidence would take an estimated {estimated_work:,} match operations " + f"across {comparisons:,} pattern/path comparisons, over this build's limits " + f"({_MAX_MATCH_WORK:,} and {_MAX_COMPARISONS:,} respectively). Narrow the policy's " + "forbidden globs or the submitted changed_paths." + ) + + command_evidence = ( + CommandEvidence(commands=tuple(dict.fromkeys(commands)), scope=command_scope) if commands is not None else None + ) + # Gated on there being a gate that reads command evidence at all, and on + # evidence actually being present: tokenizing a command is exactly the + # cost _MAX_TOTAL_COMMAND_CHARS below exists to bound, and a policy or + # caller with nothing to check against must not pay it. + segment_cache: dict[str, list[list[str]]] | None = None + phrase_cache: dict[str, list[str]] | None = None + if (command_match_gates or command_if_changed_gates) and command_evidence is not None: + total_command_chars = sum(len(command) for command in command_evidence.commands) + if total_command_chars > _MAX_TOTAL_COMMAND_CHARS: + raise PolicyCheckError( + f"Submitted commands total {total_command_chars:,} characters, over the " + f"{_MAX_TOTAL_COMMAND_CHARS:,} limit. Narrow the submitted commands." + ) + + # Tokenized exactly once here and reused for both the estimate below + # and the real evaluation further down (passed to every + # evaluate_command_match/evaluate_command_if_changed call as + # segment_cache): each is called once per gate against this same + # evidence, and without sharing this, each call would re-tokenize + # every command from scratch, multiplying the already-checked cost + # above by the number of gates. + segment_cache = tokenize_commands(command_evidence.commands) + + # Policy parsing already proved every forbidden/require phrase + # tokenizes (domain.policy's own validation), so this cannot raise. + # command_if_changed's require phrases are command-matching work + # exactly like command_match's forbidden phrases, so they share the + # same budget and cache rather than needing a third one. + command_phrases = tuple(phrase for gate in command_match_gates for phrase in gate.forbidden) + tuple( + phrase for gate in command_if_changed_gates for phrase in gate.require + ) + phrase_cache = tokenize_phrases(command_phrases) + # Per gate occurrence, not per distinct phrase text: phrase_cache + # dedupes identical phrase text across gates so each is tokenized + # once, but evaluate_command_match/evaluate_command_if_changed still + # run the match once per gate that carries it. + phrase_count = sum(len(gate.forbidden) for gate in command_match_gates) + sum( + len(gate.require) for gate in command_if_changed_gates + ) + total_phrase_tokens = sum( + len(phrase_cache[phrase]) for gate in command_match_gates for phrase in gate.forbidden + ) + sum(len(phrase_cache[phrase]) for gate in command_if_changed_gates for phrase in gate.require) + command_count = len(command_evidence.commands) + total_command_tokens = sum(len(segment) for segments in segment_cache.values() for segment in segments) + estimated_command_work = total_phrase_tokens * total_command_tokens + command_comparisons = phrase_count * command_count + if estimated_command_work > _MAX_COMMAND_MATCH_WORK or command_comparisons > _MAX_COMMAND_COMPARISONS: + raise PolicyCheckError( + f"This policy and evidence would take an estimated {estimated_command_work:,} command match " + f"operations across {command_comparisons:,} phrase/command comparisons, over this build's " + f"limits ({_MAX_COMMAND_MATCH_WORK:,} and {_MAX_COMMAND_COMPARISONS:,} respectively). Narrow " + "the policy's forbidden phrases or the submitted commands." + ) + + judge_evidence = JudgeEvidence(verdicts=tuple(judge_results)) if judge_results is not None else None + check_evidence = CheckEvidence(verdicts=tuple(check_results)) if check_results is not None else None + + # Evaluated in declaration order (not grouped by type) so a caller + # reading `results` positionally sees the same order as the policy it + # submitted. + results = tuple( + _evaluate_gate( + gate, changed_path_evidence, command_evidence, judge_evidence, check_evidence, segment_cache, phrase_cache + ) + for gate in spec.gates + ) + blocked = any(result.enforcement == "required" and result.outcome.is_blocking for result in results) + + return PolicyCheckResult( + policy_id=spec.policy_id, + schema_version=spec.schema_version, + results=results, + blocked=blocked, + ) diff --git a/src/gateway/api/routes/hooks.py b/src/gateway/api/routes/hooks.py index a21d64c526..7982960ef0 100644 --- a/src/gateway/api/routes/hooks.py +++ b/src/gateway/api/routes/hooks.py @@ -1,10 +1,13 @@ """Otari's Hook Server: evaluate an Agent Gates policy against caller-submitted evidence. -Otari never reads a caller's repository. The caller (an agent hook, -eventually the native ``otari hook`` dispatcher) already read its own -``.otari-gates.yml`` and collected its own Git evidence, and submits both -here in one request; this route parses and evaluates them and returns the -per-gate results. This is the integration mechanism that +Otari never reads a caller's repository. The caller (an agent hook, e.g. +``otari hook``) already read its own ``.otari-gates.yml`` and collected its +own Git evidence, and submits both here in one request; this route parses +and evaluates them and returns the per-gate results, exactly the way ``otari +hook`` itself evaluates the same policy in process by default (see +``agent_runtime.domain.check.run_policy_check``, which both call): this +route is the opt-in path for a caller that wants a gateway to be the one +deciding instead. This is the integration mechanism that docs/otari-product-foundation.md calls the Hook Server; see docs/agent-gates.md for the request/response contract. @@ -23,32 +26,9 @@ from pydantic import BaseModel, ConfigDict, Field from sqlalchemy.ext.asyncio import AsyncSession -from gateway.agent_runtime.domain.evaluators import ( - evaluate_changed_path, - evaluate_check_passed, - evaluate_command_if_changed, - evaluate_command_match, - evaluate_judge, - tokenize_commands, - tokenize_phrases, -) -from gateway.agent_runtime.domain.policy import MAX_GATE_ID_LENGTH, MAX_POLICY_BYTES, PolicyError, parse_policy -from gateway.agent_runtime.domain.types import ( - ChangedPathEvidence, - ChangedPathGate, - CheckEvidence, - CheckPassedGate, - CheckVerdict, - CommandEvidence, - CommandIfChangedGate, - CommandMatchGate, - EvidenceScope, - GateResult, - GateSpec, - JudgeEvidence, - JudgeGate, - JudgeVerdict, -) +from gateway.agent_runtime.domain.check import PolicyCheckError, run_policy_check +from gateway.agent_runtime.domain.policy import MAX_GATE_ID_LENGTH, MAX_POLICY_BYTES +from gateway.agent_runtime.domain.types import CheckVerdict, EvidenceScope, JudgeVerdict from gateway.api.deps import extract_credential_token, get_config, get_db_if_needed, verify_api_key_or_master_key from gateway.core.config import GatewayConfig @@ -74,7 +54,8 @@ async def verify_hook_caller( does there. That is weaker on purpose and it is all this endpoint needs: it reads no tenant data, writes nothing, bills nothing, and evaluates only the policy and evidence the caller sent in the same request. What a - request can cost is bounded by the work budgets below, not by who sent it. + request can cost is bounded by ``agent_runtime.domain.check``'s own work + budgets, not by who sent it. """ if config.is_hybrid_mode: extract_credential_token(request) @@ -100,78 +81,14 @@ async def verify_hook_caller( # still bounded input: this caps a pathological request, not a real repo. # The policy_yaml bound is domain.policy's own MAX_POLICY_BYTES, reused here # rather than duplicated so the Pydantic-level and parser-level limits cannot -# drift apart. +# drift apart. The match-cost budgets that used to sit here (per-entry +# length, and the total-work estimates for changed_path/command_match/ +# command_if_changed) moved to agent_runtime.domain.check.run_policy_check, +# since they guard the evaluator's own cost, not this route's: `otari hook`'s +# own local evaluation needs them just as much as an HTTP caller does, and +# sharing one place keeps the two from drifting apart. _MAX_CHANGED_PATHS = 10_000 -_MAX_PATH_LENGTH = 4096 - -# A per-match cost bound (domain/evaluators.py) does not bound the total cost -# of one request: MAX_POLICY_BYTES and _MAX_CHANGED_PATHS are each generous -# enough alone that maxing out both dimensions at once measured multiple -# seconds of matching in testing (100 forbidden globs x 10,000 changed paths, -# realistic lengths, took over 2.5s). This estimates total match work as -# pattern_count * total_path_length + path_count * total_pattern_length, -# which is what the matcher's own cost scales with, and rejects a request -# whose combination is disproportionate rather than let it run. Chosen with -# a safety margin under the ~350M-work / 0.58s point measured in benchmarking -# (tests/unit/agent_runtime/test_evaluators.py); a realistic policy (tens of -# gates, a handful of forbidden globs each) against a large changed-file set -# stays at least an order of magnitude under it. -_MAX_MATCH_WORK = 50_000_000 - -# A byte-weighted budget alone understates a request built from many *short* -# patterns and paths: each _segment_matches call costs a near-constant Python -# function-call overhead regardless of how few bytes it compares, so a -# request that is cheap by total bytes can still mean millions of individual -# calls. 2,500 one-byte forbidden globs against 10,000 one-byte changed paths -# measured 50,000,000 estimated work, exactly at (not over) _MAX_MATCH_WORK, -# for 25,000,000 real match calls that took ~5s. This bounds the raw call -# count directly, independent of length; benchmarking the same degenerate -# shape (short, non-matching, all-distinct strings, so neither the matcher's -# own short-circuits nor the deduplication in domain.policy and -# changed_path_evidence collapse the work) measured 2,000,000 calls at -# ~0.39-0.4s regardless of how that count split between pattern_count and -# path_count. -_MAX_COMPARISONS = 1_000_000 - _MAX_COMMANDS = 10_000 -_MAX_COMMAND_LENGTH = 4096 - -# command_match's per-(phrase, command) match cost is a product, not a sum: -# matching one forbidden phrase against one command segment is -# O(len(segment tokens) * len(phrase tokens)) (domain/evaluators.py's -# _contains_subsequence checks every candidate start position, each an -# O(len(phrase)) slice comparison). Summed over every phrase against every -# command, that product distributes into a single multiplication: -# total_pattern_tokens * total_command_tokens. This is a different shape from -# _MAX_MATCH_WORK's sum-of-cross-terms (changed_path's per-comparison cost is -# a *sum* of lengths, not a product), so it needs its own bound and its own -# calibration: 2,000 one-token forbidden phrases against 2,000 one-token -# commands (4,000,000 estimated work) measured ~0.7s; chosen with margin -# under that. -_MAX_COMMAND_MATCH_WORK = 2_000_000 - -# Independent of token length, for the same reason _MAX_COMPARISONS exists -# alongside _MAX_MATCH_WORK: many short phrases against many near-empty -# commands (e.g. all-whitespace strings, which tokenize to zero tokens each, -# so _MAX_COMMAND_MATCH_WORK's product is zero regardless of phrase count) -# is still one Python-level comparison per pair. 2,000 phrases against 10,000 -# such commands (20,000,000 comparisons, 0 estimated work) measured ~4.7s. -# Chosen with margin under the ~1,000,000-comparisons / ~0.2s point measured -# at the same degenerate shape. -_MAX_COMMAND_COMPARISONS = 500_000 - -# Tokenizing itself is not free: shlex.split costs roughly 100-350ns per -# character it tokenizes, regardless of content, which is 50-150x the cost -# of a plain len() check. That is irrelevant at the scale of one command, -# but _MAX_COMMANDS * _MAX_COMMAND_LENGTH allows up to ~41,000,000 -# characters in one request, and tokenizing all of it measured several -# seconds before either budget below ever saw a token count to reject: a -# policy with zero command_match gates would still pay this cost computing -# total_command_tokens, since that sum is what proves there is nothing to -# bound. This caps the raw character total *before* any command is -# tokenized, using only len() (uniformly cheap regardless of content). -# 2,000,000 characters measured ~0.17s; chosen with margin under that. -_MAX_TOTAL_COMMAND_CHARS = 2_000_000 # A judge verdict is a small, fixed-shape record (see JudgeVerdictRequest), not # a pattern this route matches against other input, so its bound is a plain @@ -299,44 +216,6 @@ class PolicyCheckRequest(BaseModel): description="Verifier verdicts the caller collected for this request's check_passed gates.", ) - @property - def changed_path_evidence(self) -> ChangedPathEvidence | None: - # A duplicate path adds nothing a single copy wouldn't already tell a - # gate; collapsing it here means the work-budget check below and the - # actual matching agree on the same, cheaper count rather than one - # estimating off raw input and the other paying for the duplicates. - if self.changed_paths is None: - return None - return ChangedPathEvidence(changed_paths=tuple(dict.fromkeys(self.changed_paths))) - - @property - def command_evidence(self) -> CommandEvidence | None: - if self.commands is None: - return None - return CommandEvidence(commands=tuple(dict.fromkeys(self.commands)), scope=self.command_scope) - - @property - def judge_evidence(self) -> JudgeEvidence | None: - if self.judge_results is None: - return None - return JudgeEvidence( - verdicts=tuple( - JudgeVerdict(gate_id=verdict.gate_id, outcome=verdict.outcome, reasoning=verdict.reasoning) - for verdict in self.judge_results - ) - ) - - @property - def check_evidence(self) -> CheckEvidence | None: - if self.check_results is None: - return None - return CheckEvidence( - verdicts=tuple( - CheckVerdict(gate_id=verdict.gate_id, outcome=verdict.outcome, detail=verdict.detail) - for verdict in self.check_results - ) - ) - class GateResultResponse(BaseModel): gate_id: str @@ -354,29 +233,6 @@ class PolicyCheckResponse(BaseModel): blocked: bool -def _evaluate_gate( - gate: GateSpec, - changed_path_evidence: ChangedPathEvidence | None, - command_evidence: CommandEvidence | None, - judge_evidence: JudgeEvidence | None, - check_evidence: CheckEvidence | None, - segment_cache: dict[str, list[list[str]]] | None, - phrase_cache: dict[str, list[str]] | None, -) -> GateResult: - """Dispatch one gate to its evaluator. Extend as a new gate type joins ``GateSpec``.""" - if isinstance(gate, ChangedPathGate): - return evaluate_changed_path(gate, changed_path_evidence) - if isinstance(gate, CommandMatchGate): - return evaluate_command_match(gate, command_evidence, segment_cache=segment_cache, phrase_cache=phrase_cache) - if isinstance(gate, JudgeGate): - return evaluate_judge(gate, changed_path_evidence, judge_evidence) - if isinstance(gate, CheckPassedGate): - return evaluate_check_passed(gate, changed_path_evidence, check_evidence) - return evaluate_command_if_changed( - gate, changed_path_evidence, command_evidence, segment_cache=segment_cache, phrase_cache=phrase_cache - ) - - @router.post("/check") async def check_policy(request: PolicyCheckRequest) -> PolicyCheckResponse: """Evaluate a submitted policy against submitted evidence. @@ -386,159 +242,52 @@ async def check_policy(request: PolicyCheckRequest) -> PolicyCheckResponse: sent the request, not whether its evidence is true. `blocked` is set when a required gate's outcome is not `pass`/`not_applicable` (an unresolved gate never counts as a pass). + + The actual parse-and-evaluate work is + ``agent_runtime.domain.check.run_policy_check``, shared with ``otari + hook``'s own local evaluation: this route's own job is authentication, + translating that function's tri-state request fields into its own typed + ones, and turning ``PolicyCheckError`` into a 422. """ try: - spec = parse_policy(request.policy_yaml, source="request body") - except PolicyError as exc: - raise HTTPException(status_code=422, detail=str(exc)) from exc - - for path in request.changed_paths or []: - if len(path) > _MAX_PATH_LENGTH: - raise HTTPException(status_code=422, detail=f"changed_paths entry exceeds {_MAX_PATH_LENGTH} characters.") - for command in request.commands or []: - if len(command) > _MAX_COMMAND_LENGTH: - raise HTTPException(status_code=422, detail=f"commands entry exceeds {_MAX_COMMAND_LENGTH} characters.") - - changed_path_gates = [gate for gate in spec.gates if isinstance(gate, ChangedPathGate)] - command_match_gates = [gate for gate in spec.gates if isinstance(gate, CommandMatchGate)] - command_if_changed_gates = [gate for gate in spec.gates if isinstance(gate, CommandIfChangedGate)] - judge_gates = [gate for gate in spec.gates if isinstance(gate, JudgeGate)] - check_passed_gates = [gate for gate in spec.gates if isinstance(gate, CheckPassedGate)] - - # Built once and reused below: gate.forbidden/when_changed/require are - # already deduplicated at parse time (domain.policy), and - # changed_path_evidence/command_evidence deduplicate their evidence lists - # the same way, so each estimate and its matching evaluation below always - # agree on the same, cheaper counts. command_if_changed's, judge's, and - # check_passed's own when_changed globs are path-matching work exactly - # like changed_path's forbidden globs (evaluate_judge/evaluate_check_passed - # both call the same matched_changed_paths), so all four share the same - # budget rather than needing a fifth one. - changed_path_evidence = request.changed_path_evidence - changed_paths = changed_path_evidence.changed_paths if changed_path_evidence is not None else () - path_globs = ( - [glob for gate in changed_path_gates for glob in gate.forbidden] - + [glob for gate in command_if_changed_gates for glob in gate.when_changed] - + [glob for gate in judge_gates for glob in gate.when_changed] - + [glob for gate in check_passed_gates for glob in gate.when_changed] - ) - pattern_count = len(path_globs) - total_pattern_length = sum(len(glob) for glob in path_globs) - path_count = len(changed_paths) - total_path_length = sum(len(path) for path in changed_paths) - estimated_work = pattern_count * total_path_length + path_count * total_pattern_length - comparisons = pattern_count * path_count - if estimated_work > _MAX_MATCH_WORK or comparisons > _MAX_COMPARISONS: - raise HTTPException( - status_code=422, - detail=( - f"This policy and evidence would take an estimated {estimated_work:,} match operations " - f"across {comparisons:,} pattern/path comparisons, over this build's limits " - f"({_MAX_MATCH_WORK:,} and {_MAX_COMPARISONS:,} respectively). Narrow the policy's " - "forbidden globs or the submitted changed_paths." + result = run_policy_check( + request.policy_yaml, + source="request body", + changed_paths=request.changed_paths, + commands=request.commands, + command_scope=request.command_scope, + judge_results=( + None + if request.judge_results is None + else [ + JudgeVerdict(gate_id=verdict.gate_id, outcome=verdict.outcome, reasoning=verdict.reasoning) + for verdict in request.judge_results + ] + ), + check_results=( + None + if request.check_results is None + else [ + CheckVerdict(gate_id=verdict.gate_id, outcome=verdict.outcome, detail=verdict.detail) + for verdict in request.check_results + ] ), ) - - command_evidence = request.command_evidence - # Gated on there being a gate that reads command evidence at all: - # tokenizing a command is exactly the cost _MAX_TOTAL_COMMAND_CHARS below - # exists to bound. A policy with neither command_match nor - # command_if_changed (every policy shipped before this gate type - # existed) must not pay that cost just to prove there is nothing to - # bound it against. Also gated on evidence actually being present: - # `commands` omitted from the request means command_evidence is None, - # and there is nothing to tokenize or bound in that case either. - segment_cache: dict[str, list[list[str]]] | None = None - phrase_cache: dict[str, list[str]] | None = None - if (command_match_gates or command_if_changed_gates) and command_evidence is not None: - total_command_chars = sum(len(command) for command in command_evidence.commands) - if total_command_chars > _MAX_TOTAL_COMMAND_CHARS: - raise HTTPException( - status_code=422, - detail=( - f"Submitted commands total {total_command_chars:,} characters, over the " - f"{_MAX_TOTAL_COMMAND_CHARS:,} limit. Narrow the submitted commands." - ), - ) - - # Tokenized exactly once here and reused for both the estimate below - # and the real evaluation further down (passed to every - # evaluate_command_match/evaluate_command_if_changed call as - # segment_cache): each is called once per gate against this same - # evidence, and without sharing this, each call would re-tokenize - # every command from scratch, multiplying the already-checked cost - # above by the number of gates. A request with 100 command_match - # gates each forbidding "npm" against 250 distinct ~4,000-character - # commands passed every budget here (low token content, few phrases, - # under _MAX_TOTAL_COMMAND_CHARS) yet measured ~7s of synchronous - # blocking from exactly that multiplication before this was shared. - segment_cache = tokenize_commands(command_evidence.commands) - - # Policy parsing already proved every forbidden/require phrase - # tokenizes (domain.policy's own validation), so this cannot raise. - # Shared with the evaluation below via phrase_cache for the same - # reason segment_cache is: tokenized here for the estimate and then - # again inside every evaluate_command_match/evaluate_command_if_changed - # call is the same work twice. command_if_changed's require phrases - # are command-matching work exactly like command_match's forbidden - # phrases, so they share the same budget and cache rather than - # needing a third one. - command_phrases = tuple(phrase for gate in command_match_gates for phrase in gate.forbidden) + tuple( - phrase for gate in command_if_changed_gates for phrase in gate.require - ) - phrase_cache = tokenize_phrases(command_phrases) - # Per gate occurrence, not per distinct phrase text: phrase_cache - # dedupes identical phrase text across gates so each is tokenized - # once, but evaluate_command_match/evaluate_command_if_changed still - # run _contains_subsequence once per gate that carries it. Summing - # len(phrase_cache.values()) counted a shared phrase's tokens once - # regardless of how many gates forbid/require it, undercounting the - # real per-gate matching work whenever gates share phrase text. - phrase_count = sum(len(gate.forbidden) for gate in command_match_gates) + sum( - len(gate.require) for gate in command_if_changed_gates - ) - total_phrase_tokens = sum( - len(phrase_cache[phrase]) for gate in command_match_gates for phrase in gate.forbidden - ) + sum(len(phrase_cache[phrase]) for gate in command_if_changed_gates for phrase in gate.require) - command_count = len(command_evidence.commands) - total_command_tokens = sum(len(segment) for segments in segment_cache.values() for segment in segments) - estimated_command_work = total_phrase_tokens * total_command_tokens - command_comparisons = phrase_count * command_count - if estimated_command_work > _MAX_COMMAND_MATCH_WORK or command_comparisons > _MAX_COMMAND_COMPARISONS: - raise HTTPException( - status_code=422, - detail=( - f"This policy and evidence would take an estimated {estimated_command_work:,} command match " - f"operations across {command_comparisons:,} phrase/command comparisons, over this build's " - f"limits ({_MAX_COMMAND_MATCH_WORK:,} and {_MAX_COMMAND_COMPARISONS:,} respectively). Narrow " - "the policy's forbidden phrases or the submitted commands." - ), - ) - - # Evaluated in declaration order (not grouped by type) so a caller reading - # `results` positionally sees the same order as the policy it submitted. - judge_evidence = request.judge_evidence - check_evidence = request.check_evidence - results = [ - _evaluate_gate( - gate, changed_path_evidence, command_evidence, judge_evidence, check_evidence, segment_cache, phrase_cache - ) - for gate in spec.gates - ] - blocked = any(result.enforcement == "required" and result.outcome.is_blocking for result in results) + except PolicyCheckError as exc: + raise HTTPException(status_code=422, detail=str(exc)) from exc return PolicyCheckResponse( - policy_id=spec.policy_id, - schema_version=spec.schema_version, + policy_id=result.policy_id, + schema_version=result.schema_version, results=[ GateResultResponse( - gate_id=result.gate_id, - enforcement=result.enforcement, - outcome=result.outcome.value, - message=result.message, - detail=result.detail, + gate_id=gate_result.gate_id, + enforcement=gate_result.enforcement, + outcome=gate_result.outcome.value, + message=gate_result.message, + detail=gate_result.detail, ) - for result in results + for gate_result in result.results ], - blocked=blocked, + blocked=result.blocked, ) diff --git a/src/gateway/cli.py b/src/gateway/cli.py index cef0b9f60a..8481e37780 100644 --- a/src/gateway/cli.py +++ b/src/gateway/cli.py @@ -10,15 +10,16 @@ import time from datetime import UTC, datetime from pathlib import Path -from typing import NamedTuple +from typing import Literal, NamedTuple, cast import click import uvicorn from uvicorn.config import logger +from gateway.agent_runtime.domain.check import PolicyCheckError, run_policy_check from gateway.agent_runtime.domain.evaluators import matched_changed_paths from gateway.agent_runtime.domain.policy import PolicyError, parse_policy -from gateway.agent_runtime.domain.types import CheckPassedGate, JudgeGate +from gateway.agent_runtime.domain.types import CheckPassedGate, CheckVerdict, EvidenceScope, JudgeGate, JudgeVerdict from gateway.core.config import API_KEY_HEADER, API_ROOT, load_config from gateway.log_config import setup_logger @@ -291,23 +292,28 @@ def _hook_extract_patch_paths(patch_text: str) -> list[str]: return list(seen) -# Mirrors the Hook Server's own per-command bound (routes/hooks.py's -# _MAX_COMMAND_LENGTH). A literal rather than an import: this command talks to -# a gateway over HTTP that may be a different build, so the number it truncates -# to is its own best guess at the far side's limit, not a shared constant that -# would imply the two are always one process. +# Mirrors the evaluator's own per-command bound +# (agent_runtime.domain.check's _MAX_COMMAND_LENGTH). A literal rather than +# an import: the opt-in remote mode talks to a gateway over HTTP that may be +# a different build, so the number it truncates to is its own best guess at +# the far side's limit, not a shared constant that would imply the two are +# always one process. The default, local mode calls the same evaluator +# in process and would raise `PolicyCheckError` on the same bound anyway; +# truncating here means an oversize command still gets checked, just with +# its tail cut, rather than failing the whole check open. _HOOK_MAX_COMMAND_LENGTH = 4096 -# Mirror routes/hooks.py's own _MAX_COMMANDS/_MAX_TOTAL_COMMAND_CHARS, for the -# same reason _HOOK_MAX_COMMAND_LENGTH does: per-command truncation alone does -# not bound the total. A Stop event now submits every Bash command the whole -# session ran, not the single command a PreToolUse call would carry, so -# reaching this aggregate is a real, not pathological, outcome of a long -# session with several long commands (501 commands truncated to -# _HOOK_MAX_COMMAND_LENGTH each already clears 2,000,000 characters). Left -# unbounded, the server 422s the whole request, and that failure is total: it -# takes every gate in the policy with it, changed_path included, not just the -# command-evidence ones. +# Mirror agent_runtime.domain.check's own _MAX_COMMANDS/_MAX_TOTAL_COMMAND_CHARS, +# for the same reason _HOOK_MAX_COMMAND_LENGTH does: per-command truncation +# alone does not bound the total. A Stop event now submits every Bash +# command the whole session ran, not the single command a PreToolUse call +# would carry, so reaching this aggregate is a real, not pathological, +# outcome of a long session with several long commands (501 commands +# truncated to _HOOK_MAX_COMMAND_LENGTH each already clears 2,000,000 +# characters). Left unbounded, the evaluator (local or remote) 422s/raises +# on the whole request, and that failure is total: it takes every gate in +# the policy with it, changed_path included, not just the command-evidence +# ones. _HOOK_MAX_COMMANDS = 10_000 _HOOK_MAX_TOTAL_COMMAND_CHARS = 2_000_000 @@ -620,26 +626,27 @@ def _hook_collect_codex_transcript_commands(transcript_path: Path) -> list[str] # unbounded wall-clock on a single Stop event: N gates at the timeout above # would be N * 300s in the worst case. Capped, with a visible truncation # message, the same "never let something scale unbounded and silently" rule -# _bound_commands_for_submission and the Hook Server's own work-estimate -# budgets (routes/hooks.py) already follow. Evaluated in declaration order, so -# the same gates run first every time rather than an arbitrary subset. +# _bound_commands_for_submission and the evaluator's own work-estimate +# budgets (agent_runtime.domain.check) already follow. Evaluated in +# declaration order, so the same gates run first every time rather than an +# arbitrary subset. _HOOK_JUDGE_MAX_GATES_PER_RUN = 5 # A per-call cap does not bound the total: 5 gates at up to 300s each, each # with its own possible retry (_HOOK_JUDGE_PROMPT_TOO_LONG_MARKER), is up to # 3,000s of judge calls alone. Claude Code's own command-hook timeout # defaults to 600s, after which it kills the hook and discards its output -# entirely (see the hooks reference) -- meaning the Hook Server never gets -# contacted at all, and every gate in the policy, mechanical and required +# entirely (see the hooks reference) -- meaning the policy check itself never +# gets run at all, and every gate in the policy, mechanical and required # ones included, goes unevaluated for that Stop event, not just the slow # judge gates. This is a *total* elapsed-time budget shared across every # judge gate and retry in one run (_hook_collect_judge_verdicts computes one # deadline before its gate loop, not one budget per gate), leaving real # margin under the 600s default for the git evidence collection and the -# /hooks/check request that still have to happen afterward. A gate whose -# turn comes up after the deadline has passed reports "error" without -# attempting the call at all, the same fail-open contract a missing `claude` -# binary already has. +# `run_policy_check` call (or, opted in, the /hooks/check request) that +# still has to happen afterward. A gate whose turn comes up after the +# deadline has passed reports "error" without attempting the call at all, +# the same fail-open contract a missing `claude` binary already has. _HOOK_JUDGE_TOTAL_BUDGET_SECONDS = 480 # Haiku, not the session's own (often larger) default model: a judge call is a @@ -1232,13 +1239,13 @@ def _hook_collect_judge_verdicts( """Run every applicable judge gate in the local policy, one model-CLI call each. Parses the policy locally with the same pure `domain.policy.parse_policy` - the Hook Server itself uses, purely to find which gates are judge gates - and read their `rubric`/`when_changed`; the Hook Server still re-parses - and validates the submitted `policy_yaml` on its own, so a mismatch here - only means judge evidence for a gate the server would reject anyway. A - local parse failure collects no verdicts rather than raising: the - existing fail-open request below still submits the policy text for the - server to report the same error on. + `run_policy_check` itself uses below, purely to find which gates are + judge gates and read their `rubric`/`when_changed`; that later call + re-parses and validates the same `policy_yaml` on its own, so a mismatch + here only means judge evidence for a gate that call would reject anyway. + A local parse failure collects no verdicts rather than raising: the + fail-open call to `run_policy_check` below still gets the same policy + text and reports the same error on it. A judge gate with `when_changed` is skipped locally, before ever reading the diff/transcript or shelling out to `claude -p`, when none of @@ -1500,14 +1507,14 @@ def _hook_collect_check_verdicts( """Run every applicable check_passed gate's verifier locally; return check_results. Structured exactly like `_hook_collect_judge_verdicts`: parses the policy - locally with the same pure `domain.policy.parse_policy` the Hook Server - itself uses, purely to find which gates are check_passed gates and read - their `verifier`/`when_changed`; the Hook Server still re-parses and - validates the submitted `policy_yaml` on its own, so a mismatch here only - means check evidence for a gate the server would reject anyway. A local - parse failure collects no verdicts rather than raising: the existing - fail-open request still submits the policy text for the server to report - the same error on. + locally with the same pure `domain.policy.parse_policy` `run_policy_check` + itself uses below, purely to find which gates are check_passed gates and + read their `verifier`/`when_changed`; that later call re-parses and + validates the same `policy_yaml` on its own, so a mismatch here only + means check evidence for a gate that call would reject anyway. A local + parse failure collects no verdicts rather than raising: the fail-open + call to `run_policy_check` below still gets the same policy text and + reports the same error on it. A gate with `when_changed` is skipped locally, before ever running its verifier, when none of `changed_paths` matches its globs @@ -1616,16 +1623,21 @@ def hook( Reads one JSON hook payload on stdin, collects the evidence that payload carries (a PreToolUse call's own target path, or a Stop event's Git - status), and calls POST /api/v1/hooks/check. Never reads or evaluates the - policy itself: gateway.agent_runtime does that; this command is a thin, - harness-specific transport. See docs/agent-gates.md. + status), and evaluates it against the local `.otari-gates.yml` itself, in + process, through `agent_runtime.domain.check.run_policy_check`: no server, + no credential, needed for this by default. `--url`/`--api-key` (or + `OTARI_URL`/`OTARI_API_KEY`) are the opt-in exception: give either and + this instead calls a gateway's `POST /api/v1/hooks/check` over HTTP the + way every version of this command before local evaluation existed did, + for whoever wants a shared/hosted gateway to be the one deciding rather + than the machine the agent is running on. See docs/agent-gates.md. Exit code is this harness's own protocol, not otari policy check's: Claude Code's and Codex's PreToolUse and Stop hooks both take 0 (proceed) or 2 (block, stderr shown to the agent). Never blocks on a problem that is - not a required gate failing: a missing policy, an unreachable gateway, or - a missing credential all exit 0, with a message on stderr where there is - one worth surfacing. + not a required gate failing: a missing or malformed policy, or (only in + the opt-in remote mode) an unreachable gateway or a missing credential, + all exit 0, with a message on stderr where there is one worth surfacing. `--harness` picks which payload/transcript shape is expected and which tool names are read as an edit vs. a command (see @@ -1642,7 +1654,6 @@ def hook( """ if ctx.invoked_subcommand is not None: return - import httpx try: payload = json.load(sys.stdin) @@ -1678,9 +1689,9 @@ def hook( # nothing. commands: list[str] | None = [] # "call" unless the Stop branch below really does collect the whole - # session: this is what tells the server which command-evidence gates can - # resolve at all, rather than leaving each to guess from an empty list. - command_scope = "call" + # session: this is what tells the evaluator which command-evidence gates + # can resolve at all, rather than leaving each to guess from an empty list. + command_scope: EvidenceScope = "call" # None, not [], by default: a PreToolUse call has neither a full diff nor # a finished transcript to judge against yet, and never runs # _hook_collect_judge_verdicts at all, so submitting None (rather than an @@ -1818,64 +1829,123 @@ def hook( else: return # An event this harness integration does not check yet. - try: - gateway_config = load_config(config) - except ValueError as exc: - # load_config runs GatewayConfig.validate_mode_selection(), which - # raises on a real misconfiguration (e.g. OTARI_MODE=hybrid with no - # OTARI_AI_TOKEN). That is a setup problem, not a required gate - # failing, so it falls under this command's own fail-open contract. - click.echo(f"otari hook: could not load config ({exc}), not blocking.", err=True) - return - # host is a bind address (0.0.0.0 is the documented default), not a connect - # target; a client dials localhost instead. - connect_host = "localhost" if gateway_config.host == "0.0.0.0" else gateway_config.host # noqa: S104 - resolved_url = url or f"http://{connect_host}:{gateway_config.port}" - resolved_key = api_key or gateway_config.master_key - if not resolved_key: - click.echo("otari hook: no API key or master key resolved, not blocking.", err=True) - return + # No `--url`/`--api-key` (nor their envvars): the common case, and the + # default now. Evaluate the local policy in process, the same pure + # `run_policy_check` the Hook Server route itself calls, so nothing here + # needs a running gateway, a credential, or the network at all. + if url is None and api_key is None: + try: + check_result = run_policy_check( + policy_yaml, + source=str(gates_file), + changed_paths=changed_paths, + commands=commands, + command_scope=command_scope, + judge_results=( + None + if judge_results is None + else [ + JudgeVerdict( + gate_id=v["gate_id"], + outcome=cast(Literal["pass", "fail", "error"], v["outcome"]), + reasoning=v["reasoning"], + ) + for v in judge_results + ] + ), + check_results=( + None + if check_results is None + else [ + CheckVerdict( + gate_id=v["gate_id"], + outcome=cast(Literal["pass", "fail", "error"], v["outcome"]), + detail=v["detail"], + ) + for v in check_results + ] + ), + ) + except PolicyCheckError as exc: + click.echo(f"otari hook: could not evaluate {gates_file} ({exc}), not blocking.", err=True) + return + failing = [ + { + "gate_id": gate_result.gate_id, + "enforcement": gate_result.enforcement, + "outcome": gate_result.outcome.value, + "message": gate_result.message, + "detail": gate_result.detail, + } + for gate_result in check_result.results + if gate_result.outcome.value not in ("pass", "not_applicable") + ] + blocked = check_result.blocked + else: + # Explicit opt-in: check against a gateway over HTTP instead, the way + # every version of this command before local evaluation existed did. + # For whoever wants a shared/hosted gateway to be the one deciding, + # or a central place data about the check could eventually land. + import httpx - try: - response = httpx.post( - f"{resolved_url.rstrip('/')}{API_ROOT}/hooks/check", - json={ - "policy_yaml": policy_yaml, - "changed_paths": changed_paths, - "commands": commands, - "command_scope": command_scope, - "judge_results": judge_results, - "check_results": check_results, - }, - headers={API_KEY_HEADER: resolved_key}, - timeout=15.0, - ) - response.raise_for_status() - result = response.json() - failing = [gate for gate in result["results"] if gate["outcome"] not in ("pass", "not_applicable")] - blocked = result["blocked"] - except httpx.HTTPStatusError as exc: - # Split from the transport branch below on purpose: the request did - # arrive and was answered, so "could not reach" would send whoever - # debugs this to the network instead of to the status and body that - # say what was actually wrong (a policy this build cannot parse, or - # evidence over one of the route's limits). - detail = exc.response.text[:500] - click.echo( - f"otari hook: {resolved_url} rejected the check ({exc.response.status_code}: {detail}), not blocking.", - err=True, - ) - return - except httpx.HTTPError as exc: - click.echo(f"otari hook: could not reach {resolved_url} ({exc}), not blocking.", err=True) - return - except (ValueError, TypeError, KeyError) as exc: - # A body that is not JSON, or is JSON of a shape this command does not - # recognize. Same fail-open contract as an unreachable gateway: this - # command blocks on a required gate failing and on nothing else, so a - # response it cannot read must not surface as a traceback. - click.echo(f"otari hook: unreadable response from {resolved_url} ({exc!r}), not blocking.", err=True) - return + try: + gateway_config = load_config(config) + except ValueError as exc: + # load_config runs GatewayConfig.validate_mode_selection(), which + # raises on a real misconfiguration (e.g. OTARI_MODE=hybrid with no + # OTARI_AI_TOKEN). That is a setup problem, not a required gate + # failing, so it falls under this command's own fail-open contract. + click.echo(f"otari hook: could not load config ({exc}), not blocking.", err=True) + return + # host is a bind address (0.0.0.0 is the documented default), not a + # connect target; a client dials localhost instead. + connect_host = "localhost" if gateway_config.host == "0.0.0.0" else gateway_config.host # noqa: S104 + resolved_url = url or f"http://{connect_host}:{gateway_config.port}" + resolved_key = api_key or gateway_config.master_key + if not resolved_key: + click.echo("otari hook: no API key or master key resolved, not blocking.", err=True) + return + + try: + response = httpx.post( + f"{resolved_url.rstrip('/')}{API_ROOT}/hooks/check", + json={ + "policy_yaml": policy_yaml, + "changed_paths": changed_paths, + "commands": commands, + "command_scope": command_scope, + "judge_results": judge_results, + "check_results": check_results, + }, + headers={API_KEY_HEADER: resolved_key}, + timeout=15.0, + ) + response.raise_for_status() + result = response.json() + failing = [gate for gate in result["results"] if gate["outcome"] not in ("pass", "not_applicable")] + blocked = result["blocked"] + except httpx.HTTPStatusError as exc: + # Split from the transport branch below on purpose: the request did + # arrive and was answered, so "could not reach" would send whoever + # debugs this to the network instead of to the status and body that + # say what was actually wrong (a policy this build cannot parse, or + # evidence over one of the route's limits). + detail = exc.response.text[:500] + click.echo( + f"otari hook: {resolved_url} rejected the check ({exc.response.status_code}: {detail}), not blocking.", + err=True, + ) + return + except httpx.HTTPError as exc: + click.echo(f"otari hook: could not reach {resolved_url} ({exc}), not blocking.", err=True) + return + except (ValueError, TypeError, KeyError) as exc: + # A body that is not JSON, or is JSON of a shape this command does not + # recognize. Same fail-open contract as an unreachable gateway: this + # command blocks on a required gate failing and on nothing else, so a + # response it cannot read must not surface as a traceback. + click.echo(f"otari hook: unreadable response from {resolved_url} ({exc!r}), not blocking.", err=True) + return # `failing` mirrors Outcome's own non-blocking set (types.py), not just # "pass": a future gate type's not_applicable is a clean result too, and @@ -1953,14 +2023,6 @@ def _otari_binary_path() -> str: return str(Path(sys.executable).with_name("otari")) -def _resolve_hook_credential() -> str | None: - """Whatever `otari hook` would resolve automatically at runtime, no flags given.""" - try: - return load_config(None).master_key - except ValueError: - return None - - def _gates_file_allows_bash(gates_file: Path) -> bool: """Whether the matcher should include Bash: only if a command_match gate exists. @@ -2089,21 +2151,25 @@ class _HookSetup(NamedTuple): @click.option( "--api-key", default=None, - help="Skip automatic/interactive credential resolution and use this.", + help=( + "Embed this credential in the generated command, opting the registered hook into checking " + "against a gateway over HTTP instead of evaluating the policy locally. Omit for the default: " + "no credential, no server, evaluated in process." + ), ) def hook_setup(harness: str, api_key: str | None) -> None: """Register otari hook in a supported agent's own settings. Writes or updates a PreToolUse hook entry and a Stop hook entry in the harness's own personal, gitignored settings file (see - _HOOK_SETUP_BY_HARNESS) so registering the Hook Server is not a manual - JSON edit. Both point at the same otari hook invocation; the harness - passes its own hook_event_name in the payload, so one callback serves - either event. Offers to scaffold a starter .otari-gates.yml when this - repo has none yet, and picks the PreToolUse matcher (whether it needs to - cover a shell tool) from whatever gates the policy turns out to have; - Stop needs no matcher; see docs/agent-gates.md for why both are - registered unconditionally. + _HOOK_SETUP_BY_HARNESS) so registering it is not a manual JSON edit. Both + point at the same otari hook invocation; the harness passes its own + hook_event_name in the payload, so one callback serves either event. + Offers to scaffold a starter .otari-gates.yml when this repo has none + yet, and picks the PreToolUse matcher (whether it needs to cover a shell + tool) from whatever gates the policy turns out to have; Stop needs no + matcher; see docs/agent-gates.md for why both are registered + unconditionally. """ root = _hook_find_repo_root(Path.cwd()) if root is None: @@ -2124,22 +2190,19 @@ def hook_setup(harness: str, api_key: str | None) -> None: include_bash = _gates_file_allows_bash(gates_file) matcher = f"{setup.edit_matcher}|{setup.command_matcher}" if include_bash else setup.edit_matcher - embedded_key = api_key - if not embedded_key: - resolved_key = _resolve_hook_credential() - if not resolved_key: - click.echo( - "Could not resolve a credential automatically (no master_key in config.yml, .env, or the environment)." - ) - embedded_key = click.prompt("Enter an Otari API key or master key", hide_input=True) - # A key resolved automatically is not embedded: the same resolution - # otari hook already does at runtime keeps working, and this repeats - # it rather than pinning today's value (e.g. a master key that later - # rotates). - + # No credential resolution, and no prompt: `otari hook` evaluates the + # local policy in process by default and needs neither. `--api-key` here + # is the explicit opt-in to the other mode, checking against a gateway + # over HTTP instead (see `hook`'s own docstring); embedding it is what + # lets that mode work from a hook subprocess that inherits no shell + # environment. Given no `--api-key`, the generated command carries none, + # and stays that way even if a `master_key` happens to be configured + # somewhere on this machine: resolving one here anyway would silently + # decide, on this install's behalf, that gate checks should hit the + # network at all. command_parts = [_otari_binary_path(), "hook", "--harness", harness] - if embedded_key: - command_parts += ["--api-key", embedded_key] + if api_key: + command_parts += ["--api-key", api_key] command = shlex.join(command_parts) settings_path = root / setup.settings_dir / setup.settings_name diff --git a/tests/integration/test_hooks_route.py b/tests/integration/test_hooks_route.py index 8dd97c3825..9f450efd94 100644 --- a/tests/integration/test_hooks_route.py +++ b/tests/integration/test_hooks_route.py @@ -178,7 +178,7 @@ def test_duplicated_globs_and_paths_resolve_quickly_instead_of_blocking( path_count * total_pattern_length is small when every string is one byte) yet, unmatched, cost 25,000,000 real match calls, which measured ~5s of synchronous blocking. Deduplicating at parse time and at the - evidence boundary (domain.policy, PolicyCheckRequest.changed_path_evidence) + evidence boundary (domain.policy, agent_runtime.domain.check.run_policy_check) collapses this to one pattern against one path. The budget below is deliberately far above what the deduplicated work diff --git a/tests/unit/agent_runtime/test_check.py b/tests/unit/agent_runtime/test_check.py new file mode 100644 index 0000000000..491c501dbc --- /dev/null +++ b/tests/unit/agent_runtime/test_check.py @@ -0,0 +1,108 @@ +from gateway.agent_runtime.domain.check import PolicyCheckError, run_policy_check +from gateway.agent_runtime.domain.types import CheckVerdict, JudgeVerdict, Outcome + +_CHANGED_PATH_POLICY = ( + 'schema_version: "1.0"\npolicy:\n id: test\ngates:\n' + " - id: g\n type: changed_path\n enforcement: required\n" + ' forbidden: ["CHANGELOG.md"]\n message: no hand edits\n' +) + +_JUDGE_POLICY = ( + 'schema_version: "1.0"\npolicy:\n id: test\ngates:\n' + " - id: j\n type: judge\n enforcement: advisory\n" + " rubric: does it follow convention\n message: check this\n" +) + +_CHECK_POLICY = ( + 'schema_version: "1.0"\npolicy:\n id: test\ngates:\n' + " - id: c\n type: check_passed\n enforcement: required\n" + " verifier: verify.sh\n message: verifier failed\n" +) + + +def test_a_forbidden_changed_path_blocks() -> None: + result = run_policy_check(_CHANGED_PATH_POLICY, source="test", changed_paths=["CHANGELOG.md"], commands=None) + assert result.policy_id == "test" + assert result.blocked is True + assert result.results[0].outcome is Outcome.FAIL + + +def test_an_unmatched_changed_path_passes_and_does_not_block() -> None: + result = run_policy_check(_CHANGED_PATH_POLICY, source="test", changed_paths=["README.md"], commands=None) + assert result.blocked is False + assert result.results[0].outcome is Outcome.PASS + + +def test_omitted_changed_paths_resolves_unknown_and_blocks() -> None: + """None (never collected) is distinct from [] (collected, and there is none).""" + result = run_policy_check(_CHANGED_PATH_POLICY, source="test", changed_paths=None, commands=None) + assert result.results[0].outcome is Outcome.UNKNOWN + assert result.blocked is True + + +def test_empty_changed_paths_resolves_not_applicable_and_does_not_block() -> None: + result = run_policy_check(_CHANGED_PATH_POLICY, source="test", changed_paths=[], commands=None) + assert result.results[0].outcome is Outcome.NOT_APPLICABLE + assert result.blocked is False + + +def test_duplicate_changed_paths_do_not_change_the_outcome() -> None: + result = run_policy_check( + _CHANGED_PATH_POLICY, source="test", changed_paths=["CHANGELOG.md", "CHANGELOG.md"], commands=None + ) + assert result.results[0].outcome is Outcome.FAIL + + +def test_malformed_policy_raises_policy_check_error() -> None: + try: + run_policy_check("not: valid: yaml: at: all:\n - [", source="test", changed_paths=[], commands=None) + except PolicyCheckError as exc: + assert "not valid YAML" in str(exc) + else: + raise AssertionError("expected PolicyCheckError") + + +def test_oversize_changed_path_entry_raises_policy_check_error() -> None: + try: + run_policy_check(_CHANGED_PATH_POLICY, source="test", changed_paths=["a" * 5000], commands=None) + except PolicyCheckError as exc: + assert "exceeds" in str(exc) + else: + raise AssertionError("expected PolicyCheckError") + + +def test_judge_verdict_is_relayed_into_the_result() -> None: + result = run_policy_check( + _JUDGE_POLICY, + source="test", + changed_paths=None, + commands=None, + judge_results=[JudgeVerdict(gate_id="j", outcome="fail", reasoning="does not follow it")], + ) + assert result.results[0].outcome is Outcome.FAIL + assert result.results[0].detail == "does not follow it" + # advisory: never blocks, regardless of outcome. + assert result.blocked is False + + +def test_omitted_judge_results_resolves_not_applicable() -> None: + result = run_policy_check(_JUDGE_POLICY, source="test", changed_paths=None, commands=None, judge_results=None) + assert result.results[0].outcome is Outcome.NOT_APPLICABLE + + +def test_check_passed_verdict_is_relayed_into_the_result() -> None: + result = run_policy_check( + _CHECK_POLICY, + source="test", + changed_paths=None, + commands=None, + check_results=[CheckVerdict(gate_id="c", outcome="fail", detail="conflict markers found")], + ) + assert result.results[0].outcome is Outcome.FAIL + assert result.blocked is True + + +def test_omitted_check_results_resolves_not_applicable_and_does_not_block() -> None: + result = run_policy_check(_CHECK_POLICY, source="test", changed_paths=None, commands=None, check_results=None) + assert result.results[0].outcome is Outcome.NOT_APPLICABLE + assert result.blocked is False diff --git a/tests/unit/test_hook_cli.py b/tests/unit/test_hook_cli.py index d6f5684da8..b0314c99d4 100644 --- a/tests/unit/test_hook_cli.py +++ b/tests/unit/test_hook_cli.py @@ -246,6 +246,34 @@ def fake_post(url: str, **kwargs: object) -> _FakeResponse: assert captured["json"]["changed_paths"] == ["CHANGELOG.md"] +def test_stop_event_evaluates_locally_and_blocks_on_git_status(monkeypatch: pytest.MonkeyPatch, repo: Path) -> None: + """The default, no-flag path's own full Stop-event pipeline: real Git + + evidence collection feeding the real `run_policy_check`, not a mocked + `httpx.post` standing in for the evaluator. Every other Stop-event test + in this module opts into the remote mode (`--api-key`) and mocks the + network boundary instead; this is the only one that proves the default + path's evidence collection and evaluation are wired together correctly + end to end. + """ + (repo / ".otari-gates.yml").write_text( + 'schema_version: "1.0"\npolicy:\n id: test\ngates:\n' + " - id: g\n type: changed_path\n enforcement: required\n" + ' forbidden: ["CHANGELOG.md"]\n message: forbidden\n', + encoding="utf-8", + ) + + def fake_run(*args: object, **kwargs: object) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess(args=[], returncode=0, stdout=" M CHANGELOG.md\0", stderr="") + + monkeypatch.setattr(subprocess, "run", fake_run) + monkeypatch.setattr(httpx, "post", lambda *a, **k: pytest.fail("httpx.post should not be called")) + payload = {"hook_event_name": "Stop", "cwd": str(repo)} + result = CliRunner().invoke(gateway_cli.hook, [], input=json.dumps(payload)) + assert result.exit_code == 2, result.output + assert "forbidden" in result.output + + def _transcript_line( *, command: str | None = None, text: str | None = None, side_chain: bool = False, tool_use_id: str = "toolu_1" ) -> str: @@ -742,7 +770,63 @@ def test_malformed_stdin_is_a_no_op() -> None: assert result.exit_code == 0, result.output -def test_missing_credential_does_not_block(monkeypatch: pytest.MonkeyPatch, repo: Path, config_stub: None) -> None: +def test_no_flags_evaluates_locally_with_no_credential_needed(monkeypatch: pytest.MonkeyPatch, repo: Path) -> None: + """No `--api-key`/`--url` is the default now, not a missing-setup case: + + `otari hook` evaluates `.otari-gates.yml` in process + (`agent_runtime.domain.check.run_policy_check`) and calls `httpx.post` + only when either flag opts into the other, HTTP-backed mode. A required + gate still blocks with no credential, no config, and no server at all. + """ + + def fail_if_called(*args: object, **kwargs: object) -> None: + raise AssertionError("httpx.post should not be called for the default, local evaluation path") + + monkeypatch.setattr(httpx, "post", fail_if_called) + (repo / ".otari-gates.yml").write_text( + 'schema_version: "1.0"\npolicy:\n id: test\ngates:\n' + " - id: g\n type: changed_path\n enforcement: required\n" + ' forbidden: ["CHANGELOG.md"]\n message: forbidden\n', + encoding="utf-8", + ) + payload = { + "hook_event_name": "PreToolUse", + "cwd": str(repo), + "tool_name": "Edit", + "tool_input": {"file_path": str(repo / "CHANGELOG.md")}, + } + result = CliRunner().invoke(gateway_cli.hook, [], input=json.dumps(payload)) + assert result.exit_code == 2, result.output + assert "forbidden" in result.output + + +def test_malformed_local_policy_does_not_block(monkeypatch: pytest.MonkeyPatch, repo: Path) -> None: + """The local evaluation path's own fail-open contract: a policy + + `run_policy_check` cannot parse must report and exit 0, the same as + every other evidence-collection failure this command handles, not raise. + """ + monkeypatch.setattr(httpx, "post", lambda *a, **k: pytest.fail("httpx.post should not be called")) + (repo / ".otari-gates.yml").write_text("not: valid: yaml: at: all:\n - [", encoding="utf-8") + payload = { + "hook_event_name": "PreToolUse", + "cwd": str(repo), + "tool_name": "Edit", + "tool_input": {"file_path": str(repo / "CHANGELOG.md")}, + } + result = CliRunner().invoke(gateway_cli.hook, [], input=json.dumps(payload)) + assert result.exit_code == 0, result.output + assert "could not evaluate" in result.output + + +def test_url_alone_without_a_resolvable_credential_does_not_block(monkeypatch: pytest.MonkeyPatch, repo: Path) -> None: + """The opt-in remote mode still needs a credential from somewhere: + + `--url` alone opts in, but with no `--api-key` and no configured + `master_key`, there is nothing to authenticate the request with, and + that must fail open rather than block. + """ + def fake_load_config(config_path: str | None = None) -> GatewayConfig: return GatewayConfig(master_key=None) @@ -753,7 +837,7 @@ def fake_load_config(config_path: str | None = None) -> GatewayConfig: "tool_name": "Edit", "tool_input": {"file_path": str(repo / "CHANGELOG.md")}, } - result = CliRunner().invoke(gateway_cli.hook, [], input=json.dumps(payload)) + result = CliRunner().invoke(gateway_cli.hook, ["--url", "http://gw.example:9000"], input=json.dumps(payload)) assert result.exit_code == 0, result.output assert "no API key or master key resolved" in result.output @@ -866,9 +950,15 @@ def fake_post(*args: object, **kwargs: object) -> _FakeResponse: assert "could not reach" in result.output -def test_falls_back_to_configured_master_key_and_localhost( +def test_api_key_alone_opts_into_remote_and_falls_back_to_configured_localhost( monkeypatch: pytest.MonkeyPatch, repo: Path, config_stub: None ) -> None: + """`--api-key` with no `--url` is enough to opt into the HTTP-backed mode: + + the credential is the given one, but the gateway's own URL still falls + back to the configured host/port, exactly as it did before local + evaluation existed. + """ captured: dict[str, Any] = {} def fake_post(url: str, **kwargs: object) -> _FakeResponse: @@ -883,9 +973,37 @@ def fake_post(url: str, **kwargs: object) -> _FakeResponse: "tool_name": "Edit", "tool_input": {"file_path": str(repo / "CHANGELOG.md")}, } - result = CliRunner().invoke(gateway_cli.hook, [], input=json.dumps(payload)) + result = CliRunner().invoke(gateway_cli.hook, ["--api-key", "given-key"], input=json.dumps(payload)) assert result.exit_code == 0, result.output assert captured["url"] == "http://localhost:8000/api/v1/hooks/check" + assert captured["headers"]["Otari-Key"] == "given-key" + + +def test_url_alone_opts_into_remote_and_falls_back_to_configured_master_key( + monkeypatch: pytest.MonkeyPatch, repo: Path, config_stub: None +) -> None: + """`--url` with no `--api-key` is likewise enough to opt in: the URL is + + the given one, but the credential still falls back to the configured + ``master_key``. + """ + captured: dict[str, Any] = {} + + def fake_post(url: str, **kwargs: object) -> _FakeResponse: + captured["url"] = url + captured["headers"] = kwargs.get("headers") + return _FakeResponse({"blocked": False, "results": []}) + + monkeypatch.setattr(httpx, "post", fake_post) + payload = { + "hook_event_name": "PreToolUse", + "cwd": str(repo), + "tool_name": "Edit", + "tool_input": {"file_path": str(repo / "CHANGELOG.md")}, + } + result = CliRunner().invoke(gateway_cli.hook, ["--url", "http://gw.example:9000"], input=json.dumps(payload)) + assert result.exit_code == 0, result.output + assert captured["url"] == "http://gw.example:9000/api/v1/hooks/check" # The bare token, not a ``Bearer `` prefix: deps.extract_credential_token # tolerates the prefix for back-compat, but a header named for the key # carries the raw token. @@ -1003,6 +1121,42 @@ def fake_post(url: str, **kwargs: object) -> _FakeResponse: ] +def test_stop_event_locally_evaluates_a_judge_verdict_and_warns( + monkeypatch: pytest.MonkeyPatch, judge_repo: Path, tmp_path: Path +) -> None: + """The default path's own judge-gate flow, no `httpx.post` mock: the + + locally-collected verdict must reach `run_policy_check` and come back as + an advisory, non-blocking `systemMessage`, not just get built correctly + for a mocked network call (`test_stop_event_submits_a_judge_verdict_from_claude_p` + covers that half already). + """ + + def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + if cmd[:2] == ["git", "status"]: + return subprocess.CompletedProcess(args=cmd, returncode=0, stdout="", stderr="") + if cmd[:2] == ["git", "diff"]: + return subprocess.CompletedProcess(args=cmd, returncode=0, stdout="+ changed line\n", stderr="") + if cmd[0] == "/usr/bin/claude": + return subprocess.CompletedProcess( + args=cmd, returncode=0, stdout=json.dumps({"outcome": "fail", "reasoning": "does not match"}), stderr="" + ) + raise AssertionError(f"unexpected subprocess.run call: {cmd}") + + monkeypatch.setattr(subprocess, "run", fake_run) + monkeypatch.setattr(shutil, "which", lambda name: "/usr/bin/claude" if name == "claude" else None) + monkeypatch.setattr(httpx, "post", lambda *a, **k: pytest.fail("httpx.post should not be called")) + + transcript = tmp_path / "session.jsonl" + transcript.write_text(_transcript_line(text="did some work") + "\n", encoding="utf-8") + + payload = {"hook_event_name": "Stop", "cwd": str(judge_repo), "transcript_path": str(transcript)} + result = CliRunner().invoke(gateway_cli.hook, [], input=json.dumps(payload)) + assert result.exit_code == 0, result.output + stdout_payload = json.loads(result.stdout) + assert "does not match" in stdout_payload["systemMessage"] + + def test_judge_model_is_overridable_via_flag(monkeypatch: pytest.MonkeyPatch, judge_repo: Path) -> None: def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: if cmd[:2] == ["git", "status"]: diff --git a/tests/unit/test_hook_setup_cli.py b/tests/unit/test_hook_setup_cli.py index 34b06068c3..bef2ad4715 100644 --- a/tests/unit/test_hook_setup_cli.py +++ b/tests/unit/test_hook_setup_cli.py @@ -118,18 +118,19 @@ def fail_if_called(config_path: str | None = None) -> GatewayConfig: assert command == f"{_FAKE_OTARI_PATH} hook --harness claude-code --api-key explicit-key" -def test_an_automatically_resolvable_credential_is_not_embedded(repo: Path, monkeypatch: pytest.MonkeyPatch) -> None: - """A key found the same way `otari hook` finds it at runtime is not baked +def test_no_api_key_means_no_credential_resolution_or_prompt(repo: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """With no `--api-key`, the generated command carries no credential at all: - into the generated command: repeating that resolution keeps working - after the key rotates, where an embedded copy of today's value would not. + `otari hook` evaluates the local policy in process by default and needs + neither one, so `setup` must not resolve a `master_key` from config (even + when one is configured) or prompt for anything to get the same result. """ (repo / ".otari-gates.yml").write_text(_CHANGED_PATH_ONLY_GATES, encoding="utf-8") - def fake_load_config(config_path: str | None = None) -> GatewayConfig: - return GatewayConfig(master_key="auto-resolved-key") + def fail_if_called(config_path: str | None = None) -> GatewayConfig: + raise AssertionError("load_config should not be called when --api-key is not given either") - monkeypatch.setattr(gateway_cli, "load_config", fake_load_config) + monkeypatch.setattr(gateway_cli, "load_config", fail_if_called) result = _invoke() # no --api-key, and no prompt input provided: must not be asked for one assert result.exit_code == 0, result.output settings = _read_settings(repo) @@ -137,20 +138,6 @@ def fake_load_config(config_path: str | None = None) -> GatewayConfig: assert command == f"{_FAKE_OTARI_PATH} hook --harness claude-code" -def test_prompts_for_a_credential_when_none_resolves_automatically(repo: Path, monkeypatch: pytest.MonkeyPatch) -> None: - (repo / ".otari-gates.yml").write_text(_CHANGED_PATH_ONLY_GATES, encoding="utf-8") - - def fake_load_config(config_path: str | None = None) -> GatewayConfig: - return GatewayConfig(master_key=None) - - monkeypatch.setattr(gateway_cli, "load_config", fake_load_config) - result = _invoke(input="prompted-key\n") - assert result.exit_code == 0, result.output - settings = _read_settings(repo) - command = settings["hooks"]["PreToolUse"][0]["hooks"][0]["command"] - assert command == f"{_FAKE_OTARI_PATH} hook --harness claude-code --api-key prompted-key" - - def test_rerunning_updates_the_existing_entry_instead_of_duplicating_it(repo: Path) -> None: (repo / ".otari-gates.yml").write_text(_CHANGED_PATH_ONLY_GATES, encoding="utf-8") first = _invoke("--api-key", "first-key") diff --git a/web/src/client/schema.ts b/web/src/client/schema.ts index 6ccbc4a8bd..5101c439a5 100644 --- a/web/src/client/schema.ts +++ b/web/src/client/schema.ts @@ -1265,6 +1265,12 @@ export interface paths { * sent the request, not whether its evidence is true. `blocked` is set when * a required gate's outcome is not `pass`/`not_applicable` (an unresolved * gate never counts as a pass). + * + * The actual parse-and-evaluate work is + * ``agent_runtime.domain.check.run_policy_check``, shared with ``otari + * hook``'s own local evaluation: this route's own job is authentication, + * translating that function's tri-state request fields into its own typed + * ones, and turning ``PolicyCheckError`` into a 422. */ post: operations["hooks-check_policy"]; delete?: never;