Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions actions/setup/js/assign_agent_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,13 @@ async function assignAgentToIssue(
issue_number: issueNumber,
assignees: [assignee],
};
const agentAssignment = {};
if (pullRequestRepoSlug != null) agentAssignment.target_repo = pullRequestRepoSlug;
if (baseBranch != null) agentAssignment.base_branch = baseBranch;
if (customInstructions != null) agentAssignment.custom_instructions = customInstructions;
if (customAgent != null) agentAssignment.custom_agent = customAgent;
if (model != null) agentAssignment.model = model;
if (Object.keys(agentAssignment).length > 0) assignParams.agent_assignment = agentAssignment;
await githubClient.request("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", assignParams);
return true;
} catch (error) {
Expand Down
90 changes: 90 additions & 0 deletions actions/setup/js/assign_agent_helpers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,96 @@ describe("assign_agent_helpers.cjs", () => {
expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("GH_AW_AGENT_TOKEN"));
expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("github.github.com/gh-aw/reference/copilot-cloud-agent/#authentication"));
});

it("should not include agent_assignment when all optional fields are null", async () => {
const mockRequest = vi.fn().mockResolvedValue({ status: 201 });
const restClient = { request: mockRequest };

await assignAgentToIssue("id", "copilot-swe-agent[bot]", [], "copilot", null, null, null, null, null, restClient, taskContext);

expect(mockRequest).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", {

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.

No test coverage for empty-string inputs — the empty-string edge case (which the current implementation silently swallows) is not exercised by any test.

💡 Suggested test to add
it("should not include agent_assignment field for empty-string model", async () => {
  const mockRequest = vi.fn().mockResolvedValue({ status: 201 });
  const restClient = { request: mockRequest };

  await assignAgentToIssue(
    "id", "copilot-swe-agent[bot]", [], "copilot",
    null, "", null, null, null, restClient, taskContext
  );

  // With truthy check: agent_assignment is omitted (silent data loss)
  // With null check fix: agent_assignment contains { model: "" }
  // Add the assertion that matches your intended behavior.
});

Without this test, the regression introduced by truthy vs. null-check semantics is invisible in CI.

owner: "myorg",
repo: "myrepo",
issue_number: 42,
assignees: ["copilot-swe-agent[bot]"],
});
});

it("should include agent_assignment with all fields when all are provided", async () => {
const mockRequest = vi.fn().mockResolvedValue({ status: 201 });
const restClient = { request: mockRequest };

await assignAgentToIssue("id", "copilot-swe-agent[bot]", [], "copilot", null, "claude-opus-4.6", "my-agent", "Follow the coding guidelines.", "feature-branch", restClient, taskContext, "otherorg/otherrepo");

expect(mockRequest).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", {
owner: "myorg",
repo: "myrepo",
issue_number: 42,
assignees: ["copilot-swe-agent[bot]"],
agent_assignment: {
target_repo: "otherorg/otherrepo",
base_branch: "feature-branch",
custom_instructions: "Follow the coding guidelines.",
custom_agent: "my-agent",
model: "claude-opus-4.6",
},
});
});

it("should include agent_assignment with only the provided fields", async () => {
const mockRequest = vi.fn().mockResolvedValue({ status: 201 });
const restClient = { request: mockRequest };

await assignAgentToIssue("id", "copilot-swe-agent[bot]", [], "copilot", null, null, null, "Always write tests.", null, restClient, taskContext);

expect(mockRequest).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", {
owner: "myorg",
repo: "myrepo",
issue_number: 42,
assignees: ["copilot-swe-agent[bot]"],
agent_assignment: {
custom_instructions: "Always write tests.",
},
});
});

it("should include agent_assignment with target_repo when pullRequestRepoSlug is provided", async () => {
const mockRequest = vi.fn().mockResolvedValue({ status: 201 });
const restClient = { request: mockRequest };

await assignAgentToIssue("id", "copilot-swe-agent[bot]", [], "copilot", null, null, null, null, null, restClient, taskContext, "otherorg/otherrepo");

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.

Inconsistent assertion style — this test uses expect.objectContaining, while the other three use exact payload matching; a regression that sneaks extra keys into agent_assignment would be invisible here.

💡 Suggested fix

Use exact matching for consistency:

expect(mockRequest).toHaveBeenCalledWith(
  "POST /repos/{owner}/{repo}/issues/{issue_number}/assignees",
  {
    owner: "myorg",
    repo: "myrepo",
    issue_number: 42,
    assignees: ["copilot-swe-agent[bot]"],
    agent_assignment: { target_repo: "otherorg/otherrepo" },
  }
);


expect(mockRequest).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", {
owner: "myorg",
repo: "myrepo",
issue_number: 42,
assignees: ["copilot-swe-agent[bot]"],
agent_assignment: {
target_repo: "otherorg/otherrepo",
},
});
});

it("should include empty-string agent_assignment fields when explicitly provided", async () => {
const mockRequest = vi.fn().mockResolvedValue({ status: 201 });
const restClient = { request: mockRequest };

await assignAgentToIssue("id", "copilot-swe-agent[bot]", [], "copilot", null, "", "", "", "", restClient, taskContext, "");

expect(mockRequest).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", {
owner: "myorg",
repo: "myrepo",
issue_number: 42,
assignees: ["copilot-swe-agent[bot]"],
agent_assignment: {
target_repo: "",
base_branch: "",
custom_instructions: "",
custom_agent: "",
model: "",
},
});
});
});

describe("generatePermissionErrorSummary", () => {
Expand Down
8 changes: 5 additions & 3 deletions actions/setup/js/assign_to_agent.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ async function main(config = {}) {

// Handle per-item pull_request_repo override
let effectivePullRequestRepoSlug = basePullRequestRepoSlug;
let effectivePullRequestBaseBranch = effectiveBaseBranch;
let hasValidatedPerItemPullRequestRepoOverride = false;
const hasPullRequestRepoOverrideField = message.pull_request_repo != null;
const trimmedPullRequestRepoOverride = typeof message.pull_request_repo === "string" ? message.pull_request_repo.trim() : "";
Expand All @@ -260,8 +261,9 @@ async function main(config = {}) {
return { success: false, error };
}
try {
await resolvePullRequestRepo(githubClient, pullRequestRepoParts[0], pullRequestRepoParts[1], configuredBaseBranch);
const resolvedPullRequestRepo = await resolvePullRequestRepo(githubClient, pullRequestRepoParts[0], pullRequestRepoParts[1], configuredBaseBranch);
effectivePullRequestRepoSlug = itemPullRequestRepo;
effectivePullRequestBaseBranch = resolvedPullRequestRepo.effectiveBaseBranch;
hasValidatedPerItemPullRequestRepoOverride = true;
core.info(`Using per-item pull request repository: ${itemPullRequestRepo}`);
} catch (error) {
Expand Down Expand Up @@ -389,7 +391,7 @@ async function main(config = {}) {
if (model) core.info(`Using model: ${model}`);
if (customAgent) core.info(`Using custom agent: ${customAgent}`);
if (customInstructions) core.info(`Using custom instructions: ${customInstructions.substring(0, 100)}${customInstructions.length > 100 ? "..." : ""}`);
if (effectiveBaseBranch) core.info(`Using base branch: ${effectiveBaseBranch}`);
if (effectivePullRequestBaseBranch) core.info(`Using base branch: ${effectivePullRequestBaseBranch}`);

const success = await assignAgentToIssue(
assignableId,
Expand All @@ -400,7 +402,7 @@ async function main(config = {}) {
model,
customAgent,
customInstructions,
effectiveBaseBranch,
effectivePullRequestBaseBranch,
githubClient,
taskContext,
effectivePullRequestRepoSlug,
Expand Down
13 changes: 11 additions & 2 deletions actions/setup/js/assign_to_agent.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1496,7 +1496,7 @@ describe("assign_to_agent", () => {
// Get global PR repository ID and default branch (for default-pr-repo)
mockGithub.rest.repos.get.mockResolvedValueOnce({ data: { node_id: "default-pr-repo-id", default_branch: "main" } });
// Get item PR repository
mockGithub.rest.repos.get.mockResolvedValueOnce({ data: { node_id: "item-pull-request-repo-id", default_branch: "main" } });
mockGithub.rest.repos.get.mockResolvedValueOnce({ data: { node_id: "item-pull-request-repo-id", default_branch: "develop" } });
// Find agent
mockGithub.rest.issues.checkUserCanBeAssigned.mockResolvedValueOnce({});
mockGithub.rest.users.getByUsername.mockResolvedValueOnce({ data: { id: 99999 } });
Expand All @@ -1511,7 +1511,16 @@ describe("assign_to_agent", () => {

expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Using per-item pull request repository: test-owner/item-pull-request-repo"));

expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", expect.objectContaining({ owner: "test-owner", repo: "test-repo", issue_number: 42 }));
expect(mockGithub.request).toHaveBeenLastCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", {
owner: "test-owner",
repo: "test-repo",
issue_number: 42,
assignees: ["copilot-swe-agent[bot]"],
agent_assignment: {
target_repo: "test-owner/item-pull-request-repo",
base_branch: "develop",
},
});
});

it("should reject per-item pull_request_repo not in allowed list", async () => {
Expand Down
Loading