review: --no-ai fix + --explain-tier / --force-tier flags + manifest auto-discovery + column-aware schema.yml test-removal

packages/opencode/src/altimate/review/compiled.ts

@@ -1,6 +1,7 @@
 import { promises as fs } from "node:fs"
 import path from "node:path"
 import YAML from "yaml"
+import { safeReadInside } from "./git"
 
 /**
  * Resolve dbt's COMPILED SQL for static analysis.
@@ -36,12 +37,20 @@ export async function dbtProjectName(cwd: string): Promise<string | undefined> {
 }
 
 export interface CompiledResolverOptions {
+  /** dbt project root (the dir containing `dbt_project.yml`). */
   cwd: string
   projectName?: string
   /** Directory holding HEAD-side compiled SQL (relative to cwd). */
   headDir?: string
   /** Directory holding BASE-side compiled SQL (relative to cwd). */
   baseDir?: string
+  /** Prefix within the repo-relative file path that maps to `cwd`.
+   *  For a monorepo where the dbt project lives at `packages/dbt/`, callers
+   *  pass `pathPrefix: "packages/dbt"` so a repo-relative path like
+   *  `packages/dbt/models/foo.sql` resolves inside the dbt project root as
+   *  `models/foo.sql` before joining with `target/compiled/<project>/`.
+   *  Omit when `cwd` IS the repo root. */
+  pathPrefix?: string
 }
 
 /**
@@ -52,15 +61,29 @@ export function makeCompiledResolver(opts: CompiledResolverOptions) {
   const project = opts.projectName
   const headDir = opts.headDir ?? "target/compiled"
   const baseDir = opts.baseDir ?? "target-base/compiled"
+  // Normalise the prefix so both "" and "." mean "no prefix". Match against
+  // git-style forward slashes because `git diff --name-status` always emits
+  // POSIX separators regardless of platform. `path.relative()` on Windows
+  // returns backslashes, so callers passing that verbatim would produce a
+  // prefix that never matches — normalise here (codex R20 review HIGH).
+  const prefix =
+    opts.pathPrefix && opts.pathPrefix !== "."
+      ? opts.pathPrefix.replace(/\\/g, "/").replace(/\/+$/, "")
+      : ""
 
   return async (file: string, side: "old" | "new"): Promise<string | undefined> => {
     if (!project) return undefined
+    // When the dbt project sits inside a subdir of the repo, `file` (from
+    // `git diff --name-status`) is repo-root relative and always uses
+    // POSIX separators. Strip the mapped prefix so it becomes dbt-root
+    // relative before joining with `cwd` (which IS the dbt root). Without
+    // this the compiled resolver silently misses in monorepo layouts.
+    const rel = prefix && (file === prefix || file.startsWith(prefix + "/")) ? file.slice(prefix.length + 1) : file
     const root = side === "new" ? headDir : baseDir
-    const full = path.join(opts.cwd, root, project, file)
-    try {
-      return await fs.readFile(full, "utf8")
-    } catch {
-      return undefined
-    }
+    const compiledRoot = path.join(opts.cwd, root, project)
+    // Shared realpath containment check — matches makeContentResolver's
+    // symlink-safe read so a future tweak to the containment logic can't
+    // leave one call site behind (cubic + kilo suggestion).
+    return await safeReadInside(compiledRoot, rel)
   }
 }

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"

packages/opencode/src/altimate/review/format.ts

@@ -28,7 +28,12 @@ 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)`
+  // 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})`
 }
 
 /** Full PR/MR summary comment body (markdown), prefixed with the dedup marker. */
@@ -43,6 +48,37 @@ 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) {
+    const RENDER_CAP = 8
+    // 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)`
+        : ""
+    lines.push(`> 🧭 **Tier: ${env.tier}** — ${shown}${overflow}`, "")
+  }
+
   if (!env.findings.length) {
     lines.push("No issues found in the changed dbt models. 🎉", "")
   } else {

packages/opencode/src/altimate/review/git.ts

@@ -62,10 +62,42 @@ export async function collectChangedFiles(opts: CollectOptions): Promise<Changed
   )
 }
 
+/** Read the given file only when its realpath sits inside the resolved root.
+ *  Blocks the "tracked symlink escapes the root" class of attack — e.g. a
+ *  `models/evil.sql → /etc/passwd` symlink that would otherwise leak external
+ *  files into the review pipeline (coderabbit + cubic security review).
+ *  Returns undefined when the target escapes, is missing, or realpath fails.
+ *
+ *  Exported for the compiled-SQL resolver in compiled.ts so both content
+ *  paths go through the same containment check — cubic + kilo suggested
+ *  extracting the shared helper so a future security tweak (Windows
+ *  case-sensitivity, trailing separator) can't leave one call site
+ *  behind. */
+export async function safeReadInside(root: string, rel: string): Promise<string | undefined> {
+  try {
+    // Realpath both sides so a symlink IN the repo, or a repo checked out
+    // under a symlinked path (`/var` → `/private/var` on macOS), still
+    // compares apples to apples. Missing realpath calls on the target fail
+    // fast into the catch (no read attempted).
+    const rootReal = await fs.realpath(root)
+    const targetReal = await fs.realpath(path.join(root, rel))
+    // Ensure `targetReal` is inside `rootReal` using a separator-aware
+    // startsWith check so `/repo-backup` doesn't count as inside `/repo`.
+    const sep = path.sep
+    if (targetReal !== rootReal && !targetReal.startsWith(rootReal + sep)) return undefined
+    return await fs.readFile(targetReal, "utf8")
+  } catch {
+    return undefined
+  }
+}
+
 /** Build a getContent(path, side) resolver over git refs / the working tree.
  *  `renames` maps a new path → its old path so the "old" side of a renamed file
- *  resolves from where it actually lived at `base` (not the post-rename path). */
-export function makeContentResolver(opts: CollectOptions & { renames?: Map<string, string> }) {
+ *  resolves from where it actually lived at `base` (not the post-rename path).
+ *  `gitRoot` is the repository top-level (from `git rev-parse --show-toplevel`).
+ *  When omitted, working-tree reads fall back to `opts.cwd`, which is only
+ *  correct when the CLI is invoked from the repo root. */
+export function makeContentResolver(opts: CollectOptions & { renames?: Map<string, string>; gitRoot?: string }) {
   return async (file: string, side: "old" | "new"): Promise<string | undefined> => {
     try {
       if (side === "old") {
@@ -75,13 +107,38 @@ export function makeContentResolver(opts: CollectOptions & { renames?: Map<strin
       if (opts.head) {
         return await git(["show", `${opts.head}:${file}`], opts.cwd)
       }
-      return await fs.readFile(path.join(opts.cwd, file), "utf8")
+      // File paths from `git diff --name-status` are repo-root relative,
+      // NOT `opts.cwd` relative. When the CLI is invoked from a subdirectory
+      // the naive `path.join(opts.cwd, file)` double-joins and fails ENOENT,
+      // returning undefined and silently demoting downstream detectors to
+      // the diff-only fallback. Root at the resolved git top-level when
+      // supplied by the caller; fall back to `opts.cwd` when we couldn't
+      // resolve it (non-git or bare-repo contexts). Reads are containment-
+      // checked (symlink-safe) — see safeReadInside.
+      const root = opts.gitRoot ?? opts.cwd
+      return await safeReadInside(root, file)
     } catch {
       return undefined
     }
   }
 }
 
+/** Resolve the repository top-level (`git rev-parse --show-toplevel`).
+ *  Used to root working-tree FS reads and existence checks at the repo root
+ *  regardless of the caller's cwd. Returns undefined outside a git repo.
+ *  Strips only the git-emitted terminator (`\r\n` or `\n`) rather than
+ *  `trim()` — a path with legitimate leading/trailing whitespace stays
+ *  intact (cubic-review P3). */
+export async function gitRepoRoot(cwd: string): Promise<string | undefined> {
+  try {
+    const out = await git(["rev-parse", "--show-toplevel"], cwd)
+    const root = out.replace(/[\r\n]+$/, "")
+    return root || undefined
+  } catch {
+    return undefined
+  }
+}
+
 /** Resolve a sensible default base ref (merge-base with origin/main/master). */
 export async function defaultBaseRef(cwd: string): Promise<string> {
   for (const candidate of ["origin/main", "origin/master", "main", "master"]) {

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,31 @@ 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 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
+  const tierReasons = tierForced
+    ? [
+        `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])
 
@@ -1196,8 +1217,38 @@ 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.
-  for (const file of reviewable) {
-    if (file.kind === "schema_yml") all.push(detectSchemaYmlPatterns(file, input.rubric))
+  // 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).
+  //
+  // `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).
+  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
@@ -1349,6 +1400,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)
 }