fix: 10 validated bug fixes across serve, skills, render, and perf tools

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)
 			},

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)}
 	}

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)
+	}
+}

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")
+	}
+}

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)
+	}
+}

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