ci(skills): harden test-skills job with fail-loud find guard

[jrusso1020]

Context

HF#1723 (merged 2026-06-25) added a Test: skills CI job that runs skills/**/*.test.mjs via a single node --test invocation. This PR hardens it to fail loudly when zero test files match — silently no-op'ing on a future rename would defeat the whole point of the job.

Changes

File	Change
.github/workflows/ci.yml (test-skills job)	Replace node --test 'skills/**/*.test.mjs' with a find + mapfile + count-check + node --test "\${SKILLS_TESTS[@]}" sequence. set -euo pipefail; emits ::error:: and exits 1 if no files match.
.github/workflows/ci.yml (skills filter)	Filter path stays as-is from HF#1723 (skills/**, package.json, ci.yml).
.github/workflows/ci.yml (job name)	Test (skills) → "Test: skills" to match the existing convention ("Test: runtime contract", "SDK: unit + contract + smoke").
.github/workflows/ci.yml (comment)	Expanded the doc comment with examples of what files this catches and why it matters (shell-injection regression guards).

Why fail-loud matters

Without the count check, a refactor that moves skills/media-use/scripts/lib/*.test.mjs to a new location (e.g. consolidating into a single test directory) would silently turn this job into a no-op pass — the very class of failure this job was created to prevent.

Test plan

• Local rebase against main, clean (no behavior changes vs HF#1723's intent — only the fail-loud guard + name + comment)
• CI: Test: skills runs and passes (will discover resolve.test.mjs + manifest.test.mjs + probe.test.mjs now that HF#1723 is merged)
• No regressions in other CI jobs

---

— Jerrai

[miga-heygen]

Reviewed the full diff and CI run logs. This is a clean, well-scoped CI gap fix. Verdict: LGTM.

What the PR does right:

1. Path filter is correctly scoped. The new skills output triggers on skills/** and .github/workflows/ci.yml — matching the existing cli filter convention. Skills-only PRs fire the job; unrelated PRs skip it. Including ci.yml itself is smart: this very PR proves the job runs on its own bootstrap change.

2. Zero-match guard is solid. The find → mapfile → count check pattern with set -euo pipefail is exactly the right shape. If someone renames the test directory or changes the file extension, the job fails loudly with a ::error:: annotation instead of silently passing with zero tests. This is the defensive CI pattern that prevents the "green but vacuous" problem.

3. node --test is the right runner. The skills tests import only from node: built-ins and relative paths — no workspace dependencies, no bun install needed. Using Node's native test runner avoids pulling in bun/npm setup for tests that don't need it. The job completes in ~9s with a 5-minute timeout, which is generous but appropriate.

4. Minimal footprint. Only actions/checkout and actions/setup-node — no LFS, no bun, no FFmpeg, no frozen lockfile install. Exactly the dependency set these tests actually need. Fast, cheap, and correct.

5. Job placement and naming. test-skills / "Test: skills" follows the existing convention alongside "Test: runtime contract" and "SDK: unit + contract + smoke". Clean.

6. Action pins use commit SHAs (not floating tags), consistent with the rest of the workflow.

One observation (not blocking): The CI logs show # /bin/sh: 1: ffprobe: not found from resolve.test.mjs. The tests still pass because ffprobe is only used in a code path that the test stubs out — but it does mean the test exercises the "ffprobe missing" fallback rather than the "ffprobe present" path. Not a problem introduced by this PR (it already happens locally on machines without ffmpeg), just noting it for awareness.

All CI checks pass. The gap analysis in the PR description is thorough and accurate — the existing Test job genuinely cannot reach skills tests even with a filter change, making the dedicated job the correct approach.

— Miga

[miga-heygen]

Re-review after rebase to 6c2a1137d. Scope narrowed cleanly — only the fail-loud guard and naming convention fix remain; PR #1723's base job is already merged.

Diff check (2 logical changes):

1. Find guard — The find → mapfile → count check pattern under set -euo pipefail is correct. If the glob matches zero files, the job emits ::error::No skills/**/*.test.mjs files found and exits 1. This prevents the silent no-op pass that would defeat the job's purpose. The sort in the pipeline is a nice touch for deterministic log output. The printf listing before node --test gives clear CI log visibility into exactly which files ran.

2. Job name — Test (skills) → "Test: skills" aligns with "Test: runtime contract", "SDK: unit + contract + smoke", etc. Confirmed the convention on main — every other job uses "Category: description" with quotes and a colon.

Comment update — the expanded doc comment accurately describes what the job catches and why (shell-injection regression guards, node: built-in imports only, not covered by the main Test job's code path filter). Good for future maintainers.

CI status — all checks green on this SHA. Test: skills itself passed successfully, confirming the find guard discovers and runs the test files.

Verdict: LGTM. Clean, minimal, does exactly what it says. No concerns.

— Miga

[miguel-heygen]

R2 approval at 6c2a1137d.

Specific checks:

• .github/workflows/ci.yml:62 keeps the skills filter scoped to skills/**, package.json, and the workflow file, so skills-only changes trigger this job without broadening the main Test job.
• .github/workflows/ci.yml:240 renames the job to "Test: skills", matching the existing CI naming convention.
• .github/workflows/ci.yml:255 uses find + mapfile + an explicit zero-count failure before node --test, so future test-file moves cannot silently turn the job into a passing no-op.

Verification: current-head required checks are green; the apparent CLI: npx shim (${{ matrix.os }}) failure is from a cancelled superseded run. GitHub job Test: skills passed on head SHA 6c2a1137d3cedc5468d0855eeb1e9f07d2072a1b. Locally reran the exact discovery command with Node 22 (npx -y node@22 --test ...) and it discovered 3 skills test files with all tests passing.

Verdict: APPROVE
Reasoning: The workflow change is minimal, the fail-loud guard addresses the CI no-op risk directly, and both GitHub plus local Node 22 verification exercise the new path successfully.

— Magi