Skip to content

feat(skill-quality): add the shared listing-budget report, fix check 2's joiner - #1450

Merged
kyle-sexton merged 5 commits into
mainfrom
fix/1404-listing-budget-check
Jul 26, 2026
Merged

feat(skill-quality): add the shared listing-budget report, fix check 2's joiner#1450
kyle-sexton merged 5 commits into
mainfrom
fix/1404-listing-budget-check

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during work-loop execution.

Summary

  • Adds check-listing-budget.sh + a listing-budget action — the shared/aggregate skill-listing
    character budget (skillListingBudgetFraction, default 1% of the model's context window) had no
    check at all. check-skill.sh check 2 only ever guarded the per-skill entry cap
    (skillListingMaxDescChars, 1536 chars) — a different, narrower limit. The new script pools one or
    more skills roots into a single aggregate estimate against a documented, overridable default (8000
    chars — the harness's own SLASH_COMMAND_TOOL_CHAR_BUDGET fallback), reports the biggest
    contributors on overflow, and is always advisory (exit 0): the live budget depends on a model's
    context window and a consumer's own settings, neither of which a static check can observe. Wired
    into the skill-quality-gate CI job as a report-only step pooling every plugin's skills/ root into
    one marketplace-wide aggregate (currently ~109,000 chars against the 8,000-char default floor).
  • Fixes check 2's joiner (skill-quality: check-skill.sh has no shared listing-budget check, plus three smaller gate defects #1404 item 2). The harness assembles a listing entry as description +
    " - " + when_to_use — a literal 3-char joiner — that check 2's sum omitted, under-counting by 3
    whenever when_to_use is populated. Not currently binding at present description lengths in this
    repo, but wrong in exactly the direction the listing-budget work is about.
  • Settles the issue's stated 4x discrepancy (skill-quality: check-skill.sh has no shared listing-budget check, plus three smaller gate defects #1404, "one discrepancy worth resolving"). Fetched
    current docs (code.claude.com/docs/en/settings, live HTML) confirm
    SLASH_COMMAND_TOOL_CHAR_BUDGET's documented 8000-char fallback is exactly contextTokens(200000) x ~4 chars/token x skillListingBudgetFraction(0.01) — the contextTokens x 4 x fraction derivation
    from the shipped binary, not the bare tokens x fraction reading. skillListingBudgetFraction
    (default 0.01) and skillListingMaxDescChars (default 1536, matching this repo's existing
    DESC_CHAR_CAP) were also confirmed directly from the live settings page.
  • Item 4's caveat (never hardcode skillListingBudgetFraction's default as a resolved live value)
    is folded into the new script's design: every tunable (budget chars, context tokens, fraction,
    chars/token, per-entry cap) is environment-overridable, and the default budget is anchored on the
    one documented number (8000) rather than reconstructed from assumed inputs.

Deferred: item 3 (check 12 amendment) — split into #1451

Item 3 — amending check 12 to accept a populated, single-quoted when_to_use as satisfying the
trigger-spec requirement instead of requiring a literal Use when: prefix — is not in this PR.
The issue itself sequences it behind #1096 ("already editing that file and claims check 21"), and I
verified #1096 is a real, currently open, active PR (branch feat/fresh-eyes-delegation-doctrine-gate,
708 additions) that does edit check-skill.sh (appending check 21). Per that sequencing note, item 3
is split out into a dedicated follow-up, #1451, scoped and ready to pick up once #1096 merges or is
otherwise resolved. This PR closes #1404 (items 1, 2, and 4 are complete; item 3's tracking now lives
in #1451, not in a reopened #1404).

Test plan

  • New check-listing-budget.test.sh — 8 assertions (help, missing-root error, empty root,
    under-budget OK, forced-tiny-budget WARN, joiner counted, per-entry cap applied before summing,
    multi-root pooling). All pass.
  • check-skill.test.sh — added assertion 29 (boundary case: desc(1500)+wtu(34)=1534 passes
    check 2 without the joiner fix, fails at 1537/1536 with it). Full suite: 29/29 pass.
  • scripts/check-changed-skills.test.sh (CI orchestrator self-test): 8/8 pass.
  • scripts/check-skill-portability.test.sh: 17/17 pass; check-skill-portability.sh origin/main
    reports no unexcused coupling tokens in the changed skill file.
  • Dogfooded check-skill.sh against the edited skill-quality:check skill itself: PASS, 0
    errors, 0 warnings, all 7 base-ref trigger phrases preserved (description rewrite kept every
    existing single-quoted trigger verbatim and only added new ones).
  • check-changed-skills.sh origin/main (the actual CI gate step): PASS.
  • shellcheck --rcfile=.shellcheckrc clean on both new scripts (fixed one SC2016 literal-backtick
    note and one SC2034 unused-variable warning during development).
  • actionlint .github/workflows/ci.yml: clean.
  • markdownlint-cli2 clean on the three edited Markdown files.
  • Manually ran check-listing-budget.sh plugins/*/skills against this repo: 183 skills, 61 roots,
    aggregate 108,975 chars vs the 8,000-char default budget (~13.6x over) — reproduces the
    overflow the issue's own measurement found, with concrete top contributors.

Related

Closes #1404 (items 1, 2, and 4 from that issue; item 3 is split into #1451, not resolved here).
Refs #1451 (new follow-up: item 3, check 12 amendment — sequenced behind #1096, ready to pick up once
it merges or is otherwise resolved).
Refs #1096 (open PR already editing check-skill.sh; #1451 sequences behind it).
Refs #1271 (the original listing-budget issue #1404 was extracted from).

…2's joiner (#1404)

check-skill.sh check 2 only ever guarded the per-skill listing-entry cap
(skillListingMaxDescChars, 1536 chars); the shared budget every loaded skill
draws from together (skillListingBudgetFraction, default 1% of the model's
context window) had no check at all. New check-listing-budget.sh pools one or
more skills roots into a single aggregate estimate against a documented,
overridable default (the harness's own SLASH_COMMAND_TOOL_CHAR_BUDGET
fallback of 8000 chars), reports the biggest contributors on overflow, and
never hardcodes a resolved live value (context window and
skillListingBudgetFraction are both consumer settings this static check
cannot observe) — always advisory, exit 0. Wired into the skill-quality-gate
CI job as a report-only step pooling every plugin's skills/ root into one
marketplace-wide aggregate.

Also fixes check 2 itself: the harness assembles a listing entry as
description + " - " + when_to_use (a literal 3-char joiner) that check 2's
sum omitted, under-counting by 3 whenever when_to_use is populated.

Settles the discrepancy the issue raised between two derivations of the
budget formula: fetched current docs confirm SLASH_COMMAND_TOOL_CHAR_BUDGET's
8000-char fallback is exactly contextTokens(200000) x ~4 chars/token x
skillListingBudgetFraction(0.01) — the contextTokens x 4 x fraction
derivation, not a bare tokens x fraction reading.

Item 3 (amending check 12 to accept a populated when_to_use as satisfying the
trigger-spec requirement) is deferred per the issue's own sequencing note:
PR #1096 is an open, active PR already editing check-skill.sh (claims check
21). #1404 stays open to track it.

Refs #1404

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb2f8c26ba

ℹ️ 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".

Comment thread plugins/skill-quality/scripts/check-listing-budget.sh
Comment thread plugins/skill-quality/scripts/check-listing-budget.sh Outdated
…catalog sync

Two things the local test/lint pass didn't catch before push, both flagged by
this repo's own CI gates on the first run:

- The two new scripts (check-listing-budget.sh, check-listing-budget.test.sh)
  were committed without the executable bit — the hygiene lane's shebang-vs-mode
  check caught it.
- The root README.md's generated catalog block was stale against the bumped
  skill-quality plugin.json description — regenerated via
  scripts/generate-catalog.mjs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 49s —— View job


PR Review

  • Gather context and understand the request
  • Read changed files (check-listing-budget.sh, check-listing-budget.test.sh, check-skill.sh, CI workflow, SKILL.md)
  • Review check-listing-budget.sh (new script)
  • Review check-listing-budget.test.sh (new tests)
  • Review check-skill.sh changes (joiner fix)
  • Review CI workflow changes
  • Review remaining changed files
  • Synthesize and post findings

Overall assessment

The joiner fix is correct and well-tested. The new check-listing-budget.sh design — advisory-only (exit 0), all tunables env-overridable, default anchored to the one documented number — is sound. Two concrete issues below, one also raised by Codex.


Findings

1. Report header reports arg count, not found-root count

check-listing-budget.sh:189

printf 'Shared listing-budget estimate over %d skill(s) across %d root(s):\n' "$ENTRY_COUNT" "${#ROOTS[@]}"

${#ROOTS[@]} is the count of arguments given, not the number of roots that were actually found and scanned. If an explicit root doesn't exist, it is silently skipped at line 150 ([[ -d "$root" ]] || continue) but still counted here. With plugins/*/skills in CI this doesn't matter — shell glob expansion only produces existing paths — but an explicit invocation with one misspelled root produces a misleading "N roots" header while silently omitting that root's skills from the aggregate.

The same skip-vs-count gap means the Codex P2 concern (error on a missing explicit root) and this header inaccuracy have the same root: found_any_root tracks "at least one directory existed," not "every given root was scanned." Tracking a found-root counter and using it in the printf would fix the header independently of the error-vs-skip policy choice.

Fix this →


2. disable-model-invocation: true skills included in aggregate (Codex P1)

check-listing-budget.sh:150–173

The loop includes every */SKILL.md without checking whether disable-model-invocation: true is set in the frontmatter. Skills with that flag set are not auto-invoked by the harness and, per the Codex comment, may not contribute to the shared listing budget at all. The marketplace reportedly has 51 such skills; if the Codex is correct about their budget exclusion, the 108,975-char CI aggregate is materially overstated — possibly by several thousand characters — changing the headline "13.6× over the default floor" figure.

skill_frontmatter::field is already imported via skill-frontmatter.sh, so the fix is a one-liner after extracting the frontmatter:

dmi="$(skill_frontmatter::strip_quotes "$(skill_frontmatter::field disable-model-invocation <<<"$fm")")"
[[ "$dmi" == "true" ]] && continue

Worth confirming before committing whether the harness excludes disable-model-invocation: true skills from the listing budget entirely (they're not auto-invoked, but the question is whether their names/descriptions are still serialized into the listing). If they are excluded, adding the filter is a correctness fix and the CI output would change. If they are included in the listing despite not being auto-invoked, the Codex concern doesn't apply and the current script is correct.

Fix this →


What looks good

  • Joiner fix (check 2): Clean arithmetic — ((WTU_LEN > 0)) && JOINER_LEN=3 followed by COMBINED_LEN=$((DESC_LEN + JOINER_LEN + WTU_LEN)). The identical pattern in check-listing-budget.sh (lines 162–164) keeps the two scripts consistent. Test 29 in check-skill.test.sh pins the exact boundary: desc(1500) + " - " + wtu(34) = 1537 > 1536.

  • Advisory-only design: The rationale for always exiting 0 is sound and well-documented in the header. The live budget depends on context window and consumer settings, neither of which a static check can observe. The SLASH_COMMAND_TOOL_CHAR_BUDGET 8000-char fallback as the default anchor (rather than reconstructing from assumed inputs) is the right call per item 4's caveat.

  • All tunables are env-overridable: CHECK_SKILL_LISTING_BUDGET_CHARS, CHECK_SKILL_LISTING_CONTEXT_TOKENS, CHECK_SKILL_LISTING_BUDGET_FRACTION, CHECK_SKILL_LISTING_CHARS_PER_TOKEN, CHECK_SKILL_LISTING_MAX_DESC_CHARS — consumers can override any of them without forking the script.

  • Per-entry cap applied before summing (line 169): Correctly mirrors the harness's own truncation so an already-oversized single entry (a check-2 FAIL) doesn't inflate the aggregate beyond what Claude Code would actually load.

  • Test harness: 8 test cases with self-contained throwaway git repo, covering help, missing root, empty root, under-budget, forced WARN, joiner, per-entry cap, and multi-root pooling. All deterministic.

  • CI wiring: Both new steps (test + report) are correctly scoped inside skill-quality-gate. The advisory step is positioned after the schema-validation step and will never block a PR.

  • SKILL.md for the check skill: All seven base-ref trigger phrases preserved (confirmed by the PR's own dogfood run), and the new listing-budget action is fully documented including the "different cross-skill limit" distinction from check 2's per-skill cap.


Minor

The check-listing-budget.sh header comment at line 86:

sed -n '2,79p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'

reads lines 2–79 but the header runs to line 80 (set -uo pipefail is line 81). Line 80 (# ...) would be omitted from --help output. Not critical, but 2,80p would capture it.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 50s —— View job


Security Review

No security issues found.

Reviewed all 10 changed files with focus on injection surfaces, GitHub Actions patterns, secrets handling, and path/command injection.


GitHub Actions workflow (.github/workflows/ci.yml)

The two new CI steps contain no ${{ github.* }} context interpolations in their run: blocks — they execute literal script paths with a shell glob. Trigger is pull_request (not pull_request_target), so forked PRs run in the correct isolated context without repository secrets. The skill-quality-gate job inherits the top-level permissions: contents: read with no widening. No supply-chain risk: no new actions introduced, existing actions remain pinned to SHAs.

Relevant lines: ci.yml:682–690


check-listing-budget.sh — injection and input handling

SKILL.md content never reaches command execution. The frontmatter description and when_to_use fields extracted from potentially PR-authored files flow only into ${#desc} / ${#wtu} (string-length arithmetic) and are never written to the temp contrib-file or executed as commands. Only the computed integer entry_len, the path-derived skill_name, and the caller-supplied root path are written to the contrib file.

Operator-supplied env var overrides (CHECK_SKILL_LISTING_BUDGET_CHARS, CHECK_SKILL_LISTING_MAX_DESC_CHARS, etc.) land in bash arithmetic (( )) and in awk -v assignments. awk -v is not vulnerable to injection. In bash (( )), a value of the form $(cmd) would be evaluated, but no workflow step sets these from any ${{ github.* }} or user-controlled source — they are developer-facing local overrides only, which requires machine access that already implies full trust. Not exploitable in the CI context.

printf format safety. All user-derived strings ($BUDGET_SOURCE, $skill_name, $root) are passed as %s arguments, never as the format string itself. No printf format string injection.

Temp file. mktemp creates the file atomically with mode 0600; trap 'rm -f "$CONTRIB_FILE"' EXIT ensures cleanup. Standard practice, no race exploitable here.

Source path. source "$SCRIPT_DIR/skill-frontmatter.sh" where SCRIPT_DIR is derived from ${BASH_SOURCE[0]} — a fixed reference to the script's own install location. Not attacker-influenced in the threat model of a CI runner.


check-skill.sh — joiner fix

The change is arithmetic-only: adds JOINER_LEN=3 when WTU_LEN > 0 and includes it in COMBINED_LEN=$((DESC_LEN + JOINER_LEN + WTU_LEN)). No new input surfaces or execution paths introduced.


Summary: The PR is clean. All new inputs from untrusted sources (SKILL.md frontmatter content) are handled as pure data (length arithmetic, never executed). Workflow changes follow correct pull_request / pinned-SHA / no-context-interpolation patterns.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 041e5f6f6e

ℹ️ 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".

Comment thread plugins/skill-quality/scripts/check-listing-budget.sh Outdated
Comment thread plugins/skill-quality/scripts/check-listing-budget.sh Outdated
Automated review on #1450 caught it: the usage() sed range (2,79p) clipped
the header's final line ("in the issue this script closes.") since the
comment block actually runs through line 80. 2,80p now captures the whole
header.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

This was generated by AI during work-loop execution.

Addressed the automated review's minor finding (--help sed range clipping the header's last line) in b2d3f7f2,79p -> 2,80p. Verified --help output now includes the final header line and re-ran the full test suite + shellcheck (clean).

Security review found no issues; no other findings to address.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 57s —— View job


PR Re-Review (post b2d3f7f)

  • Gather context and understand the request
  • Read updated check-listing-budget.sh (latest commit)
  • Verify sed range fix is correct
  • Assess status of open findings from prior review
  • Synthesize and post findings

Sed fix: confirmed correct

check-listing-budget.sh:86

2,79p2,80p is correct. The comment block runs through line 80 (# in the issue this script closes.), and set -uo pipefail lands at line 81. The fix captures the full header. ✅


Open findings (unchanged from prior review)

The following were not addressed in b2d3f7f and remain open:

1. Header reports arg count, not found-root count (my prior Finding 1)

check-listing-budget.sh:189

printf 'Shared listing-budget estimate over %d skill(s) across %d root(s):\n' "$ENTRY_COUNT" "${#ROOTS[@]}"

${#ROOTS[@]} is the count of arguments passed, not the count of directories actually scanned. A misspelled root is silently skipped at line 150 but still reported in the header. Under the CI glob invocation this doesn't bite (expansion only produces existing paths), but an explicit invocation with one bad path produces a misleading count. A FOUND_ROOTS=0 counter incremented at line 151 (inside the [[ -d "$root" ]] branch) and used here would fix it.

Fix this →

2. Budget source label misleading when CHECK_SKILL_LISTING_BUDGET_CHARS is set (Codex P2, second review)

check-listing-budget.sh:118–119

BUDGET_CHARS="${CHECK_SKILL_LISTING_BUDGET_CHARS:-8000}"
BUDGET_SOURCE="documented default (SLASH_COMMAND_TOOL_CHAR_BUDGET fallback)"

When CHECK_SKILL_LISTING_BUDGET_CHARS=4000 is set, the output reads:

budget: 4000 chars (documented default (SLASH_COMMAND_TOOL_CHAR_BUDGET fallback))

That's factually wrong — 4000 is not the documented default; it's a user override. The fix is to distinguish the two cases:

if [[ -n "${CHECK_SKILL_LISTING_BUDGET_CHARS:-}" ]]; then
  BUDGET_CHARS="$CHECK_SKILL_LISTING_BUDGET_CHARS"
  BUDGET_SOURCE="override (CHECK_SKILL_LISTING_BUDGET_CHARS)"
else
  BUDGET_CHARS=8000
  BUDGET_SOURCE="documented default (SLASH_COMMAND_TOOL_CHAR_BUDGET fallback)"
fi

Fix this →

3. disable-model-invocation: true skills included (Codex P1 — design question)

check-listing-budget.sh:152–173

Flagged by Codex as P1 — 51 marketplace skills have disable-model-invocation: true. If the harness excludes these from the listing budget, the 108,975-char CI aggregate overstates the real cost and the "13.6× over" headline is misleading.

The prior review noted "worth confirming before committing" — the question being whether those skills still contribute their descriptions to the serialized listing despite not being auto-invoked. If they're excluded from the listing entirely, a one-liner filter after extracting frontmatter ([[ "$dmi" == "true" ]] && continue) would correct the aggregate and the CI output would change. skill_frontmatter::field is already imported.

This is a design-question blocker rather than a clear-cut bug — if you've confirmed the harness behavior and they are included in the listing, the script is correct as-is. Worth documenting that confirmation explicitly (e.g., in the header comment near line 8) since Codex flagged it and a future maintainer will wonder.


What remains good

Everything from the prior review's "What looks good" section still holds: joiner fix, advisory-only design, env-overridable tunables, per-entry cap before summing, test harness, CI wiring, and SKILL.md updates are all sound. The sed fix lands cleanly.


Summary: b2d3f7f correctly resolves the minor sed-range finding. Three items remain before merge: the found-root counter in the header (small, clear fix), the BUDGET_SOURCE label when CHECK_SKILL_LISTING_BUDGET_CHARS is overridden (small, clear fix), and the disable-model-invocation question (design confirmation or filter).

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 28s —— View job


Security Review

  • Gather context
  • Fetch PR diff and read changed files
  • Analyze GitHub Actions workflow changes
  • Analyze check-listing-budget.sh for injection surfaces
  • Analyze check-skill.sh joiner fix
  • Analyze skill-frontmatter.sh library

No security issues found.

The commit reviewed is b2d3f7f. The prior Claude security review on 041e5f6 reached the same conclusion, and the only delta since that commit is the one-character sed range fix (2,79p2,80p) in usage(), which has zero security surface.


GitHub Actions workflow (ci.yml:682–690)

The two new run: steps contain no ${{ github.* }} context interpolations — they execute literal script paths with a shell glob expanded by the runner before the run: block processes it. The workflow trigger is pull_request (not pull_request_target), so forked PRs run in an isolated context without repository secrets. permissions: contents: read at the top level is unchanged and no step widens it. No new actions introduced; existing actions remain SHA-pinned.

check-listing-budget.sh — injection surfaces

SKILL.md content never reaches command execution. Frontmatter fields (description, when_to_use) extracted from potentially PR-authored files are consumed only as ${#desc} / ${#wtu} string-length integers. These integers participate in bash arithmetic $(( )) with no path to command substitution (no operator-set env var feeds into the arithmetic from any ${{ github.* }} source). The content of frontmatter fields is never written to a command line, a $(...), or a temp file that is later sourced or evald.

awk -v assignments (-v t="$CONTEXT_TOKENS" -v c="$CHARS_PER_TOKEN" -v f="$FRACTION") are not injectable — the -v mechanism treats the value as a string/number, not as awk code. These variables are operator-level env overrides, not PR-author-controlled.

printf format safety. Every user-derived string ($BUDGET_SOURCE, $skill_name, $root, $name) is passed as a %s positional argument, never as the format string itself. No format-string injection.

Temp file. mktemp creates with mode 0600; trap 'rm -f "$CONTRIB_FILE"' EXIT is a fixed path captured at definition time (single-quoted, expanded at fire time, but $CONTRIB_FILE is immutable after mktemp). Standard safe pattern.

source "$SCRIPT_DIR/skill-frontmatter.sh"SCRIPT_DIR is derived from ${BASH_SOURCE[0]}, a fixed reference to the script's own install location, not attacker-influenced in CI.

Skill-directory name as a path component (skill_name="${skill_md%/SKILL.md}"; skill_name="${skill_name##*/}") — the directory name flows into printf '%s' and into the TSV written to the temp file; it is later sorted and printf '%s'-printed. Never executed as a command.

check-skill.sh joiner fix

The change is arithmetic-only: JOINER_LEN=0 initialization, ((WTU_LEN > 0)) && JOINER_LEN=3, and COMBINED_LEN=$((DESC_LEN + JOINER_LEN + WTU_LEN)). WTU_LEN is always ${#CUR_WTU} (an integer); no new input surface or execution path is introduced.

skill-frontmatter.sh library

Existing, unchanged between the merge base and this PR's head for any security-relevant line. The awk key-matching patterns ($0 ~ "^" k ":[[:space:]]*") receive only hardcoded string literals from call sites in check-listing-budget.sh (description, when_to_use), not user input.


@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2d3f7f82b

ℹ️ 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".

Comment thread plugins/skill-quality/scripts/check-listing-budget.sh Outdated
… inputs

Addresses the five open Codex review findings on the new listing-budget
reporter, all confirmed against branch content and current official docs.

Exclude `disable-model-invocation: true` skills from the aggregate. The
invocation-control table at https://code.claude.com/docs/en/skills records
"Description not in context" for that frontmatter, and "Hide individual
skills" states it "removes the skill from Claude's context entirely" — such
a skill spends none of the shared description budget, so counting it
overstated the report. On this marketplace: 183 skills / 109,205 chars ->
132 / 83,594, still ~10.4x over the 8000-char default, so the finding the
check exists to surface is unchanged.

Reject a missing explicit skills root as an environment error (exit 2)
instead of silently skipping it, which omitted a whole plugin subtree from a
falsely low "OK" aggregate. The no-args resolution path keeps its own
"no skills root found" message. Report the count of roots actually scanned
rather than the count of arguments given.

Validate every numeric override as a positive number, integer or decimal,
routing failures into the documented exit 2. A nonnumeric value previously
either coerced to zero in awk — fabricating a zero-character budget and a
bogus overflow WARN while still exiting 0 — or crashed with an undocumented
exit 1.

Give a fixed CHECK_SKILL_LISTING_BUDGET_CHARS precedence over the
token/fraction reconstruction, as the script's own header always claimed,
announcing the ignored reconstruction input rather than discarding it
silently; and label it an override rather than the "documented default".

The budget verdict stays advisory throughout: OK and WARN both exit 0. Only
operator/environment errors use exit 2, which was already documented.

Derive --help's range from the header block itself so editing that block can
no longer clip or overrun the help text. Tests grow 8 -> 21 assertions,
covering every path above; the three input-handling defects had shipped
green precisely because none was asserted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d218d15db7

ℹ️ 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".

Comment thread plugins/skill-quality/scripts/check-listing-budget.sh Outdated
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

This was generated by AI during work-loop execution.

All open review findings are addressed in d218d15 — the three the re-review listed as remaining, plus the two further Codex P2s. Per-finding evidence is in each inline thread; summary here since the last top-level comment still reads "three items remain before merge".

# Finding Source Classification
1 disable-model-invocation: true skills included in the aggregate Codex P1 / re-review 3 VALID — fixed
2 Missing explicit skills root silently skipped; header counts args, not scanned roots Codex P2 / re-review 1 VALID — fixed
3 Nonnumeric overrides coerced to zero (or exit 1) instead of the documented exit 2 Codex P2 VALID — fixed
4 CHECK_SKILL_LISTING_BUDGET_CHARS mislabelled "documented default" Codex P2 / re-review 2 VALID — fixed
5 Fixed budget silently ignored when CHECK_SKILL_LISTING_CONTEXT_TOKENS is also set Codex P2 VALID — fixed

On the P1 design question, which the prior review flagged as "worth confirming before committing" — confirmed against current official documentation rather than recall, per this repo's fresh-docs mandate. https://code.claude.com/docs/en/skills: the invocation-control table records "Description not in context" for disable-model-invocation: true, and Hide individual skills states it "removes the skill from Claude's context entirely." Those skills spend none of the shared description budget, so the filter is a correctness fix. The CI report moves from 183 skills / 109,205 chars to 132 / 83,594 — still ~10.4× over the 8000-char default, so the finding this check exists to surface is unchanged. The confirmation is recorded in the script header, SKILL.md, README.md, and the changelog so a future maintainer does not have to re-litigate it.

Semantics deliberately unchanged. The budget verdict stays advisory: OK and WARN both exit 0. The new hard failures use exit 2, which was already the script's documented usage/env-error class — and one of them removes an undocumented exit 1 that a nonnumeric override could previously trigger. Nothing in this commit touches check-skill.sh's check-2 PASS/FAIL surface.

Verification on d218d15: check-listing-budget.test.sh 21/21 (was 8 — findings 2, 3 and 4 had all shipped green precisely because no assertion covered them); check-skill.test.sh 7/7; shellcheck -x clean on both scripts; markdownlint-cli2 clean on the three changed docs; the skill-quality gate self-check PASS with all 9 base-ref trigger phrases preserved and the description at 972/1536; and the literal CI invocation check-listing-budget.sh plugins/*/skills exits 0.

…iltering

`skill_frontmatter::field` returns the raw YAML scalar, so the exact-string
`== "true"` filter missed spellings a YAML reader treats as the same boolean.
Reproduced before fixing: a nine-skill fixture root covering the variants
reported 7 listing-eligible skills instead of 2 — an inline `# comment`, a
quoted value carrying a comment, `TRUE`, `True`, and a trailing-whitespace
value all leaked through and were counted, re-inflating the aggregate with
descriptions the harness keeps out of context.

Normalize before comparing: strip a whitespace-preceded YAML comment, trim
surrounding whitespace, strip one quote layer, and fold ASCII case.

Scope deliberately bounded, and stated in the code:

- Comment-stripping applies only to this boolean, never to `description` or
  `when_to_use`, where a whitespace-preceded `#` is content rather than a
  comment. Folding it there would silently truncate a description.
- YAML 1.1's `yes` / `on` aliases are NOT folded. The documented spelling is
  `true`, and over-matching risks dropping a skill over a value the harness
  may read as a plain string.
- This is a pragmatic normalizer for one known field, not a YAML parser.

Audited the sibling call sites for the same class of gap: this was the only
boolean compared against raw field output in the plugin. Every other
`skill_frontmatter::field` consumer extracts free text, and
`skill_frontmatter::metadata_field` already strips trailing comments
internally, so the shared library needed no change — keeping the blast
radius off `check-skill.sh`'s PASS/FAIL surface.

Corrects the changelog's marketplace figure to 132 skills / 83,611 chars.
The previously recorded 83,594 was measured before this PR's own SKILL.md
description edit added 17 characters, and the figure now carries the commit
it was measured at, per this repo's own measurement convention.

Tests 21 -> 22; the new case fails against the pre-fix code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@kyle-sexton
kyle-sexton merged commit 727116a into main Jul 26, 2026
27 checks passed
@kyle-sexton
kyle-sexton deleted the fix/1404-listing-budget-check branch July 26, 2026 04:39

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4121042ef

ℹ️ 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".

Comment on lines +165 to +167
local pattern='^[0-9]+$'
[[ "$kind" == "num" ]] && pattern='^([0-9]+(\.[0-9]+)?|\.[0-9]+)$'
if [[ ! "$val" =~ $pattern ]] || ! awk -v v="$val" 'BEGIN { exit (v > 0) ? 0 : 1 }'; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Normalize accepted integer overrides to decimal

When an integer override is zero-padded, such as CHECK_SKILL_LISTING_BUDGET_CHARS=08, this validation accepts it but later Bash arithmetic and printf %d interpret it as octal. With a nonempty skills root, the reporter emits invalid-octal diagnostics, renders the budget as 0, incorrectly reports OK, and exits 0; CHECK_SKILL_LISTING_MAX_DESC_CHARS=08 similarly bypasses the requested cap. Convert accepted integer values explicitly to base 10 or reject leading zeroes before using them.

Useful? React with 👍 / 👎.

Comment on lines +261 to +262
desc="$(skill_frontmatter::strip_quotes "$(skill_frontmatter::field description <<<"$fm")")"
wtu="$(skill_frontmatter::strip_quotes "$(skill_frontmatter::field when_to_use <<<"$fm")")"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Strip YAML comments before measuring listing scalars

When valid frontmatter uses an inline YAML comment outside the scalar, for example description: "12345" # maintainer note, skill_frontmatter::field returns the comment and strip_quotes can no longer remove the surrounding quotes. The reporter therefore counts the source comment and quoting rather than the five-character value Claude loads; a fixture with commented description and when_to_use values was reported as 50 characters instead of 13, which can produce false overflow warnings and contributor sizes. Parse or quote-aware-strip YAML comments before measuring these fields.

Useful? React with 👍 / 👎.

kyle-sexton added a commit that referenced this pull request Jul 26, 2026
Main shipped skill-quality 0.11.0 (#1450, the shared listing-budget reporter)
while this branch also claimed 0.11.0. Both sides are composed across six
conflicts; nothing is blanket-picked.

- .claude-plugin/plugin.json: recomputed to 0.12.0 above main's release. The
  description takes main's shape — the listing-budget reporter, and check 2
  renamed to the per-skill listing-entry cap — plus this branch's twenty-one
  count and fresh-eyes item. A shared version would have silenced
  changelog-parity entirely: its bump gate short-circuits when head equals base.
- CHANGELOG.md: this branch's section becomes 0.12.0 and main's 0.11.0 section is
  restored verbatim beneath it.
- skills/check/SKILL.md: main's frontmatter, which carries the listing-budget
  action and its two new trigger phrases, with the check count and fresh-eyes
  item re-applied. Preserving those phrases is what keeps check 3 green.
- README.md and plugins/skill-quality/README.md: main's rewrites naming the
  listing-budget reporter, with this branch's counts restored; the more detailed
  of the two injection-portability bullets is kept over main's terser duplicate.
- scripts/check-skill.test.sh: purely additive on both sides — this branch's
  check-21 fixtures and main's joiner-boundary test. Git placed the shared
  trailing `fi` outside the conflict region, so it is restored explicitly.

Main's check-skill.sh changes (check 2's 3-char joiner, header comments) merged
without conflict and do not touch check 21.

Verified post-merge: 81 assertions pass — 80 before the merge plus main's one
added test, which is the arithmetic confirming the test-file splice lost nothing.
Main's check-listing-budget.test.sh passes. check 2 now measures 1014/1536 for
the longer composed description under its new joiner arithmetic, and all 7
base-ref trigger phrases are preserved. Comparing this branch's scanner before
and after the ambiguity suppression over the SAME merged tree gives 166 records
either way, byte-identical, so suppression still changes nothing on real content.
kyle-sexton added a commit that referenced this pull request Jul 26, 2026
#1494)

## Summary

`plugins/skill-quality/.claude-plugin/plugin.json` carried **two**
`version` members on `main`:

```text
4:  "version": "0.11.0",
5:  "version": "0.12.0",
```

The advertised release was therefore parser-dependent — last-wins
readers (`jq`, Python `json.load`) resolve `0.12.0`, first-wins readers
retain `0.11.0`, and duplicate-rejecting consumers can reject the
manifest outright. Any of those can misidentify or block a plugin
update.

**Why `0.12.0` is correct.** `main` carried `0.11.0` before #1096,
released in #1450, and this plugin's CHANGELOG has `## [0.12.0]` above
main's `## [0.11.0]`. The `0.11.0` member is the stale one, and it is
what this PR deletes.

**Scope.** One file, one deletion. Nothing else in the manifest and
nothing else in the repo.

**How it happened.** Introduced while resolving a `main` merge in #1096.
Both sides of that conflict carried `"version": "0.11.0"` — main had
released 0.11.0 in #1450 and the branch also claimed 0.11.0 — so git
treated the version line as **common context outside** the conflict
region and placed only the differing `description` inside it. A scripted
replacement of the conflict region then emitted a fresh `version` +
`description` pair above the surviving common-context one.

## Test plan

A duplicate-key-aware parse is the only check that observes this, and it
is clean after the change:

```console
$ python -c "... json.load(..., object_pairs_hook=<duplicate detector>) ..."
duplicate keys: NONE
version: 0.12.0
```

Also confirmed:

- `git diff --stat` is exactly `1 file changed, 1 deletion(-)`.
- The CHANGELOG's top section is still `## [0.12.0]`, so the manifest
and the release notes agree.
- `changelog-parity-gate` and `plugin-gate` pass in CI on this branch.

Worth flagging for reviewers: `json.load`, `jq`, and JSON Schema
validation all resolve duplicate keys last-wins, so every one of them
reports `0.12.0` whether or not the duplicate is present. That is why
this defect passed a fully green 27-context suite on #1096, and it is
why the fix is verified with a duplicate-key hook rather than a plain
parse.

## Related

- #1096 — the PR whose merge-conflict resolution introduced the
duplicate member.
- #1450 — released skill-quality `0.11.0` on `main`, the collision that
produced the conflict shape.
- #1493 — follow-up covering the gate gap: no CI check currently detects
a duplicate key in a plugin manifest.

Closes #1492
kyle-sexton added a commit that referenced this pull request Jul 26, 2026
…1506)

Closes #1498

## Summary

*This was generated by AI during work-loop execution.*

No gate in CI could detect a duplicate key in a plugin/marketplace
manifest. JSON Schema validates the already-parsed document
(structurally blind to a repeated member), and every consumer downstream
resolves it last-wins with no signal — the exact shape that shipped
`plugins/skill-quality/.claude-plugin/plugin.json` with two `"version"`
members through a fully green 27-context suite (#1492).

## Fix

Added `scripts/check-manifest-duplicate-keys.py`: reads each
`plugins/*/.claude-plugin/plugin.json` and
`.claude-plugin/marketplace.json` via `json.loads(text,
object_pairs_hook=...)`, intercepting the raw key/value pairs of every
JSON object literal (at any nesting depth) *before* the standard
de-duplicating collapse into a dict. A duplicate is recorded (not
raised) so the parse still completes last-wins — identical to how
`json.load`/`JSON.parse`/`jq` resolve it — and every distinct duplicate
key in a file is reported in one pass.

Wired into the `hygiene` job in `.github/workflows/ci.yml`, directly
adjacent to the existing "Validate plugin manifests" step (per the
issue's own routing rationale — same file scope, same tier as "does it
parse"/"does it match the schema"), gated the same way the four
`check-jsonschema` steps are (skipped only on a provably docs-only
diff), and its outcome is added as one line to
`scripts/aggregate-hygiene-results.sh`'s existing `CHECK_RESULTS` block
so a duplicate key fails the job exactly like any other hygiene check.

## Verification

- `bash scripts/check-manifest-duplicate-keys.test.sh` — 17 unit/CLI
tests, all passing (nested-object duplicates, sibling-object
non-duplicates, multiple distinct duplicate keys in one file, malformed
JSON *not* double-reported, default-discovery glob covering both
`plugin.json` and `marketplace.json`, missing-file handling, exit
codes).
- `test_catches_the_1492_shaped_duplicate_version_key` reproduces the
actual #1492 defect shape (two `"version"` members) as an inline fixture
and asserts the gate fails on it — the fixture proving the gate catches
a real duplicate key, per the issue's own ask.
- Live empirical cross-check against the two claims in the issue's
verification table, run locally against a copy of the real (now-fixed)
`plugins/skill-quality/.claude-plugin/plugin.json` with the duplicate
`"version"` member re-injected:
- `check-jsonschema --schemafile
https://json.schemastore.org/claude-code-plugin-manifest.json <file>` →
`ok -- validation done`, exit 0 (confirms schema validation is blind to
it).
- `python3 scripts/check-manifest-duplicate-keys.py <file>` → `DUPLICATE
KEY: ... defines 'version' more than once ...`, exit 1 (confirms the new
gate catches exactly what schema validation cannot).
- `python3 scripts/check-manifest-duplicate-keys.py` (default discovery,
no args) against the actual repo tree → `No duplicate JSON object keys
found in 62 manifest file(s).`, exit 0 — no false positives on the real
manifest set.
- `actionlint .github/workflows/ci.yml` — clean.
- `shellcheck --rcfile=.shellcheckrc
scripts/check-manifest-duplicate-keys.test.sh` — clean.
- YAML parse + step-order check of the modified `hygiene` job — new
steps land in the intended position, ids wired correctly into
`CHECK_RESULTS`.
- `typos --config _typos.toml` over the new/changed files — clean.
- No `plugins/*` manifest touched, so the per-plugin CHANGELOG-parity
gate does not apply to this change.

## Related

- #1492 — the shipped duplicate-key instance this gate would have
caught.
- #1494 — the manifest fix for #1492.
- #1096, #1450 — the version-collision conflict shape that produces this
defect class.

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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: check-skill.sh has no shared listing-budget check, plus three smaller gate defects

1 participant