ci(skill-check): scan the docs a contributor reads

The check asserts that every make target named in a doc exists, and its
scanned set was .pi/ plus CLAUDE.md.  Since #50, CONTRIBUTING.md is the
document a *human* goes to for a build command, and it names 21 targets
that nothing verified; README.md names none today and is in for the same
reason.  The script's own header sentence is the argument — a renamed
target sends a person off the same cliff it sends an agent off.

The file list is now one `docs` variable used twice, because the failure
message carried a second copy of it and a second list is a second thing
to forget.  The `[ -d .pi ]` guard went with it: gating the whole run on
.pi/ would make the human-facing half conditional on the agent-facing
one, and an empty list is the same "nothing to scan" exit without the
coupling.

The lefthook glob is that scanned set now rather than
{Makefile,.pi/**/*.md} — #220's smaller half, and it did not fire on
CLAUDE.md either, which the script had read for months.

Verified by planting a bad target rather than by reading the diff: both
matched forms in each of the four scanned surfaces, each naming the
right file; the same two plants pass on the pre-change script; unfenced
prose still does not match; and the hook fires on a staged
CONTRIBUTING.md under the new glob where the old one skipped it.  The
count is unchanged at 47 — the set is a union — so coverage is the only
thing that moved.

Closes #220

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 .pi/ documents a make target that doesn't exist
+make skill-check      # Fail if a doc names 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,28 +662,6 @@ 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 agent docs name a missing make target, or AGENTS.md is not a symlink
+skill-check: ## Fail if the 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,24 +303,6 @@ 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.
@@ -378,14 +360,11 @@ 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,
 		)
@@ -476,12 +455,9 @@ 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,
 		)
@@ -512,12 +488,9 @@ 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,
 		)
@@ -571,12 +544,9 @@ 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,
 		)
@@ -621,12 +591,9 @@ 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,
 		)
@@ -834,12 +801,9 @@ 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,
 		)
@@ -937,12 +901,9 @@ 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

@@ -1,262 +0,0 @@
-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,10 +38,14 @@ pre-commit:
       glob: "*.go"
       run: ./scripts/bindings-check.sh
 
-    # .pi/ documents make targets; a stale one sends an agent off a
-    # cliff with total confidence.  Instant.
+    # 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.
     skill-check:
-      glob: "{Makefile,.pi/**/*.md}"
+      glob: "{Makefile,.pi/**/*.md,AGENTS.md,CLAUDE.md,README.md,CONTRIBUTING.md}"
       run: ./scripts/skill-check.sh
 
     frontend-typecheck:

scripts/skill-check.sh

@@ -14,6 +14,11 @@
 # 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
@@ -43,7 +48,19 @@ if [ -e AGENTS.md ] || [ -L AGENTS.md ]; then
 	fi
 fi
 
-[ -d .pi ] || exit 0
+# 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
 
 # `make -pq` prints the database including every rule, without running
 # anything.  It exits non-zero when a target is out of date, and under
@@ -68,7 +85,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="$({ find .pi -name '*.md' 2>/dev/null; echo CLAUDE.md; } |
+mentioned="$(printf '%s\n' "$docs" |
 	xargs awk '
 		FNR == 1 { fence = 0 }
 		/^```/   { fence = !fence; next }
@@ -93,10 +110,10 @@ for t in $mentioned; do
 done
 
 if [ -n "$missing" ]; then
-	echo "skill-check: the agent docs name make targets that do not exist:" >&2
+	echo "skill-check: the docs name make targets that do not exist:" >&2
 	for t in $missing; do
 		echo "  make $t" >&2
-		grep -rln "make $t" .pi CLAUDE.md --include='*.md' | sed 's/^/      /' >&2
+		printf '%s\n' "$docs" | xargs grep -ln "make $t" | sed 's/^/      /' >&2
 	done
 	echo "Fix the docs, or restore the target." >&2
 	exit 1