Publish the CLI as mft-config, with apply withheld pending #14 - #15
Conversation
Installing this tool meant cloning the repo, installing dependencies, building, and copying a skill directory by hand — five steps and a Node toolchain before touching a knob. Every documented example read `node dist/cli.js`. Publish the package instead: - rename `@djtechtools/mft-export` to `mft-config` and drop `private` - ship both `mft-config` and a deprecated `mft-export` alias as bins - restrict the tarball to `dist/` and `schema/` - add a `prepack` that builds and tests from a clean checkout - rename the CLI in usage output *and* the error prefix Install is now `npm install -g mft-config`, or no install at all with `npx -y mft-config`. The skill and docs reference the published command, and the skill install is a single curl that fetches only the skill directory — no clone, no build. Add CI, which did not exist. It packs the artifact and installs it into a clean directory on Linux, macOS, and Windows rather than only invoking `dist/cli.js` in the source tree. `--help` is not treated as a smoke test: it returns before the dynamic import of `./midi.js`, so it never loads the native addon and would miss a broken prebuild. The matrix loads the addon explicitly and runs an offline `plan` against a synthetic fixture, which needs no attached hardware. The fixture is synthetic on purpose — a real export carries the unit ID and the complete mapping, which the docs treat as private device data. Removing "run from the repository root" also removes what kept the `.mft-state/` journal in one place, so the skill and README now state the working-directory rule explicitly. The underlying fix, a stable state root, is tracked separately. Refs #3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous commit found the synthetic fixture was only half synthetic. It was produced by rewriting `rawTags` on a real export, which left every normalized field holding the operator's actual mapping — the exact private device data the fixture existed to avoid shipping — and left the two halves disagreeing in 188 places. Generate it instead: `test/fixtures/generate-synthetic.ts` builds each record from deterministic tag values and runs it through `normalizeEncoder` and `normalizeGlobals`, so raw and normalized cannot drift. Reviewing a 50-line generator beats reviewing 3,600 lines of JSON. The scan now reports zero mismatches and the unit ID is all zeros. The first attempt at that generator was cast with `as ConfigExport`, which hid a missing `capabilities` field; the CLI then failed at runtime with "Cannot read properties of undefined". Dropping the cast for a plain type annotation surfaces it at compile time — but only if the file is actually type-checked, and `tsconfig.json` covers `src/**` alone because `rootDir` is `src` and tests must not land in `dist`. So add `tsconfig.test.json` and wire it into `check`. Verified by deleting `capabilities` again and confirming `check` now fails with TS2741. All existing test sources were already clean under it. Also make `prepack` call `npm run check` rather than `pnpm run check`, so `npm publish` from an environment without pnpm does not fail at pack time. Refs #3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c30e7fb to
a2f61c6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c30e7fb8e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| connection.sendConfigurationWrite(refreshFrame!.bytes); | ||
| await delay(pacingMs); |
There was a problem hiding this comment.
For every plan containing an encoder target, this sends an additional global-settings frame after the final read-back and then immediately journals the plan as complete. If the frame is dropped or only partially applied, the command still reports success and consumes the plan even though the display may remain dark or the global configuration may differ from postSnapshot; handle write errors as unknown and perform full read-back verification before recording completion.
Useful? React with 👍 / 👎.
The new matrix job failed on both platforms on its first run. macOS, the only one I could exercise locally, passed — which is the argument for having the matrix. **Linux.** The addon check constructed an `Input` to read a port count. That needs a working MIDI backend, and hosted Linux runners have no ALSA sequencer (`/dev/snd/seq` is absent), so it failed with "Failed to initialise RtMidi" for a reason unrelated to whether the package shipped correctly. Call `verifyLibraryLoaded()` instead: it forces the prebuilt `.node` to be resolved and dlopen'd, which is the thing actually worth testing, without opening a sequencer client. **Windows.** `tsx --test test/*.test.ts` relies on the shell expanding the glob. pwsh does not, so tsx received the literal pattern and reported "Could not find 'test\*.test.ts'". Quoting it makes tsx do the expansion itself, which works identically on all three platforms. That second one is a pre-existing bug in the test script rather than anything this branch introduced — it was simply unreachable until there was CI running on Windows to reach it. Refs #3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — the display-refresh finding is real and independently confirmed. A review of the design plan for this work flagged the same code, and it is now tracked as #14 (item 5) along with four other apply-boundary defects. Two notes on scope: It is no longer in this PR's diff. Codex reviewed The failure mode is slightly worse than described. Beyond reporting success on a dropped frame, the send sits outside the per-target Fixing it properly is a design decision — make it a planned, verified target, or document it as explicitly best-effort with honest journal semantics — so it belongs with the rest of #14 rather than in a packaging PR. #14 blocks the MCP server work. Also fixed here since your review: CI failed on Linux (hosted runners have no ALSA sequencer, so constructing an |
The previous attempt quoted the glob so tsx would expand it. That works on Node 21 and newer, where the test runner learned to expand globs itself. It does not work on Node 20, which is this package's declared floor and what CI runs, so `check` then failed on Linux too. I verified that fix on Node 24 and concluded it worked. My shell had expanded the glob, not tsx. Verifying against the version in `engines` was the step that would have caught it. Enumerate the files in `test/run-tests.mjs` instead. No glob, no shell involvement, identical on every platform and version — and it exits non-zero when it finds nothing, so a renamed test file cannot leave CI green with an empty suite. Confirmed on Node 20.20.2 and Node 24.15.0: 18 passing on both. Also run the check job across Node 20 and 24. A version-specific bug in the test runner is exactly what just slipped through, and the floor in `engines` is not optional coverage. Refs #3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correcting my previous comment — I described the display-refresh failure mode using the pre-#13 code and did not re-check against current So my claims that the journal can end on What genuinely remains is narrower: the refresh frame is generated at apply time, so it is not in Also since the last run, CI caught a mistake of mine worth recording. I had fixed the Windows glob failure by quoting the pattern so tsx would expand it, and verified that on Node 24 — but this package's floor is Node 20, and the test runner only learned to expand globs in Node 21. My shell had done the expanding, not tsx, so |
Adversarial review of the plan made a fair objection to shipping this package with a working `apply`. The defects in #14 predate this work, but the reasoning that they were therefore "status quo, not a regression" was too convenient. Before publishing, operating the device meant working inside a checkout. After, `npx mft-config apply` from an arbitrary directory is the ordinary path — which is exactly the condition that makes the cwd-relative journal (#14 item 0d) bite, and puts a write-capable tool one command away for any agent. Publishing did not add the defects; it removed the friction containing them. So publish the read path and withhold the write. `list`, `export`, and `plan` are untouched, and they are where the install pain this project set out to fix actually lived. `apply` refuses with a message naming both defects and pointing at the issue. The gate runs before device discovery, so a refused apply never opens a MIDI port. `MFT_UNSAFE_APPLY=1` lifts it, so this project can still exercise its own write path against real hardware. It is documented under Develop and deliberately not in the agent skill; the skill instead tells the agent to plan as usual, report the plan, and say the write is unavailable rather than to go looking for a way around it. Add test/cli.test.ts — the CLI had no tests at all, and an untested safety control is one that stops working quietly. Four cases: apply refuses on the gate rather than incidentally on a missing file, the gate precedes discovery, help documents the state, and the offline commands are not caught by it. Verified by deleting the gate and watching the second case fail with "Device index 99 does not exist" instead of the gate message. Refs #3, #14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oveddan
changed the title
|
Update: An adversarial review of the design plan objected to shipping a working So this PR now publishes the read path and withholds the write:
The skill now tells the agent to export, plan, and report the plan as usual, then say the write is unavailable and offer the vendor utility — rather than to go hunting for a way around the block. Also adds The same review found several other things worth recording, since they affect what #14 should become:
I will fold these into #14 next. |
Self-review of the gate found four problems, one of which mattered.
The docs said writes were "disabled in this release", but the gate only
covered the CLI entry point. The package ships dist/ with no `exports`
field, so every module under it was public API: `import('mft-config/dist/
applier.js')` resolved and handed back `applyPatchPlan`. Verified against a
packed install. Add an `exports` field so subpath imports are refused;
confirmed the bin and a live `list` still work.
`apply` without `--yes` reported "apply requires explicit confirmation with
--yes", because argument validation ran before the gate. The tool coached
the user into completing an invocation it was going to refuse anyway. Move
the gate ahead of those checks.
The test claiming offline commands are not gated invoked `plan` with no
arguments, so it failed during argument parsing and its assertion could not
have failed. Replaced with a real plan against the synthetic fixture,
asserting a content-addressed plan comes back.
test/run-tests.mjs derived an absolute test directory and then emitted
cwd-relative paths. Making the paths absolute was not sufficient on its own
— `--import tsx` resolves against the child's cwd — so anchor the child to
the package root. Verified from both the repo root and /tmp: 22 passing.
Refs #3, #14
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Self-review found the gate was weaker than the docs claimed: no Also: |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #3. First of three chunks toward #8 and #12 — see the plan in the last section.
Why
Installing this tool meant cloning, installing, building, and copying a skill directory by hand: five steps and a Node toolchain before touching a knob. Every documented example read
node dist/cli.js. The README promises "you describe what you want and it gets done", which a five-step manual install undercuts.What changed
Packaging.
@djtechtools/mft-export→mft-config,privatedropped, bothmft-configand a deprecatedmft-exportalias shipped as bins. The tarball is restricted todist/andschema/, and aprepackbuilds and tests from a clean checkout. The CLI name is corrected in usage output and in the error prefix, which still printedmft-export:.Install is now:
or nothing at all with
npx -y mft-config.Skill and docs. Both reference the published command. The skill install is a single
curlthat fetches only the skill directory — no clone, no build — verified to pull all three files.CI, which did not exist. It packs the artifact and installs it into a clean directory on Linux, macOS, and Windows, rather than only invoking
dist/cli.jsin the source tree.--helpis deliberately not treated as a smoke test: it returns before the dynamic import of./midi.js, so it never loads the native addon and would miss a broken prebuild. The matrix loads the addon explicitly and runs an offlineplan, which needs no attached hardware.Verified against real hardware
Not just tests — the packed tarball was installed into a clean directory and run against an attached Twister on firmware
2026-07-02:list→ device discoveredexport→ 4 banks, 64 encoders, strong unit ID, 0 warningsplan→ content-addressed, eligible, correct expected/desired valuesprepackrebuilds from a deleteddist/and runs all 15 testsNo
applywas run; nothing was written to the device.Review found a defect in this PR, now fixed
The first version of the CI fixture was only half synthetic. It was made by rewriting
rawTagson a real export, which left every normalized field holding the operator's actual mapping — the exact private device data the fixture existed to avoid shipping — and left the two halves disagreeing in 188 places.It is now generated by
test/fixtures/generate-synthetic.tsthrough the shipped normalizers, so raw and normalized cannot drift. A 50-line generator is reviewable; 3,600 lines of JSON is not.That generator was initially cast with
as ConfigExport, which hid a missingcapabilitiesfield until the CLI failed at runtime. Dropping the cast surfaces it at compile time — but only if the file is type-checked, andtsconfig.jsoncoverssrc/**alone becauserootDirissrc. Sotsconfig.test.jsonwas added and wired intocheck; confirmed by deletingcapabilitiesagain and watchingcheckfail with TS2741.Known gap, tracked separately
Removing "run from the repository root" also removed what kept
.mft-state/— the backups and the single-use plan journal — in one place. It resolves against cwd (src/cli.ts:149), so the skill and README now state the working-directory rule explicitly. That is prose, not enforcement.The real fix, a stable state root, is #14, along with four other apply-boundary defects found in review: forgeable
applyEligibility, frames not checked against changes, plans replayable after a crashed apply, and an unverified display-refresh write. All are live onmaintoday and none is made worse here. #14 blocks the MCP server.Plan
Chunk A is this PR. Chunk B is #14. Chunk C is the MCP server plus a Claude Code plugin, closing #8 and #12. The standalone binary in #12 is deferred rather than rejected — its stated blocker, a per-platform native addon matrix, does not hold up:
@julusian/midialready ships nine prebuilds. macOS notarization is the real unquantified cost.🤖 Generated with Claude Code