I think #7457 might help here?

@tlongwell-block so #7457 enables the Slackbot to see that the skill's supplemental/reference material exists, but it's still unreachable/unusable

the skill is structured like:

oncall-buddy-help
├── references
│   ├── diagnostics.md
│   ├── hints-template.md
│   └── technical-ops.md
└── SKILL.md

and the agent can load(source: oncall-buddy-help), which loads only the content from SKILL.md. to be able to load the skill's reference content for deeper debugging, it needs to be able to do:

load(source: references/technical-ops)

and the model does try this, but it fails with:

Error: Source 'references/technical-ops' not found. Use load() to see available sources.

because currently load() only loads the root level SKILL.md content and for any supplemental content, examples, scripts, etc. the model would need a shell filesystem access to read the file contents directly which the Slackbot can't do.

if we're going to abstract the SKILL.md file away by calling load(source: <skill_name>) then I'm thinking it makes sense to do the same for skill reference files/supplemental content instead of having a dual path of load() vs raw file read

I think this is ok @wpfleger96 - downside is that this is less "progressive loading" which is one of the tenets of skills. So downside is if there are skills which are mountains and mountains of .md files, it will blow out context.

tagging @tlongwell-block who would be aware of this limitation and may have thoughts. I think it is ok but I don't know how bloated skills are (the ones I have seen in squareup/agents I think may end up a bit bloated, but I guess you only load one or two).

@michaelneale @tlongwell-block 🙃 sorry I totally botched this PR, my intention was to preserve progressive disclosure here not break it!

I just pushed a new change to address this - now instead of inlining supporting files on load, each supporting file is now individually addressable via load():

load(source: "oncall-buddy-help/references/technical-ops.md")

Loading the base skill still just loads SKILL.md and lists available supporting files with their load() names so the LLM knows how to reach them when needed:

## Supporting Files

The following supporting files are available:
- references/technical-ops.md → load(source: "oncall-buddy-help/references/technical-ops.md")
- references/diagnostics.md → load(source: "oncall-buddy-help/references/diagnostics.md")

Use load(source: "<skill-name>/<path>") to load individual files into context...

My intention here was to make skill supplemental files reachable for geese without filesystem access, but not break skill progressive disclosure! What do y'all think

/goose

Summary: This PR enables individual loading of skill supporting files via load(source: "skill-name/path") for environments without filesystem access. The implementation is well-designed with proper security controls (canonicalization, known-file-only matching, slash rejection in skill names) and excellent test coverage including symlink attack prevention.

🟡 Warnings

1. Canonicalization fallback may weaken security check (summon.rs:720-723)

   let canonical_skill_dir = skill
       .path
       .canonicalize()
       .unwrap_or_else(|_| skill.path.clone());

   If canonicalize() fails (e.g., skill directory has weird permissions but files are still readable), the fallback to the non-canonical path could theoretically weaken the starts_with security check if file_path.canonicalize() succeeds and returns a different prefix. Consider returning an error instead of falling back:

   let canonical_skill_dir = skill
       .path
       .canonicalize()
       .map_err(|e| format!("Cannot verify skill directory '{}': {}", skill.name, e))?;

   This is a low-probability scenario but a stricter approach would be more defensive.

🟢 Suggestions

1. Minor: Check for supporting file path early in delegate (summon.rs:1341-1347)
   The check if source_name.contains('/') happens after resolve_source has already done the work of resolving the supporting file. Moving this check before resolve_source would avoid unnecessary work, though the performance impact is negligible:

   // Could check early to avoid unnecessary resolution
   if source_name.contains('/') {
       return Err(format!(
           "Cannot delegate to supporting file '{}'. Use load() to read it instead.",
           source_name
       ));
   }

✅ Highlights

1. Excellent security design: The path traversal defense is well-layered:

   • Only iterates through pre-discovered supporting_files (not arbitrary paths)
   • Exact match required (rel_normalized == relative_path)
   • Canonicalization check catches symlink attacks
   • Skill names with / are rejected during parsing (prevents bad/skill from enabling namespace confusion)

2. Comprehensive test coverage: Six new tests covering:

   • Load names displayed correctly
   • Supporting file loading by path
   • Error messages listing available files
   • Symlink attack blocking (unix-specific)
   • Path traversal input rejection
   • Skill name with / rejection

3. Good error messages: When a namespaced path isn't found, the error lists available files (up to 10), making the UX much better for debugging.

4. Progressive disclosure preserved: The PR description correctly identifies that auto-inlining all files defeats the skill design. Listing files with load() names maintains progressive disclosure while solving the no-filesystem-access use case.

Review generated by goose