Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
fbfb4c8 to
2d5484a
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: fbfb4c8917
ℹ️ 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".
| const field = /^([A-Za-z][\w-]*):\s+(.+)$/.exec(line); | ||
| if (!field) throw new Error(`Unsupported SKILL.md frontmatter line: ${line}`); | ||
| const value = field[2]!.trim(); | ||
| if (/^["'[{|>&*!]/.test(value)) throw new Error(`Unsupported SKILL.md frontmatter value for ${field[1]}`); |
There was a problem hiding this comment.
Reject YAML syntax that this flat parser cannot preserve. For example, description: b: c is currently accepted as the string b: c, although standard YAML rejects the unquoted : sequence; similarly, description: b # c is returned with the comment text while a YAML consumer reads only b. When such frontmatter is added, the manifest disagrees with—or cannot be parsed from—the served SKILL.md, causing conforming hosts and the ordinary-resource fallback to reject the skill.
Useful? React with 👍 / 👎.
| text: bytes.toString("utf8"), | ||
| digest: `sha256:${createHash("sha256").update(bytes).digest("hex")}`, | ||
| size: bytes.length, |
There was a problem hiding this comment.
Serve non-text skill files as MCP blob content rather than decoding every file as UTF-8. If a skill contains a binary resource such as a PNG, toString("utf8") replaces invalid byte sequences, while digest and size still describe the original bytes; clients that verify the bytes returned by resources/read against the manifest will therefore reject the entire skill.
Useful? React with 👍 / 👎.
2d5484a to
f9ecf5c
Compare
Decides #12: no standalone binary (Node 20 is an acceptable requirement), and agents reach the Twister through `mft-config mcp`, a stdio MCP server. - Tools: list_devices, export_configuration, plan_changes. No write tool while the #14 defects are open. Plans are built only from snapshots the server exported itself, and device operations are serialized. - No MIDI port is held between calls, so the server contends with a DAW or the vendor utility no more than one CLI invocation does. - Serves the mft-configurator skill over the MCP Skills Extension (SEP-2640): skill:// resources plus skills/list and skills/get with per-file digests. The skill moves to skills/ in the npm package as its single copy, rewritten around the tools. - Built on the v2 TypeScript SDK, which serves both the 2025 initialize handshake and the 2026-07-28 server/discover handshake the extension is specified against. - Removes the Claude Code plugin and the Codex curl install; both hosts now install with one `mcp add` line. The README leads with that setup. - The test runner is now TypeScript, run through tsx. Closes #12 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f9ecf5c to
4e5871c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e5871c50b
ℹ️ 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".
| child.on("error", reject); | ||
| child.stdout.on("data", (chunk: Buffer) => { | ||
| buffer += chunk.toString("utf8"); | ||
| for (let newline = buffer.indexOf("\n"); newline >= 0; newline = buffer.indexOf("\n")) { |
There was a problem hiding this comment.
Handle the child's exit/close event (or add a timeout) by rejecting done. If the MCP process starts successfully but then exits before returning both skill responses—for example because startup or protocol negotiation regresses—error is never emitted, done remains pending, and the test suite hangs until the external CI timeout instead of reporting the failure.
Useful? React with 👍 / 👎.
1 participant
Closes #12.
Decision
docs/agent-skill.md.mft-config mcpruns a stdio server; the CLI stays for people running one-off commands.Install is now one line per host:
What the server does (
src/mcp.ts)list_devices,export_configuration,plan_changes, all annotated read-only.plan_changesplans only from asnapshotIdthe server itself exported, so an agent cannot plan against fabricated device state.Skill served over MCP (
src/skills.ts)io.modelcontextprotocol/skills): each skill file is askill://mft-configurator/…resource, andskills/list/skills/getreturn frontmatter plus per-file SHA-256 digests and sizes computed from the served bytes..claude/skills/toskills/and ships in the npm package as the single copy, rewritten around the tools.skill://mft-configurator/SKILL.md.SDK: v2, both protocol eras
The extension is specified against MCP revision 2026-07-28 (declared via
server/discover), which SDK v1 cannot speak. The server uses the v2 TypeScript SDK (@modelcontextprotocol/server), whoseserveStdiohandles each connection on either the 2025initializehandshake or the 2026-07-28 one, from one server factory. The device lock and snapshot store are shared across the instances that factory builds.No SDK implements the Skills Extension yet (the TypeScript SDK's is an open PR), so
skills/list/skills/getare custom methods on both.This stays plain async TypeScript on purpose; adopting Effect across the codebase is tracked separately in #24.
Core code unchanged
No hardware was available to test against, so this PR leaves every existing hardware path as it is on
main:midi.ts,protocol.ts,exporter.ts,planner.ts,applier.ts,journal.ts,model.ts,snapshot.ts,usb.tsand the rest ofsrc/are untouched.cli.tsis purely additive: themcpcommand is parsed and dispatched before any existing command's code runs. The only non-added line widens thecommandtype union to include"mcp".discover/connect/exportConfiguration/createPatchPlanexactly as the CLI does; its device-list and device-selection logic is a copy inmcp.ts, not a refactor of the CLI's.test/fake-twister.tsso the MCP tests can share it; the applier assertions themselves are unchanged and pass.Code review fixes
From a
/code-reviewpass on this PR:--devicehint → resolved by reverting the CLI refactor that caused it (see "Core code unchanged").Removed
.claude-plugin/(the plugin only wrapped the server once the skill moved) and the Codexcurl | tarinstall. Existing plugin users are covered by the README's "Upgrading from the plugin" section — still worth a release note.README
Setup now leads with adding the MCP server:
-s userscope so it works in every project, a/mcpcheck and a first prompt, an "Upgrading from the plugin" section for existing plugin users, and the CLI moved under "Without an agent". "What this tool does" lists the server's tools first, the CLI-recipe sections note how agents do the same through tools, and Develop shows how to point an agent at a local checkout's build.Testing
npm run check: 42 tests pass, including newtest/mcp.test.ts(tool surface, export → plan, snapshot provenance, multi-device refusal, serialization, skill digests vs. served bytes,skills/geterrors).mft-config mcpentry point is exercised over stdio on both protocol eras (2025 and 2026-07-28), each asserting the extension is declared andskills/listanswers.resultType: "complete",ttlMsandcacheScopeonskills/list/skills/get. The v2 client consumesresultTypebefore returning a result, so SDK-level assertions cannot see it.test/run-tests.ts, run withtsx); the repo has no plain-JS sources left.skills/list.npx -y mft-config mcpworks only after the next npm release.🤖 Generated with Claude Code