redo and move rover ranking a little

server/src/services/assignmentService/index.js

@@ -7,6 +7,7 @@ const logger = require('../../globals/logger').child('assignment');
 const { MODES, getMode, modeEvents } = require('../modeManager');
 const { roleEvents, getRole, isAdmin, isLockdownAdmin } = require('../roleService');
 const roverManager = require('../roverManager');
+const { compareRoversForAssignment } = require('./roverRanking');
 
 const socketRefs = new Map(); // socketId -> socket
 const assignments = new Map(); // socketId -> roverId
@@ -241,35 +242,16 @@ function pickRover(socket, options = {}) {
   if (candidates.length === 0) {
     return null;
   }
-  const dockedRank = (rover) => {
-    if (!rover) return 0;
-    if (rover.docked === true) return -1;
-    if (rover.docked === false) return 1;
-    const sensors = rover.lastSensor?.decoded || rover.lastSensor?.sensors || null;
-    const docked = sensors?.chargingSources?.homeBase;
-    if (docked === true) return -1;
-    if (docked === false) return 1;
-    return 0;
-  };
-  const idleRank = (rover) => (rover?.drivers?.size === 0 ? 1 : 0);
-  const compare = (a, b) => {
-    const aEmpty = idleRank(a);
-    const bEmpty = idleRank(b);
-    if (aEmpty !== bEmpty) return bEmpty - aEmpty;
-    const aDockRank = dockedRank(a);
-    const bDockRank = dockedRank(b);
-    if (aEmpty === 1 && aDockRank !== bDockRank) {
-      return bDockRank - aDockRank;
-    }
-    if (a.drivers.size !== b.drivers.size) {
-      return a.drivers.size - b.drivers.size;
-    }
-    return bDockRank - aDockRank;
-  };
-  candidates.sort(compare);
+  /*
+    Eligibility is resolved above, while this shared comparator owns only the
+    requested placement order: undocked-and-empty, battery, then driver count.
+    Keeping those concerns separate prevents a ranking change from weakening
+    lock, private-rover, role, or mode access checks.
+  */
+  candidates.sort(compareRoversForAssignment);
   const best = candidates[0];
   if (!best) return null;
-  const bestTier = candidates.filter((entry) => compare(entry, best) === 0);
+  const bestTier = candidates.filter((entry) => compareRoversForAssignment(entry, best) === 0);
   if (!bestTier.length) return best;
   return bestTier[Math.floor(Math.random() * bestTier.length)] || best;
 }

server/src/services/assignmentService/roverRanking.js

@@ -0,0 +1,65 @@
+// Rover assignment ranking
+// Purpose: Ranks otherwise eligible rovers using the fleet's assignment priorities.
+// Scope: Contains only deterministic comparison logic; access checks and the final random tie-break remain in assignmentService.
+
+function readDockedState(rover) {
+  /*
+    The rover record normally exposes the server's canonical docked state. The
+    sensor fallback covers the short interval where telemetry has arrived but
+    the derived top-level field has not yet been synchronized. Unknown docking
+    state deliberately remains unknown instead of being treated as undocked.
+  */
+  if (rover?.docked === true || rover?.docked === false) return rover.docked;
+  const sensors = rover?.lastSensor?.decoded || rover?.lastSensor?.sensors || null;
+  const homeBase = sensors?.chargingSources?.homeBase;
+  return homeBase === true || homeBase === false ? homeBase : null;
+}
+
+function driverCount(rover) {
+  /*
+    Production rover records use a Set. Returning a safe high-level count here
+    keeps ranking predictable for partially initialized records and makes the
+    comparator straightforward to exercise with small test fixtures.
+  */
+  return Number.isFinite(rover?.drivers?.size) ? rover.drivers.size : 0;
+}
+
+function batteryPercentage(rover) {
+  /*
+    percentDisplay is the canonical server-normalized percentage used by the
+    rest of the application. Missing or invalid telemetry receives no invented
+    percentage; the comparator places unknown batteries after every known one.
+  */
+  const percentage = rover?.batteryState?.percentDisplay;
+  return Number.isFinite(percentage) ? percentage : null;
+}
+
+function compareRoversForAssignment(left, right) {
+  /*
+    An undocked rover with nobody assigned is the most useful placement because
+    it starts a fresh driving session without adding another user to a queue.
+    Both conditions must be true to receive this first-priority rank.
+  */
+  const leftReadyAndEmpty = readDockedState(left) === false && driverCount(left) === 0;
+  const rightReadyAndEmpty = readDockedState(right) === false && driverCount(right) === 0;
+  if (leftReadyAndEmpty !== rightReadyAndEmpty) return leftReadyAndEmpty ? -1 : 1;
+
+  const leftBattery = batteryPercentage(left);
+  const rightBattery = batteryPercentage(right);
+  const leftHasBattery = leftBattery != null;
+  const rightHasBattery = rightBattery != null;
+  if (leftHasBattery !== rightHasBattery) return leftHasBattery ? -1 : 1;
+  if (leftHasBattery && leftBattery !== rightBattery) return rightBattery - leftBattery;
+
+  /*
+    Battery-equivalent rovers are balanced by current assignment load. Returning
+    zero after this comparison is intentional: assignmentService randomly picks
+    within that exact best tier so stable Map insertion order does not create a
+    permanent favorite rover.
+  */
+  return driverCount(left) - driverCount(right);
+}
+
+module.exports = {
+  compareRoversForAssignment,
+};

server/src/services/assignmentService/roverRanking.test.js

@@ -0,0 +1,69 @@
+// Rover assignment ranking tests
+// Purpose: Locks the operator-defined rover priority order against accidental comparator regressions.
+// Scope: Tests pure ranking only; assignment side effects and access policy remain owned by their existing services.
+const test = require('node:test');
+const assert = require('node:assert/strict');
+const { compareRoversForAssignment } = require('./roverRanking');
+
+function rover({ id, docked, battery, drivers = 0 }) {
+  /*
+    Set size matches the production rover contract without introducing socket or
+    rover-manager dependencies into these focused ordering tests.
+  */
+  return {
+    id,
+    docked,
+    batteryState: battery == null ? null : { percentDisplay: battery },
+    drivers: new Set(Array.from({ length: drivers }, (_, index) => `${id}-driver-${index}`)),
+  };
+}
+
+function rankedIds(entries) {
+  return entries.sort(compareRoversForAssignment).map((entry) => entry.id);
+}
+
+test('an undocked empty rover outranks every rover that is docked or occupied', () => {
+  const result = rankedIds([
+    rover({ id: 'occupied-high', docked: false, battery: 100, drivers: 1 }),
+    rover({ id: 'docked-high', docked: true, battery: 100 }),
+    rover({ id: 'ready-empty', docked: false, battery: 20 }),
+  ]);
+
+  assert.equal(result[0], 'ready-empty');
+});
+
+test('battery percentage ranks rovers after undocked-and-empty readiness', () => {
+  const result = rankedIds([
+    rover({ id: 'low', docked: false, battery: 35 }),
+    rover({ id: 'high', docked: false, battery: 90 }),
+    rover({ id: 'middle', docked: false, battery: 60 }),
+  ]);
+
+  assert.deepEqual(result, ['high', 'middle', 'low']);
+});
+
+test('known battery percentage outranks missing battery telemetry', () => {
+  const result = rankedIds([
+    rover({ id: 'unknown', docked: true, battery: null }),
+    rover({ id: 'known', docked: true, battery: 5 }),
+  ]);
+
+  assert.deepEqual(result, ['known', 'unknown']);
+});
+
+test('driver count breaks a battery-percentage tie', () => {
+  const result = rankedIds([
+    rover({ id: 'busy', docked: false, battery: 70, drivers: 3 }),
+    rover({ id: 'less-busy', docked: false, battery: 70, drivers: 1 }),
+  ]);
+
+  assert.deepEqual(result, ['less-busy', 'busy']);
+});
+
+test('exactly equivalent rovers remain tied for random selection by assignmentService', () => {
+  const left = rover({ id: 'left', docked: false, battery: 80, drivers: 1 });
+  const right = rover({ id: 'right', docked: false, battery: 80, drivers: 1 });
+
+  assert.equal(compareRoversForAssignment(left, right), 0);
+  assert.equal(compareRoversForAssignment(right, left), 0);
+});