Skip to content

fix(skills): stamp manifest before build merge - #2896

Open
hopstreax wants to merge 1 commit into
Graphify-Labs:v8from
hopstreax:fix/2865-manifest-stamp-order
Open

fix(skills): stamp manifest before build merge#2896
hopstreax wants to merge 1 commit into
Graphify-Labs:v8from
hopstreax:fix/2865-manifest-stamp-order

Conversation

@hopstreax

Copy link
Copy Markdown
Contributor

Summary

Fixes the --update skill runbook so manifest stamping happens before
build_merge() mutates the extraction in place.

Previously, the runbook passed new_extraction to build_merge() and only
then used the mutated extraction to determine which semantic files should be
stamped in the manifest.

The CLI extraction path already performs this calculation before the merge.

Root cause

build_merge() mutates the extraction dictionaries in place during graph
construction, including normalizing legacy extraction fields such as
path / source into source_file.

_stamped_manifest_files() uses the extraction to determine which semantic
files actually produced output during the run. Reading the extraction after
build_merge() can therefore produce a different result from the raw
extraction.

Fix

Move _stamped_manifest_files() and the related manifest preparation before
build_merge() in the shared --update runbook template.

The generated skill runbooks and expected snapshots were regenerated from
the shared template.

Tests

Added a regression test covering:

  • legacy pathsource_file mutation during build_merge()
  • different manifest-stamping results before vs. after the mutation
  • correct ordering of manifest stamping before build_merge() across the
    generated update runbooks

Validation:

  • 65 passedtests/test_skillgen.py
  • 84 passedtests/test_build.py
  • 21 passed, 1 skipped — build/merge regression tests
  • python -m tools.skillgen --check — 134 artifacts verified
  • git diff --check — passed

@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

Moves the semantic-file stamping/clearing block (_stamped_manifest_files, _cleared, _scan) to run before build_merge() in every host's references/update.md, since build_merge mutates new_extraction in place and would corrupt the computed stamps. Hoists the _stamped_manifest_files import to the top and updates the comment to cite #2865. Regenerates the corresponding skillgen expected fixtures and rationale test data.

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

Analysis details — impact, health, verification

Impact & health

Graphify review

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

Health — this change adds coupling hotspots:

  • new: test_audit_catches_a_dropped_non_allowlisted_heading() — 0 callers, 6 callees

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

· 1 more finding(s) on lines outside this diff (see the check run).

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.

1 participant