fix(skill-quality): split DESC_CHAR_CAP into spec field limit and listing cap checks (0.19.0) - #3141
kyle-sexton wants to merge 5 commits into
Conversation
…ting cap checks (0.19.0) ## Summary `check-skill.sh` set `DESC_CHAR_CAP=1536` and checked only that value. 1536 is Claude Code's in-context listing-truncation cap for `description` + `when_to_use` combined. The Agent Skills spec separately caps the `description` FIELD at 1024 characters. Two caps at two layers, only the looser one enforced, so a skill could pass the gate while breaching the documented field maximum. 19 of 224 fleet skills breach it today and all of them pass. ## Fix Check 2 now reports two distinctly-named, distinctly-messaged criteria: - 2a (WARN, new): `description` alone vs the new `DESC_FIELD_CHAR_CAP=1024`. The message names the Agent Skills spec field maximum and its packaging / API-upload enforcement, and says local loading does not enforce it. - 2b (FAIL, unchanged): `description` + `" - "` + `when_to_use` vs `DESC_CHAR_CAP=1536`, the per-skill listing-entry truncation cap. Each constant carries a comment naming its layer, and the passing-path note reports both budgets. WARN rather than FAIL is recorded in the script beside the criterion: Claude Code documents only the 1536 listing truncation and performs no local 1024 validation (a 1526-char description was observed loading into a live session listing in full), so the breach is latent on the filesystem and plugin surface and live only on the API and claude.ai upload paths, while the 19 in-fleet offenders cannot be trimmed mechanically because check 3 protects the quoted trigger phrases their descriptions carry. A follow-up may flip it to FAIL once they are trimmed. Trimming offenders is out of scope here. No new check number: the split lives inside check 2, so the advertised twenty-five-check count and every doc surface stating it stay accurate. ## Verification - `bash plugins/skill-quality/scripts/check-skill.test.sh`: 132 assertions, all pass, including two new contract tests. - New fixture `field-cap-only` (1200-char description, no `when_to_use`): WARNs `description field is 1200 chars (Agent Skills spec field maximum 1024 ...)`, emits no listing-cap finding, exits 0. - New fixture `listing-cap-only` (1000-char description + 597-char `when_to_use` = 1600 combined): FAILs `description+when_to_use is 1600 chars (cap 1536 ...)`, emits no field-maximum finding, exits 1. - `shellcheck` clean on both scripts. Refs #3119. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CEpCvCdEb8NGFhuuDwm8k8
|
Claude finished @kyle-sexton's task in 1m 5s —— View job Security review complete ✅
No security issues found. Scope reviewed: This PR splits an existing description-length check in
Nothing further to flag. |
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06c65ed013
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pendent
## Summary
Check 2's two description budgets are stated in characters but were measured
with bash `${#var}`, which counts through the current locale. On a host pinned
to a byte locale (`LC_ALL=C`, `POSIX`, or a UTF-8 locale that is not installed)
that degrades to counting UTF-8 BYTES, so both the 1024 spec field maximum (2a)
and the 1536 listing cap (2b) silently tightened for any description carrying
non-ASCII text. Check 22 already solved this for `metadata.summary`, inline.
## Fix
- New `codepoint_len` helper beside the other reporting helpers: UTF-8 to
UTF-32BE via iconv makes every codepoint exactly 4 bytes, so byte-count/4 is
the codepoint count on any host, with a `LC_ALL=C.UTF-8` `${#}` fallback for
hosts without iconv.
- Check 2 measures `DESC_LEN` and `WTU_LEN` through it, feeding both 2a and 2b.
`COMBINED_LEN` arithmetic and every message are unchanged.
- Check 22's inline measurement now calls the same helper, so the technique
lives in one place. Behavior and messages identical.
- Two contract tests under `LC_ALL=C`: a 1000-codepoint / 1200-byte description
stays silent, an 1100-codepoint one still warns at its codepoint length.
- CHANGELOG note amended into the unreleased 0.19.0 entry; no version bump.
## Verification
- Pre-fix, `LC_ALL=C`, description of 960 codepoints / 1160 bytes plus a
36-char trigger suffix (996 codepoints, 1196 bytes): WARN "description field
is 1196 chars". Post-fix, same case: no WARN, INFO reports
"description field 996/1024 chars".
- Both new assertions fail against the pre-fix script (1200 and 1300 chars
reported), so neither is vacuous.
- Full suite green: `bash plugins/skill-quality/scripts/check-skill.test.sh`
exits 0, "all assertions passed".
- `shellcheck` and `shfmt -d` clean on both scripts; markdownlint clean on the
CHANGELOG.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEpCvCdEb8NGFhuuDwm8k8
|
Claude finished @kyle-sexton's task in 54s —— View job Security review complete ✅
No security issues found. Scope: this run only needed to cover commits since the last reviewed head (
Nothing further to flag. |
|
Claude finished @kyle-sexton's task in 4m 4s —— View job Code review complete
SummaryReviewed Codex's locale finding is already resolved. The One doc-drift gap, not carried by any changed line in this diff:
claude-code-plugins/plugins/skill-quality/README.md Lines 23 to 24 in 23417aa By contrast, Everything else checked out:
Not independently verified: the PR body's claimed "132 assertions, all pass" test run and shellcheck-clean result. This lane's |
|
Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ck list ## Summary The Checks list described check 2 as only the 1536-char listing-entry cap, understating what the script reports. Check 2 has two criteria at two layers: 2a WARNs when the `description` field alone exceeds the 1024-char Agent Skills spec field maximum (enforced on the packaging/API-upload path, not by local loading), and 2b FAILs when `description` + `when_to_use` exceeds the 1536-char per-skill listing-entry cap. Split the bullet in two so each criterion carries its threshold and severity, keeping the note distinguishing the per-skill cap from the shared listing budget. ## Verification - `markdownlint-cli2 plugins/skill-quality/README.md`: 0 issues - `typos plugins/skill-quality/README.md`: clean Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CEpCvCdEb8NGFhuuDwm8k8
|
Claude finished @kyle-sexton's task in 2m 18s —— View job Code review complete
SummaryReviewed the full diff against the pre-PR base ( This is now the third review pass on this PR, and the two issues previous passes surfaced are both resolved on the current head:
Verified correct on this pass:
One non-blocking nit posted inline: Not independently re-verified: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…d cap ## Summary Check 2a had only a well-over-cap fixture (1200 chars), so the `>` versus `>=` comparison at the 1024 spec field maximum was untested. Adds the boundary pair that check 2b already has: a description of exactly 1024 chars emits no field-maximum WARN (only the informational count line), and one of 1025 chars does WARN. ## Verification Full suite passes: 136 assertions, exit 0, including the two new ones. Non-vacuity confirmed by mutating a scratch copy of the script to `>=`, under which the 1024 assertion fails while the 1025 assertion still passes. shellcheck clean on the test file. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CEpCvCdEb8NGFhuuDwm8k8
|
Claude finished @kyle-sexton's task in 58s —— View job Security review complete ✅
No security issues found. Per the skill's skip gate, this run only needed to cover commits since the last reviewed head (
Neither commit touches Nothing further to flag. |
|
Claude finished @kyle-sexton's task in 1m 37s —— View job Code review complete
SummaryThis is the fourth review pass on this PR. Since the last pass ( Verified on this pass:
No findings to report. No inline comments posted — there is nothing on the changed lines that rises to this lane's high-signal bar. Not independently verified: |
…egration-it93kq # Conflicts: # plugins/skill-quality/.claude-plugin/plugin.json # plugins/skill-quality/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 51s —— View job Security review complete ✅
Skip gate hit — no new review performed. Current head Head No security issues found (per the standing review). |
|
Claude finished @kyle-sexton's task in 1m 49s —— View job Code review — skipped (skip gate)
Skip gate hit: "This head already has a successful review from this lane that still applies." Current head No new PR content to review since the fourth pass, which found no outstanding issues (Codex's locale-counting bug and the README doc-drift gap were both resolved earlier in that pass's chain, and the check-2a boundary-pair nit was closed by |
|
Claiming this PR to freshen the branch against |
|
Coordination note for whoever freshens this branch (re the lane-claim above): the conflict against This PR's remaining value over main is only the delta: (1) main's 1536 listing cap still byte-counts under Generated by Claude Code |
|
Escalation (not acted on further): freshening this branch against |
|
No description provided. |
|
Closing as superseded — #3150 landed the same fix for #3119 at 04:55Z ( Both PRs reached the same conclusion (a separate 1024 field-cap check, WARN not FAIL) from independent evidence. Two things decided it for #3150:
Worth preserving from this PR: the observation that a 1526-char description was seen loading into a live session listing in full. That is a cleaner statement of the "latent, not live" argument than the one that shipped, and belongs in the check comment if anyone revisits the FAIL-vs-WARN call. No action needed — #3119 is closed. Generated by Claude Code |
2 participants
Closes #3119
Summary
check-skill.shsetDESC_CHAR_CAP=1536and checked only that value. 1536 is Claude Code's per-skill listing-entry truncation cap fordescription+when_to_usecombined; the Agent Skills spec separately caps thedescriptionfield itself at 1024 characters. Two caps at two layers, only the looser one enforced, so a skill could pass the gate while breaching the documented field maximum (19 of 224 fleet skills do today).Fix
Check 2 now reports two distinctly-named, distinctly-messaged criteria:
descriptionalone vs the newDESC_FIELD_CHAR_CAP=1024, the Agent Skills spec field maximum. WARN rather than FAIL is recorded in the script beside the criterion: Claude Code documents only the 1536 listing truncation (skillListingMaxDescChars) and performs no local 1024 validation (a 1526-char description was observed loading into a live session listing in full), so the breach is latent on the filesystem/plugin surface and live only on the API and claude.ai upload paths, while the 19 in-fleet offenders cannot be trimmed mechanically (check 3 protects their quoted trigger phrases). A follow-up may flip it to FAIL once they are trimmed.description+" - "+when_to_usevsDESC_CHAR_CAP=1536, the listing-entry cap; message byte-identical to before.No new check number: the split lives inside check 2, so the advertised twenty-five-check count and the doc surfaces stating it stay accurate.
skill-quality0.18.0 -> 0.19.0 with a matching CHANGELOG entry.Verification
bash plugins/skill-quality/scripts/check-skill.test.sh: 132 assertions, all pass, including two new contract tests (a 1200-char description warns on the spec field maximum only (check 2a),a 1600-char combined entry fails the listing cap only (check 2b)) — re-run main-side, and independently reproduced by a fresh-context verifier with self-built fixtures.when_to_use: WARNs on the field maximum, no listing finding, exit 0.shellcheckclean on both scripts.Related
Refs #3118 (parent spec container; this slice corrects the gate constant its brief flagged as a captured assumption)