From e68d785c2fee51f9d405cbecbefade6230372e93 Mon Sep 17 00:00:00 2001
From: Haider <haider@altimate.ai>
Date: Mon, 20 Jul 2026 21:40:50 +0530
Subject: [PATCH 1/6] feat(review): observability + recall (Round 18, B1 + G1 +
 G2 + G3 + G6)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Round-17 bench established three baselines:
- A1-default: 7/64 recall (11%), 4% precision
- A1-workaround (with --manifest): 3/8 on applicable jaffle scenarios
- Baseline B: 0/64 (plugin never invoked the CLI — separately fixed)

This ships the six review-side changes documented as improvement paths
in the Round-18 plan. Each is guarded to avoid regressing the current
customer numbers.

## B1 — bare `--no-ai` triggers yargs help path

yargs' automatic `--no-<option>` negation collides with the option
literally named `no-ai`: bare `--no-ai` is interpreted as "set
undeclared option `ai` to false" and the command silently falls to the
help path (exit 0, no review runs). Every `--no-ai` invocation across
the bench was affected — our earlier A1 numbers were captured with
`--no-ai --yolo` but the flag was inert and the AI lane always ran.

Fix: `.parserConfiguration({ "boolean-negation": false })` on the
review command's yargs builder. `--no-ai` now binds to the declared
`noAi` flag as authored. `--no-ai=true` / `--no-ai=false` still work
for programmatic parity.

## G1 — `--explain-tier` flag

Adds a boolean flag that surfaces the tier classifier's reason list
on the verdict envelope (`tierReasons: string[]`) and in the human-
readable render (`> 🧭 **Tier: X** — ...`). Read-only — doesn't
change tier classification or verdicts. When the flag is off, the
envelope omits `tierReasons` entirely (backwards compatible; existing
consumers unaffected).

## G2 — `--force-tier <trivial|lite|full>` flag

EXPERIMENTAL / bench-debug only. Overrides the classifier's tier so
we can measure the tier-gate contribution to recall misses (e.g. js2
gets `trivial/0` — can't tell if the catalog missed vs. the tier gate
filtered it out). Guardrails:

- Prints an "EXPERIMENTAL (bench / debug only)" warning to stderr
  every use.
- Envelope carries `tierForced: true` and `tierClassified: <original>`
  so audits can see the bypass.
- `tierReasons` is force-populated with a leading marker even if
  `--explain-tier` isn't set, so the renderer always shows the tier
  was forced.
- Verdict header renders as `full tier — forced (was trivial)`.

Not a default-visible feature. Documented as debug-only in the yargs
`describe`, and the stderr banner ensures no customer accidentally
uses it in CI without noticing.

## G3 — manifest auto-discovery + freshness warning

Before: `--manifest <path>` had to be passed explicitly. Otherwise
the CLI used the config-relative default `target/manifest.json`,
which silently missed whenever `cwd` wasn't exactly the dbt project
root — every such review degraded to lint-only.

After (in `run.ts`):

1. If `--manifest` isn't explicit AND the config-relative path
   doesn't exist, walk UP from `cwd` looking for `dbt_project.yml`.
   Use the adjacent `target/manifest.json` when it exists.
2. Log the discovery to stderr (`ℹ️  auto-discovered dbt manifest at
   ...`) so customers see which manifest the review used.
3. Refuse to auto-discover a manifest from a directory that doesn't
   contain a dbt project — a `target/` from an unrelated tool (e.g.
   Airflow) never gets picked up.
4. Freshness warning: when `--head` is set (CI / bench shape) AND
   any changed file has an mtime newer than the manifest, print a
   `⚠️  manifest ... appears stale` message to stderr. Skipped for
   working-tree diffs because mtime noise during active edits would
   spam warnings.

Explicit `--manifest <path>` always wins. Auto-discovery is only
attempted when the caller was silent AND the config default is absent.

## G6 — column-aware schema.yml test-removal detector

Before: the detector compared removed vs added test lines as bare
strings (`- unique`, `- not_null`). Any sibling column that STILL had
the same test type re-added its line under a different indent (yaml
re-serialization), silently cancelling the genuine removal from a
different column. js2 (drops `unique` + `not_null` from
`customers.customer_id` while `orders.order_id` still has them) hit
this exact bug: 0/2 recall in both A1 modes across Round 17.

After: `collectTestOccurrencesFromDiff` walks the unified diff
tracking `(model, column)` context via nearest preceding
`- name: <X>` headers and hunk boundaries. Removed tests are keyed by
`(model, column, test)` and only cancelled by a re-add on the SAME
tuple. One finding per removal (not one summary), with severity
elevated to `warning` for `unique` removals and for `not_null` on
`_id` / `_key` / `id` columns (silent-PK-null risk).

Verified on js2 locally: 0 findings → 2 warnings.

## Verdict envelope schema

`VerdictEnvelope` gains three optional fields, all included in the
signed canonical body so tampering is detectable:

- `tierReasons?: string[]` — from G1 (surfaces classifier reasons)
- `tierForced?: boolean` — from G2
- `tierClassified?: RiskTier` — from G2 (original tier before force)

All are optional and absent in the default (no-flag) case, so
existing verdicts remain byte-equivalent to before.

## What's NOT in this change

- G4 (`verify` subcommand) — deferred; doesn't move measured metrics.
- G5 (`--commit` / `--pr` / `--files`) — deferred; adoption UX only.
- G7 (severity mapping tune-up) — deferred; needs its own design pass.
- Real-MR taxonomy expansion — larger design lift; addressed in a
  later round.

## Follow-up

Rerun A1-default and A1-workaround on the 13-scenario corpus with
these changes and publish the deltas as Round 18 in the reviewer
plugin journal.
---
 .../src/altimate/review/dbt-patterns.ts       | 176 +++++++++++++++---
 .../opencode/src/altimate/review/format.ts    |  13 +-
 .../src/altimate/review/orchestrate.ts        |  21 ++-
 packages/opencode/src/altimate/review/run.ts  | 101 +++++++++-
 .../opencode/src/altimate/review/verdict.ts   |  16 ++
 packages/opencode/src/cli/cmd/review.ts       |  32 +++-
 6 files changed, 324 insertions(+), 35 deletions(-)

diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts
index 85b475aba..9f989aca9 100644
--- a/packages/opencode/src/altimate/review/dbt-patterns.ts
+++ b/packages/opencode/src/altimate/review/dbt-patterns.ts
@@ -790,33 +790,159 @@ export function detectModelPatterns(file: ChangedFile, newSql: string | undefine
  * Detect removal of `unique` / `not_null` / `relationships` tests in a schema.yml
  * diff — the guardrail that catches fan-out/dupes is the thing being deleted.
  */
+/** Walk a unified diff and, for each removed test-line, reconstruct which
+ *  `(model, column)` context it belonged to on the OLD side. Uses hunk headers
+ *  to reset line counters and tracks the nearest `- name:` header preceding
+ *  each line. Yields `{ model, column, test }` tuples for every removed
+ *  `unique | not_null | relationships` — including bare `- <test>` lines and
+ *  block-form `- <test>:` lines (relationships with args). Round-18 G6. */
+function collectTestOccurrencesFromDiff(diff: string | undefined, side: "-" | "+"): Set<string> {
+  // Returned as a Set of canonical `${model}\x00${column}\x00${test}` keys so
+  // callers can compare removed vs added sets without re-parsing.
+  const out = new Set<string>()
+  if (!diff) return out
+  let currentModel = ""
+  let currentColumn = ""
+  // Track indentation depth of the CURRENT model/column so we can pop when we
+  // dedent past them. A `- name: X` at indent I claims all lines at indent > I.
+  let modelIndent = -1
+  let columnIndent = -1
+  const modelRe = /^(\s*)-\s*name:\s*([A-Za-z0-9_.\-]+)\s*$/
+  const testRe = /^(\s*)-\s*(unique|not_null|relationships)\b(?::)?/i
+  for (const raw of diff.split("\n")) {
+    // Diff hunk header — reset context; don't try to interpret hunk offsets.
+    if (raw.startsWith("@@")) {
+      currentModel = ""
+      currentColumn = ""
+      modelIndent = -1
+      columnIndent = -1
+      continue
+    }
+    // We consider both context and same-side lines for STRUCTURE (they define
+    // the model/column context) but only same-side lines for TEST detection.
+    let payload: string
+    let onOurSide: boolean
+    if (raw.startsWith(side)) {
+      // remove +/- prefix
+      payload = raw.slice(1)
+      onOurSide = true
+    } else if (raw.startsWith(side === "-" ? "+" : "-") || raw.startsWith("+++") || raw.startsWith("---")) {
+      // The other side's insertion/deletion — don't read structure from it,
+      // don't produce test occurrences from it.
+      continue
+    } else {
+      // Context line (leading space or none).
+      payload = raw.startsWith(" ") ? raw.slice(1) : raw
+      onOurSide = false
+    }
+    // Pop column/model when we've dedented past them.
+    const leadingWs = payload.match(/^(\s*)/)![1].length
+    // Only pop for non-empty payload lines (blank lines carry no indent info).
+    if (payload.trim()) {
+      if (columnIndent >= 0 && leadingWs <= columnIndent) {
+        currentColumn = ""
+        columnIndent = -1
+      }
+      if (modelIndent >= 0 && leadingWs <= modelIndent) {
+        currentModel = ""
+        modelIndent = -1
+      }
+    }
+    // Detect `- name: X` — could be either a model header or a column header;
+    // disambiguate by whether we already have a model in scope.
+    const nameMatch = payload.match(modelRe)
+    if (nameMatch) {
+      const wsLen = nameMatch[1].length
+      const name = nameMatch[2]
+      // If we have no model, or this header is at the same/less indent than
+      // the current model header, treat as a new model.
+      if (currentModel === "" || wsLen <= modelIndent) {
+        currentModel = name
+        modelIndent = wsLen
+        currentColumn = ""
+        columnIndent = -1
+      } else {
+        // Deeper than the model header → column of the current model.
+        currentColumn = name
+        columnIndent = wsLen
+      }
+      continue
+    }
+    // Detect a test on OUR side and within a column context.
+    if (onOurSide && currentModel && currentColumn) {
+      const t = payload.match(testRe)
+      if (t) {
+        const test = t[2].toLowerCase()
+        out.add(`${currentModel}\x00${currentColumn}\x00${test}`)
+      }
+    }
+  }
+  return out
+}
+
 export function detectSchemaYmlPatterns(file: ChangedFile, rubric: Rubric): Finding[] {
   const kind = classifyDbtFile(file.path)
   if (kind !== "schema_yml" || file.status === "deleted") return []
-  const { added, removed } = splitDiff(file.diff)
-  const removedTests = removed.filter((l) => /^\s*-\s*(unique|not_null|relationships)\b/i.test(l))
-  // A test still present (just moved) shouldn't fire.
-  const addedTests = new Set(added.map((l) => l.trim()))
-  const genuinelyRemoved = removedTests.filter((l) => !addedTests.has(l.trim()))
+
+  // COLUMN-AWARE test-removal detection (G6 fix, Round 18). The previous
+  // string-set dedup collapsed `- unique` on ANY column into the "still
+  // present" bucket, hiding a genuine removal from column X whenever a
+  // sibling column Y still had `- unique`. This walks the diff with model/
+  // column context so a removal is only cancelled by re-appearance on the
+  // SAME `(model, column, test)` tuple.
+  const removedByCtx = collectTestOccurrencesFromDiff(file.diff, "-")
+  const addedByCtx = collectTestOccurrencesFromDiff(file.diff, "+")
+  const genuinelyRemoved: Array<{ model: string; column: string; test: string }> = []
+  for (const key of removedByCtx) {
+    if (addedByCtx.has(key)) continue
+    const [model, column, test] = key.split("\x00")
+    genuinelyRemoved.push({ model, column, test })
+  }
   if (!genuinelyRemoved.length) return []
-  const removedUnique = genuinelyRemoved.some((l) => /\bunique\b/i.test(l))
-  const f = makeFinding({
-    severity: removedUnique ? "warning" : "suggestion",
-    category: "test_coverage",
-    title: `${file.path.split("/").pop()}: removed ${genuinelyRemoved.length} data test(s)`,
-    body:
-      "This change deletes `unique`/`not_null`/`relationships` test(s) — the guardrails that catch fan-out, duplicate, " +
-      "and broken-FK regressions. Removing a `unique` test on a grain key is how silent duplicate bugs ship. " +
-      "Confirm the test is genuinely obsolete, not removed to make CI green.",
-    file: file.path,
-    confidence: "high",
-    evidence: {
-      tool: "dbt-patterns",
-      result: { rule: "removed_tests", removed: genuinelyRemoved.map((l) => l.trim()) },
-    },
-    ruleKey: "test_coverage:removed-tests",
-  })
-  return [{ ...f, severity: clampSeverity(f.category, f.severity, f.confidence) }].filter(
-    (x) => !exclusionReason(x, rubric),
-  )
+
+  // Emit ONE finding per (column, test) removal for precision — customers
+  // can see exactly which guardrail was dropped, not just a total count.
+  const findings: Finding[] = []
+  const seenModel = new Set<string>()
+  for (const r of genuinelyRemoved) {
+    const isUniquenessSignal = r.test === "unique"
+    const notNullOnLikelyPK = r.test === "not_null" && /(_id$|^id$|_key$)/i.test(r.column)
+    // Uniqueness OR not_null on an id/key column = warning (silent-dup / null-PK
+    // risk); other not_null / relationships removals = suggestion.
+    const sev: Severity = isUniquenessSignal || notNullOnLikelyPK ? "warning" : "suggestion"
+    const key = `${r.model}.${r.column}`
+    const isFirstForModel = !seenModel.has(r.model)
+    seenModel.add(r.model)
+    findings.push(
+      makeFinding({
+        severity: clampSeverity("test_coverage", sev, "high"),
+        category: "test_coverage",
+        title: `${file.path.split("/").pop()}: ${r.test} test removed from ${key}`,
+        body:
+          `The \`${r.test}\` data test was removed from column \`${r.column}\` on model \`${r.model}\`. ` +
+          (isUniquenessSignal
+            ? `Removing a \`unique\` test on a mart-layer key is how silent duplicate rows ship — ` +
+              `downstream joins fan out, aggregates double-count, and no test catches it. Restore ` +
+              `the test, or explicitly document why the grain no longer needs to be unique on this column.`
+            : notNullOnLikelyPK
+            ? `Removing \`not_null\` on what looks like an identifier column (\`${r.column}\`) means nulls ` +
+              `will slip through into downstream joins and aggregates. Restore the test unless the column ` +
+              `is genuinely nullable now.`
+            : `Removing an existing data test is a silent-regression risk. Confirm the test is genuinely ` +
+              `obsolete, not dropped to make CI green.`) +
+          (isFirstForModel && genuinelyRemoved.length > 1
+            ? `\n\n_This PR removes ${genuinelyRemoved.length} data tests in total on model(s) ` +
+              `${[...new Set(genuinelyRemoved.map((x) => x.model))].map((m) => `\`${m}\``).join(", ")}._`
+            : ""),
+        file: file.path,
+        confidence: "high",
+        evidence: {
+          tool: "dbt-patterns",
+          result: { rule: "removed_tests", model: r.model, column: r.column, test: r.test },
+        },
+        ruleKey: `test_coverage:removed-tests:${r.model}.${r.column}.${r.test}`,
+      }),
+    )
+  }
+  return findings.filter((x) => !exclusionReason(x, rubric))
 }
diff --git a/packages/opencode/src/altimate/review/format.ts b/packages/opencode/src/altimate/review/format.ts
index c6bebbc4e..f4b788cd2 100644
--- a/packages/opencode/src/altimate/review/format.ts
+++ b/packages/opencode/src/altimate/review/format.ts
@@ -28,7 +28,8 @@ export function verdictHeadline(env: VerdictEnvelope): string {
     [critical && `${critical} critical`, warning && `${warning} warning`, suggestion && `${suggestion} suggestion`]
       .filter(Boolean)
       .join(", ") || "no findings"
-  return `${VERDICT_LABEL[env.verdict]} — ${counts} (${env.tier} tier)`
+  const tierLabel = env.tierForced ? `${env.tier} tier — forced (was ${env.tierClassified})` : `${env.tier} tier`
+  return `${VERDICT_LABEL[env.verdict]} — ${counts} (${tierLabel})`
 }
 
 /** Full PR/MR summary comment body (markdown), prefixed with the dedup marker. */
@@ -43,6 +44,16 @@ export function renderSummary(env: VerdictEnvelope): string {
     )
   }
 
+  // G1 (Round 18) — surface classifier reasons when --explain-tier populated
+  // them, so a customer can see why the review ran at this tier. Also surfaces
+  // when --force-tier bypassed the classifier (tierReasons is auto-populated).
+  if (env.tierReasons && env.tierReasons.length) {
+    lines.push(
+      `> 🧭 **Tier: ${env.tier}** — ${env.tierReasons.map((r) => `\`${r}\``).join(", ")}`,
+      "",
+    )
+  }
+
   if (!env.findings.length) {
     lines.push("No issues found in the changed dbt models. 🎉", "")
   } else {
diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts
index 68cefafde..0fb5b5eb9 100644
--- a/packages/opencode/src/altimate/review/orchestrate.ts
+++ b/packages/opencode/src/altimate/review/orchestrate.ts
@@ -190,6 +190,10 @@ export interface OrchestrateInput {
   /** PR metadata passed to the AI reviewer for intent checking. */
   prTitle?: string
   prBody?: string
+  /** G1 — attach the classifier's reason list to the envelope. */
+  explainTier?: boolean
+  /** G2 — override the classifier's tier decision. Envelope carries tierForced:true. */
+  forceTier?: "trivial" | "lite" | "full"
 }
 
 /** Derive the dbt model name from a model file path. */
@@ -1088,14 +1092,24 @@ 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
 
-  const tier = classifyPR(reviewable, {
+  const tierResult = 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,
-  }).tier
+  })
+  const classifiedTier = tierResult.tier
+  // G2 — --force-tier overrides the classifier. Envelope records both the forced
+  // tier and the original classification so audits can see the bypass; the
+  // reasons list gets a leading "forced" marker so downstream doesn't confuse
+  // the forced tier for a natural one.
+  const tier = input.forceTier ?? classifiedTier
+  const tierForced = input.forceTier !== undefined && input.forceTier !== classifiedTier
+  const tierReasons = tierForced
+    ? [`forced via --force-tier (classifier said ${classifiedTier})`, ...tierResult.reasons]
+    : tierResult.reasons
 
   const lanes = new Set(input.config.reviewers.length ? input.config.reviewers : TIER_LANES[tier])
 
@@ -1349,6 +1363,9 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
     manifestHash: input.manifestHash,
     generatedAt: input.generatedAt,
     degraded,
+    tierReasons: input.explainTier || tierForced ? tierReasons : undefined,
+    tierForced: tierForced ? true : undefined,
+    tierClassified: tierForced ? classifiedTier : undefined,
   })
   return signEnvelope(envelope)
 }
diff --git a/packages/opencode/src/altimate/review/run.ts b/packages/opencode/src/altimate/review/run.ts
index 95b5c1589..5d3c7f50a 100644
--- a/packages/opencode/src/altimate/review/run.ts
+++ b/packages/opencode/src/altimate/review/run.ts
@@ -1,5 +1,5 @@
 import path from "node:path"
-import { readFile } from "node:fs/promises"
+import { readFile, stat, access } from "node:fs/promises"
 import { loadReviewConfig, resolveRubric } from "./config"
 import type { Severity } from "./finding"
 import { collectChangedFiles, makeContentResolver, defaultBaseRef, manifestHash } from "./git"
@@ -42,6 +42,11 @@ export interface ReviewPullRequestOptions {
   /** PR metadata for the AI reviewer's intent check. */
   prTitle?: string
   prBody?: string
+  /** G1 — surface the tier classifier's reasons on the verdict envelope. */
+  explainTier?: boolean
+  /** G2 — [EXPERIMENTAL] bypass the tier classifier with the supplied tier.
+   *  Envelope carries `tierForced: true` when used. Debug / bench only. */
+  forceTier?: "trivial" | "lite" | "full"
 }
 
 /** dbt adapter_type → core SQL dialect. Mostly identity; a few aliases. */
@@ -73,6 +78,68 @@ async function detectDialect(manifestAbs: string): Promise<string | undefined> {
   }
 }
 
+/**
+ * G3 (Round 18) — walk upward from `cwd` looking for a dbt project, then check
+ * for the compiled `target/manifest.json` next to it. Emits an absolute path
+ * only when the manifest exists AND the file is under the dbt project's parent
+ * (defensive against relative escapes and stale manifests from unrelated
+ * projects on the same filesystem). Returns undefined when nothing matches.
+ *
+ * We DO NOT re-run `dbt compile` here — that could touch a real warehouse or
+ * consume credentials. The customer is still responsible for a compiled
+ * artifact; we just find it when they compiled but didn't pass `--manifest`.
+ */
+async function autoDiscoverManifest(cwd: string): Promise<{ path: string; projectRoot: string } | undefined> {
+  let dir = path.resolve(cwd)
+  const root = path.parse(dir).root
+  // Walk up looking for dbt_project.yml so the manifest we auto-discover is
+  // demonstrably tied to a dbt project (never `target/manifest.json` from an
+  // unrelated cwd, e.g. Airflow's `target/`).
+  while (true) {
+    for (const fn of ["dbt_project.yml", "dbt_project.yaml"]) {
+      try {
+        await access(path.join(dir, fn))
+        const candidate = path.join(dir, "target", "manifest.json")
+        try {
+          await access(candidate)
+          return { path: candidate, projectRoot: dir }
+        } catch {
+          return undefined // dbt project found but not compiled — respect that
+        }
+      } catch {
+        /* keep walking */
+      }
+    }
+    if (dir === root) return undefined
+    dir = path.dirname(dir)
+  }
+}
+
+/** Warn to stderr when the manifest looks stale relative to changed files. */
+async function warnIfStale(manifestAbs: string, changedPaths: string[], cwd: string): Promise<void> {
+  try {
+    const manifestMtime = (await stat(manifestAbs)).mtimeMs
+    for (const rel of changedPaths) {
+      // Only compare against tracked, on-disk model files (schema.yml or SQL)
+      const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel)
+      try {
+        const changedMtime = (await stat(abs)).mtimeMs
+        if (changedMtime > manifestMtime) {
+          process.stderr.write(
+            `⚠️  manifest ${manifestAbs} appears stale — ${rel} was modified after the manifest was written. ` +
+              `Re-run \`dbt compile\` (or \`dbt build\`) to refresh before reviewing.\n`,
+          )
+          return
+        }
+      } catch {
+        /* file not on disk (e.g. removed by the change) — skip */
+      }
+    }
+  } catch {
+    /* manifest unreadable → detectDialect will have already returned undefined */
+  }
+}
+
 export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise<VerdictEnvelope> {
   const config = await loadReviewConfig(opts.cwd)
   if (opts.manifestPath) config.manifestPath = opts.manifestPath
@@ -97,9 +164,37 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
   // Resolve the manifest against the PROJECT being reviewed (cwd), not the
   // binary's process.cwd() — otherwise a relative path silently misses when the
   // CLI is invoked from elsewhere, degrading every review to lint-only.
-  const manifestAbs = path.isAbsolute(config.manifestPath)
+  let manifestAbs = path.isAbsolute(config.manifestPath)
     ? config.manifestPath
     : path.join(opts.cwd, config.manifestPath)
+  // G3 (Round 18) — auto-discovery when the caller didn't pass `--manifest` and
+  // the config-relative resolve above doesn't point at an existing file. Walk
+  // upward for the dbt project root and use its `target/manifest.json`. Explicit
+  // `--manifest` always wins (see run.ts opts.manifestPath override at the top).
+  if (!opts.manifestPath) {
+    let manifestExists = false
+    try {
+      await access(manifestAbs)
+      manifestExists = true
+    } catch {
+      /* fall through to auto-discovery */
+    }
+    if (!manifestExists) {
+      const discovered = await autoDiscoverManifest(opts.cwd)
+      if (discovered) {
+        manifestAbs = discovered.path
+        process.stderr.write(
+          `ℹ️  auto-discovered dbt manifest at ${discovered.path} ` +
+            `(dbt project root: ${discovered.projectRoot}). Pass --manifest to override.\n`,
+        )
+      }
+    }
+  }
+  // Freshness check: warn (don't fail) when the manifest predates changed files.
+  // Skip when we're diffing against the working tree (mtime signal is noisy
+  // during active edits) — only warn when the caller explicitly pinned a head
+  // ref, which is the CI / bench shape where a stale manifest is a real risk.
+  if (opts.head) await warnIfStale(manifestAbs, changedFiles.map((f) => f.path), opts.cwd)
 
   // Resolve the SQL dialect: explicit config wins; otherwise auto-detect from
   // the dbt manifest's `adapter_type` (so a BigQuery/Redshift project isn't
@@ -135,5 +230,7 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
     aiReview: opts.noAi || config.ai === false ? undefined : runAiReview,
     prTitle: opts.prTitle,
     prBody: opts.prBody,
+    explainTier: opts.explainTier,
+    forceTier: opts.forceTier,
   })
 }
diff --git a/packages/opencode/src/altimate/review/verdict.ts b/packages/opencode/src/altimate/review/verdict.ts
index d499695d9..3f07a04cf 100644
--- a/packages/opencode/src/altimate/review/verdict.ts
+++ b/packages/opencode/src/altimate/review/verdict.ts
@@ -92,6 +92,13 @@ export const VerdictEnvelope = z.object({
   idealVerdict: Verdict,
   mode: ReviewMode,
   tier: RiskTier,
+  /** G1 — reasons the classifier assigned this tier (only when --explain-tier). */
+  tierReasons: z.array(z.string()).optional(),
+  /** G2 — true when --force-tier bypassed the classifier. Included in signature so
+   *  a tampered envelope claiming natural tier can't fake a forced run. */
+  tierForced: z.boolean().optional(),
+  /** The classifier's original tier before --force-tier overrode it (G2). */
+  tierClassified: RiskTier.optional(),
   findings: z.array(Finding),
   summary: z.object({
     critical: z.number().int().nonnegative(),
@@ -127,6 +134,12 @@ export interface BuildEnvelopeInput {
   manifestHash?: string
   generatedAt?: string
   degraded?: boolean
+  /** G1 — classifier reasons for the tier (only surfaced when explainTier=true). */
+  tierReasons?: string[]
+  /** G2 — set when --force-tier was applied. */
+  tierForced?: boolean
+  /** G2 — classifier's original tier before the force override. */
+  tierClassified?: RiskTier
 }
 
 function summarize(findings: Finding[], degraded: boolean): VerdictEnvelope["summary"] {
@@ -147,6 +160,9 @@ export function buildEnvelope(input: BuildEnvelopeInput): VerdictEnvelope {
     idealVerdict: ideal,
     mode: input.mode,
     tier: input.tier,
+    tierReasons: input.tierReasons,
+    tierForced: input.tierForced,
+    tierClassified: input.tierClassified,
     findings: input.findings,
     summary: summarize(input.findings, degraded),
     engine: EngineVersions.parse(input.engine ?? {}),
diff --git a/packages/opencode/src/cli/cmd/review.ts b/packages/opencode/src/cli/cmd/review.ts
index c271dbe5f..2a2f9c07f 100644
--- a/packages/opencode/src/cli/cmd/review.ts
+++ b/packages/opencode/src/cli/cmd/review.ts
@@ -22,9 +22,15 @@ export const ReviewCommand = cmd({
   describe: "review dbt/SQL changes and emit a signed verdict (APPROVE/COMMENT/REQUEST_CHANGES)",
   builder: (yargs) =>
     yargs
+      // Disable yargs' `--no-<option>` auto-negation for this command. Without this,
+      // yargs interprets bare `--no-ai` as "set option `ai` to false" — which is
+      // NOT a declared option, so the command silently falls into the help path
+      // (exit 0, no review runs). With `boolean-negation: false`, `--no-ai` binds
+      // to the declared `noAi` boolean flag as authored. B1 fix, Round 18.
+      .parserConfiguration({ "boolean-negation": false })
       .option("base", { type: "string", describe: "base git ref (default: merge-base with origin/main)" })
       .option("head", { type: "string", describe: "head git ref (default: working tree)" })
-      .option("manifest", { type: "string", describe: "path to dbt manifest.json" })
+      .option("manifest", { type: "string", describe: "path to dbt manifest.json (auto-discovered under target/ when omitted)" })
       .option("mode", {
         type: "string",
         choices: ["comment", "gate"] as const,
@@ -47,9 +53,25 @@ export const ReviewCommand = cmd({
         default: false,
         describe: "disable the advisory LLM reviewer lane (no model calls / cost)",
       })
+      .option("explain-tier", {
+        type: "boolean",
+        default: false,
+        describe: "emit tierReasons[] in the verdict envelope explaining the tier classification",
+      })
+      .option("force-tier", {
+        type: "string",
+        choices: ["trivial", "lite", "full"] as const,
+        describe: "[EXPERIMENTAL / bench debug] override the tier classifier — verdict envelope carries tierForced: true",
+      })
       .option("cwd", { type: "string", describe: "project directory (default: current dir)" }),
   async handler(args) {
     const cwd = (args.cwd as string) || process.cwd()
+    if (args.forceTier) {
+      process.stderr.write(
+        `⚠️  --force-tier=${args.forceTier} is EXPERIMENTAL (bench / debug only). ` +
+          `Classifier bypassed; verdict envelope will carry tierForced: true.\n`,
+      )
+    }
     await bootstrap(cwd, async () => {
       const env = await reviewPullRequest({
         cwd,
@@ -58,11 +80,11 @@ export const ReviewCommand = cmd({
         manifestPath: args.manifest as string | undefined,
         mode: args.mode as ReviewMode | undefined,
         severityThreshold: args.severity as Severity | undefined,
-        // The flag is registered as `--no-ai`, so yargs sets `args.noAi`. No `ai`
-        // option is declared, so `--ai=false` is NOT a supported CLI flag; the
-        // `args.ai === false` check only covers programmatic callers that pass
-        // `ai: false` directly.
+        // With `boolean-negation: false` above, `--no-ai` binds to `noAi` and
+        // the historical `--ai=false` programmatic path stays supported.
         noAi: args.noAi === true || args.ai === false,
+        explainTier: args.explainTier === true,
+        forceTier: args.forceTier as "trivial" | "lite" | "full" | undefined,
       })
 
       if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2))

From 98a0439842c94c6bc3a65fda5196bbb41e42e456 Mon Sep 17 00:00:00 2001
From: Haider <haider@altimate.ai>
Date: Tue, 21 Jul 2026 02:29:47 +0530
Subject: [PATCH 2/6] =?UTF-8?q?fix(review):=20Codex=20R18=20review=20follo?=
 =?UTF-8?q?w-ups=20=E2=80=94=20G2=20audit=20guardrail=20+=20G6=20copy?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Independent code review by Codex (2026-07-21) surfaced two issues in
the R18 fixes:

1. G2 guardrail bug — `tierForced` was only set to `true` when the
   forced tier value happened to DIFFER from the classifier's
   decision. So `altimate-code review --force-tier=full` on a PR the
   classifier would naturally rate `full` still runs the debug bypass
   (silently exercises the flag path in orchestrate.ts) but emits an
   envelope with no `tierForced`, no `tierClassified`, and no
   "forced via" reason — breaking the audit-trail invariant we
   documented in the R18 commit.

   Fix: `tierForced = input.forceTier !== undefined`. Whenever the
   caller passed the flag, the envelope now records the bypass
   regardless of whether the forced value matched the natural one.
   The leading reason string now also names the forced value.

   Verified on js4 (naturally `full`): `--force-tier=full` now
   produces `tierForced: true, tierClassified: "full", tierReasons:
   ["forced via --force-tier=full (classifier said full)", ...]`.

2. G6 cosmetic copy issue — the schema.yml test-removal detector's
   body claimed "Removing a `unique` test on a mart-layer key is how
   silent duplicate rows ship" for EVERY unique-test removal, even
   when the schema.yml lives outside a mart layer (e.g. js2's
   `models/schema.yml` sits at the top level, not under `models/marts/`).

   Fix: detect the layer from the file path (`marts?` or `reporting`
   → "mart-layer"; otherwise "declared") and interpolate that into
   the body. The finding is still accurate for genuine mart-layer
   removals; the copy no longer misattributes for staging or
   top-level schema files.

Neither change alters what findings are surfaced, only the audit
envelope shape (G2) and finding body prose (G6). No test-file
changes; smoke tests on js2 + js4 confirm both.
---
 .../src/altimate/review/dbt-patterns.ts       |  7 ++++++-
 .../src/altimate/review/orchestrate.ts        | 19 +++++++++++++------
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts
index 9f989aca9..2e46d58a8 100644
--- a/packages/opencode/src/altimate/review/dbt-patterns.ts
+++ b/packages/opencode/src/altimate/review/dbt-patterns.ts
@@ -910,6 +910,11 @@ export function detectSchemaYmlPatterns(file: ChangedFile, rubric: Rubric): Find
     // Uniqueness OR not_null on an id/key column = warning (silent-dup / null-PK
     // risk); other not_null / relationships removals = suggestion.
     const sev: Severity = isUniquenessSignal || notNullOnLikelyPK ? "warning" : "suggestion"
+    // Detect layer from file path so the finding body doesn't misattribute
+    // (Codex-R18-review noted the previous copy said "mart-layer key" on
+    // every unique removal regardless of the schema.yml's actual layer).
+    const isMartLayer = /(^|\/)(marts?|reporting)\//.test(file.path)
+    const layerLabel = isMartLayer ? "mart-layer" : "declared"
     const key = `${r.model}.${r.column}`
     const isFirstForModel = !seenModel.has(r.model)
     seenModel.add(r.model)
@@ -921,7 +926,7 @@ export function detectSchemaYmlPatterns(file: ChangedFile, rubric: Rubric): Find
         body:
           `The \`${r.test}\` data test was removed from column \`${r.column}\` on model \`${r.model}\`. ` +
           (isUniquenessSignal
-            ? `Removing a \`unique\` test on a mart-layer key is how silent duplicate rows ship — ` +
+            ? `Removing a \`unique\` test on a ${layerLabel} key is how silent duplicate rows ship — ` +
               `downstream joins fan out, aggregates double-count, and no test catches it. Restore ` +
               `the test, or explicitly document why the grain no longer needs to be unique on this column.`
             : notNullOnLikelyPK
diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts
index 0fb5b5eb9..7414211cc 100644
--- a/packages/opencode/src/altimate/review/orchestrate.ts
+++ b/packages/opencode/src/altimate/review/orchestrate.ts
@@ -1101,14 +1101,21 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
     isComplexOf: (f) => ctxByPath.get(f.path)?.complex ?? false,
   })
   const classifiedTier = tierResult.tier
-  // G2 — --force-tier overrides the classifier. Envelope records both the forced
-  // tier and the original classification so audits can see the bypass; the
-  // reasons list gets a leading "forced" marker so downstream doesn't confuse
-  // the forced tier for a natural one.
+  // G2 — --force-tier overrides the classifier. Envelope records both the
+  // forced tier and the original classification whenever the flag is passed
+  // (regardless of whether the forced value happens to match the classifier),
+  // so audits can see the bypass every time the flag was used. The reasons
+  // list gets a leading "forced" marker so downstream doesn't confuse the
+  // forced tier for a natural one. Codex-R18-review fix — the earlier
+  // `!== classifiedTier` gate silently hid the bypass when the forced tier
+  // matched the classifier's result.
   const tier = input.forceTier ?? classifiedTier
-  const tierForced = input.forceTier !== undefined && input.forceTier !== classifiedTier
+  const tierForced = input.forceTier !== undefined
   const tierReasons = tierForced
-    ? [`forced via --force-tier (classifier said ${classifiedTier})`, ...tierResult.reasons]
+    ? [
+        `forced via --force-tier=${input.forceTier} (classifier said ${classifiedTier})`,
+        ...tierResult.reasons,
+      ]
     : tierResult.reasons
 
   const lanes = new Set(input.config.reviewers.length ? input.config.reviewers : TIER_LANES[tier])

From b32065b238a28affae6c9645c1eb3eaab25678cb Mon Sep 17 00:00:00 2001
From: Haider <haider@altimate.ai>
Date: Tue, 21 Jul 2026 11:58:23 +0530
Subject: [PATCH 3/6] fix(review): address PR-1027 code-review feedback
 (blocker + majors + minors)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Independent multi-reviewer code review on the PR surfaced one blocker,
two majors, and three minors. This commit addresses each finding and
adds regression-guard tests.

## BLOCKER — column-aware detector regresses on real diffs + broke its
own test (finding #1)

The R18 `collectTestOccurrencesFromDiff` walker required both
`- name: model` AND `- name: column` inside the same diff hunk to
attribute a removed test. With git's default `-U3` context the model
header sits many lines above a column's `tests:` block and isn't in the
hunk — so the walker misclassified the first `- name:` it saw as a
model, never set `currentColumn`, and silently dropped the removal.

The existing unit test at test/altimate/review-dbt-patterns.test.ts:141
(`"-          - unique\n-          - not_null"`) went from pass to fail
under R18, so the branch was shipping a red test suite as well.

Rewrite `detectSchemaYmlPatterns` to prefer STRUCTURAL YAML diffing:

- Accept optional old/new file content via a `SchemaYmlDetectContent`
  parameter. When provided, parse both sides with the `yaml` package,
  walk `models[]`, `snapshots[]`, `sources[]`, `seeds[]`, extract every
  `(entity, column?, test)` tuple, diff old vs new sets, emit one
  finding per genuine removal.
- Supports both column-level and model-level tests (dbt allows
  `tests:` / `data_tests:` directly under the entity for
  surrogate-key `unique`, etc.) — closes MAJOR finding #2.
- Handles both `tests` and `data_tests` (dbt 1.8+ alias), both bare
  (`- unique`) and block-form (`- relationships: {...}`) tests, and
  quoted / commented YAML names — closes MINOR finding #6.
- Sources are qualified as `source.table.column` in the model field.

Fall back to the pre-R18 string-based line detection when no content is
provided (e.g. unit-test callers, offline CI diffs without a content
resolver). Fallback emits a suggestion / warning without column
attribution — cannot distinguish "moved to sibling column" from
"removed" with diff-only input, that limitation is inherent. The
existing test's expectation is preserved.

Wire the orchestrator to always supply old/new content when calling the
detector on modified schema.yml files, so the fallback only fires in
tests / diff-only CI paths, not in production reviews.

## MAJOR — auto-manifest projectRoot not threaded to compiled resolver
(finding #3)

`autoDiscoverManifest` returned `{ path, projectRoot }` and rebased
`manifestAbs` correctly, but `dbtProjectName(opts.cwd)` and
`makeCompiledResolver({ cwd: opts.cwd })` still used `opts.cwd`. When
the CLI was invoked from a subdirectory, auto-discovery would find the
manifest in an ancestor project but the compiled resolver looked for
`target/compiled/…` under the subdir and never found it — engine lanes
silently fell back to raw Jinja.

Thread a `dbtRoot` variable through the auto-discovery path: starts as
`opts.cwd`, updated to `discovered.projectRoot` when G3 fires. Use it
for both `dbtProjectName(dbtRoot)` and
`makeCompiledResolver({ cwd: dbtRoot })` so compiled SQL is found next
to the discovered manifest.

## MINORS

- **#4 docstring** — `autoDiscoverManifest`'s docstring cited defenses
  ("relative escapes", "under project's parent") that aren't in the
  code. Rewrote the doc to describe what actually happens:
  walk up for `dbt_project.yml`; return the adjacent
  `target/manifest.json` when present; never grab a `target/` from an
  unrelated tool that happens to sit above us on the filesystem.

- **#5a verdictHeadline undefined** — `env.tierClassified` is optional
  in the schema; an externally-built envelope with `tierForced: true`
  but no `tierClassified` would render `was undefined`. Added a
  `?? "unknown"` fallback in `format.ts`.

- **#5b unbounded tierReasons in summary** — when `--force-tier` is
  passed on a large PR, `tierReasons` prepends the forced marker AND
  spreads all `classifyPR().reasons` (one entry per file that forces
  the FULL tier). Rendered comment was bloating. Cap the rendered
  summary at the first 8 reasons with a "+N more in verdict envelope"
  overflow marker; the full list stays in the signed envelope for
  audit.

## Tests

Added 9 new regression tests:

- `dbt-patterns` — structural: sibling-column edge case (the exact
  case G6 claims to fix), model-level test removal with attribution
  metadata, block-form `- relationships:`, `data_tests` alias,
  source-column tests, added-file returns 0 removals, fallback
  diff-only path preserves the existing test's expectation.
- `verdict` — `--force-tier` envelope audit: `tierForced` and
  `tierClassified` are set whenever the flag is passed (including when
  forced tier matches classifier), and are covered by the HMAC
  signature (tampered envelope stripping the audit fields fails
  verifyEnvelope).

Test suite: 155/155 passing across the six review test files
(previously 46/47 — the one test the R18 branch broke is back green).
Typecheck clean.
---
 .../src/altimate/review/dbt-patterns.ts       | 383 ++++++++++++------
 .../opencode/src/altimate/review/format.ts    |  21 +-
 .../src/altimate/review/orchestrate.ts        |  13 +-
 packages/opencode/src/altimate/review/run.ts  |  31 +-
 .../test/altimate/review-dbt-patterns.test.ts | 187 +++++++++
 .../opencode/test/altimate/review.test.ts     |  53 +++
 6 files changed, 548 insertions(+), 140 deletions(-)

diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts
index 2e46d58a8..24ff82cc1 100644
--- a/packages/opencode/src/altimate/review/dbt-patterns.ts
+++ b/packages/opencode/src/altimate/review/dbt-patterns.ts
@@ -1,4 +1,5 @@
 import path from "node:path"
+import YAML from "yaml"
 import { type Finding, type Severity, type ReviewCategory, makeFinding } from "./finding"
 import { type ChangedFile, classifyDbtFile } from "./diff-filter"
 import { type Rubric, clampSeverity, exclusionReason } from "./rubric"
@@ -790,162 +791,294 @@ export function detectModelPatterns(file: ChangedFile, newSql: string | undefine
  * Detect removal of `unique` / `not_null` / `relationships` tests in a schema.yml
  * diff — the guardrail that catches fan-out/dupes is the thing being deleted.
  */
-/** Walk a unified diff and, for each removed test-line, reconstruct which
- *  `(model, column)` context it belonged to on the OLD side. Uses hunk headers
- *  to reset line counters and tracks the nearest `- name:` header preceding
- *  each line. Yields `{ model, column, test }` tuples for every removed
- *  `unique | not_null | relationships` — including bare `- <test>` lines and
- *  block-form `- <test>:` lines (relationships with args). Round-18 G6. */
-function collectTestOccurrencesFromDiff(diff: string | undefined, side: "-" | "+"): Set<string> {
-  // Returned as a Set of canonical `${model}\x00${column}\x00${test}` keys so
-  // callers can compare removed vs added sets without re-parsing.
+/**
+ * Structural extraction of `(model, column?, test)` tuples from a parsed
+ * schema.yml document. Handles:
+ *  - Column-level tests under `models[*].columns[*].tests` / `.data_tests`
+ *  - Model-level tests directly under `models[*].tests` / `.data_tests`
+ *    (surrogate-key `unique`, etc. — no column)
+ *  - Sources (`sources[*].tables[*].(columns[*].)tests`) since dbt supports
+ *    the same test grammar there
+ *  - Snapshots (`snapshots[*].(columns[*].)tests`)
+ *  - Both bare (`- unique`) and block-form (`- relationships: {...}`)
+ *  - Both `tests` and `data_tests` (dbt 1.8+ alias)
+ *
+ * Returns a Set of canonical `${modelOrSource}\x00${column}\x00${test}` keys
+ * (column is empty string for model-level tests). Yields nothing when the
+ * document isn't shaped like a schema.yml.
+ */
+function extractTestOccurrences(doc: unknown): Set<string> {
   const out = new Set<string>()
-  if (!diff) return out
-  let currentModel = ""
-  let currentColumn = ""
-  // Track indentation depth of the CURRENT model/column so we can pop when we
-  // dedent past them. A `- name: X` at indent I claims all lines at indent > I.
-  let modelIndent = -1
-  let columnIndent = -1
-  const modelRe = /^(\s*)-\s*name:\s*([A-Za-z0-9_.\-]+)\s*$/
-  const testRe = /^(\s*)-\s*(unique|not_null|relationships)\b(?::)?/i
-  for (const raw of diff.split("\n")) {
-    // Diff hunk header — reset context; don't try to interpret hunk offsets.
-    if (raw.startsWith("@@")) {
-      currentModel = ""
-      currentColumn = ""
-      modelIndent = -1
-      columnIndent = -1
-      continue
-    }
-    // We consider both context and same-side lines for STRUCTURE (they define
-    // the model/column context) but only same-side lines for TEST detection.
-    let payload: string
-    let onOurSide: boolean
-    if (raw.startsWith(side)) {
-      // remove +/- prefix
-      payload = raw.slice(1)
-      onOurSide = true
-    } else if (raw.startsWith(side === "-" ? "+" : "-") || raw.startsWith("+++") || raw.startsWith("---")) {
-      // The other side's insertion/deletion — don't read structure from it,
-      // don't produce test occurrences from it.
-      continue
-    } else {
-      // Context line (leading space or none).
-      payload = raw.startsWith(" ") ? raw.slice(1) : raw
-      onOurSide = false
-    }
-    // Pop column/model when we've dedented past them.
-    const leadingWs = payload.match(/^(\s*)/)![1].length
-    // Only pop for non-empty payload lines (blank lines carry no indent info).
-    if (payload.trim()) {
-      if (columnIndent >= 0 && leadingWs <= columnIndent) {
-        currentColumn = ""
-        columnIndent = -1
-      }
-      if (modelIndent >= 0 && leadingWs <= modelIndent) {
-        currentModel = ""
-        modelIndent = -1
+  if (!doc || typeof doc !== "object") return out
+  const d = doc as Record<string, unknown>
+
+  const emitTests = (entity: string, column: string, tests: unknown): void => {
+    if (!Array.isArray(tests)) return
+    for (const t of tests) {
+      const name =
+        typeof t === "string"
+          ? t
+          : t && typeof t === "object"
+            ? Object.keys(t as Record<string, unknown>)[0]
+            : undefined
+      if (!name) continue
+      const n = name.toLowerCase()
+      // Restrict to the guardrail tests the detector cares about; a bespoke
+      // custom test being removed is not a signal at this level.
+      if (n === "unique" || n === "not_null" || n === "relationships") {
+        out.add(`${entity}\x00${column}\x00${n}`)
       }
     }
-    // Detect `- name: X` — could be either a model header or a column header;
-    // disambiguate by whether we already have a model in scope.
-    const nameMatch = payload.match(modelRe)
-    if (nameMatch) {
-      const wsLen = nameMatch[1].length
-      const name = nameMatch[2]
-      // If we have no model, or this header is at the same/less indent than
-      // the current model header, treat as a new model.
-      if (currentModel === "" || wsLen <= modelIndent) {
-        currentModel = name
-        modelIndent = wsLen
-        currentColumn = ""
-        columnIndent = -1
-      } else {
-        // Deeper than the model header → column of the current model.
-        currentColumn = name
-        columnIndent = wsLen
+  }
+
+  const walkEntityWithColumns = (entityName: string | undefined, node: unknown): void => {
+    if (!entityName || !node || typeof node !== "object") return
+    const n = node as Record<string, unknown>
+    // Model/source/snapshot-level tests: `tests:` or `data_tests:` right under the entity.
+    emitTests(entityName, "", n.tests)
+    emitTests(entityName, "", n.data_tests)
+    if (Array.isArray(n.columns)) {
+      for (const col of n.columns) {
+        if (!col || typeof col !== "object") continue
+        const c = col as Record<string, unknown>
+        const cname = typeof c.name === "string" ? c.name : undefined
+        if (!cname) continue
+        emitTests(entityName, cname, c.tests)
+        emitTests(entityName, cname, c.data_tests)
       }
-      continue
     }
-    // Detect a test on OUR side and within a column context.
-    if (onOurSide && currentModel && currentColumn) {
-      const t = payload.match(testRe)
-      if (t) {
-        const test = t[2].toLowerCase()
-        out.add(`${currentModel}\x00${currentColumn}\x00${test}`)
+  }
+
+  // `models:` — array of `{ name, columns?, tests? }`
+  if (Array.isArray(d.models)) {
+    for (const m of d.models) {
+      if (!m || typeof m !== "object") continue
+      const mm = m as Record<string, unknown>
+      const mname = typeof mm.name === "string" ? mm.name : undefined
+      walkEntityWithColumns(mname, mm)
+    }
+  }
+  // `snapshots:` — same shape as models
+  if (Array.isArray(d.snapshots)) {
+    for (const s of d.snapshots) {
+      if (!s || typeof s !== "object") continue
+      const ss = s as Record<string, unknown>
+      const sname = typeof ss.name === "string" ? ss.name : undefined
+      walkEntityWithColumns(sname, ss)
+    }
+  }
+  // `sources:` — `[{ name, tables: [{ name, columns?, tests? }] }]`
+  if (Array.isArray(d.sources)) {
+    for (const src of d.sources) {
+      if (!src || typeof src !== "object") continue
+      const srcObj = src as Record<string, unknown>
+      const srcName = typeof srcObj.name === "string" ? srcObj.name : undefined
+      if (!Array.isArray(srcObj.tables)) continue
+      for (const t of srcObj.tables) {
+        if (!t || typeof t !== "object") continue
+        const tt = t as Record<string, unknown>
+        const tname = typeof tt.name === "string" ? tt.name : undefined
+        const qualified = srcName && tname ? `${srcName}.${tname}` : tname || undefined
+        walkEntityWithColumns(qualified, tt)
       }
     }
   }
+  // `seeds:` — leaf entity, same test grammar
+  if (Array.isArray(d.seeds)) {
+    for (const s of d.seeds) {
+      if (!s || typeof s !== "object") continue
+      const ss = s as Record<string, unknown>
+      const sname = typeof ss.name === "string" ? ss.name : undefined
+      walkEntityWithColumns(sname, ss)
+    }
+  }
   return out
 }
 
-export function detectSchemaYmlPatterns(file: ChangedFile, rubric: Rubric): Finding[] {
+/**
+ * Fallback removed-test detector for callers that supply only the diff
+ * (no old/new content). Kept intentionally conservative: matches removed
+ * lines that look like `- unique | not_null | relationships`, subtracts any
+ * added line with the same trimmed text. Cannot distinguish "moved to
+ * another column" from "removed from this column" — that requires content.
+ * The old-string dedup false-negative on sibling columns is inherent to
+ * diff-only input; when structural content is available we use the
+ * structural path (`extractTestOccurrences` diff of old vs new).
+ */
+function fallbackRemovedTestLines(diff: string | undefined): string[] {
+  if (!diff) return []
+  const added: string[] = []
+  const removed: string[] = []
+  for (const raw of diff.split("\n")) {
+    if (raw.startsWith("+") && !raw.startsWith("+++")) added.push(raw.slice(1))
+    else if (raw.startsWith("-") && !raw.startsWith("---")) removed.push(raw.slice(1))
+  }
+  const testLine = /^\s*-\s*(unique|not_null|relationships)\b/i
+  const removedTests = removed.filter((l) => testLine.test(l))
+  const addedTrim = new Set(added.map((l) => l.trim()))
+  return removedTests.filter((l) => !addedTrim.has(l.trim()))
+}
+
+export interface SchemaYmlDetectContent {
+  oldContent?: string
+  newContent?: string
+}
+
+/**
+ * Detect removed data tests in a schema.yml diff.
+ *
+ * PREFERRED path (production, via orchestrator): pass full old/new content in
+ * `opts` — the detector parses both YAML documents, diffs by
+ * `(entity, column, test)` tuples, and emits one finding per genuine removal.
+ * This captures model-level tests (no column), sibling-column edge cases
+ * (unique removed from column X while column Y still declares it), quoted /
+ * commented YAML names, and any layout / indent style the parser accepts.
+ *
+ * FALLBACK path (callers that only have the raw diff, e.g. unit tests or
+ * pre-collected CI diffs without a content resolver): use the string-based
+ * removed-line detection preserved from the pre-R18 detector. It cannot
+ * tell "removed" from "moved to another column" for the same test type on
+ * a sibling column — that limitation is inherent to diff-only input.
+ */
+export function detectSchemaYmlPatterns(
+  file: ChangedFile,
+  rubric: Rubric,
+  opts: SchemaYmlDetectContent = {},
+): Finding[] {
   const kind = classifyDbtFile(file.path)
   if (kind !== "schema_yml" || file.status === "deleted") return []
 
-  // COLUMN-AWARE test-removal detection (G6 fix, Round 18). The previous
-  // string-set dedup collapsed `- unique` on ANY column into the "still
-  // present" bucket, hiding a genuine removal from column X whenever a
-  // sibling column Y still had `- unique`. This walks the diff with model/
-  // column context so a removal is only cancelled by re-appearance on the
-  // SAME `(model, column, test)` tuple.
-  const removedByCtx = collectTestOccurrencesFromDiff(file.diff, "-")
-  const addedByCtx = collectTestOccurrencesFromDiff(file.diff, "+")
-  const genuinelyRemoved: Array<{ model: string; column: string; test: string }> = []
-  for (const key of removedByCtx) {
-    if (addedByCtx.has(key)) continue
-    const [model, column, test] = key.split("\x00")
-    genuinelyRemoved.push({ model, column, test })
+  let removals: Array<{ model: string; column: string; test: string }> = []
+  let usedStructural = false
+
+  // PREFERRED: structural YAML diff when we have both sides' content.
+  if (opts.newContent !== undefined) {
+    let oldDoc: unknown = undefined
+    let newDoc: unknown = undefined
+    try {
+      newDoc = YAML.parse(opts.newContent)
+    } catch {
+      newDoc = undefined
+    }
+    if (opts.oldContent !== undefined) {
+      try {
+        oldDoc = YAML.parse(opts.oldContent)
+      } catch {
+        oldDoc = undefined
+      }
+    }
+    // For an added file the "old" side is empty (nothing removed).
+    if (newDoc !== undefined && (opts.oldContent === undefined || oldDoc !== undefined)) {
+      const oldSet = extractTestOccurrences(oldDoc ?? {})
+      const newSet = extractTestOccurrences(newDoc)
+      for (const key of oldSet) {
+        if (newSet.has(key)) continue
+        const [model, column, test] = key.split("\x00")
+        removals.push({ model, column, test })
+      }
+      usedStructural = true
+    }
+  }
+
+  // FALLBACK: line-based detection for diff-only callers.
+  if (!usedStructural) {
+    const removedLines = fallbackRemovedTestLines(file.diff)
+    // Represent as unattributed tuples (model + column unknown) so the
+    // finding still surfaces with the correct severity but names the file
+    // rather than a specific column.
+    for (const l of removedLines) {
+      const m = /^\s*-\s*(unique|not_null|relationships)\b/i.exec(l)
+      if (!m) continue
+      removals.push({ model: "", column: "", test: m[1].toLowerCase() })
+    }
+    // Deduplicate identical (model, column, test) triples so the fallback
+    // path emits at most one finding per removed test-line pattern.
+    const seen = new Set<string>()
+    removals = removals.filter((r) => {
+      const k = `${r.model}\x00${r.column}\x00${r.test}`
+      if (seen.has(k)) return false
+      seen.add(k)
+      return true
+    })
   }
-  if (!genuinelyRemoved.length) return []
 
-  // Emit ONE finding per (column, test) removal for precision — customers
-  // can see exactly which guardrail was dropped, not just a total count.
+  if (!removals.length) return []
+
   const findings: Finding[] = []
   const seenModel = new Set<string>()
-  for (const r of genuinelyRemoved) {
+  const isMartLayer = /(^|\/)(marts?|reporting)\//.test(file.path)
+  const layerLabel = isMartLayer ? "mart-layer" : "declared"
+
+  for (const r of removals) {
     const isUniquenessSignal = r.test === "unique"
-    const notNullOnLikelyPK = r.test === "not_null" && /(_id$|^id$|_key$)/i.test(r.column)
+    const notNullOnLikelyPK =
+      r.test === "not_null" && !!r.column && /(_id$|^id$|_key$)/i.test(r.column)
     // Uniqueness OR not_null on an id/key column = warning (silent-dup / null-PK
     // risk); other not_null / relationships removals = suggestion.
     const sev: Severity = isUniquenessSignal || notNullOnLikelyPK ? "warning" : "suggestion"
-    // Detect layer from file path so the finding body doesn't misattribute
-    // (Codex-R18-review noted the previous copy said "mart-layer key" on
-    // every unique removal regardless of the schema.yml's actual layer).
-    const isMartLayer = /(^|\/)(marts?|reporting)\//.test(file.path)
-    const layerLabel = isMartLayer ? "mart-layer" : "declared"
-    const key = `${r.model}.${r.column}`
-    const isFirstForModel = !seenModel.has(r.model)
-    seenModel.add(r.model)
+
+    // Title / body vary based on whether structural attribution is available.
+    const attributed = !!r.model && !!r.column
+    const modelLevel = !!r.model && !r.column
+    const filename = file.path.split("/").pop()
+    const locKey = attributed ? `${r.model}.${r.column}` : modelLevel ? `${r.model} (model-level)` : "(unknown location)"
+    const title = attributed
+      ? `${filename}: ${r.test} test removed from ${locKey}`
+      : modelLevel
+        ? `${filename}: model-level ${r.test} test removed from \`${r.model}\``
+        : `${filename}: ${r.test} data test removed`
+
+    let bodyLead: string
+    if (attributed) {
+      bodyLead = `The \`${r.test}\` data test was removed from column \`${r.column}\` on model \`${r.model}\`. `
+    } else if (modelLevel) {
+      bodyLead = `A model-level \`${r.test}\` test was removed from \`${r.model}\`. `
+    } else {
+      bodyLead = `A \`${r.test}\` data test was removed from this schema file. `
+    }
+    let bodyRationale: string
+    if (isUniquenessSignal) {
+      bodyRationale =
+        `Removing a \`unique\` test on a ${layerLabel} key is how silent duplicate rows ship — ` +
+        `downstream joins fan out, aggregates double-count, and no test catches it. Restore ` +
+        `the test, or explicitly document why the grain no longer needs to be unique on this column.`
+    } else if (notNullOnLikelyPK) {
+      bodyRationale =
+        `Removing \`not_null\` on what looks like an identifier column (\`${r.column}\`) means nulls ` +
+        `will slip through into downstream joins and aggregates. Restore the test unless the column ` +
+        `is genuinely nullable now.`
+    } else {
+      bodyRationale =
+        `Removing an existing data test is a silent-regression risk. Confirm the test is genuinely ` +
+        `obsolete, not dropped to make CI green.`
+    }
+    const isFirstForModel = !!r.model && !seenModel.has(r.model)
+    if (r.model) seenModel.add(r.model)
+    const bodyTail =
+      isFirstForModel && removals.length > 1
+        ? `\n\n_This PR removes ${removals.length} data tests in total on model(s) ` +
+          `${[...new Set(removals.map((x) => x.model).filter(Boolean))].map((m) => `\`${m}\``).join(", ")}._`
+        : ""
+
     findings.push(
       makeFinding({
         severity: clampSeverity("test_coverage", sev, "high"),
         category: "test_coverage",
-        title: `${file.path.split("/").pop()}: ${r.test} test removed from ${key}`,
-        body:
-          `The \`${r.test}\` data test was removed from column \`${r.column}\` on model \`${r.model}\`. ` +
-          (isUniquenessSignal
-            ? `Removing a \`unique\` test on a ${layerLabel} key is how silent duplicate rows ship — ` +
-              `downstream joins fan out, aggregates double-count, and no test catches it. Restore ` +
-              `the test, or explicitly document why the grain no longer needs to be unique on this column.`
-            : notNullOnLikelyPK
-            ? `Removing \`not_null\` on what looks like an identifier column (\`${r.column}\`) means nulls ` +
-              `will slip through into downstream joins and aggregates. Restore the test unless the column ` +
-              `is genuinely nullable now.`
-            : `Removing an existing data test is a silent-regression risk. Confirm the test is genuinely ` +
-              `obsolete, not dropped to make CI green.`) +
-          (isFirstForModel && genuinelyRemoved.length > 1
-            ? `\n\n_This PR removes ${genuinelyRemoved.length} data tests in total on model(s) ` +
-              `${[...new Set(genuinelyRemoved.map((x) => x.model))].map((m) => `\`${m}\``).join(", ")}._`
-            : ""),
+        title,
+        body: bodyLead + bodyRationale + bodyTail,
         file: file.path,
         confidence: "high",
         evidence: {
           tool: "dbt-patterns",
-          result: { rule: "removed_tests", model: r.model, column: r.column, test: r.test },
+          result: {
+            rule: "removed_tests",
+            model: r.model || undefined,
+            column: r.column || undefined,
+            test: r.test,
+            attribution: attributed ? "column" : modelLevel ? "model-level" : "diff-only",
+          },
         },
-        ruleKey: `test_coverage:removed-tests:${r.model}.${r.column}.${r.test}`,
+        ruleKey: `test_coverage:removed-tests:${r.model || "?"}.${r.column || "?"}.${r.test}`,
       }),
     )
   }
diff --git a/packages/opencode/src/altimate/review/format.ts b/packages/opencode/src/altimate/review/format.ts
index f4b788cd2..a096249a4 100644
--- a/packages/opencode/src/altimate/review/format.ts
+++ b/packages/opencode/src/altimate/review/format.ts
@@ -28,7 +28,11 @@ export function verdictHeadline(env: VerdictEnvelope): string {
     [critical && `${critical} critical`, warning && `${warning} warning`, suggestion && `${suggestion} suggestion`]
       .filter(Boolean)
       .join(", ") || "no findings"
-  const tierLabel = env.tierForced ? `${env.tier} tier — forced (was ${env.tierClassified})` : `${env.tier} tier`
+  // tierClassified is optional in the schema — guard against externally-built
+  // envelopes that mark tierForced without threading the original classification.
+  const tierLabel = env.tierForced
+    ? `${env.tier} tier — forced (was ${env.tierClassified ?? "unknown"})`
+    : `${env.tier} tier`
   return `${VERDICT_LABEL[env.verdict]} — ${counts} (${tierLabel})`
 }
 
@@ -47,11 +51,18 @@ export function renderSummary(env: VerdictEnvelope): string {
   // G1 (Round 18) — surface classifier reasons when --explain-tier populated
   // them, so a customer can see why the review ran at this tier. Also surfaces
   // when --force-tier bypassed the classifier (tierReasons is auto-populated).
+  // Truncate the rendered summary on very large diffs: classifyPR appends one
+  // reason per file forcing FULL tier (e.g. every touched schema.yml or every
+  // PII column), which bloats the comment on wide PRs. The full list stays in
+  // the signed envelope's tierReasons[]; the summary shows the first 8.
   if (env.tierReasons && env.tierReasons.length) {
-    lines.push(
-      `> 🧭 **Tier: ${env.tier}** — ${env.tierReasons.map((r) => `\`${r}\``).join(", ")}`,
-      "",
-    )
+    const RENDER_CAP = 8
+    const shown = env.tierReasons.slice(0, RENDER_CAP).map((r) => `\`${r}\``).join(", ")
+    const overflow =
+      env.tierReasons.length > RENDER_CAP
+        ? ` (+${env.tierReasons.length - RENDER_CAP} more in verdict envelope)`
+        : ""
+    lines.push(`> 🧭 **Tier: ${env.tier}** — ${shown}${overflow}`, "")
   }
 
   if (!env.findings.length) {
diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts
index 7414211cc..4215e54ec 100644
--- a/packages/opencode/src/altimate/review/orchestrate.ts
+++ b/packages/opencode/src/altimate/review/orchestrate.ts
@@ -1217,8 +1217,19 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
 
   // schema.yml-level detectors (test removal) — run on changed YAML files
   // regardless of tier, since deleting a guardrail test is always worth flagging.
+  // Pass old + new content when available so the detector diffs structural YAML
+  // (per-(entity, column, test) tuples) instead of walking the unified diff
+  // (which loses context when the model header is outside the -U3 window and
+  // silently drops sibling-column edge cases). Falls back to diff-only line
+  // detection when a content resolver isn't wired (e.g. unit-test callers).
   for (const file of reviewable) {
-    if (file.kind === "schema_yml") all.push(detectSchemaYmlPatterns(file, input.rubric))
+    if (file.kind !== "schema_yml") continue
+    const oldRef = file.oldPath ?? file.path
+    const [oldContent, newContent] = await Promise.all([
+      file.status === "modified" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
+      file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
+    ])
+    all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent }))
   }
 
   // Architectural dedup: the regex `dbt-patterns`/`rule-catalog` layer is a
diff --git a/packages/opencode/src/altimate/review/run.ts b/packages/opencode/src/altimate/review/run.ts
index 5d3c7f50a..3536804b3 100644
--- a/packages/opencode/src/altimate/review/run.ts
+++ b/packages/opencode/src/altimate/review/run.ts
@@ -79,15 +79,18 @@ async function detectDialect(manifestAbs: string): Promise<string | undefined> {
 }
 
 /**
- * G3 (Round 18) — walk upward from `cwd` looking for a dbt project, then check
- * for the compiled `target/manifest.json` next to it. Emits an absolute path
- * only when the manifest exists AND the file is under the dbt project's parent
- * (defensive against relative escapes and stale manifests from unrelated
- * projects on the same filesystem). Returns undefined when nothing matches.
+ * G3 (Round 18) — walk upward from `cwd` looking for the nearest
+ * `dbt_project.yml`; if found, return the adjacent `target/manifest.json`
+ * (when it exists). Anchored on `dbt_project.yml` so we never pick up a
+ * `target/` from an unrelated tool (e.g. an Airflow project) that happens
+ * to live above us on the filesystem.
  *
  * We DO NOT re-run `dbt compile` here — that could touch a real warehouse or
- * consume credentials. The customer is still responsible for a compiled
- * artifact; we just find it when they compiled but didn't pass `--manifest`.
+ * consume credentials. The customer is still responsible for compiling; we
+ * just find the existing artifact when they didn't pass `--manifest`.
+ *
+ * Returns undefined when no dbt project is found upward, or when the found
+ * project has no compiled manifest.
  */
 async function autoDiscoverManifest(cwd: string): Promise<{ path: string; projectRoot: string } | undefined> {
   let dir = path.resolve(cwd)
@@ -167,6 +170,12 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
   let manifestAbs = path.isAbsolute(config.manifestPath)
     ? config.manifestPath
     : path.join(opts.cwd, config.manifestPath)
+  // The "effective dbt project root" for downstream lookups (dbtProjectName,
+  // compiled-SQL resolver). Starts as opts.cwd — G3 auto-discovery updates it
+  // when it finds a dbt_project.yml in an ancestor directory, so the compiled
+  // resolver and project-name lookups target the discovered project rather
+  // than the CLI's working directory (which may be a subdir).
+  let dbtRoot = opts.cwd
   // G3 (Round 18) — auto-discovery when the caller didn't pass `--manifest` and
   // the config-relative resolve above doesn't point at an existing file. Walk
   // upward for the dbt project root and use its `target/manifest.json`. Explicit
@@ -183,6 +192,7 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
       const discovered = await autoDiscoverManifest(opts.cwd)
       if (discovered) {
         manifestAbs = discovered.path
+        dbtRoot = discovered.projectRoot
         process.stderr.write(
           `ℹ️  auto-discovered dbt manifest at ${discovered.path} ` +
             `(dbt project root: ${discovered.projectRoot}). Pass --manifest to override.\n`,
@@ -212,8 +222,11 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
 
   // 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).
-  const projectName = await dbtProjectName(opts.cwd)
-  const getCompiled = opts.getContent ? undefined : makeCompiledResolver({ cwd: opts.cwd, projectName })
+  // Use `dbtRoot` (the discovered project root when G3 auto-discovery fired,
+  // else opts.cwd) so a subdir invocation still finds `target/compiled/…`
+  // next to the discovered manifest instead of falling back to raw Jinja.
+  const projectName = await dbtProjectName(dbtRoot)
+  const getCompiled = opts.getContent ? undefined : makeCompiledResolver({ cwd: dbtRoot, projectName })
 
   return runReview({
     changedFiles,
diff --git a/packages/opencode/test/altimate/review-dbt-patterns.test.ts b/packages/opencode/test/altimate/review-dbt-patterns.test.ts
index 9fbbae2b9..db347e6b2 100644
--- a/packages/opencode/test/altimate/review-dbt-patterns.test.ts
+++ b/packages/opencode/test/altimate/review-dbt-patterns.test.ts
@@ -158,6 +158,193 @@ describe("dbt-patterns detectors", () => {
     expect(f.length).toBe(0)
   })
 
+  // ------------------------------------------------------------------------
+  // Post-R18 review follow-ups: structural YAML path + sibling-column edge case
+  // + model-level tests + fallback shape. The rewritten detector prefers full
+  // old/new file content; these tests validate both paths.
+  // ------------------------------------------------------------------------
+
+  test("schema.yml (structural): unique removed from one column while sibling keeps it → 1 finding on the affected column", () => {
+    const oldContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: [unique, not_null]
+      - name: email
+        tests: [not_null]
+  - name: orders
+    columns:
+      - name: order_id
+        tests: [unique, not_null]
+`
+    const newContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: []
+      - name: email
+        tests: [not_null]
+  - name: orders
+    columns:
+      - name: order_id
+        tests: [unique, not_null]
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/marts/_models.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    // 2 removals on customers.customer_id (unique + not_null), 0 on orders/email
+    expect(f.length).toBe(2)
+    expect(has(f, "test_coverage", "warning")).toBe(true)
+    // At least one finding names customers.customer_id specifically
+    expect(f.some((x) => (x.title || "").includes("customers.customer_id"))).toBe(true)
+    // No finding attributed to orders.order_id (sibling still has `unique`)
+    expect(f.some((x) => (x.title || "").includes("orders.order_id"))).toBe(false)
+  })
+
+  test("schema.yml (structural): model-level unique test removed → 1 finding with model-level attribution", () => {
+    const oldContent = `version: 2
+models:
+  - name: customers
+    tests:
+      - unique
+    columns:
+      - name: customer_id
+`
+    const newContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/marts/_models.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    expect(f.length).toBe(1)
+    expect(f[0].category).toBe("test_coverage")
+    expect(f[0].severity).toBe("warning")
+    expect((f[0].title || "").includes("model-level")).toBe(true)
+    // Attribution flag in evidence lets downstream see it wasn't column-scoped
+    const evResult = (f[0].evidence?.result || {}) as Record<string, unknown>
+    expect(evResult.attribution).toBe("model-level")
+  })
+
+  test("schema.yml (structural): block-form relationships removal is detected", () => {
+    const oldContent = `version: 2
+models:
+  - name: orders
+    columns:
+      - name: customer_id
+        tests:
+          - relationships:
+              to: ref('customers')
+              field: customer_id
+`
+    const newContent = `version: 2
+models:
+  - name: orders
+    columns:
+      - name: customer_id
+        tests: []
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/schema.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    expect(f.length).toBe(1)
+    expect((f[0].title || "").includes("relationships")).toBe(true)
+  })
+
+  test("schema.yml (structural): data_tests (dbt 1.8+ alias) is recognized", () => {
+    const oldContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        data_tests: [unique, not_null]
+`
+    const newContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        data_tests: []
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/schema.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    expect(f.length).toBe(2)
+  })
+
+  test("schema.yml (structural): source column test removal is detected", () => {
+    const oldContent = `version: 2
+sources:
+  - name: raw
+    tables:
+      - name: users
+        columns:
+          - name: id
+            tests: [unique, not_null]
+`
+    const newContent = `version: 2
+sources:
+  - name: raw
+    tables:
+      - name: users
+        columns:
+          - name: id
+            tests: []
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/sources.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    expect(f.length).toBe(2)
+    // Source table gets qualified as `source.table` in the model field
+    expect(f.some((x) => (x.title || "").includes("raw.users.id"))).toBe(true)
+  })
+
+  test("schema.yml (structural): added file (no oldContent) surfaces no removal findings", () => {
+    const newContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: [unique, not_null]
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/schema.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent: undefined, newContent },
+    )
+    expect(f.length).toBe(0)
+  })
+
+  test("schema.yml (fallback): diff-only path still surfaces removal (existing test's shape)", () => {
+    // Exercises the fallback line-based detection when the orchestrator can't
+    // supply file content (e.g. offline CI diffs).
+    const f = detectSchemaYmlPatterns(
+      {
+        path: "models/marts/_models.yml",
+        status: "modified",
+        diff: "-          - unique\n-          - not_null",
+      },
+      DEFAULT_RUBRIC,
+    )
+    // At least one test_coverage finding surfaces so a customer isn't blind to
+    // removed guardrails just because the content resolver wasn't wired.
+    expect(has(f, "test_coverage")).toBe(true)
+  })
+
   test("benign additive column produces NO dbt-pattern finding (precision)", () => {
     const sql = `select id, upper(status) as status_upper from {{ ref('x') }}`
     const f = detectModelPatterns(
diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts
index 0e94f0ff4..99de1966d 100644
--- a/packages/opencode/test/altimate/review.test.ts
+++ b/packages/opencode/test/altimate/review.test.ts
@@ -265,6 +265,59 @@ describe("verdict", () => {
     expect(verifyEnvelope(tampered, "test-key")).toBe(false)
   })
 
+  test("--force-tier audit fields are set whenever the flag is passed (even when forced == classified)", () => {
+    // Simulates orchestrate.ts's audit-envelope construction for the two
+    // --force-tier cases. Ensures we never silently bypass the audit trail
+    // just because the forced value happens to match the classifier's tier.
+    // Regression guard for the post-R18 review finding.
+    const withForcedDifferent = buildEnvelope({
+      findings: [],
+      tier: "full",
+      mode: "comment",
+      tierForced: true,
+      tierClassified: "trivial",
+      tierReasons: ["forced via --force-tier=full (classifier said trivial)"],
+    })
+    expect(withForcedDifferent.tierForced).toBe(true)
+    expect(withForcedDifferent.tierClassified).toBe("trivial")
+
+    const withForcedSame = buildEnvelope({
+      findings: [],
+      tier: "full",
+      mode: "comment",
+      tierForced: true,
+      tierClassified: "full",
+      tierReasons: ["forced via --force-tier=full (classifier said full)"],
+    })
+    expect(withForcedSame.tierForced).toBe(true)
+    expect(withForcedSame.tierClassified).toBe("full")
+
+    const notForced = buildEnvelope({ findings: [], tier: "lite", mode: "comment" })
+    expect(notForced.tierForced).toBeUndefined()
+    expect(notForced.tierClassified).toBeUndefined()
+  })
+
+  test("tierForced + tierClassified are covered by the signature (tampering detected)", () => {
+    // Confirms the audit fields live inside the signed canonical body — a
+    // tampered envelope claiming a natural tier when a bypass ran can't slip
+    // past verifyEnvelope.
+    const signed = signEnvelope(
+      buildEnvelope({
+        findings: [],
+        tier: "full",
+        mode: "comment",
+        tierForced: true,
+        tierClassified: "trivial",
+        tierReasons: ["forced via --force-tier=full (classifier said trivial)"],
+        generatedAt: "2026-05-29T00:00:00Z",
+      }),
+      "k",
+    )
+    expect(verifyEnvelope(signed, "k")).toBe(true)
+    const strippedForce = { ...signed, tierForced: undefined, tierClassified: undefined, tierReasons: undefined }
+    expect(verifyEnvelope(strippedForce, "k")).toBe(false)
+  })
+
   test("tampering a NESTED finding field is detected (signature covers findings)", () => {
     const f = makeFinding({
       severity: "warning",

From 6729f5374645a055abe5078c4e4fc544f54e983f Mon Sep 17 00:00:00 2001
From: Haider <haider@altimate.ai>
Date: Tue, 21 Jul 2026 15:28:25 +0530
Subject: [PATCH 4/6] fix(review): address PR-1027 bot-review round-2 (dedupe
 collapse, rename bypass, snapshot yml, +5 more)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

After the earlier fixes on `b32065b23`, three independent reviewer
bots (coderabbitai, cubic-dev-ai, dev-punia-altimate) surfaced 8 more
issues on the schema.yml test-removal path — 2 HIGH, 1 MAJOR, 4
MEDIUM, 1 LOW — plus an independent local review round after that
caught an additional blocker downstream of the fixes. This commit
addresses each.

## BLOCKER — fallback distinct-removal fix defeated by global dedupe

The R18-follow-up fix removed a local dedup in the fallback path so
each removed test-line produced its own detector-level finding. But
`runReview` runs a global `dedupe(merged)` step that fingerprints
findings by (category, file, model, column, ruleKey) via
`finding.ts:107`. For fallback findings sharing `file`, empty
`model`, empty `column`, and a ruleKey varying only by `test`, two
`unique` removals in the same file still collapsed to one downstream.
The detector-level test at `review-dbt-patterns.test.ts:397` didn't
catch it because it asserts against the pre-dedupe output.

Fix: give fallback findings a per-diff occurrence-index discriminator
appended to the ruleKey (`.#0`, `.#1`, …) so distinct removals get
distinct fingerprints. Structural (attributed) findings unchanged —
`(model.column.test)` is already unique per removal.

Added an integration-shape regression test at
`review-dbt-patterns.test.ts:432` that pipes detector output through
`dedupe(f)` and asserts 4 distinct ids survive for 4 distinct
removals. Also a same-shape guard for the structural path.

## HIGH — renamed schema.yml bypasses detector

The orchestrator gated `oldContent` fetch on `file.status === "modified"`.
A schema.yml being renamed (e.g. moved to a new subdir) that also
dropped a `unique`/`not_null`/`relationships` guardrail silently
skipped the detector because `oldContent` came back undefined and
the structural path treated the file as newly-added.

Fix: gate on `file.status !== "added"` so renamed files fetch the
old side; separately, changed the detector so an undefined
`oldContent` on a modified/renamed file falls through to the
diff-based fallback (does not silently treat as added-file).

Added test at `review-dbt-patterns.test.ts:332` covering the rename
shape.

## MAJOR — structural path treats `oldContent=undefined` as added file

`canUseStructural = newContent !== undefined && (isAddedFile ||
oldContent !== undefined)`. When the content resolver returns
undefined on a modified/renamed file for a transient reason (git
failure, ref not readable), we now leave `usedStructural = false`
and let the fallback surface the raw diff removals — instead of
producing an empty structural-diff and silently dropping every
removal in the diff.

## MEDIUM — snapshot YAML property files never reached the detector

`classifyDbtFile` matched everything under `snapshots/` as
`snapshot` regardless of extension, so `snapshots/*.yml` never
reached the `schema_yml` gate in the orchestrator. The new
`snapshots:` branch added to the structural walker was dead code
for real snapshot property files.

Fix: split the classification by extension. `snapshots/*.sql`
remains `snapshot` (unchanged tier-forcing / catalog rules).
`snapshots/*.yml` classifies as `schema_yml` (routes to the
test-removal detector). Added `classifyDbtFile` assertions for
both extensions and an end-to-end structural-detector test on a
snapshot yml.

## MEDIUM — redundant per-model summary line

`isFirstForModel` fired once per distinct model, so a diff touching
two models produced two copies of the same aggregate summary
("This PR removes N tests total on model(s) A, B") appended to
separate findings.

Fix: replaced with a single file-scoped `summaryEmitted` boolean.
The aggregate summary is appended to the first finding for the
file, once, and the distinct-models list is computed from the
whole removals array. Test at
`review-dbt-patterns.test.ts:454` asserts exactly one finding
carries the aggregate line when two models have removals.

## MEDIUM — `warnIfStale` fired on unrelated file changes

`warnIfStale` compared mtimes for every changed path. Any
`README.md` / `package.json` / `.github/*` change post-manifest
triggered a false-positive stale warning.

Fix: introduced `isManifestAffecting()` — admits only
`.sql|.py|.yml|.yaml|.csv|.md` under
`models|seeds|snapshots|macros|tests|analyses/`, plus root
`dbt_project.yml|packages.yml|profiles.yml|dependencies.yml`.
Explicitly admits `.md` under `models/` because dbt docs blocks
live there. Exported for tests; added a table-driven test file
`review-run-stale.test.ts` covering both admitted and rejected
cases (README.md, models/foo.sql, docs/foo.md, models/foo.md,
seeds/lookup.csv, snapshots/*.sql, snapshots/*.yml, dbt_project.yml,
.github/workflows/, target/…).

## LOW — YAML parse errors silently ignored

Both `YAML.parse` failures (new content, old content) now log via
`Log.create({ service: "review", tag: "detectSchemaYmlPatterns" })`
with the file path and error message, then fall through to the
diff-based fallback. Not fail-open.

## PERF — sequential schema.yml `getContent` calls

The orchestrator's schema.yml loop was serial `await` per file;
schema-heavy PRs paid one round-trip per file. Refactored to
`Promise.all` across the file list with per-file `Promise.all`
for old/new. Ordering preserved by `Promise.all` contract.

## Test status

188 pass / 0 fail across the 7 review test files. Typecheck clean.
End-to-end sanity on the js2 test-removal scenario: CLI still
surfaces both `unique` and `not_null` warnings, verdict
`COMMENT/trivial/2`, envelope signature verifies.
---
 .../src/altimate/review/dbt-patterns.ts       | 114 +++++++---
 .../src/altimate/review/diff-filter.ts        |   6 +-
 .../src/altimate/review/orchestrate.ts        |  29 ++-
 packages/opencode/src/altimate/review/run.ts  |  18 +-
 .../test/altimate/review-dbt-patterns.test.ts | 208 +++++++++++++++++-
 .../test/altimate/review-run-stale.test.ts    |  51 +++++
 .../opencode/test/altimate/review.test.ts     |   4 +
 7 files changed, 387 insertions(+), 43 deletions(-)
 create mode 100644 packages/opencode/test/altimate/review-run-stale.test.ts

diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts
index 24ff82cc1..14fa6c9b7 100644
--- a/packages/opencode/src/altimate/review/dbt-patterns.ts
+++ b/packages/opencode/src/altimate/review/dbt-patterns.ts
@@ -1,5 +1,6 @@
 import path from "node:path"
 import YAML from "yaml"
+import { Log } from "@/altimate/util/log"
 import { type Finding, type Severity, type ReviewCategory, makeFinding } from "./finding"
 import { type ChangedFile, classifyDbtFile } from "./diff-filter"
 import { type Rubric, clampSeverity, exclusionReason } from "./rubric"
@@ -948,27 +949,57 @@ export function detectSchemaYmlPatterns(
   const kind = classifyDbtFile(file.path)
   if (kind !== "schema_yml" || file.status === "deleted") return []
 
-  let removals: Array<{ model: string; column: string; test: string }> = []
+  const removals: Array<{ model: string; column: string; test: string }> = []
   let usedStructural = false
 
   // PREFERRED: structural YAML diff when we have both sides' content.
-  if (opts.newContent !== undefined) {
+  //
+  // "Added" file case (no `oldContent` supplied AND status marks the file as
+  // newly-added upstream) — the old side is empty, so there's nothing to
+  // remove; using the structural path with an empty old set is correct.
+  //
+  // "Modified but resolver returned undefined" case (`oldContent === undefined`
+  // for a file whose status is `modified` or `renamed`) — the resolver couldn't
+  // read the old side. Do NOT treat this as an added file (would silently drop
+  // real removals). Fall through to the diff-only fallback below.
+  const isAddedFile = file.status === "added"
+  const canUseStructural =
+    opts.newContent !== undefined &&
+    // For a real modification/rename we require both sides; without oldContent
+    // fall back to diff parsing so removals in the diff still surface.
+    (isAddedFile || opts.oldContent !== undefined)
+
+  if (canUseStructural) {
     let oldDoc: unknown = undefined
     let newDoc: unknown = undefined
     try {
-      newDoc = YAML.parse(opts.newContent)
-    } catch {
+      newDoc = YAML.parse(opts.newContent as string)
+    } catch (err) {
+      // Debug telemetry: YAML parsers can trip on Jinja injected into a
+      // schema.yml or on non-ASCII quirks; log so failures are diagnosable
+      // rather than silently demoting to the fallback path.
+      Log.create({ service: "review", tag: "detectSchemaYmlPatterns" }).warn(
+        `YAML.parse failed on new content of ${file.path}; falling back to diff-only detection`,
+        err instanceof Error ? { error: err.message } : undefined,
+      )
       newDoc = undefined
     }
     if (opts.oldContent !== undefined) {
       try {
         oldDoc = YAML.parse(opts.oldContent)
-      } catch {
+      } catch (err) {
+        Log.create({ service: "review", tag: "detectSchemaYmlPatterns" }).warn(
+          `YAML.parse failed on old content of ${file.path}; falling back to diff-only detection`,
+          err instanceof Error ? { error: err.message } : undefined,
+        )
         oldDoc = undefined
       }
     }
-    // For an added file the "old" side is empty (nothing removed).
-    if (newDoc !== undefined && (opts.oldContent === undefined || oldDoc !== undefined)) {
+    // Only commit to the structural path when we have a parseable new side
+    // AND (we're on an added file OR the old side also parsed). This preserves
+    // "unparseable old YAML" → fallback, not "structural with empty old = every
+    // removed test looks like a genuine removal".
+    if (newDoc !== undefined && (isAddedFile || oldDoc !== undefined)) {
       const oldSet = extractTestOccurrences(oldDoc ?? {})
       const newSet = extractTestOccurrences(newDoc)
       for (const key of oldSet) {
@@ -980,34 +1011,44 @@ export function detectSchemaYmlPatterns(
     }
   }
 
-  // FALLBACK: line-based detection for diff-only callers.
+  // FALLBACK: line-based detection for diff-only callers (unit tests, offline
+  // CI diffs) OR when the structural path couldn't parse both sides.
+  //
+  // No local deduplication: each entry in `removedLines` is already one match
+  // per raw removed line, and model/column are always empty on this path — a
+  // (model, column, test) dedup key would collapse to just `test` and silently
+  // merge distinct removals of the same test type on different columns.
+  //
+  // Downstream, `runReview` runs a global `dedupe` step that fingerprints
+  // findings by (category, file, model, column, ruleKey). Two fallback
+  // findings that share `file`, empty `model`, empty `column`, and a
+  // test-name-only `ruleKey` would still collapse there. We tag each fallback
+  // finding with a stable occurrence-index discriminator (`#0`, `#1`, …) so
+  // distinct removals in the same diff survive the global dedupe. The index
+  // is per-diff, not per-file-history — sufficient for one review pass and
+  // stable across dry-repeats of the same diff.
+  const fallbackDiscriminators: string[] = []
   if (!usedStructural) {
     const removedLines = fallbackRemovedTestLines(file.diff)
-    // Represent as unattributed tuples (model + column unknown) so the
-    // finding still surfaces with the correct severity but names the file
-    // rather than a specific column.
+    let idx = 0
     for (const l of removedLines) {
       const m = /^\s*-\s*(unique|not_null|relationships)\b/i.exec(l)
       if (!m) continue
       removals.push({ model: "", column: "", test: m[1].toLowerCase() })
+      fallbackDiscriminators.push(`#${idx++}`)
     }
-    // Deduplicate identical (model, column, test) triples so the fallback
-    // path emits at most one finding per removed test-line pattern.
-    const seen = new Set<string>()
-    removals = removals.filter((r) => {
-      const k = `${r.model}\x00${r.column}\x00${r.test}`
-      if (seen.has(k)) return false
-      seen.add(k)
-      return true
-    })
   }
 
   if (!removals.length) return []
 
   const findings: Finding[] = []
-  const seenModel = new Set<string>()
   const isMartLayer = /(^|\/)(marts?|reporting)\//.test(file.path)
   const layerLabel = isMartLayer ? "mart-layer" : "declared"
+  // Emit the "N removals across models A/B/C" summary line ONCE per file
+  // (on the first finding), not once per distinct model — otherwise a diff
+  // that removes tests from three models produces three copies of the same
+  // global summary. A single boolean guard is enough.
+  let summaryEmitted = false
 
   for (const r of removals) {
     const isUniquenessSignal = r.test === "unique"
@@ -1052,14 +1093,27 @@ export function detectSchemaYmlPatterns(
         `Removing an existing data test is a silent-regression risk. Confirm the test is genuinely ` +
         `obsolete, not dropped to make CI green.`
     }
-    const isFirstForModel = !!r.model && !seenModel.has(r.model)
-    if (r.model) seenModel.add(r.model)
-    const bodyTail =
-      isFirstForModel && removals.length > 1
-        ? `\n\n_This PR removes ${removals.length} data tests in total on model(s) ` +
-          `${[...new Set(removals.map((x) => x.model).filter(Boolean))].map((m) => `\`${m}\``).join(", ")}._`
-        : ""
-
+    // Emit the aggregate summary once per file, on the first finding, and
+    // only when there are multiple removals to summarise. Uses the file-scoped
+    // `summaryEmitted` flag rather than a per-model guard so a diff touching
+    // three models emits ONE summary, not three.
+    const shouldEmitSummary = !summaryEmitted && removals.length > 1
+    if (shouldEmitSummary) summaryEmitted = true
+    const distinctModels = [...new Set(removals.map((x) => x.model).filter(Boolean))]
+    const modelClause = distinctModels.length
+      ? ` on model(s) ${distinctModels.map((m) => `\`${m}\``).join(", ")}`
+      : ""
+    const bodyTail = shouldEmitSummary
+      ? `\n\n_This PR removes ${removals.length} data tests in total${modelClause}._`
+      : ""
+
+    // ruleKey feeds the finding fingerprint (finding.ts:107). For attributed
+    // (structural) findings, `(model.column.test)` is unique per removal in
+    // the file. For unattributed fallback findings, `(?.?.test)` collapses
+    // distinct removals of the same test type — we append the fallback
+    // discriminator so each removed line becomes a distinct finding
+    // downstream of the global dedupe.
+    const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators.shift() ?? "#?"}`
     findings.push(
       makeFinding({
         severity: clampSeverity("test_coverage", sev, "high"),
@@ -1078,7 +1132,7 @@ export function detectSchemaYmlPatterns(
             attribution: attributed ? "column" : modelLevel ? "model-level" : "diff-only",
           },
         },
-        ruleKey: `test_coverage:removed-tests:${r.model || "?"}.${r.column || "?"}.${r.test}`,
+        ruleKey: `test_coverage:removed-tests:${r.model || "?"}.${r.column || "?"}.${r.test}${discriminator}`,
       }),
     )
   }
diff --git a/packages/opencode/src/altimate/review/diff-filter.ts b/packages/opencode/src/altimate/review/diff-filter.ts
index 234c04c29..4079fb640 100644
--- a/packages/opencode/src/altimate/review/diff-filter.ts
+++ b/packages/opencode/src/altimate/review/diff-filter.ts
@@ -61,7 +61,11 @@ export function classifyDbtFile(path: string): DbtFileKind {
   const isYaml = p.endsWith(".yml") || p.endsWith(".yaml")
   if (/(^|\/)(dbt_project|profiles|packages|dependencies)\.ya?ml$/.test(p)) return "project_config"
   if (/(^|\/)macros\//.test(p)) return "macro"
-  if (/(^|\/)snapshots\//.test(p)) return "snapshot"
+  // Snapshots split by extension: `.sql` files are snapshot definitions
+  // (tier-forcing, catalog-scoped), YAML property files under `snapshots/`
+  // are schema files (declared tests + column metadata) and should route to
+  // `schema_yml` so the test-removal detector runs on them.
+  if (/(^|\/)snapshots\//.test(p) && p.endsWith(".sql")) return "snapshot"
   if (/(^|\/)seeds\//.test(p) && p.endsWith(".csv")) return "seed"
   if (/(^|\/)tests\//.test(p) && p.endsWith(".sql")) return "test"
   if (/(^|\/)analyses\//.test(p)) return "analysis"
diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts
index 4215e54ec..11e8114c9 100644
--- a/packages/opencode/src/altimate/review/orchestrate.ts
+++ b/packages/opencode/src/altimate/review/orchestrate.ts
@@ -1222,14 +1222,27 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
   // (which loses context when the model header is outside the -U3 window and
   // silently drops sibling-column edge cases). Falls back to diff-only line
   // detection when a content resolver isn't wired (e.g. unit-test callers).
-  for (const file of reviewable) {
-    if (file.kind !== "schema_yml") continue
-    const oldRef = file.oldPath ?? file.path
-    const [oldContent, newContent] = await Promise.all([
-      file.status === "modified" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
-      file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
-    ])
-    all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent }))
+  //
+  // `oldContent` is fetched for anything that has an old side — MODIFIED and
+  // RENAMED. A schema.yml being renamed (e.g. moved to a new subdir) that
+  // also drops a `unique`/`not_null` guardrail must still surface as a finding;
+  // the earlier `status === "modified"` gate silently skipped renames.
+  //
+  // Fetches run in parallel across schema files (a schema-heavy PR could touch
+  // dozens of yml files; serial `git show` per file adds up).
+  const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml")
+  if (schemaFiles.length) {
+    const schemaFindingSets = await Promise.all(
+      schemaFiles.map(async (file) => {
+        const oldRef = file.oldPath ?? file.path
+        const [oldContent, newContent] = await Promise.all([
+          file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
+          file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
+        ])
+        return detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })
+      }),
+    )
+    for (const findings of schemaFindingSets) all.push(findings)
   }
 
   // Architectural dedup: the regex `dbt-patterns`/`rule-catalog` layer is a
diff --git a/packages/opencode/src/altimate/review/run.ts b/packages/opencode/src/altimate/review/run.ts
index 3536804b3..61b6c1c4b 100644
--- a/packages/opencode/src/altimate/review/run.ts
+++ b/packages/opencode/src/altimate/review/run.ts
@@ -118,12 +118,26 @@ async function autoDiscoverManifest(cwd: string): Promise<{ path: string; projec
   }
 }
 
-/** Warn to stderr when the manifest looks stale relative to changed files. */
+/** Warn to stderr when the manifest looks stale relative to changed files.
+ *  Only considers dbt-relevant files (SQL, YAML, Python models, seed CSV,
+ *  docs markdown) — changes to `README.md` at repo root, `.github/`,
+ *  `package.json`, etc. don't affect whether the compiled manifest is still
+ *  valid, so their mtimes shouldn't trigger a stale warning. */
+/** Exported for tests — see review-run-stale.test.ts. */
+export function isManifestAffecting(rel: string): boolean {
+  // dbt source directories — code (sql/py), schema (yml), seeds (csv), and
+  // docs blocks (md files under models/ / snapshots/ / analyses/ / etc.).
+  if (/(^|\/)(models|seeds|snapshots|macros|tests|analyses)\/.*\.(sql|py|yml|yaml|csv|md)$/i.test(rel)) return true
+  // Top-level dbt project config files.
+  if (/(^|\/)(dbt_project|packages|profiles|dependencies)\.ya?ml$/i.test(rel)) return true
+  return false
+}
+
 async function warnIfStale(manifestAbs: string, changedPaths: string[], cwd: string): Promise<void> {
   try {
     const manifestMtime = (await stat(manifestAbs)).mtimeMs
     for (const rel of changedPaths) {
-      // Only compare against tracked, on-disk model files (schema.yml or SQL)
+      if (!isManifestAffecting(rel)) continue
       const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel)
       try {
         const changedMtime = (await stat(abs)).mtimeMs
diff --git a/packages/opencode/test/altimate/review-dbt-patterns.test.ts b/packages/opencode/test/altimate/review-dbt-patterns.test.ts
index db347e6b2..3d6e38421 100644
--- a/packages/opencode/test/altimate/review-dbt-patterns.test.ts
+++ b/packages/opencode/test/altimate/review-dbt-patterns.test.ts
@@ -5,6 +5,7 @@ import {
   DEFAULT_RUBRIC,
   type ChangedFile,
 } from "../../src/altimate/review"
+import { dedupe } from "../../src/altimate/review/finding"
 
 // Build a modified-model ChangedFile from a new-SQL body + a synthetic diff
 // where the given lines are "added" (prefixed with +).
@@ -313,7 +314,7 @@ sources:
     expect(f.some((x) => (x.title || "").includes("raw.users.id"))).toBe(true)
   })
 
-  test("schema.yml (structural): added file (no oldContent) surfaces no removal findings", () => {
+  test("schema.yml (structural): added file (status=added, no oldContent) surfaces no removal findings", () => {
     const newContent = `version: 2
 models:
   - name: customers
@@ -322,13 +323,62 @@ models:
         tests: [unique, not_null]
 `
     const f = detectSchemaYmlPatterns(
-      { path: "models/schema.yml", status: "modified", diff: undefined },
+      { path: "models/schema.yml", status: "added", diff: undefined },
       DEFAULT_RUBRIC,
       { oldContent: undefined, newContent },
     )
     expect(f.length).toBe(0)
   })
 
+  test("schema.yml (structural): renamed file with removed test still surfaces finding", () => {
+    // Regression guard: earlier code only fetched oldContent when
+    // status === "modified", so a rename that also dropped a guardrail test
+    // silently bypassed the detector.
+    const oldContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: [unique, not_null]
+`
+    const newContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: []
+`
+    const f = detectSchemaYmlPatterns(
+      {
+        path: "models/marts/_models.yml",
+        status: "renamed",
+        oldPath: "models/_models.yml",
+        diff: undefined,
+      },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    expect(f.length).toBe(2)
+    expect(has(f, "test_coverage", "warning")).toBe(true)
+  })
+
+  test("schema.yml (structural): modified file with oldContent=undefined falls back to diff detection", () => {
+    // When the content resolver couldn't read the old side (e.g. transient
+    // git failure) on a MODIFIED file, we must NOT treat the situation as
+    // "added file, nothing removed" — the diff still contains real removed
+    // lines. Fall back to line-based detection instead of silently dropping.
+    const f = detectSchemaYmlPatterns(
+      {
+        path: "models/marts/_models.yml",
+        status: "modified",
+        diff: "-          - unique\n-          - not_null",
+      },
+      DEFAULT_RUBRIC,
+      { oldContent: undefined, newContent: "version: 2\nmodels: []\n" },
+    )
+    expect(has(f, "test_coverage")).toBe(true)
+  })
+
   test("schema.yml (fallback): diff-only path still surfaces removal (existing test's shape)", () => {
     // Exercises the fallback line-based detection when the orchestrator can't
     // supply file content (e.g. offline CI diffs).
@@ -345,6 +395,160 @@ models:
     expect(has(f, "test_coverage")).toBe(true)
   })
 
+  test("schema.yml (fallback): distinct removals of same test type on different columns each surface", () => {
+    // Regression guard for the earlier dedup bug: (model="", column="", test)
+    // key reduced to just `test` and silently collapsed distinct removals of
+    // the same test type on different columns. The fallback path now emits
+    // one finding per removed test-line.
+    const f = detectSchemaYmlPatterns(
+      {
+        path: "models/marts/_models.yml",
+        status: "modified",
+        // Two `- unique` removed lines representing two different columns
+        // losing the `unique` test in the same PR.
+        diff: "-          - unique\n-          - not_null\n-          - unique\n-          - not_null",
+      },
+      DEFAULT_RUBRIC,
+    )
+    // 4 removed test-lines → 4 findings (fallback preserves per-line detail).
+    expect(f.length).toBe(4)
+    expect(has(f, "test_coverage", "warning")).toBe(true)
+  })
+
+  test("schema.yml (fallback): distinct removals survive the global fingerprint dedupe", () => {
+    // Integration-shape guard: the detector-level test above returns 4
+    // findings, but `runReview` runs a global `dedupe(findings)` step that
+    // fingerprints by (category, file, model, column, ruleKey). If ruleKey
+    // only varies by test-name for the fallback path, two `- unique`
+    // removals share a fingerprint and get merged post-detector. This test
+    // pipes the detector's output through `dedupe` to prove distinct
+    // removals actually reach the user.
+    const f = detectSchemaYmlPatterns(
+      {
+        path: "models/marts/_models.yml",
+        status: "modified",
+        diff: "-          - unique\n-          - not_null\n-          - unique\n-          - not_null",
+      },
+      DEFAULT_RUBRIC,
+    )
+    const deduped = dedupe(f)
+    expect(deduped.length).toBe(4)
+    // All 4 must be test_coverage findings; `unique` removals are `warning`,
+    // and fallback `not_null` (no column attribution) is `suggestion` because
+    // we can't tell if it was on an id/key column.
+    expect(deduped.every((x) => x.category === "test_coverage")).toBe(true)
+    // The 4 fingerprints must be distinct (was the exact regression: without
+    // an occurrence-index discriminator in ruleKey, the two `- unique` and
+    // two `- not_null` removals collapse to 2 findings after dedupe).
+    expect(new Set(deduped.map((x) => x.id)).size).toBe(4)
+    // 2 `- unique` (warning) + 2 `- not_null` (suggestion) — the exact split.
+    expect(deduped.filter((x) => x.severity === "warning").length).toBe(2)
+    expect(deduped.filter((x) => x.severity === "suggestion").length).toBe(2)
+  })
+
+  test("schema.yml (structural): distinct column removals also survive global dedupe", () => {
+    // Structural attribution provides (model.column.test) uniqueness natively,
+    // but the guard is worth codifying so future rule-key changes don't
+    // silently regress into the fingerprint-collision failure mode.
+    const oldContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: [unique]
+      - name: email
+        tests: [unique]
+`
+    const newContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: []
+      - name: email
+        tests: []
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/marts/_models.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    const deduped = dedupe(f)
+    expect(deduped.length).toBe(2)
+    expect(new Set(deduped.map((x) => x.id)).size).toBe(2)
+  })
+
+  test("snapshot yml property file (structural): removed test surfaces finding", () => {
+    // Regression: snapshot YAML property files were previously classified as
+    // `snapshot` kind (blocking the `schema_yml` gate in the orchestrator).
+    // Now `.yml` files under snapshots/ classify as `schema_yml` while `.sql`
+    // snapshots stay `snapshot` (tier-forcing catalog rules unchanged).
+    const oldContent = `version: 2
+snapshots:
+  - name: orders_snapshot
+    columns:
+      - name: order_id
+        tests: [unique, not_null]
+`
+    const newContent = `version: 2
+snapshots:
+  - name: orders_snapshot
+    columns:
+      - name: order_id
+        tests: []
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "snapshots/orders_snapshot.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    expect(f.length).toBe(2)
+    expect(has(f, "test_coverage", "warning")).toBe(true)
+  })
+
+  test("schema.yml summary: multi-model removals emit ONE aggregate summary line", () => {
+    // Regression guard: the aggregate "This PR removes N tests total on
+    // model(s) X, Y" summary was previously appended once per model, so a
+    // diff touching two models produced two copies of the same summary in
+    // separate findings. Now emitted once per FILE on the first finding.
+    const oldContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: [unique]
+  - name: orders
+    columns:
+      - name: order_id
+        tests: [unique]
+`
+    const newContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: []
+  - name: orders
+    columns:
+      - name: order_id
+        tests: []
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/schema.yml", status: "modified", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent },
+    )
+    expect(f.length).toBe(2)
+    // Only ONE finding body should contain the "removes N data tests in total"
+    // aggregate summary line; the other should be plain.
+    const summaryCount = f.filter((x) => (x.body || "").includes("data tests in total")).length
+    expect(summaryCount).toBe(1)
+    // And the mentioned model list should include BOTH models it touched.
+    const withSummary = f.find((x) => (x.body || "").includes("data tests in total"))!
+    expect(withSummary.body).toContain("`customers`")
+    expect(withSummary.body).toContain("`orders`")
+  })
+
   test("benign additive column produces NO dbt-pattern finding (precision)", () => {
     const sql = `select id, upper(status) as status_upper from {{ ref('x') }}`
     const f = detectModelPatterns(
diff --git a/packages/opencode/test/altimate/review-run-stale.test.ts b/packages/opencode/test/altimate/review-run-stale.test.ts
new file mode 100644
index 000000000..2e097df94
--- /dev/null
+++ b/packages/opencode/test/altimate/review-run-stale.test.ts
@@ -0,0 +1,51 @@
+import { describe, test, expect } from "bun:test"
+import { isManifestAffecting } from "../../src/altimate/review/run"
+
+/**
+ * Guards on `warnIfStale`'s changed-file filter. The stale warning gates on a
+ * changed file having an mtime newer than the manifest; iterating every path
+ * in the diff (including README.md, package.json, .github/*) triggered
+ * false-positive warnings on unrelated repo activity. The filter admits only
+ * dbt-relevant paths.
+ */
+describe("isManifestAffecting", () => {
+  // ADMITTED — these should trigger a stale-manifest warning when newer than
+  // the compiled manifest.
+  test.each([
+    "models/marts/customers.sql",
+    "models/staging/stg_orders.sql",
+    "models/schema.yml",
+    "models/marts/_models.yml",
+    "models/customers.md", // dbt docs block
+    "seeds/lookup.csv",
+    "snapshots/orders_snapshot.sql",
+    "snapshots/orders_snapshot.yml",
+    "macros/generate_schema_name.sql",
+    "tests/singular/no_orphan_orders.sql",
+    "analyses/gross_margin.sql",
+    "dbt_project.yml",
+    "packages.yml",
+    "profiles.yml",
+    "dependencies.yml",
+  ])("admits: %s", (rel) => {
+    expect(isManifestAffecting(rel)).toBe(true)
+  })
+
+  // REJECTED — these should NOT trigger a stale-manifest warning because
+  // touching them doesn't invalidate the compiled dbt manifest.
+  test.each([
+    "README.md",
+    "CHANGELOG.md",
+    "package.json",
+    ".github/workflows/ci.yml",
+    ".gitignore",
+    "docs/README.md",
+    "Dockerfile",
+    "scripts/deploy.sh",
+    "target/manifest.json", // the manifest itself
+    "target/compiled/foo.sql", // compiled artifacts
+    "tests/e2e/setup.ts",
+  ])("rejects: %s", (rel) => {
+    expect(isManifestAffecting(rel)).toBe(false)
+  })
+})
diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts
index 99de1966d..48da94ae8 100644
--- a/packages/opencode/test/altimate/review.test.ts
+++ b/packages/opencode/test/altimate/review.test.ts
@@ -366,6 +366,10 @@ describe("diff-filter", () => {
     expect(classifyDbtFile("models/marts/_marts.yml")).toBe("schema_yml")
     expect(classifyDbtFile("macros/x.sql")).toBe("macro")
     expect(classifyDbtFile("snapshots/s.sql")).toBe("snapshot")
+    // Snapshot YAML property files carry column/test declarations and route
+    // to the schema_yml detector, not the snapshot catalog rules.
+    expect(classifyDbtFile("snapshots/orders_snapshot.yml")).toBe("schema_yml")
+    expect(classifyDbtFile("snapshots/orders_snapshot.yaml")).toBe("schema_yml")
     expect(classifyDbtFile("seeds/c.csv")).toBe("seed")
     expect(classifyDbtFile("dbt_project.yml")).toBe("project_config")
     expect(classifyDbtFile("models/marts/m.py")).toBe("python_model")

From 39718d5d1ddf3a3ccd811747eceb185c21cc7938 Mon Sep 17 00:00:00 2001
From: Haider <haider@altimate.ai>
Date: Tue, 21 Jul 2026 18:06:02 +0530
Subject: [PATCH 5/6] =?UTF-8?q?perf(review):=20address=20PR-1027=20round-3?=
 =?UTF-8?q?=20nits=20(deleted-file=20fetch,=20summary=20alloc,=20shift=20O?=
 =?UTF-8?q?(N=C2=B2))?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Three centralized-review findings on the previous commit `6729f5374`,
all perf/hygiene:

## MEDIUM — schema.yml orchestrator fetched `oldContent` for deleted files

`file.status !== "added"` on the gate matched `"deleted"` too, triggering
a `git show` round-trip whose result the detector immediately discarded
(early-returns `[]` for `status === "deleted"`).

Fix: filter deleted schema files out of the loop upfront —
`reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted")`.
The `newContent` fetch also simplifies to unconditional
`getContent?.(file.path, "new")` since deleted files never reach the loop.

## MEDIUM — `distinctModels`/`modelClause` recomputed on every loop iteration

The `[...new Set(...).filter(...).map(...).join(...)]` chain was evaluated
on every removal even though the result is only used when `shouldEmitSummary`
is true (once per file). Moved the computation inside the guard.

## LOW — `fallbackDiscriminators.shift()` inside the loop is O(N²)

`Array.prototype.shift()` re-indexes all remaining elements on each call.
Replaced with a hoisted `let fallbackIdx = 0` + `fallbackDiscriminators[fallbackIdx++]`.
Negligible on 2-3 removals but grows quadratically on wider PRs.

## Test status

188 pass / 0 fail across the 7 review test files. Typecheck clean. No
behavior change beyond the deleted-file fetch skip (which is a pure
performance improvement — same output, fewer round-trips).
---
 .../src/altimate/review/dbt-patterns.ts       | 28 ++++++++++++-------
 .../src/altimate/review/orchestrate.ts        |  9 ++++--
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts
index 14fa6c9b7..e5d5fb3d0 100644
--- a/packages/opencode/src/altimate/review/dbt-patterns.ts
+++ b/packages/opencode/src/altimate/review/dbt-patterns.ts
@@ -1050,6 +1050,11 @@ export function detectSchemaYmlPatterns(
   // global summary. A single boolean guard is enough.
   let summaryEmitted = false
 
+  // Consumed in lock-step with the removals loop below (fallback path only).
+  // Using an index avoids `shift()`, which would be O(N) per call on the
+  // discriminator array — negligible on 2-3 removals but O(N²) on a large
+  // schema.yml PR that drops many tests.
+  let fallbackIdx = 0
   for (const r of removals) {
     const isUniquenessSignal = r.test === "unique"
     const notNullOnLikelyPK =
@@ -1096,16 +1101,19 @@ export function detectSchemaYmlPatterns(
     // Emit the aggregate summary once per file, on the first finding, and
     // only when there are multiple removals to summarise. Uses the file-scoped
     // `summaryEmitted` flag rather than a per-model guard so a diff touching
-    // three models emits ONE summary, not three.
+    // three models emits ONE summary, not three. Distinct-models computation
+    // is gated inside the branch so we don't pay for the Set/spread/map on
+    // every loop iteration when the summary won't be attached.
     const shouldEmitSummary = !summaryEmitted && removals.length > 1
-    if (shouldEmitSummary) summaryEmitted = true
-    const distinctModels = [...new Set(removals.map((x) => x.model).filter(Boolean))]
-    const modelClause = distinctModels.length
-      ? ` on model(s) ${distinctModels.map((m) => `\`${m}\``).join(", ")}`
-      : ""
-    const bodyTail = shouldEmitSummary
-      ? `\n\n_This PR removes ${removals.length} data tests in total${modelClause}._`
-      : ""
+    let bodyTail = ""
+    if (shouldEmitSummary) {
+      summaryEmitted = true
+      const distinctModels = [...new Set(removals.map((x) => x.model).filter(Boolean))]
+      const modelClause = distinctModels.length
+        ? ` on model(s) ${distinctModels.map((m) => `\`${m}\``).join(", ")}`
+        : ""
+      bodyTail = `\n\n_This PR removes ${removals.length} data tests in total${modelClause}._`
+    }
 
     // ruleKey feeds the finding fingerprint (finding.ts:107). For attributed
     // (structural) findings, `(model.column.test)` is unique per removal in
@@ -1113,7 +1121,7 @@ export function detectSchemaYmlPatterns(
     // distinct removals of the same test type — we append the fallback
     // discriminator so each removed line becomes a distinct finding
     // downstream of the global dedupe.
-    const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators.shift() ?? "#?"}`
+    const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators[fallbackIdx++] ?? "#?"}`
     findings.push(
       makeFinding({
         severity: clampSeverity("test_coverage", sev, "high"),
diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts
index 11e8114c9..7f8fefde3 100644
--- a/packages/opencode/src/altimate/review/orchestrate.ts
+++ b/packages/opencode/src/altimate/review/orchestrate.ts
@@ -1230,14 +1230,19 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
   //
   // Fetches run in parallel across schema files (a schema-heavy PR could touch
   // dozens of yml files; serial `git show` per file adds up).
-  const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml")
+  //
+  // DELETED schema files are filtered upfront because `detectSchemaYmlPatterns`
+  // early-returns `[]` for `status === "deleted"` anyway; not filtering here
+  // would trigger an unnecessary `getContent(oldRef, "old")` fetch (a git-show
+  // round-trip) whose result is thrown away by the detector.
+  const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted")
   if (schemaFiles.length) {
     const schemaFindingSets = await Promise.all(
       schemaFiles.map(async (file) => {
         const oldRef = file.oldPath ?? file.path
         const [oldContent, newContent] = await Promise.all([
           file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
-          file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
+          getContent?.(file.path, "new"),
         ])
         return detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })
       }),

From 310927ec9845b15c6a3e68057a4289821d1bf6ac Mon Sep 17 00:00:00 2001
From: Haider <haider@altimate.ai>
Date: Wed, 22 Jul 2026 02:52:04 +0530
Subject: [PATCH 6/6] fix(review): address PR-1027 round-4 (deleted schema.yml
 + backtick fence + Bun-alias flake + finding attribution)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Four items from cubic-review round-3 + one CI-only test flake:

- **Deleted schema.yml handling (cubic P2)** — `detectSchemaYmlPatterns`
  now diffs old YAML against `{}` when `status === "deleted"`, so
  dropping a whole schema file surfaces every prior test as a removal.
  `orchestrate.ts` no longer filters deleted files and skips
  `newContent` fetch for them (git-show at HEAD would fail).
  Two regression tests: 4-test deletion, and safe-degrade when
  `oldContent` is missing.

- **Backtick-safe tierReasons rendering (cubic P3)** — `format.ts`
  now sizes the inline-code-span fence to `max(backtick-run) + 1`
  and pads leading/trailing backticks with a space, so paths like
  `` `foo`bar` `` can't terminate the span or inject Markdown.
  Regression test covers single/double/leading/trailing/both-ends cases.

- **Structured attribution on removal findings (cubic P3)** —
  `removed_tests` findings now include `model` and `column` on the
  top-level Finding, not just inside `evidence.result`, so downstream
  dedupe / formatting / telemetry see them.

- **TUI network-options test flake (CI-only)** — Bun's transpiler
  statically resolved the `@/cli/network` string literal in
  `toContain('...')` into a `file:///…/network.ts` URL, inverting the
  comparison. Rewritten as `toMatch(/regex/)` so the alias literal
  isn't recognizable to Bun's analyzer.

Additional edge tests per codex round-4 review:
- Deleted schema.yml with unparsable oldContent → diff-only fallback.
- Deleted empty schema.yml → no fabricated findings.
- tierReasons with both leading + trailing backticks → symmetric padding.

Tests: 145 pass, 0 fail in touched suites; full review suite (188 tests) green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
---
 .../src/altimate/review/dbt-patterns.ts       |  67 ++++++++----
 .../opencode/src/altimate/review/format.ts    |  16 ++-
 .../src/altimate/review/orchestrate.ts        |  23 ++--
 .../test/altimate/review-dbt-patterns.test.ts | 101 ++++++++++++++++++
 .../opencode/test/altimate/review.test.ts     |  36 +++++++
 .../opencode/test/cli/tui/command.test.ts     |   7 +-
 6 files changed, 214 insertions(+), 36 deletions(-)

diff --git a/packages/opencode/src/altimate/review/dbt-patterns.ts b/packages/opencode/src/altimate/review/dbt-patterns.ts
index e5d5fb3d0..1467d9833 100644
--- a/packages/opencode/src/altimate/review/dbt-patterns.ts
+++ b/packages/opencode/src/altimate/review/dbt-patterns.ts
@@ -947,42 +947,55 @@ export function detectSchemaYmlPatterns(
   opts: SchemaYmlDetectContent = {},
 ): Finding[] {
   const kind = classifyDbtFile(file.path)
-  if (kind !== "schema_yml" || file.status === "deleted") return []
+  if (kind !== "schema_yml") return []
 
   const removals: Array<{ model: string; column: string; test: string }> = []
   let usedStructural = false
 
+  // Deleting a whole schema.yml removes every test declared in it — arguably
+  // a bigger removal than dropping a single test. Treat the new side as empty
+  // and diff the old document against `{}` so every prior test surfaces as a
+  // removal (cubic-review P2). Requires `oldContent` from the caller.
+  const isDeletedFile = file.status === "deleted"
+
   // PREFERRED: structural YAML diff when we have both sides' content.
   //
   // "Added" file case (no `oldContent` supplied AND status marks the file as
   // newly-added upstream) — the old side is empty, so there's nothing to
   // remove; using the structural path with an empty old set is correct.
   //
+  // "Deleted" file case — the new side is empty; use the structural path
+  // with an empty new set so every old test surfaces as a removal.
+  //
   // "Modified but resolver returned undefined" case (`oldContent === undefined`
   // for a file whose status is `modified` or `renamed`) — the resolver couldn't
   // read the old side. Do NOT treat this as an added file (would silently drop
   // real removals). Fall through to the diff-only fallback below.
   const isAddedFile = file.status === "added"
   const canUseStructural =
-    opts.newContent !== undefined &&
-    // For a real modification/rename we require both sides; without oldContent
-    // fall back to diff parsing so removals in the diff still surface.
-    (isAddedFile || opts.oldContent !== undefined)
+    (isDeletedFile
+      ? opts.oldContent !== undefined
+      : opts.newContent !== undefined &&
+        // For a real modification/rename we require both sides; without oldContent
+        // fall back to diff parsing so removals in the diff still surface.
+        (isAddedFile || opts.oldContent !== undefined))
 
   if (canUseStructural) {
     let oldDoc: unknown = undefined
     let newDoc: unknown = undefined
-    try {
-      newDoc = YAML.parse(opts.newContent as string)
-    } catch (err) {
-      // Debug telemetry: YAML parsers can trip on Jinja injected into a
-      // schema.yml or on non-ASCII quirks; log so failures are diagnosable
-      // rather than silently demoting to the fallback path.
-      Log.create({ service: "review", tag: "detectSchemaYmlPatterns" }).warn(
-        `YAML.parse failed on new content of ${file.path}; falling back to diff-only detection`,
-        err instanceof Error ? { error: err.message } : undefined,
-      )
-      newDoc = undefined
+    if (opts.newContent !== undefined) {
+      try {
+        newDoc = YAML.parse(opts.newContent)
+      } catch (err) {
+        // Debug telemetry: YAML parsers can trip on Jinja injected into a
+        // schema.yml or on non-ASCII quirks; log so failures are diagnosable
+        // rather than silently demoting to the fallback path.
+        Log.create({ service: "review", tag: "detectSchemaYmlPatterns" }).warn(
+          `YAML.parse failed on new content of ${file.path}; falling back to diff-only detection`,
+          err instanceof Error ? { error: err.message } : undefined,
+        )
+        newDoc = undefined
+      }
     }
     if (opts.oldContent !== undefined) {
       try {
@@ -995,13 +1008,18 @@ export function detectSchemaYmlPatterns(
         oldDoc = undefined
       }
     }
-    // Only commit to the structural path when we have a parseable new side
-    // AND (we're on an added file OR the old side also parsed). This preserves
-    // "unparseable old YAML" → fallback, not "structural with empty old = every
-    // removed test looks like a genuine removal".
-    if (newDoc !== undefined && (isAddedFile || oldDoc !== undefined)) {
+    // Only commit to the structural path when the appropriate sides parsed:
+    //  - deleted → old must parse; new is treated as `{}`
+    //  - added   → new must parse; old is treated as `{}`
+    //  - modified/renamed → both must parse
+    // This preserves "unparseable YAML" → fallback rather than
+    // "structural with empty side = every test looks removed/added".
+    const canCommit = isDeletedFile
+      ? oldDoc !== undefined
+      : newDoc !== undefined && (isAddedFile || oldDoc !== undefined)
+    if (canCommit) {
       const oldSet = extractTestOccurrences(oldDoc ?? {})
-      const newSet = extractTestOccurrences(newDoc)
+      const newSet = extractTestOccurrences(isDeletedFile ? {} : newDoc)
       for (const key of oldSet) {
         if (newSet.has(key)) continue
         const [model, column, test] = key.split("\x00")
@@ -1129,6 +1147,11 @@ export function detectSchemaYmlPatterns(
         title,
         body: bodyLead + bodyRationale + bodyTail,
         file: file.path,
+        // Surface the extracted attribution on the top-level Finding so
+        // downstream consumers (dedupe, formatting, telemetry) see it —
+        // not just inside `evidence.result` (cubic-review P3).
+        model: r.model || undefined,
+        column: r.column || undefined,
         confidence: "high",
         evidence: {
           tool: "dbt-patterns",
diff --git a/packages/opencode/src/altimate/review/format.ts b/packages/opencode/src/altimate/review/format.ts
index a096249a4..0befe5883 100644
--- a/packages/opencode/src/altimate/review/format.ts
+++ b/packages/opencode/src/altimate/review/format.ts
@@ -57,7 +57,21 @@ export function renderSummary(env: VerdictEnvelope): string {
   // the signed envelope's tierReasons[]; the summary shows the first 8.
   if (env.tierReasons && env.tierReasons.length) {
     const RENDER_CAP = 8
-    const shown = env.tierReasons.slice(0, RENDER_CAP).map((r) => `\`${r}\``).join(", ")
+    // Pick an inline-code-span fence longer than any backtick run inside `r`
+    // so a path like `packages/…/foo`bar`.sql` cannot terminate the span
+    // (cubic-review P3).
+    const shown = env.tierReasons
+      .slice(0, RENDER_CAP)
+      .map((r) => {
+        const runs = r.match(/`+/g)
+        const maxRun = runs ? Math.max(...runs.map((run) => run.length)) : 0
+        const fence = "`".repeat(maxRun + 1)
+        // If the reason itself starts/ends with a backtick, pad with a space so
+        // the leading/trailing backtick isn't glued to the fence.
+        const pad = /^`|`$/.test(r) ? " " : ""
+        return `${fence}${pad}${r}${pad}${fence}`
+      })
+      .join(", ")
     const overflow =
       env.tierReasons.length > RENDER_CAP
         ? ` (+${env.tierReasons.length - RENDER_CAP} more in verdict envelope)`
diff --git a/packages/opencode/src/altimate/review/orchestrate.ts b/packages/opencode/src/altimate/review/orchestrate.ts
index 7f8fefde3..07c60565b 100644
--- a/packages/opencode/src/altimate/review/orchestrate.ts
+++ b/packages/opencode/src/altimate/review/orchestrate.ts
@@ -1223,26 +1223,27 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
   // silently drops sibling-column edge cases). Falls back to diff-only line
   // detection when a content resolver isn't wired (e.g. unit-test callers).
   //
-  // `oldContent` is fetched for anything that has an old side — MODIFIED and
-  // RENAMED. A schema.yml being renamed (e.g. moved to a new subdir) that
-  // also drops a `unique`/`not_null` guardrail must still surface as a finding;
-  // the earlier `status === "modified"` gate silently skipped renames.
+  // `oldContent` is fetched for anything that has an old side — MODIFIED,
+  // RENAMED, and DELETED. A schema.yml being renamed (e.g. moved to a new
+  // subdir) that also drops a `unique`/`not_null` guardrail must still surface
+  // as a finding; the earlier `status === "modified"` gate silently skipped
+  // renames. A whole schema.yml being DELETED removes every test declared in
+  // it — an even bigger removal — so the detector runs against `oldContent`
+  // vs an empty new document (cubic-review P2).
+  //
+  // `newContent` fetch is skipped for deleted files (nothing at HEAD to read;
+  // git-show would fail).
   //
   // Fetches run in parallel across schema files (a schema-heavy PR could touch
   // dozens of yml files; serial `git show` per file adds up).
-  //
-  // DELETED schema files are filtered upfront because `detectSchemaYmlPatterns`
-  // early-returns `[]` for `status === "deleted"` anyway; not filtering here
-  // would trigger an unnecessary `getContent(oldRef, "old")` fetch (a git-show
-  // round-trip) whose result is thrown away by the detector.
-  const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted")
+  const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml")
   if (schemaFiles.length) {
     const schemaFindingSets = await Promise.all(
       schemaFiles.map(async (file) => {
         const oldRef = file.oldPath ?? file.path
         const [oldContent, newContent] = await Promise.all([
           file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
-          getContent?.(file.path, "new"),
+          file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
         ])
         return detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })
       }),
diff --git a/packages/opencode/test/altimate/review-dbt-patterns.test.ts b/packages/opencode/test/altimate/review-dbt-patterns.test.ts
index 3d6e38421..32a79c6f4 100644
--- a/packages/opencode/test/altimate/review-dbt-patterns.test.ts
+++ b/packages/opencode/test/altimate/review-dbt-patterns.test.ts
@@ -549,6 +549,107 @@ models:
     expect(withSummary.body).toContain("`orders`")
   })
 
+  test("deleted schema.yml: every prior test surfaces as a removal finding (cubic-review P2)", () => {
+    // Regression: deleting a whole schema.yml removes every test declared in
+    // it — arguably a bigger removal than dropping a single test. Previously
+    // the detector early-returned `[]` for `status === "deleted"`, so the
+    // deletion went unnoticed. Now the detector diffs the old document against
+    // an empty new document.
+    const oldContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests: [unique, not_null]
+      - name: email
+        tests: [not_null]
+  - name: orders
+    columns:
+      - name: order_id
+        tests: [unique]
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/marts/_models.yml", status: "deleted", diff: undefined },
+      DEFAULT_RUBRIC,
+      // Only oldContent is available for a deleted file; newContent is undefined
+      // because git-show at HEAD would fail.
+      { oldContent, newContent: undefined },
+    )
+    // 4 removals: customers.customer_id.unique, customers.customer_id.not_null,
+    // customers.email.not_null, orders.order_id.unique
+    expect(f.length).toBe(4)
+    expect(has(f, "test_coverage", "warning")).toBe(true)
+    // Structured attribution surfaces on the top-level Finding, not just in
+    // evidence.result (cubic-review P3).
+    const byModel = f.filter((x) => x.model === "customers")
+    expect(byModel.length).toBe(3)
+    const byColumn = f.filter((x) => x.column === "email")
+    expect(byColumn.length).toBe(1)
+  })
+
+  test("deleted schema.yml without oldContent: no findings (safe degrade)", () => {
+    // Without the old side, we can't know what tests to flag as removed.
+    // The detector should degrade to `[]` rather than fabricating findings.
+    const f = detectSchemaYmlPatterns(
+      { path: "models/marts/_models.yml", status: "deleted", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent: undefined, newContent: undefined },
+    )
+    expect(f.length).toBe(0)
+  })
+
+  test("deleted schema.yml with unparsable oldContent + deletion diff → fallback surfaces removals", () => {
+    // Structural path can't commit (old YAML fails to parse), so the diff-only
+    // fallback runs. Confirms: unparsable-old → fallback, not silent drop.
+    const oldContent = `version: 2
+models:
+  - name: customers
+    columns:
+      - name: customer_id
+        tests:
+          - not_null   # <-- unterminated block below causes parse failure
+    tests: [
+`
+    // Diff uses the block-list shape the fallback regex targets
+    // (`-      - unique` / `-      - not_null`).
+    const diff = `--- a/models/marts/_models.yml
++++ /dev/null
+@@ -1,8 +0,0 @@
+-version: 2
+-models:
+-  - name: customers
+-    columns:
+-      - name: customer_id
+-        tests:
+-          - unique
+-          - not_null
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/marts/_models.yml", status: "deleted", diff },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent: undefined },
+    )
+    // Diff-only fallback picks up the removed `- unique` and `- not_null`
+    // lines, so we still emit at least one removal finding rather than silently
+    // dropping when the structural path couldn't commit.
+    expect(f.length).toBeGreaterThan(0)
+    expect(has(f, "test_coverage", "warning")).toBe(true)
+  })
+
+  test("deleted empty schema.yml: no fabricated findings (locks in precision)", () => {
+    // Old side parses cleanly but declares no tests. Ensures the structural
+    // loop doesn't fabricate findings when oldSet is empty.
+    const oldContent = `version: 2
+models: []
+`
+    const f = detectSchemaYmlPatterns(
+      { path: "models/marts/_models.yml", status: "deleted", diff: undefined },
+      DEFAULT_RUBRIC,
+      { oldContent, newContent: undefined },
+    )
+    expect(f.length).toBe(0)
+  })
+
   test("benign additive column produces NO dbt-pattern finding (precision)", () => {
     const sql = `select id, upper(status) as status_upper from {{ ref('x') }}`
     const f = detectModelPatterns(
diff --git a/packages/opencode/test/altimate/review.test.ts b/packages/opencode/test/altimate/review.test.ts
index 48da94ae8..a1ff86436 100644
--- a/packages/opencode/test/altimate/review.test.ts
+++ b/packages/opencode/test/altimate/review.test.ts
@@ -1881,6 +1881,42 @@ describe("orchestrate", () => {
     expect(env.summary.degraded).toBe(true)
   })
 
+  test("renderSummary escapes backticks in tierReasons (cubic-review P3)", async () => {
+    // Regression: a fixed single-backtick fence around each tier reason meant a
+    // path like `weird`path`.sql` would terminate the code span mid-way and
+    // inject Markdown into the summary. Fence now sizes to max(backtick-run)+1
+    // and pads leading/trailing backticks with a space.
+    const env = buildEnvelope({
+      findings: [],
+      tier: "full",
+      mode: "comment",
+      generatedAt: "2026-05-29T00:00:00Z",
+      tierReasons: [
+        "models/x.sql", // plain path — one-backtick fence still fine
+        "path with `single` inside", // needs 2-backtick fence
+        "path with ``double`` inside", // needs 3-backtick fence
+        "`starts-with-backtick",
+        "ends-with-backtick`",
+        "`both-ends`", // both leading + trailing backticks — symmetric padding
+      ],
+    })
+    const summary = renderSummary(env)
+    // Sanity: the tier line exists
+    expect(summary).toContain("Tier: full")
+    // A path with `single` inside must be wrapped by a fence longer than the
+    // longest backtick run inside it. Two-backtick fence sees `single` as a
+    // single-backtick run inside the code span → renders correctly.
+    expect(summary).toContain("``path with `single` inside``")
+    // Three-backtick fence for two-backtick run inside.
+    expect(summary).toContain("```path with ``double`` inside```")
+    // Leading/trailing backticks get space padding so the fence doesn't fuse
+    // with the reason text.
+    expect(summary).toContain("`` `starts-with-backtick ``")
+    expect(summary).toContain("`` ends-with-backtick` ``")
+    // Both ends have backticks — same single pad wraps both sides symmetrically.
+    expect(summary).toContain("`` `both-ends` ``")
+  })
+
   test("renderSummary + inlineComments produce marker + structured output", async () => {
     const env = buildEnvelope({
       findings: [
diff --git a/packages/opencode/test/cli/tui/command.test.ts b/packages/opencode/test/cli/tui/command.test.ts
index 896a29ab1..7c8c68959 100644
--- a/packages/opencode/test/cli/tui/command.test.ts
+++ b/packages/opencode/test/cli/tui/command.test.ts
@@ -7,8 +7,11 @@ describe("tui command", () => {
   test("resolves network options through the config-aware resolver", async () => {
     const source = await tuiCommandSource()
 
-    expect(source).toContain('import { withNetworkOptions, resolveNetworkOptions } from "@/cli/network"')
-    expect(source).toContain('import { AppRuntime } from "@/effect/app-runtime"')
+    // Assertions use regex (not string literals) so Bun's transpiler doesn't
+    // statically resolve `@/cli/network` into `file:///…/network.ts` in the
+    // expected value — a CI-only cache quirk that inverts the comparison.
+    expect(source).toMatch(/import \{ withNetworkOptions, resolveNetworkOptions \} from "@\/cli\/network"/)
+    expect(source).toMatch(/import \{ AppRuntime \} from "@\/effect\/app-runtime"/)
     expect(source).toContain("await AppRuntime.runPromise(resolveNetworkOptions(args))")
     expect(source).not.toContain("resolveNetworkOptionsNoConfig(args)")
   })