Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
👋 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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 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. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/native/dbt/lineage.ts">
<violation number="1" location="packages/opencode/src/altimate/native/dbt/lineage.ts:62">
P2: MCP drops the graph returned by `include_graph`: its structured-field allow-list omits `graph`, and the text report does not contain it. Add `graph` to the MCP structured fields so lineage viewers can receive the requested graph.</violation>
</file>
<file name="packages/opencode/src/altimate/tools/impact-analysis.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/impact-analysis.ts:70">
P2: `impact_analysis` still traverses only `manifest.models`, so downstream snapshots disappear from the reported blast radius and affected tests. Include `manifest.snapshots` in this traversal so models feeding snapshots are not reported as unaffected.</violation>
</file>
<file name="packages/opencode/src/altimate/native/dbt/graph.ts">
<violation number="1" location="packages/opencode/src/altimate/native/dbt/graph.ts:30">
P2: The 500-node cap counts only non-test downstream additions, while the target, upstream nodes, and test IDs are added outside that budget. Enforce the limit against every emitted graph node, or document and expose a separate downstream-model limit.</violation>
</file>
<file name="packages/opencode/src/altimate/native/dbt/column-reach.ts">
<violation number="1" location="packages/opencode/src/altimate/native/dbt/column-reach.ts:253">
P2: Duplicate bare names are treated as ambiguous even when the compiled SQL qualifies the relation. Base this decision on the qualified lineage candidates (or the SQL relation), otherwise definite readers are downgraded to uncertain.</violation>
<violation number="2" location="packages/opencode/src/altimate/native/dbt/column-reach.ts:255">
P3: The wildcard fallback is relation-blind and reports false possible readers when `*` belongs to another parent. Associate the wildcard with its source relation before applying the undocumented-column check.</violation>
<violation number="3" location="packages/opencode/src/altimate/native/dbt/column-reach.ts:278">
P2: Indirect lineage edges are counted as certain readers even though this branch deliberately excludes them from `carried`. Set `reads` only for non-indirect edges, so indirect-only dependencies fall through to the uncertain-reader handling.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| const uniqueId = | ||
| getUniqueId(nodes, params.model) ?? | ||
| (nodes[params.model] ? params.model : (byPath?.[0] ?? Object.keys(nodes).find((id) => nodes[id] === modelNode))) | ||
| const graph = params.include_graph && uniqueId ? { graph: lineageGraph(manifest, uniqueId) } : {} |
There was a problem hiding this comment.
P2: MCP drops the graph returned by include_graph: its structured-field allow-list omits graph, and the text report does not contain it. Add graph to the MCP structured fields so lineage viewers can receive the requested graph.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/native/dbt/lineage.ts, line 62:
<comment>MCP drops the graph returned by `include_graph`: its structured-field allow-list omits `graph`, and the text report does not contain it. Add `graph` to the MCP structured fields so lineage viewers can receive the requested graph.</comment>
<file context>
@@ -35,16 +42,29 @@ export function dbtLineage(params: DbtLineageParams): DbtLineageResult {
+ const uniqueId =
+ getUniqueId(nodes, params.model) ??
+ (nodes[params.model] ? params.model : (byPath?.[0] ?? Object.keys(nodes).find((id) => nodes[id] === modelNode)))
+ const graph = params.include_graph && uniqueId ? { graph: lineageGraph(manifest, uniqueId) } : {}
// Extract compiled SQL
</file context>
There was a problem hiding this comment.
Not changed: the graph is deliberately CLI-only (altimate tool run dbt_lineage --json with include_graph) and kept off the MCP allow-list, since a 500-node graph would be sent to the model as structured content. Clarified in graph.ts and the tool description.
| ambiguous = true | ||
| continue | ||
| } | ||
| reads = true |
There was a problem hiding this comment.
P2: Indirect lineage edges are counted as certain readers even though this branch deliberately excludes them from carried. Set reads only for non-indirect edges, so indirect-only dependencies fall through to the uncertain-reader handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/native/dbt/column-reach.ts, line 278:
<comment>Indirect lineage edges are counted as certain readers even though this branch deliberately excludes them from `carried`. Set `reads` only for non-indirect edges, so indirect-only dependencies fall through to the uncertain-reader handling.</comment>
<file context>
@@ -0,0 +1,336 @@
+ ambiguous = true
+ continue
+ }
+ reads = true
+ if (edge.lineage_type !== "indirect" && typeof edge.target === "string") {
+ carried.add(edge.target.toLowerCase())
</file context>
| reads = true | |
| if (edge.lineage_type !== "indirect") reads = true |
There was a problem hiding this comment.
Not changed: an indirect edge (filter, join key, group by) is a real read of the column, so the model is a certain reader; it just contributes no carried output column. A filter-only model is covered by an existing test.
| } | ||
| const collides = parents.some((p) => allParents.some((other) => other !== p && bareName(other) === bareName(p))) | ||
| const wildcard = | ||
| selectsStar(sqlText) && parents.some((p) => [...tracked.get(p)!].some((col) => !documentedColumns(lookup(p)).has(col))) |
There was a problem hiding this comment.
P3: The wildcard fallback is relation-blind and reports false possible readers when * belongs to another parent. Associate the wildcard with its source relation before applying the undocumented-column check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/native/dbt/column-reach.ts, line 255:
<comment>The wildcard fallback is relation-blind and reports false possible readers when `*` belongs to another parent. Associate the wildcard with its source relation before applying the undocumented-column check.</comment>
<file context>
@@ -0,0 +1,336 @@
+ }
+ const collides = parents.some((p) => allParents.some((other) => other !== p && bareName(other) === bareName(p)))
+ const wildcard =
+ selectsStar(sqlText) && parents.some((p) => [...tracked.get(p)!].some((col) => !documentedColumns(lookup(p)).has(col)))
+ if (collides || wildcard) {
+ unsure.add(id)
</file context>
There was a problem hiding this comment.
Not changed: a relation-blind wildcard check can only produce a 'may read' entry (never a certain reader) and errs toward reporting; associating * with its relation would need a SQL parser here, and a missed reader is worse than an extra possible one.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // (already truncated) text (dbt_lineage: its header, its mappings travel as data), only the lineage | ||
| // fields are passed, and the whole is capped. | ||
| const STRUCTURED_FIELDS: Record<string, readonly string[]> = { | ||
| dbt_lineage: ["model_name", "model_unique_id", "confidence", "confidence_factors", "upstream", "columns", "edges", "error"], |
There was a problem hiding this comment.
[WARNING]: include_graph is a no-op for MCP clients
STRUCTURED_FIELDS.dbt_lineage omits graph, so structuredResult drops it, and the text report does not render it either (formatDbtLineage in tools/dbt-lineage.ts only prints mappings and edges). A client that calls dbt_lineage with include_graph: true therefore receives the graph nowhere: not in structuredContent, not in the text. That contradicts the stated purpose in native/dbt/graph.ts ("clients that cannot read a large manifest ... get it from dbt_lineage with include_graph") and the PR description. Add "graph" to the field list — the 100 KB fallback already covers an oversized graph — or render it in the report.
There was a problem hiding this comment.
Not changed: the graph is CLI-only by design (see the reply on the other include_graph thread); clients that need it use altimate tool run dbt_lineage --json. graph.ts now says so.
| name, | ||
| description: descriptions[name] ?? tool.description, | ||
| inputSchema: tool.jsonSchema as McpTool["inputSchema"], | ||
| ...(STRUCTURED_TOOLS.has(name) && { outputSchema: STRUCTURED_OUTPUT_SCHEMA }), |
There was a problem hiding this comment.
[SUGGESTION]: Declaring outputSchema makes the error path non-conformant
MCP requires a tool that declares an output schema to return structuredContent that conforms to it. The catch block below (and the early workspace.execute return) still answers with text only, so argument-validation failures or adapter errors for dbt_lineage/impact_analysis produce a result with no structuredContent at all. Emitting structuredContent: { report: message } in that path keeps every response of these two tools valid.
There was a problem hiding this comment.
Fixed: the catch path in server.ts now returns structuredContent { report } for dbt_lineage and impact_analysis, so every response of those tools matches the declared output schema.
| // The engine answers SQL it cannot parse with `errors` and no edges rather than throwing. | ||
| const parseErrors: string[] = Array.isArray(lineage?.errors) ? lineage.errors.map(String) : [] | ||
| const noEdges = !Array.isArray(lineage?.column_lineage) || lineage.column_lineage.length === 0 | ||
| if ((result.confidence === "low" && !Array.isArray(lineage?.column_lineage)) || (parseErrors.length > 0 && noEdges)) { |
There was a problem hiding this comment.
[SUGGESTION]: This duplicates unparsed() in native/dbt/lineage.ts:12
dbtLineage already sets confidence: "low" for exactly the two cases checked here (engine error with no column_lineage, or errors with no edges), so result.confidence === "low" is equivalent and keeps one source of truth. As written, the two copies of the parse-error rule can drift (e.g. if unparsed changes or the engine starts returning errors alongside edges). Prefer reusing the helper or the confidence flag.
There was a problem hiding this comment.
Fixed: column-reach.ts now relies on dbtLineage's confidence === 'low' (engine error or unparseable SQL) instead of re-deriving the parse-error rule.
| } | ||
|
|
||
| // Exposures on anything affected | ||
| if (data.exposures && data.exposures.length > 0) { |
There was a problem hiding this comment.
[SUGGESTION]: Exposures are listed twice when a column is traced
reach.exposures (printed at line 283) is a subset of data.exposures here: exposuresOn(affectedModelIds) covers the target and every downstream model, while column reach only narrows them to certain readers. Every dashboard therefore appears once in the Column Reach block and again under Exposures (N). Skip the entries already printed by columnReach, or omit this section when a columnReach block was shown.
There was a problem hiding this comment.
Fixed: formatImpactReport omits exposures already printed in the Column Reach block and labels the rest 'Other exposures' (impact-analysis.ts); test added.
| transitive_count: transitive.length, | ||
| test_count: affectedTestCount, | ||
| column_impact: columnImpact.length, | ||
| column_impact: reach ? reach.models.length : 0, |
There was a problem hiding this comment.
[SUGGESTION]: column_impact silently changes meaning
Previously this metadata field was the count of the target model's output columns fed by the column (from lineage.check on SELECT * FROM <model>); now it is the number of downstream models that read it. Same key, different unit — traces or clients that surface column_impact will misreport it. Rename it (e.g. column_reader_count) or keep the old field beside the new one.
There was a problem hiding this comment.
Fixed: renamed to column_reader_count (see the reply on the other column_impact thread); the old key is gone since it had no consumers.
| graph.children[one] = (children[one] ?? []).filter((other) => { | ||
| if (keep.has(other)) return true | ||
| if (!isTest(other)) return false | ||
| graph.nodes[other] ??= { ...nodeOf(other, lookup(manifest, other)), type: "test" } |
There was a problem hiding this comment.
[SUGGESTION]: Test nodes get inconsistent graph entries
Tests are inserted into graph.nodes and into a parent's children here, but they are never added to keep, so no graph.parents[testId] or graph.children[testId] entry is created. A consumer that indexes graph.parents[node.id] for every node in graph.nodes (the type declares both as Record<string, string[]> with an entry per node) gets undefined. Either create the entries for tests or document that they appear only as children.
There was a problem hiding this comment.
Fixed: test nodes now have parents and children entries in the graph (graph.ts); test added.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Both suggestions from the previous review are addressed in this increment:
Files Reviewed (3 files)
Previous Review Summaries (12 snapshots, latest commit 98c582c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 98c582c)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 315685b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 6d71706)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 299bdf0)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Previous review (commit f8fb664)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 353523d)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit e80cf4c)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit fe673cf)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 7c89b71)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 4ac010e)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit 9f92fa2)Status: 12 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (12 files)
Fix these issues in Kilo Cloud Previous review (commit 391fd7c)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (12 files)
Fix these issues in Kilo Cloud Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by deepseek-v4.1-flash-1017 · Input: 51.7K · Output: 7.3K · Cached: 378K Review guidance: REVIEW.md from base branch |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 12 finding(s)
- 11 anchored to a line (posted inline when the comment stream is on)
- 1 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/native/dbt/lineage.ts (L47)
[🔵 LOW] According to the code review checklist, the use of any type should be avoided. If it is necessary, please provide a comment explaining the reason. Consider replacing any with a more specific interface (e.g., { resource_type?: string, original_file_path?: string, name?: string }).
Please also address the Object.values<any> usage a few lines below.
2. packages/opencode/src/altimate/native/dbt/lineage.ts (L94)
[🔵 LOW] The code review checklist prohibits the use of nested ternary expressions. Please refactor this logic into an if-else statement before the return block or extract it into a helper function to improve readability.
3. packages/opencode/src/altimate/native/dbt/graph.ts (L32-L35)
[🔵 LOW] TypeScript Types: The project standards state to avoid using the any type, or if necessary, provide a comment explaining the reason. The manifest object and various node variables are typed as any across multiple functions (lookup, relationOf, nodeOf, mapsOf, and lineageGraph). Please either define an interface (or use an existing one if applicable, such as Record<string, unknown>) or add a comment explaining why any is needed (e.g., parsing a large, unstructured JSON object).
Suggested change:
// Using 'any' for the manifest and node variables as they represent large unstructured JSON objects from dbt.
function lookup(manifest: any, id: string): any {
for (const group of GROUPS) if (manifest[group]?.[id]) return manifest[group][id]
return undefined
}
4. packages/opencode/src/altimate/native/dbt/graph.ts (L101-L106)
[🟠 MEDIUM] Potential Null/Undefined Reference Issue: Test nodes are appended to graph.nodes but do not receive corresponding array initializations in graph.parents or graph.children. Since test nodes bypass the loop that initializes these relationships, downstream consumers iterating over graph.nodes may encounter runtime exceptions if they assume every node has defined parents or children arrays. Consider initializing them here.
Suggested change:
graph.children[one] = (children[one] ?? []).filter((other) => {
if (keep.has(other)) return true
if (!isTest(other)) return false
if (!graph.nodes[other]) {
graph.nodes[other] = { ...nodeOf(other, lookup(manifest, other)), type: "test" }
graph.parents[other] = []
graph.children[other] = []
}
return true
})
5. packages/opencode/src/altimate/native/dbt/graph.ts (L87-L90)
[🟠 MEDIUM] Performance Optimization: Inefficient loop termination for the downstream node limit. When count >= MAX_DOWNSTREAM, setting truncated = true and using continue only skips to the next iteration of the inner for loop, needlessly iterating through all remaining children without making meaningful progress. You can empty the queue and use break to exit the traversal entirely.
Suggested change:
if (count >= MAX_DOWNSTREAM) {
truncated = true
queue.length = 0
break
}
6. packages/opencode/src/altimate/native/dbt/column-reach.ts (L57)
[🔵 LOW] Ternary Expressions & TypeScript Types:
- Nested ternary expressions are not allowed: Please refactor this into an
if-elsestatement for better readability. - Avoid using
anytype: Using(source as any)bypasses type safety without an explanatory comment. Please use type narrowing or define a proper interface.
Suggested change:
let text: string | null = null
if (typeof source === "string") {
text = source
} else if (typeof source === "object" && source !== null && "source" in source && typeof (source as Record<string, unknown>).source === "string") {
text = (source as Record<string, unknown>).source as string
}
7. packages/opencode/src/altimate/native/dbt/column-reach.ts (L67)
[🔵 LOW] TypeScript Types: Avoid using the any type for node and manifest arguments here and in other helper functions (caseCollides, documentedColumns, childMapOf). This weakens type safety, especially for complex structures like dbt manifests.
Please define proper TypeScript interfaces for the dbt manifest structures, or reuse existing types like DbtModelInfo from types.ts.
8. packages/opencode/src/altimate/native/dbt/column-reach.ts (L229)
[🟠 MEDIUM] Performance & Async Handling: dbtLineage is called synchronously in a loop that can iterate up to MAX_MODELS (300) times. Since dbtLineage performs heavy synchronous operations (such as SQL AST parsing and redundant file system checks via loadRawManifest), this will severely block the Node.js event loop and make the application unresponsive.
Suggestions:
- Make this function asynchronous and use
await new Promise(r => setImmediate(r))periodically to yield to the event loop. - Consider modifying
dbtLineageto accept an already parsedmanifestobject instead of reading frommanifest_pathrepeatedly, which incursfs.statSyncoverhead on every call.
9. packages/opencode/src/altimate/native/dbt/column-reach.ts (L306-L317)
[🔵 LOW] Ternary Expressions: Nested ternary expressions are not allowed as they severely impact readability. Please refactor this logic using if-else blocks to determine the reason string before pushing to the uncertain array.
Suggested change:
let reason = "is downstream of a model that could not be traced or reads it ambiguously"
if (ambiguous) {
reason = "reads it through a table name more than one parent shares"
} else if (named.length > 0) {
reason = shared
? "names it outside its select list, or names another parent's column of the same name"
: "names it only outside its select list (WHERE, JOIN, GROUP BY or HAVING)"
}
uncertain.push(entry(id, reason))
10. packages/opencode/src/altimate/native/dbt/column-reach.ts (L322-L325)
[🟠 MEDIUM] Performance Optimization: exposuresOn internally calls loadRawManifest(manifestPath). Although it has an internal memory cache, it still performs synchronous file system operations (fs.existsSync, fs.statSync) on every invocation.
Since the manifest is already parsed at the beginning of columnReach, it is highly recommended to refactor exposuresOn to accept the parsed manifest object to eliminate the redundant synchronous disk I/O.
11. packages/opencode/src/altimate/tools/impact-analysis.ts (L75-L78)
[🟠 MEDIUM] Since targetModel.unique_id can be undefined, undefined may be added to the affectedModelIds Set. This causes the Set to have the type Set<string | undefined>, which is later passed to exposuresOn(args.manifest_path, affectedModelIds) that expects an Iterable<string>. This mismatch can cause TypeScript compiler errors under strict settings, and is logically imprecise. It is recommended to filter out falsy values from the entire array before initializing the Set.
Suggested change:
const affectedModelIds = new Set(
[
targetModel.unique_id,
...downstream.map((d) => d.unique_id ?? modelsByName.get(d.name)?.unique_id),
].filter((id): id is string => Boolean(id))
)
12. packages/opencode/src/altimate/tools/impact-analysis.ts
[🔴 HIGH] Using a Depth-First Search (DFS) approach to calculate the depth of dependencies causes a bug: if a model is both a direct dependency and a transitive dependency, but the DFS reaches it via a longer path first, it will be added to the visited set and recorded with a depth > 1. Later, when evaluating direct dependencies, it will be skipped because it is already in visited. Consequently, direct dependencies might be incorrectly classified as 'transitive'. Switching to a Breadth-First Search (BFS) ensures that each model is visited first via its shortest path, correctly recording its minimum depth.
Suggested change:
// A parent is matched by unique_id when known (a versioned model's id ends in its version, not its
// name; another package may have a model of the same name), else by the last segment of each
// dependency id.
const queue = [{ name: targetName, id: targetId, depth: 1, path: [targetName] }]
while (queue.length > 0) {
const { name, id, depth, path } = queue.shift()!
for (const model of models) {
const key = model.unique_id ?? model.name
if (visited.has(key)) continue
const deps = model.depends_on.map((d) => d.split(".").pop())
// An exact id decides alone: another package's model of the same name is not this one.
if (id ? model.depends_on.includes(id) : deps.includes(name)) {
visited.add(key)
const newPath = [...path, model.name]
results.push({
name: model.name,
...(model.unique_id && { unique_id: model.unique_id }),
depth,
materialized: model.materialized,
path: newPath,
})
queue.push({ name: model.name, id: model.unique_id, depth: depth + 1, path: newPath })
}
}
}
| const modelNode = findModel(nodes, params.model) | ||
| // Find target model by name or unique_id; a snapshot by its unique_id (it has compiled SQL too) | ||
| // A model by name or unique_id; a snapshot by unique_id; either by its project-relative file path. | ||
| const byPath = Object.entries<any>(nodes).find( |
There was a problem hiding this comment.
[🔵 LOW] According to the code review checklist, the use of any type should be avoided. If it is necessary, please provide a comment explaining the reason. Consider replacing any with a more specific interface (e.g., { resource_type?: string, original_file_path?: string, name?: string }).
Please also address the Object.values<any> usage a few lines below.
There was a problem hiding this comment.
Declined: the manifest is untyped JSON, so lineage.ts keeps any like the surrounding manifest-reading code.
| confidence_factors: rawLineage.error ? [String(rawLineage.error)] : [], | ||
| // The engine reports SQL it cannot parse in `errors` with no edges, rather than throwing. | ||
| confidence: rawLineage.error || unparsed(rawLineage) ? "low" : "high", | ||
| confidence_factors: rawLineage.error ? [String(rawLineage.error)] : unparsed(rawLineage) ? [String((rawLineage.errors as unknown[])[0])] : [], |
There was a problem hiding this comment.
[🔵 LOW] The code review checklist prohibits the use of nested ternary expressions. Please refactor this logic into an if-else statement before the return block or extract it into a helper function to improve readability.
There was a problem hiding this comment.
Declined: the ternary follows the surrounding style in this file and is readable as written.
| function lookup(manifest: any, id: string): any { | ||
| for (const group of GROUPS) if (manifest[group]?.[id]) return manifest[group][id] | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
[🔵 LOW] TypeScript Types: The project standards state to avoid using the any type, or if necessary, provide a comment explaining the reason. The manifest object and various node variables are typed as any across multiple functions (lookup, relationOf, nodeOf, mapsOf, and lineageGraph). Please either define an interface (or use an existing one if applicable, such as Record<string, unknown>) or add a comment explaining why any is needed (e.g., parsing a large, unstructured JSON object).
Suggested change:
| function lookup(manifest: any, id: string): any { | |
| for (const group of GROUPS) if (manifest[group]?.[id]) return manifest[group][id] | |
| return undefined | |
| } | |
| // Using 'any' for the manifest and node variables as they represent large unstructured JSON objects from dbt. | |
| function lookup(manifest: any, id: string): any { | |
| for (const group of GROUPS) if (manifest[group]?.[id]) return manifest[group][id] | |
| return undefined | |
| } |
There was a problem hiding this comment.
Declined: dbt manifest nodes are untyped JSON and graph.ts follows the any convention of the other manifest helpers.
| graph.children[one] = (children[one] ?? []).filter((other) => { | ||
| if (keep.has(other)) return true | ||
| if (!isTest(other)) return false | ||
| graph.nodes[other] ??= { ...nodeOf(other, lookup(manifest, other)), type: "test" } | ||
| return true | ||
| }) |
There was a problem hiding this comment.
[🟠 MEDIUM] Potential Null/Undefined Reference Issue: Test nodes are appended to graph.nodes but do not receive corresponding array initializations in graph.parents or graph.children. Since test nodes bypass the loop that initializes these relationships, downstream consumers iterating over graph.nodes may encounter runtime exceptions if they assume every node has defined parents or children arrays. Consider initializing them here.
Suggested change:
| graph.children[one] = (children[one] ?? []).filter((other) => { | |
| if (keep.has(other)) return true | |
| if (!isTest(other)) return false | |
| graph.nodes[other] ??= { ...nodeOf(other, lookup(manifest, other)), type: "test" } | |
| return true | |
| }) | |
| graph.children[one] = (children[one] ?? []).filter((other) => { | |
| if (keep.has(other)) return true | |
| if (!isTest(other)) return false | |
| if (!graph.nodes[other]) { | |
| graph.nodes[other] = { ...nodeOf(other, lookup(manifest, other)), type: "test" } | |
| graph.parents[other] = [] | |
| graph.children[other] = [] | |
| } | |
| return true | |
| }) |
There was a problem hiding this comment.
Already handled: test nodes get parents and children entries in the same block (graph.ts, the !graph.nodes[other] branch), so every node in graph.nodes has both.
| if (count >= MAX_DOWNSTREAM) { | ||
| truncated = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] Performance Optimization: Inefficient loop termination for the downstream node limit. When count >= MAX_DOWNSTREAM, setting truncated = true and using continue only skips to the next iteration of the inner for loop, needlessly iterating through all remaining children without making meaningful progress. You can empty the queue and use break to exit the traversal entirely.
Suggested change:
| if (count >= MAX_DOWNSTREAM) { | |
| truncated = true | |
| continue | |
| } | |
| if (count >= MAX_DOWNSTREAM) { | |
| truncated = true | |
| queue.length = 0 | |
| break | |
| } |
There was a problem hiding this comment.
Fixed in graph.ts: hitting MAX_DOWNSTREAM now empties the queue and breaks out of the traversal instead of looping over the remaining children. Behaviour is unchanged.
| } | ||
|
|
||
| /** The ways a parent's relation can be written in a child's compiled SQL, most qualified first. */ | ||
| function relationKeys(node: any): string[] { |
There was a problem hiding this comment.
[🔵 LOW] TypeScript Types: Avoid using the any type for node and manifest arguments here and in other helper functions (caseCollides, documentedColumns, childMapOf). This weakens type safety, especially for complex structures like dbt manifests.
Please define proper TypeScript interfaces for the dbt manifest structures, or reuse existing types like DbtModelInfo from types.ts.
There was a problem hiding this comment.
Declined: manifest and node values are untyped JSON, so the helpers in column-reach.ts keep any like the existing manifest code.
| break | ||
| } | ||
|
|
||
| const result = dbtLineage({ manifest_path: manifestPath, model: id, dialect }) |
There was a problem hiding this comment.
[🟠 MEDIUM] Performance & Async Handling: dbtLineage is called synchronously in a loop that can iterate up to MAX_MODELS (300) times. Since dbtLineage performs heavy synchronous operations (such as SQL AST parsing and redundant file system checks via loadRawManifest), this will severely block the Node.js event loop and make the application unresponsive.
Suggestions:
- Make this function asynchronous and use
await new Promise(r => setImmediate(r))periodically to yield to the event loop. - Consider modifying
dbtLineageto accept an already parsedmanifestobject instead of reading frommanifest_pathrepeatedly, which incursfs.statSyncoverhead on every call.
There was a problem hiding this comment.
Declined: reach is capped at 300 models and measured at about 550 ms for 300 downstream models, and loadRawManifest is cached. An async rewrite would add complexity without a measured problem.
| uncertain.push( | ||
| entry( | ||
| id, | ||
| ambiguous | ||
| ? "reads it through a table name more than one parent shares" | ||
| : named.length > 0 | ||
| ? shared | ||
| ? "names it outside its select list, or names another parent's column of the same name" | ||
| : "names it only outside its select list (WHERE, JOIN, GROUP BY or HAVING)" | ||
| : "is downstream of a model that could not be traced or reads it ambiguously", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
[🔵 LOW] Ternary Expressions: Nested ternary expressions are not allowed as they severely impact readability. Please refactor this logic using if-else blocks to determine the reason string before pushing to the uncertain array.
Suggested change:
| uncertain.push( | |
| entry( | |
| id, | |
| ambiguous | |
| ? "reads it through a table name more than one parent shares" | |
| : named.length > 0 | |
| ? shared | |
| ? "names it outside its select list, or names another parent's column of the same name" | |
| : "names it only outside its select list (WHERE, JOIN, GROUP BY or HAVING)" | |
| : "is downstream of a model that could not be traced or reads it ambiguously", | |
| ), | |
| ) | |
| let reason = "is downstream of a model that could not be traced or reads it ambiguously" | |
| if (ambiguous) { | |
| reason = "reads it through a table name more than one parent shares" | |
| } else if (named.length > 0) { | |
| reason = shared | |
| ? "names it outside its select list, or names another parent's column of the same name" | |
| : "names it only outside its select list (WHERE, JOIN, GROUP BY or HAVING)" | |
| } | |
| uncertain.push(entry(id, reason)) |
There was a problem hiding this comment.
Declined: style only. The reason selection is readable as written and matches surrounding code.
| const certain = exposuresOn(manifestPath, reachedIds) | ||
| const maybe = exposuresOn(manifestPath, [...uncertain, ...untraced].map((m) => m.unique_id)).filter( | ||
| (e) => !certain.some((c) => c.name === e.name), | ||
| ) |
There was a problem hiding this comment.
[🟠 MEDIUM] Performance Optimization: exposuresOn internally calls loadRawManifest(manifestPath). Although it has an internal memory cache, it still performs synchronous file system operations (fs.existsSync, fs.statSync) on every invocation.
Since the manifest is already parsed at the beginning of columnReach, it is highly recommended to refactor exposuresOn to accept the parsed manifest object to eliminate the redundant synchronous disk I/O.
There was a problem hiding this comment.
Declined: loadRawManifest is cached by mtime, so the extra call is one stat. Threading the parsed manifest through is not worth the signature change at this size.
| const affectedModelIds = new Set([ | ||
| targetModel.unique_id, | ||
| ...downstream.map((d) => modelsByName.get(d.name)?.unique_id).filter(Boolean), | ||
| ...downstream.map((d) => d.unique_id ?? modelsByName.get(d.name)?.unique_id).filter(Boolean), | ||
| ]) |
There was a problem hiding this comment.
[🟠 MEDIUM] Since targetModel.unique_id can be undefined, undefined may be added to the affectedModelIds Set. This causes the Set to have the type Set<string | undefined>, which is later passed to exposuresOn(args.manifest_path, affectedModelIds) that expects an Iterable<string>. This mismatch can cause TypeScript compiler errors under strict settings, and is logically imprecise. It is recommended to filter out falsy values from the entire array before initializing the Set.
Suggested change:
| const affectedModelIds = new Set([ | |
| targetModel.unique_id, | |
| ...downstream.map((d) => modelsByName.get(d.name)?.unique_id).filter(Boolean), | |
| ...downstream.map((d) => d.unique_id ?? modelsByName.get(d.name)?.unique_id).filter(Boolean), | |
| ]) | |
| const affectedModelIds = new Set( | |
| [ | |
| targetModel.unique_id, | |
| ...downstream.map((d) => d.unique_id ?? modelsByName.get(d.name)?.unique_id), | |
| ].filter((id): id is string => Boolean(id)) | |
| ) |
There was a problem hiding this comment.
Fixed in impact-analysis.ts: undefined ids are filtered out before building affectedModelIds, so the Set is typed Set<string>.
|
👋 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.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| exposures, | ||
| ...(reach && { column_reach: reach }), | ||
| ...(reachError && { column_reach_error: reachError }), | ||
| ...(exposuresError && { exposures_error: exposuresError }), |
There was a problem hiding this comment.
[SUGGESTION]: exposures_error is not forwarded to MCP structured content
The new metadata field is added here, but STRUCTURED_FIELDS.impact_analysis in mcp-server/server.ts does not list it (it does list the analogous column_reach_error). structuredResult therefore drops exposures_error, so a client that reads the structured fields instead of report still sees exposures: [] with no failure signal and renders "no dashboards" — the exact reading this fix was meant to prevent (the text notice only survives because it is also embedded in report). Add "exposures_error" to the impact_analysis allow-list.
There was a problem hiding this comment.
Fixed in server.ts: exposures_error is added to the impact_analysis structured fields, with a test.
| content: [{ type: "text" as const, text: error instanceof Error ? error.message : String(error) }], | ||
| isError: true, | ||
| ...(STRUCTURED_TOOLS.has(request.params.name) && { | ||
| structuredContent: { report: error instanceof Error ? error.message : String(error) }, |
There was a problem hiding this comment.
[SUGGESTION]: The error message is stringified twice
error instanceof Error ? error.message : String(error) is evaluated on line 199 and again here. Hoisting it into one const message = ... and reusing it for both the text content and the structured report keeps the two from drifting and removes the duplication introduced by this hunk.
There was a problem hiding this comment.
Fixed in server.ts: the message is computed once as message and reused for the text and structured content.
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 2 finding(s)
- 2 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/mcp-server/server.ts (L199-L203)
[🔵 LOW] This conditional expression for error message extraction is duplicated. Consider extracting it into a variable to avoid redundant evaluation and improve code readability.
Suggested change:
content: [{ type: "text" as const, text: error instanceof Error ? error.message : String(error) }],
isError: true,
...(STRUCTURED_TOOLS.has(request.params.name) && {
structuredContent: { report: error instanceof Error ? error.message : String(error) },
}),
2. packages/opencode/src/altimate/tools/impact-analysis.ts (L316-L319)
[🟠 MEDIUM] If the caught error contains multiline messages or stack traces (e.g., from parsing failures), appending it directly via string interpolation could break the visual layout and readability of this plain text report. Consider sanitizing the error message by replacing newlines with spaces.
Suggested change:
if (data.exposuresError) {
const sanitizedError = data.exposuresError.replace(/\r?\n/g, " ");
lines.push(`Exposures: not read (${sanitizedError})`)
lines.push("")
}
| content: [{ type: "text" as const, text: error instanceof Error ? error.message : String(error) }], | ||
| isError: true, | ||
| ...(STRUCTURED_TOOLS.has(request.params.name) && { | ||
| structuredContent: { report: error instanceof Error ? error.message : String(error) }, | ||
| }), |
There was a problem hiding this comment.
[🔵 LOW] This conditional expression for error message extraction is duplicated. Consider extracting it into a variable to avoid redundant evaluation and improve code readability.
Suggested change:
| content: [{ type: "text" as const, text: error instanceof Error ? error.message : String(error) }], | |
| isError: true, | |
| ...(STRUCTURED_TOOLS.has(request.params.name) && { | |
| structuredContent: { report: error instanceof Error ? error.message : String(error) }, | |
| }), | |
| content: [{ type: "text" as const, text: error instanceof Error ? error.message : String(error) }], | |
| isError: true, | |
| ...(STRUCTURED_TOOLS.has(request.params.name) && { | |
| structuredContent: { report: error instanceof Error ? error.message : String(error) }, | |
| }), |
There was a problem hiding this comment.
Fixed in server.ts: the error message is hoisted into one message variable.
| if (data.exposuresError) { | ||
| lines.push(`Exposures: not read (${data.exposuresError})`) | ||
| lines.push("") | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] If the caught error contains multiline messages or stack traces (e.g., from parsing failures), appending it directly via string interpolation could break the visual layout and readability of this plain text report. Consider sanitizing the error message by replacing newlines with spaces.
Suggested change:
| if (data.exposuresError) { | |
| lines.push(`Exposures: not read (${data.exposuresError})`) | |
| lines.push("") | |
| } | |
| if (data.exposuresError) { | |
| const sanitizedError = data.exposuresError.replace(/\r?\n/g, " "); | |
| lines.push(`Exposures: not read (${sanitizedError})`) | |
| lines.push("") | |
| } |
There was a problem hiding this comment.
Fixed in impact-analysis.ts: newlines in the exposures error are replaced with spaces in the report line. Test added.
|
👋 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. |
| const reachedIds = new Set([targetId, ...reached.map((m) => m.unique_id)]) | ||
| const certain = exposuresOn(manifestPath, reachedIds) | ||
| const maybe = exposuresOn(manifestPath, [...uncertain, ...untraced].map((m) => m.unique_id)).filter( | ||
| (e) => !certain.some((c) => (c.unique_id ?? c.name) === (e.unique_id ?? e.name)), |
There was a problem hiding this comment.
[SUGGESTION]: The ?? c.name fallbacks are unreachable
ColumnReachExposure.unique_id is a required string (line 28) and exposuresOn always sets it, so both ?? c.name fallbacks are dead code. Comparing the ids directly keeps the dedupe honest and removes the suggestion that an exposure can lack an id.
| (e) => !certain.some((c) => (c.unique_id ?? c.name) === (e.unique_id ?? e.name)), | |
| (e) => !certain.some((c) => c.unique_id === e.unique_id), |
There was a problem hiding this comment.
Fixed: exposuresOn always sets unique_id, so the filter in column-reach.ts now compares unique_id directly without the name fallback.
|
👋 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. |
b268877 to
3d30b5c
Compare
|
👋 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. |
3 similar comments
|
👋 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. |
|
👋 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. |
3d30b5c to
b7df342
Compare
|
👋 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. |
| // Find target model by name or unique_id | ||
| const modelNode = findModel(nodes, params.model) | ||
| // Find target model by name or unique_id; a snapshot by its unique_id (it has compiled SQL too) | ||
| // A model by name or unique_id; a snapshot by unique_id; either by its project-relative file path. |
There was a problem hiding this comment.
SUGGESTION: Two stacked comments say the same thing
// Find target model by name or unique_id; a snapshot by its unique_id (it has compiled SQL too) and the line below it restate the lookup rules twice — the first is a leftover from an earlier edit of this hunk. Keep one of them so the resolution order is stated once.
There was a problem hiding this comment.
Removed the duplicate comment in dbtLineage (native/dbt/lineage.ts); the resolution order is now stated once.
|
|
||
| const escapeRegExp = (text: string) => text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") | ||
|
|
||
| /** |
There was a problem hiding this comment.
SUGGESTION: This JSDoc describes namesColumn, not stripSql
"Whether compiled SQL names the column anywhere but as an output alias… used only to mark a model as one that may read the column" belongs to namesColumn, but it sits above stripSql (which has its own one-line description on line 109). As written stripSql carries two unrelated descriptions and namesColumn has none. Move this block down to namesColumn.
There was a problem hiding this comment.
Moved the JSDoc for namesColumn from stripSql onto namesColumn in column-reach.ts; stripSql has its own description.
| ...downstream.map((d) => modelsByName.get(d.name)?.unique_id).filter(Boolean), | ||
| ]) | ||
| const affectedModelIds = new Set( | ||
| [targetModel.unique_id, ...downstream.map((d) => d.unique_id ?? modelsByName.get(d.name)?.unique_id)].filter( |
There was a problem hiding this comment.
SUGGESTION: The modelsByName fallback is unreachable
findDownstream copies unique_id straight from the same manifest.models entries that build modelsByName, so when d.unique_id is missing the map's entry for that name lacks it too. With duplicate names across packages the map holds the last entry, so the fallback could only return another package's id if it ever ran. Dropping ?? modelsByName.get(d.name)?.unique_id also leaves modelsByName with no remaining use, so both can go.
There was a problem hiding this comment.
Removed the modelsByName fallback and the map in impact-analysis.ts; affectedModelIds now uses each downstream entry's own unique_id.
…analysis Builds on the dbt integration's structured dbt_lineage result. - dbt_lineage takes a project-relative SQL path or a snapshot as well as a model, and with include_graph returns the model's graph from the manifest (two levels up, everything downstream with exposures and tests, capped at 500 nodes). The manifest fallback reports upstream ids, column mappings and edges as data. - impact_analysis with a column traces each downstream model's and snapshot's compiled SQL in dependency order and reports column_reach: certain readers with the columns they carry, possible readers (filters and joins, select * over undocumented columns, shared table names, ephemeral parents), untraced models and the exposures on each. It matches models by unique_id and honours the dialect override. - The MCP server declares an output schema for both tools. Structured content is the tool's own result when it builds one (the dbt integration's model lineage), else its lineage fields, always with the text report and capped at 100 KB, errors included. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…test proves which manifest the graph reads Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 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. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 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. |
| await writeFile(path.join(other, "target", "manifest.json"), "{ not json") | ||
| const viaDots = await tool.execute({ model: "customers", manifest_path: `${mine}/out/../target/manifest.json`, include_graph: true }, ctx) | ||
| expect(viaDots.metadata.structuredContent).toEqual(TRACE) | ||
| expect(String(viaDots.metadata.graph_error ?? "")).not.toMatch(/json|parse/i) |
There was a problem hiding this comment.
[WARNING]: This assertion cannot hold with the current code
manifestGraph (tools/dbt-lineage.ts:70) reads the manifest from the raw args.manifest_path through loadRawManifest, whose existsSync/realpathSync follow the out link and then .., so ${mine}/out/../target/manifest.json is other/target/manifest.json — the file just made invalid — and graph_error becomes its JSON parse error (/json|parse/i matches it). The adapter answers only because manifestOutsideAdapterProject collapses .. lexically with path.resolve before realpathSync (tools/dbt-lineage.ts:145), which is the opposite of the resolver this repo documents in Filesystem.realLocation (symlink first, then ..). The placement helper and the graph reader therefore disagree, so the test fails; if the manifest mtime cache ever hides the rewrite, it stops proving which file was read. Resolve the manifest path once, with the same symlink-aware .. semantics the reader uses, and use that one path for both the placement check and manifestGraph.
There was a problem hiding this comment.
The assertion holds; this file passes locally at 315685b (16 pass). In loadRawManifest, existsSync only checks that the path exists. The file it reads is fs.realpathSync(manifestPath), and realpathSync collapses .. before it follows links.
- I probed the same layout (
mine/out -> other/deep,other/target/manifest.jsoninvalid,mine/target/manifest.json={}). - On
mine/out/../target/manifest.json, Bun and Node both give:realpathSync->mine/target/manifest.json, read through it ->{}. Only a rawreadFileSyncof the unresolved path returns{ not json. - So the graph reader opens this project's manifest, and the placement check (
path.resolve, thenrealpathSync) places it in the same file. That agreement is what this test locks in: ifloadRawManifestever read the raw path instead,graph_errorwould carry the parse error and the test would fail. - I tried
Filesystem.realLocationin 5b67069, and it made the check disagree with this reader, so it was reverted in 6d71706.
…, and a column reach that stopped early reports lower bounds
- IDENTIFIER('col') with a traced column, or a bind variable, makes the model uncertain and passes the doubt on, as a NATURAL JOIN does
- the text report says "at least" when column reach stopped early, and the exposures past the stop are unchecked, not non-readers
- the model parameter says a snapshot is found by name too
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lookup is bounded like the NATURAL JOIN one Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a quoted name with a dot is never split Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the column, so no escape can hide one Escapes (\x72), backslash-escaped quotes and other characters are not decoded; such a string marks the model uncertain, which only adds doubt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ming another column is cleared Concatenations, bind and session variables, functions and unterminated strings all count as possibly naming the column, so no argument form can produce a confident zero. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 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.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| const others = (data.exposures ?? []).filter((e) => !listed.has(e.unique_id)) | ||
| if (others.length > 0) { | ||
| // When column reach stopped early, the rest are unchecked rather than known not to read the column. | ||
| lines.push(`${data.columnReach ? (data.columnReach.truncated ? "Other exposures (column reach stopped before checking them)" : "Other exposures") : "Exposures"} (${others.length})`) |
There was a problem hiding this comment.
[SUGGESTION]: This header mislabels exposures column reach did check
When truncated is set, others is every exposure not listed by the column reach block, which includes exposures on models column reach did trace before the cut and found not to read the column. Those are known non-readers, so "column reach stopped before checking them" is wrong for them. The Column Reach line already ends with (stopped early); keeping the plain Other exposures label stays accurate for the whole list and avoids the doubled parenthetical (… (…) (N)).
There was a problem hiding this comment.
Addressed in b84dd26 and a36f4be. You're right that some of them were checked. The header now says "Other exposures (N)", with ": column reach stopped early, so some of these may not have been checked" after the count, only when it stopped early. That keeps the doubled parenthetical out, and still doesn't present the exposures past the stop as known non-readers. The "(stopped early)" on the Column Reach line covers the counts above it, not this list.
| // The column named as a string the engine cannot trace: its outputs are unknown, like a NATURAL JOIN's. | ||
| const viaIdentifier = namesThroughIdentifier( | ||
| sqlText, | ||
| parents.flatMap((parent) => [...tracked.get(parent)!]), |
There was a problem hiding this comment.
[SUGGESTION]: The parent-column union is built twice
parents.flatMap((parent) => [...tracked.get(parent)!]) is computed here for namesThroughIdentifier and again, unchanged, in the named expression below (const named = parents.flatMap(...)). Hoisting it once (const trackedColumns = parents.flatMap(...)) and using it at both sites keeps the two scans over the same parents from drifting and removes the duplicate allocation.
There was a problem hiding this comment.
Done in b84dd26: trackedColumns is built once and used by both namesThroughIdentifier and named. Nothing writes a parent's entry between the two uses, so this is a pure refactor.
…her exposures are not all called unchecked - uncertain and untraced counts say "at least" too when column reach stopped early - the other exposures' header says some of them were not checked, after the count - the parents' traced columns are built once Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ecked Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 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. |
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. |
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Re-review at a36f4be: approved. MAJOR 1 is fixed. I found no critical or major issues, and the open minors below don't block.
Earlier findings:
- MAJOR 1 (an unreadable
manifest_paththrew away the adapter's answer): fixed.- A path that doesn't exist yet is placed through its nearest existing folder.
- An in-project path keeps the adapter's answer. A missing outside path overrides only when a
dbt_project.ymlsits above it. dbt-lineage-structured.test.ts:155-246covers this with real files and symlinks.
- MINOR 1 (whole-row reads): partly fixed, the rest reasonably deferred. NATURAL JOIN in every form is now uncertain and passes its doubt on; I checked this with the real engine.
to_json(o)andmd5(cast(o as varchar))still give a confident non-reader, but the code and the PR body now name that gap. - MINOR 2 (snapshots in
downstream/exposures): reasonably deferred. It is pre-existing and listed in the follow-ups. - Nits: the
include_graphnote,count_big/count_if, the backtick alias,unique_idandbyPathare fixed. Cycles, the typo'd column, tests beyond the graph cap andsynapseare reasonably deferred.
New minors (inline), neither blocks:
select * from identifier('<parent relation>')is a confident non-reader. Blocker: no. It is an edge input, since dbt models reach parents throughref(), and it belongs to the whole-row family the PR already documents.- Manifest placement re-implements
Filesystem.realLocation. Blocker: no. Today's behaviour matches what the manifest reader opens, so this is maintainability and safe to do in a follow-up.
The 6 PR test files pass at a36f4be (120 tests). The stopped-early report is truthful: every count says "at least", the other-exposures header says some were not checked, and truncated is in the structured result. Nits are in a separate PR comment.
| // Any mention of the column in the string counts (qualified, quoted, with dots in a quoted name); | ||
| // a doubled quote inside a quoted name (`"a""b"`) is one quote. | ||
| const text = literal.replace(/""/g, '"').toLowerCase() | ||
| if ([...wanted].some((column) => text.includes(column))) return true |
There was a problem hiding this comment.
Minor: select * from identifier('<parent relation>') gives a confident non-reader
namesThroughIdentifier clears any IDENTIFIER() whose literal is a plain name that doesn't contain the traced column. That also covers the relation form, where IDENTIFIER names the parent table.
- The engine can't resolve that relation, so it reports no edge.
selectsStaris true, but the parent documents the column, sowildcardis false.- The SQL never names the column, and
opaqueis false.
So the model lands in none of models/uncertain/untraced, and neither do its children and dashboards.
Real-engine probe on the test manifest (snowflake): rpt_counts with either SQL below is reported nowhere.
select * from identifier('"shop"."main"."fct_orders"')select o.* from identifier('shop.main.fct_orders') o
Fix: treat an IDENTIFIER() right after FROM/JOIN as opaque, or any IDENTIFIER() when selectsStar(sql) is true. Alternatively, clear only a literal that has no dot and matches none of the parents' relationKeys. Add a test for the relation form.
Not blocking: it's an edge input, since dbt models reach parents through ref(), which compiles to a plain relation. It's the same whole-row family the PR already lists as a known gap.
| // followed to where it points. `..` is collapsed first, as the manifest reader (loadRawManifest's | ||
| // realpathSync) does, so this places the file that reader opens. Undefined past 64 links (a loop); | ||
| // climbing to the nearest existing folder is not counted. | ||
| const real = (p: string) => { |
There was a problem hiding this comment.
Minor: manifest placement re-implements Filesystem.realLocation
5b67069 switched this to Filesystem.realLocation. 6d71706 then replaced it with this local loop, which collapses .. lexically to match loadRawManifest's realpathSync. Today the placement does match the file the reader opens: I checked <proj>/link/../target/manifest.json under bun and node, and the tests pass.
Still, the repo now has two link resolvers that disagree:
- Link budget: 64 here, 40 in
realLocation. ..handling: lexical here, the kernel's inrealLocation.- Loop behaviour: this one returns undefined,
realLocationthrows. - Windows: this one skips the
windowsPathnormalisation thatexternal-directory.tsapplies before callingrealLocation.
One edge follows from the reader itself. loadRawManifest's own existsSync is physical, so take <proj>/link/../target/manifest.json where only the link's physical target exists: placement says "inside", and the graph read fails with ENOENT. That surfaces as graph_error, not wrong data, and the helper predates this PR.
Fix (follow-up is fine): add a lexical-.. option to the shared helper, or resolve the path once and hand that same resolved path to both the placement check and loadRawManifest. Then a single resolver decides both. The 5b67069 commit message also no longer describes the code.
Not blocking: the behaviour is correct today; this is maintainability.
|
Re-review nits for a36f4be. None of these block.
|
…ctured content (#1637) `dbt_lineage` returns the dbt integration's model lineage as structured content, and a read-only call answers from the manifest without starting the dbt integration. Includes #1635: structured lineage and column reach for `dbt_lineage` and `impact_analysis`, with the review fixes for answering from the session's own dbt project and honouring a cancel while the project is read. Closes #1636 Co-authored-by: Harish Kumar <harish@altimate.ai> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNQjkcTQBy2pEa6qoBZQfb
main was merged into the release branch, bringing #1637 (with #1635): structured `dbt_lineage`, column reach in `impact_analysis`, and read-only `dbt_lineage` calls answering from the manifest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XM3J9mB75wZRnzTSGcW2kc
Issue for this PR
Stacked on #1637 (the dbt integration's structured
dbt_lineage), so it merges after #1637. It no longer depends on #1622. The Claude Code plugin's lineage pane consumes this (AltimateAI/altimate-claude-plugin#14).Type of change
What does this PR do?
Gives clients that draw lineage structured data instead of text, and makes column impact trace real downstream SQL. On top of #1637: when the dbt integration answers,
dbt_lineagekeeps its model lineage as the structured result, adding the manifest graph oninclude_graph; the manifest fallback reports its own lineage fields.dbt_lineagereports the model's upstream ids, column mappings and edges in its metadata. It finds a model by name, unique_id or project-relative SQL path, and a snapshot by name or id. Withinclude_graphit also returns the model's graph: everything upstream and downstream, up to 500 nodes each side (upTruncated/truncatedwhen a side is cut). This is for clients that cannot read a manifest over 4 MiB; over MCP the graph is left out of the structured result, so they read it throughaltimate tool run.impact_analysiswith a column now traces each downstream model's (and snapshot's) compiled SQL in dependency order, so renames are followed. It returnscolumn_reach:uncertain: models that name the column outside the select list, select*from a parent whose schema.yml does not list it, use a NATURAL JOIN or name it through Snowflake'sIDENTIFIER('…'), read a parent whose table name another parent shares, read an ephemeral parent, or sit downstream of one of these;untraced: no compiled SQL, or SQL the engine could not parse;uncertain_exposures.It never reports a confident zero for what the engine reports it cannot settle. One known gap: a parent's whole row passed as a value (
to_json(o)) gives no edge and is not caught (see Follow-ups). It also reportsdownstream,exposures, andcolumn_reach_error. The model can be given by unique_id, and downstream matches by exact id when one is given (versioned models, same names across packages). Thedialectoverride is honoured; without it, the dialect is detected from the manifest.MCP server: both tools declare an output schema and return those fields as structured content, with the text report as
report. Fordbt_lineage,reportis only the header, since the mappings travel as data. When the dbt integration answers, its model lineage is the structured result. The structured content is capped at 60 KB, measured as serialized JSON, so it stays under Claude Code's 25k-token MCP output limit; the dbt integration's report is sent as its header only, since its edges travel as data. Above that only the report is sent, markedtruncated, and the report itself is shortened if it is still too large.Adapter and manifest from the same project: when
manifest_pathnames a manifest outside the dbt integration's project (another folder, or a nested project inside it),dbt_lineageuses that manifest and says why, so one project's lineage is never mixed with another's graph. A manifest not written yet (an uncompiled project) is placed by its path, so the project's owntarget/manifest.jsonkeeps the integration's answer.How did you verify your code works?
bun test test/altimate/column-reach.test.ts test/altimate/dbt-lineage-structured.test.ts test/altimate/tools/impact-analysis.test.ts test/altimate/impact-analysis.test.ts test/altimate/dbt-lineage-adapter.test.ts: 106 pass (withdbt-lineage-helpers.test.ts, 110 across the six files). The tests cover:IDENTIFIER()reads, a manifest path that is a link or runs through a file, and SQL paths spelled with./or backslashes;include_graphreturning a twelve-hop chain whole to its source, and the 500-node cut on each side;select *, column spellings that differ only by case are reported as uncertain, an emptycolumn_lineagefalls back tocolumn_dict, dollar-quoted literals are blanked,graph_errorexplains a missing graph, a model the engine traces only partly or finds no edges in is uncertain, never a confident non-reader, and dashboards on a metric are found through its semantic model.ALTIMATE_MCP_E2E=1 bun test test/altimate/mcp-server/: 115 of 116 pass, including the structured output. The one failure,warehouse_list discovery stops starting calls once its deadline passes, comes from the base branch. It timed out in the full 16-minute run but passes alone at this PR's head and on the base branch, and this PR does not touchwarehouse_list.bun run typecheck: clean. The marker guard and tracker-leak check pass.Ran against a compiled dbt-duckdb demo project (20 models, 3 dashboard exposures) through
altimate tool runand the MCP server inside Claude Code.fct_orders.revenueis read by 7 downstream models and 3 dashboards, which matches the SQL by hand.Column reach over 300 downstream models of 41 columns each takes 548 ms.
Not from this PR: when these test files run together in one process, 41 MCP E2E tests and 2 runner tests fail. They fail the same way on the base branch and pass file by file.
Reviewed by Claude and Codex as separate reviewers, each run on its own, until both explicitly approved.
All tests
Every test this PR adds (57). It also extends existing tests: the adapter-project test in
dbt-lineage-structured.test.tsand the structured-output checks inmcp-server/server.test.ts.column-reach.test.ts: 45 testsdbt-lineage-helpers.test.ts: 1 testdbt-lineage-structured.test.ts: 2 testsimpact-analysis.test.ts: 1 testtools/impact-analysis.test.ts: 8 testsFollow-ups (not fixed here):
target/manifest.jsondoes not gets a graph node with no edges.dbt parsefixes it.impact_analysis's downstream traversal (from Add impact analysis, training import tools, and CI validation features #350) scans every model per level (O(V·M)). An adjacency map would make it O(V + E). It also walks models only, so a snapshot and what sits below it are missing fromdownstream, the severity count andexposures, whilecolumn_reachtraces through snapshots. Walking the manifest'schild_mapover models and snapshots fixes both. This is pre-existing and unchanged here.to_json(o),md5(cast(o as varchar))): the engine reports no column edge, and a pattern can't tell a row source from a column (sum(x) as x,extract(year from d)), so these models are confident non-readers today. The engine reporting a whole-row reference would settle it.detectDialect(pre-existing) passessynapseandfabricthrough, which the engine rejects, so their models come back untraced (reported, never a false zero). Mapping both totsqlwould trace them.getModelLineage,impact_analysis's column reach can ask it first (its column edges across hops) and keep this PR's compiled-SQL tracer as the fallback for projects without the dbt integration. Today the tracer is the only path that works, since the integration is not available yet.Cost side effects
None. No model calls or warehouse queries. Column reach parses compiled SQL locally, at most 300 models per call.
Screenshots / recordings
The plugin PR has screenshots of the pane that uses this data. A demo recording will follow.
Checklist
🤖 Generated with Claude Code