Skip to content

fix(extract): give the model each file's real id stem (#4432) - #4434

Open
Deepjyoti-Sarmah wants to merge 3 commits into
Graphify-Labs:v8from
Deepjyoti-Sarmah:fix/4432-unicode-id-rule
Open

Deepjyoti-Sarmah wants to merge 3 commits into
Graphify-Labs:v8from
Deepjyoti-Sarmah:fix/4432-unicode-id-rule

Conversation

@Deepjyoti-Sarmah

Copy link
Copy Markdown
Contributor

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.py keeps Unicode letters. For a path like docs/tz/ЛЗ4.md the model cannot follow both, so it invents a stem, and the stem differs between chunks and between runs. _semantic_id_remap only 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.

 graphify extract (llm.py)
-  <untrusted_source path="docs/tz/ЛЗ4.md" sha256="...">
+  <untrusted_source path="docs/tz/ЛЗ4.md" sha256="..." stem="docs_tz_лз4">

 skill, Part B
   check_semantic_cache(...)
     writes .graphify_uncached.txt              # unchanged
+    writes .graphify_uncached_stems.txt        # path<TAB>stem, same order
   Step B1: FILE_LIST lines
-    /abs/docs/tz/ЛЗ4.md
+    /abs/docs/tz/ЛЗ4.md<TAB>docs_tz_лз4       # falls back to the old file if missing

 rule text (llm.py + both extraction-spec fragments)
-  Node ID format: lowercase, only [a-z0-9_], no dots or slashes.
+  Node ID format: lowercase. Letters and digits of any script are kept as written
+  (never transliterated to ASCII); every run of any other character becomes one _.
+  ...use the given stem character for character as the id prefix.

Both paths use one new helper, file_id_stem(path, root) in graphify/extractors/base.py. It returns make_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_remap alone. 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 its source_file moves a node that mentions another file under the wrong file.

Before / after

Same id, two different stems:

before:  ids.py stem      docs_tz_лз4_обоснование_с_листом_расчет_кдог
         model's stem     docs_tz_lz4_obosnovanie_kdog            (from the issue; changes per run)
         remap            {}                                      (nothing reconciled)

after:   prompt carries   stem="docs_tz_лз4_обоснование_с_листом_расчет_кдог"
         page node id     docs_tz_лз4_обоснование_с_листом_расчет_кдог   (same string, tested)

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:

before fix:  43 failed, 2 passed   (the 2 are the host-count guards)
after fix:   45 passed

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:

  • Cache: the semantic cache is keyed on the prompt text, so every existing entry misses once. This hits graphify extract users and all 14 skill hosts, including ASCII-only repos. Reverting causes one more miss.
  • ASCII paths: the chunk prompt is byte-identical apart from the added stem="..." (tested). File-node ids do not change for any path.
  • Skill hosts: 14 hosts get the regenerated skill and spec. Aider and devin are not touched. They never stated the [a-z0-9_] rule.
  • fix(skill): stop pasting agent-supplied values into skill scripts (#4309) #4344: one-line conflict. Both PRs touch the same check_semantic_cache line in core.md. Whichever merges second changes root = 'INPUT_PATH' to root = read_scan_root().

Not covered

  • Nothing enforces the id. The model can still ignore the stem it is given, and then ids drift as before. Enforcing it means having code mint the ids, which is a larger change.
  • Nodes with no file stem at all (concept_*, s301_totals in the issue).
  • On the skill path, a symlinked doc whose target is outside the scan root gets no stem and falls back to the rule text.
  • The aider and devin monolith skills.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Tests or CI
  • Refactor
  • Security fix

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_mints runs a Cyrillic and an ASCII document through the real extract(). The page-node id equals file_id_stem, the chunk prompt carries that stem, and _semantic_id_remap leaves a node under it alone.

  • file_id_stem is 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.md and run. It writes the expected path<TAB>stem lines, with INPUT_PATH substituted once.

  • The rule wording is checked in llm.py and 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/tmp with PYTHONPATH set to the worktree, so the code under test is this branch and not an editable install of another checkout.

# repro, no LLM
python repro.py
  docs_tz_лз4_обоснование_с_листом_расчет_кдог
  docs_tz_срз_кп18_с301_лз2_105_12_10_2026
  _semantic_id_remap -> {}

python -m pytest tests/test_unicode_id_rule.py -q --basetemp=/var/tmp/pt-4432 -p no:cacheprovider
  before: 43 failed, 2 passed
  after:  45 passed, 1 warning in 1.51s

python -m tools.skillgen --bless              -> blessed 134 artifact(s) into expected/.
python -m tools.skillgen --check              -> check OK: 134 artifact(s) match committed output and expected/.
python -m tools.skillgen --monolith-roundtrip -> monolith-roundtrip OK

python -m pytest -q --basetemp=/var/tmp/pt-4432-full
  7970 passed, 14 skipped, 1994 warnings in 390.54s

ruff check graphify/extractors/base.py graphify/llm.py tests/test_unicode_id_rule.py
  All checks passed!
pyright graphify/extractors/base.py graphify/llm.py
  19 errors, same as upstream/v8 (new test file: 0)

Graphify-specific checklist

  • I updated generated skill artifacts (uv run python -m tools.skillgen --bless) when changing their source fragments.
  • I confirmed that AST/structural extraction remains deterministic (no ambient state dependencies like ENV variables).
  • I reviewed changes for security implications (no unsafe interpolation into shell/Python).
  • I confirmed no API keys or local-only graph data are included.
  • (If applicable) I disclosed AI authorship in my commit messages.

@github-actions github-actions Bot added the contributor Pull request from a returning community contributor label Oct 13, 2026
@github-actions

Copy link
Copy Markdown

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.

@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 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-safety
  • tests/test_affected_cli.py — full-run-safety
  • tests/test_affected_defines.py — full-run-safety
  • tests/test_affected_member_seed.py — full-run-safety
  • tests/test_affected_overrides.py — full-run-safety
  • tests/test_agents_platform.py — full-run-safety
  • tests/test_alias_folding.py — impact, full-run-safety
  • tests/test_analyze.py — impact, 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 — impact, full-run-safety
  • tests/test_backend_env_isolation.py — impact, full-run-safety
  • tests/test_backend_extras.py — impact, full-run-safety
  • tests/test_benchmark.py — impact, full-run-safety
  • tests/test_benchmark_raw_graph.py — impact, full-run-safety
  • tests/test_benchmark_real_query.py — impact, full-run-safety
  • tests/test_blade_extractor.py — full-run-safety
  • tests/test_build.py — impact, full-run-safety
  • tests/test_build_located_semantic_identity.py — impact, full-run-safety
  • tests/test_build_merge_dedup_scope.py — impact, full-run-safety
  • tests/test_build_merge_hyperedges_and_prune.py — impact, full-run-safety
  • tests/test_build_merge_shrink_guard.py — impact, full-run-safety
  • tests/test_builtin_global_type_refs.py — full-run-safety
  • tests/test_c_include_resolution.py — impact, full-run-safety
  • tests/test_cache.py — full-run-safety
  • tests/test_cache_stale_import_target.py — 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 — impact, full-run-safety
  • tests/test_case_sensitive_resolution.py — 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 — impact, 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 — impact, full-run-safety
  • tests/test_cli_help.py — full-run-safety
  • tests/test_cloud_cta.py — full-run-safety
  • tests/test_cluster.py — impact, full-run-safety
  • tests/test_cluster_ambiguous_scale.py — full-run-safety
  • tests/test_cluster_exclude_hubs.py — full-run-safety
  • tests/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.
@Deepjyoti-Sarmah
Deepjyoti-Sarmah force-pushed the fix/4432-unicode-id-rule branch from 75218d9 to ca021bb Compare October 16, 2026 18:45
# 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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor Pull request from a returning community contributor

Projects

None yet

1 participant