You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Deferred from #357 review round 5. The finding is correct; the fix is a broader refactor than that PR's scope, because most of the copies it would consolidate are pre-existing.
The duplication
The same sequence -- look the PR up over the API, read head.ref and head.repo.full_name, decide whether to pass --ref -- now appears five times, each carrying the same #285/#289 explanatory comments near-verbatim:
Doing it properly means absorbing claude.yml's three pre-existing copies, not just the two new ones. Extracting a composite and converting only the new callers would leave four copies plus a composite -- strictly worse than five copies, since a reader then has to work out which sites are authoritative.
claude.yml is also the most load-bearing workflow in the repo and the one whose own reviews cannot self-verify before merge (see CLAUDE.md, "A PR fixing claude-code-review.yml (or claude.yml) itself can't self-verify"), so touching all three of its copies belongs in a PR whose reviewers are looking at exactly that.
What the extraction needs
A composite action, not a checked-out script: gemini.yml's and ai-code-review.yml's call sites run in jobs that never check the repo out, so a bare script path is unreachable there. This is the same constraint parse-workflow-ref documents.
Behavior to preserve, and one place the callers legitimately differ:
#285 -- pass --ref <head branch> so the dispatched run's check-runs land on the PR's head rather than the default branch.
#289 -- a fork PR's head branch does not exist in the base repo, so --ref cannot resolve there.
The fork case is not uniform, and the composite must not flatten it. ai-code-review.yml and claude.yml dispatch anyway without --ref, accepting the check-run mis-attribution. gemini.yml (as of feat: add Gemini CLI and multi-agent AI review workflows #357) refuses to dispatch a fork PR at all, so that gemini-code-review.yml's own fork guard is never reached by a normal mention. So the composite should report the fork status and let the caller decide, rather than deciding for it.
Offline tests plus a real uses: step in _selftest.yml, per the precedent in CLAUDE.md's Tests section: the offline table for the branch/fork logic, and the uses: call for the github.action_path resolution that gha#196 showed unit tests cannot cover.
Note on the sibling half of the same review comment
The CSV-split half of that comment was fixed in #357 directly: detect-review-request.sh and detect-bot-mention.sh both delegate to the existing split-csv-list.sh now, so that script is down to one implementation with four callers rather than four implementations.
Deferred from #357 review round 5. The finding is correct; the fix is a broader refactor than that PR's scope, because most of the copies it would consolidate are pre-existing.
The duplication
The same sequence -- look the PR up over the API, read
head.refandhead.repo.full_name, decide whether to pass--ref-- now appears five times, each carrying the same#285/#289explanatory comments near-verbatim:.github/workflows/claude.yml(three copies).github/workflows/ai-code-review.yml(added in feat: add Gemini CLI and multi-agent AI review workflows #357).github/workflows/gemini.yml(added in feat: add Gemini CLI and multi-agent AI review workflows #357)Two of the five are new, so #357 roughly doubled the count. It did not create the pattern.
Why it was not fixed in #357
Doing it properly means absorbing
claude.yml's three pre-existing copies, not just the two new ones. Extracting a composite and converting only the new callers would leave four copies plus a composite -- strictly worse than five copies, since a reader then has to work out which sites are authoritative.claude.ymlis also the most load-bearing workflow in the repo and the one whose own reviews cannot self-verify before merge (seeCLAUDE.md, "A PR fixing claude-code-review.yml (or claude.yml) itself can't self-verify"), so touching all three of its copies belongs in a PR whose reviewers are looking at exactly that.What the extraction needs
A composite action, not a checked-out script:
gemini.yml's andai-code-review.yml's call sites run in jobs that never check the repo out, so a bare script path is unreachable there. This is the same constraintparse-workflow-refdocuments.Behavior to preserve, and one place the callers legitimately differ:
#285-- pass--ref <head branch>so the dispatched run's check-runs land on the PR's head rather than the default branch.#289-- a fork PR's head branch does not exist in the base repo, so--refcannot resolve there.ai-code-review.ymlandclaude.ymldispatch anyway without--ref, accepting the check-run mis-attribution.gemini.yml(as of feat: add Gemini CLI and multi-agent AI review workflows #357) refuses to dispatch a fork PR at all, so thatgemini-code-review.yml's own fork guard is never reached by a normal mention. So the composite should report the fork status and let the caller decide, rather than deciding for it.Offline tests plus a real
uses:step in_selftest.yml, per the precedent inCLAUDE.md's Tests section: the offline table for the branch/fork logic, and theuses:call for thegithub.action_pathresolution thatgha#196showed unit tests cannot cover.Note on the sibling half of the same review comment
The CSV-split half of that comment was fixed in #357 directly:
detect-review-request.shanddetect-bot-mention.shboth delegate to the existingsplit-csv-list.shnow, so that script is down to one implementation with four callers rather than four implementations.