migration-reviewer

# `migration-reviewer` — side-by-side migrated vs source diff Code-review specialist for Databricks → AIDP migrated notebooks. DO NOT modify the notebook. Read the migrated `.ipynb` + the corresponding cell of the SOURCE notebook side-by-side and call out drift. ## When to use - After [`aidp-migrate-job`](../aidp-migrate-job/SKILL.md) completes — especially on the first few notebooks of a new workload before scaling out. - When [`migration-status`](../migration-status/SKILL.md) says PASS but the user wants a second-opinion correctness check (PASS = "all cells ran clean", not "the logic is right"). - Before signing off a migration in a regulated environment. ## Inputs - Path to the migrated `.ipynb` (under `<output-base>/<job>/notebooks/...`). - (Optional) Path to the original Databricks source notebook for side-by-side diff. If unavailable, do best-effort review of the migrated file in isolation. - (Optional) The relevant `JOB_REPORT.md` for cell pass/fail status — read this to know which cells the migrator already considered "OK". ## What you check For each cell: 1. **Write-mode drift.** Original had `.mode("overwrite")` — migrated has `.mode("append")` or vice versa. 2. **Schema drift.** Original wrote 12 columns — migrated writes 11 or 13. 3. **Path drift.** Original `s3://bucket/path` — migrated should be `oci://bucket@ns/path`. Flag any leftover `s3://` paths. 4. **Hardcoded identifiers.** Customer-specific catalog / schema names hardcoded in literals that the migrator was supposed to remap via `--catalog-manifest`. 5. **Dead Databricks-isms.** `dbutils.fs.*`, `dbutils.secrets.*`, `dbutils.widgets.*`, `dbutils.notebook.run` left in unchanged (should be rewritten or shimmed via aidp_compat). 6. **Wrong `%sql` cell handling.** AIDP supports `%sql` natively — flag any conversion to `spark.sql(...)` that lost SQL features (e.g. multi-statement, comment lines). 7. **Builtins shadowing.** `from pyspark.sql.functions import *` left in WITHOUT a corresponding `import builtins` and re-aliased `sum`. 8. **Comment-only cells gone wrong.** Cells reduced to `pass # AIDP: empty cell guard` — these are migrator stubs left when a cell couldn't be fixed; flag for review. 9. **`%run` paths.** Should be absolute on AIDP, not relative — flag leftover `./` or `../` prefixes. 10. **Output schema migration.** `.write.format("delta")` is kept as-is — AIDP supports Delta natively and the migrator preserves source format. Flag the INVERSE case: a notebook that was originally written for Delta but got rewritten to `.write.format("parquet")` during migration (which would downgrade Delta semantics). Surface as a candidate cell for a fixup_cell rewind. ## Output template ```markdown # Migration review: <notebook-name> ## Source vs Migrated diff - Source: <path or "not provided"> - Migrated: <path> - JOB_REPORT verdict: PASS / PARTIAL / FAIL — overall cell count: X OK, Y Failed, Z Fixed ## Findings ### Blockers (must fix before sign-off) - Cell N: <description> — <recommended fix> - ... ### Important (should fix) - Cell N: <description> - ... ### Style / nit - Cell N: <description> ### Good - Note specific successful rewrites worth confirming (e.g. "Cell 12 correctly rewrote `dbutils.widgets.get` → `oidlUtils.parameters.getParameter`") ## Recommendation CLEAN to sign off / NEEDS minor fix / NEEDS structural rework ## Where to act next For each blocker, point at the right skill: - Cell N → invoke aidp-fixup-cell with `why="<the specific issue>"` - Cell N → manual edit (this isn't an auto-fixable pattern); see references/gotchas.md #<n> ``` ## Method 1. **Load the migrated .ipynb** and parse `cells[].source`. 2. **Load the source .ipynb** if available; parse same way. 3. **Pair cells by index** (best-effort; the migrator preserves order). If counts differ, note that explicitly. 4. **For each pair**, run a checklist (above) and emit findings. 5. **Cross-reference** findings to `references/gotchas.md` so the user has a specific fix path. ## Boundaries - Do NOT execute either notebook. - Do NOT modify either notebook. - Do NOT speculate about runtime data quality — that's a separate (data-quality) review. - If a finding is on the boundary ("might be fine, might be wrong"), categorize as Important and explain BOTH ways. Don't pretend false certainty. ## When to escalate back to the user - The migrated notebook has > 50% of cells stubbed to `pass` — the migrator gave up; needs structural rework, not cell-level fixes. - A blocker requires changing the SOURCE (not just the migrated copy) — flag that explicitly. - A blocker requires installing a library on the cluster (e.g. Delta connector) — the user must act, this skill cannot.