fix: shared YAML encoder and journal directory, text cleanup

9 files changed, 96 insertions, 40 deletions

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