diff --git a/crates/agent_skills/agent_skills.rs b/crates/agent_skills/agent_skills.rs index 6a78346c380..7f0d863fc91 100644 --- a/crates/agent_skills/agent_skills.rs +++ b/crates/agent_skills/agent_skills.rs @@ -4,7 +4,6 @@ use fs::Fs; use futures::StreamExt; use gpui::{Global, SharedString}; use serde::{Deserialize, Serialize}; -use std::io::{self, Read}; use std::path::{Path, PathBuf}; use std::sync::Arc; use util::paths::component_matches_ignore_ascii_case; @@ -41,7 +40,6 @@ pub struct SkillScopeId(pub usize); /// entries would fan out an equally large number of concurrent OS-level I/O /// operations, potentially exhausting file descriptors or stalling the app. const SKILL_IO_CONCURRENCY: usize = 16; -const SKILL_READ_CHUNK_SIZE: usize = 4096; /// Maximum size for a single SKILL.md file (100KB) pub const MAX_SKILL_FILE_SIZE: usize = 100 * 1024; @@ -558,53 +556,15 @@ async fn find_skill_files(fs: &Arc, directory: &Path) -> Vec { .await } -/// Returns the byte index ONE PAST the end of the closing frontmatter -/// delimiter line in `bytes`, or `None` if no closing delimiter has been -/// seen yet. Used by the chunked reader to know when it has enough -/// bytes to stop pulling from disk. +/// Read `skill_file_path` from disk and parse its frontmatter. The +/// SKILL.md body is parsed away by `parse_skill_frontmatter` and not +/// surfaced here; it's re-read on demand via `read_skill_body` when a +/// skill is actually being loaded for the model. /// -/// Scans for the first `\n---` line followed by `\n`, `\r\n`, or EOF -/// (excluding the opening line itself, which sits at byte 0 and is -/// naturally skipped because we only consider lines following a `\n`). -/// This may overshoot in pathological cases (e.g. `---` inside a quoted -/// YAML string), but `parse_skill_frontmatter`'s candidate-and-validate -/// logic still produces a correct result or a YAML parse error. -fn closing_delimiter_end(bytes: &[u8]) -> Option { - for (i, &b) in bytes.iter().enumerate() { - if b != b'\n' { - continue; - } - let line_start = i + 1; - if line_start + 3 > bytes.len() { - continue; - } - if &bytes[line_start..line_start + 3] != b"---" { - continue; - } - let after_dashes = line_start + 3; - if after_dashes == bytes.len() { - return Some(after_dashes); - } - if bytes[after_dashes] == b'\n' { - return Some(after_dashes + 1); - } - if after_dashes + 1 < bytes.len() - && bytes[after_dashes] == b'\r' - && bytes[after_dashes + 1] == b'\n' - { - return Some(after_dashes + 2); - } - // Line is `---trailing` or `----`; keep scanning. - } - None -} - -/// Read just enough of `skill_file_path` from disk to parse its -/// frontmatter. The SKILL.md body is NOT loaded — that's deferred to -/// `read_skill_body`, called only when a skill is actually being -/// materialized for the model. Reading in 4KB chunks keeps the peak -/// memory cost of loading N skills proportional to total frontmatter -/// size, not total file size. +/// We load the whole file in one go rather than streaming up to the +/// closing `---`. `MAX_SKILL_FILE_SIZE` is 100KB and the metadata check +/// below caps the worst case at that, so the peak transient cost is +/// trivially small (≤ `MAX_SKILL_FILE_SIZE` × `SKILL_IO_CONCURRENCY`). pub async fn load_skill_frontmatter( fs: Arc, skill_file_path: PathBuf, @@ -612,10 +572,15 @@ pub async fn load_skill_frontmatter( ) -> Result { // Short-circuit on oversized files before reading any of their // contents, so a stray multi-GB file named `SKILL.md` can't OOM the - // app. We only act on a positive signal that the file is too large; - // if metadata fails or is unavailable, we fall through to the read - // loop, which is itself capped at `MAX_SKILL_FILE_SIZE`. - if let Ok(Some(metadata)) = fs.metadata(&skill_file_path).await + // app. If metadata is unavailable, refuse to read. + let metadata = fs + .metadata(&skill_file_path) + .await + .map_err(|e| SkillLoadError { + path: skill_file_path.clone(), + message: format!("Failed to read SKILL.md metadata: {}", e), + })?; + if let Some(metadata) = metadata && metadata.len > MAX_SKILL_FILE_SIZE as u64 { return Err(SkillLoadError { @@ -627,54 +592,15 @@ pub async fn load_skill_frontmatter( }); } - let mut reader = fs - .open_sync(&skill_file_path) + let content = fs + .load(&skill_file_path) .await .map_err(|e| SkillLoadError { path: skill_file_path.clone(), - message: format!("Failed to open file: {}", e), + message: format!("Failed to read file: {}", e), })?; - // The chunked read is intentionally synchronous: `Fs::open_sync` - // returns a synchronous `Read` (RealFs uses `std::fs::File`), and - // production callers already wrap `load_skills_from_directory` in - // `cx.background_spawn`, so any blocking happens on the background - // executor — not on the foreground thread. Routing through - // `smol::unblock` instead would schedule the work on smol's blocking - // pool, whose wakeups don't drive GPUI's test scheduler and therefore - // panic with "Parking forbidden" under `TestAppContext`. - let read_result: Result, io::Error> = (|| { - let mut accumulated: Vec = Vec::new(); - let mut chunk = [0u8; SKILL_READ_CHUNK_SIZE]; - loop { - let n = reader.read(&mut chunk)?; - if n == 0 { - break; - } - accumulated.extend_from_slice(&chunk[..n]); - if let Some(end) = closing_delimiter_end(&accumulated) { - // Discard body bytes swept up in the last chunk so that e.g. multi-byte - // graphemes split at the boundary won't cause `str::from_utf8` to fail. - accumulated.truncate(end); - break; - } - if accumulated.len() > MAX_SKILL_FILE_SIZE { - break; - } - } - Ok(accumulated) - })(); - let accumulated = read_result.map_err(|e| SkillLoadError { - path: skill_file_path.clone(), - message: format!("Failed to read file: {}", e), - })?; - - let content = std::str::from_utf8(&accumulated).map_err(|e| SkillLoadError { - path: skill_file_path.clone(), - message: format!("SKILL.md is not valid UTF-8: {}", e), - })?; - - parse_skill_frontmatter(&skill_file_path, content, source).map_err(|e| SkillLoadError { + parse_skill_frontmatter(&skill_file_path, &content, source).map_err(|e| SkillLoadError { path: skill_file_path.clone(), message: e.to_string(), }) @@ -1836,46 +1762,6 @@ description: A skill with no body content assert_eq!(skill.directory_path, PathBuf::from("/skills/my-skill")); } - #[gpui::test] - async fn test_load_skill_frontmatter_with_emoji_at_chunk_boundary(cx: &mut TestAppContext) { - // We must be able to load skill frontmatter even when a - // multipoint grapheme crosses the chunk read boundary. - let fs = FakeFs::new(cx.executor()); - let frontmatter = "---\nname: my-skill\ndescription: Example skill testing multipoint graphemes at chunk boundary\n---\n"; - - // Pad contents so that the emoji's first byte lands - // at the last byte of the first read chunk. - let padding = "a".repeat(SKILL_READ_CHUNK_SIZE - frontmatter.len() - 1); - let content = format!("{frontmatter}{padding}✅"); - - assert!( - (frontmatter.len() + padding.len()) < SKILL_READ_CHUNK_SIZE, - "emoji must start before the second chunk" - ); - assert!( - content.len() > SKILL_READ_CHUNK_SIZE, - "skill is longer than a chunk, so we know that the emoji crosses chunk boundaries" - ); - - fs.insert_tree( - "/skills", - serde_json::json!({ - "my-skill": { - "SKILL.md": content, - } - }), - ) - .await; - - load_skill_frontmatter( - fs as Arc, - PathBuf::from("/skills/my-skill/SKILL.md"), - SkillSource::Global, - ) - .await - .expect("frontmatter should parse even when a multipoint grapheme such as an emoji crosses the byte chunk boundary"); - } - #[gpui::test] async fn test_read_skill_body_returns_trimmed_body(cx: &mut TestAppContext) { let fs = FakeFs::new(cx.executor());