From 9d8c43cd68c960b9a6b8f781684f763e204a4498 Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Mon, 5 Oct 2026 18:16:47 -0400 Subject: fix: shared YAML encoder and journal directory, text cleanup --- FIXES.md | 12 ++++++-- Makefile | 6 +++- internal/admin/api_helpers.go | 12 ++------ internal/admin/api_rooms.go | 12 ++++---- internal/admin/api_rooms_bulk.go | 2 +- internal/admin/yaml_util.go | 21 ++++---------- internal/behavior/roomrefs.go | 16 +++++++---- internal/game/account_txn.go | 3 +- internal/game/harness_test.go | 4 +++ internal/net/web_test.go | 59 ++++++++++++++++++++++++++++++++++++++++ internal/persistence/txn.go | 7 +++++ 11 files changed, 110 insertions(+), 44 deletions(-) create mode 100644 internal/net/web_test.go diff --git a/FIXES.md b/FIXES.md index 169793c..b258eff 100644 --- a/FIXES.md +++ b/FIXES.md @@ -87,6 +87,12 @@ Nothing was staged or committed. The audit artifacts (`AUDIT.md`, `reports/`) ar - **Casino**: the plinko rules text now states the measured per-risk returns rather than only the target RTP. - **Refactor**: the duplicated `countExtraInbound` (game + admin, byte-for-byte) is now one `world.CountExtraInbound`, so the two room-removal paths cannot drift. +## Tranche 5 (this pass) + +- **Refactor**: the three YAML map encoders (`admin.marshalYAML`, `admin.marshalMapYAML`, `behavior.MarshalYAMLMap`) now share one `behavior.MarshalYAML`. +- **Refactor**: the `.journal` path is now `persistence.JournalDir(dataDir)` used by both the game and the admin API, so a future rename cannot leave one writer journaling to a directory startup recovery never scans. +- **Test infrastructure**: added a `make test-race` target (`go test -race ./... -timeout 300s`); the shared game harness now stops the Game owner goroutine on cleanup (previously every harness test leaked one); added the first WebSocket transport tests (`TestWebSocketCheckOrigin` unit-tests the origin boundary with a non-loopback peer, `TestWebSocketUpgradeAccepted` drives the `/ws` upgrade end-to-end). + ## Still open - **Explicit undo snapshot-existence flag** (the current fix safely refuses an intentionally empty before-image). @@ -95,9 +101,9 @@ Nothing was staged or committed. The audit artifacts (`AUDIT.md`, `reports/`) ar - **Persistence** `Recover` mid-game misuse. - **Casino**: craps Fire/ATS settlement deferral. - **Admin**: symlink handling in the raw file API. -- **Structural refactors** remaining from `AUDIT.md` §3–4 (ui.Table shrink consolidation, self-signed cert, YAML encoders, `tableCore` for pai gow, typed `GridConflict.Kind`, `refs` struct in validate). -- **Test infrastructure**: `make test-race`/CI, harness owner-goroutine leak, master-tick harness option, and coverage for steal/gather/burn/talk and the WebSocket transport. +- **Structural refactors** remaining from `AUDIT.md` §3–4 (`ui.Table` shrink consolidation, self-signed cert, `tableCore` for pai gow, typed `GridConflict.Kind`, `refs` struct in validate, `remapInt`/`remapScalar`). +- **Test infrastructure**: CI wiring for `test-race`, a master-tick harness option, and coverage for steal/gather/burn/talk. ## Suggested next step -Tranche 5: the remaining structural refactors (ui.Table, cert, YAML encoders, `tableCore` for pai gow, typed `GridConflict.Kind`), the shared vocabularies for craft/safespot validation, and the test-infrastructure work. Keep `make lint`, `go test ./...`, and `go test -race` green. +Tranche 6: the remaining structural refactors (`ui.Table`, cert, `tableCore` for pai gow, typed `GridConflict.Kind`) and the shared vocabularies for craft/safespot validation. Keep `make lint`, `go test ./...`, and `go test -race` green. diff --git a/Makefile b/Makefile index 7fbab5c..98c4805 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build run test vet lint clean +.PHONY: build run test test-race vet lint clean BINARY=thoi DATA_DIR=$(PWD)/data @@ -12,6 +12,10 @@ run: build test: DATA_DIR=$(DATA_DIR) go test ./... -timeout 60s +# Race suite: the concurrency tests are only meaningful with -race. +test-race: + DATA_DIR=$(DATA_DIR) go test -race ./... -timeout 300s + vet: go vet ./... diff --git a/internal/admin/api_helpers.go b/internal/admin/api_helpers.go index d5e23c3..2aa75f9 100644 --- a/internal/admin/api_helpers.go +++ b/internal/admin/api_helpers.go @@ -8,6 +8,7 @@ import ( "gopkg.in/yaml.v3" + "thehouseoficarus/internal/behavior" "thehouseoficarus/internal/persistence" "thehouseoficarus/internal/world" ) @@ -121,14 +122,5 @@ func writeMapAsYAML(path string, m map[string]any) ([]byte, error) { // marshalMapYAML serializes a generic map without writing: callers that // only need the after-image (mutation-plan preflight) use this. func marshalMapYAML(m map[string]any) ([]byte, error) { - var buf bytes.Buffer - enc := yaml.NewEncoder(&buf) - enc.SetIndent(2) - if err := enc.Encode(m); err != nil { - return nil, err - } - if err := enc.Close(); err != nil { - return nil, err - } - return buf.Bytes(), nil + return behavior.MarshalYAMLMap(m) } diff --git a/internal/admin/api_rooms.go b/internal/admin/api_rooms.go index 76db231..06e736c 100644 --- a/internal/admin/api_rooms.go +++ b/internal/admin/api_rooms.go @@ -726,7 +726,7 @@ func (s *AdminServer) handleRoomLink(w http.ResponseWriter, r *http.Request) { // Both sides are resolved and serialized before any write, and both // are applied in one journaled transaction so a failure cannot leave // an asymmetric link, and one Undo reverses the whole action. - txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(s.dataDir)) if err := txn.Replace(pathA, newA, 0644); err != nil { writeJSONError(w, "write A: "+err.Error(), http.StatusInternalServerError) return @@ -868,7 +868,7 @@ func (s *AdminServer) handleRoomLink(w http.ResponseWriter, r *http.Request) { } if len(writes) > 0 { - txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(s.dataDir)) for _, wr := range writes { if err := txn.Replace(wr.path, wr.data, 0644); err != nil { writeJSONError(w, "plan unlink: "+err.Error(), http.StatusInternalServerError) @@ -932,7 +932,7 @@ func (s *AdminServer) handleRoomMove(w http.ResponseWriter, r *http.Request) { } // Move the file in one journaled transaction: a failed destination write // or source removal must not leave the same room ID in two directories. - txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(s.dataDir)) if err := txn.Move(oldPath, newPath); err != nil { failJSON(w, "plan move: "+err.Error()) return @@ -1054,7 +1054,7 @@ func (s *AdminServer) handleRoomRename(w http.ResponseWriter, r *http.Request) { } // ---- Apply the file changes as one journal transaction. ---- - txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(s.dataDir)) if err := txn.Move(oldPath, newPath); err != nil { writeJSONError(w, "plan move: "+err.Error(), http.StatusInternalServerError) return @@ -1240,7 +1240,7 @@ func (s *AdminServer) handleRoomSwap(w http.ResponseWriter, r *http.Request) { // Apply both files in one journaled transaction so a failure cannot leave // the two rooms half-swapped. - txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(s.dataDir)) if err := txn.Replace(aPath, newAContent, 0644); err != nil { writeJSONError(w, "plan write A: "+err.Error(), http.StatusInternalServerError) return @@ -1393,7 +1393,7 @@ func (s *AdminServer) handleResolveDuplicate(w http.ResponseWriter, r *http.Requ } // Move + rewrite in one journaled transaction so a failure cannot // leave the same ID in two directories. - txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(s.dataDir)) if err := txn.Move(absPath, newRel); err != nil { failJSON(w, "plan rename failed: "+err.Error()) return diff --git a/internal/admin/api_rooms_bulk.go b/internal/admin/api_rooms_bulk.go index 3296375..eb5e89d 100644 --- a/internal/admin/api_rooms_bulk.go +++ b/internal/admin/api_rooms_bulk.go @@ -143,7 +143,7 @@ func (s *AdminServer) handleBulkEdit(w http.ResponseWriter, r *http.Request) { } // ---- Apply as one journaled transaction. ---- - txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(s.dataDir)) for _, wr := range writes { if wr.newPath != wr.oldPath { if err := txn.Move(wr.oldPath, wr.newPath); err != nil { diff --git a/internal/admin/yaml_util.go b/internal/admin/yaml_util.go index e1d4942..2ab7ec4 100644 --- a/internal/admin/yaml_util.go +++ b/internal/admin/yaml_util.go @@ -1,15 +1,13 @@ package admin import ( - "bytes" "fmt" "log" "os" "path/filepath" "strings" - "gopkg.in/yaml.v3" - + "thehouseoficarus/internal/behavior" "thehouseoficarus/internal/persistence" ) @@ -20,16 +18,7 @@ func readYAMLFile(dataDir, subdir, id string) ([]byte, string, error) { } func marshalYAML(data any) ([]byte, error) { - var buf bytes.Buffer - enc := yaml.NewEncoder(&buf) - enc.SetIndent(2) - if err := enc.Encode(data); err != nil { - return nil, err - } - if err := enc.Close(); err != nil { - return nil, err - } - return buf.Bytes(), nil + return behavior.MarshalYAML(data) } func writeYAMLFile(path string, data any) ([]byte, error) { @@ -214,7 +203,7 @@ func moveEntityFile(dataDir, subdir, id, targetDir string) (string, string, []by } // Move in one journaled transaction: a failed write or source removal // must not leave the entity in two directories. - txn := persistence.NewTransaction(filepath.Join(dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(dataDir)) if err := txn.Move(oldPath, newPath); err != nil { return "", "", nil, fmt.Errorf("move: %w", err) } @@ -279,7 +268,7 @@ func renameEntityFile(dataDir, subdir, oldID, newID string) (string, string, []b } // Rename in one journaled transaction: a failed write or source removal // must not leave the entity in two directories. - txn := persistence.NewTransaction(filepath.Join(dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(dataDir)) if err := txn.Move(oldPath, newPath); err != nil { return "", "", nil, fmt.Errorf("rename: %w", err) } @@ -335,7 +324,7 @@ func deleteEntityDir(dataDir, subdir, dir string) ([]ChangeDesc, error) { } var changes []ChangeDesc - txn := persistence.NewTransaction(filepath.Join(dataDir, ".journal")) + txn := persistence.NewTransaction(persistence.JournalDir(dataDir)) for _, e := range entries { if e.IsDir() || filepath.Ext(e.Name()) != ".yaml" { continue diff --git a/internal/behavior/roomrefs.go b/internal/behavior/roomrefs.go index 4953092..c3604b7 100644 --- a/internal/behavior/roomrefs.go +++ b/internal/behavior/roomrefs.go @@ -132,14 +132,13 @@ func remapRoomRefList(list []any, idMap map[int]int) bool { return changed } -// MarshalYAMLMap encodes a decoded YAML map with the same settings the admin -// editor uses, so a rewritten content file matches what the editor would write -// and cannot drop a key a typed struct does not model. -func MarshalYAMLMap(m map[string]any) ([]byte, error) { +// MarshalYAML encodes a value as indented YAML (2-space), the same settings +// the admin editor and room-ref writers use. +func MarshalYAML(data any) ([]byte, error) { var buf bytes.Buffer enc := yaml.NewEncoder(&buf) enc.SetIndent(2) - if err := enc.Encode(m); err != nil { + if err := enc.Encode(data); err != nil { return nil, err } if err := enc.Close(); err != nil { @@ -148,6 +147,13 @@ func MarshalYAMLMap(m map[string]any) ([]byte, error) { return buf.Bytes(), nil } +// MarshalYAMLMap encodes a decoded YAML map with the same settings the admin +// editor uses, so a rewritten content file matches what the editor would write +// and cannot drop a key a typed struct does not model. +func MarshalYAMLMap(m map[string]any) ([]byte, error) { + return MarshalYAML(m) +} + // asInt coerces the numeric types a YAML decoder (or a JSON round-trip) can // produce for an integer field. func asInt(v any) (int, bool) { diff --git a/internal/game/account_txn.go b/internal/game/account_txn.go index 6e1e2d1..a1a5cf0 100644 --- a/internal/game/account_txn.go +++ b/internal/game/account_txn.go @@ -6,7 +6,6 @@ import ( "fmt" "log" "os" - "path/filepath" "strings" "thehouseoficarus/internal/net" @@ -25,7 +24,7 @@ import ( // journalDir is the persistence journal directory under the game data dir. func (g *Game) journalDir() string { - return filepath.Join(g.DataDir, ".journal") + return persistence.JournalDir(g.DataDir) } // newTxn starts a lifecycle transaction. diff --git a/internal/game/harness_test.go b/internal/game/harness_test.go index ff5ac32..3bdce0f 100644 --- a/internal/game/harness_test.go +++ b/internal/game/harness_test.go @@ -123,6 +123,10 @@ func newHarness(t *testing.T) *harness { conns: make(map[string]*chanConn), } h.stop = func() { close(stopTick); <-done } + // Stop the Game's owner goroutine too, or every harness test leaks one + // (each holding a 4096-slot channel) for the life of the test binary. + // Cleanup is LIFO, so the ticker stops first, then the owner. + t.Cleanup(g.stopOwner) t.Cleanup(h.stop) return h } diff --git a/internal/net/web_test.go b/internal/net/web_test.go new file mode 100644 index 0000000..d16b31a --- /dev/null +++ b/internal/net/web_test.go @@ -0,0 +1,59 @@ +package net + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gorilla/websocket" +) + +// The browser transport previously had no tests. CheckOrigin is the security +// boundary for the /ws upgrade; test it directly with a non-loopback peer so +// the loopback bypass does not mask a hostile Origin. +func TestWebSocketCheckOrigin(t *testing.T) { + cases := []struct { + name string + origin string + host string + remoteAddr string + want bool + }{ + {"no origin", "", "mud.example:4000", "203.0.113.5:1234", true}, + {"same host", "http://mud.example:4000", "mud.example:4000", "203.0.113.5:1234", true}, + {"same host https", "https://mud.example:4000", "mud.example:4000", "203.0.113.5:1234", true}, + {"same hostname no port", "http://mud.example", "mud.example:4000", "203.0.113.5:1234", true}, + {"hostile origin", "http://evil.example", "mud.example:4000", "203.0.113.5:1234", false}, + {"loopback bypass", "http://evil.example", "mud.example:4000", "127.0.0.1:1234", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := &http.Request{Header: http.Header{}, Host: tc.host, RemoteAddr: tc.remoteAddr} + if tc.origin != "" { + req.Header.Set("Origin", tc.origin) + } + if got := upgrader.CheckOrigin(req); got != tc.want { + t.Fatalf("CheckOrigin(%q, host %q, peer %q) = %v, want %v", tc.origin, tc.host, tc.remoteAddr, got, tc.want) + } + }) + } +} + +// The /ws upgrade wires a live session and writer. Drive it end-to-end over a +// real HTTP test server (loopback, so the origin check passes). +func TestWebSocketUpgradeAccepted(t *testing.T) { + s := &Server{hub: NewHub()} + done := make(chan struct{}) + s.handler = func(*Session, string) { <-done } + srv := httptest.NewServer(newHTTPMux(s)) + defer srv.Close() + defer close(done) + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + "/ws" + conn, resp, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("upgrade rejected: %v (resp %v)", err, resp) + } + conn.Close() +} diff --git a/internal/persistence/txn.go b/internal/persistence/txn.go index 988af23..1babfc5 100644 --- a/internal/persistence/txn.go +++ b/internal/persistence/txn.go @@ -34,6 +34,13 @@ import ( // as a committed transaction that the next startup will roll back. const journalRemoveAttempts = 3 +// JournalDir returns the transaction journal directory under a data dir. Both +// the game and the admin API must use this so a future rename cannot leave one +// writer journaling to a directory startup recovery never scans. +func JournalDir(dataDir string) string { + return filepath.Join(dataDir, ".journal") +} + // journalVersion is the current journal schema version. Recover refuses a // version it does not understand rather than misreading a newer journal. const journalVersion = 1 -- cgit v1.2.3