feat: move serialization out of the payload package (#32)

Previously, the user message string was built in the payload package. This created a tight coupling between the calling code and the specific prompt format expected by the LLM.

With this change, the responsibility of serializing the request data into a user message is moved to the individual LLM provider implementations. This is achieved by introducing new request structs in the  package, which are then passed to the providers.

This refactoring improves the separation of concerns, making the code more modular and easier to maintain and extend.

Author: dangazineu

cmd/template/template.go

@@ -199,7 +199,7 @@ func execute(cmd *cobra.Command, args []string, def *Definition) error {
 		}
 	}
 
-	userMsg, err := buildExtendedUserMessage(rootFS, meta, ec, files)
+	userRequest, err := buildWorkspaceChangeRequest(rootFS, meta, ec, files)
 	if err != nil {
 		return err
 	}
@@ -217,7 +217,7 @@ func execute(cmd *cobra.Command, args []string, def *Definition) error {
 
 	systemMessage := rendered
 
-	proposal, err := llm.GetWorkspaceChangeProposals(cfg, def.Model.Family, def.Model.Size, systemMessage, userMsg)
+	proposal, err := llm.GetWorkspaceChangeProposals(cfg, def.Model.Family, def.Model.Size, systemMessage, userRequest)
 	if err != nil {
 		return err
 	}

cmd/template/user_msg_builder.go

@@ -11,19 +11,21 @@ import (
 	"github.com/vybdev/vyb/workspace/project"
 )
 
-// buildExtendedUserMessage composes the user-message payload that will be
+// buildWorkspaceChangeRequest composes a payload.WorkspaceChangeRequest that will be
 // sent to the LLM. It prepends module context information — as dictated
-// by the specification — before the raw file contents. When metadata is
-// nil or when any contextual information is missing the function falls
-// back gracefully, emitting only what is available.
-func buildExtendedUserMessage(rootFS fs.FS, meta *project.Metadata, ec *context.ExecutionContext, filePaths []string) (string, error) {
-	// If metadata is missing we revert to the original behaviour – emit
-	// just the files.
-	if meta == nil || meta.Modules == nil {
-		return payload.BuildUserMessage(rootFS, filePaths)
+// by the specification — before the raw file contents. Both meta and
+// meta.Modules must be non-nil.
+func buildWorkspaceChangeRequest(rootFS fs.FS, meta *project.Metadata, ec *context.ExecutionContext, filePaths []string) (*payload.WorkspaceChangeRequest, error) {
+	if meta == nil {
+		return nil, fmt.Errorf("metadata cannot be nil")
 	}
+	if meta.Modules == nil {
+		return nil, fmt.Errorf("metadata.Modules cannot be nil")
+	}
+
+	request := &payload.WorkspaceChangeRequest{}
 
-	// Helper to clean/normalise relative paths.
+	// Helper to clean/normalise relative paths
 	rel := func(abs string) string {
 		if abs == "" {
 			return ""
@@ -35,42 +37,43 @@ func buildExtendedUserMessage(rootFS fs.FS, meta *project.Metadata, ec *context.
 	workingRel := rel(ec.WorkingDir)
 	targetRel := rel(ec.TargetDir)
 
+	request.TargetDirectory = targetRel
+
+	// Find modules (metadata is guaranteed to be valid)
 	workingMod := project.FindModule(meta.Modules, workingRel)
 	targetMod := project.FindModule(meta.Modules, targetRel)
 
 	if workingMod == nil || targetMod == nil {
-		return "", fmt.Errorf("failed to locate working and target modules")
+		return nil, fmt.Errorf("failed to locate working and target modules")
 	}
 
-	var sb strings.Builder
+	// Set target module information
+	request.TargetModule = targetMod.Name
 
-	// ------------------------------------------------------------
-	// 1. External context of working module.
-	// ------------------------------------------------------------
-	if ann := workingMod.Annotation; ann != nil && ann.ExternalContext != "" {
-		sb.WriteString(fmt.Sprintf("# Module: `%s`\n", workingMod.Name))
-		sb.WriteString("## External Context\n")
-		sb.WriteString(ann.ExternalContext + "\n")
+	// Set target module context (combined internal and external context)
+	var targetContext strings.Builder
+	if ann := targetMod.Annotation; ann != nil {
+		if ann.ExternalContext != "" {
+			targetContext.WriteString("External Context: ")
+			targetContext.WriteString(ann.ExternalContext)
+			targetContext.WriteString("\n\n")
+		}
+		if ann.InternalContext != "" {
+			targetContext.WriteString("Internal Context: ")
+			targetContext.WriteString(ann.InternalContext)
+		}
 	}
 
-	// ------------------------------------------------------------
-	// 2. Internal context of modules between working and target.
-	// ------------------------------------------------------------
-	for m := targetMod.Parent; m != nil && m != workingMod; m = m.Parent {
-		if ann := m.Annotation; ann != nil && ann.InternalContext != "" {
-			sb.WriteString(fmt.Sprintf("# Module: `%s`\n", m.Name))
-			sb.WriteString("## Internal Context\n")
-			sb.WriteString(ann.InternalContext + "\n")
-		}
+	// Ensure TargetModuleContext is never empty
+	if targetContext.Len() == 0 {
+		targetContext.WriteString("No specific context available for this module.")
 	}
+	request.TargetModuleContext = targetContext.String()
 
-	// ------------------------------------------------------------
-	// 3. Public context of sibling modules along the path from the
-	//    parent of the target module up to (and including) the working
-	//    module. This replaces the previous logic that only considered
-	//    direct children of the working module.
-	// ------------------------------------------------------------
+	var parentModuleContexts []payload.ModuleContext
+	var subModuleContexts []payload.ModuleContext
 
+	// Collect parent and sibling module contexts
 	isAncestor := func(a, b string) bool {
 		return a == b || (a != "." && strings.HasPrefix(b, a+"/"))
 	}
@@ -82,36 +85,43 @@ func buildExtendedUserMessage(rootFS fs.FS, meta *project.Metadata, ec *context.
 				continue
 			}
 			if ann := child.Annotation; ann != nil && ann.PublicContext != "" {
-				sb.WriteString(fmt.Sprintf("# Module: `%s`\n", child.Name))
-				sb.WriteString("## Public Context\n")
-				sb.WriteString(ann.PublicContext + "\n")
+				parentModuleContexts = append(parentModuleContexts, payload.ModuleContext{
+					Name:    child.Name,
+					Content: ann.PublicContext,
+				})
 			}
 		}
 		if ancestor == workingMod {
 			break
 		}
 	}
 
-	// ------------------------------------------------------------
-	// 4. Public context of immediate sub-modules of target module.
-	// ------------------------------------------------------------
+	// Collect immediate sub-modules of target module
 	for _, child := range targetMod.Modules {
 		if ann := child.Annotation; ann != nil && ann.PublicContext != "" {
-			sb.WriteString(fmt.Sprintf("# Module: `%s`\n", child.Name))
-			sb.WriteString("## Public Context\n")
-			sb.WriteString(ann.PublicContext + "\n")
+			subModuleContexts = append(subModuleContexts, payload.ModuleContext{
+				Name:    child.Name,
+				Content: ann.PublicContext,
+			})
 		}
 	}
 
-	// ------------------------------------------------------------
-	// 5. Append file contents (only files from target module were
-	//    selected by selector.Select).
-	// ------------------------------------------------------------
-	filesMsg, err := payload.BuildUserMessage(rootFS, filePaths)
-	if err != nil {
-		return "", err
+	request.ParentModuleContexts = parentModuleContexts
+	request.SubModuleContexts = subModuleContexts
+
+	// Append file contents
+	var files []payload.FileContent
+	for _, path := range filePaths {
+		content, err := fs.ReadFile(rootFS, path)
+		if err != nil {
+			return nil, fmt.Errorf("failed to read file %s: %w", path, err)
+		}
+		files = append(files, payload.FileContent{
+			Path:    path,
+			Content: string(content),
+		})
 	}
-	sb.WriteString(filesMsg)
+	request.Files = files
 
-	return sb.String(), nil
+	return request, nil
 }

cmd/template/user_msg_builder_test.go

@@ -2,10 +2,11 @@ package template
 
 import (
 	"fmt"
-	"strings"
+	"reflect"
 	"testing"
 	"testing/fstest"
 
+	"github.com/vybdev/vyb/llm/payload"
 	"github.com/vybdev/vyb/workspace/context"
 	"github.com/vybdev/vyb/workspace/project"
 )
@@ -18,7 +19,7 @@ func Test_buildExtendedUserMessage(t *testing.T) {
 			InternalContext: fmt.Sprintf("%s internal", s),
 		}
 	}
-	// Build minimal module tree: root -> work (w) -> tgt (w/child)
+	// Build minimal module tree: root -> work (w) -> mid -> tgt (w/child)
 	root := &project.Module{Name: "."}
 	work := &project.Module{Name: "w", Parent: root, Annotation: ann("W")}
 	mid := &project.Module{Name: "w/mid", Parent: work, Annotation: ann("Mid")}
@@ -49,31 +50,64 @@ func Test_buildExtendedUserMessage(t *testing.T) {
 		TargetDir:   "w/mid/child",
 	}
 
-	msg, err := buildExtendedUserMessage(mfs, meta, ec, []string{"w/mid/child/file.txt"})
+	req, err := buildWorkspaceChangeRequest(mfs, meta, ec, []string{"w/mid/child/file.txt"})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 
 	// Basic assertions – ensure expected contexts are present.
-	mustContain := []string{"W external", "Mid internal", "Sibling public", "Cousin public", "hello"}
-	for _, s := range mustContain {
-		if !strings.Contains(msg, s) {
-			t.Fatalf("expected message to contain %q", s)
-		}
+	expectedFiles := []payload.FileContent{
+		{Path: "w/mid/child/file.txt", Content: "hello"},
+	}
+	if !reflect.DeepEqual(req.Files, expectedFiles) {
+		t.Errorf("Files mismatch: got %+v, want %+v", req.Files, expectedFiles)
 	}
 
-	mustNotContain := []string{
-		"W public", "W internal",
-		"Mid public", "Mid external",
-		"Sibling internal", "Sibling external",
-		"Cousin internal", "Cousin external",
-		"Out public", "Out internal", "Out external",
-		"mid content", "sibling content", "w content", "cousin content", "out content",
+	// Verify target module information
+	if req.TargetModule != "w/mid/child" {
+		t.Errorf("TargetModule mismatch: got %q, want %q", req.TargetModule, "w/mid/child")
 	}
 
-	for _, s := range mustNotContain {
-		if strings.Contains(msg, s) {
-			t.Fatalf("should not include contexts for target module itself, got message:\n%s", msg)
-		}
+	if req.TargetDirectory != "w/mid/child" {
+		t.Errorf("TargetDirectory mismatch: got %q, want %q", req.TargetDirectory, "w/mid/child")
+	}
+
+	expectedParentContexts := []payload.ModuleContext{
+		{Name: "w/mid/sibling", Content: "Sibling public"},
+		{Name: "w/cousin", Content: "Cousin public"},
+	}
+
+	if !reflect.DeepEqual(req.ParentModuleContexts, expectedParentContexts) {
+		t.Errorf("ParentModuleContexts mismatch:\ngot:  %+v\nwant: %+v", req.ParentModuleContexts, expectedParentContexts)
+	}
+
+	// Should be empty since target module has no sub-modules
+	if len(req.SubModuleContexts) != 0 {
+		t.Errorf("SubModuleContexts should be empty, got: %+v", req.SubModuleContexts)
+	}
+}
+
+func Test_buildExtendedUserMessage_nilValidation(t *testing.T) {
+	mfs := fstest.MapFS{
+		"file.txt": &fstest.MapFile{Data: []byte("content")},
+	}
+
+	ec := &context.ExecutionContext{
+		ProjectRoot: ".",
+		WorkingDir:  ".",
+		TargetDir:   ".",
+	}
+
+	// Test nil metadata
+	_, err := buildWorkspaceChangeRequest(mfs, nil, ec, []string{"file.txt"})
+	if err == nil || err.Error() != "metadata cannot be nil" {
+		t.Errorf("Expected 'metadata cannot be nil' error, got: %v", err)
+	}
+
+	// Test nil modules
+	meta := &project.Metadata{Modules: nil}
+	_, err = buildWorkspaceChangeRequest(mfs, meta, ec, []string{"file.txt"})
+	if err == nil || err.Error() != "metadata.Modules cannot be nil" {
+		t.Errorf("Expected 'metadata.Modules cannot be nil' error, got: %v", err)
 	}
 }

llm/README.md

@@ -40,13 +40,11 @@ debugging.
 
 ### `llm/payload`
 
-Pure data & helper utilities:
+Pure data structures for LLM communication:
 
-* `BuildUserMessage` – turns a list of files into a Markdown payload.
-* `BuildModuleContextUserMessage` – embeds annotations into the payload
-  according to precise inclusion rules.
-* Go structs mirroring every JSON schema (WorkspaceChangeProposal,
-  ModuleSelfContainedContext, …).
+* Go structs for request payloads (WorkspaceChangeRequest, ModuleContextRequest, ExternalContextsRequest)
+* Go structs for response payloads (WorkspaceChangeProposal, ModuleSelfContainedContext, ModuleExternalContextResponse)
+* All structs support JSON marshalling/unmarshalling for LLM interactions
 
 ## JSON Schema enforcement