fix: publish a working install command and stop the version drifting - #35
Merged
Merged
Conversation
The command shipped in 3.0.0 does not run. `npx @pymodel/claude-agy-mcp-install-skills` reads as a package name to npx and 404s for everyone, and the form in the script header, `npx @pymodel/claude-agy-mcp install-skills`, starts the stdio server with a stray argument and hangs. The bin is a package selection, so the working form names the package and the bin separately. The handshake version was a constant restated from package.json, guarded only by a test. That guard fired in CI one commit after the last bump, which is the whole class working as designed and also a standing tax. It is now read from package.json at module init. `../package.json` resolves to the package root from both src/ and the bundled dist/, verified by installing the tarball and reading the version back out of the running server, and npm ships package.json regardless of the files array. The test keeps the assertion and adds the constraint that actually matters now: the bin entry stays one directory below the package root. The installer replaced a skill directory outright, including a symlink. Anyone pointing a skill at a checkout they edit would have had that link silently swapped for a frozen copy on the next install. It now skips a symlinked destination and says so, and --force replaces it deliberately. The concurrency test drained its semaphore on a single macrotask tick, which is not enough under load: the gate would still be empty, the shift would be a no-op, and the assertion would measure an empty semaphore rather than a full one. It now waits for each admitted run to reach the spawn.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughThe package release updates skill installation through ChangesPackage maintenance
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The updated installer behavior, version reporting, and concurrency test synchronization are covered without an actionable regression. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
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.
Four standardization fixes on top of v3.0.0. The first one blocks anyone from installing the skills at all.
The install command in v3.0.0 does not run
npx @pymodel/claude-agy-mcp-install-skillsreads as a package name to npx, so it 404s for every user. The form in the script header,npx @pymodel/claude-agy-mcp install-skills, starts the stdio server with a stray argument and hangs.The bin is a package selection, not a subcommand, so the working form names both:
Fixed in the README and the script header. The v3.0.0 release notes need the same correction.
The version was restated, not derived
VERSIONwas a constant copied from package.json, guarded by a test. That guard fired in CI one commit after the last bump. It now reads package.json at module init:../package.jsonresolves to the package root from bothsrc/and the bundleddist/, and npm ships package.json regardless of thefilesarray.Verified against the real tarball rather than the source tree. Installing
pymodel-claude-agy-mcp-3.0.1.tgzinto a temp project and driving aninitializeover stdio returns"version":"3.0.1"from the bundled entry.The test keeps the equality assertion and adds the constraint that actually carries the risk now: the bin entry stays one directory below the package root, since that is what the relative read depends on.
The installer would clobber a symlink
It replaced the destination outright. Someone who points a skill at a checkout they edit would have had that link silently swapped for a frozen copy on the next install, which is the same silent-clobber class this project's own release notes warn about. It now skips a symlinked destination and says so, with
--forceto replace deliberately. Two tests drive the real script against temp directories.The concurrency test was timing-dependent
It drained its semaphore on a single macrotask tick. Under load the gate is still empty, the shift is a no-op, and the assertion measures an empty semaphore rather than a full one. It surfaced as a flake once the new subprocess tests started competing for the machine. It now waits for each admitted run to reach the spawn.
Verification
288 tests pass, 1 skipped. Typecheck, build and format clean. Three consecutive full runs green after the concurrency fix. Tarball contents confirmed: both skill trees, the installer at mode 755, and
--list,--dirand the symlink guard all exercised from an installed copy rather than the repo.Summary by CodeRabbit
New Features
--forceoption to replace symlinked skills during installation.npx --packagecommand.Bug Fixes
Documentation
--forceoption.