From 9cfb50ec1373f765cef222bfd90176859cd71e8d Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Thu, 9 Jul 2026 10:47:15 +0200 Subject: [PATCH 1/2] =?UTF-8?q?chore:=20review=20workflow=20supports=20mer?= =?UTF-8?q?ge=20conflicts=20=E2=80=94=20Phase=20A0=20before=20CI=20failure?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backport of mhenrixon/phlex-reactive#220, adapted to this repo: - New /github-review-pr orchestrator: Phase A0 (merge conflicts) runs before CI failures, which run before review comments. Detection via gh pr view --json mergeable with a local git merge-tree fallback that tests the PR's actual head (git fetch origin pull//head + FETCH_HEAD), ref verification before trusting exit codes, and the --name-only conflict list as the work list. Resolution is merge-never-rebase with repo-specific semantic rules. - /github-ci-failures gains a detection-only merge-conflict pre-flight that hands off to the orchestrator, plus a check-name table matching the actual CI jobs (Lint, Gem Tests, Docs Lint, Docs Tests). - allowed-tools frontmatter now covers every command each runbook instructs (gh pr list/checkout, git fetch/merge/merge-tree/rev-parse/ status/switch, bundle install, bun, docs bin/rubocop). --- .claude/commands/github-ci-failures.md | 20 ++- .claude/commands/github-review-comments.md | 2 +- .claude/commands/github-review-pr.md | 163 +++++++++++++++++++++ CLAUDE.md | 1 + 4 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 .claude/commands/github-review-pr.md diff --git a/.claude/commands/github-ci-failures.md b/.claude/commands/github-ci-failures.md index 5c2ebc2..33c8deb 100644 --- a/.claude/commands/github-ci-failures.md +++ b/.claude/commands/github-ci-failures.md @@ -2,7 +2,7 @@ description: "Use when CI checks are failing on a PR — fetches failure logs, diagnoses root causes, implements fixes, and pushes until CI is green." model: opus argument-hint: "PR number (e.g., 41 or #41)" -allowed-tools: Bash(gh pr view:*), Bash(gh pr checks:*), Bash(gh pr diff:*), Bash(gh api:*), Bash(gh run view:*), Bash(git log:*), 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 diff:*), Bash(gh api:*), Bash(gh run view:*), Bash(git log:*), Bash(git diff:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(bundle exec:*), Bash(bun:*), Bash(cd:*), Read, Write, Edit, Glob, Grep, Agent --- # Fix GitHub CI Failures: $ARGUMENTS @@ -17,12 +17,30 @@ Parse `$ARGUMENTS` flexibly. If empty, auto-detect from current branch: gh pr list --author=@me --head="$(git branch --show-current)" --state=open --json number,title ``` +Once you have the PR number, confirm it: + +```bash +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 ```bash gh pr checks ``` +| Check name | What it runs | How to Get Logs | +|------------|--------------|-----------------| +| `Lint` | `bundle exec rubocop lib spec` on the gem | `gh run view --job= --log-failed` | +| `Gem Tests (Ruby 3.2/3.3/3.4/4.0)` | `bundle exec rspec` at the root | `gh run view --job= --log-failed` | +| `Docs Lint` | `bin/rubocop`, `bun run lint:js`, `bun run lint:css` in `docs/` | `gh run view --job= --log-failed` | +| `Docs Tests` | `bundle exec rspec` in `docs/` (Playwright, after `bun run build:css`) | `gh run view --job= --log-failed` | + +Extract the run ID and job IDs from the check URLs. The URL format is: +`https://github.com/mhenrixon/daisyui/actions/runs//job/` + ## Phase 2: Fetch Failure Logs ```bash diff --git a/.claude/commands/github-review-comments.md b/.claude/commands/github-review-comments.md index 5801f76..8f97281 100644 --- a/.claude/commands/github-review-comments.md +++ b/.claude/commands/github-review-comments.md @@ -2,7 +2,7 @@ description: "Use when a PR has unresolved review comments that need responses -- evaluates each comment, implements valid fixes, pushes back on incorrect suggestions, and resolves all threads." model: opus argument-hint: "PR number (e.g., 123 or #123)" -allowed-tools: Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(git log:*), Bash(git blame:*), 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 diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(git log:*), Bash(git blame:*), Bash(git push:*), Bash(git commit:*), Bash(git add:*), Bash(bundle exec:*), Read, Write, Edit, Glob, Grep, Agent --- # Review GitHub PR Comments: $ARGUMENTS diff --git a/.claude/commands/github-review-pr.md b/.claude/commands/github-review-pr.md new file mode 100644 index 0000000..bdf26fa --- /dev/null +++ b/.claude/commands/github-review-pr.md @@ -0,0 +1,163 @@ +--- +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 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 +--- + +# Review GitHub PR (full pass): $ARGUMENTS + +You are running a full review pass on a pull request. The pass has three phases that MUST run in this order: + +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 + +**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 "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. +- 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. + +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 + +The user may provide a PR number as `$ARGUMENTS`. Parse it flexibly: + +- `PR156`, `PR 156`, `pr156` → PR 156 +- `156` → PR 156 +- `#156` → PR 156 +- Empty/blank → auto-detect from current branch + +**If no PR number is provided**, detect it automatically: + +```bash +gh pr list --author=@me --head="$(git branch --show-current)" --state=open --json number,title +``` + +If exactly one open PR exists for the current branch, use it. If none or multiple, ask the user. + +Once you have the PR number, confirm it: + +```bash +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 `.claude/rules/git-workflow.md` forbids 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: + - **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. + - **`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. + - **`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. + - **`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. + - **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 (`# "sm:..." "md:..." "lg:..."`) above EVERY modifier line. Tailwind scans those comments to generate responsive classes; dropping one in a merge silently breaks responsive variants. + - **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. +4. Run the verification gates BEFORE pushing the merge — scoped to what the conflict touched, at minimum: + ```bash + bundle exec rubocop lib spec + bundle exec rspec + # docs/ files involved (the docs app has its own lint + test setup): + cd docs && bin/rubocop && bun run lint:js && bun run lint:css + cd docs && bun run build:css && bundle exec rspec + ``` +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-ci-failures` + +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. + +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: + +1. Identify failing checks via `gh pr checks `. +2. Fetch failure logs. +3. Diagnose root cause for each. +4. Fix locally — lint first (fast, deterministic), then specs, then docs tests. +5. Verify locally before commit (`bundle exec rspec `, `bundle exec rubocop`). +6. Commit + push + report which checks are now running. + +### Phase A exit criteria + +Before moving to Phase B, one of these must be true: + +- All CI checks are green on the latest pushed commit. OR +- 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 +- 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. + +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. + +--- + +## Phase B: Run `/github-review-comments` + +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. + +The slash command is at `.claude/commands/github-review-comments.md`. Its workflow: + +1. Fetch all unresolved review threads via the GitHub GraphQL API. +2. Read and categorise each comment (valid fix / invalid suggestion / unclear), verifying against the DaisyUI 5 spec where relevant. +3. Implement accepted fixes; verify locally (specs, rubocop). +4. Commit all fixes together with a clear message; push. +5. Reply to every thread with the commit SHA (for accepted fixes) or technical reasoning (for rejections). +6. Resolve each thread via the GraphQL `resolveReviewThread` mutation. +7. Verify no unresolved threads remain. + +### Phase B exit criteria + +- All unresolved review threads have been replied to and resolved (or the user has explicitly approved leaving a specific thread open). +- The branch has been pushed with all accepted fixes. + +--- + +## Phase C: Final 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 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. 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. diff --git a/CLAUDE.md b/CLAUDE.md index 63b5415..2fa6a62 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -128,6 +128,7 @@ HEADLESS=false bundle exec rspec # Watch browser tests run | `/test-all` | Run complete test suite (gem + docs) | | `/fix-docs-tests` | Fix failing docs specs | | `/review-pr` | Review a GitHub PR for quality and patterns | +| `/github-review-pr` | Full PR pass: resolve merge conflicts, then fix CI failures, then resolve review comments (in that order) | | `/github-review-comments` | Respond to unresolved PR review comments | | `/github-ci-failures` | Diagnose and fix CI failures | From 14da429cf70c2c02eb479e059532d2de3dd715dd Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Thu, 9 Jul 2026 10:53:14 +0200 Subject: [PATCH 2/2] fix: address backport verification findings --- .claude/commands/github-review-pr.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/commands/github-review-pr.md b/.claude/commands/github-review-pr.md index bdf26fa..7a720e6 100644 --- a/.claude/commands/github-review-pr.md +++ b/.claude/commands/github-review-pr.md @@ -73,7 +73,7 @@ gh pr view --json mergeable,mergeStateStatus,baseRefName - **`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. - **`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. - **`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. - - **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 (`# "sm:..." "md:..." "lg:..."`) above EVERY modifier line. Tailwind scans those comments to generate responsive classes; dropping one in a merge silently breaks responsive variants. + - **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. - **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. 4. Run the verification gates BEFORE pushing the merge — scoped to what the conflict touched, at minimum: ```bash