Conversation
Remove all PolyPilot-specific references: - Replaced Checkout-GhAwPr.ps1 script reference with inline bash example that any repo can copy - Removed .claude/skills/ path references - Removed references/architecture.md dependency (link to official gh-aw docs instead) - Changed 'this repository' to generic language The skill can now be copied to any repository as-is. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Two findings:
-
🟡 MODERATE (Security): The inline bash checkout example uses
|| trueon thegit checkoutrestore step, which silently swallows failures for the critical.github/restoration — the very defense-in-depth mechanism the script exists to provide. Split into two commands so.github/restore is mandatory. -
🟢 MINOR (Bug): Orphan
## When to Read the Full Referenceheading left with no content above## Further Reading. Should be removed.
Generated by Expert Code Review (auto) for issue #770 · ● 5.4M
| gh pr checkout "$PR_NUMBER" | ||
| # Restore trusted .github/ from base branch | ||
| BASE_SHA=$(gh pr view "$PR_NUMBER" --json baseRefOid --jq '.baseRefOid') | ||
| git checkout "$BASE_SHA" -- .github/ .agents/ 2>/dev/null || true |
There was a problem hiding this comment.
🟡 MODERATE — blanket || true silently swallows .github/ restore failures
The whole point of this defense-in-depth step is to restore trusted .github/ from the base branch. Combining both paths in one git checkout with 2>/dev/null || true means that if .github/ fails to restore (bad SHA, transient git error, path missing), the step silently succeeds and the agent proceeds with potentially tampered agent infrastructure.
Failing scenario: A transient error or corrupted local clone causes git checkout "$BASE_SHA" -- .github/ .agents/ to fail. The || true swallows the exit code, and the workflow continues with the PR branch's .github/ intact — exactly the attack vector this step is designed to prevent.
Suggested fix: Split the restore so .github/ is mandatory and .agents/ is optional:
git checkout "$BASE_SHA" -- .github/
git checkout "$BASE_SHA" -- .agents/ 2>/dev/null || true| ## When to Read the Full Reference | ||
|
|
||
| Read `.claude/skills/gh-aw-guide/references/architecture.md` when you need: | ||
| ## Further Reading |
There was a problem hiding this comment.
🟢 MINOR — orphan heading left behind
## When to Read the Full Reference is now an empty section with no content, immediately followed by ## Further Reading. It looks like the old heading should have been removed or replaced, not left as an empty orphan.
Suggest removing line 437 (and its trailing blank line) so only ## Further Reading remains.
| gh pr checkout "$PR_NUMBER" | ||
| # Restore trusted .github/ from base branch | ||
| BASE_SHA=$(gh pr view "$PR_NUMBER" --json baseRefOid --jq '.baseRefOid') | ||
| git checkout "$BASE_SHA" -- .github/ .agents/ 2>/dev/null || true |
There was a problem hiding this comment.
🔴 CRITICAL — Restore failure is silently swallowed (fail-open)
|| true discards the exit code and 2>/dev/null hides the error message. If git checkout fails (invalid SHA, ref not found, git state issue), the workflow continues with the PR branch's potentially untrusted .github/ content intact. Since this snippet is documentation that practitioners will copy into real workflows, a fail-open example is actively dangerous — consumers get a false sense of security.
Failing scenario: BASE_SHA resolves to a ref that's been force-pushed away or garbage-collected. The restore silently fails and the agent runs against untrusted .github/ infrastructure from the PR branch.
Suggested fix — make .github/ restore mandatory, .agents/ optional:
git checkout "$BASE_SHA" -- .github/ \
|| { echo "::error::Failed to restore trusted .github/ from base branch"; exit 1; }
git checkout "$BASE_SHA" -- .agents/ 2>/dev/null || trueFlagged by: 3/3 reviewers
| # Verify PR author has write access, reject forks | ||
| AUTHOR=$(gh pr view "$PR_NUMBER" --json author --jq '.author.login') | ||
| PERM=$(gh api "repos/$GITHUB_REPOSITORY/collaborators/$AUTHOR/permission" --jq '.permission') | ||
| if [[ "$PERM" != "admin" && "$PERM" != "write" && "$PERM" != "maintain" ]]; then | ||
| echo "::error::PR author $AUTHOR has $PERM access — requires write+" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🟡 MODERATE — Comment claims "reject forks" but no fork check exists
The comment on line 208 says # Verify PR author has write access, reject forks, and the prose description lists "rejects fork PRs" as goal (1). However, the code only checks collaborator permission level — there is no isCrossRepository check. A maintainer with write access who opens a PR from their personal fork passes the permission guard despite the stated intent to reject forks.
Since this is a documentation template that practitioners will copy, the misleading comment is concerning — consumers will believe the fork case is handled when it isn't.
Suggested fix — add an explicit fork guard:
IS_FORK=$(gh pr view "$PR_NUMBER" --json isCrossRepository --jq '.isCrossRepository')
if [[ "$IS_FORK" == "true" ]]; then
echo "::error::Fork PRs are not supported for workflow_dispatch"
exit 1
fiFlagged by: 3/3 reviewers (after consensus)
| # Restore trusted .github/ from base branch | ||
| BASE_SHA=$(gh pr view "$PR_NUMBER" --json baseRefOid --jq '.baseRefOid') | ||
| git checkout "$BASE_SHA" -- .github/ .agents/ 2>/dev/null || true |
There was a problem hiding this comment.
🟡 MODERATE — .claude/ directory not included in trusted-path restore
The restore command covers .github/ and .agents/ but not .claude/, where this repository (and others following the same pattern) stores agent skills. A PR author could embed adversarial instructions in .claude/skills/ files that survive the checkout-and-restore sequence. While this gap also existed in the old Checkout-GhAwPr.ps1 (which restored specific .github/ subpaths, not .claude/), the new snippet is an opportunity to close it — especially since this is a template others will copy.
Suggested fix — add .claude/ to the restore:
git checkout "$BASE_SHA" -- .github/ .agents/ .claude/ 2>/dev/null || true(Combined with the fail-open fix above, make .github/ mandatory and others optional.)
Flagged by: 2/3 reviewers
Removes all PolyPilot-specific references so the SKILL.md can be copied to any repo. Replaced Checkout-GhAwPr.ps1 with inline bash, removed architecture.md dependency, generic language.