skill: mandatory-ID gate for re-extracted docs on --update#2162
Open
AirRocker wants to merge 1 commit into
Open
skill: mandatory-ID gate for re-extracted docs on --update#2162AirRocker wants to merge 1 commit into
AirRocker wants to merge 1 commit into
Conversation
…hed doc Semantic re-extraction of a doc that already has nodes in graph.json is non-deterministic. On a large project CLAUDE.md, successive --update runs of the same unchanged-except-one-line file returned 75 nodes and then 49, silently dropping a whole documented section. Nothing errored: the loss went into build_merge and would only have surfaced later as a question the graph could no longer answer. It was caught solely because the to_json shrink guard (Graphify-Labs#479) refused the write. Adds a required three-step gate to the --update flow, in the shared references/update.md fragment plus a pointer from the core template: 1. Snapshot that file's existing node IDs from graph.json before extracting. 2. Pass them to the extraction subagent as MANDATORY must-include IDs, with the baseline node count as an explicit target. Reusing the IDs also suppresses gratuitous ID churn, which orphans saved queries and inflates the merge diff for no semantic gain. 3. Hard-gate the merge: refuse unless node count >= 90% of baseline AND zero mandatory IDs are missing. On failure, re-dispatch naming the missing IDs; two consecutive failures stop and report rather than merging a regression. Also restates that the shrink guard is the authoritative backstop: never force past it without diffing the old and new node sets and being able to name why each removed node is legitimately gone. Fragments are the edited source; graphify/skill*.md, graphify/skills/**, graphify/always_on/** and tools/skillgen/expected/** are regenerated via `python -m tools.skillgen` and `--bless`. Markdown only, no code changes. All five skillgen guards pass (--check, --audit-coverage, --schema-singleton, --monolith-roundtrip, --always-on-roundtrip).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Semantic re-extraction of a doc that already has nodes in
graph.jsonis non-deterministic. Running--updatetwice on the same large projectCLAUDE.md— changed by one line between runs — returned 75 nodes on one run and 49 on the next, silently dropping an entire documented section.Nothing errored. The loss went straight into
build_mergeand would only have surfaced much later as a question the graph could no longer answer. It was caught solely because theto_jsonshrink guard (#479) refused the write.The failure mode is quiet by construction: the extraction "succeeds", the chunk is valid JSON, and the merge is happy. Only a node-count comparison against the prior build reveals it.
Fix
Adds a required three-step gate to the
--updateflow, applied to any changed doc/paper that already has nodes in the graph:graph.jsonbefore extracting.It also restates that the shrink guard is the authoritative backstop: never
force=Truepast it without diffing the old and new node sets and being able to name why each removed node is legitimately gone (file deleted, section removed, ID renamed with a verified replacement). ID churn on a re-extracted file is a legitimate shrink; a missing section is not.Reusing the prior IDs has a second benefit beyond completeness: it suppresses gratuitous ID churn on re-extraction (
claude_weather_proxy_worker→claude_workers_weather_proxy), which orphans saved queries and inflates the merge diff for no semantic gain.Evidence
Scope
Markdown only — no code or CLI changes.
tools/skillgen/fragments/references/shared/update.md(the gate) andtools/skillgen/fragments/core/core.md(a pointer from the--updatesection).python -m tools.skillgen+--bless.All five skillgen guards pass locally:
--check,--audit-coverage,--schema-singleton,--monolith-roundtrip,--always-on-roundtrip.Note on CI
tests/test_labeling.py::test_label_communities_batches_when_over_batch_sizemay show red. It is pre-existing and unrelated to this PR — it reproduces on an unmodified checkout with:Cause:
label_communitiesruns batches in a thread pool (graphify/llm.py,workers = max(1, min(max_concurrency, n_batches))), and forbackend="gemini"concurrency stays > 1. The test appends tocallsfrom inside the worker, so it records completion order but asserts dispatch order ([100, 100, 50]); under a different thread schedule it observes[100, 50, 100]. It passes when its file runs alone, which is why it usually goes unnoticed.Happy to split that into its own issue/PR if useful — I left it untouched here to keep this diff to instructions only.