review-plugin

Imported from gephi/gephi-plugins (.claude/skills/review-plugin/SKILL.md). Install upstream with npx skills add gephi/gephi-plugins --skill review-plugin. Copyright stays with the author.

Reviewing a gephi-plugins PR

This skill is the process for reviewing a plugin PR: which branch to stand on, when you're allowed to push versus only allowed to suggest, and how to rehearse the merge before it's real. It deliberately does not restate what to check in the code — that content already lives in this repo's own docs and goes stale if duplicated here. Read from these as the relevant phase below calls for them, don't summarize them in advance:

Guardrails — read before doing anything

This is someone else's code, on a public OSS project, and the repo has a branch model that's easy to violate by accident. A few rules, and the reasoning behind each:

• Never push to master or master-forge directly, ever — including "just to test." master-forge aggregates every plugin into one build; build.yml (main CI) explicitly excludes it from its triggers, so a direct push skips CI entirely and there's no PR to attribute the change to. The only legitimate way something lands on master-forge is a merged PR.
• Your own fix commits belong on the contributor's fork branch, not a branch in this repo. When you have push access (see "Access check" below), you're pushing to their branch on their fork — the same branch the PR is already open from — not creating something new here. That's a different thing from the "never push to master/master-forge" rule above; don't let the two blur together. gh pr checkout sets this up correctly by itself.
• The branch-adoption exception (CONTRIBUTING.md's "When a plugin's fork stops being a reliable source of truth") is not part of a routine review. If a review surfaces an unresponsive maintainer, flag it and stop — adopting a plugin onto a maintained branch is a separate, deliberate decision the user makes explicitly, not something to fold into fixing a PR.
• Prefer suggesting over rewriting. Even with push access, a substantial rewrite of someone else's logic isn't yours to make unilaterally — reserve direct pushes for mechanical fixes (a version bump, a pom.xml conflict resolution, a checklist item like a missing license file) and leave anything touching the plugin's actual behavior as a review comment for the contributor to act on.
• Confirm with the user before anything that leaves your machine or is hard to undo: pushing a commit anywhere (including to the fork, even with access), posting a PR comment or review, approving or requesting changes, or merging. Checking out, building, and the worktree merge rehearsal in Phase 2 are local and disposable, so you can move through those without stopping at every step — but say what you're about to do before you build code from someone's fork, since that means running their Maven/Java code on this machine, not a sandboxed CI runner.
• The Phase 2 worktree merge is a rehearsal, never a delivery. Nothing from it gets pushed anywhere under any circumstances — it exists purely to prove the PR would merge cleanly (or to work out what the conflict resolution should look like) and gets deleted afterward win or lose.

Access check — which mode are you in?

Before touching anything, find out whether you can actually push fixes or only suggest them:

gh pr view <N> --repo gephi/gephi-plugins --json baseRefName,headRefName,headRepositoryOwner,headRepository,maintainerCanModify,mergeable

• baseRefName should be master-forge. If it's master instead, that's itself a review finding (see CONTRIBUTING.md's "PR completeness" section) — flag it to the user before going further, since the rest of this workflow assumes the PR targets master-forge.
• maintainerCanModify: true means the contributor enabled "allow edits from maintainers" and you can push fix commits to their branch. false means comment-only mode: you can build, test, and work through the checklist locally, but any fix becomes a suggestion (a review comment, or a gh pr review with a diff suggestion) rather than something you push.
• mergeable: CONFLICTING is common and expected — it usually just means master-forge's <modules> list has moved since the fork branched off. That's exactly what Phase 2 works out.
• Tell the user which mode you're in and what mergeable says before starting Phase 1, so they're not surprised later when a fix turns into "here's a comment" instead of "here's a push."

Phase 1 — Isolated review and test

Work entirely on the contributor's own branch here; nothing in this phase involves master-forge.

1. gh pr checkout <N> — this fetches the fork, adds a remote for it if needed, and checks out a local branch tracking the contributor's branch directly. This is "the source branch" the plugin's commits live in — anything you commit here, if pushed, goes back to their fork.
2. Work through CONTRIBUTING.md's "Reviewing a plugin submission PR" checklist against this branch, including actually running mvn clean package from the repo root per AGENTS.md rather than trusting a green PR description. Since only this contributor's plugin(s) are added to pom.xml on their branch, this build is naturally isolated from every other plugin.
3. Report findings against specific named checklist items, per CONTRIBUTING.md's own instruction — not general impressions.
4. For anything you and the user agree is worth fixing directly (see Guardrails on what's in scope for a direct push):
   • maintainerCanModify: true: commit on this branch, confirm the diff with the user, then push to the fork's remote (the one gh pr checkout set up) — never to a remote or branch in gephi/gephi-plugins itself.
   • maintainerCanModify: false: don't commit anything. Draft the fix as a PR comment or a gh pr review suggestion instead, and only post it once the user says to.

Phase 2 — Integration check: does it still merge into master-forge?

This is the final gate, and it's a rehearsal — it happens in a disposable worktree outside your normal checkout, never in the branch you were just reviewing on and never by touching the real master-forge branch.

1. Fetch the current state: git fetch origin master-forge.
2. Create a throwaway worktree off it, in your scratchpad directory rather than inside this checkout: git worktree add <scratchpad>/<pr-number>-integration-check origin/master-forge.
3. In that worktree, merge the branch you reviewed in Phase 1: git merge <local-pr-branch>. Expect git to report the entire <modules> list as one conflict block, not a small localized one — it can't tell the two sides only touched different entries, since both diverged from the same near-empty list on master. The resolution is still mechanical: take master-forge's (HEAD) copy of the list wholesale and add this plugin's own name/origin/status comment block plus <module> line into it, per CONTRIBUTING.md's documentation-convention section (link above) — don't try to hand-merge the conflict markers line by line, and don't improvise the block's format from memory.
   • Only treat a conflict outside that list as worth surfacing on its own — but check what it actually is before calling it a problem: compare against master-forge, not master. master (the template) and master-forge can legitimately disagree on shared settings like gephi.version or root build-plugin versions — master moving ahead doesn't mean master-forge is due for the same bump, that's a deliberate, separate maintainer decision (see CONTRIBUTING.md/ARCHITECTURE.md on the branch model). If you see a difference here, flag it as a fact for the user to weigh in on rather than resolving it by picking either side.
4. Build from the worktree. A full mvn clean package from the root is the authoritative check — it validates and builds everything currently on master-forge, not just this plugin — but that means every other plugin already merged there, so budget real time for it. For a fast first pass, mvn -pl modules/<Folder> -am clean package proves the plugin itself still builds against the merged parent/config without paying for the whole reactor.
5. Report the result (clean merge or not, build passed or not, what the conflict resolution looked like, and anything you flagged instead of resolving) to the user. Then remove the worktree regardless of outcome — git worktree remove <path> (add --force if the merge left it dirty) — since nothing in it is meant to persist. If the user wants the actual conflict resolution applied for real, that's a Phase 1 fix-and-push (or a comment, in comment-only mode) using what you just worked out here, not something you push from the worktree itself.

Wrap-up

Summarize for the user: checklist findings, which mode you were in, whether the integration check passed, and exactly what (if anything) you pushed or posted and where. If nothing was pushed or posted, say so explicitly — "reviewed and tested, nothing sent anywhere yet" is a normal, complete outcome for a comment-only-mode review.