From af768b357f2acb8d7c7e51314ebd01f8e8b68108 Mon Sep 17 00:00:00 2001 From: Devika Verma Date: Wed, 19 Aug 2026 01:54:01 +0530 Subject: [PATCH] OPE-111: gate mislabeled catalog tools, floor catalog writes against relaxing overrides --- coworker/connectors/tool_defs.py | 14 +++++-- coworker/risk.py | 26 +++++++++--- tests/test_connectors.py | 3 +- tests/test_egress_and_overrides.py | 62 ++++++++++++++++++++++++++++ tests/test_send_target_resolution.py | 3 +- 5 files changed, 96 insertions(+), 12 deletions(-) diff --git a/coworker/connectors/tool_defs.py b/coworker/connectors/tool_defs.py index d8b365eb..e9ef6e02 100644 --- a/coworker/connectors/tool_defs.py +++ b/coworker/connectors/tool_defs.py @@ -34,9 +34,11 @@ TOOL_DEFS: tuple[ConnectorToolDef, ...] = ( ), ConnectorToolDef( "browser", + # Egress, not a read: the URL is model-chosen, so the request itself can carry + # data off-machine (same reasoning as web_fetch, OPE-111). "browser_open_url", "Open URL", - "read", + "write", "Open a URL in the Playwright browser.", ), ConnectorToolDef( @@ -86,9 +88,11 @@ TOOL_DEFS: tuple[ConnectorToolDef, ...] = ( ), ConnectorToolDef( "browser", + # Writes an image file to a resolved path (creating parents) — a local write, + # whatever the pane shows (OPE-111). "browser_screenshot", "Screenshot", - "read", + "write", "Capture a browser screenshot.", ), ConnectorToolDef( @@ -142,16 +146,18 @@ TOOL_DEFS: tuple[ConnectorToolDef, ...] = ( ), ConnectorToolDef( "github", + # Writes a whole tree to local disk — reading GitHub, writing the machine (OPE-111). "github_clone", "Clone a repo", - "read", + "write", "Clone a repository into a session folder to explore the code.", ), ConnectorToolDef( "github", + # Mutates an existing working tree (OPE-111). "github_pull", "Update a clone", - "read", + "write", "Fast-forward an existing clone to the latest commits.", ), ConnectorToolDef( diff --git a/coworker/risk.py b/coworker/risk.py index 11da31db..46938976 100644 --- a/coworker/risk.py +++ b/coworker/risk.py @@ -52,22 +52,36 @@ _STRICTNESS: dict[RiskClass, int] = { RiskOverrides = Callable[[str], Optional["RiskClass"]] +def _catalog_floor(tool_name: str) -> Optional[RiskClass]: + """The floor a connector-catalog tool must not be relaxed below. Catalog writes are + EXTERNAL by construction (`approval_for_tool` → `requires_approval=True`), and letting + an override drop one to READ would switch off approval, the Auto-Approve reviewer, and + read-only mode in a single step (OPE-111). Lazy import: risk.py must stay importable + without the connectors package.""" + try: + from .connectors.tool_defs import _KIND_BY_NAME + except ImportError: # pragma: no cover - connectors always ship, but fail open to base + return None + kind = _KIND_BY_NAME.get(tool_name) + return RiskClass.EXTERNAL if kind is not None and kind != "read" else None + + def classify( tool_name: str, metadata: Any = None, overrides: Optional[RiskOverrides] = None ) -> RiskClass: """Effective risk of a tool call. A user override may *relax* a metadata/MCP tool (the intended use — quieting an over-cautious plug-in), but may only ever **tighten** a - built-in write/exec/egress tool, never loosen it. Downgrading a built-in write to a read - would switch off path scoping AND the read-only gate at once, so it is refused here. - Precedence otherwise: the by-name base table, then aisuite metadata - (`requires_approval` → external), else read.""" - base = _BASE.get(tool_name) + built-in write/exec/egress tool or a connector-catalog write, never loosen one. + Downgrading a write to a read would switch off path scoping AND the read-only gate at + once, so it is refused here. Precedence otherwise: the by-name base table, then aisuite + metadata (`requires_approval` → external), else read.""" + base = _BASE.get(tool_name) or _catalog_floor(tool_name) if overrides is not None: ov = overrides(tool_name) if ov is not None: if base is None or _STRICTNESS[ov] >= _STRICTNESS[base]: return ov - # A loosening override on a built-in is ignored: fall through to the base class. + # A loosening override on a floored tool is ignored: fall through to the base. if base is not None: return base if bool(getattr(metadata, "requires_approval", False)): diff --git a/tests/test_connectors.py b/tests/test_connectors.py index cb155fa6..252e124f 100644 --- a/tests/test_connectors.py +++ b/tests/test_connectors.py @@ -258,7 +258,8 @@ def test_engine_connector_tools_are_cowork_scoped(tmp_path): assert "browser_open_url" not in helper.registry.names() # §36: browser READS (registry kind) are free; interactions still gate. - assert cowork.registry.get("browser_open_url").metadata.requires_approval is False + # OPE-111: open_url is egress (model-chosen URL), so it gates like an interaction. + assert cowork.registry.get("browser_open_url").metadata.requires_approval is True assert cowork.registry.get("browser_snapshot").metadata.requires_approval is False assert cowork.registry.get("browser_click").metadata.requires_approval is True assert cowork.registry.get("browser_type").metadata.requires_approval is True diff --git a/tests/test_egress_and_overrides.py b/tests/test_egress_and_overrides.py index 7b46b723..1d5567c6 100644 --- a/tests/test_egress_and_overrides.py +++ b/tests/test_egress_and_overrides.py @@ -119,6 +119,68 @@ def test_override_may_tighten(tmp_path): assert classify("mcp__x__run", None, ov) is RiskClass.EXEC +# -- catalog effects vs UX labels (OPE-111) ------------------------------------- +# These connector tools were catalogued "read" while actually writing to disk or +# carrying model-chosen egress, which routed them around approval, the reviewer, +# root scoping, and read-only mode all at once. The names are pinned here so a +# future catalog edit can't quietly reopen the hole. +_CATALOG_WRITES_IN_DISGUISE = ( + "github_clone", # writes a repo tree to disk + "github_pull", # mutates a working tree + "browser_open_url", # model-chosen URL = egress, like web_fetch + "browser_screenshot", # writes an image file, creating parent dirs + "browser_upload_file", # sends a local file's contents off-machine +) + + +def test_disguised_catalog_writes_gate(): + from coworker.connectors.tool_defs import approval_for_tool + + for name in _CATALOG_WRITES_IN_DISGUISE: + assert approval_for_tool(name) is True, name + assert classify(name) is RiskClass.EXTERNAL, name + + +def test_catalog_floor_holds_even_with_lying_metadata(): + # The floor keys off the catalog, not the attached metadata — a mis-built + # requires_approval=False cannot un-gate a catalog write. + from types import SimpleNamespace + + meta = SimpleNamespace(requires_approval=False) + assert classify("github_clone", meta) is RiskClass.EXTERNAL + + +def test_override_cannot_relax_a_catalog_write(tmp_path): + ov = _override({"github_clone": RiskClass.READ}) + assert classify("github_clone", None, ov) is RiskClass.EXTERNAL + + eng = PermissionEngine(workspace_root=tmp_path, mode=Mode.PLAN, risk_overrides=ov) + d = eng.evaluate("github_clone", {"repo": "octo/repo"}, None) + assert not d.allowed # read-only mode still applies; the downgrade did nothing + + +def test_catalog_reads_stay_relaxed_and_unprompted(tmp_path): + from coworker.connectors.tool_defs import approval_for_tool + + for name in ("browser_read_url", "email_search", "github_get_issue"): + assert approval_for_tool(name) is False, name + assert classify(name) is RiskClass.READ, name + # And the plugin-relaxation path is untouched for genuine reads. + ov = _override({"email_search": RiskClass.READ}) + assert classify("email_search", None, ov) is RiskClass.READ + + +def test_disguised_writes_blocked_in_read_only_modes(tmp_path): + from types import SimpleNamespace + + meta = SimpleNamespace(requires_approval=True) + for mode in (Mode.DISCUSS, Mode.PLAN): + eng = PermissionEngine(workspace_root=tmp_path, mode=mode) + for name in _CATALOG_WRITES_IN_DISGUISE: + d = eng.evaluate(name, {}, meta) + assert not d.allowed, f"{name} ran in {mode.value} mode" + + # -- write-path extraction / scoping ------------------------------------------- def test_write_paths_simple_tools(): assert write_paths("write_file", {"path": "a.txt"}) == (["a.txt"], True) diff --git a/tests/test_send_target_resolution.py b/tests/test_send_target_resolution.py index 645f27a0..6498b006 100644 --- a/tests/test_send_target_resolution.py +++ b/tests/test_send_target_resolution.py @@ -45,8 +45,9 @@ def test_browser_automation_reads_are_free_interactions_gate(): assert ( tools["browser_snapshot"].__aisuite_tool_metadata__.requires_approval is False ) + # OPE-111: opening a model-chosen URL is egress, not a read — it gates. assert ( - tools["browser_open_url"].__aisuite_tool_metadata__.requires_approval is False + tools["browser_open_url"].__aisuite_tool_metadata__.requires_approval is True ) assert tools["browser_click"].__aisuite_tool_metadata__.requires_approval is True assert tools["browser_type"].__aisuite_tool_metadata__.requires_approval is True