Review Fork PR

Review a pull request from a fork

A contributor works in their own fork of os8088 and opens a PR against jggonz/os8088:main. It is usually large (98 files and +27,500 lines was PR #92; 193 files and +73,000 was #80), usually long-lived — cut from a main that has since moved — and often conflicting. Your job is the maintainer's: understand it, merge main into it, find what would break real hardware, fix that, verify it, push the fixes back to the contributor's branch on their fork so the PR itself becomes mergeable, and explain every change on the PR in language a semi-technical reader can follow.

Invoking this skill authorises the multi-agent orchestration below — the Agent fan-out and the two Workflow scripts (what the user calls "ultracode"). Every decision that is the user's comes back through AskUserQuestion; everything else you decide and record.

Three files travel with this skill:

Also binding, and already in the tree: docs/UPSTREAM.md — the fork topology, the squash cycle, the shallow-clone trap and the merge-resolution rules. It is written from the contributor's side; this skill is the same cycle seen from main. Read its "Merging upstream in" and "The PR back to main" sections before step 4. Then CLAUDE.md (hard rules, performance table), SPEC.md (the binding contract), CONTRIBUTING.md (what the repo asks of a contribution).

Reviews were sized on Opus 5 and Fable 5. On a smaller model, run the review workflow with fewer, narrower units rather than trusting a wide one.

1. Which PR, and what is it

The PR number may arrive as the argument (/review-fork-pr 92). If not, list what is open and ask:

gh pr list --repo jggonz/os8088 --state open \
   --json number,title,author,headRepositoryOwner,headRefName,changedFiles

Then get the facts before forming any opinion — the topology decides what you are even able to do:

gh pr view <N> --json title,body,author,headRepository,headRepositoryOwner,headRefName,\
baseRefName,state,mergeable,mergeStateStatus,maintainerCanModify,additions,deletions,changedFiles,commits
gh api repos/jggonz/os8088/pulls/<N> --jq \
   '{maintainer_can_modify, head_repo:.head.repo.full_name, head_ref:.head.ref, head_sha:.head.sha}'
gh pr view <N> --json comments --jq '.comments[] | "=== " + .author.login + " ===\n" + .body'

State the shape back to the user in two or three lines (who, what, how big, how far behind main, what conflicts) before you go further.

2. Preflight

git rev-parse --is-shallow-repository        # true ⇒ every ancestry answer below is a lie
git fetch --unshallow                        # ...so do this FIRST (docs/UPSTREAM.md rule 0)
git status --short                           # the user's tree must stay untouched
pgrep -fl qemu-system                        # a stale QEMU answers on build/qmp.sock with an OLD image
gh auth status

Read LESSONS.md now, in full.

Fetch the PR and work in a scratch worktree, never in the user's checkout:

git fetch origin pull/<N>/head:pr-<N>
git worktree add -q "$SCRATCH/wt" pr-<N>          # $SCRATCH = the session scratchpad
git remote add <owner> git@github.com:<owner>/os8088.git 2>/dev/null || true
git fetch <owner> <headRef>

The worktree is where every build, boot, edit and commit happens. The user keeps a working tree they can use while the review runs, and step 10 hands them the branch when it is over. One caveat that has cost a session: a scratchpad path is long, and QEMU's QMP socket is an AF_UNIX path with a ~104-byte limit — boot with a short socket path (/tmp/q<N>.sock), or build in the worktree and boot from a short BUILD= directory.

Then measure the gap:

BASE=$(git merge-base main pr-<N>)
git log --oneline $BASE..main                       # what main gained since the fork point
git diff --stat $BASE pr-<N> | tail -40
git diff --name-status $BASE pr-<N> | awk '{print $1}' | sort | uniq -c   # A/M/D counts
git diff --diff-filter=D --name-only $BASE...pr-<N>  # DELETIONS - read every one

Deletions are the highest-yield list on the whole PR. A long-lived fork branch loses things main added, and git reports no conflict for a file the branch simply never received (LESSONS.md §2).

3. Ask the user what shape this run takes

One AskUserQuestion call, four questions, recommendation first:

1. Where do the fixes land?
   • Push to the contributor's branch on their fork (Recommended) — the PR itself updates; requires maintainer_can_modify
   • A new integration PR on our repo, which I merge myself — the PR #47 shape: their work plus your fixes, one PR you own
   • Local branch only — review and fix, push nothing; the user tests by hand
2. Merge main into the PR branch first?
   • Yes (Recommended) when main has moved — otherwise you are reviewing code against a base that no longer exists
   • No — review as submitted
3. What should the review weigh hardest? (multiSelect; default all) memory safety · lost-from-main regressions · redraw/CPU cost against the 4.77 MHz budget · disk and file-format integrity · the package/driver ABI
4. Stop for your approval after the review?
   • Yes — plan first, change nothing until I say go (Recommended)
   • No — review, fix and push in one pass

Default to plan-first. It is what the last several runs asked for, and on a PR this size the plan is the deliverable the user actually reads.

4. Merge main in — and find what git cannot see

In the worktree:

git merge --no-commit --no-ff main
git diff --name-only --diff-filter=U        # the textual conflicts

Resolve per file with docs/UPSTREAM.md's defaults: ours for what the branch deliberately changed, theirs for what it merely lacks, theirs for gratuitous divergence. Never blanket --ours; for a big file use that document's difflib script to list every line main added that your side lacks.

Then the four checks git merges cleanly and reports nothing about:

grep -oE '^#+ [0-9.]+' SPEC.md | sort | uniq -d      # DUPLICATE § headings - checkdocs cannot see these
git ls-files build | wc -l                           # must be 0 (SPEC.md 16)
git ls-files | grep -E '^\.claude/|\.(bin|o88|img|mod|drv)$'   # what rode in that should not have
python3 tools/checkdocs.py

• Duplicate § numbers are the standard collision. main's §67 C toolchain met the branch's §67 Cyclone 88 in #92; main's §52 ModPlug met the branch's §52 HDD before that. The branch's numbering wins at the squash, so the incoming-from-main section renumbers, and every reference to it moves with it — scope the rewrite to the lines main added (git diff $BASE <main-commit>), not to the whole file. Record the renumbering in docs/UPSTREAM.md beside the existing precedents.
• Compare API slot addresses mechanically — a slot that changed meaning assembles cleanly and calls the wrong routine (LESSONS.md §3).
• Commit the merge on its own, with a message that says what collided and how it was resolved. Fixes go in a separate commit.

Now build both kernels and boot once, before reviewing, so the reviewers argue about a tree that works: make, make KERN_SMALL=1 (or make small), make test + a screendump, plus whatever gate the feature owns (tests/heapfrag, make zcheck, make rczex, …).

5. Review — split it into units, verify every finding

Under ~30 changed files: spawn 3–5 read-only Agents in one message, one per subsystem, each pointed at the worktree with "do NOT modify any files", each told to read CLAUDE.md first and to ground every finding in a line it actually read. Then confirm the top findings yourself by reading the code.

Larger: run the workflow.

Workflow({ scriptPath: "<abs repo>/.claude/skills/review-fork-pr/workflows/review.js",
           args: { wt: "<abs worktree>", pr: <N>, base: "<merge-base sha>",
                   emphasis: ["memory", "regression", "redraw", "disk", "abi"],
                   units: [ { key: "kernel-core", title: "...", paths: "kernel/memory.inc ...",
                              focus: "..." }, ... ] } })

You write the units — that is the part no agent can do for you. Split by subsystem and by lens, not by file count: kernel/wm.inc alone has been 4,200 changed lines and wants two reviewers, one for window state and one for painting, each told the other exists so they do not duplicate. Every unit's focus should name what the PR body itself admits is unfinished, what it deleted, and which § it claims to obey. Aim for 8–14 units; the workflow adversarially verifies each finding and returns only what survived, with a CONFIRMED / REFUTED / UNCERTAIN verdict and a corrected fix.

While the agents run, do the mechanical checks yourself: the slot table address↔name cross-check, git log main -- <file> on every deleted hunk, the kernel size guards, and a boot. Two independent reviewers landing on the same defect is the strongest signal you will get; read the code yourself for anything you are about to call a blocker.