fix: ensure safe-outputs staged mode works for all handler types#21414
fix: ensure safe-outputs staged mode works for all handler types#21414
Conversation
…piler - Add `staged` field to all 38 handler schemas in main_workflow_schema.json (previously only close-pull-request and push-to-pull-request-branch had it; all schemas have `additionalProperties: false` so missing staged caused parser rejection) - Refactor compiler_safe_outputs_env.go: replace per-handler staged flag logic with a single hasSafeOutputWithoutTargetRepo() helper that covers all 40 handlers, fixing the bug where handlers like create-discussion, autofix-code-scanning-alert, noop, etc. never triggered the GH_AW_SAFE_OUTPUTS_STAGED env var - Add TestStagedFlagForAllHandlerTypes covering previously untested handler types Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot, review again and make sure all safe-outputs have the same staged behavior and test cases. |
|
@copilot stage is independent of target-repo, remove special handling |
…l handler types Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 026fc8d. Removed the |
|
@copilot review code and add integration tests in pkg/cli/workflows |
There was a problem hiding this comment.
Pull request overview
Fixes safe-outputs staged mode so it validates across all handler schemas and reliably emits the global staged env var for any configured handler (rather than a hardcoded subset).
Changes:
- Add
stagedsupport to the JSON schema for all safe-output handler config objects (previously missing for most handlers due toadditionalProperties: false). - Update compiler env-var emission to use a centralized “any handler configured” check instead of per-handler hardcoding.
- Expand tests and update the locked workflow output to reflect the staged env var being emitted.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/workflow/compiler_safe_outputs_env.go | Replaces per-handler staged env-var logic with a single global check + helper. |
| pkg/workflow/compiler_safe_outputs_env_test.go | Updates staged/target-repo expectations and adds broad coverage across handler types. |
| pkg/parser/schemas/main_workflow_schema.json | Adds staged to handler schemas so staged options validate under strict schemas. |
| .github/workflows/daily-choice-test.lock.yml | Updates locked workflow output to include GH_AW_SAFE_OUTPUTS_STAGED. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // TestStagedFlagForAllHandlerTypes tests that the staged flag is emitted for all safe output handler types. | ||
| func TestStagedFlagForAllHandlerTypes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| safeOutputs *SafeOutputsConfig | ||
| expectFlag bool | ||
| }{ |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added in 1b120ce. Workflow files in
Integration tests in
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot review all comments |
Addressed in a84b101:
|
Contribution Check ReportHey This PR looks ready for maintainer review. 🎉
Verdict: 🟢 Aligned — quality:
|
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke test (Codex) results:
Warning
|
|
Commit pushed:
|
|
Overall:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Smoke test review: the staged-flag centralization is a clean improvement. One minor nit on the double-nested if in the create-issues block — could be collapsed, but not blocking.
📰 BREAKING: Report filed by Smoke Copilot
| stagedFlagAdded := false | ||
| // Add the global staged env var once if staged mode is enabled, not in trial mode, | ||
| // and at least one handler is configured. Staged mode is independent of target-repo. | ||
| if !c.trialMode && data.SafeOutputs.Staged && hasAnySafeOutputEnabled(data.SafeOutputs) { |
There was a problem hiding this comment.
✅ Clean refactor — centralizing the staged flag check here eliminates the duplicated per-handler stagedFlagAdded logic. The comment is also clear about the intent (Staged mode is independent of target-repo).
| } | ||
| // Check if copilot is in assignees - if so, we'll output issues for assign_to_agent job | ||
| if hasCopilotAssignee(cfg.Assignees) { | ||
| if hasCopilotAssignee(data.SafeOutputs.CreateIssues.Assignees) { |
There was a problem hiding this comment.
Minor nit: the double if nesting could be collapsed into a single if data.SafeOutputs.CreateIssues != nil && hasCopilotAssignee(data.SafeOutputs.CreateIssues.Assignees) for conciseness. Not a blocker though.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Smoke Test Results — Run §23218362372
Overall: PARTIAL (2 skipped, 0 failed)
|
safe-outputs.staged: truesilently broke for most handler types due to two independent bugs.Schema —
stagedrejected by parser for 38/40 handlersAll handler schemas use
additionalProperties: false. Onlyclose-pull-requestandpush-to-pull-request-branchhadstagedin their properties; addingstaged: trueto any other handler (e.g.create-issue,add-comment,create-discussion) caused a parser validation error.Added
stagedfield to all 38 missing handler schemas.Compiler — global staged env var only emitted for 7/40 handlers
addAllSafeOutputConfigEnvVarsemittedGH_AW_SAFE_OUTPUTS_STAGED: "true"only when one of 7 hardcoded handlers was configured. A workflow with:would parse successfully (after the schema fix) but silently drop the staged env var, so the JS runtime never entered staged mode.
Replaced the per-handler logic with a single
hasSafeOutputConfigured()helper that covers all 40 handler types. Staged mode is independent oftarget-repo: it activates wheneverstaged: trueis set and at least one handler is configured, regardless of whether a cross-repotarget-repois specified.Integration Tests
Added workflow files in
pkg/cli/workflows/and integration tests incompile_integration_test.goto cover the fixed scenarios end-to-end:test-staged-create-issue.md/test-staged-add-comment.md/test-staged-create-discussion.md— previously broken by the schema bug; tests verify compilation succeeds andGH_AW_SAFE_OUTPUTS_STAGED: "true"appears in the compiled outputtest-staged-cross-repo.md— verifies staged env var is emitted even whentarget-repois configured on the handlerTestCompileStagedSafeOutputsMultipleHandlers— verifies staged works across multiple handler types in one workflow💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
✨ PR Review Safe Output Test - Run 23218362372