-
Notifications
You must be signed in to change notification settings - Fork 34
Convert PR-related safe outputs and hide-comment to handler manager architecture #8683
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
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add import for replaceTemporaryIdReferences helper - Parse and apply temporary ID map from handler manager output - Enable PRs to reference issues created earlier in workflow using temporary IDs (#aw_xxx) - Update Go compiler to pass GH_AW_TEMPORARY_ID_MAP to create_pull_request step Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add comprehensive test verifying GH_AW_TEMPORARY_ID_MAP env var is passed - Test both step config and consolidated job scenarios - Verify integration with handler manager outputs - Tests pass successfully Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot continue full conversion |
I'm proceeding with the full conversion of create-pull-request to the handler manager architecture. This is a substantial refactoring given the file's complexity (705 lines with extensive git operations, fallback logic, and error handling - nearly 2x the size of create-issue). I've updated the handler manager infrastructure:
Now converting the JavaScript file to the handler factory pattern. This involves:
Working on the conversion now... |
- Added create_pull_request to HANDLER_MAP in safe_output_handler_manager.cjs - Removed create_pull_request from STANDALONE_STEP_TYPES - This enables handler manager to process create_pull_request messages - JavaScript conversion to handler factory pattern still needed (705 lines, complex git operations) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Created comprehensive conversion plan in docs/ - Details all 9 steps needed for JavaScript conversion - Documents 31 error handling locations to convert - Estimates 3-4 hours for careful refactoring - Infrastructure changes already complete (handler manager updated) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot continue conversion |
- Converted main() to handler factory pattern returning message handler function - Added configuration extraction from config parameter - Removed agent output file reading and JSON parsing (message passed directly) - Converted all error handling to return result objects (24 return statements) - Removed all core.setOutput calls (replaced with return values) - Updated label/draft/titlePrefix to use config + message values - Fixed temporary ID resolution to use resolvedTemporaryIds parameter - Added proper closing braces for handler and factory functions - All syntax checks pass, linting clean, unit tests pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the full JavaScript conversion in commit 2f22daf. The create-pull-request handler is now fully converted to the handler manager architecture with all 9 steps completed: ✅ Handler factory pattern implemented All validation passes:
The handler now returns structured result objects for all code paths (success, fallback, staged, error, skipped) while preserving all existing functionality including git operations, fallback logic, and validation. |
|
@copilot review converted handler carefully |
- Removed environment variable reading for GH_AW_TEMPORARY_ID_MAP - Fixed parameter shadowing issue where local variable overwrote parameter - Handler now properly uses resolvedTemporaryIds parameter passed from handler manager - This ensures temporary ID resolution works correctly in handler manager architecture Critical bugs fixed: 1. Parameter shadowing: local resolvedTemporaryIds variable shadowed function parameter 2. Environment dependency: was reading from env var instead of using parameter All validation passes after fix: - JavaScript syntax ✅ - Linting (make lint-cjs) ✅ - Unit tests (make test-unit) ✅ Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Review completed. Found and fixed 2 critical bugs in commit d3795a4: Bug 1: Parameter Shadowing
Bug 2: Environment Variable Dependency
Fix Applied: Validation After Fix: Handler conversion is now complete and correct. |
* Initial plan * Fix lint-go failure: use require.NoError for error assertions Replace assert.NoError with require.NoError in create_pull_request_temporary_id_test.go to comply with testifylint rule. Error assertions should use require to stop test execution if setup fails. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot refactor the steps from the create_pull_request job and also the permissions to have contents: write |
|
🤖 DIAGNOSTIC COMPLETE: Smoke Copilot No Firewall STATUS: ALL_UNITS_OPERATIONAL. MISSION_SUCCESS. |
|
🎬 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. 🎤 |
|
✅ Firewall validation complete... Smoke Codex Firewall confirmed network sandboxing is operational. 🛡️ |
|
🎉 Yo ho ho! Changeset Generator found the treasure and completed successfully! ⚓💰 |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Firewall smoke test results:
|
Copilot Smoke Test ResultsLast 2 Merged PRs: #8705 (Fix lint Go issues), #8704 (Fix command injection validation) ✅ GitHub MCP Testing - Retrieved PR data Status: PASS cc: @pelikhan
|
Smoke Test Results: Copilot Engine (No Firewall)✅ Test 1: Review merged PRs - SKIPPED (401 authentication error) Note: GitHub API authentication is not configured for this workflow run. Tests 2 and 3 passed successfully.
|
Smoke Test Results: Copilot Safe Inputs✅ File Writing: Created test file successfully Status: PARTIAL PASS (2/3) Note:
|
Smoke Test Results - Claude EngineLast 2 Merged PRs:
Test Results:
Overall Status: PASS (5/6 tests successful)
|
|
PRs: [WIP] Fix lint Go issues; Add enhanced git patch generation with conditional creation and status monitoring for agentic workflows
|
Smoke Test Results (Run 20672814179)❌ Playwright MCP: Cannot test - requires Copilot MCP integration (not available in bash) Overall Status:
|
|
@copilot review checkout pr branch inclusion in safe_outputs job (and other steps necessary to support push_to_pull_request_branch |
The checkout PR branch steps are correctly included in the safe_outputs job to support push_to_pull_request_branch and create-pull-request handlers. Here's what's working: Current Implementation (✅ Correct):
Generated Workflow Example (changeset.lock.yml): - name: Download patch artifact
uses: actions/download-artifact@...
with:
name: aw.patch
path: /tmp/gh-aw/
- name: Checkout repository
if: contains(needs.agent.outputs.output_types, 'push_to_pull_request_branch')
uses: actions/checkout@...
with:
token: ${{ steps.app-token.outputs.token }}
persist-credentials: false
fetch-depth: 1
- name: Configure Git credentials
if: contains(needs.agent.outputs.output_types, 'push_to_pull_request_branch')
run: |
git config --global user.email "github-actions[bot]@users.noreply.github.com"
git config --global user.name "github-actions[bot]"
git remote set-url origin "https://x-access-token:$TOKEN@..."All Requirements Met: No changes needed - the implementation is correct and complete. |
Convert PR-related safe outputs and hide-comment to handler manager architecture ✅
This PR completes the conversion of four PR-related safe output types plus hide-comment to use the handler manager architecture, consistent with how
create-issueand other safe outputs are handled.Status: Five Conversions Complete ✅
All conversions are complete, TypeScript and linting passing, code cleaned up, Go formatting applied, legacy code removed, obsolete tests deleted.
Recent Fixes
Latest commit - Remove obsolete JS test files:
Previous commits:
Validation ✅
Code Quality
Handler Pattern Compliance
All five handlers now:
Summary
All five safe output types have been successfully converted to the handler manager architecture with all TypeScript, JavaScript, and Go code formatted and linted. All planning documents removed. All obsolete step generation code removed. All legacy script getter infrastructure removed. All obsolete test files deleted. Everything is clean and ready for production.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Changeset
hide-commentto the handler manager architecture. Internal refactor only; no user-facing API changes.Changeset
hide-commentto the handler manager architecture. Internal refactor only; no user-facing API changes.