From ee7ee378336cc859c8d4440d7255f48ea5b12c0c Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 19 May 2026 01:40:39 +0200 Subject: [PATCH 01/18] Draft skill bundled file read execution plan Add the approval-gated ExecPlan for roadmap item `1.3.4`. The plan captures the intended `skill_read_file` boundary, validation strategy, testing expectations, documentation updates, and review gates before implementation begins. --- .../1-3-4-skill-read-file-interface.md | 720 ++++++++++++++++++ 1 file changed, 720 insertions(+) create mode 100644 docs/execplans/1-3-4-skill-read-file-interface.md 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..48faa09e8 --- /dev/null +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -0,0 +1,720 @@ +# 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: DRAFT + +## 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 +`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. +- If adding Python `pytest-bdd` for end-to-end behaviour coverage requires a + new dependency in `tests/e2e/pyproject.toml`, keep that dependency scoped to + the e2e package and document why. Do not add Python tooling to the Rust + runtime. +- Use `rstest` fixtures for Rust unit and integration tests. Use `pytest` for + Python end-to-end tests. Add `pytest-bdd` scenarios for user-requested + behavioural e2e coverage unless a concrete repository conflict is found; if + so, stop and record the conflict. +- 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. + If `pytest-bdd` creates dependency or CI friction, record the exact failure + and ask whether to keep Python BDD or substitute the existing Rust + `rstest-bdd` layer. +- 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 `pytest-bdd` is not currently part of the e2e test package. + Severity: medium. + Likelihood: high. + Mitigation: add it only to `tests/e2e/pyproject.toml` for behaviour coverage, + keep the scenario focused, and preserve the existing `pytest` suite style. + Escalate if the project maintainers prefer Rust `rstest-bdd` only. + +- 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. + +- Risk: e2e tests may need a deterministic model tool-call flow that the + current mock LLM does not yet provide. + Severity: medium. + Likelihood: medium. + Mitigation: prefer a direct HTTP/API e2e test if the server exposes tool + execution enough for it; otherwise extend `tests/e2e/mock_llm.py` with the + smallest deterministic tool-call fixture. + +## 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. +- [ ] Receive explicit approval for this ExecPlan before implementation. +- [ ] Implement domain path policy and typed response model. +- [ ] Implement and register `skill_read_file`. +- [ ] Add unit, property, behavioural, snapshot, and e2e coverage. +- [ ] Update documentation and mark roadmap item `1.3.4` done. +- [ ] Run `coderabbit review --agent` after each major implementation + milestone and resolve concerns. +- [ ] Run final gates and commit the approved implementation. + +## Surprises & discoveries + +- Observation: `leta workspace add` succeeded, but the first Rust 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: `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: the approved implementation should either add a small e2e + `pytest-bdd` scenario because the task explicitly requests it, or stop if + that dependency conflicts with repository policy. + +- 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. + +## 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: the repository already uses `rstest-bdd` for Rust behavioural + tests, while the user explicitly requested `pytest` and `pytest-bdd`. + Keeping `pytest-bdd` scoped to the e2e package honours the request without + changing the Rust runtime test stack. + Date/Author: 2026-05-19 / 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. + +## Outcomes & retrospective + +This plan is not yet implemented. The expected outcome after approval is a +single read-only `skill_read_file` tool that reads only loaded skill bundle +resources through stable bundle-relative paths, plus tests and documentation +that prove 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 +`pytest`; add `pytest-bdd` only to that e2e package and create a small +Given/When/Then scenario for installing or loading a bundled skill, invoking +`skill_read_file`, and observing a structured text success plus a structured +denial. 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/b0245e42-5cad-47f8-8090-c3eb643834a6 +``` + +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 `pytest` and, if added, `pytest-bdd` proves the + externally observable read flow or records a documented blocker. +- 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 dependency setup fails because `pytest-bdd` is not accepted by +the repository, preserve the Rust `rstest-bdd` behavioural coverage, document +the conflict in this plan, and ask for approval before dropping the Python BDD +requirement. + +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. From 38ed636edb75b1e565bb060c65e3c2131c25cc1a Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 20 May 2026 13:46:37 +0200 Subject: [PATCH 02/18] Refresh bundled skill file read plan Update the pre-implementation ExecPlan with the current worktree path, branch and pull request state, and the latest planning evidence. Narrow behavioural test guidance to Rust `rstest-bdd` plus existing Python `pytest` end-to-end patterns where system-level coverage is actually needed. --- .../1-3-4-skill-read-file-interface.md | 111 +++++++++++------- 1 file changed, 66 insertions(+), 45 deletions(-) diff --git a/docs/execplans/1-3-4-skill-read-file-interface.md b/docs/execplans/1-3-4-skill-read-file-interface.md index 48faa09e8..5f2123fe4 100644 --- a/docs/execplans/1-3-4-skill-read-file-interface.md +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -170,14 +170,10 @@ structured input/output schema, and validation before resource access.[^1][^2] - Avoid new Rust runtime dependencies. `mime_guess`, `serde`, `serde_json`, `thiserror`, `tokio`, `rstest`, `rstest-bdd`, `insta`, and `proptest` are already available. -- If adding Python `pytest-bdd` for end-to-end behaviour coverage requires a - new dependency in `tests/e2e/pyproject.toml`, keep that dependency scoped to - the e2e package and document why. Do not add Python tooling to the Rust - runtime. -- Use `rstest` fixtures for Rust unit and integration tests. Use `pytest` for - Python end-to-end tests. Add `pytest-bdd` scenarios for user-requested - behavioural e2e coverage unless a concrete repository conflict is found; if - so, stop and record the conflict. +- 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. @@ -200,9 +196,9 @@ conflict in `Decision Log`, and ask for direction. listings, partial reads, or binary fetch URLs is out of scope unless explicitly approved. - Dependencies: if any new Rust runtime dependency appears necessary, stop. - If `pytest-bdd` creates dependency or CI friction, record the exact failure - and ask whether to keep Python BDD or substitute the existing Rust - `rstest-bdd` layer. + 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. @@ -241,12 +237,14 @@ conflict in `Decision Log`, and ask for direction. Mitigation: add attenuation only after unit tests prove no writes, network calls, generic filesystem paths, or state mutation are involved. -- Risk: Python `pytest-bdd` is not currently part of the e2e test package. +- Risk: Python e2e coverage may need deterministic tool-call plumbing rather + than a direct unit-level invocation. Severity: medium. - Likelihood: high. - Mitigation: add it only to `tests/e2e/pyproject.toml` for behaviour coverage, - keep the scenario focused, and preserve the existing `pytest` suite style. - Escalate if the project maintainers prefer Rust `rstest-bdd` only. + 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. @@ -255,14 +253,6 @@ conflict in `Decision Log`, and ask for direction. below the existing prompt-oriented cap unless RFC review dictates otherwise, and snapshot the error payload for oversized reads. -- Risk: e2e tests may need a deterministic model tool-call flow that the - current mock LLM does not yet provide. - Severity: medium. - Likelihood: medium. - Mitigation: prefer a direct HTTP/API e2e test if the server exposes tool - execution enough for it; otherwise extend `tests/e2e/mock_llm.py` with the - smallest deterministic tool-call fixture. - ## Progress - [x] (2026-05-19 00:00+02:00) Loaded `leta`, `rust-router`, @@ -281,6 +271,19 @@ conflict in `Decision Log`, and ask for direction. - [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. - [ ] Receive explicit approval for this ExecPlan before implementation. - [ ] Implement domain path policy and typed response model. - [ ] Implement and register `skill_read_file`. @@ -304,9 +307,10 @@ conflict in `Decision Log`, and ask for direction. 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: the approved implementation should either add a small e2e - `pytest-bdd` scenario because the task explicitly requests it, or stop if - that dependency conflicts with repository policy. + 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. @@ -321,6 +325,15 @@ conflict in `Decision Log`, and ask for direction. Impact: media type inference for non-inline asset metadata does not require a new Rust dependency. +- 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 @@ -346,11 +359,16 @@ conflict in `Decision Log`, and ask for direction. - Decision: Plan both Rust `rstest-bdd` coverage and Python `pytest-bdd` e2e coverage. - Rationale: the repository already uses `rstest-bdd` for Rust behavioural - tests, while the user explicitly requested `pytest` and `pytest-bdd`. - Keeping `pytest-bdd` scoped to the e2e package honours the request without - changing the Rust runtime test stack. - Date/Author: 2026-05-19 / Codex. + 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. @@ -418,12 +436,10 @@ 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 -`pytest`; add `pytest-bdd` only to that e2e package and create a small -Given/When/Then scenario for installing or loading a bundled skill, invoking -`skill_read_file`, and observing a structured text success plus a structured -denial. Use the mock LLM or direct HTTP/tool invocation path, whichever is the -smallest deterministic system-level route. +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. @@ -445,7 +461,7 @@ Stage F performs final review, validation, and commit. Run All commands run from the repository root: ```bash -cd /home/leynos/.lody/repos/github---leynos---axinite/worktrees/b0245e42-5cad-47f8-8090-c3eb643834a6 +cd /home/leynos/.lody/repos/github---leynos---axinite/worktrees/6c92fc0f-e404-4a10-ace1-30a36066de50 ``` Before editing, confirm the branch and workspace: @@ -571,8 +587,9 @@ The feature is accepted when all of the following are true: - 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 `pytest` and, if added, `pytest-bdd` proves the - externally observable read flow or records a documented blocker. +- 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. @@ -602,10 +619,9 @@ 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 dependency setup fails because `pytest-bdd` is not accepted by -the repository, preserve the Rust `rstest-bdd` behavioural coverage, document -the conflict in this plan, and ask for approval before dropping the Python BDD -requirement. +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 @@ -718,3 +734,8 @@ 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. From fbfd0f30baf326767ebd571bb68c8328363b7612 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 20 May 2026 23:46:42 +0200 Subject: [PATCH 03/18] Add scoped bundled skill file reads Introduce `skill_read_file` as a built-in read-only tool backed by a skill-owned path policy. Allow models to read `SKILL.md`, `references/**`, and text assets from loaded skill bundles without exposing raw filesystem access. Return structured skill-scoped JSON errors for unknown skills, disallowed paths, binary assets, oversized files, invalid UTF-8, and I/O failures. Add unit, property, schema, and `rstest-bdd` coverage for the happy path and policy denials. Document the user-facing tool contract, internal boundary rules, feature parity status, and roadmap completion for item `1.3.4`. --- FEATURE_PARITY.md | 4 +- docs/agent-skills-support.md | 40 ++-- docs/axinite-architecture-overview.md | 2 +- docs/developers-guide.md | 35 ++- .../1-3-4-skill-read-file-interface.md | 184 ++++++++++++++- docs/roadmap.md | 2 +- docs/users-guide.md | 39 +++- src/skills/attenuation.rs | 3 + src/skills/file_read.rs | 179 +++++++++++++++ src/skills/file_read/io.rs | 211 ++++++++++++++++++ src/skills/file_read/tests.rs | 194 ++++++++++++++++ src/skills/file_read/validation.rs | 62 +++++ src/skills/mod.rs | 1 + src/tools/builtin/mod.rs | 4 +- src/tools/builtin/skill_tools.rs | 78 +++++++ .../features/skill_read_file.feature | 11 + src/tools/builtin/skill_tools/tests.rs | 206 ++++++++++++++++- src/tools/registry/builtins.rs | 9 +- 18 files changed, 1216 insertions(+), 48 deletions(-) create mode 100644 src/skills/file_read.rs create mode 100644 src/skills/file_read/io.rs create mode 100644 src/skills/file_read/tests.rs create mode 100644 src/skills/file_read/validation.rs create mode 100644 src/tools/builtin/skill_tools/features/skill_read_file.feature 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 index 5f2123fe4..685cc727b 100644 --- a/docs/execplans/1-3-4-skill-read-file-interface.md +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -5,7 +5,7 @@ This ExecPlan (execution plan) is a living document. The sections `Surprises & Discoveries`, `Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work proceeds. -Status: DRAFT +Status: IN PROGRESS ## Purpose / big picture @@ -284,14 +284,30 @@ conflict in `Decision Log`, and ask for direction. `pytest-bdd`; the implementation should use Rust `rstest-bdd` for behavioural coverage and existing Python `pytest` e2e patterns only where system-level behaviour changes. -- [ ] Receive explicit approval for this ExecPlan before implementation. -- [ ] Implement domain path policy and typed response model. -- [ ] Implement and register `skill_read_file`. -- [ ] Add unit, property, behavioural, snapshot, and e2e coverage. -- [ ] Update documentation and mark roadmap item `1.3.4` done. -- [ ] Run `coderabbit review --agent` after each major implementation +- [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. -- [ ] Run final gates and commit the approved implementation. +- [x] Run final gates. +- [ ] Commit the approved implementation. ## Surprises & discoveries @@ -302,6 +318,15 @@ conflict in `Decision Log`, and ask for direction. 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`, @@ -325,6 +350,120 @@ conflict in `Decision Log`, and ask for direction. 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: 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 @@ -378,12 +517,33 @@ conflict in `Decision Log`, and ask for direction. 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 -This plan is not yet implemented. The expected outcome after approval is a -single read-only `skill_read_file` tool that reads only loaded skill bundle -resources through stable bundle-relative paths, plus tests and documentation -that prove the feature works without exposing raw filesystem access. +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 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..4ea3e474e --- /dev/null +++ b/src/skills/file_read.rs @@ -0,0 +1,179 @@ +//! 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, + /// Normalised 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 normalised 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) => 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..1b1f9da3e --- /dev/null +++ b/src/skills/file_read/io.rs @@ -0,0 +1,211 @@ +//! Filesystem resolution and inline decoding for scoped skill file reads. + +use std::path::{Path, PathBuf}; + +use tokio::io::AsyncReadExt; + +use super::validation::{is_asset_path, 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 CanonicalTarget { + path: PathBuf, + 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 resolve_and_validate_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 resolve_and_validate_target( + root: &Path, + relative_path: &Path, +) -> Result { + let canonical_root = tokio::fs::canonicalize(root) + .await + .map_err(|_| io_error("Skill root is not readable"))?; + let target = root.join(relative_path); + let metadata = readable_file_metadata(&target).await?; + let canonical_target = tokio::fs::canonicalize(&target) + .await + .map_err(|_| io_error("File is not available for reading"))?; + + if !canonical_target.starts_with(&canonical_root) { + return Err(path_not_readable()); + } + + Ok(CanonicalTarget { + path: canonical_target, + size: metadata.len(), + }) +} + +async fn readable_file_metadata(path: &Path) -> Result { + match tokio::fs::symlink_metadata(path).await { + Ok(metadata) if metadata.file_type().is_symlink() || !metadata.is_file() => { + Err(path_not_readable()) + } + Ok(metadata) => Ok(metadata), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Err(path_not_readable()), + Err(_) => Err(io_error("File is not available for reading")), + } +} + +async fn read_inline_content( + target: &CanonicalTarget, + relative_path: &Path, + mime_type: String, +) -> Result { + if target.size > MAX_SKILL_READ_FILE_BYTES { + return Err(non_inline_error( + SkillReadFileErrorCode::FileTooLarge, + target.size, + mime_type, + )); + } + + let bytes = read_file_contents(target).await?; + parse_utf8_content(bytes, relative_path, target.size, mime_type) +} + +async fn read_file_contents(target: &CanonicalTarget) -> Result, SkillReadFileError> { + let mut file = open_readonly_no_follow(&target.path).await?; + validate_opened_file(&file, target.size).await?; + + // The cast is safe because the size is first capped at + // MAX_SKILL_READ_FILE_BYTES, which fits in usize on supported platforms. + let mut contents = Vec::with_capacity(target.size.min(MAX_SKILL_READ_FILE_BYTES) as usize); + file.read_to_end(&mut contents) + .await + .map_err(|_| io_error("File is not available for reading"))?; + Ok(contents) +} + +#[cfg(unix)] +async fn open_readonly_no_follow(path: &Path) -> Result { + tokio::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(path) + .await + .map_err(|error| match error.raw_os_error() { + Some(libc::ELOOP) => path_not_readable(), + _ => io_error("File is not available for reading"), + }) +} + +#[cfg(not(unix))] +async fn open_readonly_no_follow(path: &Path) -> Result { + // Non-Unix platforms do not have the Unix O_NOFOLLOW path used above. + // This fallback relies on the earlier symlink_metadata rejection and the + // later validate_opened_file size check: file size must match stored size + // after open or the open is rejected. TODO: use platform-specific atomic + // no-follow open semantics for non-Unix targets when this tool supports + // those platforms as first-class deployment environments. + tokio::fs::File::open(path) + .await + .map_err(|_| io_error("File is not available for reading")) +} + +async fn validate_opened_file( + file: &tokio::fs::File, + expected_size: u64, +) -> Result<(), SkillReadFileError> { + let metadata = file + .metadata() + .await + .map_err(|_| io_error("File is not available for reading"))?; + if !metadata.is_file() { + return Err(path_not_readable()); + } + if metadata.len() != expected_size { + return Err(io_error("File changed while reading")); + } + Ok(()) +} + +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(_) => Err(SkillReadFileError::new( + SkillReadFileErrorCode::InvalidUtf8, + "File is not valid UTF-8 text", + )), + } +} + +fn error_response(display: &ReadDisplay<'_>, error: SkillReadFileError) -> SkillReadFileResponse { + 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/tests.rs b/src/skills/file_read/tests.rs new file mode 100644 index 000000000..c845123b1 --- /dev/null +++ b/src/skills/file_read/tests.rs @@ -0,0 +1,194 @@ +//! Unit and property tests for skill bundle file read operations. + +use std::path::PathBuf; + +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] +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] +async fn reads_skill_entrypoint(skill_read_fixture: SkillReadFixture) { + let response = read_skill_file(&skill_read_fixture.skill, "SKILL.md").await; + + assert!(matches!(response, SkillReadFileResponse::Success(_))); +} + +#[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] +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] +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] +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(unix)] +#[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); +} + +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 disallowed_generated_paths_do_not_validate(raw in "\\PC*") { + let looks_allowed = raw == "SKILL.md" + || raw.starts_with("references/") + || raw.starts_with("assets/"); + + if !looks_allowed + || raw.contains("..") + || raw.contains('\\') + || raw.starts_with('/') + || raw.trim().is_empty() + { + prop_assert!(validate_bundle_relative_path(&raw).is_err()); + } + } +} + +#[test] +fn skill_entrypoint_path_validates() { + assert!(validate_bundle_relative_path("SKILL.md").is_ok()); +} 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..2028607cd 100644 --- a/src/skills/mod.rs +++ b/src/skills/mod.rs @@ -18,6 +18,7 @@ pub mod attenuation; pub mod bundle; pub mod catalog; pub mod escape; +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..08e88534e 100644 --- a/src/tools/builtin/skill_tools.rs +++ b/src/tools/builtin/skill_tools.rs @@ -9,6 +9,7 @@ use std::sync::Arc; use crate::context::JobContext; use crate::skills::LoadedSkill; use crate::skills::catalog::{CatalogEntry, SkillCatalog}; +use crate::skills::file_read::{SkillReadFileResponse, read_skill_file}; use crate::skills::registry::SkillRegistry; use crate::tools::tool::{NativeTool, ToolError, ToolOutput, require_str}; @@ -34,6 +35,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 { @@ -128,6 +135,77 @@ impl NativeTool for SkillListTool { } } +// ── skill_read_file ───────────────────────────────────────────────────── + +pub struct SkillReadFileTool { + registry: Arc>, +} + +impl SkillReadFileTool { + 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 => SkillReadFileResponse::unknown_skill(skill_name, path), + }; + + let result = serde_json::to_value(response).map_err(|error| { + ToolError::ExecutionFailed(format!("failed to serialize skill read response: {error}")) + })?; + Ok(ToolOutput::success(result, start.elapsed())) + } +} + // ── skill_search ──────────────────────────────────────────────────────── pub struct SkillSearchTool { 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/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..b510d2e1c 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}; @@ -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. From d9ae7a41c5f314ecae565a4927bedebbad40cde7 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 25 May 2026 02:04:32 +0200 Subject: [PATCH 04/18] Harden skill file reads against symlink races Open bundled skill files relative to the canonical skill root with Linux `openat2`, rejecting symlinks in every path component before a file descriptor is returned. Fail closed on non-Linux targets rather than using the weaker path-based fallback. Add regression coverage for exact maximum-size reads and intermediate symlink escapes, and tidy review comments in the tool docs and ExecPlan. --- .../1-3-4-skill-read-file-interface.md | 29 ++- src/skills/file_read.rs | 4 +- src/skills/file_read/io.rs | 173 ++++++++++-------- src/skills/file_read/tests.rs | 44 +++++ src/tools/registry/builtins.rs | 2 +- 5 files changed, 174 insertions(+), 78 deletions(-) diff --git a/docs/execplans/1-3-4-skill-read-file-interface.md b/docs/execplans/1-3-4-skill-read-file-interface.md index 685cc727b..1a77c8d97 100644 --- a/docs/execplans/1-3-4-skill-read-file-interface.md +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -116,7 +116,7 @@ The relevant test bases are `src/skills/bundle/tests.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 +`tests/channels/skills_upload.rs`, and end-to-end (e2e) `tests/e2e/scenarios/test_skills.py`. The implementation should load these skills before editing: @@ -311,8 +311,9 @@ conflict in `Decision Log`, and ask for direction. ## Surprises & discoveries -- Observation: `leta workspace add` succeeded, but the first Rust LSP query - failed because `rust-analyzer` was not installed for the active toolchain. +- 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 @@ -455,6 +456,28 @@ conflict in `Decision Log`, and ask for direction. 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 diff --git a/src/skills/file_read.rs b/src/skills/file_read.rs index 4ea3e474e..c94d8bdb0 100644 --- a/src/skills/file_read.rs +++ b/src/skills/file_read.rs @@ -57,7 +57,7 @@ impl SkillReadFileResponse { pub struct SkillReadFileSuccess { /// Canonical skill identifier used for the read. pub skill: String, - /// Normalised bundle-relative path that was read. + /// Normalized bundle-relative path that was read. pub path: String, /// Best-effort media type inferred from the bundle-relative path. pub mime_type: String, @@ -70,7 +70,7 @@ pub struct SkillReadFileSuccess { pub struct SkillReadFileErrorResponse { /// Canonical skill identifier, or the requested identifier for unknown skills. pub skill: String, - /// Requested or normalised bundle-relative path. + /// Requested or normalized bundle-relative path. pub path: String, /// Stable, model-facing error payload. pub error: SkillReadFileError, diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index 1b1f9da3e..fd8437733 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -1,6 +1,6 @@ //! Filesystem resolution and inline decoding for scoped skill file reads. -use std::path::{Path, PathBuf}; +use std::path::Path; use tokio::io::AsyncReadExt; @@ -16,8 +16,8 @@ struct ReadDisplay<'a> { path: String, } -struct CanonicalTarget { - path: PathBuf, +struct OpenedTarget { + file: tokio::fs::File, size: u64, } @@ -30,12 +30,12 @@ pub(super) async fn read_validated_skill_file( path: relative_path.to_string_lossy().replace('\\', "/"), }; - let target = match resolve_and_validate_target(skill.skill_root(), relative_path).await { + 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 { + let content = match read_inline_content(target, relative_path, mime_type.clone()).await { Ok(content) => content, Err(error) => return error_response(&display, error), }; @@ -48,42 +48,18 @@ pub(super) async fn read_validated_skill_file( }) } -async fn resolve_and_validate_target( +async fn open_validated_target( root: &Path, relative_path: &Path, -) -> Result { +) -> Result { let canonical_root = tokio::fs::canonicalize(root) .await .map_err(|_| io_error("Skill root is not readable"))?; - let target = root.join(relative_path); - let metadata = readable_file_metadata(&target).await?; - let canonical_target = tokio::fs::canonicalize(&target) - .await - .map_err(|_| io_error("File is not available for reading"))?; - - if !canonical_target.starts_with(&canonical_root) { - return Err(path_not_readable()); - } - - Ok(CanonicalTarget { - path: canonical_target, - size: metadata.len(), - }) -} - -async fn readable_file_metadata(path: &Path) -> Result { - match tokio::fs::symlink_metadata(path).await { - Ok(metadata) if metadata.file_type().is_symlink() || !metadata.is_file() => { - Err(path_not_readable()) - } - Ok(metadata) => Ok(metadata), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => Err(path_not_readable()), - Err(_) => Err(io_error("File is not available for reading")), - } + open_relative_to_root(&canonical_root, relative_path).await } async fn read_inline_content( - target: &CanonicalTarget, + target: OpenedTarget, relative_path: &Path, mime_type: String, ) -> Result { @@ -95,53 +71,31 @@ async fn read_inline_content( )); } + let size = target.size; let bytes = read_file_contents(target).await?; - parse_utf8_content(bytes, relative_path, target.size, mime_type) + let actual_size = bytes.len() as u64; + if actual_size > MAX_SKILL_READ_FILE_BYTES { + 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: &CanonicalTarget) -> Result, SkillReadFileError> { - let mut file = open_readonly_no_follow(&target.path).await?; - validate_opened_file(&file, target.size).await?; - +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 // MAX_SKILL_READ_FILE_BYTES, which fits in usize on supported platforms. - let mut contents = Vec::with_capacity(target.size.min(MAX_SKILL_READ_FILE_BYTES) as usize); + let mut contents = Vec::with_capacity(size.min(MAX_SKILL_READ_FILE_BYTES) as usize); file.read_to_end(&mut contents) .await .map_err(|_| io_error("File is not available for reading"))?; Ok(contents) } -#[cfg(unix)] -async fn open_readonly_no_follow(path: &Path) -> Result { - tokio::fs::OpenOptions::new() - .read(true) - .custom_flags(libc::O_NOFOLLOW) - .open(path) - .await - .map_err(|error| match error.raw_os_error() { - Some(libc::ELOOP) => path_not_readable(), - _ => io_error("File is not available for reading"), - }) -} - -#[cfg(not(unix))] -async fn open_readonly_no_follow(path: &Path) -> Result { - // Non-Unix platforms do not have the Unix O_NOFOLLOW path used above. - // This fallback relies on the earlier symlink_metadata rejection and the - // later validate_opened_file size check: file size must match stored size - // after open or the open is rejected. TODO: use platform-specific atomic - // no-follow open semantics for non-Unix targets when this tool supports - // those platforms as first-class deployment environments. - tokio::fs::File::open(path) - .await - .map_err(|_| io_error("File is not available for reading")) -} - -async fn validate_opened_file( - file: &tokio::fs::File, - expected_size: u64, -) -> Result<(), SkillReadFileError> { +async fn metadata_for_opened_file(file: &tokio::fs::File) -> Result { let metadata = file .metadata() .await @@ -149,10 +103,85 @@ async fn validate_opened_file( if !metadata.is_file() { return Err(path_not_readable()); } - if metadata.len() != expected_size { - return Err(io_error("File changed while reading")); + Ok(metadata.len()) +} + +#[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 = std::fs::File::open(root).map_err(|_| io_error("Skill root is not readable"))?; + 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(not(target_os = "linux"))] +async fn open_relative_to_root( + _root: &Path, + _relative_path: &Path, +) -> Result { + 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) => { + path_not_readable() + } + Some(libc::ENOSYS | libc::EINVAL) => io_error("Atomic skill file reads are not supported"), + _ => io_error("File is not available for reading"), } - Ok(()) } fn parse_utf8_content( diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index c845123b1..4af5ff1e7 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -63,6 +63,29 @@ async fn reads_bundle_reference_text(skill_read_fixture: SkillReadFixture) { ); } +#[rstest] +#[tokio::test] +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] async fn reads_skill_entrypoint(skill_read_fixture: SkillReadFixture) { @@ -151,6 +174,27 @@ async fn symlink_paths_are_rejected(skill_read_fixture: SkillReadFixture) { assert_error_code(response, SkillReadFileErrorCode::PathNotReadable); } +#[cfg(unix)] +#[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); +} + fn assert_error_code(response: SkillReadFileResponse, expected: SkillReadFileErrorCode) { let SkillReadFileResponse::Error(response) = response else { panic!("expected error response"); diff --git a/src/tools/registry/builtins.rs b/src/tools/registry/builtins.rs index b510d2e1c..00a172a92 100644 --- a/src/tools/registry/builtins.rs +++ b/src/tools/registry/builtins.rs @@ -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( From f2767dd6a9b6d94c13aee3db1402333966181a7c Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 25 May 2026 20:51:51 +0200 Subject: [PATCH 05/18] Bound skill file reads to the inline limit Cap bundle file reads at `MAX_SKILL_READ_FILE_BYTES + 1` so files that grow after metadata validation return the typed oversized-file error before UTF-8 decoding. Open the skill root as a stable directory handle before the Linux `openat2` call, and make the Linux-only behavioural test expectations explicit while non-Linux reads fail closed. --- .../1-3-4-skill-read-file-interface.md | 6 ++++ src/skills/file_read/io.rs | 34 +++++++++++++------ src/skills/file_read/tests.rs | 24 +++++++++++-- 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/docs/execplans/1-3-4-skill-read-file-interface.md b/docs/execplans/1-3-4-skill-read-file-interface.md index 1a77c8d97..a50edace0 100644 --- a/docs/execplans/1-3-4-skill-read-file-interface.md +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -922,3 +922,9 @@ implementation approach. 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`. diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index fd8437733..ed5e9d33f 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -2,7 +2,7 @@ use std::path::Path; -use tokio::io::AsyncReadExt; +use tokio::io::{AsyncReadExt, Take}; use super::validation::{is_asset_path, path_not_readable}; use super::{ @@ -52,10 +52,7 @@ async fn open_validated_target( root: &Path, relative_path: &Path, ) -> Result { - let canonical_root = tokio::fs::canonicalize(root) - .await - .map_err(|_| io_error("Skill root is not readable"))?; - open_relative_to_root(&canonical_root, relative_path).await + open_relative_to_root(root, relative_path).await } async fn read_inline_content( @@ -86,15 +83,21 @@ async fn read_inline_content( 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 - // MAX_SKILL_READ_FILE_BYTES, which fits in usize on supported platforms. - let mut contents = Vec::with_capacity(size.min(MAX_SKILL_READ_FILE_BYTES) as usize); - file.read_to_end(&mut contents) + // 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) } +fn limited_reader(file: &mut tokio::fs::File, limit: u64) -> Take<&mut tokio::fs::File> { + file.take(limit) +} + async fn metadata_for_opened_file(file: &tokio::fs::File) -> Result { let metadata = file .metadata() @@ -116,7 +119,7 @@ async fn open_relative_to_root( use std::os::unix::ffi::OsStrExt; use std::os::unix::io::AsRawFd; - let root = std::fs::File::open(root).map_err(|_| io_error("Skill root is not readable"))?; + let root = open_root_directory(root)?; if !root .metadata() .map_err(|_| io_error("Skill root is not readable"))? @@ -163,6 +166,17 @@ async fn open_relative_to_root( Ok(OpenedTarget { file, size }) } +#[cfg(target_os = "linux")] +fn open_root_directory(root: &Path) -> Result { + use std::os::unix::fs::OpenOptionsExt; + + std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_CLOEXEC | libc::O_DIRECTORY | libc::O_NOFOLLOW) + .open(root) + .map_err(|_| io_error("Skill root is not readable")) +} + #[cfg(not(target_os = "linux"))] async fn open_relative_to_root( _root: &Path, diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index 4af5ff1e7..957ef9a81 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -49,6 +49,7 @@ fn skill_read_fixture() -> SkillReadFixture { #[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; @@ -65,6 +66,7 @@ async fn reads_bundle_reference_text(skill_read_fixture: SkillReadFixture) { #[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 @@ -88,6 +90,7 @@ async fn reads_bundle_reference_text_at_max_size(skill_read_fixture: SkillReadFi #[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; @@ -111,6 +114,7 @@ async fn rejects_disallowed_paths(skill_read_fixture: SkillReadFixture, #[case] #[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; @@ -130,6 +134,7 @@ async fn binary_asset_returns_non_inline_metadata(skill_read_fixture: SkillReadF #[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 @@ -147,13 +152,14 @@ async fn oversized_text_returns_file_too_large(skill_read_fixture: SkillReadFixt #[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(unix)] +#[cfg(target_os = "linux")] #[rstest] #[tokio::test] async fn symlink_paths_are_rejected(skill_read_fixture: SkillReadFixture) { @@ -174,7 +180,7 @@ async fn symlink_paths_are_rejected(skill_read_fixture: SkillReadFixture) { assert_error_code(response, SkillReadFileErrorCode::PathNotReadable); } -#[cfg(unix)] +#[cfg(target_os = "linux")] #[rstest] #[tokio::test] async fn intermediate_symlink_paths_are_rejected(skill_read_fixture: SkillReadFixture) { @@ -195,6 +201,20 @@ async fn intermediate_symlink_paths_are_rejected(skill_read_fixture: SkillReadFi 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"); From 2b2b8debae9f0ca5696982ba6bf040bec3cd7a9d Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 25 May 2026 23:37:25 +0200 Subject: [PATCH 06/18] Warn when skill file reads are unavailable Emit an operator-facing warning on non-Linux targets before returning the fail-closed `skill_read_file` error. Record that the canonical-root comment was stale because the current code no longer canonicalizes the root per read. --- docs/execplans/1-3-4-skill-read-file-interface.md | 5 +++++ src/skills/file_read/io.rs | 3 +++ 2 files changed, 8 insertions(+) diff --git a/docs/execplans/1-3-4-skill-read-file-interface.md b/docs/execplans/1-3-4-skill-read-file-interface.md index a50edace0..0021096b7 100644 --- a/docs/execplans/1-3-4-skill-read-file-interface.md +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -928,3 +928,8 @@ 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/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index ed5e9d33f..47cec73a8 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -182,6 +182,9 @@ 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", )) From 2699241c68ea81e2631f74c44355aca567ac2850 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 00:24:45 +0200 Subject: [PATCH 07/18] Gate skill file metadata helper on Linux Compile `metadata_for_opened_file` only on Linux, matching its only call site in the Linux `openat2` implementation. This keeps non-Linux builds from seeing the helper as dead code without suppressing the lint. --- src/skills/file_read/io.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index 47cec73a8..d2211abd0 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -94,10 +94,7 @@ async fn read_file_contents(target: OpenedTarget) -> Result, SkillReadFi Ok(contents) } -fn limited_reader(file: &mut tokio::fs::File, limit: u64) -> Take<&mut tokio::fs::File> { - file.take(limit) -} - +#[cfg(target_os = "linux")] async fn metadata_for_opened_file(file: &tokio::fs::File) -> Result { let metadata = file .metadata() @@ -109,6 +106,10 @@ async fn metadata_for_opened_file(file: &tokio::fs::File) -> Result Take<&mut tokio::fs::File> { + file.take(limit) +} + #[cfg(target_os = "linux")] async fn open_relative_to_root( root: &Path, From d2ec1a2cdedc267d6914353288dbf843a6df8c5a Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 11:28:33 +0200 Subject: [PATCH 08/18] Add skill read file decision tracing Emit structured tracing at `skill_read_file` validation and read failure points. Include stable skill, path, error, size, limit, and OS error fields without exposing host filesystem paths. --- src/skills/file_read.rs | 10 +++++++++- src/skills/file_read/io.rs | 29 +++++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/skills/file_read.rs b/src/skills/file_read.rs index c94d8bdb0..8ca0a6098 100644 --- a/src/skills/file_read.rs +++ b/src/skills/file_read.rs @@ -169,7 +169,15 @@ pub async fn read_skill_file(skill: &LoadedSkill, requested_path: &str) -> Skill let display_path = display_path(requested_path); let relative_path = match validate_bundle_relative_path(requested_path) { Ok(path) => path, - Err(error) => return SkillReadFileResponse::error(display_skill, &display_path, error), + 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 diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index d2211abd0..a90307544 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -61,6 +61,11 @@ async fn read_inline_content( 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, @@ -72,6 +77,11 @@ async fn read_inline_content( 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, @@ -195,10 +205,17 @@ async fn open_relative_to_root( 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) => io_error("Atomic skill file reads are not supported"), - _ => io_error("File is not available for reading"), + 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") + } } } @@ -223,6 +240,14 @@ fn parse_utf8_content( } 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) } From 6d5bbce2a44ef89df889cb2c84908599bac1d79c Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 11:44:44 +0200 Subject: [PATCH 09/18] Snapshot skill read file response JSON shapes Add insta JSON snapshots for every `SkillReadFileResponse` payload shape. Lock in the untagged success and error layouts, snake_case error codes, and metadata omission for responses without inline metadata. --- ..._skill_read_file_error_file_too_large.snap | 17 ++++ ...s__skill_read_file_error_invalid_utf8.snap | 12 +++ ...tests__skill_read_file_error_io_error.snap | 12 +++ ...kill_read_file_error_non_inline_asset.snap | 17 ++++ ...ill_read_file_error_path_not_readable.snap | 12 +++ ...__skill_read_file_error_unknown_skill.snap | 12 +++ ..._read__tests__skill_read_file_success.snap | 10 ++ src/skills/file_read/tests.rs | 98 +++++++++++++++++++ 8 files changed, 190 insertions(+) create mode 100644 src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_file_too_large.snap create mode 100644 src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_invalid_utf8.snap create mode 100644 src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_io_error.snap create mode 100644 src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_non_inline_asset.snap create mode 100644 src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_path_not_readable.snap create mode 100644 src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_unknown_skill.snap create mode 100644 src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_success.snap 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..6a69c0509 --- /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": "Path is not readable within the skill bundle" + } +} 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 index 957ef9a81..320d9e0ef 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; +use insta::assert_json_snapshot; use proptest::prelude::*; use rstest::{fixture, rstest}; use tempfile::TempDir; @@ -256,3 +257,100 @@ proptest! { fn skill_entrypoint_path_validates() { assert!(validate_bundle_relative_path("SKILL.md").is_ok()); } + +// ── JSON shape snapshot tests ──────────────────────────────────────────────── + +#[test] +fn snapshot_success_response() { + let 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(), + }); + assert_json_snapshot!("skill_read_file_success", &response); +} + +#[test] +fn snapshot_error_unknown_skill() { + let response = SkillReadFileResponse::unknown_skill("deploy-docs", "references/usage.md"); + assert_json_snapshot!("skill_read_file_error_unknown_skill", &response); +} + +#[test] +fn snapshot_error_path_not_readable() { + let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { + skill: "deploy-docs".to_string(), + path: "../secret".to_string(), + error: SkillReadFileError { + code: SkillReadFileErrorCode::PathNotReadable, + message: "Path is not readable within the skill bundle".to_string(), + metadata: None, + }, + }); + assert_json_snapshot!("skill_read_file_error_path_not_readable", &response); +} + +#[test] +fn snapshot_error_non_inline_asset() { + let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { + skill: "deploy-docs".to_string(), + path: "assets/logo.png".to_string(), + error: SkillReadFileError { + code: SkillReadFileErrorCode::NonInlineAsset, + message: "Phase 1 does not return binary or oversized assets inline.".to_string(), + metadata: Some(SkillReadFileMetadata { + size: 8, + mime_type: "image/png".to_string(), + fetch_hint: NON_INLINE_FETCH_HINT.to_string(), + }), + }, + }); + assert_json_snapshot!("skill_read_file_error_non_inline_asset", &response); +} + +#[test] +fn snapshot_error_file_too_large() { + let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { + skill: "deploy-docs".to_string(), + path: "references/large.md".to_string(), + error: SkillReadFileError { + code: SkillReadFileErrorCode::FileTooLarge, + message: "Phase 1 does not return binary or oversized assets inline.".to_string(), + metadata: Some(SkillReadFileMetadata { + size: MAX_SKILL_READ_FILE_BYTES + 1, + mime_type: "text/markdown".to_string(), + fetch_hint: NON_INLINE_FETCH_HINT.to_string(), + }), + }, + }); + assert_json_snapshot!("skill_read_file_error_file_too_large", &response); +} + +#[test] +fn snapshot_error_invalid_utf8() { + let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { + skill: "deploy-docs".to_string(), + path: "references/binary.md".to_string(), + error: SkillReadFileError { + code: SkillReadFileErrorCode::InvalidUtf8, + message: "File is not valid UTF-8 text".to_string(), + metadata: None, + }, + }); + assert_json_snapshot!("skill_read_file_error_invalid_utf8", &response); +} + +#[test] +fn snapshot_error_io_error() { + let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { + skill: "deploy-docs".to_string(), + path: "references/usage.md".to_string(), + error: SkillReadFileError { + code: SkillReadFileErrorCode::IoError, + message: "File is not available for reading".to_string(), + metadata: None, + }, + }); + assert_json_snapshot!("skill_read_file_error_io_error", &response); +} From 481c98a5784c4b7d66185e5c37fb1262ee29f1aa Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 22:50:02 +0200 Subject: [PATCH 10/18] Use runtime validation in skill read snapshots Collapse the `SkillReadFileResponse` JSON shape snapshots into one rstest case table. Build the path-not-readable payload from the real validation helper so the snapshot follows the runtime error message. --- ...ill_read_file_error_path_not_readable.snap | 2 +- src/skills/file_read/tests.rs | 86 ++++++++++--------- 2 files changed, 46 insertions(+), 42 deletions(-) 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 index 6a69c0509..7dda9a076 100644 --- 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 @@ -7,6 +7,6 @@ expression: "&response" "path": "../secret", "error": { "code": "path_not_readable", - "message": "Path is not readable within the skill bundle" + "message": "File is not available for reading" } } diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index 320d9e0ef..ba9e57d0f 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -260,40 +260,51 @@ fn skill_entrypoint_path_validates() { // ── JSON shape snapshot tests ──────────────────────────────────────────────── -#[test] -fn snapshot_success_response() { - let response = SkillReadFileResponse::Success(SkillReadFileSuccess { +#[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(), - }); - assert_json_snapshot!("skill_read_file_success", &response); + }) } -#[test] -fn snapshot_error_unknown_skill() { - let response = SkillReadFileResponse::unknown_skill("deploy-docs", "references/usage.md"); - assert_json_snapshot!("skill_read_file_error_unknown_skill", &response); +fn snapshot_error_unknown_skill() -> SkillReadFileResponse { + SkillReadFileResponse::unknown_skill("deploy-docs", "references/usage.md") } -#[test] -fn snapshot_error_path_not_readable() { - let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { - skill: "deploy-docs".to_string(), - path: "../secret".to_string(), - error: SkillReadFileError { - code: SkillReadFileErrorCode::PathNotReadable, - message: "Path is not readable within the skill bundle".to_string(), - metadata: None, - }, - }); - assert_json_snapshot!("skill_read_file_error_path_not_readable", &response); +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) } -#[test] -fn snapshot_error_non_inline_asset() { - let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { +fn snapshot_error_non_inline_asset() -> SkillReadFileResponse { + SkillReadFileResponse::Error(SkillReadFileErrorResponse { skill: "deploy-docs".to_string(), path: "assets/logo.png".to_string(), error: SkillReadFileError { @@ -305,13 +316,11 @@ fn snapshot_error_non_inline_asset() { fetch_hint: NON_INLINE_FETCH_HINT.to_string(), }), }, - }); - assert_json_snapshot!("skill_read_file_error_non_inline_asset", &response); + }) } -#[test] -fn snapshot_error_file_too_large() { - let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { +fn snapshot_error_file_too_large() -> SkillReadFileResponse { + SkillReadFileResponse::Error(SkillReadFileErrorResponse { skill: "deploy-docs".to_string(), path: "references/large.md".to_string(), error: SkillReadFileError { @@ -323,13 +332,11 @@ fn snapshot_error_file_too_large() { fetch_hint: NON_INLINE_FETCH_HINT.to_string(), }), }, - }); - assert_json_snapshot!("skill_read_file_error_file_too_large", &response); + }) } -#[test] -fn snapshot_error_invalid_utf8() { - let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { +fn snapshot_error_invalid_utf8() -> SkillReadFileResponse { + SkillReadFileResponse::Error(SkillReadFileErrorResponse { skill: "deploy-docs".to_string(), path: "references/binary.md".to_string(), error: SkillReadFileError { @@ -337,13 +344,11 @@ fn snapshot_error_invalid_utf8() { message: "File is not valid UTF-8 text".to_string(), metadata: None, }, - }); - assert_json_snapshot!("skill_read_file_error_invalid_utf8", &response); + }) } -#[test] -fn snapshot_error_io_error() { - let response = SkillReadFileResponse::Error(SkillReadFileErrorResponse { +fn snapshot_error_io_error() -> SkillReadFileResponse { + SkillReadFileResponse::Error(SkillReadFileErrorResponse { skill: "deploy-docs".to_string(), path: "references/usage.md".to_string(), error: SkillReadFileError { @@ -351,6 +356,5 @@ fn snapshot_error_io_error() { message: "File is not available for reading".to_string(), metadata: None, }, - }); - assert_json_snapshot!("skill_read_file_error_io_error", &response); + }) } From b866430aa3d3ee095cc260c0d6b4dafe4e891b59 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 23:05:54 +0200 Subject: [PATCH 11/18] Open skill roots on the blocking pool Move the Linux skill root directory open into `spawn_blocking` so the synchronous `OpenOptions::open` call does not run on the Tokio runtime. Preserve the existing skill-root read error mapping for open and join failures. --- src/skills/file_read/io.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index a90307544..cf420d64a 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -130,7 +130,7 @@ async fn open_relative_to_root( use std::os::unix::ffi::OsStrExt; use std::os::unix::io::AsRawFd; - let root = open_root_directory(root)?; + let root = open_root_directory(root).await?; if !root .metadata() .map_err(|_| io_error("Skill root is not readable"))? @@ -178,14 +178,19 @@ async fn open_relative_to_root( } #[cfg(target_os = "linux")] -fn open_root_directory(root: &Path) -> Result { +async fn open_root_directory(root: &Path) -> Result { use std::os::unix::fs::OpenOptionsExt; - std::fs::OpenOptions::new() - .read(true) - .custom_flags(libc::O_CLOEXEC | libc::O_DIRECTORY | libc::O_NOFOLLOW) - .open(root) - .map_err(|_| io_error("Skill root is not readable")) + 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"))] From 3d08e93ba78436c3d741b242ad54af7b74338aaf Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 23:19:00 +0200 Subject: [PATCH 12/18] Deduplicate skill read snapshot error builders Add a shared helper for constructing `SkillReadFileResponse::Error` snapshot fixtures. Keep existing snapshot names and payloads while reducing repeated error-response boilerplate in the file-read JSON shape tests. --- src/skills/file_read/tests.rs | 100 +++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 45 deletions(-) diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index ba9e57d0f..a55ea7fce 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -260,6 +260,23 @@ fn skill_entrypoint_path_validates() { // ── JSON shape snapshot tests ──────────────────────────────────────────────── +fn snapshot_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, + }, + }) +} + #[rstest] #[case::success("skill_read_file_success", snapshot_success_response())] #[case::unknown_skill("skill_read_file_error_unknown_skill", snapshot_error_unknown_skill())] @@ -300,61 +317,54 @@ fn snapshot_error_unknown_skill() -> SkillReadFileResponse { 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) + let SkillReadFileError { + code, + message, + metadata, + } = error; + snapshot_error_response("../secret", code, &message, metadata) } fn snapshot_error_non_inline_asset() -> SkillReadFileResponse { - SkillReadFileResponse::Error(SkillReadFileErrorResponse { - skill: "deploy-docs".to_string(), - path: "assets/logo.png".to_string(), - error: SkillReadFileError { - code: SkillReadFileErrorCode::NonInlineAsset, - message: "Phase 1 does not return binary or oversized assets inline.".to_string(), - metadata: Some(SkillReadFileMetadata { - size: 8, - mime_type: "image/png".to_string(), - fetch_hint: NON_INLINE_FETCH_HINT.to_string(), - }), - }, - }) + snapshot_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 { - SkillReadFileResponse::Error(SkillReadFileErrorResponse { - skill: "deploy-docs".to_string(), - path: "references/large.md".to_string(), - error: SkillReadFileError { - code: SkillReadFileErrorCode::FileTooLarge, - message: "Phase 1 does not return binary or oversized assets inline.".to_string(), - metadata: Some(SkillReadFileMetadata { - size: MAX_SKILL_READ_FILE_BYTES + 1, - mime_type: "text/markdown".to_string(), - fetch_hint: NON_INLINE_FETCH_HINT.to_string(), - }), - }, - }) + snapshot_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 { - SkillReadFileResponse::Error(SkillReadFileErrorResponse { - skill: "deploy-docs".to_string(), - path: "references/binary.md".to_string(), - error: SkillReadFileError { - code: SkillReadFileErrorCode::InvalidUtf8, - message: "File is not valid UTF-8 text".to_string(), - metadata: None, - }, - }) + snapshot_error_response( + "references/binary.md", + SkillReadFileErrorCode::InvalidUtf8, + "File is not valid UTF-8 text", + None, + ) } fn snapshot_error_io_error() -> SkillReadFileResponse { - SkillReadFileResponse::Error(SkillReadFileErrorResponse { - skill: "deploy-docs".to_string(), - path: "references/usage.md".to_string(), - error: SkillReadFileError { - code: SkillReadFileErrorCode::IoError, - message: "File is not available for reading".to_string(), - metadata: None, - }, - }) + snapshot_error_response( + "references/usage.md", + SkillReadFileErrorCode::IoError, + "File is not available for reading", + None, + ) } From d4fc942068cb73c69128254d8d2d98527cf393f1 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 26 May 2026 23:55:17 +0200 Subject: [PATCH 13/18] Deduplicate skill read error snapshot builders Add a shared helper for direct `SkillReadFileResponse::Error` snapshot fixtures. Keep the validation-derived path-not-readable case separate so it continues to exercise the runtime validation error payload. --- src/skills/file_read/tests.rs | 49 ++++++++++++++++------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index a55ea7fce..81635a354 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -260,23 +260,6 @@ fn skill_entrypoint_path_validates() { // ── JSON shape snapshot tests ──────────────────────────────────────────────── -fn snapshot_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, - }, - }) -} - #[rstest] #[case::success("skill_read_file_success", snapshot_success_response())] #[case::unknown_skill("skill_read_file_error_unknown_skill", snapshot_error_unknown_skill())] @@ -317,16 +300,28 @@ fn snapshot_error_unknown_skill() -> SkillReadFileResponse { fn snapshot_error_path_not_readable() -> SkillReadFileResponse { let error = validate_bundle_relative_path("../secret") .expect_err("traversal path should fail validation"); - let SkillReadFileError { - code, - message, - metadata, - } = error; - snapshot_error_response("../secret", code, &message, metadata) + 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 { - snapshot_error_response( + make_error_response( "assets/logo.png", SkillReadFileErrorCode::NonInlineAsset, "Phase 1 does not return binary or oversized assets inline.", @@ -339,7 +334,7 @@ fn snapshot_error_non_inline_asset() -> SkillReadFileResponse { } fn snapshot_error_file_too_large() -> SkillReadFileResponse { - snapshot_error_response( + make_error_response( "references/large.md", SkillReadFileErrorCode::FileTooLarge, "Phase 1 does not return binary or oversized assets inline.", @@ -352,7 +347,7 @@ fn snapshot_error_file_too_large() -> SkillReadFileResponse { } fn snapshot_error_invalid_utf8() -> SkillReadFileResponse { - snapshot_error_response( + make_error_response( "references/binary.md", SkillReadFileErrorCode::InvalidUtf8, "File is not valid UTF-8 text", @@ -361,7 +356,7 @@ fn snapshot_error_invalid_utf8() -> SkillReadFileResponse { } fn snapshot_error_io_error() -> SkillReadFileResponse { - snapshot_error_response( + make_error_response( "references/usage.md", SkillReadFileErrorCode::IoError, "File is not available for reading", From ecba24d96a57be135b33facafeb7c2618692f951 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 28 May 2026 00:31:07 +0200 Subject: [PATCH 14/18] Gate path_not_readable import to Linux in skill file read I/O `path_not_readable` is only referenced from `#[cfg(target_os = "linux")]` blocks. Import it under the same cfg so Windows builds do not report an unused import while `is_asset_path` stays available for `parse_utf8_content`. Co-authored-by: Cursor --- src/skills/file_read/io.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index cf420d64a..91b8dd8b1 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -4,7 +4,9 @@ use std::path::Path; use tokio::io::{AsyncReadExt, Take}; -use super::validation::{is_asset_path, path_not_readable}; +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, From d30728990661a6d85fd9d9f913a596bb9796e25a Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 28 May 2026 00:40:00 +0200 Subject: [PATCH 15/18] Address skill_read_file review: docs, proptests, and tracing Document `skills::file_read`, replace the vacuous path-validation proptest with three targeted rejection cases, and log unknown-skill lookups at debug level while correcting the skill tools module count. Co-authored-by: Cursor --- src/skills/file_read/tests.rs | 35 ++++++++++++++++++++------------ src/skills/mod.rs | 1 + src/tools/builtin/skill_tools.rs | 11 ++++++++-- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index 81635a354..0ec23a6c3 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -237,19 +237,28 @@ proptest! { } #[test] - fn disallowed_generated_paths_do_not_validate(raw in "\\PC*") { - let looks_allowed = raw == "SKILL.md" - || raw.starts_with("references/") - || raw.starts_with("assets/"); - - if !looks_allowed - || raw.contains("..") - || raw.contains('\\') - || raw.starts_with('/') - || raw.trim().is_empty() - { - prop_assert!(validate_bundle_relative_path(&raw).is_err()); - } + 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()); } } diff --git a/src/skills/mod.rs b/src/skills/mod.rs index 2028607cd..6a5738b26 100644 --- a/src/skills/mod.rs +++ b/src/skills/mod.rs @@ -18,6 +18,7 @@ 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. diff --git a/src/tools/builtin/skill_tools.rs b/src/tools/builtin/skill_tools.rs index 08e88534e..7e24fedc0 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; @@ -196,7 +196,14 @@ impl NativeTool for SkillReadFileTool { let response = match self.lookup_skill(skill_name)? { Some(skill) => read_skill_file(&skill, path).await, - None => SkillReadFileResponse::unknown_skill(skill_name, path), + None => { + tracing::debug!( + skill_id = %skill_name, + path = %path, + "skill_read_file: skill not found in registry" + ); + SkillReadFileResponse::unknown_skill(skill_name, path) + } }; let result = serde_json::to_value(response).map_err(|error| { From 427904b4c6ec66801ce7f6dfd01973f1554c164a Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 28 May 2026 11:36:02 +0200 Subject: [PATCH 16/18] Add tracing and expand proptests for skill file read Add `tracing::debug!` at the `InvalidUtf8` error creation site with `error_code` and `path` so UTF-8 decode failures are observable before they reach the generic error response. Add success and failure tracing at the `SkillReadFileTool` boundary with `skill_id`, `path`, `duration_ms`, and, for errors, the machine-readable `error_code`, and for successes, the inline content `size`. Expand the `disallowed_generated_paths` property tests to cover absolute paths (leading `/`), alternate separators (`\\`), bare `..` and double-traversal paths, and `CurDir`-leading patterns. Add proptests for the UTF-8 size boundary (`0..=MAX_SKILL_READ_FILE_BYTES`) and for sizes that exceed the cap. `Path::new` normalises `./` within a path away on Linux, so a mid-path `CurDir` component is not observable through `Path::components`. The new tests target the forms that are observable: leading `./`, standalone `.`, `..` variants, backslashes, and absolute paths. --- src/skills/file_read/io.rs | 15 +++++-- src/skills/file_read/tests.rs | 70 ++++++++++++++++++++++++++++++++ src/tools/builtin/skill_tools.rs | 24 ++++++++++- 3 files changed, 104 insertions(+), 5 deletions(-) diff --git a/src/skills/file_read/io.rs b/src/skills/file_read/io.rs index 91b8dd8b1..1a4209be3 100644 --- a/src/skills/file_read/io.rs +++ b/src/skills/file_read/io.rs @@ -239,10 +239,17 @@ fn parse_utf8_content( size, mime_type, )), - Err(_) => Err(SkillReadFileError::new( - SkillReadFileErrorCode::InvalidUtf8, - "File is not valid UTF-8 text", - )), + 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", + )) + } } } diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index 0ec23a6c3..efdc40300 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -260,6 +260,76 @@ proptest! { 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.unwrap().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] diff --git a/src/tools/builtin/skill_tools.rs b/src/tools/builtin/skill_tools.rs index 7e24fedc0..8992ae4e4 100644 --- a/src/tools/builtin/skill_tools.rs +++ b/src/tools/builtin/skill_tools.rs @@ -206,10 +206,32 @@ impl NativeTool for SkillReadFileTool { } }; + 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, start.elapsed())) + Ok(ToolOutput::success(result, elapsed)) } } From 109317b245ec0a64a6f2f4c95ca9d2da232cd0ad Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 29 May 2026 01:06:11 +0200 Subject: [PATCH 17/18] Extract SkillReadFileTool into own submodule, improve proptest diagnostics Replace `unwrap()` with `expect("failed to decode bytes as UTF-8")` in the `utf8_boundary_size_succeeds` proptest so failure output includes the decoding failure reason rather than a bare panic message. Move `SkillReadFileTool` into `src/tools/builtin/skill_tools/read_file.rs`, mirroring the existing `install.rs` and `remove.rs` submodule pattern. Keeps the parent module under the repository file-size ceiling and groups related code in a discoverable layout. --- src/skills/file_read/tests.rs | 5 +- src/tools/builtin/skill_tools.rs | 103 +------------------ src/tools/builtin/skill_tools/read_file.rs | 111 +++++++++++++++++++++ 3 files changed, 117 insertions(+), 102 deletions(-) create mode 100644 src/tools/builtin/skill_tools/read_file.rs diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index efdc40300..ecc497dd2 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -322,7 +322,10 @@ proptest! { 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.unwrap().len(), size as usize); + prop_assert_eq!( + utf8_check.expect("failed to decode bytes as UTF-8 in utf8_check").len(), + size as usize + ); } #[test] diff --git a/src/tools/builtin/skill_tools.rs b/src/tools/builtin/skill_tools.rs index 8992ae4e4..f06903b12 100644 --- a/src/tools/builtin/skill_tools.rs +++ b/src/tools/builtin/skill_tools.rs @@ -9,16 +9,17 @@ use std::sync::Arc; use crate::context::JobContext; use crate::skills::LoadedSkill; use crate::skills::catalog::{CatalogEntry, SkillCatalog}; -use crate::skills::file_read::{SkillReadFileResponse, read_skill_file}; 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. @@ -135,106 +136,6 @@ impl NativeTool for SkillListTool { } } -// ── skill_read_file ───────────────────────────────────────────────────── - -pub struct SkillReadFileTool { - registry: Arc>, -} - -impl SkillReadFileTool { - 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)) - } -} - // ── skill_search ──────────────────────────────────────────────────────── pub struct SkillSearchTool { 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)) + } +} From fc4e348edfe0bc840fe1167c1967562661b20a0d Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 29 May 2026 01:22:07 +0200 Subject: [PATCH 18/18] Strengthen SKILL.md test assertion, mark ExecPlan complete Replace the weak `matches!(Success(_))` assertion in `reads_skill_entrypoint` with full equality checks on `skill`, `path`, `mime_type`, and `content` so regressions in the entrypoint read path are caught at the field level. Mark the 1.3.4 ExecPlan status as COMPLETE to match the completed implementation. --- docs/execplans/1-3-4-skill-read-file-interface.md | 2 +- src/skills/file_read/tests.rs | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/execplans/1-3-4-skill-read-file-interface.md b/docs/execplans/1-3-4-skill-read-file-interface.md index 0021096b7..45f836c66 100644 --- a/docs/execplans/1-3-4-skill-read-file-interface.md +++ b/docs/execplans/1-3-4-skill-read-file-interface.md @@ -5,7 +5,7 @@ This ExecPlan (execution plan) is a living document. The sections `Surprises & Discoveries`, `Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work proceeds. -Status: IN PROGRESS +Status: COMPLETE ## Purpose / big picture diff --git a/src/skills/file_read/tests.rs b/src/skills/file_read/tests.rs index ecc497dd2..de1eaf1a4 100644 --- a/src/skills/file_read/tests.rs +++ b/src/skills/file_read/tests.rs @@ -95,7 +95,15 @@ async fn reads_bundle_reference_text_at_max_size(skill_read_fixture: SkillReadFi async fn reads_skill_entrypoint(skill_read_fixture: SkillReadFixture) { let response = read_skill_file(&skill_read_fixture.skill, "SKILL.md").await; - assert!(matches!(response, SkillReadFileResponse::Success(_))); + 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]