fix: send agent_assignment in assignAgentToIssue REST payload#47047
Conversation
Build and include agent_assignment in the POST assignees payload when model, customAgent, customInstructions, baseBranch, or pullRequestRepoSlug are provided, so the Copilot coding agent receives custom instructions and agent configuration. Closes #46799" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. 🤖 Great work! This PR looks ready for review. The fix directly addresses issue #46799 by ensuring What's working well:
The PR is in draft status. Once you mark it ready for review, it should be good to merge pending any review feedback from maintainers.
|
There was a problem hiding this comment.
Pull request overview
Adds Copilot assignment options to the REST request, addressing #46799.
Changes:
- Builds
agent_assignmentfrom configured agent options. - Adds unit coverage for full, partial, and absent options.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/assign_agent_helpers.cjs |
Adds the REST payload fields. |
actions/setup/js/assign_agent_helpers.test.cjs |
Tests payload construction. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| }; | ||
| const agentAssignment = {}; | ||
| if (pullRequestRepoSlug) agentAssignment.target_repo = pullRequestRepoSlug; | ||
| if (baseBranch) agentAssignment.base_branch = baseBranch; |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47047 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — changes look good.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause properly fixed:
agent_assignmentis now built and attached to the REST payload - ✅ Four tests cover the null, full, partial, and
target_repo-only cases — solid regression protection - ✅ Guard prevents an empty object from being sent when no optional fields are provided
- ✅ Consistent truthy checks mirror the existing style in the file
Minor Note
The existing review comment (line 408) raises a valid point about baseBranch not always being resolved when target_repo is an override — worth tracking as a follow-up if cross-repo assignments are a real use case.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 13.9 AIC · ⌖ 4.4 AIC · ⊞ 6.6K
Comment /matt to run again
There was a problem hiding this comment.
The change looks correct — agent_assignment is populated conditionally and only included when at least one field is set, avoiding sending an empty object. Tests cover key scenarios (all fields, partial fields, no fields). The existing inline comment about baseBranch resolution for per-item target_repo overrides is the only outstanding concern and is already captured.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.6 AIC · ⌖ 4.34 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report
📊 Metrics (4 tests)
|
There was a problem hiding this comment.
REQUEST_CHANGES — one correctness bug must be fixed before merging.
### Findings
High — truthy checks silently drop empty-string values (assign_agent_helpers.cjs lines 407–411)
All five conditionals use bare truthiness (if (model), if (baseBranch), etc.). Any caller that passes an empty string "" for any of those fields will have it silently omitted from the agent_assignment payload. Since all parameters default to null and the intent is clearly "skip when not provided", replace with != null guards throughout.
Medium — no empty-string test coverage (assign_agent_helpers.test.cjs)
The four new tests only cover null and non-empty strings. There is no test that pins the behavior for "", meaning the bug above is invisible in CI.
Low — inconsistent assertion style (assign_agent_helpers.test.cjs line 541)
One test uses objectContaining while the others use exact matching, weakening regression protection for that case.
🔎 Code quality review by PR Code Quality Reviewer · 39.4 AIC · ⌖ 4.78 AIC · ⊞ 5.6K
Comment /review to run again
| if (baseBranch) agentAssignment.base_branch = baseBranch; | ||
| if (customInstructions) agentAssignment.custom_instructions = customInstructions; | ||
| if (customAgent) agentAssignment.custom_agent = customAgent; | ||
| if (model) agentAssignment.model = model; |
There was a problem hiding this comment.
Truthy checks silently drop empty-string values — any caller passing "" for model, customAgent, customInstructions, baseBranch, or pullRequestRepoSlug will have that field quietly omitted from agent_assignment, with no error.
💡 Suggested fix
Replace bare truthiness with explicit null checks:
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;This preserves the skip-when-not-provided intent for null (the declared default) while forwarding empty strings a caller explicitly passes. Since all parameters default to null, != null is the correct narrowing predicate.
|
|
||
| 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", { |
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
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" },
}
);
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/29826418446 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in b20e323. Switched the |
|
🎉 This pull request is included in a new release. Release: |
assignAgentToIssueacceptedmodel,customAgent,customInstructions,baseBranch, andpullRequestRepoSlugparameters but never included them in the API request — the Copilot coding agent was always assigned with a bare{ assignees: [...] }payload, silently ignoring all custom configuration.Changes
assign_agent_helpers.cjs: Build and attachagent_assignmentto the POST assignees payload when any of the relevant fields are non-null:assign_agent_helpers.test.cjs: Four new tests covering: noagent_assignmentwhen all optional fields are null, all fields populated, partial fields, andpullRequestRepoSlug→target_repomapping.