From a04a3bbc5f63f01fa3d127b2b76027e63d7204b6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 22:41:37 +0000 Subject: [PATCH] Correct artifact file and empty input handling --- docs/internal/sources.md | 15 +- engine_test.go | 53 ++++ internal/artifact/reader.go | 78 ++++- internal/artifact/reader_fifo_linux_test.go | 43 +++ internal/artifact/reader_test.go | 316 +++++++++++++------- types.go | 16 +- 6 files changed, 392 insertions(+), 129 deletions(-) create mode 100644 internal/artifact/reader_fifo_linux_test.go diff --git a/docs/internal/sources.md b/docs/internal/sources.md index 60812f7..7f7a47b 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -79,10 +79,17 @@ are owned by the ## Ordinary Artifacts -`internal/artifact` resolves inline references and unrestricted, -caller-selected file paths. It copies content into an artifact, records -metadata and a content hash, applies a content-type fallback, and honors -context cancellation. +`internal/artifact` accepts explicitly typed inline references even when their +body is empty. It also resolves unrestricted, caller-selected paths only when +they identify regular operating-system files, checking that condition before +and after opening the file. It copies content into an artifact, records +metadata and an opaque content-equality value, and applies a content-type +fallback. + +Regular files are read synchronously in bounded chunks. Cancellation is +checked before opening, before and after every read, and before publishing the +artifact, so a canceled read never publishes partial content. The ordinary +reader does not detach file reads into background goroutines. This ordinary reader does not implement an inbound HTTP security boundary. In particular, it does not constrain files to an application root or impose an diff --git a/engine_test.go b/engine_test.go index 0618829..6598590 100644 --- a/engine_test.go +++ b/engine_test.go @@ -1049,6 +1049,59 @@ func TestArtifactReaderReceivesPublicReferenceAndPreparesArtifact(t *testing.T) } } +func TestPrepareAcceptsExplicitEmptyInlineInputs(t *testing.T) { + promptSource := fstest.MapFS{ + "prompt.yaml": &fstest.MapFile{Data: []byte(`id: empty-inline +version: "1" +default_profile: profile +inputs: + - name: transcript + required: true +session_id: 'session-{{input "transcript"}}' +messages: + - role: user + content: 'before{{input "transcript"}}after' +output: + format: text + validation_mode: none +`)}, + } + engine, err := promptkit.NewEngine( + promptkit.Config{}, + promptkit.WithPromptFS(promptSource, "."), + promptkit.WithProfiles(promptkit.Profile{ + ID: "profile", Endpoint: "http://example.test/v1", Model: "model", + }), + ) + if err != nil { + t.Fatalf("construct engine: %v", err) + } + + for name, ref := range map[string]promptkit.ArtifactRef{ + "without URI": promptkit.Inline(""), + "with URI": promptkit.InlineWithURI("memory://empty", ""), + } { + t.Run(name, func(t *testing.T) { + prepared, err := engine.Prepare(context.Background(), promptkit.RunRequest{ + PromptID: "empty-inline", + Inputs: map[string]promptkit.ArtifactRef{"transcript": ref}, + }) + if err != nil { + t.Fatalf("prepare empty input: %v", err) + } + if prepared.SessionID != "session-" { + t.Fatalf("session ID = %q, want session-", prepared.SessionID) + } + if len(prepared.Messages) != 1 || prepared.Messages[0].Content != "beforeafter" { + t.Fatalf("messages = %#v, want empty input rendered between markers", prepared.Messages) + } + if prepared.InputHashes["transcript"] == "" { + t.Fatalf("empty input did not retain an equality value: %#v", prepared.InputHashes) + } + }) + } +} + func TestArtifactReaderFailuresPreserveArtifactLoadErrors(t *testing.T) { readerErr := errors.New("artifact reader failed") diff --git a/internal/artifact/reader.go b/internal/artifact/reader.go index e042a0b..2486b9e 100644 --- a/internal/artifact/reader.go +++ b/internal/artifact/reader.go @@ -16,10 +16,12 @@ import ( var ( ErrUnsupportedRefType = errors.New("unsupported artifact reference type") - ErrMissingInlineBody = errors.New("missing body for inline artifact") ErrMissingFilePath = errors.New("missing file path for file artifact") + ErrUnsupportedFile = errors.New("file artifact path is not a regular file") ) +const fileReadChunkSize = 64 * 1024 + // Reader resolves artifact references into actual artifacts. type Reader interface { Read(ctx context.Context, ref domain.ArtifactRef) (*domain.Artifact, error) @@ -34,7 +36,7 @@ type CompositeReader struct { func NewCompositeReader() Reader { return &CompositeReader{ inlineReader: &inlineReader{}, - fileReader: &fileReader{}, + fileReader: &fileReader{open: openArtifactFile}, } } @@ -64,10 +66,6 @@ func (r *inlineReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domai default: } - if ref.Body == "" { - return nil, ErrMissingInlineBody - } - body := []byte(ref.Body) return &domain.Artifact{ ContentType: defaults.ContentTypeTextPlain, @@ -78,7 +76,15 @@ func (r *inlineReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domai }, nil } -type fileReader struct{} +type artifactFile interface { + Read([]byte) (int, error) + Stat() (os.FileInfo, error) + Close() error +} + +type fileReader struct { + open func(string) (artifactFile, error) +} func (r *fileReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain.Artifact, error) { select { @@ -91,25 +97,71 @@ func (r *fileReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain. return nil, ErrMissingFilePath } - return readFileArtifact(ref.URI) + return readFileArtifact(ctx, ref.URI, r.open) } -func readFileArtifact(path string) (*domain.Artifact, error) { - file, err := os.Open(path) +func openArtifactFile(path string) (artifactFile, error) { + return os.Open(path) +} + +func readFileArtifact(ctx context.Context, path string, open func(string) (artifactFile, error)) (*domain.Artifact, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + info, err := os.Stat(path) + if err != nil { + return nil, fmt.Errorf("failed to read file %s: %w", path, err) + } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("%w: %s", ErrUnsupportedFile, path) + } + if err := ctx.Err(); err != nil { + return nil, err + } + + file, err := open(path) if err != nil { return nil, fmt.Errorf("failed to read file %s: %w", path, err) } defer file.Close() - data, err := io.ReadAll(file) + openedInfo, err := file.Stat() if err != nil { - return nil, fmt.Errorf("failed to read file %s: %w", path, err) + return nil, fmt.Errorf("failed to inspect opened file %s: %w", path, err) + } + if !openedInfo.Mode().IsRegular() { + return nil, fmt.Errorf("%w: %s", ErrUnsupportedFile, path) + } + + data := make([]byte, 0) + chunk := make([]byte, fileReadChunkSize) + for { + if err := ctx.Err(); err != nil { + return nil, err + } + n, readErr := file.Read(chunk) + if n > 0 { + data = append(data, chunk[:n]...) + } + if err := ctx.Err(); err != nil { + return nil, err + } + if errors.Is(readErr, io.EOF) { + break + } + if readErr != nil { + return nil, fmt.Errorf("failed to read file %s: %w", path, readErr) + } } contentType := mime.TypeByExtension(filepath.Ext(path)) if contentType == "" { contentType = defaults.ContentTypeTextPlain } + hash := fmt.Sprintf("%x", sha256.Sum256(data)) + if err := ctx.Err(); err != nil { + return nil, err + } return &domain.Artifact{ Name: filepath.Base(path), @@ -117,6 +169,6 @@ func readFileArtifact(path string) (*domain.Artifact, error) { Body: data, URI: path, Size: int64(len(data)), - Hash: fmt.Sprintf("%x", sha256.Sum256(data)), + Hash: hash, }, nil } diff --git a/internal/artifact/reader_fifo_linux_test.go b/internal/artifact/reader_fifo_linux_test.go new file mode 100644 index 0000000..87cde66 --- /dev/null +++ b/internal/artifact/reader_fifo_linux_test.go @@ -0,0 +1,43 @@ +//go:build linux + +package artifact + +import ( + "context" + "errors" + "path/filepath" + "syscall" + "testing" + "time" + + "gitea.maximumdirect.net/eric/promptkit/internal/domain" +) + +func TestFileReaderRejectsFIFOBeforeOpen(t *testing.T) { + path := filepath.Join(t.TempDir(), "artifact.fifo") + if err := syscall.Mkfifo(path, 0o600); err != nil { + t.Fatalf("create fifo: %v", err) + } + + type result struct { + artifact *domain.Artifact + err error + } + done := make(chan result, 1) + go func() { + artifact, err := NewCompositeReader().Read(context.Background(), domain.ArtifactRef{ + Type: domain.ArtifactRefFile, + URI: path, + }) + done <- result{artifact: artifact, err: err} + }() + + select { + case got := <-done: + if got.artifact != nil || !errors.Is(got.err, ErrUnsupportedFile) { + t.Fatalf("artifact=%#v err=%v, want nil/ErrUnsupportedFile", got.artifact, got.err) + } + case <-time.After(time.Second): + t.Fatal("FIFO read blocked instead of rejecting the non-regular file") + } +} diff --git a/internal/artifact/reader_test.go b/internal/artifact/reader_test.go index 901e2d7..f47ba28 100644 --- a/internal/artifact/reader_test.go +++ b/internal/artifact/reader_test.go @@ -1,6 +1,7 @@ package artifact import ( + "bytes" "context" "errors" "os" @@ -11,54 +12,97 @@ import ( "gitea.maximumdirect.net/eric/promptkit/internal/domain" ) -func TestCompositeReader_Read(t *testing.T) { +func TestCompositeReaderRejectsUnsupportedReferences(t *testing.T) { + _, err := NewCompositeReader().Read(context.Background(), domain.ArtifactRef{ + Type: domain.ArtifactRefType("unsupported"), + URI: "unsupported://bucket/key", + }) + if !errors.Is(err, ErrUnsupportedRefType) { + t.Fatalf("expected ErrUnsupportedRefType, got %v", err) + } +} + +func TestCompositeReaderSourceParityAndOpaqueHashes(t *testing.T) { reader := NewCompositeReader() - ctx := context.Background() + hashes := make(map[string]string) + tests := []struct { + name string + content string + }{ + {name: "empty", content: ""}, + {name: "ordinary", content: "same content"}, + {name: "changed", content: "changed content"}, + } - t.Run("inline artifact", func(t *testing.T) { - ref := domain.ArtifactRef{ - Type: domain.ArtifactRefInline, - Body: "hello world", - } - art, err := reader.Read(ctx, ref) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if string(art.Body) != "hello world" { - t.Errorf("expected 'hello world', got %s", string(art.Body)) - } - if art.ContentType != "text/plain" { - t.Errorf("expected text/plain content type, got %q", art.ContentType) - } - if art.Hash != "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" { - t.Errorf("unexpected hash: %s", art.Hash) - } - if art.Size != int64(len(ref.Body)) { - t.Errorf("expected size %d, got %d", len(ref.Body), art.Size) - } - }) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "artifact.txt") + if err := os.WriteFile(filePath, []byte(tc.content), 0o600); err != nil { + t.Fatal(err) + } - t.Run("inline artifact missing body", func(t *testing.T) { - ref := domain.ArtifactRef{ - Type: domain.ArtifactRefInline, - Body: "", - } - _, err := reader.Read(ctx, ref) - if !errors.Is(err, ErrMissingInlineBody) { - t.Errorf("expected ErrMissingInlineBody, got %v", err) - } - }) + sources := []struct { + name string + ref domain.ArtifactRef + wantURI string + }{ + { + name: "inline", + ref: domain.ArtifactRef{Type: domain.ArtifactRefInline, Body: tc.content}, + }, + { + name: "inline with uri", + ref: domain.ArtifactRef{Type: domain.ArtifactRefInline, URI: "memory://input", Body: tc.content}, + wantURI: "memory://input", + }, + { + name: "file", + ref: domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filePath}, + wantURI: filePath, + }, + } - t.Run("unsupported ref type", func(t *testing.T) { - ref := domain.ArtifactRef{ - Type: domain.ArtifactRefType("unsupported"), - URI: "unsupported://bucket/key", - } - _, err := reader.Read(ctx, ref) - if !errors.Is(err, ErrUnsupportedRefType) { - t.Error("expected error for unsupported type") - } - }) + var sourceHash string + for _, source := range sources { + t.Run(source.name, func(t *testing.T) { + first, err := reader.Read(context.Background(), source.ref) + if err != nil { + t.Fatalf("first read: %v", err) + } + second, err := reader.Read(context.Background(), source.ref) + if err != nil { + t.Fatalf("second read: %v", err) + } + if string(first.Body) != tc.content || first.Size != int64(len(tc.content)) { + t.Fatalf("body=%q size=%d, want %q/%d", first.Body, first.Size, tc.content, len(tc.content)) + } + if first.URI != source.wantURI { + t.Fatalf("URI = %q, want %q", first.URI, source.wantURI) + } + if first.Hash == "" || first.Hash != second.Hash { + t.Fatalf("hashes are not non-empty and stable: %q/%q", first.Hash, second.Hash) + } + if sourceHash == "" { + sourceHash = first.Hash + } else if first.Hash != sourceHash { + t.Fatalf("equal content hashes differ: %q/%q", sourceHash, first.Hash) + } + if source.ref.Type == domain.ArtifactRefFile { + if first.Name != filepath.Base(filePath) || !strings.HasPrefix(first.ContentType, "text/plain") { + t.Fatalf("unexpected file metadata: %+v", first) + } + } else if first.ContentType != "text/plain" { + t.Fatalf("inline content type = %q", first.ContentType) + } + }) + } + hashes[tc.name] = sourceHash + }) + } + + if hashes["empty"] == hashes["ordinary"] || hashes["ordinary"] == hashes["changed"] { + t.Fatalf("changed content did not change opaque hash: %#v", hashes) + } } func TestCompositeReaderCopiesInlineData(t *testing.T) { @@ -87,94 +131,154 @@ func TestCompositeReaderCopiesInlineData(t *testing.T) { } } -func TestCompositeReaderHonorsCancellation(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() +func TestCompositeReaderHonorsPreCancellation(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "artifact.txt") + if err := os.WriteFile(filePath, []byte("ignored"), 0o600); err != nil { + t.Fatal(err) + } + tests := []struct { + name string + ref domain.ArtifactRef + }{ + {name: "inline", ref: domain.ArtifactRef{Type: domain.ArtifactRefInline, Body: "ignored"}}, + {name: "file", ref: domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filePath}}, + } - _, err := NewCompositeReader().Read(ctx, domain.ArtifactRef{ - Type: domain.ArtifactRefInline, - Body: "ignored", - }) - if !errors.Is(err, context.Canceled) { - t.Fatalf("expected context cancellation, got %v", err) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + artifact, err := NewCompositeReader().Read(ctx, tc.ref) + if artifact != nil || !errors.Is(err, context.Canceled) { + t.Fatalf("artifact=%#v err=%v, want nil/context.Canceled", artifact, err) + } + }) } } -func TestFileReader_Read(t *testing.T) { - content := []byte("test file content") - filePath := filepath.Join(t.TempDir(), "artifact.txt") - if err := os.WriteFile(filePath, content, 0o600); err != nil { - t.Fatal(err) - } - +func TestFileReaderFailuresAndMetadata(t *testing.T) { reader := NewCompositeReader() - ctx := context.Background() - - t.Run("file artifact loading", func(t *testing.T) { - ref := domain.ArtifactRef{ - Type: domain.ArtifactRefFile, - URI: filePath, - } - art, err := reader.Read(ctx, ref) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if string(art.Body) != string(content) { - t.Errorf("expected %s, got %s", string(content), string(art.Body)) - } - if art.Name != filepath.Base(filePath) { - t.Errorf("expected name %q, got %q", filepath.Base(filePath), art.Name) - } - if !strings.HasPrefix(art.ContentType, "text/plain") { - t.Errorf("expected text content type, got %q", art.ContentType) - } - if art.URI != filePath { - t.Errorf("expected URI %q, got %q", filePath, art.URI) - } - if art.Size != int64(len(content)) { - t.Errorf("expected size %d, got %d", len(content), art.Size) - } - if art.Hash != "60f5237ed4049f0382661ef009d2bc42e48c3ceb3edb6600f7024e7ab3b838f3" { - t.Errorf("unexpected hash: %s", art.Hash) - } - }) t.Run("missing file path", func(t *testing.T) { - ref := domain.ArtifactRef{ - Type: domain.ArtifactRefFile, - URI: "", - } - _, err := reader.Read(ctx, ref) + _, err := reader.Read(context.Background(), domain.ArtifactRef{Type: domain.ArtifactRefFile}) if !errors.Is(err, ErrMissingFilePath) { - t.Errorf("expected ErrMissingFilePath, got %v", err) + t.Fatalf("expected ErrMissingFilePath, got %v", err) } }) t.Run("missing file", func(t *testing.T) { - ref := domain.ArtifactRef{ + _, err := reader.Read(context.Background(), domain.ArtifactRef{ Type: domain.ArtifactRefFile, URI: filepath.Join(t.TempDir(), "missing.txt"), - } - if _, err := reader.Read(ctx, ref); err == nil { + }) + if err == nil { t.Fatal("expected missing file error") } }) + t.Run("directory rejected before open", func(t *testing.T) { + artifact, err := reader.Read(context.Background(), domain.ArtifactRef{ + Type: domain.ArtifactRefFile, + URI: t.TempDir(), + }) + if artifact != nil || !errors.Is(err, ErrUnsupportedFile) { + t.Fatalf("artifact=%#v err=%v, want nil/ErrUnsupportedFile", artifact, err) + } + }) + + t.Run("non-regular opened target rejected", func(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "artifact.txt") + if err := os.WriteFile(filePath, []byte("content"), 0o600); err != nil { + t.Fatal(err) + } + directoryInfo, err := os.Stat(t.TempDir()) + if err != nil { + t.Fatal(err) + } + fileReader := &fileReader{open: func(path string) (artifactFile, error) { + file, err := os.Open(path) + if err != nil { + return nil, err + } + return &reportedInfoFile{artifactFile: file, info: directoryInfo}, nil + }} + + artifact, err := fileReader.Read(context.Background(), domain.ArtifactRef{ + Type: domain.ArtifactRefFile, + URI: filePath, + }) + if artifact != nil || !errors.Is(err, ErrUnsupportedFile) { + t.Fatalf("artifact=%#v err=%v, want nil/ErrUnsupportedFile", artifact, err) + } + }) + t.Run("unknown extension uses text fallback", func(t *testing.T) { - path := filepath.Join(t.TempDir(), "artifact.unknownextension") - if err := os.WriteFile(path, content, 0o600); err != nil { + filePath := filepath.Join(t.TempDir(), "artifact.unknownextension") + if err := os.WriteFile(filePath, []byte("content"), 0o600); err != nil { t.Fatal(err) } - art, err := reader.Read(ctx, domain.ArtifactRef{ + artifact, err := reader.Read(context.Background(), domain.ArtifactRef{ Type: domain.ArtifactRefFile, - URI: path, + URI: filePath, }) if err != nil { - t.Fatalf("unexpected error: %v", err) + t.Fatalf("read artifact: %v", err) } - if art.ContentType != "text/plain" { - t.Errorf("expected text/plain fallback, got %q", art.ContentType) + if artifact.ContentType != "text/plain" { + t.Fatalf("content type = %q", artifact.ContentType) } }) } + +func TestFileReaderCancelsAfterReadProgress(t *testing.T) { + filePath := filepath.Join(t.TempDir(), "artifact.bin") + content := bytes.Repeat([]byte("x"), fileReadChunkSize*2) + if err := os.WriteFile(filePath, content, 0o600); err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithCancel(context.Background()) + var opened *cancelAfterProgressFile + reader := &fileReader{open: func(path string) (artifactFile, error) { + file, err := os.Open(path) + if err != nil { + return nil, err + } + opened = &cancelAfterProgressFile{artifactFile: file, cancel: cancel} + return opened, nil + }} + + artifact, err := reader.Read(ctx, domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filePath}) + if artifact != nil || !errors.Is(err, context.Canceled) { + t.Fatalf("artifact=%#v err=%v, want nil/context.Canceled", artifact, err) + } + if opened == nil || opened.reads != 1 { + t.Fatalf("read count = %v, want one progressing read", opened) + } +} + +type reportedInfoFile struct { + artifactFile + info os.FileInfo +} + +func (f *reportedInfoFile) Stat() (os.FileInfo, error) { + return f.info, nil +} + +type cancelAfterProgressFile struct { + artifactFile + cancel context.CancelFunc + reads int +} + +func (f *cancelAfterProgressFile) Read(buffer []byte) (int, error) { + n, err := f.artifactFile.Read(buffer) + if n > 0 { + f.reads++ + f.cancel() + } + return n, err +} diff --git a/types.go b/types.go index c9459d7..f00722b 100644 --- a/types.go +++ b/types.go @@ -243,8 +243,8 @@ type ArtifactRef struct { // URI is the file path for ArtifactRefFile and optional provenance metadata // for ArtifactRefInline. URI string - // Body is the content for ArtifactRefInline and is ignored for - // ArtifactRefFile. + // Body is the content for ArtifactRefInline, where an empty value is valid, + // and is ignored for ArtifactRefFile. Body string } @@ -700,8 +700,12 @@ type GenerateResponse struct { // File returns a file-backed artifact reference whose URI is path. // -// The default artifact reader opens path as a caller-selected operating-system -// path without restricting it to an application root or imposing a size limit. +// The default artifact reader accepts path only when it resolves to a regular +// operating-system file, checking that condition before and after opening it. +// It reads synchronously in bounded chunks and checks context cancellation +// before opening, before and after each read, and before returning the +// artifact; it cannot interrupt a filesystem operation already in progress. +// It does not restrict path to an application root or impose a size limit. // Applications accepting untrusted paths must validate them before calling // Promptkit or use [WithArtifactReader] to enforce application policy. func File(path string) ArtifactRef { @@ -709,13 +713,13 @@ func File(path string) ArtifactRef { } // Inline returns an inline artifact reference whose Body is body and whose URI -// is empty. +// is empty. An empty body is a valid, explicitly supplied input. func Inline(body string) ArtifactRef { return ArtifactRef{Type: ArtifactRefInline, Body: body} } // InlineWithURI returns an inline artifact reference with body content and uri -// provenance metadata. +// provenance metadata. An empty body is a valid, explicitly supplied input. func InlineWithURI(uri string, body string) ArtifactRef { return ArtifactRef{Type: ArtifactRefInline, URI: uri, Body: body} }