Migrate set_issue_type safe output from GraphQL to single REST issues.update call#41241
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
set_issue_type safe output from GraphQL to single REST issues.update call
|
✅ 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. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR migrates the set_issue_type safe-output handler from a multi-call GraphQL flow to a single REST issues.update call, while adding REST-specific handling for issue-intent metadata and 422 validation errors.
Changes:
- Replaced the GraphQL lookup/mutation path with
githubClient.rest.issues.update({ ..., type }). - Added REST intent-metadata formatting (including confidence normalization to
low|medium|high). - Updated tests to assert REST payloads and added coverage for 422 invalid-type mapping.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/set_issue_type.cjs | Switches handler implementation to REST issues.update, adds intent metadata formatting, and introduces 422 parsing/mapping. |
| actions/setup/js/set_issue_type.test.cjs | Reworks mocks/assertions for REST, adds a 422 mapping test, and removes GraphQL expectations. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 4
| // REST validation errors vary across endpoints and deployments; extract the list from | ||
| // either "... one of: A, B" or "... available types: A, B" when present. | ||
| const matchedPattern = AVAILABLE_TYPES_PATTERNS.find(pattern => pattern.test(errorDetails)); | ||
| const availableTypes = (matchedPattern ? matchedPattern.exec(errorDetails)?.[1] : undefined)?.trim() || errorDetails; | ||
| return `${baseMessage} Available types: ${availableTypes}`; |
There was a problem hiding this comment.
Fixed in e90bb42. mapInvalidIssueTypeError now appends Available types: only when a list is extracted from a recognized pattern; otherwise it returns the base not-found message (or the preserved no-issue-types-available mapping).
| try { | ||
| const { owner, repo } = repoParts; | ||
| const intentMetadata = normalizeIssueIntentMetadata(item); | ||
|
|
||
| // Get the issue's node ID for GraphQL | ||
| const issueNodeId = await getIssueNodeId(githubClient, owner, repo, issueNumber); | ||
|
|
||
| let typeId = null; | ||
| if (!isClear) { | ||
| // Fetch available issue types and find the matching one | ||
| const issueTypes = await fetchIssueTypes(githubClient, owner, repo); | ||
|
|
||
| if (issueTypes.length === 0) { | ||
| const error = "No issue types are available for this repository. Issue types must be configured in the repository or organization settings."; | ||
| core.error(error); | ||
| return { success: false, error }; | ||
| } | ||
|
|
||
| const matchedType = issueTypes.find(t => t.name.toLowerCase() === issueTypeName.toLowerCase()); | ||
| if (!matchedType) { | ||
| const availableNames = issueTypes.map(t => t.name).join(", "); | ||
| const error = `Issue type ${JSON.stringify(issueTypeName)} not found. Available types: ${availableNames}`; | ||
| core.error(error); | ||
| return { success: false, error }; | ||
| } | ||
|
|
||
| typeId = matchedType.id; | ||
| core.info(`Resolved issue type ${JSON.stringify(issueTypeName)} to node ID: ${typeId}`); | ||
| } | ||
|
|
||
| await setIssueTypeById(githubClient, issueNodeId, typeId, intentMetadata); | ||
| const typeValue = buildIssueTypeValue(isClear, issueTypeName, intentMetadata); | ||
| await githubClient.rest.issues.update({ |
There was a problem hiding this comment.
Fixed in e90bb42. The handler now canonicalizes case-insensitive matches against allowed and sends the canonical configured type value in the REST issues.update payload; tests assert this behavior.
| it("should handle case-insensitive type matching", async () => { | ||
| const message = { | ||
| type: "set_issue_type", | ||
| issue_number: 42, | ||
| issue_type: "bug", // lowercase | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
| expect(result.success).toBe(true); | ||
| // Should still resolve to the Bug type | ||
| expect(mockGraphql).toHaveBeenCalledWith(expect.stringContaining("updateIssue"), expect.objectContaining({ typeId: bugTypeId })); | ||
| expect(mockGithub.rest.issues.update).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| issue_number: 42, | ||
| type: "bug", | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fixed in e90bb42. Updated the case-insensitive test to use an allowed list and assert canonical type "Bug" is sent to REST and returned in handler output.
| if (!isClear && isIssueTypeValidationError(error)) { | ||
| const mappedError = mapInvalidIssueTypeError(error, issueTypeName); | ||
| core.error(`Failed to set issue type on issue #${issueNumber}: ${mappedError}`); | ||
| return { success: false, error: mappedError }; | ||
| } |
There was a problem hiding this comment.
Fixed in e90bb42. Added REST 422 mapping for issue-types-unavailable cases to preserve the prior explicit error message, plus test coverage for that mapping.
![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 /zoom-out and /improve-codebase-architecture — requesting changes on two correctness concerns in the new error-handling path, plus a few test coverage gaps.
📋 Key Themes & Highlights
Issues to address
- Overly broad 422 guard — any 422 from
issues.updateis remapped to"Issue type X not found", even if the error is unrelated to types. Narrow with a message-content check. - Fallthrough message — when no regex pattern matches the error details,
"Available types: [full error prose]"is emitted, which is confusing. ReturnbaseMessagealone in the no-match case. suggest: falsesilently dropped — the JSDoc allowsboolean; onlytrueis forwarded. Clarify intent or forward the value as-is.- Test coverage gap —
response.data.messagepath inmapInvalidIssueTypeErroris untested; theerrors[0].messagepath is the only exercised branch. - Misleading test name —
'should handle case-insensitive type matching'no longer describes actual behaviour (client-side normalisation was removed).
Positive Highlights
- ✅ Excellent complexity reduction: 3-call GraphQL flow → 1 REST call, net −44 lines
- ✅
toRestIssueIntentMetadatais clean and well-typed; confidence normalisation is correct - ✅ Good defensive pattern in
getErrorStatus: checks bothstatusandresponse.status - ✅ Tests were substantially rewritten rather than just patched, and the 422 mapping test is a good addition
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 72.8 AIC · ⌖ 8.2 AIC · ⊞ 6.5K
Comments that could not be inline-anchored
actions/setup/js/set_issue_type.cjs:69
[/zoom-out] isIssueTypeValidationError treats any 422 from issues.update as a type-not-found error. The REST endpoint can return 422 for reasons unrelated to issue types (field constraints, other validation), which would surface as a misleading "Issue type X not found" message.
<details>
<summary>💡 Suggested fix</summary>
Narrow the match by also checking the error message contains type-related content:
function isIssueTypeValidationError(error) {
if (getErrorStatus(erro…
</details>
<details><summary>actions/setup/js/set_issue_type.cjs:95</summary>
**[/improve-codebase-architecture]** When none of the `AVAILABLE_TYPES_PATTERNS` match, `availableTypes` falls back to the full raw `errorDetails` string. This produces messages like `"Issue type X not found. Available types: Type must be one of the configured types for this repository."` — the label `Available types:` is misleading when the value is a prose sentence rather than a list.
<details>
<summary>💡 Suggested fix</summary>
Return `baseMessage` (without the Available types clause) whe…
</details>
<details><summary>actions/setup/js/set_issue_type.cjs:32</summary>
**[/zoom-out]** Only `suggest: true` is forwarded; `suggest: false` is silently dropped. The JSDoc type allows `boolean`, not just `true`. If the REST API treats an absent `suggest` field differently from `suggest: false`, this is a silent behavioral divergence from the callers' intent.
<details>
<summary>💡 Suggested change</summary>
```js
// Before
if (intentMetadata.suggest) {
restMetadata.suggest = true;
}
// After — preserve explicit false
if (intentMetadata.suggest !== undefined) {
…
</details>
<details><summary>actions/setup/js/set_issue_type.test.cjs:208</summary>
**[/tdd]** The 422 mapping test only exercises the `errors[0].message` code path. The `response.data.message` fallback in `mapInvalidIssueTypeError` (line 84 of the impl) is never covered — if the server returns a bare `message` without an `errors` array, the parsing is completely untested.
<details>
<summary>💡 Add a second test case</summary>
```js
it('should map 422 with bare data.message to not-found shape', async () => {
const error = Object.assign(new Error('Validation failed'), {
…
</details>
<details><summary>actions/setup/js/set_issue_type.test.cjs:263</summary>
**[/zoom-out]** The test name `'should handle case-insensitive type matching'` implies client-side normalisation, but that behaviour was removed in this PR — the raw input is now forwarded directly to the REST API. The test just confirms pass-through; server-side case handling is assumed.
Rename the test to something like `'should pass issue_type string as-is to REST API'` to accurately document the new behaviour and prevent future readers from believing case-folding still happens client-side.
</details>
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (6 tests analyzed)
JavaScript: 6 ( Notes on removed tests: Two GraphQL-specific tests were deleted — All mocks ( Verdict
|
There was a problem hiding this comment.
REQUEST_CHANGES — one high-severity behavioral regression and two medium-severity correctness issues must be addressed before merge.
### Blocking issues
1. [HIGH] Case-sensitivity regression (line 240)
The old GraphQL path did a case-insensitive lookup and sent the canonical type ID to the API. The new REST path sends the user's raw string unchanged. If the REST API is case-sensitive, callers using issue_type: "bug" when the server expects "Bug" will silently regress from success to 422. The client-side allowed check is also case-insensitive but still passes the original casing — this inconsistency will surface as intermittent failures.
2. [MEDIUM] Invalid JSDoc cast syntax (lines 60, 82)
Both /** @type {IssueTypeAPIError} */ error casts are missing the required parentheses around the casted expression. Under // @ts-check`` the variables remain typed as object, not `IssueTypeAPIError`, so property accesses are type-unsafe and IDEs won't offer correct completions.
### Non-blocking observations
- Double regex execution (line 94):
test()in.find()followed byexec()on the same pattern runs the regex twice. Safe now (nogflag) but fragile; a singleexec()loop is cleaner. - Misleading test name (test line 263):
"should handle case-insensitive type matching"now only asserts passthrough, not normalization — rename to reflect actual behaviour.
🔎 Code quality review by PR Code Quality Reviewer · 129 AIC · ⌖ 8.05 AIC · ⊞ 5.2K
Comments that could not be inline-anchored
actions/setup/js/set_issue_type.cjs:240
Case-sensitivity regression: user-supplied issueTypeName is passed directly to the REST API without normalization, silently breaking callers that relied on the previous case-insensitive resolution.
<details>
<summary>💡 Details and suggested fix</summary>
The old GraphQL flow did:
const matchedType = issueTypes.find(t => t.name.toLowerCase() === issueTypeName.toLowerCase());
typeId = matchedType.id; // canonical ID, not the raw user stringThe new flow passes raw user input:…
actions/setup/js/set_issue_type.cjs:60
Invalid JSDoc type cast syntax: parentheses are missing around error — this annotates the variable but does not actually cast the expression, defeating // @ts-check``.
<details>
<summary>💡 Suggested fix</summary>
JSDoc type casts require parentheses around the expression being cast:
// Wrong — declaration annotation only; errorWithStatus is still typed as `object`
const errorWithStatus = /** `@type` {IssueTypeAPIError} */ error;
// Correct — actual type assertion
const errorW…
</details>
<details><summary>actions/setup/js/set_issue_type.cjs:94</summary>
**Double regex execution**: `test()` is called in the `.find()` predicate, then `exec()` is called again on the winning pattern — running the regex twice on the same string.
<details>
<summary>💡 Suggested fix</summary>
Collapse into a single `exec()` pass:
```js
let match;
for (const pattern of AVAILABLE_TYPES_PATTERNS) {
match = pattern.exec(errorDetails);
if (match) break;
}
const availableTypes = match?.[1]?.trim() || errorDetails;The current patterns lack the g flag so `last…
actions/setup/js/set_issue_type.test.cjs:263
Misleading test name: this test no longer verifies case-insensitive resolution — it only confirms that lowercase input is passed through unchanged to the REST API.
<details>
<summary>💡 Details</summary>
The old test asserted that issue_type: "bug" was resolved to the canonical Bug type ID via a case-insensitive GraphQL lookup. The new test asserts that type: "bug" is sent as-is.
This is a behavioral change: the old code normalized casing on the client; the new code delegates casin…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check
Result: 12/12 tools available ✅
|
Smoke test
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
Smoke Test Results
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Smoke test results for #41245 fix: normalize report formatting for daily-rendering-scripts-verifier.md and #41240 Fix false negatives in docs npm update detection. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
🤖 Smoke Test: Claude — Run 28119469685Core #1-12: ✅ ✅ ✅ ✅ ❌(Tavily) ✅ ✅ ✅ ✅ ✅ ✅ ✅ PR Review #13-19: 13✅ 14✅ 15✅ 16✅ 17✅ 18✅ 19 Overall: FAIL — Tavily tool list unavailable in this environment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · 75.8 AIC · ⌖ 38.2 AIC · ⊞ 8.7K
|
|
||
| /** @type {string} Safe output type handled by this module */ | ||
| const HANDLER_TYPE = "set_issue_type"; | ||
| const AVAILABLE_TYPES_PATTERNS = [/one of:\s*(.+)$/i, /available(?: types?)?:\s*(.+)$/i]; |
There was a problem hiding this comment.
Nice — extracting the available-types regexes into a named constant improves readability.
| const HANDLER_TYPE = "set_issue_type"; | ||
| const AVAILABLE_TYPES_PATTERNS = [/one of:\s*(.+)$/i, /available(?: types?)?:\s*(.+)$/i]; | ||
| const NO_ISSUE_TYPES_PATTERNS = [/no issue types? (?:are )?available/i, /issue types? (?:is|are) not (?:enabled|configured)/i]; | ||
| const NO_ISSUE_TYPES_AVAILABLE_ERROR = "No issue types are available for this repository. Issue types must be configured in the repository or organization settings."; |
There was a problem hiding this comment.
Clear, user-facing error string for the no-issue-types case. 👍
|
Migrate
Warning Firewall blocked 5 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke review pass. Tools barked. Comments dropped.
Warning
Firewall blocked 5 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · 258.1 AIC · ⌖ 19.3 AIC · ⊞ 19.3K
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. Smoke Test summary:
|
|
Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Pull request created: #41284
|
set_issue_typepreviously required a 3-call GraphQL flow (issue node lookup, type discovery, mutation). This change moves the handler to RESTPATCH /repos/:owner/:repo/issues/:number, reducing call count to one while preserving existing handler behavior and error semantics.Handler API path migration
actions/setup/js/set_issue_type.cjs.githubClient.rest.issues.update({ owner, repo, issue_number, type })call.Behavior parity retained
Intent metadata passthrough (runtime feature)
issue_intentsis enabled, handler now sends REST hash form:type: { value, rationale, confidence, suggest }LOW|MEDIUM|HIGH→low|medium|high).422 invalid-type mapping
Issue type "<name>" not found. Available types: ...Tests updated for REST flow
actions/setup/js/set_issue_type.test.cjsnow asserts RESTissues.updatepayloads for set/clear/intents paths.✨ PR Review Safe Output Test - Run 28119469685
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.