-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
docs(skills): record the merge and agent failure modes seen in the field - #3340
docs(skills): record the merge and agent failure modes seen in the field #3340rigoita wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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
Adds a "Chunking and agent-reliability lessons" section to every agent's extraction-spec.md reference, prescribing byte-based chunk packing (~250 KB markup, ~110 KB prose), splitting oversized files by line range, early-and-refine JSON writes, and disk validation that ignores failed statuses and checks hyperedge members. Adds a "Failure modes seen in the field" section to each update.md, documenting why prune_sources silently drops nodes (relative-path matching, prune-after-insert), how merges overwrite the two copies of hyperedges, dangling-member repair, absolute/backslash path normalization from extract(), and preferring targeted updates over full re-extraction. Regenerates the corresponding skillgen expected fixtures to match.
No blocking issues surfaced. 29 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 443 functions depend on the 443 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 443 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: 443 function(s) in the blast radius were not formally verified this run
Five behaviours cost real debugging time running
--updateagainst a ~4000-node, ~106-file graph. None of them errors — each fails silently and leaves a plausible-looking graph, which is what makes them expensive to find.prune_sourcesdoes not do what it reads like. It matches paths as the graph stores them (relative), so an absolute path prunes nothing; andbuild_mergeprunes after inserting, so naming a source you are simultaneously re-adding deletes the new nodes too. Stripping the source by hand and merging with noprune_sourcesis what works.graph.jsoncarries them at top level and undergraph; filtering one copy makes the merge keep only the incoming batch — 36 disappeared in one run with no error and no warning.extract()returns absolute, platform-separatorsource_filevalues while the graph stores relative forward-slash ones, so merging raw AST creates a duplicate source per file.failedhaving already written a valid chunk, so the completion status is not a reliable signal of output.Docs only, no behaviour change.
Where it was applied
The fragment sources, not the generated skill files:
The compact variant gets a deliberately shortened version — it exists to be compact.
Regenerated with
python -m tools.skillgenand--bless, so the change is 3 fragments + 28 generated artifacts + 28expected/snapshots.pytest tests/test_skillgen.pypasses 65/65.A note on verification
Developed on Windows, where 29 tests fail on a clean
v8checkout for environment reasons (os.mkfifo,socket.AF_UNIX, symlink privilege,charmapcodec, WSL/bin/bash). I confirmed those are pre-existing by stashing the change and re-running, so I verified against the targetedtests/test_skillgen.pysuite rather than claiming a clean full run.Happy to soften the specific numbers to qualitative statements if you would rather the docs not carry another project's telemetry.