Bundle plain-writing and comment-cleanup into address-pr-reviews - #216
Merged
Merged
Conversation
A cloud agent run unpacks one skill folder into a sandbox with no clone of this repo, so the plain-writing pass in Step 3 and the comment-cleanup pass in Step 5 had no skill to invoke there. PostHog Desktop uploads a referenced skill as a dependency, but it refuses when that skill is a symlink the user did not select, and the installer symlinks every skill. address-pr-reviews carries a copy of both skills under references/ and reads the copy rather than invoking the skill, so each pass runs the same way on every harness. Each copy keeps its source skill's folder shape, so the relative paths written inside it resolve. Portable-skill table destinations are relative to a skill's own directory, which puts a vendored skill under the same sync and staleness check as a vendored helper. The undeclared-file walk covers the whole skill folder and skips SKILL.md. Claude-Session: https://claude.ai/code/session_01X4SWXXerzsNdeZ7eWjB7Mw
The sync's other passes all start from the table, so a file added to a vendored skill reached no copy while CI stayed green, and the sandbox followed a link into a file that never travelled. A fourth pass walks each vendored skill's source folder and reports a file no row copies. It finds that folder by mirroring the destination, so it checks the mirror first: a row that breaks it would otherwise send the walk to a directory that is not there, where it reads nothing and reports nothing. PORTABLE_SKILL_OWN_FILES declares SKILL.md, which drops the hardcoded exception from the undeclared-file walk and makes the walk's own error message the whole rule. comment-cleanup names the rules in its own steps as the standard, and reads the Style section of AGENTS.md as well when the run has one. A copy of the skill in a sandbox has no AGENTS.md, so the standard has to travel with the rules. The two undeclared-file scenarios in the sync test share one function, and the first assertion in that file prints the output it rejected. Claude-Session: https://claude.ai/code/session_01X4SWXXerzsNdeZ7eWjB7Mw
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The only remaining finding is a minor diagnostic-wording nit, with no approval-blocking issues.
Review effort: Lite
Findings: None
What changed in this PR
This PR makes address-pr-reviews portable by bundling the plain-writing and comment-cleanup skills.
Changes:
- Vendors both skills and their supporting files.
- Strengthens portable-skill synchronization and drift checks.
- Expands documentation and test coverage.
| File | Summary |
|---|---|
ai/tests/test-portable-sync.sh |
Tests orphan, mirror, and source checks. |
ai/skills/comment-cleanup/SKILL.md |
Makes cleanup rules self-contained. |
ai/skills/address-pr-reviews/SKILL.md |
Uses bundled prose and cleanup rules. |
ai/skills/address-pr-reviews/scripts/tests/test-portable-skill.sh |
Tests bundled lint execution. |
ai/skills/address-pr-reviews/references/plain-writing/SKILL.md |
Bundles plain-writing rules. |
ai/skills/address-pr-reviews/references/plain-writing/scripts/plain-writing-lint.py |
Bundles the prose linter. |
ai/skills/address-pr-reviews/references/plain-writing/references/voice-match.md |
Bundles voice-matching guidance. |
ai/skills/address-pr-reviews/references/plain-writing/references/strict.md |
Bundles strict-writing guidance. |
ai/skills/address-pr-reviews/references/comment-cleanup/SKILL.md |
Bundles comment-cleanup rules. |
ai/README.md |
Documents portability and synchronization conventions. |
ai/helpers/portable-skills.sh |
Defines vendored skill paths; contains a minor diagnostic-wording nit. |
ai/bin/sync-portable-skills.sh |
Syncs copies and validates source completeness. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
A cloud agent run unpacks one skill folder into a sandbox with no clone of this repo, so the
plain-writingpass in Step 3 and thecomment-cleanuppass in Step 5 had no skill to invoke and were skipped. PostHog Desktop can upload a referenced skill as a dependency, but it refuses when that skill is a symlink the user did not select, andai/install-claude.shsymlinks every skill into~/.claude/skills. Naming those two skills without a leading slash is what kept the upload working, and it is also what kept them out of the bundle.address-pr-reviewsnow carries a copy of both skills underreferences/and reads the copy rather than invoking the skill, so each pass runs by the same rules on every harness. Each copy keeps its source skill's folder shape, so the relative paths written inside it resolve, including theplain-writingcopy's ownscripts/plain-writing-lint.py.Portable-skill table destinations are relative to a skill's own directory instead of its
scripts/directory, which puts a vendored skill under the same sync and staleness check as a vendored helper.PORTABLE_SKILL_OWN_FILESdeclaresSKILL.md, and the undeclared-file walk covers the whole skill folder.A fourth sync pass reports a file added to a vendored skill that no table row copies. Every other check starts from the table, so without it the bundle falls behind its source with CI green. That pass finds each source folder by mirroring the destination, so it checks the mirror first: a row writing
references/<name>/<rel>has to read fromai/skills/<name>/<rel>.comment-cleanupnamed the Style section ofAGENTS.mdas its standard in its preamble, which Step 5 reaches on every run and which a sandbox does not have. It now names the rules in its own steps as the standard and reads that section as well when the run has one.One thing a copy still names outside the bundle: the
comment-cleanupcopy carries a$HOME/.dotfiles/path that only--branchreaches, and this caller always passes explicit file paths.Test plan
All 18 tests in the
ai/README.mdlist pass. Each new assertion is mutation-tested: scoping the walk back toscripts/fails 3, removing the source-walk exclusions fails 5, deleting the fourth pass fails 2, and removing the mirror check fails 2.For the sandbox itself, copy
ai/skills/address-pr-reviewsalone to a temp directory and runpython3 references/plain-writing/scripts/plain-writing-lint.pyunderenv -i. That is what the upload gets.https://claude.ai/code/session_01X4SWXXerzsNdeZ7eWjB7Mw