fix(upgrade): loop-* skill copy doubles the skills/ path segment - #63
Merged
codejunkie99 merged 1 commit intoAug 6, 2026
Conversation
_new_loop_assets() computed each new loop-* skill's destination as dst_skills / src.relative_to(src_agent) -- but src.relative_to(src_agent) already starts with "skills/" (src_agent is .agent/, not .agent/skills/), and dst_skills is already .agent/skills/. Result: every genuinely-new loop-* skill lands at .agent/skills/skills/loop-x/SKILL.md instead of .agent/skills/loop-x/SKILL.md -- both the CLI's own --dry-run report and the real copy were wrong, silently, since no existing test exercised a fresh (non-pre-seeded) loop-* skill destination. Fix: dst_agent / src.relative_to(src_agent), matching the sibling non-loop skill-copy block earlier in the same function (line ~84), which already gets this right. New test: test_upgrade_copies_missing_loop_skills_to_the_correct_path. Confirmed it fails on the old code (asserts the doubled path is absent) and passes on the fix.
This was referenced Aug 6, 2026
Contributor
Author
|
Related, independent PRs — no dependency on this one:
This PR (the cc @codejunkie99 — happy to answer questions on any of these, or squash/reorder however's easiest to review. |
codejunkie99
pushed a commit
that referenced
this pull request
Aug 6, 2026
Patch release covering #60, #61, #62, #63: superseded-lesson filtering in recall, the doubled skills/ path in upgrade's loop-skill copy, UTF-8 I/O in learn.py, and a leaked file handle. Collapses the stacked per-version README history (v0.9.0 through v0.18.0) into a single pointer at CHANGELOG.md, and relaxes the onboarding docs test from an exact version pin to the 0.19.x series so patch releases keep asserting the loop docs and sandbox caveat without editing the assertion each time.
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.
Summary
_new_loop_assets()inharness_manager/upgrade.pycomputes each newloop-*skill's destination asdst_skills / src.relative_to(src_agent).src.relative_to(src_agent)already starts withskills/(src_agentis.agent/, not.agent/skills/), anddst_skillsis already.agent/skills/— so every genuinely-newloop-*skill lands at.agent/skills/skills/loop-x/SKILL.mdinstead of.agent/skills/loop-x/SKILL.md. Bothupgrade --dry-run's own report and the real file copy are wrong.This went unnoticed because the existing test coverage in
tests/test_loop_integrations.pyonly ever pre-seedsloop-triageat the destination before callingupgrade()— so it exercises the "already exists, skip" branch and never the fresh-copy branch that actually builds the destination path.Fix
dst_agent / src.relative_to(src_agent), matching the sibling non-loop skill-copy block earlier in the same function (_plan(), ~15 lines above), which already gets this right.Tests
New:
test_upgrade_copies_missing_loop_skills_to_the_correct_path— callsupgrade()against a fresh target with none of theloop-*skills pre-seeded, and asserts each lands at the correct (non-doubled) path.Confirmed via
git stashthat this test fails on the old code (asserts the doubled path is absent) and passes on the fix.Note
Fix duplicate
skills/path segment when copyingloop-*skill files during upgradeIn
_new_loop_assets, the destination path forloop-*skill copy actions was built usingdst_skills / src.relative_to(src_agent), which produced.agent/skills/skills/loop-*/.... The fix usesdst_agent / src.relative_to(src_agent)instead, targeting the correct.agent/skills/loop-*/...path. A new integration test in test_loop_integrations.py asserts the correct destination and confirms no.agent/skills/skillsdirectory is created.Macroscope summarized 312f132.