Skip to content

fix(skill): run Step B3 when every semantic file is cached - #4117

Closed
brunovima83 wants to merge 1 commit into
Graphify-Labs:v8from
brunovima83:fix/skillgen-cache-skip-b3
Closed

brunovima83 wants to merge 1 commit into
Graphify-Labs:v8from
brunovima83:fix/skillgen-cache-skip-b3

Conversation

@brunovima83

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #4116.

When every doc, paper and image hits the semantic cache, the split-host runbook says "skip to Part C directly". Part C reads graphify-out/.graphify_semantic.json unconditionally, and only the Step B3 merge writes it. As a result, a second /graphify on an unchanged mixed corpus fails with FileNotFoundError. Writing an empty file instead hides the error but drops every cached node.

Changes, all in tools/skillgen/fragments/core/core.md, with artifacts regenerated by skillgen:

  1. The all-cached sentence now says to skip B1 and B2 but still run Step B3's commands. It also says explicitly not to substitute an empty .graphify_semantic.json.
  2. Step B0 deletes any graphify-out/.graphify_chunk_*.json before dispatch. B3 merges every chunk file it finds, and nothing has been dispatched yet at B0. So a chunk left by an interrupted run would otherwise be merged as fresh, and with change 1 that now includes all-cached runs.

New test file: tests/test_skill_semantic_all_cached.py.

Type of change

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

Verification & Invariants

Invariant: every route into Part C leaves a .graphify_semantic.json that contains the cached semantic nodes plus this run's chunks, and nothing else.

How this is proven:

  • test_all_cached_run_is_routed_through_the_b3_merge checks that every rendered split host routes the all-cached case through B3.
  • test_all_cached_run_reaches_part_c_with_cached_nodes_and_no_stale_chunks renders the claude body and executes its real Step B0, three Step B3 and Part C Python blocks in a temp dir. The corpus is all-cached (seeded through save_semantic_cache with the same prompt_file), and a stale chunk sits on disk. The test asserts that Part C succeeds, the cached node is present and the stale node is absent.
  • Both tests fail on v8 (35adf43) and pass with the fix.

Persisted state: the semantic cache is untouched. B3 already limits cache writes to .graphify_uncached.txt (allowed_source_files), which is empty in the all-cached case, so nothing is re-saved.

  • Read the CONTRIBUTING.md guide.
  • Reproduced the issue and identified the invariant.
  • Made the smallest fix necessary.
  • Added a regression test.
  • Kept the PR description synchronized with the final implementation.
  • Documented any limitations / unsupported cases explicitly.

Limitations:

  • The aider and devin monoliths still say "skip to Part C directly". They are pinned to the v8 baseline by --monolith-roundtrip, so they need a sanctioned round-trip change and are left for a follow-up.
  • The behavioural test runs the Python bodies with sys.executable -c, not through bash. It is cross-platform, but it does not exercise the $(cat graphify-out/.graphify_python) wrapper.

How was this tested?

# env: macOS 12 x86_64, Python 3.12; uv sync --frozen --all-extras minus video/all/leiden (no wheels for this platform)
uv run pytest tests/test_skill_semantic_all_cached.py -q    # before fix (35adf43): 2 failed; after fix: 2 passed
uv run python -m tools.skillgen --bless                     # blessed 134 artifact(s)
uv run python -m tools.skillgen --check                     # OK, 134 artifacts
uv run python -m tools.skillgen --audit-coverage            # OK
uv run python -m tools.skillgen --schema-singleton          # OK
uv run python -m tools.skillgen --monolith-roundtrip        # OK
uv run python -m tools.skillgen --always-on-roundtrip       # OK
uv run pytest tests/test_skill_semantic_all_cached.py tests/test_skillgen.py tests/test_skillgen_input_path_injection.py -q   # 97 passed
uv run pytest tests/ -q                                     # 6552 passed, 16 skipped
uv run ruff check .                                         # All checks passed
uv run pyright tests/test_skill_semantic_all_cached.py      # 0 errors

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.

🤖 Generated with Claude Code

The split-host runbook told the agent to skip to Part C when every doc,
paper and image hit the semantic cache. Part C reads
.graphify_semantic.json unconditionally and only the Step B3 merge writes
it, so a second run on an unchanged mixed corpus failed with
FileNotFoundError; writing an empty file instead dropped every cached node.

The all-cached case now skips B1/B2 but still runs B3. Step B0 also clears
.graphify_chunk_*.json before dispatch: B3 merges every chunk on disk, and a
leftover from an interrupted run would otherwise be merged as fresh.

The aider and devin monoliths keep the old sentence; they are pinned to the
v8 baseline by --monolith-roundtrip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

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

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).


Graphify review — findings

Fixes the all-cached path in the generated skill docs: Part B's cache check now deletes leftover .graphify_chunk_*.json files from interrupted runs before anything is dispatched. Previously Step B3 would merge those stale chunks. When every file is cached, the instructions now skip only Steps B1 and B2 and still run Step B3's merge, because that merge is the only step that writes .graphify_semantic.json, which Part C reads unconditionally. They also warn against writing an empty file instead, since that would drop every cached node.

No blocking issues surfaced. 15 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 736 functions depend on the 736 functions this change touches.

Health — grade A; no new coupling hotspots.

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

Test selection

Test selection

330 of 330 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 — full-run-safety
  • tests/test_astro_import_ids.py — 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 — full-run-safety
  • tests/test_build.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 — full-run-safety
  • tests/test_cache.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 — full-run-safety
  • tests/test_case_sensitive_resolution.py — full-run-safety
  • tests/test_charmap_encoding.py — full-run-safety
  • tests/test_chunking.py — full-run-safety
  • tests/test_cjs_module_extension.py — 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 — 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 — full-run-safety
  • tests/test_cpp_nested_and_cli.py — full-run-safety
  • tests/test_cpp_objc_cross_file_calls.py — full-run-safety
  • tests/test_cpp_preprocess.py — full-run-safety
  • tests/test_cross_extension_reexport_self_cycle.py — full-run-safety
  • … and 280 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.

@safishamsi

Copy link
Copy Markdown
Member

Landed in v0.9.77 via an authorship-preserving cherry-pick, so your commit is on v8 with you credited as the author. Closing as shipped — thanks @brunovima83!

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.

[Bug]: All-cached semantic run skips Step B3, so Part C fails on missing .graphify_semantic.json

2 participants