From 5bad6484c1621ff953f2fad2107df52b0f384a38 Mon Sep 17 00:00:00 2001 From: RaghwendraSingh Date: Tue, 22 Sep 2026 18:59:42 +0530 Subject: [PATCH 1/8] feat(plugin): ship altimate-code as a Claude Code plugin Adds `.claude-plugin/marketplace.json` and a plugin at `plugins/claude-code/` so `altimate-code` can be installed into Claude Code: claude plugin marketplace add claude plugin install altimate-data@altimate-code Four skills, each a thin wrapper that shells out to the `altimate` / `altimate-dbt` binaries: `sql-review`, `dbt-pr-review`, `dbt-analyze`, `dbt-schema-verify`. The `.opencode/skills/` bodies could not be reused verbatim -- they instruct the model to call in-process opencode tools (`sql_analyze`, `altimate_core_rewrite`, `dbt_lineage`) that do not exist inside Claude Code, where shelling out to the binary is the only surface. Two constraints are documented in the skills rather than papered over: * **Jinja is a silent false green.** `altimate check` parses SQL, not Jinja. A dbt model beginning `{{ config(...) }}` fails to parse at line 1, every lint rule is skipped, and the lint block still reports zero findings. Verified on `int_user_query_usage.sql`: `0 errors, 0 warnings` raw versus 3 real findings (`L049 clock_in_filter`, `L027 function-on-filter-column`, `L030 select-without-limit`) once the Jinja was resolved. `sql-review` now tells the reader to check `results.validate` for `E000` before trusting a clean lint report, and to re-run against `dbt compile` output. * **`altimate-dbt` is not published to npm** and is not part of the `altimate` binary -- `bin/altimate-dbt` loads a `dist/` that only exists after a local `bun run build`. `dbt-analyze` and `dbt-schema-verify` now resolve the tool and fall back to plain `dbt ls` / manifest inspection, which covers the graph queries, instead of failing silently. `claude plugin validate --strict` passes for both the marketplace and the plugin manifest. --- .claude-plugin/marketplace.json | 20 ++++ .../claude-code/.claude-plugin/plugin.json | 14 +++ .../claude-code/skills/dbt-analyze/SKILL.md | 93 +++++++++++++++++++ .../claude-code/skills/dbt-pr-review/SKILL.md | 57 ++++++++++++ .../skills/dbt-schema-verify/SKILL.md | 68 ++++++++++++++ .../claude-code/skills/sql-review/SKILL.md | 69 ++++++++++++++ 6 files changed, 321 insertions(+) create mode 100644 .claude-plugin/marketplace.json create mode 100644 plugins/claude-code/.claude-plugin/plugin.json create mode 100644 plugins/claude-code/skills/dbt-analyze/SKILL.md create mode 100644 plugins/claude-code/skills/dbt-pr-review/SKILL.md create mode 100644 plugins/claude-code/skills/dbt-schema-verify/SKILL.md create mode 100644 plugins/claude-code/skills/sql-review/SKILL.md 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/plugins/claude-code/.claude-plugin/plugin.json b/plugins/claude-code/.claude-plugin/plugin.json new file mode 100644 index 0000000000..a6b9842abf --- /dev/null +++ b/plugins/claude-code/.claude-plugin/plugin.json @@ -0,0 +1,14 @@ +{ + "name": "altimate-data", + "displayName": "Altimate Data", + "version": "0.1.0", + "description": "Deterministic dbt and SQL analysis powered by the altimate engine", + "author": { + "name": "Altimate AI", + "email": "support@altimate.ai" + }, + "homepage": "https://github.com/AltimateAI/altimate-code", + "repository": "AltimateAI/altimate-code", + "license": "Apache-2.0", + "keywords": ["dbt", "sql", "lineage", "data-engineering"] +} diff --git a/plugins/claude-code/skills/dbt-analyze/SKILL.md b/plugins/claude-code/skills/dbt-analyze/SKILL.md new file mode 100644 index 0000000000..864524f5fe --- /dev/null +++ b/plugins/claude-code/skills/dbt-analyze/SKILL.md @@ -0,0 +1,93 @@ +--- +name: dbt-analyze +description: > + Blast radius of a dbt model change — column-level lineage plus the dependency + graph, so you know what breaks before you ship. Use when changing a model's + columns, grain, or types, or when asked "what depends on this?". Powered by + altimate-dbt. +allowed-tools: Bash, Read, Glob, Grep +--- + +# dbt blast radius + +Answer "what breaks if I change this?" before editing, not after. + +## Preflight — resolve the tool + +`altimate-dbt` is **not published to npm** and is not part of the `altimate` binary. It +only exists if someone built it from the monorepo. Resolve it, and fall back to plain +`dbt` when it is absent — the graph queries work either way. + +```bash +ls dbt_project.yml >/dev/null 2>&1 || echo "run this from the dbt project directory" +if command -v altimate-dbt >/dev/null; then + ADBT=altimate-dbt +else + echo "altimate-dbt unavailable — using plain dbt. To build it:" + echo " cd /packages/dbt-tools && bun install && bun run build" + ADBT= +fi +``` + +## 1. Dependency graph + +With `altimate-dbt`: + +```bash +altimate-dbt children --model $ARGUMENTS # everything downstream +altimate-dbt parents --model $ARGUMENTS # everything it reads +altimate-dbt columns --model $ARGUMENTS # the model's output columns +``` + +Output is JSON by default (`--format text` for prose). + +With plain dbt — equivalent for the graph, and it needs no warehouse connection beyond +a parse: + +```bash +dbt parse --profiles-dir . +dbt ls --select $ARGUMENTS+ --output name # children (downstream) +dbt ls --select +$ARGUMENTS --output name # parents (upstream) +dbt ls --select $ARGUMENTS --output json # config + columns from the manifest +``` + +Also worth running, because it finds references the graph misses (macros, `-- depends_on` +pragmas, docs): + +```bash +grep -rn "ref('$ARGUMENTS')" models/ macros/ --include='*.sql' --include='*.yml' +``` + +## 2. Column-level lineage + +Prefer the manifest path — it resolves `ref()` and `source()` to real relations, so it +is more accurate than parsing the SQL alone: + +```bash +altimate-dbt compile --model $ARGUMENTS # or: dbt compile --select $ARGUMENTS +``` + +Then trace which downstream columns derive from the ones you are touching. Read +`target/manifest.json` — each node carries `depends_on.nodes`, `columns`, and (after +`compile`, not `parse`) `compiled_code`. + +## 3. The boundary that dbt does not know about + +**dbt's DAG ends at the mart.** In repos where marts are read by external loaders, the +real consumers are outside the graph — look for a loader registry declaring which models +each loader reads (in `altimate-ingestion` that is +`packages/load-snowflake/src/load_snowflake/registry.py`, whose `LoaderEntry.source_tables` +names the dbt models) and check those too. A column with no dbt consumer is not +necessarily unused. + +## 4. Classify + +For each downstream dependency, say which it is: + +- **BREAKING** — a column it reads is removed, renamed, retyped, or made nullable; the + grain changes; a contract-enforced model loses a column. +- **SAFE** — additive only, or the change does not touch anything it reads. +- **UNKNOWN** — you could not resolve it. Say so; do not guess. + +Report the count at each depth and name the tests at risk. Recommend +`altimate-dbt build --model --downstream` to verify. diff --git a/plugins/claude-code/skills/dbt-pr-review/SKILL.md b/plugins/claude-code/skills/dbt-pr-review/SKILL.md new file mode 100644 index 0000000000..01b2cac77f --- /dev/null +++ b/plugins/claude-code/skills/dbt-pr-review/SKILL.md @@ -0,0 +1,57 @@ +--- +name: dbt-pr-review +description: > + Review dbt/SQL changes in a branch or PR and produce a verdict backed by + deterministic engine calls — column-lineage blast radius, query equivalence, + dbt config lint, PII, and an A-F grade. Use before merging a dbt PR. Reviews a + DIFF; for the whole project use dbt-project-review instead. +allowed-tools: Bash, Read, Glob, Grep +--- + +# dbt PR review + +Runs `altimate review`, the same command `altimate-ingestion` runs in CI +(`.github/workflows/dbt-pr-review.yml`). + +## Scope — read this first + +`altimate review` reviews **a diff**, not a project. It defaults to the merge-base with +`origin/main` for `--base` and the working tree for `--head`. 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`. + +## Preflight + +The engine needs a manifest. `dbt parse` produces one offline, with no warehouse +connection: + +```bash +cd && dbt deps && dbt parse --profiles-dir . +``` + +Placeholder values for any `env_var()` the profile or sources need are fine — parse never +connects. + +## Run + +```bash +altimate review \ + --cwd \ + --manifest target/manifest.json \ + --mode comment \ + --severity suggestion \ + --json +``` + +- `--mode comment` never blocks; `--mode gate` exits non-zero on REQUEST_CHANGES. +- `--base ` / `--head ` to review a range other than the default. +- `--no-ai` for the deterministic lanes only (no model calls, no cost). +- `--post` posts to the GitHub PR; only use it when explicitly asked. + +## Reporting + +Lead with the verdict and the blocking findings. For each blocking finding give the rule, +the file and line, and the fix. Separate the deterministic engine findings from the +advisory LLM lane — they carry different weight, and only the first should ever block. + +Do not re-report a finding without opening the file and confirming it. diff --git a/plugins/claude-code/skills/dbt-schema-verify/SKILL.md b/plugins/claude-code/skills/dbt-schema-verify/SKILL.md new file mode 100644 index 0000000000..b86849225b --- /dev/null +++ b/plugins/claude-code/skills/dbt-schema-verify/SKILL.md @@ -0,0 +1,68 @@ +--- +name: dbt-schema-verify +description: > + REQUIRED after building or modifying any dbt model with columns declared in + schema.yml / _models.yml. Diffs the model's actual columns against the spec and + treats any mismatch as "not done". Catches the green-build-failing-tests case + where the data values are right but the column shape is wrong. +allowed-tools: Bash, Read +--- + +# dbt schema verify + +The most common reason a build is green but the tests still fail: the model produces the +right *values* in the wrong *column shape* — extra columns, missing columns, wrong order, +wrong types. Many dbt equality tests grade the column tuple `(name, type, position)` +exactly. + +The failure mode to guard against is adding "helpful" extras (rank breakdowns, +name-resolved variants, lineage metadata) or reordering columns "more logically". Both +break the contract. + +## Preflight — resolve the tool + +`altimate-dbt` is **not published to npm** and is not part of the `altimate` binary. It +only exists if someone built it from the monorepo: + +```bash +command -v altimate-dbt >/dev/null && echo "ok" || \ + echo "build it: cd /packages/dbt-tools && bun install && bun run build" +``` + +## Run + +```bash +cd && altimate-dbt schema-verify --model $ARGUMENTS +``` + +### Fallback when altimate-dbt is unavailable + +The check is a set diff between the spec and the built relation, so it can be done by +hand. Build the model, then compare: + +```bash +dbt compile --select $ARGUMENTS # refresh target/manifest.json +python3 - <<'PY' +import json, sys +m = json.load(open("target/manifest.json")) +node = next(v for v in m["nodes"].values() + if v["resource_type"] == "model" and v["name"] == "MODEL_NAME") +spec = list(node["columns"]) # declared in schema.yml, in order +print("declared:", spec) +PY +``` + +Then read the actual columns from the warehouse (`dbt show --select $ARGUMENTS --limit 0`, +or `describe `) and diff the two lists **as ordered tuples**, not as sets — +order is part of the contract. + +## Verdicts + +- `match` — done. +- `mismatch` — **not done.** The result names `columns_extra`, `columns_missing`, + `columns_reordered` and `type_mismatches`. Fix the model (or, only if the spec is the + thing that is wrong, fix the spec — and say which you chose and why). +- `no-spec` — the model declares no columns. Report that; do not invent a spec. + +Re-run until `match` before reporting the work complete. A mismatch you have decided to +accept still gets stated explicitly, with the reason. diff --git a/plugins/claude-code/skills/sql-review/SKILL.md b/plugins/claude-code/skills/sql-review/SKILL.md new file mode 100644 index 0000000000..254d7dbec6 --- /dev/null +++ b/plugins/claude-code/skills/sql-review/SKILL.md @@ -0,0 +1,69 @@ +--- +name: sql-review +description: > + Deterministic SQL quality gate for .sql files — parsed lint (not regex), syntax + validation, injection/safety scan, PII exposure, and an A-F grade. Use before + committing SQL, when reviewing SQL changes, or when asked whether a query has + anti-patterns. Runs the altimate engine locally; no model calls. +allowed-tools: Bash, Read, Glob +--- + +# SQL review + +Runs `altimate check`, which calls the Rust analysis engine directly. Findings are +parsed from the AST, so they do not suffer the false positives a regex sweep produces. + +## Preflight + +```bash +command -v altimate >/dev/null || echo "altimate not on PATH — install: curl -fsSL https://www.altimate.sh/install | bash" +``` + +## Jinja is a silent false green — read this before trusting a clean report + +The engine parses SQL, not Jinja. A dbt model beginning `{{ config(...) }}` fails to parse +at line 1, **every lint rule is skipped, and the lint block still reports zero findings.** +A clean report on a raw dbt model means nothing. + +Verified: one model reported `0 errors, 0 warnings` raw, and 3 real findings +(`L049 clock_in_filter`, `L027 function-on-filter-column`, `L030 select-without-limit`) +once the Jinja was resolved. + +So: always check `results.validate` first. If it carries `E000 Syntax error`, the lint +result is void — say so, and re-run against compiled SQL: + +```bash +dbt compile --select # writes target/compiled/**/.sql +altimate check target/compiled/**/.sql --format json --checks lint,validate,grade +``` + +`E001 Table '' not found` from `validate` is different — that is missing schema +context, not a parse failure, and the lint findings alongside it are valid. + +## Run + +```bash +altimate check $ARGUMENTS --format json --checks lint,validate,safety,grade --severity info +``` + +- `$ARGUMENTS` is one or more `.sql` / `.ddl` paths or globs. With no argument, review + the SQL files changed in the working tree (`git diff --name-only -- '*.sql'`). +- Available checks: `lint`, `validate`, `safety`, `policy`, `pii`, `semantic`, `grade`. + Add `pii` when the SQL touches user data; add `policy --policy ` when the + repo ships a policy file. +- `--severity info|warning|error` filters the report; `--fail-on warning|error` makes it + exit non-zero, which is what a CI invocation should use. + +## Reporting + +Group findings by severity, highest first. For each: the rule code, the file and line, +what is wrong, and the concrete fix. State the grade if `grade` ran. + +Two rules when reporting: + +- **Do not restate a finding you cannot see in the source.** Open the file and confirm + the line before repeating what the engine said. +- **A finding inside a comment is not a finding.** The engine parses, so this should not + happen — if it does, say so, because that is an engine bug worth filing. + +If the engine reports zero findings, say that plainly rather than manufacturing advice. From e49ed1357cb118a88f956942d1d56495e148db54 Mon Sep 17 00:00:00 2001 From: RaghwendraSingh Date: Tue, 22 Sep 2026 21:57:25 +0530 Subject: [PATCH 2/8] feat(review): project-scope review (`--all`), `altimate dbt-perf`, and a dbt-project-review skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `altimate review` could only review a diff. It defaults to the merge-base with `origin/main`, so a model that has been wrong since the day it was written never appears. `altimate-ingestion` has run the reviewer in CI for months with ~20 genuine project-level findings sitting behind it, untouched, for exactly that reason. **`altimate review --all`** reviews every model in the manifest instead — one version each, no base ref. Mutually exclusive with `--base`/`--head` (it fails loudly rather than ignoring one of them); `--severity`, `--json`, `--output`, `--no-ai`, `--cwd`, `--manifest` and `--mode gate` all behave as they do today. Three things the pipeline needed, because it is diff-oriented throughout: * **The base-vs-head lanes are reported, not dropped.** `semantic_change` (equivalence), `structural_diff`, `dbt_config_diff`, `column_breakage`, `lineage_breakage`, `data_diff`, `topology_equivalence` and the schema.yml test-removal detector all compare two versions of a model. They are listed on the envelope's new `lanes.skipped[]` with the reason each could not run, and rendered in the summary. A report that silently omits half the lanes is worse than no report. `ai_review` is skipped too, for a different reason: it is diff-grounded and caps at 20 files, so project-wide it would review an arbitrary subset at unbounded cost. * **The diff-anchored detectors needed a diff.** `ruleMatches` in `rule-catalog.ts` is relevance-gated by construction — a rule only fires when its signal is in added/removed lines. With no hunks the entire catalog and most of `dbt-patterns.ts` evaluate to nothing while still reporting as "ran". Project scope therefore presents each whole model as added (`wholeFileAsAddedDiff`), which is also the honest shape: the review has no previous version of the file. * **Tiering is a diff cost-control and is bypassed.** `classifyPR` reads changed-line counts and "was a contract block touched in the hunk"; with neither it lands on `trivial` and silently disables most of the review. Project scope pins `full`. `--all` also counts how many models the engine lanes saw as dbt-COMPILED SQL (`lanes.compiledModels` / `lanes.totalModels`) and warns when they diverge. This is the `--all` form of the Jinja false-green: the SQL parser cannot read `{{ config() }}`, so a raw-Jinja model yields zero AST findings, which reads exactly like a clean one. Measured on `altimate-ingestion`'s `transform-snowflake`: 207 of 214 models had no compiled artifact, so without the warning the run would have reported 3% AST coverage as a project review. Performance is not a concern at the stated size — 214 models, every lane, 6.6s. The default `--max-models` cap of 500 exists so a pathological monorepo does not look hung; it never truncates silently (stderr warning naming both counts), and `--max-models 0` lifts it. **`altimate dbt-perf`** is a new deterministic, no-LLM command modelled on `altimate check`, over a new `altimate_core.dbt_perf` bridge method that wraps the engine's `analyze_dbt_perf` / `analyze_dbt_perf_from_manifest` bindings. Those bindings are NOT in the pinned `@altimateai/altimate-core@0.7.0`, so the handler resolves them at call time rather than importing them — an `import { analyzeDbtPerf }` would break every other core handler in that module on the pinned release — and returns an actionable "build and link a local core" message when they are absent, instead of an undefined-is-not-a-function stack. The pin is unchanged. Text output leads with the summary line, prints `skipped_rules` BEFORE the findings (a rule that could not run is not a clean result), groups findings by rule code, and prints `delete_scope` + `reader_requirements` in full for a `MaterializationChange` guardrail — including an explicit "(not stated — verify manually)" when the engine omits them. Those fields exist because a past performance fix switched a materialization without dropping the old relation. **`dbt-project-review`** ships in both skill systems: `.opencode/skills/` and `plugins/claude-code/skills/` (the latter shells out to the `altimate` binary, since in-process opencode tools do not exist there). Both make the diff-vs- project split explicit — `dbt-pr-review` for "is this change safe", `dbt-project-review` for "is this project healthy" — and both carry the Jinja-false-green warning, sharpened for project scope: `dbt parse` is not enough, because it writes a manifest but no compiled SQL. `claude plugin validate ./plugins/claude-code --strict` passes. Diff scope is untouched: the envelope's `scope` and `lanes` fields are emitted only in project scope, so existing PR comments and stored verdicts are byte-identical. Co-Authored-By: Claude Opus 5 (1M context) --- .opencode/skills/dbt-pr-review/SKILL.md | 10 +- .opencode/skills/dbt-project-review/SKILL.md | 128 ++++++++++ docs/docs/usage/dbt-pr-review.md | 66 +++++ .../src/altimate/native/altimate-core.ts | 42 ++++ .../opencode/src/altimate/native/types.ts | 7 + .../opencode/src/altimate/review/README.md | 5 +- .../opencode/src/altimate/review/format.ts | 39 ++- .../src/altimate/review/orchestrate.ts | 208 ++++++++++++--- packages/opencode/src/altimate/review/run.ts | 162 +++++++++--- .../opencode/src/altimate/review/verdict.ts | 41 +++ .../opencode/src/cli/cmd/dbt-perf-helpers.ts | 237 ++++++++++++++++++ packages/opencode/src/cli/cmd/dbt-perf.ts | 158 ++++++++++++ packages/opencode/src/cli/cmd/review.ts | 34 ++- packages/opencode/src/index.ts | 6 + .../altimate/altimate-core-native.test.ts | 22 ++ .../opencode/test/altimate/review.test.ts | 166 ++++++++++++ packages/opencode/test/cli/dbt-perf.test.ts | 144 +++++++++++ .../skills/dbt-project-review/SKILL.md | 139 ++++++++++ 18 files changed, 1529 insertions(+), 85 deletions(-) create mode 100644 .opencode/skills/dbt-project-review/SKILL.md create mode 100644 packages/opencode/src/cli/cmd/dbt-perf-helpers.ts create mode 100644 packages/opencode/src/cli/cmd/dbt-perf.ts create mode 100644 packages/opencode/test/cli/dbt-perf.test.ts create mode 100644 plugins/claude-code/skills/dbt-project-review/SKILL.md 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..cf50af653c --- /dev/null +++ b/.opencode/skills/dbt-project-review/SKILL.md @@ -0,0 +1,128 @@ +--- +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`. Surface both: a rule that could not run + is not a clean result. +- Where a finding carries a `guardrail` of kind `MaterializationChange`, 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. +- 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. + +## 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/docs/docs/usage/dbt-pr-review.md b/docs/docs/usage/dbt-pr-review.md index 2c2a19afe5..7db2955e83 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,70 @@ 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) | + +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. + +### 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..7a7eb18d4b 100644 --- a/packages/opencode/src/altimate/native/altimate-core.ts +++ b/packages/opencode/src/altimate/native/altimate-core.ts @@ -51,6 +51,15 @@ 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" + // --------------------------------------------------------------------------- // IFF / QUALIFY transpile transforms (ported from Python guard.py) // --------------------------------------------------------------------------- @@ -370,6 +379,39 @@ 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) + } + }) + // 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/types.ts b/packages/opencode/src/altimate/native/types.ts index ed995589e5..a34edeed76 100644 --- a/packages/opencode/src/altimate/native/types.ts +++ b/packages/opencode/src/altimate/native/types.ts @@ -1277,6 +1277,13 @@ 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 + }, // 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..7922e0de6c 100644 --- a/packages/opencode/src/altimate/review/README.md +++ b/packages/opencode/src/altimate/review/README.md @@ -70,7 +70,10 @@ it; lower layers fill gaps and add context but never override a proof. ## 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/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..1395b34920 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" @@ -198,6 +198,59 @@ export interface OrchestrateInput { forceTier?: "trivial" | "lite" | "full" /** Manifest was stale relative to change-affecting files (see run.ts::detectStaleManifest). */ staleManifest?: boolean + /** `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", +} + +/** + * 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. */ @@ -1057,6 +1110,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 +1154,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 +1235,38 @@ export async function runReview(input: OrchestrateInput): Promise (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 +1278,31 @@ 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)) - tasks.push( - semanticChangeLane( - ctx.file, - input.runner, - ctx.engineOldSql, - ctx.engineNewSql, - dialect, - ctx.impact, - input.rubric, - ), - ) + 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 (lanes.has("lineage_breakage")) { + if (runLineage && !projectScope) { all.push(lineageBreakageLane(ctx.file, ctx.impact, input.rubric)) tasks.push(columnBreakageLane(ctx, input.runner, getCompiled, dialect)) } - if (lanes.has("test_coverage")) tasks.push(missingGrainTestLane(ctx, input.runner)) + 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 +1319,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 +1506,7 @@ export async function runReview(input: OrchestrateInput): Promise ({ path: ctx.file.path, status: ctx.file.status, @@ -1439,9 +1554,20 @@ export async function runReview(input: OrchestrateInput): 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 +268,21 @@ 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 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 +320,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,10 +354,64 @@ 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 }) + // 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) + return runReview({ changedFiles, config, @@ -353,5 +436,6 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise explainTier: opts.explainTier, forceTier: opts.forceTier, staleManifest, + scope: opts.all ? "project" : "diff", }) } 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-perf-helpers.ts b/packages/opencode/src/cli/cmd/dbt-perf-helpers.ts new file mode 100644 index 0000000000..05b3843d22 --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-perf-helpers.ts @@ -0,0 +1,237 @@ +// 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 + [key: string]: unknown +} + +export interface PerfReport { + findings: PerfFinding[] + skipped_rules: PerfSkippedRule[] + summary: PerfSummary +} + +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) +} + +/** 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 +} + +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 (inputs.length) lines.push(`Inputs — ${inputs.join(", ")}`) + 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) + } + 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) { + const sev = normalizePerfSeverity(f.severity).toUpperCase() + const model = f.model ? ` ${f.model}` : "" + const scope = f.scope ? ` <${f.scope}>` : "" + lines.push(` ${sev}${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: ${inlineValue(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 (kind === "MaterializationChange") { + 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(", ")}`) + } + lines.push("") + } + } + + lines.push(opts.pass ? "PASS" : "FAIL") + return lines.join("\n") +} +// 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..20839e2d75 --- /dev/null +++ b/packages/opencode/src/cli/cmd/dbt-perf.ts @@ -0,0 +1,158 @@ +// 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, + filterPerfBySeverity, + 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, + }), + + handler: async (args: { + cwd?: string + manifest?: string + format?: "json" | "text" + severity?: PerfSeverity + "fail-on"?: "none" | "warning" | "error" + failOn?: "none" | "warning" | "error" + }) => { + 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 + } + } + + let result + try { + result = await Dispatcher.call("altimate_core.dbt_perf", { + project_dir: projectDir, + manifest_json: manifestJson, + }) + } 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 shown = filterPerfBySeverity(report.findings, minSeverity) + + if (args.format === "json") { + process.stdout.write( + JSON.stringify( + { + version: 1, + project_dir: projectDir, + manifest: manifestPath, + 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, + }), + ) + 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..aee2b7f8e5 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-