Repository navigation
fix(extract): give the model each file's real id stem (#4432) - #4434
Deepjyoti-Sarmah wants to merge 3 commits into
Conversation
|
Thanks for the pull request, @Deepjyoti-Sarmah. 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 3 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 unsupported).
Not verified on this run: \_read\_files (unsupported).
Graphify review — findings
Fixes inconsistent node IDs for files with non-ASCII or unusual paths by computing each file's canonical id prefix in code with the new file_id_stem, instead of leaving the model to guess it. The direct LLM path stamps that prefix as a stem="..." attribute on every <untrusted_source> block, and the prompt now tells the model to keep letters of any script, never transliterate, and copy the stem verbatim. The skill templates write a path<TAB>stem sidecar (.graphify_uncached_stems.txt) that subagent chunks copy unchanged; they fall back to the bare .graphify_uncached.txt list when the sidecar is missing, and paths with no root-relative form get an empty stem rather than one built from the checkout location.
Worth a look
- Backticks inside python -c shell string trigger command substitution —
graphify/skill-claw.md:253· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Backticks inside python -c shell string trigger command substitution —
graphify/skill-codex.md:253· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Backticks inside python -c shell string trigger command substitution —
graphify/skill-copilot.md:253· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Review partial — this diff was larger than one review pass covers, so later files were not reviewed; some findings may be missing.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 5592 functions depend on the 1009 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 947 callers, 57 callees - worse:
_read_files()— 21 callers, 7 callees - worse:
_semantic_id_remap()— 13 callers, 8 callees
Verification — 5592 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: 3773 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
413 of 413 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_agents_platform.py— full-run-safetytests/test_alias_folding.py— impact, full-run-safetytests/test_analyze.py— impact, 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— impact, 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— impact, full-run-safetytests/test_backend_extras.py— impact, full-run-safetytests/test_benchmark.py— impact, full-run-safetytests/test_benchmark_raw_graph.py— impact, full-run-safetytests/test_benchmark_real_query.py— impact, 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_stale_import_target.py— 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_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— impact, full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cloud_cta.py— full-run-safetytests/test_cluster.py— impact, full-run-safetytests/test_cluster_ambiguous_scale.py— full-run-safetytests/test_cluster_exclude_hubs.py— full-run-safetytests/test_cobol_extractor.py— full-run-safety- … and 363 more
non-code file(s) changed (
graphify/skill-agents.md,graphify/skill-amp.md,graphify/skill-claw.md,graphify/skill-codex.md,graphify/skill-copilot.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 (
graphify/skill-agents.md,graphify/skill-amp.md,graphify/skill-claw.md,graphify/skill-codex.md,graphify/skill-copilot.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 \_read\_files.
The verifier did not have enough to check \_read\_files, 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 `units` is annotated `'list[Path | FileSlice]'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_wrap\_untrusted (not a proof).
The verifier ran both versions of \_wrap\_untrusted 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.
· 3 more finding(s) on lines outside this diff (see the check run).
…e's stem (Graphify-Labs#4432) The prompts said ids are "only [a-z0-9_]", but ids.py keeps Unicode letters, so for a non-ASCII path the model invented a different stem per chunk and per run. The prompts now state what ids.py does, and each file's canonical stem is computed in code and handed to the model: a stem attribute in the chunk prompt, and a stems file the skill's agent copies into each subagent's file list. This makes the right id explicit; it does not enforce it, and a model can still ignore the stem.
…y-Labs#4432) Bash expands backticks inside a double-quoted -c block, so it tried to run path<TAB>stem and printed an error. Drop the backticks and keep the words; regenerate with --bless.
75218d9 to
ca021bb
Compare
# Conflicts: # graphify/skill-agents.md # graphify/skill-amp.md # graphify/skill-claw.md # graphify/skill-codex.md # graphify/skill-copilot.md # graphify/skill-droid.md # graphify/skill-kilo.md # graphify/skill-kiro.md # graphify/skill-opencode.md # graphify/skill-pi.md # graphify/skill-trae.md # graphify/skill-vscode.md # graphify/skill-windows.md # graphify/skill.md # tools/skillgen/expected/graphify__skill-agents.md # tools/skillgen/expected/graphify__skill-amp.md # tools/skillgen/expected/graphify__skill-claw.md # tools/skillgen/expected/graphify__skill-codex.md # tools/skillgen/expected/graphify__skill-copilot.md # tools/skillgen/expected/graphify__skill-droid.md # tools/skillgen/expected/graphify__skill-kilo.md # tools/skillgen/expected/graphify__skill-kiro.md # tools/skillgen/expected/graphify__skill-opencode.md # tools/skillgen/expected/graphify__skill-pi.md # tools/skillgen/expected/graphify__skill-trae.md # tools/skillgen/expected/graphify__skill-vscode.md # tools/skillgen/expected/graphify__skill-windows.md # tools/skillgen/expected/graphify__skill.md # tools/skillgen/fragments/core/core.md
What does this PR do?
Fixes #4432. #3103 reported the same thing and was closed without a fix.
The extraction prompts say node ids use "only
[a-z0-9_]".graphify/ids.pykeeps Unicode letters. For a path likedocs/tz/ЛЗ4.mdthe model cannot follow both, so it invents a stem, and the stem differs between chunks and between runs._semantic_id_remaponly strips stems it already knows, so none of them get fixed.This PR states the real rule and gives the model each file's stem, computed in code.
Both paths use one new helper,
file_id_stem(path, root)ingraphify/extractors/base.py. It returnsmake_id(_file_stem(...))of the root-relative path. The skill computes the stems in the same Python step that already runs the cache check. No path is pasted into code, and no agent types a stem.I left
_semantic_id_remapalone. A made-up stem has no known length, so a repair can't tell where the stem ends and the entity begins. The #4281 prototype also showed that re-keying a node from itssource_filemoves a node that mentions another file under the wrong file.Before / after
Same id, two different stems:
Read literally, the old rule turns that path into
docs_tz_4_. That result comes from running the rule as code, not from a model run.Tests,
tests/test_unicode_id_rule.py:Merge danger
Door: two-way. No stored format changes. The only new file is a temp file that Part B cleans up, and reverting means one commit.
Blast radius: medium. Every semantic extraction prompt changes:
graphify extractusers and all 14 skill hosts, including ASCII-only repos. Reverting causes one more miss.stem="..."(tested). File-node ids do not change for any path.[a-z0-9_]rule.check_semantic_cacheline incore.md. Whichever merges second changesroot = 'INPUT_PATH'toroot = read_scan_root().Not covered
concept_*,s301_totalsin the issue).Type of change
Verification & Invariants
Invariant: the stem the model is told to use is the same string the document pass mints for that file's node, whatever script the path is in.
test_file_id_stem_is_the_prefix_the_pipeline_mintsruns a Cyrillic and an ASCII document through the realextract(). The page-node id equalsfile_id_stem, the chunk prompt carries that stem, and_semantic_id_remapleaves a node under it alone.file_id_stemis tested with an absolute path and root".", with a path outside the root (returns""), and with",<and>in the name. The stem holds only letters, digits and_, so it cannot break out of the attribute.The Part B step is extracted from the generated
graphify/skill.mdand run. It writes the expectedpath<TAB>stemlines, withINPUT_PATHsubstituted once.The rule wording is checked in
llm.pyand in all 14 generated specs, with exact host counts so a dropped host fails the test.Generated state this invalidates: the semantic cache (once), plus 14 skill files and 14 specs, which are regenerated with
--bless.Read the CONTRIBUTING.md guide.
Reproduced the issue and identified the invariant.
Made the smallest fix necessary.
Added a regression test (if bug fix) or isolated boundary test.
Kept the PR description synchronized with the final implementation.
Documented any limitations / unsupported cases explicitly.
How was this tested?
Run from
/var/tmpwithPYTHONPATHset to the worktree, so the code under test is this branch and not an editable install of another checkout.Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments.