Repository navigation
Fix pull request creation from same-organization forks - #62720
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Safeoutputs CLI write calls were denied by the sandbox permission layer while reviewing PR #62720.
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the API requirement and includes focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds head_repo to cross-repository pull request requests, fixing same-organization fork creation.
Changes:
- Propagates the validated head repository to the PR helper.
- Includes
head_repoonly for cross-repository PRs. - Adds regression coverage for same-organization forks.
| File | Description |
|---|---|
actions/setup/js/create_pull_request.test.cjs |
Tests same-organization fork payloads. |
actions/setup/js/create_pull_request.cjs |
Passes the cross-repository head repository. |
actions/setup/js/create_or_update_pull_request.cjs |
Adds optional head_repo to the REST request. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
🧪 Test Quality Sentinel Analysis — PR #62720SummaryThis PR adds targeted regression coverage for a critical cross-repository PR creation bug (#62680). The test quality is Excellent (90/100). Test Changes1 new behavioral test added in Test Details
Test Code: it("should include head_repo when the head and target repositories have the same owner", async () => {
const { main } = require("./create_pull_request.cjs");
const handler = await main({
allow_empty: true,
"head-repo": "test-owner/docs-automation",
allowed_repos: ["test-owner/test-repo", "test-owner/docs-automation"],
});
await handler({ title: "Test PR", body: "body", branch: "my-feature" }, {});
const createCall = global.github.rest.pulls.create.mock.calls[0][0];
expect(createCall.head).toMatch(/^test-owner:my-feature(?:-[0-9a-f]+)?$/);
expect(createCall.head_repo).toBe("test-owner/docs-automation");
});Strengths:
Quality Metrics
Production Changes
Verdict✅ APPROVE — This is a high-quality, focused test that directly addresses the reported bug and provides protection against regression in a complex edge case. Test Quality Sentinel • Coverage: JavaScript (vitest) • Commits analyzed: 1 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100 Excellent. 1 new behavioral test (100% design tests, 0% implementation). No violations. Test directly addresses bug #62680 with targeted edge-case regression coverage for same-organization fork scenario.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 22 AIC · ⌖ 9.44 AIC · ⊞ 8.2K
Comment /review to run again
There was a problem hiding this comment.
Verdict
I didn't find a blocking issue in the changed lines.
Review notes
The change passes head_repo only when the head repository differs from the target repository, which matches the GitHub API case this PR is fixing. The added regression test covers the same-owner fork topology that previously failed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 31.5 AIC · ⌖ 6.98 AIC · ⊞ 20.3K
Comment /review to run again
There was a problem hiding this comment.
Reviewed with Impeccable harden/audit modes (bug-fix category: cross-org fork PR creation).
Findings: None blocking. The fix is minimal, correctly scoped, and covered by a regression test:
create_or_update_pull_request.cjs: conditionally addshead_repoonly when present — no behavior change for the existing single-repo/cross-owner-fork paths.create_pull_request.cjs:headRepois only passed whenpushRepodiffers fromitemRepo(case-insensitive compare), matching the existinggetPullRequestHeadReflogic used elsewhere in the file for consistency.- New test
should include head_repo when the head and target repositories have the same ownercorrectly exercises the same-org-fork scenario described in the linked issue.
No missing error/edge-case handling identified for this narrow fix (undefined head_repo is safely omitted via the spread guard, so the default same-repo path is unaffected).
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
codeload.hub.lumenfield.workhub.lumenfield.work
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "codeload.github.com"
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 47.9 AIC · ⌖ 13 AIC · ⊞ 8.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. The fix itself is minimal, correct, and well-targeted: head_repo is now passed through only when needed, matching GitHub's API requirement for same-organization fork PRs.
📋 Key Themes & Highlights
Key Themes
- Scope gap: The identical same-org-fork topology bug appears to still exist in
push_to_pull_request_branch.cjs(twopulls.createcall sites building an owner-qualified head ref withouthead_repo). Flagged inline for confirmation — may be intentionally out of scope for this PR since #62680 was specific tocreate_pull_request.cjs. - Test placement: The new regression test lives in the higher-level
create_pull_request.test.cjsintegration suite rather than a focused unit test oncreateOrUpdatePullRequestitself, so future direct callers of the helper wouldn't be protected by this specific assertion.
Positive Highlights
- ✅ Root cause correctly identified and fixed with a minimal, surgical change
- ✅ Conditional spread (
...(headRepo ? { head_repo: headRepo } : {})) avoids sending an unnecessary field for the common same-repo case - ✅ New test clearly distinguishes the same-org-fork case from the existing cross-owner-fork test, with a descriptive name
No blocking issues — flagging as COMMENT given the possible related gap in push_to_pull_request_branch.cjs.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 63.9 AIC · ⌖ 14.6 AIC · ⊞ 10.1K
Comment /matt to run again
| const { githubClient, repoParts, title, body, branchName, baseBranch, draft } = options; | ||
| const { githubClient, repoParts, title, body, branchName, headRepo, baseBranch, draft } = options; | ||
| return withRetry( | ||
| () => |
There was a problem hiding this comment.
[/tdd] Good targeted fix, but there's no regression test at this module's own level — the only coverage lives in create_pull_request.test.cjs, one layer up. If another caller starts using this helper directly, a regression here could slip through untested.
💡 Suggested addition
A small dedicated unit test for createOrUpdatePullRequest (e.g. create_or_update_pull_request.test.cjs) asserting that head_repo is included/omitted based on the headRepo option would decouple this guarantee from the higher-level integration test and make the contract explicit and directly discoverable.
@copilot please address this.
There was a problem hiding this comment.
Added direct createOrUpdatePullRequest coverage for both inclusion and omission of head_repo in 7795c14.
There was a problem hiding this comment.
Added direct createOrUpdatePullRequest coverage for both including and omitting head_repo. Implemented in a3b63d2.
| @@ -2555,6 +2555,7 @@ ${issueSafeFallbackFooter}`; | |||
| title, | |||
| body, | |||
| branchName: getPullRequestHeadRef(branchName), | |||
There was a problem hiding this comment.
[/diagnosing-bugs] This fixes the root cause for create_pull_request.cjs, but the identical same-organization-fork topology bug appears unaddressed at the two githubClient.rest.pulls.create call sites in actions/setup/js/push_to_pull_request_branch.cjs (reviewHeadRef/fallbackHeadRef, ~lines 1365 & 1567). Both derive the qualified head ref the same way this file did before the fix (pushRepoParts.owner:branch when pushRemoteUrl is set), but neither call passes head_repo, so PR creation there would fail for the same-org-fork case this PR fixes here.
💡 Why this matters
Since #62680 was about create_pull_request.cjs specifically, this may be intentionally out of scope — but if push_to_pull_request_branch.cjs's review/fallback PR creation can hit the same same-org-fork topology (its pushRepo/itemRepo resolution logic is structurally identical), it will hit the exact same GitHub API error. Worth confirming whether that path is reachable with a same-org fork and, if so, applying the analogous head_repo fix there too.
@copilot please address this.
There was a problem hiding this comment.
Confirmed the analogous review and fallback PR paths are reachable with same-organization forks. Commit 7795c14 now passes head_repo for both paths and covers each scenario.
There was a problem hiding this comment.
Applied the analogous head_repo payload to both review and fallback PR creation paths, with same-organization fork coverage for each. Implemented in a3b63d2.
|
@CopIoT run pr-finisher skill |
|
Please address the remaining review feedback in one pass, including the suggested direct helper-level regression coverage and confirming whether the analogous same-organization fork path in Open review follow-ups (newest first):
Run: https://github.com/github/gh-aw/actions/runs/35769351980
|
|
Please close out the remaining follow-up from the earlier review pass: confirm whether the analogous same-organization fork path in Run: https://github.com/github/gh-aw/actions/runs/35770424354
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
The push flow can create review and fallback PRs from a configured same-organization fork. Commit 7795c14 adds
Completed the helper-level coverage and same-organization fork parity fix in 7795c14; local validation and the pr-finisher review pass completed. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…handler' into copilot/fix-create-pull-request-handler # Conflicts: # actions/setup/js/create_or_update_pull_request.test.cjs # actions/setup/js/push_to_pull_request_branch.cjs # actions/setup/js/push_to_pull_request_branch.test.cjs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot update safe output specs and spec tests |
…tputs spec Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated the safe-outputs spec (Section 7.1, new requirement 12, v1.29.5 changelog entry) to document that |
|
Open review follow-ups (newest first): Run: https://github.com/github/gh-aw/actions/runs/35776888307
|
Confirmed the push flow needs the same handling: both review and fallback PR creation paths now include |
|
🎉 This pull request is included in a new release. Release: |
Cross-repository PR creation failed when the base and fork shared an organization because
head: owner:branchdid not identify the source repository. GitHub requireshead_repofor this topology.head_repoonly when the head and target repositories differ.Run: https://github.com/github/gh-aw/actions/runs/35770424354