review: --no-ai fix + --explain-tier / --force-tier flags + manifest auto-discovery + column-aware schema.yml test-removal - #1027
Merged
Conversation
added 2 commits
July 20, 2026 21:40
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.
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.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds structural dbt schema test-removal detection, manifest discovery and freshness checks, tier explanation and override controls, signed tier provenance, resolver path handling, and tier-aware output rendering. It also corrects snapshot YAML classification and expands regression coverage. Changesdbt schema test detection
Tier and review controls
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewCLI
participant reviewPullRequest
participant runReview
participant runReviewTiering
participant buildEnvelope
participant renderSummary
ReviewCLI->>reviewPullRequest: pass explain-tier and force-tier
reviewPullRequest->>runReview: forward review options
runReview->>runReviewTiering: classify review tier
runReviewTiering->>runReviewTiering: apply optional forced tier
runReview->>buildEnvelope: attach tier reasons and provenance
buildEnvelope->>renderSummary: provide verdict envelope
renderSummary-->>ReviewCLI: render tier and explanation
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opencode/src/altimate/review/run.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPrefer
FileSystem.FileSystemfor effectful file I/O.The new
node:fs/promisesimports bypass the project's Effect-driven I/O abstraction. As per coding guidelines, "PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O".Consider migrating these new operations (
stat,access) to the EffectFileSystemmodule where feasible, though sincewarnIfStaleandautoDiscoverManifestare currently standardasyncfunctions, this might require broader refactoring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/review/run.ts` at line 2, Replace the raw node:fs/promises stat and access usage in warnIfStale and autoDiscoverManifest with the project’s FileSystem.FileSystem abstraction, refactoring those async flows as needed to use Effect-based file I/O. Preserve the existing stale-file checks and manifest discovery behavior while removing the direct filesystem imports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/review/dbt-patterns.ts`:
- Around line 851-878: Update collectChangedFiles() and
collectTestOccurrencesFromDiff() so schema.yml diffs retain enough context to
identify the enclosing model when a changed column header is the first name
entry in the hunk. Prefer increasing unified diff context for schema files;
otherwise add fallback scope inference that distinguishes the column from the
missing model while preserving detection of removed unique, not_null, and
relationships tests.
In `@packages/opencode/src/altimate/review/run.ts`:
- Around line 122-124: Filter changedPaths before or within the loop so only
relevant dbt model files—schema.yml files or SQL files—reach the stale-manifest
comparison using abs. Exclude unrelated files such as README.md and package.json
while preserving the existing tracked, on-disk checks for eligible paths.
---
Nitpick comments:
In `@packages/opencode/src/altimate/review/run.ts`:
- Line 2: Replace the raw node:fs/promises stat and access usage in warnIfStale
and autoDiscoverManifest with the project’s FileSystem.FileSystem abstraction,
refactoring those async flows as needed to use Effect-based file I/O. Preserve
the existing stale-file checks and manifest discovery behavior while removing
the direct filesystem imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1242bef6-a381-4d45-a224-359ee38b5c18
📒 Files selected for processing (6)
packages/opencode/src/altimate/review/dbt-patterns.tspackages/opencode/src/altimate/review/format.tspackages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/run.tspackages/opencode/src/altimate/review/verdict.tspackages/opencode/src/cli/cmd/review.ts
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previous suggestion ( Files Reviewed (2 files)
Previous Review Summaries (5 snapshots, latest commit f218c3f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f218c3f)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit c034373)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous review (commit 310927e)Status: No Issues Found | Recommendation: Merge Incremental review (commit
|
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… minors) 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.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Contributor
Author
|
Thanks for the review. Follow-up commit Blocker no. 1 — column-aware detector regression + broken testFixed. Rewrote
Major no. 2 — model-level test removalsFixed as part of no. 1. Structural walker emits per-entity tests directly under Major no. 3 — auto-manifest projectRoot not threadedFixed. Added a Minor no. 4 — docstring overstates safety guaranteesFixed. Rewrote Minor no. 5 —
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/review/dbt-patterns.ts`:
- Around line 983-1003: Update the fallback deduplication in the !usedStructural
branch of the dbt pattern detection flow so distinct matched removal lines are
preserved even when model and column are empty. Key deduplication by each
matched line’s index or content, while still preventing duplicate processing of
the same fallback line and retaining the existing test-type extraction.
- Around line 954-981: The structural comparison around extractTestOccurrences
must only run when both old and new YAML documents are available; do not treat
undefined oldContent or an unparseable old document as an empty added-file side.
When old YAML is unavailable, leave usedStructural false so the caller executes
fallbackRemovedTestLines(file.diff), while preserving the added-file behavior
only when the old side is genuinely absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 682cedd5-0e3a-4488-9ff8-bb257c9a19d2
📒 Files selected for processing (6)
packages/opencode/src/altimate/review/dbt-patterns.tspackages/opencode/src/altimate/review/format.tspackages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/run.tspackages/opencode/test/altimate/review-dbt-patterns.test.tspackages/opencode/test/altimate/review.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/altimate/review/format.ts
- packages/opencode/src/altimate/review/orchestrate.ts
- packages/opencode/src/altimate/review/run.ts
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/review/dbt-patterns.ts">
<violation number="1" location="packages/opencode/src/altimate/review/dbt-patterns.ts:862">
P2: Snapshot YAML files under `snapshots/` never reach this extraction, so their removed tests remain unreported despite the new snapshots support. Classify snapshot YAML property files as `schema_yml` (or admit YAML `snapshot` files at this gate) before relying on this walker.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| } | ||
| // `snapshots:` — same shape as models | ||
| if (Array.isArray(d.snapshots)) { |
There was a problem hiding this comment.
P2: Snapshot YAML files under snapshots/ never reach this extraction, so their removed tests remain unreported despite the new snapshots support. Classify snapshot YAML property files as schema_yml (or admit YAML snapshot files at this gate) before relying on this walker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/dbt-patterns.ts, line 862:
<comment>Snapshot YAML files under `snapshots/` never reach this extraction, so their removed tests remain unreported despite the new snapshots support. Classify snapshot YAML property files as `schema_yml` (or admit YAML `snapshot` files at this gate) before relying on this walker.</comment>
<file context>
@@ -790,162 +791,294 @@ export function detectModelPatterns(file: ChangedFile, newSql: string | undefine
+ }
+ }
+ // `snapshots:` — same shape as models
+ if (Array.isArray(d.snapshots)) {
+ for (const s of d.snapshots) {
+ if (!s || typeof s !== "object") continue
</file context>
Contributor
Author
There was a problem hiding this comment.
Addressed in 6729f5374. classifyDbtFile in diff-filter.ts now splits by extension: snapshots/*.sql remains snapshot (tier-forcing / catalog rules unchanged), snapshots/*.yml classifies as schema_yml and reaches the test-removal detector. Assertions added in review.test.ts (snapshots/*.yml → schema_yml) and an end-to-end test at review-dbt-patterns.test.ts:417 (snapshot yml property file (structural): removed test surfaces finding).
dev-punia-altimate
left a comment
Contributor
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 5 finding(s)
- 5 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/review/dbt-patterns.ts (L994-L1002)
[🟠 MEDIUM] In fallback mode, model and column are empty strings, so the deduplication key is effectively just \x00\x00${test}. If multiple tests of the same type are removed, they will be collapsed into a single finding. This loses the total count of removed tests that the previous implementation provided (e.g., "removed 3 data test(s)"). Consider preserving the raw count of removed lines in fallback mode so the scale of test regressions is not understated to users.
2. packages/opencode/src/altimate/review/dbt-patterns.ts (L1055-L1061)
[🟠 MEDIUM] If a PR removes tests from multiple models, isFirstForModel will evaluate to true for the first finding of each distinct model. This means the exact same global summary ("This PR removes X data tests in total on model(s) A, B") will be redundantly appended to multiple findings in the review, causing comment clutter. Consider tracking a single boolean flag (e.g., let summaryPrinted = false) to ensure the global summary is only appended once to the very first finding generated for the file.
3. packages/opencode/src/altimate/review/dbt-patterns.ts (L958-L969)
[🔵 LOW] When YAML parsing fails, the exception is caught and silently ignored, which demotes the analysis to the string-diff fallback path. Consider adding a debug or telemetry log within the catch block. This would aid in diagnosing issues where valid-looking PRs fail structural parsing simply due to minor YAML syntax errors.
4. packages/opencode/src/altimate/review/run.ts (L126-L127)
[🟠 MEDIUM] Although the comment explicitly states an intention to only compare against tracked, on-disk model files (schema.yml or SQL), there is no actual file extension filtering applied to changedPaths. Because changedPaths includes all modified files in the diff, any modification to a non-dbt file (e.g., README.md or package.json) after the manifest was generated will trigger a false positive stale manifest warning.
You should filter the files before checking their mtimeMs.
Suggested change:
// Only compare against tracked, on-disk model files (schema.yml or SQL)
if (!/\.(sql|yml|yaml|py|csv)$/i.test(rel)) continue
const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel)
5. packages/opencode/src/altimate/review/orchestrate.ts (L1226-L1232)
[🔴 HIGH] 1. Logic bug with renamed files:
Using file.status === "modified" skips fetching oldContent for renamed files (where status is "renamed"). Since the detector relies on comparing oldContent with newContent, any guardrail tests removed during a file rename will be completely missed. You should use file.status !== "added" instead to ensure oldContent is fetched for both renamed and modified files.
2. Performance (Async in Loops):
Fetching file contents sequentially inside a for...of loop creates a performance bottleneck. As per the async handling checklist, independent async operations should be parallelized.
While the direct syntax fix for the status check is provided in the suggested replacement, consider refactoring the entire loop to use Promise.all concurrently:
const schemaFindings = await Promise.all(
reviewable
.filter((file) => file.kind === "schema_yml")
.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 })
})
)
all.push(...schemaFindings)Suggested change:
if (file.kind !== "schema_yml") continue
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),
])
all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent }))
Comment on lines
+994
to
+1002
| // 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 | ||
| }) |
Contributor
There was a problem hiding this comment.
[🟠 MEDIUM] In fallback mode, model and column are empty strings, so the deduplication key is effectively just \x00\x00${test}. If multiple tests of the same type are removed, they will be collapsed into a single finding. This loses the total count of removed tests that the previous implementation provided (e.g., "removed 3 data test(s)"). Consider preserving the raw count of removed lines in fallback mode so the scale of test regressions is not understated to users.
Contributor
Author
There was a problem hiding this comment.
Addressed in 6729f5374. Fallback path now emits one finding per removed test-line (dedup removed, occurrence-index discriminator added to ruleKey so global fingerprint dedupe doesn't collapse them). Aggregate "removes N tests" line preserved via file-scoped summaryEmitted — appended once to the first finding, listing distinct models when structural attribution is available (empty when it isn't). See #3619947164 for the exact fix + regression test.
Comment on lines
+1055
to
+1061
| 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(", ")}._` | ||
| : "" |
Contributor
There was a problem hiding this comment.
[🟠 MEDIUM] If a PR removes tests from multiple models, isFirstForModel will evaluate to true for the first finding of each distinct model. This means the exact same global summary ("This PR removes X data tests in total on model(s) A, B") will be redundantly appended to multiple findings in the review, causing comment clutter. Consider tracking a single boolean flag (e.g., let summaryPrinted = false) to ensure the global summary is only appended once to the very first finding generated for the file.
Contributor
Author
There was a problem hiding this comment.
Addressed in 6729f5374. Replaced the per-model isFirstForModel guard with a single file-scoped summaryEmitted boolean. The aggregate "This PR removes N data tests in total on model(s) …" line now appears exactly once per file — on the first finding — and the model list is computed from the whole removals array. New test at review-dbt-patterns.test.ts:454 asserts multi-model diffs produce ONE summary line, not one per model.
Comment on lines
+958
to
+969
| try { | ||
| newDoc = YAML.parse(opts.newContent) | ||
| } catch { | ||
| newDoc = undefined | ||
| } | ||
| if (opts.oldContent !== undefined) { | ||
| try { | ||
| oldDoc = YAML.parse(opts.oldContent) | ||
| } catch { | ||
| oldDoc = undefined | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[🔵 LOW] When YAML parsing fails, the exception is caught and silently ignored, which demotes the analysis to the string-diff fallback path. Consider adding a debug or telemetry log within the catch block. This would aid in diagnosing issues where valid-looking PRs fail structural parsing simply due to minor YAML syntax errors.
Contributor
Author
There was a problem hiding this comment.
Addressed in 6729f5374. Both YAML.parse failure branches now log via Log.create({ service: "review", tag: "detectSchemaYmlPatterns" }).warn(…) including the file path and error message, then fall through to the diff-based fallback (not fail-open). Diagnosing a valid-looking PR that fails structural parsing is now grep-friendly in the review logs.
Comment on lines
+126
to
+127
| // Only compare against tracked, on-disk model files (schema.yml or SQL) | ||
| const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel) |
Contributor
There was a problem hiding this comment.
[🟠 MEDIUM] Although the comment explicitly states an intention to only compare against tracked, on-disk model files (schema.yml or SQL), there is no actual file extension filtering applied to changedPaths. Because changedPaths includes all modified files in the diff, any modification to a non-dbt file (e.g., README.md or package.json) after the manifest was generated will trigger a false positive stale manifest warning.
You should filter the files before checking their mtimeMs.
Suggested change:
Suggested change
| // Only compare against tracked, on-disk model files (schema.yml or SQL) | |
| const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel) | |
| // Only compare against tracked, on-disk model files (schema.yml or SQL) | |
| if (!/\.(sql|yml|yaml|py|csv)$/i.test(rel)) continue | |
| const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel) |
Contributor
Author
There was a problem hiding this comment.
Addressed in 6729f5374. See #3619684688 — introduced isManifestAffecting(rel) filter in run.ts, admits only dbt-relevant extensions under dbt source directories (plus root config files). Table-driven positive/negative tests in the new review-run-stale.test.ts file cover README.md, models/foo.sql, docs/foo.md, models/foo.md, seeds/lookup.csv, snapshots/.sql, snapshots/.yml, dbt_project.yml, .github/workflows/, target/…
Comment on lines
+1226
to
+1232
| 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 })) |
Contributor
There was a problem hiding this comment.
[🔴 HIGH] 1. Logic bug with renamed files:
Using file.status === "modified" skips fetching oldContent for renamed files (where status is "renamed"). Since the detector relies on comparing oldContent with newContent, any guardrail tests removed during a file rename will be completely missed. You should use file.status !== "added" instead to ensure oldContent is fetched for both renamed and modified files.
2. Performance (Async in Loops):
Fetching file contents sequentially inside a for...of loop creates a performance bottleneck. As per the async handling checklist, independent async operations should be parallelized.
While the direct syntax fix for the status check is provided in the suggested replacement, consider refactoring the entire loop to use Promise.all concurrently:
const schemaFindings = await Promise.all(
reviewable
.filter((file) => file.kind === "schema_yml")
.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 })
})
)
all.push(...schemaFindings)Suggested change:
Suggested change
| 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 })) | |
| if (file.kind !== "schema_yml") continue | |
| 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), | |
| ]) | |
| all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })) |
Contributor
Author
There was a problem hiding this comment.
Addressed in 6729f5374. Both parts of your comment: (1) status gate changed to file.status !== "added" (covers renamed files); (2) loop refactored to Promise.all across schema files (concurrent) with per-file inner Promise.all for old/new content. Uses roughly the exact shape you suggested. See #3619993834 (rename regression test) and #3619993862 (perf note).
sahrizvi
marked this pull request as draft
July 21, 2026 09:57
…ame bypass, snapshot yml, +5 more) 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.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Contributor
Author
Bot-review round-2 addressed — pushed as
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
dev-punia-altimate
left a comment
Contributor
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 3 finding(s)
- 3 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/review/orchestrate.ts (L1233-L1241)
[🟠 MEDIUM] The change file.status !== "added" to capture renames also inadvertently includes deleted files. This causes an unnecessary fetch of oldContent (which could be an expensive operation like git show) for deleted files.
Since detectSchemaYmlPatterns immediately returns [] when file.status === "deleted", we can optimize this by filtering out deleted files upfront. This also simplifies the newContent condition.
Suggested change:
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),
getContent?.(file.path, "new"),
])
2. packages/opencode/src/altimate/review/dbt-patterns.ts (L1100-L1108)
[🟠 MEDIUM] The distinctModels and modelClause variables are recomputed on every iteration of the loop, but they are only used when shouldEmitSummary is true (which evaluates to true only once per file). This causes unnecessary object allocations and redundant O(N) operations in every loop iteration.
Consider moving this computation inside an if (shouldEmitSummary) block, or precomputing the string once before the loop.
Suggested change:
const shouldEmitSummary = !summaryEmitted && removals.length > 1
if (shouldEmitSummary) summaryEmitted = true
let bodyTail = ""
if (shouldEmitSummary) {
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}._`
}
3. packages/opencode/src/altimate/review/dbt-patterns.ts (L1116)
[🔵 LOW] Calling shift() on an array inside a loop yields O(N^2) time complexity because shift() forces a re-indexing of all remaining elements in the array. While the array length here might be small, it is a good practice to avoid array mutation inside loops when a simple index access suffices.
Consider defining an index variable (e.g. let fallbackIdx = 0;) before the for (const r of removals) loop, and using it here instead.
Suggested change:
const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators[fallbackIdx++] ?? "#?"}`
Comment on lines
+1233
to
+1241
| 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), | ||
| ]) |
Contributor
There was a problem hiding this comment.
[🟠 MEDIUM] The change file.status !== "added" to capture renames also inadvertently includes deleted files. This causes an unnecessary fetch of oldContent (which could be an expensive operation like git show) for deleted files.
Since detectSchemaYmlPatterns immediately returns [] when file.status === "deleted", we can optimize this by filtering out deleted files upfront. This also simplifies the newContent condition.
Suggested change:
Suggested change
| 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), | |
| ]) | |
| 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), | |
| getContent?.(file.path, "new"), | |
| ]) |
Contributor
Author
There was a problem hiding this comment.
Addressed in 39718d5d1. schemaFiles now filters out status === "deleted" upfront in orchestrate.ts — deleted schema.yml files never enter the fetch loop, so the redundant git show round-trip is gone. newContent fetch simplifies to unconditional getContent?.(file.path, "new") since deleted files can't reach it anymore. Used the exact shape from your suggestion.
Comment on lines
+1100
to
+1108
| 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}._` | ||
| : "" |
Contributor
There was a problem hiding this comment.
[🟠 MEDIUM] The distinctModels and modelClause variables are recomputed on every iteration of the loop, but they are only used when shouldEmitSummary is true (which evaluates to true only once per file). This causes unnecessary object allocations and redundant O(N) operations in every loop iteration.
Consider moving this computation inside an if (shouldEmitSummary) block, or precomputing the string once before the loop.
Suggested change:
Suggested change
| 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}._` | |
| : "" | |
| const shouldEmitSummary = !summaryEmitted && removals.length > 1 | |
| if (shouldEmitSummary) summaryEmitted = true | |
| let bodyTail = "" | |
| if (shouldEmitSummary) { | |
| 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}._` | |
| } |
Contributor
Author
There was a problem hiding this comment.
Addressed in 39718d5d1. Moved the distinctModels / modelClause computation inside the if (shouldEmitSummary) block — no Set/spread/filter/map/join on iterations where the summary isn't attached (all but the first). summaryEmitted flip also moved inside the block.
| // 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() ?? "#?"}` |
Contributor
There was a problem hiding this comment.
[🔵 LOW] Calling shift() on an array inside a loop yields O(N^2) time complexity because shift() forces a re-indexing of all remaining elements in the array. While the array length here might be small, it is a good practice to avoid array mutation inside loops when a simple index access suffices.
Consider defining an index variable (e.g. let fallbackIdx = 0;) before the for (const r of removals) loop, and using it here instead.
Suggested change:
Suggested change
| const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators.shift() ?? "#?"}` | |
| const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators[fallbackIdx++] ?? "#?"}` |
Contributor
Author
There was a problem hiding this comment.
Addressed in 39718d5d1. Hoisted let fallbackIdx = 0 before the for (const r of removals) loop and swapped fallbackDiscriminators.shift() for fallbackDiscriminators[fallbackIdx++]. Discriminator array is no longer mutated inside the loop; index-based access is O(1) per iteration.
Five items from coderabbit / cubic-dev-ai / kilo-code-bot on round 5: - Symlink-escape containment (coderabbit + cubic): `makeContentResolver` working-tree read and `makeCompiledResolver` compiled-SQL read now go through a `realpath` containment check that rejects any target whose resolved path falls outside the resolved root. A tracked symlink like `models/evil.sql → /etc/passwd` no longer leaks external content into the review pipeline. Applied at both the diff-side (git.ts) and compiled-side (compiled.ts) resolvers. - Symlink-consistent path prefix (cubic + kilo): `path.relative(gitRoot, dbtRoot)` produced a bogus climb path (`../../../var/...`) whenever `dbtRoot` traversed an unresolved symlink (macOS `/var` → `/private/var` is the canonical case). The mapped prefix never matched incoming repo-relative paths, so compiled SQL was silently missed on the exact monorepo layouts the subdir fix was meant to enable. Both roots are now realpath-resolved before `path.relative`, and `dbtRootReal` is handed to `makeCompiledResolver` as its `cwd`. Falls back gracefully when realpath fails (path deleted mid-run). - `gitRepoRoot` newline handling (cubic P3): `.trim()` also stripped legitimate leading/trailing path whitespace. Replaced with an explicit `\r?\n` terminator strip that leaves everything else intact. - dbt version comment (cubic P3): the `arguments:` nesting syntax is documented from dbt v1.10.5, not 1.9+. Comment updated. New regression tests in `review-subdir-invocation.test.ts`: - makeContentResolver refuses to read a symlink escaping the git root - makeCompiledResolver refuses to read a compiled symlink escaping the compiled root - gitRepoRoot preserves legitimate path characters (no trim() regression) Full altimate review suite: 3790 pass / 640 skip / 0 fail (was 3787 baseline — 3 new tests, +8 expect calls). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const rootReal = await fs.realpath(compiledRoot) | ||
| const targetReal = await fs.realpath(path.join(compiledRoot, rel)) | ||
| const sep = path.sep | ||
| if (targetReal !== rootReal && !targetReal.startsWith(rootReal + sep)) return undefined |
There was a problem hiding this comment.
SUGGESTION: This realpath + containment block duplicates safeReadInside in git.ts:70 (identical logic: resolve root + target, separator-aware startsWith check, then read the realpath'd target).
compiled.ts doesn't import git.ts today, and git.ts doesn't import compiled.ts (no cycle), so exporting safeReadInside and replacing lines 87-91 with return await safeReadInside(compiledRoot, rel) removes the copy. Keeping two copies of security-sensitive containment logic is risky — a fix to one (e.g. a Windows case-sensitivity or trailing-separator edge) won't propagate to the other.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Contributor
Author
There was a problem hiding this comment.
Addressed in ab461c450 (same fix as cubic's P3 on this pair). safeReadInside is now exported from git.ts and used by both makeContentResolver and makeCompiledResolver. No behavior change; 3790 pass / 0 fail.
…compiled.ts cubic + kilo bot review — the realpath containment check was duplicated across `makeContentResolver` (git.ts) and `makeCompiledResolver` (compiled.ts). Both bots flagged that keeping two copies of security- sensitive logic risks a future fix (e.g. Windows case-sensitivity, trailing-separator edge) landing in one call site but not the other. - Export `safeReadInside(root, rel)` from git.ts as the single realpath-checked reader. - `compiled.ts` imports it and replaces its inline containment block with `return await safeReadInside(compiledRoot, rel)`. - No behavior change; regression suite still 3790 pass / 0 fail. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
saravmajestic
previously approved these changes
Jul 23, 2026
| * 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. */ |
Contributor
There was a problem hiding this comment.
issue: This JSDoc block describes warnIfStale (the function that comes after isManifestAffecting), but it's physically placed before isManifestAffecting. TypeScript tooling and IDEs will attribute both JSDoc blocks to isManifestAffecting; warnIfStale will show no doc on hover.
Move this block to immediately precede async function warnIfStale.
Contributor
Author
There was a problem hiding this comment.
Fixed in 69553ed9d — the JSDoc block that described warnIfStale was placed above isManifestAffecting, so TS/IDE tooling attributed both blocks to the earlier symbol. Split into two self-contained blocks: isManifestAffecting gets its own docstring (concerns + why it's exported), and warnIfStale gets a fresh block immediately before its definition.
| // Top-level dbt project config files. | ||
| if (/(^|\/)(dbt_project|packages|profiles|dependencies)\.ya?ml$/i.test(rel)) return true | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
nit: The regex admits .md files under seeds/, macros/, tests/, and analyses/ directories. dbt docs blocks live under models/ (and sometimes analyses/); a macros/README.md or tests/README.md is unlikely to be manifest-affecting but would still trigger a stale warning.
Consider narrowing the .md match to models|analyses, or add a comment documenting that the wider match is a deliberate conservative over-include.
Contributor
Author
There was a problem hiding this comment.
Fixed in 69553ed9d — narrowed the .md match to models|analyses and split the check: other extensions (.sql, .py, .yml, .yaml, .csv) still admitted across all six source dirs, .md only under the two doc-block-canonical dirs. Rejected-path test list gains macros/README.md, tests/README.md, seeds/README.md, snapshots/README.md; admit list gains analyses/gross_margin.md.
Follow-up: cubic-dev-ai flagged the narrow as too tight (dbt's docs-paths config can extend where docs blocks live). See my reply on that thread — kept the narrow since docs-paths extension is project-specific config, not default behavior.
| } | ||
| } catch { | ||
| /* keep walking */ | ||
| } |
Contributor
There was a problem hiding this comment.
thought: When dbt_project.yml exists but target/manifest.json doesn't, we stop walking up and return undefined — even if an ancestor directory has a compiled project.
This is likely the right call ("found a project, it just hasn't been compiled — don't silently pick up a grandparent"), but it's worth a short comment here so future readers don't wonder if the early return is intentional.
Contributor
Author
There was a problem hiding this comment.
Fixed in 69553ed9d — added a comment at the early-return site explaining the deliberate choice: when a dbt project is found but not compiled, we return undefined rather than walk further, because a grandparent's target/manifest.json belongs to a different project's DAG. Silent grandparent-fallback would surprise the caller.
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
For every column named in a `dbt_utils.unique_combination_of_columns`
test's `combination_of_columns`, require `not_null` coverage on the
same model. Otherwise a NULL grain-key value silently passes the
uniqueness test — the guardrail is toothless. Cited as a hard rule in
`docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't.
Coverage sources:
- `constraints: [{type: not_null}]` — only counted when
`contract.enforced == true` (per codex R20 S1 high #3). On views /
non-contracted models, `constraints:` is documentation-only, not
enforced, so we don't miss a real gap.
- Column-level `data_tests: [not_null]` / `tests: [not_null]` — always
counted; dbt's test runner enforces regardless of contract state.
Recommendation flips between `constraints:` (contracted model) and
`data_tests:` (view / non-contracted) based on the model's contract
state — matches the adapter-semantics discussion in the corpus study
(PR D×2 + PR A×2).
Supported YAML shapes:
- `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns`
(exact match per codex high #1; `endsWith` would over-match).
- pre-1.9 flat args + dbt 1.9+ `arguments:` nesting.
- top-level `contract:` and nested `config.contract:` for enforcement flag.
False-positive guards:
- Column-name comparison is case-folded (Snowflake identifiers, per
codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches
`workspace_id` in `columns:`.
- Skipped on `status === "deleted"` files (no current grain to guard).
- Only runs on files in the PR diff, not repo-wide.
### Validation on 5-PR internal corpus (recall improvement)
vs S4 baseline (the tier-promotion PR this branch stacks on):
| Variant | S4 baseline | S1 result | Delta |
|---|---:|---:|---:|
| A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** |
| A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) |
Grain-key gaps caught:
- PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type`
- PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1
- PR A, D, E: 0 (fixes already landed in HEAD state per corpus study)
Strong matches to human blocker findings on PR C:
- `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`,
`int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical:
"workspace_id missing from carrier identity")
- `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time`
→ PR C human F10 (critical: "dbt mart grain includes period_end_time")
### Tests
- 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests).
- 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files).
- Codex-reviewed diff, 3 highs addressed:
- endsWith → exact-name match
- column-name case-folding
- constraints only count when contract enforced
- Codex minor #4 addressed (test fixture for top-level `contract:`)
- Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage)
Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn
stacks on PR #1027 (feat/review-r18-observability-recall).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
Three findings from the harness-bot pass on PR #1027: 1. `run.ts:114` — add rationale for the fixed-point walk's early return. When `dbt_project.yml` exists but `target/manifest.json` doesn't, we intentionally return `undefined` rather than continue walking to a grandparent's compiled project — a grandparent's DAG is a different project's DAG. Comment makes this clear so future maintainers don't "fix" it into a silent grandparent-fallback. 2. `run.ts:129` — narrow the `.md` match in `isManifestAffecting`. dbt `{% docs %}` blocks are canonically parsed under `models/` and `analyses/`. Under `macros/`, `seeds/`, `snapshots/`, and `tests/`, `.md` files are package documentation (READMEs), not manifest input. Split the check so `.md` requires `models|analyses`; other extensions (`.sql`, `.py`, `.yml`, `.yaml`, `.csv`) still admitted under all six source directories. Added `README.md` under each of the four newly-excluded directories to the rejected-path test list. 3. `run.ts:120-124` — JSDoc block that describes `warnIfStale` was physically placed above `isManifestAffecting`, so TS/IDE tooling would attribute both blocks to the earlier symbol and `warnIfStale` would show no hover doc. Split into two blocks: a self-contained docstring for `isManifestAffecting` (its own concerns + why it's exported) and a fresh docstring for `warnIfStale` immediately preceding its definition. Tests: 210/210 green (7 review-* files); review-run-stale.test.ts gains `analyses/gross_margin.md` (admitted) plus four `.md`-under- non-docs-dirs entries in the rejected list.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/review/run.ts">
<violation number="1" location="packages/opencode/src/altimate/review/run.ts:141">
P2: Stale manifests will not be reported after documentation-block changes under `macros`, `tests`, `seeds`, or `snapshots`. dbt searches all resource paths for docs blocks by default, so retain those directories in the Markdown check.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // `macros/README.md` / `tests/README.md` / `seeds/README.md` is package | ||
| // documentation, not manifest input (altimate-harness-bot review, | ||
| // PR #1027 run.ts:133). | ||
| if (/(^|\/)(models|analyses)\/.*\.md$/i.test(rel)) return true |
There was a problem hiding this comment.
P2: Stale manifests will not be reported after documentation-block changes under macros, tests, seeds, or snapshots. dbt searches all resource paths for docs blocks by default, so retain those directories in the Markdown check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/run.ts, line 141:
<comment>Stale manifests will not be reported after documentation-block changes under `macros`, `tests`, `seeds`, or `snapshots`. dbt searches all resource paths for docs blocks by default, so retain those directories in the Markdown check.</comment>
<file context>
@@ -117,21 +124,31 @@ async function autoDiscoverManifest(cwd: string): Promise<{ path: string; projec
+ // `macros/README.md` / `tests/README.md` / `seeds/README.md` is package
+ // documentation, not manifest input (altimate-harness-bot review,
+ // PR #1027 run.ts:133).
+ if (/(^|\/)(models|analyses)\/.*\.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
</file context>
Contributor
Author
There was a problem hiding this comment.
Design call — kept the narrow models|analyses scope going with the harness-bot's take. Reasoning: dbt docs blocks ({% docs %}) are canonically parsed under models/ and analyses/; docs-paths can be extended in dbt_project.yml but that's project-specific config, not the default. Silent-failure mode you flag is real but bounded — a stale-manifest warning is non-fatal observability that fires again on the next mtime-checked path, so a missed warning on a custom docs-paths config would just delay the warning to the first SQL/YAML edit in the same PR. Kept the narrow match; happy to widen if this shows up in practice.
saravmajestic
approved these changes
Jul 23, 2026
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
For every column named in a `dbt_utils.unique_combination_of_columns`
test's `combination_of_columns`, require `not_null` coverage on the
same model. Otherwise a NULL grain-key value silently passes the
uniqueness test — the guardrail is toothless. Cited as a hard rule in
`docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't.
Coverage sources:
- `constraints: [{type: not_null}]` — only counted when
`contract.enforced == true` (per codex R20 S1 high #3). On views /
non-contracted models, `constraints:` is documentation-only, not
enforced, so we don't miss a real gap.
- Column-level `data_tests: [not_null]` / `tests: [not_null]` — always
counted; dbt's test runner enforces regardless of contract state.
Recommendation flips between `constraints:` (contracted model) and
`data_tests:` (view / non-contracted) based on the model's contract
state — matches the adapter-semantics discussion in the corpus study
(PR D×2 + PR A×2).
Supported YAML shapes:
- `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns`
(exact match per codex high #1; `endsWith` would over-match).
- pre-1.9 flat args + dbt 1.9+ `arguments:` nesting.
- top-level `contract:` and nested `config.contract:` for enforcement flag.
False-positive guards:
- Column-name comparison is case-folded (Snowflake identifiers, per
codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches
`workspace_id` in `columns:`.
- Skipped on `status === "deleted"` files (no current grain to guard).
- Only runs on files in the PR diff, not repo-wide.
### Validation on 5-PR internal corpus (recall improvement)
vs S4 baseline (the tier-promotion PR this branch stacks on):
| Variant | S4 baseline | S1 result | Delta |
|---|---:|---:|---:|
| A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** |
| A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) |
Grain-key gaps caught:
- PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type`
- PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1
- PR A, D, E: 0 (fixes already landed in HEAD state per corpus study)
Strong matches to human blocker findings on PR C:
- `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`,
`int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical:
"workspace_id missing from carrier identity")
- `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time`
→ PR C human F10 (critical: "dbt mart grain includes period_end_time")
### Tests
- 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests).
- 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files).
- Codex-reviewed diff, 3 highs addressed:
- endsWith → exact-name match
- column-name case-folding
- constraints only count when contract enforced
- Codex minor #4 addressed (test fixture for top-level `contract:`)
- Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage)
Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn
stacks on PR #1027 (feat/review-r18-observability-recall).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
For every column named in a `dbt_utils.unique_combination_of_columns`
test's `combination_of_columns`, require `not_null` coverage on the
same model. Otherwise a NULL grain-key value silently passes the
uniqueness test — the guardrail is toothless. Cited as a hard rule in
`docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't.
Coverage sources:
- `constraints: [{type: not_null}]` — only counted when
`contract.enforced == true` (per codex R20 S1 high #3). On views /
non-contracted models, `constraints:` is documentation-only, not
enforced, so we don't miss a real gap.
- Column-level `data_tests: [not_null]` / `tests: [not_null]` — always
counted; dbt's test runner enforces regardless of contract state.
Recommendation flips between `constraints:` (contracted model) and
`data_tests:` (view / non-contracted) based on the model's contract
state — matches the adapter-semantics discussion in the corpus study
(PR D×2 + PR A×2).
Supported YAML shapes:
- `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns`
(exact match per codex high #1; `endsWith` would over-match).
- pre-1.9 flat args + dbt 1.9+ `arguments:` nesting.
- top-level `contract:` and nested `config.contract:` for enforcement flag.
False-positive guards:
- Column-name comparison is case-folded (Snowflake identifiers, per
codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches
`workspace_id` in `columns:`.
- Skipped on `status === "deleted"` files (no current grain to guard).
- Only runs on files in the PR diff, not repo-wide.
### Validation on 5-PR internal corpus (recall improvement)
vs S4 baseline (the tier-promotion PR this branch stacks on):
| Variant | S4 baseline | S1 result | Delta |
|---|---:|---:|---:|
| A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** |
| A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) |
Grain-key gaps caught:
- PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type`
- PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1
- PR A, D, E: 0 (fixes already landed in HEAD state per corpus study)
Strong matches to human blocker findings on PR C:
- `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`,
`int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical:
"workspace_id missing from carrier identity")
- `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time`
→ PR C human F10 (critical: "dbt mart grain includes period_end_time")
### Tests
- 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests).
- 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files).
- Codex-reviewed diff, 3 highs addressed:
- endsWith → exact-name match
- column-name case-folding
- constraints only count when contract enforced
- Codex minor #4 addressed (test fixture for top-level `contract:`)
- Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage)
Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn
stacks on PR #1027 (feat/review-r18-observability-recall).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
For every column named in a `dbt_utils.unique_combination_of_columns`
test's `combination_of_columns`, require `not_null` coverage on the
same model. Otherwise a NULL grain-key value silently passes the
uniqueness test — the guardrail is toothless. Cited as a hard rule in
`docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't.
Coverage sources:
- `constraints: [{type: not_null}]` — only counted when
`contract.enforced == true` (per codex R20 S1 high #3). On views /
non-contracted models, `constraints:` is documentation-only, not
enforced, so we don't miss a real gap.
- Column-level `data_tests: [not_null]` / `tests: [not_null]` — always
counted; dbt's test runner enforces regardless of contract state.
Recommendation flips between `constraints:` (contracted model) and
`data_tests:` (view / non-contracted) based on the model's contract
state — matches the adapter-semantics discussion in the corpus study
(PR D×2 + PR A×2).
Supported YAML shapes:
- `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns`
(exact match per codex high #1; `endsWith` would over-match).
- pre-1.9 flat args + dbt 1.9+ `arguments:` nesting.
- top-level `contract:` and nested `config.contract:` for enforcement flag.
False-positive guards:
- Column-name comparison is case-folded (Snowflake identifiers, per
codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches
`workspace_id` in `columns:`.
- Skipped on `status === "deleted"` files (no current grain to guard).
- Only runs on files in the PR diff, not repo-wide.
### Validation on 5-PR internal corpus (recall improvement)
vs S4 baseline (the tier-promotion PR this branch stacks on):
| Variant | S4 baseline | S1 result | Delta |
|---|---:|---:|---:|
| A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** |
| A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) |
Grain-key gaps caught:
- PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type`
- PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1
- PR A, D, E: 0 (fixes already landed in HEAD state per corpus study)
Strong matches to human blocker findings on PR C:
- `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`,
`int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical:
"workspace_id missing from carrier identity")
- `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time`
→ PR C human F10 (critical: "dbt mart grain includes period_end_time")
### Tests
- 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests).
- 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files).
- Codex-reviewed diff, 3 highs addressed:
- endsWith → exact-name match
- column-name case-folding
- constraints only count when contract enforced
- Codex minor #4 addressed (test fixture for top-level `contract:`)
- Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage)
Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn
stacks on PR #1027 (feat/review-r18-observability-recall).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
For every column named in a `dbt_utils.unique_combination_of_columns`
test's `combination_of_columns`, require `not_null` coverage on the
same model. Otherwise a NULL grain-key value silently passes the
uniqueness test — the guardrail is toothless. Cited as a hard rule in
`docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't.
Coverage sources:
- `constraints: [{type: not_null}]` — only counted when
`contract.enforced == true` (per codex R20 S1 high #3). On views /
non-contracted models, `constraints:` is documentation-only, not
enforced, so we don't miss a real gap.
- Column-level `data_tests: [not_null]` / `tests: [not_null]` — always
counted; dbt's test runner enforces regardless of contract state.
Recommendation flips between `constraints:` (contracted model) and
`data_tests:` (view / non-contracted) based on the model's contract
state — matches the adapter-semantics discussion in the corpus study
(PR D×2 + PR A×2).
Supported YAML shapes:
- `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns`
(exact match per codex high #1; `endsWith` would over-match).
- pre-1.9 flat args + dbt 1.9+ `arguments:` nesting.
- top-level `contract:` and nested `config.contract:` for enforcement flag.
False-positive guards:
- Column-name comparison is case-folded (Snowflake identifiers, per
codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches
`workspace_id` in `columns:`.
- Skipped on `status === "deleted"` files (no current grain to guard).
- Only runs on files in the PR diff, not repo-wide.
### Validation on 5-PR internal corpus (recall improvement)
vs S4 baseline (the tier-promotion PR this branch stacks on):
| Variant | S4 baseline | S1 result | Delta |
|---|---:|---:|---:|
| A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** |
| A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) |
Grain-key gaps caught:
- PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type`
- PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1
- PR A, D, E: 0 (fixes already landed in HEAD state per corpus study)
Strong matches to human blocker findings on PR C:
- `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`,
`int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical:
"workspace_id missing from carrier identity")
- `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time`
→ PR C human F10 (critical: "dbt mart grain includes period_end_time")
### Tests
- 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests).
- 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files).
- Codex-reviewed diff, 3 highs addressed:
- endsWith → exact-name match
- column-name case-folding
- constraints only count when contract enforced
- Codex minor #4 addressed (test fixture for top-level `contract:`)
- Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage)
Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn
stacks on PR #1027 (feat/review-r18-observability-recall).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
…adata Grounded in the 5-PR internal corpus study (data-engineering-skills/docs/pr-review-corpus-findings-r20.md) where historical altimate-code recall was 2/84 = 2.4%. Two of five PRs auto-approved despite each having 8+ substantive human findings: - PR D: test-only YAML under `models/marts/` adding `data_tests:` / `constraints:` on a contracted mart → 8 misses, including 5 dbt-trino adapter-semantic bugs. - PR E: cost-anchor redesign under `mrt_jobs_cost_savings.sql` → 11 misses, including 3 critical FinOps corrections. Adds three FileChangeClass signals + wires them into `fullTierReasons()`: - **`dbtRiskYmlChanges`** — schema.yml diff introduces or edits `data_tests:`, `constraints:`, `contract:`, or a `unique_combination_of_columns` list-item. Regex anchors to YAML key position after optional diff marker, optional indent, and optional list marker; explicitly excludes comment lines. This catches PR D. - **`martLayerChange`** — file lives under `models/marts/` or `models/mart/`. Does NOT promote on its own (description-only edits stay trivial, matching the existing test), but ENRICHES the `dbtRiskYmlChanges` reason string so the customer sees "under models/marts/ (mart-API surface)". - **`finopsPathToken`** — path/filename contains a FinOps keyword (`cost|saving|billing|credit|dbu|spend|revenue|price|rate|pricing| invoice`) at a word / segment / extension boundary. Catches PR E. Word-boundary regex prevents false positives on incidental substrings (`broadcaster` ≠ `caste`, `precast` ≠ `cast`). Regression tests (11 new, all green): - PR D shapes (data_tests, constraints, unique_combination_of_columns) all promote to full with `mart-API surface` context. - PR E shape (mrt_jobs_cost_savings.sql) + 7 other FinOps keyword variants all promote to full. - FinOps false-positive guard: `broadcaster` / `precast` stay lite. - Description-only edits under models/marts/ still trivial (pre-existing behavior preserved). - Comment lines and description strings mentioning `data_tests:` / `constraints:` do NOT promote (regex tightness). - Nested-indent YAML key position (production shape) still fires. Codex-reviewed diff. Two highs addressed: - Regex tightening to avoid comment / description-string false positives - FileChangeClass consumer audit (no external constructions; safe) Ship criteria (from plan v2): PRs D and E from the corpus must no longer auto-approve. Baseline recall on the existing 13-scenario corpus must not regress. Both hold: 96/96 tests pass (85 pre-existing + 11 new); full altimate review suite 3781/3781 green. Depends on PR #1027 (feat/review-r18-observability-recall) — this branch stacks on top so tierReasons[] wiring is in place. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
sahrizvi
added a commit
that referenced
this pull request
Jul 23, 2026
…ring dbt metadata (#1028) * feat(review): [R20 S4] triage-tier promotion for risk-bearing dbt metadata Grounded in the 5-PR internal corpus study (data-engineering-skills/docs/pr-review-corpus-findings-r20.md) where historical altimate-code recall was 2/84 = 2.4%. Two of five PRs auto-approved despite each having 8+ substantive human findings: - PR D: test-only YAML under `models/marts/` adding `data_tests:` / `constraints:` on a contracted mart → 8 misses, including 5 dbt-trino adapter-semantic bugs. - PR E: cost-anchor redesign under `mrt_jobs_cost_savings.sql` → 11 misses, including 3 critical FinOps corrections. Adds three FileChangeClass signals + wires them into `fullTierReasons()`: - **`dbtRiskYmlChanges`** — schema.yml diff introduces or edits `data_tests:`, `constraints:`, `contract:`, or a `unique_combination_of_columns` list-item. Regex anchors to YAML key position after optional diff marker, optional indent, and optional list marker; explicitly excludes comment lines. This catches PR D. - **`martLayerChange`** — file lives under `models/marts/` or `models/mart/`. Does NOT promote on its own (description-only edits stay trivial, matching the existing test), but ENRICHES the `dbtRiskYmlChanges` reason string so the customer sees "under models/marts/ (mart-API surface)". - **`finopsPathToken`** — path/filename contains a FinOps keyword (`cost|saving|billing|credit|dbu|spend|revenue|price|rate|pricing| invoice`) at a word / segment / extension boundary. Catches PR E. Word-boundary regex prevents false positives on incidental substrings (`broadcaster` ≠ `caste`, `precast` ≠ `cast`). Regression tests (11 new, all green): - PR D shapes (data_tests, constraints, unique_combination_of_columns) all promote to full with `mart-API surface` context. - PR E shape (mrt_jobs_cost_savings.sql) + 7 other FinOps keyword variants all promote to full. - FinOps false-positive guard: `broadcaster` / `precast` stay lite. - Description-only edits under models/marts/ still trivial (pre-existing behavior preserved). - Comment lines and description strings mentioning `data_tests:` / `constraints:` do NOT promote (regex tightness). - Nested-indent YAML key position (production shape) still fires. Codex-reviewed diff. Two highs addressed: - Regex tightening to avoid comment / description-string false positives - FileChangeClass consumer audit (no external constructions; safe) Ship criteria (from plan v2): PRs D and E from the corpus must no longer auto-approve. Baseline recall on the existing 13-scenario corpus must not regress. Both hold: 96/96 tests pass (85 pre-existing + 11 new); full altimate review suite 3781/3781 green. Depends on PR #1027 (feat/review-r18-observability-recall) — this branch stacks on top so tierReasons[] wiring is in place. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of * fix(review): [R20 S4] address consensus-review findings on triage promotion Bundle addresses PR #1028's consensus review: - MAJOR #1 — legacy `tests:` (pre-dbt-1.8 alias) added to risk-key patterns so `models/marts/*.yml` diffs adding `tests: [not_null]` or similar promote out of trivial. Regression tests cover both marts and non-marts placement + a word-boundary FP guard. - MAJOR #3 — vacuous `unique_combination_of_columns` regression test fixed. Original path (`mrt_billing_account_prices.yml`) also matched FinOps + marts signals so the DBT_UNIQUE_COMBO_RE could have been deleted and the test still passed. Path moved to `models/intermediate/int_grain.yml` (non-mart, non-FinOps) and reason string asserted to confirm the combo regex is the sole triggering signal. - MINOR #2 — dropped `dbus` from FINOPS_TOKEN_RE (D-Bus IPC collision with `stg_dbus_connector.sql`). Singular `dbu` is sufficient. - MINOR #4 — each risk-YAML key now has its own regex in a DBT_RISK_KEY_PATTERNS table. `dbtRiskYmlKeyMatches()` returns the specific keys that matched; new `dbtRiskYmlKeys: string[]` field on FileChangeClass. Reason string now names the exact triggering keys (e.g. "schema.yml diff touches tests under models/marts/") rather than the concatenated umbrella. - MINOR #5 — reworded the "upgrades the WEIGHT" comment. There is no weighting/ordering effect; `martLayerChange` only enriches the reason string with the mart-API-surface context. - MINOR #6 — block-scalar body FP: `stripBlockScalars()` walks the diff tracking block-scalar state so a description or long-form comment containing `data_tests:` (or similar) doesn't spuriously promote. Codex round-6 review HIGH fixed too — earlier version worked only on the +/- slice, missing the common case where `description: |` is in the context and a changed line is inside its body. Now walks the full diff (context + changed) for state and only masks output on changed lines. - MINOR #7 — FinOps boundary class extended with `\d` so digit-suffixed paths (`mrt_cost2024.sql`, `stg_billing2.sql`, `dbu1_usage.sql`) match. - NIT #8 — DBT_UNIQUE_COMBO_RE relaxed to make the list-item marker optional, so the bare-key indented form (`unique_combination_of_columns:` under a short-form `tests:` map) also matches. - NIT #9 — added regression test for removed-line risk-key promotion (removing a `data_tests:` block is at least as risk-worthy as adding one). - NIT #10 — comment reworded from `unique_combination_of_columns` is a "TEST NAME" → "dbt test macro / test parameter". Full altimate review suite: 3807 pass / 640 skip / 0 fail (was 3781 baseline — 26 new tests). Codex-round-6 minor addressed: `dbtRiskYmlKeyMatches` is now called once via a shared `scannedForRisk` local and its result reused for both `dbtRiskYmlChanges` and `dbtRiskYmlKeys`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of * fix(review): [R20 S4] address kilo-code-bot review — blockScalarStart regex tightened kilo-code-bot suggestion on PR #1028 — the earlier `blockScalarStart` regex accepted only `|`/`>` with an optional `+`/`-` chomping indicator, so YAML block-scalar headers carrying an explicit indentation indicator (`description: |2`, `|+2`, `|2-`) or a trailing comment (`description: | # legacy`) failed to open a scalar. Body lines beginning with a risk keyword (`data_tests:`, `constraints:`, …) then false-positive promoted the file to `full`. Safe-direction over-tiering (never a wrong verdict), but this regex is the sole scalar gate — widen it to close the gap. Regex now: `[|>](?:[+-]?[1-9]?|[1-9]?[+-]?)[ \\t]*(?:#.*)?$`. - `[|>]` opener - `(?:[+-]?[1-9]?|[1-9]?[+-]?)` chomp+indent in either order - optional trailing `# comment` New regression test loops over `|2`, `|+2`, and `| # legacy comment` opener shapes and asserts each produces `trivial` on a diff whose body lines contain risk keywords. Full altimate review suite: 3808 pass / 640 skip / 0 fail (was 3807 baseline — 1 new test). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of * fix(review): [R20 S4] context-line indent measurement in changedLinesForScan (cubic P2) cubic P2 bot review — the scalar-state tracker measured indent inconsistently between context and changed diff lines. Context lines start with a leading SPACE (the diff marker) and my earlier code left it in place; changed lines had their `+`/`-` marker stripped before indent measurement. Result: a valid `|1` block scalar opened in the context with a body indented exactly one space beyond the header wasn't masked, because the context-side `scalarIndent` counted one extra space and the changed-side body's indent then failed the `indent > scalarIndent` check. Fix strips the leading diff marker (`+`/`-`/space) on ALL lines before indent measurement, so context and changed lines live in the same coordinate system. New regression test: `|1` block scalar opened in context with body containing `data_tests:` / `constraints:` prose stays trivial. Full altimate review suite: 3808 pass / 640 skip / 0 fail (was 3807 baseline — 1 new test). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of * refactor(review): [R20 S4] move FinOps token list from reviewer core to `riskTierPathTokens` config The hardcoded `FINOPS_TOKEN_RE` in `risk-tier.ts` baked one team's billing/cost vocabulary into a reviewer everyone consumes. This moves the list out of core to user-configurable `.altimate/review.yml`, keeps the shipped list as an opt-in `preset:finops`, and generalizes the field so custom categories (pci, patient, etc.) work the same way. Also refactors the vacuous FP-guard test flagged by `altimate-harness-bot` (review.test.ts:755-768): the earlier `broadcaster` / `precast_table` paths did not contain any FinOps token substring, so the boundary anchors in the regex were never exercised — the test would pass even if the boundary class were deleted. Replaced with property-based loops over `RISK_TOKEN_PRESETS.finops` that assert boundary behavior for every preset token: `stg_{tok}_daily.sql` fires, `stg_x{tok}y.sql` does not. Adding a preset token extends coverage automatically. Core changes: - `config.ts` — new `riskTierPathTokens: Record<string, string[]>`, default `{}`. Users opt in by naming a category and listing tokens or `preset:<name>` markers. - `risk-tier.ts` — dropped `FINOPS_TOKEN_RE`. `FileChangeClass.finopsPathToken: boolean` → `.highRiskPathTokenCategory: string | undefined`. Reason string reads `path matches high-risk token category '<category>'`. Added `RISK_TOKEN_PRESETS` (shipped presets) and `compilePathTokenResolver` (compiles config into a resolver, boundary-anchored, handles preset expansion). - `orchestrate.ts` — threads `config.riskTierPathTokens` through `compilePathTokenResolver` into `classifyPR` as the `pathTokenCategoryOf` callback. - Comments genericized: dropped internal round / corpus / vertical- specific vocabulary ("PR D / PR E", "DBU savings > DBU cost", "misanchored billing units") in favor of neutral wording. Backwards compatibility: projects that were relying on hardcoded FinOps promotion should add `riskTierPathTokens: {finops: [preset:finops]}` to their `.altimate/review.yml`. A project that never wanted FinOps promotion (majority case) now gets no path-token promotion at all — the reviewer core is neutral. Tests: 229/229 green (review*.test.ts). Includes property-based loops over the preset for both boundary-firing and interior-substring suppression, plus a "no resolver configured → no promotion" test that locks in the neutral-core guarantee. * fix(review): [R20 S4] reject empty-string tokens in riskTierPathTokens (cubic-review P2) An empty string in `riskTierPathTokens.<category>` compiles into a regex alternative like `(?:|foo)` — the empty branch matches the empty string between two adjacent boundary chars, so any path with two `_`/`.`/`-`/`/`/digit chars in a row (e.g. `stg__orders.sql`) silently over-promotes on any configured category. Reject at the config-schema layer via `z.string().min(1)`; the resolver itself is unchanged. Test: `riskTierPathTokens rejects empty-string tokens` under `describe("config")` in review.test.ts — asserts throw on empty token, non-empty tokens still parse. 230/230 review-* tests pass. * fix(review): [R20 S4] tighten changedLinesForScan marker guard to only emit +/- lines (harness-bot P2) altimate-harness-bot review, PR #1028 risk-tier.ts:251. The earlier guard `if (marker === " ") continue` skipped only unified-diff context lines (space-prefixed). Free-form lines from a raw `git diff -p` output — `diff --git a/... b/...`, `index abc..def`, `Author:`, `Date:` — carry a `""` marker (they don't start with `+`, `-`, or space), fall past the guard, and reach `dbtRiskYmlKeyMatches` via `out.push(raw)`. In-production `file.diff` comes from the GitHub PR files API, whose per-file diff blocks begin at the first `@@` hunk header (no `diff --git` / `index` / commit-metadata lines). So the exposure was defensive parity, not a live promotion bug. But the code was asymmetric — `+++`/`---`/`@@` headers were explicitly skipped at line 220; other free-form lines weren't — and a future caller passing raw `git diff -p` output would silently see header lines in the scanner. Fix (one operator): `if (marker !== "+" && marker !== "-") continue`. Any line whose leading character isn't `+` or `-` is either a context line (fine, still updated scalar state above) or a free-form header (skipped, no emit). Test: `R20 S4: raw git diff -p free-form headers do NOT reach the scanner` builds a full raw-diff string (four header lines + the `@@` hunk + one changed line) and asserts the same tier verdict as the bare-`tests`-word test — trivial, no promotion. 231/231 review-* tests pass. * fix(review): [R20 S4] fail loud on unknown `preset:<name>` in riskTierPathTokens (cubic + harness-bot P2) Two bots on PR #1028 flagged the same silent-failure: a typo in a `preset:<name>` entry (e.g. `preset:finop` instead of `preset:finops`) compiled to an empty token list; then `if (tokens.length === 0) continue` dropped the category with no error. The user's opt-in for a risk-promotion category quietly did nothing — exactly the safety invariant this PR is meant to guarantee. Fix: `compilePathTokenResolver` now throws on an unknown preset name before assembling the resolver. Error message names the offending category, the typo, and the shipped preset list so the reader has a fix pointer: Error: riskTierPathTokens.finops: unknown preset 'finop'. Known presets: finops. Configure a bare token list (e.g. ['card', 'pan']) instead if you want a custom category. The shipped preset list is small and stable, so an unknown name is almost certainly a typo, not a forward reference. Fail at review-start means the misconfiguration is caught immediately, not discovered when a real high-risk PR slips through. Tests: `unknown preset name throws with the known-list in the message` asserts throw on `preset:finop`, throw when a typo is mixed with valid tokens, and that the known preset (`preset:finops`) still resolves promotion correctly on `mrt_cost_daily.sql`. 111/111 tests in review.test.ts pass. * fix(review): [R20 S4] catch riskTierPathTokens config error in orchestrate — don't crash the run (cubic P2) Follow-up to `919bcc17f`. That commit made `compilePathTokenResolver` throw on an unknown `preset:<name>` — the right safety fix — but nothing between the resolver and the CLI handler caught the throw, so a single typo in `.altimate/review.yml` (e.g. `preset:finop` instead of `preset:finops`) would crash every review run for the project's CI (cubic-review P2 on PR #1028 risk-tier.ts:134). Fix: wrap the `compilePathTokenResolver` call in `orchestrate.ts` in a try/catch. On throw: 1. Log a prominent warning to stderr with the config-error message and a fallback banner ("Review continuing without path-token promotion") so the CI log tells the operator what happened. 2. Fall back to `pathTokenCategoryOf = undefined` so `classifyPR` still runs — no promotion fires, but the rest of the review proceeds normally. 3. Prepend the config error to `tierResult.reasons` so it surfaces in `envelope.tierReasons` when `explainTier` is set. A reader of the envelope (or a PR-comment renderer) sees WHY their opt-in didn't fire without having to dig through CI logs. The trade-off is deliberate: the user's opt-in is dead until they fix the typo (conservative safe fallback — no silent auto-approve on billing/PCI/etc. paths), but the review itself doesn't die. Test: `runReview does NOT crash on an unknown-preset config typo — degrades gracefully` builds a minimal fake ReviewRunner + config with `preset:finop` typo, captures stderr, asserts (a) envelope is produced (no crash), (b) stderr contains both the config-error message and the fallback banner, (c) `env.tierReasons` carries the same error so envelope readers see it too. 112/112 tests in review.test.ts pass. * fix(review): [R20 S4] surface pathTokenConfigError in envelope even without --explain-tier (coderabbit review) Follow-up to `9435bc6f9` (the try/catch around `compilePathTokenResolver` that surfaces a config typo in `tierResult.reasons` instead of crashing). The envelope's `tierReasons` field was gated on `input.explainTier || tierForced` — both default false in a normal `comment`/`gate` run — so the config error I prepended to `tierResult.reasons` was silently stripped when the envelope was built. Net effect on a typoed `.altimate/review.yml`: review ran, no promotion fired, no error appeared in the PR comment. Only a stderr trace no customer ever sees. Exact silent-failure mode the fail-loud fix was supposed to close. Fix: expand the gate to include the config-error case. tierReasons: input.explainTier || tierForced || pathTokenConfigError ? tierReasons : undefined Regression test: `config error surfaces in envelope even WITHOUT --explain-tier` builds a config with `finops: [preset:finop]`, calls `runReview` with `explainTier` unset (default), and asserts `env.tierReasons` still contains both `riskTierPathTokens config invalid` and `unknown preset 'finop'`. 113/113 tests in review.test.ts pass (was 112). --------- Co-authored-by: Haider <haider@altimate.ai> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
For every column named in a `dbt_utils.unique_combination_of_columns`
test's `combination_of_columns`, require `not_null` coverage on the
same model. Otherwise a NULL grain-key value silently passes the
uniqueness test — the guardrail is toothless. Cited as a hard rule in
`docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't.
Coverage sources:
- `constraints: [{type: not_null}]` — only counted when
`contract.enforced == true` (per codex R20 S1 high #3). On views /
non-contracted models, `constraints:` is documentation-only, not
enforced, so we don't miss a real gap.
- Column-level `data_tests: [not_null]` / `tests: [not_null]` — always
counted; dbt's test runner enforces regardless of contract state.
Recommendation flips between `constraints:` (contracted model) and
`data_tests:` (view / non-contracted) based on the model's contract
state — matches the adapter-semantics discussion in the corpus study
(PR D×2 + PR A×2).
Supported YAML shapes:
- `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns`
(exact match per codex high #1; `endsWith` would over-match).
- pre-1.9 flat args + dbt 1.9+ `arguments:` nesting.
- top-level `contract:` and nested `config.contract:` for enforcement flag.
False-positive guards:
- Column-name comparison is case-folded (Snowflake identifiers, per
codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches
`workspace_id` in `columns:`.
- Skipped on `status === "deleted"` files (no current grain to guard).
- Only runs on files in the PR diff, not repo-wide.
### Validation on 5-PR internal corpus (recall improvement)
vs S4 baseline (the tier-promotion PR this branch stacks on):
| Variant | S4 baseline | S1 result | Delta |
|---|---:|---:|---:|
| A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** |
| A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) |
Grain-key gaps caught:
- PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type`
- PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1
- PR A, D, E: 0 (fixes already landed in HEAD state per corpus study)
Strong matches to human blocker findings on PR C:
- `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`,
`int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical:
"workspace_id missing from carrier identity")
- `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time`
→ PR C human F10 (critical: "dbt mart grain includes period_end_time")
### Tests
- 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests).
- 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files).
- Codex-reviewed diff, 3 highs addressed:
- endsWith → exact-name match
- column-name case-folding
- constraints only count when contract enforced
- Codex minor #4 addressed (test fixture for top-level `contract:`)
- Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage)
Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn
stacks on PR #1027 (feat/review-r18-observability-recall).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of
sahrizvi
added a commit
that referenced
this pull request
Jul 23, 2026
…findings on 5-PR corpus) (#1029) * feat(review): [R20 S1] grain-key `not_null` completeness detector For every column named in a `dbt_utils.unique_combination_of_columns` test's `combination_of_columns`, require `not_null` coverage on the same model. Otherwise a NULL grain-key value silently passes the uniqueness test — the guardrail is toothless. Cited as a hard rule in `docs/DBT_GUIDELINES.md` in the corpus repo; kilo catches it, we didn't. Coverage sources: - `constraints: [{type: not_null}]` — only counted when `contract.enforced == true` (per codex R20 S1 high #3). On views / non-contracted models, `constraints:` is documentation-only, not enforced, so we don't miss a real gap. - Column-level `data_tests: [not_null]` / `tests: [not_null]` — always counted; dbt's test runner enforces regardless of contract state. Recommendation flips between `constraints:` (contracted model) and `data_tests:` (view / non-contracted) based on the model's contract state — matches the adapter-semantics discussion in the corpus study (PR D×2 + PR A×2). Supported YAML shapes: - `unique_combination_of_columns` and `dbt_utils.unique_combination_of_columns` (exact match per codex high #1; `endsWith` would over-match). - pre-1.9 flat args + dbt 1.9+ `arguments:` nesting. - top-level `contract:` and nested `config.contract:` for enforcement flag. False-positive guards: - Column-name comparison is case-folded (Snowflake identifiers, per codex high #2) so `WORKSPACE_ID` in `combination_of_columns` matches `workspace_id` in `columns:`. - Skipped on `status === "deleted"` files (no current grain to guard). - Only runs on files in the PR diff, not repo-wide. ### Validation on 5-PR internal corpus (recall improvement) vs S4 baseline (the tier-promotion PR this branch stacks on): | Variant | S4 baseline | S1 result | Delta | |---|---:|---:|---:| | A1 (`--pure --no-ai=true`) | 118 | 129 | **+11 grain-key gaps** | | A2 (`--pure`, LLM lane) | 143 | 150 | +7 (net; LLM run-to-run noise) | Grain-key gaps caught: - PR B (#1111) ×1: `mrt_all_purpose_auto_tune_recommendation.rec_type` - PR C (#862) ×10: workspace_id x3, job_id x2, period_start_time x2, period_end_time x2, task_key x1 - PR A, D, E: 0 (fixes already landed in HEAD state per corpus study) Strong matches to human blocker findings on PR C: - `int_job_run_billing.workspace_id`, `int_job_run_cost_carrier.workspace_id`, `int_job_task_run_cost_carrier.workspace_id` → PR C human F11 (critical: "workspace_id missing from carrier identity") - `mrt_job_run_timeline.period_end_time`, `mrt_job_task_run_timeline.period_end_time` → PR C human F10 (critical: "dbt mart grain includes period_end_time") ### Tests - 71 pass / 0 fail in `review-dbt-patterns.test.ts` (58 pre-existing + 13 new S1 tests). - 3797 pass / 640 skip / 0 fail in the full altimate review suite (132 files). - Codex-reviewed diff, 3 highs addressed: - endsWith → exact-name match - column-name case-folding - constraints only count when contract enforced - Codex minor #4 addressed (test fixture for top-level `contract:`) - Codex minor #5 addressed (test fixture for non-contracted constraints ≠ coverage) Depends on PR #1028 (feat/review-r20-s4-triage-promotion), which in turn stacks on PR #1027 (feat/review-r18-observability-recall). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of * fix(review): [R20 S1] address consensus-review findings on grain-key detector Bundle addresses PR #1029's consensus review: - MAJOR #1 — model-level and column-level primary_key / not_null constraints now count as coverage. dbt 1.5+ supports model-level constraints via `constraints: [{type: primary_key, columns: [a, b]}]`, and a primary_key inherently enforces NOT NULL on Postgres/Snowflake/ BigQuery/Databricks. Grain columns declared via a model-level PK were previously falsely flagged as missing not_null. Fix scans both `mm.constraints` (model-level, honouring the `columns:` list) and column-level `constraints: [{type: primary_key}]`. - MINOR #3 — contract-precedence bug. Earlier ternary short-circuited when `cfg.contract` was any object (e.g. `config: {contract: {alias: X}}` with no `enforced` key), masking a top-level `contract: {enforced: true}`. Now evaluated independently at both locations and OR'd. - MINOR #4 — `dbt.` prefix on namespaced test names (`dbt.not_null` in dbt 1.8+) is stripped in `testName()` before matching, so it counts as coverage. - MINOR #6 — `{name: <alias>, test_name: not_null}` alternative object form is recognised. Reading `Object.keys(t)[0]` returned `name` (the alias) rather than the underlying test type. Now `test_name` wins when present, falling back to the first key. - NIT #7 — `norm()` hoisted from per-model to function scope. Consensus items NOT addressed this round: - NIT #8 (dedup GrainKeyGap for a column listed twice / multiple grain tests) — collapses downstream via the global finding fingerprint; cosmetic rather than correctness. - NIT #9 (model-level `data_tests:` scanned as a grain-test source) — reviewer noted "harmless (no false match)"; no action. - NIT #10 (documentation of fallback-skip) — no code change needed. - MINOR #5 (contract resolved from dbt_project.yml or SQL config) — cross-file / cross-context, deferred. Regression tests (all pass; 82 pass / 0 fail in review-dbt-patterns suite, 3828 pass / 0 fail in full altimate suite — up from 3785): - Model-level primary_key constraint covers grain columns - Model-level not_null constraint with `columns:` list covers named cols - Column-level primary_key counts as coverage - Precision guard: PK missing cols still flagged - Model-level constraints on non-contracted model don't count - `config.contract` without `enforced` does NOT mask top-level `contract: {enforced: true}` (MINOR #3) - `dbt.not_null` covers (MINOR #4) - `{name:, test_name: not_null}` alternative form covers (MINOR #6) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of * chore(review): [R20 S1] genericize internal-vocab leaks in dbt-patterns comments Companion cleanup to PR #1028's FinOps strip. The grain-key detector's docstrings named a specific internal column (`workspace_id`) as the Snowflake case-folding example and referenced internal corpus PR labels (`PR D×2, PR A×2`). Neither carries product meaning outside the altimate-ingestion codebase. - `dbt-patterns.ts:962` — replace `WORKSPACE_ID` / `workspace_id` example with `ORDER_ID` / `order_id`. Same illustrative point, no internal name. - `dbt-patterns.ts:1443` — replace `(PR D×2, PR A×2)` corpus-label citation with `(four instances across the sample)`. Same count, no internal reference. Tests: 250/250 green. * fix(review): [R20 S1] address altimate-harness-bot review findings on grain-key detector Two substantive findings on PR #1029: 1. `dbt-patterns.ts:1099` — grain detector was not change-scoped. `extractGrainKeyGaps(newDoc)` ran against every entity in the file, so a housekeeping edit (description bump, meta tag) surfaced all pre-existing grain gaps on unrelated models. Real precision cost: reviewers seeing findings on a PR they didn't intend suppress the whole rule. Fix: compare the `combination_of_columns` set per entity between `oldDoc` and `newDoc`; only emit gaps for entities whose grain declaration actually changed (added, removed, or column set diff). Added entities on the new side count as changed; the "added file" case (no oldDoc) unconditionally treats every entity as changed so newly-shipped grain declarations are still guarded. New helpers: `extractGrainDeclarations` returns `Map<entityName, Set<column>>` and `grainDeclChangedEntities` diffs two docs into a set of entity names whose declaration moved. Existing test that pinned the old steady-state-fires behavior (`R20 S1: unique_combination_of_columns with grain col missing not_null → warning finding`) rewritten to test the newly-added case; added a companion test that pins the new precision guarantee (`steady-state grain gap on unchanged model is NOT re-surfaced`), plus a `grain declaration changed (column added to combination_of_columns) does fire gap` test that keeps the regression case covered. 2. `dbt-patterns.ts:963` — `extractGrainKeyGaps` iterated only `d.models`, silently skipping `snapshots`, `sources`, and `seeds`. `unique_combination_of_columns` on a snapshot is a real SCD-2 grain declaration; sources declare per-table `columns:` + tests; seeds carry the model shape. Fix: new `iterateGrainEntities(d)` helper walks all four sections (mirrors `extractTestOccurrences`). Sources descend one level to iterate their `tables[]` entries which carry the model shape. Three new tests: `grain detector also covers snapshots`, `... source tables`, `... seeds`. Tests: 255/255 green (8 review-* files, +6 new grain-key tests). * fix(review): [R20 S1] qualify source-table entity names as `<source>.<table>` (cubic-review P2) Two sources both containing a table named `orders` (e.g. `raw.orders` and `legacy.orders`) previously conflated in `iterateGrainEntities`: - Grain-change detection collapsed them into a single map entry, so a change in one source's grain declaration could surface or suppress gaps for the other. - Finding fingerprint uses the entity name; two distinct source tables with the same table name would dedupe to a single finding. Fix: `iterateGrainEntities` now returns `{name, body}` pairs. Source tables are qualified as `${sourceName}.${tableName}` while models, snapshots, and seeds retain their unqualified name (they live in a flat namespace already). Tests: `grain detector also covers source tables` updated to assert the qualified name; new test `same source-table name in two sources does NOT conflate` locks in the fix. 257/257 review-* tests pass. --------- Co-authored-by: Haider <haider@altimate.ai> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improvements to
altimate-code reviewcovering two bug fixes and four observability / recall features that share the same code surface. Filed againstpackages/opencode/src/{cli/cmd/review.ts, altimate/review/*.ts}.Bug fixes
Both are reproducible against
mainand closed by this PR:--no-aiflag triggers help path instead of running the review #1025 — bare--no-ai <other-flag>triggers the yargs help path and exits 0 without running the review. Fix:.parserConfiguration({ "boolean-negation": false })on the review command's yargs builder, so--no-aibinds to the declarednoAioption as authored.--no-ai=true/--no-ai=falsecontinue to work for programmatic parity.detectSchemaYmlPatternscompared removed and added test lines as raw trimmed strings, so a genuine removal from one column was silently cancelled by an unrelated re-add on a sibling column (yaml re-serialization emits the same- unique/- not_nulllines for other columns still declaring those tests). Fix:collectTestOccurrencesFromDiffindbt-patterns.tswalks the unified diff with model/column context via nearest preceding- name: Xheader (resets on hunk boundaries), keys removals by(model, column, test), and only cancels on a same-tuple re-add. Emits one finding per removed(model, column, test)triple, with severity elevated towarningforuniqueremovals and fornot_nullon id/key columns. Body text detects layer from file path (marts?/reporting→ "mart-layer key", else "declared key") so the copy doesn't mislabel non-mart schema files.Feature enhancements bundled with the fixes
Four additions in the same code surface, small enough to review alongside the bug fixes rather than as their own PRs:
--explain-tier— new boolean flag. When set, the verdict envelope carriestierReasons: string[](the risk classifier's reasons for the tier decision) and the human-readable summary prints a🧭 **Tier: X** — <reasons>line. Read-only — doesn't change tier classification or verdicts. Envelope field is optional (absent when the flag is off), so existing verdicts remain byte-equivalent.--force-tier <trivial|lite|full>— new experimental flag for bench and debug work. Overrides the tier classifier with the supplied tier. Guardrails:⚠️ --force-tier=X is EXPERIMENTAL (bench / debug only). Classifier bypassed; verdict envelope will carry tierForced: true.warning to stderr on every use.tierForced: trueandtierClassified: <original>whenever the flag is passed, regardless of whether the forced value matches the classifier's decision. (An earlier iteration of this change guarded that flag oninput.forceTier !== classifiedTier, which meant--force-tier=fullon a naturally-fullPR silently bypassed the audit envelope. Corrected in the same branch — every use of the flag is now recorded.)<tier> tier — forced (was <classified>).tierReasonsis always populated when the flag is used, with a leadingforced via --force-tier=X (classifier said Y)marker so downstream can't confuse the forced tier for a natural one.describe; the stderr banner ensures no one uses it in CI without noticing.Manifest auto-discovery — before,
--manifest <path>had to be explicit; otherwise the CLI used the config-relative defaulttarget/manifest.json, which silently missed whencwdwasn't the dbt project root, degrading every such review to lint-only. Now:--manifestisn't explicit AND the config-relative path doesn't exist, walk UP fromcwdlooking fordbt_project.yml. Use the adjacenttarget/manifest.jsonwhen it exists.ℹ️ auto-discovered dbt manifest at ...) so users see which manifest the review used.target/from an unrelated tool never gets picked up.--headis set (CI shape) AND any changed file has an mtime newer than the manifest, print⚠️ manifest ... appears staleto stderr. Skipped for working-tree diffs (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.Verdict envelope — three new optional fields added to
VerdictEnvelopeinverdict.ts, all included in the signed canonical body so tampering is detectable:tierReasons?: string[](from--explain-tier)tierForced?: boolean(from--force-tier)tierClassified?: RiskTier(from--force-tier— original tier before the override)All are optional and absent when the corresponding flag is off, so existing verdicts remain byte-equivalent to before.
Testing
Verified locally on synthetic scenarios:
--no-ainow runs the review (verifies bug fix for review: bare--no-aiflag triggers help path instead of running the review #1025).--no-ai=trueand--no-ai=falsecontinue to work.--explain-tierpopulatestierReasonson bothtrivialandfullverdicts.--force-tier=fullon a naturally-fullPR setstierForced: trueandtierClassified: "full"on the envelope + emits the stderr warning + the leadingforced viareason.--force-tier=liteon a naturally-fullPR overrides tier toliteand recordstierClassified: "full".target/manifest.jsonwhen invoked from a subdirectory containingdbt_project.ymlupward; declines to use atarget/manifest.jsonfrom a directory that has nodbt_project.yml(verified against a plain non-dbt git repo).unique+not_nullfrom one column while sibling columns still have them) now surfaces twowarning-severity findings, one per removed(column, test)tuple. Layer-aware body renders "declared key" for a top-levelmodels/schema.ymland "mart-layer key" for amodels/marts/schema.yml.verifyEnvelopefor both flag-on and flag-off shapes.Test plan
--no-airuns the review (repro from review: bare--no-aiflag triggers help path instead of running the review #1025 no longer occurs)--no-ai=true/--no-ai=falsebehavior unchanged--explain-tierproducestierReasonsin JSON output; envelope signature verifies--force-tierprints EXPERIMENTAL warning to stderr; envelope carriestierForced: true+tierClassified: <original>even when forced tier matches classifiertarget/manifest.jsonfrom subdir; explicit--manifeststill wins; auto-discovery declines outside a dbt project--headis set; silent otherwise(column, test)findingsmainCloses #1025.
Closes #1026.
Summary by cubic
Fixes bare
--no-airuns, hardens file reads against symlink escapes with a sharedsafeReadInsidehelper, upgrades schema.yml test‑removal to a structural YAML diff (incl. deleted/renamed files), auto‑discovers the dbt manifest, and makes subdir/monorepo invocations resolve compiled SQL and working‑tree files correctly. Closes #1025 and #1026.Bug Fixes
--no-ainow runs instead of falling into help.safeReadInsidehelper.tests/data_tests; handles deleted and renamed files; model‑level and column‑level attribution; per‑removal findings; elevated severity foruniqueandnot_nullon likely key columns; multiplerelationshipson one column are distinct; snapshot property files undersnapshots/*.ymlnow route here; safe diff‑only fallback with per‑occurrence tags; emits the per‑file summary once with clearer “This schema file drops …” wording.tierClassifiedis missing; envelope schema enforcestierForced↔tierClassifiedconsistency and rejectstierForced: false; backtick‑safe, capped tier reason rendering..mdundermodels/oranalyses/as manifest‑affecting to avoid false positives.packages/dbt/models/...resolve under the discovered dbt project.New Features
--explain-tier: attachestierReasons[]to the envelope and prints a capped tier line in the summary.--force-tier <trivial|lite|full>(experimental): overrides the classifier with a stderr warning; envelope recordstierForced: trueandtierClassified; reasons start with a “forced via …” marker; headline shows “forced (was X)”.--manifestis omitted and the config default is missing, walks up to the nearestdbt_project.ymland uses itstarget/manifest.json; logs the chosen path; never picks a manifest outside a dbt project; warns if stale when--headis set; routes compiled SQL resolution and project name to the discovered project root. Explicit--manifestalways wins.tierReasons,tierForced, andtierClassified(all signed). Backwards compatible when flags aren’t used.Written for commit 69553ed. Summary will update on new commits.
Summary by CodeRabbit
New Features
--explain-tierto include tier reasoning in the verdict and enrich the summary when reasons are available.--force-tierto override tier selection and record forced-tier auditing metadata.Bug Fixes
schema.ymlguardrail removal detection with YAML-aware attribution (includingdata_tests), plus a safe diff-only fallback.--no-ai.snapshots/(now handles non-.sqlsnapshot YAML files).Tests