Fix update-discussion API error logging for explicit discussion_number#23340
Fix update-discussion API error logging for explicit discussion_number#23340
Conversation
Add logGraphQLError helper to update_discussion.cjs that surfaces the full GraphQL error details (errors array with type/path/locations, HTTP status, and actionable permission hints for INSUFFICIENT_SCOPES and NOT_FOUND) when a discussion fetch or mutation fails. Previously, failures only reported ERR_API: update discussion #N failed (attempt 1) with the root cause buried in a multi-line enhanced error. Now every GraphQL failure also emits structured diagnostic output that enables fast root-cause analysis without a debug re-run. Wrap both the fetch-discussion query and the updateDiscussion mutation in try/catch blocks that call logGraphQLError before re-throwing. Add debug: vi.fn() to the test mock core and 3 new tests covering INSUFFICIENT_SCOPES, FORBIDDEN, and NOT_FOUND error scenarios. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b7ebe3f3-899f-4f11-9dff-5f93dce4189e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| * @param {Error & { errors?: Array<{ type?: string, message: string, path?: unknown, locations?: unknown }>, request?: unknown, data?: unknown, status?: number }} error - GraphQL error | ||
| * @param {string} operation - Human-readable description of the failing operation | ||
| */ | ||
| function logGraphQLError(error, operation) { |
There was a problem hiding this comment.
- refactor into graphql helpers file / function with generalized logging
There was a problem hiding this comment.
Done in af8d72f. Moved logGraphQLError into github_api_helpers.cjs as a shared, generalized function. It accepts an optional GraphQLErrorHints parameter (insufficientScopesHint, notFoundHint, notFoundPredicate) for domain-specific messages. All four files that had local copies (update_discussion.cjs, update_project.cjs, create_project.cjs, create_project_status_update.cjs) now import and call the shared version with a module-level hints constant. Nine new tests added to github_api_helpers.test.cjs.
There was a problem hiding this comment.
Pull request overview
Improves diagnostics for update-discussion GraphQL failures by surfacing actionable error details directly in logs (especially for explicit discussion_number updates).
Changes:
- Added
logGraphQLErrorhelper to emit GraphQL error details (errors array metadata, HTTP status, permission hints, and request/response when present). - Wrapped the fetch-discussion query and
updateDiscussionmutation with try/catch to log diagnostics at the failure point before rethrowing. - Extended unit tests to cover
INSUFFICIENT_SCOPES,FORBIDDEN, andNOT_FOUNDerror logging paths (and added acore.debugmock required by shared error recovery).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| actions/setup/js/update_discussion.cjs | Adds detailed GraphQL error logging and applies it to discussion fetch + mutation paths. |
| actions/setup/js/update_discussion.test.cjs | Adds mocks and tests validating error logging behavior for common GraphQL failure types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param {string} operation - Human-readable description of the failing operation | ||
| */ | ||
| function logGraphQLError(error, operation) { | ||
| core.info(`GraphQL error during: ${operation}`); |
There was a problem hiding this comment.
The log prefix here (GraphQL error during:) is inconsistent with the existing pattern used elsewhere in this repo (e.g., actions/setup/js/update_project.cjs:46, create_project.cjs:17) which logs GraphQL Error during:. For consistent grepping/parsing across actions, consider aligning this string (including capitalization) with the established prefix.
| core.info(`GraphQL error during: ${operation}`); | |
| core.info(`GraphQL Error during: ${operation}`); |
| // Verify logGraphQLError was called: it emits the operation name, message, and permission hint | ||
| expect(mockCore.infos.some(msg => msg.includes("GraphQL error during:"))).toBe(true); | ||
| expect(mockCore.infos.some(msg => msg.includes("fetch discussion #45"))).toBe(true); | ||
| expect(mockCore.infos.some(msg => msg.includes("Request failed"))).toBe(true); | ||
| expect(mockCore.infos.some(msg => msg.includes("discussions: write"))).toBe(true); | ||
| expect(mockCore.infos.some(msg => msg.includes("INSUFFICIENT_SCOPES"))).toBe(true); |
There was a problem hiding this comment.
This test asserts the new log prefix GraphQL error during:. If you align the runtime log message with the repo’s existing convention (GraphQL Error during: as in update_project.cjs / create_project.cjs), this assertion should be updated accordingly to avoid brittle divergence between handlers.
Move the duplicated logGraphQLError helper from update_discussion.cjs, update_project.cjs, create_project.cjs, and create_project_status_update.cjs into a single generalized implementation in github_api_helpers.cjs. The shared function accepts an optional `hints` (GraphQLErrorHints) parameter for domain-specific INSUFFICIENT_SCOPES / NOT_FOUND messages and an optional notFoundPredicate to gate the NOT_FOUND hint on the error message content (used by project callers to match /projectV2\b/). Each caller defines a module-level *_GRAPHQL_HINTS constant and passes it to logGraphQLError. The shared function also surfaces error.status (HTTP status code), which was previously only logged by update_discussion.cjs. Tests added to github_api_helpers.test.cjs covering: operation/message logging, errors array details, HTTP status, request/response data, insufficientScopesHint, notFoundHint with and without notFoundPredicate, and no-hints invocation. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/fd81583a-3cf5-4b0c-89d0-94d252259614 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| } | ||
|
|
||
| if (error.status) core.info(`HTTP status: ${error.status}`); | ||
| if (error.request) core.info(`Request: ${JSON.stringify(error.request, null, 2)}`); |
There was a problem hiding this comment.
The GraphQLErrorHints typedef is well-structured and enables clear domain-specific error guidance. Consider adding a JSDoc @example block showing typical usage to help future maintainers.
| repo: context.repo.repo, | ||
| number: discussionNumber, | ||
| }); | ||
| } catch (fetchError) { |
There was a problem hiding this comment.
Good use of the shared logGraphQLError helper here. The DISCUSSION_GRAPHQL_HINTS constant at module level is clean. The notFoundHint message is clear and actionable for debugging.
…scussion-api-error # Conflicts: # actions/setup/js/update_discussion.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ILES Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5efa4be0-4e09-4f0a-9b32-2136eeb931e9 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…t errors Agent-Logs-Url: https://github.com/github/gh-aw/sessions/673c29ea-7718-47bd-b05c-c07b8c0c2aef Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 62dd016. The TypeScript errors were caused by Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
When
update-discussionfailed (e.g. due to missingdiscussions: writepermission), the only visible error was the opaqueERR_API: update discussion #N failed (attempt 1)— the actual GraphQL error was buried in the enhanced error chain and invisible in most log views.Changes
github_api_helpers.cjs(new shared helper)logGraphQLError(error, operation, hints)function with aGraphQLErrorHintstypedef, replacing the duplicated copies that existed in each handlerhintsparameter supports domain-specific messages:insufficientScopesHint— shown whenINSUFFICIENT_SCOPESerror type is presentnotFoundHint— shown whenNOT_FOUNDerror type is presentnotFoundPredicate— optional function to further gate the NOT_FOUND hint (e.g. project handlers use/projectV2\b/.test(msg))error.status(HTTP status code) which was missing from the project handler copiesupdate_discussion.cjslogGraphQLErrorcopy; imports the shared version fromgithub_api_helpers.cjsDISCUSSION_GRAPHQL_HINTSwith discussion-specific permission hints forINSUFFICIENT_SCOPES(→ addpermissions: discussions: write) andNOT_FOUND(→ check discussion number / token read access)updateDiscussionmutation in try/catch that callslogGraphQLErrorbefore re-throwing, so diagnostics fire at the point of failureupdate_project.cjs,create_project.cjs,create_project_status_update.cjslogGraphQLErrorcopies; each now imports fromgithub_api_helpers.cjsand passes a module-levelPROJECT_GRAPHQL_HINTSconstant with project-specific messagesactions/setup/setup.shgithub_api_helpers.cjstoSAFE_OUTPUTS_FILESso the new shared helper is deployed to the safe-outputs runtime directoryupdate_discussion.test.cjsdebug: vi.fn()to mockcore(required byerror_recovery.cjs)INSUFFICIENT_SCOPES,FORBIDDEN, andNOT_FOUNDerror pathsgithub_api_helpers.test.cjslogGraphQLErrorcovering: operation/message logging, errors array details, HTTP status, request/response data, hint display conditions,notFoundPredicatefiltering, and no-hints invocationExample of what is now emitted on an
INSUFFICIENT_SCOPESfailure:✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
✨ PR Review Safe Output Test - Run 23685128220