From d3a3d97548b24a07873fc02f9dbd00a4892fb42d Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 22 Jul 2026 03:04:00 +0530 Subject: [PATCH 01/10] feat(review): [R20 S4] triage-tier promotion for risk-bearing dbt metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of --- .../opencode/src/altimate/review/risk-tier.ts | 53 +++++++ .../opencode/test/altimate/review.test.ts | 149 ++++++++++++++++++ 2 files changed, 202 insertions(+) diff --git a/packages/opencode/src/altimate/review/risk-tier.ts b/packages/opencode/src/altimate/review/risk-tier.ts index 20586f24fc..79d55c6edb 100644 --- a/packages/opencode/src/altimate/review/risk-tier.ts +++ b/packages/opencode/src/altimate/review/risk-tier.ts @@ -36,6 +36,22 @@ export interface FileChangeClass { incrementalLogicChange: boolean /** Structurally complex SQL (window/subquery/large plan) — never `trivial`. */ complex: boolean + // R20 S4 — risk-signal promotion. Grounded in the 5-PR internal corpus study + // (see data-engineering-skills/docs/pr-review-corpus-findings-r20.md) where + // PRs D (test-only YAML on contracted marts) and E (cost-anchor redesign) + // both auto-approved despite each having 8+ substantive human findings. + /** schema.yml diff introduces or edits `data_tests:`, `constraints:`, + * `unique_combination_of_columns`, or `contract:` — grain / constraint + * territory reviewers repeatedly flagged as substantive. */ + dbtRiskYmlChanges: boolean + /** File lives under `models/marts/` or `models/mart/` — mart-layer changes + * land in the API surface downstream consumers depend on. */ + martLayerChange: boolean + /** Path or filename contains a FinOps keyword + * (`cost|saving|billing|credit|dbu|spend|revenue|price|rate`). The + * highest-severity blockers in the corpus (cross-model rate asymmetry, + * DBU savings > DBU cost, misanchored billing units) landed here. */ + finopsPathToken: boolean } export interface ClassifyOptions { @@ -50,6 +66,23 @@ export interface ClassifyOptions { const MATERIALIZATION_RE = /[+]?materialized\s*[:=]|config\s*\(\s*[^)]*materialized/i const INCREMENTAL_RE = /is_incremental\s*\(|unique_key|incremental_strategy|merge_update_columns|partition_by/i +// R20 S4 — signals that lift a PR out of trivial / lite. See FileChangeClass docs. +// +// Anchored to YAML key position after the optional diff marker (`+`/`-`), +// optional indentation, and optional list-item marker (`- `). Excludes comment +// lines (`#`) and description strings that happen to contain the keyword. Two +// forms because `data_tests`/`constraints`/`contract` are ALWAYS keys, whereas +// `unique_combination_of_columns` is a TEST NAME that shows up as a list item +// (`- dbt_utils.unique_combination_of_columns:`). +const DBT_RISK_KEY_RE = /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?(?:data_tests|constraints|contract)[ \t]*:/im +const DBT_UNIQUE_COMBO_RE = /^[+-]?[ \t]*(?!#)-[ \t]+(?:[\w.]+\.)?unique_combination_of_columns[ \t]*:/im +const MARTS_DIR_RE = /(?:^|\/)models\/marts?\//i +// FinOps keyword must sit at a path or filename boundary so we don't fire on +// arbitrary substrings (e.g. `broadcaster` matching `caste` never triggers +// `cost`, but `_backups_of_cost_config.sql` would still catch on the `cost` +// token via `_`). Cover common word / segment / extension boundaries. +const FINOPS_TOKEN_RE = /(?:^|[\/_.-])(?:cost|costs|saving|savings|billing|credit|credits|dbu|dbus|spend|revenue|price|prices|rate|rates|pricing|invoice|invoices)(?:$|[\/_.-])/i + /** The ADDED/REMOVED lines of a unified diff (excludes context + hunk headers), * so signal detection fires on what actually changed, not surrounding context. */ function changedLines(diff: string | undefined): string { @@ -76,6 +109,10 @@ export function classifyFile(file: ChangedFile, opts: ClassifyOptions = {}): Fil materializationChange: kind === "model_sql" && !!changed && MATERIALIZATION_RE.test(changed), incrementalLogicChange: kind === "model_sql" && !!changed && INCREMENTAL_RE.test(changed), complex: opts.isComplexOf?.(file) ?? false, + dbtRiskYmlChanges: + kind === "schema_yml" && !!changed && (DBT_RISK_KEY_RE.test(changed) || DBT_UNIQUE_COMBO_RE.test(changed)), + martLayerChange: MARTS_DIR_RE.test(file.path), + finopsPathToken: FINOPS_TOKEN_RE.test(file.path), } } @@ -91,6 +128,22 @@ export function fullTierReasons(c: FileChangeClass): string[] { if (c.materializationChange) reasons.push("materialization changed") if (c.incrementalLogicChange) reasons.push("incremental logic changed") if (c.blastRadius > 5) reasons.push(`${c.blastRadius} downstream models`) + // R20 S4 — trivial/lite promotion for signals the 5-PR corpus proved are + // reviewer-critical. Each reason surfaces in the signed envelope's + // `tierReasons`, so a customer can see WHY a nominally-tiny YAML-only diff + // ran at full tier. + // + // Path-based `martLayerChange` on its own is intentionally NOT a promotion + // reason — description-only edits under `models/marts/` are legitimately + // trivial. Promotion here requires diff-level evidence (`dbtRiskYmlChanges`) + // or a FinOps path token, either of which correlates with real reviewer + // blockers in the corpus. `martLayerChange` upgrades the WEIGHT of a + // dbtRiskYmlChanges hit but doesn't fire on its own. + if (c.dbtRiskYmlChanges) { + const loc = c.martLayerChange ? " under models/marts/ (mart-API surface)" : "" + reasons.push(`schema.yml diff touches data_tests/constraints/contract${loc}`) + } + if (c.finopsPathToken) reasons.push("path contains FinOps keyword (cost/saving/billing/dbu/etc.)") return reasons } diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index 17e1ae9788..99f8394730 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -501,6 +501,155 @@ describe("risk-tier", () => { expect(r.tier).toBe("full") expect(r.reasons.join(" ")).toContain("source") }) + + // R20 S4 — triage-tier promotion for the two auto-approval failure modes + // exposed by the 5-PR internal corpus study. Historical baseline auto- + // approved PRs D (test-only YAML on contracted marts, 8 missed findings) + // and E (cost-anchor redesign, 11 missed findings). These tests lock in + // the "never auto-approve risk-bearing dbt metadata changes" contract. + test("R20 S4: full: schema.yml adds data_tests: [not_null] on a mart (PR D shape)", () => { + const diff = + "@@ -1,3 +1,6 @@\n" + + " models:\n" + + " - name: mrt_column_lineage\n" + + " columns:\n" + + " - name: record_id\n" + + "+ data_tests:\n" + + "+ - not_null\n" + const r = classifyPR([file("models/marts/mrt_column_lineage.yml", diff)]) + expect(r.tier).toBe("full") + expect(r.reasons.join(" ")).toContain("data_tests") + // Marts context flag surfaces so the customer sees where the promotion came from. + expect(r.reasons.join(" ")).toContain("mart-API surface") + }) + + test("R20 S4: full: schema.yml adds constraints: block on a contracted model", () => { + const diff = + "@@ -1,3 +1,5 @@\n" + + " columns:\n" + + " - name: id\n" + + "+ constraints:\n" + + "+ - type: not_null\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("full") + expect(r.reasons.join(" ")).toContain("data_tests/constraints/contract") + }) + + test("R20 S4: full: schema.yml adds unique_combination_of_columns test", () => { + const diff = + "@@ -1,3 +1,7 @@\n" + + " data_tests:\n" + + "+ - dbt_utils.unique_combination_of_columns:\n" + + "+ combination_of_columns:\n" + + "+ - metastore_id\n" + + "+ - sku_name\n" + const r = classifyPR([file("models/marts/mrt_billing_account_prices.yml", diff)]) + expect(r.tier).toBe("full") + }) + + test("R20 S4: full: FinOps keyword in path (PR E shape — anchor cost redesign)", () => { + // Small SQL change on a cost-savings mart should NOT auto-approve at + // lite tier just because it's within the line limit — the corpus study + // showed these are the highest-severity blockers. + const r = classifyPR([file("models/marts/mrt_jobs_cost_savings.sql", "+select 1 as a\n")], { + blastRadiusOf: () => 1, + }) + expect(r.tier).toBe("full") + expect(r.reasons.join(" ")).toContain("FinOps keyword") + }) + + test("R20 S4: full: FinOps keyword variants (billing/dbu/savings/spend) all promote", () => { + for (const path of [ + "models/marts/mrt_billing_daily.sql", + "models/marts/mrt_dbu_by_workspace.sql", + "models/marts/mrt_credit_savings.sql", + "models/intermediate/int_query_cost.sql", + "models/staging/stg_warehouse_spend.sql", + "models/marts/mrt_list_price.sql", + "models/marts/mrt_daily_rate.sql", + ]) { + const r = classifyPR([file(path, "+select 1\n")], { blastRadiusOf: () => 0 }) + expect(r.tier).toBe("full") + expect(r.reasons.join(" ")).toContain("FinOps keyword") + } + }) + + test("R20 S4: FinOps token does NOT over-fire on incidental substrings", () => { + // False-positive guard: the token regex requires path/word boundaries so + // words like `broadcaster.py` (has `caste`) or `precast_table.sql` (has + // `cast`) don't fire the cost/dbu/etc. rules. + const r1 = classifyPR([file("models/staging/stg_broadcaster.sql", "+select 1\n")], { + blastRadiusOf: () => 0, + }) + expect(r1.tier).toBe("lite") + + const r2 = classifyPR([file("models/marts/mrt_precast_table.sql", "+select 1\n")], { + blastRadiusOf: () => 0, + }) + expect(r2.tier).toBe("lite") + }) + + test("R20 S4: trivial: description-only edits under models/marts/ still trivial (no risk keys)", () => { + // A schema.yml under marts that only changes descriptions/docs should + // stay trivial — the promotion requires diff-level risk signals, not + // just path membership. Precision guard against over-firing on doc PRs. + const r = classifyPR([file("models/marts/_m.yml", "+ description: better docs\n+ meta:\n+ owner: alice\n")]) + expect(r.tier).toBe("trivial") + }) + + test("R20 S4: dbtRiskYmlChanges does NOT fire outside schema.yml (kind gate)", () => { + // The token `data_tests:` appearing in a .sql or .md file shouldn't + // promote — the rule keys off schema.yml kind + diff-level regex, not + // the token appearing in arbitrary text. + const r = classifyPR([file("models/intermediate/int_x.sql", "+-- note: data_tests: not_null on grain\n")], { + blastRadiusOf: () => 0, + }) + expect(r.tier).toBe("lite") + }) + + test("R20 S4: dbtRiskYmlChanges does NOT fire on yml comment lines mentioning the key", () => { + // Comment lines starting with `#` (with or without diff `+`/`-` prefix) + // must not promote — a reviewer explaining `constraints:` in a schema.yml + // comment shouldn't trigger a full-tier run. + const diff = + "@@ -1,3 +1,4 @@\n" + + " models:\n" + + " - name: foo\n" + + "+ # note: constraints: are handled at the mart layer\n" + + "+ description: foo model\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("trivial") + }) + + test("R20 S4: dbtRiskYmlChanges does NOT fire on description strings mentioning the key", () => { + // A `description:` string containing the substring `data_tests:` or + // `constraints:` must not promote — the regex anchors to key position. + // Note: `contract:` in a description would trip the pre-existing + // `touchesContract` hard-floor rule (diff-filter.ts:139), which is out + // of S4 scope; keep this negative test on `data_tests` / `constraints`. + const diff = + "@@ -1,2 +1,3 @@\n" + + " models:\n" + + " - name: foo\n" + + '+ description: "grain-key columns get data_tests: not_null via dbt_utils.unique_combination_of_columns"\n' + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("trivial") + }) + + test("R20 S4: dbtRiskYmlChanges fires on YAML key at nested indent (production shape)", () => { + // Sanity: the tightened regex still fires on realistic dbt YAML shapes + // where `data_tests:` sits under `columns:` at 8-space indent. + const diff = + "@@ -1,5 +1,7 @@\n" + + " models:\n" + + " - name: mrt_order\n" + + " columns:\n" + + " - name: order_id\n" + + "+ data_tests:\n" + + "+ - not_null\n" + const r = classifyPR([file("models/marts/mrt_order.yml", diff)]) + expect(r.tier).toBe("full") + }) }) // --------------------------------------------------------------------------- From 7225507fcfb995f39d223e6aaa0cf6ff3d549dc5 Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 22 Jul 2026 19:03:49 +0530 Subject: [PATCH 02/10] fix(review): [R20 S4] address consensus-review findings on triage promotion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundle addresses PR #1028's consensus review: - MAJOR #1 — legacy `tests:` (pre-dbt-1.8 alias) added to risk-key patterns so `models/marts/*.yml` diffs adding `tests: [not_null]` or similar promote out of trivial. Regression tests cover both marts and non-marts placement + a word-boundary FP guard. - MAJOR #3 — vacuous `unique_combination_of_columns` regression test fixed. Original path (`mrt_billing_account_prices.yml`) also matched FinOps + marts signals so the DBT_UNIQUE_COMBO_RE could have been deleted and the test still passed. Path moved to `models/intermediate/int_grain.yml` (non-mart, non-FinOps) and reason string asserted to confirm the combo regex is the sole triggering signal. - MINOR #2 — dropped `dbus` from FINOPS_TOKEN_RE (D-Bus IPC collision with `stg_dbus_connector.sql`). Singular `dbu` is sufficient. - MINOR #4 — each risk-YAML key now has its own regex in a DBT_RISK_KEY_PATTERNS table. `dbtRiskYmlKeyMatches()` returns the specific keys that matched; new `dbtRiskYmlKeys: string[]` field on FileChangeClass. Reason string now names the exact triggering keys (e.g. "schema.yml diff touches tests under models/marts/") rather than the concatenated umbrella. - MINOR #5 — reworded the "upgrades the WEIGHT" comment. There is no weighting/ordering effect; `martLayerChange` only enriches the reason string with the mart-API-surface context. - MINOR #6 — block-scalar body FP: `stripBlockScalars()` walks the diff tracking block-scalar state so a description or long-form comment containing `data_tests:` (or similar) doesn't spuriously promote. Codex round-6 review HIGH fixed too — earlier version worked only on the +/- slice, missing the common case where `description: |` is in the context and a changed line is inside its body. Now walks the full diff (context + changed) for state and only masks output on changed lines. - MINOR #7 — FinOps boundary class extended with `\d` so digit-suffixed paths (`mrt_cost2024.sql`, `stg_billing2.sql`, `dbu1_usage.sql`) match. - NIT #8 — DBT_UNIQUE_COMBO_RE relaxed to make the list-item marker optional, so the bare-key indented form (`unique_combination_of_columns:` under a short-form `tests:` map) also matches. - NIT #9 — added regression test for removed-line risk-key promotion (removing a `data_tests:` block is at least as risk-worthy as adding one). - NIT #10 — comment reworded from `unique_combination_of_columns` is a "TEST NAME" → "dbt test macro / test parameter". Full altimate review suite: 3807 pass / 640 skip / 0 fail (was 3781 baseline — 26 new tests). Codex-round-6 minor addressed: `dbtRiskYmlKeyMatches` is now called once via a shared `scannedForRisk` local and its result reused for both `dbtRiskYmlChanges` and `dbtRiskYmlKeys`. Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of --- .../opencode/src/altimate/review/risk-tier.ts | 132 +++++++++++-- .../opencode/test/altimate/review.test.ts | 176 +++++++++++++++++- 2 files changed, 289 insertions(+), 19 deletions(-) diff --git a/packages/opencode/src/altimate/review/risk-tier.ts b/packages/opencode/src/altimate/review/risk-tier.ts index 79d55c6edb..0dffa2a299 100644 --- a/packages/opencode/src/altimate/review/risk-tier.ts +++ b/packages/opencode/src/altimate/review/risk-tier.ts @@ -41,9 +41,15 @@ export interface FileChangeClass { // PRs D (test-only YAML on contracted marts) and E (cost-anchor redesign) // both auto-approved despite each having 8+ substantive human findings. /** schema.yml diff introduces or edits `data_tests:`, `constraints:`, - * `unique_combination_of_columns`, or `contract:` — grain / constraint - * territory reviewers repeatedly flagged as substantive. */ + * `unique_combination_of_columns`, `contract:`, or the pre-1.8 `tests:` + * alias — grain / constraint territory reviewers repeatedly flagged as + * substantive. Convenience boolean; `dbtRiskYmlKeys` names the exact + * keys that matched. */ dbtRiskYmlChanges: boolean + /** The specific risk-YAML keys that matched in the changed lines. Empty + * when `dbtRiskYmlChanges` is false. Consumers should prefer this over + * the boolean when building human-readable reasons. */ + dbtRiskYmlKeys: string[] /** File lives under `models/marts/` or `models/mart/` — mart-layer changes * land in the API surface downstream consumers depend on. */ martLayerChange: boolean @@ -72,16 +78,39 @@ const INCREMENTAL_RE = /is_incremental\s*\(|unique_key|incremental_strategy|merg // optional indentation, and optional list-item marker (`- `). Excludes comment // lines (`#`) and description strings that happen to contain the keyword. Two // forms because `data_tests`/`constraints`/`contract` are ALWAYS keys, whereas -// `unique_combination_of_columns` is a TEST NAME that shows up as a list item -// (`- dbt_utils.unique_combination_of_columns:`). -const DBT_RISK_KEY_RE = /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?(?:data_tests|constraints|contract)[ \t]*:/im -const DBT_UNIQUE_COMBO_RE = /^[+-]?[ \t]*(?!#)-[ \t]+(?:[\w.]+\.)?unique_combination_of_columns[ \t]*:/im +// `unique_combination_of_columns` is a dbt test macro / test parameter that +// shows up as a list item under a `tests:` / `data_tests:` map +// (`- dbt_utils.unique_combination_of_columns:`). Comment wording fixed per +// consensus NIT #10. +// Consensus-review MAJOR #1 — `tests:` (the pre-dbt-1.8 alias for +// `data_tests:`) is also a risk-bearing YAML key. Without it, a schema.yml +// under `models/marts/` adding `tests: [- unique / - not_null]` would fall +// through to `trivial` and auto-approve on any project that hasn't migrated +// to `data_tests:`. Each signal is its own regex so we can name the exact +// triggering key in the reason string (consensus MINOR #4). +const DBT_RISK_KEY_PATTERNS: Array<{ key: string; re: RegExp }> = [ + { key: "data_tests", re: /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?data_tests[ \t]*:/im }, + { key: "tests", re: /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?tests[ \t]*:/im }, + { key: "constraints", re: /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?constraints[ \t]*:/im }, + { key: "contract", re: /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?contract[ \t]*:/im }, +] +// unique_combination_of_columns is a TEST NAME, not a YAML key — matches +// both list-item form (`- dbt_utils.unique_combination_of_columns:`) and +// the bare-key indented form (`unique_combination_of_columns:` under a +// short-form `tests:` map). Consensus NIT #8. +const DBT_UNIQUE_COMBO_RE = + /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?(?:[\w.]+\.)?unique_combination_of_columns[ \t]*:/im const MARTS_DIR_RE = /(?:^|\/)models\/marts?\//i // FinOps keyword must sit at a path or filename boundary so we don't fire on // arbitrary substrings (e.g. `broadcaster` matching `caste` never triggers -// `cost`, but `_backups_of_cost_config.sql` would still catch on the `cost` -// token via `_`). Cover common word / segment / extension boundaries. -const FINOPS_TOKEN_RE = /(?:^|[\/_.-])(?:cost|costs|saving|savings|billing|credit|credits|dbu|dbus|spend|revenue|price|prices|rate|rates|pricing|invoice|invoices)(?:$|[\/_.-])/i +// `cost`, but `_backups_of_cost_config.sql` still catches via the `_` boundary). +// The boundary class includes `\d` so digit-suffixed names (`mrt_cost2024`, +// `dbu1_usage`) still match — consensus MINOR #7. `dbus` was removed from +// the token list because it collides with D-Bus (IPC) file names on paths +// like `stg_dbus_connector.sql` — the singular `dbu` is sufficient +// (consensus MINOR #2). +const FINOPS_TOKEN_RE = + /(?:^|[\/_.\-\d])(?:cost|costs|saving|savings|billing|credit|credits|dbu|spend|revenue|price|prices|rate|rates|pricing|invoice|invoices)(?:$|[\/_.\-\d])/i /** The ADDED/REMOVED lines of a unified diff (excludes context + hunk headers), * so signal detection fires on what actually changed, not surrounding context. */ @@ -93,11 +122,80 @@ function changedLines(diff: string | undefined): string { .join("\n") } +/** Walk a unified diff, tracking block-scalar state from context lines AND + * changed lines, then return only the ADDED/REMOVED lines with any that + * live inside a block scalar blanked out. + * + * Codex R20 round-6 review HIGH — an earlier version stripped only the + * already-filtered +/- slice, which missed the common case where a + * pre-existing `description: |` is in the context (unchanged) but the + * added line inside its body contains `data_tests:` etc. Tracking scalar + * state over the whole diff — and only masking output on changed lines — + * closes that gap while preserving the +/- filter's role of ignoring + * surrounding-code noise. */ +function changedLinesForScan(diff: string | undefined): string { + if (!diff) return "" + const lines = diff.split("\n") + const out: string[] = [] + let scalarIndent = -1 + const blockScalarStart = /^[ \t]*[^\s#:][^:]*:[ \t]*[|>][+-]?[ \t]*$/ + for (const raw of lines) { + // Skip hunk headers entirely — they aren't code and would confuse the + // block-scalar tracker. + if (raw.startsWith("+++") || raw.startsWith("---") || raw.startsWith("@@")) continue + // Distinguish added / removed / context so we can update scalar state + // from context lines too but only include +/- in the output. + const marker = raw.startsWith("+") || raw.startsWith("-") ? raw[0] : " " + const stripped = marker === " " ? raw : raw.slice(1) + const indent = stripped.length - stripped.replace(/^[ \t]+/, "").length + const contentEmpty = stripped.trim() === "" + // Close the current scalar when we hit a non-empty line at ≤ scalarIndent. + if (scalarIndent >= 0 && !contentEmpty && indent <= scalarIndent) { + scalarIndent = -1 + } + const insideScalar = scalarIndent >= 0 && !contentEmpty && indent > scalarIndent + // Open a new scalar when the *stripped* content matches the start form. + // Do this AFTER the close-check so a line that both closes one scalar + // and opens a new one is handled correctly (rare). + if (blockScalarStart.test(stripped)) { + scalarIndent = indent + } + // Only added/removed lines are candidates for regex scanning; context + // lines only feed scalar-state tracking. When the changed line lives + // inside a scalar, emit a blank so the regex has nothing to match. + if (marker === " ") continue + out.push(insideScalar ? "" : raw) + } + return out.join("\n") +} + +/** Return the specific risk-YAML keys that matched in the changed lines, in + * a stable order matching DBT_RISK_KEY_PATTERNS. Used by classifyFile to + * populate `dbtRiskYmlKeys` so the promotion reason can name the exact + * triggering key rather than a concatenated "data_tests/constraints/contract" + * umbrella (consensus MINOR #4). Non-schema.yml files always return an + * empty list. */ +function dbtRiskYmlKeyMatches(kind: DbtFileKind, scanned: string): string[] { + if (kind !== "schema_yml" || !scanned) return [] + const matched: string[] = [] + for (const { key, re } of DBT_RISK_KEY_PATTERNS) { + if (re.test(scanned)) matched.push(key) + } + if (DBT_UNIQUE_COMBO_RE.test(scanned)) matched.push("unique_combination_of_columns") + return matched +} + /** Classify a single changed file from its diff + optional manifest signals. */ export function classifyFile(file: ChangedFile, opts: ClassifyOptions = {}): FileChangeClass { const kind = classifyDbtFile(file.path) const diff = file.diff const changed = changedLines(diff) + // Block-scalar-aware scan: walks the whole diff (including context lines) + // to track scalar state, then returns +/- lines with scalar bodies blanked + // (codex R20 round-6 HIGH). Used only for the risk-YAML regexes; other + // signals (materialization, incremental) run on the classic filter. + const scannedForRisk = kind === "schema_yml" ? changedLinesForScan(diff) : "" + const dbtRiskYmlKeys = dbtRiskYmlKeyMatches(kind, scannedForRisk) return { path: file.path, kind, @@ -109,8 +207,8 @@ export function classifyFile(file: ChangedFile, opts: ClassifyOptions = {}): Fil materializationChange: kind === "model_sql" && !!changed && MATERIALIZATION_RE.test(changed), incrementalLogicChange: kind === "model_sql" && !!changed && INCREMENTAL_RE.test(changed), complex: opts.isComplexOf?.(file) ?? false, - dbtRiskYmlChanges: - kind === "schema_yml" && !!changed && (DBT_RISK_KEY_RE.test(changed) || DBT_UNIQUE_COMBO_RE.test(changed)), + dbtRiskYmlChanges: dbtRiskYmlKeys.length > 0, + dbtRiskYmlKeys, martLayerChange: MARTS_DIR_RE.test(file.path), finopsPathToken: FINOPS_TOKEN_RE.test(file.path), } @@ -137,11 +235,17 @@ export function fullTierReasons(c: FileChangeClass): string[] { // reason — description-only edits under `models/marts/` are legitimately // trivial. Promotion here requires diff-level evidence (`dbtRiskYmlChanges`) // or a FinOps path token, either of which correlates with real reviewer - // blockers in the corpus. `martLayerChange` upgrades the WEIGHT of a - // dbtRiskYmlChanges hit but doesn't fire on its own. + // blockers in the corpus. `martLayerChange` enriches the reason string + // (adds the mart-API-surface context) but doesn't fire on its own — the + // tier is already `full` when `dbtRiskYmlChanges` is true; there's no + // additional weighting or ordering effect (consensus MINOR #5 clarified). if (c.dbtRiskYmlChanges) { + // Name the exact triggering YAML keys rather than a fixed umbrella string + // — consensus MINOR #4. Falls back to the umbrella when we can't + // enumerate (defensive, should not happen post-classifier). + const keys = c.dbtRiskYmlKeys.length ? c.dbtRiskYmlKeys.join(" / ") : "data_tests/constraints/contract" const loc = c.martLayerChange ? " under models/marts/ (mart-API surface)" : "" - reasons.push(`schema.yml diff touches data_tests/constraints/contract${loc}`) + reasons.push(`schema.yml diff touches ${keys}${loc}`) } if (c.finopsPathToken) reasons.push("path contains FinOps keyword (cost/saving/billing/dbu/etc.)") return reasons diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index 99f8394730..556503fa1d 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -532,19 +532,157 @@ describe("risk-tier", () => { "+ - type: not_null\n" const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) expect(r.tier).toBe("full") - expect(r.reasons.join(" ")).toContain("data_tests/constraints/contract") + // Reason names the specific key that fired (consensus MINOR #4). + expect(r.reasons.join(" ")).toContain("constraints") }) - test("R20 S4: full: schema.yml adds unique_combination_of_columns test", () => { + test("R20 S4: full: legacy `tests:` key (pre-dbt-1.8) also promotes (consensus MAJOR #1)", () => { + // Consensus MAJOR #1 — the original DBT_RISK_KEY_RE covered `data_tests` + // but not the pre-1.8 `tests:` alias. Projects on dbt <1.8 or mid- + // migration could add `tests: [- unique / - not_null]` under + // `models/marts/` and slip past the trivial gate. + const diff = + "@@ -1,3 +1,6 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + " columns:\n" + + " - name: id\n" + + "+ tests:\n" + + "+ - not_null\n" + const r = classifyPR([file("models/marts/mrt_x.yml", diff)]) + expect(r.tier).toBe("full") + // The specific key must be named — consensus MINOR #4. + expect(r.reasons.join(" ")).toContain("tests") + expect(r.reasons.join(" ")).toContain("mart-API surface") + }) + + test("R20 S4: legacy `tests:` outside marts still promotes (kind gate isn't marts-only)", () => { + // The rule fires on any schema_yml, not just marts — promoting an int_ + // yml adding `tests: [not_null]` on a grain key is still risk-worthy + // even without the marts-API-surface context. + const diff = + "@@ -1,3 +1,6 @@\n" + + " models:\n" + + " - name: int_x\n" + + " columns:\n" + + " - name: id\n" + + "+ tests:\n" + + "+ - unique\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("full") + }) + + test("R20 S4: block-scalar description containing `data_tests:` does NOT promote (consensus MINOR #6)", () => { + // Consensus MINOR #6 — the `(?!#)` guard only excluded `#` comments, + // not YAML block-scalar bodies (`description: |` or `description: >`). + // A long-form description that happens to contain the substring + // `data_tests:` on its own line would trigger a promotion that had + // nothing to do with actual risk changes. + const diff = + "@@ -1,3 +1,8 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + "+ description: |\n" + + "+ This mart is the source of truth for X.\n" + + "+ data_tests: are declared in the sibling schema.yml.\n" + + "+ constraints: are managed via the contract there.\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("trivial") + }) + + test("R20 S4: block-scalar OPENED IN CONTEXT (not changed) still suppresses body (codex round-6 HIGH)", () => { + // Codex R20 round-6 HIGH — an earlier version stripped only the + // filtered +/- slice, so a pre-existing `description: |` in the + // context would be invisible and a `+ data_tests: ...` line + // inside its body would wrongly promote. The scan now walks the + // whole diff for scalar state and only masks output on changed lines. + const diff = + "@@ -1,7 +1,8 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + " description: |\n" + + " This mart is the source of truth for X.\n" + + "- Some prior description body.\n" + + "+ data_tests: are declared in the sibling schema.yml.\n" + + "+ constraints: are managed via the contract there.\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("trivial") + }) + + test("R20 S4: block-scalar closes at ≤ start-indent (subsequent risk keys still fire)", () => { + // Precision guard for the block-scalar strip — when the block scalar + // ENDS and a subsequent line at ≤ its start indent adds a real risk + // key, the promotion must still fire. Otherwise the strip could + // silently suppress genuine risk changes. + const diff = + "@@ -1,3 +1,9 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + "+ description: |\n" + + "+ Body: mentions data_tests: only inside prose.\n" + + "+ columns:\n" + + "+ - name: id\n" + + "+ data_tests:\n" + + "+ - not_null\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("full") + expect(r.reasons.join(" ")).toContain("data_tests") + }) + + test("R20 S4: removing a `data_tests:` block also promotes (consensus NIT #9)", () => { + // Consensus NIT #9 — the risk-key regex matches BOTH `+` and `-` + // prefixes because `changedLines()` includes both. Removing a + // guardrail is at least as risk-worthy as adding one; this locks in + // that behavior with an explicit test. + const diff = + "@@ -1,7 +1,3 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + " columns:\n" + + " - name: id\n" + + "- data_tests:\n" + + "- - not_null\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("full") + }) + + test("R20 S4: bare `tests` (no colon) does NOT match the risk-key regex (word-boundary guard)", () => { + // FP guard — a description line like `# tests explanation`, or a + // yml value containing the string `tests` without a following colon, + // must not promote. The regex requires `[ \t]*:`. + const diff = + "@@ -1,3 +1,4 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + '+ description: "See tests documentation for the grain policy"\n' + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("trivial") + }) + + test("R20 S4: full: schema.yml adds unique_combination_of_columns test (consensus MAJOR #3 tightened)", () => { + // Locked-down test for the DBT_UNIQUE_COMBO_RE regex specifically. + // Original path (`models/marts/mrt_billing_account_prices.yml`) also + // matched `MARTS_DIR_RE` AND `FINOPS_TOKEN_RE` (via `billing` / `prices`) + // → the assertion passed on `finopsPathToken` even if the combo regex + // were deleted (consensus MAJOR #3). Path moved to non-mart, non-FinOps + // territory so `DBT_UNIQUE_COMBO_RE` is the sole possible promoter, and + // the reason string is asserted to confirm which signal fired. const diff = "@@ -1,3 +1,7 @@\n" + " data_tests:\n" + "+ - dbt_utils.unique_combination_of_columns:\n" + "+ combination_of_columns:\n" + - "+ - metastore_id\n" + - "+ - sku_name\n" - const r = classifyPR([file("models/marts/mrt_billing_account_prices.yml", diff)]) + "+ - a\n" + + "+ - b\n" + const r = classifyPR([file("models/intermediate/int_grain.yml", diff)]) expect(r.tier).toBe("full") + // The reason must name the exact triggering signal (consensus MINOR #4). + // Both `data_tests:` (the key) and `unique_combination_of_columns` (the + // test name inside the list) fire on this diff. + expect(r.reasons.join(" ")).toContain("unique_combination_of_columns") + // Explicit FP guard — no other promotion signal should fire on this path. + expect(r.reasons.join(" ")).not.toContain("FinOps") + expect(r.reasons.join(" ")).not.toContain("mart-API surface") }) test("R20 S4: full: FinOps keyword in path (PR E shape — anchor cost redesign)", () => { @@ -589,6 +727,34 @@ describe("risk-tier", () => { expect(r2.tier).toBe("lite") }) + test("R20 S4: `dbus` (D-Bus IPC) does NOT fire the FinOps rule (consensus MINOR #2)", () => { + // Consensus MINOR #2 — `dbus` in the token list matched D-Bus (IPC) + // paths (`stg_dbus_connector.sql`) that have nothing to do with + // Databricks Units. `dbu` singular is enough for the FinOps signal; + // `dbus` was removed from the token list. + const r = classifyPR([file("models/staging/stg_dbus_connector.sql", "+select 1\n")], { + blastRadiusOf: () => 0, + }) + expect(r.tier).toBe("lite") + }) + + test("R20 S4: FinOps boundary matches digit suffixes (consensus MINOR #7)", () => { + // Consensus MINOR #7 — the boundary class previously included only + // `[/_.-]`, so `mrt_cost2024.sql`, `stg_billing2.sql`, `dbu1_usage.sql` + // slipped through (the char after the keyword is a digit, not in the + // boundary class). Adding `\d` catches these versioned / iteration- + // suffixed paths. + for (const p of [ + "models/marts/mrt_cost2024.sql", + "models/staging/stg_billing2.sql", + "models/intermediate/dbu1_usage.sql", + ]) { + const r = classifyPR([file(p, "+select 1\n")], { blastRadiusOf: () => 0 }) + expect(r.tier).toBe("full") + expect(r.reasons.join(" ")).toContain("FinOps keyword") + } + }) + test("R20 S4: trivial: description-only edits under models/marts/ still trivial (no risk keys)", () => { // A schema.yml under marts that only changes descriptions/docs should // stay trivial — the promotion requires diff-level risk signals, not From 6dfbbcb34b426e9381a402ce69f11b405c5b5db7 Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 22 Jul 2026 19:26:38 +0530 Subject: [PATCH 03/10] =?UTF-8?q?fix(review):=20[R20=20S4]=20address=20kil?= =?UTF-8?q?o-code-bot=20review=20=E2=80=94=20blockScalarStart=20regex=20ti?= =?UTF-8?q?ghtened?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kilo-code-bot suggestion on PR #1028 — the earlier `blockScalarStart` regex accepted only `|`/`>` with an optional `+`/`-` chomping indicator, so YAML block-scalar headers carrying an explicit indentation indicator (`description: |2`, `|+2`, `|2-`) or a trailing comment (`description: | # legacy`) failed to open a scalar. Body lines beginning with a risk keyword (`data_tests:`, `constraints:`, …) then false-positive promoted the file to `full`. Safe-direction over-tiering (never a wrong verdict), but this regex is the sole scalar gate — widen it to close the gap. Regex now: `[|>](?:[+-]?[1-9]?|[1-9]?[+-]?)[ \\t]*(?:#.*)?$`. - `[|>]` opener - `(?:[+-]?[1-9]?|[1-9]?[+-]?)` chomp+indent in either order - optional trailing `# comment` New regression test loops over `|2`, `|+2`, and `| # legacy comment` opener shapes and asserts each produces `trivial` on a diff whose body lines contain risk keywords. Full altimate review suite: 3808 pass / 640 skip / 0 fail (was 3807 baseline — 1 new test). Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of --- .../opencode/src/altimate/review/risk-tier.ts | 10 +++++++++- .../opencode/test/altimate/review.test.ts | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/review/risk-tier.ts b/packages/opencode/src/altimate/review/risk-tier.ts index 0dffa2a299..a6206c30ea 100644 --- a/packages/opencode/src/altimate/review/risk-tier.ts +++ b/packages/opencode/src/altimate/review/risk-tier.ts @@ -138,7 +138,15 @@ function changedLinesForScan(diff: string | undefined): string { const lines = diff.split("\n") const out: string[] = [] let scalarIndent = -1 - const blockScalarStart = /^[ \t]*[^\s#:][^:]*:[ \t]*[|>][+-]?[ \t]*$/ + // YAML block-scalar header. The optional trailing tail covers: + // - explicit indentation indicator (1-9) either before or after the + // chomping indicator: `|2`, `|+2`, `|2+`, `>2-` + // - trailing comment: `description: | # legacy` + // kilo-code-bot suggestion — the earlier form matched only `|`/`>` + // with an optional `+`/`-` chomp, so `|2` and `| # comment` skipped the + // opener → body lines beginning with a risk keyword false-positive + // promoted (safe-direction over-tiering, but still wrong). + const blockScalarStart = /^[ \t]*[^\s#:][^:]*:[ \t]*[|>](?:[+-]?[1-9]?|[1-9]?[+-]?)[ \t]*(?:#.*)?$/ for (const raw of lines) { // Skip hunk headers entirely — they aren't code and would confuse the // block-scalar tracker. diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index 556503fa1d..06691c2a87 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -590,6 +590,25 @@ describe("risk-tier", () => { expect(r.tier).toBe("trivial") }) + test("R20 S4: block-scalar header with explicit indentation indicator or trailing comment (kilo-bot review)", () => { + // kilo-code-bot suggestion — `blockScalarStart` earlier accepted only + // `|`/`>` with an optional `+`/`-` chomp, so `description: |2`, + // `description: |+2`, and `description: | # legacy` were skipped and + // their body lines could false-positive promote. All three shapes + // must now open a scalar so the body is masked. + for (const opener of ["description: |2", "description: |+2", "description: | # legacy comment"]) { + const diff = + "@@ -1,3 +1,7 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + `+ ${opener}\n` + + "+ data_tests: are declared in the sibling schema.yml\n" + + "+ constraints: are managed via the contract there.\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("trivial") + } + }) + test("R20 S4: block-scalar OPENED IN CONTEXT (not changed) still suppresses body (codex round-6 HIGH)", () => { // Codex R20 round-6 HIGH — an earlier version stripped only the // filtered +/- slice, so a pre-existing `description: |` in the From 309a3f53a084796c493eab4958c9ce9ef24a6175 Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 22 Jul 2026 20:45:53 +0530 Subject: [PATCH 04/10] fix(review): [R20 S4] context-line indent measurement in changedLinesForScan (cubic P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubic P2 bot review — the scalar-state tracker measured indent inconsistently between context and changed diff lines. Context lines start with a leading SPACE (the diff marker) and my earlier code left it in place; changed lines had their `+`/`-` marker stripped before indent measurement. Result: a valid `|1` block scalar opened in the context with a body indented exactly one space beyond the header wasn't masked, because the context-side `scalarIndent` counted one extra space and the changed-side body's indent then failed the `indent > scalarIndent` check. Fix strips the leading diff marker (`+`/`-`/space) on ALL lines before indent measurement, so context and changed lines live in the same coordinate system. New regression test: `|1` block scalar opened in context with body containing `data_tests:` / `constraints:` prose stays trivial. Full altimate review suite: 3808 pass / 640 skip / 0 fail (was 3807 baseline — 1 new test). Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of --- .../opencode/src/altimate/review/risk-tier.ts | 9 ++++++-- .../opencode/test/altimate/review.test.ts | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/altimate/review/risk-tier.ts b/packages/opencode/src/altimate/review/risk-tier.ts index a6206c30ea..88eb5e5af2 100644 --- a/packages/opencode/src/altimate/review/risk-tier.ts +++ b/packages/opencode/src/altimate/review/risk-tier.ts @@ -153,8 +153,13 @@ function changedLinesForScan(diff: string | undefined): string { if (raw.startsWith("+++") || raw.startsWith("---") || raw.startsWith("@@")) continue // Distinguish added / removed / context so we can update scalar state // from context lines too but only include +/- in the output. - const marker = raw.startsWith("+") || raw.startsWith("-") ? raw[0] : " " - const stripped = marker === " " ? raw : raw.slice(1) + // Unified-diff context lines start with a leading SPACE (the diff + // marker), so we must strip that space too — otherwise a context line + // like " description: |1" is measured as 1-deeper indent than an + // otherwise-equivalent "+ data_tests: ..." changed line, and the + // scalar body fails the `indent > scalarIndent` check (cubic P2). + const marker = raw.startsWith("+") || raw.startsWith("-") ? raw[0] : raw.startsWith(" ") ? " " : "" + const stripped = marker === "" ? raw : raw.slice(1) const indent = stripped.length - stripped.replace(/^[ \t]+/, "").length const contentEmpty = stripped.trim() === "" // Close the current scalar when we hit a non-empty line at ≤ scalarIndent. diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index 06691c2a87..5ca6c4f34a 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -609,6 +609,27 @@ describe("risk-tier", () => { } }) + test("R20 S4: `|1` block scalar in context — body indent measured consistently (cubic P2 bot review)", () => { + // Cubic P2 — the diff-marker vs indentation calculation was + // inconsistent between context lines (retained their leading space) + // and changed lines (whose `+`/`-` marker was stripped). A valid `|1` + // block scalar opened in context with a body indented exactly one + // space beyond the header wasn't masked because the context + // scalarIndent counted one extra space. Now BOTH context and changed + // lines have their leading diff marker stripped before indent + // measurement. + const diff = + "@@ -1,8 +1,9 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + " description: |1\n" + + " Existing body line stays put.\n" + + "+ data_tests: are still in the sibling schema.yml, not here.\n" + + "+ constraints: too.\n" + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + expect(r.tier).toBe("trivial") + }) + test("R20 S4: block-scalar OPENED IN CONTEXT (not changed) still suppresses body (codex round-6 HIGH)", () => { // Codex R20 round-6 HIGH — an earlier version stripped only the // filtered +/- slice, so a pre-existing `description: |` in the From 136f95df348767bef00d04f7c838aa9e08c04330 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 23 Jul 2026 12:23:05 +0530 Subject: [PATCH 05/10] refactor(review): [R20 S4] move FinOps token list from reviewer core to `riskTierPathTokens` config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hardcoded `FINOPS_TOKEN_RE` in `risk-tier.ts` baked one team's billing/cost vocabulary into a reviewer everyone consumes. This moves the list out of core to user-configurable `.altimate/review.yml`, keeps the shipped list as an opt-in `preset:finops`, and generalizes the field so custom categories (pci, patient, etc.) work the same way. Also refactors the vacuous FP-guard test flagged by `altimate-harness-bot` (review.test.ts:755-768): the earlier `broadcaster` / `precast_table` paths did not contain any FinOps token substring, so the boundary anchors in the regex were never exercised — the test would pass even if the boundary class were deleted. Replaced with property-based loops over `RISK_TOKEN_PRESETS.finops` that assert boundary behavior for every preset token: `stg_{tok}_daily.sql` fires, `stg_x{tok}y.sql` does not. Adding a preset token extends coverage automatically. Core changes: - `config.ts` — new `riskTierPathTokens: Record`, default `{}`. Users opt in by naming a category and listing tokens or `preset:` markers. - `risk-tier.ts` — dropped `FINOPS_TOKEN_RE`. `FileChangeClass.finopsPathToken: boolean` → `.highRiskPathTokenCategory: string | undefined`. Reason string reads `path matches high-risk token category ''`. Added `RISK_TOKEN_PRESETS` (shipped presets) and `compilePathTokenResolver` (compiles config into a resolver, boundary-anchored, handles preset expansion). - `orchestrate.ts` — threads `config.riskTierPathTokens` through `compilePathTokenResolver` into `classifyPR` as the `pathTokenCategoryOf` callback. - Comments genericized: dropped internal round / corpus / vertical- specific vocabulary ("PR D / PR E", "DBU savings > DBU cost", "misanchored billing units") in favor of neutral wording. Backwards compatibility: projects that were relying on hardcoded FinOps promotion should add `riskTierPathTokens: {finops: [preset:finops]}` to their `.altimate/review.yml`. A project that never wanted FinOps promotion (majority case) now gets no path-token promotion at all — the reviewer core is neutral. Tests: 229/229 green (review*.test.ts). Includes property-based loops over the preset for both boundary-firing and interior-substring suppression, plus a "no resolver configured → no promotion" test that locks in the neutral-core guarantee. --- .../opencode/src/altimate/review/config.ts | 23 ++++ .../src/altimate/review/orchestrate.ts | 8 +- .../opencode/src/altimate/review/risk-tier.ts | 123 ++++++++++++++---- .../opencode/test/altimate/review.test.ts | 118 +++++++++-------- 4 files changed, 188 insertions(+), 84 deletions(-) diff --git a/packages/opencode/src/altimate/review/config.ts b/packages/opencode/src/altimate/review/config.ts index b4f775c2a5..2154a5dc00 100644 --- a/packages/opencode/src/altimate/review/config.ts +++ b/packages/opencode/src/altimate/review/config.ts @@ -46,6 +46,29 @@ export const ReviewConfig = z.object({ rubric: Rubric.partial().default({}), /** Extra path suffixes to exclude from review. */ exclude: z.array(z.string()).default([]), + /** + * Path-token categories that promote a change to `full` when a token + * appears at a path/word boundary. This is business-vertical opinion + * (billing, PII-adjacent, safety-critical) — hardcoding it into the + * reviewer core would leak one team's naming convention onto every + * consumer. Empty by default; teams opt in by naming categories and + * listing tokens. + * + * Example — enable the shipped `finops` preset (billing/cost/etc.): + * riskTierPathTokens: + * finops: [preset:finops] + * + * Or a custom category: + * riskTierPathTokens: + * pci: [card, pan, cvv] + * patient: [phi, mrn, diagnosis] + * + * Matching is case-insensitive at path/word/digit boundaries — a token + * `cost` fires on `mrt_cost_daily.sql` and `mrt_cost2024.sql` but not + * inside `broadcaster.sql` or `precast_table.sql`. The `preset:` + * marker expands to the current shipped list at reviewer startup. + */ + riskTierPathTokens: z.record(z.string(), z.array(z.string())).default({}), }) export type ReviewConfig = z.infer diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 07c60565b5..b7fec2a1ee 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -9,7 +9,7 @@ import { SEVERITY_ORDER, } from "./finding" import { type ChangedFile, filterChangedFiles } from "./diff-filter" -import { classifyPR, TIER_LANES } from "./risk-tier" +import { classifyPR, compilePathTokenResolver, TIER_LANES } from "./risk-tier" import { type Rubric, exclusionReason, clampSeverity } from "./rubric" import { type ReviewConfig } from "./config" import { type ReviewMode, type VerdictEnvelope, buildEnvelope, signEnvelope } from "./verdict" @@ -1092,6 +1092,11 @@ export async function runReview(input: OrchestrateInput): Promise 0 ? !anyManifest : reviewable.length === 0 + // High-risk path tokens are user-configured (billing/pci/patient/etc.) — + // the reviewer core carries no default list. `undefined` when no + // categories are configured, which keeps `highRiskPathTokenCategory` + // undefined per file and no promotion fires. + const pathTokenCategoryOf = compilePathTokenResolver(input.config.riskTierPathTokens) const tierResult = classifyPR(reviewable, { blastRadiusOf: (p) => { const c = ctxByPath.get(p) @@ -1099,6 +1104,7 @@ export async function runReview(input: OrchestrateInput): Promise (ctxByPath.get(f.path)?.pii.length ?? 0) > 0, isComplexOf: (f) => ctxByPath.get(f.path)?.complex ?? false, + pathTokenCategoryOf, }) const classifiedTier = tierResult.tier // G2 — --force-tier overrides the classifier. Envelope records both the diff --git a/packages/opencode/src/altimate/review/risk-tier.ts b/packages/opencode/src/altimate/review/risk-tier.ts index 88eb5e5af2..eda07f6cb8 100644 --- a/packages/opencode/src/altimate/review/risk-tier.ts +++ b/packages/opencode/src/altimate/review/risk-tier.ts @@ -36,10 +36,11 @@ export interface FileChangeClass { incrementalLogicChange: boolean /** Structurally complex SQL (window/subquery/large plan) — never `trivial`. */ complex: boolean - // R20 S4 — risk-signal promotion. Grounded in the 5-PR internal corpus study - // (see data-engineering-skills/docs/pr-review-corpus-findings-r20.md) where - // PRs D (test-only YAML on contracted marts) and E (cost-anchor redesign) - // both auto-approved despite each having 8+ substantive human findings. + // R20 S4 — risk-signal promotion. Grounded in an internal corpus study of + // real reviewer comments on dbt PRs; two failure classes drove the promotion + // gate: (a) test-only YAML edits on contracted marts, (b) cost-anchor logic + // redesigns landing in a business-critical vertical. Both had auto-approved + // despite each having 8+ substantive human findings. /** schema.yml diff introduces or edits `data_tests:`, `constraints:`, * `unique_combination_of_columns`, `contract:`, or the pre-1.8 `tests:` * alias — grain / constraint territory reviewers repeatedly flagged as @@ -53,11 +54,11 @@ export interface FileChangeClass { /** File lives under `models/marts/` or `models/mart/` — mart-layer changes * land in the API surface downstream consumers depend on. */ martLayerChange: boolean - /** Path or filename contains a FinOps keyword - * (`cost|saving|billing|credit|dbu|spend|revenue|price|rate`). The - * highest-severity blockers in the corpus (cross-model rate asymmetry, - * DBU savings > DBU cost, misanchored billing units) landed here. */ - finopsPathToken: boolean + /** The user-configured risk-token category that matched this path (e.g. + * `finops`, `pci`, `patient`), or undefined when no configured token + * matched. Categories are supplied via `riskTierPathTokens` in + * `.altimate/review.yml`; the reviewer core is neutral. */ + highRiskPathTokenCategory: string | undefined } export interface ClassifyOptions { @@ -67,6 +68,82 @@ export interface ClassifyOptions { touchesPiiOf?: (file: ChangedFile) => boolean /** Mark a path as a structurally complex change (window/subquery/large plan). */ isComplexOf?: (file: ChangedFile) => boolean + /** Resolve a path to a user-configured risk-token category (e.g. "finops", + * "pci"), or undefined when no category matches. Injected from + * `.altimate/review.yml`'s `riskTierPathTokens`; the reviewer core carries + * no default token list. */ + pathTokenCategoryOf?: (path: string) => string | undefined +} + +/** + * Shipped path-token presets that a user can opt into by putting + * `[preset:finops]` in their `riskTierPathTokens.` array. The + * lists are mined from real reviewer comments across dbt PRs in the + * billing / cost-attribution vertical; a project that doesn't care about + * that vertical simply doesn't enable the preset. Adding a new preset is + * a matter of shipping a new entry here. + */ +export const RISK_TOKEN_PRESETS: Record = { + finops: [ + "cost", + "costs", + "saving", + "savings", + "billing", + "credit", + "credits", + "dbu", + "spend", + "revenue", + "price", + "prices", + "rate", + "rates", + "pricing", + "invoice", + "invoices", + ], +} + +/** + * Compile a `riskTierPathTokens` config record into a `pathTokenCategoryOf` + * resolver — used by orchestration to inject the callback into `classifyPR`. + * Handles `preset:` expansion. Tokens match at path/word/digit + * boundaries: `mrt_cost.sql` fires, `broadcaster.sql` doesn't. Returns + * undefined (no resolver) when no categories are configured, so the + * `highRiskPathTokenCategory` field stays undefined and no promotion fires. + */ +export function compilePathTokenResolver( + cfg: Record, +): ((path: string) => string | undefined) | undefined { + const compiled: Array<{ category: string; re: RegExp }> = [] + for (const [category, entries] of Object.entries(cfg)) { + const tokens: string[] = [] + for (const entry of entries) { + if (entry.startsWith("preset:")) { + const name = entry.slice("preset:".length) + const preset = RISK_TOKEN_PRESETS[name] + if (preset) tokens.push(...preset) + } else { + tokens.push(entry) + } + } + if (tokens.length === 0) continue + // Anchor tokens at path/word/digit boundaries so incidental substrings + // don't fire (e.g. `broadcaster` should not match token `cast`). + const escaped = tokens.map((t) => t.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) + compiled.push({ + category, + re: new RegExp(`(?:^|[\\/_.\\-\\d])(?:${escaped.join("|")})(?:$|[\\/_.\\-\\d])`, "i"), + }) + } + if (compiled.length === 0) return undefined + return (path: string) => { + for (const { category, re } of compiled) { + if (re.test(path)) return category + } + return undefined + } } const MATERIALIZATION_RE = /[+]?materialized\s*[:=]|config\s*\(\s*[^)]*materialized/i @@ -101,16 +178,6 @@ const DBT_RISK_KEY_PATTERNS: Array<{ key: string; re: RegExp }> = [ const DBT_UNIQUE_COMBO_RE = /^[+-]?[ \t]*(?!#)(?:-[ \t]+)?(?:[\w.]+\.)?unique_combination_of_columns[ \t]*:/im const MARTS_DIR_RE = /(?:^|\/)models\/marts?\//i -// FinOps keyword must sit at a path or filename boundary so we don't fire on -// arbitrary substrings (e.g. `broadcaster` matching `caste` never triggers -// `cost`, but `_backups_of_cost_config.sql` still catches via the `_` boundary). -// The boundary class includes `\d` so digit-suffixed names (`mrt_cost2024`, -// `dbu1_usage`) still match — consensus MINOR #7. `dbus` was removed from -// the token list because it collides with D-Bus (IPC) file names on paths -// like `stg_dbus_connector.sql` — the singular `dbu` is sufficient -// (consensus MINOR #2). -const FINOPS_TOKEN_RE = - /(?:^|[\/_.\-\d])(?:cost|costs|saving|savings|billing|credit|credits|dbu|spend|revenue|price|prices|rate|rates|pricing|invoice|invoices)(?:$|[\/_.\-\d])/i /** The ADDED/REMOVED lines of a unified diff (excludes context + hunk headers), * so signal detection fires on what actually changed, not surrounding context. */ @@ -223,7 +290,7 @@ export function classifyFile(file: ChangedFile, opts: ClassifyOptions = {}): Fil dbtRiskYmlChanges: dbtRiskYmlKeys.length > 0, dbtRiskYmlKeys, martLayerChange: MARTS_DIR_RE.test(file.path), - finopsPathToken: FINOPS_TOKEN_RE.test(file.path), + highRiskPathTokenCategory: opts.pathTokenCategoryOf?.(file.path), } } @@ -239,7 +306,7 @@ export function fullTierReasons(c: FileChangeClass): string[] { if (c.materializationChange) reasons.push("materialization changed") if (c.incrementalLogicChange) reasons.push("incremental logic changed") if (c.blastRadius > 5) reasons.push(`${c.blastRadius} downstream models`) - // R20 S4 — trivial/lite promotion for signals the 5-PR corpus proved are + // R20 S4 — trivial/lite promotion for signals the corpus study proved are // reviewer-critical. Each reason surfaces in the signed envelope's // `tierReasons`, so a customer can see WHY a nominally-tiny YAML-only diff // ran at full tier. @@ -247,11 +314,11 @@ export function fullTierReasons(c: FileChangeClass): string[] { // Path-based `martLayerChange` on its own is intentionally NOT a promotion // reason — description-only edits under `models/marts/` are legitimately // trivial. Promotion here requires diff-level evidence (`dbtRiskYmlChanges`) - // or a FinOps path token, either of which correlates with real reviewer - // blockers in the corpus. `martLayerChange` enriches the reason string - // (adds the mart-API-surface context) but doesn't fire on its own — the - // tier is already `full` when `dbtRiskYmlChanges` is true; there's no - // additional weighting or ordering effect (consensus MINOR #5 clarified). + // or a user-configured high-risk path-token match, either of which + // correlates with real reviewer blockers. `martLayerChange` enriches the + // reason string (adds the mart-API-surface context) but doesn't fire on its + // own — the tier is already `full` when `dbtRiskYmlChanges` is true; there's + // no additional weighting or ordering effect (consensus MINOR #5 clarified). if (c.dbtRiskYmlChanges) { // Name the exact triggering YAML keys rather than a fixed umbrella string // — consensus MINOR #4. Falls back to the umbrella when we can't @@ -260,7 +327,9 @@ export function fullTierReasons(c: FileChangeClass): string[] { const loc = c.martLayerChange ? " under models/marts/ (mart-API surface)" : "" reasons.push(`schema.yml diff touches ${keys}${loc}`) } - if (c.finopsPathToken) reasons.push("path contains FinOps keyword (cost/saving/billing/dbu/etc.)") + if (c.highRiskPathTokenCategory) { + reasons.push(`path matches high-risk token category '${c.highRiskPathTokenCategory}'`) + } return reasons } diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index 5ca6c4f34a..47dfa40557 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -21,6 +21,8 @@ import { shouldReview, classifyPR, classifyFile, + compilePathTokenResolver, + RISK_TOKEN_PRESETS, type ChangedFile, runReview, modelNameFromPath, @@ -701,12 +703,9 @@ describe("risk-tier", () => { test("R20 S4: full: schema.yml adds unique_combination_of_columns test (consensus MAJOR #3 tightened)", () => { // Locked-down test for the DBT_UNIQUE_COMBO_RE regex specifically. - // Original path (`models/marts/mrt_billing_account_prices.yml`) also - // matched `MARTS_DIR_RE` AND `FINOPS_TOKEN_RE` (via `billing` / `prices`) - // → the assertion passed on `finopsPathToken` even if the combo regex - // were deleted (consensus MAJOR #3). Path moved to non-mart, non-FinOps - // territory so `DBT_UNIQUE_COMBO_RE` is the sole possible promoter, and - // the reason string is asserted to confirm which signal fired. + // Path chosen so `DBT_UNIQUE_COMBO_RE` is the sole possible promoter + // (non-mart, no configured high-risk tokens by default), and the reason + // string is asserted to confirm which signal fired. const diff = "@@ -1,3 +1,7 @@\n" + " data_tests:\n" + @@ -721,77 +720,84 @@ describe("risk-tier", () => { // test name inside the list) fire on this diff. expect(r.reasons.join(" ")).toContain("unique_combination_of_columns") // Explicit FP guard — no other promotion signal should fire on this path. - expect(r.reasons.join(" ")).not.toContain("FinOps") + expect(r.reasons.join(" ")).not.toContain("high-risk token") expect(r.reasons.join(" ")).not.toContain("mart-API surface") }) - test("R20 S4: full: FinOps keyword in path (PR E shape — anchor cost redesign)", () => { - // Small SQL change on a cost-savings mart should NOT auto-approve at - // lite tier just because it's within the line limit — the corpus study - // showed these are the highest-severity blockers. - const r = classifyPR([file("models/marts/mrt_jobs_cost_savings.sql", "+select 1 as a\n")], { + // The path-token promotion is USER-CONFIGURED, not baked into the core. + // These tests exercise the resolver + tier plumbing with an explicit + // finops preset opt-in. A project that doesn't set `riskTierPathTokens` + // gets no path-token promotion at all — verified by the default-classifier + // paths tested above. + const finopsResolver = compilePathTokenResolver({ finops: ["preset:finops"] }) + + test("R20 S4: full: high-risk path-token category matches when the finops preset is enabled", () => { + // Small SQL change on a mart whose path contains a preset token should + // NOT auto-approve at lite tier just because it's within the line limit. + const r = classifyPR([file("models/marts/mrt_cost_daily.sql", "+select 1 as a\n")], { blastRadiusOf: () => 1, + pathTokenCategoryOf: (p) => finopsResolver!(p), }) expect(r.tier).toBe("full") - expect(r.reasons.join(" ")).toContain("FinOps keyword") - }) - - test("R20 S4: full: FinOps keyword variants (billing/dbu/savings/spend) all promote", () => { - for (const path of [ - "models/marts/mrt_billing_daily.sql", - "models/marts/mrt_dbu_by_workspace.sql", - "models/marts/mrt_credit_savings.sql", - "models/intermediate/int_query_cost.sql", - "models/staging/stg_warehouse_spend.sql", - "models/marts/mrt_list_price.sql", - "models/marts/mrt_daily_rate.sql", - ]) { - const r = classifyPR([file(path, "+select 1\n")], { blastRadiusOf: () => 0 }) + expect(r.reasons.join(" ")).toContain("high-risk token category 'finops'") + }) + + test("R20 S4: high-risk token category — every preset token fires at a boundary", () => { + // Property-based: iterate the shipped preset and assert each token + // promotes when placed at a path boundary. Adding a preset token + // extends coverage automatically; a regression that broke the + // boundary character class shows up as multiple simultaneous test + // failures. + for (const tok of RISK_TOKEN_PRESETS.finops) { + const p = `models/marts/mrt_${tok}_summary.sql` + const r = classifyPR([file(p, "+select 1\n")], { + blastRadiusOf: () => 0, + pathTokenCategoryOf: (x) => finopsResolver!(x), + }) expect(r.tier).toBe("full") - expect(r.reasons.join(" ")).toContain("FinOps keyword") + expect(r.reasons.join(" ")).toContain("high-risk token category 'finops'") } }) - test("R20 S4: FinOps token does NOT over-fire on incidental substrings", () => { - // False-positive guard: the token regex requires path/word boundaries so - // words like `broadcaster.py` (has `caste`) or `precast_table.sql` (has - // `cast`) don't fire the cost/dbu/etc. rules. - const r1 = classifyPR([file("models/staging/stg_broadcaster.sql", "+select 1\n")], { - blastRadiusOf: () => 0, - }) - expect(r1.tier).toBe("lite") - - const r2 = classifyPR([file("models/marts/mrt_precast_table.sql", "+select 1\n")], { - blastRadiusOf: () => 0, - }) - expect(r2.tier).toBe("lite") + test("R20 S4: high-risk token — interior substring does NOT fire (boundary anchors exercised)", () => { + // Property-based FP guard. For every preset token, wrap it in + // ASCII-letter padding on both sides so the token IS present in the + // path as an interior substring. The alternation matches; only the + // boundary anchors (path/word/digit) block promotion. If someone + // weakens the boundary class, every one of these fails. + for (const tok of RISK_TOKEN_PRESETS.finops) { + const p = `models/staging/stg_x${tok}y.sql` + const r = classifyPR([file(p, "+select 1\n")], { + blastRadiusOf: () => 0, + pathTokenCategoryOf: (x) => finopsResolver!(x), + }) + expect(r.tier).toBe("lite") + } }) - test("R20 S4: `dbus` (D-Bus IPC) does NOT fire the FinOps rule (consensus MINOR #2)", () => { - // Consensus MINOR #2 — `dbus` in the token list matched D-Bus (IPC) - // paths (`stg_dbus_connector.sql`) that have nothing to do with - // Databricks Units. `dbu` singular is enough for the FinOps signal; - // `dbus` was removed from the token list. - const r = classifyPR([file("models/staging/stg_dbus_connector.sql", "+select 1\n")], { + test("R20 S4: high-risk token — boundary matches digit suffixes (consensus MINOR #7)", () => { + // The boundary class includes `\d` so digit-suffixed names still fire. + // A single named case is enough — the property tests above cover the + // full alternation. + const r = classifyPR([file("models/marts/mrt_cost2024.sql", "+select 1\n")], { blastRadiusOf: () => 0, + pathTokenCategoryOf: (p) => finopsResolver!(p), }) - expect(r.tier).toBe("lite") + expect(r.tier).toBe("full") + expect(r.reasons.join(" ")).toContain("high-risk token category 'finops'") }) - test("R20 S4: FinOps boundary matches digit suffixes (consensus MINOR #7)", () => { - // Consensus MINOR #7 — the boundary class previously included only - // `[/_.-]`, so `mrt_cost2024.sql`, `stg_billing2.sql`, `dbu1_usage.sql` - // slipped through (the char after the keyword is a digit, not in the - // boundary class). Adding `\d` catches these versioned / iteration- - // suffixed paths. + test("R20 S4: high-risk token — no promotion when no resolver is configured (core is neutral)", () => { + // Same paths that fire above must stay lite when the caller doesn't + // supply a resolver — the reviewer core has zero opinion about which + // paths are high-risk, all of which comes from user config. for (const p of [ + "models/marts/mrt_daily_totals.sql", "models/marts/mrt_cost2024.sql", "models/staging/stg_billing2.sql", - "models/intermediate/dbu1_usage.sql", ]) { const r = classifyPR([file(p, "+select 1\n")], { blastRadiusOf: () => 0 }) - expect(r.tier).toBe("full") - expect(r.reasons.join(" ")).toContain("FinOps keyword") + expect(r.tier).toBe("lite") } }) From f7af8af48023c8e3b69b83ffe2e84202cff2690b Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 23 Jul 2026 13:16:16 +0530 Subject: [PATCH 06/10] fix(review): [R20 S4] reject empty-string tokens in riskTierPathTokens (cubic-review P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty string in `riskTierPathTokens.` compiles into a regex alternative like `(?:|foo)` — the empty branch matches the empty string between two adjacent boundary chars, so any path with two `_`/`.`/`-`/`/`/digit chars in a row (e.g. `stg__orders.sql`) silently over-promotes on any configured category. Reject at the config-schema layer via `z.string().min(1)`; the resolver itself is unchanged. Test: `riskTierPathTokens rejects empty-string tokens` under `describe("config")` in review.test.ts — asserts throw on empty token, non-empty tokens still parse. 230/230 review-* tests pass. --- packages/opencode/src/altimate/review/config.ts | 7 ++++++- packages/opencode/test/altimate/review.test.ts | 13 +++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/review/config.ts b/packages/opencode/src/altimate/review/config.ts index 2154a5dc00..793121c510 100644 --- a/packages/opencode/src/altimate/review/config.ts +++ b/packages/opencode/src/altimate/review/config.ts @@ -68,7 +68,12 @@ export const ReviewConfig = z.object({ * inside `broadcaster.sql` or `precast_table.sql`. The `preset:` * marker expands to the current shipped list at reviewer startup. */ - riskTierPathTokens: z.record(z.string(), z.array(z.string())).default({}), + // Tokens must be non-empty. An empty string here compiles into a regex + // alternative that matches everywhere between two boundary characters + // (`(?:|foo)` matches the empty string), silently over-promoting paths + // like `stg__orders.sql` where two boundary chars are adjacent. + // Cubic-review P2 on PR #1028. + riskTierPathTokens: z.record(z.string(), z.array(z.string().min(1))).default({}), }) export type ReviewConfig = z.infer diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index 47dfa40557..e9d9197b77 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -884,6 +884,19 @@ describe("config", () => { const rubric = resolveRubric(cfg) expect(rubric.exclusions.excludeGlobs).toContain("legacy/old.sql") }) + + test("riskTierPathTokens rejects empty-string tokens (cubic-review P2)", () => { + // An empty token would compile to a `(?:|foo)` alternative that + // matches the empty string between two boundary chars — e.g. + // `stg__orders.sql` would silently over-promote. Zod schema + // requires non-empty tokens. + expect(() => + parseReviewConfig("riskTierPathTokens:\n finops:\n - ''\n - cost\n"), + ).toThrow() + // Legit non-empty tokens still parse. + const ok = parseReviewConfig("riskTierPathTokens:\n finops:\n - cost\n - preset:finops\n") + expect(ok.riskTierPathTokens.finops).toEqual(["cost", "preset:finops"]) + }) }) // --------------------------------------------------------------------------- From 0a8c3fa8a5b269ae77925f2606d92a2e686ac129 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 23 Jul 2026 14:46:38 +0530 Subject: [PATCH 07/10] fix(review): [R20 S4] tighten changedLinesForScan marker guard to only emit +/- lines (harness-bot P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit altimate-harness-bot review, PR #1028 risk-tier.ts:251. The earlier guard `if (marker === " ") continue` skipped only unified-diff context lines (space-prefixed). Free-form lines from a raw `git diff -p` output — `diff --git a/... b/...`, `index abc..def`, `Author:`, `Date:` — carry a `""` marker (they don't start with `+`, `-`, or space), fall past the guard, and reach `dbtRiskYmlKeyMatches` via `out.push(raw)`. In-production `file.diff` comes from the GitHub PR files API, whose per-file diff blocks begin at the first `@@` hunk header (no `diff --git` / `index` / commit-metadata lines). So the exposure was defensive parity, not a live promotion bug. But the code was asymmetric — `+++`/`---`/`@@` headers were explicitly skipped at line 220; other free-form lines weren't — and a future caller passing raw `git diff -p` output would silently see header lines in the scanner. Fix (one operator): `if (marker !== "+" && marker !== "-") continue`. Any line whose leading character isn't `+` or `-` is either a context line (fine, still updated scalar state above) or a free-form header (skipped, no emit). Test: `R20 S4: raw git diff -p free-form headers do NOT reach the scanner` builds a full raw-diff string (four header lines + the `@@` hunk + one changed line) and asserts the same tier verdict as the bare-`tests`-word test — trivial, no promotion. 231/231 review-* tests pass. --- .../opencode/src/altimate/review/risk-tier.ts | 13 +++++++--- .../opencode/test/altimate/review.test.ts | 25 +++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/altimate/review/risk-tier.ts b/packages/opencode/src/altimate/review/risk-tier.ts index eda07f6cb8..5b79d7694e 100644 --- a/packages/opencode/src/altimate/review/risk-tier.ts +++ b/packages/opencode/src/altimate/review/risk-tier.ts @@ -241,9 +241,16 @@ function changedLinesForScan(diff: string | undefined): string { scalarIndent = indent } // Only added/removed lines are candidates for regex scanning; context - // lines only feed scalar-state tracking. When the changed line lives - // inside a scalar, emit a blank so the regex has nothing to match. - if (marker === " ") continue + // lines only feed scalar-state tracking. Anything without a `+`/`-` + // marker (context lines, or free-form headers like `diff --git`, + // `index abc..def`, `Author:`, `Date:` from a raw `git diff -p` + // output) is not a changed line and must not reach the scanner — + // the earlier `+++`/`---`/`@@` guard covers unified-diff headers, + // but this closes the gap for anything else (altimate-harness-bot + // review, PR #1028 risk-tier.ts:251). In-production `file.diff` + // comes from the GitHub PR files API and hunks start at `@@`, so + // this is defensive parity, not a live bug. + if (marker !== "+" && marker !== "-") continue out.push(insideScalar ? "" : raw) } return out.join("\n") diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index e9d9197b77..c0d897b5e2 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -688,6 +688,31 @@ describe("risk-tier", () => { expect(r.tier).toBe("full") }) + test("R20 S4: raw `git diff -p` free-form headers do NOT reach the scanner (harness-bot review)", () => { + // altimate-harness-bot review, PR #1028 risk-tier.ts:251. The earlier + // guard skipped only `+++`/`---`/`@@` unified-diff headers and + // context (space-prefixed) lines; free-form lines from a raw + // `git diff -p` output (`diff --git`, `index abc..def`, `Author:`, + // `Date:`) fell through with a `""` marker and reached + // `dbtRiskYmlKeyMatches`. In-production `file.diff` never carries + // these — the GitHub PR files API strips them — so the risk was + // defensive parity only, but exercise it here so a future caller + // passing a raw diff can't silently over-promote. + const diff = + "diff --git a/models/intermediate/int_x.yml b/models/intermediate/int_x.yml\n" + + "index abcdef1..1234567 100644\n" + + "--- a/models/intermediate/int_x.yml\n" + + "+++ b/models/intermediate/int_x.yml\n" + + "@@ -1,3 +1,4 @@\n" + + " models:\n" + + " - name: mrt_x\n" + + '+ description: "See tests documentation for the grain policy"\n' + const r = classifyPR([file("models/intermediate/int_x.yml", diff)]) + // Same content as the bare-`tests`-word test below; must stay trivial + // regardless of whether the caller included raw diff headers. + expect(r.tier).toBe("trivial") + }) + test("R20 S4: bare `tests` (no colon) does NOT match the risk-key regex (word-boundary guard)", () => { // FP guard — a description line like `# tests explanation`, or a // yml value containing the string `tests` without a following colon, From ba6788639b0c57b04fd6dbc7aae78634e43dccf6 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 23 Jul 2026 15:19:36 +0530 Subject: [PATCH 08/10] fix(review): [R20 S4] fail loud on unknown `preset:` in riskTierPathTokens (cubic + harness-bot P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bots on PR #1028 flagged the same silent-failure: a typo in a `preset:` entry (e.g. `preset:finop` instead of `preset:finops`) compiled to an empty token list; then `if (tokens.length === 0) continue` dropped the category with no error. The user's opt-in for a risk-promotion category quietly did nothing — exactly the safety invariant this PR is meant to guarantee. Fix: `compilePathTokenResolver` now throws on an unknown preset name before assembling the resolver. Error message names the offending category, the typo, and the shipped preset list so the reader has a fix pointer: Error: riskTierPathTokens.finops: unknown preset 'finop'. Known presets: finops. Configure a bare token list (e.g. ['card', 'pan']) instead if you want a custom category. The shipped preset list is small and stable, so an unknown name is almost certainly a typo, not a forward reference. Fail at review-start means the misconfiguration is caught immediately, not discovered when a real high-risk PR slips through. Tests: `unknown preset name throws with the known-list in the message` asserts throw on `preset:finop`, throw when a typo is mixed with valid tokens, and that the known preset (`preset:finops`) still resolves promotion correctly on `mrt_cost_daily.sql`. 111/111 tests in review.test.ts pass. --- .../opencode/src/altimate/review/risk-tier.ts | 18 ++++++++++++++- .../opencode/test/altimate/review.test.ts | 23 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/review/risk-tier.ts b/packages/opencode/src/altimate/review/risk-tier.ts index 5b79d7694e..ddad1f9706 100644 --- a/packages/opencode/src/altimate/review/risk-tier.ts +++ b/packages/opencode/src/altimate/review/risk-tier.ts @@ -123,7 +123,23 @@ export function compilePathTokenResolver( if (entry.startsWith("preset:")) { const name = entry.slice("preset:".length) const preset = RISK_TOKEN_PRESETS[name] - if (preset) tokens.push(...preset) + // Unknown preset name is almost certainly a typo (e.g. + // `preset:finop` instead of `preset:finops`); the shipped list is + // small and stable. Silently dropping the entry means the user's + // opt-in for a risk-promotion category quietly does nothing — + // the exact safety invariant this PR exists to guarantee. Fail + // loud instead so the misconfiguration is caught at review + // start, not discovered when a real high-risk PR slips through + // (cubic-review P2 + altimate-harness-bot on PR #1028). + if (!preset) { + const known = Object.keys(RISK_TOKEN_PRESETS).sort().join(", ") || "(none shipped)" + throw new Error( + `riskTierPathTokens.${category}: unknown preset '${name}'. ` + + `Known presets: ${known}. Configure a bare token list (e.g. ` + + `['card', 'pan']) instead if you want a custom category.`, + ) + } + tokens.push(...preset) } else { tokens.push(entry) } diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index c0d897b5e2..12256a8975 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -812,6 +812,29 @@ describe("risk-tier", () => { expect(r.reasons.join(" ")).toContain("high-risk token category 'finops'") }) + test("R20 S4: high-risk token — unknown preset name throws with the known-list in the message", () => { + // cubic-review P2 + altimate-harness-bot on PR #1028 — a typo in a + // `preset:` entry (e.g. `preset:finop` instead of `preset:finops`) + // must not silently no-op the category. Fail loud at resolver-build so + // the misconfiguration is caught before any diff is scanned. + expect(() => compilePathTokenResolver({ finops: ["preset:finop"] })).toThrow(/unknown preset 'finop'/) + // The error message lists the known presets so the reader gets a + // fix pointer rather than "unknown, good luck". + try { + compilePathTokenResolver({ finops: ["preset:finop"] }) + throw new Error("expected throw") + } catch (e: any) { + expect(String(e.message)).toContain("finops") + } + // A category can mix a typo with valid tokens — the typo still fails, + // and the resolver-build never returns a partially-broken resolver. + expect(() => compilePathTokenResolver({ finops: ["cost", "preset:finop", "billing"] })).toThrow(/preset 'finop'/) + // A known preset still works. + const ok = compilePathTokenResolver({ finops: ["preset:finops"] }) + expect(ok).toBeDefined() + expect(ok!("models/marts/mrt_cost_daily.sql")).toBe("finops") + }) + test("R20 S4: high-risk token — no promotion when no resolver is configured (core is neutral)", () => { // Same paths that fire above must stay lite when the caller doesn't // supply a resolver — the reviewer core has zero opinion about which From 9bdd6aa81d2a48bb6bf0a67c43d404824128fad8 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 23 Jul 2026 16:24:48 +0530 Subject: [PATCH 09/10] =?UTF-8?q?fix(review):=20[R20=20S4]=20catch=20riskT?= =?UTF-8?q?ierPathTokens=20config=20error=20in=20orchestrate=20=E2=80=94?= =?UTF-8?q?=20don't=20crash=20the=20run=20(cubic=20P2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to `919bcc17f`. That commit made `compilePathTokenResolver` throw on an unknown `preset:` — the right safety fix — but nothing between the resolver and the CLI handler caught the throw, so a single typo in `.altimate/review.yml` (e.g. `preset:finop` instead of `preset:finops`) would crash every review run for the project's CI (cubic-review P2 on PR #1028 risk-tier.ts:134). Fix: wrap the `compilePathTokenResolver` call in `orchestrate.ts` in a try/catch. On throw: 1. Log a prominent warning to stderr with the config-error message and a fallback banner ("Review continuing without path-token promotion") so the CI log tells the operator what happened. 2. Fall back to `pathTokenCategoryOf = undefined` so `classifyPR` still runs — no promotion fires, but the rest of the review proceeds normally. 3. Prepend the config error to `tierResult.reasons` so it surfaces in `envelope.tierReasons` when `explainTier` is set. A reader of the envelope (or a PR-comment renderer) sees WHY their opt-in didn't fire without having to dig through CI logs. The trade-off is deliberate: the user's opt-in is dead until they fix the typo (conservative safe fallback — no silent auto-approve on billing/PCI/etc. paths), but the review itself doesn't die. Test: `runReview does NOT crash on an unknown-preset config typo — degrades gracefully` builds a minimal fake ReviewRunner + config with `preset:finop` typo, captures stderr, asserts (a) envelope is produced (no crash), (b) stderr contains both the config-error message and the fallback banner, (c) `env.tierReasons` carries the same error so envelope readers see it too. 112/112 tests in review.test.ts pass. --- .../src/altimate/review/orchestrate.ts | 23 ++++++++- .../opencode/test/altimate/review.test.ts | 50 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index b7fec2a1ee..7369510438 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -1096,7 +1096,25 @@ export async function runReview(input: OrchestrateInput): Promise` (typo + // guard, cubic + harness-bot P2). Catch that here so a single config + // typo in `.altimate/review.yml` doesn't crash every review run in the + // project's CI — surface the error to stderr AND propagate a + // `configError` reason into the envelope so the reader can see what + // broke. The review continues with no path-token promotion (the + // conservative safe fallback), so users notice their opt-in is dead + // rather than getting silent auto-approve on billing/PCI/etc. paths + // (cubic-review P2 on PR #1028 risk-tier.ts:134). + let pathTokenCategoryOf: ((p: string) => string | undefined) | undefined + let pathTokenConfigError: string | undefined + try { + pathTokenCategoryOf = compilePathTokenResolver(input.config.riskTierPathTokens) + } catch (e: any) { + pathTokenConfigError = `riskTierPathTokens config invalid: ${e?.message ?? String(e)}` + process.stderr.write(`⚠️ ${pathTokenConfigError}\n Review continuing without path-token promotion.\n`) + pathTokenCategoryOf = undefined + } const tierResult = classifyPR(reviewable, { blastRadiusOf: (p) => { const c = ctxByPath.get(p) @@ -1106,6 +1124,9 @@ export async function runReview(input: OrchestrateInput): Promise ctxByPath.get(f.path)?.complex ?? false, pathTokenCategoryOf, }) + // Surface config errors in the tier-reasons stream so a reader of the + // envelope (or PR comment) sees WHY their opt-in didn't fire. + if (pathTokenConfigError) tierResult.reasons.unshift(pathTokenConfigError) const classifiedTier = tierResult.tier // G2 — --force-tier overrides the classifier. Envelope records both the // forced tier and the original classification whenever the flag is passed diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts index 12256a8975..d9abfcd220 100644 --- a/packages/opencode/test/altimate/review.test.ts +++ b/packages/opencode/test/altimate/review.test.ts @@ -835,6 +835,56 @@ describe("risk-tier", () => { expect(ok!("models/marts/mrt_cost_daily.sql")).toBe("finops") }) + test("R20 S4: runReview does NOT crash on an unknown-preset config typo — degrades gracefully (cubic P2)", async () => { + // cubic-review P2 on PR #1028 — a typo like `preset:finop` in + // `.altimate/review.yml` would make `compilePathTokenResolver` throw + // and, previously, take the entire review run down with it. Catch + // in orchestrate.ts, log to stderr, and continue with no path-token + // promotion; surface the error in `tierReasons` so a reader of the + // envelope (or PR comment) sees why their opt-in didn't fire. + const files: ChangedFile[] = [{ path: "models/marts/m.sql", status: "modified", diff: "+select 1\n" }] + // Minimal runner — this test only needs runReview to reach the + // resolver-build path and back out with an envelope. + const runner: ReviewRunner = { + async check() { return { issues: [], ran: false } }, + async detectPii() { return { columns: [] } }, + async impact() { return { hasManifest: false, severity: "SAFE", directCount: 0, transitiveCount: 0, testCount: 0 } }, + async equivalence() { return { decided: true, equivalent: true } as EquivalenceResult }, + async grade() { return { grade: "A", decided: true } }, + } + // Silence stderr for the expected warning during the test run. + const origWrite = process.stderr.write.bind(process.stderr) + let stderrCaptured = "" + process.stderr.write = ((s: string) => { + stderrCaptured += String(s) + return true + }) as typeof process.stderr.write + try { + const env = await runReview({ + changedFiles: files, + config: { ...DEFAULT_REVIEW_CONFIG, riskTierPathTokens: { finops: ["preset:finop"] } } as any, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner, + getContent: async () => "select 1", + generatedAt: "2026-05-29T00:00:00Z", + explainTier: true, + }) + // Envelope was still produced — no crash. + expect(env.verdict).toBeDefined() + // stderr got the diagnostic + the fallback banner. + expect(stderrCaptured).toContain("riskTierPathTokens config invalid") + expect(stderrCaptured).toContain("Review continuing without path-token promotion") + // Envelope's tier-reasons stream carries the config error so a + // reader who never sees stderr still knows why promotion didn't fire. + const reasons = (env.tierReasons ?? []).join(" ") + expect(reasons).toContain("riskTierPathTokens config invalid") + expect(reasons).toContain("unknown preset 'finop'") + } finally { + process.stderr.write = origWrite + } + }) + test("R20 S4: high-risk token — no promotion when no resolver is configured (core is neutral)", () => { // Same paths that fire above must stay lite when the caller doesn't // supply a resolver — the reviewer core has zero opinion about which From 3c2c03c9977557726841573936052d59ae1367ea Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 23 Jul 2026 18:03:14 +0530 Subject: [PATCH 10/10] fix(review): [R20 S4] surface pathTokenConfigError in envelope even without --explain-tier (coderabbit review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to `9435bc6f9` (the try/catch around `compilePathTokenResolver` that surfaces a config typo in `tierResult.reasons` instead of crashing). The envelope's `tierReasons` field was gated on `input.explainTier || tierForced` — both default false in a normal `comment`/`gate` run — so the config error I prepended to `tierResult.reasons` was silently stripped when the envelope was built. Net effect on a typoed `.altimate/review.yml`: review ran, no promotion fired, no error appeared in the PR comment. Only a stderr trace no customer ever sees. Exact silent-failure mode the fail-loud fix was supposed to close. Fix: expand the gate to include the config-error case. tierReasons: input.explainTier || tierForced || pathTokenConfigError ? tierReasons : undefined Regression test: `config error surfaces in envelope even WITHOUT --explain-tier` builds a config with `finops: [preset:finop]`, calls `runReview` with `explainTier` unset (default), and asserts `env.tierReasons` still contains both `riskTierPathTokens config invalid` and `unknown preset 'finop'`. 113/113 tests in review.test.ts pass (was 112). --- .../src/altimate/review/orchestrate.ts | 8 +++- .../opencode/test/altimate/review.test.ts | 41 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 7369510438..5abc689685 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -1427,7 +1427,13 @@ export async function runReview(input: OrchestrateInput): Promise { } }) + test("R20 S4: config error surfaces in envelope even WITHOUT --explain-tier (coderabbit review)", async () => { + // coderabbit review on PR #1028 orchestrate.ts:1129 — a normal + // `comment` / `gate` run does NOT set `explainTier`, so the envelope's + // `tierReasons` field is dropped. That silently strips the config + // error, and a user with a typoed `.altimate/review.yml` sees no + // reason WHY their opt-in didn't fire in the PR comment — the whole + // point of the earlier fail-loud fix. The envelope gate now includes + // tierReasons whenever a config error was caught, regardless of + // explain-tier. + const files: ChangedFile[] = [{ path: "models/marts/m.sql", status: "modified", diff: "+select 1\n" }] + const runner: ReviewRunner = { + async check() { return { issues: [], ran: false } }, + async detectPii() { return { columns: [] } }, + async impact() { return { hasManifest: false, severity: "SAFE", directCount: 0, transitiveCount: 0, testCount: 0 } }, + async equivalence() { return { decided: true, equivalent: true } as EquivalenceResult }, + async grade() { return { grade: "A", decided: true } }, + } + const origWrite = process.stderr.write.bind(process.stderr) + process.stderr.write = (() => true) as typeof process.stderr.write + try { + // NOTE: explainTier is intentionally NOT set here (default false). + const env = await runReview({ + changedFiles: files, + config: { ...DEFAULT_REVIEW_CONFIG, riskTierPathTokens: { finops: ["preset:finop"] } } as any, + rubric: DEFAULT_RUBRIC, + mode: "comment", + runner, + getContent: async () => "select 1", + generatedAt: "2026-05-29T00:00:00Z", + }) + // Envelope was still produced. + expect(env.verdict).toBeDefined() + // Config error surfaces in tierReasons DESPITE explainTier being unset. + const reasons = (env.tierReasons ?? []).join(" ") + expect(reasons).toContain("riskTierPathTokens config invalid") + expect(reasons).toContain("unknown preset 'finop'") + } finally { + process.stderr.write = origWrite + } + }) + test("R20 S4: high-risk token — no promotion when no resolver is configured (core is neutral)", () => { // Same paths that fire above must stay lite when the caller doesn't // supply a resolver — the reviewer core has zero opinion about which