diff --git a/docs/guides/media.mdx b/docs/guides/media.mdx index 71c33128e3b..e192cb660d2 100644 --- a/docs/guides/media.mdx +++ b/docs/guides/media.mdx @@ -53,6 +53,18 @@ make later revisions much easier for both people and agents. The original and prepared file should remain easy to distinguish. +## Remember confirmed preferences + +The media-use skill records confirmed choices in `.media/preferences.json`. +Concurrent records preserve each choice, including confirmations that promote a +preference to your personal defaults after use in two projects. + +Add `.media/*.lock` to the project's `.gitignore`, while keeping +`preferences.json` committed. If recording reports "Settings are locked…", +confirm that no HyperFrames process is recording preferences before deleting +the named lock file and retrying. A locked personal tier leaves the project +record intact and postpones personal promotion until a later record. + ## Check the final timeline Watch once from beginning to end and check: diff --git a/packages/cli/src/media-use/lib/prefs-store.concurrent.test.mjs b/packages/cli/src/media-use/lib/prefs-store.concurrent.test.mjs new file mode 100644 index 00000000000..8a272459ffc --- /dev/null +++ b/packages/cli/src/media-use/lib/prefs-store.concurrent.test.mjs @@ -0,0 +1,173 @@ +import assert from "node:assert/strict"; +import { fork } from "node:child_process"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + utimesSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { test } from "node:test"; +import { fileURLToPath } from "node:url"; +import { projectPrefsPath, recordPreference, userPrefsPath } from "./prefs-store.mjs"; + +async function inSandbox(task) { + const root = mkdtempSync(join(tmpdir(), "mu-prefs-concurrent-")); + const previousHome = process.env.HYPERFRAMES_MEDIA_HOME; + process.env.HYPERFRAMES_MEDIA_HOME = join(root, "home"); + const project = join(root, "project"); + mkdirSync(project); + try { + await task({ root, project }); + } finally { + if (previousHome === undefined) delete process.env.HYPERFRAMES_MEDIA_HOME; + else process.env.HYPERFRAMES_MEDIA_HOME = previousHome; + rmSync(root, { recursive: true, force: true }); + } +} + +function worker(root, source, args = []) { + const path = join(mkdtempSync(join(root, "worker-")), "worker.cjs"); + writeFileSync(path, source); + const child = fork(path, args, { silent: true, execArgv: [] }); + let stderr = ""; + child.stderr.setEncoding("utf8").on("data", (data) => (stderr += data)); + const ready = new Promise((resolve, reject) => { + child.once("message", resolve); + child.once("error", reject); + child.once("exit", () => reject(new Error(`worker exited before ready: ${stderr}`))); + }); + const closed = new Promise((resolve, reject) => { + child.once("error", reject); + child.once("close", (code) => resolve({ code, stderr })); + }); + return { child, ready, closed }; +} + +for (const tier of ["project", "user"]) { + test(`record re-reads the ${tier} tier after another process releases its lock`, async () => { + await inSandbox(async ({ root, project }) => { + const path = tier === "project" ? projectPrefsPath(project) : userPrefsPath(); + mkdirSync(dirname(path), { recursive: true }); + const held = worker( + root, + `const fs = require("node:fs"); + const path = process.argv[2]; + fs.writeFileSync(path + ".lock", "held", { flag: "wx" }); + process.send("locked"); + setTimeout(() => { + fs.writeFileSync(path, JSON.stringify({version: 1, + preferences: {aspect: {value: "1080x1920", confirmed_in: ["other"]}}, + sightings: {aspect: {"1080x1920": ["other"]}}})); + fs.rmSync(path + ".lock"); + process.disconnect(); + }, 300);`, + [path], + ); + try { + await held.ready; + recordPreference({ projectDir: project, key: "destination", value: "youtube-shorts" }); + const result = await held.closed; + assert.equal(result.code, 0, result.stderr); + const file = JSON.parse(readFileSync(path, "utf8")); + assert.equal(file.preferences.aspect?.value, "1080x1920"); + if (tier === "project") { + assert.equal(file.preferences.destination?.value, "youtube-shorts"); + } else { + assert.deepEqual(file.sightings.destination?.["youtube-shorts"], ["project"]); + assert.deepEqual(file.sightings.aspect?.["1080x1920"], ["other"]); + } + } finally { + held.child.kill(); + await held.closed; + } + }); + }); +} + +test("parallel records preserve every project key and promote every shared user sighting", async () => { + await inSandbox(async ({ root, project }) => { + const secondProject = join(root, "second-project"); + mkdirSync(secondProject); + const modulePath = fileURLToPath(new URL("./prefs-store.mjs", import.meta.url)); + const choices = { + destination: "youtube-shorts", + aspect: "1080x1920", + language: "en", + flow: "automation", + }; + const children = [project, secondProject].flatMap((projectDir) => + Object.entries(choices).map(([key, value]) => + worker( + root, + `const {pathToFileURL} = require("node:url"); + import(pathToFileURL(process.argv[2])).then(({recordPreference}) => { + process.send("ready"); + process.once("message", () => { + try {recordPreference(JSON.parse(process.argv[3]));} + catch (error) {console.error(error.message); process.exitCode = 1;} + process.disconnect(); + }); + });`, + [modulePath, JSON.stringify({ projectDir, key, value })], + ), + ), + ); + try { + await Promise.all(children.map((child) => child.ready)); + for (const child of children) child.child.send("record"); + const results = await Promise.all(children.map((child) => child.closed)); + for (const result of results) assert.equal(result.code, 0, result.stderr); + for (const projectDir of [project, secondProject]) { + const file = JSON.parse(readFileSync(projectPrefsPath(projectDir), "utf8")); + assert.deepEqual(Object.keys(file.preferences).sort(), Object.keys(choices).sort()); + for (const [key, value] of Object.entries(choices)) + assert.equal(file.preferences[key].value, value); + assert.deepEqual(readdirSync(dirname(projectPrefsPath(projectDir))), ["preferences.json"]); + } + const user = JSON.parse(readFileSync(userPrefsPath(), "utf8")); + for (const [key, value] of Object.entries(choices)) { + assert.equal(user.preferences[key].value, value); + assert.deepEqual(user.sightings[key][value].sort(), ["project", "second-project"]); + } + assert.deepEqual(readdirSync(dirname(userPrefsPath())), ["preferences.json"]); + } finally { + for (const child of children) child.child.kill(); + await Promise.all(children.map((child) => child.closed)); + } + }); +}); + +for (const tier of ["project", "user"]) { + test(`an abandoned ${tier} lock never permits an unlocked preference write`, async () => { + await inSandbox(async ({ project }) => { + const path = tier === "project" ? projectPrefsPath(project) : userPrefsPath(); + mkdirSync(dirname(path), { recursive: true }); + const original = JSON.stringify({ version: 1, preferences: {}, sightings: {} }); + writeFileSync(path, original); + const lock = `${path}.lock`; + writeFileSync(lock, "abandoned"); + const past = new Date(Date.now() - 60_000); + utimesSync(lock, past, past); + const record = () => recordPreference({ projectDir: project, key: "language", value: "en" }); + if (tier === "project") { + assert.throws(record, (error) => error.code === "HF_SETTINGS_LOCKED"); + assert.equal(existsSync(userPrefsPath()), false); + } else { + assert.equal(record().promoted, false); + assert.equal( + JSON.parse(readFileSync(projectPrefsPath(project), "utf8")).preferences.language.value, + "en", + ); + } + assert.equal(readFileSync(path, "utf8"), original); + assert.equal(readFileSync(lock, "utf8"), "abandoned"); + assert.equal(existsSync(`${path}.${process.pid}.tmp`), false); + }); + }); +} diff --git a/packages/cli/src/media-use/lib/prefs-store.mjs b/packages/cli/src/media-use/lib/prefs-store.mjs index d28893fea03..36f87d9edab 100644 --- a/packages/cli/src/media-use/lib/prefs-store.mjs +++ b/packages/cli/src/media-use/lib/prefs-store.mjs @@ -1,5 +1,7 @@ import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import * as fs from "node:fs"; import { basename, dirname, join, resolve } from "node:path"; +import { withFileLock } from "./config-lock.mjs"; import { globalMediaDir } from "./media-home.mjs"; /** @@ -70,10 +72,25 @@ function readPrefsFile(path) { /** Atomic write (tmp + rename) so a crash never leaves a torn file. */ function writePrefsFile(path, file) { + const tmp = `${path}.${process.pid}.tmp`; + try { + writeFileSync(tmp, `${JSON.stringify(file, null, 2)}\n`); + renameSync(tmp, path); + } finally { + try { + fs.rmSync(tmp, { force: true }); + } catch {} + } +} + +function updatePrefsFile(path, update) { mkdirSync(dirname(path), { recursive: true }); - const tmp = `${path}.tmp`; - writeFileSync(tmp, `${JSON.stringify(file, null, 2)}\n`); - renameSync(tmp, path); + return withFileLock(`${path}.lock`, fs, () => { + const file = readPrefsFile(path); + const result = update(file); + writePrefsFile(path, file); + return result; + }); } /** `style_preset` entries are stored per workflow as `style_preset.`. */ @@ -112,16 +129,18 @@ function dedupe(list) { * provenance over (the old confirmations vouched for the old value). */ function recordProjectTier(projectDir, fullKey, value, projectName, now) { - const path = projectPrefsPath(projectDir); - const file = readPrefsFile(path); - const previous = file.preferences[fullKey]; - const keepProvenance = validEntry(previous) && previous.value === value; - const confirmedIn = keepProvenance - ? dedupe([...(Array.isArray(previous.confirmed_in) ? previous.confirmed_in : []), projectName]) - : [projectName]; - file.preferences[fullKey] = { value, confirmed_in: confirmedIn, updated_at: now }; - writePrefsFile(path, file); - return confirmedIn; + return updatePrefsFile(projectPrefsPath(projectDir), (file) => { + const previous = file.preferences[fullKey]; + const keepProvenance = validEntry(previous) && previous.value === value; + const confirmedIn = keepProvenance + ? dedupe([ + ...(Array.isArray(previous.confirmed_in) ? previous.confirmed_in : []), + projectName, + ]) + : [projectName]; + file.preferences[fullKey] = { value, confirmed_in: confirmedIn, updated_at: now }; + return confirmedIn; + }); } /** @@ -129,21 +148,20 @@ function recordProjectTier(projectDir, fullKey, value, projectName, now) { * the same value has been confirmed in PROMOTE_AT distinct projects. */ function recordUserSighting(fullKey, value, projectName, now) { - const path = userPrefsPath(); - const file = readPrefsFile(path); - const keySightings = isRecord(file.sightings[fullKey]) ? file.sightings[fullKey] : {}; - const seenIn = dedupe([ - ...(Array.isArray(keySightings[value]) ? keySightings[value] : []), - projectName, - ]); - keySightings[value] = seenIn; - file.sightings[fullKey] = keySightings; - const promoted = seenIn.length >= PROMOTE_AT; - if (promoted) { - file.preferences[fullKey] = { value, confirmed_in: seenIn, updated_at: now }; - } - writePrefsFile(path, file); - return promoted; + return updatePrefsFile(userPrefsPath(), (file) => { + const keySightings = isRecord(file.sightings[fullKey]) ? file.sightings[fullKey] : {}; + const seenIn = dedupe([ + ...(Array.isArray(keySightings[value]) ? keySightings[value] : []), + projectName, + ]); + keySightings[value] = seenIn; + file.sightings[fullKey] = keySightings; + const promoted = seenIn.length >= PROMOTE_AT; + if (promoted) { + file.preferences[fullKey] = { value, confirmed_in: seenIn, updated_at: now }; + } + return promoted; + }); } /** diff --git a/packages/cli/src/media-use/lib/prefs-store.test.mjs b/packages/cli/src/media-use/lib/prefs-store.test.mjs index 4c13e5fa24c..de6be127ec4 100644 --- a/packages/cli/src/media-use/lib/prefs-store.test.mjs +++ b/packages/cli/src/media-use/lib/prefs-store.test.mjs @@ -1,6 +1,8 @@ import { strict as assert } from "node:assert"; import { test } from "node:test"; import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import fs from "node:fs"; +import { syncBuiltinESMExports } from "node:module"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -39,6 +41,60 @@ function withSandbox(fn) { } } +for (const code of ["EPERM", "EBUSY"]) { + test(`temporary-file cleanup ${code} does not fail a successful preference record`, (t) => { + const remove = fs.rmSync; + t.mock.method(fs, "rmSync", (path, options) => { + if (path.endsWith(".tmp")) throw Object.assign(new Error("cleanup failed"), { code }); + return remove(path, options); + }); + syncBuiltinESMExports(); + t.after(() => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + }); + + withSandbox(({ projectA, projectB }) => { + recordPreference({ projectDir: projectA, key: "language", value: "en" }); + const second = recordPreference({ projectDir: projectB, key: "language", value: "en" }); + assert.equal(second.promoted, true); + for (const path of [ + projectPrefsPath(projectA), + projectPrefsPath(projectB), + userPrefsPath(), + ]) { + assert.equal(JSON.parse(readFileSync(path, "utf8")).preferences.language.value, "en"); + } + }); + }); + + test(`temporary-file cleanup ${code} preserves the original preference write error`, (t) => { + const remove = fs.rmSync; + const write = fs.writeFileSync; + const writeError = Object.assign(new Error("disk full"), { code: "ENOSPC" }); + t.mock.method(fs, "rmSync", (path, options) => { + if (path.endsWith(".tmp")) throw Object.assign(new Error("cleanup failed"), { code }); + return remove(path, options); + }); + t.mock.method(fs, "writeFileSync", (path, ...args) => { + if (path.endsWith(".tmp")) throw writeError; + return write(path, ...args); + }); + syncBuiltinESMExports(); + t.after(() => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + }); + + withSandbox(({ projectA }) => { + assert.throws( + () => recordPreference({ projectDir: projectA, key: "language", value: "en" }), + (error) => error === writeError, + ); + }); + }); +} + test("record writes the project tier and get merges it as source=project", () => { withSandbox(({ projectA }) => { const result = recordPreference({ projectDir: projectA, key: "destination", value: "x-feed" }); diff --git a/skills-manifest.json b/skills-manifest.json index 4bc1461bb5e..42c5e550b5c 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -18,7 +18,7 @@ "files": 13 }, "media-use": { - "hash": "523f018498ca8fdd", + "hash": "7603d1411b14d829", "files": 116 }, "remotion-to-hyperframes": { diff --git a/skills/media-use/references/memory.md b/skills/media-use/references/memory.md index 0ff3fcd64d3..9c749f87dca 100644 --- a/skills/media-use/references/memory.md +++ b/skills/media-use/references/memory.md @@ -12,6 +12,10 @@ node /scripts/prefs.mjs record --hyperframes . --key style_preset --v Only what the user actually confirmed gets recorded — never an inferred or defaulted value. A remembered value becomes the recommended default, shown with where it came from; it never skips a question. +Preference records serialize writes with `preferences.json.lock` beside each tier's file. If a process stops while holding that lock, later records leave it in place and report "Settings are locked…". After confirming no HyperFrames process is recording preferences, delete the named lock file and retry. A locked personal tier does not fail the project record; personal promotion waits for a later record. + +Keep the project's preference file committed, and add `.media/*.lock` to the project's `.gitignore` so transient locks are never shared with the team. + ## Recipes — frozen video bundles The heavyweight tier of user memory: one approved run frozen as a named, versioned bundle — `frame.md`, the storyboard skeleton (structure kept, content blanked to per-frame fill-ins), the brief skeleton (from `BRIEF.md` when the project has one — reusable frontmatter kept, run-shape and prose blanked), and the confirmed brief values. Same two tiers: project `.media/recipes//` (committed) and `~/.media/recipes//` (a freeze is already a confirmed bundle, so it promotes immediately — no two-project rule). Re-freezing a name bumps `version` and archives the old folder as `@v`. diff --git a/skills/media-use/scripts/lib/prefs-store.mjs b/skills/media-use/scripts/lib/prefs-store.mjs index d28893fea03..36f87d9edab 100644 --- a/skills/media-use/scripts/lib/prefs-store.mjs +++ b/skills/media-use/scripts/lib/prefs-store.mjs @@ -1,5 +1,7 @@ import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import * as fs from "node:fs"; import { basename, dirname, join, resolve } from "node:path"; +import { withFileLock } from "./config-lock.mjs"; import { globalMediaDir } from "./media-home.mjs"; /** @@ -70,10 +72,25 @@ function readPrefsFile(path) { /** Atomic write (tmp + rename) so a crash never leaves a torn file. */ function writePrefsFile(path, file) { + const tmp = `${path}.${process.pid}.tmp`; + try { + writeFileSync(tmp, `${JSON.stringify(file, null, 2)}\n`); + renameSync(tmp, path); + } finally { + try { + fs.rmSync(tmp, { force: true }); + } catch {} + } +} + +function updatePrefsFile(path, update) { mkdirSync(dirname(path), { recursive: true }); - const tmp = `${path}.tmp`; - writeFileSync(tmp, `${JSON.stringify(file, null, 2)}\n`); - renameSync(tmp, path); + return withFileLock(`${path}.lock`, fs, () => { + const file = readPrefsFile(path); + const result = update(file); + writePrefsFile(path, file); + return result; + }); } /** `style_preset` entries are stored per workflow as `style_preset.`. */ @@ -112,16 +129,18 @@ function dedupe(list) { * provenance over (the old confirmations vouched for the old value). */ function recordProjectTier(projectDir, fullKey, value, projectName, now) { - const path = projectPrefsPath(projectDir); - const file = readPrefsFile(path); - const previous = file.preferences[fullKey]; - const keepProvenance = validEntry(previous) && previous.value === value; - const confirmedIn = keepProvenance - ? dedupe([...(Array.isArray(previous.confirmed_in) ? previous.confirmed_in : []), projectName]) - : [projectName]; - file.preferences[fullKey] = { value, confirmed_in: confirmedIn, updated_at: now }; - writePrefsFile(path, file); - return confirmedIn; + return updatePrefsFile(projectPrefsPath(projectDir), (file) => { + const previous = file.preferences[fullKey]; + const keepProvenance = validEntry(previous) && previous.value === value; + const confirmedIn = keepProvenance + ? dedupe([ + ...(Array.isArray(previous.confirmed_in) ? previous.confirmed_in : []), + projectName, + ]) + : [projectName]; + file.preferences[fullKey] = { value, confirmed_in: confirmedIn, updated_at: now }; + return confirmedIn; + }); } /** @@ -129,21 +148,20 @@ function recordProjectTier(projectDir, fullKey, value, projectName, now) { * the same value has been confirmed in PROMOTE_AT distinct projects. */ function recordUserSighting(fullKey, value, projectName, now) { - const path = userPrefsPath(); - const file = readPrefsFile(path); - const keySightings = isRecord(file.sightings[fullKey]) ? file.sightings[fullKey] : {}; - const seenIn = dedupe([ - ...(Array.isArray(keySightings[value]) ? keySightings[value] : []), - projectName, - ]); - keySightings[value] = seenIn; - file.sightings[fullKey] = keySightings; - const promoted = seenIn.length >= PROMOTE_AT; - if (promoted) { - file.preferences[fullKey] = { value, confirmed_in: seenIn, updated_at: now }; - } - writePrefsFile(path, file); - return promoted; + return updatePrefsFile(userPrefsPath(), (file) => { + const keySightings = isRecord(file.sightings[fullKey]) ? file.sightings[fullKey] : {}; + const seenIn = dedupe([ + ...(Array.isArray(keySightings[value]) ? keySightings[value] : []), + projectName, + ]); + keySightings[value] = seenIn; + file.sightings[fullKey] = keySightings; + const promoted = seenIn.length >= PROMOTE_AT; + if (promoted) { + file.preferences[fullKey] = { value, confirmed_in: seenIn, updated_at: now }; + } + return promoted; + }); } /**