From 95449ff177abdf7d437219b867e225cbe3fd8ef6 Mon Sep 17 00:00:00 2001 From: oveddan Date: Fri, 14 Aug 2026 15:16:01 -0600 Subject: [PATCH] feat: add an update command that reads, diffs, and writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dan: "we should have an apply update method, that looks up the existing ones, and then just applies the deltas... just pull the current state, run an update against it, then update the controller, its that simple." The three-step export/plan/apply flow exists so a plan can be reviewed before it is applied. That is worth having, but it is not what most changes need, and it fails the README's own motivating example. Asking for the top row to be green aborts outright if any one of those knobs is already green, because `createPatchPlan` treats a no-op as a contract violation — which is right for `plan`, where the contract is "the state I described is the state I found", and wrong for an update. `update` reads the controller, works out what actually differs, prints it, and writes only that with --yes. Already-correct values are reported as unchanged and skipped; an update where everything is already set succeeds having done nothing. Verified against hardware: the four-knob request above reports two unchanged and two changes, where `plan` refuses at the first no-op. No plan file changes hands, so there is nothing to forge and nothing to replay. That is why this needs neither the journal nor the single-use machinery `apply` carries, and why it is not behind the #14 gate: it keeps no hidden state, and the backup is a file you name. `planUpdate` rejects two --set operations for one field rather than picking a winner. It delegates to `createPatchPlan` once the no-ops are filtered, so both paths build frames through exactly the same code. Refs #14 Co-Authored-By: Claude Opus 5 --- src/cli.ts | 72 ++++++++++++++++++++++++++++++++++++++++---- src/planner.ts | 48 +++++++++++++++++++++++++++++ test/cli.test.ts | 12 +++++++- test/planner.test.ts | 47 ++++++++++++++++++++++++++++- 4 files changed, 171 insertions(+), 8 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index b8c5c3b..bc6e3e7 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -5,12 +5,12 @@ import { dirname, resolve } from "node:path"; import { exportConfiguration } from "./exporter.js"; import type { ConfigExport } from "./model.js"; -import { createPatchPlan, type PatchInput, type PatchPlan } from "./planner.js"; +import { createPatchPlan, planUpdate, type PatchInput, type PatchPlan } from "./planner.js"; import { applyPatchPlan } from "./applier.js"; import { assertPlanNotConsumed } from "./journal.js"; interface Arguments { - command: "list" | "export" | "plan" | "apply" | "help"; + command: "list" | "export" | "plan" | "apply" | "update" | "help"; device?: number; out?: string; timeoutMs: number; @@ -18,6 +18,7 @@ interface Arguments { sets: string[]; plan?: string; yes: boolean; + backup?: string; } function usage(): string { @@ -25,14 +26,19 @@ function usage(): string { mft-config list [--timeout ] mft-config export [--device ] [--out ] [--timeout ] mft-config plan --snapshot --set [--set ] [--out ] + mft-config update --set [--set ] [--yes] [--backup ] mft-config apply --plan --yes [--device ] (disabled) This tool only sends Universal Identity, global pull (0x02), encoder bulk-pull (0x04/0x01), and device-ID pull (0x05) messages. The plan command is offline. -apply is the only command that writes settings, and it is disabled in this -release while the write-path defects in issue #14 are open. list, export, and -plan are unaffected.`; +update reads the controller, works out what actually differs, and writes only +that. Without --yes it prints the difference and writes nothing. + +apply is a separate flow for a reviewed plan file, and is disabled in this +release while the write-path defects in issue #14 are open. update is not +affected: it keeps no journal and no hidden state, and writes a backup file you +name.`; } function parseArguments(argv: string[]): Arguments { @@ -40,7 +46,9 @@ function parseArguments(argv: string[]): Arguments { if (command === "help" || command === "--help" || command === "-h") { return { command: "help", timeoutMs: 500, sets: [], yes: false }; } - if (command !== "list" && command !== "export" && command !== "plan" && command !== "apply") throw new Error(`Unknown command: ${command}`); + if (command !== "list" && command !== "export" && command !== "plan" && command !== "apply" && command !== "update") { + throw new Error(`Unknown command: ${command}`); + } const result: Arguments = { command, timeoutMs: 500, sets: [], yes: false }; for (let index = 1; index < argv.length; index += 1) { @@ -61,6 +69,9 @@ function parseArguments(argv: string[]): Arguments { } else if (option === "--set" && value !== undefined) { result.sets.push(value); index += 1; + } else if (option === "--backup" && value !== undefined) { + result.backup = value; + index += 1; } else if (option === "--plan" && value !== undefined) { result.plan = value; index += 1; @@ -76,6 +87,7 @@ function parseArguments(argv: string[]): Arguments { if (result.device !== undefined && (!Number.isInteger(result.device) || result.device < 0)) { throw new Error("--device must be a non-negative integer"); } + if (command === "update" && result.sets.length === 0) throw new Error("update requires at least one --set "); if (command === "plan" && !result.snapshot) throw new Error("plan requires --snapshot "); if (command === "plan" && result.sets.length === 0) throw new Error("plan requires at least one --set "); // Before the --plan and --yes checks: a disabled command must not coach the @@ -177,6 +189,54 @@ async function main(): Promise { const device = devices[selectedIndex]; if (!device) throw new Error(`Device index ${selectedIndex} does not exist; use the list command first`); + if (args.command === "update") { + // Read, work out the difference, write only that. No plan file changes + // hands, so there is nothing to forge and nothing to replay — which is why + // this needs neither the journal nor the single-use machinery that `apply` + // carries, and is not gated behind them. + const readConnection = backend.connect(device); + let config: ConfigExport; + try { + config = await exportConfiguration(readConnection, device, { timeoutMs: args.timeoutMs }); + } finally { + readConnection.close(); + } + + const { plan, skipped } = planUpdate(config, args.sets.map(parseSet)); + for (const skip of skipped) process.stdout.write(`unchanged ${skip.path} is already ${String(skip.value)}\n`); + if (!plan) { + process.stdout.write("Nothing to do; every requested value is already set.\n"); + return; + } + for (const change of plan.changes) { + process.stdout.write(`change ${change.path}: ${String(change.expected)} -> ${String(change.desired)}\n`); + } + if (!plan.applyEligibility.eligible) { + throw new Error(`This controller cannot accept live writes: ${plan.applyEligibility.reasons.join("; ")}`); + } + if (!args.yes) { + process.stdout.write(`\n${plan.changes.length} change(s) not written. Re-run with --yes to apply.\n`); + return; + } + + const backupPath = resolve(args.backup ?? `mft-backup-${new Date().toISOString().replaceAll(":", "-")}.json`); + const connection = backend.connectForApply(device); + try { + const result = await applyPatchPlan(connection, device, plan, { + journalPath: resolve(dirname(backupPath), "mft-updates.ndjson"), + timeoutMs: args.timeoutMs, + saveBackup: async (snapshot) => { + await writeAtomically(backupPath, `${JSON.stringify(snapshot, null, 2)}\n`); + return backupPath; + }, + }); + process.stdout.write(`\nApplied and verified ${plan.changes.length} change(s).\nBackup: ${result.backupPath}\n`); + } finally { + connection.close(); + } + return; + } + if (args.command === "apply") { const plan = JSON.parse(await readFile(resolve(args.plan!), "utf8")) as PatchPlan; const stateDirectory = resolve(".mft-state"); diff --git a/src/planner.ts b/src/planner.ts index ee282c0..ac85864 100644 --- a/src/planner.ts +++ b/src/planner.ts @@ -258,6 +258,54 @@ export function evaluateApplyEligibility(config: ConfigExport): { eligible: bool return { eligible: reasons.length === 0, reasons }; } +export interface UpdatePlan { + /** Null when every requested value is already set. */ + plan: PatchPlan | null; + skipped: Array<{ path: string; value: number | boolean }>; +} + +/** + * Plans an update: the same content-addressed plan, minus the values already + * at their target. + * + * `createPatchPlan` throws on a no-op, which is right for `plan`, where the + * contract is "the state I described is the state I found". It is wrong for an + * update, where "make the top row green" must not fail because one of those + * knobs is already green. Here a no-op is skipped and reported. + * + * Duplicate paths are rejected rather than resolved. Two `--set`s for one field + * have no obvious winner, and silently picking one is worse than refusing. + */ +export function planUpdate(config: ConfigExport, inputs: PatchInput[], now = new Date()): UpdatePlan { + if (config.schemaVersion !== "djtt.mft.config-export.v1") throw new Error("Unsupported snapshot schema"); + if (inputs.length === 0) throw new Error("At least one --set operation is required"); + const policy = firmwarePolicy(config.device.firmware.date); + const palette = config.globals.colorMap.name === "mf64" ? "mf64" : "classic"; + + const seen = new Set(); + const pending: PatchInput[] = []; + const skipped: UpdatePlan["skipped"] = []; + + for (const input of inputs) { + const target = parseTarget(config, input.path); + if (seen.has(target.normalizedPath)) throw new Error(`Duplicate --set for ${target.normalizedPath}`); + seen.add(target.normalizedPath); + + const rawExpected = target.rawTags[String(target.rule.tag)]; + if (rawExpected === undefined) throw new Error(`${target.normalizedPath} was not reported by this firmware`); + const expected = semanticRaw(rawExpected, target.rule, policy.shiftedChannelIsOneBased); + const desired = resolveValue(input.value, target.rule, palette); + if (desired === expected) { + skipped.push({ path: target.normalizedPath, value: desired }); + continue; + } + pending.push(input); + } + + if (pending.length === 0) return { plan: null, skipped }; + return { plan: createPatchPlan(config, pending, now), skipped }; +} + export function createPatchPlan(config: ConfigExport, inputs: PatchInput[], now = new Date()): PatchPlan { if (config.schemaVersion !== "djtt.mft.config-export.v1") throw new Error("Unsupported snapshot schema"); if (inputs.length === 0) throw new Error("At least one --set operation is required"); diff --git a/test/cli.test.ts b/test/cli.test.ts index bfe79a4..7907106 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -41,7 +41,17 @@ test("help still documents apply as disabled", () => { const result = runCli(["--help"]); assert.equal(result.status, 0); - assert.match(result.stdout, /apply is the only command that writes settings, and it is disabled/); + assert.match(result.stdout, /apply is a separate flow for a reviewed plan file, and is disabled/); + assert.match(result.stdout, /mft-config update --set/); +}); + +test("update rejects an empty change list and is not caught by the apply gate", () => { + // No device is touched: argument validation rejects before discovery. + const result = runCli(["update"]); + + assert.equal(result.status, 1); + assert.match(result.stderr, /update requires at least one --set/); + assert.doesNotMatch(result.stderr, /apply is disabled/); }); test("plan still works while apply is gated", () => { diff --git a/test/planner.test.ts b/test/planner.test.ts index 637bf3d..c5a5178 100644 --- a/test/planner.test.ts +++ b/test/planner.test.ts @@ -1,10 +1,11 @@ import assert from "node:assert/strict"; import { readFile } from "node:fs/promises"; +import { readFileSync } from "node:fs"; import test from "node:test"; import { encoderTransferTag, firmwarePolicy } from "../src/compatibility.js"; import type { ConfigExport, EncoderExport } from "../src/model.js"; -import { createPatchPlan } from "../src/planner.js"; +import { createPatchPlan, planUpdate } from "../src/planner.js"; import { assembleBulkParts, parseBulkPart, parseTagValues } from "../src/protocol.js"; import { snapshotHash } from "../src/snapshot.js"; @@ -112,3 +113,47 @@ test("compatibility fixtures select the expected last encoder tag", async () => assert.equal(policy.liveWriteAllowed, fixture.writeAllowed); } }); + +test("planUpdate skips values already set instead of aborting the whole update", () => { + const config = JSON.parse(readFileSync(new URL("fixtures/synthetic-four-bank.json", import.meta.url), "utf8")) as ConfigExport; + // Encoder 1 is already green in the fixture; encoder 2 is not being changed + // to its current value. `createPatchPlan` refuses the pair outright. + assert.throws( + () => createPatchPlan(config, [ + { path: "bank.1.encoder.1.colors.active", value: "green" }, + { path: "bank.1.encoder.2.colors.active", value: "blue" }, + ]), + /is already/, + ); + + const { plan, skipped } = planUpdate(config, [ + { path: "bank.1.encoder.1.colors.active", value: "green" }, + { path: "bank.1.encoder.2.colors.active", value: "blue" }, + ]); + + assert.equal(skipped.length, 1); + assert.equal(skipped[0]?.path, "bank.1.encoder.1.colors.active"); + assert.equal(plan?.changes.length, 1); + assert.equal(plan?.changes[0]?.path, "bank.1.encoder.2.colors.active"); +}); + +test("planUpdate reports nothing to do when every value is already set", () => { + const config = JSON.parse(readFileSync(new URL("fixtures/synthetic-four-bank.json", import.meta.url), "utf8")) as ConfigExport; + + const { plan, skipped } = planUpdate(config, [{ path: "bank.1.encoder.1.colors.active", value: "green" }]); + + assert.equal(plan, null, "an all-skipped update must succeed having done nothing"); + assert.equal(skipped.length, 1); +}); + +test("planUpdate refuses two --set operations for the same field", () => { + const config = JSON.parse(readFileSync(new URL("fixtures/synthetic-four-bank.json", import.meta.url), "utf8")) as ConfigExport; + + assert.throws( + () => planUpdate(config, [ + { path: "bank.1.encoder.1.colors.active", value: "blue" }, + { path: "bank.1.encoder.1.colors.active", value: "red" }, + ]), + /Duplicate --set/, + ); +});