Enforce write eligibility against the device, not the plan file

.claude/skills/mft-configurator/SKILL.md

@@ -76,7 +76,7 @@ Plans expire after 15 minutes. If the plan expired or the device may have change
 ## Apply is currently disabled
 
 `mft-config apply` refuses in the current release while the write-path defects in
-[issue #14](https://github.com/oveddan/mft-api/issues/14) are open — a plan file edited to set `applyEligibility.eligible=true` keeps a valid plan ID and bypasses the firmware allowlist, and `.mft-state` resolves against the current working directory.
+[issue #14](https://github.com/oveddan/mft-api/issues/14) are open — `.mft-state` resolves against the current working directory, so an apply run from a different directory consults a different journal, and a plan whose apply died mid-write can be replayed.
 
 When a user asks to change settings: still export, still plan, still report the plan. Then tell them the change cannot be written yet, name the issue, and offer the vendor MIDI Fighter Utility as the way to make it by hand. Do not look for a way around the block — there is an environment variable that lifts it and it is deliberately not for agent use.
 

README.md

@@ -128,13 +128,14 @@ Only `apply` can write configuration. System commands, reset commands, and
 bootloader commands are always blocked. Live writes currently require firmware
 `2026-07-02` and a strong command-`0x05` device identity.
 
-> **`apply` is disabled in the current release.** Two defects in the write path
-> ([#14](https://github.com/oveddan/mft-api/issues/14)) are open: a plan file
-> edited to set `applyEligibility.eligible=true` keeps a valid plan ID, which
-> bypasses the firmware allowlist; and `.mft-state` — the backups and the
-> single-use plan journal — resolves against the current working directory, so
-> applying from a different directory consults a different journal. That second
-> one became easy to hit once the CLI could be installed and run from anywhere.
+> **`apply` is disabled in the current release** while the write-path defects in
+> [#14](https://github.com/oveddan/mft-api/issues/14) are open. Still
+> outstanding: `.mft-state` — the backups and the single-use plan journal —
+> resolves against the current working directory, so applying from a different
+> directory consults a different journal; and a plan whose apply died mid-write
+> can be replayed, because only a *completed* plan is recorded as consumed.
+> The first became easy to hit once the CLI could be installed and run from
+> anywhere.
 >
 > `list`, `export`, and `plan` are unaffected and are the whole read path. To
 > write settings meanwhile, use the vendor MIDI Fighter Utility.
@@ -438,8 +439,21 @@ Before sending any configuration frame, `apply`:
 7. records pending, verified, failed, or unknown outcomes in an append-only
    journal.
 
-Completed plans are single-use. See [`docs/write-safety.md`](docs/write-safety.md)
-for protocol-level details and remaining limitations.
+Completed plans are single-use.
+
+**Write to a controller from one process at a time.** Nothing enforces this — a
+lock would bring its own failure modes for what is an edge case on a single-user
+desk device.
+
+Do not rely on the stale-value check to catch an overlap. Every write carries
+the whole record, rebuilt from the snapshot that writer read, so if two applies
+both export before either writes, the second one reverts the first's changes —
+and because each verifies against its own expectation, both report success and
+nothing records that a change was lost. Close the vendor MIDI Fighter Utility
+before writing from here, too.
+
+See [`docs/write-safety.md`](docs/write-safety.md) for protocol-level details
+and remaining limitations.
 
 ## Protocol coverage
 

docs/write-safety.md

@@ -31,3 +31,37 @@ The write implementation must preserve these invariants:
 Before support broadens beyond the current allowlist, tests must additionally
 cover journal recovery, two attached devices, timeouts before and after send,
 normalized fields, and confirmed restore.
+
+## One writer at a time
+
+**Nothing prevents two processes from writing to the same controller at once,
+and this is deliberate.** A Twister is a single controller on one person's desk;
+concurrent writers are an edge case, and defending against them would mean a
+cross-process lock and its own failure modes — a stale lock is a device you
+cannot write to.
+
+If it does happen, the failure is quiet, and the precondition check does not
+save you. Every write carries the **whole** record — all of a global block, all
+fifteen tags of an encoder — rebuilt from the snapshot that writer read. So two
+applies that never interleave a single frame still lose data:
+
+1. A and B both export. Both hold snapshot `S`, and both preconditions pass,
+   because neither has written yet.
+2. A writes its record and reads back. Its own change is there. A reports
+   success.
+3. B writes its record, rebuilt from `S` — which still carries A's tags at
+   their *old* values. A's change is silently reverted.
+4. B reads back and compares against what `S` plus B's own change implies.
+   It matches. B reports success too.
+
+Both processes report success, verification passes for both, and one of the two
+changes is gone with nothing recorded to say so. Interleaving frames mid-record
+is a further way to corrupt a single record, but it is not the main risk — this
+is, and it needs no unlucky timing, only two overlapping reads.
+
+Read-back cannot catch it: each writer verifies against its own expectation, and
+both expectations are individually satisfied.
+
+So: don't run two applies against the same controller simultaneously, and don't
+build tooling that does. The same applies to the vendor MIDI Fighter Utility —
+close it before writing from here.

schema/patch-plan.schema.json

@@ -1,12 +1,12 @@
 {
   "$schema": "https://json-schema.org/draft/2020-12/schema",
-  "$id": "https://djtechtools.com/schemas/mft-patch-plan-v1.json",
+  "$id": "https://djtechtools.com/schemas/mft-patch-plan-v2.json",
   "title": "MIDI Fighter Twister patch plan",
   "type": "object",
   "additionalProperties": false,
   "required": ["schemaVersion", "planId", "createdAt", "expiresAt", "snapshotHash", "deviceBinding", "applyEligibility", "changes", "frames"],
   "properties": {
-    "schemaVersion": { "const": "djtt.mft.patch-plan.v1" },
+    "schemaVersion": { "const": "djtt.mft.patch-plan.v2" },
     "planId": { "type": "string", "pattern": "^sha256:[0-9a-f]{64}$" },
     "createdAt": { "type": "string", "format": "date-time" },
     "expiresAt": { "type": "string", "format": "date-time" },

src/applier.ts

@@ -2,9 +2,9 @@ import type { DeviceDescriptor, ConfigExport } from "./model.js";
 import type { ConfigurationWriteConnection } from "./midi.js";
 import { exportConfiguration } from "./exporter.js";
 import { appendJournal } from "./journal.js";
-import { assertValidPlan, expectedSnapshotAfterChanges, type PatchPlan } from "./planner.js";
+import { assertValidPlan, deriveFrames, evaluateApplyEligibility, expectedSnapshotAfterChanges, type PatchPlan } from "./planner.js";
 import { snapshotHash } from "./snapshot.js";
-import { encodeGlobalDryRun } from "./write-codec.js";
+import { encodeGlobalDryRun, type DryRunFrame } from "./write-codec.js";
 import { firmwarePolicy } from "./compatibility.js";
 
 interface ApplyOptions {
@@ -23,6 +23,41 @@ function delay(milliseconds: number): Promise<void> {
   return new Promise((resolve) => setTimeout(resolve, milliseconds));
 }
 
+/**
+ * Returns the frames to send, derived from the live snapshot and the plan's
+ * declared changes, having confirmed they are the frames the plan carries.
+ *
+ * The comparison is over `bytes`, because `bytes` is what gets sent. Comparing
+ * the `hex` rendering instead left a hole: a plan could keep the legitimate hex
+ * string for display while carrying different bytes, pass the check, and have
+ * the modified bytes written — read-back would notice only after they reached
+ * the device.
+ *
+ * The derived frames are also what the caller sends, so the plan's own array is
+ * never the source of any byte. It exists to be reviewed, and to be checked.
+ */
+function resolveFramesToSend(snapshot: ConfigExport, plan: PatchPlan): DryRunFrame[] {
+  const derived = deriveFrames(snapshot, plan.changes, firmwarePolicy(snapshot.device.firmware.date));
+  const matches =
+    derived.length === plan.frames.length &&
+    derived.every((frame, index) => {
+      const declared = plan.frames[index];
+      return (
+        declared !== undefined &&
+        frame.target === declared.target &&
+        frame.hex === declared.hex &&
+        frame.bytes.length === declared.bytes.length &&
+        frame.bytes.every((byte, position) => byte === declared.bytes[position])
+      );
+    });
+  if (!matches) {
+    throw new Error(
+      "Patch plan frames do not match the frames its changes imply for this device; refusing to send bytes the plan does not describe",
+    );
+  }
+  return derived;
+}
+
 function assertDeviceBinding(snapshot: ConfigExport, plan: PatchPlan): void {
   const binding = plan.deviceBinding;
   if (
@@ -47,10 +82,27 @@ export async function applyPatchPlan(
   const before = await exportConfiguration(connection, device, { timeoutMs, retries: 1 });
   assertDeviceBinding(before, plan);
   if (snapshotHash(before) !== plan.snapshotHash) throw new Error("Device configuration changed since this plan was created");
+
+  // Everything above trusts the plan file. These two checks do not, and both
+  // have to run here rather than in assertValidPlan, because both need the
+  // device state that only exists after the export above.
+  //
+  // The plan's own applyEligibility is attacker-editable and the content hash
+  // is unkeyed, so re-deriving it from the device in front of us is what
+  // actually enforces the firmware allowlist.
+  const liveEligibility = evaluateApplyEligibility(before);
+  if (!liveEligibility.eligible) {
+    throw new Error(`Device is not eligible for live writes: ${liveEligibility.reasons.join("; ")}`);
+  }
+  // And the frames are only meaningful if they are the frames these changes
+  // imply. Encoder writes carry the whole record, so a plan could declare one
+  // change and ship bytes that rewrite fourteen other tags. Everything below
+  // sends these derived frames, never plan.frames.
+  const framesToSend = resolveFramesToSend(before, plan);
   const backupPath = await options.saveBackup(before);
   await appendJournal(options.journalPath, { planId: plan.planId, outcome: "started", detail: `backup=${backupPath}` });
 
-  const targets = [...new Set(plan.frames.map((frame) => frame.target))];
+  const targets = [...new Set(framesToSend.map((frame) => frame.target))];
   const policy = firmwarePolicy(device.identity.firmwareDate);
   // Encoder writes need a trailing global-settings write to re-enable the display
   // (see below). Validate that frame's tag layout before the first write lands, so
@@ -60,7 +112,7 @@ export async function applyPatchPlan(
   if (needsDisplayRefresh) encodeGlobalDryRun(before.globals.rawTags, policy);
   let progressive = before;
   for (const target of targets) {
-    const frames = plan.frames.filter((frame) => frame.target === target);
+    const frames = framesToSend.filter((frame) => frame.target === target);
     const targetChanges = plan.changes.filter((change) => change.target === target);
     const expected = expectedSnapshotAfterChanges(progressive, targetChanges);
     try {

src/cli.ts

@@ -104,10 +104,10 @@ function assertApplyEnabled(): void {
       "apply is disabled in this release while the write-path defects in",
       "https://github.com/oveddan/mft-api/issues/14 are open.",
       "",
-      "Specifically: a plan file edited to set applyEligibility.eligible=true keeps a",
-      "valid planId, which bypasses the firmware allowlist; and .mft-state (backups and",
-      "the single-use plan journal) resolves against the current working directory, so",
-      "applying from a different directory consults a different journal.",
+      "Still outstanding: .mft-state (backups and the single-use plan journal) resolves",
+      "against the current working directory, so applying from a different directory",
+      "consults a different journal; and a plan whose apply died mid-write can be",
+      "replayed, because only a completed plan is recorded as consumed.",
       "",
       "list, export, and plan are unaffected. To write settings meanwhile, use the",
       "vendor MIDI Fighter Utility.",