Skip to content

fix(skill-quality): accept when_to_use triggers in check 12 - #2412

Merged
kyle-sexton merged 1 commit into
mainfrom
cursor/check-skill-when-to-use-63e1
Aug 12, 2026
Merged

fix(skill-quality): accept when_to_use triggers in check 12#2412
kyle-sexton merged 1 commit into
mainfrom
cursor/check-skill-when-to-use-63e1

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #1451.

Check 12 now accepts a populated when_to_use field with single-quoted trigger phrases, so skills do not need a redundant Use when: prefix in description.

Verification

  • check-skill.test.sh: all assertions passed

Related

Fixes #1451.

A populated when_to_use with single-quoted trigger phrases satisfies the
trigger-spec requirement without a redundant Use when: prefix in description.

Tests: check-skill.test.sh.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@cursor

cursor Bot commented Aug 12, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 31s —— View job


Security Review

  • Gather context (PR diff, changed files)
  • Review changed files for security issues
  • Report findings

No security issues found.

This PR only touches plugins/skill-quality: a version bump, a changelog entry, a test case, and a one-line logic change in check-skill.sh's check 12 warning condition (plugins/skill-quality/scripts/check-skill.sh#L570-L578). The change adds a skill_frontmatter::extract_triggers call over $CUR_WTU and uses safe, quoted printf '%s\n' ... | ... piping — no eval, no unquoted variable expansion into a shell command, and no new external input paths. This is a static-analysis tool that reads local skill frontmatter text; the change doesn't alter how untrusted content flows into command execution, file paths, or credentials.

No CI/workflow files, permissions, or dependency pins were touched in this PR.

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 6m 58s —— View job


Code review

  • Gather context (CLAUDE.md, REVIEW.md, diff)
  • Review changed files for correctness
  • Check test coverage
  • Post findings

Root CLAUDE.md / .claude-pr/CLAUDE.md are both empty — no repo-specific guidance beyond REVIEW.md. This repo has a claude-security-review workflow, so per REVIEW.md this lane owns everything except security and leaves that scope to the security lane.

What the change does: Check 12 (plugins/skill-quality/scripts/check-skill.sh:572-579) now skips the "description has no Use when: trigger phrasing" warning when when_to_use alone already contains single-quoted trigger phrases, avoiding a redundant Use when: prefix requirement in description. I traced the logic by hand (short-circuit added is && [[ -z "$CUR_TRIG_WTU" ]]): whenever when_to_use has quoted triggers, the pre-existing elif (not-single-quoted warning) is also guaranteed not to fire, since it re-derives triggers from the same $CUR_DESC$CUR_WTU text — so the two warnings stay mutually exclusive as before. I don't see a correctness issue in the production script change itself; it does what the PR description says.

One finding, posted inline on check-skill.test.sh:349: the new test's when_to_use fixture has a bash-quoting bug. I actually ran the exact make_skill argument text through bash to see what it resolves to, and it's:

when_to_use: "'alpha trigger'", "'beta trigger'"

— two separate double-quoted segments, not the repo's real single-continuous-string convention (see mcp-tools/skills/audit/SKILL.md's description field for the actual pattern). I then read skill_frontmatter::field's fm_strip_comment (skill-frontmatter.sh:45-67) and confirmed it stops at the first closing ", silently dropping everything after — including 'beta trigger'. The test still passes, but only because a single trigger phrase is already sufficient; it doesn't actually verify multi-trigger when_to_use parsing the way its comment implies. Detail and a suggested fix are in the inline comment.

Everything else — version bump (0.15.40.15.5), CHANGELOG.md entry, and the rest of the test file — looks consistent and unremarkable.

Note: I could not independently execute check-skill.test.sh in this sandbox (script execution here required interactive approval that wasn't available), so the PR description's "all assertions passed" claim is author-reported and unverified by me — my finding above is instead based on manually resolving the fixture's bash quoting via direct execution and reading the parser source.

Comment thread plugins/skill-quality/scripts/check-skill.test.sh
@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@kyle-sexton
kyle-sexton merged commit df4fa24 into main Aug 12, 2026
35 of 36 checks passed
@kyle-sexton
kyle-sexton deleted the cursor/check-skill-when-to-use-63e1 branch August 12, 2026 09:59
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.

skill-quality: amend check 12 to accept a populated when_to_use, sequenced behind #1096

2 participants