Visitar URL original
fix(review): a readable PR comment: diff-anchored inline comments, no reposts, grouped rules, size budget, new-only PII, no portability noise by RaghwendraSingh · Pull Request #1721 · AltimateAI/altimate-code · GitHub
Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/docs/usage/dbt-pr-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ mode: comment # comment | gate
severityThreshold: suggestion
manifestPath: target/manifest.json
dialect: snowflake
portability: false # cross-engine advice; default off once the engine is known
# (set true for packages that support several adapters)
reviewers: [] # empty = risk-tier defaults; or pin lanes
dataDiff: # OFF by default — see "Data-diff in CI" below
enabled: false
Expand All @@ -279,6 +281,22 @@ rubric:
skipNonProdModels: true
```

### What the PR comment shows

- **One summary comment**, updated in place on every push, capped at about
5,000 characters. Critical findings come first; anything that does not fit is
counted, and the full list is always in the verdict envelope
(`altimate review --json`). Local CLI output is never truncated.
- **Repeated rules are grouped:** the same rule on three or more models is one
bullet listing the models.
- **Inline comments only on lines of the PR's diff.** Findings computed on
compiled SQL, or on unchanged lines, stay in the summary.
- **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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 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.

Prompt for AI agents
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>

columns the base already surfaced.

### Risk-tier path tokens

Named categories under `riskTierPathTokens` promote any diff touching a matching
Expand Down
8 changes: 8 additions & 0 deletions packages/opencode/src/altimate/review/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ export const ReviewConfig = z.object({
dialect: z.string().default(""),
/** Enable the advisory LLM reviewer lane (needs a configured model). */
ai: z.boolean().default(true),
/**
* Report cross-engine portability advice ("isn't portable across
* warehouses"). Off by default once the engine is known (from the manifest's
* adapter or `dialect`), because a project that runs on one engine gains
* nothing from it. Turn on for packages that support several adapters.
* Functions missing on the project's own engine are always reported.
*/
portability: z.boolean().optional(),
/**
* Data-diff: actually run base-vs-head against the warehouse (core DataParity)
* and report row/value deltas. OPT-IN — it costs warehouse compute and needs a
Expand Down
148 changes: 122 additions & 26 deletions packages/opencode/src/altimate/review/format.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,20 @@ export function verdictHeadline(env: VerdictEnvelope): string {
return `${VERDICT_LABEL[env.verdict]} — ${counts} (${tierLabel})`
}

/** Default size budget for the posted PR comment (characters). */
export const SUMMARY_BUDGET_CHARS = 5_000
/** The same rule on this many models or more collapses into one bullet. */
export const GROUP_MIN = 3

export interface RenderOptions {
/** Character budget for the comment; findings beyond it are counted, not listed.
* `Infinity` renders everything (local CLI output). */
maxChars?: number
}

/** Full PR/MR summary comment body (markdown), prefixed with the dedup marker. */
export function renderSummary(env: VerdictEnvelope): string {
export function renderSummary(env: VerdictEnvelope, opts: RenderOptions = {}): string {
const maxChars = opts.maxChars ?? SUMMARY_BUDGET_CHARS
const lines: string[] = [REVIEW_MARKER, "", `## ${verdictHeadline(env)}`, ""]

if (env.summary.degraded) {
Expand Down Expand Up @@ -80,33 +92,15 @@ export function renderSummary(env: VerdictEnvelope): string {
lines.push(`> 🧭 **Tier: ${env.tier}** — ${shown}${overflow}`, "")
}

if (!env.findings.length) {
lines.push("No issues found in the changed dbt models. 🎉", "")
} else {
const grouped = groupBySeverity(env.findings)
for (const sev of ["critical", "warning", "suggestion"] as const) {
const items = grouped[sev]
if (!items.length) continue
lines.push(`### ${SEVERITY_EMOJI[sev]} ${capitalize(sev)} (${items.length})`, "")
for (const f of items) {
const loc = f.file + (f.startLine ? `:${f.startLine}` : "")
lines.push(
`- **${f.title}** \n ${oneLine(f.body)} \n <sub>\`${loc}\`${f.degraded ? " · _unverified_" : ""} · ${f.category}</sub>`,
)
}
lines.push("")
}
}

const tail: string[] = []
if (env.override) {
lines.push(
tail.push(
`> 🔓 **Break-glass override** by @${env.override.by}: ${env.override.reason}`,
`> (would have been \`${env.override.priorVerdict}\`)`,
"",
)
}

lines.push(
tail.push(
"---",
`<sub>altimate dbt-pr-review · verdict \`${env.verdict}\`` +
(env.signature ? ` · signed \`${env.signature.slice(0, 18)}…\`` : "") +
Expand All @@ -115,7 +109,90 @@ export function renderSummary(env: VerdictEnvelope): string {
(env.ai?.state === "ok" && env.ai.model ? ` · AI \`${env.ai.model}\`` : "") +
"</sub>",
)
return lines.join("\n")

if (!env.findings.length) {
lines.push("No issues found in the changed dbt models. 🎉", "")
return [...lines, ...tail].join("\n")
}

// Findings fill the budget left after the fixed parts, most severe first.
// What does not fit is counted, never silently dropped: the envelope keeps
// every finding.
const overflowReserve = 220
let used = [...lines, ...tail].join("\n").length + overflowReserve
let hidden = 0
const grouped = groupBySeverity(env.findings)
for (const sev of ["critical", "warning", "suggestion"] as const) {
const items = grouped[sev]
if (!items.length) continue
const heading = [`### ${SEVERITY_EMOJI[sev]} ${capitalize(sev)} (${items.length})`, ""]
const bullets = bulletsFor(items)
let headed = false
for (const b of bullets) {
const cost = b.text.length + 1 + (headed ? 0 : heading.join("\n").length + 2)
if (used + cost > maxChars) {
hidden += b.count
continue
}
if (!headed) {
lines.push(...heading)
headed = true
}
lines.push(b.text)
used += cost
}
if (headed) lines.push("")
}
if (hidden) {
lines.push(
`> … ${hidden} more finding(s) not shown to keep this comment short — the full list is in the verdict envelope ` +

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: The --output artifact this points at does not exist for the bundled action

github/review/action.yml builds its args from --mode/--manifest/--severity/--cwd/--base/--head/--post only — it never passes --json or --output, so an action user who hits the 5K budget has no --output artifact to open, and renderSummary is the only rendering they see. The uncapped list does reach the step log (the CLI writes renderSummary(env, { maxChars: Infinity }) to stdout), and the envelope is written only for an explicit CLI --output. Point readers at the job log or at altimate review --json --output <file> instead of an artifact the action never creates.

"(`altimate review --json`, or the action's `--output` artifact).",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: 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.

Prompt for AI agents
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>
Suggested change
"(`altimate review --json`, or the action's `--output` artifact).",
"(`altimate review --json`, or the job log; use `--output <file>` when running the CLI directly).",

"",
)
}
return [...lines, ...tail].join("\n")
}

/** A finding's title without its leading `model: `, with backticked names
* 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, "`…`")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 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.

Prompt for AI agents
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>

}

/** Render findings of one severity as bullets; a rule repeated on GROUP_MIN or
* more models collapses into a single bullet listing the models. */
function bulletsFor(items: Finding[]): Array<{ text: string; count: number }> {
const groups = new Map<string, Finding[]>()
for (const f of items) {
const key = `${f.category}|${ruleTitle(f)}`
const g = groups.get(key)
if (g) g.push(f)
else groups.set(key, [f])
}
const out: Array<{ text: string; count: number }> = []
for (const g of groups.values()) {
if (g.length >= GROUP_MIN) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Grouping threshold counts findings, not models

g.length >= GROUP_MIN collapses on the number of findings, but the documented contract (the GROUP_MIN comment at line 42 and the new docs/docs/usage/dbt-pr-review.md text) is "the same rule on three or more models". Because ruleTitle() blanks backticked names, three findings for different columns of one model (m: column \a` has no description, `b`, `c`) collapse into one bullet reading 3 findings across 1 model(s): `m`that keeps onlyg[0]'s body. These findings carry no startLine (qualityLane`/AI are the only line producers), so the summary is their only surface — the other two columns vanish from the PR comment entirely.

Gate the collapse on distinct models (names.length >= GROUP_MIN) and keep per-finding bullets within a single model, or list each merged finding's column/title in the grouped bullet. There is also no test for the one-model case (review-noise.test.ts only covers 3-4 distinct models).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 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.

Prompt for AI agents
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>
Suggested change
if (g.length >= GROUP_MIN) {
if (new Set(g.map((f) => f.model ?? f.file)).size >= GROUP_MIN) {

const names = [...new Set(g.map((f) => f.model ?? f.file))]
const shown = names.slice(0, 8).map((n) => `\`${n}\``).join(", ")
const more = names.length > 8 ? ` (+${names.length - 8} more)` : ""
out.push({
text:
`- **${ruleTitle(g[0])}** — ${g.length} findings across ${names.length} model(s): ${shown}${more} \n` +
` ${oneLine(g[0].body)} \n <sub>${g[0].category}</sub>`,
count: g.length,
})
continue
}
for (const f of g) {
const loc = f.file + (f.startLine ? `:${f.startLine}` : "")
out.push({
text: `- **${f.title}** \n ${oneLine(f.body)} \n <sub>\`${loc}\`${f.degraded ? " · _unverified_" : ""} · ${f.category}</sub>`,
count: 1,
})
}
}
return out
}

/**
Expand All @@ -139,6 +216,11 @@ export function coverageNotes(env: VerdictEnvelope): string[] {
}
if (env.scope?.outOfScopeFiles)
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.",
Comment on lines +221 to +222

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 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.

Prompt for AI agents
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>
Suggested change
`${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.",
`${env.scope.movedUnchangedFiles} file(s) moved without edits; their unchanged contents were not re-reviewed. ` +
"For moved models, folder-level config in `dbt_project.yml` (`+schema`, `+materialized`, `+tags`) can still change where and how they build.",

)
if (env.ai && env.ai.state !== "ok") {
const what = { skipped: "skipped", timeout: "timed out", error: "failed" }[env.ai.state]
notes.push(`Advisory AI review ${what}${env.ai.reason ? `: ${env.ai.reason}` : ""}.`)
Expand All @@ -154,14 +236,28 @@ export interface InlineComment {
body: string
}

export function inlineComments(env: VerdictEnvelope): InlineComment[] {
/** Hidden per-finding marker on inline comments, keyed by the finding's stable
* fingerprint (rule, file, model, column — not the line), so a rerun can tell
* which findings are already on the PR. */
export function findingMarker(id: string): string {
return `<!-- altimate-finding:${id} -->`
}

/** Finding ids already posted, read from existing inline comment bodies. */
export function postedFindingIds(bodies: Array<string | null | undefined>): Set<string> {
const out = new Set<string>()
for (const b of bodies) for (const m of (b ?? "").matchAll(/<!-- altimate-finding:([\w-]+) -->/g)) out.add(m[1])
return out
}

export function inlineComments(env: VerdictEnvelope, alreadyPosted: Set<string> = new Set()): InlineComment[] {
return env.findings
.filter((f) => typeof f.startLine === "number")
.filter((f) => typeof f.startLine === "number" && !alreadyPosted.has(f.id))
.map((f) => ({
path: f.file,
line: f.startLine!,
side: "RIGHT" as const,
body: `${SEVERITY_EMOJI[f.severity]} **${f.title}**\n\n${f.body}`,
body: `${SEVERITY_EMOJI[f.severity]} **${f.title}**\n\n${f.body}\n\n${findingMarker(f.id)}`,
}))
}

Expand Down
87 changes: 82 additions & 5 deletions packages/opencode/src/altimate/review/orchestrate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,65 @@ export interface OrchestrateInput {
layout?: DbtProjectLayout
/** What the review covered: base resolution and files left out of scope. */
scope?: Omit<ReviewScope, "unverified">
/** Drop the regex catalog's cross-engine portability findings (single known engine). */
suppressPortability?: boolean
}

/** A catalog finding tagged as cross-engine portability advice. */
export function isCatalogPortability(f: Finding): boolean {
return f.evidence?.tool === "rule-catalog" && (f.evidence.result as { portability?: boolean })?.portability === true
}

/**
* Portability advice that is noise for a project on `dialect`: dropped unless
* the rule names the engines that support the construct and `dialect` isn't
* one of them — then it is an error on this project's own engine.
*/
export function isSuppressiblePortability(f: Finding, dialect: string): boolean {
if (!isCatalogPortability(f)) return false
const worksOn = (f.evidence?.result as { worksOn?: string[] })?.worksOn
return !worksOn || worksOn.includes(dialect.toLowerCase())
}

/** Line numbers on the head side of a unified diff that GitHub accepts for an
* inline comment: added lines and context lines inside a hunk. */
export function commentableLines(diff: string | undefined): Set<number> {
const out = new Set<number>()
if (!diff) return out
let right = 0
let inHunk = false
const rows = diff.split("\n")
if (rows[rows.length - 1] === "") rows.pop() // the diff's trailing newline, not a line
for (const line of rows) {
const h = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line)
if (h) {
right = Number(h[1])
inHunk = true
continue
}
// File headers (`--- a/…`, `+++ b/…`) only occur outside a hunk.
if (!inHunk || line.startsWith("\\")) continue
// A blank context line may have lost its leading space in transit.
if (line.startsWith("+") || line.startsWith(" ") || line === "") out.add(right++)
else if (!line.startsWith("-")) inHunk = false
}
return out
}

/**
* Keep a finding's line only when it can be commented on: inside the file's
* diff hunks on the head side. Anything else (a line from compiled SQL, an
* unchanged region) moves to the summary instead of making GitHub reject the
* 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)]))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: Parses every changed file's diff even when nothing needs a line

commentableLines is run over all ChangedFile.diff values up front, but only findings that already carry a startLine need their file parsed — and the only producers are qualityLane (non-Jinja SQL) and the AI lane. On a wide PR this is an extra full pass over every hunk of every changed file in memory. Build the set lazily for the files that actually have an anchored finding:

const lines = new Map(
  [...new Set(findings.filter((f) => typeof f.startLine === "number").map((f) => f.file))].map((p) => [
    p,
    commentableLines(files.find((f) => f.path === p)?.diff),
  ]),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Prompt for AI agents
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>
Suggested change
const lines = new Map(files.map((f) => [f.path, commentableLines(f.diff)]))
const filesNeedingLines = new Set(findings.filter((f) => typeof f.startLine === "number").map((f) => f.file))
const lines = new Map(
files.filter((f) => filesNeedingLines.has(f.path)).map((f) => [f.path, commentableLines(f.diff)]),
)

return findings.map((f) => {
if (typeof f.startLine !== "number") return f
if (lines.get(f.file)?.has(f.startLine)) return f
const { startLine: _s, endLine: _e, ...rest } = f
return rest as Finding
})
}

/** True when SQL text contains Jinja (`{{ }}`, `{% %}` or `{# #}`). */
Expand Down Expand Up @@ -395,6 +454,8 @@ async function qualityLane(
sql: string | undefined,
dialect: string,
oldSql?: string,
/** True when `sql` is the raw file; compiled-SQL line numbers do not exist in the PR's file. */
linesMatchFile = false,
): Promise<Finding[]> {
if (!sql || file.status === "deleted") return []
const model = modelNameFromPath(file.path)
Expand Down Expand Up @@ -432,8 +493,8 @@ async function qualityLane(
body: issue.message,
file: file.path,
model,
startLine: issue.line,
endLine: issue.line,
startLine: linesMatchFile ? issue.line : undefined,
endLine: linesMatchFile ? issue.line : undefined,
evidence: { tool: "altimate_core.check", result: issue },
// Include the message so distinct instances of the same rule (e.g. two
// different non-portable functions) don't collapse under one fingerprint.
Expand Down Expand Up @@ -1117,7 +1178,8 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
)

// A run is degraded when model files exist but none resolved against a manifest.
const runDegraded = modelFiles.length > 0 ? !anyManifest : reviewable.length === 0
// Nothing to resolve (yml-only, pure moves) is not a lint-only run.
const runDegraded = modelFiles.length > 0 && !anyManifest

// High-risk path tokens are user-configured (billing/pci/patient/etc.) —
// the reviewer core carries no default list. `undefined` when no
Expand Down Expand Up @@ -1186,7 +1248,9 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
const tasks: Promise<Finding[]>[] = []
// 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))
tasks.push(
qualityLane(ctx.file, input.runner, ctx.engineNewSql, dialect, ctx.engineOldSql, ctx.engineNewSql === ctx.newSql),
)
if (lanes.has("semantic_change")) {
tasks.push(dataDiffLane(ctx, input.runner, dialect, input.config.dataDiff))
tasks.push(grainMismatchLane(ctx, input.runner))
Expand Down Expand Up @@ -1228,9 +1292,17 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
// classify (pre-existing columns, or ones core's classifier missed) —
// dedup against the columns core actually CONSIDERED, never all
// introduced columns, so a missed column still surfaces here.
const fallbackPii = diffScopedPii.completed
let fallbackPii = diffScopedPii.completed
? currentCtx.pii.filter((col) => !diffScopedPii.classifiedColumns.has(col.toLowerCase()))
: currentCtx.pii
// Only columns this PR newly exposes: a PII column the base already
// surfaced is not this change's concern (the docs promise "newly expose").
if (fallbackPii.length && hasOldSide(currentCtx.file) && currentCtx.engineOldSql) {
const basePii = new Set(
(await input.runner.detectPii(currentCtx.engineOldSql, dialect)).columns.map((c) => c.toLowerCase()),
)
fallbackPii = fallbackPii.filter((col) => !basePii.has(col.toLowerCase()))
}
findings.push(...piiLane(currentCtx.file, fallbackPii))
}
return findings
Expand Down Expand Up @@ -1448,6 +1520,11 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
// Flatten, drop excluded, dedupe, threshold-filter.
let findings = dedupe(merged)
findings = findings.filter((f) => !exclusionReason(f, input.rubric))
// Cross-engine portability advice from the regex catalog is noise for a
// project that runs on one known engine. Core's dialect-aware check (a
// function missing on THIS engine) is a real error and is kept.
if (input.suppressPortability) findings = findings.filter((f) => !isSuppressiblePortability(f, dialect))
findings = anchorToDiff(findings, input.changedFiles)
const minSev = SEVERITY_ORDER[input.config.severityThreshold]
findings = findings.filter((f) => SEVERITY_ORDER[f.severity] >= minSev)
// Sort by severity desc, then file.
Expand Down
Loading
Loading