From 751ca8c0069c19dd4002965e6f2542bc3ff02794 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Sat, 26 Sep 2026 05:44:17 +0800 Subject: [PATCH 1/2] Make request policy a headless application boundary --- src/infermatrix_copilot/app/core.py | 8 +- src/infermatrix_copilot/app/request_policy.py | 300 +++++++++++++++++ src/infermatrix_copilot/app/run_service.py | 2 +- src/infermatrix_copilot/mcp_policy.py | 316 +----------------- test/test_phase1_cluster.py | 10 +- 5 files changed, 330 insertions(+), 306 deletions(-) create mode 100644 src/infermatrix_copilot/app/request_policy.py diff --git a/src/infermatrix_copilot/app/core.py b/src/infermatrix_copilot/app/core.py index 756f1dc13..45be44ee3 100644 --- a/src/infermatrix_copilot/app/core.py +++ b/src/infermatrix_copilot/app/core.py @@ -556,7 +556,7 @@ def reserve_run(self, spec: TaskSpec, *, owner_server_id: str, freezing it at reservation is what stops two concurrent per-call repos from racing through shared settings, which is how the deleted `configure_strict_repo` worked.""" - from ..mcp_policy import authorize_repo_path + from .request_policy import authorize_repo_path if spec.repo_path: spec = spec.model_copy(update={"repo_path": authorize_repo_path( @@ -621,13 +621,13 @@ def execute_reserved(self, run_id: str) -> int: MCP policy on the persisted request (request.json is untrusted — a host could have rewritten it), plans, then executes, driving `run_status.json` planning -> running -> terminal. Returns the exit code.""" - from ..mcp_policy import enforce_mcp_policy + from .request_policy import enforce_mcp_policy return self._execute_reserved(run_id, enforce_mcp_policy) def execute_strict_reserved(self, run_id: str) -> int: """Execute a reserved Strict review using the previous Eco workflow.""" - from ..mcp_policy import enforce_strict_review_policy + from .request_policy import enforce_strict_review_policy return self._execute_reserved(run_id, enforce_strict_review_policy) @@ -651,7 +651,7 @@ def _execute_reserved(self, run_id: str, policy) -> int: def _execute_reserved_locked(self, run_dir: Path, lock: RunLock, policy) -> int: """The body of `_execute_reserved`, run while holding the run lock.""" - from ..mcp_policy import PolicyError + from .request_policy import PolicyError # The claim, not the lock, is what makes execution at-most-once. # `RunLock` excludes concurrent children; it says nothing about a second diff --git a/src/infermatrix_copilot/app/request_policy.py b/src/infermatrix_copilot/app/request_policy.py new file mode 100644 index 000000000..359761b3d --- /dev/null +++ b/src/infermatrix_copilot/app/request_policy.py @@ -0,0 +1,300 @@ +"""Structural request gates shared by SDK, MCP, and reserved-run execution. + +The MCP transport and embedded SDK accept non-interactive requests without a +human approval prompt. The guarantee *"the host cannot widen a run's +permissions"* must hold **structurally**, and it must not depend on the on-disk +`request.json` being untampered — a same-user host process could rewrite it +between reservation and execution. This gate therefore runs in **two** places +(design rev 5/6): + +- at the **boundary** (SDK or server, when a request is reserved), and +- in the **child** (authoritative), right after it reads `request.json`. + +Either way it re-derives a *safe* `TaskSpec` from raw input, refusing anything +outside the read-only V1 surface. The allowlist of kinds is `READ_ONLY_KINDS` +from `task_spec` verbatim, so the gate can never drift from the code's own +notion of what is read-only. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import Any + +from ..task_spec import FULL_SHA_RE, READ_ONLY_KINDS, TaskSpec + +# params a V1 tool may legitimately carry through to the playbook. Anything else +# in an incoming `params` map is dropped (not an error — stripped) so a tampered +# request can't smuggle a knob (e.g. force_push) into a step. Allowed params are +# strictly value-validated below — a knob may modulate cost/depth, never widen +# permissions. +_ALLOWED_PARAMS: frozenset[str] = frozenset({ + "review_depth", "deterministic_signals", "carried_findings", +}) +_REVIEW_DEPTHS = ("auto", "light", "standard", "full") + +# `expected_head_sha` is deliberately NOT a param. `_ALLOWED_PARAMS` is a security +# allow-set whose job is stopping a tampered request from smuggling a step knob; +# widening it to carry a snapshot binding would weaken exactly that. It is a +# first-class TaskSpec field, validated here and carried through — a gate that +# stripped it would silently unpin the run it was asked to pin. + + +class PolicyError(ValueError): + """Raised when a request cannot be reduced to a safe read-only V1 task.""" + + +def authorize_repo_path(repo: str, raw_path: Any, settings: Any) -> str: + """Canonicalize and authorize a caller-supplied checkout for `repo`, or "". + + Two checks, both required, because either alone leaves a real hole: + + 1. **Identity** — the checkout's `origin` must be the alias's configured + GitHub identity. A root allowlist alone would let an allowed alias be + paired with a *different* checkout sitting under the same root, and the + run would review the wrong repository entirely while every other guard + said yes. + 2. **Containment** — the canonical path must sit under an allowed root, so a + genuine clone of the right repository in an arbitrary location is still + refused. Which clone a bot may point at is an operator's decision. + + Fails closed: an unverifiable identity (no configured full name and no + parseable origin) is refused rather than assumed.""" + if raw_path in (None, ""): + return "" + if not isinstance(raw_path, str): + raise PolicyError( + f"repo_path must be a string, got {type(raw_path).__name__}") + if settings is None: + raise PolicyError("repo_path cannot be authorized without settings") + + path = Path(raw_path).expanduser().resolve() + if not path.is_dir() or not (path / ".git").exists(): + raise PolicyError(f"repo_path is not a Git checkout: {path}") + + from ..intent import _remote_full_name, repo_identity + + want = repo_identity(repo, settings) + if not want: + raise PolicyError( + f"repo {repo!r} has no known GitHub identity (set REPO_FULL_NAMES " + "or configure its checkout), so an explicit repo_path cannot be " + "verified to belong to it") + got = _remote_full_name(str(path)) + if not got: + raise PolicyError( + f"repo_path {path} has no parseable origin remote, so it cannot be " + f"verified as a checkout of {want}") + if got.lower() != str(want).lower(): + raise PolicyError( + f"repo_path {path} is a checkout of {got}, not {want}") + + roots = [str(Path(r).expanduser().resolve()) + for r in (settings.allowed_repo_roots or [])] + if not any(str(path) == r or str(path).startswith(r + os.sep) + for r in roots): + raise PolicyError( + f"repo_path {path} is outside the allowed roots {sorted(roots)}; " + "set MCP_ALLOWED_REPO_ROOTS to permit it") + return str(path) + + +def enforce_mcp_policy(raw: dict[str, Any], *, allowed_repos: list[str], + settings: Any = None) -> TaskSpec: + """Reduce `raw` (an untrusted tool-call / `request.json` dict) to a safe, + read-only `TaskSpec`, or raise `PolicyError`. + + Structural guarantees, independent of what `raw` claims: + - `kind` MUST be one of `READ_ONLY_KINDS` (pr_review / issue_answer / + issue_filter). Any write/push-capable kind (rebase, debug, profile) is + refused — this is what makes the surface read-only regardless of the file. + - `post` is forced False (no outward writes over MCP, ever, in V1). + - `repo` MUST be in `allowed_repos`. + - `pr` / `issue`, when present, MUST be positive integers. + - `params` is stripped to the allow-set; the quality-only bounded signal list + can guide assessment but cannot grant a step knob or write authority. + - `expected_head_sha`, when present, MUST be a full 40-hex commit id, and is + **carried through** rather than stripped: it only ever narrows the run + (review this head or stop as stale), so dropping it would silently unpin + a request that asked to be pinned. + """ + if not isinstance(raw, dict): + raise PolicyError("request is not an object") + + kind = raw.get("kind") + if kind not in READ_ONLY_KINDS: + raise PolicyError( + f"kind {kind!r} is not permitted over MCP; allowed: " + f"{sorted(READ_ONLY_KINDS)}") + + repo = raw.get("repo") + if isinstance(repo, str) and "/" in repo and settings is not None: + # full `owner/repo` form: resolve through the same identity validator + # the CLI uses, so aliases mean the same thing on every surface + from ..intent import resolve_repo_alias + + owner, _, name = repo.partition("/") + alias = resolve_repo_alias(owner, name, settings) + if alias is None: + raise PolicyError(f"repo {repo!r} does not match any configured repo") + repo = alias + if repo not in allowed_repos: + raise PolicyError( + f"repo {repo!r} not in the MCP allowlist {sorted(allowed_repos)}") + + pr = _positive_or_none(raw.get("pr"), "pr") + issue = _positive_or_none(raw.get("issue"), "issue") + + raw_params = raw.get("params") or {} + if not isinstance(raw_params, dict): + raise PolicyError("params must be an object") + params = {k: v for k, v in raw_params.items() if k in _ALLOWED_PARAMS} + if "review_depth" in params: # strict: a typo must not silently pass + if str(params["review_depth"]).lower() not in _REVIEW_DEPTHS: + raise PolicyError( + f"review_depth {params['review_depth']!r} is not one of " + f"{list(_REVIEW_DEPTHS)}") + params["review_depth"] = str(params["review_depth"]).lower() + if "carried_findings" in params: + if kind != "pr_review": + params.pop("carried_findings") + else: + from ..sdk.v1.rechecks import validate_carried + from ..sdk.v1.models import InvalidRequestError + try: + validate_carried(params["carried_findings"]) + except InvalidRequestError as exc: + raise PolicyError(str(exc)) from exc + if "deterministic_signals" in params: + if kind != "pr_quality": + params.pop("deterministic_signals") + else: + signals = params["deterministic_signals"] + if not isinstance(signals, list) or not all( + isinstance(item, str) for item in signals): + raise PolicyError("deterministic_signals must be a list of strings") + if len(signals) > 12 or any(len(item) > 500 for item in signals): + raise PolicyError( + "deterministic_signals must contain at most 12 items of " + "at most 500 characters each") + params["deterministic_signals"] = [item.strip() for item in signals] + + mode = raw.get("mode", "eco") + if mode not in {"eco", "performance"}: + raise PolicyError( + f"mode {mode!r} is invalid; allowed: ['eco', 'performance']") + + expected_head_sha = _full_sha_or_empty(raw.get("expected_head_sha")) + # Re-authorized here on BOTH passes by design: the boundary check binds the + # request, and the child's check re-derives it from `request.json`, which is + # untrusted — a same-user host could rewrite the path between reservation + # and execution. + repo_path = authorize_repo_path(repo, raw.get("repo_path"), settings) + + # Build through the validated model. post is hard-forced False; report_only + # is irrelevant for READ_ONLY_KINDS (they are read-only unless post), but we + # normalize it off too for a clean record. + return TaskSpec(kind=kind, mode=mode, repo=repo, pr=pr, issue=issue, + report_only=False, post=False, params=params, + expected_head_sha=expected_head_sha, repo_path=repo_path) + + +def enforce_strict_review_policy(raw: dict[str, Any], *, + allowed_repos: list[str], + settings: Any = None) -> TaskSpec: + """Validate the Direct MCP's Strict compatibility path. + + Strict is the public name for the previous Eco PR-review workflow. It cannot + select the performance tier or widen the task beyond ``pr_review``, and it + cannot post. `expected_head_sha` rides through the shared gate below, so the + snapshot binding survives on this path too. + + **`post` is refused, not restored.** This path used to force `post=False` + for the shared gate and then put the caller's value back, making Strict the + one MCP surface that could publish. A single publisher is a design + requirement — two publishers mean two review markers and two head gates on + one PR — and a capability flag advertising `supports_post_false` would not + have stopped a caller from passing `post=true`. Removing the restore does + not create an anomaly, it removes one: `enforce_mcp_policy` already + hard-forces `post=False` for every other kind, and Strict was the outlier. + Human-driven posting is unaffected; it lives on the CLI (`--yes` with + `ALLOW_POST=1`), which is the surface that has a human on it. + + An explicit `post=true` is an error rather than a silent drop, so a caller + that believes it is publishing finds out here instead of from the absence of + a comment.""" + if not isinstance(raw, dict): + raise PolicyError("request is not an object") + if raw.get("kind") != "pr_review": + raise PolicyError("strict mode only permits PR reviews") + + post = raw.get("post", False) + if not isinstance(post, bool): + raise PolicyError("post must be a boolean") + if post: + raise PolicyError( + "strict mode cannot post: the MCP surface is never the publisher, " + "so exactly one publisher owns a PR's review marker and head gate. " + "Read the structured result and publish it yourself, or use the CLI " + "with ALLOW_POST=1 for a human-driven post.") + + normalized = dict(raw) + normalized["mode"] = "eco" + normalized["post"] = False + return enforce_mcp_policy( + normalized, allowed_repos=allowed_repos, settings=settings) + + +def enforce_quality_review_policy(raw: dict[str, Any], *, + allowed_repos: list[str], + settings: Any = None) -> TaskSpec: + """Validate the dedicated read-only PR quality workflow. + + The caller may provide bounded deterministic signals as hypotheses for the + model to verify. They never widen permissions and are not themselves a + quality verdict. Posting remains structurally impossible. + """ + if not isinstance(raw, dict): + raise PolicyError("request is not an object") + if raw.get("kind") != "pr_quality": + raise PolicyError("quality mode only permits PR quality assessments") + if raw.get("post", False): + raise PolicyError("quality mode cannot post; the caller owns publishing") + normalized = dict(raw) + normalized["mode"] = "eco" + normalized["post"] = False + return enforce_mcp_policy( + normalized, allowed_repos=allowed_repos, settings=settings) + + +def _full_sha_or_empty(value: Any) -> str: + """Coerce an optional `expected_head_sha` to "" or a full 40-hex sha. + + A prefix is refused rather than normalized: the only caller that pins a head + (the reviewbot) holds the full sha from the GitHub API, and accepting a + prefix would mean comparing it against a full sha somewhere downstream.""" + if value in (None, ""): + return "" + if not isinstance(value, str): + raise PolicyError( + f"expected_head_sha must be a string, got {type(value).__name__}") + sha = value.strip().lower() + if not FULL_SHA_RE.match(sha): + raise PolicyError( + "expected_head_sha must be exactly 40 hex chars (the full commit " + f"id), got {value!r}") + return sha + + +def _positive_or_none(value: Any, field: str) -> int | None: + """Coerce `value` to a positive int, allow None/absent, else raise.""" + if value is None: + return None + try: + n = int(value) + except (TypeError, ValueError): + raise PolicyError(f"{field} must be an integer, got {value!r}") + if n <= 0: + raise PolicyError(f"{field} must be positive, got {n}") + return n diff --git a/src/infermatrix_copilot/app/run_service.py b/src/infermatrix_copilot/app/run_service.py index cd6d74014..e756e0d15 100644 --- a/src/infermatrix_copilot/app/run_service.py +++ b/src/infermatrix_copilot/app/run_service.py @@ -21,7 +21,7 @@ from .. import run_status as rs from ..config import Settings from ..knowledge_docs import KnowledgeDocs, KnowledgeDocsError -from ..mcp_policy import ( +from .request_policy import ( PolicyError, authorize_repo_path, enforce_mcp_policy, enforce_quality_review_policy, enforce_strict_review_policy, ) diff --git a/src/infermatrix_copilot/mcp_policy.py b/src/infermatrix_copilot/mcp_policy.py index 0a111f707..420a10dec 100644 --- a/src/infermatrix_copilot/mcp_policy.py +++ b/src/infermatrix_copilot/mcp_policy.py @@ -1,301 +1,21 @@ -"""`enforce_mcp_policy` — the structural safety gate for the MCP surface. +"""Compatibility imports for the original MCP policy module. -The MCP server exposes the copilot to Claude Code / Codex, which are -non-interactive: there is no `[y/N]` and no human in the loop to approve an -outward write. So the guarantee *"the host cannot widen the server's -permissions"* must hold **structurally**, and it must not depend on the on-disk -`request.json` being untampered — a same-user host process could rewrite it -between reservation and execution. This gate therefore runs in **two** places -(design rev 5/6): - -- at the **boundary** (the server, when a tool is called), and -- in the **child** (authoritative), right after it reads `request.json`. - -Either way it re-derives a *safe* `TaskSpec` from raw input, refusing anything -outside the read-only V1 surface. The allowlist of kinds is `READ_ONLY_KINDS` -from `task_spec` verbatim, so the gate can never drift from the code's own -notion of what is read-only. +Request validation is owned by the headless application because the SDK and +reserved-run worker apply the same gates as the MCP transport. """ -from __future__ import annotations - -import os -from pathlib import Path -from typing import Any - -from .task_spec import FULL_SHA_RE, READ_ONLY_KINDS, TaskSpec - -# params a V1 tool may legitimately carry through to the playbook. Anything else -# in an incoming `params` map is dropped (not an error — stripped) so a tampered -# request can't smuggle a knob (e.g. force_push) into a step. Allowed params are -# strictly value-validated below — a knob may modulate cost/depth, never widen -# permissions. -_ALLOWED_PARAMS: frozenset[str] = frozenset({ - "review_depth", "deterministic_signals", "carried_findings", -}) -_REVIEW_DEPTHS = ("auto", "light", "standard", "full") - -# `expected_head_sha` is deliberately NOT a param. `_ALLOWED_PARAMS` is a security -# allow-set whose job is stopping a tampered request from smuggling a step knob; -# widening it to carry a snapshot binding would weaken exactly that. It is a -# first-class TaskSpec field, validated here and carried through — a gate that -# stripped it would silently unpin the run it was asked to pin. - - -class PolicyError(ValueError): - """Raised when a request cannot be reduced to a safe read-only V1 task.""" - - -def authorize_repo_path(repo: str, raw_path: Any, settings: Any) -> str: - """Canonicalize and authorize a caller-supplied checkout for `repo`, or "". - - Two checks, both required, because either alone leaves a real hole: - - 1. **Identity** — the checkout's `origin` must be the alias's configured - GitHub identity. A root allowlist alone would let an allowed alias be - paired with a *different* checkout sitting under the same root, and the - run would review the wrong repository entirely while every other guard - said yes. - 2. **Containment** — the canonical path must sit under an allowed root, so a - genuine clone of the right repository in an arbitrary location is still - refused. Which clone a bot may point at is an operator's decision. - - Fails closed: an unverifiable identity (no configured full name and no - parseable origin) is refused rather than assumed.""" - if raw_path in (None, ""): - return "" - if not isinstance(raw_path, str): - raise PolicyError( - f"repo_path must be a string, got {type(raw_path).__name__}") - if settings is None: - raise PolicyError("repo_path cannot be authorized without settings") - - path = Path(raw_path).expanduser().resolve() - if not path.is_dir() or not (path / ".git").exists(): - raise PolicyError(f"repo_path is not a Git checkout: {path}") - - from .intent import _remote_full_name, repo_identity - - want = repo_identity(repo, settings) - if not want: - raise PolicyError( - f"repo {repo!r} has no known GitHub identity (set REPO_FULL_NAMES " - "or configure its checkout), so an explicit repo_path cannot be " - "verified to belong to it") - got = _remote_full_name(str(path)) - if not got: - raise PolicyError( - f"repo_path {path} has no parseable origin remote, so it cannot be " - f"verified as a checkout of {want}") - if got.lower() != str(want).lower(): - raise PolicyError( - f"repo_path {path} is a checkout of {got}, not {want}") - - roots = [str(Path(r).expanduser().resolve()) - for r in (settings.allowed_repo_roots or [])] - if not any(str(path) == r or str(path).startswith(r + os.sep) - for r in roots): - raise PolicyError( - f"repo_path {path} is outside the allowed roots {sorted(roots)}; " - "set MCP_ALLOWED_REPO_ROOTS to permit it") - return str(path) - - -def enforce_mcp_policy(raw: dict[str, Any], *, allowed_repos: list[str], - settings: Any = None) -> TaskSpec: - """Reduce `raw` (an untrusted tool-call / `request.json` dict) to a safe, - read-only `TaskSpec`, or raise `PolicyError`. - - Structural guarantees, independent of what `raw` claims: - - `kind` MUST be one of `READ_ONLY_KINDS` (pr_review / issue_answer / - issue_filter). Any write/push-capable kind (rebase, debug, profile) is - refused — this is what makes the surface read-only regardless of the file. - - `post` is forced False (no outward writes over MCP, ever, in V1). - - `repo` MUST be in `allowed_repos`. - - `pr` / `issue`, when present, MUST be positive integers. - - `params` is stripped to the allow-set; the quality-only bounded signal list - can guide assessment but cannot grant a step knob or write authority. - - `expected_head_sha`, when present, MUST be a full 40-hex commit id, and is - **carried through** rather than stripped: it only ever narrows the run - (review this head or stop as stale), so dropping it would silently unpin - a request that asked to be pinned. - """ - if not isinstance(raw, dict): - raise PolicyError("request is not an object") - - kind = raw.get("kind") - if kind not in READ_ONLY_KINDS: - raise PolicyError( - f"kind {kind!r} is not permitted over MCP; allowed: " - f"{sorted(READ_ONLY_KINDS)}") - - repo = raw.get("repo") - if isinstance(repo, str) and "/" in repo and settings is not None: - # full `owner/repo` form: resolve through the same identity validator - # the CLI uses, so aliases mean the same thing on every surface - from .intent import resolve_repo_alias - - owner, _, name = repo.partition("/") - alias = resolve_repo_alias(owner, name, settings) - if alias is None: - raise PolicyError(f"repo {repo!r} does not match any configured repo") - repo = alias - if repo not in allowed_repos: - raise PolicyError( - f"repo {repo!r} not in the MCP allowlist {sorted(allowed_repos)}") - - pr = _positive_or_none(raw.get("pr"), "pr") - issue = _positive_or_none(raw.get("issue"), "issue") - - raw_params = raw.get("params") or {} - if not isinstance(raw_params, dict): - raise PolicyError("params must be an object") - params = {k: v for k, v in raw_params.items() if k in _ALLOWED_PARAMS} - if "review_depth" in params: # strict: a typo must not silently pass - if str(params["review_depth"]).lower() not in _REVIEW_DEPTHS: - raise PolicyError( - f"review_depth {params['review_depth']!r} is not one of " - f"{list(_REVIEW_DEPTHS)}") - params["review_depth"] = str(params["review_depth"]).lower() - if "carried_findings" in params: - if kind != "pr_review": - params.pop("carried_findings") - else: - from .sdk.v1.rechecks import validate_carried - from .sdk.v1.models import InvalidRequestError - try: - validate_carried(params["carried_findings"]) - except InvalidRequestError as exc: - raise PolicyError(str(exc)) from exc - if "deterministic_signals" in params: - if kind != "pr_quality": - params.pop("deterministic_signals") - else: - signals = params["deterministic_signals"] - if not isinstance(signals, list) or not all( - isinstance(item, str) for item in signals): - raise PolicyError("deterministic_signals must be a list of strings") - if len(signals) > 12 or any(len(item) > 500 for item in signals): - raise PolicyError( - "deterministic_signals must contain at most 12 items of " - "at most 500 characters each") - params["deterministic_signals"] = [item.strip() for item in signals] - - mode = raw.get("mode", "eco") - if mode not in {"eco", "performance"}: - raise PolicyError( - f"mode {mode!r} is invalid; allowed: ['eco', 'performance']") - - expected_head_sha = _full_sha_or_empty(raw.get("expected_head_sha")) - # Re-authorized here on BOTH passes by design: the boundary check binds the - # request, and the child's check re-derives it from `request.json`, which is - # untrusted — a same-user host could rewrite the path between reservation - # and execution. - repo_path = authorize_repo_path(repo, raw.get("repo_path"), settings) - - # Build through the validated model. post is hard-forced False; report_only - # is irrelevant for READ_ONLY_KINDS (they are read-only unless post), but we - # normalize it off too for a clean record. - return TaskSpec(kind=kind, mode=mode, repo=repo, pr=pr, issue=issue, - report_only=False, post=False, params=params, - expected_head_sha=expected_head_sha, repo_path=repo_path) - - -def enforce_strict_review_policy(raw: dict[str, Any], *, - allowed_repos: list[str], - settings: Any = None) -> TaskSpec: - """Validate the Direct MCP's Strict compatibility path. - - Strict is the public name for the previous Eco PR-review workflow. It cannot - select the performance tier or widen the task beyond ``pr_review``, and it - cannot post. `expected_head_sha` rides through the shared gate below, so the - snapshot binding survives on this path too. - - **`post` is refused, not restored.** This path used to force `post=False` - for the shared gate and then put the caller's value back, making Strict the - one MCP surface that could publish. A single publisher is a design - requirement — two publishers mean two review markers and two head gates on - one PR — and a capability flag advertising `supports_post_false` would not - have stopped a caller from passing `post=true`. Removing the restore does - not create an anomaly, it removes one: `enforce_mcp_policy` already - hard-forces `post=False` for every other kind, and Strict was the outlier. - Human-driven posting is unaffected; it lives on the CLI (`--yes` with - `ALLOW_POST=1`), which is the surface that has a human on it. - - An explicit `post=true` is an error rather than a silent drop, so a caller - that believes it is publishing finds out here instead of from the absence of - a comment.""" - if not isinstance(raw, dict): - raise PolicyError("request is not an object") - if raw.get("kind") != "pr_review": - raise PolicyError("strict mode only permits PR reviews") - - post = raw.get("post", False) - if not isinstance(post, bool): - raise PolicyError("post must be a boolean") - if post: - raise PolicyError( - "strict mode cannot post: the MCP surface is never the publisher, " - "so exactly one publisher owns a PR's review marker and head gate. " - "Read the structured result and publish it yourself, or use the CLI " - "with ALLOW_POST=1 for a human-driven post.") - - normalized = dict(raw) - normalized["mode"] = "eco" - normalized["post"] = False - return enforce_mcp_policy( - normalized, allowed_repos=allowed_repos, settings=settings) - - -def enforce_quality_review_policy(raw: dict[str, Any], *, - allowed_repos: list[str], - settings: Any = None) -> TaskSpec: - """Validate the dedicated read-only PR quality workflow. - - The caller may provide bounded deterministic signals as hypotheses for the - model to verify. They never widen permissions and are not themselves a - quality verdict. Posting remains structurally impossible. - """ - if not isinstance(raw, dict): - raise PolicyError("request is not an object") - if raw.get("kind") != "pr_quality": - raise PolicyError("quality mode only permits PR quality assessments") - if raw.get("post", False): - raise PolicyError("quality mode cannot post; the caller owns publishing") - normalized = dict(raw) - normalized["mode"] = "eco" - normalized["post"] = False - return enforce_mcp_policy( - normalized, allowed_repos=allowed_repos, settings=settings) - - -def _full_sha_or_empty(value: Any) -> str: - """Coerce an optional `expected_head_sha` to "" or a full 40-hex sha. - - A prefix is refused rather than normalized: the only caller that pins a head - (the reviewbot) holds the full sha from the GitHub API, and accepting a - prefix would mean comparing it against a full sha somewhere downstream.""" - if value in (None, ""): - return "" - if not isinstance(value, str): - raise PolicyError( - f"expected_head_sha must be a string, got {type(value).__name__}") - sha = value.strip().lower() - if not FULL_SHA_RE.match(sha): - raise PolicyError( - "expected_head_sha must be exactly 40 hex chars (the full commit " - f"id), got {value!r}") - return sha - - -def _positive_or_none(value: Any, field: str) -> int | None: - """Coerce `value` to a positive int, allow None/absent, else raise.""" - if value is None: - return None - try: - n = int(value) - except (TypeError, ValueError): - raise PolicyError(f"{field} must be an integer, got {value!r}") - if n <= 0: - raise PolicyError(f"{field} must be positive, got {n}") - return n +from .app.request_policy import ( + PolicyError, + authorize_repo_path, + enforce_mcp_policy, + enforce_quality_review_policy, + enforce_strict_review_policy, +) + +__all__ = [ + "PolicyError", + "authorize_repo_path", + "enforce_mcp_policy", + "enforce_quality_review_policy", + "enforce_strict_review_policy", +] diff --git a/test/test_phase1_cluster.py b/test/test_phase1_cluster.py index 49116c4da..7eddeeb0a 100644 --- a/test/test_phase1_cluster.py +++ b/test/test_phase1_cluster.py @@ -816,10 +816,14 @@ def test_drift_guard_unimportable_checked_class_fails_guard( drift_guard, tmp_path, monkeypatch, capsys): """An unimportable constructor-checked class IS drift (upstream moved/renamed it) and must fail the guard — the silent SKIP is exactly - how the dead pooling entry went unnoticed in the parent. Offline (no - vllm installed) every constructor check import-fails, so main() must - return 1 with a MISMATCH per entry.""" + how the dead pooling entry went unnoticed in the parent. Force the + import failure so this assertion also holds in an environment that has + vllm installed.""" monkeypatch.chdir(tmp_path) # no tests/ dir; nothing else to scan + def missing_class(module, name): + raise ImportError(f"{module}.{name} is unavailable") + + monkeypatch.setattr(drift_guard, "import_class", missing_class) rc = drift_guard.main() out = capsys.readouterr().out assert rc == 1 From dece2676d2e29c4403d618d54cf9c5bfe249c7c2 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Sat, 26 Sep 2026 05:47:30 +0800 Subject: [PATCH 2/2] Document application policy ownership and MCP compatibility --- doc/architecture/SPEC/app/request_policy.md | 28 ++++++++ doc/architecture/SPEC/mcp_policy.md | 75 +++------------------ 2 files changed, 38 insertions(+), 65 deletions(-) create mode 100644 doc/architecture/SPEC/app/request_policy.md diff --git a/doc/architecture/SPEC/app/request_policy.md b/doc/architecture/SPEC/app/request_policy.md new file mode 100644 index 000000000..c7011ccca --- /dev/null +++ b/doc/architecture/SPEC/app/request_policy.md @@ -0,0 +1,28 @@ +# app/request_policy.py — request safety policy + + + +This headless application module validates untrusted read-only requests for +the SDK, MCP transport, and reserved-run worker. It derives a new `TaskSpec` +from raw input at reservation and repeats validation in the child after +reading `request.json`; persisted input alone is not an authority. + +`enforce_mcp_policy` permits only imported `READ_ONLY_KINDS`, forces +`post=False`, limits repositories to the configured allowlist, checks positive +PR/issue numbers, validates full expected-head SHAs, and strips unknown +params. A bounded `deterministic_signals` list applies only to quality review; +carried findings are validated for PR review. `authorize_repo_path` verifies +both checkout origin identity and containment under allowed roots. + +`enforce_strict_review_policy` accepts only PR review, forces eco mode, and +rejects explicit posting. `enforce_quality_review_policy` accepts only PR +quality, forces eco mode, and rejects posting. Neither path can create an +outward write. Policy errors are `PolicyError`; the module does not execute a +run, call a model, or access knowledge. It imports only task-specification +primitives at module load; repository identity and carried-finding helpers +are loaded inside the relevant validation path. + +The old `infermatrix_copilot.mcp_policy` module re-exports these callables for +compatibility. Keep policy changes here so the application and transports +cannot diverge. `test_mcp.py`, `test_sdk_v1.py`, and `test_contract.py` cover +the read-only, snapshot, identity, and compatibility behavior. diff --git a/doc/architecture/SPEC/mcp_policy.md b/doc/architecture/SPEC/mcp_policy.md index b3034cc41..7e10dd515 100644 --- a/doc/architecture/SPEC/mcp_policy.md +++ b/doc/architecture/SPEC/mcp_policy.md @@ -1,68 +1,13 @@ -# mcp_policy.py —— 规范 +# mcp_policy.py — compatibility boundary - + -`LOC ~254 · 安全原语（MCP 结构性门） · refactor-status: ok` +This module preserves the existing imports for `PolicyError`, +`authorize_repo_path`, `enforce_mcp_policy`, +`enforce_strict_review_policy`, and `enforce_quality_review_policy`. +It contains no independent validation rules. The headless application owns +those rules in [`app/request_policy.md`](app/request_policy.md); SDK, MCP, +and reserved-run execution use the same implementation. -## 职责 -从原始 MCP 输入**重新推导**出一个*安全的* `TaskSpec`，拒绝 MCP 面不被允许做的任何事 -—— 这就是"宿主无法扩大服务端权限"的**结构性**保证。 - -## 公开契约 -`enforce_mcp_policy(raw) -> TaskSpec`（拒绝时抛错）； -`enforce_strict_review_policy(raw) -> TaskSpec`（Strict 兼容路径：限 -`pr_review`、强制 `mode="eco"`）； -`enforce_quality_review_policy(raw) -> TaskSpec`（质量路径：限 -`pr_quality`、强制 `mode="eco"`、永不发布）； -`authorize_repo_path(repo, raw_path, settings) -> str`（对调用方提供的 -checkout 路径做**身份 + 圈禁**双重校验，返回 canonical 路径）。 - -## 不变量（**C2**、**C3**、**A2**） -- **它跑两次，这是设计如此。** 一次在**边界**（server，工具被调用时），一次在 - **子进程**（**权威**，就在它读完 `request.json` 之后）。子进程这次复检之所以存在， - 是因为这条保证**不能依赖 `request.json` 未被篡改**：同用户的宿主进程完全可以在预约 - 与执行之间把它改写。 -- `kind` 必须 ∈ `READ_ONLY_KINDS`；`post` 被**硬置为 `False`**；`repo` 必须在 allowlist - 内；`pr`/`issue` 必须为正；未知 params 被**剥除**而不是透传。 -- `pr_quality` 唯一可多带一个 `deterministic_signals`：至多 12 条、每条至多 - 500 字符。它们只是供模型核验的可错假设，不能直接成为质量裁决。 -- **Strict 路径上 `post` 是被拒绝，不是被恢复。** 这条路径曾经"共享门里 - 强制 `post=False`、事后把调用方原值放回去"，使 Strict 成为唯一能发布的 - MCP 面。显式 `post=True` 现在直接抛 `PolicyError` —— 移除这个恢复不是 - 制造异常，而是消灭一个异常（`mcp_server` 侧的 `ALLOW_POST="0"` 环境闸 - 与此呼应，双闸同向）。 -- **`expected_head_sha` 只收全长 SHA**：经 `task_spec.FULL_SHA_RE` - （import，绝不重述正则）校验后随 `TaskSpec` 冻结；短 SHA / 非十六进制 - 被拒绝，空串 = 不钉快照。 -- **`authorize_repo_path` 双重校验、fail-closed**：(1) **身份** —— - checkout 的 `origin` remote 必须解析为该 alias 配置的 GitHub 全名 - （经 `intent` 的 remote/identity helper）；(2) **圈禁** —— canonical - 路径必须落在 `settings.allowed_repo_roots` 之下（默认 = 只有已配置的 - checkout 本身，最小权限）。身份无法验证 = 拒绝，绝不假定。它是 - `TaskSpec.repo_path` 的一等入口 —— **不要**把它塞进 `_ALLOWED_PARAMS`。 -- **允许集是 import 进来的，绝不重述。** 它直接引用 `task_spec.READ_ONLY_KINDS`， - 因此策略永远不会与任务模型漂移 —— 新增一个具备写能力的 kind，**无法**静默地把 MCP - 面放宽。 -- MCP 宿主是**非交互**的：这里没有 `[y/N]`，所以本文件里任何逻辑都不得退化成"问用户"。 - -## 边界 —— 不属于这里 -不执行 run；不调模型；不访问知识。 - -## 依赖（允许） -`..task_spec` + stdlib；外加 `authorize_repo_path` 内部**函数级**引入的 -`.intent`（remote→GitHub 身份解析）—— 与 `config.md` 记录函数级 import 的 -方式一致的刻意例外：身份判定必须与 intent 的 URL 路由用同一套解析， -否则两处各养一份就会漂移。模块导入期仍然只依赖 task_spec + stdlib。 - -## 测试 -`test_mcp.py`（篡改防御、只读工具集、`expected_head_sha` 贯穿与短 SHA 拒绝、 -Strict 显式 post 被拒）；`test_contract.py`（repo_path 身份不符 / 圈外 / -身份不可验证均拒绝；冻结 repo_path 直达解析与执行、压过环境路径）。 - -## 重构备注 -和 `push.guard_push`、`scopes` 一样，这是一个纯权限原语 —— 保持它无依赖。任何新的 -"MCP 可达能力"都必须表达为对 `READ_ONLY_KINDS` 的修改，**而不是这里的一个特例**。 - -## Carried finding rechecks - -Direct 1.1 / Strict 1.3 accept typed `CarriedFinding` inputs (unique IDs, source head, severity, location, title and evidence). Direct binds the carried set into its issued context and validates `FindingRecheck` answers. Strict passes the set through the policy allowlist, performs a read-only recheck against the frozen PR head, and persists answers for the public structured result. Every carried ID needs an explicit outcome: `fixed`, `still_affected`, or `unverified`. Missing evidence is never a fix. +Do not add a second policy decision here. Existing callers importing this +module retain the same callable objects and exception type.