Review Data PR

End-to-end review of a dataset-update PR. Goes deeper than /review: actually runs the steps, compares to the previous version, audits metadata coverage against a fixed checklist, and reports on /update-dataset workflow status (Slack draft, Codex review, indicator upgrade, downstream deps).

Paired skill — keep in sync. /update-dataset is the author-side counterpart of this skill: the steps it defines are the outcomes verified here. Whenever you add, remove, or change a check in this file, check whether update-dataset/SKILL.md needs a matching author-side step (and add it in the same commit if so). The reverse also holds — see the mirror note there. The creation-side skills /create-dataset and /create-snapshot belong to the same family: the checks here (§5 snapshot fields, §6 links, §7 code clarity, §9 metadata coverage, §10 quality) also gate PRs produced by /create-dataset, so when one of them changes, check whether the create skills need a matching edit in the same commit too.

Inputs

• Optional PR number. If omitted, derive it from the current branch via gh pr list --head <branch>.

Workflow

1. PR metadata

gh pr view <num> --json title,body,isDraft,mergeable,statusCheckRollup,comments,reviews

Flag if PR description is empty (per user's standing rule: keep PR body in sync with substantial changes).

Flag 🟡 if the Summary doesn't open with a tracking-issue link (Tracks: owid/owid-issues#NNNN) — /update-dataset requires it as the first line; most data updates have a corresponding owid-issues ticket.

2. Diff and changed files

gh pr view <num> --json files --jq '.files[] | "\(.additions)+ \(.deletions)- \(.path)"'

For very large diffs (>1MB) skip gh pr diff and read the changed files directly with Read.

3. Locate the new dataset

From the changed files, identify:

• New snapshot path: snapshots/<namespace>/<new_version>/<short_name>.<ext>.dvc (a .py upload script is optional — .dvc + url_download or a local file path is enough)
• New step files: etl/steps/data/{meadow,garden,grapher}/<namespace>/<new_version>/<short_name>.{py,meta.yml}
• Old version (from dag/archive/*.yml or by grepping for the same <short_name>)

3b. Update shape — version bump vs restructure

Before running the pipeline, classify the PR. If any of the following are true, you're reviewing a restructure, not a version bump, and several downstream checks apply differently:

• The short_name changed (old version uses one name, new version uses another).
• The schema changed (wide ↔ long, different file format with a different column set, new dimensions).
• The set of policies/indicators changed substantially (splits, dropped composites, newly added areas).
• Score semantics changed (e.g. binary → continuous 0–1, units/scale changed).

When it's a restructure:

• Don't expect the auto-Indicator-Upgrader to have remapped charts. When short_names differ entirely, the upgrader has nothing to match on. Look for a hand-curated v1 title → v2 title mapping table in the PR description (or a follow-up PR thread). 🟡 if charts on the old chain are still published but no mapping plan exists.
• Don't expect a .py step copy from the old version. Step files should be authored from scratch, not produced by etl update rename. If the new step files look mechanically renamed (same logic, just version-bumped strings), flag 🟡 — the author may have skipped restructure-specific decisions.
• A chart remapped onto a successor indicator needs a config-vs-shape check. Verify its pinned selectedEntityNames exist in the successor's data (v1 regional aggregates often don't — expect the garden step to rebuild them, mirroring the retired step's method), that pinned yAxis bounds don't clip the new range, and that the subtitle doesn't still describe the old construction. Any of the three broken: 🔴 (the default view renders empty, clipped, or mislabeled).
• Slack + /latest drafts are not expected in the PR body at all. /update-dataset keeps them in the author's workbench/ (steps 9 / 9b, owned by /data-updates-comms and /data-update-announcement), so their absence from the PR is correct — don't flag it.

4. Run the full pipeline end-to-end

.venv/bin/etlr data://grapher/<namespace>/<new_version>/<short_name>
.venv/bin/etlr grapher://grapher/<namespace>/<new_version>/<short_name> --grapher --force --only

The --grapher upload is required to verify MySQL ingestion and to enable later checks (chart count, indicator upgrade verification). Confirm:

• All four steps run cleanly (snapshot pulled from S3 if .dvc is committed, otherwise re-fetched)
• MySQL upload returns a dataset id and shows variable upserts
• No errors / no empty tables

Shortcut: read DB checks off the populated staging server. OWID provisions a staging-site-<branch> server (via Buildkite) that runs the ETL chain and uploads to its MySQL. Once it's built, you can read the DB-dependent checks (chart count, attributionShort, rendered titles/Jinja coverage, indicator-upgrade, ghost variables) straight off staging instead of re-running --grapher locally — which also avoids re-triggering step side-effects (e.g. a grapher step that exports to Google Sheets). Confirm the staging ETL build actually ran and finished before trusting it: query staging-site-<branch> for the new dataset's variables (they exist) and check the owidbot PR comment shows a chart-diff block (✅) — that comment is produced after the staging build. ⚠️ Do not use the GitHub build-and-deploy check as that signal — it's the docs Cloudflare Pages deploy (.github/workflows/deploy-docs-cf.yml: make docs.build → deploys site/), with no ETL chain or Grapher upload, so a green build-and-deploy says nothing about pipeline correctness or the data DB. Reserve a local build for what the staging DB can't answer — chiefly entity-level canonicalization (§8c #2) (data lives outside MySQL). If you can't confirm staging is populated, run the pipeline locally per the steps above, and say in the report whether correctness rests on the staging build or a local run.

Review the actual PR head, not a stale local checkout. The local branch can lag origin (or carry an in-progress merge). Before reading step files locally, git fetch and confirm your tree matches the PR head — git diff HEAD origin/<branch> --stat should be empty, and gh pr view <num> --json headRefOid should match git rev-parse HEAD. gh pr view --files / gh pr diff and the staging DB always reflect origin; local Reads do not. If they diverge, sync (or review via gh pr diff) before trusting local files.

5. Snapshot field comparison

Read both .dvc files (old and new) and produce a side-by-side table for these fields:

Field	Check
title	Reasonable update if scope changed
description	Updated to reflect new source / scope
date_published	Should normally differ from date_accessed — source from url_main or the file. Equality is legitimate only as the documented fallback when no producer release date is discoverable (e.g. a scraped page carries fresh rows but no updated stamp — see /update-dataset Guardrails, "Scraped chart embeds"); expect a .dvc comment explaining it, and flag 🟡 for the author to confirm rather than 🔴. Bare equality with no rationale: ask.
date_accessed	Updated to today (or run-date)
producer / attribution_short	Same source, same values (unless changed deliberately)
citation_full / attribution	Year bumped to the new release year — etl update copies both verbatim from the old .dvc, so a stale year ships silently. 🔴 if still the old version's year.
citation_full year vs date_published year	Warn (🟡) if they differ. The year inside citation_full (and attribution) should normally match date_published's year. A mismatch is sometimes legitimate — the producer labels the release by edition rather than publish date (e.g. UN IGME's "2025 report" published 2026-03-17, so citation_full (2025) ≠ date_published 2026) — but it's just as often a stale citation the author forgot to bump. Surface it for the author to confirm; don't silently pass it.
url_main	Status check — see step 6
url_download	Status check; OK to remove if data is now fetched via API
license.url	Status check
version_producer	Unchanged label + changed payload = in-place revision. If the producer's version label is the same as the old .dvc but the data changed, confirm the author verified the revision against the source's file-modification dates/hashes (not the label) and documented the behavior in a .dvc NOTE; date_published should be the replacement date. Missing NOTE on a known in-place reviser: 🟡.

• Freshness check for scraped snapshots. When the snapshot .py scrapes the producer's page or a chart platform's endpoint, re-fetch the producer's page and compare against the committed snapshot — the endpoint the script reads can lag the page (e.g. a Datawrapper chart CDN trailing the page's own <noscript> data tables by a full release, so the committed snapshot silently misses the newest wave). The committed data must match the page's current tables; a missing latest row/wave is a 🔴 (see /update-dataset Guardrails, "Scraped chart embeds").

6. Verify all links

Run the HEAD-check loop from /update-dataset § 6c on every URL in the new .dvc and .meta.yml files. A curl non-2xx is a signal, not proof — Cloudflare-fronted hosts return false 404s to curl. Apply the same escalation as /update-dataset § 6c: re-check with WebFetch, then the Wayback availability API — remembering that no automated signal is decisive (hosts like BLS block both curl and WebFetch while serving browsers fine, a Wayback capture is historical evidence only, and a missing capture is non-evidence). A URL that fails all automated checks is a 🟡 — needs a human browser check (report the evidence trail: statuses, capture date or absence); escalate to 🔴 only once a browser check confirms the link is dead or the producer's site documents its retirement. A curl-only failure that WebFetch resolves is 🟢 informational.

• docs.google.com 200 ≠ publicly viewable. Google Sheets/Docs links return HTTP 200 even when they're behind a permission wall (the 200 is the "request access"/sign-in page). When a user-facing description_key/description_processing links a Google Sheet, confirm real public access with WebFetch (ask whether the page shows data or a "you need access"/sign-in wall) — curl status alone will pass a private sheet.
• Cross-check the same sheet is cited consistently. If a dataset links a "source per data point" sheet from more than one field, verify they're the same sheet ID — divergent IDs (one current, one stale) is a 🟡.
• HTTP 200 ≠ anchor exists. For URLs carrying a #fragment, run the anchor pass from /update-dataset § 6c: the fragment must match an id/name attribute in the page HTML (skip non-DOM fragments: any fragment containing = or / — text fragments, gid=, page=, hash routes like FAOSTAT's #data/FBS — plus #! hashbangs). Rule out client-side rendering and Cloudflare challenge bodies (WebFetch de-slugged-heading check) before flagging. A confirmed missing anchor on an otherwise-working page is 🟡 — the page loads, the reader just lands at the top; escalate to 🔴 only if the linked section genuinely no longer exists and the link's claim depends on it.

7. Code clarity & docs

For each step file, check:

• Snapshot script (if present): docstring explains source choice; no hidden hardcoded year/date constants without --cli-flag parametrization (or at minimum a clear update comment). Note: a .py upload script is optional — many snapshots ship with only the .dvc and a url_download. Don't flag the absence of a script.
• Meadow / garden / grapher: clear top-level docstrings; no commented-out code; no silent exception handlers
• Garden: harmonization uses paths.regions.harmonize_names(tb, ...) (the new API), not the legacy geo.harmonize_countries
• Garden assertions: sanity checks present when the step does non-trivial logic (harmonization, renames, aggregations, derivations) and not overly brittle (e.g. avoid hard-coded "X must always exceed Y" if it's not a true invariant). Check value-bound coverage per indicator type (shares in [0,1], percentages-of-a-whole in [0,100], non-negativity for level indicators, mutually exclusive share categories summing to 100 within rounding tolerance, exception sets for documented outliers) — but verify any bound against the actual data before suggesting it: "% of GDP" indicators legitimately exceed 100 (see /update-dataset §5b-bis)
• Unit-branched aggregation — verify every count sums and every rate averages. When a regional-aggregation step routes rows by a unit string — e.g. counts (unit == "Number of deaths") get summed while everything else gets population-weighted-averaged — a count series carrying a different unit label silently falls into the averaging branch and produces a meaningless regional "total". (Real case: IGME summed "Number of deaths" but "Number of stillbirths" fell through to the rates path, so regional stillbirth totals became population-weighted averages — Africa showed ~60k instead of ~1M.) Enumerate the distinct units, confirm each is routed correctly (a region's count value should be ≫ any member country's, not a mid-range average), and prefer a robust predicate (unit.startswith("Number of")) over an exact match. Catches a class of bug a green pipeline + Jinja-renders-fine review will miss.
• Blanket title-based unit scaling — audit the raw range of every affected indicator. When garden scales values by pattern-matching indicator titles (e.g. "titles containing 'share' or 'percentage' get ×100"), verify the source actually stores every matched indicator in the assumed convention: compute each matched indicator's raw min/max and flag any whose range contradicts the rule (a "fraction" with raw max ≫ 1, or a "percent" with raw max ≈ 1). (Real case: WWBI's 134 "share"-titled indicators are fractions, but its 2 "percentage"-titled wage-bill ratios are already percent — sourced from IMF FAD, not WB surveys — so a blanket ×100 shipped them 100× too large across versions.) A quick output-side check: no %-unit column's max should exceed a grounded bound (~150 for genuine percentages-of-a-whole). "Same as the previous version" is not a pass — magnitude bugs are inherited; judge absolute plausibility (a wage bill is not 1,242% of GDP). 🔴 if a scaled indicator's convention is contradicted by its raw range.
• External-write helpers may be env-guarded — read the helper before flagging. An unconditional call to something like export_table_to_gsheet(...) / get_team_folder_id() in a garden/grapher step looks like a CI/deploy risk, but several OWID helpers early-return unless OWID_ENV.env_local == "dev" (so they no-op on staging/prod). Check the helper's guard before flagging — "unconditional call" ≠ "runs everywhere". If it is guarded, it's at most a 🟢/style note (intent could be made explicit at the call site; the dev-only side-effect can leave the exported artifact stale relative to prod), not a blocker.
• Grapher meta.yml: drop it if it only duplicates the garden values — the grapher step inherits via default_metadata=ds_garden.metadata

8. Outdated practices

Run the /check-outdated-practices skill on every new step file (snapshot, meadow, garden, and any helper modules like *_omms.py). It reads vscode_extensions/detect-outdated-practices/src/extension.ts as the single source of truth and greps the full pattern set — don't hand-maintain a copy of the patterns here, and don't eyeball helper calls and decide they look current (the geo.add_* family looks fine but is flagged). Report every hit it returns as 🟡.

Separately, the metadata/origin-stripping patterns from CLAUDE.md (pd.concat→pr.concat, pd.to_numeric/pd.to_datetime→pr.*, np.where, index.map(...), pd.DataFrame(tb) re-wrap) are not part of the extension — they're covered by the §7 code-clarity pass. Flag them there even when copy_metadata/fillna appears to mitigate.

8b. Carried-over annotations & sanity_checks (review side)

/update-dataset steps 1c+6a (annotations) and 1d+5b (sanity_checks) define the catalog/resolve procedure. As reviewer, verify the outcome:

• Annotations: scan the diff for any # NOTE: / # TODO: / # FIXME: / # HACK: / # XXX: that are unchanged from the old version. For each, confirm the PR body mentions whether the workaround is still needed, or that it was deleted with its code. Unresolved + undocumented = 🟡.
• Sanity-check log flags: grep the diff for SHOW_SANITY_CHECK_LOGS, DEBUG, LONG_FORMAT set to True. If a debug flag was left enabled, that's a 🔴 — must be reverted.
• Silent deletes: in any sanity_checks function, scan for drop, filter, tb = tb[...] — row removals that the user might miss. Make sure the PR body lists them.
• Findings surfaced, not just flags reverted: if the step has any sanity-check logic (function or inline # Sanity check block), the PR body should carry a "Sanity-check findings" section reporting what the checks said on the new data. A green pipeline run is not proof the invariants held — checks that paths.log.warning(...)/.critical(...) instead of assert/raise pass silently. If the new garden chain has logging-style checks and the PR body has no findings section, re-run the garden step (--private --force --only) and scan stdout/stderr for warning, dropped, outlier, AssertionError. Undocumented findings = 🟡; a check that newly raises on the new data = 🔴 (must be triaged with the author per /update-dataset §5b).

8c. Country harmonization audit (review side)

/update-dataset §5c defines the full audit (validate .countries.json targets against the canonical regions + income-groups catalogs, audit .excluded_countries.json, scan the garden log for the three warnings, and confirm garden-output entities are canonical). As reviewer, verify the outcome — every entity reaching Grapher must be canonical, and any that isn't must be documented in the PR body.

Run after the §4 pipeline build. Three checks:

1. Garden log warnings. Re-run the garden step capturing output and scan for the three stable warning strings:

   .venv/bin/etlr data://garden/<namespace>/<new_version>/<short_name> --force --only \
       > /tmp/<short_name>_harmon.log 2>&1
   rg -n "missing values in mapping\.|unused values in mapping\.|Unknown country names in excluded countries file:" /tmp/<short_name>_harmon.log
   

   missing values in mapping (source countries not in .countries.json) is the actionable one — 🟡 unless the PR body documents the gap. unused values in mapping / Unknown … excluded are informational 🟢.

2. Garden-output entities are canonical. This is the check that catches inline tb["country"] = "…" assignments and post-harmonization mutations the .countries.json review can't see. This one needs a local build — entity lists aren't in MySQL (modern grapher stores indicator data outside the DB), so make query can't answer it; build the garden step and load it with owid.catalog.Dataset("data/garden/<ns>/<v>/<short>"). Build the canonical set (regions + latest income groups) and diff against the entities actually in the built garden tables — see the Python snippet in /update-dataset §5c (Python checks #3 + #5). Note geo.REGIONS already includes the four WB income groups, so an isin(REGIONS) filter de-dups them too. Any entity in the garden output that isn't in canonical regions or income groups is 🔴 unless it's a legitimately custom source aggregate (e.g. " (ILO)"/" (WB)"-suffixed regions, BRICS, G7) that the PR body explicitly notes lives outside the canonical system.

3. Over-exclusion. If .excluded_countries.json exists, flag any entry that is a canonical region/aggregate (/update-dataset §5c Python check #4) — dropping a real country/region silently is 🟡 unless the PR body says why (e.g. source double-counts "World").

If the garden step doesn't use the harmonizer at all (no .countries.json; country assigned inline), checks #2 and #3 still apply — #2 is the only thing that catches non-canonical inline values.

8c-bis. Did another dataset update merge while this PR was open?

Cheap and worth doing on any PR more than a few days old. A branch serves its own snapshot of every other dataset, so if another update merged to master meanwhile, the branch's staging is behind on that dataset — and any chart combining both differs from production on two axes. Approving it syncs the stale config back and reverts the other update on a published chart. Nothing flags this: CI is green and the chart renders.

Check git log HEAD..origin/master --oneline for 📊 dataset commits; for each, look for charts carrying indicators from both datasets and compare every dimension's dataset version, staging vs production — not just the dimension this PR touches. The fix is merging master in and remapping the affected charts' foreign dimensions; charts using only the other dataset are out of chart-diff scope and never sync, so they're correctly left alone. 🔴 if a shared chart would regress.

8d. Empty-entity audit (optional to run — always offer it)

The author-side audit is optional to run in /update-dataset (the check-empty-entities skill sweeps every chart/MDim/explorer/narrative/gdoc surface, which can consume many tokens) — so a missing audit is not a finding. If the author ran it, verify the outcome: a selection that had data on production but none on staging is a 🔴 regression from the update; a gap identical on production is 🟡 pre-existing — it still needs fixing (chart-config edit or content follow-up on the gdoc), just not necessarily in this PR, so confirm the PR body documents it and a fix is planned.

If the author didn't run it, you MUST offer it to the user as an optional add-on to this review (name the token cost) — surfacing this offer is mandatory, never silently skip it — and recommend accepting when the risk is real: many charts remapped, hand-curated (non-auto) mappings, a restructure, or indicators whose country coverage shrank. Run the full sweep on opt-in.

For a cheap version of the same question — which surfaces carry this dataset at all, without the per-view availability checks — run find-chart-references --dataset-id <id>. It's the surface list both step-7 audits are built on, and it answers "did the author miss a surface entirely" in one query.

Either way, do a cheap manual spot-check as part of the base review: open 2–3 of the most-viewed upgraded charts on staging (SVG render is enough) and confirm their pinned entity selections still draw lines — an empty published chart is a 🔴 however it's found, and a spot-check hit is itself a reason to recommend the full sweep.

8e. Hardcoded-time-bounds audit (standard)