safe-outputs: pre-flight workflow scope check + full-branch allowed_files validation - #42585
Conversation
…fe output server Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…dation for safe output server Root cause: When a review/fallback branch is pushed as a new ref, GitHub validates the entire commit history for .github/workflows/** changes (not just the new commits). This caused pushes to fail when the PR branch had earlier workflow file commits, even though the current changeset only touched allowed files (e.g. .changeset/*.md). Changes: - pkg/workflow/safe_outputs_handler_registry.go: expose allow_workflows in the push_to_pull_request_branch handler config so the JS handler can read it - actions/setup/js/push_to_pull_request_branch.cjs: add detectWorkflowFileChanges() helper + pre-flight checks before review and fallback branch pushes; when allow_workflows is false (default), fail early with a typed workflows_scope_required error before checkout/push side effects occur - actions/setup/js/safe_outputs_handlers.cjs: add full-branch allowed_files check (origin/baseBranch..branch) before patch generation so the agent receives an actionable error at tool-call time rather than at apply-time - actions/setup/js/push_to_pull_request_branch.test.cjs: add 3 new tests covering the pre-flight check (fires early, skipped when allow_workflows: true) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ogic and origin/HEAD baseline Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42585 does not have the 'implementation' label and has only 1 new line of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Improves safe-outputs push reliability by proactively detecting policy/scope problems earlier in the safe-outputs lifecycle, especially for long-running branches whose reachable history includes restricted paths (like workflow files), and by surfacing actionable failures before side-effecting git push operations.
Changes:
- Exposes
allow_workflowsin the Go handler registry so the JS push handler can decide whether to run workflow-scope preflight logic. - Adds a pre-flight workflow-history check in
push_to_pull_request_branch.cjsto fail fast with a typedworkflows_scope_requirederror before creating/pushing review/fallback refs. - Adds a full-branch
allowed_filesvalidation insafe_outputs_handlers.cjs(relative toorigin/<baseBranch>..branch) to catch disallowed files in earlier commits before generating artifacts.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_handler_registry.go | Plumbs allow_workflows into the handler config so the JS handler can see it. |
| actions/setup/js/safe_outputs_handlers.cjs | Adds full-branch allowed_files history validation before artifact generation. |
| actions/setup/js/push_to_pull_request_branch.cjs | Adds workflow-history preflight and allow_workflows gating to avoid push side effects. |
| actions/setup/js/push_to_pull_request_branch.test.cjs | Adds tests for preflight behavior and updates mocks for the new git log call. |
| .github/workflows/daily-team-status.lock.yml | Regenerates a source-managed lockfile as part of this PR’s diff. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Low
| // before any transport artifacts are generated. | ||
| if (Array.isArray(pushConfig.allowed_files) && pushConfig.allowed_files.length > 0) { | ||
| try { | ||
| const branchHistoryFiles = execGitSync(["log", "--name-only", "--pretty=format:", `origin/${baseBranch}..${entry.branch}`], { cwd: pushGitCwd }) |
| # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"9f61ffba0d4e9663a43f41030c13a4c7c71ebac8122f586cbd706756a605df94","body_hash":"33c10cc22b8836b79387efda582e48c5a463e9849880a01d58704a0fa291e986","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.65"}} | ||
| # gh-aw-manifest: {"version":1,"secrets":["COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.16","digest":"sha256:94bbf52b74d38e8117387e93e698f79d678dd3879faa0e57f2ea128eda8fb507","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.16@sha256:94bbf52b74d38e8117387e93e698f79d678dd3879faa0e57f2ea128eda8fb507"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.16","digest":"sha256:bbad2f109b97a4b3375ad371a5300d42bc9251dad61cd7bc66380cad8501cf94","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.16@sha256:bbad2f109b97a4b3375ad371a5300d42bc9251dad61cd7bc66380cad8501cf94"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.16","digest":"sha256:483d6a8086752a02d581d7a42629b741e3f2fa9f3a6a10320590cf881638dad3","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.16@sha256:483d6a8086752a02d581d7a42629b741e3f2fa9f3a6a10320590cf881638dad3"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.32","digest":"sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.3.32@sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} | ||
| # gh-aw-manifest: {"version":1,"secrets":["COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.16"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.16"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.16"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.32","digest":"sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.3.32@sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} | ||
| # This file was automatically generated by gh-aw. DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md | ||
| # | ||
| # ___ _ _ |
|
|
||
| function buildWorkflowsScopeError(context, core) { |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
JavaScript: 2 ( Also noted: 5 lines were added inside the existing test Coverage gap:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 85/100. 0% implementation tests (threshold: 30%). 2 design tests covering the pre-flight workflow-scope check behavioral contract. One minor gap: safe_outputs_handlers.cjs full-branch allowed_files validation (+53 lines) has no new direct unit test.
Skills-Based Review Summary 🧠Applied 📋 Issues Found (4 inline comments + 1 test gap)
The core fix is solid — pre-flight checks that fire before @copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes primarily on test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Missing test coverage for
safe_outputs_handlers.cjs: Asafe_outputs_handlers.test.cjsalready exists but the PR adds no tests for the new 53-line full-branchallowed_filescheck — including the happy path, the disallowed-files error path, and the non-fatal catch fallback. - Baseline inconsistency:
detectWorkflowFileChangesusesorigin/HEADwhile the complementary check insafe_outputs_handlers.cjsusesorigin/${baseBranch}. For PRs targeting non-default branches this divergence can produce false-positive pre-flight rejections. - Copy-pasted pre-flight block: The
if (!allowWorkflows)guard is duplicated verbatim in the review-branch and fallback-branch paths — extracting a helper would make future changes safer. - Silent failure in
detectWorkflowFileChanges: The catch block has no log output, making silent skips invisible in Actions run logs.
Positive Highlights
- ✅ Excellent root-cause fix: pre-flight fires before
git checkout -b, preventing the branch from ending up in a renamed state after a rejected push - ✅ The typed
workflows_scope_requirederror is surfaced early and consistently across both code paths - ✅ The three new tests in
push_to_pull_request_branch.test.cjsare well-structured (Arrange / Act / Assert) with clear names and useful inline comments - ✅ The fix for the existing fallback-PR test mock (
origin/HEADguard) is a clean correctness improvement
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 118.1 AIC · ⌖ 8.32 AIC · ⊞ 6.6K
Comment /matt to run again
| */ | ||
| async function detectWorkflowFileChanges(exec, gitOptions) { | ||
| try { | ||
| const result = await exec.getExecOutput("git", ["log", "--name-only", "--pretty=format:", "HEAD", "--not", "origin/HEAD", "--", ".github/workflows/"], { ...gitOptions, ignoreReturnCode: true }); |
There was a problem hiding this comment.
[/diagnosing-bugs] detectWorkflowFileChanges uses origin/HEAD (the remote default branch) as its comparison baseline, but the complementary check added in safe_outputs_handlers.cjs uses origin/${baseBranch}. For PRs targeting a non-default branch (e.g., release/1.0), these baselines differ, which can cause the pre-flight to flag commits that are already in release/1.0's history and accepted by GitHub, producing a false-positive workflows_scope_required error.
💡 How to align the baselines
detectWorkflowFileChanges could accept a baseBranch parameter and use origin/${baseBranch} as the exclusion point, falling back to origin/HEAD only when baseBranch is not locally available:
async function detectWorkflowFileChanges(exec, gitOptions, baseBranch) {
const baseline = baseBranch ? `origin/${baseBranch}` : "origin/HEAD";
const result = await exec.getExecOutput(
"git", ["log", "--name-only", "--pretty=format:", "HEAD", "--not", baseline, "--", ".github/workflows/"],
{ ...gitOptions, ignoreReturnCode: true }
);
...
}baseBranch is already available at both call sites as baseBranch in scope.
@copilot please address this.
| // Pre-flight: check full branch history for workflow file changes. | ||
| // Like the review branch path, creating a new fallback branch ref triggers | ||
| // GitHub's scope check on the full commit history, not just the new commits. | ||
| if (!allowWorkflows) { |
There was a problem hiding this comment.
[/codebase-design] The pre-flight block (if (!allowWorkflows) { ... }) is copy-pasted verbatim in both the review-branch and fallback-branch paths. While the two paths are mutually exclusive at runtime, the duplication creates a maintenance risk: a future change to the check (e.g., adding caching or adjusting the error message) must be applied in both places.
💡 Consider extracting a shared helper
async function runWorkflowPreflightCheck(allowWorkflows, exec, gitOptions, context, core) {
if (allowWorkflows) return null;
const workflowFiles = await detectWorkflowFileChanges(exec, gitOptions);
if (workflowFiles.length > 0) {
core.info(`Pre-flight check: branch history contains workflow file changes (${workflowFiles.join(", ")}). Failing before push attempt.`);
return buildWorkflowsScopeError(`${context} pre-flight`, core);
}
return null;
}Both call sites become a single line:
const preflightError = await runWorkflowPreflightCheck(allowWorkflows, exec, baseGitOpts, "Review branch", core);
if (preflightError) return preflightError;@copilot please address this.
| .filter(Boolean) | ||
| ), | ||
| ]; | ||
| } catch { |
There was a problem hiding this comment.
[/diagnosing-bugs] The catch block in detectWorkflowFileChanges silently swallows all errors and returns [] with no trace. When origin/HEAD is not set (e.g., a shallow clone where the remote symref was not fetched) or the git command exits non-zero, the pre-flight is silently skipped. This makes future investigation harder — a log line here would at least surface the failure in the Actions run.
💡 Suggested change
} catch (err) {
// Non-fatal: fall back to skipping the check.
// The actual push will surface any real workflows-scope rejection.
gitOptions.debug?.(`detectWorkflowFileChanges: skipping pre-flight (${err?.message ?? err})`);
return [];
}Or, since core is available in the caller, pass it in and call core.debug() so the message appears in the Actions log.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — one high-severity security regression plus two correctness/coverage gaps that must be fixed before merging.
### Blocking issues (3)
1. Firewall image digest pins stripped (HIGH — .github/workflows/daily-team-status.lock.yml)
Three security-critical container images (gh-aw-firewall/agent, api-proxy, squid) lose their @sha256: pins in the manifest JSON, the comment block, both download_docker_images.sh invocations, and the imageTag field of awf-config.json. The other three images remain pinned. This is an unintended regression — not mentioned in the PR description — that allows a mutated image tag to silently replace the firewall at execution time with access to workflow secrets.
2. Wrong exclusion baseline in detectWorkflowFileChanges (HIGH — push_to_pull_request_branch.cjs line 165)
The function uses origin/HEAD (remote default branch) as the exclusion ref instead of the PR's actual base branch. For PRs targeting non-default branches, this traverses commits on the target branch itself, producing false-positive workflows_scope_required errors that incorrectly block pushes. The companion check in safe_outputs_handlers.cjs correctly uses origin/${baseBranch}.
3. ignoreReturnCode: true with unchecked exit code + broad error swallow (MEDIUM — push_to_pull_request_branch.cjs line 170)
result.exitCode is never read, so a partial truncated stdout from a killed git process is silently accepted as a complete file list. The catch { return []; } block documents only the ref-not-found case but swallows all exceptions, meaning a reliably-killed git process always reports no
[!WARNING]
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:network: allowed: - defaults - "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 249 AIC · ⌖ 7.47 AIC · ⊞ 1.6K
Comment /review to run again
| run: bash "${RUNNER_TEMP}/gh-aw/actions/restore_inline_skills.sh" | ||
| - name: Download container images | ||
| run: bash "${RUNNER_TEMP}/gh-aw/actions/download_docker_images.sh" ghcr.io/github/gh-aw-firewall/agent:0.27.16@sha256:94bbf52b74d38e8117387e93e698f79d678dd3879faa0e57f2ea128eda8fb507 ghcr.io/github/gh-aw-firewall/api-proxy:0.27.16@sha256:bbad2f109b97a4b3375ad371a5300d42bc9251dad61cd7bc66380cad8501cf94 ghcr.io/github/gh-aw-firewall/squid:0.27.16@sha256:483d6a8086752a02d581d7a42629b741e3f2fa9f3a6a10320590cf881638dad3 ghcr.io/github/gh-aw-mcpg:v0.3.32@sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477 ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4 | ||
| run: bash "${RUNNER_TEMP}/gh-aw/actions/download_docker_images.sh" ghcr.io/github/gh-aw-firewall/agent:0.27.16 ghcr.io/github/gh-aw-firewall/api-proxy:0.27.16 ghcr.io/github/gh-aw-firewall/squid:0.27.16 ghcr.io/github/gh-aw-mcpg:v0.3.32@sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477 ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4 |
There was a problem hiding this comment.
Digest pins stripped from three security-critical firewall images — pulling by tag alone means a mutated or replaced image at ghcr.io will silently execute attacker-controlled code inside the firewall container.
💡 Details and fix
The diff removes @sha256: digest pins from gh-aw-firewall/agent:0.27.16, gh-aw-firewall/api-proxy:0.27.16, and gh-aw-firewall/squid:0.27.16 in four places:
- The
gh-aw-manifestJSON header (removes"digest"+"pinned_image"fields) - The human-readable comment block (
image:taginstead ofimage:tag@sha256:...) - Both
download_docker_images.shinvocations (lines 510 and 1319) - The
awf-config.jsonimageTagfield (drops from"0.27.16,squid=sha256:...,agent=sha256:...,api-proxy=sha256:..."to"0.27.16")
The other three images (gh-aw-mcpg, gh-aw-node, github-mcp-server) retain their digest pins — this asymmetric downgrade of the security-critical firewall images is unexplained and not mentioned in the PR description.
The firewall containers implement network isolation and the API proxy; running unpinned versions in a future run where the tag has been moved could grant an attacker access to workflow secrets via a compromised image.
This appears to be an accidental side effect of running make recompile on a checkout where the three firewall image digests were locally unavailable. Per repo conventions, this is a source-managed workflow — the digest pins must be restored (or the lockfile regenerated from a canonical checkout) before merging.
| */ | ||
| async function detectWorkflowFileChanges(exec, gitOptions) { | ||
| try { | ||
| const result = await exec.getExecOutput("git", ["log", "--name-only", "--pretty=format:", "HEAD", "--not", "origin/HEAD", "--", ".github/workflows/"], { ...gitOptions, ignoreReturnCode: true }); |
There was a problem hiding this comment.
Wrong commit-range baseline for PRs targeting non-default branches — detectWorkflowFileChanges uses origin/HEAD (always the remote default branch) not the PR's actual base branch, causing false-positive workflows_scope_required errors on non-default-branch PRs.
💡 Details and fix
origin/HEAD is a symbolic ref pointing to the remote's default branch (e.g., main). For a PR that targets a non-default branch such as release/1.x, the correct exclusion baseline is origin/release/1.x. Using origin/HEAD instead causes git log HEAD --not origin/HEAD to traverse all commits between the PR branch tip and main — including commits on release/1.x itself that predate the PR and may have legitimately added workflow files.
The companion check in safe_outputs_handlers.cjs correctly uses origin/${baseBranch}..${entry.branch}, demonstrating this inconsistency.
Suggested fix: Accept the base branch as a parameter and fall back to origin/HEAD only when unavailable:
async function detectWorkflowFileChanges(exec, gitOptions, baseBranch) {
const baseline = baseBranch ? `origin/${baseBranch}` : origin/HEAD;
const result = await exec.getExecOutput(
git,
[log, --name-only, --pretty=format:, HEAD, --not, baseline, --, .github/workflows/],
{ ...gitOptions, ignoreReturnCode: true }
);
// ...
}Then pass configBaseBranch (already available in main()) at the two call sites.
There was a problem hiding this comment.
Review: safe-outputs pre-flight workflow scope check + full-branch allowed_files validation
The design is sound: using commit-history scanning to catch GitHub's push-scope enforcement before creating new branch refs (review/fallback) is the right approach, and the two-layer architecture (MCP-side content policy + apply-time scope check) is well-structured. The fallback to a graceful no-op when baselines are unavailable is correct.
4 comments, one blocking:
| # | Severity | Location | Topic |
|---|---|---|---|
| 1 | Blocking | safe_outputs_handlers.cjs:1262 |
Missing -- terminator in full-branch git log |
| 2 | Blocking | safe_outputs_handlers.cjs:1301 |
No test coverage for the full-branch allowed_files check |
| 3 | Low | push_to_pull_request_branch.test.cjs:1117 |
Mock sentinel couples to origin/HEAD implementation detail |
| 4 | Informational | push_to_pull_request_branch.cjs:165 |
JSDoc should cross-reference why origin/HEAD ≠ origin/${baseBranch} |
Additional notes (no action required)
-
AddIfTruevsAddBoolPtrforallow_workflowsin the Go registry — UsingAddIfTrueis correct here;AllowWorkflowsis a plainbool(not*bool), so it defaults tofalseand the key should be omitted rather than emitted asfalse. The JS handler'sconfig.allow_workflows === truecheck handles the absent-key case correctly. -
Dual baseline (
origin/HEADvsorigin/${baseBranch}) — After thorough analysis, both choices are intentionally correct for their respective layers. The apply-time pre-flight (origin/HEAD) must be more conservative (exclude everything GitHub already accepted) because GitHub's push scope check is based on ALL reachable commits, not just the PR delta. The MCP-side content-policy check (origin/${baseBranch}) correctly limits to what the agent actually introduced. -
Synchronous
execGitSyncin the MCP handler — Consistent with existingexecGitSyncusage insafe_outputs_handlers.cjs(lines 796, 892, 1229). Not a regression. -
Test assertions in the
allow_workflows: trueskip test — ThemockGithub.rest.pulls.createmock is set up inbeforeEachand returns{ data: { number: 999, html_url: "..." } }, soresult.success === trueis a valid assertion. ✓
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 186.1 AIC · ⌖ 7.3 AIC · ⊞ 4.9K
| // before any transport artifacts are generated. | ||
| if (Array.isArray(pushConfig.allowed_files) && pushConfig.allowed_files.length > 0) { | ||
| try { | ||
| const branchHistoryFiles = execGitSync(["log", "--name-only", "--pretty=format:", `origin/${baseBranch}..${entry.branch}`], { cwd: pushGitCwd }) |
There was a problem hiding this comment.
Missing -- end-of-options terminator in git log range argument.
The command:
execGitSync(["log", "--name-only", "--pretty=format:", `origin/${baseBranch}..${entry.branch}`], ...)does not have a trailing "--". While git prohibits .. in ref names and spawnSync prevents shell injection, adding "--" after the range is a defensive best practice — it prevents git from ever treating the last positional argument as a pathspec.
Suggested fix:
execGitSync(["log", "--name-only", "--pretty=format:", `origin/${baseBranch}..${entry.branch}`, "--"], ...)@copilot please address this.
| */ | ||
| async function detectWorkflowFileChanges(exec, gitOptions) { | ||
| try { | ||
| const result = await exec.getExecOutput("git", ["log", "--name-only", "--pretty=format:", "HEAD", "--not", "origin/HEAD", "--", ".github/workflows/"], { ...gitOptions, ignoreReturnCode: true }); |
There was a problem hiding this comment.
JSDoc cross-reference: explain why origin/HEAD differs from the MCP-side origin/${baseBranch} baseline.
The function comment explains what origin/HEAD is and what happens when it's unresolvable, but not why it's intentionally different from the origin/${baseBranch}..branch range used in safe_outputs_handlers.cjs (line 1262).
The reason matters: this check is about GitHub's push-scope enforcement, which inspects all commits reachable from the pushed ref — not just the PR delta. Excluding origin/HEAD (the default branch) removes commits GitHub has already accepted. Excluding only origin/ would leave feature-branch commits in scope and potentially miss scope violations on non-default base branches.
Suggested addition to JSDoc:
* Note: this uses `origin/HEAD` (the remote default branch set by `actions/checkout`), not
* `origin/<baseBranch>` (the PR's target branch). The two differ for PRs based on non-default
* branches. `origin/HEAD` is the correct baseline here because GitHub's push-scope check
* considers ALL commits reachable from a newly-created ref; `origin/HEAD` excludes only commits
* that GitHub has already accepted. The MCP-side allowed_files check (safe_outputs_handlers.cjs)
* uses `origin/<baseBranch>` because it enforces content policy, not push-scope acceptance.
@copilot please address this.
🤖 PR Triage — §28486872548
Score breakdown: Impact 35/50 · Urgency 20/30 · Quality 14/20 Rationale: Security-critical safe-outputs path changes (+194/-8). Adds pre-flight workflow scope checks and full-branch Labels applied:
|
|
@copilot run pr-finisher skill |
… strings Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…r threading Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All review threads have been addressed. Here's what was done across the commits:
CI will need a human re-trigger (close/reopen or |
|
@copilot fix https://github.com/github/gh-aw/actions/runs/28491425152/job/84449104694 review change for consistency, clarity, efficiency |
…eck failure; remove unnecessary block scoping and end-else comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. Two issues addressed: CI fix: Both Clarity/consistency: Removed the unnecessary |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…afe-output-server-update Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…r.go error messages to fix lint-errors CI Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed. The The fix in the latest commit:
|
|
🎉 This pull request is included in a new release. Release: |
When a Copilot agent branch contains older commits that touched
.github/workflows/**, pushing a new review or fallback branch ref fails — even when the current changeset only modifies allowed files (e.g..changeset/*.md). GitHub validates the full commit history reachable from a pushed ref, not just the incremental diff. The existingallowed_filescheck only inspects the net patch, so violations in historical commits were invisible until push-time rejection.Changes
safe_outputs_handler_registry.go: Exposeallow_workflowsin thepush_to_pull_request_branchhandler config so the JS handler can read whether the push token carriesworkflows: write.push_to_pull_request_branch.cjs: AdddetectWorkflowFileChanges()— runsgit log HEAD --not origin/HEAD -- .github/workflows/to detect workflow file changes in the branch history beyond the remote default. Add pre-flight checks before both the review-branch and fallback-branch push paths; whenallow_workflowsis false and workflow history is detected, return a typedworkflows_scope_requirederror before anygit checkout -b/git pushside effects.safe_outputs_handlers.cjs: Add a full-branchallowed_filescheck at MCP tool-call time usingorigin/<baseBranch>..<branch>. Failures are returned to the agent immediately so it can fix the changeset before retrying, rather than discovering the violation at apply time. Files matchingexcluded_filesare exempt (they are stripped from the patch at generation time). The check is non-fatal iforigin/<baseBranch>is unavailable locally.push_to_pull_request_branch.test.cjs: Three new tests covering pre-flight fires early (nogit pushattempted), pre-flight skipped whenallow_workflows: true, and correctedgit logmock in the existing fallback-PR test.