diff --git a/cli/tui/app.py b/cli/tui/app.py index f002cb840..687be9c47 100644 --- a/cli/tui/app.py +++ b/cli/tui/app.py @@ -506,6 +506,8 @@ def agent_preset_overview(self) -> str: continue marker = "●" if preset.id == current else "○" face = ", ".join(preset.tools) if preset.tools is not None else "all tools" + if preset.disallowed_tools: + face = f"{face} except {', '.join(preset.disallowed_tools)}" lines.append( f" {marker} {preset.id} [{preset.trust}] — " f"{preset.description or preset.display_name} ({face})" diff --git a/cli/tui/commands.py b/cli/tui/commands.py index 0c11a67f9..f75f40339 100644 --- a/cli/tui/commands.py +++ b/cli/tui/commands.py @@ -591,6 +591,8 @@ async def _preset_via_picker(app) -> str | None: ) continue face = ", ".join(preset.tools) if preset.tools is not None else "all tools" + if preset.disallowed_tools: + face = f"{face} except {', '.join(preset.disallowed_tools)}" detail = ( f"[{preset.trust}] {preset.description or preset.display_name} ({face})" ) diff --git a/core/agent_presets/__init__.py b/core/agent_presets/__init__.py index eab1d83f2..9e78ec15b 100644 --- a/core/agent_presets/__init__.py +++ b/core/agent_presets/__init__.py @@ -102,6 +102,7 @@ class AgentPreset: prompt: str = "" prompt_mode: str = PROMPT_MODE_APPEND tools: tuple[str, ...] | None = None + disallowed_tools: tuple[str, ...] | None = None allow_spawn: bool | None = None suggested_model: str | None = None order: int | None = None @@ -116,6 +117,7 @@ def snapshot(self) -> AgentPresetSnapshot: prompt_mode=self.prompt_mode, tools=self.tools, allow_spawn=self.allow_spawn, + disallowed_tools=self.disallowed_tools, ) @@ -127,6 +129,7 @@ class AgentPresetSnapshot: prompt: str = "" prompt_mode: str = PROMPT_MODE_APPEND tools: tuple[str, ...] | None = None + disallowed_tools: tuple[str, ...] | None = None allow_spawn: bool | None = None def to_metadata(self) -> dict[str, Any]: @@ -135,6 +138,11 @@ def to_metadata(self) -> dict[str, Any]: "prompt": self.prompt, "promptMode": self.prompt_mode, "tools": list(self.tools) if self.tools is not None else None, + "disallowedTools": ( + list(self.disallowed_tools) + if self.disallowed_tools is not None + else None + ), "allowSpawn": self.allow_spawn, } @@ -152,6 +160,10 @@ def from_metadata(cls, value: Any) -> AgentPresetSnapshot | None: tools: tuple[str, ...] | None = None if isinstance(raw_tools, list): tools = tuple(str(name) for name in raw_tools) + raw_disallowed = value.get("disallowedTools") + disallowed_tools: tuple[str, ...] | None = None + if isinstance(raw_disallowed, list): + disallowed_tools = tuple(str(name) for name in raw_disallowed) mode = value.get("promptMode") allow_spawn = value.get("allowSpawn") return cls( @@ -159,12 +171,20 @@ def from_metadata(cls, value: Any) -> AgentPresetSnapshot | None: prompt=str(value.get("prompt") or ""), prompt_mode=(mode if mode in _PROMPT_MODES else PROMPT_MODE_APPEND), tools=tools, + disallowed_tools=disallowed_tools, allow_spawn=allow_spawn if isinstance(allow_spawn, bool) else None, ) def fingerprint(self) -> tuple[Any, ...]: """Hashable identity for runtime-key comparison.""" - return (self.id, self.prompt, self.prompt_mode, self.tools, self.allow_spawn) + return ( + self.id, + self.prompt, + self.prompt_mode, + self.tools, + self.disallowed_tools, + self.allow_spawn, + ) def compose_system_prompt(self, base: str) -> str: if not self.prompt: @@ -175,13 +195,23 @@ def compose_system_prompt(self, base: str) -> str: return f"{base}\n\n## Persona\n{self.prompt}" def tool_filter(self) -> Any | None: - """AgentRunSpec-contract narrowing filter, or None when unrestricted.""" - if self.tools is None: + """AgentRunSpec-contract narrowing filter, or None when unrestricted. + + ``tools`` is an allowlist; ``disallowed_tools`` subtracts from it — + or applies alone when no allowlist is present — matching the + cross-product ``disallowedTools`` frontmatter dialect. + """ + if self.tools is None and self.disallowed_tools is None: return None - allowed = frozenset(self.tools) + allowed = frozenset(self.tools) if self.tools is not None else None + denied = frozenset(self.disallowed_tools or ()) def preset_filter(names: tuple[str, ...]) -> tuple[str, ...]: - return tuple(name for name in names if name in allowed) + return tuple( + name + for name in names + if name not in denied and (allowed is None or name in allowed) + ) return preset_filter @@ -323,6 +353,40 @@ def _parse_preset_file(path: Path, *, trust: str) -> AgentPreset: broken="allow-spawn must be a boolean", ) + # ``disallowed-tools`` is a RESTRICTION, so it parses strictly like + # ``tools:`` — a malformed value marks the preset broken instead of + # silently dropping the deny list (fail closed: the agent would otherwise + # keep tools the author tried to remove). ``*`` is rejected for the same + # reason: it would mean "deny everything", which the subtraction filter + # cannot express, and the unrestricted fallback would invert the intent. + # ``max-turns``/``memory`` are deliberately NOT parsed — nothing reads + # them, and Claude Code's ``memory`` is a scope string, not a bool. + # ``permission-mode`` likewise stays out: permission remains an + # independent session-level knob (dsh's split). + raw_disallowed = data.get( + "disallowed-tools", + data.get("disallowedTools", data.get("disallowed_tools")), + ) + disallowed_tools, disallowed_error = _parse_tools( + raw_disallowed, field="disallowed-tools" + ) + # ``*`` must die in BOTH spellings: the bare string folds to ``None`` + # (unrestricted), while ``["*"]`` would normalize to ``("*",)`` — a deny + # entry no roster ever matches, i.e. it denies nothing. Both invert the + # author's intent, so both mark the preset broken. + if ( + disallowed_error is None + and raw_disallowed is not None + and (disallowed_tools is None or "*" in disallowed_tools) + ): + disallowed_error = ( + "disallowed-tools: '*' would deny every tool; list tools explicitly" + ) + if disallowed_error is not None: + return AgentPreset( + id=preset_id, trust=trust, path=str(path), broken=disallowed_error + ) + order = data.get("order") model = data.get("model") return AgentPreset( @@ -334,14 +398,17 @@ def _parse_preset_file(path: Path, *, trust: str) -> AgentPreset: prompt=text[match.end() :].strip(), prompt_mode=mode or PROMPT_MODE_APPEND, tools=tools, + disallowed_tools=disallowed_tools, allow_spawn=allow_spawn, suggested_model=str(model) if isinstance(model, str) and model else None, order=order if isinstance(order, int) else None, ) -def _parse_tools(value: Any) -> tuple[tuple[str, ...] | None, str | None]: - """``tools:`` field → normalized allowlist. Absent or ``*`` = unrestricted.""" +def _parse_tools( + value: Any, *, field: str = "tools" +) -> tuple[tuple[str, ...] | None, str | None]: + """Tool-name list field → normalized names. Absent or ``*`` = unrestricted.""" if value is None: return None, None if isinstance(value, str): @@ -350,10 +417,10 @@ def _parse_tools(value: Any) -> tuple[tuple[str, ...] | None, str | None]: names = [part for part in re.split(r"[,\s]+", value) if part] elif isinstance(value, list): if any(not isinstance(item, str) for item in value): - return None, "tools list entries must be strings" + return None, f"{field} list entries must be strings" names = [item for item in value if item.strip()] else: - return None, "tools must be a string or a list of strings" + return None, f"{field} must be a string or a list of strings" normalized: list[str] = [] for name in names: folded = normalize_tool_name(name) diff --git a/core/application/views.py b/core/application/views.py index 32f6d1eae..26eef9f2b 100644 --- a/core/application/views.py +++ b/core/application/views.py @@ -274,6 +274,11 @@ def agent_preset_view(preset) -> dict[str, Any]: "name": preset.display_name or preset.id, "description": preset.description, "tools": list(preset.tools) if preset.tools is not None else None, + "disallowedTools": ( + list(preset.disallowed_tools) + if preset.disallowed_tools is not None + else None + ), "broken": preset.broken, } diff --git a/desktop/src/features/presets/PresetPicker.test.tsx b/desktop/src/features/presets/PresetPicker.test.tsx index 9d526c141..5a80825d9 100644 --- a/desktop/src/features/presets/PresetPicker.test.tsx +++ b/desktop/src/features/presets/PresetPicker.test.tsx @@ -12,6 +12,7 @@ const reader: AgentPresetEntry = { name: "Code reader", description: "Read-only investigator", tools: ["read", "grep", "glob"], + disallowedTools: null, broken: null, }; @@ -21,6 +22,7 @@ const damaged: AgentPresetEntry = { name: "damaged", description: "", tools: null, + disallowedTools: null, broken: "missing YAML frontmatter block", }; diff --git a/desktop/src/generated/app-server.ts b/desktop/src/generated/app-server.ts index 40d3ff3f6..12c6dabcb 100644 --- a/desktop/src/generated/app-server.ts +++ b/desktop/src/generated/app-server.ts @@ -1149,6 +1149,7 @@ export interface AgentPresetEntry { name: string; description: string; tools: string[] | null; + disallowedTools: string[] | null; broken: string | null; } export interface PresetCurrentResult { diff --git a/protocol/app-server.schema.json b/protocol/app-server.schema.json index 9bfcc4c61..30e6489b1 100644 --- a/protocol/app-server.schema.json +++ b/protocol/app-server.schema.json @@ -5900,6 +5900,15 @@ "type": "string" } }, + "disallowedTools": { + "type": [ + "array", + "null" + ], + "items": { + "type": "string" + } + }, "broken": { "type": [ "string", @@ -5913,6 +5922,7 @@ "name", "description", "tools", + "disallowedTools", "broken" ] }, diff --git a/tests/app_server/test_preset_methods.py b/tests/app_server/test_preset_methods.py index 2790bc396..2d98920a8 100644 --- a/tests/app_server/test_preset_methods.py +++ b/tests/app_server/test_preset_methods.py @@ -29,6 +29,17 @@ def _messages(sink: io.BytesIO) -> list[dict[str, Any]]: def test_preset_roster_selection_and_lock_over_the_wire(tmp_path: Path) -> None: workspace = tmp_path / "workspace" workspace.mkdir() + # A deny-only project preset: the wire must carry the restriction, not + # claim "all tools" (the reviewer's display point, verified end to end). + project_preset = workspace / ".agents" / "presets" + project_preset.mkdir(parents=True) + (project_preset / "no-writes.md").write_text( + "---\n" + "description: no writes\n" + "disallowed-tools: write, edit\n" + "---\nYou may not write files.", + encoding="utf-8", + ) application = DeepCodeApplication.open(tmp_path / "state.sqlite3") project = application.projects.add(str(workspace)) thread = application.threads.start(project.id, title="Preset wire test") @@ -69,6 +80,12 @@ def test_preset_roster_selection_and_lock_over_the_wire(tmp_path: Path) -> None: roster = {entry["id"]: entry for entry in responses[2]["result"]["presets"]} assert "code-reader" in roster and roster["code-reader"]["trust"] == "system" assert roster["code-reader"]["tools"] == ["read", "grep", "glob", "skill"] + # Presets without a deny list report null (absent), never an empty claim. + assert roster["code-reader"]["disallowedTools"] is None + # Deny-only preset: no allowlist, but the restriction is on the wire. + assert roster["no-writes"]["tools"] is None + assert roster["no-writes"]["disallowedTools"] == ["write", "edit"] + assert roster["no-writes"]["broken"] is None assert responses[3]["result"]["agentPreset"] == "code-reader" assert responses[4]["result"]["agentPreset"] == "code-reader" diff --git a/tests/test_agent_presets.py b/tests/test_agent_presets.py index 235eb7583..5316b3cc6 100644 --- a/tests/test_agent_presets.py +++ b/tests/test_agent_presets.py @@ -336,3 +336,173 @@ async def exercise() -> None: agent_preset=None, ) assert first != second + + +# --------------------------------------------------------------------------- +# Subagent dialect: disallowed-tools (strict) / max-turns+memory (dropped) +# --------------------------------------------------------------------------- + + +def test_disallowed_tools_load_and_unparsed_dialect_keys_stay_out( + tmp_path: Path, +) -> None: + workspace = tmp_path / "ws" + _write_preset( + workspace / ".agents" / "presets", + "reviewer", + "---\n" + "name: reviewer\n" + "description: cross-dialect fields\n" + "tools: read, bash\n" + "disallowed-tools: write, edit\n" + "max-turns: 12\n" # deliberately NOT parsed: nothing reads it + "memory: true\n" # deliberately NOT parsed (Claude Code: scope string) + "permission-mode: plan\n" # deliberately NOT parsed into the preset + "---\nYou review code.", + ) + snapshot = resolve_agent_preset("reviewer", workspace) + assert snapshot.tools == ("read", "bash") + assert snapshot.disallowed_tools == ("write", "edit") + # The dropped fields leave no trace — nothing may look enforced. + assert not hasattr(snapshot, "max_turns") + assert not hasattr(snapshot, "memory") + # Permission stays an independent session-level knob (dsh's split): + # the preset never carries it. + assert not hasattr(snapshot, "permission_mode") + + +def test_malformed_disallowed_tools_marks_preset_broken(tmp_path: Path) -> None: + # Fail closed: a restriction that cannot be parsed must not silently + # vanish (the agent would keep the tools the author tried to remove). + workspace = tmp_path / "ws" + _write_preset( + workspace / ".agents" / "presets", + "sloppy", + "---\n" + "disallowed-tools: 42\n" # not a string or list -> broken + "---\nbody", + ) + roster = {p.id: p for p in list_agent_presets(workspace)} + assert roster["sloppy"].broken == ( + "disallowed-tools must be a string or a list of strings" + ) + with pytest.raises(BrokenPresetError, match="sloppy"): + resolve_agent_preset("sloppy", workspace) + + +def test_non_string_entry_in_disallowed_tools_marks_preset_broken( + tmp_path: Path, +) -> None: + # ``disallowed-tools: [Bash, 3]`` — the reviewer's example: one bad + # entry must break the preset, not drop the deny list. + workspace = tmp_path / "ws" + _write_preset( + workspace / ".agents" / "presets", + "mixed", + "---\ndisallowed-tools: [Bash, 3]\n---\nbody", + ) + roster = {p.id: p for p in list_agent_presets(workspace)} + assert roster["mixed"].broken == "disallowed-tools list entries must be strings" + + +def test_star_disallowed_tools_marks_preset_broken(tmp_path: Path) -> None: + # ``*`` would mean "deny everything"; the subtraction filter cannot + # express that, and both spellings fall through to a deny-nothing or + # unrestricted interpretation — fail closed on both. + workspace = tmp_path / "ws" + _write_preset( + workspace / ".agents" / "presets", + "star-string", + "---\ndisallowed-tools: '*'\n---\nbody", + ) + _write_preset( + workspace / ".agents" / "presets", + "star-list", + "---\ndisallowed-tools: ['*']\n---\nbody", + ) + roster = {p.id: p for p in list_agent_presets(workspace)} + message = "disallowed-tools: '*' would deny every tool; list tools explicitly" + assert roster["star-string"].broken == message + assert roster["star-list"].broken == message + + +def test_unparsed_dialect_keys_never_break_a_preset(tmp_path: Path) -> None: + # max-turns/memory are simply not read: garbage there is inert, not + # broken (unknown frontmatter keys are tolerated). + workspace = tmp_path / "ws" + _write_preset( + workspace / ".agents" / "presets", + "sloppy", + "---\nmax-turns: later\nmemory: sometimes\n---\nbody", + ) + roster = {p.id: p for p in list_agent_presets(workspace)} + assert roster["sloppy"].broken is None + assert roster["sloppy"].disallowed_tools is None + + +def test_key_variant_precedence_is_kebab_then_camel_then_snake( + tmp_path: Path, +) -> None: + workspace = tmp_path / "ws" + _write_preset( + workspace / ".agents" / "presets", + "variants", + "---\n" + "disallowed-tools: Write\n" + "disallowedTools: Edit\n" + "disallowed_tools: Bash\n" + "---\n", + ) + snapshot = resolve_agent_preset("variants", workspace) + assert snapshot.disallowed_tools == ("write",) + + # Without kebab, camel wins over snake. + _write_preset( + workspace / ".agents" / "presets", + "variants2", + "---\ndisallowedTools: Edit\ndisallowed_tools: Bash\n---\n", + ) + snapshot = resolve_agent_preset("variants2", workspace) + assert snapshot.disallowed_tools == ("edit",) + + +def test_tool_filter_subtracts_disallowed_from_any_roster() -> None: + # Allowlist AND NOT disallowed: the deny side still wins. + both = AgentPresetSnapshot( + id="p", tools=("read", "bash"), disallowed_tools=("write", "edit") + ) + assert both.tool_filter()(("read", "bash", "write", "glob")) == ("read", "bash") + # Disallowed-only narrows an arbitrary roster. + deny_only = AgentPresetSnapshot(id="q", disallowed_tools=("write",)) + assert deny_only.tool_filter()(("read", "write", "bash")) == ("read", "bash") + # No declaration at all -> unrestricted. + assert AgentPresetSnapshot(id="r").tool_filter() is None + + +def test_snapshot_roundtrip_and_fingerprint_cover_subagent_fields() -> None: + snapshot = AgentPresetSnapshot( + id="p", + tools=("read",), + disallowed_tools=("write",), + ) + metadata = snapshot.to_metadata() + assert metadata["disallowedTools"] == ["write"] + # The dropped fields are gone from the wire — nothing may look enforced. + assert "maxTurns" not in metadata + assert "memory" not in metadata + + decoded = AgentPresetSnapshot.from_metadata(metadata) + assert decoded == snapshot + assert decoded.fingerprint() == snapshot.fingerprint() + + # A different deny-list yields a different runtime key (no stale reuse). + other = AgentPresetSnapshot(id="p", tools=("read",), disallowed_tools=("edit",)) + assert other.fingerprint() != snapshot.fingerprint() + + # Tolerant decode: garbage deny list degrades to unset; stale keys from + # older sessions (maxTurns/memory) are ignored. + decoded = AgentPresetSnapshot.from_metadata( + {"id": "p", "disallowedTools": "x", "maxTurns": -1, "memory": "yes"} + ) + assert decoded is not None + assert decoded.disallowed_tools is None diff --git a/tests/test_tui.py b/tests/test_tui.py index 4c1413c7a..d40eefa1d 100644 --- a/tests/test_tui.py +++ b/tests/test_tui.py @@ -1883,3 +1883,34 @@ def listing(session_id, title, current=False): local_items = list(seen["scopes"][0].items) assert local_items[0].disabled_reason == "current session" assert local_items[1].disabled_reason is None + + +def test_agent_preset_overview_shows_the_deny_list(tmp_path: Path, monkeypatch) -> None: + from types import SimpleNamespace + + monkeypatch.setenv("DEEPCODE_HOME", str(tmp_path / "home")) + presets = tmp_path / ".agents" / "presets" + presets.mkdir(parents=True) + (presets / "deny-only.md").write_text( + "---\ndescription: deny only\ndisallowed-tools: write, edit\n---\nbody", + encoding="utf-8", + ) + (presets / "allow-and-deny.md").write_text( + "---\n" + "description: allow and deny\n" + "tools: read, bash\n" + "disallowed-tools: write\n" + "---\nbody", + encoding="utf-8", + ) + stub = SimpleNamespace( + thread_client=SimpleNamespace( + workspace=str(tmp_path), + current_agent_preset_id=lambda: None, + ) + ) + # Unbound call: the method only touches thread_client, no TuiApp state. + overview = tui_app.TuiApp.agent_preset_overview(stub) + # A deny-only preset must not claim "all tools" (the reviewer's point). + assert "all tools except write, edit" in overview + assert "read, bash except write" in overview