Implement bundled skill file reads (1.3.4)#188
Conversation
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.
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.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughImplement the read-only ChangesScoped Skill File Read Feature
Sequence Diagram(s)sequenceDiagram
participant Model
participant SkillReadFileTool
participant SkillRegistry
participant read_skill_file
participant Filesystem
Model->>SkillReadFileTool: skill_read_file(skill, path)
SkillReadFileTool->>SkillRegistry: lookup(skill_name)
alt skill exists
SkillRegistry-->>SkillReadFileTool: LoadedSkill
SkillReadFileTool->>read_skill_file: read_skill_file(LoadedSkill, path)
read_skill_file->>read_skill_file: validate_bundle_relative_path(path)
alt validation fails
read_skill_file-->>SkillReadFileTool: SkillReadFileResponse::Error
else validation succeeds
read_skill_file->>Filesystem: open & read scoped path
Filesystem-->>read_skill_file: Success or Error metadata
read_skill_file-->>SkillReadFileTool: SkillReadFileResponse
end
SkillReadFileTool-->>Model: JSON tool output
else skill not found
SkillReadFileTool-->>Model: unknown_skill JSON error
end
Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 4 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
scope: docs
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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`.
github-actions
Bot
added
size: XL
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideAdds a new read-only, skill-scoped skill_read_file interface that lets the model read specific text files from bundled skills via a dedicated domain policy module, a new builtin tool adapter, and updated registration/attenuation, along with comprehensive tests and documentation updates to mark roadmap item 1.3.4 complete. Sequence diagram for the new skill_read_file tool executionsequenceDiagram
actor Model
participant SkillReadFileTool
participant SkillRegistry
participant LoadedSkill
participant FileReadPolicy as skills_file_read
Model->>SkillReadFileTool: execute(params, ctx)
SkillReadFileTool->>SkillReadFileTool: require_str(params, "skill") / require_str(params, "path")
SkillReadFileTool->>SkillRegistry: lookup_skill(skill_name)
SkillRegistry-->>SkillReadFileTool: Option<LoadedSkill>
alt skill found
SkillReadFileTool->>LoadedSkill: clone()
SkillReadFileTool->>FileReadPolicy: read_skill_file(loaded_skill, path)
FileReadPolicy->>FileReadPolicy: validate_bundle_relative_path(path)
FileReadPolicy->>FileReadPolicy: read_validated_skill_file(skill, relative_path)
FileReadPolicy-->>SkillReadFileTool: SkillReadFileResponse
else unknown skill
SkillReadFileTool->>FileReadPolicy: SkillReadFileResponse::unknown_skill(skill_name, path)
FileReadPolicy-->>SkillReadFileTool: SkillReadFileResponse
end
SkillReadFileTool->>SkillReadFileTool: serde_json::to_value(response)
SkillReadFileTool-->>Model: ToolOutput::success(result, elapsed)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbfd0f30ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/skills/file_read/io.rs`:
- Around line 60-66: The current sequence canonicalize + starts_with on
canonical_target followed by reopening by pathname (see canonical_target,
canonical_root and read_file_contents) creates a TOCTOU and allows symlink
escapes in intermediate path components; change the implementation to open files
atomically relative to the validated skill root handle instead of re-opening by
path: obtain a handle/dirfd for the validated root (the canonical_root result),
then open the requested target via an anchored open that forbids symlinks in
every component (on Unix use openat2 with RESOLVE_NO_SYMLINKS or equivalent; on
non-Unix use a directory-capability crate like cap_std to open relative to the
root handle), and have read_file_contents accept/read from the opened file
descriptor/handle rather than reopening by pathname. Ensure all path resolution
and symlink checks happen inside that single atomic open call so there is no
check-then-open window.
In `@src/tools/registry/builtins.rs`:
- Around line 244-250: Update the method doc comment to reflect the five
registered skill management tools by adding "read_file" to the summary;
specifically, adjust the docstring that documents the behavior of the function
which calls self.register_sync with SkillReadFileTool, SkillInstallTool,
SkillRemoveTool, etc., so it reads something like "Register skill management
tools (list, search, read_file, install, remove)" to match the actual
registrations (symbols: register_sync, SkillReadFileTool, SkillInstallTool,
SkillRemoveTool).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c7e8f5e-2a9a-475e-ac23-2850a9ee517c
📒 Files selected for processing (18)
FEATURE_PARITY.mddocs/agent-skills-support.mddocs/axinite-architecture-overview.mddocs/developers-guide.mddocs/execplans/1-3-4-skill-read-file-interface.mddocs/roadmap.mddocs/users-guide.mdsrc/skills/attenuation.rssrc/skills/file_read.rssrc/skills/file_read/io.rssrc/skills/file_read/tests.rssrc/skills/file_read/validation.rssrc/skills/mod.rssrc/tools/builtin/mod.rssrc/tools/builtin/skill_tools.rssrc/tools/builtin/skill_tools/features/skill_read_file.featuresrc/tools/builtin/skill_tools/tests.rssrc/tools/registry/builtins.rs
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.
github-actions
Bot
removed
the
risk: low
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/skills/file_read/io.rs (1)
51-59:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftEliminate the remaining root-handle race.
tokio::fs::canonicalize(root)on Lines 55-58 is followed by
std::fs::File::open(root)on Line 119, so the bundle root is still
re-resolved by pathname. A concurrent rename or symlink swap of the root between
those steps can retarget the dirfd and make the lateropenat2checks operate
inside a different directory tree. Anchor the read to a stable root handle
before any attacker-controlled re-resolution.Also applies to: 109-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skills/file_read/io.rs` around lines 51 - 59, The code currently canonicalizes the path then later re-opens the root by pathname, leaving a TOCTOU where a concurrent rename/symlink swap can change the directory target; to fix, open a stable directory handle for the bundle root up-front (e.g., open a std::fs::File with O_DIRECTORY/Read-only) inside open_validated_target and use that handle (its fd) for subsequent validation and for calls into open_relative_to_root/open_relative_to_root_by_fd instead of re-resolving the path; replace the tokio::fs::canonicalize(root) + later std::fs::File::open(root) sequence by opening the root dir first, perform fstat/canonicalization from the opened handle (or use fd-based checks), and pass the handle/fd into open_relative_to_root / the code that currently calls std::fs::File::open so all checks and openat/openat2 operations are anchored to the stable root handle (affecting open_validated_target, open_relative_to_root, and the later code around the existing std::fs::File::open call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/skills/file_read/io.rs`:
- Around line 74-95: The current read_file_contents (and the caller flow) uses
file.read_to_end which can exhaust memory if the file grows after the initial
metadata check; change read_file_contents to read at most
MAX_SKILL_READ_FILE_BYTES + 1 bytes (set Vec capacity to that bound) and stop as
soon as that extra byte is read so you can detect growth early; if the buffer
length exceeds MAX_SKILL_READ_FILE_BYTES return the typed FileTooLarge error
(using SkillReadFileErrorCode::FileTooLarge and the observed actual_size)
instead of proceeding to parse_utf8_content; keep references to OpenedTarget and
the size field for capacity decisions and ensure parse_utf8_content is only
called when the size is within the allowed limit.
In `@src/skills/file_read/tests.rs`:
- Around line 66-87: The failing tests assume Linux-specific behavior from
open_relative_to_root in src/skills/file_read/io.rs (which returns
SkillReadFileErrorCode::IoError on non-linux), so update the tests: add
#[cfg(target_os = "linux")] to reads_bundle_reference_text_at_max_size and to
the symlink-related tests (intermediate_symlink_paths_are_rejected and
symlink_paths_are_rejected) so they only expect Success/PathNotReadable on
Linux, or alternatively add conditional branches in those tests that assert the
non-Linux branch returns SkillReadFileErrorCode::IoError (referencing the test
functions reads_bundle_reference_text_at_max_size,
intermediate_symlink_paths_are_rejected, symlink_paths_are_rejected and the
error enum SkillReadFileErrorCode/open_relative_to_root) to make platform
behavior explicit.
---
Duplicate comments:
In `@src/skills/file_read/io.rs`:
- Around line 51-59: The code currently canonicalizes the path then later
re-opens the root by pathname, leaving a TOCTOU where a concurrent
rename/symlink swap can change the directory target; to fix, open a stable
directory handle for the bundle root up-front (e.g., open a std::fs::File with
O_DIRECTORY/Read-only) inside open_validated_target and use that handle (its fd)
for subsequent validation and for calls into
open_relative_to_root/open_relative_to_root_by_fd instead of re-resolving the
path; replace the tokio::fs::canonicalize(root) + later
std::fs::File::open(root) sequence by opening the root dir first, perform
fstat/canonicalization from the opened handle (or use fd-based checks), and pass
the handle/fd into open_relative_to_root / the code that currently calls
std::fs::File::open so all checks and openat/openat2 operations are anchored to
the stable root handle (affecting open_validated_target, open_relative_to_root,
and the later code around the existing std::fs::File::open call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 555701a4-605f-4ebd-b58b-fd7c0184572e
📒 Files selected for processing (5)
docs/execplans/1-3-4-skill-read-file-interface.mdsrc/skills/file_read.rssrc/skills/file_read/io.rssrc/skills/file_read/tests.rssrc/tools/registry/builtins.rs
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/skills/file_read/io.rs`:
- Around line 170-179: open_root_directory currently calls the blocking
std::fs::OpenOptions::open synchronously which can block the Tokio runtime; make
open_root_directory async and perform the
OpenOptions::new().read(true).custom_flags(...).open(root) inside
tokio::task::spawn_blocking (await the JoinHandle), then map the result to
SkillReadFileError via io_error like before. Update the function signature
(open_root_directory -> async fn open_root_directory(...)) and ensure errors
from the spawned task are propagated/mapped into io_error so callers of
open_root_directory receive the same SkillReadFileError on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0268ea47-b032-4540-9264-213d34f7d431
📒 Files selected for processing (3)
docs/execplans/1-3-4-skill-read-file-interface.mdsrc/skills/file_read/io.rssrc/skills/file_read/tests.rs
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/skills/file_read/tests.rs`:
- Around line 281-291: The test snapshot_error_path_not_readable currently
constructs a hard-coded SkillReadFileResponse/SkillReadFileErrorResponse with
message "Path is not readable within the skill bundle"; instead call or reuse
the real helper (read_skill_file or the validation helper in
src/skills/file_read/validation.rs) with the same input ("../secret") to
generate the actual SkillReadFileResponse payload and assert_json_snapshot!
against that result so the snapshot reflects the runtime message ("File is not
available for reading") and full metadata from the real implementation.
- Around line 274-278: Multiple near-duplicate tests in
src/skills/file_read/tests.rs (e.g., snapshot_error_unknown_skill using
SkillReadFileResponse::unknown_skill) should be collapsed into a single
parameterized test: replace the repeated individual #[test] functions with an
#[rstest] table (or a helper that yields (snapshot_name, response)) that
enumerates each case name and the corresponding SkillReadFileResponse variant,
then call assert_json_snapshot! inside the single test using the parameterized
snapshot_name and response; update imports to include rstest macros if missing
and remove the old duplicate test functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ebf7a742-38db-42c8-8869-367faf72f2ef
⛔ Files ignored due to path filters (7)
src/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_file_too_large.snapis excluded by!**/*.snapsrc/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_invalid_utf8.snapis excluded by!**/*.snapsrc/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_io_error.snapis excluded by!**/*.snapsrc/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_non_inline_asset.snapis excluded by!**/*.snapsrc/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_path_not_readable.snapis excluded by!**/*.snapsrc/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_error_unknown_skill.snapis excluded by!**/*.snapsrc/skills/file_read/snapshots/ironclaw__skills__file_read__tests__skill_read_file_success.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/skills/file_read.rssrc/skills/file_read/io.rssrc/skills/file_read/tests.rs
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +345 to +356 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);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/skills/file_read/tests.rs: What lead to degradation?The module contains 4 functions with similar structure: snapshot_error_file_too_large,snapshot_error_invalid_utf8,snapshot_error_io_error,snapshot_error_non_inline_asset Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +345 to +356 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);
}❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Current state of snapshot tests in src/skills/file_read/tests.rs ==="
cat -n src/skills/file_read/tests.rsLength of output: 14814 File: The fix is already in place. The current branch (lines 263–278) defines The CodeScene finding was generated against the diff view of the original flat implementations, not the current branch state. Use the following message to suppress it in the CodeScene UI: The four functions flagged by this diagnostic — The remaining structural similarity across the builder functions is intentional: each constructs a distinct, named snapshot fixture with different fields, and the shared helper already eliminates the behavioural duplication that the DRY principle targets. Further consolidation (for example, a table-driven approach) would obscure the distinct fixture identities required by the |
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/skills/file_read/tests.rs: What lead to degradation?The module contains 2 functions with similar structure: snapshot_error_file_too_large,snapshot_error_non_inline_asset Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Current snapshot builder functions ==="
cat -n src/skills/file_read/tests.rsLength of output: 14607 File: AssessmentFurther refactoring here would be deleterious. Both functions already delegate to the
Extracting a further CodeScene suppression message
|
|
@coderabbitai Please investigate the cause of the following lint violations in the Windows branch of the CI matrix using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== src/skills/file_read/io.rs — full file with line numbers ==="
cat -n src/skills/file_read/io.rs
echo ""
echo "=== All uses of path_not_readable in io.rs ==="
rg -n "path_not_readable" src/skills/file_read/io.rs
echo ""
echo "=== All uses of is_asset_path in io.rs ==="
rg -n "is_asset_path" src/skills/file_read/io.rsLength of output: 12188 File: Root causeEvery call to
The fix is to split the single import into two lines — one unconditional and one cfg-gated: use super::validation::is_asset_path;
#[cfg(target_os = "linux")]
use super::validation::path_not_readable; |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
This branch implements roadmap task
(1.3.4)by adding a read-onlyskill_read_fileinterface for bundled skill resources. It gives the model a narrow, skill-scoped way to readSKILL.md,references/**, and allowed text assets from loaded bundles without exposing raw filesystem access.Roadmap task:
(1.3.4)Execplan: docs/execplans/1-3-4-skill-read-file-interface.md
Review walkthrough
rstest-bddcoverage.Validation
cargo test --features test-helpers --lib skills::file_read -- --nocapture: passed, 17 tests.cargo test --features test-helpers --lib skill -- --nocapture: passed, 270 tests.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 FEATURE_PARITY.md: passed with 0 errors.git diff --check: passed.make check-fmt: passed.make lint: passed.make test: passed, including 4091 nextest tests and 5 GitHub tool tests.make all: passed after the final source and ExecPlan updates, including 4091 nextest tests and 5 GitHub tool tests.coderabbit review --agent: actionable findings were addressed. The final confirmation retry is blocked by CodeRabbit service rate limits after the last non-Unix fallback comment; see the ExecPlan notes for the command logs.Notes
The non-Unix file-open fallback documents its current mitigation explicitly: it relies on the pre-open
symlink_metadatarejection and the post-open size revalidation, with a TODO for platform-specific atomic no-follow semantics if non-Unix deployments become first-class for this tool.No Python e2e test was added because this branch changes a model-facing built-in tool contract, not a new HTTP route, CLI command, persistence workflow, UI flow, or network boundary. The Rust adapter and
rstest-bddscenarios cover the externally visible tool behaviour.References
Summary by Sourcery
Add a read-only
skill_read_filetool and underlying skill-scoped file read subsystem so models can read specific bundled skill resources via bundle-relative paths without exposing host filesystem details.New Features:
skill_read_filebuilt-in tool that reads allowed text files from loaded skill bundles using a strict JSON schema.skills::file_readmodule that enforces bundle-relative path policy, scoped filesystem I/O, and structured success/error responses for skill file reads.Enhancements:
skill_read_filein the tool registry and include it in the installed-skill read-only attenuation set.Documentation:
skill_read_fileinterface.Tests:
rstest-bdd) tests covering path validation, inline text reads, non-inline assets, error codes, and symlink/traversal rejection for skill bundle file reads.skill_read_filetool schema, execution behaviour, and BDD scenarios for allowed reads and denied filesystem traversal.