diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json new file mode 100644 index 0000000000..6e572a9604 --- /dev/null +++ b/.claude-plugin/marketplace.json @@ -0,0 +1,20 @@ +{ + "name": "altimate-code", + "owner": { + "name": "Altimate AI", + "email": "support@altimate.ai" + }, + "metadata": { + "description": "Altimate Code as a Claude Code plugin — deterministic dbt/SQL analysis driven by the altimate engine", + "version": "0.1.0" + }, + "plugins": [ + { + "name": "altimate-data", + "source": "./plugins/claude-code", + "description": "Deterministic dbt and SQL analysis: lint, grade, PII, column-level lineage, blast radius, and schema verification, powered by the altimate CLI", + "category": "data-engineering", + "keywords": ["dbt", "sql", "lineage", "trino", "data-engineering"] + } + ] +} diff --git a/.opencode/skills/dbt-factor/SKILL.md b/.opencode/skills/dbt-factor/SKILL.md new file mode 100644 index 0000000000..95f44c06c5 --- /dev/null +++ b/.opencode/skills/dbt-factor/SKILL.md @@ -0,0 +1,27 @@ +--- +name: dbt-factor +description: Use when the same or near-duplicate SQL (CTEs, filters, aggregations) is repeated across several dbt models, or when asked to compute it once and reuse it, to deduplicate model logic, or to cut repeated scans. Proposes a shared intermediate model, with an equivalence proof for each consumer. +--- + +# Compute repeated logic once + +## Requirements +**Agent:** `builder` (edits model files) +**Tools used:** `bash` (`altimate dbt-factor`, `altimate dbt-impact`, `altimate dbt-parity`), `read`, `edit` + +```bash +altimate dbt-factor --project [--min-occurrences 2] [--json] +altimate dbt-factor --model # only proposals that touch this model +altimate dbt-factor --apply [--proposal N] # write the patch +``` + +Each proposal is a multi-file patch: +- a new `models/intermediate/int_.sql` with a yml stub; +- an edit to each consumer so it reads the new model; +- a proof for each consumer, plus the savings estimate and the storage trade-off (the + new model is materialised). + +`--apply` writes the files only when no consumer proof is REFUTED and every consumer's +Jinja maps back safely. It then runs `dbt-impact` on each consumer it touched and +prints a `dbt-parity` command for each one. Run those commands after building +(skill `dbt-parity`). Set a `{{ config() }}` on the new model before you build it. diff --git a/.opencode/skills/dbt-impact/SKILL.md b/.opencode/skills/dbt-impact/SKILL.md new file mode 100644 index 0000000000..0b09e03aa7 --- /dev/null +++ b/.opencode/skills/dbt-impact/SKILL.md @@ -0,0 +1,60 @@ +--- +name: dbt-impact +description: Use before changing, dropping, renaming or retyping a dbt model or column, adding a column that another job or model reads, changing its grain or materialization, or when asked "what depends on this?" or "what breaks downstream?". Lists every dbt model, loader and served table hop by hop, with a BREAKING / RISKY / SAFE verdict for each. +--- + +# What breaks downstream + +## Requirements +**Agent:** any (read-only analysis) +**Tools used:** `bash` (`altimate dbt-impact`), `read` + +```bash +altimate dbt-impact --model [--column ] \ + --kind drop|rename|retype|grain|materialization|add \ + [--new-name x | --to-type t | --to incremental [--strategy delete+insert] [--unique-key k]] \ + [--sinks sink_map.json | --no-exposure-sinks] [--json] +# a branch's changes: both sides compiled with dbt, compiled SQL compared +altimate dbt-impact --base origin/main [--head HEAD] [--sinks sink_map.json] \ + [--jobs jobs.json | --jobs-from selectors] [--live] [--profiles-dir d] [--dbt-bin path] +# or from two already compiled manifests +altimate dbt-impact --base-manifest base/manifest.json --head-manifest head/manifest.json [--jobs ...] +``` + +- Each hop is a dbt child, a dbt descendant, a loader, or a served table, with a + verdict and its evidence. `Overall` is the worst verdict, followed by its reasons. +- Without `--sinks`, the sink map comes from exposures whose `meta.sink` declares + `{loader, target, columns, read_mode}`. That map is partial: a mart with no exposure has + UNKNOWN loaders, not none, unless the dbt var `sink_map_complete: true` is set. An + explicit `--sinks` map is complete. With no map at all the verdict is UNKNOWN + (`sinks_unknown`), never SAFE. A clean dbt graph alone does not show that a mart column + is unused. A loader that reads a column tolerantly (fills NULL) stays BREAKING. +- `--kind materialization` defaults to the `append` strategy. Pass `--strategy` (and + `--unique-key` when the new config sets one); the report then carries `delete_scope` + and the `reader_requirements` every reader must meet. +- Diff mode compiles base and head (`dbt compile`, offline first: no warehouse needed + unless a model introspects) and compares `compiled_code`, so `{% set %}`, vars and + macros classify. It detects added, renamed and retyped columns (`resolved_kind`). + `--no-compile` falls back to a naive render; models it cannot render are + `UNCLASSIFIED`. Run those again with `--model`. +- **Schema lag** (`schema_lag`): CI builds everything at once, production does not. + For every column a change adds, renames or retypes, readers are checked for: + - `schema_lag:cross_job` (BREAKING): the reader's job does not build the producer. + Needs a job map: `--jobs jobs.json` (`{"jobs": {name: {"select", "exclude", + "schedule"}}}`, dbt selection syntax, resolved with `dbt ls`) or + `--jobs-from selectors` (one job per `selectors.yml` selector; cron from a + `schedules.json` next to it). Without one, cross-job is skipped and the report says so. + - `schema_lag:incremental_needs_full_refresh` (BREAKING): the producer is incremental + with `on_schema_change` ignore (the default) or fail. + - `schema_lag:source_not_landed`: the reader uses a source column newly declared in + YAML. RISKY without a live catalog; `--live` reads the deployed columns over the + configured connection and makes it BREAKING when the column is missing. + - Ephemeral producers are inlined and never lag. + Each finding names the consumer, column, producer, jobs (with cron and worst-case + window) and one fix (e.g. `+` parent selection, `on_schema_change: + append_new_columns`, a two-phase release). +- Use `--fail-on breaking|risky` to gate CI. + +Report BREAKING hops and schema-lag findings first and name the consumers, jobs and the +suggested fix. Do not call a change safe when the sink map was missing or when cross-job +was skipped for lack of a job map on a project that runs several jobs. diff --git a/.opencode/skills/dbt-optimize/SKILL.md b/.opencode/skills/dbt-optimize/SKILL.md new file mode 100644 index 0000000000..1bbab37052 --- /dev/null +++ b/.opencode/skills/dbt-optimize/SKILL.md @@ -0,0 +1,57 @@ +--- +name: dbt-optimize +description: Use when asked to optimise, speed up, or cut the cost / memory / bytes scanned of a dbt model, or to find and fix its performance or cost problem — especially when the output rows must stay exactly the same. Finds the cost problem, proposes rewrites with an equivalence proof, applies one safely, and hands off to a data parity check. +--- + +# Optimise a dbt model without changing its rows + +## Requirements +**Agent:** `builder` (edits model files) +**Tools used:** `bash` (`altimate dbt-perf`, `altimate dbt-optimize`, `altimate dbt-parity`), `read`, `edit` + +Run from the dbt project directory. All steps are deterministic engine calls. + +## 0. Compile first +`altimate` reads `target/manifest.json` and needs the COMPILED SQL in it: +`dbt compile` (not `dbt parse`). Check `had_compiled_sql: true` in step 1's summary. + +## 1. Find the cost problem +```bash +altimate dbt-perf --format json [--run-results target/run_results.json --sort cost] +``` +Keep the findings whose `model` is the target. `--run-results` adds measured runtime +and ranks by it. A finding's `guardrail` of kind `materialization_change` comes with +`delete_scope` and `reader_requirements` — quote both if you act on it. + +## 2. Get proven rewrites +```bash +altimate dbt-optimize --model [--json] +``` +Each proposal has a proof: +- **PROVEN** — the engine showed both versions return the same rows. Apply it. +- **REFUTED** — it changes the rows; the differences are listed. Never apply it. +- **UNDECIDABLE** — unknown; apply only if step 4 then passes. + +## 3. Apply +```bash +altimate dbt-optimize --model --apply [--proposal N] [--allow-unproven] +``` +It keeps the `{{ config() }}` block and turns relations back into `ref()`s. When it +prints `NOT APPLIED` (Jinja such as `{% if %}`, macros or `var()`), it prints the +compiled-SQL patch: port that change into the model by hand, keeping the Jinja. +If no proposal fits the finding, write the fix yourself. The step 4 bar still applies. + +## 4. Prove the rows did not change +Build the edited model into a separate relation (e.g. a dev target), then run the +command step 3 printed: +```bash +altimate dbt-parity --model --before --after [--columns a,b,c] +``` +`PARITY: MATCH` (exit 0) is the bar. Details are in the `dbt-parity` skill. +Without a warehouse connection, say so and report the proof from step 2 as the only +evidence. + +## Report +State the finding (with cost, if measured), the change, its proof status, and the +parity result. Before you change a column or the materialization, run +`altimate dbt-impact` (skill `dbt-impact`). diff --git a/.opencode/skills/dbt-parity/SKILL.md b/.opencode/skills/dbt-parity/SKILL.md new file mode 100644 index 0000000000..9460d07578 --- /dev/null +++ b/.opencode/skills/dbt-parity/SKILL.md @@ -0,0 +1,37 @@ +--- +name: dbt-parity +description: Use to verify that a changed or rewritten dbt model returns exactly the same rows as before. It proves the data is unchanged: row counts, duplicate grain keys, EXCEPT both ways, and per-column match rates, run over a warehouse connection. +--- + +# Prove a dbt model change kept the same data + +## Requirements +**Agent:** any (read-only analysis) +**Tools used:** `bash` (`altimate dbt-parity`), `read` + +```bash +altimate dbt-parity --model --before --after \ + [--connection ] [--columns a,b,c] [--sample-rows N] [--json] +``` + +- `--before`: the relation built from the old SQL (usually prod). `--after`: the same + model built from the new SQL into another schema or target. +- The grain comes from the model's uniqueness test, its contract primary key, or its + `unique_key`. The output says which one it used. +- `--columns` turns on per-column match rates, which name the column that differs. +- Exit 0 and `PARITY: MATCH` mean the rows are the same. `MISMATCH` and `INCONCLUSIVE` + exit 1. `INCONCLUSIVE` means a check errored. It is never a pass. +- `--connection`: a configured warehouse connection. Without one, the first configured + connection is used. A connection can also come from an environment variable: + `ALTIMATE_CODE_CONN_='{"type":"trino","host":…,"port":…,"user":…,"catalog":…}'`. + +## PII: samples are opt-in +Without `--sample-rows`, only counts leave the warehouse. With `--sample-rows N`, +up to N differing rows are printed each way, and they end up in this conversation. +Ask the user before you use it on tables that may hold personal or regulated data. + +## Reading a mismatch +- Row delta with EXCEPT = 0: rows were duplicated or dropped. Check + `Duplicated grain keys`. +- EXCEPT > 0 both ways with equal counts: values changed. The column match rates name + the columns. diff --git a/.opencode/skills/dbt-pr-review/SKILL.md b/.opencode/skills/dbt-pr-review/SKILL.md index 8d5f25144f..ef4f74f408 100644 --- a/.opencode/skills/dbt-pr-review/SKILL.md +++ b/.opencode/skills/dbt-pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: dbt-pr-review -description: Cloudflare-style AI code review for dbt/SQL pull requests. Produces a signed APPROVE/COMMENT/REQUEST_CHANGES verdict where every blocking finding is backed by a deterministic engine call — column-lineage blast radius, query equivalence, PII classification, and A–F grade. Use to review a dbt PR or the working-tree changes before merge. +description: Cloudflare-style AI code review for dbt/SQL pull requests. Produces a signed APPROVE/COMMENT/REQUEST_CHANGES verdict where every blocking finding is backed by a deterministic engine call — column-lineage blast radius, query equivalence, PII classification, and A–F grade. Use to review a dbt PR or the working-tree changes before merge. Reviews a DIFF; for the whole project use `dbt-project-review`. --- # dbt PR Review @@ -9,6 +9,14 @@ description: Cloudflare-style AI code review for dbt/SQL pull requests. Produces **Agent:** `reviewer` (read-only) — also works from `analyst`/`builder`. **Tools used:** `dbt_pr_review` (primary), `impact_analysis`, `altimate_core_equivalence`, `altimate_core_check`, `lineage_check`, read-only `git`. +## Scope — diff, not project + +This reviews **a diff**: base..head, defaulting to the merge-base with `origin/main`. +A model that has been wrong since the day it was written will not appear unless it is in +the diff. If the question is "is this project healthy?" rather than "is this change +safe?", use `dbt-project-review` (`altimate review --all`), which reviews every model in +the manifest and reports the base-vs-head lanes as skipped. + ## When to Use This Skill Use when the user wants to: diff --git a/.opencode/skills/dbt-project-review/SKILL.md b/.opencode/skills/dbt-project-review/SKILL.md new file mode 100644 index 0000000000..1ec2f3fd4b --- /dev/null +++ b/.opencode/skills/dbt-project-review/SKILL.md @@ -0,0 +1,144 @@ +--- +name: dbt-project-review +applyPaths: + - "dbt_project.yml" + - "**/dbt_project.yml" +description: Review an ENTIRE dbt project rather than a diff — every model in the manifest, plus a deterministic performance pass. Use when the question is "is this project healthy?", "what technical debt is in here?", "audit the whole warehouse", or when onboarding to an unfamiliar dbt repo. For "is this change safe?" use `dbt-pr-review` instead, which reviews a diff. +--- + +# dbt project review + +## Requirements +**Agent:** `reviewer` (read-only) — also works from `analyst`. +**Tools used:** `bash` (`altimate review --all`, `altimate dbt-perf`), read-only `git`, `read`. + +## Diff vs project — pick the right one + +| Question | Command | Skill | +| --- | --- | --- | +| Is **this change** safe to merge? | `altimate review` | `dbt-pr-review` | +| Is **this project** healthy? | `altimate review --all` | this one | + +They are not interchangeable. `altimate review` defaults to the merge-base with +`origin/main`; a model that has been wrong since the day it was written never appears in +its output. In a mature repo that is most of the findings — a project running +`altimate review` in CI for months can still be sitting on dozens of untouched +project-level issues, because none of them was ever in a recent diff. + +## Preflight — compile, do not just parse + +The engine analyses SQL, not Jinja. **`dbt parse` is not enough for a project review.** +It writes a manifest (so the DAG, contracts and tests resolve) but no compiled SQL, and +every model that falls back to raw Jinja fails to parse at `{{ config(...) }}` on line 1 +— the AST lanes then return **zero findings for that model**, which is indistinguishable +from a clean one. + +```bash +cd +dbt deps +dbt compile --profiles-dir . # writes target/manifest.json AND target/compiled/** +``` + +`dbt compile` needs credentials to resolve `source()` metadata but does not run queries. +If compile is impossible, run anyway — but read the compiled-coverage line in the report +(below) and say plainly how much of the project was actually analysed. + +## 1. Project review + +```bash +altimate review --all \ + --cwd \ + --manifest target/manifest.json \ + --severity warning \ + --no-ai \ + --json +``` + +- `--all` is mutually exclusive with `--base`/`--head` — it has no base ref. +- `--severity suggestion` for everything; `warning` is the useful default on a first pass. +- `--max-models N` caps the run (`0` lifts the cap; the default is 500). Any truncation + is announced on stderr — never assume it didn't happen. +- `--mode gate` exits non-zero on `REQUEST_CHANGES`, for a scheduled CI health check. +- `--no-ai` keeps it deterministic and free. + +### Read `lanes` before you read `findings` + +The envelope carries a `lanes` object that the diff mode does not have: + +```jsonc +"lanes": { + "ran": ["core_lint", "dbt_patterns", "lexical_scan", "dbt_config_lint", + "grain_mismatch", "missing_grain_test", "sibling_filter_consistency", + "pii_classify", "pii_detect"], + "skipped": [{ "lane": "semantic_change", "reason": "proves base-vs-head row equivalence; …" }], + "compiledModels": 7, + "totalModels": 214 +} +``` + +Two rules: + +- **Every base-vs-head lane is skipped and says so.** Equivalence, structural diff, + column breakage, data-diff, config diff, lineage breakage, topology equivalence and + the schema.yml test-removal detector all compare two versions of a model; project + scope has one. Do not report their silence as "no issues found there" — report them + as not run, and point the user at `dbt-pr-review` for those. +- **`compiledModels` < `totalModels` means the report is partial.** Say the number. A + review where 207 of 214 models had no compiled SQL analysed 3% of the project with + the AST lanes, and the honest headline is that, not the finding count. + +## 2. Performance pass + +`altimate review --all` covers correctness, PII, contracts and grain. It does not do +the cost/performance rule set — that is a separate deterministic command: + +```bash +altimate dbt-perf --cwd --manifest target/manifest.json \ + --format json --severity warning +``` + +- It prints `skipped_rules` for every rule that could not run, and a summary line + carrying `had_compiled_sql` / `had_sink_map` / `had_run_results` / `parse_failures`. Surface both: a rule that could not run + is not a clean result. +- Where a finding carries a `guardrail` of kind `materialization_change`, report its + `delete_scope` and `reader_requirements` verbatim. Those fields exist because a past + performance fix switched a materialization without dropping the old relation and + caused a data incident. A recommendation to change a materialization that omits them + is not a complete recommendation. +- `--fail-on warning|error` for CI. +- `--run-results target/run_results.json --sort cost` ranks findings by measured runtime; + `--sinks ` lets the column rules see loaders outside dbt. +- If it reports that it needs a newer `@altimateai/altimate-core`, say so and stop — + do not substitute a hand-rolled performance opinion for the engine's. + +## Acting on the findings + +This skill reports. To fix what it finds, use these skills, which work on one model +or one change at a time: + +- Performance or cost findings → `dbt-optimize` (`altimate dbt-optimize --model X`): + rewrites with an equivalence proof, applied safely. +- Logic repeated across models (P104 and similar) → `dbt-factor` + (`altimate dbt-factor --project`). +- Before you drop, rename or retype a column, or change a materialization → + `dbt-impact` (`altimate dbt-impact --model X --column c`). It includes loaders and + served tables when you pass `--sinks`. +- After any rewrite → `dbt-parity` to prove the rows are unchanged. + +## 3. Reporting + +Lead with **scope and coverage**, then findings: + +1. How many models were analysed, how many had compiled SQL, which lanes ran and which + did not. +2. Findings grouped by category (not by file) and ordered by severity — in a project + review the pattern matters more than the instance. "31 models cross join without an + `ON` clause" is the finding; the 31 paths are the evidence. +3. The three or four themes worth fixing first, with the concrete first step. + +Two rules carried over from `dbt-pr-review`: + +- **Do not restate a finding you cannot see in the source.** Open the file and confirm + the line before repeating what the engine said. +- **Never present a partial run as a complete one.** If compile did not happen, or the + model cap bit, or a lane was skipped, that belongs in the first paragraph. diff --git a/.opencode/skills/query-optimize/SKILL.md b/.opencode/skills/query-optimize/SKILL.md index 6d1dd6d6d2..2f3c0eb309 100644 --- a/.opencode/skills/query-optimize/SKILL.md +++ b/.opencode/skills/query-optimize/SKILL.md @@ -11,6 +11,14 @@ description: Analyze and optimize SQL queries for better performance Analyze SQL queries for performance issues and suggest concrete optimizations including rewritten SQL. +## dbt models: use `dbt-optimize` instead + +If the SQL is a **dbt model** (a file under `models/` or a model name), use the +`dbt-optimize` skill. It reads the compiled project, ties rewrites to the project's +perf findings, writes the proven rewrite back into the model while keeping its Jinja, +and hands off to `dbt-parity` to prove the rows did not change. Use `dbt-factor` when +the cost is logic repeated across models. This skill is for standalone SQL. + ## Workflow 1. **Get the SQL query** -- Either: diff --git a/docs/docs/usage/dbt-pr-review.md b/docs/docs/usage/dbt-pr-review.md index 2c2a19afe5..cb04942dc7 100644 --- a/docs/docs/usage/dbt-pr-review.md +++ b/docs/docs/usage/dbt-pr-review.md @@ -118,6 +118,8 @@ Options: | `--explain-tier` | Emit the classifier's tier-reason list on the verdict envelope so you can see why a diff was rated `trivial`, `lite`, or `full`. Reasons already surface in the PR comment for `full`-tier runs — this flag adds them to `trivial`/`lite` for debugging. | | `--force-tier ` | **[EXPERIMENTAL / bench debug]** Bypass the classifier and force `trivial` / `lite` / `full`. The verdict envelope carries `tierForced: true` and the classifier's original decision for audit. | | `--json` / `--output ` | Emit the verdict envelope as JSON. | +| `--all` | **Project scope**: review every model in the manifest instead of a diff. Mutually exclusive with `--base`/`--head`. See [Project review](#project-review---all). | +| `--max-models ` | `--all` only. Cap how many models are reviewed (`0` = no cap; default 500). Truncation is always announced on stderr. | > **Full vs lint-only.** With a compiled `manifest.json` present, the reviewer > proves lineage and equivalence exactly. Without it (or without a warehouse) it @@ -158,6 +160,78 @@ Options: different surface — they gate the **agent's own build loop** so it can't declare "done" on failing dbt tests or schema drift. +## Project review (`--all`) + +`altimate review` reviews **a diff**. A model that has been wrong since the day it was +written never appears in its output — which is why a project that has run the reviewer +in CI for months can still be sitting on untouched project-level findings. + +`--all` is the other shape: every model in the manifest, one version each, no base ref. + +```bash +altimate review --all --manifest target/manifest.json --severity warning --no-ai +``` + +### What runs, and what cannot + +Tiering is a cost control for diffs, so `--all` always runs at `full` tier. The lanes +that evaluate **one version** of a model run; the lanes that compare a **base and a +head** have no input and are reported as skipped — with the reason — on the envelope's +`lanes` field and in the rendered summary. + +| Runs | Skipped (needs a base version) | +|------|-------------------------------| +| `core_lint` (AST lint / warehouse cost) | `semantic_change` (equivalence) | +| `dbt_patterns` + the declarative rule catalog | `structural_diff` (the `*_change` rules) | +| `lexical_scan` | `dbt_config_diff` (materialization / incremental change) | +| `dbt_config_lint` | `column_breakage` | +| `grain_mismatch` | `lineage_breakage` | +| `missing_grain_test` | `data_diff` | +| `sibling_filter_consistency` | `topology_equivalence` | +| `pii_classify` / `pii_detect` | `schema_yml_test_removal` | +| | `ai_review` (diff-grounded, caps at 20 files) | +| | `dbt_patterns:fanout_join` (fires on a join *added* to an aggregating model) | + +A report that silently omitted half the lanes would be worse than no report, so the +skip list is part of the output rather than a footnote. + +The last entry is the subtle one. Presenting a whole model as added is what makes the +diff-anchored detectors evaluate it at all, but a detector whose *condition* is "this +appeared in the diff" then degenerates. `fanout_join` ("a join was ADDED to a model that +aggregates") becomes "this model aggregates and has a join", which describes most correct +analytics SQL — it fired on 65 of 215 models before being scoped out. Detectors that test +a whole-file property (`select *`, `ORDER BY` in a CTE) are unaffected and still run. + +### Compile first — `dbt parse` is not enough + +The engine parses SQL, not Jinja. A model that falls back to raw `{{ config(...) }}` +source fails to parse at line 1, every AST rule is skipped **inside** the engine, and +the lane returns zero findings — indistinguishable from a clean model. Over a whole +project that silence is the difference between a review and a rubber stamp, so `--all` +counts it and says so: + +``` +⚠️ 207 of 214 models had no compiled SQL — the engine analysed their raw Jinja, + which the SQL parser cannot read, so the AST lanes (core lint, PII classification, + grain, filter consistency) returned nothing for them. Run `dbt compile` and re-run. +``` + +The same counts are on the envelope as `lanes.compiledModels` / `lanes.totalModels`. + +### Performance + +`--all` covers correctness, PII, contracts and grain, not cost. The performance rule +set is a separate deterministic command: + +```bash +altimate dbt-perf --cwd --manifest target/manifest.json --format json +``` + +It reports `skipped_rules` alongside `findings`; a rule that could not run is not a +clean result. Findings carrying a `MaterializationChange` guardrail print their +`delete_scope` and `reader_requirements` — those fields exist because a past +performance fix switched a materialization without dropping the old relation. + ## GitHub Action Add the review to any repo with a workflow that compiles the project, then runs diff --git a/packages/opencode/src/altimate/native/altimate-core.ts b/packages/opencode/src/altimate/native/altimate-core.ts index 1e8a22b7ef..efc58d0cf8 100644 --- a/packages/opencode/src/altimate/native/altimate-core.ts +++ b/packages/opencode/src/altimate/native/altimate-core.ts @@ -51,6 +51,66 @@ function fail(error: unknown): AltimateCoreResult { return { success: false, data: {}, error: String(error) } } +/** Returned when the installed @altimateai/altimate-core predates the dbt-perf + * bindings. Names the fix rather than the symbol, because the reader is a CLI + * user, not the person who wrote the binding. */ +export const DBT_PERF_UNAVAILABLE = + "dbt-perf needs a newer @altimateai/altimate-core than the one installed " + + "(no analyze_dbt_perf binding). Build and link it locally:\n" + + " cd /crates/altimate-core-node && npm run build\n" + + " cd /packages/opencode && bun link @altimateai/altimate-core" + +// altimate_change start — optional bindings for the dbt mart optimizer commands +/** The message for a binding this altimate-core build does not export. Names + * the export and the command, so the reader knows which build they need. */ +export function bindingUnavailable(exportName: string, command: string): string { + return ( + `this altimate-core build lacks ${exportName} (needed by \`${command}\`). ` + + "Build and link a newer one, or point NAPI_RS_NATIVE_LIBRARY_PATH at a local build:\n" + + " cd /crates/altimate-core-node && npm run build\n" + + " export NAPI_RS_NATIVE_LIBRARY_PATH=/altimate-core..node" + ) +} + +/** True when the loaded core exports `name` as a function. */ +export function hasBinding(name: string): boolean { + return typeof (core as unknown as Record)[name] === "function" +} + +type AnyBinding = (...args: unknown[]) => unknown + +/** Call an export that may not exist in this core build. The bindings return a + * `serde_json::Value` (an object once napi converts it) or a JSON string — + * directly, or through a Promise for the async ones (`proposeRewritesForModel` + * runs the equivalence engine on a tokio runtime); all are accepted. */ +async function callOptionalBinding( + exportName: string, + command: string, + invoke: (fn: AnyBinding) => unknown, +): Promise { + const fn = (core as unknown as Record)[exportName] + if (typeof fn !== "function") return fail(bindingUnavailable(exportName, command)) + try { + const raw = await invoke(fn as AnyBinding) + const value = typeof raw === "string" ? JSON.parse(raw) : raw + return ok(true, Array.isArray(value) ? { value: toPlain(value) } : toData(value)) + } catch (e) { + return fail(e) + } +} + +function toPlain(v: unknown): unknown { + return v === undefined ? undefined : JSON.parse(JSON.stringify(v)) +} + +/** A list result arrives either bare (wrapped as `value`) or under `key`. */ +function asArray(data: Record, key: string): unknown[] { + if (Array.isArray(data.value)) return data.value + if (Array.isArray(data[key])) return data[key] as unknown[] + return [] +} +// altimate_change end + // --------------------------------------------------------------------------- // IFF / QUALIFY transpile transforms (ported from Python guard.py) // --------------------------------------------------------------------------- @@ -370,6 +430,64 @@ export function registerAll(): void { return fail(e) } }) + /** + * Whole-project dbt performance analysis. + * + * The `analyze_dbt_perf*` bindings ship in a LATER altimate-core than the + * pinned one, so they are resolved at call time rather than imported: an + * `import { analyzeDbtPerf }` would make every other core handler in this + * module fail to load on the pinned release. When they are absent the + * handler returns an actionable error instead of an + * "undefined is not a function" stack — see cli/cmd/dbt-perf.ts, which + * surfaces it verbatim. + */ + register("altimate_core.dbt_perf", async (params) => { + const c = core as unknown as Record + const fromManifest = c["analyzeDbtPerfFromManifest"] + const fromDir = c["analyzeDbtPerf"] + const useManifest = typeof params.manifest_json === "string" && params.manifest_json.length > 0 + const fn = useManifest ? fromManifest : fromDir + if (typeof fn !== "function") return fail(DBT_PERF_UNAVAILABLE) + try { + const raw = (fn as (a: string, b?: string) => unknown)( + useManifest ? (params.manifest_json as string) : (params.project_dir ?? ""), + params.options_json ?? undefined, + ) + // The binding returns a `serde_json::Value` (a JS object once napi has + // converted it). Tolerate a JSON string too, which is how every other + // review binding in this file returns its payload. + const report = typeof raw === "string" ? JSON.parse(raw) : raw + return ok(true, toData(report)) + } catch (e) { + return fail(e) + } + }) + + // altimate_change start — dbt mart optimizer: rewrite proposals, parity plans, + // impact analysis and factoring. Resolved at call time like dbt_perf; a + // missing export is an actionable error naming the export, never an empty result. + register("altimate_core.dbt_rewrites", async (params) => + callOptionalBinding("proposeRewritesForModel", "altimate dbt-optimize", (fn) => + fn(params.project_dir, params.model, params.options_json ?? undefined), + ).then((r) => (r.success ? ok(true, { proposals: asArray(r.data, "proposals") }) : r)), + ) + register("altimate_core.dbt_parity_plan", async (params) => + callOptionalBinding("dbtParityPlan", "altimate dbt-parity", (fn) => + fn(params.project_dir, params.model, params.before, params.after, params.options_json ?? undefined), + ), + ) + register("altimate_core.dbt_impact", async (params) => + callOptionalBinding("analyzeDbtImpact", "altimate dbt-impact", (fn) => + fn(params.project_dir, params.change_json, params.options_json ?? undefined), + ), + ) + register("altimate_core.dbt_factoring", async (params) => + callOptionalBinding("proposeFactoring", "altimate dbt-factor", (fn) => + fn(params.project_dir, params.options_json ?? undefined), + ).then((r) => (r.success ? ok(true, { proposals: asArray(r.data, "proposals") }) : r)), + ) + // altimate_change end + // AST base-vs-head structural diff — the `*_change` SQL rules, moved off diff-line regex. register("altimate_core.structural_diff", async (params) => { try { diff --git a/packages/opencode/src/altimate/native/connections/data-diff.ts b/packages/opencode/src/altimate/native/connections/data-diff.ts index 37018a36f3..482d06e6f3 100644 --- a/packages/opencode/src/altimate/native/connections/data-diff.ts +++ b/packages/opencode/src/altimate/native/connections/data-diff.ts @@ -365,6 +365,21 @@ function buildColumnDiscoverySQL(tableName: string, dialect: string): string { const table = esc(parts[parts.length - 1]) return `PRAGMA table_info('${table}')` } + // altimate_change start — Trino: a 3-part name's catalog selects WHICH + // information_schema to read; the session catalog's would miss the table. + // Trino folds unquoted identifiers to lowercase, so the filters are too. + case "trino": { + if (!SAFE_TABLE_NAME.test(tableName)) { + throw new Error(`Unsafe table name for Trino information_schema: ${tableName}`) + } + const lower = parts.map((p) => esc(p.toLowerCase())) + const table = lower[lower.length - 1] + const conditions = [`table_name = '${table}'`] + if (lower.length >= 2) conditions.push(`table_schema = '${lower[lower.length - 2]}'`) + const infoSchema = lower.length === 3 ? `"${lower[0].replace(/"/g, '""')}".information_schema.columns` : "information_schema.columns" + return `SELECT column_name, column_default FROM ${infoSchema} WHERE ${conditions.join(" AND ")} ORDER BY ordinal_position` + } + // altimate_change end default: { // Postgres, Redshift, DuckDB, SQL Server, BigQuery, Databricks, etc. const conditions = [tableFilter] @@ -621,7 +636,10 @@ export function buildPartitionWhereClause( return Number.isNaN(d.getTime()) ? trimmed : d.toISOString().slice(0, 10) })() : partitionValue - const escaped = normalized.replace(/'/g, "''") + // altimate_change start — Trino never coerces a varchar literal to a + // date/timestamp, so the literal is cast, from a zone-less ISO timestamp. + const escaped = (dialect === "trino" ? trinoTimestampLiteral(normalized) : normalized).replace(/'/g, "''") + // altimate_change end // Cast the literal appropriately per dialect switch (dialect) { @@ -638,11 +656,34 @@ export function buildPartitionWhereClause( case "fabric": // Style 23 = ISO-8601 (yyyy-mm-dd), locale-safe return `${expr} = CONVERT(DATE, '${escaped}', 23)` + // altimate_change start — Trino: typed literal (DATE_TRUNC of a DATE + // column compares fine against a TIMESTAMP — Trino widens the date) + case "trino": + return `${expr} = CAST('${escaped}' AS TIMESTAMP)` + // altimate_change end default: return `${expr} = '${escaped}'` } } +// altimate_change start — Trino partition literal normalization +/** + * A partition value as a zone-less `yyyy-mm-dd[ hh:mm:ss[.fff]]` literal that + * `CAST(… AS TIMESTAMP)` accepts. The Trino driver can hand back + * `2024-01-01 00:00:00.000 UTC` (timestamp with time zone) or a JS Date string; + * both are reduced to the ISO prefix. + * + * Exported for targeted unit testing; not part of the stable public API. + */ +export function trinoTimestampLiteral(value: string): string { + const trimmed = value.trim() + const iso = trimmed.match(/^\d{4}-\d{2}-\d{2}(?:[ T]\d{2}:\d{2}(?::\d{2}(?:\.\d+)?)?)?/) + if (iso) return iso[0].replace("T", " ") + const d = new Date(trimmed) + return Number.isNaN(d.getTime()) ? trimmed : d.toISOString().slice(0, 23).replace("T", " ") +} +// altimate_change end + /** * Extract DiffStats from a successful outcome (if present). * diff --git a/packages/opencode/src/altimate/native/dbt/runner.ts b/packages/opencode/src/altimate/native/dbt/runner.ts index b67a16887c..5fde65cf41 100644 --- a/packages/opencode/src/altimate/native/dbt/runner.ts +++ b/packages/opencode/src/altimate/native/dbt/runner.ts @@ -76,5 +76,117 @@ export function runDbt(params: DbtRunParams): Promise { }) } +// altimate_change start — dbt ls / dbt compile without a warehouse, for +// dbt-impact's compiled diff mode and job-map resolution. + +/** Options shared by the offline helpers. Every artifact goes to `targetPath` + * / `logPath` (pass a temp dir), so the project's own `target/` is untouched. */ +export interface DbtOfflineOptions { + projectDir: string + /** dbt executable (default `$ALTIMATE_DBT_BIN`, else `dbt` on PATH). */ + dbtBin?: string + profilesDir?: string + target?: string + targetPath?: string + logPath?: string + timeoutMs?: number +} + +export function dbtBinary(explicit?: string): string { + return explicit || process.env.ALTIMATE_DBT_BIN || "dbt" +} + +function commonArgs(o: DbtOfflineOptions): string[] { + const a = ["--project-dir", o.projectDir] + if (o.profilesDir) a.push("--profiles-dir", o.profilesDir) + if (o.target) a.push("--target", o.target) + if (o.targetPath) a.push("--target-path", o.targetPath) + if (o.logPath) a.push("--log-path", o.logPath) + return a +} + +/** Run `dbt ` (no `+` rewriting, unlike {@link runDbt}). */ +export function runDbtArgs(args: string[], o: Pick): Promise { + return new Promise((resolve) => { + execFile( + dbtBinary(o.dbtBin), + args, + { cwd: o.projectDir, timeout: o.timeoutMs ?? 600_000, maxBuffer: 256 * 1024 * 1024 }, + (error, stdout, stderr) => { + if (!error) return resolve({ stdout: stdout || "", stderr: stderr || "", exit_code: 0 }) + const code = (error as any).code + if (code === "ENOENT") + return resolve({ stdout: "", stderr: `dbt CLI not found (${dbtBinary(o.dbtBin)})`, exit_code: 127 }) + resolve({ + stdout: stdout || "", + stderr: error.killed ? "dbt command timed out" : stderr || error.message, + exit_code: typeof code === "number" ? code : 1, + }) + }, + ) + }) +} + +/** The models a selection resolves to, via `dbt ls` (parse only: no warehouse + * connection). Returns `unique_id`s. */ +export async function dbtLs( + sel: { select?: string; exclude?: string; selector?: string }, + o: DbtOfflineOptions, +): Promise<{ ok: true; ids: string[] } | { ok: false; error: string }> { + const args = ["ls", "--resource-type", "model", "--output", "json", "--output-keys", "unique_id", "--quiet"] + if (sel.selector) args.push("--selector", sel.selector) + if (sel.select?.trim()) args.push("--select", ...sel.select.trim().split(/\s+/)) + if (sel.exclude?.trim()) args.push("--exclude", ...sel.exclude.trim().split(/\s+/)) + const r = await runDbtArgs([...args, ...commonArgs(o)], o) + const ids = parseLsIds(r.stdout) + if (r.exit_code !== 0) return { ok: false, error: lastLines(r.stderr || r.stdout) } + return { ok: true, ids } +} + +/** `unique_id`s from `dbt ls --output json` (one JSON object per line, mixed + * with log lines). */ +export function parseLsIds(stdout: string): string[] { + const ids: string[] = [] + for (const line of stdout.split("\n")) { + const t = line.trim() + if (!t.startsWith("{")) continue + try { + const id = (JSON.parse(t) as { unique_id?: string }).unique_id + if (id) ids.push(id) + } catch { + // a log line that happens to start with "{" + } + } + return ids +} + +/** + * `dbt compile` without touching the warehouse when possible: first with + * `--no-populate-cache --no-introspect` (works while the warehouse is down, + * unless a model needs an introspective query), then a normal compile. + * The manifest lands in `targetPath`. + */ +export async function dbtCompileOffline( + select: string[], + o: DbtOfflineOptions, +): Promise<{ ok: boolean; offline: boolean; error?: string }> { + const base = ["compile", ...(select.length ? ["--select", ...select] : []), ...commonArgs(o)] + const off = await runDbtArgs([...base, "--no-populate-cache", "--no-introspect"], o) + if (off.exit_code === 0) return { ok: true, offline: true } + const on = await runDbtArgs(base, o) + if (on.exit_code === 0) return { ok: true, offline: false } + return { ok: false, offline: false, error: lastLines(on.stdout + "\n" + on.stderr) } +} + +function lastLines(s: string, n = 6): string { + return s + .split("\n") + .map((l) => l.replace(/\x1b\[[0-9;]*m/g, "").trim()) + .filter((l) => l && !/^INFO:/.test(l)) + .slice(-n) + .join(" | ") +} +// altimate_change end + // Exported for testing export { ensureUpstreamSelector } diff --git a/packages/opencode/src/altimate/native/types.ts b/packages/opencode/src/altimate/native/types.ts index ed995589e5..1be6ace403 100644 --- a/packages/opencode/src/altimate/native/types.ts +++ b/packages/opencode/src/altimate/native/types.ts @@ -1277,6 +1277,33 @@ export const BridgeMethods = { params: { base_sql: string; head_sql: string } result: AltimateCoreResult }, + /** Whole-project dbt performance analysis (`altimate dbt-perf`). Exactly one + * of `project_dir` / `manifest_json` is used: the manifest text wins when + * supplied. `data` carries the engine's `PerfReport`. */ + "altimate_core.dbt_perf": {} as { + params: { project_dir?: string; manifest_json?: string; options_json?: string } + result: AltimateCoreResult + }, + /** Rewrite proposals for one model (`altimate dbt-optimize`); `data.proposals`. */ + "altimate_core.dbt_rewrites": {} as { + params: { project_dir: string; model: string; options_json?: string } + result: AltimateCoreResult + }, + /** Data parity plan for a model change (`altimate dbt-parity`); `data` is the `ParityPlan`. */ + "altimate_core.dbt_parity_plan": {} as { + params: { project_dir: string; model: string; before: string; after: string; options_json?: string } + result: AltimateCoreResult + }, + /** Blast radius of a model/column change (`altimate dbt-impact`); `data` is the impact report. */ + "altimate_core.dbt_impact": {} as { + params: { project_dir: string; change_json: string; options_json?: string } + result: AltimateCoreResult + }, + /** Shared-logic factoring proposals (`altimate dbt-factor`); `data.proposals`. */ + "altimate_core.dbt_factoring": {} as { + params: { project_dir: string; options_json?: string } + result: AltimateCoreResult + }, // altimate_change end // --- altimate-core Phase 1 (P0) --- "altimate_core.fix": {} as { params: AltimateCoreFixParams; result: AltimateCoreResult }, diff --git a/packages/opencode/src/altimate/review/README.md b/packages/opencode/src/altimate/review/README.md index ee72b55af1..0efdc7c6ff 100644 --- a/packages/opencode/src/altimate/review/README.md +++ b/packages/opencode/src/altimate/review/README.md @@ -54,7 +54,7 @@ it; lower layers fill gaps and add context but never override a proof. - + ## Layer → file map @@ -67,10 +67,37 @@ it; lower layers fill gaps and add context but never override a proof. | verdict | `verdict.ts` (rubric → verdict, `stableStringify`, HMAC sign/verify), `rubric.ts`, `risk-tier.ts` (`TIER_LANES`) | | entry | `run.ts` (`reviewPullRequest` — wires resolvers + `runAiReview`), `git.ts`, `finding.ts`, `diff-filter.ts`, `format.ts`, `post-github.ts`, `config.ts` | +## Lineage lanes and `analyzeDbtImpact` + +When the runner exposes `dbtImpact` (core `analyzeDbtImpact`, via +`altimate_core.dbt_impact`), the lineage lanes ask it about each removed output +column (modified model) or the model itself (delete/rename). A **BREAKING** hop +outside dbt — a loader or a served table — is the only thing that lifts the +modified-model fan-out cap to `critical` (`lineage_breakage:external-sink`); a +break inside the dbt graph keeps the cap. Without the binding, the lanes behave +as before. The same normalizer backs `altimate dbt-impact` +(`cli/cmd/dbt-impact-helpers.ts`). + +## Schema-lag lane + +`schema_lag` (lite and full tiers, diff scope only) runs when a modified model's +output columns change (column lineage, old vs new) or a YAML file in the diff +changes declared columns. It sends the base → head compiled SQL to +`analyzeDbtImpact` and turns each `schema_lag` finding into a `lineage_breakage` +finding (`ruleKey` = the reason: `schema_lag:cross_job`, +`schema_lag:incremental_needs_full_refresh`, `schema_lag:source_not_landed`; +critical when BREAKING) carrying the suggested fix. Cross-job needs a job map: +`schemaLag: {jobs: }` or `{jobsFrom: selectors}` in `.altimate/review.yml`, +resolved once per review with `dbt ls`; without it the core reports the check as +skipped. + ## Entry points - Tool: `dbt_pr_review` (`../tools/dbt-pr-review.ts`) — callable from any agent. -- CLI: `altimate review` (`../../cli/cmd/review.ts`). +- CLI: `altimate review` (`../../cli/cmd/review.ts`) — a **diff**; `--all` switches it to + project scope (every model in the manifest, no base ref). Project scope runs only the + single-version lanes and reports every base-vs-head lane as skipped-with-reason on + `VerdictEnvelope.lanes` (`PROJECT_SCOPE_SKIPS` in `orchestrate.ts`). - Agent: `reviewer` (read-only) — `../prompts/reviewer.txt`. - Action: `github/review/action.yml`. diff --git a/packages/opencode/src/altimate/review/config.ts b/packages/opencode/src/altimate/review/config.ts index 793121c510..8527f0c712 100644 --- a/packages/opencode/src/altimate/review/config.ts +++ b/packages/opencode/src/altimate/review/config.ts @@ -74,6 +74,20 @@ export const ReviewConfig = z.object({ // 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({}), + // altimate_change start — schema_lag lane: which job builds what + /** + * Job map for the `schema_lag` lane's cross-job check: a file + * (`{"jobs": {name: {select, exclude, schedule}}}`) or `selectors` (one job + * per `selectors.yml` selector). Resolved with `dbt ls`. Empty = the project + * counts as one job, and cross-job lag is reported as skipped. + */ + schemaLag: z + .object({ + jobs: z.string().default(""), + jobsFrom: z.enum(["", "selectors"]).default(""), + }) + .optional(), + // altimate_change end }) export type ReviewConfig = z.infer diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts index 1ed240b083..2837c4d097 100644 --- a/packages/opencode/src/altimate/review/dbt-patterns.ts +++ b/packages/opencode/src/altimate/review/dbt-patterns.ts @@ -45,6 +45,32 @@ export function splitDiff(diff: string | undefined): DiffLines { return { added, removed } } +/** + * Blank the CONTENTS of multi-line comments — both SQL `/* … *\/` and Jinja + * `{# … #}` — across the whole text, preserving every newline so line + * numbering survives. + * + * `stripComments` is applied per LINE by the detectors, which means a block + * comment spanning several lines is invisible to it — only the first and last + * lines carry a delimiter. Measured on a 215-model project, that let prose + * inside a comment fire real findings: `int_query_time_bins.sql` was reported + * for a CROSS JOIN by a comment explaining that the CROSS JOIN had been + * REMOVED. Applied once where the diff lines are assembled, so every detector + * and the declarative catalog get it. + * + * A hunk containing only part of a block comment (no opening or no closing + * delimiter) is inherently undecidable and is left alone. + */ +export function blankBlockComments(s: string): string { + const blank = (m: string) => m.replace(/[^\n]/g, " ") + // Jinja comments too (`{#- … -#}` whitespace-control variants included): dbt + // models routinely carry long design notes in them, and prose about SQL reads + // as SQL. Measured: a `{# … #}` block citing `docs/…-2026-06-02.md §5.1/§6` + // was reported as an unguarded DIVISION, and one quoting `365 / + // self.lookback_days` from the legacy implementation likewise. + return s.replace(/\/\*[\s\S]*?\*\//g, blank).replace(/\{#[\s\S]*?#\}/g, blank) +} + /** Strip line/block comments so detectors don't fire on commented-out code. */ function stripComments(s: string): string { return s @@ -107,6 +133,11 @@ interface Ctx { removed: string[] model: string inMartOrReporting: boolean + /** True when `added` is the WHOLE model rather than a real hunk (project + * scope — see orchestrate.ts). Detectors whose CONDITION is "this appeared + * in the diff" degenerate here and must opt out; detectors that test a + * whole-file property are unaffected. */ + wholeFile: boolean } type Detector = (c: Ctx) => Finding | null @@ -161,27 +192,189 @@ const detectSelectStar: Detector = (c) => { // 5. LEFT/RIGHT JOIN silently collapsed to INNER by a WHERE/AND on the outer table. -// 6. Cross join / cartesian product added. +/** Table FUNCTIONS that follow `CROSS JOIN` in the lateral-expansion idiom. + * `CROSS JOIN UNNEST(arr)` is not a join between two relations at all — it + * expands an array into rows for the SAME row, which is why Trino/Presto, + * BigQuery and Athena all spell array expansion this way. Treating it as a + * cartesian product is simply wrong, not merely noisy. */ +const CROSS_JOIN_TABLE_FN = /^(unnest|lateral|table|json_table|generate_series|sequence|explode|flatten|openjson)\b/i + +/** + * Extract the CTE bodies of a model as `name → body` (body WITHOUT the outer + * parens). Paren-balanced and literal-blanked, so a `(` inside a string or a + * nested subquery can't end the body early. Best-effort: a model whose CTE list + * doesn't parse simply yields fewer entries, which costs a suppression, never a + * false suppression. + */ +function cteBodies(sql: string): Map { + const out = new Map() + const s = stripLiterals(stripComments(sql)) + const re = /(?:^|[\s,(])([A-Za-z_][A-Za-z0-9_]*)\s+as\s*(?:materialized\s+|not\s+materialized\s+)?\(/gi + for (let m = re.exec(s); m; m = re.exec(s)) { + const open = m.index + m[0].length - 1 + let depth = 0 + let end = -1 + for (let i = open; i < s.length; i++) { + if (s[i] === "(") depth++ + else if (s[i] === ")") { + depth-- + if (depth === 0) { + end = i + break + } + } + } + if (end === -1) continue + // Only keep the FIRST binding of a name: a later `x as (` inside a window + // or cast context must not overwrite a real CTE. + if (!out.has(m[1].toLowerCase())) out.set(m[1].toLowerCase(), s.slice(open + 1, end)) + re.lastIndex = open + 1 + } + return out +} + +/** True when a token sits at paren depth 0 of `body` (so a `group by` inside a + * nested subquery doesn't count as the body's own). */ +function hasAtTopLevel(body: string, re: RegExp): boolean { + let depth = 0 + for (let i = 0; i < body.length; i++) { + const ch = body[i] + if (ch === "(") depth++ + else if (ch === ")") depth = Math.max(0, depth - 1) + else if (depth === 0) { + re.lastIndex = 0 + if (re.test(body.slice(i, i + 40))) return true + } + } + return false +} + +/** + * PROVABLY at most one row: an aggregate select with no `GROUP BY`, or an + * explicit `LIMIT 1`. Both are guarantees of the SQL standard, not heuristics — + * `select max(x) from t` returns exactly one row for any `t`. + * + * A CTE with a `GROUP BY` is NOT accepted: it may return many rows and we + * cannot see the join predicate (a cross join has none), so flagging it is the + * honest outcome. + */ +function isSingleRowCte(body: string): boolean { + if (hasAtTopLevel(body, /^\s*limit\s+1\b/i)) return true + if (hasAtTopLevel(body, /^\s*group\s+by\b/i)) return false + // An aggregate anywhere in the (top-level) select list with no GROUP BY + // collapses the relation to a single row. + return /\b(count|sum|min|max|avg|any_value|array_agg|approx_distinct)\s*\(/i.test(body) +} + +/** + * A parenthesised operand built only from constants — `( select 0 union all + * select 1 … )`, the hand-rolled spine. No `FROM` and no `ref()`/`source()` + * means there is no relation to multiply against, so the row count is a fixed + * literal and the cross join is a deliberate, bounded expansion. + */ +function isConstantSpine(operand: string): boolean { + if (!operand.startsWith("(")) return false + const body = stripLiterals(stripComments(operand)) + return !/\bfrom\b/i.test(body) && !/\{\{\s*(ref|source)\s*\(/i.test(body) +} + +/** Read the operand that follows a `CROSS JOIN`, paren-balanced when it opens + * with `(`, else the next identifier (a CTE / relation / `{{ ref() }}` name). */ +function crossJoinOperand(sql: string, afterIndex: number): string { + const rest = sql.slice(afterIndex).replace(/^\s+/, "") + if (rest.startsWith("(")) { + let depth = 0 + for (let i = 0; i < rest.length; i++) { + if (rest[i] === "(") depth++ + else if (rest[i] === ")") { + depth-- + if (depth === 0) return rest.slice(0, i + 1) + } + } + return rest + } + return rest.slice(0, 200) +} + +/** + * 6. Cross join / cartesian product added. + * + * Scoped to the shapes that can actually be a cartesian product. Measured on a + * real 215-model Trino project, the naive `\bcross\s+join\b` match was ~2/3 + * false positives at `critical` — and `join_risk` is a blocking category, so + * every one of them was a merge-blocking verdict on correct, idiomatic SQL that + * the project's own guidelines prescribe. Three shapes are suppressed, each + * because the row count is PROVABLE from the text rather than merely likely: + * + * 1. `CROSS JOIN UNNEST(...)` (and the other table functions) — lateral array + * expansion, not a join between relations. + * 2. `CROSS JOIN ` where the CTE is an aggregate with no GROUP BY, or + * carries `LIMIT 1` — a scalar broadcast of exactly one row. + * 3. `CROSS JOIN ( select … )` — a bounded literal spine. + * + * Anything else still fires: a bare relation, a `{{ ref() }}`, or a CTE whose + * cardinality cannot be established (including a GROUP BY one) is exactly the + * case a reviewer needs to see. + */ const detectCrossJoin: Detector = (c) => { - const hit = c.added.find((l) => /\bcross\s+join\b/i.test(stripComments(l))) - if (!hit) return null - return makeFinding({ - severity: "critical", - category: "join_risk", - title: `${c.model}: CROSS JOIN creates a cartesian product`, - body: - "A CROSS JOIN multiplies every left row by every right row — an M×N explosion that inflates row counts, " + - "every downstream aggregate, and warehouse cost. Confirm a join key is intended and add an `ON` clause.", - file: c.file.path, - model: c.model, - confidence: "high", - evidence: { tool: "dbt-patterns", result: { rule: "cross_join", line: hit.trim() } }, - ruleKey: "join_risk:cross-join", - }) + // Join the added lines so an operand split across lines (`cross join` then + // `unnest(` on the next) is still classified — the per-line scan this + // replaced could not see it, and that is the single most common formatting + // of the UNNEST idiom. + const added = c.added.map((l) => stripComments(l)) + const joined = added.join("\n") + const ctes = cteBodies(c.newSql) + const re = /\bcross\s+join\b/gi + for (let m = re.exec(joined); m; m = re.exec(joined)) { + const operand = crossJoinOperand(joined, m.index + m[0].length).replace(/^\s+/, "") + if (CROSS_JOIN_TABLE_FN.test(operand)) continue + if (isConstantSpine(operand)) continue + const ident = /^([A-Za-z_][A-Za-z0-9_]*)/.exec(operand)?.[1]?.toLowerCase() + if (ident) { + const body = ctes.get(ident) + if (body && isSingleRowCte(body)) continue + } + // The line the match STARTED on, for the finding's evidence. + const lineNo = joined.slice(0, m.index).split("\n").length - 1 + const hit = added[lineNo] ?? operand + return makeFinding({ + // `warning` / `medium`, NOT `critical` / `high` — severity tracks how the + // finding was ESTABLISHED, not only what it would mean if true. A genuine + // cartesian product is critical, and core's AST `CartesianProduct` rule + // grades it that way because it has a parse tree to prove it with. This + // detector only ever runs in DEGRADED mode (CORE_AST_COVERED suppresses it + // whenever core lint works), and in that mode it cannot see the operand's + // cardinality at all. A heuristic that cannot prove its claim must not + // hard-block a merge — `join_risk` is in the default `blockOn`, so a + // `critical` here is a blocking verdict. The signal survives: three + // warnings still trip `warningPatternThreshold`. + severity: "warning", + category: "join_risk", + title: `${c.model}: CROSS JOIN may create a cartesian product`, + body: + "A CROSS JOIN multiplies every left row by every right row — an M×N explosion that inflates row counts, " + + "every downstream aggregate, and warehouse cost. Confirm a join key is intended and add an `ON` clause.\n\n" + + "_Not flagged: `CROSS JOIN UNNEST(...)`, a cross join to a single-row aggregate CTE, or one to a " + + "constant spine — those are bounded by construction. What remains is typically a cross join to a " + + "`{{ ref() }}`ed relation (e.g. a date spine), whose row count is not knowable from this file — which " + + "is why this is a warning to verify rather than an assertion that it is wrong._", + file: c.file.path, + model: c.model, + confidence: "medium", + evidence: { tool: "dbt-patterns", result: { rule: "cross_join", line: hit.trim() } }, + ruleKey: "join_risk:cross-join", + }) + } + return null } // 7. A new JOIN added into a model that aggregates → likely fan-out inflating SUM/COUNT. const detectFanout: Detector = (c) => { + // Change-scoped by CONDITION, not just by wording: the signal is "a join was + // ADDED to an aggregating model". With the whole file presented as added it + // degenerates to "this model aggregates and has a join", which describes most + // correct analytics SQL — measured at 65 of 215 models on a real project. + if (c.wholeFile) return null const sql = stripComments(c.newSql) const aggregates = /\b(sum|count|avg|min|max)\s*\(/i.test(sql) && /\bgroup\s+by\b/i.test(sql) if (!aggregates) return null @@ -617,8 +810,47 @@ const detectSelfJoin: Detector = (c) => { } // 39. Window function without PARTITION BY (whole-table window — often unintended). +/** + * Reads the balanced `OVER ( … )` rather than the single line the `OVER` sits + * on. A window spec is routinely formatted across three lines — + * + * row_number() over ( + * partition by account, full_table_name + * order by ingestion_timestamp desc + * ) as rn + * + * — so the per-line test saw `over (` with no `partition by` beside it and + * fired. Measured on a 215-model project: 37 findings, and ALL 37 windows were + * partitioned. This is the third instance of the same defect class in this file + * (the CROSS JOIN operand scan and the multi-line comment blanking were the + * first two): a single-line regex applied to SQL whose meaning spans lines. A + * fourth is likely — prefer a balanced/joined scan whenever a construct can be + * broken across lines. + */ const detectWindowNoPartition: Detector = (c) => { - const hit = c.added.find((l) => /\bover\s*\(/i.test(stripComments(l)) && !/partition\s+by/i.test(l)) + const text = c.added.map((l) => stripComments(l)).join("\n") + let hit: string | undefined + for (const m of text.matchAll(/\bover\s*\(/gi)) { + // Walk to the matching close paren so nested calls inside the spec + // (`order by coalesce(x, 0)`) don't truncate it. + let depth = 0 + let end = text.length + for (let i = m.index + m[0].length - 1; i < text.length; i++) { + if (text[i] === "(") depth++ + else if (text[i] === ")") { + depth-- + if (depth === 0) { + end = i + break + } + } + } + const spec = text.slice(m.index, end + 1) + if (/partition\s+by/i.test(spec)) continue + // Report the line the OVER started on. + hit = c.added[text.slice(0, m.index).split("\n").length - 1] ?? spec + break + } if (!hit) return null return pattern( c, @@ -765,10 +997,18 @@ const MODEL_DETECTORS: Detector[] = [ ] /** Run the dbt anti-pattern detectors over a changed MODEL file. */ -export function detectModelPatterns(file: ChangedFile, newSql: string | undefined, rubric: Rubric): Finding[] { +export function detectModelPatterns( + file: ChangedFile, + newSql: string | undefined, + rubric: Rubric, + opts?: { wholeFile?: boolean; dialect?: string }, +): Finding[] { const kind = classifyDbtFile(file.path) if (!isModelSql(kind) || file.status === "deleted" || !newSql) return [] - const { added, removed } = splitDiff(file.diff) + // Blank multi-line block comments BEFORE the per-line detectors see the + // hunk — see blankBlockComments. Splitting on "\n" is safe because the + // blanker preserves newlines exactly. + const { added, removed } = splitDiff(blankBlockComments(file.diff ?? "")) const ctx: Ctx = { file, kind, @@ -777,6 +1017,7 @@ export function detectModelPatterns(file: ChangedFile, newSql: string | undefine removed, model: modelNameFromPath(file.path), inMartOrReporting: /(^|\/)(marts|reporting)\//.test(file.path), + wholeFile: opts?.wholeFile === true, } const out: Finding[] = [] for (const d of MODEL_DETECTORS) { @@ -784,7 +1025,7 @@ export function detectModelPatterns(file: ChangedFile, newSql: string | undefine if (f) out.push({ ...f, severity: clampSeverity(f.category, f.severity, f.confidence) }) } // Declarative rule catalog (data-driven checks) complements the programmatic detectors. - out.push(...evaluateCatalog(file, newSql, added, removed, rubric)) + out.push(...evaluateCatalog(file, newSql, added, removed, rubric, opts?.dialect)) return out.filter((f) => !exclusionReason(f, rubric)) } diff --git a/packages/opencode/src/altimate/review/format.ts b/packages/opencode/src/altimate/review/format.ts index 0befe5883c..ed6fa0be26 100644 --- a/packages/opencode/src/altimate/review/format.ts +++ b/packages/opencode/src/altimate/review/format.ts @@ -33,7 +33,10 @@ export function verdictHeadline(env: VerdictEnvelope): string { const tierLabel = env.tierForced ? `${env.tier} tier — forced (was ${env.tierClassified ?? "unknown"})` : `${env.tier} tier` - return `${VERDICT_LABEL[env.verdict]} — ${counts} (${tierLabel})` + // Project scope reviews the whole manifest rather than a diff; say so in the + // headline so a reader never mistakes it for a PR verdict. + const scopeLabel = env.scope === "project" ? "whole project" : tierLabel + return `${VERDICT_LABEL[env.verdict]} — ${counts} (${scopeLabel})` } /** Full PR/MR summary comment body (markdown), prefixed with the dedup marker. */ @@ -79,8 +82,40 @@ export function renderSummary(env: VerdictEnvelope): string { lines.push(`> 🧭 **Tier: ${env.tier}** — ${shown}${overflow}`, "") } + // Lane report — only present in project scope. It goes ABOVE the findings: + // "no findings" means nothing until the reader knows which lanes produced it. + if (env.lanes) { + lines.push( + `> 🛠️ **Project review** — every model in the manifest, no base ref.`, + `> Lanes run: ${env.lanes.ran.length ? env.lanes.ran.map((l) => `\`${l}\``).join(", ") : "_none_"}`, + "", + ) + // The AST lanes are only as good as their input. `dbt compile` is the + // customer's job, and when it hasn't run the engine silently returns zero + // findings per unparseable model — which reads as "clean". Say so. + const { compiledModels: compiled, totalModels: total } = env.lanes + if (total > 0 && compiled < total) { + lines.push( + `> ⚠️ **${total - compiled} of ${total} models had no compiled SQL** — the engine analysed their raw Jinja,`, + `> which the SQL parser cannot read, so the AST lanes (core lint, PII classification, grain,`, + `> filter consistency) returned nothing for them. Run \`dbt compile\` and re-run for a complete review.`, + "", + ) + } + if (env.lanes.skipped.length) { + lines.push(`
Lanes skipped (${env.lanes.skipped.length}) — why`, "") + for (const s of env.lanes.skipped) lines.push(`- \`${s.lane}\` — ${s.reason}`) + lines.push("", "
", "") + } + } + if (!env.findings.length) { - lines.push("No issues found in the changed dbt models. 🎉", "") + lines.push( + env.scope === "project" + ? "No issues found in any model in the manifest — within the lanes listed above. 🎉" + : "No issues found in the changed dbt models. 🎉", + "", + ) } else { const grouped = groupBySeverity(env.findings) for (const sev of ["critical", "warning", "suggestion"] as const) { diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts index d0d89143a4..f318a27952 100644 --- a/packages/opencode/src/altimate/review/orchestrate.ts +++ b/packages/opencode/src/altimate/review/orchestrate.ts @@ -12,7 +12,7 @@ import { type ChangedFile, filterChangedFiles } from "./diff-filter" 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" +import { type ReviewMode, type ReviewScope, type VerdictEnvelope, buildEnvelope, signEnvelope } from "./verdict" import { detectModelPatterns, detectSchemaYmlPatterns, splitDiff } from "./dbt-patterns" import { type AiReviewInput } from "./ai-review" @@ -163,8 +163,83 @@ export interface ReviewRunner { /** Warehouse connection name to diff against; omit for the default. */ warehouse?: string, ): Promise<{ rowsOnlyInBase?: number; rowsOnlyInHead?: number; rowsChanged?: number; summary?: string } | null> + // altimate_change start — dbt impact past the dbt graph (analyzeDbtImpact) + /** + * Per-hop blast radius of one change — dbt models, loaders and served + * tables, each with a verdict. Returns null when the core build lacks the + * binding (the lanes then keep their dbt-graph-only behaviour). + */ + dbtImpact?(change: { model: string; column?: string; kind: string }): Promise + /** + * Schema-lag findings for a base → head SQL change: readers that production + * can run against a relation that does not have the new column yet. Null + * when the core build lacks the binding. + */ + schemaLag?( + change: { model: string; kind: "sql"; base_sql: string; head_sql: string }, + manifests?: { manifest: string; base_manifest: string }, + ): Promise + // altimate_change end +} + +// altimate_change start — schema_lag lane +export interface SchemaLagResult { + findings: Array<{ + reason: string + verdict: string + consumer: string + column: string + producer: string + consumer_jobs?: string[] + producer_jobs?: string[] + window?: string + message: string + fix: string + }> + skipped: string[] +} + +/** + * Compiled SQL of both sides for the schema_lag lane. One `target/` holds at + * most one side, so run.ts compiles base and head (or takes + * `--base-manifest` / `--head-manifest`); the head manifest is the DAG and node + * config (`materialized`, `on_schema_change`) the check reads. + */ +export interface LagSides { + sql(model: string, side: "old" | "new"): string | undefined + manifest: string + baseManifest: string +} +// altimate_change end + +// altimate_change start — impact evidence past the dbt graph +export interface ExternalImpactEntry { + hop: number + node_type: string + name: string + verdict: string + reason?: string } +/** BREAKING consumers outside dbt (a loader or a served table): the only + * evidence that lets a MODIFIED model's lineage finding reach `critical`. */ +interface ExternalBreakEvidence { + breaks: Array +} + +function externalBreaks(entries: ExternalImpactEntry[], column?: string): ExternalBreakEvidence["breaks"] { + return entries + .filter((e) => e.verdict.toLowerCase() === "breaking" && !/^(model|dbt_)/i.test(e.node_type)) + .map((e) => ({ ...e, column })) +} + +function describeBreaks(b: ExternalBreakEvidence["breaks"]): string { + return b + .map((e) => `${e.node_type} \`${e.name}\`${e.column ? ` (column \`${e.column}\`)` : ""}${e.reason ? ` — ${e.reason}` : ""}`) + .join("; ") +} +// altimate_change end + export interface OrchestrateInput { changedFiles: ChangedFile[] config: ReviewConfig @@ -198,6 +273,66 @@ export interface OrchestrateInput { forceTier?: "trivial" | "lite" | "full" /** Manifest was stale relative to change-affecting files (see run.ts::detectStaleManifest). */ staleManifest?: boolean + // altimate_change start — schema_lag lane: compiled base/head, produced on demand + /** Undefined result: no compiled sides, and the lane uses the engine SQL. */ + lagSides?: () => Promise + // altimate_change end + /** `diff` (default) reviews base..head; `project` reviews every model in the + * manifest with no base version. See {@link ReviewScope}. */ + scope?: ReviewScope +} + +/** Which lanes ran and which could not, with the reason. Emitted on the + * envelope in `project` scope; `diff` scope leaves it undefined (every lane + * the tier enables runs there, so the list would be noise). */ +export interface LaneReport { + ran: string[] + skipped: Array<{ lane: string; reason: string }> + /** How many of `totalModels` the engine lanes saw as dbt-COMPILED SQL. + * The SQL parser cannot parse Jinja: on a model that falls back to raw + * `{{ config() }}` source the parse fails at line 1, every AST rule inside + * the engine is skipped, and the lane returns ZERO findings — which reads + * exactly like a clean model. Over a whole project that silence is the + * difference between a review and a rubber stamp, so the counts are + * reported and `renderSummary` warns when they diverge. */ + compiledModels: number + totalModels: number +} + +/** + * Lanes that compare a model's BASE version to its HEAD version. In project + * scope there is exactly one version of each model, so none of them has an + * input — they are reported as skipped, never silently dropped. + */ +const PROJECT_SCOPE_SKIPS: Record = { + lineage_breakage: + "reports deleted/renamed models and the blast radius of a change — both are defined relative to a base ref", + column_breakage: "diffs a model's output columns between its base and head versions", + semantic_change: "proves base-vs-head row equivalence; in project scope there is no base version", + structural_diff: "compares the base and head parse trees (the `*_change` rules)", + dbt_config_diff: "compares the base and head `{{ config() }}` blocks (materialization / incremental changes)", + data_diff: "runs the base and head SQL against the warehouse and diffs the resulting rows", + topology_equivalence: "pairs models deleted in a diff against models added in the same diff (merge refactors)", + schema_yml_test_removal: "detects `unique` / `not_null` / `relationships` tests REMOVED between two schema.yml versions", + ai_review: + "the advisory LLM lane is grounded in the diff and caps at 20 files — project-wide it would review an arbitrary subset at unbounded cost; run `altimate review` on a diff for it", + schema_lag: "compares a model's base and head output / source columns to find readers that run before the producer is rebuilt", + "dbt_patterns:fanout_join": + "fires when a join is ADDED to an aggregating model; with the whole model presented as added it degenerates to \"this model aggregates and has a join\", which describes most correct analytics SQL", +} + +/** + * Present a whole model to the diff-anchored detectors as if every line were + * added. The dbt-patterns detectors and the declarative rule catalog are + * relevance-gated on the diff by construction (`ruleMatches` requires the + * signal to appear in added/removed lines), so without this they evaluate to + * nothing in project scope and the report would be a quiet lie. + */ +function wholeFileAsAddedDiff(sql: string): string { + return sql + .split("\n") + .map((l) => "+" + l) + .join("\n") } /** Derive the dbt model name from a model file path. */ @@ -241,7 +376,14 @@ function impactToSeverity(impact: ImpactResult): Severity | null { } } -function lineageBreakageLane(file: ChangedFile & { kind: string }, impact: ImpactResult, rubric: Rubric): Finding[] { +function lineageBreakageLane( + file: ChangedFile & { kind: string }, + impact: ImpactResult, + rubric: Rubric, + // altimate_change start — loader/served-table evidence from analyzeDbtImpact + external?: ExternalBreakEvidence, + // altimate_change end +): Finding[] { const model = modelNameFromPath(file.path) const total = impact.directCount + impact.transitiveCount const degraded = !impact.hasManifest @@ -268,6 +410,9 @@ function lineageBreakageLane(file: ChangedFile & { kind: string }, impact: Impac body: `\`${model}\` is ${file.status} but ${total} downstream model${total !== 1 ? "s" : ""}` + ` (+${impact.testCount} test${impact.testCount !== 1 ? "s" : ""}) still depend on it.` + + // altimate_change start — consumers outside dbt, when analyzeDbtImpact named them + (external?.breaks.length ? ` Outside dbt it also breaks: ${describeBreaks(external.breaks)}.` : "") + + // altimate_change end (degraded ? "\n\n_No manifest available — verify the blast radius locally._" : ""), file: file.path, model, @@ -279,13 +424,34 @@ function lineageBreakageLane(file: ChangedFile & { kind: string }, impact: Impac ] } + // altimate_change start — a modified model whose change BREAKS a loader or a + // served table (analyzeDbtImpact, column-level) is a real break, not fan-out. + if (external?.breaks.length) { + return [ + makeFinding({ + severity: clampSeverity("lineage_breakage", "critical", degraded ? "unknown" : "high"), + category: "lineage_breakage", + title: `${model}: change breaks consumers outside dbt`, + body: + `This change to \`${model}\` breaks ${external.breaks.length} consumer(s) the dbt graph does not show: ` + + `${describeBreaks(external.breaks)}. Keep the column(s) or update those consumers in the same release.`, + file: file.path, + model, + confidence: degraded ? "unknown" : "high", + degraded, + evidence: { tool: "altimate_core.dbt_impact", result: { impact, external: external.breaks } }, + ruleKey: "lineage_breakage:external-sink", + }), + ] + } + // altimate_change end const sev = impactToSeverity(impact) if (!sev) return [] // For a MODIFIED model, blast radius alone is NOT a breaking change — a wide - // fan-out doesn't mean the edit broke anything (it may be additive/safe). Until - // a real column-drop classifier exists, cap modified-model lineage impact at - // `warning` so merely touching a popular model never blocks. Only delete/rename - // (handled above) is treated as a genuine break. + // fan-out doesn't mean the edit broke anything (it may be additive/safe). Cap + // modified-model lineage impact at `warning` so merely touching a popular model + // never blocks. Delete/rename (handled above) and a loader/served-table break + // proven by analyzeDbtImpact (also above) are the genuine breaks. const capped: Severity = sev === "critical" ? "warning" : sev const clamped = clampSeverity("lineage_breakage", capped, degraded ? "unknown" : "high") return [ @@ -463,6 +629,110 @@ function parseLineageSource(src: string): { table: string; col: string } { return { col: (parts.at(-1) ?? "").toLowerCase(), table: (parts.at(-2) ?? "").toLowerCase() } } +// altimate_change start — analyzeDbtImpact evidence, memoized per model file +const columnEvidenceMemo = new WeakMap>() + +/** Loader/served-table breaks caused by dropping `removed` columns of the + * model. Empty when the runner has no dbtImpact (or the core lacks it). */ +function columnImpactEvidence(ctx: ModelContext, runner: ReviewRunner, removed: string[]): Promise { + const cached = columnEvidenceMemo.get(ctx) + if (cached) return cached + const p = (async (): Promise => { + if (!runner.dbtImpact || !removed.length) return { breaks: [] } + const model = modelNameFromPath(ctx.file.path) + const breaks: ExternalBreakEvidence["breaks"] = [] + for (const column of removed) { + const entries = await runner.dbtImpact({ model, column, kind: "drop" }).catch(() => null) + if (entries) breaks.push(...externalBreaks(entries, column)) + } + return { breaks } + })() + columnEvidenceMemo.set(ctx, p) + return p +} + +/** Evidence for the lineage lane: model-level for a delete/rename, removed + * output columns (from column lineage on compiled SQL) for a modification. */ +async function lineageImpactEvidence( + ctx: ModelContext, + runner: ReviewRunner, + dialect: string, +): Promise { + if (!runner.dbtImpact) return { breaks: [] } + const { file } = ctx + if (file.status === "deleted" || file.status === "renamed") { + const model = modelNameFromPath(file.oldPath ?? file.path) + const entries = await runner.dbtImpact({ model, kind: "drop" }).catch(() => null) + return { breaks: entries ? externalBreaks(entries) : [] } + } + if (file.status !== "modified" || !runner.columnLineage || !ctx.engineOldSql || !ctx.engineNewSql) + return { breaks: [] } + const cols = async (sql: string) => + new Set((await runner.columnLineage!(sql, dialect)).map((e) => e.target.toLowerCase()).filter(Boolean)) + const [oldCols, newCols] = await Promise.all([cols(ctx.engineOldSql), cols(ctx.engineNewSql)]) + const removed = [...oldCols].filter((c) => !newCols.has(c)) + return columnImpactEvidence(ctx, runner, removed) +} +// altimate_change end + +// altimate_change start — schema_lag lane +/** + * SCHEMA LAG — a reader of a column the change adds (or a newly declared + * source column) that production can run before the producer is rebuilt: + * another job builds it, it is incremental and never gains the column, or the + * loader has not landed it. CI builds everything at once, so it passes there. + * Triggered when the model's output columns change or a YAML file in the diff + * changes declared columns. + */ +export async function schemaLagLane( + ctx: ModelContext, + runner: ReviewRunner, + dialect: string, + sourceYamlChanged: boolean, + lagSides?: () => Promise, +): Promise { + if (!runner.schemaLag) return [] + const { file, engineOldSql, engineNewSql } = ctx + if (file.status !== "modified" || !engineOldSql || !engineNewSql || engineOldSql.trim() === engineNewSql.trim()) return [] + const model = modelNameFromPath(file.path) + // The engine SQL can mix sides (raw Jinja base against a compiled head from a + // stale target/), so prefer the compiled base and head when they exist. + const sides = lagSides ? await lagSides().catch(() => undefined) : undefined + const oldSql = sides?.sql(model, "old") ?? engineOldSql + const newSql = sides?.sql(model, "new") ?? engineNewSql + if (oldSql.trim() === newSql.trim()) return [] + let columnsChanged = sourceYamlChanged + if (!columnsChanged && runner.columnLineage) { + const cols = async (sql: string) => + [...new Set((await runner.columnLineage!(sql, dialect)).map((e) => e.target.toLowerCase()).filter(Boolean))].sort() + const [o, n] = await Promise.all([cols(oldSql), cols(newSql)]) + columnsChanged = o.join(",") !== n.join(",") + } else if (!runner.columnLineage) columnsChanged = true + if (!columnsChanged) return [] + const r = await runner + .schemaLag( + { model, kind: "sql", base_sql: oldSql, head_sql: newSql }, + sides ? { manifest: sides.manifest, base_manifest: sides.baseManifest } : undefined, + ) + .catch(() => null) + if (!r) return [] + return r.findings.map((f) => + makeFinding({ + severity: clampSeverity("lineage_breakage", f.verdict === "breaking" ? "critical" : "warning", "high"), + category: "lineage_breakage", + title: `${f.reason}: \`${f.consumer}\` reads \`${f.producer}.${f.column}\` before it exists in production`, + body: `${f.message}. Suggested fix: ${f.fix}.`, + file: file.path, + model, + column: f.column, + confidence: "high", + evidence: { tool: "altimate_core.dbt_impact", result: { schema_lag: f, skipped: r.skipped } }, + ruleKey: f.reason, + }), + ) +} +// altimate_change end + /** * COLUMN-LEVEL breakage — the real lineage differentiator a linter can't do. * When a PR removes/renames an OUTPUT column of a model, trace (via the engine's @@ -503,6 +773,14 @@ async function columnBreakageLane( } } } + // altimate_change start — removed columns read by loaders / served tables + const external = await columnImpactEvidence(ctx, runner, removed) + for (const e of external.breaks) { + const key = e.column ?? "?" + if (!brokenBy.has(key)) brokenBy.set(key, new Set()) + brokenBy.get(key)!.add(`${e.node_type}:${e.name}`) + } + // altimate_change end if (!brokenBy.size) return [] const detail = [...brokenBy.entries()] @@ -1057,6 +1335,18 @@ interface ModelContext { export async function runReview(input: OrchestrateInput): Promise { const reviewable = filterChangedFiles(input.changedFiles, input.rubric.exclusions.excludeGlobs) + const scope: ReviewScope = input.scope ?? "diff" + // `project` scope (`altimate review --all`): one version of every model, no + // base ref. Every base-vs-head lane below is gated on this and reported in + // `laneReport.skipped` with the reason it could not run. + const projectScope = scope === "project" + const laneReport: LaneReport = { ran: [], skipped: [], compiledModels: 0, totalModels: 0 } + const laneRan = (lane: string) => { + if (!laneReport.ran.includes(lane)) laneReport.ran.push(lane) + } + const laneSkipped = (lane: string, reason: string) => { + if (!laneReport.skipped.some((s) => s.lane === lane)) laneReport.skipped.push({ lane, reason }) + } const dialect = input.config.dialect const getContent = input.getContent const getCompiled = input.getCompiled @@ -1089,7 +1379,18 @@ export async function runReview(input: OrchestrateInput): Promise { - const c = ctxByPath.get(p) - return c ? c.impact.directCount + c.impact.transitiveCount : 0 - }, - touchesPiiOf: (f) => (ctxByPath.get(f.path)?.pii.length ?? 0) > 0, - isComplexOf: (f) => ctxByPath.get(f.path)?.complex ?? false, - pathTokenCategoryOf, - }) + // Tiering is a COST control for diffs: it reads diff signals (changed-line + // count, a contract/source block touched in the hunk) to decide which + // expensive lanes a small PR can skip. None of those signals exist in + // project scope, where the classifier would land on `trivial` and silently + // disable most of the review. Project scope is not a tiering situation — it + // runs every lane that can evaluate a single model version. + const tierResult = projectScope + ? { tier: "full" as const, reasons: ["project scope (--all): tiering is a diff cost-control and does not apply"] } + : classifyPR(reviewable, { + blastRadiusOf: (p) => { + const c = ctxByPath.get(p) + return c ? c.impact.directCount + c.impact.transitiveCount : 0 + }, + touchesPiiOf: (f) => (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) @@ -1151,6 +1460,42 @@ export async function runReview(input: OrchestrateInput): Promise /\.ya?ml$/i.test(f.path) && /(^|\n)[+-]\s*-?\s*(\{\s*)?name:/.test(f.diff ?? "")) + const notEnabled = + `not enabled at \`${tier}\` tier` + (input.config.reviewers.length ? " / by the `reviewers` config" : "") + if (projectScope) { + // Single-version lanes: what a project review actually consists of. + const note = (lane: string, enabled: boolean) => (enabled ? laneRan(lane) : laneSkipped(lane, notEnabled)) + note("core_lint", runCoreLint) + note("dbt_patterns", runDbtPatterns) + note("lexical_scan", runDbtPatterns) + note("dbt_config_lint", runDbtConfig) + note("grain_mismatch", runSemantic) + note("missing_grain_test", runTestCoverage) + note("sibling_filter_consistency", runSemantic) + laneRan("pii_classify") + note("pii_detect", runPiiExposure) + // Base-vs-head lanes: reported, never dropped. + for (const [lane, reason] of Object.entries(PROJECT_SCOPE_SKIPS)) laneSkipped(lane, reason) + } + const all: Finding[][] = [] // Files where the diff-scoped PII classifier completed (looked at the change), // those where it actually emitted a finding, and — per file — the lowercased @@ -1162,28 +1507,42 @@ export async function runReview(input: OrchestrateInput): Promise[] = [] // Engine lanes consume COMPILED SQL (rendered by dbt) when available. - if (lanes.has("sql_quality") || lanes.has("warehouse_cost")) - tasks.push(qualityLane(ctx.file, input.runner, ctx.engineNewSql, dialect, ctx.engineOldSql)) - if (lanes.has("semantic_change")) { - tasks.push(dataDiffLane(ctx, input.runner, dialect, input.config.dataDiff)) + if (runCoreLint) tasks.push(qualityLane(ctx.file, input.runner, ctx.engineNewSql, dialect, ctx.engineOldSql)) + if (runSemantic) { + // grainMismatchLane reads one version of the model; the other two are + // base-vs-head and no-op in project scope (see PROJECT_SCOPE_SKIPS). tasks.push(grainMismatchLane(ctx, input.runner)) + if (!projectScope) { + tasks.push(dataDiffLane(ctx, input.runner, dialect, input.config.dataDiff)) + tasks.push( + semanticChangeLane( + ctx.file, + input.runner, + ctx.engineOldSql, + ctx.engineNewSql, + dialect, + ctx.impact, + input.rubric, + ), + ) + } + } + if (runLineage && !projectScope) { + // altimate_change start — the lineage lane waits on analyzeDbtImpact evidence + const lineageCtx = ctx tasks.push( - semanticChangeLane( - ctx.file, - input.runner, - ctx.engineOldSql, - ctx.engineNewSql, - dialect, - ctx.impact, - input.rubric, + lineageImpactEvidence(lineageCtx, input.runner, dialect).then((external) => + lineageBreakageLane(lineageCtx.file, lineageCtx.impact, input.rubric, external), ), ) - } - if (lanes.has("lineage_breakage")) { - all.push(lineageBreakageLane(ctx.file, ctx.impact, input.rubric)) + // altimate_change end tasks.push(columnBreakageLane(ctx, input.runner, getCompiled, dialect)) } - if (lanes.has("test_coverage")) tasks.push(missingGrainTestLane(ctx, input.runner)) + if (runSchemaLag) { + laneRan("schema_lag") + tasks.push(schemaLagLane(ctx, input.runner, dialect, sourceYamlChanged, input.lagSides)) + } + if (runTestCoverage) tasks.push(missingGrainTestLane(ctx, input.runner)) // PII classification always runs (cheap name-pattern check, diff-scoped to // newly-introduced columns) — exposing PII is a hard-floor concern that // shouldn't depend on the cost tier enabling the pii_exposure lane. If core @@ -1200,7 +1559,7 @@ export async function runReview(input: OrchestrateInput): Promise() - if (lanes.has("sql_quality") || lanes.has("warehouse_cost")) { + if (runCoreLint) { await Promise.all( [...ctxByPath.values()].map(async (ctx) => { if (!ctx.engineNewSql || ctx.file.status === "deleted") return @@ -1391,7 +1748,7 @@ export async function runReview(input: OrchestrateInput): Promise ({ path: ctx.file.path, status: ctx.file.status, @@ -1439,9 +1796,20 @@ export async function runReview(input: OrchestrateInput): Promise = { trivial: ["sql_quality", "dbt_patterns"], - lite: ["sql_quality", "lineage_breakage", "semantic_change", "test_coverage", "dbt_patterns", "ai_review"], + lite: ["sql_quality", "lineage_breakage", "schema_lag", "semantic_change", "test_coverage", "dbt_patterns", "ai_review"], full: [ "sql_quality", "lineage_breakage", + "schema_lag", "semantic_change", "contract_violation", "pii_exposure", diff --git a/packages/opencode/src/altimate/review/rule-catalog.ts b/packages/opencode/src/altimate/review/rule-catalog.ts index eaf4389c4a..c428cfe5e1 100644 --- a/packages/opencode/src/altimate/review/rule-catalog.ts +++ b/packages/opencode/src/altimate/review/rule-catalog.ts @@ -42,6 +42,17 @@ export interface Rule { absentNewSql?: RegExp /** drop a candidate added-line match if it ALSO matches this (false-positive guard). */ unless?: RegExp + /** Opt out of string-literal blanking. Set ONLY when the rule's signal lives + * INSIDE a quoted literal (a leading `%` wildcard, `interval '1 day'`, an + * environment name). Everything else is matched against literal-blanked text + * so SQL-looking prose inside a string can't fire a rule. */ + literalsMatter?: boolean + /** Warehouses this rule applies to, as core dialect names (`bigquery`, + * `redshift`, `databricks`, …). OMITTED means "applies everywhere", so an + * existing rule that says nothing keeps its current behaviour. When the + * project's dialect is unknown, every rule runs — a missing dialect must + * never silently suppress findings. */ + dialects?: string[] kind?: RuleKind pathGlob?: RegExp example: RuleExample @@ -59,44 +70,86 @@ function stripComments(s: string): string { .replace(/\{#[\s\S]*?#\}/g, " ") } +/** + * Blank single-quoted string-literal CONTENTS (keeping the quotes) so a rule + * cannot be tripped by SQL-looking text inside a literal. + * + * `dbt-patterns.ts` has carried this for its programmatic detectors since + * early on; the declarative catalog never called it. Measured consequence: the + * ONLY `critical` finding in a whole-project review of a 215-model repo was + * `create-or-replace` matching inside `|| 'CREATE OR REPLACE TRANSIENT TABLE + * instead as it will '` — a model emitting ADVICE TEXT about the statement. + * A reviewer whose single blocking finding is a string literal is worse than + * one with no findings at all. + * + * Handles doubled-quote escapes (`'it''s'`). Rules that legitimately inspect + * literal CONTENT (a leading `%` wildcard, an interval string, an environment + * name) opt out with `literalsMatter: true` — and the catalog's own + * example/counter corpus is what proves they did: a rule that needs literals + * and forgets the flag fails its positive example. + */ +function stripLiterals(s: string): string { + return s.replace(/'(?:[^']|'')*'/g, "''") +} + export interface RuleInput { file: ChangedFile newSql: string added: string[] removed: string[] + /** Core dialect name from the dbt manifest's `adapter_type` (see run.ts). + * Undefined when it could not be determined — every rule then applies. */ + dialect?: string } /** True when the rule matches the change. */ export function ruleMatches(rule: Rule, input: RuleInput): boolean { + // Dialect gate. A `bq-*` rule advising `SELECT * EXCEPT` is not merely noisy + // on a Trino project — it tells the reader the finding is about a warehouse + // they do not run. Measured: 223 of 1,268 findings (17.5%) came from rules + // named for another engine. `run.ts` has always detected `adapter_type`; it + // simply was not plumbed this far. An UNKNOWN dialect runs every rule, so a + // detection failure can never silently suppress findings. + if (rule.dialects?.length && input.dialect && !rule.dialects.includes(input.dialect)) return false const kind = classifyDbtFile(input.file.path) const wantKind = rule.kind ?? "model_sql" if (wantKind !== "any" && kind !== wantKind) return false if (rule.pathGlob && !rule.pathGlob.test(input.file.path)) return false + // Blank string-literal contents unless the rule's signal lives inside one. + // POSITIVE matchers (`added`/`removed`/`newSql`) read the blanked text so SQL + // inside a string can't fire them. The GUARDS (`unless`, `absentNewSql`) read + // the ORIGINAL text: their job is to recognise context, and blanking can erase + // the very context they look for. Real case — `case-when-no-end` guards on + // `{{`, and the line `interval '{{ days }}' day` carries its Jinja INSIDE a + // literal, so a blanked guard stopped seeing it and the rule fired on valid + // templated SQL. + const clean = rule.literalsMatter ? stripComments : (t: string) => stripLiterals(stripComments(t)) + const guard = stripComments + // Relevance gate: a rule only fires when its signal is in THE DIFF, never on // an unrelated edit to a matching file. added/removed are diff-anchored by // construction; a `newSql` (whole-file) condition must also appear in the // changed lines to count as relevant. `absentNewSql` is a whole-file guard. - const diffText = stripComments([...input.added, ...input.removed].join("\n")) + const diffText = clean([...input.added, ...input.removed].join("\n")) let relevant = false if (rule.added) { const hit = input.added.some((l) => { - const s = stripComments(l) - return rule.added!.test(s) && !(rule.unless && rule.unless.test(s)) + return rule.added!.test(clean(l)) && !(rule.unless && rule.unless.test(guard(l))) }) if (!hit) return false relevant = true } if (rule.removed) { - if (!input.removed.some((l) => rule.removed!.test(stripComments(l)))) return false + if (!input.removed.some((l) => rule.removed!.test(clean(l)))) return false relevant = true } if (rule.newSql) { - if (!rule.newSql.test(stripComments(input.newSql))) return false + if (!rule.newSql.test(clean(input.newSql))) return false if (rule.newSql.test(diffText)) relevant = true } if (rule.absentNewSql) { - if (rule.absentNewSql.test(stripComments(input.newSql))) return false + if (rule.absentNewSql.test(guard(input.newSql))) return false } return relevant } @@ -108,8 +161,9 @@ export function evaluateCatalog( added: string[], removed: string[], rubric: Rubric, + dialect?: string, ): Finding[] { - const input: RuleInput = { file, newSql, added, removed } + const input: RuleInput = { file, newSql, added, removed, dialect } const model = modelName(file.path) const matched: { rule: Rule; finding: Finding }[] = [] for (const rule of CATALOG) { @@ -167,7 +221,10 @@ const r = ( severity: Severity, title: string, body: string, - match: Pick, + match: Pick< + Rule, + "added" | "removed" | "newSql" | "absentNewSql" | "unless" | "kind" | "pathGlob" | "literalsMatter" | "dialects" + >, example: RuleExample, counter?: RuleExample, ): Rule => ({ id, category, severity, title, body, ...match, example, counter }) @@ -376,7 +433,7 @@ const DATETIME: Rule[] = [ "suggestion", "comparing a date column to a string literal", "Relying on implicit string→date casts is fragile; cast explicitly to date/timestamp.", - { added: /\b\w*(date|_at|_ts)\b\s*(=|>|<|>=|<=)\s*'\d{4}\/\d/i }, + { literalsMatter: true, added: /\b\w*(date|_at|_ts)\b\s*(=|>|<|>=|<=)\s*'\d{4}\/\d/i }, { add: "where order_date >= '2024/01/01'" }, { add: "where order_date >= date '2024-01-01'" }, ), @@ -804,7 +861,7 @@ const MATERIALIZATION: Rule[] = [ "suggestion", "ephemeral model with heavy joins is inlined everywhere", "An `ephemeral` model is inlined into every consumer; with joins that's repeated recompute. Prefer view/table.", - { newSql: /materialized\s*[=:]\s*['"]?ephemeral[\s\S]*\bjoin\b/i }, + { literalsMatter: true, newSql: /materialized\s*[=:]\s*['"]?ephemeral[\s\S]*\bjoin\b/i }, { add: "{{ config(materialized='ephemeral') }} select * from a join b on a.k=b.k" }, { add: "{{ config(materialized='view') }} select * from a join b on a.k=b.k" }, ), @@ -814,7 +871,7 @@ const MATERIALIZATION: Rule[] = [ "suggestion", "staging model materialized as table", "Staging models are usually views/ephemeral; a `table` here adds storage + a full rebuild each run.", - { newSql: /materialized\s*[=:]\s*['"]?table/i, pathGlob: /(^|\/)stg_|(^|\/)staging\//i }, + { literalsMatter: true, newSql: /materialized\s*[=:]\s*['"]?table/i, pathGlob: /(^|\/)stg_|(^|\/)staging\//i }, { add: "{{ config(materialized='table') }} select 1", newSql: "{{ config(materialized='table') }} select 1", @@ -832,7 +889,7 @@ const MATERIALIZATION: Rule[] = [ "warning", "incremental on_schema_change='ignore'", "With `on_schema_change='ignore'`, added/reordered columns silently misalign on append. Use 'sync_all_columns' or 'append_new_columns'.", - { added: /on_schema_change\s*[=:]\s*['"]ignore/i }, + { literalsMatter: true, added: /on_schema_change\s*[=:]\s*['"]ignore/i }, { add: "on_schema_change='ignore'" }, { add: "on_schema_change='sync_all_columns'" }, ), @@ -842,7 +899,7 @@ const MATERIALIZATION: Rule[] = [ "warning", "delete+insert incremental without unique_key", "`incremental_strategy='delete+insert'` with no `unique_key` can't delete matched rows — duplicates accrue.", - { newSql: /delete\s*\+\s*insert/i, absentNewSql: /unique_key/i }, + { literalsMatter: true, newSql: /delete\s*\+\s*insert/i, absentNewSql: /unique_key/i }, { add: "incremental_strategy='delete+insert' select 1", newSql: "incremental_strategy='delete+insert' select 1" }, { add: "incremental_strategy='delete+insert' unique_key='id'", @@ -855,7 +912,7 @@ const MATERIALIZATION: Rule[] = [ "warning", "merge incremental without unique_key", "`incremental_strategy='merge'` needs a `unique_key` to match rows; without it it appends and duplicates.", - { newSql: /incremental_strategy\s*[=:]\s*['"]merge/i, absentNewSql: /unique_key/i }, + { literalsMatter: true, newSql: /incremental_strategy\s*[=:]\s*['"]merge/i, absentNewSql: /unique_key/i }, { add: "incremental_strategy='merge' select 1", newSql: "incremental_strategy='merge' select 1" }, { add: "incremental_strategy='merge' unique_key='id'", newSql: "incremental_strategy='merge' unique_key='id'" }, ), @@ -875,7 +932,7 @@ const MATERIALIZATION: Rule[] = [ "warning", "destructive pre/post-hook", "A `pre_hook`/`post_hook` running DELETE/TRUNCATE/DROP runs every build and can destroy data.", - { added: /(pre_hook|post_hook)[\s\S]*\b(delete|truncate|drop)\b/i }, + { literalsMatter: true, added: /(pre_hook|post_hook)[\s\S]*\b(delete|truncate|drop)\b/i }, { add: "post_hook='truncate table x'" }, { add: "post_hook='grant select on x to role'" }, ), @@ -885,7 +942,7 @@ const MATERIALIZATION: Rule[] = [ "suggestion", "large table materialization without clustering/partitioning", "A `table`/incremental over a big source with no partition_by/cluster_by scans fully on read. Add partitioning.", - { + { literalsMatter: true, newSql: /materialized\s*[=:]\s*['"]?(table|incremental)/i, absentNewSql: /(partition_by|cluster_by|partitioned\s+by|clustered\s+by)/i, pathGlob: /(^|\/)(marts|fct_|fact_)/i, @@ -958,7 +1015,7 @@ const DBT_CONV: Rule[] = [ "suggestion", "env_var() inside a model", "`{{ env_var(...) }}` in model SQL couples logic to the environment and can leak secrets into compiled code.", - { added: /\{\{\s*env_var\s*\(/i }, + { literalsMatter: true, added: /\{\{\s*env_var\s*\(/i }, { add: "where token = '{{ env_var('API_KEY') }}'" }, { add: "where d > {{ var('cutoff', 0) }}" }, ), @@ -988,7 +1045,7 @@ const DBT_CONV: Rule[] = [ "warning", "ref() pointing at a raw/source table name", "Using `ref()` on a source-style name (raw_/src_) suggests a missing `source()`; lineage will be wrong.", - { added: /\{\{\s*ref\(\s*['"](raw_|src_|source_)/i }, + { literalsMatter: true, added: /\{\{\s*ref\(\s*['"](raw_|src_|source_)/i }, { add: "from {{ ref('raw_orders') }}" }, { add: "from {{ source('app','orders') }}" }, ), @@ -1138,7 +1195,7 @@ const SNAPSHOTS: Rule[] = [ "warning", "check-strategy snapshot without check_cols", "A `strategy='check'` snapshot needs `check_cols`; without it changes aren't detected.", - { kind: "snapshot", newSql: /strategy\s*[=:]\s*['"]check/i, absentNewSql: /check_cols/i }, + { literalsMatter: true, kind: "snapshot", newSql: /strategy\s*[=:]\s*['"]check/i, absentNewSql: /check_cols/i }, { add: "{{ config(strategy='check') }}", newSql: "{{ config(strategy='check') }}" }, { add: "{{ config(strategy='check', check_cols=['status']) }}", @@ -1151,7 +1208,7 @@ const SNAPSHOTS: Rule[] = [ "warning", "timestamp-strategy snapshot without updated_at", "A `strategy='timestamp'` snapshot requires `updated_at`; missing it breaks SCD tracking.", - { kind: "snapshot", newSql: /strategy\s*[=:]\s*['"]timestamp/i, absentNewSql: /updated_at/i }, + { literalsMatter: true, kind: "snapshot", newSql: /strategy\s*[=:]\s*['"]timestamp/i, absentNewSql: /updated_at/i }, { add: "{{ config(strategy='timestamp') }}", newSql: "{{ config(strategy='timestamp') }}" }, { add: "{{ config(strategy='timestamp', updated_at='modified_at') }}", @@ -1183,7 +1240,7 @@ const SECURITY_PII: Rule[] = [ "critical", "hardcoded credential/secret in SQL", "A literal password/secret/api_key/token in model SQL leaks into compiled artifacts and git. Use env/secret manager.", - { added: /\b(password|secret|api_key|apikey|access_token|private_key)\s*[=:]\s*['"][^'"]{4,}/i }, + { literalsMatter: true, added: /\b(password|secret|api_key|apikey|access_token|private_key)\s*[=:]\s*['"][^'"]{4,}/i }, { add: "where api_key = 'sk-12345abc'" }, { add: "where d > {{ var('cutoff', 0) }}" }, ), @@ -1233,7 +1290,7 @@ const SECURITY_PII: Rule[] = [ "suggestion", "surrogate key built from raw PII", "Hashing PII into a join key still derives from PII and can correlate individuals; prefer a tokenized id.", - { added: /generate_surrogate_key\s*\(\s*\[[^\]]*\b(email|ssn|phone)\b/i }, + { literalsMatter: true, added: /generate_surrogate_key\s*\(\s*\[[^\]]*\b(email|ssn|phone)\b/i }, { add: "{{ dbt_utils.generate_surrogate_key(['email']) }}" }, { add: "{{ dbt_utils.generate_surrogate_key(['order_id']) }}" }, ), @@ -1246,7 +1303,7 @@ const BIGQUERY: Rule[] = [ "warning", "BigQuery: query on a partitioned table with no partition filter", "Without a `_PARTITIONTIME`/partition-column predicate, BigQuery scans the whole table (full cost).", - { + { dialects: ["bigquery"], added: /from\s+[^;\n]*(events|logs|fct_|fact_|_fct|_fact)/i, absentNewSql: /(_partitiontime|_partitiondate|\bwhere\b[\s\S]*(date|_at|created|partition))/i, }, @@ -1262,7 +1319,7 @@ const BIGQUERY: Rule[] = [ "warning", "BigQuery: wildcard table without _TABLE_SUFFIX filter", "Wildcard tables (`events_*`) scan every shard unless filtered on `_TABLE_SUFFIX`.", - { added: /from\s+[`"][^`"]*\*[`"]/i, absentNewSql: /_table_suffix/i }, + { dialects: ["bigquery"], added: /from\s+[`"][^`"]*\*[`"]/i, absentNewSql: /_table_suffix/i }, { add: "from `proj.ds.events_*`", newSql: "from `proj.ds.events_*`" }, { add: "from `proj.ds.events_*` where _table_suffix between '2024' and '2025'", @@ -1275,20 +1332,24 @@ const BIGQUERY: Rule[] = [ "warning", "BigQuery: legacy-SQL syntax", "Legacy SQL (`[proj:ds.table]`, `TABLE_DATE_RANGE`) is deprecated; use Standard SQL.", - { added: /(\[[\w-]+:[\w.]+\]|table_date_range|table_query)/i }, + { dialects: ["bigquery"], added: /(\[[\w-]+:[\w.]+\]|table_date_range|table_query)/i }, { add: "from [my-proj:ds.table]" }, { add: "from `my-proj`.`ds`.`table`" }, ), - r( - "bq-cross-join-unnest", - "fanout", - "suggestion", - "BigQuery: CROSS JOIN UNNEST can explode rows", - "`CROSS JOIN UNNEST(array)` multiplies rows by array length; confirm the fan-out is intended.", - { added: /cross\s+join\s+unnest\s*\(/i }, - { add: "cross join unnest(items) as item" }, - { add: "left join unnest(items) as item" }, - ), + // bq-cross-join-unnest removed. Three reasons, in order of weight: + // 1. `CROSS JOIN UNNEST(arr)` is the ONLY way to expand an array in Trino, + // Presto, Athena and BigQuery — so the rule fired on every correct use of + // the idiom. Measured on a real Trino project: 10 findings, 10 of them the + // plain idiom, 0 genuine unintended fan-out. + // 2. It was labelled "BigQuery:" while firing on a Trino repo, telling the + // reader the finding was about a dialect they do not use. The Rule shape + // carries no dialect, so it could not be scoped rather than retired. + // 3. The concern it gestures at — an array expansion the author did not + // intend — is now handled where it can be reasoned about: `detectCrossJoin` + // in dbt-patterns.ts classifies the CROSS JOIN operand and stays silent on + // UNNEST precisely because a table function is not a relation join. + // (Same disposition as bq-safe-cast-missing above: a blanket regex whose + // positive case is the common correct one erodes trust faster than it helps.) // bq-safe-cast-missing removed: it flagged EVERY numeric/date CAST as "consider // SAFE_CAST", which is a false-positive on the overwhelmingly common case of a // clean cast. The genuine dirty-data concern is too context-dependent for a @@ -1299,7 +1360,7 @@ const BIGQUERY: Rule[] = [ "suggestion", "BigQuery: FLOAT64 for money loses precision", "Use `NUMERIC`/`BIGNUMERIC` for currency; `FLOAT64` introduces rounding error.", - { added: /cast\s*\(\s*\w*(amount|price|revenue|cost|total)\w*\s+as\s+float64/i }, + { dialects: ["bigquery"], added: /cast\s*\(\s*\w*(amount|price|revenue|cost|total)\w*\s+as\s+float64/i }, { add: "cast(amount as float64)" }, { add: "cast(amount as numeric)" }, ), @@ -1309,7 +1370,7 @@ const BIGQUERY: Rule[] = [ "suggestion", "BigQuery: prefer SELECT * EXCEPT/REPLACE over bare *", "If you must use `*`, BigQuery's `* EXCEPT(...)`/`* REPLACE(...)` is safer than a bare star.", - { added: /select\s+\*\s+from/i, unless: /except\s*\(|replace\s*\(|\{\{/i }, + { dialects: ["bigquery"], added: /select\s+\*\s+from/i, unless: /except\s*\(|replace\s*\(|\{\{/i }, { add: "select * from t" }, { add: "select * except(x) from t" }, ), @@ -1319,7 +1380,7 @@ const BIGQUERY: Rule[] = [ "suggestion", "BigQuery: GENERATE_ARRAY/DATE_ARRAY can be huge", "A wide `GENERATE_ARRAY`/`GENERATE_DATE_ARRAY` materializes every element; bound the range.", - { added: /generate_(date_|timestamp_)?array\s*\(/i }, + { dialects: ["bigquery"], added: /generate_(date_|timestamp_)?array\s*\(/i }, { add: "generate_date_array('2000-01-01', current_date)" }, { add: "from {{ ref('date_spine') }}" }, ), @@ -1329,7 +1390,7 @@ const BIGQUERY: Rule[] = [ "suggestion", "BigQuery: DATETIME vs TIMESTAMP confusion", "`DATETIME` has no timezone; mixing it with `TIMESTAMP` in comparisons shifts results.", - { added: /cast\s*\(\s*\w*\s+as\s+datetime\b/i }, + { dialects: ["bigquery"], added: /cast\s*\(\s*\w*\s+as\s+datetime\b/i }, { add: "cast(ts as datetime)" }, { add: "cast(ts as timestamp)" }, ), @@ -1339,7 +1400,7 @@ const BIGQUERY: Rule[] = [ "suggestion", "BigQuery: array access needs OFFSET/ORDINAL", "Indexing an array as `arr[0]` errors in BigQuery; use `arr[OFFSET(0)]` or `[ORDINAL(1)]`.", - { added: /\b\w+\[\s*\d+\s*\]/ }, + { dialects: ["bigquery"], added: /\b\w+\[\s*\d+\s*\]/ }, { add: "select items[0]" }, { add: "select items[offset(0)]" }, ), @@ -1349,7 +1410,7 @@ const BIGQUERY: Rule[] = [ "suggestion", "BigQuery: large exact COUNT(DISTINCT) — APPROX is cheaper", "On BigQuery, `APPROX_COUNT_DISTINCT` is dramatically cheaper than exact distinct at scale.", - { added: /count\s*\(\s*distinct/i, pathGlob: /(marts|reporting)/i }, + { dialects: ["bigquery"], added: /count\s*\(\s*distinct/i, pathGlob: /(marts|reporting)/i }, { add: "count(distinct user_id)", path: "models/marts/m.sql" }, { add: "count(user_id)", path: "models/marts/m.sql" }, ), @@ -1359,7 +1420,7 @@ const BIGQUERY: Rule[] = [ "suggestion", "BigQuery: case-sensitive string join", "BigQuery string comparison is case-sensitive by default; normalize with LOWER or set collation.", - { added: /\bon\s+\w+\.\w*(name|code|sku)\b\s*=\s*\w+\./i }, + { dialects: ["bigquery"], added: /\bon\s+\w+\.\w*(name|code|sku)\b\s*=\s*\w+\./i }, { add: "on a.sku = b.sku" }, { add: "on lower(a.sku) = lower(b.sku)" }, ), @@ -1372,7 +1433,7 @@ const SNOWFLAKE: Rule[] = [ "suggestion", "Snowflake: VARIANT path used without explicit cast", "Reading `col:field` from a VARIANT returns VARIANT; cast (`::string`) so downstream types are stable.", - { added: /\w+:[a-z_]\w*(?!\s*::)/i, unless: /::|http|case\s+when/i }, + { dialects: ["snowflake"], added: /\w+:[a-z_]\w*(?!\s*::)/i, unless: /::|http|case\s+when/i }, { add: "select payload:user_id from t" }, { add: "select payload:user_id::int from t" }, ), @@ -1382,7 +1443,7 @@ const SNOWFLAKE: Rule[] = [ "suggestion", "Snowflake: LATERAL FLATTEN multiplies rows", "`LATERAL FLATTEN(input => arr)` produces one row per element; verify the fan-out is intended.", - { added: /lateral\s+flatten\s*\(/i }, + { dialects: ["snowflake"], added: /lateral\s+flatten\s*\(/i }, { add: "from t, lateral flatten(input => t.items)" }, { add: "from t" }, ), @@ -1392,7 +1453,7 @@ const SNOWFLAKE: Rule[] = [ "warning", "Snowflake: time-travel (AT/BEFORE) in a model", "`AT(TIMESTAMP …)`/`BEFORE(STATEMENT …)` reads historical data — non-reproducible in a model.", - { added: /\b(at|before)\s*\(\s*(timestamp|offset|statement)\s*=>/i }, + { dialects: ["snowflake"], added: /\b(at|before)\s*\(\s*(timestamp|offset|statement)\s*=>/i }, { add: "from t at(offset => -60)" }, { add: "from t" }, ), @@ -1402,7 +1463,7 @@ const SNOWFLAKE: Rule[] = [ "warning", "Snowflake: RESULT_SCAN in a model", "`RESULT_SCAN(LAST_QUERY_ID())` depends on session state and can't be reproduced by dbt.", - { added: /result_scan\s*\(/i }, + { dialects: ["snowflake"], added: /result_scan\s*\(/i }, { add: "from table(result_scan(last_query_id()))" }, { add: "from {{ ref('x') }}" }, ), @@ -1417,7 +1478,7 @@ const SNOWFLAKE: Rule[] = [ "suggestion", "Snowflake: transient table has no fail-safe", "A `transient` materialization saves storage but loses Fail-safe recovery; confirm that's acceptable.", - { added: /transient\s*[=:]\s*true|materialized\s*[=:]\s*['"]?transient/i }, + { dialects: ["snowflake"], added: /transient\s*[=:]\s*true|materialized\s*[=:]\s*['"]?transient/i }, { add: "{{ config(transient=true) }}" }, { add: "{{ config(materialized='table') }}" }, ), @@ -1427,7 +1488,7 @@ const SNOWFLAKE: Rule[] = [ "critical", "Snowflake: COPY INTO / PUT inside a model", "`COPY INTO`/`PUT`/`GET` are loading commands, not transforms — they don't belong in a model.", - { added: /\b(copy\s+into|put\s+file|get\s+@)\b/i }, + { dialects: ["snowflake"], added: /\b(copy\s+into|put\s+file|get\s+@)\b/i }, { add: "copy into t from @stage" }, { add: "select * from t" }, ), @@ -1437,7 +1498,7 @@ const SNOWFLAKE: Rule[] = [ "suggestion", "Snowflake: ILIKE is case-insensitive scan", "`ILIKE` can't use search-optimization the way an anchored equality can; prefer LOWER(col)=… where possible.", - { added: /\bilike\b/i }, + { dialects: ["snowflake"], added: /\bilike\b/i }, { add: "where name ilike '%shirt%'" }, { add: "where lower(name) = 'shirt'" }, ), @@ -1450,7 +1511,7 @@ const SNOWFLAKE: Rule[] = [ "warning", "Snowflake: SAMPLE/TABLESAMPLE returns random subset", "`SAMPLE (n)` returns a different random subset each run — non-deterministic and lossy in a model.", - { added: /\bsample\s*\(\s*\d/i }, + { dialects: ["snowflake"], added: /\bsample\s*\(\s*\d/i }, { add: "from t sample (10)" }, { add: "from t" }, ), @@ -1463,7 +1524,7 @@ const REDSHIFT: Rule[] = [ "suggestion", "Redshift: large table without dist/sort keys", "A Redshift `table` over a big source with no `dist`/`sort` config redistributes on every join — slow.", - { + { dialects: ["redshift"], literalsMatter: true, newSql: /materialized\s*[=:]\s*['"]?table/i, absentNewSql: /(dist\s*[=:]|sort\s*[=:]|diststyle|sortkey)/i, pathGlob: /(marts|fct_|fact_)/i, @@ -1485,7 +1546,7 @@ const REDSHIFT: Rule[] = [ "suggestion", "Redshift: VARCHAR without length defaults to 256", "Redshift `VARCHAR` defaults to 256 chars and truncates silently; size it explicitly for long text.", - { added: /cast\s*\(\s*\w+\s+as\s+varchar\s*\)/i }, + { dialects: ["redshift"], added: /cast\s*\(\s*\w+\s+as\s+varchar\s*\)/i }, { add: "cast(notes as varchar)" }, { add: "cast(notes as varchar(4096))" }, ), @@ -1495,7 +1556,7 @@ const REDSHIFT: Rule[] = [ "suggestion", "Redshift: LISTAGG overflows past 64KB", "Redshift `LISTAGG` errors when the result exceeds 64KB; bound or pre-truncate the inputs.", - { added: /\blistagg\s*\(/i }, + { dialects: ["redshift"], added: /\blistagg\s*\(/i }, { add: "listagg(comment, ',')" }, { add: "array_agg(comment)" }, ), @@ -1505,7 +1566,7 @@ const REDSHIFT: Rule[] = [ "warning", "Redshift: leader-node-only function on a table", "`GENERATE_SERIES`/some functions run only on the leader node and error when used with table data.", - { added: /\bgenerate_series\s*\(/i }, + { dialects: ["redshift"], added: /\bgenerate_series\s*\(/i }, { add: "select generate_series(1, n) from t" }, { add: "from {{ ref('numbers') }}" }, ), @@ -1515,7 +1576,7 @@ const REDSHIFT: Rule[] = [ "suggestion", "Redshift: GETDATE()/SYSDATE in transform", "`GETDATE()`/`SYSDATE` bake run time into output (non-idempotent); pass an as-of var.", - { added: /\b(getdate|sysdate)\b/i }, + { dialects: ["redshift"], added: /\b(getdate|sysdate)\b/i }, { add: "where d > getdate() - 7" }, { add: "where d > '2024-01-01'" }, ), @@ -1525,7 +1586,7 @@ const REDSHIFT: Rule[] = [ "suggestion", "Redshift: SUPER path without cast", "Reading from a SUPER column without casting yields SUPER; cast for stable downstream types.", - { added: /\w+\.\w+\s*::\s*super\b|\bjson_extract_path_text\s*\(/i }, + { dialects: ["redshift"], added: /\w+\.\w+\s*::\s*super\b|\bjson_extract_path_text\s*\(/i }, { add: "select json_extract_path_text(data, 'id')" }, { add: "select id from t" }, ), @@ -1538,7 +1599,7 @@ const DATABRICKS: Rule[] = [ "suggestion", "Spark/Databricks: explode() multiplies rows", "`explode(array)`/`posexplode` emits one row per element — a fan-out; verify downstream grain.", - { added: /\b(explode|posexplode|explode_outer)\s*\(/i }, + { dialects: ["databricks"], added: /\b(explode|posexplode|explode_outer)\s*\(/i }, { add: "select explode(items)" }, { add: "select items" }, ), @@ -1548,7 +1609,7 @@ const DATABRICKS: Rule[] = [ "suggestion", "Spark: collect_list/collect_set ordering is non-deterministic", "`collect_list`/`collect_set` produce arrays in arbitrary order; sort explicitly if order matters.", - { added: /\bcollect_(list|set)\s*\(/i, unless: /sort_array/i }, + { dialects: ["databricks"], added: /\bcollect_(list|set)\s*\(/i, unless: /sort_array/i }, { add: "collect_list(id)" }, { add: "sort_array(collect_list(id))" }, ), @@ -1558,7 +1619,7 @@ const DATABRICKS: Rule[] = [ "warning", "Databricks: MERGE INTO without a selective condition", "A `MERGE` whose `ON` is too broad (or `1=1`) rewrites the whole Delta table each run.", - { added: /merge\s+into[\s\S]*\bon\s+(1\s*=\s*1|true)\b/i }, + { dialects: ["databricks"], added: /merge\s+into[\s\S]*\bon\s+(1\s*=\s*1|true)\b/i }, { add: "merge into t using s on 1=1" }, { add: "merge into t using s on t.id=s.id" }, ), @@ -1568,7 +1629,7 @@ const DATABRICKS: Rule[] = [ "suggestion", "Databricks: large Delta table without partitioning/Z-ORDER", "A big Delta `table` with no `partition_by`/Z-ORDER scans all files; add partitioning for prune-ability.", - { + { dialects: ["databricks"], literalsMatter: true, newSql: /materialized\s*[=:]\s*['"]?(table|incremental)/i, absentNewSql: /(partition_by|zorder|z_order|liquid)/i, pathGlob: /(marts|fct_|fact_)/i, @@ -1590,7 +1651,7 @@ const DATABRICKS: Rule[] = [ "suggestion", "Spark: CACHE/UNCACHE TABLE in a model", "`CACHE TABLE`/`REFRESH` are session/runtime ops, not transforms — remove from model SQL.", - { added: /\b(cache\s+table|uncache\s+table|refresh\s+table)\b/i }, + { dialects: ["databricks"], added: /\b(cache\s+table|uncache\s+table|refresh\s+table)\b/i }, { add: "cache table t" }, { add: "select * from t" }, ), @@ -1600,7 +1661,7 @@ const DATABRICKS: Rule[] = [ "suggestion", "Spark: to_date/to_timestamp without format", "`to_date(str)` without an explicit format guesses and silently NULLs unparseable values.", - { added: /\bto_(date|timestamp)\s*\(\s*[a-z_][\w.]*\s*\)/i }, + { dialects: ["databricks"], added: /\bto_(date|timestamp)\s*\(\s*[a-z_][\w.]*\s*\)/i }, { add: "to_date(order_date)" }, { add: "to_date(order_date, 'yyyy-MM-dd')" }, ), @@ -1673,7 +1734,10 @@ const STYLE: Rule[] = [ "warning", "hardcoded environment/schema name", "A literal `'prod'`/`'production'`/`analytics.` in SQL breaks across environments; use target/var.", - { added: /['"](prod|production|staging|dev)['"]/i }, + // `tags=['staging', 'billing']` is a dbt LAYER label, not an environment. + // Measured on a 215-model project: 46 of 46 findings were the model's own + // config tag, and they were 33% of every warning in the review. + { literalsMatter: true, added: /['"](prod|production|staging|dev)['"]/i, unless: /\btags\s*[=:]/i }, { add: "where env = 'production'" }, { add: "where env = '{{ target.name }}'" }, ), @@ -1683,7 +1747,7 @@ const STYLE: Rule[] = [ "suggestion", "LIKE without a wildcard — use =", "`like 'exact'` with no `%`/`_` is just equality; `=` is clearer and faster.", - { added: /\blike\s+'[^%_']+'/i }, + { literalsMatter: true, added: /\blike\s+'[^%_']+'/i }, { add: "where code like 'ABC'" }, { add: "where code like 'ABC%'" }, ), @@ -1733,7 +1797,7 @@ const STYLE: Rule[] = [ "suggestion", "string interval literal portability", "`interval '1 day'` literal syntax differs across warehouses; prefer dateadd/date_add functions.", - { added: /interval\s+'\d+\s*\w+'/i }, + { literalsMatter: true, added: /interval\s+'\d+\s*\w+'/i }, { add: "d + interval '1 day'" }, { add: "dateadd(day, 1, d)" }, ), @@ -1846,7 +1910,7 @@ const MISC: Rule[] = [ "suggestion", "comparing boolean to true/'true'", "`is_active = true` is redundant; use the boolean directly. `= 'true'` (string) is a type bug.", - { added: /\b\w+\s*=\s*('true'|'false')/i }, + { literalsMatter: true, added: /\b\w+\s*=\s*('true'|'false')/i }, { add: "where active = 'true'" }, { add: "where active" }, ), @@ -2166,7 +2230,7 @@ const MISC: Rule[] = [ "suggestion", "ILIKE with surrounding wildcards", "`ilike '%x%'` is a case-insensitive full scan; consider a search index or normalized column.", - { added: /\bilike\s+'%[^']*%'/i }, + { literalsMatter: true, added: /\bilike\s+'%[^']*%'/i }, { add: "where name ilike '%shirt%'" }, { add: "where name = 'shirt'" }, ), @@ -2219,9 +2283,14 @@ const MISC2: Rule[] = [ "suggestion", "comparing to empty string '' ", "`= ''` treats empty and NULL differently across warehouses; decide and handle both explicitly.", - { added: /=\s*''/ }, + // literalsMatter: the signal is that the literal is EMPTY. Without the + // opt-out, `stripLiterals` rewrites every `= 'some_value'` to `= ''` and + // this rule matches all of them — it went 20 -> 214 on a real project the + // moment literal blanking was switched on. The counter-example below pins + // that: a non-empty literal must NOT fire. + { literalsMatter: true, added: /=\s*''/ }, { add: "where name = ''" }, - { add: "where name is null" }, + { add: "where name = 'active'" }, ), r( "decimal-no-scale", diff --git a/packages/opencode/src/altimate/review/run.ts b/packages/opencode/src/altimate/review/run.ts index 9e8db4cb7b..37dc69e5f0 100644 --- a/packages/opencode/src/altimate/review/run.ts +++ b/packages/opencode/src/altimate/review/run.ts @@ -1,13 +1,19 @@ import path from "node:path" -import { readFile, realpath, stat, access } from "node:fs/promises" +import { readFile, realpath, stat, access, mkdtemp, rm } from "node:fs/promises" +import { tmpdir } from "node:os" +import { existsSync } from "node:fs" +import { jobsFromSelectors, readJobMap, resolveJobMap } from "../../cli/cmd/dbt-impact-jobs" +import { compileSides } from "../../cli/cmd/dbt-impact-compiled" +import { findModel, loadManifest } from "../../cli/cmd/dbt-mart-helpers" import { Installation } from "../../installation" import { loadReviewConfig, resolveRubric } from "./config" import type { Severity } from "./finding" import { collectChangedFiles, makeContentResolver, defaultBaseRef, gitRepoRoot, manifestHash } from "./git" import { makeCompiledResolver, dbtProjectName } from "./compiled" +import { parseManifest } from "../native/dbt/manifest" import { buildCatalogSchemaContext } from "./schema-context" import { createDispatcherRunner } from "./runner" -import { runReview } from "./orchestrate" +import { runReview, type LagSides } from "./orchestrate" import { runAiReview } from "./ai-review" import type { ReviewMode, VerdictEnvelope } from "./verdict" import type { ChangedFile } from "./diff-filter" @@ -50,6 +56,77 @@ export interface ReviewPullRequestOptions { /** G2 — [EXPERIMENTAL] bypass the tier classifier with the supplied tier. * Envelope carries `tierForced: true` when used. Debug / bench only. */ forceTier?: "trivial" | "lite" | "full" + /** Project scope: review EVERY model in the manifest instead of a diff. + * Mutually exclusive with `base` / `head`. See {@link ReviewScope}. */ + all?: boolean + /** `--all` only: cap the number of models reviewed. Omitted → + * {@link DEFAULT_ALL_MODEL_CAP}; `0` → no cap. */ + maxModels?: number + // altimate_change start — schema_lag lane inputs + /** Job map file (overrides config `schemaLag.jobs`). */ + jobs?: string + /** `selectors`: one job per `selectors.yml` selector (overrides config `schemaLag.jobsFrom`). */ + jobsFrom?: "selectors" + /** Compiled base / head manifests; when omitted the lane compiles both sides itself. */ + baseManifest?: string + headManifest?: string + /** dbt executable, profiles dir and target for the lane's `dbt ls` / `dbt compile`. */ + dbt?: { dbtBin?: string; profilesDir?: string; target?: string } + // altimate_change end +} + +/** Default `--all` model cap. A 214-model project runs the whole lane set in + * ~7s, so the cap is not there to trim ordinary projects — it is there so a + * pathological monorepo doesn't look hung. It never truncates silently (see + * the stderr warning below). Lift it with `--max-models 0`. */ +export const DEFAULT_ALL_MODEL_CAP = 500 + +/** + * Build the `--all` file list: every model in the dbt manifest, as a single + * version with no base side. + * + * Paths come out of the manifest dbt-project relative (`models/marts/x.sql`) + * but everything downstream — the working-tree resolver, the compiled-SQL + * resolver's prefix stripping, the finding `file` field — speaks repo-relative + * paths, so `pathPrefix` is prepended here (it is `""` when the dbt project IS + * the repo root). + * + * Models are sorted by path so two runs over the same manifest produce the same + * order, and the cap (when it bites) always truncates the same tail rather than + * whatever the manifest's object key order happened to be. + */ +async function manifestModelFiles( + manifestAbs: string, + pathPrefix: string, + maxModels?: number, +): Promise { + const parsed = await parseManifest({ path: manifestAbs }) + const prefix = pathPrefix && pathPrefix !== "." ? pathPrefix.replace(/\\/g, "/").replace(/\/+$/, "") : "" + const paths = parsed.models + .map((m) => m.path) + .filter((p): p is string => !!p) + .map((p) => (prefix ? `${prefix}/${p}` : p)) + .sort() + if (!paths.length) { + throw new Error( + `--all: no models found in ${manifestAbs}. Run \`dbt parse\` (or \`dbt compile\`) in the project first, ` + + `or point --manifest at the right artifact.`, + ) + } + const cap = maxModels === undefined ? DEFAULT_ALL_MODEL_CAP : maxModels + const capped = cap > 0 && paths.length > cap ? paths.slice(0, cap) : paths + if (capped.length < paths.length) { + // Loud, never silent: a truncated project review that reads as a clean one + // is exactly the failure this mode exists to fix. + process.stderr.write( + `⚠️ --all: reviewing ${capped.length} of ${paths.length} models (cap: ${cap}). ` + + `Pass --max-models 0 to review every model.\n`, + ) + } + // `status: "added"` is the honest shape: the review has no previous version + // of any of these files. It is also what makes the single-version lanes + // (missing grain test, PII on introduced columns) evaluate the whole model. + return capped.map((p) => ({ path: p, status: "added" as const })) } /** dbt adapter_type → core SQL dialect. Mostly identity; a few aliases. */ @@ -207,32 +284,26 @@ async function detectStaleManifest(manifestAbs: string, changed: ChangedFile[], } export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise { + if (opts.all && (opts.base || opts.head)) { + throw new Error("--all reviews the whole manifest and has no base/head; drop --base/--head or drop --all.") + } const config = await loadReviewConfig(opts.cwd) if (opts.manifestPath) config.manifestPath = opts.manifestPath if (opts.mode) config.mode = opts.mode if (opts.severityThreshold) config.severityThreshold = opts.severityThreshold + // altimate_change start — schema_lag lane: CLI job map overrides config + if (opts.jobs || opts.jobsFrom) + config.schemaLag = { jobs: opts.jobs ? path.resolve(opts.cwd, opts.jobs) : "", jobsFrom: opts.jobsFrom ?? "" } + if (!!opts.baseManifest !== !!opts.headManifest) throw new Error("--base-manifest and --head-manifest go together") + // altimate_change end const rubric = resolveRubric(config) - // Only resolve a base ref if we actually need git (to collect changed files - // or to read old/new content). A caller that supplies BOTH `changedFiles` and - // `getContent` (e.g. a non-git CI integration) must not be forced through a - // git lookup that can fail when there's no usable history. - const needGit = !opts.changedFiles || !opts.getContent - const base = opts.base ?? (needGit ? await defaultBaseRef(opts.cwd) : "") - const changedFiles = opts.changedFiles ?? (await collectChangedFiles({ base, head: opts.head, cwd: opts.cwd })) // Resolve the repo top-level once; used to root working-tree FS reads, the // stale-manifest existence check, and the compiled-SQL resolver's path // mapping. Falls back to opts.cwd when we couldn't resolve it (non-git or // bare-repo contexts) — safe for the repo-root-invoked case but not helpful // in the subdir-invocation case that this addresses. const gitRoot = (await gitRepoRoot(opts.cwd)) ?? opts.cwd - // Map renamed files → their old path so getContent resolves the "old" side - // from where the file lived at `base`. - const renames = new Map( - changedFiles.filter((f) => f.status === "renamed" && f.oldPath).map((f) => [f.path, f.oldPath as string]), - ) - const getContent = - opts.getContent ?? makeContentResolver({ base, head: opts.head, cwd: opts.cwd, renames, gitRoot }) // Resolve the manifest against the PROJECT being reviewed (cwd), not the // binary's process.cwd() — otherwise a relative path silently misses when the @@ -270,32 +341,11 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise } } } - // Freshness check: detect (and warn — non-fatal) when the manifest predates - // change-affecting files. Runs for both the pinned-head CI shape AND the - // working-tree local shape — the local scenario "dbt compile once, edit for - // an hour, then altimate review" is where staleness actually bites in - // practice, so gating this behind `--head` (the prior behavior) silently - // under-warned the most common developer workflow. mtime granularity on - // the working tree can be noisy during a live edit session, but the check - // is limited to files that would materially change the manifest - // (`isManifestAffecting`), so noise is bounded. Return value is stamped - // into the signed envelope so downstream auditors see it too. - const staleManifest = await detectStaleManifest(manifestAbs, changedFiles, gitRoot) - - // Resolve the SQL dialect: explicit config wins; otherwise auto-detect from - // the dbt manifest's `adapter_type` (so a BigQuery/Redshift project isn't - // analyzed as the snowflake default — wrong-dialect portability suppression). - if (!config.dialect) config.dialect = (await detectDialect(manifestAbs)) ?? "snowflake" - - // Prefer dbt's catalog.json (real warehouse columns from `dbt docs generate`) - // for the schema context — complete columns are what make column-lineage - // breakage and proven equivalence actually fire (the manifest only has - // documented columns). Falls back to manifest-derived schema when absent. - const catalogAbs = path.join(path.dirname(manifestAbs), "catalog.json") - const catalogSchema = await buildCatalogSchemaContext(catalogAbs) - const runner = createDispatcherRunner({ manifestPath: manifestAbs, schemaContext: catalogSchema }) - const mhash = await manifestHash(manifestAbs, opts.cwd) + // Resolved here, ahead of the file list, because `--all` needs `pathPrefix` + // to build it: the manifest names models dbt-project relative and they have + // to be re-rooted at the repo before the resolvers below can find them. + // // Prefer dbt's COMPILED SQL (target/compiled) for the engine lanes — the clean // approach (Datafold/Recce/Fusion all render-then-analyze rather than parse Jinja). // Use `dbtRoot` (the discovered project root when G3 auto-discovery fired, @@ -325,11 +375,134 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise /* keep original values on realpath failure */ } const pathPrefix = path.relative(gitRootReal, dbtRootReal) + + // File list. `--all` replaces the diff with the whole manifest: every model + // as a single version with no base ref. That is the point of the mode — the + // findings that matter in a mature project have not been in a diff for + // months. The base-vs-head lanes cannot run on it and runReview reports them + // as skipped (see orchestrate.ts::PROJECT_SCOPE_SKIPS). + let base = "" + let changedFiles: ChangedFile[] + if (opts.all) { + changedFiles = await manifestModelFiles(manifestAbs, pathPrefix, opts.maxModels) + } else { + // Only resolve a base ref if we actually need git (to collect changed files + // or to read old/new content). A caller that supplies BOTH `changedFiles` and + // `getContent` (e.g. a non-git CI integration) must not be forced through a + // git lookup that can fail when there's no usable history. + const needGit = !opts.changedFiles || !opts.getContent + base = opts.base ?? (needGit ? await defaultBaseRef(opts.cwd) : "") + changedFiles = opts.changedFiles ?? (await collectChangedFiles({ base, head: opts.head, cwd: opts.cwd })) + } + + // Map renamed files → their old path so getContent resolves the "old" side + // from where the file lived at `base`. + const renames = new Map( + changedFiles.filter((f) => f.status === "renamed" && f.oldPath).map((f) => [f.path, f.oldPath as string]), + ) + const getContent = + opts.getContent ?? makeContentResolver({ base, head: opts.head, cwd: opts.cwd, renames, gitRoot }) + const getCompiled = opts.getContent ? undefined : makeCompiledResolver({ cwd: dbtRootReal, projectName, pathPrefix }) - return runReview({ + // Freshness check: detect (and warn — non-fatal) when the manifest predates + // change-affecting files. Runs for both the pinned-head CI shape AND the + // working-tree local shape — the local scenario "dbt compile once, edit for + // an hour, then altimate review" is where staleness actually bites in + // practice, so gating this behind `--head` (the prior behavior) silently + // under-warned the most common developer workflow. mtime granularity on + // the working tree can be noisy during a live edit session, but the check + // is limited to files that would materially change the manifest + // (`isManifestAffecting`), so noise is bounded. Return value is stamped + // into the signed envelope so downstream auditors see it too. + const staleManifest = await detectStaleManifest(manifestAbs, changedFiles, gitRoot) + + // Resolve the SQL dialect: explicit config wins; otherwise auto-detect from + // the dbt manifest's `adapter_type` (so a BigQuery/Redshift project isn't + // analyzed as the snowflake default — wrong-dialect portability suppression). + if (!config.dialect) config.dialect = (await detectDialect(manifestAbs)) ?? "snowflake" + + // Prefer dbt's catalog.json (real warehouse columns from `dbt docs generate`) + // for the schema context — complete columns are what make column-lineage + // breakage and proven equivalence actually fire (the manifest only has + // documented columns). Falls back to manifest-derived schema when absent. + const catalogAbs = path.join(path.dirname(manifestAbs), "catalog.json") + const catalogSchema = await buildCatalogSchemaContext(catalogAbs) + // altimate_change start — schema_lag lane: the job map, resolved lazily (dbt ls) + const projectAbs = path.dirname(path.dirname(manifestAbs)) + const dbtOpts = { + dbtBin: opts.dbt?.dbtBin, + profilesDir: + opts.dbt?.profilesDir ?? (existsSync(path.join(projectAbs, "profiles.yml")) ? projectAbs : undefined), + target: opts.dbt?.target, + } + const lag = config.schemaLag + let jobsMemo: Promise | undefined + const jobs = + lag && (lag.jobs || lag.jobsFrom) + ? () => + (jobsMemo ??= (async () => { + const map = lag.jobs ? readJobMap(path.resolve(projectAbs, lag.jobs)) : jobsFromSelectors(projectAbs) + if ("error" in map) { + process.stderr.write(`⚠️ schema_lag: ${map.error}; cross-job lag is skipped\n`) + return undefined + } + const tmp = await mkdtemp(path.join(tmpdir(), "review-ls-")) + try { + const r = await resolveJobMap(map, { + projectDir: projectAbs, + ...dbtOpts, + targetPath: path.join(tmp, "target"), + logPath: path.join(tmp, "logs"), + }) + for (const e of r.errors) process.stderr.write(`⚠️ schema_lag: ${e}\n`) + return r.map + } finally { + await rm(tmp, { recursive: true, force: true }) + } + })()) + : undefined + const runner = createDispatcherRunner({ manifestPath: manifestAbs, schemaContext: catalogSchema, jobs }) + // altimate_change end + const mhash = await manifestHash(manifestAbs, opts.cwd) + + // altimate_change start — schema_lag lane: compiled base/head, built once on first use + const cleanups: Array<() => void> = [] + let sidesMemo: Promise | undefined + const lagSides = () => + (sidesMemo ??= (async (): Promise => { + let basePath = opts.baseManifest ? path.resolve(opts.cwd, opts.baseManifest) : undefined + let headPath = opts.headManifest ? path.resolve(opts.cwd, opts.headManifest) : undefined + if (!basePath && !opts.all && base && !opts.changedFiles) { + const models = [ + ...new Set( + changedFiles + .filter((f) => f.status === "modified" && f.path.endsWith(".sql") && /(^|\/)models\//.test(f.path)) + .map((f) => path.basename(f.path, ".sql")), + ), + ] + if (!models.length) return undefined + const sides = await compileSides({ projectDir: projectAbs, gitRoot, base, head: opts.head, changed: models, dbt: dbtOpts }) + cleanups.push(sides.cleanup) + for (const note of sides.notes) process.stderr.write(`ℹ️ schema_lag: ${note}\n`) + basePath = sides.baseManifest + headPath = sides.headManifest + } + if (!basePath || !headPath) return undefined + const b = loadManifest(projectAbs, basePath) + const h = loadManifest(projectAbs, headPath) + if (!b.ok || !h.ok) return undefined + return { + sql: (model, side) => findModel(side === "old" ? b.manifest : h.manifest, model)?.compiled_code ?? undefined, + manifest: h.path, + baseManifest: b.path, + } + })()) + try { + // altimate_change end + return await runReview({ changedFiles, config, rubric, @@ -353,5 +526,11 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise explainTier: opts.explainTier, forceTier: opts.forceTier, staleManifest, + scope: opts.all ? "project" : "diff", + lagSides, }) + } finally { + for (const cleanup of cleanups) cleanup() + } + // altimate_change end } diff --git a/packages/opencode/src/altimate/review/runner.ts b/packages/opencode/src/altimate/review/runner.ts index eec3732d81..6e77dd882d 100644 --- a/packages/opencode/src/altimate/review/runner.ts +++ b/packages/opencode/src/altimate/review/runner.ts @@ -3,6 +3,10 @@ import { EngineCoerce } from "../native/engine-coerce" import { parseManifest } from "../native/dbt/manifest" import type { CheckResult, EquivalenceResult, GradeResult, ImpactResult, ReviewRunner } from "./orchestrate" import { buildReviewSchemaContext, type SchemaContext } from "./schema-context" +// altimate_change start — analyzeDbtImpact normalization (shared with `altimate dbt-impact`) +import path from "path" +import { normalizeImpactReport } from "../../cli/cmd/dbt-impact-helpers" +// altimate_change end /** * Production ReviewRunner backed by the native Dispatcher (the Rust core). @@ -26,9 +30,23 @@ interface CachedManifest { children: Map // unique_id -> direct child unique_ids testDeps: Map> // model unique_id -> set of test unique_ids depending on it schemaContext?: SchemaContext // model/source columns for equivalence resolution + /** Lower-cased names of every relation dbt knows (models, sources, seeds, snapshots). */ + relationNames?: Set ok: boolean } +/** + * A validation error naming a relation dbt knows but the review schema lacks + * (a model whose columns are not documented) is the schema's gap, not an + * error in the SQL: drop it. Everything else is kept. + */ +export function isSchemaGapError(message: string, relationNames: Set | undefined): boolean { + const m = /Table '([^']+)' not found/.exec(message) + if (!m || !relationNames) return false + const name = m[1].split(".").pop()!.replace(/"/g, "").toLowerCase() + return relationNames.has(name) +} + function asArray(v: unknown): T[] { return Array.isArray(v) ? (v as T[]) : [] } @@ -96,6 +114,11 @@ function lintCategory(rule: string, code: string): string | undefined { export interface DispatcherRunnerOptions { manifestPath: string + // altimate_change start — sink map for analyzeDbtImpact (loaders / served tables) + sinksPath?: string + /** Resolved job map for the schema-lag lane's cross-job check (lazy). */ + jobs?: () => Promise + // altimate_change end /** Optional inline schema context to enable real equivalence proofs. */ schemaContext?: Record } @@ -143,7 +166,16 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun asArray(res.seeds), asArray((res as any).snapshots), ) - return { models, byName, children, testDeps, schemaContext, ok: models.size > 0 } + const relationNames = new Set() + for (const r of [ + ...nodes, + ...asArray(res.sources), + ...asArray(res.seeds), + ]) { + for (const n of [r?.name, r?.alias, r?.identifier]) + if (typeof n === "string" && n) relationNames.add(n.toLowerCase()) + } + return { models, byName, children, testDeps, schemaContext, relationNames, ok: models.size > 0 } } catch { return { models: new Map(), @@ -198,6 +230,7 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun ? { ...schema, dialect } : { tables: { _altimate_lint_: { columns: [{ name: "_", type: "string" }] } }, version: "1", dialect } const res = await Dispatcher.call("altimate_core.check", { sql, schema_context: schemaContext, base_sql: baseSql }) + const relationNames = (await loadManifest()).relationNames const data = (res.data ?? {}) as Record // `altimate_core.check` is a composite: { validation, lint, safety }. // The AST anti-pattern findings live at data.lint.findings (each a @@ -210,7 +243,9 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun // in lint-only mode the throwaway `_altimate_lint_` schema makes // every real table "unknown" and would flood the review. hasTables - ? asArray(data.validation?.errors).map((e: any) => ({ ...e, rule: "validate", severity: "error" })) + ? asArray(data.validation?.errors) + .filter((e: any) => !isSchemaGapError(String(e?.message ?? ""), relationNames)) + .map((e: any) => ({ ...e, rule: "validate", severity: "error" })) : [], ) .concat( @@ -455,6 +490,47 @@ export function createDispatcherRunner(opts: DispatcherRunnerOptions): ReviewRun } }, + // altimate_change start — per-hop impact incl. loaders / served tables. + // Null (not []) when the core build lacks analyzeDbtImpact, so the lanes + // know there was no evidence rather than evidence of nothing. + async dbtImpact(change: { model: string; column?: string; kind: string }) { + try { + const res = await Dispatcher.call("altimate_core.dbt_impact", { + // The manifest lives at /target/manifest.json. + project_dir: path.dirname(path.dirname(path.resolve(opts.manifestPath))), + change_json: JSON.stringify(change), + options_json: opts.sinksPath ? JSON.stringify({ sinks: opts.sinksPath }) : undefined, + }) + if (!res.success) return null + return normalizeImpactReport(res.data).entries + } catch { + return null + } + }, + + async schemaLag( + change: { model: string; kind: "sql"; base_sql: string; head_sql: string }, + manifests?: { manifest: string; base_manifest: string }, + ) { + try { + const jobs = opts.jobs ? await opts.jobs().catch(() => undefined) : undefined + const options: Record = { ...manifests } + if (opts.sinksPath) options.sinks = opts.sinksPath + if (jobs) options.jobs = jobs + const res = await Dispatcher.call("altimate_core.dbt_impact", { + project_dir: path.dirname(path.dirname(path.resolve(opts.manifestPath))), + change_json: JSON.stringify(change), + options_json: Object.keys(options).length ? JSON.stringify(options) : undefined, + }) + if (!res.success) return null + const r = normalizeImpactReport(res.data) + return { findings: r.schema_lag, skipped: r.schema_lag_skipped } + } catch { + return null + } + }, + // altimate_change end + async columnLineage(sql: string, dialect?: string): Promise> { try { const res = await Dispatcher.call("altimate_core.column_lineage", { diff --git a/packages/opencode/src/altimate/review/verdict.ts b/packages/opencode/src/altimate/review/verdict.ts index fe7441bb81..044dc312c9 100644 --- a/packages/opencode/src/altimate/review/verdict.ts +++ b/packages/opencode/src/altimate/review/verdict.ts @@ -78,6 +78,19 @@ export function applyMode(verdict: Verdict, mode: ReviewMode): Verdict { export const RiskTier = z.enum(["trivial", "lite", "full"]) export type RiskTier = z.infer +/** + * What the review looked at. + * + * `diff` (the default) is the PR shape: base..head, per-file hunks, and the + * lanes that compare the two versions of a model. + * `project` is the `--all` shape: every model in the manifest, one version + * each. The base-vs-head lanes cannot run there and are reported as SKIPPED + * with a reason in `lanes.skipped` — a report that silently omits half the + * lanes is worse than no report. + */ +export const ReviewScope = z.enum(["diff", "project"]) +export type ReviewScope = z.infer + export const EngineVersions = z.object({ reviewer: z.string().default("dbt-pr-review/1"), core: z.string().optional(), @@ -103,6 +116,23 @@ export const VerdictEnvelope = z.object({ tierForced: z.boolean().optional(), /** The classifier's original tier before --force-tier overrode it (G2). */ tierClassified: RiskTier.optional(), + /** What was reviewed: `diff` (base..head — the default, so it is omitted) + * or `project` (`--all`: every model in the manifest, no base version). */ + scope: ReviewScope.optional(), + /** Which review lanes ran and which could not, with the reason. Present in + * `project` scope, where the base-vs-head lanes have no input — a consumer + * must be able to tell "clean" from "that half of the review never ran". */ + lanes: z + .object({ + ran: z.array(z.string()), + skipped: z.array(z.object({ lane: z.string(), reason: z.string() })), + /** Models the engine lanes saw as dbt-COMPILED SQL, out of `totalModels`. + * Below 100% the AST lanes ran against raw Jinja, could not parse it, + * and returned zero findings for those models — see LaneReport. */ + compiledModels: z.number().int().nonnegative(), + totalModels: z.number().int().nonnegative(), + }) + .optional(), findings: z.array(Finding), summary: z.object({ critical: z.number().int().nonnegative(), @@ -178,6 +208,15 @@ export interface BuildEnvelopeInput { tierClassified?: RiskTier /** True when mtime signals the manifest predates a change-affecting file. */ staleManifest?: boolean + /** `project` when the run reviewed the whole manifest (`--all`). */ + scope?: ReviewScope + /** Lane run/skip report — see the envelope field. */ + lanes?: { + ran: string[] + skipped: Array<{ lane: string; reason: string }> + compiledModels: number + totalModels: number + } } function summarize(findings: Finding[], degraded: boolean): VerdictEnvelope["summary"] { @@ -201,6 +240,8 @@ export function buildEnvelope(input: BuildEnvelopeInput): VerdictEnvelope { tierReasons: input.tierReasons, tierForced: input.tierForced, tierClassified: input.tierClassified, + scope: input.scope, + lanes: input.lanes, findings: input.findings, summary: summarize(input.findings, degraded), engine: EngineVersions.parse(input.engine ?? {}), diff --git a/packages/opencode/src/cli/cmd/dbt-factor-helpers.ts b/packages/opencode/src/cli/cmd/dbt-factor-helpers.ts new file mode 100644 index 0000000000..e5f75916bf --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-factor-helpers.ts @@ -0,0 +1,119 @@ +// altimate_change start — dbt-factor-helpers: normalize and render the engine's +// factoring proposals (shared logic → one new intermediate model), and build +// the multi-file patch for one. +import path from "path" +import { type Manifest, type NormalizedProof, findModel, formatProof, normalizeProof, unifiedPatch } from "./dbt-mart-helpers" +import { type CostAssessment, type FileEdit, formatCost, mapConsumerSql } from "./dbt-optimize-helpers" + +export interface FactorConsumer { + model: string + consumer_sql?: string + proof: NormalizedProof +} + +export interface FactorProposal { + index: number + /** New model name, always `int_`-prefixed. */ + name: string + new_model_sql: string + schema_yml?: string + consumers: FactorConsumer[] + occurrences?: number + savings?: unknown + storage?: unknown + raw: Record +} + +/** Normalize the engine's proposals; field names are tolerated in the forms + * the plan used (`new_model_name`/`name`, `new_model_sql`/`sql`, …). */ +export function normalizeFactorProposals(list: unknown): FactorProposal[] { + const arr = Array.isArray(list) ? (list as Record[]) : [] + return arr.map((p, i) => { + const base = String(p.new_model_name ?? p.name ?? `shared_${i + 1}`) + const consumers = Array.isArray(p.consumers) ? (p.consumers as Record[]) : [] + return { + index: i + 1, + name: base.startsWith("int_") ? base : `int_${base}`, + new_model_sql: String(p.new_model_sql ?? p.sql ?? ""), + schema_yml: typeof (p.schema_yml ?? p.yml) === "string" ? String(p.schema_yml ?? p.yml) : undefined, + consumers: consumers.map((c) => ({ + model: String(c.model ?? c.name ?? "?"), + consumer_sql: typeof (c.consumer_sql ?? c.after_sql) === "string" ? String(c.consumer_sql ?? c.after_sql) : undefined, + proof: normalizeProof(c.proof), + })), + occurrences: typeof p.occurrences === "number" ? p.occurrences : consumers.length, + savings: p.savings ?? p.savings_estimate, + storage: p.storage ?? p.storage_tradeoff, + raw: p, + } + }) +} + +const show = (v: unknown) => (v === undefined || v === null ? "(not stated)" : typeof v === "string" ? v : JSON.stringify(v)) + +export function formatFactorProposal(p: FactorProposal): string[] { + const lines = [`#${p.index} ${p.name} — shared by ${p.consumers.length} consumer(s) (${p.occurrences ?? "?"} occurrence(s))`] + lines.push(` savings: ${show(p.savings)}`) + lines.push(` storage trade-off: ${show(p.storage)}`) + const cost = formatCost(p.raw.cost as CostAssessment | undefined) + if (cost) lines.push(` cost: ${cost}`) + for (const c of p.consumers) lines.push(` ${c.model}: ${formatProof(c.proof)}`) + return lines +} + +export type FactorPlan = + | { ok: true; edits: FileEdit[]; patch: string } + | { ok: false; reason: string; patch: string } + +/** + * The multi-file patch: `models/intermediate/.sql`, its yml contract, + * and each consumer edited to read it. Refuses when any consumer proof is + * refuted, or when a consumer's Jinja cannot be mapped back (the compiled + * patch is still returned for reading). + */ +export function buildFactorPlan(input: { + manifest: Manifest + proposal: FactorProposal + readRaw: (file: string) => string | undefined +}): FactorPlan { + const { manifest, proposal: p } = input + const dir = "models/intermediate" + const newFile = path.join(dir, `${p.name}.sql`) + const ymlFile = path.join(dir, `${p.name}.yml`) + const compiledPatch = + unifiedPatch(newFile + " (compiled)", "", p.new_model_sql) + + p.consumers.map((c) => unifiedPatch(`${c.model} (compiled)`, "", c.consumer_sql ?? "")).join("") + const refuted = p.consumers.filter((c) => c.proof.status === "refuted") + if (refuted.length) + return { + ok: false, + reason: `consumer proof REFUTED for ${refuted.map((c) => c.model).join(", ")} — factoring would change their results`, + patch: compiledPatch, + } + const invalid = p.consumers.filter((c) => c.proof.status === "invalid_sql") + if (invalid.length) + return { + ok: false, + reason: `generated SQL does not bind for ${invalid.map((c) => c.model).join(", ")} — ${invalid[0].proof.reason ?? "invalid SQL"}`, + patch: compiledPatch, + } + const edits: FileEdit[] = [] + let newSql: string | undefined + for (const c of p.consumers) { + const node = findModel(manifest, c.model) + if (!node?.original_file_path) return { ok: false, reason: `consumer ${c.model} not in the manifest`, patch: compiledPatch } + if (!c.consumer_sql) return { ok: false, reason: `no edited SQL for consumer ${c.model}`, patch: compiledPatch } + const raw = input.readRaw(node.original_file_path) ?? node.raw_code ?? "" + const m = mapConsumerSql({ model: node, raw, consumerSql: c.consumer_sql, newModelName: p.name }) + if (!m.ok) return { ok: false, reason: `${c.model}: ${m.reason}`, patch: compiledPatch } + newSql ??= m.newModelSql(p.new_model_sql) + edits.push({ file: node.original_file_path, before: raw, after: m.sql }) + } + if (!newSql) return { ok: false, reason: "proposal has no consumers", patch: compiledPatch } + const yml = + p.schema_yml ?? + `version: 2\n\nmodels:\n - name: ${p.name}\n description: "Shared logic factored out of ${p.consumers.map((c) => c.model).join(", ")}."\n` + edits.unshift({ file: newFile, before: "", after: newSql }, { file: ymlFile, before: "", after: yml }) + return { ok: true, edits, patch: edits.map((e) => unifiedPatch(e.file, e.before, e.after)).join("") } +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-factor.ts b/packages/opencode/src/cli/cmd/dbt-factor.ts new file mode 100644 index 0000000000..b3713596dd --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-factor.ts @@ -0,0 +1,170 @@ +// altimate_change start — dbt-factor: shared SQL across models, proposed as one +// new intermediate model plus consumer edits, each consumer with its own proof. +import type { Argv } from "yargs" +import { cmd } from "./cmd" +import { Dispatcher } from "../../altimate/native" +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs" +import path from "path" +import { findModel, loadManifest } from "./dbt-mart-helpers" +import { buildFactorPlan, formatFactorProposal, normalizeFactorProposals } from "./dbt-factor-helpers" +import { changesForFile, formatImpactText, normalizeImpactReport } from "./dbt-impact-helpers" +import { parityCommand } from "./dbt-optimize-helpers" + +export const DbtFactorCommand = cmd({ + command: "dbt-factor", + describe: "find SQL repeated across dbt models and propose computing it once in a shared model (deterministic)", + builder: (yargs: Argv) => + yargs + .option("project", { describe: "scan the whole project (default when --model is absent)", type: "boolean" }) + .option("model", { describe: "only proposals that touch this model", type: "string" }) + .option("min-occurrences", { describe: "minimum number of models sharing the logic", type: "number", default: 2 }) + .option("apply", { + describe: "write proposal --proposal (default #1); refuses if any consumer proof is refuted", + type: "boolean", + default: false, + }) + .option("proposal", { describe: "1-based proposal number to apply", type: "number", default: 1 }) + .option("cwd", { describe: "dbt project directory (default: current dir)", type: "string" }) + .option("manifest", { describe: "path to dbt manifest.json", type: "string" }) + .option("run-results", { + describe: "dbt run_results.json — measured model time, the cost gate's size signal", + type: "string", + }) + .option("trino-profile", { describe: "trino-profile.json — the same size signal", type: "string" }) + .option("json", { describe: "emit the full result as JSON", type: "boolean", default: false }), + + handler: async (args: { + project?: boolean + model?: string + "min-occurrences"?: number + apply?: boolean + proposal?: number + cwd?: string + manifest?: string + "run-results"?: string + "trino-profile"?: string + json?: boolean + }) => { + const projectDir = path.resolve(args.cwd ?? process.cwd()) + const loaded = loadManifest(projectDir, args.manifest) + if (!loaded.ok) return fatal(loaded.error) + if (args.model && !findModel(loaded.manifest, args.model)) + return fatal(`model not found (or ambiguous) in the manifest: ${args.model}`) + + // Core `FactoringOpts` is `deny_unknown_fields`; its key is `min_models`. + const options: Record = { min_models: args["min-occurrences"] ?? 2 } + if (args["run-results"]) options.run_results = path.resolve(args["run-results"]) + if (args["trino-profile"]) options.trino_profile = path.resolve(args["trino-profile"]) + const res = await Dispatcher.call("altimate_core.dbt_factoring", { + project_dir: projectDir, + options_json: JSON.stringify(options), + }).catch((e: unknown) => ({ success: false, data: {} as Record, error: String(e) })) + if (!res.success) return fatal(res.error ?? "altimate_core.dbt_factoring failed") + let proposals = normalizeFactorProposals(res.data.proposals) + if (args.model) proposals = proposals.filter((p) => p.consumers.some((c) => c.model === args.model)) + + const readRaw = (rel: string) => { + const abs = path.join(projectDir, rel) + return existsSync(abs) ? readFileSync(abs, "utf-8") : undefined + } + const plans = proposals.map((p) => ({ p, plan: buildFactorPlan({ manifest: loaded.manifest, proposal: p, readRaw }) })) + + const out: string[] = [] + let applied: string[] = [] + let refused: string | undefined + const impacts: unknown[] = [] + if (args.apply) { + const chosen = plans.find((x) => x.p.index === (args.proposal ?? 1)) + if (!chosen) refused = `no proposal #${args.proposal ?? 1} (there are ${plans.length})` + else if (!chosen.plan.ok) refused = chosen.plan.reason + else { + const clash = chosen.plan.edits.find((e) => !e.before && existsSync(path.join(projectDir, e.file))) + if (clash) refused = `refusing to overwrite existing file ${clash.file}` + else { + for (const e of chosen.plan.edits) { + const abs = path.join(projectDir, e.file) + mkdirSync(path.dirname(abs), { recursive: true }) + writeFileSync(abs, e.after) + } + applied = chosen.plan.edits.map((e) => e.file) + // Impact of every touched consumer: an output-column change there is + // a break somewhere downstream, and must be seen before merging. + for (const c of chosen.p.consumers) { + const edit = chosen.plan.edits.find((e) => e.file === findModel(loaded.manifest, c.model)?.original_file_path) + const d = changesForFile({ + model: c.model, + status: "modified", + baseRaw: edit?.before, + headRaw: edit?.after, + }) + for (const u of d.unclassified) out.push(`dbt-impact ${u.model}: UNCLASSIFIED — ${u.reason}`) + for (const change of d.changes) { + const r = await Dispatcher.call("altimate_core.dbt_impact", { + project_dir: projectDir, + change_json: JSON.stringify(change), + }).catch((e: unknown) => ({ success: false, data: {} as Record, error: String(e) })) + impacts.push({ change, report: r.success ? normalizeImpactReport(r.data) : { error: r.error } }) + if (r.success) out.push(formatImpactText(change, normalizeImpactReport(r.data))) + else out.push(`dbt-impact ${c.model}: ${r.error}`) + } + if (!d.changes.length && !d.unclassified.length) + out.push(`dbt-impact ${c.model}: no change to its SQL or materialization — nothing downstream to break`) + } + } + } + } + + const parity = proposals.flatMap((p) => + p.consumers.map((c) => { + const node = findModel(loaded.manifest, c.model) + return node ? parityCommand(node) : `altimate dbt-parity --model ${c.model} --before --after ` + }), + ) + + if (args.json) { + process.stdout.write( + JSON.stringify( + { + version: 1, + proposals: plans.map(({ p, plan }) => ({ + ...p.raw, + index: p.index, + name: p.name, + consumers: p.consumers.map((c) => ({ model: c.model, proof_status: c.proof.status, proof: c.proof })), + patch: plan.patch, + apply_blocked: plan.ok ? undefined : plan.reason, + })), + apply: args.apply ? { applied, refused, impacts } : undefined, + parity_commands: [...new Set(parity)], + }, + null, + 2, + ) + "\n", + ) + } else { + const lines: string[] = [] + lines.push(proposals.length ? `Factoring proposals (${proposals.length}):` : "No factoring proposals.") + for (const { p, plan } of plans) { + lines.push(...formatFactorProposal(p)) + if (!plan.ok) lines.push(` cannot auto-apply: ${plan.reason}`) + lines.push(plan.patch) + } + if (args.apply) { + if (refused) lines.push(`NOT APPLIED: ${refused}`) + else lines.push(`APPLIED: ${applied.join(", ")}`, "", ...out) + } + if (parity.length) { + lines.push("", "Next — build the new model and consumers, then prove each consumer's data is unchanged:") + for (const c of new Set(parity)) lines.push(` ${c}`) + } + console.error(lines.join("\n")) + } + if (args.apply && !applied.length) process.exitCode = 1 + }, +}) + +function fatal(msg: string): void { + console.error(`Error: ${msg}`) + process.exitCode = 1 +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-impact-compiled.ts b/packages/opencode/src/cli/cmd/dbt-impact-compiled.ts new file mode 100644 index 0000000000..5783620dd5 --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-impact-compiled.ts @@ -0,0 +1,163 @@ +// altimate_change start — dbt-impact compiled diff mode: compare the +// `compiled_code` of the base and head manifests instead of a naive Jinja +// render, so `{% set %}`, vars and macros no longer leave a model UNCLASSIFIED. +import { execFile } from "child_process" +import { existsSync, mkdirSync, mkdtempSync, rmSync, symlinkSync } from "fs" +import { tmpdir } from "os" +import path from "path" +import { promisify } from "util" +import { dbtCompileOffline, type DbtOfflineOptions } from "../../altimate/native/dbt/runner" +import { configUniqueKey, type DbtChange, type DiffChanges, naiveRender } from "./dbt-impact-helpers" +import type { Manifest, ManifestNode } from "./dbt-mart-helpers" + +const exec = promisify(execFile) + +const norm = (s?: string | null) => (s ?? "").replace(/\s+/g, " ").trim() + +function models(m: Manifest): Map { + const out = new Map() + for (const [id, n] of Object.entries(m.nodes ?? {})) + if (n.resource_type === "model" && (n.config as { enabled?: boolean } | undefined)?.enabled !== false) out.set(id, n) + return out +} + +const cfg = (n: ManifestNode, k: string) => (n.config as Record | undefined)?.[k] + +/** + * The impact changes between two manifests. Per model (restricted to `only` + * names when given): gone in head → `drop`; a changed `materialized` → + * `materialization`; different `compiled_code` → a `sql` change on the + * compiled SQL. A model compiled on neither side falls back to the naive + * render of `raw_code`, and is unclassified only when that fails too. + */ +export function changesFromManifests(base: Manifest, head: Manifest, only?: Set): DiffChanges { + const b = models(base) + const h = models(head) + const changes: DbtChange[] = [] + const unclassified: DiffChanges["unclassified"] = [] + const keep = (n: ManifestNode) => !only || only.has(String(n.name)) + for (const [id, bn] of b) { + if (!keep(bn)) continue + const model = String(bn.name) + const hn = h.get(id) + if (!hn) { + changes.push({ model, kind: "drop" }) + continue + } + const [from, to] = [cfg(bn, "materialized"), cfg(hn, "materialized")] as Array + if (from !== to && to) { + const c: DbtChange = { model, kind: "materialization", from, to } + const strategy = cfg(hn, "incremental_strategy") + if (typeof strategy === "string") c.incremental_strategy = strategy + const uk = cfg(hn, "unique_key") + const keys = typeof uk === "string" ? [uk] : Array.isArray(uk) ? uk.map(String) : configUniqueKey(hn.raw_code) + if (keys.length) c.unique_key = keys + changes.push(c) + } + const [bc, hc] = [norm(bn.compiled_code), norm(hn.compiled_code)] + if (bc && hc) { + if (bc !== hc) + changes.push({ model, kind: "sql", base_sql: String(bn.compiled_code), head_sql: String(hn.compiled_code) }) + continue + } + if (norm(bn.raw_code) === norm(hn.raw_code)) continue + const [br, hr] = [naiveRender(bn.raw_code ?? ""), naiveRender(hn.raw_code ?? "")] + if (br === undefined || hr === undefined) { + unclassified.push({ model, reason: "not compiled on both sides, and uses Jinja beyond ref/source/config" }) + continue + } + if (br.trim() !== hr.trim()) changes.push({ model, kind: "sql", base_sql: br, head_sql: hr }) + } + return { changes, unclassified } +} + +/** A project tree at a git ref, extracted into `dest` without touching the + * repository (`git archive`). Returns the project directory inside it. */ +export async function extractTree(gitRoot: string, ref: string, relProject: string, dest: string): Promise { + mkdirSync(dest, { recursive: true }) + const tar = path.join(dest, "..", `${path.basename(dest)}.tar`) + await exec("git", ["archive", "--format=tar", "-o", tar, ref, ...(relProject ? ["--", relProject] : [])], { + cwd: gitRoot, + maxBuffer: 64 * 1024 * 1024, + }) + await exec("tar", ["-xf", tar, "-C", dest]) + rmSync(tar, { force: true }) + return path.join(dest, relProject) +} + +export interface CompiledSides { + baseManifest?: string + headManifest?: string + /** The head project directory (for `dbt ls`). */ + headDir: string + notes: string[] + cleanup: () => void +} + +export type CompileFn = typeof dbtCompileOffline + +/** + * Compile both sides of a diff into a temp dir: base from `git archive`, head + * from `git archive` (or the working tree when `head` is omitted, with every + * artifact redirected to the temp dir). Only the changed models are compiled + * on base, and the changed models plus their descendants on head. A side + * whose compile fails keeps whatever manifest dbt wrote (parse only), and the + * failure is reported. + */ +export async function compileSides(input: { + projectDir: string + gitRoot: string + base: string + head?: string + changed: string[] + dbt: Pick + compile?: CompileFn +}): Promise { + const compile = input.compile ?? dbtCompileOffline + const tmp = mkdtempSync(path.join(tmpdir(), "dbt-impact-")) + const cleanup = () => { + if (!process.env.ALTIMATE_KEEP_IMPACT_TMP) rmSync(tmp, { recursive: true, force: true }) + } + const rel = path.relative(input.gitRoot, input.projectDir) + const profilesDir = + input.dbt.profilesDir ?? (existsSync(path.join(input.projectDir, "profiles.yml")) ? input.projectDir : undefined) + const notes: string[] = [] + const withPackages = (dir: string) => { + const pk = path.join(input.projectDir, "dbt_packages") + if (dir !== input.projectDir && existsSync(pk) && !existsSync(path.join(dir, "dbt_packages"))) + symlinkSync(pk, path.join(dir, "dbt_packages")) + } + const side = async (name: "base" | "head", dir: string, select: string[]) => { + withPackages(dir) + const targetPath = path.join(tmp, `${name}-target`) + const r = await compile(select, { + projectDir: dir, + dbtBin: input.dbt.dbtBin, + profilesDir, + target: input.dbt.target, + targetPath, + logPath: path.join(tmp, `${name}-logs`), + }) + const manifest = path.join(targetPath, "manifest.json") + if (!r.ok) notes.push(`${name}: dbt compile failed (${r.error ?? "unknown error"}); uncompiled models fall back to the raw SQL`) + else if (!r.offline) notes.push(`${name}: compiled with a warehouse connection (the offline compile needed introspection)`) + return existsSync(manifest) ? manifest : undefined + } + try { + const baseDir = await extractTree(input.gitRoot, input.base, rel, path.join(tmp, "base")) + const headDir = input.head + ? await extractTree(input.gitRoot, input.head, rel, path.join(tmp, "head")) + : input.projectDir + const baseManifest = await side("base", baseDir, input.changed) + const headManifest = await side( + "head", + headDir, + input.changed.map((m) => `${m}+`), + ) + return { baseManifest, headManifest, headDir, notes, cleanup } + } catch (e) { + notes.push(`could not prepare the base/head trees: ${e instanceof Error ? e.message : String(e)}`) + return { headDir: input.projectDir, notes, cleanup } + } +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-impact-helpers.ts b/packages/opencode/src/cli/cmd/dbt-impact-helpers.ts new file mode 100644 index 0000000000..42ff2efe85 --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-impact-helpers.ts @@ -0,0 +1,300 @@ +// altimate_change start — dbt-impact-helpers: change construction, impact-report +// normalization and rendering for `altimate dbt-impact` and the review lane. + +export type ChangeKind = "drop" | "rename" | "retype" | "grain" | "materialization" | "add" +export const CHANGE_KINDS: readonly ChangeKind[] = ["drop", "rename", "retype", "grain", "materialization", "add"] + +/** The change handed to `analyzeDbtImpact` as `changeJson` — the engine's + * `Change`: `{model, column?, kind, …kind-specific fields}`. `sql` compares + * two versions of the model (diff mode). */ +export interface DbtChange { + model: string + column?: string + kind: ChangeKind | "sql" + new_name?: string + from_type?: string + to_type?: string + removed_key?: string + from?: string + to?: string + incremental_strategy?: string + unique_key?: string[] + /** `add`: the columns added. */ + columns?: string[] + base_sql?: string + head_sql?: string +} + +/** `unique_key='a'` or `unique_key=['a', 'b']` from a model's `config()`. */ +export function configUniqueKey(raw?: string): string[] { + const m = raw?.match(/unique_key\s*=\s*(\[[^\]]*\]|['"][^'"]+['"])/)?.[1] + if (!m) return [] + return [...m.matchAll(/['"]([^'"]+)['"]/g)].map((x) => x[1]) +} + +/** Build a CLI change, enforcing the fields each kind requires. */ +export function buildChange(input: { + model: string + column?: string + kind: ChangeKind + newName?: string + toType?: string + fromType?: string + to?: string + strategy?: string + uniqueKey?: string[] +}): DbtChange | { error: string } { + const c: DbtChange = { model: input.model, kind: input.kind } + if (input.column) c.column = input.column + switch (input.kind) { + case "rename": + if (!input.newName) return { error: "--kind rename needs --new-name" } + c.new_name = input.newName + break + case "retype": + if (!input.toType) return { error: "--kind retype needs --to-type" } + c.to_type = input.toType + if (input.fromType) c.from_type = input.fromType + break + case "grain": + if (input.column) c.removed_key = input.column + break + case "add": + if (!input.column) return { error: "--kind add needs --column " } + delete c.column + c.columns = [input.column] + break + case "materialization": + if (!input.to) return { error: "--kind materialization needs --to " } + c.to = input.to + if (input.strategy) c.incremental_strategy = input.strategy + if (input.uniqueKey?.length) c.unique_key = input.uniqueKey + break + } + return c +} + +export type ImpactVerdict = "breaking" | "risky" | "safe" | "unknown" + +export interface ImpactEntry { + hop: number + /** dbt model, loader, served table, … as the engine names it. */ + node_type: string + name: string + verdict: ImpactVerdict + reason?: string + columns?: string[] + [key: string]: unknown +} + +/** A reader production can run against a relation that lacks the column. */ +export interface SchemaLagFinding { + reason: string + verdict: ImpactVerdict + consumer: string + column: string + producer: string + consumer_jobs?: string[] + producer_jobs?: string[] + schedules?: Record + window?: string + message: string + fix: string + alternatives?: string[] +} + +export interface ImpactReport { + change?: unknown + /** The engine's overall verdict, when it states one. */ + verdict?: ImpactVerdict + entries: ImpactEntry[] + notes: string[] + had_sink_map?: boolean + loaders?: string[] + served_tables?: string[] + /** Loaders are unknown (no sink map, or the model is not in a partial one). */ + sinks_unknown?: boolean + /** Schema-lag findings (`schema_lag:cross_job`, ...). */ + schema_lag: SchemaLagFinding[] + /** Schema-lag checks that could not run, and why. */ + schema_lag_skipped: string[] + /** Reasons behind the verdict. */ + reasons: string[] + /** SQL diff: the kind it resolved to (`add`, `drop`, ...). */ + resolved_kind?: string + added_columns?: string[] +} + +/** Hop distance for the engine's hop kinds (it reports kinds, not depths). */ +const HOP_ORDER: Record = { model: 0, dbt_child: 1, dbt_descendant: 2, loader: 3, served_table: 4 } + +export const VERDICT_RANK: Record = { breaking: 3, risky: 2, unknown: 1, safe: 0 } + +export function normalizeVerdict(v: unknown): ImpactVerdict { + const s = typeof v === "string" ? v.toLowerCase() : "" + if (s === "breaking" || s === "break" || s === "broken") return "breaking" + if (s === "risky" || s === "risk") return "risky" + if (s === "safe") return "safe" + return "unknown" +} + +/** Normalize the engine's impact report: `hops: [{kind, target, verdict, + * evidence, column_level}]`. Grouped forms (`{hop, nodes: [...]}`) and other + * field spellings are tolerated; a missing verdict is `unknown`, never `safe`. */ +export function normalizeImpactReport(data: unknown): ImpactReport { + const d = (data ?? {}) as Record + const list = (["hops", "impacts", "entries", "value"] as const).map((k) => d[k]).find(Array.isArray) as + | Record[] + | undefined + const entries: ImpactEntry[] = [] + for (const raw of list ?? []) { + const group = Array.isArray(raw?.nodes) ? (raw.nodes as Record[]) : [raw] + for (const e of group) { + if (!e || typeof e !== "object") continue + const nodeType = String(e.node_type ?? e.kind ?? e.type ?? "model") + const hop = + typeof e.hop === "number" ? e.hop : typeof raw?.hop === "number" ? raw.hop : (HOP_ORDER[nodeType] ?? 1) + entries.push({ + ...e, + hop, + node_type: nodeType, + name: String(e.target ?? e.name ?? e.node ?? e.unique_id ?? "?"), + verdict: normalizeVerdict(e.verdict ?? e.severity), + reason: [e.evidence, e.reason, e.message].find((x): x is string => typeof x === "string"), + columns: Array.isArray(e.columns) ? (e.columns as string[]) : undefined, + }) + } + } + const notes = Array.isArray(d.notes) ? (d.notes as string[]) : [] + const summary = (d.summary ?? {}) as Record + const had = d.had_sink_map ?? summary.had_sink_map + const strs = (v: unknown) => (Array.isArray(v) ? (v as unknown[]).map(String) : undefined) + const lag = Array.isArray(d.schema_lag) ? (d.schema_lag as Record[]) : [] + const rk = (d.resolved_kind as { kind?: unknown } | undefined)?.kind + return { + change: d.change, + verdict: d.verdict === undefined ? undefined : normalizeVerdict(d.verdict), + entries, + notes, + had_sink_map: typeof had === "boolean" ? had : undefined, + loaders: strs(d.loaders), + served_tables: strs(d.served_tables), + sinks_unknown: typeof d.sinks_unknown === "boolean" ? d.sinks_unknown : undefined, + schema_lag: lag.map((f) => ({ ...(f as unknown as SchemaLagFinding), verdict: normalizeVerdict(f.verdict) })), + schema_lag_skipped: strs(d.schema_lag_skipped) ?? [], + reasons: strs(d.reasons) ?? [], + resolved_kind: typeof rk === "string" ? rk : undefined, + added_columns: strs(d.added_columns), + } +} + +/** True for a hop inside the dbt graph (the changed model or a dbt reader). */ +export function isDbtHop(nodeType: string): boolean { + return /^(model|dbt_)/i.test(nodeType) +} + +export function worstVerdict(entries: ImpactEntry[]): ImpactVerdict { + return entries.reduce((w, e) => (VERDICT_RANK[e.verdict] > VERDICT_RANK[w] ? e.verdict : w), "safe") +} + +/** The engine's overall verdict, raised to the worst hop when a hop is worse. */ +export function reportVerdict(r: ImpactReport): ImpactVerdict { + const hops = worstVerdict(r.entries) + return r.verdict && VERDICT_RANK[r.verdict] > VERDICT_RANK[hops] ? r.verdict : hops +} + +/** True when a BREAKING entry is outside dbt — a loader or a served table. + * That is the evidence the review lane needs to block a modified model. */ +export function hasExternalBreak(entries: ImpactEntry[]): boolean { + return entries.some((e) => e.verdict === "breaking" && !isDbtHop(e.node_type)) +} + +export function formatImpactText(change: DbtChange, r: ImpactReport): string { + const lines: string[] = [] + const target = change.column ? `${change.model}.${change.column}` : change.model + const detail = change.new_name ?? change.to_type ?? change.to ?? change.columns?.join(", ") + lines.push(`dbt-impact: ${change.kind} ${target}${detail ? ` → ${detail}` : ""}`) + if (r.had_sink_map === false) + lines.push("Sink map: NO — loaders and served tables beyond dbt are not visible (pass --sinks)") + if (r.loaders?.length) lines.push(`Loaders: ${r.loaders.join(", ")}`) + if (r.served_tables?.length) lines.push(`Served tables: ${r.served_tables.join(", ")}`) + lines.push("") + if (!r.entries.length) lines.push("No downstream consumers reported.") + const hops = [...new Set(r.entries.map((e) => e.hop))].sort((a, b) => a - b) + for (const h of hops) { + lines.push(`Hop ${h}:`) + for (const e of r.entries.filter((x) => x.hop === h).sort((a, b) => VERDICT_RANK[b.verdict] - VERDICT_RANK[a.verdict])) { + const cols = e.columns?.length ? ` [${e.columns.join(", ")}]` : "" + lines.push(` ${e.verdict.toUpperCase().padEnd(9)} ${e.node_type} ${e.name}${cols}${e.reason ? ` — ${e.reason}` : ""}`) + } + } + if (r.schema_lag.length) { + lines.push("Schema lag:") + for (const f of r.schema_lag) { + lines.push(` ${f.verdict.toUpperCase().padEnd(9)} ${f.reason} ${f.consumer} reads ${f.producer}.${f.column}`) + lines.push(` ${f.message}`) + lines.push(` Fix: ${f.fix}`) + } + } + for (const s of r.schema_lag_skipped) lines.push(`Skipped: schema_lag ${s}`) + for (const n of r.notes) lines.push(`Note: ${n}`) + lines.push("") + lines.push(`Overall: ${reportVerdict(r).toUpperCase()}${r.reasons.length ? ` (${r.reasons.join(", ")})` : ""}`) + return lines.join("\n") +} + +// --------------------------------------------------------------------------- +// Diff mode +// --------------------------------------------------------------------------- + +/** Render a raw model just enough for column-level analysis: drop the config + * block and Jinja comments, and spell ref/source as bare relation names. + * Returns undefined for anything with other Jinja — it would not parse. */ +export function naiveRender(raw: string): string | undefined { + let s = raw.replace(/\{#[\s\S]*?#\}/g, "").replace(/\{\{-?\s*config\s*\([\s\S]*?\)\s*-?\}\}/, "") + s = s.replace(/\{\{-?\s*ref\s*\(\s*['"]([^'"]+)['"]\s*\)\s*-?\}\}/g, "$1") + s = s.replace(/\{\{-?\s*source\s*\(\s*['"]([^'"]+)['"]\s*,\s*['"]([^'"]+)['"]\s*\)\s*-?\}\}/g, "$1.$2") + return /\{\{|\{%/.test(s) ? undefined : s +} + +export interface DiffChanges { + changes: DbtChange[] + /** Modified models whose change could not be classified, with why. */ + unclassified: Array<{ model: string; reason: string }> +} + +/** + * Turn one changed model file into impact changes: + * deleted/renamed → `drop` of the model; a changed `materialized=` → + * `materialization`; otherwise a `sql` change carrying both versions (refs + * spelled as bare names) for the engine to compare. A modification that + * cannot be rendered (Jinja beyond ref/source/config) is reported as + * unclassified, not silently treated as safe. + */ +export function changesForFile(input: { + model: string + status: "added" | "modified" | "deleted" | "renamed" + baseRaw?: string + headRaw?: string +}): DiffChanges { + const { model, status, baseRaw, headRaw } = input + if (status === "added") return { changes: [], unclassified: [] } + if (status === "deleted" || status === "renamed") return { changes: [{ model, kind: "drop" }], unclassified: [] } + const changes: DbtChange[] = [] + const mat = (raw?: string) => raw?.match(/materialized\s*=\s*['"](\w+)['"]/)?.[1] + const [from, to] = [mat(baseRaw), mat(headRaw)] + if (from !== to && to) { + const change: DbtChange = { model, kind: "materialization", from, to } + const strategy = headRaw?.match(/incremental_strategy\s*=\s*['"]([\w+]+)['"]/)?.[1] + if (strategy) change.incremental_strategy = strategy + const uniqueKey = configUniqueKey(headRaw) + if (uniqueKey.length) change.unique_key = uniqueKey + changes.push(change) + } + const b = baseRaw ? naiveRender(baseRaw) : undefined + const h = headRaw ? naiveRender(headRaw) : undefined + if (!b || !h) return { changes, unclassified: [{ model, reason: "model uses Jinja beyond ref/source/config" }] } + if (b.trim() !== h.trim()) changes.push({ model, kind: "sql", base_sql: b, head_sql: h }) + return { changes, unclassified: [] } +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-impact-jobs.ts b/packages/opencode/src/cli/cmd/dbt-impact-jobs.ts new file mode 100644 index 0000000000..2676152e61 --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-impact-jobs.ts @@ -0,0 +1,139 @@ +// altimate_change start — dbt-impact job maps: which job builds which models, +// for the cross-job schema-lag check. One neutral format, adapters into it, +// and resolution of each job's selection with `dbt ls` (no warehouse). +import { existsSync, readFileSync } from "fs" +import path from "path" +import YAML from "yaml" +import { dbtLs, type DbtOfflineOptions } from "../../altimate/native/dbt/runner" + +/** One scheduled dbt invocation. `models` is the resolved selection + * (`unique_id`s); the engine ignores a job without it. */ +export interface JobDef { + select?: string + exclude?: string + /** A `selectors.yml` selector, when the job runs `--selector`. */ + selector?: string + /** Cron expression, when known. */ + schedule?: string + models?: string[] +} + +/** `{"jobs": {name: {select, exclude, schedule}}}` */ +export interface JobMap { + jobs: Record +} + +const str = (v: unknown): string | undefined => { + if (typeof v === "string") return v + if (Array.isArray(v)) return v.map(String).join(" ") + return undefined +} + +/** Parse a job map (JSON or YAML). Any document with a top-level `jobs` map + * works, so a benchmark `case.yaml` can be passed as is. */ +export function parseJobMap(text: string, source = "job map"): JobMap | { error: string } { + let doc: unknown + try { + doc = YAML.parse(text) + } catch (e) { + return { error: `${source}: not JSON or YAML (${e instanceof Error ? e.message : String(e)})` } + } + const jobs = (doc as { jobs?: unknown } | null)?.jobs + if (!jobs || typeof jobs !== "object" || Array.isArray(jobs)) + return { error: `${source}: expected {"jobs": {name: {"select", "exclude", "schedule"}}}` } + const out: JobMap = { jobs: {} } + for (const [name, raw] of Object.entries(jobs as Record)) { + if (!raw || typeof raw !== "object") return { error: `${source}: job ${name} is not an object` } + const j = raw as Record + const def: JobDef = {} + const select = str(j.select) + const exclude = str(j.exclude) + const selector = str(j.selector) + const schedule = str(j.schedule ?? j.cron) + if (select?.trim()) def.select = select.trim() + if (exclude?.trim()) def.exclude = exclude.trim() + if (selector?.trim()) def.selector = selector.trim() + if (schedule?.trim()) def.schedule = schedule.trim() + if (Array.isArray(j.models)) def.models = j.models.map(String) + if (!def.select && !def.selector && !def.models) + return { error: `${source}: job ${name} needs "select", "selector" or "models"` } + out.jobs[name] = def + } + if (!Object.keys(out.jobs).length) return { error: `${source}: no jobs` } + return out +} + +/** Read a `--jobs` file. */ +export function readJobMap(file: string): JobMap | { error: string } { + const abs = path.resolve(file) + if (!existsSync(abs)) return { error: `--jobs file not found: ${abs}` } + return parseJobMap(readFileSync(abs, "utf-8"), abs) +} + +/** + * Adapter: one job per selector in the project's `selectors.yml`. Schedules + * come from a `schedules.json` next to it when present (`{selector: cron}` or + * `{selector: {cron}}`); otherwise they are unknown. + */ +export function jobsFromSelectors(projectDir: string): JobMap | { error: string } { + const file = ["selectors.yml", "selectors.yaml"].map((f) => path.join(projectDir, f)).find((f) => existsSync(f)) + if (!file) return { error: `--jobs-from selectors: no selectors.yml in ${projectDir}` } + let doc: { selectors?: Array<{ name?: unknown }> } + try { + doc = YAML.parse(readFileSync(file, "utf-8")) ?? {} + } catch (e) { + return { error: `${file}: ${e instanceof Error ? e.message : String(e)}` } + } + const names = (doc.selectors ?? []).map((s) => s?.name).filter((n): n is string => typeof n === "string") + if (!names.length) return { error: `${file}: no selectors` } + const schedules = readSchedules(projectDir) + const out: JobMap = { jobs: {} } + for (const n of names) { + out.jobs[n] = { selector: n } + if (schedules[n]) out.jobs[n].schedule = schedules[n] + } + return out +} + +function readSchedules(projectDir: string): Record { + const file = path.join(projectDir, "schedules.json") + if (!existsSync(file)) return {} + try { + const raw = JSON.parse(readFileSync(file, "utf-8")) as Record + const out: Record = {} + for (const [k, v] of Object.entries(raw)) { + const cron = typeof v === "string" ? v : (v as { cron?: unknown } | null)?.cron + if (typeof cron === "string") out[k] = cron + } + return out + } catch { + return {} + } +} + +export type LsFn = typeof dbtLs + +/** Resolve every job without `models` with `dbt ls`. A job that fails to + * resolve keeps no models (the engine then ignores it) and is reported. */ +export async function resolveJobMap( + map: JobMap, + opts: DbtOfflineOptions, + ls: LsFn = dbtLs, +): Promise<{ map: JobMap; errors: string[] }> { + const errors: string[] = [] + const out: JobMap = { jobs: {} } + for (const [name, def] of Object.entries(map.jobs)) { + if (def.models) { + out.jobs[name] = def + continue + } + const r = await ls({ select: def.select, exclude: def.exclude, selector: def.selector }, opts) + if (r.ok) out.jobs[name] = { ...def, models: r.ids } + else { + errors.push(`job ${name}: dbt ls failed: ${r.error}`) + out.jobs[name] = def + } + } + return { map: out, errors } +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-impact.ts b/packages/opencode/src/cli/cmd/dbt-impact.ts new file mode 100644 index 0000000000..7ecb8b49ae --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-impact.ts @@ -0,0 +1,422 @@ +// altimate_change start — dbt-impact: blast radius of a model/column change, +// per hop, past the dbt graph (loaders, served tables) when a sink map is given. +import type { Argv } from "yargs" +import { cmd } from "./cmd" +import { Dispatcher } from "../../altimate/native" +import { existsSync, mkdtempSync, rmSync, statSync } from "fs" +import { tmpdir } from "os" +import path from "path" +import { collectChangedFiles, gitRepoRoot, makeContentResolver } from "../../altimate/review/git" +import { findModel, loadManifest, type Manifest } from "./dbt-mart-helpers" +import { + CHANGE_KINDS, + type ChangeKind, + type DbtChange, + type ImpactReport, + type ImpactVerdict, + VERDICT_RANK, + buildChange, + changesForFile, + formatImpactText, + normalizeImpactReport, + reportVerdict, +} from "./dbt-impact-helpers" +import { changesFromManifests, compileSides } from "./dbt-impact-compiled" +import { type JobMap, jobsFromSelectors, readJobMap, resolveJobMap } from "./dbt-impact-jobs" + +/** + * `altimate dbt-impact --model X [--column c] [--kind drop|…]` — what breaks + * downstream if X (or X.c) changes this way, hop by hop, with a verdict per + * consumer. `--base/--head` derives the changes from a git diff instead. + */ +export const DbtImpactCommand = cmd({ + command: "dbt-impact", + describe: "show what breaks downstream (dbt models, loaders, served tables) if a model or column changes", + builder: (yargs: Argv) => + yargs + .option("model", { describe: "model name (or unique_id)", type: "string" }) + .option("column", { describe: "column of --model (--kind add: the new column)", type: "string" }) + .option("kind", { describe: "kind of change", choices: CHANGE_KINDS, default: "drop" as ChangeKind }) + .option("new-name", { describe: "--kind rename: the new column name", type: "string" }) + .option("to-type", { describe: "--kind retype: the new type", type: "string" }) + .option("from-type", { describe: "--kind retype: the current type", type: "string" }) + .option("to", { describe: "--kind materialization: the new materialization", type: "string" }) + .option("strategy", { + describe: "--kind materialization: the new incremental_strategy (append, merge, delete+insert, …; default append)", + type: "string", + }) + .option("unique-key", { + describe: "--kind materialization: the new unique_key column(s); default: the model's current one", + type: "string", + array: true, + }) + .option("exposure-sinks", { + describe: + "without --sinks, derive the sink map from exposures' meta.sink (--no-exposure-sinks: loaders unknown). " + + "That map is partial: a mart without an exposure has unknown loaders unless the dbt var sink_map_complete is true", + type: "boolean", + default: true, + }) + .option("base", { describe: "diff mode: base git ref", type: "string" }) + .option("head", { describe: "diff mode: head git ref (default: working tree)", type: "string" }) + .option("base-manifest", { describe: "diff mode: compiled base manifest.json (skips compiling base)", type: "string" }) + .option("head-manifest", { describe: "diff mode: compiled head manifest.json (skips compiling head)", type: "string" }) + .option("compile", { + describe: "diff mode: compile both sides with dbt (--no-compile: naive Jinja render of the raw SQL)", + type: "boolean", + default: true, + }) + .option("jobs", { + describe: 'job map (JSON/YAML): {"jobs": {name: {"select", "exclude", "schedule"}}}, resolved with dbt ls', + type: "string", + }) + .option("jobs-from", { + describe: "derive the job map: selectors = one job per selectors.yml selector (+ schedules.json cron)", + choices: ["selectors"] as const, + }) + .option("live", { + describe: "read deployed columns of lagging producers / sources over the configured warehouse connection", + type: "boolean", + default: false, + }) + .option("warehouse", { describe: "--live: warehouse connection name (default: the first configured)", type: "string" }) + .option("dbt-bin", { describe: "dbt executable (default $ALTIMATE_DBT_BIN, else dbt)", type: "string" }) + .option("profiles-dir", { describe: "dbt profiles dir for compile / ls", type: "string" }) + .option("dbt-target", { describe: "dbt target for compile / ls", type: "string" }) + .option("sinks", { describe: "sink map JSON: loaders / served tables that read each mart", type: "string" }) + .option("target-ddl", { + describe: "backend CREATE TABLE file(s) (Postgres / ClickHouse) giving served column types; repeatable", + type: "string", + array: true, + }) + .option("cwd", { describe: "dbt project directory (default: current dir)", type: "string" }) + .option("manifest", { describe: "path to dbt manifest.json", type: "string" }) + .option("fail-on", { + describe: "exit 1 when the worst verdict reaches this level", + choices: ["none", "risky", "breaking"] as const, + default: "none" as const, + }) + .option("json", { describe: "emit the full result as JSON", type: "boolean", default: false }) + .check((a) => { + const manifests = !!(a["base-manifest"] && a["head-manifest"]) + if (!a.model && !a.base && !manifests) + throw new Error("pass --model X, or --base (or --base-manifest + --head-manifest) for diff mode") + if (a.model && (a.base || manifests)) throw new Error("--model and diff mode are mutually exclusive") + if (a.head && !a.base) throw new Error("--head needs --base") + if (!!a["base-manifest"] !== !!a["head-manifest"]) throw new Error("--base-manifest and --head-manifest go together") + if (a.jobs && a["jobs-from"]) throw new Error("--jobs and --jobs-from are mutually exclusive") + return true + }), + + handler: async (args: ImpactArgs) => { + const projectDir = path.resolve(args.cwd ?? process.cwd()) + const dbt = { dbtBin: args["dbt-bin"], profilesDir: args["profiles-dir"], target: args["dbt-target"] } + const options: Record = {} + if (args.sinks) { + const abs = path.resolve(args.sinks) + if (!existsSync(abs) || !statSync(abs).isFile()) return fatal(`--sinks file not found: ${abs}`) + options.sinks = abs + } + // Without --sinks the engine derives the sink map from exposures' meta.sink. + if (args["exposure-sinks"] === false) options.sinks_from_exposures = false + // Served column types let a narrowing be judged against what the target + // already stores, instead of assuming every narrowing breaks. + if (args["target-ddl"]?.length) { + const files = args["target-ddl"].map((p) => path.resolve(p)) + const missing = files.find((f) => !existsSync(f) || !statSync(f).isFile()) + if (missing) return fatal(`--target-ddl file not found: ${missing}`) + options.target_ddl = files + } + + // 1. The changes to analyse. + let changes: DbtChange[] = [] + let unclassified: Array<{ model: string; reason: string }> = [] + const notes: string[] = [] + let headManifest: Manifest | undefined + let headDir = projectDir + let cleanup = () => {} + if (args.model) { + const loaded = loadManifest(projectDir, args.manifest) + if (!loaded.ok) return fatal(loaded.error) + headManifest = loaded.manifest + if (args.manifest) options.manifest = loaded.path + const node = findModel(loaded.manifest, args.model) + if (!node) return fatal(`model not found (or ambiguous) in the manifest: ${args.model}`) + const change = buildChange({ + model: node.name ?? args.model, + column: args.column, + kind: args.kind ?? "drop", + newName: args["new-name"], + toType: args["to-type"], + fromType: args["from-type"], + to: args.to, + strategy: args.strategy, + uniqueKey: args["unique-key"], + }) + if ("error" in change) return fatal(change.error) + changes = [change] + } else if (args["base-manifest"] && args["head-manifest"]) { + const b = loadManifest(projectDir, args["base-manifest"]) + const h = loadManifest(projectDir, args["head-manifest"]) + if (!b.ok) return fatal(b.error) + if (!h.ok) return fatal(h.error) + headManifest = h.manifest + options.manifest = h.path + options.base_manifest = b.path + ;({ changes, unclassified } = changesFromManifests(b.manifest, h.manifest)) + } else { + const files = await changedModelFiles(projectDir, args.base!, args.head) + if ("error" in files) return fatal(files.error) + if (args.compile === false || !files.models.length) { + const derived = await changesFromDiffNaive(files, args.base!, args.head) + changes = derived.changes + unclassified = derived.unclassified + } else { + const sides = await compileSides({ + projectDir, + gitRoot: files.root, + base: args.base!, + head: args.head, + changed: files.models, + dbt, + }) + cleanup = sides.cleanup + notes.push(...sides.notes) + headDir = sides.headDir + const b = sides.baseManifest ? loadManifest(projectDir, sides.baseManifest) : undefined + const h = sides.headManifest ? loadManifest(projectDir, sides.headManifest) : undefined + if (b?.ok && h?.ok) { + headManifest = h.manifest + options.manifest = h.path + options.base_manifest = b.path + ;({ changes, unclassified } = changesFromManifests(b.manifest, h.manifest, new Set(files.models))) + } else { + notes.push("no compiled manifests: falling back to the naive render of the raw SQL") + const derived = await changesFromDiffNaive(files, args.base!, args.head) + changes = derived.changes + unclassified = derived.unclassified + } + } + } + + try { + // 2. Which job builds what (cross-job schema lag). + let jobs: JobMap | undefined + if (args.jobs || args["jobs-from"]) { + const map = args.jobs ? readJobMap(args.jobs) : jobsFromSelectors(headDir) + if ("error" in map) return fatal(map.error) + const lsTmp = mkdtempSync(path.join(tmpdir(), "dbt-impact-ls-")) + try { + const profilesDir = + dbt.profilesDir ?? (existsSync(path.join(projectDir, "profiles.yml")) ? projectDir : undefined) + const r = await resolveJobMap(map, { + projectDir: headDir, + ...dbt, + profilesDir, + targetPath: path.join(lsTmp, "target"), + logPath: path.join(lsTmp, "logs"), + }) + jobs = r.map + notes.push(...r.errors) + } finally { + rmSync(lsTmp, { recursive: true, force: true }) + } + options.jobs = jobs + } + + // 3. Impact per change. + const analyse = async (opts: Record) => { + const out: Array<{ change: DbtChange; report: ImpactReport }> = [] + for (const change of changes) { + const res = await Dispatcher.call("altimate_core.dbt_impact", { + project_dir: projectDir, + change_json: JSON.stringify(change), + options_json: Object.keys(opts).length ? JSON.stringify(opts) : undefined, + }).catch((e: unknown) => ({ success: false, data: {} as Record, error: String(e) })) + if (!res.success) return { error: res.error ?? "altimate_core.dbt_impact failed" } + out.push({ change, report: normalizeImpactReport(res.data) }) + } + return { results: out } + } + let run = await analyse(options) + if ("error" in run) return fatal(run.error!) + // 4. --live: read the deployed columns of every lagging producer / source + // and re-judge with them. + if (args.live && headManifest) { + const live = await deployedColumns(run.results, headManifest, args.warehouse) + notes.push(...live.notes) + if (Object.keys(live.columns).length) { + const again = await analyse({ ...options, deployed_columns: live.columns }) + if ("error" in again) return fatal(again.error!) + run = again + } + } else if (args.live) notes.push("--live: no manifest to resolve relation names from; skipped") + const results = run.results + const worst = results + .map((r) => reportVerdict(r.report)) + .reduce((w, v) => (VERDICT_RANK[v] > VERDICT_RANK[w] ? v : w), "safe") + const jobSummary = jobs + ? Object.fromEntries(Object.entries(jobs.jobs).map(([k, v]) => [k, { schedule: v.schedule, models: v.models?.length ?? 0 }])) + : undefined + + if (args.json) { + process.stdout.write( + JSON.stringify({ version: 1, worst, results, unclassified, notes, jobs: jobSummary }, null, 2) + "\n", + ) + } else { + const out: string[] = [] + if (!changes.length) out.push("No impact-relevant model changes in the diff.") + for (const r of results) out.push(formatImpactText(r.change, r.report), "") + for (const u of unclassified) out.push(`UNCLASSIFIED ${u.model}: ${u.reason} — run with --model ${u.model} --column --kind `) + for (const n of notes) out.push(`Note: ${n}`) + if (results.length > 1) out.push(`Worst across ${results.length} change(s): ${worst.toUpperCase()}`) + console.error(out.join("\n")) + } + const failOn = args["fail-on"] ?? "none" + if ((failOn === "breaking" && worst === "breaking") || (failOn === "risky" && (worst === "breaking" || worst === "risky"))) + process.exitCode = 1 + } finally { + cleanup() + } + }, +}) + +interface ImpactArgs { + model?: string + column?: string + kind?: ChangeKind + "new-name"?: string + "to-type"?: string + "from-type"?: string + to?: string + strategy?: string + "unique-key"?: string[] + "exposure-sinks"?: boolean + base?: string + head?: string + "base-manifest"?: string + "head-manifest"?: string + compile?: boolean + jobs?: string + "jobs-from"?: "selectors" + live?: boolean + warehouse?: string + "dbt-bin"?: string + "profiles-dir"?: string + "dbt-target"?: string + sinks?: string + "target-ddl"?: string[] + cwd?: string + manifest?: string + "fail-on"?: "none" | "risky" | "breaking" + json?: boolean +} + +interface ChangedModelFiles { + root: string + files: Awaited> + /** Model names touched by the diff (old and new names of a rename). */ + models: string[] +} + +async function changedModelFiles(projectDir: string, base: string, head: string | undefined): Promise { + const root = await gitRepoRoot(projectDir) + if (!root) return { error: `${projectDir} is not inside a git repository` } + let files + try { + files = await collectChangedFiles({ base, head, cwd: root }) + } catch (e) { + return { error: `git diff failed: ${e instanceof Error ? e.message : String(e)}` } + } + const relProject = path.relative(root, projectDir) + const models = files.filter( + (f) => f.path.endsWith(".sql") && (relProject ? f.path.startsWith(relProject + "/") : true) && /(^|\/)models\//.test(f.path), + ) + const names = new Set() + for (const f of models) { + names.add(path.basename(f.path, ".sql")) + if (f.oldPath) names.add(path.basename(f.oldPath, ".sql")) + } + return { root, files: models, models: [...names] } +} + +/** The naive diff mode: render raw Jinja just enough (ref/source/config). */ +async function changesFromDiffNaive( + changed: ChangedModelFiles, + base: string, + head: string | undefined, +): Promise<{ changes: DbtChange[]; unclassified: Array<{ model: string; reason: string }> }> { + const { root, files: models } = changed + const renames = new Map(models.filter((f) => f.oldPath).map((f) => [f.path, f.oldPath!])) + const read = makeContentResolver({ base, head, cwd: root, gitRoot: root, renames }) + const changes: DbtChange[] = [] + const unclassified: Array<{ model: string; reason: string }> = [] + for (const f of models) { + const model = path.basename(f.oldPath && f.status === "renamed" ? f.oldPath : f.path, ".sql") + const r = changesForFile({ + model, + status: f.status, + baseRaw: await read(f.path, "old"), + headRaw: f.status === "deleted" ? undefined : await read(f.path, "new"), + }) + changes.push(...r.changes) + unclassified.push(...r.unclassified) + } + return { changes, unclassified } +} + +/** The relation a schema-lag producer names: a model, or `source_name.table`. */ +export function producerRelation(m: Manifest, producer: string): { database?: string; schema?: string; table: string; relation: string } | undefined { + const all = [...Object.values(m.nodes ?? {}), ...Object.values((m.sources ?? {}) as Record>)] + const hit = all.find((n) => { + const r = n as Record + if (r.resource_type === "source") return `${r.source_name}.${r.name}` === producer + return r.resource_type === "model" && r.name === producer + }) as Record | undefined + if (!hit || typeof hit.relation_name !== "string") return undefined + const table = String(hit.identifier ?? hit.alias ?? hit.name) + return { + database: typeof hit.database === "string" ? hit.database : undefined, + schema: typeof hit.schema === "string" ? hit.schema : undefined, + table, + relation: hit.relation_name.replace(/["`]/g, "").toLowerCase(), + } +} + +/** Deployed columns of every producer named by a schema-lag finding, read + * over the configured connection (`schema.inspect`). */ +async function deployedColumns( + results: Array<{ report: ImpactReport }>, + manifest: Manifest, + warehouse?: string, +): Promise<{ columns: Record; notes: string[] }> { + const columns: Record = {} + const notes: string[] = [] + const producers = new Set(results.flatMap((r) => r.report.schema_lag.map((f) => f.producer))) + for (const p of producers) { + const rel = producerRelation(manifest, p) + if (!rel) { + notes.push(`--live: no relation for ${p} in the manifest`) + continue + } + const r = (await Dispatcher.call("schema.inspect", { + table: rel.table, + schema_name: rel.schema, + database: rel.database, + warehouse, + }).catch((e: unknown) => ({ error: String(e) }))) as { columns?: Array<{ name: string }>; error?: string } + if (r.error || !Array.isArray(r.columns)) { + notes.push(`--live: cannot read ${rel.relation}: ${r.error ?? "no columns returned"}`) + continue + } + columns[rel.relation] = r.columns.map((c) => c.name.toLowerCase()) + } + if (!producers.size) notes.push("--live: no schema-lag producer to check") + return { columns, notes } +} + +function fatal(msg: string): void { + console.error(`Error: ${msg}`) + process.exitCode = 1 +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-mart-helpers.ts b/packages/opencode/src/cli/cmd/dbt-mart-helpers.ts new file mode 100644 index 0000000000..0aa9908f76 --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-mart-helpers.ts @@ -0,0 +1,346 @@ +// altimate_change start — dbt-mart-helpers: shared pure helpers for the dbt mart +// optimizer commands (dbt-optimize, dbt-parity, dbt-impact, dbt-factor). +// Manifest lookup, proof normalization, and the compiled-SQL → raw-model mapper +// that decides whether an engine rewrite can be written back into a dbt model +// without losing its Jinja. No yargs import, so all of it is unit-testable. +import { existsSync, readFileSync, statSync } from "fs" +import path from "path" +import { createTwoFilesPatch } from "diff" + +// --------------------------------------------------------------------------- +// Manifest +// --------------------------------------------------------------------------- + +export interface ManifestNode { + unique_id?: string + name?: string + resource_type?: string + original_file_path?: string + patch_path?: string | null + raw_code?: string + compiled_code?: string | null + relation_name?: string | null + config?: Record + depends_on?: { nodes?: string[] } + [key: string]: unknown +} + +export interface Manifest { + nodes?: Record + child_map?: Record + [key: string]: unknown +} + +export type ManifestLoad = { ok: true; manifest: Manifest; path: string } | { ok: false; error: string } + +/** Resolve and parse the manifest. An explicit path must exist; otherwise the + * conventional `/target/manifest.json` is required — these commands + * need the model's file path and compiled SQL, which only the manifest has. */ +export function loadManifest(projectDir: string, manifestArg?: string): ManifestLoad { + const p = manifestArg ? path.resolve(manifestArg) : path.join(projectDir, "target", "manifest.json") + if (!existsSync(p) || !statSync(p).isFile()) + return { + ok: false, + error: manifestArg + ? `manifest not found: ${p}` + : `no manifest at ${p} — run \`dbt compile\` (not just \`dbt parse\`: the compiled SQL is needed) or pass --manifest`, + } + try { + return { ok: true, manifest: JSON.parse(readFileSync(p, "utf-8")) as Manifest, path: p } + } catch (e) { + return { ok: false, error: `cannot read manifest ${p}: ${e instanceof Error ? e.message : String(e)}` } + } +} + +/** A model node by name or unique_id. Ambiguous names (two packages) resolve to + * the root-project candidate only when exactly one is not from a dependency. */ +export function findModel(manifest: Manifest, name: string): ManifestNode | undefined { + const nodes = Object.values(manifest.nodes ?? {}).filter((n) => n.resource_type === "model") + const byId = nodes.find((n) => n.unique_id === name) + if (byId) return byId + const matches = nodes.filter((n) => n.name === name) + return matches.length === 1 ? matches[0] : undefined +} + +/** Direct downstream models of a node (from `child_map`, models only). */ +export function downstreamModels(manifest: Manifest, uniqueId: string): ManifestNode[] { + const kids = manifest.child_map?.[uniqueId] ?? [] + return kids + .map((id) => manifest.nodes?.[id]) + .filter((n): n is ManifestNode => !!n && n.resource_type === "model") +} + +// --------------------------------------------------------------------------- +// Proofs +// --------------------------------------------------------------------------- + +export type ProofStatus = "proven" | "refuted" | "undecidable" | "invalid_sql" | "unknown" + +export interface ProofDifference { + aspect?: string + description?: string + severity?: string +} + +export interface NormalizedProof { + status: ProofStatus + confidence?: number + differences: ProofDifference[] + reason?: string +} + +/** The engine tags its proof enum as `verdict` today; the plan spoke of + * `kind`. Accept both, and a bare string. An unrecognized value is + * `unknown`, which every caller treats like `undecidable` — never as proven. */ +export function normalizeProof(p: unknown): NormalizedProof { + if (typeof p === "string") return { status: proofStatus(p), differences: [] } + const o = (p && typeof p === "object" ? p : {}) as Record + const status = proofStatus(o.verdict ?? o.kind ?? o.status) + return { + status, + confidence: typeof o.confidence === "number" ? o.confidence : undefined, + differences: Array.isArray(o.differences) ? (o.differences as ProofDifference[]) : [], + reason: typeof o.reason === "string" ? o.reason : undefined, + } +} + +function proofStatus(v: unknown): ProofStatus { + const s = typeof v === "string" ? v.toLowerCase() : "" + if (s === "proven" || s === "refuted" || s === "undecidable" || s === "invalid_sql") return s + return "unknown" +} + +/** One line for a proof: status plus the evidence that status needs. */ +export function formatProof(p: NormalizedProof): string { + switch (p.status) { + case "proven": + return `PROVEN equivalent${p.confidence !== undefined ? ` (confidence ${p.confidence})` : ""}` + case "refuted": { + const d = p.differences.map((x) => `${x.aspect ? `${x.aspect}: ` : ""}${x.description ?? ""}`).filter(Boolean) + return `REFUTED — ${d.length ? d.join("; ") : "differences not stated"}` + } + case "invalid_sql": + return `INVALID SQL — ${p.reason ?? "the rewrite does not bind"} (never applicable)` + case "undecidable": + return `UNDECIDABLE — ${p.reason ?? "no reason given"} (a data parity run is the only remaining bar)` + default: + return "UNKNOWN proof status (treated as undecidable)" + } +} + +// --------------------------------------------------------------------------- +// Compiled SQL → raw model mapping +// --------------------------------------------------------------------------- + +/** A Jinja expression in the raw model and what it rendered to. */ +export interface JinjaBinding { + jinja: string + rendered: string +} + +export type Alignment = + | { ok: true; header: string; bindings: JinjaBinding[]; droppedJinjaComments: boolean } + | { ok: false; reason: string } + +const EXPR = /\{\{[\s\S]*?\}\}/g +const REF_LIKE = /^\{\{-?\s*(ref|source)\s*\(/ +const CONFIG = /^\{\{-?\s*config\s*\(/ +const THIS = /^\{\{-?\s*this\s*-?\}\}$/ + +function escapeRegex(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") +} + +/** A literal raw-model segment as a regex that tolerates whitespace changes. */ +function literalPattern(seg: string): string { + return seg + .split(/\s+/) + .filter((t) => t.length) + .map(escapeRegex) + .join("\\s+") +} + +/** + * Align a raw dbt model with its compiled SQL. Succeeds only when the raw + * model's Jinja is limited to `config()` (renders to nothing), `ref()`, + * `source()` and `this` — i.e. when the compiled SQL IS the raw body with + * relation names substituted. Anything else (control flow, macros, `var()`) + * means the compiled text was produced by code a rewrite cannot be mapped + * back through, and the alignment refuses with the reason. + */ +export function alignRawToCompiled(raw: string, compiled: string): Alignment { + const statement = raw.match(/\{%-?\s*(\w+)/) + if (statement) + return { + ok: false, + reason: `the model uses Jinja control flow (\`{% ${statement[1]} … %}\`), so its compiled SQL is one branch of a template — a rewrite of it cannot be written back over the template safely`, + } + const src = raw.replace(/\{#[\s\S]*?#\}/g, "") + + const exprs = [...src.matchAll(EXPR)] + let header = "" + const parts: string[] = [] + const captureExprs: string[] = [] + let last = 0 + for (const m of exprs) { + const e = m[0] + const lit = src.slice(last, m.index) + if (lit.trim()) parts.push(literalPattern(lit)) + if (CONFIG.test(e)) { + if (captureExprs.length || src.slice(0, m.index).trim()) + return { ok: false, reason: "the `{{ config() }}` block is not at the top of the model" } + // The header is taken from the RAW text so Jinja comments above the + // config block survive. + header = raw.slice(0, raw.indexOf(e) + e.length) + } else if (REF_LIKE.test(e) || THIS.test(e)) { + parts.push("(.+?)") + captureExprs.push(e) + } else { + return { + ok: false, + reason: `the model renders \`${e.length > 60 ? e.slice(0, 57) + "...}}" : e}\`, which is not a ref/source/config — its compiled output cannot be mapped back to the template`, + } + } + last = (m.index ?? 0) + e.length + } + const tail = src.slice(last) + if (tail.trim()) parts.push(literalPattern(tail)) + + const re = new RegExp("^\\s*" + parts.join("\\s*") + "\\s*$") + const m = compiled.match(re) + if (!m) + return { + ok: false, + reason: + "the compiled SQL does not line up with the raw model — a stale target/ (re-run `dbt compile`), or a ref dbt renders as more than a relation name (microbatch filtering)", + } + const bindings: JinjaBinding[] = [] + for (const [i, jinja] of captureExprs.entries()) { + const rendered = m[i + 1].trim() + const prior = bindings.find((b) => b.rendered === rendered) + // Two different Jinja expressions rendering to the same relation would make + // the reverse mapping ambiguous. + if (prior && normJinja(prior.jinja) !== normJinja(jinja)) + return { ok: false, reason: `two different refs render to ${rendered}; the reverse mapping is ambiguous` } + if (!prior) bindings.push({ jinja, rendered }) + } + const body = raw.slice(header.length) + return { ok: true, header, bindings, droppedJinjaComments: /\{#[\s\S]*?#\}/.test(body) } +} + +function normJinja(s: string): string { + return s.replace(/\s+/g, "").replace(/"/g, "'") +} + +/** Identifier parts of a rendered relation, honouring double quotes. */ +export function relationParts(rendered: string): string[] { + const out: string[] = [] + const re = /"((?:[^"]|"")*)"|([^."\s]+)/g + for (const m of rendered.matchAll(re)) out.push(m[1] !== undefined ? m[1].replace(/""/g, '"') : m[2]) + return out +} + +/** A regex matching a relation with each part optionally quoted. */ +function relationRegex(rendered: string): RegExp { + const parts = relationParts(rendered) + const body = parts.map((p) => `"?${escapeRegex(p)}"?`).join("\\s*\\.\\s*") + return new RegExp(`(? b.rendered.length - a.rendered.length) + for (const b of ordered) body = body.replace(relationRegex(b.rendered), () => b.jinja) + // Anything still spelled as a schema-qualified relation of a binding was not + // mapped (e.g. re-quoted differently) and would hardcode an environment. + for (const b of align.bindings) { + const parts = relationParts(b.rendered) + if (parts.length < 2) continue + const tail = parts.slice(-2) + const re = new RegExp(`"?${escapeRegex(tail[0])}"?\\s*\\.\\s*"?${escapeRegex(tail[1])}"?(?![\\w"])`, "i") + if (re.test(body)) + return { + ok: false, + reason: `the rewrite references ${b.rendered} in a form that could not be mapped back to \`${b.jinja}\``, + } + } + const warnings: string[] = [] + const rawBody = input.raw.slice(input.raw.indexOf(align.header) + align.header.length) + if (/--|\/\*/.test(rawBody) && !/--|\/\*/.test(body)) + warnings.push("the engine's rewrite carries no SQL comments; the model's comments are dropped by this patch") + if (align.droppedJinjaComments) warnings.push("Jinja comments ({# … #}) are dropped by this patch") + const sql = (align.header ? align.header + "\n\n" : "") + body + "\n" + return { ok: true, sql, warnings } +} + +/** Map model-level SQL (a new model, or an edited consumer) onto Jinja: + * every rendered relation in `bindings` becomes its ref/source. */ +export function relationsToRefs(sql: string, bindings: JinjaBinding[]): string { + let out = sql + for (const b of [...bindings].sort((a, c) => c.rendered.length - a.rendered.length)) + out = out.replace(relationRegex(b.rendered), () => b.jinja) + return out +} + +// --------------------------------------------------------------------------- +// Patches + misc +// --------------------------------------------------------------------------- + +/** A unified diff of one file, `--- a/` / `+++ b/`. */ +export function unifiedPatch(file: string, before: string, after: string): string { + return createTwoFilesPatch( + before ? `a/${file}` : "/dev/null", + after ? `b/${file}` : "/dev/null", + before, + after, + undefined, + undefined, + { context: 3 }, + ) + .split("\n") + .filter((l) => !l.startsWith("=====")) + .join("\n") +} + +/** POSIX-shell quote, for the follow-up commands this CLI prints. */ +export function shellQuote(s: string): string { + return /^[\w@%+=:,./-]+$/.test(s) ? s : `'${s.replace(/'/g, `'\\''`)}'` +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-optimize-helpers.ts b/packages/opencode/src/cli/cmd/dbt-optimize-helpers.ts new file mode 100644 index 0000000000..d1dc12d2eb --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-optimize-helpers.ts @@ -0,0 +1,330 @@ +// altimate_change start — dbt-optimize-helpers: pure proposal handling for +// `altimate dbt-optimize` — normalization, the apply gate, patch building and +// text rendering. Split from the command so it is testable without yargs. +import path from "path" +import { + type ManifestNode, + type NormalizedProof, + alignRawToCompiled, + formatProof, + mapRewriteToRaw, + normalizeProof, + relationsToRefs, + shellQuote, + unifiedPatch, +} from "./dbt-mart-helpers" +import { type PerfFinding, isMaterializationChange } from "./dbt-perf-helpers" + +// --------------------------------------------------------------------------- +// Types — the engine's `RewriteProposal`, loose on purpose (cross-binary contract) +// --------------------------------------------------------------------------- + +export interface ModelLevelChange { + new_model_name?: string + new_model_sql?: string + consumer_sql?: string +} + +export interface RewriteProposal { + rule?: string + source?: string + before_sql?: string + after_sql?: string + ast_diff?: { identical?: boolean; diff_count?: number; diffs?: Array<{ change_type?: string; description?: string }> } | null + proof?: unknown + guardrail?: { kind?: string; [key: string]: unknown } | null + finding_code?: string | null + applicable?: boolean + parity_required?: boolean + model_level?: ModelLevelChange | null + cost?: CostAssessment | null + [key: string]: unknown +} + +/** The engine's cost gate (`perf::cost_gate`): whether a proven rewrite may be + * presented as a performance fix. */ +export interface CostAssessment { + verdict?: "possible_gain" | "unverified" | "break_even" | "readability_only" | string + perf_claim?: boolean + plan_identical?: boolean + extra_writes?: number + model_seconds?: number + floor_seconds?: number + reason?: string +} + +/** Proposals the cost gate says are not performance fixes. */ +export function notAPerfFix(p: RewriteProposal): boolean { + return p.cost?.verdict === "readability_only" || p.cost?.verdict === "break_even" +} + +export function formatCost(c: CostAssessment | null | undefined): string | undefined { + if (!c?.verdict) return undefined + const label: Record = { + possible_gain: "POSSIBLE GAIN", + unverified: "UNVERIFIED (not a measured win)", + break_even: "BREAK-EVEN (not proposed as a perf fix)", + readability_only: "READABILITY ONLY (same plan; not a perf fix)", + } + return `${label[c.verdict] ?? c.verdict.toUpperCase()}${c.reason ? ` — ${c.reason}` : ""}` +} + +export interface NormalizedProposal extends RewriteProposal { + index: number + proofNormalized: NormalizedProof +} + +export function normalizeProposals(raw: unknown): NormalizedProposal[] { + const list = Array.isArray(raw) ? (raw as RewriteProposal[]) : [] + return list.map((p, i) => ({ ...(p ?? {}), index: i + 1, proofNormalized: normalizeProof(p?.proof) })) +} + +/** Perf findings that belong to `model` (the engine names models by name or + * unique_id depending on the rule). */ +export function findingsForModel(findings: PerfFinding[], model: ManifestNode): PerfFinding[] { + const names = new Set([model.name, model.unique_id].filter(Boolean) as string[]) + return findings.filter((f) => (f.model && names.has(f.model)) || f.related_models?.some((m) => names.has(m))) +} + +// --------------------------------------------------------------------------- +// The apply gate +// --------------------------------------------------------------------------- + +export type ApplyDecision = + | { ok: true; proposal: NormalizedProposal; note?: string } + | { ok: false; reason: string } + +/** + * Pick the proposal `--apply` acts on and decide whether it may be applied. + * + * - `--proposal N` names one; otherwise the first PROVEN, applicable proposal, + * then (only with `--allow-unproven`) the first undecidable one. + * - A refuted proposal is never applied — the engine showed it changes results. + * - An undecidable (or unknown) proof needs `--allow-unproven`, and the parity + * command printed afterwards is then the only bar left. + */ +export function chooseProposal( + proposals: NormalizedProposal[], + opts: { index?: number; allowUnproven: boolean }, +): ApplyDecision { + if (!proposals.length) return { ok: false, reason: "the engine proposed no rewrites for this model" } + let p: NormalizedProposal | undefined + if (opts.index !== undefined) { + p = proposals.find((x) => x.index === opts.index) + if (!p) return { ok: false, reason: `no proposal #${opts.index} (there are ${proposals.length})` } + } else { + // A performance fix first; a proven readability-only / break-even rewrite + // is still a safe edit, taken only when nothing better is proven, and the + // result says it is not a performance fix. + const isProven = (x: NormalizedProposal) => x.proofNormalized.status === "proven" && x.applicable !== false + const perf = proposals.filter((x) => !notAPerfFix(x)) + p = + perf.find(isProven) ?? + (opts.allowUnproven + ? perf.find((x) => x.proofNormalized.status !== "refuted" && x.applicable !== false) + : undefined) + if (!p) { + const fallback = proposals.find((x) => notAPerfFix(x) && isProven(x)) + if (fallback) + return { + ok: true, + proposal: fallback, + note: `proposal #${fallback.index} (${fallback.rule ?? "?"}) is ${String(fallback.cost?.verdict).toUpperCase()}: a safe edit, not a performance fix — no proven performance fix was found`, + } + } + if (!p) + return { + ok: false, + reason: proposals.some((x) => x.proofNormalized.status !== "refuted") + ? "no proposal is PROVEN equivalent; re-run with --allow-unproven to apply an undecidable one (then run the parity check)" + : "every proposal was REFUTED by the equivalence engine — none is safe to apply", + } + } + if (p.proofNormalized.status === "refuted" || p.applicable === false) + return { ok: false, reason: `proposal #${p.index} (${p.rule ?? "?"}) is REFUTED — it changes the model's results` } + if (p.proofNormalized.status !== "proven" && !opts.allowUnproven) + return { + ok: false, + reason: `proposal #${p.index} (${p.rule ?? "?"}) is ${p.proofNormalized.status.toUpperCase()}; pass --allow-unproven to apply it, then run the parity check`, + } + return { ok: true, proposal: p } +} + +// --------------------------------------------------------------------------- +// Patches +// --------------------------------------------------------------------------- + +export interface FileEdit { + /** Project-relative path. */ + file: string + before: string + after: string +} + +export type PatchPlan = + | { ok: true; edits: FileEdit[]; patch: string; warnings: string[] } + | { ok: false; reason: string; patch: string } + +/** + * Build the file edits for a proposal against the model's raw file. + * + * Statement-level proposals rewrite the model's compiled SQL; they are mapped + * back onto the raw file only when {@link mapRewriteToRaw} says that is safe. + * Model-level proposals add `.sql` beside the model and edit + * the model to read it; both go through the same relation → ref mapping, plus + * a binding for the new model itself. + * + * On refusal, `patch` is still the compiled-SQL diff so the reader can apply it + * by hand. + */ +export function buildPatchPlan(input: { model: ManifestNode; raw: string; proposal: RewriteProposal }): PatchPlan { + const { model, raw, proposal } = input + const file = model.original_file_path ?? `${model.name}.sql` + const compiled = model.compiled_code ?? "" + const before = proposal.before_sql ?? compiled + const ml = proposal.model_level + if (!compiled) + return { + ok: false, + reason: "the manifest has no compiled SQL for this model — run `dbt compile`", + patch: unifiedPatch(file + " (compiled)", before, proposal.after_sql ?? ""), + } + + if (ml && ml.new_model_name && ml.new_model_sql && ml.consumer_sql) { + const compiledPatch = + unifiedPatch(path.join(path.dirname(file), `${ml.new_model_name}.sql`) + " (compiled)", "", ml.new_model_sql) + + unifiedPatch(file + " (compiled)", before, ml.consumer_sql) + const consumer = mapConsumerSql({ model, raw, consumerSql: ml.consumer_sql, newModelName: ml.new_model_name }) + if (!consumer.ok) return { ok: false, reason: consumer.reason, patch: compiledPatch } + const newSql = consumer.newModelSql(ml.new_model_sql) + const consumerAfter = consumer.sql + const newFile = path.join(path.dirname(file), `${ml.new_model_name}.sql`) + const edits: FileEdit[] = [ + { file: newFile, before: "", after: newSql }, + { file, before: raw, after: consumerAfter }, + ] + return { + ok: true, + edits, + patch: edits.map((e) => unifiedPatch(e.file, e.before, e.after)).join(""), + warnings: [ + `${ml.new_model_name} gets no {{ config() }} block and inherits the folder's materialization — set one before building`, + ], + } + } + + const after = proposal.after_sql ?? "" + const compiledPatch = unifiedPatch(file + " (compiled)", before, after) + const mapped = mapRewriteToRaw({ raw, compiled, beforeSql: before, afterSql: after }) + if (!mapped.ok) return { ok: false, reason: mapped.reason, patch: compiledPatch } + const edits = [{ file, before: raw, after: mapped.sql }] + return { ok: true, edits, patch: unifiedPatch(file, raw, mapped.sql), warnings: mapped.warnings } +} + +export type ConsumerMapping = + | { ok: true; sql: string; newModelSql: (sql: string) => string } + | { ok: false; reason: string } + +/** + * Map an engine-edited consumer (compiled SQL that now reads a NEW model) back + * onto the consumer's raw file: its existing relations become their refs, the + * new model — spelled as a relation in the consumer's schema, or bare — + * becomes `{{ ref('') }}`, and the consumer's `{{ config() }}` block is + * kept. `newModelSql` maps the new model's own SQL with the same bindings. + */ +export function mapConsumerSql(input: { + model: ManifestNode + raw: string + consumerSql: string + newModelName: string +}): ConsumerMapping { + const { model, raw, consumerSql, newModelName } = input + const align = alignRawToCompiled(raw, model.compiled_code ?? "") + if (!align.ok) return align + const relParts = (model.relation_name ?? "").split(".") + const newRendered = relParts.length > 1 ? [...relParts.slice(0, -1), `"${newModelName}"`].join(".") : "" + const newRef = `{{ ref('${newModelName}') }}` + const bindings = [...align.bindings] + if (newRendered) bindings.push({ jinja: newRef, rendered: newRendered }) + let consumer = relationsToRefs(consumerSql, bindings) + consumer = consumer.replace( + new RegExp(`(\\bfrom|\\bjoin)(\\s+)"?${newModelName}"?(?![\\w."])`, "gi"), + `$1$2${newRef}`, + ) + if (!consumer.includes(newRef)) + return { ok: false, reason: `the edited consumer does not visibly read ${newModelName}; refusing to guess where its ref goes` } + return { + ok: true, + sql: (align.header ? align.header + "\n\n" : "") + consumer.trim() + "\n", + newModelSql: (sql) => relationsToRefs(sql, align.bindings).trim() + "\n", + } +} + +/** The follow-up parity command. `before` is the model's current relation; + * `after` is the relation the rebuilt model lands in, which only the caller + * knows — a placeholder unless `--parity-after` supplied it. */ +export function parityCommand(model: ManifestNode, after?: string): string { + const before = model.relation_name ?? `` + return [ + "altimate dbt-parity", + `--model ${shellQuote(model.name ?? "")}`, + `--before ${shellQuote(before.replace(/"/g, ""))}`, + `--after ${after ? shellQuote(after) : ""}`, + ].join(" ") +} + +// --------------------------------------------------------------------------- +// Text rendering +// --------------------------------------------------------------------------- + +function guardrailLines(g: RewriteProposal["guardrail"]): string[] { + if (!g || !g.kind || g.kind === "none") return [] + if (isMaterializationChange(g.kind)) { + const v = (x: unknown) => (x === undefined || x === null ? "(not stated — verify manually)" : JSON.stringify(x)) + return [ + ` GUARDRAIL MaterializationChange: ${String(g.from ?? "?")} → ${String(g.to ?? "?")}`, + ` delete scope: ${v(g.delete_scope)}`, + ` reader requirements: ${v(g.reader_requirements)}`, + ] + } + const rest = Object.entries(g) + .filter(([k]) => k !== "kind") + .map(([k, x]) => `${k}=${typeof x === "string" ? x : JSON.stringify(x)}`) + return [` guardrail: ${g.kind}${rest.length ? ` (${rest.join(", ")})` : ""}`] +} + +const AST_DESC_MAX = 120 + +/** The AST diff as counts per change type plus a few short examples. The + * engine's descriptions can be whole-subtree dumps, so each is cut to one + * readable line. */ +export function astDiffLines(d: NonNullable): string[] { + const diffs = Array.isArray(d.diffs) ? d.diffs : [] + if (d.identical) return [" AST diff: identical"] + const byType = new Map() + for (const x of diffs) byType.set(x.change_type ?? "?", (byType.get(x.change_type ?? "?") ?? 0) + 1) + const counts = [...byType.entries()].map(([k, v]) => `${v} ${k}`).join(", ") + const lines = [` AST diff: ${d.diff_count ?? diffs.length} change(s)${counts ? ` (${counts})` : ""}`] + for (const x of diffs.slice(0, 3)) { + const desc = (x.description ?? "").replace(/\s+/g, " ") + lines.push(` - ${x.change_type ?? "?"}: ${desc.length > AST_DESC_MAX ? desc.slice(0, AST_DESC_MAX - 1) + "…" : desc}`) + } + return lines +} + +export function formatProposal(p: NormalizedProposal): string[] { + const lines: string[] = [] + const tag = p.model_level ? " [model-level]" : "" + const finding = p.finding_code ? ` for ${p.finding_code}` : "" + lines.push(`#${p.index} ${p.rule ?? "(unnamed rule)"}${tag} — source: ${p.source ?? "?"}${finding}`) + lines.push(` proof: ${formatProof(p.proofNormalized)}`) + const cost = formatCost(p.cost) + if (cost) lines.push(` cost: ${cost}`) + const d = p.ast_diff + if (d && typeof d === "object") lines.push(...astDiffLines(d)) + lines.push(...guardrailLines(p.guardrail)) + if (p.parity_required) lines.push(" parity required: yes — only a data parity run can confirm this one") + if (p.model_level?.new_model_name) lines.push(` new model: ${p.model_level.new_model_name}`) + return lines +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-optimize.ts b/packages/opencode/src/cli/cmd/dbt-optimize.ts new file mode 100644 index 0000000000..c36eb35b7b --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-optimize.ts @@ -0,0 +1,197 @@ +// altimate_change start — dbt-optimize: verified rewrite proposals for one dbt +// model, with an apply path that refuses refuted rewrites and Jinja it cannot +// map back. Deterministic: engine calls only, no LLM, no warehouse. +import type { Argv } from "yargs" +import { cmd } from "./cmd" +import { Dispatcher } from "../../altimate/native" +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs" +import path from "path" +import { findModel, loadManifest } from "./dbt-mart-helpers" +import { normalizePerfReport, normalizePerfSeverity } from "./dbt-perf-helpers" +import { + buildPatchPlan, + chooseProposal, + findingsForModel, + formatProposal, + normalizeProposals, + parityCommand, +} from "./dbt-optimize-helpers" + +/** + * `altimate dbt-optimize --model X` — perf findings for X, then the engine's + * rewrite proposals for X, each with its static equivalence proof. + * + * `--apply` writes the chosen proposal back into the model file. It refuses a + * refuted proposal, needs `--allow-unproven` for an undecidable one, and + * refuses (printing the patch instead) whenever the compiled rewrite cannot be + * mapped back onto the raw model's Jinja safely. + */ +export const DbtOptimizeCommand = cmd({ + command: "dbt-optimize", + describe: "propose verified SQL rewrites for one dbt model, and optionally apply one (deterministic, no LLM)", + builder: (yargs: Argv) => + yargs + .option("model", { describe: "model name (or unique_id)", type: "string", demandOption: true }) + .option("cwd", { describe: "dbt project directory (default: current dir)", type: "string" }) + .option("manifest", { + describe: "path to dbt manifest.json (default: /target/manifest.json)", + type: "string", + }) + .option("apply", { + describe: "write the chosen proposal into the model file (refuses refuted / unmappable proposals)", + type: "boolean", + default: false, + }) + .option("proposal", { describe: "1-based proposal number to apply (default: first proven)", type: "number" }) + .option("allow-unproven", { + describe: "allow --apply on an UNDECIDABLE proof (a parity run is then the only bar)", + type: "boolean", + default: false, + }) + .option("parity-after", { + describe: "relation the rebuilt model lands in, for the printed dbt-parity command", + type: "string", + }) + .option("run-results", { + describe: "dbt run_results.json — measured model time, the cost gate's size signal (drops break-even rewrites)", + type: "string", + }) + .option("trino-profile", { + describe: "trino-profile.json — the same size signal from a Trino build profile", + type: "string", + }) + .option("json", { describe: "emit the full result as JSON", type: "boolean", default: false }), + + handler: async (args: { + model: string + cwd?: string + manifest?: string + apply?: boolean + proposal?: number + "allow-unproven"?: boolean + "parity-after"?: string + "run-results"?: string + "trino-profile"?: string + json?: boolean + }) => { + const projectDir = path.resolve(args.cwd ?? process.cwd()) + const loaded = loadManifest(projectDir, args.manifest) + if (!loaded.ok) return fatal(loaded.error) + const model = findModel(loaded.manifest, args.model) + if (!model) return fatal(`model not found (or ambiguous) in the manifest: ${args.model}`) + + // 1. Perf findings for the model — the context the proposals answer. + const perf = await Dispatcher.call("altimate_core.dbt_perf", { + project_dir: projectDir, + manifest_json: readFileSync(loaded.path, "utf-8"), + }).catch((e: unknown) => ({ success: false, data: {} as Record, error: String(e) })) + const findings = perf.success ? findingsForModel(normalizePerfReport(perf.data).findings, model) : [] + const perfError = perf.success ? undefined : perf.error + + // 2. Rewrite proposals. + const res = await Dispatcher.call("altimate_core.dbt_rewrites", { + project_dir: projectDir, + model: model.name ?? args.model, + // The structural diff is the reviewer's "what changed" summary. + // Without run artifacts every plan-changing rewrite is `cost: unverified`. + options_json: JSON.stringify({ + ast_diff: true, + ...(args["run-results"] ? { run_results: path.resolve(args["run-results"]) } : {}), + ...(args["trino-profile"] ? { trino_profile: path.resolve(args["trino-profile"]) } : {}), + }), + }).catch((e: unknown) => ({ success: false, data: {} as Record, error: String(e) })) + if (!res.success) return fatal(res.error ?? "altimate_core.dbt_rewrites failed") + const proposals = normalizeProposals(res.data.proposals) + + // 3. Apply gate + patch. + const allowUnproven = args["allow-unproven"] ?? false + const decision = args.apply ? chooseProposal(proposals, { index: args.proposal, allowUnproven }) : undefined + const filePath = path.join(projectDir, model.original_file_path ?? "") + const raw = existsSync(filePath) ? readFileSync(filePath, "utf-8") : (model.raw_code ?? "") + const plan = + decision?.ok === true ? buildPatchPlan({ model, raw, proposal: decision.proposal }) : undefined + let applied: string[] = [] + if (plan?.ok) { + for (const e of plan.edits) { + const abs = path.join(projectDir, e.file) + if (!e.before && existsSync(abs)) { + // A model-level proposal must not overwrite an existing model. + return fatal(`refusing to overwrite existing file ${e.file}`) + } + mkdirSync(path.dirname(abs), { recursive: true }) + writeFileSync(abs, e.after) + } + applied = plan.edits.map((e) => e.file) + } + const nextCommand = parityCommand(model, args["parity-after"]) + + if (args.json) { + process.stdout.write( + JSON.stringify( + { + version: 1, + model: model.name, + file: model.original_file_path, + findings, + perf_error: perfError, + proposals: proposals.map(({ proofNormalized, ...p }) => ({ ...p, proof_status: proofNormalized.status })), + apply: args.apply + ? { + chosen: decision?.ok ? decision.proposal.index : undefined, + refused: decision && !decision.ok ? decision.reason : plan && !plan.ok ? plan.reason : undefined, + applied, + warnings: [...(decision?.ok && decision.note ? [decision.note] : []), ...(plan?.ok ? plan.warnings : [])], + patch: plan?.patch, + } + : undefined, + parity_command: nextCommand, + }, + null, + 2, + ) + "\n", + ) + } else { + const out: string[] = [] + out.push(`dbt-optimize ${model.name} (${model.original_file_path ?? "?"})`) + out.push("") + if (perfError) out.push(`Perf findings unavailable: ${perfError}`) + else if (!findings.length) out.push("Perf findings: none for this model.") + else { + out.push(`Perf findings (${findings.length}):`) + for (const f of findings) + out.push(` ${normalizePerfSeverity(f.severity).toUpperCase()} [${f.code ?? f.rule ?? "?"}] ${f.message ?? ""}`) + } + out.push("") + if (!proposals.length) out.push("No rewrite proposals.") + else { + out.push(`Rewrite proposals (${proposals.length}):`) + for (const p of proposals) out.push(...formatProposal(p)) + } + if (args.apply) { + out.push("") + if (decision && !decision.ok) out.push(`NOT APPLIED: ${decision.reason}`) + else if (plan && !plan.ok) { + out.push(`NOT APPLIED: ${plan.reason}`) + out.push("The rewrite, against the compiled SQL — apply by hand if it still makes sense:") + out.push(plan.patch) + } else if (plan?.ok) { + out.push(`APPLIED proposal #${decision!.ok ? decision!.proposal.index : "?"} to: ${applied.join(", ")}`) + if (decision?.ok && decision.note) out.push(` note: ${decision.note}`) + for (const w of plan.warnings) out.push(` warning: ${w}`) + out.push(plan.patch) + } + } + out.push("") + out.push("Next — rebuild the model, then prove the data did not change:") + out.push(` ${nextCommand}`) + console.error(out.join("\n")) + } + if (args.apply && !(plan?.ok ?? false)) process.exitCode = 1 + }, +}) + +function fatal(msg: string): void { + console.error(`Error: ${msg}`) + process.exitCode = 1 +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-parity-helpers.ts b/packages/opencode/src/cli/cmd/dbt-parity-helpers.ts new file mode 100644 index 0000000000..5c9c1dc138 --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-parity-helpers.ts @@ -0,0 +1,273 @@ +// altimate_change start — dbt-parity-helpers: run the engine's ParityPlan over an +// executor and judge it. Pure apart from the injected executor, so the whole +// pass/fail logic is testable with a fake warehouse. + +// --------------------------------------------------------------------------- +// Types — the engine's `ParityPlan` +// --------------------------------------------------------------------------- + +export interface ParitySqlTask { + id: string + table_side?: string + sql: string + expected_shape?: string +} + +export interface ParityPlanTask { + kind: string + task: ParitySqlTask + pass_when: string +} + +export interface ParityPlan { + model?: string + before?: string + after?: string + grain_keys?: string[] + grain_source?: string + columns?: string[] + tasks: ParityPlanTask[] + notes?: string[] +} + +export function normalizeParityPlan(data: unknown): ParityPlan { + const d = (data ?? {}) as Record + const tasks = Array.isArray(d.tasks) ? (d.tasks as ParityPlanTask[]).filter((t) => t?.task?.sql) : [] + return { ...(d as Partial), tasks } +} + +/** Task kinds whose result rows are ROW VALUES, not aggregates. They can carry + * PII, so they only run with an explicit `--sample-rows` (data-parity skill rule). */ +export const ROW_VALUE_KINDS = new Set(["except_sample", "join_diff"]) + +export interface QueryResult { + columns: string[] + rows: (string | null)[][] +} + +export type Executor = (sql: string, task: ParityPlanTask) => Promise + +export type TaskStatus = "pass" | "fail" | "skipped" | "error" | "unevaluated" + +export interface TaskOutcome { + id: string + kind: string + pass_when: string + status: TaskStatus + /** First row as column → value, for single-row tasks. */ + values?: Record + /** Row-set tasks: the rows (only when sampling was allowed). */ + columns?: string[] + rows?: (string | null)[][] + row_count?: number + detail?: string +} + +export type ParityVerdict = "match" | "mismatch" | "inconclusive" + +export interface ParityResult { + verdict: ParityVerdict + plan: ParityPlan + outcomes: TaskOutcome[] + summary: ParitySummary +} + +export interface ParitySummary { + row_count_before?: number + row_count_after?: number + row_delta?: number + duplicated_keys_before?: number + duplicated_keys_after?: number + except_before_minus_after?: number + except_after_minus_before?: number + column_match_rates?: Array<{ column: string; matches: number; joined_rows: number; rate: number }> +} + +// --------------------------------------------------------------------------- +// Execution +// --------------------------------------------------------------------------- + +function toNum(v: string | null | undefined): number | undefined { + if (v === null || v === undefined || v.trim() === "") return undefined + const n = Number(v) + return Number.isFinite(n) ? n : undefined +} + +/** Run every task. Row-value tasks are skipped unless `sampleRows > 0`; a + * failing query is an `error` outcome, never a silent pass. */ +export async function runParityPlan( + plan: ParityPlan, + exec: Executor, + opts: { sampleRows: number }, +): Promise { + const outcomes: TaskOutcome[] = [] + for (const t of plan.tasks) { + const base = { id: t.task.id, kind: t.kind, pass_when: t.pass_when } + if (ROW_VALUE_KINDS.has(t.kind) && opts.sampleRows <= 0) { + outcomes.push({ + ...base, + status: "skipped", + detail: "returns row values — pass --sample-rows N to run it (rows may contain PII)", + }) + continue + } + try { + const r = await exec(t.task.sql, t) + const first = r.rows[0] + const values = first ? Object.fromEntries(r.columns.map((c, i) => [c.toLowerCase(), first[i] ?? null])) : {} + const rowSet = t.task.expected_shape === "RowSet" || ROW_VALUE_KINDS.has(t.kind) + outcomes.push({ + ...base, + status: "unevaluated", + values: rowSet ? undefined : values, + columns: rowSet ? r.columns : undefined, + rows: rowSet ? r.rows.slice(0, Math.max(opts.sampleRows, 0)) : undefined, + row_count: r.rows.length, + }) + } catch (e) { + outcomes.push({ ...base, status: "error", detail: e instanceof Error ? e.message : String(e) }) + } + } + evaluate(outcomes) + return { verdict: verdictOf(outcomes), plan, outcomes, summary: summarize(outcomes) } +} + +/** Resolve a name in a `pass_when` expression: the task's own column first, + * then another task's id (its first column). */ +function resolve(name: string, own: TaskOutcome, all: TaskOutcome[]): number | undefined { + const n = name.trim().toLowerCase() + if (/^-?\d+(\.\d+)?$/.test(n)) return Number(n) + if (own.values && n in own.values) return toNum(own.values[n]) + const other = all.find((o) => o.id.toLowerCase() === n && o.values) + if (other?.values) return toNum(Object.values(other.values)[0]) + return undefined +} + +/** Judge each executed outcome against its `pass_when`. The forms the engine + * emits are `a = b`, `no rows`, and `every __matches = joined_rows`; + * anything else stays `unevaluated`, which makes the verdict inconclusive. */ +export function evaluate(outcomes: TaskOutcome[]): void { + for (const o of outcomes) { + if (o.status !== "unevaluated") continue + const rule = o.pass_when.trim() + if (/^no rows$/i.test(rule)) { + o.status = (o.row_count ?? 0) === 0 ? "pass" : "fail" + continue + } + const every = rule.match(/^every\s+__matches\s*=\s*(\w+)$/i) + if (every) { + const total = toNum(o.values?.[every[1].toLowerCase()]) + const cols = Object.entries(o.values ?? {}).filter(([k]) => k.endsWith("__matches")) + if (total === undefined || !cols.length) { + o.detail = `cannot evaluate "${rule}"` + continue + } + o.status = cols.every(([, v]) => toNum(v) === total) ? "pass" : "fail" + continue + } + const eq = rule.match(/^([\w.]+)\s*(=|==)\s*([\w.]+)$/) + if (eq) { + const a = resolve(eq[1], o, outcomes) + const b = resolve(eq[3], o, outcomes) + if (a === undefined || b === undefined) { + o.detail = `cannot evaluate "${rule}" (missing value)` + continue + } + o.status = a === b ? "pass" : "fail" + continue + } + o.detail = `unrecognized pass condition "${rule}"` + } +} + +export function verdictOf(outcomes: TaskOutcome[]): ParityVerdict { + if (outcomes.some((o) => o.status === "fail")) return "mismatch" + if (outcomes.some((o) => o.status === "error" || o.status === "unevaluated")) return "inconclusive" + if (!outcomes.some((o) => o.status === "pass")) return "inconclusive" + return "match" +} + +function first(outcomes: TaskOutcome[], id: string): number | undefined { + const o = outcomes.find((x) => x.id === id) + return o?.values ? toNum(Object.values(o.values)[0]) : undefined +} + +export function summarize(outcomes: TaskOutcome[]): ParitySummary { + const s: ParitySummary = { + row_count_before: first(outcomes, "row_count_before"), + row_count_after: first(outcomes, "row_count_after"), + duplicated_keys_before: first(outcomes, "grain_duplicates_before"), + duplicated_keys_after: first(outcomes, "grain_duplicates_after"), + except_before_minus_after: first(outcomes, "except_count_before_minus_after"), + except_after_minus_before: first(outcomes, "except_count_after_minus_before"), + } + if (s.row_count_before !== undefined && s.row_count_after !== undefined) + s.row_delta = s.row_count_after - s.row_count_before + const rates = outcomes.find((o) => o.kind === "column_match_rates") + if (rates?.values) { + const joined = toNum(rates.values["joined_rows"]) ?? 0 + s.column_match_rates = Object.entries(rates.values) + .filter(([k]) => k.endsWith("__matches")) + .map(([k, v]) => { + const matches = toNum(v) ?? 0 + return { column: k.slice(0, -"__matches".length), matches, joined_rows: joined, rate: joined ? matches / joined : 1 } + }) + } + return s +} + +// --------------------------------------------------------------------------- +// Text +// --------------------------------------------------------------------------- + +const n = (v: number | undefined) => (v === undefined ? "?" : v.toLocaleString("en-US")) + +export function formatParityText(r: ParityResult, opts: { sampleRows: number }): string { + const { plan, summary: s } = r + const lines: string[] = [] + lines.push(`dbt-parity ${plan.model ?? "?"}: ${plan.before ?? "?"} (before) vs ${plan.after ?? "?"} (after)`) + lines.push( + `Grain: ${plan.grain_keys?.length ? plan.grain_keys.join(", ") : "(none)"} — source: ${plan.grain_source ?? "?"}`, + ) + lines.push("") + lines.push( + `Rows: before ${n(s.row_count_before)}, after ${n(s.row_count_after)}` + + (s.row_delta !== undefined ? `, delta ${s.row_delta >= 0 ? "+" : ""}${n(s.row_delta)}` : ""), + ) + if (s.duplicated_keys_before !== undefined || s.duplicated_keys_after !== undefined) + lines.push(`Duplicated grain keys: before ${n(s.duplicated_keys_before)}, after ${n(s.duplicated_keys_after)}`) + lines.push( + `EXCEPT: before−after ${n(s.except_before_minus_after)}, after−before ${n(s.except_after_minus_before)}`, + ) + if (s.column_match_rates?.length) { + lines.push("Column match rates (rows joined on the grain):") + for (const c of s.column_match_rates) + lines.push(` ${c.column}: ${(c.rate * 100).toFixed(4)}% (${n(c.matches)}/${n(c.joined_rows)})`) + } + lines.push("") + lines.push("Checks:") + for (const o of r.outcomes) { + const extra = o.detail ? ` — ${o.detail}` : "" + lines.push(` ${o.status.toUpperCase().padEnd(11)} ${o.id} (pass when: ${o.pass_when})${extra}`) + if (o.rows?.length) { + lines.push(` ${o.columns?.join(" | ") ?? ""}`) + for (const row of o.rows) lines.push(` ${row.map((v) => (v === null ? "NULL" : v)).join(" | ")}`) + } + } + if (opts.sampleRows <= 0 && r.outcomes.some((o) => o.status === "skipped")) + lines.push(" (row samples not fetched: they may contain PII — opt in with --sample-rows N)") + if (plan.notes?.length) { + lines.push("") + for (const note of plan.notes) lines.push(`Note: ${note}`) + } + lines.push("") + lines.push( + r.verdict === "match" + ? "PARITY: MATCH" + : r.verdict === "mismatch" + ? "PARITY: MISMATCH" + : "PARITY: INCONCLUSIVE (a check errored or could not be evaluated — not a pass)", + ) + return lines.join("\n") +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-parity.ts b/packages/opencode/src/cli/cmd/dbt-parity.ts new file mode 100644 index 0000000000..71ceb41d2b --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-parity.ts @@ -0,0 +1,117 @@ +// altimate_change start — dbt-parity: run the engine's data parity plan for a +// model change over an existing warehouse connection and judge it. +import type { Argv } from "yargs" +import { cmd } from "./cmd" +import { Dispatcher } from "../../altimate/native" +import * as Registry from "../../altimate/native/connections/registry" +import { warehouseTypeToDialect } from "../../altimate/native/connections/data-diff" +import path from "path" +import { findModel, loadManifest } from "./dbt-mart-helpers" +import { type Executor, formatParityText, normalizeParityPlan, runParityPlan } from "./dbt-parity-helpers" + +/** + * `altimate dbt-parity --model X --before --after `. + * + * The engine plans the checks (row counts, grain uniqueness, EXCEPT both ways, + * per-column match rates, key-joined diff); this command runs them over a + * warehouse connection and exits non-zero unless every check passes. Queries + * returning ROW VALUES run only with an explicit `--sample-rows N`, because + * those rows end up in the terminal (and in an agent's context). + */ +export const DbtParityCommand = cmd({ + command: "dbt-parity", + describe: "prove a rebuilt dbt model returns the same data as before (runs the engine's parity plan)", + builder: (yargs: Argv) => + yargs + .option("model", { describe: "model name (or unique_id)", type: "string", demandOption: true }) + .option("before", { describe: "relation holding the model BEFORE the change", type: "string", demandOption: true }) + .option("after", { describe: "relation holding the model AFTER the change", type: "string", demandOption: true }) + .option("connection", { describe: "warehouse connection name (default: the first configured)", type: "string" }) + .option("columns", { + describe: "comma-separated columns to compare (enables per-column match rates)", + type: "string", + }) + .option("sample-rows", { + describe: "fetch up to N differing rows each way (row values — may contain PII; default 0 = none)", + type: "number", + default: 0, + }) + .option("cwd", { describe: "dbt project directory (default: current dir)", type: "string" }) + .option("manifest", { describe: "path to dbt manifest.json", type: "string" }) + .option("json", { describe: "emit the full result as JSON", type: "boolean", default: false }), + + handler: async (args: { + model: string + before: string + after: string + connection?: string + columns?: string + "sample-rows"?: number + cwd?: string + manifest?: string + json?: boolean + }) => { + const projectDir = path.resolve(args.cwd ?? process.cwd()) + const loaded = loadManifest(projectDir, args.manifest) + if (!loaded.ok) return fatal(loaded.error) + const model = findModel(loaded.manifest, args.model) + if (!model) return fatal(`model not found (or ambiguous) in the manifest: ${args.model}`) + + const connection = args.connection ?? Registry.list().warehouses[0]?.name + if (!connection) + return fatal("no warehouse connection configured — add one (warehouse.add / ALTIMATE_CODE_CONN_) or pass --connection") + const type = Registry.getConfig(connection)?.type + if (!type) return fatal(`connection "${connection}" not found`) + const dialect = warehouseTypeToDialect(String(type)) + + const sampleRows = Math.max(0, Math.floor(args["sample-rows"] ?? 0)) + const options: Record = { dialect } + if (sampleRows > 0) options.sample_rows = sampleRows + const columns = (args.columns ?? "") + .split(",") + .map((c) => c.trim()) + .filter(Boolean) + if (columns.length) options.columns = columns + + const res = await Dispatcher.call("altimate_core.dbt_parity_plan", { + project_dir: projectDir, + model: model.name ?? args.model, + before: args.before, + after: args.after, + options_json: JSON.stringify(options), + }).catch((e: unknown) => ({ success: false, data: {} as Record, error: String(e) })) + if (!res.success) return fatal(res.error ?? "altimate_core.dbt_parity_plan failed") + const plan = normalizeParityPlan(res.data) + if (!plan.tasks.length) return fatal("the engine returned a parity plan with no tasks") + + let connector: Awaited> + try { + connector = await Registry.get(connection) + } catch (e) { + return fatal(`cannot connect to "${connection}": ${e instanceof Error ? e.message : String(e)}`) + } + // Complete result sets: a driver-default LIMIT would turn a count into a + // sample and make a mismatch look like a match. + const exec: Executor = async (sql) => { + const r = await connector.execute(sql, undefined, undefined, { noLimit: true }) + return { + columns: r.columns, + rows: r.rows.map((row: unknown[]) => row.map((v) => (v === null || v === undefined ? null : String(v)))), + } + } + const result = await runParityPlan(plan, exec, { sampleRows }) + + if (args.json) { + process.stdout.write(JSON.stringify({ version: 1, connection, dialect, ...result }, null, 2) + "\n") + } else { + console.error(formatParityText(result, { sampleRows })) + } + if (result.verdict !== "match") process.exitCode = 1 + }, +}) + +function fatal(msg: string): void { + console.error(`Error: ${msg}`) + process.exitCode = 1 +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-perf-helpers.ts b/packages/opencode/src/cli/cmd/dbt-perf-helpers.ts new file mode 100644 index 0000000000..6e5a6ebf4f --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-perf-helpers.ts @@ -0,0 +1,369 @@ +// altimate_change start — dbt-perf-helpers: pure normalization + text rendering +// for the deterministic `altimate dbt-perf` command. Exported separately from +// the command so they can be unit-tested without importing the yargs module +// (same split as check-helpers.ts). + +// --------------------------------------------------------------------------- +// Types — the engine's `PerfReport`, as the napi binding serializes it. +// Everything is optional/loose on purpose: this is a cross-binary contract and +// a field the engine adds or renames must not crash the CLI. +// --------------------------------------------------------------------------- + +export type PerfSeverity = "info" | "warning" | "error" + +/** A guardrail attached to a finding. `MaterializationChange` is the one this + * command renders in full: a past perf "fix" that switched a materialization + * without deleting the old relation caused a data incident, which is why + * `delete_scope` and `reader_requirements` exist at all. */ +export interface PerfGuardrail { + kind?: string + /** What must be dropped/rebuilt before the change is safe. */ + delete_scope?: unknown + /** What downstream readers must be able to tolerate. */ + reader_requirements?: unknown + [key: string]: unknown +} + +export interface PerfFinding { + code?: string + rule?: string + family?: string + scope?: string + severity?: string + model?: string + related_models?: string[] + message?: string + suggestion?: string + evidence?: unknown[] + inputs_used?: string[] + confidence?: unknown + cost?: unknown + guardrail?: PerfGuardrail + tension?: unknown + blast_radius?: unknown + suppression?: unknown + [key: string]: unknown +} + +export interface PerfSkippedRule { + code?: string + name?: string + reason?: string +} + +export interface PerfSummary { + models_analyzed?: number + rules_run?: number + rules_skipped?: number + findings_total?: number + had_compiled_sql?: boolean + had_sink_map?: boolean + had_run_results?: boolean + had_trino_profile?: boolean + /** Models whose compiled SQL the engine could not parse: SQL-shaped rules + * produced nothing for them, so they were not read, not found clean. */ + parse_failures?: number + parse_failed_models?: string[] + [key: string]: unknown +} + +/** The engine's `CostEvidence`, measured from run_results / a Trino profile. */ +export interface PerfCost { + exec_time_s?: number | null + exec_share?: number | null + peak_memory_bytes?: number | null + physical_input_bytes?: number | null + on_critical_path?: boolean + baseline_delta_pct?: number | null + [key: string]: unknown +} + +export type PerfSortOrder = "rule" | "cost" + + +export interface PerfReport { + findings: PerfFinding[] + skipped_rules: PerfSkippedRule[] + summary: PerfSummary +} + +/** The engine's `optionsJson`. Unknown keys are REJECTED by the binding, so + * only keys that were actually supplied are emitted. */ +export interface PerfOptions { + dialect?: string + sinks?: string + run_results?: string + trino_profile?: string + include_experimental?: boolean + sinks_from_exposures?: boolean +} + +export function buildPerfOptions(input: { + dialect?: string + sinks?: string + runResults?: string + trinoProfile?: string + experimental?: boolean + /** `false` stops the engine deriving the sink map from exposures' `meta.sink`. */ + exposureSinks?: boolean +}): PerfOptions { + const o: PerfOptions = {} + if (input.dialect) o.dialect = input.dialect + if (input.sinks) o.sinks = input.sinks + if (input.runResults) o.run_results = input.runResults + if (input.trinoProfile) o.trino_profile = input.trinoProfile + if (input.experimental) o.include_experimental = true + // Sent only when off: an older core rejects the unknown key. + if (input.exposureSinks === false) o.sinks_from_exposures = false + return o +} + +export const PERF_SEVERITY_RANK: Record = { error: 2, warning: 1, info: 0 } + +// --------------------------------------------------------------------------- +// Normalization +// --------------------------------------------------------------------------- + +/** Coerce an engine severity to the CLI's three levels. Unknown values become + * `warning` rather than `info`: an unrecognized severity must not slip under a + * `--fail-on warning` gate. */ +export function normalizePerfSeverity(s: unknown): PerfSeverity { + const lower = typeof s === "string" ? s.toLowerCase() : "" + if (lower === "error" || lower === "critical" || lower === "high" || lower === "fatal") return "error" + if (lower === "info" || lower === "low" || lower === "suggestion" || lower === "note") return "info" + return "warning" +} + +/** Normalize whatever the binding returned into a `PerfReport`. The engine owns + * this shape; a missing or oddly-typed section becomes empty rather than + * throwing, so a contract drift degrades to "reported nothing" (visible in the + * summary line) instead of a crash. */ +export function normalizePerfReport(data: unknown): PerfReport { + const d = (data ?? {}) as Record + const findings = Array.isArray(d.findings) ? (d.findings as PerfFinding[]) : [] + const skipped = Array.isArray(d.skipped_rules) ? (d.skipped_rules as PerfSkippedRule[]) : [] + const summary = d.summary && typeof d.summary === "object" ? (d.summary as PerfSummary) : {} + return { findings, skipped_rules: skipped, summary } +} + +export function filterPerfBySeverity(findings: PerfFinding[], min: PerfSeverity): PerfFinding[] { + const minRank = PERF_SEVERITY_RANK[min] + return findings.filter((f) => PERF_SEVERITY_RANK[normalizePerfSeverity(f.severity)] >= minRank) +} + +/** Exit gate. Computed on the UNFILTERED findings so `--severity` only controls + * what is printed, never whether CI fails (matching `altimate check`). */ +export function perfPass(findings: PerfFinding[], failOn: "none" | "warning" | "error"): boolean { + if (failOn === "none") return true + const worst = findings.map((f) => PERF_SEVERITY_RANK[normalizePerfSeverity(f.severity)]) + const errors = worst.filter((r) => r === 2).length + const warnings = worst.filter((r) => r === 1).length + if (failOn === "error") return errors === 0 + return errors === 0 && warnings === 0 +} + +// --------------------------------------------------------------------------- +// Text formatter +// --------------------------------------------------------------------------- + +/** Render a value that may be a string, an array of strings, or a nested + * object, on one line. Used for the guardrail fields, whose exact shape is the + * engine's to choose. */ +function inlineValue(v: unknown): string { + if (v === null || v === undefined) return "" + if (typeof v === "string") return v + if (Array.isArray(v)) return v.map(inlineValue).filter(Boolean).join(", ") + if (typeof v === "object") return JSON.stringify(v) + return String(v as string | number | boolean) +} + +/** True for the materialization-change guardrail in either serialization the + * engine has used (`MaterializationChange`, `materialization_change`). */ +export function isMaterializationChange(kind: unknown): boolean { + return typeof kind === "string" && kind.replace(/_/g, "").toLowerCase() === "materializationchange" +} + +function num(v: unknown): number | undefined { + return typeof v === "number" && Number.isFinite(v) ? v : undefined +} + +function bytes(n: number): string { + const units = ["B", "KB", "MB", "GB", "TB"] + let v = n + let i = 0 + while (v >= 1024 && i < units.length - 1) { + v /= 1024 + i++ + } + return `${v.toFixed(i === 0 ? 0 : 1)} ${units[i]}` +} + +/** Sort key for `--sort cost`: measured execution time first, then share of + * the run, then bytes read, then peak memory. A finding with no measured cost + * sorts last (-1), never as if it were free. */ +export function perfCostKey(f: PerfFinding): number[] { + const c = (f.cost && typeof f.cost === "object" ? f.cost : {}) as PerfCost + const keys = [num(c.exec_time_s), num(c.exec_share), num(c.physical_input_bytes), num(c.peak_memory_bytes)] + return keys.map((k) => k ?? -1) +} + +/** Order findings for display. `rule` keeps the engine's order (the text + * renderer groups by rule code); `cost` is most-expensive first, stable. */ +export function sortPerfFindings(findings: PerfFinding[], order: PerfSortOrder): PerfFinding[] { + if (order !== "cost") return findings + return findings + .map((f, i) => ({ f, i, k: perfCostKey(f) })) + .sort((a, b) => { + for (let j = 0; j < a.k.length; j++) if (a.k[j] !== b.k[j]) return b.k[j] - a.k[j] + return a.i - b.i + }) + .map((x) => x.f) +} + +/** One-line rendering of a `CostEvidence`. Falls back to the raw value when + * the engine sends a shape this CLI does not know. */ +export function formatPerfCost(cost: unknown): string { + if (!cost || typeof cost !== "object" || Array.isArray(cost)) return inlineValue(cost) + const c = cost as PerfCost + const parts: string[] = [] + const t = num(c.exec_time_s) + if (t !== undefined) { + const share = num(c.exec_share) + parts.push(`exec ${t.toFixed(1)}s${share !== undefined ? ` (${(share * 100).toFixed(1)}% of run)` : ""}`) + } + const input = num(c.physical_input_bytes) + if (input !== undefined) parts.push(`input ${bytes(input)}`) + const mem = num(c.peak_memory_bytes) + if (mem !== undefined) parts.push(`peak memory ${bytes(mem)}`) + if (c.on_critical_path) parts.push("on critical path") + const delta = num(c.baseline_delta_pct) + if (delta !== undefined) parts.push(`${delta >= 0 ? "+" : ""}${delta.toFixed(1)}% vs baseline`) + return parts.length ? parts.join(", ") : inlineValue(cost) +} + +/** Group key: the rule code, falling back to the rule name. */ +function groupKey(f: PerfFinding): string { + return f.code ?? f.rule ?? "unknown" +} + +export interface FormatPerfOptions { + /** The findings that survived `--severity`, for the body. */ + findings: PerfFinding[] + /** The full, unfiltered set — the summary must not lie about what was found. */ + allFindings: PerfFinding[] + skippedRules: PerfSkippedRule[] + summary: PerfSummary + pass: boolean + minSeverity: PerfSeverity + /** `cost` renders one flat list, most expensive first, instead of rule groups. */ + sort?: PerfSortOrder +} + +export function formatPerfText(opts: FormatPerfOptions): string { + const { summary, allFindings } = opts + const lines: string[] = [] + + const tally = { error: 0, warning: 0, info: 0 } + for (const f of allFindings) tally[normalizePerfSeverity(f.severity)]++ + + // 1. Summary first — everything below is only interpretable against it. + lines.push( + `Analyzed ${summary.models_analyzed ?? "?"} model(s): ` + + `${summary.rules_run ?? "?"} rule(s) run, ${summary.rules_skipped ?? opts.skippedRules.length} skipped — ` + + `${allFindings.length} finding(s) (${tally.error} error, ${tally.warning} warning, ${tally.info} info)`, + ) + // The two inputs that decide how much of the rule set could even run. A + // report with neither is a shallow one and should read as such. + const inputs: string[] = [] + if (summary.had_compiled_sql !== undefined) inputs.push(`compiled SQL: ${summary.had_compiled_sql ? "yes" : "NO"}`) + if (summary.had_sink_map !== undefined) inputs.push(`sink map: ${summary.had_sink_map ? "yes" : "NO"}`) + if (summary.had_run_results !== undefined) inputs.push(`run_results: ${summary.had_run_results ? "yes" : "NO"}`) + if (summary.had_trino_profile !== undefined) + inputs.push(`trino profile: ${summary.had_trino_profile ? "yes" : "NO"}`) + if (inputs.length) lines.push(`Inputs — ${inputs.join(", ")}`) + // A model whose SQL did not parse was not read; zero findings on it is not clean. + if (typeof summary.parse_failures === "number" && summary.parse_failures > 0) { + const named = summary.parse_failed_models?.length ? `: ${summary.parse_failed_models.join(", ")}` : "" + lines.push(`Parse failures — ${summary.parse_failures} model(s) not read by SQL rules${named}`) + } + lines.push("") + + // 2. Skipped rules, BEFORE the findings. A rule that could not run is not a + // clean result, and a reader who sees only findings will read it as one. + if (opts.skippedRules.length) { + lines.push(`--- RULES SKIPPED (${opts.skippedRules.length}) ---`) + for (const r of opts.skippedRules) { + const code = r.code ? `[${r.code}] ` : "" + lines.push(` ${code}${r.name ?? "(unnamed)"}: ${r.reason ?? "no reason given"}`) + } + lines.push("") + } + + // 3. Findings, grouped by rule code. + if (!opts.findings.length) { + lines.push( + allFindings.length + ? `No findings at or above "${opts.minSeverity}" (${allFindings.length} below the threshold).` + : "No findings.", + ) + } else { + const groups = new Map() + for (const f of opts.findings) { + const k = groupKey(f) + if (!groups.has(k)) groups.set(k, []) + groups.get(k)!.push(f) + } + if (opts.sort === "cost") { + lines.push(`--- FINDINGS BY COST — ${opts.findings.length} finding(s) ---`) + for (const f of opts.findings) renderFinding(lines, f, true) + lines.push("") + } else { + for (const [code, items] of [...groups.entries()].sort((a, b) => a[0].localeCompare(b[0]))) { + const rule = items[0].rule && items[0].rule !== code ? ` ${items[0].rule}` : "" + const family = items[0].family ? ` (${items[0].family})` : "" + lines.push(`--- ${code}${rule}${family} — ${items.length} finding(s) ---`) + for (const f of items) renderFinding(lines, f, false) + lines.push("") + } + } + } + + lines.push(opts.pass ? "PASS" : "FAIL") + return lines.join("\n") +} + +/** Render one finding. `withCode` prefixes the rule code, for the flat + * cost-ordered list where there is no group header to carry it. */ +function renderFinding(lines: string[], f: PerfFinding, withCode: boolean): void { + const sev = normalizePerfSeverity(f.severity).toUpperCase() + const code = withCode ? ` [${groupKey(f)}]` : "" + const model = f.model ? ` ${f.model}` : "" + const scope = f.scope ? ` <${f.scope}>` : "" + lines.push(` ${sev}${code}${model}${scope}: ${f.message ?? ""}`) + if (f.related_models?.length) lines.push(` related: ${f.related_models.join(", ")}`) + if (f.suggestion) lines.push(` suggestion: ${f.suggestion}`) + if (f.confidence !== undefined && f.confidence !== null) lines.push(` confidence: ${inlineValue(f.confidence)}`) + if (f.cost !== undefined && f.cost !== null) lines.push(` cost: ${formatPerfCost(f.cost)}`) + if (f.blast_radius !== undefined && f.blast_radius !== null) + lines.push(` blast radius: ${inlineValue(f.blast_radius)}`) + if (f.tension !== undefined && f.tension !== null) lines.push(` tension: ${inlineValue(f.tension)}`) + // A materialization change is the one guardrail whose omission has + // already caused a data incident: the old relation survives the switch + // and readers silently keep hitting it. Print both fields in full. + const g = f.guardrail + if (g) { + const kind = g.kind ?? "" + if (isMaterializationChange(kind)) { + lines.push(` GUARDRAIL MaterializationChange:`) + lines.push(` delete scope: ${inlineValue(g.delete_scope) || "(not stated — verify manually)"}`) + lines.push(` reader requirements: ${inlineValue(g.reader_requirements) || "(not stated — verify manually)"}`) + } else if (kind) { + lines.push(` guardrail: ${kind}`) + } + } + if (f.suppression !== undefined && f.suppression !== null) lines.push(` suppression: ${inlineValue(f.suppression)}`) + const ev = Array.isArray(f.evidence) ? f.evidence : [] + for (const e of ev) lines.push(` evidence: ${inlineValue(e)}`) + if (f.inputs_used?.length) lines.push(` inputs used: ${f.inputs_used.join(", ")}`) +} +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/dbt-perf.ts b/packages/opencode/src/cli/cmd/dbt-perf.ts new file mode 100644 index 0000000000..ba7be7915b --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-perf.ts @@ -0,0 +1,230 @@ +// altimate_change start — dbt-perf: deterministic dbt performance analysis CLI +// command (no LLM required). Sibling of `altimate check`: one engine call, one +// normalization pass, one renderer, an exit code. +import type { Argv } from "yargs" +import { cmd } from "./cmd" +import { Dispatcher } from "../../altimate/native" +import { existsSync, readFileSync, statSync } from "fs" +import path from "path" +import { + type PerfSeverity, + type PerfSortOrder, + buildPerfOptions, + filterPerfBySeverity, + sortPerfFindings, + formatPerfText, + normalizePerfReport, + perfPass, +} from "./dbt-perf-helpers" + +/** + * `altimate dbt-perf` — whole-project dbt performance analysis. + * + * Deterministic and offline: it reads the dbt manifest (and the compiled SQL + * next to it, when present) and runs the engine's perf rule set. No model + * calls, no warehouse connection, no cost. + * + * Scope note: this is a PROJECT command, like `altimate review --all` and + * unlike `altimate review`. It does not look at a diff. + */ +export const DbtPerfCommand = cmd({ + command: "dbt-perf", + describe: "analyze a dbt project for performance problems (deterministic, no LLM)", + builder: (yargs: Argv) => + yargs + .option("cwd", { + describe: "dbt project directory (default: current dir)", + type: "string", + }) + .option("manifest", { + describe: "path to dbt manifest.json (default: /target/manifest.json)", + type: "string", + }) + .option("format", { + describe: "output format", + choices: ["json", "text"] as const, + default: "text" as const, + }) + .option("severity", { + describe: "minimum severity to report", + choices: ["info", "warning", "error"] as const, + default: "info" as const, + }) + .option("fail-on", { + describe: "exit 1 if findings at this level or above are found (computed before --severity)", + choices: ["none", "warning", "error"] as const, + default: "none" as const, + }) + .option("run-results", { + describe: "dbt run_results.json — measured execution time per model (enables cost-ranked rules)", + type: "string", + }) + .option("trino-profile", { + describe: "Trino build profile JSON (peak memory, bytes read per model)", + type: "string", + }) + .option("sinks", { + describe: "sink map JSON: which loaders / served tables read each mart column beyond the dbt graph", + type: "string", + }) + .option("exposure-sinks", { + describe: + "without --sinks, derive the sink map from exposures' meta.sink (--no-exposure-sinks: loaders unknown)", + type: "boolean", + default: true, + }) + .option("experimental", { + describe: "also run rules held out of the default report", + type: "boolean", + default: false, + }) + .option("sort", { + describe: "finding order: grouped by rule, or most expensive first (needs --run-results or --trino-profile)", + choices: ["rule", "cost"] as const, + default: "rule" as const, + }), + + handler: async (args: { + cwd?: string + manifest?: string + format?: "json" | "text" + severity?: PerfSeverity + "fail-on"?: "none" | "warning" | "error" + failOn?: "none" | "warning" | "error" + "run-results"?: string + "trino-profile"?: string + sinks?: string + "exposure-sinks"?: boolean + experimental?: boolean + sort?: PerfSortOrder + }) => { + const startTime = Date.now() + const projectDir = path.resolve(args.cwd ?? process.cwd()) + + // Resolve the manifest. An explicit --manifest must exist — silently + // falling back to the project scan would analyze something the caller + // didn't ask for. Without the flag, the conventional path is used when it + // is there, and otherwise the project directory is handed to the engine so + // it can locate its own inputs. + let manifestPath: string | undefined + if (args.manifest) { + manifestPath = path.resolve(args.manifest) + if (!existsSync(manifestPath) || !statSync(manifestPath).isFile()) { + console.error(`Error: manifest not found: ${manifestPath}`) + process.exitCode = 1 + return + } + } else { + const conventional = path.join(projectDir, "target", "manifest.json") + if (existsSync(conventional)) manifestPath = conventional + } + + let manifestJson: string | undefined + if (manifestPath) { + try { + manifestJson = readFileSync(manifestPath, "utf-8") + } catch (e) { + console.error(`Error reading manifest ${manifestPath}: ${e instanceof Error ? e.message : String(e)}`) + process.exitCode = 1 + return + } + } + + // Optional inputs: each must exist. The engine also rejects an unreadable + // path, but checking here names the flag the user got wrong. + const inputPaths: Record = { + "--run-results": args["run-results"], + "--trino-profile": args["trino-profile"], + "--sinks": args.sinks, + } + const resolved: Record = {} + for (const [flag, p] of Object.entries(inputPaths)) { + if (!p) continue + const abs = path.resolve(p) + if (!existsSync(abs) || !statSync(abs).isFile()) { + console.error(`Error: ${flag} file not found: ${abs}`) + process.exitCode = 1 + return + } + resolved[flag] = abs + } + const options = buildPerfOptions({ + runResults: resolved["--run-results"], + trinoProfile: resolved["--trino-profile"], + sinks: resolved["--sinks"], + experimental: args.experimental, + exposureSinks: args["exposure-sinks"], + }) + + let result + try { + result = await Dispatcher.call("altimate_core.dbt_perf", { + project_dir: projectDir, + manifest_json: manifestJson, + options_json: Object.keys(options).length ? JSON.stringify(options) : undefined, + }) + } catch (e) { + // Dispatcher-level failure (registration, native load). Fatal: unlike + // `check`, there is no per-file result to degrade into. + console.error(`Error: dbt-perf failed: ${e instanceof Error ? e.message : String(e)}`) + process.exitCode = 1 + return + } + if (!result.success) { + // Carries the "you need a newer altimate-core, here is how to build it" + // message when the binding is missing (native/altimate-core.ts). + console.error(`Error: ${result.error ?? "altimate_core.dbt_perf failed"}`) + process.exitCode = 1 + return + } + + const report = normalizePerfReport(result.data) + const failOn = args["fail-on"] ?? args.failOn ?? "none" + // Pass/fail on the UNFILTERED findings: --severity controls display only. + const pass = perfPass(report.findings, failOn) + const minSeverity: PerfSeverity = args.severity ?? "info" + const sort: PerfSortOrder = args.sort ?? "rule" + const shown = sortPerfFindings(filterPerfBySeverity(report.findings, minSeverity), sort) + if (sort === "cost" && !report.summary.had_run_results && !report.summary.had_trino_profile) + console.error( + "Note: --sort cost without --run-results or --trino-profile has no measured cost to sort on; engine order kept.", + ) + + if (args.format === "json") { + process.stdout.write( + JSON.stringify( + { + version: 1, + project_dir: projectDir, + manifest: manifestPath, + inputs: options, + sort, + findings: shown, + skipped_rules: report.skipped_rules, + summary: { ...report.summary, findings_shown: shown.length, pass }, + }, + null, + 2, + ) + "\n", + ) + } else { + console.error( + formatPerfText({ + findings: shown, + allFindings: report.findings, + skippedRules: report.skipped_rules, + summary: report.summary, + pass, + minSeverity, + sort, + }), + ) + console.error(`Completed in ${Date.now() - startTime}ms`) + } + + // process.exitCode, not process.exit(): index.ts's finally block still has + // to run Telemetry.shutdown(). + if (!pass) process.exitCode = 1 + }, +}) +// altimate_change end diff --git a/packages/opencode/src/cli/cmd/review.ts b/packages/opencode/src/cli/cmd/review.ts index 88d6395b41..9aae6ab012 100644 --- a/packages/opencode/src/cli/cmd/review.ts +++ b/packages/opencode/src/cli/cmd/review.ts @@ -16,13 +16,17 @@ import type { Severity } from "../../altimate/review/finding" * * Local: altimate review (working tree vs origin/main) * CI: altimate review --post --mode gate (posts verdict + gates merge) + * Project: altimate review --all (every model in the manifest, + * no diff — base-vs-head lanes + * are reported as skipped) * * Shares the exact engine the reviewer agent / dbt_pr_review tool use, so the * CLI, the agent, and the GitHub Action can never diverge. */ export const ReviewCommand = cmd({ command: "review", - describe: "review dbt/SQL changes and emit a signed verdict (APPROVE/COMMENT/REQUEST_CHANGES)", + describe: + "review dbt/SQL changes — or the whole project with --all — and emit a signed verdict (APPROVE/COMMENT/REQUEST_CHANGES)", builder: (yargs) => yargs // Disable yargs' `--no-