diff --git a/CHANGELOG.md b/CHANGELOG.md index df10576..3d0a304 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Clever Semantic Versioning](https://www.w3.org/subm ### Added +- **Sandboxed `read_file`/`write_file` Helpers for Inline Code (issue #93)** (`agents/file_access.py`, `agents/tool.py`, `docs/index.md`): Exposes two injected local callables — `read_file` and `write_file` — inside inline-code tool bodies (§4.5.2) when the agent isn't running with `safe_mode` (or the invocation opts into `_unsafe_mode`), so inline code can read and write file contents dynamically (a path computed at runtime, several files chosen by the code, or a read-modify-write cycle) without the previous static `file_read → inline → file_write` wiring. + + `read_file(path, max_chars=None, offset=0)` returns the raw decoded file contents — honoring the same `offset`/`max_chars` character-windowing as the `file_read` tool (ADR-2033), and returning the content itself rather than the `[FILE_READ_SUCCESS]` envelope. `write_file(path, content, mode="w")` writes via the `file_write` modes (`w`/`a`/`insert`, §4.5.5) and returns the number of characters written. Both helpers confine every access to the sandbox root: a path beginning with `~`, containing a `..` traversal component, or whose resolved real path (symlinks followed) falls outside the sandbox root is rejected with a `ValueError` naming the offending path. `write_file` performs a write only when the invocation context contains `_unsafe_mode: true` (raising `ValueError` otherwise); `read_file` does not require it, preserving the read/write privilege split. When neither condition holds, neither name is bound, so referencing it raises `NameError`, and the §13.2.1 built-in table (no `open`, `os`, `io`, `pathlib`, or file descriptors) is unchanged; the §13.2.2 expression sandbox gains no file access. + + Internally, the `file_read`/`file_write` validation and I/O are extracted into a single shared core (`FileAccessCore`, `SandboxRootPolicy`) that backs both the built-in tools and the injected helpers — including the `w`/`a`/`insert` write mechanics themselves, not only their validation — so containment, windowing, and write-mode semantics have exactly one implementation. The shared containment resolves symlinks (`realpath`), closing a symlink-escape gap in the built-in tools as well. The injected callables are plain closures over the core, not bound methods, so inline code cannot reach the (intentionally unconfined-at-its-own-level) core through a `__self__` reference. + + See `docs/adr/ADR-2035-inline-code-sandboxed-file-access.md` for the full set of design decisions (sanctioned helpers vs. raw `open()`, raw-value returns, the containment boundary and `ValueError` model, and the mode gating), reflected in Actor Configuration Standard revision 1.2.0. + + **Module:** `src/cleveractors/agents/file_access.py` (`SandboxRootPolicy`, `FileAccessCore`, `InlineFileHelperFactory`, `coerce_read_window`), `src/cleveractors/agents/tool.py` (`ToolAgent._execute_python_code`, `ToolAgent._file_read_tool`, `ToolAgent._file_write_tool`). BDD: `features/inline_code_file_helpers.feature`, `features/sandbox_root_policy.feature`. Robot: `robot/inline_code_file_helpers.robot`. + - **Skill Package Schema and Agent-Side Skill Loading (issue #88)** (`agents/skills.py`, `agents/skill_schema.py`, `agents/skill_resolution.py`, `agents/factory.py`, `agents/llm.py`, `agents/tool.py`, `agents/llm_tools.py`, `runtime.py`, `runtime_dispatch.py`, `core/application.py`): Adds a normative Skill package schema (Package Registry Standard §16) aligned with the open [agentskills.io](https://agentskills.io/specification) Agent Skills format, and a new optional `skills` field on `type: llm` agent configs that resolves, validates, and loads Skill packages through the existing `cleveractors.registry` client. A `skill`-type package is a single YAML mapping with required `name`, `description`, and `instructions`, plus optional `license`, `compatibility`, `metadata`, `allowed_tools`, and `resources` (bundled files, UTF-8 or base64-encoded, validated as well-formed at load time) — see `docs/actor-registry-standard.md` §16 for the full schema. `SkillLoader` resolves each `skills` reference (via `registry:`, `ID:`, or `local:` schemes, mirroring the existing template package-reference resolution path in `cleveractors.templates.base._resolve_package_ref`), validates it with `SkillValidator` (rejecting duplicate skill names across a `skills` list) and returns an augmented config with a discovery catalogue appended to `system_prompt`, a synthesized `skill` activation tool appended to `tools` (recognizing an already-declared `skill` tool in either shorthand or normalized OpenAI function-calling form, so it is never duplicated), and the resolved catalogue under an internal `_loaded_skills` key. A skill's content-addressed `package_id` is identical regardless of which reference scheme resolved it, and any reference that cannot be resolved — including an `ID:` reference absent from the local store's identity map — fails with a clear registry error rather than a misleading validation error. Both `SkillLoader`/`SkillReferenceResolver` and `AgentFactory` (`acreate_agent`) expose matching sync and async resolution paths, so a `registry:` reference resolves correctly whether agent creation runs from plain sync code or from the async runtime dispatch layer; every reference in a `skills` list shares one `PackageContentResolver` per agent-creation call — built unconditionally regardless of whether a local store or registry API key is configured, and closed once that call completes, including when agent creation itself fails. `Skill.to_context_dict()` surfaces `license`, `compatibility`, `metadata`, and `allowed_tools` alongside `name`/`description`/`instructions`/`resources`; base64-encoded resources that are not valid UTF-8 (genuine binary assets) decode safely instead of raising. diff --git a/benchmarks/file_access_benchmark.py b/benchmarks/file_access_benchmark.py new file mode 100644 index 0000000..57e3fc5 --- /dev/null +++ b/benchmarks/file_access_benchmark.py @@ -0,0 +1,47 @@ +"""ASV benchmarks for the sandbox-confined file-access cores (ADR-2035). + +Measures the performance of the hot paths shared by the built-in +``file_read`` / ``file_write`` tools and the inline-code ``read_file`` / +``write_file`` helpers: + + - sandbox-root containment resolution (``SandboxRootPolicy.resolve``) + - windowed reads (``FileAccessCore.read_window``) at full-file and + offset/max_chars-bounded sizes +""" + +from __future__ import annotations + +import os +import shutil +import tempfile + +from cleveractors.agents.file_access import FileAccessCore, SandboxRootPolicy + + +class FileAccessBenchmark: + """Benchmark the containment and windowing hot paths.""" + + def setup(self) -> None: + """Create a real sandbox with a sizable file to read.""" + self._sandbox = tempfile.mkdtemp() + self._policy = SandboxRootPolicy(self._sandbox) + self._core = FileAccessCore(self._policy) + self._path = os.path.join(self._sandbox, "data.txt") + with open(self._path, "w", encoding="utf-8") as handle: + handle.write("0123456789" * 5000) + + def teardown(self) -> None: + """Remove the sandbox.""" + shutil.rmtree(self._sandbox, ignore_errors=True) + + def time_resolve_within_root(self) -> None: + """Measure a successful containment resolution.""" + self._policy.resolve("data.txt") + + def time_read_full_window(self) -> None: + """Measure a full-file windowed read.""" + self._core.read_window(self._path, None, 0) + + def time_read_bounded_window(self) -> None: + """Measure an offset/max_chars-bounded windowed read.""" + self._core.read_window(self._path, 8000, 4000) diff --git a/docs/adr/ADR-2035-inline-code-sandboxed-file-access.md b/docs/adr/ADR-2035-inline-code-sandboxed-file-access.md new file mode 100644 index 0000000..5ea4771 --- /dev/null +++ b/docs/adr/ADR-2035-inline-code-sandboxed-file-access.md @@ -0,0 +1,383 @@ +# ADR-2035: Sandboxed `read_file`/`write_file` Helpers for Inline Code — Specification Extension + +**Status:** Approved + +**Date:** 2026-08-04 + +**Author:** Luis Mendes (CoreRasurae) + +**Issue:** #93 — feat(agents): expose sandboxed file helpers to inline code + +--- + +## Context + +Inline-code tools (§4.5.2) — e.g. a `Python_exec`-style `type: tool` agent +whose tool definition carries a `code` body — execute in the restricted +inline-code sandbox built by `ToolAgent._execute_python_code` +(`cleveractors.agents.tool`, at commit 46bff0d). That sandbox exposes the +**Restricted Built-ins for Inline Code** set of §13.2.1 (type constructors, +iteration/aggregation utilities, `isinstance`/`type`, `locals`, `print`, the +`ValueError`/`IndexError`/`KeyError`/`TypeError` categories) plus a single +injected module, `json` (`safe_globals["json"] = __import__("json")`), and the +four local variables `input_data` / `message` / `context` / `result`. + +Crucially, that set contains **no filesystem access whatsoever**: `open()` is +not in the §13.2.1 built-in table, and §13.2.3 prohibits "performing arbitrary +I/O against the filesystem" *unconditionally* — in both safe and unsafe mode. +Today `_execute_python_code` even turns a `NameError` for `open` into a +diagnostic steering the author toward "the file_read and file_write tools for +file operations." That is conformant with the current standard, but it means an +inline-code body cannot read or write file contents dynamically from within a +single code body. The only sanctioned path is **static composition** — wiring a +built-in `file_read` tool (§4.5.1) into the inline tool as `input_data`, and a +separate `file_write` tool after it. Static composition cannot express dynamic +access (a path computed at runtime), multi-file access (reading N files chosen +by the code), or a read-modify-write cycle inside one body. + +Meanwhile the built-in tools already own exactly the validated I/O this gap +needs: + +- `ToolAgent._file_read_tool` (`cleveractors.agents.tool`) performs shell-command + detection, safe-mode `..`/absolute-path rejection, and — per ADR-2033 — the + `offset`/`max_chars` character-windowing that returns + `content[offset:window_end]`, wrapped in the `[FILE_READ_SUCCESS]` envelope + destined for an LLM. +- `ToolAgent._file_write_tool` (with helpers `_validate_file_write_args`, + `_validate_file_path_safety`, `_prepare_append_content`, + `_handle_insert_position`) performs the `_unsafe_mode`-gated, + containment-checked `w`/`a`/`insert` writes of §4.5.5, returning a + human-readable success sentence. + +This ADR decides **how** to close the inline-code gap. The central design +question is not *whether* inline code should reach the filesystem in unsafe mode +— the issue already scopes that to unsafe mode — but *through what surface*: +raw `open()`/`os`/`pathlib` (widening §13.2.1), or a narrow pair of sanctioned +callables layered over the already-validated tool cores. It also resolves the +subsidiary questions that follow from that choice: what the helpers return +(raw content vs. the LLM envelope), what containment boundary they enforce and +which exception type a violation raises, how the read/write privilege split is +preserved, and which specification sections must change in lockstep with the +code. + +--- + +## Decision + +### D-1: Sanctioned helper functions, not raw `open()` + +**What:** In unsafe mode the inline-code sandbox gains exactly **two** injected +local callables — `read_file` and `write_file` — and nothing else. `open`, the +`os` / `io` / `pathlib` modules, and raw file descriptors remain absent. The +§13.2.1 built-in table is **not** widened. + +**Why:** The capability inline code actually needs is "read this file's +contents" and "write these contents to that file" — not the full, unbounded +`open()` surface (arbitrary modes, file-descriptor manipulation, `os.*` +traversal of the whole filesystem). Injecting two purpose-built callables, in +the exact manner `json` is already injected (a name in the execution namespace, +not a new entry in `__builtins__`), grants precisely that capability and no +more. It keeps the normative §13.2.1 built-in table unchanged — so the +regression surface for "what builtins can inline code see" stays fixed — while +still giving authors dynamic, multi-file, read-modify-write access. It also +concentrates all filesystem policy in one auditable place (the shared cores of +D-2) rather than scattering it across whatever an author might do with a raw +`open()` handle. + +**Why injected locals, not builtins:** `read_file`/`write_file` are host-provided +facilities bound to host state (the sandbox root, the invocation context), just +like `json` is a host-provided module. Modelling them as injected locals — the +established §4.5.2(2)/(3) mechanism — means their presence is trivially +conditioned on mode (D-5) by simply not binding the names, which yields the +required `NameError` in safe mode for free. + +### D-2: Single validation surface — reuse the existing tool cores + +**What:** The validation-and-I/O logic currently embedded in `_file_read_tool` +and `_file_write_tool` is extracted into reusable **cores** that back *both* the +built-in tools *and* the injected helpers. There is exactly one implementation +of: path containment, `offset`/`max_chars` read windowing, and `w`/`a`/`insert` +write semantics. The built-in tools become thin adapters that call a core and +then format the `[FILE_READ_SUCCESS]` / "Successfully wrote…" strings; the +helpers call the *same* core and return its raw result (D-3). + +**Why:** The acceptance criteria require the helpers to honor "the same +semantics as the `file_read` tool" (offset/max_chars) and "the `file_write` +core (§4.5.5 modes)". If the helpers reimplemented that logic, the two surfaces +would inevitably drift — a containment fix or a mode fix applied to one and +forgotten on the other would open a security or correctness gap on the +untouched surface. A single validation surface makes "confined to the sandbox +root" and "preserve the read/write privilege split" provable properties of one +code path rather than an invariant that must be manually re-checked on two. +This is the Facade/shared-core relationship already implicit in the codebase +(the tools *are* the only file I/O today); the extraction just makes the core +independently callable. + +### D-3: Signatures and return values — raw content and counts, not LLM envelopes + +**What:** The injected callables have these exact signatures and returns: + +- `read_file(path, max_chars=None, offset=0) -> str` — returns the **raw decoded + file contents** for the requested window, reusing the ADR-2033 + `offset`/`max_chars` character-windowing (`content[offset:window_end]`, + `window_end = char_count if max_chars is None else min(char_count, offset + + max_chars)`). It returns the *content itself*, **not** the + `[FILE_READ_SUCCESS]…[FILE_CONTENT_START]…[FILE_CONTENT_END]` envelope, and + emits no `TRUNCATED` / `MORE_CONTENT_AT_OFFSET` / `NO_MORE_CONTENT_AT_OFFSET` + markers. +- `write_file(path, content, mode="w") -> int` — writes via the `file_write` + core honoring the §4.5.5 `w` / `a` / `insert` modes, and returns the + **number of characters written** (`len(content)`) as an `int`, **not** the + "Successfully wrote/appended/inserted …" sentence. + +**Why return raw values, not the tool strings:** The `[FILE_READ_SUCCESS]` +envelope and the "Successfully wrote…" sentence exist to communicate *to an +LLM* — they are affordances of the tool-calling protocol (§4.5.7), carrying +continuation hints and file metadata a model needs to decide its next call. +An inline-code body is a *program*, not a model: it wants the bytes it asked +for and an integer it can compare, and it would otherwise have to *parse the +markers back out* of the envelope to use the content — re-introducing exactly +the brittle header-parsing ADR-2033's Consequences warned against. Returning +`str` content and an `int` count matches how `json.loads`/`json.dumps` (the +sandbox's only other injected facility) behave: they return Python values, not +narration. Pagination is still fully expressible — the caller drives it by +passing `offset`/`max_chars` and observing `len()` of the returned window — +without needing the header markers. + +**Why `insert` mode has no `position` parameter in the helper signature:** The +signature the issue fixes is `write_file(path, content, mode="w")`. `insert` +without an explicit position defaults to `"end"` per §4.5.5 (equivalent to +append at the end). Exposing the full `position` matrix through the helper is +deferred as a non-goal of this issue; the core still supports it for the tool. + +### D-4: Sandbox-root containment — realpath resolution, `ValueError` on escape + +**What:** Both helpers confine all access to the **sandbox root**, defined as +the resolved real path of the sandbox's working directory +(`os.path.realpath(os.getcwd())` under the current sandbox strategy — the same +process-CWD boundary `_validate_file_path_safety` already treats as the root). +A path is admitted **iff** its fully resolved real path — with `..` collapsed +and **symlinks followed** — is equal to the sandbox root or a descendant of it. +The following are rejected, each raising **`ValueError`** whose message names +the offending path: + +- any path containing a `..` traversal component; +- any path beginning with `~` (home-directory expansion); +- any absolute or relative path whose resolved real path falls outside the + sandbox root — **including a symlink that itself lives inside the root but + points outside it** (symlink-escape). + +**Why `realpath` (symlinks followed), strengthening the existing check:** The +current `_validate_file_path_safety` computes `os.path.abspath` and compares +`startswith(working_dir + os.sep)`. `abspath` normalises `..` but does **not** +resolve symlinks, so a symlink placed inside the root pointing outside it passes +the `startswith` test yet reads/writes outside the root. The acceptance criteria +explicitly demand rejecting "any path whose resolved real path (symlinks +followed) falls outside the sandbox root," so the shared containment core +resolves via `realpath` before the boundary comparison. Because the core is +shared (D-2), this closes the symlink-escape gap for the built-in tools too — a +strict strengthening, never a relaxation, of today's boundary. + +**Why `ValueError`, not `ExecutionError`:** A containment violation surfaces +*inside the running inline-code body*, and that body can only catch exception +categories the §13.2.1 table exposes to it — `ValueError` is one; the host's +`ExecutionError` is not. Raising `ValueError` lets an inline author write +`try: read_file(p) except ValueError: …` using the sandbox's own vocabulary, +whereas an `ExecutionError` would be an opaque, uncatchable escape from the +sandbox's error model. The built-in-tool adapters continue to translate core +failures into `ExecutionError` for the tool-calling protocol; only the +helper-facing surface raises `ValueError`. + +### D-5: Mode gating — injection in unsafe mode, `write_file` also gated on `_unsafe_mode` + +**What:** + +- The helpers are injected into the inline namespace when the agent's + `safe_mode` (§4.5) is `false`, or the invocation context carries + `_unsafe_mode: true` — the same two conditions §4.5.4 already uses to relax + `file_read`/`file_write`'s own restrictions. When neither holds, the names + are simply never bound, so referencing `read_file` or `write_file` raises + `NameError`, exactly as any other undefined name would. +- `write_file` performs a write **only when the invocation context contains + `_unsafe_mode: true`**; absent that, it raises `ValueError` (per D-4's + helper-facing error model) and performs no write. This mirrors the existing + `_file_write_tool` gate (which raises when `_unsafe_mode` is absent), and + preserves the §4.5.4 / §13.3 rule that *writes always require `_unsafe_mode` + regardless of host mode*. +- `read_file` does **not** require `_unsafe_mode`. Within an already-unsafe host + (the only place the helper exists at all), reads are permitted subject only to + the D-4 containment boundary — preserving the existing read/write privilege + split, where reads are less restricted than writes. +- The §13.2.2 **expression** sandbox (`transform.fn`, and the bridge + `custom_predicate` / `state_extractor` / `state_flattener` fields) is + **unchanged** and gains no file access. `read_file`/`write_file` are injected + only into the §13.2.1 inline-code namespace, never into expression + evaluation. + +**Why two gates for write, one for read:** There are two orthogonal switches +already established by §4.5/§4.5.4 for the pre-existing tools: the agent's own +`safe_mode` configuration and the per-invocation `_unsafe_mode` context flag; +either being in its relaxed state is enough for the helpers to exist at all. +The context flag is *additionally* layered on top as the per-invocation opt-in +that governs *writes specifically*. Collapsing the two into one would either +let a write happen without the explicit per-invocation opt-in (unsafe) or +forbid reads that either knob has already sanctioned (needlessly restrictive). +Keeping `read_file` off the `_unsafe_mode` gate reproduces the exact asymmetry +the tools already implement: `_file_read_tool` never checks `_unsafe_mode` for +a relative in-root read, while `_file_write_tool` refuses without it. + +**Relationship to §13.1's host-level unsafe mode:** §13.1 describes a coarser, +host-level switch (typically a CLI/config flag) that controls whether the host +injects `_unsafe_mode` into invocation contexts in the first place; it is not +itself threaded into an agent's `safe_mode` field automatically. The gate above +reuses the same per-agent/per-invocation signals the pre-existing `file_read`/ +`file_write` tools already key off — it does not introduce a new, independent +host-mode check. + +### D-6: Specification amendments applied in the same change + +**What:** This ADR mandates the following amendments to `docs/index.md`, applied +in the **same commit** as the implementing code (per the project's +same-commit documentation rule). This is the amendment set the issue's +acceptance criteria refer to as "ADR-2035 D-6": + +1. **§13.2.3 (Prohibited Capabilities)** — reworded so the blanket prohibition + on filesystem I/O admits a single, explicitly-scoped exception: the sanctioned + `read_file`/`write_file` helpers injected into the *inline-code* sandbox + (§13.2.1) in unsafe mode. The prohibition remains absolute for the §13.2.2 + expression sandbox and for every other form of I/O (network beyond + `http_request`, subprocess, dynamic import/eval, host introspection). +2. **§4.5.2 (Inline Tools)** — the list of injected local facilities is extended + to document `read_file`/`write_file` alongside `json`, with their signatures, + return values, containment behavior, and mode gating. +3. **§13.2.1 (Restricted Built-ins for Inline Code)** — a note clarifying that + the built-in *table itself is unchanged* (no `open`, no `os`/`io`/`pathlib`, + no file descriptors) and that filesystem access is available *only* through + the two injected helpers, not through any built-in. +4. **§13.3 (Filesystem Boundaries)** — a cross-reference recording that the + helpers are bound by the same boundary as the tools (`..`/`~` rejection, + sandbox-root containment) and that `write_file` requires `_unsafe_mode` just + as `file_write` does. + +**Why in the same change:** These are not documentation of a future intention; +they are the normative statement of the behavior this commit introduces. Under +the project's continuous-documentation rule, spec and code that describe the +same behavior ship together, so the spec never describes a state the code has +not yet reached (or vice versa). Per the standard ADR process, these amendments +are applied to `docs/index.md` only **after** this ADR is accepted. + +--- + +## Consequences + +### Positive + +- **Closes the dynamic/multi-file gap:** inline code can now compute a path at + runtime, read several files chosen by the code, and perform read-modify-write + cycles inside one body — none of which static `file_read → inline → file_write` + composition can express. +- **No new built-in surface:** the §13.2.1 table is untouched; the only new + capability is two narrowly-scoped, host-controlled callables, so the + "what can inline code do" audit surface grows by exactly two well-defined + functions rather than the entire `open()`/`os` API. +- **One validation surface:** containment, windowing, and write-mode semantics + live in a single core (D-2); a fix to either applies to both the tools and the + helpers automatically. +- **Stronger containment everywhere:** the realpath-based boundary (D-4) closes + a symlink-escape gap that the previous `abspath`-only check left open — for the + built-in tools as well, since the core is shared. +- **Programmatic ergonomics:** helpers return `str` content and an `int` count, + matching `json`'s value-returning model, so inline authors never parse the + LLM-facing envelope markers back out. +- **Privilege split preserved:** reads stay less restricted than writes; writes + still demand the explicit `_unsafe_mode` per-invocation opt-in. + +### Negative / Risks + +- **A sanctioned filesystem exception now exists in §13.2.3.** The prohibition + is no longer categorical for the inline-code sandbox; it is now "no filesystem + I/O *except* the two named helpers, in unsafe mode, within the sandbox root." + This is a deliberate, bounded widening of capability, mitigated by the + containment boundary (D-4) and the double gate on writes (D-5), but it is a + larger attack surface than "no filesystem access at all." +- **Containment correctness now rests on `realpath` behavior.** Symlink + resolution and root-descendant comparison must be exactly right; a bug here is + a sandbox-escape, so it must be covered by explicit escape-attempt tests + (`..`, absolute-out-of-root, `~`, and symlink-out-of-root). +- **Full-file read cost inherited (ADR-2033 D-6):** `read_file` reads the whole + file before slicing the window, so paginating a very large file re-reads it on + each call — an accepted, documented trade-off carried over unchanged, not + introduced here. +- **Helper `insert` positional coverage is partial:** `write_file`'s signature + omits `position`, so `insert` via the helper defaults to end-insertion; full + positional insert remains available only through the built-in tool. + +### Follow-up Required + +- **Expose `position` for `insert` through the helper** — only if a concrete use + case needs mid-file insertion from inline code; deferred as a non-goal here. +- **Configurable sandbox root** — this ADR fixes the root at the process CWD (the + boundary the code already uses). A future sandbox strategy that relocates the + root (e.g. a git-worktree or copy-on-write sandbox) should thread an explicit + root through the shared core rather than reading `os.getcwd()` directly. + +--- + +## Alternatives Considered + +### A-1: Add `open()` (and/or `os`/`io`/`pathlib`) to the §13.2.1 built-ins + +**Rejected because:** this grants far more than the "read a file / write a file" +capability the issue calls for — arbitrary modes, raw file descriptors, and the +whole `os` traversal surface — and it disperses filesystem policy across +whatever an author does with a raw handle instead of concentrating it in one +validated core (D-2). It would also mutate the normative §13.2.1 table, enlarging +the regression surface the acceptance criteria explicitly want frozen ("the +§13.2.1 built-in table is unchanged"). Two sanctioned callables give the needed +capability with a fraction of the surface. + +### A-2: Have the helpers return the `[FILE_READ_SUCCESS]` envelope / success sentence + +**Rejected because:** those strings are LLM-facing protocol artifacts (§4.5.7), +carrying continuation hints a *program* does not want. An inline body would have +to parse the markers back out to obtain the content — re-introducing the exact +brittle header-parsing ADR-2033 warned about. Returning raw `str`/`int` (D-3) +matches the `json` facility's value-returning model and makes pagination +expressible via `offset`/`max_chars` + `len()` without any marker parsing. + +### A-3: Raise `ExecutionError` (as the tools do) on containment/`_unsafe_mode` violations + +**Rejected because:** `ExecutionError` is a host type not present in the §13.2.1 +error-category table, so inline code cannot catch it — a violation would be an +opaque, uncatchable escape from the sandbox's own error model. `ValueError` +(D-4) is in that table, letting authors handle a rejected path with the +sandbox's native vocabulary. The tool adapters keep translating core failures to +`ExecutionError` for the protocol; only the helper-facing surface raises +`ValueError`. + +### A-4: Gate `read_file` on `_unsafe_mode` as well (symmetric with `write_file`) + +**Rejected because:** it would break the read/write privilege split the tools +already implement — `_file_read_tool` permits an in-root relative read without +`_unsafe_mode`, while `_file_write_tool` demands it. Reads are only reachable at +all inside an already-unsafe host; requiring a second opt-in for them would be +strictly more restrictive than the equivalent tool, for no security gain the +containment boundary (D-4) does not already provide. + +### A-5: Reimplement read/write logic directly in the helpers (no shared core) + +**Rejected because:** it creates two independent copies of containment, +windowing, and write-mode logic that will drift — a containment fix applied to +the tool and forgotten on the helper (or vice versa) silently opens a gap on the +untouched surface. Extracting one shared core (D-2) makes "confined to the +sandbox root" and "preserves the read/write split" properties of a single code +path, provable once rather than audited twice. + +### A-6: A copy-on-write / snapshot filesystem for inline code instead of a live-path boundary + +**Rejected because:** it solves a different problem (isolating *mutations* for +rollback) at far greater implementation cost, and still needs a root boundary to +decide what the snapshot covers. The issue asks for confined live access reusing +the existing validated cores, not a new sandbox filesystem strategy; a snapshot +layer, if ever wanted, is an orthogonal future ADR (see the configurable-root +follow-up). diff --git a/docs/index.md b/docs/index.md index 6060113..d47b791 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1,6 +1,6 @@ # The Actor Configuration Standard -**Version:** 1.1.0 +**Version:** 1.2.0 **Status:** Normative --- @@ -517,12 +517,17 @@ When `code` is provided, the tool executes the given code body in a sandboxed en 1. Restrict the available built-in functions and types to the **Restricted Built-ins for Inline Code** set defined in §13.2.1. 2. Expose a JSON parsing and serialization facility under the name `json`, providing at minimum the operations `loads` (parse a JSON string), `dumps` (serialize a value to JSON), and an error category `JSONDecodeError` signaled when parsing fails. -3. Provide the following local variables to the tool body: +3. When the agent's `safe_mode` (§4.5) is `false`, or the invocation context contains `_unsafe_mode: true`, expose exactly two additional injected local callables providing sandbox-confined filesystem access (§13.2.3, §13.3) — the same two conditions §4.5.4 already uses to relax `file_read`/`file_write`'s own restrictions: + - `read_file(path, max_chars=None, offset=0)` — returns the raw decoded file contents as a string, applying the same `offset`/`max_chars` character-windowing as the `file_read` tool (§4.5.1). It returns the content itself, not the `[FILE_READ_SUCCESS]` envelope, and does not require `_unsafe_mode`. + - `write_file(path, content, mode="w")` — writes `content` using the `file_write` modes (§4.5.5) and returns the number of characters written as an integer. It MUST perform a write only when the invocation context contains `_unsafe_mode: true`. + + Both callables MUST confine access to the sandbox root: a `path` beginning with `~`, containing a `..` traversal component, or whose resolved real path (symlinks followed) falls outside the sandbox root MUST be rejected with a `ValueError` naming the path (a `write_file` call without `_unsafe_mode` MUST likewise raise `ValueError`). When neither condition above holds, neither name is bound, so referencing it raises `NameError`. These helpers are exposed only to inline-code bodies, never to the expression sandbox (§13.2.2). +4. Provide the following local variables to the tool body: - `input_data`: the raw input message (string). - `message`: an alias for `input_data`. - `context`: the active context dictionary (passed by reference; modifications propagate back to the caller). - `result`: initially `null`; the tool body sets this to its return value. -4. When `result` is `null` after the tool body completes, fall back to one of (in order): `output`, `response`, `answer`, defaulting to the empty string if none are set. +5. When `result` is `null` after the tool body completes, fall back to one of (in order): `output`, `response`, `answer`, defaulting to the empty string if none are set. Inline-code tools MUST be enabled only when the host is operating in unsafe mode (see §13). @@ -2400,7 +2405,7 @@ When a compliant implementation does honor these fields, `verbose: true` SHOULD ### 9.6 `version` -Implementations MAY accept a `version` parameter. This field is informational only and MAY be used to select compatibility behaviors. The current version of this standard is `"1.1.0"`. +Implementations MAY accept a `version` parameter. This field is informational only and MAY be used to select compatibility behaviors. The current version of this standard is `"1.2.0"`. --- @@ -2729,6 +2734,8 @@ When an inline-code tool body is executed, the following built-in facilities MUS | Error categories | `ValueError`, `IndexError`, `KeyError`, `TypeError` | | Module access | A facility exposing only the JSON parsing/serialization module under the name `json` | +This built-in table is exhaustive and unchanged by the file-access helpers: `open`, the `os` / `io` / `pathlib` modules, and raw file descriptors remain absent. Filesystem access from inline code is available only through the injected `read_file` / `write_file` callables of §4.5.2 (unsafe mode only), never through a built-in. + #### 13.2.2 Restricted Built-ins for Expression Evaluation When an inline **expression** (such as the `transform.fn` parameter or the bridge `custom_predicate`/`state_extractor`/`state_flattener` fields) is evaluated, only the following minimal set of built-in facilities MUST be available: @@ -2742,13 +2749,13 @@ When an inline **expression** (such as the `transform.fn` parameter or the bridg Neither sandbox MAY expose any of the following: -- Performing arbitrary I/O against the filesystem, network, or system. +- Performing arbitrary I/O against the filesystem, network, or system. The sole sanctioned exception is the inline-code `read_file` / `write_file` helpers of §4.5.2: in unsafe mode the inline-code sandbox (§13.2.1) MAY expose those two sandbox-root-confined callables and nothing wider. This exception does not extend to the expression sandbox (§13.2.2), which MUST expose no filesystem access. - Dynamic evaluation or compilation of arbitrary source code. - Introspection of the host or surrounding execution environment. - Dynamic import of modules other than those explicitly listed. - Subprocess spawning or signaling. -Any attempt to expose these capabilities MUST be refused at configuration load time or at invocation time. +Any attempt to expose these capabilities, beyond the single sanctioned filesystem exception above, MUST be refused at configuration load time or at invocation time. ### 13.3 Filesystem Boundaries @@ -2762,6 +2769,8 @@ In unsafe mode (the host is in unsafe mode AND `_unsafe_mode: true` is in the in - `file_read` permits absolute paths. - `file_write` is enabled but still refuses `..` traversal and home-directory expansion (`~`). +The inline-code `read_file` / `write_file` helpers (§4.5.2) are bound by this same boundary: they reject `..` traversal and `~` expansion and confine every access to the sandbox root (resolved real path, symlinks followed). As with `file_write`, `write_file` performs a write only when the invocation context contains `_unsafe_mode: true`; `read_file`, like `file_read`, does not require it. + ### 13.4 Network Boundaries The `http_request` tool MAY be invoked in either mode. Outbound network access SHOULD be restricted according to host policy; that policy is outside the scope of this standard. @@ -3962,7 +3971,7 @@ Compliant implementations SHOULD signal the following error categories with the ## 21. Versioning of This Standard -This document is **Version 1.1.0** of the Actor Configuration Standard. Future revisions: +This document is **Version 1.2.0** of the Actor Configuration Standard. Future revisions: - A **patch** version (1.0.x) corrects errors and clarifies semantics without changing required behavior. - A **minor** version (1.x.0) adds new optional features without breaking conformance of existing implementations. @@ -3976,3 +3985,4 @@ Conformance MUST be declared against a specific version of this standard. Implem |---------|--------| | 1.0.0 | Initial version. | | 1.1.0 | §5.4: `content_not_contains` Honored By extended from `Stream router, conditional nodes, bridge router` to `All subsystems`, adding pure-graph edges. Minor revision — restores the symmetric contains/not-contains treatment `content_contains` already had, without breaking conformance of configurations that do not use `content_not_contains` on pure-graph edges. See `docs/adr/ADR-2034-content-not-contains-all-subsystems.md`. | +| 1.2.0 | §4.5.2, §13.2.1, §13.2.3, §13.3: added the sanctioned inline-code `read_file`/`write_file` helpers, injected into the inline-code sandbox in unsafe mode and confined to the sandbox root. Minor revision — adds an optional capability without changing the §13.2.1 built-in table or breaking conformance of configurations that do not use the helpers. See `docs/adr/ADR-2035-inline-code-sandboxed-file-access.md`. | diff --git a/features/inline_code_file_helpers.feature b/features/inline_code_file_helpers.feature new file mode 100644 index 0000000..3b7905d --- /dev/null +++ b/features/inline_code_file_helpers.feature @@ -0,0 +1,126 @@ +Feature: Inline-code sandboxed file helpers (read_file / write_file) + Inline-code tool bodies gain two injected helpers, read_file and write_file, + when the host is in unsafe mode (ADR-2035). They reuse the file_read/file_write + validated cores, return raw values rather than the LLM envelope, and confine + every access to the sandbox root. + + Scenario: The helpers are absent in safe mode + Given an inline-code tool agent in safe mode + When the inline code tries to read "notes.txt" + Then the tool execution fails because read_file is not available + + Scenario: write_file writes content and read_file reads it back in unsafe mode + Given an inline-code tool agent in unsafe mode granting write access + When the inline code writes "hello from inline" to "notes.txt" and reads it back + Then the tool result should be "hello from inline" + + Scenario: write_file returns the number of characters written + Given an inline-code tool agent in unsafe mode granting write access + When the inline code writes "abcde" to "count.txt" and returns the write count + Then the tool result should be "5" + + Scenario: read_file honors offset and max_chars windowing + Given a sandbox file "data.txt" containing "0123456789" + And an inline-code tool agent in unsafe mode granting write access + When the inline code reads "data.txt" with offset 2 and max_chars 3 + Then the tool result should be "234" + + Scenario: read_file reading at or beyond end of file yields empty content + Given a sandbox file "short.txt" containing "abc" + And an inline-code tool agent in unsafe mode granting write access + When the inline code reads "short.txt" with offset 10 and max_chars 5 + Then the tool result should be empty + + Scenario: read_file with a negative max_chars matches the file_read tool's windowing exactly + # Both surfaces share one windowing implementation (FileAccessCore.read_window, + # ADR-2035 D-2), so a negative max_chars produces the same backward Python + # slice (content[offset:offset+max_chars]) on both — this locks in that + # parity rather than changing the pre-existing ADR-2033 max_chars coercion. + Given a sandbox file "data.txt" containing "0123456789" + And an inline-code tool agent in unsafe mode granting write access + When the inline code reads "data.txt" with offset 0 and max_chars -3 + Then the tool result should be "0123456" + + Scenario: write_file appends a Markdown section with separating spacing + Given a sandbox file "doc.txt" containing "intro" + And an inline-code tool agent in unsafe mode granting write access + When the inline code appends "# Section" to "doc.txt" and reads it back + Then the tool result should be "intro\n\n# Section" + + Scenario: write_file inserts content and read_file confirms it + Given a sandbox file "list.txt" containing "a\nb\n" + And an inline-code tool agent in unsafe mode granting write access + When the inline code inserts "z" into "list.txt" and reads it back + Then the tool result should be "a\nb\nz\n" + + Scenario: read_file does not require write access + Given a sandbox file "readable.txt" containing "visible" + And an inline-code tool agent in unsafe mode without write access + When the inline code tries to read "readable.txt" + Then the tool result should be "visible" + + Scenario: write_file refuses to write without _unsafe_mode in the context + Given an inline-code tool agent in unsafe mode without write access + When the inline code tries to write "blocked" to "denied.txt" + Then the caught error should mention "unsafe mode" + And no file "denied.txt" should exist in the sandbox + + Scenario: write_file rejects a non-string content value + Given an inline-code tool agent in unsafe mode granting write access + When the inline code tries to write a non-string value to "bad.txt" + Then the caught error should mention "content must be a string" + + Scenario: write_file rejects an invalid write mode + Given an inline-code tool agent in unsafe mode granting write access + When the inline code performs an invalid-mode write of "x" to "modes.txt" with mode "bad" + Then the caught error should mention "Invalid mode" + + Scenario: read_file rejects a negative offset + Given a sandbox file "data.txt" containing "0123456789" + And an inline-code tool agent in unsafe mode granting write access + When the inline code reads "data.txt" with a negative offset + Then the caught error should mention "offset must be non-negative" + + Scenario: read_file rejects a non-integer max_chars + Given an inline-code tool agent in unsafe mode granting write access + When the inline code reads a file with a non-integer max_chars + Then the caught error should mention "max_chars must be an integer" + + Scenario: read_file rejects a non-integer offset + Given an inline-code tool agent in unsafe mode granting write access + When the inline code reads a file with a non-integer offset + Then the caught error should mention "offset must be an integer" + + Scenario: read_file rejects an empty path + Given an inline-code tool agent in unsafe mode granting write access + When the inline code reads an empty path + Then the caught error should mention "non-empty string" + + Scenario Outline: The helpers reject paths that escape the sandbox root + Given an inline-code tool agent in unsafe mode granting write access + When the inline code tries to read "" + Then the caught error should mention "" + + Examples: + | path | + | ../escape.txt | + | ~/secret.txt | + | /etc/hostname | + + Scenario: The helpers reject a symlink that escapes the sandbox root + Given an inline-code tool agent in unsafe mode granting write access + And a symlink "link.txt" in the sandbox pointing outside the sandbox root + When the inline code tries to read "link.txt" + Then the caught error should mention "link.txt" + + Scenario: write_file confines writes to the sandbox root + Given an inline-code tool agent in unsafe mode granting write access + When the inline code tries to write "x" to "../escape.txt" + Then the caught error should mention "../escape.txt" + + Scenario: write_file refuses to write through a symlink escaping the sandbox root + Given an inline-code tool agent in unsafe mode granting write access + And a symlink "wlink.txt" in the sandbox pointing outside the sandbox root + When the inline code tries to write "leak" to "wlink.txt" + Then the caught error should mention "wlink.txt" + And the escaped symlink target should still contain "outside the root" diff --git a/features/sandbox_root_policy.feature b/features/sandbox_root_policy.feature new file mode 100644 index 0000000..52291b0 --- /dev/null +++ b/features/sandbox_root_policy.feature @@ -0,0 +1,52 @@ +Feature: Sandbox-root containment policy (security) + The SandboxRootPolicy (ADR-2035 D-4) is the single admission rule guarding + the inline-code read_file/write_file helpers. These scenarios exercise it + directly, with crafted roots, to prove that no path outside the sandbox root + can be reached and that legitimate in-root paths are still admitted. + + Scenario: A relative path inside the root is admitted + Given a sandbox root policy + When I resolve the relative path "notes.txt" + Then resolution is admitted and stays inside the root + + Scenario: A nested relative path inside the root is admitted + Given a sandbox root policy + When I resolve the relative path "sub/dir/notes.txt" + Then resolution is admitted and stays inside the root + + Scenario: An absolute path inside the root is admitted + Given a sandbox root policy + When I resolve an absolute path that is inside the root + Then resolution is admitted and stays inside the root + + Scenario: A sibling directory sharing the root's name prefix is rejected + Given a sandbox root policy with a sibling directory sharing its name prefix + When I resolve an absolute path into the sibling directory + Then resolution is rejected as outside the root + + Scenario: A symlink inside the root that targets outside is rejected + Given a sandbox root policy + And a symlink "escape" inside the root targeting a path outside it + When I resolve the relative path "escape" + Then resolution is rejected as outside the root + + Scenario: A symlink inside the root that targets inside is admitted + Given a sandbox root policy + And a symlink "loop" inside the root targeting another path inside it + When I resolve the relative path "loop" + Then resolution is admitted and stays inside the root + + Scenario Outline: Paths that attempt to escape the root are rejected + Given a sandbox root policy + When I resolve the raw path "" + Then resolution is rejected + + Examples: + | path | + | ../escape.txt | + | sub/../../escape | + | .. | + | ~/secret | + | ~root/secret | + | ..\\escape.txt | + | /etc/hostname | diff --git a/features/steps/inline_code_file_helpers_steps.py b/features/steps/inline_code_file_helpers_steps.py new file mode 100644 index 0000000..e91efc8 --- /dev/null +++ b/features/steps/inline_code_file_helpers_steps.py @@ -0,0 +1,274 @@ +"""Step implementations for the inline-code read_file/write_file helpers (ADR-2035).""" + +import os +import tempfile +from pathlib import Path + +from behave import given, then, when +from features.steps._coverage_utils import _run + +from cleveractors.agents.tool import ToolAgent +from cleveractors.core.exceptions import ExecutionError + + +def _unescape(text: str) -> str: + """Interpret literal ``\\n`` / ``\\t`` in a Gherkin scalar as real whitespace.""" + return text.replace("\\n", "\n").replace("\\t", "\t") + + +def _ensure_sandbox(context): + """Create the per-scenario sandbox directory once, resolving symlinks.""" + if getattr(context, "sandbox", None) is None: + context.sandbox = Path(tempfile.mkdtemp()).resolve() + return context.sandbox + + +def _exec_inline(context, code: str) -> None: + """Run *code* as the sole inline tool of an agent, rooted at the sandbox. + + The process working directory is switched to the sandbox for the duration + so the helpers' sandbox root (the CWD) is the sandbox, then restored. + """ + _ensure_sandbox(context) + agent = ToolAgent( + "inline", + {"tools": [{"name": "t", "code": code}], "safe_mode": context.safe_mode}, + ) + old_cwd = os.getcwd() + os.chdir(context.sandbox) + try: + context.result = _run(agent.process_message("", dict(context.helper_ctx))) + context.error = None + except ExecutionError as exc: + context.result = None + context.error = str(exc) + finally: + os.chdir(old_cwd) + + +# ── Given ──────────────────────────────────────────────────────────── + + +@given("an inline-code tool agent in safe mode") +def step_agent_safe(context): + context.safe_mode = True + context.helper_ctx = {} + + +@given("an inline-code tool agent in unsafe mode granting write access") +def step_agent_unsafe_write(context): + context.safe_mode = False + context.helper_ctx = {"_unsafe_mode": True} + + +@given("an inline-code tool agent in unsafe mode without write access") +def step_agent_unsafe_no_write(context): + context.safe_mode = False + context.helper_ctx = {} + + +@given('a sandbox file "{name}" containing "{content}"') +def step_sandbox_file(context, name, content): + sandbox = _ensure_sandbox(context) + (sandbox / name).write_text(_unescape(content), encoding="utf-8") + + +@given('a symlink "{name}" in the sandbox pointing outside the sandbox root') +def step_sandbox_symlink(context, name): + sandbox = _ensure_sandbox(context) + outside = Path(tempfile.mkdtemp()).resolve() + target = outside / "secret.txt" + target.write_text("outside the root", encoding="utf-8") + os.symlink(target, sandbox / name) + context.symlink_target = target + + +# ── When ───────────────────────────────────────────────────────────── + + +@when('the inline code tries to read "{name}"') +def step_try_read(context, name): + _exec_inline( + context, + f"try:\n" + f" result = read_file({name!r})\n" + f"except ValueError as e:\n" + f' result = "ERR:" + str(e)\n', + ) + + +@when('the inline code writes "{content}" to "{name}" and reads it back') +def step_write_read(context, content, name): + _exec_inline( + context, + f"write_file({name!r}, {content!r})\nresult = read_file({name!r})\n", + ) + + +@when('the inline code writes "{content}" to "{name}" and returns the write count') +def step_write_count(context, content, name): + _exec_inline( + context, + f"n = write_file({name!r}, {content!r})\nresult = str(n)\n", + ) + + +@when( + 'the inline code reads "{name}" with offset {offset:d} and max_chars {max_chars:d}' +) +def step_read_window(context, name, offset, max_chars): + _exec_inline( + context, + f"try:\n" + f" result = read_file({name!r}, {max_chars}, {offset})\n" + f"except ValueError as e:\n" + f' result = "ERR:" + str(e)\n', + ) + + +@when('the inline code appends "{content}" to "{name}" and reads it back') +def step_append_read(context, content, name): + _exec_inline( + context, + f'write_file({name!r}, {content!r}, "a")\nresult = read_file({name!r})\n', + ) + + +@when('the inline code inserts "{content}" into "{name}" and reads it back') +def step_insert_read(context, content, name): + _exec_inline( + context, + f'write_file({name!r}, {content!r}, "insert")\nresult = read_file({name!r})\n', + ) + + +@when('the inline code tries to write "{content}" to "{name}"') +def step_try_write(context, content, name): + _exec_inline( + context, + f"try:\n" + f" write_file({name!r}, {content!r})\n" + f' result = "WROTE"\n' + f"except ValueError as e:\n" + f' result = "ERR:" + str(e)\n', + ) + + +@when('the inline code tries to write a non-string value to "{name}"') +def step_try_write_nonstr(context, name): + _exec_inline( + context, + f"try:\n" + f" write_file({name!r}, 123)\n" + f' result = "WROTE"\n' + f"except ValueError as e:\n" + f' result = "ERR:" + str(e)\n', + ) + + +@when( + 'the inline code performs an invalid-mode write of "{content}" to "{name}" ' + 'with mode "{mode}"' +) +def step_try_write_mode(context, content, name, mode): + _exec_inline( + context, + f"try:\n" + f" write_file({name!r}, {content!r}, {mode!r})\n" + f' result = "WROTE"\n' + f"except ValueError as e:\n" + f' result = "ERR:" + str(e)\n', + ) + + +@when('the inline code reads "{name}" with a negative offset') +def step_read_negative(context, name): + _exec_inline( + context, + f"try:\n" + f" result = read_file({name!r}, None, -5)\n" + f"except ValueError as e:\n" + f' result = "ERR:" + str(e)\n', + ) + + +@when("the inline code reads a file with a non-integer max_chars") +def step_read_bad_maxchars(context): + _exec_inline( + context, + "try:\n" + ' result = read_file("data.txt", "x")\n' + "except ValueError as e:\n" + ' result = "ERR:" + str(e)\n', + ) + + +@when("the inline code reads a file with a non-integer offset") +def step_read_bad_offset(context): + _exec_inline( + context, + "try:\n" + ' result = read_file("data.txt", None, "y")\n' + "except ValueError as e:\n" + ' result = "ERR:" + str(e)\n', + ) + + +@when("the inline code reads an empty path") +def step_read_empty_path(context): + _exec_inline( + context, + "try:\n" + ' result = read_file("")\n' + "except ValueError as e:\n" + ' result = "ERR:" + str(e)\n', + ) + + +# ── Then ───────────────────────────────────────────────────────────── + + +@then('the tool result should be "{expected}"') +def step_result_equals(context, expected): + assert context.error is None, f"unexpected error: {context.error}" + assert context.result == _unescape(expected), ( + f"expected {_unescape(expected)!r}, got {context.result!r}" + ) + + +@then("the tool result should be empty") +def step_result_empty(context): + assert context.error is None, f"unexpected error: {context.error}" + assert context.result == "", f"expected empty result, got {context.result!r}" + + +@then("the tool execution fails because read_file is not available") +def step_read_file_absent(context): + assert context.error is not None, "expected a NameError-driven failure" + assert "read_file" in context.error and "not available" in context.error, ( + f"unexpected error message: {context.error}" + ) + + +@then('the caught error should mention "{fragment}"') +def step_caught_mentions(context, fragment): + assert context.error is None, f"unexpected hard error: {context.error}" + assert isinstance(context.result, str) and context.result.startswith("ERR:"), ( + f"expected a caught ValueError, got {context.result!r}" + ) + assert _unescape(fragment) in context.result, ( + f"expected {fragment!r} in {context.result!r}" + ) + + +@then('no file "{name}" should exist in the sandbox') +def step_no_file(context, name): + assert not (context.sandbox / name).exists(), f"{name} should not have been written" + + +@then('the escaped symlink target should still contain "{expected}"') +def step_symlink_target_intact(context, expected): + actual = context.symlink_target.read_text(encoding="utf-8") + assert actual == _unescape(expected), ( + f"symlink target was modified: expected {expected!r}, got {actual!r}" + ) diff --git a/features/steps/sandbox_root_policy_steps.py b/features/steps/sandbox_root_policy_steps.py new file mode 100644 index 0000000..015a901 --- /dev/null +++ b/features/steps/sandbox_root_policy_steps.py @@ -0,0 +1,98 @@ +"""Step implementations for the SandboxRootPolicy containment security tests (ADR-2035).""" + +import os +import tempfile +from pathlib import Path + +from behave import given, then, when + +from cleveractors.agents.file_access import SandboxRootPolicy + + +@given("a sandbox root policy") +def step_policy(context): + context.root = Path(tempfile.mkdtemp()).resolve() + context.policy = SandboxRootPolicy(str(context.root)) + context.resolved = None + context.error = None + + +@given("a sandbox root policy with a sibling directory sharing its name prefix") +def step_policy_sibling(context): + base = Path(tempfile.mkdtemp()).resolve() + root = base / "box" + root.mkdir() + sibling = base / "box-evil" + sibling.mkdir() + (sibling / "secret.txt").write_text("outside", encoding="utf-8") + context.root = root + context.policy = SandboxRootPolicy(str(root)) + context.sibling_file = str(sibling / "secret.txt") + context.resolved = None + context.error = None + + +@given('a symlink "{name}" inside the root targeting a path outside it') +def step_symlink_outside(context, name): + outside = Path(tempfile.mkdtemp()).resolve() + target = outside / "secret.txt" + target.write_text("outside", encoding="utf-8") + os.symlink(target, context.root / name) + + +@given('a symlink "{name}" inside the root targeting another path inside it') +def step_symlink_inside(context, name): + target = context.root / "real.txt" + target.write_text("inside", encoding="utf-8") + os.symlink(target, context.root / name) + + +def _resolve(context, path): + try: + context.resolved = context.policy.resolve(path) + context.error = None + except ValueError as exc: + context.resolved = None + context.error = str(exc) + + +@when('I resolve the relative path "{path}"') +def step_resolve_relative(context, path): + _resolve(context, path) + + +@when('I resolve the raw path "{path}"') +def step_resolve_raw(context, path): + _resolve(context, path) + + +@when("I resolve an absolute path that is inside the root") +def step_resolve_absolute_inside(context): + _resolve(context, str(context.root / "inside.txt")) + + +@when("I resolve an absolute path into the sibling directory") +def step_resolve_sibling(context): + _resolve(context, context.sibling_file) + + +@then("resolution is admitted and stays inside the root") +def step_admitted(context): + assert context.error is None, f"unexpectedly rejected: {context.error}" + root = str(context.root) + assert context.resolved == root or context.resolved.startswith(root + os.sep), ( + f"{context.resolved!r} escaped root {root!r}" + ) + + +@then("resolution is rejected as outside the root") +def step_rejected_outside(context): + assert context.error is not None, f"expected rejection, got {context.resolved!r}" + assert "outside the sandbox root" in context.error, ( + f"unexpected rejection message: {context.error}" + ) + + +@then("resolution is rejected") +def step_rejected(context): + assert context.error is not None, f"expected rejection, got {context.resolved!r}" diff --git a/features/steps/tool_coverage_gaps_steps.py b/features/steps/tool_coverage_gaps_steps.py index d221e9e..e5d7161 100644 --- a/features/steps/tool_coverage_gaps_steps.py +++ b/features/steps/tool_coverage_gaps_steps.py @@ -9,6 +9,7 @@ from unittest.mock import AsyncMock, MagicMock, patch from behave import given, then, when from features.steps._coverage_utils import _catch, _run +from cleveractors.agents.file_access import FileAccessCore from cleveractors.agents.tool import ToolAgent from cleveractors.core.exceptions import AgentCreationError, ExecutionError @@ -328,6 +329,20 @@ def step_fr_missing(context): ) +@when("I call file read tool on a symlink escaping the working directory") +def step_fr_symlink_escape(context): + outside = Path(tempfile.mkdtemp()) + (outside / "secret.txt").write_text("outside", encoding="utf-8") + a = ToolAgent("tc", {"tools": ["file_read"]}) + old = os.getcwd() + os.chdir(str(context.td)) + os.symlink(outside / "secret.txt", context.td / "link.txt") + try: + context.error = _run(_catch(a._file_read_tool, {"file": "link.txt"}, None)) + finally: + os.chdir(old) + + @when("I call file read tool with empty args") def step_fr_empty(context): a = ToolAgent("tc", {"tools": ["file_read"]}) @@ -581,10 +596,9 @@ def step_ps_val_error(context): @when("I prepare append content for a section") def step_ap_section(context): - a = ToolAgent("tc", {"tools": ["file_write"]}) ap = context.td / "ap.txt" ap.write_text("existing\n") - context.result = a._prepare_append_content(str(ap), "# Section") + context.result = FileAccessCore.prepare_append_content(str(ap), "# Section") @then("the content should start with a newline") @@ -595,47 +609,45 @@ def step_ap_newline(context): # ── handle insert ──────────────────────────────────────────────────── -def _ins_agent(context): +def _ins_path(context): ip = context.td / "ins.txt" ip.write_text("a\nb\nc\n") - a = ToolAgent("tc", {"tools": ["file_write"]}) - return a, str(ip) + return str(ip) @when("I insert content at the end of a file") def step_in_end(context): - a, ip = _ins_agent(context) - lines, _ = a._handle_insert_position(ip, "end_c", "end") + ip = _ins_path(context) + lines, _ = FileAccessCore.handle_insert_position(ip, "end_c", "end") context.result = lines @when("I insert content at the start of a file") def step_in_start(context): - a, ip = _ins_agent(context) - lines, _ = a._handle_insert_position(ip, "start_c", "start") + ip = _ins_path(context) + lines, _ = FileAccessCore.handle_insert_position(ip, "start_c", "start") context.result = lines @when("I insert content at line {n} of a file") def step_in_line(context, n): - a, ip = _ins_agent(context) - lines, _ = a._handle_insert_position(ip, "l2", int(n)) + ip = _ins_path(context) + lines, _ = FileAccessCore.handle_insert_position(ip, "l2", int(n)) context.result = lines @when("I insert content with an invalid position") def step_in_invalid(context): - a, ip = _ins_agent(context) + ip = _ins_path(context) try: - a._handle_insert_position(ip, "x", "middle") - except ExecutionError as e: + FileAccessCore.handle_insert_position(ip, "x", "middle") + except ValueError as e: context.error = str(e) @when("I insert content into a nonexistent file") def step_in_new(context): - a = ToolAgent("tc", {"tools": ["file_write"]}) - lines, _ = a._handle_insert_position("/n/x.txt", "new", "end") + lines, _ = FileAccessCore.handle_insert_position("/n/x.txt", "new", "end") context.result = lines @@ -854,8 +866,7 @@ def step_append_no_newline(context): fp = os.path.join(td, "no_trailing.txt") with open(fp, "w") as f: f.write("existing") - a = ToolAgent("tc", {"tools": ["file_write"]}) - prefix = a._prepare_append_content(fp, "# Section") + prefix = FileAccessCore.prepare_append_content(fp, "# Section") context.result = prefix @@ -868,15 +879,13 @@ def step_append_double_newline(context): fp = os.path.join(td, "double_nl.txt") with open(fp, "w") as f: f.write("existing\n\n") - a = ToolAgent("tc", {"tools": ["file_write"]}) - prefix = a._prepare_append_content(fp, "# Section") + prefix = FileAccessCore.prepare_append_content(fp, "# Section") context.result = prefix @when("I prepare append content for missing file") def step_append_missing(context): - a = ToolAgent("tc", {"tools": ["file_write"]}) - prefix = a._prepare_append_content("/nonexistent/path.txt", "# Section") + prefix = FileAccessCore.prepare_append_content("/nonexistent/path.txt", "# Section") context.result = prefix diff --git a/features/tool_coverage_gaps.feature b/features/tool_coverage_gaps.feature index dae449d..76b11af 100644 --- a/features/tool_coverage_gaps.feature +++ b/features/tool_coverage_gaps.feature @@ -133,6 +133,11 @@ Feature: Tool Agent Coverage Gaps When I call file read tool with empty args Then the file read raises requires a file path + Scenario: File read tool rejects a symlink escaping the sandbox root in safe mode + Given a tool agent coverage test environment + When I call file read tool on a symlink escaping the working directory + Then the file read raises Unsafe file path blocked in safe mode + Scenario: File write tool writes content successfully Given a tool agent coverage test environment When I call file write tool with valid content in unsafe mode diff --git a/robot/InlineFileHelperTestLib.py b/robot/InlineFileHelperTestLib.py new file mode 100644 index 0000000..4055f1c --- /dev/null +++ b/robot/InlineFileHelperTestLib.py @@ -0,0 +1,103 @@ +"""Robot Framework library for the inline-code file-helper integration tests. + +Drives a real :class:`~cleveractors.agents.tool.ToolAgent` inline-code tool +against a real temporary directory — no mocks, no LLM — exercising the +sandbox-confined ``read_file`` / ``write_file`` helpers end to end (ADR-2035). +""" + +from __future__ import annotations + +import asyncio +import os +import shutil +import tempfile +from pathlib import Path + +from cleveractors.agents.tool import ToolAgent +from cleveractors.core.exceptions import ExecutionError + + +class InlineFileHelperTestLib: + """Keywords for integration-style tests of the inline file helpers.""" + + def __init__(self) -> None: + self._sandbox: Path | None = None + self._old_cwd: str | None = None + self._write_ctx: dict[str, object] = {} + self._safe_mode: bool = False + self._result: str | None = None + self._error: str | None = None + + # -- lifecycle ------------------------------------------------------- + + def start_unsafe_inline_agent_with_write_access(self) -> None: + """Create a real sandbox directory and enter it, granting write access.""" + self._sandbox = Path(tempfile.mkdtemp()).resolve() + self._old_cwd = os.getcwd() + os.chdir(self._sandbox) + self._safe_mode = False + self._write_ctx = {"_unsafe_mode": True} + self._result = None + self._error = None + + def clean_up_sandbox(self) -> None: + """Restore the working directory and remove the sandbox.""" + if self._old_cwd is not None: + os.chdir(self._old_cwd) + self._old_cwd = None + if self._sandbox is not None: + shutil.rmtree(self._sandbox, ignore_errors=True) + self._sandbox = None + + # -- actions --------------------------------------------------------- + + def inline_code_writes_and_reads_back(self, content: str, filename: str) -> str: + """Run inline code that writes *content* to *filename* then reads it back.""" + code = f"write_file({filename!r}, {content!r})\nresult = read_file({filename!r})\n" + return self._run(code) + + def inline_read_is_rejected_for(self, path: str) -> str: + """Run inline code that reads *path*, catching the containment ValueError.""" + code = ( + "try:\n" + f" result = read_file({path!r})\n" + "except ValueError as exc:\n" + ' result = "ERR:" + str(exc)\n' + ) + return self._run(code) + + # -- assertions ------------------------------------------------------ + + def file_in_sandbox_should_contain(self, filename: str, expected: str) -> None: + """Assert the on-disk sandbox file holds exactly *expected*.""" + assert self._sandbox is not None, "sandbox not started" + actual = (self._sandbox / filename).read_text(encoding="utf-8") + assert actual == expected, f"expected {expected!r} on disk, got {actual!r}" + + def result_should_equal(self, expected: str) -> None: + assert self._error is None, f"unexpected error: {self._error}" + assert self._result == expected, f"expected {expected!r}, got {self._result!r}" + + def result_should_report_rejection_of(self, path: str) -> None: + assert self._error is None, f"unexpected hard error: {self._error}" + assert self._result is not None and self._result.startswith("ERR:"), ( + f"expected a caught ValueError, got {self._result!r}" + ) + assert path in self._result, f"expected {path!r} named in {self._result!r}" + + # -- internals ------------------------------------------------------- + + def _run(self, code: str) -> str: + agent = ToolAgent( + "inline", + {"tools": [{"name": "t", "code": code}], "safe_mode": self._safe_mode}, + ) + try: + self._result = asyncio.run( + agent.process_message("", dict(self._write_ctx)) + ) + self._error = None + except ExecutionError as exc: + self._result = None + self._error = str(exc) + return self._result or (self._error or "") diff --git a/robot/inline_code_file_helpers.robot b/robot/inline_code_file_helpers.robot new file mode 100644 index 0000000..8f23333 --- /dev/null +++ b/robot/inline_code_file_helpers.robot @@ -0,0 +1,23 @@ +*** Settings *** +Documentation Integration tests for the inline-code sandboxed file helpers +... (ADR-2035). A real ToolAgent runs an inline-code tool against +... a real temporary directory — no mocks, no LLM — reading and +... writing an actual file within the sandbox and rejecting an +... attempt to escape the sandbox root. +Library InlineFileHelperTestLib.py +Test Setup Start Unsafe Inline Agent With Write Access +Test Teardown Clean Up Sandbox + +*** Test Cases *** +Inline Code Writes And Reads A Real File Within The Sandbox + [Documentation] write_file persists to disk and read_file returns the + ... exact bytes back, all inside the sandbox root. + ${result}= Inline Code Writes And Reads Back persisted to disk report.txt + Result Should Equal persisted to disk + File In Sandbox Should Contain report.txt persisted to disk + +Inline Code Cannot Escape The Sandbox Root + [Documentation] A parent-traversal read is rejected with a ValueError + ... naming the offending path. + ${result}= Inline Read Is Rejected For ../escape.txt + Result Should Report Rejection Of ../escape.txt diff --git a/src/cleveractors/agents/file_access.py b/src/cleveractors/agents/file_access.py new file mode 100644 index 0000000..ca28168 --- /dev/null +++ b/src/cleveractors/agents/file_access.py @@ -0,0 +1,441 @@ +"""Sandbox-confined file access cores shared by the built-in ``file_read`` / +``file_write`` tools and the inline-code ``read_file`` / ``write_file`` helpers. + +This module realises ADR-2035's *single validation surface* decision (D-2): +containment, ``offset``/``max_chars`` read windowing (ADR-2033), and the +``w``/``a``/``insert`` write semantics (§4.5.5) each have exactly one +implementation here, so the two public surfaces — the LLM-facing tools and the +inline-code helpers — cannot drift apart. + +Design (see ``docs/adr/ADR-2035-inline-code-sandboxed-file-access.md``): + +* :class:`SandboxRootPolicy` — the containment boundary (D-4). A path is + admitted iff its fully resolved real path (``..`` collapsed, symlinks + followed) is the sandbox root or a descendant of it. It is a *Specification* + object: it answers "is this path admissible?" and nothing else. +* :class:`FileAccessCore` — a *Facade* over the windowed read and moded write + mechanics. It performs I/O only; it does not decide policy. +* :class:`InlineFileHelperFactory` — a *Factory* that binds the two inline-code + callables (``read_file`` / ``write_file``) to a core and an invocation + context, applying the mode gating of D-5. + +The helper-facing surface raises :class:`ValueError` on every rejection so an +inline-code body can catch it with the sandbox's own error vocabulary +(§13.2.1); the tool adapters translate core failures into ``ExecutionError`` +for the tool-calling protocol. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from typing import Any, Callable, Literal, Optional, Union + +__all__ = [ + "PositionType", + "WriteMode", + "FileReadResult", + "FileWriteResult", + "SandboxRootPolicy", + "FileAccessCore", + "InlineFileHelperFactory", + "coerce_read_window", +] + +PositionType = Union[None, int, Literal["start", "end"]] +WriteMode = Literal["w", "a", "insert"] + + +@dataclass(frozen=True) +class FileReadResult: + """Outcome of a windowed file read (ADR-2033 semantics). + + Attributes: + content: The decoded window ``content[offset:window_end]``. + char_count: Total character count of the whole file. + line_count: Total line count of the whole file. + window_end: Character position where the returned window stops. + truncated: True when ``max_chars`` stopped the window before EOF. + at_eof_beyond: True when ``offset`` landed at or beyond EOF + (``offset > 0 and offset >= char_count``); ``content`` is empty. + """ + + content: str + char_count: int + line_count: int + window_end: int + truncated: bool + at_eof_beyond: bool + + +@dataclass(frozen=True) +class FileWriteResult: + """Outcome of a moded write (§4.5.5 semantics). + + Attributes: + chars_written: Number of characters written (``len(content)``). + insert_location: 1-indexed line the content was inserted before. + ``None`` for ``w``/``a`` modes, where no line position applies. + """ + + chars_written: int + insert_location: Optional[int] = None + + +def coerce_read_window(max_chars: Any, offset: Any) -> tuple[Optional[int], int]: + """Coerce and validate the ``max_chars`` / ``offset`` read-window pair. + + Mirrors the ``int()`` coercion the ``file_read`` tool established + (ADR-2033 D-4): an integer-valued float is truncated, only genuinely + non-numeric input is rejected, and ``offset`` must be non-negative. + + Args: + max_chars: Optional character cap (``None`` means "to EOF"). + offset: Character position to start from (``None`` treated as ``0``). + + Returns: + The coerced ``(max_chars, offset)`` pair. + + Raises: + ValueError: If either value is non-numeric, or ``offset`` is negative. + """ + coerced_max: Optional[int] = None + if max_chars is not None: + try: + coerced_max = int(max_chars) + except (TypeError, ValueError) as exc: + raise ValueError( + f"max_chars must be an integer, got {max_chars!r}" + ) from exc + + try: + coerced_offset = 0 if offset is None else int(offset) + except (TypeError, ValueError) as exc: + raise ValueError(f"offset must be an integer, got {offset!r}") from exc + + if coerced_offset < 0: + raise ValueError(f"offset must be non-negative, got {coerced_offset}") + + return coerced_max, coerced_offset + + +class SandboxRootPolicy: + """The sandbox-root containment boundary (ADR-2035 D-4). + + A path is admitted iff its fully resolved real path — ``..`` collapsed and + symlinks followed — is the sandbox root or a descendant of it. Raw ``..`` + traversal components and ``~`` home-directory expansion are rejected + outright, before resolution. + + The root defaults to the process working directory at construction time, + matching the boundary the ``file_write`` tool already enforces. Follows the + *Specification* pattern: it encapsulates one reusable, composable admission + rule and owns no I/O. + """ + + def __init__(self, root: Optional[str] = None) -> None: + """Initialise the policy. + + Args: + root: Sandbox root directory. Defaults to the current working + directory. The value is resolved to a real path (symlinks + followed) so containment comparisons are stable. + """ + self._root: str = os.path.realpath(root if root else os.getcwd()) + + def is_within(self, resolved_path: str) -> bool: + """Return whether an already-resolved real path lies within the root. + + Args: + resolved_path: An ``os.path.realpath`` result to test. + + Returns: + True if the path is the root itself or a descendant of it. + """ + return resolved_path == self._root or resolved_path.startswith( + self._root + os.sep + ) + + def resolve(self, path: str) -> str: + """Resolve ``path`` against the root, enforcing containment. + + Args: + path: A relative or absolute path supplied by inline code. + + Returns: + The resolved real path, guaranteed to be within the sandbox root. + + Raises: + ValueError: Naming the offending path, when it is empty/non-string, + begins with ``~``, contains a ``..`` traversal component, or + resolves (symlinks followed) outside the sandbox root. + """ + if not isinstance(path, str) or not path: + raise ValueError(f"file path must be a non-empty string, got {path!r}") + if path.startswith("~"): + raise ValueError(f"home-directory paths ('~') are not allowed: {path!r}") + if self._has_parent_traversal(path): + raise ValueError(f"parent traversal ('..') is not allowed: {path!r}") + + base = path if os.path.isabs(path) else os.path.join(self._root, path) + resolved = os.path.realpath(base) + if not self.is_within(resolved): + raise ValueError(f"path resolves outside the sandbox root: {path!r}") + return resolved + + @staticmethod + def _has_parent_traversal(path: str) -> bool: + """Return whether any path component is a ``..`` parent reference.""" + components = path.replace("\\", "/").split("/") + return ".." in components + + +class FileAccessCore: + """Facade over the windowed-read and moded-write file mechanics. + + Backs both the built-in tools and the inline-code helpers so the read + windowing (ADR-2033) and the ``w``/``a``/``insert`` write semantics + (§4.5.5) exist in exactly one place (ADR-2035 D-2). This class performs I/O + only; containment is the responsibility of :class:`SandboxRootPolicy`, and + the callers decide which paths reach it. + """ + + def __init__(self, root_policy: Optional[SandboxRootPolicy] = None) -> None: + """Initialise the core. + + Args: + root_policy: Containment policy used by the inline-code helpers. + Defaults to a policy rooted at the current working directory. + """ + self._root_policy: SandboxRootPolicy = root_policy or SandboxRootPolicy() + + @property + def root_policy(self) -> SandboxRootPolicy: + """The containment policy backing helper path resolution.""" + return self._root_policy + + def read_window( + self, path: str, max_chars: Optional[int], offset: int + ) -> FileReadResult: + """Read a UTF-8 file and return the ``[offset, offset+max_chars)`` window. + + Args: + path: Path to an existing, readable file (already validated by the + caller — this method performs no containment check). + max_chars: Character cap for the window, or ``None`` to read to EOF. + offset: Character position to start the window at. + + Returns: + A :class:`FileReadResult` describing the window and the whole file. + + Raises: + OSError: If the file cannot be opened or read. + UnicodeDecodeError: If the file is not valid UTF-8. + """ + with open(path, "r", encoding="utf-8") as handle: + content = handle.read() + + char_count = len(content) + line_count = content.count("\n") + 1 + + # offset == 0 always falls through, even for an empty file, so the + # no-offset path stays byte-for-byte identical to the pre-offset + # behaviour (ADR-2033 D-4). + if offset > 0 and offset >= char_count: + return FileReadResult( + content="", + char_count=char_count, + line_count=line_count, + window_end=offset, + truncated=False, + at_eof_beyond=True, + ) + + window_end = ( + char_count if max_chars is None else min(char_count, offset + max_chars) + ) + truncated = max_chars is not None and window_end < char_count + return FileReadResult( + content=content[offset:window_end], + char_count=char_count, + line_count=line_count, + window_end=window_end, + truncated=truncated, + at_eof_beyond=False, + ) + + def write( + self, + path: str, + content: str, + mode: WriteMode = "w", + position: PositionType = None, + ) -> FileWriteResult: + """Write ``content`` to ``path`` honouring the §4.5.5 write modes. + + This is the single implementation of the write mechanics — both the + ``file_write`` tool and the ``write_file`` inline helper call this + method rather than each opening/writing the file independently + (ADR-2035 D-2). + + Args: + path: Destination path (already validated by the caller — this + method performs no containment check). + content: The content to write. + mode: ``w`` (overwrite), ``a`` (append), or ``insert``. + position: Insert position for ``insert`` mode (see §4.5.5). + + Returns: + A :class:`FileWriteResult` with the characters written and, for + ``insert`` mode, the 1-indexed insertion line. + + Raises: + ValueError: If ``mode`` is not one of the defined values. + OSError: If the file cannot be written. + """ + if mode == "w": + with open(path, "w", encoding="utf-8") as handle: + handle.write(content) + return FileWriteResult(chars_written=len(content)) + + if mode == "a": + to_append = self.prepare_append_content(path, content) + with open(path, "a", encoding="utf-8") as handle: + handle.write(to_append) + return FileWriteResult(chars_written=len(content)) + + if mode == "insert": + lines, location = self.handle_insert_position(path, content, position) + with open(path, "w", encoding="utf-8") as handle: + handle.writelines(lines) + return FileWriteResult(chars_written=len(content), insert_location=location) + + raise ValueError( + f"Invalid mode {mode!r}. Use 'w' (write), 'a' (append), or 'insert'." + ) + + @staticmethod + def prepare_append_content(path: str, content: str) -> str: + """Prepare append content, adding section spacing for Markdown headings. + + Only content that looks like a document section (starts with ``#``) + receives a separating blank line, so simple appends stay unaffected. + """ + if not content.lstrip().startswith("#"): + return content + + try: + with open(path, "r", encoding="utf-8") as handle: + existing = handle.read() + except FileNotFoundError: + return content + + if existing and not existing.endswith("\n\n"): + prefix = "\n" if existing.endswith("\n") else "\n\n" + else: + prefix = "" + return prefix + content + + @staticmethod + def handle_insert_position( + path: str, content: str, position: PositionType + ) -> tuple[list[str], int]: + """Compute the line list and 1-indexed location for an ``insert`` write.""" + try: + with open(path, "r", encoding="utf-8") as handle: + lines = handle.readlines() + except FileNotFoundError: + lines = [] + + formatted = content if content.endswith("\n") else content + "\n" + + if position is None or position == "end": + lines.append(formatted) + return lines, len(lines) + if position == "start": + lines.insert(0, formatted) + return lines, 1 + if isinstance(position, int): + line_idx = max(0, min(position - 1, len(lines))) + lines.insert(line_idx, formatted) + return lines, line_idx + 1 + + raise ValueError( + f"Invalid position {position!r}. Use 'start', 'end', or a line number." + ) + + +class InlineFileHelperFactory: + """Builds the ``read_file`` / ``write_file`` callables injected into inline + code in unsafe mode (ADR-2035 D-1, D-5). + + The factory captures whether the invocation context opted into writes + (``_unsafe_mode``); ``read_file`` is always permitted (subject to + containment), while ``write_file`` refuses without that opt-in — preserving + the read/write privilege split. Every rejection is a :class:`ValueError` so + inline code can catch it with the sandbox's own vocabulary. + + ``build()`` returns closures rather than bound methods: inline code that + reaches for ``read_file.__self__`` finds nothing, so the unconfined + :class:`FileAccessCore` (whose own methods perform no containment check — + that is the caller's responsibility) is not reachable through the returned + callables. This is defense-in-depth only — the inline sandbox's + ``__import__`` builtin already makes arbitrary filesystem access possible + by other means — but it closes the specific, cleaner bypass path a bound + method would otherwise expose. + """ + + def __init__(self, core: FileAccessCore, *, write_enabled: bool) -> None: + """Initialise the factory. + + Args: + core: The shared file-access core to delegate I/O to. + write_enabled: Whether the invocation context contained + ``_unsafe_mode: true`` (gates ``write_file``). + """ + self._core = core + self._write_enabled = write_enabled + + def build(self) -> dict[str, Callable[..., Any]]: + """Return the injectable ``{name: callable}`` mapping.""" + core = self._core + write_enabled = self._write_enabled + + def read_file( + path: str, max_chars: Optional[int] = None, offset: int = 0 + ) -> str: + """Return the raw decoded contents of ``path`` within the sandbox + root, honouring the ADR-2033 ``offset``/``max_chars`` windowing. + Returns the content itself — not the ``[FILE_READ_SUCCESS]`` + envelope. + + Raises: + ValueError: On containment violation or invalid window + arguments. + """ + resolved = core.root_policy.resolve(path) + coerced_max, coerced_offset = coerce_read_window(max_chars, offset) + return core.read_window(resolved, coerced_max, coerced_offset).content + + def write_file(path: str, content: str, mode: WriteMode = "w") -> int: + """Write ``content`` to ``path`` within the sandbox root; return + the number of characters written. + + Raises: + ValueError: When the context did not opt into writes + (``_unsafe_mode``), on containment violation, when + ``content`` is not a string, or on an invalid mode. + """ + if not write_enabled: + raise ValueError( + "write_file requires unsafe mode " + "('_unsafe_mode: true' in the invocation context)" + ) + if not isinstance(content, str): + raise ValueError( + f"content must be a string, got {type(content).__name__}" + ) + resolved = core.root_policy.resolve(path) + return core.write(resolved, content, mode).chars_written + + return {"read_file": read_file, "write_file": write_file} diff --git a/src/cleveractors/agents/tool.py b/src/cleveractors/agents/tool.py index 742a2fd..5468f32 100644 --- a/src/cleveractors/agents/tool.py +++ b/src/cleveractors/agents/tool.py @@ -12,11 +12,16 @@ import logging import os import re import subprocess # nosec B404: intentional subprocess for tool execution -from typing import Any, List, Literal, Optional, Union +from typing import Any, List, Optional import aiohttp from cleveractors.agents.base import Agent +from cleveractors.agents.file_access import ( + FileAccessCore, + InlineFileHelperFactory, + SandboxRootPolicy, +) from cleveractors.agents.llm_tools import SKILL_TOOL_NAME from cleveractors.core.exceptions import AgentCreationError, ExecutionError from cleveractors.templates.renderer import TemplateRenderer @@ -456,6 +461,23 @@ class ToolAgent(Agent): # Add json module for convenience safe_globals["json"] = __import__("json") # type: ignore[assignment] + # Inject the sandbox-confined file helpers when this agent's safe_mode + # is false, or the invocation context opts into _unsafe_mode (ADR-2035 + # D-1/D-5; §4.5.2 point 3) — the same two conditions §4.5.4 already + # uses to relax file_read/file_write. When neither holds, the names + # are never bound, so referencing read_file/write_file raises + # NameError — which _execute_python_code surfaces as a helpful + # ExecutionError below. + ctx_unsafe = isinstance(context, dict) and bool( + context.get("_unsafe_mode", False) + ) + helpers_enabled = (not self.safe_mode) or ctx_unsafe + if helpers_enabled: + helpers = InlineFileHelperFactory( + self._make_file_core(), write_enabled=ctx_unsafe + ).build() + safe_globals.update(helpers) + try: # Execute the code in a shared environment so helper functions see locals exec_env = dict(safe_globals) @@ -617,6 +639,15 @@ class ToolAgent(Agent): except Exception as e: raise ExecutionError(f"HTTP request failed: {e}") from e + def _make_file_core(self) -> FileAccessCore: + """Build a file-access core rooted at the current working directory. + + A fresh core is created per operation so the sandbox root tracks the + process CWD at call time (Behave/Robot suites ``chdir`` into temp + directories), matching the tool's long-standing dynamic-root behavior. + """ + return FileAccessCore(SandboxRootPolicy()) + async def _file_read_tool( self, args: dict[str, Any], context: Optional[dict[str, Any]] ) -> str: @@ -674,13 +705,18 @@ class ToolAgent(Agent): f"grep, find, ls, cat. Use file_read only for file/directory paths." ) + unsafe_ctx = bool(context and context.get("_unsafe_mode", False)) if self.safe_mode: if ".." in filepath: raise ExecutionError("Unsafe file path blocked in safe mode") - if filepath.startswith("/") and not ( - context and context.get("_unsafe_mode", False) - ): + if filepath.startswith("/") and not unsafe_ctx: raise ExecutionError("Unsafe file path blocked in safe mode") + if not unsafe_ctx and not os.path.isabs(filepath): + # Close the symlink-escape gap (ADR-2035 D-4): reject a + # relative path that resolves (symlinks followed) outside the + # sandbox root even when it carries no literal '..' component. + if not SandboxRootPolicy().is_within(os.path.realpath(filepath)): + raise ExecutionError("Unsafe file path blocked in safe mode") max_chars = args.get("max_chars") if max_chars is not None: @@ -717,39 +753,30 @@ class ToolAgent(Agent): result_parts.append(f" {marker} {entry}{size_str}") return "\n".join(result_parts) - with open(filepath, "r", encoding="utf-8") as f: - content = f.read() - - char_count = len(content) - line_count = content.count("\n") + 1 - - # offset == 0 always falls through here, even for an empty file, - # so the no-offset path stays byte-for-byte identical to the - # pre-offset behavior (ADR-2033 D-4). - if offset > 0 and offset >= char_count: - header = ( - f"[FILE_READ_SUCCESS]📄 File: {filepath} | " - f"Lines: {line_count} | Size: {char_count} chars | " - f"NO_MORE_CONTENT_AT_OFFSET: {offset}" - ) - return f"{header}\n[FILE_CONTENT_START]\n[FILE_CONTENT_END]" - - window_end = ( - char_count if max_chars is None else min(char_count, offset + max_chars) + # Delegate the offset/max_chars windowing to the shared core so the + # tool and the inline-code read_file helper cannot drift (ADR-2035 + # D-2; ADR-2033 windowing semantics). + read_result = self._make_file_core().read_window( + filepath, max_chars, offset ) - truncated = max_chars is not None and window_end < char_count - content = content[offset:window_end] - header = ( f"[FILE_READ_SUCCESS]📄 File: {filepath} | " - f"Lines: {line_count} | Size: {char_count} chars" + f"Lines: {read_result.line_count} | " + f"Size: {read_result.char_count} chars" ) - if truncated: + if read_result.at_eof_beyond: + header += f" | NO_MORE_CONTENT_AT_OFFSET: {offset}" + return f"{header}\n[FILE_CONTENT_START]\n[FILE_CONTENT_END]" + + if read_result.truncated: header += ( f" | TRUNCATED to {max_chars} chars" - f" | MORE_CONTENT_AT_OFFSET: {window_end}" + f" | MORE_CONTENT_AT_OFFSET: {read_result.window_end}" ) - return f"{header}\n[FILE_CONTENT_START]\n{content}\n[FILE_CONTENT_END]" + return ( + f"{header}\n[FILE_CONTENT_START]\n" + f"{read_result.content}\n[FILE_CONTENT_END]" + ) except FileNotFoundError: parent_dir = os.path.dirname(filepath) or "." raise ExecutionError( @@ -812,10 +839,12 @@ class ToolAgent(Agent): logger.error("Directory traversal pattern detected in: %s", filepath) raise ExecutionError("Unsafe file path blocked in safe mode") - # Check if path escapes working directory - path_escapes_working_dir = ( - not absolute_path.startswith(working_dir + os.sep) - and absolute_path != working_dir + # Check if path escapes the sandbox root. Resolution follows symlinks + # (realpath) via the shared containment policy so a symlink that + # escapes the root is rejected too (ADR-2035 D-4) — a strict + # strengthening of the previous abspath-only check. + path_escapes_working_dir = not SandboxRootPolicy().is_within( + os.path.realpath(normalized_path) ) if not unsafe_mode: @@ -849,71 +878,17 @@ class ToolAgent(Agent): logger.error("Home directory expansion blocked for %s", filepath) raise ExecutionError("Home directory paths (~) are not allowed") - def _prepare_append_content(self, filepath: str, content: str) -> str: - """Prepare content for append mode with proper spacing for sections.""" - # Only add spacing if content looks like a document section (starts with #) - # This prevents breaking simple append operations - is_section = content.lstrip().startswith("#") - - if not is_section: - # Simple append without spacing - return content - - try: - with open(filepath, "r", encoding="utf-8") as f: - existing_content = f.read() - - # Add spacing for section-based content - if existing_content and not existing_content.endswith("\n\n"): - if existing_content.endswith("\n"): - prefix = "\n" # Has one newline, add one more - else: - prefix = "\n\n" # No newline at all, add two - else: - prefix = "" - except FileNotFoundError: - prefix = "" # File doesn't exist yet - - return prefix + content - - def _handle_insert_position( - self, - filepath: str, - content: str, - position: Union[None, int, Literal["start", "end"]], - ) -> tuple[list[str], int]: - """Handle insert mode positioning logic.""" - try: - with open(filepath, "r", encoding="utf-8") as f: - existing_lines = f.readlines() - except FileNotFoundError: - existing_lines = [] - - # Ensure content ends with newline - formatted_content = content if content.endswith("\n") else content + "\n" - - # Determine insertion position - if position is None or position == "end": - existing_lines.append(formatted_content) - insert_location = len(existing_lines) - elif position == "start": - existing_lines.insert(0, formatted_content) - insert_location = 1 - elif isinstance(position, int): - line_idx = max(0, min(position - 1, len(existing_lines))) - existing_lines.insert(line_idx, formatted_content) - insert_location = line_idx + 1 - else: - raise ExecutionError( - f"Invalid position '{position}'. Use 'start', 'end', or line number." - ) - - return existing_lines, insert_location - async def _file_write_tool( self, args: dict[str, Any], context: Optional[dict[str, Any]] ) -> str: - """File writing tool with support for write, append, and insert modes.""" + """File writing tool with support for write, append, and insert modes. + + Delegates the actual write mechanics to + :meth:`FileAccessCore.write` (ADR-2035 D-2) — this method only + validates inputs/path safety and formats the result message; the + ``w``/``a``/``insert`` semantics live in exactly one place, shared + with the inline-code ``write_file`` helper. + """ filepath = args.get("file", "") content = args.get("content", "") mode = args.get("mode", "w") # w=write, a=append, insert=insert at pos @@ -931,38 +906,22 @@ class ToolAgent(Agent): self._validate_file_path_safety(filepath, unsafe_mode) try: - if mode == "w": - # Standard write (overwrite) - with open(filepath, "w", encoding="utf-8") as f: - f.write(content) - return f"Successfully wrote {len(content)} characters to {filepath}" - - if mode == "a": - # Append mode with proper spacing - content_to_append = self._prepare_append_content(filepath, content) - with open(filepath, "a", encoding="utf-8") as f: - f.write(content_to_append) - return f"Successfully appended {len(content)} characters to {filepath}" - - if mode == "insert": - # Insert mode at specified position - existing_lines, insert_location = self._handle_insert_position( - filepath, content, position - ) - with open(filepath, "w", encoding="utf-8") as f: - f.writelines(existing_lines) - return ( - f"Successfully inserted {len(content)} characters " - f"at line {insert_location} in {filepath}" - ) - - raise ExecutionError( - f"Invalid mode '{mode}'. Use 'w' (write), 'a' (append), or 'insert'." - ) + result = self._make_file_core().write(filepath, content, mode, position) except Exception as e: logger.error("File write failed for %s: %s", filepath, e) raise ExecutionError(f"File write failed: {e}") from e + if mode == "a": + return ( + f"Successfully appended {result.chars_written} characters to {filepath}" + ) + if mode == "insert": + return ( + f"Successfully inserted {result.chars_written} characters " + f"at line {result.insert_location} in {filepath}" + ) + return f"Successfully wrote {result.chars_written} characters to {filepath}" + async def _progress_bar_tool( self, args: dict[str, Any], context: Optional[dict[str, Any]] ) -> str: