Skip to content

fix: apply_samples derivePrHeadRef uses target-repo config for siderepo PR lookups #41295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 2 commits into
base: main
Choose a base branch
from
+68 −4
Open

fix: apply_samples derivePrHeadRef uses target-repo config for siderepo PR lookups #41295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df35786
Initial plan
Copilot Jun 24, 2026
9cdc876
fix: derivePrHeadRef uses target-repo from safe-outputs config for si…
Copilot Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-apply-samples-siderepo-target-repo.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions actions/setup/js/apply_samples.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,14 @@ async function derivePrHeadRef(entry) {
return directRef.trim();
}

// Determine the target repo for any API lookups. Prefer the entry's repo if
// the sample sets one (cross-repo workflows), otherwise fall back to
// GITHUB_REPOSITORY.
const repoSlug = (typeof entry.arguments.repo === "string" && entry.arguments.repo.trim()) || process.env.GITHUB_REPOSITORY || "";
// Determine the target repo for any API lookups.
// Resolution order:
// a. entry.arguments.repo — explicit per-sample override (cross-repo workflows).
// b. target-repo from the safe-outputs config file (GH_AW_SAFE_OUTPUTS_CONFIG_PATH)
// for the tool — covers siderepo workflow_dispatch where the sample arguments
// carry `pull_request_number` but not a `repo` override (issue #41292).
// c. GITHUB_REPOSITORY — host repo fallback.
const repoSlug = (typeof entry.arguments.repo === "string" && entry.arguments.repo.trim()) || readConfiguredTargetRepo(entry.tool) || process.env.GITHUB_REPOSITORY || "";

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/zoom-out] The single-line || chain is harder to scan than the equivalent if/else block used in resolvePatchWorkspace (lines 308–312).

Keeping the two functions stylistically consistent would lower cognitive load when future contributors need to modify the resolution order.

💡 Suggested style alignment with resolvePatchWorkspace 
let repoSlug = "";
if (typeof entry.arguments.repo === "string" && entry.arguments.repo.trim()) {
  repoSlug = entry.arguments.repo.trim();     // a: explicit per-sample override
} else {
  repoSlug = readConfiguredTargetRepo(entry.tool); // b: config target-repo
}
if (!repoSlug) {
  repoSlug = process.env.GITHUB_REPOSITORY || ""; // c: host repo fallback
}

This matches the three-tier structure that is already documented in the comment block above.

const [owner, repo] = repoSlug.split("/");
if (!owner || !repo) return null;

Expand Down
55 changes: 55 additions & 0 deletions actions/setup/js/apply_samples.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,61 @@ describe("apply_samples.cjs preStagePatch (create_pull_request / push_to_pull_re
}
});

it("derives push_to_pull_request_branch branch using target-repo from safe-outputs config (issue #41292 siderepo workflow_dispatch)", async () => {
// Reproduces the siderepo failure: a workflow_dispatch provides
// `pull_request_number` but no `repo` override in the sample arguments.
// The safe-outputs config carries `target-repo: "githubnext/gh-aw-side-repo"`,
// which derivePrHeadRef must use when building the PR fetch URL.
const workspace = makeTempDir("gh-aw-prestage-push-siderepo-");
initRepo(workspace, "main");

const headRef = "feat/siderepo-pr-branch";
const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValueOnce({
ok: true,
status: 200,
json: async () => ({ head: { ref: headRef } }),
});

const configPath = path.join(workspace, "config.json");
fs.writeFileSync(
configPath,
JSON.stringify({
push_to_pull_request_branch: { "target-repo": "githubnext/gh-aw-side-repo" },
})
);

const prevBase = process.env.GH_AW_CUSTOM_BASE_BRANCH;
const prevEvent = process.env.GITHUB_EVENT_PATH;
const prevRepo = process.env.GITHUB_REPOSITORY;
const prevConfig = process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH;
delete process.env.GITHUB_EVENT_PATH; // no event; rely on config
process.env.GH_AW_CUSTOM_BASE_BRANCH = "main";
process.env.GITHUB_REPOSITORY = "githubnext/gh-aw-test"; // host repo (wrong repo)
process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH = configPath;
try {
const entry = {
tool: "push_to_pull_request_branch",
// No `repo` override — agent emits only pull_request_number.
arguments: { message: "Multi-commit test push from Copilot in side repo", pull_request_number: 447 },
sidecars: { patch: newFileDiff("src/siderepo-feature.py", "# side repo\n") },
};
await preStagePatch(entry, 0, workspace);
expect(git(["rev-parse", "--abbrev-ref", "HEAD"], workspace).trim()).toBe(headRef);
// Must fetch from the side repo, NOT the host repo.
expect(fetchSpy).toHaveBeenCalledWith(expect.stringContaining("/repos/githubnext/gh-aw-side-repo/pulls/447"), expect.anything());
expect(fetchSpy).not.toHaveBeenCalledWith(expect.stringContaining("/repos/githubnext/gh-aw-test/pulls/447"), expect.anything());

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new test exercises path 3 of derivePrHeadRef (pull_request_number with no event payload). Path 2 — issue_comment/pull_request_review_comment events where payload.issue.pull_request is set — also resolves through the same repoSlug and would have fetched from the wrong repo before this fix.

💡 Suggested complementary test

Add a second it block that populates GITHUB_EVENT_PATH with an issue-comment payload containing issue.pull_request and issue.number, sets push_to_pull_request_branch.target-repo in the config, and asserts that fetchSpy hits githubnext/gh-aw-side-repo/pulls/<issue.number>. This nails down both paths that consume repoSlug and guards against any future regression specific to the event-driven flow.

The current test already does the heavy lifting; this would be a near-copy with a different event payload wired in.

} finally {
fetchSpy.mockRestore();
if (prevBase === undefined) delete process.env.GH_AW_CUSTOM_BASE_BRANCH;
else process.env.GH_AW_CUSTOM_BASE_BRANCH = prevBase;
if (prevEvent !== undefined) process.env.GITHUB_EVENT_PATH = prevEvent;
if (prevRepo === undefined) delete process.env.GITHUB_REPOSITORY;
else process.env.GITHUB_REPOSITORY = prevRepo;
if (prevConfig === undefined) delete process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH;
else process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH = prevConfig;
}

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] Missing edge-case: arguments.repo overriding the config target-repo is untested.

The new test correctly covers tier-2 (config → side repo). But there is no assertion that tier-1 (arguments.repo) still wins when both are set. Without it, a future refactor could silently break the priority order.

💡 Suggested complementary test 
it("entry.arguments.repo takes precedence over target-repo in safe-outputs config", async () => {
  // setup: write config with target-repo, but also set entry.arguments.repo
  const entry = {
    tool: "push_to_pull_request_branch",
    arguments: { repo: "explicit/override-repo", pull_request_number: 123 },
    sidecars: { patch: newFileDiff("src/file.py", "# content\n") },
  };
  await preStagePatch(entry, 0, workspace);
  // tier-1 explicit repo must win over config target-repo
  expect(fetchSpy).toHaveBeenCalledWith(
    expect.stringContaining("/repos/explicit/override-repo/pulls/123"),
    expect.anything()
  );
  expect(fetchSpy).not.toHaveBeenCalledWith(
    expect.stringContaining("/repos/githubnext/gh-aw-side-repo/pulls/123"),
    expect.anything()
  );
});

});

it("is a no-op when the sample tool isn't in the patch-sidecar set", async () => {
// We assert this at the driver level (PATCH_SIDECAR_TOOLS gate in main()),
// but preStagePatch itself should also be a no-op when called with an
Expand Down
Loading