Reviewing Pull Requests

Reviewing pull requests

A PR arrives with a title, a description and a green check mark, and all three are the author's claims. This skill judges a PR from three things instead: the issue (what is actually wrong or wanted), the diff read in the context of the code around it, and a verification run on a checkout of the branch. The verdict is about whether the change fits this project and has the shape the maintainers would have given it — because the most common fate of a working contribution here is not "declined" but "re-done smaller by the maintainer" (33 of 126 closed PRs with a discussion).

Everything mechanical is review.mjs:

R=.claude/skills/reviewing-pull-requests/review.mjs
node $R list                 # every open PR: kind, age, size by file kind, issues, flags, rivals
node $R dupes                # open PRs competing for the same issue or subject
node $R dossier 11536        # the PR in full: body, linked issues + their discussion, commits,
                             # files by kind, human review threads (bots folded), rivals
node $R verify 11536         # worktree at .claude/worktrees/pr-11536, install, typecheck, the
                             # PR's specs green on the branch and red with production reverted,
                             # sibling specs, dependency lines, docs impact — ~25 s for a small PR
node $R issue 6853           # an issue with its human comments: the problem statement itself
node $R clean 11536          # remove the worktree (all of them without a number)

dossier and verify save their reports under .claude/reviews/pr-N/ (gitignored). Nothing here writes to GitHub — see Conduct.

What is not a factor

State these plainly in a report when the author or a bot leans on them:

• CI status. Informational. Forks fail on missing tokens, runners flake, and a green run proves the spec passed, not that it tests anything. verify answers the real question.
• Merge conflicts. The maintainer merges main into the branch himself; a PR is merged with a merge commit, never squashed. Conflicts matter only when they span the whole change (a branch thousands of commits behind, like the MapLibre PR that was rebuilt instead).
• Age, size:* and lgtm labels. size:* is a bot's line count; lgtm is a bot mirroring a maintainer's APPROVED seconds later, not a judgement of its own. don't merge yet and State:* are issue labels; on PRs the state toggle is draft ↔ ready.
• Bot reviews. greptile, gemini and codecov comment on every PR. Their findings are unverified suggestions; a bot's "approve" is nothing.
• The description. Read it last, after the issue and the diff, so it cannot frame the reading. Accepted PR bodies are structured prose (## Why / ## What, ## Problem / ## Fix); a body longer than the diff is a warning, not a virtue.
• Whether the author used an agent. Thirteen agent-assisted PRs by one contributor merged with a single APPROVED each. What the maintainers refuse is bulk that does not integrate — additions only, planning artifacts in the tree, "vibe coded" — not the tool.

The dimensions, in the order they decide outcomes

1. Fit. Does this belong in Trilium's core at all, and in this shape? The questions are in references/philosophy.md, one per principle: script/widget material? an option for one use case? one surface of an app-wide behavior? a workaround for a library? outside the data/sync model? an undiscussed design in a maintainer-led area (mobile, sync, encryption, storage, trilium://)? a changed default or convention? a dependency? Fit decides more closed PRs than everything else combined; 40 of the closed PRs had specs.
2. Problem and cause. Is the problem real (an issue, a reproduction), is the cause the one the diff changes, and does the diff fix what the reporter described? The maintainer tests every PR by hand and the most common substantive review comment is "it doesn't fix the issue" / "it crashes on 00:aa:00" / "doesn't work on this theme". A fix at the place a symptom is seen rather than produced is REWORK. Is the new behavior the right one? When the old behavior is plainly broken (a dropped word, a crash, an empty result) but the input is ambiguous, the fix has to choose what the input means — and a red/green spec only proves the PR's own choice. Settle the intended semantics before reading the diff: what the reporter meant, what the User Guide shows, what similar apps do. Search syntax, parsers, link and date formats, and defaults are where this hides: #11596 split towers#book into a word plus a #book filter, while the convention elsewhere (Obsidian, hashtags, Gmail operators) is that a sigil opens syntax only at a word start and is literal inside one (C#). With no issue to say what the user meant, the choice is YOUR CALL.
3. Targeting and scope. A bugfix changes the cause and nothing else: no riding feature ("not part of the fix, it's a completely separate feature so it deserves its own PR"), no reformatting ("makes it really hard to see what actually got changed"), no "while I was here", no labels or options beyond what the feature needs. A feature does what the issue thread agreed to, not more.
4. Size and complexity, against the yardstick. An accepted bugfix is a median 26 production lines in 2 files with a spec larger than the fix; a feature commit ~74 lines, a feature PR ~370. Numbers and exemplars in references/accepted-shape.md. Beyond the median, ask the maintainer's question: which of these files is the bug, and which is the refactor you took the opportunity for? Count abstractions, not lines: a wrapper class, a helper with one caller, a new module beside one that does the job, a regex where a parser exists, a third mechanism next to two. "Too complicated. Extract to a function with simple ifs and returns" is a verdict this project gives. A near-copy of an existing function is a finding whatever its size, never "acceptable" or "optional": the two drift (#11665's "Search now" handler copied the ribbon's refreshResults() and already reported the query error differently). Grep for the calls the new code makes, name the shared helper in the verdict, and list the extraction under You finish or Fixes before merge.
5. Robustness. Bad input ("Always validate user data"), a database with 100 000 notes ("my Trilium board has 581 tasks"), other themes and platforms (Windows paths, the flatpak), sync peers on an older version, a hidden loss of an existing capability (the sort dialog that lost "folders at the bottom"). Look for the branch the author did not test.
6. Spec. verify says whether the PR's tests fail without the production change and pass with it. A new test that passes both ways covers existing behavior, not this fix. A core change is proven under apps/server and apps/standalone. No spec on a covered module is a gap; a spec that is "quite a bit complicated" is a gap too — the house writes concise ones.
7. Docs and i18n. A user-facing change ships with its User Guide page in the same PR, "even if it's niche"; docs/User Guide/** Markdown and the generated doc_notes/** move together (hand-edited HTML or unsynced Markdown is a gap); only the en catalogues are edited; a new option is whitelisted, defaulted and has a control (the seven-file recipe in accepted-shape). verify runs docs.mjs impact for the diff; run it with the feature's name too.
8. Conventions. The mechanical checks at the end of accepted-shape (!, forEach, inline style, hand-rolled <input> where FormTextBox exists, localStorage, Node built-ins in core, comment style, ~10-SLOC modules). Each is a "you finish" gap. They change a verdict only in bulk, when they show the code was written beside the codebase rather than in it.

For a diff under packages/ckeditor5, load the ckeditor5-reviewing skill for the plugin-level defects; this skill still owns the verdict.

Workflow A — one PR

1. node $R dossier N. Read in this order: the linked issue and its comments (the problem, in the reporter's and the maintainer's words — if there is none, decide whether the problem is real before anything else), the commits (subject and body per commit; they land verbatim on main), the files by kind and the flags, the human review threads (what a maintainer already asked; whether it was done), the rivals, and only then the description.
2. node $R verify N. Read the report: install, typecheck, each spec's green/red result and which tests prove the change, sibling specs, dependency lines, docs impact.
3. Read the diff in the worktree, not on GitHub: git -C .claude/worktrees/pr-N diff $(git -C .claude/worktrees/pr-N merge-base HEAD origin/main) and then the changed files whole, with what surrounds them. This is where "reuse the existing helper" (useNoteLabel, isCtrlKey, FormTextBox, an existing option) and "wrong layer" are visible and a diff view hides them. For a bugfix, find the cause yourself first, then compare.
4. Run the mechanical checks from accepted-shape in the worktree that apply to the touched areas.
5. Judge the eight dimensions. Write findings ordered by weight — a fit problem first, a missing en-GB twin last.
6. Verdict, with the gaps list, and the report block below.

When the PR has a rival (dupes), do both and write one comparison: the smaller change that fixes the cause usually wins; the earlier one has no precedence by age; sometimes the right answer is a third, smaller change that neither made — say so, and name it.