From 767c89f553f14c8ed10596ebb2c1e567470490db Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:07:37 -0400 Subject: [PATCH 01/22] fix(deps): declare html2text in pyproject (P2-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit converter.py imports html2text at module load, but it was only in environment.yml — a clean pip install broke on first webfetch import. Add it to core dependencies + a packaging test asserting it's declared. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- pyproject.toml | 1 + tests/test_packaging.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 tests/test_packaging.py diff --git a/pyproject.toml b/pyproject.toml index 83b60b6..5a5b742 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,6 +38,7 @@ dependencies = [ "numpy>=1.26.0,<2.0", "feedparser>=6.0.0", "pypdf>=4.0.0", + "html2text>=2020.1.16", # eager import in tools/webfetch/converter.py "jupyter_client>=8.0.0", "ipykernel>=6.0.0", # Durable sessions: ADK DatabaseSessionService (SQLAlchemy async) needs an diff --git a/tests/test_packaging.py b/tests/test_packaging.py new file mode 100644 index 0000000..6653332 --- /dev/null +++ b/tests/test_packaging.py @@ -0,0 +1,36 @@ +"""Packaging consistency: every eagerly-imported third-party module must be a +declared dependency in pyproject.toml, so a clean ``pip install`` doesn't fail +on first import (a package built from pyproject alone won't see environment.yml). +""" +from __future__ import annotations + +import tomllib +from pathlib import Path + +_PYPROJECT = Path(__file__).resolve().parent.parent / "pyproject.toml" + + +def _declared_dependencies() -> list[str]: + data = tomllib.loads(_PYPROJECT.read_text()) + return list(data.get("project", {}).get("dependencies", [])) + + +def _dep_names() -> set[str]: + """Normalized distribution names from the dependency specifiers.""" + names = set() + for spec in _declared_dependencies(): + # Strip version/extras/markers: name is the leading run of allowed chars. + name = spec.split(";")[0].strip() + for sep in ("[", ">", "<", "=", "!", "~", " "): + name = name.split(sep)[0] + names.add(name.strip().lower()) + return names + + +def test_html2text_is_declared(): + """converter.py imports html2text at module load (tools/webfetch/converter.py).""" + assert "html2text" in _dep_names(), ( + "html2text is imported eagerly by tools/webfetch/converter.py but is not " + "declared in pyproject.toml [project.dependencies] — a clean pip install " + "breaks when the webfetch tools are imported." + ) From 8be9f70fc2f215d86882528b497d2315356149ad Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:14:11 -0400 Subject: [PATCH 02/22] fix(tools): contain glob/grep patterns to the authorized root (P0-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The permission engine authorizes only the `path` argument, but glob ran `search_path.glob(pattern)` and grep's Python fallback ran `path.glob(file_pattern)` — a pattern of `../*` or an absolute path escaped the granted directory (reproduced against a sibling dir). Reject absolute/`..` patterns at the source in both tools, and containment-check each resolved result (drops symlinks under the root that point outside). Shared helpers glob_pattern_escapes_root / path_is_within added to file_utils. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/file_utils.py | 29 +++++++ src/agentic_cli/tools/glob_tool.py | 15 ++++ src/agentic_cli/tools/grep_tool.py | 18 +++++ tests/tools/test_glob_grep_containment.py | 97 +++++++++++++++++++++++ 4 files changed, 159 insertions(+) create mode 100644 tests/tools/test_glob_grep_containment.py diff --git a/src/agentic_cli/file_utils.py b/src/agentic_cli/file_utils.py index 540aa8e..aa25e75 100644 --- a/src/agentic_cli/file_utils.py +++ b/src/agentic_cli/file_utils.py @@ -3,6 +3,7 @@ import fcntl import json import os +import re import tempfile import time from contextlib import contextmanager @@ -59,6 +60,34 @@ def sanitize_filename(name: str) -> str: return "".join(c if c.isalnum() or c in "-_" else "_" for c in name) +def glob_pattern_escapes_root(pattern: str) -> bool: + """True if a glob pattern would search outside its base directory. + + Rejects absolute patterns and any pattern containing a ``..`` path + component. ``pathlib.Path.glob`` does not normalize ``..``, so a pattern + like ``../*`` escapes the base directory even though the permission engine + only authorized that base. Callers should reject such patterns before + globbing. + """ + if os.path.isabs(pattern): + return True + return ".." in re.split(r"[\\/]+", pattern) + + +def path_is_within(path: Path, root: Path) -> bool: + """True if ``path`` resolves to a location inside ``root``. + + Both operands are fully resolved (following symlinks) before the check, so + a symlink under ``root`` that points outside is correctly rejected. Returns + False on any resolution error (loop, permission) — fail closed. + """ + try: + path.resolve().relative_to(root.resolve()) + return True + except (OSError, ValueError, RuntimeError): + return False + + def _atomic_write(path: Path, content: str) -> None: """Write content to a file atomically and durably. diff --git a/src/agentic_cli/tools/glob_tool.py b/src/agentic_cli/tools/glob_tool.py index 057e03e..3e0614b 100644 --- a/src/agentic_cli/tools/glob_tool.py +++ b/src/agentic_cli/tools/glob_tool.py @@ -9,6 +9,7 @@ from pathlib import Path from typing import Any, Literal +from agentic_cli.file_utils import glob_pattern_escapes_root, path_is_within from agentic_cli.tools.registry import ( ToolCategory, register_tool, @@ -72,12 +73,26 @@ def glob( "path": str(search_path), } + # The permission engine authorizes only `path`; a pattern like "../*" or an + # absolute pattern would escape that authorized root, so reject it. + if glob_pattern_escapes_root(pattern): + return { + "success": False, + "error": f"Pattern escapes the search root: {pattern!r}", + "path": str(search_path), + } + # Find matching files matches = list(search_path.glob(pattern)) # Filter results filtered = [] for match in matches: + # Drop anything resolving outside the authorized root (e.g. a symlink + # inside `path` that points elsewhere) — defense in depth. + if not path_is_within(match, search_path): + continue + # Skip hidden files if not requested if not include_hidden and match.name.startswith("."): continue diff --git a/src/agentic_cli/tools/grep_tool.py b/src/agentic_cli/tools/grep_tool.py index 81e93c1..76d5bde 100644 --- a/src/agentic_cli/tools/grep_tool.py +++ b/src/agentic_cli/tools/grep_tool.py @@ -10,6 +10,7 @@ from pathlib import Path from typing import Any, Literal +from agentic_cli.file_utils import glob_pattern_escapes_root, path_is_within from agentic_cli.tools.registry import ( ToolCategory, register_tool, @@ -70,6 +71,18 @@ def grep( "path": str(search_path), } + # The permission engine authorizes only `path`; a file_pattern like "../*" + # or an absolute pattern would escape that authorized root, so reject it. + if file_pattern and glob_pattern_escapes_root(file_pattern): + return { + "success": False, + "error": f"File pattern escapes the search root: {file_pattern!r}", + "matches": [], + "total_matches": 0, + "files_searched": 0, + "truncated": False, + } + # Try to use ripgrep if available (faster, respects .gitignore) if _ripgrep_available(): return _grep_with_ripgrep( @@ -276,6 +289,11 @@ def _grep_python( if not file_path.is_file(): continue + # Skip files resolving outside the authorized root (e.g. a symlink + # under `path` pointing elsewhere) — defense in depth. + if not path_is_within(file_path, path): + continue + try: content = file_path.read_text() lines = content.splitlines() diff --git a/tests/tools/test_glob_grep_containment.py b/tests/tools/test_glob_grep_containment.py new file mode 100644 index 0000000..a4d65cd --- /dev/null +++ b/tests/tools/test_glob_grep_containment.py @@ -0,0 +1,97 @@ +"""P0-4: glob/grep patterns must not escape the permission-authorized root. + +The permission engine authorizes only the ``path`` argument. A ``pattern`` / +``file_pattern`` of ``../*`` or an absolute path would let the tool read +outside the granted directory. These tests pin the containment behavior. +""" +from __future__ import annotations + +import agentic_cli.tools.grep_tool as grep_mod +from agentic_cli.tools.glob_tool import glob +from agentic_cli.tools.grep_tool import grep + + +def test_glob_rejects_parent_escape(tmp_path): + root = tmp_path / "root" + root.mkdir() + (root / "inside.txt").write_text("x") + (tmp_path / "secret.txt").write_text("SECRET") + + r = glob(pattern="../*", path=str(root)) + + assert r["success"] is False + assert "secret" not in str(r).lower() + + +def test_glob_rejects_absolute_pattern(tmp_path): + root = tmp_path / "root" + root.mkdir() + r = glob(pattern="/etc/*", path=str(root)) + assert r["success"] is False + + +def test_glob_normal_pattern_still_works(tmp_path): + root = tmp_path / "root" + root.mkdir() + (root / "a.py").write_text("x") + (root / "b.txt").write_text("y") + r = glob(pattern="*.py", path=str(root)) + assert r["success"] is True + assert r["files"] == ["a.py"] + + +def test_glob_recursive_pattern_still_works(tmp_path): + root = tmp_path / "root" + (root / "sub").mkdir(parents=True) + (root / "sub" / "deep.py").write_text("x") + r = glob(pattern="**/*.py", path=str(root)) + assert r["success"] is True + assert "sub/deep.py" in r["files"] + + +def test_glob_skips_symlink_escaping_root(tmp_path): + root = tmp_path / "root" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (outside / "passwd").write_text("SECRET") + (root / "link").symlink_to(outside, target_is_directory=True) + + r = glob(pattern="link/*", path=str(root)) + + # Pattern itself is legal, but the symlinked result resolves outside root. + assert r["success"] is True + assert all("passwd" not in str(f) for f in r["files"]) + + +def test_grep_rejects_parent_escape_file_pattern(tmp_path, monkeypatch): + root = tmp_path / "root" + root.mkdir() + (root / "a.txt").write_text("needle") + (tmp_path / "secret.txt").write_text("needle SECRET") + # Force the Python fallback — that's the path that follows ../ literally. + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: False) + + r = grep(pattern="needle", path=str(root), file_pattern="../*") + + assert r["success"] is False + + +def test_grep_python_skips_symlink_escaping_root(tmp_path, monkeypatch): + root = tmp_path / "root" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.txt").write_text("needle SECRET") + (root / "link.txt").symlink_to(outside / "secret.txt") + (root / "real.txt").write_text("needle here") + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: False) + + r = grep(pattern="needle", path=str(root)) + + assert r["success"] is True + files = {m["file"] for m in r["matches"]} + # link.txt resolves to outside/secret.txt (outside root) → must be skipped, + # even though its own path name doesn't contain "secret". + assert not any("link.txt" in f for f in files) + assert any("real.txt" in f for f in files) From a1a45bb8bd4973cd85e2a5a2936dad51060ae828 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:18:26 -0400 Subject: [PATCH 03/22] fix(document): harden compile_document host execution (P0-3) Three host-side gaps closed, keeping compilation decoupled from the container sandbox: - output_pdf delivery used shutil.copy2, which follows a destination symlink -> write-through-symlink primitive. Now a no-follow atomic write (temp file in dest dir + os.replace); a symlink at the output path is replaced, never written through. - the TeX subprocess inherited the full host environment (secrets, API keys) -> now an allowlisted env (PATH/HOME/TEXINPUTS/locale only). - the capability covered only source_path -> added optional filesystem.read (assets_dir) and filesystem.write (output_pdf). Adds a Capability.optional flag: an optional target arg that's absent/ empty is skipped in the engine's _resolve, so scoping the extra caps doesn't spuriously prompt when those args aren't supplied. latexmk stays the default engine (.latexmkrc risk remains a documented deferral). Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 68 ++++++++++++++++--- .../workflow/permissions/capabilities.py | 5 ++ .../workflow/permissions/engine.py | 4 ++ tests/permissions/test_engine.py | 47 +++++++++++++ tests/tools/test_document_compile.py | 59 +++++++++++++++- 5 files changed, 174 insertions(+), 9 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 6a86788..00176fe 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -12,6 +12,11 @@ structured result. It does NOT execute arbitrary code; a ``report_writer``-style agent uses it to turn an authored ``.tex`` into a PDF. +The subprocess receives only an allowlisted environment (``_ENV_PASSTHROUGH``), +not the full host environment, so host secrets aren't handed to the TeX process. +Delivery to ``output_pdf`` is a no-follow atomic write (temp file + ``os.replace``) +so an attacker-placed symlink at the destination can't redirect the write. + Provisioning is host-based: the engine must be on ``PATH`` (TeX Live / MacTeX). If neither is found the tool returns a structured error with an install hint. @@ -26,6 +31,7 @@ import shutil import signal import subprocess +import tempfile import time from pathlib import Path from typing import Any @@ -36,6 +42,51 @@ _ENGINES = ("latexmk", "pdflatex") _LOG_TAIL_LINES = 40 +# Only these host env vars reach the TeX process. The tool must not hand the +# whole host environment (API keys, tokens) to a subprocess that — on the +# latexmk path — can execute arbitrary Perl from a .latexmkrc. PATH/HOME are +# needed for the engine binary and kpathsea; TEXINPUTS is set explicitly. +_ENV_PASSTHROUGH = ( + "PATH", "HOME", "TERM", "TMPDIR", "TEMP", "TMP", + "LANG", "LC_ALL", "LC_CTYPE", "SOURCE_DATE_EPOCH", +) + + +def _build_env(assets_dir: str | None) -> dict[str, str]: + """Minimal, allowlisted environment for the TeX subprocess.""" + env = {k: os.environ[k] for k in _ENV_PASSTHROUGH if k in os.environ} + env.setdefault("PATH", os.defpath) + if assets_dir: + # Prepend assets_dir; the trailing empty entries let kpathsea append the + # default search path. A caller-inherited TEXINPUTS is intentionally + # dropped (not in the allowlist) so it can't redirect input resolution. + env["TEXINPUTS"] = f"{assets_dir}{os.pathsep}{os.pathsep}" + return env + + +def _deliver_no_follow(produced: Path, dest: Path) -> None: + """Copy ``produced`` to ``dest`` without following a symlink at ``dest``. + + Writes to a private temp file in dest's directory, then atomically renames + it over dest. ``os.replace`` swaps the destination *name*: if dest is a + symlink the link itself is replaced (not written through), so an + attacker-placed symlink can't redirect the write outside the intended path. + """ + dest.parent.mkdir(parents=True, exist_ok=True) + fd, tmp_name = tempfile.mkstemp( + dir=str(dest.parent), prefix=f".{dest.name}.", suffix=".tmp" + ) + tmp_path = Path(tmp_name) + try: + with os.fdopen(fd, "wb") as out, open(produced, "rb") as src: + shutil.copyfileobj(src, out) + out.flush() + os.fsync(out.fileno()) + os.replace(tmp_path, dest) + except BaseException: + tmp_path.unlink(missing_ok=True) + raise + def _which(name: str) -> str | None: """Locate an executable on PATH (seam for tests).""" @@ -89,7 +140,13 @@ def _parse_errors(log_text: str) -> list[str]: @register_tool( category=ToolCategory.EXECUTION, - capabilities=[Capability("document.compile", target_arg="source_path")], + capabilities=[ + Capability("document.compile", target_arg="source_path"), + # The tool also reads assets_dir and writes output_pdf when those are + # supplied; scope them explicitly (optional → not exercised when absent). + Capability("filesystem.read", target_arg="assets_dir", optional=True), + Capability("filesystem.write", target_arg="output_pdf", optional=True), + ], description=( "Compile a LaTeX source file to PDF using a host TeX engine (latexmk or " "pdflatex). Shell-escape is disabled for pdflatex (-no-shell-escape); " @@ -143,11 +200,7 @@ def compile_document( } work_dir = src.parent - env = dict(os.environ) - if assets_dir: - prev = env.get("TEXINPUTS", "") - # Prepend assets_dir; trailing empty entry preserves the default path. - env["TEXINPUTS"] = f"{assets_dir}{os.pathsep}{prev}{os.pathsep}" + env = _build_env(assets_dir) argv = _build_argv(chosen, src.name) start = time.monotonic() @@ -187,8 +240,7 @@ def compile_document( if output_pdf: dest = Path(output_pdf) try: - dest.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(produced, dest) + _deliver_no_follow(produced, dest) except OSError as exc: return { "success": False, diff --git a/src/agentic_cli/workflow/permissions/capabilities.py b/src/agentic_cli/workflow/permissions/capabilities.py index 0cb1de8..f07aa8e 100644 --- a/src/agentic_cli/workflow/permissions/capabilities.py +++ b/src/agentic_cli/workflow/permissions/capabilities.py @@ -21,6 +21,11 @@ class Capability: name: str # e.g. "filesystem.read" target_arg: str | None = None # arg name holding the target; None → target "*" + optional: bool = False # when the target arg is absent/empty, skip + # this capability (the side effect isn't + # performed) instead of resolving it to a + # spurious target. Only for genuinely + # optional args (e.g. an output path). @dataclass(frozen=True) diff --git a/src/agentic_cli/workflow/permissions/engine.py b/src/agentic_cli/workflow/permissions/engine.py index a1d3b8b..c4ef1f0 100644 --- a/src/agentic_cli/workflow/permissions/engine.py +++ b/src/agentic_cli/workflow/permissions/engine.py @@ -175,6 +175,10 @@ def _resolve( resolved.append(ResolvedCapability(cap.name, "*")) continue value = args.get(cap.target_arg, "") + if cap.optional and (value is None or value == ""): + # Optional target not supplied → the side effect isn't performed + # this call, so don't resolve (and don't spuriously prompt) it. + continue matcher = get_matcher(cap.name) items = value if isinstance(value, (list, tuple)) else [value] for item in items: diff --git a/tests/permissions/test_engine.py b/tests/permissions/test_engine.py index 6b1f030..4059769 100644 --- a/tests/permissions/test_engine.py +++ b/tests/permissions/test_engine.py @@ -469,3 +469,50 @@ async def test_reloaded_wildcard_rule_still_matches(self, ctx, tmp_path, monkeyp ) assert result.allowed is True w2.request_user_input.assert_not_called() + + +class TestOptionalCapability: + """A capability marked optional is only exercised when its target arg is + supplied — so a tool with an optional output/asset path doesn't prompt for a + write/read it isn't performing this call.""" + + def _engine(self, ctx): + return PermissionEngine( + settings=_stub_settings(), workflow=_stub_workflow(), ctx=ctx, + ) + + def test_optional_cap_skipped_when_arg_absent(self, ctx): + engine = self._engine(ctx) + resolved = engine._resolve( + [Capability("filesystem.write", target_arg="output_pdf", optional=True)], + {}, # output_pdf not supplied + ) + assert resolved == [] + + def test_optional_cap_skipped_when_arg_empty(self, ctx): + engine = self._engine(ctx) + resolved = engine._resolve( + [Capability("filesystem.write", target_arg="output_pdf", optional=True)], + {"output_pdf": None}, + ) + assert resolved == [] + + def test_optional_cap_resolved_when_arg_present(self, ctx, tmp_path): + engine = self._engine(ctx) + target = str(tmp_path / "out.pdf") + resolved = engine._resolve( + [Capability("filesystem.write", target_arg="output_pdf", optional=True)], + {"output_pdf": target}, + ) + assert len(resolved) == 1 + assert resolved[0].name == "filesystem.write" + + def test_required_cap_still_resolves_when_arg_absent(self, ctx): + """Non-optional (default) behavior is unchanged: an absent target still + resolves (to be evaluated/asked), never silently skipped.""" + engine = self._engine(ctx) + resolved = engine._resolve( + [Capability("filesystem.write", target_arg="path")], # optional=False + {}, + ) + assert len(resolved) == 1 diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index 1a66369..5d0e451 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -139,7 +139,8 @@ def fake_run(argv, *, cwd, env, timeout): return subprocess.CompletedProcess(argv, 0, stdout="ok", stderr="") monkeypatch.setattr(mod, "_run", fake_run) - monkeypatch.setattr(mod.shutil, "copy2", lambda src, dst: (_ for _ in ()).throw(OSError("disk full"))) + # Delivery is a no-follow atomic write; force the atomic rename to fail. + monkeypatch.setattr(mod.os, "replace", lambda src, dst: (_ for _ in ()).throw(OSError("disk full"))) r = compile_document(str(tex), output_pdf=str(out)) assert r["success"] is False @@ -189,3 +190,59 @@ def communicate(self, timeout=None): assert popen_kwargs.get("start_new_session") is True, "Popen must use start_new_session=True" assert len(killpg_calls) >= 1, "os.killpg must be called on timeout" + + +# --- P0-3 hardening: env allowlist, no-follow delivery, capability scope --- + +def test_env_is_allowlisted_not_full_environ(monkeypatch, tmp_path): + """Env inheritance leak: the TeX process must not receive host secrets; + PATH is preserved and assets_dir still reaches TEXINPUTS.""" + _fake_engine(monkeypatch) + monkeypatch.setenv("MY_SECRET_TOKEN", "sk-must-not-leak") + monkeypatch.setenv("PATH", "/custom/bin") + tex = tmp_path / "r.tex"; tex.write_text("x") + captured = {} + + def fake_run(argv, *, cwd, env, timeout): + captured["env"] = env + (Path(cwd) / "r.pdf").write_bytes(b"%PDF") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + compile_document(str(tex), assets_dir="/tmp/assets") + assert "MY_SECRET_TOKEN" not in captured["env"] + assert captured["env"]["PATH"] == "/custom/bin" + assert "/tmp/assets" in captured["env"]["TEXINPUTS"] + + +def test_delivery_does_not_follow_output_symlink(monkeypatch, tmp_path): + """output_pdf may be an attacker-placed symlink; delivery must not write + through it to the link target.""" + _fake_engine(monkeypatch) + tex = tmp_path / "r.tex"; tex.write_text("x") + outside = tmp_path / "outside.pdf"; outside.write_bytes(b"ORIGINAL") + deliver = tmp_path / "deliver"; deliver.mkdir() + link = deliver / "report.pdf"; link.symlink_to(outside) + + def fake_run(argv, *, cwd, env, timeout): + (Path(cwd) / "r.pdf").write_bytes(b"%PDF-NEW") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + r = compile_document(str(tex), output_pdf=str(link)) + assert r["success"] is True + assert outside.read_bytes() == b"ORIGINAL" # link target NOT overwritten + assert link.read_bytes() == b"%PDF-NEW" # PDF delivered to the path + assert not link.is_symlink() # symlink replaced by a real file + + +def test_capabilities_scope_assets_and_output(): + """document.compile alone under-authorizes: reading assets_dir and writing + output_pdf need explicit (optional) filesystem capabilities.""" + from agentic_cli.tools.registry import get_registry + + defn = get_registry().get("compile_document") + caps = {(c.name, c.target_arg, c.optional) for c in defn.capabilities} + assert ("document.compile", "source_path", False) in caps + assert ("filesystem.read", "assets_dir", True) in caps + assert ("filesystem.write", "output_pdf", True) in caps From 3a4434a68404e415b3f8103b9fb75483a4bc9f5d Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:22:49 -0400 Subject: [PATCH 04/22] fix(permissions): fail closed when the engine is missing (P1-7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both adapters (ADK PermissionPlugin, LangGraph wrap_tool_for_permission) treated an absent PermissionEngine as allow. base_manager always constructs the engine, so a missing engine with permissions enabled is a misconfiguration — allowing silently bypasses all gating. Now: engine absent + permissions_enabled -> deny; engine absent + permissions disabled -> allow (matches the master switch). Also covers the MCP synthetic-capability path. The three test_engine_absent_allows tests are rewritten to pin both halves of the new semantic. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- .../workflow/adk/permission_plugin.py | 26 +++++++++-- .../workflow/langgraph/permission_wrap.py | 17 ++++++- tests/integration/test_permission_adk.py | 46 +++++++++++++++++-- .../integration/test_permission_langgraph.py | 44 ++++++++++++++++-- tests/workflow/test_adk_mcp_permissions.py | 27 ++++++++++- 5 files changed, 144 insertions(+), 16 deletions(-) diff --git a/src/agentic_cli/workflow/adk/permission_plugin.py b/src/agentic_cli/workflow/adk/permission_plugin.py index cfab22f..1088e10 100644 --- a/src/agentic_cli/workflow/adk/permission_plugin.py +++ b/src/agentic_cli/workflow/adk/permission_plugin.py @@ -5,7 +5,8 @@ 2. Unregistered MCP toolset tool → gate through the engine under a synthetic ``mcp`` capability (no rule → ASK). 3. Tool has no capability declaration → deny (author error, loud). -4. Engine absent from service registry → allow (test/dev fallback). +4. Engine absent from service registry → fail closed (deny) when permissions + are enabled; allow only when permissions are disabled. 5. Otherwise call engine.check() and return None on allow, error dict on deny. """ @@ -59,6 +60,25 @@ def _is_mcp_tool(tool: "BaseTool") -> bool: _MCP_TARGET_ARG = "__mcp_target__" +def _no_engine_result(tool_name: str) -> dict | None: + """Return value when the permission engine is absent from the registry. + + Fail closed: if permissions are enabled but no engine is wired, deny the + call. In production ``base_manager`` always constructs the engine, so a + missing engine with permissions on is a misconfiguration — allowing would + silently bypass all gating. Only permissions-disabled allows (None). + """ + from agentic_cli.config import get_settings + + if get_settings().permissions_enabled: + logger.warning("permission_engine_missing", tool=tool_name) + return { + "success": False, + "error": "Permission denied: permission engine unavailable", + } + return None + + class PermissionPlugin(BasePlugin): """ADK plugin: gates every tool call through :class:`PermissionEngine`.""" @@ -91,7 +111,7 @@ async def before_tool_callback( engine = get_service(PERMISSION_ENGINE) if engine is None: - return None # test/dev fallback + return _no_engine_result(tool.name) result = await engine.check(tool.name, caps, tool_args) if result.allowed: @@ -102,7 +122,7 @@ async def _check_mcp(self, tool: "BaseTool") -> dict | None: """Gate an MCP tool through the engine under a synthetic capability.""" engine = get_service(PERMISSION_ENGINE) if engine is None: - return None # test/dev fallback + return _no_engine_result(tool.name) caps = [Capability("mcp", target_arg=_MCP_TARGET_ARG)] result = await engine.check(tool.name, caps, {_MCP_TARGET_ARG: tool.name}) if result.allowed: diff --git a/src/agentic_cli/workflow/langgraph/permission_wrap.py b/src/agentic_cli/workflow/langgraph/permission_wrap.py index 08600fa..d04d6b7 100644 --- a/src/agentic_cli/workflow/langgraph/permission_wrap.py +++ b/src/agentic_cli/workflow/langgraph/permission_wrap.py @@ -4,7 +4,8 @@ Adapter check order matches ADK's PermissionPlugin: 1. EXEMPT tool → returned unwrapped (no engine call ever). 2. Tool has no capability declaration → wrapper returns deny dict at call time. -3. Engine absent from service registry → wrapper runs the original tool (fallback). +3. Engine absent from service registry → fail closed (deny) when permissions + are enabled; run the tool only when permissions are disabled. 4. Otherwise call engine.check(); return on allow, deny dict on deny. """ @@ -47,7 +48,19 @@ async def _guarded(*args: Any, **kwargs: Any) -> Any: "error": "Permission denied: tool has no capability declaration", } engine = get_service(PERMISSION_ENGINE) - if engine is not None: + if engine is None: + # Fail closed: permissions on but no engine wired is a + # misconfiguration (base_manager always builds one) — deny rather + # than run ungated. Only permissions-disabled falls through to run. + from agentic_cli.config import get_settings + + if get_settings().permissions_enabled: + logger.warning("permission_engine_missing", tool=name) + return { + "success": False, + "error": "Permission denied: permission engine unavailable", + } + else: result = await engine.check(name, caps, kwargs) if not result.allowed: return {"success": False, "error": f"Permission denied: {result.reason}"} diff --git a/tests/integration/test_permission_adk.py b/tests/integration/test_permission_adk.py index 5d36ade..02ffb8e 100644 --- a/tests/integration/test_permission_adk.py +++ b/tests/integration/test_permission_adk.py @@ -115,7 +115,8 @@ def writer_x(path: str): assert result == {"success": False, "error": "Permission denied: rule: builtin/deny"} @pytest.mark.asyncio - async def test_engine_absent_allows(self, monkeypatch): + async def test_engine_absent_denies_when_permissions_enabled(self, monkeypatch): + """Fail closed: permissions on but no engine wired -> deny, not allow.""" from agentic_cli.tools.registry import get_registry from agentic_cli.workflow.adk.permission_plugin import PermissionPlugin @@ -123,19 +124,54 @@ async def test_engine_absent_allows(self, monkeypatch): "agentic_cli.workflow.adk.permission_plugin.get_service", lambda k: None, ) + monkeypatch.setattr( + "agentic_cli.config.get_settings", + lambda: SimpleNamespace(permissions_enabled=True), + ) + reg = get_registry() + + @reg.register( + name="reader_y_deny", + capabilities=[Capability("filesystem.read", target_arg="path")], + ) + def reader_y_deny(path: str): + return {} + + plugin = PermissionPlugin() + result = await plugin.before_tool_callback( + tool=SimpleNamespace(name="reader_y_deny"), + tool_args={"path": "/tmp/x"}, + tool_context=None, + ) + assert result is not None and result["success"] is False + + @pytest.mark.asyncio + async def test_engine_absent_allows_when_permissions_disabled(self, monkeypatch): + """Permissions off is the only case a missing engine allows.""" + from agentic_cli.tools.registry import get_registry + from agentic_cli.workflow.adk.permission_plugin import PermissionPlugin + + monkeypatch.setattr( + "agentic_cli.workflow.adk.permission_plugin.get_service", + lambda k: None, + ) + monkeypatch.setattr( + "agentic_cli.config.get_settings", + lambda: SimpleNamespace(permissions_enabled=False), + ) reg = get_registry() @reg.register( - name="reader_y", + name="reader_y_allow", capabilities=[Capability("filesystem.read", target_arg="path")], ) - def reader_y(path: str): + def reader_y_allow(path: str): return {} plugin = PermissionPlugin() result = await plugin.before_tool_callback( - tool=SimpleNamespace(name="reader_y"), + tool=SimpleNamespace(name="reader_y_allow"), tool_args={"path": "/tmp/x"}, tool_context=None, ) - assert result is None # fallback allow + assert result is None diff --git a/tests/integration/test_permission_langgraph.py b/tests/integration/test_permission_langgraph.py index a9c4e45..4b6765e 100644 --- a/tests/integration/test_permission_langgraph.py +++ b/tests/integration/test_permission_langgraph.py @@ -88,7 +88,10 @@ def write_lg(path: str): assert result == {"success": False, "error": "Permission denied: rule: builtin/deny"} @pytest.mark.asyncio - async def test_engine_absent_allows(self, monkeypatch): + async def test_engine_absent_denies_when_permissions_enabled(self, monkeypatch): + """Fail closed: permissions on but no engine wired -> deny, not run.""" + from types import SimpleNamespace + from agentic_cli.tools.registry import get_registry from agentic_cli.workflow.langgraph.permission_wrap import wrap_tool_for_permission @@ -96,15 +99,48 @@ async def test_engine_absent_allows(self, monkeypatch): "agentic_cli.workflow.langgraph.permission_wrap.get_service", lambda k: None, ) + monkeypatch.setattr( + "agentic_cli.config.get_settings", + lambda: SimpleNamespace(permissions_enabled=True), + ) + reg = get_registry() + + @reg.register( + name="read_lg_deny", + capabilities=[Capability("filesystem.read", target_arg="path")], + ) + def read_lg_deny(path: str): + return {"ok": True} + + wrapped = wrap_tool_for_permission(read_lg_deny) + result = await wrapped(path="/x") + assert result["success"] is False + + @pytest.mark.asyncio + async def test_engine_absent_allows_when_permissions_disabled(self, monkeypatch): + """Permissions off is the only case a missing engine runs the tool.""" + from types import SimpleNamespace + + from agentic_cli.tools.registry import get_registry + from agentic_cli.workflow.langgraph.permission_wrap import wrap_tool_for_permission + + monkeypatch.setattr( + "agentic_cli.workflow.langgraph.permission_wrap.get_service", + lambda k: None, + ) + monkeypatch.setattr( + "agentic_cli.config.get_settings", + lambda: SimpleNamespace(permissions_enabled=False), + ) reg = get_registry() @reg.register( - name="read_lg2", + name="read_lg_allow", capabilities=[Capability("filesystem.read", target_arg="path")], ) - def read_lg2(path: str): + def read_lg_allow(path: str): return {"ok": True} - wrapped = wrap_tool_for_permission(read_lg2) + wrapped = wrap_tool_for_permission(read_lg_allow) result = await wrapped(path="/x") assert result == {"ok": True} diff --git a/tests/workflow/test_adk_mcp_permissions.py b/tests/workflow/test_adk_mcp_permissions.py index 00e943f..7fd8e04 100644 --- a/tests/workflow/test_adk_mcp_permissions.py +++ b/tests/workflow/test_adk_mcp_permissions.py @@ -91,8 +91,31 @@ async def test_no_rule_asks_user_and_denies(self, tmp_path): res = await _check(eng, _MCPTool("notion_search")) assert res is not None and res["success"] is False - async def test_engine_absent_allows(self, tmp_path): - # No engine in the registry -> test/dev fallback allows. + async def test_engine_absent_denies_when_permissions_enabled(self, tmp_path, monkeypatch): + # No engine but permissions enabled -> fail closed (deny), not allow. + from types import SimpleNamespace + + monkeypatch.setattr( + "agentic_cli.config.get_settings", + lambda: SimpleNamespace(permissions_enabled=True), + ) + token = set_service_registry({}) + try: + res = await PermissionPlugin().before_tool_callback( + tool=_MCPTool("notion_search"), tool_args={}, tool_context=None + ) + finally: + token.var.reset(token) + assert res is not None and res["success"] is False + + async def test_engine_absent_allows_when_permissions_disabled(self, tmp_path, monkeypatch): + # Permissions off is the only case a missing engine allows. + from types import SimpleNamespace + + monkeypatch.setattr( + "agentic_cli.config.get_settings", + lambda: SimpleNamespace(permissions_enabled=False), + ) token = set_service_registry({}) try: res = await PermissionPlugin().before_tool_callback( From 3cfdc7e4a5a10ec12f3b07caf5640146d43a5162 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:25:07 -0400 Subject: [PATCH 05/22] refine(document): keep TeX config vars in the scrubbed env (P0-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The env allowlist was too aggressive — dropping TEXMFHOME/TEXMFVAR/etc. would break users with a custom TeX tree. Pass through TeX's own TEX* config vars (still scrubbing host secrets, still never inheriting the caller's TEXINPUTS). Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 13 +++++++++++-- tests/tools/test_document_compile.py | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 00176fe..d0e7118 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -53,8 +53,17 @@ def _build_env(assets_dir: str | None) -> dict[str, str]: - """Minimal, allowlisted environment for the TeX subprocess.""" - env = {k: os.environ[k] for k in _ENV_PASSTHROUGH if k in os.environ} + """Minimal, allowlisted environment for the TeX subprocess. + + Passes PATH/HOME/locale plus TeX's own ``TEX*`` configuration variables + (so a custom ``TEXMFHOME`` etc. keeps working) — but not arbitrary host env, + and never the caller's ``TEXINPUTS`` (set explicitly below). + """ + env = { + k: v + for k, v in os.environ.items() + if k in _ENV_PASSTHROUGH or (k.startswith("TEX") and k != "TEXINPUTS") + } env.setdefault("PATH", os.defpath) if assets_dir: # Prepend assets_dir; the trailing empty entries let kpathsea append the diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index 5d0e451..b2fd2c3 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -246,3 +246,24 @@ def test_capabilities_scope_assets_and_output(): assert ("document.compile", "source_path", False) in caps assert ("filesystem.read", "assets_dir", True) in caps assert ("filesystem.write", "output_pdf", True) in caps + + +def test_env_passes_tex_config_but_not_texinputs(monkeypatch, tmp_path): + """TeX's own TEX* config vars pass through (so a custom TEXMFHOME works), + but a caller-inherited TEXINPUTS is dropped in favor of our controlled one.""" + _fake_engine(monkeypatch) + monkeypatch.setenv("TEXMFHOME", "/home/user/texmf") + monkeypatch.setenv("TEXINPUTS", "/evil/inputs") + tex = tmp_path / "r.tex"; tex.write_text("x") + captured = {} + + def fake_run(argv, *, cwd, env, timeout): + captured["env"] = env + (Path(cwd) / "r.pdf").write_bytes(b"%PDF") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + compile_document(str(tex), assets_dir="/tmp/assets") + assert captured["env"].get("TEXMFHOME") == "/home/user/texmf" + assert "/evil/inputs" not in captured["env"]["TEXINPUTS"] + assert "/tmp/assets" in captured["env"]["TEXINPUTS"] From 6e33620e7d1d35196c5904d2b9501ea4f0dcd3e9 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:42:19 -0400 Subject: [PATCH 06/22] fix(tools): apply grep containment on the ripgrep path too (P0-4 review) Review found the symlink-containment filter was only added to grep's Python fallback; the ripgrep path (the common one when rg is installed) returned rg's file paths verbatim. A followed symlink (e.g. via a host RIPGREP_CONFIG_PATH injecting --follow) could surface outside-root file content under an inside-looking path. Filter rg's reported files through path_is_within (resolving rg's relative paths against the search root), and drop RIPGREP_CONFIG_PATH from the rg subprocess env so the --follow vector is removed at source. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/grep_tool.py | 14 ++++++ tests/tools/test_glob_grep_containment.py | 52 +++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/src/agentic_cli/tools/grep_tool.py b/src/agentic_cli/tools/grep_tool.py index 76d5bde..c7b0da5 100644 --- a/src/agentic_cli/tools/grep_tool.py +++ b/src/agentic_cli/tools/grep_tool.py @@ -5,6 +5,7 @@ """ import functools +import os import re import subprocess from pathlib import Path @@ -158,12 +159,17 @@ def _grep_with_ripgrep( cmd.append("--") cmd.append(str(path)) + # Drop RIPGREP_CONFIG_PATH so a host config can't inject flags (e.g. + # --follow, which would make rg traverse symlinks out of the authorized + # root). Containment below is the backstop; this removes the vector. + rg_env = {k: v for k, v in os.environ.items() if k != "RIPGREP_CONFIG_PATH"} try: result = subprocess.run( cmd, capture_output=True, text=True, timeout=30, + env=rg_env, ) except subprocess.TimeoutExpired: return { @@ -207,6 +213,14 @@ def _grep_with_ripgrep( if data.get("type") == "match": match_data = data.get("data", {}) file_path = match_data.get("path", {}).get("text", "") + # Drop files resolving outside the authorized root (e.g. a symlink + # rg followed) — parity with the Python fallback's containment. rg + # may report a path relative to the search root, so resolve it there. + if file_path: + fp = Path(file_path) + abs_fp = fp if fp.is_absolute() else (path / fp) + if not path_is_within(abs_fp, path): + continue files_searched.add(file_path) file_counts[file_path] = file_counts.get(file_path, 0) + 1 total_matches += 1 diff --git a/tests/tools/test_glob_grep_containment.py b/tests/tools/test_glob_grep_containment.py index a4d65cd..2cd9421 100644 --- a/tests/tools/test_glob_grep_containment.py +++ b/tests/tools/test_glob_grep_containment.py @@ -6,6 +6,9 @@ """ from __future__ import annotations +import json +import subprocess + import agentic_cli.tools.grep_tool as grep_mod from agentic_cli.tools.glob_tool import glob from agentic_cli.tools.grep_tool import grep @@ -95,3 +98,52 @@ def test_grep_python_skips_symlink_escaping_root(tmp_path, monkeypatch): # even though its own path name doesn't contain "secret". assert not any("link.txt" in f for f in files) assert any("real.txt" in f for f in files) + + +def test_grep_ripgrep_filters_outside_root(tmp_path, monkeypatch): + """The ripgrep path (used when rg is installed) must apply the same + containment as the Python fallback — a followed symlink that rg reports + with an outside-root path must be dropped.""" + root = tmp_path / "root" + root.mkdir() + (root / "real.txt").write_text("needle here") + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: True) + + outside = tmp_path / "outside" / "secret.txt" + inside = root / "real.txt" + + def fake_run(cmd, **kwargs): + lines = [ + json.dumps({"type": "match", "data": { + "path": {"text": str(outside)}, "line_number": 1, + "lines": {"text": "needle SECRET\n"}}}), + json.dumps({"type": "match", "data": { + "path": {"text": str(inside)}, "line_number": 1, + "lines": {"text": "needle here\n"}}}), + ] + return subprocess.CompletedProcess(cmd, 0, stdout="\n".join(lines), stderr="") + + monkeypatch.setattr(grep_mod.subprocess, "run", fake_run) + r = grep(pattern="needle", path=str(root)) + files = {m["file"] for m in r["matches"]} + assert any("real.txt" in f for f in files) + assert not any("secret.txt" in f for f in files) + + +def test_grep_ripgrep_scrubs_config_path_env(tmp_path, monkeypatch): + """A host RIPGREP_CONFIG_PATH could inject --follow (defeating containment); + it must not be inherited by the rg subprocess.""" + root = tmp_path / "root" + root.mkdir() + monkeypatch.setenv("RIPGREP_CONFIG_PATH", "/home/user/.rgrc") + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: True) + captured = {} + + def fake_run(cmd, **kwargs): + captured["env"] = kwargs.get("env") + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + monkeypatch.setattr(grep_mod.subprocess, "run", fake_run) + grep(pattern="x", path=str(root)) + assert captured["env"] is not None + assert "RIPGREP_CONFIG_PATH" not in captured["env"] From 75ac08f9bb1635badec803c19457662305cce334 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:42:19 -0400 Subject: [PATCH 07/22] fix(review): address minor findings from the hardening review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - engine.check: guard empty outcomes (all-optional caps absent) — was a latent IndexError on outcomes[0]; now allows (nothing to gate). - compile_document: pass PERL5LIB/PERLLIB (latexmk is Perl) so the env scrub can't break module loading; preserve the delivered PDF's mode via copystat (mkstemp is 0600) so reports stay readable; document that no-follow delivery guards only the final path component (parent-dir symlink residual, deferred to spec §9). - test_packaging: scope the docstring to a regression pin, not an exhaustive import audit. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 10 ++++++++ .../workflow/permissions/engine.py | 5 ++++ tests/permissions/test_engine.py | 12 ++++++++++ tests/test_packaging.py | 9 ++++--- tests/tools/test_document_compile.py | 24 +++++++++++++++++++ 5 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index d0e7118..976dc9c 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -49,6 +49,8 @@ _ENV_PASSTHROUGH = ( "PATH", "HOME", "TERM", "TMPDIR", "TEMP", "TMP", "LANG", "LC_ALL", "LC_CTYPE", "SOURCE_DATE_EPOCH", + # latexmk is a Perl program; without its module path it can fail to load. + "PERL5LIB", "PERLLIB", ) @@ -80,6 +82,11 @@ def _deliver_no_follow(produced: Path, dest: Path) -> None: it over dest. ``os.replace`` swaps the destination *name*: if dest is a symlink the link itself is replaced (not written through), so an attacker-placed symlink can't redirect the write outside the intended path. + + This protects only the final path component. A symlinked ``dest.parent`` + (or an ancestor) still redirects the write; the permission engine + canonicalizes ``output_pdf`` at check time, but a check→write window + remains. Full parent containment is deferred (spec §9, OS-sandbox). """ dest.parent.mkdir(parents=True, exist_ok=True) fd, tmp_name = tempfile.mkstemp( @@ -91,6 +98,9 @@ def _deliver_no_follow(produced: Path, dest: Path) -> None: shutil.copyfileobj(src, out) out.flush() os.fsync(out.fileno()) + # Match the produced PDF's mode/mtime (mkstemp is 0600) so the delivered + # file has the readability a consumer expects, as the old copy2 did. + shutil.copystat(produced, tmp_path) os.replace(tmp_path, dest) except BaseException: tmp_path.unlink(missing_ok=True) diff --git a/src/agentic_cli/workflow/permissions/engine.py b/src/agentic_cli/workflow/permissions/engine.py index c4ef1f0..3f338d0 100644 --- a/src/agentic_cli/workflow/permissions/engine.py +++ b/src/agentic_cli/workflow/permissions/engine.py @@ -148,6 +148,11 @@ async def check( resolved = self._resolve(capabilities, args) outcomes = self._evaluate(resolved) + # No capabilities to evaluate (e.g. every cap is optional and its target + # arg was absent) → nothing to gate, allow. + if not outcomes: + return CheckResult(True, "no applicable capabilities") + # DENY wins. deny_hits = [(c, r) for c, r in outcomes if r is not None and r.effect is Effect.DENY] if deny_hits: diff --git a/tests/permissions/test_engine.py b/tests/permissions/test_engine.py index 4059769..6a5aaa5 100644 --- a/tests/permissions/test_engine.py +++ b/tests/permissions/test_engine.py @@ -516,3 +516,15 @@ def test_required_cap_still_resolves_when_arg_absent(self, ctx): {}, ) assert len(resolved) == 1 + + @pytest.mark.asyncio + async def test_check_allows_when_all_optional_caps_absent(self, ctx): + """All-optional caps with absent args resolve to [] — check() must not + crash (IndexError on outcomes[0]) and should allow (nothing to gate).""" + engine = self._engine(ctx) + result = await engine.check( + "some_tool", + [Capability("filesystem.write", target_arg="out", optional=True)], + {}, + ) + assert result.allowed is True diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 6653332..5ee3b5d 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -1,6 +1,9 @@ -"""Packaging consistency: every eagerly-imported third-party module must be a -declared dependency in pyproject.toml, so a clean ``pip install`` doesn't fail -on first import (a package built from pyproject alone won't see environment.yml). +"""Packaging consistency regression pins. + +A clean ``pip install`` uses only pyproject.toml (not environment.yml), so an +eagerly-imported third-party module missing from ``[project.dependencies]`` +breaks on first import. This pins the specific modules that regressed; it is +not an exhaustive import-vs-dependency audit. """ from __future__ import annotations diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index b2fd2c3..d671081 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -200,6 +200,7 @@ def test_env_is_allowlisted_not_full_environ(monkeypatch, tmp_path): _fake_engine(monkeypatch) monkeypatch.setenv("MY_SECRET_TOKEN", "sk-must-not-leak") monkeypatch.setenv("PATH", "/custom/bin") + monkeypatch.setenv("PERL5LIB", "/opt/perl/lib") # latexmk (Perl) needs this tex = tmp_path / "r.tex"; tex.write_text("x") captured = {} @@ -212,6 +213,7 @@ def fake_run(argv, *, cwd, env, timeout): compile_document(str(tex), assets_dir="/tmp/assets") assert "MY_SECRET_TOKEN" not in captured["env"] assert captured["env"]["PATH"] == "/custom/bin" + assert captured["env"]["PERL5LIB"] == "/opt/perl/lib" assert "/tmp/assets" in captured["env"]["TEXINPUTS"] @@ -267,3 +269,25 @@ def fake_run(argv, *, cwd, env, timeout): assert captured["env"].get("TEXMFHOME") == "/home/user/texmf" assert "/evil/inputs" not in captured["env"]["TEXINPUTS"] assert "/tmp/assets" in captured["env"]["TEXINPUTS"] + + +def test_delivery_preserves_readable_mode(monkeypatch, tmp_path): + """Delivered PDF keeps the produced file's mode (copystat), not the + mkstemp default 0600 — a report is not a secret and consumers expect it + readable.""" + import os as _os + + _fake_engine(monkeypatch) + tex = tmp_path / "r.tex"; tex.write_text("x") + out = tmp_path / "deliver" / "report.pdf" + + def fake_run(argv, *, cwd, env, timeout): + p = Path(cwd) / "r.pdf" + p.write_bytes(b"%PDF") + _os.chmod(p, 0o644) + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + r = compile_document(str(tex), output_pdf=str(out)) + assert r["success"] is True + assert out.stat().st_mode & 0o044 # group/other readable, not 0600 From d15b9db2d8f60c27d105de313a887e3ecfaf96f4 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 20:43:42 -0400 Subject: [PATCH 08/22] fix(review2): close residuals from the branch re-review (P0-3, P0-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the hardening branch reproduced defects in the new code: P0-3 compile_document: - _build_env used k.startswith('TEX'), leaking non-TeX vars like TEXT_API_TOKEN to the subprocess — defeating the very allowlist. Scope to TEXMF* + a fixed set of TeX vars. - assets_dir containing os.pathsep (e.g. 'assets:/etc') was authorized as one filesystem.read target but became multiple TEXINPUTS roots. Reject it. - Run latexmk with -norc so a build-dir/home .latexmkrc (arbitrary Perl) is never executed — the two arbitrary-code vectors (rc + shell-escape) are now both off by default. Docstrings/description updated to match. P0-4 glob: - include_hidden=False only checked the basename, so '**/*' still returned files under a dot-directory (.hidden/secret.txt). Reject any result with a hidden component in its path relative to the root. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 71 ++++++++++++++++------- src/agentic_cli/tools/glob_tool.py | 7 ++- tests/tools/test_document_compile.py | 51 ++++++++++++++++ tests/tools/test_glob_grep_containment.py | 13 +++++ 4 files changed, 119 insertions(+), 23 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 976dc9c..2e100c6 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -1,21 +1,23 @@ """Compile a LaTeX document to PDF with a host TeX engine. ``compile_document`` is a narrow, permission-gated tool: it runs ``latexmk`` -(preferred) or ``pdflatex`` as a guarded subprocess — shell-escape is disabled -for the **pdflatex** path (``-no-shell-escape`` flag). For ``latexmk``, the -engine relies on its default restricted mode; note that ``latexmk`` also reads -``.latexmkrc`` (arbitrary Perl) from the build directory and home directory, so -callers with an untrusted ``.tex``/build directory should prefer ``pdflatex``. -OS-sandbox confinement for the general case is deferred. +(preferred) or ``pdflatex`` as a guarded subprocess with the two arbitrary-code +vectors disabled — ``latexmk`` with ``-norc`` (so no ``.latexmkrc`` Perl is read +from the build directory or home) and ``pdflatex`` with ``-no-shell-escape`` (no +``\\write18``). It runs on the host (not the container sandbox — an intentional +decoupling); OS-sandbox confinement of the build tree is deferred (spec §9). The tool runs with a wall-clock timeout and a scoped working dir, and returns a -structured result. It does NOT execute arbitrary code; a ``report_writer``-style -agent uses it to turn an authored ``.tex`` into a PDF. +structured result. It does not execute arbitrary host code by default; a +``report_writer``-style agent uses it to turn an authored ``.tex`` into a PDF. +Build intermediates are still written in the source directory, so a fully +untrusted ``.tex`` + build dir remains out of scope for the current controls. -The subprocess receives only an allowlisted environment (``_ENV_PASSTHROUGH``), -not the full host environment, so host secrets aren't handed to the TeX process. -Delivery to ``output_pdf`` is a no-follow atomic write (temp file + ``os.replace``) -so an attacker-placed symlink at the destination can't redirect the write. +The subprocess receives only an allowlisted environment (``_ENV_PASSTHROUGH`` + +the ``TEXMF*``/TeX config vars), not the full host environment, so host secrets +aren't handed to the TeX process. Delivery to ``output_pdf`` is a no-follow +atomic write (temp file + ``os.replace``) so an attacker-placed symlink at the +destination can't redirect the write. Provisioning is host-based: the engine must be on ``PATH`` (TeX Live / MacTeX). If neither is found the tool returns a structured error with an install hint. @@ -53,18 +55,27 @@ "PERL5LIB", "PERLLIB", ) +# TeX's own search/config vars (kpathsea) that don't fall under the TEXMF* +# namespace. TEXINPUTS is deliberately excluded — it is set explicitly below. +_TEX_VARS = ( + "TEXFONTS", "TEXFORMATS", "TEXPOOL", "TEXPSHEADERS", + "TEXCONFIG", "TEXDOCS", "TEXSOURCES", +) + def _build_env(assets_dir: str | None) -> dict[str, str]: """Minimal, allowlisted environment for the TeX subprocess. - Passes PATH/HOME/locale plus TeX's own ``TEX*`` configuration variables - (so a custom ``TEXMFHOME`` etc. keeps working) — but not arbitrary host env, - and never the caller's ``TEXINPUTS`` (set explicitly below). + Passes PATH/HOME/locale plus TeX's own configuration variables — the + ``TEXMF*`` tree and a fixed set of other TeX vars — so a custom + ``TEXMFHOME`` etc. keeps working, but not arbitrary host env (a name like + ``TEXT_API_TOKEN`` starts with "TEX" yet is not a TeX var), and never the + caller's ``TEXINPUTS`` (set explicitly below). """ env = { k: v for k, v in os.environ.items() - if k in _ENV_PASSTHROUGH or (k.startswith("TEX") and k != "TEXINPUTS") + if k in _ENV_PASSTHROUGH or k.startswith("TEXMF") or k in _TEX_VARS } env.setdefault("PATH", os.defpath) if assets_dir: @@ -143,9 +154,17 @@ def _detect_engine(engine: str | None) -> str | None: def _build_argv(engine: str, source: str) -> list[str]: - """Compiler argv — never enables shell-escape.""" + """Compiler argv — never enables shell-escape. + + latexmk runs with ``-norc`` so it won't read ``.latexmkrc`` (arbitrary + Perl) from the build directory or home; pdflatex runs with + ``-no-shell-escape``. Neither path executes arbitrary host code by default. + """ if engine == "latexmk": - return ["latexmk", "-pdf", "-interaction=nonstopmode", "-halt-on-error", source] + return [ + "latexmk", "-norc", "-pdf", "-interaction=nonstopmode", + "-halt-on-error", source, + ] return [ "pdflatex", "-no-shell-escape", "-interaction=nonstopmode", "-halt-on-error", source, @@ -168,9 +187,9 @@ def _parse_errors(log_text: str) -> list[str]: ], description=( "Compile a LaTeX source file to PDF using a host TeX engine (latexmk or " - "pdflatex). Shell-escape is disabled for pdflatex (-no-shell-escape); " - "latexmk uses its default restricted mode but also reads .latexmkrc from " - "the build/home directory. Returns the PDF path plus any compiler errors. " + "pdflatex). Runs on the host: latexmk with -norc (no .latexmkrc) and " + "pdflatex with -no-shell-escape, so it does not execute arbitrary host " + "code by default. Returns the PDF path plus any compiler errors. " "Requires TeX Live/MacTeX on PATH." ), ) @@ -205,6 +224,16 @@ def compile_document( "duration_ms": 0, } + if assets_dir and os.pathsep in assets_dir: + # A path-list separator would turn one authorized filesystem.read target + # into several TEXINPUTS search roots (e.g. "assets:/etc" also reads /etc). + return { + "success": False, + "error": f"assets_dir must be a single path (no {os.pathsep!r}): {assets_dir}", + "pdf_path": None, "engine": None, "log_tail": "", "errors": [], + "duration_ms": 0, + } + chosen = _detect_engine(engine) if chosen is None: looked = engine or "/".join(_ENGINES) diff --git a/src/agentic_cli/tools/glob_tool.py b/src/agentic_cli/tools/glob_tool.py index 3e0614b..dab2d5a 100644 --- a/src/agentic_cli/tools/glob_tool.py +++ b/src/agentic_cli/tools/glob_tool.py @@ -93,8 +93,11 @@ def glob( if not path_is_within(match, search_path): continue - # Skip hidden files if not requested - if not include_hidden and match.name.startswith("."): + # Skip results with any hidden component, not just a hidden basename — + # a pattern like "**/*" otherwise leaks files under a dot-directory. + if not include_hidden and any( + part.startswith(".") for part in match.relative_to(search_path).parts + ): continue # Skip directories if not requested diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index d671081..99a9c9d 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -291,3 +291,54 @@ def fake_run(argv, *, cwd, env, timeout): r = compile_document(str(tex), output_pdf=str(out)) assert r["success"] is True assert out.stat().st_mode & 0o044 # group/other readable, not 0600 + + +# --- P0-3 re-review: env scope, assets_dir separator, latexmk -norc --- + +def test_env_excludes_nontex_vars_starting_with_tex(monkeypatch, tmp_path): + """`k.startswith('TEX')` is too broad — TEXT_API_TOKEN etc. must NOT leak; + only real TeX vars (TEXMF*/known) pass.""" + _fake_engine(monkeypatch) + monkeypatch.setenv("TEXT_API_TOKEN", "sk-must-not-leak") + monkeypatch.setenv("TEXMFHOME", "/home/u/texmf") + tex = tmp_path / "r.tex"; tex.write_text("x") + captured = {} + + def fake_run(argv, *, cwd, env, timeout): + captured["env"] = env + (Path(cwd) / "r.pdf").write_bytes(b"%PDF") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + compile_document(str(tex)) + assert "TEXT_API_TOKEN" not in captured["env"] + assert captured["env"].get("TEXMFHOME") == "/home/u/texmf" + + +def test_assets_dir_with_path_separator_rejected(monkeypatch, tmp_path): + """assets_dir authorized as one filesystem path must not smuggle extra + TEXINPUTS roots via os.pathsep (e.g. 'assets:/etc').""" + import os as _os + + _fake_engine(monkeypatch) + tex = tmp_path / "r.tex"; tex.write_text("x") + r = compile_document(str(tex), assets_dir=f"assets{_os.pathsep}/etc") + assert r["success"] is False + assert "assets_dir" in r["error"].lower() + + +def test_latexmk_uses_norc(monkeypatch, tmp_path): + """latexmk must run with -norc so a build-dir/home .latexmkrc (arbitrary + Perl) is not executed.""" + _fake_engine(monkeypatch) + tex = tmp_path / "r.tex"; tex.write_text("x") + captured = {} + + def fake_run(argv, *, cwd, env, timeout): + captured["argv"] = argv + (Path(cwd) / "r.pdf").write_bytes(b"%PDF") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + compile_document(str(tex)) + assert "-norc" in captured["argv"] diff --git a/tests/tools/test_glob_grep_containment.py b/tests/tools/test_glob_grep_containment.py index 2cd9421..fd297de 100644 --- a/tests/tools/test_glob_grep_containment.py +++ b/tests/tools/test_glob_grep_containment.py @@ -147,3 +147,16 @@ def fake_run(cmd, **kwargs): grep(pattern="x", path=str(root)) assert captured["env"] is not None assert "RIPGREP_CONFIG_PATH" not in captured["env"] + + +def test_glob_excludes_hidden_ancestor(tmp_path): + """include_hidden=False must drop results with a hidden ANCESTOR, not just + a hidden basename (e.g. .hidden/secret.txt via **/*).""" + root = tmp_path / "root" + (root / ".hidden").mkdir(parents=True) + (root / ".hidden" / "secret.txt").write_text("s") + (root / "visible.txt").write_text("v") + r = glob(pattern="**/*", path=str(root), include_hidden=False) + assert r["success"] is True + assert all(".hidden" not in f for f in r["files"]) + assert any("visible.txt" in f for f in r["files"]) From c33ff749e11a3232251c0e61f93c23aaaeea6e3c Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:54:53 -0400 Subject: [PATCH 09/22] fix(document): anchor option-like source filename (P0-3 critical) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A source_path whose basename begins with '-' was appended to the engine argv verbatim, so latexmk/pdflatex parsed it as an OPTION — e.g. '-pdflatex=CMD.tex', '-r', '-use-make' execute arbitrary commands/Perl. -norc does not close this: if the option consumes the sole source arg, latexmk defaults to compiling every *.tex in the dir. source_path is model-controlled, so this is an arbitrary-host-exec vector. Anchor the source with './' (_safe_source_arg) for both engines so a leading '-' is always parsed as a path, never an option. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 9 ++++++++- tests/tools/test_document_compile.py | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 2e100c6..4b75e2a 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -171,6 +171,13 @@ def _build_argv(engine: str, source: str) -> list[str]: ] +def _safe_source_arg(name: str) -> str: + """Anchor a source filename so a leading ``-`` can't be parsed as an engine + option (arbitrary-exec via ``-pdflatex=CMD`` etc.). ``name`` is a basename + and the subprocess cwd is the source's directory, so ``./`` resolves it.""" + return name if name.startswith("./") else f"./{name}" + + def _parse_errors(log_text: str) -> list[str]: """Extract LaTeX error lines (those beginning with '!') from a log.""" return [ln for ln in log_text.splitlines() if ln.startswith("!")] @@ -250,7 +257,7 @@ def compile_document( work_dir = src.parent env = _build_env(assets_dir) - argv = _build_argv(chosen, src.name) + argv = _build_argv(chosen, _safe_source_arg(src.name)) start = time.monotonic() try: proc = _run(argv, cwd=str(work_dir), env=env, timeout=float(timeout_s)) diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index 99a9c9d..2d62f66 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -342,3 +342,24 @@ def fake_run(argv, *, cwd, env, timeout): monkeypatch.setattr(mod, "_run", fake_run) compile_document(str(tex)) assert "-norc" in captured["argv"] + + +def test_option_like_source_name_not_treated_as_flag(monkeypatch, tmp_path): + """A source basename starting with '-' must be anchored (./) so the engine + parses it as a file, not an option — otherwise '-pdflatex=CMD.tex' et al. + execute arbitrary host commands (which -norc does NOT prevent).""" + _fake_engine(monkeypatch) + tex = tmp_path / "-pdflatex=evil.tex" + tex.write_text("x") + captured = {} + + def fake_run(argv, *, cwd, env, timeout): + captured["argv"] = argv + (Path(cwd) / (tex.stem + ".pdf")).write_bytes(b"%PDF") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + r = compile_document(str(tex)) + assert "-pdflatex=evil.tex" not in captured["argv"] # never a bare option-like token + assert "./-pdflatex=evil.tex" in captured["argv"] # anchored as a path + assert r["success"] is True From bdcb544fe5e228744b42bc9c89d7e9aaf2f6e5b4 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:11:55 -0400 Subject: [PATCH 10/22] fix(document): build in a private temp dir, isolate intermediates (P0-3) Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 110 ++++++++++++---------- tests/tools/test_document_compile.py | 28 +++++- 2 files changed, 84 insertions(+), 54 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 4b75e2a..3b45fd7 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -7,11 +7,12 @@ ``\\write18``). It runs on the host (not the container sandbox — an intentional decoupling); OS-sandbox confinement of the build tree is deferred (spec §9). -The tool runs with a wall-clock timeout and a scoped working dir, and returns a -structured result. It does not execute arbitrary host code by default; a -``report_writer``-style agent uses it to turn an authored ``.tex`` into a PDF. -Build intermediates are still written in the source directory, so a fully -untrusted ``.tex`` + build dir remains out of scope for the current controls. +The tool runs with a wall-clock timeout and a private temp build dir, and +returns a structured result. It does not execute arbitrary host code by +default; a ``report_writer``-style agent uses it to turn an authored ``.tex`` +into a PDF. Build intermediates (``*.aux``, ``*.log``) are isolated in a +``tempfile.TemporaryDirectory`` and never written to the source or delivery dir; +only the final PDF is promoted via ``_deliver_no_follow``. The subprocess receives only an allowlisted environment (``_ENV_PASSTHROUGH`` + the ``TEXMF*``/TeX config vars), not the full host environment, so host secrets @@ -63,7 +64,7 @@ ) -def _build_env(assets_dir: str | None) -> dict[str, str]: +def _build_env(assets_dir: str | None, source_dir: str | None = None) -> dict[str, str]: """Minimal, allowlisted environment for the TeX subprocess. Passes PATH/HOME/locale plus TeX's own configuration variables — the @@ -71,6 +72,9 @@ def _build_env(assets_dir: str | None) -> dict[str, str]: ``TEXMFHOME`` etc. keeps working, but not arbitrary host env (a name like ``TEXT_API_TOKEN`` starts with "TEX" yet is not a TeX var), and never the caller's ``TEXINPUTS`` (set explicitly below). + + ``assets_dir`` and ``source_dir`` become TEXINPUTS read roots so figures and + ``\\input`` siblings resolve even though the build runs in a private temp dir. """ env = { k: v @@ -78,11 +82,10 @@ def _build_env(assets_dir: str | None) -> dict[str, str]: if k in _ENV_PASSTHROUGH or k.startswith("TEXMF") or k in _TEX_VARS } env.setdefault("PATH", os.defpath) - if assets_dir: - # Prepend assets_dir; the trailing empty entries let kpathsea append the - # default search path. A caller-inherited TEXINPUTS is intentionally - # dropped (not in the allowlist) so it can't redirect input resolution. - env["TEXINPUTS"] = f"{assets_dir}{os.pathsep}{os.pathsep}" + roots = [r for r in (assets_dir, source_dir) if r] + if roots: + # Trailing empty entry lets kpathsea append its default search path. + env["TEXINPUTS"] = os.pathsep.join(roots) + os.pathsep return env @@ -254,59 +257,66 @@ def compile_document( "duration_ms": 0, } - work_dir = src.parent - env = _build_env(assets_dir) - + env = _build_env(assets_dir, source_dir=str(src.parent)) argv = _build_argv(chosen, _safe_source_arg(src.name)) start = time.monotonic() - try: - proc = _run(argv, cwd=str(work_dir), env=env, timeout=float(timeout_s)) - except subprocess.TimeoutExpired: - return { - "success": False, "error": f"Compilation timed out after {timeout_s}s", - "pdf_path": None, "engine": chosen, "log_tail": "", "errors": [], - "duration_ms": int((time.monotonic() - start) * 1000), - } - except OSError as exc: - return { - "success": False, "error": f"Failed to run {chosen}: {exc}", - "pdf_path": None, "engine": chosen, "log_tail": "", "errors": [], - "duration_ms": int((time.monotonic() - start) * 1000), - } - duration_ms = int((time.monotonic() - start) * 1000) - log_path = work_dir / (src.stem + ".log") - try: - log_text = log_path.read_text(errors="replace") if log_path.is_file() else (proc.stdout or "") - except OSError: - log_text = proc.stdout or "" - log_tail = "\n".join(log_text.splitlines()[-_LOG_TAIL_LINES:]) - produced = work_dir / (src.stem + ".pdf") - success = proc.returncode == 0 and produced.is_file() - - if not success: - return { - "success": False, "pdf_path": None, "engine": chosen, - "log_tail": log_tail, "errors": _parse_errors(log_text), - "duration_ms": duration_ms, "error": None, - } + with tempfile.TemporaryDirectory(prefix="texbuild-") as build_dir: + build = Path(build_dir) + try: + shutil.copy2(src, build / src.name) + except OSError as exc: + return { + "success": False, "error": f"Failed to stage source: {exc}", + "pdf_path": None, "engine": chosen, "log_tail": "", "errors": [], + "duration_ms": int((time.monotonic() - start) * 1000), + } + + try: + proc = _run(argv, cwd=str(build), env=env, timeout=float(timeout_s)) + except subprocess.TimeoutExpired: + return { + "success": False, "error": f"Compilation timed out after {timeout_s}s", + "pdf_path": None, "engine": chosen, "log_tail": "", "errors": [], + "duration_ms": int((time.monotonic() - start) * 1000), + } + except OSError as exc: + return { + "success": False, "error": f"Failed to run {chosen}: {exc}", + "pdf_path": None, "engine": chosen, "log_tail": "", "errors": [], + "duration_ms": int((time.monotonic() - start) * 1000), + } + duration_ms = int((time.monotonic() - start) * 1000) + + log_path = build / (src.stem + ".log") + try: + log_text = log_path.read_text(errors="replace") if log_path.is_file() else (proc.stdout or "") + except OSError: + log_text = proc.stdout or "" + log_tail = "\n".join(log_text.splitlines()[-_LOG_TAIL_LINES:]) + produced = build / (src.stem + ".pdf") + success = proc.returncode == 0 and produced.is_file() + + if not success: + return { + "success": False, "pdf_path": None, "engine": chosen, + "log_tail": log_tail, "errors": _parse_errors(log_text), + "duration_ms": duration_ms, "error": None, + } - final = produced - if output_pdf: - dest = Path(output_pdf) + dest = Path(output_pdf) if output_pdf else (src.parent / (src.stem + ".pdf")) try: _deliver_no_follow(produced, dest) except OSError as exc: return { "success": False, - "error": f"Failed to deliver PDF to {output_pdf}: {exc}", + "error": f"Failed to deliver PDF to {dest}: {exc}", "pdf_path": str(produced), "engine": chosen, "log_tail": log_tail, "errors": [], "duration_ms": duration_ms, } - final = dest return { - "success": True, "pdf_path": str(final), "engine": chosen, + "success": True, "pdf_path": str(dest), "engine": chosen, "log_tail": log_tail, "errors": [], "duration_ms": duration_ms, "error": None, } diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index 2d62f66..81ba58b 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -26,12 +26,13 @@ def test_missing_source_returns_error(tmp_path): assert r["success"] is False and "not found" in r["error"] -def test_success_places_pdf_and_keeps_intermediates(monkeypatch, tmp_path): +def test_success_delivers_pdf_and_isolates_intermediates(monkeypatch, tmp_path): _fake_engine(monkeypatch) tex = tmp_path / "r.tex" tex.write_text("\\documentclass{article}\\begin{document}hi\\end{document}") def fake_run(argv, *, cwd, env, timeout): + # fake_run receives the private build dir as cwd; write artifacts there (Path(cwd) / "r.pdf").write_bytes(b"%PDF-1.5 fake") (Path(cwd) / "r.log").write_text("output written on r.pdf") (Path(cwd) / "r.aux").write_text("\\relax") @@ -42,9 +43,9 @@ def fake_run(argv, *, cwd, env, timeout): r = compile_document(str(tex), output_pdf=str(out), assets_dir=str(tmp_path / "assets")) assert r["success"] is True assert r["pdf_path"] == str(out) - assert out.is_file() # PDF promoted to delivery dir - assert (tmp_path / "r.aux").is_file() # intermediates stay in build dir - assert not (out.parent / "r.aux").exists() # not beside the delivered PDF + assert out.is_file() # PDF promoted to delivery dir + assert not (tmp_path / "r.aux").exists() # intermediates NOT in the source dir + assert not (out.parent / "r.aux").exists() # nor beside the delivered PDF def test_failure_parses_errors(monkeypatch, tmp_path): @@ -363,3 +364,22 @@ def fake_run(argv, *, cwd, env, timeout): assert "-pdflatex=evil.tex" not in captured["argv"] # never a bare option-like token assert "./-pdflatex=evil.tex" in captured["argv"] # anchored as a path assert r["success"] is True + + +def test_default_delivery_to_source_dir_without_intermediates(monkeypatch, tmp_path): + """No output_pdf → PDF lands at /.pdf, but the build's + intermediates never touch the source dir.""" + _fake_engine(monkeypatch) + tex = tmp_path / "r.tex"; tex.write_text("x") + + def fake_run(argv, *, cwd, env, timeout): + (Path(cwd) / "r.pdf").write_bytes(b"%PDF") + (Path(cwd) / "r.aux").write_text("aux") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + r = compile_document(str(tex)) # no output_pdf + assert r["success"] is True + assert r["pdf_path"] == str(tmp_path / "r.pdf") + assert (tmp_path / "r.pdf").is_file() # delivered to source dir + assert not (tmp_path / "r.aux").exists() # intermediate isolated in temp From 4e7d1f3c3760dbe47d7def55c54b0241e5893304 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:18:48 -0400 Subject: [PATCH 11/22] fix(document): tail-read the compiler .log instead of whole file (P0-3) Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 20 ++++++++++++++++---- tests/tools/test_document_compile.py | 16 ++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 3b45fd7..d9cc196 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -44,6 +44,7 @@ _ENGINES = ("latexmk", "pdflatex") _LOG_TAIL_LINES = 40 +_LOG_TAIL_BYTES = 64 * 1024 # Only these host env vars reach the TeX process. The tool must not hand the # whole host environment (API keys, tokens) to a subprocess that — on the @@ -186,6 +187,20 @@ def _parse_errors(log_text: str) -> list[str]: return [ln for ln in log_text.splitlines() if ln.startswith("!")] +def _read_log_tail(log_path: Path, fallback: str) -> str: + """Return at most the last _LOG_TAIL_BYTES of the log (decoded), else + ``fallback``. Bounds memory on a runaway compiler log.""" + try: + if not log_path.is_file(): + return fallback + with open(log_path, "rb") as f: + size = f.seek(0, os.SEEK_END) + f.seek(max(0, size - _LOG_TAIL_BYTES)) + return f.read().decode("utf-8", errors="replace") + except OSError: + return fallback + + @register_tool( category=ToolCategory.EXECUTION, capabilities=[ @@ -289,10 +304,7 @@ def compile_document( duration_ms = int((time.monotonic() - start) * 1000) log_path = build / (src.stem + ".log") - try: - log_text = log_path.read_text(errors="replace") if log_path.is_file() else (proc.stdout or "") - except OSError: - log_text = proc.stdout or "" + log_text = _read_log_tail(log_path, fallback=proc.stdout or "") log_tail = "\n".join(log_text.splitlines()[-_LOG_TAIL_LINES:]) produced = build / (src.stem + ".pdf") success = proc.returncode == 0 and produced.is_file() diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index 81ba58b..b8f3a68 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -383,3 +383,19 @@ def fake_run(argv, *, cwd, env, timeout): assert r["pdf_path"] == str(tmp_path / "r.pdf") assert (tmp_path / "r.pdf").is_file() # delivered to source dir assert not (tmp_path / "r.aux").exists() # intermediate isolated in temp + + +# --- P0-3 hardening: tail-read .log to bound memory on runaway compiler logs --- + + +def test_read_log_tail_bounds_large_log(tmp_path): + log = tmp_path / "big.log" + log.write_text("START\n" + ("x" * 200_000) + "\n! Real error.\nEND\n") + out = mod._read_log_tail(log, fallback="FB") + assert "END" in out and "! Real error." in out # tail retained + assert "START" not in out # head dropped + assert len(out) <= mod._LOG_TAIL_BYTES + 16 # bounded + + +def test_read_log_tail_missing_returns_fallback(tmp_path): + assert mod._read_log_tail(tmp_path / "nope.log", fallback="FB") == "FB" From 62c6c9dd3ba63e09a28a727105c362ebeb0d7bc5 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:23:17 -0400 Subject: [PATCH 12/22] test(document_compile): add OSError coverage and tighten log-tail bound - Fix 1: Add test_read_log_tail_oserror_returns_fallback to verify _read_log_tail returns fallback on any OSError (PermissionError, etc.) without propagating - Fix 2: Change test_read_log_tail_bounds_large_log bound from +16 to +3 (decode can only shorten, never lengthen; +3 covers one UTF-8 replacement char) Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- tests/tools/test_document_compile.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index b8f3a68..369f6cf 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -394,8 +394,19 @@ def test_read_log_tail_bounds_large_log(tmp_path): out = mod._read_log_tail(log, fallback="FB") assert "END" in out and "! Real error." in out # tail retained assert "START" not in out # head dropped - assert len(out) <= mod._LOG_TAIL_BYTES + 16 # bounded + assert len(out) <= mod._LOG_TAIL_BYTES + 3 # bounded def test_read_log_tail_missing_returns_fallback(tmp_path): assert mod._read_log_tail(tmp_path / "nope.log", fallback="FB") == "FB" + + +def test_read_log_tail_oserror_returns_fallback(monkeypatch, tmp_path): + """An existing-but-unreadable log (open raises OSError) returns fallback, not raises.""" + log = tmp_path / "x.log"; log.write_text("data") + + def boom(*a, **k): + raise PermissionError("nope") + + monkeypatch.setattr("builtins.open", boom) + assert mod._read_log_tail(log, fallback="FB") == "FB" From 9831277b90c3c0ea657fa9b4621739424e04381c Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:25:32 -0400 Subject: [PATCH 13/22] fix(document): cap captured subprocess output in _run (P0-3) Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 39 +++++++++++++++-------- tests/tools/test_document_compile.py | 27 +++++++++++----- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index d9cc196..7017433 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -45,6 +45,7 @@ _ENGINES = ("latexmk", "pdflatex") _LOG_TAIL_LINES = 40 _LOG_TAIL_BYTES = 64 * 1024 +_MAX_CAPTURE_CHARS = 200_000 # Only these host env vars reach the TeX process. The tool must not hand the # whole host environment (API keys, tokens) to a subprocess that — on the @@ -131,19 +132,24 @@ def _run( argv: list[str], *, cwd: str, env: dict[str, str], timeout: float ) -> subprocess.CompletedProcess: """Run a subprocess in its own process group so a timeout kills the whole - tree (latexmk + its pdflatex grandchild), not just the direct child. Seam - for tests. POSIX (macOS/Linux), which is what the framework targets.""" - proc = subprocess.Popen( - argv, cwd=cwd, env=env, - stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, - start_new_session=True, - ) - try: - out, err = proc.communicate(timeout=timeout) - except subprocess.TimeoutExpired: - os.killpg(os.getpgid(proc.pid), signal.SIGKILL) - proc.communicate() # reap the killed group - raise + tree (latexmk + its pdflatex grandchild), not just the direct child. + stdout/stderr are captured to temp files and only the last + _MAX_CAPTURE_CHARS of each are retained, bounding host memory. Seam for + tests. POSIX (macOS/Linux), which is what the framework targets.""" + with tempfile.TemporaryFile() as out_f, tempfile.TemporaryFile() as err_f: + proc = subprocess.Popen( + argv, cwd=cwd, env=env, + stdout=out_f, stderr=err_f, + start_new_session=True, + ) + try: + proc.wait(timeout=timeout) + except subprocess.TimeoutExpired: + os.killpg(os.getpgid(proc.pid), signal.SIGKILL) + proc.wait() # reap the killed group + raise + out = _tail_of_file(out_f, _MAX_CAPTURE_CHARS) + err = _tail_of_file(err_f, _MAX_CAPTURE_CHARS) return subprocess.CompletedProcess(argv, proc.returncode, stdout=out, stderr=err) @@ -187,6 +193,13 @@ def _parse_errors(log_text: str) -> list[str]: return [ln for ln in log_text.splitlines() if ln.startswith("!")] +def _tail_of_file(f, limit: int) -> str: + """Return the last ``limit`` bytes of an open binary temp file, decoded.""" + size = f.seek(0, os.SEEK_END) + f.seek(max(0, size - limit)) + return f.read().decode("utf-8", errors="replace") + + def _read_log_tail(log_path: Path, fallback: str) -> str: """Return at most the last _LOG_TAIL_BYTES of the log (decoded), else ``fallback``. Bounds memory on a runaway compiler log.""" diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index 369f6cf..df2129a 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -161,8 +161,9 @@ def test_forced_unsupported_engine_rejected(monkeypatch, tmp_path): # --- Fix wave — final review (I1/I2/M1/M2/M3) --- -def test_run_group_kill_on_timeout(monkeypatch): - """I1: _run must start a new session and kill the process group on timeout.""" +def test_run_group_kill_on_timeout(monkeypatch, tmp_path): + """I1 regression: _run starts a new session and kills the process group on + timeout (now via proc.wait, not communicate).""" import subprocess as _subprocess popen_kwargs: dict = {} @@ -174,23 +175,33 @@ class FakePopen: def __init__(self, argv, **kwargs): popen_kwargs.update(kwargs) - def communicate(self, timeout=None): + def wait(self, timeout=None): if timeout is not None: raise _subprocess.TimeoutExpired([], timeout) - # reap call after kill - return ("", "") + return 0 monkeypatch.setattr(mod.subprocess, "Popen", FakePopen) monkeypatch.setattr(mod.os, "getpgid", lambda pid: pid) monkeypatch.setattr(mod.os, "killpg", lambda pgid, sig: killpg_calls.append((pgid, sig))) try: - mod._run(["latexmk"], cwd="/tmp", env={}, timeout=1.0) + mod._run(["latexmk"], cwd=str(tmp_path), env={}, timeout=1.0) except _subprocess.TimeoutExpired: pass # expected - assert popen_kwargs.get("start_new_session") is True, "Popen must use start_new_session=True" - assert len(killpg_calls) >= 1, "os.killpg must be called on timeout" + assert popen_kwargs.get("start_new_session") is True + assert len(killpg_calls) >= 1 + + +def test_run_caps_captured_output(tmp_path): + import os as _os + import sys as _sys + + argv = [_sys.executable, "-c", "print('x' * 1_000_000)"] + r = mod._run(argv, cwd=str(tmp_path), env={"PATH": _os.environ.get("PATH", "")}, timeout=30) + assert r.returncode == 0 + assert len(r.stdout) <= mod._MAX_CAPTURE_CHARS + 8 # bounded (decode slack) + assert r.stdout.rstrip().endswith("x") # tail retained # --- P0-3 hardening: env allowlist, no-follow delivery, capability scope --- From 72e35ed470a496214ab64659f3340ca461623c97 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:31:33 -0400 Subject: [PATCH 14/22] fix(document.compile): assert SIGKILL on timeout, rename capture-limit to bytes, tighten test slack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix 1 (Important): Assert SIGKILL signal in test_run_group_kill_on_timeout. Fix 2 (Minor): Rename _MAX_CAPTURE_CHARS→_MAX_CAPTURE_BYTES (it's a byte limit). Fix 3 (Minor): Tighten test_run_caps_captured_output slack from +8 to +1 byte. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- AGENTS.md | 160 ++++++++++++++++++++++ src/agentic_cli/tools/document/compile.py | 8 +- tests/tools/test_document_compile.py | 6 +- 3 files changed, 168 insertions(+), 6 deletions(-) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..ddece9f --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,160 @@ +# Agentic CLI - Shared Framework for Agentic Applications + +## Project Overview + +Agentic CLI is a shared library providing the core infrastructure for building domain-specific CLI applications powered by LLM agents. + +## Tech Stack + +- **Language**: Python 3.12+ +- **CLI UI**: `thinking-prompt` - enhanced CLI with thinking boxes and markdown +- **Workflow**: Google ADK + LangGraph - dual orchestration backends (selectable via settings) +- **Config**: `pydantic-settings` - type-safe configuration +- **Logging**: `structlog` - structured logging + +## Project Structure + +``` +agentic-cli/ +├── src/agentic_cli/ +│ ├── __init__.py # Package exports, lazy imports +│ ├── config.py # BaseSettings (pydantic-settings) +│ ├── constants.py # Shared constants, truncate() +│ ├── settings_persistence.py +│ ├── logging.py +│ ├── cli/ +│ │ ├── app.py # BaseCLIApp +│ │ ├── commands.py # Command, CommandRegistry +│ │ ├── builtin_commands.py +│ │ ├── workflow_controller.py # WorkflowController, factory +│ │ ├── message_processor.py +│ │ └── settings*.py # Settings UI (introspection, dialog) +│ ├── workflow/ +│ │ ├── base_manager.py # BaseWorkflowManager (abstract) +│ │ ├── task_progress.py # build_task_progress_event(), parse_plan_progress() +│ │ ├── events.py # WorkflowEvent, EventType +│ │ ├── thinking.py # ThinkingDetector +│ │ ├── config.py # AgentConfig +│ │ ├── context.py # ContextVars for tool access (get_context_*()) +│ │ ├── adk/ # ADK orchestrator +│ │ │ ├── manager.py # GoogleADKWorkflowManager +│ │ │ ├── event_processor.py # ADKEventProcessor +│ │ │ └── llm_event_logger.py # LLM traffic logging +│ │ └── langgraph/ # LangGraph orchestrator +│ │ ├── manager.py # LangGraphWorkflowManager +│ │ ├── graph_builder.py # LangGraphBuilder (graph + LLM factory) +│ │ ├── state.py +│ │ ├── persistence/ # Checkpointers, stores +│ │ └── tools/ # LangChain-compatible wrappers +│ ├── tools/ +│ │ ├── registry.py # ToolRegistry, @register_tool, ToolCategory +│ │ ├── executor.py # SafePythonExecutor +│ │ ├── knowledge_tools.py # kb_search, kb_ingest, kb_list, kb_read +│ │ ├── arxiv_tools.py # search_arxiv, fetch_arxiv_paper, analyze_arxiv_paper +│ │ ├── execution_tools.py # execute_python +│ │ ├── interaction_tools.py # ask_clarification +│ │ ├── file_read.py # read_file, diff_compare +│ │ ├── file_write.py # write_file, edit_file +│ │ ├── glob_tool.py # glob +│ │ ├── grep_tool.py # grep +│ │ ├── search.py # web_search (Tavily/Brave backends) +│ │ ├── webfetch_tool.py # web_fetch (orchestrator) +│ │ ├── memory_tools.py # save_memory, search_memory + MemoryStore +│ │ ├── planning_tools.py # save_plan, get_plan + PlanStore +│ │ ├── task_tools.py # save_tasks, get_tasks + TaskStore +│ │ ├── reflection_tools.py # save_reflection + ToolReflectionStore +│ │ ├── shell/ # 8-layer shell security +│ │ └── webfetch/ # Fetcher, converter, validator, robots +│ ├── knowledge_base/ +│ │ ├── models.py # Document, SearchResult +│ │ ├── embeddings.py # EmbeddingService +│ │ ├── vector_store.py # VectorStore (FAISS) +│ │ ├── _mocks.py # MockEmbeddingService, MockVectorStore +│ │ └── manager.py # KnowledgeBaseManager +│ └── persistence/ +│ ├── session.py # SessionPersistence +│ ├── artifacts.py # ArtifactManager +│ └── _utils.py # Atomic write utilities +├── tests/ +│ ├── conftest.py # MockContext, shared fixtures +│ ├── test_*.py # Unit tests +│ ├── tools/ # Tool-specific tests +│ └── integration/ # ADK & LangGraph pipeline tests +└── examples/ # Demo scripts +``` + +## Running Commands + +**IMPORTANT**: Always use `conda run -n agenticcli` prefix for running commands: + +```bash +# Create the environment (first time only) +conda env create -f environment.yml + +# Install package +conda run -n agenticcli pip install -e . + +# Run tests +conda run -n agenticcli python -m pytest tests/ -v + +# Run Python +conda run -n agenticcli python -c "from agentic_cli import BaseCLIApp; print(BaseCLIApp)" +``` + +## Branching Strategy + +- **main**: Stable branch, matches latest release. Only updated via merges from `develop` when releasing. +- **develop**: Integration branch for ongoing work. Small fixes can be committed directly here. +- **feature/\***: Feature branches for larger changes. Branch from `develop`, merge back to `develop`. +- **fix/\***: Fix branches for fixing issues. Branch from `develop`, merge back to `develop`. +- **refactor/\***: For larger refactoring changes. Branch from `develop`, merge back to `develop`. + +Workflow: +1. For small fixes: commit directly to `develop` +2. For features: create `feature/` (or `fix/` or `refactor/`) from `develop`, work there, merge back to `develop` +3. When ready to release: merge `develop` → `main` and tag the release + +### What NOT to commit +- `docs/` is gitignored on purpose (see `.gitignore`). It is a scratchpad for review notes, plans, and internal analysis. **Never `git add docs/…` or suggest committing anything under `docs/`.** If a document belongs in the repo, it lives elsewhere (README, CHANGELOG, top-level `*.md`). + +## Development Principles + +### Code Style +- Follow PEP 8 style guidelines +- Use type hints throughout +- Prefer descriptive variable names + +### Key Design Decisions +- **Abstract base classes**: BaseCLIApp and BaseWorkflowManager for domain extension +- **Dual orchestrator**: ADK and LangGraph backends, selectable via settings +- **Lazy initialization**: Defer heavy imports until needed +- **Event-based streaming**: Real-time updates via AsyncGenerator +- **UI-agnostic workflow**: WorkflowEvent objects can be consumed by any UI + +### Key Design Patterns +- **Tool error handling**: All tools return `{"success": bool, ...}` dicts. Never raise `ToolError`. +- **Tool registration**: Use `@register_tool(category=..., capabilities=..., description=...)` decorator. `capabilities=` is required — pass `EXEMPT` for tools that need no permission check or a list of `Capability(name, target_arg=...)` tuples the engine matches against rules. Tools are auto-discovered via the global `ToolRegistry`. +- **Permissions**: `workflow/permissions/` holds a framework-independent engine that evaluates declared capabilities against rules from four sources (builtin, user `~/.{app_name}/settings.json`, project `./.{app_name}/settings.json`, in-memory session). ADK + LangGraph gate tool calls via `workflow/adk/permission_plugin.py::PermissionPlugin` and `workflow/langgraph/permission_wrap.py::wrap_tool_for_permission`. See `docs/superpowers/specs/2026-04-18-permissions-system-design.md`. +- **Service registry**: Tools access services and shared state via `get_service(key)` from `workflow.service_registry`. A single ContextVar holds a `dict[str, Any]` set by the workflow manager during processing. Complex services (KBManager, SandboxManager, MemoryStore) are lazily created; simple state (plan string, task list) lives directly in the registry dict. +- **Manager detection**: Tools decorated with `@requires("kb_manager")` etc. are scanned by `BaseWorkflowManager._detect_required_managers()` which lazily creates only the needed services. +- **Atomic writes**: Use `atomic_write_json`/`atomic_write_text` from `persistence/_utils.py` for file persistence. + +### Console Output +All console output must go through `ThinkingPromptSession` methods. Never use `rich.Console` or `print()` directly. + +Available session methods: +- `session.add_response(text, markdown=True)` - Display text/markdown response +- `session.add_rich(renderable)` - Display Rich renderables (Panel, Table, etc.) +- `session.add_message(role, content)` - Add message to history +- `session.add_error(content)` - Display error message +- `session.add_warning(content)` - Display warning message +- `session.add_success(content)` - Display success message +- `session.clear()` - Clear the terminal screen + +## Testing + +- **Framework**: pytest with `asyncio_mode = "auto"` +- **MockContext**: From `tests/conftest.py` — provides isolated settings and temp dirs for all tests +- **MockVectorStore** and **MockEmbeddingService**: In `knowledge_base/_mocks.py` for testing without ML dependencies +- **FAISS tests**: Guard with `pytest.importorskip("faiss")` since FAISS is not installed in dev env +- **Integration tests**: `tests/integration/` covers ADK and LangGraph pipeline tests diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 7017433..4651bf3 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -45,7 +45,7 @@ _ENGINES = ("latexmk", "pdflatex") _LOG_TAIL_LINES = 40 _LOG_TAIL_BYTES = 64 * 1024 -_MAX_CAPTURE_CHARS = 200_000 +_MAX_CAPTURE_BYTES = 200_000 # Only these host env vars reach the TeX process. The tool must not hand the # whole host environment (API keys, tokens) to a subprocess that — on the @@ -134,7 +134,7 @@ def _run( """Run a subprocess in its own process group so a timeout kills the whole tree (latexmk + its pdflatex grandchild), not just the direct child. stdout/stderr are captured to temp files and only the last - _MAX_CAPTURE_CHARS of each are retained, bounding host memory. Seam for + _MAX_CAPTURE_BYTES of each are retained, bounding host memory. Seam for tests. POSIX (macOS/Linux), which is what the framework targets.""" with tempfile.TemporaryFile() as out_f, tempfile.TemporaryFile() as err_f: proc = subprocess.Popen( @@ -148,8 +148,8 @@ def _run( os.killpg(os.getpgid(proc.pid), signal.SIGKILL) proc.wait() # reap the killed group raise - out = _tail_of_file(out_f, _MAX_CAPTURE_CHARS) - err = _tail_of_file(err_f, _MAX_CAPTURE_CHARS) + out = _tail_of_file(out_f, _MAX_CAPTURE_BYTES) + err = _tail_of_file(err_f, _MAX_CAPTURE_BYTES) return subprocess.CompletedProcess(argv, proc.returncode, stdout=out, stderr=err) diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index df2129a..3cf3ac8 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -1,6 +1,7 @@ """Offline tests for compile_document — subprocess and engine lookup faked.""" from __future__ import annotations +import signal import subprocess from pathlib import Path @@ -191,6 +192,7 @@ def wait(self, timeout=None): assert popen_kwargs.get("start_new_session") is True assert len(killpg_calls) >= 1 + assert killpg_calls[0][1] == signal.SIGKILL def test_run_caps_captured_output(tmp_path): @@ -200,8 +202,8 @@ def test_run_caps_captured_output(tmp_path): argv = [_sys.executable, "-c", "print('x' * 1_000_000)"] r = mod._run(argv, cwd=str(tmp_path), env={"PATH": _os.environ.get("PATH", "")}, timeout=30) assert r.returncode == 0 - assert len(r.stdout) <= mod._MAX_CAPTURE_CHARS + 8 # bounded (decode slack) - assert r.stdout.rstrip().endswith("x") # tail retained + assert len(r.stdout) <= mod._MAX_CAPTURE_BYTES + 1 # bounded (byte cap + trailing newline) + assert r.stdout.rstrip().endswith("x") # tail retained # --- P0-3 hardening: env allowlist, no-follow delivery, capability scope --- From e4ad61af199899c4fa928a06301e16eb6c82a4c2 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:33:22 -0400 Subject: [PATCH 15/22] fix(document): exact TEXMF env allowlist instead of prefix (P0-3) Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 14 +++++++++++--- tests/tools/test_document_compile.py | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 4651bf3..bc524cc 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -65,12 +65,20 @@ "TEXCONFIG", "TEXDOCS", "TEXSOURCES", ) +# The kpathsea TEXMF* configuration variables (exact — a strict secret boundary, +# so a name like TEXMF_SECRET is not passed through). +_TEXMF_VARS = ( + "TEXMFHOME", "TEXMFVAR", "TEXMFCONFIG", "TEXMFCACHE", "TEXMFLOCAL", + "TEXMFDIST", "TEXMFMAIN", "TEXMFSYSVAR", "TEXMFSYSCONFIG", "TEXMFDBS", + "TEXMFCNF", "TEXMFOUTPUT", +) + def _build_env(assets_dir: str | None, source_dir: str | None = None) -> dict[str, str]: """Minimal, allowlisted environment for the TeX subprocess. - Passes PATH/HOME/locale plus TeX's own configuration variables — the - ``TEXMF*`` tree and a fixed set of other TeX vars — so a custom + Passes PATH/HOME/locale plus TeX's own configuration variables — an exact + list of ``TEXMF*`` vars and a fixed set of other TeX vars — so a custom ``TEXMFHOME`` etc. keeps working, but not arbitrary host env (a name like ``TEXT_API_TOKEN`` starts with "TEX" yet is not a TeX var), and never the caller's ``TEXINPUTS`` (set explicitly below). @@ -81,7 +89,7 @@ def _build_env(assets_dir: str | None, source_dir: str | None = None) -> dict[st env = { k: v for k, v in os.environ.items() - if k in _ENV_PASSTHROUGH or k.startswith("TEXMF") or k in _TEX_VARS + if k in _ENV_PASSTHROUGH or k in _TEXMF_VARS or k in _TEX_VARS } env.setdefault("PATH", os.defpath) roots = [r for r in (assets_dir, source_dir) if r] diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index 3cf3ac8..e944349 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -329,6 +329,25 @@ def fake_run(argv, *, cwd, env, timeout): assert captured["env"].get("TEXMFHOME") == "/home/u/texmf" +def test_env_texmf_uses_exact_allowlist_not_prefix(monkeypatch, tmp_path): + """A real TEXMF var passes; a TEXMF-prefixed non-var (potential secret) does not.""" + _fake_engine(monkeypatch) + monkeypatch.setenv("TEXMFHOME", "/home/u/texmf") + monkeypatch.setenv("TEXMF_SECRET", "leak") + tex = tmp_path / "r.tex"; tex.write_text("x") + captured = {} + + def fake_run(argv, *, cwd, env, timeout): + captured["env"] = env + (Path(cwd) / "r.pdf").write_bytes(b"%PDF") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + compile_document(str(tex)) + assert captured["env"].get("TEXMFHOME") == "/home/u/texmf" + assert "TEXMF_SECRET" not in captured["env"] + + def test_assets_dir_with_path_separator_rejected(monkeypatch, tmp_path): """assets_dir authorized as one filesystem path must not smuggle extra TEXINPUTS roots via os.pathsep (e.g. 'assets:/etc').""" From a5d6fd36cbd9af1903a4782cd47d57baa6e52954 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:38:34 -0400 Subject: [PATCH 16/22] fix(tools): pre-limit scan/size ceilings for glob and grep (P0-4) Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/glob_tool.py | 15 ++++++++--- src/agentic_cli/tools/grep_tool.py | 33 ++++++++++++++--------- tests/tools/test_glob_grep_containment.py | 26 ++++++++++++++++++ 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/agentic_cli/tools/glob_tool.py b/src/agentic_cli/tools/glob_tool.py index dab2d5a..784acc0 100644 --- a/src/agentic_cli/tools/glob_tool.py +++ b/src/agentic_cli/tools/glob_tool.py @@ -16,6 +16,8 @@ ) from agentic_cli.workflow.permissions import Capability +_MAX_SCAN = 10_000 # hard ceiling on matches materialized before sorting/limiting + @register_tool( category=ToolCategory.READ, @@ -82,8 +84,15 @@ def glob( "path": str(search_path), } - # Find matching files - matches = list(search_path.glob(pattern)) + # Find matching files, capping how many we materialize (a pathological + # pattern like "**/*" over a huge tree must not exhaust memory). + matches = [] + scan_truncated = False + for p in search_path.glob(pattern): + matches.append(p) + if len(matches) >= _MAX_SCAN: + scan_truncated = True + break # Filter results filtered = [] @@ -115,7 +124,7 @@ def glob( filtered.sort(key=lambda p: p.stat().st_mtime, reverse=True) # Truncate if needed - truncated = len(filtered) > max_results + truncated = scan_truncated or len(filtered) > max_results filtered = filtered[:max_results] # Format output diff --git a/src/agentic_cli/tools/grep_tool.py b/src/agentic_cli/tools/grep_tool.py index c7b0da5..6654e34 100644 --- a/src/agentic_cli/tools/grep_tool.py +++ b/src/agentic_cli/tools/grep_tool.py @@ -18,6 +18,9 @@ ) from agentic_cli.workflow.permissions import Capability +_MAX_FILES = 10_000 # cap the number of files the Python fallback scans +_MAX_FILE_BYTES = 5_000_000 # skip files larger than this (avoid reading whole huge files) + @register_tool( category=ToolCategory.READ, @@ -284,20 +287,19 @@ def _grep_python( total_matches = 0 file_counts: dict[str, int] = {} - # Get files to search + # Get files to search, capping how many we materialize. if path.is_file(): - files = [path] + candidates = iter([path]) + elif file_pattern: + candidates = path.rglob(file_pattern) if recursive else path.glob(file_pattern) else: - if file_pattern: - if recursive: - files = list(path.rglob(file_pattern)) - else: - files = list(path.glob(file_pattern)) - else: - if recursive: - files = [f for f in path.rglob("*") if f.is_file()] - else: - files = [f for f in path.iterdir() if f.is_file()] + candidates = path.rglob("*") if recursive else path.iterdir() + + files = [] + for f in candidates: + files.append(f) + if len(files) >= _MAX_FILES: + break for file_path in files: if not file_path.is_file(): @@ -308,6 +310,13 @@ def _grep_python( if not path_is_within(file_path, path): continue + # Skip files that are too large to read whole (reliability bound). + try: + if file_path.stat().st_size > _MAX_FILE_BYTES: + continue + except OSError: + continue + try: content = file_path.read_text() lines = content.splitlines() diff --git a/tests/tools/test_glob_grep_containment.py b/tests/tools/test_glob_grep_containment.py index fd297de..34503a6 100644 --- a/tests/tools/test_glob_grep_containment.py +++ b/tests/tools/test_glob_grep_containment.py @@ -9,6 +9,7 @@ import json import subprocess +import agentic_cli.tools.glob_tool as glob_mod import agentic_cli.tools.grep_tool as grep_mod from agentic_cli.tools.glob_tool import glob from agentic_cli.tools.grep_tool import grep @@ -160,3 +161,28 @@ def test_glob_excludes_hidden_ancestor(tmp_path): assert r["success"] is True assert all(".hidden" not in f for f in r["files"]) assert any("visible.txt" in f for f in r["files"]) + + +def test_glob_caps_scanned_matches(tmp_path, monkeypatch): + root = tmp_path / "root" + root.mkdir() + for i in range(6): + (root / f"f{i}.txt").write_text("x") + monkeypatch.setattr(glob_mod, "_MAX_SCAN", 3) + r = glob(pattern="*", path=str(root), max_results=100) + assert r["success"] is True + assert len(r["files"]) <= 3 + assert r["truncated"] is True + + +def test_grep_python_skips_oversized_files(tmp_path, monkeypatch): + root = tmp_path / "root" + root.mkdir() + (root / "small.txt").write_text("needle here") + (root / "big.txt").write_text("needle " + ("x" * 1000)) + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: False) + monkeypatch.setattr(grep_mod, "_MAX_FILE_BYTES", 100) + r = grep(pattern="needle", path=str(root)) + files = {m["file"] for m in r["matches"]} + assert any("small.txt" in f for f in files) + assert not any("big.txt" in f for f in files) # oversized file skipped From 59a45b8c20f526665c82f59456887a950cce48f2 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:54:49 -0400 Subject: [PATCH 17/22] chore: stop tracking AGENTS.md Swept into 72e35ed by a 'git add -A'; it was meant to stay an untracked local file. Remove from the tree (kept on disk, untracked). Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- AGENTS.md | 160 ------------------------------------------------------ 1 file changed, 160 deletions(-) delete mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md deleted file mode 100644 index ddece9f..0000000 --- a/AGENTS.md +++ /dev/null @@ -1,160 +0,0 @@ -# Agentic CLI - Shared Framework for Agentic Applications - -## Project Overview - -Agentic CLI is a shared library providing the core infrastructure for building domain-specific CLI applications powered by LLM agents. - -## Tech Stack - -- **Language**: Python 3.12+ -- **CLI UI**: `thinking-prompt` - enhanced CLI with thinking boxes and markdown -- **Workflow**: Google ADK + LangGraph - dual orchestration backends (selectable via settings) -- **Config**: `pydantic-settings` - type-safe configuration -- **Logging**: `structlog` - structured logging - -## Project Structure - -``` -agentic-cli/ -├── src/agentic_cli/ -│ ├── __init__.py # Package exports, lazy imports -│ ├── config.py # BaseSettings (pydantic-settings) -│ ├── constants.py # Shared constants, truncate() -│ ├── settings_persistence.py -│ ├── logging.py -│ ├── cli/ -│ │ ├── app.py # BaseCLIApp -│ │ ├── commands.py # Command, CommandRegistry -│ │ ├── builtin_commands.py -│ │ ├── workflow_controller.py # WorkflowController, factory -│ │ ├── message_processor.py -│ │ └── settings*.py # Settings UI (introspection, dialog) -│ ├── workflow/ -│ │ ├── base_manager.py # BaseWorkflowManager (abstract) -│ │ ├── task_progress.py # build_task_progress_event(), parse_plan_progress() -│ │ ├── events.py # WorkflowEvent, EventType -│ │ ├── thinking.py # ThinkingDetector -│ │ ├── config.py # AgentConfig -│ │ ├── context.py # ContextVars for tool access (get_context_*()) -│ │ ├── adk/ # ADK orchestrator -│ │ │ ├── manager.py # GoogleADKWorkflowManager -│ │ │ ├── event_processor.py # ADKEventProcessor -│ │ │ └── llm_event_logger.py # LLM traffic logging -│ │ └── langgraph/ # LangGraph orchestrator -│ │ ├── manager.py # LangGraphWorkflowManager -│ │ ├── graph_builder.py # LangGraphBuilder (graph + LLM factory) -│ │ ├── state.py -│ │ ├── persistence/ # Checkpointers, stores -│ │ └── tools/ # LangChain-compatible wrappers -│ ├── tools/ -│ │ ├── registry.py # ToolRegistry, @register_tool, ToolCategory -│ │ ├── executor.py # SafePythonExecutor -│ │ ├── knowledge_tools.py # kb_search, kb_ingest, kb_list, kb_read -│ │ ├── arxiv_tools.py # search_arxiv, fetch_arxiv_paper, analyze_arxiv_paper -│ │ ├── execution_tools.py # execute_python -│ │ ├── interaction_tools.py # ask_clarification -│ │ ├── file_read.py # read_file, diff_compare -│ │ ├── file_write.py # write_file, edit_file -│ │ ├── glob_tool.py # glob -│ │ ├── grep_tool.py # grep -│ │ ├── search.py # web_search (Tavily/Brave backends) -│ │ ├── webfetch_tool.py # web_fetch (orchestrator) -│ │ ├── memory_tools.py # save_memory, search_memory + MemoryStore -│ │ ├── planning_tools.py # save_plan, get_plan + PlanStore -│ │ ├── task_tools.py # save_tasks, get_tasks + TaskStore -│ │ ├── reflection_tools.py # save_reflection + ToolReflectionStore -│ │ ├── shell/ # 8-layer shell security -│ │ └── webfetch/ # Fetcher, converter, validator, robots -│ ├── knowledge_base/ -│ │ ├── models.py # Document, SearchResult -│ │ ├── embeddings.py # EmbeddingService -│ │ ├── vector_store.py # VectorStore (FAISS) -│ │ ├── _mocks.py # MockEmbeddingService, MockVectorStore -│ │ └── manager.py # KnowledgeBaseManager -│ └── persistence/ -│ ├── session.py # SessionPersistence -│ ├── artifacts.py # ArtifactManager -│ └── _utils.py # Atomic write utilities -├── tests/ -│ ├── conftest.py # MockContext, shared fixtures -│ ├── test_*.py # Unit tests -│ ├── tools/ # Tool-specific tests -│ └── integration/ # ADK & LangGraph pipeline tests -└── examples/ # Demo scripts -``` - -## Running Commands - -**IMPORTANT**: Always use `conda run -n agenticcli` prefix for running commands: - -```bash -# Create the environment (first time only) -conda env create -f environment.yml - -# Install package -conda run -n agenticcli pip install -e . - -# Run tests -conda run -n agenticcli python -m pytest tests/ -v - -# Run Python -conda run -n agenticcli python -c "from agentic_cli import BaseCLIApp; print(BaseCLIApp)" -``` - -## Branching Strategy - -- **main**: Stable branch, matches latest release. Only updated via merges from `develop` when releasing. -- **develop**: Integration branch for ongoing work. Small fixes can be committed directly here. -- **feature/\***: Feature branches for larger changes. Branch from `develop`, merge back to `develop`. -- **fix/\***: Fix branches for fixing issues. Branch from `develop`, merge back to `develop`. -- **refactor/\***: For larger refactoring changes. Branch from `develop`, merge back to `develop`. - -Workflow: -1. For small fixes: commit directly to `develop` -2. For features: create `feature/` (or `fix/` or `refactor/`) from `develop`, work there, merge back to `develop` -3. When ready to release: merge `develop` → `main` and tag the release - -### What NOT to commit -- `docs/` is gitignored on purpose (see `.gitignore`). It is a scratchpad for review notes, plans, and internal analysis. **Never `git add docs/…` or suggest committing anything under `docs/`.** If a document belongs in the repo, it lives elsewhere (README, CHANGELOG, top-level `*.md`). - -## Development Principles - -### Code Style -- Follow PEP 8 style guidelines -- Use type hints throughout -- Prefer descriptive variable names - -### Key Design Decisions -- **Abstract base classes**: BaseCLIApp and BaseWorkflowManager for domain extension -- **Dual orchestrator**: ADK and LangGraph backends, selectable via settings -- **Lazy initialization**: Defer heavy imports until needed -- **Event-based streaming**: Real-time updates via AsyncGenerator -- **UI-agnostic workflow**: WorkflowEvent objects can be consumed by any UI - -### Key Design Patterns -- **Tool error handling**: All tools return `{"success": bool, ...}` dicts. Never raise `ToolError`. -- **Tool registration**: Use `@register_tool(category=..., capabilities=..., description=...)` decorator. `capabilities=` is required — pass `EXEMPT` for tools that need no permission check or a list of `Capability(name, target_arg=...)` tuples the engine matches against rules. Tools are auto-discovered via the global `ToolRegistry`. -- **Permissions**: `workflow/permissions/` holds a framework-independent engine that evaluates declared capabilities against rules from four sources (builtin, user `~/.{app_name}/settings.json`, project `./.{app_name}/settings.json`, in-memory session). ADK + LangGraph gate tool calls via `workflow/adk/permission_plugin.py::PermissionPlugin` and `workflow/langgraph/permission_wrap.py::wrap_tool_for_permission`. See `docs/superpowers/specs/2026-04-18-permissions-system-design.md`. -- **Service registry**: Tools access services and shared state via `get_service(key)` from `workflow.service_registry`. A single ContextVar holds a `dict[str, Any]` set by the workflow manager during processing. Complex services (KBManager, SandboxManager, MemoryStore) are lazily created; simple state (plan string, task list) lives directly in the registry dict. -- **Manager detection**: Tools decorated with `@requires("kb_manager")` etc. are scanned by `BaseWorkflowManager._detect_required_managers()` which lazily creates only the needed services. -- **Atomic writes**: Use `atomic_write_json`/`atomic_write_text` from `persistence/_utils.py` for file persistence. - -### Console Output -All console output must go through `ThinkingPromptSession` methods. Never use `rich.Console` or `print()` directly. - -Available session methods: -- `session.add_response(text, markdown=True)` - Display text/markdown response -- `session.add_rich(renderable)` - Display Rich renderables (Panel, Table, etc.) -- `session.add_message(role, content)` - Add message to history -- `session.add_error(content)` - Display error message -- `session.add_warning(content)` - Display warning message -- `session.add_success(content)` - Display success message -- `session.clear()` - Clear the terminal screen - -## Testing - -- **Framework**: pytest with `asyncio_mode = "auto"` -- **MockContext**: From `tests/conftest.py` — provides isolated settings and temp dirs for all tests -- **MockVectorStore** and **MockEmbeddingService**: In `knowledge_base/_mocks.py` for testing without ML dependencies -- **FAISS tests**: Guard with `pytest.importorskip("faiss")` since FAISS is not installed in dev env -- **Integration tests**: `tests/integration/` covers ADK and LangGraph pipeline tests From 51da5e5059519ea1d4b049a3cbb5174e21831b21 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Fri, 10 Jul 2026 23:57:42 -0400 Subject: [PATCH 18/22] fix(document.compile): absolute TEXINPUTS roots, ignore_cleanup_errors, delivery pdf_path=None I-1: resolve assets_dir/source_dir to absolute before setting TEXINPUTS so relative paths don't mis-resolve when the TeX engine runs in a private temp build dir. M-1: TemporaryDirectory(ignore_cleanup_errors=True) to guard rare OSError on cleanup. M-2: delivery-failure return now sets pdf_path=None (the temp dir is gone on return); update matching test assertion. T1: fix output_pdf docstring clause ("isolated in a private temp dir"). Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 12 +++++++---- tests/tools/test_document_compile.py | 25 ++++++++++++++++++++++- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index bc524cc..1a43474 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -92,7 +92,11 @@ def _build_env(assets_dir: str | None, source_dir: str | None = None) -> dict[st if k in _ENV_PASSTHROUGH or k in _TEXMF_VARS or k in _TEX_VARS } env.setdefault("PATH", os.defpath) - roots = [r for r in (assets_dir, source_dir) if r] + roots = [ + str(Path(r).expanduser().resolve()) + for r in (assets_dir, source_dir) + if r + ] if roots: # Trailing empty entry lets kpathsea append its default search path. env["TEXINPUTS"] = os.pathsep.join(roots) + os.pathsep @@ -251,7 +255,7 @@ def compile_document( Args: source_path: Path to the .tex file to compile. output_pdf: If set, the produced PDF is copied here (parents created); - build intermediates stay in the source's directory. + build intermediates are isolated in a private temp dir. assets_dir: Directory prepended to TEXINPUTS so figures/resources resolve by bare name (e.g. an artifacts dir). engine: Force an engine ("latexmk"/"pdflatex"); default auto-detects @@ -297,7 +301,7 @@ def compile_document( argv = _build_argv(chosen, _safe_source_arg(src.name)) start = time.monotonic() - with tempfile.TemporaryDirectory(prefix="texbuild-") as build_dir: + with tempfile.TemporaryDirectory(prefix="texbuild-", ignore_cleanup_errors=True) as build_dir: build = Path(build_dir) try: shutil.copy2(src, build / src.name) @@ -344,7 +348,7 @@ def compile_document( return { "success": False, "error": f"Failed to deliver PDF to {dest}: {exc}", - "pdf_path": str(produced), "engine": chosen, + "pdf_path": None, "engine": chosen, "log_tail": log_tail, "errors": [], "duration_ms": duration_ms, } diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index e944349..f5affdf 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -1,6 +1,7 @@ """Offline tests for compile_document — subprocess and engine lookup faked.""" from __future__ import annotations +import os import signal import subprocess from pathlib import Path @@ -147,7 +148,7 @@ def fake_run(argv, *, cwd, env, timeout): r = compile_document(str(tex), output_pdf=str(out)) assert r["success"] is False assert "deliver" in r["error"].lower() or str(out) in r["error"] - assert r["pdf_path"] is not None # PDF still exists in build dir + assert r["pdf_path"] is None # build dir (with the PDF) is cleaned up on return assert "duration_ms" in r @@ -442,3 +443,25 @@ def boom(*a, **k): monkeypatch.setattr("builtins.open", boom) assert mod._read_log_tail(log, fallback="FB") == "FB" + + +def test_texinputs_roots_are_absolute_for_relative_source(monkeypatch, tmp_path): + """Relative source_path/assets_dir must resolve to ABSOLUTE TEXINPUTS roots + (the build runs in a temp dir, so relative roots would resolve there).""" + _fake_engine(monkeypatch) + monkeypatch.chdir(tmp_path) + (tmp_path / "assets").mkdir() + (tmp_path / "r.tex").write_text("x") + captured = {} + + def fake_run(argv, *, cwd, env, timeout): + captured["env"] = env + (Path(cwd) / "r.pdf").write_bytes(b"%PDF") + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(mod, "_run", fake_run) + compile_document("r.tex", assets_dir="assets") # relative paths + entries = [e for e in captured["env"]["TEXINPUTS"].split(os.pathsep) if e] + assert entries + assert all(os.path.isabs(e) for e in entries) + assert str((tmp_path / "assets").resolve()) in entries From 92ade462bcc8fea314382f60240c53baeaacc9cb Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:51:47 -0400 Subject: [PATCH 19/22] test(document): fix gated LaTeX test for the temp-build isolation contract test_document_compile_latex.py asserted the .log stays in the source dir; after the private-temp-build change intermediates are isolated (temp dir, cleaned up), so on a real engine that assertion would fail. The test is @pytest.mark.latex (skipped here), so it slipped past the offline suite. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- tests/tools/test_document_compile_latex.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_document_compile_latex.py b/tests/tools/test_document_compile_latex.py index 8fec1c2..5e72181 100644 --- a/tests/tools/test_document_compile_latex.py +++ b/tests/tools/test_document_compile_latex.py @@ -31,5 +31,7 @@ def test_compiles_minimal_document(tmp_path): assert r["success"] is True, r assert Path(r["pdf_path"]).is_file() and Path(r["pdf_path"]).stat().st_size > 0 assert out.is_file() - assert (build / "r.log").is_file() # intermediates in build dir + # Isolation contract: the build runs in a private temp dir (cleaned up), so + # intermediates never land in the source dir or beside the delivered PDF. + assert not (build / "r.log").exists() # source dir stays clean assert not (out.parent / "r.log").exists() # not beside delivered PDF From 2dd8b15322a0ab2f27ff6ed994523f459cda8833 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:53:27 -0400 Subject: [PATCH 20/22] fix(grep): don't let directories consume the file budget; report scan truncation (P0-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _grep_python counted directories against _MAX_FILES and never set truncated when the cap was hit, so N directories before a matching file returned no matches with truncated=False — a silently-wrong result. Count only files against the budget and set truncated when the file cap is reached. Also tighten the glob scan-cap test to an exact count. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/grep_tool.py | 8 +++++- tests/tools/test_glob_grep_containment.py | 32 ++++++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/agentic_cli/tools/grep_tool.py b/src/agentic_cli/tools/grep_tool.py index 6654e34..b289b95 100644 --- a/src/agentic_cli/tools/grep_tool.py +++ b/src/agentic_cli/tools/grep_tool.py @@ -296,9 +296,15 @@ def _grep_python( candidates = path.rglob("*") if recursive else path.iterdir() files = [] + scan_truncated = False for f in candidates: + # Count only files against the budget — directories must not exhaust it + # (otherwise dirs before the files silently drop matches). + if not f.is_file(): + continue files.append(f) if len(files) >= _MAX_FILES: + scan_truncated = True break for file_path in files: @@ -366,5 +372,5 @@ def _grep_python( "matches": matches, "total_matches": total_matches, "files_searched": files_searched, - "truncated": total_matches > max_results, + "truncated": scan_truncated or total_matches > max_results, } diff --git a/tests/tools/test_glob_grep_containment.py b/tests/tools/test_glob_grep_containment.py index 34503a6..66e4992 100644 --- a/tests/tools/test_glob_grep_containment.py +++ b/tests/tools/test_glob_grep_containment.py @@ -171,7 +171,7 @@ def test_glob_caps_scanned_matches(tmp_path, monkeypatch): monkeypatch.setattr(glob_mod, "_MAX_SCAN", 3) r = glob(pattern="*", path=str(root), max_results=100) assert r["success"] is True - assert len(r["files"]) <= 3 + assert len(r["files"]) == 3 # exactly the ceiling (6 files, all pass filters) assert r["truncated"] is True @@ -186,3 +186,33 @@ def test_grep_python_skips_oversized_files(tmp_path, monkeypatch): files = {m["file"] for m in r["matches"]} assert any("small.txt" in f for f in files) assert not any("big.txt" in f for f in files) # oversized file skipped + + +def test_grep_python_reports_truncated_when_file_cap_hit(tmp_path, monkeypatch): + """Hitting the _MAX_FILES scan ceiling must be reported as truncated, not + silently dropped (else a partial result claims to be complete).""" + root = tmp_path / "root" + root.mkdir() + for i in range(3): + (root / f"f{i}.txt").write_text("needle") + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: False) + monkeypatch.setattr(grep_mod, "_MAX_FILES", 2) # fewer than the 3 files + r = grep(pattern="needle", path=str(root)) + assert r["truncated"] is True + + +def test_grep_python_directories_do_not_consume_file_budget(tmp_path, monkeypatch): + """Directories must not count against _MAX_FILES — otherwise dirs before the + files exhaust the budget and matches are silently missed.""" + root = tmp_path / "root" + root.mkdir() + for i in range(20): + (root / f"dir{i}").mkdir() + (root / "a.txt").write_text("needle") + (root / "b.txt").write_text("needle") + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: False) + monkeypatch.setattr(grep_mod, "_MAX_FILES", 2) # exactly the 2 real files + r = grep(pattern="needle", path=str(root)) + files = {m["file"] for m in r["matches"]} + assert any("a.txt" in f for f in files) + assert any("b.txt" in f for f in files) From 2ea7573329b4c9977c30d46d3e7d70d692878ac1 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Sat, 11 Jul 2026 00:56:23 -0400 Subject: [PATCH 21/22] fix(document): cap TeX child file size and CPU (RLIMIT_FSIZE/CPU) (P0-3) _run bounded retained memory but the child could still fill host disk or burn CPU within the wall-clock timeout. Add a preexec_fn that sets RLIMIT_FSIZE (caps any file the child writes, incl. the redirected stdout/stderr temp files) and RLIMIT_CPU (backstop to the timeout). Best-effort; POSIX. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/document/compile.py | 25 +++++++++++++++++++++-- tests/tools/test_document_compile.py | 13 ++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/agentic_cli/tools/document/compile.py b/src/agentic_cli/tools/document/compile.py index 1a43474..d2ad630 100644 --- a/src/agentic_cli/tools/document/compile.py +++ b/src/agentic_cli/tools/document/compile.py @@ -46,6 +46,9 @@ _LOG_TAIL_LINES = 40 _LOG_TAIL_BYTES = 64 * 1024 _MAX_CAPTURE_BYTES = 200_000 +# Cap the size of any single file the TeX child writes (bounds runaway-`.tex` +# disk use, incl. the log/pdf/aux and our redirected stdout/stderr temp files). +_RLIMIT_FSIZE_BYTES = 500 * 1024 * 1024 # Only these host env vars reach the TeX process. The tool must not hand the # whole host environment (API keys, tokens) to a subprocess that — on the @@ -140,19 +143,37 @@ def _which(name: str) -> str | None: return shutil.which(name) +def _child_rlimits(cpu_seconds: int): + """Build a ``preexec_fn`` that caps the child's file size and CPU time, so a + runaway ``.tex`` can't fill the disk or burn CPU within the wall-clock + timeout. Best-effort (a platform without ``resource`` just skips it).""" + def _apply() -> None: # runs in the forked child, before exec + try: + import resource + resource.setrlimit( + resource.RLIMIT_FSIZE, (_RLIMIT_FSIZE_BYTES, _RLIMIT_FSIZE_BYTES) + ) + resource.setrlimit(resource.RLIMIT_CPU, (cpu_seconds, cpu_seconds)) + except (ValueError, OSError, ImportError): + pass + return _apply + + def _run( argv: list[str], *, cwd: str, env: dict[str, str], timeout: float ) -> subprocess.CompletedProcess: """Run a subprocess in its own process group so a timeout kills the whole tree (latexmk + its pdflatex grandchild), not just the direct child. stdout/stderr are captured to temp files and only the last - _MAX_CAPTURE_BYTES of each are retained, bounding host memory. Seam for - tests. POSIX (macOS/Linux), which is what the framework targets.""" + _MAX_CAPTURE_BYTES of each are retained, bounding host memory. The child + also runs under RLIMIT_FSIZE/RLIMIT_CPU limits. Seam for tests. POSIX + (macOS/Linux), which is what the framework targets.""" with tempfile.TemporaryFile() as out_f, tempfile.TemporaryFile() as err_f: proc = subprocess.Popen( argv, cwd=cwd, env=env, stdout=out_f, stderr=err_f, start_new_session=True, + preexec_fn=_child_rlimits(int(timeout) + 30), ) try: proc.wait(timeout=timeout) diff --git a/tests/tools/test_document_compile.py b/tests/tools/test_document_compile.py index f5affdf..cc1446d 100644 --- a/tests/tools/test_document_compile.py +++ b/tests/tools/test_document_compile.py @@ -465,3 +465,16 @@ def fake_run(argv, *, cwd, env, timeout): assert entries assert all(os.path.isabs(e) for e in entries) assert str((tmp_path / "assets").resolve()) in entries + + +def test_run_limits_child_file_size(monkeypatch, tmp_path): + """A runaway child writing beyond RLIMIT_FSIZE is killed (SIGXFSZ), not + allowed to fill the disk.""" + import os as _os + import sys as _sys + + monkeypatch.setattr(mod, "_RLIMIT_FSIZE_BYTES", 4096) + argv = [_sys.executable, "-c", "open('big.bin','wb').write(b'x' * 1_000_000)"] + r = mod._run(argv, cwd=str(tmp_path), env={"PATH": _os.environ.get("PATH", "")}, timeout=30) + assert r.returncode != 0 # killed by the file-size limit + assert (tmp_path / "big.bin").stat().st_size <= 4096 * 8 # capped, not 1MB From 22aa503aed40c35a6809c38331c1e66b596fcff4 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Sat, 11 Jul 2026 01:00:23 -0400 Subject: [PATCH 22/22] fix(grep): bound ripgrep output to a temp file + capped read (P0-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _grep_with_ripgrep used subprocess.run(capture_output=True) — a large tree could allocate unbounded JSON in host memory (rg's --max-count is per-file). Capture rg stdout to a temp file, read back at most _MAX_RG_OUTPUT_BYTES, and flag truncation; preserve the timeout + process-group kill and the RIPGREP_CONFIG_PATH scrub. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/tools/grep_tool.py | 51 ++++++++++++++--------- tests/test_grep_tool_security.py | 22 ++++++---- tests/tools/test_glob_grep_containment.py | 51 +++++++++++++++++++---- 3 files changed, 90 insertions(+), 34 deletions(-) diff --git a/src/agentic_cli/tools/grep_tool.py b/src/agentic_cli/tools/grep_tool.py index b289b95..eb48448 100644 --- a/src/agentic_cli/tools/grep_tool.py +++ b/src/agentic_cli/tools/grep_tool.py @@ -6,8 +6,10 @@ import functools import os -import re +import signal import subprocess +import tempfile +import re from pathlib import Path from typing import Any, Literal @@ -20,6 +22,7 @@ _MAX_FILES = 10_000 # cap the number of files the Python fallback scans _MAX_FILE_BYTES = 5_000_000 # skip files larger than this (avoid reading whole huge files) +_MAX_RG_OUTPUT_BYTES = 10_000_000 # cap ripgrep JSON we read into memory @register_tool( @@ -166,23 +169,31 @@ def _grep_with_ripgrep( # --follow, which would make rg traverse symlinks out of the authorized # root). Containment below is the backstop; this removes the vector. rg_env = {k: v for k, v in os.environ.items() if k != "RIPGREP_CONFIG_PATH"} + # Capture rg output to a temp file and read back at most _MAX_RG_OUTPUT_BYTES + # so a large tree can't allocate unbounded JSON in host memory. + rg_truncated = False try: - result = subprocess.run( - cmd, - capture_output=True, - text=True, - timeout=30, - env=rg_env, - ) - except subprocess.TimeoutExpired: - return { - "success": False, - "error": "Search timed out after 30 seconds", - "matches": [], - "total_matches": 0, - "files_searched": 0, - "truncated": False, - } + with tempfile.TemporaryFile(mode="w+b") as out_f: + proc = subprocess.Popen( + cmd, stdout=out_f, stderr=subprocess.DEVNULL, + env=rg_env, start_new_session=True, + ) + try: + proc.wait(timeout=30) + except subprocess.TimeoutExpired: + os.killpg(os.getpgid(proc.pid), signal.SIGKILL) + proc.wait() + return { + "success": False, + "error": "Search timed out after 30 seconds", + "matches": [], + "total_matches": 0, + "files_searched": 0, + "truncated": False, + } + out_f.seek(0) + raw = out_f.read(_MAX_RG_OUTPUT_BYTES) + rg_truncated = bool(out_f.read(1)) # more output than the cap remained except FileNotFoundError: # Ripgrep not found, fall back to Python return _grep_python( @@ -197,6 +208,8 @@ def _grep_with_ripgrep( output_mode=output_mode, ) + stdout_text = raw.decode("utf-8", errors="replace") + # Parse ripgrep JSON output import json @@ -205,7 +218,7 @@ def _grep_with_ripgrep( file_counts: dict[str, int] = {} total_matches = 0 - for line in result.stdout.strip().split("\n"): + for line in stdout_text.strip().split("\n"): if not line: continue try: @@ -249,7 +262,7 @@ def _grep_with_ripgrep( "matches": matches, "total_matches": total_matches, "files_searched": len(files_searched), - "truncated": len(matches) >= max_results, + "truncated": rg_truncated or len(matches) >= max_results, } diff --git a/tests/test_grep_tool_security.py b/tests/test_grep_tool_security.py index 75fff5a..df08b30 100644 --- a/tests/test_grep_tool_security.py +++ b/tests/test_grep_tool_security.py @@ -6,24 +6,32 @@ """ from pathlib import Path -from unittest.mock import MagicMock, patch +from unittest.mock import patch from agentic_cli.tools.grep_tool import grep +class _FakeProc: + pid = 4321 + returncode = 0 + + def wait(self, timeout=None): + return 0 + + def _run_grep_capturing_argv(tmp_path: Path, pattern: str, rg_stdout: str = ""): """Call grep() forcing the ripgrep path and capture the argv built.""" captured = {} - def fake_run(cmd, *args, **kwargs): + def fake_popen(cmd, *args, **kwargs): captured["cmd"] = cmd - result = MagicMock() - result.stdout = rg_stdout - result.returncode = 0 - return result + out = kwargs.get("stdout") + if out is not None and rg_stdout: + out.write(rg_stdout.encode()) + return _FakeProc() with patch("agentic_cli.tools.grep_tool._ripgrep_available", return_value=True), \ - patch("agentic_cli.tools.grep_tool.subprocess.run", side_effect=fake_run): + patch("agentic_cli.tools.grep_tool.subprocess.Popen", side_effect=fake_popen): out = grep(pattern=pattern, path=str(tmp_path)) return captured["cmd"], out diff --git a/tests/tools/test_glob_grep_containment.py b/tests/tools/test_glob_grep_containment.py index 66e4992..ab09e3f 100644 --- a/tests/tools/test_glob_grep_containment.py +++ b/tests/tools/test_glob_grep_containment.py @@ -15,6 +15,15 @@ from agentic_cli.tools.grep_tool import grep +class _FakeProc: + """Stand-in for a ripgrep subprocess (grep now uses Popen + a temp file).""" + pid = 4321 + returncode = 0 + + def wait(self, timeout=None): + return 0 + + def test_glob_rejects_parent_escape(tmp_path): root = tmp_path / "root" root.mkdir() @@ -113,18 +122,20 @@ def test_grep_ripgrep_filters_outside_root(tmp_path, monkeypatch): outside = tmp_path / "outside" / "secret.txt" inside = root / "real.txt" - def fake_run(cmd, **kwargs): - lines = [ + def fake_popen(cmd, **kwargs): + out = kwargs["stdout"] + lines = "\n".join([ json.dumps({"type": "match", "data": { "path": {"text": str(outside)}, "line_number": 1, "lines": {"text": "needle SECRET\n"}}}), json.dumps({"type": "match", "data": { "path": {"text": str(inside)}, "line_number": 1, "lines": {"text": "needle here\n"}}}), - ] - return subprocess.CompletedProcess(cmd, 0, stdout="\n".join(lines), stderr="") + ]) + "\n" + out.write(lines.encode()) + return _FakeProc() - monkeypatch.setattr(grep_mod.subprocess, "run", fake_run) + monkeypatch.setattr(grep_mod.subprocess, "Popen", fake_popen) r = grep(pattern="needle", path=str(root)) files = {m["file"] for m in r["matches"]} assert any("real.txt" in f for f in files) @@ -140,16 +151,40 @@ def test_grep_ripgrep_scrubs_config_path_env(tmp_path, monkeypatch): monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: True) captured = {} - def fake_run(cmd, **kwargs): + def fake_popen(cmd, **kwargs): captured["env"] = kwargs.get("env") - return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + return _FakeProc() - monkeypatch.setattr(grep_mod.subprocess, "run", fake_run) + monkeypatch.setattr(grep_mod.subprocess, "Popen", fake_popen) grep(pattern="x", path=str(root)) assert captured["env"] is not None assert "RIPGREP_CONFIG_PATH" not in captured["env"] +def test_grep_ripgrep_output_bounded(tmp_path, monkeypatch): + """rg output beyond _MAX_RG_OUTPUT_BYTES is not read into memory whole; the + result is flagged truncated.""" + root = tmp_path / "root" + root.mkdir() + inside = root / "a.txt" + inside.write_text("needle") + monkeypatch.setattr(grep_mod, "_ripgrep_available", lambda: True) + monkeypatch.setattr(grep_mod, "_MAX_RG_OUTPUT_BYTES", 200) + + def fake_popen(cmd, **kwargs): + out = kwargs["stdout"] + line = (json.dumps({"type": "match", "data": { + "path": {"text": str(inside)}, "line_number": 1, + "lines": {"text": "needle\n"}}}) + "\n").encode() + for _ in range(50): # well over the 200-byte cap + out.write(line) + return _FakeProc() + + monkeypatch.setattr(grep_mod.subprocess, "Popen", fake_popen) + r = grep(pattern="needle", path=str(root)) + assert r["truncated"] is True + + def test_glob_excludes_hidden_ancestor(tmp_path): """include_hidden=False must drop results with a hidden ANCESTOR, not just a hidden basename (e.g. .hidden/secret.txt via **/*)."""