diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..2aa64fa --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,35 @@ +# The unit gate. `node --test` over the pure modules in lib/, no framework. +# +# Until MX-835 this repo had NO test file, so `typecheck` was the whole story: +# it proved the code COMPILES and never executed a line of it. That is exactly +# how MX-835 shipped — `batteryFromSi` returned `{ minutesRemaining: undefined }` +# on a Mac plugged into the wall, which types perfectly under a `.optional()` +# field and which bb's RPC serialiser then refused as "not a JSON value", +# failing the whole `current` call and blanking the panel. +# +# What this job covers: lib/*.test.ts, which is the pure half of the plugin +# (battery-state mapping). What it does NOT cover: server.ts and app.tsx, whose +# code needs a live bb host. Green here means the pure modules behave, not that +# the plugin works. +# +# `--ignore-scripts` on the install: better-sqlite3 is a devDependency needed +# only for its types, and its native rebuild is minutes of CI for nothing. +name: Test + +on: + pull_request: + push: + branches: [main, main-public] + workflow_dispatch: + +jobs: + test: + name: node --test lib/ + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: "22" + - run: npm ci --no-audit --no-fund --ignore-scripts + - run: npm test diff --git a/.github/workflows/typecheck.yml b/.github/workflows/typecheck.yml index 48b0ec0..206bfb1 100644 --- a/.github/workflows/typecheck.yml +++ b/.github/workflows/typecheck.yml @@ -11,9 +11,10 @@ # # What a green run here covers: `tsc --noEmit` under `strict` with # `skipLibCheck: false`, over server.ts, app.tsx, components, lib, hooks and -# types. What it does NOT cover: this repo has ZERO test files, so no line of -# it is ever EXECUTED in CI. Green means it compiles, not that it works — a -# weaker claim than the sibling plugins' Test job, and deliberately named so. +# types. What it does NOT cover: whether any of it WORKS. Green means it +# compiles — MX-835 was a type-correct `undefined` that bb's RPC serialiser +# refused at runtime, and it passed this job. Execution lives in the sibling +# `Test` job (.github/workflows/test.yml), which covers lib/ only. name: Typecheck on: diff --git a/lib/battery.test.ts b/lib/battery.test.ts new file mode 100644 index 0000000..31d7fe6 --- /dev/null +++ b/lib/battery.test.ts @@ -0,0 +1,150 @@ +// The one thing this file exists to prevent: a battery state carrying a key +// whose value is `undefined`. +// +// bb's RPC serialiser walks the result and refuses `undefined` — it is not a +// JSON value — and it fails the WHOLE call, not the field. On 2026-09-09 this +// Mac, sitting on AC power with no time estimate, produced +// +// system@0.1.0 running (rpc current failed: rpc result at +// $result.sample.battery.minutesRemaining is not a JSON value (undefined)) +// +// and the System panel rendered nothing at all (MX-835). `JSON.stringify` +// DROPS such keys, so `encodeBattery`/`decodeBattery` round-trip clean and no +// persisted-path test could ever have caught it; the assertion below is on the +// live object, and it is `assertJsonValue` — the serialiser's rule, not a spot +// check on one field name, because the next field added is the next outage. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + asMinutes, + asPct, + batteryFromRemote, + batteryFromSi, + decodeBattery, + encodeBattery, + presentBattery, + withBattery, +} from "./battery.ts"; + +/** bb's rule: every value reachable from the result must be a JSON value. */ +function assertJsonValue(value: unknown, path = "$result"): void { + assert.notEqual(value, undefined, `${path} is not a JSON value (undefined)`); + if (Array.isArray(value)) { + value.forEach((v, i) => assertJsonValue(v, `${path}[${i}]`)); + } else if (value !== null && typeof value === "object") { + for (const [k, v] of Object.entries(value)) assertJsonValue(v, `${path}.${k}`); + } +} + +// Measured on mgrin's MacBook 2026-09-09T01:18Z, plugged in: IOKit reports +// timeRemaining 65535 (0xFFFF, "no estimate") and systeminformation passes it +// through verbatim. This is the exact reading that broke `current`. +const MAC_ON_AC = { + hasBattery: true, + cycleCount: 151, + isCharging: false, + designedCapacity: 74742, + maxCapacity: 63271, + currentCapacity: 62453, + voltage: 12.976, + capacityUnit: "mWh", + percent: 100, + timeRemaining: 65535, + acConnected: true, + type: "Li-ion", + model: "bq40z651", + manufacturer: "Apple", + serial: "REDACTED", +}; + +test("MX-835: a Mac on AC with no time estimate serialises — no undefined key", () => { + const state = batteryFromSi(MAC_ON_AC); + assertJsonValue({ sample: { battery: state } }); + assert.equal("minutesRemaining" in (state as object), false); + assert.deepEqual(state, { + present: true, + charging: false, + acConnected: true, + pct: 100, + cycleCount: 151, + healthPct: 85, + }); +}); + +test("a charging battery drops minutesRemaining rather than holding undefined", () => { + const state = batteryFromSi({ ...MAC_ON_AC, isCharging: true, timeRemaining: 42 }); + assertJsonValue(state); + assert.equal("minutesRemaining" in (state as object), false); + assert.equal((state as { charging: boolean }).charging, true); +}); + +test("a discharging battery with a real estimate keeps it", () => { + const state = batteryFromSi({ ...MAC_ON_AC, acConnected: false, timeRemaining: 137 }); + assertJsonValue(state); + assert.equal((state as { minutesRemaining?: number }).minutesRemaining, 137); +}); + +test("every optional reading is omitted, not undefined, when unavailable", () => { + const state = batteryFromSi({ hasBattery: true, acConnected: false }); + assertJsonValue(state); + assert.deepEqual(state, { present: true, charging: false, acConnected: false }); + assert.deepEqual(Object.keys(state as object).sort(), ["acConnected", "charging", "present"]); +}); + +test("ABSENT and UNKNOWN stay distinct, and neither is a number", () => { + assert.deepEqual(batteryFromSi({ hasBattery: false }), { present: false }); + assert.equal(batteryFromSi({}), undefined); // hasBattery not a boolean => UNKNOWN + assert.equal(batteryFromSi(null), undefined); +}); + +test("the remote sampler omits minutes it did not emit", () => { + const state = batteryFromRemote( + new Map([["battery_present", "1"], ["battery_pct", "77"], ["battery_ac", "1"]]), + ); + assertJsonValue({ sample: { battery: state } }); + assert.deepEqual(state, { present: true, charging: false, acConnected: true, pct: 77 }); +}); + +test("the remote sampler: no key is UNKNOWN, battery_present=0 is ABSENT", () => { + assert.equal(batteryFromRemote(new Map()), undefined); + assert.deepEqual(batteryFromRemote(new Map([["battery_present", "0"]])), { present: false }); +}); + +test("withBattery omits the sample key entirely when the state is UNKNOWN", () => { + const base = { ts: 1, cpuPct: 3 }; + const unknown = withBattery(base, undefined); + assertJsonValue({ sample: unknown }); + assert.equal("battery" in unknown, false); + assert.deepEqual(withBattery(base, { present: false }), { ts: 1, cpuPct: 3, battery: { present: false } }); +}); + +test("presentBattery never writes a key it was given as undefined", () => { + const state = presentBattery( + { charging: false, acConnected: false }, + { pct: 5, minutesRemaining: undefined, cycleCount: undefined, healthPct: undefined }, + ); + assertJsonValue(state); + assert.deepEqual(Object.keys(state).sort(), ["acConnected", "charging", "pct", "present"]); +}); + +test("65535 and -1 are refused as estimates; a plausible figure is kept", () => { + assert.equal(asMinutes(65535), undefined); + assert.equal(asMinutes(-1), undefined); + assert.equal(asMinutes(0), undefined); + assert.equal(asMinutes(1439), 1439); + assert.equal(asMinutes(1440), undefined); +}); + +test("a percentage outside 0..100 is not clamped into range", () => { + assert.equal(asPct(-1), undefined); + assert.equal(asPct(101), undefined); + assert.equal(asPct(50.4), 50); +}); + +test("encode/decode round-trips the live state, UNKNOWN stays NULL", () => { + const state = batteryFromSi(MAC_ON_AC)!; + assert.deepEqual(decodeBattery(encodeBattery(state)), state); + assert.equal(encodeBattery(undefined), null); + assert.equal(decodeBattery(null), undefined); + assert.equal(decodeBattery('{"present":"maybe"}'), undefined); // corrupt cell => UNKNOWN +}); diff --git a/lib/battery.ts b/lib/battery.ts index 68b763b..4d0a885 100644 --- a/lib/battery.ts +++ b/lib/battery.ts @@ -19,6 +19,19 @@ // emitted coerces to 0, and `|| 0` collapses a genuine 0 into the same 0. A // battery routed through it turns a desktop into a dying laptop. Nothing here // uses it; absence stays `undefined` the whole way through. +// +// One more rule, and it is the one that took the plugin down (MX-835): an +// optional field that has no reading is an ABSENT KEY, never a present key +// holding `undefined`. bb's RPC serialiser refuses `undefined` — it is not a +// JSON value — and it refuses the WHOLE call, not the field, so a Mac sitting +// on AC power with no time estimate returned +// rpc current failed: $result.sample.battery.minutesRemaining is not a JSON +// value (undefined) +// and the panel rendered nothing at all. The persisted path never saw it +// because `JSON.stringify` drops such keys silently, which is exactly why the +// bug reached a running machine: encode/decode round-trips clean, the live +// object does not. Build the optional fields through `presentBattery` / +// `withBattery` below rather than assigning them directly. import { z } from "zod"; export const batteryStateShape = z.discriminatedUnion("present", [ @@ -36,6 +49,40 @@ export const batteryStateShape = z.discriminatedUnion("present", [ }), ]); export type BatteryState = z.infer; +export type PresentBattery = Extract; + +/** + * The PRESENT state, carrying only the optional readings that actually have a + * value. A key holding `undefined` is not a JSON value and bb's RPC serialiser + * rejects the entire response over one — see the module header (MX-835). + */ +export function presentBattery( + base: { charging: boolean; acConnected: boolean }, + optional: { + pct?: number | undefined; + minutesRemaining?: number | undefined; + cycleCount?: number | undefined; + healthPct?: number | undefined; + }, +): PresentBattery { + const state: PresentBattery = { present: true, ...base }; + for (const [key, value] of Object.entries(optional)) { + if (value !== undefined) (state as Record)[key] = value; + } + return state; +} + +/** + * Attach a battery state to a sample, or leave the key off entirely when the + * state is UNKNOWN. Same rule as `presentBattery`, one level up: `battery` + * is optional on the sample too, so `{ battery: undefined }` fails the same way. + */ +export function withBattery( + base: T, + state: BatteryState | undefined, +): T & { battery?: BatteryState } { + return state === undefined ? base : { ...base, battery: state }; +} const finite = (v: unknown): number | undefined => typeof v === "number" && Number.isFinite(v) ? v : undefined; @@ -67,19 +114,19 @@ export function batteryFromSi(raw: unknown): BatteryState | undefined { const charging = b.isCharging === true; const designed = finite(b.designedCapacity); const max = finite(b.maxCapacity); - return { - present: true, - pct: asPct(b.percent), - charging, - acConnected: b.acConnected === true, - // A "time remaining" while charging is an estimate of the wrong thing. - minutesRemaining: charging ? undefined : asMinutes(b.timeRemaining), - cycleCount: finite(b.cycleCount), - healthPct: - designed !== undefined && max !== undefined && designed > 0 - ? Math.round((max / designed) * 100) - : undefined, - }; + return presentBattery( + { charging, acConnected: b.acConnected === true }, + { + pct: asPct(b.percent), + // A "time remaining" while charging is an estimate of the wrong thing. + minutesRemaining: charging ? undefined : asMinutes(b.timeRemaining), + cycleCount: finite(b.cycleCount), + healthPct: + designed !== undefined && max !== undefined && designed > 0 + ? Math.round((max / designed) * 100) + : undefined, + }, + ); } /** @@ -98,13 +145,13 @@ export function batteryFromRemote(values: Map): BatteryState | u return Number.isFinite(n) ? n : undefined; }; const charging = values.get("battery_charging") === "1"; - return { - present: true, - pct: asPct(numeric("battery_pct")), - charging, - acConnected: values.get("battery_ac") === "1", - minutesRemaining: charging ? undefined : asMinutes(numeric("battery_minutes")), - }; + return presentBattery( + { charging, acConnected: values.get("battery_ac") === "1" }, + { + pct: asPct(numeric("battery_pct")), + minutesRemaining: charging ? undefined : asMinutes(numeric("battery_minutes")), + }, + ); } /** NULL in the column means UNKNOWN, which is what every pre-migration row is. */ diff --git a/package.json b/package.json index a8b3171..fc063ec 100644 --- a/package.json +++ b/package.json @@ -3,6 +3,7 @@ "version": "0.1.0", "type": "module", "scripts": { + "test": "node --test --experimental-strip-types --test-timeout=10000 lib/*.test.ts", "typecheck": "tsc --noEmit" }, "engines": { diff --git a/server.ts b/server.ts index 8b1f168..ae01c52 100644 --- a/server.ts +++ b/server.ts @@ -29,6 +29,7 @@ import { batteryStateShape, decodeBattery, encodeBattery, + withBattery, } from "./lib/battery"; const run = promisify(execFile); @@ -56,6 +57,10 @@ const sampleShape = z.object({ // Optional on purpose, at every layer: absent means the sampler could not // tell, which is a different answer from "this machine has no battery" // ({ present: false }) and from any number at all. See lib/battery.ts. + // + // Optional means the KEY IS ABSENT. Every construction of a Sample goes + // through `withBattery`, because `{ battery: undefined }` is not JSON and + // bb's RPC serialiser fails the whole `current` call over it (MX-835). battery: batteryStateShape.optional(), }); type Sample = z.infer; @@ -387,7 +392,7 @@ export default async function plugin(bb: BbPluginApi) { ]); const dataVol = fs.find((f) => f.mount === "/System/Volumes/Data") ?? fs.find((f) => f.mount === "/") ?? fs[0]; - return { + return withBattery({ ts: Date.now(), cpuPct: Number(load.currentLoad.toFixed(1)), // si reports avgLoad PER CORE; store the raw 1-minute load so the number @@ -402,8 +407,7 @@ export default async function plugin(bb: BbPluginApi) { swapUsedMb: Math.round((mem.swapused ?? 0) / 1048576), diskTotalGb: dataVol ? Math.round(dataVol.size / 1073741824) : 0, diskUsedGb: dataVol ? Math.round(dataVol.used / 1073741824) : 0, - battery, - }; + }, battery); } const insert = (hostId: string, s: Sample) => { @@ -423,7 +427,7 @@ export default async function plugin(bb: BbPluginApi) { const rowToSample = (r: Record): Sample => { const count = Number(r.cpu_count) || 1; const load1 = Number(r.load1); - return { + return withBattery({ ts: Number(r.ts), // Pre-migration rows have no cpu_pct; fall back to the old (wrong but // present) load-derived figure so the sparkline has no hole. @@ -442,8 +446,7 @@ export default async function plugin(bb: BbPluginApi) { diskUsedGb: Number(r.disk_used_gb), // Same shape as cpu_pct above: a missing column is a stated absence, not // Number(null) === 0. - battery: decodeBattery(r.battery), - }; + }, decodeBattery(r.battery)); }; const latest = (hostId: string): Sample | null => { @@ -579,7 +582,7 @@ export default async function plugin(bb: BbPluginApi) { const num = (key: string) => Number(values.get(key)) || 0; const memTotalMb = Math.round(num("mem_total_kb") / 1024); const memUsedMb = Math.round(num("mem_used_kb") / 1024); - const sample: Sample = { + const sample: Sample = withBattery({ ts: Date.now(), cpuPct: Math.min(100, Math.max(0, num("cpu_pct"))), load1: num("load1"), @@ -594,8 +597,7 @@ export default async function plugin(bb: BbPluginApi) { diskUsedGb: Math.round(num("disk_used_kb") / 1048576), // Deliberately NOT num(): a remote host may legitimately be a desktop, // and num() would render that as 0%. - battery: batteryFromRemote(values), - }; + }, batteryFromRemote(values)); return { sample, topCpu, topMem, uptime: values.get("uptime") ?? "" }; } finally { // Always reap the one-shot terminal — success, failure, or abort. On diff --git a/tsconfig.json b/tsconfig.json index 92fe185..74d3f7b 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -18,6 +18,9 @@ ] }, "noEmit": true, + // The test files import siblings by their real `./x.ts` path, which is + // what `node --test --experimental-strip-types` resolves at runtime. + "allowImportingTsExtensions": true, "skipLibCheck": false }, "include": [