fix(skills): preserve nested staged asset paths

[miguel-heygen]

Summary

• preserve safe project-relative paths from asset_candidates instead of collapsing them to basenames
• recognize assets already present under nested directories such as assets/keyframes/
• stage missing capture assets into the requested nested destination across the three shared skill implementations

Regression

stage-assets.mjs previously warned that scene-01-meeting.png would 404 even when the approved file existed at assets/keyframes/scene-01-meeting.png.

Tests

• node --test skills/product-launch-video/scripts/lib/*.test.mjs (8/8)
• exact reported staging repro: staged 1/1 asset(s) with no anomaly
• bun packages/cli/scripts/gen-skills-manifest.ts --check
• shared asset helpers are byte-identical
• git diff --check

[vanceingalls]

R1 review — I read all three copies of scripts/lib/assets.mjs, the new test file, assemble-index.mjs, and stage-assets.mjs end-to-end, plus a repo-wide search for other callers of stageAssets/basenamesFromCandidates and other capture→assets staging paths.

Position: APPROVE. The fix is right — false-negative "not found under capture" for assets/keyframes/scene-01-meeting.png was because basenamesFromCandidates collapsed the nested path, dest = assets/scene-01-meeting.png missed the on-disk file at assets/keyframes/scene-01-meeting.png, then capture lookup by basename missed too. New assetPathsFromCandidates preserves the relative path; stageAssets uses it for both destination (with mkdirSync(dirname(dest), { recursive: true })) and source (nested-then-basename fallback per capture dir). Field-signal case now hits the existsSync(dest) shortcut and staged++.

Strengths

• Defensive filter block in assetPathsFromCandidates (scripts/lib/assets.mjs:15-22) closes traversal / absolute / drive-letter escapes AT the parse boundary — the right layer. posix.normalize before the ../ check catches obliqued attempts (assets/foo/../../secret → ../secret → filtered).
• basenamesFromCandidates retained as assetPathsFromCandidates(...).map(basename) — clean backward-compat, no consumer whiplash.
• Per-dir [nested, basename] flatMap (assets.mjs:53-55) is exactly the right precedence: nested wins when present, basename is the fallback for older captures that stashed flat. First-wins across capture dirs is preserved.
• Byte-identical patch across three skills (faceless-explainer, pr-to-video, product-launch-video) — confirmed by re-reading each diff.

Important — gap in the new regression test
skills/product-launch-video/scripts/lib/assets.test.mjs covers (a) escape rejection and (b) the destination-already-present shortcut. It does NOT cover the actually-new code path: capture-source-found + nested-destination-copy (i.e. mkdirSync(dirname(dest), { recursive: true }) + copyFileSync into assets/keyframes/foo.png). That's the largest new behavioral surface — the assertion in the current test lands on existsSync(dest) → staged++, which took the same path before this PR. Add one case that writes capture/assets/keyframes/foo.png (or capture/assets/foo.png for the basename-fallback shape), calls stageAssets, and asserts (i) staged === 1, (ii) assets/keyframes/foo.png exists post-call, (iii) no anomalies. Not blocking, but leaves the actual mkdirSync-then-copy path unexercised.

Nits

• Stale header comments: skills/product-launch-video/scripts/stage-assets.mjs:8 (Writes: assets/<basename>) and skills/product-launch-video/scripts/assemble-index.mjs:29 (capture/{assets,assets/videos,screenshots}/<basename> for staging) — both still say <basename> even though staging is now relative-path-aware. Two-word fixes.
• basenamesFromCandidates has no external callers in the tree (only stageAssets inside the same file uses assetPathsFromCandidates directly). Keep the export if there's an out-of-tree consumer; otherwise it's dead and can be pruned in a follow-up.

Extrapolation check (clean)

• stageAssets is only called from stage-assets.mjs and assemble-index.mjs per skill — both go through the same helper, so the fix propagates automatically. No other in-tree pipeline reads asset_candidates for existence checks: build-frame.mjs does its own capture/assets/fonts → assets/fonts staging with a different contract (font-family names, not asset paths), so it isn't affected by this change and doesn't share the failure mode. check/render were already nested-path-aware per the field signal — this PR realigns staging with them.
• Adversarial spot-checks: UNC \\server\share\file → after replaceAll("\\","/") becomes //server/share/file → posix.normalize collapses to /server/share/file → posix.isAbsolute filters. Symlinks in capture/ get followed by copyFileSync (target contents land inside assets/, no escape). Case-sensitivity is unchanged from pre-PR behavior.

Verdict: APPROVE
Reasoning: Fix is correctly targeted, backward-compatible, and adds a defensive escape filter at the parse boundary. The one gap worth chasing is a test that exercises the new mkdirSync+copyFileSync path rather than the already-covered existsSync shortcut — flag as important follow-up, not a blocker.

— Via

[james-russo-rames-d-jusso]

Reviewed at 46f2274.

asset_candidates now preserves project-relative subdirectory paths (assets/keyframes/scene-01-meeting.png) instead of collapsing to basenames, so nested approved assets stop reporting as 404-missing. stageAssets first tries the nested source path, then falls back to basename lookup for existing assets that were originally landed flat. Path-traversal filters guard ../, absolute, and Windows-drive-prefixed inputs. Byte-identical across the three shared skill implementations.

Blockers

(none)

Concerns

(none)

Nits

🟡 Test asymmetry across the three shared skills. assets.test.mjs is added only to skills/product-launch-video/scripts/lib/; faceless-explainer and pr-to-video don't get a copy. Byte-identity across the three assets.mjs files means one test effectively covers all three — but nothing at CI enforces byte-identity, so future drift in the other two skills' copies would go untested. Same pattern as #2484; a shared-file-parity CI check would close this permanently for both. Short-term, mirroring the test file to the other two skills matches the "shared file, three homes" pattern.

🟡 basenamesFromCandidates is now orphaned as an external export. After this PR, stageAssets uses assetPathsFromCandidates internally, and grepping the workspace shows no other callers of basenamesFromCandidates outside the three declaration sites. It's retained as a thin wrapper (assetPathsFromCandidates(value).map(basename)) — reasonable if the intent is to keep the public skills-lib export stable — but worth deleting in a follow-up if there really are no consumers hiding behind dynamic imports or downstream skills-repo forks.

Green notes

🟢 Path-traversal filter is comprehensive. After the assets/ strip and posix.normalize, the filter rejects ., anything starting with ../, absolute paths (posix.isAbsolute), and Windows drive-letter prefixes (/^[A-Za-z]:\//). Verified against assets/../secret.png, /tmp/secret.png, C:/secret.png in the new test — all yield []. Backslash normalization runs first (.replaceAll("\\", "/")) so mixed Windows-style separators can't smuggle through the checks. ✓

🟢 Byte-identity verified across the three assets.mjs copies at head SHA. MD5 e23f679c5f458e71f24756932cb18a2c for all three; origin/main baseline was also identical (2fdee5b642fb818c7898a38a4cc3b5fd). ✓

🟢 Fallback ordering (nested-first, then basename per captureDir). captureDirs.flatMap((dir) => [join(dir, assetPath), join(dir, basename(assetPath))]).find(existsSync) — for assets/keyframes/scene-01.png, order is: capture/assets/keyframes/scene-01.png → capture/assets/scene-01.png → capture/assets/videos/keyframes/scene-01.png → capture/assets/videos/scene-01.png → screenshots-equivalent. Nested-path wins first; basename fallback preserves compatibility with existing flat-landed assets. Correct precedence. ✓

🟢 mkdirSync(dirname(dest), { recursive: true }) creates the destination subdirectory tree before copyFileSync. For a flat asset path (dirname("scene-01.png") === "."), this is a no-op mkdir on the already-existing assetsDir. For nested (dirname("keyframes/scene-01.png") === "keyframes"), it creates assets/keyframes/. ✓

🟢 Anomaly message uses the nested assetPath — asset "keyframes/scene-01.png" named by a frame but not found under capture/ — so a 404 message is now debuggable back to the exact subdirectory the author declared, not a bare basename. Small readability win. ✓

🟢 stageAssets return shape is unchanged ({ staged, wanted, anomalies }). wanted is a Set of paths (nested when nested was declared, basename when declared flat) — the test asserts [...result.wanted] === ["keyframes/scene-01-meeting.png"] which matches. No downstream breakage from a shape change. ✓

🟢 CI green at 46f2274.

Verdict framing

Right shape — extend to full project-relative paths with sane traversal defenses, keep the basename fallback so existing pre-nested assets still resolve. Byte-identity discipline maintained across the three skills. LGTM from my side.

— Review by Rames D Jusso