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
4 changes: 2 additions & 2 deletions pkg/workflow/frontmatter_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ func TestParseFrontmatterConfig(t *testing.T) {
t.Error("expected error for non-expression string timeout-minutes, got nil")
return
}
if !strings.Contains(err.Error(), "timeout-minutes") {
t.Errorf("error message should mention 'timeout-minutes', got: %v", err)
if !strings.Contains(err.Error(), "must be an integer or a GitHub Actions expression") {
t.Errorf("error message should describe the integer/expression requirement, got: %v", err)
}
})

Expand Down
28 changes: 24 additions & 4 deletions pkg/workflow/model_costs_pricing_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,36 @@ func validateDefaultAiCreditsPricing(workflowData *WorkflowData) error {
)
}
if p.Input <= 0 {
return fmt.Errorf("models.default-ai-credits-pricing: input must be a positive value (got %g); use a small positive rate such as 0.000001 for effectively-free self-hosted models", p.Input)
return NewValidationError(
"models.default-ai-credits-pricing.input",
fmt.Sprintf("%g", p.Input),
fmt.Sprintf("input must be a positive value, got %g. Expected a value greater than 0.", p.Input),
"Set a positive input rate.\n\nExample:\nmodels:\n default-ai-credits-pricing:\n input: 0.000001\n output: 0.000001",
)
}
if p.Output <= 0 {
return fmt.Errorf("models.default-ai-credits-pricing: output must be a positive value (got %g); use a small positive rate such as 0.000001 for effectively-free self-hosted models", p.Output)
return NewValidationError(
"models.default-ai-credits-pricing.output",
fmt.Sprintf("%g", p.Output),
fmt.Sprintf("output must be a positive value, got %g. Expected a value greater than 0.", p.Output),
"Set a positive output rate.\n\nExample:\nmodels:\n default-ai-credits-pricing:\n input: 0.000001\n output: 0.000001",
)
}
if p.CachedInput != nil && *p.CachedInput <= 0 {
return fmt.Errorf("models.default-ai-credits-pricing: cache_read must be a positive value when set (got %g)", *p.CachedInput)
return NewValidationError(
"models.default-ai-credits-pricing.cache_read",
fmt.Sprintf("%g", *p.CachedInput),
fmt.Sprintf("cache_read must be a positive value when set, got %g. Expected a value greater than 0.", *p.CachedInput),
"Set cache_read to a positive value when you configure it.\n\nExample:\nmodels:\n default-ai-credits-pricing:\n input: 0.000001\n output: 0.000001\n cache_read: 0.0000005",
)
}
if p.CacheWrite != nil && *p.CacheWrite <= 0 {
return fmt.Errorf("models.default-ai-credits-pricing: cache_write must be a positive value when set (got %g)", *p.CacheWrite)
return NewValidationError(
"models.default-ai-credits-pricing.cache_write",
fmt.Sprintf("%g", *p.CacheWrite),
fmt.Sprintf("cache_write must be a positive value when set, got %g. Expected a value greater than 0.", *p.CacheWrite),
"Set cache_write to a positive value when you configure it.\n\nExample:\nmodels:\n default-ai-credits-pricing:\n input: 0.000001\n output: 0.000001\n cache_write: 0.00000125",
)
}
modelCostsPricingValidationLog.Printf("Validated default-ai-credits-pricing: input=%g output=%g", p.Input, p.Output)
return nil
Expand Down
1 change: 1 addition & 0 deletions pkg/workflow/model_costs_pricing_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ func TestValidateDefaultAiCreditsPricing(t *testing.T) {
require.Error(t, err)
assert.Contains(t, err.Error(), "input")
assert.Contains(t, err.Error(), "positive")
assert.Contains(t, err.Error(), "Example:")
})

t.Run("zero output is rejected", func(t *testing.T) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/workflow/safe_outputs_target_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,9 @@ func TestValidateSafeOutputsTarget(t *testing.T) {
if !strings.Contains(err.Error(), tt.errText) {
t.Errorf("validateSafeOutputsTarget() error = %v, should contain %q", err, tt.errText)
}
if !strings.Contains(err.Error(), "Example:") {
t.Errorf("validateSafeOutputsTarget() error = %v, should contain %q", err, "Example:")
}
}
})
}
Expand Down Expand Up @@ -406,6 +409,9 @@ func TestValidateTargetValue(t *testing.T) {
if !strings.Contains(err.Error(), tt.errText) {
t.Errorf("validateTargetValue() error = %v, should contain %q", err, tt.errText)
}
if !strings.Contains(err.Error(), "Example:") {
t.Errorf("validateTargetValue() error = %v, should contain %q", err, "Example:")
}
}
})
}
Expand Down
19 changes: 13 additions & 6 deletions pkg/workflow/safe_outputs_urls_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,23 @@

package workflow

import "testing"
import (
"strings"
"testing"
)

func TestValidateSafeOutputsURLs(t *testing.T) {
tests := []struct {
name string
config *SafeOutputsConfig
wantErr bool
errText string
}{
{name: "nil config", config: nil, wantErr: false},
{name: "empty policy", config: &SafeOutputsConfig{}, wantErr: false},
{name: "allowed-only", config: &SafeOutputsConfig{URLs: SafeOutputsURLsPolicyAllowedOnly}, wantErr: false},
{name: "allowed-or-code-region", config: &SafeOutputsConfig{URLs: SafeOutputsURLsPolicyAllowedOrCodeRegion}, wantErr: false},
{name: "invalid", config: &SafeOutputsConfig{URLs: "unknown"}, wantErr: true},
{name: "nil config", config: nil, wantErr: false, errText: ""},
{name: "empty policy", config: &SafeOutputsConfig{}, wantErr: false, errText: ""},
{name: "allowed-only", config: &SafeOutputsConfig{URLs: SafeOutputsURLsPolicyAllowedOnly}, wantErr: false, errText: ""},
{name: "allowed-or-code-region", config: &SafeOutputsConfig{URLs: SafeOutputsURLsPolicyAllowedOrCodeRegion}, wantErr: false, errText: ""},
{name: "invalid", config: &SafeOutputsConfig{URLs: "unknown"}, wantErr: true, errText: "Example:"},
}

for _, tt := range tests {
Expand All @@ -23,6 +27,9 @@ func TestValidateSafeOutputsURLs(t *testing.T) {
if (err != nil) != tt.wantErr {
t.Fatalf("validateSafeOutputsURLs() error = %v, wantErr %v", err, tt.wantErr)
}
if tt.wantErr && tt.errText != "" && (err == nil || !strings.Contains(err.Error(), tt.errText)) {
t.Fatalf("validateSafeOutputsURLs() error = %v, should contain %q", err, tt.errText)
}
})
}
}
14 changes: 10 additions & 4 deletions pkg/workflow/safe_outputs_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ func validateSafeOutputsURLs(config *SafeOutputsConfig) error {
return nil
default:
return fmt.Errorf(
"safe-outputs.urls: invalid value %q (expected one of: %q, %q)",
"safe-outputs.urls: invalid value %q. Expected one of: %q, %q. Example:\n safe-outputs:\n urls: %q",
config.URLs,
SafeOutputsURLsPolicyAllowedOnly,
SafeOutputsURLsPolicyAllowedOrCodeRegion,
SafeOutputsURLsPolicyAllowedOnly,
)
}
}
Expand Down Expand Up @@ -203,12 +204,17 @@ func validateTargetValue(configName, target string) error {
if target == "event" || strings.Contains(target, "github.event") {
suggestion = "\n\nDid you mean to use \"${{ github.event.issue.number }}\" instead of \"" + target + "\"?"
}
exampleTargetKey := configName
if idx := strings.LastIndex(exampleTargetKey, "."); idx >= 0 {
exampleTargetKey = exampleTargetKey[idx+1:]
}

// Invalid target value
return fmt.Errorf(
"invalid target value for %s: %q\n\nValid target values are:\n - \"triggering\" (default) - targets the triggering issue/PR/discussion\n - \"*\" - targets any item specified in the output\n - A positive integer (e.g., \"123\")\n - A GitHub Actions expression (e.g., \"${{ github.event.issue.number }}\")%s",
"invalid target value for %s: %q. Expected one of: \"triggering\", \"*\", a positive integer like \"123\", or a GitHub Actions expression like \"${{ github.event.issue.number }}\".\n\nExample:\n safe-outputs:\n %s:\n target: \"triggering\"%s",
configName,
target,
exampleTargetKey,
suggestion,
)
}
Expand All @@ -229,15 +235,15 @@ func validateSafeOutputsMergePullRequest(config *SafeOutputsConfig) error {
validateNonEmptyStringList := func(field string, values []string) error {
for i, value := range values {
if strings.TrimSpace(value) == "" {
return fmt.Errorf("safe-outputs.merge-pull-request.%s[%d] cannot be empty", field, i)
return fmt.Errorf("safe-outputs.merge-pull-request.%s[%d] cannot be empty. Expected a non-empty string value. Example:\n safe-outputs:\n merge-pull-request:\n %s:\n - \"safe-to-merge\"", field, i, field)
}
}
return nil
}

validateRefGlobList := func(field string, patterns []string) error {
return validateGlobPatternList(patterns, validateRefGlob, func(i int, pat string, msgs []string) error {
return fmt.Errorf("invalid glob pattern %q in safe-outputs.merge-pull-request.%s[%d]: %s", pat, field, i, strings.Join(msgs, "; "))
return fmt.Errorf("invalid glob pattern %q in safe-outputs.merge-pull-request.%s[%d]: %s. Expected a valid ref glob pattern. Example:\n safe-outputs:\n merge-pull-request:\n %s:\n - \"feature/*\"", pat, field, i, strings.Join(msgs, "; "), field)
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func TestValidateSafeOutputsMergePullRequestLabelValidation(t *testing.T) {
}
require.Error(t, err, "expected merge-pull-request label validation to fail")
require.ErrorContains(t, err, tt.wantErr, "expected validation error to include field-specific message")
require.ErrorContains(t, err, "Example:", "expected validation error to include an example")
})
}
}
Expand Down Expand Up @@ -93,6 +94,7 @@ func TestValidateSafeOutputsMergePullRequestAllowedBranchesValidation(t *testing

require.Error(t, err, "expected merge-pull-request allowed-branches validation to fail")
require.ErrorContains(t, err, tt.wantErr, "expected field-specific allowed-branches error")
require.ErrorContains(t, err, "Example:", "expected validation error to include an example")
})
}
}
12 changes: 6 additions & 6 deletions pkg/workflow/templatables.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import (

var templatablesLog = logger.New("workflow:templatables")

const templatableBoolErrorExample = "value must be a boolean or a GitHub Actions expression (e.g. '${{ inputs.flag }}')"
const templatableBoolErrorExample = "value must be a boolean or a GitHub Actions expression. Expected true, false, or an expression string. Example: <field>: true or <field>: ${{ inputs.flag }}"

// TemplatableInt32 represents an integer frontmatter field that also accepts
// GitHub Actions expression strings (e.g. "${{ inputs.timeout }}"). The
Expand Down Expand Up @@ -72,11 +72,11 @@ func (t *TemplatableInt32) UnmarshalJSON(data []byte) error {
var s string
if err := json.Unmarshal(data, &s); err != nil {
templatablesLog.Printf("TemplatableInt32 rejected: not number or string: %s", data)
return fmt.Errorf("timeout-minutes must be an integer or a GitHub Actions expression (e.g. '${{ inputs.timeout }}'), got %s", data)
return fmt.Errorf("value must be an integer or a GitHub Actions expression, got %s. Expected an integer literal or an expression string. Example: <field>: 30 or <field>: ${{ inputs.timeout }}", data)
}
if !isExpression(s) {
templatablesLog.Printf("TemplatableInt32 rejected non-expression string: %q", s)
return fmt.Errorf("timeout-minutes must be an integer or a GitHub Actions expression (e.g. '${{ inputs.timeout }}'), got string %q", s)
return fmt.Errorf("value must be an integer or a GitHub Actions expression, got string %q. Expected an integer literal or an expression string. Example: <field>: 30 or <field>: ${{ inputs.timeout }}", s)
}
*t = TemplatableInt32(s)
return nil
Expand Down Expand Up @@ -329,7 +329,7 @@ func defaultIntStr(n int) *string {
return &s
}

const templatableBoolOrIntErrorExample = "value must be a boolean, a non-negative integer (0–100), or a GitHub Actions expression (e.g. '${{ inputs.dedup }}')"
const templatableBoolOrIntErrorExample = "value must be a boolean, a non-negative integer (0–100), or a GitHub Actions expression. Expected true/false, an integer from 0 to 100, or an expression string. Example: deduplicate-by-title: true, deduplicate-by-title: 1, or deduplicate-by-title: ${{ inputs.dedup }}"

// TemplatableBoolOrInt represents a field that accepts a boolean, a non-negative integer
// (0–100), or a GitHub Actions expression string (e.g. "${{ inputs.dedup }}").
Expand Down Expand Up @@ -365,7 +365,7 @@ func (t *TemplatableBoolOrInt) UnmarshalYAML(node *yaml.Node) error {
case "!!int":
n, err := strconv.Atoi(node.Value)
if err != nil || n < 0 || n > 100 {
return fmt.Errorf("integer must be between 0 and 100, got %q", node.Value)
return fmt.Errorf("integer must be between 0 and 100, got %q. Expected a value in that range. Example: deduplicate-by-title: 1", node.Value)
}
*t = TemplatableBoolOrInt(node.Value)
return nil
Expand Down Expand Up @@ -394,7 +394,7 @@ func (t *TemplatableBoolOrInt) UnmarshalJSON(data []byte) error {
var n int
if err := json.Unmarshal(data, &n); err == nil {
if n < 0 || n > 100 {
return fmt.Errorf("integer must be between 0 and 100, got %d", n)
return fmt.Errorf("integer must be between 0 and 100, got %d. Expected a value in that range. Example: deduplicate-by-title: 1", n)
}
*t = TemplatableBoolOrInt(strconv.Itoa(n))
return nil
Expand Down
1 change: 1 addition & 0 deletions pkg/workflow/templatables_bool_or_int_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ func TestTemplatableBoolOrIntUnmarshalJSONRejectsFloat(t *testing.T) {
var value TemplatableBoolOrInt
err := json.Unmarshal([]byte("1.5"), &value)
require.Error(t, err, "float input should be rejected")
require.ErrorContains(t, err, "Example:", "error should include a corrective example")
}
17 changes: 10 additions & 7 deletions pkg/workflow/utc_offset.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package workflow

import (
"errors"
"fmt"
"regexp"
"strconv"
Expand All @@ -15,26 +14,30 @@ var utcOffsetLog = logger.New("workflow:utc_offset")

var utcOffsetPattern = regexp.MustCompile(`^([+-])(\d{2}):(\d{2})$`)

func utcOffsetFormatError(value string) error {
return fmt.Errorf("must be a numeric UTC offset like +00:00 or -08:00, got %q. Expected format ±HH:MM in the range -14:00 to +14:00. Example: utc: \"-08:00\"", value)
}

// NormalizeUTCOffset validates and normalizes a numeric UTC offset.
func NormalizeUTCOffset(raw string) (string, error) {
trimmed := strings.TrimSpace(raw)
matches := utcOffsetPattern.FindStringSubmatch(trimmed)
if matches == nil {
utcOffsetLog.Printf("UTC offset %q does not match expected +HH:MM/-HH:MM format", trimmed)
return "", errors.New("must be a numeric UTC offset like +00:00 or -08:00")
return "", utcOffsetFormatError(trimmed)
}

hours, err := strconv.Atoi(matches[2])
if err != nil {
return "", errors.New("must be a numeric UTC offset like +00:00 or -08:00")
return "", utcOffsetFormatError(trimmed)
}
minutes, err := strconv.Atoi(matches[3])
if err != nil {
return "", errors.New("must be a numeric UTC offset like +00:00 or -08:00")
return "", utcOffsetFormatError(trimmed)
}
if hours > 14 || minutes > 59 || (hours == 14 && minutes != 0) {
utcOffsetLog.Printf("UTC offset %q out of range (hours=%d, minutes=%d)", trimmed, hours, minutes)
return "", errors.New("must be a numeric UTC offset like +00:00 or -08:00")
return "", utcOffsetFormatError(trimmed)
}

normalized := fmt.Sprintf("%s%02d:%02d", matches[1], hours, minutes)
Expand All @@ -51,11 +54,11 @@ func ParseUTCOffsetLocation(raw string) (*time.Location, error) {

hours, err := strconv.Atoi(normalized[1:3])
if err != nil {
return nil, fmt.Errorf("invalid UTC offset format: %w", err)
return nil, fmt.Errorf("normalized UTC offset %q could not be parsed. Expected normalized format ±HH:MM. Example: +00:00. Underlying error: %w", normalized, err)
}
minutes, err := strconv.Atoi(normalized[4:6])
if err != nil {
return nil, fmt.Errorf("invalid UTC offset format: %w", err)
return nil, fmt.Errorf("normalized UTC offset %q could not be parsed. Expected normalized format ±HH:MM. Example: -08:00. Underlying error: %w", normalized, err)
}
offsetSeconds := hours*60*60 + minutes*60
if normalized[0] == '-' {
Expand Down
23 changes: 23 additions & 0 deletions pkg/workflow/utc_offset_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//go:build !integration

package workflow

import (
"testing"

"github.com/stretchr/testify/require"
)

func TestNormalizeUTCOffset_InvalidValueIncludesExample(t *testing.T) {
_, err := NormalizeUTCOffset("bad-offset")
require.Error(t, err)
require.ErrorContains(t, err, "must be a numeric UTC offset")
require.ErrorContains(t, err, "Example:")
}

func TestParseUTCOffsetLocation_MalformedValueIncludesExample(t *testing.T) {
_, err := ParseUTCOffsetLocation("bad-offset")
require.Error(t, err)
require.ErrorContains(t, err, "must be a numeric UTC offset")
require.ErrorContains(t, err, "Example:")
}
30 changes: 24 additions & 6 deletions pkg/workflow/validation_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,24 @@ import (

var validationHelpersLog = logger.New("workflow:validation_helpers")

// nestedYAMLExample renders a dotted field path (e.g. "sandbox.mcp.port") as a
// nested YAML mapping example with the given scalar value on the innermost key
// (e.g. "sandbox:\n mcp:\n port: 1"). Fields without dots are rendered as a
// single "field: value" line.
func nestedYAMLExample(fieldName string, value any) string {
parts := strings.Split(fieldName, ".")
var b strings.Builder
for i, part := range parts {
indent := strings.Repeat(" ", i)
if i == len(parts)-1 {
fmt.Fprintf(&b, "%s%s: %v", indent, part, value)
} else {
fmt.Fprintf(&b, "%s%s:\n", indent, part)
}
}
return b.String()
}

// validateIntRange validates that a value is within the specified inclusive range [min, max].
// It returns an error if the value is outside the range, with a descriptive message
// including the field name and the actual value.
Expand All @@ -57,8 +75,8 @@ var validationHelpersLog = logger.New("workflow:validation_helpers")
// }
func validateIntRange(value, min, max int, fieldName string) error {
if value < min || value > max {
return fmt.Errorf("%s must be between %d and %d, got %d",
fieldName, min, max, value)
return fmt.Errorf("%s must be between %d and %d, got %d. Expected an integer in this inclusive range. Example:\n%s",
fieldName, min, max, value, nestedYAMLExample(fieldName, min))
}
return nil
}
Expand All @@ -72,12 +90,12 @@ func validateMountStringFormat(mount string) (source, dest, mode string, err err
parts := strings.Split(mount, ":")
if len(parts) != 3 {
validationHelpersLog.Printf("Invalid mount format: %q (expected 3 colon-separated parts, got %d)", mount, len(parts))
return "", "", "", errors.New("must follow 'source:destination:mode' format with exactly 3 colon-separated parts")
return "", "", "", errors.New("must follow 'source:destination:mode' format with exactly 3 colon-separated parts. Expected three colon-separated values: source, destination, and mode. Example: /host/path:/container/path:ro")
}
mode = parts[2]
if mode != "ro" && mode != "rw" {
validationHelpersLog.Printf("Invalid mount mode: %q in %q (must be 'ro' or 'rw')", mode, mount)
return parts[0], parts[1], parts[2], fmt.Errorf("mode must be 'ro' or 'rw', got %q", mode)
return parts[0], parts[1], parts[2], fmt.Errorf("mode must be 'ro' or 'rw', got %q. Expected one of: ro, rw. Example: /host/path:/container/path:ro", mode)
}
validationHelpersLog.Printf("Valid mount: source=%s, dest=%s, mode=%s", parts[0], parts[1], mode)
return parts[0], parts[1], parts[2], nil
Expand Down Expand Up @@ -138,7 +156,7 @@ func parseMountEntry(mount string) (mountParts, mountValidationKind) {
// a non-nil error for all non-OK mountValidationKind values.
func validateMountEntries(mounts []string, onValid func(int, mountParts), onInvalid func(int, string, mountParts, mountValidationKind) error) error {
if onInvalid == nil {
return errors.New("internal error: onInvalid callback must not be nil")
return errors.New("internal error: onInvalid callback must not be nil. Expected a callback that returns an error for each invalid mount entry. Example: provide an onInvalid callback that returns an error for non-OK mount kinds")
}

for i, mount := range mounts {
Expand All @@ -151,7 +169,7 @@ func validateMountEntries(mounts []string, onValid func(int, mountParts), onInva
}
err := onInvalid(i, mount, parts, kind)
if err == nil {
return fmt.Errorf("internal error: onInvalid callback returned nil for mount kind %d", kind)
return fmt.Errorf("internal error: onInvalid callback returned nil for mount kind %d. Expected a non-nil error for invalid mount kinds. Example: return an error like \"safe-outputs.mounts[0] has an invalid entry\" when kind is invalid", kind)
}
return err
}
Expand Down
Loading
Loading