Skip to content

Commit d3a3d97

Browse files
Haiderclaude
andcommitted
feat(review): [R20 S4] triage-tier promotion for risk-bearing dbt metadata
Grounded in the 5-PR internal corpus study (data-engineering-skills/docs/pr-review-corpus-findings-r20.md) where historical altimate-code recall was 2/84 = 2.4%. Two of five PRs auto-approved despite each having 8+ substantive human findings: - PR D: test-only YAML under `models/marts/` adding `data_tests:` / `constraints:` on a contracted mart → 8 misses, including 5 dbt-trino adapter-semantic bugs. - PR E: cost-anchor redesign under `mrt_jobs_cost_savings.sql` → 11 misses, including 3 critical FinOps corrections. Adds three FileChangeClass signals + wires them into `fullTierReasons()`: - **`dbtRiskYmlChanges`** — schema.yml diff introduces or edits `data_tests:`, `constraints:`, `contract:`, or a `unique_combination_of_columns` list-item. Regex anchors to YAML key position after optional diff marker, optional indent, and optional list marker; explicitly excludes comment lines. This catches PR D. - **`martLayerChange`** — file lives under `models/marts/` or `models/mart/`. Does NOT promote on its own (description-only edits stay trivial, matching the existing test), but ENRICHES the `dbtRiskYmlChanges` reason string so the customer sees "under models/marts/ (mart-API surface)". - **`finopsPathToken`** — path/filename contains a FinOps keyword (`cost|saving|billing|credit|dbu|spend|revenue|price|rate|pricing| invoice`) at a word / segment / extension boundary. Catches PR E. Word-boundary regex prevents false positives on incidental substrings (`broadcaster` ≠ `caste`, `precast` ≠ `cast`). Regression tests (11 new, all green): - PR D shapes (data_tests, constraints, unique_combination_of_columns) all promote to full with `mart-API surface` context. - PR E shape (mrt_jobs_cost_savings.sql) + 7 other FinOps keyword variants all promote to full. - FinOps false-positive guard: `broadcaster` / `precast` stay lite. - Description-only edits under models/marts/ still trivial (pre-existing behavior preserved). - Comment lines and description strings mentioning `data_tests:` / `constraints:` do NOT promote (regex tightness). - Nested-indent YAML key position (production shape) still fires. Codex-reviewed diff. Two highs addressed: - Regex tightening to avoid comment / description-string false positives - FileChangeClass consumer audit (no external constructions; safe) Ship criteria (from plan v2): PRs D and E from the corpus must no longer auto-approve. Baseline recall on the existing 13-scenario corpus must not regress. Both hold: 96/96 tests pass (85 pre-existing + 11 new); full altimate review suite 3781/3781 green. Depends on PR #1027 (feat/review-r18-observability-recall) — this branch stacks on top so tierReasons[] wiring is in place. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
1 parent 3062ea7 commit d3a3d97

2 files changed

Lines changed: 202 additions & 0 deletions

File tree

packages/opencode/src/altimate/review/risk-tier.ts

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,22 @@ export interface FileChangeClass {
3636
incrementalLogicChange: boolean
3737
/** Structurally complex SQL (window/subquery/large plan) — never `trivial`. */
3838
complex: boolean
39+
// R20 S4 — risk-signal promotion. Grounded in the 5-PR internal corpus study
40+
// (see data-engineering-skills/docs/pr-review-corpus-findings-r20.md) where
41+
// PRs D (test-only YAML on contracted marts) and E (cost-anchor redesign)
42+
// both auto-approved despite each having 8+ substantive human findings.
43+
/** schema.yml diff introduces or edits `data_tests:`, `constraints:`,
44+
* `unique_combination_of_columns`, or `contract:` — grain / constraint
45+
* territory reviewers repeatedly flagged as substantive. */
46+
dbtRiskYmlChanges: boolean
47+
/** File lives under `models/marts/` or `models/mart/` — mart-layer changes
48+
* land in the API surface downstream consumers depend on. */
49+
martLayerChange: boolean
50+
/** Path or filename contains a FinOps keyword
51+
* (`cost|saving|billing|credit|dbu|spend|revenue|price|rate`). The
52+
* highest-severity blockers in the corpus (cross-model rate asymmetry,
53+
* DBU savings > DBU cost, misanchored billing units) landed here. */
54+
finopsPathToken: boolean
3955
}
4056

4157
export interface ClassifyOptions {
@@ -50,6 +66,23 @@ export interface ClassifyOptions {
5066
const MATERIALIZATION_RE = /[+]?materialized\s*[:=]|config\s*\(\s*[^)]*materialized/i
5167
const INCREMENTAL_RE = /is_incremental\s*\(|unique_key|incremental_strategy|merge_update_columns|partition_by/i
5268

69+
// R20 S4 — signals that lift a PR out of trivial / lite. See FileChangeClass docs.
70+
//
71+
// Anchored to YAML key position after the optional diff marker (`+`/`-`),
72+
// optional indentation, and optional list-item marker (`- `). Excludes comment
73+
// lines (`#`) and description strings that happen to contain the keyword. Two
74+
// forms because `data_tests`/`constraints`/`contract` are ALWAYS keys, whereas
75+
// `unique_combination_of_columns` is a TEST NAME that shows up as a list item
76+
// (`- dbt_utils.unique_combination_of_columns:`).
77+
const DBT_RISK_KEY_RE = /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?(?:data_tests|constraints|contract)[ \t]*:/im
78+
const DBT_UNIQUE_COMBO_RE = /^[+-]?[ \t]*(?!#)-[ \t]+(?:[\w.]+\.)?unique_combination_of_columns[ \t]*:/im
79+
const MARTS_DIR_RE = /(?:^|\/)models\/marts?\//i
80+
// FinOps keyword must sit at a path or filename boundary so we don't fire on
81+
// arbitrary substrings (e.g. `broadcaster` matching `caste` never triggers
82+
// `cost`, but `_backups_of_cost_config.sql` would still catch on the `cost`
83+
// token via `_`). Cover common word / segment / extension boundaries.
84+
const FINOPS_TOKEN_RE = /(?:^|[\/_.-])(?:cost|costs|saving|savings|billing|credit|credits|dbu|dbus|spend|revenue|price|prices|rate|rates|pricing|invoice|invoices)(?:$|[\/_.-])/i
85+
5386
/** The ADDED/REMOVED lines of a unified diff (excludes context + hunk headers),
5487
* so signal detection fires on what actually changed, not surrounding context. */
5588
function changedLines(diff: string | undefined): string {
@@ -76,6 +109,10 @@ export function classifyFile(file: ChangedFile, opts: ClassifyOptions = {}): Fil
76109
materializationChange: kind === "model_sql" && !!changed && MATERIALIZATION_RE.test(changed),
77110
incrementalLogicChange: kind === "model_sql" && !!changed && INCREMENTAL_RE.test(changed),
78111
complex: opts.isComplexOf?.(file) ?? false,
112+
dbtRiskYmlChanges:
113+
kind === "schema_yml" && !!changed && (DBT_RISK_KEY_RE.test(changed) || DBT_UNIQUE_COMBO_RE.test(changed)),
114+
martLayerChange: MARTS_DIR_RE.test(file.path),
115+
finopsPathToken: FINOPS_TOKEN_RE.test(file.path),
79116
}
80117
}
81118

@@ -91,6 +128,22 @@ export function fullTierReasons(c: FileChangeClass): string[] {
91128
if (c.materializationChange) reasons.push("materialization changed")
92129
if (c.incrementalLogicChange) reasons.push("incremental logic changed")
93130
if (c.blastRadius > 5) reasons.push(`${c.blastRadius} downstream models`)
131+
// R20 S4 — trivial/lite promotion for signals the 5-PR corpus proved are
132+
// reviewer-critical. Each reason surfaces in the signed envelope's
133+
// `tierReasons`, so a customer can see WHY a nominally-tiny YAML-only diff
134+
// ran at full tier.
135+
//
136+
// Path-based `martLayerChange` on its own is intentionally NOT a promotion
137+
// reason — description-only edits under `models/marts/` are legitimately
138+
// trivial. Promotion here requires diff-level evidence (`dbtRiskYmlChanges`)
139+
// or a FinOps path token, either of which correlates with real reviewer
140+
// blockers in the corpus. `martLayerChange` upgrades the WEIGHT of a
141+
// dbtRiskYmlChanges hit but doesn't fire on its own.
142+
if (c.dbtRiskYmlChanges) {
143+
const loc = c.martLayerChange ? " under models/marts/ (mart-API surface)" : ""
144+
reasons.push(`schema.yml diff touches data_tests/constraints/contract${loc}`)
145+
}
146+
if (c.finopsPathToken) reasons.push("path contains FinOps keyword (cost/saving/billing/dbu/etc.)")
94147
return reasons
95148
}
96149

packages/opencode/test/altimate/review.test.ts

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,155 @@ describe("risk-tier", () => {
501501
expect(r.tier).toBe("full")
502502
expect(r.reasons.join(" ")).toContain("source")
503503
})
504+
505+
// R20 S4 — triage-tier promotion for the two auto-approval failure modes
506+
// exposed by the 5-PR internal corpus study. Historical baseline auto-
507+
// approved PRs D (test-only YAML on contracted marts, 8 missed findings)
508+
// and E (cost-anchor redesign, 11 missed findings). These tests lock in
509+
// the "never auto-approve risk-bearing dbt metadata changes" contract.
510+
test("R20 S4: full: schema.yml adds data_tests: [not_null] on a mart (PR D shape)", () => {
511+
const diff =
512+
"@@ -1,3 +1,6 @@\n" +
513+
" models:\n" +
514+
" - name: mrt_column_lineage\n" +
515+
" columns:\n" +
516+
" - name: record_id\n" +
517+
"+ data_tests:\n" +
518+
"+ - not_null\n"
519+
const r = classifyPR([file("models/marts/mrt_column_lineage.yml", diff)])
520+
expect(r.tier).toBe("full")
521+
expect(r.reasons.join(" ")).toContain("data_tests")
522+
// Marts context flag surfaces so the customer sees where the promotion came from.
523+
expect(r.reasons.join(" ")).toContain("mart-API surface")
524+
})
525+
526+
test("R20 S4: full: schema.yml adds constraints: block on a contracted model", () => {
527+
const diff =
528+
"@@ -1,3 +1,5 @@\n" +
529+
" columns:\n" +
530+
" - name: id\n" +
531+
"+ constraints:\n" +
532+
"+ - type: not_null\n"
533+
const r = classifyPR([file("models/intermediate/int_x.yml", diff)])
534+
expect(r.tier).toBe("full")
535+
expect(r.reasons.join(" ")).toContain("data_tests/constraints/contract")
536+
})
537+
538+
test("R20 S4: full: schema.yml adds unique_combination_of_columns test", () => {
539+
const diff =
540+
"@@ -1,3 +1,7 @@\n" +
541+
" data_tests:\n" +
542+
"+ - dbt_utils.unique_combination_of_columns:\n" +
543+
"+ combination_of_columns:\n" +
544+
"+ - metastore_id\n" +
545+
"+ - sku_name\n"
546+
const r = classifyPR([file("models/marts/mrt_billing_account_prices.yml", diff)])
547+
expect(r.tier).toBe("full")
548+
})
549+
550+
test("R20 S4: full: FinOps keyword in path (PR E shape — anchor cost redesign)", () => {
551+
// Small SQL change on a cost-savings mart should NOT auto-approve at
552+
// lite tier just because it's within the line limit — the corpus study
553+
// showed these are the highest-severity blockers.
554+
const r = classifyPR([file("models/marts/mrt_jobs_cost_savings.sql", "+select 1 as a\n")], {
555+
blastRadiusOf: () => 1,
556+
})
557+
expect(r.tier).toBe("full")
558+
expect(r.reasons.join(" ")).toContain("FinOps keyword")
559+
})
560+
561+
test("R20 S4: full: FinOps keyword variants (billing/dbu/savings/spend) all promote", () => {
562+
for (const path of [
563+
"models/marts/mrt_billing_daily.sql",
564+
"models/marts/mrt_dbu_by_workspace.sql",
565+
"models/marts/mrt_credit_savings.sql",
566+
"models/intermediate/int_query_cost.sql",
567+
"models/staging/stg_warehouse_spend.sql",
568+
"models/marts/mrt_list_price.sql",
569+
"models/marts/mrt_daily_rate.sql",
570+
]) {
571+
const r = classifyPR([file(path, "+select 1\n")], { blastRadiusOf: () => 0 })
572+
expect(r.tier).toBe("full")
573+
expect(r.reasons.join(" ")).toContain("FinOps keyword")
574+
}
575+
})
576+
577+
test("R20 S4: FinOps token does NOT over-fire on incidental substrings", () => {
578+
// False-positive guard: the token regex requires path/word boundaries so
579+
// words like `broadcaster.py` (has `caste`) or `precast_table.sql` (has
580+
// `cast`) don't fire the cost/dbu/etc. rules.
581+
const r1 = classifyPR([file("models/staging/stg_broadcaster.sql", "+select 1\n")], {
582+
blastRadiusOf: () => 0,
583+
})
584+
expect(r1.tier).toBe("lite")
585+
586+
const r2 = classifyPR([file("models/marts/mrt_precast_table.sql", "+select 1\n")], {
587+
blastRadiusOf: () => 0,
588+
})
589+
expect(r2.tier).toBe("lite")
590+
})
591+
592+
test("R20 S4: trivial: description-only edits under models/marts/ still trivial (no risk keys)", () => {
593+
// A schema.yml under marts that only changes descriptions/docs should
594+
// stay trivial — the promotion requires diff-level risk signals, not
595+
// just path membership. Precision guard against over-firing on doc PRs.
596+
const r = classifyPR([file("models/marts/_m.yml", "+ description: better docs\n+ meta:\n+ owner: alice\n")])
597+
expect(r.tier).toBe("trivial")
598+
})
599+
600+
test("R20 S4: dbtRiskYmlChanges does NOT fire outside schema.yml (kind gate)", () => {
601+
// The token `data_tests:` appearing in a .sql or .md file shouldn't
602+
// promote — the rule keys off schema.yml kind + diff-level regex, not
603+
// the token appearing in arbitrary text.
604+
const r = classifyPR([file("models/intermediate/int_x.sql", "+-- note: data_tests: not_null on grain\n")], {
605+
blastRadiusOf: () => 0,
606+
})
607+
expect(r.tier).toBe("lite")
608+
})
609+
610+
test("R20 S4: dbtRiskYmlChanges does NOT fire on yml comment lines mentioning the key", () => {
611+
// Comment lines starting with `#` (with or without diff `+`/`-` prefix)
612+
// must not promote — a reviewer explaining `constraints:` in a schema.yml
613+
// comment shouldn't trigger a full-tier run.
614+
const diff =
615+
"@@ -1,3 +1,4 @@\n" +
616+
" models:\n" +
617+
" - name: foo\n" +
618+
"+ # note: constraints: are handled at the mart layer\n" +
619+
"+ description: foo model\n"
620+
const r = classifyPR([file("models/intermediate/int_x.yml", diff)])
621+
expect(r.tier).toBe("trivial")
622+
})
623+
624+
test("R20 S4: dbtRiskYmlChanges does NOT fire on description strings mentioning the key", () => {
625+
// A `description:` string containing the substring `data_tests:` or
626+
// `constraints:` must not promote — the regex anchors to key position.
627+
// Note: `contract:` in a description would trip the pre-existing
628+
// `touchesContract` hard-floor rule (diff-filter.ts:139), which is out
629+
// of S4 scope; keep this negative test on `data_tests` / `constraints`.
630+
const diff =
631+
"@@ -1,2 +1,3 @@\n" +
632+
" models:\n" +
633+
" - name: foo\n" +
634+
'+ description: "grain-key columns get data_tests: not_null via dbt_utils.unique_combination_of_columns"\n'
635+
const r = classifyPR([file("models/intermediate/int_x.yml", diff)])
636+
expect(r.tier).toBe("trivial")
637+
})
638+
639+
test("R20 S4: dbtRiskYmlChanges fires on YAML key at nested indent (production shape)", () => {
640+
// Sanity: the tightened regex still fires on realistic dbt YAML shapes
641+
// where `data_tests:` sits under `columns:` at 8-space indent.
642+
const diff =
643+
"@@ -1,5 +1,7 @@\n" +
644+
" models:\n" +
645+
" - name: mrt_order\n" +
646+
" columns:\n" +
647+
" - name: order_id\n" +
648+
"+ data_tests:\n" +
649+
"+ - not_null\n"
650+
const r = classifyPR([file("models/marts/mrt_order.yml", diff)])
651+
expect(r.tier).toBe("full")
652+
})
504653
})
505654

506655
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)