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".
Write a document a reader finishes in a few minutes and still knows what changed and what was
decided. Explain the change, and flag where the new code breaks a house standard. Do not hunt for
bugs — that is /code-review.
Input
A PR number or URL. If none was given, ask for it.
the description, to check its claims against the diff
files.txt
every changed path with its +/-, largest first
files-skippable.txt
lock files, notebooks and generated data — count these, do not read
commits.txt
non-merge commits on the head branch that are not in the base
merges.txt
merge commits in that same range
base-check.txt
every branch named in the body, and whether it has landed yet
full.diff
the whole diff
diff-index.txt
path<TAB>per-file diff; cat the chunk you want
new-symbols.txt
every def/class the diff adds, with its line number on the head
new-tests.txt
every test function and class the diff adds
Read base-check.txt first. A branch marked NOT-MERGED is a base that has not landed, so its
commits land with this PR and belong in the document — say so in one sentence at the top and tag
each item with the branch it came from. commits.txt is the author's own work.
new-symbols.txt is a candidate list, not the answer: it also catches a nested function and a
pre-existing def whose signature changed. Confirm each one is really new before you call it new.
Keep the script in step with this file. If a step below needs something /tmp/pr-<N>/ does not
hold — because the skill changed, or because this PR has a shape the script does not cover — add it
to prefetch.sh and re-run, rather than running the command by hand. The point of the script is
that one call answers the whole of steps 1 and 2.
Step 2 — Read the diff, not the description
cat the per-file chunks from diff-index.txt, 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.
Say what you skipped, with counts, using files-skippable.txt. 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 body.md is still true.
Step 3 — Write it
Save to koen/reviews/<N>/summary.md when a git-ignored koen/ folder exists, otherwise
reviews/<N>/summary.md in the repo root — and say the file is untracked. One folder per PR, so
anything else about that review lands beside the summary later.
Flows come first, because they are why the reader opened the document.
# PR <N> — <title>
`<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
- [ ] **2.1 NEW CLASS `Name`** — `path/file.py:<line>` — <what it is for>. Built by `A.b()`. (Flow 1.2)
- [ ] **2.2 NEW MODULE `path/file.py`** — <what it is for>. Defines `file.py: func()`. (Flow 1.1)
- [ ] **2.3 NEW helpers in `path/file.py`** — `f()` <does X>, `g()` <does Y>. (Flow 1.1)
- [ ] **3. Changes** — everything changed or deleted except tests, grouped by theme
- [ ] **3.1 <Theme>**
- [ ] **3.1.1 <label>** (Flow 1.2) — When we <do X>, we previously <did Z>, now we <do A>.
`path/file.py: Class.method()`
- [ ] **3.1.2 <label>** — DELETED `Class.old()` and `file.py: helper()`, because <reason>.
- [ ] **4. Tests**
- [ ] **4.1 NEW `path/test_file.py`** — <n> tests<, and how they are set up, when it is worth a clause>
- [ ] `test_a()` — <the condition, what runs, and what is then true>
- [ ] `test_b()` — <the condition, what runs, and what is then true>
- [ ] **4.2 REWRITTEN `test_c()`** — `path/test_file.py` — now asserts <X> instead of <Y>, because
<reason>.
- [ ] **4.3 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. Code standards** — only where the new code breaks one; drop the section when it does not
- [ ] **5.1** `path/file.py: Class.method()` — <the problem, one line>
- [ ] **6. Blocked on** — drop this section unless something real stops the merge
- [ ] **6.1** <the problem, and what has to happen before this can go in>
- [ ] **7. Comments for the author** — leave it empty; the review UI writes into it
Number the comments section after the sections that survived, so it is 5, 6 or 7 depending on which
of those were dropped. It is always last, and always there.
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 30 lines, and allow more only when it
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
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. Let a repeated line or two go — the copy has to be
big enough that pulling it out is worth a helper
an import inside a function; fine only to break a circular import, and worth one short note
a name that does not say what the thing is, or pads a name that does. Length is free when every
word earns it, so drop articles and filler. Say it as a plain sentence and stop there:
test_no_hint_when_the_do_owns_none_of_the_jobs() violates the no-filler rule, rename to test_no_hint_when_do_owns_no_jobs()
Rules
Every bullet is a checkbox, at every depth.
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 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. 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.
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 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. 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.
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. 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.
Number every section and subsection, **<N>.<M> <label>**, in every section and to the
same depth as the template shows. A leaf bullet is not numbered — its position under a
numbered parent already names it. This is what lets a comment say which bullet it is about.
Cross-reference by number, e.g. (Flow 1.2) or (3.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.
1---2name: syft-pr-review3description: 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".4---56# Review a syft PR78Write a document a reader finishes in a few minutes and still knows **what changed** and **what was9decided**. Explain the change, and flag where the new code breaks a house standard. Do not hunt for10bugs — that is `/code-review`.1112## Input1314A PR number or URL. If none was given, ask for it.1516## Step 1 — Prefetch, in one call1718```bash19./.claude/skills/syft-pr-review/prefetch.sh <N> # writes /tmp/pr-<N>/20```2122It prints an index and writes:2324| file | what is in it |25| --------------------- | ------------------------------------------------------------------- |26| `meta.json` | title, body, author, state, base, head, +/-, file count |27| `body.md` | the description, to check its claims against the diff |28| `files.txt` | every changed path with its +/-, largest first |29| `files-skippable.txt` | lock files, notebooks and generated data — count these, do not read |30| `commits.txt` | non-merge commits on the head branch that are not in the base |31| `merges.txt` | merge commits in that same range |32| `base-check.txt` | every branch named in the body, and whether it has landed yet |33| `full.diff` | the whole diff |34| `diff-index.txt` | `path<TAB>per-file diff`; `cat` the chunk you want |35| `new-symbols.txt` | every `def`/`class` the diff adds, with its line number on the head |36| `new-tests.txt` | every test function and class the diff adds |3738Read `base-check.txt` first. A branch marked NOT-MERGED is a base that has not landed, so its39commits land with this PR and belong in the document — say so in one sentence at the top and tag40each item with the branch it came from. `commits.txt` is the author's own work.4142`new-symbols.txt` is a candidate list, not the answer: it also catches a nested function and a43pre-existing `def` whose signature changed. Confirm each one is really new before you call it new.4445**Keep the script in step with this file.** If a step below needs something `/tmp/pr-<N>/` does not46hold — because the skill changed, or because this PR has a shape the script does not cover — add it47to `prefetch.sh` and re-run, rather than running the command by hand. The point of the script is48that one call answers the whole of steps 1 and 2.4950## Step 2 — Read the diff, not the description5152`cat` the per-file chunks from `diff-index.txt`, source files first, then the places that call them53— that is where the flows are. For tests, read the name and what it asserts, not the whole body.54Say what you skipped, with counts, using `files-skippable.txt`. Open the changed file from the repo55when a diff chunk alone is not enough to describe something correctly.5657Check before you write: whether a name is a method or a plain function, whether a folder is really a58package, and whether each claim in `body.md` is still true.5960## Step 3 — Write it6162Save to `koen/reviews/<N>/summary.md` when a git-ignored `koen/` folder exists, otherwise63`reviews/<N>/summary.md` in the repo root — and say the file is untracked. One folder per PR, so64anything else about that review lands beside the summary later.6566Flows come first, because they are why the reader opened the document.6768```md69# PR <N> — <title>7071`<repo>#<N>` · <author> · <head> → <base> · +<add>/-<del> across <files> files · <state>7273<2-4 plain sentences: what this PR does and why.>7475> Read time ~<n> min. Not read: <what, with counts>.7677---7879- [ ] **1. Flows**8081 - [ ] **1.1 <Flow name>** _(new)_82 - [ ] When <trigger>, `A.start()` calls `B.handle()`, which calls `C.save()`.83 - [ ] Decision: we could have done Z; we do A because <reason>.84 - [ ] **1.2 <Flow name>** _(changed)_85 - [ ] When we <do X>, we previously <did Z>. Now <A>.86 - [ ] Now `Class.method()` computes X and calls `other.py: thing()`.8788- [ ] **2. What is new** — additions only8990 - [ ] **2.1 NEW CLASS `Name`** — `path/file.py:<line>` — <what it is for>. Built by `A.b()`. (Flow 1.2)91 - [ ] **2.2 NEW MODULE `path/file.py`** — <what it is for>. Defines `file.py: func()`. (Flow 1.1)92 - [ ] **2.3 NEW helpers in `path/file.py`** — `f()` <does X>, `g()` <does Y>. (Flow 1.1)9394- [ ] **3. Changes** — everything changed or deleted except tests, grouped by theme9596 - [ ] **3.1 <Theme>**97 - [ ] **3.1.1 <label>** (Flow 1.2) — When we <do X>, we previously <did Z>, now we <do A>.98 `path/file.py: Class.method()`99 - [ ] **3.1.2 <label>** — DELETED `Class.old()` and `file.py: helper()`, because <reason>.100101- [ ] **4. Tests**102103 - [ ] **4.1 NEW `path/test_file.py`** — <n> tests<, and how they are set up, when it is worth a clause>104 - [ ] `test_a()` — <the condition, what runs, and what is then true>105 - [ ] `test_b()` — <the condition, what runs, and what is then true>106 - [ ] **4.2 REWRITTEN `test_c()`** — `path/test_file.py` — now asserts <X> instead of <Y>, because107 <reason>.108 - [ ] **4.3 UPDATED for the new code** — <n> tests across <n> files follow the new109 `Class.method()` signature. Nothing is asserted differently.110 - [ ] Decision: <only when a test settles something a reader would otherwise wonder about>111112- [ ] **5. Code standards** — only where the new code breaks one; drop the section when it does not113114 - [ ] **5.1** `path/file.py: Class.method()` — <the problem, one line>115116- [ ] **6. Blocked on** — drop this section unless something real stops the merge117118 - [ ] **6.1** <the problem, and what has to happen before this can go in>119120- [ ] **7. Comments for the author** — leave it empty; the review UI writes into it121```122123Number the comments section after the sections that survived, so it is 5, 6 or 7 depending on which124of those were dropped. It is always last, and always there.125126### What counts as a code-standards problem127128Judge only the lines this PR touched. Do not review the code around them, and do not turn this into129a second review. Name the file and the `Class.method()` or `file.py: function()`, keep each to one130line, and write nothing when there is nothing wrong. Look for:131132- a function or method you have to scroll to read; aim for 30 lines, and allow more only when it133 calls nothing else. Test functions are exempt — a long test is normal, so never flag one for134 length135- a method that calls out and then works on the answer inline — `ab = Y.Z()` followed by logic on136 `ab` belongs in its own helper137- string building that is not an f-string138- a repeated or magic value that belongs in a module-level constant139- two functions or methods that do substantially the same work; one of them belongs, called from140 both places. Name the pair and what they share. Let a repeated line or two go — the copy has to be141 big enough that pulling it out is worth a helper142- an import inside a function; fine only to break a circular import, and worth one short note143- a name that does not say what the thing is, or pads a name that does. Length is free when every144 word earns it, so drop articles and filler. Say it as a plain sentence and stop there:145 `test_no_hint_when_the_do_owns_none_of_the_jobs() violates the no-filler rule, rename to146test_no_hint_when_do_owns_no_jobs()`147148## Rules149150- [ ] **Every bullet is a checkbox**, at every depth.151- [ ] **Scannable, not prose.** Keep a bullet to two or three lines. When a bullet reaches for a152 semicolon to join items, or repeats the same shape three times over, those items are separate153 child bullets. The reader is looking for one thing, not reading front to back.154- [ ] **Length and position follow the code.** Word count and placement are both claims about how155 much something matters, so a ten-line class gets a few words, not five bullets. Group small156 related additions under one bullet. Collect mechanical cleanups — a moved import, a renamed157 local, a deleted comment — into one short note at the end of the theme they belong to, never a158 bullet of their own.159- [ ] **Say it once.** Section 2 describes code that was added. Section 3 covers what behaviour160 changed and what was deleted, and refers to new code by name rather than describing it again.161 Tests belong only in section 4.162- [ ] **One test, one bullet, nested under its file.** The file gets the parent bullet with the163 count, each test a child bullet. Do not walk through test bodies.164- [ ] **A test bullet carries its own context.** One sentence naming the condition, what runs, and165 what is then true — the reader has only that line. Write _if a job list holds no unique name,166 the hint under the table renders a position instead of a name_, not _with no unique name left,167 the hint gives a position_. Give the minimum that makes it stand up, not the whole setup.168- [ ] **Tests, in proportion.** Collapse tests that only follow the new code — a changed call, a169 rebuilt fixture — into a single bullet with a count, since nothing is asserted differently.170 Give a bullet of its own to a test whose meaning actually changed, and say why.171- [ ] **Standards: new code only.** Flag a standards problem only in lines this PR touched, and172 only when there is one. Never audit the surrounding code.173- [ ] **No question list.** A reviewer can ask their own questions, so do not collect them. Only174 name something that genuinely blocks the merge, and leave section 6 out when nothing does.175- [ ] **Plain words.** Explain any term you have to use. Do not write "on the wire" (say _sent over176 the network_), "opaque", "envelope", "surface", "inert" or "residual".177- [ ] **Every bullet stands alone.** A reader three levels deep must not need the bullet above it. A178 theme heading is a filing label, not context: "B — Errors that name both sides" says nothing179 about what raises the error or who the two sides are, so every bullet under it names its own180 subject.181- [ ] **Name both sides.** Never just "a version mismatch" — say which versions, held by whom, and182 compared against what: this client against the peer's version file, the local folder against the183 copy on Drive, one protocol against another. The same goes for any comparison or hand-off.184- [ ] **Qualify every name.** `Class.method()` for a method, `file.py: function()` for a plain185 function. Never a bare `_helper()` — the reader will guess the wrong owner.186- [ ] **Say who calls whom, and when it runs.** "`A.b()` calls `c.py: d()`", not "`A.b()` → `d()`".187- [ ] **Before and after, everywhere:** _When we do X, we previously did Z. Now we do A._188- [ ] **Say what was decided** — the alternative and why it lost — not only what changed. Only for189 decisions that change how someone reads the code; skip the rest.190- [ ] **Budget reading time:** roughly 40 bullets for a normal PR, 120 for a very large one.191- [ ] Do not repeat the PR description. If your text matches it, you read the wrong thing.192- [ ] **Number every section and subsection**, `**<N>.<M> <label>**`, in every section and to the193 same depth as the template shows. A leaf bullet is not numbered — its position under a194 numbered parent already names it. This is what lets a comment say which bullet it is about.195- [ ] Cross-reference by number, e.g. `(Flow 1.2)` or `(3.1.2)`. Links such as `[x](#slug)` do not196 work: GitHub gives headings no `id` inside a comment, so there is nothing to point at.197198## Finally199200Print the file path and a short spoken summary. Do not paste the document back.201202Offer to post it as a PR comment. If the user agrees, post a **new** comment — never edit an203existing one, because editing discards any checkboxes they have already ticked. Their ticks can be204recovered from `userContentEdits` in the GraphQL API, but only before the history is trimmed.
Run npx skillmds@latest add openmined/syft-pr-review in your terminal (requires Node.js), paste this page's agent-chat prompt into Claude, Cursor, or any MCP-connected agent, or download the SKILL.md file and copy it into your agent's skills directory.
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". It is listed under Coding & Dev Tools on SkillMD.
This skill has not completed SkillMD's automated safety review yet. Independent scanners report: SkillSpector: PASS, Skill Scanner: PASS. Capability flags: executes scripts. SkillMD never runs a skill's scripts for you; review the SKILL.md before installing.
This skill is tagged as working with Claude Code, Claude.ai, OpenAI Codex. SKILL.md is an open format, so most agents that read a skills directory can load it too.
Yes. Installing skills from SkillMD is free, and the skill stays under its author's original license.
openmined (@openmined) published this skill. Their other Agent Skills are listed on their SkillMD profile.
openmined