Skip to content

Commit ae52fe0

Browse files
author
Haider
committed
fix(review): v0.9.3 pre-release review findings
- Stamp `Installation.VERSION` into `engine.cliVersion` on the signed verdict envelope so auditors reconstructing a stored verdict months later can identify which policy version generated it. The version is included in the canonical body, so tampering breaks the HMAC signature. - Add `staleManifest` boolean to the envelope, populated when a change- affecting source file has been modified after the manifest was written. Renames the internal `warnIfStale` to `detectStaleManifest` and removes the `opts.head` gate so the local working-tree workflow (compile once, edit for an hour, then review) also surfaces the signal instead of silently under-warning. - Emit `tierReasons[]` on the envelope whenever the classifier lands on `full` tier, not only when `--explain-tier` / `--force-tier` / `pathTokenConfigError` fires. A REQUEST_CHANGES on a schema.yml diff now carries the "why" in the PR-comment blockquote by default; `trivial` / `lite` stay quiet to avoid noise on approvals. - Document `--explain-tier`, `--force-tier`, `riskTierPathTokens`, and manifest auto-discovery in `docs/docs/usage/dbt-pr-review.md`.
1 parent 39d7d94 commit ae52fe0

5 files changed

Lines changed: 74 additions & 21 deletions

File tree

docs/docs/usage/dbt-pr-review.md

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,13 @@ Options:
106106
|------|-------------|
107107
| `--base <ref>` | Base git ref. Defaults to the merge-base with `origin/main`. |
108108
| `--head <ref>` | Head git ref. Omit to review the working tree. |
109-
| `--manifest <path>` | Path to the compiled `manifest.json` (default `target/manifest.json`). |
109+
| `--manifest <path>` | Path to the compiled `manifest.json`. When omitted, the reviewer walks up from the current directory to find the nearest `dbt_project.yml` and uses its adjacent `target/manifest.json`; the discovered path is logged to stderr. |
110110
| `--mode comment\|gate` | `comment` never blocks; `gate` exits non-zero on `REQUEST_CHANGES`. |
111111
| `--severity <level>` | Minimum severity to surface: `critical`, `warning`, `suggestion`. |
112112
| `--post` | Post the verdict to the GitHub PR (uses `GITHUB_TOKEN` + the Actions event). |
113113
| `--no-ai` | Disable the advisory LLM reviewer lane (no model calls / cost) — deterministic-only. |
114+
| `--explain-tier` | Emit the classifier's tier-reason list on the verdict envelope so you can see why a diff was rated `trivial`, `lite`, or `full`. Reasons already surface in the PR comment for `full`-tier runs — this flag adds them to `trivial`/`lite` for debugging. |
115+
| `--force-tier <tier>` | **[EXPERIMENTAL / bench debug]** Bypass the classifier and force `trivial` / `lite` / `full`. The verdict envelope carries `tierForced: true` and the classifier's original decision for audit. |
114116
| `--json` / `--output <file>` | Emit the verdict envelope as JSON. |
115117

116118
> **Full vs lint-only.** With a compiled `manifest.json` present, the reviewer
@@ -232,6 +234,9 @@ dataDiff: # OFF by default — see "Data-diff in CI" below
232234
warehouse: "" # connection name; empty = default connection
233235
exclude:
234236
- models/legacy/**
237+
riskTierPathTokens: # OFF by default — see "Risk-tier path tokens" below
238+
finops: [preset:finops] # promote any FinOps-named path to `full` tier
239+
pii: [ssn, email, phone] # or supply your own case-insensitive tokens
235240
rubric:
236241
blockOn: [lineage_breakage, contract_violation, pii_exposure, semantic_change]
237242
warningPatternThreshold: 3
@@ -242,6 +247,17 @@ rubric:
242247
skipNonProdModels: true
243248
```
244249
250+
### Risk-tier path tokens
251+
252+
Named categories under `riskTierPathTokens` promote any diff touching a matching
253+
path to `full` review tier, so those areas never auto-approve on `trivial`
254+
classification. Values are case-insensitive substrings; the `preset:finops` marker
255+
expands to the built-in FinOps keyword list (cost, billing, spend, revenue, etc.).
256+
Prior to v0.9.3 the FinOps list was hardcoded and always on; it is now opt-in via
257+
this config. When a category value is invalid the CLI logs a stderr warning AND
258+
surfaces the error in the verdict envelope's `tierReasons[]` (and in the PR
259+
comment), so a typo can't silently kill your opt-in.
260+
245261
## Data-diff in CI
246262

247263
Static equivalence proves a refactor *can't* change results. **Data-diff** goes

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

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,8 @@ export interface OrchestrateInput {
180180
manifestHash?: string
181181
coreVersion?: string
182182
modelVersion?: string
183+
/** altimate-code CLI release recorded in engine.cliVersion for audit reconstruction. */
184+
cliVersion?: string
183185
/**
184186
* Optional LLM reviewer lane. Injected (not imported) so the orchestrator
185187
* stays pure/unit-testable: production wires `runAiReview` (harness LLM);
@@ -194,6 +196,8 @@ export interface OrchestrateInput {
194196
explainTier?: boolean
195197
/** G2 — override the classifier's tier decision. Envelope carries tierForced:true. */
196198
forceTier?: "trivial" | "lite" | "full"
199+
/** Manifest was stale relative to change-affecting files (see run.ts::detectStaleManifest). */
200+
staleManifest?: boolean
197201
}
198202

199203
/** Derive the dbt model name from a model file path. */
@@ -1423,17 +1427,19 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
14231427
tier,
14241428
mode: input.mode,
14251429
rubric: input.rubric,
1426-
engine: { core: input.coreVersion, model: input.modelVersion },
1430+
engine: { core: input.coreVersion, model: input.modelVersion, cliVersion: input.cliVersion },
14271431
manifestHash: input.manifestHash,
1432+
staleManifest: input.staleManifest,
14281433
generatedAt: input.generatedAt,
14291434
degraded,
14301435
// Include tierReasons whenever `--explain-tier` / `--force-tier` is set,
1431-
// OR when a riskTierPathTokens config error was caught above — the error
1432-
// must surface in the envelope (and downstream PR comment) even in a
1433-
// normal `comment`/`gate` run, otherwise a config typo silently kills
1434-
// the user's opt-in with only a stderr trace they'll never see
1435-
// (coderabbit review, PR #1028 orchestrate.ts:1129).
1436-
tierReasons: input.explainTier || tierForced || pathTokenConfigError ? tierReasons : undefined,
1436+
// when a riskTierPathTokens config error was caught above, OR when the
1437+
// classifier lands on `full` tier — a naturally-full run is a customer-
1438+
// visible policy call ("why did my YAML-only diff get REQUEST_CHANGES?")
1439+
// and the reason list is what makes the verdict debuggable without the
1440+
// customer having to re-run with --explain-tier. `trivial`/`lite` runs
1441+
// stay silent to avoid noise on approvals.
1442+
tierReasons: input.explainTier || tierForced || pathTokenConfigError || tier === "full" ? tierReasons : undefined,
14371443
tierForced: tierForced ? true : undefined,
14381444
tierClassified: tierForced ? classifiedTier : undefined,
14391445
})

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

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ export interface ReviewPullRequestOptions {
3737
/** Model identifier recorded in the envelope. */
3838
modelVersion?: string
3939
coreVersion?: string
40+
/** altimate-code CLI release recorded in engine.cliVersion for audit reconstruction. */
41+
cliVersion?: string
4042
/** Disable the LLM reviewer lane (default: enabled; self-degrades if no model). */
4143
noAi?: boolean
4244
/** PR metadata for the AI reviewer's intent check. */
@@ -129,7 +131,7 @@ async function autoDiscoverManifest(cwd: string): Promise<{ path: string; projec
129131
* docs markdown blocks) or top-level dbt config. `README.md` at repo root,
130132
* `.github/`, `package.json`, etc. are excluded so their mtimes don't
131133
* trigger a stale warning. Exported for tests — see review-run-stale.test.ts.
132-
* Referenced by `warnIfStale`. */
134+
* Referenced by `detectStaleManifest`. */
133135
export function isManifestAffecting(rel: string): boolean {
134136
// Code / schema / seed CSV live under any dbt source directory.
135137
if (/(^|\/)(models|seeds|snapshots|macros|tests|analyses)\/.*\.(sql|py|yml|yaml|csv)$/i.test(rel)) return true
@@ -144,12 +146,14 @@ export function isManifestAffecting(rel: string): boolean {
144146
return false
145147
}
146148

147-
/** Warn to stderr when the manifest looks stale relative to changed files.
148-
* Compares the manifest's mtime against every change-affecting path (per
149-
* `isManifestAffecting`); a single change newer than the manifest is
150-
* enough to fire the warning once and return. Non-fatal — the review
151-
* proceeds against the (possibly stale) manifest. */
152-
async function warnIfStale(manifestAbs: string, changedPaths: string[], fsRoot: string): Promise<void> {
149+
/** Detect a stale manifest and warn to stderr. Returns `true` when a
150+
* change-affecting file was modified after the manifest — signalling the
151+
* verdict may have been computed against out-of-date metadata. Non-fatal;
152+
* the review proceeds against the (possibly stale) manifest and the
153+
* caller mirrors the flag onto the signed envelope so a downstream
154+
* auditor can distinguish stale-manifest verdicts from clean ones
155+
* (stderr alone is easy for CI to swallow). */
156+
async function detectStaleManifest(manifestAbs: string, changedPaths: string[], fsRoot: string): Promise<boolean> {
153157
try {
154158
const manifestMtime = (await stat(manifestAbs)).mtimeMs
155159
for (const rel of changedPaths) {
@@ -166,7 +170,7 @@ async function warnIfStale(manifestAbs: string, changedPaths: string[], fsRoot:
166170
`⚠️ manifest ${manifestAbs} appears stale — ${rel} was modified after the manifest was written. ` +
167171
`Re-run \`dbt compile\` (or \`dbt build\`) to refresh before reviewing.\n`,
168172
)
169-
return
173+
return true
170174
}
171175
} catch {
172176
/* file not on disk (e.g. removed by the change) — skip */
@@ -175,6 +179,7 @@ async function warnIfStale(manifestAbs: string, changedPaths: string[], fsRoot:
175179
} catch {
176180
/* manifest unreadable → detectDialect will have already returned undefined */
177181
}
182+
return false
178183
}
179184

180185
export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise<VerdictEnvelope> {
@@ -241,11 +246,17 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
241246
}
242247
}
243248
}
244-
// Freshness check: warn (don't fail) when the manifest predates changed files.
245-
// Skip when we're diffing against the working tree (mtime signal is noisy
246-
// during active edits) — only warn when the caller explicitly pinned a head
247-
// ref, which is the CI / bench shape where a stale manifest is a real risk.
248-
if (opts.head) await warnIfStale(manifestAbs, changedFiles.map((f) => f.path), gitRoot)
249+
// Freshness check: detect (and warn — non-fatal) when the manifest predates
250+
// change-affecting files. Runs for both the pinned-head CI shape AND the
251+
// working-tree local shape — the local scenario "dbt compile once, edit for
252+
// an hour, then altimate review" is where staleness actually bites in
253+
// practice, so gating this behind `--head` (the prior behavior) silently
254+
// under-warned the most common developer workflow. mtime granularity on
255+
// the working tree can be noisy during a live edit session, but the check
256+
// is limited to files that would materially change the manifest
257+
// (`isManifestAffecting`), so noise is bounded. Return value is stamped
258+
// into the signed envelope so downstream auditors see it too.
259+
const staleManifest = await detectStaleManifest(manifestAbs, changedFiles.map((f) => f.path), gitRoot)
249260

250261
// Resolve the SQL dialect: explicit config wins; otherwise auto-detect from
251262
// the dbt manifest's `adapter_type` (so a BigQuery/Redshift project isn't
@@ -306,10 +317,12 @@ export async function reviewPullRequest(opts: ReviewPullRequestOptions): Promise
306317
manifestHash: mhash,
307318
modelVersion: opts.modelVersion,
308319
coreVersion: opts.coreVersion,
320+
cliVersion: opts.cliVersion,
309321
aiReview: opts.noAi || config.ai === false ? undefined : runAiReview,
310322
prTitle: opts.prTitle,
311323
prBody: opts.prBody,
312324
explainTier: opts.explainTier,
313325
forceTier: opts.forceTier,
326+
staleManifest,
314327
})
315328
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ export const EngineVersions = z.object({
8282
reviewer: z.string().default("dbt-pr-review/1"),
8383
core: z.string().optional(),
8484
model: z.string().optional(),
85+
/** The altimate-code CLI release that generated this verdict — lets an
86+
* auditor reconstruct which policy version applied months later, when
87+
* the verdict envelope has outlived the running binary. */
88+
cliVersion: z.string().optional(),
8589
})
8690
export type EngineVersions = z.infer<typeof EngineVersions>
8791

@@ -110,6 +114,12 @@ export const VerdictEnvelope = z.object({
110114
engine: EngineVersions,
111115
/** Hash of the dbt manifest the verdict was computed against, when present. */
112116
manifestHash: z.string().optional(),
117+
/** True when a change-affecting source file was modified after the manifest
118+
* was written (checked via mtime; see run.ts::detectStaleManifest). Durably
119+
* records in the signed envelope that the verdict may have been computed
120+
* against out-of-date metadata — a stderr warning alone is easy for CI to
121+
* swallow. */
122+
staleManifest: z.boolean().optional(),
113123
/** ISO timestamp; injected by the caller (no clock access in pure code). */
114124
generatedAt: z.string().optional(),
115125
/** Optional break-glass override record. */
@@ -166,6 +176,8 @@ export interface BuildEnvelopeInput {
166176
tierForced?: boolean
167177
/** G2 — classifier's original tier before the force override. */
168178
tierClassified?: RiskTier
179+
/** True when mtime signals the manifest predates a change-affecting file. */
180+
staleManifest?: boolean
169181
}
170182

171183
function summarize(findings: Finding[], degraded: boolean): VerdictEnvelope["summary"] {
@@ -193,6 +205,7 @@ export function buildEnvelope(input: BuildEnvelopeInput): VerdictEnvelope {
193205
summary: summarize(input.findings, degraded),
194206
engine: EngineVersions.parse(input.engine ?? {}),
195207
manifestHash: input.manifestHash,
208+
staleManifest: input.staleManifest ? true : undefined,
196209
generatedAt: input.generatedAt,
197210
})
198211
}

packages/opencode/src/cli/cmd/review.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { promises as fs } from "node:fs"
22
import { UI } from "../ui"
33
import { cmd } from "./cmd"
44
import { bootstrap } from "../bootstrap"
5+
import { Installation } from "../../installation"
56
import { reviewPullRequest } from "../../altimate/review/run"
67
import { renderSummary } from "../../altimate/review/format"
78
import { postGitHubReview, resolveGitHubTarget } from "../../altimate/review/post-github"
@@ -85,6 +86,10 @@ export const ReviewCommand = cmd({
8586
noAi: args.noAi === true || args.ai === false,
8687
explainTier: args.explainTier === true,
8788
forceTier: args.forceTier as "trivial" | "lite" | "full" | undefined,
89+
// Stamp the CLI version into engine.cliVersion so an auditor can
90+
// reconstruct which policy version generated a stored verdict long
91+
// after the binary that ran it is gone.
92+
cliVersion: Installation.VERSION,
8893
})
8994

9095
if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2))

0 commit comments

Comments
 (0)