fix(schema): declare Swift member-containment pairs in CONTAINS DDL (#2769)
* fix(schema): declare Swift member-containment pairs in CONTAINS DDL * fix(schema): declare remaining Rust impl/trait and JS/TS object-literal HAS_METHOD pairs; guard streamed emit sinks against undeclared pairs (PR #2769 review) * refactor(schema): share one declared-pairs constant across router and sinks DECLARED_REL_PAIRS was being computed independently in three places (csv-generator.ts, graph-emit-sink.ts, pdg-emit-sink.ts) from the same static RELATION_SCHEMA parse. Export the existing constant from csv-generator.ts (already imported by both sinks) instead. assertDeclaredPair now takes the pre-built pairKey rather than the two labels, since every caller (RelPairRouter.route, both sinks' addRelationship) needs that same key immediately after for its own Map/stream lookup on the per-streamed-edge hot path — avoids rebuilding the template string twice per edge. Also drops two schema.test.ts assertions that duplicated coverage already in the more narrowly-named regression tests below them, and trims the v32 ladder comment to point at assertDeclaredPair's docstring instead of re-explaining the same failure mechanism. * fix(schema): use replaceAll for the pair-arrow error message (CodeQL) .replace(str, ...) only touches the first match; CodeQL flags that as incomplete string escaping regardless of the caller's invariant that pairKey contains exactly one '|'. replaceAll is equivalent here and silences the alert. --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
This commit is contained in:
Karl Lehenbauer
co-authored by
Gergo Magyar
Gergo Magyar
parent
c238085676
commit
bebb1d2367
@@ -23,7 +23,10 @@ import { parseTruthyEnv } from '../ingestion/utils/env.js';
|
||||
import { SYMBOL_NODE_LABELS } from '../ingestion/utils/symbol-labels.js';
|
||||
import { applyCjkSegmentationIfEnabled } from '../search/cjk-segmentation.js';
|
||||
|
||||
const DECLARED_RELATION_PAIRS = parseRelationSchemaPairs(RELATION_SCHEMA);
|
||||
/** Computed once — `RELATION_SCHEMA` is a static template literal. Exported so
|
||||
* the streamed sinks (`GraphEmitSink`, `PdgEmitSink`) share this parse
|
||||
* instead of each re-deriving it from the same DDL string. */
|
||||
export const DECLARED_RELATION_PAIRS = parseRelationSchemaPairs(RELATION_SCHEMA);
|
||||
|
||||
/**
|
||||
* Deterministic output ordering — optional (out-of-core / windowed-resolve
|
||||
|
||||
@@ -95,8 +95,8 @@ import fs from 'fs';
|
||||
import path from 'path';
|
||||
import type { GraphNode, GraphRelationship, RelationshipType } from 'gitnexus-shared';
|
||||
import type { KnowledgeGraph } from '../graph/types.js';
|
||||
import { REL_CSV_HEADER, buildRelRow } from './csv-generator.js';
|
||||
import { getNodeLabel } from './rel-pair-routing.js';
|
||||
import { DECLARED_RELATION_PAIRS, REL_CSV_HEADER, buildRelRow } from './csv-generator.js';
|
||||
import { assertDeclaredPair, getNodeLabel } from './rel-pair-routing.js';
|
||||
import { NODE_TABLES } from './schema.js';
|
||||
import { DEFAULT_EMIT_CHUNK_ROWS, SyncCsvWriter } from './sync-csv-writer.js';
|
||||
|
||||
@@ -412,6 +412,7 @@ export class GraphEmitSink implements KnowledgeGraph, GraphEmitControl {
|
||||
if (!this.validTables.has(fromLabel) || !this.validTables.has(toLabel)) return;
|
||||
|
||||
const pairKey = `${fromLabel}|${toLabel}`;
|
||||
assertDeclaredPair(pairKey, DECLARED_RELATION_PAIRS);
|
||||
let writer = this.relWriters.get(pairKey);
|
||||
if (writer === undefined) {
|
||||
try {
|
||||
|
||||
@@ -49,11 +49,12 @@ import type { GraphNode, GraphRelationship, RelationshipType } from 'gitnexus-sh
|
||||
import type { KnowledgeGraph } from '../graph/types.js';
|
||||
import {
|
||||
BASICBLOCK_CSV_HEADER,
|
||||
DECLARED_RELATION_PAIRS,
|
||||
REL_CSV_HEADER,
|
||||
buildBasicBlockRow,
|
||||
buildRelRow,
|
||||
} from './csv-generator.js';
|
||||
import { getNodeLabel } from './rel-pair-routing.js';
|
||||
import { assertDeclaredPair, getNodeLabel } from './rel-pair-routing.js';
|
||||
import { DEFAULT_EMIT_CHUNK_ROWS, SyncCsvWriter } from './sync-csv-writer.js';
|
||||
import { NODE_TABLES, type NodeTableName } from './schema.js';
|
||||
|
||||
@@ -165,6 +166,7 @@ export class PdgEmitSink implements KnowledgeGraph {
|
||||
// `RelPairRouter` exactly so the streamed set matches the whole-graph set.
|
||||
if (!this.validTables.has(fromLabel) || !this.validTables.has(toLabel)) return;
|
||||
const pairKey = `${fromLabel}|${toLabel}`;
|
||||
assertDeclaredPair(pairKey, DECLARED_RELATION_PAIRS);
|
||||
let writer = this.relWriters.get(pairKey);
|
||||
if (writer === undefined) {
|
||||
try {
|
||||
|
||||
@@ -69,6 +69,29 @@ export interface RelPairMeta {
|
||||
rows: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Fail fast on an endpoint-label pair absent from the relationship DDL, the
|
||||
* same guard `RelPairRouter.route` applies to the whole-graph emit. Exported
|
||||
* so the streamed sinks (`GraphEmitSink`, `PdgEmitSink`) can apply it too —
|
||||
* without this, an undeclared pair on a streaming run reaches `COPY`, fails
|
||||
* the bulk insert, and is silently dropped by the per-edge fallback instead
|
||||
* of failing loudly like the non-streaming path does.
|
||||
*
|
||||
* Takes the already-built `From|To` pairKey rather than the two labels — every
|
||||
* caller needs that same key immediately after for its own Map/stream lookup,
|
||||
* and this is on the per-edge hot path, so building it twice would be a
|
||||
* needless allocation per edge. `|` cannot appear inside a label (node labels
|
||||
* are `NODE_TABLES` identifiers), so splitting it back apart for the error
|
||||
* message is safe.
|
||||
*/
|
||||
export const assertDeclaredPair = (pairKey: string, declaredPairs: ReadonlySet<string>): void => {
|
||||
if (!declaredPairs.has(pairKey)) {
|
||||
throw new Error(
|
||||
`Relationship label pair ${pairKey.replaceAll('|', '→')} is not declared in the LadybugDB relation schema`,
|
||||
);
|
||||
}
|
||||
};
|
||||
|
||||
/**
|
||||
* Routes already-escaped relationship CSV rows to per-FROM→TO-label-pair
|
||||
* files. Filters edges whose endpoint labels are not valid node tables
|
||||
@@ -124,11 +147,7 @@ export class RelPairRouter {
|
||||
}
|
||||
|
||||
const pairKey = `${fromLabel}|${toLabel}`;
|
||||
if (!this.declaredPairs.has(pairKey)) {
|
||||
throw new Error(
|
||||
`Relationship label pair ${fromLabel}→${toLabel} is not declared in the LadybugDB relation schema`,
|
||||
);
|
||||
}
|
||||
assertDeclaredPair(pairKey, this.declaredPairs);
|
||||
const ws = this.streams.get(pairKey);
|
||||
if (ws === undefined) {
|
||||
// First edge for this pair: open the stream, write header + row.
|
||||
|
||||
@@ -332,6 +332,8 @@ CREATE REL TABLE ${REL_TABLE_NAME} (
|
||||
FROM Method TO Interface,
|
||||
FROM Method TO \`Constructor\`,
|
||||
FROM Method TO \`Property\`,
|
||||
FROM Method TO \`Variable\`,
|
||||
FROM Method TO \`Const\`,
|
||||
FROM Method TO CodeElement,
|
||||
FROM \`Template\` TO \`Template\`,
|
||||
FROM \`Template\` TO Function,
|
||||
@@ -376,6 +378,12 @@ CREATE REL TABLE ${REL_TABLE_NAME} (
|
||||
FROM \`Enum\` TO Community,
|
||||
FROM \`Enum\` TO Class,
|
||||
FROM \`Enum\` TO Interface,
|
||||
FROM \`Enum\` TO Function,
|
||||
FROM \`Enum\` TO Method,
|
||||
FROM \`Enum\` TO \`Struct\`,
|
||||
FROM \`Enum\` TO \`Constructor\`,
|
||||
FROM \`Enum\` TO \`Property\`,
|
||||
FROM \`Enum\` TO \`TypeAlias\`,
|
||||
FROM \`Macro\` TO Community,
|
||||
FROM \`Macro\` TO Function,
|
||||
FROM \`Macro\` TO Method,
|
||||
@@ -386,10 +394,12 @@ CREATE REL TABLE ${REL_TABLE_NAME} (
|
||||
FROM \`Namespace\` TO Community,
|
||||
FROM \`Namespace\` TO \`Struct\`,
|
||||
FROM \`Trait\` TO Method,
|
||||
FROM \`Trait\` TO Function,
|
||||
FROM \`Trait\` TO \`Constructor\`,
|
||||
FROM \`Trait\` TO \`Property\`,
|
||||
FROM \`Trait\` TO Community,
|
||||
FROM \`Impl\` TO Method,
|
||||
FROM \`Impl\` TO Function,
|
||||
FROM \`Impl\` TO \`Constructor\`,
|
||||
FROM \`Impl\` TO \`Property\`,
|
||||
FROM \`Impl\` TO Community,
|
||||
@@ -400,10 +410,16 @@ CREATE REL TABLE ${REL_TABLE_NAME} (
|
||||
FROM \`TypeAlias\` TO \`Trait\`,
|
||||
FROM \`TypeAlias\` TO Class,
|
||||
FROM \`Const\` TO Community,
|
||||
FROM \`Const\` TO Method,
|
||||
FROM \`Static\` TO Community,
|
||||
FROM \`Variable\` TO Community,
|
||||
FROM \`Variable\` TO Method,
|
||||
FROM \`Property\` TO Community,
|
||||
FROM \`Property\` TO \`Property\`,
|
||||
FROM \`Property\` TO Class,
|
||||
FROM \`Property\` TO \`Enum\`,
|
||||
FROM \`Property\` TO Function,
|
||||
FROM \`Property\` TO \`Struct\`,
|
||||
FROM \`Record\` TO Method,
|
||||
FROM \`Record\` TO \`Constructor\`,
|
||||
FROM \`Record\` TO \`Property\`,
|
||||
|
||||
@@ -645,8 +645,31 @@ export interface RepoMeta {
|
||||
* as namespace edges (#2746), enabling qualified constructor and method CALLS
|
||||
* edges. Pre-v31 indexes retain the old package-target/missing-edge graph for
|
||||
* unchanged files through the reuse gate. Force a full re-analyze.
|
||||
*
|
||||
* v32: the relation DDL (the single shared `CodeRelation` REL TABLE) gains
|
||||
* sixteen FROM/TO pairs carried by `HAS_METHOD`/`HAS_PROPERTY` and
|
||||
* scope-resolution edges: Enum→{Function, Method, Struct, Constructor,
|
||||
* Property, TypeAlias}, Property→{Class, Enum, Function, Struct},
|
||||
* Method→{Variable, Const}, Trait→Function, Impl→Function, Const→Method and
|
||||
* Variable→Method. The Enum/Property set was observed on Swift (enums carry
|
||||
* computed properties, methods, initializers and nested types) and is also
|
||||
* reached by Java/PHP enum members; Trait/Impl→Function covers a Rust
|
||||
* `impl`/`trait` method, which is minted as a `Function` node, not `Method`;
|
||||
* Const/Variable→Method and its sibling Method→Const cover a JS/TS object
|
||||
* literal's shorthand methods, whose owner is labelled `Const`/`Variable`. A
|
||||
* pre-v32 database physically lacks these from-to pairs — see
|
||||
* `assertDeclaredPair` (rel-pair-routing.ts) for why an incremental top-up
|
||||
* fails loudly on one path and silently on the other. Force a full re-analyze.
|
||||
*
|
||||
* (This shipped as v31 on its own branch; `main` took 31 for #2746 first, so
|
||||
* it is renumbered here. Re-check both constants against origin/main
|
||||
* immediately before merging — this is the sixth time that collision has
|
||||
* bitten. If this change is ever reverted, do not free 32 for reuse — the
|
||||
* reuse gate is exact equality, so an index already stamped 32 would satisfy
|
||||
* it against a differently-shaped reverted DB. Start the next allocation at
|
||||
* 33 instead.)
|
||||
*/
|
||||
export const INCREMENTAL_SCHEMA_VERSION = 31;
|
||||
export const INCREMENTAL_SCHEMA_VERSION = 32;
|
||||
|
||||
export interface IndexedRepo {
|
||||
repoPath: string;
|
||||
|
||||
@@ -73,12 +73,12 @@ describe('CALL_SUMMARY relation-type exclusion (U-C1)', () => {
|
||||
});
|
||||
|
||||
describe('CALL_SUMMARY incremental reuse gate (U-C5)', () => {
|
||||
it('INCREMENTAL_SCHEMA_VERSION is bumped to 31 (Python module-import resolution, #2746)', () => {
|
||||
it('INCREMENTAL_SCHEMA_VERSION is bumped to 32 (Rust/Swift/JS-TS member-containment DDL, #2769)', () => {
|
||||
// Moves with every bump BY DESIGN — that is the point of pinning it. A
|
||||
// change that alters emitted ids or edges without bumping would otherwise
|
||||
// ship silently, and an existing index would keep serving the old graph
|
||||
// through the reuse gate below.
|
||||
expect(INCREMENTAL_SCHEMA_VERSION).toBe(31);
|
||||
expect(INCREMENTAL_SCHEMA_VERSION).toBe(32);
|
||||
});
|
||||
|
||||
it('a pre-current stamp fails the `=== INCREMENTAL_SCHEMA_VERSION` reuse gate → forces full re-analyze', () => {
|
||||
@@ -208,7 +208,12 @@ describe('CALL_SUMMARY incremental reuse gate (U-C5)', () => {
|
||||
// A pre-v31 (v30) index treats `from pkg import models` as a named package
|
||||
// import, so unchanged files retain the old missing qualified CALLS edges.
|
||||
expect(passesReuseGate(30)).toBe(false);
|
||||
// A pre-v32 (v31) index predates the Rust impl/trait, JS/TS object-literal
|
||||
// and Swift member-containment relation pairs (#2769), so an incremental
|
||||
// top-up emitting one of those edges would fail the bulk COPY (or silently
|
||||
// drop it on the streamed path) → must NOT reuse.
|
||||
expect(passesReuseGate(31)).toBe(false);
|
||||
// The current stamp passes the gate (incremental top-up eligible).
|
||||
expect(passesReuseGate(31)).toBe(true);
|
||||
expect(passesReuseGate(32)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -127,6 +127,29 @@ describe('GraphEmitSink routing', () => {
|
||||
expect(manifest).toMatchObject({ totalRows: 0 });
|
||||
expect(real.relationshipCount).toBe(0);
|
||||
});
|
||||
|
||||
it('throws rather than silently dropping an undeclared endpoint-label pair (#2769)', () => {
|
||||
// Before #2769's fix, this pair reached COPY, failed the bulk insert, and
|
||||
// the per-edge fallback swallowed the failure into `catch {}` — the run
|
||||
// still exited 0 with the edge silently missing. Both endpoints are valid
|
||||
// node tables (so the validTables gate above does not catch it); the pair
|
||||
// itself is simply absent from RELATION_SCHEMA.
|
||||
const real = createKnowledgeGraph();
|
||||
const sink = new GraphEmitSink(real, csvDir);
|
||||
sink.beginStreaming();
|
||||
|
||||
const undeclared: GraphRelationship = {
|
||||
id: 'CALLS:Static:a->Static:b',
|
||||
sourceId: 'Static:src/a.ts:A',
|
||||
targetId: 'Static:src/a.ts:B',
|
||||
type: 'CALLS',
|
||||
confidence: 1,
|
||||
reason: 'direct',
|
||||
};
|
||||
expect(() => sink.addRelationship(undeclared)).toThrow(
|
||||
/Relationship label pair Static→Static is not declared/,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('GraphEmitSink arming', () => {
|
||||
|
||||
@@ -78,6 +78,27 @@ describe('PdgEmitSink — routing', () => {
|
||||
sink.finalize();
|
||||
});
|
||||
|
||||
it('throws rather than silently dropping an undeclared endpoint-label pair (#2769)', () => {
|
||||
// PDG edges are always BasicBlock|BasicBlock in production, but the sink
|
||||
// applies no declared-pair gate of its own — this pins that the shared
|
||||
// `assertDeclaredPair` guard (also wired into GraphEmitSink) still fires
|
||||
// here rather than reaching COPY and silently dropping the edge.
|
||||
const real = createKnowledgeGraph();
|
||||
const sink = new PdgEmitSink(real, path.join(tmpRoot, 'pdg-csv'));
|
||||
|
||||
const undeclared: GraphRelationship = {
|
||||
id: 'CFG:Static:a->Static:b',
|
||||
sourceId: 'Static:src/a.ts:A',
|
||||
targetId: 'Static:src/a.ts:B',
|
||||
type: 'CFG',
|
||||
confidence: 1,
|
||||
reason: 'seq',
|
||||
};
|
||||
expect(() => sink.addRelationship(undeclared)).toThrow(
|
||||
/Relationship label pair Static→Static is not declared/,
|
||||
);
|
||||
});
|
||||
|
||||
it('delegates structural nodes, CALLS, and the whole-program TAINT_PATH edge to the real graph', () => {
|
||||
const real = createKnowledgeGraph();
|
||||
const sink = new PdgEmitSink(real, path.join(tmpRoot, 'pdg-csv'));
|
||||
|
||||
@@ -22,6 +22,7 @@ import {
|
||||
EMBEDDING_SCHEMA,
|
||||
CREATE_VECTOR_INDEX_QUERY,
|
||||
} from '../../src/core/lbug/schema.js';
|
||||
import { parseRelationSchemaPairs } from '../../src/core/lbug/rel-pair-routing.js';
|
||||
|
||||
describe('LadybugDB Schema', () => {
|
||||
describe('NODE_TABLES', () => {
|
||||
@@ -212,32 +213,70 @@ describe('LadybugDB Schema', () => {
|
||||
expect(RELATION_SCHEMA).toContain('FROM Class TO CodeElement');
|
||||
});
|
||||
|
||||
it('declares the Swift enum/property member-containment pairs (#2769)', () => {
|
||||
// Asserted through the runtime's own parser, not raw strings, so a
|
||||
// cosmetic DDL formatting change cannot fail this without a semantic
|
||||
// change (see PR #2769 review Finding 7). The Rust impl/trait and JS/TS
|
||||
// object-literal pairs get their own tests below — narrower and named
|
||||
// for the specific abort each one fixes.
|
||||
const declaredPairs = parseRelationSchemaPairs(RELATION_SCHEMA);
|
||||
const memberPairs = [
|
||||
// Swift enum members (also reached by Java/PHP enum methods)
|
||||
'Enum|Function',
|
||||
'Enum|Method',
|
||||
'Enum|Struct',
|
||||
'Enum|Constructor',
|
||||
'Enum|Property',
|
||||
'Enum|TypeAlias',
|
||||
'Property|Class',
|
||||
'Property|Enum',
|
||||
'Property|Function',
|
||||
'Property|Struct',
|
||||
'Method|Variable',
|
||||
'Method|Const',
|
||||
];
|
||||
|
||||
for (const pair of memberPairs) {
|
||||
expect(declaredPairs.has(pair)).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it('has all FROM/TO pairs needed for HAS_METHOD edges', () => {
|
||||
// HAS_METHOD sources: Class, Interface, Struct, Trait, Impl, Record
|
||||
// HAS_METHOD targets: Method, Constructor (Property is now HAS_PROPERTY)
|
||||
const declaredPairs = parseRelationSchemaPairs(RELATION_SCHEMA);
|
||||
const sources = ['Class', 'Interface'];
|
||||
const backtickSources = ['Struct', 'Trait', 'Impl', 'Record'];
|
||||
const targets = ['Method'];
|
||||
const backtickTargets = ['Constructor'];
|
||||
|
||||
// Non-backtick source → non-backtick target
|
||||
for (const src of sources) {
|
||||
for (const tgt of targets) {
|
||||
expect(RELATION_SCHEMA).toContain(`FROM ${src} TO ${tgt}`);
|
||||
}
|
||||
for (const tgt of backtickTargets) {
|
||||
expect(RELATION_SCHEMA).toContain(`FROM ${src} TO \`${tgt}\``);
|
||||
for (const src of [...sources, ...backtickSources]) {
|
||||
for (const tgt of [...targets, ...backtickTargets]) {
|
||||
expect(declaredPairs.has(`${src}|${tgt}`)).toBe(true);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
// Backtick source → all targets
|
||||
for (const src of backtickSources) {
|
||||
for (const tgt of targets) {
|
||||
expect(RELATION_SCHEMA).toContain(`FROM \`${src}\` TO ${tgt}`);
|
||||
}
|
||||
for (const tgt of backtickTargets) {
|
||||
expect(RELATION_SCHEMA).toContain(`FROM \`${src}\` TO \`${tgt}\``);
|
||||
}
|
||||
it('has FROM/TO pairs for HAS_METHOD edges whose member is minted as Function, not Method (#2769)', () => {
|
||||
// A Rust `impl`/`trait` method is minted as a `Function` node, not
|
||||
// `Method` (tree-sitter-queries.ts), so `Impl|Method`/`Trait|Method`
|
||||
// never fire for it — this was the reproduced abort on any Rust repo
|
||||
// with an `impl` or `trait` block (PR #2769 review Finding 1).
|
||||
const declaredPairs = parseRelationSchemaPairs(RELATION_SCHEMA);
|
||||
for (const pair of ['Trait|Function', 'Impl|Function']) {
|
||||
expect(declaredPairs.has(pair)).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it('has FROM/TO pairs for HAS_METHOD edges owned by a JS/TS object-literal binding (#2769)', () => {
|
||||
// A `const x = { m() {} }` / `var x = { m() {} }` owner is labelled
|
||||
// verbatim `Const`/`Variable` (ast-helpers.ts), and its shorthand
|
||||
// method is a `Method` node — this was the second reproduced abort,
|
||||
// hit on ordinary TS/JS code including GitNexus's own source tree
|
||||
// (PR #2769 review Finding 1).
|
||||
const declaredPairs = parseRelationSchemaPairs(RELATION_SCHEMA);
|
||||
for (const pair of ['Const|Method', 'Variable|Method']) {
|
||||
expect(declaredPairs.has(pair)).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user