agent_skills: Read SKILL.md in one shot via fs.load (#57510)

Follow-up to #57466 simplifying `load_skill_frontmatter` rather than
fixing a separate bug.

#57466 fixed the chunked-read loop so it truncates accumulated bytes at
the frontmatter boundary, preventing `str::from_utf8` from failing on a
multi-byte grapheme split across chunks. That fix is correct, but the
chunked read it's working around isn't pulling its weight:
`MAX_SKILL_FILE_SIZE` is 100KB and there's already a metadata pre-check,
so "stop reading early once we've seen the closing `---`" saves at most
~25 pages per file while forcing the use of `open_sync`, a hand-rolled
loop, and a long comment about `smol::unblock` vs the GPUI test
scheduler.

This PR:

- replaces the chunked `open_sync` + read loop with a single
`fs.load(...)` call (the same primitive `read_skill_body` already uses);
- deletes `closing_delimiter_end`, `SKILL_READ_CHUNK_SIZE`, the
`std::io::{self, Read}` import, and the `Parking forbidden` paragraph;
- tightens the metadata pre-check so a metadata error (permissions, I/O,
etc.) bails out instead of silently falling through to a blind read;
- removes the now-obsolete
`test_load_skill_frontmatter_with_emoji_at_chunk_boundary` test — by
construction, a single full read can't split a UTF-8 sequence at any
boundary.

Closes AI-303

Release Notes:

- N/A

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<dyn Fs>, directory: &Path) -> Vec<PathBuf> {
         .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<usize> {
-    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<dyn Fs>,
     skill_file_path: PathBuf,
@@ -612,10 +572,15 @@ pub async fn load_skill_frontmatter(
 ) -> Result<Skill, SkillLoadError> {
     // 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<Vec<u8>, io::Error> = (|| {
-        let mut accumulated: Vec<u8> = 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<dyn Fs>,
-            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());