chore(deps): Bump glob from 13.0.5 to 13.0.6 - #8
Closed
dependabot[bot] wants to merge 1 commit into
Closed
Conversation
Bumps [glob](https://github.com/isaacs/node-glob) from 13.0.5 to 13.0.6. - [Changelog](https://github.com/isaacs/node-glob/blob/main/changelog.md) - [Commits](isaacs/node-glob@v13.0.5...v13.0.6) --- updated-dependencies: - dependency-name: glob dependency-version: 13.0.6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
dependabot
Bot
added
dependencies
Contributor
Author
|
OK, I won't notify you again about this release, but will get in touch when a new version is available. If you'd rather skip all updates until the next major or minor version, let me know by commenting If you change your mind, just re-open this PR and I'll resolve any conflicts on it. |
This was referenced Mar 5, 2026
16 tasks
5 tasks
anandgupta42
added a commit
that referenced
this pull request
Mar 22, 2026
- Track loops by `(tool, inputHash)` not just tool name (#2) - Use "Failed after" narrative for error traces (#3) - Add keyboard accessibility to viewer tabs (role, tabindex, Enter/Space) (#4) - Use full command as dedup key, not `slice(0,60)` (#5) - Sort timeline events by time before rendering (#6) - Pass `tracesDir` to footer text in `listRecaps` (#7) - Increase `MAX_RECAPS` to 100, add eviction warning log (#8) - Resolve assistant `parentID` for recap enrichment (#9) - Remove unused `tracer` variable in test (#10) - Clarify `--no-trace` backward-compat flag in docs (#1) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
anandgupta42
added a commit
that referenced
this pull request
Mar 23, 2026
…381) * feat: rename tracer to recap with loop detection, post-session summary, and enhanced viewer - Rename `Tracer` class to `Recap` with backward-compat aliases - Rename CLI command `trace` to `recap` (hidden `trace` alias preserved) - Add loop detection: flags repeated tool calls with same input (3+ in last 10) - Add post-session summary: `narrative`, `topTools`, `loops` in trace output - New Summary tab (default) in HTML viewer with: - Truncated prompt with expand toggle - Files changed with SQL diff previews - Tool-agnostic outcome extraction (dbt, pytest, Airflow, pip, SQL) - Deduped dbt commands with pass/fail status, clickable to waterfall - Smart command grouping (boring ls/cd collapsed, meaningful shown) - Error details with resolution tracking - Cost breakdown in collapsible section - Virality: Share Recap (self-contained HTML download), Copy Summary (markdown), Copy Link, branded footer - Fix XSS: timeline items escaped with `e()` - Fix memory leak: per-session `sessionUserMsgIds` with cleanup on eviction - Fix JS syntax: onclick quote escaping in collapsible section - Bound `toolCallHistory` to prevent unbounded growth (cap at 200) - Summary view wrapped in try-catch for visible error messages - Update all 13 test files for rename + 8 new adversarial viewer tests - Update docs: `tracing.md` → `recap.md`, CLI/TUI references updated Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: share/copy buttons scoping bug + `t.text` undefined + adversarial viewer tests - Fix critical bug: Share Recap and Copy Summary buttons referenced variables from Summary IIFE scope — rewrote `buildMarkdownSummary` to be self-contained - Fix `t.text` → `t.result` in narrative (was rendering "undefined") - Fix `sessionUserMsgIds` not cleaned on MAX_RECAPS eviction (memory leak) - Fix zero cost display: show `$0.00` instead of em-dash - Add try-catch error boundary around Summary view rendering - Add 8 adversarial viewer tests: XSS, NaN/Infinity, null metadata, 200+ spans, JS syntax validation, tool-agnostic outcomes, backward compat Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address all 10 CodeRabbit review comments - Track loops by `(tool, inputHash)` not just tool name (#2) - Use "Failed after" narrative for error traces (#3) - Add keyboard accessibility to viewer tabs (role, tabindex, Enter/Space) (#4) - Use full command as dedup key, not `slice(0,60)` (#5) - Sort timeline events by time before rendering (#6) - Pass `tracesDir` to footer text in `listRecaps` (#7) - Increase `MAX_RECAPS` to 100, add eviction warning log (#8) - Resolve assistant `parentID` for recap enrichment (#9) - Remove unused `tracer` variable in test (#10) - Clarify `--no-trace` backward-compat flag in docs (#1) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add screenshots and update recap viewer documentation - Add Summary tab and full-page screenshots to docs - Update viewer section with 5-tab description - Detail what Summary tab shows: files changed, outcomes, timeline, cost - Add screenshot at top of recap.md for quick visual reference Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: move Recap to Use section, Telemetry to Reference - Move Recap from Configure > Observability to Use (peer to Commands, Skills) - Move Telemetry from Configure > Observability to Reference (internal analytics) - Remove the Observability section entirely Recap is a feature users interact with after sessions, not a config setting. Telemetry is internal product analytics, not user-facing observability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: viewer UX improvements from 100-trace analysis - Collapse Files Changed after 5 entries with "Show all N files" toggle - Rename "GENS" → "LLM Calls" in header cards - Hide Tokens card when cost is $0 (not actionable without cost context) - Hide Cost metric card when $0.00 (wasted space) - Add prominent error summary banner right after header metrics - Improved dbt outcome detection: catch [PASS], [ERROR], N of M, Compilation Error - Outcome detection rate improved from 18% → 33% across 100 real traces - Updated doc screenshots with cleaner samples Tested across 100 real production traces: 0 crashes, 0 JS errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: always show Cost and Tokens cards $0.00 is a valid cost (Anthropic Max plan). Hiding it implies we don't support cost tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: tool-agnostic outcome extraction for schema, validation, SQL, lineage tools 500-trace analysis revealed: - Schema tasks: 0% outcome visibility → 100% - Validation tasks: 0% outcome visibility → 100% - SQL tasks: 55% outcome visibility → 100% Added outcome extraction for: - schema_inspect, lineage_check, altimate_core_validate results - SQL error messages (not just row counts) - Improved empty session display (shows prompt if available) Tested across 500 diverse synthetic traces (SQL, Airflow, Dagster, Python, schema, validation, migration, connectors) + 100 real traces. 0 crashes, 0 JS errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address 4 new CodeRabbit review comments - Add `inputHash` to `TraceFile.summary.loops` schema type (#11) - Replace `startTrace()` API name with plain language in docs (#12) - Use `CSS.escape()` for spanId in querySelector to handle special chars (#13) - Sort spans by startTime before searching for error resolution (#14) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: round 3 review — sort spans once, clean narrative for 0 LLM calls - Sort spans once before error resolution loop instead of per-error (perf) - Narrative omits "Made 0 LLM calls" for tool-only sessions (UX) - Updated tests to match new narrative format Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add missing `altimate_change` markers for recap rename in upstream-shared files Wrap renamed code (Tracer→Recap, trace→recap) with markers so the Marker Guard CI check passes. The diff-based checker uses -U5 context windows per hunk — markers must be close enough to added lines to appear within each hunk's context. Files fixed: - `trace.ts` — handler body, option descriptions, viewer message, compat alias - `app.tsx` — recapViewerServer return, openRecapInBrowser function - `dialog-trace-list.tsx` — error title, Recaps title, compat alias - `worker.ts` — getOrCreateRecap, part events, session title/finalization - `index.ts` — .command(RecapCommand) registration Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add altimate_change markers to all upstream-shared files Marker Guard CI was failing — 5 upstream-shared files had custom code (recap rename) without altimate_change markers. Fixed: trace.ts, app.tsx, dialog-trace-list.tsx, worker.ts, index.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: type errors in training-import.test.ts from main merge Pre-existing type issues from main: mock missing `context`/`rule` fields and readFile return type mismatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
13 tasks
kulvirgit
pushed a commit
that referenced
this pull request
Mar 30, 2026
…381) * feat: rename tracer to recap with loop detection, post-session summary, and enhanced viewer - Rename `Tracer` class to `Recap` with backward-compat aliases - Rename CLI command `trace` to `recap` (hidden `trace` alias preserved) - Add loop detection: flags repeated tool calls with same input (3+ in last 10) - Add post-session summary: `narrative`, `topTools`, `loops` in trace output - New Summary tab (default) in HTML viewer with: - Truncated prompt with expand toggle - Files changed with SQL diff previews - Tool-agnostic outcome extraction (dbt, pytest, Airflow, pip, SQL) - Deduped dbt commands with pass/fail status, clickable to waterfall - Smart command grouping (boring ls/cd collapsed, meaningful shown) - Error details with resolution tracking - Cost breakdown in collapsible section - Virality: Share Recap (self-contained HTML download), Copy Summary (markdown), Copy Link, branded footer - Fix XSS: timeline items escaped with `e()` - Fix memory leak: per-session `sessionUserMsgIds` with cleanup on eviction - Fix JS syntax: onclick quote escaping in collapsible section - Bound `toolCallHistory` to prevent unbounded growth (cap at 200) - Summary view wrapped in try-catch for visible error messages - Update all 13 test files for rename + 8 new adversarial viewer tests - Update docs: `tracing.md` → `recap.md`, CLI/TUI references updated Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: share/copy buttons scoping bug + `t.text` undefined + adversarial viewer tests - Fix critical bug: Share Recap and Copy Summary buttons referenced variables from Summary IIFE scope — rewrote `buildMarkdownSummary` to be self-contained - Fix `t.text` → `t.result` in narrative (was rendering "undefined") - Fix `sessionUserMsgIds` not cleaned on MAX_RECAPS eviction (memory leak) - Fix zero cost display: show `$0.00` instead of em-dash - Add try-catch error boundary around Summary view rendering - Add 8 adversarial viewer tests: XSS, NaN/Infinity, null metadata, 200+ spans, JS syntax validation, tool-agnostic outcomes, backward compat Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address all 10 CodeRabbit review comments - Track loops by `(tool, inputHash)` not just tool name (#2) - Use "Failed after" narrative for error traces (#3) - Add keyboard accessibility to viewer tabs (role, tabindex, Enter/Space) (#4) - Use full command as dedup key, not `slice(0,60)` (#5) - Sort timeline events by time before rendering (#6) - Pass `tracesDir` to footer text in `listRecaps` (#7) - Increase `MAX_RECAPS` to 100, add eviction warning log (#8) - Resolve assistant `parentID` for recap enrichment (#9) - Remove unused `tracer` variable in test (#10) - Clarify `--no-trace` backward-compat flag in docs (#1) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add screenshots and update recap viewer documentation - Add Summary tab and full-page screenshots to docs - Update viewer section with 5-tab description - Detail what Summary tab shows: files changed, outcomes, timeline, cost - Add screenshot at top of recap.md for quick visual reference Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: move Recap to Use section, Telemetry to Reference - Move Recap from Configure > Observability to Use (peer to Commands, Skills) - Move Telemetry from Configure > Observability to Reference (internal analytics) - Remove the Observability section entirely Recap is a feature users interact with after sessions, not a config setting. Telemetry is internal product analytics, not user-facing observability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: viewer UX improvements from 100-trace analysis - Collapse Files Changed after 5 entries with "Show all N files" toggle - Rename "GENS" → "LLM Calls" in header cards - Hide Tokens card when cost is $0 (not actionable without cost context) - Hide Cost metric card when $0.00 (wasted space) - Add prominent error summary banner right after header metrics - Improved dbt outcome detection: catch [PASS], [ERROR], N of M, Compilation Error - Outcome detection rate improved from 18% → 33% across 100 real traces - Updated doc screenshots with cleaner samples Tested across 100 real production traces: 0 crashes, 0 JS errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: always show Cost and Tokens cards $0.00 is a valid cost (Anthropic Max plan). Hiding it implies we don't support cost tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: tool-agnostic outcome extraction for schema, validation, SQL, lineage tools 500-trace analysis revealed: - Schema tasks: 0% outcome visibility → 100% - Validation tasks: 0% outcome visibility → 100% - SQL tasks: 55% outcome visibility → 100% Added outcome extraction for: - schema_inspect, lineage_check, altimate_core_validate results - SQL error messages (not just row counts) - Improved empty session display (shows prompt if available) Tested across 500 diverse synthetic traces (SQL, Airflow, Dagster, Python, schema, validation, migration, connectors) + 100 real traces. 0 crashes, 0 JS errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address 4 new CodeRabbit review comments - Add `inputHash` to `TraceFile.summary.loops` schema type (#11) - Replace `startTrace()` API name with plain language in docs (#12) - Use `CSS.escape()` for spanId in querySelector to handle special chars (#13) - Sort spans by startTime before searching for error resolution (#14) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: round 3 review — sort spans once, clean narrative for 0 LLM calls - Sort spans once before error resolution loop instead of per-error (perf) - Narrative omits "Made 0 LLM calls" for tool-only sessions (UX) - Updated tests to match new narrative format Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add missing `altimate_change` markers for recap rename in upstream-shared files Wrap renamed code (Tracer→Recap, trace→recap) with markers so the Marker Guard CI check passes. The diff-based checker uses -U5 context windows per hunk — markers must be close enough to added lines to appear within each hunk's context. Files fixed: - `trace.ts` — handler body, option descriptions, viewer message, compat alias - `app.tsx` — recapViewerServer return, openRecapInBrowser function - `dialog-trace-list.tsx` — error title, Recaps title, compat alias - `worker.ts` — getOrCreateRecap, part events, session title/finalization - `index.ts` — .command(RecapCommand) registration Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add altimate_change markers to all upstream-shared files Marker Guard CI was failing — 5 upstream-shared files had custom code (recap rename) without altimate_change markers. Fixed: trace.ts, app.tsx, dialog-trace-list.tsx, worker.ts, index.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: type errors in training-import.test.ts from main merge Pre-existing type issues from main: mock missing `context`/`rule` fields and readFile return type mismatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merged
11 tasks
suryaiyer95
added a commit
that referenced
this pull request
May 13, 2026
Audited the v9 skill content and stripped phrasings that were shaped by ADE-Bench observations but would harm real-world use: 1. "Never blame the data or the test" Iron Rules (troubleshoot #8, develop #13) softened from "the grader's data is the spec" to "investigate before concluding the data is wrong." Real data CAN be corrupt; the rule now requires evidence (upstream bug, constraint violation, documented anomaly) before that conclusion, rather than treating all data as inerrant. 2. "Turn 1 is TodoWrite, every time" -> "For non-trivial tasks, plan with TodoWrite before acting." Skip TodoWrite for one-shot fixes (single edit, single grep) -- no point listing one item in a todo for a literal one-step task. 3. Removed specific frozen row counts ("6-row discrepancy on an 11000-row model") -- those numbers came from one specific benchmark trial. Replaced with generic "small absolute discrepancies on large models are still real bugs." 4. Removed verbatim ADE-Bench prompt phrase "even if the prompt only says 'the project is broken'" from P1 trigger. Replaced with generic "even when the prompt only describes a different symptom or is generic." 5. P4-extra "(and the grader)" / "the grader's expected column set" -> neutral framings about downstream consumers, tests, and BI dashboards. 6. anthropic.txt: same softening on "Never blame the data or the test" + "Plan first - TodoWrite" sections. Kept intentionally (per user direction): - "Decide and act - never pause to ask the user" (Iron Rule 5 troubleshoot, Iron Rule 9 develop, autonomous-operation in anthropic.txt). This is the autonomous-agent contract; the permission system handles destructive-op confirmation separately. All Pattern Catalog recipes (P1-P6) and the Step 5d hard-stop checklist remain -- those are general dbt engineering practice.
6 tasks
6 tasks
6 tasks
sahrizvi
pushed a commit
that referenced
this pull request
Jul 22, 2026
…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
sahrizvi
pushed a commit
that referenced
this pull request
Jul 22, 2026
…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
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
…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
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
…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
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
…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
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
…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
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
…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
sahrizvi
pushed a commit
that referenced
this pull request
Jul 23, 2026
…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
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
…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
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>
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
1 participant
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.
Bumps glob from 13.0.5 to 13.0.6.
Commits
e80cb3813.0.69cdbbffrevert tsgo, not ready for test coverage correctness yet89c99bause tsgo compilerb7275d5update deps, expand engines to include node 18942e360update workflows, pull taprc out of package.json4a0d53cupdate tap for mockImport bugfixef94ad2update tap180c2d4update docs37993c8remove stray console.error in testDependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)