feat: dbt_project_health tool — reuse altimate-core dbt health checks - #1030
feat: dbt_project_health tool — reuse altimate-core dbt health checks#1030mdesmet wants to merge 2 commits into
Conversation
Wires the new `altimate_core::dbt::health` checks (altimate-core-internal, requires @altimateai/altimate-core 0.8.0) into altimate-code: - Bridge method type `altimate_core.dbt_project_health` + Dispatcher registration calling core.dbtProjectHealth. - New DbtProjectHealthTool: runs dbt health checks directly against a dbt project directory (no manifest required), optional config + catalog paths, formats findings grouped by severity. - Registers the tool in the tool registry and altimate barrel export; bumps the @altimateai/altimate-core dependency 0.5.1 -> 0.8.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
|
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. |
📝 WalkthroughWalkthroughAdds built-in Changesdbt health tooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant DbtHealthConfigTool
participant DbtProjectHealthTool
participant Dispatcher
participant AltimateCore
Command->>DbtHealthConfigTool: Infer and write dbt health config
DbtHealthConfigTool->>Dispatcher: Call altimate_core.dbt_health_infer_config
Dispatcher->>AltimateCore: Send project directory
AltimateCore-->>Dispatcher: Return inferred YAML
DbtHealthConfigTool-->>Command: Return written path and check count
Command->>DbtProjectHealthTool: Evaluate project health
DbtProjectHealthTool->>Dispatcher: Call altimate_core.dbt_project_health
Dispatcher->>AltimateCore: Send project, config, and catalog paths
AltimateCore-->>Dispatcher: Return JSON findings
DbtProjectHealthTool-->>Command: Return sorted findings and severity counts
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
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/tools/dbt-project-health.ts`:
- Around line 37-40: Update the config_path handling in the dbt project health
tool so an explicitly supplied configuration file that is missing or unreadable
produces a clear tool error instead of falling back to default checks. Validate
file existence and propagate read failures, while preserving the current
behavior when config_path is not provided.
- Around line 57-63: Update the findings handling in the dbt project health flow
to validate result.data.findings with a runtime schema before copying or sorting
it. Reject malformed or missing payloads by returning the existing error
response, while preserving the current severity and code ordering for valid
HealthFinding arrays.
🪄 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: 71c27c6d-b7a3-4fad-9d71-6ce242129103
📒 Files selected for processing (6)
packages/opencode/package.jsonpackages/opencode/src/altimate/index.tspackages/opencode/src/altimate/native/altimate-core.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/dbt-project-health.tspackages/opencode/src/tool/registry.ts
| if (args.config_path) { | ||
| const file = Bun.file(args.config_path) | ||
| if (await file.exists()) config_json = await file.text() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently ignore an explicitly supplied config file.
When config_path is set but the file is missing or unreadable, the tool runs with default checks and reports results as if the requested configuration was applied. Return a clear tool error instead.
Suggested handling
if (args.config_path) {
- const file = Bun.file(args.config_path)
- if (await file.exists()) config_json = await file.text()
+ try {
+ config_json = await Bun.file(args.config_path).text()
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error)
+ return {
+ title: "dbt health: ERROR",
+ metadata: { error: message },
+ output: `Failed to read dbt health config: ${message}`,
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (args.config_path) { | |
| const file = Bun.file(args.config_path) | |
| if (await file.exists()) config_json = await file.text() | |
| } | |
| if (args.config_path) { | |
| try { | |
| config_json = await Bun.file(args.config_path).text() | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| return { | |
| title: "dbt health: ERROR", | |
| metadata: { error: message }, | |
| output: `Failed to read dbt health config: ${message}`, | |
| } | |
| } | |
| } |
🤖 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/tools/dbt-project-health.ts` around lines 37 -
40, Update the config_path handling in the dbt project health tool so an
explicitly supplied configuration file that is missing or unreadable produces a
clear tool error instead of falling back to default checks. Validate file
existence and propagate read failures, while preserving the current behavior
when config_path is not provided.
| const findings = ((result.data.findings as HealthFinding[] | undefined) ?? []).slice() | ||
| findings.sort( | ||
| (a, b) => (SEVERITY_ORDER[a.severity] ?? 3) - (SEVERITY_ORDER[b.severity] ?? 3) || a.code.localeCompare(b.code), | ||
| ) | ||
|
|
||
| const counts = { error: 0, warning: 0, info: 0 } as Record<string, number> | ||
| for (const f of findings) counts[f.severity] = (counts[f.severity] ?? 0) + 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/opencode/src/altimate/tools/dbt-project-health.ts
printf '\n---- search HealthFinding ----\n'
rg -n "HealthFinding|findings" packages/opencode/src/altimate -g '!**/dist/**' -g '!**/build/**'Repository: AltimateAI/altimate-code
Length of output: 27057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file ---\n'
nl -ba packages/opencode/src/altimate/tools/dbt-project-health.ts | sed -n '1,220p'
printf '\n--- search definitions ---\n'
rg -n "type HealthFinding|interface HealthFinding|Schema.*HealthFinding|findings:" packages/opencode/src/altimate -g '!**/dist/**' -g '!**/build/**'Repository: AltimateAI/altimate-code
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- altimate-core dbt_project_health references ---'
rg -n "dbt_project_health|HealthFinding|JSON.parse\\(json\\)" packages/opencode/src/altimate/native/altimate-core.ts packages/opencode/src/altimate/native -g '!**/dist/**' -g '!**/build/**'
printf '\n%s\n' '--- dispatcher definition ---'
rg -n "class Dispatcher|export const Dispatcher|call\\(" packages/opencode/src/altimate/native packages/opencode/src/altimate -g '!**/dist/**' -g '!**/build/**'Repository: AltimateAI/altimate-code
Length of output: 14037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- native altimate_core.dbt_project_health implementation ---'
nl -ba packages/opencode/src/altimate/native/altimate-core.ts | sed -n '260,285p'
printf '\n%s\n' '--- dispatcher return typing ---'
nl -ba packages/opencode/src/altimate/native/types.ts | sed -n '1238,1260p'
printf '\n%s\n' '--- dispatcher implementation ---'
rg -n "class Dispatcher|export class Dispatcher|static async call|async call" packages/opencode/src/altimate/native packages/opencode/src/altimate -g '!**/dist/**' -g '!**/build/**'Repository: AltimateAI/altimate-code
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- native altimate_core.dbt_project_health implementation ---'
nl -ba packages/opencode/src/altimate/native/altimate-core.ts | sed -n '260,285p'
printf '%s\n' '--- dispatcher return typing ---'
nl -ba packages/opencode/src/altimate/native/types.ts | sed -n '1238,1260p'
printf '%s\n' '--- dispatcher implementation ---'
rg -n "class Dispatcher|export class Dispatcher|static async call|async call" packages/opencode/src/altimate/native packages/opencode/src/altimate -g '!**/dist/**' -g '!**/build/**'Repository: AltimateAI/altimate-code
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- native altimate_core.dbt_project_health implementation ---'
sed -n '268,278p' packages/opencode/src/altimate/native/altimate-core.ts | cat -n
printf '\n%s\n' '--- native types entry ---'
sed -n '1242,1254p' packages/opencode/src/altimate/native/types.ts | cat -n
printf '\n%s\n' '--- dispatcher implementation files ---'
fd -a "dispatcher*.ts" packages/opencode/src/altimate packages/opencode/src -E dist -E buildRepository: AltimateAI/altimate-code
Length of output: 1545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dispatcher.ts ---'
sed -n '1,260p' packages/opencode/src/altimate/native/dispatcher.ts | cat -n
printf '\n%s\n' '--- AltimateCoreResult definition ---'
rg -n "type AltimateCoreResult|interface AltimateCoreResult|export type AltimateCoreResult" packages/opencode/src/altimate/native/types.ts packages/opencode/src/altimate/native -g '!**/dist/**' -g '!**/build/**'Repository: AltimateAI/altimate-code
Length of output: 3918
Validate the native findings payload at runtime. result.data.findings is only typed here; a malformed or missing bridge payload can throw on .slice()/sort() or be treated as zero findings. Parse it with a runtime schema and return an error on invalid data.
🤖 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/tools/dbt-project-health.ts` around lines 57 -
63, Update the findings handling in the dbt project health flow to validate
result.data.findings with a runtime schema before copying or sorting it. Reject
malformed or missing payloads by returning the existing error response, while
preserving the current severity and code ordering for valid HealthFinding
arrays.
Source: Coding guidelines
There was a problem hiding this comment.
1 issue found across 6 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/tools/dbt-project-health.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/dbt-project-health.ts:39">
P2: A misspelled or unavailable `config_path` silently runs default health checks and can report a clean result despite requested disabled checks or severity overrides not being applied. Return an explicit tool error when the config cannot be read instead of omitting it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let config_json: string | undefined | ||
| if (args.config_path) { | ||
| const file = Bun.file(args.config_path) | ||
| if (await file.exists()) config_json = await file.text() |
There was a problem hiding this comment.
P2: A misspelled or unavailable config_path silently runs default health checks and can report a clean result despite requested disabled checks or severity overrides not being applied. Return an explicit tool error when the config cannot be read instead of omitting it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/tools/dbt-project-health.ts, line 39:
<comment>A misspelled or unavailable `config_path` silently runs default health checks and can report a clean result despite requested disabled checks or severity overrides not being applied. Return an explicit tool error when the config cannot be read instead of omitting it.</comment>
<file context>
@@ -0,0 +1,88 @@
+ let config_json: string | undefined
+ if (args.config_path) {
+ const file = Bun.file(args.config_path)
+ if (await file.exists()) config_json = await file.text()
+ }
+
</file context>
…project config)
Adds a deterministic config-inference flow for the dbt health checks:
- `dbt_health_config` tool: calls the new core `dbtHealthInferConfig` (via the dispatcher)
and writes the inferred config to <project_dir>/.altimate/dbt-health.yml (per-project, so
multiple dbt projects each get their own file). Requires @altimateai/altimate-core 0.8.0.
- `dbt_project_health` now auto-discovers <project_dir>/.altimate/dbt-health.{yml,yaml,json}
when no explicit config_path is passed, so the inferred config is used automatically.
- `/dbt-health-config` slash command (.opencode/command): infers + writes the config for
each dbt project in the worktree, then runs the health checks to show the effect.
- Bridge type + dispatcher registration for altimate_core.dbt_health_infer_config.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/tools/dbt-health-config.ts`:
- Around line 39-40: Update the output-writing flow around mkdir and Bun.write
to prevent symlink escapes outside project_dir. Use the existing symlink-aware
containsReal check on the final output path and reject any symlinked directory
or file components before writing, or use an equivalent atomic no-follow file
creation approach; preserve normal writes for safe paths.
- Line 35: Update the output-path construction around outPath to use node:path’s
join with args.project_dir and DBT_HEALTH_CONFIG_RELPATH. Remove the manual
trailing-slash trimming and string concatenation so path separators are
normalized consistently across platforms.
🪄 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: 25a9ad29-0e52-48e6-ab98-5954341588fe
📒 Files selected for processing (7)
.opencode/command/dbt-health-config.mdpackages/opencode/src/altimate/index.tspackages/opencode/src/altimate/native/altimate-core.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/dbt-health-config.tspackages/opencode/src/altimate/tools/dbt-project-health.tspackages/opencode/src/tool/registry.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/opencode/src/altimate/index.ts
- packages/opencode/src/tool/registry.ts
- packages/opencode/src/altimate/native/altimate-core.ts
- packages/opencode/src/altimate/tools/dbt-project-health.ts
| } | ||
|
|
||
| const yaml = String((result.data as { config?: unknown }).config ?? "") | ||
| const outPath = `${args.project_dir.replace(/\/+$/, "")}/${DBT_HEALTH_CONFIG_RELPATH}` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline packages/opencode/src/altimate/tools/dbt-health-config.ts --view expanded || true
echo '--- relevant lines ---'
sed -n '1,120p' packages/opencode/src/altimate/tools/dbt-health-config.ts
echo '--- path semantics probe ---'
node - <<'JS'
const path = require('node:path');
const rel = 'dbt_health_config.yml';
const inputs = [
'/',
'/tmp/',
'C:/',
'C:/work/',
'C:\\',
'C:\\work\\',
'relative/',
];
for (const p of inputs) {
const stripped = `${p.replace(/\/+$/, '')}/${rel}`;
const joined = path.join(p, rel);
const posixJoined = path.posix.join(p, rel);
const winJoined = path.win32.join(p, rel);
console.log(JSON.stringify({ p, stripped, joined, posixJoined, winJoined }));
}
JSRepository: AltimateAI/altimate-code
Length of output: 4011
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' packages/opencode/src/altimate/tools/dbt-health-config.ts
node - <<'JS'
const path = require('node:path');
const rel = 'dbt_health_config.yml';
for (const p of ['/', '/tmp/', 'C:/', 'C:/work/', 'C:\\', 'C:\\work\\', 'relative/']) {
const stripped = `${p.replace(/\/+$/, '')}/${rel}`;
const joined = path.join(p, rel);
console.log(JSON.stringify({ p, stripped, joined }));
}
JSRepository: AltimateAI/altimate-code
Length of output: 3259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const path = require('node:path');
const samples = [
'/.altimate/dbt-health.yml',
'C:/.altimate/dbt-health.yml',
'C:\\/.altimate/dbt-health.yml',
'C:/work/.altimate/dbt-health.yml',
'C:\\work\\/.altimate/dbt-health.yml',
'//server/share/.altimate/dbt-health.yml',
];
for (const s of samples) {
console.log(JSON.stringify({
s,
posix_dirname: path.posix.dirname(s),
win_dirname: path.win32.dirname(s),
posix_isAbs: path.posix.isAbsolute(s),
win_isAbs: path.win32.isAbsolute(s),
normalized_win: path.win32.normalize(s),
}));
}
JSRepository: AltimateAI/altimate-code
Length of output: 1352
Build the output path with node:path. Manual trimming plus string concatenation can leave mixed separators on Windows paths; join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH) normalizes it consistently.
🤖 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/tools/dbt-health-config.ts` at line 35, Update
the output-path construction around outPath to use node:path’s join with
args.project_dir and DBT_HEALTH_CONFIG_RELPATH. Remove the manual trailing-slash
trimming and string concatenation so path separators are normalized consistently
across platforms.
| await mkdir(dirname(outPath), { recursive: true }) | ||
| await Bun.write(outPath, yaml) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline packages/opencode/src/altimate/tools/dbt-health-config.ts --view expanded || true
printf '\n== File contents (relevant slice) ==\n'
wc -l packages/opencode/src/altimate/tools/dbt-health-config.ts
sed -n '1,220p' packages/opencode/src/altimate/tools/dbt-health-config.ts | cat -n
printf '\n== Search for related path / symlink handling ==\n'
rg -n "symlink|realpath|canonical|containment|mkdir\\(|Bun\\.write\\(|dbt-health.yml|\\.altimate|project_dir|projectDir|outPath" packages/opencode/src -SRepository: AltimateAI/altimate-code
Length of output: 41320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== util/filesystem.ts excerpt ==\n'
sed -n '200,280p' packages/opencode/src/util/filesystem.ts | cat -n
printf '\n== write helpers / containment usages ==\n'
rg -n "containsReal|containsPath|mkdir\\(|writeFile\\(|Bun\\.write\\(" packages/opencode/src/util packages/opencode/src/altimate packages/opencode/src/cli -SRepository: AltimateAI/altimate-code
Length of output: 6872
Prevent symlink escapes in the write path. mkdir(..., { recursive: true }) plus Bun.write(outPath, yaml) can still follow a pre-existing .altimate directory or dbt-health.yml symlink and write outside project_dir. Reuse the symlink-aware containment check (containsReal) on the final path and reject symlinked output components, or create the file atomically with no-follow semantics.
🤖 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/tools/dbt-health-config.ts` around lines 39 -
40, Update the output-writing flow around mkdir and Bun.write to prevent symlink
escapes outside project_dir. Use the existing symlink-aware containsReal check
on the final output path and reject any symlinked directory or file components
before writing, or use an equivalent atomic no-follow file creation approach;
preserve normal writes for safe paths.
Source: Coding guidelines
There was a problem hiding this comment.
2 issues found across 7 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/tools/dbt-health-config.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/dbt-health-config.ts:35">
P3: The output path is built with manual slash-trimming and template concatenation, but `dirname` from `node:path` is already imported. Using `join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH)` would normalize separators consistently (particularly relevant on Windows) and is more idiomatic given the existing import.</violation>
<violation number="2" location="packages/opencode/src/altimate/tools/dbt-health-config.ts:39">
P2: The `mkdir` + `Bun.write` sequence will follow pre-existing symlinks. If `.altimate/` or `dbt-health.yml` is a symlink pointing outside `project_dir`, this writes attacker-controlled content to an arbitrary path. Consider resolving `outPath` after directory creation and verifying it still resides within `project_dir` (e.g., via a realpath containment check) before writing.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| let written = false | ||
| if (args.write !== false) { | ||
| await mkdir(dirname(outPath), { recursive: true }) |
There was a problem hiding this comment.
P2: The mkdir + Bun.write sequence will follow pre-existing symlinks. If .altimate/ or dbt-health.yml is a symlink pointing outside project_dir, this writes attacker-controlled content to an arbitrary path. Consider resolving outPath after directory creation and verifying it still resides within project_dir (e.g., via a realpath containment check) before writing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/tools/dbt-health-config.ts, line 39:
<comment>The `mkdir` + `Bun.write` sequence will follow pre-existing symlinks. If `.altimate/` or `dbt-health.yml` is a symlink pointing outside `project_dir`, this writes attacker-controlled content to an arbitrary path. Consider resolving `outPath` after directory creation and verifying it still resides within `project_dir` (e.g., via a realpath containment check) before writing.</comment>
<file context>
@@ -0,0 +1,55 @@
+
+ let written = false
+ if (args.write !== false) {
+ await mkdir(dirname(outPath), { recursive: true })
+ await Bun.write(outPath, yaml)
+ written = true
</file context>
| } | ||
|
|
||
| const yaml = String((result.data as { config?: unknown }).config ?? "") | ||
| const outPath = `${args.project_dir.replace(/\/+$/, "")}/${DBT_HEALTH_CONFIG_RELPATH}` |
There was a problem hiding this comment.
P3: The output path is built with manual slash-trimming and template concatenation, but dirname from node:path is already imported. Using join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH) would normalize separators consistently (particularly relevant on Windows) and is more idiomatic given the existing import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/tools/dbt-health-config.ts, line 35:
<comment>The output path is built with manual slash-trimming and template concatenation, but `dirname` from `node:path` is already imported. Using `join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH)` would normalize separators consistently (particularly relevant on Windows) and is more idiomatic given the existing import.</comment>
<file context>
@@ -0,0 +1,55 @@
+ }
+
+ const yaml = String((result.data as { config?: unknown }).config ?? "")
+ const outPath = `${args.project_dir.replace(/\/+$/, "")}/${DBT_HEALTH_CONFIG_RELPATH}`
+
+ let written = false
</file context>
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/tools/dbt-project-health.ts (L38-L52)
[🟠 MEDIUM] If args.config_path is explicitly provided but the file does not exist, this logic will silently proceed with config_json = undefined. It is recommended to throw an error or return an error response when an explicitly provided configuration file is not found, to avoid running health checks with unintended settings.
Suggested change:
let config_json: string | undefined
if (args.config_path) {
const file = Bun.file(args.config_path)
if (!(await file.exists())) {
return {
title: "dbt health: ERROR",
metadata: { error: `Config file not found: ${args.config_path}` },
output: `Failed to find config file at ${args.config_path}`,
}
}
config_json = await file.text()
} else {
const candidates = [
`${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yml`,
`${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yaml`,
`${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.json`,
]
for (const candidate of candidates) {
const file = Bun.file(candidate)
if (await file.exists()) {
config_json = await file.text()
break
}
}
}
2. packages/opencode/src/altimate/tools/dbt-project-health.ts (L93-L95)
[🔵 LOW] According to the review checklist, nested ternary expressions are not allowed. Please refactor this to use an if...else statement or logical operators to improve readability.
Suggested change:
for (const f of findings) {
let where = ""
if (f.file) {
where = ` (${f.file})`
} else if (f.unique_id) {
where = ` (${f.unique_id})`
}
lines.push(`[${f.severity.toUpperCase()}] ${f.code} ${f.alias}${where}`)
3. packages/opencode/src/altimate/tools/dbt-health-config.ts (L34)
[🔴 HIGH] Potential runtime TypeError. Accessing .config directly on the typecasted result.data will throw a TypeError: Cannot read properties of undefined if result.data is null or undefined (even when result.success is true). Please use optional chaining (?.config) to ensure safe access.
Suggested change:
const yaml = String((result.data as { config?: unknown })?.config ?? "")
4. packages/opencode/src/altimate/tools/dbt-health-config.ts (L35)
[🔴 HIGH] Unsafe path construction and potential directory traversal. The output path is constructed using simple string concatenation. If the AI agent provides an empty string, /, or ../ for project_dir, it could attempt to write to unexpected system locations outside the intended workspace.
Consider importing join from node:path, using join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH), and adding a check to ensure the resolved path remains within the allowed workspace context.
Suggested change:
// Ensure `join` is imported from `node:path`
const outPath = join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH)
// Recommended: add validation to ensure `outPath` is within the workspace root
5. packages/opencode/src/altimate/tools/dbt-health-config.ts (L37-L42)
[🔴 HIGH] Unhandled exceptions in file system operations and inconsistent FS API usage.
- Unhandled Exceptions:
mkdirand file write operations are not enclosed in atry...catchblock. If the tool encounters permission issues or invalid paths, these async calls will throw exceptions that could crash the execution. They should be caught to gracefully return an error payload (similar to!result.success). - Inconsistent API: The code imports
mkdirfrom standardnode:fs/promisesbut uses the runtime-specificBun.write. For consistency within the file and to avoid potentialReferenceErrors outside of Bun, it's recommended to usewriteFilefromnode:fs/promises.
Suggested change:
let written = false
if (args.write !== false) {
try {
await mkdir(dirname(outPath), { recursive: true })
// Ensure `writeFile` is imported from `node:fs/promises` alongside `mkdir`
await writeFile(outPath, yaml)
written = true
} catch (err) {
return {
title: "dbt health config: ERROR",
metadata: { error: String(err) },
output: `Failed to write config file to ${outPath}: ${err}`,
}
}
}
| let config_json: string | undefined | ||
| const candidates = args.config_path | ||
| ? [args.config_path] | ||
| : [ | ||
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yml`, | ||
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yaml`, | ||
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.json`, | ||
| ] | ||
| for (const candidate of candidates) { | ||
| const file = Bun.file(candidate) | ||
| if (await file.exists()) { | ||
| config_json = await file.text() | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] If args.config_path is explicitly provided but the file does not exist, this logic will silently proceed with config_json = undefined. It is recommended to throw an error or return an error response when an explicitly provided configuration file is not found, to avoid running health checks with unintended settings.
Suggested change:
| let config_json: string | undefined | |
| const candidates = args.config_path | |
| ? [args.config_path] | |
| : [ | |
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yml`, | |
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yaml`, | |
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.json`, | |
| ] | |
| for (const candidate of candidates) { | |
| const file = Bun.file(candidate) | |
| if (await file.exists()) { | |
| config_json = await file.text() | |
| break | |
| } | |
| } | |
| let config_json: string | undefined | |
| if (args.config_path) { | |
| const file = Bun.file(args.config_path) | |
| if (!(await file.exists())) { | |
| return { | |
| title: "dbt health: ERROR", | |
| metadata: { error: `Config file not found: ${args.config_path}` }, | |
| output: `Failed to find config file at ${args.config_path}`, | |
| } | |
| } | |
| config_json = await file.text() | |
| } else { | |
| const candidates = [ | |
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yml`, | |
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yaml`, | |
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.json`, | |
| ] | |
| for (const candidate of candidates) { | |
| const file = Bun.file(candidate) | |
| if (await file.exists()) { | |
| config_json = await file.text() | |
| break | |
| } | |
| } | |
| } |
| for (const f of findings) { | ||
| const where = f.file ? ` (${f.file})` : f.unique_id ? ` (${f.unique_id})` : "" | ||
| lines.push(`[${f.severity.toUpperCase()}] ${f.code} ${f.alias}${where}`) |
There was a problem hiding this comment.
[🔵 LOW] According to the review checklist, nested ternary expressions are not allowed. Please refactor this to use an if...else statement or logical operators to improve readability.
Suggested change:
| for (const f of findings) { | |
| const where = f.file ? ` (${f.file})` : f.unique_id ? ` (${f.unique_id})` : "" | |
| lines.push(`[${f.severity.toUpperCase()}] ${f.code} ${f.alias}${where}`) | |
| for (const f of findings) { | |
| let where = "" | |
| if (f.file) { | |
| where = ` (${f.file})` | |
| } else if (f.unique_id) { | |
| where = ` (${f.unique_id})` | |
| } | |
| lines.push(`[${f.severity.toUpperCase()}] ${f.code} ${f.alias}${where}`) |
| } | ||
| } | ||
|
|
||
| const yaml = String((result.data as { config?: unknown }).config ?? "") |
There was a problem hiding this comment.
[🔴 HIGH] Potential runtime TypeError. Accessing .config directly on the typecasted result.data will throw a TypeError: Cannot read properties of undefined if result.data is null or undefined (even when result.success is true). Please use optional chaining (?.config) to ensure safe access.
Suggested change:
| const yaml = String((result.data as { config?: unknown }).config ?? "") | |
| const yaml = String((result.data as { config?: unknown })?.config ?? "") |
| } | ||
|
|
||
| const yaml = String((result.data as { config?: unknown }).config ?? "") | ||
| const outPath = `${args.project_dir.replace(/\/+$/, "")}/${DBT_HEALTH_CONFIG_RELPATH}` |
There was a problem hiding this comment.
[🔴 HIGH] Unsafe path construction and potential directory traversal. The output path is constructed using simple string concatenation. If the AI agent provides an empty string, /, or ../ for project_dir, it could attempt to write to unexpected system locations outside the intended workspace.
Consider importing join from node:path, using join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH), and adding a check to ensure the resolved path remains within the allowed workspace context.
Suggested change:
| const outPath = `${args.project_dir.replace(/\/+$/, "")}/${DBT_HEALTH_CONFIG_RELPATH}` | |
| // Ensure `join` is imported from `node:path` | |
| const outPath = join(args.project_dir, DBT_HEALTH_CONFIG_RELPATH) | |
| // Recommended: add validation to ensure `outPath` is within the workspace root |
| let written = false | ||
| if (args.write !== false) { | ||
| await mkdir(dirname(outPath), { recursive: true }) | ||
| await Bun.write(outPath, yaml) | ||
| written = true | ||
| } |
There was a problem hiding this comment.
[🔴 HIGH] Unhandled exceptions in file system operations and inconsistent FS API usage.
- Unhandled Exceptions:
mkdirand file write operations are not enclosed in atry...catchblock. If the tool encounters permission issues or invalid paths, these async calls will throw exceptions that could crash the execution. They should be caught to gracefully return an error payload (similar to!result.success). - Inconsistent API: The code imports
mkdirfrom standardnode:fs/promisesbut uses the runtime-specificBun.write. For consistency within the file and to avoid potentialReferenceErrors outside of Bun, it's recommended to usewriteFilefromnode:fs/promises.
Suggested change:
| let written = false | |
| if (args.write !== false) { | |
| await mkdir(dirname(outPath), { recursive: true }) | |
| await Bun.write(outPath, yaml) | |
| written = true | |
| } | |
| let written = false | |
| if (args.write !== false) { | |
| try { | |
| await mkdir(dirname(outPath), { recursive: true }) | |
| // Ensure `writeFile` is imported from `node:fs/promises` alongside `mkdir` | |
| await writeFile(outPath, yaml) | |
| written = true | |
| } catch (err) { | |
| return { | |
| title: "dbt health config: ERROR", | |
| metadata: { error: String(err) }, | |
| output: `Failed to write config file to ${outPath}: ${err}`, | |
| } | |
| } | |
| } |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
sahrizvi
left a comment
There was a problem hiding this comment.
Review: 3 critical, 4 major, 10 minor
The design is sound and both tools follow the established Tool.define + Dispatcher.call + {title, metadata, output} shape closely. Three things block merge: a dependency that isn't published yet, a test this PR deterministically breaks, and a file write that sidesteps the repo's permission layer.
Inline comments below cover the critical/major/minor findings. Issues that don't map to a specific line — plus nits, one item needing verification against the core, and what's done well — are in a follow-up comment.
| "@ai-sdk/vercel": "2.0.39", | ||
| "@ai-sdk/xai": "3.0.82", | ||
| "@altimateai/altimate-core": "0.5.1", | ||
| "@altimateai/altimate-core": "0.8.0", |
There was a problem hiding this comment.
CRITICAL — 0.8.0 is not published, and bun.lock still pins 0.5.1
npm view @altimateai/altimate-core versions
→ … "0.5.1", "0.6.0", "0.7.0" dist-tags.latest = 0.7.0
0.8.0 does not exist on npm today. Separately, bun.lock still resolves 0.5.1 at lines 273 and 642 plus all five platform binaries, and the lockfile isn't part of this PR (git log -1 -- bun.lock points at a pre-existing commit).
Every CI job runs plain bun install (.github/workflows/ci.yml:100,108,287,348,408,465,505), and Bun applies frozen-lockfile behaviour when CI is set — so the manifest/lock mismatch fails the install step. If it did install from the current lock it would fetch 0.5.1, which exports neither dbtProjectHealth nor dbtHealthInferConfig, and both tools would return TypeError: core.dbtProjectHealth is not a function on every call.
The PR description already flags the ordering dependency, so this is partly known — noting it because the branch is red until that release lands.
Fix: publish 0.8.0, then bun install and commit the regenerated bun.lock with the root entry and all five platform optional deps at 0.8.0.
One wrinkle: bunfig.toml sets minimumReleaseAge = 259200 (3 days) and this package is not in minimumReleaseAgeExcludes, so a freshly published 0.8.0 will still be refused for three days after publish. Add it to the excludes list or plan the timing.
| // dbt project health checks — reads the dbt project files directly (no manifest required). | ||
| register("altimate_core.dbt_project_health", async (params) => { | ||
| try { | ||
| const json = core.dbtProjectHealth(params.project_dir, params.config_json, params.catalog_path) | ||
| return ok(true, { findings: JSON.parse(json) }) | ||
| } catch (e) { | ||
| return fail(e) | ||
| } | ||
| }) | ||
| // Deterministically infer a dbt health-check config (YAML) from a project's conventions. | ||
| register("altimate_core.dbt_health_infer_config", async (params) => { | ||
| try { | ||
| return ok(true, { config: core.dbtHealthInferConfig(params.project_dir) }) | ||
| } catch (e) { | ||
| return fail(e) | ||
| } | ||
| }) |
There was a problem hiding this comment.
CRITICAL — these two registrations break altimate-core-native.test.ts
packages/opencode/test/altimate/altimate-core-native.test.ts:176-231 keeps a hand-maintained ALL_METHODS array (43 entries) and asserts an exact count:
const coreCount = registered.filter((m) => m.startsWith("altimate_core.")).length
expect(coreCount).toBe(ALL_METHODS.length)Registering two more methods without adding them to the array makes it 45 vs 43. This is a deterministic CI failure, not a coverage gap.
Fix: add "altimate_core.dbt_project_health" and "altimate_core.dbt_health_infer_config" to ALL_METHODS.
| // dbt project health checks — reads the dbt project files directly (no manifest required). | ||
| register("altimate_core.dbt_project_health", async (params) => { | ||
| try { | ||
| const json = core.dbtProjectHealth(params.project_dir, params.config_json, params.catalog_path) |
There was a problem hiding this comment.
MINOR — no version guard on the new exports
This file already establishes the guard pattern for exactly this situation at line 176:
params.base_sql && typeof core.lintDiff === "function"added because lintDiff postdates 0.5.1. The surrounding try/catch does convert a missing-export TypeError into the normal failure envelope, so this is diagnostics quality rather than a crash — but the operator sees a raw TypeError string instead of an actionable message.
Fix: guard with typeof core.dbtProjectHealth === "function" and fail with "dbt_project_health requires @altimateai/altimate-core >= 0.8.0". Same for dbtHealthInferConfig below.
| const outPath = `${args.project_dir.replace(/\/+$/, "")}/${DBT_HEALTH_CONFIG_RELPATH}` | ||
|
|
||
| let written = false | ||
| if (args.write !== false) { | ||
| await mkdir(dirname(outPath), { recursive: true }) | ||
| await Bun.write(outPath, yaml) | ||
| written = true | ||
| } |
There was a problem hiding this comment.
CRITICAL — this write has none of the repo's write guards
project_dir comes straight from the model. There is no path.resolve against Instance.directory, no containment check, no symlink resolution, and no permission prompt. WriteTool (src/tool/write.ts:64-70) resolves against Instance.directory and then gates every write behind assertExternalDirectoryEffect and assertSensitiveWriteEffect.
This is the first write-capable tool under src/altimate/tools/ — no other file there calls Bun.write/writeFile — so it establishes a path around the guard layer.
Three concrete failure modes:
- an absolute or
../-containingproject_dircreates directories and writes files outside the workspace with no prompt; - a
.altimatedirectory or.altimate/dbt-health.ymlsymlink inside an otherwise legitimate project is followed and its external target truncated; mkdir(…, { recursive: true })creates the whole tree on the way there.
Fix: resolve project_dir against Instance.directory, build the path with path.join, canonicalize and verify containment, then gate the write. This tool uses the Promise/Zod adapter, so call the Promise-style helpers — assertExternalDirectory(ctx, target, options) / assertExternalDirectoryLegacy(...) / assertSensitiveWrite(ctx, target) (src/tool/external-directory.ts:54,89,113) — not the Effect-only variants. Re-validate symlinked path components immediately before writing.
| write: z | ||
| .boolean() | ||
| .optional() | ||
| .describe("Write the config to <project_dir>/.altimate/dbt-health.yml (default true). If false, only return the YAML."), |
There was a problem hiding this comment.
MAJOR — hand-edited config is silently destroyed
write defaults to true and Bun.write at line 40 unconditionally replaces an existing .altimate/dbt-health.yml — no diff, no backup, no overwrite argument, and no mention in the tool description.
.opencode/command/dbt-health-config.md:23 explicitly tells users the file "is committed with the project and can be hand-edited", so the advertised workflow is precisely the one that erases their edits.
Fix: add overwrite: boolean defaulting to false, or open with wx and fail when the file exists. At minimum, report that the file already existed and what changed.
| config_path: z | ||
| .string() | ||
| .optional() | ||
| .describe("Optional path to a JSON health-check config (disabled checks, severity overrides, options)"), |
There was a problem hiding this comment.
MINOR — this description is inaccurate and incomplete
It says "path to a JSON health-check config", but auto-discovery below accepts .yml/.yaml/.json and the sibling dbt_health_config tool writes YAML. As written, the model is steered away from passing the very file it just generated.
The description also never mentions that a project-local config is auto-discovered when this is omitted, so the model can't reason about when to pass it.
Fix: "Optional path to a health-check config (YAML or JSON). When omitted, auto-discovers <project_dir>/.altimate/dbt-health.{yml,yaml,json}."
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yml`, | ||
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.yaml`, | ||
| `${args.project_dir.replace(/\/+$/, "")}/.altimate/dbt-health.json`, |
There was a problem hiding this comment.
MINOR — build these with node:path.join
${args.project_dir.replace(/\/+$/, "")}/… hand-rolls path joining in three places here and once more in dbt-health-config.ts:35. It doesn't normalize .. segments and assumes POSIX separators.
Fix: path.join(projectDir, ".altimate", "dbt-health.yml"). dbt-health-config.ts already imports dirname from node:path.
| /** A single finding returned by `altimate_core.dbt_project_health` (Rust `HealthFinding`). */ | ||
| interface HealthFinding { | ||
| code: string | ||
| alias: string | ||
| resource_type: "model" | "source" | "exposure" | "macro" | "project" | ||
| unique_id?: string | ||
| file?: string | ||
| severity: "error" | "warning" | "info" | ||
| message: string | ||
| recommendation?: string | ||
| reason_to_flag?: string | ||
| metadata?: unknown | ||
| } |
There was a problem hiding this comment.
MINOR — HealthFinding is an unvalidated local guess
This interface is hand-declared here with no link to the Rust HealthFinding struct and no runtime validation — the shape is only asserted via a cast at line 69. Every other bridge method has a typed result interface in native/types.ts. If the core renames or drops a field, TypeScript won't catch it and the formatter silently emits undefined text.
Fix: move the type into native/types.ts as part of a DbtProjectHealthResult, or parse the JSON through a z.object() schema in the handler.
| reason_to_flag?: string | ||
| metadata?: unknown |
There was a problem hiding this comment.
MINOR — these two fields are declared but never rendered
formatFindings (lines 90-100) emits code, alias, file/unique_id, message, and recommendation — reason_to_flag and metadata never reach the output. reason_to_flag is arguably the single most useful field in a governance report, since it's what tells the reader why a rule fired.
Fix: render reason_to_flag (it's what makes a finding actionable), or drop both from the interface.
| function formatFindings(findings: HealthFinding[]): string { | ||
| if (findings.length === 0) return "✓ No dbt health issues found." | ||
| const lines: string[] = [] | ||
| for (const f of findings) { | ||
| const where = f.file ? ` (${f.file})` : f.unique_id ? ` (${f.unique_id})` : "" | ||
| lines.push(`[${f.severity.toUpperCase()}] ${f.code} ${f.alias}${where}`) | ||
| lines.push(` ${f.message}`) | ||
| if (f.recommendation) lines.push(` → ${f.recommendation}`) | ||
| } | ||
| return lines.join("\n") | ||
| } |
There was a problem hiding this comment.
MINOR — no domain-level cap on output
formatFindings emits 2–3 lines per finding with no limit. This is bounded in practice: the shared Tool.define wrapper applies Truncate.Service at MAX_LINES = 2000 / MAX_BYTES = 50 * 1024 (src/tool/truncate.ts:15-16) and spills the full text to a file, and because findings are sorted error-first the head truncation keeps the important ones while metadata counts stay accurate. That's a good outcome largely by accident of the sort order.
What's missing is that the model is never told it's seeing a partial list.
Fix: cap at a domain-appropriate number and append "… N more findings omitted", so the truncation is explicit rather than inferred.
Remaining findings — not line-attributable, plus nitsMAJOR — no tests for either toolNeither
Worth adding beyond the basics: absolute and Needs verification against the core (couldn't confirm from this repo)The bridge passes YAML file contents in a parameter named Two things to confirm on the core side:
NITs
Two things worth explicitly retiring
What's done well
|
Summary
Wires the new Rust dbt health checks (
altimate_core::dbt::health, added in AltimateAI/altimate-core-internal#762) into altimate-code, so dbt governance / modelling / documentation / test checks run directly against a dbt project's files — no compiledmanifest.jsonrequired — through the@altimateai/altimate-corenapi binding.Changes
altimate_core.dbt_project_healthbridge-method type and aDispatcher.registerhandler callingcore.dbtProjectHealth(projectDir, configJson?, catalogPath?).DbtProjectHealthTool(dbt_project_health) — takes aproject_dir(+ optionalconfig_path,catalog_path), runs the checks, and formats findings grouped by severity. Registered in the tool registry and the altimate barrel export.@altimateai/altimate-core0.5.1 → 0.8.0 (the version that exportsdbtProjectHealth).Dependency
Requires
@altimateai/altimate-core0.8.0, published from AltimateAI/altimate-core-internal#762 (stacked on #761). Merge/release that first.Verification
TypeScript typecheck is clean on all changed files against the 0.8.0 core types (verified by overlaying the freshly-built core
index.d.ts). End-to-end, the underlying core returns findings for a sample project (25 findings across 13 check codes on the fixture).🤖 Generated with Claude Code
Summary by cubic
Adds dbt project health checks that run directly on project files and a deterministic config inference flow, powered by
@altimateai/altimate-core0.8.0. You can now generate a per-project.altimate/dbt-health.ymland have the health tool pick it up automatically.New Features
altimate_core.dbt_project_healthandaltimate_core.dbt_health_infer_config.dbt_project_healthtool: runs checks on files; acceptsproject_dirwith optionalconfig_path/catalog_path; auto-discovers.altimate/dbt-health.{yml,yaml,json}; outputs findings with severity counts.dbt_health_configtool: infers and writes.altimate/dbt-health.ymlper project; adds/dbt-health-configcommand to infer then run health; tools registered and exported.Dependencies
@altimateai/altimate-corefrom0.5.1to0.8.0.@altimateai/altimate-core0.8.0to be published first.Written for commit fefe41f. Summary will update on new commits.
Summary by CodeRabbit
.altimate/dbt-health.yml, returning the written path(s) and resulting severity counts.