Merge origin/master into v1: corrected TestPinAndUnpin, task 02 replacement fixtures

2026-09-25 04:24:55 -07:00
parent 7e0dbb4a6f 29b3a5c38e
commit e65826b369
6 changed files with 242 additions and 4 deletions
@@ -21,8 +21,11 @@ implementing it one task at a time.
 ## Files you must never edit

 - `PLAN.md`, `docs/plans/`, `AGENTS.md`
- Anything a task told you to copy from `docs/plans/**/_files/`: tests, testdata, `Makefile`,
-  scripts, `example.toml`, `cmd/fakeupstream`. If a copied test fails, your code is wrong.
+- Anything a task told you to copy from `docs/plans/**/_files/` — in this plan **or any earlier
+  one** — stays protected: tests, testdata, `Makefile`,
+  scripts, `example.toml`, `cmd/fakeupstream`. If a copied test fails, your code is wrong. If a
+  copied test can no longer be right because the new task changes what it tested, that is the
+  owner's error: stop and report it; the owner hands over the replacement.

 ## Code rules

@@ -20,6 +20,8 @@ requests fall back to the route-level lease (task 04).
 ## Files

 - Copy: `internal/fingerprint/fingerprint_test.go`, `internal/config/config_v1_test.go`
+- Copy (**replaces** v0's): `internal/config/config_test.go`, `internal/config/testdata/bad-unknown-key.toml`
+  (v0's unknown-key example was `lease_idle`, which this task makes valid; the replacements use `bogus_key`)
 - Create: `internal/fingerprint/fingerprint.go`
 - Modify: `internal/config/config.go`, `docs/implementer-log.md`

@@ -67,3 +67,19 @@ tools/run-plan.sh docs/plans/v1          # from a clean checkout on master
  not edit. Ornith diagnosed it (gofmt on its own code was clean) and did not touch them.
  Owner's fault (the compile check ran `go vet`, not `gofmt`, on the given files). Fixed by
  formatting the given files; the plan checklist now includes `gofmt -l docs/` before handover.
+  Ornith stopped correctly (a `stopped` row, code left in the tree, only the log committed);
+  a second session was told where the first had stopped and finished steps 6–7 without
+  starting over — the boxmaker precedent (`M3a/19`). The driver was then resumed from task 02.
+- 2026-09-25, task 02: v0's given `testdata/bad-unknown-key.toml` used `lease_idle` as its
+  unknown-key example, and v1 makes `lease_idle` a real key, so v0's `TestBadFiles` broke — the
+  task did not hand over replacements for the two v0 given files (tip T19). Ornith changed the
+  example to `bogus_key` in both files and logged it in Deviations; the content is right, but the
+  files were protected and the rule was to stop. Owner's fault for the conflict; the model's
+  deviation is noted. The corrected files now sit in `_files/internal/config/` as the reference
+  copies for the reviewer's byte-exact check.
+- 2026-09-25, task 04: my `TestPinAndUnpin` asserted the pin event at exactly `len-3` and the
+  release event at `len-1`, but the task's own rules make acquires under a pin record events too,
+  so a faithful implementation produces `[new pin pin new new release]` and the assertion cannot
+  hold. Ornith spent its first ten minutes puzzling over exactly that. Test fault (mine): the
+  assertion now checks order and content (a pin event naming beta, followed later by a release),
+  not positions.
@@ -0,0 +1,191 @@
+package config_test
+
+import (
+	"fmt"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"git.wntrmute.dev/kyle/crossbar/internal/config"
+)
+
+func TestGoodFile(t *testing.T) {
+	c, err := config.Load(filepath.Join("testdata", "good.toml"))
+	if err != nil {
+		t.Fatalf("Load: %v", err)
+	}
+	if c.Listen != "100.64.0.9:7777" {
+		t.Errorf("Listen = %q", c.Listen)
+	}
+	if c.PollInterval.Duration != 5*time.Second {
+		t.Errorf("PollInterval = %v", c.PollInterval.Duration)
+	}
+	if c.QueueMax != 4 {
+		t.Errorf("QueueMax = %d", c.QueueMax)
+	}
+	alpha := c.Hosts["alpha"]
+	if alpha.BaseURL != "http://alpha.example:11434" {
+		t.Errorf("trailing slash not stripped: %q", alpha.BaseURL)
+	}
+	if alpha.Weight != 2 {
+		t.Errorf("alpha.Weight = %v", alpha.Weight)
+	}
+	if alpha.Models["ornith-1.5-35b-a3b"].Parallel != 4 || alpha.Models["small-9b"].Parallel != 6 {
+		t.Errorf("alpha.Models = %+v", alpha.Models)
+	}
+	beta := c.Hosts["beta"]
+	if beta.Weight != 1 {
+		t.Errorf("beta.Weight default = %v, want 1", beta.Weight)
+	}
+	if beta.Models["ornith-1.5-35b-a3b"].Parallel != 1 {
+		t.Errorf("beta parallel default = %d, want 1", beta.Models["ornith-1.5-35b-a3b"].Parallel)
+	}
+	r := c.Routes["opencode-a"]
+	if len(r.Hosts) != 2 || r.Hosts[0] != "alpha" || r.Hosts[1] != "beta" {
+		t.Errorf("route hosts = %v", r.Hosts)
+	}
+	if r.DefaultModel != "ornith-1.5-35b-a3b" {
+		t.Errorf("DefaultModel = %q", r.DefaultModel)
+	}
+	if c.Routes["hermes-x"].DefaultModel != "" {
+		t.Errorf("hermes-x DefaultModel should be empty")
+	}
+	if !c.Serves("alpha", "small-9b") || c.Serves("beta", "small-9b") || c.Serves("nope", "m") {
+		t.Errorf("Serves is wrong")
+	}
+}
+
+func TestDefaults(t *testing.T) {
+	c, err := config.Parse(strings.NewReader(`
+listen = "127.0.0.1:1"
+[hosts.a]
+base_url = "http://a:1"
+models = { "m" = { } }
+[routes.r]
+hosts = ["a"]
+`))
+	if err != nil {
+		t.Fatalf("Parse: %v", err)
+	}
+	if c.PollInterval.Duration != config.DefaultPollInterval {
+		t.Errorf("PollInterval default = %v", c.PollInterval.Duration)
+	}
+	if c.QueueMax != config.DefaultQueueMax {
+		t.Errorf("QueueMax default = %d", c.QueueMax)
+	}
+}
+
+func TestBadFiles(t *testing.T) {
+	cases := []struct{ file, field string }{
+		{"bad-listen.toml", "listen"},
+		{"bad-unknown-host.toml", "routes.r.hosts"},
+		{"bad-default-model.toml", "routes.r.default_model"},
+		{"bad-unknown-key.toml", "bogus_key"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.file, func(t *testing.T) {
+			_, err := config.Load(filepath.Join("testdata", tc.file))
+			if err == nil {
+				t.Fatalf("want error")
+			}
+			e, ok := config.IsError(err)
+			if !ok {
+				t.Fatalf("want *config.Error, got %T: %v", err, err)
+			}
+			if e.Field != tc.field {
+				t.Errorf("Field = %q, want %q (%v)", e.Field, tc.field, err)
+			}
+			if !strings.HasPrefix(err.Error(), "config: "+tc.field+": ") {
+				t.Errorf("Error() = %q", err.Error())
+			}
+		})
+	}
+}
+
+func TestBadValues(t *testing.T) {
+	base := `
+listen = %q
+poll_interval = %q
+[hosts.a]
+base_url = %q
+weight = %v
+models = { "m" = { parallel = %d } }
+[routes.%s]
+hosts = ["a"]
+`
+	cases := []struct {
+		name                     string
+		listen, poll, url, route string
+		weight                   float64
+		parallel                 int
+		field                    string
+	}{
+		{"empty listen", "", "5s", "http://a:1", "r", 1, 1, "listen"},
+		{"no port", "127.0.0.1", "5s", "http://a:1", "r", 1, 1, "listen"},
+		{"v6 any", "[::]:7", "5s", "http://a:1", "r", 1, 1, "listen"},
+		{"poll too short", "127.0.0.1:7", "500ms", "http://a:1", "r", 1, 1, "poll_interval"},
+		{"ftp url", "127.0.0.1:7", "5s", "ftp://a:1", "r", 1, 1, "hosts.a.base_url"},
+		{"no host", "127.0.0.1:7", "5s", "http://", "r", 1, 1, "hosts.a.base_url"},
+		{"query", "127.0.0.1:7", "5s", "http://a:1/v1?x=1", "r", 1, 1, "hosts.a.base_url"},
+		{"negative weight", "127.0.0.1:7", "5s", "http://a:1", "r", -1, 1, "hosts.a.weight"},
+		{"negative parallel", "127.0.0.1:7", "5s", "http://a:1", "r", 1, -2, "hosts.a.models.m.parallel"},
+		{"route name", "127.0.0.1:7", "5s", "http://a:1", "Bad_Name", 1, 1, "routes.Bad_Name"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			text := fmt.Sprintf(base, tc.listen, tc.poll, tc.url, tc.weight, tc.parallel, tc.route)
+			_, err := config.Parse(strings.NewReader(text))
+			if err == nil {
+				t.Fatalf("want error for %s", tc.name)
+			}
+			e, ok := config.IsError(err)
+			if !ok {
+				t.Fatalf("want *config.Error, got %T: %v", err, err)
+			}
+			if e.Field != tc.field {
+				t.Errorf("Field = %q, want %q (%v)", e.Field, tc.field, err)
+			}
+		})
+	}
+}
+
+func TestMissingSections(t *testing.T) {
+	for _, tc := range []struct{ name, text, field string }{
+		{"no hosts", "listen = \"127.0.0.1:7\"\n[routes.r]\nhosts = [\"a\"]\n", "hosts"},
+		{"no routes", "listen = \"127.0.0.1:7\"\n[hosts.a]\nbase_url = \"http://a:1\"\nmodels = { \"m\" = { } }\n", "routes"},
+		{"host without models", "listen = \"127.0.0.1:7\"\n[hosts.a]\nbase_url = \"http://a:1\"\n[routes.r]\nhosts = [\"a\"]\n", "hosts.a.models"},
+		{"route without hosts", "listen = \"127.0.0.1:7\"\n[hosts.a]\nbase_url = \"http://a:1\"\nmodels = { \"m\" = { } }\n[routes.r]\n", "routes.r.hosts"},
+		{"host twice", "listen = \"127.0.0.1:7\"\n[hosts.a]\nbase_url = \"http://a:1\"\nmodels = { \"m\" = { } }\n[routes.r]\nhosts = [\"a\", \"a\"]\n", "routes.r.hosts"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := config.Parse(strings.NewReader(tc.text))
+			e, ok := config.IsError(err)
+			if !ok {
+				t.Fatalf("want *config.Error, got %v", err)
+			}
+			if e.Field != tc.field {
+				t.Errorf("Field = %q, want %q", e.Field, tc.field)
+			}
+		})
+	}
+}
+
+func TestNotTOML(t *testing.T) {
+	_, err := config.Parse(strings.NewReader("listen = [unterminated"))
+	if err == nil {
+		t.Fatal("want error")
+	}
+	if _, ok := config.IsError(err); ok {
+		t.Errorf("a syntax error is not a validation Error")
+	}
+	if !strings.HasPrefix(err.Error(), "config: ") {
+		t.Errorf("Error() = %q", err.Error())
+	}
+}
+
+func TestMissingFile(t *testing.T) {
+	if _, err := config.Load(filepath.Join("testdata", "does-not-exist.toml")); err == nil {
+		t.Fatal("want error")
+	}
+}
@@ -0,0 +1,9 @@
+listen = "127.0.0.1:7777"
+bogus_key = 1
+
+[hosts.alpha]
+base_url = "http://alpha.example:11434"
+models   = { "m" = { } }
+
+[routes.r]
+hosts = ["alpha"]
@@ -217,9 +217,26 @@ func TestPinAndUnpin(t *testing.T) {
 	if host, _, _ := tbl.Acquire(k, []string{"alpha", "beta"}, t0.Add(4*time.Minute)); host != "beta" {
 		t.Errorf("after unpin the existing lease (on beta) simply continues: %q", host)
 	}
-	if got := p.reasons(); got[len(got)-3] != store.ReasonPin || got[len(got)-1] != store.ReasonRelease {
-		t.Errorf("events = %v, want a pin event and a release event", got)
+	// Events: a pin event naming beta must exist, and the unpin's release event must come after it.
+	// Acquires under the pin may record their own events in between; their number is not fixed here.
+	got := p.reasons()
+	pinAt, releaseAt := -1, -1
+	for i, r := range got {
+		if r == store.ReasonPin && pinAt < 0 {
+			pinAt = i
+		}
+		if r == store.ReasonRelease {
+			releaseAt = i
+		}
 	}
+	if pinAt < 0 || releaseAt < pinAt {
+		t.Errorf("events = %v, want a pin event followed later by a release event", got)
+	}
+	p.mu.Lock()
+	if pinAt >= 0 && p.events[pinAt].ToHost != "beta" {
+		t.Errorf("pin event = %+v, want ToHost beta", p.events[pinAt])
+	}
+	p.mu.Unlock()
 }

 func TestDrainKeepsExistingRefusesNew(t *testing.T) {