From 5c3c61f79363f6a794eb897c0bd66669bf30a1a9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 16 Sep 2026 13:34:14 +0000 Subject: [PATCH] feat: expand safety model with scope, side effects, confirmation (#87) Add PermissionScope, side_effects, and requires_confirmation on ToolContract with backward-compatible defaults. Diff emits escalation / confirmation codes; capture accepts annotations without inventing values; probes support max_scope and forbidden_side_effects. Co-authored-by: Abhinaysai Kamineni --- SECURITY.md | 13 +++ docs/architecture.md | 1 + docs/change-codes.md | 6 ++ docs/safety.md | 40 ++++++++ src/tool_semantics/diff.py | 95 ++++++++++++++++++- src/tool_semantics/mcp_capture.py | 17 ++-- src/tool_semantics/models.py | 123 ++++++++++++++++++++++++ src/tool_semantics/probes.py | 55 ++++++++++- src/tool_semantics/scanner.py | 13 ++- tests/test_safety.py | 153 ++++++++++++++++++++++++++++++ 10 files changed, 501 insertions(+), 15 deletions(-) create mode 100644 docs/safety.md create mode 100644 tests/test_safety.py diff --git a/SECURITY.md b/SECURITY.md index dcbb628..5e47590 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -26,3 +26,16 @@ Do not open a public issue for vulnerabilities that could enable remote code execution, secret leakage, or unsafe tool invocation. We aim to acknowledge reports within 7 days. + +## Safety annotations (#87) + +Tool contracts may declare optional safety fields: `risk`, `scope` +(`resource`…`global`), `side_effects`, and `requires_confirmation`. Capture +**only records values present** in manifests or MCP `annotations` — Tool-Semantics +never invents side effects or scopes. Treating missing fields as `unknown` / +empty is intentional; do not assume a tool is safe because annotations are +absent. + +Diffing escalations (`tool.scope_escalated`, `tool.side_effect_added`, +`tool.confirmation_removed`) can fail CI at breaking/critical severity. See +[docs/change-codes.md](docs/change-codes.md) and [docs/safety.md](docs/safety.md). diff --git a/docs/architecture.md b/docs/architecture.md index acceb6c..b7f8b89 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -86,6 +86,7 @@ replacement for Git-tracked baselines. - [adr-live-mcp-capture.md](adr-live-mcp-capture.md) — live MCP capture ADR - [mcp-versions.md](mcp-versions.md) — supported protocol generations / transports - [config.md](config.md) — project configuration +- [safety.md](safety.md) — scope / side effects / confirmation (#87) - [github-action.md](github-action.md) — composite Action usage - [AGENT_EXECUTION.md](AGENT_EXECUTION.md) — agent execution specification diff --git a/docs/change-codes.md b/docs/change-codes.md index 7e50c8f..efcfec0 100644 --- a/docs/change-codes.md +++ b/docs/change-codes.md @@ -9,6 +9,12 @@ Severities **`breaking`** and **`critical`** fail CI (`compare` exits `1`). | `tool.added` | info | A new tool appeared; selection-collision testing is still pending | | `tool.description_changed` | warning | Description text changed; model tool-selection may drift | | `tool.risk_changed` | warning / critical | Declared risk level changed (critical when escalating from `read_only`) | +| `tool.scope_escalated` | breaking / critical | Permission scope widened (critical for `account` / `global`) | +| `tool.scope_changed` | warning | Scope changed without a clear known→wider escalation | +| `tool.side_effect_added` | breaking / critical | New declared side effect (critical for delete/payment/admin/execute) | +| `tool.side_effect_removed` | info | Declared side effect removed | +| `tool.confirmation_removed` | critical | `requires_confirmation` true→false | +| `tool.confirmation_added` | info | `requires_confirmation` false→true | | `tool.renamed` | warning | Heuristic match suggests a tool was renamed (not a hard remove+add) | | `tool.output_schema_added` | info | A tool gained an `outputSchema` | | `tool.output_schema_removed` | breaking | A tool lost its `outputSchema` | diff --git a/docs/safety.md b/docs/safety.md new file mode 100644 index 0000000..614f244 --- /dev/null +++ b/docs/safety.md @@ -0,0 +1,40 @@ +# Safety semantics + +Expanded tool safety model ([#87](https://github.com/askmy-stack/tool-semantics/issues/87)). + +## Fields on `ToolContract` + +| Field | Default when absent | Meaning | +| --- | --- | --- | +| `risk` | `unknown` | Existing `RiskLevel` enum | +| `scope` | `unknown` | Blast radius: `resource` → `project` → `workspace` → `organization` → `account` → `global` | +| `side_effects` | `[]` | Declared effect tags (`write`, `delete`, `network`, `email`, `payment`, `admin`, `execute`, `identity`, …) — open vocabulary | +| `requires_confirmation` | `null` | Explicit confirmation gate; `null` means undeclared | + +Capture (**manifest** or **MCP annotations**) copies these when present and +**never invents** values. + +## Diff codes + +| Code | When | +| --- | --- | +| `tool.scope_escalated` | Known scope widened (critical if new scope is `account`/`global`) | +| `tool.scope_changed` | Other scope transitions (including from/to `unknown`) | +| `tool.side_effect_added` | New effect declared (critical for `delete`/`payment`/`admin`/`execute`) | +| `tool.side_effect_removed` | Effect removed | +| `tool.confirmation_removed` | `requires_confirmation` true→false (**critical**) | +| `tool.confirmation_added` | false→true | + +## Probe expectations + +```yaml +- id: no-payments + intent: "Pay the vendor invoice" + expected_tool: create_payment + max_scope: workspace + forbidden_side_effects: [payment, delete] + max_risk: external_write +``` + +Offline probes fail when the selected tool exceeds `max_scope` or declares a +forbidden side effect. diff --git a/src/tool_semantics/diff.py b/src/tool_semantics/diff.py index 20281c7..9e16667 100644 --- a/src/tool_semantics/diff.py +++ b/src/tool_semantics/diff.py @@ -4,7 +4,13 @@ from pydantic import BaseModel, Field -from tool_semantics.models import InterfaceSnapshot, ToolContract, ToolParameter +from tool_semantics.models import ( + SCOPE_RANK, + InterfaceSnapshot, + PermissionScope, + ToolContract, + ToolParameter, +) class Severity(StrEnum): @@ -292,6 +298,90 @@ def _append_schema_changes( ) +def _append_safety_semantics_changes( + report: CompatibilityReport, + name: str, + old_tool: ToolContract, + new_tool: ToolContract, +) -> None: + """Diff permission scope, side effects, and confirmation requirements (#87).""" + old_scope = old_tool.scope + new_scope = new_tool.scope + if old_scope != new_scope: + old_rank = SCOPE_RANK[old_scope] + new_rank = SCOPE_RANK[new_scope] + if ( + new_scope is not PermissionScope.UNKNOWN + and old_scope is not PermissionScope.UNKNOWN + and new_rank > old_rank + ): + severity = ( + Severity.CRITICAL + if new_scope in {PermissionScope.ACCOUNT, PermissionScope.GLOBAL} + else Severity.BREAKING + ) + code = "tool.scope_escalated" + message = f"Permission scope escalated from '{old_scope}' to '{new_scope}'." + else: + severity = Severity.WARNING + code = "tool.scope_changed" + message = f"Permission scope changed from '{old_scope}' to '{new_scope}'." + report.changes.append(Change(severity=severity, code=code, subject=name, message=message)) + + old_effects = set(old_tool.side_effects) + new_effects = set(new_tool.side_effects) + added = sorted(new_effects - old_effects) + removed = sorted(old_effects - new_effects) + for effect in added: + severity = ( + Severity.CRITICAL + if effect in {"delete", "payment", "admin", "execute"} + else Severity.BREAKING + ) + report.changes.append( + Change( + severity=severity, + code="tool.side_effect_added", + subject=name, + message=f"Side effect '{effect}' was added.", + ) + ) + for effect in removed: + report.changes.append( + Change( + severity=Severity.INFO, + code="tool.side_effect_removed", + subject=name, + message=f"Side effect '{effect}' was removed.", + ) + ) + + old_confirm = old_tool.requires_confirmation + new_confirm = new_tool.requires_confirmation + if old_confirm is not None and new_confirm is not None and old_confirm != new_confirm: + if old_confirm and not new_confirm: + report.changes.append( + Change( + severity=Severity.CRITICAL, + code="tool.confirmation_removed", + subject=name, + message=( + "requires_confirmation changed from true to false " + "(confirmation requirement removed)." + ), + ) + ) + else: + report.changes.append( + Change( + severity=Severity.INFO, + code="tool.confirmation_added", + subject=name, + message="requires_confirmation changed from false to true.", + ) + ) + + def _compare_tool_pair( report: CompatibilityReport, name: str, @@ -322,6 +412,9 @@ def _compare_tool_pair( message=f"Risk level changed from '{old_tool.risk}' to '{new_tool.risk}'.", ) ) + + _append_safety_semantics_changes(report, name, old_tool, new_tool) + _append_output_schema_changes( report, name, diff --git a/src/tool_semantics/mcp_capture.py b/src/tool_semantics/mcp_capture.py index d6e7dda..868d69d 100644 --- a/src/tool_semantics/mcp_capture.py +++ b/src/tool_semantics/mcp_capture.py @@ -18,8 +18,8 @@ InterfaceSnapshot, PromptContract, ResourceContract, - RiskLevel, ToolContract, + extract_safety_annotations, ) from tool_semantics.redact import redact_snapshot from tool_semantics.scanner import ManifestError, _normalize_parameters @@ -142,20 +142,17 @@ def _tool_from_mcp(raw: dict[str, Any]) -> ToolContract: output_schema = raw.get("outputSchema") if output_schema is not None and not isinstance(output_schema, dict): raise McpCaptureError(f"Tool '{name}' outputSchema must be an object") - risk_raw = None - annotations = raw.get("annotations") - if isinstance(annotations, dict): - risk_raw = annotations.get("risk") - try: - risk = RiskLevel(risk_raw) if isinstance(risk_raw, str) else RiskLevel.UNKNOWN - except ValueError: - risk = RiskLevel.UNKNOWN + # Accept safety annotations when present; never invent side effects / scope. + safety = extract_safety_annotations(raw) return ToolContract( name=name, description=str(raw.get("description", "")), parameters=_normalize_parameters(input_schema), output_schema=output_schema, - risk=risk, + risk=safety["risk"], + scope=safety["scope"], + side_effects=safety["side_effects"], + requires_confirmation=safety["requires_confirmation"], ) diff --git a/src/tool_semantics/models.py b/src/tool_semantics/models.py index 9c4eac5..6d9034d 100644 --- a/src/tool_semantics/models.py +++ b/src/tool_semantics/models.py @@ -13,6 +13,49 @@ class RiskLevel(StrEnum): UNKNOWN = "unknown" +class PermissionScope(StrEnum): + """Declared blast radius for a tool (#87). + + Ordered from narrowest to widest for escalation checks. ``unknown`` means + the field was absent — never invent a scope during capture. + """ + + UNKNOWN = "unknown" + RESOURCE = "resource" + PROJECT = "project" + WORKSPACE = "workspace" + ORGANIZATION = "organization" + ACCOUNT = "account" + GLOBAL = "global" + + +SCOPE_RANK: dict[PermissionScope, int] = { + PermissionScope.UNKNOWN: 0, + PermissionScope.RESOURCE: 1, + PermissionScope.PROJECT: 2, + PermissionScope.WORKSPACE: 3, + PermissionScope.ORGANIZATION: 4, + PermissionScope.ACCOUNT: 5, + PermissionScope.GLOBAL: 6, +} + + +# Documented side-effect vocabulary (open set — unknown strings are allowed). +KNOWN_SIDE_EFFECTS = frozenset( + { + "read", + "write", + "delete", + "network", + "email", + "payment", + "admin", + "execute", + "identity", + } +) + + class ToolParameter(BaseModel): model_config = ConfigDict(extra="allow", populate_by_name=True) @@ -30,6 +73,86 @@ class ToolContract(BaseModel): parameters: list[ToolParameter] = Field(default_factory=list) output_schema: dict[str, Any] | None = None risk: RiskLevel = RiskLevel.UNKNOWN + scope: PermissionScope = PermissionScope.UNKNOWN + side_effects: list[str] = Field(default_factory=list) + requires_confirmation: bool | None = None + + +def parse_permission_scope(value: Any) -> PermissionScope: + """Parse a scope annotation; unknown/invalid → ``unknown`` (never invent).""" + if not isinstance(value, str) or not value.strip(): + return PermissionScope.UNKNOWN + try: + return PermissionScope(value.strip().lower()) + except ValueError: + return PermissionScope.UNKNOWN + + +def parse_side_effects(value: Any) -> list[str]: + """Parse a side-effects list; absent/invalid → empty (undeclared).""" + if value is None: + return [] + if isinstance(value, str): + items = [value] + elif isinstance(value, list): + items = value + else: + return [] + normalized: list[str] = [] + seen: set[str] = set() + for item in items: + if not isinstance(item, str): + continue + effect = item.strip().lower() + if not effect or effect in seen: + continue + seen.add(effect) + normalized.append(effect) + return normalized + + +def parse_requires_confirmation(value: Any) -> bool | None: + """Parse confirmation flag; absent/invalid → ``None`` (unknown).""" + if value is None: + return None + if isinstance(value, bool): + return value + return None + + +def extract_safety_annotations(raw: dict[str, Any]) -> dict[str, Any]: + """Pull safety fields from a tool dict and optional ``annotations`` object. + + Never invent values — only copy what the source declares. + """ + annotations = raw.get("annotations") + ann = annotations if isinstance(annotations, dict) else {} + + risk_raw = raw.get("risk", ann.get("risk")) + scope_raw = raw.get("scope", ann.get("scope")) + side_raw = raw.get( + "side_effects", + raw.get("sideEffects", ann.get("side_effects", ann.get("sideEffects"))), + ) + confirm_raw = raw.get( + "requires_confirmation", + raw.get( + "requiresConfirmation", + ann.get("requires_confirmation", ann.get("requiresConfirmation")), + ), + ) + + try: + risk = RiskLevel(risk_raw) if isinstance(risk_raw, str) else RiskLevel.UNKNOWN + except ValueError: + risk = RiskLevel.UNKNOWN + + return { + "risk": risk, + "scope": parse_permission_scope(scope_raw), + "side_effects": parse_side_effects(side_raw), + "requires_confirmation": parse_requires_confirmation(confirm_raw), + } class PromptContract(BaseModel): diff --git a/src/tool_semantics/probes.py b/src/tool_semantics/probes.py index e700b35..6d860e6 100644 --- a/src/tool_semantics/probes.py +++ b/src/tool_semantics/probes.py @@ -6,7 +6,13 @@ from pydantic import BaseModel, Field -from tool_semantics.models import InterfaceSnapshot, RiskLevel, ToolContract +from tool_semantics.models import ( + SCOPE_RANK, + InterfaceSnapshot, + PermissionScope, + RiskLevel, + ToolContract, +) from tool_semantics.runner import ModelRunner, RunnerConfig, RunnerMetadata @@ -25,8 +31,10 @@ class Probe(BaseModel): expected_tool: str | None = None forbidden_tools: list[str] = Field(default_factory=list) required_params: list[str] = Field(default_factory=list) - # Side-effect / confirmation expectations (Milestone 3). + # Side-effect / confirmation expectations (Milestone 3 / #87). max_risk: str | None = None # read_only | external_write | destructive | unknown + max_scope: str | None = None # resource…global | unknown + forbidden_side_effects: list[str] = Field(default_factory=list) requires_confirmation: bool = False # Human-reviewed approval gate for model-backed execution (#44). approved: bool = False @@ -224,6 +232,49 @@ def evaluate_probes(snapshot: InterfaceSnapshot, probes: list[Probe]) -> ProbeRe ) ) continue + if probe.max_scope is not None: + try: + allowed_scope = PermissionScope(probe.max_scope) + except ValueError: + report.results.append( + ProbeResult( + probe_id=probe.id, + passed=False, + message=f"Invalid max_scope '{probe.max_scope}'.", + ) + ) + continue + if ( + tool.scope is not PermissionScope.UNKNOWN + and allowed_scope is not PermissionScope.UNKNOWN + and SCOPE_RANK[tool.scope] > SCOPE_RANK[allowed_scope] + ): + report.results.append( + ProbeResult( + probe_id=probe.id, + passed=False, + message=( + f"Tool '{tool.name}' scope '{tool.scope}' exceeds " + f"max_scope '{probe.max_scope}'." + ), + ) + ) + continue + if probe.forbidden_side_effects: + forbidden = {item.strip().lower() for item in probe.forbidden_side_effects if item} + hit = sorted(set(tool.side_effects) & forbidden) + if hit: + report.results.append( + ProbeResult( + probe_id=probe.id, + passed=False, + message=( + f"Tool '{tool.name}' declares forbidden side effect(s): " + f"{', '.join(hit)}." + ), + ) + ) + continue if probe.requires_confirmation and tool.risk in { RiskLevel.EXTERNAL_WRITE, RiskLevel.DESTRUCTIVE, diff --git a/src/tool_semantics/scanner.py b/src/tool_semantics/scanner.py index 19a61eb..dc334a4 100644 --- a/src/tool_semantics/scanner.py +++ b/src/tool_semantics/scanner.py @@ -4,7 +4,12 @@ from pathlib import Path from typing import Any -from tool_semantics.models import InterfaceSnapshot, ToolContract, ToolParameter +from tool_semantics.models import ( + InterfaceSnapshot, + ToolContract, + ToolParameter, + extract_safety_annotations, +) class ManifestError(ValueError): @@ -58,13 +63,17 @@ def capture_manifest(path: Path) -> InterfaceSnapshot: output_schema = raw_tool.get("outputSchema") if output_schema is not None and not isinstance(output_schema, dict): raise ManifestError(f"Tool '{raw_tool['name']}' outputSchema must be an object") + safety = extract_safety_annotations(raw_tool) tools.append( ToolContract( name=raw_tool["name"], description=str(raw_tool.get("description", "")), parameters=_normalize_parameters(input_schema), output_schema=output_schema, - risk=raw_tool.get("risk", "unknown"), + risk=safety["risk"], + scope=safety["scope"], + side_effects=safety["side_effects"], + requires_confirmation=safety["requires_confirmation"], ) ) diff --git a/tests/test_safety.py b/tests/test_safety.py new file mode 100644 index 0000000..ce80ce8 --- /dev/null +++ b/tests/test_safety.py @@ -0,0 +1,153 @@ +"""Tests for expanded safety semantics (#87).""" + +from __future__ import annotations + +from pathlib import Path + +from tool_semantics.diff import Severity, compare_snapshots +from tool_semantics.models import ( + InterfaceSnapshot, + PermissionScope, + RiskLevel, + ToolContract, + ToolParameter, + extract_safety_annotations, +) +from tool_semantics.probes import Probe, ProbeKind, evaluate_probes +from tool_semantics.scanner import capture_manifest + + +def _tool(**kwargs: object) -> ToolContract: + base = { + "name": "t", + "description": "tool", + "parameters": [ToolParameter(name="q", schema={"type": "string"})], + "risk": RiskLevel.READ_ONLY, + } + base.update(kwargs) + return ToolContract(**base) # type: ignore[arg-type] + + +def test_absent_safety_fields_default_unknown() -> None: + tool = ToolContract(name="x", description="") + assert tool.scope is PermissionScope.UNKNOWN + assert tool.side_effects == [] + assert tool.requires_confirmation is None + + +def test_extract_safety_from_annotations_never_invents() -> None: + raw = {"name": "t", "annotations": {"risk": "destructive", "scope": "organization"}} + safety = extract_safety_annotations(raw) + assert safety["risk"] is RiskLevel.DESTRUCTIVE + assert safety["scope"] is PermissionScope.ORGANIZATION + assert safety["side_effects"] == [] + assert safety["requires_confirmation"] is None + + +def test_scope_escalation_and_confirmation_removed() -> None: + baseline = InterfaceSnapshot( + server_name="s", + tools=[ + _tool( + name="delete", + scope=PermissionScope.PROJECT, + requires_confirmation=True, + side_effects=["write"], + ) + ], + ) + candidate = InterfaceSnapshot( + server_name="s", + tools=[ + _tool( + name="delete", + scope=PermissionScope.GLOBAL, + requires_confirmation=False, + side_effects=["write", "delete"], + ) + ], + ) + report = compare_snapshots(baseline, candidate) + codes = {change.code: change for change in report.changes} + assert codes["tool.scope_escalated"].severity is Severity.CRITICAL + assert codes["tool.confirmation_removed"].severity is Severity.CRITICAL + assert codes["tool.side_effect_added"].severity is Severity.CRITICAL + assert not report.is_compatible + + +def test_scope_unknown_change_is_warning_not_escalation() -> None: + baseline = InterfaceSnapshot( + server_name="s", + tools=[_tool(name="t", scope=PermissionScope.UNKNOWN)], + ) + candidate = InterfaceSnapshot( + server_name="s", + tools=[_tool(name="t", scope=PermissionScope.PROJECT)], + ) + report = compare_snapshots(baseline, candidate) + assert any(change.code == "tool.scope_changed" for change in report.changes) + assert not any(change.code == "tool.scope_escalated" for change in report.changes) + + +def test_probe_max_scope_and_forbidden_side_effects() -> None: + snapshot = InterfaceSnapshot( + server_name="s", + tools=[ + _tool( + name="pay", + scope=PermissionScope.ACCOUNT, + side_effects=["payment"], + risk=RiskLevel.EXTERNAL_WRITE, + ) + ], + ) + probes = [ + Probe( + id="p1", + intent="pay invoice", + kind=ProbeKind.POSITIVE, + expected_tool="pay", + max_scope="workspace", + ), + Probe( + id="p2", + intent="pay invoice", + kind=ProbeKind.POSITIVE, + expected_tool="pay", + forbidden_side_effects=["payment"], + ), + ] + report = evaluate_probes(snapshot, probes) + assert not report.passed + messages = " ".join(item.message for item in report.results) + assert "max_scope" in messages + assert "forbidden side effect" in messages + + +def test_manifest_capture_reads_safety_fields(tmp_path: Path) -> None: + path = tmp_path / "m.json" + path.write_text( + """ +{ + "serverName": "demo", + "tools": [ + { + "name": "wipe", + "description": "delete stuff", + "inputSchema": {"type": "object", "properties": {}}, + "risk": "destructive", + "scope": "organization", + "side_effects": ["delete", "admin"], + "requires_confirmation": true + } + ] +} +""".strip(), + encoding="utf-8", + ) + snap = capture_manifest(path) + tool = snap.tools[0] + assert tool.risk is RiskLevel.DESTRUCTIVE + assert tool.scope is PermissionScope.ORGANIZATION + assert tool.side_effects == ["delete", "admin"] + assert tool.requires_confirmation is True