From a75e4903c3bb03fbd6776ce69186af5953967901 Mon Sep 17 00:00:00 2001 From: Koen van der Veen Date: Wed, 2 Sep 2026 14:55:53 +0200 Subject: [PATCH] chore: sharpen syft-pr-review on proportionality, self-contained bullets and duplication Five pieces of feedback, three of which were rules the skill already had and did not enforce hard enough, so they are sharpened rather than joined by a neighbour: - 'Length follows the code' becomes 'Length and position follow the code', and names the case that prompted it: mechanical cleanups get one short note at the end of their theme, never a bullet of their own. - 'Every bullet stands alone' now says a theme heading is a filing label, not context, so each bullet under it names its own subject. - The test-bullet rule splits: one keeps the shape, a new one requires the bullet to carry its own context and gives the before/after wording. Two additions to the code standards: duplicated functions or methods, and test names padded with articles and filler. The semicolon clause is dropped from the test rule because 'Scannable, not prose' already covers it. 193 lines / 1988 words / 19 rules, inside the budget. --- .claude/skills/syft-pr-review/SKILL.md | 33 +++++++++++++++++--------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/.claude/skills/syft-pr-review/SKILL.md b/.claude/skills/syft-pr-review/SKILL.md index 149d9b758b0..9950e1da080 100644 --- a/.claude/skills/syft-pr-review/SKILL.md +++ b/.claude/skills/syft-pr-review/SKILL.md @@ -101,8 +101,8 @@ Flows come first, because they are why the reader opened the document. - [ ] **4. Tests** - [ ] **NEW `path/test_file.py`** — tests<, and how they are set up, when it is worth a clause> - - [ ] `test_a()` — - - [ ] `test_b()` — + - [ ] `test_a()` — + - [ ] `test_b()` — - [ ] **REWRITTEN `test_c()`** — `path/test_file.py` — now asserts instead of , because . - [ ] **UPDATED for the new code** — tests across files follow the new @@ -130,8 +130,12 @@ line, and write nothing when there is nothing wrong. Look for: `ab` belongs in its own helper - string building that is not an f-string - a repeated or magic value that belongs in a module-level constant +- two functions or methods that do substantially the same work; one of them belongs, called from + both places. Name the pair and what they share - an import inside a function; fine only to break a circular import, and worth one short note -- a test whose name does not say what it checks; a long name is fine +- a test name that does not say what it checks, or pads a name that does. Length is free when every + word earns it, so drop articles and filler: `test_no_hint_when_do_owns_no_jobs`, not + `test_no_hint_when_the_do_owns_none_of_the_jobs` ## Rules @@ -139,16 +143,20 @@ line, and write nothing when there is nothing wrong. Look for: - [ ] **Scannable, not prose.** Keep a bullet to two or three lines. When a bullet reaches for a semicolon to join items, or repeats the same shape three times over, those items are separate child bullets. The reader is looking for one thing, not reading front to back. -- [ ] **Length follows the code.** A ten-line class gets a few words, not five bullets. Group small - related additions under one bullet. Give something its own top-level bullet only when a reader - needs it on its own. +- [ ] **Length and position follow the code.** Word count and placement are both claims about how + much something matters, so a ten-line class gets a few words, not five bullets. Group small + related additions under one bullet. Collect mechanical cleanups — a moved import, a renamed + local, a deleted comment — into one short note at the end of the theme they belong to, never a + bullet of their own. - [ ] **Say it once.** Section 2 describes code that was added. Section 3 covers what behaviour changed and what was deleted, and refers to new code by name rather than describing it again. Tests belong only in section 4. -- [ ] **One test, one bullet, nested under its file.** Never a single bullet that lists several - tests separated by semicolons — a reader scans that list for one name and has to read a - paragraph instead. The file gets the parent bullet with the count; each test gets a child - bullet of `` `test_name()` — what it checks``, one sentence. Do not walk through test bodies. +- [ ] **One test, one bullet, nested under its file.** The file gets the parent bullet with the + count, each test a child bullet. Do not walk through test bodies. +- [ ] **A test bullet carries its own context.** One sentence naming the condition, what runs, and + what is then true — the reader has only that line. Write _if a job list holds no unique name, + the hint under the table renders a position instead of a name_, not _with no unique name left, + the hint gives a position_. Give the minimum that makes it stand up, not the whole setup. - [ ] **Tests, in proportion.** Collapse tests that only follow the new code — a changed call, a rebuilt fixture — into a single bullet with a count, since nothing is asserted differently. Give a bullet of its own to a test whose meaning actually changed, and say why. @@ -158,7 +166,10 @@ line, and write nothing when there is nothing wrong. Look for: name something that genuinely blocks the merge, and leave section 6 out when nothing does. - [ ] **Plain words.** Explain any term you have to use. Do not write "on the wire" (say _sent over the network_), "opaque", "envelope", "surface", "inert" or "residual". -- [ ] **Every bullet stands alone.** A reader three levels deep must not need the bullet above it. +- [ ] **Every bullet stands alone.** A reader three levels deep must not need the bullet above it. A + theme heading is a filing label, not context: "B — Errors that name both sides" says nothing + about what raises the error or who the two sides are, so every bullet under it names its own + subject. - [ ] **Name both sides.** Never just "a version mismatch" — say which versions, held by whom, and compared against what: this client against the peer's version file, the local folder against the copy on Drive, one protocol against another. The same goes for any comparison or hand-off.