agent_skills: Read SKILL.md in one shot via fs.load (#57510) (cherry-pick to preview) (#57831)

Cherry-pick of #57510 to preview

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

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;
@@ -557,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,
@@ -611,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 {
@@ -626,51 +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; 4096];
-        loop {
-            let n = reader.read(&mut chunk)?;
-            if n == 0 {
-                break;
-            }
-            accumulated.extend_from_slice(&chunk[..n]);
-            if closing_delimiter_end(&accumulated).is_some() {
-                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(),
     })