Repository navigation
Conversation
|
@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.
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. |
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.
Enforce skill limits before reading all file contents
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 👍 / 👎.
tsarlandie-oai
left a comment
There was a problem hiding this comment.
LGTM - Left some minor comments.
| uri | ||
| } | ||
|
|
||
| /// Whether files of this type are sent as `text` rather than a base64 `blob`. |
There was a problem hiding this comment.
We read and send 'text' files as utf8 but some of these file formats (xml) have their own encoding definition. It might not be utf8 and this might lead to us breaking encoding.
There was a problem hiding this comment.
@tsarlandie-oai We never transcode. A file is sent as text only if its bytes are valid UTF-8. If it’s UTF-16 with a BOM or Latin-1 with non-ASCII characters, we send it as a base64 blob. Either way, the host gets the exact bytes covered by the digest.
| ) | ||
| } | ||
|
|
||
| fn mime_type_of(path: &str) -> &'static str { |
There was a problem hiding this comment.
Maybe we could add a dependency on mime_guess?
It also uses a hardcoded list of mime types but it's much longer and we are less likely to make new releases of rmcp just to update the list ;)
There was a problem hiding this comment.
Thanks for the suggestion. I looked into it, but mime_guess maps .ts to video/vnd.dlna.mpeg-tts, so TypeScript scripts in a skill would be sent as blobs. The table only decides whether something is text or a blob, and unknown types just omit mimeType, so I'd keep the list short.
| Some("webp") => "image/webp", | ||
| Some("pdf") => "application/pdf", | ||
| Some("zip") => "application/zip", | ||
| _ => "application/octet-stream", |
There was a problem hiding this comment.
I believe mimeType is optional in the spec so maybe it would be better to return None here and not include it if we are not sure.
There was a problem hiding this comment.
Good call! An unknown extension now leaves out mimeType and is still sent as a blob.
| .collect(); | ||
| files.sort_by(|a, b| a.uri.cmp(&b.uri)); | ||
|
|
||
| check_limits(files.len(), files.iter().map(|file| file.size).sum())?; |
There was a problem hiding this comment.
nit: we could check the size limit as we go and before reading the files so that we don't exceed it.
| Err(name) => (name.to_string_lossy().into_owned(), false), | ||
| }; | ||
| if name.starts_with('.') { | ||
| continue; |
There was a problem hiding this comment.
does the SEP specify that these should be skipped?
There was a problem hiding this comment.
No, @jamadeo, the SEP doesn't cover this. It's a catalog decision to keep .git, .DS_Store, and similar files out of skills. Hidden files aren't served at all, so the manifest stays complete. I've added this to the module docs.
61ebd49 to
b9f8c43
Compare
5f7cf89 to
58f424a
Compare
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.How Has This Been Tested?
Add tests
Breaking Changes
None.
Types of changes
Checklist