fix(config): put the old value back when a setter is rejected

Config.Save() validates the whole config, so a setter that assigned
before validating did not merely fail its own call: the rejected value
stayed in memory and failed every later save, of every unrelated
setting, silently and for the rest of the session. Nothing reached
disk, so a restart cleared it — which is what made the fault invisible
and unreportable.

The defect is precisely "assignment precedes a validation that can
reject that argument", and that predicate enumerates seven setters
rather than the whole file. Each snapshots the field and restores it on
the error path.

The remaining setters were read rather than assumed and are unchanged:
shortcuts.Config.Validate returns nil unconditionally, the bools and
SetFavoritesPlaylistID pass through no validation that inspects them,
Config.Validate does not validate Downloads at all, and SetViewVisible
refuses an unknown, non-hideable or launch-page view before assigning.
SetLibraryDirectory was already correct and is the precedent the new
comment points at: it validates a candidate before assigning, so there
is nothing to undo.

The rationale sits above the setter section rather than on Save(),
which is bound — a doc comment there renders into frontend/bindings
for an audience with no use for it.

Closes #231

CLAUDE.md

@@ -161,7 +161,7 @@ make ui-test          # Vitest component/store suite in a real browser (no app)
 make ui-visual        # Same, including toMatchScreenshot comparisons
 make ui-setup         # Install the Vitest provider's own Chromium (once)
 make bindings-check   # Fail if frontend/bindings is stale vs the Go bindings
-make skill-check      # Fail if a doc names a make target that doesn't exist
+make skill-check      # Fail if .pi/ documents a make target that doesn't exist
 make commit-check     # Fail if a commit subject is not a Conventional Commit
 make lint             # golangci-lint v2 (strict), all three build configurations
 make test             # All tests with race detector, all three build configurations
@@ -662,6 +662,28 @@ rather than renaming them.
   one of the shell's rows, which is what the skip link is absolutely
   positioned to avoid.
 - `config` — TOML-based settings. Settings page uses HTMX + templ for server-rendered HTML fragments.
+
+  **A setter that can reject its argument puts the old value back**, and
+  that is a correctness rule rather than hygiene (#231). `Save()`
+  validates the *whole* config, so a value left behind by a failed write
+  does not merely fail its own call: it fails every later save, of every
+  unrelated setting — theme, launch page, shortcuts, libraries — for the
+  rest of the session. Nothing reaches disk, so a restart clears it,
+  which is exactly what makes the fault invisible and unreportable. One
+  rejected track-list column list was enough to stop the app saving
+  anything at all.
+
+  Two shapes are safe and a third is the trap. A setter that assigns and
+  *then* validates snapshots the field first and restores it on the
+  error path — seven do. `SetLibraryDirectory` is the better shape where
+  the value can be built on its own: it validates a candidate *before*
+  assigning, so there is nothing to undo. And a setter whose argument no
+  validation inspects needs neither — the bools, the favourites playlist
+  id and the shortcut bindings, plus `SetViewVisible`, which refuses an
+  unknown, non-hideable or launch-page view up front so
+  `GeneralConfig.Validate` never sees one it would fail on. Which set a
+  new setter joins is decided by whether its own `Validate` can reject
+  it, not by preference.
 - `playlist` / `smartplaylist` — Playlist CRUD and rule-based smart playlists.
 - `mediacontrols` — OS media controls behind one `Handler`: MPRIS over
   D-Bus on desktop Linux, a MediaSession on Android, a no-op stub

Makefile

@@ -192,7 +192,7 @@ css-check: ## Fail on a css`` literal ended early by a backtick, or a nested rul
 # Every command in them is a make target on purpose, so this is
 # checkable.  It also asserts AGENTS.md is a symlink to CLAUDE.md, so the
 # two harnesses cannot drift onto two descriptions of one project.
-skill-check: ## Fail if the docs name a missing make target, or AGENTS.md is not a symlink
+skill-check: ## Fail if the agent docs name a missing make target, or AGENTS.md is not a symlink
 	@./scripts/skill-check.sh
 
 # Conventional Commits, which CLAUDE.md claimed CI enforced for a long

backend/config/config.go

@@ -303,6 +303,24 @@ func (c *Config) GetLibraryDirectory() string {
 	return string(c.Library.DirectoryPath)
 }
 
+// A rejected setter puts the old value back, and that is not tidiness
+// (#231).  Save validates the *whole* config, so a value left behind by
+// a failed write does not merely fail its own call: it fails every
+// later save, of every unrelated setting, silently and for the rest of
+// the session.  Nothing reaches disk, so a restart clears it -- which
+// is exactly what makes the fault hard to see and impossible to report.
+//
+// The setters below that assign and then validate therefore snapshot
+// the field first and restore it on the error path.  SetLibraryDirectory
+// is the other safe shape and the better one where the value can be
+// built on its own: it validates a candidate *before* assigning
+// anything, so there is nothing to undo.
+//
+// Not every setter needs either.  A bool, an int64 and the shortcut
+// bindings pass through no validation that can reject them, and
+// SetViewVisible refuses an unknown, non-hideable or launch-page view
+// up front, so GeneralConfig.Validate never sees one it would fail on.
+
 // SetLibraryDirectory validates and saves a new library directory,
 // then emits the LibraryConfigChanged event so listeners (e.g. the
 // Library scanner) can react.
@@ -360,11 +378,14 @@ func (c *Config) SetScanConcurrency(mode string) error {
 		c.Library.ApplyDefaults()
 	}
 
+	previous := c.Library.ScanConcurrency
 	c.Library.ScanConcurrency = library.ScanConcurrency(
 		mode,
 	)
 
 	if err := c.Library.Validate(); err != nil {
+		c.Library.ScanConcurrency = previous
+
 		return fmt.Errorf(
 			"invalid scan concurrency mode: %w", err,
 		)
@@ -455,9 +476,12 @@ func (c *Config) SetThemeAccentColor(
 		c.Theme.ApplyDefaults()
 	}
 
+	previous := c.Theme.AccentColor
 	c.Theme.AccentColor = color
 
 	if err := c.Theme.Validate(); err != nil {
+		c.Theme.AccentColor = previous
+
 		return fmt.Errorf(
 			"invalid theme accent color: %w", err,
 		)
@@ -488,9 +512,12 @@ func (c *Config) SetThemeBackgroundShade(
 		c.Theme.ApplyDefaults()
 	}
 
+	previous := c.Theme.BackgroundShade
 	c.Theme.BackgroundShade = theme.BackgroundShade(shade)
 
 	if err := c.Theme.Validate(); err != nil {
+		c.Theme.BackgroundShade = previous
+
 		return fmt.Errorf(
 			"invalid theme background shade: %w", err,
 		)
@@ -544,9 +571,12 @@ func (c *Config) SetDefaultPage(page string) error {
 		c.General.ApplyDefaults()
 	}
 
+	previous := c.General.DefaultPage
 	c.General.DefaultPage = View(page)
 
 	if err := c.General.Validate(); err != nil {
+		c.General.DefaultPage = previous
+
 		return fmt.Errorf(
 			"invalid default page: %w", err,
 		)
@@ -591,9 +621,12 @@ func (c *Config) SetQueueFallback(mode string) error {
 		c.General.ApplyDefaults()
 	}
 
+	previous := c.General.QueueFallback
 	c.General.QueueFallback = QueueFallback(mode)
 
 	if err := c.General.Validate(); err != nil {
+		c.General.QueueFallback = previous
+
 		return fmt.Errorf(
 			"invalid queue fallback: %w", err,
 		)
@@ -801,9 +834,12 @@ func (c *Config) SetTrackListColumns(
 		c.TrackList = &tracklist.Config{}
 	}
 
+	previous := c.TrackList.Columns
 	c.TrackList.Columns = columns
 
 	if err := c.TrackList.Validate(); err != nil {
+		c.TrackList.Columns = previous
+
 		return fmt.Errorf(
 			"invalid track-list columns: %w", err,
 		)
@@ -901,9 +937,12 @@ func (c *Config) SetFavoritesIconStyle(
 		c.Favorites.ApplyDefaults()
 	}
 
+	previous := c.Favorites.IconStyle
 	c.Favorites.IconStyle = favorites.IconStyle(style)
 
 	if err := c.Favorites.Validate(); err != nil {
+		c.Favorites.IconStyle = previous
+
 		return fmt.Errorf(
 			"invalid favorites icon style: %w", err,
 		)

backend/config/setter_rollback_test.go

@@ -0,0 +1,262 @@
+package config
+
+import (
+	"log/slog"
+	"path/filepath"
+	"testing"
+
+	"yellowjacket/backend/library"
+	"yellowjacket/backend/tracklist"
+)
+
+// newSavableConfig builds a loaded, valid config in a temp directory,
+// so Save() writes rather than refusing with errSaveBeforeLoad.
+//
+// The library directory is real and set, because Config.Validate only
+// validates the Library section when DirectoryPath is non-empty -- an
+// empty one would hide a poisoned ScanConcurrency from the whole-config
+// save that is the symptom under test.
+func newSavableConfig(t *testing.T) *Config {
+	t.Helper()
+
+	c := &Config{
+		logger:   slog.Default(),
+		filePath: filepath.Join(t.TempDir(), "config.toml"),
+		Library: &library.Config{
+			DirectoryPath: library.Directory(t.TempDir()),
+		},
+	}
+
+	c.applyDefaults()
+
+	if err := c.Load(); err != nil {
+		t.Fatalf("Load() error: %v", err)
+	}
+
+	if err := c.Save(); err != nil {
+		t.Fatalf("Save() on a fresh config error: %v", err)
+	}
+
+	return c
+}
+
+// TestSetterRejectionDoesNotPoisonTheConfig is the whole of #231.
+//
+// Every setter here assigns to the in-memory config and then validates.
+// When the validation rejects the argument, the rejected value has to go
+// back -- not because the caller sees it (it gets an error either way),
+// but because Config.Save() validates the *whole* config.  A value left
+// behind by a failed setter therefore fails every later save, of every
+// unrelated setting, silently and for the rest of the session.
+//
+// So each case asserts three things in order: the setter reports the
+// error, the getter still reports the old value, and an unrelated save
+// still works.  The third is the one the user feels.
+func TestSetterRejectionDoesNotPoisonTheConfig(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name string
+		// reject calls the setter with an argument its own Validate
+		// refuses.
+		reject func(*Config) error
+		// read reports the value the setter writes, so the rollback is
+		// asserted on the config rather than only on the save.
+		read func(*Config) string
+	}{
+		{
+			name: "scan concurrency",
+			reject: func(c *Config) error {
+				return c.SetScanConcurrency("telepathy")
+			},
+			read: (*Config).GetScanConcurrency,
+		},
+		{
+			name: "theme accent colour",
+			reject: func(c *Config) error {
+				return c.SetThemeAccentColor("not-a-hex")
+			},
+			read: (*Config).GetThemeAccentColor,
+		},
+		{
+			name: "theme background shade",
+			reject: func(c *Config) error {
+				return c.SetThemeBackgroundShade("chartreuse")
+			},
+			read: (*Config).GetThemeBackgroundShade,
+		},
+		{
+			name: "default page",
+			reject: func(c *Config) error {
+				return c.SetDefaultPage("nowhere")
+			},
+			read: (*Config).GetDefaultPage,
+		},
+		{
+			name: "queue fallback",
+			reject: func(c *Config) error {
+				return c.SetQueueFallback("improvise")
+			},
+			read: (*Config).GetQueueFallback,
+		},
+		{
+			name: "favorites icon style",
+			reject: func(c *Config) error {
+				return c.SetFavoritesIconStyle("asterisk")
+			},
+			read: (*Config).GetFavoritesIconStyle,
+		},
+		{
+			name: "track-list columns",
+			reject: func(c *Config) error {
+				// titleArtist is a drawing definition, not a
+				// configurable column (#197), so it is exactly what
+				// the frontend used to be able to send.
+				return c.SetTrackListColumns([]tracklist.Column{
+					{ID: "titleArtist"},
+				})
+			},
+			read: func(c *Config) string {
+				return columnIDs(c.GetTrackListColumns())
+			},
+		},
+		{
+			name: "track-list columns, duplicated",
+			reject: func(c *Config) error {
+				// The route #197 closed was one invalid id; a
+				// duplicate is the one still reachable from a client
+				// that assembles the list itself.
+				return c.SetTrackListColumns([]tracklist.Column{
+					{ID: tracklist.ColTrackName},
+					{ID: tracklist.ColTrackName},
+				})
+			},
+			read: func(c *Config) string {
+				return columnIDs(c.GetTrackListColumns())
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			c := newSavableConfig(t)
+			before := tc.read(c)
+
+			if err := tc.reject(c); err == nil {
+				t.Fatal("setter accepted an invalid value, want an error")
+			}
+
+			if after := tc.read(c); after != before {
+				t.Errorf(
+					"value after a rejected write = %q, want the previous %q",
+					after, before,
+				)
+			}
+
+			// The symptom: an unrelated setting can no longer be saved.
+			if err := c.SetPopupVolume(true); err != nil {
+				t.Errorf("an unrelated setter failed after a rejected write: %v", err)
+			}
+
+			if err := c.Save(); err != nil {
+				t.Errorf("Save() failed after a rejected write: %v", err)
+			}
+		})
+	}
+}
+
+// TestRejectedSetterLeavesNothingOnDisk pairs with the sweep above: the
+// rollback must not be undone by what the file already holds, so a
+// config reloaded from disk after a rejected write agrees with memory.
+func TestRejectedSetterLeavesNothingOnDisk(t *testing.T) {
+	t.Parallel()
+
+	c := newSavableConfig(t)
+
+	if err := c.SetThemeAccentColor("#123456"); err != nil {
+		t.Fatalf("SetThemeAccentColor() error: %v", err)
+	}
+
+	if err := c.SetThemeAccentColor("not-a-hex"); err == nil {
+		t.Fatal("SetThemeAccentColor accepted a non-colour, want an error")
+	}
+
+	reloaded := &Config{logger: slog.Default(), filePath: c.filePath}
+	reloaded.applyDefaults()
+
+	if err := reloaded.Load(); err != nil {
+		t.Fatalf("Load() error: %v", err)
+	}
+
+	if got := reloaded.GetThemeAccentColor(); got != "#123456" {
+		t.Errorf("accent colour on disk = %q, want %q", got, "#123456")
+	}
+
+	if c.GetThemeAccentColor() != reloaded.GetThemeAccentColor() {
+		t.Errorf(
+			"in-memory accent %q disagrees with disk %q after a rejected write",
+			c.GetThemeAccentColor(), reloaded.GetThemeAccentColor(),
+		)
+	}
+}
+
+// TestSetLibraryDirectoryValidatesBeforeAssigning pins the precedent the
+// seven rolled-back setters follow: this one has always built and
+// validated a candidate before assigning, so a bad path never reaches
+// the config at all.
+func TestSetLibraryDirectoryValidatesBeforeAssigning(t *testing.T) {
+	t.Parallel()
+
+	c := newSavableConfig(t)
+	before := c.GetLibraryDirectory()
+
+	if err := c.SetLibraryDirectory(filepath.Join(t.TempDir(), "no-such-dir")); err == nil {
+		t.Fatal("SetLibraryDirectory accepted a missing directory, want an error")
+	}
+
+	if after := c.GetLibraryDirectory(); after != before {
+		t.Errorf("library directory = %q, want the previous %q", after, before)
+	}
+
+	if err := c.Save(); err != nil {
+		t.Errorf("Save() failed after a rejected library directory: %v", err)
+	}
+}
+
+// TestSetViewVisibleRefusesBeforeAssigning covers the other setter left
+// out of the rollback pass: it guards its own argument up front, so
+// GeneralConfig.Validate never sees a view it would reject.
+func TestSetViewVisibleRefusesBeforeAssigning(t *testing.T) {
+	t.Parallel()
+
+	c := newSavableConfig(t)
+
+	if err := c.SetViewVisible("no-such-view", false); err == nil {
+		t.Fatal("SetViewVisible accepted an unknown view, want an error")
+	}
+
+	if err := c.SetViewVisible(c.GetDefaultPage(), false); err == nil {
+		t.Fatal("SetViewVisible hid the launch page, want an error")
+	}
+
+	if err := c.Save(); err != nil {
+		t.Errorf("Save() failed after a refused view visibility change: %v", err)
+	}
+}
+
+// columnIDs renders a column list for comparison in the table above.
+func columnIDs(cols []tracklist.Column) string {
+	ids := make([]byte, 0, len(cols)*8)
+
+	for i, col := range cols {
+		if i > 0 {
+			ids = append(ids, ',')
+		}
+
+		ids = append(ids, col.ID...)
+	}
+
+	return string(ids)
+}

lefthook.yml

@@ -38,14 +38,10 @@ pre-commit:
       glob: "*.go"
       run: ./scripts/bindings-check.sh
 
-    # The docs document make targets; a stale one sends an agent — or a
-    # contributor reading CONTRIBUTING.md — off a cliff with total
-    # confidence.  The glob is the script's own scanned set, because a
-    # hook that does not fire on a file the check reads is the drift the
-    # check exists to prevent: it was `{Makefile,.pi/**/*.md}` while the
-    # script already read CLAUDE.md.  Instant.
+    # .pi/ documents make targets; a stale one sends an agent off a
+    # cliff with total confidence.  Instant.
     skill-check:
-      glob: "{Makefile,.pi/**/*.md,AGENTS.md,CLAUDE.md,README.md,CONTRIBUTING.md}"
+      glob: "{Makefile,.pi/**/*.md}"
       run: ./scripts/skill-check.sh
 
     frontend-typecheck:

scripts/skill-check.sh

@@ -14,11 +14,6 @@
 # missing: CLAUDE.md names 27 targets and nothing verified one of them,
 # so the file the agents trust most was the file least checked.
 #
-# README.md and CONTRIBUTING.md are in it too, and the header sentence
-# above is why: a person who has *not* read the Makefile goes looking in
-# the contributor-facing doc, so a renamed target sends them off the
-# same cliff it sends an agent off.  CONTRIBUTING.md names 21 targets.
-#
 # **AGENTS.md is a symlink to CLAUDE.md.**  This repo is worked on by
 # two agent harnesses that read different files by convention — Claude
 # Code reads CLAUDE.md, others read AGENTS.md — and two harnesses
@@ -48,19 +43,7 @@ if [ -e AGENTS.md ] || [ -L AGENTS.md ]; then
 	fi
 fi
 
-# The scan is over the docs that are actually there: a checkout without
-# .pi/ still has README.md and CONTRIBUTING.md to check, and gating the
-# whole run on .pi/ would have made the human-facing half conditional on
-# the agent-facing one.  This list is used twice — once to read the
-# mentions out and once to say which file a missing target came from —
-# because a second list is a second thing to forget.
-# `ls` exits non-zero when *any* of its arguments is missing while still
-# printing the ones that are there, and under `set -e` that would sink
-# the assignment rather than scanning what exists, so swallow it.
-docs="$({ find .pi -name '*.md' 2>/dev/null
-	ls CLAUDE.md README.md CONTRIBUTING.md 2>/dev/null || true; })"
-
-[ -n "$docs" ] || exit 0
+[ -d .pi ] || exit 0
 
 # `make -pq` prints the database including every rule, without running
 # anything.  It exits non-zero when a target is out of date, and under
@@ -85,7 +68,7 @@ targets="$({ make -pqRr 2>/dev/null || true; } |
 # AGENTS.md is deliberately not in this list: it is a symlink to
 # CLAUDE.md, asserted above, so scanning it would report every failure
 # twice under two names.
-mentioned="$(printf '%s\n' "$docs" |
+mentioned="$({ find .pi -name '*.md' 2>/dev/null; echo CLAUDE.md; } |
 	xargs awk '
 		FNR == 1 { fence = 0 }
 		/^```/   { fence = !fence; next }
@@ -110,10 +93,10 @@ for t in $mentioned; do
 done
 
 if [ -n "$missing" ]; then
-	echo "skill-check: the docs name make targets that do not exist:" >&2
+	echo "skill-check: the agent docs name make targets that do not exist:" >&2
 	for t in $missing; do
 		echo "  make $t" >&2
-		printf '%s\n' "$docs" | xargs grep -ln "make $t" | sed 's/^/      /' >&2
+		grep -rln "make $t" .pi CLAUDE.md --include='*.md' | sed 's/^/      /' >&2
 	done
 	echo "Fix the docs, or restore the target." >&2
 	exit 1