Add fork-backed safe-output pull request support#45909
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…backed-prs # Conflicts: # .github/workflows/avenger.lock.yml # .github/workflows/hourly-ci-cleaner.lock.yml # .github/workflows/skillet.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
There was a problem hiding this comment.
Pull request overview
Adds automation-fork support for safe-output pull request creation and updates.
Changes:
- Adds fork head repository and credential configuration.
- Routes branch operations and metadata toward configured forks.
- Updates schemas, types, tests, and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_handler_registry.go |
Emits fork configuration. |
pkg/workflow/safe_outputs_handler_manager_token_test.go |
Tests token precedence. |
pkg/workflow/push_to_pull_request_branch.go |
Parses fork update settings. |
pkg/workflow/github_token.go |
Resolves checkout credentials. |
pkg/workflow/create_pull_request.go |
Defines fork creation settings. |
pkg/workflow/compiler_safe_outputs_config_test.go |
Tests configuration pass-through. |
pkg/parser/schemas/main_workflow_schema.json |
Adds schema fields. |
docs/src/content/docs/specs/safe-outputs-specification.md |
Specifies fork behavior. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents configuration and credentials. |
actions/setup/js/types/safe-outputs-config.d.ts |
Adds TypeScript declarations. |
actions/setup/js/safe_outputs_handlers.cjs |
Adds head repository metadata. |
actions/setup/js/push_to_pull_request_branch.test.cjs |
Tests configured-fork updates. |
actions/setup/js/push_to_pull_request_branch.cjs |
Implements fork branch updates. |
actions/setup/js/push_signed_commits.cjs |
Supports explicit push remotes. |
actions/setup/js/create_pull_request.test.cjs |
Tests owner-qualified heads. |
actions/setup/js/create_pull_request.cjs |
Implements fork-backed PR creation. |
actions/setup/js/create_pull_request_helpers.cjs |
Supports qualified compare heads. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 16/17 changed files
- Comments generated: 13
- Review effort level: Medium
|
|
||
| // Per-config PAT tokens take highest precedence (overrides GitHub App) | ||
| perConfigToken := createPRToken | ||
| perConfigToken := createPRHeadToken |
| * @returns {Promise<string | undefined>} SHA of the commit that landed on the target branch | ||
| */ | ||
| async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, cwd, gitAuthEnv, signedCommits = true, allowGitPushFallback = true, resolvedTemporaryIds, currentRepo, validationConfig }) { | ||
| async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, cwd, gitAuthEnv, pushRemoteUrl, pushToken, signedCommits = true, allowGitPushFallback = true, resolvedTemporaryIds, currentRepo, validationConfig }) { |
|
|
||
| // SECURITY: Check if this is a fork PR - only explicitly configured automation-owned | ||
| // forks are eligible for updates. | ||
| const { isFork, reason: forkReason } = detectForkPR(pullRequest); |
| let previousExtraheaders = []; | ||
| let overrideApplied = false; | ||
| try { | ||
| previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, token); |
| 8. **Distinct Upstream and Head Repositories**: `target-repo` identifies the upstream repository that receives the pull request and owns the base branch. `head-repo`, when configured, identifies the repository that receives the pushed branch. When `head-repo` is omitted, the head repository defaults to `target-repo`. | ||
| 9. **Owner-Qualified Head Reference**: When `head-repo` differs from `target-repo`, the created pull request MUST use an owner-qualified head reference identifying the head repository owner and pushed branch. Unqualified same-name branch references MUST NOT be used in fork-backed mode. | ||
| 10. **Ephemeral Fork Branch Model**: When `head-repo` differs from `target-repo`, implementations SHOULD create or refresh an ephemeral branch in `head-repo` from the resolved upstream base SHA, apply the agent changes, and open the pull request back to the upstream base. Implementations MAY support explicit synchronization of that ephemeral branch with a newer upstream base, but implicit reuse of arbitrary pre-existing fork branches MUST NOT occur. | ||
| 11. **Summary and Manifest Provenance**: Successful executions MUST record `upstream_repo`, `head_repo`, `base_sha`, `pushed_head_sha`, and `credential_role` in the safe-output summary and machine-readable manifest. |
| - When `safe-outputs.push-to-pull-request-branch.target` is `"*"`, requests MUST include `pull_request_number`. | ||
| - The handler MUST refuse pushes unless the resolved pull request head repository exactly matches the configured `head-repo` (or `target-repo` when `head-repo` is omitted) | ||
| - Arbitrary contributor forks MUST remain unsupported write targets even when the upstream repository itself is allowlisted | ||
| - Successful executions MUST record `upstream_repo`, `head_repo`, `base_sha`, `pushed_head_sha`, and `credential_role` in the safe-output summary and machine-readable manifest |
| core.info(`Fork PR check: not a fork (${forkReason})`); | ||
| } | ||
| core.info(`Fork PR check: not a fork (${forkReason})`); | ||
| const pushRemoteUrl = pushRepo.toLowerCase() === itemRepo.toLowerCase() ? "" : `${(process.env.GITHUB_SERVER_URL || "https://github.com").replace(/\/+$/, "")}/${pushRepo}.git`; |
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commits. How it works:
Example configuration: safe-outputs:
create-pull-request:
target-repo: microsoft/vscode
head-repo: automation-owner/vscode
head-github-app:
client-id: ${{ vars.FORK_APP_CLIENT_ID }}
private-key: ${{ secrets.FORK_APP_PRIVATE_KEY }} |
…backed-prs # Conflicts: # .github/workflows/avenger.lock.yml # .github/workflows/hourly-ci-cleaner.lock.yml # .github/workflows/release.lock.yml # .github/workflows/skillet.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in
All review threads were addressed in prior commits ( |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on correctness and test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Missing git remote for fork checkout —
branchRemoteRefreferencesrefs/remotes/gh-aw-head/<branch>but agh-aw-headremote is never added;git rev-parse --verifyandgit checkout -Bwill fail on any real fork-backed run. - Duplicated override/restore boilerplate — the
withGitHubHostTokenhelper introduced inpush_to_pull_request_branch.cjsis the right abstraction, but four files each reimplement the same try/finally block instead of sharing it fromgit_auth_helpers.cjs. - Null
head.repobypass — when GitHub returns a nullhead.repo(deleted fork), the expected-head-repo check is skipped rather than blocked; the guard further down may also miss it. - Test allowlist coverage — both new fork tests use positive paths only; there are no negative tests to verify that an unconfigured
head-repovalue is actually rejected by the allowlist. - Manifest
head_repopre-set vs actual —safe_outputs_handlers.cjssetsentry.head_repofrom config before the handler runs; it should be taken from the handler result to stay accurate.
Positive Highlights
- ✅ The
detectForkPRsecurity gate is preserved and the new path only opens whenhead-repois explicitly configured — good layered defence. - ✅ Comprehensive Go tests for schema pass-through and token generation.
- ✅
getPullRequestHeadRefcorrectly uses the owner-qualified form (owner:branch) for all PR-creation paths including fallback issue bodies. - ✅ The
withGitHubHostTokenhelper inpush_to_pull_request_branch.cjsis a clean abstraction — just needs to be moved to the shared module.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 128.8 AIC · ⌖ 5.39 AIC · ⊞ 6.7K
Comment /matt to run again
🤖 PR Triage
Rationale: Fork-backed safe-output PR support (25 files, +1085/-244). Significant new feature enabling cross-org automation. Well-described with config examples. AI bot reviewed (COMMENTED). CI unknown. Requires careful review of auth/token handling and fork PR semantics.
|
|
@copilot review #45909 (review) |
…g; add missing negative test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed. Addressing the actionable correctness issue from the Matt Pocock Skills review: Fixed (item #3 — null Tests updated (item #4): Updated the existing deleted-head-repo test to assert against the new early rejection message, and added a second negative test that exercises the Fixed in The other findings:
|
|
@copilot update JSON schema and specification with head-github-app |
…push-to-pull-request-branch Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. Updated
The specification already documented both fields; this brings the schema into alignment with the implementation and docs. |
|
@copilot add integration git test |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added 4 integration git tests in the
All 58 tests pass. |
Safe outputs could only create and update PRs where the pushed branch lived in the same repository as the PR target. This change adds first-class support for upstream PRs whose head branch is pushed to an explicitly configured automation-owned fork, while keeping arbitrary contributor forks blocked.
Config surface
head-repoandhead-github-tokento:safe-outputs.create-pull-requestsafe-outputs.push-to-pull-request-branchCreate PR against upstream, push branch to fork
target-repoas the upstream/base repository.head-repowhen configured.owner:branch) when the head repo differs from the target repo.contents: write.Follow-up updates for fork-backed PRs
push-to-pull-request-branch.head.repo.full_nameexactly matches the configuredhead-repo.Git/auth behavior
Manifest and result metadata
head_repoin emitted safe-output entries and returned results so downstream summaries/manifests can distinguish upstream target vs fork head.Docs/spec
Example configuration: