Make address-pr-reviews run from a copied skill folder - #214
Merged
Merged
Conversation
A cloud agent run uploads one skill folder into a sandbox with no clone of this repo. address-pr-reviews now carries every helper it calls under its own scripts/ and names no path outside that folder. Its SKILL.md also names no other skill with a leading slash, which PostHog Desktop reads as a dependency and refuses to upload when that skill is a symlink the user did not select. ai/helpers/portable-skills.sh holds the table of portable skills and the helpers each one carries. sync-portable-skills.sh writes the copies from it. CI rejects a stale copy, a SKILL.md that reaches outside its folder, and a skill folder that fails to run with an empty HOME. record-step.sh keeps the step record machine-local. The record only feeds the ran report and go's resume, which read state under $HOME, so a sandbox skips it and a clone that has lost the helper still fails loudly. Claude-Session: https://claude.ai/code/session_017t1JMgHA9F5jDiaoVbRdhq
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/safety/documentation issues in newly added enforcement/utilities (notably the frontmatter scan behavior and some mismatched contracts/comments) that should be addressed before merging.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes selected AI skills “portable” so they can run when only the skill folder is copied into a cloud sandbox (no ~/.dotfiles clone) and so PostHog Desktop doesn’t reject uploads due to /other-skill dependency-style references in SKILL.md.
Changes:
- Introduces a “portable skills” mechanism (table + sync script) that vendors required helpers into each skill’s
scripts/folder and enforces parity in CI. - Updates
address-pr-reviews(andwait-for-pr-reviewsdocs) to avoid external skill references and absolute~/.dotfiles/...paths. - Adds CI and hermetic tests to prevent regressions (SKILL.md lint + per-skill copied-folder execution tests).
File summaries
| File | Description |
|---|---|
| ai/tests/test-portable-skills.sh | New lint/test ensuring portable SKILL.md files don’t reference ~/.dotfiles/, /other-skill, wiki-links, or frontmatter dependencies. |
| ai/skills/wait-for-pr-reviews/SKILL.md | Removes slash-style skill references in prose to avoid PostHog Desktop dependency parsing. |
| ai/skills/address-pr-reviews/SKILL.md | Updates docs/commands to use vendored scripts/ helpers and clarifies what is/ isn’t bundled in a sandbox. |
| ai/skills/address-pr-reviews/scripts/tests/test-portable-skill.sh | New hermetic test that runs the copied skill under env -i with empty HOME and missing DOTFILES_DIR. |
| ai/skills/address-pr-reviews/scripts/record-step.sh | Adds a portable wrapper that records step completion only when a dotfiles clone is present. |
| ai/skills/address-pr-reviews/scripts/record-dismissed-comment.sh | Switches sourcing from ${DOTFILES_DIR} to vendored scripts/lib/* for sandbox portability. |
| ai/skills/address-pr-reviews/scripts/lib/logging.sh | Vendors shared logging utilities (plus heartbeat) into the portable skill. |
| ai/skills/address-pr-reviews/scripts/lib/github.sh | Vendors shared GitHub helper functions into the portable skill. |
| ai/skills/address-pr-reviews/scripts/lib/fs.sh | Vendors atomic write helper used by portable scripts. |
| ai/skills/address-pr-reviews/scripts/lib/copilot.sh | Vendors Copilot/GitHub review-comment helper library into the portable skill. |
| ai/skills/address-pr-reviews/scripts/helpers/pending-reviews.jq | Vendors the jq verdict program needed for pending-review detection. |
| ai/skills/address-pr-reviews/scripts/git-pr | Vendors PR URL resolution helper into the portable skill. |
| ai/skills/address-pr-reviews/scripts/gh-resolve-threads | Vendors thread listing/resolution CLI into the portable skill. |
| ai/skills/address-pr-reviews/scripts/fetch-unaddressed-comments.sh | Switches sourcing to vendored libraries for sandbox portability. |
| ai/skills/address-pr-reviews/scripts/detect-pr.sh | Vendors PR detection logic into the portable skill (used by SKILL.md instructions). |
| ai/skills/address-pr-reviews/scripts/check-pending-reviews.sh | Vendors pending-review detection wrapper into the portable skill. |
| ai/README.md | Renames/rewrites docs from “bundled helpers” to “portable skills” and documents enforcement. |
| ai/helpers/portable-skills.sh | New canonical table of portable skills and which helper sources are vendored where. |
| ai/bin/sync-pr-review-helpers.sh | Removes the old wait-for-pr-reviews-only helper sync script. |
| ai/bin/sync-portable-skills.sh | New sync/check script that copies helpers listed in the portable table into skill folders. |
| ai/AGENTS.md | Documents the stricter portability rules for skills listed as portable. |
| .github/workflows/test.yml | Updates CI to enforce portable skills sync + lint + new address-pr-reviews portable test + dismissed-state test. |
Review details
- Files reviewed: 20/22 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
haacked
marked this pull request as ready for review
September 17, 2026 19:59
record-step.sh expands HOME through ${HOME:-}, so a sandbox that does not
export it reaches the documented skip instead of aborting under set -u.
The hermetic test's git mock answers the four calls git-pr makes to resolve
a PR from the current branch, so detect-pr.sh's no-argument path runs the
vendored copy. New assertions cover that path, an unset HOME, and a clone
that is present without its helper.
sync-portable-skills.sh --check names the source file to edit, which is the
file a developer must change for the sync to keep it.
SKILL.md names Steps 2 and 5 as the step record's call sites.
Claude-Session: https://claude.ai/code/session_017t1JMgHA9F5jDiaoVbRdhq
The lint and the sync script are what keep a skill folder uploadable, and neither ran its own reporting paths in CI. test-portable-skills-lint.sh drives the lint over fixture trees through PORTABLE_SKILLS_DIR, and test-portable-sync.sh drives the sync script over a copied tree. sync-portable-skills.sh reports a file under a skill's scripts/ that neither table names, which is what a renamed or deleted row leaves behind, and a plain run deletes it. PORTABLE_SKILL_OWN_FILES lists the scripts each skill owns, so a copy and a hand-maintained script are told apart. record-step.sh finds the clone from its own location, so a clone checked out anywhere records. pwd -P resolves the installer's symlink into the clone, and in a sandbox the same walk lands outside the unpacked folder and skips. portable-skill-sandbox.sh holds the sandbox setup, assert_run, and the tally both hermetic tests ran their own copies of. Each assert_run writes its own stdout and stderr files, so a check that reads one cannot be pointed at another command's output by an assertion inserted between them. ai/AGENTS.md carries a pointer to the portable skills section rather than the rule, which only resolves in this repo. The README test list is the CI list. Claude-Session: https://claude.ai/code/session_017t1JMgHA9F5jDiaoVbRdhq
The frontmatter check in test-portable-skills.sh read from line 2 to the first --- in any file, so a prose line reading "dependencies:" in a SKILL.md with no frontmatter reported as a declaration, and a file with no --- at all was read to its end. It now runs only on a file that opens with the marker. detect-pr.sh returns the same keys from every JSON path, so a consumer can read a field without testing which path produced the object. logging.sh passes the message through printf's %s, so an API error body carrying backslash escapes or terminal control sequences prints as text. github.sh describes what resolve_pr_target accepts: a PR URL or number, or no argument to ask git-pr for the current branch's PR. pending-reviews.jq names lib/copilot.sh in both the locations it is read from. Claude-Session: https://claude.ai/code/session_017t1JMgHA9F5jDiaoVbRdhq
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.
address-pr-reviewscould not run in a PostHog Desktop cloud session. Two separate things broke it.The upload never started. PostHog Desktop reads
/other-skillin a SKILL.md as a dependency. It refuses the whole upload when that skill is a symlink the user did not select, and every skill in this repo is installed as a symlink.address-pr-reviewsmentioned/wait-for-pr-reviews,/go, and/ranin prose, so it threw on the first one every time. Selecting the named skill by hand would not have helped, because the next reference throws next.Past that, the run would have died at Step 1. The skill called
~/.dotfiles/bin/detect-pr.shand three other helpers by absolute path, and its own scripts sourced${DOTFILES_DIR}/bin/lib/copilot.sh. A cloud sandbox clones the target repo, not this one.address-pr-reviewsnow carries the nine helpers it calls under its ownscripts/, and its SKILL.md names no path outside that folder and no other skill with a leading slash.wait-for-pr-reviewshad the same slash problem on one line, so it is fixed here too.ai/helpers/portable-skills.shis the table of which skills are portable and what each one carries.ai/bin/sync-portable-skills.sh, renamed fromsync-pr-review-helpers.sh, writes the copies from that table.The step record stays machine-local.
scripts/record-step.shdelegates toai/bin/log-step-done.shwhen the clone is present and skips when it is not, because the record only feeds theranreport andgo's resume, which read state under$HOME. A clone that is present but has lost the helper still fails loudly.What stops this regressing
sync-portable-skills.sh --checkrejects a stale, missing, or wrongly permissioned copy.ai/tests/test-portable-skills.shrejects a portable SKILL.md that names a~/.dotfiles/path, references another skill, or declares frontmatter dependencies. It reports every/nameother than the skill's own instead of checking against a list of this repo's skills, because Desktop also resolves names from~/.agents/skillsand from marketplace plugins.scripts/tests/test-portable-skill.shcopies the folder elsewhere and runs it underenv -iwith an emptyHOMEand a missingDOTFILES_DIR.bin/lib/test-dismissed-state.shalso joins CI. It covers the two scripts whose sourcing changed, and it was not running anywhere.Test plan
parse-skill-references.tsregex verbatim over both SKILL.md files. Before,address-pr-reviewsresolveswait-for-pr-reviews,go, andran, andwait-for-pr-reviewsresolvesgo. After, neither resolves anything external./wait-for-pr-reviews., a/review-codethat exists only in~/.agents/skills, two references one character apart, a wiki link, and a frontmatterdependencies:key. Each is caught.record-dismissed-comment.shto${DOTFILES_DIR}sourcing and confirmed the hermetic test fails on it.record-step.shthree ways. No clone skips with exit 0, a clone missing the helper exits non-zero, a working clone writes the record.Two known gaps.
--checkwalks the table rather than the skill folder, so a copy whose row was renamed stays behind unreported, which the script header records. The two hermetic tests duplicate about 70 lines of mock scaffolding, because a shared helper would have to live outside both skill folders.https://claude.ai/code/session_017t1JMgHA9F5jDiaoVbRdhq