fix: nest issue-intent metadata correctly in close_issue and assign_to_agent payloads#46850
Conversation
…o_agent payloads
- close_issue.cjs: send `state: { value: "closed", ...intentMetadata }` instead of
spreading intentMetadata as top-level siblings of a scalar state
- assign_agent_helpers.cjs: send `assignees: [{ login, ...intentMetadata }]` instead of
spreading intentMetadata as top-level params with string assignees
- Update tests to assert the corrected nested payload shapes
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes issue-intent metadata persistence by using the API’s required nested payload structures.
Changes:
- Nests close metadata inside the
stateobject. - Nests assignment metadata inside each assignee object.
- Updates tests to verify both payload shapes.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/close_issue.cjs |
Corrects close-issue payload nesting. |
actions/setup/js/close_issue.test.cjs |
Verifies nested state metadata. |
actions/setup/js/assign_agent_helpers.cjs |
Corrects per-assignee metadata nesting. |
actions/setup/js/assign_agent_helpers.test.cjs |
Verifies nested assignee metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-structured. Both intent-path changes follow the same pattern of nesting metadata where the API actually reads it, and the fallback error paths remain intact. Tests confirm the old top-level keys are absent and the new nested shapes are asserted. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.2 AIC · ⌖ 4.31 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 34.9 AIC · ⌖ 4.59 AIC · ⊞ 5.6K
Comment /review to run again
| owner, | ||
| repo, | ||
| issue_number: issueNumber, | ||
| state: { value: "closed", ...intentMetadata }, |
There was a problem hiding this comment.
Unverified API contract: state: { value: "closed", ...intentMetadata } deviates from the GitHub REST PATCH-issues spec, which requires state to be the string "closed" — passing an object will produce a 422 and always fall back to the legacy path, so intent metadata is never persisted.
💡 Details
The standard REST API endpoint PATCH /repos/{owner}/{repo}/issues/{issue_number} documents state as a string enum ("open" | "closed"). Sending an object here will cause the server to reject the request, the catch block will swallow the error, and the legacy fallback (github.rest.issues.update) runs with no metadata — identical to the pre-fix behaviour.
If the issue-intent API uses a different shape (e.g. a GraphQL mutation or a preview endpoint), the route string and/or the field name must change accordingly. If this shape has been verified against the target API, add a comment citing the spec so reviewers can confirm it.
Expected REST shape if metadata stays at the top level:
{
owner, repo, issue_number: issueNumber,
state: "closed",
state_reason: baseParams.state_reason,
...intentMetadata, // rationale, confidence, etc. alongside state, not inside it
}|
|
||
| try { | ||
| core.info(`Assigning via issues assignees REST API with login: ${agentLogin}`); | ||
| const assignee = useIssueIntent && intentMetadata && Object.keys(intentMetadata).length > 0 ? { login: agentLogin, ...intentMetadata } : agentLogin; |
There was a problem hiding this comment.
Broken API contract: the GitHub REST POST .../assignees endpoint expects assignees to be an array of login strings; sending an object { login, ...intentMetadata } will cause a 422 or silent failure, leaving the issue unassigned.
💡 Details
The diff parallels the description with add_labels per-item objects, but the assignees REST endpoint has no such per-element object shape in its spec. Sending [{ login: '...', rationale: '...' }] instead of `['...']' will fail.
If the issue-intent API supports a non-standard per-assignee metadata shape, that must be documented with a spec reference. Otherwise the metadata needs to be carried at a different level in the request, not inside the assignee element.
Minimal safe fix (keeps previous logic, avoids regression):
const assignParams = {
owner: targetOwner,
repo: targetRepo,
issue_number: issueNumber,
assignees: [agentLogin],
};
if (useIssueIntent && intentMetadata && Object.keys(intentMetadata).length > 0) {
// intentMetadata goes wherever the target API actually reads it
Object.assign(assignParams, intentMetadata);
}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the root-cause fix is correct, minor test coverage gaps noted.
📋 Key Themes & Highlights
Key Themes
- Correct fix: metadata was silently dropped at top-level by the GitHub API; nesting it inside the
stateobject (close path) and inside the per-assignee object (assign path) is the right structural change. - Test coverage gaps: the intent path is tested, but
state_reasonforwarding and the empty-metadata fallback path lack explicit assertions (see inline comments).
Positive Highlights
- ✅ PR description is clear and includes before/after code snippets — easy to verify intent.
- ✅ Both call sites (close and assign) are updated consistently.
- ✅ Legacy fallback path (
github.rest.issues.updatewith scalar state) is correctly left unchanged. - ✅ Tests are updated in the same commit as the implementation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 33 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
| expect(requestCalls[0].params.state).toEqual({ value: "closed", rationale: "Duplicate confirmed", confidence: "MEDIUM" }); | ||
| expect(requestCalls[0].params.rationale).toBeUndefined(); | ||
| expect(requestCalls[0].params.confidence).toBeUndefined(); | ||
| expect(requestCalls[0].params.headers).toBeUndefined(); |
There was a problem hiding this comment.
[/tdd] The intent-path test doesn't assert that state_reason is still forwarded — a future refactor could silently drop it.
💡 Add the missing assertion
expect(requestCalls[0].params.state_reason).toBe("completed");Without this, the test wouldn't catch a regression where state_reason is accidentally omitted from the intent request body.
@copilot please address this.
| assignees: [{ login: "copilot-swe-agent[bot]", rationale: "Agent owns the code path", confidence: "HIGH" }], | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] No test covers the fallback where useIssueIntent is false or intentMetadata is empty — the assignee should remain a plain string in those cases.
💡 Suggested test
it("uses plain string assignee when intent metadata is absent", async () => {
// setup: useIssueIntent=true but intentMetadata={}
// expect: assignees: ["copilot-swe-agent[bot]"] (not an object)
});Without this, a regression that always emits the object form would go undetected.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
close_issueandassign_to_agentwere spreading intent metadata (rationale,confidence,suggest) as top-level request params, where the GitHub API silently ignores them. The mutation returns HTTP 200 with no error, so the error fallback never fires and no intent is persisted.Changes
close_issue.cjs— wrapstateas an object instead of a scalar string, nesting metadata inside it:The legacy fallback path (
github.rest.issues.updatewith scalarstate) is unchanged.assign_agent_helpers.cjs— use per-assignee object form (mirroringadd_labels), keyed bylogin:Tests updated to assert the corrected nested payload shapes.