From d279aee1ff661bda35d87caf0a6e0ac06810aee6 Mon Sep 17 00:00:00 2001 From: Zhichang Yu Date: Mon, 13 Jul 2026 23:32:07 +0800 Subject: [PATCH] Go ports of workflow and chunker fixes (#16878) Ports two Python fixes to Go: the variable_ref_patt underscore/colon fix (#16792) and the TokenChunker upstream-chunks fix (#16825). Keeps Go behavior aligned with upstream Python. --- internal/agent/canvas/variable_test.go | 153 +++++++++++++++++- internal/agent/runtime/template.go | 14 +- internal/ingestion/component/chunker/token.go | 16 +- .../ingestion/component/chunker/token_test.go | 39 +++++ 4 files changed, 208 insertions(+), 14 deletions(-) diff --git a/internal/agent/canvas/variable_test.go b/internal/agent/canvas/variable_test.go index 30746b27e0..4e404b31ac 100644 --- a/internal/agent/canvas/variable_test.go +++ b/internal/agent/canvas/variable_test.go @@ -14,15 +14,19 @@ // in canvas.get_value_with_variable AFTER the regex match succeeds. // - list indexing (xs.0) — same nested-path machinery. // -// Cpn IDs in tests use underscores (e.g. "llm_0") which is the real -// RAGFlow naming convention; the original documented regex -// `[a-zA-Z:0-9]+` did not allow underscores — see variable.go -// VarRefPattern comment. +// The VarRefPattern regex (internal/agent/runtime/template.go) is kept +// in sync with Python's agent/component/base.py variable_ref_patt. +// PR #16792 (Jun 2026) widened cpn_id from [a-zA-Z:0-9]+ to +// [a-zA-Z0-9_:]+ to accept underscored frontend ids and colon-bearing +// legacy DSL ids. This file pins both behaviors to prevent silent +// regression. package canvas import ( "reflect" "testing" + + "ragflow/internal/agent/runtime" ) func TestVariableResolver(t *testing.T) { @@ -203,3 +207,144 @@ func TestExtractRefs(t *testing.T) { t.Fatalf("ExtractRefs: got %v want %v", got, want) } } + +// TestVarRefPattern_MatchesUnderscoredComponentIDs pins that frontend- +// emitted ids like "userfillup_abc@line" match VarRefPattern. +// Regression test for the original #16758 underscore fix +// (Python equivalent: test_variable_ref_patt_matches_underscored_component_ids). +func TestVarRefPattern_MatchesUnderscoredComponentIDs(t *testing.T) { + cases := []struct { + text string + expected string // group-1 capture (bare ref) + }{ + {"{userfillup_abc@line}", "userfillup_abc@line"}, + {"{retrieval_xyz@chunks}", "retrieval_xyz@chunks"}, + {"{llm_0@content}", "llm_0@content"}, + {"{message_0@answer}", "message_0@answer"}, + } + + for _, c := range cases { + t.Run(c.text, func(t *testing.T) { + matches := VarRefPattern.FindAllStringSubmatch(c.text, 1) + if len(matches) == 0 || len(matches[0]) < 2 { + t.Fatalf("Expected %q to match VarRefPattern", c.text) + } + got := matches[0][1] + if got != c.expected { + t.Fatalf("%q: wrong capture — got %q, expected %q", c.text, got, c.expected) + } + }) + } +} + +// TestVarRefPattern_MatchesColonBearingComponentIDs pins that legacy +// DSL ids like "UserFillUp:CateInput@text" match VarRefPattern. +// Regression test from PR #16792 review note — colon-bearing cpn_ids +// are real and used in test fixtures/templates +// (Python equivalent: test_variable_ref_patt_matches_colon_bearing_component_ids). +func TestVarRefPattern_MatchesColonBearingComponentIDs(t *testing.T) { + cases := []struct { + text string + expected string + }{ + {"{UserFillUp:CateInput@text}", "UserFillUp:CateInput@text"}, + {"{UserFillUp:CodeInput@x}", "UserFillUp:CodeInput@x"}, + {"{UserFillUp:LoopInput@value}", "UserFillUp:LoopInput@value"}, + {"{Retrieval:KBSearch@formalized_content}", "Retrieval:KBSearch@formalized_content"}, + {"{CodeExec:Double@result}", "CodeExec:Double@result"}, + {"{Browser:BusyHatsSink@content}", "Browser:BusyHatsSink@content"}, + } + + for _, c := range cases { + t.Run(c.text, func(t *testing.T) { + matches := VarRefPattern.FindAllStringSubmatch(c.text, 1) + if len(matches) == 0 || len(matches[0]) < 2 { + t.Fatalf("Expected %q to match VarRefPattern — colon-bearing cpn_id lost its support.", c.text) + } + got := matches[0][1] + if got != c.expected { + t.Fatalf("%q: wrong capture — got %q, expected %q", c.text, got, c.expected) + } + }) + } +} + +// TestVarRefPattern_StillMatchesLegacyIDs verifies backward-compat: +// legacy ids without underscores/colons must still resolve. +func TestVarRefPattern_StillMatchesLegacyIDs(t *testing.T) { + cases := []struct { + text string + expected string + }{ + {"{begin@line}", "begin@line"}, + {"{retrieval@chunks}", "retrieval@chunks"}, + {"{sys.query}", "sys.query"}, + {"{sys.user_id}", "sys.user_id"}, + {"{env.HOME}", "env.HOME"}, + } + + for _, c := range cases { + t.Run(c.text, func(t *testing.T) { + matches := VarRefPattern.FindAllStringSubmatch(c.text, 1) + if len(matches) == 0 || len(matches[0]) < 2 { + t.Fatalf("Expected %q to match VarRefPattern", c.text) + } + if matches[0][1] != c.expected { + t.Fatalf("%q: wrong capture — got %q, expected %q", c.text, matches[0][1], c.expected) + } + }) + } +} + +// TestVarRefPattern_DoesNotMatchBareVarName verifies that +// `{line}` without a cpn_id prefix is intentionally not a template +// ref — it must remain literal so the user sees the literal text. +func TestVarRefPattern_DoesNotMatchBareVarName(t *testing.T) { + if VarRefPattern.MatchString("{line}") { + t.Fatal("Bare `{line}` should not match — only `cpn_id@var` / `sys.*` / `env.*` are valid template refs.") + } +} + +// TestVarRefPattern_ConsistencyPin verifies that the runtime.VarRefPattern +// pattern string and its factory-compiled regex agree. This guards against +// accidental edits that change the string without updating the compile. +func TestVarRefPattern_ConsistencyPin(t *testing.T) { + // The Go regexp package does not expose a way to reconstruct a + // regexp from its string representation at test time while + // preserving the same object identity, so we instead pin that the + // source string matches the expected pattern character-by-character. + want := `\{+\s*([a-zA-Z:0-9_]+@[A-Za-z0-9_.-]+|sys\.[A-Za-z0-9_.]+|env\.[A-Za-z0-9_.]+|item|index)\s*\}+` + if runtime.VarRefPattern.String() != want { + t.Errorf( + "VarRefPattern string has drifted.\n got %s\n want %s\n"+ + "Did you edit the regex? If so, update this test AND the Python source at agent/component/base.py.", + runtime.VarRefPattern.String(), want, + ) + } +} + +// TestResolveTemplate_SubstitutesUnderscoredRef ensures that +// an underscored ref is substituted end-to-end through ResolveTemplate. +func TestResolveTemplate_SubstitutesUnderscoredRef(t *testing.T) { + s := NewCanvasState("run-1", "task-1") + s.SetVar("userfillup_abc", "line", "hello world") + + rendered, err := ResolveTemplate("Repeat: {{userfillup_abc@line}}", s) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rendered != "Repeat: hello world" { + t.Fatalf("got %q want %q", rendered, "Repeat: hello world") + } +} + +// TestExtractRefs_ColonBearing verifies ExtractRefs returns colon-bearing refs. +func TestExtractRefs_ColonBearing(t *testing.T) { + got := ExtractRefs( + "{{UserFillUp:CateInput@text}} {{Retrieval:KBSearch@f}} {{sys.query}}", + ) + want := []string{"UserFillUp:CateInput@text", "Retrieval:KBSearch@f", "sys.query"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ExtractRefs colon-bearing: got %v want %v", got, want) + } +} diff --git a/internal/agent/runtime/template.go b/internal/agent/runtime/template.go index 6f443d5985..fa57e90bff 100644 --- a/internal/agent/runtime/template.go +++ b/internal/agent/runtime/template.go @@ -28,20 +28,16 @@ import ( ) // VarRefPattern matches the RAGFlow variable reference syntax. -// Mirrors agent/component/base.py:368 in spirit with one deviation: the -// cpn_id part includes '_' (real RAGFlow cpn_ids are like "begin_0", -// "llm_0", "cpn_0"). The Python regex as documented in the plan -// (`[a-zA-Z:0-9]+`) would not match those — this looks like a -// documentation bug in the plan; the Python source likely has -// the underscore too. The pattern uses underscore-friendly -// matching; a future cross-check against the live Python source -// can confirm the exact behavior. +// Matches agent/component/base.py variable_ref_patt exactly after +// PR #16792 (which widened cpn_id from [a-zA-Z:0-9]+ to +// [a-zA-Z0-9_:]+ to accept underscores in frontend-emitted ids as +// well as colons in legacy DSL ids). // // Pattern: // // \{+\s*()\s*\}+ // where = cpn_id@param | sys.x | env.x | item | index -// cpn_id = [a-zA-Z:0-9_]+ (note: underscore added; see deviation note) +// cpn_id = [a-zA-Z:0-9_]+ (character classes match Python's [a-zA-Z0-9_:]+) // param = [A-Za-z0-9_.-]+ // // Capture group 1 holds the bare ref without braces (e.g. "cpn_0@content", diff --git a/internal/ingestion/component/chunker/token.go b/internal/ingestion/component/chunker/token.go index a2109b4ecb..0975e9415b 100644 --- a/internal/ingestion/component/chunker/token.go +++ b/internal/ingestion/component/chunker/token.go @@ -193,6 +193,20 @@ func (c *TokenChunkerComponent) invoke(ctx context.Context, inputs map[string]an } return c.invokeTextPayload(ctx, *upstream.HTMLResult, delimPattern, childrenPattern), nil default: + // Port of token_chunker.py:347 — when the upstream emitted + // chunks (output_format == "chunks", e.g. a TitleChunker + // feeding into this TokenChunker), consume those chunks rather + // than the raw parser json_result. Otherwise fall back to the + // structured json_result. This fixes #16812 where a + // TitleChunker → TokenChunker chain silently discarded the + // chapter-level chunks and re-chunked the raw parser output. + var items []schema.ChunkDoc + if upstream.OutputFormat == schema.PayloadFormatChunks { + items = upstream.Chunks + } else { + items = upstream.JSONResult + } + // Re-acquire the source PDF (if the Parser forwarded storage // refs) so image/table sections are cropped on demand rather // than carried through the wire. Best-effort: a nil engine @@ -204,7 +218,7 @@ func (c *TokenChunkerComponent) invoke(ctx context.Context, inputs map[string]an if engine != nil { defer engine.Close() } - return c.invokeJSONPayload(ctx, upstream.JSONResult, delimPattern, childrenPattern, engine), nil + return c.invokeJSONPayload(ctx, items, delimPattern, childrenPattern, engine), nil } } diff --git a/internal/ingestion/component/chunker/token_test.go b/internal/ingestion/component/chunker/token_test.go index 3c1ef9c52b..e6fe116231 100644 --- a/internal/ingestion/component/chunker/token_test.go +++ b/internal/ingestion/component/chunker/token_test.go @@ -284,3 +284,42 @@ func TestTokenChunker_NewAcceptsDefaults(t *testing.T) { t.Errorf("default delimiter_mode = %q, want token_size", got) } } + +// TestTokenChunker_PrefersUpstreamChunks is the Go port of the Python +// regression test for #16812 (PR #16825). When a TitleChunker feeds +// this TokenChunker with output_format == "chunks" AND both a "chunks" +// list and a raw "json" list on the wire, the TokenChunker must +// consume the upstream chunks (CHAPTER-AWARE) and must NOT fall through +// to the raw parser json_result (RAW-PARSER-JSON). +func TestTokenChunker_PrefersUpstreamChunks(t *testing.T) { + c, err := NewTokenChunker(map[string]any{ + "delimiter_mode": "delimiter", + "delimiters": []string{"\n"}, + }) + if err != nil { + t.Fatalf("NewTokenChunker: %v", err) + } + out, err := c.Invoke(context.Background(), map[string]any{ + "name": "doc.md", + "output_format": "chunks", + "chunks": []map[string]any{{"text": "CHAPTER-AWARE", "doc_type_kwd": "text"}}, + "json": []map[string]any{{"text": "RAW-PARSER-JSON", "doc_type_kwd": "text"}}, + }) + if err != nil { + t.Fatalf("Invoke: %v", err) + } + chunks, _ := out["chunks"].([]map[string]any) + if len(chunks) == 0 { + t.Fatal("chunks: want >=1, got 0") + } + for i, ck := range chunks { + text, _ := ck["text"].(string) + if text == "RAW-PARSER-JSON" { + t.Fatalf("chunk[%d] consumed the raw parser json_result instead of upstream chunks: %q", i, text) + } + if text == "CHAPTER-AWARE" { + return // happy path: upstream chunk preserved + } + } + t.Fatalf("upstream chunk 'CHAPTER-AWARE' was not found in output: %v", out["chunks"]) +}