Skip to content

fix(review): make the dbt PR review's fidelity and AI status visible in CI - #1241

Open
anandgupta42 wants to merge 30 commits into
mainfrom
fix/dbt-pr-review-ci-experience
Open

anandgupta42 wants to merge 30 commits into
mainfrom
fix/dbt-pr-review-ci-experience

Conversation

@anandgupta42

@anandgupta42 anandgupta42 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #1240

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

I ran the dbt PR review end to end on jaffle_shop_duckdb with five injected changes (semantic filter change, removed test, SELECT * + non-portable function, lineage-breaking rename, safe column reorder) and mined the 30 most recent reviews the action posted on altimate-ingestion. The engine caught the lineage break and the semantic change precisely, but only after I ran dbt docs generate, which no doc or example mentioned. Without it the run showed a "Lint-only run — no dbt manifest/warehouse was available" banner even though the manifest was present (the banner flipped on any single undecidable finding). The AI lane attempted a call on every run and failed silently; telemetry shows 18% of CI runs never complete the AI step. In the dogfood repo 57% of reviews were lint-only, one PR against deployment was diffed against main, one rule ("new model has no uniqueness/grain test") was 427 of 853 findings, and the worst comment ran 406 lines for 130 findings with seven models each carrying two near-identical paragraphs.

Nineteen commits, each answering a review round or a request in-thread:

1. Fidelity and status made visible (2d95db79a6)

  • Envelope summary gains lintOnly, undecidableFindings, artifactHints, aiReview; degraded keeps its old combined meaning.
  • Lint-only banner only when no changed model resolved against a manifest; separate undecidable line; artifact hints name the exact command (dbt docs generate, compile the base); one "AI reviewer:" status line.
  • runAiReview returns {findings, status, reason}; timeout scales with the files actually sent.
  • Base ref, PR title and body come from GITHUB_EVENT_PATH; headless review_run carries project_id.

2. First review round (a22c79ffef)

  • Action and defaultBaseRef compare against the PR merge-base (fork point) for the file list and the old content; the action fetches base ref and head SHA and unshallows first.
  • Empty scope renders "Nothing to review" instead of a manifest banner; undecidable wording names its three causes; artifact hints check compiled SQL per changed model and side; grouped identifiers are Markdown-fenced.

3. Second review round (448232494a)

  • Merge-base against the selected head (custom --head included); hints skipped when no models changed and derived from the engine's filtered classification (Python models included); one clear hint when the dbt project name cannot be resolved; aborted AI streams report timeout, never partial success; review_run gains lint_only and empty_scope.

4. Readable summary (5a38790046)

  • Repetitive lanes (fan-out, undecidable equivalence, grain not_null) render as one item each listing the members with their specifics; headers show "N findings · M items"; non-critical sections past 12 items fold; a "Read first" block picks up to three items when there are eight or more findings; a hidden finding-id block enables "Since last review: N fixed · M new · K unchanged" on rerun. Findings stay atomic; inline comments and JSON are unchanged.

5. Third review round (5710ccc5b3)

  • Example and docs compile target-base from the PR merge-base, the same commit the review diffs against; defaultBaseRef's last-resort fallback is the selected head's parent; AI prompt fetch and model resolution race the lane deadline; impact results carry a resolved flag so lintOnly reflects whether any changed model actually resolved; undecidable_findings telemetry uses the same fallback as the renderer.

6. Fourth review round (5866f62dfd)

  • Example and docs check out the PR head SHA so the head compile matches the reviewed commit; the action fetches a custom head into a resolvable ref; AI error stream events are failures; the rerun delta says "no longer surfaced" and flags changed review settings; the grouped undecidable bullet prescribes compiling only when a compiled artifact is missing; compiled SQL resolves beside a custom manifest target; the AI status line renders whenever the lane ran.

7. Fifth review round (e8b26c98d7)

  • Custom head fetched by any refspec (SHA, tag, branch) into a pinned ref; policy signature hashes the actual exclusion values and AI/data-diff flags and uses only user-configured reviewers, with tier changes reported as "analysis scope changed"; deletion-only diffs are not lint-only; no AI line on empty scope; manifest path realpath-resolved; docs snippet fetches the base ref before the merge-base.

8. Sixth review round (5d49239e9b)

  • The lane deadline covers LLM.stream, the text await and review_ai_parse; the policy signature covers rubric thresholds, blockOn, exclusion values, reviewers and the data-diff config; base compiled project resolves independently when the project was renamed; catalog.json must parse to suppress its hint; deleted models never produce a base hint; markers parse from the footer only; empty scope skips the AI lane; the action rejects heads starting with - and passes -- before the refspec.

9. Seventh review round (472a2ddddc)

  • The stream drain is raced against the lane deadline (a stalled provider returns timeout instead of hanging CI); dialect joins the policy signature; deletion-only reviews emit no catalog hint; an ambiguous base compiled project directory is reported rather than guessed; empty-scope behaviour is asserted where it is decided (orchestrate), not in the formatter.

10–11. Deferred list + previously unanswered threads (fa8088d818, 166be53f9a)

  • An audit found 30 threads from earlier commits that never got a reply; most were already fixed by later rounds and are now answered with the fixing commit. The eleven with substance are fixed: sticky-comment selection accepts only the bot's own comment (a forged marker cannot drive the delta); a 401/403 on --post (read-only fork token) prints the summary and does not fail the job; whitespace-only AI output is an error; empty head means omitted; locations are escaped in every render path; grouped bullets keep the unverified marker; no delta line on a clean re-review; the lint-only banner states its real condition; telemetry fallback preserves the lint-only/empty-scope split; docs snippet is non-fatal on artifact failures and uses the reviewed head for the merge-base.

12. Fork-only 403 tolerance (b3201896bb): the non-fatal post fallback applies only to pull requests from forks; a same-repo 401/403 still fails; --json stdout stays machine-readable.

13. Explicit AI-lane model selector + gateway route (e2fcefc7c7)

  • aiModel in .altimate/review.yml, --ai-model, ALTIMATE_REVIEW_AI_MODEL (flag > env > config). Headless CLI with no model skips the lane with a reason and makes no network call; the in-session tool still uses the session model; an unavailable configured model is an error naming it. The model is recorded in summary.aiReview.model, on every AI status line, and in review_run.ai_model.
  • Action routes: A altimate_api_key (hosted tenant) > B model + model_api_key (BYO) > C altimate_gateway_key (OpenAI-compatible altimate gateway; default model altimate-gateway/altimate-base; altimate_gateway_url defaults to the production gateway and is overridable for a self-hosted one); ai_model overrides within a route; gateway URL must be HTTPS.

14. Selector follow-ups (6be1dd4d43): blank overrides count as unset; gateway URL normalised; the in-session tool uses the active session model; effective model in the policy signature; Provider.parseModel; empty compiled artifacts count as missing; App-token marker ownership restricted to <slug>[bot].

15. CI gate + last follow-ups (3e93b60b86): docs/example pin v0.10.0 and the 0.8.5 release-gate test accepts any pin >= 0.8.5; empty scope distinguishes excluded files from no dbt files; an unused AI model is not hashed into the policy signature.

16. Disabled lane never runs + telemetry redaction (e52796a1bc): ai: false/--no-ai skips the AI lane even when reviewers lists it; empty-scope reason from dbt classification (build artifacts are not dbt files); custom model ids hashed in telemetry; catalog usability requires columns.

17–18. Gateway default URL (latest two commits): altimate_gateway_url defaults to the production gateway with an override; the tracker-leak check allowlists that one public hostname (self-tests cover the allowlist and that the apex and other subdomains are still flagged); the certificate mismatch after the staging rename is recorded as resolved.

19. AI-lane timeout and output budget: measured on the production gateway, altimate-base reasons before answering (~3,000 reasoning tokens on a 12-file prompt, ~25 tok/s, 155 s, truncated by a 4K cap). The timeout is now configurable (aiTimeoutSeconds / --ai-timeout / env / action ai_timeout_seconds, default min(300, 120 + 4 × files)), the lane sets an explicit 8K output budget, the status line shows duration, telemetry records duration and token usage, and a truncated response is an error naming the knob rather than partial findings.

20. Timeout ceiling raised (d6a648629d): range is 10..1800 s (was 10..900); the gateway route defaults to 900 s (was 300), matching the gateway's LiteLLM request_timeout; the action enforces the numeric bounds rather than only checking for digits.

21. Per-run reasoning level (323b787c2a): aiReasoningEffort (none|minimal|low|medium|high) in .altimate/review.yml, --ai-reasoning / ALTIMATE_REVIEW_AI_REASONING, action input ai_reasoning. Unset sends nothing. Threaded to LLM.stream as a provider option (behind altimate_change markers); @ai-sdk/openai-compatible emits it as reasoning_effort. Client half of AltimateAI/altimate-gateway#18; no live effect until that deploys.

Rendered on a real PR. altimate-ingestion #1375, whose posted comment was 406 lines for 130 findings, renders with this branch (source build, dbt parse manifest, no AI credentials) in 64 lines for 20 findings grouped into 12 items, with a "Read first" block, the three missing artifacts named with their commands, and an explicit AI status line. Diff scope is not identical to the original run (the branch has moved), so the comparison is about format, not finding-for-finding.

Verdict logic is unchanged; computeIdealVerdict and applyMode are byte-for-byte the same as main, and AI findings are still excluded from the verdict.

Not in this PR (tracked in docs/internal/2026-09-03-dbt-pr-review-deep-dive.md): catalog rules can currently produce critical and three catalog warnings can block, contrary to the README; .altimate/review.yml is loaded from the PR head; gate-mode lifecycle and applyOverride() wiring; inline-comment dedupe; the positional equivalence false positive (core); the feedback loop; an AI-written executive summary.

How did you verify your code works?

  • From packages/opencode: bun test test/altimate/review (314 pass), bun run typecheck; from the root: bun run script/upstream/analyze.ts --markers --base main --strict, git diff --check, YAML parse of the action and example.
  • Re-ran the injected-change review against each commit from source (bun run --conditions=browser packages/opencode/src/index.ts review --base main inside the jaffle project): no lint-only banner with the manifest present; with catalog.json removed the hint names dbt docs generate; with target-base removed it names the base compile; the AI line reports its failure reason; grain, fan-out and equivalence groups render as one item each; 19 findings render in 71 lines; counts, verdict and exit codes unchanged versus the first run.
  • Not verified: the action YAML in a real GitHub run (no act locally), and the AI lane with real credentials (none on this machine).

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

🤖 Generated with Claude Code

https://claude.ai/code/session_015QY5UKRRakY19d8PszeUFQ


Note

Medium Risk
Changes PR diff semantics (merge-base), CI artifact expectations, and external AI gateway calls; verdict blocking logic is unchanged but misconfigured refs or timeouts could confuse CI outcomes.

Overview
Improves dbt PR review so CI and comments reflect what actually ran instead of a misleading “lint-only” banner when a manifest exists but equivalence or catalog artifacts are missing.

The advisory AI lane now returns explicit status (ok, skipped, timeout, error) with redacted reasons, duration, and token usage; supports an explicit model, configurable timeout (10–1800s) and output budget, and optional reasoning_effort. Headless runs no longer fail silently or hang without reporting why the lane did not produce findings.

The GitHub review action gains a third credential route via altimate_gateway_key, passes --ai-model / --ai-reasoning / --ai-timeout, fetches the PR base ref and uses the merge-base (and PR head SHA) for --base/--head, and bumps the example pin to v0.10.0.

User docs and the altimate-ingestion example workflow now describe compiling the PR head, dbt docs generate, and compiling the merge-base into target-base/compiled, plus gateway/BYO model precedence and new .altimate/review.yml AI knobs.

Adds internal telemetry analysis and a dbt PR review deep-dive plan (not product runtime).

Reviewed by Cursor Bugbot for commit a3541b8. Bugbot is set up for automated code reviews on this repo. Configure here.

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dbt PR review: CI experience hides the engine's proofs (undocumented artifacts, misleading lint-only banner, silent AI lane, guessed base ref)

1 participant