refactor(sources): unify root-fallback decision and short-circuit github root fetch#2198
Merged
Merged
Conversation
…hub root fetch Follow-ups from the PR #2118 review. Finding 1 (DRY): the "use the repo root as the requested skill" decision was implemented twice with asymmetric gates — an explicit SKILL.md check in the git path (`groupRemoteFilesBySkillRoot`) and a delegated check in the github path (`discoverGithubSkillDirs`). Extract a shared `shouldUseRootFallback({ skillFilter, isWildcard, hasRootSkillFile, hasRequestedSkillDir })` used by both so they cannot drift. Finding 2 (perf + behavior lock): the github fallback trigger was widened in #2118 to fire whenever the requested dir is absent, which performed a full root-file fetch even when the root had no SKILL.md to install. Detect the root SKILL.md from the directory listing already in hand and gate on it via the shared helper, so the full root fetch is short-circuited when nothing would be installed. Add tests locking the intended behavior: the root SKILL.md is installed under the requested name when it coexists with real skill dirs and the requested skill is absent, and no root files are fetched when there is no root SKILL.md. Finding 3 (.gitignore drive-by): `.tokensave/` is a third-party byproduct that no tool emits, so the hand-written .gitignore section is the correct owner (per the gitignore guidelines); left as-is intentionally. Closes #2119 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@dyoshikawa Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the three non-blocking (low) follow-ups from the PR #2118 review.
Finding 1 — DRY: shared root-fallback decision
The "install the repo root as the single requested skill" decision was implemented twice with asymmetric gates:
groupRemoteFilesBySkillRoot) — explicithasRootSkillFile && !grouped.has(name)check.discoverGithubSkillDirs) — only gated on!hasRequestedSkillDir, delegating the SKILL.md check downstream.Extracted a shared
shouldUseRootFallback({ skillFilter, isWildcard, hasRootSkillFile, hasRequestedSkillDir })used by both paths so they cannot drift.Finding 2 — perf + behavior lock
#2118 widened the github fallback trigger to fire whenever the requested dir is absent, so
fetchRootLevelFallbackSkillperformed a full root-file fetch even when the root had noSKILL.mdand nothing would be installed. Now the rootSKILL.mdis detected from the directory listing already in hand and gated via the shared helper, short-circuiting the fetch when there is nothing to install.Added two tests to lock the intended behavior:
SKILL.mdis installed under the requested name when it coexists with real skill dirs and the requested skill is absent (locks the widened-trigger edge case flagged in the review).getFileContentnot called) when the requested skill is absent and there is no rootSKILL.md.Finding 3 —
.gitignore.tokensave/.tokensave/is a third-party byproduct that no tool emits, so per the gitignore guidelines (HAND_MAINTAINED_GITIGNORE_ENTRIESis for tool-external paths) the hand-written section is the correct owner. Left as-is intentionally; no code change.Verification
sources.test.ts: 41 pass.pnpm cicheckgreen (6920 unit tests).Closes #2119
🤖 Generated with Claude Code