diff --git a/pkg/workflow/mcp_mount_validation.go b/pkg/workflow/mcp_mount_validation.go index fe7541446e6..6d873b1534a 100644 --- a/pkg/workflow/mcp_mount_validation.go +++ b/pkg/workflow/mcp_mount_validation.go @@ -37,11 +37,10 @@ func validateMCPMountsSyntax(toolName string, mountsRaw any) error { } mcpMountValidationLog.Printf("Validating %d mount(s) for tool %q", len(mounts), toolName) - for i, mount := range mounts { - parts, kind := parseMountEntry(mount) + return validateMountEntries(mounts, func(i int, parts mountParts) { + mcpMountValidationLog.Printf("Mount[%d] valid for tool %q: source=%s, dest=%s, mode=%s", i, toolName, parts.source, parts.dest, parts.mode) + }, func(i int, mount string, parts mountParts, kind mountValidationKind) error { switch kind { - case mountValidationOK: - mcpMountValidationLog.Printf("Mount[%d] valid for tool %q: source=%s, dest=%s, mode=%s", i, toolName, parts.source, parts.dest, parts.mode) case mountValidationFormatError: mcpMountValidationLog.Printf("Mount[%d] format error for tool %q: %q", i, toolName, mount) return fmt.Errorf("tool '%s' mcp configuration mounts[%d] must follow 'source:destination:mode' format, got: %q.\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", toolName, i, mount, toolName, constants.DocsToolsURL) @@ -57,7 +56,5 @@ func validateMCPMountsSyntax(toolName string, mountsRaw any) error { default: return fmt.Errorf("internal error: unsupported mount validation kind %d for tool %q mount %q", kind, toolName, mount) } - } - - return nil + }) } diff --git a/pkg/workflow/sandbox_validation.go b/pkg/workflow/sandbox_validation.go index 0b76fbf693b..d8c876ceeb9 100644 --- a/pkg/workflow/sandbox_validation.go +++ b/pkg/workflow/sandbox_validation.go @@ -28,11 +28,10 @@ var githubActionsExpressionPattern = regexp.MustCompile(`\$\{\{[\s\S]*\}\}`) // validateMountsSyntax validates that mount strings follow the correct syntax // Expected format: "source:destination:mode" where mode is either "ro" or "rw" func validateMountsSyntax(mounts []string) error { - for i, mount := range mounts { - parts, kind := parseMountEntry(mount) + return validateMountEntries(mounts, func(i int, parts mountParts) { + sandboxValidationLog.Printf("Validated mount %d: source=%s, dest=%s, mode=%s", i, parts.source, parts.dest, parts.mode) + }, func(i int, mount string, parts mountParts, kind mountValidationKind) error { switch kind { - case mountValidationOK: - sandboxValidationLog.Printf("Validated mount %d: source=%s, dest=%s, mode=%s", i, parts.source, parts.dest, parts.mode) case mountValidationFormatError: return NewValidationError( fmt.Sprintf("sandbox.mounts[%d]", i), @@ -64,9 +63,7 @@ func validateMountsSyntax(mounts []string) error { default: return fmt.Errorf("internal error: unsupported mount validation kind %d for sandbox mount %q", kind, mount) } - } - - return nil + }) } // validateSandboxConfig validates the sandbox configuration diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 6cef6a994cf..82456553c66 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -145,6 +145,33 @@ func parseMountEntry(mount string) (mountParts, mountValidationKind) { return mountParts{source: source, dest: dest, mode: mode}, mountValidationOK } +// validateMountEntries applies shared mount parsing and classification across +// callers while allowing each caller to preserve its own logging and error +// construction. onValid may be nil. onInvalid must be non-nil and must return +// 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") + } + + for i, mount := range mounts { + parts, kind := parseMountEntry(mount) + if kind == mountValidationOK { + if onValid != nil { + onValid(i, parts) + } + continue + } + err := onInvalid(i, mount, parts, kind) + if err == nil { + return fmt.Errorf("internal error: onInvalid callback returned nil for mount kind %d", kind) + } + return err + } + + return nil +} + // validateStringEnumField checks that a config field, if present, contains one // of the allowed string values. Non-string values and unrecognised strings are // removed from the map (treated as absent) and a warning is logged. Use this diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index 093a3ae0b5b..10c8f923e2d 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -3,6 +3,7 @@ package workflow import ( + "fmt" "strings" "testing" @@ -589,6 +590,116 @@ func TestParseMountEntry(t *testing.T) { } } +func TestValidateMountEntries(t *testing.T) { + t.Run("returns nil and calls onValid for each mount when all are valid", func(t *testing.T) { + var validated []mountParts + err := validateMountEntries( + []string{ + "/host/a:/a:ro", + "/host/b:/b:rw", + }, + func(_ int, parts mountParts) { + validated = append(validated, parts) + }, + func(i int, mount string, parts mountParts, kind mountValidationKind) error { + return fmt.Errorf("unexpected invalid mount at %d: %s (%v) %#v", i, mount, kind, parts) + }, + ) + + require.NoError(t, err) + assert.Equal(t, []mountParts{ + {source: "/host/a", dest: "/a", mode: "ro"}, + {source: "/host/b", dest: "/b", mode: "rw"}, + }, validated) + }) + + t.Run("returns nil for empty mounts", func(t *testing.T) { + err := validateMountEntries( + nil, + func(_ int, _ mountParts) { t.Fatal("onValid should not be called") }, + func(i int, mount string, parts mountParts, kind mountValidationKind) error { + return fmt.Errorf("onInvalid should not be called at %d: %s (%v) %#v", i, mount, kind, parts) + }, + ) + + require.NoError(t, err) + }) + + t.Run("reports index 0 when first mount is invalid", func(t *testing.T) { + err := validateMountEntries( + []string{"/host/data:/data:nope"}, + nil, + func(i int, mount string, parts mountParts, kind mountValidationKind) error { + assert.Equal(t, 0, i) + assert.Equal(t, "/host/data:/data:nope", mount) + assert.Equal(t, mountValidationModeError, kind) + assert.Equal(t, mountParts{source: "/host/data", dest: "/data", mode: "nope"}, parts) + return fmt.Errorf("invalid at %d", i) + }, + ) + + require.EqualError(t, err, "invalid at 0") + }) + + t.Run("returns first invalid mount after valid prefix", func(t *testing.T) { + var validated []mountParts + err := validateMountEntries( + []string{ + "/host/data:/data:ro", + "/host/data:/data:nope", + "/host/other:/other:rw", + }, + func(_ int, parts mountParts) { + validated = append(validated, parts) + }, + func(i int, mount string, parts mountParts, kind mountValidationKind) error { + assert.Equal(t, 1, i) + assert.Equal(t, "/host/data:/data:nope", mount) + assert.Equal(t, mountValidationModeError, kind) + assert.Equal(t, mountParts{source: "/host/data", dest: "/data", mode: "nope"}, parts) + return fmt.Errorf("stop at %d", i) + }, + ) + + require.EqualError(t, err, "stop at 1") + assert.Equal(t, []mountParts{{source: "/host/data", dest: "/data", mode: "ro"}}, validated) + }) + + t.Run("accepts nil onValid callback", func(t *testing.T) { + err := validateMountEntries( + []string{"/host/data:/data:ro"}, + nil, + func(i int, mount string, parts mountParts, kind mountValidationKind) error { + return fmt.Errorf("unexpected invalid mount at %d: %s (%v)", i, mount, kind) + }, + ) + + require.NoError(t, err) + }) + + t.Run("returns internal error for nil onInvalid callback", func(t *testing.T) { + err := validateMountEntries( + []string{"/host/data:/data:nope"}, + nil, + nil, + ) + + require.EqualError(t, err, "internal error: onInvalid callback must not be nil") + }) + + t.Run("returns internal error when onInvalid returns nil", func(t *testing.T) { + err := validateMountEntries( + []string{"/host/data:/data:nope"}, + nil, + func(_ int, _ string, _ mountParts, _ mountValidationKind) error { + return nil + }, + ) + + require.EqualError(t, err, "internal error: onInvalid callback returned nil for mount kind 2") + }) +} + // TestPreprocessProtectedFilesField tests the preprocessProtectedFilesField helper. func TestPreprocessProtectedFilesField(t *testing.T) { tests := []struct {