perf(security-guard): prioritize security-relevant files in PR diff#5329
Conversation
The Security Guard agent's pre-fetched diff dumped full patches for every changed file, so large refactor PRs sent oversized prompts (cost, latency, and a larger first-request that can trip the upstream Copilot provider). Rework the pre-fetch so it includes full patches only for security-relevant files (matching the same path regex the relevance gate uses), largest first, and lists every other changed file by name only. A non-security file's patch no longer bloats the prompt (e.g. a workflow/test-only PR drops from ~22 KB to a few hundred bytes). Also stop instructing the agent to re-fetch the entire PR diff on truncation: security-relevant patches are shown first, so it should only fetch a still-missing security-relevant file via get_pull_request_diff. Prompt copy and the truncation note updated to match. Recompiled the lock (security-relevant patch prioritization in the pr-diff step); the COPILOT_DUMMY_BYOK placeholder fix from #5306 and the gh-aw-mcpg image pin are preserved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the Security Guard workflow’s pre-fetched PR diff to reduce initial prompt size and avoid oversized first requests, by prioritizing full patches for security-relevant files and omitting patches for non-security churn.
Changes:
- Pre-fetch only includes full patches for security-relevant files (sorted largest-first) and lists all other changed files by name with +/- counts.
- Updates truncation guidance and prompt instructions to avoid re-fetching the entire PR diff unnecessarily.
- Regenerates
security-guard.lock.ymlto reflect the updated workflow source.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/security-guard.md | Adjusts PR-file prefetch logic to inline only security-relevant patches and updates prompt/truncation instructions accordingly. |
| .github/workflows/security-guard.lock.yml | Regenerated locked workflow reflecting the updated prefetch logic and prompt text. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 4
| DIFF_LIMIT=100000 | ||
| SECURITY_RE='host-iptables|setup-iptables|squid-config|docker-manager|seccomp-profile|domain-patterns|entrypoint\.sh|Dockerfile|(^|/)containers/' | ||
| DIFF_TMP="$(mktemp)" | ||
| # Include full patches only for security-relevant files (largest first); | ||
| # list every other changed file by name so large non-security refactors | ||
| # don't bloat the prompt. | ||
| gh api "repos/${GH_REPO}/pulls/${PR_NUMBER}/files" --paginate --slurp \ | ||
| | jq -r --arg re "$SECURITY_RE" ' | ||
| ([.[][]]) as $files | ||
| | ([$files[] | select(.filename | test($re))] | sort_by(-(.additions + .deletions))) as $sec | ||
| | ([$files[] | select(.filename | test($re) | not)]) as $other | ||
| | ( $sec[] | ||
| | "### " + .filename + " (+" + (.additions|tostring) + "/-" + (.deletions|tostring) + ") [security-relevant]\n" + (.patch // "(binary or no textual patch)") + "\n" ), | ||
| ( if ($other | length) > 0 | ||
| then "\n### Other changed files (not security-relevant — patches omitted to save context):\n" | ||
| + ([$other[] | "- " + .filename + " (+" + (.additions|tostring) + "/-" + (.deletions|tostring) + ")"] | join("\n")) + "\n" | ||
| else empty end ) | ||
| ' > "$DIFF_TMP" || true |
| ## ⚡ Fast Path | ||
|
|
||
| Read the pre-fetched diff below first. If you see `[DIFF TRUNCATED ...]`, fetch full context once with `mcp__github__get_pull_request_diff` before deciding to noop. Only use the fast path when the full diff contains **no** security-weakening changes: no weakened DROP/REJECT or expanded ACCEPT, no egress/domain allowlist expansion, no firewall chain changes, no capability additions, no ACL regressions, no seccomp relaxations, no DNS/wildcard bypass, no input validation weakening, and no secrets. Then call `safeoutputs noop` immediately — do not read additional files or make further tool calls. | ||
| Read the pre-fetched diff below first. Security-relevant files are included in full; other changed files are listed by name only. If you see `[DIFF TRUNCATED ...]` and a **security-relevant** patch is missing, fetch just that file's context once with `mcp__github__get_pull_request_diff` before deciding to noop. Only use the fast path when the security-relevant changes contain **no** security-weakening changes: no weakened DROP/REJECT or expanded ACCEPT, no egress/domain allowlist expansion, no firewall chain changes, no capability additions, no ACL regressions, no seccomp relaxations, no DNS/wildcard bypass, no input validation weakening, and no secrets. Then call `safeoutputs noop` immediately — do not read additional files or make further tool calls. |
| 1. **Review the pre-fetched diff below** (security-relevant files in full; other files listed by name) | ||
| 2. **Batch all independent reads** in a single tool-use block rather than making sequential calls | ||
| 3. **Use ONLY the pre-fetched diff below.** Do NOT call `gh pr diff`, `gh pr view`, `gh api`, `git diff`, `git log`, or `git show`. Do NOT read files from the checkout. If `[DIFF TRUNCATED ...]` appears, call `mcp__github__get_pull_request_diff` once — then stop making tool calls and analyze inline. | ||
| 3. **Use ONLY the pre-fetched diff below.** Do NOT call `gh pr diff`, `gh pr view`, `gh api`, `git diff`, `git log`, or `git show`. Do NOT read files from the checkout. If `[DIFF TRUNCATED ...]` appears and a security-relevant patch is missing, call `mcp__github__get_pull_request_diff` once — then stop making tool calls and analyze inline. |
|
|
||
| **IMPORTANT: Be concise.** Report each security finding in ≤ 150 words. Maximum 5 findings total. | ||
| If `[DIFF TRUNCATED ...]` is present, fetch full context once with `mcp__github__get_pull_request_diff` before deciding to noop. | ||
| If `[DIFF TRUNCATED ...]` is present and a security-relevant patch is missing, fetch just that file's context once with `mcp__github__get_pull_request_diff` before deciding to noop. |
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
|
@copilot address review feedback |
Addressed in b7b21ac.
|
|
✅ Smoke Claude passed |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Smoke Gemini completed. All facets verified. 💎 Smoke test completed with partial success. GitHub MCP tools missing. Connectivity, file writing, and bash testing PASSED. |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
✅ Build Test Suite completed successfully! |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test: Claude Engine Validation
Overall result: PASS
|
🔐 Smoke Test: Copilot PAT Auth — PASS
Overall: PASS — @lpcox
|
🔬 Smoke Test Results
PR: perf(security-guard): prioritize security-relevant files in PR diff Overall: FAIL —
|
|
Copilot BYOK Smoke Test Results ✅ GitHub MCP connectivity Overall: PASS — Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY) via api-proxy.
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
Smoke Test Results: Gemini Engine
Overall Status: FAIL Note: MCP tools were missing from the agent environment. Connectivity was verified using an explicit proxy (172.30.0.10:3128). Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Chroot Version Comparison Results
Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot environments.
|
Smoke Test: API Proxy OpenTelemetry Tracing
All scenarios pass. ✅
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
Smoke Test Results:
Overall: PASS
|
Smoke Test Results — FAIL ❌
|
|
Context
While diagnosing the Security Guard failure on PR #5310 (
Authentication failed with provider at http://172.30.0.30:10002 (HTTP 403)), I noticed the agent's first request was very large (151.2k tokens, 125.4k cached). The upstream-403 root cause was already addressed separately by #5306 (givingCOPILOT_DUMMY_BYOKa validghu_…token format). This PR is the complementary prompt-size improvement (option 3 from the diagnosis): shrink the pre-fetched diff so big PRs don't send oversized prompts.Problem
The
Fetch PR changed filesstep dumped full patches for every changed file in the PR (up to 100 KB), then instructed the agent to re-fetch additional context when truncated. On large refactor PRs this bloats the prompt (cost, latency, and a bigger first request that is more likely to trip the upstream provider).Change
The pre-fetch now:
host-iptables,setup-iptables,squid-config,docker-manager,seccomp-profile,domain-patterns,entrypoint.sh,Dockerfile,containers/), largest first.(^|/)tests?/|\.test\.) from that security-relevant inline set, matching the relevance gate behavior.mcp__github__get_pull_request_diffreturns the full PR diff; if truncation hides a needed security patch, fetch once and locate that file section.The agent only runs when ≥1 security-relevant file changed (existing
if:gate), so it always has full security patches to review; non-security churn no longer inflates the context.Measured effect
For a workflow/test-only PR (no security files), the pre-fetched block drops from ~22 KB → a few hundred bytes. For mixed PRs, only security-relevant patches are sent in full.
Notes
security-guard.lock.ymlto mirror thepr-diffstep and prompt wording changes while preserving theCOPILOT_DUMMY_BYOKplaceholder from [WIP] Fix security guard workflow failure due to authentication issues #5306 and thegh-aw-mcpgimage pin.scripts/ci/security-guard-workflow.test.tsandready-for-aw-workflows.test.tspass.Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>