fix(skill-creator): audit_skill_regression compare can't survive a rename - #293
Merged
Conversation
…name The migration-gate identity check hashed --after's absolute path against the path recorded at snapshot time (or, for git-ref origin, looked up --after's relative path inside the historical ref). Both are the same path by construction only when the skill wasn't renamed or moved — so any legitimate skill rename hit a hard identity-mismatch error, with no way to proceed short of hand-editing the provenance manifest (which the tool's own docs explicitly warn against elsewhere). Add an explicit --renamed-from <old-path> flag to `compare`. When given, the identity check verifies that declared path instead of --after, for both baseline-origin modes; the content/tree-hash checks are untouched, so an undeclared mismatch (or a renamed-from pointing at the wrong old path) still fails exactly as before. Same shape as the existing --allow-identical-baseline escape hatch: narrow, explicit, opt-in. Found while renaming transcript-research-brief -> fact-check-pro in a private sibling skill repo today; git-diff verification stood in as the regression check for that rename since this tool structurally couldn't run. Documented the flag in the migration-gate section of SKILL.md so the next rename doesn't rediscover this the same way. Tests: 3 new cases (pre-edit-snapshot positive/negative, git-ref positive) alongside the existing 35, all green; plus an end-to-end CLI smoke test reproducing the original failure and confirming the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DcR3tHN3tKMy9WnYRJJZKk
…-from
An independent, fresh-context read-only reviewer (general-purpose subagent,
not a fork — per CLAUDE.md's "自审不算数" discipline for compounding skill
edits) was given the reader spec "another agent about to run the migration
gate for a skill rename, working only from SKILL.md" and told to actually
exercise the previous commit's changes against real corpus (today's real
transcript-research-brief -> fact-check-pro rename), not synthetic fixtures.
It surfaced two real, reproduced defects and two documentation gaps:
1. --renamed-from's docs said "old relative path" without saying relative to
what. It's resolved via Path.resolve() against the process's CWD — same
as --before/--after — but unlike those two, a wrong renamed_from has no
existence check to catch a bad resolution, so it fails deep and
confusingly instead. This bites skill-creator's own edits specifically:
the migration-gate walkthrough's own `cd <skill-creator-path>` convention
puts CWD in skill-creator's own repo, which is normally a *different*
repo than the skill being edited — reproduced live, exact repro in the
independent-review artifact.
2. The git-ref branch's single broad except block reported "requires the
edited skill to be inside a Git worktree and the ref to resolve" for
THREE distinguishable failures: --after genuinely not in a worktree,
renamed_from/after resolving outside that worktree (the actual failure
in defect 1), and the ref not resolving. Split into three narrower
try/except blocks, each with an accurate, specific message; the
renamed_from-outside-repo case now names both paths and states the CWD-
relative-resolution cause directly.
3. The pre-edit-snapshot hint only fired when renamed_from was omitted
entirely; a *wrong* renamed_from value produced the same bare error as no
flag at all, giving no way to tell the two apart. Hint now always shows
the resolved path when renamed_from was given.
4. SKILL.md's migration-gate walkthrough said "materialize the directory
from the chosen ref" with no command — the only place git archive |
tar -x -C actually appears is under an unrelated heading ("Concurrent
sessions on the same skill repo"), several hundred lines away. Its
"Extract into a fresh, non-existent directory" phrasing, read literally,
contradicts `tar -x -C` requiring the target to already exist (reproduced
on this machine's bsdtar: "could not chdir"). And git archive's output
directory structure is one level deeper than snapshot's, which no text
stated — a --before value copied from the snapshot recipe by pattern-
match points at the wrong depth. Inlined the actual command, clarified
the extraction-target wording, and stated the depth difference and its
--before consequence explicitly.
All fixes verified against the same live corpus that surfaced them (the real
fact-check-pro rename, git-archive-reconstructed from its actual pre-rename
commit) plus new regression tests: 39/39 passing (up from 38), including 2
new cases locking in the specific error messages so they can't silently
regress back to the generic ones. Full independent-review record — reviewer
prompt verbatim, every finding with disposition and reason, what wasn't
verified (the pre-edit-snapshot branch's equivalent CWD-mismatch case was
read but not exercised against real corpus, since the real snapshot this
session took earlier had already been cleaned from scratch space by the
time of review) — committed separately to the private skill-reviews archive,
not this public repo.
Existing-skill migration gate re-run against the same immutable pre-edit ref
(b8556f2) after this round's edits: 3
candidates (1 SKILL.md paragraph, the 2 files from the first round),
all preserved_or_moved with verified reasons, 0 unclassified, verify passed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DcR3tHN3tKMy9WnYRJJZKk
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
1 participant
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.
What broke
The existing-skill migration gate's
comparecommand verifies identity between--before(the pre-edit snapshot) and--after(the edited skill) beforeproducing a regression review. For
--baseline-origin pre-edit-snapshot, ithashes
--after's absolute path and compares it against the path recordedwhen
snapshot --sourcewas run. For--baseline-origin git-ref:<ref>, itlooks up
--after's relative path inside the historical git tree.Both checks assume the skill's path is unchanged between snapshot-time and
compare-time. That assumption breaks for the one operation this exact tool's
own SKILL.md documents extensively elsewhere: renaming a skill. Any
legitimate rename hits a hard
pre-edit snapshot source identity does not match the edited skill(or an equivalent git-ref lookup failure), with nodocumented way to proceed short of hand-editing the provenance manifest —
which the tool's own docs explicitly warn against (
.skill-regression-*markers are described as things you should never hand-edit).
The fix
Add an explicit
--renamed-from <old-path>flag tocompare. When given, theidentity check verifies that declared path instead of
--after's currentpath/relative-path — for both baseline-origin modes. The content/tree-hash
checks (
tree_hash(before)vs the manifest, andbefore's tree vs thehistorical git ref) are untouched, so an undeclared path change, or a
--renamed-frompointing at the wrong old path, still fails exactly asbefore. Same design as the existing
--allow-identical-baselineflag: anarrow, explicit, opt-in override for one named legitimate scenario, not a
general relaxation.
The provenance recorded in the report now includes
renamed_fromwhen theflag is used, so a reviewer can see a rename was declared and what the old
path was.
Also documented the flag in SKILL.md's migration-gate walkthrough, right next
to the existing
compareexample, so the next rename doesn't rediscover thisthe same way.
How this was found
Renaming a skill (
transcript-research-brief→fact-check-pro) in aprivate sibling skill repo today, following this exact skill's own
"existing-skill migration gate" procedure.
snapshotsucceeded;comparefailed with the identity error above. Read the source
(
_resolve_baseline_provenance, lines ~218–256 before this change) andconfirmed the root cause is the absolute-path hash. Verified via
git diffthat the rename itself preserved 100% of content (git's rename detection
made this exact and complete) and used that as the regression check for that
specific rename, since this tool structurally couldn't run.
Testing
tests/test_audit_skill_regression.pystill passunchanged (ran before and after the fix).
test_pre_edit_snapshot_rejects_undeclared_rename— confirms thedefault (no flag) behavior is unchanged: an undeclared rename still
fails with the same error as before.
test_pre_edit_snapshot_accepts_declared_rename— confirms a declaredrename succeeds, the report records
renamed_from, and a--renamed-frompointing at the wrong old path still fails (theoverride doesn't degrade into "skip identity checking").
test_git_ref_baseline_accepts_declared_rename— same shape for thegit-ref:baseline-origin mode, including an undeclared-rename negativecase.
the fix was written in).
directory →
comparewithout the flag (reproduces the original failure,now with an improved error message pointing at
--renamed-from) →comparewith the flag (succeeds,report.jsonprovenance showsrenamed_from).Scope
Touches only
daymade-skill/skill-creator/{SKILL.md, scripts/audit_skill_regression.py, tests/test_audit_skill_regression.py}. No other files. No new dependencies, no new capability beyond making an already-documented, already-supported operation (skill rename) actually work with the migration-gate tooling.🤖 Generated with Claude Code
https://claude.ai/code/session_01DcR3tHN3tKMy9WnYRJJZKk