diff --git a/.claude/commands/github-review-failures.md b/.claude/commands/github-review-failures.md index 558c40b..9737a9a 100644 --- a/.claude/commands/github-review-failures.md +++ b/.claude/commands/github-review-failures.md @@ -29,9 +29,11 @@ If exactly one open PR exists for the current branch, use it. If none or multipl Once you have the PR number, confirm it: ```bash -gh pr view --json title,state,url +gh pr view --json title,state,url,mergeable ``` +**Pre-flight: merge conflicts (detection only).** If `mergeable` is `CONFLICTING`, STOP — do not diagnose CI on a conflicted branch (the merge itself may fix or cause the failures). Report the conflict and hand off to `/github-review-pr`, whose Phase A0 owns the resolution runbook — this command's toolset deliberately does not include the merge machinery. If `mergeable` is `UNKNOWN`, note it and proceed: the orchestrator resolves the ambiguity; a standalone run shouldn't block on GitHub's recompute. + --- ## Phase 1: Identify Failing Checks diff --git a/.claude/commands/github-review-pr.md b/.claude/commands/github-review-pr.md index 53d28ea..4d6d65a 100644 --- a/.claude/commands/github-review-pr.md +++ b/.claude/commands/github-review-pr.md @@ -1,26 +1,29 @@ --- -description: "Use when a PR needs full review — fixes CI failures first, then addresses unresolved review comments. Run failures first because comment fixes trigger new CI runs that obscure the original failures." +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." model: opus argument-hint: "PR number (e.g., 156 or #156)" -allowed-tools: Bash(gh pr list:*), Bash(gh pr view:*), Bash(gh pr checks:*), 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 push:*), Bash(git commit:*), Bash(git add:*), Bash(bundle exec:*), Read, Write, Edit, Glob, Grep, Agent +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(bun install:*), Bash(cd:*), Read, Write, Edit, Glob, Grep, Agent --- # Review GitHub PR (full pass): $ARGUMENTS -You are running a full review pass on a pull request. The pass has two phases that MUST run in this order: +You are running a full review pass on a pull request. The pass has three phases that MUST run in this order: -1. **Phase A: CI failures** — fix anything red before touching review comments. -2. **Phase B: review comments** — only after Phase A leaves CI green (or pending green after a push). +1. **Phase A0: merge conflicts** — bring the branch up to date with its base and resolve any conflicts before anything else. +2. **Phase A: CI failures** — fix anything red before touching review comments. +3. **Phase B: review comments** — only after Phase A leaves CI green (or pending green after a push). ## Why this order matters -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: +**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. + +**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: - The failing spec 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. - A review-comment fix accidentally repairs the CI failure as a side effect, and you lose the chance to verify the failure was real. - A review-comment fix accidentally INTRODUCES a CI failure, and you can't tell whether the new failure was pre-existing or your fault. -Failures-first eliminates this confusion. CI is either green or red on a known commit; the review-comment fixes layer cleanly on top. +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. ## Phase 0: Determine the PR Number @@ -47,6 +50,49 @@ gh pr view --json title,state,url --- +## Phase A0: Merge conflicts + +Check whether the branch merges cleanly into its base: + +```bash +gh pr view --json mergeable,mergeStateStatus,baseRefName +``` + +| `mergeable` | Action | +|-------------|--------| +| `MERGEABLE` | Skip to Phase A. | +| `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 ` and `git fetch origin pull//head`, verify both refs resolve (`git rev-parse --verify origin/^{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/ 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). | +| `CONFLICTING` | Resolve, below. | + +### Resolution procedure + +1. Check out the PR's branch (`gh pr checkout `) with a clean tree (`git status`). Stash nothing — if the tree is dirty, stop and ask the user. +2. `git fetch origin ` then **`git merge origin/`** — MERGE, never rebase. The branch is shared (it has a PR); a rebase would require a force-push, which is forbidden on shared branches. +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: + - **`CHANGELOG.md` (Unreleased)**: union — keep BOTH sides' entries (main's landed bullets and this branch's), most recent first, without duplicating the Keep-a-Changelog `### Added`/`### Fixed`/`### Changed` subheads. Losing either side is a real regression reviewers rarely catch. + - **`lib/phlex_forms/version.rb`**: releases land DIRECTLY on `main` via `rake release[X.Y.Z]` (the Rakefile aborts unless the current branch IS `main` — no release PRs exist), so an ordinary feature branch never edits this file — a conflict here means the BRANCH bumped it on purpose. 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. + - **`docs/Gemfile.lock`** (the docs app's committed lockfile — the gem root's `Gemfile.lock` is gitignored and can never conflict): NEVER hand-merge lockfile hunks. The common conflict is the `phlex-forms (X.Y.Z)` path-gem pin, which `rake release` bumps directly on `main` — take the base's file (it carries the released pin). If the BRANCH deliberately changed docs dependencies, re-apply that intent by editing `docs/Gemfile` and running `cd docs && bundle install` — but note the Rakefile documents that a full re-resolve can fail on this lockfile's broad PLATFORMS list ("Could not find gems … valid for all resolution platforms"); if it fails, stop and ask rather than hand-editing the lock. + - **`docs/bun.lock`**: same principle — take the base's file; if the branch changed `docs/package.json`, re-run `cd docs && bun install` so the lock re-resolves on top. Never hand-merge. + - **Append-only registries** (`docs/app/models/doc.rb`): both sides usually appended — keep both `page` lines, in base order first. (`docs/config/routes.rb` is static docs-kit boilerplate here — pages register only in `doc.rb` and are served through the one `docs/:doc` route, so routes.rb shouldn't conflict per-page.) + - There are no tracked generated artifacts in this repo (the docs CSS builds under `docs/app/assets/builds/` are gitignored), so nothing needs a regenerate-instead-of-merge rule beyond the lockfiles above. +4. Run the verification gates BEFORE pushing the merge — scoped to what the conflict touched, at minimum: + ```bash + bundle exec rubocop lib spec # the CI Lint job's exact invocation + bundle exec rspec + # docs/ files involved (the docs app has its own .rubocop.yml — + # pass the changed files explicitly): + cd docs && bundle exec rubocop + ``` +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. + +### Phase A0 exit criteria + +- The PR reports `MERGEABLE` (or the local `git merge-tree` check is clean), AND the merge commit (if one was needed) is pushed. +- If the merge produced changes, CI is now re-running — that's expected; Phase A reads the fresh run. +- 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. + +--- + ## Phase A: Run `/github-review-failures` Invoke the existing `/github-review-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. @@ -95,18 +141,23 @@ The slash command is at `.claude/commands/github-review-comments.md`. Its workfl ## Phase C: Final report -After both phases complete, report: +Before reporting, re-check mergeability once more (`gh pr view --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. + +After all phases complete, report: -1. **Phase A summary**: which CI failures were diagnosed and fixed. Note the commit SHAs for the fixes. -2. **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). -3. **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. +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". +2. **Phase A summary**: which CI failures were diagnosed and fixed. Note the commit SHAs for the fixes. +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). +4. **End state**: final mergeability + CI status on the latest commit. +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. --- ## Important Notes - **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. -- **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. This is the only allowed reverse direction. -- **If the PR has no failures AND no unresolved comments**, report "PR is clean" and stop. +- **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. +- **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.) +- **If the PR merges cleanly, has no failures AND no unresolved comments**, report "PR is clean" and stop. - **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. - **Don't re-implement the child slash commands' logic**. Invoke them and let them do their work. This command is the orchestrator.