Skip to content

create-pull-request: keep branch push on protected-files fallback; reserve push-failed issue for real push errors#33840

Closed
Copilot wants to merge 9 commits into
mainfrom
copilot/protected-files-fallback-issue
Closed

create-pull-request: keep branch push on protected-files fallback; reserve push-failed issue for real push errors#33840
Copilot wants to merge 9 commits into
mainfrom
copilot/protected-files-fallback-issue

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 21, 2026

With protected-files: fallback-to-issue and default patch-format: bundle, the flow treated fallback as an implicit push failure, skipped branch push, and generated the push-failed issue template (no branch/compare link). This change makes fallback-to-issue preserve branch push behavior and only use push-failed messaging when push actually fails.

  • Behavioral fix: push is no longer short-circuited by protected-files fallback

    • Removed the manifestProtectionFallback-based skip in both transport paths:
      • bundle replay path
      • patch (git am) path
    • Branch push now runs in fallback-to-issue mode, so reviewers get a branch-backed compare surface in GitHub UI.
  • Template selection semantics corrected

    • manifest_protection_create_pr_fallback.md (compare URL path) is now the normal fallback when push succeeds.
    • manifest_protection_push_failed_fallback.md is now used only for actual push errors (permission/rejection/etc.), via existing error handling.
  • Focused test expectation updates

    • Updated protected-files fallback tests (bundle + patch transport) to assert:
      • push is attempted
      • fallback issue includes compare URL
      • artifact-only gh run download / git am instructions are not used on successful push
// Before: fallback implied push failure
if (manifestProtectionFallback) {
  manifestProtectionPushFailedError = new Error("Push skipped ...");
} else {
  await pushSignedCommits(...);
}

// After: always attempt push; fallback template depends on real push outcome
try {
  await pushSignedCommits(...);
} catch (pushError) {
  if (manifestProtectionFallback) manifestProtectionPushFailedError = pushError;
  // ... existing push-failure fallback handling
}

pr-sous-chef: requested branch update (run id: 26252608888).

Generated by 👨‍🍳 PR Sous Chef · ● 173.6K ·

Copilot AI and others added 3 commits May 21, 2026 18:49
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>
Copilot AI changed the title [WIP] Fix branch link missing in fallback issue for protected files create-pull-request: keep branch push on protected-files fallback; reserve push-failed issue for real push errors May 21, 2026
Copilot AI requested a review from pelikhan May 21, 2026 18:56
@pelikhan pelikhan marked this pull request as ready for review May 21, 2026 19:11
Copilot AI review requested due to automatic review settings May 21, 2026 19:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes protected-files: fallback-to-issue behavior in create-pull-request so that branch push is still attempted (yielding a GitHub compare surface when successful), and so that the “push failed” fallback template is used only when an actual push error occurs.

Changes:

  • Removes the “skip push” behavior previously triggered by protected-files fallback in both bundle and patch transport paths.
  • Adjusts fallback issue template semantics: compare-url template on successful push; push-failed template only on real push errors.
  • Updates protected-files fallback tests to assert push attempts and compare URL presence; updates generated workflow lock file.
Show a summary per file
File Description
actions/setup/js/create_pull_request.cjs Removes protected-files fallback push short-circuit; refactors push error handling to distinguish real push failures from protected-files fallback.
actions/setup/js/create_pull_request.test.cjs Updates protected-files fallback tests to assert branch push occurs and compare URL is included in the review issue body.
.github/workflows/pr-code-quality-reviewer.lock.yml Regenerated lock workflow output reflecting updated compiled template/import set.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment on lines +1429 to +1432
if (!fallbackAsIssue) {
const error = `Failed to push changes: ${pushError instanceof Error ? pushError.message : String(pushError)}`;
return { success: false, error, error_type: "push_failed" };
}
Comment on lines 1674 to 1680
} catch (pushError) {
// Push failed - create fallback issue instead of PR (if fallback is enabled)
core.error(`Git push failed: ${pushError instanceof Error ? pushError.message : String(pushError)}`);

if (manifestProtectionFallback) {
// Push failed specifically for a protected-file modification. Don't create
// a generic push-failed issue — fall through to the manifestProtectionFallback
@github-actions
Copy link
Copy Markdown
Contributor

``
Please wait for checks to settle, then revisit whether a branch update is needed.

Generated by 👨‍🍳 PR Sous Chef · ● 2.6M ·

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot merge main and recompile , lint go

Copilot AI and others added 2 commits May 21, 2026 19:59
…es-fallback-issue

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 21, 2026

@copilot merge main and recompile , lint go

Done in f2d856c (plus merge commit 3d8fddc): merged latest main, ran make recompile, and ran make golint (passes).

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot merge main and recompile

…es-fallback-issue

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 21, 2026

@copilot merge main and recompile

Done in 74e50c6: merged latest main and ran make recompile.

@pelikhan
Copy link
Copy Markdown
Collaborator

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 21, 2026

@copilot lint js https://github.com/github/gh-aw/actions/runs/26250743615/job/77264146546#step:6:1

update wasm

Done in cec9836: fixed the lint-js failure (Prettier formatting in actions/setup/js/create_pull_request.cjs) and updated wasm golden output (pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden).

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot merge main and recompile

…es-fallback-issue

# Conflicts:
#	.github/workflows/ai-moderator.lock.yml
#	.github/workflows/changeset.lock.yml
#	.github/workflows/codex-github-remote-mcp-test.lock.yml
#	.github/workflows/daily-cache-strategy-analyzer.lock.yml
#	.github/workflows/daily-fact.lock.yml
#	.github/workflows/daily-observability-report.lock.yml
#	.github/workflows/duplicate-code-detector.lock.yml
#	.github/workflows/grumpy-reviewer.lock.yml
#	.github/workflows/issue-arborist.lock.yml
#	.github/workflows/necromancer.lock.yml
#	.github/workflows/schema-feature-coverage.lock.yml
#	.github/workflows/smoke-call-workflow.lock.yml
#	.github/workflows/smoke-codex.lock.yml
#	pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 21, 2026

@copilot merge main and recompile

Done in 38777816c7: merged latest main into this branch and ran make recompile.

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot do a security review for agentic threats of this approach

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 21, 2026

@copilot do a security review for agentic threats of this approach

Security review of this approach (push even in protected-files: fallback-to-issue):

  • High risk (agentic): unreviewed workflow execution on branch push. If protected files include .github/workflows/** and this job/token can push them, an attacker-controlled patch can land on a branch and trigger on: push workflows before human PR review.
  • Medium risk: copy/paste shell injection in manual fallback commands. The fallback issue templates embed values like title/branch in shell snippets; if not shell-escaped, a crafted title can break quoting and execute unintended commands when copied.
  • Medium risk: reviewer-context downgrade on bundle push failure. In the bundle path, push failure still routes to generic push-failed fallback instead of protected-file-specific push-failed messaging, which weakens reviewer awareness for protected-file changes.

Recommended guardrails:

  1. Keep token/job permissions least-privilege (avoid workflows: write unless strictly required).
  2. Add branch/actor guards on on: push workflows so bot-created fallback branches don’t run sensitive jobs by default.
  3. Shell-escape interpolated values in fallback command snippets.
  4. Align bundle push-failure handling with protected-file push-failed template path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

create-pull-request: protected-files fallback-to-issue + bundle transport drops the branch (no link in fallback issue)

3 participants