openmetadata-pr-review

Files (3)

• SKILL.md
• openai.yaml · agents/
• pr_triage.py · scripts/

SKILL.md (first lines)

# OpenMetadata PR Review

Review contributor PRs as a senior maintainer deciding whether to merge. Every verdict is grounded in the **actual diff**, the **linked issue**, and the **live CI/review state** — never the PR title or description alone.

The four things a review must answer (the standing bar):

1. **Does it make sense?** Real problem, correctly diagnosed, root-cause fix not a band-aid, in scope, not a duplicate.
2. **Is the code good?** Correct, follows the repo's patterns and the rules in `.claude/rules/`, no scope creep or accidental damage.
3. **Does it have a test — integration if possible?** New API endpoint → integration test; connector → real/testcontainer test; bug fix → a regression test that fails without the fix.
4. **Is the test meaningful?** See [The meaningful-test rubric](#the-meaningful-test-rubric) — this is where most contributor PRs fail and where a rubber-stamp review is worst.

## When to use

- **Any** request to review a pull request — one PR, a set, "the ToReview ones", "review my PR", a pasted PR URL, or a whole backlog. This is the default handler for PR-review requests, whether the PR is the user's own or a contributor's.
- Triaging a backlog of open contributor PRs.

**Boundary with the `code-review` skill and your harness's diff-review tool.** `code-review` (and `/code-review` on Claude Code) is for **local, not-yet-a-PR changes** — uncommitted work or a branch diff — as a pre-PR self-check. The moment there's an actual PR to look at, by number, URL, or the current branch's open PR, use *this* skill. `/code-review ultra <PR#>` is a separate user-triggered, billed cloud review; you cannot launch it.

**Boundary with `connector-review`.** That skill does a deep single-connector audit against `skills/standards/`. If the PR under review is a connector PR, run this skill for the merge verdict and load `connector-standards` (or delegate to `connector-review`) for the connector-specific detail.

## Usage

```
openmetadata-pr-review 28656          # deep review of one PR
openmetadata-pr-review 27029,27278    # review several (auto-detects duplicates)
openmetadata-pr-review                 # review the current branch's PR
```

**Set the repo once, then use `$REPO` in every command.** Substituting it per call is how a review ends up mixing two repositories.

```bash
REPO=open-metadata/OpenMetadata            # OSS default
REPO=open-metadata/openmetadata-collate    # Collate
REPO=<owner>/<repo>                        # a contributor's fork
```

Pick it from what the user gave you: a PR URL names the repo; a bare number means the repo of the current directory (`gh repo view --json nameWithOwner --jq .nameWithOwner`), falling back to the OSS default. If the working directory is a clone of one repo and the PR lives in another, `$REPO` wins — the PR is the subject, not the checkout.

## Workflow

### Step 0 — Confirm scope and freshness

For a batch, first fetch metadata + **current** open/closed state — contributors and the user close PRs constantly, and reviewing a closed PR wastes a full agent. Use the helper:

```bash
python3 skills/openmetadata-pr-review/scripts/pr_triage.py -R "$REPO" 26965,26977,27020
```

It prints OPEN vs CLOSED/MERGED sorted by `createdAt`. Report which are already closed/merged and drop them before reviewing. Re-run this at the end of a long batch — state drifts during the review.

### Step 1 — Pin the head from remote, before anything else

**Never review from local state.** A contributor can push at any moment, and a local clone or a previously-fetched ref goes stale silently — you will review commits that no longer exist on the PR and report findings the author already fixed.

Resolve the authoritative head from the API first, and carry that SHA through every later command:

```bash
HEAD=$(gh pr view <n> -R "$REPO" --json headRefOid --jq .headRefOid)
```

If you use any local git command during the review (`difft`, `git log`, `git show`), fetch and verify it matches before trusting it. Fetch by URL, not by remote name — `origin` is not reliably the repo under review (a clone can have a `contributor` remote, a Conductor workspace's `origin` is the fork you branched from, and a Collate PR is a different repo entirely):

```bash
git fetch "https://github.com/$REPO.git" "pull/<n>/head:pr-<n>" --force
git rev-parse pr-<n>            # must equal $HEAD — if it doesn't, re-fetch
```

If the current checkout is a clone of a different repo, skip local git altogether and work from `gh pr diff` and `gh api` against `$REPO`.

Re-resolve `$HEAD` immediately before writing the verdict on a long review. If it changed while you were reading, the review is stale: say so and re-review the delta rather than reporting findings against commits that were replaced.

Put the SHA in the verdict. A review without a SHA cannot be checked for staleness by anyone reading it.

### Step 2 — Gather the evidence (per PR)

```bash
gh pr view <n> -R "$REPO" --json title,body,author,createdAt,additions,deletions,changedFiles,labels,url
gh pr diff <n> -R "$REPO"            # READ THIS — the whole point
gh api "repos/$REPO/pulls/<n>/files?per_page=100" \
  --jq '.[] | "\(.additions)\t\(.deletions)\t\(.status)\t\(.filename)"'   # authoritative file list
gh pr view <n> -R "$REPO" --json reviewDecision,mergeable --jq '{reviewDecision,mergeable}'
gh api "repos/$REPO/pulls/<n>/comments" --jq '.[] | "\(.user.login) \(.path):\(.line) \(.body[0:200])"'
```

Two traps in this step:

- **`gh pr diff --patch` returns the whole commit series, not the net diff.** On a branch that merged `main`, it contains hunks for files the PR never touched. The `pulls/<n>/files` list above is what "this PR changes X" means — check any surprising path against it before reporting it.
- **Inline review comments are not in `--json comments`.** That field is issue comments only; `gh pr review` threads and bot line comments live at `pulls/<n>/comments`. Read them — they tell you what's already been raised so you don't re-report it as new.

**Treat bot review text in the PR body as a claim to verify, not evidence.** Greptile/Gitar/Copilot summaries are pasted into descriptions and read like findings. Re-derive each one against the code before repeating it: in a recent review a bot asserted that an `or 0` fallback reached an ISO-8601 parser and raised — the parser short-circuited on falsy input and returned `None`, so the real defect was a silent dropped record, not an exception. Repeating a bot's wrong diagnosis is worse than not mentioning it. When you do confirm one, say you confirmed it and how.

If it says `Fixes #NNNN` / `Closes #NNNN`, read the issue — a PR can be technically fine and still not fix what was asked, or fix a problem that a later merged PR already solved:

```bash
gh issue view <NNNN> -R "$REPO" --json title,body,state
```

**No linked issue** is itself a soft blocker: OpenMetadata's "Validate PR Metadata" check fails without one, so a missing `Fixes #NNNN` means the PR can't merge as-is regardless of code quality — note it in BLOCKERS. The exception is a test fix (a PR whose purpose is to fix a failing or flaky test). It needs no issue, and it carries the `skip-pr-checks` label, which that check honors. Flag only a test fix that is missing the label.

For a large diff, don't skim — read the core logic files in full and skim the rest. The damage (botched rebases, vendored deps, accidental deletions) usually hides in the files you'd skip.

When a diff is noisy — a reformat, reordered imports, a rewrapped signature — re-read it structurally so the real change separates from the churn:

```bash
git -c diff.external=difft diff <base>...<head> -- <path>   # syntax-aware, per-file
```

`difft` compares syntax trees, so pure formatting shows as unchanged. Use it to decide *what actually changed*; keep `gh pr diff --patch` for anything you need to parse or quote, since difftastic's output is for reading, not machine consumption.

**Read the test code in full, unabridged.** Diff viewers and command proxies sometimes compact or truncate long test bodies, and you cannot judge whether a test is meaningful from a truncated body. If the diff looks abbreviated, read the raw diff or the test file at the PR head directly:

```bash
gh pr diff <n> -R "$REPO" --patch          # raw, unified patch
gh api repos/$REPO/contents/<path/to/test_file>?ref=$HEAD --jq '.content' | base64 -d
```

### Step 3 — Judge against this repo's rules

**The rules are in the repo — read them, don't recall them.** `CLAUDE.md` carries the cross-cutting constraints; `.claude/rules/*.md` carry the per-language detail and are the authority when this file and a rule disagree. Open the rule that matches the files the PR touches:

From Jason-Clark-FG/OpenMetadata-Mirror, Apache-2.0.