Skip to content

feat(claude-ops): add audit-skill-visibility — why most of a skill fleet never gets used - #3035

Merged
kyle-sexton merged 23 commits into
mainfrom
claude/skills-discovery-plugin-z1ij9y
Aug 19, 2026
Merged

feat(claude-ops): add audit-skill-visibility — why most of a skill fleet never gets used#3035
kyle-sexton merged 23 commits into
mainfrom
claude/skills-discovery-plugin-z1ij9y

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

No linked issue

Summary

Adds /claude-ops:audit-skill-visibility — a read-only skill that audits whether the model can actually see each installed skill, which is the question behind "why does most of my fleet never get used?" A skill the model cannot see can never be chosen, so unused is very often a visibility failure rather than a preference.

The work started as a usage heat map. Investigation killed that framing: Claude Code already ships unused-skill reporting (/doctor Check 1, and /skill-doctor, whose function moved into a Stats tab — both verified in the 2.1.232 bundle). Rebuilding it would duplicate a shipped feature. The differentiated remainder is why, and the mechanism turned out to be documented: the skill listing is budgeted at skillListingBudgetFraction of the context window, and on overflow Claude Code drops descriptions starting with the skills you invoke least. A skill at zero usage loses its description, loses the keywords a request would match, and stays at zero.

Fix

New skill (plugins/claude-ops/skills/audit-skill-visibility/) reporting three independent fields per skill, because one flat verdict collapses questions that demand opposite actions:

  • reachabilitymodel-reachable / user-only / hidden / misconfigured / unknown, each with causes, evidence, and a remedy. misconfigured remedies are asserted never to contain removal wording: several causes are silent (malformed frontmatter loads with empty metadata and surfaces only under --debug), so they read exactly like disuse.
  • observationactive / cooling / dormant / no-observation-in-horizon / not-observable, always horizon-qualified.
  • starvation — budget-contest state, split by confidence. Whether the listing overflows is arithmetic over documented settings; which skills lose descriptions comes from an undocumented scorer and ships as a labelled likelihood band, never a cutoff.

Two properties are enforced in code, not documented as intent:

  • Cold verdicts are withheld when the data cannot support them. A store younger than the window being asked about cannot distinguish "never invoked" from "never observed". On this container — a 3-day-old install — a naive report would have labelled 210 of 213 skills "never used". Windows clamp to a computed observed_horizon; declined claims land in a first-class withheld section with reasons.
  • Sources are reconciled, never summed. Native counters and skill-usage.jsonl were demonstrated recording the same invocation with matching sub-second timestamps. At an instant the count is the max across sources — while two same-instant events from one source still count twice, because those are two real invocations.

Supporting changes:

  • clean.sh gains an opt-in skill-usage.jsonl retention target on its own 365-day window. Inert unless --skill-usage-scope is passed — that is the rollback path. Scope and directory arrive as flags, never environment: a skill-spawned clean.sh inherits no CLAUDE_PLUGIN_OPTION_*, and CLAUDE_PLUGIN_DATA in that context was observed pointing at an unrelated plugin's data directory, so data-dir exits 2 rather than guessing a delete path.
  • plugins/claude-ops/lib/state-key.sh added as a registered carrier and enrolled in scripts/sync-state-key.sh, so reports key by repo identity and worktree instead of collapsing to one file per machine.
  • Registration across the nine-file precedent set; claude-ops 0.32.6 → 0.33.0.

Verification

All gates green on the final commit:

Gate Result
scripts/validate-plugins.sh Validation passed
check-skill.sh --require-evals audit-skill-visibility PASS — 0 errors, 0 warnings
check-skill.sh observability PASS — 0 errors, 0 warnings
Skill unit tests 52 passed
claude-observability.test.sh 32/32 passed
check-skill-leaf-names.sh --check pass
check-manifest-duplicate-keys.py pass
sync-plugin-options-docs.py --check pass
sync-state-key.sh --check All 2 plugin copies match
check-cross-plugin-source-drift.sh --check no drift
ruff / shellcheck / markdownlint clean

Specific regressions pinned by test, each a defect found during the audit:

  • A 1M-token context yields budget_chars == 40000, not the familiar 8,000 — the budget is derived, and hardcoding 8,000 would be wrong for most current models.
  • Skills with disable-model-invocation (59 of 213 locally, 28% of the fleet) contribute zero to the demand sum; the fixture gives them full-length descriptions to prove it. Counting them would inflate overflow enough to flip the headline verdict.
  • The double-count fixture asserts the reconciled total equals the max, not the sum.
  • pluginUsage install-seeded rows never read as usage or recency (measured: guardrails 105, claude-ops 90, both with zero skill invocations).
  • --follow recovers history a rename severs, and authored_at ignores a filesystem mtime pushed 30 days into the future.
  • clean.sh with no skill-usage flag leaves the store untouched (the rollback property), while data-dir, traversal, and unknown scopes each exit 2.

Related

N/A — no tracked issue. Design record: docs/topics/skills-discovery-plugin/ (Brief, design/design-threads.md, design/contracts.md, design/module-boundary.md).

Context on the naming: the skill was first audit-skill-adoption, rejected because "adoption" promises consumer adoption in a public marketplace, which single-operator telemetry cannot measure. It shipped briefly as audit-skill-starvation and was renamed before this PR — starvation is one visibility failure among several, and "visibility" is Claude Code's own term (skillOverrides is documented under "Override skill visibility").


Generated by Claude Code

claude added 20 commits August 17, 2026 03:32
…y heat-map topic

Survey grounding plus the round-1 open-question register for the
/planning:interview session defining the skills usage heat-map capability.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
…, register round 2

Round 1 accepted (usage-primary lens, claude-ops skill placement, fleet
denominator, markdown+HTML output, heuristics-only why-unused). Investigation
found native skillUsage/pluginUsage counters in ~/.claude.json, verified the
bespoke skill-usage.jsonl store live, and surfaced its retention gap.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Round 2 accepted (hybrid native-first sources, retention fix in scope with its
own window, opportunistic source ladder with labeled coverage). Registers the
unanswered multi-operator probe as Q9 rather than assuming single-operator, plus
naming, churn-axis, integration-seam, and cold-tier questions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
The topic-docs binding puts interview-checklist.md in the memory tier
(.work/<topic-slug>/, never committed); the contract slice under docs/topics/
is the tracked half. Earlier commits in this branch tracked the ledger by
mistake. The memory slice carries its own self-ignore, so the working files
stay local from here.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
…ivot

Four fresh-context agents (two adversarial validators with the orchestrator
rationale withheld, one external researcher, one build-surface explorer)
overturned the original premise rather than refining it.

Two findings drove the pivot. Claude Code already ships unused-skill reporting
(/doctor Check 1; /skill-doctor, whose function moved into a Stats tab),
verified directly in the 2.1.232 bundle. And the skill listing is budgeted at
~1% of context, dropping descriptions least-invoked-first — so "unused" is
partly self-causing, and the differentiated job is separating starved from
genuinely unwanted.

The Brief also records defects that would have shipped a confidently wrong
report: a demonstrated native/JSONL double-count, pluginUsage counting
hook/agent/MCP/LSP dispatch rather than skill invocation (46 of 65 plugins read
as "used today" when none had been used), a 60-second counter debounce,
lifetime-only counts, and tier windows exceeding the observable horizon by an
order of magnitude. An honesty floor with a not-observable tier is now a
load-bearing constraint.

Register gate clean: 19 registered, 0 open, 4 deferred, brief cross-check ok.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
…ation

A fresh-context verifier checked the load-bearing research claims against the
live docs (raw fetch, not summarization) and the v2.1.232 bundle. Three
corrections, one of which removes a false statement.

- The "13 documented reasons a skill is never auto-invoked" claim was wrong: no
  such official list exists. Replaced with 15 evidenced causes (11 genuinely
  silent), provenance stated, and a standing rule never to cite them to a user
  as documented.
- The listing budget's "8,000 chars" is derived, not a constant: fraction x
  context window x a 4-bytes-per-token estimate. On a 1M-context model it is
  ~40,000. Hardcoding 8,000 would be wrong for most current models.
- Bundled prompt skills and skillOverrides name-only entries are exempt from the
  description contest, so a starvation report built on pure usage ranking
  misattributes both.

Also recorded: the counter debounce suppresses lastUsedAt as well as the count
and does not apply to telemetry, so OTEL and local counts legitimately disagree;
redaction vocabulary differs per event; plugin_loaded needs per-session dedupe by
plugin_id_hash with safe_mode sessions excluded.

Confirmed safe to build on: skill_activated with its three trigger values, the
two listing settings and the budget env var, and plugin_loaded as the denominator.

Register gate clean; markdownlint 0 issues.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Nine threads with statuses. Three are open and need the user: the source
strategy (collector required vs declared capability tiers), how "starved" is
computed, and whether reach state is one enum or two orthogonal axes.

The starvation thread splits the claim in two, which is the substantive design
move: whether the listing overflows at all is arithmetic over documented inputs
(budget fraction x context window x bytes-per-token, against the summed
description lengths), while which specific skills lose descriptions depends on
the undocumented scorer. The first ships as a finding, the second as a labeled
likelihood band.

Q18 stays USER-RESERVED and untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
… boundary

T1, T2, T3 approved. All threads now resolved or directional; none open.

Type modeling caught an error in the approved T3 shape: `exempt` was placed on
the reachability axis, but a bundled prompt skill is fully model-reachable and
merely exempt from the description contest. Left there, every bundled skill
would have rendered as unreachable. The row now carries three independent
fields — reachability, observation, starvation — and the Brief's acceptance
criterion 2 is superseded with a dated note rather than a silent rewrite.

contracts.md fixes the field vocabulary, the capability tiers, the budget
arithmetic split by confidence, and the report schema, including a first-class
`withheld` section so a declined verdict is visible rather than missing.

module-boundary.md states ownership (one writer, many readers), the
collect/classify/render split that keeps the judgement pure and testable, and
nine fixtures each pinning a defect the audit found.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
The binary gate caught three threads marked DIRECTIONAL with no research tag,
which fails as written. They had in fact been resolved by contracts.md and
module-boundary.md, so the fix records where each decision's rationale lives
rather than softening the check: a thread marked "decided" without recorded
rationale hands /planning:plan an unexplainable decision.

All nine threads now read RESOLVED with rationale. Gate PASSES.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Tracer-bullet ordering: phase 1 is a thin end-to-end slice with the honesty
floor live from the first commit, rather than a horizontal layer that only
becomes runnable at the end.

Several sanity checks are regression guards against defects this session already
found: a 1M-token fixture asserts budget_chars == 40000 so the hardcoded 8,000
cannot return, the double-count fixture asserts max rather than sum, and a grep
proves no misconfigured row renders alongside removal wording.

Registration follows precedent 4a1184c (nine files, two generated), including
the two files that deliberately do not change.

Draft state — a fresh-context plan reviewer is running against it; findings will
land before this is presented for approval. Known gap already: the classifier's
implementation language is written as classify.* and is not yet decided.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
A fresh-context reviewer returned 13 findings against the draft plan; each was
verified against the actual files before being applied. Three would have shipped
defects.

The headline correctness bug: the budget demand sum counted
disable-model-invocation skills, which spend no description budget at all. That
is 59 of 213 skills locally — 28% of the fleet — and including them inflates
overflow_chars enough to flip the run's headline verdict. This repo's own
check-listing-budget.sh already skips them for exactly that reason, which also
surfaced a reuse decision the draft never argued; the plan now argues replace
and requires a cross-reference in both files.

The dangerous one: clean.sh is skill-spawned, so it inherits no
CLAUDE_PLUGIN_OPTION_*, and its data-dir branch would need CLAUDE_PLUGIN_DATA —
which the repo's smoke test observed pointing at an unrelated plugin's data
directory. A prune trusting it could delete another plugin's files. Scope and
dir now arrive as explicit flags, the data-dir branch re-derives its own path,
and the change ships with a rollback story.

The blocking one: CI FAILs any SKILL.md diff lacking evals/evals.json, so
deferring evals to the last phase meant phases 1-5 could never land.

Also: acceptance criteria 11 and 12 had no phase at all (churn is now Phase 5);
the classifier language is pinned to Python per repo precedent rather than left
as classify.*; JSON lands in Phase 1 so later checks are mechanical; sanity
checks move to fixtures with exact counts instead of machine-dependent numbers;
the state-key sync array and Test 9 join the file table; and AC 10's "four
locations" is corrected to six.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Q18 was reserved for this gate specifically so it would be decided with
plan-time context rather than during the interview. Approving the plan as
presented resolves it: no phase reads session transcripts, so V1 stays
machine-local and the documented cloud/ephemeral gap stands, with transcripts
noted as the known path if the scope-limited framing proves too narrow.

Execution shape is fully sequential; Phase 1 gates the rest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
First slice of the skill-starvation diagnostic: denominator through a pure
classifier to markdown and JSON, running end to end against a fixture.

The honesty floor is live from this first commit rather than bolted on later,
because it is the whole point of the skill. A usage store younger than the
window being asked about cannot tell "never invoked" from "never observed", and
reporting the second as the first libels most of a fleet on a fresh install —
measured during design as 210 of 213 skills. The engine computes an
observed_horizon, clamps every window to it, and routes unsupported claims into
a first-class withheld section with reasons.

Reconciliation is MAX across sources at an instant, never a sum: native counters
and the JSONL store record the same invocation, while two same-instant events
from one source are genuinely two invocations. pluginUsage is gated behind
usageCount > 0 because its rows are install-seeded with a current lastUsedAt.

classify() is pure with an injected clock, which is what makes the horizon
behavior testable at all. Eleven unit tests each pin a defect found in audit;
the wrapper additionally asserts the fresh-install fixture withholds every cold
verdict end to end.

evals ship in this phase deliberately — check-changed-skills.sh passes
--require-evals for any SKILL.md diff, so deferring them would have blocked
every later phase.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Adds the second of the three independent fields: can the model ever select this
skill, kept orthogonal to whether it has been observed.

The orthogonality is the point. A single flat verdict collapses "cannot be
selected" with "has not been seen", and those demand opposite actions — only
model-reachable-with-no-observation is a starvation candidate, user-only means
the operator types it by design, and misconfigured is a fix.

Two guards ride the misconfigured class. Its remedies are phrased as fixes and
asserted never to contain removal wording, because several of its causes are
silent — the skill looks fine, can never be selected, and nothing surfaces
outside --debug — so they read exactly like disuse. And every row carries
provenance saying the cause catalogue is assembled from scattered docs plus
binary strings rather than an official list, because no such list exists and
citing one to a user would cite something that is not there.

Enablement arrives as an input rather than being read here, so classify() stays
pure; undetermined resolves to unknown rather than being guessed.

Exact counts are asserted against a four-skill fixture of known composition
rather than the live machine, whose fleet size varies and cannot distinguish a
regression from a smaller install.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
The third field, and the one carrying the skill's headline finding. The claim is
split by confidence because the two halves genuinely differ in how much they can
be trusted.

The certain half — whether the listing overflows and by how much — is arithmetic
over documented settings: budget fraction times context window times the
product's own bytes-per-token estimate, against summed description lengths. No
undocumented constant is involved, so it renders as a plain statement rather
than a hedge, and it holds at every capability tier.

The inferential half — which particular skills lose their descriptions — comes
from a scorer that is undocumented and pinned to one build. It renders as a
ranked likelihood band, explicitly labelled, never as a cutoff line.

The budget is DERIVED, never hardcoded: a 1M-token context yields 40,000
characters, and the familiar 8,000 is only that formula at a 200k window. A test
pins both, so nobody reinstates the constant. SLASH_COMMAND_TOOL_CHAR_BUDGET
short-circuits everything and has its own test.

Three exempt classes spend zero budget, including the disable-model-invocation
class that plan review caught — 59 of 213 skills locally, whose inclusion would
inflate the overflow enough to flip the verdict. The fixture gives the exempt
rows descriptions as long as the competing ones and asserts they contribute
nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Three parsers and a tier resolver, so a claim renders only where the present
sources can support it rather than varying silently with whatever happened to be
on the machine.

Reconciliation is the load-bearing rule: native counters and the JSONL store
both record the same invocation, so at a given instant the count is the MAX
across sources, never the sum. Two genuine same-instant events from ONE source
still count twice, which is why this is not a plain dedupe — id-less rows at
second granularity would collapse real invocations.

parse_otel flags `custom_skill` as redacted and leaves it unattributed.
It is the placeholder for user-defined and third-party skills, not a name;
attributing it would pile every third-party skill's usage onto one fictional
row.

parse_native gates on usageCount > 0 and takes firstStartTime as its horizon,
since those counters cannot see before the config file existed. parse_jsonl
skips a malformed row rather than failing the whole report — one bad line in an
observability store must not cost the report.

Tier capabilities are explicit: invocation_trigger only at T-full, and no
windowed claim at T-baseline because native counts are lifetime-since-install.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
This phase exists because plan review found acceptance criteria 11 and 12 had no
phase at all — churn was in the Brief and in no work item.

Two git mechanics are deliberate and are now proved against a real temp repo
rather than asserted. --follow recovers history that a rename severs, which
matters because this repo demonstrably ports skills between plugins; without it
a ported skill reports only the commits since its move. And authored_at comes
from the committer date, never filesystem mtime — in a fresh clone every mtime
is the checkout time, which would report the entire fleet as authored today. The
test pushes a file's mtime 30 days into the future and asserts the reported date
ignores it.

Skills authored in another repo render blank, not zero: no measurement is a
different fact from never touched, and zero would read as neglect.

The section carries an explicit label against a waste reading, guarded by a
grep. In a public marketplace a skill is authored FOR CONSUMERS, so high
authoring effort beside low local use is the normal case — reading that quadrant
as wasted effort would invert what it means.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Adds claude-ops's first lib/ as a registered carrier of the canonical
state-key.sh, and writes the JSON report to a keyed path.

The carrier is added to sync-state-key.sh's copies= array in the same change,
which is the point: a third copy left out of that array is unmaintained by the
sync gate, and any byte of divergence then fails the drift check. Both gates now
pass — "All 2 plugin copies match", no unregistered or drifted clusters.

report_path RAISES on an empty data root or state key rather than defaulting.
CLAUDE_PLUGIN_DATA carries no project, worktree, or session segment, so a fixed
filename under it is one file per machine — every repo overwrites the last, and
a read-back can serve one project's findings as another's. Worse, in a
skill-spawned subprocess that variable was observed pointing at an unrelated
plugin's data directory, so trusting it silently is how a report lands somewhere
surprising. The CLI turns the refusal into a clean exit 2 naming the command
that produces a key.

Output is one file per run plus an appended history line, never a rolling
latest.json, so a future scheduled run cannot inherit an overwrite defect.

The optional HTML view is deferred, not dropped: markdown is the durable record,
and with no live collector there is no large scope to render yet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
Final phase. Adds the opt-in skill-usage retention target and registers
audit-skill-starvation across the catalog surfaces; claude-ops 0.32.6 -> 0.33.0.

The retention change is the only destructive path in an otherwise read-only
feature, so it is proven by test rather than argument. Test 9 grew eight
assertions and the rollback property is asserted first: with no skill-usage flag
the old rows survive a hook prune, so a run without the flag behaves exactly as
before and reverting is dropping one branch. The rest pin that the target uses
its own 365-day window instead of inheriting --keep-days, that --dry-run names
the resolved target and changes nothing, and that a short window really does
prune.

Scope and directory arrive as flags because clean.sh runs as a skill-spawned
subprocess: it inherits no CLAUDE_PLUGIN_OPTION_*, so it cannot see the hook's
configured scope, and CLAUDE_PLUGIN_DATA in that context was observed pointing at
an unrelated plugin's data directory. Deriving a delete path from either could
prune someone else's files. data-dir therefore exits 2 rather than guessing, a
traversing dir exits 2, and an unknown scope exits 2 rather than defaulting.

Six retention and flag surfaces moved together — the count acceptance criterion
10 originally got wrong, corrected during plan review from four.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
…isibility

The old name failed both tests it should have passed. Semantically it was
narrow: starvation is ONE way a skill goes unseen, but the skill also reports
skills hidden by an override, disabled by their plugin, invisibly misconfigured,
or kept out of context by disable-model-invocation. And "starvation" was a
metaphor coined in this session, so a reader had to learn it before the name
meant anything.

"Visibility" is not invented — it is Claude Code's own term for this concept:
skillOverrides is documented under "Override skill visibility". Every class the
skill reports is a visibility state, which makes the name both accurate and
self-explanatory: can the model see this skill, and if not, why.

Starvation survives as the name of the specific field measuring the
description-budget contest, which is what it accurately describes.

Renamed via git mv so history follows: skill directory, engine, unit tests, and
test wrapper, plus every reference in the manifest, README, CHANGELOG,
observability read-routing, and the topic's design artifacts. Catalog and cheat
sheet regenerated.

Trimmed the description to 1524/1536 chars: the skill about description-budget
overflow had overflowed its own description budget.

All gates green: validate-plugins, leaf-names, manifest-keys, options-docs,
state-key sync, markdownlint, ruff, shellcheck, 52 unit tests, observability
32/32, and check-skill PASS with 0 errors 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT

@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: 720d8c421e

ℹ️ 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/claude-ops/skills/observability/scripts/clean.sh Outdated
Three conflicts, all additive collisions rather than competing edits:

- scripts/sync-state-key.sh — main registered context-budget and improvement as
  state-key carriers while this branch registered claude-ops. Resolved as the
  union of all four; sync --check now reports "All 4 plugin copies match".
- plugin.json — main advanced 0.32.6 -> 0.32.9 with no change to the description
  body (verified by normalizing out this branch's inserted clause and comparing
  the remainder byte-for-byte). Kept this branch's eleven-skill description;
  0.33.0 remains the correct minor above main's 0.32.9.
- CHANGELOG.md — both sides appended. Kept both, 0.33.0 above main's 0.32.9,
  0.32.8, and 0.32.7 entries.

Catalog and cheat sheet regenerated after the merge rather than hand-resolved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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


Security review

Ran /review:security-review against this PR's diff (origin/main...HEAD, head ca80587), scoped to security only (no style/quality/test-coverage commentary — that's /review:code-review).

Todo

  • Gather context (PR diff, changed files, custom instructions)
  • Skip-gate check — PR is open, has security-relevant surface (clean.sh file-deletion path, audit_skill_visibility.py file-write path), no prior applicable review on this head
  • Review changed files for security issues
  • Post findings
  • Final summary

Scope reviewed: plugins/claude-ops/skills/observability/scripts/clean.sh (new opt-in skill-usage.jsonl pruning target), plugins/claude-ops/skills/audit-skill-visibility/scripts/audit_skill_visibility.py (new read-only audit engine, including its --write/report_path() persistence path), plugins/claude-ops/lib/state-key.sh (new state-key derivation, shared-source registered), plus the manifest/doc/test changes. Docs-only files were skipped as having no security surface.

Finding posted inline:

  1. IMPORTANTaudit_skill_visibility.py:206 report_path() builds a write path from --state-key using only str.strip('/'), which does not reject internal .. segments, so a crafted --state-key can escape data_root and cause os.makedirs(...) to create directories / write the JSON report outside the intended plugin-data tree. The intended caller, lib/state-key.sh, already defends this exact bug class deliberately, and clean.sh in this same PR adds an equivalent explicit traversal check (with a passing test) for its analogous --skill-usage-dir flag — report_path() has no equivalent guard or test. Not reachable from the currently-documented --fixture invocation today, but the script is user-invocable and should not rely on every future caller pre-sanitizing the value. See the inline comment for a concrete fix suggestion and a code-fix link.

Reviewed and found sound:

  • clean.sh's new --skill-usage-scope/--skill-usage-dir/--keep-skill-usage-days flags: all shell variables are quoted, jq receives untrusted values only via --arg (no injection), the feature is inert without the opt-in flag (verified against the diff), and --skill-usage-dir is explicitly validated against absolute paths, drive letters, and .. before use — with a passing traversal-rejection test.
  • read_churn()'s git log invocation in audit_skill_visibility.py uses subprocess.run with an argument list (no shell=True), so no command injection via repo_root/rel_path.
  • state-key.sh's remote-URL handling: validates the derived identity against a strict segment regex before using it as directory components, hashing anything that doesn't match (explicitly closes the ../../../etc-as-remote-URL traversal case).
  • No changes to GitHub Actions workflow permissions, secrets handling, or trigger types in this diff.

Not re-reported (already flagged by the automated Codex review and out of this lane's scope — logic/functional bugs, not security): the data-dir scope's path-resolution mismatch in clean.sh, the fixture-only (no live collector) state of audit_skill_visibility.py, the starvation-verdict overflow granularity, the joiner-character undercount, and the single global observation horizon across mixed-retention sources.

Branch: claude/skills-discovery-plugin-z1ij9y

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown

Last security-reviewed head: 5e1d1ed70c7818295cdc66068649da90719e3477. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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


Claude finished the code review

  • Gather context (PR body, changed files, existing Codex review comments)
  • Invoke /review:code-review, scope to files changed in this PR
  • Independently read and verify audit_skill_visibility.py and clean.sh against the writer (claude-ops-paths.sh) and sibling script (check-listing-budget.sh)
  • Post inline comments for verified findings
  • Post summary comment

Summary

Reviewed the diff against origin/main per the /review:code-review lane (correctness + maintainability, scoped out of security). I read audit_skill_visibility.py in full, traced clean.sh's data-dir scope against the actual writer path in claude-ops-paths.sh, and cross-checked the description-budget arithmetic against plugins/skill-quality/scripts/check-listing-budget.sh. Five findings survived that verification and are posted inline:

  1. audit_skill_visibility.py:729 — no collect path exists; every real (non-fixture) invocation exits with an error. The skill is user-invocable and its SKILL.md markets it against real "why is my fleet unused" questions, but there's currently no way to run it against an operator's actual install.
  2. audit_skill_visibility.py:491 — a single global observed_horizon (max of all source starts) is applied to every skill, so a short-retention source (e.g. 7-day OTEL) can permanently veto a long-retention source's (e.g. lifetime native counters) ability to render a verdict for skills OTEL never saw.
  3. audit_skill_visibility.py:343 — any overflow_chars > 0 marks the entire competing set likely-starved, not just the least-used tail that would actually lose descriptions, so high-usage skills near the top of the ranking get mislabeled as starved.
  4. clean.sh:276--skill-usage-scope data-dir resolves under $REPO_ROOT (same as repo scope), never under the plugin data root the writer (claude_ops::resolve_skill_usage_dir) actually uses, so this scope can never prune the log it claims to target.
  5. audit_skill_visibility.py:303_demand_chars omits the 3-char " - " joiner the actual listing inserts between description and when_to_use (already modeled as JOINER_CHARS=3 in check-listing-budget.sh), undercounting demand and risking a wrong overflowing/listing-fits verdict near the boundary.

Note: these same five issues were also flagged by the Codex review on an earlier commit (720d8c421e); I independently re-derived and verified each against the current HEAD before re-reporting, rather than restating them on trust — all five are still present and confirmed against the code as written now.

One related, lower-confidence observation I did not file inline (no live collect path exists yet to actually exercise it, per finding #1, so it's speculative): the reachability/starvation classifiers key frontmatter with underscored names (disable_model_invocation, skill_override), while the actual YAML frontmatter in this repo's own SKILL.md files uses hyphenated keys (disable-model-invocation) and the product-level setting is skillOverrides. Worth double-checking that whatever collect implementation lands normalizes these consistently, since the fixture-only tests can't catch a mismatch here.

Everything else in the diff (SKILL.md, evals, lib/state-key.sh — a registered byte-identical carrier of the canonical claude-config copy, docs) looked consistent with the PR's stated verification gates; I did not re-run the test suite myself (no execution environment configured for this task), so treat the author's listed gate results as author-claimed and unverified by me.

…visibility

Five review findings, all verified against the code before acting. Four were
real defects.

- Overflow no longer marks the WHOLE fleet starved (P1). Descriptions are
  dropped least-invoked-first only until the listing fits, so the starved set is
  the prefix whose demand covers the overflow. Previously a one-character
  overflow libelled every competing skill — including the most-used ones the
  mechanism protects longest — and the renderer then told the user they were
  running name-only.
- A short-retention source no longer erases a longer one's coverage (P1). The
  per-row "nothing recorded" claim is now backed by the WIDEST horizon, because
  a source that looked across a year and saw nothing supports the claim
  regardless of another source's 7-day retention. The run header keeps the
  narrowest as observed_horizon: they answer different questions. This restores
  the per-source gating design thread T5 specified.
- The listing joiner is counted (P2). The harness inserts a literal " - "
  between description and when_to_use; concatenating undercounted every
  two-field entry by three characters. skill-quality's check-listing-budget.sh
  already models it as JOINER_CHARS=3, and contracts.md required the two be
  reconciled.
- clean.sh's data-dir scope resolves the store the writer actually uses (P2). It
  anchored the relative dir under the repo root while the writer stores at
  <data-root>/skill-usage/<repo-slug>, so the branch pruned a path the writer
  never touches. The data root is still never taken from the environment — it is
  an explicit flag — and the slug is derived by sourcing the hooks' own helper
  so the two cannot drift.
- report_path rejects traversal (security review). state_key becomes directory
  components and makedirs builds whatever it is given, so `../../../tmp/evil`
  escaped the plugin namespace. Segments are now validated against the same
  pattern lib/state-key.sh applies to its own output, with tests for traversal
  and for the unsafe alphabet.

The fifth finding — no live collector, fixture-only — is real and NOT fixed
here; it is a deliberate scope boundary and is now stated in the module
docstring rather than left implicit.

CI gates: unset GIT_DIR/GIT_WORK_TREE/GIT_CONFIG in the test wrapper so the
churn fixtures cannot write their throwaway identity into the caller's config;
annotate the duplicated GNU date probe with portability-ok as its sibling
already is; set the exec bit on the three scripts to match the audit-install-state
convention; and prune the contract slice, which the merge gate requires.

63 unit tests, observability 32/32, ruff, shellcheck, portability, and
validate-plugins all green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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


Security review — ca80587d7650da

  • Gather context (PR diff, changed files, custom instructions)
  • Determine review scope — one new commit since the last reviewed head (ca80587d7650da)
  • Review the new commit's diff for security issues
  • Post findings
  • Final summary

Scope: the delta between the last security-reviewed head (ca80587) and current HEAD (d7650da), i.e. commit fix(claude-ops): address review findings and CI gates on audit-skill-visibility. This commit is a direct response to the prior review round (this lane's own comment plus the Codex P1/P2 items), so the review below is framed against those.

Prior IMPORTANT finding — CONFIRMED FIXED: path traversal in report_path() via --state-key (original finding).

The fix, audit_skill_visibility.py:191,229-233, now validates every /-split segment of state_key against SAFE_KEY_SEGMENT = re.compile(r"[a-z0-9][a-z0-9._-]*") (a fullmatch) before building the path, raising ValueError on anything that doesn't match — the same alphabet lib/state-key.sh already enforces on its own output. Because the pattern requires the first character to be [a-z0-9], a segment of . or .. can never match, closing the traversal regardless of where it appears (leading, trailing, or embedded — a/../b correctly fails since .. is its own segment). I checked this against the concrete bypass attempts an attacker would try (../../../../tmp/evil, github.com/o/../../../etc, a/../b, bare .., /../x, absolute/drive paths, embedded null-adjacent punctuation, uppercase, whitespace, ;, ~) and all are rejected by the regex. test_audit_skill_visibility.py::ReportPathTest.test_refuses_a_traversing_state_key and test_refuses_key_segments_outside_the_safe_alphabet cover exactly these cases. Verdict: closed, well-tested, no bypass found.

Rest of the diff — no new security issues:

  • clean.sh's --skill-usage-data-root addition (the Codex P2 data-dir fix) validates the root is absolute (/* | [A-Za-z]:*) before use and re-derives the store path via claude_ops::repo_slug(), which is already sanitized to [A-Za-z0-9._-] plus a hash — no injection or traversal surface. (Note, out of this lane's scope since it's a correctness/reliability issue and not exploitable: this branch references $SKILL_DIR at clean.sh:325 before it's assigned at line 342; under this script's set -uo pipefail that's an unbound-variable abort, not an unintended write — it fails closed with no prune executed, so no data-safety impact — but the intended data-dir happy path isn't exercised by any test in this commit's claude-observability.test.sh additions, so as written it can't actually reach prune_file.)
  • audit_skill_visibility.test.sh's new unset GIT_DIR GIT_WORK_TREE GIT_CONFIG (addressing test-fixture isolation: git -C does not survive an exported GIT_DIR #2840) is itself a safety fix: it stops an inherited GIT_DIR from redirecting a churn-fixture's git config write into the caller's shared .git/config. Sound.
  • The joiner_chars / starvation-band / horizon-selection changes are pure data-model corrections (Codex P1 items) with no file I/O, subprocess, or external-input handling — no security surface.
  • No changes to GitHub Actions workflows, permissions, secrets, or trigger types in this commit.

Net result: the one finding from the prior review round is fixed and tested; nothing new in this delta introduces a vulnerability. No blocking security findings on d7650da.

Branch: claude/skills-discovery-plugin-z1ij9y

Comment thread plugins/claude-ops/skills/observability/scripts/clean.sh
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown

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

…lity

Two independent reviewers blocked on the same thing: the skill was registered
and user-invocable but could only read hand-authored fixtures, so an operator
had no path to a report about their own installation. They were right — that is
a stub, not a tool. This adds the collection path the design specified and the
plan deferred twice.

Live is now the default; --fixture replays a prepared bundle for tests and
reproductions. The fleet and its frontmatter come from walking a plugins root,
the native counters from ~/.claude.json, and the plugin's own store from a
supplied path. Each source is optional: a missing one narrows the reported tier
rather than failing the run, which is the same honesty the verdicts apply.

parse_frontmatter normalizes the hyphenated `disable-model-invocation` key to
the underscored form the classifier uses — the code review flagged this
mismatch as speculative because no live path existed to exercise it, and it
would have reported every user-only skill as model-reachable. It reads five
scalar keys rather than pulling in a YAML dependency, and reports _malformed
instead of guessing, because misconfigured is a fix-me and a wrong-but-confident
parse is worse than an honest "cannot tell".

Enablement stays None -> unknown: the filesystem cannot answer it, and guessing
would libel a disabled plugin's skills.

Also fixes a defect the security review caught in the previous commit: clean.sh
referenced SKILL_DIR before assigning it, so under set -u the data-dir branch
aborted before reaching prune_file. The assignment moved above both consumers,
the empty-base guard no longer fires for a scope that builds its path from the
data root, and a happy-path test now proves the branch resolves to the writer's
real layout — <data-root>/skill-usage/<repo-slug> — which no test covered before.

The renderer no longer implies the whole competing set is running name-only: it
reports how many are starved and how many keep their descriptions. Against this
repo the live run reports 221 skills, 163 competing, 154 starved, 9 retained,
106,419 characters over an 8,000-character budget.

71 unit tests, observability 33/33, and every repo gate green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D7jUPqbSEeZeEwF1wkt9WT
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Review round addressed — all findings fixed

Codex and the code-review lane independently reported the same five findings, and the security lane added a sixth. I verified each against the code before acting; all six were real. Two rounds of fixes are pushed (d7650da, 5e1d1ed7).

The blocking one: no live collection path

Both lanes were right that a registered, user-invocable skill that can only read hand-authored fixtures is a stub rather than a tool. Live collection is now the default; --fixture is reserved for tests and reproductions.

The fleet and its frontmatter come from walking a plugins root, native counters from ~/.claude.json, and the plugin's own store from a supplied path. Each source is optional — a missing one narrows the reported tier rather than failing the run, which is the same honesty the verdicts themselves apply.

The code-review's speculative note about the frontmatter key mismatch was correct and is now load-bearing: parse_frontmatter normalizes hyphenated disable-model-invocation to the underscored form the classifier uses. Without it, every user-only skill would have reported as model-reachable. Enablement stays Noneunknown, because the filesystem cannot answer it and guessing would libel a disabled plugin's skills.

Live run against this repo: 221 skills, 163 competing, 106,419 characters over an 8,000-character budget.

The other four

Finding Fix
Overflow marked the whole competing set starved Only the least-used prefix whose demand covers the overflow is likely-starved; the rest are likely-retained. On this repo: 154 starved, 9 retained — previously all 163 were labelled.
Global horizon let a short source veto a long one Per-row "nothing recorded" claims are backed by the widest horizon; the run header keeps the narrowest as observed_horizon. They answer different questions. This restores what design thread T5 specified.
Missing - joiner joiner_chars: int = 3, added only when both fields are present, mirroring check-listing-budget.sh's JOINER_CHARS=3.
data-dir never reached the writer's store Resolves <data-root>/skill-usage/<repo-slug> by sourcing the hooks' own repo_slug so the two cannot drift. The root is an explicit flag — CLAUDE_PLUGIN_DATA is still never trusted here.

Security finding, and a bug it exposed

report_path() now validates every key segment against the same alphabet lib/state-key.sh enforces on its own output, with tests for traversal and for the unsafe alphabet.

The follow-up security review also caught a defect I introduced in the first fix round: clean.sh referenced SKILL_DIR before assigning it, so under set -u the data-dir branch aborted before reaching prune_file. Correct on both counts — it failed closed, and no test covered the happy path. The assignment moved above both consumers, the empty-base guard no longer fires for a scope that builds its path from the data root, and a happy-path test now proves the branch resolves to the writer's real layout.

Verification

71 unit tests, observability 33/33 (was 32/32 — the new data-dir happy path), plus ruff, shellcheck, shell-portability, fixture-git-isolation, and validate-plugins all green. The contract slice is pruned per the merge gate; the design record lives in this PR body and in the branch history.


Generated by Claude Code

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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


Security review — d7650da5e1d1ed

  • Gather context (PR diff, changed files, custom instructions)
  • Invoke /review:security-review — skip-gate check: PR open, new commit since last reviewed head has security-relevant surface (new live file-collection paths reading ~/.claude.json, a plugins root, and an operator-supplied skill-usage store)
  • Review the new commit's diff for security issues
  • Post findings
  • Final summary

Scope: the delta between the last security-reviewed head (d7650da) and current HEAD (5e1d1ed), commit feat(claude-ops): add the live collection path, fix data-dir reachability. This is the direct response to the prior code-review round's P1 finding #1 (no collect path existed) and P2 finding #4 (clean.sh --skill-usage-scope data-dir never reached the writer's actual store).

No security issues found in this delta.

Reviewed in detail:

  • collect_fleet() walks --plugins-root (default ./plugins) with os.listdir/os.path.isdir/os.path.isfile and opens each SKILL.md read-only. No shell execution, no symlink-sensitive write, and the root is either the CLI operator's own argument or their own cwd — reading files the invoking user already has access to, not a trust-boundary crossing.
  • parse_frontmatter() is a hand-rolled scalar-only reader (no yaml.load, no eval/exec), so a malicious SKILL.md can at worst be reported as _malformed — it cannot execute code or escape the read.
  • collect_native() and collect_jsonl() are plain open()/json.load reads gated by OSError/ValueError, no path validation needed since both paths are supplied by the invoking operator (--claude-json, --skill-usage) rather than derived from an untrusted source.
  • The new --context-window / SLASH_COMMAND_TOOL_CHAR_BUDGET env read is guarded by .isdigit() before int() — no injection surface, and it only feeds arithmetic.
  • Confirmed the previously-fixed report_path() traversal guard (SAFE_KEY_SEGMENT fullmatch, line 303) is untouched by this commit and still intact.
  • clean.sh's data-dir scope fix (lines 269–303) now requires an explicit --skill-usage-data-root, validated absolute (/* | [A-Za-z]:*) before use, and re-derives the writer's exact layout via the shared claude_ops::repo_slug — this closes the P2 gap without reintroducing the CLAUDE_PLUGIN_DATA-from-environment risk this same PR already guarded against. Backed by a new happy-path test (9s-d2) proving the resolved path lands under the supplied data root.
  • Test-only changes (test_audit_skill_visibility.py's new FrontmatterParseTest/CollectFleetTest, claude-observability.test.sh's 9s-d2) have no security surface.

No changes to GitHub Actions workflow permissions, secrets handling, or trigger types in this commit.

Branch: claude/skills-discovery-plugin-z1ij9y

@kyle-sexton
kyle-sexton enabled auto-merge (squash) August 19, 2026 17:46
@kyle-sexton
kyle-sexton merged commit b4fb6fb into main Aug 19, 2026
48 checks passed
@kyle-sexton
kyle-sexton deleted the claude/skills-discovery-plugin-z1ij9y branch August 19, 2026 17:48
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 8m 9s —— View job


Todo

  • Gather context (PR body, prior review rounds, changed files)
  • Invoke /review:code-review and follow its criteria
  • Review the delta since the last reviewed head (d7650da5e1d1ed7)
  • Post inline findings via mcp__github_inline_comment__create_inline_comment
  • Post summary comment

Code review — d7650da5e1d1ed7

Scope: per this lane's rule, security is out (already covered by /review:security-review). I diffed the two commits directly (git diff d7650da 5e1d1ed7) rather than against origin/main, since this is a fresh checkout without full history to that branch point; the four files that actually changed are audit_skill_visibility.py, test_audit_skill_visibility.py, claude-observability.test.sh, and clean.sh.

This commit's own stated purpose — add the live collect path two earlier review rounds blocked on — landed as claimed: collect_fleet/collect_native/collect_jsonl are wired into main(), live is now the default, and --fixture is reserved for tests. I also re-verified the five previously-flagged Codex/Claude findings (missing collect path, global horizon, binary starvation verdict, joiner-char undercount, clean.sh data-dir scope) — all five were already fixed in the prior commit (d7650da) and remain fixed here; this commit only adds starved_count/renderer wording on top and doesn't touch that logic. The clean.sh SKILL_DIR-before-use fix (the unbound-variable defect the security lane caught) is correct and now covered by a happy-path test (9s-d2) proving data-dir resolves to the writer's actual <data-root>/skill-usage/<repo-slug> layout.

Two new findings from this commit's own changes, posted inline:

  1. audit_skill_visibility.py:268collect_fleet() always sets plugin_enabled: None (correct — enablement isn't knowable from disk), but reachability() checks enabled is None (line 583, → unknown) before checking disable_model_invocation (line 602, → user-only). So every live-collected skill's reachability now resolves to unknown, and this very commit's parse_frontmatter hyphen-normalization fix — meant to stop disable-model-invocation skills from misreporting as model-reachable — never gets exercised in live mode. No test covers the plugin_enabled=None + disable_model_invocation=True combination end-to-end.
  2. audit_skill_visibility.py:257-260 — the SKILL.md read in collect_fleet() only catches OSError; a non-UTF-8 file raises UnicodeDecodeError (a ValueError, not caught), crashing the whole live audit run instead of degrading just that one skill to _malformed as the module's own resilience design intends (and as parse_jsonl/collect_native already do).

Test coverage: the new FrontmatterParseTest/CollectFleetTest unit tests are solid for the functions they target in isolation, but nothing exercises main()'s new live-collection wiring end-to-end (no CLI-level test with --plugins-root instead of --fixture) — which is exactly the seam where finding #1 above was hiding.

Branch: claude/skills-discovery-plugin-z1ij9y

"source": "plugin",
# Enablement is not knowable from the filesystem alone, and
# guessing it would libel a disabled plugin's skills.
"plugin_enabled": None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

reachability()'s enablement check masks this commit's own frontmatter fix for every live-collected skill.

collect_fleet() always sets plugin_enabled: None here (line 268) — intentional, since enablement genuinely can't be read from the filesystem. But reachability() (lines 545-617) checks if enabled is None: at line 583 (→ value: "unknown") before it ever reaches frontmatter.get("disable_model_invocation") at line 602 (→ value: "user-only").

Since every live-collected entry has plugin_enabled=None, every one of them (that isn't caught earlier by malformed/no-description) resolves to reachability.value == "unknown" — the disable_model_invocation branch this commit's parse_frontmatter hyphen-normalization was specifically written to feed correctly is unreachable in live mode. The commit message states the old bug "would have reported every user-only skill as model-reachable"; as wired, it now reports every skill (user-only or fully reachable) as unknown instead, which is a different but equally uninformative failure for the tool's headline question. The starvation/listing classifier (_eligibility, line ~435) isn't affected — it reads disable_model_invocation directly and correctly.

None of the new tests catch this: CollectFleetTest checks collect_fleet() in isolation, and the existing reachability() tests (e.g. test_disable_model_invocation_is_user_only_not_unused) all construct rows with plugin_enabled=True, never the None shape the live collector actually produces.

Suggest reordering reachability() so the disable_model_invocation (and skill_override) checks run before the enabled is None check — disable-model-invocation is independently determinable from frontmatter regardless of plugin enablement — and adding a test that pushes a live-shaped entry (plugin_enabled=None, disable_model_invocation=True) through reachability()/classify() end-to-end.

Fix this →

Comment on lines +257 to +260
try:
with open(path, encoding="utf-8") as handle:
frontmatter = parse_frontmatter(handle.read())
except OSError:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A non-UTF-8 SKILL.md crashes the whole live audit instead of degrading just that skill.

try:
    with open(path, encoding="utf-8") as handle:
        frontmatter = parse_frontmatter(handle.read())
except OSError:
    frontmatter = {"_malformed": True}

handle.read() under encoding="utf-8" raises UnicodeDecodeError — a ValueError subclass, not an OSError subclass — when a SKILL.md contains bytes invalid for UTF-8 (e.g. a stray Latin-1 smart-quote, which is a realistic real-world occurrence in a large third-party plugin fleet). That's not caught here, so it propagates out of collect_fleet() and aborts main()'s live path entirely for every skill in the run, rather than reporting just the offending skill as misconfigured/_malformed.

This directly contradicts the module's own stated design goal a few lines up ("a wrong-but-confident parse is worse... misconfigured is a fix-me, not a delete-me") and the resilience pattern the sibling parsers already follow (parse_jsonl skips one bad row rather than failing the whole store; collect_native catches (OSError, ValueError) around json.load).

Suggest except (OSError, UnicodeDecodeError): (or (OSError, ValueError) to match collect_native's pattern).

Fix this →

kyle-sexton added a commit that referenced this pull request Aug 19, 2026
No linked issue

## Summary

A plugin's prose counts its own skills by hand — "bundling fourteen
skills", "the other thirteen skills are zero-config" — and nothing
recomputed those numbers when a skill landed. Adding one silently
falsified every sentence that had counted the old set, in files nobody
opens during that change.

The measured record: PR #3011 went stale on six such counts, each caught
by a different human or automated reviewer rather than by anything
mechanical, and `session-flow`'s own CHANGELOG records an "other eleven
skills" line that was **already off by one before that PR touched it**.
A reviewer catch is not a control.

This adds the gate, and fixes the four live drifts it found on a clean
`main`.

## Fix

**New gate** — `scripts/check-skill-count-claims.sh` + `.test.sh` +
`skill-count-claim-exemptions.txt`, wired into `ci.yml` as
`skill-count-claim-gate` (self-test first, then `--check`) and added to
`ci-status.needs`. It matches a closed set of composition-claim grammars
across each plugin's README, manifest, and skill bodies, and compares
the claimed number — spelled or numeric — against the skills the plugin
actually has.

The spelled form is why nothing found this before: the claims read
"fourteen skills", not "14 skills", so no numeric grep ever hit them.

**Four live drifts found and fixed:**

| Plugin | Claimed | Actual |
|---|---|---|
| `testing` | "Four skills" | **five** — and the README table omitted
`/testing:audit` entirely |
| `verification` | "Two skills" | three |
| `improvement` | "One skill" | two, in its own first release |
| `docs-hygiene` | "the other five skills do not use it" | eight-skill
plugin, **and** `extract-ssot` does use it |

The `testing` case is the payoff: a count drift marked a wholly
undocumented skill, so a reader on that front page had no way to learn
`audit` exists. Both halves are fixed — the count and the missing table
row.

For `docs-hygiene` the sentence was substantively false too, so it now
names its subjects instead of counting them. That is the treatment this
plugin's own `audit-noise` `enum-list` shape prescribes for a hardcoded
consumer count; the two agree on the diagnosis and differ only in
enforcement.

**Scope is stated in the script header, not left implicit:**

- The recognized grammars are a **closed set**, because most sentences
pairing a number with the word "skills" assert nothing about plugin size
("gate one skill", "nine skills moved into three new plugins"). A gate
that fires on those trains people to route around it.
- One grammar was **evaluated and rejected**: a definite-article
reference ("the two skills…") reads as a claim about a named pair as
often as about the whole set — `discipline`, which has 17 skills, says
exactly that.
- **`CHANGELOG.md` is out of scope.** A dated entry describes the
release it shipped with; rewriting it to match today's tree would
falsify history.
- **Document-internal counts** ("the four buckets" in a document
defining five) are a real second class this gate does not hold, and the
header says so rather than letting the coverage read as complete.

**Portability annotations.** `check-shell-portability.sh --all` was
permanently red on 13 hits across three files, which made it unusable as
an audit tool: a mode that always fails teaches you to ignore it, so a
genuine 14th hit would land in the noise. Every one is a false positive
in one of two classes — PCRE tokens as test *data* in
`resolve-convention-pattern.test.sh` (the shorthand is the thing under
test; the assertions prove the resolver rejects it), and Windows path
fixtures (`C:\temp\skills`, `\\server\share\skills`,
`D:\work\acme\keepme`). In each case the construct cannot be spelled
portably because it *is* the input under test, so a per-site
`portability-ok:` with a stated reason is the correct disposition. The
whole-file `portability-scope:` escape was deliberately not used — the
gate reserves it for its own fixture corpus, and these are neighbouring
gates' tests.

## Review findings addressed

**Three Codex P2s, all verified real against the tree before acting**
(`b8df7ec2`):

1. **A plugin whose last skill is removed loses its `skills/`
directory**, and the scan skipped any plugin without one — the
stale-count-by-removal case the gate most needs, reported as success.
Every plugin is now scanned; missing or empty `skills/` is a count of
zero.
2. **Matching worked per physical line**, so a markdown wrap between the
verb and the count hid a claim, and reflowing a paragraph could
permanently disable the gate for a claim it used to hold. Each line is
now tested alone and joined with its successor, stopping at block
boundaries. This found a live wrapped claim in
`plugins/naming/README.md` (correct) and one in
`claude-config/skills/setup/SKILL.md` (a prerequisite-scope subset
count, now exempted with its reason).
3. **The prefilter tested `*[Ss]kill*` on raw text**, which rejects an
all-caps `SKILLS`, dropping a heading before the case fold could reach
it. The fold now runs first.

Fixing 2 surfaced two defects the review did not name: joining
**double-counted** every wrapped-adjacent claim (46 sites reported where
24 exist — the guard must re-test the pattern against the next line, not
check containment of the matched extent, because most forms open with a
boundary group that captures the join's own space); and one sentence can
satisfy **two grammars at once**, so the guard re-tests every form
rather than only the one that matched.

**One Claude review finding** (`3369fbc6`): the `--check` FAIL message
reconstructed the plugin's real count as `expected + 1`, which is wrong
exactly when the zero-clamp fires — it reported "has one" directly above
"should be zero". The count is now carried through the report row and
read from it. Enforcement was never affected; the diagnostic was, in
precisely the last-skill-removed case the zero-skill scan was built for.

## Verification

**22 black-box assertions**, with the *negative* ones load-bearing —
ordinary prose like "gate one skill" and "nine skills moved into three
new plugins" must not flag.

**Mutation-tested.** Reverting any of these turns the suite red:
mismatch-reported-as-ok, `--check`-never-fails,
drop-the-minus-one-basis, drop-the-stale-exemption-guard,
scan-CHANGELOG, exemption-never-hits, drop-the-case-fold,
skip-plugins-without-a-skills-dir, no-line-joining,
narrow-the-dedup-guard-to-one-form, and restore-the-`expected +
1`-reconstruction.

**Gates run locally, all green on the merged tree:**

- `scripts/check-skill-count-claims.sh --check` — 24 claims match the
tree, 2 exempted
- `bash scripts/check-skill-count-claims.test.sh` — all 22 assertions
passed
- `shellcheck --rcfile=.shellcheckrc` and `shfmt -d` on both new scripts
and the three annotated files
- `actionlint` on `ci.yml`
- `scripts/check-shell-portability.sh --all` — `No unexcused GNU-only
constructs in 1325 shell file(s)` (was `exit 1` with 13 hits)
- `scripts/check-changelog-parity.sh` in all four modes — 212 headings
preserved across 6 changed changelogs
- `scripts/check-lane-coverage.sh --check` — all 39 lanes reachable from
`ci-status.needs`
- `scripts/validate-plugins.sh`, catalog sync, cheat-sheet generator
tests, sync-manifest check
- `scripts/affected-tests.sh --run` — all 11 selected suites passed, no
unmapped files

**The gate proved itself on the merge.** `main` landed a `claude-ops`
eleventh skill while this branch was open. Resolving that conflict took
main's manifest wholesale (so the new skill's description survived
rather than being clobbered by a stale ten-skill string), and the gate
then verified the new claim: `plugins/claude-ops/README.md:4` and its
manifest both read "eleven skills" against eleven skill directories.

**Version bumps + CHANGELOG entries:** `testing` 0.6.2, `verification`
0.5.3, `improvement` 0.1.1, `docs-hygiene` 0.17.2, `claude-ops` 0.33.1,
`repo-hygiene` 0.10.7.

*(This section previously listed `claude-ops 0.32.10` and `repo-hygiene
0.10.6` — stale after the merge with `main` renumbered both. A
hand-written count in prose going stale is the exact defect this PR
gates against, and the PR body is not a surface the gate covers. Caught
by review.)*

## Related

- Refs #3011 — the PR whose six stale counts are this gate's motivating
evidence
- Refs #3035 — landed the `claude-ops` eleventh skill mid-review; its
count is what the gate verified on the merge
-
`docs/adr/0016-source-skill-recommendation-from-the-catalog-not-the-listing.md`
— the decision record from #3011
- `plugins/docs-hygiene/skills/audit-noise/SKILL.md` — the `enum-list`
noise shape, this gate's advisory counterpart
- `scripts/check-skill-leaf-names.sh` — the existing gate whose
script/self-test/CI-lane shape this one follows

---------

Co-authored-by: Claude <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 19, 2026
…lly ships (0.33.2) (#3044)

No linked issue

## Summary

Three cosmetic-looking doc strings in `audit-skill-visibility` were left
behind by the two late changes in #3035 — the `audit-skill-starvation` →
`audit-skill-visibility` rename, and the final commit that added the
live collection path. One of them is not cosmetic in effect: the docs
told an operator to hand-author a JSON fixture instead of running the
engine live, which is the path that actually answers the question the
skill exists for.

## Fix

- **`SKILL.md` *Run it*** — the live invocation is now the primary form,
and the section states **where each live input comes from**, because the
two resolve differently and the difference is load-bearing:
- *Usage* reads this machine's `~/.claude.json` wherever you run it
(`--claude-json` overrides).
- *The fleet being audited* — the denominator — defaults to `./plugins`
**relative to the current directory**, not to an installed root. From a
plugins-layout checkout that audits the tree you're standing in;
anywhere else the bare command exits non-zero with `no skills found`,
and `--plugins-root DIR` names the fleet. Nothing is silently audited
either way.
- **`argument-hint`** — no longer claims "fixture-driven at this phase";
now documents `--plugins-root` and states that the fleet defaults to
`./plugins`.
- **`SKILL.md` field table** — the `starvation` row's Phase column reads
**live**, matching `observation` and `reachability`. It said "Phase 3"
for a field that has computed live since the skill shipped.
- **`audit_skill_visibility.py:781`** — the Markdown report's H1 reads
`# Skill visibility report`. No test asserted the old string.

No behavior change: the engine, its collection paths, and its verdicts
are untouched. The `description` frontmatter was deliberately left alone
at 1524/1536 chars.

Version bumped `0.33.1` → `0.33.2` with the matching `CHANGELOG.md`
entry.

### Correction during review

The first revision of this PR claimed the bare invocation "collects from
this machine's real install". **That was wrong**, and Codex's P1 caught
it. I had verified it from this marketplace checkout, which happens to
contain `plugins/` — so the run measured the repo's own source tree and
I read the result as the installed fleet.
`audit_skill_visibility.py:945` is:

```python
plugins_root = args.plugins_root or os.path.join(os.getcwd(), "plugins")
```

Fixed in 2c272a0; the docs, the changelog entry, and this description
now describe the actual split.

### Deliberately not done

Making live fleet discovery independent of the working directory. That's
a behavior change in a PR that claims none, and picking "the installed
root" is a design decision rather than a lookup — `~/.claude/plugins/`
holds `cache/`, `data/`, `marketplaces/`, `synced/` and
`installed_plugins.json`, with `marketplaces/` empty in this container,
so there's no verifiable single root to default to here. Worth doing as
its own change, with that decision made deliberately.

## Verification

Run locally on `2c272a05`:

| Check | Result |
|---|---|
| `test_audit_skill_visibility.py` | 71 passed |
| `audit_skill_visibility.test.sh` | ok — fresh-install fixture withheld
every cold verdict (4/4) |
| `check-skill.sh --require-evals audit-skill-visibility` | **PASS — 0
errors, 0 warnings** (description 1524/1536) |
| `check-changelog-parity.sh --check` / `--check-order` / `--check-bump
origin/main` | all clean |

Both documented invocations were executed, not asserted. From an empty
directory (`$PWD` below is that empty directory, `MARKETPLACE` is this
checkout):

```console
$ python3 audit_skill_visibility.py --render markdown
error: no skills found under '$PWD/plugins'; pass --plugins-root <path> to point at a plugins directory
$ echo $?
2

$ python3 audit_skill_visibility.py --plugins-root MARKETPLACE/plugins --render markdown
# Skill visibility report

- Observed horizon: `2026-08-15T05:32:07.955000+00:00`
- Sources: native
- Capability tier: `T-baseline` (sources present: native; claims supported: lifetime_count)
- Skills: 221
- Withheld claims: 213
```

## Related

Refs #3035 — the merged PR that shipped the skill and left this drift
behind.

---------

Co-authored-by: Claude <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 20, 2026
…per scope (0.34.0) (#3059)

No linked issue

## Summary

`audit-skill-visibility` could only measure a plugins **directory**,
defaulting to `./plugins` relative to the working directory. That
answers *"what is in this checkout"*, not *"what is installed"* — the
question the skill exists for. #3044 documented that limitation
honestly; this removes it.

`--installed` reads `~/.claude/plugins/installed_plugins.json` instead.
Both answers are now available and they legitimately differ: measured
here the repo holds **221** skills and the installed fleet **216**,
because three plugins present in the checkout (`ai-slop`,
`context-budget`, `improvement` — 2+1+2 skills) were never installed.
Neither number is wrong; they answer different questions.

## Fix

**The manifest lists one entry per install SCOPE, not per plugin.** Here
67 plugins carried **134** entries. Since the fleet is the denominator
the listing budget is measured against, counting entries would have
roughly **doubled the reported overflow** and fabricated the headline
number. `resolve_installed()` is pure (manifest + marketplace registry
in, resolution out; no filesystem), keys by plugin identity, and reports
both counts so the 134→67 collapse is auditable rather than trusted.

**Multi-scope installs resolve by the documented precedence `local >
project > user`** — the record that loads is the highest-precedence
*applicable* one, never the newest version installed. Superseded records
are listed under a new **Fleet resolution** section so a pin being
outranked stays visible. Getting this wrong is not cosmetic: 7 plugins
here ship different skill *sets* between scopes and 19 skills different
`description` text.

**Records that cannot load here are excluded and reported.** `project`
and `local` installs load only in the `projectPath` they name; another
project's records would inflate the fleet with skills the model can
never see. Current project comes from `CLAUDE_PROJECT_DIR`, falling back
to cwd — matching `fleet-state.sh`.

**A directory-source marketplace loads its checkout**, not either cached
`installPath` — verified by a skill executing out of the marketplace
directory. The plugin root for those comes from the catalog's declared
`source`, because `plugins/<name>` is the common layout but not a rule.
Since neither cached version is running there, those report
`marketplace-directory` and emit no superseded pair rather than naming
two versions that are both beside the point.

**Supporting change:** `collect_fleet` delegates to a new per-plugin
`collect_fleet_at`, because installs are scattered across versioned
cache paths with no single parent to walk.

## Corrections during review

Five review findings, all real. Two are corrections to claims I made,
and I'd rather they sit in the description than only in the thread:

**1. I said scope precedence was "undocumented and not verifiable here."
It is documented — in the plugin I was editing.**
`plugins/claude-ops/skills/plugins/context/scope-semantics.md` states
`local > project > user` and warns against my exact heuristic by name:
*"the record that actually loads is the one at the highest-precedence
scope present, never simply 'the newest version installed.'"* I searched
the compiled CLI, found only string constants, and concluded the answer
was unknowable — without grepping the repo I was working in. My first
revision shipped a `lastUpdated` sort, which is precisely the failure
mode that document exists to prevent. Caught by Codex (P1); fixed in
`0b87680a`, pinned by `test_scope_precedence_beats_the_newest_version`
(an *older* project pin must beat a *newer* user install).

**2. In #3044 I deferred this work claiming "no verifiable single root
to default to."** Also a wrong premise — `installed_plugins.json` is
authoritative and always was. I was looking for a single root directory
rather than a manifest.

Also fixed in `0b87680a`: `projectPath` filtering (Codex P1),
catalog-declared source paths instead of a hardcoded layout (Codex P2),
two dead fields that nothing downstream read (review lane), and the
missing test for `collect_installed` — the function `--installed`
actually wires to (review lane).

## Verification

All run locally on `0b87680a`:

| Check | Result |
|---|---|
| `test_audit_skill_visibility.py` | **84 passed** (76 → 84) |
| `audit_skill_visibility.test.sh` | fresh-install fixture 4/4 withheld;
`--installed` collapsed 2 records → 1 plugin |
| `check-skill.sh --require-evals` | **PASS — 0 errors, 0 warnings**
(description 1524/1536; SKILL.md trimmed to 200 lines to clear the soft
target it had crossed) |
| `ruff check` / `ruff format --check` | clean |
| `shellcheck` | clean |
| `check-changelog-parity.sh --check-bump` | clean |
| `scripts/validate-plugins.sh` | Validation passed |

Live, both modes executed rather than asserted:

```console
$ audit_skill_visibility.py --installed --render json
entries 134  plugins 67  superseded 0  not_applicable 0  skills 216

$ audit_skill_visibility.py --plugins-root plugins --render json
skills 221, no "fleet" key — prior behavior unchanged
```

Branches the live install cannot reach are covered by tests instead:
precedence (`local > project > user`), another project's install
excluded end to end, catalog-declared `.` source path, and an unreadable
config directory.

## Related

Refs #3044 — documented the cwd-relative limitation this removes, and
whose "Deliberately not done" section this closes out. Refs #3035 —
shipped the skill.

---------

Co-authored-by: Claude <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.

2 participants