Skip to content

Commit b32065b

Browse files
sahrizvisahrizvi
committed
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.
1 parent 98a0439 commit b32065b

6 files changed

Lines changed: 548 additions & 140 deletions

File tree

0 commit comments

Comments
 (0)