forked from molecule-ai/molecule-core
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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang
parent
932ada2c59
commit
608d6745b6
@ -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
|
||||
|
||||
@ -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 {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user