-
Notifications
You must be signed in to change notification settings - Fork 46
Add fallback to create issue on discussion failure #13569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added FallbackToIssue field to CreateDiscussionsConfig struct (default: true) - Updated parseDiscussionsConfig to set default value - Added fallback_to_issue to handler config builder - Updated create_discussion.cjs to handle fallback logic when permissions errors occur - Reuses existing create_issue handler for fallback - Adds note to issue body when created as fallback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot keep working |
- Added TestParseDiscussionsConfigFallbackToIssue test - Recompiled all workflow lock files with updated configuration - All tests pass, linting passes - Ready for review Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the implementation! All tasks from the checklist are now done: ✅ Added Commit: c64c24d The feature is ready for review. When a discussion creation fails due to permissions issues, it will automatically fallback to creating an issue with a note explaining it was meant to be a discussion. Users can disable this with |
| const errorMessage = getErrorMessage(error); | ||
|
|
||
| // Check if this is a permissions error and fallback is enabled | ||
| if (fallbackToIssue && createIssueHandler && isPermissionsError(errorMessage)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot refactor into helper function to avoid duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored into handleFallbackToIssue helper function. Both error paths (fetching discussion info and creating discussion) now use the same helper, reducing code duplication by ~40 lines.
Commit: 2339840
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a fallback mechanism for the safe-outputs.create-discussion handler to automatically create issues when discussion creation fails due to permissions errors. The feature is enabled by default but can be disabled via configuration.
Changes:
- Added
FallbackToIssueconfiguration field (default: true) to enable/disable fallback behavior - Implemented permissions error detection and automatic fallback to
create_issuehandler in JavaScript - Added fallback note to issues created as discussion substitutes
- Updated all workflow lock files to include the new configuration parameter
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/create_discussion.go | Added FallbackToIssue field, default value handling, and configuration logging |
| pkg/workflow/create_discussion_validation_test.go | Added tests for fallback-to-issue configuration parsing and defaults |
| pkg/workflow/compiler_safe_outputs_config.go | Updated handler registry to include fallback_to_issue in config builder |
| actions/setup/js/create_discussion.cjs | Implemented isPermissionsError detection, create_issue handler initialization, and fallback logic with note appending |
| .github/workflows/*.lock.yml | Regenerated workflow files with fallback_to_issue:true in handler configs |
| .github/workflows/go.mod | Added empty require block (likely formatting) |
| .github/dependabot.yml | Added gomod ecosystem monitoring for .github/workflows directory |
Comments suppressed due to low confidence (2)
actions/setup/js/create_discussion.cjs:415
- The fallback note message is duplicated in two places (lines 250 and 415). Consider extracting this to a constant at the module level to ensure consistency and make future updates easier. For example:
const FALLBACK_NOTE = `\n\n---\n\n> **Note:** This was intended to be a discussion, but discussions could not be created due to permissions issues. This issue was created as a fallback.\n`;This would also make it easier to test the exact message format.
actions/setup/js/create_discussion.cjs:179
- There's a potential issue with the max count tracking when fallback occurs. The
processedCountis incremented at line 179 before the discussion creation is attempted. When a fallback to create_issue happens, the create_issue handler has its own internalprocessedCountthat also gets incremented.
This means:
- The create_discussion handler increments its count
- The create_issue handler (initialized with the same
maxvalue) increments its own count
If multiple fallbacks occur, the create_issue handler could reach its max limit independently, even though the create_discussion handler may still have quota remaining. This could lead to confusing behavior where some fallbacks succeed and others fail with "max count reached" errors.
Consider one of these approaches:
- Pass a higher or unlimited max count to the create_issue handler since it's only used for fallback
- Share the same counter between both handlers
- Document this behavior clearly if it's intentional
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require ( | ||
| ) |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty require() block appears to be unintentional formatting. The .github/workflows/go.mod file originally had no require block at all, and this change adds an empty one. Consider either:
- Removing the empty require block to keep the file minimal, or
- If there are dependencies to add, include them in this block
If this file is auto-generated or managed by a tool, please verify the tool is working correctly.
| /** | ||
| * Checks if an error is a permissions-related error | ||
| * @param {string} errorMessage - The error message to check | ||
| * @returns {boolean} True if the error is permissions-related | ||
| */ | ||
| function isPermissionsError(errorMessage) { | ||
| return ( | ||
| errorMessage.includes("Resource not accessible") || | ||
| errorMessage.includes("Insufficient permissions") || | ||
| errorMessage.includes("Bad credentials") || | ||
| errorMessage.includes("Not Authenticated") || | ||
| errorMessage.includes("requires authentication") || | ||
| errorMessage.includes("Discussions not enabled") || | ||
| errorMessage.includes("Failed to fetch repository information") | ||
| ); | ||
| } |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new isPermissionsError function and the fallback mechanism lack test coverage. While Go tests were added for configuration parsing, there are no JavaScript tests verifying:
- The
isPermissionsErrorfunction correctly identifies various permission error messages - The fallback to create_issue works correctly when permissions errors occur
- The fallback note is properly appended to the issue body
- The fallback mechanism correctly handles cases when create_issue also fails
Consider adding unit tests for this functionality in a new test file such as create_discussion.test.cjs.
| createIssueHandler = await createIssueMain({ | ||
| ...config, // Pass through most config | ||
| title_prefix: titlePrefix, | ||
| max: maxCount, | ||
| expires: expiresHours, | ||
| }); | ||
| } |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When initializing the create_issue handler for fallback, the configuration is passed using spread operator which may not properly handle all configuration values. Specifically:
- The
labelsfield in the config object might be an array in create_discussion but create_issue expects it to be either an array or comma-separated string - The
allowed_repos,allowed_labelsandtarget-repofields are spread from the original config which is correct - However, discussion-specific fields like
category,close_older_discussions, andrequired_categoryare also being spread and may cause confusion
Consider being more explicit about which configuration fields should be passed to create_issue to avoid unintended behavior.
| // Add fallback-to-issue flag | ||
| if data.SafeOutputs.CreateDiscussions.FallbackToIssue != nil && *data.SafeOutputs.CreateDiscussions.FallbackToIssue { | ||
| customEnvVars = append(customEnvVars, " GH_AW_DISCUSSION_FALLBACK_TO_ISSUE: \"true\"\n") | ||
| } |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The environment variable GH_AW_DISCUSSION_FALLBACK_TO_ISSUE is set here but is not actually used by the JavaScript handler. The create_discussion.cjs handler reads the fallback_to_issue configuration from the config object passed to its main() function, which comes from the GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG JSON (set via addHandlerManagerConfigEnvVar in compiler_safe_outputs_config.go).
This environment variable is redundant and could be removed to avoid confusion. The configuration is already properly passed through the handler config JSON at line 144 in compiler_safe_outputs_config.go via AddBoolPtr("fallback_to_issue", c.FallbackToIssue).
- Created handleFallbackToIssue helper function to consolidate duplicated fallback logic - Both error paths (fetch discussion info and create discussion) now use the helper - Added contextMessage parameter for specific error context - Reduces code duplication by ~40 lines Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🔍 PR Triage ResultsCategory: feature | Risk: medium | Priority: 60/100 Scores Breakdown
📋 Recommended Action: batch_reviewWell-implemented feature that improves reliability by adding a fallback mechanism. The PR includes tests, good documentation, and has undergone review (6 review comments addressed). While ready for merge, it's not urgent enough for fast-track. Can be reviewed alongside other reliability improvement PRs. The refactoring into helper functions shows good code quality. Triaged by PR Triage Agent on 2026-02-04T00:35:56Z
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎉 Yo ho ho! Changeset Generator found the treasure and completed successfully! ⚓💰 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ Smoke test complete - 9/10 tests passed, issue created, discussion commented |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check ✅All required development tools are available in the agent container:
Result: 12/12 tools available ✅ The agent container environment is properly configured with all necessary development tools for workflow execution.
|
|
GitHub MCP (last 2 merged PRs): ✅ Update Docker image version in workflow; [WIP] Update Multi-Device Docs Tester .lock.yml for version v0.0.98
|
|
🧪 Smoke Test Results PR #13612: Download agent file from GitHub on demand Test Results:
Overall: PASS ✅
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Implementation Plan: Discussion-to-Issue Fallback
FallbackToIssuefield toCreateDiscussionsConfigstruct in GoparseDiscussionsConfigto handle the new field with default truefallback_to_issueconfig to create_discussion.cjs via handler configSummary
This PR adds an option to fallback to
safe-outputs.create-issuewhensafe-outputs.create-discussionfails with a permissions issue. The default value istruebut it can be turned off using:Implementation Details
FallbackToIssue *boolfield toCreateDiscussionsConfigwith default value oftruecreate_discussion.cjsto:create_issuehandler when fallback is enabledhandleFallbackToIssuehelper function for better maintainabilityTesting
TestParseDiscussionsConfigFallbackToIssuetest to verify default and explicit configurationOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.