Preserve step-installed skills during base-branch restore; allow Claude's Skill tool - #62419
Conversation
…laude Skill tool Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories (53 additions).
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Preserving all untracked agent files permits earlier steps executing PR-controlled code to persist malicious skills past the security restore.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
This PR preserves step-installed skills during base-branch restoration and enables Claude’s Skill tool by default.
Changes:
- Preserves untracked files while replacing Git-tracked agent configuration.
- Adds restoration tests, documentation, and release notes.
- Regenerates Claude tool allowlists in workflow fixtures and lock files.
| File | Description |
|---|---|
actions/setup/sh/restore_base_github_folders.sh |
Preserves untracked agent files during restore. |
actions/setup/sh/restore_base_github_folders_test.sh |
Tests tracked-file removal and fallbacks. |
pkg/workflow/claude_tools.go |
Allows Claude’s Skill tool. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden |
Updates Claude golden output. |
docs/src/content/docs/reference/dependencies.md |
Documents PR restoration behavior. |
.changeset/claude-skills-survive-base-restore.md |
Adds patch release note. |
.github/workflows/step-name-alignment.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/static-analysis-report.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/smoke-github-claude.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/smoke-claude-on-copilot.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/smoke-agent-public-none.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/smoke-agent-public-approved.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/smoke-agent-all-none.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/smoke-agent-all-merged.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/sergo.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/semantic-function-refactor.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/scout.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/safe-output-health.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/ruflo-backed-task.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/prompt-clustering-analysis.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/portfolio-analyst.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/lockfile-stats.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/instructions-janitor.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/go-pattern-detector.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/go-fan.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/github-mcp-tools-report.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/github-mcp-structural-analysis.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/example-workflow-analyzer.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/deepsec-security-scan.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/daily-vulnhunter-scan.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/daily-harness-experiment-proposer.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/daily-elixir-credo-snippet-audit.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/daily-choice-test.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/daily-astrostylelite-markdown-spellcheck.lock.yml |
Regenerates Claude allowlist. |
.github/workflows/aw-failure-investigator.lock.yml |
Regenerates Claude allowlist. |
| # PR-branch content is removed by deleting the git-tracked files under each item | ||
| # rather than the whole directory. Everything the PR branch can control is | ||
| # tracked in git, so untracked files are necessarily produced by earlier steps of | ||
| # the agent job itself (for example the APM package restore, which unpacks skills | ||
| # into .claude/skills/ or .github/skills/). Those files are trusted and must |
There was a problem hiding this comment.
Addressed in 3cf8635: restored full removal of agent configuration folders, removed the tracked/untracked trust distinction, and removed the tests and release-note claims that relied on it.
|
|
||
| ## Pull request runs | ||
|
|
||
| On pull request triggers the agent job restores agent config folders (`.agents`, `.github`, and the engine folder such as `.claude`) from a base-branch snapshot so a fork PR cannot inject skill or instruction files. This restore removes only the files tracked by git in the PR branch, so package files that APM unpacked into `.claude/skills/` or `.github/skills/` earlier in the job are preserved. |
There was a problem hiding this comment.
Addressed in 3cf8635: the paragraph now begins “For pull request triggers,” and describes the restored full-folder replacement behavior.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues found.
📋 Key Themes & Highlights
Analysis
The fix correctly reframes the security invariant from "path == PR-owned" to "path is git-tracked == PR-owned," which precisely matches how actions/checkout + git checkout -B populate the workspace. This is the right root-cause fix rather than a narrower one-off patch for APM skills specifically — it will transparently cover any future trusted step that writes untracked files into agent folders.
Test coverage (/tdd)
- Test 7 exercises the primary regression scenario (untracked APM skill under
.claude/skills/survives while tracked PR content is replaced/removed) — directly reproduces the reported bug (#62167) before the fix and demonstrates it's fixed after. - Test 8 covers the "folder absent from base" branch with untracked files present.
- Test 9 covers the
git ls-filesfailure fallback (corrupt index), verifying the safe "remove everything" fallback still fires — this is the correct conservative choice since an unreadable index means tracked/untracked status can't be trusted. - All 25 assertions in
restore_base_github_folders_test.shpass locally, andgo build ./pkg/workflow/...compiles cleanly with the updatedclaude_engine_tools_test.goexpectations.
Design notes (/diagnosing-bugs)
- Root cause (ordering of
steps:-declared restores vs.Checkout PR branchvs.Restore agent config folders) is clearly explained in the PR description and the script's own header comment — good self-documentation for future maintainers. - The empty-directory cleanup (
find "${dest}" -type d -empty -delete) after removing tracked files is a nice touch to avoid leaving stray empty dirs when a PR-tracked directory becomes empty. - Minor (non-blocking) observation: the NUL-separated temp-file approach for
git ls-files -zis a robust way to avoid the classic word-splitting/newline pitfalls of command substitution — good attention to shell correctness here.
Positive Highlights
- ✅ Root-cause fix, not a special-case patch for APM alone
- ✅ Conservative fallbacks in both non-git and git-failure scenarios preserve the original "wipe everything" safety property
- ✅ Regression tests added before/alongside the fix, covering the exact reported scenario plus edge cases
- ✅ Documentation (
dependencies.md) and changeset updated to reflect the new behavior - ✅
Skillallowed-tool addition is well-scoped and is a no-op when no skills are registered (per Claude's own tool-exposure behavior)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 80.1 AIC · ⌖ 14.4 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
Impeccable Review — bug_fix (harden, audit)
Reviewed the security-sensitive restore script rewrite and the Claude Skill allowed-tool addition.
What I checked
remove_pr_content()correctly scopes deletion togit ls-files-tracked paths, falls back to fullrm -rfwhen the workspace isn't a git worktree orgit ls-filesfails (corrupt index), and cleans up now-empty directories — the invariant "PR-branch content ⟺ git-tracked" holds givencheckout+git checkout -B.- Base-snapshot restore now uses
cp -R "${SNAPSHOT}/." "${DEST}/"after clearing tracked content, so base-branch files win collisions as documented, while untracked (trusted, step-installed) files survive. - New tests (7–9) cover: untracked APM skills surviving a restore, tracked-only removal when a folder is absent from base, and fallback-to-full-removal on a corrupted git index. Golden/test-expectation updates for the
Skillallowed-tool addition are consistent with the code change. claude_tools.go: addingSkillto the default allowed-tools list is inert unless a skill is registered by Claude, matching the stated rationale.
No blocking issues found in the changed lines. Nice defensive coding — the NUL-separated temp-file handling for git ls-files -z output correctly avoids the command-substitution NUL-stripping pitfall.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 54.6 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
Please address the open review follow-ups and then run the Open review follow-ups (newest first):
I also requested a branch refresh for this PR.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| while IFS= read -r -d '' tracked; do | ||
| # -r handles gitlink entries (submodule directories) | ||
| rm -rf "${WORKSPACE:?}/${tracked:?}" | ||
| done <"${tracked_list}" | ||
| rm -f "${tracked_list}" | ||
|
|
||
| # Drop directories that only held PR-branch files | ||
| if [ -d "${dest}" ]; then | ||
| find "${dest}" -type d -empty -delete | ||
| fi |
There was a problem hiding this comment.
This finding was raised against 203b4c6, which deleted only git-tracked paths. That behavior was reverted in 3cf8635: remove_pr_content no longer exists and the script again does a full rm -rf of each agent config folder (and rm -f of each root instruction file) before overlaying the base snapshot, removing entries that the base branch doesn't have at all. No untracked-equals-trusted assumption remains, so the fork-PR injection boundary is back to "remove the whole agent config folder".
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran the
CI on the current HEAD is stale and needs a maintainer re-trigger (close/reopen or push); I cannot trigger it from here. |
|
@copilot update compiler threat specification |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
👏 Excellent work on fixing the APM skill preservation issue! This PR addresses a critical security and functional gap in the base-branch restore process while properly enabling Claude's Skill tool capability. The approach is solid:
This looks ready for review and merge. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"See Network Configuration for more information.
|
Updated in Added CTR-028 Agent Configuration Restore Provenance to
|
|
@copilot please refresh this branch from the base branch, address any remaining review feedback, and then run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
…gine-base-folder-restore Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed in |
|
Please address the remaining review feedback below, inspect the failing checks, refresh the branch if needed, and then run the Open review follow-ups (newest first):
Failed checks:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
With
engine: claudeand an APM import, skills unpacked into.claude/skills/were deleted before the agent started: theRestore agent config folders from base branchstep either overwrote.claudefrom the base snapshot orrm -rf'd it when the base branch had no.claude/. ADR-28290 addressed this forpre-agent-steps, but the canonicalshared/apm.mddeclares its restore under frontmattersteps:, which is emitted beforeCheckout PR branch, so the ordering invariant doesn't apply to it.Compiled order for a
pull_requestworkflow importingshared/apm.md:Changes
actions/setup/sh/restore_base_github_folders.sh— removes only the git-tracked files under each agent folder/root file, then overlays the base snapshot, instead ofrm -rf+ copy. Aftergit checkout -B <branch> origin/pr-headeverything the PR branch controls is tracked, so fork-injection protection is unchanged while untracked files written earlier in the same job (APM packages) survive. Collisions resolve in favor of the base-branch version.rm -rfwhen the workspace is not a git worktree or whengit ls-filesfails (corrupt index); per-entryrm -rfso submodule gitlinks are removable. Tracked paths are read NUL-separated from a temp file since command substitution drops NULs.pkg/workflow/claude_tools.go— addsSkillto Claude's default--allowed-tools. Previously anySkillcall returnedpermission_deniedunless the workflow usedpermission-mode: bypassPermissions. Claude only exposes the tool when a skill is registered, so this is inert otherwise.restore_base_github_folders_test.sh(untracked APM skills survive a base restore; folder absent from base keeps untracked files;ls-filesfailure falls back to full removal). Existing tests use non-git temp workspaces and therefore cover the fallback path.reference/dependencies.md.Notes for review
The security property now rests on "PR-branch content ⟺ git-tracked". The one way to violate it would be attacker-controlled untracked files surviving into the workspace, which
actions/checkout+git checkout -Bdoes not produce.pr-sous-chefRun: https://github.com/github/gh-aw/actions/runs/35656487865
pr-sous-chefrun https://github.com/github/gh-aw/actions/runs/35743734337Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.