fix(review): a readable PR comment: diff-anchored inline comments, no reposts, grouped rules, size budget, new-only PII, no portability noise

RA
2d ago
+580-61

Issue for this PR

Closes #

Stacked on #1720, which is stacked on #1719; merge in that order.

Type of change

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

What does this PR do?

On a real dbt repo the review comment was mostly noise:

  • Reposts: nearly half of the bot's inline comments were the same comment re-posted on a later push.
  • Repeated rules: four generic rules made up most on-diff findings, emitted once per model.
  • Size: summaries ran to tens of thousands of characters.
  • Broken anchors: inline comments were anchored to compiled-SQL line numbers, so a single line outside the diff got a 422 and every inline comment was lost.

Fixes:

  • Anchors: a finding keeps its line only if that line is a head-side line inside the file's diff hunks. Engine lint on compiled SQL no longer sets raw-file lines. Anything unanchorable stays in the summary.
  • No reposts: each inline comment carries <!-- altimate-finding:<id> -->. The id is the existing fingerprint (rule, file, model, column — no line), so moving code doesn't change it. On rerun, findings already on the PR are skipped. A non-blocking rerun with nothing new only updates the summary, with no new "Reviewed with comments" entry. Blocking verdicts always post.
  • Grouping: the same rule (title minus model name, backticked names blanked) on 3+ models becomes one bullet listing the models.
  • Budget: the posted summary is capped at about 5K characters, most severe first. A count says what was left out; the envelope keeps everything. CLI and agent-tool output are uncapped.
  • PII: the fallback flags only columns the change newly exposes, as the docs already promise. Behavior change: a touched model that already exposed email no longer gets a critical (blocking in gate mode) finding. The test that asserted the old behavior is inverted, and four fixtures whose fake detector returned PII for any SQL are now SQL-aware.
  • Portability: cross-engine advice from the regex catalog ("isn't portable") is dropped once the engine is known from the manifest adapter or dialect. portability: true turns it back on for multi-adapter packages. Core's dialect-aware check (a function missing on your engine) is unchanged.

Follow-up after an adversarial review (commit 1c5dc3a):

  • Portability is now an explicit per-rule tag (portability: { worksOn? }) instead of matching the comment text. The text match was dropping BigQuery's legacy-SQL warning ("use Standard SQL") on BigQuery projects.
  • Advice is kept when the construct fails on your engine. With a known engine, portability advice is dropped only when that engine supports the construct. DISTINCT ON on Snowflake or GROUP BY alias on SQL Server stays, because it errors there.
  • Moved files are named in the comment, with a caveat that folder-level dbt_project.yml config (+schema, +materialized) can still change how they build.
  • No more "lint-only" label for runs without model files (yml-only changes, pure moves).
  • Tests: suppression is checked in both directions (Postgres drops DISTINCT ON advice, Snowflake keeps it), plus the legacy-SQL case and the move note.

How did you verify your code works?

  • New test/altimate/review-noise.test.ts (13 tests):
    • hunk line parsing (blank context lines, ++ SQL lines, headers)
    • anchoring and the compiled-SQL line drop
    • fingerprint markers and dedup
    • telemetry for a skipped review
    • grouping thresholds and backticked names
    • budget with a 200-finding envelope
    • portability suppression on a real catalog rule
  • 1,872 tests across the review files and release gates pass. bun typecheck is clean.

Cost side effects

Two extra GitHub API list calls per --post (review comments and reviews, paginated), plus one extra local PII detection per modified model on the fallback path. No model calls, warehouse queries or cloud resources.

Screenshots / recordings

N/A (CLI).

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR
  • I checked for cost side effects (model calls, queries or cloud resources that grow with user data) and described them, or wrote "none"

🤖 Generated with Claude Code

Summary by cubic

Fixes the dbt PR review comment so it is readable over noisy: inline comments now land on lines that actually exist in the diff, findings already on the PR are not re-posted on later pushes, repeated rules are grouped, and the posted summary respects a ~5,000-character budget (local CLI and agent output stay complete).

Behavior changes

  • PII fallback now flags only columns the change newly exposes; a model that already exposed PII in the base no longer gets a critical (gate-blocking) finding.
  • Cross-engine portability advice is an explicit catalog-rule tag, hidden once a single engine is known from the manifest adapter or dialect — but kept when it would fail on the project's own engine (e.g. DISTINCT ON on Snowflake). Set portability: true in review.yml for multi-adapter packages.
  • Files moved without edits are called out in the comment, and yml-only or pure-move runs are no longer labeled lint-only.