Close the apply boundary: forgeable eligibility, replayable plans, cwd-relative state

[oveddan]

Five defects in the write path, found while reviewing the distribution plan in #12. All are live in the CLI today. None is caused by the MCP server work, but that work widens the blast radius by letting an agent supply plan JSON, so these should land first.

This blocks the MCP server. See #12.

1. applyEligibility sits outside the content hash

planBody() (src/planner.ts:212) hashes {createdAt, expiresAt, snapshotHash, deviceBinding, changes, frames}. applyEligibility is attached outside that body (src/planner.ts:207). But assertValidPlan gates on it (src/planner.ts:227):

if (!plan.applyEligibility.eligible) throw new Error(...)

So editing a plan file to flip eligible: true and clear reasons leaves planId valid, and a plan rejected for legacy firmware or weak device identity applies anyway. The firmware allowlist is the main guard against writing to an untested firmware, and it is bypassable with a text editor.

Fix: include applyEligibility in planBody(), and recompute eligibility inside the apply path from the freshly-read device rather than trusting the serialized field.

2. Frames are not proven to match changes

The hash is unkeyed, so it proves integrity in transit, not authenticity — anyone can recompute it. Nothing re-derives frames from changes, so a plan can declare one change and carry frames that write something else. assertConfigurationWrite (src/protocol.ts:97) validates frame shape, not correspondence to the declared intent, and the mismatch would only be noticed after bytes were sent.

Fix: have assertValidPlan re-derive frames from changes via encodeGlobalDryRun / encodeEncoderDryRun and compare before connecting.

3. Plans are not single-use

assertPlanNotConsumed (src/journal.ts:28) rejects only outcome === "complete". A plan whose apply died mid-write — journaled unknown — is replayable. There is also no atomic reservation between the check and the started record, so two applies can both pass the check.

The README states "Completed plans are single-use", which is literally true and materially misleading.

Fix: block on any prior started / pending / unknown until explicitly resolved; make check→started an atomic reservation; add a per-device apply mutex. A long-running MCP server makes concurrent invocation natural, so this stops being theoretical.

4. .mft-state is relative to the current working directory

src/cli.ts:149 resolves the journal, backups, and last-verified.json against cwd. Consequences:

• Running apply from a different directory than a previous apply consults a different journal, silently defeating (3).
• An MCP server's cwd is set by its host, not the user's shell, so server and CLI would never share a journal.
• Backups can land in a plugin cache directory the user never looks at.

This was previously masked by the skill instructing the agent to run from the repository root. Now that the CLI installs from npm there is no repository root, so the constraint is documented prose in the skill and README rather than anything enforced.

Fix: one stable per-user state root shared by CLI and any server, with an explicit --state-dir / env override, plus migration for existing cwd-local journals.

5. The display re-enable write is unverified

src/applier.ts:81-97 sends a global write after encoder targets so the LED display comes back. It sits outside the per-target try/catch and:

• is not in the plan's frames, so the hash-covered list a reviewer approves is not the complete set of bytes sent;
• gets no read-back and never receives verified;
• on a throw leaves the journal's last entry as pending with no terminal outcome, contradicting docs/write-safety.md item 7 — and since (3) only blocks complete, the plan stays replayable;
• means postSnapshot (returned at src/applier.ts:98, written to last-verified.json) predates the final write.

Reachable in practice: encodeGlobalDryRun calls assertExactTags (src/write-codec.ts:41), which throws if a freshly-read globals tag set is not exactly allowlisted — after every encoder write has already landed.

Fix: either make it a planned, verified target, or document and test it as explicitly best-effort with honest journal semantics. Correct docs/write-safety.md and the README's "reads the full configuration after every target" either way.

Acceptance criteria

• A plan with hand-edited applyEligibility is rejected
• A plan whose frames do not match its changes is rejected before any send
• A plan with a prior started / pending / unknown journal entry is refused
• Two concurrent applies of one plan cannot both proceed
• CLI and a differently-rooted process share one journal
• The journal always reaches a terminal outcome, including when the display refresh fails
• Docs claiming full verification and single-use match what the code does

None of these paths has a regression test today.

[oveddan]

Correction: item 5 is already fixed on main, by #13.

I wrote this issue against the pre-#13 code and did not re-check it after that PR merged. #13 addresses essentially all of item 5:

• the send is wrapped in try/catch and journals unknown on failure, so there is no path that leaves a pending entry with no terminal outcome;
• the refresh frame gets a full read-back, journaling unknown on read failure and failed if the hash moved, then verified;
• progressive = observed after that read-back, so postSnapshot is no longer stale;
• encodeGlobalDryRun(before.globals.rawTags, policy) runs as a pre-flight at the top of the apply, so the assertExactTags throw I described as "reachable after every encoder write has already landed" now happens before any write.

That last point was the concrete failure scenario I gave, and it no longer holds.

What remains of item 5 is narrower: the refresh frame is generated at apply time and is not part of plan.frames, so the hash-covered frame list a reviewer approves is still not the complete set of bytes the apply will send. Worth closing, but a much smaller claim than the original text.

Items 1-4 are unchanged and re-verified against current main:

• planBody() still excludes applyEligibility
• assertValidPlan still does not re-derive frames from changes
• assertPlanNotConsumed still matches only outcome === "complete"
• src/cli.ts:149 still resolves .mft-state against cwd

Item 1 remains the sharpest: a plan file edited to flip eligible: true keeps a valid planId.

[oveddan]

Second correction, from an adversarial review of the design plan. Two of my original claims were wrong, one proposed fix is impossible, and five more defects belong here.

Also: apply is now withheld in the published package (PR #15) until this issue is resolved. list, export, and plan ship normally. That changes the urgency here — nothing is shipping a live write path — but not the substance.

Corrections to what is already filed

0a — the weak-identity half is false. I claimed a "legacy-firmware or weak-identity plan applies". Only the legacy-firmware path does. A weak snapshot stores deviceBinding.unitId = null while snapshot.device.unitId?.hex is undefined, so assertDeviceBinding (src/applier.ts:29) rejects it on undefined !== null. That protection is accidental and fragile, but it is real.

My original demonstration used a null unit ID, so it proved only that the hash survives an edit — not that a write lands. Redone properly with a strong unit ID and firmware 2023-06-08:

• plan refuses: eligible: false, ["Live writes are not allowlisted for firmware 2023-06-08"]
• flip applyEligibility to {eligible: true, reasons: []}, change nothing else
• assertValidPlan passes — the planId is untouched
• assertDeviceBinding matches, and nothing downstream rechecks liveWriteAllowed

So 0a is real and exploitable end to end, via legacy firmware specifically.

0a — hashing the field is not the fix. The hash is unkeyed, so anyone editing the file can recompute it. The load-bearing change is recomputing eligibility from the freshly-read device inside the apply path. Adding applyEligibility to planBody() is still worth doing for coherence, but it invalidates every existing v1 plan, so it should come with a schemaVersion bump rather than a silent break.

0b — the fix I proposed cannot be implemented. I wrote that assertValidPlan should re-derive frames from changes. It cannot: PlannedChange carries only the changed tag, while encodeEncoderDryRun requires the complete 15-tag record, tags 10-24 (src/write-codec.ts:54). The untouched values exist only in the snapshot or in the untrusted frame itself. assertValidPlan also runs at src/applier.ts:44, before the fresh export at src/applier.ts:47, so it has no snapshot available.

Correct placement: after the fresh export and after the snapshot-hash check, before backup, journal, or any write — re-derive from the live snapshot's raw records plus the declared changes. Or have the plan carry a hash-covered planning-basis record.

0c — a process-local mutex is not sufficient. The CLI, an MCP server, a second MCP server, and a second terminal are separate processes. Exclusion has to be a filesystem lock in the shared state root, held from reservation through terminal journal outcome, and keyed per physical device at minimum — interleaved multipart encoder writes corrupt state.

Additional defects

0f. A malformed expiry never expires. assertValidPlan evaluates new Date(plan.expiresAt).getTime() <= now. For an unparseable date that is NaN <= now, which is false, so the plan passes the expiry check. Verified.

0g. apply does not validate against its own schema. It does JSON.parse(...) as PatchPlan — a cast, not a check. schema/patch-plan.schema.json exists and is referenced by nothing.

0h. There is no recovery workflow. 0c proposes blocking a plan left in started / pending / unknown "until explicitly resolved", but defines no command or state transition that resolves it. Without one, the fix turns a silent hazard into a permanent wedge.

0i. Device selection is positional and unstable. Discovery order can change between invocations, so --device 1 is not a stable identity across the plan/apply gap. Custody should key on the strong unit ID.

0j. Temp-file names collide. writeAtomically uses ${destination}.tmp-${process.pid} (src/cli.ts:93). Two processes writing the same destination can collide, and the loser's cleanup can unlink the winner's temp file. Backup filenames are also only timestamp-to-the-second.

One more, deferred rather than filed: backup retention is undefined once everything lands in a single per-user state root instead of scattered per-directory ones.

Note on 0d verification

The check I originally proposed — apply a plan, then retry from a different directory — does not prove anything. Snapshot drift rejects the second attempt whether or not the journals are shared. A valid test either exercises an unknown write that did not land, or asserts directly that both interfaces resolve the identical journal path.

[oveddan]

#16 merged. Fixed: 0a (eligibility recomputed from the live device, field now hashed, schema v2), 0b (frames, target, tag, and value all re-derived from the change's path against the live snapshot; applier sends derived bytes), 0f (NaN expiry).

Review of those fixes found three more holes in them, all now closed: hex compared instead of the bytes actually sent; target/tag trusted so a change could display one path and write another tag; desired not range-checked so a 1..16 field accepted 100.

Not doing: 0c/0h locking and recovery. The snapshot-hash precondition already blocks meaningful replay, and re-planning already recovers a crashed apply, so the wedge I described does not exist. Concurrency is documented instead — and the documented risk is stale-snapshot clobbering, not interleaved frames: every write carries the whole record, so two overlapping applies each verify successfully while one silently reverts the other.

Remaining: 0d (cwd-relative state), 0g (schema not validated), 0i (positional device selection), 0j (temp-file collisions). apply stays gated on 0d.