Skip to content

docs(skills): delegated reviews run the preflight; image-heavy reviews name gemini - #5171

Open
sonichi wants to merge 2 commits into
mainfrom
docs/delegated-review-routing
Open

sonichi wants to merge 2 commits into
mainfrom
docs/delegated-review-routing

Conversation

@sonichi

@sonichi sonichi commented Oct 6, 2026

Copy link
Copy Markdown
Owner

What

Docs only: two SKILL.md files, no code.

  • skills/review-preflight/SKILL.md: new "Delegated reviews" section. When a PR review is handed to Codex (claude-codex) or Gemini (claude-router), the delegate never loads CLAUDE.md or REVIEW.md, so it never sees the lessons the preflight prints. The delegating session therefore:

    • runs review-preflight.py <PR> and pastes its criteria into the brief,
    • checks the gate items it reports (prior art, stale approvals) itself,
    • verifies any blocker before posting.

    It also notes that the script exits non-zero in repos with no REVIEW.md. Today's reviews hit that in pipehouse, cinny-webclient, ag2space-backend and cinny-desktop.

  • skills/claude-router/SKILL.md: one paragraph. A PR review whose diff is mostly images or video has to name gemini explicitly, because keyword routing sends any prompt containing "review" to codex.

Why

Chi asked for reviews to run on Codex or agy instead of Claude (2026-10-06). The routine for doing that lived only in one session's memory, so other workers and hosts would not follow it. Two gaps showed up in practice:

  • the preflight's criteria never reached the delegate;
  • a docs PR with 73 screenshots (AG2Platform/pipehouse#52) would have been routed to Codex.

Evidence

route-ai.sh --dry-run at this head. Paths are shortened to <repo>; the scripts are unchanged by this PR, so main gives the same output.

$ route-ai.sh --dry-run -- "Review the screenshots in this PR for private data"
engine: codex
command: bash skills/claude-codex/scripts/codex-run.sh --cd <repo> -- Review\ the\ screenshots\ in\ this\ PR\ for\ private\ data
$ route-ai.sh --dry-run -- "Review the multimodal screenshots in this PR"
engine: codex
command: bash skills/claude-codex/scripts/codex-run.sh --cd <repo> -- Review\ the\ multimodal\ screenshots\ in\ this\ PR
$ route-ai.sh --dry-run -- "Use gemini to review the screenshots in this PR"
engine: gemini
command: bash skills/claude-gemini/scripts/gemini-run.sh --cd <repo> --approval-mode plan -- Use\ gemini\ to\ review\ the\ screenshots\ in\ this\ PR

Even "multimodal" plus "review" picks codex, because the review branch is checked first. Only naming the engine switches it, which is what the new paragraph tells the reader to do.

bash scripts/review-checks.sh --diff <this diff> → rc 0 (hardcoded-paths + root-artifacts clean; prose-cap has no in-scope .py files).

Overlap

#4994 (rename claude-gemini → agy) also edits skills/claude-router/SKILL.md. This PR adds one paragraph after the "honor that directly" line and touches nothing #4994 rewrites, but whichever merges second may need a trivial rebase, and the new paragraph's gemini engine name follows whatever #4994 settles on.

🤖 Generated with Claude Code

…s name gemini

review-preflight: a review handed to Codex or Gemini never loads CLAUDE.md or
REVIEW.md, so the delegating session runs the preflight, pastes its criteria
into the brief, and checks the gate items itself. claude-router: route-ai.sh
sends any prompt containing "review" to codex, so a screenshot/video review
must name gemini explicitly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonichi
sonichi requested a review from qingyun-wu as a code owner October 6, 2026 21:24

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 892f81c3dbff2094be96cf0005b481b868bb04e8 (docs-only, 2 SKILL.md files). Preflight run; REVIEW.md applied.

Blocker: the new "no REVIEW.md -> exits non-zero" sentence describes behaviour the script does not have

skills/review-preflight/SKILL.md:36-37 (new) says the preflight "reads <repo>/REVIEW.md and exits non-zero where there is none". The script resolves the guide from its own file location, not from the cwd or the PR's repo:

  • skills/review-preflight/scripts/review-preflight.py:43: repo_root() runs git rev-parse --show-toplevel with cwd=Path(__file__).resolve().parent, and falls back to parents[3] (the sutando checkout).
  • :53-57: resolve_guide() returns repo_root() / "REVIEW.md" unless --guide is passed.

So when it is invoked the way the skill documents (skills/review-preflight/scripts/review-preflight.py, from a sutando checkout), it always finds sutando's REVIEW.md and exits 0, whatever repo the PR belongs to. Repro at this head, from a fresh empty git repo with no REVIEW.md:

$ cd <scratch>/norev && git init -q
$ python3 <clone>/skills/review-preflight/scripts/review-preflight.py 52 >/dev/null; echo rc=$?
rc=0
$ python3 <clone>/skills/review-preflight/scripts/review-preflight.py 5171 | head -1
review-preflight: criteria from <clone>/REVIEW.md

The same thing happens with the installed engine copy (rc=0). This is worse than a wrong exit code. A delegator reviewing pipehouse/cinny-webclient/etc. who follows this paragraph gets rc 0, assumes the repo has its own guide, and pastes sutando's lessons into the foreign-repo brief. Prior-art lookups would also default to gh's {owner}/{repo} inference from the cwd (:94-103), not the PR's repo. My guess is that the non-zero exits seen in the four repos came from a relative-path invocation (python3 skills/review-preflight/... from inside the foreign repo), which makes Python itself fail with "can't open file". The script's guard never fired. I have not verified that.

Suggested fix: say that the guide comes from the checkout the script lives in, so for a PR in another repo the delegator passes --guide <that repo>/REVIEW.md (or --repo owner/name for prior art), or writes the brief from the PR when that repo has no guide. Also note that rc 0 does not mean the criteria match the PR's repo.

Verified, no change needed

  • Router claim (skills/claude-router/SKILL.md:34-35): this is accurate at skills/claude-router/scripts/route-ai.sh:103-114. Explicit names are checked first (codex -> gemini -> kimi/pi), then the review|diff|regression|bug|implement|patch branch sends to codex (:109-110), before the multimodal -> gemini branch (:111). It matches the dry-run evidence in the PR body.
  • The referenced command and paths exist: skills/review-preflight/scripts/review-preflight.py, skills/claude-codex, skills/claude-gemini, skills/claude-router/scripts/route-ai.sh.
  • Overlap with #4994: that PR's claude-router/SKILL.md hunk only rewrites lines 1-12 (description and the intro sentence). This PR inserts after line 32. There is no textual conflict, only the gemini naming question the body already flags.

Nits (non-blocking)

  • route-ai.sh:103 checks codex before gemini, so a prompt that names both (for example one quoting a codex-run path) still goes to codex. --engine gemini (route-ai.sh:12) is the robust form and is worth naming in the new paragraph alongside "name gemini".
  • "that model never loads CLAUDE.md": Codex is launched with codex exec -C "$WORKDIR" (skills/claude-codex/scripts/codex-run.sh:161), so in a sutando checkout it does load AGENTS.md, whose line 110 tells it to run the preflight. The delegator step is still right, because the delegate may lack network or gh auth and foreign repos have no AGENTS.md. The absolute "never" overstates it, though.

Not verified

  • CI was still pending at review time (python/shell lanes). It does not matter for a docs-only diff, and I did not wait for it.
  • I did not run Codex or Gemini end to end with a preflight-augmented brief.

…e PR repo's

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants