docs(skills): add ol-dbt-migration-validation, and fix ol-dbt-local-dev's incremental rule

[quazi-h]

What are the relevant tickets?

Supports epic #2072 (dimensional-layer mart/reporting migration). Related: #2407 / DBT_WAREHOUSE_CI_QA_SPEC.md, which specs the automated CI side of the same problem.

Description (What does it do?)

Adds skills/data/ol-dbt-migration-validation, and corrects one rule in the existing ol-dbt-local-dev skill that turned out to be actively misleading.

Why a third dbt skill. The two we have drive the tools: ol-dbt-local-dev (register, build, snapshot) and ol-dbt-fast-validation (validate / impact / diff). Between them they answer "does it compile, and what breaks downstream?". Neither answers the question a migration PR actually turns on — "is the data the same?" — which is an experimental-design problem, not a tooling one. DBT_WAREHOUSE_CI_QA_SPEC.md covers the CI automation but is explicitly not a per-PR playbook for a human.

Derived by validating #2403 (marts__micromasters_dedp_exam_grades → dim_course_run) end to end, so every number in it was measured rather than reasoned about.

The load-bearing content is which local numbers count as evidence. ol-dbt local register stores whatever metadata_location Glue reports, and for a dbt-materialized Iceberg table that is a __dbt_tmp-<uuid> path — an artifact of dbt's create-temp-then-swap. Re-measured immediately after a fresh register of staging + intermediate + dimensional on 2026-09-09:

layer	views	on __dbt_tmp
raw	1,388	0
staging	256	244
intermediate	159	157
dimensional	56	55
mart	29	29
reporting	30	29
external	110	110
non-raw total	640	624 (97.5%)

Registering does not clear it — this is the steady state, not a race. Such a view either 404s (loud) or silently returns the temp table's accumulated snapshots: duplicated and partially missing rows, measured at 31x on int__mitxonline__proctored_exam_grades. So an absolute row count or fill-rate percentage read through a glue__ view is not evidence, and some have already been quoted in PR bodies as though they were. A difference between two locally built sides of the same registration is evidence, because the pollution cancels. The whole skill is organised around that distinction: register once, build both sides in one invocation, compare.

Three traps it records, each of which cost real time.

1. --full-refresh is mandatory when the model under test is incremental — and this is the ol-dbt-local-dev rule being amended. The old wording said to "reserve --full-refresh for when the incremental state is stale or wrong", which can never fire: the failure mode is a state that is perfectly valid and merely does not reflect your new code. An incremental run whose key set is unchanged is a no-op that still reports OK, so a changed expression is never re-evaluated and you read the old values believing they are new. feat(#2088): migrate marts__micromasters_dedp_exam_grades to dim_course_run #2403's own documented test command hit exactly this — dim_course_run merged in 0.11s and never re-derived the column under test, for two months. Reframed as iterating vs concluding, which is the distinction that actually decides it.

2. ~/.ol-dbt/local.duckdb is shared by every worktree and session on the machine. Mid-validation, a concurrent session rebuilt dim_course_run from main and semester went from 4,513/4,513 populated to 12/4,513, with no warning from either side. This is a second, independent reason to materialize both comparison sides in one invocation — the feat(#2088): migrate marts__micromasters_dedp_exam_grades to dim_course_run #2403 result survived only because both marts were already physical tables.

3. dbt defaults to --indirect-selection=eager, which selects relationships_* tests owned by other models and compares your local build against production Glue views. Measured on feat(#2088): migrate marts__micromasters_dedp_exam_grades to dim_course_run #2403's two changed models: 20 tests under eager, 12 under buildable, 11 under cautious — and the 9 that eager adds were all cross-model, including a 42.7M-row tfact_grade scan and every failure the PR body had been documenting as expected noise. The skill also records why expected-failure counts should never go in a PR body: feat(#2088): migrate marts__micromasters_dedp_exam_grades to dim_course_run #2403 documented 87/74/13 orphans and the same tests produced 861/178/150 plus an undocumented fourth failure, because the numbers depend on when you registered, not on the change.

Also included: the per-column multiset diff (localises a difference to one column, needs no join key), accepting on the distinct functional mapping rather than the whole row, and the rule that a column empty on both sides is unverified, not passing — which is exactly how passing_grade looked clean on an earlier #2403 diff while being just as broken as semester.

Registers the skill in agent-config.toml and adds a migration profile alongside the existing dbt one.

How can this be tested?

No code changes — two Markdown files and a TOML registration. Nothing executes.

Verify the registration is well-formed:

uv tool install 'agent-config-kit[cli]'
agent-kit validate agent-config.toml
agent-kit apply agent-config.toml --profile migration --dry-run

Frontmatter conforms to the Agent Skills spec — name matches the directory (ol-dbt-migration-validation), description states what it covers and when to trigger, license and metadata.category match the two sibling skills.

To sanity-check the central factual claim yourself, register any dbt-built layer and count how many views point at a temp location:

-- against ~/.ol-dbt/local.duckdb
select regexp_extract(glue_database, 'production_([a-z]+)$', 1) as layer,
       count(*) as views,
       sum(case when metadata_location like '%__dbt_tmp%' then 1 else 0 end) as on_dbt_tmp
from _glue_source_registry
group by 1 order by 3 desc;

And to reproduce the test-selection numbers without building anything:

cd src/ol_dbt && DBT_PROFILES_DIR=$(pwd) \
  dbt ls --select dim_course_run marts__micromasters_dedp_exam_grades \
    --resource-type test --indirect-selection=cautious -t dev_local | wc -l   # 11
# ...same with --indirect-selection=eager                                     # 20

Additional Context

The amendment to ol-dbt-local-dev is the part most worth a careful read. That skill is @tmacey's (#2425) and Rachel extended it in #2469; I have changed one of its ## Rules bullets and added a second. I could have left the new skill to disagree with it, but a reader hitting two skills that contradict each other on --full-refresh is worse off than before, and the old rule is the direct cause of #2403's instructions verifying nothing since July. Happy to split that hunk out if you would rather it landed separately.

Deliberate overlap. The new skill repeats ol-dbt-fast-validation's primary-key guidance instead of only linking to it. A reader mid-migration should not have to hop skills for the one step that silently invalidates the entire comparison if done wrong (a non-unique key pairs rows many-to-many and reports join artifacts as mismatches — one measured case read 17,782 unmatched of 20,908 rows, all artifact).

This documents a limitation rather than waiting on it. The __dbt_tmp root cause is tracked internally and is not fixed; when it is, the skill's step 5 can assert considerably more and the table above becomes historical. It names that dependency inline so the next reader knows which parts expire.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The playbook contains contradictory commands and overstates what local comparisons and row-count parity prove.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds guidance for validating dbt migrations against local production-backed data.

Changes:

• Adds a migration-validation skill and profile.
• Clarifies incremental/full-refresh guidance.
• Registers the new skill in agent configuration.

File summaries

File	Description
skills/data/ol-dbt-migration-validation/SKILL.md	Adds the validation playbook.
skills/data/ol-dbt-local-dev/SKILL.md	Updates local-development rules.
agent-config.toml	Registers the skill and migration profile.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 5
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[blarghmatey]

Looks good, but worth addressing copilot's comments before merging.

[quazi-h]

Ran the corrections against dbt instead of leaving them derived

The five fixes in eba58e6 were reasoned from reading dim_course_run.sql, override_ref.sql and git history. I've now executed them on dbt 1.12.4 / dev_local (DuckDB). All five hold. Measurements added in 562e4d3.

The incremental claim — the original skill was empirically wrong

Built three purpose-made incremental models (isolated verify schema, deleted afterwards) to separate the two competing theories. The decisive case is the third:

model	predicate	key set across runs	expression	result after incremental run
_verify_watermark	pk > max(pk)	unchanged 1..5	V1 → V2	all rows still V1, OK in 0.09s
_verify_nofilter	none	unchanged 1..5	V1 → V2	all rows V2, OK in 0.07s
_verify_scd2	SCD2 not exists	—	V1 → V2	6 rows: 3 expired V1 + 3 current V2

Rows 1 and 2 have an identical unchanged key set and opposite outcomes. That falsifies "an incremental run whose key set is unchanged is a no-op" directly, and confirms your "delete+insert can replace an existing key when the model emits it." What separates them is only the predicate — which is what the text now says.

Row 3 confirms the SCD2 behaviour I added: the prior row is expired and retained, the new derivation appended with a fresh effective_date. A plain select * returns both generations.

And the genuine no-op: re-running _verify_scd2 with the label unchanged reselected nothing — OK in 0.10s, still 6 rows. That is the #2403 signature reproduced.

dbt build → skip: confirmed, plus a nuance that makes it worse

3 of 3 SKIP relation main_verify._verify_downstream after an upstream test failed. Your point was right.

But it's intermittent here: dbt_project.yml sets +error_if: ">10", so 4 failing rows → WARN and the downstream still builds, while 24 → FAIL + SKIP. dbt build therefore appears to work until a test crosses the threshold. Documented.

Test selection on dim_course_run: eager 17 / buildable 8 / cautious 7. All 10 that eager adds are relationships_*, and 6 are owned by other models (tfact_grade, tfact_enrollment, tfact_certificate, dim_product, and both bridge_*) — exactly the mechanism.

The cancellation fix — this is the one the data argues hardest for

override_ref resolution, from compiled SQL:

from "local"."main_dimensional"."dim_course_run" as built_one              -- built locally
cross join "local"."main"."glue__ol_warehouse_production_dimensional__dim_user"  -- unbuilt -> Glue view

Then I measured duplication across all 29 registered dim_ views in one registration:

view	rows	distinct pk	ratio
dim_ocw_resource	179,863	2,908	61.85x
dim_course_run	10,884	8,508	1.28x
dim_course	4,321	4,071	1.06x
24 others	—	—	1.00x
dim_discussion_topic, dim_video	—	—	404, pointer rot

A 1.00x view and a 61.85x view in the same layer and the same registration. Duplication can only cancel between identical views — which is precisely what a migration that re-points ref() does not have. Your objection was correct and the old wording would have licensed accepting or rejecting a migration on that artifact.

Incidentally, the view the skill cited for 31x now 404s — the __dbt_tmp path was cleaned up since 2026-09-09. That is the pointer-rot half of the same claim, observed live.

Also re-measured

Pollution table five days on: 651/667 non-raw views (97.6%), every layer but staging identical to the 2026-09-09 figures — it is the steady state, as claimed.

Scope

The grain fix (count parity ≠ grain integrity) is arithmetic, not a dbt behaviour; confirmed with a 5-row counterexample where the delta is 0 while one key is dropped and another fanned out.

I did not rebuild the real dim_course_run or any production-mirror model — all dbt writes went to a throwaway verify schema, now dropped. So this verifies the semantics the skill asserts, not a re-run of #2403 itself.

[Copilot]

🟡 Changes recommended

The documented commands contain invocation errors and omit the old divergent dependency path required for a controlled comparison.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

skills/data/ol-dbt-migration-validation/SKILL.md:158

• Repository local-development commands invoke dbt through uv run (docs/LOCAL_DEV_QUICK_REF.md:17-41). Using bare dbt here can select an unrelated global executable or fail in a normal unactivated uv checkout, undermining the reproducibility of the acceptance procedure.
  skills/data/ol-dbt-migration-validation/SKILL.md:262
• As with the build step, run dbt through the repository's uv environment; otherwise the validation may use a different dbt/adapter version from the one locked by this project or fail when the virtual environment is not activated (docs/LOCAL_DEV_QUICK_REF.md:32-35).

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

[Copilot]

🟡 Changes recommended

The playbook has unreliable ref discovery, incorrect key guidance, and insufficient protection against shared DuckDB races.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

skills/data/ol-dbt-migration-validation/SKILL.md:359

• This recommendation is reversed: ol-dbt diff's per-column comparison joins on the supplied key, so a non-unique key (or nullable scalar key) is exactly what can fan out or mispair rows and corrupt the mismatch rate. Conversely, the top-level “Rows without an exact match” count comes from compare_relations, where the key is used only for ordering (src/ol_dbt_cli/ol_dbt_cli/commands/diff.py:415-425,450-480). If the key is not proven, skip the keyed diff and rely on the keyless multiset check in step 5(b).

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🟡 Changes recommended

Key discovery misses nested mart schemas, and registration failures can leave validation using stale inputs.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

[Copilot]

🟡 Changes recommended

Several documented commands have path, registration, and control-flow errors that can invalidate or prevent validation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

skills/data/ol-dbt-migration-validation/SKILL.md:210

• These arguments are model basenames, but the files created in step 2 live under src/ol_dbt/models/...; at this point the procedure has not changed into that directory. Following the steps from the repository root makes both grep calls fail and the two empty outputs appear identical, silently omitting all divergent upstreams. Pass the actual paths created in step 2.
  skills/data/ol-dbt-local-dev/SKILL.md:27
• The new prerequisite says every command below must use uv run --frozen, but most subsequent copyable examples still invoke bare ol-dbt (including run and diff, which invoke dbt). Those are exactly the commands that can pick up the shadowing global executable this section warns about. Either update every example or explicitly require activating .venv before following the bare-command examples.
  skills/data/ol-dbt-migration-validation/SKILL.md:180
• A full ancestor tree does not necessarily terminate at raw sources. override_source.sql:36-53 derives the Glue database from each source's declared schema, and this repository has dimensional and reporting sources as well. Register every source layer reached by the selected graph; otherwise a valid +<upstream> build can still fail because the required non-raw Glue view was never registered.

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

[quazi-h]

Addressed the three suppressed comments from this round's review body as well, in the same commit (8d30b83) — noting them here since they have no threads to reply on:

SKILL.md:210 — ref-diff took basenames. Correct, and it fails silently in the clean direction: step 2 writes those files under src/ol_dbt/models/..., so from the repo root both greps fail, two empty outputs diff as identical, and you read that as "no divergent refs" and omit the upstreams. Same root cause as the patch_path thread; both are now anchored to the repo root, which is stated once at the top of the procedure.

SKILL.md:180 — ancestor trees do not necessarily end at raw. Verified: this project declares dimensional and reporting sources alongside ol_warehouse_raw_data, and override_source derives the Glue database from each source's declared schema. The note said "register raw as well"; it now says register every source layer the selection reaches, with a command to list them:

DBT_PROFILES_DIR=src/ol_dbt uv run --frozen dbt ls --project-dir src/ol_dbt \\
  --select +<upstream> --resource-type source -t dev_local | grep '^source:'

ol-dbt-local-dev:27 — the prerequisite contradicted its own examples. Fair. I had decided to state the uv run --frozen rule once rather than prefix twenty lines, but that left every copyable example bare, including ol-dbt run and ol-dbt diff, which are exactly the ones that shell out to dbt. Taking your second option: the prerequisite now says to activate the venv once (verified — which dbt then resolves to this repo's 1.12.4), with the per-command prefix as the alternative, and says plainly that doing neither is what breaks.

Worth recording that two of this round's six findings were commands I had shipped without running as written — including one where I tested grep \"$yml\" and committed grep \"src/ol_dbt/$yml\". Every snippet changed in 8d30b83 was executed verbatim from the stated working directory before commit.

[Copilot]

🟡 Changes recommended

Several documented commands either fail from the required working directory or do not enforce the stated test and filtering constraints.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

skills/data/ol-dbt-migration-validation/SKILL.md:437

• This grep scans every uniqueness test in the shared schema file rather than the selected model's YAML block. Files such as _marts__combined__models.yml contain many models and many unrelated uniqueness tests, so this output does not identify which key belongs to <model> and can lead directly to the weak-key fan-out warned about below. Scope extraction to the selected model block (and include its complete test config/where) or instruct the reader to inspect that block directly.

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

[Copilot]

🟡 Changes recommended

Several documented checks can silently pass invalid comparisons or fail when executed as instructed.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (7)

skills/data/ol-dbt-migration-validation/SKILL.md:354

• This checker does not implement the gate described below. It prints NOT OK: none when the selector matched no comparison models, and it always exits 0 even when bad is non-empty. Explicitly require both expected model names to have success status and exit nonzero otherwise.

import json; d=json.load(open('src/ol_dbt/target/run_results.json'))
BAD={'error','fail','skipped','runtime error'}
bad=[(r['status'], r['unique_id'].split('.')[-1]) for r in d['results'] if r['status'] in BAD]
print('NOT OK:', bad or 'none')
"

skills/data/ol-dbt-migration-validation/SKILL.md:556

• The key-discovery pipeline cannot find a single-column key: its grep only emits compound-test syntax, while the following rule requires locating unique plus not_null. For example, models/dimensional/_dim_date.yml:11-15 declares date_key using those two generic tests, and this command produces no key evidence for it. Include column names and the generic test names in the scoped output.

awk -v m="<model>" '$0=="- name: "m {f=1; print; next} f && /^- name: / {exit} f' \
  "src/ol_dbt/$yml" \
  | grep -nE 'unique_combination_of_columns|expect_compound_columns_to_be_unique|combination_of_columns|column_list|where:'

skills/data/ol-dbt-migration-validation/SKILL.md:665

• This query omits the <pred> that lines 649-650 require. For a conditionally unique model such as dim_course_run, it therefore compares historical rows too, so the reported per-column differences do not describe the same population as the grain and mapping checks.

select count(*) from (select "<col>" from <pre> except all select "<col>" from <new>);
select count(*) from (select "<col>" from <new> except all select "<col>" from <pre>);

**skills/data/ol-dbt-migration-validation/SKILL.md:692**
* The fill-rate example also drops the required `<pred>`. This makes its denominator include rows outside the population where the selected key is proven, contradicting lines 582-591 and potentially hiding a current-row fill-rate regression among SCD2 history.

select 'pre' as side, count() as n_rows,
count("") as non_null, round(100.0count("")/count(),2) as pct
from

union all select 'new', count(), count(""), round(100.0count("")/count(),2) from ;

**skills/data/ol-dbt-migration-validation/SKILL.md:742**
* These two checks do not cover what a row-level diff reports. Per-column multisets lose key-to-value association, and step 6 checks only `<changed_col>`; for example, swapping an untouched value between two keys leaves every multiset, row count, and changed-column mapping unchanged. Add a filtered full-row `EXCEPT ALL` comparison (excluding nondeterministic columns), or filtered wrapper models, before claiming the keyed diff can be skipped without loss.

You lose nothing by skipping it. Step 5(b)'s multiset diff needs no key at all, and

step 6's mapping assertion is hand-written SQL — both take a where directly, so

together they cover what 5(d) would have told you, on a key you can actually

defend. Reach for ol-dbt diff when the key is proven across the whole relation.
**skills/data/ol-dbt-migration-validation/SKILL.md:751**
* The procedure mandates repository-root execution, but this bare `dbt run` has neither `--project-dir` nor `DBT_PROFILES_DIR`; from the root it fails because there is no root `dbt_project.yml`. Anchor the negative-control command like the earlier dbt commands.

uv run --frozen dbt run -t dev_local --full-refresh --select "_pre"
**skills/data/ol-dbt-migration-validation/SKILL.md:251**
* Calling a snapshot an alternative conflicts with this procedure's same-registration requirement. If the current model was built under an older registration and the edited model follows step 1 by re-registering, the frozen baseline and new build use different inputs, so the diff can attribute source drift to code. Specify the fresh-register/build/snapshot/no-reregister sequence, or restrict this alternative to non-acceptance iteration.

ol-dbt local snapshot <model> --as <model>_baseline is the alternative when

the old code is not in git (an uncommitted edit) — it freezes the current build

as a plain table, immune to pointer rot. Then diff with --old-raw. Prefer the

_pre model file when the old code is a commit away, which for a migration PR

it always is.

- **Files reviewed:** 3/3 changed files
- **Comments generated:** 4
- **Review effort level:** Balanced (auto)
</details>

> [!NOTE]
> Copilot is running an experiment and ran this review at Balanced.