review: --no-ai fix + --explain-tier / --force-tier flags + manifest auto-discovery + column-aware schema.yml test-removal#1027
Conversation
Round-17 bench established three baselines:
- A1-default: 7/64 recall (11%), 4% precision
- A1-workaround (with --manifest): 3/8 on applicable jaffle scenarios
- Baseline B: 0/64 (plugin never invoked the CLI — separately fixed)
This ships the six review-side changes documented as improvement paths
in the Round-18 plan. Each is guarded to avoid regressing the current
customer numbers.
## B1 — bare `--no-ai` triggers yargs help path
yargs' automatic `--no-<option>` negation collides with the option
literally named `no-ai`: bare `--no-ai` is interpreted as "set
undeclared option `ai` to false" and the command silently falls to the
help path (exit 0, no review runs). Every `--no-ai` invocation across
the bench was affected — our earlier A1 numbers were captured with
`--no-ai --yolo` but the flag was inert and the AI lane always ran.
Fix: `.parserConfiguration({ "boolean-negation": false })` on the
review command's yargs builder. `--no-ai` now binds to the declared
`noAi` flag as authored. `--no-ai=true` / `--no-ai=false` still work
for programmatic parity.
## G1 — `--explain-tier` flag
Adds a boolean flag that surfaces the tier classifier's reason list
on the verdict envelope (`tierReasons: string[]`) and in the human-
readable render (`> 🧭 **Tier: X** — ...`). Read-only — doesn't
change tier classification or verdicts. When the flag is off, the
envelope omits `tierReasons` entirely (backwards compatible; existing
consumers unaffected).
## G2 — `--force-tier <trivial|lite|full>` flag
EXPERIMENTAL / bench-debug only. Overrides the classifier's tier so
we can measure the tier-gate contribution to recall misses (e.g. js2
gets `trivial/0` — can't tell if the catalog missed vs. the tier gate
filtered it out). Guardrails:
- Prints an "EXPERIMENTAL (bench / debug only)" warning to stderr
every use.
- Envelope carries `tierForced: true` and `tierClassified: <original>`
so audits can see the bypass.
- `tierReasons` is force-populated with a leading marker even if
`--explain-tier` isn't set, so the renderer always shows the tier
was forced.
- Verdict header renders as `full tier — forced (was trivial)`.
Not a default-visible feature. Documented as debug-only in the yargs
`describe`, and the stderr banner ensures no customer accidentally
uses it in CI without noticing.
## G3 — manifest auto-discovery + freshness warning
Before: `--manifest <path>` had to be passed explicitly. Otherwise
the CLI used the config-relative default `target/manifest.json`,
which silently missed whenever `cwd` wasn't exactly the dbt project
root — every such review degraded to lint-only.
After (in `run.ts`):
1. If `--manifest` isn't explicit AND the config-relative path
doesn't exist, walk UP from `cwd` looking for `dbt_project.yml`.
Use the adjacent `target/manifest.json` when it exists.
2. Log the discovery to stderr (`ℹ️ auto-discovered dbt manifest at
...`) so customers see which manifest the review used.
3. Refuse to auto-discover a manifest from a directory that doesn't
contain a dbt project — a `target/` from an unrelated tool (e.g.
Airflow) never gets picked up.
4. Freshness warning: when `--head` is set (CI / bench shape) AND
any changed file has an mtime newer than the manifest, print a
`⚠️ manifest ... appears stale` message to stderr. Skipped for
working-tree diffs because mtime noise during active edits would
spam warnings.
Explicit `--manifest <path>` always wins. Auto-discovery is only
attempted when the caller was silent AND the config default is absent.
## G6 — column-aware schema.yml test-removal detector
Before: the detector compared removed vs added test lines as bare
strings (`- unique`, `- not_null`). Any sibling column that STILL had
the same test type re-added its line under a different indent (yaml
re-serialization), silently cancelling the genuine removal from a
different column. js2 (drops `unique` + `not_null` from
`customers.customer_id` while `orders.order_id` still has them) hit
this exact bug: 0/2 recall in both A1 modes across Round 17.
After: `collectTestOccurrencesFromDiff` walks the unified diff
tracking `(model, column)` context via nearest preceding
`- name: <X>` headers and hunk boundaries. Removed tests are keyed by
`(model, column, test)` and only cancelled by a re-add on the SAME
tuple. One finding per removal (not one summary), with severity
elevated to `warning` for `unique` removals and for `not_null` on
`_id` / `_key` / `id` columns (silent-PK-null risk).
Verified on js2 locally: 0 findings → 2 warnings.
## Verdict envelope schema
`VerdictEnvelope` gains three optional fields, all included in the
signed canonical body so tampering is detectable:
- `tierReasons?: string[]` — from G1 (surfaces classifier reasons)
- `tierForced?: boolean` — from G2
- `tierClassified?: RiskTier` — from G2 (original tier before force)
All are optional and absent in the default (no-flag) case, so
existing verdicts remain byte-equivalent to before.
## What's NOT in this change
- G4 (`verify` subcommand) — deferred; doesn't move measured metrics.
- G5 (`--commit` / `--pr` / `--files`) — deferred; adoption UX only.
- G7 (severity mapping tune-up) — deferred; needs its own design pass.
- Real-MR taxonomy expansion — larger design lift; addressed in a
later round.
## Follow-up
Rerun A1-default and A1-workaround on the 13-scenario corpus with
these changes and publish the deltas as Round 18 in the reviewer
plugin journal.
Independent code review by Codex (2026-07-21) surfaced two issues in the R18 fixes: 1. G2 guardrail bug — `tierForced` was only set to `true` when the forced tier value happened to DIFFER from the classifier's decision. So `altimate-code review --force-tier=full` on a PR the classifier would naturally rate `full` still runs the debug bypass (silently exercises the flag path in orchestrate.ts) but emits an envelope with no `tierForced`, no `tierClassified`, and no "forced via" reason — breaking the audit-trail invariant we documented in the R18 commit. Fix: `tierForced = input.forceTier !== undefined`. Whenever the caller passed the flag, the envelope now records the bypass regardless of whether the forced value matched the natural one. The leading reason string now also names the forced value. Verified on js4 (naturally `full`): `--force-tier=full` now produces `tierForced: true, tierClassified: "full", tierReasons: ["forced via --force-tier=full (classifier said full)", ...]`. 2. G6 cosmetic copy issue — the schema.yml test-removal detector's body claimed "Removing a `unique` test on a mart-layer key is how silent duplicate rows ship" for EVERY unique-test removal, even when the schema.yml lives outside a mart layer (e.g. js2's `models/schema.yml` sits at the top level, not under `models/marts/`). Fix: detect the layer from the file path (`marts?` or `reporting` → "mart-layer"; otherwise "declared") and interpolate that into the body. The finding is still accurate for genuine mart-layer removals; the copy no longer misattributes for staging or top-level schema files. Neither change alters what findings are surfaced, only the audit envelope shape (G2) and finding body prose (G6). No test-file changes; smoke tests on js2 + js4 confirm both.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds structural dbt schema test-removal detection, manifest discovery and freshness checks, tier explanation and override controls, signed tier provenance, and tier-aware output rendering. It also corrects snapshot YAML classification and expands regression coverage. Changesdbt schema test detection
Tier and review controls
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewCLI
participant reviewPullRequest
participant runReview
participant classifyPR
participant buildEnvelope
participant format
ReviewCLI->>reviewPullRequest: pass explain-tier and force-tier
reviewPullRequest->>runReview: forward review options
runReview->>classifyPR: classify pull request tier
runReview->>runReview: apply optional forced tier
runReview->>buildEnvelope: attach tier reasons and provenance
buildEnvelope->>format: provide verdict envelope
format-->>ReviewCLI: render tier and explanation
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opencode/src/altimate/review/run.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPrefer
FileSystem.FileSystemfor effectful file I/O.The new
node:fs/promisesimports bypass the project's Effect-driven I/O abstraction. As per coding guidelines, "PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O".Consider migrating these new operations (
stat,access) to the EffectFileSystemmodule where feasible, though sincewarnIfStaleandautoDiscoverManifestare currently standardasyncfunctions, this might require broader refactoring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/review/run.ts` at line 2, Replace the raw node:fs/promises stat and access usage in warnIfStale and autoDiscoverManifest with the project’s FileSystem.FileSystem abstraction, refactoring those async flows as needed to use Effect-based file I/O. Preserve the existing stale-file checks and manifest discovery behavior while removing the direct filesystem imports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/review/dbt-patterns.ts`:
- Around line 851-878: Update collectChangedFiles() and
collectTestOccurrencesFromDiff() so schema.yml diffs retain enough context to
identify the enclosing model when a changed column header is the first name
entry in the hunk. Prefer increasing unified diff context for schema files;
otherwise add fallback scope inference that distinguishes the column from the
missing model while preserving detection of removed unique, not_null, and
relationships tests.
In `@packages/opencode/src/altimate/review/run.ts`:
- Around line 122-124: Filter changedPaths before or within the loop so only
relevant dbt model files—schema.yml files or SQL files—reach the stale-manifest
comparison using abs. Exclude unrelated files such as README.md and package.json
while preserving the existing tracked, on-disk checks for eligible paths.
---
Nitpick comments:
In `@packages/opencode/src/altimate/review/run.ts`:
- Line 2: Replace the raw node:fs/promises stat and access usage in warnIfStale
and autoDiscoverManifest with the project’s FileSystem.FileSystem abstraction,
refactoring those async flows as needed to use Effect-based file I/O. Preserve
the existing stale-file checks and manifest discovery behavior while removing
the direct filesystem imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1242bef6-a381-4d45-a224-359ee38b5c18
📒 Files selected for processing (6)
packages/opencode/src/altimate/review/dbt-patterns.tspackages/opencode/src/altimate/review/format.tspackages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/run.tspackages/opencode/src/altimate/review/verdict.tspackages/opencode/src/cli/cmd/review.ts
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Notes:
Previous Review Summary (commit 98a0439)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 98a0439)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Notes:
Reviewed by glm-5.2 · Input: 80.9K · Output: 5.6K · Cached: 719.3K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… minors) Independent multi-reviewer code review on the PR surfaced one blocker, two majors, and three minors. This commit addresses each finding and adds regression-guard tests. ## BLOCKER — column-aware detector regresses on real diffs + broke its own test (finding #1) The R18 `collectTestOccurrencesFromDiff` walker required both `- name: model` AND `- name: column` inside the same diff hunk to attribute a removed test. With git's default `-U3` context the model header sits many lines above a column's `tests:` block and isn't in the hunk — so the walker misclassified the first `- name:` it saw as a model, never set `currentColumn`, and silently dropped the removal. The existing unit test at test/altimate/review-dbt-patterns.test.ts:141 (`"- - unique\n- - not_null"`) went from pass to fail under R18, so the branch was shipping a red test suite as well. Rewrite `detectSchemaYmlPatterns` to prefer STRUCTURAL YAML diffing: - Accept optional old/new file content via a `SchemaYmlDetectContent` parameter. When provided, parse both sides with the `yaml` package, walk `models[]`, `snapshots[]`, `sources[]`, `seeds[]`, extract every `(entity, column?, test)` tuple, diff old vs new sets, emit one finding per genuine removal. - Supports both column-level and model-level tests (dbt allows `tests:` / `data_tests:` directly under the entity for surrogate-key `unique`, etc.) — closes MAJOR finding #2. - Handles both `tests` and `data_tests` (dbt 1.8+ alias), both bare (`- unique`) and block-form (`- relationships: {...}`) tests, and quoted / commented YAML names — closes MINOR finding #6. - Sources are qualified as `source.table.column` in the model field. Fall back to the pre-R18 string-based line detection when no content is provided (e.g. unit-test callers, offline CI diffs without a content resolver). Fallback emits a suggestion / warning without column attribution — cannot distinguish "moved to sibling column" from "removed" with diff-only input, that limitation is inherent. The existing test's expectation is preserved. Wire the orchestrator to always supply old/new content when calling the detector on modified schema.yml files, so the fallback only fires in tests / diff-only CI paths, not in production reviews. ## MAJOR — auto-manifest projectRoot not threaded to compiled resolver (finding #3) `autoDiscoverManifest` returned `{ path, projectRoot }` and rebased `manifestAbs` correctly, but `dbtProjectName(opts.cwd)` and `makeCompiledResolver({ cwd: opts.cwd })` still used `opts.cwd`. When the CLI was invoked from a subdirectory, auto-discovery would find the manifest in an ancestor project but the compiled resolver looked for `target/compiled/…` under the subdir and never found it — engine lanes silently fell back to raw Jinja. Thread a `dbtRoot` variable through the auto-discovery path: starts as `opts.cwd`, updated to `discovered.projectRoot` when G3 fires. Use it for both `dbtProjectName(dbtRoot)` and `makeCompiledResolver({ cwd: dbtRoot })` so compiled SQL is found next to the discovered manifest. ## MINORS - **#4 docstring** — `autoDiscoverManifest`'s docstring cited defenses ("relative escapes", "under project's parent") that aren't in the code. Rewrote the doc to describe what actually happens: walk up for `dbt_project.yml`; return the adjacent `target/manifest.json` when present; never grab a `target/` from an unrelated tool that happens to sit above us on the filesystem. - **#5a verdictHeadline undefined** — `env.tierClassified` is optional in the schema; an externally-built envelope with `tierForced: true` but no `tierClassified` would render `was undefined`. Added a `?? "unknown"` fallback in `format.ts`. - **#5b unbounded tierReasons in summary** — when `--force-tier` is passed on a large PR, `tierReasons` prepends the forced marker AND spreads all `classifyPR().reasons` (one entry per file that forces the FULL tier). Rendered comment was bloating. Cap the rendered summary at the first 8 reasons with a "+N more in verdict envelope" overflow marker; the full list stays in the signed envelope for audit. ## Tests Added 9 new regression tests: - `dbt-patterns` — structural: sibling-column edge case (the exact case G6 claims to fix), model-level test removal with attribution metadata, block-form `- relationships:`, `data_tests` alias, source-column tests, added-file returns 0 removals, fallback diff-only path preserves the existing test's expectation. - `verdict` — `--force-tier` envelope audit: `tierForced` and `tierClassified` are set whenever the flag is passed (including when forced tier matches classifier), and are covered by the HMAC signature (tampered envelope stripping the audit fields fails verifyEnvelope). Test suite: 155/155 passing across the six review test files (previously 46/47 — the one test the R18 branch broke is back green). Typecheck clean.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Thanks for the review. Follow-up commit Blocker #1 — column-aware detector regression + broken testFixed. Rewrote
Major #2 — model-level test removalsFixed as part of #1. Structural walker emits per-entity tests directly under Major #3 — auto-manifest projectRoot not threadedFixed. Added a Minor #4 — docstring overstates safety guaranteesFixed. Rewrote Minor #5 —
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/altimate/review/dbt-patterns.ts`:
- Around line 983-1003: Update the fallback deduplication in the !usedStructural
branch of the dbt pattern detection flow so distinct matched removal lines are
preserved even when model and column are empty. Key deduplication by each
matched line’s index or content, while still preventing duplicate processing of
the same fallback line and retaining the existing test-type extraction.
- Around line 954-981: The structural comparison around extractTestOccurrences
must only run when both old and new YAML documents are available; do not treat
undefined oldContent or an unparseable old document as an empty added-file side.
When old YAML is unavailable, leave usedStructural false so the caller executes
fallbackRemovedTestLines(file.diff), while preserving the added-file behavior
only when the old side is genuinely absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 682cedd5-0e3a-4488-9ff8-bb257c9a19d2
📒 Files selected for processing (6)
packages/opencode/src/altimate/review/dbt-patterns.tspackages/opencode/src/altimate/review/format.tspackages/opencode/src/altimate/review/orchestrate.tspackages/opencode/src/altimate/review/run.tspackages/opencode/test/altimate/review-dbt-patterns.test.tspackages/opencode/test/altimate/review.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/altimate/review/format.ts
- packages/opencode/src/altimate/review/orchestrate.ts
- packages/opencode/src/altimate/review/run.ts
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/review/dbt-patterns.ts">
<violation number="1" location="packages/opencode/src/altimate/review/dbt-patterns.ts:862">
P2: Snapshot YAML files under `snapshots/` never reach this extraction, so their removed tests remain unreported despite the new snapshots support. Classify snapshot YAML property files as `schema_yml` (or admit YAML `snapshot` files at this gate) before relying on this walker.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| } | ||
| // `snapshots:` — same shape as models | ||
| if (Array.isArray(d.snapshots)) { |
There was a problem hiding this comment.
P2: Snapshot YAML files under snapshots/ never reach this extraction, so their removed tests remain unreported despite the new snapshots support. Classify snapshot YAML property files as schema_yml (or admit YAML snapshot files at this gate) before relying on this walker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/dbt-patterns.ts, line 862:
<comment>Snapshot YAML files under `snapshots/` never reach this extraction, so their removed tests remain unreported despite the new snapshots support. Classify snapshot YAML property files as `schema_yml` (or admit YAML `snapshot` files at this gate) before relying on this walker.</comment>
<file context>
@@ -790,162 +791,294 @@ export function detectModelPatterns(file: ChangedFile, newSql: string | undefine
+ }
+ }
+ // `snapshots:` — same shape as models
+ if (Array.isArray(d.snapshots)) {
+ for (const s of d.snapshots) {
+ if (!s || typeof s !== "object") continue
</file context>
There was a problem hiding this comment.
Addressed in 6729f5374. classifyDbtFile in diff-filter.ts now splits by extension: snapshots/*.sql remains snapshot (tier-forcing / catalog rules unchanged), snapshots/*.yml classifies as schema_yml and reaches the test-removal detector. Assertions added in review.test.ts (snapshots/*.yml → schema_yml) and an end-to-end test at review-dbt-patterns.test.ts:417 (snapshot yml property file (structural): removed test surfaces finding).
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 5 finding(s)
- 5 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/review/dbt-patterns.ts (L994-L1002)
[🟠 MEDIUM] In fallback mode, model and column are empty strings, so the deduplication key is effectively just \x00\x00${test}. If multiple tests of the same type are removed, they will be collapsed into a single finding. This loses the total count of removed tests that the previous implementation provided (e.g., "removed 3 data test(s)"). Consider preserving the raw count of removed lines in fallback mode so the scale of test regressions is not understated to users.
2. packages/opencode/src/altimate/review/dbt-patterns.ts (L1055-L1061)
[🟠 MEDIUM] If a PR removes tests from multiple models, isFirstForModel will evaluate to true for the first finding of each distinct model. This means the exact same global summary ("This PR removes X data tests in total on model(s) A, B") will be redundantly appended to multiple findings in the review, causing comment clutter. Consider tracking a single boolean flag (e.g., let summaryPrinted = false) to ensure the global summary is only appended once to the very first finding generated for the file.
3. packages/opencode/src/altimate/review/dbt-patterns.ts (L958-L969)
[🔵 LOW] When YAML parsing fails, the exception is caught and silently ignored, which demotes the analysis to the string-diff fallback path. Consider adding a debug or telemetry log within the catch block. This would aid in diagnosing issues where valid-looking PRs fail structural parsing simply due to minor YAML syntax errors.
4. packages/opencode/src/altimate/review/run.ts (L126-L127)
[🟠 MEDIUM] Although the comment explicitly states an intention to only compare against tracked, on-disk model files (schema.yml or SQL), there is no actual file extension filtering applied to changedPaths. Because changedPaths includes all modified files in the diff, any modification to a non-dbt file (e.g., README.md or package.json) after the manifest was generated will trigger a false positive stale manifest warning.
You should filter the files before checking their mtimeMs.
Suggested change:
// Only compare against tracked, on-disk model files (schema.yml or SQL)
if (!/\.(sql|yml|yaml|py|csv)$/i.test(rel)) continue
const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel)
5. packages/opencode/src/altimate/review/orchestrate.ts (L1226-L1232)
[🔴 HIGH] 1. Logic bug with renamed files:
Using file.status === "modified" skips fetching oldContent for renamed files (where status is "renamed"). Since the detector relies on comparing oldContent with newContent, any guardrail tests removed during a file rename will be completely missed. You should use file.status !== "added" instead to ensure oldContent is fetched for both renamed and modified files.
2. Performance (Async in Loops):
Fetching file contents sequentially inside a for...of loop creates a performance bottleneck. As per the async handling checklist, independent async operations should be parallelized.
While the direct syntax fix for the status check is provided in the suggested replacement, consider refactoring the entire loop to use Promise.all concurrently:
const schemaFindings = await Promise.all(
reviewable
.filter((file) => file.kind === "schema_yml")
.map(async (file) => {
const oldRef = file.oldPath ?? file.path
const [oldContent, newContent] = await Promise.all([
file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
])
return detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })
})
)
all.push(...schemaFindings)Suggested change:
if (file.kind !== "schema_yml") continue
const oldRef = file.oldPath ?? file.path
const [oldContent, newContent] = await Promise.all([
file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
])
all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent }))
| // Deduplicate identical (model, column, test) triples so the fallback | ||
| // path emits at most one finding per removed test-line pattern. | ||
| const seen = new Set<string>() | ||
| removals = removals.filter((r) => { | ||
| const k = `${r.model}\x00${r.column}\x00${r.test}` | ||
| if (seen.has(k)) return false | ||
| seen.add(k) | ||
| return true | ||
| }) |
There was a problem hiding this comment.
[🟠 MEDIUM] In fallback mode, model and column are empty strings, so the deduplication key is effectively just \x00\x00${test}. If multiple tests of the same type are removed, they will be collapsed into a single finding. This loses the total count of removed tests that the previous implementation provided (e.g., "removed 3 data test(s)"). Consider preserving the raw count of removed lines in fallback mode so the scale of test regressions is not understated to users.
There was a problem hiding this comment.
Addressed in 6729f5374. Fallback path now emits one finding per removed test-line (dedup removed, occurrence-index discriminator added to ruleKey so global fingerprint dedupe doesn't collapse them). Aggregate "removes N tests" line preserved via file-scoped summaryEmitted — appended once to the first finding, listing distinct models when structural attribution is available (empty when it isn't). See #3619947164 for the exact fix + regression test.
| const isFirstForModel = !!r.model && !seenModel.has(r.model) | ||
| if (r.model) seenModel.add(r.model) | ||
| const bodyTail = | ||
| isFirstForModel && removals.length > 1 | ||
| ? `\n\n_This PR removes ${removals.length} data tests in total on model(s) ` + | ||
| `${[...new Set(removals.map((x) => x.model).filter(Boolean))].map((m) => `\`${m}\``).join(", ")}._` | ||
| : "" |
There was a problem hiding this comment.
[🟠 MEDIUM] If a PR removes tests from multiple models, isFirstForModel will evaluate to true for the first finding of each distinct model. This means the exact same global summary ("This PR removes X data tests in total on model(s) A, B") will be redundantly appended to multiple findings in the review, causing comment clutter. Consider tracking a single boolean flag (e.g., let summaryPrinted = false) to ensure the global summary is only appended once to the very first finding generated for the file.
There was a problem hiding this comment.
Addressed in 6729f5374. Replaced the per-model isFirstForModel guard with a single file-scoped summaryEmitted boolean. The aggregate "This PR removes N data tests in total on model(s) …" line now appears exactly once per file — on the first finding — and the model list is computed from the whole removals array. New test at review-dbt-patterns.test.ts:454 asserts multi-model diffs produce ONE summary line, not one per model.
| try { | ||
| newDoc = YAML.parse(opts.newContent) | ||
| } catch { | ||
| newDoc = undefined | ||
| } | ||
| if (opts.oldContent !== undefined) { | ||
| try { | ||
| oldDoc = YAML.parse(opts.oldContent) | ||
| } catch { | ||
| oldDoc = undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
[🔵 LOW] When YAML parsing fails, the exception is caught and silently ignored, which demotes the analysis to the string-diff fallback path. Consider adding a debug or telemetry log within the catch block. This would aid in diagnosing issues where valid-looking PRs fail structural parsing simply due to minor YAML syntax errors.
There was a problem hiding this comment.
Addressed in 6729f5374. Both YAML.parse failure branches now log via Log.create({ service: "review", tag: "detectSchemaYmlPatterns" }).warn(…) including the file path and error message, then fall through to the diff-based fallback (not fail-open). Diagnosing a valid-looking PR that fails structural parsing is now grep-friendly in the review logs.
| // Only compare against tracked, on-disk model files (schema.yml or SQL) | ||
| const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel) |
There was a problem hiding this comment.
[🟠 MEDIUM] Although the comment explicitly states an intention to only compare against tracked, on-disk model files (schema.yml or SQL), there is no actual file extension filtering applied to changedPaths. Because changedPaths includes all modified files in the diff, any modification to a non-dbt file (e.g., README.md or package.json) after the manifest was generated will trigger a false positive stale manifest warning.
You should filter the files before checking their mtimeMs.
Suggested change:
| // Only compare against tracked, on-disk model files (schema.yml or SQL) | |
| const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel) | |
| // Only compare against tracked, on-disk model files (schema.yml or SQL) | |
| if (!/\.(sql|yml|yaml|py|csv)$/i.test(rel)) continue | |
| const abs = path.isAbsolute(rel) ? rel : path.join(cwd, rel) |
There was a problem hiding this comment.
Addressed in 6729f5374. See #3619684688 — introduced isManifestAffecting(rel) filter in run.ts, admits only dbt-relevant extensions under dbt source directories (plus root config files). Table-driven positive/negative tests in the new review-run-stale.test.ts file cover README.md, models/foo.sql, docs/foo.md, models/foo.md, seeds/lookup.csv, snapshots/.sql, snapshots/.yml, dbt_project.yml, .github/workflows/, target/…
| if (file.kind !== "schema_yml") continue | ||
| const oldRef = file.oldPath ?? file.path | ||
| const [oldContent, newContent] = await Promise.all([ | ||
| file.status === "modified" ? getContent?.(oldRef, "old") : Promise.resolve(undefined), | ||
| file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined), | ||
| ]) | ||
| all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })) |
There was a problem hiding this comment.
[🔴 HIGH] 1. Logic bug with renamed files:
Using file.status === "modified" skips fetching oldContent for renamed files (where status is "renamed"). Since the detector relies on comparing oldContent with newContent, any guardrail tests removed during a file rename will be completely missed. You should use file.status !== "added" instead to ensure oldContent is fetched for both renamed and modified files.
2. Performance (Async in Loops):
Fetching file contents sequentially inside a for...of loop creates a performance bottleneck. As per the async handling checklist, independent async operations should be parallelized.
While the direct syntax fix for the status check is provided in the suggested replacement, consider refactoring the entire loop to use Promise.all concurrently:
const schemaFindings = await Promise.all(
reviewable
.filter((file) => file.kind === "schema_yml")
.map(async (file) => {
const oldRef = file.oldPath ?? file.path
const [oldContent, newContent] = await Promise.all([
file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined),
])
return detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })
})
)
all.push(...schemaFindings)Suggested change:
| if (file.kind !== "schema_yml") continue | |
| const oldRef = file.oldPath ?? file.path | |
| const [oldContent, newContent] = await Promise.all([ | |
| file.status === "modified" ? getContent?.(oldRef, "old") : Promise.resolve(undefined), | |
| file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined), | |
| ]) | |
| all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })) | |
| if (file.kind !== "schema_yml") continue | |
| const oldRef = file.oldPath ?? file.path | |
| const [oldContent, newContent] = await Promise.all([ | |
| file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined), | |
| file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined), | |
| ]) | |
| all.push(detectSchemaYmlPatterns(file, input.rubric, { oldContent, newContent })) |
There was a problem hiding this comment.
Addressed in 6729f5374. Both parts of your comment: (1) status gate changed to file.status !== "added" (covers renamed files); (2) loop refactored to Promise.all across schema files (concurrent) with per-file inner Promise.all for old/new content. Uses roughly the exact shape you suggested. See #3619993834 (rename regression test) and #3619993862 (perf note).
…ame bypass, snapshot yml, +5 more) After the earlier fixes on `b32065b23`, three independent reviewer bots (coderabbitai, cubic-dev-ai, dev-punia-altimate) surfaced 8 more issues on the schema.yml test-removal path — 2 HIGH, 1 MAJOR, 4 MEDIUM, 1 LOW — plus an independent local review round after that caught an additional blocker downstream of the fixes. This commit addresses each. ## BLOCKER — fallback distinct-removal fix defeated by global dedupe The R18-follow-up fix removed a local dedup in the fallback path so each removed test-line produced its own detector-level finding. But `runReview` runs a global `dedupe(merged)` step that fingerprints findings by (category, file, model, column, ruleKey) via `finding.ts:107`. For fallback findings sharing `file`, empty `model`, empty `column`, and a ruleKey varying only by `test`, two `unique` removals in the same file still collapsed to one downstream. The detector-level test at `review-dbt-patterns.test.ts:397` didn't catch it because it asserts against the pre-dedupe output. Fix: give fallback findings a per-diff occurrence-index discriminator appended to the ruleKey (`.#0`, `.#1`, …) so distinct removals get distinct fingerprints. Structural (attributed) findings unchanged — `(model.column.test)` is already unique per removal. Added an integration-shape regression test at `review-dbt-patterns.test.ts:432` that pipes detector output through `dedupe(f)` and asserts 4 distinct ids survive for 4 distinct removals. Also a same-shape guard for the structural path. ## HIGH — renamed schema.yml bypasses detector The orchestrator gated `oldContent` fetch on `file.status === "modified"`. A schema.yml being renamed (e.g. moved to a new subdir) that also dropped a `unique`/`not_null`/`relationships` guardrail silently skipped the detector because `oldContent` came back undefined and the structural path treated the file as newly-added. Fix: gate on `file.status !== "added"` so renamed files fetch the old side; separately, changed the detector so an undefined `oldContent` on a modified/renamed file falls through to the diff-based fallback (does not silently treat as added-file). Added test at `review-dbt-patterns.test.ts:332` covering the rename shape. ## MAJOR — structural path treats `oldContent=undefined` as added file `canUseStructural = newContent !== undefined && (isAddedFile || oldContent !== undefined)`. When the content resolver returns undefined on a modified/renamed file for a transient reason (git failure, ref not readable), we now leave `usedStructural = false` and let the fallback surface the raw diff removals — instead of producing an empty structural-diff and silently dropping every removal in the diff. ## MEDIUM — snapshot YAML property files never reached the detector `classifyDbtFile` matched everything under `snapshots/` as `snapshot` regardless of extension, so `snapshots/*.yml` never reached the `schema_yml` gate in the orchestrator. The new `snapshots:` branch added to the structural walker was dead code for real snapshot property files. Fix: split the classification by extension. `snapshots/*.sql` remains `snapshot` (unchanged tier-forcing / catalog rules). `snapshots/*.yml` classifies as `schema_yml` (routes to the test-removal detector). Added `classifyDbtFile` assertions for both extensions and an end-to-end structural-detector test on a snapshot yml. ## MEDIUM — redundant per-model summary line `isFirstForModel` fired once per distinct model, so a diff touching two models produced two copies of the same aggregate summary ("This PR removes N tests total on model(s) A, B") appended to separate findings. Fix: replaced with a single file-scoped `summaryEmitted` boolean. The aggregate summary is appended to the first finding for the file, once, and the distinct-models list is computed from the whole removals array. Test at `review-dbt-patterns.test.ts:454` asserts exactly one finding carries the aggregate line when two models have removals. ## MEDIUM — `warnIfStale` fired on unrelated file changes `warnIfStale` compared mtimes for every changed path. Any `README.md` / `package.json` / `.github/*` change post-manifest triggered a false-positive stale warning. Fix: introduced `isManifestAffecting()` — admits only `.sql|.py|.yml|.yaml|.csv|.md` under `models|seeds|snapshots|macros|tests|analyses/`, plus root `dbt_project.yml|packages.yml|profiles.yml|dependencies.yml`. Explicitly admits `.md` under `models/` because dbt docs blocks live there. Exported for tests; added a table-driven test file `review-run-stale.test.ts` covering both admitted and rejected cases (README.md, models/foo.sql, docs/foo.md, models/foo.md, seeds/lookup.csv, snapshots/*.sql, snapshots/*.yml, dbt_project.yml, .github/workflows/, target/…). ## LOW — YAML parse errors silently ignored Both `YAML.parse` failures (new content, old content) now log via `Log.create({ service: "review", tag: "detectSchemaYmlPatterns" })` with the file path and error message, then fall through to the diff-based fallback. Not fail-open. ## PERF — sequential schema.yml `getContent` calls The orchestrator's schema.yml loop was serial `await` per file; schema-heavy PRs paid one round-trip per file. Refactored to `Promise.all` across the file list with per-file `Promise.all` for old/new. Ordering preserved by `Promise.all` contract. ## Test status 188 pass / 0 fail across the 7 review test files. Typecheck clean. End-to-end sanity on the js2 test-removal scenario: CLI still surfaces both `unique` and `not_null` warnings, verdict `COMMENT/trivial/2`, envelope signature verifies.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Bot-review round-2 addressed — pushed as
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 3 finding(s)
- 3 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/review/orchestrate.ts (L1233-L1241)
[🟠 MEDIUM] The change file.status !== "added" to capture renames also inadvertently includes deleted files. This causes an unnecessary fetch of oldContent (which could be an expensive operation like git show) for deleted files.
Since detectSchemaYmlPatterns immediately returns [] when file.status === "deleted", we can optimize this by filtering out deleted files upfront. This also simplifies the newContent condition.
Suggested change:
const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted")
if (schemaFiles.length) {
const schemaFindingSets = await Promise.all(
schemaFiles.map(async (file) => {
const oldRef = file.oldPath ?? file.path
const [oldContent, newContent] = await Promise.all([
file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined),
getContent?.(file.path, "new"),
])
2. packages/opencode/src/altimate/review/dbt-patterns.ts (L1100-L1108)
[🟠 MEDIUM] The distinctModels and modelClause variables are recomputed on every iteration of the loop, but they are only used when shouldEmitSummary is true (which evaluates to true only once per file). This causes unnecessary object allocations and redundant O(N) operations in every loop iteration.
Consider moving this computation inside an if (shouldEmitSummary) block, or precomputing the string once before the loop.
Suggested change:
const shouldEmitSummary = !summaryEmitted && removals.length > 1
if (shouldEmitSummary) summaryEmitted = true
let bodyTail = ""
if (shouldEmitSummary) {
const distinctModels = [...new Set(removals.map((x) => x.model).filter(Boolean))]
const modelClause = distinctModels.length
? ` on model(s) ${distinctModels.map((m) => `\`${m}\``).join(", ")}`
: ""
bodyTail = `\n\n_This PR removes ${removals.length} data tests in total${modelClause}._`
}
3. packages/opencode/src/altimate/review/dbt-patterns.ts (L1116)
[🔵 LOW] Calling shift() on an array inside a loop yields O(N^2) time complexity because shift() forces a re-indexing of all remaining elements in the array. While the array length here might be small, it is a good practice to avoid array mutation inside loops when a simple index access suffices.
Consider defining an index variable (e.g. let fallbackIdx = 0;) before the for (const r of removals) loop, and using it here instead.
Suggested change:
const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators[fallbackIdx++] ?? "#?"}`
| const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml") | ||
| if (schemaFiles.length) { | ||
| const schemaFindingSets = await Promise.all( | ||
| schemaFiles.map(async (file) => { | ||
| const oldRef = file.oldPath ?? file.path | ||
| const [oldContent, newContent] = await Promise.all([ | ||
| file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined), | ||
| file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined), | ||
| ]) |
There was a problem hiding this comment.
[🟠 MEDIUM] The change file.status !== "added" to capture renames also inadvertently includes deleted files. This causes an unnecessary fetch of oldContent (which could be an expensive operation like git show) for deleted files.
Since detectSchemaYmlPatterns immediately returns [] when file.status === "deleted", we can optimize this by filtering out deleted files upfront. This also simplifies the newContent condition.
Suggested change:
| const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml") | |
| if (schemaFiles.length) { | |
| const schemaFindingSets = await Promise.all( | |
| schemaFiles.map(async (file) => { | |
| const oldRef = file.oldPath ?? file.path | |
| const [oldContent, newContent] = await Promise.all([ | |
| file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined), | |
| file.status !== "deleted" ? getContent?.(file.path, "new") : Promise.resolve(undefined), | |
| ]) | |
| const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted") | |
| if (schemaFiles.length) { | |
| const schemaFindingSets = await Promise.all( | |
| schemaFiles.map(async (file) => { | |
| const oldRef = file.oldPath ?? file.path | |
| const [oldContent, newContent] = await Promise.all([ | |
| file.status !== "added" ? getContent?.(oldRef, "old") : Promise.resolve(undefined), | |
| getContent?.(file.path, "new"), | |
| ]) |
There was a problem hiding this comment.
Addressed in 39718d5d1. schemaFiles now filters out status === "deleted" upfront in orchestrate.ts — deleted schema.yml files never enter the fetch loop, so the redundant git show round-trip is gone. newContent fetch simplifies to unconditional getContent?.(file.path, "new") since deleted files can't reach it anymore. Used the exact shape from your suggestion.
| const shouldEmitSummary = !summaryEmitted && removals.length > 1 | ||
| if (shouldEmitSummary) summaryEmitted = true | ||
| const distinctModels = [...new Set(removals.map((x) => x.model).filter(Boolean))] | ||
| const modelClause = distinctModels.length | ||
| ? ` on model(s) ${distinctModels.map((m) => `\`${m}\``).join(", ")}` | ||
| : "" | ||
| const bodyTail = shouldEmitSummary | ||
| ? `\n\n_This PR removes ${removals.length} data tests in total${modelClause}._` | ||
| : "" |
There was a problem hiding this comment.
[🟠 MEDIUM] The distinctModels and modelClause variables are recomputed on every iteration of the loop, but they are only used when shouldEmitSummary is true (which evaluates to true only once per file). This causes unnecessary object allocations and redundant O(N) operations in every loop iteration.
Consider moving this computation inside an if (shouldEmitSummary) block, or precomputing the string once before the loop.
Suggested change:
| const shouldEmitSummary = !summaryEmitted && removals.length > 1 | |
| if (shouldEmitSummary) summaryEmitted = true | |
| const distinctModels = [...new Set(removals.map((x) => x.model).filter(Boolean))] | |
| const modelClause = distinctModels.length | |
| ? ` on model(s) ${distinctModels.map((m) => `\`${m}\``).join(", ")}` | |
| : "" | |
| const bodyTail = shouldEmitSummary | |
| ? `\n\n_This PR removes ${removals.length} data tests in total${modelClause}._` | |
| : "" | |
| const shouldEmitSummary = !summaryEmitted && removals.length > 1 | |
| if (shouldEmitSummary) summaryEmitted = true | |
| let bodyTail = "" | |
| if (shouldEmitSummary) { | |
| const distinctModels = [...new Set(removals.map((x) => x.model).filter(Boolean))] | |
| const modelClause = distinctModels.length | |
| ? ` on model(s) ${distinctModels.map((m) => `\`${m}\``).join(", ")}` | |
| : "" | |
| bodyTail = `\n\n_This PR removes ${removals.length} data tests in total${modelClause}._` | |
| } |
There was a problem hiding this comment.
Addressed in 39718d5d1. Moved the distinctModels / modelClause computation inside the if (shouldEmitSummary) block — no Set/spread/filter/map/join on iterations where the summary isn't attached (all but the first). summaryEmitted flip also moved inside the block.
| // distinct removals of the same test type — we append the fallback | ||
| // discriminator so each removed line becomes a distinct finding | ||
| // downstream of the global dedupe. | ||
| const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators.shift() ?? "#?"}` |
There was a problem hiding this comment.
[🔵 LOW] Calling shift() on an array inside a loop yields O(N^2) time complexity because shift() forces a re-indexing of all remaining elements in the array. While the array length here might be small, it is a good practice to avoid array mutation inside loops when a simple index access suffices.
Consider defining an index variable (e.g. let fallbackIdx = 0;) before the for (const r of removals) loop, and using it here instead.
Suggested change:
| const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators.shift() ?? "#?"}` | |
| const discriminator = attributed || modelLevel ? "" : `.${fallbackDiscriminators[fallbackIdx++] ?? "#?"}` |
There was a problem hiding this comment.
Addressed in 39718d5d1. Hoisted let fallbackIdx = 0 before the for (const r of removals) loop and swapped fallbackDiscriminators.shift() for fallbackDiscriminators[fallbackIdx++]. Discriminator array is no longer mutated inside the loop; index-based access is O(1) per iteration.
…ry 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).
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
3 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/review/dbt-patterns.ts">
<violation number="1" location="packages/opencode/src/altimate/review/dbt-patterns.ts:1132">
P3: Column-aware removal findings lose their structured model/column attribution: consumers receive only title/body text while `Finding.model` and `Finding.column` stay unset. Include the extracted tuple on the finding, not only in `evidence.result`.</violation>
</file>
<file name="packages/opencode/src/altimate/review/format.ts">
<violation number="1" location="packages/opencode/src/altimate/review/format.ts:60">
P3: Tier explanations render incorrectly for changed paths containing backticks, because each reason is wrapped with a fixed one-backtick delimiter. Choose a delimiter longer than any backtick run in `r` so path text cannot terminate the code span or inject Markdown into the summary.</violation>
</file>
<file name="packages/opencode/src/altimate/review/orchestrate.ts">
<violation number="1" location="packages/opencode/src/altimate/review/orchestrate.ts:1238">
P2: Deleting a schema file can remove every guardrail for models that remain in the project, but this filter prevents the test-removal detector from seeing it. Consider analyzing the old document against an empty new document (or emitting a file-level removal finding) for deleted schema files.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // early-returns `[]` for `status === "deleted"` anyway; not filtering here | ||
| // would trigger an unnecessary `getContent(oldRef, "old")` fetch (a git-show | ||
| // round-trip) whose result is thrown away by the detector. | ||
| const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted") |
There was a problem hiding this comment.
P2: Deleting a schema file can remove every guardrail for models that remain in the project, but this filter prevents the test-removal detector from seeing it. Consider analyzing the old document against an empty new document (or emitting a file-level removal finding) for deleted schema files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/orchestrate.ts, line 1238:
<comment>Deleting a schema file can remove every guardrail for models that remain in the project, but this filter prevents the test-removal detector from seeing it. Consider analyzing the old document against an empty new document (or emitting a file-level removal finding) for deleted schema files.</comment>
<file context>
@@ -1196,8 +1217,37 @@ export async function runReview(input: OrchestrateInput): Promise<VerdictEnvelop
+ // early-returns `[]` for `status === "deleted"` anyway; not filtering here
+ // would trigger an unnecessary `getContent(oldRef, "old")` fetch (a git-show
+ // round-trip) whose result is thrown away by the detector.
+ const schemaFiles = reviewable.filter((f) => f.kind === "schema_yml" && f.status !== "deleted")
+ if (schemaFiles.length) {
+ const schemaFindingSets = await Promise.all(
</file context>
| title, | ||
| body: bodyLead + bodyRationale + bodyTail, | ||
| file: file.path, | ||
| confidence: "high", |
There was a problem hiding this comment.
P3: Column-aware removal findings lose their structured model/column attribution: consumers receive only title/body text while Finding.model and Finding.column stay unset. Include the extracted tuple on the finding, not only in evidence.result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/dbt-patterns.ts, line 1132:
<comment>Column-aware removal findings lose their structured model/column attribution: consumers receive only title/body text while `Finding.model` and `Finding.column` stay unset. Include the extracted tuple on the finding, not only in `evidence.result`.</comment>
<file context>
@@ -790,33 +792,357 @@ export function detectModelPatterns(file: ChangedFile, newSql: string | undefine
+ title,
+ body: bodyLead + bodyRationale + bodyTail,
+ file: file.path,
+ confidence: "high",
+ evidence: {
+ tool: "dbt-patterns",
</file context>
| confidence: "high", | |
| model: r.model || undefined, | |
| column: r.column || undefined, | |
| confidence: "high", |
| // the signed envelope's tierReasons[]; the summary shows the first 8. | ||
| if (env.tierReasons && env.tierReasons.length) { | ||
| const RENDER_CAP = 8 | ||
| const shown = env.tierReasons.slice(0, RENDER_CAP).map((r) => `\`${r}\``).join(", ") |
There was a problem hiding this comment.
P3: Tier explanations render incorrectly for changed paths containing backticks, because each reason is wrapped with a fixed one-backtick delimiter. Choose a delimiter longer than any backtick run in r so path text cannot terminate the code span or inject Markdown into the summary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/review/format.ts, line 60:
<comment>Tier explanations render incorrectly for changed paths containing backticks, because each reason is wrapped with a fixed one-backtick delimiter. Choose a delimiter longer than any backtick run in `r` so path text cannot terminate the code span or inject Markdown into the summary.</comment>
<file context>
@@ -43,6 +48,23 @@ export function renderSummary(env: VerdictEnvelope): string {
+ // the signed envelope's tierReasons[]; the summary shows the first 8.
+ if (env.tierReasons && env.tierReasons.length) {
+ const RENDER_CAP = 8
+ const shown = env.tierReasons.slice(0, RENDER_CAP).map((r) => `\`${r}\``).join(", ")
+ const overflow =
+ env.tierReasons.length > RENDER_CAP
</file context>
| const shown = env.tierReasons.slice(0, RENDER_CAP).map((r) => `\`${r}\``).join(", ") | |
| const shown = env.tierReasons | |
| .slice(0, RENDER_CAP) | |
| .map((r) => { | |
| const fence = "`".repeat(Math.max(1, ...(r.match(/`+/g)?.map((run) => run.length + 1) ?? []))) | |
| return `${fence}${r}${fence}` | |
| }) | |
| .join(", ") |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
2 participants
Summary
Improvements to
altimate-code reviewcovering two bug fixes and four observability / recall features that share the same code surface. Filed againstpackages/opencode/src/{cli/cmd/review.ts, altimate/review/*.ts}.Bug fixes
Both are reproducible against
mainand closed by this PR:--no-aiflag triggers help path instead of running the review #1025 — bare--no-ai <other-flag>triggers the yargs help path and exits 0 without running the review. Fix:.parserConfiguration({ "boolean-negation": false })on the review command's yargs builder, so--no-aibinds to the declarednoAioption as authored.--no-ai=true/--no-ai=falsecontinue to work for programmatic parity.detectSchemaYmlPatternscompared removed and added test lines as raw trimmed strings, so a genuine removal from one column was silently cancelled by an unrelated re-add on a sibling column (yaml re-serialization emits the same- unique/- not_nulllines for other columns still declaring those tests). Fix:collectTestOccurrencesFromDiffindbt-patterns.tswalks the unified diff with model/column context via nearest preceding- name: Xheader (resets on hunk boundaries), keys removals by(model, column, test), and only cancels on a same-tuple re-add. Emits one finding per removed(model, column, test)triple, with severity elevated towarningforuniqueremovals and fornot_nullon id/key columns. Body text detects layer from file path (marts?/reporting→ "mart-layer key", else "declared key") so the copy doesn't mislabel non-mart schema files.Feature enhancements bundled with the fixes
Four additions in the same code surface, small enough to review alongside the bug fixes rather than as their own PRs:
--explain-tier— new boolean flag. When set, the verdict envelope carriestierReasons: string[](the risk classifier's reasons for the tier decision) and the human-readable summary prints a🧭 **Tier: X** — <reasons>line. Read-only — doesn't change tier classification or verdicts. Envelope field is optional (absent when the flag is off), so existing verdicts remain byte-equivalent.--force-tier <trivial|lite|full>— new experimental flag for bench and debug work. Overrides the tier classifier with the supplied tier. Guardrails:⚠️ --force-tier=X is EXPERIMENTAL (bench / debug only). Classifier bypassed; verdict envelope will carry tierForced: true.warning to stderr on every use.tierForced: trueandtierClassified: <original>whenever the flag is passed, regardless of whether the forced value matches the classifier's decision. (An earlier iteration of this change guarded that flag oninput.forceTier !== classifiedTier, which meant--force-tier=fullon a naturally-fullPR silently bypassed the audit envelope. Corrected in the same branch — every use of the flag is now recorded.)<tier> tier — forced (was <classified>).tierReasonsis always populated when the flag is used, with a leadingforced via --force-tier=X (classifier said Y)marker so downstream can't confuse the forced tier for a natural one.describe; the stderr banner ensures no one uses it in CI without noticing.Manifest auto-discovery — before,
--manifest <path>had to be explicit; otherwise the CLI used the config-relative defaulttarget/manifest.json, which silently missed whencwdwasn't the dbt project root, degrading every such review to lint-only. Now:--manifestisn't explicit AND the config-relative path doesn't exist, walk UP fromcwdlooking fordbt_project.yml. Use the adjacenttarget/manifest.jsonwhen it exists.ℹ️ auto-discovered dbt manifest at ...) so users see which manifest the review used.target/from an unrelated tool never gets picked up.--headis set (CI shape) AND any changed file has an mtime newer than the manifest, print⚠️ manifest ... appears staleto stderr. Skipped for working-tree diffs (mtime noise during active edits would spam warnings).Explicit
--manifest <path>always wins. Auto-discovery is only attempted when the caller was silent AND the config default is absent.Verdict envelope — three new optional fields added to
VerdictEnvelopeinverdict.ts, all included in the signed canonical body so tampering is detectable:tierReasons?: string[](from--explain-tier)tierForced?: boolean(from--force-tier)tierClassified?: RiskTier(from--force-tier— original tier before the override)All are optional and absent when the corresponding flag is off, so existing verdicts remain byte-equivalent to before.
Testing
Verified locally on synthetic scenarios:
--no-ainow runs the review (verifies bug fix for review: bare--no-aiflag triggers help path instead of running the review #1025).--no-ai=trueand--no-ai=falsecontinue to work.--explain-tierpopulatestierReasonson bothtrivialandfullverdicts.--force-tier=fullon a naturally-fullPR setstierForced: trueandtierClassified: "full"on the envelope + emits the stderr warning + the leadingforced viareason.--force-tier=liteon a naturally-fullPR overrides tier toliteand recordstierClassified: "full".target/manifest.jsonwhen invoked from a subdirectory containingdbt_project.ymlupward; declines to use atarget/manifest.jsonfrom a directory that has nodbt_project.yml(verified against a plain non-dbt git repo).unique+not_nullfrom one column while sibling columns still have them) now surfaces twowarning-severity findings, one per removed(column, test)tuple. Layer-aware body renders "declared key" for a top-levelmodels/schema.ymland "mart-layer key" for amodels/marts/schema.yml.verifyEnvelopefor both flag-on and flag-off shapes.Test plan
--no-airuns the review (repro from review: bare--no-aiflag triggers help path instead of running the review #1025 no longer occurs)--no-ai=true/--no-ai=falsebehavior unchanged--explain-tierproducestierReasonsin JSON output; envelope signature verifies--force-tierprints EXPERIMENTAL warning to stderr; envelope carriestierForced: true+tierClassified: <original>even when forced tier matches classifiertarget/manifest.jsonfrom subdir; explicit--manifeststill wins; auto-discovery declines outside a dbt project--headis set; silent otherwise(column, test)findingsmainCloses #1025.
Closes #1026.
Summary by cubic
Fixes bare
--no-ainot running, upgrades schema.yml test-removal to a structural YAML diff, and adds tier observability/override plus manifest auto-discovery wired to compiled SQL, with minor perf cleanups. Follow-ups cover rename handling, snapshot YAML classification, stale-manifest noise, and distinct-removal dedup, improving recall and auditability. Closes #1025 and #1026.Bug Fixes
--no-ainow runs instead of falling into help.data_tests; falls back to diff-only when content isn’t available; fixes sibling-column cancels; emits one finding per removal; keeps elevated severity foruniqueandnot_nullon likely key columns; labels by file path; works on renamed files; logs YAML parse errors; snapshot property files undersnapshots/*.ymlnow route here; the per-file summary line is emitted once; fallback findings carry per-occurrence tags to survive dedupe.tierClassifiedis missing; envelope always recordstierForced: trueandtierClassifiedwhen forcing; summary caps renderedtierReasonsto the first 8 with a “+N more in verdict envelope” marker.--headis set) now considers only dbt-relevant paths to avoid false positives.oldContentfetch for deleted schema.yml files; compute the per-file summary only when needed; replace an O(N²)shift()with an index for fallback rule-key tags.New Features
--explain-tier: addstierReasons[]to the envelope and prints a tier line in the summary (render capped as above).--force-tier <trivial|lite|full>(experimental): overrides the classifier with a stderr warning; envelope recordstierForced: trueandtierClassified; reasons include a leading “forced via …” marker.--manifestis omitted and the config default is missing, walks up to the nearestdbt_project.ymland uses itstarget/manifest.json; logs the chosen path; never picks a manifest outside a dbt project; warns if stale when--headis set; routes compiled SQL resolution and project name to the discovered project root. Explicit--manifestalways wins.tierReasons,tierForced, andtierClassified(all signed). Backwards compatible when flags are unused.Written for commit 39718d5. Summary will update on new commits.
Summary by CodeRabbit
--explain-tierto include tier reasoning in the verdict output.--force-tierto override tier selection while preserving the original classification.schema.ymlpattern detection to precisely attribute removedunique/not_null/relationships(includingdata_tests) to the correct model/source/snapshot, column, and test type using structural diffing with a safer fallback.--no-ai.snapshots/to avoid misrouting.