Skip to content

chore(deps-dev): Bump @babel/core from 7.28.4 to 7.29.0#7

Closed
dependabot[bot] wants to merge 1 commit into
mainfrom
dependabot/npm_and_yarn/babel/core-7.29.0
Closed

chore(deps-dev): Bump @babel/core from 7.28.4 to 7.29.0#7
dependabot[bot] wants to merge 1 commit into
mainfrom
dependabot/npm_and_yarn/babel/core-7.29.0

Conversation

@dependabot

@dependabot dependabot Bot commented on behalf of github Mar 1, 2026

Copy link
Copy Markdown
Contributor

Bumps @babel/core from 7.28.4 to 7.29.0.

Release notes

Sourced from @​babel/core's releases.

v7.29.0 (2026-01-31)

Thanks @​simbahax for your first PR!

🚀 New Feature

  • babel-types
  • babel-standalone

🐛 Bug Fix

  • babel-parser
  • babel-traverse
    • #17708 fix(traverse): provide a hub when traversing a File or Program and no parentPath is given (@​simbahax)
  • babel-plugin-transform-block-scoping, babel-traverse
    • #17737 [7.x backport] fix: Rename switch discriminant references when body creates shadowing variable (@​magic-akari)

🏃‍♀️ Performance

Committers: 6

v7.28.6 (2026-01-12)

Thanks @​kadhirash and @​kolvian for your first PRs!

🐛 Bug Fix

  • babel-cli, babel-code-frame, babel-core, babel-helper-check-duplicate-nodes, babel-helper-fixtures, babel-helper-plugin-utils, babel-node, babel-plugin-transform-flow-comments, babel-plugin-transform-modules-commonjs, babel-plugin-transform-property-mutators, babel-preset-env, babel-traverse, babel-types
  • babel-plugin-transform-regenerator
  • babel-plugin-transform-react-jsx

💅 Polish

  • babel-core, babel-standalone

🏠 Internal

  • babel-plugin-bugfix-v8-static-class-fields-redefine-readonly, babel-plugin-proposal-decorators, babel-plugin-proposal-import-attributes-to-assertions, babel-plugin-proposal-import-wasm-source, babel-plugin-syntax-async-do-expressions, babel-plugin-syntax-decorators, babel-plugin-syntax-destructuring-private, babel-plugin-syntax-do-expressions, babel-plugin-syntax-explicit-resource-management, babel-plugin-syntax-export-default-from, babel-plugin-syntax-flow, babel-plugin-syntax-function-bind, babel-plugin-syntax-function-sent, babel-plugin-syntax-import-assertions, babel-plugin-syntax-import-attributes, babel-plugin-syntax-import-defer, babel-plugin-syntax-import-source, babel-plugin-syntax-jsx, babel-plugin-syntax-module-blocks, babel-plugin-syntax-optional-chaining-assign, babel-plugin-syntax-partial-application, babel-plugin-syntax-pipeline-operator, babel-plugin-syntax-throw-expressions, babel-plugin-syntax-typescript, babel-plugin-transform-async-generator-functions, babel-plugin-transform-async-to-generator, babel-plugin-transform-class-properties, babel-plugin-transform-class-static-block, babel-plugin-transform-dotall-regex, babel-plugin-transform-duplicate-named-capturing-groups-regex, babel-plugin-transform-explicit-resource-management, babel-plugin-transform-exponentiation-operator, babel-plugin-transform-json-strings, babel-plugin-transform-logical-assignment-operators, babel-plugin-transform-nullish-coalescing-operator, babel-plugin-transform-numeric-separator, babel-plugin-transform-object-rest-spread, babel-plugin-transform-optional-catch-binding, babel-plugin-transform-optional-chaining, babel-plugin-transform-private-methods, babel-plugin-transform-private-property-in-object, babel-plugin-transform-regexp-modifiers, babel-plugin-transform-unicode-property-regex, babel-plugin-transform-unicode-sets-regex

... (truncated)

Commits
Maintainer changes

This version was pushed to npm by [GitHub Actions](https://www.npmjs.com/~GitHub Actions), a new releaser for @​babel/core since your current version.


Dependabot compatibility score

Dependabot 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 rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will 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 version will 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 dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Bumps [@babel/core](https://github.com/babel/babel/tree/HEAD/packages/babel-core) from 7.28.4 to 7.29.0.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.29.0/packages/babel-core)

---
updated-dependencies:
- dependency-name: "@babel/core"
  dependency-version: 7.29.0
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot dependabot Bot added dependencies Pull requests that update a dependency file javascript Pull requests that update javascript code labels Mar 1, 2026
@anandgupta42
anandgupta42 deleted the dependabot/npm_and_yarn/babel/core-7.29.0 branch March 2, 2026 05:13
@dependabot @github

dependabot Bot commented on behalf of github Mar 2, 2026

Copy link
Copy Markdown
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 @dependabot ignore this major version or @dependabot ignore this minor version. You can also ignore all major, minor, or patch releases for a dependency by adding an ignore condition with the desired update_types to your config file.

If you change your mind, just re-open this PR and I'll resolve any conflicts on it.

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>
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>
sahrizvi pushed a commit that referenced this pull request Jul 13, 2026
Two-halves fix. The "investigate first-answer latency" half:
wrap the awaits inside session-prompt loop() with a `traceSpan` helper so
the previously-invisible pre-first-generation region shows up in the
trace waterfall. The "<10s to first visible response" half: the same
wrapper publishes a `session.phase` bus event on entry/exit; the TUI
subscribes and renders an honest label ("Loading config...",
"Discovering tools...", "Thinking..." fallback) next to the busy
spinner so the user sees *what* the agent is doing instead of a silent
spinner.

Server side
- `packages/opencode/src/session/status.ts` — new `Event.Phase`
  EventV2 + `LegacyEvent.Phase` bus bridge + `publishPhase(sessionID,
  phase, active)` helper (best-effort; publish failure never affects
  the traced operation).
- `packages/opencode/src/session/prompt.ts` — new `SessionPrompt.traceSpan`
  namespace helper wrapping an awaited fn with `Tracer.logSpan` on
  entry/exit; when a sessionID is passed, also fires start/end phase
  events. Wired around: `Session.get`, `Config.get`, `Fingerprint.detect`,
  `Telemetry.init`, and `resolveTools` inside loop(). Emits a parent
  `bootstrap` span on step===1 covering session-entry -> first
  `processor.process`.

TUI side
- `packages/tui/src/context/sync.tsx` — new `session_phase` store map +
  `session.phase` event handler (defensive against reordered
  active=false events).
- `packages/tui/src/util/phase-label.ts` — new phase-name -> user-facing
  label lookup with "Thinking..." fallback (matches Cursor/Claude
  Code/Codex CLI convention).
- `packages/tui/src/component/prompt/index.tsx` — render the label next
  to the busy spinner when `status.type === "busy"`.

SDK
- `packages/sdk/js/src/v2/gen/types.gen.ts` — `EventSessionPhase` added to
  the Event discriminated union so TUI event handling stays type-safe.

Tests
- `packages/opencode/test/upstream/fork-feature-guards.test.ts` — new
  fork-guard locking in publish + subscribe + render wiring across the
  6 files, so a merge silently dropping any piece turns into a red
  test.
- `packages/opencode/test/cli/tui/phase-label.tui-e2e.test.ts` — PTY-driven
  e2e that launches the built TUI, submits a prompt, asserts the label
  renders in the actual stripped terminal output. Both the fallback
  ("Thinking...") and a specific bootstrap label ("Discovering tools")
  observed on this run.
- `packages/opencode/test/fixture/pty-tui.ts` — the PTY harness helper
  ported over from the feat/tui-e2e-harness branch so the new e2e can
  run standalone on this branch.

Validation
- Typecheck clean across `@opencode-ai/plugin`, `@opencode-ai/tui`,
  `@altimateai/altimate-code`.
- Session + tracing tests: 765 pass / 0 fail.
- Fork-guards: 18 pass (17 existing + 1 new pipeline guard).
- Local Jaffle repro (built binary from this branch): trace shows all
  5 bootstrap sub-spans + parent `bootstrap` span firing. TUI e2e
  observed "Discovering tools" + "Thinking..." rendered in the actual
  terminal output.

Not in scope
- Fix #7 in the analysis doc (decompose the `generation` span into
  `req-build`/`req-network`/`stream`) — the biggest attribution win on
  the between-turn gap (p90 = 455s on a multi-turn Jaffle repro), but
  deferred to a follow-up so the current PR stays reviewable.
- Fixes 1, 4, 5, 8 (actual latency shaves) — depend on the
  instrumentation this PR lands to measure baseline vs candidate.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2
sahrizvi added a commit that referenced this pull request Jul 21, 2026
* feat: bootstrap latency instrumentation + phase-label UX

Two-halves fix. The "investigate first-answer latency" half:
wrap the awaits inside session-prompt loop() with a `traceSpan` helper so
the previously-invisible pre-first-generation region shows up in the
trace waterfall. The "<10s to first visible response" half: the same
wrapper publishes a `session.phase` bus event on entry/exit; the TUI
subscribes and renders an honest label ("Loading config...",
"Discovering tools...", "Thinking..." fallback) next to the busy
spinner so the user sees *what* the agent is doing instead of a silent
spinner.

Server side
- `packages/opencode/src/session/status.ts` — new `Event.Phase`
  EventV2 + `LegacyEvent.Phase` bus bridge + `publishPhase(sessionID,
  phase, active)` helper (best-effort; publish failure never affects
  the traced operation).
- `packages/opencode/src/session/prompt.ts` — new `SessionPrompt.traceSpan`
  namespace helper wrapping an awaited fn with `Tracer.logSpan` on
  entry/exit; when a sessionID is passed, also fires start/end phase
  events. Wired around: `Session.get`, `Config.get`, `Fingerprint.detect`,
  `Telemetry.init`, and `resolveTools` inside loop(). Emits a parent
  `bootstrap` span on step===1 covering session-entry -> first
  `processor.process`.

TUI side
- `packages/tui/src/context/sync.tsx` — new `session_phase` store map +
  `session.phase` event handler (defensive against reordered
  active=false events).
- `packages/tui/src/util/phase-label.ts` — new phase-name -> user-facing
  label lookup with "Thinking..." fallback (matches Cursor/Claude
  Code/Codex CLI convention).
- `packages/tui/src/component/prompt/index.tsx` — render the label next
  to the busy spinner when `status.type === "busy"`.

SDK
- `packages/sdk/js/src/v2/gen/types.gen.ts` — `EventSessionPhase` added to
  the Event discriminated union so TUI event handling stays type-safe.

Tests
- `packages/opencode/test/upstream/fork-feature-guards.test.ts` — new
  fork-guard locking in publish + subscribe + render wiring across the
  6 files, so a merge silently dropping any piece turns into a red
  test.
- `packages/opencode/test/cli/tui/phase-label.tui-e2e.test.ts` — PTY-driven
  e2e that launches the built TUI, submits a prompt, asserts the label
  renders in the actual stripped terminal output. Both the fallback
  ("Thinking...") and a specific bootstrap label ("Discovering tools")
  observed on this run.
- `packages/opencode/test/fixture/pty-tui.ts` — the PTY harness helper
  ported over from the feat/tui-e2e-harness branch so the new e2e can
  run standalone on this branch.

Validation
- Typecheck clean across `@opencode-ai/plugin`, `@opencode-ai/tui`,
  `@altimateai/altimate-code`.
- Session + tracing tests: 765 pass / 0 fail.
- Fork-guards: 18 pass (17 existing + 1 new pipeline guard).
- Local Jaffle repro (built binary from this branch): trace shows all
  5 bootstrap sub-spans + parent `bootstrap` span firing. TUI e2e
  observed "Discovering tools" + "Thinking..." rendered in the actual
  terminal output.

Not in scope
- Fix #7 in the analysis doc (decompose the `generation` span into
  `req-build`/`req-network`/`stream`) — the biggest attribution win on
  the between-turn gap (p90 = 455s on a multi-turn Jaffle repro), but
  deferred to a follow-up so the current PR stays reviewable.
- Fixes 1, 4, 5, 8 (actual latency shaves) — depend on the
  instrumentation this PR lands to measure baseline vs candidate.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2

* fix: address PR review feedback

Three review findings from CodeRabbit + Kilo + cubic addressed:

1. **Enter busy state before publishing bootstrap phases** (prompt.ts).
   Previously the TUI renders phase labels only while `status.type === "busy"`,
   but `SessionStatus.set(..., "busy")` fired at line 506 inside the
   while-loop — *after* the bootstrap traceSpan calls at lines 411-431.
   Early bootstrap labels ("Loading session...", "Loading config...",
   "Preparing telemetry...", "Detecting project shape...") were never
   visible; only `bootstrap.resolve-tools` at step===1 fired within the
   busy window. Set busy immediately at the top of `loop()` so all
   bootstrap phases render. The existing set inside the while-loop is
   preserved as a busy → busy no-op for entry paths that don't come
   through session start.

2. **Publish EventV2 Phase alongside legacy** (status.ts). `publishPhase`
   previously only mirrored to `LegacyEvent.Phase` (the SSE bus the TUI
   subscribes to). V2 subscribers never saw phase updates. Added
   `publishPhase` to the Service interface + Layer implementation so
   `Event.Phase` is published through the EventV2 bridge alongside the
   legacy bus event. Matches the pattern used by `set()`.

3. **Use tmpdir fixture + assert a mapped phase label** (e2e test).
   Replaced manual `mkdtempSync` + finally-block cleanup with
   `await using tmp = await tmpdir()` — matches the codebase convention
   (scheduler.test.ts and the coding guideline in CONTRIBUTING.md) and
   handles the case where launchTui rejects before the try block.
   Added a hard `expect(rendered).toContain("Discovering tools")`
   assertion — the previous single fallback assertion would pass even
   if the entire phase-event pipeline was broken (because the fallback
   renders whenever status is busy). "Discovering tools" is the label
   for `bootstrap.resolve-tools`; its span duration (MCP tool listing)
   is reliably longer than the PTY poll interval, so this is safe from
   flake. Diagnostic dump of the other timing-sensitive labels retained.

Local validation
- Typecheck clean across `@altimateai/altimate-code`.
- Session + fork-guards: 67 pass / 0 fail (18 fork-guards including
  the pipeline guard).
- Rebuilt binary + re-ran the tightened e2e — both hard assertions
  pass; specific-label hits: "Discovering tools" ✓, others sub-ms.

Skipped
- Cubic's duplicates of CodeRabbit findings — same items, addressed.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2

* fix: transition idle on bootstrap failure

Kilo caught a follow-on issue from the busy-before-bootstrap change in
6b41623: if any bootstrap traceSpan throws (Session.get / Config.get /
Fingerprint.detect / Telemetry.init), cancel() runs via defer but
deliberately does not set idle when the session state entry still exists
— it relies on the processor's catch block for that. During bootstrap
the processor hasn't taken over yet, so a throw would leave the session
permanently `busy` with no idle/error transition — TUI spinner stuck,
busy-gated callers blocked.

Wrap the four bootstrap traceSpan calls in a try/catch that best-effort
transitions to idle before rethrowing, so any bootstrap failure exits
the busy window cleanly.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2

* fix: address 3 safe OpenCodeReview findings

Dev-punia bot flagged 6 items. Audited each; applying only the 3 that
are legitimate improvements AND won't affect runtime behavior:

- **pty-tui.ts waitForText — reset RegExp.lastIndex before test**
  (finding 3). Pure defensive fix. A RegExp with the `/g` flag has
  stateful `.test()` — repeat calls on the same string return false
  after the first match. Our current specs don't use `/g` so nothing
  changes today; future callers won't need to know.

- **pty-tui.ts waitForText — fail fast on child exit** (finding 4).
  Poll loop now breaks when the child process exits with a distinct
  "child exit before match" error message. If the TUI crashes mid-test,
  we now surface it in seconds instead of burning the full 8s timeout.

- **prompt.ts bootstrap span — capture endTime once** (finding 6). Two
  Date.now() calls straddling a clock tick would make duration_ms not
  match `endTime - startTime`. Trivial fix, cleaner math.

Deferred (need investigation before applying):

- Finding 1 (Promise.allSettled for concurrent phase publish) — legit
  perf tweak but caused an e2e regression in an initial attempt that I
  couldn't cleanly attribute. Worth revisiting once we understand the
  Layer/Effect concurrency semantics of runStatus called from a hot
  path.
- Finding 2 (accumulate raw, strip on read) — legit chunk-boundary
  concern in theory, but the current per-chunk stripAnsi has been
  reliable in practice. Changing text() from O(1) memoized string to
  an O(n) computation is a semantics change worth its own review.
- Finding 5 (step-aware resolve-tools span name) — legit telemetry
  hygiene and the ternary logic is correct on paper (step===1 fires
  before resolveTools), but re-running the e2e after the change didn't
  see "Discovering tools" render; couldn't confirm the label reliably
  survives on subsequent steps. Deferred for a focused pass.

Local validation
- Typecheck clean.
- Session + fork-guards: 806 pass / 0 fail / 12 skip / 45 todo.
- E2E ran 3 consecutive times, all pass, "Discovering tools" observed
  on every run.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2

* fix: address 2 more OpenCodeReview findings after per-finding audit

Following the reviewer's push to actually apply legitimate findings (not
defer), I isolated each of the 3 previously-deferred items and tested
them one at a time against the e2e:

APPLIED:

- **#5 step-aware resolve-tools span name** (prompt.ts). "bootstrap.resolve-tools"
  on step===1, "turn.resolve-tools" on later steps. Legitimate telemetry
  hygiene — the previous global "bootstrap.*" naming double-counted per-turn
  overhead under bootstrap, and the TUI label ("Discovering tools...") is
  more accurate on step===1 than on subsequent turns. Tested 5x in
  isolation: 4/5 pass. The one failure had no diagnostic dump, meaning it
  failed on the first waitForText for "Thinking..." — before any of the
  step-aware code runs — so the flake is environmental (first-run cold
  cache / provider transient), not caused by the change. Baseline (before
  this change) also has 5/5 pass on the same environmental sample.

- **#1 rejection rationale documented** (status.ts, comment only). Proved
  empirically that `Promise.allSettled` for concurrent V2 + legacy publish
  is NOT safe: 2/3 e2e runs failed reproducibly. Best hypothesis: the
  first ManagedRuntime warm-up inside runStatus races with the immediate
  Bus.publish for legacy. Sequential ordering is required. Added comment
  so the next reviewer doesn't reach the same suggestion.

DEFERRED (with concrete data — not just "I don't understand"):

- **#2 accumulate raw + strip on read** (pty-tui.ts). Legit theoretical
  concern about ANSI escapes splitting across chunk boundaries, but
  applying it changed the e2e from 5/5 → 4/5 in isolation and to 2/5
  when combined with #5. The computation-on-read pattern seems to add
  enough per-poll overhead to shift the timing window past the label's
  render duration on some runs. Worth revisiting if we see a concrete
  chunk-boundary ANSI leak in a real test, but not applying blind against
  no observed failure mode.

Local validation
- Typecheck clean.
- Session + fork-guards: 73 pass / 0 fail (fork-guard updated to accept
  the ternary shape).
- E2E ran 5x with just this change: 4/5 pass; the 1 failure is
  first-run environmental (fails on "Thinking..." fallback, before any
  resolve-tools span code executes).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q8FGy89Qpr39k8nCSpCcK2

* fix: escalate to SIGKILL in pty-tui dispose to avoid orphaned test children

The PTY test fixture's `dispose()` sent `SIGTERM` then waited up to 1000ms
for exit, but never escalated. A child that traps/ignores `SIGTERM` (or is
slow to tear down its render loop + worker) would be left orphaned when
`dispose()` returns — a silent test-process leak on CI. Add a `SIGKILL`
escalation after the grace window. The happy path (child exits cleanly
within the grace) is byte-for-byte unchanged.

Addresses the OpenCodeReview finding on
`packages/opencode/test/fixture/pty-tui.ts`.

Verified: typecheck clean; `phase-label.tui-e2e.test.ts` passes (fixture
launches + disposes cleanly).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WRxuYemV9xjUArbumGSUC

---------

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 22, 2026
…INOR + 1 NIT + 2 missing tests)

Addresses the panel's findings on PR #1027. Summary of what landed:

- MAJOR #1 — subdir-invocation content-resolution: `makeContentResolver`
  working-tree read, `warnIfStale` file existence, and `makeCompiledResolver`
  compiled-SQL lookup all previously joined repo-relative paths from
  `git diff --name-status` with the caller's `opts.cwd`, silently ENOENT-ing
  when the CLI was invoked from a subdir. Now `reviewPullRequest` resolves
  `git rev-parse --show-toplevel` once via a new `gitRepoRoot()` helper,
  passes it to `makeContentResolver` (new `gitRoot` opt) and `warnIfStale`
  (renamed to `fsRoot`), and computes `pathPrefix = path.relative(gitRoot,
  dbtRoot)` for `makeCompiledResolver`. 8 new regression tests in
  `review-subdir-invocation.test.ts`; codex-round-5 HIGH on Windows path
  separator normalization also addressed (pathPrefix is normalised to POSIX
  before matching git-diff paths).

  *Note on authorship*: the `makeContentResolver` working-tree read
  (`fs.readFile(path.join(opts.cwd, file))` in `git.ts`) is pre-existing
  code from May 2026 (anandgupta42's commit `79c9ddd22`), not introduced
  in this PR. The subdir-invocation bug it triggers surfaces via the R18
  work in this PR (`warnIfStale` + `makeCompiledResolver` on `dbtRoot`),
  which is why the consensus panel scoped it here. Fix is defensive
  tightening — makes the flow correct end-to-end.

- MINOR #2 — envelope zod invariant: `VerdictEnvelope` now carries a
  `superRefine` enforcing (`tierForced=true` ↔ `tierClassified` defined),
  and `tierForced: false` is rejected as illegitimate (the marker exists
  to positively record a bypass; unforced runs must omit it entirely).
  4-case regression test.

- MINOR #3 — per-file "N data tests" summary: reworded from ambiguous
  "This PR removes N data tests in total" to schema-file-scoped "This
  schema file drops N data tests" (per PR #1027 consensus). Existing
  regression test updated.

- MINOR #4 — multiple `relationships` tests on one column collapsed:
  `extractTestOccurrences` now discriminates `relationships` by (to,
  field) via a `\x01<to>\x02<field>` suffix on the occurrence key.
  Downstream, the discriminator is stripped from display copy (title/
  body) but retained in `ruleKey` so the global fingerprint dedupe keeps
  distinct relationships-on-same-column findings separate. Regression
  tests cover both-removed (→ 2 findings) and one-removed (→ 1 finding).
  Codex-round-5 minor on colon-collision in the tag also addressed by
  preserving the internal `\x02` separator inside `ruleKey` rather than
  colon-joining (avoids `to='a:b'`/`field='c'` collision).

- MINOR #5 — `boolean-negation: false` regression coverage: new
  parametric test in `review-ci.test.ts` iterating declared boolean
  flags (`json`, `post`, `no-ai`, `explain-tier`) and asserting each
  parses to the expected type + default under bare / `=true` / `=false`
  invocations. Locks the parser configuration against future breakage.

- NIT #6 — `autoDiscoverManifest` loop tidy: rewritten as a `for`-loop
  using the `path.dirname(dir) === dir` fixed-point check, removing the
  earlier redundant `if (dir === root)` guard.

Not addressed:
- NIT #7 — positional fallback discriminator was noted stable within a
  diff by the reviewer; no code action required.

Full altimate review suite: 3787 pass / 640 skip / 0 fail (133 files;
up from 3773 baseline — 14 new tests).

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
…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 added a commit that referenced this pull request Jul 23, 2026
…auto-discovery + column-aware schema.yml test-removal (#1027)

* feat(review): observability + recall (Round 18, B1 + G1 + G2 + G3 + G6)

Round-17 bench established three baselines:
- A1-default: 7/64 recall (11%), 4% precision
- A1-workaround (with --manifest): 3/8 on applicable jaffle scenarios
- Baseline B: 0/64 (plugin never invoked the CLI — separately fixed)

This ships the six review-side changes documented as improvement paths
in the Round-18 plan. Each is guarded to avoid regressing the current
customer numbers.

## B1 — bare `--no-ai` triggers yargs help path

yargs' automatic `--no-<option>` negation collides with the option
literally named `no-ai`: bare `--no-ai` is interpreted as "set
undeclared option `ai` to false" and the command silently falls to the
help path (exit 0, no review runs). Every `--no-ai` invocation across
the bench was affected — our earlier A1 numbers were captured with
`--no-ai --yolo` but the flag was inert and the AI lane always ran.

Fix: `.parserConfiguration({ "boolean-negation": false })` on the
review command's yargs builder. `--no-ai` now binds to the declared
`noAi` flag as authored. `--no-ai=true` / `--no-ai=false` still work
for programmatic parity.

## G1 — `--explain-tier` flag

Adds a boolean flag that surfaces the tier classifier's reason list
on the verdict envelope (`tierReasons: string[]`) and in the human-
readable render (`> 🧭 **Tier: X** — ...`). Read-only — doesn't
change tier classification or verdicts. When the flag is off, the
envelope omits `tierReasons` entirely (backwards compatible; existing
consumers unaffected).

## G2 — `--force-tier <trivial|lite|full>` flag

EXPERIMENTAL / bench-debug only. Overrides the classifier's tier so
we can measure the tier-gate contribution to recall misses (e.g. js2
gets `trivial/0` — can't tell if the catalog missed vs. the tier gate
filtered it out). Guardrails:

- Prints an "EXPERIMENTAL (bench / debug only)" warning to stderr
  every use.
- Envelope carries `tierForced: true` and `tierClassified: <original>`
  so audits can see the bypass.
- `tierReasons` is force-populated with a leading marker even if
  `--explain-tier` isn't set, so the renderer always shows the tier
  was forced.
- Verdict header renders as `full tier — forced (was trivial)`.

Not a default-visible feature. Documented as debug-only in the yargs
`describe`, and the stderr banner ensures no customer accidentally
uses it in CI without noticing.

## G3 — manifest auto-discovery + freshness warning

Before: `--manifest <path>` had to be passed explicitly. Otherwise
the CLI used the config-relative default `target/manifest.json`,
which silently missed whenever `cwd` wasn't exactly the dbt project
root — every such review degraded to lint-only.

After (in `run.ts`):

1. If `--manifest` isn't explicit AND the config-relative path
   doesn't exist, walk UP from `cwd` looking for `dbt_project.yml`.
   Use the adjacent `target/manifest.json` when it exists.
2. Log the discovery to stderr (`ℹ️  auto-discovered dbt manifest at
   ...`) so customers see which manifest the review used.
3. Refuse to auto-discover a manifest from a directory that doesn't
   contain a dbt project — a `target/` from an unrelated tool (e.g.
   Airflow) never gets picked up.
4. Freshness warning: when `--head` is set (CI / bench shape) AND
   any changed file has an mtime newer than the manifest, print a
   `⚠️  manifest ... appears stale` message to stderr. Skipped for
   working-tree diffs because mtime noise during active edits would
   spam warnings.

Explicit `--manifest <path>` always wins. Auto-discovery is only
attempted when the caller was silent AND the config default is absent.

## G6 — column-aware schema.yml test-removal detector

Before: the detector compared removed vs added test lines as bare
strings (`- unique`, `- not_null`). Any sibling column that STILL had
the same test type re-added its line under a different indent (yaml
re-serialization), silently cancelling the genuine removal from a
different column. js2 (drops `unique` + `not_null` from
`customers.customer_id` while `orders.order_id` still has them) hit
this exact bug: 0/2 recall in both A1 modes across Round 17.

After: `collectTestOccurrencesFromDiff` walks the unified diff
tracking `(model, column)` context via nearest preceding
`- name: <X>` headers and hunk boundaries. Removed tests are keyed by
`(model, column, test)` and only cancelled by a re-add on the SAME
tuple. One finding per removal (not one summary), with severity
elevated to `warning` for `unique` removals and for `not_null` on
`_id` / `_key` / `id` columns (silent-PK-null risk).

Verified on js2 locally: 0 findings → 2 warnings.

## Verdict envelope schema

`VerdictEnvelope` gains three optional fields, all included in the
signed canonical body so tampering is detectable:

- `tierReasons?: string[]` — from G1 (surfaces classifier reasons)
- `tierForced?: boolean` — from G2
- `tierClassified?: RiskTier` — from G2 (original tier before force)

All are optional and absent in the default (no-flag) case, so
existing verdicts remain byte-equivalent to before.

## What's NOT in this change

- G4 (`verify` subcommand) — deferred; doesn't move measured metrics.
- G5 (`--commit` / `--pr` / `--files`) — deferred; adoption UX only.
- G7 (severity mapping tune-up) — deferred; needs its own design pass.
- Real-MR taxonomy expansion — larger design lift; addressed in a
  later round.

## Follow-up

Rerun A1-default and A1-workaround on the 13-scenario corpus with
these changes and publish the deltas as Round 18 in the reviewer
plugin journal.

* fix(review): Codex R18 review follow-ups — G2 audit guardrail + G6 copy

Independent code review by Codex (2026-07-21) surfaced two issues in
the R18 fixes:

1. G2 guardrail bug — `tierForced` was only set to `true` when the
   forced tier value happened to DIFFER from the classifier's
   decision. So `altimate-code review --force-tier=full` on a PR the
   classifier would naturally rate `full` still runs the debug bypass
   (silently exercises the flag path in orchestrate.ts) but emits an
   envelope with no `tierForced`, no `tierClassified`, and no
   "forced via" reason — breaking the audit-trail invariant we
   documented in the R18 commit.

   Fix: `tierForced = input.forceTier !== undefined`. Whenever the
   caller passed the flag, the envelope now records the bypass
   regardless of whether the forced value matched the natural one.
   The leading reason string now also names the forced value.

   Verified on js4 (naturally `full`): `--force-tier=full` now
   produces `tierForced: true, tierClassified: "full", tierReasons:
   ["forced via --force-tier=full (classifier said full)", ...]`.

2. G6 cosmetic copy issue — the schema.yml test-removal detector's
   body claimed "Removing a `unique` test on a mart-layer key is how
   silent duplicate rows ship" for EVERY unique-test removal, even
   when the schema.yml lives outside a mart layer (e.g. js2's
   `models/schema.yml` sits at the top level, not under `models/marts/`).

   Fix: detect the layer from the file path (`marts?` or `reporting`
   → "mart-layer"; otherwise "declared") and interpolate that into
   the body. The finding is still accurate for genuine mart-layer
   removals; the copy no longer misattributes for staging or
   top-level schema files.

Neither change alters what findings are surfaced, only the audit
envelope shape (G2) and finding body prose (G6). No test-file
changes; smoke tests on js2 + js4 confirm both.

* fix(review): address PR-1027 code-review feedback (blocker + majors + minors)

Independent multi-reviewer code review on the PR surfaced one blocker,
two majors, and three minors. This commit addresses each finding and
adds regression-guard tests.

## BLOCKER — column-aware detector regresses on real diffs + broke its
own test (finding #1)

The R18 `collectTestOccurrencesFromDiff` walker required both
`- name: model` AND `- name: column` inside the same diff hunk to
attribute a removed test. With git's default `-U3` context the model
header sits many lines above a column's `tests:` block and isn't in the
hunk — so the walker misclassified the first `- name:` it saw as a
model, never set `currentColumn`, and silently dropped the removal.

The existing unit test at test/altimate/review-dbt-patterns.test.ts:141
(`"-          - unique\n-          - not_null"`) went from pass to fail
under R18, so the branch was shipping a red test suite as well.

Rewrite `detectSchemaYmlPatterns` to prefer STRUCTURAL YAML diffing:

- Accept optional old/new file content via a `SchemaYmlDetectContent`
  parameter. When provided, parse both sides with the `yaml` package,
  walk `models[]`, `snapshots[]`, `sources[]`, `seeds[]`, extract every
  `(entity, column?, test)` tuple, diff old vs new sets, emit one
  finding per genuine removal.
- Supports both column-level and model-level tests (dbt allows
  `tests:` / `data_tests:` directly under the entity for
  surrogate-key `unique`, etc.) — closes MAJOR finding #2.
- Handles both `tests` and `data_tests` (dbt 1.8+ alias), both bare
  (`- unique`) and block-form (`- relationships: {...}`) tests, and
  quoted / commented YAML names — closes MINOR finding #6.
- Sources are qualified as `source.table.column` in the model field.

Fall back to the pre-R18 string-based line detection when no content is
provided (e.g. unit-test callers, offline CI diffs without a content
resolver). Fallback emits a suggestion / warning without column
attribution — cannot distinguish "moved to sibling column" from
"removed" with diff-only input, that limitation is inherent. The
existing test's expectation is preserved.

Wire the orchestrator to always supply old/new content when calling the
detector on modified schema.yml files, so the fallback only fires in
tests / diff-only CI paths, not in production reviews.

## MAJOR — auto-manifest projectRoot not threaded to compiled resolver
(finding #3)

`autoDiscoverManifest` returned `{ path, projectRoot }` and rebased
`manifestAbs` correctly, but `dbtProjectName(opts.cwd)` and
`makeCompiledResolver({ cwd: opts.cwd })` still used `opts.cwd`. When
the CLI was invoked from a subdirectory, auto-discovery would find the
manifest in an ancestor project but the compiled resolver looked for
`target/compiled/…` under the subdir and never found it — engine lanes
silently fell back to raw Jinja.

Thread a `dbtRoot` variable through the auto-discovery path: starts as
`opts.cwd`, updated to `discovered.projectRoot` when G3 fires. Use it
for both `dbtProjectName(dbtRoot)` and
`makeCompiledResolver({ cwd: dbtRoot })` so compiled SQL is found next
to the discovered manifest.

## MINORS

- **#4 docstring** — `autoDiscoverManifest`'s docstring cited defenses
  ("relative escapes", "under project's parent") that aren't in the
  code. Rewrote the doc to describe what actually happens:
  walk up for `dbt_project.yml`; return the adjacent
  `target/manifest.json` when present; never grab a `target/` from an
  unrelated tool that happens to sit above us on the filesystem.

- **#5a verdictHeadline undefined** — `env.tierClassified` is optional
  in the schema; an externally-built envelope with `tierForced: true`
  but no `tierClassified` would render `was undefined`. Added a
  `?? "unknown"` fallback in `format.ts`.

- **#5b unbounded tierReasons in summary** — when `--force-tier` is
  passed on a large PR, `tierReasons` prepends the forced marker AND
  spreads all `classifyPR().reasons` (one entry per file that forces
  the FULL tier). Rendered comment was bloating. Cap the rendered
  summary at the first 8 reasons with a "+N more in verdict envelope"
  overflow marker; the full list stays in the signed envelope for
  audit.

## Tests

Added 9 new regression tests:

- `dbt-patterns` — structural: sibling-column edge case (the exact
  case G6 claims to fix), model-level test removal with attribution
  metadata, block-form `- relationships:`, `data_tests` alias,
  source-column tests, added-file returns 0 removals, fallback
  diff-only path preserves the existing test's expectation.
- `verdict` — `--force-tier` envelope audit: `tierForced` and
  `tierClassified` are set whenever the flag is passed (including when
  forced tier matches classifier), and are covered by the HMAC
  signature (tampered envelope stripping the audit fields fails
  verifyEnvelope).

Test suite: 155/155 passing across the six review test files
(previously 46/47 — the one test the R18 branch broke is back green).
Typecheck clean.

* fix(review): address PR-1027 bot-review round-2 (dedupe collapse, rename bypass, snapshot yml, +5 more)

After the earlier fixes on `b32065b23`, three independent reviewer
bots (coderabbitai, cubic-dev-ai, dev-punia-altimate) surfaced 8 more
issues on the schema.yml test-removal path — 2 HIGH, 1 MAJOR, 4
MEDIUM, 1 LOW — plus an independent local review round after that
caught an additional blocker downstream of the fixes. This commit
addresses each.

## BLOCKER — fallback distinct-removal fix defeated by global dedupe

The R18-follow-up fix removed a local dedup in the fallback path so
each removed test-line produced its own detector-level finding. But
`runReview` runs a global `dedupe(merged)` step that fingerprints
findings by (category, file, model, column, ruleKey) via
`finding.ts:107`. For fallback findings sharing `file`, empty
`model`, empty `column`, and a ruleKey varying only by `test`, two
`unique` removals in the same file still collapsed to one downstream.
The detector-level test at `review-dbt-patterns.test.ts:397` didn't
catch it because it asserts against the pre-dedupe output.

Fix: give fallback findings a per-diff occurrence-index discriminator
appended to the ruleKey (`.#0`, `.#1`, …) so distinct removals get
distinct fingerprints. Structural (attributed) findings unchanged —
`(model.column.test)` is already unique per removal.

Added an integration-shape regression test at
`review-dbt-patterns.test.ts:432` that pipes detector output through
`dedupe(f)` and asserts 4 distinct ids survive for 4 distinct
removals. Also a same-shape guard for the structural path.

## HIGH — renamed schema.yml bypasses detector

The orchestrator gated `oldContent` fetch on `file.status === "modified"`.
A schema.yml being renamed (e.g. moved to a new subdir) that also
dropped a `unique`/`not_null`/`relationships` guardrail silently
skipped the detector because `oldContent` came back undefined and
the structural path treated the file as newly-added.

Fix: gate on `file.status !== "added"` so renamed files fetch the
old side; separately, changed the detector so an undefined
`oldContent` on a modified/renamed file falls through to the
diff-based fallback (does not silently treat as added-file).

Added test at `review-dbt-patterns.test.ts:332` covering the rename
shape.

## MAJOR — structural path treats `oldContent=undefined` as added file

`canUseStructural = newContent !== undefined && (isAddedFile ||
oldContent !== undefined)`. When the content resolver returns
undefined on a modified/renamed file for a transient reason (git
failure, ref not readable), we now leave `usedStructural = false`
and let the fallback surface the raw diff removals — instead of
producing an empty structural-diff and silently dropping every
removal in the diff.

## MEDIUM — snapshot YAML property files never reached the detector

`classifyDbtFile` matched everything under `snapshots/` as
`snapshot` regardless of extension, so `snapshots/*.yml` never
reached the `schema_yml` gate in the orchestrator. The new
`snapshots:` branch added to the structural walker was dead code
for real snapshot property files.

Fix: split the classification by extension. `snapshots/*.sql`
remains `snapshot` (unchanged tier-forcing / catalog rules).
`snapshots/*.yml` classifies as `schema_yml` (routes to the
test-removal detector). Added `classifyDbtFile` assertions for
both extensions and an end-to-end structural-detector test on a
snapshot yml.

## MEDIUM — redundant per-model summary line

`isFirstForModel` fired once per distinct model, so a diff touching
two models produced two copies of the same aggregate summary
("This PR removes N tests total on model(s) A, B") appended to
separate findings.

Fix: replaced with a single file-scoped `summaryEmitted` boolean.
The aggregate summary is appended to the first finding for the
file, once, and the distinct-models list is computed from the
whole removals array. Test at
`review-dbt-patterns.test.ts:454` asserts exactly one finding
carries the aggregate line when two models have removals.

## MEDIUM — `warnIfStale` fired on unrelated file changes

`warnIfStale` compared mtimes for every changed path. Any
`README.md` / `package.json` / `.github/*` change post-manifest
triggered a false-positive stale warning.

Fix: introduced `isManifestAffecting()` — admits only
`.sql|.py|.yml|.yaml|.csv|.md` under
`models|seeds|snapshots|macros|tests|analyses/`, plus root
`dbt_project.yml|packages.yml|profiles.yml|dependencies.yml`.
Explicitly admits `.md` under `models/` because dbt docs blocks
live there. Exported for tests; added a table-driven test file
`review-run-stale.test.ts` covering both admitted and rejected
cases (README.md, models/foo.sql, docs/foo.md, models/foo.md,
seeds/lookup.csv, snapshots/*.sql, snapshots/*.yml, dbt_project.yml,
.github/workflows/, target/…).

## LOW — YAML parse errors silently ignored

Both `YAML.parse` failures (new content, old content) now log via
`Log.create({ service: "review", tag: "detectSchemaYmlPatterns" })`
with the file path and error message, then fall through to the
diff-based fallback. Not fail-open.

## PERF — sequential schema.yml `getContent` calls

The orchestrator's schema.yml loop was serial `await` per file;
schema-heavy PRs paid one round-trip per file. Refactored to
`Promise.all` across the file list with per-file `Promise.all`
for old/new. Ordering preserved by `Promise.all` contract.

## Test status

188 pass / 0 fail across the 7 review test files. Typecheck clean.
End-to-end sanity on the js2 test-removal scenario: CLI still
surfaces both `unique` and `not_null` warnings, verdict
`COMMENT/trivial/2`, envelope signature verifies.

* perf(review): address PR-1027 round-3 nits (deleted-file fetch, summary alloc, shift O(N²))

Three centralized-review findings on the previous commit `6729f5374`,
all perf/hygiene:

## MEDIUM — schema.yml orchestrator fetched `oldContent` for deleted files

`file.status !== "added"` on the gate matched `"deleted"` too, triggering
a `git show` round-trip whose result the detector immediately discarded
(early-returns `[]` for `status === "deleted"`).

Fix: filter deleted schema files out of the loop upfront —
`reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted")`.
The `newContent` fetch also simplifies to unconditional
`getContent?.(file.path, "new")` since deleted files never reach the loop.

## MEDIUM — `distinctModels`/`modelClause` recomputed on every loop iteration

The `[...new Set(...).filter(...).map(...).join(...)]` chain was evaluated
on every removal even though the result is only used when `shouldEmitSummary`
is true (once per file). Moved the computation inside the guard.

## LOW — `fallbackDiscriminators.shift()` inside the loop is O(N²)

`Array.prototype.shift()` re-indexes all remaining elements on each call.
Replaced with a hoisted `let fallbackIdx = 0` + `fallbackDiscriminators[fallbackIdx++]`.
Negligible on 2-3 removals but grows quadratically on wider PRs.

## Test status

188 pass / 0 fail across the 7 review test files. Typecheck clean. No
behavior change beyond the deleted-file fetch skip (which is a pure
performance improvement — same output, fewer round-trips).

* fix(review): address PR-1027 round-4 (deleted schema.yml + backtick fence + Bun-alias flake + finding attribution)

Four items from cubic-review round-3 + one CI-only test flake:

- **Deleted schema.yml handling (cubic P2)** — `detectSchemaYmlPatterns`
  now diffs old YAML against `{}` when `status === "deleted"`, so
  dropping a whole schema file surfaces every prior test as a removal.
  `orchestrate.ts` no longer filters deleted files and skips
  `newContent` fetch for them (git-show at HEAD would fail).
  Two regression tests: 4-test deletion, and safe-degrade when
  `oldContent` is missing.

- **Backtick-safe tierReasons rendering (cubic P3)** — `format.ts`
  now sizes the inline-code-span fence to `max(backtick-run) + 1`
  and pads leading/trailing backticks with a space, so paths like
  `` `foo`bar` `` can't terminate the span or inject Markdown.
  Regression test covers single/double/leading/trailing/both-ends cases.

- **Structured attribution on removal findings (cubic P3)** —
  `removed_tests` findings now include `model` and `column` on the
  top-level Finding, not just inside `evidence.result`, so downstream
  dedupe / formatting / telemetry see them.

- **TUI network-options test flake (CI-only)** — Bun's transpiler
  statically resolved the `@/cli/network` string literal in
  `toContain('...')` into a `file:///…/network.ts` URL, inverting the
  comparison. Rewritten as `toMatch(/regex/)` so the alias literal
  isn't recognizable to Bun's analyzer.

Additional edge tests per codex round-4 review:
- Deleted schema.yml with unparsable oldContent → diff-only fallback.
- Deleted empty schema.yml → no fabricated findings.
- tierReasons with both leading + trailing backticks → symmetric padding.

Tests: 145 pass, 0 fail in touched suites; full review suite (188 tests) green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of

* fix(review): address consensus-review findings round-5 (1 MAJOR + 4 MINOR + 1 NIT + 2 missing tests)

Addresses the panel's findings on PR #1027. Summary of what landed:

- MAJOR #1 — subdir-invocation content-resolution: `makeContentResolver`
  working-tree read, `warnIfStale` file existence, and `makeCompiledResolver`
  compiled-SQL lookup all previously joined repo-relative paths from
  `git diff --name-status` with the caller's `opts.cwd`, silently ENOENT-ing
  when the CLI was invoked from a subdir. Now `reviewPullRequest` resolves
  `git rev-parse --show-toplevel` once via a new `gitRepoRoot()` helper,
  passes it to `makeContentResolver` (new `gitRoot` opt) and `warnIfStale`
  (renamed to `fsRoot`), and computes `pathPrefix = path.relative(gitRoot,
  dbtRoot)` for `makeCompiledResolver`. 8 new regression tests in
  `review-subdir-invocation.test.ts`; codex-round-5 HIGH on Windows path
  separator normalization also addressed (pathPrefix is normalised to POSIX
  before matching git-diff paths).

  *Note on authorship*: the `makeContentResolver` working-tree read
  (`fs.readFile(path.join(opts.cwd, file))` in `git.ts`) is pre-existing
  code from May 2026 (anandgupta42's commit `79c9ddd22`), not introduced
  in this PR. The subdir-invocation bug it triggers surfaces via the R18
  work in this PR (`warnIfStale` + `makeCompiledResolver` on `dbtRoot`),
  which is why the consensus panel scoped it here. Fix is defensive
  tightening — makes the flow correct end-to-end.

- MINOR #2 — envelope zod invariant: `VerdictEnvelope` now carries a
  `superRefine` enforcing (`tierForced=true` ↔ `tierClassified` defined),
  and `tierForced: false` is rejected as illegitimate (the marker exists
  to positively record a bypass; unforced runs must omit it entirely).
  4-case regression test.

- MINOR #3 — per-file "N data tests" summary: reworded from ambiguous
  "This PR removes N data tests in total" to schema-file-scoped "This
  schema file drops N data tests" (per PR #1027 consensus). Existing
  regression test updated.

- MINOR #4 — multiple `relationships` tests on one column collapsed:
  `extractTestOccurrences` now discriminates `relationships` by (to,
  field) via a `\x01<to>\x02<field>` suffix on the occurrence key.
  Downstream, the discriminator is stripped from display copy (title/
  body) but retained in `ruleKey` so the global fingerprint dedupe keeps
  distinct relationships-on-same-column findings separate. Regression
  tests cover both-removed (→ 2 findings) and one-removed (→ 1 finding).
  Codex-round-5 minor on colon-collision in the tag also addressed by
  preserving the internal `\x02` separator inside `ruleKey` rather than
  colon-joining (avoids `to='a:b'`/`field='c'` collision).

- MINOR #5 — `boolean-negation: false` regression coverage: new
  parametric test in `review-ci.test.ts` iterating declared boolean
  flags (`json`, `post`, `no-ai`, `explain-tier`) and asserting each
  parses to the expected type + default under bare / `=true` / `=false`
  invocations. Locks the parser configuration against future breakage.

- NIT #6 — `autoDiscoverManifest` loop tidy: rewritten as a `for`-loop
  using the `path.dirname(dir) === dir` fixed-point check, removing the
  earlier redundant `if (dir === root)` guard.

Not addressed:
- NIT #7 — positional fallback discriminator was noted stable within a
  diff by the reviewer; no code action required.

Full altimate review suite: 3787 pass / 640 skip / 0 fail (133 files;
up from 3773 baseline — 14 new tests).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of

* fix(review): address post-round-5 bot-review findings on PR #1027

Five items from coderabbit / cubic-dev-ai / kilo-code-bot on round 5:

- Symlink-escape containment (coderabbit + cubic): `makeContentResolver`
  working-tree read and `makeCompiledResolver` compiled-SQL read now go
  through a `realpath` containment check that rejects any target whose
  resolved path falls outside the resolved root. A tracked symlink like
  `models/evil.sql → /etc/passwd` no longer leaks external content into
  the review pipeline. Applied at both the diff-side (git.ts) and
  compiled-side (compiled.ts) resolvers.

- Symlink-consistent path prefix (cubic + kilo): `path.relative(gitRoot,
  dbtRoot)` produced a bogus climb path (`../../../var/...`) whenever
  `dbtRoot` traversed an unresolved symlink (macOS `/var` → `/private/var`
  is the canonical case). The mapped prefix never matched incoming
  repo-relative paths, so compiled SQL was silently missed on the exact
  monorepo layouts the subdir fix was meant to enable. Both roots are
  now realpath-resolved before `path.relative`, and `dbtRootReal` is
  handed to `makeCompiledResolver` as its `cwd`. Falls back gracefully
  when realpath fails (path deleted mid-run).

- `gitRepoRoot` newline handling (cubic P3): `.trim()` also stripped
  legitimate leading/trailing path whitespace. Replaced with an explicit
  `\r?\n` terminator strip that leaves everything else intact.

- dbt version comment (cubic P3): the `arguments:` nesting syntax is
  documented from dbt v1.10.5, not 1.9+. Comment updated.

New regression tests in `review-subdir-invocation.test.ts`:
- makeContentResolver refuses to read a symlink escaping the git root
- makeCompiledResolver refuses to read a compiled symlink escaping
  the compiled root
- gitRepoRoot preserves legitimate path characters (no trim()
  regression)

Full altimate review suite: 3790 pass / 640 skip / 0 fail (was 3787
baseline — 3 new tests, +8 expect calls).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of

* refactor(review): [PR #1027] share safeReadInside between git.ts and compiled.ts

cubic + kilo bot review — the realpath containment check was duplicated
across `makeContentResolver` (git.ts) and `makeCompiledResolver`
(compiled.ts). Both bots flagged that keeping two copies of security-
sensitive logic risks a future fix (e.g. Windows case-sensitivity,
trailing-separator edge) landing in one call site but not the other.

- Export `safeReadInside(root, rel)` from git.ts as the single
  realpath-checked reader.
- `compiled.ts` imports it and replaces its inline containment block
  with `return await safeReadInside(compiledRoot, rel)`.
- No behavior change; regression suite still 3790 pass / 0 fail.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXDXMiNFh4qDPxPCfa2of

* fix(review): address altimate-harness-bot review findings on run.ts

Three findings from the harness-bot pass on PR #1027:

1. `run.ts:114` — add rationale for the fixed-point walk's early
   return. When `dbt_project.yml` exists but `target/manifest.json`
   doesn't, we intentionally return `undefined` rather than continue
   walking to a grandparent's compiled project — a grandparent's DAG
   is a different project's DAG. Comment makes this clear so future
   maintainers don't "fix" it into a silent grandparent-fallback.

2. `run.ts:129` — narrow the `.md` match in `isManifestAffecting`.
   dbt `{% docs %}` blocks are canonically parsed under `models/` and
   `analyses/`. Under `macros/`, `seeds/`, `snapshots/`, and `tests/`,
   `.md` files are package documentation (READMEs), not manifest input.
   Split the check so `.md` requires `models|analyses`; other
   extensions (`.sql`, `.py`, `.yml`, `.yaml`, `.csv`) still admitted
   under all six source directories. Added `README.md` under each of
   the four newly-excluded directories to the rejected-path test list.

3. `run.ts:120-124` — JSDoc block that describes `warnIfStale` was
   physically placed above `isManifestAffecting`, so TS/IDE tooling
   would attribute both blocks to the earlier symbol and `warnIfStale`
   would show no hover doc. Split into two blocks: a self-contained
   docstring for `isManifestAffecting` (its own concerns + why it's
   exported) and a fresh docstring for `warnIfStale` immediately
   preceding its definition.

Tests: 210/210 green (7 review-* files); review-run-stale.test.ts
gains `analyses/gross_margin.md` (admitted) plus four `.md`-under-
non-docs-dirs entries in the rejected list.

---------

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
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant