|
| 1 | +--- |
| 2 | +name: review-plugin |
| 3 | +description: Guides reviewing, testing, and merge-checking a third-party plugin submission or update pull request against the gephi-plugins repo, from checking out the contributor's fork branch through a final disposable-worktree merge into master-forge to catch pom.xml <modules>-list conflicts before they're real. Enforces this repo's three-branch model so master-forge and master are never pushed to directly, and gates every GitHub-visible or fork-visible action (pushes, PR comments/reviews, merges) on explicit user approval. Use whenever asked to review, test, fix, or merge-check a gephi-plugins pull request; when given a github.com/gephi/gephi-plugins PR URL or number; or when asked to resolve a plugin's pom.xml conflicts against master-forge, check whether a plugin update still builds, or work through a plugin-submission checklist — even if the user doesn't mention branches, forks, or worktrees explicitly. |
| 4 | +--- |
| 5 | + |
| 6 | +# Reviewing a gephi-plugins PR |
| 7 | + |
| 8 | +This skill is the *process* for reviewing a plugin PR: which branch to stand on, when you're allowed |
| 9 | +to push versus only allowed to suggest, and how to rehearse the merge before it's real. It |
| 10 | +deliberately does not restate *what* to check in the code — that content already lives in this |
| 11 | +repo's own docs and goes stale if duplicated here. Read from these as the relevant phase below calls |
| 12 | +for them, don't summarize them in advance: |
| 13 | + |
| 14 | +| Doc | What's there | |
| 15 | +|---|---| |
| 16 | +| `CONTRIBUTING.md` → "Reviewing a plugin submission PR" | The actual review checklist (build, pom.xml, manifest, SPI correctness, licensing, file hygiene, tests, PR completeness) | |
| 17 | +| `CONTRIBUTING.md` → "Documenting a plugin in master-forge's `<modules>` list" | Exact format for the name/origin/status comment block a new `<module>` entry needs | |
| 18 | +| `CONTRIBUTING.md` → "Maintaining a plugin after approval" | The unresponsive-fork/branch-adoption exception — a separate maintainer decision, out of scope for a routine review (see Guardrails) | |
| 19 | +| `ARCHITECTURE.md` → "Repository / branch model" | What `master` / `master-forge` / `parent-pom` are each for, and why | |
| 20 | +| `README.md` → "Submit a plugin" / "Update a plugin" | The fork+PR flow from the contributor's side, including "allow edits from maintainers" | |
| 21 | +| `AGENTS.md` | Build/test commands (`mvn clean package`, single-module builds, etc.) | |
| 22 | + |
| 23 | +## Guardrails — read before doing anything |
| 24 | + |
| 25 | +This is someone else's code, on a public OSS project, and the repo has a branch model that's easy to |
| 26 | +violate by accident. A few rules, and the reasoning behind each: |
| 27 | + |
| 28 | +- **Never push to `master` or `master-forge` directly, ever — including "just to test."** |
| 29 | + `master-forge` aggregates every plugin into one build; `build.yml` (main CI) explicitly excludes it |
| 30 | + from its triggers, so a direct push skips CI entirely and there's no PR to attribute the change to. |
| 31 | + The only legitimate way something lands on `master-forge` is a merged PR. |
| 32 | +- **Your own fix commits belong on the contributor's fork branch, not a branch in this repo.** When |
| 33 | + you have push access (see "Access check" below), you're pushing to *their* branch on *their* fork — |
| 34 | + the same branch the PR is already open from — not creating something new here. That's a different |
| 35 | + thing from the "never push to master/master-forge" rule above; don't let the two blur together. |
| 36 | + `gh pr checkout` sets this up correctly by itself. |
| 37 | +- **The branch-adoption exception (`CONTRIBUTING.md`'s "When a plugin's fork stops being a reliable |
| 38 | + source of truth") is not part of a routine review.** If a review surfaces an unresponsive |
| 39 | + maintainer, flag it and stop — adopting a plugin onto a maintained branch is a separate, deliberate |
| 40 | + decision the user makes explicitly, not something to fold into fixing a PR. |
| 41 | +- **Prefer suggesting over rewriting.** Even with push access, a substantial rewrite of someone else's |
| 42 | + logic isn't yours to make unilaterally — reserve direct pushes for mechanical fixes (a version bump, |
| 43 | + a pom.xml conflict resolution, a checklist item like a missing license file) and leave anything |
| 44 | + touching the plugin's actual behavior as a review comment for the contributor to act on. |
| 45 | +- **Confirm with the user before anything that leaves your machine or is hard to undo**: pushing a |
| 46 | + commit anywhere (including to the fork, even with access), posting a PR comment or review, approving |
| 47 | + or requesting changes, or merging. Checking out, building, and the worktree merge rehearsal in Phase |
| 48 | + 2 are local and disposable, so you can move through those without stopping at every step — but say |
| 49 | + what you're about to do before you build code from someone's fork, since that means running their |
| 50 | + Maven/Java code on this machine, not a sandboxed CI runner. |
| 51 | +- **The Phase 2 worktree merge is a rehearsal, never a delivery.** Nothing from it gets pushed anywhere |
| 52 | + under any circumstances — it exists purely to prove the PR *would* merge cleanly (or to work out |
| 53 | + what the conflict resolution should look like) and gets deleted afterward win or lose. |
| 54 | + |
| 55 | +## Access check — which mode are you in? |
| 56 | + |
| 57 | +Before touching anything, find out whether you can actually push fixes or only suggest them: |
| 58 | + |
| 59 | +``` |
| 60 | +gh pr view <N> --repo gephi/gephi-plugins --json baseRefName,headRefName,headRepositoryOwner,headRepository,maintainerCanModify,mergeable |
| 61 | +``` |
| 62 | + |
| 63 | +- `baseRefName` should be `master-forge`. If it's `master` instead, that's itself a review finding |
| 64 | + (see `CONTRIBUTING.md`'s "PR completeness" section) — flag it to the user before going further, |
| 65 | + since the rest of this workflow assumes the PR targets `master-forge`. |
| 66 | +- `maintainerCanModify: true` means the contributor enabled "allow edits from maintainers" and you can |
| 67 | + push fix commits to their branch. `false` means **comment-only mode**: you can build, test, and work |
| 68 | + through the checklist locally, but any fix becomes a suggestion (a review comment, or a `gh pr |
| 69 | + review` with a diff suggestion) rather than something you push. |
| 70 | +- `mergeable: CONFLICTING` is common and expected — it usually just means `master-forge`'s |
| 71 | + `<modules>` list has moved since the fork branched off. That's exactly what Phase 2 works out. |
| 72 | +- Tell the user which mode you're in and what `mergeable` says before starting Phase 1, so they're not |
| 73 | + surprised later when a fix turns into "here's a comment" instead of "here's a push." |
| 74 | + |
| 75 | +## Phase 1 — Isolated review and test |
| 76 | + |
| 77 | +Work entirely on the contributor's own branch here; nothing in this phase involves `master-forge`. |
| 78 | + |
| 79 | +1. `gh pr checkout <N>` — this fetches the fork, adds a remote for it if needed, and checks out a |
| 80 | + local branch tracking the contributor's branch directly. This *is* "the source branch" the plugin's |
| 81 | + commits live in — anything you commit here, if pushed, goes back to their fork. |
| 82 | +2. Work through `CONTRIBUTING.md`'s "Reviewing a plugin submission PR" checklist against this branch, |
| 83 | + including actually running `mvn clean package` from the repo root per `AGENTS.md` rather than |
| 84 | + trusting a green PR description. Since only this contributor's plugin(s) are added to `pom.xml` on |
| 85 | + their branch, this build is naturally isolated from every other plugin. |
| 86 | +3. Report findings against specific named checklist items, per `CONTRIBUTING.md`'s own instruction — |
| 87 | + not general impressions. |
| 88 | +4. For anything you and the user agree is worth fixing directly (see Guardrails on what's in scope for |
| 89 | + a direct push): |
| 90 | + - **`maintainerCanModify: true`**: commit on this branch, confirm the diff with the user, then push |
| 91 | + to the fork's remote (the one `gh pr checkout` set up) — never to a remote or branch in |
| 92 | + `gephi/gephi-plugins` itself. |
| 93 | + - **`maintainerCanModify: false`**: don't commit anything. Draft the fix as a PR comment or a `gh pr |
| 94 | + review` suggestion instead, and only post it once the user says to. |
| 95 | + |
| 96 | +## Phase 2 — Integration check: does it still merge into master-forge? |
| 97 | + |
| 98 | +This is the final gate, and it's a rehearsal — it happens in a disposable worktree outside your normal |
| 99 | +checkout, never in the branch you were just reviewing on and never by touching the real |
| 100 | +`master-forge` branch. |
| 101 | + |
| 102 | +1. Fetch the current state: `git fetch origin master-forge`. |
| 103 | +2. Create a throwaway worktree off it, in your scratchpad directory rather than inside this checkout: |
| 104 | + `git worktree add <scratchpad>/<pr-number>-integration-check origin/master-forge`. |
| 105 | +3. In that worktree, merge the branch you reviewed in Phase 1: `git merge <local-pr-branch>`. Expect |
| 106 | + git to report the *entire* `<modules>` list as one conflict block, not a small localized one — it |
| 107 | + can't tell the two sides only touched different entries, since both diverged from the same |
| 108 | + near-empty list on `master`. The resolution is still mechanical: take `master-forge`'s (`HEAD`) |
| 109 | + copy of the list wholesale and add this plugin's own name/origin/status comment block plus |
| 110 | + `<module>` line into it, per `CONTRIBUTING.md`'s documentation-convention section (link above) — |
| 111 | + don't try to hand-merge the conflict markers line by line, and don't improvise the block's format |
| 112 | + from memory. |
| 113 | + - Only treat a conflict *outside* that list as worth surfacing on its own — but check what it |
| 114 | + actually is before calling it a problem: compare against `master-forge`, not `master`. `master` |
| 115 | + (the template) and `master-forge` can legitimately disagree on shared settings like |
| 116 | + `gephi.version` or root build-plugin versions — `master` moving ahead doesn't mean |
| 117 | + `master-forge` is due for the same bump, that's a deliberate, separate maintainer decision (see |
| 118 | + `CONTRIBUTING.md`/`ARCHITECTURE.md` on the branch model). If you see a difference here, flag it |
| 119 | + as a fact for the user to weigh in on rather than resolving it by picking either side. |
| 120 | +4. Build from the worktree. A full `mvn clean package` from the root is the authoritative check — it |
| 121 | + validates and builds *everything* currently on `master-forge`, not just this plugin — but that |
| 122 | + means every other plugin already merged there, so budget real time for it. For a fast first pass, |
| 123 | + `mvn -pl modules/<Folder> -am clean package` proves the plugin itself still builds against the |
| 124 | + merged parent/config without paying for the whole reactor. |
| 125 | +5. Report the result (clean merge or not, build passed or not, what the conflict resolution looked |
| 126 | + like, and anything you flagged instead of resolving) to the user. Then remove the worktree |
| 127 | + regardless of outcome — `git worktree remove <path>` (add `--force` if the merge left it dirty) — |
| 128 | + since nothing in it is meant to persist. If the user wants the actual conflict resolution applied |
| 129 | + for real, that's a Phase 1 fix-and-push (or a comment, in comment-only mode) using what you just |
| 130 | + worked out here, not something you push from the worktree itself. |
| 131 | + |
| 132 | +## Wrap-up |
| 133 | + |
| 134 | +Summarize for the user: checklist findings, which mode you were in, whether the integration check |
| 135 | +passed, and exactly what (if anything) you pushed or posted and where. If nothing was pushed or |
| 136 | +posted, say so explicitly — "reviewed and tested, nothing sent anywhere yet" is a normal, complete |
| 137 | +outcome for a comment-only-mode review. |
0 commit comments