Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/daily-choice-test.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .github/workflows/mcp-inspector.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .github/workflows/notion-issue-summary.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .github/workflows/smoke-copilot-arm.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .github/workflows/smoke-copilot.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 47 additions & 0 deletions pkg/workflow/compiler_safe_outputs_steps.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package workflow

import (
"encoding/json"
"fmt"
"sort"
"strings"

"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/stringutil"
)

var consolidatedSafeOutputsStepsLog = logger.New("workflow:compiler_safe_outputs_steps")
Expand Down Expand Up @@ -331,6 +334,14 @@ func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) []string {
// Note: The project handler manager has been removed.
// All project-related operations are now handled by the unified handler.

// Add GH_AW_SAFE_OUTPUT_JOBS so the handler manager knows which message types are
// handled by custom safe-output job steps and should be silently skipped rather than
// reported as "No handler loaded for message type '...'".
if customJobsJSON := buildCustomSafeOutputJobsJSON(data); customJobsJSON != "" {
steps = append(steps, fmt.Sprintf(" GH_AW_SAFE_OUTPUT_JOBS: %q\n", customJobsJSON))
consolidatedSafeOutputsStepsLog.Print("Added GH_AW_SAFE_OUTPUT_JOBS env var for custom safe job types")
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log message says "Added GH_AW_SAFE_OUTPUT_JOBS env var for custom safe job types" but uses consolidatedSafeOutputsStepsLog.Print (debug-level). Consider upgrading to a higher-visibility log or adding a comment explaining this is intentionally debug-only, since this is a new feature that was hard to diagnose when missing.

// Add custom safe output env vars
c.addCustomSafeOutputEnvVars(&steps, data)

Expand Down Expand Up @@ -433,3 +444,39 @@ func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) []string {

return steps
}

// buildCustomSafeOutputJobsJSON builds a JSON mapping of custom safe output job names to empty
// strings, for use in the GH_AW_SAFE_OUTPUT_JOBS env var of the handler manager step.
// This allows the handler manager to silently skip messages handled by custom safe-output job
// steps rather than reporting them as "No handler loaded for message type '...'".
func buildCustomSafeOutputJobsJSON(data *WorkflowData) string {
if data.SafeOutputs == nil || len(data.SafeOutputs.Jobs) == 0 {
return ""
}

// Build mapping of normalized job names to empty strings (no URL output for custom jobs)
jobMapping := make(map[string]string, len(data.SafeOutputs.Jobs))
for jobName := range data.SafeOutputs.Jobs {
normalizedName := stringutil.NormalizeSafeOutputIdentifier(jobName)
jobMapping[normalizedName] = ""
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordered map is built after sorting keys, but Go maps have no guaranteed iteration order, so marshaling ordered directly won't preserve the sorted order. Consider using a sorted slice of key-value pairs or a custom JSON marshaler to ensure deterministic output. Alternatively, just marshal jobMapping directly — the sort loop doesn't help here.


// Sort keys for deterministic output
keys := make([]string, 0, len(jobMapping))
for k := range jobMapping {
keys = append(keys, k)
}
sort.Strings(keys)

ordered := make(map[string]string, len(keys))
for _, k := range keys {
ordered[k] = jobMapping[k]
}
Comment on lines +464 to +474
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key-sorting/ordered map logic here is misleading: Go maps don’t preserve insertion order, and encoding/json already sorts string map keys deterministically during marshaling. Consider removing the keys/sort.Strings/ordered block (and the sort import) and marshal jobMapping directly (or, if you truly need a custom order, marshal from a sorted slice/struct instead of a map).

Copilot uses AI. Check for mistakes.

jsonBytes, err := json.Marshal(ordered)
if err != nil {
consolidatedSafeOutputsStepsLog.Printf("Warning: failed to marshal custom safe output jobs: %v", err)
return ""
}
return string(jsonBytes)
}
23 changes: 23 additions & 0 deletions pkg/workflow/compiler_safe_outputs_steps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,29 @@ func TestBuildHandlerManagerStep(t *testing.T) {
},
// Note: create_project is now handled by the unified handler manager,
// not the separate project handler manager
{
name: "handler manager with custom safe jobs includes GH_AW_SAFE_OUTPUT_JOBS",
safeOutputs: &SafeOutputsConfig{
CreateIssues: &CreateIssuesConfig{},
Jobs: map[string]*SafeJobConfig{
"send-slack-message": {
Description: "Send a Slack message",
},
},
},
checkContains: []string{
"GH_AW_SAFE_OUTPUT_JOBS: \"{\\\"send_slack_message\\\":\\\"\\\"}\"",
},
},
{
name: "handler manager without custom safe jobs does not include GH_AW_SAFE_OUTPUT_JOBS",
safeOutputs: &SafeOutputsConfig{
CreateIssues: &CreateIssuesConfig{},
},
checkNotContains: []string{
"GH_AW_SAFE_OUTPUT_JOBS",
},
},
}

for _, tt := range tests {
Expand Down
Loading