Repository navigation
Clean checkout includeIf credentials before agent verification - #66299
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The cleanup now matches verifier behavior while preserving unrelated includes and files outside the runner temporary directory.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes credential cleanup for actions/checkout configurations linked through includeIf.
Changes:
- Removes checkout credential includes from repository and submodule configs.
- Deletes credential files only within
RUNNER_TEMP. - Adds regression coverage for duplicates, worktrees, submodules, and preserved includes.
| File | Description |
|---|---|
actions/setup/sh/clean_git_credentials.sh |
Cleans checkout-generated credential includes safely. |
actions/setup/sh/clean_git_credentials_test.sh |
Tests cleanup and verification behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![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; approving with one non-blocking follow-up observation.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause correctly addressed: the fix teaches
clean_git_credentials.shto followincludeIf.*.pathentries pointing atgit-credentials-<uuid>.configfiles, matching exactly howactions/checkout@v7persists credentials (per the linked issue #66152 root-cause analysis). - ✅ Regression test (Test 9) written to the correct seam: it reproduces the fail-closed bug first (
verifier detects included credentials before cleanup) then asserts the fix resolves it — textbook/diagnosing-bugsPhase 5 discipline. - ✅ Test coverage is thorough for a security-sensitive path: duplicate includeIf entries, worktree includes, submodule includes, unrelated includes preserved, and deletion scoped strictly to files under
RUNNER_TEMP(file outsideRUNNER_TEMPcorrectly retained). - ✅ Defensive
--fixed-value --get-allcheck before--unset-allavoids accidentally matching unrelated keys with the same name but different values. - ✅ Ran the full test suite locally (26/26 passing) and shellcheck clean on the modified script.
Non-blocking follow-up (out of diff scope)
actions/setup/sh/clean_git_credentials_checkout.sh delegates to the now-fixed clean_git_credentials.sh when available, but its own inline fallback branch (used when the shared script isn't deployed/executable yet at checkout time) duplicates the pre-fix cleanup logic without the new includeIf handling. Since that fallback also runs immediately before verify_git_credentials.sh in compiled workflows (e.g. smoke-claude.lock.yml), the same fail-closed scenario from #66152 could resurface through that code path specifically. Worth a follow-up PR to either extract shared logic or port the includeIf cleanup into the fallback, so the fix lands in all copies of this duplicated cleanup logic (/codebase-design — duplicated domain logic across clean_git_credentials.sh, clean_git_credentials_checkout.sh, clean_git_credentials_pre_setup.sh).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 106 AIC · ⌖ 14.5 AIC · ⊞ 10.1K
Comment /matt to run again
| fi | ||
| echo "Removed checkout credential include from git config" | ||
| fi | ||
| done < <(git config --file "${GIT_CONFIG_PATH}" --null --get-regexp '^includeif\..*\.path$' 2>/dev/null) |
There was a problem hiding this comment.
@copilot add a maximum number of iteration + failure if max is reached to ensure termination
There was a problem hiding this comment.
Added a 1,000-iteration limit per config in 9e95d7c. If another include entry remains, cleanup exits 1 with an explicit error. Boundary regression tests pass (31 assertions total).
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
When checkout persists credentials in a separate config linked through
includeIf, cleanup misses them but verification follows the link. The resulting fail-closed check prevents the agent from starting.RUNNER_TEMP; preserve unrelated includes and files outside that directory.