feat(review): [R20 S4] triage promotion — never auto-approve risk-bearing dbt metadata - #1028
Conversation
![claude[bot]](https://cdn.github.com/in/1236702?s=60&v=4){kind=small}
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable path-token risk promotion, scalar-aware ChangesRisk-tier promotion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewConfig
participant runReview
participant compilePathTokenResolver
participant classifyPR
participant tierResult
ReviewConfig->>runReview: provide riskTierPathTokens
runReview->>compilePathTokenResolver: compile configured tokens
compilePathTokenResolver-->>runReview: pathTokenCategoryOf or error
runReview->>classifyPR: classify changes with resolver
classifyPR->>tierResult: return tier and promotion reasons
runReview->>tierResult: prepend resolver error when compilation fails
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 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. |
![kilo-code-bot[bot]](https://cdn.github.com/in/2193792?s=60&v=4){kind=small}
| // dbtRiskYmlChanges hit but doesn't fire on its own. | ||
| if (c.dbtRiskYmlChanges) { | ||
| const loc = c.martLayerChange ? " under models/marts/ (mart-API surface)" : "" | ||
| reasons.push(`schema.yml diff touches data_tests/constraints/contract${loc}`) |
There was a problem hiding this comment.
SUGGESTION: Reason string omits unique_combination_of_columns
DBT_UNIQUE_COMBO_RE (line 78) matches unique_combination_of_columns and feeds into dbtRiskYmlChanges, but the surfaced reason only lists data_tests/constraints/contract. A customer who adds only a unique_combination_of_columns test (covered by the test at review.test.ts:466) would see a reason naming keys they didn't touch — inconsistent with both the field doc (lines 43-45) and the cubic summary in the PR description, which both list all four signals.
| reasons.push(`schema.yml diff touches data_tests/constraints/contract${loc}`) | |
| reasons.push(`schema.yml diff touches data_tests/constraints/contract/unique_combination_of_columns${loc}`) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of Fix: Test: No redundant code; the three-way OR condition is already minimal and correctly grouped. Files Reviewed (2 files)
Previous Review Summaries (9 snapshots, latest commit 9bdd6aa)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9bdd6aa)Status: No New Issues Found | Recommendation: Merge Incremental review of HEAD Files Reviewed (4 files — rebase only, content unchanged)
Note: One previously-raised, non-blocking SUGGESTION remains open as an active inline comment — Previous review (commit 9435bc6)Status: 1 Issue Found | Recommendation: Address before merge Incremental review of commit Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files — incremental commit 9435bc6)
Fix these issues in Kilo Cloud Previous review (commit 919bcc1)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 9b2c024)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 29c2423)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (4 files)
Previous review (commit cd8b970)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (2 files)
Previous review (commit e52c863)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (2 files)
Previous review (commit 4544cf8)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit 8eac0d9)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Reviewed by glm-5.2 · Input: 41K · Output: 6.3K · Cached: 334.4K Review guidance: REVIEW.md from base branch |
Validation report against offline dbt-PR corpusRan the branch CLI against a set of merged dbt PRs used for offline validation (both Recall delta
Tier changesOne PR in the corpus was previously classified InterpretationThe change is a routing improvement, not a detector. On this corpus:
Value is foundational: this branch unblocks subsequent detector PRs from being neutered by miscalibrated triage. Real recall wins show up in the detector-work PRs stacked above it. |
![cubic-dev-ai[bot]](https://cdn.github.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Consensus Code Review — PR #1028Title: feat(review): [S4] triage promotion — never auto-approve risk-bearing dbt metadata Panel: multi-model review. Verdict: REQUEST CHANGES — 2 MAJOR (targeted fixes), 5 MINOR, 3 NITThe promotion mechanism is verified correct by all 8 reviewers ( Counts: Critical 0 · Major 2 · Minor 5 · Nit 3 MAJOR1. Legacy
|
| Reviewer | Verdict on synthesis | Key point |
|---|---|---|
| a reviewer | CHANGES NEEDED → incorporated | no. 2 → MINOR (cost-only fail-safe); accredited example wrong; contract: test does exist (removed MINOR no. 8); no. 3 is MINOR/low-MAJOR. |
| a reviewer | CHANGES NEEDED → incorporated | no. 2 → MINOR; no. 3 correctly MAJOR; no. 1 correctly top. |
| a reviewer | CHANGES NEEDED → incorporated | no. 2 → MINOR (dbus is a real bug though); no. 3 correctly MAJOR. |
| Reviewer | (author) | Agreed with all three downgrades/corrections. |
Applied in convergence: no. 2 MAJOR→MINOR (3 reviewers, unanimous); retracted the accredited example (shell-verified non-match); removed the "no contract: test" MINOR (test exists at review.test.ts:405). no. 3 kept MAJOR ( + + a reviewer vs 's lone MINOR). no. 1 unchanged as top finding. Converged in 1 round.
Finding attribution
| # | Finding | Origin | Type |
|---|---|---|---|
| 1 | Legacy tests: key not promoted |
multiple reviewers, a reviewer, multiple reviewers | Consensus (8-way) |
| 3 | Vacuous unique_combination_of_columns test |
a reviewer | Unique (shell-verified) |
| 2 | FinOps over-promotion on *_rate/dbus paths |
multiple reviewers, a reviewer, multiple reviewers | Consensus; severity set MINOR in convergence |
| 4 | Reason omits actual triggering signal | , a reviewer | Unique |
| 5 | "upgrades the WEIGHT" comment misleading | multiple reviewers | Unique |
| 6 | Block-scalar description FP | a reviewer | Unique |
| 7 | FinOps boundary misses digit suffixes | Reviewer | Unique |
| 8–10 | NITs (short-form regex, removed-line untested, comment) | multiple reviewers, a reviewer, | Nit |
| ✓ | Promotion ordering correct; martLayerChange enrichment-only | all 8 | Consensus (positive) |
Reviewed by a multi-model panel.
…motion 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
8eac0d9 to
4544cf8
Compare
| const lines = diff.split("\n") | ||
| const out: string[] = [] | ||
| let scalarIndent = -1 | ||
| const blockScalarStart = /^[ \t]*[^\s#:][^:]*:[ \t]*[|>][+-]?[ \t]*$/ |
There was a problem hiding this comment.
SUGGESTION: blockScalarStart skips block-scalar headers that carry an explicit indentation indicator (description: |2, >2-) or a trailing comment (description: | # legacy)
[+-]? consumes only a chomping indicator, so legal headers like |2, |+2, or | # comment don't match. When such an opener is in the diff the scalar is never opened and its body lines aren't blanked — so a body line that happens to start with a risk keyword (data_tests: / tests: / …) would false-positive promote the file to full. Narrow and safe-direction (over-tiering only, never a wrong verdict), but this regex is the sole scalar gate, so widening the class closes the gap.
| const blockScalarStart = /^[ \t]*[^\s#:][^:]*:[ \t]*[|>][+-]?[ \t]*$/ | |
| const blockScalarStart = /^[ \t]*[^\s#:][^:]*:[ \t]*[|>][+-]?[0-9]?[+-]?[ \t]*(?:#.*)?$/ |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in e52c863ae. blockScalarStart regex widened to [|>](?:[+-]?[1-9]?|[1-9]?[+-]?)[ \t]*(?:#.*)?$ so |2, |+2, |2- (explicit indentation indicator, either order relative to chomp) and | # comment (trailing comment) all open a scalar. Regression test loops over |2, |+2, | # legacy comment shapes and asserts each produces trivial on a diff whose body contains risk keywords.
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
… 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
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
![altimate-harness-bot[bot]](https://cdn.github.com/u/94671887?s=60&v=4){kind=small}
| // False-positive guard: the token regex requires path/word boundaries so | ||
| // words like `broadcaster.py` (has `caste`) or `precast_table.sql` (has | ||
| // `cast`) don't fire the cost/dbu/etc. rules. | ||
| const r1 = classifyPR([file("models/staging/stg_broadcaster.sql", "+select 1\n")], { |
There was a problem hiding this comment.
nit: This test uses and to guard against false positives, but neither word actually contains any FinOps token ( has , has — neither is in the token list). The boundary regex is never exercised here; the test would pass even if the boundary class were removed entirely.
A stronger FP guard would use a word that contains a FinOps token as an interior substring:
generaterateecorporaterateo
These would catch a regression where the boundary anchors were weakened.
There was a problem hiding this comment.
Addressed — the strip commit (21f99146e) moves the FinOps token list out of risk-tier.ts core into user-configurable .altimate/review.yml (riskTierPathTokens), keeps the shipped list as an opt-in preset:finops, and replaces this vacuous test with property-based loops over RISK_TOKEN_PRESETS.finops:
- Positive (fires):
stg_${tok}_daily.sqlfor every preset token — the boundary case. - Negative (does NOT fire):
stg_x${tok}y.sql— letters on both sides so the alternation matches only because of the interior substring; only the boundary anchors block promotion. If someone weakens the boundary class, every one of these fails simultaneously. - Neutrality guarantee:
no promotion when no resolver is configured (core is neutral)— a project that doesn't opt into any category gets zero path-token promotion.
Adding a preset token now extends coverage automatically. Thanks for the catch.
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
…ns 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.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| tokens.push(entry) | ||
| } | ||
| } | ||
| if (tokens.length === 0) continue |
There was a problem hiding this comment.
Silent unknown preset name. A typo in .altimate/review.yml — e.g. preset:finop instead of preset:finops — silently produces an empty tokens list, then if (tokens.length === 0) continue drops the category with no error or warning. The user's risk-promotion gate quietly does nothing.
This is the exact safety invariant the PR is meant to guarantee, so a misconfiguration should be loud. The preset list is small and stable, so an unknown name is almost certainly a typo, not an intentional forward-reference:
if (preset) tokens.push(...preset)
else throw new Error(`riskTierPathTokens.${category}: unknown preset "${name}" (available: ${Object.keys(RISK_TOKEN_PRESETS).join(", ")})`)Throwing at startup is appropriate here — compilePathTokenResolver is called once during orchestration setup, not in a hot path. A misconfigured safety gate that silently becomes a no-op is worse than a startup crash.
There was a problem hiding this comment.
Fixed in 919bcc17f — compilePathTokenResolver now throws on an unknown preset:<name> before assembling the resolver:
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.
Rationale: the shipped preset list is small and stable, so an unknown name is almost certainly a typo — not an intentional forward reference. Fail at review-start means the misconfiguration is caught immediately, not discovered when a real high-risk PR slips through unchecked.
Regression test 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 in the same category (so a partially-broken resolver is never returned), and that preset:finops still resolves promotion correctly on mrt_cost_daily.sql.
| } | ||
| return out.join("\n") | ||
| } | ||
|
|
There was a problem hiding this comment.
Git diff header lines bypass the context-line guard. When changedLinesForScan receives a full unified diff (e.g. from a local git diff), lines like diff --git a/... b/... and index abc123..def456 don't start with +, -, or , so marker is "" and stripped = raw (the full header). The guard if (marker === " ") continue only skips context lines (space-prefixed), not empty-marker lines — so header lines are passed through to out.push(...) and into the risk-key regex scan.
In practice file.diff comes from the GitHub PR files API and doesn't include these headers (hunks start directly at @@), so the real-world impact is low. But the +++/---/@@ guard at line 257 already handles some diff headers — the inconsistency means future callers passing a raw git diff output would get unexpected scanner input. The fix is one character:
// Before:
if (marker === " ") continue
// After (skip anything that isn't an actual added/removed line):
if (marker !== "+" && marker !== "-") continueThere was a problem hiding this comment.
Valid — traced the code.
At line 228 marker is "+", "-", " ", or "" (fallback for anything else). At line 246 the guard only skips marker === " ", so a line with an empty marker — diff --git ..., index abc..def, Author: ..., Date: ... — falls through and gets pushed to out, then feeds dbtRiskYmlKeyMatches.
Real-world exposure right now is near-zero: file.diff from GitHub's PR files API arrives as raw hunks starting at @@, so this never hits in production. Would any DBT risk-key regex actually match a git-header line? Not by construction — the patterns require the exact word (data_tests/tests/constraints/contract) at YAML key position followed by :, and no header text collides. But the code was asymmetric: +++/---/@@ unified-diff headers were explicitly skipped at line 220, while other free-form lines weren't. A future caller passing raw git diff -p output would silently see those in the scanner.
Fixed in 9b2c024e1 — swapped the guard to if (marker !== "+" && marker !== "-") continue. Any line whose leading character isn't + or - is either a context line (fine, already updated scalar state above) or a free-form header (skipped, no emit). Added a regression test raw git diff -p free-form headers do NOT reach the scanner — builds a full raw-diff string (four header lines + hunk header + one changed line) and asserts the same tier verdict as the bare-tests-word test. 231/231 review-* tests pass.
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
…ns 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.
…y 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.
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
…ns 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.
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
…ns 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.
There was a problem hiding this comment.
2 issues 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/risk-tier.ts">
<violation number="1" location="packages/opencode/src/altimate/review/risk-tier.ts:134">
P2: Throwing on an unknown preset is a good safety fix, but nothing catches it between compilePathTokenResolver and the CLI handler, so a single config typo in `.altimate/review.yml` will crash every review run for that repo with a raw Node stack trace rather than a clean error message. Consider wrapping the runReview/reviewPullRequest call (or the config-load step) in a try/catch that surfaces this specific misconfiguration message cleanly and exits non-zero, since this validation now runs mid-pipeline (after the per-file PII/impact/complexity engine passes) rather than at config-load time.</violation>
</file>
<file name="packages/opencode/test/altimate/review.test.ts">
<violation number="1" location="packages/opencode/test/altimate/review.test.ts:820">
P3: The `try`/`catch` block calls `compilePathTokenResolver({ finops: ["preset:finop"] })` with identical arguments as the preceding `expect().toThrow()` — redundant assertion that invokes the throwing function a second time. The `toContain("finops")` check also passes from the category name alone (`riskTierPathTokens.finops:`) without exercising the "known presets" intent the comment describes. Combine into one assertion: `expect(() => compilePathTokenResolver({ finops: ["preset:finop"] })).toThrow(/Known presets: finops/)` covers both the known-list requirement and the throw behavior directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // loud instead so the misconfiguration is caught at review | ||
| // start, not discovered when a real high-risk PR slips through | ||
| // (cubic-review P2 + altimate-harness-bot on PR #1028). | ||
| if (!preset) { |
There was a problem hiding this comment.
P2: Throwing on an unknown preset is a good safety fix, but nothing catches it between compilePathTokenResolver and the CLI handler, so a single config typo in .altimate/review.yml will crash every review run for that repo with a raw Node stack trace rather than a clean error message. Consider wrapping the runReview/reviewPullRequest call (or the config-load step) in a try/catch that surfaces this specific misconfiguration message cleanly and exits non-zero, since this validation now runs mid-pipeline (after the per-file PII/impact/complexity engine passes) rather than at config-load time.
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/risk-tier.ts, line 134:
<comment>Throwing on an unknown preset is a good safety fix, but nothing catches it between compilePathTokenResolver and the CLI handler, so a single config typo in `.altimate/review.yml` will crash every review run for that repo with a raw Node stack trace rather than a clean error message. Consider wrapping the runReview/reviewPullRequest call (or the config-load step) in a try/catch that surfaces this specific misconfiguration message cleanly and exits non-zero, since this validation now runs mid-pipeline (after the per-file PII/impact/complexity engine passes) rather than at config-load time.</comment>
<file context>
@@ -123,7 +123,23 @@ export function compilePathTokenResolver(
+ // loud instead so the misconfiguration is caught at review
+ // start, not discovered when a real high-risk PR slips through
+ // (cubic-review P2 + altimate-harness-bot on PR #1028).
+ if (!preset) {
+ const known = Object.keys(RISK_TOKEN_PRESETS).sort().join(", ") || "(none shipped)"
+ throw new Error(
</file context>
There was a problem hiding this comment.
Fixed in 9435bc6f9 — orchestrate.ts now wraps the compilePathTokenResolver call in a try/catch, so a single config typo no longer crashes the run. On throw:
- Prominent warning to stderr with the resolver's error message +
Review continuing without path-token promotionfallback banner. - Fall back to
pathTokenCategoryOf = undefinedsoclassifyPRstill runs — no path-token promotion fires, but the rest of the review proceeds normally. - Prepend the config error to
tierResult.reasonsso it surfaces inenvelope.tierReasonswhenexplainTieris set — a reader who never sees stderr (envelope JSON, PR-comment renderer) still knows why their opt-in didn't fire.
Trade-off is deliberate: the user's opt-in is dead until they fix the typo (safe fallback — no silent auto-approve on billing/PCI paths), but the review itself doesn't die.
Regression test runReview does NOT crash on an unknown-preset config typo — degrades gracefully: builds a config with finops: [preset:finop], captures stderr, asserts (a) envelope is produced (no crash), (b) stderr contains the config-error message + fallback banner, (c) env.tierReasons carries the same error.
| // `preset:<name>` entry (e.g. `preset:finop` instead of `preset:finops`) | ||
| // must not silently no-op the category. Fail loud at resolver-build so | ||
| // the misconfiguration is caught before any diff is scanned. | ||
| expect(() => compilePathTokenResolver({ finops: ["preset:finop"] })).toThrow(/unknown preset 'finop'/) |
There was a problem hiding this comment.
P3: The try/catch block calls compilePathTokenResolver({ finops: ["preset:finop"] }) with identical arguments as the preceding expect().toThrow() — redundant assertion that invokes the throwing function a second time. The toContain("finops") check also passes from the category name alone (riskTierPathTokens.finops:) without exercising the "known presets" intent the comment describes. Combine into one assertion: expect(() => compilePathTokenResolver({ finops: ["preset:finop"] })).toThrow(/Known presets: finops/) covers both the known-list requirement and the throw behavior directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/review.test.ts, line 820:
<comment>The `try`/`catch` block calls `compilePathTokenResolver({ finops: ["preset:finop"] })` with identical arguments as the preceding `expect().toThrow()` — redundant assertion that invokes the throwing function a second time. The `toContain("finops")` check also passes from the category name alone (`riskTierPathTokens.finops:`) without exercising the "known presets" intent the comment describes. Combine into one assertion: `expect(() => compilePathTokenResolver({ finops: ["preset:finop"] })).toThrow(/Known presets: finops/)` covers both the known-list requirement and the throw behavior directly.</comment>
<file context>
@@ -812,6 +812,29 @@ describe("risk-tier", () => {
+ // `preset:<name>` entry (e.g. `preset:finop` instead of `preset:finops`)
+ // must not silently no-op the category. Fail loud at resolver-build so
+ // the misconfiguration is caught before any diff is scanned.
+ expect(() => compilePathTokenResolver({ finops: ["preset:finop"] })).toThrow(/unknown preset 'finop'/)
+ // The error message lists the known presets so the reader gets a
+ // fix pointer rather than "unknown, good luck".
</file context>
| compilePathTokenResolver({ finops: ["preset:finop"] }) | ||
| throw new Error("expected throw") | ||
| } catch (e: any) { | ||
| expect(String(e.message)).toContain("finops") |
There was a problem hiding this comment.
SUGGESTION: toContain("finops") doesn't lock in the known-presets list
The preceding comment says this assertion verifies "the error message lists the known presets", but "finops" also appears in riskTierPathTokens.finops (the category name) and is part of the fixed template regardless of whether the Known presets: list renders. So this assertion still passes if the known-list were dropped from the message entirely — it doesn't actually exercise the fix pointer. Assert on the substring that only exists when the list renders:
| expect(String(e.message)).toContain("finops") | |
| expect(String(e.message)).toContain("Known presets: finops") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "--- a/models/intermediate/int_x.yml\n" + | ||
| "+++ b/models/intermediate/int_x.yml\n" + | ||
| "@@ -1,3 +1,4 @@\n" + | ||
| " models:\n" + |
There was a problem hiding this comment.
Non-discriminating test — the headers contain no risk tokens, so this passes before and after the guard fix.
The diff --git a/.../int_x.yml b/.../int_x.yml and index abcdef1..1234567 lines have no data_tests:, constraints:, etc. — they produce trivial regardless of whether the empty-marker guard is present. The test validates correct behaviour for free-form-header content, but it doesn't regress if the guard is removed.
A discriminating version embeds a risk key in the free-form header and asserts the tier stays trivial:
const diff =
// free-form header with a risk key — must be swallowed by the guard
"data_tests: risk-key smuggled in a free-form header line\n" +
"@@ -1,3 +1,4 @@\n" +
" models:\n" +
" - name: mrt_x\n" +
'+ description: "no risk token in the actual diff"\n'
const r = classifyPR([file("models/intermediate/int_x.yml", diff)])
// Under the old guard (marker === " ") the data_tests: header reaches
// the scanner and returns "high"; under the new guard it is skipped.
expect(r.tier).toBe("trivial")Without this the guard fix has no automated safety net.
…trate — 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.
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
…ns 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.
…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
…motion 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
… 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
…ForScan (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
…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.
…s (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.
…y 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.
…rPathTokens (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.
…trate — 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.
9435bc6 to
9bdd6aa
Compare
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/orchestrate.ts`:
- Around line 1127-1129: Update the envelope construction in the flow around
tierResult and buildEnvelope so pathTokenConfigError is included in tierReasons
regardless of explainTier or tierForced. Preserve the existing reason ordering
by prepending the error, and ensure comment/gate JSON output carries the
configuration error instead of only logging it to stderr.
🪄 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 Plus
Run ID: a5b479aa-9bae-4692-bd05-250161b3721c
📒 Files selected for processing (4)
packages/opencode/src/altimate/review/config.tspackages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/risk-tier.tspackages/opencode/test/altimate/review.test.ts
…ithout --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).
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
reviewing as requested
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
…ns 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.
…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>
3 participants
Summary
Adds triage-tier promotion rules to
risk-tier.tsso the reviewer never auto-approves risk-bearing dbt metadata changes. Two failure modes closed:models/marts/addingdata_tests:/constraints:/unique_combination_of_columnson a contracted mart → previously classifiedtrivial→ auto-approved.lite→ approved without the deeper detector lanes running.Three new
FileChangeClasssignals wire intofullTierReasons():dbtRiskYmlChangesdata_tests:/constraints:/contract:/unique_combination_of_columnslist itemmartLayerChangemodels/marts/ormodels/mart/. Description-only edits under marts stay trivial (matches existing behavior).finopsPathTokencost|saving|billing|credit|dbu|spend|revenue|price|rate|pricing|invoiceat word/segment/extension boundary. FP guard on incidental substrings (broadcaster≠caste).Test plan
packages/opencode/test/altimate/review.test.ts— all green.Ship criteria
Depends on
Stacks on top of #1027 (
feat/review-r18-observability-recall). This branch uses thetierReasons[]wiring landed there so promotion reasons surface in the signed envelope. Merge #1027 first, then rebase this onto main.Summary by CodeRabbit
riskTierPathTokens) to promote matching changes to the full review risk tier.preset:<name>expansion.schema.ymlby detecting specific high-risk YAML keys and model-layer context.