fix: m2 wave-3 bug sweep — JSON-decoded narrate paths, expandEnv tail preservation, strict numeric flags (#192)

Three RED-first fixes, each pinned by failing tests on main:

1. narrate (LOW): extractPath scanned for the first quote pair instead of
   decoding JSON — an escaped quote inside a path truncated it
   ("we\"ird.go" → "we\"). extractShell was fixed for exactly this
   failure; extractPath now decodes JSON first with the same best-effort
   scan fallback.

2. config (MED): an unterminated ${VAR reference consumed the ENTIRE
   remaining config value — "a${b" expanded to "a$" — silent
   config-value corruption (bare $ references preserved their tails, ${
   did not). Unterminated and empty (${) braces now emit verbatim.

3. cmd/odek (LOW-MED): --max-iter, --thinking-budget, and --temperature
   used unchecked fmt.Sscanf — "abc" was silently ignored
   (--temperature even overwrote the value with 0) and "1800junk" was
   accepted as 1800, hiding typos from the operator. All three now use
   strconv and reject garbage, matching every sibling numeric flag
   (--max-runtime, --max-tool-calls, ...). The pinned
   TestParseRunFlags_MaxIterNonNumeric documented the Sscanf quirk as
   intended; its contract is updated to the strict behavior with a
   comment explaining the change.

Author: jkyberneees

cmd/odek/main.go

@@ -9,6 +9,7 @@ import (
 	"os/signal"
 	"path/filepath"
 	"runtime/debug"
+	"strconv"
 	"strings"
 	"sync/atomic"
 	"time"
@@ -469,8 +470,10 @@ func parseRunFlags(args []string) (runFlags, error) {
 			if i+1 >= len(args) {
 				return f, fmt.Errorf("--max-iter requires a value")
 			}
-			var n int
-			fmt.Sscanf(args[i+1], "%d", &n)
+			n, err := strconv.Atoi(args[i+1])
+			if err != nil {
+				return f, fmt.Errorf("--max-iter requires an integer, got %q", args[i+1])
+			}
 			if n > 0 {
 				f.MaxIter = n
 			}
@@ -491,14 +494,20 @@ func parseRunFlags(args []string) (runFlags, error) {
 			if i+1 >= len(args) {
 				return f, fmt.Errorf("--thinking-budget requires a value")
 			}
-			fmt.Sscanf(args[i+1], "%d", &f.ThinkingBudget)
+			n, err := strconv.Atoi(args[i+1])
+			if err != nil {
+				return f, fmt.Errorf("--thinking-budget requires an integer, got %q", args[i+1])
+			}
+			f.ThinkingBudget = n
 			i += 2
 		case "--temperature":
 			if i+1 >= len(args) {
 				return f, fmt.Errorf("--temperature requires a value")
 			}
-			var t float64
-			fmt.Sscanf(args[i+1], "%f", &t)
+			t, err := strconv.ParseFloat(args[i+1], 64)
+			if err != nil {
+				return f, fmt.Errorf("--temperature requires a number, got %q", args[i+1])
+			}
 			f.Temp = t
 			i += 2
 		case "--sandbox":

cmd/odek/main_test.go

@@ -574,16 +574,13 @@ func captureStdout(fn func()) string {
 
 // Test parseRunFlags with a non-numeric --max-iter value.
 func TestParseRunFlags_MaxIterNonNumeric(t *testing.T) {
-	f, err := parseRunFlags([]string{"--max-iter", "abc", "task"})
-	if err != nil {
-		t.Fatalf("parseRunFlags error: %v", err)
-	}
-	// fmt.Sscanf with non-numeric leaves the zero value (not set)
-	if f.MaxIter != 0 {
-		t.Errorf("MaxIter = %d, want 0 (non-numeric should leave zero)", f.MaxIter)
-	}
-	if f.Task != "task" {
-		t.Errorf("Task = %q, want %q", f.Task, "task")
+	// Contract changed (m2 wave-3): a non-numeric --max-iter is an error,
+	// matching every sibling numeric flag (--max-runtime, --max-tool-calls,
+	// …). The old behavior — fmt.Sscanf silently ignoring "abc" and the
+	// run proceeding with the default cap — hid typos from the operator.
+	_, err := parseRunFlags([]string{"--max-iter", "abc", "task"})
+	if err == nil {
+		t.Fatal("--max-iter abc must be rejected, not silently ignored")
 	}
 }
 

cmd/odek/run_flags_numeric_test.go

@@ -0,0 +1,31 @@
+package main
+
+import "testing"
+
+// Numeric flags used unchecked fmt.Sscanf: "abc" parsed as 0 silently
+// (--temperature OVERWROTE the value with 0; --max-iter was skipped), and
+// "1800junk" was accepted as 1800. Sibling budget flags check errors;
+// these must too.
+func TestParseRunFlags_NumericFlagsRejectGarbage(t *testing.T) {
+	if _, err := parseRunFlags([]string{"--temperature", "abc", "do-work"}); err == nil {
+		t.Fatal("--temperature abc accepted silently (value would be zeroed)")
+	}
+	if _, err := parseRunFlags([]string{"--max-iter", "abc", "do-work"}); err == nil {
+		t.Fatal("--max-iter abc accepted silently")
+	}
+	if _, err := parseRunFlags([]string{"--max-iter", "1800junk", "do-work"}); err == nil {
+		t.Fatal("--max-iter 1800junk accepted (trailing garbage ignored)")
+	}
+	if _, err := parseRunFlags([]string{"--thinking-budget", "x", "do-work"}); err == nil {
+		t.Fatal("--thinking-budget x accepted silently")
+	}
+
+	// Valid values still parse.
+	f, err := parseRunFlags([]string{"--max-iter", "42", "--temperature", "0.7", "--thinking-budget", "1024", "do-work"})
+	if err != nil {
+		t.Fatalf("valid flags rejected: %v", err)
+	}
+	if f.MaxIter != 42 || f.Temp != 0.7 || f.ThinkingBudget != 1024 {
+		t.Fatalf("valid values: %+v", f)
+	}
+}

internal/config/expandenv_tail_test.go

@@ -0,0 +1,22 @@
+package config
+
+import "testing"
+
+// An unterminated ${VAR consumed the ENTIRE remaining value: "a${b"
+// expanded to "a$" — silent config-value corruption. The '$' plus the
+// rest must survive verbatim (an unterminated brace is not a variable
+// reference).
+func TestExpandEnv_UnterminatedBracePreservesTail(t *testing.T) {
+	cases := map[string]string{
+		"a${b":       "a${b",
+		"pre ${tail": "pre ${tail",
+		"${only":     "${only",
+		"$":          "$",
+		"${}":        "${}",
+	}
+	for in, want := range cases {
+		if got := expandEnv(in); got != want {
+			t.Errorf("expandEnv(%q) = %q, want %q", in, got, want)
+		}
+	}
+}

internal/config/loader.go

@@ -875,10 +875,18 @@ func parseVarName(s string) (string, int) {
 		// ${VAR}
 		for k := 1; k < len(s); k++ {
 			if s[k] == '}' {
+				if k == 1 {
+					// ${} — empty name is not a variable reference; emit
+					// verbatim like an unterminated brace.
+					return "", 0
+				}
 				return s[1:k], k + 1
 			}
 		}
-		return "", len(s) // unterminated — consume everything
+		// Unterminated ${ — not a variable reference: consume only the '$'
+		// (width 0) so the '{' and the rest emit verbatim; consuming the
+		// whole tail silently corrupted config values ("a${b" → "a$").
+		return "", 0
 	}
 	// $VAR or $VAR_NAME123
 	if !isVarStart(s[0]) {

internal/narrate/extractpath_test.go

@@ -0,0 +1,22 @@
+package narrate
+
+import "testing"
+
+// extractPath scanned for the first quote pair instead of decoding JSON —
+// an escaped quote inside the path truncated it (extractShell was fixed
+// for exactly this failure; extractPath wasn't).
+func TestExtractPath_EscapedQuoteInPath(t *testing.T) {
+	got := extractPath(`{"path":"we\"ird.go","line":1}`)
+	if got != `we"ird.go` {
+		t.Fatalf("extractPath = %q, want %q (escaped quotes must decode)", got, `we"ird.go`)
+	}
+}
+
+func TestExtractPath_PlainPathStillWorks(t *testing.T) {
+	if got := extractPath(`{"path":"/a/b/main.go"}`); got != "main.go" {
+		t.Fatalf("plain path: %q, want main.go", got)
+	}
+	if got := extractPath(`{"file":"/tmp/x.txt"}`); got != "x.txt" {
+		t.Fatalf("file key: %q, want x.txt", got)
+	}
+}

internal/narrate/narrate.go

@@ -100,6 +100,26 @@ func truncate(s string, n int) string {
 }
 
 func extractPath(args string) string {
+	// Decode the JSON properly: paths routinely contain escaped quotes
+	// ("we\"ird.go"), and a raw first-quote scan truncates at the escape —
+	// the same failure extractShell was fixed for. Falls back to a
+	// best-effort scan only for non-JSON input.
+	var parsed struct {
+		Path string `json:"path"`
+		File string `json:"file"`
+	}
+	if err := json.Unmarshal([]byte(args), &parsed); err == nil {
+		path := parsed.Path
+		if path == "" {
+			path = parsed.File
+		}
+		if path != "" {
+			if lastSlash := strings.LastIndex(path, "/"); lastSlash >= 0 {
+				path = path[lastSlash+1:]
+			}
+			return path
+		}
+	}
 	for _, key := range []string{`"path"`, `"file"`} {
 		if idx := strings.Index(args, key); idx >= 0 {
 			rest := args[idx+len(key):]