yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

skill-check does not scan README.md or CONTRIBUTING.md #220

New Issue
Open
opened 2026-08-26 07:36:59 +00:00 by logan · 2 comments
logan commented 2026-08-26 07:36:59 +00:00
Collaborator
Copy Link
Copy Source

Report

scripts/skill-check.sh asserts that every make <target> named in an
agent-facing doc exists. Its scanned set is .pi/**/*.md plus
CLAUDE.md. It does not scan README.md, and as of #50 it does not
scan CONTRIBUTING.md either — which is now the document a human
contributor reads a build command out of.

CONTRIBUTING.md names 21 make targets (setup, dev, build-dev,
build-prod, android, generate, bindings, bindings-check,
lint, test, ui-test, e2e, css-check, dev-headless,
sandbox-seed, dev-logs, dev-stop, testdata, vulncheck,
commit-check, release-dry). All 21 exist today; nothing would say
so if one were renamed.

The script's own header states the reason it exists — "a renamed or
deleted target turns into a failing commit rather than into an agent
confidently running a command that no longer exists". The same
sentence is true of a person, and the contributor-facing doc is
precisely where somebody who has not read the Makefile goes looking.

A second, smaller half. The lefthook pre-commit glob for this
check is {Makefile,.pi/**/*.md} while the script scans .pi/ and
CLAUDE.md, so a commit touching only CLAUDE.md does not fire the
hook at all. CI's make skill-check step catches it, so this is a
slower feedback loop rather than a hole — but the glob and the script
disagreeing about their own scanned set is the kind of drift the check
exists to prevent.

Direction

Add README.md and CONTRIBUTING.md to the find/echo list in
scripts/skill-check.sh, and widen the lefthook glob to match whatever
the script ends up scanning. The fence rule the script already applies
matters more in these two files than in .pi/: both are mostly
hard-wrapped prose, and the backtick-anywhere half of the rule is what
will actually match, since the commands in them are in fenced blocks or
inline code.

Worth checking while there: a bare make mention that is deliberately
not a target (there is none today) would need the same treatment the
.pi/ docs get.

Found while doing #50, which created CONTRIBUTING.md. Filed
rather than fixed there, because changing what a CI gate scans is a
separate change from moving prose between two files.

**Report** `scripts/skill-check.sh` asserts that every `make <target>` named in an agent-facing doc exists. Its scanned set is `.pi/**/*.md` plus `CLAUDE.md`. It does not scan `README.md`, and as of #50 it does not scan `CONTRIBUTING.md` either — which is now the document a *human* contributor reads a build command out of. `CONTRIBUTING.md` names 21 make targets (`setup`, `dev`, `build-dev`, `build-prod`, `android`, `generate`, `bindings`, `bindings-check`, `lint`, `test`, `ui-test`, `e2e`, `css-check`, `dev-headless`, `sandbox-seed`, `dev-logs`, `dev-stop`, `testdata`, `vulncheck`, `commit-check`, `release-dry`). All 21 exist today; nothing would say so if one were renamed. The script's own header states the reason it exists — "a renamed or deleted target turns into a failing commit rather than into an agent confidently running a command that no longer exists". The same sentence is true of a person, and the contributor-facing doc is precisely where somebody who has *not* read the Makefile goes looking. **A second, smaller half.** The lefthook `pre-commit` glob for this check is `{Makefile,.pi/**/*.md}` while the script scans `.pi/` **and** `CLAUDE.md`, so a commit touching only `CLAUDE.md` does not fire the hook at all. CI's `make skill-check` step catches it, so this is a slower feedback loop rather than a hole — but the glob and the script disagreeing about their own scanned set is the kind of drift the check exists to prevent. **Direction** Add `README.md` and `CONTRIBUTING.md` to the `find`/`echo` list in `scripts/skill-check.sh`, and widen the lefthook glob to match whatever the script ends up scanning. The fence rule the script already applies matters more in these two files than in `.pi/`: both are mostly hard-wrapped prose, and the backtick-anywhere half of the rule is what will actually match, since the commands in them are in fenced blocks or inline code. Worth checking while there: a bare `make` mention that is deliberately not a target (there is none today) would need the same treatment the `.pi/` docs get. **Found while doing** #50, which created `CONTRIBUTING.md`. Filed rather than fixed there, because changing what a CI gate scans is a separate change from moving prose between two files.
logan added the Kind/TestingKind/Documentation
Priority
Low
4
 labels 2026-08-26 07:36:59 +00:00
logan self-assigned this 2026-08-28 07:33:25 +00:00
logan added the
Status
In Progress
 label 2026-08-28 07:33:26 +00:00
logan commented 2026-08-28 07:33:26 +00:00
Author
Collaborator
Copy Link
Copy Source

Picking this up on docs/220-skill-check-scope.

Approach, as the issue's Direction states it: add README.md and
CONTRIBUTING.md to the script's scanned set, widen the lefthook
pre-commit glob so it matches whatever the script scans (it is
{Makefile,.pi/**/*.md} today, which misses CLAUDE.md as well), and
extend the failure message's grep so it names the new files when it
reports a missing target.

Verified by planting a bad make target in each newly-scanned file in
turn and checking the script actually fails on it, rather than by
trusting that widening the list was enough.

Picking this up on `docs/220-skill-check-scope`. Approach, as the issue's Direction states it: add `README.md` and `CONTRIBUTING.md` to the script's scanned set, widen the lefthook `pre-commit` glob so it matches whatever the script scans (it is `{Makefile,.pi/**/*.md}` today, which misses `CLAUDE.md` as well), and extend the failure message's `grep` so it names the new files when it reports a missing target. Verified by planting a bad `make` target in each newly-scanned file in turn and checking the script actually fails on it, rather than by trusting that widening the list was enough.
logan commented 2026-08-28 07:56:03 +00:00
Author
Collaborator
Copy Link
Copy Source

PR open: #229 (docs/220-skill-check-scope), commit 26251ba.

README.md and CONTRIBUTING.md are in the scanned set, the file list
is one variable rather than two copies, and the lefthook glob is that
set — which also closes the smaller half here, since it did not fire on
CLAUDE.md either.

Verified by planting a bad target rather than by reading the diff: both
matched forms in each of the four scanned surfaces, each naming the
right file; the same plants pass on the pre-change script; unfenced
prose still does not match; and the hook fires on a staged
CONTRIBUTING.md under the new glob where the old one reported
(skip) no matching staged files. The target count is unchanged at 47,
because the set is a union — coverage moved, the count could not.

CI green (run 18244, re-run): check and e2e both success. The
first attempt failed on #146's yt-dlp flake, which this branch cannot
have caused — it contains no Go — and the diagnosis it produced is on
that issue.

Filed #228 for the one thing found and deliberately not fixed here:
a make … span the file hard-wraps across two lines is matched by
nothing, and CONTRIBUTING.md:80 already holds one. Nothing is broken
today; a target named only inside a wrapped span would be missed.

Not merged.

PR open: #229 (`docs/220-skill-check-scope`), commit `26251ba`. `README.md` and `CONTRIBUTING.md` are in the scanned set, the file list is one variable rather than two copies, and the lefthook glob is that set — which also closes the smaller half here, since it did not fire on `CLAUDE.md` either. Verified by planting a bad target rather than by reading the diff: both matched forms in each of the four scanned surfaces, each naming the right file; the same plants pass on the pre-change script; unfenced prose still does not match; and the hook fires on a staged `CONTRIBUTING.md` under the new glob where the old one reported `(skip) no matching staged files`. The target count is unchanged at 47, because the set is a union — coverage moved, the count could not. **CI green** (run 18244, re-run): `check` and `e2e` both success. The first attempt failed on #146's `yt-dlp` flake, which this branch cannot have caused — it contains no Go — and the diagnosis it produced is on that issue. Filed **#228** for the one thing found and deliberately not fixed here: a `make …` span the file hard-wraps across two lines is matched by nothing, and `CONTRIBUTING.md:80` already holds one. Nothing is broken today; a target named *only* inside a wrapped span would be missed. Not merged.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Kind/Documentation Kind/Testing
Priority
Low
4
Status
In Progress
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees logan
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#220