Skip to content

Clarify SearchDepth scope in fsskills documentation#497

Merged
qmuntal merged 3 commits into
microsoft:mainfrom
PratikDhanave:fix-fsskills-searchdepth-directory-discovery
Jul 17, 2026
Merged

Clarify SearchDepth scope in fsskills documentation#497
qmuntal merged 3 commits into
microsoft:mainfrom
PratikDhanave:fix-fsskills-searchdepth-directory-discovery

Conversation

@PratikDhanave

@PratikDhanave PratikDhanave commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Following review, this PR is repurposed from a behavior change to a documentation clarification.

SearchDepth controls only resource and script discovery within an already-discovered skill directory. Skill-directory discovery (locating SKILL.md directories) is bounded independently and intentionally, matching the .NET SDK. That scope was not explicit in the option's documentation, which is what led to the original misread.

Change

  • Document the intended scope on SourceOptions.SearchDepth and on searchForSkills.
  • Add TestFileSource_SearchDepth_DoesNotAffectSkillDirectoryDiscovery, which asserts that a large SearchDepth does not widen skill-directory discovery — locking in the intended contract.

No behavior change; the functional code is identical to main.

Verification

  • go test -race -shuffle=on ./agent/skills/... — passing
  • go vet, gofmt, gofumpt — clean

@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 14, 2026 16:41
Copilot AI review requested due to automatic review settings July 14, 2026 16:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an inconsistency in agent/skills/fsskills where the configured SourceOptions.SearchDepth was honored for within-skill file scanning but not for skill-directory discovery, which was previously hardcoded to defaultSearchDepth (2). The change threads the configured depth into the directory-discovery walk so both recursive traversals are depth-bounded consistently.

Changes:

  • Pass s.searchDepth into discoverSkillDirectories / searchForSkills so skill-directory discovery honors SearchDepth.
  • Update SourceOptions.SearchDepth documentation to reflect that it now governs both discovery of skill directories and scanning files within a skill.
  • Add a regression test proving deeper skill directories are discovered only when SearchDepth is increased.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
agent/skills/fsskills/source.go Threads configured search depth into skill-directory discovery and updates the SearchDepth doc comment accordingly.
agent/skills/fsskills/source_test.go Adds a test asserting that skill-directory discovery respects SearchDepth (default vs. increased depth).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@qmuntal

qmuntal commented Jul 15, 2026

Copy link
Copy Markdown
Member

Thanks for identifying and documenting this behavior.

After comparing it with the .NET SDK, the current Go behavior is intentional. Skill-directory discovery is bounded independently at two levels, while SearchDepth controls resource and script discovery within an already discovered skill directory.

Making SearchDepth control skill-directory discovery would broaden its existing semantics and cause the Go SDK to diverge from .NET. For cross-SDK consistency, we won’t merge this change.

Could you repurpose this PR to update the Go documentation so the intended scope of SearchDepth is explicit?

SearchDepth controls only resource and script discovery within an
already-discovered skill directory; skill-directory discovery (locating
SKILL.md directories) is bounded independently and intentionally, matching
the .NET SDK. This was not obvious from the option's documentation.

Document the intended scope on SourceOptions.SearchDepth and on
searchForSkills, and add a test asserting that SearchDepth does not widen
skill-directory discovery. No behavior change.
@PratikDhanave
PratikDhanave force-pushed the fix-fsskills-searchdepth-directory-discovery branch from b565867 to d742800 Compare July 15, 2026 15:49
@PratikDhanave PratikDhanave changed the title Make fsskills skill-directory discovery honor SearchDepth Clarify SearchDepth scope in fsskills documentation Jul 15, 2026
@PratikDhanave

Copy link
Copy Markdown
Contributor Author

Thanks for the clarification and the .NET comparison — that makes sense. I've repurposed this PR to a documentation-only change: it now documents that SearchDepth scopes to within-skill resource/script discovery only, and that skill-directory discovery is bounded independently. I also added a test that locks in the intended contract (a large SearchDepth does not widen directory discovery). No behavior change.

@qmuntal
qmuntal enabled auto-merge July 16, 2026 08:12
@qmuntal
qmuntal added this pull request to the merge queue Jul 17, 2026
Merged via the queue into microsoft:main with commit 9e26ff1 Jul 17, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants