Skip to content

Commit efe2404

Browse files
jrusso1020jrusso1020
authored
fix(skills): stage media downloads in private directories (#3787)
1 parent 91a34ff commit efe2404

5 files changed

Lines changed: 131 additions & 15 deletions

File tree

‎skills-manifest.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
"files": 12
5151
},
5252
"media-use": {
53-
"hash": "a397849a32047f7e",
53+
"hash": "591daf2adb78e1cd",
5454
"files": 156
5555
},
5656
"motion-graphics": {

‎skills/media-use/scripts/lib/heygen-video-provider.mjs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { execFileSync } from "node:child_process";
2+
import { mkdtempSync, rmSync } from "node:fs";
23
import { tmpdir } from "node:os";
34
import { join } from "node:path";
45
import { freezeUrl } from "./freeze.mjs";
@@ -106,10 +107,20 @@ export async function heygenVideoGenerate(intent, ctx) {
106107
return null;
107108
}
108109

109-
const tmpPath = join(tmpdir(), `media-use-heygen-video-${process.pid}-${Date.now()}.mp4`);
110+
let tmpDir;
111+
let tmpPath;
110112
try {
113+
tmpDir = mkdtempSync(join(tmpdir(), "media-use-heygen-video-"));
114+
tmpPath = join(tmpDir, "video.mp4");
111115
await freezeUrl(videoUrl, tmpPath);
112116
} catch (err) {
117+
if (tmpDir) {
118+
try {
119+
rmSync(tmpDir, { recursive: true, force: true });
120+
} catch {
121+
// Preserve the download failure if temporary-directory cleanup fails.
122+
}
123+
}
113124
console.error(`media-use: heygen video download failed: ${err.message}`);
114125
return null;
115126
}

‎skills/media-use/scripts/lib/heygen-video-provider.test.mjs‎

Lines changed: 88 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,32 @@
11
import { strict as assert } from "node:assert";
2-
import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
2+
import {
3+
chmodSync,
4+
mkdtempSync,
5+
readFileSync,
6+
rmSync,
7+
statSync,
8+
symlinkSync,
9+
readdirSync,
10+
writeFileSync,
11+
} from "node:fs";
312
import http from "node:http";
413
import { tmpdir } from "node:os";
5-
import { join } from "node:path";
14+
import { basename, dirname, join } from "node:path";
615
import { test } from "node:test";
716
import { HEYGEN_NOT_AUTHENTICATED_MESSAGE } from "./heygen-cli.mjs";
817
import { AVATAR_VIDEO_SIGNIN_MESSAGE } from "./heygen-video-provider.mjs";
918

19+
function cleanupDownload(localPath) {
20+
if (
21+
basename(localPath) === "video.mp4" &&
22+
basename(dirname(localPath)).startsWith("media-use-heygen-video-")
23+
) {
24+
rmSync(dirname(localPath), { recursive: true, force: true });
25+
} else {
26+
rmSync(localPath, { force: true });
27+
}
28+
}
29+
1030
const VIDEO_FIXTURE = Buffer.from("tiny heygen video fixture");
1131
let importCount = 0;
1232

@@ -148,7 +168,10 @@ test("downloads a generated avatar video and returns the generated MP4 result",
148168
});
149169
assert.ok(result);
150170
assert.equal(join(tmpdir(), result.localPath.slice(tmpdir().length + 1)), result.localPath);
151-
assert.match(result.localPath, /media-use-heygen-video-\d+-\d+\.mp4$/);
171+
assert.match(result.localPath, /media-use-heygen-video-[^/\\]+[/\\]video\.mp4$/);
172+
if (process.platform !== "win32") {
173+
assert.equal(statSync(dirname(result.localPath)).mode & 0o777, 0o700);
174+
}
152175
assert.deepEqual(result, {
153176
localPath: result.localPath,
154177
ext: ".mp4",
@@ -163,7 +186,7 @@ test("downloads a generated avatar video and returns the generated MP4 result",
163186
},
164187
);
165188
} finally {
166-
if (localPath) rmSync(localPath, { force: true });
189+
if (localPath) cleanupDownload(localPath);
167190
await closeServer(server);
168191
}
169192
});
@@ -190,7 +213,7 @@ test("tags video creation but not avatar or voice discovery", async () => {
190213
},
191214
);
192215
} finally {
193-
if (localPath) rmSync(localPath, { force: true });
216+
if (localPath) cleanupDownload(localPath);
194217
await closeServer(server);
195218
}
196219
});
@@ -221,7 +244,7 @@ test("uses explicit avatar and voice overrides without discovery", async () => {
221244
},
222245
);
223246
} finally {
224-
if (localPath) rmSync(localPath, { force: true });
247+
if (localPath) cleanupDownload(localPath);
225248
await closeServer(server);
226249
}
227250
});
@@ -246,7 +269,7 @@ test("caches discovered avatar and voice IDs for the process", async () => {
246269
},
247270
);
248271
} finally {
249-
for (const localPath of localPaths) rmSync(localPath, { force: true });
272+
for (const localPath of localPaths) cleanupDownload(localPath);
250273
await closeServer(server);
251274
}
252275
});
@@ -342,3 +365,61 @@ test("download failure after a successful create returns null and logs a diagnos
342365
await closeServer(server);
343366
}
344367
});
368+
369+
test("uses private unique downloads even when time is fixed and the old name is planted", async (t) => {
370+
const root = mkdtempSync(join(tmpdir(), "hf-video-temp-test-"));
371+
const previousTmpdir = process.env.TMPDIR;
372+
process.env.TMPDIR = root;
373+
t.mock.method(Date, "now", () => 123456);
374+
t.mock.method(globalThis, "fetch", async () => ({
375+
ok: true,
376+
headers: { get: () => "4" },
377+
body: [Buffer.from("new!")],
378+
}));
379+
try {
380+
const victim = join(root, "victim");
381+
writeFileSync(victim, "unchanged");
382+
symlinkSync(victim, join(root, `media-use-heygen-video-${process.pid}-123456.mp4`));
383+
await withFakeHeygen(
384+
{ response: JSON.stringify({ data: { video_url: "https://example.invalid/video.mp4" } }) },
385+
async () => {
386+
const generate = await freshGenerate();
387+
const first = await generate("First", { avatarId: "a", voiceId: "v" });
388+
const second = await generate("Second", { avatarId: "a", voiceId: "v" });
389+
assert.ok(first && second);
390+
assert.notEqual(first.localPath, second.localPath);
391+
assert.equal(readFileSync(first.localPath, "utf8"), "new!");
392+
assert.equal(readFileSync(second.localPath, "utf8"), "new!");
393+
assert.equal(readFileSync(victim, "utf8"), "unchanged");
394+
},
395+
);
396+
} finally {
397+
if (previousTmpdir === undefined) delete process.env.TMPDIR;
398+
else process.env.TMPDIR = previousTmpdir;
399+
rmSync(root, { recursive: true, force: true });
400+
}
401+
});
402+
403+
test("removes private download staging on failure while returning null", async (t) => {
404+
const root = mkdtempSync(join(tmpdir(), "hf-video-temp-fail-"));
405+
const previousTmpdir = process.env.TMPDIR;
406+
process.env.TMPDIR = root;
407+
t.mock.method(globalThis, "fetch", async () => {
408+
throw new Error("download failed");
409+
});
410+
t.mock.method(console, "error", () => {});
411+
try {
412+
await withFakeHeygen(
413+
{ response: JSON.stringify({ data: { video_url: "https://example.invalid/video.mp4" } }) },
414+
async () => {
415+
const generate = await freshGenerate();
416+
assert.equal(await generate("Failure", { avatarId: "a", voiceId: "v" }), null);
417+
},
418+
);
419+
assert.deepEqual(readdirSync(root), []);
420+
} finally {
421+
if (previousTmpdir === undefined) delete process.env.TMPDIR;
422+
else process.env.TMPDIR = previousTmpdir;
423+
rmSync(root, { recursive: true, force: true });
424+
}
425+
});

‎skills/media-use/scripts/lib/lut-preset-provider.mjs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import { existsSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs";
2-
import { join } from "node:path";
1+
import { existsSync, mkdtempSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs";
2+
import { dirname, join } from "node:path";
33
import { withReservedFile, withReservedFileSync } from "./manifest.mjs";
44
import { freezeUrl } from "./freeze.mjs";
55
import { tokenOverlap } from "./match.mjs";
@@ -216,17 +216,18 @@ export async function freezeLibraryLut(match, { projectDir, type, localOnly = fa
216216
type,
217217
".cube",
218218
async ({ id, localPath, fullPath }) => {
219-
const tmpPath = `${fullPath}.tmp`;
219+
const tmpDir = mkdtempSync(join(dirname(fullPath), ".lut-download-"));
220+
const tmpPath = join(tmpDir, "download.cube");
220221
try {
221-
// Download + validate at a .tmp path, then atomically rename. A crash
222+
// Download + validate in a private directory, then atomically rename. A crash
222223
// (SIGKILL/OOM) between write and validate can't orphan an invalid .cube
223224
// at the final path — only a validated cube is ever renamed into place.
224225
await freezeUrl(match.url, tmpPath);
225226
assertValidCubeFile(tmpPath, `downloaded library LUT ${match.id} failed validation`);
226227
renameSync(tmpPath, fullPath);
227228
return libraryRecord(match, { id, localPath, fullPath, via: "url" });
228229
} finally {
229-
rmSync(tmpPath, { force: true });
230+
rmSync(tmpDir, { recursive: true, force: true });
230231
}
231232
},
232233
);

‎skills/media-use/scripts/lib/lut-preset-provider.test.mjs‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,14 @@
11
import { strict as assert } from "node:assert";
2-
import { mkdtempSync, rmSync, existsSync, readFileSync, readdirSync } from "node:fs";
2+
import {
3+
mkdtempSync,
4+
rmSync,
5+
existsSync,
6+
readFileSync,
7+
readdirSync,
8+
statSync,
9+
symlinkSync,
10+
writeFileSync,
11+
} from "node:fs";
312
import { join } from "node:path";
413
import { tmpdir } from "node:os";
514
import { test } from "node:test";
@@ -127,6 +136,15 @@ test("url library entries respect localOnly and freeze through fetch", async ()
127136
globalThis.fetch = async (url) => {
128137
fetchCalls++;
129138
assert.equal(url, match.url);
139+
const directory = join(projectDir, ".media/luts");
140+
const staging = readdirSync(directory).filter((name) => name.startsWith(".lut-download-"));
141+
assert.equal(staging.length, 1);
142+
if (process.platform !== "win32") {
143+
assert.equal(statSync(join(directory, staging[0])).mode & 0o777, 0o700);
144+
}
145+
const victim = join(projectDir, "victim.cube");
146+
writeFileSync(victim, "unchanged");
147+
symlinkSync(victim, join(directory, "lut_001.cube.tmp"));
130148
return {
131149
ok: true,
132150
headers: { get: () => String(body.length) },
@@ -139,6 +157,11 @@ test("url library entries respect localOnly and freeze through fetch", async ()
139157
assert.match(frozen.localPath, /^\.media\/luts\/lut_001\.cube$/);
140158
assert.equal(validateCubeFile(join(projectDir, frozen.localPath)).ok, true);
141159
assert.equal(frozen.metadata.provenance.via, "url");
160+
assert.equal(readFileSync(join(projectDir, "victim.cube"), "utf8"), "unchanged");
161+
assert.deepEqual(readdirSync(join(projectDir, ".media/luts")).sort(), [
162+
"lut_001.cube",
163+
"lut_001.cube.tmp",
164+
]);
142165
} finally {
143166
globalThis.fetch = originalFetch;
144167
rmSync(projectDir, { recursive: true, force: true });

0 commit comments

Comments
 (0)