openmetadata-pr-review

# 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. 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: | PR touches | Read | |---|---| | `**/*.java` | `.claude/rules/java.md` | | UI `*.{ts,tsx}` | `.claude/rules/frontend-react.md`, `component-library.md`, `frontend-styling.md`, `frontend-a11y.md`, `frontend-performance.md`, `i18n.md` | | `openmetadata-ui/src/main/resources/ui/playwright/**` | `.claude/rules/frontend-playwright.md` | | `ingestion/src/**/*.py` | `.claude/rules/python-ingestion.md` (+ the `connector-standards` skill for connector PRs) | | `openmetadata-spec/**` or any `generated/**` | `.claude/rules/schema-first.md` | | `bootstrap/sql/**` | `.claude/rules/migrations.md` | The high-signal checks, distilled — use these to decide *which* rule to open, not as a replacement for it: **Makes sense / architecture** - Root-cause fix, not a band-aid. Targeted, not defensive belt-and-suspenders. - Extends an established pattern (`docs/design-patterns.md`) rather than inventing a parallel one. - Connector-specific logic stays in connector files — never in shared `builders.py`, `lineage/parser.py`, `sqa_mixin.py`. - No new dependency that duplicates existing functionality. A new connector that's really a thin wrapper of an existing one (e.g. Supabase = hosted Postgres) is a scope question, not an automatic yes. - Doesn't duplicate another open PR (see [Duplicate detection](#duplicate-detection)). - **Every cache bounded.** A bare `dict`/`HashMap`/`Map` keyed on entities is an OOM red flag on large catalogs — `CLAUDE.md` makes this a hard constraint, so an unbounded cache is a blocker, not a nit. - **Comments explain why.** A diff full of `// Create user` above `createUser()` is noise the repo explicitly rejects. **Python** — pytest + plain `assert`, no `unittest.TestCase`; lazy `%`-style logging, never f-strings in `logger.*`; `model_str()` for RootModel→str; Pydantic v2 `Field(default=...)`; `Either(left=StackTraceError)` on topology yields. **Java** — no wildcard imports; `mvn spotless:apply` clean; small methods, single trailing return, ≤3 nesting; no empty catch, no bare `catch (Exception)`, no magic strings; integration test in `openmetadata-integration-tests/` for any new/changed endpoint. **TypeScript/React** — no `any`; no new Ant Design (use `openmetadata-ui-core-components`); `tw:` Tailwind prefix and design tokens, no hex; i18n keys, no string literals; generated models regenerated, not hand-edited. **Cross-layer contract** (a frequent "incomplete" failure) - Schema change (`openmetadata-spec/`) → `make generate` → generated Python/TS committed and in sync (watch for a red `generate-types` check). - **A new member in a `oneOf` union has blast radius outside the diff.** Adding a connection type to `databaseService.json`/`pipelineService.json` widens the union every `connection.<field>` access is typed against, so code in *untouched* connectors starts failing type-check (e.g. adding a connection with no `hostPort` breaks `kafkaconnect/metadata.py`'s `connection.hostPort`). Read the static-check errors for files the PR never touched — those are the regression, and they are easy to miss because they're not in the diff. - New connector → JSON schema + generated models + `setup.py` plugin entry + UI icon wiring (`ServiceIconUtils.ts`) + locale `*.md` doc + tests. Partial = NEEDS_WORK. - New connection field → `yarn parse-schema`; `"format": "password"` (never `"password": true`) so secrets route through the secrets manager and get masked. - Migration in `bootstrap/sql/` → append-only, **both** MySQL and PostgreSQL, idempotent, and tested against real multi-row data. **Guardrails / safety** - No customer names or data anywhere (code, tests, fixtures, logs). Placeholders only. - Destructive features (deleting lineage, secrets, entities) must be opt-in, age/precedence-guarded, and covered by a real integration test against MySQL **and** PostgreSQL. Scrutinize the deletion predicate hard. - No committed secrets, `.orig` merge artifacts, `package-lock.json` in a yarn repo, CRLF conversions, or vendored dependency trees. - **`.github/workflows/**` changes are a supply-chain surface.** Any workflow edit in a contributor PR gets read line by line — new `pull_request_target` triggers, third-party actions pinned by tag instead of SHA, and secrets exposed to fork-run jobs are blockers. - **License headers are per-module and only the UI is enforced.** `ingestion/` and `openmetadata-airflow-apis/` Python files carry the Collate Community License header, UI TS/TSX carries Apache-2.0, Java mostly carries none. A wrong Python or Java header ships silently — check it by eye against a sibling file in the same directory. ### Step 4 — Assess the test (integration if possible, and meaningful) This is the step most reviews skip. Do it explicitly for every PR. See the rubric below. A PR with no test, or a test that only proves the mocks are wired, does **not** clear the bar — say so plainly. For a full coverage analysis on a large PR, the `test-enforcement` skill has the 90%-changed-class procedure. **On a multi-file PR, map source file → test file before judging quality.** "Has tests" is not a property of a PR, it's a property of each file. List every new/changed source file with its line count, then list which test file exercises it; the ones with nothing pointed at them are the finding. Grep the test files for the source module's symbols rather than trusting the directory layout — a `test_<connector>.py` next to a four-module connector routinely covers two of them. In one connector review this turned "68 tests, all passing" into "the 240-line status module has zero tests and the 656-line lineage module has two constructor tests" — and both untested modules were where the runtime bugs were. ### Step 5 — Verdict + report Use the verdict scale and per-PR format in [Output](#output). For a batch, add the grouped triage. Fill every slot of the per-PR contract, in order. The `read:` lines are part of the contract, not a nicety: each names the artefact you opened *this session* that backs the slot above it. A slot whose `read:` is `not read` is itself a finding — it tells the user which parts of the verdict are unbacked. Write that instead of softening the claim with "should", "seems" or "probably", and instead of inferring from a proxy: the checks summary is not the job log, the diff's file list is not the test body, and the PR description's claim is not the issue. ## The meaningful-test rubric A test is meaningful only if a wrong implementation would make it fail. Apply these in order: 1. **Would it fail without the fix?** For a bug fix, the test must reproduce the bug — assert the *broken* output is now correct. If the test passes against unpatched `main`, it proves nothing. When in doubt, mentally (or actually) revert the fix and check the test would go red. 2. **Does it assert an observable outcome?** API response body, DB/search state, emitted lineage edges, computed stats, resolved ORM types, the actual parsed value. NOT `verify(mock).methodWasCalled()` and NOT asserting the return of a method whose collaborators are all mocked. 3. **Mock only at boundaries.** Mocking an external HTTP client or third-party driver is fine. Mocking 3+ of the project's *own* classes to test internal plumbing means the test is testing wiring, not behavior — flag it. `thenCallRealMethod()` on the one method under test with everything else mocked is the classic tell. 4. **Prefer integration.** OpenMetadata has real infra — `OpenMetadataApplicationTest`, Docker/testcontainers, real OpenSearch. For endpoints and connectors, an integration test that exercises the real path beats any amount of mocked unit tests. Note when a PR *could* have used it and didn't. Real-DB integration tests in this repo routinely catch bugs (missing enum cases, NOT-NULL violations on create) that mocked unit tests structurally cannot. 5. **Ask the killer question:** "If this test passes but the code is wrong, does anything break?" If the answer is "no, everything real is mocked out," the test is decoration. You don't need to check out the branch — reason about whether each assertion would still hold against the *unpatched* code. For a high-stakes or destructive PR where that reasoning is uncertain, it's worth fetching the branch and running just that one test (with and without the fix). **Telling an integration test from a mocked one in this repo:** - Java integration test → lives in `openmetadata-integration-tests/`, class ends `*IT`, extends `BaseEntityIT`, uses `SdkClients`/`TestNamespace`, or extends `OpenMetadataApplicationTest`. A `*Test` in `openmetadata-service/` full of `Mockito.mock(...)` + `thenCallRealMethod()` is a unit test, often mock-wiring. - Python integration test → lives in `ingestion/tests/integration/`, uses `testcontainers` (a real DB/engine spun up), real `OpenMetadata` client calls. A test in `ingestion/tests/unit/` built from `MagicMock`/`patch` with the source object faked is a unit test — fine for pure parsing logic, insufficient for topology/lineage/connection behavior. - UI → a Jest test that mocks the component's whole data layer proves rendering, not behavior. User-facing changes want a Playwright spec under `openmetadata-ui/src/main/resources/ui/playwright/`; note that some libraries (RAQB, for one) are fully mocked in Jest, so their logic is *only* testable in Playwright. Common failing patterns seen in the wild: `unittest.TestCase` + `MagicMock` source object with the real method rebound onto it; a `test_x` that builds hand-crafted mock rows and asserts the parser's own branching rather than feeding real SQL through `LineageParser`; a connector test asserting the connector's own (wrong) return types so it stays green while the real topology path is broken; `assertEqual(null, ...)` instead of `assertNull`. Report the test explicitly per PR: **present + meaningful**, **present but mock-wiring only**, or **missing**. ## Reading CI `red` is not a verdict — *which job* is red is. Pull the failing job's log before you call it: ```bash gh pr checks <n> -R "$REPO" | awk -F'\t' '$2!="pass" && $2!="skipping"' # current state per check gh api "repos/$REPO/actions/jobs/<job-id>/logs" # the one failing job ```