skills: fix the flaws reviews of past PRs found in the skills

Problem

The skills under .claude/skills were used to review the first commit of twelve merged PRs (#19,
#97, #109, #112, #136, #183, #189, #212, #273, #341, #355, #815). Each review ended with a note on
where a skill was wrong, unclear or missing something. The flaws below are the distinct ones, each
re-checked on main @0958eaf8 and still present there. They are small and independent, so they are
collected here; split any item out once it needs its own discussion.

Wrong scripts and rules (bugs)

skills_lint.py reports clean when it finds no skills. It ends the push checklist, so
&& git push goes ahead over a tree with no .claude/skills at all. Six of the twelve
reviews hit it.
```
$ cd "$(mktemp -d)" && git init -q
$ python3.14 -I <repo>/.claude/skills/morphic-curating-skills/scripts/skills_lint.py; echo exit=$?
skills_lint: clean (0 skills)
exit=0
```
Finding nothing to check should fail, or at least not print clean; differentials.md's
validate-the-instrument-first says a pass over zero inputs does not count.
The push checklist's artifact grep flags every go.sum line. The pattern's
(^|[^a-z0-9])[hm][0-9]{1,2}([^0-9a-z]|$) alternative matches the h1: hash prefix.
```
$ pat=...   # the pat= line of morphic-shipping-prs/SKILL.md
$ printf '+example.com/m v1.0.0 h1:abc=\n' | grep -ciE "$pat"
1
```
On refactor(cmd/morphic)!: run the CLI on cobra #815's first commit, all 55 added go.sum lines were hits. Excluding the file from the diff
grep (git diff origin/main...HEAD -- ':!go.sum') is one fix.

sweep.py skips flow-style YAML specs. is_spec looks for a version key first on a line,
so a valid spec written as a flow mapping is left out of the corpus. A sweep run to check a
change to format detection then shares that change's blind spot, which is what happened on
feat(engine)!: let compilers own format detection and options #341's first commit (its sniff refused exactly such a spec).

$ printf '{openapi: 3.1.0, info: {title: t, version: "1"}, paths: {}}\n' > flow.yaml
$ printf 'openapi: 3.1.0\ninfo: {title: t, version: "1"}\npaths: {}\n' > block.yaml
$ python3.14 -I -c 'import sys; sys.path.insert(0, "<repo>/.claude/skills/morphic-verifying-changes/scripts")
import sweep; print([sweep.is_spec(open(f, "rb").read()) for f in ("flow.yaml", "block.yaml")])'
[False, True]

Unclear or wrong wording (docs)

diff-contains-only-in-scope-files cites refactor(compilers)!: rename frontend→compiler and backend→emitter #5 for a removed .claude/ ignore entry.
commits-and-prs.md says "a removed .claude/ ignore entry had to be restored (refactor(compilers)!: rename frontend→compiler and backend→emitter #5, fix(compilers/openapi): carry declared annotations, record preservation #119)", but
git show ea3292c2 -- .gitignore (refactor(compilers)!: rename frontend→compiler and backend→emitter #5) only adds docs/superpowers/ and removes nothing.

Missing rules (features)

price-common-path names no instrument when perf is absent. finding-cost.md gives
perf stat -e instructions:u on Linux and nothing else; which perf → perf not found on the
Linux hosts the reviews ran on. Name a fallback, such as alternated go test -bench runs with
-count and benchstat. (fix(compilers/openapi): numeric bases, example fields, and $ref cycles #109, Finish the compiler decomposition and fix three silent failures #136)
toolchain.md does not say to run the toolchain the tree's go.mod names.
grep -rn GOTOOLCHAIN .claude/skills finds only an anecdote in claims.md. At fix: settle what a case boundary is, and how a branch is named #189's first
commit, four tests failed under go1.27.3 and passed under GOTOOLCHAIN=go1.26.3, so a reviewer
following the skills alone blames the change.
No probe covers a change under cmd/. sweep.py passes the same flags to both builds, so
it cannot see a change to argument parsing. Add an argv differential: every documented spelling
and misuse class against the base binary, with the exit-code contract. (refactor(cmd/morphic)!: run the CLI on cobra #815)
No rule says a recover covers only its own goroutine. It cannot reach a library's own
goroutines (an errgroup fan-out) or a fatal error such as a stack overflow. (openapi: diagnose reference cycles instead of crashing with a stack overflow #19)
No rule requires a shortcut reader to fall back to the full read when it cannot decide. A
prefix, index or sniff that refuses instead refuses valid input; mirror-library-read-rules-exactly
comes closest but is worded for reference pre-scans. (feat(engine)!: let compilers own format detection and options #341)
No rule says which YAML version's scalar resolution the compiler follows (1.1 or 1.2).
The !!timestamp precedent and fix(compilers/openapi): numeric bases, example fields, and $ref cycles #109's octal reading went opposite ways.
No lesson on JSON null decoding into a non-pointer. A guard that asks "is this value
text" accepts null as an empty string. ir-lessons.md covers custom IR decoders on null,
not this. (feat(compilers/openapi): fill typed fields from vendor extensions #355)
No probe tests a heuristic default against the values the golden spec writes. The golden
spec appears only in cost rules (price-common-path). (feat(compilers/openapi): fill typed fields from vendor extensions #355)
character-predicate-witness-per-class speaks of one predicate. When a change redefines
one of a pair (upper and lower), the partner needs witnessing at the same classes. (fix: settle what a case boundary is, and how a branch is named #189)
No rule names a $ref chain as an input class for annotation fallbacks. Give every hop of
a two-hop chain a distinct value for each annotation read through it. (refactor(compilers/openapi): extract reference resolution into resolver #112)
split-guard-per-site does not cover an effect turned into a return value. That is a
guard moved to every return path and caller, and a diagnostic emitted only on a memoized first
call makes every path load-bearing. sweep-comments-a-behaviour-change-falsifies names the
pattern, but only for comments. (refactor(compilers/openapi): make the schema walk free functions #212)
The maintained-count probe greps the diff, not commit bodies. no-maintained-counts-or-state-in-prose
runs git diff origin/main...HEAD | grep …; a stale count in a commit body goes unseen. Add
git log --format=%B origin/main..HEAD. (refactor: move the naming and ID grammars into the framework #183)
No probe for a change whose effect depends on how the repository is checked out. The
deciding control for refactor: move the naming and ID grammars into the framework #183 was a clone with core.autocrlf=true.
No probe for an added ignore pattern. diff-contains-only-in-scope-files covers a removed
.gitignore line only. For an added one, run git add -A --dry-run and git check-ignore -v
at base and head over planted paths, including ones a tool documents as committable. (refactor: remove duplicated code and scaffolding across the repository #97)

Direction

Proposal: fix 1–4 first, since each has a probe that shows it on main; add a test beside the script
for 1 and 3 that fails on today's behaviour. Place 5–18 where morphic-curating-skills says a
lesson goes, each with a Rule, Why and Probe, and run skills_lint.py after each.

Out of scope

Notes that only reflected the reviewed revisions being older than the skills (commands, paths and
documents added later) are left out; they are not flaws in the skills.

Related: #1160, #1161

Priority: P4, housekeeping (changes no lowering, validation, IR or emit-contract behaviour). Probes
above run on morphic @0958eaf8.

What fixing this buys

Every review and every change that follows the skills inherits their flaws. The three bugs make a
check say something untrue; the missing rules leave a defect class to whoever happens to think of
it. Fixing them turns each into a step a review runs every time.

flowchart LR
  subgraph today["Today"]
    direction TB
    L["skills_lint finds no skills<br/>and prints clean"] --> L2["the push checklist passes<br/>with nothing checked"]
    G["the artifact grep matches<br/>the hash prefix in go.sum"] --> G2["a real hit hides among<br/>dozens of false ones"]
    S["sweep.py skips<br/>flow-style YAML"] --> S2["a sweep of a detection change<br/>cannot see what it broke"]
    M["no rule for items 5–18"] --> M2["each defect is found only if<br/>a reviewer thinks of it"]
  end
  subgraph after["After this issue"]
    direction TB
    L3["the lint fails on<br/>an empty tree"]
    G3["go.sum is excluded,<br/>so a hit means something"]
    S3["the sweep reaches<br/>every valid spec"]
    M3["a Rule, Why and Probe<br/>every review runs"]
  end
  L2 --> L3
  G2 --> G3
  S2 --> S3
  M2 --> M3

How much the missing rules matter was measured. The first commit of each of the twelve merged PRs
listed above was reviewed twice, by two reviews that differ mainly in where their rules come from:

Following the skills: the rules come only from .claude/skills as merged in chore(skills): add the repository's agent skills #1143.
Following the catalog: the rules come from the review catalog (about 130 keyed rules) that
chore(skills): add the repository's agent skills #1143 turned into these skills.

A grader who did not know which review was which matched every finding against what the PR's later
commits fixed. A false alarm is a finding the grader judged not to be a defect of that commit.

	Following the skills	Following the catalog
Defects later commits fixed, caught (of 47)	19	23
False alarms	8 of 107 findings	20 of 137 findings

The skills raised half the false alarms. The catalog caught more because it carried rules the
skills do not: items 5–18 here, and code-shape rules (a receiver or field nothing reads, a function
that lost its only caller) that today's lint and archtest cover only in part. Closing that gap
keeps the skills' precision and recovers the catches.

Verification at `0958eaf8`

I re-ran each claim above against main at 0958eaf8, and built the old revisions where a claim depends on one. Every item points at something real, and fixing 1 to 4 first is still the right order. Eleven items need a correction before anyone works from them: 1, 3, 4, 5, 7, 8, 9, 10, 13, 16 and 17. The comment's explanation, that the catalog caught more because it carried items 5 to 18, does not hold for most of them.

Item	Result	Evidence
1	Bug confirmed, stated consequence wrong	The lint prints `clean (0 skills)` and exits 0 only when run by absolute path from another tree. The checklist runs it by relative path, which exits 2 in a tree without skills, so `&& git push` stops. Wherever the script exists, its own skill does too. The fix is still worth making, because reviewing an old revision with today's script reaches this.
2	Confirmed	All 55 added `go.sum` lines hit on #815's first commit (`15c2ca10`). Removing the `[hm][0-9]{1,2}` alternative gives 0, and `':!go.sum'` leaves 0 hits on that commit. Other added lines can still hit, such as `/users/` paths.
3	Confirmed, narrower	`is_spec` misses a mapping whose version key is not first on a line: a one-line flow mapping, with or without `---`. A multi-line flow mapping with the key on its own line is found. The #341 claim needs a spec past the 64 KiB sniff cap. A 70 KB flow spec exits 2 at `40f12412` and 0 at its parent and on `main`, while the small `flow.yaml` above compiles at that commit. No YAML file in `testdata` (213) or in a local corpus of 3,780 more is skipped this way.
4	Confirmed, different fix	`.claude/` entered `.gitignore` in #97, so #5 could not have removed it, and #119's branch touches no `.gitignore`. The entry was removed and restored on #324's branch (`a89252c4`, `16bcd3e2`), which the squash does not show. The Why packs several clauses into one citation list: #5 does back the planning docs (`0b692476`) and #119 the rewrite notes. Add #324 and keep both.
5	Gap confirmed, proposed fallback weak	`finding-cost.md` names only `/usr/bin/time -l` on macOS and `perf stat` on Linux. `go test -bench` is wall-clock time, which `price-common-path`'s Why says load skews, and none of the nine benchmarks takes an external spec, so it cannot price the golden spec. Alternated runs of the built binary, comparing CPU time, would stay closer to the probe.
6	Confirmed	At `325d69c8`, with `GOFLAGS` empty: no failures on go1.26.3, and the same four tests fail on go1.27.2 and go1.27.3 (listed below). Only bare `go test` is exposed, since `Makefile:35` already pins `GOTOOLCHAIN` from `go.mod`. #888's agent file carried the line: "If `go version` disagrees with go.mod's `go` line, set `GOTOOLCHAIN` to match before blaming the code."
7	Confirmed, headline too wide	`sweep.py` runs the real binary, so it sees output and exit-code changes in `compile`. It runs no other subcommand and passes both builds the same flags, so argument parsing is out of its reach. No skill rule covers that.
8	True of the skills, but the fact has a home	No skill says what `recover` reaches. `barrier/doc.go:7` and `docs/parser-library.md#goroutine-per-node` both state it, and `.golangci.yml` forbids `recover` outside tests, so `barrier.Catch` is the one way in. A rule should cite those instead of restating them.
9	Partly	No rule says to fall back to the full read. The failure it describes is covered, though: `check-stays-silent-on-valid-input` requires a refusal to "flag only what it can prove wrong" and tests "every valid spelling". `mirror-library-read-rules-exactly` is not worded for reference pre-scans either. Its Rule opens "A pre-scan, index or stepped reader", and only its Why is about references. Sharpen the first rule instead of adding one.
10	Premise confirmed, wrong home	On `main`, `0644` is 420, the 1.1 reading. `0o17` is 15 and `yes` stays a string, the 1.2 reading, as does `on` under `type: boolean`. So the compiler follows no single version. `docs/compilers/openapi.md` says a value is read "by its resolved tag alone", and no document names a version. That is a library behaviour, so it belongs in a `docs/parser-library.md` entry.
11	Confirmed	`encoding/json/v2` decodes `null` into a `string` as `""` with a nil error, and #355's first commit (`d316e5d1`) has that shape. `main` reads a `*string`, and `x-deprecated-reason: null` stays in `Unmodeled` with an info diagnostic, so there is no live defect.
12	Confirmed	A search that allows for wrapped lines finds two mentions of the golden spec: `.claude/skills/morphic-measuring-performance/SKILL.md:17` and `price-common-path`. Both are about cost.
13	Partly	The rule does speak of one predicate, but #189 shows no upper and lower pair: `IsLower` is unchanged in `325d69c8` and in the fix. `f93f1e35` says one test was applied to "both boundary rules", which "ask different questions". The lesson is to recheck a redefined predicate at each call site.
14	Confirmed	The catalog's `resolve-to-true-target` said to follow a chain "to its end (not one hop)", and its Probe added "a two-hop alias chain". That key is gone from the skills, and no skill mentions one hop or two hops. Restoring it and extending it to annotations is less work than a new rule.
15	Confirmed	`anchorIndex.sites` at `fd554d5a8` returns its warning only from the call that builds the memo, so a caller that drops the returned diagnostics loses it for good.
16	Partly, low value	The Probe does read only the diff, but its Rule covers only "a comment or doc", and `claims.md` already treats commit text as claims to execute. All of the last 25 merges set their own `PR: <url>` body, so branch bodies did not land. I would drop this one.
17	Confirmed, thin	#183's body records the `core.autocrlf=true` control. That defect now has its own guards: `TestGitAttributes_PinsLineEndings`, `TestGitAttributes_ResolveToLF` and `TestTrackedFiles_HoldNoCarriageReturn`. One instance is thin support for a general rule.
18	Confirmed	`diff-contains-only-in-scope-files` covers a removed line only. #97's `4f64c463` added `.claude/`, and #1143 had to rewrite that to `.claude/*` plus `!.claude/skills/`.

Commands behind the corrections

# Item 1, in an empty repository: the checklist's own line
$ python3.14 .claude/skills/morphic-curating-skills/scripts/skills_lint.py; echo exit=$?
can't open file '.../skills_lint.py': [Errno 2] No such file or directory
exit=2

# Item 3: exit 2 at 40f12412, exit 0 at 40f12412^ and at 0958eaf8
$ python3.14 -c 'print("{openapi: 3.1.0, info: {title: t, version: \"1\"}, paths: {}, x-pad: \"%s\"}" % ("a"*70000))' > bigflow.yaml
$ morphic compile -- bigflow.yaml
morphic: engine: no compiler recognizes "bigflow.yaml" as a supported spec format

# Item 4
$ git log --format='%h %s' -S'.claude/' -- .gitignore
acf5e3ba chore(skills): add the repository's agent skills (#1143)
386265e9 refactor: remove duplicated code and scaffolding across the repository (#97)
$ git fetch origin refs/pull/324/head
$ git show --format= a89252c4 -- .gitignore | grep -E '^-\.claude/'
-.claude/
$ git show --format= 16bcd3e2 -- .gitignore | grep -E '^\+\.claude/'
+.claude/

# Item 6, from a worktree of 325d69c8
$ GOFLAGS= GOTOOLCHAIN=go1.26.3 go test ./...   # exit 0, 13 packages ok
$ GOFLAGS= GOTOOLCHAIN=go1.27.3 go test ./...   # exit 1, the four tests below

# Item 16
$ git log -25 --first-parent --format=%b origin/main | grep -c '^PR: '
25

The four tests in item 6 are TestSchema_UnserializableExtension, TestMeta_UnserializableExtensionStillWarns, TestAuth_UnserializableExtensionStillWarns and TestRawFromNode_SeparatesAbsentFromUnconvertible. The extension they treat as unserializable, {"1":"intkey"}, serializes under 1.27. GOFLAGS has to be free of -trimpath at that revision, because its archtest finds go.mod through runtime.Caller and fails for an unrelated reason otherwise.

Item 10 used these properties on one model:

a: {type: integer, default: 0644}        # 420
b: {type: string, default: 2021-01-01}   # "2021-01-01"
c: {type: string, default: yes}          # "yes"
d: {type: integer, default: 0o17}        # 15
e: {type: boolean, default: on}          # "on"

The measurement in the comment above

The "about 130 keyed rules" holds: tools/review-agent/catalog.md at #888's head (319b21b2) has exactly 130 keys, and 91 of them survive in the skills. I assumed that is the version the reviews used.

The explanation for the extra catches does not hold for most items. I searched the catalog and #888's agent file for each item's terms, then read the catalog-only rules whose names suggest overlap. Only three items turned up: 14, 6 (in the agent file, not the catalog) and part of 5 (justify-perf-with-real-measurement names benchstat, though not as a stand-in for perf). For items 7 to 13 and 15 to 18 the catalog has nothing the skills lack, so they cannot explain the difference. The code-shape half of the explanation is plausible, since the 39 catalog-only keys include delete-unreachable-code and drop-constant-parameters.

The differences are also small for this sample. Catches differ by 4 of 47 (40.4% and 48.9%), and even if all four ran one way, an exact paired test gives p = 0.125. False alarms are 7.5% and 14.6%, with Fisher's exact p = 0.106 if findings are treated as independent. "Half the false alarms" holds as a rate, while the counts are 8 and 20. "Keeps the skills' precision and recovers the catches" is a forecast that nothing here measured.

Filing and direction

CLAUDE.md asks for one mechanism per issue, and for each skill flaw to carry the type: label its kind calls for. This issue holds 18 unrelated items under type:bug, across bug, docs and feature work, while #1160, #1161, #1165 and #1166 are one flaw each. I would split it, starting with 1 to 4, which are ready to fix. Items 7 to 15, 17 and 18 are negative claims with no named probe, which the filing rules also ask for.

"Each with a Rule, Why and Probe" skips the order in morphic-curating-skills, which takes a check before a rule and sharpens a rule before copying it. That order sends 8 and 10 to documents, 9 to an existing rule and 14 to a restored one, and leaves 17 mostly covered by tests. Word caps are no obstacle, as the largest reference file is 2,414 of its 4,000 words. P4 is the right tier.

Not verified

The review table's figures, "six of the twelve reviews", and perf being missing on the Linux hosts come from runs this repository does not hold. I measured on macOS.

Navigation Menu

Repository navigation

skills: fix the flaws reviews of past PRs found in the skills #1162

Problem

Wrong scripts and rules (bugs)

Unclear or wrong wording (docs)

Missing rules (features)

Direction

Out of scope

What fixing this buys

Verification at `0958eaf8`

Commands behind the corrections

The measurement in the comment above

Filing and direction

Not verified

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

skills: fix the flaws reviews of past PRs found in the skills #1162

Description

Problem

Wrong scripts and rules (bugs)

Unclear or wrong wording (docs)

Missing rules (features)

Direction

Out of scope

Activity

fuad-daoud commented on Oct 30, 2026

What fixing this buys

OmarAlJarrah commented on Oct 30, 2026

Verification at 0958eaf8

Commands behind the corrections

The measurement in the comment above

Filing and direction

Not verified

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Verification at `0958eaf8`