Improve comparison base resolution for review skills - #27827
Craig Macomber (Microsoft) (CraigMacomber) merged 5 commits into
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (187 lines, 3 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the “review” and “api-changes” skills to stop assuming origin/main and instead resolve a more accurate comparison base (PR base branch when available, otherwise the canonical microsoft/FluidFramework upstream). It adds a reusable “comparison-base” skill to centralize that logic and improve behavior across forks, renamed remotes, and local-only change scenarios.
Changes:
- Added a new
.claude/skills/comparison-baseskill to resolve PR target/upstream and select a comparison commit on the target’s first-parent history, with divergence detection. - Updated the
reviewskill to use the resolved base commit and consistently choose local-vs-remote diff endpoints (including staged/unstaged/untracked in local reviews). - Updated the
api-changesskill to compare API report diffs against the resolved base commit (notorigin/main).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .claude/skills/review/SKILL.md | Switches the review workflow to use resolved target/base commit and supports local vs remote diffing consistently. |
| .claude/skills/comparison-base/SKILL.md | New reusable workflow that resolves target branch + selects a stable comparison commit and checks divergence. |
| .claude/skills/api-changes/SKILL.md | Updates API-change detection to diff against the resolved comparison base (and working tree). |
Suppressed comments (1)
.claude/skills/comparison-base/SKILL.md:110
- Since the skill now resolves
$TARGET_REPOSITORY(owner/repo), it should be returned in the Outputs list so callers (e.g.review) can print an unambiguousrepo:branchbase in reports.
- `$TARGET_SOURCE`, `$TARGET_BRANCH`, and `$TARGET_REF`
| First determine whether an open PR exists for the review branch. Prefer active PR metadata from the editor's PR integration when available, but verify its head repository and branch match `$REVIEW_REPOSITORY` and `$REVIEW_HEAD_BRANCH` when those values are known. Its published head must also match `$REVIEW_OID` or be an ancestor of a locally-ahead `$REVIEW_REF`. | ||
|
|
||
| Otherwise, query the GitHub commit-to-PR API (`repos/{owner}/{repo}/commits/$REVIEW_OID/pulls`) for each GitHub repository named by configured remotes, including `microsoft/FluidFramework`. Treat HTTP 404 or 422 as no commit candidates and continue; locally-ahead or unpublished commits may not exist in the queried repository. If `$REVIEW_REPOSITORY` and `$REVIEW_HEAD_BRANCH` are known, also query open PRs by that full head repository owner plus branch so locally-ahead commits can resolve their published PR. Read `state`, `base.repo.clone_url`, `base.ref`, `head.repo.full_name`, `head.ref`, `head.sha`, and `html_url` from the REST response. Only accept open PRs whose head repository and branch match the known review identity and whose head SHA matches `$REVIEW_OID` or is its ancestor. | ||
|
|
||
| Do not rely on bare `gh pr view` or an unqualified branch name. These are ambiguous in fork checkouts. If multiple open PRs match, ask the user which PR to use. |
There was a problem hiding this comment.
I know gh won't work in all cases, but I think these gh commands will reveal the right info to the agent in many cases and may be easier for it to follow:
gh pr view --json url,state,baseRefName,headRefName,headRefOid,headRepository
gh repo view --json nameWithOwner,defaultBranchRef
There was a problem hiding this comment.
I have incorporated this approach as an option (with some tweaks to make it robust with no default repository), and also adjusted the process so it stop when it find a good branch to reduce extra tool calls.
Tyler Butler (tylerbutler)
left a comment
There was a problem hiding this comment.
It seems like the bundle size test has to do something similar where it figures out the base commit - but I guess it's easier in CI where the remotes are fixed.
It's not just the fixed remotes, but in our PR CI you know that there is always a PR with a clear merge target. This skill can work without a PR, and even without a branch (review local changes). It only uses PRs as an optional heuristic mostly so that if your branch is merging into something other than main it can still give a good review. |
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
63630aa
into
microsoft:main
Description
The existing review skill was often counterproductive for me as it was very specific about what to review against, often worse than what the model would come up with on its own. These changes correct its assumptions, making the skill much more generally useful, even with less powerful models, explicitly directing it to handle removes with different names (like my upstream), compare to the merge point with the target branch not its head, handle local changes (untracked, modifications and staged changes) etc.
The new approach checks for an existing PRs as a heuristic to find the correct upstream branch and remote, then looks for an upstream for the https://github.com/microsoft/FluidFramework repo, regardless of its name.
It also warns if the merge base and target branch are too out of sync by commit count, not just diff size (which was slow and confusing when I hit it in my fork due to using the wrong remote).
I also refactored our two existing skills which hard coded origin/main to both use this extracted common logic for improved consistency and maintainability.
I used this skill to review itself, with a couple models and at a couple points in the process (like when it was just local changes on main, when it was staged on a branch, and after the PR was created. Even Haiku 4.5 could follow the steps and produce a review using this skill (its review quality wasn't great, but it found the correct diff to review).
Reviewer Guidance
The review process is outlined on this wiki page.