Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/72f88ad8-5de4-45a5-b339-b927650bbd4a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a compiler-time validation step to enforce constraints on safe-outputs.*.max values, with accompanying tests and compiler pipeline wiring.
Changes:
- Introduces
validateSafeOutputsMaxto validatemaxfields across safe-outputs configs (skipping expression values) and dispatch-repository tools. - Wires the new validator into the compiler validation pipeline.
- Adds a dedicated test suite covering common max scenarios and config extraction behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_validation.go | Adds reflection-based validation for max fields, plus dispatch-repository tools validation and error formatting. |
| pkg/workflow/compiler.go | Invokes the new validateSafeOutputsMax during workflow compilation validation. |
| pkg/workflow/safe_outputs_max_validation_test.go | Adds unit + extraction-based tests for the new max validation behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
pkg/workflow/safe_outputs_validation.go:468
- The dispatch_repository tools loop ranges over a map, so the first reported invalid tool (and therefore the error message) can be nondeterministic when multiple tools have invalid max values. Consider collecting tool keys, sorting them, and validating in order for stable compiler output.
// Validate max on dispatch_repository tools (different structure: map of tools)
if config.DispatchRepository != nil {
for toolName, tool := range config.DispatchRepository.Tools {
if tool == nil || tool.Max == nil || isExpressionString(*tool.Max) {
continue
}
pkg/workflow/safe_outputs_max_validation_test.go:177
- Integration-style tests here assume
max: 0and negative values should always be rejected, but the current codebase supports “unlimited” behavior for some safe-outputs via 0/nil (and the spec requiresmax: -1). These cases should be adjusted once the intended canonical semantics are decided, otherwise this test suite will lock in behavior that breaks existing configs/handlers.
t.Run("max of 0 is rejected during config extraction via compiler", func(t *testing.T) {
frontmatter := map[string]any{
"safe-outputs": map[string]any{
"add-comment": map[string]any{
"max": 0,
},
},
}
config := compiler.extractSafeOutputsConfig(frontmatter)
require.NotNil(t, config, "config should be extracted")
err := validateSafeOutputsMax(config)
require.Error(t, err, "max: 0 should fail validation")
assert.Contains(t, err.Error(), "max must be greater than 0", "error message should mention max > 0")
})
t.Run("max of -5 is rejected during config extraction via compiler", func(t *testing.T) {
frontmatter := map[string]any{
"safe-outputs": map[string]any{
"create-issue": map[string]any{
"max": -5,
},
},
}
config := compiler.extractSafeOutputsConfig(frontmatter)
require.NotNil(t, config, "config should be extracted")
err := validateSafeOutputsMax(config)
require.Error(t, err, "max: -5 should fail validation")
assert.Contains(t, err.Error(), "max must be greater than 0", "error message should mention max > 0")
pkg/workflow/safe_outputs_max_validation_test.go:65
- The safe-outputs specification doc in this repo requires supporting unlimited semantics via
max: -1(MR5), but this test (and the new validator) rejects -1. Either update the validator/tests to allow -1 (possibly with a warning), or update the spec + runtime to match the new rule.
t.Run("max of -1 is invalid", func(t *testing.T) {
config := &SafeOutputsConfig{
CreateIssues: &CreateIssuesConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("-1")},
},
}
err := validateSafeOutputsMax(config)
require.Error(t, err, "max: -1 should be invalid")
assert.Contains(t, err.Error(), "max must be greater than 0", "error should mention max > 0")
})
- Files reviewed: 3/3 changed files
- Comments generated: 4
| // Validate max on all named safe output fields that embed BaseSafeOutputConfig | ||
| for fieldName, toolName := range safeOutputFieldMapping { | ||
| field := val.FieldByName(fieldName) | ||
| if !field.IsValid() || field.IsNil() { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Iteration over safeOutputFieldMapping is over a map, so if multiple safe-outputs have invalid max values the reported error can vary between runs due to randomized map iteration order. Consider iterating over a sorted slice of field names (and using that to look up toolName) so the compiler reports a deterministic first error.
This issue also appears on line 463 of the same file.
| if n <= 0 { | ||
| safeOutputsMaxValidationLog.Printf("Invalid max value %d for dispatch_repository tool %s", n, toolName) | ||
| return fmt.Errorf( | ||
| "safe-outputs.dispatch-repository.%s: max must be greater than 0, got %d\n\nThe max field controls how many times this safe output can be triggered.\nProvide a positive integer value (e.g., max: 1 or max: 5)", |
There was a problem hiding this comment.
The error path uses "safe-outputs.dispatch-repository" (dash), but the preferred configuration key is "dispatch_repository" (underscore) per parseDispatchRepositoryConfig (pkg/workflow/dispatch_repository.go:29-31) and the SafeOutputsConfig yaml tag (compiler_types.go). Using the preferred key in the error message (or mentioning both forms) would make it easier for users to locate/fix the setting.
| "safe-outputs.dispatch-repository.%s: max must be greater than 0, got %d\n\nThe max field controls how many times this safe output can be triggered.\nProvide a positive integer value (e.g., max: 1 or max: 5)", | |
| "safe-outputs.dispatch_repository.%s: max must be greater than 0, got %d\n\nThe max field controls how many times this safe output can be triggered.\nProvide a positive integer value (e.g., max: 1 or max: 5)", |
| // validateSafeOutputsMax validates that all max fields in safe-outputs configs are > 0. | ||
| // GitHub Actions expressions (e.g. "${{ inputs.max }}") are not evaluable at compile time | ||
| // and are therefore skipped. | ||
| func validateSafeOutputsMax(config *SafeOutputsConfig) error { | ||
| if config == nil { | ||
| return nil | ||
| } | ||
|
|
||
| safeOutputsMaxValidationLog.Print("Validating safe-outputs max fields") | ||
|
|
||
| val := reflect.ValueOf(config).Elem() | ||
|
|
||
| // Validate max on all named safe output fields that embed BaseSafeOutputConfig | ||
| for fieldName, toolName := range safeOutputFieldMapping { | ||
| field := val.FieldByName(fieldName) | ||
| if !field.IsValid() || field.IsNil() { | ||
| continue | ||
| } | ||
|
|
||
| elem := field.Elem() | ||
| baseCfgField := elem.FieldByName("BaseSafeOutputConfig") | ||
| if !baseCfgField.IsValid() { | ||
| continue | ||
| } | ||
|
|
||
| maxField := baseCfgField.FieldByName("Max") | ||
| if !maxField.IsValid() || maxField.IsNil() { | ||
| continue | ||
| } | ||
|
|
||
| maxPtr, ok := maxField.Interface().(*string) | ||
| if !ok || maxPtr == nil || isExpressionString(*maxPtr) { | ||
| continue | ||
| } | ||
|
|
||
| n, err := strconv.Atoi(*maxPtr) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| if n <= 0 { | ||
| toolDisplayName := strings.ReplaceAll(toolName, "_", "-") | ||
| safeOutputsMaxValidationLog.Printf("Invalid max value %d for %s", n, toolDisplayName) | ||
| return fmt.Errorf( | ||
| "safe-outputs.%s: max must be greater than 0, got %d\n\nThe max field controls how many times this safe output can be triggered.\nProvide a positive integer value (e.g., max: 1 or max: 5)", | ||
| toolDisplayName, n, | ||
| ) |
There was a problem hiding this comment.
This validation enforces max > 0 globally, which conflicts with existing repo semantics where some safe outputs explicitly treat max: 0 / nil as “unlimited” (e.g., create-code-scanning-alert parses with default max 0 “unlimited” in pkg/workflow/create_code_scanning_alert.go:53-60, and multiple JS handlers document “0 means unlimited”). It also conflicts with the safe-outputs specification doc requirement that max: -1 unlimited MUST be supported. The validator likely needs type-specific rules (or support 0/-1 as unlimited) rather than rejecting all <= 0 values.
| t.Run("max of 0 is invalid", func(t *testing.T) { | ||
| config := &SafeOutputsConfig{ | ||
| AddComments: &AddCommentsConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("0")}, | ||
| }, | ||
| } | ||
| err := validateSafeOutputsMax(config) | ||
| require.Error(t, err, "max: 0 should be invalid") | ||
| assert.Contains(t, err.Error(), "max must be greater than 0", "error should mention max > 0") | ||
| assert.Contains(t, err.Error(), "add-comment", "error should mention the field name") |
There was a problem hiding this comment.
This test asserts max: 0 is invalid, but several existing safe-output handlers/configs use 0 (or nil) to mean “unlimited” (e.g., create-code-scanning-alert / missing-tool / missing-data). If the intended behavior is changing globally, the runtime handlers and parsing defaults need to be updated too; otherwise the test should reflect the existing unlimited semantics for the affected types.
This issue also appears in the following locations of the same file:
- line 56
- line 146
| t.Run("max of 0 is invalid", func(t *testing.T) { | |
| config := &SafeOutputsConfig{ | |
| AddComments: &AddCommentsConfig{ | |
| BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("0")}, | |
| }, | |
| } | |
| err := validateSafeOutputsMax(config) | |
| require.Error(t, err, "max: 0 should be invalid") | |
| assert.Contains(t, err.Error(), "max must be greater than 0", "error should mention max > 0") | |
| assert.Contains(t, err.Error(), "add-comment", "error should mention the field name") | |
| t.Run("max of 0 is valid and means unlimited", func(t *testing.T) { | |
| config := &SafeOutputsConfig{ | |
| AddComments: &AddCommentsConfig{ | |
| BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("0")}, | |
| }, | |
| } | |
| err := validateSafeOutputsMax(config) | |
| assert.NoError(t, err, "max: 0 should be valid and treated as unlimited") |
|
@copilot review comments |
…spatch_repository key Agent-Logs-Url: https://github.com/github/gh-aw/sessions/03cc41b8-6408-4874-9d4d-5848612ee6d1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… comment Agent-Logs-Url: https://github.com/github/gh-aw/sessions/03cc41b8-6408-4874-9d4d-5848612ee6d1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all four review points in commits ebaa5b2 and f015ef8:
|
Summary
Adds a validation step that rejects invalid
maxvalues in all safe-outputs configurations. Per the safe-outputs specification, valid values are positive integers (strict limit),-1(unlimited), ornull/omitted (use type default). Values of0and other negative integers (except-1) are rejected as semantically invalid.Changes
pkg/workflow/safe_outputs_validation.go: AddedvalidateSafeOutputsMaxfunction that:safeOutputFieldMapping(via reflection, consistent withsafe_outputs_state.go), with sorted field names for deterministic error reportingmaxvalues are either> 0or exactly-1(unlimited)${{ inputs.max }}) since they can't be evaluated at compile timeDispatchRepositoryConfig.Toolsmap separately (different structure — tools have their ownMaxfield), usingdispatch_repository(underscore) in error messages to match the preferred config keymaxInvalidErrSuffixconstant to avoid duplicationpkg/workflow/compiler.go: CallsvalidateSafeOutputsMaxafter the existing safe-outputs target validation, following the established compiler validation pipeline pattern.pkg/workflow/safe_outputs_max_validation_test.go: New test file covering: nil config, zero max, negative max (-2), valid positive max,-1unlimited, expression skipping,dispatch_repositorytools, and integration tests throughextractSafeOutputsConfig.Error message example