Skip to content

Commit 1dfaf30

Browse files
colbymchenryclaude
colbymchenry
and
claude
authored
fix(resolution): filter stale-target edges so watch sync survives FK violations (colbymchenry#455) (colbymchenry#463)
PR colbymchenry#62 plugged this FK violation at the extraction-layer insertEdges site (empty-named nodes whose containment edges had no target), but the same violation kept reappearing on v0.9.5 during the daemon's *watch sync* once an agent's daemon had been running long enough. The resolution-layer insertEdges (and the callback-synthesizer pass) wasn't guarded the same way: a per-resolver name cache or a framework resolver's WeakMap-keyed lookup map could hand back a Node whose row had been removed by a recent file rewrite, and the FK check then aborted the entire resolution batch, leaving the daemon log filling with `Watch sync failed { error: 'FOREIGN KEY constraint failed' }`. The resolution layer now mirrors the colbymchenry#62 defense — one cache-aware getNodesByIds per pass drops any edge whose source or target is no longer in the nodes table, so the rest of the resolved batch still lands. Regression test seeds the resolver's nameCache with a stale Node and calls resolveAndPersist directly; verified to throw FOREIGN KEY constraint failed without the fix and pass with it. Full suite: 984/984 pass. Closes colbymchenry#455. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
1 parent e76cc54 commit 1dfaf30

4 files changed

Lines changed: 276 additions & 6 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
1010
## [Unreleased]
1111

1212
### Fixed
13+
- **Watch sync no longer aborts with `FOREIGN KEY constraint failed`.** PR #62 plugged this FK violation at the extraction-layer `insertEdges` site (empty-named nodes whose containment edges had no target), but the same violation kept reappearing on v0.9.5 during the daemon's *watch sync* — not on initial index — once an agent's daemon had been running long enough to accumulate edits. The resolution-layer `insertEdges` (and the callback-synthesizer pass) wasn't guarded the same way: a per-resolver name cache or a framework resolver's WeakMap-keyed lookup map could hand back a `Node` whose row had been removed by a recent file rewrite, and the FK check then aborted the entire resolution batch, leaving the user's daemon log filling with `Watch sync failed { error: 'FOREIGN KEY constraint failed' }`. The resolution layer now applies the same defense-in-depth filter the extraction layer already had — one cache-aware `getNodesByIds` per pass drops any edge whose source or target is no longer in the `nodes` table, so the rest of the resolved batch still lands. Surfaces as a fresh `codegraph init`/`index` cycle now surviving its first watch-sync cycle without the FK error, and the daemon recovering naturally instead of compounding into further failures. Closes #455.
1314
- **Hermes Agent: `codegraph install --target hermes` no longer corrupts `~/.hermes/config.yaml`.** Hermes serializes its config with PyYAML's default block style, which writes list items at the *same* indent as the parent mapping key (`cli:` and `- hermes-cli` both at column 2). The previous line-based YAML patcher mistook that first ` - hermes-cli` for the next sibling key, truncated the `cli:` block, and then spliced `- mcp-codegraph` at indent 4 *before* the existing items — leaving subsequent entries (`- browser`, `- clarify`, …) and even other platforms (`telegram:`, `discord:`) appearing at the `platform_toolsets:` level, which is no longer parseable YAML. The installer now recognizes the same-indent list style, finds the real end of the block at the next sibling key, and appends `- mcp-codegraph` at whatever indent the existing items already use. Re-installing on an already-corrupted file (or a 4-space-nested config that worked before) still produces a clean, parseable result. Closes #456.
1415
- **NestJS: `RouterModule.register([...])` route prefixes now propagate to controller routes.** Previously a controller declared inside a module wired through NestJS's `RouterModule` (a common pattern for modular apps with nested route prefixes) was indexed with its raw `@Controller(...) + @Get(...)` path — so `UsersController` under `RouterModule.register([{ path: 'admin', module: AdminModule, children: [{ path: 'users', module: UsersModule }] }])` showed up as `GET /` instead of `GET /admin/users`. The new cross-file pass walks every `*.module.{ts,js}` for `RouterModule.register/forRoot/forChild([...])` (recursive `children`) and `@Module({ controllers: [...] })`, then prepends the correct prefix to each affected route — including non-empty `@Controller` paths and method-level params (`/admin/users/:id`). The route node's `id` is preserved across the update so existing route→handler edges stay intact, and the pass is idempotent so incremental sync recovers when `app.module.ts` itself is edited. Closes #459.
1516

‎__tests__/sync-fk-regression.test.ts‎

Lines changed: 232 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,232 @@
1+
/**
2+
* Sync FK regression test (issue #455).
3+
*
4+
* #62 plugged FK violations at the extraction layer (empty-named nodes whose
5+
* containment edges had no target). #455 reports the same `FOREIGN KEY constraint
6+
* failed` reappearing on v0.9.5, but during *watch sync* on a Python-only project —
7+
* a different trigger than the C/C++ header empty-name issue #62 covered.
8+
*
9+
* The reproducer below drives the same path the daemon takes: extract → resolve →
10+
* insert edges. The resolution pass's `insertEdges` was not guarded the way the
11+
* extraction-layer insert was after #62, so any edge with a stale source/target
12+
* (e.g. a synthesized framework target whose node was deleted by a concurrent
13+
* file rewrite) throws and aborts the sync, leaving the FK error the user sees.
14+
*
15+
* The test asserts: a sequence of file rewrites + sync()s never throws, and the
16+
* graph stays internally consistent (every edge's source + target are real nodes).
17+
*/
18+
import { describe, it, expect, beforeAll, afterEach } from 'vitest';
19+
import * as fs from 'fs';
20+
import * as path from 'path';
21+
import * as os from 'os';
22+
import CodeGraph from '../src/index';
23+
import { initGrammars, loadAllGrammars } from '../src/extraction/grammars';
24+
25+
beforeAll(async () => {
26+
await initGrammars();
27+
await loadAllGrammars();
28+
});
29+
30+
describe('watch sync FK regression (#455)', () => {
31+
let tmpDir: string | undefined;
32+
let cg: CodeGraph | undefined;
33+
34+
afterEach(() => {
35+
if (cg) {
36+
cg.close();
37+
cg = undefined;
38+
}
39+
if (tmpDir) {
40+
fs.rmSync(tmpDir, { recursive: true, force: true });
41+
tmpDir = undefined;
42+
}
43+
});
44+
45+
function assertGraphIntegrity(cg: CodeGraph): void {
46+
// Every edge must reference real nodes. If FK was disabled or violated,
47+
// dangling refs would show up here.
48+
const db = (cg as unknown as { db: { getDb(): { prepare(sql: string): { get(): unknown } } } }).db;
49+
const sqlite = db.getDb();
50+
const dangling = sqlite
51+
.prepare(
52+
`SELECT count(*) as c FROM edges e
53+
WHERE NOT EXISTS (SELECT 1 FROM nodes n WHERE n.id = e.source)
54+
OR NOT EXISTS (SELECT 1 FROM nodes n WHERE n.id = e.target)`
55+
)
56+
.get() as { c: number };
57+
expect(dangling.c).toBe(0);
58+
}
59+
60+
it('survives repeated sync() cycles on a Django-style Python project without FK errors', async () => {
61+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-fk455-'));
62+
63+
// Mimic a small Django app: requirements + manage.py marker, models/views/urls
64+
// in two app packages that cross-reference each other.
65+
fs.writeFileSync(path.join(tmpDir, 'manage.py'), '# django marker\n');
66+
fs.writeFileSync(path.join(tmpDir, 'requirements.txt'), 'django==4.2\n');
67+
68+
fs.mkdirSync(path.join(tmpDir, 'users'));
69+
fs.writeFileSync(path.join(tmpDir, 'users/__init__.py'), '');
70+
fs.writeFileSync(
71+
path.join(tmpDir, 'users/models.py'),
72+
'class User:\n' +
73+
' def __init__(self, name):\n' +
74+
' self.name = name\n'
75+
);
76+
fs.writeFileSync(
77+
path.join(tmpDir, 'users/views.py'),
78+
'from users.models import User\n' +
79+
'class UserListView:\n' +
80+
' def get(self, request):\n' +
81+
' return User("a")\n'
82+
);
83+
fs.writeFileSync(
84+
path.join(tmpDir, 'users/urls.py'),
85+
'from django.urls import path\n' +
86+
'from users.views import UserListView\n' +
87+
'urlpatterns = [path("users/", UserListView.as_view(), name="user-list")]\n'
88+
);
89+
90+
fs.mkdirSync(path.join(tmpDir, 'posts'));
91+
fs.writeFileSync(path.join(tmpDir, 'posts/__init__.py'), '');
92+
fs.writeFileSync(
93+
path.join(tmpDir, 'posts/models.py'),
94+
'from users.models import User\n' +
95+
'class Post:\n' +
96+
' def __init__(self, author):\n' +
97+
' self.author = author\n'
98+
);
99+
fs.writeFileSync(
100+
path.join(tmpDir, 'posts/views.py'),
101+
'from posts.models import Post\n' +
102+
'class PostListView:\n' +
103+
' def get(self, request):\n' +
104+
' return Post(None)\n'
105+
);
106+
fs.writeFileSync(
107+
path.join(tmpDir, 'posts/urls.py'),
108+
'from django.urls import path\n' +
109+
'from posts.views import PostListView\n' +
110+
'urlpatterns = [path("posts/", PostListView.as_view(), name="post-list")]\n'
111+
);
112+
113+
cg = CodeGraph.initSync(tmpDir);
114+
await cg.indexAll();
115+
assertGraphIntegrity(cg);
116+
117+
// Drive the same path the daemon's file watcher drives: a series of file
118+
// rewrites + sync()s. We shuffle line counts on each rewrite so node IDs
119+
// (file:kind:name:line) shift around, forcing real INSERT OR REPLACE +
120+
// CASCADE behavior across files that cross-reference each other.
121+
const targets = [
122+
'users/views.py',
123+
'posts/views.py',
124+
'users/urls.py',
125+
'posts/urls.py',
126+
'users/models.py',
127+
];
128+
129+
for (let iter = 0; iter < 8; iter++) {
130+
const file = targets[iter % targets.length]!;
131+
const full = path.join(tmpDir, file);
132+
const content = fs.readFileSync(full, 'utf8');
133+
// Insert N blank lines at the top to shift every node's line number.
134+
const padded = '\n'.repeat(iter + 1) + content;
135+
// Use a future mtime so the size+mtime pre-filter in
136+
// ExtractionOrchestrator.sync can't skip the file.
137+
fs.writeFileSync(full, padded);
138+
const now = Date.now() + (iter + 1) * 1_000;
139+
fs.utimesSync(full, now / 1000, now / 1000);
140+
141+
// The fix should make this never throw; before the fix, FK errors fire
142+
// during the resolution-layer insertEdges call inside sync().
143+
await expect(cg.sync()).resolves.toBeDefined();
144+
assertGraphIntegrity(cg);
145+
}
146+
});
147+
148+
it("drops resolution edges whose target node is no longer in the graph (the pathology #455 reports)", async () => {
149+
// This narrower test reproduces the exact failure mode the user sees in
150+
// their daemon log: the resolver hands `insertEdges` an edge whose target
151+
// doesn't exist in `nodes`, and the FK constraint aborts the whole sync.
152+
//
153+
// We force the bug by populating the resolver's per-name cache with a
154+
// stale node (whose id is *not* in the DB) and then asking it to resolve
155+
// a reference to that name. Without the fix this throws
156+
// `FOREIGN KEY constraint failed`; with it, the bad edge is filtered out
157+
// and resolution returns normally.
158+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-fk455-stale-'));
159+
fs.writeFileSync(path.join(tmpDir, 'a.py'), 'def caller():\n Target()\n');
160+
fs.writeFileSync(path.join(tmpDir, 'b.py'), 'class Target:\n pass\n');
161+
162+
cg = CodeGraph.initSync(tmpDir);
163+
await cg.indexAll();
164+
165+
// Reach in to the internals — the simplest way to forge the "stale node
166+
// ID in the resolver's lookup path" condition the production bug arises
167+
// from. The fix is what the test is verifying; touching internals here
168+
// is a means to that end, not a contract we're asserting.
169+
type Internals = {
170+
queries: {
171+
getNodesByName(name: string): Array<{ id: string; name: string }>;
172+
getAllNodeNames(): string[];
173+
};
174+
resolver: {
175+
warmCaches(): void;
176+
resolveAndPersist(
177+
refs: Array<{
178+
fromNodeId: string;
179+
referenceName: string;
180+
referenceKind: string;
181+
line: number;
182+
column: number;
183+
filePath: string;
184+
language: string;
185+
}>
186+
): { resolved: unknown[] };
187+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
188+
nameCache: { set(key: string, value: any): void };
189+
};
190+
};
191+
const internals = cg as unknown as Internals;
192+
const queries = internals.queries;
193+
const resolver = internals.resolver;
194+
195+
const caller = queries.getNodesByName('caller')[0];
196+
const target = queries.getNodesByName('Target')[0];
197+
expect(caller).toBeDefined();
198+
expect(target).toBeDefined();
199+
200+
// Warm caches so warmCaches no-ops on the resolveAndPersist call below
201+
// and our seeded nameCache entry isn't overwritten.
202+
resolver.warmCaches();
203+
204+
// Forge a stale lookup result: a Node whose `id` doesn't exist in the
205+
// `nodes` table. This is structurally what happens when a framework
206+
// resolver's WeakMap cache hands back a Node that was deleted by a
207+
// concurrent file rewrite — the user's #455 scenario.
208+
const staleNode = { ...target!, id: 'class:stale.py:Target:1' };
209+
resolver.nameCache.set('Target', [staleNode]);
210+
211+
// Ask the resolver to persist an edge that will resolve via the seeded
212+
// (stale) cache entry. Without the FK filter this would throw
213+
// `FOREIGN KEY constraint failed` and abort the whole batch.
214+
expect(() =>
215+
resolver.resolveAndPersist([
216+
{
217+
fromNodeId: caller!.id,
218+
referenceName: 'Target',
219+
referenceKind: 'calls',
220+
line: 2,
221+
column: 4,
222+
filePath: 'a.py',
223+
language: 'python',
224+
},
225+
])
226+
).not.toThrow();
227+
228+
// The bad edge must not have been persisted either — FK enforcement is
229+
// still on, and post-fix the dangling-edge count remains zero.
230+
assertGraphIntegrity(cg);
231+
});
232+
});

‎src/resolution/callback-synthesizer.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -812,6 +812,19 @@ export function synthesizeCallbackEdges(queries: QueryBuilder, ctx: ResolutionCo
812812
seen.add(key);
813813
merged.push(e);
814814
}
815-
if (merged.length > 0) queries.insertEdges(merged);
816-
return merged.length;
815+
if (merged.length > 0) {
816+
// Defense-in-depth (issues #42, #455): drop edges whose source/target
817+
// no longer resolves to a real node. Some channel maps cache native
818+
// Node refs across a resolver lifetime (WeakMap-keyed by context), so
819+
// a file rewrite between map build and synthesis can leave stale IDs
820+
// here. One FK violation aborts the whole batch — better to skip the
821+
// dead edges and emit the rest than lose every synthesized edge.
822+
const allIds = new Set<string>();
823+
for (const e of merged) { allIds.add(e.source); allIds.add(e.target); }
824+
const existing = queries.getNodesByIds([...allIds]);
825+
const validEdges = merged.filter((e) => existing.has(e.source) && existing.has(e.target));
826+
if (validEdges.length > 0) queries.insertEdges(validEdges);
827+
return validEdges.length;
828+
}
829+
return 0;
817830
}

‎src/resolution/index.ts‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,28 @@ export class ReferenceResolver {
607607
});
608608
}
609609

610+
/**
611+
* Defense-in-depth: drop edges whose source or target is no longer in
612+
* the nodes table. PR #62 (issue #42) applied this filter at the
613+
* extraction-layer `insertEdges` site; #455 reports the same
614+
* `FOREIGN KEY constraint failed` reappearing here at the
615+
* resolution-layer site during watch sync, where a resolver lookup that
616+
* crosses a framework-specific cache can hand us a target whose node
617+
* was removed by a concurrent file rewrite. One batched, cache-aware
618+
* `getNodesByIds` query is enough to skip those edges quietly instead
619+
* of aborting the whole sync.
620+
*/
621+
private filterEdgesByExistingNodes(edges: Edge[]): Edge[] {
622+
if (edges.length === 0) return edges;
623+
const allIds = new Set<string>();
624+
for (const e of edges) {
625+
allIds.add(e.source);
626+
allIds.add(e.target);
627+
}
628+
const existing = this.queries.getNodesByIds([...allIds]);
629+
return edges.filter((e) => existing.has(e.source) && existing.has(e.target));
630+
}
631+
610632
/**
611633
* Resolve and persist edges to database
612634
*/
@@ -620,8 +642,9 @@ export class ReferenceResolver {
620642
const edges = this.createEdges(result.resolved);
621643

622644
// Insert edges into database
623-
if (edges.length > 0) {
624-
this.queries.insertEdges(edges);
645+
const validEdges = this.filterEdgesByExistingNodes(edges);
646+
if (validEdges.length > 0) {
647+
this.queries.insertEdges(validEdges);
625648
}
626649

627650
// Clean up resolved refs from unresolved_refs table so metrics are accurate
@@ -668,8 +691,9 @@ export class ReferenceResolver {
668691

669692
// Persist edges immediately
670693
const edges = this.createEdges(result.resolved);
671-
if (edges.length > 0) {
672-
this.queries.insertEdges(edges);
694+
const validEdges = this.filterEdgesByExistingNodes(edges);
695+
if (validEdges.length > 0) {
696+
this.queries.insertEdges(validEdges);
673697
}
674698

675699
// Clean up resolved refs so they don't appear in the next batch

0 commit comments

Comments
 (0)