fix(skills): guard context-injection commands so absent tools or refs can't abort a skill#103
Draft
taminomara wants to merge 1 commit into
Draft
Conversation
taminomara
force-pushed
the
fix/which-availability-guards
branch
from
98fb02d to
9b856fc
Compare
… can't abort a skill Context-injection commands (the bang-backtick reads in SKILL.md) run at skill load, and a non-zero exit can abort the skill before its gate logic runs. Several skills used commands that exit non-zero when their subject is absent: - `which <tool>` exits non-zero (and prints a verbose not-found line to stderr on some builds) when the tool is not installed — the reported failure when an executable under test is missing. Now `which <tool> 2>/dev/null || echo "not installed"` across every skill that probed git/gh/jq, plus the two "git installed" checks in tdd and refactor that used the `git --version` anti-pattern the guidance itself warns against (exit 127 when git is absent). - `git symbolic-ref --short refs/remotes/origin/HEAD` exits 128 when `origin/HEAD` is unset; `git log/diff origin/HEAD...HEAD` likewise; `git branch --show-current` / `git status --porcelain` exit 128 outside a repo; `gh pr diff --name-only` fails with no PR; `git config user.name/email` exit 1 when the identity is unset. Each is now guarded with `2>/dev/null || echo <sentinel>`, and each consumer that gated on an empty value now also reads the sentinel. - Standardized han-release's prerequisite sentinel from MISSING to "not installed". Also corrected the plugin-builder guidance, which wrongly claimed `which` has "no error exit code" on not-found and listed `|| echo` as a pattern to avoid; it now teaches the guarded `which ... 2>/dev/null || echo "not installed"` form and no longer treats `|| echo` as an anti-pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
taminomara
force-pushed
the
fix/which-availability-guards
branch
from
9b856fc to
613811b
Compare
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
1 participant
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.
Problem
Context-injection commands (the bang-backtick reads in
SKILL.md) run at skill load, and a non-zero exit can abort the skill before its gate logic runs. Multiple skills used commands that exit non-zero when their subject is absent — most visibly thewhich <tool>availability checks, which exit1when the tool isn't installed (the failure seen when an executable under test is missing).What changed
Tool-availability checks. Every
which <tool>context-injection read now useswhich <tool> || echo "not installed"(exits 0, injects a clear sentinel).tddandrefactorused thegit --versionanti-pattern for their "git installed" check (exit 127 when git is absent); both now use the guardedwhichform.Git ref / repo / PR state. Guarded the commands that exit non-zero when a precondition is absent, each with
2>/dev/null || echo <sentinel>, and updated every consumer that gated on an empty value to also read the sentinel:git symbolic-ref --short refs/remotes/origin/HEADandgit log/diff origin/HEAD...HEAD— exit 128 whenorigin/HEADis unsetgit branch --show-current/git status --porcelain— exit 128 outside a repogh pr diff --name-only— fails with no PRgit config user.name/email— exit 1 when the identity is unsethan-release. Standardized its prerequisite sentinel from
MISSINGtonot installed.Guidance. The plugin-builder guidance wrongly claimed
whichhas "no error exit code" on not-found and listed|| echoas a pattern to avoid. Corrected to teach the guarded form and carve out the exception for any command that exits non-zero when its subject is absent.Evidence
which <missing>→ exit 1;which <missing> || echo "not installed"→ exit 0.git symbolic-ref --short refs/remotes/origin/HEADwith noorigin/HEAD→ exit 128;git log origin/HEAD..HEAD→ exit 128.Known residuals (intentionally not changed)
Three
han-releasereads of the han repo's own guaranteed-present files (grep … CHANGELOG.md,jq … marketplace.json×2) are left as-is: a word-sentinel would corrupt their parsed output, and the clean fix is a file-existence prerequisite gate — out of scope for this PR.🤖 Generated with Claude Code