Repository navigation
fix(detect): break manifest key duplicate collapse ties by seen time, not iteration order - #3781
ayushcodes10 wants to merge 9 commits into
Conversation
A manifest written across a mix of versions or call sites, some passing root to relativize keys and some not, an outdated installed skill runbook for one, can end up carrying both an absolute and a relative key for the same file, each with different data. Both load_manifest and the relativize step inside save_manifest already canonicalize every key through a comprehension, so the two forms do collapse into one entry, but a plain dict comprehension keeps whichever raw key happens to iterate last, an artifact of on disk JSON key order that has nothing to do with which entry is actually current. A fresher hash could be silently discarded in favor of a stale one purely because of how the keys happened to accumulate, letting the incremental change detector call a genuinely changed file unchanged. Reproduced directly: a manifest with a stale absolute entry and a fresh relative entry for the same file returns the stale one when it happens to be written second, and the fresh one when the order is reversed, confirming the collapse really is order dependent today. Both call sites now go through one small helper that breaks a collision by each entry's own seen timestamp when both sides have one and they disagree, since that field exists specifically to record when an entry was last confirmed current. A legacy or partial entry without that field falls back to the existing last one wins behavior unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
One test covers load_manifest directly, checking both on disk orderings of a stale absolute entry and a fresh relative entry for the same file, confirming the fresher one wins regardless of which key iterates last. A companion test covers the write side, seeding a save through an untouched file's existing duplicate rows and confirming the relativize step keeps the fresher entry there too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh
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 2 advisory finding(s) below merit a look before merge.
Formal verification. 2 change(s) tested, no difference found (not proven).
Graphify review — findings
Fixes manifest key collapse when a file has both an absolute and a relative key (from save_manifest call sites that vary on passing root): load_manifest and save_manifest now route through _collapse_manifest_duplicates, which keeps the entry with the later seen timestamp rather than whichever raw key iterates last in the on-disk JSON. This stops detect_incremental() from retaining a stale hash and under-reporting a real content change; legacy entries lacking seen (or non-dict values) fall back to the old last-wins behavior.
Worth a look
- Duplicate collapse misses relative keys with dot segments —
graphify/detect.py:2231· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Tie-break loses fresher entry when only one side has a numeric seen —
graphify/detect.py:2196· 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 — 2831 functions depend on the 747 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 686 callers, 45 callees - new:
_rebuild_code()— 144 callers, 55 callees - new:
detect()— 112 callers, 15 callees - new:
_extract_generic()— 18 callers, 29 callees - new:
save_manifest()— 41 callers, 12 callees - new:
extract_js()— 87 callers, 4 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_xaml()— 19 callers, 17 callees - …and 47 more — each is listed as a finding
Verification — 2831 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: 1213 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
298 of 298 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-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.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— impact, 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— impact, full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.py— impact, 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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— impact, 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— impact, full-run-safetytests/test_claude_cli_backend.py— impact, 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_cluster.py— full-run-safetytests/test_cobol_extractor.py— full-run-safetytests/test_codebuddy.py— full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— impact, full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_external_call_guards.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safety- … and 248 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.
Formal verification
No difference found (not proven): No behavior difference found in load\_manifest (not a proof).
The verifier ran both versions of load\_manifest 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.
No difference found (not proven): No behavior difference found in save\_manifest (not a proof).
The verifier ran both versions of save\_manifest 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 grounded finding(s) anchored inline below; 53 more finding(s) on lines outside this diff (see the check run).
| return result | ||
|
|
||
|
|
||
| def load_manifest( |
There was a problem hiding this comment.
load_manifest()
9 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| ) | ||
|
|
||
|
|
||
| def save_manifest( |
There was a problem hiding this comment.
save_manifest()
fans out to 12 callees (efferent coupling); 41 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
_to_absolute_from_storage joined a relative key onto the resolved root with a plain path join, which never collapses a parent segment the way resolve does. A relative key such as sub/../foo.py, the kind of format mismatch this function exists to tolerate across a mix of call sites and versions per its own docstring, then canonicalized to a different string than the plain absolute key for the same file, so the duplicate collapse this function exists for silently missed it. The joined result is now run through normpath, which purely lexically collapses dot segments without touching the filesystem or following symlinks, matching the sibling relativize function's own choice not to resolve the key. Fixes review finding on PR 3781.
Covers the review finding directly: a relative manifest key with a redundant dot dot segment must canonicalize onto the same absolute path as a plain key for the same file and collapse with it, keeping the more recently seen entry, instead of surviving as a second, uncollapsed row. Confirmed against pre fix code via git stash: the test fails there and passes only with the fix applied.
|
Addressing both findings: "Duplicate collapse misses relative keys with dot segments" (detect.py:2231) — real, fixed. "Tie-break loses fresher entry when only one side has a numeric seen" (detect.py:2196) — working as documented, not changing it. When only one side of a collision carries a Full suite (5780 passed) is green. |
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 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
Fixes manifest.json duplicate-key handling so an entry present under both an absolute and a relative key for the same file (from a mix of save_manifest call sites/versions) collapses to the more recently seen entry instead of whichever key iterates last in the on-disk JSON, preventing detect_incremental() from keeping a stale hash and missing a real change. Both load and save paths now route through _collapse_manifest_duplicates, which breaks ties by timestamp and falls back to last-wins only for legacy entries lacking seen. Also runs the re-anchored key through os.path.normpath in _to_absolute_from_storage so relative keys with ../. segments canonicalize to the same string as the plain absolute key (lexical only, no symlink resolution).
Worth a look
- Duplicate-collapse tie-break drops fresher entry when only one side has a seen timestamp —
graphify/detect.py:2200· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- normpath on absolute Windows/relative-root keys may still not match relative-key canonical form —
graphify/detect.py:2167· 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 — 2833 functions depend on the 749 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 686 callers, 45 callees - new:
_rebuild_code()— 144 callers, 55 callees - new:
detect()— 112 callers, 15 callees - new:
_extract_generic()— 18 callers, 29 callees - new:
save_manifest()— 41 callers, 12 callees - new:
extract_js()— 87 callers, 4 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_xaml()— 19 callers, 17 callees - …and 47 more — each is listed as a finding
Verification — 2833 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 4c73561 (diverged from this PR's base — delta is approximate).
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: 1215 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
298 of 298 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-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.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— impact, 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— impact, full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.py— impact, 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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— impact, 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— impact, full-run-safetytests/test_claude_cli_backend.py— impact, 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_cluster.py— full-run-safetytests/test_cobol_extractor.py— full-run-safetytests/test_codebuddy.py— full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— impact, full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_external_call_guards.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safety- … and 248 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 grounded finding(s) anchored inline below; 53 more finding(s) on lines outside this diff (see the check run).
| return result | ||
|
|
||
|
|
||
| def load_manifest( |
There was a problem hiding this comment.
load_manifest()
10 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| ) | ||
|
|
||
|
|
||
| def save_manifest( |
There was a problem hiding this comment.
save_manifest()
fans out to 12 callees (efferent coupling); 41 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Path.is_absolute() only recognizes the current platform's own path syntax, so a manifest key using a different platform's absolute form, a POSIX /abs/path loaded on Windows, or a C: or UNC path loaded on POSIX, was wrongly judged relative and joined onto the scan root, producing a nonsense path like root plus C: colon Users colon x colon foo dot py instead of being left alone. A manifest genuinely can move between platforms, which is this whole module's own stated scope for tolerating format mismatches. A new helper checks for a leading slash or backslash or a drive letter pattern in addition to the native is_absolute check, so a foreign absolute key is recognized and passed through unchanged on any platform instead of only the one that produced it. Fixes review finding on PR 3781.
Covers the review finding directly: a Windows drive letter key, a UNC key, and a POSIX absolute key must all be recognized as already absolute regardless of which platform the test runs on, and must not be joined onto the scan root. Confirmed against pre fix code via git stash, the test fails there since the recognizing helper does not exist yet.
|
Addressing both findings: "normpath on absolute Windows/relative-root keys may still not match relative-key canonical form" (detect.py:2167) — real, fixed, and worse than described. `Path.is_absolute()` only recognizes the CURRENT platform's own syntax: verified with `PureWindowsPath`/`PurePosixPath` directly that a POSIX-style `/abs/path` key loaded on Windows, or a `C:...`/UNC key loaded on POSIX, is judged NOT absolute. Pre-fix, that meant `_to_absolute_from_storage` didn't just fail to normalize such a key — it actively joined it onto the scan root, producing a nonsense path like `/C:/Users/x/foo.py`. Reproduced this directly against the pre-fix code. Fixed with a platform-independent `_looks_absolute` check (leading slash/backslash, or a drive-letter pattern, in addition to the native check) so a foreign-platform key is recognized and left alone on any platform. This matters here specifically because a manifest genuinely can move between machines, which is this whole module's own stated scope. "Duplicate-collapse tie-break drops fresher entry when only one side has a seen timestamp" (detect.py:2200) — same point as last round, still not changing it. This is explicitly documented as the intended fallback in the function's own docstring, and I don't think there's a principled alternative: with no timestamp on one side, there's no signal to compare against, so falling back to last-wins isn't clearly wrong — a seen-less entry could just as easily be the genuinely current one from a call site that never tracked `seen` at all. Happy to revisit with a concrete scenario where this loses real data. Full suite (5781 passed) is green. |
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 2 advisory finding(s) below merit a look before merge.
Formal verification. 3 change(s) tested, no difference found (not proven).
Graphify review — findings
Fixes manifest.json duplicate-key collapse so an entry seen across mixed save_manifest call sites (some passing root, some not) resolves to a single deterministic key: _collapse_manifest_duplicates now keeps the entry with the later seen timestamp instead of whichever raw key iterates last, preventing detect_incremental() from keeping a stale hash and under-reporting a real content change. Runs joined keys through os.path.normpath (lexical only, no symlink resolution) so a relative key with a redundant ../. segment canonicalizes to the same string as its plain absolute form. Recognizes absolute keys under any platform's syntax via _looks_absolute, so a POSIX, drive-letter, or UNC key loaded on a foreign platform passes through unchanged rather than being wrongly joined onto the scan root.
Worth a look
- Legacy absolute manifest keys are no longer passed through verbatim —
graphify/detect.py:2190· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- normpath on foreign-platform absolute key mangles path separators —
graphify/detect.py· 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 — 2837 functions depend on the 753 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 686 callers, 45 callees - new:
_rebuild_code()— 144 callers, 55 callees - new:
detect()— 112 callers, 15 callees - new:
_extract_generic()— 18 callers, 29 callees - new:
save_manifest()— 41 callers, 12 callees - new:
extract_js()— 87 callers, 4 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_xaml()— 19 callers, 17 callees - …and 47 more — each is listed as a finding
Verification — 2837 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 4c73561 (diverged from this PR's base — delta is approximate).
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: 1219 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
298 of 298 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-safetytests/test_affected_member_seed.py— full-run-safetytests/test_agents_platform.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— impact, 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— impact, full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_build.py— impact, 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_cache.py— full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— impact, 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— impact, full-run-safetytests/test_claude_cli_backend.py— impact, 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_cluster.py— full-run-safetytests/test_cobol_extractor.py— full-run-safetytests/test_codebuddy.py— full-run-safetytests/test_community_hub_labels.py— full-run-safetytests/test_community_labels_skill.py— full-run-safetytests/test_confidence.py— full-run-safetytests/test_corrupt_graph_json.py— full-run-safetytests/test_cpp_nested_and_cli.py— impact, full-run-safetytests/test_cpp_objc_cross_file_calls.py— full-run-safetytests/test_cpp_preprocess.py— full-run-safetytests/test_cross_extension_reexport_self_cycle.py— full-run-safetytests/test_cross_language_call_resolution.py— full-run-safetytests/test_cross_repo_external_call_guards.py— full-run-safetytests/test_cross_repo_member_calls.py— full-run-safety- … and 248 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.
Formal verification
No difference found (not proven): No behavior difference found in load\_manifest (not a proof).
The verifier ran both versions of load\_manifest 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.
No difference found (not proven): No behavior difference found in save\_manifest (not a proof).
The verifier ran both versions of save\_manifest 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.
No difference found (not proven): No behavior difference found in \_to\_absolute\_from\_storage (not a proof).
The verifier ran both versions of \_to\_absolute\_from\_storage 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 grounded finding(s) anchored inline below; 53 more finding(s) on lines outside this diff (see the check run).
| return result | ||
|
|
||
|
|
||
| def load_manifest( |
There was a problem hiding this comment.
load_manifest()
10 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| ) | ||
|
|
||
|
|
||
| def save_manifest( |
There was a problem hiding this comment.
save_manifest()
fans out to 12 callees (efferent coupling); 41 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
Checked both, neither needs a change: "Legacy absolute manifest keys are no longer passed through verbatim" (detect.py:2190) — intentional, and I think actually beneficial, not a regression. Before this PR, an absolute key was returned as-is (`str(p)`); now it's run through `os.path.normpath` too. The only keys this can actually change are ones that are absolute but NOT already in canonical form — a stray `..`/`.` segment, a trailing slash, a doubled separator. `normpath` is idempotent on anything already canonical, which is what `detect()`'s own `str(Path(...).resolve())` output always is — so for the common case (a legacy key written by graphify itself) this is a no-op. For the uncommon case (a key written by a different tool, or an older graphify version, in a non-canonical but still valid form), normalizing it makes it MORE likely to match `detect()`'s own canonical output on comparison, not less — which is the entire point of this PR's duplicate collapse. I don't see a scenario where preserving the non-canonical string verbatim would have been the more correct behavior. "normpath on foreign-platform absolute key mangles path separators" — true, and an accepted, unavoidable side effect, not new breakage. A POSIX key encountered on Windows (or vice versa) already isn't a valid local path on this platform, before or after `normpath` touches it — it was never going to resolve to a real file here either way. What `normpath` changes is only its string form (e.g. `/` becomes `\` if this runs on Windows), which doesn't make an already-unreachable foreign reference any more or less reachable. What matters for this PR's purpose is that it consistently canonicalizes to the SAME form on repeated calls, which it does, so two occurrences of the same foreign key still collapse correctly. I don't think there's a normalization choice here that avoids the separator conversion while still being platform-independent. Full suite is green, no changes pushed for this round. |
Follow-up edits on #3845 to match the settled maintainer decisions: - CODE_OF_CONDUCT.md: set the enforcement/reporting contact to safi@graphify.com (was an unspecified 'email the maintainer if known'). - README team-graph section: add the machine-local never-commit list (.graphify_root, .graphify_python, .graphify_analysis.json, the AST cache, needs_update) and the re-extract-to-update note. manifest.json stays committable since #3781 made its keys portable. - Add RELEASING.md (maintainer-facing): version bump + changelog + uv.lock, test on 3.10/3.13, push v8, gh release create -> PyPI via trusted publishing (no twine/token), verify with uvx --refresh. - CONTRIBUTING Further Reading: drop the paid Gumroad link, point at RELEASING.md instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Shipped in v0.9.69 (now on PyPI) via an authorship-preserving cherry-pick, so your commit keeps contributor-graph credit. Thanks @ayushcodes10! Manifest duplicate-key collapse now breaks ties by last-seen time (plus foreign-abs-path and dot-segment normalization). Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.69 |
Fixes #1964, specifically the still-live gap flagged in this new comment from @cabgab-dev.
The original symptom in this issue (absolute-path manifest keys breaking incremental detection across clones/CI) looked fixed on current
v8when I checked earlier —save_manifest(root=...)relativizes keys, andload_manifest(root=...)re-anchors them, both citing "the #777/#1964 portability guarantee". But a manifest written across a mix of call sites — some passingroot, some not (an outdated installed skill runbook, for one) — can still end up with both an absolute and a relative key for the same file, each carrying different data.Both
load_manifestandsave_manifest's relativize step already canonicalize every raw key through a dict comprehension, which does correctly collapse the two forms down to one entry — but keeps whichever raw key happens to iterate last in the on-disk JSON, an artifact of arbitrary key order with no relation to which entry is actually current. A fresher hash can get silently discarded in favor of a stale one purely because of how the duplicate keys happened to accumulate, which is exactly @cabgab-dev's reported symptom:detect_incremental()sometimes reads the stale duplicate and under-reports genuine changes (146 files missing from the semantic cache reported as "2 files changed").Reproduced directly: built a manifest with one stale absolute-keyed entry and one fresh relative-keyed entry for the same file, and confirmed via
load_manifestthat swapping their on-disk JSON order flips which one survives — the collapse really is order-dependent today.Fixed with a small shared helper,
_collapse_manifest_duplicates, used at both collapse sites. It breaks a collision by each entry's ownseentimestamp (present specifically to record when an entry was last confirmed current) when both sides have one and disagree, falling back to the historical last-wins behavior for legacy/partial entries with noseenfield — so this only changes behavior for the exact ambiguous case it's meant to fix.Two regression tests in
tests/test_detect.py, each checked in both on-disk key orderings to prove the fix is order-independent, both confirmed to fail against pre-fix code viagit stashfirst. Full suite is green aside from the same 16 pre-existing failures from the newly-added Erlang/R/Solidity/VB.NET extractor tests (missing optional grammar packages in my dev environment, unrelated to this change — already noted on #3780).🤖 Generated with Claude Code
https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh