Fix undefined property error for comment outputs in workflows without reactions#3831
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…nt env vars Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where the compiler unconditionally referenced needs.activation.outputs.comment_id and needs.activation.outputs.comment_repo in safe output jobs, even though these outputs are only created when a reaction: is configured in the workflow frontmatter. The fix adds conditional logic to only include these environment variables when data.AIReaction != "" && data.AIReaction != "none", matching the condition under which the activation job creates these outputs.
Key changes:
- Added conditional logic in
create_pull_request.goandpush_to_pull_request_branch.goto only reference comment outputs when reactions are configured - Comprehensive test suite covering workflows with reactions, without reactions, and with
reaction: none - Updated existing test to include
reaction:configuration
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/create_pull_request.go | Added conditional check before adding GH_AW_COMMENT_ID and GH_AW_COMMENT_REPO env vars |
| pkg/workflow/push_to_pull_request_branch.go | Added conditional check before adding GH_AW_COMMENT_ID and GH_AW_COMMENT_REPO env vars |
| pkg/workflow/comment_env_vars_conditional_test.go | New comprehensive test suite covering all scenarios (with/without/none reactions) |
| pkg/workflow/push_to_pull_request_branch_test.go | Added reaction: rocket to existing test to ensure env vars are present |
| .github/workflows/*.lock.yml | Regenerated lock files removing undefined comment env var references from workflows without reactions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot merge main, lint, fix tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed all tasks:
The merge brought in recent changes including fuzz testing, documentation improvements, and safe output refactoring. All tests pass successfully. (commit 4baedb8) |
Fix undefined property error for comment outputs in workflows without reactions
Problem Analysis
The workflow referenced
needs.activation.outputs.comment_idandneeds.activation.outputs.comment_repoin thecreate_pull_requestjob, but these outputs were not defined in the activation job.Root Cause:
pkg/workflow/create_pull_request.go(lines 91-92) unconditionally added environment variables that referenced these outputsreaction:is configured in the workflow frontmatter (seepkg/workflow/compiler_jobs.golines 560-563)Implementation Complete ✅
pkg/workflow/create_pull_request.goto conditionally add comment environment variables only when neededpkg/workflow/push_to_pull_request_branch.goto apply the same fixChanges Made
GH_AW_COMMENT_IDandGH_AW_COMMENT_REPOenv vars whendata.AIReaction != "" && data.AIReaction != "none"reaction: rocketin theon:sectionVerification Results
Recent Merge
Merged main branch (commits 09b416e, 870057e, 59bea31) which includes:
Original 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.