Repository navigation
fix(skill): stop a stale .graphify_partial.txt from poisoning the next build (#4814) - #4832
Deepjyoti-Sarmah wants to merge 4 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. |
github-actions
Bot
added
the
contributor
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
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 5 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 4 changed function(s) (1 sampled, not proven, 3 not verified).
Formal verification. PR-changed functions: 1/4 verified (0 proven, 1 may-equivalent, 0 distinguished) · 3 not verified (1 vacuous, 2 unsupported).
Not verified on this run: dispatch\_command (vacuous: never exercised), detect\_incremental (unsupported), \_rebuild\_code (unsupported).
Graphify review — findings
Makes graphify update re-extract only files whose content differs from the manifest instead of the whole corpus, with --force or a missing manifest still taking the full path. Extracts the deleted-versus-excluded manifest split into classify_manifest_deletions so detect_incremental and the update path share it, and counts resolution inputs as scanned so they don't read as excluded on every run. Clears .graphify_partial.txt at the start of cache checking in every skill variant so a stale list from an earlier --update can't leave fully cached docs unstamped and re-queued, while the new partial-list read in the update merge/manifest blocks embeds Python in a double-quoted bash string and is the riskiest part of this change.
Worth a look
- Unescaped double quotes inside bash double-quoted
python -cstring break partial-list read —graphify/skills/claude/references/update.md:177· Escalate · high- NOT verified (no proof, no reproducing execution) — needs human review
- Unescaped double quotes inside a bash -c "..." block break the partial-list read —
graphify/skills/agents/references/update.md:177· Escalate · high- NOT verified (no proof, no reproducing execution) — needs human review
- Unescaped double quotes inside bash -c "..." break the Python when a partial list exists —
graphify/skills/copilot/references/update.md:177· Escalate · high- NOT verified (no proof, no reproducing execution) — needs human review
- Unescaped double quotes inside bash -c "..." break the Python when a partial list exists —
graphify/skills/droid/references/update.md:177· Escalate · high- NOT verified (no proof, no reproducing execution) — needs human review
- Unescaped double quotes inside bash -c "..." break the Python when a partial list exists —
graphify/skills/kilo/references/update.md:177· Escalate · high- NOT verified (no proof, no reproducing execution) — 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 — 4947 functions depend on the 1314 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
_rebuild_code()— 228 callers, 70 callees - worse:
load_manifest()— 17 callers, 5 callees - worse:
classify_manifest_files()— 4 callers, 5 callees - new:
_scenario_code_only_update()— 0 callers, 8 callees - new:
test_the_shipped_runbook_drops_a_stale_list_before_step9()— 0 callers, 7 callees - new:
_scenario_coarse_clock()— 0 callers, 6 callees - new:
test_a_half_read_doc_on_the_update_path_is_not_stamped()— 0 callers, 6 callees
Verification — 4947 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: 3451 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
468 of 468 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— impact, 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— impact, 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— 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_backend_opt_in.py— impact, 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— impact, 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— full-run-safetytests/test_cache_long_paths.py— full-run-safetytests/test_cache_stale_import_target.py— impact, 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— impact, full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cloud_cta.py— impact, full-run-safety- … and 418 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.
Docs that may be stale (advisory)
ARCHITECTURE.md§ Pipeline (lines 5-12): references changed symbolsdetectARCHITECTURE.md§ Module responsibilities (lines 13-41): references changed symbolsdetectARCHITECTURE.md§ Adding a new language extractor (lines 87-94): references changed symbolsdetectCHANGELOG.md§ 0.9.96 (2026-10-25) (lines 5-35): references changed symbolsdetectCHANGELOG.md§ 0.9.84 (2026-10-10) (lines 276-289): references changed symbols__init__CHANGELOG.md§ 0.9.77 (2026-10-05) (lines 370-389): references changed symbols__init__CHANGELOG.md§ 0.9.58 (2026-09-10) (lines 622-639): references changed symbols__init__CHANGELOG.md§ 0.9.42 (2026-08-13) (lines 841-859): references changed symbols__init__CHANGELOG.md§ 0.9.25 (2026-07-22) (lines 1034-1038): references changed symbolsdetectCHANGELOG.md§ 0.9.23 (2026-07-21) (lines 1054-1063): references changed symbols__init__
…and 10 more.
Formal verification
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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: not verifiable: all 40 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
Could not verify: Could not verify detect\_incremental.
The verifier did not have enough to check detect\_incremental, 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: nondeterministic output (both versions disagreed with themselves on the witness) — not a behaviour change; a function whose output is not a function of its inputs (random / time / uuid / os.urandom, or state carried across calls) cannot be soundly distinguished by execution, so this tier abstains for the whole function
No difference found (not proven): No behavior difference found in \_queue\_pending\_full (not a proof).
The verifier ran both versions of \_queue\_pending\_full 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.
Could not verify: Could not verify \_rebuild\_code.
The verifier did not have enough to check \_rebuild\_code, 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: nondeterministic output (both versions disagreed with themselves on the witness) — not a behaviour change; a function whose output is not a function of its inputs (random / time / uuid / os.urandom, or state carried across calls) cannot be soundly distinguished by execution, so this tier abstains for the whole function
· 4 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| return project.next_update_queue() | ||
|
|
||
|
|
||
| def _scenario_code_only_update(project: Project) -> set[str]: |
There was a problem hiding this comment.
_scenario_code_only_update()
fans out to 8 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return project.next_update_queue() | ||
|
|
||
|
|
||
| def _scenario_coarse_clock(project: Project) -> set[str]: |
There was a problem hiding this comment.
_scenario_coarse_clock()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| ) | ||
|
|
||
|
|
||
| def test_the_shipped_runbook_drops_a_stale_list_before_step9(tmp_path): |
There was a problem hiding this comment.
test_the_shipped_runbook_drops_a_stale_list_before_step9()
fans out to 7 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return tail[:-1] | ||
|
|
||
|
|
||
| def test_a_half_read_doc_on_the_update_path_is_not_stamped(tmp_path): |
There was a problem hiding this comment.
test_a_half_read_doc_on_the_update_path_is_not_stamped()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
9f637c8 to
75b4d81
Compare
…bs#4814) A .graphify_partial.txt left by an --update (which stops before Step 9) is read by a later full build's Step 9, so a doc that build served in full is left unstamped and re-queued once. The A/B harness drives the real rendered B0/B3.5/B3-cache/Step-9 blocks against four candidate lifecycles - baseline, delete-at-start (A), run-id (B), freshness-on-read (C) - and scores them with the real detect_incremental and check_semantic_cache. Every policy starts from the same pre-fix counterfactual, so the matrix stays valid after the fix lands. Measured (see test_scoreboard): baseline 2/7, delete 7/7, runid 7/7, fresh 6/7 Choose A: B buys nothing on supported scenarios and adds a persistent file plus a header every reader must filter; C has a coarse-clock hole.
…run (Graphify-Labs#4814) Step B3.5 writes the list of files a subagent only partly read; Step 9 reads it and the same block deletes it. An --update stops at Step 8, so its list stays on disk. A later full build that serves that doc from the semantic cache never runs B3.5, so it never rewrites the list, and Step 9 reads the stale entry: the doc is left unstamped and the next --update re-queues a file this build already had in full. Apply the same rule B0 already applies to leftover .graphify_chunk_*.json files: delete the file before the stage that writes it. - B0 deletes it before B3.5 can rewrite it (full builds). - The --update runbook deletes it at its start, because an update stops before Step 9 and so can never rely on Step 9's cleanup. This also covers a code-only update, which skips B0 and B3 entirely. The A/B harness keeps reproducing the pre-fix baseline by stripping shipped hooks from its counterfactual, and gains an end-to-end regression against the real rendered fragments.
…aphify-Labs#4807) An --update stops before Step 9, so its only manifest save is the merge block in references/update.md, and that call passed no partial set. A doc B3.5 had just flagged as partly read therefore got stamped as done: detect_incremental never re-queued it and the head-only node set stayed live permanently - the same permanent-loss shape Graphify-Labs#2225 fixed on the full-build path. Pass the run's .graphify_partial.txt into _stamped_manifest_files, and drop the list after use because an --update has no Step 9 to clean up after it. Pinned by driving the real rendered manifest-save tail and asserting the doc is re-queued by detect_incremental and the list is gone afterwards.
38606b3 to
b5d4a62
Compare
) The Graphify-Labs#4807 regression extracted the manifest tail and ran it as Python, so it could not see a quoting error in the block's python -c wrapper. An unescaped encoding="utf-8" broke the block only when .graphify_partial.txt existed, which is exactly the path the test is about. Drive the shipped block through bash with the list on disk, and assert both the partial filter and the end-of-save cleanup.
What does this PR do?
Fixes #4814 and #4807. Both are the same missing lifecycle on
graphify-out/.graphify_partial.txt.Step B3.5 writes the file, listing sources a subagent read only partway. Step 9 reads it so those sources stay unstamped, then deletes it.
--updatestops at Step 8, so its list outlived the run. A later full build that served the doc from the semantic cache never ran B3.5 and never rewrote the list. Step 9 read the stale entry and left a doc it served in full unstamped, so the next--updatere-queued it.Before, on the reproduction from the issue:
After:
The same script with the list removed also printed
[], which is how the list became the suspect.The second half is #4807. An
--updatehas one manifest save, in its merge block. It passed no partial set, so a doc B3.5 had just flagged as partly read was stamped as done and never retried.Both hooks delete before any writer runs, so any list a reader sees was written by this run. The update path has no Step 9, so its manifest save both consumes the list and removes it.
Delete-at-start was picked over a run id or an mtime check by measuring all three against the real rendered blocks. Delete-at-start and run-id both score 7/7 on the supported scenarios. Run-id adds a persistent id file and a header every reader has to filter. Mtime freshness scores 6/7 and fails when a stale list carries the same timestamp as this run's
.graphify_detect.json.Type of change
Verification & Invariants
The invariant is that a doc is left unstamped only when a subagent read it partway in this run. Step 9's partial set is a subset of what this run's B3.5 wrote.
The start hooks run before B3.5, so any list a reader sees was written after them. The update path reads the list at its only manifest save and deletes it there. This PR deletes at the end of that save. The follow-up #4811 keeps the list longer, because a merge re-run and Step 9 after an update also need it. That is a separate change with its own tests.
Generated state: all 134 skill artifacts are regenerated from the fragments and
--checkpasses. No graph, cache, or manifest format changes, so nothing to migrate.Merge Danger
Door: two-way. Runbook text, regenerated artifacts, and one test file. Reverting
0f70bd86orb5d4a62drestores the previous behavior;2af27a8eis test-only. A stale list already on disk heals at the next run's start.Blast Radius: skill-only, across all 13 hosts.
graphify-out/remain unsafe. Step 9's cleanup is a single globalrm, so a second run can remove a live run's list.test_another_runs_step9_destroys_the_list_for_every_candidatepins it. This change does not try to make the list run-scoped.runidandfreshpolicies are harness models, not shipped code. They exist to justify the choice and are exercised only there.unlinkon a fixed relative path andrm -fon a fixed path.How was this tested?
Commands run, with output:
The new test file runs each lifecycle through the real B0, B3.5, B3 cache-save, and Step 9 blocks, and reads the outcome from the real
detect_incrementalandcheck_semantic_cache:Three end-to-end tests use the shipped fragments with no policy injected:
test_the_shipped_runbook_drops_a_stale_list_before_step9runs the issue's reproduction.test_a_half_read_doc_on_the_update_path_is_not_stampedruns the real manifest-save tail fromreferences/update.mdand asserts the doc is re-queued and the list is removed.test_the_shipped_update_merge_block_runs_with_a_partial_listruns the whole merge block through bash with the list on disk. This is the path that evaluates the list read, so a quoting error in thepython -cwrapper fails the test instead of shipping.Environment note: the suite ran with the repo's
.venvbecause the system Python has no numpy.graphify update .was run after the change;graphify-out/is gitignored.Graphify-specific checklist
python -m tools.skillgen --bless) when changing their source fragments.rmandunlinktargets are fixed literal paths.