|
| 1 | +--- |
| 2 | +description: "Use when a PR needs full review — resolves merge conflicts with the base first, then fixes CI failures, then addresses unresolved review comments. Conflicts first so CI diagnoses the post-merge reality; failures before comments because comment fixes trigger new CI runs that obscure the original failures." |
| 3 | +model: opus |
| 4 | +argument-hint: "PR number (e.g., 156 or #156)" |
| 5 | +allowed-tools: Bash(gh pr list:*), Bash(gh pr view:*), Bash(gh pr checks:*), Bash(gh pr checkout:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(gh run view:*), Bash(git log:*), Bash(git blame:*), Bash(git diff:*), Bash(git status:*), Bash(git switch:*), Bash(git fetch:*), Bash(git merge:*), Bash(git merge-tree:*), Bash(git rev-parse:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(bundle exec:*), Bash(bundle install:*), Bash(bin/rubocop:*), Bash(bun:*), Bash(cd:*), Read, Write, Edit, Glob, Grep, Agent |
| 6 | +--- |
| 7 | + |
| 8 | +# Review GitHub PR (full pass): $ARGUMENTS |
| 9 | + |
| 10 | +You are running a full review pass on a pull request. The pass has three phases that MUST run in this order: |
| 11 | + |
| 12 | +1. **Phase A0: merge conflicts** — bring the branch up to date with its base and resolve any conflicts before anything else. |
| 13 | +2. **Phase A: CI failures** — fix anything red before touching review comments. |
| 14 | +3. **Phase B: review comments** — only after Phase A leaves CI green (or pending green after a push). |
| 15 | + |
| 16 | +## Why this order matters |
| 17 | + |
| 18 | +**Conflicts before failures**: CI results only matter for the code that will actually merge. On a conflicted (or stale) branch you'd diagnose failures against a base that no longer exists — and the conflict resolution itself changes code, invalidating the run you just fixed. Resolving conflicts first means Phase A reads CI for the post-merge reality, and you spend exactly one extra CI cycle instead of two. |
| 19 | + |
| 20 | +**Failures before comments**: if you fix review comments first, every commit pushes a new CI run. By the time the review-comment fixes finish, the original failure logs are buried under new pipeline runs. Symptoms: |
| 21 | + |
| 22 | +- The "Gem Tests (Ruby 3.4) failed" log you needed to read is now from a stale run; the latest run is still in progress on top of your unrelated comment fixes. |
| 23 | +- A review-comment fix accidentally repairs the CI failure as a side effect, and you lose the chance to verify the failure was real. |
| 24 | +- A review-comment fix accidentally INTRODUCES a CI failure, and you can't tell whether the new failure was pre-existing or your fault. |
| 25 | + |
| 26 | +Conflicts-first, then failures-first eliminates this confusion. CI is either green or red on a known commit against the current base; the review-comment fixes layer cleanly on top. |
| 27 | + |
| 28 | +## Phase 0: Determine the PR Number |
| 29 | + |
| 30 | +The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: |
| 31 | + |
| 32 | +- `PR156`, `PR 156`, `pr156` → PR 156 |
| 33 | +- `156` → PR 156 |
| 34 | +- `#156` → PR 156 |
| 35 | +- Empty/blank → auto-detect from current branch |
| 36 | + |
| 37 | +**If no PR number is provided**, detect it automatically: |
| 38 | + |
| 39 | +```bash |
| 40 | +gh pr list --author=@me --head="$(git branch --show-current)" --state=open --json number,title |
| 41 | +``` |
| 42 | + |
| 43 | +If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. |
| 44 | + |
| 45 | +Once you have the PR number, confirm it: |
| 46 | + |
| 47 | +```bash |
| 48 | +gh pr view <PR_NUMBER> --json title,state,url |
| 49 | +``` |
| 50 | + |
| 51 | +--- |
| 52 | + |
| 53 | +## Phase A0: Merge conflicts |
| 54 | + |
| 55 | +Check whether the branch merges cleanly into its base: |
| 56 | + |
| 57 | +```bash |
| 58 | +gh pr view <PR_NUMBER> --json mergeable,mergeStateStatus,baseRefName |
| 59 | +``` |
| 60 | + |
| 61 | +| `mergeable` | Action | |
| 62 | +|-------------|--------| |
| 63 | +| `MERGEABLE` | Skip to Phase A. | |
| 64 | +| `UNKNOWN` | GitHub is recomputing (common right after pushes, and it can stay UNKNOWN for minutes). Don't poll it — verify **locally**, against the PR's actual head (NOT `HEAD`, which may be some other checked-out branch): `git fetch origin <base>` and `git fetch origin pull/<PR>/head`, verify both refs resolve (`git rev-parse --verify origin/<base>^{commit}` and `git rev-parse --verify FETCH_HEAD^{commit}` — a bad ref also exits 1 from merge-tree, so exit code alone can't be trusted), then `git merge-tree --write-tree --name-only origin/<base> FETCH_HEAD`. Clean exit → no conflicts, skip to Phase A. Exit 1 **with conflict output** → resolve below (the `--name-only` file list is your work list). | |
| 65 | +| `CONFLICTING` | Resolve, below. | |
| 66 | + |
| 67 | +### Resolution procedure |
| 68 | + |
| 69 | +1. Check out the PR's branch (`gh pr checkout <PR_NUMBER>`) with a clean tree (`git status`). Stash nothing — if the tree is dirty, stop and ask the user. |
| 70 | +2. `git fetch origin <base>` then **`git merge origin/<base>`** — MERGE, never rebase. The branch is shared (it has a PR); a rebase would require a force-push, which `.claude/rules/git-workflow.md` forbids on shared branches. |
| 71 | +3. Resolve every conflicted file **semantically** — read both sides and produce the version that preserves BOTH changes' intent. Never blanket `--ours`/`--theirs` a source file. Repo-specific rules: |
| 72 | + - **Lockfiles** (`Gemfile.lock`, `docs/Gemfile.lock`, `docs/bun.lock` — all three are tracked): NEVER hand-merge a lockfile. Take the base's file, then re-resolve the branch's own dependency changes on top: `bundle install` at the root (and/or in `docs/`), `bun install` in `docs/` for `bun.lock`. Only the regenerated file gets committed. |
| 73 | + - **`CHANGELOG.md` (Unreleased)**: union — keep BOTH sides' entries (main's landed bullets and this branch's), most recent first, without duplicating the `### Added`/`### Changed` subheads. Losing either side is a real regression reviewers rarely catch. |
| 74 | + - **`lib/daisy_ui/version.rb`**: releases land DIRECTLY on `main` via `rake release[X.Y.Z]` (the task aborts off-main and pushes to `origin/main` itself — no PR), so an ordinary feature branch never edits this file — a conflict here means the BRANCH bumped it on purpose (a release-prep PR). Keep the branch's bump in that case; if the intent isn't obvious from the branch's own commits, stop and ask. Only take the base's version when the branch's edit was clearly accidental. |
| 75 | + - **`lib/daisy_ui/updated_at.rb`**: machine-written by `rake release` on every release — take the base's side; it is regenerated at the next release regardless. |
| 76 | + - **Component modifier maps** (`register_modifiers` blocks in `lib/daisy_ui/*.rb`): both sides usually added different modifiers — keep both, and preserve the responsive variant comment block (six lines per modifier, including the container-query variants: `# "sm:..."`, `# "@sm:..."`, `# "md:..."`, `# "@md:..."`, `# "lg:..."`, `# "@lg:..."`) above EVERY modifier line. Tailwind scans those comments to generate responsive classes; dropping one in a merge silently breaks responsive variants. |
| 77 | + - **Generated assets**: nothing generated is tracked in this repo (docs CSS builds and `tailwind.sources.css` are gitignored) — so there is no artifact to regenerate-instead-of-merge; every remaining conflict is source and merges semantically. |
| 78 | +4. Run the verification gates BEFORE pushing the merge — scoped to what the conflict touched, at minimum: |
| 79 | + ```bash |
| 80 | + bundle exec rubocop lib spec |
| 81 | + bundle exec rspec |
| 82 | + # docs/ files involved (the docs app has its own lint + test setup): |
| 83 | + cd docs && bin/rubocop && bun run lint:js && bun run lint:css |
| 84 | + cd docs && bun run build:css && bundle exec rspec |
| 85 | + ``` |
| 86 | +5. Commit the merge (keep git's standard merge-commit message; add a body line naming any non-obvious resolution choice) and `git push` — a merge commit never needs force. |
| 87 | + |
| 88 | +### Phase A0 exit criteria |
| 89 | + |
| 90 | +- The PR reports `MERGEABLE` (or the local `git merge-tree` check is clean), AND the merge commit (if one was needed) is pushed. |
| 91 | +- If the merge produced changes, CI is now re-running — that's expected; Phase A reads the fresh run. |
| 92 | +- If a conflict cannot be resolved with confidence (both sides rewrote the same logic and the correct combination isn't decidable from the code), **stop and ask the user** — a guessed resolution that compiles is worse than a question. |
| 93 | + |
| 94 | +--- |
| 95 | + |
| 96 | +## Phase A: Run `/github-ci-failures` |
| 97 | + |
| 98 | +Invoke the existing `/github-ci-failures` slash command with the same `$ARGUMENTS` value. Its purpose: fix every failing CI check, push, leave the branch in a state where CI is either green or running-pending-toward-green. |
| 99 | + |
| 100 | +Follow that command's full process — phases 1–6 of the failures runbook. The slash command is at `.claude/commands/github-ci-failures.md`. Its workflow: |
| 101 | + |
| 102 | +1. Identify failing checks via `gh pr checks <PR>`. |
| 103 | +2. Fetch failure logs. |
| 104 | +3. Diagnose root cause for each. |
| 105 | +4. Fix locally — lint first (fast, deterministic), then specs, then docs tests. |
| 106 | +5. Verify locally before commit (`bundle exec rspec <files>`, `bundle exec rubocop`). |
| 107 | +6. Commit + push + report which checks are now running. |
| 108 | + |
| 109 | +### Phase A exit criteria |
| 110 | + |
| 111 | +Before moving to Phase B, one of these must be true: |
| 112 | + |
| 113 | +- All CI checks are green on the latest pushed commit. OR |
| 114 | +- All CI checks are pending (running) on the latest pushed commit, AND no checks failed in the most recent completed run on this commit. OR |
| 115 | +- A persistent CI failure exists that is **not caused by changes on this branch** (e.g., a flaky Playwright docs test on `main`, or an environmental failure like a browser-install timeout). Report this explicitly and proceed to Phase B with the caveat noted. |
| 116 | + |
| 117 | +If failures persist on this branch's changes, **do NOT proceed to Phase B**. Report what's still failing, what's been tried, and ask the user how to proceed. |
| 118 | + |
| 119 | +--- |
| 120 | + |
| 121 | +## Phase B: Run `/github-review-comments` |
| 122 | + |
| 123 | +Once Phase A's exit criteria are met, invoke `/github-review-comments` with the same `$ARGUMENTS`. Its purpose: address every unresolved review thread on the PR, push fixes, reply with commit SHAs, and resolve the threads. |
| 124 | + |
| 125 | +The slash command is at `.claude/commands/github-review-comments.md`. Its workflow: |
| 126 | + |
| 127 | +1. Fetch all unresolved review threads via the GitHub GraphQL API. |
| 128 | +2. Read and categorise each comment (valid fix / invalid suggestion / unclear), verifying against the DaisyUI 5 spec where relevant. |
| 129 | +3. Implement accepted fixes; verify locally (specs, rubocop). |
| 130 | +4. Commit all fixes together with a clear message; push. |
| 131 | +5. Reply to every thread with the commit SHA (for accepted fixes) or technical reasoning (for rejections). |
| 132 | +6. Resolve each thread via the GraphQL `resolveReviewThread` mutation. |
| 133 | +7. Verify no unresolved threads remain. |
| 134 | + |
| 135 | +### Phase B exit criteria |
| 136 | + |
| 137 | +- All unresolved review threads have been replied to and resolved (or the user has explicitly approved leaving a specific thread open). |
| 138 | +- The branch has been pushed with all accepted fixes. |
| 139 | + |
| 140 | +--- |
| 141 | + |
| 142 | +## Phase C: Final report |
| 143 | + |
| 144 | +Before reporting, re-check mergeability once more (`gh pr view <PR> --json mergeable`, or the local `git merge-tree` check if UNKNOWN) — the base can move underneath a long pass. If a NEW conflict appeared, loop back to Phase A0. |
| 145 | + |
| 146 | +After all phases complete, report: |
| 147 | + |
| 148 | +1. **Phase A0 summary**: whether the branch was conflicted, which files conflicted, how each was resolved (and the merge commit SHA) — or "clean merge, no action". |
| 149 | +2. **Phase A summary**: which CI failures were diagnosed and fixed. Note the commit SHAs for the fixes. |
| 150 | +3. **Phase B summary**: which review comments were accepted (with commit SHAs), which were pushed back on (with reasoning), and the final unresolved-thread count (should be 0). |
| 151 | +4. **End state**: final mergeability + CI status on the latest commit. |
| 152 | +5. **Outstanding work**: anything that still needs attention — e.g., CI was pending at the end of Phase B and the user should verify the latest run after the comment fixes. |
| 153 | + |
| 154 | +--- |
| 155 | + |
| 156 | +## Important Notes |
| 157 | + |
| 158 | +- **Do not interleave the phases.** Don't fix a CI failure, then a review comment, then another CI failure. The whole point of this command is the strict ordering. |
| 159 | +- **A new CI failure emerging during Phase B** (e.g., a comment fix breaks a spec) means looping back to Phase A — fix the new failure before continuing comment work. Likewise, **a new conflict appearing mid-pass** (the base moved) means looping back to Phase A0. These loop-backs are the only allowed reverse directions. |
| 160 | +- **If the PR is already merged**, there is nothing to review — report that and stop. (A stale `$ARGUMENTS` or a just-merged PR shows up as `state: MERGED` in Phase 0's confirm step.) |
| 161 | +- **If the PR merges cleanly, has no failures AND no unresolved comments**, report "PR is clean" and stop. |
| 162 | +- **If `$ARGUMENTS` is the same as the current open PR**, the two child slash commands will see the same PR. They share state through the git branch and the GitHub API, not through any in-process variable. |
| 163 | +- **Don't re-implement the child slash commands' logic**. Invoke them and let them do their work. This command is the orchestrator. |
0 commit comments