Skip to content

fix(commonlisp): derive node ids from the full path stem - #3131

Open
guitelesc wants to merge 1 commit into
Graphify-Labs:v8from
guitelesc:fix/commonlisp-path-qualified-ids
Open

fix(commonlisp): derive node ids from the full path stem#3131
guitelesc wants to merge 1 commit into
Graphify-Labs:v8from
guitelesc:fix/commonlisp-path-qualified-ids

Conversation

@guitelesc

Copy link
Copy Markdown

extract_commonlisp builds its id prefix from the bare path.stem, so a/sample.lisp and b/sample.lisp both mint sample / sample_init. Every other extractor uses _file_stem(path), which preserves the full repo-relative path for exactly this reason (#1504) — commonlisp.py never imported it. It looks like the line predates the port out of extract.py and came along unchanged.

Impact

Not cosmetic — nodes are lost. The in-batch collision rescue hides it when both files land in one extract() call, but incremental graphify update extracts in separate batches:

=== SEPARATE batches (what incremental update does) ===
  [a] 'sample'  'sample_init'   <- a/sample.lisp
  [b] 'sample'  'sample_init'   <- b/sample.lisp
  COLLIDING IDS ACROSS BATCHES: ['sample', 'sample_init']

=== merged graph: 2 nodes (expected 4) ===

build() merges last-writer-wins and b/sample.lisp disappears. It does warn loudly, which is how I found it.

The id-remap post-pass doesn't cover this either: it rewrites absolute-path-derived ids (#502), and a bare stem matches none of its keys.

This also means any graph containing a .lisp file trips graph_has_legacy_ids, so query/serve print the "pre-#1504 node-ID scheme" nudge on a freshly built graph. On this repo, tests/fixtures/sample.lisp alone was enough.

The fix

stem = _make_id(_file_stem(path))

Pre-collapsed through _make_id rather than passed to _cl_id raw, because _CL_CHAR_MAP maps / to _slash — the naive stem = _file_stem(path) that matches the other extractors yields tests_slashfixtures_slashsample_init. Pre-collapsing is idempotent under _cl_id and leaves the CL operator-char handling intact (upi= -> upi_eq, verified via header_eq / header_lt in the existing fixture).

Testing

Added test_cl_ids_are_path_qualified_across_directories, modelled on the existing test_decldef_merge_does_not_merge_across_directories. Red/green verified by reverting the one-line change: fails with overlap ['sample', 'sample_init'], passes with it.

uv run pytest tests/ -q — 5086 passed, 11 skipped, 0 failed.

One note for reviewers: the AST cache is keyed by content hash, so after this lands a graph built with an older graphify keeps the old ids until graphify-out/cache/ast is cleared. The cache dir is version-stamped (v0.9.50-s2), so a release bump handles it.

🤖 Generated with Claude Code

`extract_commonlisp` built its id prefix from the bare `path.stem`, so
`a/sample.lisp` and `b/sample.lisp` both minted `sample` / `sample_init`.
Every other extractor uses `_file_stem(path)`, which preserves the full
repo-relative path for exactly this reason (Graphify-Labs#1504); commonlisp never
imported it.

The in-batch collision rescue hides this when both files land in one
extract() call, but incremental `graphify update` extracts in separate
batches -- there the ids collide, build() merges them last-writer-wins,
and one file's nodes are dropped from the graph. The id-remap post-pass
does not cover it either: it rewrites absolute-path-derived ids (Graphify-Labs#502),
and a bare stem matches none of its keys.

The stem is pre-collapsed through `_make_id` rather than passed to
`_cl_id` raw, because `_CL_CHAR_MAP` would map the `/` separators to
`_slash` (`tests_slashfixtures_slashsample_init`). Pre-collapsing is
idempotent under `_cl_id` and leaves the CL operator-char handling
intact (`upi=` -> `upi_eq`).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Fixes Common Lisp node id collisions between same-named .lisp files in different directories by deriving the id prefix from the path-qualified _file_stem (run through _make_id) instead of the bare path.stem, so separate extract batches no longer merge and drop one file's nodes (#1504). Adds a regression test asserting extract_commonlisp mints disjoint ids for a/sample.lisp and b/sample.lisp.

No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 547 functions depend on the 547 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract_commonlisp() — 28 callers, 6 callees
  • new: _process_form() — 1 callers, 10 callees
  • new: _handle_defpackage() — 1 callers, 7 callees
  • new: _handle_def_form() — 1 callers, 6 callees

Verification — 547 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 547 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify extract\_commonlisp.

The verifier did not have enough to check extract\_commonlisp, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

· 4 more finding(s) on lines outside this diff (see the check run).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant