fix(skill-quality): read integer overrides as decimal and stop measuring YAML comments

[kyle-sexton]

Three stranded post-merge review findings on the skill-quality plugin, plus the
repository-level doc contradiction filed alongside them. Reviews posted after their PRs
merged were invisible to the merge gate, so these sat unread.

Zero-padded integer overrides were parsed as octal

require_positive_number's ^[0-9]+$ accepts a zero-padded value, but bash arithmetic and
printf %d then read it in base 8. Reproduced three ways against origin/main in a
detached worktree:

• CHECK_SKILL_LISTING_BUDGET_CHARS=08 — printf: 08: invalid octal number, the budget
  rendered as 0, and the report still announced OK and exited 0.
• CHECK_SKILL_LISTING_BUDGET_CHARS=0123 — budget silently became 83.
• CHECK_SKILL_LISTING_MAX_DESC_CHARS=010 — entries capped at 8 instead of the requested 10.

Accepted integer overrides are now forced to base 10 at the one place the digits become a
number. All three require_positive_number ... int call sites are covered. The ratio and
fraction overrides are deliberately left alone: 0.01 is the documented default fraction
and must keep its leading zero, and both reach only awk, which has no octal input.
CHECK_SKILL_LISTING_CHARS_PER_TOKEN is likewise untouched because it never reaches $(( ))
or %d. No other script in this plugin takes a numeric environment override, so there is no
sibling occurrence of the pattern left behind.

A trailing YAML comment was measured as part of the listing scalars

skill_frontmatter::field returned the comment along with the value, which also hid the
surrounding quotes from strip_quotes so the quoting was counted too. A fixture with
commented description and when_to_use scalars measured 52 characters against a true 15,
producing false overflow warnings and wrong contributor sizes.

The fix went into the shared helper rather than the reporter, because check-skill.sh
reads the same fields through it — the per-skill entry cap (Check 2) and the
trigger-preservation diff had the same defect. Stripping is quote-aware and confined to the
plain/flow branch: inside a block scalar a # is content, such as a markdown heading in a
description: | body.

normalize_bool's own sed comment strip is now redundant and was removed, along with the
comment asserting that comment-stripping is "never applied to description /
when_to_use". It has exactly one caller and that caller sources its value through
skill_frontmatter::field, so nothing loses the strip.

Blast radius was measured rather than predicted: every SKILL.md under plugins/ was run
through both the old and the new extractor and the measured description / when_to_use
lengths are identical across the repo — no real skill's measurement moves.

The fresh-eyes rule contradicted the dispatch ladder

docs/PLUGIN-PHILOSOPHY.md stated the normative rule as requiring "a named subagent" while
its own dispatch ladder makes a generic fresh-context subagent the default and its
named-agent bar says a named agent is earned, not default. An author following the rule
would create a named agent unnecessarily.

Resolved toward the ladder rather than hedged, because that is what the repo actually
enforces: the delegation-wording detector in check-skill.sh accepts a generic worker —
it matches fresh-context plus any of agent/subagent/worker/advisor/reviewer/verifier/
dispatch/delegate — and never requires a named agent. docs/topics/fresh-eyes-checkpoint-audit/PLAN.md
specified this same reconciliation ("generic or named"); it was applied to the preceding
paragraph and missed on the rule sentence itself. The author-facing spec
skills/check/reference/fresh-eyes-declarations.md is doctrine-agnostic and needed no change.

Folding this nine-word documentation clause into the plugin PR instead of buying a second
full CI cycle is a deliberate, operator-approved deviation from one-PR-per-plugin.

Also verified, no change needed

The fourth finding in this batch reported two version members in
plugins/skill-quality/.claude-plugin/plugin.json. Already fixed on main by dcfa7c8b70
(#1494); the manifest carries exactly one. Recorded here so the thread resolves against
evidence rather than silence.

Testing

Seven regression cases added to check-listing-budget.test.sh covering each octal
manifestation, the decimal fraction that must keep working, the quoted and plain comment
forms, a # that is content rather than a comment, and a # inside a block scalar.

Gates run green from the worktree root: the listing-budget and check-skill suites,
run-plugin-tests.sh, check-changelog-parity.sh --check / --check-bump / --check-order,
check-changed-skills.sh, check-shell-portability.sh (+ its suite),
check-contract-slice-prune.sh --check / --check-diff, check-contract-clause-coverage.py,
ShellCheck over the three changed scripts, markdownlint over the two changed docs, and the
CI invocation check-listing-budget.sh plugins/*/skills.

Related

Refs #1938 — the stranded-findings triage sweep these threads came from.

No linked issue

[cursor]

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 finished @kyle-sexton's task in 1m 44s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Review check-listing-budget.sh for injection / numeric-parsing issues
• Review skill-frontmatter.sh YAML comment-stripping logic
• Review test file changes
• Review doc/manifest/changelog changes
• Post final findings

No security issues found. This PR is scoped to a bash/awk parsing correctness fix plus a documentation wording reconciliation — no GitHub Actions workflow, permissions, or trust-boundary changes are in the diff.

What I checked specifically, given the change:

• to_decimal() (plugins/skill-quality/scripts/check-listing-budget.sh#L122-L124) uses bash arithmetic expansion $((10#$1)) on a value that could in principle be attacker-influenced (an env var override). At every one of the three call sites (#L128, #L136, #L144), to_decimal is invoked after require_positive_number ... int has already validated the same variable against ^[0-9]+$, so only pure-digit strings ever reach the arithmetic expansion — no operator/command injection is possible through this path (confirmed by reading the call order at each site, not just the function in isolation).
• fm_strip_comment() (plugins/skill-quality/scripts/skill-frontmatter.sh#L286-L308) is a pure awk string-scanning function over frontmatter scalar text. Its output (desc, wtu) is only ever used for ${#var} length measurement and printf %s/grep in the reporter — never passed to eval, source, or a shell command line — so a maliciously crafted SKILL.md (e.g. one designed to smuggle shell metacharacters through the description field) cannot achieve command injection via this new code path.
• skill_frontmatter::field's pre-existing dynamic-regex construction ($0 ~ "^" k ":...") takes key from the caller, not from file content, and every call site in this diff passes a literal identifier (description, when_to_use, disable-model-invocation) — unchanged by this PR and not attacker-reachable.
• The docs/PLUGIN-PHILOSOPHY.md, plugin.json (version bump only), and CHANGELOG.md changes are prose/metadata only.

Confidence: high — the diff is small (2 scripts + tests + docs), self-contained, and the validate-then-convert ordering that closes the octal/injection concern is visible at each call site in the same file.

[claude]

Claude finished @kyle-sexton's task in 4m 30s —— View job

---

Code review

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review docs/PLUGIN-PHILOSOPHY.md wording fix
• Review check-listing-budget.sh octal fix
• Review skill-frontmatter.sh comment-stripping fix
• Review check-listing-budget.test.sh new tests
• Review CHANGELOG.md / plugin.json version bump
• Post findings

This repo has a .github/workflows/claude-security-review.yml, so per REVIEW.md this lane owns correctness/design/tests/docs and leaves security to that lane — nothing in this diff needed a security finding regardless.

Octal fix (check-listing-budget.sh). Traced all three require_positive_number ... int call sites (MAX_DESC_CHARS, BUDGET_CHARS, CONTEXT_TOKENS) — each now routes through to_decimal() right after validation, before the value is used arithmetically or printed with %d. $((10#$1)) correctly forces base‑10 for a zero‑padded value (08 → 8, 0123 → 123) and is only reachable after the ^[0-9]+$ gate, so no non-digit input can reach it. The num-kind overrides (CHECK_SKILL_LISTING_CHARS_PER_TOKEN, CHECK_SKILL_LISTING_BUDGET_FRACTION) are correctly left alone — they only reach awk, whose decimal-literal parsing doesn't reinterpret a leading zero as octal. This matches the PR's claim; I didn't find a sibling numeric override in this plugin's other scripts that was missed.

Comment-stripping fix (skill-frontmatter.sh). fm_strip_comment is quote-aware: for a quoted scalar it walks to the matching closing quote (handling \-escapes in double-quoted and doubled-' escapes in single-quoted) and returns through that quote, leaving strip_quotes downstream able to see the real outer quotes; for a plain scalar it cuts at the first whitespace-preceded #, matching YAML's own rule. It's invoked only on the plain/flow branch's val, after the existing block-scalar (|/>) detection, so a # inside a folded/literal block body is untouched — verified against the new fixtures (quoted, plain, # without preceding whitespace, and inside a block scalar). check-skill.sh reads description/when_to_use/name through the same skill_frontmatter::field, so Check 2's per-entry cap and the trigger-preservation diff inherit the fix as claimed, confirmed by grep. metadata_field() (used for upstream-version/summary) still does a naive, non-quote-aware comment strip and wasn't touched — that's fine, it's out of this PR's stated scope (those values aren't length-budgeted), just noting it as a latent, lower-priority instance of the same class of bug if it's ever revisited.

Docs fix (PLUGIN-PHILOSOPHY.md). Verified the new wording ("generic or named") is consistent with the paragraph immediately above it, with the dispatch ladder's actual default, and with check-skill.sh's delegation-wording regex (plugins/skill-quality/scripts/check-skill.sh:1044-1046), which already accepts a generic worker (agent|subagent|worker|advisor|reviewer|verifier|dispatch|delegate) and never requires a named one — so the old "a named subagent" rule text was genuinely out of sync with what the repo enforces. Also confirmed docs/topics/fresh-eyes-checkpoint-audit/PLAN.md:140 specifies the same "generic or named" reconciliation, supporting the PR's account of what was missed and now applied.

Version/changelog. plugin.json and the new ## [0.12.3] entry agree, and a patch bump is the right call for a bug-fix-only release.

Tests. The 7 new cases in check-listing-budget.test.sh cover the two independent bugs plus the guard case that must keep working (0.01 fraction survives). Left one minor suggestion inline on lines 286/296‑297: two of the assertions use an unanchored substring grep ('budget:.*8 chars (override') that would also accidentally match 18 chars/28 chars; harmless today since no path through this fix can produce those values, but worth tightening for future-regression precision.

Not independently verified: I was unable to execute Bash in this session (script/test invocations were consistently blocked pending approval that never resolved), so I could not run the test suite, ShellCheck, or markdownlint myself. The PR's Testing section describes green runs of the full gate list — that is author-claimed and unverified by me; my review is based on static reading of the diff and cross-referencing the call sites by hand.

No correctness, design, or test-coverage issues found beyond the one inline suggestion (nit-level, not blocking).

[github-actions]

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

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review check-listing-budget.sh octal/arithmetic changes for injection risk
• Review skill-frontmatter.sh comment-stripping logic for injection risk
• Review test file changes
• Review doc/manifest/changelog changes
• Post final findings

No security issues found. No GitHub Actions workflow, permissions, secrets, or trust-boundary surface is touched by this diff — it's confined to two plugin scripts, their tests, and prose/metadata (docs, plugin.json version bump, CHANGELOG.md).

What I traced specifically, given the fix's shape:

• to_decimal() does $((10#$1)) on a value that can come from an environment override. I checked all three call sites — MAX_DESC_CHARS, BUDGET_CHARS, CONTEXT_TOKENS — and in every one, to_decimal runs only after require_positive_number ... int has gated the same variable against ^[0-9]+$. That pattern excludes +, -, and anything but bare digits, so nothing that reaches bash arithmetic expansion can carry an operator or command substitution. No ordering gap and no bypass path exists — every write site I found respects the validate-then-convert sequence.
• fm_strip_comment() (skill-frontmatter.sh#L45-L67) is pure awk string-scanning over frontmatter text — no system()/pipe-to-shell calls in the awk program. I traced every downstream consumer of its output (desc, wtu, CUR_DESC, BASE_DESC, etc. across check-listing-budget.sh and check-skill.sh): all uses are ${#var} length measurement, printf with a fixed format string and the value only ever as a %s/%d argument (never as the format string itself), or grep -qxF -- (fixed-string, not regex) — never eval, source, or interpolation into a constructed shell command. A crafted SKILL.md description therefore has no path to command injection or format-string injection through this new code, even with adversarial content (embedded quotes, #, backticks, $(...)).
• Confirmed the awk regex used for the comment boundary (match(v, /[[:space:]]+#/)) is a simple non-backtracking match — no catastrophic-backtracking/ReDoS surface from attacker-controlled frontmatter length.
• docs/PLUGIN-PHILOSOPHY.md, plugin.json (version bump only), and CHANGELOG.md are prose/metadata-only changes with no executable or config-schema impact.
• No .github/workflows/ files are touched by this PR (verified via git diff origin/main...HEAD --name-only), so no CI trigger/permissions/supply-chain surface applies here — consistent with deferring that class of finding to zizmor's lane per instructions.

Confidence: high. The diff is small and self-contained, and the validate-then-convert ordering that closes the arithmetic-expansion concern is directly visible at each call site in the same file, not inferred.

[claude]

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

---

Code review

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review docs/PLUGIN-PHILOSOPHY.md wording fix
• Review check-listing-budget.sh octal fix
• Review skill-frontmatter.sh comment-stripping fix
• Review check-listing-budget.test.sh new tests
• Review CHANGELOG.md / plugin.json version bump
• Post findings

Per REVIEW.md, this repo runs .github/workflows/claude-security-review.yml, so this lane owns correctness/design/tests/docs and leaves security to that lane.

Octal fix (check-listing-budget.sh). Traced all three require_positive_number ... int call sites (MAX_DESC_CHARS L128, BUDGET_CHARS L136, CONTEXT_TOKENS L205) — each routes through to_decimal() right after require_positive_number validates ^[0-9]+$, before any arithmetic or %d use. require_positive_number's own awk -v v="$val" 'BEGIN { exit (v > 0) ? 0 : 1 }' gate also confirms "0"/all-zero values are rejected before to_decimal ever sees them, so there's no zero-division-style edge case. $((10#$1)) correctly forces base-10 regardless of digit value (08→8, 0123→123). The ratio/fraction overrides (CHARS_PER_TOKEN, BUDGET_FRACTION) are correctly left alone — awk's string-to-number conversion doesn't reinterpret a leading zero as octal, and the PR's claim that no other script in the plugin carries this pattern checks out (skill_frontmatter::field/check-skill.sh take no numeric env overrides).

Comment-stripping fix (skill-frontmatter.sh). fm_strip_comment (L45-L64) is correctly quote-aware: for a quoted scalar it walks to the matching closing quote (handling \-escapes and doubled '' single-quote escapes), returning through that quote so strip_quotes downstream still sees real outer quotes; for a plain scalar it cuts at the first whitespace-preceded #, matching YAML's rule that an inline comment must be preceded by whitespace (verified against fixture 15b's tag#7 stays, which correctly survives). It's applied only on the plain/flow branch (print fm_strip_comment(val) at the end), never on the block-scalar (|/>) branch above it, so block content is untouched — confirmed against fixture 15c. check-skill.sh reads description/when_to_use/name through the same skill_frontmatter::field (L212, L240-241, L258-259, L287-288, L297-298), so Check 2's per-entry cap and the trigger-preservation diff correctly inherit the fix, as claimed. metadata_field() still does a naive, non-quote-aware strip and wasn't touched — correctly out of this PR's scope (those values aren't length-budgeted).

Docs fix (PLUGIN-PHILOSOPHY.md). Re-read the surrounding paragraph and the rule sentence together: the new "generic or named" wording is now consistent with both, and with check-skill.sh's delegation-wording regex (L1044-L1046), which accepts a generic worker (agent|subagent|worker|advisor|reviewer|verifier|dispatch|delegate) and never requires a named one. The old "a named subagent" text was genuinely out of sync with what the repo enforces.

Tests. Verified the math by hand for all three new fixtures: the quoted-comment fixture ("12345" + "1234567", joiner 3) sums to 15; the plain-scalar fixture (12345 stripped to 5 + tag#7 stays kept whole at 11, joiner 3) sums to 19; the block-scalar fixture (abc # def, no when_to_use) sums to 9 — all match the asserted aggregates. The octal cases (14/14b/14c/14d) each isolate one call site and the 14d case specifically confirms BUDGET_FRACTION=0.01 survives the normalization untouched. Good coverage of both bugs plus the one guard case that must keep working.

One nit from this PR's prior review round — the unanchored substring match in check-listing-budget.test.sh ('budget:.*8 chars (override' at L286 and L297) — is still present and unresolved in this diff. Leaving it as previously filed rather than re-posting a duplicate inline comment; still non-blocking, since no path through to_decimal can actually produce 18/28 today.

Version/changelog. plugin.json 0.12.2→0.12.3 and the new ## [0.12.3] entry agree in content and are a correct patch bump for a bug-fix-only release.

Not independently verified: I did not execute the test suite, ShellCheck, or markdownlint myself — the PR's Testing section (green run of the full gate list) is author-claimed and unverified by me. My review is based on static reading of the diff, hand-tracing call sites, and hand-computing the test fixture arithmetic.

No correctness, design, or test-coverage issues found beyond the one pre-existing nit noted above (non-blocking).

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review check-listing-budget.sh octal/arithmetic changes
• Review skill-frontmatter.sh comment-stripping logic
• Review test file changes
• Review doc/manifest/changelog changes
• Post final findings

No security issues found. This diff (6 files, docs/PLUGIN-PHILOSOPHY.md, plugin.json, CHANGELOG.md, check-listing-budget.sh, check-listing-budget.test.sh, skill-frontmatter.sh) is confined to a bash/awk parsing correctness fix, its regression tests, and prose/metadata edits. No .github/workflows/ file is touched (confirmed via git diff origin/main...HEAD --name-only), so no CI trigger/permissions/supply-chain surface applies here.

What I traced specifically, given the fix's shape, against b7d1f63 (current HEAD):

• to_decimal() applies $((10#$1)) — bash arithmetic expansion — to a value that can originate from an attacker/user-controlled environment variable. I checked all three call sites: MAX_DESC_CHARS, BUDGET_CHARS, and CONTEXT_TOKENS. In every case to_decimal runs only after require_positive_number ... int has gated the same variable against ^[0-9]+$. That pattern permits only bare digits — no +, -, x, or command-substitution syntax — so nothing that reaches the arithmetic expansion can carry an operator or injected command. The validate-then-convert ordering holds at every write site; there's no bypass path.
• fm_strip_comment() (skill-frontmatter.sh#L45-L69) is pure awk string-scanning over frontmatter text — no system(), no pipe-to-shell. Its output is only ever consumed via ${#var} length measurement or printf %s — never via eval, source, or interpolation into a constructed shell command — so a maliciously crafted SKILL.md description/when_to_use (embedded quotes, #, backticks, $(...)) has no path to command or format-string injection through this new code.
• The comment-boundary regex (match(v, /[[:space:]]+#/)) is a simple linear, non-backtracking match — no ReDoS surface even against adversarial frontmatter length.
• docs/PLUGIN-PHILOSOPHY.md, plugin.json (version bump only), and CHANGELOG.md are prose/metadata-only changes with no executable or config-schema impact.

Confidence: high. The diff is small and self-contained, and the validate-then-convert ordering that closes the arithmetic-expansion concern is directly visible at each call site in the same file, not inferred.

[claude]

Claude finished @kyle-sexton's task in 4m 30s —— View job

---

Code review

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review docs/PLUGIN-PHILOSOPHY.md wording fix
• Review check-listing-budget.sh octal fix
• Review skill-frontmatter.sh comment-stripping fix
• Review check-listing-budget.test.sh new tests
• Review CHANGELOG.md / plugin.json version bump
• Post findings

Per REVIEW.md, this repo runs .github/workflows/claude-security-review.yml, so this lane owns correctness/design/tests/docs and leaves security to that lane.

Octal fix (check-listing-budget.sh). I diffed the feature branch against its actual pre-merge base (a89a4a3, the merge-base of baa0e20 and the main tip it was merged with) since the local origin/main ref was stale and network fetch was unavailable in this sandbox. Confirmed against the pre-fix version of the file that the bug is real: before this PR, MAX_DESC_CHARS/BUDGET_CHARS/CONTEXT_TOKENS flowed straight from require_positive_number into bash ((...))/printf %d with no decimal normalization. to_decimal() ($((10#$1))) now sits between validation and use at all three call sites. Traced each: to_decimal only ever receives a string that already matched ^[0-9]+$ and passed the awk-based >0 check, so it can't receive anything but digits, and forcing base 10 is correct regardless of which digits are present (no "is this valid octal" branching needed). The num-kind overrides (CHARS_PER_TOKEN, BUDGET_FRACTION) are correctly left alone since they only reach awk, whose string-to-number conversion doesn't treat a leading zero as octal.

Comment-stripping fix (skill-frontmatter.sh). fm_strip_comment is quote-aware and only applied on the plain/flow branch (print fm_strip_comment(val)), never on the block-scalar branch above it. Hand-traced the quoted-scalar walk (escape handling for \" and doubled '') and the plain-scalar whitespace-before-# rule against the new fixtures — the logic is correct, including the edge cases (# as the entire value, an unterminated quote left whole, a # with no preceding whitespace surviving as content). normalize_bool's redundant sed strip was correctly removed given it has exactly one caller (check-listing-budget.sh:271, confirmed by grep) that already sources through skill_frontmatter::field.

Docs fix (PLUGIN-PHILOSOPHY.md). Read the full "Fresh-eyes checkpoints" section: the preceding paragraph already said "generic or named" pre-PR (confirmed by diffing against a89a4a3), so the fix correctly brings only the out-of-sync rule sentence into line with it. Also confirmed check-skill.sh's delegation-wording regex (agents?|workers?|advisors?|reviewers?|verifiers?|dispatch...|delegat..., around check-skill.sh#L1044) never requires a named agent, and docs/topics/fresh-eyes-checkpoint-audit/PLAN.md:140 states the same "generic or named" reconciliation — both support the PR's account of what was missed.

Tests. Hand-computed the arithmetic for all three new comment-stripping fixtures (quoted: 5+3+7=15, plain: 5+3+11=19, block: 9) and they match the asserted aggregates. The octal fixtures correctly isolate each call site and 14d correctly guards that BUDGET_FRACTION=0.01 survives.

Version/changelog. Worth noting for the record: plugin.json/CHANGELOG.md now show 0.14.1, not the 0.12.3 visible in the raw PR diff — this branch's main baseline (0.12.2) was behind current main (0.14.0) by the time of the merge commit (b7d1f63), and the merge resolution correctly renumbered the bump onto the current tip (0.14.0 → 0.14.1) and folded this PR's two Fixed bullets into that entry rather than leaving a stale 0.12.3 heading mid-changelog. No duplication or lost content — verified by reading the full changelog header order (0.14.1 → 0.14.0 → 0.13.0 → 0.12.2 → …).

Pre-existing, still open: the unanchored substring assertions in check-listing-budget.test.sh (currently around lines 286 and 297, 'budget:.*8 chars (override') flagged in an earlier review round are still present in this diff. Non-blocking — no path through to_decimal can produce 18/28 today — leaving as previously filed rather than re-filing a duplicate inline comment on the same unresolved thread.

Not independently verified: I could not execute Bash for test/lint runs in this sandbox (network and several shell operations required approval that wasn't available), so the PR's Testing section (green run of the full gate list, check-changelog-parity.sh, etc.) is author-claimed and unverified by me. My review is based on static reading of the diff, hand-tracing call sites and the merge history, and hand-computing the test fixture arithmetic.

No correctness, design, or test-coverage issues found. The only outstanding item is the pre-existing, non-blocking test-assertion nit noted above.

[claude]

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

---

Code review

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review docs/PLUGIN-PHILOSOPHY.md wording fix
• Review check-listing-budget.sh octal fix
• Review skill-frontmatter.sh comment-stripping fix
• Review check-listing-budget.test.sh new tests
• Review CHANGELOG.md / plugin.json version bump
• Post findings

Per REVIEW.md, this repo runs .github/workflows/claude-security-review.yml, so this lane owns correctness/design/tests/docs and leaves security to that lane. This review is against the current tip (9d8ff35), whose diff vs origin/main is unchanged in substance from the prior review round — the new commit is a merge sync from origin/main, not new content (confirmed: git diff origin/main...HEAD --stat matches the PR's six listed files exactly).

Octal fix (check-listing-budget.sh). Traced all three require_positive_number ... int call sites (MAX_DESC_CHARS, BUDGET_CHARS, CONTEXT_TOKENS) — each routes through to_decimal() ($((10#$1))) immediately after the ^[0-9]+$ validation and before any bash arithmetic or %d use. Since the validation regex admits only bare digits, to_decimal can never receive anything but a digit string, so forcing base 10 is unconditionally correct. The num-kind overrides (CHARS_PER_TOKEN, BUDGET_FRACTION) are correctly left untouched since they only reach awk, which parses a leading zero as decimal, not octal — confirmed this holds by reading require_positive_number's own awk -v v="$val" 'BEGIN { exit (v > 0) ? 0 : 1 }' gate, which uses the same decimal parsing.

Comment-stripping fix (skill-frontmatter.sh). fm_strip_comment (L45-L69) is correctly quote-aware: I hand-traced the double-quote branch's backslash-pair skip (\\" correctly treated as escaped-backslash-then-close, \" correctly treated as an escaped quote that doesn't terminate the scalar) and the single-quote doubled-'' escape, and both terminate at the right closing quote in every case I checked, including malformed/unterminated input falling back to returning the value whole. It's wired in only on the plain/flow branch (print fm_strip_comment(val)), after the existing block-scalar |/> handling already prints and exits — so a # inside a folded/literal block body is structurally unreachable by this new code, not just untested. Confirmed via grep that normalize_bool's only caller (check-listing-budget.sh:271) already sources its value through skill_frontmatter::field, so removing its redundant sed comment-strip doesn't change behavior. check-skill.sh's reads of description/when_to_use (Check 2, trigger-preservation diff) go through the same skill_frontmatter::field, so they correctly inherit the fix.

Docs fix (PLUGIN-PHILOSOPHY.md). Re-read the "Fresh-eyes checkpoints" section as a whole: the paragraph immediately above the rule already says "generic or named," so the fix brings the previously out-of-sync rule sentence into line with both that paragraph and check-skill.sh's actual delegation-wording regex (L1044-L1046), which matches any of agent(s)/subagent(s)/worker(s)/advisor(s)/reviewer(s)/verifier(s)/dispatch.../delegat... and never requires a named agent — verified directly by reading the regex, not just taking the PR's word for it.

Tests. The 7 new cases correctly isolate each of the three octal call sites, the per-entry cap, and the fraction guard (0.01 surviving untouched), plus three comment-stripping fixtures (quoted, plain-with-non-comment-#, block-scalar). I hand-verified the arithmetic in each fixture's expected aggregate and it checks out.

Still open, non-blocking: the unanchored substring assertion flagged in an earlier review round ('budget:.*8 chars (override', now at check-listing-budget.test.sh#L296) is still present in this diff — it would also match 18/28 chars, though no path through to_decimal can produce those values today. Leaving it against the existing inline thread rather than re-filing a duplicate comment.

Version/changelog. plugin.json (0.14.0 → 0.14.1) and the new ## [0.14.1] CHANGELOG.md entry agree in content, correctly sit above ## [0.14.0], and a patch bump is right for a bug-fix-only release.

Not independently verified: I did not execute the test suite, ShellCheck, or markdownlint in this session — the PR's Testing section (green run of the full gate list) is author-claimed and unverified by me. This review is based on static reading of the diff and hand-tracing the call sites, awk logic, and test fixture arithmetic.

No correctness, design, or test-coverage issues found beyond the one pre-existing, non-blocking test-assertion nit noted above.

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review check-listing-budget.sh octal/arithmetic changes
• Review skill-frontmatter.sh comment-stripping logic
• Review test file changes
• Review doc/manifest/changelog changes
• Post final findings

No security issues found. Diff reviewed against 9d8ff35 (git diff origin/main...HEAD) — 6 files: docs/PLUGIN-PHILOSOPHY.md, plugins/skill-quality/.claude-plugin/plugin.json, plugins/skill-quality/CHANGELOG.md, plugins/skill-quality/scripts/check-listing-budget.sh, plugins/skill-quality/scripts/check-listing-budget.test.sh, plugins/skill-quality/scripts/skill-frontmatter.sh. No .github/workflows/ file is touched, so no CI trigger/permissions/supply-chain surface applies (that class is zizmor's lane regardless).

What I traced, given the fix's shape:

• to_decimal() applies $((10#$1)) — bash arithmetic expansion — to a value that can originate from an environment override. Confirmed at all three call sites — MAX_DESC_CHARS, BUDGET_CHARS, CONTEXT_TOKENS — that to_decimal only ever runs after require_positive_number ... int has gated the same variable against ^[0-9]+$. That pattern permits only bare digits — no +, -, whitespace, or command-substitution syntax — so nothing reaching the arithmetic expansion can carry an operator or injected command. The validate-then-convert ordering holds at every write site; no bypass path exists.
• fm_strip_comment() (skill-frontmatter.sh#L45-L69) is pure awk string-scanning over frontmatter text — no system(), no pipe-to-shell. Its return value is only ever consumed via ${#var} length measurement or printf %s/grep -qxF -- (fixed-string) downstream — never eval, source, or interpolated into a constructed shell command line — so a maliciously crafted SKILL.md description/when_to_use (embedded quotes, #, backticks, $(...)) has no path to command injection through this new code.
• The comment-boundary regex (match(v, /[[:space:]]+#/)) is a simple linear, non-backtracking match — no ReDoS surface even against adversarial frontmatter length.
• docs/PLUGIN-PHILOSOPHY.md, plugin.json (version bump only), and CHANGELOG.md are prose/metadata-only changes with no executable or config-schema impact.
• Test-file additions (check-listing-budget.test.sh) only add fixtures and assertions; no security-relevant surface.

Confidence: high. The diff is small and self-contained, and the validate-then-convert ordering that closes the arithmetic-expansion concern is directly visible at each call site in the same file, not inferred. This matches the conclusion of the prior security-review passes on this PR — no new findings on this iteration.

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review check-listing-budget.sh octal/arithmetic changes
• Review skill-frontmatter.sh comment-stripping logic
• Review test file changes
• Review doc/manifest/changelog changes
• Post final findings

No security issues found.

This PR (6 files: docs/PLUGIN-PHILOSOPHY.md, plugins/skill-quality/.claude-plugin/plugin.json, plugins/skill-quality/CHANGELOG.md, plugins/skill-quality/scripts/check-listing-budget.sh, plugins/skill-quality/scripts/check-listing-budget.test.sh, plugins/skill-quality/scripts/skill-frontmatter.sh) is confined to a bash/awk parsing correctness fix, its regression tests, and prose/metadata changes. No .github/workflows/ file is touched, so no CI trigger/permissions/supply-chain surface applies here (that class is zizmor's lane regardless).

What I traced specifically, reading the code directly at HEAD (6b62cdb):

• to_decimal() applies $((10#$1)) — bash arithmetic expansion — to a value that can originate from an environment override an operator controls. Checked all three call sites: MAX_DESC_CHARS, BUDGET_CHARS, CONTEXT_TOKENS — in every case to_decimal runs only after require_positive_number ... int gates the same variable against ^[0-9]+$. That pattern admits only bare digits — no +, -, whitespace, or command-substitution syntax — so nothing that reaches the arithmetic expansion can carry an operator or an injected command. The validate-then-convert ordering holds at every write site; there is no bypass path (no code path assigns to MAX_DESC_CHARS/BUDGET_CHARS/CONTEXT_TOKENS from the override and skips require_positive_number before to_decimal runs).
• fm_strip_comment() is pure awk string-scanning over frontmatter text — no system(), no pipe to a shell, no dynamic regex built from file content (the k interpolated into the awk pattern is always a literal caller-supplied key, e.g. description/when_to_use/disable-model-invocation, never data read from the SKILL.md itself). Traced every consumer of its output (desc, wtu in check-listing-budget.sh): used only for ${#var} length measurement and printf %d/%s with a fixed format string — never eval, source, or interpolated into a constructed shell command line. A maliciously crafted SKILL.md description (embedded quotes, #, backticks, $(...)) has no path to command or format-string injection through this new code.
• The comment-boundary regex (match(v, /[[:space:]]+#/)) is a simple, linear, non-backtracking match — no ReDoS surface even against adversarial frontmatter length.
• docs/PLUGIN-PHILOSOPHY.md is a prose-only wording fix; plugin.json is a version-number bump only (confirmed the file still carries exactly one version field, no other schema change); CHANGELOG.md is a changelog entry. None touch executable or config-schema surface.
• check-listing-budget.test.sh's new fixtures only write to a mktemp scratch dir and read report output via grep/string comparison — no security-relevant surface.

Confidence: high. The diff is small and self-contained, and the validate-then-convert ordering that closes the arithmetic-expansion concern is directly visible at each call site in the same file, not inferred.
(branch)

[claude]

Claude finished @kyle-sexton's task in 3m 27s —— View job

---

Code review

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review docs/PLUGIN-PHILOSOPHY.md wording fix
• Review check-listing-budget.sh octal fix
• Review skill-frontmatter.sh comment-stripping fix
• Review check-listing-budget.test.sh (including the previously-flagged unanchored assertion)
• Review CHANGELOG.md / plugin.json version bump
• Post findings

Per REVIEW.md, this repo runs .github/workflows/claude-security-review.yml, so this lane owns correctness/design/tests/docs and leaves security to that lane (already covered exhaustively there).

The previously-flagged nit is now fixed. Commit 6b62cdb anchors the digit boundary in the two octal-override test assertions (check-listing-budget.test.sh:296,306) with grep -qE 'budget:.*(^|[^0-9])123 chars \(override' / ...8 chars \(override'. I traced the actual printed line (printf ' budget: %d chars (%s)\n' ... at check-listing-budget.sh#L306) — the character immediately preceding the digits is always a literal space from the format string, so [^0-9] correctly matches there for legitimate values, while a spurious 18/28 chars would fail the anchor (the char before the trailing digit is itself a digit, and ^ can't apply mid-line). The fix is correct and closes the gap the earlier review round flagged.

Independent re-verification of the core fix (static, hand-traced):

• to_decimal() (check-listing-budget.sh#L178-180) is applied at all three call sites (MAX_DESC_CHARS L184, BUDGET_CHARS L194, CONTEXT_TOKENS L205), always immediately after require_positive_number ... int gates the same variable to ^[0-9]+$. I traced every downstream use of these three variables (L284, L306, L308-318) and confirmed none of them reference the pre-to_decimal value — the normalization isn't shadowed or bypassed anywhere.
• fm_strip_comment() (skill-frontmatter.sh#L45-67) — hand-traced the quoted-scalar walk and the plain-scalar whitespace-before-# rule against several edge cases beyond the shipped fixtures (empty quoted string, a value that's entirely #, a # immediately following a close-quote with no space), all resolve correctly. It's wired in only on the plain/flow branch, strictly after the block-scalar (|/>) branch already prints and exits, so block content is structurally unreachable by the new stripping code.
• Docs fix: re-read the full "Fresh-eyes checkpoints" section end-to-end — the paragraph above the rule (L512) and the rule sentence itself (L518) now both say "generic or named," and independently confirmed via grep that check-skill.sh's delegation-wording regex (check-skill.sh#L1049) matches generic worker terms (agents?|workers?|advisors?|reviewers?|verifiers?|dispatch...|delegat...) and never requires a named agent — the doc fix is accurate to what the repo enforces, not just self-consistent.
• Version/changelog: plugin.json (0.14.1) and the ## [0.14.1] CHANGELOG.md entry agree and sit correctly above ## [0.14.0].

Not independently verified: Bash execution was blocked pending approval that never resolved in this sandbox (same limitation noted in earlier review rounds on this PR), so I could not run the test suite, ShellCheck, or markdownlint myself. The PR's Testing section (green run of the full gate list) is author-claimed and unverified by me. This review is based on static reading of the diff and hand-tracing call sites, awk logic, and the regex fix.

No correctness, design, or test-coverage issues found. The single outstanding nit from earlier rounds is resolved in this diff.