Repository navigation
fix(watch): preserve semantic evidence across AST id collisions - #4950
andrebrait wants to merge 1 commit into
Conversation
|
Thanks for the pull request, @andrebrait. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
…AST id collisions Squashed from origin/fix/semantic-ast-collision (db62842) onto upstream/v8.
db62842 to
6e0de8f
Compare
…AST id collisions Squashed from origin/fix/semantic-ast-collision (6e0de8f) onto upstream/v8.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; no formal proof of the changed code.
Graphify review — findings
Fixes AST-only watch updates dropping semantic evidence whose node id collides with a code definition: _reconcile_existing_graph now keeps saved semantic nodes that have a source_file, then runs them through _namespace_reference_stub_collisions so both the code definition and the document-owned node survive in graph.json, with or without --no-cluster. The collision pass gains a preserve_saved_semantics mode that namespaces pre-marker semantic nodes from other files, falls back to numbered suffixes when the __ref__ name is already taken, and leaves AST-origin edges alone. Afterward, any leftover non-AST node sharing an id with a fresh AST node (same-source, external-stub or sourceless copies) is discarded so it can't overwrite the new definition, which also drops its provenance if it wasn't renamed.
Worth a look
- Semantic copies left unrenamed are silently dropped along with their provenance —
graphify/watch.py:1358· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1288 functions depend on the 463 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
_rebuild_code()— 218 callers, 67 callees - worse:
_reconcile_existing_graph()— 1 callers, 14 callees
Verification — 1288 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: 1024 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
489 of 489 test file(s) selected (100%) via static blast radius.
Escalated to a full run for safety — the selection is not trustworthy on its own (see below). CI should run the whole suite.
tests/pi.test.ts— full-run-safetytests/test_4874_jvm_resolution.py— full-run-safetytests/test_4909_reference_binding_scale.py— full-run-safetytests/test_affected_cli.py— full-run-safetytests/test_affected_defines.py— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_affected_overrides.py— full-run-safetytests/test_affected_path_file_node.py— full-run-safetytests/test_affected_seed_reference_forms.py— full-run-safetytests/test_agents_platform.py— full-run-safetytests/test_alias_folding.py— full-run-safetytests/test_analyze.py— full-run-safetytests/test_anthropic_custom_endpoint.py— full-run-safetytests/test_antigravity_install.py— full-run-safetytests/test_apm_fallback_version.py— full-run-safetytests/test_architecture_doc.py— full-run-safetytests/test_astro_extraction.py— full-run-safetytests/test_astro_import_ids.py— full-run-safetytests/test_atomic_canvas_export.py— full-run-safetytests/test_atomic_version_stamp.py— full-run-safetytests/test_atomic_writes.py— full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_backend_opt_in.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_benchmark_real_query.py— full-run-safetytests/test_binary_media_words.py— full-run-safetytests/test_blade_extractor.py— full-run-safetytests/test_build.py— impact, full-run-safetytests/test_build_located_semantic_identity.py— impact, full-run-safetytests/test_build_merge_dedup_scope.py— impact, full-run-safetytests/test_build_merge_hyperedges_and_prune.py— impact, full-run-safetytests/test_build_merge_shrink_guard.py— impact, full-run-safetytests/test_builtin_global_type_refs.py— full-run-safetytests/test_c_include_resolution.py— impact, full-run-safetytests/test_cache.py— full-run-safetytests/test_cache_long_paths.py— full-run-safetytests/test_cache_stale_import_target.py— impact, full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— full-run-safetytests/test_cargo_missing_manifest.py— full-run-safetytests/test_carried_hyperedge_remap.py— impact, full-run-safetytests/test_case_only_decl_ids_4904.py— full-run-safetytests/test_case_sensitive_resolution.py— full-run-safetytests/test_charmap_encoding.py— full-run-safetytests/test_chunking.py— impact, full-run-safetytests/test_cjs_module_extension.py— full-run-safetytests/test_claude_cli_backend.py— full-run-safetytests/test_claude_md.py— full-run-safety- … and 439 more
non-code file(s) changed (
CHANGELOG.md) → running the full suite for safety (a code graph can't see config/fixture/data deps)
changed code file(s) with no mapped test (
CHANGELOG.md) — a coverage gap or a missing link — running the full suite rather than only the selected tests
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
· 2 more finding(s) on lines outside this diff (see the check run).
|
Landed in 0.9.101 (published to PyPI), cherry-picked with your authorship preserved. This is the AST-id-collision facet (a saved semantic node whose id collides with a fresh code definition in the reconcile path); it is distinct from the clustered build fold in #4896/#4927, and both shipped. Thanks @andrebrait. |
Problem
An AST-only update silently discards saved cross-file semantic evidence when a freshly extracted AST node takes its id. Reproduced while auditing tracked semantic graphs, including a CodeRabbit concept in Leveret.
Change
Only graph.json carries portable provenance. No extraction cache is tracked, and no cache-based freshness requirement or commit/push hook policy is introduced.
Verification
Full upstream CI was not run on the fork because its Actions are disabled. No upstream bot review requests were posted because the authenticated account has read-only permission in the base repository.