Repository navigation
Conversation
github-actions
Bot
added
T-test
|
@gocamille is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
81be0d8 to
a17dc19
Compare
a17dc19 to
31c52d7
Compare
31c52d7 to
e8b5386
Compare
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct CatalogFile { |
There was a problem hiding this comment.
The snapshot stores the full contents of every file, not just the metadata. That uses more memory, but resources/read always returns the exact bytes used to calculate the digests, even if a file changes on disk after it’s loaded.
|
|
||
| impl Snapshot { | ||
| fn load(root: &Path) -> Result<Self, SkillCatalogError> { | ||
| let handle = Dir::open_ambient_dir(root, ambient_authority()).map_err(|source| { |
There was a problem hiding this comment.
Every listing and read goes through this root handle, cap-std, never through a joined path. Checking file types while walking the directory and then reading files later with std::fs left a window where a file or folder could be replaced with a symlink, which might lead outside the root. cap-std prevents access outside the handle on both Unix and Windows.
| files.sort_by(|a, b| a.uri.cmp(&b.uri)); | ||
|
|
||
| let total: u64 = files.iter().map(|file| file.size).sum(); | ||
| if files.len() > SKILL_FILE_LIMIT || total > SKILL_BYTE_LIMIT { |
There was a problem hiding this comment.
“Over the limit” means the skill is skipped, not just flagged with a warning. The spec says servers SHOULD NOT serve it, since conforming hosts aren’t required to load it. If people need to serve larger skills, we can add an opt-in later.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e8b5386c3d
ℹ️ 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".
| let bytes = handle | ||
| .read(&entry.path) |
There was a problem hiding this comment.
When a skill contains a very large file or an excessive number of files, loading reads and retains every file in bytes_of before build_skill checks SKILL_BYTE_LIMIT and SKILL_FILE_LIMIT. A skill containing a multi-gigabyte asset can therefore exhaust the server's memory during startup or reload instead of being skipped at the documented 16 MiB limit; even skills that will subsequently be rejected are fully loaded. Check file counts before loading and enforce the byte budget with bounded reads so oversized skills cannot exhaust memory before validation.
Useful? React with 👍 / 👎.
Motivation and Context
Follows #1392.
It adds
SkillCatalogbehind a newskills-fsfeature. It loads each folder under a root that contains aSKILL.md, then handlesskills/list,skills/get,resources/read, andresources/directory/readusing an in-memory snapshot. The module docs explain how to connect it to aServerHandler. I also added inline comments explaining the design decisions.How Has This Been Tested?
Add tests
Breaking Changes
None.
Types of changes
Checklist