diff --git a/packages/opencode/src/altimate/review/config.ts b/packages/opencode/src/altimate/review/config.ts index b4f775c2a..793121c51 100644 --- a/packages/opencode/src/altimate/review/config.ts +++ b/packages/opencode/src/altimate/review/config.ts @@ -46,6 +46,34 @@ 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. + */ + // 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/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index 07c60565b..5abc68968 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,29 @@ 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. + // + // `compilePathTokenResolver` throws on an unknown `preset:` (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) @@ -1099,7 +1122,11 @@ export async function runReview(input: OrchestrateInput): Promise (ctxByPath.get(f.path)?.pii.length ?? 0) > 0, isComplexOf: (f) => 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 @@ -1400,7 +1427,13 @@ export async function runReview(input: OrchestrateInput): Promise 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] + // 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) + } + } + 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 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 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 + /** 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 { @@ -60,11 +205,100 @@ 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 + // 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. + 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. + // 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. + 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. 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") +} + +/** 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, @@ -76,6 +310,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: dbtRiskYmlKeys.length > 0, + dbtRiskYmlKeys, + martLayerChange: MARTS_DIR_RE.test(file.path), + highRiskPathTokenCategory: opts.pathTokenCategoryOf?.(file.path), } } @@ -91,6 +329,30 @@ 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 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. + // + // 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 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 + // 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 ${keys}${loc}`) + } + 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 17e1ae978..24bde20a7 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, @@ -501,6 +503,504 @@ 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") + // Reason names the specific key that fired (consensus MINOR #4). + expect(r.reasons.join(" ")).toContain("constraints") + }) + + 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 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: `|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 + // 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: 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, + // 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. + // 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" + + "+ - dbt_utils.unique_combination_of_columns:\n" + + "+ combination_of_columns:\n" + + "+ - 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("high-risk token") + expect(r.reasons.join(" ")).not.toContain("mart-API surface") + }) + + // 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("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("high-risk token category 'finops'") + } + }) + + 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: 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("full") + 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: 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: 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 + // 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", + ]) { + const r = classifyPR([file(p, "+select 1\n")], { blastRadiusOf: () => 0 }) + expect(r.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") + }) }) // --------------------------------------------------------------------------- @@ -523,6 +1023,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"]) + }) }) // ---------------------------------------------------------------------------