diff --git a/FEATURE_PARITY.md b/FEATURE_PARITY.md index 40b3bb40c..a251a54da 100644 --- a/FEATURE_PARITY.md +++ b/FEATURE_PARITY.md @@ -207,7 +207,7 @@ This document tracks feature parity between IronClaw (Rust implementation) and O | Post-compaction read audit | ✅ | ❌ | Layer 3: workspace rules appended to summaries | | Post-compaction context injection | ✅ | ❌ | Workspace context as system event | | Custom system prompts | ✅ | ✅ | Template variables, safety guardrails | -| Skills (modular capabilities) | ✅ | ✅ | Prompt-based skills with trust gating, attenuation, activation criteria, catalog, selector, and loaded bundle-root metadata | +| Skills (modular capabilities) | ✅ | ✅ | Prompt-based skills with trust gating, attenuation, activation criteria, catalog, selector, loaded bundle-root metadata, and scoped bundled-file reads | | Skill routing blocks | ✅ | 🚧 | ActivationCriteria (keywords, patterns, tags) but no "Use when / Don't use when" blocks | | Skill path compaction | ✅ | ❌ | ~ prefix to reduce prompt tokens | | Thinking modes (off/minimal/low/medium/high/xhigh/adaptive) | ✅ | ❌ | Configurable reasoning depth | @@ -550,7 +550,7 @@ This document tracks feature parity between IronClaw (Rust implementation) and O - ✅ Cron job scheduling (routines) - ✅ CLI subcommands (onboard, config, status, memory) - ✅ Gateway token auth -- ✅ Skills system (prompt-based with trust gating, attenuation, activation criteria, and loaded bundle-root metadata) +- ✅ Skills system (prompt-based with trust gating, attenuation, activation criteria, loaded bundle-root metadata, and scoped bundled-file reads) - ✅ Session file permissions (0o600) - ✅ Memory CLI commands (search, read, write, tree, status) - ✅ Shell env scrubbing + command injection detection diff --git a/docs/agent-skills-support.md b/docs/agent-skills-support.md index 552045f72..e9cbcd632 100644 --- a/docs/agent-skills-support.md +++ b/docs/agent-skills-support.md @@ -387,16 +387,18 @@ hard-coded read-only allowlist. The current allowlist is intentionally small: - `json` - `skill_list` - `skill_search` +- `skill_read_file` This is stronger than post-hoc approval checking because the filtered-out tools do not appear in the model prompt at all. ### 7.2 Practical effect on skill management -The attenuation list includes `skill_list` and `skill_search`, but not -`skill_install` or `skill_remove`. In practice, that means an installed skill -can remain discoverable and inspectable, but it cannot grant the model the -ability to install or remove skills through its own prompt. +The attenuation list includes `skill_list`, `skill_search`, and +`skill_read_file`, but not `skill_install` or `skill_remove`. In practice, that +means an installed skill can remain discoverable, searchable, and able to read +its own bundled reference material, but it cannot grant the model the ability +to install or remove skills through its own prompt. That is one of the main security properties of the current subsystem. @@ -484,13 +486,15 @@ It does not inject: - the skill's filesystem path - the source root -- ancillary bundled files -- a dedicated skill-file read tool +- ancillary bundled files inline -That is a real limitation, not just a documentation omission. The current -subsystem records enough runtime metadata for a future skill-scoped file reader, -but remains a `SKILL.md` prompt-body injector rather than a general skill-bundle -runtime. +Ancillary bundled files are deliberately accessed lazily through the +`skill_read_file` tool. The tool accepts a canonical skill identifier and a +bundle-relative path, then delegates to `src/skills/file_read.rs` for path +policy and filesystem checks. The adapter in +`src/tools/builtin/skill_tools.rs` only translates JSON parameters and +responses; it must not duplicate path policy or call the generic `read_file` +tool. ## 9. Extension points @@ -504,6 +508,7 @@ Table 5. Main extension points in the current design. | New activation logic | Change `prefilter_skills()` scoring or add more deterministic inputs. | | New management surface | Add tool, command, or web handlers that call the registry. | | Richer context injection | Extend dispatcher wrapping or `Reasoning::with_skill_context()`. | +| Bundled file read policy | Change `src/skills/file_read.rs` and keep adapters thin. | | Stronger operator validation | Extend `doctor` or add settings checks around the registry and catalog. | The current design keeps these seams fairly local. The parser, registry, @@ -529,19 +534,20 @@ The current implementation has several important constraints: or channel metadata. - skill injection is limited to the interactive dispatcher path and does not currently apply to worker jobs, routines, or the OpenAI-compatible proxy. -- the runtime injects only the selected `SKILL.md` body, not a file path, - ancillary references, or the private filesystem root. -- there is no dedicated skill-scoped file-reading tool in the live runtime. +- the runtime injects only the selected `SKILL.md` body, not ancillary + references or the private filesystem root. +- `skill_read_file` is read-only and inline-text-only. It does not list + directories, glob files, expose absolute paths, return base64 assets, or + fetch binary content. - the read-only allowlist for installed skills is hard-coded, so expanding the skill-safe tool surface requires code changes. - `skill_install` supports exactly one of `content`, `url`, or `name`, and its tool schema no longer always requires `name`. The exact-one-source rule is enforced at runtime because the OpenAI-compatible schema subset used by this project forbids top-level `oneOf`. -- validated `.skill` installs now preserve bundled `references/` and `assets/` - on disk, and loaded skills retain canonical root and entrypoint metadata, but - the runtime still injects only `SKILL.md` and does not yet expose a - skill-scoped file-reading interface. +- validated `.skill` installs preserve bundled `references/` and `assets/` on + disk, loaded skills retain canonical root and entrypoint metadata, and + `skill_read_file` can read allowed bundle-relative text files on demand. - rediscovery infers the package kind from the installed tree. A tree with `references/` or `assets/` is treated as a bundle; a tree containing only `SKILL.md` is indistinguishable from a raw markdown install and is treated as diff --git a/docs/axinite-architecture-overview.md b/docs/axinite-architecture-overview.md index 3506ab1e7..559c1faf9 100644 --- a/docs/axinite-architecture-overview.md +++ b/docs/axinite-architecture-overview.md @@ -162,7 +162,7 @@ Table 2. AppBuilder phases and the state they add. | Secrets | Creates the encrypted secrets store when a master key is available, injects stored provider credentials into the config overlay, and falls back to operating system credentials when no encrypted store is available | LLM credential resolution intentionally happens more than once | | LLM | Builds the provider chain and any recording wrapper used for tracing | The chain is responsible for retry, failover, and routing decorators | | Tools and workspace | Creates the safety layer, tool registry, embedding provider, workspace memory, and optional image or builder tools | Workspace memory only exists when a database backend is active | -| Skills | [`init_skills()`](./developers-guide.md#init_skills) discovers local and installed skills, records canonical runtime roots and `SKILL.md` entrypoints in `LoadedSkill`, wires skill tools into the `ToolRegistry`, and exposes the registry and catalog handles used by the shared staged install path for raw `SKILL.md` content and validated passive `.skill` bundles | Runs during construction only; `RuntimeSideEffects` does not load skills. Active-skill prompts expose logical bundle-relative metadata, not private host filesystem roots | +| Skills | [`init_skills()`](./developers-guide.md#init_skills) discovers local and installed skills, records canonical runtime roots and `SKILL.md` entrypoints in `LoadedSkill`, wires skill tools into the `ToolRegistry`, and exposes the registry and catalog handles used by the shared staged install path for raw `SKILL.md` content, validated passive `.skill` bundles, and scoped bundled-file reads | Runs during construction only; `RuntimeSideEffects` does not load skills. Active-skill prompts expose logical bundle-relative metadata, not private host filesystem roots. `skill_read_file` resolves only bundle-relative paths through `src/skills/file_read.rs` rather than the generic filesystem tools | | Extensions | Starts MCP session and process managers, creates the WASM runtime, loads runtime extensions, loads registry metadata, and creates the extension manager | The extension manager is registered back into the tool system so the agent can discover and manage extensions in chat | diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 44de7d2dc..f521e3538 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -481,6 +481,7 @@ The skills sub-system is split into two top-level modules: | Module | Purpose | | --- | --- | | `src/skills/bundle/` | ZIP archive sniffing, path parsing, and passive bundle validation | +| `src/skills/file_read.rs` and `src/skills/file_read/` | Runtime `skill_read_file` response types, bundle-relative path policy, and scoped file I/O | | `src/skills/install_source.rs` | Shared blankness and trimming helpers used by install adapters before they choose a source mode | | `src/skills/registry/` | In-memory registry; discovery, loading, staged install, removal | @@ -1585,15 +1586,41 @@ The builder defaults: version `1.0.0`, trust `Trusted`, source `/tmp`, `location` override is accepted via `.location(LoadedSkillLocation::new(...))`; if omitted, the builder derives `SingleFile` from `/tmp/SKILL.md`. +##### Scoped skill file reads (roadmap 1.3.4) + +`src/skills/file_read.rs` owns the domain contract for `skill_read_file`. +Adapters must pass a loaded `LoadedSkill` and the requested bundle-relative +path into `read_skill_file(...)` rather than resolving filesystem paths +themselves. + +The policy accepts only `SKILL.md`, files under `references/`, and files under +`assets/`. It rejects absolute paths, traversal, backslashes, nested +`SKILL.md`, unsupported roots, symlinks, non-regular files, and targets whose +canonical path does not remain under the loaded skill root. It also rechecks +metadata after opening the file to avoid replacement races. + +The response model returns inline UTF-8 text for readable files. Unknown +skills, disallowed paths, missing files, invalid UTF-8, I/O failures, oversized +files, and binary assets use typed JSON payloads. Binary assets and oversized +files include non-inline metadata; they do not return base64 content in this +phase. + +The built-in adapter is `SkillReadFileTool` in +`src/tools/builtin/skill_tools.rs`. It looks up the skill in `SkillRegistry`, +drops the registry lock before async file I/O, and serializes the domain +response into `ToolOutput::success(...)`. + ##### rstest-bdd `rstest-bdd = "0.5.0"` and `rstest-bdd-macros = "0.5.0"` (with `compile-time-validation`) are `[dev-dependencies]` added for behavioural tests -introduced in 1.3.3. They are used in +introduced in 1.3.3 and extended in 1.3.4. They are used in `src/agent/dispatcher/tests/skill_bundle_context_bdd.rs` to validate the -model-facing active-skill prompt contract. The `compile-time-validation` feature -causes the macro to verify that every step referenced in a `.feature` file has a -matching Rust step function at compile time. +model-facing active-skill prompt contract and in +`src/tools/builtin/skill_tools/features/skill_read_file.feature` to validate +the model-facing bundled-file read behaviour. The `compile-time-validation` +feature causes the macro to verify that every step referenced in a `.feature` +file has a matching Rust step function at compile time. See `execplans/1-3-3-persist-canonical-skill-roots-in-the-loaded-model.md` for the full implementation history. diff --git a/docs/execplans/1-3-4-skill-read-file-interface.md b/docs/execplans/1-3-4-skill-read-file-interface.md new file mode 100644 index 000000000..45f836c66 --- /dev/null +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -0,0 +1,935 @@ +# Add the `skill_read_file` interface for bundled skill resources + +This ExecPlan (execution plan) is a living document. The sections +`Constraints`, `Tolerances`, `Risks`, `Progress`, +`Surprises & Discoveries`, `Decision Log`, and +`Outcomes & Retrospective` must be kept up to date as work proceeds. + +Status: COMPLETE + +## Purpose / big picture + +Roadmap item `1.3.4` completes the runtime access path for multi-file skill +bundles. Roadmap items `1.3.1`, `1.3.2`, and `1.3.3` already validate passive +`.skill` archives, preserve `SKILL.md`, `references/`, and `assets/` during +installation, and store the canonical runtime root plus bundle-relative +entrypoint in each `LoadedSkill`. The missing behaviour is a model-callable, +read-only interface that can read a specific bundle-relative file without +giving the model raw host filesystem access. + +After this change, a model can call `skill_read_file` with a canonical skill +identifier such as `deploy-docs` and a path such as `references/usage.md`. +Text files under `SKILL.md`, `references/**`, and text-compatible +`assets/**` are returned inline through a stable JSON payload. Absolute paths, +path traversal, unsupported locations, unknown skills, binary assets, and +oversized files return deterministic skill-scoped error payloads. This lets +skills use progressive disclosure: activate the skill, read `SKILL.md`, and +then lazily read only the referenced bundled resources that are needed. + +The implementation must not start until this plan is explicitly approved. + +## Approval gates + +The first gate is plan approval. A human reviewer must explicitly approve this +ExecPlan before implementation starts. Silence is not approval. + +The second gate is implementation completion. The implementer must finish the +domain path policy, tool adapter, tool registration, attenuation update, +tests, documentation, and roadmap update without widening generic filesystem +access. + +The third gate is milestone review. After each major milestone, run +`coderabbit review --agent`, resolve or record every concern, and continue only +when the review has no unresolved blocking findings. + +The fourth gate is validation. Run the targeted tests while developing and the +final repository gates before committing: `make check-fmt`, `make lint`, +`make test`, and the aggregate `make all` if the individual target transcript +does not already satisfy the repository gate. Long-running validation commands +must be run sequentially through `tee` to log files under `/tmp`. + +The fifth gate is documentation sync. Update user-facing and maintainer-facing +documents before marking roadmap item `1.3.4` done. + +## Repository orientation + +Start with `AGENTS.md`, `docs/contents.md`, +`docs/welcome-to-axinite.md`, and +`docs/axinite-architecture-overview.md`. These describe the repository rules, +product direction, and top-level runtime shape. `docs/roadmap.md` defines +item `1.3.4` as the read-only bundled-resource access task and states the +success rule: the model can read bundle-relative files, oversized or +disallowed files fail through a skill-scoped error path, and raw filesystem +access is not exposed. + +`docs/rfcs/0003-skill-bundle-installation.md` is the design authority for this +feature. Keep the sections `Problem`, `Reference Model`, `Runtime Model +Interface`, `Why A Dedicated Tool Instead Of Raw File Paths`, `Security +Considerations`, `Testing`, and `Rollout Plan` open while implementing. The +RFC suggests the model-facing schema, successful response, typed non-inline +error response, and tool semantics for `skill_read_file`. + +`docs/execplans/1-3-2-extend-skill-installation-flows-bundles.md` and +`docs/execplans/1-3-3-persist-canonical-skill-roots-in-the-loaded-model.md` +record the completed prerequisites. The important inherited invariant is that +archive and install policy lives in `src/skills/`, while web handlers and +tool adapters only translate transport input into those shared policies. + +`src/skills/mod.rs` contains the core runtime model. `LoadedSkillLocation` +stores a private filesystem root, a bundle-relative entrypoint, a canonical +skill identifier, and `SkillPackageKind`. `LoadedSkill::skill_root()`, +`LoadedSkill::skill_entrypoint()`, and `LoadedSkill::package_kind()` are the +expected accessors for resolving reads. + +`src/skills/bundle/mod.rs` and `src/skills/bundle/path.rs` already own archive +validation for the bundle layout: `SKILL.md`, `references/**`, and `assets/**` +are allowed; `scripts/`, traversal, links, duplicate normalized paths, and +executables are rejected during installation. Do not duplicate archive +validation, but reuse the same vocabulary for runtime read policy. + +`src/skills/registry.rs` exposes `SkillRegistry::skills()`. The new reader can +look up an installed or loaded skill by `manifest.name` and resolve paths +against that loaded skill's private root. If this lookup shape becomes awkward, +add the smallest registry helper in `src/skills/registry.rs` rather than +letting the tool adapter scan private fields or infer install paths itself. + +`src/tools/builtin/skill_tools.rs`, `src/tools/builtin/skill_tools/install.rs`, +and `src/tools/builtin/skill_tools/remove.rs` show the existing +registry-backed skill tools. `src/tools/registry/builtins.rs` registers skill +management tools with the active `ToolRegistry`. `src/tools/builtin/mod.rs` +re-exports built-in tool types. The new tool should follow these local +patterns. + +`src/skills/attenuation.rs` contains `READ_ONLY_TOOLS`, the conservative list +of tools available when installed skills lower the visible tool ceiling. +`skill_read_file` belongs there only after the implementation is provably +read-only, skill-scoped, non-networked, and non-mutating. + +`src/tools/builtin/file.rs` and `src/tools/builtin/path_utils.rs` contain +generic file-tool behaviour and path helpers. Treat these as implementation +references, not as a capability to expose. `skill_read_file` must not call the +generic `read_file` tool or return absolute paths to the model. + +The relevant test bases are `src/skills/bundle/tests.rs`, +`src/skills/registry/tests/install.rs`, +`src/skills/registry/tests/discovery.rs`, +`src/skills/registry/tests/prop_tests.rs`, +`src/tools/builtin/skill_tools/tests.rs`, +`src/agent/dispatcher/tests/skill_bundle_context_bdd.rs`, +`tests/channels/skills_upload.rs`, and end-to-end (e2e) +`tests/e2e/scenarios/test_skills.py`. + +The implementation should load these skills before editing: + +- `leta`, for symbol navigation and reference checks. +- `rust-router`, then the smallest relevant Rust follow-on skill. For this + work, expect `rust-types-and-apis` for schema and value types, + `rust-errors` for response/error shape, and `rust-memory-and-state` only if + registry sharing or locks become unclear. +- `hexagonal-architecture`, as boundary discipline: domain policy belongs in + `src/skills/`; built-in tools and web/e2e paths are adapters. +- `domain-web-services`, only if the implementation adds or changes web + gateway behaviour. +- `nextest`, if debugging or filtering `cargo nextest` runs becomes + necessary. +- `commit-message` and `pr-creation`, when committing or updating the pull + request. + +External prior art supports the same narrow shape. Model Context Protocol +(MCP) resources describe file-like context as resources read by URI, with +explicit URI validation and permission checks before reads. OpenAI Apps SDK tool +annotations include `readOnlyHint` for tools that retrieve information without +modifying external state. Axinite does not need to adopt either protocol +wholesale, but these references support an explicit read-only descriptor, +structured input/output schema, and validation before resource access.[^1][^2] + +## Constraints + +- Do not implement raw local filesystem reads, directory listing, globbing, or + arbitrary path access. `skill_read_file` may only resolve against a loaded + skill's canonical private root. +- Do not return absolute host paths in successful or error responses. The + model-facing response may include only the canonical `skill` and + bundle-relative `path`. +- Allow only `SKILL.md`, `references/**`, and `assets/**` as + bundle-relative paths. Reject absolute paths, empty paths, parent directory + traversal, Windows-style drive prefixes, repeated root prefixes, and paths + outside those allowed locations. +- Keep archive validation and install policy in `src/skills/`. The tool + adapter must call shared policy or a domain helper rather than reimplementing + independent rules in `src/tools/`. +- Return text inline only. Phase 1 must not return base64 content or binary + bytes for assets. +- Return deterministic JSON error payloads for unknown skills, unreadable + paths, binary assets, oversized files, invalid UTF-8, and I/O failures. The + native `ToolError` should be reserved for malformed tool parameters or + unexpected adapter failures where no model-facing typed response can be + produced. +- Preserve existing single-file `SKILL.md` installs and active-skill prompt + injection behaviour. +- Avoid new Rust runtime dependencies. `mime_guess`, `serde`, `serde_json`, + `thiserror`, `tokio`, `rstest`, `rstest-bdd`, `insta`, and `proptest` are + already available. +- Use `rstest` fixtures for Rust unit and integration tests. Use + `rstest-bdd` for behavioural Rust coverage where the behaviour is best + expressed as a scenario. Use the existing Python `pytest` e2e style only + when the change affects externally observable server workflows. +- Use `proptest` for path-policy invariants over a range of path inputs. + Do not add Kani or Verus unless the implementation introduces a stronger + invariant that cannot be covered credibly by property tests and review. +- Do not mark roadmap item `1.3.4` done until implementation, tests, + documentation, `coderabbit review --agent`, and final gates pass. + +If satisfying the objective requires violating a constraint, stop, document the +conflict in `Decision Log`, and ask for direction. + +## Tolerances + +- Scope: if implementation needs changes to more than 12 non-test Rust source + files or more than 700 net non-documentation lines, stop and reassess the + boundary design. +- Interface: if the work requires changing public HTTP routes, database + schema, extension WIT contracts, or the generic `read_file` contract, stop + and ask for approval. +- Tool schema: adding one model-callable built-in tool named + `skill_read_file` is in scope. Adding additional tool methods, directory + listings, partial reads, or binary fetch URLs is out of scope unless + explicitly approved. +- Dependencies: if any new Rust runtime dependency appears necessary, stop. + New test-only dependencies require a short decision-log entry explaining why + existing `rstest`, `rstest-bdd`, `proptest`, or `pytest` coverage is + insufficient. +- Security: if a proposed design exposes absolute paths, follows symlinks out + of the skill root, reads through a link target, or falls back to generic + filesystem tools, stop immediately. +- Tests: if targeted tests still fail after three focused fix attempts, stop + and document the failing command, log path, and failure summary. +- Validation time: if any single gate approaches the 1200-second command + limit, stop the current command, capture the log, and split the next run into + smaller documented pieces. +- Ambiguity: if multiple valid interpretations of "assets text files" affect + output shape or allowed media types, choose the conservative interpretation: + UTF-8 text may be returned inline, binary or oversized content returns the + typed non-inline error. + +## Risks + +- Risk: the current `ToolError` type is flat and string-oriented, while RFC + 0003 requires structured skill-scoped errors. + Severity: medium. + Likelihood: high. + Mitigation: return RFC-shaped JSON as a successful `ToolOutput` for expected + domain denials, and use `ToolError::InvalidParameters` only for malformed + tool-call parameters. + +- Risk: symlinks or time-of-check/time-of-use races could escape the skill + root even though archive validation rejects symlinks. + Severity: high. + Likelihood: medium. + Mitigation: validate the lexical bundle path before joining, canonicalize the + final target where practical, reject symlink metadata, and verify the + resolved file remains below the loaded skill root. + +- Risk: adding `skill_read_file` to `READ_ONLY_TOOLS` before the behaviour is + fully constrained would weaken installed-skill attenuation. + Severity: high. + Likelihood: low. + Mitigation: add attenuation only after unit tests prove no writes, network + calls, generic filesystem paths, or state mutation are involved. + +- Risk: Python e2e coverage may need deterministic tool-call plumbing rather + than a direct unit-level invocation. + Severity: medium. + Likelihood: medium. + Mitigation: prefer the existing Python `pytest` e2e style if this becomes + externally observable. If the existing mock model or HTTP route cannot drive + a deterministic tool call, record the blocker and keep the Rust + `rstest-bdd` behavioural coverage as the primary scenario layer. + +- Risk: response-size caps may conflict with existing `SKILL.md` loading caps. + Severity: low. + Likelihood: medium. + Mitigation: define a named read cap in the skill read policy, keep it at or + below the existing prompt-oriented cap unless RFC review dictates otherwise, + and snapshot the error payload for oversized reads. + +## Progress + +- [x] (2026-05-19 00:00+02:00) Loaded `leta`, `rust-router`, + `hexagonal-architecture`, `execplans`, `firecrawl-mcp`, + `en-gb-oxendict-style`, `commit-message`, and `pr-creation` skills relevant + to this planning task. +- [x] (2026-05-19 00:00+02:00) Created a `leta` workspace for this worktree. +- [x] (2026-05-19 00:01+02:00) Confirmed the starting branch was + `feat/skill-read-file-plan`, not the main branch, then renamed it to + `1-3-4-skill-read-file-interface`. +- [x] (2026-05-19 00:02+02:00) Used a Wyvern agent team for read-only + reconnaissance across implementation seams, roadmap/RFC requirements, and + testing strategy. +- [x] (2026-05-19 00:06+02:00) Used Firecrawl to check external prior art for + MCP resources and OpenAI read-only tool annotations. +- [x] (2026-05-19 00:10+02:00) Drafted this pre-implementation ExecPlan. +- [x] (2026-05-19 00:22+02:00) Ran `coderabbit review --agent`, applied the + wording and punctuation findings, and reran it to zero findings. +- [x] (2026-05-20 12:00+02:00) Reopened the planning context on the existing + `1-3-4-skill-read-file-interface` branch, confirmed it tracks + `origin/1-3-4-skill-read-file-interface`, and found the earlier draft PR + `#187` closed. +- [x] (2026-05-20 12:00+02:00) Refreshed external prior-art checks with + Firecrawl against the MCP resources specification and OpenAI Apps SDK + reference. +- [x] (2026-05-20 12:00+02:00) Used a Wyvern agent team for a fresh read-only + planning brief and incorporated its boundary and testing cautions. +- [x] (2026-05-20 12:00+02:00) Corrected stale plan assumptions about Python + `pytest-bdd`; the implementation should use Rust `rstest-bdd` for + behavioural coverage and existing Python `pytest` e2e patterns only where + system-level behaviour changes. +- [x] (2026-05-20 21:46+02:00) Received explicit user instruction to proceed + with implementation under this ExecPlan. +- [x] (2026-05-20 21:46+02:00) Confirmed the working branch is + `1-3-4-skill-read-file-interface`, clean, and tracking + `origin/1-3-4-skill-read-file-interface`. +- [x] (2026-05-20 21:57+02:00) Implemented the first Rust slice: + domain-owned `src/skills/file_read.rs`, `SkillReadFileTool`, built-in tool + registration, attenuation allow-listing, `rstest` unit coverage, + `proptest` path-policy coverage, and `rstest-bdd` behavioural scenarios. +- [x] (2026-05-20 21:57+02:00) Ran targeted validation: + `cargo test --features test-helpers skills::file_read -- --nocapture` + passed 11 tests; `cargo test --features test-helpers + skill_read_file_tool -- --nocapture` passed 2 tests; `cargo test --features + test-helpers skill_read_file_schema -- --nocapture` passed 1 test; and + `cargo test --features test-helpers bdd_model -- --nocapture` passed 2 + `rstest-bdd` scenario tests. +- [x] Implement domain path policy and typed response model. +- [x] Implement and register `skill_read_file`. +- [x] Add unit, property, and behavioural coverage. +- [x] Update documentation and mark roadmap item `1.3.4` done. +- [x] Run `coderabbit review --agent` after each major implementation + milestone and resolve concerns. +- [x] Run final gates. +- [ ] Commit the approved implementation. + +## Surprises & discoveries + +- Observation: `leta workspace add` succeeded, but the first Rust Language + Server Protocol (LSP) query failed because `rust-analyzer` was not installed + for the active toolchain. + Evidence: `leta grep ...` reported that the Rust language server failed to + start and suggested `rustup component add rust-analyzer`. + Impact: install the component and restart the `leta` daemon before relying + on semantic Rust navigation. + +- Observation: after resuming implementation on 2026-05-20, `leta workspace + add` reported the workspace was already present, but `leta show` and + `leta refs` failed with `EOF while parsing a value at line 1 column 0`. + Evidence: direct calls for `LoadedSkillLocation`, + `register_skill_tools`, and `READ_ONLY_TOOLS` all failed with the same + parser error. + Impact: continue with direct source inspection and `rg` for this milestone, + and keep the failure documented so navigation assumptions are not hidden. + +- Observation: `pytest` is used for Python/Playwright e2e tests, but + `pytest-bdd` is not currently in `tests/e2e/pyproject.toml`. + Evidence: the e2e package lists `pytest`, `pytest-asyncio`, + `pytest-playwright`, `pytest-timeout`, `playwright`, `aiohttp`, and `httpx`, + while Rust-side BDD is already present through `rstest-bdd`. + Impact: do not add Python `pytest-bdd` for this roadmap item unless a later + approved design specifically needs it. Use Rust `rstest-bdd` for + behavioural coverage and the existing Python `pytest` style for any + necessary e2e checks. + +- Observation: RFC 0003 asks for stable structured error payloads, but the + native tool error enum is not shaped for domain-denial JSON. + Evidence: `src/tools/tool/traits.rs` and `src/error/tool.rs` expose variants + such as `InvalidParameters` and `ExecutionFailed`, while existing tools + return structured successful `ToolOutput` values for normal results. + Impact: expected skill-scoped denials should be JSON tool results, not + generic execution failures. + +- Observation: `mime_guess` is already a normal dependency. + Evidence: `Cargo.toml` contains `mime_guess = "2.0.5"`. + Impact: media type inference for non-inline asset metadata does not require + a new Rust dependency. + +- Observation: the initial filtered test command + `cargo test --features test-helpers skill_read_file -- --nocapture` matched + no tests because the new tests live under module and scenario names rather + than a single shared test name. + Evidence: the command passed while reporting zero executed tests; the later + module-specific commands executed the intended domain, adapter, schema, and + BDD tests. + Impact: use explicit module filters such as `skills::file_read`, + `skill_read_file_tool`, `skill_read_file_schema`, and `bdd_model` when + rerunning the targeted suite. + +- Observation: CodeRabbit caught four useful implementation concerns during + the first milestone review: incomplete Rustdoc, excessive function + complexity, an over-large inline test module, and a size-cast comment that + needed to match the bounded comparison. + Evidence: `/tmp/coderabbit-skill-read-file-slice-axinite-1-3-4-skill-read-file-interface.out`. + Impact: split policy I/O and validation into submodules, moved tests into + `src/skills/file_read/tests.rs`, completed public docs, and simplified the + size conversion explanation before continuing. + +- Observation: a second CodeRabbit pass identified a replacement race between + lexical validation and file reads. + Evidence: + `/tmp/coderabbit-skill-read-file-slice-rerun-axinite-1-3-4-skill-read-file-interface.out`. + Impact: revalidate the opened file metadata after opening so symlinks, + non-regular files, and oversized files still fail through the + skill-scoped error path even if a bundle file changes between checks. + +- Observation: the CodeRabbit review after the split reported one valid + cleanup finding: an opened file descriptor's metadata cannot report the + path as a symlink, so the post-open symlink check was dead code. + Evidence: + `/tmp/coderabbit-skill-read-file-docs-final-axinite-1-3-4-skill-read-file-interface.out`. + Impact: remove the ineffective `is_symlink()` branch while retaining the + pre-open symlink rejection, post-open regular-file check, and size + revalidation. + +- Observation: the final CodeRabbit pass asked for explicit symlink + regression coverage. + Evidence: + `/tmp/coderabbit-skill-read-file-clean-final-axinite-1-3-4-skill-read-file-interface.out`. + Impact: add a Unix unit test that creates a bundle-relative symlink under + `references/` and verifies `read_skill_file` returns `path_not_readable`. + +- Observation: the follow-up CodeRabbit pass identified missing positive path + policy property coverage, bare-directory cases, unclear PNG fixture bytes, + and a remaining symlink time-of-check/time-of-use gap. + Evidence: + `/tmp/coderabbit-skill-read-file-final-clear-axinite-1-3-4-skill-read-file-interface.out`. + Impact: add positive generated cases for `references/**` and `assets/**`, + assert `SKILL.md` validates, cover bare `references/` and `assets/`, use a + real PNG signature for the binary fixture, and open files with + `O_NOFOLLOW` on Unix before post-open metadata validation. + +- Observation: the next CodeRabbit retry hit a recoverable rate limit. + Evidence: + `/tmp/coderabbit-skill-read-file-final-after-fixes-axinite-1-3-4-skill-read-file-interface.out` + reported `rate_limit` with a suggested wait time of 55 seconds. + Impact: continue with local gates and retry CodeRabbit before committing. + +- Observation: the post-gate CodeRabbit review asked for a clearer audit + trail on the non-Unix fallback, which cannot use Unix `O_NOFOLLOW`. + Evidence: + `/tmp/coderabbit-skill-read-file-after-gates-axinite-1-3-4-skill-read-file-interface.out`. + Impact: document that non-Unix currently relies on the earlier + `symlink_metadata` rejection plus the later opened-file size revalidation, + and leave a TODO for platform-specific atomic no-follow semantics. + +- Observation: the immediate CodeRabbit retry after the non-Unix fallback + comment hit another recoverable rate limit. + Evidence: + `/tmp/coderabbit-skill-read-file-final-reviewed-axinite-1-3-4-skill-read-file-interface.out` + reported `rate_limit` with a suggested wait time of 2 minutes and + 28 seconds. + Impact: wait and retry before committing so the latest CodeRabbit concern + has a clean follow-up result if the service quota permits it. + +- Observation: the waited CodeRabbit retry still hit a recoverable rate + limit, with a longer suggested delay. + Evidence: + `/tmp/coderabbit-skill-read-file-final-retry-axinite-1-3-4-skill-read-file-interface.out` + reported `rate_limit` with a suggested wait time of 5 minutes and + 34 seconds. + Impact: treat CodeRabbit availability as the only remaining external review + constraint; local gates continue to run, and one more retry will be made + before commit. + +- Observation: a final CodeRabbit retry after the clean aggregate gate still + hit the same service-side rate limit. + Evidence: + `/tmp/coderabbit-skill-read-file-final-last-axinite-1-3-4-skill-read-file-interface.out` + reported `rate_limit` with a suggested wait time of 5 minutes and + 36 seconds. + Impact: all previously reported CodeRabbit concerns are fixed, but the + final clean-review confirmation is blocked by CodeRabbit service quota. Do + not treat this as a code concern; include it in the handoff and PR context. + +- Observation: final local validation passed after the last source and + ExecPlan changes. + Evidence: `/tmp/markdownlint-final-axinite-1-3-4-skill-read-file-interface.out` + reported zero Markdown errors; `/tmp/diff-check-final-axinite-1-3-4-skill-read-file-interface.out` + was clean; `/tmp/all-final-axinite-1-3-4-skill-read-file-interface.out` + passed `make all`, including 4091 nextest tests and 5 GitHub tool tests. + Impact: the implementation is locally gated and ready to commit. + +- Observation: the 2026-05-25 review correctly identified that canonicalizing + a target and then reopening it by path was still a time-of-check/time-of-use + gap, including for intermediate symlink components. + Evidence: `src/skills/file_read/io.rs` previously stored a canonical target + path in `CanonicalTarget` and later opened that path in + `read_file_contents`. + Impact: replace the path reopen with a Linux `openat2` call anchored to the + canonical skill-root directory file descriptor and using + `RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS`, then read from that opened handle. + Non-Linux targets now fail closed with a skill-scoped I/O error rather than + using a weaker plain `File::open` fallback. + +- Observation: the same review requested exact maximum-size coverage and + clearer documentation/comment spelling. + Evidence: review comments called out the missing + `MAX_SKILL_READ_FILE_BYTES` boundary case, two Rustdoc comments using + non-Oxford spelling, the stale skill tool registry summary, and first-use + definitions for `e2e` and `LSP`. + Impact: add an exact-size success test, update the Rustdoc and registry + comments, and define end-to-end (e2e) and Language Server Protocol (LSP) in + the ExecPlan. + +- Observation: no Python e2e test was added for this slice. + Evidence: the implementation changes a model-facing built-in tool contract + but does not add a new HTTP route, CLI command, persistence workflow, UI + flow, or network boundary. Rust `rstest-bdd` scenarios exercise the + externally visible tool contract through the built-in tool adapter. + Impact: keep system-level coverage focused on the Rust tool contract and + avoid adding a Python e2e path that would duplicate lower-level assertions + without covering a distinct server workflow. + +- Observation: the requested branch already existed locally and remotely. + Evidence: `git branch --list --verbose --verbose + '*1-3-4-skill-read-file-interface*'` showed a local branch tracking + `origin/1-3-4-skill-read-file-interface`, and `gh pr view` showed the + associated pull request `#187` was closed. + Impact: continue on the existing tracking branch, update the plan in place, + and create a new draft pull request after the refreshed plan is committed and + pushed. + +## Decision log + +- Decision: Write this as a pre-implementation plan only and do not implement + `skill_read_file` until approval. + Rationale: the `execplans` skill requires an explicit approval gate, and the + user specifically stated that the plan must be approved before + implementation. + Date/Author: 2026-05-19 / Codex. + +- Decision: Treat `skill_read_file` as a built-in tool adapter backed by a + domain-owned path policy in `src/skills/`. + Rationale: hexagonal architecture applies here as boundary discipline. The + skill subsystem owns bundle roots and path rules; the built-in tool is only + the driving adapter that accepts JSON and returns JSON. + Date/Author: 2026-05-19 / Codex. + +- Decision: Use structured JSON `ToolOutput` for expected policy denials such + as `path_not_readable`, `non_inline_asset`, and `file_too_large`. + Rationale: RFC 0003 defines model-facing error payloads, and using native + `ToolError` for normal denials would collapse useful error codes into + generic execution failures. + Date/Author: 2026-05-19 / Codex. + +- Decision: Plan both Rust `rstest-bdd` coverage and Python `pytest-bdd` e2e + coverage. + Rationale: this decision was superseded on 2026-05-20 after re-reading the + task. The request names `rstest-bdd`, not Python `pytest-bdd`, and the + repository already has Python e2e tests in plain `pytest`. + Date/Author: 2026-05-19 / Codex. Superseded 2026-05-20 / Codex. + +- Decision: Use Rust `rstest-bdd` for behavioural coverage and keep Python e2e + tests in the existing `pytest` style when system-level coverage is needed. + Rationale: this matches the user request and avoids adding a second BDD + framework to the Python e2e package without a concrete need. + Date/Author: 2026-05-20 / Codex. + +- Decision: Use `proptest` for path policy and decline Kani/Verus unless a + stronger proof obligation emerges during implementation. + Rationale: the new invariant is bounded input validation over path strings + and file classifications. Property tests are proportionate and already used + in nearby skill and dispatcher tests; Kani or Verus would be disproportionate + unless the design introduces a formal business axiom or unsafe code. + Date/Author: 2026-05-19 / Codex. + +- Decision: begin implementation after the 2026-05-20 user approval message + and keep this ExecPlan as the live delivery log. + Rationale: the approval gate is satisfied by the explicit request to + proceed with implementation of this plan. + Date/Author: 2026-05-20 / Codex. + +- Decision: do not add snapshot tests for this change. + Rationale: the stable model-facing JSON shape is asserted directly in unit + and behavioural tests, including success, policy-denial, and unknown-skill + responses. Snapshot files would add review churn without making the + contract more precise for this small response vocabulary. + Date/Author: 2026-05-20 / Codex. + +- Decision: mark roadmap item `1.3.4` done as part of the implementation + branch, before the final commit. + Rationale: the code, user documentation, internal documentation, and feature + parity notes now describe the implemented behaviour; final gates remain as + the commit condition rather than a separate roadmap semantics change. + Date/Author: 2026-05-20 / Codex. + +## Outcomes & retrospective + +Implementation is complete and awaiting final repository gates and commit. +The branch now contains a single read-only `skill_read_file` tool that reads +only loaded skill bundle resources through stable bundle-relative paths, plus +tests and documentation proving the feature works without exposing raw +filesystem access. + +## Plan of work + +Stage A defines the domain policy. Add a small policy module under +`src/skills/`, for example `src/skills/file_read.rs`, and export it from +`src/skills/mod.rs` or a narrow submodule path. The module should define the +request and response vocabulary used by the built-in tool: a validated +bundle-relative path type, success response, expected error response, +non-inline metadata, and a semantic error enum. It should expose a function +with a shape close to: + +```rust +pub async fn read_skill_file( + skill: &LoadedSkill, + requested_path: &str, +) -> SkillReadFileResponse; +``` + +The exact signature may change if the implementation requires injection of +filesystem operations for tests, but the policy must accept a `LoadedSkill` or +equivalent domain object rather than a raw root path from the model. +Validation in this stage should cover allowed paths, rejected paths, unknown +or unsupported path forms, file size limits, binary detection, invalid UTF-8, +and metadata for non-inline responses. Use `proptest` for the lexical path +invariant: generated +absolute paths, traversal segments, alternate separators, and repeated root +prefixes must never produce a resolved path outside the skill root. + +Stage B adds the tool adapter. Add `SkillReadFileTool` alongside the existing +skill tools, either in `src/tools/builtin/skill_tools/read_file.rs` or a +dedicated `src/tools/builtin/skill_read_file.rs` if that keeps the file under +the repository's size guidance. The tool schema should match RFC 0003: +`skill` and `path` are required strings, and `additionalProperties` is false. +The tool should acquire the registry read lock, find the loaded skill by +`manifest.name`, call the domain policy, and return the structured JSON result +through `ToolOutput::success`. Unknown skills should return a deterministic +skill-scoped JSON error rather than leaking the registry layout. + +Stage C registers the tool and tightens attenuation. Export the new tool from +`src/tools/builtin/mod.rs`, register it in +`ToolRegistry::register_skill_tools()` in +`src/tools/registry/builtins.rs`, and update the log message and any registry +tests that assert tool counts. Add `skill_read_file` to +`src/skills/attenuation.rs::READ_ONLY_TOOLS` after tests prove the tool reads +only bundled skill files and remains scoped. If hosted or worker catalogue +tests assert built-in tool +schemas, update those expectations so hosted workers see the same explicit +tool contract. + +Stage D adds behavioural and end-to-end coverage. Extend Rust tests with +`rstest` fixtures and a focused `rstest-bdd` feature that proves an active +skill can reference a bundled file and the model-facing tool reads it by +canonical skill name. Add Python e2e coverage under `tests/e2e/` using the +existing `pytest` style only if the change affects an externally observable +server workflow. Use the mock LLM or direct HTTP/tool invocation path, +whichever is the smallest deterministic system-level route. + +Stage E updates documentation. Update `docs/users-guide.md` with the new user +behaviour, including request and response examples and phase-1 limitations. +Update `docs/agent-skills-support.md` with the internal lifecycle and +maintainer-facing boundary rules. Update `docs/developers-guide.md` with any +new conventions for skill-file policy or e2e BDD tests. Update +`docs/axinite-architecture-overview.md` if the skills row needs to mention the +new read path. Check `FEATURE_PARITY.md` for feature status changes. Mark +`docs/roadmap.md` item `1.3.4` done only after implementation and validation +are complete. + +Stage F performs final review, validation, and commit. Run +`coderabbit review --agent`, resolve concerns, run the final gates through +`tee`, inspect logs, run Markdown lint for changed Markdown files, run +`git diff --check`, and commit with a file-based commit message. + +## Concrete steps + +All commands run from the repository root: + +```bash +cd /home/leynos/.lody/repos/github---leynos---axinite/worktrees/6c92fc0f-e404-4a10-ace1-30a36066de50 +``` + +Before editing, confirm the branch and workspace: + +```bash +git branch --show-current +leta workspace add "$PWD" +``` + +Expected branch output: + +```plaintext +1-3-4-skill-read-file-interface +``` + +Use `leta` for symbol navigation before editing Rust: + +```bash +leta show LoadedSkillLocation +leta show LoadedSkill.skill_root +leta show 'src/tools/registry/builtins.rs:register_skill_tools' +leta refs READ_ONLY_TOOLS +``` + +Add red tests for Stage A and Stage B first. The initial targeted test command +should fail before implementation: + +```bash +ACTION=skill-read-file-red +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +cargo test --features test-helpers skill_read_file -- --nocapture 2>&1 | tee "$LOG" +``` + +After implementing the domain policy and tool adapter, rerun targeted tests: + +```bash +ACTION=skill-read-file-targeted +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +cargo test --features test-helpers skill_read_file -- --nocapture 2>&1 | tee "$LOG" +``` + +Run the Rust behavioural tests: + +```bash +ACTION=skill-read-file-bdd +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +cargo test --features test-helpers skill_bundle -- --nocapture 2>&1 | tee "$LOG" +``` + +Run Python e2e coverage from the repository root after installing the e2e +package in the active Python environment, if necessary: + +```bash +ACTION=skill-read-file-e2e +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +pytest tests/e2e/scenarios/test_skills.py -v --timeout=120 2>&1 | tee "$LOG" +``` + +Run `coderabbit review --agent` after major milestones: + +```bash +ACTION=coderabbit-skill-read-file +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +coderabbit review --agent 2>&1 | tee "$LOG" +``` + +Run final gates sequentially: + +```bash +ACTION=check-fmt +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +make check-fmt 2>&1 | tee "$LOG" + +ACTION=lint +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +make lint 2>&1 | tee "$LOG" + +ACTION=test +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +make test 2>&1 | tee "$LOG" +``` + +If repository policy requires the aggregate target in addition to the explicit +subtargets, run: + +```bash +ACTION=all +LOG="/tmp/${ACTION}-axinite-$(git branch --show-current).out" +make all 2>&1 | tee "$LOG" +``` + +Run Markdown and whitespace checks after documentation edits: + +```bash +bunx markdownlint-cli2 docs/execplans/1-3-4-skill-read-file-interface.md \ + docs/users-guide.md docs/agent-skills-support.md docs/developers-guide.md \ + docs/axinite-architecture-overview.md docs/roadmap.md +git diff --check +``` + +Commit only after the gates pass, using the file-based commit-message workflow +from the `commit-message` skill. + +## Validation and acceptance + +The feature is accepted when all of the following are true: + +- Calling `skill_read_file` with a loaded skill name and `SKILL.md` returns a + JSON object containing the same `skill`, the normalized bundle-relative + `path`, a text `mime_type`, and inline `content`. +- Calling `skill_read_file` with `references/usage.md` from an installed + bundle returns the file content without exposing an absolute path. +- Calling `skill_read_file` with absolute paths, `..`, paths outside + `SKILL.md`, `references/**`, or `assets/**`, unknown skill names, and missing + files returns deterministic skill-scoped JSON errors. +- Calling `skill_read_file` for binary assets returns `error.code` equal to + `non_inline_asset` with `metadata.size`, `metadata.mime_type`, and a stable + `metadata.fetch_hint`. +- Calling `skill_read_file` for oversized text returns `error.code` equal to + `file_too_large` with the same metadata fields. +- Installed-skill attenuation allows `skill_read_file` while still blocking + generic `read_file`. +- Snapshot tests cover any stable model-facing output whose formatting matters. +- Property tests prove path validation cannot resolve outside the loaded skill + root over generated path inputs. +- Python e2e coverage with the existing `pytest` harness proves the externally + observable read flow or records a documented blocker, if the final + implementation affects a system-level workflow. +- Documentation explains current user-visible and maintainer-visible + behaviour. +- `coderabbit review --agent` has no unresolved blocking concerns. +- `make check-fmt`, `make lint`, and `make test` succeed. If `make all` is run + as the aggregate gate, it succeeds too. + +Quality criteria: + +- Tests: targeted Rust tests, Rust behavioural tests, property tests, and + relevant Python e2e tests pass. +- Lint/typecheck: clippy and formatting gates pass through the Makefile + targets. +- Security: no raw filesystem path is exposed to the model, no generic file + read is reused as the model-facing capability, and symlink/path traversal + escapes are rejected. +- Documentation: changed Markdown passes markdownlint and `git diff --check`. + +## Idempotence and recovery + +The implementation steps are additive and can be repeated. If a targeted test +fails, inspect the corresponding `/tmp/*-axinite-1-3-4-skill-read-file-interface.out` +log before rerunning. If a staged design begins to duplicate path policy in the +tool adapter, stop and move that logic back into `src/skills/` before +continuing. + +If `coderabbit review --agent` reports concerns, fix them in the smallest +logical commit or record why the concern is not applicable in `Decision Log`. +Do not proceed to the next milestone with unresolved blocking concerns. + +If Python e2e setup cannot drive the new tool deterministically, preserve the +Rust `rstest-bdd` behavioural coverage, document the blocker in this plan, and +ask for approval before dropping system-level e2e coverage. + +If the implementation has to be rolled back before commit, use ordinary Git +diff review and reverse patches for the files changed by this task. Do not use +`git reset --hard` or checkout commands that would discard unrelated user +work. + +## Interfaces and dependencies + +The model-facing tool name is: + +```plaintext +skill_read_file +``` + +The input schema is: + +```json +{ + "type": "object", + "properties": { + "skill": { + "type": "string", + "description": "Installed skill name exactly as advertised to the model." + }, + "path": { + "type": "string", + "description": "Bundle-relative path, such as SKILL.md or references/usage.md." + } + }, + "required": ["skill", "path"], + "additionalProperties": false +} +``` + +A successful text response is: + +```json +{ + "skill": "deploy-docs", + "path": "references/usage.md", + "mime_type": "text/markdown", + "content": "# Usage\n..." +} +``` + +An expected denial response is: + +```json +{ + "skill": "deploy-docs", + "path": "assets/logo.png", + "error": { + "code": "non_inline_asset", + "message": "Phase 1 does not return binary or oversized assets inline.", + "metadata": { + "size": 18231, + "mime_type": "image/png", + "fetch_hint": "Treat this as a passive asset; request only referenced text files in phase 1." + } + } +} +``` + +Expected error codes are: + +- `unknown_skill` +- `path_not_readable` +- `non_inline_asset` +- `file_too_large` +- `invalid_utf8` +- `io_error` + +The final code should expose these concepts through named Rust types rather +than open-coded JSON fragments wherever practical. The adapter may serialize +the final response with `serde_json::json!` only at the boundary. + +## Artifacts and notes + +Firecrawl research used during planning: + +- MCP resources are described as context-bearing data such as files or + application-specific information. The read operation is URI-based, and the + specification calls out URI validation, permission checks, and proper + handling of binary data.[^1] +- OpenAI Apps SDK documentation identifies `readOnlyHint` as the tool + annotation for tools that retrieve information without modifying external + data and points implementers towards explicit input and output schemas.[^2] + +Wyvern agent-team findings used during planning: + +- The implementation seam is `LoadedSkillLocation` plus + `ToolRegistry::register_skill_tools()`, with a new built-in tool and an + update to `READ_ONLY_TOOLS`. +- Existing tests already cover bundle validation, install lifecycle, active + skill context snapshots, `rstest-bdd`, and path-property testing. +- Python e2e tests use `pytest`; Python `pytest-bdd` is not yet installed, + while Rust `rstest-bdd` is already present. + +[^1]: Model Context Protocol, "Resources", + . +[^2]: OpenAI Developers, "Reference - Apps SDK", + . + +## Revision note + +2026-05-19: Created the initial draft for approval. The draft captures the +planned domain boundary, tool schema, validation strategy, documentation sync, +quality gates, and approval requirement for roadmap item `1.3.4`. + +2026-05-19: Applied CodeRabbit wording and punctuation findings and reran +`coderabbit review --agent` to zero findings. This does not change the planned +implementation approach. + +2026-05-20: Refreshed the draft after discovering the requested branch already +existed and its previous draft pull request was closed. Corrected the concrete +worktree path and narrowed behavioural-test guidance to Rust `rstest-bdd` plus +existing Python `pytest` e2e patterns where applicable. + +2026-05-25: Applied follow-up review fixes for the file-read hardening path. +The implementation now opens the bundle root as a stable directory handle +before calling Linux `openat2`, caps inline reads at +`MAX_SKILL_READ_FILE_BYTES + 1`, and makes Linux-only behavioural tests +explicit while non-Linux allowed reads fail closed with `io_error`. + +2026-05-25: Verified follow-up overall comments. The per-read canonicalization +comment was stale because `open_validated_target` no longer canonicalizes the +root. The non-Linux diagnostic comment remained valid, so the fail-closed +branch now emits a warning before returning `io_error`. diff --git a/docs/roadmap.md b/docs/roadmap.md index a79a53916..ee1cece99 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -177,7 +177,7 @@ packaging. - Success: runtime state records the installed skill root and `SKILL.md` entrypoint, and active-skill injection can refer to a stable bundle-relative file layout. -- [ ] 1.3.4. Add a read-only `skill_read_file` interface for bundled resources. +- [x] 1.3.4. Add a read-only `skill_read_file` interface for bundled resources. Requires 1.3.2 and 1.3.3. - See [RFC 0003 §Problem](./rfcs/0003-skill-bundle-installation.md#problem) and diff --git a/docs/users-guide.md b/docs/users-guide.md index f5269dfd2..1b0866aad 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -33,10 +33,8 @@ such as sending both a name and a URL, fail before any download or install attempt. Malformed archives report explicit `invalid_skill_bundle: ...` errors that describe the archive-shape problem. -This slice does not yet add runtime file reads from bundled references and -assets. When a bundled skill is active, the runtime still injects only the -selected `SKILL.md` body into the prompt. The runtime now records the -installed skill root and `SKILL.md` entrypoint internally. The model-facing +When a bundled skill is active, the runtime injects the selected `SKILL.md` +body into the prompt and advertises stable bundle metadata. The model-facing active-skill block includes this required metadata contract: - `skill`: required string. The stable skill identifier from the loaded skill @@ -50,7 +48,38 @@ active-skill block includes this required metadata contract: `bundle`. Host-local filesystem paths are not exposed as model instructions. A dedicated -`skill_read_file` tool is planned for a later release. +`skill_read_file` tool lets the model read allowed bundle-relative files +without access to the generic filesystem tools. + +The `skill_read_file` input schema requires: + +```json +{ + "skill": "deploy-docs", + "path": "references/usage.md" +} +``` + +The `skill` value must match the active skill identifier. The `path` value must +be one of `SKILL.md`, a file below `references/`, or a file below `assets/`. +Absolute paths, `..`, backslashes, nested `SKILL.md` files, and other roots are +rejected. + +A successful text read returns inline UTF-8 content: + +```json +{ + "skill": "deploy-docs", + "path": "references/usage.md", + "mime_type": "text/markdown", + "content": "# Usage\n..." +} +``` + +Binary assets and oversized files are not returned inline in this phase. They +return a typed error with size, media type, and a stable fetch hint. Unknown +skills, missing files, disallowed paths, invalid UTF-8, and I/O failures also +return skill-scoped JSON error payloads rather than host-local paths. ## Hosted workers and remote tools diff --git a/src/skills/attenuation.rs b/src/skills/attenuation.rs index 2a4a0e997..d91b7d969 100644 --- a/src/skills/attenuation.rs +++ b/src/skills/attenuation.rs @@ -32,6 +32,7 @@ const READ_ONLY_TOOLS: &[&str] = &[ "json", "skill_list", "skill_search", + "skill_read_file", ]; /// Result of tool attenuation, including transparency information. @@ -146,6 +147,7 @@ mod tests { make_tool("time"), make_tool("echo"), make_tool("json"), + make_tool("skill_read_file"), ] } @@ -182,6 +184,7 @@ mod tests { assert!(!kept_names.contains(&"memory_write")); assert!(kept_names.contains(&"memory_search")); assert!(kept_names.contains(&"memory_read")); + assert!(kept_names.contains(&"skill_read_file")); assert!(kept_names.contains(&"time")); assert_eq!(result.min_trust, SkillTrust::Installed); } diff --git a/src/skills/file_read.rs b/src/skills/file_read.rs new file mode 100644 index 000000000..8ca0a6098 --- /dev/null +++ b/src/skills/file_read.rs @@ -0,0 +1,187 @@ +//! Read-only, skill-scoped access to bundled skill files. +//! +//! This module owns the runtime policy for `skill_read_file`: callers provide a +//! loaded skill and a bundle-relative path, and the policy either returns UTF-8 +//! text inline or a deterministic model-facing denial payload. + +use serde::Serialize; + +use crate::skills::LoadedSkill; + +mod io; +mod validation; + +use io::read_validated_skill_file; +use validation::{display_path, validate_bundle_relative_path}; + +/// Maximum file size, in bytes, returned inline by `skill_read_file`. +pub const MAX_SKILL_READ_FILE_BYTES: u64 = crate::skills::MAX_PROMPT_FILE_SIZE; + +const NON_INLINE_FETCH_HINT: &str = + "Treat this as a passive asset; request only referenced text files in phase 1."; + +/// Complete model-facing response for a skill file read. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(untagged)] +pub enum SkillReadFileResponse { + /// UTF-8 text returned inline. + Success(SkillReadFileSuccess), + /// Expected skill-scoped denial or read failure. + Error(SkillReadFileErrorResponse), +} + +impl SkillReadFileResponse { + /// Deterministic error payload for a skill that is not loaded. + pub fn unknown_skill(skill: &str, path: &str) -> Self { + Self::error( + skill, + path, + SkillReadFileError::new( + SkillReadFileErrorCode::UnknownSkill, + "Skill is not loaded or available for reading", + ), + ) + } + + fn error(skill: &str, path: &str, error: SkillReadFileError) -> Self { + Self::Error(SkillReadFileErrorResponse { + skill: skill.to_string(), + path: path.to_string(), + error, + }) + } +} + +/// Successful inline text response. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct SkillReadFileSuccess { + /// Canonical skill identifier used for the read. + pub skill: String, + /// Normalized bundle-relative path that was read. + pub path: String, + /// Best-effort media type inferred from the bundle-relative path. + pub mime_type: String, + /// UTF-8 file content returned inline. + pub content: String, +} + +/// Expected error response. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct SkillReadFileErrorResponse { + /// Canonical skill identifier, or the requested identifier for unknown skills. + pub skill: String, + /// Requested or normalized bundle-relative path. + pub path: String, + /// Stable, model-facing error payload. + pub error: SkillReadFileError, +} + +/// Stable error payload. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct SkillReadFileError { + /// Machine-readable reason for the denial or failure. + pub code: SkillReadFileErrorCode, + /// Short human-readable explanation for the model. + pub message: String, + /// Size, media type, and fetch guidance for non-inline files. + #[serde(skip_serializing_if = "Option::is_none")] + pub metadata: Option, +} + +impl SkillReadFileError { + fn new(code: SkillReadFileErrorCode, message: impl Into) -> Self { + Self { + code, + message: message.into(), + metadata: None, + } + } + + fn with_metadata( + code: SkillReadFileErrorCode, + message: impl Into, + metadata: SkillReadFileMetadata, + ) -> Self { + Self { + code, + message: message.into(), + metadata: Some(metadata), + } + } +} + +/// Stable model-facing error codes. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum SkillReadFileErrorCode { + /// The requested skill is not currently loaded. + UnknownSkill, + /// The requested path is missing, disallowed, non-regular, or outside the skill root. + PathNotReadable, + /// The requested asset is binary and cannot be returned inline in phase 1. + NonInlineAsset, + /// The requested file is larger than [`MAX_SKILL_READ_FILE_BYTES`]. + FileTooLarge, + /// The requested non-asset file is not valid UTF-8. + InvalidUtf8, + /// Filesystem I/O failed while resolving or reading the scoped file. + IoError, +} + +/// Metadata for files that cannot be returned inline in phase 1. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct SkillReadFileMetadata { + /// File size in bytes. + pub size: u64, + /// Best-effort media type inferred from the bundle-relative path. + pub mime_type: String, + /// Stable instruction for how the model should treat non-inline content. + pub fetch_hint: String, +} + +/// Read a bundle-relative file under the loaded skill's private root. +/// +/// The caller supplies an already loaded skill and a path such as `SKILL.md`, +/// `references/usage.md`, or `assets/note.txt`. The path is validated before +/// any filesystem access: absolute paths, traversal, backslashes, unsupported +/// roots, and nested `SKILL.md` files are rejected. Successful reads return +/// inline UTF-8 text; expected denials return a typed +/// [`SkillReadFileResponse::Error`] payload instead of leaking host paths. +/// +/// # Example +/// +/// ```no_run +/// # async fn example(skill: &ironclaw::skills::LoadedSkill) { +/// use ironclaw::skills::file_read::{SkillReadFileResponse, read_skill_file}; +/// +/// match read_skill_file(skill, "references/usage.md").await { +/// SkillReadFileResponse::Success(file) => { +/// println!("{}", file.content); +/// } +/// SkillReadFileResponse::Error(error) => { +/// eprintln!("{}: {}", error.path, error.error.message); +/// } +/// } +/// # } +/// ``` +pub async fn read_skill_file(skill: &LoadedSkill, requested_path: &str) -> SkillReadFileResponse { + let display_skill = skill.skill_identifier(); + let display_path = display_path(requested_path); + let relative_path = match validate_bundle_relative_path(requested_path) { + Ok(path) => path, + Err(error) => { + tracing::debug!( + skill_id = %display_skill, + path = %display_path, + error_code = ?error.code, + "skill_read_file: path validation rejected" + ); + return SkillReadFileResponse::error(display_skill, &display_path, error); + } + }; + + read_validated_skill_file(skill, &relative_path).await +} + +#[cfg(test)] +mod tests; diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs new file mode 100644 index 000000000..1a4209be3 --- /dev/null +++ b/src/skills/file_read/io.rs @@ -0,0 +1,297 @@ +//! Filesystem resolution and inline decoding for scoped skill file reads. + +use std::path::Path; + +use tokio::io::{AsyncReadExt, Take}; + +use super::validation::is_asset_path; +#[cfg(target_os = "linux")] +use super::validation::path_not_readable; +use super::{ + MAX_SKILL_READ_FILE_BYTES, NON_INLINE_FETCH_HINT, SkillReadFileError, SkillReadFileErrorCode, + SkillReadFileMetadata, SkillReadFileResponse, SkillReadFileSuccess, +}; +use crate::skills::LoadedSkill; + +struct ReadDisplay<'a> { + skill: &'a str, + path: String, +} + +struct OpenedTarget { + file: tokio::fs::File, + size: u64, +} + +pub(super) async fn read_validated_skill_file( + skill: &LoadedSkill, + relative_path: &Path, +) -> SkillReadFileResponse { + let display = ReadDisplay { + skill: skill.skill_identifier(), + path: relative_path.to_string_lossy().replace('\\', "/"), + }; + + let target = match open_validated_target(skill.skill_root(), relative_path).await { + Ok(target) => target, + Err(error) => return error_response(&display, error), + }; + let mime_type = mime_type_for(relative_path); + let content = match read_inline_content(target, relative_path, mime_type.clone()).await { + Ok(content) => content, + Err(error) => return error_response(&display, error), + }; + + SkillReadFileResponse::Success(SkillReadFileSuccess { + skill: display.skill.to_string(), + path: display.path, + mime_type, + content, + }) +} + +async fn open_validated_target( + root: &Path, + relative_path: &Path, +) -> Result { + open_relative_to_root(root, relative_path).await +} + +async fn read_inline_content( + target: OpenedTarget, + relative_path: &Path, + mime_type: String, +) -> Result { + if target.size > MAX_SKILL_READ_FILE_BYTES { + tracing::debug!( + size = target.size, + limit = MAX_SKILL_READ_FILE_BYTES, + "skill_read_file: file exceeds size cap (metadata)" + ); + return Err(non_inline_error( + SkillReadFileErrorCode::FileTooLarge, + target.size, + mime_type, + )); + } + + let size = target.size; + let bytes = read_file_contents(target).await?; + let actual_size = bytes.len() as u64; + if actual_size > MAX_SKILL_READ_FILE_BYTES { + tracing::debug!( + size = actual_size, + limit = MAX_SKILL_READ_FILE_BYTES, + "skill_read_file: file exceeds size cap (post-read)" + ); + return Err(non_inline_error( + SkillReadFileErrorCode::FileTooLarge, + actual_size, + mime_type, + )); + } + parse_utf8_content(bytes, relative_path, size, mime_type) +} + +async fn read_file_contents(target: OpenedTarget) -> Result, SkillReadFileError> { + let OpenedTarget { mut file, size } = target; + // The cast is safe because the size is first capped at the read limit, + // which fits in usize on supported platforms. + let read_limit = MAX_SKILL_READ_FILE_BYTES + 1; + let mut contents = Vec::with_capacity(size.min(read_limit) as usize); + limited_reader(&mut file, read_limit) + .read_to_end(&mut contents) + .await + .map_err(|_| io_error("File is not available for reading"))?; + Ok(contents) +} + +#[cfg(target_os = "linux")] +async fn metadata_for_opened_file(file: &tokio::fs::File) -> Result { + let metadata = file + .metadata() + .await + .map_err(|_| io_error("File is not available for reading"))?; + if !metadata.is_file() { + return Err(path_not_readable()); + } + Ok(metadata.len()) +} + +fn limited_reader(file: &mut tokio::fs::File, limit: u64) -> Take<&mut tokio::fs::File> { + file.take(limit) +} + +#[cfg(target_os = "linux")] +async fn open_relative_to_root( + root: &Path, + relative_path: &Path, +) -> Result { + use std::ffi::CString; + use std::os::fd::{FromRawFd, OwnedFd}; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::io::AsRawFd; + + let root = open_root_directory(root).await?; + if !root + .metadata() + .map_err(|_| io_error("Skill root is not readable"))? + .is_dir() + { + return Err(path_not_readable()); + } + + let path = + CString::new(relative_path.as_os_str().as_bytes()).map_err(|_| path_not_readable())?; + // SAFETY: Linux requires `open_how` to be zero-initialized so unknown + // future fields default to zero before the supported fields are set. + let mut how: libc::open_how = unsafe { std::mem::zeroed() }; + how.flags = (libc::O_RDONLY | libc::O_CLOEXEC) as u64; + how.mode = 0; + how.resolve = libc::RESOLVE_BENEATH | libc::RESOLVE_NO_SYMLINKS; + + // SAFETY: `root` is a live directory file descriptor, `path` is a + // NUL-terminated relative path owned by this stack frame, and `how` points + // to an initialized `open_how` with its correct size. `openat2` returns a + // new file descriptor on success, which is immediately wrapped in + // `OwnedFd` to transfer ownership and ensure it is closed. + let fd = unsafe { + libc::syscall( + libc::SYS_openat2, + root.as_raw_fd(), + path.as_ptr(), + &how, + std::mem::size_of::(), + ) + }; + + if fd < 0 { + let error = std::io::Error::last_os_error(); + return Err(openat2_error(error)); + } + + // SAFETY: `fd` was returned by a successful `openat2` call above and has + // not been transferred elsewhere. + let file = std::fs::File::from(unsafe { OwnedFd::from_raw_fd(fd as libc::c_int) }); + let file = tokio::fs::File::from_std(file); + let size = metadata_for_opened_file(&file).await?; + + Ok(OpenedTarget { file, size }) +} + +#[cfg(target_os = "linux")] +async fn open_root_directory(root: &Path) -> Result { + use std::os::unix::fs::OpenOptionsExt; + + let root = root.to_path_buf(); + tokio::task::spawn_blocking(move || { + std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_CLOEXEC | libc::O_DIRECTORY | libc::O_NOFOLLOW) + .open(root) + }) + .await + .map_err(|_| io_error("Skill root is not readable"))? + .map_err(|_| io_error("Skill root is not readable")) +} + +#[cfg(not(target_os = "linux"))] +async fn open_relative_to_root( + _root: &Path, + _relative_path: &Path, +) -> Result { + tracing::warn!( + "skill_read_file is unavailable on this platform because atomic symlink-safe reads require Linux openat2" + ); + Err(io_error( + "Atomic skill file reads are not supported on this platform", + )) +} + +#[cfg(target_os = "linux")] +fn openat2_error(error: std::io::Error) -> SkillReadFileError { + match error.raw_os_error() { + Some(libc::ENOENT | libc::ENOTDIR | libc::ELOOP | libc::EXDEV | libc::EAGAIN) => { + tracing::debug!(os_error = ?error, "skill_read_file: openat2 path not readable"); + path_not_readable() + } + Some(libc::ENOSYS | libc::EINVAL) => { + tracing::warn!(os_error = ?error, "skill_read_file: openat2 unsupported on this kernel"); + io_error("Atomic skill file reads are not supported") + } + _ => { + tracing::debug!(os_error = ?error, "skill_read_file: openat2 I/O error"); + io_error("File is not available for reading") + } + } +} + +fn parse_utf8_content( + bytes: Vec, + relative_path: &Path, + size: u64, + mime_type: String, +) -> Result { + match String::from_utf8(bytes) { + Ok(content) => Ok(content), + Err(_) if is_asset_path(relative_path) => Err(non_inline_error( + SkillReadFileErrorCode::NonInlineAsset, + size, + mime_type, + )), + Err(_) => { + tracing::debug!( + error_code = ?SkillReadFileErrorCode::InvalidUtf8, + path = %relative_path.display(), + "skill_read_file: file is not valid UTF-8" + ); + Err(SkillReadFileError::new( + SkillReadFileErrorCode::InvalidUtf8, + "File is not valid UTF-8 text", + )) + } + } +} + +fn error_response(display: &ReadDisplay<'_>, error: SkillReadFileError) -> SkillReadFileResponse { + let skill_id = display.skill; + let path = &display.path; + tracing::debug!( + skill_id = %skill_id, + path = %path, + error_code = ?error.code, + "skill_read_file: read failed" + ); + SkillReadFileResponse::error(display.skill, &display.path, error) +} + +fn io_error(message: impl Into) -> SkillReadFileError { + SkillReadFileError::new(SkillReadFileErrorCode::IoError, message) +} + +fn non_inline_error( + code: SkillReadFileErrorCode, + size: u64, + mime_type: String, +) -> SkillReadFileError { + SkillReadFileError::with_metadata( + code, + "Phase 1 does not return binary or oversized assets inline.", + metadata_payload(size, mime_type), + ) +} + +fn metadata_payload(size: u64, mime_type: String) -> SkillReadFileMetadata { + SkillReadFileMetadata { + size, + mime_type, + fetch_hint: NON_INLINE_FETCH_HINT.to_string(), + } +} + +fn mime_type_for(path: &Path) -> String { + mime_guess::from_path(path) + .first_raw() + .unwrap_or("text/plain") + .to_string() +} diff --git a/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_file_too_large.snap b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_file_too_large.snap new file mode 100644 index 000000000..3f9d6e6e3 --- /dev/null +++ b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_file_too_large.snap @@ -0,0 +1,17 @@ +--- +source: src/skills/file_read/tests.rs +expression: "&response" +--- +{ + "skill": "deploy-docs", + "path": "references/large.md", + "error": { + "code": "file_too_large", + "message": "Phase 1 does not return binary or oversized assets inline.", + "metadata": { + "size": 65537, + "mime_type": "text/markdown", + "fetch_hint": "Treat this as a passive asset; request only referenced text files in phase 1." + } + } +} diff --git a/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_invalid_utf8.snap b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_invalid_utf8.snap new file mode 100644 index 000000000..95a20d142 --- /dev/null +++ b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_invalid_utf8.snap @@ -0,0 +1,12 @@ +--- +source: src/skills/file_read/tests.rs +expression: "&response" +--- +{ + "skill": "deploy-docs", + "path": "references/binary.md", + "error": { + "code": "invalid_utf8", + "message": "File is not valid UTF-8 text" + } +} diff --git a/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_io_error.snap b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_io_error.snap new file mode 100644 index 000000000..408d9bbf8 --- /dev/null +++ b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_io_error.snap @@ -0,0 +1,12 @@ +--- +source: src/skills/file_read/tests.rs +expression: "&response" +--- +{ + "skill": "deploy-docs", + "path": "references/usage.md", + "error": { + "code": "io_error", + "message": "File is not available for reading" + } +} diff --git a/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_non_inline_asset.snap b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_non_inline_asset.snap new file mode 100644 index 000000000..18ed2ec21 --- /dev/null +++ b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_non_inline_asset.snap @@ -0,0 +1,17 @@ +--- +source: src/skills/file_read/tests.rs +expression: "&response" +--- +{ + "skill": "deploy-docs", + "path": "assets/logo.png", + "error": { + "code": "non_inline_asset", + "message": "Phase 1 does not return binary or oversized assets inline.", + "metadata": { + "size": 8, + "mime_type": "image/png", + "fetch_hint": "Treat this as a passive asset; request only referenced text files in phase 1." + } + } +} diff --git a/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_path_not_readable.snap b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_path_not_readable.snap new file mode 100644 index 000000000..7dda9a076 --- /dev/null +++ b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_path_not_readable.snap @@ -0,0 +1,12 @@ +--- +source: src/skills/file_read/tests.rs +expression: "&response" +--- +{ + "skill": "deploy-docs", + "path": "../secret", + "error": { + "code": "path_not_readable", + "message": "File is not available for reading" + } +} diff --git a/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_unknown_skill.snap b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_unknown_skill.snap new file mode 100644 index 000000000..8dd41a48a --- /dev/null +++ b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_unknown_skill.snap @@ -0,0 +1,12 @@ +--- +source: src/skills/file_read/tests.rs +expression: "&response" +--- +{ + "skill": "deploy-docs", + "path": "references/usage.md", + "error": { + "code": "unknown_skill", + "message": "Skill is not loaded or available for reading" + } +} diff --git a/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_success.snap b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_success.snap new file mode 100644 index 000000000..d525c0428 --- /dev/null +++ b/src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_success.snap @@ -0,0 +1,10 @@ +--- +source: src/skills/file_read/tests.rs +expression: "&response" +--- +{ + "skill": "deploy-docs", + "path": "references/usage.md", + "mime_type": "text/markdown", + "content": "# Usage\n" +} diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs new file mode 100644 index 000000000..de1eaf1a4 --- /dev/null +++ b/src/skills/file_read/tests.rs @@ -0,0 +1,455 @@ +//! Unit and property tests for skill bundle file read operations. + +use std::path::PathBuf; + +use insta::assert_json_snapshot; +use proptest::prelude::*; +use rstest::{fixture, rstest}; +use tempfile::TempDir; + +use super::*; +use crate::skills::test_support::TestSkillBuilder; +use crate::skills::{LoadedSkillLocation, SkillPackageKind}; + +struct SkillReadFixture { + _dir: TempDir, + skill: LoadedSkill, +} + +#[fixture] +fn skill_read_fixture() -> SkillReadFixture { + let dir = tempfile::tempdir().expect("tempdir should be created"); + std::fs::create_dir_all(dir.path().join("references")) + .expect("references dir should be created"); + std::fs::create_dir_all(dir.path().join("assets")).expect("assets dir should be created"); + std::fs::write(dir.path().join("SKILL.md"), "# Deploy docs\n") + .expect("skill prompt should be written"); + std::fs::write(dir.path().join("references/usage.md"), "# Usage\n") + .expect("reference should be written"); + std::fs::write(dir.path().join("assets/note.txt"), "asset notes\n") + .expect("text asset should be written"); + std::fs::write( + dir.path().join("assets/logo.png"), + [0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A], + ) + .expect("binary asset should be written"); + + let location = LoadedSkillLocation::new( + "deploy-docs", + dir.path(), + PathBuf::from("SKILL.md"), + SkillPackageKind::Bundle, + ) + .expect("test location should be valid"); + let skill = TestSkillBuilder::new("deploy-docs") + .location(location) + .build(); + + SkillReadFixture { _dir: dir, skill } +} + +#[rstest] +#[tokio::test] +#[cfg(target_os = "linux")] +async fn reads_bundle_reference_text(skill_read_fixture: SkillReadFixture) { + let response = read_skill_file(&skill_read_fixture.skill, "references/usage.md").await; + + assert_eq!( + response, + SkillReadFileResponse::Success(SkillReadFileSuccess { + skill: "deploy-docs".to_string(), + path: "references/usage.md".to_string(), + mime_type: "text/markdown".to_string(), + content: "# Usage\n".to_string(), + }) + ); +} + +#[rstest] +#[tokio::test] +#[cfg(target_os = "linux")] +async fn reads_bundle_reference_text_at_max_size(skill_read_fixture: SkillReadFixture) { + std::fs::write( + skill_read_fixture + .skill + .skill_root() + .join("references/max-size.md"), + "x".repeat(MAX_SKILL_READ_FILE_BYTES as usize), + ) + .expect("max-size reference should be written"); + + let response = read_skill_file(&skill_read_fixture.skill, "references/max-size.md").await; + + let SkillReadFileResponse::Success(success) = response else { + panic!("max-size reference should be returned inline"); + }; + assert_eq!(success.skill, "deploy-docs"); + assert_eq!(success.path, "references/max-size.md"); + assert_eq!(success.mime_type, "text/markdown"); + assert_eq!(success.content.len(), MAX_SKILL_READ_FILE_BYTES as usize); +} + +#[rstest] +#[tokio::test] +#[cfg(target_os = "linux")] +async fn reads_skill_entrypoint(skill_read_fixture: SkillReadFixture) { + let response = read_skill_file(&skill_read_fixture.skill, "SKILL.md").await; + + assert_eq!( + response, + SkillReadFileResponse::Success(SkillReadFileSuccess { + skill: "deploy-docs".to_string(), + path: "SKILL.md".to_string(), + mime_type: "text/markdown".to_string(), + content: "# Deploy docs\n".to_string(), + }) + ); +} + +#[rstest] +#[case::absolute("/etc/passwd")] +#[case::traversal("../secret")] +#[case::nested_entrypoint("references/SKILL.md")] +#[case::unsupported_root("scripts/install.sh")] +#[case::windows_separator("references\\usage.md")] +#[case::bare_references_directory("references/")] +#[case::bare_assets_directory("assets/")] +#[tokio::test] +async fn rejects_disallowed_paths(skill_read_fixture: SkillReadFixture, #[case] path: &str) { + let response = read_skill_file(&skill_read_fixture.skill, path).await; + + assert_error_code(response, SkillReadFileErrorCode::PathNotReadable); +} + +#[rstest] +#[tokio::test] +#[cfg(target_os = "linux")] +async fn binary_asset_returns_non_inline_metadata(skill_read_fixture: SkillReadFixture) { + let response = read_skill_file(&skill_read_fixture.skill, "assets/logo.png").await; + + let SkillReadFileResponse::Error(response) = response else { + panic!("binary asset should return a typed error payload"); + }; + assert_eq!(response.error.code, SkillReadFileErrorCode::NonInlineAsset); + assert_eq!( + response + .error + .metadata + .expect("metadata should be present") + .mime_type, + "image/png" + ); +} + +#[rstest] +#[tokio::test] +#[cfg(target_os = "linux")] +async fn oversized_text_returns_file_too_large(skill_read_fixture: SkillReadFixture) { + std::fs::write( + skill_read_fixture + .skill + .skill_root() + .join("references/large.md"), + "x".repeat(MAX_SKILL_READ_FILE_BYTES as usize + 1), + ) + .expect("large reference should be written"); + + let response = read_skill_file(&skill_read_fixture.skill, "references/large.md").await; + + assert_error_code(response, SkillReadFileErrorCode::FileTooLarge); +} + +#[rstest] +#[tokio::test] +#[cfg(target_os = "linux")] +async fn missing_file_returns_path_not_readable(skill_read_fixture: SkillReadFixture) { + let response = read_skill_file(&skill_read_fixture.skill, "references/missing.md").await; + + assert_error_code(response, SkillReadFileErrorCode::PathNotReadable); +} + +#[cfg(target_os = "linux")] +#[rstest] +#[tokio::test] +async fn symlink_paths_are_rejected(skill_read_fixture: SkillReadFixture) { + std::os::unix::fs::symlink( + skill_read_fixture + .skill + .skill_root() + .join("references/usage.md"), + skill_read_fixture + .skill + .skill_root() + .join("references/link.md"), + ) + .expect("symlink should be created"); + + let response = read_skill_file(&skill_read_fixture.skill, "references/link.md").await; + + assert_error_code(response, SkillReadFileErrorCode::PathNotReadable); +} + +#[cfg(target_os = "linux")] +#[rstest] +#[tokio::test] +async fn intermediate_symlink_paths_are_rejected(skill_read_fixture: SkillReadFixture) { + let external = tempfile::tempdir().expect("external tempdir should be created"); + std::fs::write(external.path().join("usage.md"), "# Escaped\n") + .expect("external reference should be written"); + std::os::unix::fs::symlink( + external.path(), + skill_read_fixture + .skill + .skill_root() + .join("references/external"), + ) + .expect("intermediate symlink should be created"); + + let response = read_skill_file(&skill_read_fixture.skill, "references/external/usage.md").await; + + assert_error_code(response, SkillReadFileErrorCode::PathNotReadable); +} + +#[cfg(not(target_os = "linux"))] +#[rstest] +#[case::reference("references/usage.md")] +#[case::entrypoint("SKILL.md")] +#[tokio::test] +async fn allowed_reads_fail_closed_on_non_linux( + skill_read_fixture: SkillReadFixture, + #[case] path: &str, +) { + let response = read_skill_file(&skill_read_fixture.skill, path).await; + + assert_error_code(response, SkillReadFileErrorCode::IoError); +} + +fn assert_error_code(response: SkillReadFileResponse, expected: SkillReadFileErrorCode) { + let SkillReadFileResponse::Error(response) = response else { + panic!("expected error response"); + }; + assert_eq!(response.error.code, expected); +} + +proptest! { + #[test] + fn allowed_reference_paths_validate(file_stem in "[a-z][a-z0-9_-]{0,32}") { + let path = format!("references/{file_stem}.md"); + prop_assert!(validate_bundle_relative_path(&path).is_ok()); + } + + #[test] + fn allowed_asset_paths_validate(file_stem in "[a-z][a-z0-9_-]{0,32}") { + let path = format!("assets/{file_stem}.txt"); + prop_assert!(validate_bundle_relative_path(&path).is_ok()); + } + + #[test] + fn nested_entrypoints_are_rejected(root in "(references|assets)") { + let path = format!("{root}/SKILL.md"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn unsupported_root_paths_are_rejected( + root in "[a-z][a-z0-9_-]{1,10}", + filename in "[a-z][a-z0-9_-]{0,10}\\.[a-z]{1,4}", + ) { + prop_assume!(root != "references" && root != "assets"); + let path = format!("{root}/{filename}"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn traversal_segments_are_rejected( + prefix in "(references|assets|scripts)?/?", + stem in "[a-z0-9_-]{0,10}", + ) { + let path = format!("{prefix}../{stem}"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn absolute_paths_are_rejected( + root in prop_oneof![Just("references"), Just("assets"), Just("SKILL")], + suffix in "(/[a-z0-9_-]{0,10})?", + ) { + let path = format!("/{root}{suffix}"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn backslash_separators_are_rejected( + root in prop_oneof![Just("references"), Just("assets"), Just("scripts")], + child in "[a-z0-9_-]{1,10}", + ) { + let path = format!("{root}\\{child}"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn bare_dotdot_is_rejected( + root in prop_oneof![Just("references"), Just("assets")], + ) { + let path = format!("{root}/.."); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn double_traversal_is_rejected( + root in prop_oneof![Just("references"), Just("assets")], + leaf in "[a-z0-9_-]{0,10}", + ) { + let path = format!("{root}/../../{leaf}"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn double_traversal_alone_is_rejected(leaf in "[a-z0-9_-]{0,10}") { + let path = format!("../../{leaf}"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn bare_dot_leading_is_rejected( + segment in "[a-z0-9_-]{1,10}", + ) { + let path = format!("./{segment}"); + prop_assert!(validate_bundle_relative_path(&path).is_err()); + } + + #[test] + fn bare_dot_alone_is_rejected( + dot in Just("."), + ) { + prop_assert!(validate_bundle_relative_path(dot).is_err()); + } + + #[test] + fn utf8_boundary_size_succeeds(size in (0..=MAX_SKILL_READ_FILE_BYTES)) { + let content = "x".repeat(size as usize); + let utf8_check = std::str::from_utf8(content.as_bytes()); + prop_assert!(utf8_check.is_ok()); + prop_assert_eq!( + utf8_check.expect("failed to decode bytes as UTF-8 in utf8_check").len(), + size as usize + ); + } + + #[test] + fn size_boundary_above_cap_is_measured(size in (MAX_SKILL_READ_FILE_BYTES + 1..=MAX_SKILL_READ_FILE_BYTES + 1024)) { + let content = "x".repeat(size as usize); + prop_assert!(content.len() > MAX_SKILL_READ_FILE_BYTES as usize); + } +} + +#[test] +fn skill_entrypoint_path_validates() { + assert!(validate_bundle_relative_path("SKILL.md").is_ok()); +} + +// ── JSON shape snapshot tests ──────────────────────────────────────────────── + +#[rstest] +#[case::success("skill_read_file_success", snapshot_success_response())] +#[case::unknown_skill("skill_read_file_error_unknown_skill", snapshot_error_unknown_skill())] +#[case::path_not_readable( + "skill_read_file_error_path_not_readable", + snapshot_error_path_not_readable() +)] +#[case::non_inline_asset( + "skill_read_file_error_non_inline_asset", + snapshot_error_non_inline_asset() +)] +#[case::file_too_large( + "skill_read_file_error_file_too_large", + snapshot_error_file_too_large() +)] +#[case::invalid_utf8("skill_read_file_error_invalid_utf8", snapshot_error_invalid_utf8())] +#[case::io_error("skill_read_file_error_io_error", snapshot_error_io_error())] +fn snapshot_skill_read_file_response_shapes( + #[case] snapshot_name: &str, + #[case] response: SkillReadFileResponse, +) { + assert_json_snapshot!(snapshot_name, &response); +} + +fn snapshot_success_response() -> SkillReadFileResponse { + SkillReadFileResponse::Success(SkillReadFileSuccess { + skill: "deploy-docs".to_string(), + path: "references/usage.md".to_string(), + mime_type: "text/markdown".to_string(), + content: "# Usage\n".to_string(), + }) +} + +fn snapshot_error_unknown_skill() -> SkillReadFileResponse { + SkillReadFileResponse::unknown_skill("deploy-docs", "references/usage.md") +} + +fn snapshot_error_path_not_readable() -> SkillReadFileResponse { + let error = validate_bundle_relative_path("../secret") + .expect_err("traversal path should fail validation"); + SkillReadFileResponse::error("deploy-docs", "../secret", error) +} + +fn make_error_response( + path: &str, + code: SkillReadFileErrorCode, + message: &str, + metadata: Option, +) -> SkillReadFileResponse { + SkillReadFileResponse::Error(SkillReadFileErrorResponse { + skill: "deploy-docs".to_string(), + path: path.to_string(), + error: SkillReadFileError { + code, + message: message.to_string(), + metadata, + }, + }) +} + +fn snapshot_error_non_inline_asset() -> SkillReadFileResponse { + make_error_response( + "assets/logo.png", + SkillReadFileErrorCode::NonInlineAsset, + "Phase 1 does not return binary or oversized assets inline.", + Some(SkillReadFileMetadata { + size: 8, + mime_type: "image/png".to_string(), + fetch_hint: NON_INLINE_FETCH_HINT.to_string(), + }), + ) +} + +fn snapshot_error_file_too_large() -> SkillReadFileResponse { + make_error_response( + "references/large.md", + SkillReadFileErrorCode::FileTooLarge, + "Phase 1 does not return binary or oversized assets inline.", + Some(SkillReadFileMetadata { + size: MAX_SKILL_READ_FILE_BYTES + 1, + mime_type: "text/markdown".to_string(), + fetch_hint: NON_INLINE_FETCH_HINT.to_string(), + }), + ) +} + +fn snapshot_error_invalid_utf8() -> SkillReadFileResponse { + make_error_response( + "references/binary.md", + SkillReadFileErrorCode::InvalidUtf8, + "File is not valid UTF-8 text", + None, + ) +} + +fn snapshot_error_io_error() -> SkillReadFileResponse { + make_error_response( + "references/usage.md", + SkillReadFileErrorCode::IoError, + "File is not available for reading", + None, + ) +} diff --git a/src/skills/file_read/validation.rs b/src/skills/file_read/validation.rs new file mode 100644 index 000000000..94f975e9c --- /dev/null +++ b/src/skills/file_read/validation.rs @@ -0,0 +1,62 @@ +//! Bundle-relative path validation for scoped skill file reads. + +use std::path::{Component, Path, PathBuf}; + +use super::{SkillReadFileError, SkillReadFileErrorCode}; + +pub(super) fn display_path(requested_path: &str) -> String { + requested_path.replace('\\', "/") +} + +pub(super) fn validate_bundle_relative_path( + requested_path: &str, +) -> Result { + if requested_path.trim().is_empty() || requested_path.contains('\\') { + return Err(path_not_readable()); + } + + let path = Path::new(requested_path); + let mut segments = Vec::new(); + for component in path.components() { + match component { + Component::Normal(segment) => segments.push(segment.to_owned()), + Component::CurDir + | Component::ParentDir + | Component::RootDir + | Component::Prefix(_) => { + return Err(path_not_readable()); + } + } + } + + if !is_allowed_bundle_path(&segments) { + return Err(path_not_readable()); + } + + Ok(segments.iter().collect()) +} + +fn is_allowed_bundle_path(segments: &[std::ffi::OsString]) -> bool { + match segments { + [entrypoint] => entrypoint == "SKILL.md", + [root, rest @ ..] => { + (root == "references" || root == "assets") + && !rest.is_empty() + && rest.iter().all(|segment| segment != "SKILL.md") + } + _ => false, + } +} + +pub(super) fn path_not_readable() -> SkillReadFileError { + SkillReadFileError::new( + SkillReadFileErrorCode::PathNotReadable, + "File is not available for reading", + ) +} + +pub(super) fn is_asset_path(path: &Path) -> bool { + path.components() + .next() + .is_some_and(|component| component.as_os_str() == "assets") +} diff --git a/src/skills/mod.rs b/src/skills/mod.rs index 3ecd0a020..6a5738b26 100644 --- a/src/skills/mod.rs +++ b/src/skills/mod.rs @@ -18,6 +18,8 @@ pub mod attenuation; pub mod bundle; pub mod catalog; pub mod escape; +/// Read-only, skill-scoped access to bundled skill resources (references and assets). +pub mod file_read; pub mod gating; /// Shared source-field normalisation helpers for skill install adapters. pub(crate) mod install_source; diff --git a/src/tools/builtin/mod.rs b/src/tools/builtin/mod.rs index 5faa5908b..8711c2e8e 100644 --- a/src/tools/builtin/mod.rs +++ b/src/tools/builtin/mod.rs @@ -40,7 +40,9 @@ pub use routine::{ }; pub use secrets_tools::{SecretDeleteTool, SecretListTool}; pub use shell::ShellTool; -pub use skill_tools::{SkillInstallTool, SkillListTool, SkillRemoveTool, SkillSearchTool}; +pub use skill_tools::{ + SkillInstallTool, SkillListTool, SkillReadFileTool, SkillRemoveTool, SkillSearchTool, +}; pub use time::TimeTool; mod html_converter; pub mod image_analyze; diff --git a/src/tools/builtin/skill_tools.rs b/src/tools/builtin/skill_tools.rs index 18a33b4ff..f06903b12 100644 --- a/src/tools/builtin/skill_tools.rs +++ b/src/tools/builtin/skill_tools.rs @@ -1,6 +1,6 @@ //! Agent-callable tools for managing skills (prompt-level extensions). //! -//! Four tools for discovering, installing, listing, and removing skills +//! Five tools for discovering, installing, listing, removing, and reading bundled files from skills //! entirely through conversation, following the extension_tools pattern. use std::collections::HashSet; @@ -13,11 +13,13 @@ use crate::skills::registry::SkillRegistry; use crate::tools::tool::{NativeTool, ToolError, ToolOutput, require_str}; mod install; +mod read_file; mod remove; #[cfg(test)] mod tests; pub use install::SkillInstallTool; +pub use read_file::SkillReadFileTool; pub use remove::SkillRemoveTool; /// Build a minimal JSON Schema object descriptor. @@ -34,6 +36,12 @@ fn object_schema(properties: serde_json::Value, required: &[&str]) -> serde_json schema } +fn strict_object_schema(properties: serde_json::Value, required: &[&str]) -> serde_json::Value { + let mut schema = object_schema(properties, required); + schema["additionalProperties"] = serde_json::Value::Bool(false); + schema +} + // ── skill_list ────────────────────────────────────────────────────────── pub struct SkillListTool { diff --git a/src/tools/builtin/skill_tools/features/skill_read_file.feature b/src/tools/builtin/skill_tools/features/skill_read_file.feature new file mode 100644 index 000000000..12f8d9f10 --- /dev/null +++ b/src/tools/builtin/skill_tools/features/skill_read_file.feature @@ -0,0 +1,11 @@ +Feature: Skill bundle file reads + + Scenario: A model reads a referenced bundled text file + Given a loaded skill bundle with a referenced usage file + When the model calls skill_read_file for the usage file + Then the tool returns the referenced text without a host filesystem path + + Scenario: A model is denied raw filesystem traversal + Given a loaded skill bundle with a referenced usage file + When the model calls skill_read_file with a traversal path + Then the tool returns a skill-scoped path_not_readable error diff --git a/src/tools/builtin/skill_tools/read_file.rs b/src/tools/builtin/skill_tools/read_file.rs new file mode 100644 index 000000000..70c625349 --- /dev/null +++ b/src/tools/builtin/skill_tools/read_file.rs @@ -0,0 +1,111 @@ +//! Skill bundle file read tool implementation. + +use std::sync::Arc; + +use crate::context::JobContext; +use crate::skills::LoadedSkill; +use crate::skills::file_read::{SkillReadFileResponse, read_skill_file}; +use crate::skills::registry::SkillRegistry; +use crate::tools::tool::{NativeTool, ToolError, ToolOutput, require_str}; + +use super::strict_object_schema; + +/// Tool for reading text files from a loaded skill bundle by bundle-relative path. +pub struct SkillReadFileTool { + registry: Arc>, +} + +impl SkillReadFileTool { + /// Create a new read tool backed by the shared skill registry. + pub fn new(registry: Arc>) -> Self { + Self { registry } + } + + fn lookup_skill(&self, skill_name: &str) -> Result, ToolError> { + let guard = self + .registry + .read() + .map_err(|e| ToolError::ExecutionFailed(format!("Lock poisoned: {}", e)))?; + Ok(guard.find_by_name(skill_name).cloned()) + } +} + +impl NativeTool for SkillReadFileTool { + fn name(&self) -> &str { + "skill_read_file" + } + + fn description(&self) -> &str { + "Read a text file from a loaded skill bundle using a bundle-relative path." + } + + fn parameters_schema(&self) -> serde_json::Value { + strict_object_schema( + serde_json::json!({ + "skill": { + "type": "string", + "description": "Loaded skill name exactly as advertised to the model." + }, + "path": { + "type": "string", + "description": "Bundle-relative path, such as SKILL.md or references/usage.md." + } + }), + &["skill", "path"], + ) + } + + async fn execute( + &self, + params: serde_json::Value, + _ctx: &JobContext, + ) -> Result { + let start = std::time::Instant::now(); + let skill_name = require_str(¶ms, "skill")?.trim(); + let path = require_str(¶ms, "path")?; + if skill_name.is_empty() { + return Err(ToolError::InvalidParameters( + "skill must not be empty".to_string(), + )); + } + + let response = match self.lookup_skill(skill_name)? { + Some(skill) => read_skill_file(&skill, path).await, + None => { + tracing::debug!( + skill_id = %skill_name, + path = %path, + "skill_read_file: skill not found in registry" + ); + SkillReadFileResponse::unknown_skill(skill_name, path) + } + }; + + let elapsed = start.elapsed(); + match &response { + SkillReadFileResponse::Success(success) => { + tracing::debug!( + skill_id = %skill_name, + path = %path, + size = success.content.len(), + duration_ms = elapsed.as_millis(), + "skill_read_file: success" + ); + } + SkillReadFileResponse::Error(error) => { + tracing::debug!( + skill_id = %skill_name, + path = %path, + error_code = ?error.error.code, + duration_ms = elapsed.as_millis(), + "skill_read_file: error" + ); + } + } + + let result = serde_json::to_value(response).map_err(|error| { + ToolError::ExecutionFailed(format!("failed to serialize skill read response: {error}")) + })?; + Ok(ToolOutput::success(result, elapsed)) + } +} diff --git a/src/tools/builtin/skill_tools/tests.rs b/src/tools/builtin/skill_tools/tests.rs index 2da2a2d21..fa2a1b152 100644 --- a/src/tools/builtin/skill_tools/tests.rs +++ b/src/tools/builtin/skill_tools/tests.rs @@ -4,6 +4,7 @@ use std::collections::BTreeSet; use std::sync::Arc; use rstest::{fixture, rstest}; +use rstest_bdd_macros::{given, scenario, then, when}; use crate::context::JobContext; use crate::skills::catalog::SkillCatalog; @@ -11,13 +12,25 @@ use crate::skills::registry::SkillRegistry; use crate::skills::{LoadedSkillLocation, SkillPackageKind, SkillSource}; use crate::tools::tool::{ApprovalRequirement, NativeTool, Tool}; -use super::{SkillInstallTool, SkillListTool, SkillRemoveTool, SkillSearchTool}; +use super::{SkillInstallTool, SkillListTool, SkillReadFileTool, SkillRemoveTool, SkillSearchTool}; struct TestRegistryHandle { _dir: tempfile::TempDir, registry: Arc>, } +#[derive(Default)] +struct SkillReadFileWorld { + bundle_dir: Option, + registry: Option>>, + output: Option, +} + +#[fixture] +fn skill_read_file_world() -> SkillReadFileWorld { + SkillReadFileWorld::default() +} + #[fixture] fn test_registry() -> TestRegistryHandle { let dir = tempfile::tempdir().expect("tempdir creation failed"); @@ -37,6 +50,27 @@ fn skill_markdown(name: &str) -> String { format!("---\nname: {name}\n---\n\n# {name}\n") } +fn insert_deploy_docs_bundle( + registry: &Arc>, + root: &std::path::Path, +) { + let location = LoadedSkillLocation::new( + "deploy-docs", + root, + std::path::PathBuf::from("SKILL.md"), + SkillPackageKind::Bundle, + ) + .expect("bundle location should be valid"); + let skill = crate::skills::test_support::TestSkillBuilder::new("deploy-docs") + .location(location) + .build(); + registry + .write() + .expect("registry lock should be writable") + .commit_loaded_skill("deploy-docs", skill) + .expect("skill should be inserted"); +} + /// Assert the common contract of a skill tool's static metadata. fn assert_tool_schema( tool: &dyn Tool, @@ -117,6 +151,14 @@ fn test_skill_list_schema(test_registry: TestRegistryHandle) { ApprovalRequirement::Always, &["name"] as &[&str], )] +#[case( + |r: &TestRegistryHandle, _c: Arc| -> Box { + Box::new(SkillReadFileTool::new(Arc::clone(&r.registry))) + }, + "skill_read_file", + ApprovalRequirement::Never, + &["path", "skill"] as &[&str], +)] fn test_skill_tool_schema( test_registry: TestRegistryHandle, test_catalog: Arc, @@ -129,6 +171,15 @@ fn test_skill_tool_schema( assert_tool_schema(&*tool, expected_name, expected_approval, expected_keys); } +#[rstest] +fn skill_read_file_schema_is_strict(test_registry: TestRegistryHandle) { + let tool = SkillReadFileTool::new(Arc::clone(&test_registry.registry)); + let schema = NativeTool::parameters_schema(&tool); + + assert_schema_required_fields(&tool, &["skill", "path"]); + assert_eq!(schema["additionalProperties"], false); +} + #[rstest] fn skill_install_schema_does_not_require_catalogue_name(test_registry: TestRegistryHandle) { let tool = SkillInstallTool::new(Arc::clone(&test_registry.registry), test_catalog()); @@ -224,6 +275,159 @@ async fn skill_install_tool_installs_inline_content(test_registry: TestRegistryH ); } +#[rstest] +#[tokio::test] +async fn skill_read_file_tool_reads_bundle_reference(test_registry: TestRegistryHandle) { + let bundle_dir = tempfile::tempdir().expect("bundle tempdir should be created"); + std::fs::create_dir_all(bundle_dir.path().join("references")) + .expect("references dir should be created"); + std::fs::write(bundle_dir.path().join("SKILL.md"), "# Deploy docs\n") + .expect("SKILL.md should be written"); + std::fs::write(bundle_dir.path().join("references/usage.md"), "# Usage\n") + .expect("reference should be written"); + insert_deploy_docs_bundle(&test_registry.registry, bundle_dir.path()); + + let tool = SkillReadFileTool::new(Arc::clone(&test_registry.registry)); + let output = NativeTool::execute( + &tool, + serde_json::json!({ + "skill": "deploy-docs", + "path": "references/usage.md", + }), + &JobContext::default(), + ) + .await + .expect("skill_read_file should succeed"); + + assert_eq!(output.result["skill"], "deploy-docs"); + assert_eq!(output.result["path"], "references/usage.md"); + assert_eq!(output.result["content"], "# Usage\n"); + assert!(output.result.get("error").is_none()); +} + +#[rstest] +#[tokio::test] +async fn skill_read_file_tool_reports_unknown_skill(test_registry: TestRegistryHandle) { + let tool = SkillReadFileTool::new(Arc::clone(&test_registry.registry)); + + let output = NativeTool::execute( + &tool, + serde_json::json!({ + "skill": "missing", + "path": "SKILL.md", + }), + &JobContext::default(), + ) + .await + .expect("unknown skill should be a structured tool result"); + + assert_eq!(output.result["skill"], "missing"); + assert_eq!(output.result["path"], "SKILL.md"); + assert_eq!(output.result["error"]["code"], "unknown_skill"); +} + +#[given("a loaded skill bundle with a referenced usage file")] +fn bdd_loaded_skill_bundle(skill_read_file_world: &mut SkillReadFileWorld) { + let bundle_dir = tempfile::tempdir().expect("bundle tempdir should be created"); + std::fs::create_dir_all(bundle_dir.path().join("references")) + .expect("references dir should be created"); + std::fs::write(bundle_dir.path().join("SKILL.md"), "# Deploy docs\n") + .expect("SKILL.md should be written"); + std::fs::write(bundle_dir.path().join("references/usage.md"), "# Usage\n") + .expect("reference should be written"); + + let registry = Arc::new(std::sync::RwLock::new(SkillRegistry::new( + bundle_dir.path().join("unused-user-dir"), + ))); + insert_deploy_docs_bundle(®istry, bundle_dir.path()); + + skill_read_file_world.bundle_dir = Some(bundle_dir); + skill_read_file_world.registry = Some(registry); +} + +#[when("the model calls skill_read_file for the usage file")] +fn bdd_model_reads_usage_file(skill_read_file_world: &mut SkillReadFileWorld) { + execute_bdd_read( + skill_read_file_world, + serde_json::json!({ + "skill": "deploy-docs", + "path": "references/usage.md", + }), + ); +} + +#[when("the model calls skill_read_file with a traversal path")] +fn bdd_model_reads_traversal_path(skill_read_file_world: &mut SkillReadFileWorld) { + execute_bdd_read( + skill_read_file_world, + serde_json::json!({ + "skill": "deploy-docs", + "path": "../secrets.txt", + }), + ); +} + +#[then("the tool returns the referenced text without a host filesystem path")] +fn bdd_tool_returns_reference_text(skill_read_file_world: &SkillReadFileWorld) { + let output = skill_read_file_world + .output + .as_ref() + .expect("When step should execute tool"); + assert_eq!(output["skill"], "deploy-docs"); + assert_eq!(output["path"], "references/usage.md"); + assert_eq!(output["content"], "# Usage\n"); + + let root = skill_read_file_world + .bundle_dir + .as_ref() + .expect("Given step should create bundle") + .path() + .to_string_lossy(); + assert!(!output.to_string().contains(root.as_ref())); +} + +#[then("the tool returns a skill-scoped path_not_readable error")] +fn bdd_tool_returns_path_not_readable(skill_read_file_world: &SkillReadFileWorld) { + let output = skill_read_file_world + .output + .as_ref() + .expect("When step should execute tool"); + assert_eq!(output["skill"], "deploy-docs"); + assert_eq!(output["path"], "../secrets.txt"); + assert_eq!(output["error"]["code"], "path_not_readable"); +} + +fn execute_bdd_read(skill_read_file_world: &mut SkillReadFileWorld, params: serde_json::Value) { + let registry = Arc::clone( + skill_read_file_world + .registry + .as_ref() + .expect("Given step should create registry"), + ); + let tool = SkillReadFileTool::new(registry); + let runtime = tokio::runtime::Runtime::new().expect("tokio runtime should start"); + let output = runtime + .block_on(NativeTool::execute(&tool, params, &JobContext::default())) + .expect("skill_read_file should return a tool output"); + skill_read_file_world.output = Some(output.result); +} + +#[scenario( + path = "src/tools/builtin/skill_tools/features/skill_read_file.feature", + name = "A model reads a referenced bundled text file" +)] +fn bdd_model_reads_referenced_bundled_text_file(skill_read_file_world: SkillReadFileWorld) { + assert!(skill_read_file_world.output.is_some()); +} + +#[scenario( + path = "src/tools/builtin/skill_tools/features/skill_read_file.feature", + name = "A model is denied raw filesystem traversal" +)] +fn bdd_model_is_denied_raw_filesystem_traversal(skill_read_file_world: SkillReadFileWorld) { + assert!(skill_read_file_world.output.is_some()); +} + #[rstest] #[tokio::test] async fn skill_install_tool_rejects_duplicate_inline_install(test_registry: TestRegistryHandle) { diff --git a/src/tools/registry/builtins.rs b/src/tools/registry/builtins.rs index 0e2711941..00a172a92 100644 --- a/src/tools/registry/builtins.rs +++ b/src/tools/registry/builtins.rs @@ -16,9 +16,9 @@ use crate::tools::builtin::{ ApplyPatchTool, CancelJobTool, CreateJobTool, EchoTool, ExtensionInfoTool, HttpTool, JobEventsTool, JobPromptTool, JobStatusTool, JsonTool, ListDirTool, ListJobsTool, MemoryReadTool, MemorySearchTool, MemoryTreeTool, MemoryWriteTool, PromptQueue, ReadFileTool, - ShellTool, SkillInstallTool, SkillListTool, SkillRemoveTool, SkillSearchTool, TimeTool, - ToolActivateTool, ToolAuthTool, ToolInstallTool, ToolListTool, ToolRemoveTool, ToolSearchTool, - ToolUpgradeTool, WriteFileTool, + ShellTool, SkillInstallTool, SkillListTool, SkillReadFileTool, SkillRemoveTool, + SkillSearchTool, TimeTool, ToolActivateTool, ToolAuthTool, ToolInstallTool, ToolListTool, + ToolRemoveTool, ToolSearchTool, ToolUpgradeTool, WriteFileTool, }; use crate::tools::registry::loader::ToolRegistry; use crate::tools::tool::{Tool, ToolDomain}; @@ -228,7 +228,7 @@ impl ToolRegistry { tracing::debug!("Registered 8 extension management tools"); } - /// Register skill management tools (list, search, install, remove). + /// Register skill management tools (list, search, read_file, install, remove). /// /// These allow the LLM to manage prompt-level skills through conversation. pub fn register_skill_tools( @@ -241,12 +241,13 @@ impl ToolRegistry { Arc::clone(®istry), Arc::clone(&catalog), ))); + self.register_sync(Arc::new(SkillReadFileTool::new(Arc::clone(®istry)))); self.register_sync(Arc::new(SkillInstallTool::new( Arc::clone(®istry), Arc::clone(&catalog), ))); self.register_sync(Arc::new(SkillRemoveTool::new(registry))); - tracing::debug!("Registered 4 skill management tools"); + tracing::debug!("Registered 5 skill management tools"); } /// Register routine management tools.