fix(installer): stop copying __pycache__ into IDE skill trees - #2695
Open
tjoignant wants to merge 1 commit into
Open
fix(installer): stop copying __pycache__ into IDE skill trees#2695tjoignant wants to merge 1 commit into
tjoignant wants to merge 1 commit into
Conversation
The skill copy filtered OS and editor artifacts but not Python bytecode
caches, so the five __pycache__/*.pyc files carried in the published
tarball landed in every user's skill tree. Their sha256 is identical from
package to disk, and they are cpython-311 regardless of the local
interpreter, so they are the maintainer's build output rather than
anything generated locally.
_installSharedScripts already states the intended rule for _bmad/scripts
("dev-only tests and Python caches must not land in user projects") and
filters on it. This applies the same rule to the skill copy.
Hoists the predicate to a testable shouldSkipSkillArtifact() and adds
Test Suite 53, which asserts end to end that a cache planted in the
source tree does not reach the installed skill while helper.py does.
Refs bmad-code-org#2694
Greptile SummaryThe PR prevents Python bytecode caches from being copied into IDE skill trees while preserving the existing OS, editor-artifact, and dotfile filtering behavior.
Confidence Score: 4/5The installer change appears safe to merge; the only identified issue is non-blocking test-fixture cleanup on exceptional paths. The artifact predicate preserves the previous filtering behavior and correctly blocks Python cache entries, while the new test can leave temporary directories behind if setup or filesystem operations throw. Files Needing Attention: test/test-installation-components.js
|
| Filename | Overview |
|---|---|
| tools/installer/ide/_config-driven.js | Correctly centralizes the existing artifact filter and extends it to exclude Python bytecode caches without changing root-copy semantics. |
| test/test-installation-components.js | Adds meaningful direct and end-to-end coverage, but exceptional paths can leak both newly allocated temporary directory trees. |
Prompt To Fix All With AI
### Issue 1
test/test-installation-components.js:4078-4079
**Cleanup skips exceptional paths**
The new test removes its two temporary directory trees only at the end of the `try` block. When an awaited filesystem or installer operation throws, control jumps directly to `catch`, leaving the temporary fixtures on disk to accumulate across repeated local or CI runs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(installer): stop copying __pycache__..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4078
to
+4079
| await fs.remove(tempProjectDir53); | ||
| await fs.remove(path.dirname(installedBmadDir53)); |
There was a problem hiding this comment.
The new test removes its two temporary directory trees only at the end of the try block. When an awaited filesystem or installer operation throws, control jumps directly to catch, leaving the temporary fixtures on disk to accumulate across repeated local or CI runs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/test-installation-components.js
Line: 4078-4079
Comment:
**Cleanup skips exceptional paths**
The new test removes its two temporary directory trees only at the end of the `try` block. When an awaited filesystem or installer operation throws, control jumps directly to `catch`, leaving the temporary fixtures on disk to accumulate across repeated local or CI runs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
📝 WalkthroughWalkthroughChangesSkill artifact filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test-installation-components.js (1)
4048-4053: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover
.pyoand suffix-filter integration cases.The predicate includes
.pyo, but the unit assertions cover only.pyc. The end-to-end fixture places the.pyconly inside__pycache__, so it does not verify suffix filtering duringfs.copy. Add root-level.pycand.pyofixtures, then assert both are excluded whilehelper.pyremains installed.Proposed test additions
assert(shouldSkipSkillArtifact('sprint_plan.cpython-311.pyc') === true, '.pyc files are treated as artifacts'); + assert(shouldSkipSkillArtifact('sprint_plan.pyo') === true, '.pyo files are treated as artifacts'); ... await fs.writeFile(path.join(sourceScripts53, 'helper.py'), 'print("hello")\n'); + await fs.writeFile(path.join(sourceScripts53, 'helper.pyc'), 'bytecode'); + await fs.writeFile(path.join(sourceScripts53, 'helper.pyo'), 'bytecode'); ... assert(await fs.pathExists(path.join(installedSkill53, 'scripts', 'helper.py')), 'Python sources are installed'); + assert(!(await fs.pathExists(path.join(installedSkill53, 'scripts', 'helper.pyc'))), '.pyc files are excluded'); + assert(!(await fs.pathExists(path.join(installedSkill53, 'scripts', 'helper.pyo'))), '.pyo files are excluded');Also applies to: 4055-4076
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test-installation-components.js` around lines 4048 - 4053, Extend the installation fixture and assertions around shouldSkipSkillArtifact to cover root-level .pyc and .pyo files, verifying both are excluded during fs.copy. Keep the existing __pycache__ coverage and assert helper.py remains installed to confirm ordinary Python sources are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 4048-4053: Extend the installation fixture and assertions around
shouldSkipSkillArtifact to cover root-level .pyc and .pyo files, verifying both
are excluded during fs.copy. Keep the existing __pycache__ coverage and assert
helper.py remains installed to confirm ordinary Python sources are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fd2687b7-77a6-4262-acdc-4baedcf86d07
📒 Files selected for processing (2)
test/test-installation-components.jstools/installer/ide/_config-driven.js
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
The IDE skill copy now filters
__pycache__directories and.pyc/.pyofiles, so Python bytecode shipped in the npm tarball no longer lands in users' skill trees.Why
Fixes #2694.
Installing today writes the maintainer's build artifacts into every project:
They are genuinely shipped rather than locally generated — the sha256 is identical from the published package through
files-manifest.csvto the installed file, and the bytecode iscpython-311even when the local interpreter is 3.14.The installer already declares this must not happen.
Installer._installSharedScriptsfilters the same artifacts for_bmad/scriptsunder the comment "Ship only the runtime scripts — dev-only tests and Python caches must not land in user projects" — which is why_bmad/scripts/is clean while.claude/skills/**/scripts/is not. This applies one rule in both places.How
installSkillsFromManifestinto a module-levelshouldSkipSkillArtifact(name)intools/installer/ide/_config-driven.js, exported so it is unit-testable.__pycache__to the skipped names and.pyc/.pyoto the skipped suffixes. Existing behaviour is unchanged: OS artifacts, dotfiles (with the.gitkeepexemption), and editor backups filter exactly as before.tests/directories alone._installSharedScriptsdrops them for_bmad/scripts, but they are tracked in the skill manifest today, so removing them is a separate call for maintainers rather than something to slip into this fix.Testing
node test/test-installation-components.js— 465 passed, 0 failed (was 455).New Test Suite 53 covers the predicate directly and adds an end-to-end case: it plants
scripts/__pycache__/helper.cpython-311.pycalongsidescripts/helper.pyin the source fixture, runs a realclaude-codeinstall, and asserts the.pyarrives while__pycache__does not.I verified the test is not vacuous by reverting only the two filter entries on the fixed branch — the end-to-end assertion fails, and passes again once restored.
npx prettier --checkandnpx eslint --max-warnings=0are clean on both changed files.