From 33e08be3d8b3d7bf2c8f3ad4f4ae9205cc1208d9 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Mon, 20 Jul 2026 14:55:14 -0700 Subject: [PATCH 1/2] Allow agents to author safe coverage proposals --- CHANGELOG.md | 9 + STABILITY.md | 18 +- docs/agent-contract-current.md | 10 +- docs/quickstart.md | 9 + llms-full.txt | 10 +- src/agents_shipgate/cli/agent_result.py | 31 ++- src/agents_shipgate/cli/preflight.py | 13 +- src/agents_shipgate/core/agent_boundary.py | 4 +- src/agents_shipgate/core/codex_boundary.py | 24 ++ .../core/manifest_proposals.py | 219 ++++++++++++++++++ src/agents_shipgate/core/preflight.py | 81 ++++++- tests/test_codex_boundary_check.py | 109 +++++++++ tests/test_preflight.py | 212 +++++++++++++++++ 13 files changed, 727 insertions(+), 22 deletions(-) create mode 100644 src/agents_shipgate/core/manifest_proposals.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 445ae732..797ade7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,15 @@ remains reproducibility provenance, but cannot extend reviewer-owned trust. Baseline, acknowledgement, and severity-override expiry use the later of that date and the verifier wall clock, so a forged backdated commit fails closed. +- **Agent-authored coverage proposals (contract v18 clarification).** Preflight + and local control now distinguish proposal authorship from approval for one + narrow manifest shape: an exact append-only addition of valid built-in + `tool_sources` rows may be authored by a coding agent and routed to verify. + Existing rows, all other manifest values, authority-bearing fields, custom + adapters, and unsafe paths remain human-routed; the concrete trust-root diff + still requires reviewer approval. Conventional test/golden fixtures are no + longer inferred as undeclared deployed surfaces unless the manifest + explicitly declares them. No schema or runtime-contract version changes. - **Evidence-basis policy gate (P0, `0.16.0b5`).** Semantic claims and risk hints now carry a typed evidence basis, stable claim IDs, and derived policy diff --git a/STABILITY.md b/STABILITY.md index 25f209f1..22f884e8 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -628,9 +628,12 @@ The stable top-level fields in the v0.3 preflight result are: - `required_evidence[]` — deterministic evidence requirements for a `--capability-request` high-risk action proposal. - `changed_files[]` and `protected_surface_touches[]` — optional path review - projection from `--changed-files` and/or `--diff`. -- `requires_human_review` — true when the requested preflight input touches a - protected surface or lacks high/critical required evidence. + projection from `--changed-files` and/or `--diff`. A touch's existing + `requires_human_review` flag may be false only for a resolvable, append-only + proposal that adds valid built-in `tool_sources` coverage without changing + any other manifest value or existing source row. +- `requires_human_review` — true when a requested protected touch requires + pre-edit human routing or the plan lacks high/critical required evidence. - `policy_snapshot_hash`, `trust_root_graph_hash`, and `trust_root_graph` — deterministic hashes/projection for policy and trust-root drift review. - `policy_drift` and `trust_root_graph_diff` — populated when @@ -646,6 +649,15 @@ The stable top-level fields in the v0.3 preflight result are: - `host_grant_drift` — optional host-grant drift payload when a host baseline is present or explicitly supplied. +Preflight distinguishes proposal authorship from approval. A coding agent may +author the exact coverage-increasing manifest proposal described above and is +then routed to `verify`; path-only plans, custom adapters, `trust`/`optional` +fields, source edits/removals/reordering, non-contained or symlinked paths, and +mixed manifest changes remain human-routed. The exception never asserts +action effect, authority, binding, policy, or approval evidence. The resulting +`shipgate.yaml` diff is still a protected-surface change, and committed +verification continues to require human review before merge or execution. + ### JSON report fields (stable) In `agents-shipgate-reports/report.json`, the following are guaranteed: diff --git a/docs/agent-contract-current.md b/docs/agent-contract-current.md index 06f81e5a..8838dc52 100644 --- a/docs/agent-contract-current.md +++ b/docs/agent-contract-current.md @@ -142,8 +142,14 @@ proactive routing surface for coding agents before edits. It accepts a single surfaces, forbidden shortcut actions, required evidence for proposed high-risk capabilities, host-grant drift when a host baseline is present, deterministic `signals[]`, `control`, `requires_verify`, `verification_command`, -`allowed_next_commands[]`, and `plan_summary`. It is not a second gate; it must never be read as passed or -mergeable. The release gate remains `release_decision.decision`. +`allowed_next_commands[]`, and `plan_summary`. A concrete, resolvable diff that +only appends valid built-in `tool_sources` rows may mark that manifest touch +`requires_human_review=false` and route the coding agent to verification. This +authorizes proposal authorship only: existing rows and all other manifest +values must be unchanged, authority-bearing fields and custom adapters are +excluded, and the resulting trust-root diff still requires human review. It is +not a second gate; it must never be read as passed or mergeable. The release +gate remains `release_decision.decision`. ## Read these first for release gating diff --git a/docs/quickstart.md b/docs/quickstart.md index e8501af5..c5abadb1 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -28,6 +28,15 @@ as diagnostic context only, and do not infer control from prose. Only authorizes only the stated coding-agent route, and `human_review_required` requires the agent to stop. +When `check` discovers an undeclared deployed tool surface, a coding agent may +prepare the narrow coverage proposal itself: append a built-in `tool_sources` +row with a contained existing path, leaving all existing rows and other +manifest values unchanged. Run preflight with the concrete diff, then follow +its exact verify command. This is proposal authorship, not approval; custom +adapters, trust/optional declarations, semantic or authority claims, and any +broader manifest change remain human-routed, and the concrete protected diff +still needs human review before merge. + ### PR And Local Verification For local pre-commit work, omit `--base` and `--head` so uncommitted edits are diff --git a/llms-full.txt b/llms-full.txt index 1ea53e66..fb5df05b 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -1166,8 +1166,14 @@ proactive routing surface for coding agents before edits. It accepts a single surfaces, forbidden shortcut actions, required evidence for proposed high-risk capabilities, host-grant drift when a host baseline is present, deterministic `signals[]`, `control`, `requires_verify`, `verification_command`, -`allowed_next_commands[]`, and `plan_summary`. It is not a second gate; it must never be read as passed or -mergeable. The release gate remains `release_decision.decision`. +`allowed_next_commands[]`, and `plan_summary`. A concrete, resolvable diff that +only appends valid built-in `tool_sources` rows may mark that manifest touch +`requires_human_review=false` and route the coding agent to verification. This +authorizes proposal authorship only: existing rows and all other manifest +values must be unchanged, authority-bearing fields and custom adapters are +excluded, and the resulting trust-root diff still requires human review. It is +not a second gate; it must never be read as passed or mergeable. The release +gate remains `release_decision.decision`. ## Read these first for release gating diff --git a/src/agents_shipgate/cli/agent_result.py b/src/agents_shipgate/cli/agent_result.py index ef3db7ec..01ea7b72 100644 --- a/src/agents_shipgate/cli/agent_result.py +++ b/src/agents_shipgate/cli/agent_result.py @@ -1,7 +1,7 @@ from __future__ import annotations import json -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Any from agents_shipgate.config.loader import load_manifest @@ -144,7 +144,11 @@ def _undeclared_tool_surfaces_changed( catalog = load_triggers() undeclared: list[str] = [] for path in changed_files: - if path in declared_set or is_agent_boundary_path(path): + if ( + path in declared_set + or is_agent_boundary_path(path) + or _is_non_deployed_fixture_path(path) + ): continue # Per-file classification: a glob rule matches on the path, a # diff_contains rule (n8n, @function_tool) on this file's added lines. @@ -167,6 +171,29 @@ def _undeclared_tool_surfaces_changed( return sorted(dict.fromkeys(undeclared)) +def _is_non_deployed_fixture_path(path: str) -> bool: + """Exclude conventional test artifacts from undeclared deployment inference. + + This filter is intentionally applied only after declared surfaces have + been computed. A fixture explicitly named by ``shipgate.yaml`` therefore + remains a real capability surface and is still routed through verify. + """ + + parts = PurePosixPath(path.replace("\\", "/")).parts + fixture_dirs = { + "__snapshots__", + "fixture", + "fixtures", + "golden", + "goldens", + "test", + "tests", + } + if any(part.lower() in fixture_dirs for part in parts[:-1]): + return True + return len(parts) >= 3 and parts[0] == "samples" and "expected" in parts[2:-1] + + def _declared_tool_surfaces_changed( *, workspace: Path, diff --git a/src/agents_shipgate/cli/preflight.py b/src/agents_shipgate/cli/preflight.py index 9828d2d3..aeb732f1 100644 --- a/src/agents_shipgate/cli/preflight.py +++ b/src/agents_shipgate/cli/preflight.py @@ -90,14 +90,20 @@ def preflight( ) else: changed = _read_changed_files(changed_files) + diff_text = None if diff is not None: - changed = sorted(set(changed) | set(_changed_files_from_diff(diff))) + diff_text = _read_diff(diff) + changed = sorted( + set(changed) + | {item.path for item in parse_unified_diff(diff_text) if item.path} + ) request = _read_capability_request(capability_request) base = _read_base_preflight(base_preflight) result = build_preflight_result( workspace=workspace, config=config, changed_files=changed, + diff_text=diff_text, capability_request=request, base_preflight=base, host_baseline=host_baseline, @@ -134,9 +140,8 @@ def _read_changed_files(path: Path | None) -> list[str]: return [line.strip() for line in path.read_text(encoding="utf-8").splitlines() if line.strip()] -def _changed_files_from_diff(path: Path) -> list[str]: - text = sys.stdin.read() if str(path) == "-" else path.read_text(encoding="utf-8") - return sorted({item.path for item in parse_unified_diff(text) if item.path}) +def _read_diff(path: Path) -> str: + return sys.stdin.read() if str(path) == "-" else path.read_text(encoding="utf-8") def _read_capability_request(path: Path | None) -> CapabilityRequestV1 | None: diff --git a/src/agents_shipgate/core/agent_boundary.py b/src/agents_shipgate/core/agent_boundary.py index 156340ab..441d70ad 100644 --- a/src/agents_shipgate/core/agent_boundary.py +++ b/src/agents_shipgate/core/agent_boundary.py @@ -228,8 +228,8 @@ def evaluate_agent_boundary( item.path for item in diagnostics if ( - item.code == "content_source" - and item.path == ".codex/config.toml" + (item.code == "content_source" and item.path == ".codex/config.toml") + or item.code == "proposal_safe_manifest_addition" ) }, ) diff --git a/src/agents_shipgate/core/codex_boundary.py b/src/agents_shipgate/core/codex_boundary.py index 27751aed..b08eae9c 100644 --- a/src/agents_shipgate/core/codex_boundary.py +++ b/src/agents_shipgate/core/codex_boundary.py @@ -40,6 +40,9 @@ boundary_adapters_for_path, is_agent_boundary_path, ) +from agents_shipgate.core.manifest_proposals import ( + assess_coverage_increasing_tool_source_proposal, +) from agents_shipgate.schemas.agent_control import ( CodingAgentCommandAction, HumanControlAction, @@ -478,6 +481,27 @@ def add(rule_id: str, *, path: str | None, evidence: dict[str, Any]) -> None: if not path: continue normalized = path.replace("\\", "/") + if normalized == "shipgate.yaml": + resolved = resolve(diff_file) + assessment = assess_coverage_increasing_tool_source_proposal( + workspace=workspace, + diff_file=diff_file, + resolved=resolved, + ) + if assessment.proposal_safe: + evaluated_files.append(_evaluated_file_record(path, resolved)) + diagnostics.append( + AgentResultDiagnostic( + level="info", + code="proposal_safe_manifest_addition", + message=( + "Manifest change only appends valid built-in tool-source " + "coverage. The coding agent may author this proposal, but " + "verify and human review still govern the concrete diff." + ), + path=normalized, + ) + ) if _is_codex_config_path(normalized): resolved = resolve(diff_file) evaluated_files.append(_evaluated_file_record(path, resolved)) diff --git a/src/agents_shipgate/core/manifest_proposals.py b/src/agents_shipgate/core/manifest_proposals.py new file mode 100644 index 00000000..e39f8a33 --- /dev/null +++ b/src/agents_shipgate/core/manifest_proposals.py @@ -0,0 +1,219 @@ +"""Fail-closed classification for agent-authored manifest proposals. + +This module does not approve a manifest change. It recognizes one narrow +authoring-safe shape: valid built-in ``tool_sources`` rows appended to an +otherwise byte-semantically unchanged manifest. The resulting concrete diff +still goes through verify and remains a protected-surface review item. +""" + +from __future__ import annotations + +from copy import deepcopy +from dataclasses import dataclass +from pathlib import Path, PurePosixPath +from typing import Any + +from pydantic import ValidationError +from ruamel.yaml import YAML +from ruamel.yaml.error import YAMLError + +from agents_shipgate.core.boundary_diff import ( + DiffFile, + ResolvedFileText, + _resolve_changed_file_text, +) +from agents_shipgate.schemas.manifest import AgentsShipgateManifest +from agents_shipgate.schemas.manifest.tool_sources import BUILTIN_TOOL_SOURCE_TYPES + +_SAFE_SOURCE_KEYS = frozenset({"id", "type", "path", "mode"}) +_FILE_SOURCE_TYPES = frozenset({"mcp", "openapi", "conductor"}) + + +@dataclass(frozen=True) +class ToolSourceProposalAssessment: + """Semantic result for one proposed ``shipgate.yaml`` change.""" + + proposal_safe: bool + reason: str + added_source_ids: tuple[str, ...] = () + + +def assess_coverage_increasing_tool_source_proposal( + *, + workspace: Path, + diff_file: DiffFile, + resolved: ResolvedFileText | None = None, +) -> ToolSourceProposalAssessment: + """Recognize an append-only, coverage-increasing manifest proposal. + + Safety is deliberately structural and monotonic: + + * both manifest sides must parse and validate; + * every non-``tool_sources`` value must remain identical; + * all existing source rows must remain identical and in the same order; + * added rows may use only built-in adapters and non-authority fields; and + * every added path must resolve to an existing, non-symlink workspace + artifact of the expected broad shape. + + A safe result authorizes proposal authorship only. It never supplies or + asserts approval, action semantics, bindings, policy evidence, or release + authority. + """ + + root = workspace.resolve() + if resolved is None: + resolved = _resolve_changed_file_text(root, diff_file, []) + if resolved.old_text is None or resolved.new_text is None: + return _unsafe("manifest diff could not be resolved against the workspace") + + try: + old_payload = _load_yaml_mapping(resolved.old_text) + new_payload = _load_yaml_mapping(resolved.new_text) + AgentsShipgateManifest.model_validate(old_payload) + AgentsShipgateManifest.model_validate(new_payload) + except (TypeError, ValueError, ValidationError, YAMLError) as exc: + return _unsafe(f"manifest side is invalid: {type(exc).__name__}") + + old_without_sources = deepcopy(old_payload) + new_without_sources = deepcopy(new_payload) + old_had_sources = "tool_sources" in old_payload + old_sources = old_without_sources.pop("tool_sources", []) + new_sources = new_without_sources.pop("tool_sources", []) + if old_without_sources != new_without_sources: + return _unsafe("proposal changes manifest keys outside tool_sources") + if not isinstance(old_sources, list) or not isinstance(new_sources, list): + return _unsafe("tool_sources must remain a list") + if len(new_sources) <= len(old_sources): + return _unsafe("proposal does not append a tool source") + if new_sources[: len(old_sources)] != old_sources: + return _unsafe("proposal removes, reorders, or modifies an existing tool source") + + additions = new_sources[len(old_sources) :] + added_rows_reason = _validate_exact_added_rows( + diff_file=diff_file, + old_had_sources=old_had_sources, + additions=additions, + ) + if added_rows_reason is not None: + return _unsafe(added_rows_reason) + added_ids: list[str] = [] + for row in additions: + reason = _validate_added_source(root, row) + if reason is not None: + return _unsafe(reason) + assert isinstance(row, dict) # proved by _validate_added_source + added_ids.append(str(row["id"])) + + return ToolSourceProposalAssessment( + proposal_safe=True, + reason=( + "proposal only appends valid built-in tool_sources entries; the " + "concrete manifest diff still requires deterministic verification" + ), + added_source_ids=tuple(added_ids), + ) + + +def _load_yaml_mapping(text: str) -> dict[str, Any]: + loader = YAML(typ="safe") + loader.allow_duplicate_keys = False + payload = loader.load(text) + if not isinstance(payload, dict): + raise TypeError("manifest must be a YAML mapping") + return dict(payload) + + +def _validate_added_source(root: Path, row: Any) -> str | None: + if not isinstance(row, dict): + return "added tool source must be a mapping" + if not set(row).issubset(_SAFE_SOURCE_KEYS): + return "added tool source contains authority-bearing or unsupported fields" + source_id = row.get("id") + source_type = row.get("type") + source_path = row.get("path") + mode = row.get("mode") + if not isinstance(source_id, str) or not source_id.strip(): + return "added tool source id must be non-empty" + if source_type not in BUILTIN_TOOL_SOURCE_TYPES: + return "proposal-safe additions are limited to built-in tool source types" + if not isinstance(source_path, str) or not source_path.strip(): + return "added tool source path must be non-empty" + if source_type == "codex_plugin": + if mode not in {"package", "marketplace"}: + return "proposal-safe Codex plugin additions require an explicit mode" + elif mode is not None: + return "only Codex plugin additions may declare mode" + + normalized = source_path.replace("\\", "/") + pure = PurePosixPath(normalized) + if pure.is_absolute() or normalized in {"", "."} or ".." in pure.parts: + return "added tool source path must be a contained non-root relative path" + candidate = root.joinpath(*pure.parts) + if _path_has_symlink(root, pure): + return "added tool source path must not traverse a symlink" + try: + candidate.resolve().relative_to(root) + except (OSError, ValueError): + return "added tool source path resolves outside the workspace" + if not candidate.exists(): + return "added tool source path does not exist" + + if source_type in _FILE_SOURCE_TYPES and not candidate.is_file(): + return f"{source_type} proposal-safe source must be a file" + if source_type == "codex_plugin": + if mode == "package": + plugin_manifest = candidate / ".codex-plugin" / "plugin.json" + plugin_manifest_relative = PurePosixPath(plugin_manifest.relative_to(root).as_posix()) + if ( + not candidate.is_dir() + or _path_has_symlink(root, plugin_manifest_relative) + or not plugin_manifest.is_file() + ): + return "Codex plugin package lacks .codex-plugin/plugin.json" + elif not candidate.is_file(): + return "Codex plugin marketplace source must be a file" + return None + + +def _validate_exact_added_rows( + *, + diff_file: DiffFile, + old_had_sources: bool, + additions: list[Any], +) -> str | None: + """Require the textual diff to contain exactly the parsed source rows.""" + + if diff_file.removed_lines: + return "proposal-safe manifest changes must be insertion-only" + if not diff_file.added_lines: + return "proposal-safe manifest change contains no added source lines" + if any(not line.strip() or "#" in line for line in diff_file.added_lines): + return "proposal-safe source lines must not contain comments or blank additions" + added_text = "\n".join(diff_file.added_lines) + "\n" + snippet = f"tool_sources:\n{added_text}" if old_had_sources else added_text + try: + payload = _load_yaml_mapping(snippet) + except (TypeError, ValueError, YAMLError): + return "added lines are not a standalone tool_sources append" + if set(payload) != {"tool_sources"} or payload.get("tool_sources") != additions: + return "added lines contain content outside the appended tool source rows" + return None + + +def _path_has_symlink(root: Path, path: PurePosixPath) -> bool: + current = root + for part in path.parts: + current = current / part + if current.is_symlink(): + return True + return False + + +def _unsafe(reason: str) -> ToolSourceProposalAssessment: + return ToolSourceProposalAssessment(proposal_safe=False, reason=reason) + + +__all__ = [ + "ToolSourceProposalAssessment", + "assess_coverage_increasing_tool_source_proposal", +] diff --git a/src/agents_shipgate/core/preflight.py b/src/agents_shipgate/core/preflight.py index 9dd47653..c81d75d5 100644 --- a/src/agents_shipgate/core/preflight.py +++ b/src/agents_shipgate/core/preflight.py @@ -25,6 +25,9 @@ from agents_shipgate.core.lenses.effective_policy import ( build_effective_policy_snapshot, ) +from agents_shipgate.core.manifest_proposals import ( + assess_coverage_increasing_tool_source_proposal, +) from agents_shipgate.schemas.agent_control import ( CodingAgentCommandAction, HumanControlAction, @@ -231,6 +234,7 @@ def build_preflight_result( capability_requests: list[CapabilityRequestV1 | dict[str, Any]] | None = None, host_permission_requests: list[HostPermissionRequestV1 | dict[str, Any]] | None = None, plan: PreflightPlanV1 | dict[str, Any] | None = None, + diff_text: str | None = None, base_preflight: ( PreflightResultV1 | PreflightResultV2 | PreflightResultV3 | dict[str, Any] | None ) = None, @@ -240,11 +244,12 @@ def build_preflight_result( config_path = config if config.is_absolute() else root / config config_path = config_path.resolve() request_plan = _coerce_plan(plan) + effective_diff_text = request_plan.diff_text if request_plan is not None else diff_text changed_inputs = list(changed_files or []) if request_plan is not None: changed_inputs.extend(request_plan.changed_files) - if request_plan.diff_text: - changed_inputs.extend(_changed_files_from_diff_text(request_plan.diff_text)) + if effective_diff_text: + changed_inputs.extend(_changed_files_from_diff_text(effective_diff_text)) changed = _normalize_changed_files(changed_inputs) graph = build_trust_root_graph(root) policy_hash, notes = _policy_hash_for_config(config_path) @@ -261,6 +266,12 @@ def build_preflight_result( for node in graph.nodes ] touches = classify_protected_touches(changed) + touches = _classify_proposal_safe_manifest_touch( + workspace=root, + config_path=config_path, + touches=touches, + diff_text=effective_diff_text, + ) requests = _coerce_capability_requests( capability_request=capability_request, capability_requests=capability_requests, @@ -271,7 +282,7 @@ def build_preflight_result( plan=request_plan, ) required_evidence = required_evidence_for_capability_requests(requests) - requires_human_review = bool(touches) or any( + requires_human_review = any(touch.requires_human_review for touch in touches) or any( not item.satisfied and item.severity in {"high", "critical"} for item in required_evidence ) base = _coerce_base_preflight(base_preflight) @@ -523,20 +534,76 @@ def signals_for_protected_touches( id=f"protected_surface:{touch.path}", kind="protected_surface_touch", severity="critical" if touch.scope_type == "whole_file" else "high", - actor="human", + actor="human" if touch.requires_human_review else "coding_agent", subject=touch.kind, path=touch.path, reason=( - f"{touch.path} matches protected surface {touch.pattern}; " - "a coding agent must not self-approve trust-root edits." + ( + f"{touch.path} matches protected surface {touch.pattern}; " + "a coding agent must not self-approve trust-root edits." + ) + if touch.requires_human_review + else ( + f"{touch.path} contains only an exact append-only proposal for " + "built-in tool-source coverage; proposal authorship is allowed, " + "but the concrete protected-surface diff is not approved." + ) + ), + recommendation=( + "Route this protected-surface edit to a human before making or relying on it." + if touch.requires_human_review + else ( + "Apply only the exact planned tool-source addition, then run the " + "required verifier and route its concrete review evidence to a human." + ) ), - recommendation="Route this protected-surface edit to a human before making or relying on it.", related_command="agents-shipgate preflight --workspace . --plan - --json", ) for touch in touches ] +def _classify_proposal_safe_manifest_touch( + *, + workspace: Path, + config_path: Path, + touches: list[PreflightProtectedSurfaceTouch], + diff_text: str | None, +) -> list[PreflightProtectedSurfaceTouch]: + """Mark one exact coverage-increasing manifest proposal as authoring-safe. + + Path-only preflight remains fail-closed. This exception requires both + sides of a concrete diff and changes only the existing per-touch routing + flag; it does not approve the resulting protected-surface edit. + """ + + if not diff_text: + return touches + config_display = _display_path(config_path, workspace) + diff_file = next( + ( + item + for item in parse_unified_diff(diff_text) + if item.path.replace("\\", "/") == config_display + ), + None, + ) + if diff_file is None: + return touches + assessment = assess_coverage_increasing_tool_source_proposal( + workspace=workspace, + diff_file=diff_file, + ) + if not assessment.proposal_safe: + return touches + return [ + touch.model_copy(update={"requires_human_review": False}) + if touch.kind == "manifest" and touch.path == config_display + else touch + for touch in touches + ] + + def signals_for_capability_requests( requests: list[CapabilityRequestV1], ) -> list[PreflightSignalV1]: diff --git a/tests/test_codex_boundary_check.py b/tests/test_codex_boundary_check.py index 3c4c42b6..460db788 100644 --- a/tests/test_codex_boundary_check.py +++ b/tests/test_codex_boundary_check.py @@ -1,5 +1,6 @@ from __future__ import annotations +import difflib import json from pathlib import Path @@ -206,6 +207,114 @@ def _write_manifest(tmp_path: Path, tool_sources: str) -> None: ) +def _manifest_diff(old: str, new: str) -> str: + return "diff --git a/shipgate.yaml b/shipgate.yaml\n" + "".join( + difflib.unified_diff( + old.splitlines(keepends=True), + new.splitlines(keepends=True), + fromfile="a/shipgate.yaml", + tofile="b/shipgate.yaml", + ) + ) + + +def test_safe_manifest_source_proposal_routes_to_verify_not_unclassified_review( + tmp_path: Path, +) -> None: + _write_manifest( + tmp_path, + " - id: existing\n type: mcp\n path: mcp-tools.json\n", + ) + (tmp_path / "mcp-tools.json").write_text('{"tools": []}\n', encoding="utf-8") + plugin_manifest = tmp_path / "plugins" / "reviewer" / ".codex-plugin" / "plugin.json" + plugin_manifest.parent.mkdir(parents=True) + plugin_manifest.write_text('{"name": "reviewer"}\n', encoding="utf-8") + old = (tmp_path / "shipgate.yaml").read_text(encoding="utf-8") + new = old + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + + result = build_codex_agent_result( + agent="claude-code", + workspace=tmp_path, + diff_text=_manifest_diff(old, new), + config=Path("shipgate.yaml"), + policy=None, + ) + + assert result.control.state == "agent_action_required" + assert result.control.must_stop is False + assert result.control.next_action.kind == "verify" + assert not any( + item.check_id == "SHIP-AGENT-BOUNDARY-PROTECTED-SURFACE-UNCLASSIFIED" + for item in result.violated_rules + ) + assert any(item.code == "proposal_safe_manifest_addition" for item in result.diagnostics) + + +def test_test_golden_mcp_json_is_not_inferred_as_deployed_surface(tmp_path: Path) -> None: + _write_manifest( + tmp_path, + " - id: existing\n type: mcp\n path: mcp-tools.json\n", + ) + diff = ( + "diff --git a/tests/golden/codex_boundary_result/mcp_auto_approve_write.json " + "b/tests/golden/codex_boundary_result/mcp_auto_approve_write.json\n" + "--- a/tests/golden/codex_boundary_result/mcp_auto_approve_write.json\n" + "+++ b/tests/golden/codex_boundary_result/mcp_auto_approve_write.json\n" + "@@ -1 +1 @@\n" + '-{"tools": []}\n' + '+{"tools": [{"name": "write"}]}\n' + ) + + result = build_codex_agent_result( + agent="claude-code", + workspace=tmp_path, + diff_text=diff, + config=Path("shipgate.yaml"), + policy=None, + ) + + assert result.control.state == "agent_action_required" + assert result.control.next_action.kind == "verify" + assert not any(item.code == "undeclared_capability_surface" for item in result.diagnostics) + + +def test_explicitly_declared_test_fixture_still_routes_to_verify(tmp_path: Path) -> None: + fixture = "tests/fixtures/deployed-tools.json" + _write_manifest( + tmp_path, + f" - id: deployed-fixture\n type: mcp\n path: {fixture}\n", + ) + target = tmp_path / fixture + target.parent.mkdir(parents=True) + target.write_text('{"tools": []}\n', encoding="utf-8") + diff = ( + f"diff --git a/{fixture} b/{fixture}\n" + f"--- a/{fixture}\n" + f"+++ b/{fixture}\n" + "@@ -1 +1 @@\n" + '-{"tools": []}\n' + '+{"tools": [{"name": "write"}]}\n' + ) + + result = build_codex_agent_result( + agent="claude-code", + workspace=tmp_path, + diff_text=diff, + config=Path("shipgate.yaml"), + policy=None, + ) + + assert result.control.state == "agent_action_required" + assert result.control.next_action.kind == "verify" + assert any(item.code == "capability_change_requires_verify" for item in result.diagnostics) + assert not any(item.code == "undeclared_capability_surface" for item in result.diagnostics) + + def test_check_warns_on_change_under_declared_directory_source(tmp_path: Path) -> None: # A directory tool source (loaders scan files inside it) must match a # changed file *under* the directory, not only an exact path equal to it. diff --git a/tests/test_preflight.py b/tests/test_preflight.py index 0257d9ab..f968a5c4 100644 --- a/tests/test_preflight.py +++ b/tests/test_preflight.py @@ -1,5 +1,6 @@ from __future__ import annotations +import difflib import json import sys from pathlib import Path @@ -70,6 +71,17 @@ def _write(root: Path, path: str, text: str = "x\n") -> None: target.write_text(text, encoding="utf-8") +def _manifest_diff(old: str, new: str) -> str: + return "diff --git a/shipgate.yaml b/shipgate.yaml\n" + "".join( + difflib.unified_diff( + old.splitlines(keepends=True), + new.splitlines(keepends=True), + fromfile="a/shipgate.yaml", + tofile="b/shipgate.yaml", + ) + ) + + def test_preflight_routes_protected_surface_touches_to_human(tmp_path: Path) -> None: root = _workspace(tmp_path) @@ -98,6 +110,163 @@ def test_preflight_routes_protected_surface_touches_to_human(tmp_path: Path) -> assert any(".codex/config.toml" in pattern for pattern in result.forbidden_file_edits) +def test_preflight_allows_exact_append_only_builtin_source_proposal( + tmp_path: Path, +) -> None: + root = _workspace(tmp_path) + plugin = root / "plugins" / "reviewer" + _write(plugin, ".codex-plugin/plugin.json", '{"name": "reviewer"}\n') + old = (root / "shipgate.yaml").read_text(encoding="utf-8") + new = old + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + + result = build_preflight_result( + workspace=root, + diff_text=_manifest_diff(old, new), + ) + + assert result.changed_files == ["shipgate.yaml"] + assert result.requires_human_review is False + assert result.protected_surface_touches[0].requires_human_review is False + assert result.control.state == "agent_action_required" + assert result.control.must_stop is False + assert result.control.next_action.kind == "verify" + assert result.control.allowed_next_commands == [ + "agents-shipgate verify --workspace . --config shipgate.yaml --ci-mode advisory --json" + ] + signal = next(item for item in result.signals if item.kind == "protected_surface_touch") + assert signal.actor == "coding_agent" + assert "not approved" in signal.reason + + +@pytest.mark.parametrize( + "addition", + [ + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + " trust: internal\n" + ), + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + " optional: true\n" + ), + (" - id: custom-source\n type: vendor_custom\n path: plugins/reviewer\n"), + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + ), + " - id: root-config\n type: codex_config\n path: .\n", + " - id: missing\n type: mcp\n path: missing-tools.json\n", + " - id: directory\n type: mcp\n path: plugins/reviewer\n", + ( + " - id: marketplace\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: marketplace\n" + ), + ], +) +def test_preflight_rejects_unsafe_source_proposal( + tmp_path: Path, + addition: str, +) -> None: + root = _workspace(tmp_path) + plugin = root / "plugins" / "reviewer" + _write(plugin, ".codex-plugin/plugin.json", '{"name": "reviewer"}\n') + old = (root / "shipgate.yaml").read_text(encoding="utf-8") + + result = build_preflight_result( + workspace=root, + diff_text=_manifest_diff(old, old + addition), + ) + + assert result.requires_human_review is True + assert result.protected_surface_touches[0].requires_human_review is True + assert result.control.state == "human_review_required" + assert result.control.must_stop is True + + +def test_preflight_rejects_source_addition_mixed_with_other_manifest_change( + tmp_path: Path, +) -> None: + root = _workspace(tmp_path) + plugin = root / "plugins" / "reviewer" + _write(plugin, ".codex-plugin/plugin.json", '{"name": "reviewer"}\n') + old = (root / "shipgate.yaml").read_text(encoding="utf-8") + new = old.replace("name: preflight-test", "name: renamed") + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + + result = build_preflight_result( + workspace=root, + diff_text=_manifest_diff(old, new), + ) + + assert result.requires_human_review is True + assert result.control.state == "human_review_required" + + +def test_preflight_rejects_source_addition_mixed_with_comment_claim( + tmp_path: Path, +) -> None: + root = _workspace(tmp_path) + plugin = root / "plugins" / "reviewer" + _write(plugin, ".codex-plugin/plugin.json", '{"name": "reviewer"}\n') + old = (root / "shipgate.yaml").read_text(encoding="utf-8") + new = old.replace('version: "0.1"', '# human-approved\nversion: "0.1"') + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + + result = build_preflight_result( + workspace=root, + diff_text=_manifest_diff(old, new), + ) + + assert result.requires_human_review is True + assert result.control.state == "human_review_required" + + +def test_preflight_rejects_plugin_source_with_symlinked_manifest(tmp_path: Path) -> None: + root = _workspace(tmp_path) + plugin_manifest = root / "plugins" / "reviewer" / ".codex-plugin" / "plugin.json" + plugin_manifest.parent.mkdir(parents=True) + outside = tmp_path / "outside-plugin.json" + outside.write_text('{"name": "reviewer"}\n', encoding="utf-8") + plugin_manifest.symlink_to(outside) + old = (root / "shipgate.yaml").read_text(encoding="utf-8") + new = old + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + + result = build_preflight_result( + workspace=root, + diff_text=_manifest_diff(old, new), + ) + + assert result.requires_human_review is True + assert result.control.state == "human_review_required" + + @pytest.mark.parametrize( "path,expected_kind,expected_scope_type", [ @@ -370,6 +539,49 @@ def test_cli_preflight_json_changed_files_and_diff(tmp_path: Path) -> None: assert any(signal["kind"] == "protected_surface_touch" for signal in payload["signals"]) +def test_cli_preflight_uses_diff_semantics_for_safe_manifest_proposal( + tmp_path: Path, +) -> None: + root = _workspace(tmp_path) + plugin = root / "plugins" / "reviewer" + _write(plugin, ".codex-plugin/plugin.json", '{"name": "reviewer"}\n') + old = (root / "shipgate.yaml").read_text(encoding="utf-8") + new = old + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + diff = tmp_path / "manifest.diff" + diff.write_text(_manifest_diff(old, new), encoding="utf-8") + + result = runner.invoke( + app, + [ + "preflight", + "--workspace", + str(root), + "--diff", + str(diff), + "--json", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["control"]["state"] == "agent_action_required" + assert payload["control"]["must_stop"] is False + assert payload["protected_surface_touches"] == [ + { + "path": "shipgate.yaml", + "kind": "manifest", + "pattern": "**/shipgate.yaml", + "scope_type": "key_level", + "requires_human_review": False, + } + ] + + def test_cli_preflight_capability_request(tmp_path: Path) -> None: root = _workspace(tmp_path) request = tmp_path / "request.json" From c459fc78ce655f62f303f829ecc0e987fee40b58 Mon Sep 17 00:00:00 2001 From: Pengfei Hu Date: Mon, 20 Jul 2026 16:54:02 -0700 Subject: [PATCH 2/2] Fail closed on composite manifest diffs --- src/agents_shipgate/cli/agent_result.py | 17 ++-- src/agents_shipgate/core/codex_boundary.py | 12 ++- .../core/manifest_proposals.py | 7 ++ src/agents_shipgate/core/preflight.py | 19 ++--- tests/test_codex_boundary_check.py | 82 +++++++++++++++++++ tests/test_preflight.py | 33 ++++++++ 6 files changed, 153 insertions(+), 17 deletions(-) diff --git a/src/agents_shipgate/cli/agent_result.py b/src/agents_shipgate/cli/agent_result.py index 01ea7b72..ea50d423 100644 --- a/src/agents_shipgate/cli/agent_result.py +++ b/src/agents_shipgate/cli/agent_result.py @@ -176,22 +176,27 @@ def _is_non_deployed_fixture_path(path: str) -> bool: This filter is intentionally applied only after declared surfaces have been computed. A fixture explicitly named by ``shipgate.yaml`` therefore - remains a real capability surface and is still routed through verify. + remains a real capability surface and is still routed through verify. Test + directories may appear below a package root, but fixture/golden names are + excluded only at the repository root or beneath an explicit test root so a + production path such as ``services/fixtures`` remains in scope. """ parts = PurePosixPath(path.replace("\\", "/")).parts - fixture_dirs = { + lowered = tuple(part.lower() for part in parts) + test_roots = {"test", "tests"} + fixture_roots = { "__snapshots__", "fixture", "fixtures", "golden", "goldens", - "test", - "tests", } - if any(part.lower() in fixture_dirs for part in parts[:-1]): + if any(part in test_roots for part in lowered[:-1]): return True - return len(parts) >= 3 and parts[0] == "samples" and "expected" in parts[2:-1] + if lowered and lowered[0] in fixture_roots: + return True + return len(lowered) >= 4 and lowered[0] == "samples" and lowered[2] == "expected" def _declared_tool_surfaces_changed( diff --git a/src/agents_shipgate/core/codex_boundary.py b/src/agents_shipgate/core/codex_boundary.py index b08eae9c..7f16efa9 100644 --- a/src/agents_shipgate/core/codex_boundary.py +++ b/src/agents_shipgate/core/codex_boundary.py @@ -476,12 +476,22 @@ def add(rule_id: str, *, path: str | None, evidence: dict[str, Any]) -> None: }, ) + manifest_diff_files = [ + item + for item in diff_files + if item.path and item.path.replace("\\", "/") == "shipgate.yaml" + ] + proposal_candidate = ( + manifest_diff_files[0] if len(manifest_diff_files) == 1 else None + ) for diff_file in diff_files: path = diff_file.path if not path: continue normalized = path.replace("\\", "/") - if normalized == "shipgate.yaml": + # A block-level safe signal may clear the path-wide unclassified guard + # only when this is the sole record targeting the protected manifest. + if diff_file is proposal_candidate: resolved = resolve(diff_file) assessment = assess_coverage_increasing_tool_source_proposal( workspace=workspace, diff --git a/src/agents_shipgate/core/manifest_proposals.py b/src/agents_shipgate/core/manifest_proposals.py index e39f8a33..5a9854b4 100644 --- a/src/agents_shipgate/core/manifest_proposals.py +++ b/src/agents_shipgate/core/manifest_proposals.py @@ -26,6 +26,10 @@ from agents_shipgate.schemas.manifest.tool_sources import BUILTIN_TOOL_SOURCE_TYPES _SAFE_SOURCE_KEYS = frozenset({"id", "type", "path", "mode"}) +# MCP/OpenAPI/Conductor declarations name a single protocol artifact. The +# remaining built-in framework/config adapters legitimately accept a file or a +# directory; containment/existence is checked here and verify validates the +# concrete loader shape. _FILE_SOURCE_TYPES = frozenset({"mcp", "openapi", "conductor"}) @@ -187,6 +191,9 @@ def _validate_exact_added_rows( return "proposal-safe manifest changes must be insertion-only" if not diff_file.added_lines: return "proposal-safe manifest change contains no added source lines" + # Intentionally over-restrict the authoring safelist: even a quoted ``#`` + # in an otherwise valid id/path is human-routed so comments or approval + # claims cannot be smuggled into the proposal-only exception. if any(not line.strip() or "#" in line for line in diff_file.added_lines): return "proposal-safe source lines must not contain comments or blank additions" added_text = "\n".join(diff_file.added_lines) + "\n" diff --git a/src/agents_shipgate/core/preflight.py b/src/agents_shipgate/core/preflight.py index c81d75d5..2312a1e3 100644 --- a/src/agents_shipgate/core/preflight.py +++ b/src/agents_shipgate/core/preflight.py @@ -580,19 +580,18 @@ def _classify_proposal_safe_manifest_touch( if not diff_text: return touches config_display = _display_path(config_path, workspace) - diff_file = next( - ( - item - for item in parse_unified_diff(diff_text) - if item.path.replace("\\", "/") == config_display - ), - None, - ) - if diff_file is None: + matching_diff_files = [ + item + for item in parse_unified_diff(diff_text) + if item.path.replace("\\", "/") == config_display + ] + # A per-record safe result must never clear the path-wide fail-closed + # routing for another record targeting the same protected manifest. + if len(matching_diff_files) != 1: return touches assessment = assess_coverage_increasing_tool_source_proposal( workspace=workspace, - diff_file=diff_file, + diff_file=matching_diff_files[0], ) if not assessment.proposal_safe: return touches diff --git a/tests/test_codex_boundary_check.py b/tests/test_codex_boundary_check.py index 460db788..debd3511 100644 --- a/tests/test_codex_boundary_check.py +++ b/tests/test_codex_boundary_check.py @@ -4,6 +4,7 @@ import json from pathlib import Path +import pytest from jsonschema import Draft202012Validator from typer.testing import CliRunner @@ -255,6 +256,55 @@ def test_safe_manifest_source_proposal_routes_to_verify_not_unclassified_review( assert any(item.code == "proposal_safe_manifest_addition" for item in result.diagnostics) +@pytest.mark.parametrize("safe_block_first", [True, False]) +def test_duplicate_manifest_blocks_with_unsafe_edit_remain_human_routed( + tmp_path: Path, + safe_block_first: bool, +) -> None: + _write_manifest( + tmp_path, + " - id: existing\n type: mcp\n path: mcp-tools.json\n", + ) + (tmp_path / "mcp-tools.json").write_text('{"tools": []}\n', encoding="utf-8") + plugin_manifest = tmp_path / "plugins" / "reviewer" / ".codex-plugin" / "plugin.json" + plugin_manifest.parent.mkdir(parents=True) + plugin_manifest.write_text('{"name": "reviewer"}\n', encoding="utf-8") + old = (tmp_path / "shipgate.yaml").read_text(encoding="utf-8") + safe = old + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + unsafe = old.replace( + " path: mcp-tools.json\n", + " path: mcp-tools.json\n trust: internal\n", + ) + safe_block = _manifest_diff(old, safe) + unsafe_block = _manifest_diff(old, unsafe) + composite = ( + safe_block + unsafe_block if safe_block_first else unsafe_block + safe_block + ) + + result = build_codex_agent_result( + agent="claude-code", + workspace=tmp_path, + diff_text=composite, + config=Path("shipgate.yaml"), + policy=None, + ) + + assert result.control.state == "human_review_required" + assert result.control.must_stop is True + assert any( + item.check_id == "SHIP-AGENT-BOUNDARY-PROTECTED-SURFACE-UNCLASSIFIED" + for item in result.violated_rules + ) + assert not any( + item.code == "proposal_safe_manifest_addition" for item in result.diagnostics + ) + + def test_test_golden_mcp_json_is_not_inferred_as_deployed_surface(tmp_path: Path) -> None: _write_manifest( tmp_path, @@ -283,6 +333,38 @@ def test_test_golden_mcp_json_is_not_inferred_as_deployed_surface(tmp_path: Path assert not any(item.code == "undeclared_capability_surface" for item in result.diagnostics) +def test_production_path_named_fixtures_is_still_inferred_as_deployed_surface( + tmp_path: Path, +) -> None: + _write_manifest( + tmp_path, + " - id: existing\n type: mcp\n path: mcp-tools.json\n", + ) + path = "services/fixtures/prod-mcp.json" + diff = ( + f"diff --git a/{path} b/{path}\n" + "new file mode 100644\n" + "--- /dev/null\n" + f"+++ b/{path}\n" + "@@ -0,0 +1 @@\n" + '+{"tools": [{"name": "write"}]}\n' + ) + + result = build_codex_agent_result( + agent="claude-code", + workspace=tmp_path, + diff_text=diff, + config=Path("shipgate.yaml"), + policy=None, + ) + + assert result.control.state == "agent_action_required" + assert result.control.next_action.kind == "discover" + assert any( + item.code == "undeclared_capability_surface" for item in result.diagnostics + ) + + def test_explicitly_declared_test_fixture_still_routes_to_verify(tmp_path: Path) -> None: fixture = "tests/fixtures/deployed-tools.json" _write_manifest( diff --git a/tests/test_preflight.py b/tests/test_preflight.py index f968a5c4..54d1da3b 100644 --- a/tests/test_preflight.py +++ b/tests/test_preflight.py @@ -143,6 +143,39 @@ def test_preflight_allows_exact_append_only_builtin_source_proposal( assert "not approved" in signal.reason +@pytest.mark.parametrize("safe_block_first", [True, False]) +def test_preflight_rejects_duplicate_manifest_blocks_when_one_is_unsafe( + tmp_path: Path, + safe_block_first: bool, +) -> None: + root = _workspace(tmp_path) + plugin = root / "plugins" / "reviewer" + _write(plugin, ".codex-plugin/plugin.json", '{"name": "reviewer"}\n') + old = (root / "shipgate.yaml").read_text(encoding="utf-8") + safe = old + ( + " - id: reviewer-plugin\n" + " type: codex_plugin\n" + " path: plugins/reviewer\n" + " mode: package\n" + ) + unsafe = old.replace( + " path: tools.json\n", + " path: tools.json\n trust: internal\n", + ) + safe_block = _manifest_diff(old, safe) + unsafe_block = _manifest_diff(old, unsafe) + composite = ( + safe_block + unsafe_block if safe_block_first else unsafe_block + safe_block + ) + + result = build_preflight_result(workspace=root, diff_text=composite) + + assert result.requires_human_review is True + assert result.protected_surface_touches[0].requires_human_review is True + assert result.control.state == "human_review_required" + assert result.control.must_stop is True + + @pytest.mark.parametrize( "addition", [