From 36c44a75653219eccba221b7597325104d55ec81 Mon Sep 17 00:00:00 2001 From: Koen van der Veen Date: Wed, 2 Sep 2026 11:14:09 +0200 Subject: [PATCH 1/8] chore: add syft-pr-review skill Adds a Claude Code skill that turns a syft PR into a short review document: the flows that changed first, then what was added, then every individual change, all as tickable checkboxes so a reviewer can work through it. Also un-ignores .claude/skills/syft-pr-review/ so the skill is shared, while other local skills under .claude/ stay ignored. --- .claude/skills/syft-pr-review/SKILL.md | 121 +++++++++++++++++++++++++ .gitignore | 3 + 2 files changed, 124 insertions(+) create mode 100644 .claude/skills/syft-pr-review/SKILL.md diff --git a/.claude/skills/syft-pr-review/SKILL.md b/.claude/skills/syft-pr-review/SKILL.md new file mode 100644 index 00000000000..b48ea3d1cfd --- /dev/null +++ b/.claude/skills/syft-pr-review/SKILL.md @@ -0,0 +1,121 @@ +--- +name: syft-pr-review +description: Write a review document for a syft pull request. Takes a PR number or URL, reads the diff, and produces a checkbox-structured summary of the flows that changed, what was added, and each individual change. Use when the user says "review PR 1234", "write me a review doc for ", or "what changed in this PR". +allowed-tools: Bash(gh:*), Bash(git:*), Bash(cat:*), Bash(sed:*), Bash(grep:*), Bash(rg:*), Bash(mkdir:*), Bash(wc:*), Bash(ls:*), Read, Write, Glob, Grep +--- + +# Review a syft PR + +Write a document a reader finishes in a few minutes and still knows **what changed** and **what was +decided**. Explain the change; do not hunt for bugs (that is `/code-review`). + +## Input + +A PR number or URL. If none was given, ask for it. + +## Step 1 — Load the PR, and find the branch it really builds on + +```bash +gh pr view --json title,body,author,state,baseRefName,headRefName,additions,deletions,changedFiles +gh pr view --json files -q '.files[] | "\(.additions)+ \(.deletions)- \(.path)"' | sort -rn +``` + +If the description names a base branch other than the GitHub one, check whether it is merged yet: +`git merge-base --is-ancestor origin/ origin/`. If it is not, its commits land +with this PR, so review those too — do not shrink the document to the author's own commits. Say so +in one sentence at the top and tag each item with the branch it came from. The author's own commits: + +```bash +git log --oneline --no-merges origin/ ^origin/ ^origin/ +``` + +## Step 2 — Read the diff, not the description + +```bash +gh pr diff > /tmp/pr-.diff +grep -n '^diff --git' /tmp/pr-.diff # file boundaries; read the chunks with sed +``` + +Read source files first, then the places that call them — that is where the flows are. Read test +names only. Count and skip lock files, generated data, fixtures and reformatting, and say what you +skipped. Open the changed file from the repo when a diff chunk alone is not enough to describe +something correctly. + +Check before you write: whether a name is a method or a plain function, whether a folder is really a +package, and whether each claim in the PR description is still true. + +## Step 3 — Write it + +Save to `koen/pr-reviews/pr--.md` when a git-ignored `koen/` folder exists, otherwise +`pr-reviews/` in the repo root — and say the file is untracked. + +Flows come first, because they are why the reader opened the document. + +```md +# PR — + +`<repo>#<N>` · <author> · <head> → <base> · +<add>/-<del> across <files> files · <state> + +<2-4 plain sentences: what this PR does and why.> + +> Read time ~<n> min. Not read: <what, with counts>. + +--- + +- [ ] **1. Flows** + + - [ ] **1.1 <Flow name>** _(new)_ + - [ ] When <trigger>, `A.start()` calls `B.handle()`, which calls `C.save()`. + - [ ] Decision: we could have done Z; we do A because <reason>. + - [ ] **1.2 <Flow name>** _(changed)_ + - [ ] When we <do X>, we previously <did Z>. Now <A>. + - [ ] Now `Class.method()` computes X and calls `other.py: thing()`. + +- [ ] **2. What is new** — additions only + + - [ ] **NEW CLASS `Name`** — `path/file.py:<line>` — <what it is for>. Built by `A.b()`. (Flow 1.2) + - [ ] **NEW MODULE `path/file.py`** — <what it is for>. Defines `file.py: func()`. (Flow 1.1) + - [ ] **NEW helpers in `path/file.py`** — `f()` <does X>, `g()` <does Y>. (Flow 1.1) + +- [ ] **3. Changes** — everything changed or deleted, grouped by theme + + - [ ] **A — <Theme>** + - [ ] **A1 <label>** (Flow 1.2) — When we <do X>, we previously <did Z>, now we <do A>. + `path/file.py: Class.method()` + - [ ] **A2 <label>** — DELETED `Class.old()` and `file.py: helper()`, because <reason>. + +- [ ] **4. Open questions** + - [ ] <real decisions a reviewer must make; drop the section when there are none> +``` + +## Rules + +- [ ] **Every bullet is a checkbox**, at every depth. +- [ ] **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. +- [ ] **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. +- [ ] **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. +- [ ] **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. +- [ ] **Qualify every name.** `Class.method()` for a method, `file.py: function()` for a plain + function. Never a bare `_helper()` — the reader will guess the wrong owner. +- [ ] **Say who calls whom, and when it runs.** "`A.b()` calls `c.py: d()`", not "`A.b()` → `d()`". +- [ ] **Before and after, everywhere:** _When we do X, we previously did Z. Now we do A._ +- [ ] **Say what was decided** — the alternative and why it lost — not only what changed. +- [ ] **Budget reading time:** roughly 40 bullets for a normal PR, 120 for a very large one. +- [ ] Do not repeat the PR description. If your text matches it, you read the wrong thing. +- [ ] Cross-reference by name, e.g. `(Flow 1.2)`. Links such as `[x](#slug)` do not work: GitHub + gives headings no `id` inside a comment, so there is nothing to point at. + +## Finally + +Print the file path and a short spoken summary. Do not paste the document back. + +Offer to post it as a PR comment. If the user agrees, post a **new** comment — never edit an +existing one, because editing discards any checkboxes they have already ticked. Their ticks can be +recovered from `userContentEdits` in the GraphQL API, but only before the history is trimmed. diff --git a/.gitignore b/.gitignore index 148f0148f3a..a3387ece41d 100644 --- a/.gitignore +++ b/.gitignore @@ -176,6 +176,9 @@ SyftBox/ # Claude AI settings .claude/* !.claude/settings.json +!.claude/skills/ +.claude/skills/* +!.claude/skills/syft-pr-review/ CLAUDE.md # CI test outputs From d20afffa078f16079a69eaa617ca7fb1d8e37717 Mon Sep 17 00:00:00 2001 From: Koen van der Veen <koenlennartvanderveen@gmail.com> Date: Wed, 2 Sep 2026 11:29:13 +0200 Subject: [PATCH 2/8] chore: document the team/personal skills split in .gitignore --- .gitignore | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.gitignore b/.gitignore index a3387ece41d..69c1a177003 100644 --- a/.gitignore +++ b/.gitignore @@ -174,6 +174,14 @@ SyftBox/ *~ # Claude AI settings +# +# Skills are split in two: +# .claude/skills/ team skills, checked in - add one !-rule per skill +# .claude/skills.local/ personal skills, never checked in +# +# Claude Code only discovers skills under .claude/skills/, so a personal skill +# is symlinked from there into skills.local/. The .claude/skills/* rule below +# ignores those symlinks, so only the explicitly listed team skills are tracked. .claude/* !.claude/settings.json !.claude/skills/ From cdb4f67d04fb3b29ecbd433f07066404002945c7 Mon Sep 17 00:00:00 2001 From: Koen van der Veen <koenlennartvanderveen@gmail.com> Date: Wed, 2 Sep 2026 11:34:04 +0200 Subject: [PATCH 3/8] chore: make .claude skills private by default, shared per !-rule --- .gitignore | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 69c1a177003..778a663445f 100644 --- a/.gitignore +++ b/.gitignore @@ -175,13 +175,9 @@ SyftBox/ # Claude AI settings # -# Skills are split in two: -# .claude/skills/ team skills, checked in - add one !-rule per skill -# .claude/skills.local/ personal skills, never checked in -# -# Claude Code only discovers skills under .claude/skills/, so a personal skill -# is symlinked from there into skills.local/. The .claude/skills/* rule below -# ignores those symlinks, so only the explicitly listed team skills are tracked. +# Skills are private by default: .claude/skills/* ignores every skill, so a new +# one is never committed by accident. Share a skill with the team by adding a +# !-rule for it below. .claude/* !.claude/settings.json !.claude/skills/ From 9e43851513673977d597c3b99ee0d976e6c81f0f Mon Sep 17 00:00:00 2001 From: Koen van der Veen <koenlennartvanderveen@gmail.com> Date: Wed, 2 Sep 2026 11:55:43 +0200 Subject: [PATCH 4/8] chore: add a tests section to syft-pr-review, drop open questions --- .claude/skills/syft-pr-review/SKILL.md | 35 +++++++++++++++++++------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/.claude/skills/syft-pr-review/SKILL.md b/.claude/skills/syft-pr-review/SKILL.md index b48ea3d1cfd..0a35f3a5a46 100644 --- a/.claude/skills/syft-pr-review/SKILL.md +++ b/.claude/skills/syft-pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: syft-pr-review -description: Write a review document for a syft pull request. Takes a PR number or URL, reads the diff, and produces a checkbox-structured summary of the flows that changed, what was added, and each individual change. Use when the user says "review PR 1234", "write me a review doc for <PR link>", or "what changed in this PR". +description: Write a review document for a syft pull request. Takes a PR number or URL, reads the diff, and produces a checkbox-structured summary of the flows that changed, what was added, each individual change, and the tests. Use when the user says "review PR 1234", "write me a review doc for <PR link>", or "what changed in this PR". allowed-tools: Bash(gh:*), Bash(git:*), Bash(cat:*), Bash(sed:*), Bash(grep:*), Bash(rg:*), Bash(mkdir:*), Bash(wc:*), Bash(ls:*), Read, Write, Glob, Grep --- @@ -36,10 +36,10 @@ gh pr diff <N> > /tmp/pr-<N>.diff grep -n '^diff --git' /tmp/pr-<N>.diff # file boundaries; read the chunks with sed ``` -Read source files first, then the places that call them — that is where the flows are. Read test -names only. Count and skip lock files, generated data, fixtures and reformatting, and say what you -skipped. Open the changed file from the repo when a diff chunk alone is not enough to describe -something correctly. +Read source files first, then the places that call them — that is where the flows are. For tests, +read the name and what it asserts, not the whole body. Count and skip lock files, generated data, +fixtures and reformatting, and say what you skipped. Open the changed file from the repo when a diff +chunk alone is not enough to describe something correctly. Check before you write: whether a name is a method or a plain function, whether a folder is really a package, and whether each claim in the PR description is still true. @@ -77,15 +77,24 @@ Flows come first, because they are why the reader opened the document. - [ ] **NEW MODULE `path/file.py`** — <what it is for>. Defines `file.py: func()`. (Flow 1.1) - [ ] **NEW helpers in `path/file.py`** — `f()` <does X>, `g()` <does Y>. (Flow 1.1) -- [ ] **3. Changes** — everything changed or deleted, grouped by theme +- [ ] **3. Changes** — everything changed or deleted except tests, grouped by theme - [ ] **A — <Theme>** - [ ] **A1 <label>** (Flow 1.2) — When we <do X>, we previously <did Z>, now we <do A>. `path/file.py: Class.method()` - [ ] **A2 <label>** — DELETED `Class.old()` and `file.py: helper()`, because <reason>. -- [ ] **4. Open questions** - - [ ] <real decisions a reviewer must make; drop the section when there are none> +- [ ] **4. Tests** + + - [ ] **NEW `path/test_file.py`** — <n> tests: `test_a()` <what it checks>, `test_b()` + <what it checks>. + - [ ] **REWRITTEN `test_c()`** — now asserts <X> instead of <Y>, because <reason>. + - [ ] **UPDATED for the new code** — <n> tests across <n> files follow the new + `Class.method()` signature. Nothing is asserted differently. + - [ ] Decision: <only when a test settles something a reader would otherwise wonder about> + +- [ ] **5. Blocked on** — drop this section unless something real stops the merge + - [ ] <the problem, and what has to happen before this can go in> ``` ## Rules @@ -96,6 +105,13 @@ Flows come first, because they are why the reader opened the document. needs it on its 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. +- [ ] **Tests, in proportion.** One sentence per new test, saying what it checks. 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 line to a test whose meaning actually + changed, and say why. Do not walk through test bodies. +- [ ] **No question list.** A reviewer can ask their own questions, so do not collect them. Only + name something that genuinely blocks the merge, and leave section 5 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. @@ -106,7 +122,8 @@ Flows come first, because they are why the reader opened the document. function. Never a bare `_helper()` — the reader will guess the wrong owner. - [ ] **Say who calls whom, and when it runs.** "`A.b()` calls `c.py: d()`", not "`A.b()` → `d()`". - [ ] **Before and after, everywhere:** _When we do X, we previously did Z. Now we do A._ -- [ ] **Say what was decided** — the alternative and why it lost — not only what changed. +- [ ] **Say what was decided** — the alternative and why it lost — not only what changed. Only for + decisions that change how someone reads the code; skip the rest. - [ ] **Budget reading time:** roughly 40 bullets for a normal PR, 120 for a very large one. - [ ] Do not repeat the PR description. If your text matches it, you read the wrong thing. - [ ] Cross-reference by name, e.g. `(Flow 1.2)`. Links such as `[x](#slug)` do not work: GitHub From 575ee82b5340626fdec33fdbd83939d69b76cb6d Mon Sep 17 00:00:00 2001 From: Koen van der Veen <koenlennartvanderveen@gmail.com> Date: Wed, 2 Sep 2026 12:13:01 +0200 Subject: [PATCH 5/8] chore: add a code standards section to syft-pr-review --- .claude/skills/syft-pr-review/SKILL.md | 30 ++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/.claude/skills/syft-pr-review/SKILL.md b/.claude/skills/syft-pr-review/SKILL.md index 0a35f3a5a46..146f14c5c95 100644 --- a/.claude/skills/syft-pr-review/SKILL.md +++ b/.claude/skills/syft-pr-review/SKILL.md @@ -1,13 +1,14 @@ --- name: syft-pr-review -description: Write a review document for a syft pull request. Takes a PR number or URL, reads the diff, and produces a checkbox-structured summary of the flows that changed, what was added, each individual change, and the tests. Use when the user says "review PR 1234", "write me a review doc for <PR link>", or "what changed in this PR". +description: Write a review document for a syft pull request. Takes a PR number or URL, reads the diff, and produces a checkbox-structured summary of the flows that changed, what was added, each individual change, the tests, and any code-standards problems in the new code. Use when the user says "review PR 1234", "write me a review doc for <PR link>", or "what changed in this PR". allowed-tools: Bash(gh:*), Bash(git:*), Bash(cat:*), Bash(sed:*), Bash(grep:*), Bash(rg:*), Bash(mkdir:*), Bash(wc:*), Bash(ls:*), Read, Write, Glob, Grep --- # Review a syft PR Write a document a reader finishes in a few minutes and still knows **what changed** and **what was -decided**. Explain the change; do not hunt for bugs (that is `/code-review`). +decided**. Explain the change, and flag where the new code breaks a house standard. Do not hunt for +bugs — that is `/code-review`. ## Input @@ -93,10 +94,29 @@ Flows come first, because they are why the reader opened the document. `Class.method()` signature. Nothing is asserted differently. - [ ] Decision: <only when a test settles something a reader would otherwise wonder about> -- [ ] **5. Blocked on** — drop this section unless something real stops the merge +- [ ] **5. Code standards** — only where the new code breaks one; drop the section when it does not + + - [ ] `path/file.py: Class.method()` — <the problem, one line> + +- [ ] **6. Blocked on** — drop this section unless something real stops the merge - [ ] <the problem, and what has to happen before this can go in> ``` +### What counts as a code-standards problem + +Judge only the lines this PR touched. Do not review the code around them, and do not turn this into +a second review. Name the file and the `Class.method()` or `file.py: function()`, keep each to one +line, and write nothing when there is nothing wrong. Look for: + +- a function or method you have to scroll to read; aim for 10 lines, and allow more only when it + calls nothing else +- a method that calls out and then works on the answer inline — `ab = Y.Z()` followed by logic on + `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 +- 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 + ## Rules - [ ] **Every bullet is a checkbox**, at every depth. @@ -110,8 +130,10 @@ Flows come first, because they are why the reader opened the document. 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 line to a test whose meaning actually changed, and say why. Do not walk through test bodies. +- [ ] **Standards: new code only.** Flag a standards problem only in lines this PR touched, and + only when there is one. Never audit the surrounding code. - [ ] **No question list.** A reviewer can ask their own questions, so do not collect them. Only - name something that genuinely blocks the merge, and leave section 5 out when nothing does. + 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. From f89baef09a39f641ad5490beb195e9c26414749c Mon Sep 17 00:00:00 2001 From: Koen van der Veen <koenlennartvanderveen@gmail.com> Date: Wed, 2 Sep 2026 12:17:55 +0200 Subject: [PATCH 6/8] chore: relax the function length standard to 20 lines --- .claude/skills/syft-pr-review/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/syft-pr-review/SKILL.md b/.claude/skills/syft-pr-review/SKILL.md index 146f14c5c95..b9c48318e21 100644 --- a/.claude/skills/syft-pr-review/SKILL.md +++ b/.claude/skills/syft-pr-review/SKILL.md @@ -108,7 +108,7 @@ Judge only the lines this PR touched. Do not review the code around them, and do a second review. Name the file and the `Class.method()` or `file.py: function()`, keep each to one line, and write nothing when there is nothing wrong. Look for: -- a function or method you have to scroll to read; aim for 10 lines, and allow more only when it +- a function or method you have to scroll to read; aim for 20 lines, and allow more only when it calls nothing else - a method that calls out and then works on the answer inline — `ab = Y.Z()` followed by logic on `ab` belongs in its own helper From 3f4a1f6ba08955c9e2be58f31a19894de0496ddb Mon Sep 17 00:00:00 2001 From: Koen van der Veen <koenlennartvanderveen@gmail.com> Date: Wed, 2 Sep 2026 12:20:30 +0200 Subject: [PATCH 7/8] chore: set the function length standard to 30 lines --- .claude/skills/syft-pr-review/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/syft-pr-review/SKILL.md b/.claude/skills/syft-pr-review/SKILL.md index b9c48318e21..1eb8b3325d5 100644 --- a/.claude/skills/syft-pr-review/SKILL.md +++ b/.claude/skills/syft-pr-review/SKILL.md @@ -108,7 +108,7 @@ Judge only the lines this PR touched. Do not review the code around them, and do a second review. Name the file and the `Class.method()` or `file.py: function()`, keep each to one line, and write nothing when there is nothing wrong. Look for: -- a function or method you have to scroll to read; aim for 20 lines, and allow more only when it +- a function or method you have to scroll to read; aim for 30 lines, and allow more only when it calls nothing else - a method that calls out and then works on the answer inline — `ab = Y.Z()` followed by logic on `ab` belongs in its own helper From 7129cf10d07a7f4f966417b7f81daac303ab4755 Mon Sep 17 00:00:00 2001 From: Koen van der Veen <koenlennartvanderveen@gmail.com> Date: Wed, 2 Sep 2026 12:25:39 +0200 Subject: [PATCH 8/8] chore: exempt test functions from the length standard --- .claude/skills/syft-pr-review/SKILL.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/skills/syft-pr-review/SKILL.md b/.claude/skills/syft-pr-review/SKILL.md index 1eb8b3325d5..2cb70a15965 100644 --- a/.claude/skills/syft-pr-review/SKILL.md +++ b/.claude/skills/syft-pr-review/SKILL.md @@ -109,7 +109,8 @@ a second review. Name the file and the `Class.method()` or `file.py: function()` line, and write nothing when there is nothing wrong. Look for: - a function or method you have to scroll to read; aim for 30 lines, and allow more only when it - calls nothing else + calls nothing else. Test functions are exempt — a long test is normal, so never flag one for + length - a method that calls out and then works on the answer inline — `ab = Y.Z()` followed by logic on `ab` belongs in its own helper - string building that is not an f-string