From 608d6745b6b04214ea8cd356ae734dca15adf237 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Tue, 14 Apr 2026 14:28:22 -0700 Subject: [PATCH] fix(org): use yaml.Marshal for category_routing + newline-guard block appends Addresses code-review warnings on PR #75: - renderCategoryRoutingYAML now builds yaml.Node + yaml.Marshal, escaping YAML-reserved chars in role names correctly (was JSON-as-YAML, fragile on unicode line separators). - New appendYAMLBlock helper guarantees a newline boundary when concatenating YAML fragments into config.yaml (category_routing + initial_prompt both used to risk merging into the previous line). - Fixed struct comment (replace-per-key, not UNION). - Added TestCategoryRouting_EscapesYAMLSpecials and TestAppendYAMLBlock_NewlineGuard. Co-Authored-By: Claude Opus 4.6 (1M context) --- platform/internal/handlers/org.go | 82 +++++++++++++++----------- platform/internal/handlers/org_test.go | 50 +++++++++++++++- 2 files changed, 93 insertions(+), 39 deletions(-) diff --git a/platform/internal/handlers/org.go b/platform/internal/handlers/org.go index b9273c49..61322204 100644 --- a/platform/internal/handlers/org.go +++ b/platform/internal/handlers/org.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "regexp" + "sort" "strings" "time" @@ -101,8 +102,10 @@ type OrgWorkspace struct { WorkspaceAccess string `yaml:"workspace_access" json:"workspace_access"` // #65: "none" (default), "read_only", "read_write" Plugins []string `yaml:"plugins" json:"plugins"` InitialPrompt string `yaml:"initial_prompt" json:"initial_prompt"` - // CategoryRouting overrides/extends defaults.category_routing per-workspace. - // UNION semantics on the keys (mirroring plugin merge in #68). + // CategoryRouting extends/overrides defaults.category_routing per-workspace. + // Merge semantics: workspace keys replace defaults' value for the same key + // (empty list drops the category entirely); new keys are added. See + // mergeCategoryRouting. CategoryRouting map[string][]string `yaml:"category_routing" json:"category_routing"` Schedules []OrgSchedule `yaml:"schedules" json:"schedules"` Channels []OrgChannel `yaml:"channels" json:"channels"` @@ -379,7 +382,8 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, defa // Render category_routing into config.yaml so the agent can read its routing // table at runtime without hardcoded role names in prompts (issue #51). - // Per-workspace block UNIONs + per-key overrides defaults (mirrors plugin merge). + // Per-workspace keys replace defaults per-key (empty list drops the key); + // see mergeCategoryRouting for exact semantics. routing := mergeCategoryRouting(defaults.CategoryRouting, ws.CategoryRouting) if len(routing) > 0 { if configFiles == nil { @@ -389,8 +393,7 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, defa if err != nil { log.Printf("Org import: failed to render category_routing for %s: %v", ws.Name, err) } else { - existing := configFiles["config.yaml"] - configFiles["config.yaml"] = append(existing, []byte(block)...) + configFiles["config.yaml"] = appendYAMLBlock(configFiles["config.yaml"], block) } } @@ -411,8 +414,7 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, defa lines[i] = strings.TrimRight(line, " \t") } indented := strings.Join(lines, "\n ") - existing := configFiles["config.yaml"] - configFiles["config.yaml"] = append(existing, []byte(fmt.Sprintf("initial_prompt: |\n %s\n", indented))...) + configFiles["config.yaml"] = appendYAMLBlock(configFiles["config.yaml"], fmt.Sprintf("initial_prompt: |\n %s\n", indented)) log.Printf("Org import: injected initial_prompt (%d chars) into config.yaml for %s", len(trimmed), ws.Name) } @@ -701,47 +703,55 @@ func mergeCategoryRouting(defaultRouting, wsRouting map[string][]string) map[str // renderCategoryRoutingYAML emits a deterministic YAML block of the form: // // category_routing: -// security: ["Backend Engineer", "DevOps"] -// ui: ["Frontend Engineer"] +// security: [Backend Engineer, DevOps] +// ui: [Frontend Engineer] // -// Keys are sorted for stable output (test-friendly + diff-friendly). +// Keys are sorted for stable, test-friendly output. Uses yaml.Node + yaml.Marshal +// so role names containing YAML-reserved characters (colons, quotes, unicode line +// separators, etc.) are escaped by the YAML library — no ad-hoc quoting. func renderCategoryRoutingYAML(routing map[string][]string) (string, error) { if len(routing) == 0 { return "", nil } - // yaml.v3 marshal with sorted keys — Go map iteration is random, so we - // sort and emit by hand for deterministic output. keys := make([]string, 0, len(routing)) for k := range routing { + if k == "" { + continue + } keys = append(keys, k) } - // simple sort - for i := 1; i < len(keys); i++ { - for j := i; j > 0 && keys[j-1] > keys[j]; j-- { - keys[j-1], keys[j] = keys[j], keys[j-1] - } - } - var b strings.Builder - b.WriteString("category_routing:\n") + sort.Strings(keys) + + inner := &yaml.Node{Kind: yaml.MappingNode} for _, k := range keys { - roles := routing[k] - // JSON-marshal each role to handle quoting/escaping safely. - jsonRoles, err := json.Marshal(roles) - if err != nil { - return "", err + keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: k} + valNode := &yaml.Node{Kind: yaml.SequenceNode, Style: yaml.FlowStyle} + for _, role := range routing[k] { + valNode.Content = append(valNode.Content, &yaml.Node{Kind: yaml.ScalarNode, Value: role}) } - // Quote the key with json too in case of weird chars. - jsonKey, err := json.Marshal(k) - if err != nil { - return "", err - } - b.WriteString(" ") - b.Write(jsonKey) - b.WriteString(": ") - b.Write(jsonRoles) - b.WriteString("\n") + inner.Content = append(inner.Content, keyNode, valNode) } - return b.String(), nil + doc := &yaml.Node{Kind: yaml.MappingNode} + doc.Content = []*yaml.Node{ + {Kind: yaml.ScalarNode, Value: "category_routing"}, + inner, + } + out, err := yaml.Marshal(doc) + if err != nil { + return "", err + } + return string(out), nil +} + +// appendYAMLBlock concatenates a YAML fragment to an existing buffer, guaranteeing +// a newline boundary between them. Upstream code writes config.yaml in fragments +// (base template → category_routing → initial_prompt) and the base isn't +// guaranteed to end in \n, which would merge the last line into the next block. +func appendYAMLBlock(existing []byte, block string) []byte { + if len(existing) > 0 && existing[len(existing)-1] != '\n' { + existing = append(existing, '\n') + } + return append(existing, []byte(block)...) } // mergePlugins returns the union of defaults and per-workspace plugin lists diff --git a/platform/internal/handlers/org_test.go b/platform/internal/handlers/org_test.go index a57b0f79..a1e133a7 100644 --- a/platform/internal/handlers/org_test.go +++ b/platform/internal/handlers/org_test.go @@ -524,14 +524,58 @@ func TestCategoryRouting_RenderedIntoWorkspaceConfig(t *testing.T) { t.Errorf("ui roles wrong: %v", cr["ui"]) } // Output should be deterministic (keys sorted) — security < ui - if !strings.Contains(block, "\"security\":") || !strings.Contains(block, "\"ui\":") { - t.Errorf("expected JSON-quoted keys in block: %s", block) - } if strings.Index(block, "security") > strings.Index(block, "ui") { t.Errorf("expected sorted keys (security before ui), got:\n%s", block) } } +// YAML-reserved characters in role names must be escaped by the YAML library. +// Regression guard for the earlier hand-rolled JSON-as-YAML implementation. +func TestCategoryRouting_EscapesYAMLSpecials(t *testing.T) { + routing := map[string][]string{ + "security": {"Role: with colon", `Role "with quotes"`, "Role\nwith newline"}, + } + block, err := renderCategoryRoutingYAML(routing) + if err != nil { + t.Fatalf("render failed: %v", err) + } + var parsed map[string]interface{} + if err := yaml.Unmarshal([]byte(block), &parsed); err != nil { + t.Fatalf("rendered YAML is invalid for special chars: %v\n---\n%s", err, block) + } + cr := parsed["category_routing"].(map[string]interface{}) + roles := cr["security"].([]interface{}) + if len(roles) != 3 || roles[0] != "Role: with colon" { + t.Errorf("special-char roles did not round-trip: %v", roles) + } +} + +// appendYAMLBlock must guarantee a newline boundary between existing buffer and +// the new block so downstream parsers see two separate top-level keys. +func TestAppendYAMLBlock_NewlineGuard(t *testing.T) { + cases := []struct { + name string + existing string + block string + }{ + {"existing ends without newline", "name: foo", "category_routing:\n a: [b]\n"}, + {"existing ends with newline", "name: foo\n", "category_routing:\n a: [b]\n"}, + {"empty existing", "", "category_routing:\n a: [b]\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := appendYAMLBlock([]byte(tc.existing), tc.block) + var parsed map[string]interface{} + if err := yaml.Unmarshal(got, &parsed); err != nil { + t.Fatalf("appended YAML invalid: %v\n---\n%s", err, string(got)) + } + if _, ok := parsed["category_routing"]; !ok { + t.Errorf("expected top-level category_routing key, got: %v", parsed) + } + }) + } +} + func TestCategoryRouting_EmptyRendersNothing(t *testing.T) { got, err := renderCategoryRoutingYAML(nil) if err != nil {