fix: add_comment silently skips when integration token can't write to discussions#39926
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…skipped in add_comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…mment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the safeoutputs MCP handler should refuse add comment to discussions when discussions are not enabled. |
There was a problem hiding this comment.
⚠️ Not ready to approve
It introduces a misleading sync/source-of-truth comment in actions/setup-cli/install.sh that contradicts the documented generation flow and should be corrected before merge.
Pull request overview
This PR adjusts the add_comment safe-output handler to treat a missing discussions:write permission (manifesting as “Resource not accessible by integration” from the Discussion GraphQL mutation) as a skippable configuration mismatch rather than a hard failure, aligning behavior with other handlers that skip on integration permission mismatches.
Changes:
- Add
isDiscussionIntegrationAccessError()to detect the integration access error via either the top-level error message orerrors[]entries. - In the discussion fallback/retry path, emit an actionable
core.warningand return{ success: false, skipped: true }instead of failing the safe-outputs job. - Add tests covering both error-string and GraphQL
errors[]variants of the integration access error.
File summaries
| File | Description |
|---|---|
| actions/setup/js/add_comment.cjs | Detects “Resource not accessible by integration” for discussion comment attempts and returns a skipped result with a configuration warning. |
| actions/setup/js/add_comment.test.cjs | Adds regression tests ensuring the discussion integration access error is treated as skipped (not fatal). |
| actions/setup-cli/install.sh | Updates the header sync comment (currently appears to be self-referential/misleading). |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set +o histexpand | ||
|
|
||
| # Kept in sync with install-gh-aw.sh — edit that file, then copy to this path. | ||
| # Kept in sync with actions/setup-cli/install.sh — edit this file, then copy to that path. |
… in MCP handler Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. The
This prevents the entry from being written to NDJSON at all, so no silent skip or late execution failure occurs. Two new tests cover the refusal and the allowed path (commit |
|
@copilot run pr-finisher skill |
…up-cli/install.sh Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 74c7169. Fixed the self-referential sync comment by updating |
When
add_commenttargets a GitHub Discussion and the runner'sGITHUB_TOKENlacksdiscussions:write, the GraphQL mutation returns "Resource not accessible by integration". Previously this returned{success: false}— a hard failure that failed the entire safe-outputs job. It should be a skippable configuration mismatch, the same wayresolve_pr_review_threadhandles the identical error.Changes
add_comment.cjs: AddisDiscussionIntegrationAccessError()that checks both the top-level error message and GraphQLerrors[]array. In the discussion retry catch block, return{success: false, skipped: true}with acore.warningpointing tosafe-outputs.add-comment.github-tokenrather than propagating a fatal failure.add_comment.test.cjs: Two new tests — one for mutation error string, one for the GraphQLerrors[]array variant.safe_outputs_handlers.cjs:addCommentHandlernow refuses calls that includereply_to_id(a discussion-only threading field) whendiscussions: trueis not set in thesafe-outputs.add-commentconfiguration. This gives the agent immediate, actionable feedback at MCP time before anything is written to NDJSON, rather than a late execution-phase failure.safe_outputs_handlers.test.cjs: Two new tests covering the MCP-phase refusal (nodiscussions: true+reply_to_id→ error with guidance) and the allowed path (discussions: true+reply_to_id→ success).