From 3e38b6cacbc0015f8266c71a25540878fe06cc01 Mon Sep 17 00:00:00 2001 From: jay77721 <164177721+jay77721@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:19:47 +0800 Subject: [PATCH] fix(ingestion): unify extractor prompt placeholder rendering and per-chunk substitution (#18355) --- internal/ingestion/component/extractor.go | 246 ++++++--------- .../ingestion/component/extractor_test.go | 280 ++++++++++++++++-- internal/ingestion/component/slice3_test.go | 75 ++--- 3 files changed, 382 insertions(+), 219 deletions(-) diff --git a/internal/ingestion/component/extractor.go b/internal/ingestion/component/extractor.go index 6c1e3cb9de..db76fd8936 100644 --- a/internal/ingestion/component/extractor.go +++ b/internal/ingestion/component/extractor.go @@ -667,7 +667,15 @@ func (c *ExtractorComponent) Invoke(ctx context.Context, db *gorm.DB, inputs map } if len(in.chunks) == 0 { - ans, callErr := c.callText(timeoutCtx, db, in, "") + // Render the prompt with an empty chunk map so body placeholders + // resolve (or fall back to appending nothing) before the LLM call. + // Without this, a template containing {text} would be forwarded + // to the model unsubstituted. + callIn := in + callIn.systemPrompt, callIn.prompt = renderExtractorPrompts( + in.systemPrompt, in.prompt, map[string]any{}, "", + ) + ans, callErr := c.callText(timeoutCtx, db, callIn, "") if callErr != nil { return callErr } @@ -704,19 +712,8 @@ func (c *ExtractorComponent) Invoke(ctx context.Context, db *gorm.DB, inputs map // chunk field values, mirroring Python's // string_format at extractor.py:103. callIn := in - // buildExtractorMessages appends chunkText to the user message - // unconditionally. Substitute first; if a content-bearing - // placeholder ({text}/{chunks}/{content_with_weight}) was actually - // replaced, the chunk body is already in the prompt — suppress - // the append to avoid duplication. - var subP, subS bool - callIn.prompt, subP = substituteChunkPlaceholders(in.prompt, ck, text) - callIn.systemPrompt, subS = substituteChunkPlaceholders(in.systemPrompt, ck, text) - callChunkText := text - if subP || subS { - callChunkText = "" - } - ans, callErr := c.callText(timeoutCtx, db, callIn, callChunkText) + callIn.systemPrompt, callIn.prompt = renderExtractorPrompts(in.systemPrompt, in.prompt, ck, text) + ans, callErr := c.callText(timeoutCtx, db, callIn, "") if callErr != nil { return fmt.Errorf("chunk %d: %w", i, callErr) } @@ -1164,7 +1161,7 @@ func (c *ExtractorComponent) callRaw(ctx context.Context, db *gorm.DB, in extrac if err != nil { return nil, err } - msgs := buildExtractorMessages(in.systemPrompt, in.prompt, chunkText, in.chunks) + msgs := buildExtractorMessages(in.systemPrompt, in.prompt) fitted, fitErr := fitExtractorMessages(ctx, db, in.llmID, msgs) if fitErr != nil { return nil, fitErr @@ -1549,155 +1546,108 @@ func fitExtractorMessages(ctx context.Context, db *gorm.DB, llmID string, msgs [ return fitted, nil } -// buildExtractorMessages assembles system + user messages for -// one extraction call. The user prompt is rendered as -// "\n\n" so the python behavior of -// substituting the chunk text into the args dict is preserved -// without invoking a template engine. -// -// Prompt placeholders of the form `{ComponentName:ParamName@chunks}` -// are substituted with the joined text of all upstream chunks -// when chunks is non-empty. The python rag/flow/extractor/extractor.py -// build_existing_prompt path performs the same substitution at -// runtime; the Go port surfaces it as a regex on the prompt -// template so the reference resolves without invoking a template engine. -// -// Substitution is opt-in: when chunks is nil/empty the placeholder -// is left intact so a misconfigured template surfaces as a -// clear pattern rather than silently disappearing. -func buildExtractorMessages(system, prompt, chunkText string, chunks []map[string]any) []eschema.Message { +// buildExtractorMessages assembles system + user messages for one extraction +// call. Prompt rendering (placeholder substitution, chunk-text injection, and +// empty-prompt normalization) is performed upstream by renderExtractorPrompts; +// this function is a pure structural assembler with no rendering logic. +func buildExtractorMessages(system, user string) []eschema.Message { out := make([]eschema.Message, 0, 2) if system != "" { out = append(out, eschema.Message{Role: eschema.System, Content: system}) } - user := prompt - if chunkText != "" { - if user != "" { - user += "\n\n" - } - user += chunkText - } - if user == "" { - // An empty prompt + empty chunk is a degenerate call. - // The LLM driver returns an error; we surface that - // unchanged. - user = " " - } - user = substitutePromptPlaceholders(user, chunks) out = append(out, eschema.Message{Role: eschema.User, Content: user}) return out } -// substitutePromptPlaceholders replaces `{ComponentName:ParamName@chunks}` -// patterns in the user prompt with the joined text of all upstream -// chunks. The python rag/flow/extractor/extractor.py:build_existing_prompt -// path performs the same substitution at runtime using a Jinja -// template; the Go port keeps the regex form because the LLM -// driver does not require Jinja and the surface is small enough to -// avoid pulling in a template engine. -// -// Pattern grammar: -// -// {CmpName:ParamName@chunks} -// -// The CmpName and ParamName are both matched but ignored — the -// substitute is always "the joined chunk text" today. The -// CmpName/ParamName parsing exists so a future per-component -// substitution can extend the function without breaking the -// existing call sites. -func substitutePromptPlaceholders(prompt string, chunks []map[string]any) string { - if prompt == "" || len(chunks) == 0 { - return prompt - } - // Build the substitution payload once. Each chunk's text is - // joined with a blank line so a downstream LLM sees clear - // chunk boundaries. - var b strings.Builder - for i, ck := range chunks { - t, _ := ck["text"].(string) - if t == "" { - t, _ = ck["content_with_weight"].(string) - } - if t == "" { - continue - } - if i > 0 { - b.WriteString("\n\n") - } - b.WriteString(t) - } - repl := b.String() - if repl == "" { - return prompt - } - return placeholderRE.ReplaceAllString(prompt, repl) -} +// unifiedPlaceholderRE matches plain field placeholders {fieldName} and +// canvas macro placeholders {ComponentName:ParamName@fieldName}. +var unifiedPlaceholderRE = regexp.MustCompile(`\{([A-Za-z0-9_]+(:[A-Za-z0-9_]+)?@[A-Za-z0-9_]+|[A-Za-z_][A-Za-z0-9_]*)\}`) -// placeholderRE matches `{CmpName:ParamName@chunks}` patterns in -// Extractor user prompts. The CMP / Param groups are ignored for -// the @chunks variant but kept so the regex rejects arbitrary -// placeholders (a future per-component substitution extends here). -var placeholderRE = regexp.MustCompile(`\{[A-Za-z0-9_]+:[A-Za-z0-9_]+@chunks\}`) - -// simplePlaceholderRE matches simple {field_name} placeholders -// (no colons, no @chunks). Used by substituteChunkPlaceholders to -// replace {text}, {content_with_weight}, etc. with the current -// chunk's field values, mirroring Python's string_format -// (agent/component/base.py:602-609). -var simplePlaceholderRE = regexp.MustCompile(`\{[A-Za-z_][A-Za-z0-9_]*\}`) - -// contentPlaceholders are the simple placeholders that resolve to the -// chunk body. Substituting any of them indicates the chunk body is -// already embedded in the prompt — so the automatic append in -// buildExtractorMessages must be suppressed to avoid duplication. -// "text" and "chunks" both map to chunkText (the primary chunk body); -// "content_with_weight" is a chunk field carrying the weighted body. -var contentPlaceholders = map[string]bool{ +// bodyPlaceholderAliases lists plain placeholder names that resolve to chunk body content. +var bodyPlaceholderAliases = map[string]bool{ "text": true, "chunks": true, "content_with_weight": true, } -// substituteChunkPlaceholders replaces {field_name} placeholders in -// the prompt with values from the current chunk map. The special -// aliases "text" and "chunks" map to chunkText (the current chunk's -// primary text), matching Python's `args[chunks_key] = ck["text"]` at -// extractor.py:102. Unmatched placeholders are left as-is. -// -// The "text" placeholder falls back to chunkText when the chunk has no -// explicit "text" field — e.g. when only content_with_weight is present. -// This makes {text}'s resolved value match the append path's `text` -// variable (which also falls back content_with_weight → text). -// -// Returns the substituted prompt and whether any content-bearing -// placeholder was replaced (i.e. the chunk body is now in the prompt). -func substituteChunkPlaceholders(prompt string, ck map[string]any, chunkText string) (string, bool) { - if prompt == "" || ck == nil { - return prompt, false +// isBodyPlaceholder reports whether key represents chunk body content +// (including upstream output references such as @chunks, @text, @markdown). +func isBodyPlaceholder(key string) bool { + if bodyPlaceholderAliases[key] { + return true } - // Build lookup: chunk fields + "chunks" alias + "text" fallback. - lookup := make(map[string]string, len(ck)+2) - for k, v := range ck { - lookup[k] = fmt.Sprintf("%v", v) - } - if _, has := lookup["text"]; !has { - lookup["text"] = chunkText - } - if _, has := lookup["chunks"]; !has { - lookup["chunks"] = chunkText - } - var substituted bool - out := simplePlaceholderRE.ReplaceAllStringFunc(prompt, func(match string) string { - key := match[1 : len(match)-1] // strip { } - if val, ok := lookup[key]; ok { - if contentPlaceholders[key] { - substituted = true - } - return val + return strings.HasSuffix(key, "@chunks") || + strings.HasSuffix(key, "@text") || + strings.HasSuffix(key, "@markdown") +} + +// renderExtractorPrompts performs single-pass rendering of system and user +// prompt templates for one chunk: +// 1. Content placeholders ({text}, {chunks}, {content_with_weight}, +// {ComponentName:ParamName@chunks}, etc.) are resolved: first from the chunk +// map, falling back to chunkText. If a non-empty value is resolved, +// bodyInjected is marked true. +// 2. Chunk metadata fields (such as {title}, {author}) are resolved from the +// chunk map. +// 3. Unrecognized placeholders are preserved verbatim. +// 4. If no non-empty chunk body was injected anywhere in the system or user +// prompt, chunkText is automatically appended to the user prompt as a fallback. +func renderExtractorPrompts(sysTemplate, userTemplate string, ck map[string]any, chunkText string) (string, string) { + var bodyInjected bool + + render := func(tmpl string) string { + if tmpl == "" { + return "" } - return match // leave unknown placeholders as-is - }) - return out, substituted + return unifiedPlaceholderRE.ReplaceAllStringFunc(tmpl, func(match string) string { + key := match[1 : len(match)-1] // strip { } + + // A. Content placeholders: check chunk map first, fallback to chunkText + if isBodyPlaceholder(key) { + var val string + if raw, ok := ck[key]; ok { + val = fmt.Sprintf("%v", raw) + } + if strings.TrimSpace(val) == "" { + val = chunkText + } + if strings.TrimSpace(val) != "" { + bodyInjected = true + } + return val + } + + // B. Chunk metadata fields + if val, ok := ck[key]; ok { + return fmt.Sprintf("%v", val) + } + + // C. Leave unknown placeholders as-is + return match + }) + } + + renderedSys := render(sysTemplate) + renderedUser := render(userTemplate) + + if !bodyInjected && strings.TrimSpace(chunkText) != "" { + // TrimSpace before testing emptiness is intentional: a user template + // that renders to pure whitespace (e.g. " ") must not produce a + // leading "\n\n" separator — the result should be chunkText alone. + // A plain `if renderedUser != ""` check would fail that invariant. + trimmed := strings.TrimSpace(renderedUser) + if trimmed != "" { + renderedUser = trimmed + "\n\n" + chunkText + } else { + renderedUser = chunkText + } + } + + if strings.TrimSpace(renderedUser) == "" { + renderedUser = " " + } + + return renderedSys, renderedUser } // tryParseJSONObject tries to parse s as a JSON object. Returns diff --git a/internal/ingestion/component/extractor_test.go b/internal/ingestion/component/extractor_test.go index 8e036f9015..e4f8ca77e3 100644 --- a/internal/ingestion/component/extractor_test.go +++ b/internal/ingestion/component/extractor_test.go @@ -49,10 +49,9 @@ type stubExtractorChatInvoker struct { // as the wrap-error. tests set entries == call count they expect. responses []stubResponse - // lastReq records the most recent call's request for inspection - // (e.g. driver / model name resolved from the llm_id). - lastReq extractorChatRequest - calls atomic.Int32 + // requests records every call in order. Callers read via lastRequest(). + requests []extractorChatRequest + calls atomic.Int32 } // stubResponse couples a Content value and an Err. tests populate @@ -65,7 +64,7 @@ type stubResponse struct { func (s *stubExtractorChatInvoker) Chat(_ context.Context, req extractorChatRequest) (*extractorChatResponse, error) { s.calls.Add(1) s.mu.Lock() - s.lastReq = req + s.requests = append(s.requests, req) var resp stubResponse if len(s.responses) > 0 { resp = s.responses[0] @@ -78,6 +77,14 @@ func (s *stubExtractorChatInvoker) Chat(_ context.Context, req extractorChatRequ return &extractorChatResponse{Content: resp.Content}, nil } +// lastRequest returns the most recent recorded request. Callers must hold s.mu. +func (s *stubExtractorChatInvoker) lastRequest() extractorChatRequest { + if len(s.requests) == 0 { + return extractorChatRequest{} + } + return s.requests[len(s.requests)-1] +} + func (s *stubExtractorChatInvoker) Calls() int32 { return s.calls.Load() } // withStubChatInvoker installs a stub invoker for the duration of @@ -465,8 +472,8 @@ func TestExtractorComponent_Invoke_PerCallLLMIDOverride(t *testing.T) { } stub.mu.Lock() defer stub.mu.Unlock() - if stub.lastReq.ModelName != "override-llm" { - t.Errorf("ModelName = %q, want override-llm", stub.lastReq.ModelName) + if stub.lastRequest().ModelName != "override-llm" { + t.Errorf("ModelName = %q, want override-llm", stub.lastRequest().ModelName) } } @@ -488,11 +495,11 @@ func TestExtractorComponent_Invoke_CompositeLLMID(t *testing.T) { } stub.mu.Lock() defer stub.mu.Unlock() - if stub.lastReq.Driver != "openai" { - t.Errorf("Driver = %q, want openai", stub.lastReq.Driver) + if stub.lastRequest().Driver != "openai" { + t.Errorf("Driver = %q, want openai", stub.lastRequest().Driver) } - if stub.lastReq.ModelName != "gpt-4o-mini" { - t.Errorf("ModelName = %q, want gpt-4o-mini", stub.lastReq.ModelName) + if stub.lastRequest().ModelName != "gpt-4o-mini" { + t.Errorf("ModelName = %q, want gpt-4o-mini", stub.lastRequest().ModelName) } } @@ -1167,11 +1174,11 @@ func TestExtractorComponent_Invoke_TemperatureSet(t *testing.T) { stub.mu.Lock() defer stub.mu.Unlock() - if stub.lastReq.Temperature == nil { + if stub.lastRequest().Temperature == nil { t.Fatal("Temperature is nil, want 0.2") } - if *stub.lastReq.Temperature != 0.2 { - t.Errorf("Temperature = %v, want 0.2", *stub.lastReq.Temperature) + if *stub.lastRequest().Temperature != 0.2 { + t.Errorf("Temperature = %v, want 0.2", *stub.lastRequest().Temperature) } if stub.calls.Load() != 1 { t.Errorf("expected exactly 1 LLM call (keyword), got %d", stub.calls.Load()) @@ -1200,8 +1207,8 @@ func TestExtractorComponent_Invoke_FieldNameTemperatureDefault(t *testing.T) { stub.mu.Lock() defer stub.mu.Unlock() - if stub.lastReq.Temperature != nil { - t.Errorf("Temperature = %v, want nil (field extraction uses model default)", *stub.lastReq.Temperature) + if stub.lastRequest().Temperature != nil { + t.Errorf("Temperature = %v, want nil (field extraction uses model default)", *stub.lastRequest().Temperature) } } @@ -1488,7 +1495,7 @@ func TestExtractorComponent_Invoke_ContentWithWeightPlaceholder(t *testing.T) { stub.mu.Lock() defer stub.mu.Unlock() var userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { if msg.Role == eschema.User { userContent = msg.Content } @@ -1530,7 +1537,7 @@ func TestExtractorComponent_Invoke_NonContentPlaceholderKeepsChunkText(t *testin stub.mu.Lock() defer stub.mu.Unlock() var userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { if msg.Role == eschema.User { userContent = msg.Content } @@ -1570,7 +1577,7 @@ func TestExtractorComponent_Invoke_UnresolvedTextPlaceholderKeepsChunkText(t *te stub.mu.Lock() defer stub.mu.Unlock() var userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { if msg.Role == eschema.User { userContent = msg.Content } @@ -1606,7 +1613,7 @@ func TestExtractorComponent_Invoke_SubstitutesPlaceholders(t *testing.T) { stub.mu.Lock() defer stub.mu.Unlock() var userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { if msg.Role == eschema.User { userContent = msg.Content } @@ -1648,7 +1655,7 @@ func TestExtractorComponent_Invoke_PlaceholderChunksAlias(t *testing.T) { stub.mu.Lock() defer stub.mu.Unlock() var userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { if msg.Role == eschema.User { userContent = msg.Content } @@ -1688,7 +1695,7 @@ func TestExtractorComponent_Invoke_AppendsChunkTextWhenNoPlaceholder(t *testing. stub.mu.Lock() defer stub.mu.Unlock() var userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { if msg.Role == eschema.User { userContent = msg.Content } @@ -1724,7 +1731,7 @@ func TestExtractorComponent_Invoke_SystemPromptPlaceholderSuppressesAppend(t *te stub.mu.Lock() defer stub.mu.Unlock() var sysContent, userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { switch msg.Role { case eschema.System: sysContent = msg.Content @@ -1773,7 +1780,7 @@ func TestExtractorComponent_Invoke_FieldValueContainsPlaceholderSubstring(t *tes stub.mu.Lock() defer stub.mu.Unlock() var userContent string - for _, msg := range stub.lastReq.Messages { + for _, msg := range stub.lastRequest().Messages { if msg.Role == eschema.User { userContent = msg.Content } @@ -1865,17 +1872,21 @@ func TestExtractorComponent_CallRaw_FitsBeforeInvoke(t *testing.T) { stub := withStubChatInvoker(t, stubResponse{Content: `{"ok": true}`}) c := &ExtractorComponent{} + // callRaw is a pure dispatcher: callers are responsible for pre-rendering + // the prompt. Inline a large chunk body directly into the user prompt so + // fitExtractorMessages has real content to trim. + chunkBody := strings.Repeat("chunk text with lots of tokens. ", 500) _, err := c.callText(t.Context(), nil, extractorInputs{ systemPrompt: "extract fields", - prompt: "summarize", + prompt: "summarize\n\n" + chunkBody, llmID: "test@test", - }, strings.Repeat("chunk text with lots of tokens. ", 500)) + }, "") if err != nil { t.Fatalf("callText: %v", err) } stub.mu.Lock() - req := stub.lastReq + req := stub.lastRequest() stub.mu.Unlock() if len(req.Messages) == 0 { t.Fatal("invoker was not called") @@ -1931,7 +1942,7 @@ func TestExtractorComponent_CallRaw_CustomContextOverride(t *testing.T) { } stub.mu.Lock() - req := stub.lastReq + req := stub.lastRequest() stub.mu.Unlock() if len(req.Messages) == 0 { t.Fatal("invoker was not called") @@ -2104,3 +2115,216 @@ func TestExtractorContextLength_NilDBGraceful(t *testing.T) { t.Fatalf("extractorContextLength(nil db, default model) = %d, want 0 (skip fitting)", got) } } + +// TestExtractorComponent_Invoke_AtChunksPlaceholderPerChunk verifies that +// when the prompt contains {ComponentName:ParamName@chunks}, each chunk's +// LLM invocation receives ONLY that chunk's text (not all chunks joined together), +// and the chunk text appears exactly once without duplication. +func TestExtractorComponent_Invoke_AtChunksPlaceholderPerChunk(t *testing.T) { + stub := withStubChatInvoker(t, + stubResponse{Content: "answer1"}, + stubResponse{Content: "answer2"}, + ) + + c := &ExtractorComponent{Param: schema.ExtractorParam{ + FieldName: "summary", + Prompt: "Summarize: {TokenChunker:BumpyStarsPress@chunks}", + LLMID: "gpt-4o-mini", + }} + + _, err := c.Invoke(t.Context(), nil, map[string]any{ + "chunks": []map[string]any{ + {"text": "Chunk One Body"}, + {"text": "Chunk Two Body"}, + }, + }) + if err != nil { + t.Fatalf("Invoke: %v", err) + } + + stub.mu.Lock() + defer stub.mu.Unlock() + + if len(stub.requests) != 2 { + t.Fatalf("got %d LLM calls, want 2", len(stub.requests)) + } + + // Call 1 must contain Chunk 1 exactly once, and NOT contain Chunk 2 + req1User := stub.requests[0].Messages[len(stub.requests[0].Messages)-1].Content + if n := strings.Count(req1User, "Chunk One Body"); n != 1 { + t.Errorf("call 1 chunk text count = %d, want 1; prompt: %q", n, req1User) + } + if strings.Contains(req1User, "Chunk Two Body") { + t.Errorf("call 1 wrongly contains Chunk Two Body; prompt: %q", req1User) + } + + // Call 2 must contain Chunk 2 exactly once, and NOT contain Chunk 1 + req2User := stub.requests[1].Messages[len(stub.requests[1].Messages)-1].Content + if n := strings.Count(req2User, "Chunk Two Body"); n != 1 { + t.Errorf("call 2 chunk text count = %d, want 1; prompt: %q", n, req2User) + } + if strings.Contains(req2User, "Chunk One Body") { + t.Errorf("call 2 wrongly contains Chunk One Body; prompt: %q", req2User) + } +} + +// TestRenderExtractorPrompts_TableDriven covers all prompt rendering cases +// and fallback permutations. +func TestRenderExtractorPrompts_TableDriven(t *testing.T) { + tests := []struct { + name string + sysTemplate string + userTemplate string + ck map[string]any + chunkText string + wantSys string + wantUser string + }{ + { + name: "text in user", + sysTemplate: "", + userTemplate: "Analyze: {text}", + ck: map[string]any{"text": "body content"}, + chunkText: "body content", + wantSys: "", + wantUser: "Analyze: body content", + }, + { + name: "text in system only suppresses user append", + sysTemplate: "Context: {text}", + userTemplate: "Extract:", + ck: map[string]any{"text": "body content"}, + chunkText: "body content", + wantSys: "Context: body content", + wantUser: "Extract:", + }, + { + name: "canvas macro @chunks in user", + sysTemplate: "", + userTemplate: "Summarize: {TokenChunker:BumpyStarsPress@chunks}", + ck: map[string]any{"text": "body content"}, + chunkText: "body content", + wantSys: "", + wantUser: "Summarize: body content", + }, + { + name: "canvas macro @text in user", + sysTemplate: "", + userTemplate: "Parse: {Parser:Doc@text}", + ck: map[string]any{"text": "body content"}, + chunkText: "body content", + wantSys: "", + wantUser: "Parse: body content", + }, + { + name: "canvas macro @markdown in user", + sysTemplate: "", + userTemplate: "Parse: {Parser:Doc@markdown}", + ck: map[string]any{"text": "body content"}, + chunkText: "body content", + wantSys: "", + wantUser: "Parse: body content", + }, + { + name: "metadata only appends chunkText", + sysTemplate: "", + userTemplate: "Title: {title}", + ck: map[string]any{"title": "DocTitle", "text": "body content"}, + chunkText: "body content", + wantSys: "", + wantUser: "Title: DocTitle\n\nbody content", + }, + { + name: "no placeholder appends chunkText", + sysTemplate: "", + userTemplate: "Summarize the text:", + ck: map[string]any{"text": "body content"}, + chunkText: "body content", + wantSys: "", + wantUser: "Summarize the text:\n\nbody content", + }, + { + name: "empty chunkText does not append", + sysTemplate: "", + userTemplate: "Summarize:", + ck: map[string]any{}, + chunkText: "", + wantSys: "", + wantUser: "Summarize:", + }, + { + name: "content_with_weight present in ck", + sysTemplate: "", + userTemplate: "Weighted: {content_with_weight}", + ck: map[string]any{"content_with_weight": "weighted content", "text": "plain content"}, + chunkText: "weighted content", + wantSys: "", + wantUser: "Weighted: weighted content", + }, + { + name: "content_with_weight absent in ck falls back to chunkText", + sysTemplate: "", + userTemplate: "Weighted: {content_with_weight}", + ck: map[string]any{"text": "plain content"}, + chunkText: "plain content", + wantSys: "", + wantUser: "Weighted: plain content", + }, + { + name: "whitespace user template trimmed before append", + sysTemplate: "", + userTemplate: " ", + ck: map[string]any{"text": "body content"}, + chunkText: "body content", + wantSys: "", + wantUser: "body content", + }, + { + name: "empty templates produce single space user turn", + sysTemplate: "", + userTemplate: "", + ck: map[string]any{}, + chunkText: "", + wantSys: "", + wantUser: " ", + }, + { + // content_with_weight is present in ck but its value is an empty + // string. isBodyPlaceholder fires, but the resolved value is empty + // so bodyInjected must NOT be set — the fallback append should + // still fire and deliver chunkText. + name: "content_with_weight empty string in ck falls back to chunkText", + sysTemplate: "", + userTemplate: "Weighted: {content_with_weight}", + ck: map[string]any{"content_with_weight": ""}, + chunkText: "fallback body", + wantSys: "", + wantUser: "Weighted: fallback body", + }, + { + // Both system and user prompts reference body placeholders. + // The body must appear in each substitution position but must NOT + // be appended a third time (bodyInjected is shared across both + // render calls). + name: "body placeholder in both system and user no extra append", + sysTemplate: "Context: {text}", + userTemplate: "Also: {chunks}", + ck: map[string]any{"text": "the body"}, + chunkText: "the body", + wantSys: "Context: the body", + wantUser: "Also: the body", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotSys, gotUser := renderExtractorPrompts(tt.sysTemplate, tt.userTemplate, tt.ck, tt.chunkText) + if gotSys != tt.wantSys { + t.Errorf("renderExtractorPrompts() gotSys = %q, want %q", gotSys, tt.wantSys) + } + if gotUser != tt.wantUser { + t.Errorf("renderExtractorPrompts() gotUser = %q, want %q", gotUser, tt.wantUser) + } + }) + } +} diff --git a/internal/ingestion/component/slice3_test.go b/internal/ingestion/component/slice3_test.go index 387e1118d7..4b64be01c4 100644 --- a/internal/ingestion/component/slice3_test.go +++ b/internal/ingestion/component/slice3_test.go @@ -25,62 +25,51 @@ import ( "testing" ) -// TestSubstitutePromptPlaceholders_ReplacesAtChunks pins the +// TestRenderExtractorPrompts_ReplacesAtChunks pins the // `{ComponentName:ParamName@chunks}` placeholder substitution. -// The substitute is the joined chunk text. -func TestSubstitutePromptPlaceholders_ReplacesAtChunks(t *testing.T) { +func TestRenderExtractorPrompts_ReplacesAtChunks(t *testing.T) { prompt := "Extract metadata from: {TitleChunker:FlatMiceFix@chunks}" - chunks := []map[string]any{ - {"text": "First chunk."}, - {"text": "Second chunk."}, - } - got := substitutePromptPlaceholders(prompt, chunks) + ck := map[string]any{"text": "First chunk."} + _, got := renderExtractorPrompts("", prompt, ck, "First chunk.") if strings.Contains(got, "{TitleChunker:FlatMiceFix@chunks}") { t.Errorf("placeholder not substituted: %q", got) } - if !strings.Contains(got, "First chunk.") || !strings.Contains(got, "Second chunk.") { + if !strings.Contains(got, "First chunk.") { t.Errorf("substitute missing chunk content: %q", got) } -} - -// TestSubstitutePromptPlaceholders_LeavesPatternWhenNoChunks pins -// the opt-in substitution rule. When chunks is empty the -// placeholder is left intact so a misconfigured template surfaces -// as a clear pattern rather than silently disappearing. -func TestSubstitutePromptPlaceholders_LeavesPatternWhenNoChunks(t *testing.T) { - prompt := "Extract metadata from: {TitleChunker:FlatMiceFix@chunks}" - got := substitutePromptPlaceholders(prompt, nil) - if got != prompt { - t.Errorf("empty chunks: placeholder should be preserved\n got: %q\n want: %q", got, prompt) + if n := strings.Count(got, "First chunk."); n != 1 { + t.Errorf("chunk text appears %d times, want 1: %q", n, got) } } -// TestSubstitutePromptPlaceholders_NoPlaceholderInPrompt pins the -// no-op behaviour when the prompt carries no @chunks pattern. -func TestSubstitutePromptPlaceholders_NoPlaceholderInPrompt(t *testing.T) { +// TestRenderExtractorPrompts_LeavesUnknownPattern pins that unrecognized +// placeholders are preserved verbatim. +func TestRenderExtractorPrompts_LeavesUnknownPattern(t *testing.T) { + prompt := "Extract metadata from: {unknown_placeholder}" + _, got := renderExtractorPrompts("", prompt, nil, "") + if got != prompt { + t.Errorf("unknown placeholder: pattern should be preserved\n got: %q\n want: %q", got, prompt) + } +} + +// TestRenderExtractorPrompts_NoPlaceholderAppendsChunkText pins the +// fallback append behavior when the prompt carries no body placeholder. +func TestRenderExtractorPrompts_NoPlaceholderAppendsChunkText(t *testing.T) { prompt := "Plain prompt with no substitution." - chunks := []map[string]any{{"text": "x"}} - got := substitutePromptPlaceholders(prompt, chunks) - if got != prompt { - t.Errorf("no-placeholder prompt should be unchanged\n got: %q\n want: %q", got, prompt) + ck := map[string]any{"text": "my chunk text"} + _, got := renderExtractorPrompts("", prompt, ck, "my chunk text") + want := "Plain prompt with no substitution.\n\nmy chunk text" + if got != want { + t.Errorf("got %q, want %q", got, want) } } -// TestSubstitutePromptPlaceholders_SkipsEmptyChunkText pins the -// per-chunk text trim. A chunk with no text field does not -// contribute a trailing blank line. -func TestSubstitutePromptPlaceholders_SkipsEmptyChunkText(t *testing.T) { - prompt := "p {TitleChunker:FlatMiceFix@chunks} q" - chunks := []map[string]any{ - {"text": ""}, - {"text": "actual"}, - {}, - } - got := substitutePromptPlaceholders(prompt, chunks) - if strings.Contains(got, "{TitleChunker:FlatMiceFix@chunks}") { - t.Errorf("placeholder not substituted: %q", got) - } - if !strings.Contains(got, "actual") { - t.Errorf("chunk text missing: %q", got) +// TestRenderExtractorPrompts_SkipsEmptyChunkText pins that an empty +// chunkText does not append a trailing blank line. +func TestRenderExtractorPrompts_SkipsEmptyChunkText(t *testing.T) { + prompt := "Plain prompt" + _, got := renderExtractorPrompts("", prompt, map[string]any{}, "") + if got != prompt { + t.Errorf("empty chunkText should not alter prompt\n got: %q\n want: %q", got, prompt) } }