Repository navigation
Conversation
…bs#4858) Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
Thanks for the pull request, @yUm1k4. 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. |
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; formal verification proved 0 of 2 changed function(s) (1 sampled, not proven, 1 not verified).
Formal verification. PR-changed functions: 1/2 verified (0 proven, 1 may-equivalent, 0 distinguished) · 1 not verified (1 witness not reproduced).
Not verified on this run: extract\_terraform (witness not reproduced).
Graphify review — findings
Fixes stale absolute Terraform node IDs after an AST cache is copied between checkouts: each cached Terraform/HCL result now records the scope used to mint its IDs, and extraction on a cache hit uses it to rewrite node IDs and edge endpoints relative to the current scan root. Module-source topology is still resolved against the current permitted corpus, and the scope marker stays internal and never reaches the exported graph, which makes the leak risk on files outside the root worth a close look. Bumps _AST_CACHE_SCHEMA to 30 so entries without the marker are retired (the semantic cache is untouched), so existing graphs with absolute Terraform IDs need graphify extract . --code-only --force.
Worth a look
- _terraform_scope leaks into results when the file is outside root —
graphify/extractors/terraform.py:88· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2879 functions depend on the 157 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 1054 callers, 62 callees - worse:
extract_terraform()— 28 callers, 10 callees
Verification — 2879 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: 1695 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
469 of 469 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_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_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— impact, full-run-safetytests/test_blade_extractor.py— full-run-safetytests/test_build.py— full-run-safetytests/test_build_located_semantic_identity.py— full-run-safetytests/test_build_merge_dedup_scope.py— full-run-safetytests/test_build_merge_hyperedges_and_prune.py— full-run-safetytests/test_build_merge_shrink_guard.py— full-run-safetytests/test_builtin_global_type_refs.py— full-run-safetytests/test_c_include_resolution.py— full-run-safetytests/test_cache.py— impact, full-run-safetytests/test_cache_long_paths.py— impact, 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— full-run-safetytests/test_case_sensitive_resolution.py— full-run-safetytests/test_charmap_encoding.py— impact, 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-safetytests/test_cli_broken_pipe.py— full-run-safetytests/test_cli_export.py— full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cloud_cta.py— full-run-safety- … and 419 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.
Docs that may be stale (advisory)
CHANGELOG.md§ 0.9.96 (2026-10-25) (lines 31-61): references changed symbolsscanCHANGELOG.md§ 0.9.16 (2026-07-14) (lines 1175-1206): references changed symbolsscanCHANGELOG.md§ 0.8.46 (2026-06-23) (lines 1501-1512): references changed symbolsscanCHANGELOG.md§ 0.8.40 (2026-06-16) (lines 1558-1578): references changed symbolsscanCHANGELOG.md§ 0.8.1 (2026-05-15) (lines 1918-1929): references changed symbolsscandocs/superpowers/plans/2026-05-04-incremental-updates-dedup.md§ Task 4: Incremental updates — semantic cache + manifest in __main__.py (lines 603-911): references changed symbolsscangraphify/skill-agents.md§ Step 1 - Ensure graphify is installed (lines 65-112): references changed symbolsscangraphify/skill-agents.md§ Part A - Structural extraction for code files (lines 178-206): references changed symbolsscangraphify/skill-agents.md§ Part B - Semantic extraction (parallel subagents) (lines 207-415): references changed symbolsscangraphify/skill-agents.md§ Step 9 - Save manifest, update cost tracker, clean up, and report (lines 626-735): references changed symbolsscan
…and 10 more.
Formal verification
Could not verify: extract\_terraform: a possible difference could NOT be confirmed.
The solver proposed a distinguishing input for extract\_terraform, but re-running both versions on it did NOT reproduce a difference. Treat as unconfirmed, not a behavior change.
Guarantee:
Note: Solver-proposed witness refuted on replay — not a hard failure.
No difference found (not proven): No behavior difference found in prepare\_terraform (not a proof).
The verifier ran both versions of prepare\_terraform on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 2 more finding(s) on lines outside this diff (see the check run).
|
Scope cleanup now happens before the out-of-root early return. Regression tests cover fresh and cached out-of-root results, symlink escapes, idempotence, and preservation of the stored cache metadata. The only mutation to an out-of-root result is removal of the internal scope marker; node IDs, edge endpoints, and topology remain unchanged. |
What does this PR do?
Fixes #4858.
Terraform/HCL AST caches copied between checkout directories could retain the original absolute scope in node IDs. A cached rebuild then produced different IDs from a clean build of the same project.
The extractor now records its original scope in internal _terraform_scope metadata. The existing preparation pass consumes it to normalize node IDs and edge endpoints against the current scan root. Module-source resolution still uses the current permitted corpus.
AST cache schema 30 retires entries without scope metadata. This causes a one-time AST re-extraction across languages; semantic caches are unchanged. Existing graphs with stale Terraform IDs require a full code rebuild:
No public function signatures, CLI flags, or exported graph schema change.
Type of change
Verification & Invariants
The invariant is that moving a project together with its AST cache produces the same node IDs and reference edges as a clean build, while preserving module isolation.
The CLI reproduction compares a fresh build in A, a build in B using A's copied cache, and a clean build in C. Before the fix, B retains A's absolute scope. After the fix, all three agree, with a 100% AST cache hit in B.
The 16 new cases cover copied caches without parsing, root/nested directories, .tf/.hcl/.tfvars, actual parallel workers, cross-file references, distinct module scopes, module topology, idempotent preparation, and old-cache invalidation.
How was this tested?
Recorded local validation: macOS ARM64, Python 3.13.7, tree-sitter-hcl 1.2.0. Dependencies were installed with uv sync --all-extras --frozen.
All seven full-suite failures were also reproduced on an unmodified snapshot of base commit 0a8cd4e: six URL/fetch tests fail on DNS resolution of example.com, and one shell-trace control test fails in the local Bash 3.2.57 environment. These unrelated failures are unchanged by this PR.
Linux and Windows have not been tested locally. The full suite and Pyright are not reported as fully passing.
Graphify-specific checklist