fix(cli): give the tables section and the divergence anchors one tabl… · deepnote/deepnote@cf94e5d · GitHub
Skip to content

Commit cf94e5d

Browse files
jamesbhobbsclaude
jamesbhobbs
and
claude
committed
fix(cli): give the tables section and the divergence anchors one table identity
The `tables` section keyed a table by the name as written, so `FROM analytics.users` and `FROM users` became two rows with two separate reach counts — while the join anchors, which already fold to the short name, treated them as one table. The same tool answered "what is this table" two different ways in two sections, and reach is the liveness-weighted multiplier the entire severity ranking rests on, so the split under-counted and mis-ranked everything downstream of it. Both now go through `canonicalTableKey`. Folding to the short name globally would conflate a `users` in one warehouse with a `users` in another, so the key is scoped by integration: a block's `sql_integration_id`, or an `unknown` bucket that is never pooled with a declared one. What integration scoping does not resolve is `analytics.users` and `staging.users` behind a *single* integration, which still merge. Telling those apart needs to know which schema an unqualified `users` resolved to, which is a property of the warehouse search path rather than of the query text, and merging under-counts rather than invents. Every qualified spelling observed is kept in `qualifiedNames` so the conflation is visible on the row, and the text output qualifies a short name with its integration whenever the same name appears behind more than one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c9e83f1 commit cf94e5d

5 files changed

Lines changed: 219 additions & 10 deletions

File tree

‎docs/deepnote-cli-audit.md‎

Lines changed: 11 additions & 1 deletion

‎packages/cli/src/commands/audit.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,14 +227,20 @@ function outputTables(audit: WorkspaceAudit): void {
227227
output(c.bold('Tables — ranked by live reach'))
228228

229229
const shown = audit.tables.slice(0, MAX_LISTED_ROWS)
230+
// A short name is only unique within one integration, so it is qualified in the output whenever
231+
// the same name appears behind more than one — otherwise two different tables print identically.
232+
const ambiguous = new Set(
233+
audit.tables.map(table => table.name).filter((name, index, names) => names.indexOf(name) !== index)
234+
)
230235
for (const table of shown) {
231236
// Both numbers, always. The raw count is the one people quote; the live count is the one that
232237
// is true, and printing them together is what stops a blast radius being read as 8× its size.
233238
const reach =
234239
table.liveProjectCount === table.projectCount
235240
? `${plural(table.projectCount, 'project')}`
236241
: `${table.liveProjectCount} live of ${plural(table.projectCount, 'project')}`
237-
output(` ${table.name} ${c.dim(`— ${reach}, ${plural(table.blockCount, 'SQL block')}`)}`)
242+
const label = ambiguous.has(table.name) ? `${table.name} ${c.dim(`(${table.integrationId})`)}` : table.name
243+
output(` ${label} ${c.dim(`— ${reach}, ${plural(table.blockCount, 'SQL block')}`)}`)
238244
}
239245
const more = remainder(audit.tables.length, shown.length)
240246
if (more) {

‎packages/cli/src/utils/governance/audit.test.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -952,3 +952,133 @@ describe('holderReach', () => {
952952
expect(holderReach(undefined, ages)).toEqual({ live: 0, total: 1 })
953953
})
954954
})
955+
956+
describe('auditWorkspace — one table identity, shared with the divergence anchors', () => {
957+
it('merges a qualified and a bare reference to the same table', () => {
958+
// Keyed on the name as written, these were two rows with two reach counts — while the join
959+
// anchors, which fold to the short name, treated them as one table. Reach is the multiplier
960+
// the whole ranking rests on, so the split under-counted every finding on either row.
961+
const audit = auditWorkspace(
962+
workspace([
963+
project(
964+
'p1',
965+
'Qualified',
966+
[
967+
{
968+
name: 'A',
969+
blocks: [{ id: 'b1', type: 'sql', content: 'SELECT * FROM analytics.users', integrationId: 'wh' }],
970+
},
971+
],
972+
[{ id: 'wh', name: 'Warehouse', type: 'snowflake' }]
973+
),
974+
project(
975+
'p2',
976+
'Bare',
977+
[{ name: 'B', blocks: [{ id: 'b2', type: 'sql', content: 'SELECT * FROM users', integrationId: 'wh' }] }],
978+
[{ id: 'wh', name: 'Warehouse', type: 'snowflake' }]
979+
),
980+
])
981+
)
982+
983+
const users = audit.tables.filter(table => table.name === 'users')
984+
expect(users).toHaveLength(1)
985+
expect(users[0].projectCount).toBe(2)
986+
expect(users[0].blockCount).toBe(2)
987+
// Both spellings are kept, so the merge is inspectable rather than silent.
988+
expect(users[0].qualifiedNames).toEqual(['analytics.users', 'users'])
989+
})
990+
991+
it('keeps same-named tables behind two integrations apart', () => {
992+
const audit = auditWorkspace(
993+
workspace([
994+
project(
995+
'p1',
996+
'Prod',
997+
[
998+
{
999+
name: 'A',
1000+
blocks: [{ id: 'b1', type: 'sql', content: 'SELECT * FROM analytics.users', integrationId: 'prod' }],
1001+
},
1002+
],
1003+
[{ id: 'prod', name: 'Prod warehouse', type: 'snowflake' }]
1004+
),
1005+
project(
1006+
'p2',
1007+
'Staging',
1008+
[
1009+
{
1010+
name: 'B',
1011+
blocks: [{ id: 'b2', type: 'sql', content: 'SELECT * FROM staging.users', integrationId: 'stg' }],
1012+
},
1013+
],
1014+
[{ id: 'stg', name: 'Staging warehouse', type: 'snowflake' }]
1015+
),
1016+
])
1017+
)
1018+
1019+
const users = audit.tables.filter(table => table.name === 'users')
1020+
expect(users).toHaveLength(2)
1021+
expect(users.map(table => table.integrationId).sort()).toEqual(['prod', 'stg'])
1022+
expect(users.every(table => table.projectCount === 1)).toBe(true)
1023+
})
1024+
1025+
it('keeps a block that declares no integration out of a known integration bucket', () => {
1026+
const audit = auditWorkspace(
1027+
workspace([
1028+
project(
1029+
'p1',
1030+
'Declared',
1031+
[{ name: 'A', blocks: [{ id: 'b1', type: 'sql', content: 'SELECT * FROM users', integrationId: 'wh' }] }],
1032+
[{ id: 'wh', name: 'Warehouse', type: 'snowflake' }]
1033+
),
1034+
project('p2', 'Undeclared', [
1035+
{ name: 'B', blocks: [{ id: 'b2', type: 'sql', content: 'SELECT * FROM users' }] },
1036+
]),
1037+
])
1038+
)
1039+
1040+
const users = audit.tables.filter(table => table.name === 'users')
1041+
expect(users).toHaveLength(2)
1042+
expect(users.map(table => table.integrationId).sort()).toEqual(['unknown', 'wh'])
1043+
})
1044+
1045+
it('scores a finding against the merged reach, not half of it', () => {
1046+
const qualified = (id: string, name: string) =>
1047+
datedProject(
1048+
id,
1049+
name,
1050+
daysAgo(10),
1051+
[
1052+
{
1053+
name: 'N',
1054+
blocks: [
1055+
{
1056+
id: `${id}-b`,
1057+
type: 'sql',
1058+
content: 'SELECT * FROM analytics.users WHERE deleted_at = NULL',
1059+
integrationId: 'wh',
1060+
},
1061+
],
1062+
},
1063+
],
1064+
[{ id: 'wh', name: 'Warehouse', type: 'snowflake' }]
1065+
)
1066+
const bare = (id: string, name: string) =>
1067+
datedProject(
1068+
id,
1069+
name,
1070+
daysAgo(10),
1071+
[{ name: 'N', blocks: [{ id: `${id}-b`, type: 'sql', content: 'SELECT * FROM users', integrationId: 'wh' }] }],
1072+
[{ id: 'wh', name: 'Warehouse', type: 'snowflake' }]
1073+
)
1074+
1075+
const audit = auditWorkspace(
1076+
workspace([qualified('p1', 'One'), ...['p2', 'p3', 'p4', 'p5'].map(id => bare(id, id))]),
1077+
{ now: NOW }
1078+
)
1079+
1080+
// The four bare readers count towards the reach of the one qualified writer's finding.
1081+
expect(audit.tables.filter(table => table.name === 'users')).toHaveLength(1)
1082+
expect(issuesOf(audit, 'sql-null-comparison')[0].score.blastRadius).toBeGreaterThan(0.75)
1083+
})
1084+
})

‎packages/cli/src/utils/governance/audit.ts‎

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { getBlockLabel } from '../block-label'
2121
import { findExternalEndpoints } from './egress'
2222
import { redactSecrets, runProjectGovernanceChecks } from './index'
2323
import { type SeverityScore, scoreFinding } from './scoring'
24-
import { findTableReferences } from './sql-tables'
24+
import { canonicalTableKey, findTableReferences, UNKNOWN_INTEGRATION_SCOPE } from './sql-tables'
2525
import { type AssetAge, assetAge, formatAge, medianAgeDays } from './staleness'
2626
import { buildSubjectIndex, SCATTER_NOTEBOOK_THRESHOLD } from './subject-index'
2727
import { createSubjectFingerprinter, redactSubjects } from './subjects'
@@ -70,8 +70,21 @@ export interface IntegrationUsage {
7070

7171
/** A table referenced by the workspace's SQL, and how much live work depends on it. */
7272
export interface TableUsage {
73-
/** Name as written, lower-cased: `analytics.public.users`. */
73+
/** The table's short name, lower-cased: `users` for `analytics.public.users`. */
7474
name: string
75+
/**
76+
* The integration this row is scoped to, or `unknown` for blocks that declare none.
77+
*
78+
* A `users` behind two warehouses is two rows, because it is two tables.
79+
*/
80+
integrationId: string
81+
/**
82+
* Every qualified spelling seen for this table, sorted. `['users']` when nobody qualified it.
83+
*
84+
* Present so that merging `analytics.users` with a bare `users` is visible rather than silent —
85+
* two schemas behind one integration will show up here as two entries on one row.
86+
*/
87+
qualifiedNames: string[]
7588
/** Projects whose SQL references it. */
7689
projectCount: number
7790
/** Of those, the ones edited within the past year. */
@@ -455,18 +468,28 @@ export function auditWorkspace(workspace: LoadedWorkspace, options: AuditOptions
455468
if (block.type === 'sql') {
456469
sqlBlockCount++
457470
const referenced = findTableReferences(content)
471+
// Keyed through `canonicalTableKey` so that `analytics.users` and a bare `users` behind
472+
// the same integration are one row with one reach count — the same identity the
473+
// divergence anchors use, rather than a second answer to the same question.
458474
tablesByBlock.set(
459475
block.id,
460-
referenced.map(reference => reference.name)
476+
referenced.map(reference => canonicalTableKey(reference.shortName, integrationId))
461477
)
462-
for (const { name } of referenced) {
463-
const usage = tables.get(name) ?? {
464-
name,
478+
for (const { name, shortName } of referenced) {
479+
const key = canonicalTableKey(shortName, integrationId)
480+
const usage = tables.get(key) ?? {
481+
name: shortName.toLowerCase(),
482+
integrationId: integrationId ?? UNKNOWN_INTEGRATION_SCOPE,
483+
qualifiedNames: [],
465484
projectCount: 0,
466485
liveProjectCount: 0,
467486
blockCount: 0,
468487
projects: [],
469488
}
489+
if (!usage.qualifiedNames.includes(name)) {
490+
usage.qualifiedNames.push(name)
491+
usage.qualifiedNames.sort()
492+
}
470493
if (!usage.projects.some(entry => entry.projectId === project.id)) {
471494
usage.projects.push({ projectId: project.id, projectName: project.name, live: projectIsLive })
472495
usage.projectCount++
@@ -475,7 +498,7 @@ export function auditWorkspace(workspace: LoadedWorkspace, options: AuditOptions
475498
}
476499
}
477500
usage.blockCount++
478-
tables.set(name, usage)
501+
tables.set(key, usage)
479502
}
480503
}
481504
if (integrationId) {
@@ -807,7 +830,12 @@ export function auditWorkspace(workspace: LoadedWorkspace, options: AuditOptions
807830
// bigger dependency than one three live projects query.
808831
tables: [...tables.values()].sort(
809832
(a, b) =>
810-
b.liveProjectCount - a.liveProjectCount || b.projectCount - a.projectCount || a.name.localeCompare(b.name)
833+
b.liveProjectCount - a.liveProjectCount ||
834+
b.projectCount - a.projectCount ||
835+
a.name.localeCompare(b.name) ||
836+
// The short name is no longer unique now that rows are scoped per integration, so the
837+
// scope is the final tie-break. Without it two warehouses' `users` would sort unstably.
838+
a.integrationId.localeCompare(b.integrationId)
811839
),
812840
staleness,
813841
egress: [...egress.values()].sort((a, b) => b.blockCount - a.blockCount || a.host.localeCompare(b.host)),

‎packages/cli/src/utils/governance/sql-tables.ts‎

Lines changed: 35 additions & 0 deletions

0 commit comments

Comments
 (0)