Skip to content

feat(skills): add demo-video skill for narrated video assembly - #2404

Open
auyidi1 wants to merge 23 commits into
mainfrom
feat/demo-video-skill
Open

feat(skills): add demo-video skill for narrated video assembly#2404
auyidi1 wants to merge 23 commits into
mainfrom
feat/demo-video-skill

Conversation

@auyidi1

@auyidi1 auyidi1 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an experimental demo-video skill that assembles ordered frames or clips with per-segment narration into a narrated MP4 via FFmpeg. It closes the video-assembly gap in the Design Thinking demo workflow by bridging existing skills: tts-voiceover (narration WAVs) and vscode-playwright (frame capture).

What's included

  • SKILL.md — overview, segments.yml manifest schema, quick start, parameters reference, and the reuse bridge to tts-voiceover / vscode-playwright, plus FFmpeg prerequisites.
  • scripts/assemble_video.py — the assembler CLI (--manifest, --output, --fps, --resolution, -v/--verbose). Validates the manifest, resolves each segment's visual (frame image or clip) and narration WAV, derives per-segment duration from the narration length via ffprobe (overridable with duration:), renders normalized per-segment MP4s, and concatenates them. Uses subprocess.run with list args and no shell=True.
  • scripts/assemble-video.sh + scripts/Invoke-AssembleVideo.ps1 — dual bash/PowerShell wrappers mirroring the video-to-gif convention; the bash wrapper runs uv sync and forwards to the Python CLI.
  • examples/ — an example segments.yml manifest and a README describing the local fixture setup (fixtures are intentionally kept out of source control).
  • Testspytest suite (parse/validation, duration default+override, frame/clip branches, error paths, subprocess safety), an Atheris fuzz harness with corpus fixtures, and a Pester helper test.
  • Registration — added to the experimental and hve-core-all collections with regenerated plugin outputs.

Manifest schema

output: ./output/demo.mp4
resolution: 1280x720
fps: 24
segments:
  - type: frame
    visual: ./assets/intro.png
    narration: ./assets/intro.wav
    duration: 3.0
  - type: clip
    clip: ./assets/interaction.mp4
    narration: ./assets/interaction.wav

Validation

  • npm run validate:skills — pass
  • npm run test:py — 13 passed
  • npm run lint:collections-metadata / plugin:validate — pass
  • npm run lint:frontmatter — pass (demo-video files clean)
  • npm run validate:copyright — pass

Notes / follow-ups

  • Runtime dependency: FFmpeg + ffprobe must be on PATH. The end-to-end smoke run was not executed here because FFmpeg is not installed in the dev container.
  • The demo-setup orchestration (PR feat(skills): add demo-setup skill to design-thinking collection #1623) that would auto-generate the demo script and capture prototype frames is pending; until it merges, the DT → demo-video wiring is manual.

Checklist

  • Reviewed and validated by a qualified human reviewer

Add an experimental demo-video skill that assembles ordered frames or
clips with per-segment narration into a narrated MP4 via FFmpeg. Closes
the video-assembly gap in the Design Thinking demo workflow by bridging
tts-voiceover narration and vscode-playwright frame capture.

- Add SKILL.md with manifest schema, quick start, and reuse bridge
- Add Python assembler (assemble_video.py) with segments.yml validation,
  ffprobe-derived durations, and safe list-arg subprocess calls
- Add bash and PowerShell wrappers mirroring the video-to-gif convention
- Add pytest suite, Atheris fuzz harness, corpus fixtures, and Pester
  helper tests
- Register the skill in the experimental and hve-core-all collections and
  regenerate plugin outputs
@auyidi1
auyidi1 requested a review from a team as a code owner July 6, 2026 23:24
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Dependency Review

The following issues were found:
  • ✅ 0 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ⚠️ 1 package(s) with unknown licenses.
See the Details below.

License Issues

.github/skills/experimental/demo-video/uv.lock

PackageVersionLicenseIssue Type
pytest9.1.1NullUnknown License
Allowed Licenses: MIT, MIT-0, MIT-CMU, Apache-2.0, BSD-2-Clause, BSD-3-Clause, ISC, 0BSD, BlueOak-1.0.0, CC0-1.0, Unlicense, CC-BY-4.0, CC-BY-3.0, PSF-2.0, Python-2.0, HPND, LicenseRef-scancode-secret-labs-2011, WTFPL, LicenseRef-scancode-unicode
Excluded from license check: pkg:pypi/lxml, pkg:pypi/typing-extensions, pkg:pypi/certifi, pkg:pypi/charset-normalizer, pkg:pypi/shapely, pkg:pypi/backports-tarfile, pkg:pypi/cryptography, pkg:pypi/jaraco-classes, pkg:pypi/jaraco-context, pkg:pypi/jaraco-functools, pkg:pypi/keyrings-alt, pkg:pypi/torch, pkg:pypi/filelock, pkg:pypi/hf-xet, pkg:pypi/idna, pkg:pypi/safetensors, pkg:pypi/regex, pkg:npm/dompurify, pkg:npm/lunr-languages, pkg:npm/%40github/copilot, pkg:npm/%40github/copilot-darwin-arm64, pkg:npm/%40github/copilot-darwin-x64, pkg:npm/%40github/copilot-linux-arm64, pkg:npm/%40github/copilot-linux-x64, pkg:npm/%40github/copilot-win32-arm64, pkg:npm/%40github/copilot-win32-x64, pkg:npm/%40github/copilot-linuxmusl-arm64, pkg:npm/%40github/copilot-linuxmusl-x64, pkg:npm/lightningcss, pkg:npm/lightningcss-android-arm64, pkg:npm/lightningcss-darwin-arm64, pkg:npm/lightningcss-darwin-x64, pkg:npm/lightningcss-freebsd-x64, pkg:npm/lightningcss-linux-arm-gnueabihf, pkg:npm/lightningcss-linux-arm64-gnu, pkg:npm/lightningcss-linux-arm64-musl, pkg:npm/lightningcss-linux-x64-gnu, pkg:npm/lightningcss-linux-x64-musl, pkg:npm/lightningcss-win32-arm64-msvc, pkg:npm/lightningcss-win32-x64-msvc, pkg:npm/uri-js, pkg:npm/hve-core

OpenSSF Scorecard

Scorecard details
PackageVersionScoreDetails
pip/atheris 3.1.0 🟢 6.1
Details
CheckScoreReason
Code-Review🟢 7Found 23/30 approved changesets -- score normalized to 7
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Maintained⚠️ 23 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 2
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
Packaging⚠️ -1packaging workflow not detected
Binary-Artifacts🟢 10no binaries found in the repo
SAST⚠️ 0no SAST tool detected
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Fuzzing🟢 10project is fuzzed
Signed-Releases⚠️ -1no releases found
License🟢 10license file detected
Security-Policy🟢 10security policy file detected
Branch-Protection⚠️ 0branch protection not enabled on development/release branches
Pinned-Dependencies⚠️ 0dependency not pinned by hash detected -- score normalized to 0
pip/colorama 0.4.6 UnknownUnknown
pip/coverage 7.15.0 UnknownUnknown
pip/iniconfig 2.3.0 UnknownUnknown
pip/packaging 26.2 UnknownUnknown
pip/pluggy 1.6.0 UnknownUnknown
pip/pygments 2.20.0 UnknownUnknown
pip/pytest 9.1.1 UnknownUnknown
pip/pytest-cov 7.1.0 UnknownUnknown
pip/pytest-mock 3.15.1 UnknownUnknown
pip/pyyaml 6.0.3 UnknownUnknown
pip/ruff 0.15.20 UnknownUnknown
pip/tomli 2.4.1 UnknownUnknown
pip/atheris 3.1.0 🟢 6.1
Details
CheckScoreReason
Code-Review🟢 7Found 23/30 approved changesets -- score normalized to 7
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Maintained⚠️ 23 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 2
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
Packaging⚠️ -1packaging workflow not detected
Binary-Artifacts🟢 10no binaries found in the repo
SAST⚠️ 0no SAST tool detected
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Fuzzing🟢 10project is fuzzed
Signed-Releases⚠️ -1no releases found
License🟢 10license file detected
Security-Policy🟢 10security policy file detected
Branch-Protection⚠️ 0branch protection not enabled on development/release branches
Pinned-Dependencies⚠️ 0dependency not pinned by hash detected -- score normalized to 0
pip/colorama 0.4.6 UnknownUnknown
pip/coverage 7.15.0 UnknownUnknown
pip/iniconfig 2.3.0 UnknownUnknown
pip/packaging 26.2 UnknownUnknown
pip/pluggy 1.6.0 UnknownUnknown
pip/pygments 2.20.0 UnknownUnknown
pip/pytest 9.1.1 UnknownUnknown
pip/pytest-cov 7.1.0 UnknownUnknown
pip/pytest-mock 3.15.1 UnknownUnknown
pip/pyyaml 6.0.3 UnknownUnknown
pip/ruff 0.15.20 UnknownUnknown
pip/tomli 2.4.1 UnknownUnknown

Scanned Files

  • .github/skills/experimental/demo-video/uv.lock
  • plugins/experimental/skills/experimental/demo-video/uv.lock

@codecov-commenter

codecov-commenter commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.23622% with 101 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.62%. Comparing base (53ddf1a) to head (dcf95b7).

Files with missing lines Patch % Lines
.../experimental/demo-video/scripts/assemble_video.py 68.30% 71 Missing ⚠️
...mental/demo-video/scripts/Invoke-AssembleVideo.ps1 0.00% 30 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2404      +/-   ##
==========================================
+ Coverage   82.76%   85.62%   +2.85%     
==========================================
  Files         154       87      -67     
  Lines       20853     9132   -11721     
  Branches       13       13              
==========================================
- Hits        17259     7819    -9440     
+ Misses       3592     1311    -2281     
  Partials        2        2              
Flag Coverage Δ
docusaurus 94.44% <ø> (ø)
pester 85.97% <0.00%> (-0.31%) ⬇️
pytest 68.30% <68.30%> (-11.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...mental/demo-video/scripts/Invoke-AssembleVideo.ps1 0.00% <0.00%> (ø)
.../experimental/demo-video/scripts/assemble_video.py 68.30% <68.30%> (ø)

... and 70 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Eval Execution

Status: Passed — no merge-blocking failures (6 advisory assertion failure(s) present)

  • Artifacts evaluated: 1
  • Specs run: 1
  • Assertions passed: 3
  • Assertions failed (blocking): 0
  • Assertions failed (advisory): 6
  • Failed specs (merge-blocking): 0
Artifact Kind Status Specs Passed Failed (blocking) Failed (advisory)
demo-video skill ⚠️ advisory-fail 1 3 0 6

Legend — ✅ clean · ⚠️ advisory failures only (non-blocking) · ⏭️ skipped · ❌ merge-blocking failure

Only Failed specs (merge-blocking) gates this PR. Advisory assertion failures are signal-quality checks captured during iteration; review them, but they do not block merge and may be acceptable.

rezatnoMsirhC and others added 5 commits July 9, 2026 11:37
Use the canonical "Copyright (c) 2026 Microsoft Corporation. All rights reserved." form so Copyright Header Validation passes on PR #2404.
… narration

Recommend the tts-voiceover skill's Azure AI Speech (Azure AI Foundry) neural voices as the default for shareable narration and mark offline open-source TTS as a no-credential fallback only.
Add knowledge, tool-trigger, and bleed-detection stimuli for the demo-video skill so the eval stimulus-presence gate passes on PR #2404.
Replace the truncated 'narrat' stem with 'narration|narrated' so Spell Check passes on PR #2404.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a new experimental demo-video skill to assemble ordered prototype frames/clips and per-segment narration WAVs into a single narrated MP4 using FFmpeg, and registers the skill across collections, plugins, and behavior evals.

Changes:

  • Added the demo-video skill package (docs, Python assembler CLI, bash/PowerShell wrappers, examples, and tests including a fuzz harness).
  • Registered demo-video in experimental and hve-core-all collections and regenerated plugin READMEs.
  • Extended skill behavior conformance eval coverage with demo-video knowledge/tool-trigger/bleed-detection stimuli.

Reviewed changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
plugins/hve-core-all/README.md Adds demo-video to the packaged skills list
plugins/experimental/README.md Adds demo-video to the experimental skills list
evals/behavior-conformance/skill-behavior.eval.yaml Adds 3 new demo-video behavior-conformance stimuli and updates total count
collections/hve-core-all.collection.yml Registers demo-video skill in the hve-core-all collection
collections/hve-core-all.collection.md Lists demo-video in the hve-core-all collection documentation table
collections/experimental.collection.yml Registers demo-video skill in the experimental collection
collections/experimental.collection.md Lists demo-video in the experimental collection documentation table
.github/skills/experimental/demo-video/SKILL.md Documents the skill purpose, manifest schema, usage, and prerequisites
.github/skills/experimental/demo-video/pyproject.toml Declares Python dependencies + pytest/ruff config for the skill
.github/skills/experimental/demo-video/uv.lock Pins the Python environment for the skill
.github/skills/experimental/demo-video/scripts/assemble_video.py Implements the manifest-driven FFmpeg assembly CLI
.github/skills/experimental/demo-video/scripts/assemble-video.sh Bash wrapper to uv sync and run the Python assembler
.github/skills/experimental/demo-video/scripts/Invoke-AssembleVideo.ps1 PowerShell wrapper to uv sync and run the Python assembler
.github/skills/experimental/demo-video/tests/test_assemble_video.py Pytest coverage for manifest parsing/validation and ffmpeg subprocess safety
.github/skills/experimental/demo-video/tests/fuzz_harness.py Adds an Atheris/pytest-compatible fuzz harness for the manifest validator
.github/skills/experimental/demo-video/tests/DemoVideoHelpers.Tests.ps1 Adds Pester tests for wrapper parameter/argument forwarding
.github/skills/experimental/demo-video/tests/corpus/valid-manifest.yml Adds fuzz/test corpus fixture for a valid manifest shape
.github/skills/experimental/demo-video/tests/corpus/unknown-key.yml Adds fuzz/test corpus fixture for unknown-key validation
.github/skills/experimental/demo-video/tests/corpus/empty-segments.yml Adds fuzz/test corpus fixture for empty segments validation
.github/skills/experimental/demo-video/examples/segments.yml Adds an example segments manifest
.github/skills/experimental/demo-video/examples/README.md Documents how to set up local fixtures for the example manifest
Comments suppressed due to low confidence (1)

.github/skills/experimental/demo-video/examples/README.md:30

  • The file ends with a whitespace-only line (trailing spaces). This tends to fail markdownlint/formatters; remove the trailing spaces/extra blank line at EOF.
*🤖 Crafted with precision by ✨Copilot following brilliant human instruction, then carefully refined by our team of discerning human reviewers.*

Comment thread .github/skills/experimental/demo-video/scripts/Invoke-AssembleVideo.ps1 Outdated
Comment thread .github/skills/experimental/demo-video/scripts/assemble_video.py Outdated
Comment thread .github/skills/experimental/demo-video/scripts/assemble_video.py
Comment thread .github/skills/experimental/demo-video/examples/README.md Outdated
Comment thread .github/skills/experimental/demo-video/scripts/assemble_video.py
Comment thread .github/skills/experimental/demo-video/SKILL.md
- Require PowerShell 7.4 in the wrapper to match repo conventions
- Make uv install hint platform-neutral (winget/curl)
- Remove dead subprocess.CalledProcessError handler
- Emit debug logging so --verbose has observable effect
- Drop unused manifest_path parameter from _validate_manifest
- Use H2 heading in examples README (title in frontmatter)
- Fix Pester fallback repo-root walk (5 levels)
- Remove stale 'planned' qualifier in SKILL.md
@github-actions github-actions Bot mentioned this pull request Jul 9, 2026
The _validate_manifest signature no longer takes manifest_path; update the pytest and fuzz-harness call sites accordingly.

@bindsi bindsi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@auyidi1 if you don´t mind you can add the additional context from our chat yesterday to the readme as well. Thanks for this great skill

jkim323 and others added 2 commits July 10, 2026 21:57
# Conflicts:
#	collections/hve-core-all.collection.md
#	evals/behavior-conformance/skill-behavior.eval.yaml
#	plugins/hve-core-all/README.md

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.

Comment thread .github/skills/experimental/demo-video/scripts/assemble_video.py Outdated
Comment thread .github/skills/experimental/demo-video/scripts/assemble_video.py
Comment thread .github/skills/experimental/demo-video/tests/fuzz_harness.py Outdated
Comment thread collections/hve-core-all.collection.md

@chaosdinosaur chaosdinosaur 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.

Thanks for the demo-video skill — the assembler/wrapper design is clean, the subprocess handling is injection-safe (list args, no shell=True, yaml.safe_load), and the repo Python-skill conventions (ruff/pytest/fuzz/uv.lock, dual wrappers, copyright headers) are all met. A few items I didn't see raised elsewhere on the PR:

  • Blocking: the shipped examples/segments.yml uses ./examples/fixtures/... paths, but the assembler resolves relative paths against the manifest's own directory (examples/), so they expand to examples/examples/fixtures/... and don't match the fixture layout examples/README.md documents — the flagship example fails on first run. Changing the manifest paths to ./fixtures/... (and output: ./output/demo.mp4) fixes it; please verify end-to-end since the PR notes the smoke run was skipped.
  • The npm run test:py 13 passed figure in the PR body isn't reproducible — the two discoverable pytest files contain 11 test functions; please re-run and update the count.
  • tests/DemoVideoHelpers.Tests.ps1 doesn't mirror its system under test (Invoke-AssembleVideo.ps1); renaming to Invoke-AssembleVideo.Tests.ps1 matches the Pester convention.
  • Minor: SKILL.md's Manifest Schema omits the top-level output/resolution/fps keys the code and example use; an empty-string narration skips the narration_wav fallback; and _validate_manifest returns the segments list redundantly.

Note

This review was AI-assisted and should be independently validated by a qualified human reviewer before acting on any finding.

auyidi added 2 commits July 13, 2026 18:47
…z scope

Validate effective frame rate is positive after merging CLI and manifest values, enforce that a declared segment type matches its visual or clip source, and narrow fuzz suppression to ManifestError so unexpected crashes surface. Adds tests for fps and type validations.
Coerce unexpected manifest keys to str when building error messages so non-string YAML keys raise ManifestError instead of a TypeError that crashes the Atheris fuzz target. Adds a regression test and applies ruff formatting.
Comment thread .github/skills/experimental/demo-video/examples/README.md

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dependency Review — demo-video Skill

This PR introduces a new Python skill with the following dependencies. No existing workflow, devcontainer, or copilot-setup-steps.yml files are modified, so SHA-pinning and environment-sync checks are not applicable.

Dependencies Introduced

Package Resolved Group License Notes
pyyaml 6.0.3 runtime MIT YAML manifest parsing — necessary, no alternative in stdlib
pytest 9.1.1 dev MIT Standard test runner
pytest-cov 7.1.0 dev MIT Coverage reporting
pytest-mock 3.15.1 dev MIT Mock support for pytest
ruff 0.15.20 dev MIT Linter — consistent with other skills
atheris 3.1.0 fuzz Apache-2.0 OSSF fuzz harness — consistent with repo convention
Transitive (colorama, coverage, iniconfig, packaging, pluggy, pygments, tomli) indirect MIT/BSD/Apache All permissive, no concerns

Safety Checks

Check Result
License compatibility (MIT project) ✅ All permissive (MIT, Apache-2.0, BSD-3-Clause)
New dependencies justified pyyaml is necessary for manifest parsing; dev/fuzz deps follow repo convention
Duplicates existing functionality ✅ No duplication detected
GitHub Actions SHA pinning ✅ No workflow files modified
Devcontainer / setup alignment ✅ Neither .devcontainer/ nor copilot-setup-steps.yml touched
Known vulnerabilities ✅ No known CVEs for these versions

All dependency safety checks passed. No action required from this review.

Generated by Dependabot PR Review for #2404 · 39.3 AIC · ⌖ 7.05 AIC · ⊞ 8.5K

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dependency Review — demo-video skill

Scope: pyproject.toml + uv.lock added under .github/skills/experimental/demo-video/


Dependencies introduced

Package Version constraint Group License Notes
pyyaml >=6.0 runtime MIT Actively maintained; widely used
pytest >=9.0 dev MIT Standard test framework
pytest-cov >=5.0 dev MIT Coverage plugin
pytest-mock >=3.14 dev MIT Mock plugin
ruff >=0.15 dev MIT Linter/formatter
atheris >=3.0 fuzz Apache-2.0 OSS fuzz harness; Apache-2.0 is MIT-compatible

Indirect lock-file entries (colorama, coverage, iniconfig, packaging, pluggy, pygments, tomli) are all MIT, BSD, or Apache-2.0 — all compatible with the project's MIT license.


Safety check results

  • License compatibility — all dependencies (runtime and dev) carry MIT, BSD, or Apache-2.0 licenses, compatible with the project's MIT license.
  • No duplicationpyyaml is the only runtime dependency; no existing repo-level dependency provides YAML parsing for this skill's use case.
  • SHA/hash pinninguv.lock pins every wheel and sdist by SHA-256 hash. No GitHub Actions references were modified in this PR.
  • Environment synchronization — no changes to .devcontainer/ or copilot-setup-steps.yml; synchronization concern is N/A.
  • Vulnerability posture — all packages are well-established, actively maintained, and carry no known vulnerability signals. atheris is correctly isolated in the fuzz dependency group (no macOS wheels) per repo convention.
  • uv.lock committed — lock file is present alongside pyproject.toml, satisfying Dependabot resolution requirements.

All dependency safety checks passed. No findings require changes.

Generated by Dependabot PR Review for #2404 · 42.7 AIC · ⌖ 8.7 AIC · ⊞ 8.5K

… tts-voiceover

The main merge left collections/hve-core-all.collection.md out of sync with its YAML source: the generated markdown retained the demo-video entry but dropped the tts-voiceover row that the YAML still lists. Regenerate via plugin:generate so the table includes both skills and prevent a merge-time regression that would remove tts-voiceover from main.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.

Comment thread .github/skills/experimental/demo-video/scripts/assemble_video.py
Comment thread .github/skills/experimental/demo-video/scripts/assemble_video.py
Comment thread .github/skills/experimental/demo-video/examples/README.md Outdated
auyidi1 and others added 3 commits July 15, 2026 09:28
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines +1 to +11
output: ./examples/output/demo.mp4
resolution: 1280x720
fps: 24
segments:
- type: frame
visual: ./examples/fixtures/intro.png
narration: ./examples/fixtures/intro.wav
duration: 3.0
- type: clip
clip: ./examples/fixtures/interaction.mp4
narration: ./examples/fixtures/interaction.wav

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The assembler resolves manifest-relative paths against manifest_path.parent (the examples/ directory). The paths in this file use the ./examples/... prefix, which causes them to resolve to <skill-root>/examples/examples/... — a location that does not exist. The example will fail on first run.

The examples/README.md documents fixtures at examples/fixtures/, which means the manifest (located at examples/segments.yml) should reference them as ./fixtures/....

Suggested fix:

output: ./output/demo.mp4
resolution: 1280x720
fps: 24
segments:
  - type: frame
    visual: ./fixtures/intro.png
    narration: ./fixtures/intro.wav
    duration: 3.0
  - type: clip
    clip: ./fixtures/interaction.mp4
    narration: ./fixtures/interaction.wav

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: the repo Pester convention names test files after the script they cover. Since this file tests scripts/Invoke-AssembleVideo.ps1, consider renaming it to Invoke-AssembleVideo.Tests.ps1 to make the source-to-test mapping clear at a glance.

Comment on lines +28 to +50
narration: ./audio/intro.wav
duration: 4.5
- type: clip
clip: ./clips/interaction.mp4
narration: ./audio/interaction.wav
```

### Segment fields

* `type` identifies whether the segment is a still image (`frame`) or a motion clip (`clip`)
* `visual` points to an image file for a frame segment
* `clip` points to a motion clip file for a clip segment
* `narration` points to the WAV file generated from narration text (the script also accepts `narration_wav` as an alias)
* `duration` is optional and overrides the inferred duration when you want a fixed segment length

## Quick Start

Use the bash or PowerShell wrappers to invoke the assembler from the skill directory.

```bash
scripts/assemble-video.sh --manifest examples/segments.yml --output ./output/demo.mp4
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Manifest Schema section only shows the segments list. The three top-level keys that the assembler validates and uses (output, resolution, fps) are not documented, so a user reading SKILL.md alone would not know they exist or what their defaults are. The PR description's own schema example includes all three.

Consider expanding the schema example and field table to cover them:

output: ./output/demo.mp4   # optional; path for the assembled MP4
resolution: 1280x720        # optional; default 1280x720
fps: 24                     # optional; default 24
segments:
  - type: frame
    ...

And add field descriptions alongside the existing segment-field bullet list:

  • output sets the destination path for the assembled MP4 (resolved relative to the manifest; defaults to ./demo.mp4)
  • resolution controls output width and height in WIDTHxHEIGHT form (default 1280x720)
  • fps sets the frame rate applied when rendering each segment (default 24)

@chaosdinosaur chaosdinosaur 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-reviewing at 2f2d912 — the type/source enforcement, --fps validation, non-string-key handling, and tighter fuzz scope from the follow-up commits all look good and are covered by new tests. I'm not repeating the items already open on the PR (the examples/segments.yml path-doubling thread and the Pester filename / SKILL.md top-level-keys notes from @rezatnoMsirhC still apply and remain unresolved — the example still fails on first run). A few net-new items I didn't see raised elsewhere:

  • Please verify (from the concat change): the final concat now uses -c copy (per the earlier suggestion) instead of re-encoding. With fractional per-segment durations and fixed-size AAC frames, stream-copy concat can drift A/V sync or leave gaps at segment seams that the previous re-encode masked. Since FFmpeg isn't in the dev container and the smoke run is still skipped, this primary output path is untested — please confirm the final MP4 stays in sync end-to-end, or keep a re-encode fallback for the concat step.
  • Add a shipped-example regression test: a test that loads examples/segments.yml and asserts the resolved fixture paths match the README layout would guard the open path-doubling issue from recurring.
  • PR body count: npm run test:py 13 passed isn't reproducible — there are now 14 discoverable test functions for this skill (12 in test_assemble_video.py + 2 in fuzz_harness.py). Please re-run and update the figure (or note it''s a repo-wide aggregate).
  • Minor: an empty-string narration: still skips the narration_wav fallback (only None/absent falls back); and _validate_manifest returns the segments list a second time even though the config dict it returns already contains segments.

Also note the branch is currently BEHIND main and the four approvals predate the still-open 07-15 findings, so a fresh look after the fixes would be worthwhile.

Note

This review was AI-assisted and should be independently validated by a qualified human reviewer before acting on any finding.

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.

8 participants