Skip to content

Add Plugin: html2video-for-mcode - #41

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

Wzdhehe wants to merge 39 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: 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
Validated 26 hosted Plugins and all examples.
$ echo $?
0
$ node --test "tests/*.test.mjs"     # 237 tests: 237 pass, 0 fail, 0 skip (14 files)

All 237 skill tests pass, with 0 skips, in the development tree and in both published trees. 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 two isolated tool-less sandboxes (symlinks/junctions creatable, and not creatable), both reporting 0 fail and 23 skipped with stated reasons — and the scoped workflow installs them 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 18, 2026

Copy link
Copy Markdown
Author

总计:评审之后的全部优化与新增(1.1.0 → 1.6.0,测试 93 → 174)

五条阻塞项的逐条修复见前面的回复;本条把其余的全部优化与新增(含新增的预览模式)汇总成一份,便于整体复核。对应 CHANGELOG 1.2.0 / 1.3.0 / 1.3.1 / 1.3.2 / 1.3.3 / 1.3.4 / 1.3.5 / 1.3.6 / 1.3.7 / 1.4.0 / 1.5.0 / 1.6.0 十二段。

1. 预览模式(放映页,新增交付物)

新增 scripts/preview-page.mjs <项目> [--open] [--no-script]preview/play/index.html:单文件、零依赖、file:// 双击即看的放映页,现在也是 Gate 4 的正式验收物——"元素永久隐形 / 关动效反而空白 / 列高塌陷"这类静默故障在浏览器里 30 秒可见,不必等 3–6 分钟的 motion 编码。

  • 只做"把 HTML 画面放一遍": (触屏左右滑)翻页、X 动效 / 关动效对照O 总览、F 全屏;刻意不做计时器/进度条/逐句跟读那套播放器 UI(要看时间就看成片)。动效开的按键语义后来重做为手动逐级入场,见 §11。
  • 口播文案面板按数据决定加不加载:有 clauses + 有对时 → 列出该张口播;有 clauses 没对时 → 只列并标"(未对时)";没有 clauses 或 --no-script → 面板完全不出现。窄窗口/手机上收成底部抽屉且默认收起(画面优先)。
  • 快照机制保证时序真实:副本把 timings.json 里的实测 stage 延迟写进 <html style>(等价渲染管线注入)——直接双击 slides/*.html 看到的是"动画全挤在开头两秒"的假象(tokens.css 只有占位值),放映页里浏览器播放时序 = 成片时序;副本带 <base href> 指回 slides/,改完 slides 重跑即可。
  • 评审后的持续加固(未在前两条追评里):兜底注入从 no-fx 扩到三个工具箱(旧项目不升级也能在放映页看到当前正确行为,页面与终端双提示);竖版 1080×1920 画布参数化(原先写死横屏,竖版被切掉近半);iframe src 强制相对解析(挡 javascript: 文件名);2MB 输入上限。

2. 图表与表格工具箱(评审后陆续合入)+ CSS 传播根治(v1.2.0)

  • 图表工具箱 v2:@property 注册属性驱动的纯 CSS 动效(fx-sweep 环形扫出 / fx-grow-w 宽度生长 / fx-count 数字滚动 / fx-dot 数据点),全部逐帧可 seek;.chart* 原语(三列定宽行、plot-cell/bar-wrap 三层柱结构、.chart-ticks 刻度列、两种朝向的目标线)。经 7 轮视觉验收收敛出九条数据可视化纪律(同类同色一条强调、柱高 = 数值÷轴上限且轴从 0 起、数值不放进被缩放元素等),写进 references/authoring.md
  • 表格工具箱:.tbl / .kv / .matrix / .rank 四原语,可读性硬指标(主数据正文号 30px、行高 ≥72px、数值右对齐等宽、只画横线)由测试盯住。
  • 受管块机制(根治"改了 CSS 但项目里没生效"):新增 scripts/css-kit.mjs,三个工具箱(no-fx / 图表 / 表格)以带内容 rev 的受管块写进项目 tokens.css;--upgrade-css 从"存在性探针 + 追加"重写为按 rev 原地替换——旧实现补过一次就永远报"无需升级"(源模块后续改动永不传播)、追加还会压掉项目端覆写并留重复段,这两类缺陷都有专门回归测试。写前备份 tokens.css.bak;新增 --check-css(只查不改,落后退出码 1);check-slides 加"工具箱落后 / 类名无定义"两项提示;preview-page 的兜底注入扩到三段。
  • .matrix 高亮列不覆盖表头(<th class="hi"> 此前静默拿不到背景)。

3. 三维审计(文档对齐 / 安全 / 流程)修复(v1.3.0)

  • 安全:
    • SSRF:IPv4-mapped IPv6 十六进制形式穿透——WHATWG URL 会把 http://[::ffff:127.0.0.1]/ 的 hostname 规范化成 ::ffff:7f00:1,旧逻辑剥前缀后剩余 hex 组落不进任何拦截规则,loopback / 私网 / 云元数据全部可绕过(端到端复现:本地服务器真实收到请求)。已修:换算回点分 IPv4 再判;公网映射形式(::ffff:808:808 = 8.8.8.8)仍放行。fetch-policy 补经 new URL() 规范化路径的负例。
    • 放映页 iframe 强制相对解析:slide 文件名含 :(如 javascript:alert(1).html,Linux/macOS 合法)时 iframe.src 会被当 URL scheme 在放映页同源执行(Chromium 实测);现一律 src = './' + …
    • 输入上限 2MB:构造的超大 tokens.css / slide(4MB 实测让检查跑 39s 的二次方放大)现在直接拒绝扫描。
  • 流程:
    • 竖版(1080×1920)放映页修复:舞台尺寸 / 缩放 / 缩略图比例原先写死 1920×1080,竖版 Gate 4 画面被切掉近半;现按 script.json 画布参数化,authoring.md 补竖版字幕安全区数值。
    • still 复截静默吞动画:capture 在 still 路径作废该张帧目录(防旧帧污染)后,build-video 回退静态图出片此前零提示;现点名 ⚠ 并给出补跑命令(capture --mode motion --ids <id>),文档同步"still 复看后必须对同 ids 重跑 motion"。
    • 工作流补点:新增"恢复老项目先跑 --check-css"(各 Phase 判定标志 + 升级后重跑矩阵)、Phase 4 补条件项 check-theme、Phase 6 交付清单补 out/subs.srtbuild/timings.json
  • 受管块补强:CRLF 行尾比较归一(git autocrlf 检出的文件此前误报"被手工改过")、升级动作分两阶段(病态布局一次到位)、.bak 预存时显式警告。
  • 文档对齐:插件包顶层 THIRD-PARTY-NOTICES 与技能内副本同步、README 验证段改为两种仓库语境都成立的命令、authoring 纪律条数/顺序、evals 4 处过期内容、SKILL 脚本表补列实存开关。

4. 调研链路的环境归属(v1.3.1)

工具链对照表"调研"行补齐:mcode = web_search / web_fetch + 内置浏览器取 SPA 正文;其他环境 = mmx search + Playwright 无头浏览器。并明确工具开放、纪律不变:本机装了其他搜索技能 / 插件 / 无头浏览器均可用来取正文,来源分级、两源交叉、口径标注的纪律不因工具而松。

5. 工作流:开工对齐新增"数据与图表"必问项(v1.3.2)

图表工具箱合入后,入口此前只藏在 Phase 1/4 的文档里,开工对齐必问清单没有图表项。现新增一行"数据与图表":题材含关键数字 / 对比 / 占比 / 趋势时必问 —— ①有没有值得单独成张的可视化数据;②要不要图表张、大约几张(默认建议:有硬数据且版式合适 → 1–2 张);③样式偏好(横向条形 / 柱状 / 环形 / 折线 / 进度条,或按数据形态让 agent 选);④图上数字同受 Gate 0 约束(≥2 独立来源、带口径与时点)。就算用户没提图表,数据密集题材也要主动给这个选项。Phase 1 配套"数据密集的题材主动做图"(≥2 个可比数字 → 优先 data-viz + 图表工具箱,不罗成 bullets)。

6. Gate 1 呈现格式补"表格与动效"必填列(v1.3.3)

实测反馈:Gate 1 给用户过的表此前只有「版式 / 画面 / 口播」,用到表格原语或有入场动效(淡入错峰、数字滚动、条形生长)的张没有说明,用户只能凭空想象、等 Gate 4 才第一次看到动起来的样子。现 Gate 1 呈现格式定为「# / 版式 / 画面 / 表格与动效 / 口播逐字稿」,表格与动效列必填:表格张说明形态(几行几列、用哪个原语、高亮哪行/列),动效张点名效果与次序 —— 让用户凭这张表就能在脑内放一遍成片。

7. 配图 SOP 新增"路径 D · 公告与报道截图"(v1.3.4)

暴雷 / 监管处罚 / 诉讼 / 翻车类题材,公司官网不可能提供素材(路径 A/B 必然落空),此前 SOP 没覆盖这条真实路径。新增路径 D:素材取自事件本身的一手载体——交易所公告 / 监管文书 > 公司官方声明 > 媒体报道页;一线权威(新华社/央视/澎湃)与主流门户(腾讯新闻/新浪/网易等)均为合格来源,排除的只有自媒体 / 聚合号。截图纪律:画面保留媒体名与日期、只截正文区、MANIFEST 登记"来源 URL / 日期 / 报道性合理引用"、入画 .img-frame.contain + .img-cap 注明来源;附 Playwright 截图最小命令。SKILL.md Phase 3 优先级链与合规自查同步。

8. 铁律 4 措辞收紧:开工对齐必须"问、等答"(v1.3.5)

实测踩坑:agent 把必问清单执行成"一条消息甩出『默认项(不特别说的话就按这个):中文普通话 · 中文字幕 · 1920×1080 · 温润男声 · 10–12 张…』然后继续推进"—— 用户没有逐项表态的机会,等于没对齐;根因是旧措辞"(用户没答的项可用默认值,但必须先问)"留有"先告知、不反对即同意"的解读空间。现规则:第一次响应 = 问题以问句形式逐项列出、然后停下等回答;每项可附推荐默认值,但必须是"问、等答";只有用户明确说"按默认"或没答到的项才用默认,且进入下一步前复述最终采用了哪些默认。实测反例原文写进 SKILL.md 作为禁例。

9. 开工对齐"素材边界"补负面事件加问项(v1.3.6)

实测踩坑:负面新闻稿视频,agent 问了主题 / 图表 / 受众,但没问"要不要新闻或媒体平台截图当配图"—— 路径 D(v1.3.4)写进了取图 SOP,却没接进开工必问清单。现"素材边界"行加注:负面事件题材(暴雷 / 处罚 / 诉讼 / 争议)必须加问"要不要用公告或新闻报道截图当配图"(默认建议:要,1–2 张,来源分级与截图纪律见 image-sources.md 路径 D)。

10. 开工对齐清单重排 + Gate 3 补"选素材 / 补充素材"问(v1.3.7)

  • 开工必问清单按实测反馈重排为 11 项:新增"受众"(专业从业者/泛科技大众/管理层汇报/客户演示/内部培训,决定术语密度与口吻 —— 此前只藏在"主题与领域"里没单列);新增"敏感内容与免责"(免责追问从仅受监管领域扩展到财经观点/医疗/法律/政策/负面事件报道/个人信息任一命中,默认要);"素材边界"改为"配图与素材边界"(先问要不要配图,负面事件加问公告/新闻截图);"音色"标注大致方向即可。
  • Gate 3 改为三问(素材收集走完、事实与候选图到手之后):① 逐张挑"用哪些";② "有没有要补充的素材"——链接/截图/logo/图片/数据文件/参考口播稿都可以交给 agent,用户提供的素材优先级最高(图片过 prep-image --check、MANIFEST 登记"来源:用户提供",链接按路径 A/B/D 落盘);③ 来源与授权确认。此前 Gate 3 只有整体确认,用户没有逐张挑选与补充素材的入口。

11. 放映页交互模型重做:动效开 = 手动逐级入场 + 三项实测反馈修复(v1.4.0)

  • 逐级入场:实测反馈"动效开时 ←/→ 只能切整张,二级小标题/小图表只能干等它们到点自己出现,不符合体感"。现动效开时 = 先出下一级(下一级当场动画入场,出完才翻下一张; 逐级回退,回退不重播动画直接终态);动效关仍是整张切换。实现:快照副本 <head> 注入一段在 CSS 动画启动前执行的脚本,按 ?s=k 重设各级 --tN 延迟(已过级 -60s 终态 / 当前级 0ms 当场入场 / 未来级 +60s 保持隐藏等按键);父页只换 iframe src,与快照同一套机制、无跨文档访问(file:// 下 iframe 为独立源)。顶栏页码显示当前级(如 1 / 8 · 级 2/3)。重播按钮与 R 键移除(逐级步进下重播没有意义);触屏条前后箭头加大(76px 宽 / 24px 字号)。
  • 换帧不闪白(实测反馈:切大页白屏一闪):改为双缓冲——两张叠放 iframe,新地址先喂给隐藏帧,load 后再等两帧 rAF(新文档首帧已画)才对调显示,旧画面始终在屏;逐级换帧同理受益。
  • UI 双语(实测反馈:英文项目整套中文按钮):按钮 / 提示 / 面板标题 / 占位文案随 script.jsonlang 中英自适应(en* → Motion on / Narration on / Overview…),终端输出仍为中文(agent 视角)。
  • 开关移到底栏且写明状态(实测反馈:"动效/口播"看不出现在是开还是关,顶部开关多余):底栏按钮直接写"动效开/动效关""口播开/口播关"(关态亮警示色),桌面也常显,顶部动效开关移除。

12. 第二轮评审整改:输出收监 / 网络边界 / 字幕时间轴(v1.5.0,3 个 P1 + 4 个 P2)

逐条对应评审意见,全部有可执行证据:

  • [P1] 输出收监 —— 输入路径收监不能支撑"写入都在项目内"的承诺:输出位置(preview/build/frames/<id>/)是项目内固定位置 + 受监 id,但项目内的目录段本身可能是指向项目外的符号链接,递归删除会穿透它而命令报成功。新增 tools.assertContained / safeOut(逐段已存在祖先的 realpath 必须仍在项目 realpath 内),接到 capture(帧目录递归删除、preview 写入)、build-video、preview-page、plan-timings、grab-frames 的全部写删点。canary 回归测试(Windows 用目录 junction 免管理员建链,不 skip)断言"必须拒绝 + canary 完好"。
  • [P1] 网络边界落在真实请求点 —— 原来只校验最初输入的 URL。现 url-policy.assertResolvedHost每个真实请求做解析后再验 IP:页面 URL、每一跳手动重定向、以及浏览器侧的全部请求(导航、Chromium 自己跟随的 30x、子资源)经 context.route 拦截 + 域名解析缓存。端到端测试:本地服务器在直下与浏览器两条路径上都必须收到 0 个请求,并用可注入的假解析器覆盖 rebinding(无需真 DNS)。
  • [P1] 字幕时间轴 —— 复现了报告里的现象(字幕是全时长百分比动画,逐帧只覆盖动画窗,其余靠 tpad 冻尾帧 → 之后的字幕变化永远进不了画面;no-fx/静态路径 finish() 把字幕停在 opacity:0,整片没有字幕)。现 capture 逐句输出"终态基底 + 该句字幕"静态图 + 清单(framesCover 与每段窗口),build-video 用 concat 拼成"帧序列段 + 各字幕段"(段长之和由既有自检背书)。回归用逐像素判据:第二句窗内的画面必须贴合第二句字幕图(实测 0.93)、远离第一句(7.0);旧行为下两个数正好反过来(0.99 / 7.1),测试会红 —— 已用"删掉清单重建"验证过。
  • [P2] ASR 请求错误计入失败:鉴权/限流/网络失败此前不进统计,1 通过 / 0 不通过 / 退出 0 把闸门放过去了;现单独计数、不计入通过、非零退出并点名段落(用本地 503 mock 验证)。
  • [P2] 放映页属性解析:单引号/无引号属性此前不被识别,会再插一组重复属性而浏览器保留先出现的那组 → 实测延迟/画布尺寸/no-fx 全部失效;现就地改写、不产生重复属性。
  • [P2] ffmpeg 发现补 cwd 与项目目录:README 明说支持把 ffmpeg-static 装在视频项目里,但工具发现不查 process.cwd()prep-image/asr 不传项目目录;现两处都传,并有源码级防回退断言。
  • [P2] --out-dir 规范化比较:macOS 上 /var/private/var 是同一目录,字符串比较会把项目内合法路径误判越界(官方 CI 的 macOS 环境实测踩到);现取最深已存在祖先的 realpath 再比。

13. 交付质量三修:封面 / 切页 / 语速与试听对齐(v1.6.0)

来自真实交付的反馈(转发出去的缩略图是纯黑首帧、每次切页黑屏、默认语速偏慢):

  • 封面与首帧:此前每段都带 fade=t=in,成片第 0 帧全黑。现 capture 为第 1 张另出 preview/cover.png(基础动画终态、字幕隐藏),首段用它做 0.25s 溶解(帧 0 就是完整封面,不加时长),mux 时以 attached_pic 内嵌(主视频仍 -c:v copy),并导出 out/cover.png 供平台上传。
  • 切页不经过黑场:默认硬切(只在末段保留结尾淡出),另提供交叉溶解(script.jsontransition--transition xfade;每段先多留尾帧、叠化吃掉,总时长不变)。放映页按 T 现场对比两种切页。
  • 语速与试听:默认 speed 1.0 → 1.1(≈中文 5.3 字/秒);试听环节现在一次问两件事 —— 3 个候选音色 + 首选音色的三档语速(1.05/1.15/1.25),用户定完写进 script.json;且 plan-timings 开始读取 script.speed(此前是纯声明字段),实测语速偏离 基准×speed 超 20% 或 speed 越界即告警。
  • Gate 4 加预览模拟与两个新问项:①切页关系 ②大标题与子信息一次性出现还是逐级揭示;check-slides 末尾打印当前转场值与每张级数,拿实测数字去问。
  • 三道画面闸门:封面已内嵌 / 首帧亮度高于黑电平 / 每个切页点前后无黑帧(不过即非零退出)。
  • 文档改为英文:SKILL.md、6 个 references、CHANGELOG、THIRD-PARTY-NOTICES 全文英文(README.zh-CN.md 保留中文),并同时做素材源国际化 —— 一手文书/官方声明/主流媒体三档各给国际(Reuters/AP/Bloomberg/FT/BBC/CNBC)与中文(新华社/央视/澎湃/财新/腾讯新闻)两组对等例子,规则写成"按一手性与编辑责任分级,不按国家";受监管题材改为按辖区命名监管机构(SEC/FTC/FDA、FCA/ESMA/EMA、CSRC/NMPA/SAMR)与按市场约定涨跌色。

验证

  • 测试 93 → 174 例(node:test,零依赖,新增 css-kit 文件 + 各文件补例),dev 树与发布树本地全绿(发布树 ffmpeg 用例以本机 ffmpeg 补跑,0 skip);validate.mjs OK。
  • workflow 清单已加 css-kit.test.mjs;因改过 workflow 文件,CI runs 处于 action_required(需维护者批准后执行,属预期)。
  • PR 范围不变:47 个文件 = 插件前缀 46 + workflow 1,无越界;上游根文件未动。

对应提交(插件分支):acdfefd7 / 74551814(放映页初版与定位收窄)→ 4089ba5c(图表/表格工具箱)→ b66404d5 + b4fc4341(受管块 + workflow)→ 6e2fcae4(三维审计修复)→ 0fe6a11d(环境归属)→ 3f1a0c2c(数据与图表必问项)→ 17385f32(Gate 1 表格与动效列)→ 105f7976(路径 D 公告与报道截图)→ 619f8607(开工对齐必须问并等答)→ d49b876a(素材边界加问负面事件截图)→ 33f6a1ef(开工对齐重排 + Gate 3 三问)→ fcf36309(放映页逐级步进 + 双缓冲 + 双语,1.4.0)→ 733d7a8a(二审整改:输出收监/网络边界/字幕时间轴,1.5.0)→ 349b14be(封面/切页/语速与试听对齐 + 英文文档,1.6.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.

Re-reviewed current head fcf363095d6062441f6df1dd6a988a1493213f54 (v1.4.0). Request changes: the added validation and executable tests address parts of the previous review, but the following issues remain reproducible on this exact head.

All script paths below are relative to plugins/Wzdhehe/html2video-for-mcode/skills/html2video-for-mcode/scripts/.

Security boundaries

The two findings below are intentionally summarized without exploit inputs or reproduction steps, consistent with this repository's SECURITY.md.

  1. [P1] Output containment is still incomplete. The capture pipeline's output writes and recursive frame cleanup do not consistently enforce the real filesystem boundary. A controlled test using disposable canary data demonstrated deletion outside the supplied project while the command reported success. Validate the canonical project boundary and all output-path ancestors before destructive operations, including derived output paths; add regression coverage for output-directory indirection. The current safe-ID/input-path checks are insufficient to substantiate the README's containment guarantee.

  2. [P1] The image-fetch network policy does not cover all actual requests. Controlled local-server tests demonstrated access to a disallowed destination through both the direct-download and browser paths. Enforce destination validation at the actual connection/request boundary, covering name resolution, navigation redirects, and browser subresources. Pure URL-classification tests alone do not verify this boundary; add end-to-end tests asserting that rejected destinations receive no requests.

Functional correctness

  1. [P1] Keep subtitle changes in the rendering window. capture.mjs:221-235 excludes .kit-sub animations when calculating animEnd, then stops frame capture after the visual entrance animations finish. build-video.mjs pads the rest with the final captured frame, freezing later subtitles. In a real plan-timings → capture --mode motion → build-video run, an 8-second MP4 passed duration and full-decode checks, but its SRT scheduled the fourth clause at 5.322 seconds while the frame at 6 seconds still displayed the second clause. With no-fx, capture falls back to a static frame at subtitle time zero, producing no burned-in subtitles. This violates the documented promise that subtitles continue normally when effects are disabled. Include subtitle changes in the capture duration, or burn them independently during encoding; test both multiple clauses sharing a stage and no-fx.

  2. [P2] Count ASR request errors as failures and exit nonzero. asr.mjs:230-239 catches request errors into { error }, but the final summary only counts failed text verdicts. A local mock returning HTTP 503 produced an error followed by 完成: 1 通过 / 0 待复核 / 0 不通过, with exit code 0. Authentication, rate-limit, and network failures can therefore falsely pass the ASR gate. Track processing errors separately, exclude them from the pass count, and return nonzero when any required transcription failed.

  3. [P2] Handle existing single-quoted HTML attributes in the preview generator. preview-page.mjs:63-73,86-92 recognizes only double-quoted style and class attributes. Valid input such as <html class='theme' style='--t2:800ms'> receives duplicate attributes; Chromium retains the first ones. Real-browser verification showed that the original delay remained, the injected canvas width was absent, and no-fx was not applied. Parse/update existing attributes without producing duplicates, with coverage for single-quoted and unquoted forms, so preview timing, portrait dimensions, and the effects toggle match rendering.

  4. [P2] Complete discovery of project-local ffmpeg dependencies. tools.mjs:20-21 does not search process.cwd(), while prep-image.mjs:23-24 and asr.mjs:65-66 do not supply a project directory or set KIT_PROJECT_DIR. The README explicitly supports ffmpeg-static / ffprobe-static installed in the video project. With system PATH isolated and a tool present under the project's node_modules, running the image-processing entry point from that project still exits 2 with “找不到 ffmpeg”. Include the invocation/project directory consistently in tool discovery and cover these entry points without system ffmpeg.

  5. [P2] Canonicalize output directories before the containment comparison on macOS. fetch-official-images.mjs:57-60 compares lexical paths, so the equivalent /var/... and /private/var/... spellings can be treated as different directories. A legitimate absolute --out-dir inside the working project is rejected. This also causes the plugin's existing no-clobber test to fail on the tested macOS environment. Compare canonical paths, resolving existing ancestors when the output does not exist, while preserving the security boundary.

Validation

  • Reviewed and reproduced against the exact head above; source files were not modified.
  • macOS, Node 26.4.0, local ffmpeg/ffprobe and real Playwright Chromium.
  • Repository validation passed all 27 hosted plugins and examples.
  • Full npm run check: 640 tests, 616 passed, 1 failed, 23 skipped. The failure is the macOS path-alias issue in item 7.
  • Plugin-only suite: 150/151 passed with the default temporary directory; 151/151 passed, zero skips, after using a canonical temporary-directory path. Both existing render smoke tests passed, but they do not detect the subtitle failure above.
  • ASR error handling used a local HTTP mock and a fake credential; no real ASR service was called. Windows and Linux were not exercised locally.
  • At review time, GitHub CI, scoped smoke, and CodeQL runs were action_required; the visible [code]smith check was skipped. These are not passing CI evidence.

@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

Re-reviewed all seven items on the new head 733d7a8a (v1.5.0) — every one is verified fixed with executable evidence. Thanks for the precise reports; three of them reproduced exactly as you described.

Note on the head you reviewed: you tested fcf36309 (v1.4.0). Items 2 and 7 were already partially addressed in the unreleased work below that head, and items 1/3/4/5/6 are new fixes. All of it is now on the branch and in the CHANGELOG under 1.5.0.

[P1] 1. Output containment — fixed (tools.mjs assertContained/safeOut)

You were right that the input-side safeId/safeRel checks did not substantiate the containment guarantee: the output paths (preview/, build/frames/<id>/) are fixed positions inside the project plus a validated id, but a directory segment inside the project can itself be a symlink pointing outsidefs.rmSync(recursive) then deletes through it while the command reports success. That is the canary scenario you demonstrated.

Fix: new assertContained(root, abs) resolves the deepest existing ancestor of the target and requires its realpath to remain inside the project's realpath; safeOut(root, ...segs) is the safe path builder. Wired into every write/delete site: capture.mjs (frame-dir recursive delete, preview writes), build-video.mjs (out/build/asr writes and cleanup, segment paths), preview-page.mjs, plan-timings.mjs, grab-frames.mjs.

Regression coverage (tests/review-round2.test.mjs): a canary directory outside the project is linked into build/frames and into preview, then the pipeline must exit non-zero and the canary must be byte-identical afterwards. On Windows the test creates the link via a directory junction (mklink /J, no admin needed) so it actually runs instead of skipping — 0 skips on this machine.

[P1] 2. Network policy at the actual request boundary — fixed

Confirmed: the previous checks covered only the initially-supplied URL. Now every real request passes url-policy.assertResolvedHost — DNS resolution followed by the blocked-address check — at: the page URL, each manual redirect hop of a download, and every browser request (navigation, Chromium's own 30x redirects, subresources) via context.route with a per-hostname resolution cache.

End-to-end evidence you asked for: tests/review-round2.test.mjs starts a local HTTP server, points the fetcher at it in both the direct-download and the browser paths, and asserts the server received zero requests while the command exits non-zero. Rebinding itself is covered by an injectable resolver (assertResolvedHost(url, { lookup })): a public-looking hostname resolving to 127.0.0.1 / 169.254.169.254 is rejected and no connection is attempted.

[P1] 3. Subtitle changes inside the rendering window — fixed (your item 3 reproduced exactly)

Reproduced: subtitles are full-duration percentage keyframes, while capture --mode motion excluded .kit-sub from the animation-window calculation and build-video padded the remainder with the last frame via tpad. So every subtitle change after the animation window was frozen — your 5.32 s SRT clause vs the 6 s frame is exactly this. The no-fx case was worse: finish() jumps the subtitle animation to opacity: 0, so no subtitle was burned at all.

Fix, two parts:

  • capture.mjs now emits, per clause, a subtitle still — base animations finished, .kit-sub hidden, then only that clause's subtitle seeked to the middle of the span the frame sequence cannot cover (build/substills/<id>/s<k>.png), plus a manifest (build/substills/<id>.json) recording framesCover and each span.
  • build-video.mjs composes the slide from parts: [frame sequence] + [one still per uncovered span], concatenated in one filter_complex pass, with the last part padded so the sum equals the slide duration (the existing duration self-check backs this).

Regression test asserts the actual pixels: extract the frame at 5.0 s, crop the subtitle band, and require it to match that clause's still (measured 0.93) and to be far from the other clause's still (7.0). On the old behaviour those two numbers invert (0.99 / 7.1), so the test fails without the fix — verified by rebuilding with the manifest removed. Both paths are covered (motion and no-fx/static), and multiple clauses sharing a stage are handled (spans that the frames already cover are skipped).

[P2] 4. ASR request errors — fixed

Request failures are now counted separately, excluded from the pass count, and the run exits non-zero naming the affected parts: 完成: N 通过 / N 待复核 / N 不通过 / N 请求失败. Test drives a local mock returning 503 and asserts non-zero exit plus the "请求失败" line.

[P2] 5. Single-quoted HTML attributes — fixed

Attribute handling now matches double-quoted, single-quoted and unquoted forms, and rewrites the existing attribute in place instead of appending a duplicate. Tests cover <html class='theme-a' style='--t2:800ms'> (exactly one style/class attribute afterwards, value appended to the original) and unquoted <html class=deck style=color:red>, including the idempotent no-fx case.

[P2] 6. ffmpeg discovered from the project — fixed

candidatePaths() now includes process.cwd() and the caller-supplied project directory, and prep-image.mjs / asr.mjs pass their project directory through. Two tests: a behavioural one (create node_modules/ffmpeg-static in a project, chdir into it, findTool('ffmpeg') must find it with KIT_PROJECT_DIR unset) and a source-level assertion that both entry points keep passing the project dir.

[P2] 7. Canonical path comparison on macOS — fixed

--out-dir containment now compares canonical paths (realpath of the deepest existing ancestor, then re-append the non-existing tail), so /var/... and /private/var/... are recognised as the same directory while the boundary is preserved. Your macOS observation (the plugin's own no-clobber test failing there) was the tell — thanks.

Validation on this machine

  • Full suite in the dev tree and in both publish trees: 171 tests / 171 pass / 0 fail / 0 skipped (ffmpeg and Chromium on PATH).
  • Repository validator: OK plugin Wzdhehe/html2video-for-mcode (26 hosted plugins validated).
  • PR scope unchanged: 46 files = plugin prefix + the scoped workflow; no upstream root files touched. CI runs remain action_required (workflow file changed earlier in this PR, needs maintainer approval).

If it is useful, the two security tests are the ones I would look at first — they are negative tests with disposable canary data and a local server, so they double as a repro harness for anything you want to try yourself.

@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

Third review round — every finding fixed, plus a review of the fix itself (1.7.0, tests 174 → 235)

Both review rounds are now closed item by item. Below: what was wrong, what changed, and the command
that demonstrates it. Sections 1–5 are the review findings; section 6 is a standards/spec review of
this change set, which found a further batch (including two false claims in our own docs);
section 7 is what was deliberately left alone.

1. CSS propagation did not work, and it failed silently in six different ways

The skill writes generated CSS into the project's slides/tokens.css. The old mechanism delimited
only the three toolkit blocks (no-fx / charts / tables) and decided "up to date" by looking at the one
block it recognised, so the project could keep rendering old rules while every command printed ok:

# Silent path Why it was invisible
the same toolkit present twice the later copy wins the cascade; only the first was examined
a stale copy outside the managed blocks never scanned at all
a mangled closing delimiter the regex stopped matching, and the probe read "no block" as "fine"; the upgrade then emitted a second opening marker
drift in the generated body itself (theme tokens, --fs-*, fx keyframes, .fx-stagger) it was not managed, so it was never compared
a current region plus a stale copy of the generated body outside it the outside scan only knew toolkit selectors (found by re-reviewing the new mechanism)
an override whose position was reported as "none" the note did not say whether the outside rule sits before the region (loses the cascade) or after it (wins) (same re-review)

Fix. The whole generated body is now one tokens managed region with the three toolkit blocks
nested inside; the status vocabulary is explicit (ok / stale / duplicate / broken /
legacy-outside / missing). --upgrade-css replaces the region in place, merges duplicates, removes
orphan markers, and cleans leftover copies outside the region rule by rule — verbatim module rules
are deleted, a project override next to a copy survives, and the original file is kept in
tokens.css.bak. Put your own rules after the region (documented in SKILL.md and both READMEs).

# ⑤/⑥ and the whole vocabulary, one run:
node scripts/init-project.mjs <project> --check-css          # every non-ok item listed, exit 1
node scripts/init-project.mjs <project> --upgrade-css        # in-place, idempotent, .bak written
node --test "tests/*.test.mjs"                               # css-kit: 29 cases incl. ⑤/⑥

Ordering bug found while fixing (worth flagging, it was mine). The leftover-copy cleanup ran
before the region step, which deleted the toolkit text out of a legacy "bare text" body — the
in-place match then missed, so the upgrade both duplicated the body and left the old rules
winning the cascade. Fixed by reordering and appending the region at the tail when there is no
anchor; the regression test asserts the region lands after legacy content, and a pixel-level test
proves the render actually changes (a stale frame measures darker; after the upgrade it returns to the
current picture within 0.05 luma).

2. Stale CSS is no longer silent — it blocks

check-slides (the static gate) treats a stale region as an error and exits 1; capture and
build-video refuse to run and print the exact fix command. --allow-stale-css skips the gate
explicitly at the two rendering entry points; check-slides deliberately has no such flag (it is
the check you run before rendering). The play page's fallback injection now says plainly that a correct
preview page does not mean the rendered video is current.

node scripts/capture.mjs <project>                # exit 1 + issue list + the upgrade command
node scripts/capture.mjs <project> --allow-stale-css   # explicit opt-out, warns and continues

3. Wrong artefacts (highest priority)

  • SSRF policy completed (scripts/url-policy.mjs): blocked ranges now cover 0/8, 10/8,
    100.64/10, 127/8, 169.254/16, 172.16/12, 192.0.0/24, 192.0.2/24, 192.168/16,
    198.18/15, 198.51.100/24, 203.0.113/24, 224/4, 240/4, plus fec0::/10 and
    2001:db8::/32; a trailing-dot FQDN (localhost.) is normalised first; embedded IPv4 inside
    ::ffff:, NAT64 64:ff9b::/96 and 6to4 2002::/16 is decoded recursively. Every case has a
    negative test; public hosts still pass.
  • Subtitle stills are invalidated with the subtitles: capture deletes build/substills/<id>/
    and its manifest before rewriting them, and build-video validates the manifest against the current
    slide (duration, stills[].k < clauses.length, framesCover) and ignores it with a named warning
    otherwise. Before this, deleting clauses (or --no-subs) and rebuilding spliced the old
    subtitles back into the video.
    node --test tests/subtitles-invalidate.test.mjs   # pixel-level: old subtitle must not reappear
  • Path jail hardened (scripts/tools.mjs): ancestors are walked with lstat (a dangling
    symlink is now caught instead of being treated as "does not exist"), and an unresolvable realpath
    fails closed instead of returning the absolute path.
  • Output containment at every write site, leaf files included — the second class of escape, where
    a directory inside the project is itself a link pointing outside. Covered now: plan-timings,
    check-timing, build-video (concat.txt, audio-timeline.wav, audio-mix.wav, final.mp4,
    asr/checklist.md), asr, capture, fetch-official-images (output directory included, not
    just the files), and init-project — where the skeleton writes (8 directories + 5 files) were
    the last uncovered spot. Verified by a direct unit call plus junction-based integration tests:
    node --test tests/safe-paths.test.mjs
    (On Windows the link is made with mklink /J, so these tests never skip for lack of privileges.)

4. Six functional defects — each one a documented way to get a wrong file

  • check-timing --calibrate could write back a mixture of exact-punctuation and
    nearest-neighbour boundaries while reporting method: "nearest". The two passes now allocate
    separately, nearest is reported only when every boundary was measured, and a run that
    calibrated nothing does not rewrite the file at all.
  • init-project.mjs --topic 主题 <project> treated the topic's value as the project directory and
    silently created a skeleton named after the topic in the current directory. All positional-argument
    scripts now share tools.positionalDir, and VALUE_FLAGS lists only genuinely value-taking flags
    (a boolean flag listed there swallows the next positional — capture --allow-stale-css <project>
    used to operate on the current directory).
  • fetch-official-images: --json was listed as value-taking, so --json <page URL> swallowed the
    URL (position-independent now); --min accepts WxH; a --get run where every download failed now
    exits 1.
  • Entry-point validation: capture --mode only still|motion, --dsf only 1–4, grab-frames --at
    only (0,1]; grab-frames exits 1 on zero frames and 2 when ffmpeg is missing.
  • The subtitle-band gate matched padding-bottom/margin-bottom and flagged elements that are not
    positioned at all; it now requires position: absolute|fixed in the same rule.
  • Slide transitions are parsed in exactly one place (tools.readTransition). Before, check-slides
    accepted the bare string "xfade" while the renderer only read object.type, so the gate could
    report a cross-dissolve that the video rendered as a hard cut.

5. Packaging and the validation entry point (a green light that proved nothing)

The host validator scans _official-plugins/plugins/**, and the plugin is staged there by hand. When
the staged copy was one release behind, the validator reported OK for the previous version — which
is exactly how the 1024-character description limit was missed twice. New publish/validate-plugin.mjs
stages the tree, fingerprints every file (sha256) and refuses to continue if the staged copy
differs from the source tree, then runs the upstream validator. The description is now 977
characters, with the symptom list extracted to references/symptoms.md and linked from SKILL.md.

node publish/validate-plugin.mjs
# 2/3 指纹一致: 52 个文件逐字节相同(sha256)
# 3/3 运行上游校验器 … OK   plugin Wzdhehe/html2video-for-mcode

The scoped workflow now runs node --test "tests/*.test.mjs" (a glob — enumerating files had already
missed eight of them), carries permissions: contents: read, and pins both actions to full SHAs with
version comments.

6. A standards/spec review of this change set — what it caught

This diff was then reviewed against its own spec and the repo's documented conventions. The real
findings are all fixed here:

  1. build-video discovered its tools before validating the configuration. On a machine without
    ffmpeg, a bad width/fps/bgm was reported as "ffmpeg not found" (exit 2) and the real error was
    swallowed. Tool discovery now happens after every config check.
  2. requireFreshCss was the one entry point reading tokens.css without the 2 MB scan cap the
    other three enforce; it now refuses an oversized file on the same rule.
  3. init-project's skeleton writes were not contained (covered in §3) and
    fetch-official-images created its output directory unchecked — both fixed, both with tests.
  4. readTransition did not name the source it rejected: --transition bogus was reported as
    script.transition being invalid.
  5. Two generateTokensCss() re-assemblies collapsed into one tools.expectedTokens() shared by
    check-slides and the play page; the dead needsUpgrade export was removed.
  6. Doc-vs-code drift, five places — the READMEs still promised "three rev-stamped managed blocks …
    rules outside the blocks are untouched" (the pre-1.7.0 contract, and the opposite of what the
    leftover cleanup does); the README module list named six of eight modules and kept the old
    mechanism's name; the init-project and check-slides headers still described their pre-1.7.0
    behaviour; capture's usage line said --dsf 1|2 and omitted two flags.
  7. A false claim in our own README: "where those are missing they skip with a stated reason."
    Without ffmpeg the rendering suites fail, not skip — confirmed by reading the execution order
    (requireTool ran before validation) rather than by trusting a measurement made in an environment
    where ffmpeg was discoverable from node_modules. The READMEs now say what actually happens.

7. Evidence

$ node --test "tests/*.test.mjs"      # dev tree
ℹ tests 235   ℹ pass 235   ℹ fail 0   ℹ skipped 0        # 14 files

# same suite in both published trees: 235 / 235 / 0 fail / 0 skip
$ node publish/validate-plugin.mjs
OK   plugin Wzdhehe/html2video-for-mcode               # 52 files fingerprint-identical, exit 0

The suite needs ffmpeg, ffprobe and Chromium (any of PATH, the repo's or the project's
node_modules); the CI workflow installs them, and both published trees were run with them present.

Deliberately not changed (disclosed rather than hidden):

  • A stale body-shaped leftover outside the region that is small (< 400 chars and < 3 rules) is
    reported as your own override with a position hint instead of being reported as a leftover.
    Tightening that threshold risks deleting real project rules; the safe direction is "report, never
    delete".
  • The guard test for VALUE_FLAGS scans argv.includes('--x'), so re-adding a boolean flag to that
    list would not be caught automatically (not a live bug — it was verified manually).
  • The workflow uses a shell glob rather than directory discovery (same intent: no file enumeration).
  • Tests hard-code the region delimiters on purpose: they pin the on-disk format.
  • The host repository's own actions/checkout pin in ci.yml is one hex character too long, which is
    why it could not be copied verbatim; reported, not touched (it is outside this PR's scope).

@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

Fourth review pass (standards + spec axes) over the published diff — four real findings, all fixed in 1.7.1 (9ab1dfb2 + b945b51b on this branch, skill repo 1f0d0d63).

1. The corrected smoke workflow had never been pushed. The 1.7.0 entry described a workflow that runs node --test tests/*.test.mjs via a shell glob, sets permissions: contents: read and pins both actions to full SHAs — but this branch still carried the old version: explicit file enumeration (so cover-transition, review-round2, review-round3 and subtitles-invalidate never ran in any CI), no permissions: block, and floating @v4 tags. The rewrite had only ever been applied to the local file. The corrected workflow is what this branch carries now (b945b51b), and its header comment records exactly this failure mode so it doesn't happen silently again.

2. This Plugin would have reddened the monorepo's own CI. npm run check runs a root-level node --test, which auto-discovers plugins/**/tests/*.test.mjs — on runners that have neither ffmpeg nor Playwright. Exactly one case lacked the tool guard every other rendering case has: the ASR-503 case (asr.mjs exits with 找不到 ffmpeg at startup, before any request, so the assertion on "请求失败" never got its chance). It now skips with its stated reason like the rest. Verified in a clean sandbox with neither tool resolvable: 236 tests, 0 fail, 22 skipped with stated reasons — i.e. npm run check stays green on a bare runner.

3. The 1.7.0 README sentence "the rendering suites fail rather than skip" was itself wrong — it rested on an observation made where ffmpeg-static was silently discoverable through a workspace-level node_modules, so nothing ever actually skipped or failed. Both READMEs now state the measured behaviour: tests needing a tool skip with their stated reason; the pipeline scripts themselves exit with 找不到 ffmpeg instead of running half a pipeline.

4. Plan B2's third manifest check was claimed but not implemented. The 1.7.0 comment said build-video validates the subtitle manifest's duration, stills[].k < clauses.length and framesCover — the code checked only the first two. framesCover is now compared against the current frame sequence's real duration, so a re-captured animation window (frame count changed, slide duration unchanged) can no longer splice subtitle stills at stale offsets. New pixel-level case in subtitles-invalidate: a stale manifest is ignored with a named warning — measured luma difference against the stale still: 45 (it is not in the picture); the same still under an aligned manifest splices in with difference 0.

Smaller items from the same pass: VALUE_FLAGS no longer lists --speed / --voice (nothing parses them; a stray --speed silently swallowed the next positional), the six per-script flag() helpers collapsed into the single flagValue in tools.mjs, and the internal KIT_PROJECT_DIR variable is documented in both READMEs.

Evidence

dev tree + both published trees:  node --test tests/*.test.mjs
  → tests 236 / pass 236 / fail 0 / skipped 0   (14 files, each tree)

clean sandbox (no ffmpeg, no ffprobe, no Playwright resolvable):
  → tests 236 / pass 214 / fail 0 / skipped 22  (every skip names its reason)

node publish/validate-plugin.mjs
  → 52 files fingerprint-identical (sha256) · upstream validator: OK plugin Wzdhehe/html2video-for-mcode

Remote bytes re-verified after the pushes: skill repo main 50/50 files identical, this branch 53/53 (52 plugin files + the workflow). Test counts in the READMEs / CHANGELOG now read 236 in fourteen files everywhere they appear.

@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

Thank you for both reviews. They targeted 737ee96b (v1.0.1, review 1) and fcf36309 (v1.4.0, review 2, submitted 13:13Z). The current head is b945b51b (v1.7.1) — every finding below was fixed on this branch after those heads, each with a regression test, and all fixes were re-verified today against the current head. Per SECURITY.md, this reply names tests rather than inputs.

Review 1 (v1.0.1) Review 2 (v1.4.0) Fixed in Verification on current head
1 input-derived IDs/paths escape 1 output containment incomplete v1.1.0 f015a8b8 (strict ID regex + resolve-and-containment + symlink checks), v1.5.0 733d7a8a ① (assertContained/safeOut at every write/delete site, junction canary), v1.7.0 114b628a B3/B4 (dangling-symlink caught via lstat, realpath failure fails closed, leaf files contained, skeleton writes contained) safe-paths 21/21 (malicious ids/paths, symlink escapes, canary intactness), review-round2 9/9 (in-project link pointing outside → refused before any write/delete), review-round3 19/19
2 silent overwrites — (covered by the row above + 7) v1.1.0 no-clobber 13/13: init-project refuses non-empty targets without --force (and --force resets only the 5 generated files); prep-image/fetch-official-images refuse existing files without --force
3 --base-url credential exfiltration v1.1.0 (HTTPS allowlist of official hosts; a custom endpoint requires the explicit, separately warned --allow-any-endpoint) endpoint-allowlist 6/6, incl. a local-server proof that the gate sits before the request
4 unvalidated fetches (SSRF / file: / redirects / size) 2 policy not at the real request point v1.1.0 (protocol/host/redirect/size rules), v1.5.0 ② (assertResolvedHost: DNS resolution validated per request; every browser subresource and every redirect hop goes through the same policy) fetch-policy 77/77 (incl. mapped-IPv6 forms and the ranges added in 1.7.0) + review-round2's end-to-end canary: a rejected destination receives zero requests on both the direct and the browser path
5 no executable tests v1.1.0 (72 cases), since grown 236 tests in 14 files, plain node:test, auto-discovered by the repo-root node --test; today's runs: 236/236 pass, 0 skip, in the dev tree and both published trees
3 subtitles frozen outside the animation window v1.5.0 ③ (per-sentence subtitle stills spliced in by the builder), v1.7.0 B2 (stale stills/manifests invalidated), v1.7.1 9ab1dfb2 (manifest framesCover compared with the live frame sequence) subtitles-invalidate 2/2 pixel-level: a clause scheduled late does reach the picture (same-sentence Δ1.0 / deleted-old-sentence Δ26.4 / --no-subs Δ13.6; stale-manifest Δ45, aligned Δ0); cover-transition 3/3 (no black frames at any cut, hard-cut and cross-dissolve, duration unchanged)
4 ASR request errors exit 0 v1.5.0 ④ review-round2: local 503 mock → errors counted as failures, exit ≠ 0, named in the output
5 single-quoted/unquoted attributes v1.5.0 ⑤ review-round2: single-quoted and unquoted style/class rewritten in place, no duplicate attributes
6 project-local ffmpeg not discovered v1.5.0 ⑥ (discovery includes cwd/project dir; asr/prep-image pass it) review-round2: ffmpeg-static found under the project's node_modules with system PATH isolated
7 macOS /var vs /private/var v1.5.0 ⑦ (--out-dir compared via canonical paths, resolving existing ancestors) no-clobber 13/13 (verified on Windows; the canonicalisation is platform-neutral path resolution)

Two notes on your Validation section. First, the CI runs marked action_required need a maintainer's approval to execute on this branch — with that approval, the scoped workflow now on b945b51b installs ffmpeg + Chromium and runs the full suite via shell-glob discovery (it previously enumerated files and missed four suites; that was fixed in b945b51b). Second, the one failure in your full npm run check (640 tests) was the macOS path-alias case in item 7, fixed as above; on a runner without ffmpeg/Playwright the suite now degrades to skips-with-reasons rather than failures — verified in a clean sandbox: 236 tests, 0 fail, 22 skipped with stated reasons (details in comment 5734652983).

If any of these still reproduces on b945b51b, please name the case and it will be fixed immediately. Otherwise, could you re-review the current head?

…ight 守卫(无工具 ubuntu CI 不再红) + 沙盒证据数字修正(236→237)
@Wzdhehe

Wzdhehe commented Sep 18, 2026

Copy link
Copy Markdown
Author

Final audit round (1.7.2, commits 21fd7c1b on the skill repo / 3a2f1886 on this branch) — two findings from our own last pass, plus one evidence correction

  1. --json never printed JSON in its documented position. The flag is boolean but was read through the value-taking reader, so fetch-official-images.mjs <url> --json (flag last, as the usage line documents) resolved it to undefined and silently printed the human-readable list instead. Now read by presence. The pre-existing test only asserted that the run reached URL validation — it could not see this — so a new end-to-end case drives a local file:// fixture page through the real listing path with the flag in both positions and asserts an actual JSON array comes out.
  2. The two symlink-canary cases could redden a tool-less CI. They had no Playwright guard, and both capture and preview-page load Playwright before their containment check. On a runner where symlinks are creatable but Playwright is absent (ubuntu CI is exactly this), capture exits 未找到 playwright and the case failed on its expected-message assertion — the same class of host-CI reddening fixed in 1.7.1 for the ASR case, in a different environment shape. Both canaries now skip with a stated reason when Playwright is missing.
  3. Evidence correction to the 1.7.1 numbers. Its "clean sandbox: 0 fail, 22 skipped" was measured in a Windows sandbox where junction creation itself failed (the shell was unreachable on the stripped PATH), which masked exactly the case above and made the number look better than a bare ubuntu runner would have shown. That claim was too narrow and is withdrawn. Corrected measurements after the fixes, in two isolated tool-less sandbox shapes — symlinks/junctions creatable, and not creatable — now agree: 237 tests, 0 fail, 23 skipped, every skip naming its reason.

Updated evidence, all reproduced today:

dev tree + both published trees (with tools):  237 tests / 237 pass / 0 fail / 0 skipped  (14 files)
two tool-less sandboxes (links creatable / not): 237 / 214 pass / 0 fail / 23 skipped (reasons stated)
node 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.2 in both plugin.json files; CHANGELOG 1.7.2 and both READMEs carry the corrected counts. Everything else from the last pass — host validation, layout, security disclosures, dependency-free imports, third-party notices, workflow YAML/pins/permissions, README claim spot-checks, evals sanity — was checked and holds. Requesting re-review at 3a2f1886.

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