Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53879 does not have the 'implementation' label and has 0 new lines of code in business logic directories (file_count: 2, default_business_additions: 0).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. (PR contains shell script changes: commit_cache_memory_git.sh and commit_cache_memory_git_test.sh — these are outside the analysis scope of Go *_test.go and JavaScript .test.cjs/.test.js files.)
|
|
✅ Ponytail Reviewer completed successfully!
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
This hardening is moving in the right direction, but it also introduces a repository-reset path that can break legitimate cache-memory repositories and discard the integrity history this script is supposed to preserve.
Blocking theme
- Reinitializing the repository when Git metadata is symlinked is not a safe sanitization strategy; it can destroy valid history in linked-worktree or redirected metadata layouts.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 5.47 AIC · ⌖ 8.73 AIC · ⊞ 7K
Comment /review to run again
|
|
||
| if has_symlinked_git_metadata; then | ||
| echo "WARNING: Detected symlinked cache-memory git metadata; reinitializing git metadata" | ||
| rm -rf .git |
There was a problem hiding this comment.
This hardening silently wipes the entire cache-memory Git history whenever .git, .git/config, .git/info, or .git/hooks is a symlink. In the common linked-worktree layout, .git is supposed to be a file/symlink-like indirection into the real metadata directory, so rm -rf .git && git init detaches the checkout from the original repo and destroys the integrity branch history you are trying to preserve.
💡 Why this blocks the change
The new has_symlinked_git_metadata guard treats any symlink as hostile and reinitializes the repository in place. That is too blunt for Git’s normal storage models: worktrees and some cached checkouts legitimately redirect metadata outside the working tree. Reinitializing here means later commits land in a brand-new repo with no prior refs, so auditability and cache continuity are broken.
A safer fix is to reject unexpected metadata layouts without mutating them, or to validate the resolved target stays inside an allowed directory before using it.
if has_symlinked_git_metadata; then
echo "Refusing to mutate symlinked git metadata" >&2
exit 1
fiThat preserves the security boundary without corrupting valid repositories.
There was a problem hiding this comment.
Pull request overview
Hardens post-agent cache commits against agent-controlled Git hooks and configuration.
Changes:
- Scrubs risky Git metadata and configuration.
- Disables hooks/fsmonitor and bypasses commit verification.
- Adds hook/filter regression coverage.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/commit_cache_memory_git.sh |
Hardens post-agent staging and commits. |
actions/setup/sh/commit_cache_memory_git_test.sh |
Tests malicious hooks and filters. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 5
- Review effort level: Balanced
| has_symlinked_git_metadata() { | ||
| local path | ||
| for path in .git .git/config .git/info .git/hooks; do | ||
| if [ -L "$path" ]; then | ||
| return 0 | ||
| fi | ||
| done | ||
| return 1 | ||
| } |
| mkdir -p .git/info | ||
| rm -f .git/info/exclude .git/info/attributes .git/info/grafts .git/info/sparse-checkout |
| git config --unset-all core.attributesFile >/dev/null 2>&1 || true | ||
| git config --unset-all core.fsmonitor >/dev/null 2>&1 || true | ||
| git config --unset-all core.sshCommand >/dev/null 2>&1 || true | ||
| git config --unset-all core.hooksPath >/dev/null 2>&1 || true |
| # Commit on the current integrity branch; allow empty commits in case | ||
| # the agent made no changes (idempotent). | ||
| if git commit --allow-empty -m "run-${RUN_ID}" -q 2>/tmp/gh-aw-commit-err; then | ||
| if git commit --no-verify --allow-empty -m "run-${RUN_ID}" -q 2>/tmp/gh-aw-commit-err; then |
| cat > "${D}/evil-hooks/pre-commit" <<EOF | ||
| #!/usr/bin/env bash | ||
| touch "${SENTINEL_HOOK}" | ||
| EOF | ||
| chmod +x "${D}/evil-hooks/pre-commit" | ||
| printf 'content\n' > "${D}/agent-data" | ||
| printf 'agent-data filter=evil\n' > "${D}/.gitattributes" | ||
| run_script "${D}" >/dev/null | ||
| assert "pre-commit hook was not executed" "[ ! -e '${SENTINEL_HOOK}' ]" |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two missing core.* scrubs and a missing reinit test path.
📋 Key Themes & Highlights
Issues Found
core.worktreenot scrubbed (line 55 of the script) — could redirectgit add -Ato an agent-controlled pathcore.gitProxynot scrubbed (line 58) — executes arbitrary commands on every remote URL match- User identity set after reinit block (line 47) — leaves a narrow window where commit identity is undetermined
scrub_git_config_entriescase-handling —-iflag gives false confidence; lower-case contract should be documentedevalin test helper — sets a risky precedent even if currently safe- Symlinked
.gitreinit path has no test coverage — a regression there would be invisible
Positive Highlights
- ✅ Defence-in-depth: hooks dir cleaned,
core.hooksPath /dev/null,--no-verify, and config scrubbing are layered correctly - ✅ Regression test covers the primary attack vector (hook + clean filter) end-to-end
- ✅
has_symlinked_git_metadatacorrectly checks all relevant paths including.git/config,.git/info,.git/hooks - ✅
set -euo pipefailin both files ensures unexpected errors surface immediately
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 51.3 AIC · ⌖ 11.7 AIC · ⊞ 7.8K
Comment /matt to run again
| git init -q | ||
| fi | ||
|
|
||
| # Agent-written cache state may contain hooks or configuration that executes |
There was a problem hiding this comment.
[/diagnosing-bugs] core.worktree is not scrubbed — an agent-written value would cause git add -A to stage files from an agent-controlled directory instead of the cache dir.
💡 Suggested fix
Add this alongside the other core.* unsets:
git config --unset-all core.worktree >/dev/null 2>&1 || truecore.worktree changes what directory git treats as the working tree, so it's a high-severity execution-redirection surface.
@copilot please address this.
| # Agent-written cache state may contain hooks or configuration that executes | ||
| # during staging or committing. Clear those command surfaces before either step. | ||
| if [ -d .git/hooks ]; then | ||
| find .git/hooks -mindepth 1 -maxdepth 1 \( -type f -o -type l \) ! -name '*.sample' -delete |
There was a problem hiding this comment.
[/diagnosing-bugs] core.gitProxy is not scrubbed — it accepts a shell command that git executes for every remote URL match, making it a persistent command-injection surface even if no push/fetch occurs in this script.
💡 Suggested fix
git config --unset-all core.gitProxy >/dev/null 2>&1 || trueAlso consider uploadpack.packObjectsHook and uploadpack.packObjectsHook for completeness, which likewise execute arbitrary commands.
@copilot please address this.
| fi | ||
| done | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] After rm -rf .git && git init -q, the new repo has no user.email/user.name yet, so if the script exits early between git init and the git config user.* lines below, the subsequent commit would use whatever global/system identity is configured — potentially none, causing an error with unhelpful output.
💡 Suggestion
Move the git config user.* lines immediately after git init (inside the if block), so the identity is always set before any other git operation:
if has_symlinked_git_metadata; then
rm -rf .git
git init -q
git config user.email "gh-aw@github.com"
git config user.name "gh-aw"
fi@copilot please address this.
|
|
||
| cd "$CACHE_DIR" | ||
|
|
||
| scrub_git_config_entries() { |
There was a problem hiding this comment.
[/diagnosing-bugs] scrub_git_config_entries calls git config --local, but if .git was just reinitialized (empty config), there is nothing to unset — that is fine. However if the grep -E -i match produces zero lines the while loop body never runs and exits 0. The subtle risk: git config --name-only --list may emit keys with uppercase prefixes that the ^${key_prefix}\. pattern misses on case-sensitive filesystems if key_prefix is passed in mixed case. The -i flag on grep mitigates this, but key_prefix is always lower-case at the call sites, so the -i is redundant and its presence may give false confidence.
💡 Suggestion
Document the lower-case contract in the function comment, or normalise with tr '[:upper:]' '[:lower:]' before the grep, so callers never need to think about case:
| tr '[:upper:]' '[:lower:]' | grep -E "^${key_prefix}\."@copilot please address this.
|
|
||
| assert() { | ||
| local name="$1" | ||
| local condition="$2" |
There was a problem hiding this comment.
[/tdd] assert uses eval on caller-supplied strings, which is an injection risk if a test name or path ever contains shell metacharacters. Given this is a test-only file and paths come from mktemp -d they are likely safe, but it sets a dangerous precedent.
💡 Suggested alternative
Pass the command as a function reference or use a simple if/then pattern instead of eval:
assert() {
local name="$1"; shift
if "$@" 2>/dev/null; then
echo " ✓ ${name}"
TESTS_PASSED=$((TESTS_PASSED + 1))
else
echo " ✗ ${name}"
TESTS_FAILED=$((TESTS_FAILED + 1))
fi
}
# call: assert "name" test -e "${SENTINEL_HOOK}"This eliminates eval entirely for the majority of assertions.
@copilot please address this.
| cat > "${D}/evil-hooks/pre-commit" <<EOF | ||
| #!/usr/bin/env bash | ||
| touch "${SENTINEL_HOOK}" | ||
| EOF |
There was a problem hiding this comment.
[/tdd] The test only covers core.hooksPath (external hooks dir) and filter.*. It does not exercise the symlinked-.git reinitialisation path (has_symlinked_git_metadata), which is one of the two major new code paths added. A bug there would go undetected.
💡 Suggested additional test
echo "Test 3: Symlinked .git metadata is reinitialized"
D3="${WORKSPACE}/test3"
REAL_GIT="${WORKSPACE}/real-git"
git -C "$(mkdir -p "${D3}" && echo ${D3})" init -q
mv "${D3}/.git" "${REAL_GIT}"
ln -s "${REAL_GIT}" "${D3}/.git" # simulate symlinked metadata
run_script "${D3}" >/dev/null
assert "symlinked .git was replaced with real dir" "[ ! -L '${D3}/.git' ]"
assert "repo still functional after reinit" "git -C '${D3}' status"@copilot please address this.
There was a problem hiding this comment.
Ponytail review (over-engineering only): one shrink opportunity found; the rest is tightly-scoped security hardening with a single justified regression test.
net: -3 lines possible.
Generated by ✂️ Ponytail Reviewer for #53879 · auto · 26.2 AIC · ⌖ 4.91 AIC · ⊞ 7.3K
Comment /ponytail to run again
| done | ||
| return 1 | ||
| } | ||
|
|
There was a problem hiding this comment.
shrink: four separate --unset-all calls for single core.* keys, each with its own >/dev/null 2>&1 || true. Loop: for k in attributesFile fsmonitor sshCommand hooksPath; do git config --unset-all "core.$k" >/dev/null 2>&1 || true; done, saves 3 lines.
|
Hey What looks great:
This PR is ready for review and merge. The test coverage for the security fix is solid and directly validates the mitigations. Well done! ✨
|
There was a problem hiding this comment.
Review: Harden post-agent cache Git commits
Good hardening work. The approach — clearing hooks, scrubbing dangerous config sections before staging, and using --no-verify on commit — closes the main injection surfaces. Two non-blocking issues noted inline:
core.worktreenot scrubbed (security gap): could redirectgit add -Ato an arbitrary path.evalin testasserthelper (minor reliability): condition strings with shell-special chars can misfire silently.
No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 47.3 AIC · ⌖ 8.92 AIC · ⊞ 5.7K
| git config --unset-all core.attributesFile >/dev/null 2>&1 || true | ||
| git config --unset-all core.fsmonitor >/dev/null 2>&1 || true | ||
| git config --unset-all core.sshCommand >/dev/null 2>&1 || true | ||
| git config --unset-all core.hooksPath >/dev/null 2>&1 || true |
There was a problem hiding this comment.
core.worktree not scrubbed — an agent could write core.worktree = /sensitive/path to the local git config, causing git add -A to stage files from an arbitrary path instead of the cache directory. Add:
git config --unset-all core.worktree >/dev/null 2>&1 || trueafter the existing core.hooksPath --unset-all line.
@copilot please address this.
| local condition="$2" | ||
| if eval "${condition}" 2>/dev/null; then | ||
| echo " ✓ ${name}" | ||
| TESTS_PASSED=$((TESTS_PASSED + 1)) |
There was a problem hiding this comment.
assert helper uses eval — the test harness evaluates condition strings with eval "${condition}". If a test condition accidentally contains shell-special characters (e.g., from a path with spaces or glob chars), it may silently misfire. Consider replacing eval with bash -c "${condition}" or restructuring assertions to pass the condition as a proper subshell.
@copilot please address this.
|
@copilot Please address the latest blocking review feedback on this PR and then run the Current blockers to resolve:
Failing checks on the current head:
A branch refresh was attempted by sous-chef as well.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot A recent author update landed on this PR, but there is still no maintainer-visible resolution on the blocking reviews. Please refresh the branch if needed, re-run the affected checks, and then run the Current maintainer-visible blockers:
A branch refresh was attempted by sous-chef as well.
|
Confirmed on current HEAD |
|
@copilot A recent author update landed on this PR, but the blocking security reviews still need a final maintainer-visible resolution summary. Current maintainer-visible blockers:
|
Confirmed on current HEAD |
|
Triage: category= Security-hardening change (git symlink/worktree metadata) with repeated CHANGES_REQUESTED from github-actions[bot] and active gh-aw-bot escalation thread. mergeable_state=blocked. Needs maintainer resolution given security sensitivity — recommend fast-tracking human review to close out blockers.
|
|
@copilot A recent author update landed on this PR, but the blocking security reviews still need a final maintainer-visible resolution summary. Current maintainer-visible blockers:
|
Confirmed on current HEAD |
|
@copilot Please address the latest maintainer-visible blockers on this PR and then run the Current maintainer-visible blockers:
A branch refresh was attempted by sous-chef as well.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Resolved in |
|
@copilot Please address the latest maintainer-visible blockers on this PR and then run the Current maintainer-visible blockers:
|
Current HEAD |
The security scan flagged dynamic Git operations in setup scripts. Review identified the post-agent cache commit path as able to inherit agent-written hooks and Git configuration before staging or committing.
Git metadata hardening
.gitmetadata.include*,filter*,merge*, aliases, credential settings, and unsafecore.*keys)./dev/nullhooks and disabled fsmonitor.Commit defense in depth
--no-verify.Regression coverage
pre-commithook and Git clean filter, then verifies neither executes while agent changes are committed.run: https://github.com/github/gh-aw/actions/runs/32204573204> Generated by 👨🍳 PR Sous Chef · gpt54 · 25.9 AIC · ⌖ 8.32 AIC · ⊞ 9.3K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32210439073> Generated by 👨🍳 PR Sous Chef · gpt54 · 25.8 AIC · ⌖ 8.18 AIC · ⊞ 9.3K · ◷