fix: admin undo/redo failure, video poker cabinet ids, room ref behavior

14 files changed, 152 insertions, 4 deletions

diff --git a/internal/admin/api_rooms.go b/internal/admin/api_rooms.go
index 586bfff..76db231 100644
--- a/internal/admin/api_rooms.go
+++ b/internal/admin/api_rooms.go
@@ -1315,6 +1315,25 @@ func (s *AdminServer) handleResolveDuplicate(w http.ResponseWriter, r *http.Requ
 		failJSON(w, "invalid path")
 		return
 	}
+	// Only a real duplicate-ID file may be resolved here. Trusting the client
+	// path would let this endpoint delete a live indexed room with none of the
+	// normal delete semantics (exit cleanup, casino eviction, undo).
+	stem := strings.TrimSuffix(filepath.Base(absPath), ".yaml")
+	isDuplicate := false
+	for _, d := range behavior.CheckDuplicateIDs(roomsDir) {
+		if d.ID != stem {
+			continue
+		}
+		for _, p := range d.Paths {
+			if pa, err := filepath.Abs(p); err == nil && pa == absPath {
+				isDuplicate = true
+			}
+		}
+	}
+	if !isDuplicate {
+		failJSON(w, "path is not a duplicate-ID room file")
+		return
+	}
 
 	switch body.Action {
 	case "delete":
diff --git a/internal/admin/api_triggers.go b/internal/admin/api_triggers.go
index 0dd086c..ff3e7d6 100644
--- a/internal/admin/api_triggers.go
+++ b/internal/admin/api_triggers.go
@@ -56,8 +56,12 @@ func (s *AdminServer) handleTriggers(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 
-		newContent, err := writeMapAsYAML(path, m)
+		newContent, err := triggerMapToYAML(m)
 		if err != nil {
+			failJSON(w, "serialize error: "+err.Error())
+			return
+		}
+		if err := persistence.WriteFile(path, newContent, 0644); err != nil {
 			failJSON(w, "write error: "+err.Error())
 			return
 		}
diff --git a/internal/admin/correct_audit_fixes_test.go b/internal/admin/correct_audit_fixes_test.go
index 60073f7..d97732d 100644
--- a/internal/admin/correct_audit_fixes_test.go
+++ b/internal/admin/correct_audit_fixes_test.go
@@ -49,3 +49,27 @@ func TestRestoreAccountSecretRefusesNonMappingBody(t *testing.T) {
 		t.Fatalf("password hash not preserved: %s", out)
 	}
 }
+
+// Regression: a failed undo returned a nil change exactly like an empty
+// history, so the UI reported "Nothing to undo" for a real failure.
+func TestUndoFailureIsReportedDistinctly(t *testing.T) {
+	dir := t.TempDir()
+	us := NewUndoStack(dir)
+	us.Push(ChangeDesc{
+		Description: "edit with a missing before-image",
+		FilePath:    filepath.Join(dir, "rooms", "1.yaml"),
+		OldContent:  nil,
+		NewContent:  []byte("id: 1\n"),
+	})
+	if ch := us.Undo(); ch != nil {
+		t.Fatal("undo with a missing before-image unexpectedly succeeded")
+	}
+	if err := us.LastError(); err == nil {
+		t.Fatal("LastError = nil after a failed undo; the UI would report 'Nothing to undo'")
+	}
+	// A genuinely empty history is a clean nil change and nil error.
+	us2 := NewUndoStack(t.TempDir())
+	if ch := us2.Undo(); ch != nil || us2.LastError() != nil {
+		t.Fatal("empty history must be a clean nil change and nil error")
+	}
+}
diff --git a/internal/admin/server.go b/internal/admin/server.go
index 8fcba77..800cfa1 100644
--- a/internal/admin/server.go
+++ b/internal/admin/server.go
@@ -908,6 +908,10 @@ func (s *AdminServer) doUndo(w http.ResponseWriter, r *http.Request) {
 	}
 	change := s.undoStack.Undo()
 	if change == nil {
+		if err := s.undoStack.LastError(); err != nil {
+			writeJSONError(w, "undo failed: "+err.Error(), http.StatusInternalServerError)
+			return
+		}
 		writeJSON(w, map[string]any{"message": "Nothing to undo"})
 		return
 	}
@@ -927,6 +931,10 @@ func (s *AdminServer) doRedo(w http.ResponseWriter, r *http.Request) {
 	}
 	change := s.undoStack.Redo()
 	if change == nil {
+		if err := s.undoStack.LastError(); err != nil {
+			writeJSONError(w, "redo failed: "+err.Error(), http.StatusInternalServerError)
+			return
+		}
 		writeJSON(w, map[string]any{"message": "Nothing to redo"})
 		return
 	}
diff --git a/internal/admin/undo.go b/internal/admin/undo.go
index 5683f13..c3231e3 100644
--- a/internal/admin/undo.go
+++ b/internal/admin/undo.go
@@ -85,6 +85,10 @@ type UndoStack struct {
 	// writes those files directly, so it must refuse the same files the raw
 	// file editor refuses. Optional; nil disables the refusal.
 	playerFileOnlineCheck func(relPath string) bool
+	// lastErr records the most recent Undo/Redo failure so the HTTP handler
+	// can distinguish a genuine failure from an empty history (both return a
+	// nil change). Owner-task access only.
+	lastErr error
 }
 
 func NewUndoStack(dataDir string) *UndoStack {
@@ -179,8 +183,10 @@ func (us *UndoStack) Undo() *ChangeDesc {
 
 	if err := us.applyUndo(last); err != nil {
 		log.Printf("undo: failed %s: %v", last.ShortDesc(), err)
+		us.lastErr = err
 		return nil
 	}
+	us.lastErr = nil
 	// Runtime migration moves with the file images: undo applies the
 	// inverse room-ID map so players, Hub membership, and casino room keys
 	// return to the old location together.
@@ -209,8 +215,10 @@ func (us *UndoStack) Redo() *ChangeDesc {
 
 	if err := us.applyRedo(last); err != nil {
 		log.Printf("redo: failed %s: %v", last.ShortDesc(), err)
+		us.lastErr = err
 		return nil
 	}
+	us.lastErr = nil
 	// Redo applies the forward room-ID map through the runtime migration.
 	if len(last.IDMap) > 0 && us.migrateRoomIDs != nil {
 		forward := make(map[int]int, len(last.IDMap))
@@ -803,6 +811,15 @@ func (us *UndoStack) Clear() {
 	defer us.mu.Unlock()
 	us.history = nil
 	us.redo = nil
+	us.lastErr = nil
 	os.Remove(us.filePath)
 	log.Printf("undo: cleared history")
 }
+
+// LastError returns the error from the most recent failed Undo/Redo, or nil.
+// A nil change plus a nil LastError means the history/redo stack was empty.
+func (us *UndoStack) LastError() error {
+	us.mu.Lock()
+	defer us.mu.Unlock()
+	return us.lastErr
+}
diff --git a/internal/casino/config_validate.go b/internal/casino/config_validate.go
index 74dc0a4..41d5b83 100644
--- a/internal/casino/config_validate.go
+++ b/internal/casino/config_validate.go
@@ -359,8 +359,12 @@ func ValidateRoomConfigs(tables []TableConfig, machines []MachineConfig) error {
 	for _, c := range tables {
 		check("table", c.ID, c.Validate())
 		if c.Game == GameVideoPoker {
+			// Cabinets are addressed by their unique ID (not the game name), so
+			// claim the ID as an address. Skipping the claim let a cabinet ID
+			// collide with a machine variant/game name and become unreachable.
+			checkAddressable(strings.ToLower(c.ID), c.ID)
 			continue
-		} // Cabinets are addressed by unique ID, not game name.
+		}
 		checkAddressable(strings.ToLower(string(c.Game)), c.ID)
 	}
 	// A machine addresses by its variant when one is declared; a bare game
diff --git a/internal/casino/correct_audit_fixes_test.go b/internal/casino/correct_audit_fixes_test.go
index e5e3bd3..dc33c47 100644
--- a/internal/casino/correct_audit_fixes_test.go
+++ b/internal/casino/correct_audit_fixes_test.go
@@ -21,3 +21,14 @@ func TestCrapsShooterChangeResetsHand(t *testing.T) {
 		t.Fatalf("new shooter inherited hand: point=%d rolled=%v made=%v seen=%v", tb.point, tb.rolled, tb.made, tb.seen)
 	}
 }
+
+// Regression: a video-poker cabinet addressed by an ID that collides with a
+// machine variant/game name validated but was unreachable (the machine won the
+// address lookup).
+func TestVideoPokerCabinetIDCollisionIsRejected(t *testing.T) {
+	machines := []MachineConfig{{ID: "m1", Game: GameSlots, Variant: "x"}}
+	tables := []TableConfig{{ID: "x", Game: GameVideoPoker}}
+	if err := ValidateRoomConfigs(tables, machines); err == nil {
+		t.Fatal("cabinet ID colliding with a machine variant was accepted; the cabinet would be unreachable")
+	}
+}
diff --git a/internal/game/cmd_registry.go b/internal/game/cmd_registry.go
index d10ba0e..2defb9e 100644
--- a/internal/game/cmd_registry.go
+++ b/internal/game/cmd_registry.go
@@ -122,7 +122,7 @@ var commandRegistry = map[string]commandDef{
 	"i":             {(*Game).executeInventory, ClassInstant},
 	"inv":           {(*Game).executeInventory, ClassInstant},
 	"inventory":     {(*Game).executeInventory, ClassInstant},
-	"quit":          {(*Game).executeQuit, ClassActive},
+	"quit":          {(*Game).executeQuit, ClassInstant},
 	"description":   {(*Game).executeDescription, ClassInstant},
 	"desc":          {(*Game).executeDescription, ClassInstant},
 	"option":        {(*Game).executeOption, ClassInstant},
diff --git a/internal/net/conn.go b/internal/net/conn.go
index a4be83a..d8c867e 100644
--- a/internal/net/conn.go
+++ b/internal/net/conn.go
@@ -79,7 +79,11 @@ func (t *tcpConn) ReadMessage() (string, error) {
 	text = stripIAC(text)
 	text = truncateLine(text, maxLineLength)
 	if t.echoOff.Load() {
-		t.conn.Write([]byte("\r\n"))
+		// Refresh the write deadline: the writer goroutine may have left a
+		// stale deadline in the past, which would silently drop this echo on
+		// an idle connection.
+		_ = t.conn.SetWriteDeadline(time.Now().Add(writeDeadline))
+		_, _ = t.conn.Write([]byte("\r\n"))
 	}
 	return text, nil
 }
diff --git a/internal/net/server.go b/internal/net/server.go
index 71e34c8..8e23a1f 100644
--- a/internal/net/server.go
+++ b/internal/net/server.go
@@ -808,6 +808,11 @@ func (s *Server) Shutdown() {
 			if remaining <= 0 {
 				break
 			}
+			// Cap each wait so one stalled writer cannot consume the whole
+			// budget and starve every later session's flush.
+			if remaining > 200*time.Millisecond {
+				remaining = 200 * time.Millisecond
+			}
 			select {
 			case <-sess.writeDone:
 			case <-time.After(remaining):
diff --git a/internal/validate/checks.go b/internal/validate/checks.go
index d3fa111..2030225 100644
--- a/internal/validate/checks.go
+++ b/internal/validate/checks.go
@@ -1624,6 +1624,17 @@ func validateGather(prefix string, cfg *behavior.GatherConfig, itemIDs map[strin
 					prefix, label, len(cfg.Tools)),
 			})
 		}
+		if d.Weight <= 0 {
+			label := d.ItemID
+			if label == "" {
+				label = "<table:" + d.Table + ">"
+			}
+			issues = append(issues, Issue{
+				Level:   "WARN",
+				Type:    "semantic",
+				Message: fmt.Sprintf("%s: drop %q has weight %d; it can never be selected", prefix, label, d.Weight),
+			})
+		}
 	}
 
 	return issues
diff --git a/internal/validate/correct_audit_fixes_test.go b/internal/validate/correct_audit_fixes_test.go
index 8ed6f52..f11dd3f 100644
--- a/internal/validate/correct_audit_fixes_test.go
+++ b/internal/validate/correct_audit_fixes_test.go
@@ -31,3 +31,19 @@ func TestValidateTalkConfigRequiresStartNode(t *testing.T) {
 		}
 	}
 }
+
+// Regression: a gather drop with weight <= 0 can never be selected (the
+// weighted roll has zero total); it must be flagged.
+func TestValidateGatherWarnsOnWeightlessDrop(t *testing.T) {
+	cfg := &behavior.GatherConfig{Drops: []behavior.DropEntry{{ItemID: "ore", Weight: 0}}}
+	issues := validateGather("test", cfg, map[string]bool{"ore": true}, map[string]bool{})
+	found := false
+	for _, iss := range issues {
+		if strings.Contains(iss.Message, "weight") {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("weightless drop not warned: %+v", issues)
+	}
+}
diff --git a/internal/world/correct_audit_fixes_test.go b/internal/world/correct_audit_fixes_test.go
new file mode 100644
index 0000000..0ecb87a
--- /dev/null
+++ b/internal/world/correct_audit_fixes_test.go
@@ -0,0 +1,18 @@
+package world
+
+import (
+	"testing"
+
+	"gopkg.in/yaml.v3"
+)
+
+// Regression: a reference-style room object entry (id only) that also carries
+// behavior fields silently lost them at runtime, because the loader always
+// uses the object file. It must now be rejected at load time.
+func TestRoomObjectReferenceRejectsBehaviorFields(t *testing.T) {
+	src := "id: 1\nname: Test\nobjects:\n  - id: chest\n    on_use:\n      - steps:\n          - message: hi\n"
+	var r Room
+	if err := yaml.Unmarshal([]byte(src), &r); err == nil {
+		t.Fatal("reference object entry with on_use was accepted; the behavior would be silently lost")
+	}
+}
diff --git a/internal/world/room.go b/internal/world/room.go
index e200f32..9b9bef7 100644
--- a/internal/world/room.go
+++ b/internal/world/room.go
@@ -171,6 +171,13 @@ func (ro *RoomObject) UnmarshalYAML(value *yaml.Node) error {
 		ro.Local = &def
 		return nil
 	}
+	// A reference entry (no name/description) that also carries behavior
+	// fields silently loses them at runtime, because the loader always uses
+	// the object file. Reject it instead of discarding authored behavior.
+	if def.RemovalItem != "" || len(def.OnUse) > 0 || def.Steal != nil ||
+		def.Gather != nil || def.Talk != nil || def.Safespot != nil {
+		return fmt.Errorf("room object entry %q cannot carry behavior (on_use/gather/talk/steal/removal_item/safespot); define it in objects/%s.yaml or give the entry a name", ref.ID, ref.ID)
+	}
 	ro.ID = ref.ID
 	ro.HiddenOverride = ref.Hidden
 	return nil