Skip to content
Open
22 changes: 13 additions & 9 deletions internal/apiform/encoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,19 @@ func (e *encoder) encodeValue(key string, val reflect.Value, writer *multipart.W
return writer.WriteField(key, "")
}

// Unwrap interfaces before detecting io.Reader. A non-nil interface can
// contain a typed nil pointer, which must retain the encoder's empty-field
// semantics instead of being passed to io.Copy.
for val.Kind() == reflect.Interface {
if val.IsNil() {
return writer.WriteField(key, "")
}
val = val.Elem()
}
if val.Kind() == reflect.Pointer && val.IsNil() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Apply the same typed-nil semantics before choosing multipart transport. This branch correctly serializes a typed-nil reader as an empty scalar field, but pkg/cmd/multipartbody.go:inspectMultipartBody still sees the non-nil io.Reader interface and marks it as hasUpload. As a result, multipartRequestOptions unnecessarily selects one-shot streaming, applies option.WithMaxRetries(0), and rejects otherwise replayable 307/308 redirects. I verified the mismatch end to end with a synthetic HTTP 429 response: the empty-field request made one attempt and failed instead of retrying. Please update the upload classifier to recognize typed-nil readers as scalars and add a request-level retry regression.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The transport classifier now applies the same typed-nil reader semantics as the encoder, so typed-nil readers stay on the buffered scalar path instead of being treated as uploads. I also updated the known-length framing path so a typed-nil optional reader can coexist with a real file upload.

Added request-level regressions covering the reported 429 retry case, replay across both 307 and 308 redirects, buffered Content-Length, the empty scalar-field representation, and a known-length file upload alongside a typed-nil reader.

Fresh CI, CodeQL and Castiron runs are currently action_required pending maintainer approval.

return writer.WriteField(key, "")
}

t := val.Type()

if t.Implements(reflect.TypeOf((*io.Reader)(nil)).Elem()) {
Expand All @@ -57,9 +70,6 @@ func (e *encoder) encodeValue(key string, val reflect.Value, writer *multipart.W

switch t.Kind() {
case reflect.Pointer:
if val.IsNil() || !val.IsValid() {
return writer.WriteField(key, "")
}
return e.encodeValue(key, val.Elem(), writer)

case reflect.Slice, reflect.Array:
Expand All @@ -68,12 +78,6 @@ func (e *encoder) encodeValue(key string, val reflect.Value, writer *multipart.W
case reflect.Map:
return e.encodeMap(key, val, writer)

case reflect.Interface:
if val.IsNil() {
return writer.WriteField(key, "")
}
return e.encodeValue(key, val.Elem(), writer)

case reflect.String:
return writer.WriteField(key, val.String())

Expand Down
48 changes: 48 additions & 0 deletions internal/apiform/typed_nil_reader_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package apiform

import (
"bytes"
"io"
"mime/multipart"
"testing"

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

type panicOnRead struct{}

func (*panicOnRead) Read([]byte) (int, error) {
panic("Read called on typed nil receiver")
}

func TestMarshalTreatsTypedNilReaderAsEmptyField(t *testing.T) {
t.Parallel()

var concrete *panicOnRead
var reader io.Reader = concrete
tests := map[string]any{
"concrete pointer in any map": map[string]any{"file": concrete},
"pointer in reader map": map[string]io.Reader{"file": reader},
"nil reader interface": map[string]io.Reader{"file": nil},
}

for name, value := range tests {
t.Run(name, func(t *testing.T) {
t.Parallel()

var buf bytes.Buffer
writer := multipart.NewWriter(&buf)
require.NoError(t, writer.SetBoundary("xxx"))

require.NotPanics(t, func() {
require.NoError(t, Marshal(value, writer))
require.NoError(t, writer.Close())
})

require.Equal(t,
"--xxx\r\nContent-Disposition: form-data; name=\"file\"\r\n\r\n\r\n--xxx--\r\n",
buf.String(),
)
})
}
}
14 changes: 14 additions & 0 deletions pkg/cmd/multipartbody.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,14 @@ type multipartBodyInfo struct {
knownLength bool
}

func isTypedNilReader(value any) bool {
if _, ok := value.(io.Reader); !ok {
return false
}
reflected := reflect.ValueOf(value)
return reflected.Kind() == reflect.Pointer && reflected.IsNil()
}

func inspectMultipartBody(value any) multipartBodyInfo {
switch value := value.(type) {
case map[string]any:
Expand All @@ -240,6 +248,9 @@ func inspectMultipartBody(value any) multipartBodyInfo {
case fileUpload:
return multipartBodyInfo{hasUpload: true, knownLength: value.hasKnownSize()}
default:
if isTypedNilReader(value) {
return multipartBodyInfo{knownLength: true}
}
_, isReader := value.(io.Reader)
return multipartBodyInfo{hasUpload: isReader, knownLength: !isReader}
}
Expand Down Expand Up @@ -311,6 +322,9 @@ func transformFileUploads(
}
return result, nil
default:
if isTypedNilReader(value) {
return value, nil
}
if _, isReader := value.(io.Reader); isReader {
return nil, errors.New("multipart body contains an unknown-size reader")
}
Expand Down
146 changes: 146 additions & 0 deletions pkg/cmd/multipartbody_typed_nil_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
package cmd

import (
"context"
"io"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"strings"
"sync/atomic"
"testing"

"github.com/openai/openai-cli/internal/apiform"
"github.com/openai/openai-go/v3"
"github.com/openai/openai-go/v3/option"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func typedNilMultipartReader() io.Reader {
var reader *strings.Reader
return reader
}

func TestInspectMultipartBodyTreatsTypedNilReaderAsScalar(t *testing.T) {
info := inspectMultipartBody(map[string]any{"file": typedNilMultipartReader()})

require.False(t, info.hasUpload)
require.True(t, info.knownLength)
}

func TestMultipartRequestOptionsRetryTypedNilReaderAsScalar(t *testing.T) {
var requestCount atomic.Int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requestCount.Add(1)
if !assert.NoError(t, r.ParseMultipartForm(1<<20)) {
http.Error(w, "invalid multipart form", http.StatusBadRequest)
return
}
assert.Equal(t, []string{""}, r.MultipartForm.Value["file"])
assert.Empty(t, r.MultipartForm.File["file"])
assert.Positive(t, r.ContentLength)

w.Header().Set("Content-Type", "application/json")
if requestCount.Load() == 1 {
w.Header().Set("Retry-After", "0")
w.WriteHeader(http.StatusTooManyRequests)
_, _ = io.WriteString(w, `{"error":{"message":"retry me","type":"rate_limit_error"}}`)
return
}
_, _ = io.WriteString(w, `{"id":"video_123","object":"video","status":"queued"}`)
}))
t.Cleanup(server.Close)

options, err := multipartRequestOptions(map[string]any{
"file": typedNilMultipartReader(),
"prompt": "hello",
}, apiform.FormatBrackets)
require.NoError(t, err)
client := openai.NewClient(
option.WithAPIKey("test-key"),
option.WithBaseURL(server.URL+"/"),
)

_, err = client.Videos.New(context.Background(), openai.VideoNewParams{}, options...)
require.NoError(t, err)
require.Equal(t, int32(2), requestCount.Load())
}

func TestMultipartRequestOptionsReplayTypedNilReaderAcrossRedirects(t *testing.T) {
for _, status := range []int{http.StatusTemporaryRedirect, http.StatusPermanentRedirect} {
t.Run(http.StatusText(status), func(t *testing.T) {
var requestCount atomic.Int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requestCount.Add(1)
if r.URL.Path != "/redirected" {
http.Redirect(w, r, "/redirected", status)
return
}

if !assert.NoError(t, r.ParseMultipartForm(1<<20)) {
http.Error(w, "invalid multipart form", http.StatusBadRequest)
return
}
assert.Equal(t, []string{""}, r.MultipartForm.Value["file"])
assert.Empty(t, r.MultipartForm.File["file"])
assert.Positive(t, r.ContentLength)
w.Header().Set("Content-Type", "application/json")
_, _ = io.WriteString(w, `{"id":"video_123","object":"video","status":"queued"}`)
}))
t.Cleanup(server.Close)

options, err := multipartRequestOptions(map[string]any{
"file": typedNilMultipartReader(),
"prompt": "hello",
}, apiform.FormatBrackets)
require.NoError(t, err)
client := openai.NewClient(
option.WithAPIKey("test-key"),
option.WithBaseURL(server.URL+"/"),
)

_, err = client.Videos.New(context.Background(), openai.VideoNewParams{}, options...)
require.NoError(t, err)
require.Equal(t, int32(2), requestCount.Load())
})
}
}

func TestMultipartRequestOptionsKnownUploadAllowsTypedNilReaderField(t *testing.T) {
path := filepath.Join(t.TempDir(), "payload.txt")
require.NoError(t, os.WriteFile(path, []byte("payload"), 0o600))
upload, err := openFileUpload(path)
require.NoError(t, err)

var requestCount atomic.Int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requestCount.Add(1)
assert.Positive(t, r.ContentLength)
if !assert.NoError(t, r.ParseMultipartForm(1<<20)) {
http.Error(w, "invalid multipart form", http.StatusBadRequest)
return
}
assert.Equal(t, []string{""}, r.MultipartForm.Value["optional"])
assert.Len(t, r.MultipartForm.File["file"], 1)
w.Header().Set("Content-Type", "application/json")
_, _ = io.WriteString(w, `{"id":"file_123","object":"file","bytes":7,"filename":"payload.txt","purpose":"assistants"}`)
}))
t.Cleanup(server.Close)

options, err := multipartRequestOptions(map[string]any{
"file": upload,
"optional": typedNilMultipartReader(),
"purpose": "assistants",
}, apiform.FormatBrackets)
require.NoError(t, err)
client := openai.NewClient(
option.WithAPIKey("test-key"),
option.WithBaseURL(server.URL+"/"),
)

_, err = client.Files.New(context.Background(), openai.FileNewParams{}, options...)
require.NoError(t, err)
require.Equal(t, int32(1), requestCount.Load())
}