Stop the skill files drifting unchecked - #396
Merged
Merged
Conversation
jserv
requested review from
Max042004,
alanhc,
henrybear327 and
xalestar
and removed request for
Max042004
September 28, 2026 20:22
xalestar
reviewed
Sep 29, 2026
xalestar
left a comment
xalestar
left a comment
Collaborator
There was a problem hiding this comment.
Checked the added claims against the tree at 1a0b3e4 and they hold: pending_* in signal.h, ATTN_BIT_PTRACE, the release clear in shim_globals_attn_and, both HVC #6 routes, FD_* in linux-wire.h, the usbdev_table_lock entry, the departed-ioctl generator wiring, the print-% guard skip, and the sysctl.h stub. check-atomics (26 self-test cases), check-skill-refs (29) and check-ascii pass on the branch, and no docs/ or top-level .md file carries a literal __ATOMIC_ constant, so scoping the prose scan to .claude/skills/ misses nothing today.
Two points on the description rather than the diff:
- The "Verified with
make check's scanners" paragraph listscheck-stub-shadowandcheck-stub-constantsamong them. Neither is on thecheck:prerequisite line (mk/tests.mk:311); both hang off theverify-*targets (mk/verify.mk:623). The newelfuse-verifytext already says this correctly forcheck-stub-shadow, so the description should match. - "A fresh clone reaches the new content through the skill descriptions alone": the
description:lines (.claude/skills/elfuse-syscall/SKILL.md:3,.claude/skills/elfuse-security/SKILL.md:3) don't mention usbdevfs or USB.elfuse-syscallfires on "touching src/syscall/", so an edit tousbdev.creaches it, but a task phrased as "USB device" or "URB" matches nothing. Either add usbdevfs to those two descriptions, or soften the sentence. The cubic summary's "routing table" also points atAGENTS.md, which this PR leaves out.
jserv
force-pushed
the
refine-skills
branch
from
September 29, 2026 17:48
1a0b3e4 to
d0f5e2d
Compare
xalestar
reviewed
Sep 30, 2026
xalestar
left a comment
xalestar
left a comment
Collaborator
There was a problem hiding this comment.
The four earlier points are addressed. One leftover in the description: it still says check-atomics (26 cases), but the self-test now runs 28, which the be0757c message itself counts (eight prose cases).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jserv
force-pushed
the
refine-skills
branch
2 times, most recently
from
October 1, 2026 22:09
41a2e1a to
5ed054e
Compare
The skills drift in ways check-skill-refs cannot see. It resolves paths, targets and section anchors, so a wrong count, a stale enumeration or a banned identifier passes it untouched. elfuse-guest-abi named the __ATOMIC_* order constants for calls the source writes as atomic_fetch_or_explicit and atomic_fetch_and_explicit. check-atomics rejects that spelling, so an agent following the skill wrote code the build refuses. The same file omitted HVC 13 and the X7 ptrace request, and said the shim_data cache decides whether a syscall takes HVC 5 at all rather than whether a call with an EL1 fast path can skip one. Its list of those paths stopped at identity and urandom; the shim also serves getpgid(0), getsid(0), getrandom and two futex shapes inline, and gettid reads CONTEXTIDR_EL1 rather than a cached slot. usbdevfs had no coverage anywhere: the second largest source file in the tree, two make check gates, a generated ioctl table and four lock-ordering entries, reachable from no skill and no routing row. It now has a bullet in elfuse-syscall and an entry in the elfuse-security boundary list. elfuse-verify pinned six counts inside the bullet that tells the reader to recompute counts, called only a minority of src parsable when most of it parses, listed sys/sysctl.h among the headers no stub can supply beside the stub the tree carries for it, and described check-proof-targets as a CI job when make check runs it too. It also told the reader every scanner runs with no arguments, which for the generator behind check-usbdev-departed writes its header. Two skills restated a source comment in the sentence that cited it, and both copies had already diverged from it. They cite it now. The frama-c MCP material moves to references/frama-c-mcp.md. It was 291 of 738 lines behind a conditional most triggers never take, and a body is a cost paid every time the skill fires.
check-atomics read src only, and its builtin pattern required a call paren, so it caught __atomic_load_n but never a bare order constant. Both gaps met in the skill files, where elfuse-guest-abi named __ATOMIC_SEQ_CST for a call the source writes in C11 and nothing objected. The skills are what a fresh clone carries, so they earn the same gate the source has. Only the banned-spelling half applies to prose: a skill explaining why a bare atomic_load is wrong has to be able to write one, while the builtin vocabulary hands the next reader the wrong spelling for an operation the tree writes in C11. The order constants are gated in prose rather than in C, where every real use sits inside a call the builtin rule already catches. The paren requirement was the same hole from the other side. A bare __atomic_fetch_or named in prose teaches the spelling whether or not a call follows it, so the prose path takes a pattern of its own that does not require one. The star form the rule is stated with stays legal, because the star is not a word character. The skill files are enumerated the way check-ascii.py enumerates its sources, tracked plus untracked and not ignored, so a skill still being drafted is gated while an ignored scratch file is not. A directory walk would have failed the build on the latter. A gate nothing reaches is not a gate. check-atomics ran only under make check, and that lane ignores paths matching **.md and .claude/**, so the pull request this half was written for reached no run of it. It now runs in lint.yml, which carries no path filter. Negative control: reinjecting the original defect fails at its line and removing it passes. Eight prose self-test cases cover both directions, including the three forms prose must keep, and the failure path prints which pattern stopped matching instead of raising.
docs/testing.md and docs/internals.md each introduce the IOKit COM seam substitution with ELFUSE_USB_FIXTURE=loopback alone, and place the build flavor that supplies it 55 to 60 lines below in the same section with no reference tying the two. Read in order, the first sentence says the env var is sufficient, and it is not: a default build links usbdev-fixture-stub.c, whose usbdev_fixture_loopback returns false, so the env var yields a modeled device with no service behind it. The device that answers a transfer needs USB_LOOPBACK_FIXTURE=1 as well, and the loopback lanes set both. Both files now name the flavor in the sentence that introduces the substitution. Measured rather than read: nm finds _usbdev_fixture_lock zero times in build/elfuse and once in build/elfuse-loopback, which is the invariant docs/internals.md pins its dated byte counts to. The two skill descriptions gain the routing vocabulary for this material. Both bodies already covered usbdevfs URBs and raw descriptor blobs, and neither description named USB, so work on it reached neither skill by the words the model matches on.
The reason check-atomics leaves plain-operator access to an _Atomic object unchecked was written out three times: in the script's module docstring, in elfuse-conventions, and in elfuse-security. The three had already drifted apart in wording, and one commit rewrapped the docstring copy without touching the other two. The docstring wins, because it sits at the code that implements the rule, and because elfuse-skills already sets that pattern for check-skill-refs.py. Both skills now state what is unchecked and point there for why. What each keeps is the part only it says: elfuse-conventions the spelling rule it demonstrates in its own paragraph, elfuse-security the judgment that this is the one memory-order case worth an auditor's budget rather than a skip. elfuse-conventions was the only skill of eight ending without an Authoritative sources section. It has one now, in the shape its seven siblings use. Two entries carry facts the body does not: that check-ascii reads only .c, .h and .S under src, tests and frama-c-stubs, so the markdown half of the em dash ban has no gate behind it, and that the installed hooks run check-format.sh and check-commentflow.sh at commit time and the commit-log check at push time, none of the gates listed above among them.
check-skill-refs.py was built to take a routing document on the command line, and the makefile never passed one, so the skill paths in the untracked AGENTS.md routing table went unchecked. The target now passes $(wildcard AGENTS.md), which covers the table where it exists and leaves a fresh clone that has none with the bare invocation it had before. The wildcard is required rather than tidy: the script exits 1 on a file argument that does not exist. The same target ran its self-test locally and not in CI, where the step invoked the checker bare. A regression in the checker's own logic would have passed CI while failing on any developer machine. It now runs its self-test in both places, as check-atomics already does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The skill files under
.claude/skills/had drifted from the tree in ways no gate could see.check-skill-refsresolves paths, make targets and section anchors, so a wrong count, a stale enumeration, or a banned identifier passes it untouched. This corrects what three independent reviews and an audit found, and closes the one defect class that a script can own.The worst of it was
elfuse-guest-abinaming the__ATOMIC_*order constants for calls the source writes asatomic_fetch_or_explicitandatomic_fetch_and_explicit.check-atomicsrejects that spelling, so an agent following the skill wrote code the build refuses, and the two skills disagreed with each other. The same file omitted HVC 13 and the X7 ptrace request entirely, and described the shim_data cache as deciding whether a syscall takes HVC 5 at all, rather than whether a call with an EL1 fast path can skip one. Its list of those paths stopped at identity and urandom; the shim also servesgetpgid(0),getsid(0),getrandomand two futex shapes inline, andgettidreadsCONTEXTIDR_EL1rather than a cached slot.usbdevfs had no coverage anywhere: the second largest source file in the tree, with two
make checkgates, a generated ioctl table and four lock-ordering entries, reachable from no skill.elfuse-verifypinned six counts inside the very bullet that tells the reader to recompute counts, called only a minority ofsrc/parsable when most of it parses, and listedsys/sysctl.hamong the headers no stub can supply, beside the stub the tree carries for it. Two skills restated a source comment in the same sentence that cited it, and both copies had already diverged.The second commit extends
check-atomicsto read the skill files, under the banned-spelling half only. Prose has to be able to quote a bareatomic_loadto explain why it is wrong; what it must not do is hand the next reader the builtin vocabulary for an operation the tree writes in C11. That makes the defect above a build failure rather than a review question.elfuse-verifyalso loses 268 lines to a newreferences/frama-c-mcp.md. The frama-c MCP material was 291 of 738 lines sitting behind a conditional most triggers never take, and a skill body is a cost paid every time the skill fires.The scanners on the
make checkprerequisite line were run individually:check-syscall-coverage,check-eintr-contract,check-lock-order,check-atomics,check-ascii,check-skill-refsandcheck-proof-targetsall pass. The proof-only prerequisitescheck-stub-shadowandcheck-stub-constantsalso pass, as do thecheck-atomics(28 cases),check-skill-refs(29 cases) andcheck-asciiself-tests. Two negative controls: reinjecting__ATOMIC_SEQ_CSTintoelfuse-guest-abifails the new gate at its line and passes once removed, and movingreferences/aside failscheck-skill-refswith the dangling pointer, which is the fresh-clone failure that leaving the file untracked would have caused. Every factual claim the diff adds was checked against the source, script or make target it names.Left out deliberately: the matching routing row in
AGENTS.md. That file is untracked by project rule, so the usbdevfs entry lives only in a working copy and a fresh clone reaches the new content through the skill descriptions alone.