Skip to content

fix(python): preserve nested class identities by enclosing class - #4228

Closed
xiehuanyi wants to merge 1 commit into
Graphify-Labs:v8from
xiehuanyi:fix/python-nested-class-identity-20261008
Closed

xiehuanyi wants to merge 1 commit into
Graphify-Labs:v8from
xiehuanyi:fix/python-nested-class-identity-20261008

Conversation

@xiehuanyi

Copy link
Copy Markdown
Contributor

What does this PR do?

Same-named nested Python classes in one file currently share a node id, so three separate Params definitions collapse to one node and their identically named methods collapse with them. Include the enclosing class id when assigning a nested class id. Keep decorator and docstring edges on that same owner.

Addresses the declaration-identity problem in #4220. Top-level class ids remain unchanged. Bump the AST cache schema so a warm extraction does not keep the collapsed definitions.

Type of change

  • Bug fix

Verification & Invariants

  • Read CONTRIBUTING.md, AGENTS.md, ARCHITECTURE.md and SECURITY.md.
  • Reproduced two failing regressions on upstream v8.
  • Added regression coverage for different parents, repeated nesting, same-name parent/child, methods, decorator/docstring owners, warm caches and relocated checkouts.
  • Kept the fix scoped to Python nested class identity.
  • Documented the cache and identity invariant in ARCHITECTURE.md.

How was this tested?

uv run --frozen pytest -q tests/test_python_nested_class_identity.py tests/test_languages.py tests/test_cache.py
  563 passed (before the additional decorator/docstring regression)
uv run --frozen pytest -q tests/test_python_nested_class_identity.py
  4 passed
PATH=/usr/bin:/bin:$PATH uv run --frozen pytest tests/ -q --tb=short
  6798 passed, 14 skipped
uv run --frozen ruff check .
uv run --frozen graphify update .
uv run --frozen pre-commit run --files <the five changed files>
git diff --check
  passed

The initial full-suite run encountered 85 failures caused by the local Git wrapper relying on HOME, which the tests deliberately isolate. The unmodified v8 checkout reproduced the same 85 failures. Using system Git resolved them without changing tests.

Pyright was run on both the unmodified base and this branch. It reports pre-existing errors; comparing diagnostic file/message/severity multisets found no newly introduced diagnostics. It is not a clean type-check result.

Graphify-specific checklist

  • No generated skill source changed; skillgen verification passed.
  • AST identities are deterministic across caches and relocated checkouts.
  • No unsafe shell/source interpolation introduced.
  • No credentials or local graph output committed.
  • AI assistance disclosed in commit metadata: OpenAI Codex.

This change does not extend the resolver for qualified nested-class references or for classes local to functions. Those are separate resolution concerns.

@xiehuanyi
xiehuanyi requested a review from safishamsi as a code owner October 8, 2026 11:07
Copilot AI balanced review requested due to automatic review settings October 8, 2026 11:07

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

Thanks for the pull request, @xiehuanyi. 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.

@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.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.

Formal verification. PR-changed functions: 0/2 verified (0 proven, 0 may-equivalent, 0 distinguished) · 2 not verified (2 unsupported).

Not verified on this run: \_extract\_python\_rationale (unsupported), \_extract\_generic (unsupported).


Graphify review — findings

Fixes Python nested class identity by deriving the id from the enclosing class rather than the file stem, so same-named nested classes like Forward.Params and Backward.Params keep separate nodes, methods, containment edges, decorator links, and docstring rationale. Top-level class ids are unchanged. _AST_CACHE_SCHEMA moves to 7 so cached extractions with the old collapsed ids are discarded and rebuilt.

Worth a look

  • Function-local class docstring rationales can point at non-existent node IDs — graphify/extract.py:1821 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 3202 functions depend on the 657 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract() — 800 callers, 51 callees
  • new: _rebuild_code() — 158 callers, 56 callees
  • new: detect() — 121 callers, 16 callees
  • new: _extract_generic() — 18 callers, 34 callees
  • new: save_semantic_cache() — 65 callers, 9 callees
  • new: load_cached() — 60 callers, 8 callees
  • new: extract_js() — 87 callers, 5 callees
  • new: file_hash() — 54 callers, 6 callees
  • …and 64 more — each is listed as a finding

Verification — 3202 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: 3009 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

345 of 345 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/test_affected_cli.py — full-run-safety
  • tests/test_affected_member_seed.py — full-run-safety
  • tests/test_agents_platform.py — full-run-safety
  • tests/test_analyze.py — full-run-safety
  • tests/test_anthropic_custom_endpoint.py — full-run-safety
  • tests/test_antigravity_install.py — full-run-safety
  • tests/test_apm_fallback_version.py — full-run-safety
  • tests/test_architecture_doc.py — full-run-safety
  • tests/test_astro_extraction.py — impact, full-run-safety
  • tests/test_astro_import_ids.py — impact, full-run-safety
  • tests/test_atomic_canvas_export.py — full-run-safety
  • tests/test_atomic_version_stamp.py — full-run-safety
  • tests/test_atomic_writes.py — full-run-safety
  • tests/test_backend_env_isolation.py — full-run-safety
  • tests/test_backend_extras.py — full-run-safety
  • tests/test_benchmark.py — full-run-safety
  • tests/test_benchmark_raw_graph.py — full-run-safety
  • tests/test_blade_extractor.py — impact, full-run-safety
  • tests/test_build.py — impact, full-run-safety
  • tests/test_build_located_semantic_identity.py — full-run-safety
  • tests/test_build_merge_dedup_scope.py — full-run-safety
  • tests/test_build_merge_hyperedges_and_prune.py — full-run-safety
  • tests/test_build_merge_shrink_guard.py — full-run-safety
  • tests/test_builtin_global_type_refs.py — impact, full-run-safety
  • tests/test_cache.py — impact, full-run-safety
  • tests/test_cache_stale_import_target.py — impact, full-run-safety
  • tests/test_callflow_html.py — full-run-safety
  • tests/test_cargo_introspect.py — full-run-safety
  • tests/test_cargo_missing_manifest.py — full-run-safety
  • tests/test_carried_hyperedge_remap.py — full-run-safety
  • tests/test_case_sensitive_resolution.py — impact, full-run-safety
  • tests/test_charmap_encoding.py — impact, full-run-safety
  • tests/test_chunking.py — impact, full-run-safety
  • tests/test_cjs_module_extension.py — impact, full-run-safety
  • tests/test_claude_cli_backend.py — full-run-safety
  • tests/test_claude_md.py — full-run-safety
  • tests/test_cli_broken_pipe.py — full-run-safety
  • tests/test_cli_export.py — full-run-safety
  • tests/test_cli_help.py — full-run-safety
  • tests/test_cluster.py — full-run-safety
  • tests/test_cluster_exclude_hubs.py — full-run-safety
  • tests/test_cobol_extractor.py — impact, full-run-safety
  • tests/test_codebuddy.py — full-run-safety
  • tests/test_community_hub_labels.py — full-run-safety
  • tests/test_community_labels_skill.py — full-run-safety
  • tests/test_confidence.py — full-run-safety
  • tests/test_corrupt_graph_json.py — full-run-safety
  • tests/test_cpp_method_declarations.py — impact, full-run-safety
  • tests/test_cpp_nested_and_cli.py — impact, full-run-safety
  • tests/test_cpp_objc_cross_file_calls.py — impact, full-run-safety
  • … and 295 more

non-code file(s) changed (ARCHITECTURE.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 (ARCHITECTURE.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.

Formal verification

Could not verify: Could not verify \_extract\_python\_rationale.

The verifier did not have enough to check \_extract\_python\_rationale, 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: no capturable inputs from the test suite; property tier: not verifiable: the input domain has 59 values but only 24 distinct were tested — a small finite domain must be EXHAUSTED, not sampled (an untested input could invert the result)

Could not verify: Could not verify \_extract\_generic.

The verifier did not have enough to check \_extract\_generic, 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: no capturable inputs from the test suite; property tier: parameter `config` is annotated `LanguageConfig` — outside the synthesizable primitive/collection set

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

@safishamsi

Copy link
Copy Markdown
Member

Landed in v0.9.81 via an authorship-preserving cherry-pick. Note: this and #4222 both bumped _AST_CACHE_SCHEMA 6→7, so I merged them to a single bump to 7 (with a combined comment) rather than double-bumping. Closing as shipped — thanks @xiehuanyi!

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.

3 participants