feat(PF-693): codegraph explain single-edge resolution trace#41
Closed
mbenhamd wants to merge 2 commits into
Closed
feat(PF-693): codegraph explain single-edge resolution trace#41mbenhamd wants to merge 2 commits into
mbenhamd wants to merge 2 commits into
Conversation
3a79047 to
39f02de
Compare
mbenhamd
added a commit
that referenced
this pull request
May 24, 2026
…, rawMetadata Council double-Codex review (passes A + B) flagged real gaps. Addressed both BLOCKERs + top REVIEWs: ## BLOCKER fixes - **Round-trip claim was broken**: PR #41 advertised "canonical fallback via `--source/--target/--kind`" when `edgeId` is stale, but `callers/callees --json` exposed neighbour node `kind` only and OMITTED `source`, `target`, `line`, `col`. Users had no way to reconstruct the canonical identity from JSON output. Fix: add an `edge: { kind, source, target, line, col }` object to each `GraphRelationEntry`. Updates `callers.json` + `callees.json` schemas to document the new field. - **`explain` overpromised**: the tool name implies a causal explanation but the resolver in `src/resolution/` discards loser strategy attempts. Only the winner's tag survives. Fix: add `traceAvailable: boolean` to `ExplainResult` (always `false` today). Narrative text mode prints "trace: breadcrumbs only (resolver loser strategies not persisted; see traceAvailable=false)". Honest signal; a future PR will persist `edge_resolution_traces` and flip this to `true`. ## REVIEW fixes - **`parseMetadata` silently dropped arrays/scalars**: legacy or unexpected metadata shapes returned `null`, indistinguishable from "no metadata". Fix: refactored to return `{ metadata, rawMetadata }` — `metadata` is the object form when present, `rawMetadata` is the raw JSON string when content was an array/scalar/malformed. - **Mixed positional+canonical mode silently picked one**: a user passing both `<edgeId>` and `--source/--target` would have the flags ignored without warning. Fix: detect mixed-mode up front and error with "Provide either a positional <edgeId> or canonical flags, not both." - **Ambiguity test skipped on precondition fail**: relied on the resolver emitting ≥2 calls edges for the same target. Rewritten with a hand-built fixture (two edges, same source/target/kind, different line) that guarantees the ambiguity case fires AND verifies `--line N` disambiguates. ## Schema - `explain.json`: `rawMetadata: string | null` and `traceAvailable: boolean` are required. - `callers.json` / `callees.json`: optional `edge` object documenting the canonical identity round-trip surface. ## Tests added (+3, +6 total now 12) - `traceAvailable: false` is locked at false until resolver trace table lands. - Non-object metadata (JSON array) → `metadata: null`, `rawMetadata` is the raw string. - Ambiguity test uses hand-built fixture (no more skip-on-precondition). - `cli-json-schemas`: callers schema test asserts presence of `edge: { kind, source, target }` on real outputs. ## Validation - `tsc --noEmit`: clean - `vitest run`: 1076 passed | 2 skipped | 62 files (PR #41 round 1 baseline was 1065 → +11 from round 2 tests) - `npm run build`: clean (Node 24) - `npm run test:eval:structural`: required=5/5, all-pass=8/8, recall=1.00, precision=1.00, fp=0 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
4d4605f to
5d82d64
Compare
39f02de to
2a713c1
Compare
Third consumer of the CLI envelope/cli-tools family, completing the trio with diff (PR #39) and duplicates (PR #40). Surfaces every persisted provenance breadcrumb for a single edge: - Edge identity (source, target, kind, line, col) - Source + target node snapshots - Extractor provenance: `edges.provenance` column (tree-sitter / scip / heuristic) - Resolver provenance: `metadata.resolvedBy` strategy tag (import / framework / qualified-name / exact-match / instance-method / file-path / fuzzy) + `metadata.confidence` - Full raw metadata JSON for forward-compat Stacked on PR #40 (which stacks on PR #39). ## Surface ``` codegraph explain <edgeId> # happy path codegraph explain --source <id> --target <id> \ # rebuild-stable canonical --kind <k> [--line N] [--col N] codegraph explain <edgeId> --json # full envelope ``` Default text output uses `formatExplainNarrative` for a concise multi-line trace; `--json` emits the full payload via `cliJsonEnvelope('explain', …)`. ## Council RFC outcome (Codex) | Fork | Decision | |---|---| | A: identifier surface | **Both** — positional `<edgeId>` + canonical flags | | B: expose `edgeId` in callers/callees JSON | **Yes** — small `rowToEdge` patch plumbs it through | | C: output shape | **JSON + text narrative** (terminal use case dominates) | | D: `--rerun` | **Skip** — re-resolution is a different diagnostic mode | | E: ambiguous canonical lookup | **Error with `--line N --col N` hint** | ## Edge.id exposure `Edge` gains an optional `id?: number` field with a docstring noting it's index-local (resets on rebuild). `rowToEdge` propagates it from the DB row. `collectGraphRelations` includes `edgeId` in the caller/callee JSON entries when present. `schemas/cli/callers.json` and `schemas/cli/callees.json` document the new optional field. ## Read-only safety `pathToFileURL(dbPath).href + '?immutable=1'` — same pattern as PRs #39 and #40. No `-shm`/`-wal` sidecar creation; the DB stays bit-for-bit unchanged. Regression test snapshots size + mtime of `db`/`-wal`/`-shm`/`-journal` before/after and asserts equal. ## Schema gate No v6 requirement — `provenance`/`metadata` columns existed in v5. `assertCodeGraphDb` rejects DBs missing BOTH `edges` AND `nodes` tables via a single `WHERE name IN (…)` probe. Missing resolver metadata is tolerated: `resolvedBy: null`, `confidence: null`, `metadata: null` — explain explains what's there. ## Tests (10 unit + 1 schema validation) - Integer-id lookup surfaces resolver provenance - Canonical lookup by (source, target, kind) - Ambiguous canonical → throws with `--line` hint - No-match canonical → throws with full identifier in message - Missing edge id → throws "no edge with id N" - Non-positive / non-integer edge id → rejected before DB open - Narrative format contains expected keywords - Non-CodeGraph SQLite file → "not a CodeGraph database" - Read-only invariant: DB + sidecars unchanged after explain - Missing DB path → "not found" ## Reviewer trail - Codex RFC → locked 5 design forks - Codex pass 1 → 3 REVIEW + 5 NITPICK (no BLOCKER) - `--kind` should be required for canonical lookup - `assertCodeGraphDb` should check both tables - `confidence` should be clamped to [0, 1] - Document `edgeId` in callers/callees schemas - Codex round 2 → "closed — proceed to PR" ## Validation - `tsc --noEmit`: clean - `vitest run`: 1065 passed | 2 skipped | 62 files - `npm run build`: clean (Node 24) - `npm run test:eval:structural`: required=5/5, all-pass=8/8, blocking=0, recall=1.00, precision=1.00, fp=0 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…, rawMetadata Council double-Codex review (passes A + B) flagged real gaps. Addressed both BLOCKERs + top REVIEWs: ## BLOCKER fixes - **Round-trip claim was broken**: PR #41 advertised "canonical fallback via `--source/--target/--kind`" when `edgeId` is stale, but `callers/callees --json` exposed neighbour node `kind` only and OMITTED `source`, `target`, `line`, `col`. Users had no way to reconstruct the canonical identity from JSON output. Fix: add an `edge: { kind, source, target, line, col }` object to each `GraphRelationEntry`. Updates `callers.json` + `callees.json` schemas to document the new field. - **`explain` overpromised**: the tool name implies a causal explanation but the resolver in `src/resolution/` discards loser strategy attempts. Only the winner's tag survives. Fix: add `traceAvailable: boolean` to `ExplainResult` (always `false` today). Narrative text mode prints "trace: breadcrumbs only (resolver loser strategies not persisted; see traceAvailable=false)". Honest signal; a future PR will persist `edge_resolution_traces` and flip this to `true`. ## REVIEW fixes - **`parseMetadata` silently dropped arrays/scalars**: legacy or unexpected metadata shapes returned `null`, indistinguishable from "no metadata". Fix: refactored to return `{ metadata, rawMetadata }` — `metadata` is the object form when present, `rawMetadata` is the raw JSON string when content was an array/scalar/malformed. - **Mixed positional+canonical mode silently picked one**: a user passing both `<edgeId>` and `--source/--target` would have the flags ignored without warning. Fix: detect mixed-mode up front and error with "Provide either a positional <edgeId> or canonical flags, not both." - **Ambiguity test skipped on precondition fail**: relied on the resolver emitting ≥2 calls edges for the same target. Rewritten with a hand-built fixture (two edges, same source/target/kind, different line) that guarantees the ambiguity case fires AND verifies `--line N` disambiguates. ## Schema - `explain.json`: `rawMetadata: string | null` and `traceAvailable: boolean` are required. - `callers.json` / `callees.json`: optional `edge` object documenting the canonical identity round-trip surface. ## Tests added (+3, +6 total now 12) - `traceAvailable: false` is locked at false until resolver trace table lands. - Non-object metadata (JSON array) → `metadata: null`, `rawMetadata` is the raw string. - Ambiguity test uses hand-built fixture (no more skip-on-precondition). - `cli-json-schemas`: callers schema test asserts presence of `edge: { kind, source, target }` on real outputs. ## Validation - `tsc --noEmit`: clean - `vitest run`: 1076 passed | 2 skipped | 62 files (PR #41 round 1 baseline was 1065 → +11 from round 2 tests) - `npm run build`: clean (Node 24) - `npm run test:eval:structural`: required=5/5, all-pass=8/8, recall=1.00, precision=1.00, fp=0 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5d82d64 to
0884d45
Compare
mbenhamd
added a commit
that referenced
this pull request
May 24, 2026
…itive (#39) * feat(PF-691): `codegraph diff <oldDb> <newDb>` DB-vs-DB structural primitive First consumer of the PF-690 fingerprint columns (PR #38). Compares two `.codegraph/codegraph.db` files and reports added/removed/changed nodes and edges, with `changedFields` enumerating which tracked column drifted on each change (astHash → body change, sigHash/signature → contract change, qualifiedName → in-file move, etc.). ## Design (council RFC) CodeGraph stays VCS-agnostic — the diff operates on already-built DB files, the caller handles git checkouts. Calling `git stash` / `git checkout` from the indexer would be destructive (agy: "massive anti-pattern"). Both council members (Codex + agy) converged on DB-vs-DB; this is that primitive. - Nodes matched by stable `id` (deterministic from filePath + kind + name + line via `generateNodeId`). Renames and file moves surface as removed-old + added-new pairs. - Edges matched by canonical identity `(source, target, kind, line, col)` per the PF-625 UNIQUE INDEX, with NULL line/col folding to -1. ## Read-only safety (Codex round 1 BLOCKER, fixed in round 2) `diffDatabases` opens both DBs via `new DatabaseSync(\`file:${escaped}?immutable=1\`, { readOnly: true })`, not the production `DatabaseConnection.open` path. The latter sets `journal_mode = WAL` and runs forward migrations — both mutate the file on disk, which is unacceptable for a tool whose purpose is comparing historical snapshots. `immutable=1` tells SQLite the file will never change, skipping all locking and -shm/-wal sidecar creation. The regression test `does not mutate either DB or create WAL sidecars` snapshots size + mtime of `db`, `db-wal`, `db-shm`, `db-journal` before diff and asserts they all match after. ## v5 schema fallback `loadNodes` probes `PRAGMA table_info('nodes')` and projects missing fingerprint columns as `NULL AS <col>`, so diffing a pre-PR-#38 v5 backup against a v6 DB works cleanly — fingerprint fields just don't appear in `changedFields` for legacy rows. Regression test: `handles legacy schema (no fingerprint columns) without failing`. ## Surface - `codegraph diff <oldDb> <newDb> [-j|--json]` CLI subcommand - `diffDatabases(oldPath, newPath): DiffResult` library export from `src/diff.ts` - `schemas/cli/diff.json` JSON Schema (envelope.tool enum widened to include diff/duplicates/explain for PRs #39-#41) - 9 unit tests covering identical/added/removed/body-change/signature- change/summary-counts/missing-DB/read-only-no-mutation/v5-fallback ## Validation - `npx tsc --noEmit`: clean - `npx vitest run`: 1040 passed | 2 skipped | 60 files - `npm run build`: clean (Node 24) - `npm run test:eval:structural`: required=5/5, all-pass=8/8, blocking=0, recall=1.00, precision=1.00, fp=0 ## Reviewer trail - Codex pass 1 → BLOCKER on read-only safety - Round 2 closure → "closed — proceed to PR" Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(PF-691): PR #39 round 3 — file-level diff, edge metadata, fingerprint coverage Council double-Codex review (passes A + B) flagged real gaps in PR #39's scope. Addressed BLOCKERs + top REVIEWs: ## BLOCKER fixes - **File-level diff missing**: a file added/removed with zero indexed symbols silently produced empty output. Added `addedFiles`, `removedFiles`, `changedFiles` arrays via the `files` table. changedFiles surfaces `contentHash`, `size`, `nodeCount` drift. - **Non-tree-sitter NULL fingerprints**: Liquid/Vue/Svelte/YAML extractors emit nodes with NULL `ast_hash`, so body-level changes in those file types silently report "unchanged". Added `fingerprintCoverage: { old, new }` counter so consumers can detect the gap. CLI prints a yellow warning when coverage is partial. - **Line-sensitive node IDs**: documented in source as a known pre-existing CodeGraph design decision (not introduced by this PR); downstream consumers wanting move-stable matching should diff by (filePath, qualifiedName) after consuming this output. ## REVIEW fixes - **Edge metadata never compared**: edges with the same canonical identity but different `metadata`/`provenance` were silently matched as "unchanged", even though `codegraph explain` surfaces exactly those fields. Added `changedEdges: EdgeChange[]` with `changedFields` listing which of metadata/provenance differs. - **`-1` sentinel leaked into JSON output**: internal Map keys still use -1 as the NULL bucket (matches PF-625 UNIQUE INDEX), but `edgeIdentityForOutput` now preserves `null` in the public `EdgeIdentity` shape. - **Read-only validation deferred**: `DatabaseSync` opens lazily, so a non-SQLite `.db` file would error at first query with a raw SQLITE_NOTADB message. `openReadOnly` now runs `SELECT 1` to force validation, then wraps as `Invalid CodeGraph database at <path>`. - **`codegraph diff` missing from CLI top-of-file usage block**. Added. - **Text output truncation silent**: now prints "+N more (use --json for full output)" footer. ## Schema + tests - `schemas/cli/diff.json` widened: requires the 4 new fields, declares `nullable line/col` correctly, includes `fingerprintCoverage` and `fileChange`/`edgeChange` definitions. - 5 new tests: null-instead-of-`-1`, file-level add/remove/change, edge metadata drift via hand-built fixture, corrupt-DB wrapper, expanded summary-count invariant. ## Validation - `tsc --noEmit`: clean - `vitest run`: 1045 passed | 2 skipped | 60 files - `npm run build`: clean (Node 24) - `npm run test:eval:structural`: required=5/5, all-pass=8/8, recall=1.00, precision=1.00, fp=0 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(PF-691): close oldDb if openReadOnly(newDb) throws Codex PR review P2: putting both `openReadOnly` calls before the `try` block leaked the old DB's SQLite handle when the second open failed (unreadable path, corrupt file, etc.). Wrap newDb open in a nested try/catch that closes oldDb before re-throwing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 24, 2026
mbenhamd
added a commit
that referenced
this pull request
May 24, 2026
Third consumer of PF-690 fingerprint columns + first consumer of edge resolver provenance. Replaces auto-closed PR #41 (closed when its base branch was deleted on PR #39/#42 merge); same content rebased onto main. ## Surface ``` codegraph explain <edgeId> # happy path codegraph explain --source <id> --target <id> --kind <k> # canonical fallback [--line N] [--col N] # disambiguators codegraph explain <edgeId> --json ``` ## Honest scope `traceAvailable: false` is locked at false in the output — the resolver in `src/resolution/` discards loser strategy attempts, so only the winning resolver tag + confidence survive. The tool surfaces persisted breadcrumbs, NOT a causal explanation. A future PR will persist `edge_resolution_traces` and flip this to `true`. ## Round-trip stability `callers --json` / `callees --json` now expose `edge: { kind, source, target, line, col }` per relation, so users can pipe output into `codegraph explain --source/--target/--kind` even across rebuilds (when edgeId is invalidated). ## Read-only safety Same `pathToFileURL + '?immutable=1'` URI pattern as PRs #39/#42. ## Schema gate No v6 requirement — provenance/metadata columns existed in v5. `assertCodeGraphDb` checks both `edges` AND `nodes` tables in a single query; rejects with `not a CodeGraph database` if either is missing. ## Output fields - Edge identity (source, target, kind, line, col) - Source + target node snapshots (qualifiedName, filePath, startLine) - `extractorProvenance` (tree-sitter / scip / heuristic) - `resolvedBy` strategy tag + `confidence` ∈ [0, 1] - `metadata` (object form) + `rawMetadata` (string form for arrays/scalars/malformed — PR review fix so non-object metadata isn't silently dropped) - `traceAvailable` (always `false` today; honest signal) ## Edge.id exposure `Edge` gains optional `id?: number` field (index-local, resets on rebuild). `rowToEdge` propagates it. `collectGraphRelations` adds `edgeId` + `edge` object to JSON output. ## Tests 12 unit tests + 1 schema validation test covering: integer-id lookup with provenance assertions, canonical lookup, ambiguous canonical with `--line` disambiguation (hand-built fixture, no skip-on-precondition), no-match, missing-id, invalid-id rejection, narrative format, non-CodeGraph DB rejection, read-only invariant, missing DB, `traceAvailable: false` locked, non-object metadata exposed via `rawMetadata`. ## Reviewer trail - Codex RFC → locked 5 design forks (positional+canonical identifier, edge.id exposure, JSON+text, skip --rerun, ambiguity error) - Codex pass 1 + round 2 closure (kind required, dbCheck both tables, confidence clamping, edgeId schema docs) - Council double-Codex deep review → 2 BLOCKERs addressed (canonical round-trip needed edge object in callers JSON; traceAvailable honesty) - Codex PR review on round 2 → no findings ## Validation - `tsc --noEmit`: clean - `vitest run`: 1078 passed | 2 skipped | 62 files - `npm run build`: clean (Node 24) - `npm run test:eval:structural`: required=5/5, all-pass=8/8, recall=1.00, precision=1.00, fp=0 Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
Closing as stale. Content already merged into main via PR #43 ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third consumer of the CLI envelope/cli-tools family, completing the trio with diff (PR #39) and duplicates (PR #40). Surfaces every persisted provenance breadcrumb for a single edge so users can answer "why does CodeGraph think this call resolves here?"
Stacked on
feature/pf-692-codegraph-duplicates(PR #40). Rebases to main after PRs #39 and #40 merge.Output
Default terminal format is a concise multi-line narrative:
--jsonemits the full payload throughcliJsonEnvelope('explain', …).Council RFC outcome (Codex)
<edgeId>+ canonical flags. Index-local int for fast copy-paste fromcallers --json; canonical for rebuild stability.edgeIdin callers/callees JSONrowToEdgepatch plumbs it through. Documented as index-local in the Edge type's docstring.--rerun--line N --col Nhint. Explain explains ONE edge.Edge.id exposure
src/types.tsEdgegains an optionalid?: numberfield with a docstring noting it's index-local — resets on rebuild.src/db/queries.tsrowToEdgepropagates the row id.src/bin/codegraph.tscollectGraphRelationsaddsedgeIdto the JSON entries when present.schemas/cli/callers.jsonandschemas/cli/callees.jsondocument the new optional field.Read-only safety
pathToFileURL(dbPath).href + '?immutable=1'— same pattern as PRs #39 and #40. No-shm/-walsidecar creation; DB stays bit-for-bit unchanged. Regression test snapshotsdb/-wal/-shm/-journalbefore/after.Schema gate
No v6 requirement —
provenance/metadatacolumns existed in v5.assertCodeGraphDbprobessqlite_masterfor bothedgesANDnodestables in a single query; rejects withnot a CodeGraph databaseif either is missing. Missing resolver metadata is tolerated:resolvedBy: null,confidence: null,metadata: null— explain explains what's there.Reviewer trail
REVIEW--kindshould be required for canonical lookupExplainCanonical.kindnowstring(not optional); CLI rejects with hintREVIEWassertCodeGraphDbshould check both tablesWHERE name IN ('edges', 'nodes')REVIEWconfidenceshould be clamped to [0, 1]nullbefore emitNITPICKedgeIdin callers/callees schemasedgeId: integer minimum: 1Files
src/explain.ts—explainEdgeById,explainEdgeByCanonical,formatExplainNarrative(~340 lines)src/types.ts—Edge.id?: numberwith index-local docstringsrc/db/queries.ts—rowToEdgepropagates the row idsrc/bin/codegraph.ts—codegraph explain [edgeId]subcommand;collectGraphRelationsaddsedgeIdschemas/cli/explain.json— JSON Schema (envelope.tool=explain)schemas/cli/callers.json,schemas/cli/callees.json— documentedgeId__tests__/explain.test.ts— 10 unit tests__tests__/cli-json-schemas.test.ts— schema validation forexplain --jsonTest plan
npx tsc --noEmit— cleannpx vitest run— 1065 passed | 2 skipped | 62 files, 0 failuresnpm run build— clean (Node 24)npm run test:eval:structural— required=5/5, all-pass=8/8, blocking=0, recall=1.00, precision=1.00, fp=0🤖 Generated with Claude Code