fix(sync): resolve cross-file refs when an edit adds or removes the satisfying symbol (#1240) (#1249)

* chore: ignore .kommandr/ directory * fix(sync): resolve cross-file refs when an edit adds or removes the satisfying symbol (#1240) Incremental sync scoped reference resolution to the changed files' own refs, and a completed pass deleted every ref it failed to resolve — so a symbol change in one file could never repair references in UNCHANGED files, in either direction, until a full re-index: - New-export case: a.ts imports/calls `greet` before b.ts defines it. The failed refs were deleted at index time; when b.ts later gained `greet`, nothing revisited a.ts — the calls/imports edges stayed missing while status reported a clean index. - Removal case: when a re-index (or file deletion) dropped a symbol, the incoming edges cascade-deleted and the callers — whose resolved refs had been consumed — never got a chance to rebind to an alternative definition or reconnect when the symbol returned. Fix, sharing one lifecycle: - Schema v8: unresolved_refs gains status ('pending'/'failed') and name_tail (last dotted segment, so `h.greet` is findable by `greet`). Both resolver persist paths now park unresolvable refs as failed instead of deleting them. All pending-work readers (batched drain, non-progress guard, #1187 orphan sweep, status pendingRefs) filter to pending, preserving their invariants and keeping status honest. - Sync retry: after scoped resolution, failed refs whose name tail matches a symbol name now present in the changed files are re-resolved through a per-ref-yielding path (watchdog-safe, #1091 class). Names matching >500 failed refs are skipped as external/builtin noise (#999 rationale). - Removal side: createEdges stamps each resolution edge with its originating reference (metadata.refName, + refKind when kind promotion rewrote it). When the #899 restore misses a target or sync deletes a file, the dropped edge is resurrected as exactly that ref — re-resolved in the same sync (rebinding to an alternative definition) or parked failed until the symbol reappears. Edges without the stamp (pre-upgrade, synthesized) still drop silently: reconstructing from the target's plain name would strip receiver context and risk a rebind a full re-index would never make. - Pure-removal syncs clear resolver caches so a long-lived daemon can't resolve resurrected refs against the pre-removal graph. Validated: issue repro now yields a graph byte-identical to a full re-index; move/remove-readd/file-deletion scenarios all rebind or heal; baseline-vs-new A/B on express and gin shows identical node/edge counts and no timing regression (DB grows ~25% from the parked ref rows — pure cache, reset by any full re-index). 8 regression tests added. Fixes #1240 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-10 12:19:08 -05:00
parent 386bff0f84
commit 9d0cd3a7d1
12 changed files with 570 additions and 43 deletions
@@ -9,7 +9,7 @@ import { SqliteDatabase } from './sqlite-adapter';
 /**
 * Current schema version
 */
-export const CURRENT_SCHEMA_VERSION = 7;
+export const CURRENT_SCHEMA_VERSION = 8;

 /**
 * Migration definition
@@ -119,6 +119,37 @@ const migrations: Migration[] = [
      `);
    },
  },
+  {
+    version: 8,
+    description:
+      'Track attempted-but-unresolvable refs as status=failed so sync can retry them when a changed file adds a matching symbol (#1240)',
+    up: (db) => {
+      // DDL only — instant on any size database. No backfill needed: rows are
+      // only ever queried by name_tail once they carry status='failed', and
+      // both fields are written together by markReferencesFailed. Legacy rows
+      // (all 'pending' after this migration) are orphans from interrupted runs
+      // that the #1187 sweep grinds down on the next sync, marking survivors
+      // failed with their tails as it goes. The tail index is partial: on a
+      // healthy index the pending set is empty and the failed set is the only
+      // population worth indexing. Keep the definitions in lockstep with
+      // schema.sql. ALTER TABLE has no IF NOT EXISTS, so guard each column for
+      // idempotency — a database created from current schema.sql already has
+      // both (matters when migrations are re-run from an older recorded
+      // version, as the v6 regression test does).
+      const cols = db.prepare('PRAGMA table_info(unresolved_refs)').all() as Array<{ name: string }>;
+      const hasColumn = (name: string) => cols.some((c) => c.name === name);
+      if (!hasColumn('status')) {
+        db.exec("ALTER TABLE unresolved_refs ADD COLUMN status TEXT NOT NULL DEFAULT 'pending'");
+      }
+      if (!hasColumn('name_tail')) {
+        db.exec("ALTER TABLE unresolved_refs ADD COLUMN name_tail TEXT NOT NULL DEFAULT ''");
+      }
+      db.exec(`
+        CREATE INDEX IF NOT EXISTS idx_unresolved_status ON unresolved_refs(status);
+        CREATE INDEX IF NOT EXISTS idx_unresolved_failed_tail ON unresolved_refs(name_tail) WHERE status = 'failed';
+      `);
+    },
+  },
 ];

 /**
@@ -109,6 +109,20 @@ interface UnresolvedRefRow {
  candidates: string | null;
  file_path: string;
  language: string;
+  status: string;
+  name_tail: string;
+}
+
+/**
+ * Last segment of a (possibly dotted/qualified) reference name — the part a
+ * new symbol's plain node name could match: 'util.greet' → 'greet',
+ * 'mod::fn' → 'fn', 'greet' → 'greet'. Written to unresolved_refs.name_tail
+ * when a ref is marked failed, so the #1240 retry lookup can match dotted
+ * refs against newly-added node names.
+ */
+function referenceNameTail(referenceName: string): string {
+  const idx = Math.max(referenceName.lastIndexOf('.'), referenceName.lastIndexOf(':'));
+  return idx >= 0 ? referenceName.slice(idx + 1) : referenceName;
 }

 /**
@@ -1627,19 +1641,26 @@ export class QueryBuilder {
   * re-index (issue #899). Same edge-kind rules as
   * {@link getDependentFilePaths}: all kinds except `contains`.
   */
-  getCrossFileIncomingEdgesWithTarget(filePath: string): Array<Edge & { targetName: string; targetKind: NodeKind }> {
-    const sql = `SELECT e.*, tgt.name AS target_name, tgt.kind AS target_kind
+  getCrossFileIncomingEdgesWithTarget(
+    filePath: string
+  ): Array<Edge & { targetName: string; targetKind: NodeKind; sourceFilePath: string; sourceLanguage: Language }> {
+    const sql = `SELECT e.*, tgt.name AS target_name, tgt.kind AS target_kind,
+        src.file_path AS source_file_path, src.language AS source_language
      FROM edges e
      JOIN nodes tgt ON tgt.id = e.target
      JOIN nodes src ON src.id = e.source
      WHERE tgt.file_path = ?
        AND e.kind != 'contains'
        AND src.file_path != ?`;
-    const rows = this.db.prepare(sql).all(filePath, filePath) as Array<EdgeRow & { target_name: string; target_kind: NodeKind }>;
+    const rows = this.db.prepare(sql).all(filePath, filePath) as Array<
+      EdgeRow & { target_name: string; target_kind: NodeKind; source_file_path: string; source_language: Language }
+    >;
    return rows.map(row => ({
      ...rowToEdge(row),
      targetName: row.target_name,
      targetKind: row.target_kind,
+      sourceFilePath: row.source_file_path,
+      sourceLanguage: row.source_language,
    }));
  }

@@ -1827,12 +1848,16 @@ export class QueryBuilder {
  }

  /**
-   * Get the count of unresolved references without loading them into memory
+   * Get the count of PENDING (never-attempted) references without loading
+   * them into memory. Rows marked status='failed' — attempted by a completed
+   * pass, no match — are excluded: they are not outstanding work, only retry
+   * candidates for the #1240 sweep, so they must not trip the #1187 orphan
+   * sweep or the `status` pending-refs warning.
   */
  getUnresolvedReferencesCount(): number {
    if (!this.stmts.getUnresolvedCount) {
      this.stmts.getUnresolvedCount = this.db.prepare(
-        'SELECT COUNT(*) as count FROM unresolved_refs'
+        "SELECT COUNT(*) as count FROM unresolved_refs WHERE status = 'pending'"
      );
    }
    const row = this.stmts.getUnresolvedCount.get() as { count: number };
@@ -1840,13 +1865,15 @@ export class QueryBuilder {
  }

  /**
-   * Get a batch of unresolved references using LIMIT/OFFSET pagination.
-   * Used to process references in bounded memory chunks.
+   * Get a batch of PENDING unresolved references using LIMIT/OFFSET
+   * pagination. Used to process references in bounded memory chunks; failed
+   * rows are excluded so the batched drain loop terminates once every row
+   * has been attempted.
   */
  getUnresolvedReferencesBatch(offset: number, limit: number): UnresolvedReference[] {
    if (!this.stmts.getUnresolvedBatch) {
      this.stmts.getUnresolvedBatch = this.db.prepare(
-        'SELECT * FROM unresolved_refs LIMIT ? OFFSET ?'
+        "SELECT * FROM unresolved_refs WHERE status = 'pending' LIMIT ? OFFSET ?"
      );
    }
    const rows = this.stmts.getUnresolvedBatch.all(limit, offset) as UnresolvedRefRow[];
@@ -1913,7 +1940,7 @@ export class QueryBuilder {
      const chunk = filePaths.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
      const placeholders = chunk.map(() => '?').join(',');
      const chunkRows = this.db
-        .prepare(`SELECT * FROM unresolved_refs WHERE file_path IN (${placeholders})`)
+        .prepare(`SELECT * FROM unresolved_refs WHERE status = 'pending' AND file_path IN (${placeholders})`)
        .all(...chunk) as UnresolvedRefRow[];
      rows.push(...chunkRows);
    }
@@ -1971,6 +1998,97 @@ export class QueryBuilder {
    deleteMany(refs);
  }

+  /**
+   * Mark refs a completed resolution pass could not resolve as status='failed'
+   * instead of deleting them (#1240). Failed rows are invisible to the pending
+   * count/batch readers (so drain loops and the #1187 orphan sweep still
+   * terminate) but stay queryable by name_tail so a later sync can retry them
+   * when a changed file introduces a symbol that could satisfy them. name_tail
+   * is (re)written here so rows inserted before the v8 migration get their
+   * tail the first time they're attempted.
+   */
+  markReferencesFailed(refs: Array<{ fromNodeId: string; referenceName: string; referenceKind: string }>): void {
+    if (refs.length === 0) return;
+    const stmt = this.db.prepare(
+      "UPDATE unresolved_refs SET status = 'failed', name_tail = ? WHERE from_node_id = ? AND reference_name = ? AND reference_kind = ?"
+    );
+    const markMany = this.db.transaction((items: typeof refs) => {
+      for (const ref of items) {
+        stmt.run(referenceNameTail(ref.referenceName), ref.fromNodeId, ref.referenceName, ref.referenceKind);
+      }
+    });
+    markMany(refs);
+  }
+
+  /**
+   * Failed refs whose name tail matches one of the given symbol names — the
+   * candidates a sync should retry after files carrying those names changed
+   * (#1240). Names matching more than `perNameCeiling` failed refs are
+   * skipped entirely: at that population a name is external/builtin noise
+   * (`get`, `map`, …) that one new definition won't resolve — the same
+   * rationale as resolution's AMBIGUOUS_NAME_CEILING (#999) — and retrying an
+   * arbitrary subset would be both wasted work and incoherent coverage.
+   */
+  getRetryableFailedReferences(names: string[], perNameCeiling: number = 500): UnresolvedReference[] {
+    if (names.length === 0) return [];
+
+    // Pass 1: per-tail counts, chunked under the SQLite parameter limit.
+    const retryNames: string[] = [];
+    for (let i = 0; i < names.length; i += SQLITE_PARAM_CHUNK_SIZE) {
+      const chunk = names.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
+      const placeholders = chunk.map(() => '?').join(',');
+      const counts = this.db
+        .prepare(
+          `SELECT name_tail, COUNT(*) as count FROM unresolved_refs WHERE status = 'failed' AND name_tail IN (${placeholders}) GROUP BY name_tail`
+        )
+        .all(...chunk) as Array<{ name_tail: string; count: number }>;
+      for (const row of counts) {
+        if (row.count <= perNameCeiling) retryNames.push(row.name_tail);
+      }
+    }
+    if (retryNames.length === 0) return [];
+
+    // Pass 2: load the surviving rows.
+    const rows: UnresolvedRefRow[] = [];
+    for (let i = 0; i < retryNames.length; i += SQLITE_PARAM_CHUNK_SIZE) {
+      const chunk = retryNames.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
+      const placeholders = chunk.map(() => '?').join(',');
+      const chunkRows = this.db
+        .prepare(`SELECT * FROM unresolved_refs WHERE status = 'failed' AND name_tail IN (${placeholders})`)
+        .all(...chunk) as UnresolvedRefRow[];
+      rows.push(...chunkRows);
+    }
+
+    return rows.map((row) => ({
+      fromNodeId: row.from_node_id,
+      referenceName: row.reference_name,
+      referenceKind: row.reference_kind as EdgeKind,
+      line: row.line,
+      column: row.col,
+      candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined,
+      filePath: row.file_path,
+      language: row.language as Language,
+    }));
+  }
+
+  /**
+   * Distinct node names present in the given files — the symbol names a sync
+   * pass uses to look up retryable failed refs after those files changed.
+   */
+  getNodeNamesByFiles(filePaths: string[]): string[] {
+    if (filePaths.length === 0) return [];
+    const names = new Set<string>();
+    for (let i = 0; i < filePaths.length; i += SQLITE_PARAM_CHUNK_SIZE) {
+      const chunk = filePaths.slice(i, i + SQLITE_PARAM_CHUNK_SIZE);
+      const placeholders = chunk.map(() => '?').join(',');
+      const rows = this.db
+        .prepare(`SELECT DISTINCT name FROM nodes WHERE file_path IN (${placeholders})`)
+        .all(...chunk) as Array<{ name: string }>;
+      for (const row of rows) names.add(row.name);
+    }
+    return [...names];
+  }
+
  // ===========================================================================
  // Statistics
  // ===========================================================================
@@ -67,7 +67,15 @@ CREATE TABLE IF NOT EXISTS files (
    errors TEXT -- JSON array
 );

-- Unresolved References: References that need resolution after full indexing
+-- Unresolved References: References that need resolution after full indexing.
+-- status lifecycle: rows are inserted 'pending' by extraction; a completed
+-- resolution pass either deletes a row (resolved) or marks it 'failed'
+-- (attempted, no match — kept so a later sync can retry it when a changed
+-- file introduces a symbol that could satisfy it, #1240). name_tail is the
+-- last segment of reference_name ('util.greet' → 'greet'), written when a
+-- row is marked failed, so the retry lookup matches new node names against
+-- dotted refs too. Rows follow their from_node via ON DELETE CASCADE, so
+-- re-extracting or deleting a file clears its stale rows in any status.
 CREATE TABLE IF NOT EXISTS unresolved_refs (
    id INTEGER PRIMARY KEY AUTOINCREMENT,
    from_node_id TEXT NOT NULL,
@@ -78,6 +86,8 @@ CREATE TABLE IF NOT EXISTS unresolved_refs (
    candidates TEXT, -- JSON array
    file_path TEXT NOT NULL DEFAULT '',
    language TEXT NOT NULL DEFAULT 'unknown',
+    status TEXT NOT NULL DEFAULT 'pending',
+    name_tail TEXT NOT NULL DEFAULT '',
    FOREIGN KEY (from_node_id) REFERENCES nodes(id) ON DELETE CASCADE
 );

@@ -172,6 +182,8 @@ CREATE INDEX IF NOT EXISTS idx_unresolved_from_node ON unresolved_refs(from_node
 CREATE INDEX IF NOT EXISTS idx_unresolved_name ON unresolved_refs(reference_name);
 CREATE INDEX IF NOT EXISTS idx_unresolved_file_path ON unresolved_refs(file_path);
 CREATE INDEX IF NOT EXISTS idx_unresolved_from_name ON unresolved_refs(from_node_id, reference_name);
+CREATE INDEX IF NOT EXISTS idx_unresolved_status ON unresolved_refs(status);
+CREATE INDEX IF NOT EXISTS idx_unresolved_failed_tail ON unresolved_refs(name_tail) WHERE status = 'failed';
 CREATE INDEX IF NOT EXISTS idx_edges_provenance ON edges(provenance);

 -- Project metadata for version/provenance tracking