Skip to content

chore: sharpen syft-pr-review on proportionality, self-contained bullets and duplication - #9500

Merged
koenvanderveen merged 2 commits into
devfrom
koen/pr-review-skill-standards
Sep 2, 2026
Merged

chore: sharpen syft-pr-review on proportionality, self-contained bullets and duplication#9500
koenvanderveen merged 2 commits into
devfrom
koen/pr-review-skill-standards

Conversation

@koenvanderveen

Copy link
Copy Markdown
Collaborator

Summary

Five pieces of feedback on the review documents the skill produces. Three were rules the skill already had and did not enforce hard enough, so those are sharpened in place rather than joined by a near-duplicate neighbour.

Changes

Sharpened rules

  • Length follows the codeLength and position follow the code — makes explicit that word count and placement are both claims about how much something matters, and names the case that prompted it: mechanical cleanups (a moved import, a renamed local, a deleted comment) 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. B — Errors that name both sides tells a reader nothing about what raises the error or who the two sides are, so every bullet under it names its own subject.
  • One test, one bullet splits in two. The first keeps the shape; the new A test bullet carries its own context requires the condition, what runs, and what is then true, with the before/after wording: 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.

New code standards

  • Two functions or methods doing substantially the same work — one belongs, called from both places.
  • A test name that pads one that works: 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.

Removed

  • The semicolon clause in the test rule, which Scannable, not prose already covers.

Testing

193 lines / 1988 words / 19 rules, inside the 220 / 2200 / 20 budget. pre-commit passes. Regenerated the review on #9497 with it — see the comment there.

…ets 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.
A skill edit landed on a working branch before it lands on the base rides
along in that branch's PR diff, so the review would describe the reviewer's
own tooling change as the PR author's work. Classify .claude/ as skippable,
alongside lock files and generated data.
@koenvanderveen
koenvanderveen merged commit d9a2565 into dev Sep 2, 2026
33 checks passed
@koenvanderveen
koenvanderveen deleted the koen/pr-review-skill-standards branch September 2, 2026 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant