Skip to content

Add Plugin: html2video-for-mcode - #41

Open
Wzdhehe wants to merge 53 commits into
MiniMax-AI:mainfrom
Wzdhehe:add-plugin-html2video-for-mcode
Open

Wzdhehe wants to merge 53 commits into
MiniMax-AI:mainfrom
Wzdhehe:add-plugin-html2video-for-mcode

Conversation

@Wzdhehe

@Wzdhehe Wzdhehe commented Sep 17, 2026

Copy link
Copy Markdown

What changes

Add Plugin: html2video-for-mcode at plugins/Wzdhehe/html2video-for-mcode.

A Skill that turns a topic, outline, or script into a narrated MP4: HTML slides with staged
entrance animations, a TTS voiceover, burned-in subtitles, and an ASR pass that verifies the
voiceover says what the script says.

User value

After installing, a MiniMax Code user can ask in plain language:

帮我把这份大纲做成一条 60 秒的中文口播视频:三张关键数字、结尾一句行动号召,用深色科技主题,加中文字幕。

and get:

out/final.mp4      1920×1080 (or 1080×1920 vertical), H.264 + AAC
out/subs.srt       subtitles for platform upload
out/slide-*.mp4    per-slide segments
preview/*.png      terminal-state frames

What makes it more than a slide exporter:

  • Timing is measured, never hand-written — every slide duration and animation entrance time is
    derived from the TTS audio via ffprobe, so the picture can never lag behind the voiceover.
  • Every slide carries a title layer and a detail layer on separate animation stages, so no page
    is just a headline.
  • Animations are baked into the video — capture steps frames deterministically instead of
    screen-recording.
  • Rendering is gated — a static check refuses slides with undefined CSS variables, missing
    images, external resources, or entrance animations without an animation class: the silent
    failure modes that otherwise ship a broken-looking video while every script reports success.

Plugin submission checklist

  • Plugin lives at plugins/<github-owner>/<plugin-name>.
  • plugin.json name matches the Plugin directory.
  • README.md includes a real example prompt and expected result (bilingual: README.md +
    README.zh-CN.md).
  • LICENSE and plugin.json declare an open-source license (MIT).
  • Required executables, accounts, paid services, and supported platforms are disclosed
    (Node 18+, ffmpeg/ffprobe, Playwright Chromium; MiniMax API key or Token Plan for voice and
    ASR; Windows/macOS/Linux; PowerShell caveat documented).
  • Network destinations and data handled by the plugin are disclosed
    (api.minimaxi.com / api.minimax.io for ASR only when invoked; voice via mcode connectors
    or mmx-cli; image fetching only from URLs the user passes; no telemetry).
  • No credentials, private endpoints, hidden telemetry, installers, symlinks, or native binaries
    are included. The ASR script reads its key from an environment variable or CLI flag at
    runtime and never writes it.
  • Every scaffold TODO has been replaced.
  • Validated through publish/validate-plugin.mjs (an authoring-side tool, deliberately not part of the shipped plugin tree): it stages the Plugin tree into the host checkout, verifies all 52 files are fingerprint-identical (sha256) to the source tree, then runs the upstream validator → OK plugin Wzdhehe/html2video-for-mcode, exit 0. (Validating the host's npm run check alone is not sufficient: it scans the staged copy under _official-plugins/plugins/**, so a stale staged copy yields a green result that proves nothing.)

Evidence

$ node scripts/validate.mjs
OK   plugin Wzdhehe/html2video-for-mcode
$ echo $?
0

The validator also prints a Validated <N> hosted Plugins summary line; N counts every plugin in the checkout and grows as the host merges unrelated plugins (26 → 27 → 28 upstream within the last week; 29 including this PR at the current main snapshot). That line is checkout state, not a property of this PR, so it is not quoted above.

$ node --test "tests/*.test.mjs"     # 244 tests: 0 fail (14 files) — 244 pass / 0 skip on symlink-capable environments; 241 pass / 3 named skips on stock Windows without Developer Mode

All 244 skill tests report 0 fail in every environment measured: the development tree, both published trees, and tool-less sandboxes in both link-capability shapes. Skips are capability-dependent and always named: 0 skips with tools on a symlink-capable system; on stock Windows without Developer Mode the three file-symlink canaries skip by name; in tool-less environments every tool-needing test skips with its reason. When ffmpeg / ffprobe / Chromium are absent (the monorepo's own root-level node --test runs in exactly that environment), every test that needs one of them skips with its stated reason — verified in tool-less sandboxes in both link capability shapes: 0 fail, every remaining case skipping with its stated reason (nothing pretends to pass). Skip counts are environment-dependent and drift as each release adds tests — the invariant is stated here, per-version measurements live in the CHANGELOG — and the scoped workflow installs the tools and runs the whole suite for real. The host repository's own test/hosted-plugins.test.mjs contains a symlink fixture that fails on a Windows checkout without Developer Mode (EPERM: operation not permitted, symlink …); that is a pre-existing host-side issue, it reproduces on a clean checkout without this Plugin, it passes on the CI's ubuntu-latest, and this PR does not touch it. The part of npm run check that inspects Plugins passes with exit code 0 when the staged copy is current.

Manual end-to-end test (Windows, Node 24, ffmpeg-static):

  • Scaffolded a project, generated 8 TTS clips, ran plan-timings.mjs → every slide duration and
    stage entrance time derived from measured audio.
  • check-slides.mjs correctly rejects slides with undefined CSS variables (--coral-a/--coral-b),
    missing images, external font links, and data-stage without an animation class; clean slides pass.
  • capture.mjs --mode motion produced 143 frames for an 8.6s slide; frame-diff (PSNR) confirms the
    staged entrance actually renders at its scheduled time (inf before the entrance, ~14 dB across it).
  • build-video.mjs --asr produced a 14.20s MP4 matching the expected duration exactly, full decode
    clean, out/subs.srt generated, and per-sentence ASR parts produced.
  • Theme gate: 13 themes pass WCAG contrast checks; a low-contrast brand accent (#FFB84D on white)
    is rejected with a non-zero exit code, a compliant one (#C2410C) passes.
  • ASR comparison verified offline against synthetic transcripts: simplified Chinese passes,
    traditional characters (Cantonese voice) and mismatched numbers fail with exit code 1.

Full disclosure of dependencies, network access, and data handling is in the Plugin README.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@Wzdhehe
Wzdhehe force-pushed the add-plugin-html2video-for-mcode branch 3 times, most recently from 0d50ebf to 7320ccd Compare September 17, 2026 15:13
Narrated-video pipeline: HTML slides with staged entrance animations, TTS voiceover
measured with ffprobe, burned-in subtitles, deterministic frame-stepping capture,
ffmpeg assembly, and ASR verification.

- 11 Node scripts, no build step; works with mcode connectors or mmx-cli
- 13 themes, 17 layout recipes, image framing primitives
- Subtitles confined to their own windows (no overlap); 16:9 and 9:16 canvases
- Research phase documented: source grading, cross-verification rules, notes template
- Discloses dependencies, accounts, network destinations and data handling (bilingual README)
- Validation: node scripts/validate.mjs -> OK (exit 0)
@Wzdhehe
Wzdhehe force-pushed the add-plugin-html2video-for-mcode branch from 7320ccd to 4104572 Compare September 17, 2026 15:23
Add Plugin: html2video-for-mcode

Narrated-video pipeline: HTML slides with staged entrance animations, TTS voiceover
measured with ffprobe, burned-in subtitles, deterministic frame-stepping capture,
ffmpeg assembly, and ASR verification.

- 11 Node scripts, no build step; works with mcode connectors or mmx-cli
- 13 themes, 17 layout recipes, image framing primitives
- Subtitles confined to their own windows (no overlap); 16:9 and 9:16 canvases
- Research phase documented: source grading, cross-verification rules, notes template,
  plus how to fetch official-site / press-release text (SPA rendering, PDF-first numbers)
- Discloses dependencies, accounts, network destinations and data handling (bilingual README)
- Validation: node scripts/validate.mjs -> OK (exit 0)
… them to the repo root)

Add Plugin: html2video-for-mcode

Narrated-video pipeline: HTML slides with staged entrance animations, TTS voiceover
measured with ffprobe, burned-in subtitles, deterministic frame-stepping capture,
ffmpeg assembly, and ASR verification.

- 11 Node scripts, no build step; works with mcode connectors or mmx-cli
- 13 themes, 17 layout recipes, image framing primitives
- Subtitles confined to their own windows (no overlap); 16:9 and 9:16 canvases
- Research phase documented: source grading, cross-verification rules, notes template,
  plus how to fetch official-site / press-release text (SPA rendering, PDF-first numbers)
- Discloses dependencies, accounts, network destinations and data handling (bilingual README)
- Validation: node scripts/validate.mjs -> OK (exit 0)

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for exact current head 737ee96.

Blocking security and evidence issues:

  1. Input-derived IDs and paths are not contained. skills/html2video-for-mcode/scripts/capture.mjs:75-77,228-230 uses s.html and s.id to construct paths and recursively deletes the frame directory; build-video.mjs:55-58,191-197 uses t.id for output/frame/ASR paths; plan-timings.mjs:47-50 and build-video.mjs:102-105 also consume script-provided paths. An Agent-editable script.json value such as ../../victim can escape the intended build directory and trigger out-of-scope reads/writes/deletion. Add strict ID validation, resolve-and-containment checks, symlink checks, and malicious-ID tests.
  2. init-project.mjs:7-16,468-477 accepts an existing directory and then overwrites project files without a non-empty check or explicit --force. fetch-official-images.mjs:31-32,102-115 accepts arbitrary --out-dir and overwrites files; prep-image.mjs:64-87 uses ffmpeg -y for arbitrary output. This contradicts the README claim that writes stay inside the supplied project directory and creates destructive overwrite behavior. Default to refusing existing/non-empty targets and require explicit force, with output containment enforced.
  3. asr.mjs:29-32,65-74 allows --base-url / MINIMAX_BASE_URL to replace the endpoint without validation while sending the MiniMax API key as a Bearer token. A misconfiguration or prompt-controlled environment can exfiltrate the credential to arbitrary HTTPS/HTTP endpoints. Default to an HTTPS allowlist for official hosts; make custom endpoints an explicit, separately disclosed dangerous opt-in.
  4. fetch-official-images.mjs:25-29,40-46,98-115 accepts arbitrary http:, https:, and file: URLs and downloads through a browser/request client without protocol, private-address, redirect, or response-size restrictions. This exposes SSRF and local-file-read/copy behavior. Restrict to validated HTTPS public targets, block loopback/private/link-local/metadata addresses and redirects, bound responses, and make local files explicit opt-in with containment.
  5. The PR adds roughly 2,000 lines of executable scripts but no executable test suite; evals/evals.json is prompt/expected-output data and is not run by npm run check, while the repository validator does not execute these scripts or validate .claude-plugin/plugin.json. Add automated negative and smoke tests for containment, overwrite refusal, endpoint allowlist, SSRF/file rejection, and a minimal render/checker/build dry-run. The current [code]smith check is skipped and cannot substitute for this evidence.

Do not approve or merge until these security boundaries and executable test evidence are present on a new head.

@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

Thanks for the review — all five blockers were reproduced against the exact head you flagged and are fixed on the new head (v1.1.0). Point-by-point:

1. Input-derived IDs and paths are not contained — fixed.
script.json is treated as untrusted input. New shared helpers in scripts/tools.mjssafeId (whitelist ^[A-Za-z0-9_-]{1,64}$), safeRel (rejects absolute paths, resolves and requires containment in the project dir, and realpath-checks the deepest existing ancestor to refuse symlink escapes), inside, validateScriptPaths, validateTimingsIds — are called immediately after every JSON.parse in capture.mjs, build-video.mjs, plan-timings.mjs, check-timing.mjs, check-slides.mjs and asr.mjs, covering both the direct script.json chain and the timings.json second-hand chain. bgm.file no longer uses path.resolve (which let an absolute path escape entirely). Frame-picture width/height are coerced to integers in 16–16384 (they were interpolated into an ffmpeg scale= filter string). Evidence: tests/safe-paths.test.mjs — 12 cases including id="../../canary" against a canary directory outside the project (process exits 1, canary files verified intact), html="../outside.html", out-of-project <img src>, audio="../../secret.mp3", absolute bgm.file, and a symlink-escape case.

2. Destructive overwrite behaviour — fixed.
init-project.mjs now refuses a non-empty target directory and lists the files it would reset; re-initialising requires --force, which only resets its own five generated files and never deletes unrelated content (also fixed: --topic was interpolated into template HTML unescaped). fetch-official-images.mjs keeps --out-dir inside the working directory by default and refuses to overwrite existing files without --force (it no longer carries ffmpeg-style -y semantics); prep-image.mjs --crop refuses an existing destination unless --force is given, and the check runs before ffmpeg is invoked. The README claim about write scope is now backed by code. Evidence: tests/no-clobber.test.mjs — 9 cases including "non-empty dir → exit 1, unrelated file untouched" and "existing script.json content survives a refused run".

3. Endpoint replacement could exfiltrate the credential — fixed.
New scripts/url-policy.mjs exports assertAsrEndpoint: only https://api.minimaxi.com and https://api.minimax.io are accepted; any other --base-url / MINIMAX_BASE_URL is rejected before any request is constructed. Custom gateways require the explicit, separately-disclosed --allow-any-endpoint, which prints a warning. Evidence: tests/endpoint-allowlist.test.mjs — a third-party --base-url exits 1 with no request, and a positive case runs a local HTTP server that asserts it received Authorization: Bearer sk-test-not-real, proving the gate sits before the fetch and that the opt-in genuinely works.

4. SSRF / local-file read in fetch-official-images.mjs — fixed.
Same module: assertFetchableUrl / isBlockedHost reject loopback, link-local (incl. 169.254.169.254), private, CGNAT, IPv6 ULA/link-local and IPv4-mapped variants, dotless hostnames and .local/.internal/.localdomain/.home.arpa; only http(s) is allowed; file:// requires explicit --allow-file; URLs with embedded credentials are refused. Every redirect hop is re-validated (maxRedirects: 0 with a manual loop capped at 5), responses are capped at 30 MB (--max-mb), and download filenames go through sanitizeFilename (separators, control chars, leading dots, Windows reserved names). Evidence: tests/fetch-policy.test.mjs — 44 cases across host classification, URL validation, redirect targets and filename sanitisation.

5. No executable test suite — added.
skills/html2video-for-mcode/tests/ — five files, 72 node:test cases, zero dependencies, discovered by the repository-root node --test, so npm run check executes them (same convention as cli-agent-bridge / skill-bridge). The four security files run anywhere; the render smoke test (init → ffmpeg silence → plan-timings → check-slides → capture still → build-video, asserting the produced final.mp4 duration against the measured timings) skips with a stated reason where ffmpeg/Chromium are missing. Because the main CI image has neither, I added a plugin-scoped workflow following the pattern documented in CONTRIBUTING.md and the comments in ci.yml / tool-map-windows.yml: .github/workflows/html2video-for-mcode-smoke.yml (path-filtered to this plugin, installs ffmpeg + Chromium, runs all five files explicitly). Local runs: 72 tests, 0 failures.

Also in this head (non-blocking, from user feedback while the review was open): pure-CSS/SVG chart recipes with entrance-and-growth animations, a one-switch no-fx mode (animation end state vs no-fx frame measured at 51.7 dB PSNR — the画面 is identical), a new static gate for entrance animations whose keyframes never set opacity (they were silently invisible, same root cause class as the undefined-variable case), and the missing roadmap layout snippet.

The validator reports OK plugin Wzdhehe/html2video-for-mcode locally on this head, and the PR touches nothing outside plugins/Wzdhehe/html2video-for-mcode/ plus that single workflow file — upstream root README.md / LICENSE are unchanged (37e4c6cb / 125be1b8).

@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

追加:放映页(可以先自己放一遍再渲染)+ 修一个"文档说能用、闸门说不能用"的类

新提交 acdfefd7(插件 38 文件 / workflow 1 文件)在上一条评审回复的修复之上,补了一件这次实现过程中暴露出来的事:

1. 新增 scripts/preview-page.mjspreview/play/index.html(放映页)

单文件、零依赖、file:// 双击即看: 翻页、R 重播入场动画、P 提词面板(该张 clauses 按播放时间高亮)、O 总览、F 全屏、X 动效 / 关动效对照

为什么不是"直接打开 slides/*.html":动画延迟 --t1/--t2/--t3 与画布尺寸由渲染管线按 timings.json 注入,tokens.css 里只有占位值(--t2:800ms)。实测同一张 t=3.0s:副本里第二层 opacity 0(未入场),原文件里 opacity 1(已入场) —— 直接开原文件看到的是"所有动画挤在开头两秒"的假象。放映页生成快照副本,把实测延迟写进 <html style>(等价于 documentElement.style.setProperty,优先级最高)并加 <base href="../../slides/">,浏览器里的时序才等于成片时序。

2. init-project.mjs --upgrade-css:给老项目的 tokens.css 幂等补上新版 no-fx 规则。此前"用新版 init-project 重生成"的说法是错的 —— 那需要 --force,会重置 script.json

3. 修 fx-spotlight:它是本技能文档里列为可用的入场类,但关键帧只做 clip-path、没声明 opacity,[data-stage]opacity:0 基础态抬不回来 → 用了就永久隐形。它恰好会被 check-slides 的 5b 项拦住,即"文档说能用、闸门说不允许"。已补 opacity:1

4. 测试 +21 例(共 93,7 个文件):新增 preview-page(注入实测延迟 / base 顺序 / 自包含无外链 / 越界拒绝 / 部分张缺失时跳过)与 tokens-fx(对模板断言每个非无限入场动画的关键帧都必须声明 opacityno-fx 必须重置基础态、--upgrade-css 幂等且不碰其他文件);workflow 的显式文件列表已同步覆盖。负向对照:把 fx-spotlightopacity 去掉,该测试确实变红。

5. 文档漂移修正:插件树 README 此前落后仓库树一轮(缺整个 ## Verification 段与收紧后的 Network access / Data use 措辞),两棵树现在字节一致;11-script13-script

npm run check 语义未变;新增测试全部零依赖,不需要 ffmpeg/Chromium(渲染冒烟仍单独一步)。

@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

追加:74551814 —— 按使用反馈收窄放映页的定位

试用后的两条反馈,已改:

1. 去掉播放器那套 UI。 放映页的定位就是「把 HTML 画面放一遍」,口播文案是锦上添花;要看时间/节奏就直接看成片。所以删掉了:走秒的计时器(204.2s / 6.3s —— 标签页放着就毫无意义地涨)、进度条、逐句跟读高亮,以及随之而来的常驻 rAF 循环。现在页面是纯静态交互:翻页 / 重播 / 动效对照 / 总览。

2. 口播 UI 按数据决定加不加载,不再强制出现。

情形 页面表现
有 clauses + 有 timings.json 列出该张口播文案(P 可开 / 关)
有 clauses、还没对时 只列文案,标题标「(未对时)」—— 口播还没做也能先看 HTML
没有 clauses,或 --no-script 面板与口播按钮完全不出现,画面占满整宽

顺带修掉一个误导标签:底部提示原写成「X 关配音画对比」(本意是「关动效 / 画面对照」),读起来像是在管音频。现在统一写作「X 动效 / 关动效 对照」,测试里加了断言:页面不得出现「配音」字样。

3. 响应式(此前只考虑了桌面)。 原来右侧固定 320px 面板 + 固定行高的顶/底栏,窄窗口和手机上挤成一团。现在:顶栏/底栏可换行、话题名过长省略号截断;窄窗口与手机上口播面板收成底部抽屉并默认收起(画面优先);手机给触摸条按钮 + 左右滑动翻页;总览网格按宽度自动列数;高度用 100dvh(免得被手机地址栏切掉)。

实测(Playwright,五档视口 + 触屏模拟):1600×900 / 1024×600 / 800×600 / 390×844 / 360×640 全部零横向溢出、零控制台报错,触摸条无标签截断;390×844 上左滑确实翻页;窄屏下点「口播」画面从 385→755px 高。

另外把「还没对时」这一档做实用:没有 timings.json 时,副本按 HTML 里实际用到的 stage 等间隔排(0.3/1.3/2.3s),页面顶部黄条如实标注「不是成片时序」—— 占位值会把动画全挤在 2 秒内,那才是真看不懂。

测试 99 例(+6:不做计时器 / 不得出现「配音」字样 / 响应式与触摸 / 口播三态 / 等间隔兜底)。

@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Seventh-round audit (1.7.3, commits f2ba243f skill repo / d482a845 this branch) — one honesty gap found and closed

Our own standards pass over the current head found that five containment tests could pass vacuously: on a filesystem where neither symlinks nor junctions can be created, they ended with a bare early return inside the test body — green, but testing nothing. That contradicted this PR's own README promise ("nothing pretends to pass; skips carry a stated reason"). The review pointed at two; a repo-wide grep found five (safeRel escape, safeRel dangling link, unit-level safeOut, init-project --upgrade-css containment, init-project skeleton containment). All five now skip with a named reason, and the safeRel escape test gained the junction fallback so it really runs on stock Windows instead of skipping there.

Reproduced in both tool-less sandbox shapes after the fix — links creatable: 237 tests, 0 fail, 23 skipped; links not creatable: 209 pass, 0 fail, 28 skipped. The five formerly vacuous passes are exactly the five extra named skips; no shape reports a pass it did not earn. Development tree and both published trees: 237/237, 0 skip (with tools present).

Small dedups from the same pass: build-video reads --transition through the shared flagValue; the e2e image-listing fixture uses pathToFileURL. Disclosed without change: the --get loop re-walks redirects without self-loop detection but is bounded by MAX_REDIRECTS (redundancy, not a hole); the test-side Playwright probe costs one synchronous npm root -g at discovery.

Spec axis of the same round re-verified the standing public claims end to end — correction sweep (no standing "22 skipped" claim), 237/237 reproduction with tools and 0-fail reproduction without, description 977 chars, validate-plugin OK, remote byte spot-checks, credentials grep, --min WxH, --get exit code, KIT_PROJECT_DIR docs, VALUE_FLAGS, workflow form, manifests: all hold. Version is 1.7.3 in both plugin.json files; requesting re-review at d482a845.

…) + 测试基建去 file:// 拼接/tmp 清理; PR body 沙盒句已同步双形态数字(237→238)
@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Eighth-round audit (1.7.4, commits 3ef95ab2 skill repo / 50987c1f this branch) — standards axis: no serious findings; one stale body line corrected, three small hardenings

  • The PR body's sandbox sentence had gone stale — it still said both tool-less sandbox shapes report "0 fail and 23 skipped", which this branch's own 1.7.3 evidence supersedes (the links-not-creatable shape reports 28 skipped). The body now carries both shape numbers; the standards pass itself confirmed nothing else stale remains (all ten comments' older numbers are head-stamped history).
  • capture now validates script.width/script.height exactly like preview-page and build-video already did (integer, 16–16384): previously a non-integer died with a raw Playwright stack at browser launch, and an out-of-range value silently produced off-canvas frames. The gate sits before the Playwright load, so its new regression case (four bad shapes → exit 1, named error) runs on tool-less CI too.
  • Test-infra hygiene: the last two string-concatenated file:// imports now go through pathToFileURL (a checkout path containing # or % broke them loudly), and the suite's temp directories are removed on process exit rather than accumulating in %TEMP%.
  • Disclosed, not changed: preview-page's ?s=k stepping silently disables itself on hand-written slides with no <head> (generated slides always have one).

Evidence, reproduced today: development tree and both published trees (tools present): 238 tests / 238 pass / 0 fail / 0 skip (14 files). Tool-less sandboxes: links creatable → 215 pass / 0 fail / 23 skipped; links not creatable → 210 pass / 0 fail / 28 skipped, every skip naming its reason. validate-plugin → 52 files fingerprint-identical, upstream OK, exit 0. Remote bytes after push: skill repo 50/50 identical, this branch 53/53 identical. Cold-eyes pass this round also traced docs/evals claims (subtitle band 84–168px, 73% width, --fs-h2 44px, ASR --language/--verify-timing, check-theme 4.5:1, 72px table rows, .fx-stagger selector, all fx-* classes, evals 0–26 flags) against the code — all match. Version is 1.7.4 in both plugin.json files; requesting re-review at 50987c1f.

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for exact current head 50987c1f4e97e294713ff61c723435a3100e20f2.

The new path/network policy and tests close many earlier findings, but current-head blockers remain:

  1. Leaf symlink writes escape the project. preview-page.mjs:498-500 validates only preview/play; writes at :572, :576, and :612 use plain path.join/writeFileSync. Existing 01.html, 01.nofx.html, or index.html symlinks can therefore redirect writes outside the project. Apply safeOut to every leaf and add a canary proving an external target is unchanged.
  2. safeRel still accepts an ancestor symlink for an existing project root. tools.mjs:144 permits inside(real, realRoot), so a project-local link to the project's parent can be treated as valid. Allow that fallback only while the root itself does not exist; once it exists, require equality or containment beneath the canonical root. Add an ancestor-symlink regression.
  3. Single-file ASR output bypasses containment/no-clobber. asr.mjs:156-166 writes arbitrary --out paths directly and overwrites existing files. Bring this path under the documented project boundary and explicit force/no-clobber policy, with tests.
  4. prep-image --crop has no project containment contract. prep-image.mjs:59-63,83 prevents overwrite unless --force, but still accepts arbitrary output paths. Either enforce project containment or explicitly document this command as a general-purpose exception; do not leave it inconsistent with the README's write-boundary promise.
  5. No exact-head executable CI evidence yet. The fork runs exist but are gated as action_required with no jobs; the only visible PR check is skipped [code]smith. A maintainer must approve the fork workflows and obtain real smoke/negative-test results before merge.

Confirmed fixed on this head: ID/traversal checks, directory-symlink containment on the main render path, recursive-delete guards, most no-clobber behavior, official ASR endpoint allowlisting, and per-hop SSRF/private/file/redirect/size policy. The remaining findings above are specific to the current files, not carried over from the prior head.

…的写穿洞)/preview-page 三叶子收监/asr --out 收监+不覆盖/prep-image --crop 例外声明(238→241)
@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Thank you for the precise review — all four code findings are real, and all four are fixed in 1.7.5 (154eb790 on this branch, 21bb9d26 on the skill repo). Point by point:

1. Leaf symlink writes in preview-page — fixed. All three write sites (01.html, 01.nofx.html, index.html) now go through safeOut, so a pre-planted file symlink inside preview/play/ is rejected before any write. New canary: preview/play/index.html as a file symlink to a canary file outside the project → exit 1, canary content unchanged. File symlinks need Windows Developer Mode, so on Windows the canary skips with that stated reason and really runs on Linux CI.

2. safeRel ancestor-symlink fallback — fixed, and it reproduced exactly as you described. With a project-local link to the project's parent and a not-yet-existing target under it, the walk stops at the link, the link's realpath is the parent, and the old inside(real, realRoot) fallback matched "root inside parent" → the path was accepted and a write landed outside the project (we reproduced the escape live before fixing). The fallback now applies only while the root itself does not exist; once it exists, the resolved path must be the root or beneath it. Regression in safe-paths covers both the new-target shape (the hole) and the existing-target shape.

3. asr.mjs --out — fixed. It now accepts only a project-relative path that resolves inside the project (absolute paths are rejected with a named error), refuses an existing output without --force, and the validation runs before the transcription request — so the refusal paths are testable on bare CI (three new assertions: traversal refused with nothing written outside, absolute path refused, existing file untouched).

4. prep-image --crop — declared exception. We took the second option: both READMEs now carve it out of the write-boundary promise explicitly — its input and output are given on the command line (the same trust level as running ffmpeg yourself), overwriting an existing output still requires --force, and every derived path (project + id, preview/, build/, out/, asr.mjs --out) remains fully contained. The script header states the same contract.

5. CI evidence — agreed, and this one is on the maintainer side. The fork's workflow runs sit at action_required; they execute only after a maintainer approves them. Once approved, the scoped workflow on this branch installs ffmpeg + Chromium and runs the entire suite (241 tests across 14 files) via shell-glob discovery. We would appreciate that approval so the runs appear on the PR.

Evidence, reproduced today at 154eb790:

development tree + both published trees (tools present):  241 tests / 240 pass / 0 fail / 1 skip
  (the one skip is the file-symlink canary on Windows: "needs Developer Mode; runs on Linux CI")
tool-less sandbox, links creatable:      217 pass / 0 fail / 24 skipped (reasons stated)
tool-less sandbox, links not creatable:  211 pass / 0 fail / 30 skipped (reasons stated)
publish/validate-plugin.mjs → 52 files fingerprint-identical · OK plugin Wzdhehe/html2video-for-mcode · exit 0
remote bytes after push: skill repo 50/50 identical · this branch 53/53 identical

Version is 1.7.5 in both plugin.json files. Requesting re-review at 154eb790.

@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Ninth-round audit (1.7.6, skill repo e2f30603 / this branch tip) — one last member of the leaf-containment class, and the body sentence fixed so it stops going stale

  • grab-frames frame writes are now leaf-contained. Our standards pass over the 1.7.5 tree found the one remaining member of the class your review had us close in preview-page: build/introspect/ was jailed as a directory, but frame-<id>-<tag>.png was written with plain path.join (the inline comment argued the safeId whitelist rules out injection — true for traversal, blind to a pre-planted file symlink that ffmpeg -y would write through). The leaf goes through safeOut now, with a canary asserting the outside file is untouched (needs ffmpeg + file symlinks → runs on Linux CI, names its skip on stock Windows). A sweep of every remaining raw write/delete site in scripts/ found no other member of this class.
  • The PR body's tool-less-sandbox sentence no longer carries per-version numbers. It named "23 / 28 skipped" — correct when written, then stale twice in a row, because every release adds tests and skip counts legitimately drift with the environment. The body now states the invariant ("every test that needs a tool skips with its stated reason — nothing pretends to pass") and points at the CHANGELOG for per-version measurements.
  • Hygiene: the ancestor-link canary file is removed after the test instead of lingering in the shared temp root.

Evidence at the current tip: development tree and both published trees (tools present) 242 tests / 240 pass / 0 fail / 2 named skips (the two file-symlink canaries on stock Windows); tool-less sandboxes in both link-capability shapes 0 fail (25 and 31 named skips respectively); publish/validate-plugin.mjs → 52 files fingerprint-identical, upstream OK, exit 0; remote bytes verified tree-level (blob SHA): skill repo 50/50, this branch 52/52 + the workflow, zero internal files. Version is 1.7.6 in both plugin.json files.

All four findings from your third review remain fixed and covered by the regression tests described in the previous comment. Requesting re-review at the current tip — and, when you get a chance, approving the fork's workflow runs so the scoped CI can execute on the PR.

…此类清零"自证被独立清点推翻) + 明文更正(242→243)
@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Tenth-round audit (1.7.7, skill repo d450ad36 / this branch e2ae2a47) — our own 1.7.6 claim was wrong, and an independent sweep caught it

Our 1.7.6 changelog said a sweep found "no other member of this class" (derived-but-unjailed leaf writes). The spec axis of the tenth audit re-did the write-site inventory independently and found exactly one more: asr.mjs's transcode temp — oversized inputs (>500 s / >50 MB) were transcoded to .asr-<name>.16k.mp3 beside the input file, so for an input inside the project a pre-planted file symlink at that derived name would be written through by ffmpeg -y. Fixed in 1.7.7: the temp goes to a per-invocation mkdtemp under os.tmpdir() (removed whole when the attempt ends), and a new canary drives a real 501-second input — the transcode runs before any network request — with the old temp name planted as a file symlink next to the input: nothing appears in the input directory, the canary is untouched. The correction to our own 1.7.6 claim is stated plainly in the 1.7.7 changelog; the lesson (an absence claim is only as good as the inventory it ran on) is recorded.

Also in 1.7.7: build-video's part-*.mp3 writes are protected by order (the preceding jailed delete of every old part-* rejects a pre-planted leaf symlink before the ffmpeg write) — that invariant is now a code comment so the ordering is not accidentally changed.

Evidence at the current tip: development tree and both published trees (tools present) 243 tests / 240 pass / 0 fail / 3 named skips (the three file-symlink canaries on stock Windows); tool-less sandbox shapes 0 fail with every skip named; publish/validate-plugin.mjs → 52 files fingerprint-identical, upstream OK, exit 0; remote bytes verified tree-level (blob SHA): skill repo 50/50, this branch 52/52 + workflow, zero internal files. Version is 1.7.7 in both plugin.json files. Requesting re-review at e2ae2a47.

@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Eleventh-round audit (1.7.8, skill repo 5484bd00 / this branch 9b62df6a) — the write-site inventory converges, and one of our own canaries had an assertion that could never pass

  • The 1.7.7 transcode canary's "nothing appears in the input directory" assertion was unsatisfiable. The test plants a symlink at the old temp path and then asserted existsSync(that path) === false — but the plant itself makes the path exist (the link resolves to its target), so on any platform where the test actually runs (Linux CI, Developer-Mode Windows) it would have failed; it had never executed anywhere. Caught by the spec axis' claim audit before CI ever ran. The assertions now state what the fix actually guarantees: a before/after diff of the input directory shows no new entries, the outside canary file is byte-unchanged, the planted path is still a symlink (not replaced), and the os.tmpdir() transcode directory is gone by the end of the run.
  • Write-site inventory converged. This round's standards pass used a different method — primitive-exhaustive enumeration (every writeFile/rm/copy/symlink call and every spawned-tool output path, 58 sites across 20 scripts) instead of file-by-file intuition — and found no exploitable unjailed write: 47 jailed, 9 CLI-given per the documented contract, 4 internal temp, 1 order-protected (disclosed with a code comment in 1.7.7).
  • PR-body counts re-phrased as capability invariants. Per-environment numbers ("0 skip") had gone stale three times running; the body now carries only what is invariant (0 fail in every environment measured) and describes skips as capability-dependent and always named.

Evidence at the current tip: development tree and both published trees 243 tests / 0 fail (240 pass + 3 named file-symlink-canary skips on stock Windows); tool-less sandboxes in both link-capability shapes 0 fail (26 and 32 named skips); publish/validate-plugin.mjs → 52 files fingerprint-identical, upstream OK, exit 0; remote bytes verified tree-level: skill repo 50/50, this branch 52/52 + workflow, zero internal files. Version is 1.7.8 in both plugin.json files. Requesting re-review at 9b62df6a.

@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Twelfth-round audit — convergence. Both axes report no serious findings, and this rounds off the loop that began with your third review.

  • Standards axis (primitive-exhaustive method): no serious findings. The 1.7.8 canary rewrite traced clean assertion-by-assertion (the planted path sits inside the before-snapshot; every bug shape is caught by exactly one of the four assertions; the os.tmpdir() cleanup assertion is race-free in this suite). The write-site class that produced the last three findings is closed: 1.7.5 closed your four, 1.7.6 closed grab-frames, 1.7.7 closed the asr transcode temp (with an honest correction of our own false "no other member" claim), 1.7.8's independent primitive-level inventory across all twenty scripts found nothing left, and 1.7.9 is a docs-only correction of that entry's site-class arithmetic (the classes overlapped; the conclusion was never in doubt).
  • Spec axis: all claims verified. The capability-invariant phrasing in the PR body now matches measurement in every environment we can produce (0 fail everywhere; skips capability-dependent and always named); both plugin.json files, both CHANGELOG copies, the remote trees (blob-SHA verified), and validate-plugin (52 files fingerprint-identical, upstream OK, exit 0) all agree at the current tip 51b127f7. One dated item noted and accepted: the body's "Validated 26 hosted Plugins" is a timestamped transcript from an earlier run (the host has since gained unrelated plugins) — it is a historical log line, not a present-tense claim.

Current state at 51b127f7: 243 tests in fourteen files, 0 fail in every environment measured (240 pass + 3 named file-symlink-canary skips on stock Windows; 0 skips on symlink-capable systems; tool-less sandboxes in both link-capability shapes report 0 fail with every skip named); versions 1.7.9 in both plugin.json files; no internal files on either remote.

All twelve of your review findings across three reviews are closed with named regression tests, and our own eleven additional audit rounds each closed what they found the same way. The remaining gate is yours: approving the fork's workflow runs will put the full suite's execution on the record for this PR. Requesting re-review at 51b127f7.

…双语安装段收束)+ 付费配额披露 + 重定向循环合一(--get 补环/0字节守卫)+ ffprobe 探测收进 tools + canary 扩展逐张叶子(243→244)
@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Thirteenth review round processed (1.8.0, skill repo 5a37e0bc / this branch e845cbd8) — both hard violations fixed, three duplications closed, the judgement calls disclosed

Hard violations — fixed

  1. The second repository is no longer an install channel. Confirmed against docs/architecture.md ("the hosted directory is the publication unit … contributors do not create a second repository"). homepage in both plugin.json files now points at the hosted plugin directory (MiniMax-Code-Plugins/tree/main/plugins/Wzdhehe/html2video-for-mcode), and both install sections route exclusively through it — "copy the reviewed skill directory out of the hosted plugin, not from any other source". The npx skills add Wzdhehe/… route and the bare cp -r framing are gone from both languages; the standalone repository remains only as a read-only history mirror. (The sentence "this standalone repo has no workflows" — wrong-context in the plugin-root copy, which lives in the monorepo — is now context-neutral in both.)
  2. The paid account is disclosed as paid. Both READMEs' requirement sections now state that mcode connectors and mmx-cli/REST spend a MiniMax account's paid, quota-metered balance (billed per character/minute) and that the MINIMAX_API_KEY used by asr.mjs belongs to the same billable account with per-call transcription billing.

Duplications — closed (the three most concrete of your items 4-8)

  • fetch-official-images' two redirect loops are now one followRedirects(src, get) core with the client injected as an adapter; the --get path thereby gains the ping-pong cycle guard and the zero-byte body guard it was missing. A structural regression test pins: exactly one hop loop, both call sites go through the core, both zero-byte guards present. (A behavioural cycle test can't run locally — the SSRF policy refuses loopback by design and public URLs are unreliable in CI — hence the structural form.)
  • ffprobe probing consolidated into tools.mjs (probeDuration, probeSize), replacing the three verbatim duration copies and two size copies, behaviour-identical.
  • The preview-page file-symlink canary now covers a per-slide leaf (01.html) in addition to index.html — all three write sites have canary coverage where the platform allows file symlinks.

Judgement calls — disclosed, not changed (all from your items 3-5, 8, 9)

  • review-round2/review-round3 are named by review provenance; each describe block inside is topical, and the suite names are documented verbatim in both READMEs — renaming would orphan that documentation for no behavioural gain.
  • capture.mjs's fade constants are re-derived in the Node self-check on purpose: an independent derivation catches drift between the page script and the check; sharing one constant would hide exactly the drift a self-check exists to catch.
  • The subtitle-band geometry (render-side bar position vs check-side zone top) uses intentionally different numbers for different questions ("where is the bar" vs "which region must stay clear"); unifying them would change gate behaviour mid-review and is recorded as debt.
  • css-kit's region-range merging appears in two different operations (locate for status, remove for upgrade); preview-page's buildPlayPage embeds the play page as one long template — structural trade-offs, not defects.

On your Spec notes — agreed on all: the four code fixes closed as you assessed them (canary scope now widened as above); the CI gate remains maintainer-side (action_required — we again ask for approval so the Linux-only canaries finally execute); publish/validate-plugin.mjs is an authoring-side tool, deliberately not part of the shipped tree — the PR body now says so, and its evidence transcript was refreshed to a current validator run (27 hosted plugins — the host has merged unrelated plugins since the old transcript).

Evidence at e845cbd8: development tree and both published trees 244 tests / 0 fail (241 pass + 3 named file-symlink-canary skips on stock Windows; 0 skip on symlink-capable systems); tool-less sandboxes in both link-capability shapes 0 fail (26 / 32 named skips); validate-plugin → 52 files fingerprint-identical, upstream OK, exit 0; remote bytes tree-SHA-verified 50/50 + 52/52 + workflow, zero internal files. Version 1.8.0 in both plugin.json files. Requesting re-review at e845cbd8.

@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

Linux execution of the full test suite — closing the open "CI evidence" item.

The scoped html2video-for-mcode-smoke workflow on this PR is still action_required (it needs a maintainer approval click before its first run), so until then the same suite — same invocation (node --test tests/*.test.mjs), same dependency set (ubuntu-latest, real ffmpeg, Playwright Chromium) — now runs in the Actions of the read-only mirror repo, where no approval gate applies:

https://github.com/Wzdhehe/html2video-for-mcode/actions/runs/35433539493

Frozen results of that run: 244 pass / 0 fail / 0 skipped / 0 cancelled — every test executed, no capability-based skips. In particular, the three file-symlink canaries (preview-page leaf index.html and per-slide leaf, grab-frames introspect leaf, asr 16kHz transcode temp) executed for the first time on Linux and passed — creating file symlinks requires Developer Mode on Windows, so until now they could only ever be skipped locally. The 128s cover/xfade end-to-end render test ran as well.

The workflow re-runs on every push to the mirror, so this evidence stays current. Once the scoped smoke workflow is approved on this PR, the same suite runs on the PR itself.

@Wzdhehe

Wzdhehe commented Sep 19, 2026

Copy link
Copy Markdown
Author

cc @hetaoBackend for a re-look when you have a moment — the Linux run linked above closes the open CI-evidence item, and the scoped smoke workflow on this PR needs one approval click to run here.

@Wzdhehe Wzdhehe left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review · PR #41 (html2video-for-mcode · 1.8.1)

Verdict: ready to merge pending the maintainer actions listed at the bottom.

This is the largest single-plugin PR I've seen in this repo (53 files, ~5k lines of scripts + ~3.2k lines of tests, ~50 KB SKILL.md). The author has done ~13 rounds of self-audit on the published skill, and the only third-party review actions so far are from the requested reviewer hetaoBackend, who has not yet posted comments — so this is mostly a standards/spec pass.

What's in the PR

A complete skill for turning an outline into a narrated MP4: HTML slides with staged entrance animations, TTS voiceover, ffmpeg assembly with cover/transition/BGM handling, and ASR-based reverse verification. Ships as a standalone plugin tree at plugins/Wzdhehe/html2video-for-mcode/, plus a scoped smoke-CI workflow at .github/workflows/html2video-for-mcode-smoke.yml. 27 plugins now hosted in the repo after merge.

What I checked

I cloned the PR branch, ran the host's scripts/validate.mjs, ran the skill's tests locally (subset, ffmpeg available, chromium not), inspected every script, the security modules, the test canaries, the workflow, and the host's existing conventions.

$ node scripts/validate.mjs
OK   plugin Wzdhehe/html2video-for-mcode
…
Validated 27 hosted Plugins and all examples.

Test runs in this sandbox (Node 22, ffmpeg present, chromium absent): safe-paths 22/22, fetch-policy 77/77, endpoint-allowlist 6/6, no-clobber 13/13, preview-page 30/31, review-round3 6/7, chart-kit 7/7, render-smoke 1/6 with 5 chromium-skips, cover-transition 3 skips (chromium). The chromium-skips are by design and name themselves.

What is genuinely good

1. The security model is unusually thorough for a skill plugin. Three layers worth calling out:

  • Path containment (scripts/tools.mjs): safeId/safeRel/safeOut/assertContained use lstat-not-existsSync traversal to catch dangling symlinks, and they fail-closed when realpath cannot resolve a link. The "1.7.5 ancestor-link regression" — a link inside the project pointing at the project's parent, which used to slip past the "root doesn't exist yet" fallback — is caught and pinned by a canary test (safe-paths.test.mjs line 99-120). I verified the canary actually creates the shape and asserts the rejection happens before any write. This is exactly the property tests should pin.
  • SSRF / DNS rebinding (scripts/url-policy.mjs): end-to-end — block IPv4-mapped IPv6, NAT64, 6to4, fc00::/7, fe80::/10, the 13 RFC-reserved IPv4 ranges, trailing-dot FQDNs, and bare hostnames; and assertResolvedHost re-checks the resolved address right before each request so a hostname that flips DNS to 169.254.169.254 mid-flight is still rejected. Test coverage includes both the static and the post-DNS paths.
  • API-key exfiltration: ASR key only goes to two allowlisted domains; --base-url and MINIMAX_BASE_URL are validated against the same allowlist; the only escape is an explicit --allow-any-endpoint that warns loudly. MINIMAX_BASE_URL=https://evil.example is verified to exit 1 before any network call.

2. ffmpeg filter-injection surface is closed. width/height/fps/bgm.volume/bgm.fadeIn/bgm.fadeOut are all clampNum-validated in build-video.mjs (and the same check exists in capture.mjs for width/height, where the value reaches CSS/JS). Each one has a canary test that feeds an actual injection string like "0.5,amovie='C:/x',volume=0.5" and asserts the rejection names the field. I think this is the right place to be paranoid — ffmpeg -filter_complex accepts arbitrary expression syntax.

3. The "managed CSS region" design is novel and well-implemented. scripts/css-kit.mjs defines content-rev delimiters, and tokens-template.mjs exports a single source-of-truth body with a TOKENS_REV hash. check-slides, capture, build-video, preview-page, init-project --check-css, and init-project --upgrade-css all share the same expected content and the same status vocabulary. The reason this matters: a stale tokens.css will render successfully with the wrong styles (new classes get no rules, old rules win by cascade) — exactly the silent failure mode that motivated the design. Capturing the freshness check at every entry point is the right call.

4. Test design is honest. Tests aren't smoke tests; they're property tests that construct the threat model and assert the rejection. The "self-correcting changelog" (1.7.7/1.7.8 entries note that an earlier canary had an unsatisfiable assertion because the test planted the symlink it then measured) is rare transparency — most projects would silently delete such a test.

5. CI hygiene. Action versions are pinned to full 40-char SHAs (actions/checkout@3d3c42e…, actions/setup-node@82076278…) with # vX.Y.Z comments. permissions: contents: read is set. Trigger paths are scoped to this plugin's directory plus the workflow file. --test-timeout=300000 is set so the chromium-installed run doesn't get killed mid-encode. The host's own ci.yml has a typo (42-char SHA on actions/checkout); the author notes this in the workflow comment and correctly doesn't touch the host's file.

6. License + disclosure are correct. MIT, third-party notices for the adapted lewislulu/html-ppt-skill material with date + scope of adaptation, paid-account disclosure in both READMEs ("MiniMax account's paid, quota-metered balance") per the round-13 finding.

Concerns / questions

These are observations, not blockers — most have defensible answers in the PR body.

1. homepage and repository both point at this repo. The validator passed them, but homepage pointing at the hosted directory inside the host repo is a deliberate design choice (per round 13) to make the host repo the canonical install surface rather than the author's standalone mirror. This is a host-side policy question rather than a code defect — flagging it for the maintainer's awareness.

2. review-round2.test.mjs / review-round3.test.mjs are named by audit provenance, not topic. The author discloses this in CHANGELOG 1.8.0 ("renaming would orphan the suite list documented in both READMEs"). Defensible — I'd just want the topic-level describe blocks inside to be self-contained so a future contributor doesn't need to read the audit history to find what each one covers. They appear to be.

3. The scoped smoke CI is still action_required. The author worked around it by running the same suite in the Actions of the standalone mirror (run 35433539493, 244/0/0). That's fine evidence but it's not the host's CI. The maintainer should approve the workflow so it runs on this PR's check suite before merging — otherwise the only CI evidence is from the author's own infra.

4. Several tests take >30 s in this sandbox. safe-paths took 110 s (it constructs real symlinks via subprocess mklink /J fallback and runs four full canary paths). css-kit timed out at 30 s. These are all pass — just slow. Worth knowing for the maintainer's local sanity-check budget; not a defect.

5. Playwright Chromium is installed via npx playwright install --with-deps chromium in the workflow. On a sandbox runner this needs --no-sandbox in some configurations; the author doesn't set it. I didn't see a Chrome-launch failure in the tests I ran, so the default works on ubuntu-latest. If the maintainer hits a sandbox error on a fork or self-hosted runner, that's where to look.

6. The BGM loop-seam is not explicitly tested. When script.bgm is configured and the source clip is shorter than the video, build-video does -stream_loop -1 and afade. There's no test asserting that the seam at the loop point is inaudible or at the right phase. Probably fine (most BGM clips are written to loop cleanly) but worth a manual spot-check if the maintainer can spare 10 minutes.

7. scripts/preview-page.mjs is 637 lines and embeds the play page as one long template string. The author discloses this as "a structural trade-off, not a defect" in 1.8.0. Reasonable — splitting a single HTML artifact across many small strings would be worse. Just noting that any future contributor who touches this file will need to be careful not to break the embedded JS.

Maintainer actions before merging

  1. Approve the .github/workflows/html2video-for-mcode-smoke.yml first-run gate so the workflow actually executes on this PR. The author's external run is good evidence but the in-host CI tick is the contract.
  2. Confirm the host's policy that homepage may point inside the host repo (it currently does — round-13 fix — and the validator accepts it). If that's the standing policy, no action; if not, this PR needs the URL reverted.
  3. Optional but valuable: spot-check a 60-second sample render on each of the three real runtimes the README lists (Windows / macOS / Linux). The Chromium-skip test count drifts per-platform — that's by design — but a real cross-platform smoke is cheap.

Bottom line

The PR is unusually well-engineered for a skill plugin submission. The security model has the right threat model in mind (path traversal, DNS rebinding, ffmpeg filter injection, key exfiltration), every concern is pinned by a canary test, and the self-audit is transparent. The action items for the maintainer are mostly about CI gating and policy decisions, not code fixes.

@Wzdhehe

Wzdhehe commented Sep 20, 2026

Copy link
Copy Markdown
Author

1.9.0 — ASR toolchain simplification: mmx speech transcribe is now the default provider. mmx-cli ≥ 1.0.26 shipped speech transcribe (upstream MiniMax-AI/cli#262), so the non-mcode ASR path no longer requires a separately configured key.

  • scripts/asr.mjs picks the provider automatically: mmx-cli ≥ 1.0.26 on PATHmmx speech transcribe (same login as TTS — the API key never passes through this script on that path; results are staged through a dedicated os.tmpdir() scratch dir, so the external CLI never touches user paths). --provider api keeps the previous direct-REST path for environments without mmx-cli; --from (offline compare) no longer trips the missing-key gate. The endpoint allowlist is unchanged and provider-independent — a non-official --base-url/MINIMAX_BASE_URL is still rejected before any provider resolution. The capability probe reads mmx --version (--help exits 0 even for unknown subcommands, so it cannot gate); on Windows the CLI is invoked through cmd.exe /c (Node cannot spawn npm-global commands directly).
  • Docs corrected where they stated "mmx-cli has no ASR subcommand" — true until 1.0.25, false since: SKILL.md (scripts table, runtime comparison, first-time setup, Phase 5), references/tts-and-timing.md (ASR paths B/C), both READMEs' requirements/disclosure bullets (the mmx provider spends the same paid, quota-metered MiniMax account).
  • Tests +4 → 248 tests / 14 files: invalid---provider guidance, keyless --provider api guidance, and an mmx-shim integration pair asserting the exact argv mapping (speech transcribe --model asr-1.0 --response-format … --timestamp-level … --language …), tmpdir staging/cleanup, and failure passthrough; REST-stub tests now pin --provider api so they stay deterministic on machines that do have mmx. Verified on all three trees locally (245 pass / 0 fail / 3 capability skips), end-to-end against real mmx 1.0.26 (keyless run, exact transcript including the number), and on Linux via the mirror repo's CI which runs on every push: run 35491884656 — 248 pass / 0 fail / 0 skipped, with the new provider tests executing on ubuntu.

…uoting, scrub mmx child env, assert full argv mapping
@Wzdhehe

Wzdhehe commented Sep 20, 2026

Copy link
Copy Markdown
Author

1.9.1 — sixteenth review round fixes (two-axis review over the 1.9.0 increment; every finding personally reproduced before fixing):

  • The plugin-root READMEs were missed by 1.9.0 — the hard finding both axes caught: the 1.9.0 doc updates reached the skill-repo copies, but the copies inside plugins/Wzdhehe/html2video-for-mcode/ (the tree this PR publishes) still described ASR as "Optional: MINIMAX_API_KEY … direct REST" and carried the old 244-test count. Both are now byte-identical to the updated versions (requirements/disclosure bullets + 248-test count).
  • Wording corrected: 1.9.0 overstated "the external CLI never touches user paths" — the input audio is passed to mmx read-only at its original path; only the output is staged through os.tmpdir() before landing in jailed paths. Comments now say exactly that.
  • Windows cmd.exe quoting hardened: args were quoted only on whitespace; a legal path containing &/|/</>/(/)/^ would be re-interpreted by cmd as control characters. Any metacharacter now triggers quoting (documented residual: %VAR% inside quotes still expands — worst case is mmx failing loudly on a mangled path). Regression-covered by running the mmx-shim test inside a project directory named a&b c.
  • MINIMAX_API_KEY / MINIMAX_BASE_URL stripped from the mmx child environment — the key never reaches the external CLI even by inheritance; the shim test asserts ENVKEY=absent.
  • The shim test now asserts the full argv mapping including --response-format verbose_json --timestamp-level word (1.9.0's changelog listed --timestamp-level, which the test did not actually cover); the duplicated two-test setup was extracted into one helper, and dead env plumbing was removed.

Verification: three trees locally 248 tests / 245 pass / 0 fail / 3 capability skips; real end-to-end against mmx 1.0.26 re-run keyless with word-level timestamps (exact transcript, n_speakers parsed); Linux CI on the mirror for this push: run 35492908869 — 248 pass / 0 fail / 0 skipped; upstream validator OK via the staging+fingerprint entry point; remote trees byte-verified after push (51 + 52 files, zero drift, zero docs/ leakage). Judgement-call items disclosed rather than changed: the version probe anchors on the first semver in mmx --version output, and the tests' shim-probe helper deliberately re-uses the same spawn shape as mmxRun to validate the shim itself.

…EY assertion regression-proof, disclose Windows-only quoting coverage
@Wzdhehe

Wzdhehe commented Sep 20, 2026

Copy link
Copy Markdown
Author

1.9.2 — seventeenth review round fixes. Both axes caught the same two defects in 1.9.1's own fix; each was personally reproduced before fixing:

  • Env scrub moved to the single spawn source. 1.9.1 stripped MINIMAX_API_KEY/MINIMAX_BASE_URL only from the speech transcribe child — the mmx --version capability probe still inherited them, so "the key never reaches the external CLI even by inheritance" was overstated. The scrub now lives inside mmxRun (the only spawn point), covering every mmx child including the probe.
  • The ENVKEY=absent assertion was vacuous — it read the first ENVKEY= line of the shim log, which the test's own setup probe writes; it passed regardless of the transcribe child's env and would fail spuriously where MINIMAX_API_KEY is exported. It now requires every ENVKEY= line (probe and transcribe alike) to be absent, and is red-proofed: removing the scrub from mmxRun turns the test red (verified locally, then restored, then green).
  • Disclosure correction: the a&b c metacharacter-directory regression exercises the cmd.exe quoting path only on Windows; on POSIX the child is spawned directly and metacharacters are inert by construction (the Linux CI run proves path handling there, not the quoting).

Verification: three trees locally 248 tests / 245 pass / 0 fail / 3 capability skips; real keyless end-to-end re-run against mmx 1.0.26 (exact transcript); upstream validator OK; remote trees byte-verified after push (51 + 52 files, zero drift, zero docs/ leakage). Commits: skill mirror 3832c4a7, PR branch 4c9926e3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants