Refactor safe output handlers into sequential processing with handler registry #7809
Conversation
- Created SafeOutputHandler interface for unified handler processing - Implemented handlers for: create_issue, create_discussion, add_comment, update_issue, update_discussion - Added SafeOutputContext to track temporary ID map and handler dependencies - Created handler registry with sequential processing loop - Integrated into buildConsolidatedSafeOutputsJob - Removed old inline handler code for migrated types Co-authored-by: pelikhan <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if len(data.SafeOutputs.AddComments.AllowedReasons) > 0 { | ||
| reasonsJSON, err := json.Marshal(data.SafeOutputs.AddComments.AllowedReasons) | ||
| if err == nil { | ||
| customEnvVars = append(customEnvVars, " GH_AW_ALLOWED_REASONS: \""+string(reasonsJSON)+"\"\n") |
Check failure
Code scanning / CodeQL
Potentially unsafe quoting Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 15 hours ago
In general, the fix is to avoid manually embedding JSON text inside a double-quoted string without escaping. Either: (a) escape backslashes and double quotes before concatenation, or (b) avoid manual quoting and let a structured serializer (YAML/JSON) handle quoting. Since in this file the rest of the environment variables are built as already-quoted YAML lines, the minimal, behavior-preserving fix is to properly escape reasonsJSON for inclusion inside double quotes.
The best targeted fix here is to escape backslashes and double quotes in the JSON string before concatenating it into the GH_AW_ALLOWED_REASONS line. Following the pattern from the background, we should first escape backslashes (\ → \\), then double quotes (" → \"). That way, any embedded quote cannot terminate the surrounding quoted value in the generated workflow. We only need strings.ReplaceAll from the standard library, so we can safely add an import.
Concretely:
- In
pkg/workflow/safe_output_handler_add_comment.go, addstringsto the imports. - In the block that builds
GH_AW_ALLOWED_REASONS, replace the direct use ofstring(reasonsJSON)with a small escaping step:- Convert
reasonsJSONto a string. - Apply
strings.ReplaceAllfor backslashes, then for double quotes. - Use the escaped value in the concatenated env var string.
- Convert
No other behavior changes are needed; the JSON payload remains the same text, just safely quoted for the context in which it is embedded.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "strings" | ||
|
|
||
| "github.com/githubnext/gh-aw/pkg/logger" | ||
| ) | ||
| @@ -58,7 +59,10 @@ | ||
| if len(data.SafeOutputs.AddComments.AllowedReasons) > 0 { | ||
| reasonsJSON, err := json.Marshal(data.SafeOutputs.AddComments.AllowedReasons) | ||
| if err == nil { | ||
| customEnvVars = append(customEnvVars, " GH_AW_ALLOWED_REASONS: \""+string(reasonsJSON)+"\"\n") | ||
| reasonsStr := string(reasonsJSON) | ||
| reasonsStr = strings.ReplaceAll(reasonsStr, `\`, `\\`) | ||
| reasonsStr = strings.ReplaceAll(reasonsStr, `"`, `\"`) | ||
| customEnvVars = append(customEnvVars, " GH_AW_ALLOWED_REASONS: \""+reasonsStr+"\"\n") | ||
| } | ||
| } | ||
|
|
- All unit tests pass for migrated handlers - Compilation test confirms sequential processing works - Temporary ID map correctly passed between handlers - Outputs properly defined for all handlers Co-authored-by: pelikhan <[email protected]>
|
@copilpt also process close_issue close_discussion handlers |
|
@copilot also process close_issue close_discussion handlers |
- Created CloseIssueHandler with temporary ID map support - Created CloseDiscussionHandler with temporary ID map support - Updated handler registry to include close handlers - Removed inline close handler code from compiler_safe_outputs_core.go - All tests pass with new handlers integrated Co-authored-by: pelikhan <[email protected]>
Added
The handlers are registered in this order: create_issue → create_discussion → update_issue → update_discussion → close_issue → close_discussion → add_comment |
|
🤖 DIAGNOSTIC COMPLETE: Smoke Copilot No Firewall STATUS: ALL_UNITS_OPERATIONAL. MISSION_SUCCESS. |
|
✅ Firewall validation complete... Smoke Codex Firewall confirmed network sandboxing is operational. 🛡️ |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🎉 Yo ho ho! Smoke Copilot Safe Inputs found the treasure and completed successfully! ⚓💰 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
📰 VERDICT: Smoke Copilot Playwright has concluded. All systems operational. This is a developing story. 🎤 |
|
🎉 Yo ho ho! Changeset Generator found the treasure and completed successfully! ⚓💰 |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test Results (Run 20530429091)Last 2 Merged PRs:
Test Results:
Overall Status: PASS (6/7 tests passed - gateway not required for core functionality) cc: @pelikhan
|
Smoke Test Results: Copilot Engine (No Firewall)Last 2 Merged PRs:
Test Results:
Status: PASS (4/5 tests successful)
|
Smoke Test Results✅ File Writing: Created Overall Status: PARTIAL PASS (2/3 tests passed, 1 tool unavailable)
|
|
Firewall smoke test (Codex + AWF)
|
Smoke Test Results❌ Playwright MCP: Failed - Chromium download blocked in restricted environment Overall Status: ❌ FAIL (1/2 tests passed, 1 skipped)
|
Smoke Test Results - Claude (Sonnet 4.5)Last 2 Merged PRs:
Test Results:
Overall Status: PASS (5/6 tests passed)
|
|
PRs reviewed: Refactor: Extract step-building helpers from buildConsolidatedSafeOutputsJob; Update jsweep to process 3 files per round with comprehensive testing
|
|
@copilot add update_issue, update_discussion |
- Removed duplicate NOTE comment on line 241 - Update handlers are already properly migrated and noted in line 147 - No functional changes, just cleanup Co-authored-by: pelikhan <[email protected]>
I cleaned up a redundant NOTE comment in commit 17cee39. All 7 handlers now process through the registry: create_issue → create_discussion → update_issue → update_discussion → close_issue → close_discussion → add_comment |
Refactor Safe Output Handlers into Sequential Processing Loop
Implementation Complete ✅
This PR successfully refactors the safe output handler system to use a unified handler registry with sequential message processing.
Changes Made
New Architecture:
SafeOutputHandlerinterface for unified handler processingSafeOutputContextto track temporary ID map and handler dependenciesbuildConsolidatedSafeOutputsJobMigrated Handlers:
create_issue- Generates temporary ID mapcreate_discussion- Consumes temporary ID mapupdate_issue- Consumes temporary ID map ✅update_discussion- Consumes temporary ID map ✅close_issue- Consumes temporary ID mapclose_discussion- Consumes temporary ID mapadd_comment- Consumes temporary ID map and references other handler outputsKey Features:
Processing Order:
Testing:
Files Changed
New Files:
pkg/workflow/safe_output_handler.go- Handler interface and registrypkg/workflow/safe_output_handler_processor.go- Sequential processing looppkg/workflow/safe_output_handler_create_issue.go- Create issue handlerpkg/workflow/safe_output_handler_create_discussion.go- Create discussion handlerpkg/workflow/safe_output_handler_add_comment.go- Add comment handlerpkg/workflow/safe_output_handler_update_issue.go- Update issue handler ✅pkg/workflow/safe_output_handler_update_discussion.go- Update discussion handler ✅pkg/workflow/safe_output_handler_close_issue.go- Close issue handlerpkg/workflow/safe_output_handler_close_discussion.go- Close discussion handlerModified Files:
pkg/workflow/compiler_safe_outputs_core.go- Integrated handler registry and removed inline codeFuture Work
Other safe output handlers (create_pull_request, close_pull_request, etc.) can be migrated to this system in future PRs following the same pattern.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.