fix: atomic saves, casino RNG/config isolation, tick lifecycle

2 files changed, 99 insertions, 21 deletions

diff --git a/internal/player/store.go b/internal/player/store.go
index 9bfdf40..d0dbd0f 100644
--- a/internal/player/store.go
+++ b/internal/player/store.go
@@ -8,6 +8,7 @@ import (
 
 	"gopkg.in/yaml.v3"
 	"thehouseoficarus/internal/item"
+	"thehouseoficarus/internal/persistence"
 )
 
 type AccountStore struct {
@@ -41,7 +42,7 @@ func (s *AccountStore) SaveAccount(acc *Account) error {
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(path, data, 0644)
+	return persistence.WriteFile(path, data, 0600)
 }
 
 func (s *AccountStore) AccountExists(name string) bool {
@@ -61,7 +62,7 @@ func (s *AccountStore) FindAccount(name string) (string, error) {
 	}
 	lower := strings.ToLower(name)
 	for _, e := range entries {
-		if e.IsDir() {
+		if e.IsDir() || !strings.HasSuffix(e.Name(), ".yaml") {
 			continue
 		}
 		base := strings.TrimSuffix(e.Name(), ".yaml")
@@ -103,23 +104,20 @@ func (s *AccountStore) LoadCharacter(name string) (*Player, error) {
 
 func (s *AccountStore) SaveCharacter(p *Player) error {
 	path := s.CharPath(p.Name)
-	if p.GodMode && p.GodBackup != nil {
-		buffedSkills := p.Skills
-		buffedHP := p.HP
-		p.Skills = p.GodBackup
-		if p.HP > p.MaxHP() {
-			p.HP = p.MaxHP()
+	// Serialize a value copy so saving god-mode stats never temporarily
+	// changes the live player. The caller still owns all referenced maps.
+	snapshot := *p
+	if snapshot.GodMode && snapshot.GodBackup != nil {
+		snapshot.Skills = snapshot.GodBackup
+		if snapshot.HP > snapshot.MaxHP() {
+			snapshot.HP = snapshot.MaxHP()
 		}
-		defer func() {
-			p.Skills = buffedSkills
-			p.HP = buffedHP
-		}()
 	}
-	data, err := yaml.Marshal(p)
+	data, err := yaml.Marshal(&snapshot)
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(path, data, 0644)
+	return persistence.WriteFile(path, data, 0600)
 }
 
 func (s *AccountStore) CharacterExists(name string) bool {
@@ -139,7 +137,7 @@ func (s *AccountStore) FindCharacter(name string) (string, error) {
 	}
 	lower := strings.ToLower(name)
 	for _, e := range entries {
-		if e.IsDir() {
+		if e.IsDir() || !strings.HasSuffix(e.Name(), ".yaml") {
 			continue
 		}
 		base := strings.TrimSuffix(e.Name(), ".yaml")
@@ -150,10 +148,7 @@ func (s *AccountStore) FindCharacter(name string) (string, error) {
 	return "", fmt.Errorf("character not found: %s", name)
 }
 
-// ListCharacterNames returns the stem (character name) of every
-// data/players/characters/*.yaml file. Used by room-ID migration to walk and
-// rewrite offline characters. Online characters are excluded by the caller
-// (their in-memory state is updated directly).
+// ListAccountNames returns the stem of each account YAML file.
 func (s *AccountStore) ListAccountNames() ([]string, error) {
 	dir := filepath.Join(s.dataDir, "players", "accounts")
 	entries, err := os.ReadDir(dir)
@@ -162,7 +157,7 @@ func (s *AccountStore) ListAccountNames() ([]string, error) {
 	}
 	var names []string
 	for _, e := range entries {
-		if e.IsDir() {
+		if e.IsDir() || !strings.HasSuffix(e.Name(), ".yaml") {
 			continue
 		}
 		name := strings.TrimSuffix(e.Name(), ".yaml")
@@ -174,6 +169,8 @@ func (s *AccountStore) ListAccountNames() ([]string, error) {
 	return names, nil
 }
 
+// ListCharacterNames returns character YAML stems for offline migrations.
+// Callers exclude online characters and update their in-memory state instead.
 func (s *AccountStore) ListCharacterNames() ([]string, error) {
 	dir := filepath.Join(s.dataDir, "players", "characters")
 	entries, err := os.ReadDir(dir)
@@ -182,7 +179,7 @@ func (s *AccountStore) ListCharacterNames() ([]string, error) {
 	}
 	var names []string
 	for _, e := range entries {
-		if e.IsDir() {
+		if e.IsDir() || !strings.HasSuffix(e.Name(), ".yaml") {
 			continue
 		}
 		name := strings.TrimSuffix(e.Name(), ".yaml")
diff --git a/internal/player/store_test.go b/internal/player/store_test.go
new file mode 100644
index 0000000..593e9d1
--- /dev/null
+++ b/internal/player/store_test.go
@@ -0,0 +1,81 @@
+package player
+
+import (
+	"os"
+	"path/filepath"
+	"reflect"
+	"sync"
+	"testing"
+)
+
+func testAccountStore(t *testing.T) *AccountStore {
+	t.Helper()
+	dir := t.TempDir()
+	for _, kind := range []string{"accounts", "characters"} {
+		if err := os.MkdirAll(filepath.Join(dir, "players", kind), 0755); err != nil {
+			t.Fatal(err)
+		}
+	}
+	return NewAccountStore(dir)
+}
+
+func TestSaveGodPlayerDoesNotMutateLiveState(t *testing.T) {
+	s := testAccountStore(t)
+	p := New("Alice")
+	p.GodBackup = p.Skills
+	p.Skills = map[SkillName]int{Hitpoints: 1000000}
+	p.GodMode = true
+	p.HP = 999
+	expected := *p
+	expected.Skills = p.GodBackup
+	wantHP := expected.MaxHP()
+
+	// Concurrent saves of an otherwise immutable player used to race while
+	// temporarily swapping Skills/HP. Saving must be observationally read-only.
+	var wg sync.WaitGroup
+	for i := 0; i < 2; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			if err := s.SaveCharacter(p); err != nil {
+				t.Error(err)
+			}
+		}()
+	}
+	wg.Wait()
+	if p.HP != 999 || p.Skills[Hitpoints] != 1000000 {
+		t.Fatal("save changed live god-mode state")
+	}
+	saved, err := s.LoadCharacter(p.Name)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if saved.HP != wantHP || !reflect.DeepEqual(saved.Skills, p.GodBackup) {
+		t.Fatalf("saved god-mode stats instead of underlying character: %+v", saved)
+	}
+}
+
+func TestAccountStoreIgnoresNonYAMLFiles(t *testing.T) {
+	s := testAccountStore(t)
+	for _, path := range []string{s.AccountPath("Alice"), s.CharPath("Alice")} {
+		for _, name := range []string{"Alice.yaml", ".save-stale", "Backup.yaml.bak"} {
+			if err := os.WriteFile(filepath.Join(filepath.Dir(path), name), nil, 0600); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+	for _, list := range []func() ([]string, error){s.ListAccountNames, s.ListCharacterNames} {
+		got, err := list()
+		if err != nil || !reflect.DeepEqual(got, []string{"Alice"}) {
+			t.Fatalf("list = %v, %v", got, err)
+		}
+	}
+	for _, find := range []func(string) (string, error){s.FindAccount, s.FindCharacter} {
+		if got, err := find("alice"); err != nil || got != "Alice" {
+			t.Fatalf("case-insensitive lookup = %q, %v", got, err)
+		}
+		if _, err := find(".save-stale"); err == nil {
+			t.Fatal("temporary file treated as an account/character")
+		}
+	}
+}