A wrong URL answered 500, and only the real database said so

/api/admin/clients/not-a-uuid/sites returned 500. `c.id = $1::uuid` makes
Postgres cast the path segment, and casting a malformed string - or the
empty one the shape check handed back in its place - is an ERROR, not a
miss. `c.id::text = $1` cannot fail: an id that is not a uuid matches
nothing, which is the 404 a wrong URL should get.

The two sibling resolvers were already written this way and correctly
404'd the same input. I applied the rule to two of three places, which is
the shape of a rule that holds until somebody adds the next write path.
The shape check is gone with it - it existed only to produce the empty
string that then broke the cast.

The in-memory fake could not have caught this and did not: it resolves a
merchant with a map lookup, so every handler test passed, including the
one named for the case. That test stays, because 404-not-500 is still the
contract, but the property belongs to Postgres - so
api_admin_monitor_live_test.go asserts it where it lives, over every
free-text identifier these queries take. It skips without
TEST_DATABASE_URL, like the rest of the live store tests.

Also in deploy.sh, found by reading its own output: step 3 reported the
WRONG backup. `ls | tail -1` sorts alphabetically, so pre-...-demo-12
sorts before pre-...-demo-6 and it printed a dump from four days earlier.
A deploy that names the wrong safety net is worse than one that names
none, because that is the file somebody reaches for at the worst possible
moment. It echoes the filename it just wrote, and refuses to continue on
an empty one - pipefail catches a failing pg_dump, but a zero-byte gzip
would still have satisfied it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGcjxF1cNLcuwc3DAPcnfj

server/deploy.sh

@@ -58,7 +58,19 @@ git rev-parse HEAD | "${SSH[@]}" "cat > $REMOTE_DIR/release/$VERSION/GIT_SHA"
 if [ "${DRY_RUN:-}" != "" ]; then echo "DRY_RUN: shipped to $REMOTE_DIR/release/$VERSION, nothing changed"; exit 0; fi
 
 step "3. Back up the database"
-"${SSH[@]}" "docker exec behavision-db sh -c 'PGPASSWORD=\$POSTGRES_PASSWORD pg_dump -U behavision -d behavision' | gzip > $REMOTE_DIR/backups/pre-$VERSION-\$(date +%Y%m%d-%H%M%S).sql.gz && ls -la $REMOTE_DIR/backups | tail -1"
+# The dump's own filename is echoed, not `ls | tail -1`, which reported the
+# WRONG file: ls sorts alphabetically, so pre-...-demo-12-... sorts before
+# pre-...-demo-6-... and the line printed a backup from four days earlier. A
+# deploy that names the wrong safety net is worse than one that names none -
+# that is the file somebody reaches for at the worst possible moment.
+#
+# Bare `-s` on the dump so an empty or failed one cannot be reported as a
+# backup: pg_dump exiting non-zero already fails the pipeline under pipefail,
+# but a zero-byte gzip would still satisfy it.
+"${SSH[@]}" "set -e; f=$REMOTE_DIR/backups/pre-$VERSION-\$(date +%Y%m%d-%H%M%S).sql.gz; \
+  docker exec behavision-db sh -c 'PGPASSWORD=\$POSTGRES_PASSWORD pg_dump -U behavision -d behavision' | gzip > \$f; \
+  [ -s \$f ] || { echo 'backup is empty - refusing to continue' >&2; exit 1; }; \
+  ls -la \$f"
 
 step "4. Image"
 "${SSH[@]}" "cd $REMOTE_DIR/release/$VERSION && docker build -q -t behavision-backend:$VERSION -f Dockerfile.runtime . && docker tag behavision-backend:$VERSION behavision-backend:latest"

server/internal/api/handlers_admin_monitor.go

@@ -22,7 +22,7 @@ import "net/http"
 // precisely what an admin opens the console to look at, and hiding it would
 // make the one screen that can fix it the one screen that cannot see it.
 func (s *Server) clientForAdmin(w http.ResponseWriter, r *http.Request) (ClientDetail, bool) {
-	c, err := s.Store.ClientDetail(r.Context(), pathUUID(r, "id"))
+	c, err := s.Store.ClientDetail(r.Context(), r.PathValue("id"))
 	if err != nil {
 		s.serverError(w, "admin client", err)
 		return ClientDetail{}, false
@@ -179,15 +179,3 @@ func (s *Server) auditAdminRead(r *http.Request, clientID, action, entity, entit
 		Detail: map[string]any{"path": r.URL.Path, "rows": n},
 	})
 }
-
-// pathUUID reads a path segment that must be a uuid, returning "" otherwise so
-// the lookup misses and the caller answers 404. Handing a malformed string to
-// Postgres as a uuid is an error, not a miss, and would surface as a 500 on
-// what is really just a wrong URL.
-func pathUUID(r *http.Request, name string) string {
-	v := r.PathValue(name)
-	if !looksLikeUUID(v) {
-		return ""
-	}
-	return v
-}

server/internal/store/api_admin_monitor.go

@@ -22,6 +22,15 @@ import (
 // ClientDetail is one merchant, with the owner a support conversation starts
 // from. ListClients cannot carry it: an owner lookup per row would be a query
 // per merchant on a screen that only needs the name.
+//
+// `c.id::text = $1`, for the same reason the two resolvers below use it, and
+// this one learned it the hard way: as `c.id = $1::uuid` it answered 500 to
+// /api/admin/clients/not-a-uuid/sites on the first real database, because
+// casting a malformed string - or the empty one a shape check hands back - to
+// uuid is an ERROR in Postgres rather than a miss. Comparing the column as
+// text cannot fail: an id that is not a uuid simply matches nothing, which is
+// the 404 a wrong URL should get. The sibling queries were already written
+// this way and correctly 404'd; only this one was not.
 func (s *Store) ClientDetail(ctx context.Context, clientID string) (api.ClientDetail, error) {
 	var c api.ClientDetail
 	var at time.Time
@@ -36,7 +45,7 @@ func (s *Store) ClientDetail(ctx context.Context, clientID string) (api.ClientDe
 		                  WHERE au.client_id = c.id AND au.role = 'owner'
 		                    AND au.active ORDER BY au.created_at LIMIT 1), '')
 		  FROM clients c
-		 WHERE c.id = $1::uuid`, clientID).
+		 WHERE c.id::text = $1`, clientID).
 		Scan(&c.ID, &c.Slug, &c.Name, &c.Active, &at, &c.Sites, &c.Users,
 			&c.OwnerEmail, &c.OwnerName)
 	if errors.Is(err, pgx.ErrNoRows) {

server/internal/store/api_admin_monitor_live_test.go

@@ -0,0 +1,57 @@
+package store
+
+import (
+	"context"
+	"testing"
+)
+
+// A malformed identifier must MISS, never error.
+//
+// This exists because the in-memory fake cannot catch it and did not. The API
+// fake resolves a merchant with a map lookup, so every handler test passed
+// while the real query answered 500 to /api/admin/clients/not-a-uuid/sites:
+// `c.id = $1::uuid` makes Postgres cast the path segment, and casting a
+// malformed string - or the empty one a shape check hands back in its place -
+// is an ERROR rather than no match. Comparing the column as text cannot fail.
+//
+// Kept as a live test on purpose. There is no way to assert this against a
+// fake: the property belongs to Postgres, and a fake that reproduced it would
+// be a second implementation of the thing under test.
+func TestLiveMalformedIdentifiersMissRatherThanError(t *testing.T) {
+	st := liveStore(t)
+	ctx := context.Background()
+
+	for _, id := range []string{"", "not-a-uuid", "'; DROP TABLE clients;--", "42"} {
+		c, err := st.ClientDetail(ctx, id)
+		if err != nil {
+			t.Errorf("ClientDetail(%q): %v - a wrong URL must be a miss, not a 500", id, err)
+		}
+		if c.ID != "" {
+			t.Errorf("ClientDetail(%q) matched %q", id, c.ID)
+		}
+	}
+
+	// The sibling resolvers take a real client id and a free-text reference, so
+	// the reference is the part a caller controls and the part that must not
+	// blow up. A malformed CLIENT id here is covered above.
+	const noClient = "00000000-0000-4000-8000-000000000000"
+	for _, ref := range []string{"", "not-a-uuid", "'; --", "42"} {
+		if id, err := st.AdminSiteID(ctx, noClient, ref); err != nil {
+			t.Errorf("AdminSiteID(%q): %v", ref, err)
+		} else if id != "" {
+			t.Errorf("AdminSiteID(%q) matched %q", ref, id)
+		}
+		if id, err := st.AdminCameraID(ctx, noClient, noClient, ref); err != nil {
+			t.Errorf("AdminCameraID(%q): %v", ref, err)
+		} else if id != "" {
+			t.Errorf("AdminCameraID(%q) matched %q", ref, id)
+		}
+	}
+
+	// And a sale id is the same shape of input on the tenant side.
+	if s, err := st.Sale(ctx, noClient, "not-a-uuid"); err != nil {
+		t.Errorf("Sale(not-a-uuid): %v", err)
+	} else if s.ID != "" {
+		t.Errorf("Sale(not-a-uuid) matched %q", s.ID)
+	}
+}