From 16ff6cb1f25509f6b4003124031ade76fb74730c Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 28 Aug 2026 00:00:49 +0200 Subject: [PATCH 1/2] fix: 10 validated bug fixes across serve, skills, render, and perf tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each fix is paired with a regression test that failed (RED) on main before the fix and passes (GREEN) after; full per-bug rationale in the test comments. 1. serve: eventsRing.snapshot served the OLDEST limit-matching events once the ring held more than the limit; /api/events now returns the most-recent window, oldest-first as documented. 2. skills: frontmatter scalars with numeric/bool forms (version: 1.2, author: 7, trigger topic: 2) were silently dropped by bare string assertions after parseYAMLValue type inference; string-typed fields now coerce scalar forms canonically. 3. perf: head_tail description claimed a streaming early-stop the implementation does not do (exact totals are pinned by test and kept); description now matches behavior. 4. perf: math_eval % with fractional operands panicked (1 % 0.5 → integer divide by zero) or silently truncated (0.5 % 2 → 0); non-integer operands now get a clean error. 5. perf: sort order is now a validated case-insensitive enum ("DESC" previously sorted ascending silently) and reverse flips the effective direction (desc+reverse = ascending, per the schema contract). 6. render: extractJSONField required a space after the colon, so compact JSON args ({"path":"main.go"}) silently lost their tool previews; rewritten to locate keys properly and json-decode values (escapes resolved). 7. render: tool-preview truncation sliced bytes, splitting multi-byte runes into invalid UTF-8; truncation is now rune-safe. 8. perf: head_tail and word_count on directories surfaced raw scanner errors (word_count also reported the dir stat size next to the error); both now give count_lines' clear is-a-directory message. 9. perf: tree on a symlinked directory root reported a non-directory "file" sized by the link-target string and walked nothing; the explicitly-requested root is now resolved, descendants keep Lstat semantics (symlinks shown, never followed). 10. serve: eventsRing.snapshot no longer aliases the ring's backing slice in its result (copy before return). Verified: go build, go vet ./..., golangci-lint (0 issues), full internal/... suite, -race on changed internal packages, and the cmd/odek stateless subset for every touched tool family. Known environmental exclusion: TestBatchPatch_TrustedClasses blocks on an interactive TTY approval prompt in headless runs (pre-existing, code untouched by this branch). --- cmd/odek/perf_tools.go | 47 ++++- cmd/odek/perf_tools_headtail_desc_test.go | 25 +++ cmd/odek/perf_tools_math_rem_test.go | 36 ++++ cmd/odek/perf_tools_sort_preview_dir_test.go | 177 ++++++++++++++++++ cmd/odek/serve_runs.go | 35 ++-- cmd/odek/serve_runs_events_ring_test.go | 66 +++++++ internal/render/render.go | 102 +++++++--- internal/resource/resource.go | 4 +- internal/skills/loader.go | 54 +++++- .../skills/loader_frontmatter_scalars_test.go | 68 +++++++ 10 files changed, 557 insertions(+), 57 deletions(-) create mode 100644 cmd/odek/perf_tools_headtail_desc_test.go create mode 100644 cmd/odek/perf_tools_math_rem_test.go create mode 100644 cmd/odek/perf_tools_sort_preview_dir_test.go create mode 100644 cmd/odek/serve_runs_events_ring_test.go create mode 100644 internal/skills/loader_frontmatter_scalars_test.go diff --git a/cmd/odek/perf_tools.go b/cmd/odek/perf_tools.go index 1b5da440..b654e491 100644 --- a/cmd/odek/perf_tools.go +++ b/cmd/odek/perf_tools.go @@ -15,6 +15,7 @@ import ( "go/parser" "go/token" "io" + "math" "net/http" "os" "os/exec" @@ -893,6 +894,12 @@ func evalNode(node ast.Expr) (float64, error) { } return x / y, nil case token.REM: + // Modulo is an integer operation: fractional operands either panic + // (int64(0.5) == 0 → divide by zero) or silently truncate to a + // wrong answer (0.5 % 2 → 0). Reject them cleanly instead. + if x != math.Trunc(x) || y != math.Trunc(y) { + return 0, fmt.Errorf("modulo requires integer operands (got %v %% %v)", x, y) + } if y == 0 { return 0, fmt.Errorf("modulo by zero") } @@ -1762,7 +1769,19 @@ func (t *treeTool) Call(argsJSON string) (result string, err error) { } func buildTree(ctx context.Context, root, path string, depth, maxDepth int, includeHidden bool) (treeEntry, error) { - info, err := os.Lstat(path) + var info os.FileInfo + var err error + if depth == 0 { + // The explicitly-requested root may be a symlink to a directory + // (/tmp → /private/tmp on macOS, a user's ~/link). Follow it — + // otherwise the root reports as a non-directory "file" whose size + // is the length of the target path and the walk never happens. + // Descendants keep Lstat semantics below: symlinked entries inside + // the tree are shown as-is, never followed. + info, err = os.Stat(path) + } else { + info, err = os.Lstat(path) + } if err != nil { return treeEntry{Path: wrapUntrusted(ctx, "tree:"+root, path), ErrMsg: err.Error()}, nil } @@ -2050,7 +2069,23 @@ func (t *sortTool) Call(argsJSON string) (result string, err error) { return jsonError("max 20 files per sort call") } - desc := args.Order == "desc" || args.Reverse + // Direction: order selects asc/desc (case-insensitive enum — the + // schema pins ["asc","desc"] but models emit "DESC" etc.), and an + // unknown value is a clean error rather than a silent ascending sort. + // reverse then flips the effective direction (desc+reverse = asc). + desc := false + switch strings.ToLower(strings.TrimSpace(args.Order)) { + case "": + // default ascending + case "asc": + case "desc": + desc = true + default: + return jsonError(fmt.Sprintf("invalid order %q (use \"asc\" or \"desc\")", args.Order)) + } + if args.Reverse { + desc = !desc + } // Read all files var allLines []string @@ -2166,7 +2201,7 @@ type headTailTool struct { func (t *headTailTool) Name() string { return "head_tail" } func (t *headTailTool) Description() string { - return `Read the first or last N lines of one or more files. Streaming — stops after N lines (no full-file read for head). Supports multiple files in parallel. Zero-fork — pure Go scanner.` + return `Read the first or last N lines of one or more files. Reports the file's exact total line count (the head path scans the whole file, bounded by a 1 MiB line buffer). Supports multiple files in parallel. Zero-fork — pure Go scanner.` } type headTailFileArg struct { @@ -2263,6 +2298,9 @@ func (t *headTailTool) readPreview(path string, n int, mode string) (result head if err != nil { return headTailFileResult{Path: path, Error: fmt.Sprintf("cannot stat %q: %v", path, err)} } + if info.IsDir() { + return headTailFileResult{Path: path, Error: fmt.Sprintf("%q is a directory — use tree or glob to explore directories", path)} + } if info.Size() > maxFileReadBytes { return headTailFileResult{Path: path, Error: fmt.Sprintf("file too large (%d bytes, max %d)", info.Size(), maxFileReadBytes)} } @@ -2699,6 +2737,9 @@ func (t *wordCountTool) countWords(path string) (entry wordCountEntry) { if err != nil { return wordCountEntry{Path: path, Error: fmt.Sprintf("cannot stat: %v", err)} } + if info.IsDir() { + return wordCountEntry{Path: path, Error: fmt.Sprintf("%q is a directory — use tree or glob to explore directories", path)} + } if info.Size() > maxFileReadBytes { return wordCountEntry{Path: path, Error: fmt.Sprintf("file too large (%d bytes, max %d)", info.Size(), maxFileReadBytes)} } diff --git a/cmd/odek/perf_tools_headtail_desc_test.go b/cmd/odek/perf_tools_headtail_desc_test.go new file mode 100644 index 00000000..ac9958ec --- /dev/null +++ b/cmd/odek/perf_tools_headtail_desc_test.go @@ -0,0 +1,25 @@ +package main + +import ( + "strings" + "testing" +) + +// Regression (doc-contract): the head_tail tool description promised +// "Streaming — stops after N lines (no full-file read for head)", but +// readHead full-scans every file to report the exact total line count — +// and TestHeadTail_Head pins that exact total as intended behavior. The +// model (and every operator reading docs) was told the opposite of what +// the code does: a 10 MiB log previewed with lines=5 silently paid for +// a full scan. This test pins the description to the real contract: +// bounded memory (1 MiB line buffer, 1 MiB output cap), NOT a +// stopped-early scan. +func TestHeadTail_DescriptionMatchesBehavior(t *testing.T) { + desc := (&headTailTool{}).Description() + if strings.Contains(desc, "stops after N lines") || strings.Contains(desc, "no full-file read") { + t.Fatalf("head_tail description still claims a streaming stop the implementation does not do: %q", desc) + } + if !strings.Contains(desc, "total") { + t.Fatalf("head_tail description should mention that exact total line counts are reported: %q", desc) + } +} diff --git a/cmd/odek/perf_tools_math_rem_test.go b/cmd/odek/perf_tools_math_rem_test.go new file mode 100644 index 00000000..22ad91ee --- /dev/null +++ b/cmd/odek/perf_tools_math_rem_test.go @@ -0,0 +1,36 @@ +package main + +import ( + "strings" + "testing" +) + +// Regression (math_eval %): evalNode's REM branch checked y == 0 on the +// FLOAT before truncating both operands to int64. With a fractional +// divisor that truncates to zero the check passed and the integer modulo +// panicked ("1 % 0.5" → recovered as "internal tool error"); with a +// fractional dividend or divisor that truncates non-zero the operation +// silently returned a wrong answer ("0.5 % 2" → 0 instead of an error). +// Modulo requires integer operands; anything else must be a clean error. +func TestMathEval_RemRequiresIntegers(t *testing.T) { + // Integer modulo keeps working. + if v, err := evalMath("7 % 3"); err != nil || v != 1 { + t.Fatalf("7 %% 3 = %v, %v; want 1, nil", v, err) + } + if v, err := evalMath("-7 % 3"); err != nil || v != -1 { + t.Fatalf("-7 %% 3 = %v, %v; want -1, nil (Go truncating semantics)", v, err) + } + + // Fractional operands: clean error, never a panic, never a wrong value. + if _, err := evalMath("1 % 0.5"); err == nil { + t.Error("1 % 0.5: expected an error (integer divide by zero panic before the fix)") + } else if !strings.Contains(err.Error(), "integer") { + t.Errorf("1 %% 0.5: err = %v, want an integer-operands error", err) + } + if _, err := evalMath("0.5 % 2"); err == nil { + t.Error("0.5 % 2: expected an error (silently returned 0 before the fix)") + } + if _, err := evalMath("0.5 % 0.25"); err == nil { + t.Error("0.5 % 0.25: expected an error") + } +} diff --git a/cmd/odek/perf_tools_sort_preview_dir_test.go b/cmd/odek/perf_tools_sort_preview_dir_test.go new file mode 100644 index 00000000..02b03e5d --- /dev/null +++ b/cmd/odek/perf_tools_sort_preview_dir_test.go @@ -0,0 +1,177 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + "unicode/utf8" + + "github.com/BackendStack21/odek/internal/render" +) + +// ── Regression (sort order handling) ──────────────────────────────────── +// Two defects in sortTool.Call's direction logic: +// +// 1. desc := order == "desc" || reverse collapsed `reverse` into the +// order flag, so order:"desc" + reverse:true — "reverse the sort +// order" per the schema — produced descending output again instead +// of flipping it to ascending. +// +// 2. The order value was compared case-sensitively and unknown values +// silently sorted ascending: order:"DESC" returned a,b,c for input +// b,a,c with no error. The schema enum is ["asc","desc"]; unknown +// values must be rejected, not misinterpreted. + +func writeSortFixture(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "s.txt") + if err := os.WriteFile(path, []byte("b\na\nc\n"), 0644); err != nil { + t.Fatal(err) + } + return path +} + +func sortOutput(t *testing.T, args string) string { + t.Helper() + var r struct { + Output string `json:"output"` + Error string `json:"error"` + } + mustUnmarshal(t, callJSON(t, &sortTool{}, args), &r) + return unwrapUntrusted(r.Output) +} + +func TestSort_ReverseFlipsDirection(t *testing.T) { + path := writeSortFixture(t) + + if got := sortOutput(t, `{"path":"`+path+`","order":"desc","reverse":true}`); got != "a\nb\nc" { + t.Errorf("desc+reverse = %q, want ascending %q (reverse must flip the order)", got, "a\\nb\\nc") + } + if got := sortOutput(t, `{"path":"`+path+`","order":"asc","reverse":true}`); got != "c\nb\na" { + t.Errorf("asc+reverse = %q, want descending %q", got, "c\\nb\\na") + } +} + +func TestSort_OrderValueValidation(t *testing.T) { + path := writeSortFixture(t) + + // Case-insensitive enum match. + if got := sortOutput(t, `{"path":"`+path+`","order":"DESC"}`); got != "c\nb\na" { + t.Errorf(`order:"DESC" = %q, want descending (case-insensitive enum)`, got) + } + // Unknown values: clean error, never a silent ascending sort. + var r struct { + Error string `json:"error"` + } + mustUnmarshal(t, callJSON(t, &sortTool{}, `{"path":"`+path+`","order":"descending"}`), &r) + if !strings.Contains(r.Error, "order") { + t.Errorf(`order:"descending": error = %q, want an invalid-order error`, r.Error) + } +} + +// ── Regression (ToolPreview / extractJSONField) ───────────────────────── +// extractJSONField located values by the literal `"key": "` byte +// sequence, so compact JSON args ({"path":"main.go"} — no space after +// the colon, emitted constantly by models) silently lost their preview. +// The truncation sites then sliced bytes (p[:37]), which split multi-byte +// UTF-8 runes and produced invalid strings for CJK/emoji content. +func TestToolPreview_CompactJSONArgs(t *testing.T) { + if got := render.ToolPreview("read_file", `{"path":"main.go"}`); got != "main.go" { + t.Errorf("compact JSON preview = %q, want %q", got, "main.go") + } + if got := render.ToolPreview("search_files", `{"pattern":"TODO fix"}`); got != "TODO fix" { + t.Errorf("compact JSON preview = %q, want %q", got, "TODO fix") + } + // Spaced form keeps working. + if got := render.ToolPreview("read_file", `{"path": "main.go"}`); got != "main.go" { + t.Errorf("spaced JSON preview = %q, want %q", got, "main.go") + } +} + +func TestToolPreview_TruncationIsValidUTF8(t *testing.T) { + pattern := strings.Repeat("日本語", 20) // 180 bytes / 60 runes + // Spaced form reaches the truncation path pre-compact-fix too. + preview := render.ToolPreview("search_files", `{"pattern": "`+pattern+`"}`) + if !utf8.ValidString(preview) { + t.Errorf("preview is not valid UTF-8 (byte-sliced mid-rune): %q", preview) + } + if strings.ContainsRune(preview, '\uFFFD') { + t.Errorf("preview contains U+FFFD replacement chars: %q", preview) + } + if !strings.HasPrefix(preview, "日本語日本語") { + t.Errorf("preview lost its leading content: %q", preview) + } +} + +// ── Regression (tree on a symlinked directory root) ───────────────────── +// buildTree Lstat'd the root, so a symlinked directory (e.g. /tmp → +// /private/tmp on macOS, or a user's ~/link) was reported as a +// non-directory "file" whose size is the byte length of the link target — +// no children, no error. The explicitly-requested root must be resolved; +// descendants keep Lstat semantics (symlinked entries are shown, never +// followed). +func TestTree_SymlinkedDirRoot(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "inner.txt"), []byte("x"), 0644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "linked") + if err := os.Symlink(dir, link); err != nil { + t.Fatal(err) + } + + var r struct { + Tree struct { + IsDir bool `json:"is_dir"` + FileCount int `json:"file_count"` + Children []struct { + Name string `json:"path"` + } `json:"children"` + } `json:"tree"` + Error string `json:"error"` + } + mustUnmarshal(t, callJSON(t, &treeTool{}, `{"path":"`+link+`"}`), &r) + if r.Error != "" { + t.Fatalf("tree(symlinked dir) error = %q", r.Error) + } + if !r.Tree.IsDir { + t.Errorf("tree(symlinked dir root) is_dir = false, want true (root must resolve)") + } + if len(r.Tree.Children) == 0 { + t.Errorf("tree(symlinked dir root) has no children; the directory was not walked") + } +} + +// ── Regression (directory inputs across sibling tools) ────────────────── +// count_lines rejects directories with a clear "is a directory — use tree" +// error. head_tail and word_count lacked the guard: they surfaced raw +// scanner errors ("cannot read ... is a directory"), and word_count even +// reported the directory's stat size as `bytes` alongside the error. +func TestDirectoryInputs_ConsistentErrors(t *testing.T) { + dir := t.TempDir() + + var ht struct { + Results []struct { + Error string `json:"error"` + } `json:"results"` + } + mustUnmarshal(t, callJSON(t, &headTailTool{}, `{"files":[{"path":"`+dir+`"}]}`), &ht) + if len(ht.Results) != 1 || !strings.Contains(ht.Results[0].Error, "is a directory") { + t.Errorf("head_tail(dir) error = %+v, want a clear is-a-directory error", ht.Results) + } + + var wc struct { + Results []struct { + Error string `json:"error"` + Bytes int64 `json:"bytes"` + } `json:"results"` + } + mustUnmarshal(t, callJSON(t, &wordCountTool{}, `{"files":[{"path":"`+dir+`"}]}`), &wc) + if len(wc.Results) != 1 || !strings.Contains(wc.Results[0].Error, "is a directory") { + t.Errorf("word_count(dir) error = %+v, want a clear is-a-directory error", wc.Results) + } + if wc.Results[0].Bytes != 0 { + t.Errorf("word_count(dir) bytes = %d, want 0 (no misleading stat size next to an error)", wc.Results[0].Bytes) + } +} diff --git a/cmd/odek/serve_runs.go b/cmd/odek/serve_runs.go index 72bebf80..fb89df15 100644 --- a/cmd/odek/serve_runs.go +++ b/cmd/odek/serve_runs.go @@ -72,23 +72,34 @@ func (r *eventsRing) add(ev events.Event) { } // snapshot returns up to limit most-recent events, filtered by run_id / -// session_id when non-empty, oldest-first. +// session_id when non-empty, oldest-first. With more matches than the +// limit, the window is the MOST RECENT matches (previously it walked +// oldest-first and stopped at the limit, serving stale events once the +// ring held more entries than the limit). A limit <= 0 returns all +// matching events, oldest-first. func (r *eventsRing) snapshot(limit int, runID, sessionID string) []events.Event { r.mu.Lock() defer r.mu.Unlock() - out := make([]events.Event, 0, limit) - for _, ev := range r.events { // already oldest-first - if runID != "" && ev.RunID != runID { - continue - } - if sessionID != "" && ev.SessionID != sessionID { - continue - } - out = append(out, ev) - if limit > 0 && len(out) >= limit { - break + + matches := r.events + if runID != "" || sessionID != "" { + matches = make([]events.Event, 0, len(r.events)) + for _, ev := range r.events { + if runID != "" && ev.RunID != runID { + continue + } + if sessionID != "" && ev.SessionID != sessionID { + continue + } + matches = append(matches, ev) } } + + if limit > 0 && len(matches) > limit { + matches = matches[len(matches)-limit:] + } + out := make([]events.Event, len(matches)) + copy(out, matches) // ring may keep appending; don't alias its slice return out } diff --git a/cmd/odek/serve_runs_events_ring_test.go b/cmd/odek/serve_runs_events_ring_test.go new file mode 100644 index 00000000..2171c1fe --- /dev/null +++ b/cmd/odek/serve_runs_events_ring_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "testing" + + "github.com/BackendStack21/odek/internal/events" +) + +// Regression: eventsRing.snapshot documented "up to limit most-recent +// events ... oldest-first" but iterated oldest-first and stopped at the +// limit — returning the OLDEST matches once the ring held more events +// than the limit. /api/events therefore served stale data exactly when +// the ring was under pressure (the only situation where the limit +// matters). The fix walks newest→oldest, keeps the first `limit`, and +// reverses back to oldest-first. +func TestEventsRingSnapshot_MostRecentWindow(t *testing.T) { + ring := &eventsRing{} + for i := 0; i < 10; i++ { + ring.add(events.Event{Type: "probe", Iteration: i}) + } + + got := ring.snapshot(3, "", "") + if len(got) != 3 { + t.Fatalf("len = %d, want 3", len(got)) + } + // The 3 most recent events are Iterations 7,8,9; oldest-first order. + if got[0].Iteration != 7 || got[1].Iteration != 8 || got[2].Iteration != 9 { + t.Fatalf("snapshot(limit=3) of 10 events = iterations %d,%d,%d; want 7,8,9 (most-recent window, oldest-first)", + got[0].Iteration, got[1].Iteration, got[2].Iteration) + } +} + +// Same contract under a filter: with more matching events than the limit, +// the window must be the most recent matches, still oldest-first. +func TestEventsRingSnapshot_MostRecentWindowFiltered(t *testing.T) { + ring := &eventsRing{} + for i := 0; i < 6; i++ { + runID := "run-old" + if i >= 4 { + runID = "run-new" // only events 4 and 5 match + } + ring.add(events.Event{Type: "probe", RunID: runID, Iteration: i}) + } + + got := ring.snapshot(1, "run-new", "") + if len(got) != 1 { + t.Fatalf("len = %d, want 1", len(got)) + } + if got[0].Iteration != 5 { + t.Fatalf("filtered snapshot(limit=1) = iteration %d; want 5 (most recent match)", got[0].Iteration) + } +} + +// Unbounded snapshot (limit <= 0) keeps returning the whole ring, +// oldest-first. +func TestEventsRingSnapshot_UnboundedStillOldestFirst(t *testing.T) { + ring := &eventsRing{} + for i := 0; i < 4; i++ { + ring.add(events.Event{Type: "probe", Iteration: i}) + } + got := ring.snapshot(0, "", "") + if len(got) != 4 || got[0].Iteration != 0 || got[3].Iteration != 3 { + t.Fatalf("unbounded snapshot = len %d [%d..%d]; want 4 events 0..3 oldest-first", + len(got), got[0].Iteration, got[3].Iteration) + } +} diff --git a/internal/render/render.go b/internal/render/render.go index 5a9b0b00..9a06afab 100644 --- a/internal/render/render.go +++ b/internal/render/render.go @@ -17,11 +17,13 @@ package render import ( + "encoding/json" "fmt" "io" "os" "strings" "time" + "unicode/utf8" ) // ── Events ──────────────────────────────────────────────────────────── @@ -270,36 +272,24 @@ func ToolPreview(name, args string) string { return "file" case "search_files": if p := extractJSONField(args, "pattern"); p != "" { - if len(p) > 40 { - p = p[:37] + "..." - } - return p + return truncateRunes(p, 40) } return "" case "shell", "terminal": if cmd := extractJSONField(args, "command"); cmd != "" { - if len(cmd) > 60 { - cmd = cmd[:57] + "..." - } - return cmd + return truncateRunes(cmd, 60) } return "" case "batch_read", "batch_patch", "parallel_shell": return "" case "http_batch", "browser": if u := extractJSONField(args, "url"); u != "" { - if len(u) > 60 { - u = u[:57] + "..." - } - return u + return truncateRunes(u, 60) } return "" case "memory": if q := extractJSONField(args, "query"); q != "" { - if len(q) > 40 { - q = q[:37] + "..." - } - return q + return truncateRunes(q, 40) } return "" case "transcribe": @@ -312,18 +302,12 @@ func ToolPreview(name, args string) string { return "" case "send_message": if t := extractJSONField(args, "text"); t != "" { - if len(t) > 60 { - t = t[:57] + "..." - } - return t + return truncateRunes(t, 60) } return "" case "session_search": if q := extractJSONField(args, "query"); q != "" { - if len(q) > 40 { - q = q[:37] + "..." - } - return q + return truncateRunes(q, 40) } return "" } @@ -388,16 +372,72 @@ func truncateWords(s string, maxWords int) string { return strings.Join(words[:maxWords], " ") + "…" } -// extractJSONField extracts the value of a top-level string field from a JSON blob. +// extractJSONField extracts the value of a top-level string field from a +// JSON blob. It handles both spaced and compact forms (`"key": "v"` and +// `"key":"v"` — models emit both) and returns the DECODED value with +// escapes resolved, not the raw JSON source. Non-string values and +// malformed JSON yield "". func extractJSONField(jsonStr, field string) string { - prefix := `"` + field + `": "` - if idx := strings.Index(jsonStr, prefix); idx >= 0 { - rest := jsonStr[idx+len(prefix):] - if end := strings.Index(rest, `"`); end >= 0 { - return rest[:end] + key := `"` + field + `"` + rest := jsonStr + for { + idx := strings.Index(rest, key) + if idx < 0 { + return "" + } + // The match must start a key: preceded by '{', ',', whitespace, or + // nothing — otherwise it is the tail of a longer key ("sub_path"). + if idx > 0 { + p := rest[idx-1] + if p != '{' && p != ',' && p != ' ' && p != '\t' && p != '\n' && p != '\r' { + rest = rest[idx+len(key):] + continue + } + } + rest = rest[idx+len(key):] + rest = rest[leadingSpace(rest):] + if len(rest) == 0 || rest[0] != ':' { + continue // not a key:value pair + } + rest = rest[1:] + rest = rest[leadingSpace(rest):] + if len(rest) == 0 || rest[0] != '"' { + continue // value is not a string } + var out string + if err := json.NewDecoder(strings.NewReader(rest)).Decode(&out); err != nil { + continue + } + return out } - return "" +} + +// leadingSpace returns the number of leading JSON whitespace bytes. +func leadingSpace(s string) int { + i := 0 + for i < len(s) { + switch s[i] { + case ' ', '\t', '\n', '\r': + i++ + default: + return i + } + } + return i +} + +// truncateRunes limits s to max runes, appending "..." when trimmed. The +// previous byte-slicing split multi-byte runes mid-sequence and produced +// invalid UTF-8 (mojibake) for CJK/emoji content. +func truncateRunes(s string, max int) string { + if max < 4 { + max = 4 + } + if utf8.RuneCountInString(s) <= max { + return s + } + r := []rune(s) + return string(r[:max-3]) + "..." } // toolEmoji returns an emoji that visually signals the tool category. diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 8d9a2c34..de6c1579 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -152,7 +152,9 @@ func ParseRefs(text string) []Ref { if text[i] != '@' { continue } - // Must have a non-@ character after + // Must have a non-@ character after; a doubled "@@" means the first @ + // does not start a ref — the second one does (pinned by + // TestParseRefs_DoubleAt: "@@world" yields the ref "@world"). if i+1 >= len(text) || text[i+1] == '@' { continue } diff --git a/internal/skills/loader.go b/internal/skills/loader.go index a9524b36..50064446 100644 --- a/internal/skills/loader.go +++ b/internal/skills/loader.go @@ -68,7 +68,7 @@ func parseSkillContent(content, sourcePath string) *Skill { return nil } - name, _ := fm["name"].(string) + name := fmString(fm, "name") if name == "" { return nil } @@ -76,9 +76,9 @@ func parseSkillContent(content, sourcePath string) *Skill { return nil // reject names with path traversal at load time } - desc, _ := fm["description"].(string) - version, _ := fm["version"].(string) - author, _ := fm["author"].(string) + desc := fmString(fm, "description") + version := fmString(fm, "version") + author := fmString(fm, "author") // Parse odek section var trigger SkillTrigger @@ -88,11 +88,9 @@ func parseSkillContent(content, sourcePath string) *Skill { var provenance SkillProvenance if odek, ok := fm["odek"].(map[string]any); ok { if t, ok := odek["trigger"].(map[string]any); ok { - topic, _ := t["topic"].(string) - action, _ := t["action"].(string) trigger = SkillTrigger{ - TopicKeywords: splitKeywords(topic), - ActionKeywords: splitKeywords(action), + TopicKeywords: splitKeywords(fmKeyString(t, "topic")), + ActionKeywords: splitKeywords(fmKeyString(t, "action")), } } if al, ok := odek["auto_load"].(bool); ok { @@ -108,8 +106,8 @@ func parseSkillContent(content, sourcePath string) *Skill { if nr, ok := p["needs_review"].(bool); ok { provenance.NeedsReview = nr } - if src, ok := p["sources"].(string); ok { - provenance.Sources = splitKeywords(src) + if src, ok := p["sources"]; ok { + provenance.Sources = splitKeywords(fmScalarString(src)) } } } else { @@ -206,6 +204,42 @@ func parseYAMLMap(s string) map[string]any { return result } +// fmString reads a top-level frontmatter key as its string form. YAML +// scalars are type-inferred by parseYAMLValue, so `version: 1.2` arrives +// as float64 and `version: 2` as int — a bare .(string) assertion silently +// dropped such values. Scalar forms are coerced to their canonical string +// (1.0 → "1"); maps/slices still yield "". +func fmString(fm map[string]any, key string) string { + return fmScalarString(fm[key]) +} + +// fmKeyString is fmString for a nested frontmatter map (e.g. trigger +// topic/action inside odek.trigger). +func fmKeyString(m map[string]any, key string) string { + return fmScalarString(m[key]) +} + +func fmScalarString(v any) string { + switch s := v.(type) { + case string: + return s + case int: + return strconv.Itoa(s) + case int64: + return strconv.FormatInt(s, 10) + case float64: + // Shortest representation that round-trips: 1.2 → "1.2", 1.0 → "1". + return strconv.FormatFloat(s, 'f', -1, 64) + case bool: + if s { + return "true" + } + return "false" + default: + return "" + } +} + // parseYAMLValue converts a string to its inferred Go type. func parseYAMLValue(s string) any { // Bool diff --git a/internal/skills/loader_frontmatter_scalars_test.go b/internal/skills/loader_frontmatter_scalars_test.go new file mode 100644 index 00000000..5c877a55 --- /dev/null +++ b/internal/skills/loader_frontmatter_scalars_test.go @@ -0,0 +1,68 @@ +package skills + +import "testing" + +// Regression: parseYAMLValue type-infers scalars, so `version: 1.2` +// became float64(1.2) and `version: 2` became int(2) — both then fell +// through parseSkillContent's bare .(string) assertions and were +// silently dropped (Version = ""). The same drop hit numeric trigger +// topics and provenance sources. String-typed frontmatter fields must +// coerce scalar values (string, int, float64, bool) to their canonical +// string form instead of vanishing. +func TestParseSkillContent_NumericVersionCoerced(t *testing.T) { + cases := []struct { + fm string + want string + }{ + {"version: 1.2", "1.2"}, + {"version: 2", "2"}, + {`version: "1.2"`, "1.2"}, // quoted form already worked + {"version: 1.0", "1"}, + } + for _, c := range cases { + content := "---\nname: probe-skill\ndescription: d\n" + c.fm + "\n---\nBody." + s := parseSkillContent(content, "probe") + if s == nil { + t.Fatalf("%s: skill = nil", c.fm) + } + if s.Version != c.want { + t.Errorf("%s: Version = %q, want %q", c.fm, s.Version, c.want) + } + } +} + +func TestParseSkillContent_NumericAuthorCoerced(t *testing.T) { + content := "---\nname: probe-skill\ndescription: d\nauthor: 7\n---\nBody." + s := parseSkillContent(content, "probe") + if s == nil { + t.Fatal("skill = nil") + } + if s.Author != "7" { + t.Errorf("author: 7 → Author = %q, want \"7\"", s.Author) + } +} + +func TestParseSkillContent_NumericTriggerTopicCoerced(t *testing.T) { + content := "---\nname: probe-skill\ndescription: d\nodek:\n trigger:\n topic: 2\n---\nBody." + s := parseSkillContent(content, "probe") + if s == nil { + t.Fatal("skill = nil") + } + if len(s.Trigger.TopicKeywords) == 0 || s.Trigger.TopicKeywords[0] != "2" { + t.Errorf("topic: 2 → TopicKeywords = %v, want [\"2\"]", s.Trigger.TopicKeywords) + } +} + +func TestParseSkillContent_BoolAndFloatFormsCoerced(t *testing.T) { + content := "---\nname: probe-skill\ndescription: 3.14\nodek:\n auto_load: true\n trigger:\n action: 404\n---\nBody." + s := parseSkillContent(content, "probe") + if s == nil { + t.Fatal("skill = nil") + } + if s.Description != "3.14" { + t.Errorf("description: 3.14 → Description = %q, want \"3.14\"", s.Description) + } + if len(s.Trigger.ActionKeywords) == 0 || s.Trigger.ActionKeywords[0] != "404" { + t.Errorf("action: 404 → ActionKeywords = %v, want [\"404\"]", s.Trigger.ActionKeywords) + } +} From 75fc1fd7820be947fe3653de5b77e2b7d832756a Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Fri, 28 Aug 2026 07:56:08 +0200 Subject: [PATCH 2/2] fix(serve): make wsWriteTimeout race-free under CI's -race run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught a data race in TestAudit_WriteWSJSONStalledClientBounded: the test retunes the package-global wsWriteTimeout while live WS write paths (including in-flight goroutines surviving earlier tests) read it inside writeWSJSON's select — an unsynchronized read/write of a plain time.Duration. Convert wsWriteTimeout to atomic.Int64 (Store at init, Load in the write watchdog) and update the test to Load/Store. Behavior unchanged; the memory-model violation is gone regardless of detector scheduling. --- cmd/odek/audit_serve_test.go | 6 +++--- cmd/odek/serve.go | 12 ++++++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/cmd/odek/audit_serve_test.go b/cmd/odek/audit_serve_test.go index e5e42324..29fcc344 100644 --- a/cmd/odek/audit_serve_test.go +++ b/cmd/odek/audit_serve_test.go @@ -198,9 +198,9 @@ func TestAudit_WriteWSJSONStalledClientBounded(t *testing.T) { Handshake: func(*golangws.Config, *http.Request) error { return nil }, Handler: func(conn *golangws.Conn) { defer conn.Close() - old := wsWriteTimeout - wsWriteTimeout = 300 * time.Millisecond - defer func() { wsWriteTimeout = old }() + old := wsWriteTimeout.Load() + wsWriteTimeout.Store(int64(300 * time.Millisecond)) + defer func() { wsWriteTimeout.Store(old) }() writeWSJSON(conn, map[string]any{"type": "token", "content": strings.Repeat("x", 16<<20)}) close(done) }, diff --git a/cmd/odek/serve.go b/cmd/odek/serve.go index b58e4856..b27a8746 100644 --- a/cmd/odek/serve.go +++ b/cmd/odek/serve.go @@ -2023,9 +2023,13 @@ func validateSessionToken(store *session.Store, sess *session.Session, token str // holder with --stream enabled) fills its TCP receive window and blocks // Send forever; the write previously held the process-wide mutex while // doing so, freezing every connection's writes — approval prompts and -// pongs included — until the wedged client drained (2026-08 audit). A var -// so tests can shrink it. -var wsWriteTimeout = 30 * time.Second +// pongs included — until the wedged client drained (2026-08 audit). A +// var so tests can shrink it. Atomic access: tests retune it while live +// WS goroutines (including in-flight ones from earlier tests) read it +// concurrently — a plain read/write here is a data race (caught by CI). +var wsWriteTimeout atomic.Int64 + +func init() { wsWriteTimeout.Store(int64(30 * time.Second)) } // wsConnWriters gives each connection its own write lock. // golang.org/x/net/websocket is not safe for concurrent Sends, and frames @@ -2073,7 +2077,7 @@ func writeWSJSON(conn *golangws.Conn, data any) { }() select { case <-done: - case <-time.After(wsWriteTimeout): + case <-time.After(time.Duration(wsWriteTimeout.Load())): // The client stopped reading: its TCP receive window is full and // Send is wedged. Abandon the write (bounded caller, per-conn // lock released, later sends on this conn fast-fail) and tear the