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

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/post-github.ts, line 65:

<comment>`BOT_REVIEW_RE` classifies `Changes requested` as non-blocking. After a gate run requests changes, a clean rerun can hit this guard and return without resolving the old blocking review; track review state and dismiss or resolve that review before skipping.</comment>

<file context>
@@ -54,8 +54,16 @@ export interface PostResult {
 }
 
+/** Headline of a review this bot posted (see verdictHeadline). */
+const BOT_REVIEW_RE = /(Approved|Reviewed with comments|Changes requested)\b.*\((trivial|lite|full) tier/
+
 export async function postGitHubReview(env: VerdictEnvelope, target: GitHubTarget): Promise<PostResult> {
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/post-github.ts, line 108:

<comment>`priorBotReview` accepts any review whose free text resembles the headline; it never verifies the author or a bot marker. A human or another automation can therefore suppress this run's non-blocking review when no new inline comments exist; use an authenticated reviewer identity or unique marker.</comment>

<file context>
@@ -85,9 +93,34 @@ export async function postGitHubReview(env: VerdictEnvelope, target: GitHubTarge
+      octo.paginate(octo.rest.pulls.listReviews, { owner, repo, pull_number: prNumber, per_page: 100 }),
+    ])
+    posted = postedFindingIds(reviewComments.map((c) => c.body))
+    priorBotReview = reviews.some((r) => BOT_REVIEW_RE.test(r.body ?? ""))
+  } catch {
+    /* listing failed: post everything rather than nothing */
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/docs/usage/dbt-pr-review.md, line 297:

<comment>This bullet overstates the new-only PII guarantee when the base lacks compiled SQL. The fallback emits all head-side PII because `engineOldSql` is unavailable, so an existing PII column can still be flagged; either make that path abstain/compare a safe base representation, or qualify the documentation for unverifiable bases.</comment>

<file context>
@@ -279,6 +281,22 @@ rubric:
+- **No re-posting on reruns:** each inline comment carries the finding's
+  fingerprint, so a push does not repeat findings already on the PR, and a
+  non-blocking rerun with nothing new only updates the summary.
+- **PII:** flagged only for PII columns the change newly exposes, not for
+  columns the base already surfaced.
+
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/format.ts, line 160:

<comment>This normalization misses findings whose producer puts the model or filename outside a `model: ` prefix, so the promised cross-model grouping fails for rules such as removed-test findings. Group by a stable rule identity or normalize those producer-specific location fields before building the key.</comment>

<file context>
@@ -115,7 +109,90 @@ export function renderSummary(env: VerdictEnvelope): string {
+ *  blanked, so the same rule on different models/columns shares a key. */
+function ruleTitle(f: Finding): string {
+  const t = f.model && f.title.startsWith(`${f.model}: `) ? f.title.slice(f.model.length + 2) : f.title
+  return t.replace(/`[^`]*`/g, "`…`")
+}
+
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/format.ts, line 175:

<comment>This groups three findings from one model and renders only the first body as “across 1 model(s)”. Use the distinct model/file count for the `GROUP_MIN` threshold so repeated issues within one model stay separate.</comment>

<file context>
@@ -115,7 +109,90 @@ export function renderSummary(env: VerdictEnvelope): string {
+  }
+  const out: Array<{ text: string; count: number }> = []
+  for (const g of groups.values()) {
+    if (g.length >= GROUP_MIN) {
+      const names = [...new Set(g.map((f) => f.model ?? f.file))]
+      const shown = names.slice(0, 8).map((n) => `\`${n}\``).join(", ")
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/run.ts, line 322:

<comment>Unrecognized manifest adapters are treated as known and suppress all catalog portability findings by default. Restrict automatic suppression to dialects recognized by `ADAPTER_DIALECT`, while keeping explicit `dialect` overrides authoritative.</comment>

<file context>
@@ -318,7 +318,9 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
   // analyzed as the snowflake default — wrong-dialect portability suppression).
-  if (!config.dialect) config.dialect = (await detectDialect(manifestAbs)) ?? "snowflake"
+  const detectedDialect = config.dialect ? undefined : await detectDialect(manifestAbs)
+  const dialectKnown = !!config.dialect || !!detectedDialect
+  if (!config.dialect) config.dialect = detectedDialect ?? "snowflake"
 
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/test/altimate/review-noise.test.ts, line 175:

<comment>This test does not verify the claimed severity ordering: `toContain` passes regardless of where `m0` appears. Assert that a critical finding appears before a suggestion (and, ideally, that all three critical findings are shown) so a renderer regression cannot pass.</comment>

<file context>
@@ -0,0 +1,235 @@
+    const env = buildEnvelope({ findings: many, tier: "full", mode: "comment" })
+    const md = renderSummary(env)
+    expect(md.length).toBeLessThanOrEqual(SUMMARY_BUDGET_CHARS)
+    expect(md).toContain("m0: distinct issue number 0")
+    expect(md).toMatch(/… \d+ more finding\(s\) not shown/)
+    // Local output is complete.
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/format.ts, line 149:

<comment>The bundled action does not pass `--output`, so this artifact is never created; point readers to the job log or explicit `altimate review --json --output <file>` usage.</comment>

<file context>
@@ -115,7 +109,90 @@ export function renderSummary(env: VerdictEnvelope): string {
+  if (hidden) {
+    lines.push(
+      `> … ${hidden} more finding(s) not shown to keep this comment short — the full list is in the verdict envelope ` +
+        "(`altimate review --json`, or the action's `--output` artifact).",
+      "",
+    )
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/orchestrate.ts, line 253:

<comment>Build `commentableLines` sets only for files with findings carrying `startLine`; this eagerly parses every changed diff even when no inline comment can be emitted.</comment>

<file context>
@@ -206,6 +206,57 @@ export interface OrchestrateInput {
+ * whole batch of inline comments.
+ */
+export function anchorToDiff(findings: Finding[], files: ChangedFile[]): Finding[] {
+  const lines = new Map(files.map((f) => [f.path, commentableLines(f.diff)]))
+  return findings.map((f) => {
+    if (typeof f.startLine !== "number") return f
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/rule-catalog.ts, line 1715:

<comment>`portability: {}` suppresses the PIVOT/UNPIVOT warning for every known engine, even though this rule provides no verified support set. Populate `worksOn` with verified engines or retain portability findings when support is unknown so engine-specific failures are not hidden.</comment>

<file context>
@@ -1703,7 +1712,7 @@ const STYLE: Rule[] = [
     "PIVOT/UNPIVOT is dialect-specific",
     "`PIVOT`/`UNPIVOT` syntax varies by warehouse and is hard to test; consider conditional aggregation.",
-    { added: /\b(pivot|unpivot)\s*\(/i },
+    { added: /\b(pivot|unpivot)\s*\(/i, portability: {} },
     { add: "from t pivot(sum(x) for k in ('a'))" },
     { add: "sum(case when k='a' then x end)" },
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/format.ts, line 221:

<comment>This note incorrectly claims every unchanged move has SQL that was not re-reviewed. `movedUnchangedFiles` also counts unchanged renames of YAML and other files, so the summary is misleading for those moves; describe unchanged content generically or restrict the counter to model files.</comment>

<file context>
@@ -216,6 +216,11 @@ export function coverageNotes(env: VerdictEnvelope): string[] {
     notes.push(`${env.scope.outOfScopeFiles} changed file(s) outside the dbt project were not reviewed.`)
+  if (env.scope?.movedUnchangedFiles)
+    notes.push(
+      `${env.scope.movedUnchangedFiles} file(s) moved without edits; their SQL was not re-reviewed. ` +
+        "Folder-level config in `dbt_project.yml` (`+schema`, `+materialized`, `+tags`) can still change where and how a moved model builds.",
+    )
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/review/rule-catalog.ts, line 1888:

<comment>Add DuckDB to `worksOn`; otherwise the new single-engine suppression still reports `FETCH FIRST` as non-portable on a dialect that supports it.</comment>

<file context>
@@ -1876,7 +1885,7 @@ const MISC: Rule[] = [
     "FETCH FIRST portability",
     "`FETCH FIRST n ROWS` isn't supported everywhere; `LIMIT` (with ORDER BY) is more portable.",
-    { added: /fetch\s+first\s+\d+/i },
+    { added: /fetch\s+first\s+\d+/i, portability: { worksOn: ["postgres", "snowflake", "oracle", "trino"] } },
     { add: "fetch first 10 rows only" },
     { add: "limit 10" },
</file context>

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/test/altimate/review-noise.test.ts, line 262:

<comment>This test does not exercise the “not a lint-only run” behavior it names. Build the envelope through the moved-only review path, or add an assertion backed by a fixture that would fail if that path sets `degraded` or renders the lint-only banner.</comment>

<file context>
@@ -227,9 +248,24 @@ describe("portability noise", () => {
+
+describe("moved files", () => {
+  test("moves are named in the comment with the folder-config caveat, and are not a lint-only run", async () => {
+    const env = buildEnvelope({
+      findings: [],
+      tier: "trivial",
</file context>